fix(templates): stop propose from skipping the specs artifact#1412
fix(templates): stop propose from skipping the specs artifact#1412clay-good wants to merge 11 commits into
Conversation
The propose skill and command opened by telling the agent it would create
"proposal.md, design.md, tasks.md" — omitting specs entirely, even though
the default spec-driven schema defines four artifacts and tasks requires
specs.
Artifact status is derived from file existence (detectCompleted), so an
agent that wrote tasks.md while tasks was still blocked flipped it to
"done" and satisfied the workflow's stop condition ("all applyRequires
artifacts are done") with specs never created.
Two guidance fixes in propose.ts, applied to both the skill and command
bodies:
- the opening list now names specs/<capability>/spec.md and frames the
list as schema-derived rather than fixed
- step 4b forbids writing an artifact whose status is blocked, pointing
at its missingDeps first
Closes #1260
Closes #788
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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 |
…ocked Review of the first commit found the blocked-artifact rule would deadlock propose. In spec-driven, tasks requires [specs, design], so tasks is blocked whenever design is absent — but docs/concepts.md states "Dependencies are enablers, not gates. You can skip design if you don't need it", the schema says to create design.md "only if any apply", and a change with no design.md validates clean. Forbidding writes to blocked artifacts would have left propose unable to finish those changes. Replaced with a stop-time sweep that targets the actual failure without gating anything: before stopping, create any artifact still `ready` or tell the user which one is being skipped and why. specs is caught because it is still `ready` at that point; design stays legitimately skippable. Also from review: - apply the same guard to ff-change.ts, which runs the byte-identical loop and reproduced the same spec-less change via /opsx:ff - tell the agent to resolve a glob resolvedOutputPath to a concrete path (specs generates specs/**/*.md, so a literal write creates a `**` dir) - mark the spec bullet as a delta inside the change directory, so it cannot be read as the main spec under openspec/specs/ - harden the ordering assertion so a missing guard cannot satisfy it Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Third review round: - the sweep was single-pass, so creating a ready artifact could unblock others that then went unwritten; it now repeats until none remain - "every artifact your schema requires" collided with the applyRequires field the agent reads 35 lines later, which literally means ["tasks"]; say "the artifacts your schema defines" instead - the negative test pinned the exact rejected sentence, so any reworded prohibition would have passed; match the shape instead (verified against four phrasings of the deadlock rule) - drop "immediately" from a test title that only asserts ordering Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
…ional
CodeRabbit flagged that the sweep's escape hatch let the agent skip any
remaining ready artifact, including specs — which openspec validate
requires ("Change must have at least one delta").
"Required" is not machine-readable: the graph has tasks requiring both
specs and design, so keying off the dependency edges would force design
again and reintroduce the deadlock. The artifact's own instruction text
does carry the distinction — spec-driven's design opens with "When to
include design.md (create only if any apply)" while specs is
unconditional — so the skip permission is anchored there instead.
Test asserts both halves: the conditional wording is present and no
unconditional skip escape hatch remains.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
CodeRabbit caught the interaction between the previous two commits: a conditional artifact that is skipped never gets a file, so it stays `ready` forever and "repeat until none remain" would not terminate for any change that legitimately omits design.md. The sweep now repeats until only skipped artifacts remain, and skipping an artifact takes it out of later sweeps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Now that a conditional artifact can be legitimately skipped, the closing summary claiming "All artifacts created!" would be false. Report the required set instead, and list any conditional artifact that was skipped alongside the ones created. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three adversarial reviews converged on the same defects, two of which I reproduced against the CLI. Deadlock (blocker): skipping design left tasks blocked forever. Nothing was ready, the sweep terminated, and the stop condition could never be met — and the word "blocked" appeared nowhere in the skill, so no step authorized writing it. The loop now states outright that dependencies are enablers, not gates: a required artifact blocked only by a skipped conditional gets written anyway. Over-scoping: sweeping every `ready` artifact told propose to write artifacts outside the apply closure. Reproduced with a custom schema carrying a post-implementation `retro`. The required set is now applyRequires plus its transitive requires, and everything else is explicitly left alone. Stale guardrail: "Create ALL artifacts needed for implementation (as defined by schema's `apply.requires`)" encoded the exact premise this fix refutes, and none of the earlier commits touched it. Also: step 4b heading and its closing bullet contradicted the guard above them; the 498-char bullet is now six short ones; the preamble and glob sentences are tightened. Tests drop three vacuous or wording-pinned assertions for positive checks on each guarantee. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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: |
c90dea9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://38d25b18.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 |
…shape The single-star-glob status/apply consistency test pins the exact status JSON artifact shape with toEqual. Adding the `requires` edge to every artifact made the actual object a superset of the expected, failing CI on all platforms (local runs masked it as an env-only zsh-installer failure). Include `requires: []` in the expected shape so the test reflects the new contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Third adversarial-review round on the final state. Two doc/prose findings: 1. Step 4's title still read "Create artifacts in sequence until apply-ready" — the bug's own vocabulary. In the prewritten-tasks case the change is already apply-ready when step 4 begins (applyRequires=["tasks"], tasks done), so a literal agent anchoring on the title could create nothing and stop — the exact early-stop this PR kills. A prior round fixed the 4b sub-heading but left the step title. Retitled to "Create every artifact in the required set" (all four propose/ff bodies), which also reframes the step around the required set so 4b is the driver. Pinned by a new template test. 2. docs/cli.md's `status --json` example still omitted the `requires` field (agent-contract.md was updated last round, this example was missed). Filled in with real values from the CLI for that exact scenario. Skills regenerated; six golden hashes refreshed. Full suite 2042 pass; only the 17 env-only zsh-installer failures remain (CI-green). The correctness and tests/hygiene review passes were clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
…requires field Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
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