feat(skills): summarize install output by skill and agent#125
Merged
Conversation
Previously each managed skill publication printed its own line, which produced a wall of near-identical output when installing several skills across multiple agents. The new compact summary aggregates publications and renders one of four shapes depending on whether a single or multiple skills were installed across one or multiple agents, with optional ANSI colors when stdout supports them. Also extract the agent label lookup into managed-skill-host-labels so both the list and install commands share a single source of truth, and update the docs and snapshots to reflect the new output shape. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThis PR refactors the skills install command's output behavior to emit a consolidated summary instead of per-skill/per-agent messages. The changes introduce new data structures ( Sequence DiagramsequenceDiagram
participant CliCmd as Skills Install Command
participant BundledInstaller as Bundled Skill Installer
participant RegistryInstaller as Registry Skill Installer
participant SummaryWriter as Summary Writer
participant Stdout as Terminal Output
CliCmd->>BundledInstaller: installBundledSkill(skillName)
BundledInstaller->>BundledInstaller: Install to each agent, collect publications
BundledInstaller-->>CliCmd: return ManagedSkillInstallSummary
CliCmd->>RegistryInstaller: installRegistrySkills(...)
RegistryInstaller->>RegistryInstaller: Execute installs, accumulate results
RegistryInstaller-->>CliCmd: return ManagedSkillInstallSummary[]
CliCmd->>CliCmd: Aggregate all summaries
CliCmd->>SummaryWriter: writeManagedSkillInstallSummary(summaries, translator)
SummaryWriter->>SummaryWriter: Select format (single/multi skill/agent)
SummaryWriter->>SummaryWriter: Format with colors and localization
SummaryWriter->>Stdout: Write consolidated summary
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
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. 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 (3)
src/application/commands/skills/install.ts (1)
63-65: Consider parallelizing bundled installs in Line 63–Line 65.These installs are per-skill and can be run concurrently while preserving summary order via
Promise.all(...).♻️ Proposed refactor
- const summaries: ManagedSkillInstallSummary[] = []; - - for (const skillName of availableBundledSkillNames) { - summaries.push(await installBundledSkill(skillName, context)); - } + const summaries: ManagedSkillInstallSummary[] = await Promise.all( + availableBundledSkillNames.map(skillName => + installBundledSkill(skillName, context), + ), + );As per coding guidelines, "Use
Promise.all()for independent async operations instead of sequentialawait".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/install.ts` around lines 63 - 65, The loop that sequentially awaits installBundledSkill for each name should be parallelized: map availableBundledSkillNames to an array of promises by calling installBundledSkill(skillName, context) for each name, run them with Promise.all to await all installs concurrently, and then push or assign the resolved summaries in the original order into the summaries array; update the code around the installBundledSkill calls (where availableBundledSkillNames is iterated) to use Promise.all(...) on the mapped promises so installs run in parallel while preserving summary order.src/application/commands/skills/install-output.test.ts (1)
7-45: Add tests for the other three output-shape branches.Current coverage validates the multi-skill/multi-agent path well, but the single-skill/single-agent, single-skill/multi-agent, and multi-skill/single-agent branches should also be asserted to prevent formatting regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/install-output.test.ts` around lines 7 - 45, The test file only covers the multi-skill/multi-agent branch; add three new tests in src/application/commands/skills/install-output.test.ts that call writeManagedSkillInstallSummary with appropriate summary fixtures and translators to cover: single-skill/single-agent, single-skill/multi-agent, and multi-skill/single-agent branches. Use the same helpers (createTextBuffer, createTranslator) and either add or reuse fixture creators (e.g., createSingleSkillSingleAgentSummary, createSingleSkillMultiAgentSummary, createMultiSkillSingleAgentSummary) to produce the different output shapes, then assert the expected stdout lines for presence/absence of "Agents:" and "Skills:" lines plus correct pluralization on the first line (e.g., "skill" vs "skills") to prevent formatting regressions when writeManagedSkillInstallSummary runs.src/application/commands/skills/install-output.ts (1)
54-55: Consider deduplicating skill names before counting/rendering.If duplicate summaries for the same skill are ever passed in, the summary count and
Skills:line will be inflated.Suggested diff
- const skillNames = completedSummaries.map(summary => summary.name); + const skillNames = [ + ...new Set(completedSummaries.map(summary => summary.name)), + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/install-output.ts` around lines 54 - 55, The code builds skillNames from completedSummaries without deduplication which can inflate counts and the "Skills:" line; update the logic in install-output.ts to derive unique skill names (e.g., use a Set over completedSummaries.map(s => s.name)) and then use that deduplicated array wherever skillNames is used for counting/rendering; keep readUniqueAgentNames unchanged but ensure any places that display or count skills reference the new unique skillNames collection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/application/commands/skills/install-output.test.ts`:
- Around line 54-55: Replace hardcoded POSIX paths in
src/application/commands/skills/install-output.test.ts with platform-safe
construction using Node's path helpers: import { join, resolve } from
"node:path" and build fixture paths (the objects with path:
"/tmp/codex/skills/chatgpt" and the other entries at the noted ranges) via
join/resolve (e.g., resolve(tmpDir, "codex", "skills", "chatgpt")) so tests are
platform-agnostic; update all occurrences referenced in the comment (lines
around the path fields at 54-55, 58-59, 67-68, 71-72) to use these path helpers.
---
Nitpick comments:
In `@src/application/commands/skills/install-output.test.ts`:
- Around line 7-45: The test file only covers the multi-skill/multi-agent
branch; add three new tests in
src/application/commands/skills/install-output.test.ts that call
writeManagedSkillInstallSummary with appropriate summary fixtures and
translators to cover: single-skill/single-agent, single-skill/multi-agent, and
multi-skill/single-agent branches. Use the same helpers (createTextBuffer,
createTranslator) and either add or reuse fixture creators (e.g.,
createSingleSkillSingleAgentSummary, createSingleSkillMultiAgentSummary,
createMultiSkillSingleAgentSummary) to produce the different output shapes, then
assert the expected stdout lines for presence/absence of "Agents:" and "Skills:"
lines plus correct pluralization on the first line (e.g., "skill" vs "skills")
to prevent formatting regressions when writeManagedSkillInstallSummary runs.
In `@src/application/commands/skills/install-output.ts`:
- Around line 54-55: The code builds skillNames from completedSummaries without
deduplication which can inflate counts and the "Skills:" line; update the logic
in install-output.ts to derive unique skill names (e.g., use a Set over
completedSummaries.map(s => s.name)) and then use that deduplicated array
wherever skillNames is used for counting/rendering; keep readUniqueAgentNames
unchanged but ensure any places that display or count skills reference the new
unique skillNames collection.
In `@src/application/commands/skills/install.ts`:
- Around line 63-65: The loop that sequentially awaits installBundledSkill for
each name should be parallelized: map availableBundledSkillNames to an array of
promises by calling installBundledSkill(skillName, context) for each name, run
them with Promise.all to await all installs concurrently, and then push or
assign the resolved summaries in the original order into the summaries array;
update the code around the installBundledSkill calls (where
availableBundledSkillNames is iterated) to use Promise.all(...) on the mapped
promises so installs run in parallel while preserving summary order.
🪄 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: 84cc584b-ea76-447e-9c57-eff3f68818d6
⛔ Files ignored due to path filters (1)
src/application/commands/skills/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/index.cli.test.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/install-output.test.tssrc/application/commands/skills/install-output.tssrc/application/commands/skills/install.tssrc/application/commands/skills/list.tssrc/application/commands/skills/managed-skill-host-labels.tssrc/application/commands/skills/registry-skill-install.tssrc/application/commands/skills/shared.tssrc/i18n/catalog.ts
Comment on lines
+54
to
+55
| path: "/tmp/codex/skills/chatgpt", | ||
| }, |
There was a problem hiding this comment.
Use platform-safe path construction in test fixtures.
These fixture paths are hardcoded with /. Please build them with node:path helpers to keep tests platform-agnostic.
Suggested diff
+import { join } from "node:path";
import { describe, expect, test } from "bun:test";
@@
{
agentName: "codex",
- path: "/tmp/codex/skills/chatgpt",
+ path: join("tmp", "codex", "skills", "chatgpt"),
},
{
agentName: "claude",
- path: "/tmp/claude/skills/chatgpt",
+ path: join("tmp", "claude", "skills", "chatgpt"),
},
@@
{
agentName: "codex",
- path: "/tmp/codex/skills/vision",
+ path: join("tmp", "codex", "skills", "vision"),
},
{
agentName: "claude",
- path: "/tmp/claude/skills/vision",
+ path: join("tmp", "claude", "skills", "vision"),
},As per coding guidelines, "Never assume POSIX path separators in code, tests, snapshots, or assertions; use node:path helpers (join(), resolve(), relative()) for path construction".
Also applies to: 58-59, 67-68, 71-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/commands/skills/install-output.test.ts` around lines 54 - 55,
Replace hardcoded POSIX paths in
src/application/commands/skills/install-output.test.ts with platform-safe
construction using Node's path helpers: import { join, resolve } from
"node:path" and build fixture paths (the objects with path:
"/tmp/codex/skills/chatgpt" and the other entries at the noted ranges) via
join/resolve (e.g., resolve(tmpDir, "codex", "skills", "chatgpt")) so tests are
platform-agnostic; update all occurrences referenced in the comment (lines
around the path fields at 54-55, 58-59, 67-68, 71-72) to use these path helpers.
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.
Previously each managed skill publication printed its own line, which produced a wall of near-identical output when installing several skills across multiple agents. The new compact summary aggregates publications and renders one of four shapes depending on whether a single or multiple skills were installed across one or multiple agents, with optional ANSI colors when stdout supports them.
Also extract the agent label lookup into managed-skill-host-labels so both the list and install commands share a single source of truth, and update the docs and snapshots to reflect the new output shape.