fix(skills): reconcile managed skill publication form on sync#153
Merged
Conversation
Registry skill startup synchronization and update currency checks previously compared only metadata, so a symlinked target on a copy-only host (e.g., CodeBuddy) was treated as up-to-date even though it violated the host's required publication mode. Add `isManagedSkillPublicationCurrent` and consult it from both the startup sync and `isRegistrySkillCurrentInAllHosts` so a stale symlink triggers republication as a copy when the host requires it. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThis PR adds publication mode-aware synchronization logic for managed skills. A new isManagedSkillPublicationCurrent() helper validates whether a managed skill's filesystem state (symlink vs. copy) matches its intended publication mode. The auto-sync workflow now resolves publication mode per installation, checks metadata consistency, and skips republishing if the skill is already current. The update module's currentness detection now combines metadata equality with publication state validation, ensuring accurate detection across hosts. A new test verifies that symlinked registry skills are properly converted to copies for CodeBuddy during CLI startup. Sequence Diagram(s)sequenceDiagram
participant CLI
participant AutoSync
participant Filesystem
participant Publisher
CLI->>AutoSync: startup triggers registry sync
AutoSync->>Filesystem: readManagedSkillTargetState
AutoSync->>AutoSync: resolveManagedSkillPublicationMode(agentName)
AutoSync->>Filesystem: compare canonical vs installed metadata
alt metadata differs
AutoSync->>Publisher: publishBundledSkillInstallation(publicationMode)
Publisher->>Filesystem: write files (copy or symlink)
else metadata matches
AutoSync->>Filesystem: isManagedSkillPublicationCurrent(path, publicationMode)
alt publication not current
AutoSync->>Publisher: publishBundledSkillInstallation(publicationMode)
Publisher->>Filesystem: write files (copy or symlink)
else publication current
AutoSync->>CLI: skip publishing
end
end
AutoSync->>CLI: log "Registry skill synchronized during CLI startup."
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: 2
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/update.ts (1)
623-655: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd direct
skills updatecoverage for the new publication-currentness check.This path now changes when
skills updatedecides a managed registry skill is already current, but the added test only exercises startup sync. A stale symlink on a copy-only host can still regress here without failing the suite, so this needs a focused update-path test as well.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/update.ts` around lines 623 - 655, Add a focused test for the skills update path that exercises isRegistrySkillCurrentInAllHosts' new publication-currentness check: create a test that sets up a managed registry skill across hosts (including a copy-only host with a stale symlink), stub/spy resolveManagedSkillHostInstallations to return those hosts and stub directoryExists/readManagedSkillMetadata/isManagedSkillPublicationCurrent (and resolveManagedSkillPublicationMode) to simulate both publicationCurrent = true and publicationCurrent = false cases, then call the code path that runs "skills update" and assert that when publicationCurrent is true no update/install is attempted and when false the update/install flow runs; reference the function names isRegistrySkillCurrentInAllHosts, resolveManagedSkillHostInstallations, readManagedSkillMetadata, isManagedSkillPublicationCurrent, and resolveManagedSkillPublicationMode when locating code to stub and where to assert 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/application/commands/skills/auto-sync.ts`:
- Around line 294-321: The current code returns early when a managed target's
packageName or version differs from skill.metadata, preventing stale managed
installs from being reconciled; instead, remove the return in the
metadata-mismatch branch so the republish path runs, and only short-circuit when
metadata matches AND isManagedSkillPublicationCurrent(...) returns true.
Concretely: in the targetState.kind === "managed" block, keep the
context.logger.warn(...) when targetState.metadata differs from skill.metadata
but do not return there; afterwards change the existing short-circuit to: if
(metadata matches && await
isManagedSkillPublicationCurrent(installation.installedSkillDirectoryPath,
publicationMode)) return; ensuring variables referenced (targetState,
skill.metadata.packageName/version, installation, publicationMode,
isManagedSkillPublicationCurrent, context.logger.warn) are used to implement
this flow.
In `@src/application/commands/skills/managed-skill-publication.ts`:
- Around line 24-26: The switch branch for publicationMode "copy" currently
awaits lstat(skillDirectoryPath) which can throw ENOENT; modify this logic to
EAFP-style: wrap the lstat(...) call in a try/catch, return false when the
caught error.code === "ENOENT" (treat missing target as not current), and
rethrow or propagate other errors; keep the existing return for the non-symlink
case when lstat succeeds so callers can republish instead of aborting.
---
Outside diff comments:
In `@src/application/commands/skills/update.ts`:
- Around line 623-655: Add a focused test for the skills update path that
exercises isRegistrySkillCurrentInAllHosts' new publication-currentness check:
create a test that sets up a managed registry skill across hosts (including a
copy-only host with a stale symlink), stub/spy
resolveManagedSkillHostInstallations to return those hosts and stub
directoryExists/readManagedSkillMetadata/isManagedSkillPublicationCurrent (and
resolveManagedSkillPublicationMode) to simulate both publicationCurrent = true
and publicationCurrent = false cases, then call the code path that runs "skills
update" and assert that when publicationCurrent is true no update/install is
attempted and when false the update/install flow runs; reference the function
names isRegistrySkillCurrentInAllHosts, resolveManagedSkillHostInstallations,
readManagedSkillMetadata, isManagedSkillPublicationCurrent, and
resolveManagedSkillPublicationMode when locating code to stub and where to
assert 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: 84fcd209-3ac2-4620-a4fd-9dd61f4340c6
📒 Files selected for processing (4)
src/application/commands/skills/auto-sync.tssrc/application/commands/skills/index.cli.test.tssrc/application/commands/skills/managed-skill-publication.tssrc/application/commands/skills/update.ts
Comment on lines
294
to
+321
| if (targetState.kind === "managed") { | ||
| if ( | ||
| targetState.metadata?.packageName === skill.metadata.packageName | ||
| && targetState.metadata.version === skill.metadata.version | ||
| targetState.metadata?.packageName !== skill.metadata.packageName | ||
| || targetState.metadata.version !== skill.metadata.version | ||
| ) { | ||
| context.logger.warn( | ||
| { | ||
| agentName: installation.agentName, | ||
| canonicalPackageName: skill.metadata.packageName, | ||
| canonicalVersion: skill.metadata.version, | ||
| path: installation.installedSkillDirectoryPath, | ||
| skillName: skill.name, | ||
| targetPackageName: targetState.metadata?.packageName, | ||
| targetVersion: targetState.metadata?.version, | ||
| }, | ||
| "Registry skill startup synchronization skipped because the target metadata does not match canonical metadata.", | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| context.logger.warn( | ||
| { | ||
| agentName: installation.agentName, | ||
| canonicalPackageName: skill.metadata.packageName, | ||
| canonicalVersion: skill.metadata.version, | ||
| path: installation.installedSkillDirectoryPath, | ||
| skillName: skill.name, | ||
| targetPackageName: targetState.metadata?.packageName, | ||
| targetVersion: targetState.metadata?.version, | ||
| }, | ||
| "Registry skill startup synchronization skipped because the target metadata does not match canonical metadata.", | ||
| ); | ||
| return; | ||
| if ( | ||
| await isManagedSkillPublicationCurrent( | ||
| installation.installedSkillDirectoryPath, | ||
| publicationMode, | ||
| ) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Metadata mismatch should trigger republishing, not an early skip.
This branch now returns when a managed target’s packageName or version differs from canonical metadata. That prevents startup sync from reconciling stale managed installations at all; only the “metadata matches and publication is current” case should short-circuit.
💡 Suggested fix
if (targetState.kind === "managed") {
- if (
- targetState.metadata?.packageName !== skill.metadata.packageName
- || targetState.metadata.version !== skill.metadata.version
- ) {
- context.logger.warn(
- {
- agentName: installation.agentName,
- canonicalPackageName: skill.metadata.packageName,
- canonicalVersion: skill.metadata.version,
- path: installation.installedSkillDirectoryPath,
- skillName: skill.name,
- targetPackageName: targetState.metadata?.packageName,
- targetVersion: targetState.metadata?.version,
- },
- "Registry skill startup synchronization skipped because the target metadata does not match canonical metadata.",
- );
- return;
- }
+ const metadataCurrent =
+ targetState.metadata?.packageName === skill.metadata.packageName
+ && targetState.metadata.version === skill.metadata.version;
if (
+ metadataCurrent
+ &&
await isManagedSkillPublicationCurrent(
installation.installedSkillDirectoryPath,
publicationMode,
)
) {
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 294 - 321, The
current code returns early when a managed target's packageName or version
differs from skill.metadata, preventing stale managed installs from being
reconciled; instead, remove the return in the metadata-mismatch branch so the
republish path runs, and only short-circuit when metadata matches AND
isManagedSkillPublicationCurrent(...) returns true. Concretely: in the
targetState.kind === "managed" block, keep the context.logger.warn(...) when
targetState.metadata differs from skill.metadata but do not return there;
afterwards change the existing short-circuit to: if (metadata matches && await
isManagedSkillPublicationCurrent(installation.installedSkillDirectoryPath,
publicationMode)) return; ensuring variables referenced (targetState,
skill.metadata.packageName/version, installation, publicationMode,
isManagedSkillPublicationCurrent, context.logger.warn) are used to implement
this flow.
Treat disappearing copy-mode skill targets as not current so sync and update paths can republish instead of aborting on ENOENT. This keeps the publication-form reconciliation robust against filesystem races. Signed-off-by: Kevin Cui <bh@bugs.cc>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 31-37: Replace the hardcoded POSIX "/tmp/..." with a platform-safe
temp path: import os and path (node:os, node:path) in the test and construct the
target using path.join(os.tmpdir(),
"oo-missing-managed-skill-publication-target") (or use fs.mkdtemp for a unique
dir) and pass that value to isManagedSkillPublicationCurrent so the test no
longer assumes POSIX separators.
🪄 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: e714f0ed-96c0-46ca-a88d-5d7eebebf5fa
📒 Files selected for processing (2)
src/application/commands/skills/managed-skill-publication.test.tssrc/application/commands/skills/managed-skill-publication.ts
Build the missing publication target path from the platform temp directory and a v7 UUID so the test stays portable across operating systems. Signed-off-by: Kevin Cui <bh@bugs.cc>
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.
Registry skill startup synchronization and update currency checks previously compared only metadata, so a symlinked target on a copy-only host (e.g., CodeBuddy) was treated as up-to-date even though it violated the host's required publication mode.
Add
isManagedSkillPublicationCurrentand consult it from both the startup sync andisRegistrySkillCurrentInAllHostsso a stale symlink triggers republication as a copy when the host requires it.