fix(templates): deduplicate apply skill and command instructions#1153
fix(templates): deduplicate apply skill and command instructions#1153Shashank200345 wants to merge 7 commits into
Conversation
Extract shared APPLY_INSTRUCTIONS constant so skill and command templates reference the same string. Eliminates content drift reported in Fission-AI#1139.
|
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracts apply-change instruction text into getApplyInstructions(mode) and wires skill and opsx command templates to use it; updates mode-specific continue/archive references and adjusts parity-test hashes and adds a normalization parity assertion. ChangesApply Change Workflow Instructions
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 |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for chasing this drift, the dedupe direction is useful. I’m requesting changes because the shared body now makes the generated skill recommend /opsx:continue and /opsx:archive, which regresses skills-only/non-command installs, and the template parity test currently fails because the intentional prompt hash changes were not updated.
If you keep the shared constant, please parameterize or split the invocation-specific lines so the skill path can keep skill-friendly references while the command path keeps /opsx:*; also avoid marking #1139 fixed unless we intentionally accept source-level dedupe as the first slice, since generated projects still get two full editable copies.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/templates/skill-templates-parity.test.ts (1)
32-70: ⚡ Quick winAdd a direct apply skill/command parity assertion.
These hash updates re-baseline the outputs, but they still won’t fail if the skill and command drift together and both constants get updated in the same PR. Since this change is specifically about deduplicating the apply instructions, add one focused assertion that normalizes the two mode-specific phrases and verifies
getApplyChangeSkillTemplate().instructionsmatchesgetOpsxApplyCommandTemplate().contentafter normalization. That pins the actual contract from issue#1139instead of only pinning snapshots.♻️ Suggested test shape
describe('skill templates split parity', () => { + it('keeps apply skill and command instructions in sync apart from mode-specific wording', () => { + const normalizeApplyInstructions = (value: string) => + value + .replaceAll('`/opsx:continue`', '<continue-ref>') + .replaceAll('the openspec-continue-change skill', '<continue-ref>') + .replaceAll('You can archive this change with `/opsx:archive`.', '<archive-ref>') + .replaceAll('Ready to archive this change.', '<archive-ref>'); + + expect( + normalizeApplyInstructions(getApplyChangeSkillTemplate().instructions) + ).toEqual( + normalizeApplyInstructions(getOpsxApplyCommandTemplate().content) + ); + }); + it('preserves all template function payloads exactly', () => {🤖 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/templates/skill-templates-parity.test.ts` around lines 32 - 70, Add a direct parity assertion that compares getApplyChangeSkillTemplate().instructions to getOpsxApplyCommandTemplate().content after normalizing mode-specific wording; implement a small normalizer (e.g., trim, collapse whitespace, toLowerCase, and replace or remove the differing mode-specific phrases/labels used in apply vs opsx apply) and assert normalized(getApplyChangeSkillTemplate().instructions) === normalized(getOpsxApplyCommandTemplate().content) so the test fails if the actual contract between those two templates drifts.
🤖 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 `@test/core/templates/skill-templates-parity.test.ts`:
- Around line 32-70: Add a direct parity assertion that compares
getApplyChangeSkillTemplate().instructions to
getOpsxApplyCommandTemplate().content after normalizing mode-specific wording;
implement a small normalizer (e.g., trim, collapse whitespace, toLowerCase, and
replace or remove the differing mode-specific phrases/labels used in apply vs
opsx apply) and assert normalized(getApplyChangeSkillTemplate().instructions)
=== normalized(getOpsxApplyCommandTemplate().content) so the test fails if the
actual contract between those two templates drifts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 538f213c-5750-4220-898b-d6df2164f991
📒 Files selected for processing (2)
src/core/templates/workflows/apply-change.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/templates/workflows/apply-change.ts
Updated! The shared constant is now a parameterized function — |
|
Updated! Added a normalized body parity assertion that strips |
…github.com/Shashank200345/OpenSpec into fix/deduplicate-skill-command-instructions
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/templates/workflows/apply-change.ts (1)
29-29: 💤 Low valueConsider parameterizing invocation examples in a follow-up.
Lines 29 and 40 contain hardcoded
/opsx:applyexamples (e.g.,`/opsx:apply add-auth`,`/opsx:apply <other>`). For command mode these are correct, but for skill mode the invocation mechanism may differ. The current PR successfully parameterizes the continue/archive references (lines 64, 136), which was the stated objective. If skills are invoked differently than slash commands, consider parameterizing these examples in a follow-up to maintain consistency.Also applies to: 40-40
🤖 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/templates/workflows/apply-change.ts` at line 29, The template in apply-change.ts hardcodes the `/opsx:apply` invocation in inline examples (e.g., the strings containing "/opsx:apply add-auth" and "/opsx:apply <other>"); change these to use a parameterized placeholder (e.g., an invocation variable like {INVOKE_SYNTAX} or {INVOCATION} inserted into the template rendering) so the same template works for slash-command mode and skill-mode; update all occurrences of the literal "/opsx:apply" in apply-change.ts to reference that placeholder (similar to the already-parameterized continue/archive references) and ensure the rendering code supplies the correct invocation string depending on command vs skill mode.
🤖 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/templates/workflows/apply-change.ts`:
- Line 29: The template in apply-change.ts hardcodes the `/opsx:apply`
invocation in inline examples (e.g., the strings containing "/opsx:apply
add-auth" and "/opsx:apply <other>"); change these to use a parameterized
placeholder (e.g., an invocation variable like {INVOKE_SYNTAX} or {INVOCATION}
inserted into the template rendering) so the same template works for
slash-command mode and skill-mode; update all occurrences of the literal
"/opsx:apply" in apply-change.ts to reference that placeholder (similar to the
already-parameterized continue/archive references) and ensure the rendering code
supplies the correct invocation string depending on command vs skill mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 250131e7-98d9-48eb-b72f-62f845e45dbb
📒 Files selected for processing (1)
src/core/templates/workflows/apply-change.ts
2 participants
What's this about?
I was looking into the duplication issue between skill and command
files and noticed that
apply-change.tshad the instruction bodywritten out twice — once in
getApplyChangeSkillTemplate()andagain in
getOpsxApplyCommandTemplate(). They had already driftedapart in 3 places.
What I did
Pulled the shared instructions into a single
APPLY_INSTRUCTIONSconstant at the top of the file. Both functions now point to the
same string, so there's only one place to edit going forward.
How I verified it
Ran
openspec init --tools claude --forceand diffed the twogenerated files. Before the fix there were 3 content differences.
After — only frontmatter (name, description, metadata) differs,
which is expected and correct.
Notes
This fixes
apply-change.tsspecifically. The same pattern can beapplied to the other workflow files (
propose.ts,explore.ts, etc.)as a follow-up if the maintainers are happy with this approach.
Summary by CodeRabbit