fix(templates): correct generated workflow guidance - #1500
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:
📝 WalkthroughWalkthroughGenerated OpenSpec workflows retain the selected store across applicable commands, validate synced specs before reporting success, and check optional workflow availability before suggesting it. Tests and a patch changeset document the updated guidance. ChangesGenerated workflow guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SyncWorkflow
participant OpenSpecCLI
participant MainSpecs
SyncWorkflow->>OpenSpecCLI: Sync specs with selected-root flags
OpenSpecCLI->>MainSpecs: Update main specs
SyncWorkflow->>OpenSpecCLI: Validate specs with openspec validate --specs
OpenSpecCLI-->>SyncWorkflow: Return validation result
SyncWorkflow-->>MainSpecs: Show completion summary after successful validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/core/templates/skill-templates-parity.test.ts (1)
207-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expanded store-capable command list.
This test checks only the generic sticky wording. It would still pass if
contextorviewwere removed fromSTORE_SELECTION_GUIDANCE. Assert the newly added command entries directly.Suggested assertion
expect(STORE_SELECTION_GUIDANCE).toContain( 'append it to every applicable command below, even when the example does not repeat the flag' ); + expect(STORE_SELECTION_GUIDANCE).toContain('`context`, `view`');🤖 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 207 - 214, Expand the test around STORE_SELECTION_GUIDANCE to assert that the store-capable context and view command entries are present directly, in addition to the existing sticky-selection wording checks. Use the exact command identifiers or guidance text from STORE_SELECTION_GUIDANCE so the test fails if either entry is removed.
🤖 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 `@skills/openspec-update-change/SKILL.md`:
- Line 18: Update both CLI fallback commands in the openspec-continue-change
guidance to quote the <name> and <artifact-id> placeholders, matching the
surrounding command syntax and preventing shell redirection when copied.
In `@src/core/templates/workflows/update-change.ts`:
- Line 93: Move the `/opsx:new` availability fallback from the end of the
guardrails into the intent-change steps at the locations represented by the
relevant template sections, placing the precondition immediately before
recommending `/opsx:new`; retain `openspec new change <name>` as the alternative
when unavailable. Update `test/core/templates/update-change.test.ts` to verify
both ordering and fallback text.
---
Nitpick comments:
In `@test/core/templates/skill-templates-parity.test.ts`:
- Around line 207-214: Expand the test around STORE_SELECTION_GUIDANCE to assert
that the store-capable context and view command entries are present directly, in
addition to the existing sticky-selection wording checks. Use the exact command
identifiers or guidance text from STORE_SELECTION_GUIDANCE so the test fails if
either entry is removed.
🪄 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 Plus
Run ID: 565f3baa-ce2b-45a1-93a7-bd53d8c93a43
📒 Files selected for processing (18)
.changeset/fix-generated-workflow-guidance.mdskills/openspec-apply-change/SKILL.mdskills/openspec-archive-change/SKILL.mdskills/openspec-bulk-archive-change/SKILL.mdskills/openspec-continue-change/SKILL.mdskills/openspec-explore/SKILL.mdskills/openspec-ff-change/SKILL.mdskills/openspec-new-change/SKILL.mdskills/openspec-onboard/SKILL.mdskills/openspec-propose/SKILL.mdskills/openspec-sync-specs/SKILL.mdskills/openspec-update-change/SKILL.mdskills/openspec-verify-change/SKILL.mdsrc/core/templates/workflows/store-selection.tssrc/core/templates/workflows/sync-specs.tssrc/core/templates/workflows/update-change.tstest/core/templates/skill-templates-parity.test.tstest/core/templates/update-change.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at exact head 29431b9. Focused template, parity, and initialization coverage passes; the generated guidance now preserves selected stores, validates syncs before success, and keeps propose planning-only until a separate apply request.
Deploying openspec-docs with
|
| Latest commit: |
19c648e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6f184451.openspec-docs.pages.dev |
| Branch Preview URL: | https://codex-fix-generated-workflow.openspec-docs.pages.dev |
19c648e to
39d60c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 229-249: Update the first updateVariants expectation for the
generated skills/openspec-update-change/SKILL.md content to use
`/openspec-continue-change`; retain `/opsx:continue` only in the generated
commands/opsx/update.md expectation.
🪄 Autofix
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 Plus
Run ID: a3e8e887-56df-4264-899f-b96efd2a3ab6
📒 Files selected for processing (19)
.changeset/fix-generated-workflow-guidance.mdskills/openspec-apply-change/SKILL.mdskills/openspec-archive-change/SKILL.mdskills/openspec-bulk-archive-change/SKILL.mdskills/openspec-continue-change/SKILL.mdskills/openspec-explore/SKILL.mdskills/openspec-ff-change/SKILL.mdskills/openspec-new-change/SKILL.mdskills/openspec-onboard/SKILL.mdskills/openspec-propose/SKILL.mdskills/openspec-sync-specs/SKILL.mdskills/openspec-update-change/SKILL.mdskills/openspec-verify-change/SKILL.mdsrc/core/templates/workflows/store-selection.tssrc/core/templates/workflows/sync-specs.tssrc/core/templates/workflows/update-change.tstest/core/init.test.tstest/core/templates/skill-templates-parity.test.tstest/core/templates/update-change.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- .changeset/fix-generated-workflow-guidance.md
- src/core/templates/workflows/store-selection.ts
- test/core/templates/skill-templates-parity.test.ts
- src/core/templates/workflows/sync-specs.ts
- src/core/templates/workflows/update-change.ts
- test/core/templates/update-change.test.ts
| const updateVariants: Array<[string, string]> = [ | ||
| [ | ||
| await fs.readFile( | ||
| path.join( | ||
| testDir, | ||
| '.claude', | ||
| 'skills', | ||
| 'openspec-update-change', | ||
| 'SKILL.md' | ||
| ), | ||
| 'utf-8' | ||
| ), | ||
| '`/opsx:continue`', | ||
| ], | ||
| [ | ||
| await fs.readFile( | ||
| path.join(testDir, '.claude', 'commands', 'opsx', 'update.md'), | ||
| 'utf-8' | ||
| ), | ||
| '`/opsx:continue`', | ||
| ], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the skill reference for the generated skill variant.
The generated SKILL.md must use `/openspec-continue-change`, not `/opsx:continue`. The current assertion will fail against the generated skill output or require a forbidden direct /opsx:* reference in that output. Keep `/opsx:continue` only for the generated command variant.
Proposed fix
[
await fs.readFile(
path.join(
testDir,
'.claude',
'skills',
'openspec-update-change',
'SKILL.md'
),
'utf-8'
),
- '`/opsx:continue`',
+ '`/openspec-continue-change`',
],Based on learnings: committed skills/<skill-name>/SKILL.md files must use skill invocations instead of direct /opsx:* references.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateVariants: Array<[string, string]> = [ | |
| [ | |
| await fs.readFile( | |
| path.join( | |
| testDir, | |
| '.claude', | |
| 'skills', | |
| 'openspec-update-change', | |
| 'SKILL.md' | |
| ), | |
| 'utf-8' | |
| ), | |
| '`/opsx:continue`', | |
| ], | |
| [ | |
| await fs.readFile( | |
| path.join(testDir, '.claude', 'commands', 'opsx', 'update.md'), | |
| 'utf-8' | |
| ), | |
| '`/opsx:continue`', | |
| ], | |
| const updateVariants: Array<[string, string]> = [ | |
| [ | |
| await fs.readFile( | |
| path.join( | |
| testDir, | |
| '.claude', | |
| 'skills', | |
| 'openspec-update-change', | |
| 'SKILL.md' | |
| ), | |
| 'utf-8' | |
| ), | |
| '`/openspec-continue-change`', | |
| ], | |
| [ | |
| await fs.readFile( | |
| path.join(testDir, '.claude', 'commands', 'opsx', 'update.md'), | |
| 'utf-8' | |
| ), | |
| '`/opsx:continue`', | |
| ], |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 230-239: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(
path.join(
testDir,
'.claude',
'skills',
'openspec-update-change',
'SKILL.md'
),
'utf-8'
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 243-246: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(
path.join(testDir, '.claude', 'commands', 'opsx', 'update.md'),
'utf-8'
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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` around lines 229 - 249, Update the first
updateVariants expectation for the generated
skills/openspec-update-change/SKILL.md content to use
`/openspec-continue-change`; retain `/opsx:continue` only in the generated
commands/opsx/update.md expectation.
Source: Learnings
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the rebased head: the workflow behavior is unchanged, parity hashes correctly track current main, 102 focused init/template tests pass locally, and the full hosted matrix is green. The remaining bot comment does not apply because Claude default delivery generates the namespaced command surface, while cross-tool and skills-only outputs correctly use skill references.
2 participants
Status
LGTM.
What was wrong
Generated workflows could lose a selected store because concrete command examples omitted the sticky
--store <id>rule. The update workflow suggested expanded-only workflows before explaining that they might not be installed, and its/opsx:newCLI fallback reused the active change name. The sync workflow reported success without validating the main specs it had just changed.How it was fixed
openspec validate --specswith the selected root after applying deltas and before sync reports success.openspec init --tools claude, then regenerate the committed skills and parity hashes.Replication / proof
The regression assertions failed on current
mainfor the reported behaviors, then passed with this patch.openspec init --tools claudegenerated:--store "<id>"command.git diff --checkpassed.Notes / nits
This changes generated instructions only. It does not change CLI behavior, APIs, schemas, persisted data, or architecture.
Closes #1493
Summary by CodeRabbit
Improvements
Tests