feat(skills): add JSON output to install/uninstall/update/sync#238
Merged
Conversation
Add `--json` / `--format json` to the four skills mutation commands (`install`, `uninstall`, `update`, `sync upload`, `sync apply`) so that callers can drive them from scripts and other agents without parsing localized human-readable text. The four install-style commands share a single envelope with `command`, `status`, `summary`, `skills[]` (with per-agent `targets[]`), and top-level `errors[]`. `sync upload` uses `records[]` instead, because its operation unit is a sync record rather than an agent-side install target. `--json` implies non-interactive: `install` returns a top-level `confirmation_required` error instead of prompting, and all commands report stable English `error.code` enums so consumers can branch on machine-readable codes rather than message text. Argument errors still exit 2 without JSON; other failures emit the payload and exit 1. Also extends `telemetry-decisions.test.ts` to cover the new option shape and adds CLI integration tests for each command's JSON path. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughThis PR adds structured JSON output support across five 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: 6
🧹 Nitpick comments (1)
src/application/commands/skills/sync.cli.test.ts (1)
26-56: ⚡ Quick winMove the local test helper to the bottom and use it to absorb the repeated setup.
seedRegistrySkillis file-local today, so it should live at the end of this test file; that also gives you a natural place to fold the repeated sandbox/auth/host setup into one factory instead of repeating it across cases.As per coding guidelines,
In test files, extract repeated setup (mock, stub, or setup objects) into a local factory function at the bottom of the file.andIf a test helper function might be called by other test files, place it in the __tests__/helpers.ts file. Otherwise, place the function in the test file at the end.🤖 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/sync.cli.test.ts` around lines 26 - 56, The helper function seedRegistrySkill should be moved to the bottom of the test file and used to centralize repeated sandbox/auth/host setup: relocate seedRegistrySkill below the tests, extract the repeated createCliSandbox + sandbox/env + host setup into a small local factory (e.g., makeTestSandbox or makeSandboxWithAuthAndHost) that calls createCliSandbox and returns {sandbox, env, hostDirectory, canonicalDirectory} so tests call that factory and then seedRegistrySkill; if this helper will be reused by other test files, instead move it into __tests__/helpers.ts and export it.
🤖 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/install.ts`:
- Around line 235-248: installPresetSkillPackages currently swallows and logs
preset-package exceptions so the caller (the block using
installPresetSkillPackages, buildRegistrySkillResult, and buildReport) can
finish with no errors[]; change the flow so preset-package failures are
propagated into the report: modify installPresetSkillPackages (and the other
call site at the similar block around the other occurrence) to return structured
failure results or throw up errors that the caller can catch, then in the caller
loop convert each failure into an entry in the errors array (include packageName
and error.message) before calling buildReport(skills, errors, skills.length);
finally add a regression test exercising the no-argument `skills install --json`
path that asserts failing preset installs appear in the JSON errors array.
In `@src/application/commands/skills/sync.ts`:
- Around line 260-309: Wrap the entire body of runSyncUploadJsonReport in a
top-level try/catch (mirroring the pattern used in runSyncApplyJsonReport) so
any unexpected exception (e.g., thrown by resolveAvailableManagedSkillHosts or
collectRegistrySkillSyncRecords) is caught and converted into a machine-readable
SyncUploadReport via buildSyncUploadReport; in the catch convert the error using
toUploadError(error) (or similar logic used for known failures), push it into
the errors array and return buildSyncUploadReport(recordResults ?? [], ignored
?? 0, errors) to guarantee --json always emits a valid JSON report.
- Around line 441-513: The catch path in applySingleSyncRecord currently
discards any per-target successes by returning targets: [] after
installRegistrySkills throws; inspect installRegistrySkills (and any error type
it throws) to see if it can return partial summaries/publications on failure,
and if so change the catch to extract that partial result (e.g., from a thrown
error object or a returned summary variable) and merge successful publication
entries into the returned SkillResult.targets instead of an empty array;
specifically preserve publication objects (summary.publications ->
agentName/path) as targets with status "installed" and previousState "absent",
and only omit or mark failed agents, while still mapping the error using
mapApplyInstallErrorCode and applyInstallErrorMessage to populate
error.code/message.
In `@src/application/commands/skills/uninstall.ts`:
- Around line 532-541: The current catch swallows
removePath(canonicalDirectoryPath) failures (using context.logger.warn) which
leaves the command reporting status: "removed"; change the catch to treat the
directory removal as a hard failure: replace the warn-and-continue with logging
the error via context.logger.error (including err and skillName) and rethrow (or
throw a new Error that includes the original error and canonicalDirectoryPath)
so the uninstall flow fails and the final JSON result reflects a failed
uninstall instead of "removed".
- Around line 362-376: The uninstall result currently only fails when
hasUnmanaged && removedCount === 0, which lets mixed removals report "removed";
change the logic so any presence of an unmanaged target makes the skill report
failure (or a "partial" state if you prefer) regardless of removedCount: update
the conditional(s) that check hasUnmanaged and removedCount (the block that
returns an object with skillId, kind, previousVersion, version, status, targets,
error using uninstallErrorMessages.not_managed and the similar block later
around lines 551-568) to return status:"failed" (or status:"partial" plus an
appropriate error) whenever hasUnmanaged is true, keeping other fields
(skillName/lastRemovedVersion/targets) intact and reuse the same error payload.
In `@src/application/commands/skills/update.ts`:
- Around line 1010-1026: The pre-scan loop over installations calls
readManagedSkillMetadata() and isManagedSkillPublicationCurrent() unguarded and
can throw if the host directory is missing; mirror the guard used in
isRegistrySkillCurrentInAllHosts() by checking
directoryExists(installation.installedSkillDirectoryPath) before calling those
functions and, if the directory is absent, return a HostVersionState for that
host with agentName set, installedPath set to null (or the path if you prefer),
previousPackageName and previousVersion null, and publicationCurrent false;
otherwise proceed to call readManagedSkillMetadata and
isManagedSkillPublicationCurrent as currently written (refer to the
installations map, readManagedSkillMetadata, isManagedSkillPublicationCurrent,
and HostVersionState symbols to locate the code).
---
Nitpick comments:
In `@src/application/commands/skills/sync.cli.test.ts`:
- Around line 26-56: The helper function seedRegistrySkill should be moved to
the bottom of the test file and used to centralize repeated sandbox/auth/host
setup: relocate seedRegistrySkill below the tests, extract the repeated
createCliSandbox + sandbox/env + host setup into a small local factory (e.g.,
makeTestSandbox or makeSandboxWithAuthAndHost) that calls createCliSandbox and
returns {sandbox, env, hostDirectory, canonicalDirectory} so tests call that
factory and then seedRegistrySkill; if this helper will be reused by other test
files, instead move it into __tests__/helpers.ts and export it.
🪄 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: 09b7eaad-d4b2-42e1-bacb-3535653fceb1
📒 Files selected for processing (13)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/install.cli.test.tssrc/application/commands/skills/install.tssrc/application/commands/skills/operation-result.tssrc/application/commands/skills/sync.cli.test.tssrc/application/commands/skills/sync.tssrc/application/commands/skills/uninstall.cli.test.tssrc/application/commands/skills/uninstall.tssrc/application/commands/skills/update.cli.test.tssrc/application/commands/skills/update.tssrc/application/commands/telemetry-decisions.test.tssrc/i18n/catalog.ts
The upload and apply JSON paths in `oo skills sync` only guarded
against errors raised by their inner steps. Any unexpected throw
outside those guards — for example an ENOTDIR when the skills home
path is not a directory — would bubble up and risk leaking raw
error text into stdout, breaking the documented JSON contract.
Wrap `runSyncUploadJsonReport` and `runSyncApplyJsonReport` with an
outer try/catch that logs the raw error and returns a stable report
carrying a single `{ code: "unknown" }` entry. Add a CLI test that
forces ENOTDIR on the upload path to verify the safety net and to
assert that raw error text never appears in the JSON payload.
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.
Add
--json/--format jsonto the four skills mutation commands (install,uninstall,update,sync upload,sync apply) so that callers can drive them from scripts and other agents without parsing localized human-readable text.The four install-style commands share a single envelope with
command,status,summary,skills[](with per-agenttargets[]), and top-levelerrors[].sync uploadusesrecords[]instead, because its operation unit is a sync record rather than an agent-side install target.--jsonimplies non-interactive:installreturns a top-levelconfirmation_requirederror instead of prompting, and all commands report stable Englisherror.codeenums so consumers can branch on machine-readable codes rather than message text. Argument errors still exit 2 without JSON; other failures emit the payload and exit 1.Also extends
telemetry-decisions.test.tsto cover the new option shape and adds CLI integration tests for each command's JSON path.