fix: use skill references in SKILL.md for skills-only delivery#1194
fix: use skill references in SKILL.md for skills-only delivery#1194mc856 wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (7)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a delivery-aware transformer that rewrites ChangesSkills-Only Delivery Reference Transformation
Sequence Diagram(s)sequenceDiagram
participant InitCommand
participant UpdateCommand
participant getTransformerForTool
participant generateSkillContent
InitCommand->>getTransformerForTool: resolve transformer(tool.value, delivery)
getTransformerForTool-->>InitCommand: transformToSkillReferences / transformToHyphenCommands / undefined
InitCommand->>generateSkillContent: pass transformer for SKILL.md
UpdateCommand->>getTransformerForTool: resolve transformer(tool.value, delivery)
getTransformerForTool-->>UpdateCommand: transformToSkillReferences / transformToHyphenCommands / undefined
UpdateCommand->>generateSkillContent: pass transformer for SKILL.md
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
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).
mc856
force-pushed
the
fix/skills-only-references
branch
from
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.
2 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