refactor(skills): collapse installed-skill walkers into one inventory module - #317
Conversation
… module The area's central fact — "skill X from package P at version V, present on agents A,B" — was re-derived by five independent filesystem walkers with divergent precedence rules: the shared merge in installed-managed-skills.ts (host-first-wins by name across kinds), a private same-name clone in update.ts (registry-only, hostNames union that was never read), sync.ts's collector (max-semver per package+skill), info-inventory.ts's host scanner (identity-key grouping with a shadow-host second pass), plus copy-pasted directory scans in auto-sync.ts and legacy-gpt-image-2-cleanup.ts. Land skills/installed-skills.ts as the single owner of that fact: - readInstalledSkills(env, settingsFilePath) walks the canonical registry root and every available host once and returns one row per skill identity (kind + packageName + name) with a per-agent copy breakdown. - The row hides the unified rules: installed = at least one copy with parseable .oo-metadata.json; installed version = highest semver across copies (ties keep the canonical-then-agent-order copy); rows ordered bundled-in-embedded-order, then registry, then local, names sorted within each kind; metadata-less and unparseable copies attach to a same-name installed row as agent copies instead of forming rows. - update, check-update, sync, info/list, recommend, and package uninstall now project from the shared rows; locate (documented SKILL.md-presence contract), auto-sync's repair loop, and self-uninstall's physical-path walk deliberately stay outside the seam. Behavior fixes in previously untested divergent-copy states: - check-update and sync upload now report the same installed version (the highest copy); check-update previously reported whichever host copy the agent declaration order found first. - A same-name bundled/local host copy no longer hides a registry skill from check-update and package uninstall (both now agree with update, which already ignored non-registry copies). The merge rules gain direct table tests in installed-skills.test.ts; new CLI tests pin both fixes. The `skills list/info` JSON contract and documented inventory ordering are unchanged.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/application/commands/skills/sync.ts (1)
1-1: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle canonical-only registry rows consistently
readInstalledSkills()can emit registry rows withcanonicalbut noagents, andisInstalledRegistrySkill()/installedRegistrySkillNamesForPackage()still treat them as installed. That makessync.tsupload them as active, while text uninstall throws if nothing is removable; the JSON path maps the same case tonot-applicable. Either exclude these rows from package resolution/sync or make both uninstall paths treat them the same way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/sync.ts` at line 1, Update isInstalledRegistrySkill() and installedRegistrySkillNamesForPackage() so registry rows containing canonical without agents are not treated as installed or active during sync. Ensure sync.ts excludes these rows from package resolution/upload, and keep text and JSON uninstall behavior consistent by treating them as not-applicable when no removable agents exist.
🧹 Nitpick comments (4)
src/application/commands/skills/info-inventory.test.ts (1)
108-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompose
basePathwithjoin()instead of a POSIX literal.As per coding guidelines, "Never assume POSIX path separators in code, tests, snapshots, or assertions. Use
node:pathhelpers such asjoin(),resolve(), andrelative()for path construction."♻️ Optional cleanup
- const basePath = "/tmp/host/path"; + const basePath = join(tmpdir(), "host", "path");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/info-inventory.test.ts` at line 108, Update the basePath setup in the inventory test to construct the path with the appropriate node:path join helper instead of a POSIX-style literal, preserving the intended /tmp, host, and path segments.Source: Coding guidelines
src/application/commands/skills/installed-skills.ts (1)
228-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
rowsByIdentity.seton every host copy.
setis only needed when the row was just created; re-setting an existing row each iteration is a no-op that obscures intent.♻️ Optional cleanup
- const row = rowsByIdentity.get(identityKey) - ?? createRow(copy.name, copy.metadata); + let row = rowsByIdentity.get(identityKey); + + if (row === undefined) { + row = createRow(copy.name, copy.metadata); + rowsByIdentity.set(identityKey, row); + } + const version = readMetadataVersion(copy.metadata); - - rowsByIdentity.set(identityKey, row);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/installed-skills.ts` around lines 228 - 257, Update the host-copy loop around rowsByIdentity and createRow so rowsByIdentity.set is called only when a new row is created; preserve the existing row lookup and agent append behavior for already-known identity keys.src/application/commands/skills/check-update.ts (1)
128-137: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHosts are resolved twice per invocation.
resolveAvailableManagedSkillHosts(context.env)runs here and again insidereadInstalledSkills, duplicating host probing. Consider havingreadInstalledSkillsaccept pre-resolved hosts (or return them) so the handler's guard and the scan share one resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/check-update.ts` around lines 128 - 137, Update the check-update handler and readInstalledSkills flow so resolveAvailableManagedSkillHosts(context.env) executes only once per invocation. Pass the resolved hosts into readInstalledSkills, or otherwise reuse a returned resolution, while preserving the empty-host guard before scanning installed skills.src/application/commands/skills/sync.cli.test.ts (1)
82-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate canonical-path setup logic.
This block recomputes
storePaths/canonicalDirectorythatseedRegistrySkillalready derives internally, just to overwrite the canonical metadata with a diverged version. Consider extending the helper (e.g., an optionalcanonicalOverrideVersionparam) or adding a small helper for "diverge canonical copy" so this path-resolution isn't duplicated per test.As per coding guidelines, "In test files, extract repeated setup (mock, stub, or setup objects) into a local factory function at the bottom of the file."
♻️ Example helper extension
export async function seedRegistrySkill(options: { sandbox: Awaited<ReturnType<typeof createCliSandbox>>; skillName: string; packageName: string; version: string; + canonicalVersionOverride?: string; }): Promise<void> { ... + if (options.canonicalVersionOverride !== undefined) { + await writeFile( + resolveManagedSkillMetadataFilePath(canonicalDirectory), + renderSkillMetadataJson(createRegistrySkillMetadata({ + packageName: options.packageName, + version: options.canonicalVersionOverride, + })), + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/sync.cli.test.ts` around lines 82 - 99, Remove the duplicated storePaths and canonicalDirectory resolution from the test setup around seedRegistrySkill. Extend seedRegistrySkill with an optional canonicalOverrideVersion, or add a local helper at the bottom of the test file that diverges the canonical copy using the paths it derives, then use it to write the 0.3.0 metadata while preserving the existing test behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/application/commands/skills/update.ts`:
- Around line 361-364: Update the default update filter in
src/application/commands/skills/update.ts lines 361-364 to select registry
entries by inventory kind only, removing the skill-name bundled check. Apply the
same registry-only filter to the JSON path at
src/application/commands/skills/update.ts lines 701-702, preserving registry
rows even when their names match bundled skills.
---
Outside diff comments:
In `@src/application/commands/skills/sync.ts`:
- Line 1: Update isInstalledRegistrySkill() and
installedRegistrySkillNamesForPackage() so registry rows containing canonical
without agents are not treated as installed or active during sync. Ensure
sync.ts excludes these rows from package resolution/upload, and keep text and
JSON uninstall behavior consistent by treating them as not-applicable when no
removable agents exist.
---
Nitpick comments:
In `@src/application/commands/skills/check-update.ts`:
- Around line 128-137: Update the check-update handler and readInstalledSkills
flow so resolveAvailableManagedSkillHosts(context.env) executes only once per
invocation. Pass the resolved hosts into readInstalledSkills, or otherwise reuse
a returned resolution, while preserving the empty-host guard before scanning
installed skills.
In `@src/application/commands/skills/info-inventory.test.ts`:
- Line 108: Update the basePath setup in the inventory test to construct the
path with the appropriate node:path join helper instead of a POSIX-style
literal, preserving the intended /tmp, host, and path segments.
In `@src/application/commands/skills/installed-skills.ts`:
- Around line 228-257: Update the host-copy loop around rowsByIdentity and
createRow so rowsByIdentity.set is called only when a new row is created;
preserve the existing row lookup and agent append behavior for already-known
identity keys.
In `@src/application/commands/skills/sync.cli.test.ts`:
- Around line 82-99: Remove the duplicated storePaths and canonicalDirectory
resolution from the test setup around seedRegistrySkill. Extend
seedRegistrySkill with an optional canonicalOverrideVersion, or add a local
helper at the bottom of the test file that diverges the canonical copy using the
paths it derives, then use it to write the 0.3.0 metadata while preserving the
existing test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 992881bb-6e8f-41ca-8065-c48f89d48f74
📒 Files selected for processing (19)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/__tests__/helpers.tssrc/application/commands/skills/auto-sync.tssrc/application/commands/skills/check-update.cli.test.tssrc/application/commands/skills/check-update.tssrc/application/commands/skills/info-inventory.test.tssrc/application/commands/skills/info-inventory.tssrc/application/commands/skills/installed-managed-skills.tssrc/application/commands/skills/installed-skills.test.tssrc/application/commands/skills/installed-skills.tssrc/application/commands/skills/legacy-gpt-image-2-cleanup.tssrc/application/commands/skills/managed-skill-uninstall.tssrc/application/commands/skills/recommend/plan.tssrc/application/commands/skills/sync.cli.test.tssrc/application/commands/skills/sync.tssrc/application/commands/skills/uninstall.cli.test.tssrc/application/commands/skills/uninstall.tssrc/application/commands/skills/update.ts
💤 Files with no reviewable changes (1)
- src/application/commands/skills/installed-managed-skills.ts
Resolves the predicted overlap with #316 (skill-directory-state): per-host probes in check-update/update/uninstall/auto-sync keep main's readSkillDirectoryState / isCurrentRegistryPublication implementations, the data-source layer keeps this branch's readInstalledSkills projection, and the private directory-scan copies deleted on both sides stay deleted (auto-sync and legacy-gpt-image-2-cleanup now import the shared readSkillsDirectoryEntries). info-inventory keeps this branch's row projection; main's readHostScan semantics (not-directory and IO errors read as unparseable) are already covered by the inventory module's copy states.
1 participant
Summary
The skills area's central fact — "skill X from package P at version V, present on agents A,B" — was re-derived by five independent filesystem walkers with divergent merge rules:
installed-managed-skills.ts(shared by check-update / recommend / uninstall)update.ts:359private same-name clonehostNamesunion that nothing ever readsync.tscollectorinfo-inventory.tshost scannerauto-sync.ts/legacy-gpt-image-2-cleanup.tsConcrete symptom:
check-updateandsync uploadcould report two different versions for the same skill, andcheck-update/uninstalldisagreed withupdateabout whether a shadowed registry skill was installed at all. None of these divergences were pinned by any test.This PR lands
skills/installed-skills.tsas the single owner of that fact:readInstalledSkills(env, settingsFilePath)walks the canonical registry root and every available host once, returning one row per skill identity (kind + packageName + name) with a per-agent copy breakdown (state: managed | unmanaged | unparseable)..oo-metadata.json; installed version = highest semver across copies (ties keep the canonical-then-agent-order copy); rows ordered bundled (embedded order) → registry → local, names sorted within each kind; metadata-less/unparseable copies attach to a same-name installed row instead of forming rows.update,check-update,sync,info/list,recommend, and packageuninstallnow project from the shared rows. Deliberately outside the seam:locate(documented SKILL.md-presence contract), auto-sync's repair loop, and self-uninstall's physical-path walk — the first two now merely reuse the exported directory-scan primitive.Behavior changes (all in previously untested divergent-copy states)
versionpreviously took the first non-null host copy. Per-host repair logic (update,check-updatestatus) still inspects each copy and is unchanged.check-updateand packageuninstall(both now agree withupdate, which already ignored non-registry copies).unparseablecopies, asskills listalready did) instead of aborting the whole command.The
skills list/info --jsonfield shape, ordering guarantees, shadow-host semantics, and summary counts are unchanged.Tests
installed-skills.test.ts: table tests over a seeded tmpdir for precedence, max-semver + tie-break, identity separation, shadow attach/drop, canonical-root filtering, ordering, legacy metadata, and the selectors.resolveHostControlStateunit tests migrated to the copy-state input shape; the 901-linelist.cli.test.tssnapshot suite passes unchanged.Docs
docs/commands.md/docs/commands.zh-CN.md: documentedcurrentVersionas the highest installed copy (same answer assync upload).All gates pass:
lint:fix,ts-check,knip,bun run test(1680 pass / 0 fail).