fix(skillify): cap skill names to the 64-char loader limit#322
Conversation
Codex's skill loader silently drops any skill whose frontmatter `name` exceeds 64 chars, so `hivemind skillify` was minting/installing skills that never load. Cap names to the loader ceiling at the write seam and migrate existing over-long installs on pull. - skill-writer: capSkillName() truncates >64 names at a hyphen boundary with a low-order hash suffix for disambiguation; writeNewSkill validates the raw name then caps and returns the canonical on-disk name. assertValidSkillName stays a path-safety validator (100-char ceiling), decoupled from the loader limit. - worker: cap only KEEP (never MERGE, so legacy over-long merge targets keep their lineage); record the canonical result.name in state + the org row. - pull: validate the raw name before capping; migrate a prior un-capped install to the capped dir (preserving content that is not older than remote), gated on manifest ownership; retire the stale dir (dir removed before its manifest entry, retried on later pulls); skip within-run cap collisions. - mine-local: record the canonical capped name in the local manifest. Tests cover capping/hashing/idempotency/collision, raw-name validation, equal- and newer-version migration, ownership guards, and collision skips.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughSkill names are deterministically capped to 64 characters before storage. Pulls migrate managed uncapped directories, handle collisions, and preserve eligible local content. Writers, workers, manifests, and deduplication state use the canonical on-disk name. ChangesCanonical skill identity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PullCommand
participant SkillWriter
participant Manifest
participant LocalFilesystem
PullCommand->>SkillWriter: cap raw skill name
PullCommand->>Manifest: check stale directory ownership
PullCommand->>LocalFilesystem: write capped SKILL.md
PullCommand->>LocalFilesystem: retire managed stale directory
PullCommand->>Manifest: remove stale manifest entry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 5 files changed
Generated for commit 2b6d5c4. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const migrated = readFileSync(staleFile, "utf-8").replace(/^name:.*$/m, `name: ${name}`); | ||
| writeFileSync(skillFile, migrated); | ||
| } else { | ||
| writeFileSync(skillFile, renderSkillFile(row, name)); |
There was a problem hiding this comment.
False positive in this context (verified by an independent codex review focused on this alert).
The destination resolves under the invoking user's own skill root — resolvePullDestination → ~/.claude/skills or <cwd>/.claude/skills (pull.ts:190-193), a user-owned, non-world-writable directory. Untrusted row names are traversal-validated and length-capped before any path is built (assertValidSkillName on the raw name, then capSkillName), and authors are validated (assertValidAuthor).
A js/file-system-race (TOCTOU) is exploitable only across a privilege boundary — e.g. a less-privileged attacker swapping a component for a symlink between check and use to redirect a privileged write. Here there is no privilege boundary and no privileged writer: any process that could swap a file in ~/.claude/skills between the check and the write already has the same user-level access to modify those files directly, so the race grants no escalation. The existsSync check only decides whether to make a .bak; the subsequent write targets a validated, single-segment path.
No concrete exploit requires a fix. (The one genuinely-unguarded read was additionally hardened with a try/catch fallback.) Marking as won't-fix / false positive for this local-only path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/claude-code/mine-local-orchestrator.test.ts (1)
166-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise a returned name that differs from the input.
At least one mock should return a specific capped name distinct from
opts.name, then assert that exact value in the manifest. Returningopts.nameeverywhere only satisfies the new shape and cannot detect broken canonical-name propagation.As per path instructions, prefer asserting on specific values (paths, messages) over generic substrings.
Also applies to: 576-576, 649-649
🤖 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/mine-local-orchestrator.test.ts` at line 166, Update the mocks in the affected test cases to return a specific capped name different from opts.name, then assert that exact canonical name in the generated manifest. Apply this to the mocks around the visible name property and the additional occurrences, preserving the existing manifest assertions while ensuring returned-name propagation is exercised.Source: Path instructions
tests/claude-code/skillify-skill-writer.test.ts (1)
94-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse exact validation and destination messages.
The generic substring assertions can pass when execution reaches the wrong error branch.
tests/claude-code/skillify-skill-writer.test.ts#L94-L100: assert the complete path-separator error containingevil.tests/claude-code/skillify-skill-writer.test.ts#L376-L380: assertinvalid skill name: too long (101 chars).tests/claude-code/skillify-pull.test.ts#L606-L607: assert the complete collision destination including the claimant.tests/claude-code/skillify-pull.test.ts#L619-L621: assert"(invalid name — skipped)".As per path instructions, prefer asserting on specific values (paths, messages) over generic substrings.
🤖 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/skillify-skill-writer.test.ts` around lines 94 - 100, Replace the generic assertions with exact expected messages at all four sites: in tests/claude-code/skillify-skill-writer.test.ts:94-100, assert the complete path-separator error includes evil; at tests/claude-code/skillify-skill-writer.test.ts:376-380, assert “invalid skill name: too long (101 chars)”; in tests/claude-code/skillify-pull.test.ts:606-607, assert the complete collision destination including the claimant; and at tests/claude-code/skillify-pull.test.ts:619-621, assert “(invalid name — skipped)”.Source: Path instructions
🤖 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/pull.ts`:
- Around line 508-513: Update the capped-destination ownership flow around
claimedDirs and the existing-destination handling near the affected pull logic
to persist and recover the raw claimant or another stable source identity in the
manifest. Before treating an existing destination as installed or allowing
--force to overwrite it, compare that persisted owner with the current row’s raw
identity; skip or report collisions without overwriting when they differ, while
preserving reuse for the same owner.
- Around line 674-698: Update the stale-directory cleanup around recordPull and
removePullEntry so the managed stale directory and entry are not removed until a
canonical manifest entry is confirmed. If recordPull fails, preserve the stale
entry and surface manifestError, or retry the manifest migration independently
before performing cleanup. Keep user-created directories and dry-run behavior
unchanged.
---
Nitpick comments:
In `@tests/claude-code/mine-local-orchestrator.test.ts`:
- Line 166: Update the mocks in the affected test cases to return a specific
capped name different from opts.name, then assert that exact canonical name in
the generated manifest. Apply this to the mocks around the visible name property
and the additional occurrences, preserving the existing manifest assertions
while ensuring returned-name propagation is exercised.
In `@tests/claude-code/skillify-skill-writer.test.ts`:
- Around line 94-100: Replace the generic assertions with exact expected
messages at all four sites: in
tests/claude-code/skillify-skill-writer.test.ts:94-100, assert the complete
path-separator error includes evil; at
tests/claude-code/skillify-skill-writer.test.ts:376-380, assert “invalid skill
name: too long (101 chars)”; in tests/claude-code/skillify-pull.test.ts:606-607,
assert the complete collision destination including the claimant; and at
tests/claude-code/skillify-pull.test.ts:619-621, assert “(invalid name —
skipped)”.
🪄 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: 80158a96-514f-4416-b125-00c6dbc9c6ac
📒 Files selected for processing (7)
src/commands/mine-local.tssrc/skillify/pull.tssrc/skillify/skill-writer.tssrc/skillify/skillify-worker.tstests/claude-code/mine-local-orchestrator.test.tstests/claude-code/skillify-pull.test.tstests/claude-code/skillify-skill-writer.test.ts
Follow-up from CodeRabbit + CodeQL on the name-cap PR: - Persist the raw row name in the pull manifest and check it before reusing a capped destination, so a collision across separate (e.g. --skill) pulls is detected — not just within one run — and never overwrites, even with --force. - Retire a migrated stale dir only after the canonical capped install is recorded in the manifest; if recordPull failed, keep the stale entry so a later pull retries instead of leaving the canonical file untracked. - Read the stale file defensively during migration (fall back to the remote row if it vanished mid-run) — clears the CodeQL file-race warning.
| // rather than throwing out of the pull. | ||
| try { | ||
| const migrated = readFileSync(staleFile, "utf-8").replace(/^name:.*$/m, `name: ${name}`); | ||
| writeFileSync(skillFile, migrated); |
There was a problem hiding this comment.
False positive in this context (verified by an independent codex review focused on this alert).
The destination resolves under the invoking user's own skill root — resolvePullDestination → ~/.claude/skills or <cwd>/.claude/skills (pull.ts:190-193), a user-owned, non-world-writable directory. Untrusted row names are traversal-validated and length-capped before any path is built (assertValidSkillName on the raw name, then capSkillName), and authors are validated (assertValidAuthor).
A js/file-system-race (TOCTOU) is exploitable only across a privilege boundary — e.g. a less-privileged attacker swapping a component for a symlink between check and use to redirect a privileged write. Here there is no privilege boundary and no privileged writer: any process that could swap a file in ~/.claude/skills between the check and the write already has the same user-level access to modify those files directly, so the race grants no escalation. The existsSync check only decides whether to make a .bak; the subsequent write targets a validated, single-segment path.
No concrete exploit requires a fix. (The one genuinely-unguarded read was additionally hardened with a try/catch fallback.) Marking as won't-fix / false positive for this local-only path.
| const migrated = readFileSync(staleFile, "utf-8").replace(/^name:.*$/m, `name: ${name}`); | ||
| writeFileSync(skillFile, migrated); | ||
| } catch { | ||
| writeFileSync(skillFile, renderSkillFile(row, name)); |
There was a problem hiding this comment.
False positive in this context (verified by an independent codex review focused on this alert).
The destination resolves under the invoking user's own skill root — resolvePullDestination → ~/.claude/skills or <cwd>/.claude/skills (pull.ts:190-193), a user-owned, non-world-writable directory. Untrusted row names are traversal-validated and length-capped before any path is built (assertValidSkillName on the raw name, then capSkillName), and authors are validated (assertValidAuthor).
A js/file-system-race (TOCTOU) is exploitable only across a privilege boundary — e.g. a less-privileged attacker swapping a component for a symlink between check and use to redirect a privileged write. Here there is no privilege boundary and no privileged writer: any process that could swap a file in ~/.claude/skills between the check and the write already has the same user-level access to modify those files directly, so the race grants no escalation. The existsSync check only decides whether to make a .bak; the subsequent write targets a validated, single-segment path.
No concrete exploit requires a fix. (The one genuinely-unguarded read was additionally hardened with a try/catch fallback.) Marking as won't-fix / false positive for this local-only path.
Follow-ups from codex review of the branch: - Cross-run cap collisions: persist the raw row name in the manifest (PulledEntry.rawName, preserved through loadManifest) and fall back to the entry's name for legacy entries, so a collider pulled in a separate run is detected — never overwritten, even with --force. - Stale-dir migration: gate content migration on manifest ownership (never copy an unmanaged user dir's content), preserve a stale copy that is not older than remote, and adopt an untracked canonical file on a later skipped pull so a failed recordPull doesn't leave the migration permanently incomplete. - Read the stale file defensively during migration. Tests: legacy-entry collision fallback, cross-run collision via persisted rawName, adopt-on-skip migration completion, and a mergeSkill regression proving a legacy >64 target is updated in place (not forked) — the latter verified end-to-end against the real functions on a real filesystem.
2 participants
Summary
Codex's skill loader silently drops any skill whose frontmatter
nameexceeds 64 chars (invalid name: exceeds maximum length of 64 characters).hivemind skillifywas minting and installing skills over that limit, so they never loaded — e.g.pg-deeplake-multi-layer-issue-diagnosis-and-workaround-prioritization(68 chars).The 64-char ceiling applies to the frontmatter
name, not the directory name (verified empirically: a 64-char name loads, 66+ is rejected;<name>--<author>dirs well over 64 load fine as long as thenamefits).Changes
capSkillName()truncates >64 names at a hyphen boundary with a low-order hash suffix for disambiguation;writeNewSkillvalidates the raw name, then caps, and returns the canonical on-disk name.assertValidSkillNamestays a path-safety validator (100-char ceiling), decoupled from the loader limit.KEEP(neverMERGE, so legacy over-long merge targets keep their version/author/lineage); records the canonicalresult.namein local state and the org row.rawName, skipping (never overwriting) the collider.PulledEntrygains an optionalrawName(persisted raw row name) for cross-run collision detection.Existing over-long rows in the org table are left unchanged and self-heal on the next pull.
Version Bump
None in this PR — bug fix only, no
package.jsonchange; the release is cut separately by the maintainer flow.Test plan
npx tsc --noEmitclean.npx vitest run— 4588 tests pass.codex review(multiple rounds; core confirmed sound) and CodeRabbit (2 Major data-integrity items addressed in e829418).Summary by CodeRabbit
New Features
Bug Fixes