Support nested files and folders for Skills#2719
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Medium urgency. Solid feature implementation — schema, DAL, routes, SDK, and tests all align well. A few issues need attention: an as any cast that silently swallows the update schema pipe, sequential DB updates inside replaceSkillFiles that could be batched, and a SkillFrontmatterSchema behavior change from z.object to z.looseObject that accepts unknown keys without explicit acknowledgement.
| const SkillApiUpdateSchema = SkillUpdateSchema.transform((skill) => { | ||
| const skillFile = skill.files?.find((skill) => skill.filePath === SKILL_ENTRY_FILE_PATH); | ||
| if (!skillFile) { | ||
| return { files: [] } as any; |
There was a problem hiding this comment.
The as any cast here silently bypasses the .pipe() type check. When SKILL.md is absent from the update payload, this returns { files: [] } which doesn't match the pipe target schema shape (it's missing name, description, content). While .partial() makes those optional, the as any mask makes it impossible for TypeScript to catch future regressions here.
Consider returning a properly typed partial object instead:
return { files: skill.files } satisfies { files: ... };Or narrow the pipe target to avoid needing the cast.
| .transform((skill) => { | ||
| const skillFile = skill.files.find((skill) => skill.filePath === SKILL_ENTRY_FILE_PATH); | ||
| if (!skillFile) { | ||
| throw new Error('should never happens'); |
There was a problem hiding this comment.
Typo: 'should never happens' → 'should never happen'. Also, since SkillFilesInputSchema already validates that SKILL_ENTRY_FILE_PATH is present, this branch is indeed unreachable — but consider using a Zod issue instead of throwing a raw Error to stay consistent with the validation pipeline.
|
|
||
| const SkillIndexSchema = z.int().min(0); | ||
|
|
||
| const SkillFrontmatterSchema = z.looseObject({ |
There was a problem hiding this comment.
z.looseObject is a deliberate behavior change from the old z.object — it now accepts arbitrary extra keys in the frontmatter without stripping them. This is reasonable (allowing custom frontmatter), but the extra keys will silently flow into transformSkill's return value and potentially into SkillCreateDataSchema via the spread. Confirm the .pipe(SkillCreateDataSchema) with z.strictObject catches any unexpected keys downstream.
| await db | ||
| .update(skillFiles) | ||
| .set({ | ||
| content: file.content, | ||
| updatedAt: now, | ||
| }) | ||
| .where( | ||
| and( | ||
| projectScopedWhere(skillFiles, params.scopes), | ||
| eq(skillFiles.skillId, params.skillId), | ||
| eq(skillFiles.id, existingFile.id) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Each existing file that changed content is updated with a separate UPDATE query inside the loop. For skills with many files this creates N sequential round-trips within the transaction. Consider batching updates — e.g. using a single sql call with a CASE expression or at minimum using Promise.all for the updates (they're independent and already inside a transaction).
| if (content !== undefined) updateData.content = content; | ||
|
|
||
| if (!params.data.files) { | ||
| throw new Error('Skill updates must include files'); |
There was a problem hiding this comment.
This throw makes files mandatory for every updateSkill call, but the SkillApiUpdate type (from the schema) already has files as required. The runtime check is defensive, but it means any internal caller that passes data without files gets a cryptic 'Skill updates must include files' error instead of a type error. If this invariant is truly required, enforce it at the type level rather than at runtime.
| ); | ||
|
|
||
| const skillPromises = Object.entries(typed.skills).map(async ([skillId, skill]) => { | ||
| for (const [skillId, skill] of Object.entries(typed.skills)) { |
There was a problem hiding this comment.
The Promise.all → sequential for...of change removes parallelism for skill upserts during full project create/update. This is correct because upsertSkill now uses transactions internally and Doltgres may not handle concurrent transactions well, but worth a brief comment explaining why this was serialized.
| @@ -0,0 +1,25 @@ | |||
| import type { SkillInsert } from '@inkeep/agents-core'; | |||
There was a problem hiding this comment.
import type { SkillInsert } from '@inkeep/agents-core' — this is a self-referential package import. While it may work with the monorepo setup, it's fragile. Other files in agents-core use relative imports for internal types (e.g. ../../types/entities). Consider using a relative import for consistency.
| files: SkillFilesInputSchema, | ||
| }) | ||
| .transform((skill) => { | ||
| const skillFile = skill.files.find((skill) => skill.filePath === SKILL_ENTRY_FILE_PATH); |
There was a problem hiding this comment.
The inner .find() callback shadows the outer skill parameter name: .find((skill) => skill.filePath === ...). Use a different name (e.g. file) to avoid confusion.
| const skillFile = skill.files.find((skill) => skill.filePath === SKILL_ENTRY_FILE_PATH); | |
| const skillFile = skill.files.find((file) => file.filePath === SKILL_ENTRY_FILE_PATH); |
| .openapi('SkillCreate'); | ||
|
|
||
| const SkillApiUpdateSchema = SkillUpdateSchema.transform((skill) => { | ||
| const skillFile = skill.files?.find((skill) => skill.filePath === SKILL_ENTRY_FILE_PATH); |
There was a problem hiding this comment.
Same shadowed variable name issue here — the .find() callback parameter skill shadows the outer skill.
| const skillFile = skill.files?.find((skill) => skill.filePath === SKILL_ENTRY_FILE_PATH); | |
| const skillFile = skill.files?.find((file) => file.filePath === SKILL_ENTRY_FILE_PATH); |
| message: | ||
| env.ENVIRONMENT === 'development' && error instanceof Error | ||
| ? error.message | ||
| : 'Failed to update project', |
There was a problem hiding this comment.
Leaking internal error messages in development mode is useful for debugging but make sure error.message can't contain sensitive data (e.g. SQL query fragments, connection strings). Drizzle errors can sometimes include query text.
|
TL;DR — Evolves Skills from single-file entities into multi-file directory structures. A new Key changes
Summary ｜ 94 files ｜ 2894 commits ｜ base: Database schema:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(12) Total Issues | Risk: High
🔴❗ Critical (2) ❗🔴
🔴 1) packages/agents-core/src/validation/schemas/skills.ts:120-178 Breaking API contracts for Skill create/update
files:
packages/agents-core/src/validation/schemas/skills.tsagents-api/__snapshots__/openapi.json
Issue: This PR introduces breaking changes to the Skills API contract:
- SkillApiInsertSchema (create): Changed from
{ name, description, content, metadata }to{ files: [...] }where files must include SKILL.md - SkillApiUpdateSchema (update): Changed from optional
{ description?, metadata?, content? }to required{ files: [...] }
The name, description, and content fields are now extracted from the SKILL.md frontmatter rather than accepted as top-level fields.
Why: Existing API clients sending POST /skills or PATCH /skills/{id} with the old format will fail validation. The OpenAPI snapshot confirms SkillCreate.required: ["files"] and SkillUpdate.required: ["files"]. This breaks:
- Generated SDK clients (Speakeasy)
- Any external integrations using the Skills API
- CLI
pullcommands that may reconstruct skills
Fix: Consider one of:
- Accept both formats during a migration period (detect old vs new format by checking for
filesvsnamekeys) - Version the API endpoint (
/v2/skills) for the new format - If intentional breaking change, document in changelog, create migration guide, and consider a major/minor version bump
Refs:
🔴 2) agents-manage-ui/src/app/[tenantId]/projects/[projectId]/skills/skills-data.ts:12-14 Waterfall fetch pattern causes N+1 network requests
Issue: The loadSkillsData function fetches the skill list, then sequentially fetches each skill's details in parallel using Promise.all. While Promise.all parallelizes the detail fetches, this still creates a waterfall:
const { data } = await fetchSkills(tenantId, projectId); // Wait for list
const skillDetails = await Promise.all(
data.map((skill) => fetchSkill(tenantId, projectId, skill.id)) // Then N detail fetches
);Why: For a project with 10 skills, this creates 11 requests (1 list + 10 details) with the initial list fetch blocking all detail fetches. This degrades page load performance significantly as skill count grows.
Fix: Consider adding a backend batch endpoint that returns skills with their files in a single request. Alternatively:
// Option 1: Backend batch endpoint (recommended)
const { data } = await fetchSkillsWithFiles(tenantId, projectId);
// Option 2: If list endpoint can return file counts, use streaming
// Show skeleton from list, then Suspense boundary for detailsRefs:
🟠⚠️ Major (6) 🟠⚠️
🟠 1) packages/agents-core/src/validation/schemas/skills.ts:160-163 Silent data loss when SKILL.md missing from update
Issue: The SkillApiUpdateSchema transform returns { files: [] } as any when SKILL.md is not found in the update payload. Combined with the route handler logic at skills.ts:311-313, this causes all files to be deleted instead of returning a validation error.
Why: A client might reasonably expect to update just metadata or a single auxiliary file, but instead all files are silently removed. The as any cast also hides type safety issues.
Fix: Either:
- Throw a validation error when SKILL.md is missing from updates that include files
- Separate "update single file" from "replace all files" operations semantically
- Make
filesoptional in updates - when undefined, preserve existing files; when empty array, explicitly delete
Refs:
🟠 2) agents-manage-ui/src/components/skills/delete-skill-confirmation.tsx:38-40 Missing router.refresh() after skill deletion
Issue: When redirectOnDelete is true, the redirect navigates to /skills but without router.refresh(), the deleted skill may still appear in the sidebar tree due to React's cached data.
Why: Users may see the deleted skill in the sidebar until manual page refresh. The peer component DeleteSkillFileConfirmation correctly calls router.refresh() after its redirect.
Fix: Add router.refresh() after the redirect:
if (redirectOnDelete) {
router.push(`/${tenantId}/projects/${projectId}/skills`);
router.refresh();
} else {
router.refresh();
}Refs:
🟠 3) agents-manage-ui/src/components/skills/skill-file-editor.tsx:257-261 Race condition in create mode navigation
Issue: router.push() followed by router.refresh() in create mode may not reliably navigate to the new file. The refresh could complete before navigation processes.
if (isCreateMode) {
router.push(buildSkillFileViewHref(tenantId, projectId, skillId, savedFilePath));
router.refresh();
didSave = true;
return;
}Why: Users may not see navigation complete, staying on the create form or seeing stale data after creating a nested file.
Fix: Rely on server-side revalidation via revalidatePath() which already exists in createSkillFileAction. Remove the client-side router.refresh():
if (isCreateMode) {
router.push(buildSkillFileViewHref(tenantId, projectId, skillId, savedFilePath));
didSave = true;
return;
}Refs:
🟠 4) agents-manage-ui/src/components/skills/skill-file-editor.tsx:376 Mobile create-file save can trigger unsaved-leave dialog
Issue: After successful save in create mode, the form is not reset before navigation, so isDirty remains true while navigation is pending.
Why: Mobile users may see the unsaved changes dialog appear immediately after saving a new file, blocking their navigation.
Fix: Reset the form's dirty state before navigation:
if (isCreateMode) {
form.reset({ filePath: '', content: '', extension: '.md' });
router.push(buildSkillFileViewHref(tenantId, projectId, skillId, savedFilePath));
didSave = true;
return;
}Refs:
🟠 5) packages/agents-core/src/__tests__/data-access/skills.test.ts Missing critical test coverage for entry file protection
scope: Test file coverage gaps
Issue: Several critical protection paths are untested:
createSkillFileByIdthrows when attempting to create SKILL.md (line 209-210)createSkillFileByIdthrows on duplicate file paths (line 213-214)deleteSkillFileByIdthrows when attempting to delete SKILL.md (line 335-336)
Why: These guards prevent data corruption. If they regress:
- Creating duplicate SKILL.md could corrupt skill definitions
- Duplicate file paths cause undefined UI behavior
- Deleting SKILL.md orphans nested files and makes skills unloadable
Fix: Add test cases:
it('rejects creating SKILL.md through file creation API', async () => {
await expect(createSkillFileById(db)({
scopes, skillId, data: { filePath: 'SKILL.md', content: '...' }
})).rejects.toThrow('Use the skill update flow to manage SKILL.md');
});
it('rejects creating duplicate file paths', async () => {
await createSkillFileById(db)({ scopes, skillId, data: { filePath: 'a.md', content: 'x' } });
await expect(createSkillFileById(db)({
scopes, skillId, data: { filePath: 'a.md', content: 'y' }
})).rejects.toThrow('Skill file already exists at path');
});
it('rejects deleting SKILL.md through file deletion API', async () => {
const skill = await getSkillByIdWithFiles(db)({ scopes, skillId });
const entryFile = skill.files.find(f => f.filePath === 'SKILL.md');
await expect(deleteSkillFileById(db)({
scopes, skillId, fileId: entryFile.id
})).rejects.toThrow('Use the skill delete flow to remove SKILL.md');
});Refs:
🟠 6) agents-manage-ui/src/lib/utils/skill-files.ts:110-134 URL encoding mismatch causes false 404s for files with special characters
Issue: resolveSkillFileFromRoute compares decoded route tokens (Next.js auto-decodes URL params) against stored paths. When file paths contain special characters like spaces, the encoding/decoding flow becomes inconsistent.
Why: Files with spaces (e.g., 'templates/day plans/card.html') may fail to resolve, showing 404 when navigating via direct URL vs. clicking in the tree.
Fix: Normalize both sides of the comparison:
const decodedToken = decodeURIComponent(routeToken);
return (
files.find((file) => file.routePath === decodedToken) ??
files.find((file) => file.treePath === decodedToken) ??
null
);Also add test case with special characters in file paths.
Refs:
🟡 Minor (4) 🟡
🟡 1) agents-manage-ui/src/components/skills/tree-node.tsx:177-182 Missing accessible name for expand/collapse button
Issue: The chevron toggle button contains only an icon with no aria-label, making it inaccessible to screen reader users.
Why: Violates WCAG 2.1 Success Criterion 4.1.2 (Name, Role, Value).
Fix:
<SidebarMenuAction
className={cn('top-1', !isCollapsed && 'rotate-90')}
onClick={handleCollapse}
aria-label={isCollapsed ? 'Expand folder' : 'Collapse folder'}
aria-expanded={!isCollapsed}
>
<ChevronRight className="size-4" />
</SidebarMenuAction>Refs:
Inline Comments:
- 🟡 Minor:
skills.ts:149Typo 'should never happens' - 🟡 Minor:
skill-files.ts:21Typo 'whcih' in TODO comment - 🟡 Minor:
skill-file-editor.tsx:267Empty catch block silently swallows errors
💭 Consider (2) 💭
💭 1) packages/agents-core/src/validation/schemas/skills.ts:16 z.looseObject() vs z.object() for SkillFrontmatterSchema
Issue: Most API schemas use z.object() but SkillFrontmatterSchema uses z.looseObject().
Why: May be intentional for forward-compatibility with custom frontmatter fields.
Fix: Document the design decision if intentional, or switch to z.object() for consistency.
💭 2) agents-api/src/domains/manage/routes/skills.ts:37 TenantProjectSkillFileParamsSchema defined inline
Issue: Peer routes import param schemas from @inkeep/agents-core, but this schema is defined locally.
Why: Creates slight inconsistency; affects reusability.
Fix: Consider moving to packages/agents-core/src/validation/schemas/skills.ts for consistency with peer patterns.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
skills.ts:532 |
DAL throws when files undefined | Duplicate of Major #1 - same root cause, covered by schema/route handler analysis |
skills.ts:88 |
N+1 in replaceSkillFiles loop | Low impact - operates within single transaction, file counts typically small (<20), database-level optimization |
skill-file-editor.tsx:354 |
Delete dialog rendered inside button | Radix Dialog uses portals correctly; pattern is unconventional but functional |
skill-files.ts:93 |
Missing cache() wrapper for fetchSkillFile | No such function exists; skill files accessed via fetchSkill which has cache() |
| Migration 0013 | New skill_files table | Additive schema change (non-breaking), well-structured with proper FK/cascade |
🚫 REQUEST CHANGES
Summary: This PR introduces valuable nested file/folder support for Skills, but has two critical issues requiring resolution before merge:
-
Breaking API contracts - The Skill create/update schemas changed incompatibly. Existing SDK clients and integrations will fail. Either provide backward compatibility or document as an intentional breaking change with migration guidance.
-
N+1 fetch waterfall - The skills page data loading creates O(n) network requests. Consider a batch endpoint for scalability.
Additionally, the silent data loss when SKILL.md is missing from updates (Major #1) and missing test coverage for critical protection paths (Major #5) should be addressed to ensure data integrity.
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-breaking-changes |
5 | 2 | 0 | 0 | 0 | 0 | 2 |
pr-review-standards |
6 | 2 | 0 | 0 | 0 | 0 | 2 |
pr-review-frontend |
8 | 3 | 0 | 0 | 1 | 0 | 1 |
pr-review-tests |
9 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
6 | 1 | 2 | 0 | 2 | 0 | 0 |
| Total | 34 | 9 | 2 | 0 | 3 | 0 | 5 |
| .transform((skill) => { | ||
| const skillFile = skill.files.find((skill) => skill.filePath === SKILL_ENTRY_FILE_PATH); | ||
| if (!skillFile) { | ||
| throw new Error('should never happens'); |
There was a problem hiding this comment.
🟡 Minor: Typo in error message
Issue: Error message contains grammatical error 'should never happens'.
Why: While defensive code, this message appears in production bundles and could surface in logs or error reports.
Fix: (1-click apply)
| throw new Error('should never happens'); | |
| throw new Error('should never happen'); |
| revalidatePath(buildSkillFileViewHref(tenantId, projectId, skillId, filePath)); | ||
| revalidatePath(buildSkillFileViewHref(tenantId, projectId, skillId, SKILL_ENTRY_FILE_PATH)); | ||
| } | ||
| // TODO remove all validation, whcih are done in backend |
There was a problem hiding this comment.
🟡 Minor: Typo in TODO comment
Issue: Comment contains typo 'whcih' instead of 'which'.
Why: Minor code quality issue that's inconsistent with the codebase's standards.
Fix: (1-click apply)
| // TODO remove all validation, whcih are done in backend | |
| // TODO remove all validation, which is done in backend |
| form.reset({ filePath, content }); | ||
| router.refresh(); | ||
| didSave = true; | ||
| } catch {} |
There was a problem hiding this comment.
🟡 Minor: Empty catch block silently swallows errors
Issue: This empty catch block will silently ignore unexpected errors from network failures or API issues.
Why: Makes debugging difficult when unexpected errors occur - no logging or user feedback.
Fix:
| } catch {} | |
| } catch (error) { | |
| console.error('Failed to save skill file:', error); | |
| toast.error('An unexpected error occurred while saving'); | |
| } |
Ito Test Report ❌20 test cases ran. 2 failed, 18 passed. The unified run executed 20 test cases with 18 passed and 2 failed, showing that core skills routing, creation/edit/delete flows, serialization and validation rules, deep-link and explicit not-found handling, mobile usability, and API safeguards (auth enforcement, path traversal rejection, XSS non-execution, duplicate/conflict and entry-file invariants) are largely working as expected. The two key regressions are medium-severity issues likely introduced by this PR: folder names containing spaces can be double-encoded (for example, %2520) causing file reload/deep-link not_found failures, and non-entry file edits can fail to set dirty state so Save stays disabled and unsaved-changes navigation protection is skipped, risking silent data loss. ❌ Failed (2)
🟠 Rapid interaction stress on save/delete controls
Relevant code:
const form = useForm({
resolver,
defaultValues: {
filePath: isCreateMode ? '' : filePath,
content: initialContent,
extension: '.md' as const,
},
mode: 'onChange',
});
const [isDeleteOpen, setIsDeleteOpen] = useState(false);
const { isDirty, isValid, isSubmitting } = form.formState;
const isSaveDisabled = isSubmitting || !canEdit || !isDirty || !isValid;
<Button type="submit" disabled={isSaveDisabled} size="sm">
Save
</Button>
<UnsavedChangesDialog dirty={canEdit && isDirty} onSubmit={handleSave} />
useEffect(() => {
if (!dirty) {
return;
}
const requestNavigationConfirmation = (navigate: PendingNavigation) => {
pendingNavigationRef.current = navigate;
setShowUnsavedDialog(true);
};
const handleDocumentClick = (event: MouseEvent) => {
if (!dirty || isNavigatingRef.current) {
return;
}🟠 Encoded folder paths can double-encode and break file reload
Relevant code:
const { tenantId, projectId, skillId, parentPath } = await params;
return (
<SkillFileEditor
tenantId={tenantId}
projectId={projectId}
skillId={skillId}
filePath=""
initialDirectoryPath={parentPath?.join('/')}
initialContent=""
/>
);
function encodeSkillFileRoutePath(routePath: string): string {
return routePath
.split('/')
.filter(Boolean)
.map((segment) => encodeURIComponent(segment))
.join('/');
}
export function resolveSkillFileFromRoute(
files: readonly SkillFileRecord[],
routeToken?: string
): SkillFileRecord | null {
if (!routeToken) {
return null;
}
const skillEntry = files.find(
(file) => file.skillId === routeToken && isSkillEntryFile(file.filePath)
);
if (skillEntry) {
return skillEntry;
}
const fileById = files.find((file) => file.fileId === routeToken);
if (fileById) {
return fileById;
}
return (
files.find((file) => file.routePath === routeToken) ??
files.find((file) => file.treePath === routeToken) ??
null
);
}✅ Passed (18)
Commit: Tell us how we did: Give Ito Feedback |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This is a re-review scoped to the 1 commit since the last automated review (bd73ee1e7bb9 → aa3c0426).
Changes analyzed:
agents-cli/src/commands/pull-v4/__tests__/skill-generator.test.ts(+4/-2)agents-cli/src/commands/pull-v4/generators/skill-generator.ts(+3/-1)agents-cli/src/commands/pull-v4/introspect/tool-references.test.ts(+1/-0)
Summary of Delta Changes
The commit aa3c0426 ("fixes for tests") addresses test alignment with the new skills schema:
-
Test fixtures updated — Added required
files: []property to skill test fixtures inskill-generator.test.tsandtool-references.test.tsto align with the newSkillWithFilesApiSelectSchema -
Schema fix — Made
metadataoptional inskill-generator.tsschema (.optional()) to prevent validation failures when metadata is undefined or empty -
Error message expectation updated — Changed expected validation error from
"Must be valid JSON object → at metadata"to"Invalid input: expected array, received undefined → at files"to reflect the new requiredfilesfield
Assessment
✅ Clean delta — These are straightforward test alignment changes that correctly adapt the test suite to the new multi-file skills API contract. No new issues introduced.
✅ Schema change is appropriate — Making metadata optional with the existing transform (v) => (Object.keys(v ?? {}).length ? v : undefined) correctly handles the case where metadata is empty or undefined.
🕐 Pending Recommendations (from prior review)
The following issues from the previous review remain applicable to the full PR scope:
- 🔴 Breaking API contracts for Skill create/update — Skills API changed from flat fields to
{ files: [...] }format - 🔴 Waterfall fetch pattern causes N+1 requests —
skills-data.tsfetches skill list then each detail sequentially - 🟠 Silent data loss when SKILL.md missing from update
- 🟠 Missing router.refresh() after skill deletion
- 🟠 Race condition in create mode navigation
- 🟠 URL encoding mismatch causes false 404s
- 🟠 Missing test coverage for entry file protection
💡 APPROVE WITH SUGGESTIONS
Summary: The delta changes are clean test fixes that correctly align the CLI test suite with the new multi-file skills schema. No new issues introduced in this commit. The prior review findings (breaking API changes, N+1 fetch pattern, etc.) remain applicable to the full PR scope and should be addressed before merge.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
3 | 0 | 0 | 0 | 0 | 7 | 0 |
| Total | 3 | 0 | 0 | 0 | 0 | 7 | 0 |
Note: Delta-only review — sub-reviewers not dispatched for minimal test-only changes.
Ito Test Report ❌18 test cases ran. 1 failed, 17 passed. Overall, 18 Skills-area tests were executed with 17 passing and 1 failing, confirming expected behavior across routing/deep-link resolution (including nested and encoded paths), permission and unauthenticated API gating, file create/update/delete integrity, edge-case validations (duplicate/path traversal/reserved SKILL.md/frontmatter/double-submit), inert handling of script-like content, and mobile usability. The single confirmed defect is a high-severity unsaved-changes regression introduced by this PR where browser Back/Forward bypasses the editor’s confirmation guard and can silently discard dirty edits (for example in notes.md), creating real data-loss risk. ❌ Failed (1)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Unauthenticated read/write attempts to skill-file manage endpoints were denied and no unauthorized mutation was observed. |  |
| Adversarial | Saved script payload in xss-probe.html as plain text; payload remained literal after reload and window.__xss_probe stayed undefined. |  |
| Edge | Duplicate create was rejected and only one reference/safety-checklist.txt remained after reload. |
 |
| Edge | Invalid traversal-like path inputs were rejected and no invalid files appeared. |  |
| Edge | Reserved SKILL.md creation through add-file flow was blocked with validation feedback and no duplicate entry file. |
 |
| Edge | Mismatched SKILL.md frontmatter name was rejected, and persisted content remained unchanged after reload. |
 |
| Edge | Rapid double-submit created only one rapid-submit-check.txt and routed once to the created file. |
 |
| Logic | Created safety-checklist.txt from folder context; success toast appeared, route resolved to file URL, and file remained present after refresh. |  |
| Logic | Appended marker to safety-checklist.txt; marker persisted through reload and matched exactly in a second tab. |  |
| Logic | Deleted active non-entry file delete-me.txt from skill qa-delete-skill; app redirected to SKILL.md with success toast, and direct navigation to deleted file URL returned controlled 404 not_found. |  |
| Logic | Deleting SKILL.md via Delete skill removed skill qa-delete-skill, redirected to /skills empty state with success toast, and previously created nested file notes/delete-me.txt became inaccessible (API returned 404 not_found). |  |
| Mobile | Mobile viewport workflow remained usable; save succeeded; remove-file cancel returned safely without deletion. |  |
| Happy-path | Skills root redirected to /skills/files/route-skill with file tree and editor rendered as expected. |
 |
| Happy-path | Empty skills state remained stable and Create Skill was gated off for simulated non-editor access. |  |
| Happy-path | Nested folder route rendered expected breadcrumb/parent navigation and opened itinerary-card.html correctly. |
 |
| Happy-path | Encoded deep-link /skills/files/.../day%20plans/checklist.md resolved successfully; earlier failure was environmental, not route resolution. |
 |
| Happy-path | FileId alias route resolved to the same file and browser history transitions stayed coherent. |  |
Commit: aa3c042
Tell us how we did: Give Ito Feedback
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This is a re-review scoped to the 6 commits since the last automated review (aa3c0426909e → f72940a20e57).
Commits analyzed:
f72940a20— typecheck (PR-specific)b2bba9fd5— Merge branch 'main' into prd-6236- 4 commits from main (Doltgres branch merge API, Version Packages, etc.)
Summary of Delta Changes
The PR-specific delta consists of a single commit (f72940a20 - "typecheck"):
-
File renamed:
agents-cli/src/commands/pull-v4/skill.ts→agents-cli/src/commands/pull-v4/generators/skill-generator.ts- Moves the skill generator into the
generators/directory alongside other generators - Consistent with the established pattern for agent-generator, scheduled-trigger-generator, etc.
- Moves the skill generator into the
-
Removed unused function: Deleted
asRecord()helper fromgeneration-types.ts- The function was already extracted to
collector-common.ts(via the merged#2654) - No duplicate definitions remain
- The function was already extracted to
Assessment
✅ Clean delta — The PR-specific changes are purely organizational:
- File relocation follows existing code structure patterns
- Dead code removal improves maintainability
- No new functionality or risk introduced in this delta
The merge from main (b2bba9fd5) brings in unrelated features (Doltgres branch merge API) that are out of scope for this skills feature review.
🕐 Pending Recommendations (from prior reviews)
The following issues from previous reviews remain applicable to the full PR scope. These have not been addressed in the current delta:
- 🔴 Breaking API contracts for Skill create/update — Skills API changed from flat fields to
{ files: [...] }format - 🔴 Waterfall fetch pattern causes N+1 requests —
skills-data.tsfetches skill list then each detail sequentially - 🟠 Silent data loss when SKILL.md missing from update — Returns
{ files: [] }which deletes all files - 🟠 Missing router.refresh() after skill deletion — Deleted skill may appear in sidebar
- 🟠 Race condition in create mode navigation —
router.push()+router.refresh()may not reliably navigate - 🟠 URL encoding mismatch causes false 404s — Files with special characters may fail to resolve
- 🟠 Missing test coverage for entry file protection — SKILL.md creation/deletion guards untested
Additionally from pullfrog review:
- 🟠
as anycast bypasses type safety —SkillApiUpdateSchematransform - 🟠 Sequential DB updates in
replaceSkillFiles— Could be batched - 💭
z.looseObjectbehavior change — Accepts unknown frontmatter keys
💡 APPROVE WITH SUGGESTIONS
Summary: The delta since the last review is clean — just a file reorganization and dead code removal. No new issues introduced. However, the prior review findings remain unaddressed and should be considered before merging. The most critical items are the breaking API contract change and the N+1 fetch pattern.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
2 | 0 | 0 | 0 | 0 | 10 | 0 |
| Total | 2 | 0 | 0 | 0 | 0 | 10 | 0 |
Note: Delta-only review — sub-reviewers not dispatched for minimal organizational changes.
sarah-inkeep
left a comment
sarah-inkeep
left a comment
There was a problem hiding this comment.
A few things I noticed:
-
if I make a change, then hit save, then make another change the save button stays disabled then if I try to navigate away the unsaved changes modal opens but when I hit the save button, nothing happens. https://www.loom.com/share/869f4fd9928d48c1bed52d10322b6db6
-
should spaces be allowed in filenames? I created a file called
my fileand it was created successfully but then when I tried to load it I got a 404 -
the cancel and x buttons on the delete skill modal do not seem to do anything when the modal is opened from the delete skill button in the file header but they do work when the delete modal is opened from the context menu
-
I noticed that some of the skills I had created before that are returned from
tenants/${*tenantId*}/projects/${*projectId*}/skills?limit=100are not visible in the new ui, I think it is because their files array is empty, I'm not sure if maybe I messed something up when I ran my migrations locally but I just want to make sure any skills created before the new nested files will be properly migrated. -
Could you take a look at these Claude code comments as well, I think some of them might be relevant #2719 (review)
non blocking (feel free to defer these):
-
maybe if it’s easy we could auto add a hyphen when people type a space in the skill name, just a thought to enhance the ux but feel free to defer
-
maybe in the delete warning for a skill we could note that all other files in the folder will be deleted as well?
9 participants
No description provided.