feat(validate): accept zero-delta changes that declare skip_specs#1399
feat(validate): accept zero-delta changes that declare skip_specs#1399clay-good wants to merge 11 commits into
Conversation
The proposal instruction and template told authors to leave Modified Capabilities 'empty if no requirement changes', but openspec validate unconditionally rejects a change with zero deltas (CHANGE_NO_DELTAS). An author following the guidance literally passed the propose phase and only discovered the contradiction at validate time, after every artifact was already written. State the >=1-delta constraint where authors read it: the proposal instruction, the specs instruction, and the proposal template comment. Guidance-only; no validation behavior changes. Fixes #1384 Co-Authored-By: Claude Fable 5 <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 spec-driven workflow now supports explicit ChangesExplicit zero-delta workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 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 `@schemas/spec-driven/templates/proposal.md`:
- Around line 18-20: Update the guidance comment in the proposal template to
include the pure-refactor requirement for a testable non-functional property,
matching the corresponding instructions in schema.yaml. Preserve the existing
capability/delta guidance and clarify that pure refactors must still define at
least one verifiable non-functional outcome.
🪄 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: c4c2c50c-57ad-47d7-9a5a-362952053446
📒 Files selected for processing (2)
schemas/spec-driven/schema.yamlschemas/spec-driven/templates/proposal.md
…emplate Address review: the schemas files use plain hyphens, not em-dashes; the proposal template comment now carries the same pure-refactor advice as the schema instruction (CodeRabbit suggestion); the specs-instruction note moves below the per-capability bullets so it no longer interrupts the list intro. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
This trades the late validation failure for a worse contract: pure refactors are now told to invent a changed requirement, while docs/examples.md still says they have no delta and docs/concepts.md says implementation-only changes do not belong in specs. Please model true no-behavior work as an explicit skipped/not-applicable specs state, or make the hard-minimum policy a coordinated product and documentation change rather than updating only the schema prompt.
A pure refactor has no spec deltas by design (docs/examples.md Recipe 5), but openspec validate hard-rejected every zero-delta change, so the propose guidance either contradicted the validator or pushed authors to invent requirements. Model the no-behavior-change case explicitly instead: skip_specs: true in the change's .openspec.yaml makes validate accept zero deltas (and error when the marker coexists with delta spec files), marks specs-producing artifacts complete so the workflow isn't blocked on files that must not exist, and the propose/specs guidance now points at the marker rather than telling refactors to fabricate a delta. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deploying openspec-docs with
|
| Latest commit: |
97196c8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5df6b1c4.openspec-docs.pages.dev |
| Branch Preview URL: | https://fix-propose-guidance-zero-de.openspec-docs.pages.dev |
clay-good
changed the title
|
Reworked per the review: the previous revision kept the hard one-delta minimum and told pure refactors to name a changed guarantee, which contradicted docs/examples.md Recipe 5 and concepts.md. This revision takes the suggested first option — an explicit not-applicable specs state. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The new direction fixes the fake-requirement contradiction, but changeDeclaresSkipSpecs() trusts raw skip_specs: true without validating the metadata shape: I reproduced a .openspec.yaml containing only that field, with the required schema missing, passing as valid while status/instructions reject the same metadata. Please validate the marker through the shared metadata schema (and add the adversarial case), and surface specs as explicitly skipped/not-applicable in status rather than collapsing them to [x] complete.
Review follow-ups: the marker was read from raw YAML, so metadata that status/instructions reject (e.g. skip_specs without the required schema field) silently passed validate; it now only counts when the file parses under ChangeMetadataSchema, and a set-but-invalid marker surfaces as an explicit error. Status also stops presenting the synthesized specs completion as done: skipped artifacts render as '[~] specs (skipped: change declares skip_specs)' and drop out of the progress denominator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both round-2 points addressed in 215494a:
|
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/validation.skip-specs.test.ts`:
- Around line 109-119: Update Validator.validateChange so the CHANGE_NO_DELTAS
issue is filtered only when readSkipSpecsMarker(changeDir) reports valid
metadata, not merely when the marker is declared; preserve the existing
filtering behavior for valid skip_specs metadata and retain the no-deltas error
for invalid metadata.
🪄 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: 1c00c9d8-d600-4b32-bd61-254cb5087582
📒 Files selected for processing (10)
.changeset/skip-specs-explicit-zero-delta.mddocs/examples.mdsrc/commands/workflow/shared.tssrc/commands/workflow/status.tssrc/core/artifact-graph/instruction-loader.tssrc/core/change-status-policy.tssrc/core/validation/constants.tssrc/core/validation/validator.tstest/core/artifact-graph/instruction-loader.test.tstest/core/validation.skip-specs.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/skip-specs-explicit-zero-delta.md
- test/core/artifact-graph/instruction-loader.test.ts
- src/core/validation/constants.ts
- src/core/validation/validator.ts
…_specs change Requesting instructions for an artifact the change skips would otherwise hand an agent 'create spec files' guidance whose output validate then rejects as a conflict. The instructions payload now carries skipped: true (JSON included) and the text output opens with a warning naming the marker and the way out (remove skip_specs if requirements now change). Co-Authored-By: Claude Fable 5 <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/commands/workflow/instructions.ts`:
- Around line 192-199: Update printInstructionsText so when instructions.skipped
is true, it stops before emitting the creation directive and output path, or
otherwise makes the remaining instructions explicitly non-actionable. Preserve
the existing warning text and normal creation instructions for non-skipped
artifacts.
🪄 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: f66235ae-0361-4f11-82bf-21c3fcc9161e
📒 Files selected for processing (3)
src/commands/workflow/instructions.tssrc/core/artifact-graph/instruction-loader.tstest/core/artifact-graph/instruction-loader.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/core/artifact-graph/instruction-loader.test.ts
- src/core/artifact-graph/instruction-loader.ts
… too validateChangeDeltaSpecs already explains when a set marker is not honored because the metadata fails the shared schema; validateChange (archive's non-blocking proposal pass) kept only the bare no-deltas error. Both paths now surface the same invalid-metadata message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tifact The warning was followed by the normal creation directive, output path, and template - a contradictory prompt that could still push an agent to write spec files validate rejects as conflicting. The text output for a skipped artifact is now the warning alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y, and agent surfaces Deep-check follow-ups before re-review: - The marker read moves to utils (readSkipSpecsMarker) and is shared by validate and archive; a metadata file that mentions skip_specs but is not valid YAML now gets the explicit invalid-metadata error instead of silently falling back to the zero-delta error whose guidance tells the user to set the very marker they set. - The marker conflict now fires on ANY file under specs/ (root spec.md, headerless files, stray notes) - previously only parsed deltas counted, so validate and the artifact graph's specs/** glob could disagree, and archive skipped the check entirely for headerless files. Archive now runs delta validation for a marked change with any spec content, so the two commands agree. - instructions apply treats skipped artifacts as present when computing missingArtifacts: a custom schema whose apply phase requires the specs-producing artifact (or has no apply block) no longer reports a marked change permanently blocked while status calls it complete. - The instructions JSON payload carries the do-not-create warning (skipped + warning fields), not just the text output, since workflow skills drive the CLI in --json mode. - continue/update-change skill templates and docs/agent-contract.md now document the 'skipped' artifact status and the new instructions fields; skills/ regenerated and golden hashes updated; cli.md documents the marker under validate and the [~] status rendering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion, agent-journey gaps Correctness round: a non-boolean skip_specs (skip_specs: "yes") was silently ignored while the zero-delta guidance told the user to set the marker they had set - the unhonorable-marker error now fires on key presence, not value. The any-file conflict probe ran unconditionally outside the historical error handling, crashing validate with raw ENOTDIR/EACCES where the old path degraded to the no-deltas error - it is now gated on a declared marker and fails closed into the conflict, in both validate and archive. Dot-files (.DS_Store, .gitkeep) no longer trigger the conflict - hasAnyFileUnder skips dot entries like discoverSpecFiles. The invalid-YAML heuristic is anchored so comments mentioning skip_specs don't claim the marker was set. Coherence round: instructions for dependents no longer present a skipped specs artifact as a done dependency with files to read - it renders as status="skipped" with no path, and DependencyInfo carries skipped for JSON consumers. The propose/ff skill completion loops accept skipped alongside done, so schemas without an apply block can't deadlock an agent waiting for specs to become done. Archive also forces validation when the marker is mentioned but unhonorable, the validate next-steps footer stops telling marker changes to add deltas, and docs cover the un-skip path, the instructions JSON fields, the flag/marker distinction, and the .openspec.yaml fields. Skills regenerated; golden hashes updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… unguarded A mutation pass showed two surviving mutations: disabling archive's marker enforcement (the only place the marker gates a destructive operation) and disabling the marker-aware next-steps footer both left the whole suite green. New tests kill both: archive of a marked change blocks on stray spec files and on unhonorable metadata (and still archives cleanly with none), and the validate footer shows the marker bullets on a conflict while a plain zero-delta change keeps the delta-authoring bullets (the generic guidance mentions skip_specs and must not flip the branch). Kills verified by re-running both mutations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…; distinct footer for unhonorable markers Fourth review round (alfred-style simulator, mutation adversary, cold reader): - The archive skill - the terminal step of the skip_specs journey - still told agents to warn that any non-done artifact is incomplete, so the one change every other surface calls complete triggered a false confirmation prompt. Both prompt variants now accept done or skipped; skills regenerated, hashes updated. - The validate/change next-steps footer now distinguishes the two marker errors: a conflict leads with deleting the spec files, an unhonorable marker (invalid metadata, no files) leads with fixing .openspec.yaml instead of telling the user to delete files that don't exist. - The archive comment claiming 'archive never accepts a change validate rejects' overclaimed: an UNMARKED zero-delta change still archives with only non-blocking warnings, a pre-existing gap this PR does not change. The comment now says exactly that. - validateChange's marker scope documented (proposal-level pass; the conflict gate lives in validateChangeDeltaSpecs); the skip_specs schema comment and agent-contract now state the generates-under-specs/ prefix as the contract custom schemas inherit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
One remaining cross-surface trust gap: readSkipSpecsMarker validates only the Zod shape, not whether schema actually resolves. I reproduced .openspec.yaml with schema: does-not-exist and skip_specs: true: openspec validate refactor reports valid with exit 0, while openspec status rejects the same metadata; archive can likewise bypass delta validation when specs/ is empty. Please route the marker through the same resolvable-metadata contract as status and instructions, then add this adversarial case.
2 participants
Fixes #1384.
Status: ready for re-review (round 2 review feedback addressed).
What was missing: the propose guidance said "leave Modified Capabilities empty if no requirement changes," but
openspec validateunconditionally rejects a change with zero deltas (CHANGE_NO_DELTAS). A pure refactor — which Recipe 5 explicitly documents as having no spec delta — followed the guidance and then failed at validate time, after proposal/design/tasks were already written. There was no way to say "this change intentionally touches no specs."What it does: a change can now declare
skip_specs: truein its.openspec.yaml:openspec validateaccepts zero deltas for such a change (INFO note instead of ERROR). The marker is honored only when the file parses under the sharedChangeMetadataSchema— metadata that status/instructions would reject (e.g. missingschema) yields an explicit error instead of a silently-honored marker.specs/is an explicit conflict error, never a silent drop.tasks), tracked separately soopenspec statusrenders them as[~] skipped, not[x]done.openspec instructions specson such a change emits only a warning naming the marker (no creation directive or template), and the--jsonpayload carries the sameskipped/warningfields;instructions applycounts skipped artifacts as present, so custom schemas whose apply phase requires the specs artifact don't dead-end.specs/(rootspec.md, headerless files, stray notes), andopenspec archiveruns the same check, so validate and archive always agree. A metadata file that mentionsskip_specsbut isn't valid YAML gets the explicit invalid-metadata error rather than a misleading zero-delta hint.continue/updateskill templates anddocs/agent-contract.mddocument the newskippedstatus and instructions fields (skills regenerated, golden hashes updated);docs/cli.mddocuments the marker and the[~]rendering.CHANGE_NO_DELTASguidance, propose/specs schema instructions, and proposal template now point at the marker ("do not invent a requirement just to satisfy validation"), and Recipe 5 shows it.Proof (real CLI output):
Tests cover accept/reject/conflict/malformed-metadata/adversarial-metadata paths (
test/core/validation.skip-specs.test.ts), the graph unblock and skipped status (instruction-loader.test.ts), and the metadata schema. Full suite green except the 17 known environment-only zsh-installer failures (#1321).Notes: per-change and explicit by design — unlike the project-wide
requireSpecDeltasconfig proposed in #977, the marker is a reviewable statement in the change itself. Archive enforces the marker (conflict and unhonorable-metadata states block), and its behavior-critical paths are mutation-tested: reverting any of the 12 key behaviors kills at least one test. Deliberately out of scope: an unmarked zero-delta change still archives with only non-blocking warnings — that gap predates this PR (pinned by the existing archive tests) and closing it would change behavior for changes that never opted into anything; the archive code comment states the boundary explicitly.