fix(init): use skill references for tools without a command adapter#1404
fix(init): use skill references for tools without a command adapter#1404clay-good wants to merge 6 commits into
Conversation
Adapterless tools (kimi, vibe, hermes, forgecode, codeartsagent, agents) skip command generation even under the default 'both' delivery, but their generated SKILL.md files still told agents to run /opsx:* commands that were never created, and the init summary suggested /opsx:propose. Route the existing skill-reference transform by command-surface capability so these tools get /openspec-* references, and point the getting-started hint at the skill when no selected tool got commands. Fixes #1155 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSkill and command reference transformation now accounts for each tool’s command-surface capability. Initialization, update, migration, and skills.sh generation produce skill-compatible references when commands are unavailable. ChangesCapability-aware skill reference generation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant InitCommand
participant CapabilityResolver
participant TransformerSelector
participant SkillGenerator
participant ConsoleOutput
InitCommand->>CapabilityResolver: resolve tool command surface
CapabilityResolver->>TransformerSelector: provide delivery and capability
TransformerSelector->>SkillGenerator: select skill or command reference transformation
SkillGenerator-->>InitCommand: generate transformed skill content
InitCommand->>ConsoleOutput: print matching start reference
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alfred-openspec
left a comment
There was a problem hiding this comment.
The capability-only transform breaks Kimi’s documented invocation contract: Kimi skills use /skill:openspec-* (see docs/supported-tools.md, docs/how-commands-work.md, and docs/commands.md), but this head rewrites both generated cross-references and the getting-started hint to /openspec-*. Please make the skill-reference transform tool-specific and add an end-to-end Kimi assertion for the actual invocation syntax.
…references - transform the committed skills.sh distribution too: pass transformToSkillReferences in generate-skillssh.mjs and the parity test, regenerate skills/ (that channel installs SKILL.md files only, so /opsx:* commands never exist there) - key the getting-started hint purely on whether any selected tool got commands, so the delivery=commands + adapterless corner can no longer print /opsx:propose - make the one-time profile-migration message capability-aware for projects whose detected tools have no command adapter - import CommandSurfaceCapability type-only instead of duplicating the union inline (a value import would close a module cycle) - cover the update path: the kimi migration test now asserts refreshed skills contain no /opsx references Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per review: the blanket /openspec-* rewrite contradicted Kimi's documented invocation contract (/skill:openspec-*, see docs/supported-tools.md). Skill-reference transforms are now selected per tool via getSkillReferenceTransformer, with Kimi mapped to /skill:<name> and every other tool keeping the documented /<name> form; the getting-started hint and migration message use the same per-tool syntax. End-to-end Kimi assertions cover generated skill content, the refreshed update path, and the hint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed in 881d893: skill-reference transforms are now selected per tool via Note this also fixes a pre-existing docs mismatch: under |
alfred-openspec
left a comment
There was a problem hiding this comment.
The Kimi /skill: prefix is fixed at 881d893, but the commands-only adapterless corner still prints a dead getting-started instruction. Reproduced with global delivery=commands plus openspec init --tools kimi: setup creates neither skills nor commands, then tells the user to run /skill:openspec-propose. Please gate the hint on an actually generated surface (or print a configuration correction) and add an end-to-end commands-only adapterless assertion.
Per review: with delivery=commands and only adapterless tools selected, init generated neither skills nor commands yet still advertised an invocation. Print a configuration correction instead, with the exact 'openspec config set delivery both' remedy, covered by an end-to-end commands-only adapterless test. Also from the adversarial review round: mixed selections that disagree on invocation syntax (kimi + vibe) now fall back to the default /openspec-* form in the shared hint and migration message instead of picking the first tool's syntax; add the missing changeset; correct the codex doc comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed in 8f6d324:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/migration.ts`:
- Around line 215-217: Update the transformer-selection logic around
skillTransformers in migration.ts and the corresponding initialization logic in
init.ts to compare each transformer’s output for the same dummy input rather
than function identity. Use the shared transformer when all outputs agree,
including independently created closures with the same prefix; otherwise retain
the transformToSkillReferences fallback and existing command-surface behavior.
In `@test/core/init.test.ts`:
- Line 986: The delivery=commands test at test/core/init.test.ts:986-986 and the
mixed adapterless tools test at test/core/init.test.ts:1013-1013 each declare
logCalls twice consecutively; remove one duplicate declaration from each test
while retaining the single declaration used by its assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fbf35083-deb1-46a4-ab0b-b5064d9d50bf
📒 Files selected for processing (5)
.changeset/adapterless-skill-references.mdsrc/core/init.tssrc/core/migration.tssrc/utils/command-references.tstest/core/init.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/command-references.ts
- src/core/init.ts
| expect(await fileExists(path.join(testDir, '.kimi-code', 'skills', 'openspec-explore', 'SKILL.md'))).toBe(false); | ||
| expect(await fileExists(path.join(testDir, '.kimi-code', 'commands'))).toBe(false); | ||
|
|
||
| const logCalls = (console.log as unknown as { mock: { calls: unknown[][] } }).mock.calls.flat().map(String); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove duplicate constant declarations.
The variable logCalls is declared twice consecutively in these test cases, which will cause a SyntaxError: Identifier 'logCalls' has already been declared and break the test build.
test/core/init.test.ts#L986-L986: Remove the duplicatedlogCallsdeclaration in the delivery=commands test.test/core/init.test.ts#L1013-L1013: Remove the duplicatedlogCallsdeclaration in the mixed adapterless tools test.
📍 Affects 1 file
test/core/init.test.ts#L986-L986(this comment)test/core/init.test.ts#L1013-L1013
🤖 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 `@test/core/init.test.ts` at line 986, The delivery=commands test at
test/core/init.test.ts:986-986 and the mixed adapterless tools test at
test/core/init.test.ts:1013-1013 each declare logCalls twice consecutively;
remove one duplicate declaration from each test while retaining the single
declaration used by its assertions.
From the third adversarial review round: the 'Restart your IDE for slash commands' line printed directly after the message saying nothing was generated. Gate it on an actually generated surface and pin that in the commands-only adapterless test. Also: use randomUUID() for init test temp dirs (matches update.test.ts, removes a theoretical Date.now collision), and clarify the changeset wording about the skills.sh channel's default reference form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Third adversarial review round complete (three independent passes: newest-commit correctness, cross-file consistency, break-nothing gate — all MERGE-SAFE/SHIP). Residual fixes in 70617b3: the 'Restart your IDE' line is now suppressed when nothing was generated (it printed directly after the configuration correction), init test temp dirs use randomUUID(), and the changeset wording clarifies the skills.sh channel uses the default |
alfred-openspec
left a comment
There was a problem hiding this comment.
The commands-only dead hint is fixed at 86155d0, but the new mixed-syntax fallback deliberately reintroduces an invalid instruction for Kimi. Reproduced openspec init --tools kimi,vibe: both skill trees are generated correctly, then the shared setup summary says /openspec-propose, which Vibe accepts but Kimi does not (Kimi requires /skill:openspec-propose). Please emit per-tool hints or a syntax-neutral instruction instead of choosing a form that is wrong for one configured tool; the current mixed-tool test should assert every advertised instruction is usable, not codify the fallback.
Per review: the mixed-syntax fallback advertised /openspec-propose,
which Mistral Vibe accepts but Kimi Code does not. Group successful
tools by their transformed reference and print one labeled hint line
per distinct form, so every advertised instruction is usable by the
tool it names; the mixed-tool test asserts exactly that. The migration
message compares transformed outputs instead of function identities
(also per review) and stays syntax-neutral ('the openspec-propose
skill') when detected tools disagree.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed in 36997e8:
|
Status
Ready for review. Fixes #1155. Hardened by three independent adversarial review passes; full suite green locally (2,034 passed; the only failures are the 17 known environment-only
zsh-installerones covered by #1400).What was wrong
Tools with no command adapter — Kimi Code, Mistral Vibe, Hermes, ForgeCode, CodeArts Agent — skip command generation even under the default
bothdelivery (Commands skipped for: kimi (no adapter)). But the skills generated for them still told agents to run/opsx:apply,/opsx:archive,/opsx:continue, … — commands that were never created — and the init summary ended withStart your first change: /opsx:propose. The committedskills/tree (thenpx skills add Fission-AI/OpenSpecchannel) had the same problem: it ships SKILL.md files only, yet referenced/opsx:*throughout. Reported in #1155 (the "5 commands" count from that report was already fixed on main; the references and the hint were not).#1194 fixed this for the global
delivery: skillssetting, but the per-tool case (deliveryboth, tool has no adapter) was left out:getTransformerForToolonly looked at delivery, not at the tool's command surface.How it was fixed
getTransformerForToolnow also takes the tool's command-surface capability (type-only import ofCommandSurfaceCapability) and returns a skill-reference transform when the capability isnone, honoring each tool's documented invocation syntax (Kimi Code:/skill:openspec-*; all others:/openspec-*per fix: use skill references in SKILL.md for skills-only delivery #1194). All three call sites (init + update ×2) passresolveCommandSurfaceCapability(tool.value)./skill:openspec-propose "your idea"for Kimi Code) instead of a command that doesn't exist. When mixed selections disagree on syntax (kimi + vibe), init prints one labeled hint per distinct form so every advertised instruction is usable by the tool it names; the one-time migration message stays syntax-neutral in that case. When nothing was generated at all (delivery: commands+ adapterless tools only), init prints a configuration correction (openspec config set delivery both) instead of any invocation hint, and suppresses the "Restart your IDE" line.scripts/generate-skillssh.mjsand its parity test apply the transform, andskills/is regenerated — the diff there is purely the reference swap (35 lines).Codex SKILL.md content is deliberately unchanged (
skills-invocablenever triggers the new branch) — #1143 owns codex reference rewriting. The shared getting-started hint does improve for codex-only setups too, since codex lost its/opsxprompts in #1283 and the old hint pointed at them.Replication / proof
Before (clean build of main):
After (this branch):
Regression tests: adapterless tool under default delivery gets skill references and the skill-based hint; adapter-backed tool (claude) keeps
/opsx:*in both skills and hint; the kimi update-path test asserts refreshed skills contain no/opsx(mutation-verified: reverting the update.ts change now fails tests); transformer unit tests cover all three capabilities; skills.sh parity test pins the transformed output.Notes
skills/diff is reference-swaps only.dist/index.js, which never re-exportsgetTransformerForTool.delivery: commands+ adapterless-only tools still generates nothing (the misleading hint is now replaced with a configuration correction, but whether that delivery setting should fall back to skills for such tools is a design question — worth its own issue;openspec updatein that same config silently removes the skills and reports success, also pre-existing); core-profile skills reference workflows outside the profile (affects adapter-backed tools identically); the unconditional "Restart your IDE for slash commands" line (fix: only show 'Restart your IDE' message for IDE-embedded tools #1097); the interactive welcome screen's/opsxexamples;inferDeliverytreating an adapterless project's lack of commands as a skills-only preference.🤖 Generated with Claude Code
Summary by CodeRabbit
/opsx-*vs/openspec-*//skill:openspec-*targets.openspec-*/skill:openspec-*references instead of unused/opsx:*variants.