fix(skillify): locally migrate legacy over-long skill installs to the 64-char cap#327
Merged
Merged
Conversation
PR #322 caps skill names to codex's 64-char loader limit, but the migration of pre-cap installs only runs while processing a MATCHING remote row during a pull. A legacy managed install whose org/workspace is not the currently-routed one never matches, so it stays un-capped forever and every codex session logs 'invalid name: exceeds maximum length of 64 characters' for it (observed live). Add a local, remote-independent migration pass that runs from autoPullSkills before the network query (works offline and logged-out): - Scans ONLY manifest-managed global entries - never infers ownership from directory names, never touches project-scoped installs. - Validates author and names with the same validators pull uses before building any path. - Copy-then-swap: copy to the capped dir, rewrite the frontmatter name, record the canonical manifest entry (rawName preserved), fan out symlinks, and only then retire the legacy dir + entry - a failure at any earlier step leaves the original fully intact for the next retry. - A collision with the SAME rawName is a leftover from a prior migration/pull and is reconciled (legacy removed, canonical kept); a different rawName is a true collision and is skipped.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an offline migration for overlong global skill install names, including collision safety, manifest and symlink reconciliation, retry handling, and tests. ChangesLegacy capped install migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SessionStart
participant autoPullSkills
participant LegacyCapMigration
participant Manifest
participant FileSystem
participant QueryFn
SessionStart->>autoPullSkills: start automatic pull
autoPullSkills->>LegacyCapMigration: migrate legacy capped installs
LegacyCapMigration->>Manifest: inspect managed global entries
LegacyCapMigration->>FileSystem: copy and rewrite capped install
LegacyCapMigration->>Manifest: update canonical entry and symlinks
LegacyCapMigration-->>autoPullSkills: complete or report swallowed failure
autoPullSkills->>QueryFn: load pull configuration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 2 files changed
Generated for commit 625ce1a. |
![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
🧹 Nitpick comments (2)
tests/claude-code/legacy-cap-migration.test.ts (1)
106-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer exact content equality over
.toContain(...)substring checks.Since
writeSkillproduces fully deterministic SKILL.md content, these body-preservation checks (e.g. lines 106, 221, 247-249, 269-270, 317, 322, 363, 418, 442, 458, 476) could assert the exact expected content instead of a substring — as already done correctly at line 132 (fileAfterFirst). A substring check wouldn't catch unintended corruption elsewhere in the file (e.g. a stray extra newline or a mangled unrelated frontmatter field).As per path instructions, "Prefer asserting on specific values (paths, messages) over generic substrings."
Also applies to: 221-222, 247-249, 269-270, 317-317, 322-322, 363-363, 418-418, 442-442, 458-458, 476-476
🤖 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 `@tests/claude-code/legacy-cap-migration.test.ts` around lines 106 - 107, Replace the listed `.toContain(...)` assertions in the legacy migration tests with exact equality checks against the complete deterministic SKILL.md content produced by `writeSkill`. Reuse the established `fileAfterFirst` expected-content pattern where applicable, ensuring checks cover the full body and frontmatter rather than only selected substrings.Source: Path instructions
src/skillify/legacy-cap-migration.ts (1)
202-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
recordPullpayload construction.The two
recordPullcalls (steps b and d) are identical except forsymlinks(and a freshly re-computedpulledAt). Consider extracting a smallbuildCanonicalEntry(symlinks)helper to avoid the drift risk of editing one call and forgetting the other, and to reuse a singlepulledAttimestamp for both writes.🤖 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/skillify/legacy-cap-migration.ts` around lines 202 - 236, Extract the shared `recordPull` payload construction in the migration flow into a `buildCanonicalEntry(symlinks)` helper, reusing the existing entry fields and accepting only the symlink list as variable input. Capture one `pulledAt` timestamp before both writes and use the helper for the step (b) and step (d) `recordPull` calls, preserving their current symlink behavior.
🤖 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/skillify/legacy-cap-migration.ts`:
- Around line 145-191: Update the collision-reconciliation logic in migrateEntry
to treat readInstalledName(cappedFile) returning null the same as an overlong
canonical name, routing both through the repair path instead of retiring the
legacy entry. Extend the existing try/catch to cover the entire reconciliation
block, including rmSync, unlinkSymlinks, and removePullEntry, so per-entry
failures are swallowed and do not abort migrateLegacyCappedInstalls.
---
Nitpick comments:
In `@src/skillify/legacy-cap-migration.ts`:
- Around line 202-236: Extract the shared `recordPull` payload construction in
the migration flow into a `buildCanonicalEntry(symlinks)` helper, reusing the
existing entry fields and accepting only the symlink list as variable input.
Capture one `pulledAt` timestamp before both writes and use the helper for the
step (b) and step (d) `recordPull` calls, preserving their current symlink
behavior.
In `@tests/claude-code/legacy-cap-migration.test.ts`:
- Around line 106-107: Replace the listed `.toContain(...)` assertions in the
legacy migration tests with exact equality checks against the complete
deterministic SKILL.md content produced by `writeSkill`. Reuse the established
`fileAfterFirst` expected-content pattern where applicable, ensuring checks
cover the full body and frontmatter rather than only selected substrings.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fa6cce33-261c-4d96-b33f-817d918942c1
📒 Files selected for processing (4)
src/skillify/auto-pull.tssrc/skillify/legacy-cap-migration.tstests/claude-code/legacy-cap-migration.test.tsvitest.config.ts
CodeRabbit review on #327 flagged two real defects in the same-rawName reconciliation block: - It ran OUTSIDE the per-entry try/catch, so an I/O error in the repair teardown (rmSync / unlinkSymlinks / removePullEntry) escaped migrateLegacyCappedInstalls and could abort autoPullSkills - breaking the swallow-all-per-entry-failures contract. - An ORPHANED canonical manifest row (dir deleted manually, frontmatter unreadable) fell into the already-migrated branch and retired the legacy install - dropping the only surviving copy. The reconciliation now lives inside the try, and a null installed name routes to the repair path (tear down the broken canonical row, re-migrate from the intact legacy) instead of retiring the legacy. Tests cover both: a throw inside reconciliation is swallowed with both copies left intact, and an orphaned row re-migrates instead of retiring.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
#322 caps skill frontmatter names to codex's 64-char loader limit and migrates prior un-capped installs — but only while processing a MATCHING remote row during a pull. A legacy managed install whose org/workspace isn't the currently-routed one never matches, so it stays un-capped forever. Observed live: every codex session logs
invalid name: exceeds maximum length of 64 charactersfor pre-#322 installs.Fix
New local, remote-independent migration pass (
src/skillify/legacy-cap-migration.ts), wired intoautoPullSkillsBEFORE the network query — runs offline and logged-out:Known limitation (intentional): legacy dirs with NO manifest entry are not touched — manifest ownership is the safety boundary. Those still self-heal via a full
hivemind skillify pullagainst their org.Validation (real harness)
Replicated a manifest-managed legacy install (the real 69-char
pg-deeplake-multi-layer-issue-diagnosis-and-workaround-prioritization--sasun) in a sandbox HOME and ran a real interactive claude session with this build via--plugin-dir. The real SessionStart hook migrated it:pg-deeplake-multi-layer-issue-diagnosis-and-workaround-420af--sasunname:→ 60 chars (deterministic hash suffix, samecapSkillNameas pull)rawNamepreserving the original.A real
codex execsession was separately shown to load a capped-name skill by name (and to reject the un-capped legacy ones — the exact bug).tscclean,npm run buildclean; targeted suites 107 passed; full suite 5652 passed with per-file coverage thresholds met (new file: 97% stmts / 89% branches).codex reviewof the diff: initial round's blockers (post-rename orphaning, missing path validation, same-rawName reconciliation, cross-project scope) all addressed as above.Session Context
Session transcript
Summary by CodeRabbit
Bug Fixes
Tests