fix(templates): stop propose from skipping the specs artifact - #1412
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:
📝 WalkthroughWalkthroughThe propose and fast-forward templates now enumerate schema-derived artifacts, resolve output paths, and check for remaining ready artifacts before stopping. Tests validate artifact listings, guard ordering, output-path guidance, blocked-status wording, and updated template hashes. ChangesPropose and fast-forward workflow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ProposeOrFfWorkflow
participant OpenSpecStatus
participant ChangeArtifacts
ProposeOrFfWorkflow->>OpenSpecStatus: Run status --json
OpenSpecStatus-->>ProposeOrFfWorkflow: Return artifact states
ProposeOrFfWorkflow->>ChangeArtifacts: Create artifact at resolvedOutputPath
ChangeArtifacts-->>OpenSpecStatus: File exists
OpenSpecStatus-->>ProposeOrFfWorkflow: Mark artifact done and expose remaining ready artifacts
ProposeOrFfWorkflow->>ChangeArtifacts: Create remaining ready artifacts
🚥 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.
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 `@src/core/templates/workflows/propose.ts`:
- Around line 79-82: Require all required ready artifacts, including specs, to
be created before stopping; only explicitly optional artifacts may be skipped
with an explanation. Apply this guard in src/core/templates/workflows/propose.ts
at lines 79-82 and 195-198, src/core/templates/workflows/ff-change.ts at lines
69-72 and 175-178, and update the generated skills in
skills/openspec-propose/SKILL.md at line 80 and
skills/openspec-ff-change/SKILL.md at lines 69-70. Update
test/core/templates/propose.test.ts at lines 86-90 to verify required artifacts
cannot be skipped while optional artifacts remain skippable.
🪄 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: bfd44dc8-069b-4eee-8a72-1f54cb9b4a7c
📒 Files selected for processing (6)
skills/openspec-ff-change/SKILL.mdskills/openspec-propose/SKILL.mdsrc/core/templates/workflows/ff-change.tssrc/core/templates/workflows/propose.tstest/core/templates/propose.test.tstest/core/templates/skill-templates-parity.test.ts
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 `@skills/openspec-ff-change/SKILL.md`:
- Line 70: Update the artifact-completion guidance in
skills/openspec-ff-change/SKILL.md at line 70 and
skills/openspec-propose/SKILL.md at line 80 so only explicitly optional
artifacts may be skipped; require every schema-required artifact to be created
before completion, including specs/<capability>/spec.md, while preserving the
existing repeat-until-none-ready behavior.
🪄 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: 4972c885-3ae3-4d69-9c66-08de858feffb
📒 Files selected for processing (6)
skills/openspec-ff-change/SKILL.mdskills/openspec-propose/SKILL.mdsrc/core/templates/workflows/ff-change.tssrc/core/templates/workflows/propose.tstest/core/templates/propose.test.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/core/templates/skill-templates-parity.test.ts
- src/core/templates/workflows/propose.ts
- src/core/templates/workflows/ff-change.ts
- test/core/templates/propose.test.ts
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 `@skills/openspec-ff-change/SKILL.md`:
- Line 70: Update the artifact sweep instructions to track IDs of explicitly
skipped conditional artifacts, such as spec-driven design.md, and exclude those
IDs when checking for remaining ready artifacts. Continue sweeping until no
unskipped artifacts remain ready, while still requiring every applyRequires
artifact to reach done status.
🪄 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: 951bbf16-c1f5-42b1-ba1d-aee48b7493da
📒 Files selected for processing (6)
skills/openspec-ff-change/SKILL.mdskills/openspec-propose/SKILL.mdsrc/core/templates/workflows/ff-change.tssrc/core/templates/workflows/propose.tstest/core/templates/propose.test.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/templates/workflows/ff-change.ts
- src/core/templates/workflows/propose.ts
- test/core/templates/skill-templates-parity.test.ts
- test/core/templates/propose.test.ts
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 `@skills/openspec-ff-change/SKILL.md`:
- Around line 70-71: Update the final completion summaries in
skills/openspec-ff-change/SKILL.md lines 84-88 and
skills/openspec-propose/SKILL.md lines 94-98 to state that all required
artifacts are complete, and explicitly list any conditional artifacts skipped
during the workflow; ensure the summaries no longer claim that every artifact
was created.
🪄 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: 1f305fc9-de59-4545-b65b-9e2703028f57
📒 Files selected for processing (6)
skills/openspec-ff-change/SKILL.mdskills/openspec-propose/SKILL.mdsrc/core/templates/workflows/ff-change.tssrc/core/templates/workflows/propose.tstest/core/templates/propose.test.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/templates/workflows/propose.ts
- src/core/templates/workflows/ff-change.ts
- test/core/templates/skill-templates-parity.test.ts
- test/core/templates/propose.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
One blocker before approval: status --json does not expose each artifact's requires edges, so the new instructions cannot reliably compute the transitive required set when an apply artifact is already done. I reproduced this at bb1d33a with only tasks.md present: the JSON reports applyRequires: ["tasks"] and tasks: done, but no edge from tasks to specs/design; please derive the closure through instructions (or expose the edges) and add this prewritten-tasks regression.
…d set alfred's PR #1412 blocker: the propose/ff loop told the agent to build the "required set" from `applyRequires` plus everything it transitively `requires`, but `status --json` never carried those edges. A prewritten `tasks.md` reads `done` with no visible link to specs/design, so the agent could still stop without a spec. The only thing making it work was a hardcoded schema list in the instruction text, which is wrong for any custom schema. Fix (alfred's "expose the edges" path): every artifact in `status --json` now carries its `requires` edge list for all statuses, including `done`. The loop derives the required set by walking those edges from `applyRequires` - computable from a single status call, for any schema. Proven end-to-end: with only tasks.md present, `tasks` reports `requires:["specs","design"]` and the closure walk correctly flags specs/design/proposal as still-missing. Also hardens the agent instructions from three adversarial reviews: - 4b heading named the buggy stop condition ("every artifact the apply phase depends on"); now names the required set explicitly - skip decision now tells the agent to fetch `openspec instructions` and read the `instruction` field before skipping (specs is never optional) - names the walk's seed (`from those`) and the source command (`status --json`, since instructions calls the edges `dependencies`) - completion headline no longer claims "all created" when a conditional was skipped Contract doc (agent-contract.md), the cli-artifact-workflow spec, and tests (source-level prewritten-tasks regression + template guards) updated; skills regenerated and six golden hashes refreshed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deploying openspec-docs with
|
| Latest commit: |
a792673
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fd76d5fc.openspec-docs.pages.dev |
| Branch Preview URL: | https://fix-propose-includes-specs.openspec-docs.pages.dev |
|
@alfred-openspec — fixed at 5a95821, taking the "expose the edges" path you offered.
The edge from Regression added exactly where you asked — source level in While here, two more adversarial review passes hardened the instruction prose: the 4b heading no longer restates the |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The original blocker is fixed: the exact-head build, 123 focused tests, clean skill regeneration, and prewritten-tasks.md reproduction all pass. One release-tracking fix remains before approval: this now adds artifacts[].requires to the public status --json contract and changes shipped workflow templates, so please add a patch changeset and refresh the PR description's now-stale template-only/no-JSON-contract claims.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The original dependency-closure blocker and release-tracking follow-up are fixed at c259a6f: the prewritten-tasks.md case now exposes every requires edge, and the retitled create step plus CLI/contract docs consistently drive the full required set. Exact-head build, 123 focused tests, clean skill regeneration, the manual reproduction, release tracking, and the full CI/CodeQL matrix all pass.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Approved at c259a6f. The new requires edges close the prewritten-tasks.md hole, the workflow guidance and generated skills are aligned, and the public contract, regression coverage, patch changeset, and exact-head CI all match the fix.
Squashed for rebase; see PR #1412 for the full commit history.
c259a6f to
a792673
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the exact squashed head. The dependency closure now preserves transitive requires edges even when tasks.md already exists, with the original regression covered; build, generated-file parity, all 2,105 local tests, and required CI pass.
2 participants
Closes #1260
Closes #788
Status: Ready to merge. Addresses @alfred-openspec's blocker and release-tracking follow-up (see the Review round section). One small additive change to the
status --jsonoutput — each artifact now carries itsrequiresedges — plus the propose/ff instruction text. No CLI behavior or schema change; the JSON change is backward-compatible (new field, existing consumers unaffected). Ships with a patch changeset.Review round: @alfred-openspec's blocker (resolved)
The blocker: the loop told the agent to compute the required set as
applyRequiresplus everything it transitivelyrequires, butstatus --jsonnever carried those edges. Atbb1d33a, a prewrittentasks.mdreadsdonewith no visible link tospecs/design, so the agent could still stop with no spec. The hardcoded(spec-driven: proposal, specs, design, tasks)parenthetical was the only thing masking it — and it is wrong for any custom schema.The fix (the "expose the edges" path @alfred offered): every artifact in
status --jsonnow carries itsrequiresedge list for all statuses, includingdone. The loop derives the required set by walking those edges fromapplyRequires— computable from a singlestatuscall, for any schema, with no hardcoded list.Proof — @alfred's exact repro, only
tasks.mdpresent:Walking the closure from
applyRequires=["tasks"]using only these edges yields the required set{proposal, specs, design, tasks}and flagsspecs(with design, proposal) as still-missing — the agent creates them. Regression locked at the source level (instruction-loader.test.ts: adonetasksmust still exposerequires:["specs","design"]) and the template level.Two more adversarial reviews hardened the instruction prose: the 4b heading no longer restates the
apply.requires-only stop condition, the skip decision now tells the agent to read the artifact'sinstructionfield before skipping (specsis never optional), and the walk's seed/source (status --json) are named explicitly.What was wrong
/opsx:proposecould finish and declare a change "ready to implement" with no spec written at all — in a spec-driven tool. Two independent reports: #1260 and #788./opsx:ffreproduced it too.Two things combined:
proposal.md,design.md,tasks.md. The defaultspec-drivenschema defines four artifacts, andtasksrequiresspecs. The first thing the agent read told it specs was not part of the job.applyRequires, which is["tasks"]. An artifact becomesdonethe moment a matching file exists (detectCompleted), so writingtasks.mdearly flippedtasksfromblockedtodoneand satisfied the stop condition. The Guardrails section said the same thing outright: "Create ALL artifacts needed for implementation (as defined by schema'sapply.requires)".How it was fixed
The preamble now names
specs/<capability>/spec.mdand is framed as schema-derived rather than a fixed three. Step 4b and the guardrail are rewritten around the required set —applyRequiresplus everything it transitivelyrequires:Applied identically to
propose.tsandff-change.ts(skill and command bodies each), since they run byte-identical loops.Design notes
blocked. An earlier draft said "never write a blocked artifact" — that deadlocks propose, becausetasksrequires bothspecsanddesign, anddocs/concepts.md:455says "Dependencies are enablers, not gates… You can skip design if you don't need it." A change with nodesign.mdvalidates clean. The loop instead states the permission explicitly.instructionfield.specsmust not be skippable (openspec validaterejects a change with no deltas), but "required" is not machine-readable — the graph hastasksrequiring both. The artifact's own instruction is the only signal that distinguishes them: spec-driven'sdesignopens with "create only if any apply";specssays nothing of the kind. A schema-leveloptional: truefield would be the durable fix — worth a follow-up.ready." A custom schema can define artifacts outside the apply path (a post-implementation retro, say); propose has no business creating those.Replication / proof
The failure chain, against
dist/. Writeproposal.mdanddesign.md, skipspecs, writetasks.md:Stop condition satisfied, yet:
Four scenarios traced against the final text: the #1260 state (specs now created), a docs-only change with design skipped (terminates, no deadlock — verified
tasks blocked ['design']with nothingready), a custom schema with an out-of-closure artifact (left alone), and a user hand-writingtasks.mdfirst (the set check catches the rest).test/core/templates/propose.test.ts— 9 cases across all four bodies, one per guarantee. The preamble case readsschemas/spec-driven/schema.yamlvialoadSchema()and asserts every artifact id it defines appears, so this drift cannot recur silently when the schema changes.Full suite: 2036 passed, zero unrelated failures. The only failures are the 17 known environment-only
zsh-installercases (Oh My Zsh), documented in CLAUDE.md.Notes
requiresfield on each artifact instatus --json(documented indocs/agent-contract.mdand pinned by a newcli-artifact-workflowspec scenario). The human-readablestatusrenderer is untouched. Plus the regeneratedskills/openspec-propose/SKILL.mdandskills/openspec-ff-change/SKILL.mdand six golden hashes; all 12 skills verified to regenerate byte-identically..changeset/propose-includes-specs.md) covers both the propose/ff fix and the additivestatus --jsonrequiresfield.ff-change.tsis included deliberately — it runs the byte-identical loop and reproduced the same spec-less change via/opsx:ff.status: "skipped"forskip_specschanges. Complementary: this one defines what completeness means, feat(validate): accept zero-delta changes that declare skip_specs #1399 adds a declarative way to mark specs intentionally absent. Whichever lands second takes a trivial resolution.openspec instructions applyexit 1 when blocked — downstream detection insrc/commands/; this is upstream prevention insrc/core/templates/.docs/commands.md:245,259now understate the loop's behavior;codebuddy/crush/lingma/qoderadapters emitname: OPSX: <X>unescaped, which breaks YAML frontmatter for every command. Happy to file both.🤖 Generated with Claude Code