feat(validate): accept zero-delta changes that declare skip_specs - #1399
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 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 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
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.
Deploying openspec-docs with
|
| Latest commit: |
e3c8d34
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://857ff3cb.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.
|
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
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
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.
|
Fixed in b805f4b + d7ceca9. The marker is now routed through the full loadable-metadata contract, in two steps that mirror status/instructions exactly:
Adversarial cases pinned in tests: unknown schema (validate both paths + archive), exists-but-unparseable schema, parses-but-invalid schema, extension alias |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-review at 349c4e6: the marker now requires the same loadable-schema contract as status and instructions, and the reproduced unknown-schema case fails validate and blocks archive instead of bypassing delta validation. The added unloadable-schema, unreadable-metadata, symlink, false-marker, project-schema, and normalized-path cases are sound; 165 focused tests, clean skill regeneration, build, and the full exact-head CI and CodeQL matrix pass.
Squashed for rebase; see PR #1399 for the full commit history.
349c4e6 to
e3c8d34
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 skip_specs marker now uses the same loadable-metadata contract as status/instructions, conflicts fail closed across validate/archive, skipped artifacts compose correctly with the transitive proposal flow, and all 2,148 local tests plus required CI pass.
|
@clay-good How does skip-specs work for schemas that don't have spec defined artifacts? i.e what happens if schema is not spec-driven? seeems like a workflow specific flag than a generalized flag |
It works for any resolvable schema, not just spec-driven ones: validate demands a spec delta from every change regardless of schema, so skip_specs is the escape hatch that lets a non-spec-driven change pass, and the artifact-completion step is a harmless no-op there since it only touches artifacts whose generates path lives under specs/, but you're right that a cleaner generalized form is a schema-level "requires specs" declaration rather than a per-change marker. |
|
Since this is already merged, I’d keep |
3 participants
Fixes #1384.
Status: ready for re-review (round 5 feedback addressed: the marker now requires the full loadable-metadata contract).
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 metadata would load for status/instructions: the file parses under the sharedChangeMetadataSchema, the schema name is a known schema, and the schema itself loads viaresolveSchema. Anything status/instructions would reject (missingschema, unknown name, a schema.yaml that exists but does not parse, a metadata file that exists but cannot be read) 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, symlinks — the artifact graph’s globs follow them, so they count as content), 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.