fix(skills): preserve same-version legacy targets during auto-sync#195
Merged
Conversation
Startup synchronization previously rewrote oo-managed legacy symlink targets and re-published skills whenever the publication state was considered stale, even when the installed version already matched the running CLI. This produced unexpected churn for users who had not asked to migrate legacy installations. Auto-sync now leaves same-version targets alone and only adopts legacy matching local copies by writing metadata in place. Bundled skill targets installed at the development version are also skipped so local development copies stay untouched across release-version startups. Use `oo skills add` for bundled and local skills, and `oo skills update` for registry skills, to migrate legacy symlinks explicitly. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThis PR refines the managed skill startup synchronization logic by removing dependency on "publication current" state checks and replacing them with direct metadata/version matching. It adds explicit handling to skip synchronization when the running CLI or installed skill is a development build. The changes preserve symlinks instead of converting them to copies and require users to run explicit 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.
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/auto-sync.ts (1)
479-493:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove the “adopted” log after metadata writes complete.
At Line 482 the info log is emitted before
writeLocalSkillMetadata(...)finishes. If either write fails, logs can contain both “adopted” and “failed” for the same attempt.Suggested fix
if ( metadataState.metadata === undefined && !metadataState.exists ) { - context.logger.info( - { - agentName: installation.agentName, - path: installation.installedSkillDirectoryPath, - skillName: skill.name, - }, - "Local skill startup synchronization adopted a legacy matching target.", - ); - await Promise.all([ - writeLocalSkillMetadata(skill.path), - writeLocalSkillMetadata(installation.installedSkillDirectoryPath), - ]); + await writeLocalSkillMetadata(skill.path); + await writeLocalSkillMetadata( + installation.installedSkillDirectoryPath, + ); + context.logger.info( + { + agentName: installation.agentName, + path: installation.installedSkillDirectoryPath, + skillName: skill.name, + }, + "Local skill startup synchronization adopted a legacy matching target.", + ); return; }🤖 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/auto-sync.ts` around lines 479 - 493, The info log about "Local skill startup synchronization adopted a legacy matching target" is currently emitted before the two async writes complete; move the context.logger.info call so it runs after awaiting Promise.all([writeLocalSkillMetadata(skill.path), writeLocalSkillMetadata(installation.installedSkillDirectoryPath)]), keeping the same log payload (agentName, path, skillName) and message, so the "adopted" message is only logged on successful completion of writeLocalSkillMetadata for both targets (refer to metadataState, installation, skill, writeLocalSkillMetadata and context.logger).
🧹 Nitpick comments (1)
src/application/commands/skills/index.cli.test.ts (1)
372-419: ⚡ Quick winAdd an assertion for metadata adoption in the local-symlink startup test.
This test verifies symlink preservation, but the new behavior also adopts legacy matching targets by writing local metadata. Please assert
.oo-metadata.jsonexists after startup to lock that behavior.Suggested test assertion
expect((await lstat(codexSkillDirectoryPath)).isSymbolicLink()).toBeTrue(); expect(await readFile(join(codexSkillDirectoryPath, "SKILL.md"), "utf8")).toBe( "# Campaign Writer\n", ); + await expect( + stat(join(canonicalSkillDirectoryPath, ".oo-metadata.json")), + ).resolves.toMatchObject({ + isFile: expect.any(Function), + }); expect(content).not.toContain( `"msg":"Local skill synchronized during CLI startup."`, );As per coding guidelines, "Any modification must include sufficient tests".
🤖 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.cli.test.ts` around lines 372 - 419, The test "leaves synchronized local symlink targets unchanged during cli startup" currently verifies symlink preservation but doesn't assert that the new metadata adoption behavior occurred; update this test (in the same test function) to check that the local metadata file ".oo-metadata.json" has been created inside the local symlink target after startup by asserting its existence/readability at join(codexSkillDirectoryPath, ".oo-metadata.json") (use existing helpers like readFile or stat/exists as used elsewhere) so the test locks in the metadata-adoption behavior introduced by the startup synchronization code.
🤖 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.
Outside diff comments:
In `@src/application/commands/skills/auto-sync.ts`:
- Around line 479-493: The info log about "Local skill startup synchronization
adopted a legacy matching target" is currently emitted before the two async
writes complete; move the context.logger.info call so it runs after awaiting
Promise.all([writeLocalSkillMetadata(skill.path),
writeLocalSkillMetadata(installation.installedSkillDirectoryPath)]), keeping the
same log payload (agentName, path, skillName) and message, so the "adopted"
message is only logged on successful completion of writeLocalSkillMetadata for
both targets (refer to metadataState, installation, skill,
writeLocalSkillMetadata and context.logger).
---
Nitpick comments:
In `@src/application/commands/skills/index.cli.test.ts`:
- Around line 372-419: The test "leaves synchronized local symlink targets
unchanged during cli startup" currently verifies symlink preservation but
doesn't assert that the new metadata adoption behavior occurred; update this
test (in the same test function) to check that the local metadata file
".oo-metadata.json" has been created inside the local symlink target after
startup by asserting its existence/readability at join(codexSkillDirectoryPath,
".oo-metadata.json") (use existing helpers like readFile or stat/exists as used
elsewhere) so the test locks in the metadata-adoption behavior introduced by the
startup synchronization code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40a7d8f3-36f5-44a0-8d27-438d646c498f
📒 Files selected for processing (4)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/auto-sync.tssrc/application/commands/skills/index.cli.test.ts
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.
Startup synchronization previously rewrote oo-managed legacy symlink targets and re-published skills whenever the publication state was considered stale, even when the installed version already matched the running CLI. This produced unexpected churn for users who had not asked to migrate legacy installations.
Auto-sync now leaves same-version targets alone and only adopts legacy matching local copies by writing metadata in place. Bundled skill targets installed at the development version are also skipped so local development copies stay untouched across release-version startups. Use
oo skills addfor bundled and local skills, andoo skills updatefor registry skills, to migrate legacy symlinks explicitly.