refactor(skills): publish skills with copy mode only#168
Conversation
Remove symlink publication for bundled, registry, and local skills because supported agents handle linked skill directories inconsistently. Startup synchronization now treats existing managed symlink targets as stale and replaces them with copied directories. Local canonical skills are also synchronized to newly detected hosts without overwriting registry-managed targets. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThis PR eliminates the symlink-or-copy publication strategy for bundled and published skills, converting to copy-only behavior. The Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (1)
src/application/commands/skills/index.test.ts (1)
2953-2961: ⚡ Quick winMove this copied-directory assertion into a shared test helper.
This helper is now duplicating the same
realpath(...) !== ...pluslstat(...).isSymbolicLink() === falsechecks that also appear in other changed test files. Centralizing it in__tests__/helpers.tswill keep future publication-mode changes from drifting across suites.As per coding guidelines, "When identical logic appears in 2+ files, extract it to a shared module with parameterized varying parts" and "If a helper function might be called by other test files, place it in the tests/helpers.ts file."
🤖 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/index.test.ts` around lines 2953 - 2961, Extract the duplicated assertion helper expectCopiedSkillDirectory into the shared test helpers file by creating/exporting the function from __tests__/helpers.ts (keep the same async signature: expectCopiedSkillDirectory(skillDirectoryPath: string, canonicalSkillDirectoryPath: string): Promise<void>) and then replace the local definition in src/application/commands/skills/index.test.ts with an import of that helper; update any other test files that contain the same realpath + lstat checks to import and use the shared expectCopiedSkillDirectory, and remove the duplicate local implementations so all suites use the single exported helper.
🤖 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/managed-skill-publication.test.ts`:
- Around line 12-23: The test "treats a copied target as current" currently
creates an empty directory and asserts isManagedSkillPublicationCurrent(...)
returns true; instead, seed the fixture directory with the concrete managed
publication files that isManagedSkillPublicationCurrent expects (e.g., the skill
manifest and any marker files or metadata created for managed installs) before
calling isManagedSkillPublicationCurrent, or alternatively tighten
isManagedSkillPublicationCurrent to validate those files' presence/content
rather than treating any non-symlink directory as current; update the test to
create those files (using mkdir + writeFile) so the assertion reflects a real
managed publication.
---
Nitpick comments:
In `@src/application/commands/skills/index.test.ts`:
- Around line 2953-2961: Extract the duplicated assertion helper
expectCopiedSkillDirectory into the shared test helpers file by
creating/exporting the function from __tests__/helpers.ts (keep the same async
signature: expectCopiedSkillDirectory(skillDirectoryPath: string,
canonicalSkillDirectoryPath: string): Promise<void>) and then replace the local
definition in src/application/commands/skills/index.test.ts with an import of
that helper; update any other test files that contain the same realpath + lstat
checks to import and use the shared expectCopiedSkillDirectory, and remove the
duplicate local implementations so all suites use the single exported helper.
🪄 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: d968661d-898a-4b63-8ede-7c1881f31f9f
📒 Files selected for processing (19)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/auto-sync.tssrc/application/commands/skills/bundled-skill-filesystem.tssrc/application/commands/skills/index.cli.test.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/init.test.tssrc/application/commands/skills/init.tssrc/application/commands/skills/managed-skill-publication.test.tssrc/application/commands/skills/managed-skill-publication.tssrc/application/commands/skills/publish.test.tssrc/application/commands/skills/publish.tssrc/application/commands/skills/registry-skill-install.tssrc/application/commands/skills/registry-skill-publication.tssrc/application/commands/skills/shared.test.tssrc/application/commands/skills/shared.tssrc/application/commands/skills/update.test.tssrc/application/commands/skills/update.tssrc/i18n/catalog.ts
💤 Files with no reviewable changes (3)
- src/i18n/catalog.ts
- src/application/commands/skills/update.ts
- src/application/commands/skills/registry-skill-install.ts
| test("treats a copied target as current", async () => { | ||
| const skillDirectoryPath = join( | ||
| tmpdir(), | ||
| `oo-managed-skill-publication-target-${Bun.randomUUIDv7()}`, | ||
| ); | ||
|
|
||
| try { | ||
| await mkdir(skillDirectoryPath, { recursive: true }); | ||
|
|
||
| await expect( | ||
| isManagedSkillPublicationCurrent(skillDirectoryPath), | ||
| ).resolves.toBeTrue(); |
There was a problem hiding this comment.
Don't bless an empty directory as a current managed publication.
This fixture only creates an empty folder, so the test now codifies that any non-symlink directory is "current". That would let partially deleted or otherwise corrupted copied installs bypass repair/startup synchronization. Seed the fixture with the required managed files, or keep isManagedSkillPublicationCurrent(...) validating them before returning true.
🤖 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/managed-skill-publication.test.ts` around
lines 12 - 23, The test "treats a copied target as current" currently creates an
empty directory and asserts isManagedSkillPublicationCurrent(...) returns true;
instead, seed the fixture directory with the concrete managed publication files
that isManagedSkillPublicationCurrent expects (e.g., the skill manifest and any
marker files or metadata created for managed installs) before calling
isManagedSkillPublicationCurrent, or alternatively tighten
isManagedSkillPublicationCurrent to validate those files' presence/content
rather than treating any non-symlink directory as current; update the test to
create those files (using mkdir + writeFile) so the assertion reflects a real
managed publication.
1 participant
Remove symlink publication for bundled, registry, and local skills because supported agents handle linked skill directories inconsistently.
Startup synchronization now treats existing managed symlink targets as stale and replaces them with copied directories. Local canonical skills are also synchronized to newly detected hosts without overwriting registry-managed targets.
Need help on this PR? Tag
@codesmithwith what you need.