fix: use skill references in SKILL.md for skills-only delivery#1194
Conversation
|
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:
📝 WalkthroughWalkthroughAdds delivery-aware reference transformation for skills-only delivery, wires it into init and update SKILL.md generation, re-exports the helpers, and expands unit and integration coverage. ChangesSkills-Only Delivery Reference Transformation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant InitCommand
participant UpdateCommand
participant getTransformerForTool
participant generateSkillContent
InitCommand->>getTransformerForTool: resolve transformer(tool.value, delivery)
getTransformerForTool-->>InitCommand: return delivery-appropriate transformer
InitCommand->>generateSkillContent: generate transformed SKILL.md
UpdateCommand->>getTransformerForTool: resolve transformer(tool.value, delivery)
getTransformerForTool-->>UpdateCommand: return delivery-appropriate transformer
UpdateCommand->>generateSkillContent: generate transformed SKILL.md
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/command-references.ts (1)
22-26: ⚖️ Poor tradeoffSync burden: mapping duplicated across files.
The comment warns that
COMMAND_TO_SKILL_REFERENCEmust stay in sync withWORKFLOW_TO_SKILL_DIRin two other locations. This creates a maintenance risk: if one copy is updated without updating the others, skill references will break at runtime. Consider consolidating these mappings into a single source of truth (e.g., exportingWORKFLOW_TO_SKILL_DIRfrom a shared module and deriving the skill reference map from it).🤖 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/utils/command-references.ts` around lines 22 - 26, The duplicate mapping risk: consolidate the source of truth by exporting WORKFLOW_TO_SKILL_DIR from a single shared module and derive COMMAND_TO_SKILL_REFERENCE from that exported mapping instead of maintaining separate copies; update code that currently defines COMMAND_TO_SKILL_REFERENCE (and the local copies of WORKFLOW_TO_SKILL_DIR) to import the shared WORKFLOW_TO_SKILL_DIR and build the command-to-skill map (e.g., compute COMMAND_TO_SKILL_REFERENCE by mapping keys/values from WORKFLOW_TO_SKILL_DIR) so only one mapping needs to be maintained.src/core/init.ts (1)
540-546: ⚡ Quick winExtract duplicated transformer selection logic into a helper function.
The transformer selection conditional appears identically in 5 locations across 3 files. This violates DRY and creates maintenance risk: if the logic needs to change (e.g., adding a new delivery mode or tool-specific behavior), all 5 sites must be updated consistently.
Extract this logic into a shared helper function in
src/utils/command-references.ts:♻️ Proposed refactor
/** * Selects the appropriate command-reference transformer based on tool and delivery mode. * `@param` toolId - The AI tool identifier (e.g., 'claude', 'opencode', 'pi') * `@param` delivery - The delivery mode ('skills', 'commands', or 'both') * `@returns` The transformer function or undefined */ export function getTransformerForTool( toolId: string, delivery: 'skills' | 'commands' | 'both' ): ((text: string) => string) | undefined { if (toolId === 'opencode' || toolId === 'pi') { return transformToHyphenCommands; } if (delivery === 'skills') { return transformToSkillReferences; } return undefined; }Then replace each of the 5 duplicated blocks with:
const transformer = getTransformerForTool(tool.value, delivery); // or for workspace/skills.ts: const transformer = getTransformerForTool(tool.value, profileContext.delivery);🤖 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/core/init.ts` around lines 540 - 546, Duplicate transformer-selection logic used in multiple places should be pulled into a helper; create a function getTransformerForTool(toolId: string, delivery: 'skills' | 'commands' | 'both') that returns transformToHyphenCommands when toolId === 'opencode' || toolId === 'pi', returns transformToSkillReferences when delivery === 'skills', and otherwise returns undefined, then replace the inline conditional in src/core/init.ts (the const transformer assignment) and the four other occurrences with calls to getTransformerForTool(tool.value, delivery) (or getTransformerForTool(tool.value, profileContext.delivery) where appropriate) so all sites use the single shared implementation.
🤖 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.
Nitpick comments:
In `@src/core/init.ts`:
- Around line 540-546: Duplicate transformer-selection logic used in multiple
places should be pulled into a helper; create a function
getTransformerForTool(toolId: string, delivery: 'skills' | 'commands' | 'both')
that returns transformToHyphenCommands when toolId === 'opencode' || toolId ===
'pi', returns transformToSkillReferences when delivery === 'skills', and
otherwise returns undefined, then replace the inline conditional in
src/core/init.ts (the const transformer assignment) and the four other
occurrences with calls to getTransformerForTool(tool.value, delivery) (or
getTransformerForTool(tool.value, profileContext.delivery) where appropriate) so
all sites use the single shared implementation.
In `@src/utils/command-references.ts`:
- Around line 22-26: The duplicate mapping risk: consolidate the source of truth
by exporting WORKFLOW_TO_SKILL_DIR from a single shared module and derive
COMMAND_TO_SKILL_REFERENCE from that exported mapping instead of maintaining
separate copies; update code that currently defines COMMAND_TO_SKILL_REFERENCE
(and the local copies of WORKFLOW_TO_SKILL_DIR) to import the shared
WORKFLOW_TO_SKILL_DIR and build the command-to-skill map (e.g., compute
COMMAND_TO_SKILL_REFERENCE by mapping keys/values from WORKFLOW_TO_SKILL_DIR) so
only one mapping needs to be maintained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c67c585-1070-4587-8421-be46f4b1dcbb
📒 Files selected for processing (10)
.changeset/skills-only-references.mdsrc/core/init.tssrc/core/update.tssrc/core/workspace/skills.tssrc/utils/command-references.tssrc/utils/index.tstest/core/init.test.tstest/core/update.test.tstest/core/workspace/skills.test.tstest/utils/command-references.test.ts
|
Pushed 4ef4c48 addressing the CodeRabbit review: extracted the duplicated transformer selection into On consolidating the |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for tightening this up. The Claude/init/update/workspace path is in much better shape, but I found one remaining skills-only edge case before this can merge.
getTransformerForTool() currently returns transformToHyphenCommands for OpenCode and Pi before it checks delivery === 'skills'. That means delivery: 'skills' still generates OpenCode/Pi SKILL.md files that point at /opsx-* commands even though command files were removed or never generated. For skills-only delivery, the missing-command fix needs to win for every tool, including OpenCode and Pi. Please make delivery === 'skills' return skill references first, then keep the hyphen transform for OpenCode/Pi when commands are actually generated, and add focused coverage for those two tools.
Checked: focused command-reference/init/update/workspace tests pass locally.
|
Done in 8c66688 — |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Approving latest head. I verified the skills-only transformer now wins for every tool, including OpenCode/Pi, and init/update/workspace skill generation have focused regression coverage. Targeted install, tsc, lint on src, and the relevant Vitest suites all pass.
8c66688 to
3e62cf9
Compare
|
@alfred-openspec I rebased this onto the latest upstream main and resolved the merge conflict; GitHub now reports it as mergeable and CodeRabbit is green again. The previous approval was marked stale after the force-push, so could you please re-confirm when you have a chance? |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-approved after the rebase. I rechecked the diff: skills-only delivery now emits skill references for all tools, OpenCode/Pi precedence is covered, and the affected init/update paths plus transformer helper have focused regression tests. Verified locally with install, tsc, eslint on src, and targeted Vitest suites.
When delivery is configured as 'skills', generated SKILL.md files contained hardcoded /opsx:* command references pointing to commands that were never generated, breaking cross-skill workflows. Add transformToSkillReferences() with the explicit command-to-skill mapping (kept in sync with WORKFLOW_TO_SKILL_DIR) and wire it in init and update wherever skill content is generated, following the approach outlined in Fission-AI#881. Closes Fission-AI#881 Closes Fission-AI#879 Generated with Claude (Cowork) using claude-fable-5; verified with the full vitest suite (1683 tests passing).
Review follow-up for the skills-only delivery fix: workspace skill setup (src/core/workspace/skills.ts) generates SKILL.md via the same generateSkillContent path but was not wired with transformToSkillReferences, leaving dangling /opsx:* references when delivery is 'skills'. Wire both call sites, add a regression test (verified to fail against the unwired code), strengthen the update skills-only test with content assertions, and correct the COMMAND_TO_SKILL_REFERENCE comment (WORKFLOW_TO_SKILL_DIR exists in both profile-sync-drift.ts and init.ts). Generated with Claude (Cowork) using claude-fable-5; verified with eslint and targeted vitest suites (144 tests passing).
…rTool Address CodeRabbit review: the tool/delivery transformer selection was duplicated at five call sites across init.ts, update.ts, and workspace/skills.ts. Extract it into a documented helper in command-references.ts with unit tests locking the selection matrix (opencode/pi precedence, skills-only delivery, default). Generated with Claude (Cowork) using claude-fable-5; verified with eslint, tsc, and targeted vitest suites (147 tests passing).
…ls-only delivery Address review: getTransformerForTool returned transformToHyphenCommands for opencode/pi before checking delivery, so skills-only delivery still emitted /opsx-* references to commands that were never generated. Check delivery === 'skills' first so skill references win for every tool, and keep the hyphen transform for opencode/pi only when commands are generated. Add unit coverage for the opencode/pi selection matrix and an opencode skills-only init integration test asserting no /opsx: or /opsx- references remain. Generated with Claude (Cowork) using claude-fable-5; verified with eslint, tsc, and targeted vitest suites (148 tests passing).
3e62cf9 to
3c8c30a
Compare
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 `@test/core/init.test.ts`:
- Around line 746-750: Update the Claude skillContent assertions in the test to
also reject references containing "/opsx-" alongside the existing "/opsx:"
check, matching the OpenCode case while preserving the "/openspec-" expectation.
In `@test/core/update.test.ts`:
- Around line 1490-1496: Update the skillContent assertions in the update test
to reject both legacy reference formats, adding coverage for `/opsx-` alongside
the existing `/opsx:` check while preserving the `/openspec-` expectation.
🪄 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: 498aa762-dc4e-4381-a593-1bacac251d92
📒 Files selected for processing (8)
.changeset/skills-only-references.mdsrc/core/init.tssrc/core/update.tssrc/utils/command-references.tssrc/utils/index.tstest/core/init.test.tstest/core/update.test.tstest/utils/command-references.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- .changeset/skills-only-references.md
- src/utils/index.ts
- src/utils/command-references.ts
- src/core/init.ts
- test/utils/command-references.test.ts
- src/core/update.ts
|
I rebased this onto the latest upstream main and resolved the merge conflicts from the v1.6.0 changes — the transformer selection now also covers the newly added oh-my-pi tool, with matching test coverage. GitHub reports the PR as mergeable again. I also added docstrings to the touched entry points to clear the docstring coverage warning. @alfred-openspec the rebase marked your approval as stale — could you re-confirm when you get a chance? |
|
Nice fix, and the opencode/pi precedence is now handled correctly. One gap before this is complete: The new Why it matters: The current tests miss it because they only assert against the Suggested fix:
Everything else looks good to go. |
COMMAND_TO_SKILL_REFERENCE was missing the update workflow, so a /opsx:update reference in any skill template would survive skills-only transformation untouched. Align the map with the canonical 12-entry WORKFLOW_TO_SKILL_DIR and assert the generated openspec-update-change skill carries no raw command references.
|
Good catch — added the |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified at bb81a67: the skill-reference map now matches all 12 canonical workflows, including update, and skills-only delivery wins over hyphen command references for every tool. The 172 focused transformer, init, and update tests pass locally, and the full cross-platform CI suite is green.
3 participants
Fixes #881, closes #879.
When
delivery: 'skills'is configured, generated SKILL.md files contained hardcoded/opsx:*command references pointing to commands that were never generated. This implements the fix outlined in #881: addstransformToSkillReferences()with the explicit 11-entry command-to-skill mapping and wires it at everygenerateSkillContentcall site —init.ts, both sites inupdate.ts(includingupgradeLegacyTools), and both sites inworkspace/skills.ts.Deviations from the issue snapshot
pibranch in the transformer condition. The issue snippet only checksopencode, but main already routespithroughtransformToHyphenCommands; copying the snippet verbatim would regress pi.workspace/skills.tswired in addition to the files listed in the issue. It generates SKILL.md from the sameglobalConfig.deliveryand is itself a skills-only surface, so the bug persisted there. Regression test included (verified to fail against the unwired code).WORKFLOW_TO_SKILL_DIRexists in bothinit.ts(local copy) andprofile-sync-drift.ts(exported); the two are identical andCOMMAND_TO_SKILL_REFERENCEmatches both.Open question for maintainers
In skills-only mode, opencode/pi still take the
transformToHyphenCommandsbranch, so their SKILL.md files contain/opsx-*references to commands that were also never generated — same class of bug. I didn't expand scope because it's unclear whether/openspec-<skill>slash references resolve in those tools. Happy to file a follow-up issue. The legacy-upgrade path also lacks a skills-only content assertion; same offer.Testing
npx eslint src/cleantransformToSkillReferencesunit tests: per-mapping assertions for all 11 commands, multi-reference, backtick, unknown-command, and bulk-archive/archive boundary cases/opsx:and does contain/openspec-(explore template verified to contain/opsx:references, so the assertion is discriminative); new workspace test mutation-verifiedGenerated with Claude (Cowork) using claude-fable-5.
Summary by CodeRabbit
SKILL.mdfiles no longer include unsupported/opsx:markers and instead link to the expected/openspec-skill references.SKILL.mdoutputs, plus command reference transformation behavior.