fix(cli): prevent silent spec drop in the spec-driven workflow#1277
fix(cli): prevent silent spec drop in the spec-driven workflow#1277clay-good wants to merge 10 commits into
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:
📝 WalkthroughWalkthroughAdds a ChangesPrevent Silent Spec Drop
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
clay-good
force-pushed
the
prevent-silent-spec-drop
branch
from
db7167a to
23f3c10
Compare
clay-good
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md (1)
1-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDocument that
--yesdoes not bypass the archive validation guard.The PR objectives explicitly state that
--yesdoes not bypass the guard, but this spec does not mention--yesbehavior. Since archive has confirmation flows for--no-validate, spec updates, and incomplete tasks, users might reasonably expect--yesto force through validation failures. Clarify that--yesonly affects confirmation prompts, not the validation gate itself.Consider adding a scenario such as:
Scenario:
--yesdoes not bypass validation
- WHEN executing
openspec archive change-name --yes- AND the change has no delta specs and neither
--skip-specsnor--no-validateis provided- THEN the archive is still blocked with
CHANGE_NO_DELTAS- AND the change is not archived
🤖 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 `@openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md` around lines 1 - 51, Clarify the archive spec’s validation behavior by explicitly stating that `--yes` only skips confirmation prompts and does not bypass the validation gate. Update the Archive Validation section in spec.md to add a scenario for `openspec archive change-name --yes` showing that, without `--skip-specs` or `--no-validate`, a spec-less change still fails with `CHANGE_NO_DELTAS` and is not archived. Refer to the existing “Archive Validation” and “Skip Specs Option” requirements so the new scenario aligns with the current `openspec archive` behavior.
🧹 Nitpick comments (4)
openspec/changes/prevent-silent-spec-drop/tasks.md (1)
21-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit test coverage for
--yesnot bypassing the validation guard.The PR objectives state that
--yesdoes not bypass the guard, but the test checklist does not explicitly verify this. Item 5.3 tests--no-validatewith--yes, but there is no test for--yesalone on a no-delta change. Consider adding:
- 5.X Archive: change with no delta specs and
--yes(without--skip-specsor--no-validate) → still blocked withCHANGE_NO_DELTAS, non-zero exit🤖 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 `@openspec/changes/prevent-silent-spec-drop/tasks.md` around lines 21 - 31, Add explicit archive test coverage to verify that `--yes` alone does not bypass the no-delta-specs guard. In the test checklist for the archive flow, add a case alongside the existing archive scenarios in tasks.md that exercises a change with no `specs/` using `--yes` without `--skip-specs` or `--no-validate`, and assert it still fails with `CHANGE_NO_DELTAS` and a non-zero exit. Keep this aligned with the existing archive coverage entries so the behavior is validated by the same archive test suite.openspec/changes/prevent-silent-spec-drop/design.md (2)
45-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
The code block contains TypeScript-style comments. Add
typescriptto satisfy markdownlint and improve syntax highlighting.+```typescript
// before: if (hasDeltaSpecs) { run validateChangeDeltaSpecs }
// after: if (!options.skipSpecs) { run validateChangeDeltaSpecs }🤖 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 `@openspec/changes/prevent-silent-spec-drop/design.md` around lines 45 - 48, The fenced code block in the design document is missing a language specifier, which triggers markdownlint and reduces syntax highlighting. Update the markdown fence in the relevant snippet to use a TypeScript language tag, and keep the existing comment lines unchanged so the example remains readable and correctly highlighted.
9-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
The code block contains TypeScript-style pseudocode. Add
typescriptto satisfy markdownlint and improve syntax highlighting.+```typescript
if (totalDeltas === 0) {
issues.push({ level: 'ERROR', path: 'file', message: CHANGE_NO_DELTAS });
}🤖 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 `@openspec/changes/prevent-silent-spec-drop/design.md` around lines 9 - 13, Add the missing language tag to the fenced code block in the design document so the pseudocode is marked as TypeScript. Update the markdown fence around the snippet containing totalDeltas and issues.push to use a typescript code fence, keeping the existing code unchanged, so markdownlint passes and syntax highlighting works.openspec/changes/prevent-silent-spec-drop/proposal.md (1)
18-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
This block shows a shell session. Add
shellorbashto satisfy markdownlint and enable syntax highlighting.+```shell
$ openspec validate silent-drop
...
$ openspec archive silent-drop --yes
...exit 0 — archived; no openspec/specs/ directory created
🤖 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 `@openspec/changes/prevent-silent-spec-drop/proposal.md` around lines 18 - 29, The fenced shell-session example in proposal.md is missing a language tag, so update the code fence around the openspec commands to use a shell-friendly specifier such as shell or bash. Keep the existing content unchanged and ensure the block is still clearly identified as a terminal session for markdownlint and syntax highlighting.
🤖 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.
Outside diff comments:
In `@openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md`:
- Around line 1-51: Clarify the archive spec’s validation behavior by explicitly
stating that `--yes` only skips confirmation prompts and does not bypass the
validation gate. Update the Archive Validation section in spec.md to add a
scenario for `openspec archive change-name --yes` showing that, without
`--skip-specs` or `--no-validate`, a spec-less change still fails with
`CHANGE_NO_DELTAS` and is not archived. Refer to the existing “Archive
Validation” and “Skip Specs Option” requirements so the new scenario aligns with
the current `openspec archive` behavior.
---
Nitpick comments:
In `@openspec/changes/prevent-silent-spec-drop/design.md`:
- Around line 45-48: The fenced code block in the design document is missing a
language specifier, which triggers markdownlint and reduces syntax highlighting.
Update the markdown fence in the relevant snippet to use a TypeScript language
tag, and keep the existing comment lines unchanged so the example remains
readable and correctly highlighted.
- Around line 9-13: Add the missing language tag to the fenced code block in the
design document so the pseudocode is marked as TypeScript. Update the markdown
fence around the snippet containing totalDeltas and issues.push to use a
typescript code fence, keeping the existing code unchanged, so markdownlint
passes and syntax highlighting works.
In `@openspec/changes/prevent-silent-spec-drop/proposal.md`:
- Around line 18-29: The fenced shell-session example in proposal.md is missing
a language tag, so update the code fence around the openspec commands to use a
shell-friendly specifier such as shell or bash. Keep the existing content
unchanged and ensure the block is still clearly identified as a terminal session
for markdownlint and syntax highlighting.
In `@openspec/changes/prevent-silent-spec-drop/tasks.md`:
- Around line 21-31: Add explicit archive test coverage to verify that `--yes`
alone does not bypass the no-delta-specs guard. In the test checklist for the
archive flow, add a case alongside the existing archive scenarios in tasks.md
that exercises a change with no `specs/` using `--yes` without `--skip-specs` or
`--no-validate`, and assert it still fails with `CHANGE_NO_DELTAS` and a
non-zero exit. Keep this aligned with the existing archive coverage entries so
the behavior is validated by the same archive test suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fcff2276-c41b-4632-83a5-963d10eca0db
📒 Files selected for processing (6)
openspec/changes/prevent-silent-spec-drop/.openspec.yamlopenspec/changes/prevent-silent-spec-drop/design.mdopenspec/changes/prevent-silent-spec-drop/proposal.mdopenspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.mdopenspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.mdopenspec/changes/prevent-silent-spec-drop/tasks.md
✅ Files skipped from review due to trivial changes (1)
- openspec/changes/prevent-silent-spec-drop/.openspec.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.md
clay-good
force-pushed
the
prevent-silent-spec-drop
branch
from
23f3c10 to
48ea8ae
Compare
clay-good
changed the title
clay-good
force-pushed
the
prevent-silent-spec-drop
branch
from
48ea8ae to
af48253
Compare
clay-good
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (4)
openspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.md (1)
14-20: 📐 Maintainability & Code Quality | 🔵 TrivialClarify how "partial" specs are detected without referencing layer 2 directly.
The scenario references "partial" delta specs, but this file doesn't define what constitutes partial. The capability-coverage rule (from the layer 2 dependency) defines this. Consider adding a brief note or reference so readers of this standalone spec understand what "partial" means, e.g., "where declared capabilities lack corresponding delta spec files per the capability-coverage rule."
🤖 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 `@openspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.md` around lines 14 - 20, Clarify the meaning of “partial” delta specs in the scenario so the standalone spec is understandable: update the wording in the spec’s scenario around openspec archive to define partial as declared capabilities that do not have corresponding delta spec files, using the capability-coverage rule as the reference. Keep the existing scenario structure and make the detection criteria explicit without mentioning layer 2 directly.openspec/changes/prevent-silent-spec-drop/proposal.md (1)
22-29: 📐 Maintainability & Code Quality | 🔵 TrivialAdd shell language tag to the fenced code block.
The reproduction example is missing a language identifier (e.g.,
bashorshell). Adding one improves rendering and copy-paste ergonomics.🤖 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 `@openspec/changes/prevent-silent-spec-drop/proposal.md` around lines 22 - 29, The fenced reproduction block in the proposal is missing a shell language tag, so update the code block in the document to use a shell/bash identifier. Locate the example block describing the validate/archive commands and add the language tag to that fenced snippet so it renders and copies correctly.openspec/changes/prevent-silent-spec-drop/tasks.md (1)
9-9: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test scenario for unparseable-proposal fail-open behavior.
Task 1.4 specifies "fail-open on unparseable proposal," but no test in section 6 explicitly covers this. Add a test ensuring that a malformed proposal (e.g., unclosed code fence, invalid frontmatter) does not trigger coverage errors and does not crash the validator.
🤖 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 `@openspec/changes/prevent-silent-spec-drop/tasks.md` at line 9, Add a test case for the fail-open path in validateChangeCapabilityCoverage(changeDir) within src/core/validation/validator.ts. Cover a malformed proposal (such as invalid frontmatter or an unclosed code fence) and assert the validator does not throw and does not emit capability coverage errors when the proposal cannot be parsed. Place the test alongside the existing validator coverage tests so the unparseable-proposal behavior is explicitly verified.openspec/changes/prevent-silent-spec-drop/design.md (1)
49-52: 📐 Maintainability & Code Quality | 🔵 TrivialClarify how the
specsartifact is resolved.Decision 3 gives two ways to find the artifact (
graph.getArtifact('specs')vs. the artifact whosegeneratescoversspecs/**). These diverge when the artifact has a different name. Pick one authoritative method and document when the other applies, or state that both must agree.🤖 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 `@openspec/changes/prevent-silent-spec-drop/design.md` around lines 49 - 52, Clarify the schema-aware lookup for the `specs` artifact by choosing one authoritative resolution path in the design and using it consistently in the validation flow: either rely on `graph.getArtifact('specs')` as the primary check or define the fallback based on the artifact whose `generates` includes `specs/**`, but do not leave both as interchangeable. Update the `validateChangeDeltaSpecs`/schema-graph logic to state exactly when the alternate lookup is used, and make sure the documented rule matches the actual behavior in the resolved schema path so the `hasValidationErrors → ArchiveBlockedError` / `return null` handling applies only when a `specs` artifact is truly present.
🤖 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.
Nitpick comments:
In `@openspec/changes/prevent-silent-spec-drop/design.md`:
- Around line 49-52: Clarify the schema-aware lookup for the `specs` artifact by
choosing one authoritative resolution path in the design and using it
consistently in the validation flow: either rely on `graph.getArtifact('specs')`
as the primary check or define the fallback based on the artifact whose
`generates` includes `specs/**`, but do not leave both as interchangeable.
Update the `validateChangeDeltaSpecs`/schema-graph logic to state exactly when
the alternate lookup is used, and make sure the documented rule matches the
actual behavior in the resolved schema path so the `hasValidationErrors →
ArchiveBlockedError` / `return null` handling applies only when a `specs`
artifact is truly present.
In `@openspec/changes/prevent-silent-spec-drop/proposal.md`:
- Around line 22-29: The fenced reproduction block in the proposal is missing a
shell language tag, so update the code block in the document to use a shell/bash
identifier. Locate the example block describing the validate/archive commands
and add the language tag to that fenced snippet so it renders and copies
correctly.
In `@openspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.md`:
- Around line 14-20: Clarify the meaning of “partial” delta specs in the
scenario so the standalone spec is understandable: update the wording in the
spec’s scenario around openspec archive to define partial as declared
capabilities that do not have corresponding delta spec files, using the
capability-coverage rule as the reference. Keep the existing scenario structure
and make the detection criteria explicit without mentioning layer 2 directly.
In `@openspec/changes/prevent-silent-spec-drop/tasks.md`:
- Line 9: Add a test case for the fail-open path in
validateChangeCapabilityCoverage(changeDir) within
src/core/validation/validator.ts. Cover a malformed proposal (such as invalid
frontmatter or an unclosed code fence) and assert the validator does not throw
and does not emit capability coverage errors when the proposal cannot be parsed.
Place the test alongside the existing validator coverage tests so the
unparseable-proposal behavior is explicitly verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f10cf86-9ff2-41c5-b71d-d22d37a7d4e3
📒 Files selected for processing (8)
openspec/changes/prevent-silent-spec-drop/.openspec.yamlopenspec/changes/prevent-silent-spec-drop/design.mdopenspec/changes/prevent-silent-spec-drop/proposal.mdopenspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.mdopenspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.mdopenspec/changes/prevent-silent-spec-drop/specs/cli-validate/spec.mdopenspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.mdopenspec/changes/prevent-silent-spec-drop/tasks.md
✅ Files skipped from review due to trivial changes (2)
- openspec/changes/prevent-silent-spec-drop/.openspec.yaml
- openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.md
clay-good
force-pushed
the
prevent-silent-spec-drop
branch
from
af48253 to
3e1e8fb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openspec/changes/prevent-silent-spec-drop/proposal.md (1)
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
The shell commands should specify
shellorbashfor proper syntax highlighting and accessibility.-``` +# ```shell $ openspec validate silent-drop # exit 1: CHANGE_NO_DELTAS $ openspec archive silent-drop --yes # exit 0: "archived"; no openspec/specs/ created🤖 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 `@openspec/changes/prevent-silent-spec-drop/proposal.md` around lines 22 - 29, Add a shell language specifier to the fenced command block in the proposal so the openspec validate/archive examples are rendered as shell/bash code, updating the markdown fence where the commands are shown and keeping the rest of the example text unchanged.
🤖 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.
Nitpick comments:
In `@openspec/changes/prevent-silent-spec-drop/proposal.md`:
- Around line 22-29: Add a shell language specifier to the fenced command block
in the proposal so the openspec validate/archive examples are rendered as
shell/bash code, updating the markdown fence where the commands are shown and
keeping the rest of the example text unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd5d7ac5-97a7-4932-8db2-4d12744f7721
📒 Files selected for processing (8)
openspec/changes/prevent-silent-spec-drop/.openspec.yamlopenspec/changes/prevent-silent-spec-drop/design.mdopenspec/changes/prevent-silent-spec-drop/proposal.mdopenspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.mdopenspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.mdopenspec/changes/prevent-silent-spec-drop/specs/cli-validate/spec.mdopenspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.mdopenspec/changes/prevent-silent-spec-drop/tasks.md
✅ Files skipped from review due to trivial changes (3)
- openspec/changes/prevent-silent-spec-drop/.openspec.yaml
- openspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.md
- openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.md
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
This is the right consolidation direction, but one contract gap needs fixing before implementation: validate also needs the same schema-aware delta gate as archive. On current main, src/commands/validate.ts calls validator.validateChangeDeltaSpecs(changeDir) unconditionally in single and bulk validation, and validateChangeDeltaSpecs emits CHANGE_NO_DELTAS when there is no specs/ directory. So a proposal-only schema with no specs artifact would still fail openspec validate, even though the design says #997 is exempt and archive/validate should agree. Please add tasks and tests so validate resolves the change schema and only runs the existing delta-spec validation when schemaProducesDeltaSpecs(...) is true. Coverage should include proposal-only + no specs passes validate, while spec-driven + no specs still fails with CHANGE_NO_DELTAS.
clay-good
force-pushed
the
prevent-silent-spec-drop
branch
from
3e1e8fb to
04a386d
Compare
|
Thanks @alfred-openspec and CodeRabbit — addressed all of it in the proposal/design/specs/tasks (no implementation yet; this is still the change proposal). @alfred-openspec — the validate contract gap (the important one). You're right: making only
Captured in CodeRabbit:
The proposal continues to pass its own new rules ( |
clay-good
force-pushed
the
prevent-silent-spec-drop
branch
2 times, most recently
from
6d3298d to
9ae0781
Compare
|
Ran a deep self-review (three independent passes: consistency, spec-delta correctness, and adversarial engineering) and folded the findings into the proposal/design/specs/tasks. Still proposal-only; Substantive corrections:
Re #1252 / #1246 (flagged as a possible conflict): verified complementary, not conflicting. #1246 is the |
|
Prerequisite: land #1252 before this PR. #1252 (APPROVED) is the tactical fix for the distinct |
Deterministic, CLI-enforced prevention of silent spec drop in the spec-driven workflow, addressing one root fault (correctness decisions in agent prompts) across four layers: schema-aware delta gate shared by validate+archive (Fission-AI#997), the non-transitive apply-gate loop fix (incorporates Fission-AI#1250), the keystone (all archive templates call `openspec archive` — Fission-AI#656/Fission-AI#863), and a specced archived-drift audit. Hardened across three review rounds (alfred-openspec, CodeRabbit), an internal adversarial pass, and an open-PR coordination scan: - reconciles Fission-AI#977 (allow-specless) via the schema-aware gate + --skip-specs, deliberately not allowing silent specless archives under spec-driven; - coordinates with Fission-AI#902 (propose/ff spec discovery), the unify-template-generation-pipeline manifest, add-change-stacking-awareness (provides/touches markers + overlap warnings), and add-artifact- regeneration-support (complementary staleness); - rebases onto approved Fission-AI#1186/Fission-AI#1151/Fission-AI#1153/Fission-AI#1252; Fission-AI#1252 is a prerequisite. Closes Fission-AI#1212 Fission-AI#1260 Fission-AI#1222 Fission-AI#1264 Fission-AI#799 Fission-AI#656 Fission-AI#863 Fission-AI#913. Supersedes PRs Fission-AI#1250 Fission-AI#1271 Fission-AI#1241 Fission-AI#1233. Addresses Fission-AI#997 Fission-AI#977 Fission-AI#902 Fission-AI#164 Fission-AI#426 Fission-AI#911. Out of scope Fission-AI#1246 Fission-AI#1112 Fission-AI#1252(prereq) Fission-AI#1120 Fission-AI#827 Fission-AI#1265. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
force-pushed
the
prevent-silent-spec-drop
branch
from
9ae0781 to
dd24830
Compare
|
Ran an open-PR/issue coordination scan over the 13 source files this change will touch, to make sure it composes with in-flight work. Folded the results into the proposal/design (no scope change — sequencing + reconciliation only). Highlights: Design reconciliations (called out explicitly):
Mechanical sequencing (verified states 2026-06-29):
Architecture coherence: with No competing PR implements the deterministic CLI gate — this is not a duplicate of any open PR. |
… (silent-spec-drop §1) The deterministic CLI gate — the guarantee layer of prevent-silent-spec-drop. - extractDeclaredCapabilities: pure parser of a proposal's ## Capabilities (heading + bold-label forms; non-kebab → warning, not dropped). - schemaProducesDeltaSpecs(schema): true iff an artifact generates under specs/. - Validator.validateChangeCapabilityCoverage: one ERROR per declared capability with no specs/<id>/spec.md (presence-only; delta-format left to delta check). - makeDeltaRequirementResolver: memoized, schema-aware predicate shared by validate and archive; fail-safe to requiring deltas when resolution throws. - validate (single + bulk) and archive both gate delta validation + coverage on the shared predicate, so they agree by construction and proposal-only schemas pass both (Fission-AI#997). archive's self-defeating `if (hasDeltaSpecs)` gate is gone; it now blocks total, partial, and format (non-delta) drops. - instructions apply exits non-zero when blocked; spec-driven apply.requires is [specs, tasks] (the non-transitive apply gate now names specs). Updated archive/artifact-workflow tests for the new behavior; added the c1 apply fixture and declared-capabilities unit tests. Full suite green (the pre-existing zsh-installer env failures are unrelated). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…`; harden propose/ff loop (§2–3) Keystone: the archive skill/command and bulk-archive skill/command no longer assess sync by agent judgment or move the change with raw `mkdir`+`mv`. All four templates now run `openspec archive --json --yes`, which deterministically validates, syncs delta specs, and moves the change — so the §1 CLI guarantees are finally reachable from the agent path (Fission-AI#656, Fission-AI#863). On a block they surface the diagnostic and re-run; they never bypass with `--skip-specs`/`--no-validate` and never self-certify. Loop: propose/ff guardrails state the change is not apply-ready until every `apply.requires` artifact is `done` (for spec-driven that includes `specs`) — backing the schema gate from §1. Updated the template parity baselines for the 8 changed templates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ity (§4–5) - `openspec validate --archived`: detection-only audit that flags archived changes whose declared capabilities never reached openspec/specs/. Forward-only (pre-contract archives reported as not-auditable); exits 1 on drift. Does not regenerate content. Registered in CLI + completion registry. - spec-driven schema: the specs artifact instruction now states a change spec is a DELTA spec (## ADDED/MODIFIED/REMOVED/RENAMED), never a full ## Purpose/## Requirements spec, and that archive blocks otherwise; the proposal Capabilities instruction states ids must match the spec folder and that validate/archive enforce coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- capability-coverage.test.ts: validateChangeCapabilityCoverage + schemaProducesDeltaSpecs. - archive-spec-gate.test.ts: total/partial/format drop blocked; --yes does not bypass; --skip-specs bypasses; valid change archives; proposal-only archives. - validate-schema-aware.test.ts: proposal-only passes, spec-driven fails (single + bulk); --archived audit detects drift and clears after sync. - archive-uses-cli.test.ts: all four archive surfaces invoke `openspec archive`, no raw mv, no agent-driven sync delegation. - artifact-workflow: design-less change with specs+tasks stays apply-ready. Full suite: 1782 passing (pre-existing zsh-installer env failures excluded). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the changeset (minor) and mark all implementation tasks complete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implemented (this PR now contains the full change, not just the proposal)Built against current §1 — Deterministic CLI gate (the guarantee)
§2 — Loop: propose/ff guardrails state the change isn't apply-ready until every §3 — Keystone: all four archive surfaces ( §4 — Recovery: §5 — Clarity: the §6 — Tests: Heads-up: the new audit reveals pre-existing drift in 6 other active changesRunning Prerequisite reminder: land #1252 first (complementary |
…d-to-end Add test/cli-e2e/issue-1212-spec-drop.test.ts, a regression that drives the real built CLI against a spec-driven project reproducing Fission-AI#1212: - PR Fission-AI#1250: `instructions apply --json` on a change with no delta specs exits 1 with state=blocked, missingArtifacts includes "specs", and the spec-driven "Delta specs must exist…" hint. (This PR contains all of Fission-AI#1250 plus the archive guard its reviewer deferred.) - Fission-AI#1212 safeguard: `archive --json --yes` refuses — the change is not moved and no main spec is silently created. - Fission-AI#1212 happy path: once the delta spec exists, apply is unblocked and archive syncs the main spec. - Fission-AI#1212 recovery: `validate --archived` detects an already-archived change whose declared capability never reached openspec/specs/. Docs: proposal now records the e2e proof for Fission-AI#1212/Fission-AI#1250. Full e2e suite green (32), full suite 1786 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#1212 and #1250 are now fully addressed — with an end-to-end regressionAdded PR #1250 — fully superseded. This PR contains all of #1250's changes and the archive guard its reviewer explicitly deferred:
Issue #1212 — both requested safeguards + recovery, proven:
Verified e2e green (32 tests incl. the new 4); full suite 1786 passing; lint clean. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for pushing this through implementation. The overall direction still looks right, and the new archive/apply/keystone coverage is much stronger, but I found one blocking regression in the validate path.
openspec validate <change> now skips proposal validation entirely. In ValidateCommand.validateChange(), the report is built only from validateChangeDeltaSpecs() and validateChangeCapabilityCoverage() when the schema produces specs, and returns a clean empty report when it does not. That drops the pre-existing validator.validateChange(proposal.md) checks for required proposal sections and change-shape warnings/errors.
I reproduced this on the PR head with a change whose proposal.md is missing ## Why but has a valid delta spec. node bin/openspec.js validate bad --json exits 0 with no issues. Proposal-only schemas would now pass even with malformed or incomplete proposals too.
Please preserve the old proposal validation in both single and bulk validate, then layer the schema-aware delta gate on top. Add coverage for: invalid proposal + valid delta still fails validate, and proposal-only schema + invalid proposal still fails validate while proposal-only + valid proposal + no specs passes.
@alfred-openspec — blocking regression fixed (c6ad446)You're right that
Two honest notes on the fix:
Surfacing this also flagged three store-test fixtures with sub-50-char Where this PR standsImplementation is complete and green: 1789 tests pass, full e2e suite green, Not "merge-button" ready — two reasons, both external/decision-level: 1. Sequencing (PRs to land first):
2. A judgment call that's yours: the new audit ( Also worth a glance for reviewers: this is a deliberate behavior change (archive hard-blocks by default, |
…e (review) alfred-openspec found that `openspec validate <change>` skipped proposal validation: the report was built only from delta-spec + coverage checks (and was empty for proposal-only schemas), so a malformed proposal (e.g. missing ## Why) with a valid delta passed. Fix: add Validator.validateChangeProposal(changeDir) — full validateChange (required sections, Why length, What Changes) minus the delta-quantity rules, which are schema-gated/advisory — and run it ALWAYS in both single and bulk validate, layering the schema-aware delta gate + coverage on top. Result (covered by new tests): - invalid proposal + valid delta -> FAILS - proposal-only schema + invalid proposal -> FAILS - proposal-only + valid proposal + no specs -> PASSES (Fission-AI#997 preserved) Also: trim this change's own Why under the 1000-char limit (detail is in design.md) and lengthen three store-test proposal fixtures that had sub-50-char Why sections (now surfaced by the restored validation). Full suite 1789 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
force-pushed
the
prevent-silent-spec-drop
branch
from
c6ad446 to
6b1c915
Compare
…bish review) Reframe per the steer "more deterministic and grounded in reality": - Deterministic spine: the CLI computes the impact set (which downstream artifacts to revisit, in build order, with paths) as a pure function of schema edges + filesystem. The agent only rewrites prose. Grounded in real APIs already present: getUnlockedArtifacts (direct dependents), getBuildOrder (order), resolveArtifactOutputs (paths); reverse map built at graph.ts:82-87. - Replace fragile mtime staleness with a newline-normalized SHA-256 content digest (reproducible cross-platform). Drift = upstream digest vs recorded baseline; no baseline => "unknown", never a false positive. mtime and pure-git rejected with rationale; digest ledger is a separable, optional layer. - Explicit determinism boundary decision (CLI decides files/order/drift; agent rewrites). Skill MUST source the file list/order from `openspec status --impact`, never compute it. - Corrected all code citations to verified lines (graph.ts:82-87, instruction-loader.ts:366/429, status.ts); noted Fission-AI#1277's coverage helpers are not in this branch's base (coordinate, don't reuse). - Specs updated: artifact-graph Content Digest requirement; cli status digest + deterministic impact ordering; skill determinism + baseline-aware audit. tasks add digest/determinism/cross-platform tests + optional ledger section. Still validates clean under `openspec validate add-update-workflow --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ame refs - Digest ledger tracks DIRECT upstream digests; document that transitive drift emerges hop-by-hop as downstream is reconciled (no transitive bookkeeping). - Ground audit's no-baseline structural facts on signals available in this branch (missing/empty output, blocked/incomplete); capability-coverage is an add-on only when Fission-AI#1277's validateChangeCapabilityCoverage is present. - Add the "update revises only existing downstream; defer not-yet-created ones to /opsx:continue" rule across proposal/design/specs/tasks; impact entries now carry existence/status. - Note artifact-level (not file-level) granularity and that getDownstream terminates by the schema's acyclic guarantee. - Remove direct personal references from the docs. Validates clean under `openspec validate add-update-workflow --strict`; 10 deltas. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sign After a comprehensive sweep of open issues, PRs, and discussions, grounded the proposal in the complete adjacent landscape and answered the open design questions the cluster raises: - Fission-AI#783 (Cross-artifact quality review before apply) is now a primary Closes: it IS audit mode. Answer its open "new skill vs. extend validate" question via the determinism split — deterministic checks (drift/completeness/coverage) are CLI/validate-shaped; the semantic cross-artifact review is the skill. Added a skill spec scenario for the Fission-AI#783 patterns (scope contradiction, spec gap, duplication). - Discussion Fission-AI#1206 ("refine proposal now?") + prior-art PR Fission-AI#372: official answer is /opsx:update. - New design Decision 8 (command family): delineate /opsx:update from /opsx:clarify (Fission-AI#702, within-artifact), /opsx:review (Fission-AI#1251, plan-vs-code), and verify; /opsx:update consolidates update+regen+refine into one action, addressing skill-sprawl (Fission-AI#1263, Fission-AI#783). - Reuse, don't reinvent: audit's empty/incomplete check reuses Fission-AI#1098's artifactOutputComplete (same outputs.ts the digest helper lives in); capability coverage reuses Fission-AI#1277's validateChangeCapabilityCoverage. - New open questions: surface deterministic coherence in `validate` for a CI gate (Fission-AI#783-B, Fission-AI#829); naming reconciliation with Fission-AI#783's /opsx:refine. - Confirmed add-update-command* branches are the `openspec update` tool-file refresh (not artifact update) — no collision. Validates clean under --strict; 10 deltas; all relative links resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
This fixes the validate contract regressions I called out on the previous heads. I re-ran the focused validate/archive/capability coverage tests plus typecheck, and the schema-aware gate now preserves proposal validation while letting proposal-only schemas pass without delta specs.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for the quick fixes. One blocker remains: validate/archive still disagree for proposal-shape errors. In , proposal validation is still non-blocking in text mode and skipped entirely for JSON mode, while \ now fails malformed proposals via . I reproduced this on this branch with a change missing \ but containing a valid delta: \ exits 1, then \ exits 0 and archives/syncs it, printing only a non-blocking proposal warning. Since this PR’s contract is that archive blocks what validate rejects, archive needs to run the same proposal validation and block on errors before syncing/moving, including \ mode.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for the quick fixes. One blocker remains: validate/archive still disagree for proposal-shape errors.
In src/core/archive.ts, proposal validation is still non-blocking in text mode and skipped entirely for JSON mode, while openspec validate <change> now fails malformed proposals via validateChangeProposal().
I reproduced this on this branch with a change missing ## Why but containing a valid delta: openspec validate bad-prop exits 1, then openspec archive bad-prop --yes exits 0 and archives/syncs it, printing only a non-blocking proposal warning. Since this PR’s contract is that archive blocks what validate rejects, archive needs to run the same proposal validation and block on errors before syncing/moving, including --json mode.
clay-good
changed the title
clay-good
changed the title
…es (review) Archive now runs the same validateChangeProposal() as `openspec validate` and blocks on ERROR-level proposal issues before syncing/moving, in both output modes, so archive never archives a change that validate rejects. Warnings remain informative in human mode. The text-mode block path also sets a non-zero exit code, matching the spec's 'aborted with a non-zero exit code' scenarios. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@alfred-openspec — blocking regression fixed (12cf1a0): archive now blocks proposal errors in both modesYou're right that What was wrong: How it's fixed: archive now calls the same Repro (your scenario), on the new head: change missing
Coverage added (all three fail on the previous head, pass now): malformed proposal + valid delta blocked in text mode; same in Full suite: 1,824 passing; the only failures are pre-existing on the branch head and unrelated (zsh-installer EACCES sandbox failures + capstone journey 3, both fail identically on 6b1c915). CodeRabbit's fence nitpick: stale — the fenced command block it flagged (proposal.md ~22–29) was removed in an earlier revision; the change docs contain no fenced blocks at all now, and |
…e gate passes Journey 3 stubbed every non-spec artifact as '# <id> / Done.', which leaves proposal.md without the required ## Why and ## What Changes sections. Now that archive blocks on change validation errors instead of silently proceeding, the journey's archive step exited 1. The journey exercises store plumbing, not validation, so its proposal fixture should simply be valid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Ran this branch (7a4bfad) against current main: after a local rebuild, the new suites pass here — validate-schema-aware, archive-spec-gate, capability-coverage, declared-capabilities, 38 tests — and the schema-aware gate behaves as described for spec-driven vs proposal-only fixtures. One false positive in the coverage extractor that I'd flag as blocking, since it can refuse a change that shipped everything it declared. In the bold-label form, a ## Capabilities
- **New Capabilities**:
- `data-export`
- **Depends on**: the existing `cli-validate` command
- **Docs**: update `cli-archive` usage notesextracts |
|
Jun's blocker is valid. I reproduced the bold-label false positive at head Please fix this by resetting I also re-ran |
|
@clay-good Let me know if we're still continuing with this PR too. Similar to the other PR which tries to re-introduce the deterministc merging. |
4 participants
Status
Ready for review. Dogfooded as an OpenSpec change proposal (
openspec/changes/prevent-silent-spec-drop/) plus its full implementation.validate --strictpasses (the proposal satisfies its own new coverage rule), the full suite is 1789 passing, the e2e suite is green (32), and the CLI was rebuilt and re-verified against currentmain(2026-06-29).What was wrong
A spec-driven change could run the entire workflow — propose → implement → archive — and silently leave
openspec/specs/stale, with no error and no recovery. A forensic read across #1212, #1250, #656, #863, #164, #194, #997, #1260, #1222, #1264, #799 (plus live Discord reports) shows it isn't one bug but one architectural fault surfacing at four layers: correctness-critical decisions lived in agent prompts instead of deterministic CLI logic. Agents were asked to judge what a delta spec is, decide whether to sync, and track loop termination — and they judged wrong often enough to lose spec data.The four layers:
applyRequiresartifacts were done;spec-driven'sapplyRequires=[tasks]excludedspecs, soopenspec instructions specswas never called (non-deterministic — fix(apply): fail with exit 1 when apply instructions are blocked #1250's author couldn't reproduce it). (spec-driven fast-path workflow silently produces stale specs with no recovery path #1212, No Specs generated after /opsx:propose #1260)openspec archive: it judged sync state, optionally ran a separate agent sync, thenmkdir+mv, bypassing all CLI validation for agents. (question: why achive skill or command relies on llm to manually merge specs why it does not use openspec archive cli command? #656, AI commands and skills for/opsx:archivedo not invokeopenspec archiveCLI, unlike official docs and tutorials #863)if (hasDeltaSpecs), hiding three drop modes: total (no specs), partial (declaresa,b,c, shipsa), and format (a full spec where a delta belongs — the Discord case). (spec-driven fast-path workflow silently produces stale specs with no recovery path #1212, Error: Change 'my-task' has issues #164, Model tends to cheat around "no deltas found error" #194)How it was fixed
Ordered by leverage. Deterministic CLI checks are the guarantee; the schema/skill/docs changes remove agent judgment from the critical paths.
validate— the self-defeatingif (hasDeltaSpecs)gate is gone, gating is on!--skip-specs— running delta validation plus a new schema-aware declared-capability coverage rule. It blocks total (CHANGE_NO_DELTAS), partial (coverage), and format ("No delta sections found") drops. It applies only when the schema graph includes aspecsartifact, so proposal-only schemas are unaffected ([Proposal] Route change validation by workflow schema (delta vs proposal-only) #997).--yesno longer bypasses validation;--skip-specs/--no-validateare the explicit overrides.spec-drivenbecomesapply.requires: [specs, tasks], and propose/ff walk the full schema build order instead of stopping atapplyRequires. Incorporates fix(apply): fail with exit 1 when apply instructions are blocked #1250./opsx:archive, bulk archive, and their commands) now runopenspec archive --json— which validates, syncs, and moves — instead of judging sync state and moving files agent-side. This makes the CLI gate reachable from agent workflows (question: why achive skill or command relies on llm to manually merge specs why it does not use openspec archive cli command? #656, AI commands and skills for/opsx:archivedo not invokeopenspec archiveCLI, unlike official docs and tutorials #863).openspec instructions applyexits non-zero when blocked (incorporates fix(apply): fail with exit 1 when apply instructions are blocked #1250);openspec validate --archivedaudits already-archived changes for capabilities that never reachedopenspec/specs/(detection, not regeneration); the specs/apply/archive prompts clarify delta-vs-full-spec (Model tends to cheat around "no deltas found error" #194, Stage 3 instruction ambiguous - "Update specs/" unclear about source/destination #426).Replication / proof
Original bug reproduced on current
main:openspec validateexits 1 (CHANGE_NO_DELTAS) whileopenspec archive --yesexits 0 and archives with nothing written toopenspec/specs/; andgrep "openspec archive" archive-change.tsreturns no match — the skill never called the CLI.Now fixed, proven end-to-end by
test/cli-e2e/issue-1212-spec-drop.test.ts, which drives the real built CLI against a spec-driven project:instructions apply --jsonon a change with no delta specs exits 1,state=blocked,missingArtifactsincludesspecs(this is all of fix(apply): fail with exit 1 when apply instructions are blocked #1250).archive --json --yesrefuses — the change is not moved and no main spec is silently created (the spec-driven fast-path workflow silently produces stale specs with no recovery path #1212 safeguard).validate --archiveddetects an already-archived change whose declared capability never reachedopenspec/specs/(recovery).Coverage-rule safety: validated against a 28-proposal corpus — zero false extractions, and it surfaced 7 real pre-existing inconsistencies. Full suite 1789 passing (the pre-existing zsh-installer env failures are unrelated).
Notes / scope
/opsx:archivedo not invokeopenspec archiveCLI, unlike official docs and tutorials #863 (skill bypassed the CLI), /opsx-archive can offer 'Sync now' even when openspec-sync-specs is not installed (core profile) #913 (archive's "Sync now" needed an uninstalled skill — eliminated now that archive syncs via the CLI).openspec-sync-specsskill/instructions #1120 (Junie packaging), Feature Request: Target Main Specs on Sync and Archive #827 (main-spec evolution), Issues regarding renamed files, archived filenames, and the workflow reading archived files. #1265 (change naming / archived reads), and regenerating content for changes already archived without specs (the audit only detects them).design.md; implementation sequencing (CLI gate → loop → keystone → audit/docs) is intasks.md.🤖 Generated with Claude Code