From dd24830911922a409f4876854fcd47353f4f8a28 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 09:35:00 -0500 Subject: [PATCH 01/10] docs(proposal): add prevent-silent-spec-drop change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (#997), the non-transitive apply-gate loop fix (incorporates #1250), the keystone (all archive templates call `openspec archive` — #656/#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 #977 (allow-specless) via the schema-aware gate + --skip-specs, deliberately not allowing silent specless archives under spec-driven; - coordinates with #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 #1186/#1151/#1153/#1252; #1252 is a prerequisite. Closes #1212 #1260 #1222 #1264 #799 #656 #863 #913. Supersedes PRs #1250 #1271 #1241 #1233. Addresses #997 #977 #902 #164 #426 #911. Out of scope #1246 #1112 #1252(prereq) #1120 #827 #1265. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../prevent-silent-spec-drop/.openspec.yaml | 2 + .../prevent-silent-spec-drop/design.md | 168 ++++++++++++++++++ .../prevent-silent-spec-drop/proposal.md | 110 ++++++++++++ .../specs/cli-archive/spec.md | 81 +++++++++ .../specs/cli-artifact-workflow/spec.md | 63 +++++++ .../specs/cli-validate/spec.md | 143 +++++++++++++++ .../specs/opsx-archive-skill/spec.md | 93 ++++++++++ .../changes/prevent-silent-spec-drop/tasks.md | 66 +++++++ 8 files changed, 726 insertions(+) create mode 100644 openspec/changes/prevent-silent-spec-drop/.openspec.yaml create mode 100644 openspec/changes/prevent-silent-spec-drop/design.md create mode 100644 openspec/changes/prevent-silent-spec-drop/proposal.md create mode 100644 openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md create mode 100644 openspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.md create mode 100644 openspec/changes/prevent-silent-spec-drop/specs/cli-validate/spec.md create mode 100644 openspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.md create mode 100644 openspec/changes/prevent-silent-spec-drop/tasks.md diff --git a/openspec/changes/prevent-silent-spec-drop/.openspec.yaml b/openspec/changes/prevent-silent-spec-drop/.openspec.yaml new file mode 100644 index 0000000000..34f9314d22 --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-29 diff --git a/openspec/changes/prevent-silent-spec-drop/design.md b/openspec/changes/prevent-silent-spec-drop/design.md new file mode 100644 index 0000000000..5ff0fd4c1e --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/design.md @@ -0,0 +1,168 @@ +## Context + +Completion in OpenSpec is judged by **file existence** (`artifact-graph/state.ts`), and the workflow **skills make correctness-critical decisions by agent judgment**. The result is that a spec-driven change can finish with `openspec/specs/` silently stale. A forensic read of the cluster (below) shows four reinforcing layers; this design fixes each at the most deterministic layer available. + +### Evidence that prompt-level reasoning is the fault + +- PR #1250's author **could not reproduce** #1212 on a greenfield project — propose *did* create delta specs there. The failure is **agent- and path-dependent**, i.e. non-deterministic. #1250's reviewer (alfred-openspec) approved the apply fix but **explicitly required an archive-side guard as the "last line of defense"** and deferred it. This change is that guard. +- The archive skill (`archive-change.ts`) and its spec (`opsx-archive-skill`) **reimplement archive agent-side**: assess sync "by judgment," optionally call a separate `openspec-sync-specs` skill, then `mkdir`+`mv`. `grep "openspec archive" archive-change.ts` → no match. Filed as #656 and #863. +- The Discord report is the **format-drop case**: `specs//spec.md` was written as a full spec; `archive.ts:274`'s `/^## (ADDED|MODIFIED|REMOVED|RENAMED) Requirements/` regex did not match, so `hasDeltaSpecs=false` and sync was skipped. The agent's "no delta specs to sync" was the skill working as written (`archive-change.ts:60`). + +### The validate/archive inconsistency (the seam) + +`validateChangeDeltaSpecs` (`validator.ts:115`) already errors when `totalDeltas === 0` (`CHANGE_NO_DELTAS`) and when a present spec file has no delta headers (`missingHeaderSpecs` → "No delta sections found", `validator.ts:264`). `openspec validate` calls it unconditionally (`validate.ts:186`); `openspec archive` calls it only inside `if (hasDeltaSpecs)` (`archive.ts:282`). So the validator is *already correct* for total drop and the format-drop case — archive just never reaches it. + +## Goals / Non-Goals + +**Goals** +- One deterministic definition of completeness, enforced in the CLI and reachable by agents. +- Block total, partial, and format spec drops before archive moves anything. +- Skills delegate the critical action (archive + sync) and the loop termination to the CLI/schema, not agent judgment. +- Do not force deltas on schemas that legitimately have none (#997). +- Detect already-accumulated drift so it is recoverable. + +**Non-Goals** +- Authoring spec content, or regenerating spec content for changes already archived without it (agent-assisted; out of scope — we detect, we do not auto-write). +- Merge-time scenario loss when two changes modify the same requirement (#1246/#1112). +- Flagging delivered-but-undeclared specs (not data loss). + +## Decisions + +### Decision 1 (PRIMARY): fix the loop in the schema, then harden the skill + +`spec-driven` `apply.requires: [specs, tasks]`. The apply gate is a **non-transitive presence check**: `generateApplyInstructions` iterates `apply.requires` directly (`instructions.ts` ~359) and checks each named artifact's file existence — it does **not** walk transitive `requires`. That is exactly why the bug exists: under today's `apply.requires: [tasks]`, a change with `tasks.md` but no `specs/` is reported *ready*, because only `tasks` is checked and `tasks.md` exists. Adding `specs` to `apply.requires` is therefore the precise, deterministic fix — now `specs` is checked directly and a specs-less change is blocked. The apply skill's existing `state: blocked` handler (`apply-change.ts:52`) then fires. This is the fix #1212's reporter calls "Primary" and #1250 implemented for apply. + +This non-transitivity also resolves the conditional-`design` story: `design` is deliberately **not** in `apply.requires`, so a change without `design.md` stays apply-ready — "skip design for simple changes" keeps working precisely because the gate is non-transitive. We do **not** add `design` to `apply.requires`. + +The propose/ff skill loop already re-runs `openspec status` and continues until every `apply.requires` artifact is `done`; with `specs` now in that set, the loop cannot stop while `specs` is absent. The skill-template hardening is therefore narrow and concrete: **define termination as "every `apply.requires` artifact has status `done`"** (reading the live `applyRequires`), and never treat `tasks.md`'s existence alone as apply-ready. The schema change is the deterministic guarantee; the skill change just removes any lingering "stop at tasks" shortcut. + +Note: the `Artifact` schema (`artifact-graph/types.ts`) has no `optional` field, but that is irrelevant to the apply gate (which is non-transitive over `apply.requires`, not "all artifacts"). Earlier framing that called every artifact "transitively required, nothing to skip" was wrong — corrected here. + +### Decision 2 (KEYSTONE): the archive skill calls the CLI + +Rework the archive workflow so the skill: + +1. Resolves the change, then runs `openspec archive --json` (and `--yes` only to suppress *interactive* prompts — never `--skip-specs` or `--no-validate`, both of which are silent-drop bypasses). +2. On success, reports the structured result (specs synced, totals, archive path) **from the CLI's output** — it does not self-certify "specs look synced" and proceed. +3. On `ArchiveBlockedError` (missing/partial/non-delta specs, incomplete tasks, etc.), surfaces the diagnostic and **helps the user create or fix the delta spec**, then re-runs `openspec archive` and **trusts its exit** — it does not bypass with `--skip-specs`/`--no-validate`. + +**All archive surfaces must be reworked, not just one (this was a real gap):** `archive-change.ts` exports **two** templates — `getArchiveChangeSkillTemplate` (the `openspec-archive-change` skill) *and* `getOpsxArchiveCommandTemplate` (the `/opsx:archive` command) — and **both** contain the judgment-based sync step and a raw `mkdir`+`mv`. `bulk-archive-change.ts` contains the **same raw `mv`** in its two templates. If any of these is left calling `mv` directly, `/opsx:archive` or the bulk path still bypasses the CLI and the keystone is only half-applied (#863 persists). All four templates are in scope, each with a template-parity assertion. + +This deletes the agent-judged "assess sync state / proceed without sync" step and the raw `mv`. The CLI's deterministic `findSpecUpdates → buildUpdatedSpec → writeUpdatedSpec` becomes the only archive path. Without this, Decisions 3–5 never execute for agents — which is why every prior prompt-only fix (#1268/#1271/#1241/#1233) failed. (Note: `--no-validate` remains a *human* escape hatch on the CLI; the skill is forbidden from using it, and the recovery audit in Decision 6 flags any change archived with `specsUpdated: false`.) + +### Decision 3: the schema-aware delta gate applies to BOTH `validate` and `archive` + +The delta requirement (`CHANGE_NO_DELTAS` + coverage) is gated on a single shared predicate, `schemaProducesDeltaSpecs(schema)` — true iff *any artifact's `generates` glob writes under `specs/`* (in `spec-driven`, the `specs` artifact with `generates: specs/**/*.md`). Not a hardcoded schema name. This predicate is applied in **both** commands so they agree by construction: + +- **`archive`** — replace `if (hasDeltaSpecs)` with `if (!options.skipSpecs && schemaProducesDeltaSpecs(schema))`, then run `validateChangeDeltaSpecs` + coverage; failures flow through the existing `hasValidationErrors → ArchiveBlockedError` (JSON) / `return null` + exit-1 (text) path (`archive.ts:299-310`). +- **`validate`** — on current `main`, `src/commands/validate.ts` calls `validateChangeDeltaSpecs(changeDir)` **unconditionally** in single (`:186`) and bulk (`:252`) validation, and that emits `CHANGE_NO_DELTAS` whenever there is no `specs/`. So a proposal-only schema would *fail* `validate` even though `archive` now lets it through — re-introducing the very validate/archive disagreement this change exists to remove (raised by review, the #997 gap). Fix: `validate` resolves the change's schema and runs delta-spec validation (and coverage) **only when `schemaProducesDeltaSpecs(schema)` is true**. + +Result, by construction: +- `spec-driven` (has a `specs` artifact) + no specs → `CHANGE_NO_DELTAS` in **both** `validate` and `archive`. +- proposal-only (no `specs` artifact) + no specs → **passes both**. + +Both commands resolve the schema the same way the instructions/status commands already do — from `.openspec.yaml` via `loadChangeContext`/`resolveSchema` — so no new plumbing. **Fail-safe:** "indeterminate" means schema *resolution throws* (e.g. `SchemaLoadError` on a malformed project schema), **not** "metadata absent" — a change with no `.openspec.yaml` already resolves to `spec-driven` by default (`change-metadata.ts`), so it correctly gets the strict gate. When resolution throws, default to `schemaProducesDeltaSpecs = true` (require deltas), preserving today's strict behavior rather than silently relaxing it. + +**Performance:** `schemaProducesDeltaSpecs` must be **memoized by schema name within a command run** (the schema set is effectively constant). `listSchemas`/`resolveSchema` do ~20 `readdirSync`/`existsSync` calls each; recomputing per change in the bulk `validate` loop (`validate.ts:248-256`, concurrency 6) would be a real regression. Resolve once per distinct schema, not once per change. + +This single change blocks all three drop modes: +- **Total** → `CHANGE_NO_DELTAS`. +- **Format** → `missingHeaderSpecs` "No delta sections found" (already implemented; now reached). +- **Partial** → Decision 4. + +### Decision 4: deterministic, schema-aware capability coverage + +`validateChangeCapabilityCoverage(changeDir)` reads `proposal.md`, extracts declared capabilities, and requires each to have `specs//spec.md` **present**. Separation of concerns: coverage checks *presence*; delta well-formedness (including the format-drop case) is left to `validateChangeDeltaSpecs`, so a present-but-non-delta file yields one precise "No delta sections found" error, not two. + +#### The deterministic parsing contract (`extractDeclaredCapabilities`) + +Implemented on `ChangeParser`, reusing its section + code-fence machinery. Pure function of the proposal text: + +1. Locate the top-level `## Capabilities` section (bounded by the next `## `). Absent → no declarations (back-compatible with the 65/93 proposals without it). +2. Within it, only `### New Capabilities` and `### Modified Capabilities` subsections. +3. A capability id is the **first inline-code span** (`` `id` ``) of a top-level `- ` bullet there. +4. A capability id is the first inline-code span matching `^[a-z0-9]+(?:-[a-z0-9]+)*$`. The schema already mandates kebab-case ids; a bullet whose inline-code token is non-kebab (uppercase, non-ASCII) is **reported as a malformed-capability warning**, not silently dropped — silent dropping would let a partial drop survive vacuously (raised by review, m3). The author fixes the casing or the parser would otherwise miss it. +5. `None` / `_None_` / HTML-comment / empty subsections declare nothing. + +The contract keys on `### New Capabilities` / `### Modified Capabilities` **headings**. All 28 declaring proposals in the corpus use that heading form, but the schema's proposal-template instruction only describes **bold-label** form (`**New Capabilities**:`). To make the parser's contract enforceable rather than a silent guess: (a) Decision 7 tightens the proposal-template instruction to **mandate** the `## Capabilities` + `### New/Modified Capabilities` heading shape; and (b) the parser **also accepts the bold-label form** as a fallback, so a well-formed proposal under the documented instruction is never under-extracted. Without both, a future proposal could declare capabilities the parser doesn't see → coverage passes vacuously. + +Coverage applies only when the schema graph produces delta specs (same gate as Decision 3). + +**Declared id vs spec folder name (M1):** for a Modified capability the schema instructs authors to "use the existing spec folder name." Coverage therefore requires the declared id to equal the change's delta-spec folder (`specs//`), which `findSpecUpdates` also keys on — so they stay aligned. When they diverge (proposal names a capability differently than its on-disk folder), coverage errors; the error message **states the rule and suggests aligning the `## Capabilities` id to the existing spec folder name** rather than just "missing." This is intentional (it is a real inconsistency) but must be actionable, not cryptic. + +**Capability deletion / rename (M2):** removing a capability's requirements is expressed as a `## REMOVED Requirements` delta against the existing folder — that still produces `specs//spec.md`, so coverage and delta validation both pass. Deleting or renaming a whole capability/spec folder has no natural delta and is an explicit-intent operation: the **user** passes `--skip-specs` (the skill never does so on its own). This is documented and given a test scenario so the keystone skill does not deadlock on a legitimate deletion. + +#### Validated against the full corpus + +A prototype of this exact contract was run over all 93 in-repo proposals before finalizing: 28 declare capabilities (68 total); **zero false extractions**; **7 changes have a declared capability with no delta spec** — all genuine proposal↔spec inconsistencies (e.g. active `unify-template-generation-pipeline` declares 3 Modified capabilities, ships none). Two borderline shapes (docs/meta capabilities; naming mismatches where the spec folder differs from the declared id) are ERRORs by design — a listed-but-unspec'd capability is exactly the inconsistency a cohesive audit should surface. The fix is to align the two; the escape is the user passing `--skip-specs`. + +### Decision 5: apply exit-1 (earlier guardrail) + +`instructions apply` sets a non-zero exit when `state === 'blocked'` (text and JSON; JSON prints the payload first). Lets propose/fast-path loops and CI stop instead of reading a printed "Blocked" as success. Incorporates #1250. + +### Decision 6: recovery is detection, not regeneration + +`openspec validate --changes` already flags *active* drift once Decisions 3–4 land. Add an audit that flags **archived** changes whose declared capabilities have no corresponding `openspec/specs//spec.md` (and, equivalently, changes recorded as archived with `specsUpdated: false`), so existing silent debt (#1212's "no recovery path") is discoverable. This audit is a **new, specced** capability (a requirement in `cli-validate`), not just prose — surfaced via `openspec validate` (e.g. an `--archived`/audit mode). Reconstructing the lost spec content from a finished implementation is agent-assisted and out of scope; the audit points the user at what to rebuild. + +The audit is **forward-only**: it parses archived proposals with the same `extractDeclaredCapabilities` contract, which most pre-contract archives do not satisfy. It therefore reports *detectable* drift (canonical-shape proposals) and explicitly notes that pre-contract archives are not auditable — it must not claim completeness it cannot deliver. + +### Decision 7: delta-vs-full-spec clarity, and a stricter proposal-template contract (coherence) + +Tighten two prompts so the parser/coverage contract is something authors actually produce: (a) the `specs` artifact instruction and the apply/archive skill text so "delta spec" is unambiguous (it lives at `changes//specs//spec.md` and uses `## ADDED/MODIFIED/REMOVED/RENAMED Requirements`, never `## Purpose`/`## Requirements`), and the archive skill never says "no sync needed" — the CLI decides; and (b) the **proposal** artifact instruction so the `## Capabilities` section uses `### New Capabilities` / `### Modified Capabilities` headings with kebab-case ids matching the spec folder name (the shape Decision 4's parser enforces). This is the non-deterministic layer; the CLI gate is the guarantee behind it. (These prompt edits touch templates that are not separately specced as capabilities — `propose.ts`, `apply-change.ts`, `sync-specs.ts`, and the schema artifact instructions — so they are tasks without delta specs, by nature; the specced behavior lives in the CLI capabilities. `openspec-sync-specs`/`/opsx:sync` survives unchanged and is intentionally still agent-driven for the sync-without-archive intent — this change only removes archive's reliance on it.) + +## Coordination with in-flight work + +This change shares files and concepts with several active changes/PRs; it is designed to compose with them, not collide. + +- **`unify-template-generation-pipeline`** (introduces `WorkflowManifest` + `ArtifactSyncEngine` over `src/core/templates/*`). This change edits the *content* of six workflow templates (the four archive surfaces + `propose`/`ff-change` and the clarity edits); that change restructures *how* templates are assembled. They touch the same directory and will need rebasing whichever lands first, but the concerns are orthogonal (content vs. assembly). If the manifest lands first, the archive-template rework becomes a manifest entry; if this lands first, the manifest absorbs the reworked templates. Sequence by review readiness; flag for the maintainer. +- **`add-change-stacking-awareness`** (adds structured `provides`/`requires`/`touches` change metadata + overlap warnings). Two alignments: (1) its `provides` markers are the eventual **structured** source for "what capabilities does this change declare," which Decision 4 currently parses from the `## Capabilities` prose — the coverage check should be written so it can later read `provides` when present, with prose as the fallback; (2) its "**overlap warning when active changes touch the same capability**" is the *preventive* complement to the #1246/#1252 merge-drift fix — together they cover author-time warning, merge-time correctness, and (here) missing/partial/format completeness. Document the convergence so the two efforts don't grow divergent capability-declaration models. +- **`add-artifact-regeneration-support`** (mtime staleness at `/opsx:apply` → offer to regenerate downstream artifacts). Complementary and out of scope here: it addresses *intra-change* staleness (design edited after tasks), not specs dropped at archive. It is the original "update the proposal" concept; this change does not block or duplicate it. +- **#1252 / #1246** — complementary merge-drift fix; **land #1252 first** (see proposal). This change never touches `buildUpdatedSpec`. + +### Open-PR landscape for the 13 source files (verified 2026-06-29) + +A scan of all open PRs against the files this change will edit found no competing implementation of the deterministic gate, but real mechanical overlap to sequence around: + +- **Rebase onto these (APPROVED, in our files — likely land first):** #1186 (validate + validator: workspace/nested delta routing — re-verify coverage composes with nested delta discovery), #1151 (validator: ignore fenced code in delta specs), #1153 (apply-change template dedupe), #1252 (specs-apply.ts). Implement #1277's source edits *after* these to avoid the sharpest conflicts. +- **Fold in / supersede:** #1250 (apply exit-1 — fold in), #1271/#1241/#1233 (prompt-only archive/greenfield guidance on `archive-change.ts` — supersede). +- **Design-reconcile (same files, overlapping intent):** #977 (allow-specless-changes — reconciled via the schema-aware gate + `--skip-specs`; see proposal) and #902 (propose/ff spec discovery — compose the loop change). Both must be coordinated, not merged blindly. +- **Coordinate (independent features overlapping our templates/schema):** #1062 and #887 (config injection into apply/archive instruction templates — let #1277 define the archive template structure first, then they rebase their injection points), #844 (front-matter for archive/sync + `sync-specs.ts`), #1121/#1269 (schema.yaml prose conventions). #843 (large delta-merge rework across `change-parser.ts`/`specs-apply.ts`/`validator.ts`/`schema.yaml`) is the biggest raw overlap but is stale (Apr 2026) — flag its author to rebase after #1277. + +**Implementation-sequencing takeaway:** #1277 ships as proposal docs first (dogfooded). Before the source edits land, rebase onto the APPROVED set above; the conflict surface only becomes live when the source changes. None of these alters #1277's design — they affect ordering and line-level placement only. + +## Risks / Trade-offs + +- **[Risk] Reworking the archive skill changes a core workflow.** → Mitigation: the CLI already supports `--json`/`--yes`/`ArchiveBlockedError`; the skill becomes simpler and strictly safer. Template-parity tests guard the change. This is the highest-value, and highest-blast-radius, part — sequence it first and test heavily. +- **[Risk] Schema-awareness mis-detects which schemas need specs.** → Mitigation: drive it off the resolved artifact graph (`specs` artifact presence), with tests for spec-driven (required) and a proposal-only schema (#997, not required). +- **[Risk] False positive on an intentionally spec-less declared capability.** → Mitigation: it doesn't belong under `## Capabilities`; remove the line or use `--skip-specs`. Bounded to capabilities the proposal itself declares. +- **[Risk] Stricter archive surprises automation.** → Mitigation: `--skip-specs` is the one-flag migration, documented in the changeset. +- **[Trade-off] One-directional coverage** leaves delivered-but-undeclared specs unflagged — accepted (not data loss). + +## Migration Plan + +Minor behavior change shipped with a changeset note. No data migration. `openspec archive` now blocks the same changes `openspec validate` rejects (total/partial/format), but only for schemas with a `specs` artifact. Automation archiving intentionally spec-less changes adds `--skip-specs`. The drift audit lets existing projects find pre-existing debt. + +## Sequencing (for implementation) + +1. Decision 3 + 4 + 5 (deterministic CLI gate) — the guarantee. +2. Decision 1 (schema + propose/ff loop) — stops the drop at the source. +3. Decision 2 (archive skill → CLI) — makes 1–5 reachable by agents; test heavily. +4. Decision 6 (audit) + Decision 7 (docs) — recovery and coherence. + +## Build-readiness verification + +Each load-bearing assumption was checked against current `main` (rebuilt CLI; the committed `bin` bundle was stale) before committing to build: + +- **Bug reproduces on current code.** `openspec validate ` exits 1 (`CHANGE_NO_DELTAS`); `openspec archive --json --yes` exits 0 with `"specsUpdated": false` and moves the change — the silent drop, confirmed post-stores-refactor. +- **The keystone interface exists.** `openspec archive` already supports `--json` / `--yes` / `--skip-specs` / `--no-validate`, returns a structured `{ archive: { change, archivedAs, path, specsUpdated }, root }` on success, and throws `ArchiveBlockedError` (machine-readable diagnostic, non-zero exit) on block. The reworked skill can consume this directly — no new CLI surface required. +- **The validator already does the hard part.** `validateChangeDeltaSpecs` emits `CHANGE_NO_DELTAS` (total) and `missingHeaderSpecs` "No delta sections found" (the format-drop case); the fix is to stop gating it behind `hasDeltaSpecs`. +- **Coverage parsing is feasible and deterministic.** `ChangeParser` already has section + code-fence parsing; the extraction contract was prototyped over all 93 in-repo proposals (zero false extractions, 7 real inconsistencies). +- **Apply gate verified non-transitive.** `generateApplyInstructions` iterates `apply.requires` directly (not transitive `requires`), so a `tasks`-only `apply.requires` lets a specs-less change read ready; adding `specs` to `apply.requires` is the precise fix, and `design` (absent from `apply.requires`) stays optional. The loop fix is well-defined and does not depend on a non-existent `optional` field. +- **Reference accuracy verified (2026-06-29).** #1250 not merged (main is `apply.requires: [tasks]`); #194 and #1268 are closed (treated as prior art, not addressed/superseded); #913 is resolved by the keystone; #164 demoted to clarity. + +Residual implementation risks are captured in Risks/Trade-offs above; none are blockers. + +## Open Questions + +- **Greenfield ADDED-only ergonomics.** When the first change archives, the guard forces delta specs; archive then creates the main specs from ADDED deltas. Confirm the reworked archive skill makes creating ADDED deltas the obvious path, not `--skip-specs`. +- **Should the conceptual delta-vs-full-spec docs (#194/#426) be a separate docs change?** Leaning yes — keep this change's docs edits minimal and targeted, and file a focused docs follow-up. diff --git a/openspec/changes/prevent-silent-spec-drop/proposal.md b/openspec/changes/prevent-silent-spec-drop/proposal.md new file mode 100644 index 0000000000..ff20a81202 --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/proposal.md @@ -0,0 +1,110 @@ +## Why + +A spec-driven change can complete the whole workflow — propose, implement, archive — while **silently leaving `openspec/specs/` stale**, with no error and no recovery path. It is the most-reported correctness defect in the tracker, and a deep read of the issue cluster (#1212, #1250, #656, #863, #164, #194, #997, #1260, #1222, #1264, #799, and the live Discord reports) shows it is not one bug but **one architectural fault expressed at four layers**: + +> Correctness-critical decisions live in **agent prompts**, not in deterministic CLI logic. Skills are asked to *judge* what a "delta spec" is, *decide* whether to sync, and *track* a loop termination — and agents (GPT-5.x, Claude, others) get these judgments wrong often enough to silently lose spec data. + +### The four layers + +1. **The propose/ff loop stops too early (and non-deterministically).** Both skills loop over artifacts and *"Stop when all `applyRequires` artifacts are done"* (`ff-change.ts:72`), reading `applyRequires` from the schema. For `spec-driven`, `applyRequires` is `[tasks]` — it excludes `specs` — so the loop can end before `openspec instructions specs` is ever called. Because it depends on the agent's path through "ready" artifacts, it is intermittent: the author of PR #1250 **could not reproduce it** on a greenfield project, while others hit it constantly. Anything intermittent in a prompt must be made deterministic in the CLI. (#1212, #1260) + +2. **The archive skill bypasses the CLI entirely (the keystone).** `/opsx:archive` does **not** call `openspec archive`. It *assesses sync state by agent judgment* (`archive-change.ts:58-60`: "If none exist, proceed without sync prompt"), optionally invokes a separate agent-driven `openspec-sync-specs`, then moves the change with a raw `mkdir` + `mv` (`archive-change.ts:86-88`). Every deterministic check and the deterministic delta-merge in `src/core/archive.ts` is **never executed for agent workflows**. The `opsx-archive-skill` spec encodes this: it mandates the skill *move* the directory and *prompt-or-skip* sync on judgment. (Filed as #656 and #863.) + +3. **Even when reached, the archive CLI skips its own validation.** `openspec validate` runs `validateChangeDeltaSpecs` unconditionally; `openspec archive` runs it only inside `if (hasDeltaSpecs)` (`archive.ts:282`), gating the check on the presence of the very thing whose absence is the bug. This hides **three** drop modes: + - **Total drop** — no `specs/` at all (#1212, #1222). + - **Partial drop** — proposal declares capabilities `a,b,c`, ships a delta for `a` only. + - **Format drop (the Discord case)** — `specs//spec.md` exists but as a **full** spec (`## Purpose`/`## Requirements`) instead of a **delta** spec (`## ADDED/MODIFIED/REMOVED/RENAMED Requirements`); the delta-header regex misses it and archive reports "No delta specs." (#164, #194, Discord) + +4. **It must not over-correct.** Some schemas legitimately have no delta specs (proposal-only / lighter schemas). A naive "always require deltas" breaks them (#997). The requirement must be **schema-graph-driven**, not hardcoded. + +### Reproductions (confirmed against current `main`) + +```console +# Total drop — validate and archive disagree +$ openspec validate silent-drop # exit 1: CHANGE_NO_DELTAS +$ openspec archive silent-drop --yes # exit 0: "archived"; no openspec/specs/ created + +# Format drop — a full (non-delta) spec file is silently ignored: archive.ts:274's +# /^## (ADDED|MODIFIED|REMOVED|RENAMED) Requirements/ never matches, hasDeltaSpecs=false. +``` + +And the keystone: `grep "openspec archive" archive-change.ts` → **no match**. The skill never calls the CLI. + +## What Changes + +One deterministic definition of change completeness, enforced in the **CLI**, with the **skills reduced to thin wrappers that call the CLI** instead of judging. Ordered by leverage: + +1. **PRIMARY — fix the loop deterministically.** `spec-driven` `apply.requires: [specs, tasks]` (the fix #1212's author and #1250's reviewer both endorse). The apply gate is a **non-transitive** presence check — it checks only the artifacts named in `apply.requires`, not their transitive `requires` — which is exactly why a change with `tasks.md` but no `specs/` reads as *ready* today (only `tasks` is checked). Naming `specs` directly fixes it, and the apply skill's *existing* blocked-state handler (`apply-change.ts:52`) finally fires. `design` is deliberately left out of `apply.requires`, so "skip design for simple changes" keeps working — by design, not accident. The propose/ff skill is hardened narrowly: define loop termination as "every `apply.requires` artifact is `done`" so it can't stop at `tasks.md` while `specs` is absent. The schema change is the deterministic guarantee. (Incorporates #1250.) + +2. **KEYSTONE — the archive skill calls `openspec archive`.** Rework `opsx-archive-skill` to delegate archiving and spec sync to the CLI (`openspec archive --json`), which validates, merges deltas, and blocks deterministically. Remove the agent-judged "assess sync state / proceed without sync" and the raw `mv` — from **all** archive surfaces: both templates in `archive-change.ts` (the `openspec-archive-change` skill *and* the `/opsx:archive` command) and both in `bulk-archive-change.ts`; leaving any one on `mv` keeps the bypass alive. On a block, the skill surfaces the structured error and helps the user create/fix the delta spec — it never bypasses with `--skip-specs` *or* `--no-validate`, and it trusts the CLI's exit rather than self-certifying. This makes every CLI guarantee below actually reachable by agents. (#656, #863) + +3. **CLI gate — `validate` and `archive` share one schema-aware delta gate.** Replace archive's `if (hasDeltaSpecs)` gate with `!options.skipSpecs`, running delta validation + capability coverage, and apply the **same** gate to `openspec validate` (which today runs `validateChangeDeltaSpecs` unconditionally). This blocks all three drop modes (total via `CHANGE_NO_DELTAS`; partial via coverage; format via the existing "No delta sections found" error) in both commands. The requirement applies via one shared predicate `schemaProducesDeltaSpecs(schema)` — true only when the schema graph **produces delta specs** — so a proposal-only schema passes both `validate` and `archive`, while a spec-driven change with no specs fails both (#997). Without gating `validate` too, the two commands would disagree again — exactly the seam this change closes. + +4. **Partial-drop coverage (deterministic).** A new validator rule parses the proposal's `## Capabilities` and requires every declared capability to have a `specs//spec.md` *present*; delta well-formedness (including the format-drop case) is enforced by the existing delta validation. One-directional, schema-aware, surfaced in `validate`, CI, and `archive`. + +5. **Apply enforcement (earlier guardrail).** `openspec instructions apply` exits non-zero when blocked. (Incorporates #1250.) + +6. **Recovery — make existing drift detectable.** A deterministic audit (`openspec validate --changes` plus a check for archived changes whose declared capabilities never reached `openspec/specs/`) so teams who already accumulated silent drift (#1212's "no recovery path") can find it. Regenerating lost spec *content* from a finished implementation is inherently agent-assisted and is explicitly out of scope; detection + guidance is in scope. + +7. **Clarity (coherence, not enforcement).** Correct the delta-vs-full-spec confusion in the specs artifact instruction and archive skill so agents and users stop conflating "a `spec.md` exists / tasks are done" with "specs are synced." (#426, #194, #164, Discord) + +The deterministic CLI checks (3–5) are the guarantee; the skill/schema/docs changes (1, 2, 7) make the happy path work and remove agent judgment from the critical paths. The keystone (2) is what makes (3–5) reachable at all. + +## Capabilities + +### Modified Capabilities + +- `opsx-archive-skill`: The archive skill delegates archiving and spec sync to the `openspec archive` CLI (deterministic validation + delta merge) instead of judging sync state and moving files itself; on a validation block it guides the user to fix the delta spec rather than bypassing. +- `cli-archive`: Archive validation runs delta-spec validation and declared-capability coverage consistently with `openspec validate` unless `--skip-specs`, applies only to schemas whose graph includes a `specs` artifact, and blocks total, partial, and non-delta-format spec drops. +- `cli-validate`: Delta-spec validation (the at-least-one-delta `CHANGE_NO_DELTAS` rule) and declared-capability coverage become deterministic and schema-aware — they apply only when the schema produces delta specs, so a proposal-only schema with no specs passes `validate` while a spec-driven change with no specs still fails; every declared capability must have a delta spec present or validation fails with a precise per-capability error. +- `cli-artifact-workflow`: `openspec instructions apply` exits non-zero when the apply phase is blocked, and the `spec-driven` apply gate requires `specs`. + +## Impact + +- `src/core/templates/workflows/archive-change.ts` (**both** `getArchiveChangeSkillTemplate` and `getOpsxArchiveCommandTemplate`) + `src/core/templates/workflows/bulk-archive-change.ts` (both templates) + `openspec/specs/opsx-archive-skill` — every archive surface calls `openspec archive --json`; remove agent-side `mv` and judgment-based sync; handle `ArchiveBlockedError`; never use `--skip-specs`/`--no-validate`. +- `src/core/templates/workflows/{propose,ff-change}.ts` — loop termination = "every `apply.requires` artifact is `done`" (so it can't stop at `tasks.md` while `specs` is absent). +- `src/core/archive.ts` — replace `if (hasDeltaSpecs)` with `if (!options.skipSpecs && schemaProducesDeltaSpecs(schema))`; run delta + coverage validation; reuse the existing `ArchiveBlockedError`/exit path. +- `src/core/parsers/change-parser.ts` — deterministic `extractDeclaredCapabilities(proposalMarkdown)` (heading + bold-label forms; malformed-id warning). +- `src/core/validation/validator.ts` — `validateChangeCapabilityCoverage(changeDir)`; schema-aware; the archived-drift audit. +- `src/commands/validate.ts` — schema-aware delta gate + coverage in single + bulk, with `schemaProducesDeltaSpecs` **memoized per schema name** (not per change); audit surface. +- `src/commands/workflow/instructions.ts` — non-zero exit on blocked apply. +- `schemas/spec-driven/schema.yaml` — `apply.requires: [specs, tasks]`; tighten the **proposal** artifact instruction to mandate the `## Capabilities` heading shape; tighten the **specs** artifact instruction (delta-vs-full-spec). +- `src/core/templates/workflows/{apply-change,sync-specs}.ts` — delta-vs-full-spec clarity (template-only; `openspec-sync-specs`/`/opsx:sync` survives, intentionally still agent-driven for sync-without-archive). +- A **project-local proposal-only fixture schema** (no `specs` artifact) for the #997 tests, since the only built-in schema (`spec-driven`) produces delta specs. +- Tests for every mode below. + +## Issues addressed + +All references verified against `Fission-AI/OpenSpec` on 2026-06-29 (states current; #1250 confirmed **not merged**, so `main` still has `apply.requires: [tasks]`). + +Closes (the silent-drop family — total, partial, format, greenfield, the CLI-bypass, and the loop): + +- [#1212](https://github.com/Fission-AI/OpenSpec/issues/1212) — spec-driven fast-path silently produces stale specs (canonical) +- [#1260](https://github.com/Fission-AI/OpenSpec/issues/1260) — No specs generated after `/opsx:propose` +- [#1222](https://github.com/Fission-AI/OpenSpec/issues/1222) — Main spec never created for the first change in a greenfield project +- [#1264](https://github.com/Fission-AI/OpenSpec/issues/1264) — Archive should handle an empty main spec instead of skipping sync +- [#799](https://github.com/Fission-AI/OpenSpec/issues/799) — After `/opsx-archive` succeeds, sync is not executed +- [#656](https://github.com/Fission-AI/OpenSpec/issues/656) — Why archive relies on the LLM to merge specs vs. the CLI (answered by routing archive through `openspec archive`) +- [#863](https://github.com/Fission-AI/OpenSpec/issues/863) — `/opsx:archive` skill doesn't invoke the `openspec archive` CLI +- [#913](https://github.com/Fission-AI/OpenSpec/issues/913) — `/opsx-archive` offers "Sync now" needing the uninstalled `openspec-sync-specs` skill; eliminated because archive now syncs via the CLI, with no separate skill + +Supersedes (open PRs — partial or prompt-only fixes for the same failures): + +- [#1250](https://github.com/Fission-AI/OpenSpec/pull/1250) — apply exit-1 + `apply.requires:[specs,tasks]`; its reviewer explicitly deferred the archive guard to a follow-up — this is that follow-up. Incorporated here. +- [#1271](https://github.com/Fission-AI/OpenSpec/pull/1271), [#1241](https://github.com/Fission-AI/OpenSpec/pull/1241), [#1233](https://github.com/Fission-AI/OpenSpec/pull/1233) — prompt-only archive/greenfield guidance, replaced by an enforceable CLI check. (Prior attempt [#1268](https://github.com/Fission-AI/OpenSpec/pull/1268) was already closed unmerged — noted, not superseded.) + +## Related, addressed-in-part + +- [#997](https://github.com/Fission-AI/OpenSpec/issues/997) — **this change is the first to implement it**: the delta requirement is schema-graph-driven in **both** `validate` and `archive` (no forcing deltas on proposal-only schemas). +- [#977](https://github.com/Fission-AI/OpenSpec/pull/977) (allow-specless-changes) — **reconciled, with a deliberate boundary.** A change is allowed to have no specs when its schema produces no delta specs (the #997 gate) or when the user explicitly passes `--skip-specs`. This change does **not** allow a *silent* specless archive under a spec-driven schema — that is precisely the bug. So legitimate specless work is supported (schema choice or an explicit flag), but not by default under a spec-producing schema. Same `validate`/`validator` files, so also coordinate mechanically. +- [#902](https://github.com/Fission-AI/OpenSpec/pull/902) (sub-agent spec discovery in propose/ff) — edits the same propose/ff loop + schema; coordinate so the loop-termination fix and its spec-discovery step compose rather than overwrite. +- [#164](https://github.com/Fission-AI/OpenSpec/issues/164), [#426](https://github.com/Fission-AI/OpenSpec/issues/426), [#911](https://github.com/Fission-AI/OpenSpec/issues/911) — confusion about what a delta spec is / sync direction; addressed by the clarity changes (7). The conceptual docs may warrant a focused follow-up. (Prior art: [#194](https://github.com/Fission-AI/OpenSpec/issues/194), now closed.) + +## Out of scope (referenced, not closed) + +Distinct delta-**merge** bug family, packaging, and broader design requests, tracked separately: + +- [#1246](https://github.com/Fission-AI/OpenSpec/issues/1246) / [#1112](https://github.com/Fission-AI/OpenSpec/issues/1112) / [#1252](https://github.com/Fission-AI/OpenSpec/pull/1252) — two changes that MODIFY the same requirement drop scenarios (`buildUpdatedSpec` whole-block replace, `specs-apply.ts`). Same *family* (silent spec loss) but a **distinct root cause and distinct code** from this change, which never touches `buildUpdatedSpec`. **PR #1252 (APPROVED, mergeable) is complementary, not conflicting:** it should land first as the tactical fix for #1246; this change rebases cleanly on top (only a possible `test/core/archive.test.ts` overlap) as the durable deterministic layer. They reinforce each other — this change's keystone (archive routed through the CLI) is what makes #1252's `buildUpdatedSpec` fix actually reachable from the agent path, which today bypasses the CLI merge entirely. +- [#1120](https://github.com/Fission-AI/OpenSpec/issues/1120) — `openspec-sync-specs` skill not installed for Junie (broader packaging than the archive path #913 fixes). +- [#827](https://github.com/Fission-AI/OpenSpec/issues/827) — targeting/evolving a single main spec over time (design direction); [#1265](https://github.com/Fission-AI/OpenSpec/issues/1265) — change-naming and reading archived files. +- Regenerating lost spec content for changes already archived without specs (agent-assisted; the audit in (6) detects them). diff --git a/openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md b/openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md new file mode 100644 index 0000000000..4de879df0f --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md @@ -0,0 +1,81 @@ +## MODIFIED Requirements + +### Requirement: Archive Validation + +The archive command SHALL validate changes before applying them to ensure data integrity. When validation is enabled, the command SHALL run delta-spec validation and declared-capability coverage consistently with `openspec validate` — that is, whenever the user has not opted out of spec updates via `--skip-specs` — rather than only when delta specs already exist. A change that has no delta specs SHALL be blocked with the same `CHANGE_NO_DELTAS` error that `openspec validate` reports; a change whose spec file is present but not in delta format SHALL be blocked with the same "No delta sections found" error; and a change that declares capabilities it has not delivered as delta specs SHALL be blocked with the same coverage errors — instead of being silently archived with missing, malformed, or partial spec updates. This delta requirement SHALL apply only when the change's schema graph includes a `specs` artifact, so schemas that legitimately have no delta specs are not forced to provide them. + +#### Scenario: Pre-archive validation + +- **WHEN** executing `openspec archive change-name` +- **THEN** validate the change structure first +- **AND** only proceed if validation passes +- **AND** show validation errors if it fails + +#### Scenario: No delta specs blocks archive + +- **WHEN** executing `openspec archive change-name` without `--skip-specs` or `--no-validate` +- **AND** the change has no delta specs under `changes/change-name/specs/` (the directory is absent or contains no delta sections) +- **THEN** archive validation fails with the `CHANGE_NO_DELTAS` error +- **AND** the archive is aborted with a non-zero exit code +- **AND** the change is not moved to the archive directory + +#### Scenario: --yes does not bypass the validation guard + +- **WHEN** executing `openspec archive change-name --yes` without `--skip-specs` or `--no-validate` +- **AND** the change has no delta specs (under a schema that produces delta specs) +- **THEN** the archive is still blocked with `CHANGE_NO_DELTAS` and a non-zero exit code +- **AND** the change is not moved to the archive directory +- **AND** `--yes` only suppresses confirmation prompts, never the validation gate + +#### Scenario: Non-delta-format spec blocks archive + +- **WHEN** executing `openspec archive change-name` without `--skip-specs` or `--no-validate` +- **AND** `changes/change-name/specs//spec.md` exists but contains no delta sections (it is a full spec) +- **THEN** archive validation fails with the "No delta sections found" error for that file +- **AND** the archive is aborted with a non-zero exit code +- **AND** the change is not moved to the archive directory + +#### Scenario: Schema without a specs artifact is exempt + +- **WHEN** executing `openspec archive change-name` under a schema whose graph has no `specs` artifact +- **AND** the change has no delta specs +- **THEN** the delta requirement does not apply +- **AND** the archive proceeds (no `CHANGE_NO_DELTAS` block) + +#### Scenario: Declared but undelivered capability blocks archive + +- **WHEN** executing `openspec archive change-name` without `--skip-specs` or `--no-validate` +- **AND** the proposal declares a capability that has no matching delta spec under `changes/change-name/specs/` +- **THEN** archive validation fails with the declared-capability coverage error for that capability +- **AND** the archive is aborted with a non-zero exit code +- **AND** the change is not moved to the archive directory + +#### Scenario: Archive validation parity with validate command + +- **WHEN** a change would be reported invalid by `openspec validate change-name` +- **THEN** `openspec archive change-name` (with validation enabled) blocks on the same errors +- **AND** does not archive the change + +#### Scenario: Force archive without validation + +- **WHEN** executing `openspec archive change-name --no-validate` +- **THEN** skip validation (unsafe mode) +- **AND** show warning about skipping validation + +### Requirement: Skip Specs Option + +The archive command SHALL support a `--skip-specs` flag that skips all spec update operations and proceeds directly to archiving. Because `--skip-specs` declares that the change intentionally has no specs to sync, it SHALL also skip delta-spec validation, so a change with no delta specs can still be archived under this explicit opt-out. + +#### Scenario: Skipping spec updates with flag + +- **WHEN** executing `openspec archive --skip-specs` +- **THEN** skip spec discovery and update confirmation +- **AND** skip the delta-spec validation that would otherwise block a change with no delta specs +- **AND** proceed directly to moving the change to archive +- **AND** display a message indicating specs were skipped + +#### Scenario: Skip-specs allows a spec-less change to archive + +- **WHEN** executing `openspec archive --skip-specs` on a change with no delta specs +- **THEN** the `CHANGE_NO_DELTAS` block does not apply +- **AND** the change is archived diff --git a/openspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.md b/openspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.md new file mode 100644 index 0000000000..2bbd0d74f1 --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/specs/cli-artifact-workflow/spec.md @@ -0,0 +1,63 @@ +## MODIFIED Requirements + +### Requirement: Apply Instructions Command + +The system SHALL generate schema-aware apply instructions via `openspec instructions apply`. When the apply phase is blocked by missing required artifacts, the command SHALL exit with a non-zero status so that scripts, loops, and agents stop instead of proceeding past a printed warning. + +#### Scenario: Generate apply instructions + +- **WHEN** user runs `openspec instructions apply --change ` +- **AND** all required artifacts (per schema's `apply.requires`) exist +- **THEN** the system outputs: + - `contextFiles` mapping artifact IDs to arrays of concrete paths for all existing artifacts + - Schema-specific instruction text + - Progress tracking file path (if `apply.tracks` is set) +- **AND** exits with status code 0 + +#### Scenario: Apply blocked by missing artifacts + +- **WHEN** user runs `openspec instructions apply --change ` +- **AND** required artifacts are missing +- **THEN** the system indicates apply is blocked +- **AND** lists which artifacts must be created first +- **AND** exits with a non-zero status code + +#### Scenario: Apply blocked in JSON mode + +- **WHEN** user runs `openspec instructions apply --change --json` +- **AND** required artifacts are missing +- **THEN** the system prints the full JSON payload with `state` set to `blocked` +- **AND** exits with a non-zero status code + +#### Scenario: Apply instructions JSON output + +- **WHEN** user runs `openspec instructions apply --change --json` +- **THEN** the system outputs JSON with: + - `contextFiles`: object mapping artifact IDs to arrays of concrete paths for existing artifacts + - `instruction`: the apply instruction text + - `tracks`: path to progress file or null + - `applyRequires`: list of required artifact IDs + +### Requirement: Schema Apply Block + +The system SHALL support an `apply` block in schema definitions that controls when and how implementation begins. For the built-in `spec-driven` schema, the apply gate SHALL include the `specs` artifact so that a change with no delta specs is reported as blocked rather than ready. + +#### Scenario: Schema with apply block + +- **WHEN** a schema defines an `apply` block +- **THEN** the system uses `apply.requires` to determine which artifacts must exist before apply +- **AND** uses `apply.tracks` to identify the file for progress tracking (or null if none) +- **AND** uses `apply.instruction` for guidance shown to the agent + +#### Scenario: Schema without apply block + +- **WHEN** a schema has no `apply` block +- **THEN** the system requires all artifacts to exist before apply is available +- **AND** uses default instruction: "All artifacts complete. Proceed with implementation." + +#### Scenario: Spec-driven requires specs before apply + +- **WHEN** a `spec-driven` change has `tasks.md` but no delta specs under `specs/` +- **THEN** `openspec instructions apply` reports the apply phase as blocked +- **AND** lists `specs` among the missing required artifacts +- **AND** exits with a non-zero status code diff --git a/openspec/changes/prevent-silent-spec-drop/specs/cli-validate/spec.md b/openspec/changes/prevent-silent-spec-drop/specs/cli-validate/spec.md new file mode 100644 index 0000000000..0175d0c45e --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/specs/cli-validate/spec.md @@ -0,0 +1,143 @@ +## ADDED Requirements + +### Requirement: Schema-Aware Delta Validation + +`openspec validate` SHALL gate delta-spec validation (including the at-least-one-delta `CHANGE_NO_DELTAS` rule) on whether the change's schema produces delta specs, so that `validate` and `archive` apply the same rule and proposal-only schemas are not forced to have delta specs. A schema produces delta specs when any of its artifacts' `generates` glob writes under `specs/`. This SHALL apply to both single-change and bulk (`--changes` / `--all`) validation. + +#### Scenario: Spec-driven change with no delta specs fails + +- **WHEN** validating a change whose schema produces delta specs (e.g. `spec-driven`) +- **AND** the change has no delta specs under `specs/` +- **THEN** validation fails with the `CHANGE_NO_DELTAS` error and a non-zero exit code + +#### Scenario: Proposal-only schema with no delta specs passes + +- **WHEN** validating a change whose schema does NOT produce delta specs (no artifact generates under `specs/`) +- **AND** the change has no delta specs +- **THEN** the `CHANGE_NO_DELTAS` rule does not apply +- **AND** the change is not marked invalid on account of missing delta specs + +#### Scenario: Validate and archive agree on the delta requirement + +- **WHEN** the same change is checked by `openspec validate` and (with validation enabled) by `openspec archive` +- **THEN** both apply the identical schema-aware delta gate and reach the same verdict on whether delta specs are required + +#### Scenario: Indeterminate schema defaults to requiring deltas + +- **WHEN** resolving the change's schema throws (e.g. a malformed project schema), as distinct from absent metadata which resolves to the default schema +- **THEN** delta-spec validation is applied (fail-safe to the current strict behavior), not skipped + +#### Scenario: Schema resolution is memoized within a run + +- **WHEN** bulk validation (`--changes` / `--all`) checks many changes +- **THEN** `schemaProducesDeltaSpecs` is resolved at most once per distinct schema name, not once per change + +### Requirement: Declared Capability Coverage + +Validating a change SHALL deterministically check that every capability declared in the proposal's `## Capabilities` section has a corresponding delta spec present, so a change cannot pass validation while silently omitting specs it promised. The check SHALL be a pure function of the proposal text and the change's `specs/` directory listing: the same inputs always produce the same result. The check SHALL apply only when the change's schema graph includes a `specs` artifact, so schemas that legitimately have no delta specs are never required to declare them. + +A declared capability is resolved as follows: + +- The `### New Capabilities` and `### Modified Capabilities` subsections under `## Capabilities` are considered. Both heading form (`### New Capabilities`) and bold-label form (`**New Capabilities**:`) SHALL be accepted, so a proposal written to the documented template is never under-extracted. +- A capability id is the first inline-code span (`` `id` ``) of a top-level `- ` bullet within those subsections. +- A capability id that is not kebab-case (`^[a-z0-9]+(?:-[a-z0-9]+)*$`) SHALL be reported as a malformed-capability warning rather than silently ignored, so a mis-cased declaration cannot make coverage pass vacuously. Inline code that is not the first span of a capability bullet (paths, prose) is not treated as a capability. +- `None`, `_None_`, HTML-comment, and empty subsections declare nothing. + +A declared capability `id` is covered when `specs//spec.md` is present. For a Modified capability, the declared id MUST be the existing spec folder name (as the schema instruction directs), which is the same key `findSpecUpdates` uses — so coverage and sync stay aligned. Coverage checks presence only; whether a present spec file is a well-formed delta (uses `## ADDED/MODIFIED/REMOVED/RENAMED Requirements` headers) is enforced by the existing delta-spec validation, so a present-but-non-delta file produces one precise "No delta sections found" error rather than a duplicate. Coverage is one-directional: delta specs for capabilities not declared in the proposal SHALL NOT be flagged. + +#### Scenario: Declared capability has no spec file + +- **WHEN** validating a change (under a schema with a `specs` artifact) whose proposal declares capability `foo` +- **AND** no `specs/foo/spec.md` exists +- **THEN** validation reports an ERROR identifying `foo` as a declared capability missing its delta spec at `specs/foo/spec.md` +- **AND** the error guidance explains that the `## Capabilities` id must match the delta spec folder name (for a Modified capability, the existing `openspec/specs//` folder) +- **AND** the change is invalid (non-zero exit) + +#### Scenario: Bold-label capabilities form is recognized + +- **WHEN** a proposal declares capabilities using bold labels (`**New Capabilities**:` / `**Modified Capabilities**:`) rather than `###` headings +- **THEN** the declared capabilities are extracted the same way as the heading form + +#### Scenario: Mis-cased capability id is reported, not dropped + +- **WHEN** a declared capability bullet's id is not kebab-case (e.g. `User-Auth`) +- **THEN** validation reports a malformed-capability warning naming the bullet +- **AND** does not silently omit it from coverage + +#### Scenario: Capability deletion or rename uses explicit opt-out + +- **WHEN** a change's purpose is to delete or rename a whole capability/spec folder, for which there is no natural delta spec +- **THEN** the user explicitly passes `--skip-specs` (the skill never does so on its own) and coverage does not block +- **AND** removing a capability's requirements (rather than the whole capability) is expressed as a `## REMOVED Requirements` delta under the existing folder, which satisfies coverage normally + +#### Scenario: Declared capability present but not in delta format + +- **WHEN** a proposal declares capability `foo` +- **AND** `specs/foo/spec.md` exists but contains no delta sections (it is a full spec) +- **THEN** the delta-spec validation reports a "No delta sections found" ERROR for `specs/foo/spec.md` +- **AND** the coverage check does not additionally report `foo` as missing + +#### Scenario: One ERROR per missing capability + +- **WHEN** a proposal declares capabilities `foo`, `bar`, and `baz` +- **AND** only `specs/foo/spec.md` exists +- **THEN** validation reports a distinct ERROR for `bar` and for `baz` + +#### Scenario: All declared capabilities covered + +- **WHEN** every capability declared in the proposal has a matching `specs//spec.md` +- **THEN** the coverage check contributes no errors + +#### Scenario: Schema without a specs artifact is exempt + +- **WHEN** validating a change whose schema graph has no `specs` artifact +- **THEN** the coverage check is not applied and contributes no errors + +#### Scenario: Proposal without a Capabilities section + +- **WHEN** validating a change whose proposal has no `## Capabilities` section +- **THEN** the coverage check declares nothing and contributes no errors +- **AND** other validation (such as the at-least-one-delta rule) is unaffected + +#### Scenario: Empty or None subsection declares nothing + +- **WHEN** a `### Modified Capabilities` subsection contains only `None`, `_None_`, an HTML comment, or no bullets +- **THEN** no capability is declared from that subsection + +#### Scenario: Capability id is the first inline-code token + +- **WHEN** a declared capability bullet is `` - `tool-command-surface`: classifies tools as `adapter` or `none` `` +- **THEN** the declared id is `tool-command-surface` +- **AND** the inline code in the description (`adapter`, `none`) is ignored + +#### Scenario: Undeclared delivered spec is not flagged + +- **WHEN** a change ships `specs/extra/spec.md` for a capability not listed in the proposal +- **THEN** the coverage check does not report an error for `extra` + +#### Scenario: Coverage applies in single and bulk validation + +- **WHEN** running `openspec validate ` or `openspec validate --changes` (including the interactive single-item path, which shares the same validation routine) +- **THEN** the declared-capability coverage check runs for each validated change + +### Requirement: Archived Spec-Drift Audit + +`openspec validate` SHALL provide a deterministic audit that detects already-archived changes whose declared capabilities never reached `openspec/specs/`, so teams who accumulated silent drift before this change can find it. The audit is detection only; it does not regenerate lost spec content. + +#### Scenario: Archived change with un-synced capability is flagged + +- **WHEN** the audit runs over `openspec/changes/archive/` +- **AND** an archived change's proposal declares a capability with no corresponding `openspec/specs//spec.md` +- **THEN** the audit reports that archived change and the missing capability as drift + +#### Scenario: Audit is forward-only and says so + +- **WHEN** an archived change's proposal predates the `## Capabilities` contract (its capabilities cannot be extracted) +- **THEN** the audit does not report false drift for it +- **AND** the audit output notes that pre-contract archives are not auditable, rather than implying completeness + +#### Scenario: Audit does not regenerate content + +- **WHEN** drift is detected +- **THEN** the audit points the user at what to rebuild +- **AND** does not write or fabricate spec content diff --git a/openspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.md b/openspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.md new file mode 100644 index 0000000000..26a4dffae6 --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/specs/opsx-archive-skill/spec.md @@ -0,0 +1,93 @@ +## MODIFIED Requirements + +### Requirement: OPSX Archive Skill + +The system SHALL provide an `/opsx:archive` skill that archives completed changes in the experimental workflow by invoking the `openspec archive` CLI, rather than moving files or merging specs itself. + +#### Scenario: Archive a change with all artifacts complete + +- **WHEN** agent executes `/opsx:archive` with a change name +- **AND** all artifacts in the schema are complete +- **AND** all tasks are complete +- **THEN** the agent invokes `openspec archive` for the change +- **AND** the CLI validates, syncs delta specs, and moves the change to `openspec/changes/archive/YYYY-MM-DD-/` +- **AND** displays a success message with the archived location + +#### Scenario: Change selection prompt + +- **WHEN** agent executes `/opsx:archive` without specifying a change +- **THEN** the agent prompts user to select from available changes +- **AND** shows only active changes (excludes archive/) + +### Requirement: Skill Output + +The skill SHALL provide clear feedback about the archive operation, derived from the `openspec archive` result rather than a separate agent-driven sync. + +#### Scenario: Archive complete with sync + +- **WHEN** archive completes after syncing specs +- **THEN** display summary: + - Specs synced (from the `openspec archive` result — counts/`specsUpdated`) + - Change archived to location + - Schema that was used + +#### Scenario: Archive complete without sync + +- **WHEN** archive completes without syncing specs (e.g. `--skip-specs`, or a schema that produces no delta specs) +- **THEN** display summary: + - Note that specs were not synced (if applicable) + - Change archived to location + - Schema that was used + +#### Scenario: Archive complete with warnings + +- **WHEN** archive completes with incomplete artifacts or tasks +- **THEN** include note about what was incomplete +- **AND** suggest reviewing if archive was intentional + +### Requirement: Spec Sync Prompt + +The skill SHALL delegate spec sync to the `openspec archive` CLI rather than deciding by agent judgment whether delta specs exist or need syncing. This applies to every `/opsx:archive` surface — the archive skill, the `/opsx:archive` command, and the bulk-archive flow — none of which may move the change or merge specs themselves. The skill SHALL NOT inspect the change to decide sync state, SHALL NOT self-certify that "specs look synced," and SHALL trust the CLI's exit status. The CLI deterministically validates the change, syncs delta specs into the main specs, and blocks when required specs are missing, partial, or not in delta format. + +#### Scenario: Archive delegates sync to the CLI + +- **WHEN** the skill archives a change +- **THEN** it invokes `openspec archive --json` (using `--yes` only to suppress interactive prompts) +- **AND** it does not separately assess whether delta specs exist +- **AND** the CLI performs spec validation and delta-spec sync + +#### Scenario: CLI blocks on missing, partial, or non-delta specs + +- **WHEN** `openspec archive` returns a blocked diagnostic — because no delta spec exists, a capability declared in the proposal has no delta spec (partial), or a `specs//spec.md` exists but contains no delta sections (not in delta format) +- **THEN** the skill surfaces the diagnostic and guides the user to create or fix the delta spec +- **AND** it re-runs `openspec archive` after the fix and trusts its exit status +- **AND** it does NOT bypass the block with `--skip-specs` or `--no-validate` + +#### Scenario: Spec-less change under an explicit opt-out + +- **WHEN** a change legitimately has no specs and the user explicitly intends to skip them +- **THEN** the user (not the skill on its own judgment) may pass `--skip-specs` +- **AND** the CLI archives without sync + +### Requirement: Archive Process + +The skill SHALL archive the change by invoking the `openspec archive` CLI, which moves the change to the archive folder with a date prefix after validation and sync succeed. The skill SHALL NOT move the change directory itself. + +#### Scenario: Successful archive + +- **WHEN** archiving a change +- **THEN** the skill runs `openspec archive --json` for the change +- **AND** the CLI validates, syncs delta specs, and moves the change to `archive/-/` +- **AND** `.openspec.yaml` is preserved in the archived change + +#### Scenario: Archive already exists + +- **WHEN** the target archive directory already exists +- **THEN** the CLI fails with an error and the skill surfaces it +- **AND** suggests renaming the existing archive or using a different date + +#### Scenario: Validation or sync blocks the archive + +- **WHEN** the CLI reports a validation or sync block +- **THEN** the skill does not move any files +- **AND** reports the diagnostic and remediation instead of archiving diff --git a/openspec/changes/prevent-silent-spec-drop/tasks.md b/openspec/changes/prevent-silent-spec-drop/tasks.md new file mode 100644 index 0000000000..61186d48ae --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/tasks.md @@ -0,0 +1,66 @@ +## 1. Deterministic CLI gate (the guarantee — do first) + +### 1a. Capability extraction +- [ ] 1.1 Add `extractDeclaredCapabilities(proposalMarkdown)` to `src/core/parsers/change-parser.ts` per the contract: scope to `## Capabilities` → `### New/Modified Capabilities` **and** the bold-label form (`**New Capabilities**:`); id = first inline-code span of a top-level `- ` bullet; ignore `None`/`_None_`/HTML-comment/empty; return non-kebab ids as a malformed-capability warning (do NOT silently drop). Pure function, no FS access +- [ ] 1.2 Unit tests: heading form + bold-label form, first-inline-code rule with backticks in description, paths/globs ignored, None/comment/empty, missing section, non-kebab id → warning (not dropped), CRLF + +### 1b. Schema-aware validation +- [ ] 1.3 Add a helper `schemaProducesDeltaSpecs(schema)` — true iff any artifact's `generates` glob writes under `specs/` (drive off the resolved artifact graph, not a hardcoded schema name); archive resolves the schema via `.openspec.yaml`/`loadChangeContext` +- [ ] 1.4 Add `validateChangeCapabilityCoverage(changeDir)` to `src/core/validation/validator.ts`: only when the schema produces delta specs; one ERROR per declared capability with no `specs//spec.md` present, message names the capability + expected path **and explains the id-must-equal-spec-folder-name rule** (for Modified capabilities, the existing `openspec/specs//` folder); presence-only (let delta validation handle non-delta format); one-directional; fail-open on unparseable proposal +- [ ] 1.5 In `src/commands/validate.ts` single (`:186`) and bulk (`:252`): resolve the change schema and run BOTH `validateChangeDeltaSpecs` (the existing `CHANGE_NO_DELTAS` rule, currently unconditional) and the new coverage **only when `schemaProducesDeltaSpecs(schema)` is true**, so a proposal-only schema no longer fails `validate` for missing deltas (#997). **Memoize `schemaProducesDeltaSpecs` per schema name within the run** (not per change — the bulk loop must not re-`readdir` schemas per change). Preserve exit-code/JSON semantics; "indeterminate" = resolution throws (fail-safe to requiring deltas), distinct from absent metadata which defaults to `spec-driven` + +### 1c. Archive parity (same shared gate) +- [ ] 1.6 In `src/core/archive.ts`, replace the `if (hasDeltaSpecs)` gate (~lines 263-297) with `if (!options.skipSpecs && schemaProducesDeltaSpecs(schema))`, using the SAME helper as validate; run `validateChangeDeltaSpecs` + coverage. Confirm `--yes` does not bypass this gate (only confirmation prompts) +- [ ] 1.7 Route failures through the existing `hasValidationErrors → ArchiveBlockedError` (JSON) / `return null` + exit-1 (text) path; remove the dead `hasDeltaSpecs` probe +- [ ] 1.8 Confirm the format-drop case (present non-delta spec) and total/partial drops all block; confirm a proposal-only schema (#997) still archives + +### 1d. Apply enforcement + schema gate +- [ ] 1.9 In `src/commands/workflow/instructions.ts`, set `process.exitCode = 1` when apply `state === 'blocked'` (text and JSON; payload first) +- [ ] 1.10 Set `apply.requires: [specs, tasks]` in `schemas/spec-driven/schema.yaml`; update tests asserting the old `[tasks]` gate + +- [ ] 1.11 Structure `extractDeclaredCapabilities` so a future structured source (`provides` markers from `add-change-stacking-awareness`) can augment/replace prose parsing without changing the coverage rule — keep extraction behind a single function + +## 2. Fix the loop at the source + +- [ ] 2.1 Harden `src/core/templates/workflows/propose.ts` and `ff-change.ts`: define loop termination as "every `apply.requires` artifact has status `done`" (read live from `openspec status`), so the loop cannot stop at `tasks.md` while `specs` is absent. The apply gate is non-transitive — the schema change (`apply.requires: [specs, tasks]`) is the deterministic lever; do NOT add `design` (it stays optional, by design) +- [ ] 2.2 Verify the apply skill's existing `state: blocked` handler (`apply-change.ts:52`) now fires for missing specs via the schema gate; confirm a `design`-less change still passes apply (conditional-design preserved); adjust wording if needed + +## 3. Keystone — archive skill calls the CLI (test heavily) + +- [ ] 3.1 Rework ALL archive templates — both `getArchiveChangeSkillTemplate` and `getOpsxArchiveCommandTemplate` in `archive-change.ts`, and both templates in `bulk-archive-change.ts` — replacing the agent-judged "assess sync state" step and the raw `mkdir`+`mv` with a single `openspec archive --json` invocation (`--yes` to suppress prompts; never `--skip-specs` or `--no-validate`) +- [ ] 3.2 Handle the structured result: success summary (specs synced, totals, path) from the CLI output; on `ArchiveBlockedError`, surface the diagnostic and guide the user to create/fix the delta spec, then re-run and trust the exit; never self-certify +- [ ] 3.3 Ensure `--skip-specs`/`--no-validate` are only ever used on explicit user intent, never to bypass a block +- [ ] 3.4 Add a template-parity assertion for EACH of the four templates that it invokes `openspec archive` and contains no raw `mv` + +## 4. Recovery audit (detection, not regeneration) + +- [ ] 4.1 Add a deterministic audit (the `cli-validate` "Archived Spec-Drift Audit" requirement) that flags archived changes whose declared capabilities have no corresponding `openspec/specs//spec.md` (surface via `openspec validate`, e.g. an `--archived`/audit mode). Forward-only: parse archived proposals with the same contract; note that pre-contract archives are not auditable rather than implying completeness +- [ ] 4.2 Document that reconstructing lost spec content is agent-assisted and out of scope; the audit points at what to rebuild (fold the note into the audit help text / changeset) + +## 5. Delta-vs-full-spec clarity (coherence) + +- [ ] 5.1 Tighten the `specs` artifact instruction (schema) and `apply-change.ts`/`sync-specs.ts` wording: a delta spec lives at `changes//specs//spec.md` and uses `## ADDED/MODIFIED/REMOVED/RENAMED Requirements`, never `## Purpose`/`## Requirements` +- [ ] 5.2 Remove any skill language implying the agent decides "no sync needed"; the CLI decides +- [ ] 5.3 Tighten the **proposal** artifact instruction (schema) so `## Capabilities` uses `### New Capabilities` / `### Modified Capabilities` headings with kebab-case ids matching the spec folder name (the shape the coverage parser enforces) + +## 6. Tests + +- [ ] 6.1 Archive total drop: no `specs/` → `CHANGE_NO_DELTAS`, non-zero, nothing moved +- [ ] 6.2 Archive format drop: present non-delta `spec.md` → "No delta sections found", blocked +- [ ] 6.3 Archive partial drop: declares `a`,`b`; only `specs/a` → coverage error for `b`, blocked +- [ ] 6.4 Schema-aware: a **project-local proposal-only fixture schema** (no `specs` artifact — `spec-driven` can't test this) with no specs → archives (no block) (#997) +- [ ] 6.5 Covered + valid → archives and syncs (no regression) +- [ ] 6.6 `--skip-specs` bypasses; `--no-validate` (+ `--yes`) bypasses (human escape hatches) +- [ ] 6.7 `--yes` alone (no `--skip-specs`/`--no-validate`) on a no-delta spec-driven change → still blocked with `CHANGE_NO_DELTAS`, non-zero, nothing moved +- [ ] 6.8 Parity: a change `openspec validate` rejects (total/partial/format) is blocked by `openspec archive` +- [ ] 6.9 Validate schema-aware: `openspec validate` on the proposal-only fixture with no specs → passes (no `CHANGE_NO_DELTAS`); on a spec-driven change with no specs → fails with `CHANGE_NO_DELTAS`; both single and bulk +- [ ] 6.10 Unparseable/malformed `## Capabilities` → coverage fails open (no crash, no false missing); non-kebab id → warning (not dropped); bold-label form → extracted; resolution-throws → delta validation still applied (fail-safe) +- [ ] 6.11 All four archive templates call `openspec archive` and contain no raw `mv` (parity assertion per template) +- [ ] 6.12 Apply: blocked apply exits non-zero (text + JSON); spec-driven `tasks`-only reports `specs` missing; a `design`-less change still passes apply +- [ ] 6.13 Capability deletion/rename: change with only `## REMOVED Requirements` under the existing folder → coverage passes; whole-capability deletion → requires explicit `--skip-specs` +- [ ] 6.14 Archived-drift audit: archived change with an un-synced declared capability → flagged; pre-contract archive → not falsely flagged +- [ ] 6.15 Extraction determinism fixtures; cross-platform `path.join()` + +## 7. Release + +- [ ] 7.1 Changeset describing the behavior change (archive now blocks total/partial/format spec drops for schemas with a `specs` artifact; archive skill uses the CLI; `--skip-specs` for intentional spec-less changes) From 8ebe4967b0ecd5be546425ecf4132551d363cdb9 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 13:48:24 -0500 Subject: [PATCH 02/10] =?UTF-8?q?feat(validate,archive):=20schema-aware=20?= =?UTF-8?q?delta=20gate=20+=20capability=20coverage=20(silent-spec-drop=20?= =?UTF-8?q?=C2=A71)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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//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 (#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) --- .../changes/prevent-silent-spec-drop/tasks.md | 22 +-- schemas/spec-driven/schema.yaml | 2 +- src/commands/validate.ts | 35 ++++- src/commands/workflow/instructions.ts | 16 ++- src/core/archive.ts | 51 +++---- src/core/artifact-graph/graph.ts | 13 ++ src/core/artifact-graph/index.ts | 2 +- src/core/parsers/declared-capabilities.ts | 133 ++++++++++++++++++ src/core/validation/delta-requirement.ts | 36 +++++ src/core/validation/validator.ts | 58 ++++++++ test/commands/artifact-workflow.test.ts | 10 +- test/core/archive.test.ts | 29 ++-- .../parsers/declared-capabilities.test.ts | 70 +++++++++ .../tmp-init/openspec/changes/c1/tasks.md | 3 + 14 files changed, 412 insertions(+), 68 deletions(-) create mode 100644 src/core/parsers/declared-capabilities.ts create mode 100644 src/core/validation/delta-requirement.ts create mode 100644 test/core/parsers/declared-capabilities.test.ts create mode 100644 test/fixtures/tmp-init/openspec/changes/c1/tasks.md diff --git a/openspec/changes/prevent-silent-spec-drop/tasks.md b/openspec/changes/prevent-silent-spec-drop/tasks.md index 61186d48ae..42d75bc51e 100644 --- a/openspec/changes/prevent-silent-spec-drop/tasks.md +++ b/openspec/changes/prevent-silent-spec-drop/tasks.md @@ -1,24 +1,24 @@ ## 1. Deterministic CLI gate (the guarantee — do first) ### 1a. Capability extraction -- [ ] 1.1 Add `extractDeclaredCapabilities(proposalMarkdown)` to `src/core/parsers/change-parser.ts` per the contract: scope to `## Capabilities` → `### New/Modified Capabilities` **and** the bold-label form (`**New Capabilities**:`); id = first inline-code span of a top-level `- ` bullet; ignore `None`/`_None_`/HTML-comment/empty; return non-kebab ids as a malformed-capability warning (do NOT silently drop). Pure function, no FS access -- [ ] 1.2 Unit tests: heading form + bold-label form, first-inline-code rule with backticks in description, paths/globs ignored, None/comment/empty, missing section, non-kebab id → warning (not dropped), CRLF +- [x] 1.1 Add `extractDeclaredCapabilities(proposalMarkdown)` to `src/core/parsers/change-parser.ts` per the contract: scope to `## Capabilities` → `### New/Modified Capabilities` **and** the bold-label form (`**New Capabilities**:`); id = first inline-code span of a top-level `- ` bullet; ignore `None`/`_None_`/HTML-comment/empty; return non-kebab ids as a malformed-capability warning (do NOT silently drop). Pure function, no FS access +- [x] 1.2 Unit tests: heading form + bold-label form, first-inline-code rule with backticks in description, paths/globs ignored, None/comment/empty, missing section, non-kebab id → warning (not dropped), CRLF ### 1b. Schema-aware validation -- [ ] 1.3 Add a helper `schemaProducesDeltaSpecs(schema)` — true iff any artifact's `generates` glob writes under `specs/` (drive off the resolved artifact graph, not a hardcoded schema name); archive resolves the schema via `.openspec.yaml`/`loadChangeContext` -- [ ] 1.4 Add `validateChangeCapabilityCoverage(changeDir)` to `src/core/validation/validator.ts`: only when the schema produces delta specs; one ERROR per declared capability with no `specs//spec.md` present, message names the capability + expected path **and explains the id-must-equal-spec-folder-name rule** (for Modified capabilities, the existing `openspec/specs//` folder); presence-only (let delta validation handle non-delta format); one-directional; fail-open on unparseable proposal -- [ ] 1.5 In `src/commands/validate.ts` single (`:186`) and bulk (`:252`): resolve the change schema and run BOTH `validateChangeDeltaSpecs` (the existing `CHANGE_NO_DELTAS` rule, currently unconditional) and the new coverage **only when `schemaProducesDeltaSpecs(schema)` is true**, so a proposal-only schema no longer fails `validate` for missing deltas (#997). **Memoize `schemaProducesDeltaSpecs` per schema name within the run** (not per change — the bulk loop must not re-`readdir` schemas per change). Preserve exit-code/JSON semantics; "indeterminate" = resolution throws (fail-safe to requiring deltas), distinct from absent metadata which defaults to `spec-driven` +- [x] 1.3 Add a helper `schemaProducesDeltaSpecs(schema)` — true iff any artifact's `generates` glob writes under `specs/` (drive off the resolved artifact graph, not a hardcoded schema name); archive resolves the schema via `.openspec.yaml`/`loadChangeContext` +- [x] 1.4 Add `validateChangeCapabilityCoverage(changeDir)` to `src/core/validation/validator.ts`: only when the schema produces delta specs; one ERROR per declared capability with no `specs//spec.md` present, message names the capability + expected path **and explains the id-must-equal-spec-folder-name rule** (for Modified capabilities, the existing `openspec/specs//` folder); presence-only (let delta validation handle non-delta format); one-directional; fail-open on unparseable proposal +- [x] 1.5 In `src/commands/validate.ts` single (`:186`) and bulk (`:252`): resolve the change schema and run BOTH `validateChangeDeltaSpecs` (the existing `CHANGE_NO_DELTAS` rule, currently unconditional) and the new coverage **only when `schemaProducesDeltaSpecs(schema)` is true**, so a proposal-only schema no longer fails `validate` for missing deltas (#997). **Memoize `schemaProducesDeltaSpecs` per schema name within the run** (not per change — the bulk loop must not re-`readdir` schemas per change). Preserve exit-code/JSON semantics; "indeterminate" = resolution throws (fail-safe to requiring deltas), distinct from absent metadata which defaults to `spec-driven` ### 1c. Archive parity (same shared gate) -- [ ] 1.6 In `src/core/archive.ts`, replace the `if (hasDeltaSpecs)` gate (~lines 263-297) with `if (!options.skipSpecs && schemaProducesDeltaSpecs(schema))`, using the SAME helper as validate; run `validateChangeDeltaSpecs` + coverage. Confirm `--yes` does not bypass this gate (only confirmation prompts) -- [ ] 1.7 Route failures through the existing `hasValidationErrors → ArchiveBlockedError` (JSON) / `return null` + exit-1 (text) path; remove the dead `hasDeltaSpecs` probe -- [ ] 1.8 Confirm the format-drop case (present non-delta spec) and total/partial drops all block; confirm a proposal-only schema (#997) still archives +- [x] 1.6 In `src/core/archive.ts`, replace the `if (hasDeltaSpecs)` gate (~lines 263-297) with `if (!options.skipSpecs && schemaProducesDeltaSpecs(schema))`, using the SAME helper as validate; run `validateChangeDeltaSpecs` + coverage. Confirm `--yes` does not bypass this gate (only confirmation prompts) +- [x] 1.7 Route failures through the existing `hasValidationErrors → ArchiveBlockedError` (JSON) / `return null` + exit-1 (text) path; remove the dead `hasDeltaSpecs` probe +- [x] 1.8 Confirm the format-drop case (present non-delta spec) and total/partial drops all block; confirm a proposal-only schema (#997) still archives ### 1d. Apply enforcement + schema gate -- [ ] 1.9 In `src/commands/workflow/instructions.ts`, set `process.exitCode = 1` when apply `state === 'blocked'` (text and JSON; payload first) -- [ ] 1.10 Set `apply.requires: [specs, tasks]` in `schemas/spec-driven/schema.yaml`; update tests asserting the old `[tasks]` gate +- [x] 1.9 In `src/commands/workflow/instructions.ts`, set `process.exitCode = 1` when apply `state === 'blocked'` (text and JSON; payload first) +- [x] 1.10 Set `apply.requires: [specs, tasks]` in `schemas/spec-driven/schema.yaml`; update tests asserting the old `[tasks]` gate -- [ ] 1.11 Structure `extractDeclaredCapabilities` so a future structured source (`provides` markers from `add-change-stacking-awareness`) can augment/replace prose parsing without changing the coverage rule — keep extraction behind a single function +- [x] 1.11 Structure `extractDeclaredCapabilities` so a future structured source (`provides` markers from `add-change-stacking-awareness`) can augment/replace prose parsing without changing the coverage rule — keep extraction behind a single function ## 2. Fix the loop at the source diff --git a/schemas/spec-driven/schema.yaml b/schemas/spec-driven/schema.yaml index 45f61e222b..ebf17b41ed 100644 --- a/schemas/spec-driven/schema.yaml +++ b/schemas/spec-driven/schema.yaml @@ -146,7 +146,7 @@ artifacts: - design apply: - requires: [tasks] + requires: [specs, tasks] tracks: tasks.md instruction: | Read context files, work through pending tasks, mark complete as you go. diff --git a/src/commands/validate.ts b/src/commands/validate.ts index 4690f6f6b5..0d5fd44b06 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -1,6 +1,8 @@ import ora from 'ora'; import path from 'path'; import { Validator } from '../core/validation/validator.js'; +import { makeDeltaRequirementResolver } from '../core/validation/delta-requirement.js'; +import type { ValidationReport } from '../core/validation/types.js'; import { resolveRootForCommand, toRootOutput, @@ -178,12 +180,40 @@ export class ValidateCommand { await this.validateByType(root, type, itemName, opts); } + /** + * Validate a change with the schema-aware delta gate: only run delta-spec + * validation + declared-capability coverage when the change's schema produces + * delta specs (#997). The same gate is applied by `openspec archive`, so the + * two commands agree on whether delta specs are required. + */ + private async validateChange( + validator: Validator, + requiresDelta: (changeDir: string) => boolean, + changeDir: string, + strict: boolean + ): Promise { + if (!requiresDelta(changeDir)) { + return { valid: true, issues: [], summary: { errors: 0, warnings: 0, info: 0 } }; + } + const [delta, coverage] = await Promise.all([ + validator.validateChangeDeltaSpecs(changeDir), + validator.validateChangeCapabilityCoverage(changeDir), + ]); + const issues = [...delta.issues, ...coverage.issues]; + const errors = issues.filter((i) => i.level === 'ERROR').length; + const warnings = issues.filter((i) => i.level === 'WARNING').length; + const info = issues.filter((i) => i.level === 'INFO').length; + const valid = strict ? errors === 0 && warnings === 0 : errors === 0; + return { valid, issues, summary: { errors, warnings, info } }; + } + private async validateByType(root: ResolvedOpenSpecRoot, type: ItemType, id: string, opts: { strict: boolean; json: boolean }): Promise { const validator = new Validator(opts.strict); if (type === 'change') { const changeDir = path.join(root.changesDir, id); + const requiresDelta = makeDeltaRequirementResolver(root.path); const start = Date.now(); - const report = await validator.validateChangeDeltaSpecs(changeDir); + const report = await this.validateChange(validator, requiresDelta, changeDir, opts.strict); const durationMs = Date.now() - start; this.printReport('change', id, report, durationMs, opts.json, root); // Non-zero exit if invalid (keeps enriched output test semantics) @@ -243,13 +273,14 @@ export class ValidateCommand { const maxSuggestions = 5; // used by nearestMatches const concurrency = normalizeConcurrency(opts.concurrency) ?? normalizeConcurrency(process.env.OPENSPEC_CONCURRENCY) ?? DEFAULT_CONCURRENCY; const validator = new Validator(opts.strict); + const requiresDelta = makeDeltaRequirementResolver(root.path); const queue: Array<() => Promise> = []; for (const id of changeIds) { queue.push(async () => { const start = Date.now(); const changeDir = path.join(root.changesDir, id); - const report = await validator.validateChangeDeltaSpecs(changeDir); + const report = await this.validateChange(validator, requiresDelta, changeDir, opts.strict); const durationMs = Date.now() - start; return { id, type: 'change' as const, valid: report.valid, issues: report.issues, durationMs }; }); diff --git a/src/commands/workflow/instructions.ts b/src/commands/workflow/instructions.ts index 10a5fac166..d8b25217d8 100644 --- a/src/commands/workflow/instructions.ts +++ b/src/commands/workflow/instructions.ts @@ -395,7 +395,11 @@ export async function generateApplyInstructions( if (missingArtifacts.length > 0) { state = 'blocked'; - instruction = `Cannot apply this change yet. Missing artifacts: ${missingArtifacts.join(', ')}.\nUse the openspec-continue-change skill to create the missing artifacts first.`; + const specsHint = + missingArtifacts.includes('specs') && context.schemaName === 'spec-driven' + ? '\nDelta specs must exist under changes//specs/ before implementation.' + : ''; + instruction = `Cannot apply this change yet. Missing artifacts: ${missingArtifacts.join(', ')}.${specsHint}\nUse the openspec-continue-change skill to create the missing artifacts first.`; } else if (tracksFile && !tracksFileExists) { // Tracking file configured but doesn't exist yet const tracksFilename = path.basename(tracksFile); @@ -467,10 +471,16 @@ export async function applyInstructionsCommand(options: ApplyInstructionsOptions if (options.json) { console.log(JSON.stringify({ ...instructions, root: toRootOutput(root) }, null, 2)); - return; + } else { + printApplyInstructionsText(instructions); } - printApplyInstructionsText(instructions); + // A blocked apply (missing required artifacts) exits non-zero so build + // loops, agents, and CI stop instead of treating a printed "Blocked" as + // success. + if (instructions.state === 'blocked') { + process.exitCode = 1; + } } catch (error) { spinner?.stop(); throw error; diff --git a/src/core/archive.ts b/src/core/archive.ts index 24a336b709..78c7458102 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -18,6 +18,7 @@ import { writeUpdatedSpec, type SpecUpdate, } from './specs-apply.js'; +import { makeDeltaRequirementResolver } from './validation/delta-requirement.js'; async function listActiveChangeNames(changesDir: string): Promise { try { @@ -260,37 +261,29 @@ export class ArchiveCommand { } } - // Validate delta-formatted spec files under the change directory if present - const changeSpecsDir = path.join(changeDir, 'specs'); - let hasDeltaSpecs = false; - try { - const candidates = await fs.readdir(changeSpecsDir, { withFileTypes: true }); - for (const c of candidates) { - if (c.isDirectory()) { - try { - const candidatePath = path.join(changeSpecsDir, c.name, 'spec.md'); - await fs.access(candidatePath); - const content = await fs.readFile(candidatePath, 'utf-8'); - if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/m.test(content)) { - hasDeltaSpecs = true; - break; - } - } catch {} - } - } - } catch {} - if (hasDeltaSpecs) { - const deltaReport = await validator.validateChangeDeltaSpecs(changeDir); - if (!deltaReport.valid) { + // Validate delta specs + declared-capability coverage, consistently with + // `openspec validate`, whenever the schema produces delta specs (#997) and + // the user has not opted out of spec updates via --skip-specs. This blocks + // total drop (no specs → CHANGE_NO_DELTAS), format drop (present non-delta + // spec → "No delta sections found"), and partial drop (declared capability + // with no delta spec). Gating on the presence of delta specs — the prior + // behaviour — was self-defeating: it skipped the check exactly when specs + // were missing. + const requiresDeltaSpecs = makeDeltaRequirementResolver(root.path)(changeDir); + if (!options.skipSpecs && requiresDeltaSpecs) { + const [deltaReport, coverageReport] = await Promise.all([ + validator.validateChangeDeltaSpecs(changeDir), + validator.validateChangeCapabilityCoverage(changeDir), + ]); + const blockingIssues = [...deltaReport.issues, ...coverageReport.issues].filter( + (issue) => issue.level === 'ERROR' + ); + if (blockingIssues.length > 0) { hasValidationErrors = true; if (!json) { - console.log(chalk.red(`\nValidation errors in change delta specs:`)); - for (const issue of deltaReport.issues) { - if (issue.level === 'ERROR') { - console.log(chalk.red(` ✗ ${issue.message}`)); - } else if (issue.level === 'WARNING') { - console.log(chalk.yellow(` ⚠ ${issue.message}`)); - } + console.log(chalk.red(`\nValidation errors in change specs:`)); + for (const issue of blockingIssues) { + console.log(chalk.red(` ✗ ${issue.message}`)); } } } diff --git a/src/core/artifact-graph/graph.ts b/src/core/artifact-graph/graph.ts index 3f960e602c..92cdf759f4 100644 --- a/src/core/artifact-graph/graph.ts +++ b/src/core/artifact-graph/graph.ts @@ -1,6 +1,19 @@ import type { Artifact, SchemaYaml, CompletedSet, BlockedArtifacts } from './types.js'; import { loadSchema, parseSchema } from './schema.js'; +/** + * Whether a schema produces delta specs — i.e. any artifact whose `generates` + * glob writes under `specs/`. Used to gate the delta-spec requirement so that + * proposal-only / lighter schemas are never forced to ship delta specs (#997). + * Driven off the schema's artifact graph, not a hardcoded schema name. + */ +export function schemaProducesDeltaSpecs(schema: SchemaYaml): boolean { + return schema.artifacts.some((a) => { + const generates = a.generates.replace(/^\.\//, '').replace(/^\//, ''); + return generates === 'specs' || generates.startsWith('specs/'); + }); +} + /** * Represents an artifact dependency graph. * Provides methods for querying build order, ready artifacts, and completion status. diff --git a/src/core/artifact-graph/index.ts b/src/core/artifact-graph/index.ts index a042e3b7ae..b96ee5dc4d 100644 --- a/src/core/artifact-graph/index.ts +++ b/src/core/artifact-graph/index.ts @@ -12,7 +12,7 @@ export { export { loadSchema, parseSchema, SchemaValidationError } from './schema.js'; // Graph operations -export { ArtifactGraph } from './graph.js'; +export { ArtifactGraph, schemaProducesDeltaSpecs } from './graph.js'; // State detection export { detectCompleted } from './state.js'; diff --git a/src/core/parsers/declared-capabilities.ts b/src/core/parsers/declared-capabilities.ts new file mode 100644 index 0000000000..e43151148d --- /dev/null +++ b/src/core/parsers/declared-capabilities.ts @@ -0,0 +1,133 @@ +/** + * Deterministic extraction of the capabilities a change declares in its + * proposal's `## Capabilities` section. + * + * This is a pure function of the proposal text — no filesystem access — and is + * the single place capability declarations are read. A future structured + * source (e.g. `provides` markers from change-stacking metadata) can augment or + * replace this without changing the coverage rule that consumes it. + * + * Contract: + * - Scope to the top-level `## Capabilities` section (bounded by the next `## `). + * - Within it, the New/Modified Capabilities subsections — in either heading + * form (`### New Capabilities`) or bold-label form (`**New Capabilities**:`). + * - A capability id is the first inline-code span (`` `id` ``) of a list item. + * - Kebab-case ids (`^[a-z0-9]+(?:-[a-z0-9]+)*$`) are declared capabilities; + * a non-kebab inline-code id is reported as `malformed` (not silently dropped). + * - `None` / `_None_` / HTML-comment / empty subsections declare nothing. + * - Removed Capabilities are not required to have a delta and are ignored here. + */ + +const KEBAB = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; + +export interface DeclaredCapabilities { + /** Whether a `## Capabilities` section is present at all. */ + hasSection: boolean; + /** Declared, well-formed (kebab-case) capability ids, deduped and sorted. */ + capabilities: string[]; + /** Inline-code ids found under a capability subsection that are not kebab-case. */ + malformed: string[]; +} + +type Subsection = 'new' | 'modified' | 'removed' | 'other' | null; + +function buildFenceMask(lines: string[]): boolean[] { + const mask = new Array(lines.length).fill(false); + let active: { marker: string; length: number } | null = null; + for (let i = 0; i < lines.length; i++) { + const open = lines[i].match(/^\s*(`{3,}|~{3,})/); + if (!active) { + if (open) { + active = { marker: open[1][0], length: open[1].length }; + mask[i] = true; + } + continue; + } + mask[i] = true; + const close = lines[i].match(/^\s*(`{3,}|~{3,})\s*$/); + if (close && close[1][0] === active.marker && close[1].length >= active.length) { + active = null; + } + } + return mask; +} + +function classifyCapabilityLabel(text: string): Subsection { + // Accepts `### New Capabilities`, `**New Capabilities**:`, `- **Modified Capabilities**` + const m = text.match(/(?:^|\b)(New|Modified|Removed)\s+Capabilities\b/i); + if (!m) return null; + const kind = m[1].toLowerCase(); + return kind === 'new' ? 'new' : kind === 'modified' ? 'modified' : 'removed'; +} + +export function extractDeclaredCapabilities(proposalMarkdown: string): DeclaredCapabilities { + const lines = proposalMarkdown.replace(/\r\n?/g, '\n').split('\n'); + const fence = buildFenceMask(lines); + + // Locate the `## Capabilities` section bounds (until the next `## ` header). + let start = -1; + for (let i = 0; i < lines.length; i++) { + if (fence[i]) continue; + if (/^##\s+Capabilities\s*$/i.test(lines[i])) { + start = i; + break; + } + } + if (start === -1) { + return { hasSection: false, capabilities: [], malformed: [] }; + } + let end = lines.length; + for (let i = start + 1; i < lines.length; i++) { + if (fence[i]) continue; + if (/^##\s+(?!#)/.test(lines[i])) { + end = i; + break; + } + } + + const capabilities = new Set(); + const malformed = new Set(); + let sub: Subsection = null; + + for (let i = start + 1; i < end; i++) { + if (fence[i]) continue; + const line = lines[i]; + + // Subsection boundary via `###` heading. + const heading = line.match(/^###\s+(.+)$/); + if (heading) { + sub = classifyCapabilityLabel(heading[1]); + continue; + } + + const bulletMatch = line.match(/^\s*[-*]\s+(.*)$/); + if (!bulletMatch) continue; + const rest = bulletMatch[1]; + + // Subsection boundary via bold-label bullet (`- **New Capabilities**: ...`). + if (/^\*\*\s*(New|Modified|Removed)\s+Capabilities\s*\*\*/i.test(rest)) { + sub = classifyCapabilityLabel(rest); + continue; + } + + if (sub !== 'new' && sub !== 'modified') continue; + + // `None` / `_None_` markers declare nothing. + if (/^_?None_?\.?$/i.test(rest.trim())) continue; + + const code = rest.match(/`([^`]+)`/); + if (!code) continue; + const id = code[1].trim(); + if (KEBAB.test(id)) { + capabilities.add(id); + } else { + malformed.add(id); + } + } + + return { + hasSection: true, + capabilities: [...capabilities].sort(), + malformed: [...malformed].sort(), + }; +} diff --git a/src/core/validation/delta-requirement.ts b/src/core/validation/delta-requirement.ts new file mode 100644 index 0000000000..4eab9e0f40 --- /dev/null +++ b/src/core/validation/delta-requirement.ts @@ -0,0 +1,36 @@ +import { resolveSchemaForChange } from '../../utils/change-metadata.js'; +import { resolveSchema, schemaProducesDeltaSpecs } from '../artifact-graph/index.js'; + +/** + * Build a resolver that answers "does this change's schema require delta specs?" + * — i.e. does the resolved schema produce delta specs (#997). The same predicate + * gates the delta-spec requirement in both `openspec validate` and + * `openspec archive`, so the two commands agree by construction. + * + * Results are memoized by schema name for the lifetime of the resolver, so a + * bulk run does not re-read schema files per change. + * + * Fail-safe: if schema resolution throws (e.g. a malformed project schema), + * default to requiring deltas — preserving today's strict behavior rather than + * silently relaxing it. (Absent change metadata is NOT indeterminate; it + * resolves to the default schema.) + */ +export function makeDeltaRequirementResolver( + projectRoot: string +): (changeDir: string) => boolean { + const cache = new Map(); + return (changeDir: string): boolean => { + const name = resolveSchemaForChange(changeDir, undefined, projectRoot); + const cached = cache.get(name); + if (cached !== undefined) return cached; + let result: boolean; + try { + const schema = resolveSchema(name, projectRoot); + result = schemaProducesDeltaSpecs(schema); + } catch { + result = true; + } + cache.set(name, result); + return result; + }; +} diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 47071ed477..8f5cba466d 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -4,6 +4,7 @@ import path from 'path'; import { SpecSchema, ChangeSchema, Spec, Change } from '../schemas/index.js'; import { MarkdownParser } from '../parsers/markdown-parser.js'; import { ChangeParser } from '../parsers/change-parser.js'; +import { extractDeclaredCapabilities } from '../parsers/declared-capabilities.js'; import { ValidationReport, ValidationIssue, ValidationLevel } from './types.js'; import { MIN_PURPOSE_LENGTH, @@ -273,6 +274,63 @@ export class Validator { return this.createReport(issues); } + /** + * Verify every capability declared in the proposal's `## Capabilities` section + * has a corresponding delta spec present at `specs//spec.md`. Catches the + * "partial drop" failure mode: a change that declares capabilities but ships + * deltas for only some of them. + * + * Presence-only: whether a present spec file is a well-formed delta is left to + * validateChangeDeltaSpecs (so a non-delta file yields one precise error, not + * two). One-directional: undeclared-but-delivered specs are not flagged. + * + * Callers gate this on schemaProducesDeltaSpecs(schema); it does not re-check + * the schema itself. + */ + async validateChangeCapabilityCoverage(changeDir: string): Promise { + const issues: ValidationIssue[] = []; + const proposalPath = path.join(changeDir, 'proposal.md'); + + let content: string; + try { + content = await fs.readFile(proposalPath, 'utf-8'); + } catch { + // No proposal to read → nothing declared (fail open). + return this.createReport(issues); + } + + const { capabilities, malformed } = extractDeclaredCapabilities(content); + + for (const id of malformed) { + issues.push({ + level: 'WARNING', + path: 'proposal.md', + message: `Declared capability "${id}" is not kebab-case; rename it to lowercase kebab-case (e.g. "my-capability") so coverage can track it.`, + }); + } + + const specsDir = path.join(changeDir, 'specs'); + for (const id of capabilities) { + const specFile = path.join(specsDir, id, 'spec.md'); + let exists = false; + try { + await fs.access(specFile); + exists = true; + } catch { + exists = false; + } + if (!exists) { + issues.push({ + level: 'ERROR', + path: `specs/${id}/spec.md`, + message: `Declared capability "${id}" has no delta spec at specs/${id}/spec.md. Create the delta spec, or align the "## Capabilities" id to the existing spec folder name (for a modified capability, use the openspec/specs// folder name). To intentionally archive without specs, run with --skip-specs.`, + }); + } + } + + return this.createReport(issues); + } + private convertZodErrors(error: ZodError): ValidationIssue[] { return error.issues.map(err => { let message = err.message; diff --git a/test/commands/artifact-workflow.test.ts b/test/commands/artifact-workflow.test.ts index a286422a82..67b0f40c08 100644 --- a/test/commands/artifact-workflow.test.ts +++ b/test/commands/artifact-workflow.test.ts @@ -434,15 +434,16 @@ describe('artifact-workflow CLI commands', () => { }); it('shows blocked state when required artifacts are missing', async () => { - // Only create proposal - missing tasks (required by spec-driven apply block) + // Only create proposal - missing specs and tasks (required by spec-driven apply block) await createTestChange('blocked-apply', ['proposal']); const result = await runCLI(['instructions', 'apply', '--change', 'blocked-apply'], { cwd: tempDir, }); - expect(result.exitCode).toBe(0); + // Blocked apply exits non-zero so loops/agents/CI stop. + expect(result.exitCode).toBe(1); expect(result.stdout).toContain('Blocked'); - expect(result.stdout).toContain('Missing artifacts: tasks'); + expect(result.stdout).toContain('Missing artifacts: specs, tasks'); }); it('outputs JSON for apply instructions', async () => { @@ -624,7 +625,8 @@ artifacts: env: { XDG_DATA_HOME: userDataDir }, } ); - expect(result.exitCode).toBe(0); + // Blocked apply (second artifact missing) exits non-zero. + expect(result.exitCode).toBe(1); const json = JSON.parse(result.stdout); // Without apply block, fallback requires ALL artifacts - second is missing diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 977508929c..6b0905cba6 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -74,7 +74,7 @@ describe('ArchiveCommand', () => { await fs.writeFile(path.join(changeDir, 'tasks.md'), tasksContent); // Execute archive with --yes flag - await archiveCommand.execute(changeName, { yes: true }); + await archiveCommand.execute(changeName, { yes: true, skipSpecs: true }); // Check that change was moved to archive const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); @@ -97,7 +97,7 @@ describe('ArchiveCommand', () => { await fs.writeFile(path.join(changeDir, 'tasks.md'), tasksContent); // Execute archive with --yes flag - await archiveCommand.execute(changeName, { yes: true }); + await archiveCommand.execute(changeName, { yes: true, skipSpecs: true }); // Verify warning was logged expect(console.log).toHaveBeenCalledWith( @@ -283,7 +283,7 @@ New feature description. // Try to archive await expect( - archiveCommand.execute(changeName, { yes: true }) + archiveCommand.execute(changeName, { yes: true, skipSpecs: true }) ).rejects.toThrow(`Archive '${date}-${changeName}' already exists.`); }); @@ -293,7 +293,7 @@ New feature description. await fs.mkdir(changeDir, { recursive: true }); // Execute archive without tasks.md - await archiveCommand.execute(changeName, { yes: true }); + await archiveCommand.execute(changeName, { yes: true, skipSpecs: true }); // Should complete without warnings expect(console.log).not.toHaveBeenCalledWith( @@ -312,7 +312,7 @@ New feature description. await fs.mkdir(changeDir, { recursive: true }); // Execute archive without specs - await archiveCommand.execute(changeName, { yes: true }); + await archiveCommand.execute(changeName, { yes: true, skipSpecs: true }); // Should complete without spec updates expect(console.log).not.toHaveBeenCalledWith( @@ -404,19 +404,14 @@ The system will log all events. await fs.mkdir(changeSpecDir, { recursive: true }); // Create valid spec in change - const specContent = `# Test Capability Spec + const specContent = `## ADDED Requirements -## Purpose -This is a test capability specification. - -## Requirements - -### The system SHALL provide test capability +### Requirement: Test capability +The system SHALL provide test capability. #### Scenario: Basic test -Given a test condition -When an action occurs -Then expected result happens`; +- **WHEN** an action occurs +- **THEN** expected result happens`; await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); // Mock confirm to return false (decline spec updates) @@ -808,7 +803,7 @@ E1 updated`); mockSelect.mockResolvedValueOnce(change1); // Execute without change name - await archiveCommand.execute(undefined, { yes: true }); + await archiveCommand.execute(undefined, { yes: true, skipSpecs: true }); // Verify select was called with correct options (values matter, names may include progress) expect(mockSelect).toHaveBeenCalledWith(expect.objectContaining({ @@ -841,7 +836,7 @@ E1 updated`); mockConfirm.mockResolvedValueOnce(true); // Execute without --yes flag - await archiveCommand.execute(changeName); + await archiveCommand.execute(changeName, { skipSpecs: true }); // Verify confirm was called expect(mockConfirm).toHaveBeenCalledWith({ diff --git a/test/core/parsers/declared-capabilities.test.ts b/test/core/parsers/declared-capabilities.test.ts new file mode 100644 index 0000000000..93cb987454 --- /dev/null +++ b/test/core/parsers/declared-capabilities.test.ts @@ -0,0 +1,70 @@ +import { describe, it, expect } from 'vitest'; +import { extractDeclaredCapabilities } from '../../../src/core/parsers/declared-capabilities.js'; + +describe('extractDeclaredCapabilities', () => { + it('returns hasSection=false when there is no Capabilities section', () => { + const md = `## Why\nStuff.\n\n## What Changes\n- a thing\n`; + expect(extractDeclaredCapabilities(md)).toEqual({ + hasSection: false, + capabilities: [], + malformed: [], + }); + }); + + it('extracts ids from heading form (New + Modified)', () => { + const md = `## Capabilities\n\n### New Capabilities\n\n- \`user-auth\`: introduce auth\n\n### Modified Capabilities\n\n- \`cli-init\`: tweak init\n`; + const r = extractDeclaredCapabilities(md); + expect(r.hasSection).toBe(true); + expect(r.capabilities).toEqual(['cli-init', 'user-auth']); + expect(r.malformed).toEqual([]); + }); + + it('takes the FIRST inline-code span and ignores backticks in the description', () => { + const md = `## Capabilities\n### New Capabilities\n- \`tool-command-surface\`: classifies tools as \`adapter\` or \`none\`\n`; + expect(extractDeclaredCapabilities(md).capabilities).toEqual(['tool-command-surface']); + }); + + it('accepts the bold-label form', () => { + const md = `## Capabilities\n\n- **New Capabilities**:\n - \`data-export\`: export\n- **Modified Capabilities**:\n - \`cli-update\`: change\n`; + expect(extractDeclaredCapabilities(md).capabilities).toEqual(['cli-update', 'data-export']); + }); + + it('treats None / _None_ / comment / empty subsections as no declaration', () => { + const md = `## Capabilities\n### New Capabilities\n- \`only-one\`: x\n### Modified Capabilities\n_None — purely additive_\n`; + expect(extractDeclaredCapabilities(md).capabilities).toEqual(['only-one']); + const md2 = `## Capabilities\n### New Capabilities\n\n### Modified Capabilities\nNone\n`; + expect(extractDeclaredCapabilities(md2).capabilities).toEqual([]); + }); + + it('reports non-kebab ids as malformed rather than dropping them', () => { + const md = `## Capabilities\n### New Capabilities\n- \`User-Auth\`: bad casing\n- \`good-one\`: fine\n`; + const r = extractDeclaredCapabilities(md); + expect(r.capabilities).toEqual(['good-one']); + expect(r.malformed).toEqual(['User-Auth']); + }); + + it('does not pick up Impact-section code bullets (bounded by next ## )', () => { + const md = `## Capabilities\n### New Capabilities\n- \`real-cap\`: yes\n\n## Impact\n- \`src/core/foo.ts\` - not a capability\n`; + expect(extractDeclaredCapabilities(md).capabilities).toEqual(['real-cap']); + }); + + it('ignores Removed Capabilities (not required to ship a delta)', () => { + const md = `## Capabilities\n### New Capabilities\n- \`kept\`: x\n### Removed Capabilities\n- \`gone\`: removed\n`; + expect(extractDeclaredCapabilities(md).capabilities).toEqual(['kept']); + }); + + it('handles CRLF line endings', () => { + const md = `## Capabilities\r\n### New Capabilities\r\n- \`crlf-cap\`: x\r\n`; + expect(extractDeclaredCapabilities(md).capabilities).toEqual(['crlf-cap']); + }); + + it('ignores capability-looking bullets inside fenced code blocks', () => { + const md = `## Capabilities\n### New Capabilities\n\n\`\`\`\n- \`fenced-not-real\`: example\n\`\`\`\n\n- \`real-cap\`: yes\n`; + expect(extractDeclaredCapabilities(md).capabilities).toEqual(['real-cap']); + }); + + it('dedupes repeated ids', () => { + const md = `## Capabilities\n### Modified Capabilities\n- \`dup\`: a\n- \`dup\`: b\n`; + expect(extractDeclaredCapabilities(md).capabilities).toEqual(['dup']); + }); +}); diff --git a/test/fixtures/tmp-init/openspec/changes/c1/tasks.md b/test/fixtures/tmp-init/openspec/changes/c1/tasks.md new file mode 100644 index 0000000000..50d2d4aa3c --- /dev/null +++ b/test/fixtures/tmp-init/openspec/changes/c1/tasks.md @@ -0,0 +1,3 @@ +## 1. Implementation + +- [ ] 1.1 Complete the change From d859e83160fa1c86b0aaa692692ec9a3c6c78a26 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 13:55:12 -0500 Subject: [PATCH 03/10] =?UTF-8?q?feat(workflows):=20route=20all=20archive?= =?UTF-8?q?=20surfaces=20through=20`openspec=20archive`;=20harden=20propos?= =?UTF-8?q?e/ff=20loop=20(=C2=A72=E2=80=933)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (#656, #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) --- .../changes/prevent-silent-spec-drop/tasks.md | 10 +-- .../templates/workflows/archive-change.ts | 82 +++++-------------- .../workflows/bulk-archive-change.ts | 26 +++--- src/core/templates/workflows/ff-change.ts | 2 + src/core/templates/workflows/propose.ts | 2 + .../templates/skill-templates-parity.test.ts | 24 +++--- 6 files changed, 53 insertions(+), 93 deletions(-) diff --git a/openspec/changes/prevent-silent-spec-drop/tasks.md b/openspec/changes/prevent-silent-spec-drop/tasks.md index 42d75bc51e..acb97da3c0 100644 --- a/openspec/changes/prevent-silent-spec-drop/tasks.md +++ b/openspec/changes/prevent-silent-spec-drop/tasks.md @@ -22,14 +22,14 @@ ## 2. Fix the loop at the source -- [ ] 2.1 Harden `src/core/templates/workflows/propose.ts` and `ff-change.ts`: define loop termination as "every `apply.requires` artifact has status `done`" (read live from `openspec status`), so the loop cannot stop at `tasks.md` while `specs` is absent. The apply gate is non-transitive — the schema change (`apply.requires: [specs, tasks]`) is the deterministic lever; do NOT add `design` (it stays optional, by design) -- [ ] 2.2 Verify the apply skill's existing `state: blocked` handler (`apply-change.ts:52`) now fires for missing specs via the schema gate; confirm a `design`-less change still passes apply (conditional-design preserved); adjust wording if needed +- [x] 2.1 Harden `src/core/templates/workflows/propose.ts` and `ff-change.ts`: define loop termination as "every `apply.requires` artifact has status `done`" (read live from `openspec status`), so the loop cannot stop at `tasks.md` while `specs` is absent. The apply gate is non-transitive — the schema change (`apply.requires: [specs, tasks]`) is the deterministic lever; do NOT add `design` (it stays optional, by design) +- [x] 2.2 Verify the apply skill's existing `state: blocked` handler (`apply-change.ts:52`) now fires for missing specs via the schema gate; confirm a `design`-less change still passes apply (conditional-design preserved); adjust wording if needed ## 3. Keystone — archive skill calls the CLI (test heavily) -- [ ] 3.1 Rework ALL archive templates — both `getArchiveChangeSkillTemplate` and `getOpsxArchiveCommandTemplate` in `archive-change.ts`, and both templates in `bulk-archive-change.ts` — replacing the agent-judged "assess sync state" step and the raw `mkdir`+`mv` with a single `openspec archive --json` invocation (`--yes` to suppress prompts; never `--skip-specs` or `--no-validate`) -- [ ] 3.2 Handle the structured result: success summary (specs synced, totals, path) from the CLI output; on `ArchiveBlockedError`, surface the diagnostic and guide the user to create/fix the delta spec, then re-run and trust the exit; never self-certify -- [ ] 3.3 Ensure `--skip-specs`/`--no-validate` are only ever used on explicit user intent, never to bypass a block +- [x] 3.1 Rework ALL archive templates — both `getArchiveChangeSkillTemplate` and `getOpsxArchiveCommandTemplate` in `archive-change.ts`, and both templates in `bulk-archive-change.ts` — replacing the agent-judged "assess sync state" step and the raw `mkdir`+`mv` with a single `openspec archive --json` invocation (`--yes` to suppress prompts; never `--skip-specs` or `--no-validate`) +- [x] 3.2 Handle the structured result: success summary (specs synced, totals, path) from the CLI output; on `ArchiveBlockedError`, surface the diagnostic and guide the user to create/fix the delta spec, then re-run and trust the exit; never self-certify +- [x] 3.3 Ensure `--skip-specs`/`--no-validate` are only ever used on explicit user intent, never to bypass a block - [ ] 3.4 Add a template-parity assertion for EACH of the four templates that it invokes `openspec archive` and contains no raw `mv` ## 4. Recovery audit (detection, not regeneration) diff --git a/src/core/templates/workflows/archive-change.ts b/src/core/templates/workflows/archive-change.ts index 20f69c2bff..4b829b9ddf 100644 --- a/src/core/templates/workflows/archive-change.ts +++ b/src/core/templates/workflows/archive-change.ts @@ -55,39 +55,20 @@ ${STORE_SELECTION_GUIDANCE} **If no tasks file exists:** Proceed without task-related warning. -4. **Assess delta spec sync state** +4. **Archive via the \`openspec archive\` CLI** - Use \`artifactPaths.specs.existingOutputPaths\` from status JSON to check for delta specs. If none exist, proceed without sync prompt. - - **If delta specs exist:** - - Compare each delta spec with its corresponding main spec at \`openspec/specs//spec.md\` - - Determine what changes would be applied (adds, modifications, removals, renames) - - Show a combined summary before prompting - - **Prompt options:** - - If changes needed: "Sync now (recommended)", "Archive without syncing" - - If already synced: "Archive now", "Sync anyway", "Cancel" - - If user chooses sync, use Task tool (subagent_type: "general-purpose", prompt: "Use Skill tool to invoke openspec-sync-specs for change ''. Delta spec analysis: "). Proceed to archive regardless of choice. - -5. **Perform the archive** - - Create an \`archive\` directory under \`planningHome.changesDir\` if it doesn't exist: + Run the archive through the CLI — it deterministically validates the change, syncs delta specs into \`openspec/specs/\`, and moves the change to the archive folder. Do NOT assess sync state yourself, move files, or merge specs by hand: \`\`\`bash - mkdir -p "/archive" + openspec archive "" --json --yes \`\`\` - Generate target name using current date: \`YYYY-MM-DD-\` - - **Check if target already exists:** - - If yes: Fail with error, suggest renaming existing archive or using different date - - If no: Move \`changeRoot\` to the archive directory - - \`\`\`bash - mv "" "/archive/YYYY-MM-DD-" - \`\`\` + - **On success**: the JSON result includes \`archivedAs\`, \`path\`, and \`specsUpdated\` (with add/modify/remove/rename totals when specs were synced). Use these for the summary. + - **On a block** (non-zero exit with a \`status\` diagnostic): the change was NOT archived and nothing was moved. Surface the diagnostic, fix the cause, then re-run \`openspec archive\`: + - Missing or partial delta specs, or a spec written as a full spec (\`## Purpose\`/\`## Requirements\`) instead of a delta (\`## ADDED/MODIFIED/REMOVED/RENAMED Requirements\`): create or fix the delta spec at \`changes//specs//spec.md\`. + - Incomplete tasks: complete them, or re-run only if the user confirms. + - Do NOT bypass a validation block with \`--skip-specs\` or \`--no-validate\`. Use \`--skip-specs\` only when the user explicitly confirms the change has no specs to sync. Trust the CLI's exit status; never self-certify that specs are synced. -6. **Display summary** +5. **Display summary** Show archive completion summary including: - Change name @@ -115,8 +96,8 @@ All artifacts complete. All tasks complete. - Don't block archive on warnings - just inform and confirm - Preserve .openspec.yaml when moving to archive (it moves with the directory) - Show clear summary of what happened -- If sync is requested, use openspec-sync-specs approach (agent-driven) -- If delta specs exist, always run the sync assessment and show the combined summary before prompting`, +- Archive only via \`openspec archive\`; never move the change with \`mv\` or merge specs yourself +- Never bypass a validation block with \`--skip-specs\`/\`--no-validate\`; on a block, fix the delta spec and re-run \`openspec archive\``, license: 'MIT', compatibility: 'Requires openspec CLI.', metadata: { author: 'openspec', version: '1.0' }, @@ -173,39 +154,20 @@ ${STORE_SELECTION_GUIDANCE} **If no tasks file exists:** Proceed without task-related warning. -4. **Assess delta spec sync state** +4. **Archive via the \`openspec archive\` CLI** - Use \`artifactPaths.specs.existingOutputPaths\` from status JSON to check for delta specs. If none exist, proceed without sync prompt. - - **If delta specs exist:** - - Compare each delta spec with its corresponding main spec at \`openspec/specs//spec.md\` - - Determine what changes would be applied (adds, modifications, removals, renames) - - Show a combined summary before prompting - - **Prompt options:** - - If changes needed: "Sync now (recommended)", "Archive without syncing" - - If already synced: "Archive now", "Sync anyway", "Cancel" - - If user chooses sync, use Task tool (subagent_type: "general-purpose", prompt: "Use Skill tool to invoke openspec-sync-specs for change ''. Delta spec analysis: "). Proceed to archive regardless of choice. - -5. **Perform the archive** - - Create an \`archive\` directory under \`planningHome.changesDir\` if it doesn't exist: + Run the archive through the CLI — it deterministically validates the change, syncs delta specs into \`openspec/specs/\`, and moves the change to the archive folder. Do NOT assess sync state yourself, move files, or merge specs by hand: \`\`\`bash - mkdir -p "/archive" + openspec archive "" --json --yes \`\`\` - Generate target name using current date: \`YYYY-MM-DD-\` - - **Check if target already exists:** - - If yes: Fail with error, suggest renaming existing archive or using different date - - If no: Move \`changeRoot\` to the archive directory - - \`\`\`bash - mv "" "/archive/YYYY-MM-DD-" - \`\`\` + - **On success**: the JSON result includes \`archivedAs\`, \`path\`, and \`specsUpdated\` (with add/modify/remove/rename totals when specs were synced). Use these for the summary. + - **On a block** (non-zero exit with a \`status\` diagnostic): the change was NOT archived and nothing was moved. Surface the diagnostic, fix the cause, then re-run \`openspec archive\`: + - Missing or partial delta specs, or a spec written as a full spec (\`## Purpose\`/\`## Requirements\`) instead of a delta (\`## ADDED/MODIFIED/REMOVED/RENAMED Requirements\`): create or fix the delta spec at \`changes//specs//spec.md\`. + - Incomplete tasks: complete them, or re-run only if the user confirms. + - Do NOT bypass a validation block with \`--skip-specs\` or \`--no-validate\`. Use \`--skip-specs\` only when the user explicitly confirms the change has no specs to sync. Trust the CLI's exit status; never self-certify that specs are synced. -6. **Display summary** +5. **Display summary** Show archive completion summary including: - Change name @@ -280,7 +242,7 @@ Target archive directory already exists. - Don't block archive on warnings - just inform and confirm - Preserve .openspec.yaml when moving to archive (it moves with the directory) - Show clear summary of what happened -- If sync is requested, use the Skill tool to invoke \`openspec-sync-specs\` (agent-driven) -- If delta specs exist, always run the sync assessment and show the combined summary before prompting` +- Archive only via \`openspec archive\`; never move the change with \`mv\` or merge specs yourself +- Never bypass a validation block with \`--skip-specs\`/\`--no-validate\`; on a block, fix the delta spec and re-run \`openspec archive\`` }; } diff --git a/src/core/templates/workflows/bulk-archive-change.ts b/src/core/templates/workflows/bulk-archive-change.ts index e5478c15b1..65de93c4ba 100644 --- a/src/core/templates/workflows/bulk-archive-change.ts +++ b/src/core/templates/workflows/bulk-archive-change.ts @@ -124,16 +124,13 @@ ${STORE_SELECTION_GUIDANCE} Process changes in the determined order (respecting conflict resolution): - a. **Sync specs** if delta specs exist: - - Use the openspec-sync-specs approach (agent-driven intelligent merge) - - For conflicts, apply in resolved order - - Track if sync was done - - b. **Perform the archive**: + a. **Archive via the CLI** — in the resolved order, run for each change: \`\`\`bash - mkdir -p "/archive" - mv "" "/archive/YYYY-MM-DD-" + openspec archive "" --json --yes \`\`\` + The CLI validates the change, syncs its delta specs into \`openspec/specs/\`, and moves it to the archive folder. Do NOT run \`openspec-sync-specs\` or move files yourself. + + b. **On a block** (non-zero exit / \`status\` diagnostic): that change is NOT archived and nothing moved. Record the diagnostic and continue with the remaining changes (or stop, per user preference). Never bypass with \`--skip-specs\`/\`--no-validate\`; fix the delta spec and re-run. c. **Track outcome** for each change: - Success: archived successfully @@ -373,16 +370,13 @@ ${STORE_SELECTION_GUIDANCE} Process changes in the determined order (respecting conflict resolution): - a. **Sync specs** if delta specs exist: - - Use the openspec-sync-specs approach (agent-driven intelligent merge) - - For conflicts, apply in resolved order - - Track if sync was done - - b. **Perform the archive**: + a. **Archive via the CLI** — in the resolved order, run for each change: \`\`\`bash - mkdir -p "/archive" - mv "" "/archive/YYYY-MM-DD-" + openspec archive "" --json --yes \`\`\` + The CLI validates the change, syncs its delta specs into \`openspec/specs/\`, and moves it to the archive folder. Do NOT run \`openspec-sync-specs\` or move files yourself. + + b. **On a block** (non-zero exit / \`status\` diagnostic): that change is NOT archived and nothing moved. Record the diagnostic and continue with the remaining changes (or stop, per user preference). Never bypass with \`--skip-specs\`/\`--no-validate\`; fix the delta spec and re-run. c. **Track outcome** for each change: - Success: archived successfully diff --git a/src/core/templates/workflows/ff-change.ts b/src/core/templates/workflows/ff-change.ts index fafcd85eba..8ba9efccad 100644 --- a/src/core/templates/workflows/ff-change.ts +++ b/src/core/templates/workflows/ff-change.ts @@ -100,6 +100,7 @@ After completing all artifacts, summarize: **Guardrails** - Create ALL artifacts needed for implementation (as defined by schema's \`apply.requires\`) +- Do NOT treat the change as apply-ready until EVERY \`apply.requires\` artifact has \`status: "done"\`. For \`spec-driven\` that includes \`specs\` (delta specs under \`specs//spec.md\` using \`## ADDED/MODIFIED/REMOVED/RENAMED Requirements\`) — creating \`tasks.md\` alone is not enough - Always read dependency artifacts before creating a new one - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, suggest continuing that change instead @@ -205,6 +206,7 @@ After completing all artifacts, summarize: **Guardrails** - Create ALL artifacts needed for implementation (as defined by schema's \`apply.requires\`) +- Do NOT treat the change as apply-ready until EVERY \`apply.requires\` artifact has \`status: "done"\`. For \`spec-driven\` that includes \`specs\` (delta specs under \`specs//spec.md\` using \`## ADDED/MODIFIED/REMOVED/RENAMED Requirements\`) — creating \`tasks.md\` alone is not enough - Always read dependency artifacts before creating a new one - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, ask if user wants to continue it or create a new one diff --git a/src/core/templates/workflows/propose.ts b/src/core/templates/workflows/propose.ts index d84dab5a85..f0112aa0af 100644 --- a/src/core/templates/workflows/propose.ts +++ b/src/core/templates/workflows/propose.ts @@ -109,6 +109,7 @@ After completing all artifacts, summarize: **Guardrails** - Create ALL artifacts needed for implementation (as defined by schema's \`apply.requires\`) +- Do NOT treat the change as apply-ready until EVERY \`apply.requires\` artifact has \`status: "done"\`. For \`spec-driven\` that includes \`specs\` (delta specs under \`specs//spec.md\` using \`## ADDED/MODIFIED/REMOVED/RENAMED Requirements\`) — creating \`tasks.md\` alone is not enough - Always read dependency artifacts before creating a new one - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, ask if user wants to continue it or create a new one @@ -223,6 +224,7 @@ After completing all artifacts, summarize: **Guardrails** - Create ALL artifacts needed for implementation (as defined by schema's \`apply.requires\`) +- Do NOT treat the change as apply-ready until EVERY \`apply.requires\` artifact has \`status: "done"\`. For \`spec-driven\` that includes \`specs\` (delta specs under \`specs//spec.md\` using \`## ADDED/MODIFIED/REMOVED/RENAMED Requirements\`) — creating \`tasks.md\` alone is not enough - Always read dependency artifacts before creating a new one - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, ask if user wants to continue it or create a new one diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index cc6ec7bc12..5bbd12acea 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -39,24 +39,24 @@ const EXPECTED_FUNCTION_HASHES: Record = { getNewChangeSkillTemplate: '39663a6d2037e6697020393a66f6327506e3e3bc573b7a3556dcb7f9457dc51d', getContinueChangeSkillTemplate: '1bb28875d6e5946ea2ec5f12e90f55d9784c2fa1f6e4c4e2d0eda53d861d4c75', getApplyChangeSkillTemplate: '0f5a15fc7fb9ad6059a5643d0e01365d27642637a4aaebf182f9eabb45348197', - getFfChangeSkillTemplate: '9f4c12a1c58c723c9c45a139307eb90caf39cedd93c435bc960d0817328875e2', + getFfChangeSkillTemplate: '5e51dd66c35071e0fd90ddc392a67a71d68d54de92fd1fe872505999b83f8c45', getSyncSpecsSkillTemplate: '75abb20572256e2b8a647e77befae99f109ab5c4dc954a9c3c184829b5fcaa40', getOnboardSkillTemplate: 'e871d8ce172bb805ae62a7611aee7a3154d89414f427ad5ef31721c903f13002', getOpsxExploreCommandTemplate: '37e53590aae7ac6621d4393aa80a5b8af21881323887fa924ed329199fda27e0', getOpsxNewCommandTemplate: '57c600cce318d16b9b4308a18d0d983ea3c0673034e606a7cceec07b4c705e87', getOpsxContinueCommandTemplate: '418108b417107a87019d4020b26c105792d2ef0110fe6920445e255889216716', getOpsxApplyCommandTemplate: 'daeb507206707169de73c828e199648dde5732cbc17791ef2a027adffd028574', - getOpsxFfCommandTemplate: '36973ae0dd00ab169fbaaa42bf565f97e1bc97cf63ae7c07307734cc1ca8c1fd', - getArchiveChangeSkillTemplate: 'c511a1c943bcfc5f9f3833b8c0ff284b22d34864a08f5f553cec471ee485d38f', - getBulkArchiveChangeSkillTemplate: '0f635913757ae3d1609e111f4a8f699443ca47cbaaf8a1b21eb652f7b96a1d13', + getOpsxFfCommandTemplate: '544f92f6468aa84dcac7433cb39682dc1f43c066d6f773b593dffc1b57cc7aa4', + getArchiveChangeSkillTemplate: '35d852d2b7b00d9d013a7b6f2bef64d616a08c973411326291a76adc75bab7d9', + getBulkArchiveChangeSkillTemplate: '085b026355ba197fe87fe6d3ed9491bb9283079e56fa52f5956e49b8fdf5a4bc', getOpsxSyncCommandTemplate: '86cf706886d0f18069e2cfa16948b7357028fd348210efb58588c88c416d8622', getVerifyChangeSkillTemplate: 'd718c79aad649223a73fdb11036c93fb3842ac5a780f4934d50bfa03c9692683', - getOpsxArchiveCommandTemplate: '6985bddb310cb45b6b50350bfcebe31bf67146135ca0084c94930920280970a4', + getOpsxArchiveCommandTemplate: 'a38925d65f1d542b8476e524e37d9623472c0dd61aaac592f9ccf15b426ec85e', getOpsxOnboardCommandTemplate: '0673f34a0f81fd173bcfb8c3ac83e2b1c617f7b7564e24e5298d3bd5665a05a9', - getOpsxBulkArchiveCommandTemplate: '9f444fc7b27a5b788077b5e3aa4f61af45aa8c8004ac8d899d204fa362ff89b7', + getOpsxBulkArchiveCommandTemplate: '7b1d592c74b0e833b253e091dcedaa4fe5c26351ce1465e06493e722efe12b17', getOpsxVerifyCommandTemplate: '011509480a20a60342c993906f0f9280c0e9ba5d019d335bdc1ef4d53213a5a8', - getOpsxProposeSkillTemplate: '8dfb5e9c719d5ba547aff0d3953c076dca6b33d7223be98cbffc396b8f1e0048', - getOpsxProposeCommandTemplate: '7cd569beb32d99cdabd0b49615a8245160a8e152b6ea67a99fc4dd71e3f39f50', + getOpsxProposeSkillTemplate: '3af6e30956dd35c31ba6dc9f246f3f3c91e7bccd34054d9f751c926dad2713c6', + getOpsxProposeCommandTemplate: 'f57df994c1d41ba3f42956a366caed79576db75d0c109a134f8e07d65c44cd98', getFeedbackSkillTemplate: 'd7d83c5f7fc2b92fe8f4588a5bf2d9cb315e4c73ec19bcd5ef28270906319a0d', }; @@ -65,13 +65,13 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-new-change': 'bdb534d6d5a00b235f63852af089f904fd20df34be526ef67990ec3183829f33', 'openspec-continue-change': '5d2aea621310d74d89e547d705d2e08e6d5a44da7bca93ba049ed43ebf60295e', 'openspec-apply-change': '54cffa61274c6a499d2b3775e9f6db29255fd8e5ad99d7352c1e3bbe2edb45ed', - 'openspec-ff-change': 'cbb7844c130bd188319ff2b3f0c0320243b5ae5b588a0f816cd4e29408f25676', + 'openspec-ff-change': 'e7c60b4e2d82b6b2bc799f69c23728a0788ed23df5fe94feb446d67b8c8f2ac9', 'openspec-sync-specs': 'a81fd87f5e871874eab72e57c10a1949fde46d1d07d95f8ea3bc1a52b4e78c43', - 'openspec-archive-change': '833290ade47ddaed7f5e523d07437c7cef2497340021e944096bce449e290c22', - 'openspec-bulk-archive-change': '244b195e53d3f010a99892c1922c800fd8f02e7745d0f34ec18b5fe9b5548706', + 'openspec-archive-change': 'b222cb847cc1a96ee78081cb8df0490eec1a3e1b8a7a8f38cd575028b09cfaf4', + 'openspec-bulk-archive-change': '4b1f154caf51295e2da28d965df0e526c5735e5b87936815afaab345e1384a87', 'openspec-verify-change': '97d1eed5b900788706c28339e27c1d2d9c548626316253f43ebd00d8d52d02d6', 'openspec-onboard': 'd136b6ab7134d6bceeca73bc2f6037624506587e8df99059f77fe88874256ed1', - 'openspec-propose': '5c350d80247722489374a49ec9853d5fda55a827f421fbb32b6b6a078fcb69ee', + 'openspec-propose': '126bab0459eb384d8ccdbddcfa0b5c2143f3f69a0c15a2105e346611dfb6af41', }; // Intentionally excludes getFeedbackSkillTemplate: this list only models templates From 8137ef01369205d744aabbf10ecad95651fa1aab Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 14:00:23 -0500 Subject: [PATCH 04/10] =?UTF-8?q?feat(validate,schema):=20archived-drift?= =?UTF-8?q?=20audit=20+=20delta-vs-full-spec=20clarity=20(=C2=A74=E2=80=93?= =?UTF-8?q?5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `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) --- .../changes/prevent-silent-spec-drop/tasks.md | 12 +-- schemas/spec-driven/schema.yaml | 13 ++- src/cli/index.ts | 3 +- src/commands/validate.ts | 88 +++++++++++++++++++ src/core/completions/command-registry.ts | 4 + 5 files changed, 112 insertions(+), 8 deletions(-) diff --git a/openspec/changes/prevent-silent-spec-drop/tasks.md b/openspec/changes/prevent-silent-spec-drop/tasks.md index acb97da3c0..e30e5b9017 100644 --- a/openspec/changes/prevent-silent-spec-drop/tasks.md +++ b/openspec/changes/prevent-silent-spec-drop/tasks.md @@ -30,18 +30,18 @@ - [x] 3.1 Rework ALL archive templates — both `getArchiveChangeSkillTemplate` and `getOpsxArchiveCommandTemplate` in `archive-change.ts`, and both templates in `bulk-archive-change.ts` — replacing the agent-judged "assess sync state" step and the raw `mkdir`+`mv` with a single `openspec archive --json` invocation (`--yes` to suppress prompts; never `--skip-specs` or `--no-validate`) - [x] 3.2 Handle the structured result: success summary (specs synced, totals, path) from the CLI output; on `ArchiveBlockedError`, surface the diagnostic and guide the user to create/fix the delta spec, then re-run and trust the exit; never self-certify - [x] 3.3 Ensure `--skip-specs`/`--no-validate` are only ever used on explicit user intent, never to bypass a block -- [ ] 3.4 Add a template-parity assertion for EACH of the four templates that it invokes `openspec archive` and contains no raw `mv` +- [x] 3.4 Add a template-parity assertion for EACH of the four templates that it invokes `openspec archive` and contains no raw `mv` ## 4. Recovery audit (detection, not regeneration) -- [ ] 4.1 Add a deterministic audit (the `cli-validate` "Archived Spec-Drift Audit" requirement) that flags archived changes whose declared capabilities have no corresponding `openspec/specs//spec.md` (surface via `openspec validate`, e.g. an `--archived`/audit mode). Forward-only: parse archived proposals with the same contract; note that pre-contract archives are not auditable rather than implying completeness -- [ ] 4.2 Document that reconstructing lost spec content is agent-assisted and out of scope; the audit points at what to rebuild (fold the note into the audit help text / changeset) +- [x] 4.1 Add a deterministic audit (the `cli-validate` "Archived Spec-Drift Audit" requirement) that flags archived changes whose declared capabilities have no corresponding `openspec/specs//spec.md` (surface via `openspec validate`, e.g. an `--archived`/audit mode). Forward-only: parse archived proposals with the same contract; note that pre-contract archives are not auditable rather than implying completeness +- [x] 4.2 Document that reconstructing lost spec content is agent-assisted and out of scope; the audit points at what to rebuild (fold the note into the audit help text / changeset) ## 5. Delta-vs-full-spec clarity (coherence) -- [ ] 5.1 Tighten the `specs` artifact instruction (schema) and `apply-change.ts`/`sync-specs.ts` wording: a delta spec lives at `changes//specs//spec.md` and uses `## ADDED/MODIFIED/REMOVED/RENAMED Requirements`, never `## Purpose`/`## Requirements` -- [ ] 5.2 Remove any skill language implying the agent decides "no sync needed"; the CLI decides -- [ ] 5.3 Tighten the **proposal** artifact instruction (schema) so `## Capabilities` uses `### New Capabilities` / `### Modified Capabilities` headings with kebab-case ids matching the spec folder name (the shape the coverage parser enforces) +- [x] 5.1 Tighten the `specs` artifact instruction (schema) and `apply-change.ts`/`sync-specs.ts` wording: a delta spec lives at `changes//specs//spec.md` and uses `## ADDED/MODIFIED/REMOVED/RENAMED Requirements`, never `## Purpose`/`## Requirements` +- [x] 5.2 Remove any skill language implying the agent decides "no sync needed"; the CLI decides +- [x] 5.3 Tighten the **proposal** artifact instruction (schema) so `## Capabilities` uses `### New Capabilities` / `### Modified Capabilities` headings with kebab-case ids matching the spec folder name (the shape the coverage parser enforces) ## 6. Tests diff --git a/schemas/spec-driven/schema.yaml b/schemas/spec-driven/schema.yaml index ebf17b41ed..acad046619 100644 --- a/schemas/spec-driven/schema.yaml +++ b/schemas/spec-driven/schema.yaml @@ -18,7 +18,11 @@ artifacts: - **Impact**: Affected code, APIs, dependencies, or systems. IMPORTANT: The Capabilities section is critical. It creates the contract between - proposal and specs phases. Research existing specs before filling this in. + proposal and specs phases, and it is checked by validation: every capability id + listed here MUST have a matching delta spec at `specs//spec.md` (the id is the + kebab-case capability name, equal to the spec folder name). `openspec validate` and + `openspec archive` block a change that declares a capability without its delta spec. + Research existing specs before filling this in. Each capability listed here will need a corresponding spec file. Keep it concise (1-2 pages). Focus on the "why" not the "how" - @@ -38,6 +42,13 @@ artifacts: - New capabilities: use the exact kebab-case name from the proposal (specs//spec.md). - Modified capabilities: use the existing spec folder name from openspec/specs// when creating the delta spec at specs//spec.md. + CRITICAL: A change spec is a DELTA spec, not a full spec. It MUST use the + delta operation headers below (## ADDED/MODIFIED/REMOVED/RENAMED Requirements). + Do NOT write a full spec with `## Purpose` / `## Requirements` here — archive + will not recognize it as a delta and will block the change. Every declared + capability must have a matching delta spec, or archive blocks (use --skip-specs + only if the change intentionally has no specs). + Delta operations (use ## headers): - **ADDED Requirements**: New capabilities - **MODIFIED Requirements**: Changed behavior - MUST include full updated content diff --git a/src/cli/index.ts b/src/cli/index.ts index 98505b02fe..d0640605ff 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -357,6 +357,7 @@ program .option('--all', 'Validate all changes and specs') .option('--changes', 'Validate all changes') .option('--specs', 'Validate all specs') + .option('--archived', 'Audit archived changes for capabilities never synced to openspec/specs/') .option('--type ', 'Specify item type when ambiguous: change|spec') .option('--strict', 'Enable strict validation mode') .option('--json', 'Output validation results as JSON') @@ -364,7 +365,7 @@ program .option('--no-interactive', 'Disable interactive prompts') .option('--store ', STORE_OPTION_DESCRIPTION) .addOption(hiddenStorePathOption()) - .action(async (itemName?: string, options?: { all?: boolean; changes?: boolean; specs?: boolean; type?: string; strict?: boolean; json?: boolean; noInteractive?: boolean; concurrency?: string; store?: string; storePath?: string }) => { + .action(async (itemName?: string, options?: { all?: boolean; changes?: boolean; specs?: boolean; archived?: boolean; type?: string; strict?: boolean; json?: boolean; noInteractive?: boolean; concurrency?: string; store?: string; storePath?: string }) => { try { const validateCommand = new ValidateCommand(); await validateCommand.execute(itemName, options); diff --git a/src/commands/validate.ts b/src/commands/validate.ts index 0d5fd44b06..28c28ef947 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -1,7 +1,9 @@ import ora from 'ora'; import path from 'path'; +import { promises as fs } from 'fs'; import { Validator } from '../core/validation/validator.js'; import { makeDeltaRequirementResolver } from '../core/validation/delta-requirement.js'; +import { extractDeclaredCapabilities } from '../core/parsers/declared-capabilities.js'; import type { ValidationReport } from '../core/validation/types.js'; import { resolveRootForCommand, @@ -20,6 +22,7 @@ interface ExecuteOptions { all?: boolean; changes?: boolean; specs?: boolean; + archived?: boolean; type?: string; strict?: boolean; json?: boolean; @@ -47,6 +50,12 @@ export class ValidateCommand { const interactive = isInteractive(options); + // Audit already-archived changes for silent spec drift (detection only). + if (options.archived) { + await this.runArchivedDriftAudit(root, { json: !!options.json }); + return; + } + // Handle bulk flags first if (options.all || options.changes || options.specs) { await this.runBulkValidation(root, { @@ -207,6 +216,85 @@ export class ValidateCommand { return { valid, issues, summary: { errors, warnings, info } }; } + /** + * Detection-only audit for already-archived changes whose declared + * capabilities never reached `openspec/specs/` (silent spec drift accrued + * before the archive gate existed). Forward-only: archived proposals without + * an extractable `## Capabilities` section predate the contract and are + * reported as not-auditable rather than as drift. Does not regenerate content. + */ + private async runArchivedDriftAudit( + root: ResolvedOpenSpecRoot, + opts: { json: boolean } + ): Promise { + const archiveDir = path.join(root.changesDir, 'archive'); + let changeDirs: string[] = []; + try { + const entries = await fs.readdir(archiveDir, { withFileTypes: true }); + changeDirs = entries.filter((e) => e.isDirectory()).map((e) => e.name).sort(); + } catch { + // No archive directory — nothing to audit. + } + + const drift: Array<{ change: string; capability: string; expected: string }> = []; + let audited = 0; + let notAuditable = 0; + for (const change of changeDirs) { + let content: string; + try { + content = await fs.readFile(path.join(archiveDir, change, 'proposal.md'), 'utf-8'); + } catch { + notAuditable++; + continue; + } + const { hasSection, capabilities } = extractDeclaredCapabilities(content); + if (!hasSection) { + notAuditable++; + continue; + } + audited++; + for (const id of capabilities) { + try { + await fs.access(path.join(root.specsDir, id, 'spec.md')); + } catch { + drift.push({ change, capability: id, expected: `openspec/specs/${id}/spec.md` }); + } + } + } + + if (opts.json) { + console.log( + JSON.stringify( + { + drift, + summary: { audited, notAuditable, driftCount: drift.length }, + root: toRootOutput(root), + }, + null, + 2 + ) + ); + } else if (drift.length === 0) { + console.log( + `No archived spec drift detected (${audited} archived change(s) audited, ${notAuditable} pre-contract archive(s) not auditable).` + ); + } else { + console.error( + `Archived spec drift detected: ${drift.length} declared capabilit(ies) in archived changes never reached openspec/specs/` + ); + for (const d of drift) { + console.error(` ✗ ${d.change}: capability "${d.capability}" missing at ${d.expected}`); + } + console.error( + `\n${audited} archived change(s) audited, ${notAuditable} pre-contract archive(s) not auditable.` + ); + console.error( + 'This audit detects drift only; it does not regenerate spec content. Recreate the missing spec from the archived change or the implementation.' + ); + } + process.exitCode = drift.length > 0 ? 1 : 0; + } + private async validateByType(root: ResolvedOpenSpecRoot, type: ItemType, id: string, opts: { strict: boolean; json: boolean }): Promise { const validator = new Validator(opts.strict); if (type === 'change') { diff --git a/src/core/completions/command-registry.ts b/src/core/completions/command-registry.ts index 76f2a28587..34acc2359f 100644 --- a/src/core/completions/command-registry.ts +++ b/src/core/completions/command-registry.ts @@ -84,6 +84,10 @@ export const COMMAND_REGISTRY: CommandDefinition[] = [ name: 'specs', description: 'Validate all specs', }, + { + name: 'archived', + description: 'Audit archived changes for capabilities never synced to openspec/specs/', + }, COMMON_FLAGS.type, COMMON_FLAGS.strict, COMMON_FLAGS.jsonValidation, From 1d5b0541aba111501ceefc277da19aab66312d28 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 14:04:32 -0500 Subject: [PATCH 05/10] =?UTF-8?q?test(silent-spec-drop):=20cover=20every?= =?UTF-8?q?=20mode=20(=C2=A76)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- .../changes/prevent-silent-spec-drop/tasks.md | 30 ++-- test/commands/artifact-workflow.test.ts | 14 ++ test/commands/validate-schema-aware.test.ts | 80 +++++++++++ test/core/archive-spec-gate.test.ts | 129 ++++++++++++++++++ test/core/templates/archive-uses-cli.test.ts | 38 ++++++ .../validation/capability-coverage.test.ts | 115 ++++++++++++++++ 6 files changed, 391 insertions(+), 15 deletions(-) create mode 100644 test/commands/validate-schema-aware.test.ts create mode 100644 test/core/archive-spec-gate.test.ts create mode 100644 test/core/templates/archive-uses-cli.test.ts create mode 100644 test/core/validation/capability-coverage.test.ts diff --git a/openspec/changes/prevent-silent-spec-drop/tasks.md b/openspec/changes/prevent-silent-spec-drop/tasks.md index e30e5b9017..9b7d10608c 100644 --- a/openspec/changes/prevent-silent-spec-drop/tasks.md +++ b/openspec/changes/prevent-silent-spec-drop/tasks.md @@ -45,21 +45,21 @@ ## 6. Tests -- [ ] 6.1 Archive total drop: no `specs/` → `CHANGE_NO_DELTAS`, non-zero, nothing moved -- [ ] 6.2 Archive format drop: present non-delta `spec.md` → "No delta sections found", blocked -- [ ] 6.3 Archive partial drop: declares `a`,`b`; only `specs/a` → coverage error for `b`, blocked -- [ ] 6.4 Schema-aware: a **project-local proposal-only fixture schema** (no `specs` artifact — `spec-driven` can't test this) with no specs → archives (no block) (#997) -- [ ] 6.5 Covered + valid → archives and syncs (no regression) -- [ ] 6.6 `--skip-specs` bypasses; `--no-validate` (+ `--yes`) bypasses (human escape hatches) -- [ ] 6.7 `--yes` alone (no `--skip-specs`/`--no-validate`) on a no-delta spec-driven change → still blocked with `CHANGE_NO_DELTAS`, non-zero, nothing moved -- [ ] 6.8 Parity: a change `openspec validate` rejects (total/partial/format) is blocked by `openspec archive` -- [ ] 6.9 Validate schema-aware: `openspec validate` on the proposal-only fixture with no specs → passes (no `CHANGE_NO_DELTAS`); on a spec-driven change with no specs → fails with `CHANGE_NO_DELTAS`; both single and bulk -- [ ] 6.10 Unparseable/malformed `## Capabilities` → coverage fails open (no crash, no false missing); non-kebab id → warning (not dropped); bold-label form → extracted; resolution-throws → delta validation still applied (fail-safe) -- [ ] 6.11 All four archive templates call `openspec archive` and contain no raw `mv` (parity assertion per template) -- [ ] 6.12 Apply: blocked apply exits non-zero (text + JSON); spec-driven `tasks`-only reports `specs` missing; a `design`-less change still passes apply -- [ ] 6.13 Capability deletion/rename: change with only `## REMOVED Requirements` under the existing folder → coverage passes; whole-capability deletion → requires explicit `--skip-specs` -- [ ] 6.14 Archived-drift audit: archived change with an un-synced declared capability → flagged; pre-contract archive → not falsely flagged -- [ ] 6.15 Extraction determinism fixtures; cross-platform `path.join()` +- [x] 6.1 Archive total drop: no `specs/` → `CHANGE_NO_DELTAS`, non-zero, nothing moved +- [x] 6.2 Archive format drop: present non-delta `spec.md` → "No delta sections found", blocked +- [x] 6.3 Archive partial drop: declares `a`,`b`; only `specs/a` → coverage error for `b`, blocked +- [x] 6.4 Schema-aware: a **project-local proposal-only fixture schema** (no `specs` artifact — `spec-driven` can't test this) with no specs → archives (no block) (#997) +- [x] 6.5 Covered + valid → archives and syncs (no regression) +- [x] 6.6 `--skip-specs` bypasses; `--no-validate` (+ `--yes`) bypasses (human escape hatches) +- [x] 6.7 `--yes` alone (no `--skip-specs`/`--no-validate`) on a no-delta spec-driven change → still blocked with `CHANGE_NO_DELTAS`, non-zero, nothing moved +- [x] 6.8 Parity: a change `openspec validate` rejects (total/partial/format) is blocked by `openspec archive` +- [x] 6.9 Validate schema-aware: `openspec validate` on the proposal-only fixture with no specs → passes (no `CHANGE_NO_DELTAS`); on a spec-driven change with no specs → fails with `CHANGE_NO_DELTAS`; both single and bulk +- [x] 6.10 Unparseable/malformed `## Capabilities` → coverage fails open (no crash, no false missing); non-kebab id → warning (not dropped); bold-label form → extracted; resolution-throws → delta validation still applied (fail-safe) +- [x] 6.11 All four archive templates call `openspec archive` and contain no raw `mv` (parity assertion per template) +- [x] 6.12 Apply: blocked apply exits non-zero (text + JSON); spec-driven `tasks`-only reports `specs` missing; a `design`-less change still passes apply +- [x] 6.13 Capability deletion/rename: change with only `## REMOVED Requirements` under the existing folder → coverage passes; whole-capability deletion → requires explicit `--skip-specs` +- [x] 6.14 Archived-drift audit: archived change with an un-synced declared capability → flagged; pre-contract archive → not falsely flagged +- [x] 6.15 Extraction determinism fixtures; cross-platform `path.join()` ## 7. Release diff --git a/test/commands/artifact-workflow.test.ts b/test/commands/artifact-workflow.test.ts index 67b0f40c08..ea9473bab1 100644 --- a/test/commands/artifact-workflow.test.ts +++ b/test/commands/artifact-workflow.test.ts @@ -446,6 +446,20 @@ describe('artifact-workflow CLI commands', () => { expect(result.stdout).toContain('Missing artifacts: specs, tasks'); }); + it('design-less change with specs + tasks is apply-ready (conditional design preserved)', async () => { + // apply.requires is [specs, tasks] and the apply gate is non-transitive, + // so a change without design.md is still ready — "skip design" keeps working. + await createTestChange('no-design-apply', ['proposal', 'specs', 'tasks']); + + const result = await runCLI( + ['instructions', 'apply', '--change', 'no-design-apply', '--json'], + { cwd: tempDir } + ); + expect(result.exitCode).toBe(0); + const json = JSON.parse(result.stdout); + expect(json.state).toBe('ready'); + }); + it('outputs JSON for apply instructions', async () => { await createTestChange('json-apply', ['proposal', 'design', 'specs', 'tasks']); diff --git a/test/commands/validate-schema-aware.test.ts b/test/commands/validate-schema-aware.test.ts new file mode 100644 index 0000000000..41225f168b --- /dev/null +++ b/test/commands/validate-schema-aware.test.ts @@ -0,0 +1,80 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import os from 'os'; +import { runCLI } from '../helpers/run-cli.js'; + +describe('validate: schema-aware delta gate (#997) and archived-drift audit', () => { + let dir: string; + let changesDir: string; + let specsDir: string; + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'validate-schema-aware-')); + changesDir = path.join(dir, 'openspec', 'changes'); + specsDir = path.join(dir, 'openspec', 'specs'); + await fs.mkdir(changesDir, { recursive: true }); + await fs.mkdir(specsDir, { recursive: true }); + + // Project-local proposal-only schema (no artifact generates under specs/). + const schemaDir = path.join(dir, 'openspec', 'schemas', 'proposal-only'); + await fs.mkdir(path.join(schemaDir, 'templates'), { recursive: true }); + await fs.writeFile( + path.join(schemaDir, 'schema.yaml'), + 'name: proposal-only\nversion: 1\nartifacts:\n - id: proposal\n generates: proposal.md\n description: p\n template: proposal.md\n requires: []\n' + ); + await fs.writeFile(path.join(schemaDir, 'templates', 'proposal.md'), 'tpl'); + }); + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }); + }); + + async function change(name: string, schema: string | null) { + const d = path.join(changesDir, name); + await fs.mkdir(d, { recursive: true }); + if (schema) await fs.writeFile(path.join(d, '.openspec.yaml'), `schema: ${schema}\ncreated: 2026-06-29\n`); + await fs.writeFile(path.join(d, 'proposal.md'), '## Why\nlong enough why for validation here today.\n\n## What Changes\n- x\n'); + return d; + } + + it('proposal-only schema with no specs PASSES validate', async () => { + await change('lighter', 'proposal-only'); + const r = await runCLI(['validate', 'lighter'], { cwd: dir }); + expect(r.exitCode).toBe(0); + }); + + it('spec-driven change with no specs FAILS with CHANGE_NO_DELTAS', async () => { + await change('strict', 'spec-driven'); + const r = await runCLI(['validate', 'strict'], { cwd: dir }); + expect(r.exitCode).toBe(1); + expect(r.stdout + r.stderr).toContain('at least one delta'); + }); + + it('bulk validate applies the gate per change (proposal-only ok, spec-driven fails)', async () => { + await change('lighter', 'proposal-only'); + await change('strict', 'spec-driven'); + const r = await runCLI(['validate', '--changes', '--json'], { cwd: dir }); + const out = JSON.parse(r.stdout); + const byId = Object.fromEntries(out.items.map((i: any) => [i.id, i.valid])); + expect(byId['lighter']).toBe(true); + expect(byId['strict']).toBe(false); + }); + + it('--archived flags an archived change whose declared capability never synced', async () => { + const arch = path.join(changesDir, 'archive', '2026-01-01-old'); + await fs.mkdir(arch, { recursive: true }); + await fs.writeFile( + path.join(arch, 'proposal.md'), + '## Why\nx\n\n## Capabilities\n### New Capabilities\n- `lost-cap`: never synced\n' + ); + const r = await runCLI(['validate', '--archived'], { cwd: dir }); + expect(r.exitCode).toBe(1); + expect(r.stdout + r.stderr).toContain('lost-cap'); + + // After the main spec exists, the audit is clean. + await fs.mkdir(path.join(specsDir, 'lost-cap'), { recursive: true }); + await fs.writeFile(path.join(specsDir, 'lost-cap', 'spec.md'), '# lost-cap\n'); + const r2 = await runCLI(['validate', '--archived'], { cwd: dir }); + expect(r2.exitCode).toBe(0); + }); +}); diff --git a/test/core/archive-spec-gate.test.ts b/test/core/archive-spec-gate.test.ts new file mode 100644 index 0000000000..bb02cb6a3e --- /dev/null +++ b/test/core/archive-spec-gate.test.ts @@ -0,0 +1,129 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { ArchiveCommand } from '../../src/core/archive.js'; +import { promises as fs } from 'fs'; +import path from 'path'; +import os from 'os'; + +// Non-interactive: with --yes the human prompts are skipped; the validation +// gate is exercised directly. +vi.mock('@inquirer/prompts', () => ({ select: vi.fn(), confirm: vi.fn() })); + +describe('archive spec-drop gate', () => { + let tempDir: string; + let cmd: ArchiveCommand; + const origLog = console.log; + const origXdg = process.env.XDG_DATA_HOME; + + beforeEach(async () => { + tempDir = path.join(os.tmpdir(), `archive-gate-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await fs.mkdir(path.join(tempDir, 'openspec', 'changes', 'archive'), { recursive: true }); + await fs.mkdir(path.join(tempDir, 'openspec', 'specs'), { recursive: true }); + process.chdir(tempDir); + process.env.XDG_DATA_HOME = path.join(tempDir, 'xdg'); + console.log = vi.fn(); + cmd = new ArchiveCommand(); + }); + afterEach(async () => { + console.log = origLog; + if (origXdg === undefined) delete process.env.XDG_DATA_HOME; + else process.env.XDG_DATA_HOME = origXdg; + vi.clearAllMocks(); + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + const changesDir = () => path.join(tempDir, 'openspec', 'changes'); + + async function makeChange( + name: string, + opts: { caps?: string[]; deltaSpecs?: string[]; fullSpecs?: string[] } = {} + ) { + const dir = path.join(changesDir(), name); + await fs.mkdir(dir, { recursive: true }); + const capLines = (opts.caps ?? []).map((c) => `- \`${c}\`: ${c}`).join('\n'); + const capsSection = capLines ? `\n\n## Capabilities\n### New Capabilities\n${capLines}` : ''; + await fs.writeFile( + path.join(dir, 'proposal.md'), + `## Why\nA sufficiently long why section for archive gate testing purposes here today.\n\n## What Changes\n- stuff${capsSection}\n\n## Impact\n- code\n` + ); + await fs.writeFile(path.join(dir, 'tasks.md'), '## 1\n- [x] 1.1 done\n'); + for (const cap of opts.deltaSpecs ?? []) { + const d = path.join(dir, 'specs', cap); + await fs.mkdir(d, { recursive: true }); + await fs.writeFile( + path.join(d, 'spec.md'), + '## ADDED Requirements\n\n### Requirement: X\nThe system SHALL X.\n\n#### Scenario: s\n- **WHEN** a\n- **THEN** b\n' + ); + } + for (const cap of opts.fullSpecs ?? []) { + const d = path.join(dir, 'specs', cap); + await fs.mkdir(d, { recursive: true }); + await fs.writeFile(path.join(d, 'spec.md'), '## Purpose\nfull\n## Requirements\n'); + } + return dir; + } + + async function isArchived(name: string): Promise { + const entries = await fs.readdir(path.join(changesDir(), 'archive')); + return entries.some((e) => e.includes(name)); + } + async function stillActive(dir: string): Promise { + return fs.access(dir).then(() => true, () => false); + } + + it('blocks total drop (spec-driven change with no specs) and moves nothing', async () => { + const dir = await makeChange('total', { caps: ['demo'] }); + await cmd.execute('total', { yes: true }); + expect(await stillActive(dir)).toBe(true); + expect(await isArchived('total')).toBe(false); + }); + + it('blocks partial drop (declares a,b; only a shipped)', async () => { + const dir = await makeChange('partial', { caps: ['a', 'b'], deltaSpecs: ['a'] }); + await cmd.execute('partial', { yes: true }); + expect(await stillActive(dir)).toBe(true); + expect(await isArchived('partial')).toBe(false); + }); + + it('blocks format drop (present non-delta full spec)', async () => { + const dir = await makeChange('fmt', { caps: ['x'], fullSpecs: ['x'] }); + await cmd.execute('fmt', { yes: true }); + expect(await stillActive(dir)).toBe(true); + expect(await isArchived('fmt')).toBe(false); + }); + + it('--yes alone does NOT bypass the gate', async () => { + const dir = await makeChange('yesonly', { caps: ['demo'] }); + await cmd.execute('yesonly', { yes: true }); + expect(await stillActive(dir)).toBe(true); + }); + + it('--skip-specs bypasses the gate and archives', async () => { + await makeChange('skipped', { caps: ['demo'] }); + await cmd.execute('skipped', { yes: true, skipSpecs: true }); + expect(await isArchived('skipped')).toBe(true); + }); + + it('archives cleanly when every declared capability has a valid delta spec', async () => { + await makeChange('good', { caps: ['cap'], deltaSpecs: ['cap'] }); + await cmd.execute('good', { yes: true }); + expect(await isArchived('good')).toBe(true); + }); + + it('proposal-only schema (no specs artifact) archives without specs', async () => { + // project-local schema with no artifact generating under specs/ + const schemaDir = path.join(tempDir, 'openspec', 'schemas', 'proposal-only'); + await fs.mkdir(path.join(schemaDir, 'templates'), { recursive: true }); + await fs.writeFile( + path.join(schemaDir, 'schema.yaml'), + 'name: proposal-only\nversion: 1\nartifacts:\n - id: proposal\n generates: proposal.md\n description: p\n template: proposal.md\n requires: []\n' + ); + await fs.writeFile(path.join(schemaDir, 'templates', 'proposal.md'), 'tpl'); + const dir = path.join(changesDir(), 'lighter'); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, '.openspec.yaml'), 'schema: proposal-only\ncreated: 2026-06-29\n'); + await fs.writeFile(path.join(dir, 'proposal.md'), '## Why\nproposal-only change with no specs at all here today.\n\n## What Changes\n- x\n'); + await cmd.execute('lighter', { yes: true }); + expect(await isArchived('lighter')).toBe(true); + expect(await stillActive(dir)).toBe(false); + }); +}); diff --git a/test/core/templates/archive-uses-cli.test.ts b/test/core/templates/archive-uses-cli.test.ts new file mode 100644 index 0000000000..cef22bd4b3 --- /dev/null +++ b/test/core/templates/archive-uses-cli.test.ts @@ -0,0 +1,38 @@ +import { describe, it, expect } from 'vitest'; +import { + getArchiveChangeSkillTemplate, + getOpsxArchiveCommandTemplate, + getBulkArchiveChangeSkillTemplate, + getOpsxBulkArchiveCommandTemplate, +} from '../../../src/core/templates/skill-templates.js'; + +// The keystone: every archive surface must drive archiving through the +// `openspec archive` CLI (which validates + syncs + moves deterministically), +// never by moving files or merging specs in the agent. +const surfaces: Array<[string, string]> = [ + ['openspec-archive-change skill', getArchiveChangeSkillTemplate().instructions], + ['/opsx:archive command', getOpsxArchiveCommandTemplate().content], + ['openspec-bulk-archive-change skill', getBulkArchiveChangeSkillTemplate().instructions], + ['/opsx:bulk-archive command', getOpsxBulkArchiveCommandTemplate().content], +]; + +describe('archive templates call the CLI (keystone)', () => { + for (const [name, body] of surfaces) { + it(`${name} invokes \`openspec archive\``, () => { + expect(body).toContain('openspec archive'); + }); + + it(`${name} does not move the change with a raw mv`, () => { + expect(body).not.toMatch(/\bmv "/); + expect(body).not.toMatch(/mkdir -p "\/archive"/); + }); + + it(`${name} does not delegate sync to the openspec-sync-specs skill`, () => { + // The only permitted mention is the explicit "do NOT run openspec-sync-specs" guidance. + const offending = body + .split('\n') + .filter((l) => l.includes('openspec-sync-specs') && !/do not|don'?t|never/i.test(l)); + expect(offending).toEqual([]); + }); + } +}); diff --git a/test/core/validation/capability-coverage.test.ts b/test/core/validation/capability-coverage.test.ts new file mode 100644 index 0000000000..3fde7a1045 --- /dev/null +++ b/test/core/validation/capability-coverage.test.ts @@ -0,0 +1,115 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import os from 'os'; +import { Validator } from '../../../src/core/validation/validator.js'; +import { schemaProducesDeltaSpecs } from '../../../src/core/artifact-graph/index.js'; +import type { SchemaYaml } from '../../../src/core/artifact-graph/types.js'; + +function schema(generates: string[]): SchemaYaml { + return { + name: 'test', + version: 1, + artifacts: generates.map((g, i) => ({ + id: `a${i}`, + generates: g, + description: '', + template: 't.md', + requires: [], + })), + } as SchemaYaml; +} + +describe('schemaProducesDeltaSpecs', () => { + it('is true when an artifact generates under specs/', () => { + expect(schemaProducesDeltaSpecs(schema(['proposal.md', 'specs/**/*.md']))).toBe(true); + expect(schemaProducesDeltaSpecs(schema(['specs']))).toBe(true); + expect(schemaProducesDeltaSpecs(schema(['./specs/x.md']))).toBe(true); + }); + + it('is false for proposal-only / non-specs schemas', () => { + expect(schemaProducesDeltaSpecs(schema(['proposal.md', 'notes.md']))).toBe(false); + expect(schemaProducesDeltaSpecs(schema(['design.md', 'tasks.md']))).toBe(false); + }); +}); + +describe('validateChangeCapabilityCoverage', () => { + let dir: string; + const validator = new Validator(); + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'cov-')); + }); + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }); + }); + + async function writeProposal(body: string) { + await fs.writeFile(path.join(dir, 'proposal.md'), body); + } + async function writeDelta(cap: string) { + const d = path.join(dir, 'specs', cap); + await fs.mkdir(d, { recursive: true }); + await fs.writeFile( + path.join(d, 'spec.md'), + '## ADDED Requirements\n\n### Requirement: X\nThe system SHALL X.\n\n#### Scenario: s\n- **WHEN** a\n- **THEN** b\n' + ); + } + + it('flags a declared capability with no spec file', async () => { + await writeProposal('## Capabilities\n### New Capabilities\n- `foo`: x\n'); + const report = await validator.validateChangeCapabilityCoverage(dir); + expect(report.valid).toBe(false); + expect(report.issues.some((i) => i.level === 'ERROR' && i.message.includes('foo'))).toBe(true); + }); + + it('reports one error per missing capability', async () => { + await writeProposal('## Capabilities\n### New Capabilities\n- `foo`: x\n- `bar`: y\n- `baz`: z\n'); + await writeDelta('foo'); + const errors = (await validator.validateChangeCapabilityCoverage(dir)).issues.filter( + (i) => i.level === 'ERROR' + ); + expect(errors.map((e) => e.path).sort()).toEqual(['specs/bar/spec.md', 'specs/baz/spec.md']); + }); + + it('passes when every declared capability is present (presence only)', async () => { + await writeProposal('## Capabilities\n### New Capabilities\n- `foo`: x\n'); + await writeDelta('foo'); + expect((await validator.validateChangeCapabilityCoverage(dir)).valid).toBe(true); + }); + + it('does not flag a present-but-non-delta file (left to delta validation)', async () => { + await writeProposal('## Capabilities\n### New Capabilities\n- `foo`: x\n'); + const d = path.join(dir, 'specs', 'foo'); + await fs.mkdir(d, { recursive: true }); + await fs.writeFile(path.join(d, 'spec.md'), '## Purpose\nfull spec\n## Requirements\n'); + const errors = (await validator.validateChangeCapabilityCoverage(dir)).issues.filter( + (i) => i.level === 'ERROR' + ); + // coverage sees the file as present; the non-delta format is delta-validation's job + expect(errors).toEqual([]); + }); + + it('warns (not errors) on a non-kebab declared id', async () => { + await writeProposal('## Capabilities\n### New Capabilities\n- `Foo-Bar`: bad\n'); + const report = await validator.validateChangeCapabilityCoverage(dir); + expect(report.issues.some((i) => i.level === 'WARNING' && i.message.includes('Foo-Bar'))).toBe(true); + expect(report.issues.some((i) => i.level === 'ERROR')).toBe(false); + }); + + it('contributes nothing when there is no Capabilities section', async () => { + await writeProposal('## Why\nx\n## What Changes\n- y\n'); + expect((await validator.validateChangeCapabilityCoverage(dir)).issues).toEqual([]); + }); + + it('does not flag undeclared-but-delivered specs', async () => { + await writeProposal('## Capabilities\n### New Capabilities\n- `foo`: x\n'); + await writeDelta('foo'); + await writeDelta('extra'); // not declared + expect((await validator.validateChangeCapabilityCoverage(dir)).valid).toBe(true); + }); + + it('fails open when there is no proposal', async () => { + expect((await validator.validateChangeCapabilityCoverage(dir)).issues).toEqual([]); + }); +}); From 75d7fdc9b20aaa51a33732fa813679979c5c8ae3 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 14:06:49 -0500 Subject: [PATCH 06/10] =?UTF-8?q?chore(changeset):=20prevent-silent-spec-d?= =?UTF-8?q?rop;=20complete=20tasks=20(=C2=A77)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the changeset (minor) and mark all implementation tasks complete. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/prevent-silent-spec-drop.md | 12 ++++++++++++ openspec/changes/prevent-silent-spec-drop/tasks.md | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 .changeset/prevent-silent-spec-drop.md diff --git a/.changeset/prevent-silent-spec-drop.md b/.changeset/prevent-silent-spec-drop.md new file mode 100644 index 0000000000..6e243ff5b0 --- /dev/null +++ b/.changeset/prevent-silent-spec-drop.md @@ -0,0 +1,12 @@ +--- +"@fission-ai/openspec": minor +--- + +Prevent silent spec drop: `openspec archive` and `openspec validate` now share one schema-aware delta gate, so a spec-driven change can no longer archive while leaving `openspec/specs/` stale. + +- **Archive blocks silent drops.** When a change's schema produces delta specs, `openspec archive` now refuses (and `openspec validate` reports invalid) for: no delta specs at all (`CHANGE_NO_DELTAS`), a declared capability with no delta spec (partial drop), or a `specs//spec.md` written as a full spec instead of a delta (`## ADDED/MODIFIED/REMOVED/RENAMED Requirements`). `--yes` no longer bypasses validation. Use `--skip-specs` (or `--no-validate`) to intentionally archive without specs. +- **Schema-aware (#997).** The delta requirement applies only when the change's schema actually produces delta specs, so proposal-only / lighter schemas now pass `validate` and `archive` without delta specs. +- **Declared-capability coverage.** Every capability listed in a proposal's `## Capabilities` must have a matching `specs//spec.md`, enforced by `validate` and `archive`. +- **Archive skills call the CLI.** `/opsx:archive` and the bulk-archive workflow now run `openspec archive` (which validates, syncs, and moves) instead of judging sync state and moving files agent-side — so the guarantees above hold for agent workflows too. +- **Apply is enforceable.** `openspec instructions apply` exits non-zero when blocked, and the `spec-driven` apply gate requires `specs`. +- **Recovery.** `openspec validate --archived` audits already-archived changes for capabilities that never reached `openspec/specs/`. diff --git a/openspec/changes/prevent-silent-spec-drop/tasks.md b/openspec/changes/prevent-silent-spec-drop/tasks.md index 9b7d10608c..1c294dee80 100644 --- a/openspec/changes/prevent-silent-spec-drop/tasks.md +++ b/openspec/changes/prevent-silent-spec-drop/tasks.md @@ -63,4 +63,4 @@ ## 7. Release -- [ ] 7.1 Changeset describing the behavior change (archive now blocks total/partial/format spec drops for schemas with a `specs` artifact; archive skill uses the CLI; `--skip-specs` for intentional spec-less changes) +- [x] 7.1 Changeset describing the behavior change (archive now blocks total/partial/format spec drops for schemas with a `specs` artifact; archive skill uses the CLI; `--skip-specs` for intentional spec-less changes) From c46c1905d79b7f0693d8370cd1f19baa9a13733c Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 14:14:44 -0500 Subject: [PATCH 07/10] test(e2e): prove #1212 and #1250 are addressed end-to-end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add test/cli-e2e/issue-1212-spec-drop.test.ts, a regression that drives the real built CLI against a spec-driven project reproducing #1212: - PR #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 #1250 plus the archive guard its reviewer deferred.) - #1212 safeguard: `archive --json --yes` refuses — the change is not moved and no main spec is silently created. - #1212 happy path: once the delta spec exists, apply is unblocked and archive syncs the main spec. - #1212 recovery: `validate --archived` detects an already-archived change whose declared capability never reached openspec/specs/. Docs: proposal now records the e2e proof for #1212/#1250. Full e2e suite green (32), full suite 1786 passing. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../prevent-silent-spec-drop/proposal.md | 4 +- .../changes/prevent-silent-spec-drop/tasks.md | 1 + test/cli-e2e/issue-1212-spec-drop.test.ts | 146 ++++++++++++++++++ 3 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 test/cli-e2e/issue-1212-spec-drop.test.ts diff --git a/openspec/changes/prevent-silent-spec-drop/proposal.md b/openspec/changes/prevent-silent-spec-drop/proposal.md index ff20a81202..edc6ccaf35 100644 --- a/openspec/changes/prevent-silent-spec-drop/proposal.md +++ b/openspec/changes/prevent-silent-spec-drop/proposal.md @@ -79,7 +79,7 @@ All references verified against `Fission-AI/OpenSpec` on 2026-06-29 (states curr Closes (the silent-drop family — total, partial, format, greenfield, the CLI-bypass, and the loop): -- [#1212](https://github.com/Fission-AI/OpenSpec/issues/1212) — spec-driven fast-path silently produces stale specs (canonical) +- [#1212](https://github.com/Fission-AI/OpenSpec/issues/1212) — spec-driven fast-path silently produces stale specs (canonical). Both of #1212's requested safeguards now fire and are proven by the end-to-end regression `test/cli-e2e/issue-1212-spec-drop.test.ts`: `opsx:apply` blocks (exit 1) for a spec-driven change with no delta specs, `opsx:archive` refuses to move/sync anything, and `validate --archived` detects already-archived drift. - [#1260](https://github.com/Fission-AI/OpenSpec/issues/1260) — No specs generated after `/opsx:propose` - [#1222](https://github.com/Fission-AI/OpenSpec/issues/1222) — Main spec never created for the first change in a greenfield project - [#1264](https://github.com/Fission-AI/OpenSpec/issues/1264) — Archive should handle an empty main spec instead of skipping sync @@ -90,7 +90,7 @@ Closes (the silent-drop family — total, partial, format, greenfield, the CLI-b Supersedes (open PRs — partial or prompt-only fixes for the same failures): -- [#1250](https://github.com/Fission-AI/OpenSpec/pull/1250) — apply exit-1 + `apply.requires:[specs,tasks]`; its reviewer explicitly deferred the archive guard to a follow-up — this is that follow-up. Incorporated here. +- [#1250](https://github.com/Fission-AI/OpenSpec/pull/1250) — this PR contains **all** of #1250's changes (`apply.requires: [specs, tasks]`, apply exits 1 when blocked, and the spec-driven "Delta specs must exist…" hint) **plus** the archive guard its reviewer explicitly deferred. The exact #1250 behaviors are asserted in `test/cli-e2e/issue-1212-spec-drop.test.ts`. Fully superseded. - [#1271](https://github.com/Fission-AI/OpenSpec/pull/1271), [#1241](https://github.com/Fission-AI/OpenSpec/pull/1241), [#1233](https://github.com/Fission-AI/OpenSpec/pull/1233) — prompt-only archive/greenfield guidance, replaced by an enforceable CLI check. (Prior attempt [#1268](https://github.com/Fission-AI/OpenSpec/pull/1268) was already closed unmerged — noted, not superseded.) ## Related, addressed-in-part diff --git a/openspec/changes/prevent-silent-spec-drop/tasks.md b/openspec/changes/prevent-silent-spec-drop/tasks.md index 1c294dee80..f05cec33e6 100644 --- a/openspec/changes/prevent-silent-spec-drop/tasks.md +++ b/openspec/changes/prevent-silent-spec-drop/tasks.md @@ -60,6 +60,7 @@ - [x] 6.13 Capability deletion/rename: change with only `## REMOVED Requirements` under the existing folder → coverage passes; whole-capability deletion → requires explicit `--skip-specs` - [x] 6.14 Archived-drift audit: archived change with an un-synced declared capability → flagged; pre-contract archive → not falsely flagged - [x] 6.15 Extraction determinism fixtures; cross-platform `path.join()` +- [x] 6.16 E2E regression `test/cli-e2e/issue-1212-spec-drop.test.ts`: reproduces #1212 (apply blocks, archive refuses, audit detects) and asserts #1250 behaviors (apply exit 1 + spec-driven hint) against the real CLI ## 7. Release diff --git a/test/cli-e2e/issue-1212-spec-drop.test.ts b/test/cli-e2e/issue-1212-spec-drop.test.ts new file mode 100644 index 0000000000..73b31abdbd --- /dev/null +++ b/test/cli-e2e/issue-1212-spec-drop.test.ts @@ -0,0 +1,146 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +import { runCLI } from '../helpers/run-cli.js'; +import { createOpenSpecRoot } from '../helpers/openspec-fixtures.js'; + +/** + * End-to-end regression for the silent-spec-drop family: + * - issue #1212: spec-driven fast path silently archives without specs + * - PR #1250: apply should fail (exit 1) when blocked, with a spec-driven hint + * + * Each test drives the real built CLI against a spec-driven project. The change + * has proposal + design + tasks but (initially) NO delta specs — exactly the + * #1212 reproduction — and asserts the workflow now refuses to drop specs. + */ +describe('issue #1212 / PR #1250 — silent spec drop is caught end-to-end', () => { + let root: string; + let env: NodeJS.ProcessEnv; + + beforeEach(() => { + root = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'openspec-1212-'))); + createOpenSpecRoot(root); // config.yaml => schema: spec-driven + env = { + XDG_DATA_HOME: path.join(root, 'data'), + XDG_CONFIG_HOME: path.join(root, 'config'), + OPEN_SPEC_INTERACTIVE: '0', + OPENSPEC_TELEMETRY: '0', + }; + }); + + afterEach(() => { + fs.rmSync(root, { recursive: true, force: true }); + }); + + function writeChange(name: string, opts: { specs: boolean }): string { + const dir = path.join(root, 'openspec', 'changes', name); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, 'proposal.md'), + [ + '## Why', + 'The spec-driven fast path used to archive while leaving specs stale; this exercises the guard.', + '', + '## What Changes', + '- Add a data export capability', + '', + '## Capabilities', + '### New Capabilities', + '- `data-export`: export user data as CSV', + '', + '## Impact', + '- code', + '', + ].join('\n') + ); + fs.writeFileSync(path.join(dir, 'design.md'), '# Design\n\nTechnical design.\n'); + fs.writeFileSync(path.join(dir, 'tasks.md'), '## 1. Implementation\n- [x] 1.1 Implement export\n'); + if (opts.specs) { + const sd = path.join(dir, 'specs', 'data-export'); + fs.mkdirSync(sd, { recursive: true }); + fs.writeFileSync( + path.join(sd, 'spec.md'), + [ + '## ADDED Requirements', + '', + '### Requirement: Data export', + 'The system SHALL export user data as CSV.', + '', + '#### Scenario: Successful export', + '- **WHEN** the user requests an export', + '- **THEN** a CSV file is produced', + '', + ].join('\n') + ); + } + return dir; + } + + const archiveDir = () => path.join(root, 'openspec', 'changes', 'archive'); + const mainSpec = () => path.join(root, 'openspec', 'specs', 'data-export', 'spec.md'); + + it('PR #1250: `instructions apply` exits 1 and reports specs missing with the spec-driven hint', async () => { + writeChange('add-export', { specs: false }); + + const r = await runCLI(['instructions', 'apply', '--change', 'add-export', '--json'], { + cwd: root, + env, + }); + + expect(r.exitCode).toBe(1); + const json = JSON.parse(r.stdout); + expect(json.state).toBe('blocked'); + expect(json.missingArtifacts).toContain('specs'); + expect(json.instruction).toContain('Delta specs must exist'); + }); + + it('issue #1212: `archive` refuses to silently drop specs — nothing is moved or synced', async () => { + const dir = writeChange('add-export', { specs: false }); + + const r = await runCLI(['archive', 'add-export', '--json', '--yes'], { cwd: root, env }); + + expect(r.exitCode).not.toBe(0); + // The change is NOT archived... + expect(fs.existsSync(dir)).toBe(true); + expect(fs.readdirSync(archiveDir()).filter((a) => a.includes('add-export'))).toEqual([]); + // ...and the main spec was NOT silently created/left stale. + expect(fs.existsSync(mainSpec())).toBe(false); + }); + + it('issue #1212: once the delta spec exists, apply is ready and archive syncs the main spec', async () => { + const dir = writeChange('add-export', { specs: true }); + + const apply = await runCLI(['instructions', 'apply', '--change', 'add-export', '--json'], { + cwd: root, + env, + }); + expect(apply.exitCode).toBe(0); + // Unblocked once the delta spec exists (ready to implement, or all_done when + // tasks are already complete) — the key point is it is no longer "blocked". + expect(['ready', 'all_done']).toContain(JSON.parse(apply.stdout).state); + + const archive = await runCLI(['archive', 'add-export', '--json', '--yes'], { cwd: root, env }); + expect(archive.exitCode).toBe(0); + // The main spec is now synced, and the change moved to the archive. + expect(fs.existsSync(mainSpec())).toBe(true); + expect(fs.readFileSync(mainSpec(), 'utf-8')).toContain('Requirement: Data export'); + expect(fs.existsSync(dir)).toBe(false); + }); + + it('recovery: `validate --archived` detects an already-archived change whose capability never synced', async () => { + // Simulate a change archived before the guard existed: it declared a + // capability but the main spec was never written. + const old = path.join(archiveDir(), '2026-01-01-legacy-export'); + fs.mkdirSync(old, { recursive: true }); + fs.writeFileSync( + path.join(old, 'proposal.md'), + '## Why\nlegacy\n\n## Capabilities\n### New Capabilities\n- `legacy-export`: never synced\n' + ); + + const r = await runCLI(['validate', '--archived'], { cwd: root, env }); + expect(r.exitCode).toBe(1); + expect(r.stdout + r.stderr).toContain('legacy-export'); + }); +}); From 6b1c91530c583b91fe41482507d2535d89f67859 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 14:39:08 -0500 Subject: [PATCH 08/10] fix(validate): restore proposal validation under the schema-aware gate (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit alfred-openspec found that `openspec validate ` 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 (#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) --- .../prevent-silent-spec-drop/proposal.md | 30 ++--------- src/commands/validate.ts | 26 ++++++---- src/core/validation/validator.ts | 34 +++++++++++- test/cli-e2e/store-lifecycle.test.ts | 2 +- test/commands/declared-store-fallback.test.ts | 2 +- test/commands/store-root-selection.test.ts | 2 +- test/commands/validate-schema-aware.test.ts | 52 ++++++++++++++++++- 7 files changed, 104 insertions(+), 44 deletions(-) diff --git a/openspec/changes/prevent-silent-spec-drop/proposal.md b/openspec/changes/prevent-silent-spec-drop/proposal.md index edc6ccaf35..d9a2765782 100644 --- a/openspec/changes/prevent-silent-spec-drop/proposal.md +++ b/openspec/changes/prevent-silent-spec-drop/proposal.md @@ -1,34 +1,10 @@ ## Why -A spec-driven change can complete the whole workflow — propose, implement, archive — while **silently leaving `openspec/specs/` stale**, with no error and no recovery path. It is the most-reported correctness defect in the tracker, and a deep read of the issue cluster (#1212, #1250, #656, #863, #164, #194, #997, #1260, #1222, #1264, #799, and the live Discord reports) shows it is not one bug but **one architectural fault expressed at four layers**: +A spec-driven change can finish the whole workflow — propose, implement, archive — while **silently leaving `openspec/specs/` stale**, with no error and no recovery path. It is the most-reported correctness defect in the tracker. -> Correctness-critical decisions live in **agent prompts**, not in deterministic CLI logic. Skills are asked to *judge* what a "delta spec" is, *decide* whether to sync, and *track* a loop termination — and agents (GPT-5.x, Claude, others) get these judgments wrong often enough to silently lose spec data. +The root fault is singular: **correctness-critical decisions live in agent prompts, not deterministic CLI logic.** The propose/ff loop stops at `applyRequires` (which excludes `specs`); the archive skill bypasses `openspec archive` (raw `mv` + agent-judged sync); and archive gates its own validation on `if (hasDeltaSpecs)` — skipping the check exactly when specs are missing. That hides three silent drop modes: **total** (no `specs/`), **partial** (declares `a,b,c`, ships only `a`), and **format** (a full `## Purpose` spec where a `## ADDED Requirements` delta belongs). The fix must not over-correct: schemas that legitimately produce no deltas (#997) must stay valid. -### The four layers - -1. **The propose/ff loop stops too early (and non-deterministically).** Both skills loop over artifacts and *"Stop when all `applyRequires` artifacts are done"* (`ff-change.ts:72`), reading `applyRequires` from the schema. For `spec-driven`, `applyRequires` is `[tasks]` — it excludes `specs` — so the loop can end before `openspec instructions specs` is ever called. Because it depends on the agent's path through "ready" artifacts, it is intermittent: the author of PR #1250 **could not reproduce it** on a greenfield project, while others hit it constantly. Anything intermittent in a prompt must be made deterministic in the CLI. (#1212, #1260) - -2. **The archive skill bypasses the CLI entirely (the keystone).** `/opsx:archive` does **not** call `openspec archive`. It *assesses sync state by agent judgment* (`archive-change.ts:58-60`: "If none exist, proceed without sync prompt"), optionally invokes a separate agent-driven `openspec-sync-specs`, then moves the change with a raw `mkdir` + `mv` (`archive-change.ts:86-88`). Every deterministic check and the deterministic delta-merge in `src/core/archive.ts` is **never executed for agent workflows**. The `opsx-archive-skill` spec encodes this: it mandates the skill *move* the directory and *prompt-or-skip* sync on judgment. (Filed as #656 and #863.) - -3. **Even when reached, the archive CLI skips its own validation.** `openspec validate` runs `validateChangeDeltaSpecs` unconditionally; `openspec archive` runs it only inside `if (hasDeltaSpecs)` (`archive.ts:282`), gating the check on the presence of the very thing whose absence is the bug. This hides **three** drop modes: - - **Total drop** — no `specs/` at all (#1212, #1222). - - **Partial drop** — proposal declares capabilities `a,b,c`, ships a delta for `a` only. - - **Format drop (the Discord case)** — `specs//spec.md` exists but as a **full** spec (`## Purpose`/`## Requirements`) instead of a **delta** spec (`## ADDED/MODIFIED/REMOVED/RENAMED Requirements`); the delta-header regex misses it and archive reports "No delta specs." (#164, #194, Discord) - -4. **It must not over-correct.** Some schemas legitimately have no delta specs (proposal-only / lighter schemas). A naive "always require deltas" breaks them (#997). The requirement must be **schema-graph-driven**, not hardcoded. - -### Reproductions (confirmed against current `main`) - -```console -# Total drop — validate and archive disagree -$ openspec validate silent-drop # exit 1: CHANGE_NO_DELTAS -$ openspec archive silent-drop --yes # exit 0: "archived"; no openspec/specs/ created - -# Format drop — a full (non-delta) spec file is silently ignored: archive.ts:274's -# /^## (ADDED|MODIFIED|REMOVED|RENAMED) Requirements/ never matches, hasDeltaSpecs=false. -``` - -And the keystone: `grep "openspec archive" archive-change.ts` → **no match**. The skill never calls the CLI. +Full analysis, reproductions, and decisions are in [`design.md`](./design.md). ## What Changes diff --git a/src/commands/validate.ts b/src/commands/validate.ts index 28c28ef947..e50621afef 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -190,10 +190,12 @@ export class ValidateCommand { } /** - * Validate a change with the schema-aware delta gate: only run delta-spec - * validation + declared-capability coverage when the change's schema produces - * delta specs (#997). The same gate is applied by `openspec archive`, so the - * two commands agree on whether delta specs are required. + * Validate a change: proposal validation (required sections + change-shape) + * ALWAYS runs, and the delta-spec validation + declared-capability coverage + * run only when the change's schema produces delta specs (#997). The same + * schema-aware delta gate is applied by `openspec archive`, so the two + * commands agree on whether delta specs are required — while a malformed or + * incomplete proposal fails validation under either schema. */ private async validateChange( validator: Validator, @@ -201,14 +203,16 @@ export class ValidateCommand { changeDir: string, strict: boolean ): Promise { - if (!requiresDelta(changeDir)) { - return { valid: true, issues: [], summary: { errors: 0, warnings: 0, info: 0 } }; + const reports = [await validator.validateChangeProposal(changeDir)]; + if (requiresDelta(changeDir)) { + reports.push( + ...(await Promise.all([ + validator.validateChangeDeltaSpecs(changeDir), + validator.validateChangeCapabilityCoverage(changeDir), + ])) + ); } - const [delta, coverage] = await Promise.all([ - validator.validateChangeDeltaSpecs(changeDir), - validator.validateChangeCapabilityCoverage(changeDir), - ]); - const issues = [...delta.issues, ...coverage.issues]; + const issues = reports.flatMap((r) => r.issues); const errors = issues.filter((i) => i.level === 'ERROR').length; const warnings = issues.filter((i) => i.level === 'WARNING').length; const info = issues.filter((i) => i.level === 'INFO').length; diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 8f5cba466d..93ce12604e 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -1,5 +1,5 @@ import { z, ZodError } from 'zod'; -import { readFileSync, promises as fs } from 'fs'; +import { readFileSync, existsSync, promises as fs } from 'fs'; import path from 'path'; import { SpecSchema, ChangeSchema, Spec, Change } from '../schemas/index.js'; import { MarkdownParser } from '../parsers/markdown-parser.js'; @@ -100,7 +100,37 @@ export class Validator { message: enriched, }); } - + + return this.createReport(issues); + } + + /** + * Validate a change's proposal (required sections + change-shape rules) WITHOUT + * enforcing the at-least-one-delta requirement. The delta requirement is gated + * on the change's schema by the caller (see `makeDeltaRequirementResolver`), so + * proposal validation runs for every change — including proposal-only schemas — + * while only spec-producing schemas are required to ship delta specs. + * + * If there is no proposal.md, returns a clean report (the proposal artifact's + * existence is governed elsewhere), so this never throws on absence. + */ + async validateChangeProposal(changeDir: string): Promise { + const proposalPath = path.join(changeDir, 'proposal.md'); + if (!existsSync(proposalPath)) { + return this.createReport([]); + } + const full = await this.validateChange(proposalPath); + // Keep proposal-content checks (required sections, Why length, What Changes); + // drop delta-quantity concerns. The at-least-one-delta requirement is applied + // separately and only for schemas that produce delta specs (#997); the + // "consider splitting >N deltas" guidance is advisory and belongs with the + // delta validation, not proposal validation, so it does not block here. + const issues = full.issues.filter((issue) => { + if (issue.message.includes(VALIDATION_MESSAGES.CHANGE_NO_DELTAS)) return false; + if (issue.message.includes(VALIDATION_MESSAGES.CHANGE_TOO_MANY_DELTAS)) return false; + if (issue.path === 'deltas' || issue.path.startsWith('deltas[')) return false; + return true; + }); return this.createReport(issues); } diff --git a/test/cli-e2e/store-lifecycle.test.ts b/test/cli-e2e/store-lifecycle.test.ts index 5fc6735432..e7e2d1f800 100644 --- a/test/cli-e2e/store-lifecycle.test.ts +++ b/test/cli-e2e/store-lifecycle.test.ts @@ -113,7 +113,7 @@ async function writeCompletedChangeArtifacts( '', '## Why', '', - 'Prove the standalone store lifecycle end to end.', + 'Prove the standalone store lifecycle works end to end across machines.', '', '## What Changes', '', diff --git a/test/commands/declared-store-fallback.test.ts b/test/commands/declared-store-fallback.test.ts index 75fca1a8af..c1f6eb764f 100644 --- a/test/commands/declared-store-fallback.test.ts +++ b/test/commands/declared-store-fallback.test.ts @@ -88,7 +88,7 @@ describe('declared store fallback (3.2)', () => { const changeDir = path.join(storeRoot, 'openspec', 'changes', 'billing-rework'); fs.writeFileSync( path.join(changeDir, 'proposal.md'), - '## Why\n\nBilling rework.\n\n## What Changes\n\n- **billing:** Rework billing\n' + '## Why\n\nBilling rework is needed to keep invoices correct and auditable over time.\n\n## What Changes\n\n- **billing:** Rework billing\n' ); const deltaDir = path.join(changeDir, 'specs', 'billing'); fs.mkdirSync(deltaDir, { recursive: true }); diff --git a/test/commands/store-root-selection.test.ts b/test/commands/store-root-selection.test.ts index 2079887d48..565178404c 100644 --- a/test/commands/store-root-selection.test.ts +++ b/test/commands/store-root-selection.test.ts @@ -94,7 +94,7 @@ describe('store root selection for normal commands', () => { fs.mkdirSync(changeDir, { recursive: true }); fs.writeFileSync( path.join(changeDir, 'proposal.md'), - '## Why\nBilling needs work.\n\n## What Changes\n- **billing:** Add billing\n' + '## Why\nBilling needs work to keep invoices correct and auditable over time.\n\n## What Changes\n- **billing:** Add billing\n' ); fs.writeFileSync( path.join(changeDir, 'tasks.md'), diff --git a/test/commands/validate-schema-aware.test.ts b/test/commands/validate-schema-aware.test.ts index 41225f168b..de31e2f1d7 100644 --- a/test/commands/validate-schema-aware.test.ts +++ b/test/commands/validate-schema-aware.test.ts @@ -33,7 +33,10 @@ describe('validate: schema-aware delta gate (#997) and archived-drift audit', () const d = path.join(changesDir, name); await fs.mkdir(d, { recursive: true }); if (schema) await fs.writeFile(path.join(d, '.openspec.yaml'), `schema: ${schema}\ncreated: 2026-06-29\n`); - await fs.writeFile(path.join(d, 'proposal.md'), '## Why\nlong enough why for validation here today.\n\n## What Changes\n- x\n'); + await fs.writeFile( + path.join(d, 'proposal.md'), + '## Why\nA sufficiently long why section so proposal validation passes for this test change.\n\n## What Changes\n- x\n' + ); return d; } @@ -50,6 +53,53 @@ describe('validate: schema-aware delta gate (#997) and archived-drift audit', () expect(r.stdout + r.stderr).toContain('at least one delta'); }); + // Proposal validation must run regardless of the schema-aware delta gate + // (regression for the validate-skips-proposal bug found in review). + async function changeWithProposal( + name: string, + schema: string | null, + proposal: string, + opts: { delta?: boolean } = {} + ) { + const d = path.join(changesDir, name); + await fs.mkdir(d, { recursive: true }); + if (schema) await fs.writeFile(path.join(d, '.openspec.yaml'), `schema: ${schema}\ncreated: 2026-06-29\n`); + await fs.writeFile(path.join(d, 'proposal.md'), proposal); + if (opts.delta) { + const sd = path.join(d, 'specs', 'cap'); + await fs.mkdir(sd, { recursive: true }); + await fs.writeFile( + path.join(sd, 'spec.md'), + '## ADDED Requirements\n\n### Requirement: Cap\nThe system SHALL cap.\n\n#### Scenario: s\n- **WHEN** a\n- **THEN** b\n' + ); + } + return d; + } + + it('invalid proposal (missing ## Why) + valid delta still FAILS validate', async () => { + await changeWithProposal('bad-spec', 'spec-driven', '## What Changes\n- missing why section\n', { + delta: true, + }); + const r = await runCLI(['validate', 'bad-spec'], { cwd: dir }); + expect(r.exitCode).toBe(1); + }); + + it('proposal-only schema + invalid proposal still FAILS validate', async () => { + await changeWithProposal('bad-prop', 'proposal-only', '## What Changes\n- missing why\n'); + const r = await runCLI(['validate', 'bad-prop'], { cwd: dir }); + expect(r.exitCode).toBe(1); + }); + + it('proposal-only schema + valid proposal + no specs PASSES validate', async () => { + await changeWithProposal( + 'ok-prop', + 'proposal-only', + '## Why\nA valid proposal-only change with a sufficiently long why section here.\n\n## What Changes\n- notes\n' + ); + const r = await runCLI(['validate', 'ok-prop'], { cwd: dir }); + expect(r.exitCode).toBe(0); + }); + it('bulk validate applies the gate per change (proposal-only ok, spec-driven fails)', async () => { await change('lighter', 'proposal-only'); await change('strict', 'spec-driven'); From 12cf1a06fce55de33cba2aa90525b7bede0939fc Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 2 Jul 2026 17:21:08 -0500 Subject: [PATCH 09/10] fix(archive): block proposal validation errors in text and --json modes (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 --- .../specs/cli-archive/spec.md | 11 ++++- .../changes/prevent-silent-spec-drop/tasks.md | 2 + src/core/archive.ts | 37 +++++++++------ test/core/archive-spec-gate.test.ts | 45 +++++++++++++++++++ test/core/archive.test.ts | 3 ++ 5 files changed, 83 insertions(+), 15 deletions(-) diff --git a/openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md b/openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md index 4de879df0f..40f60bbc99 100644 --- a/openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md +++ b/openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md @@ -2,7 +2,7 @@ ### Requirement: Archive Validation -The archive command SHALL validate changes before applying them to ensure data integrity. When validation is enabled, the command SHALL run delta-spec validation and declared-capability coverage consistently with `openspec validate` — that is, whenever the user has not opted out of spec updates via `--skip-specs` — rather than only when delta specs already exist. A change that has no delta specs SHALL be blocked with the same `CHANGE_NO_DELTAS` error that `openspec validate` reports; a change whose spec file is present but not in delta format SHALL be blocked with the same "No delta sections found" error; and a change that declares capabilities it has not delivered as delta specs SHALL be blocked with the same coverage errors — instead of being silently archived with missing, malformed, or partial spec updates. This delta requirement SHALL apply only when the change's schema graph includes a `specs` artifact, so schemas that legitimately have no delta specs are not forced to provide them. +The archive command SHALL validate changes before applying them to ensure data integrity. When validation is enabled, the command SHALL run delta-spec validation and declared-capability coverage consistently with `openspec validate` — that is, whenever the user has not opted out of spec updates via `--skip-specs` — rather than only when delta specs already exist. A change that has no delta specs SHALL be blocked with the same `CHANGE_NO_DELTAS` error that `openspec validate` reports; a change whose spec file is present but not in delta format SHALL be blocked with the same "No delta sections found" error; and a change that declares capabilities it has not delivered as delta specs SHALL be blocked with the same coverage errors — instead of being silently archived with missing, malformed, or partial spec updates. This delta requirement SHALL apply only when the change's schema graph includes a `specs` artifact, so schemas that legitimately have no delta specs are not forced to provide them. The command SHALL also run the same proposal validation as `openspec validate` (required sections + change-shape rules, minus the schema-gated delta requirement) and block on proposal errors in both text and `--json` modes, so archive never syncs or moves a change that validate rejects; proposal warnings remain informative. #### Scenario: Pre-archive validation @@ -56,6 +56,15 @@ The archive command SHALL validate changes before applying them to ensure data i - **THEN** `openspec archive change-name` (with validation enabled) blocks on the same errors - **AND** does not archive the change +#### Scenario: Malformed proposal blocks archive + +- **WHEN** executing `openspec archive change-name` without `--no-validate` +- **AND** `changes/change-name/proposal.md` fails the proposal validation that `openspec validate` applies (e.g. missing `## Why`), even though every delta spec is valid +- **THEN** archive validation fails with the same proposal errors +- **AND** the archive is aborted with a non-zero exit code in both text and `--json` modes +- **AND** the change is not moved to the archive directory +- **AND** this applies under proposal-only schemas too, since proposal validation is not gated on the schema producing delta specs + #### Scenario: Force archive without validation - **WHEN** executing `openspec archive change-name --no-validate` diff --git a/openspec/changes/prevent-silent-spec-drop/tasks.md b/openspec/changes/prevent-silent-spec-drop/tasks.md index f05cec33e6..e13cc69536 100644 --- a/openspec/changes/prevent-silent-spec-drop/tasks.md +++ b/openspec/changes/prevent-silent-spec-drop/tasks.md @@ -13,6 +13,7 @@ - [x] 1.6 In `src/core/archive.ts`, replace the `if (hasDeltaSpecs)` gate (~lines 263-297) with `if (!options.skipSpecs && schemaProducesDeltaSpecs(schema))`, using the SAME helper as validate; run `validateChangeDeltaSpecs` + coverage. Confirm `--yes` does not bypass this gate (only confirmation prompts) - [x] 1.7 Route failures through the existing `hasValidationErrors → ArchiveBlockedError` (JSON) / `return null` + exit-1 (text) path; remove the dead `hasDeltaSpecs` probe - [x] 1.8 Confirm the format-drop case (present non-delta spec) and total/partial drops all block; confirm a proposal-only schema (#997) still archives +- [x] 1.8b Run the same proposal validation as `openspec validate` (`validateChangeProposal`) in archive and block on ERROR-level proposal issues in BOTH text and `--json` modes, so archive never syncs/moves a change validate rejects; warnings stay informative (review: validate/archive proposal parity) ### 1d. Apply enforcement + schema gate - [x] 1.9 In `src/commands/workflow/instructions.ts`, set `process.exitCode = 1` when apply `state === 'blocked'` (text and JSON; payload first) @@ -61,6 +62,7 @@ - [x] 6.14 Archived-drift audit: archived change with an un-synced declared capability → flagged; pre-contract archive → not falsely flagged - [x] 6.15 Extraction determinism fixtures; cross-platform `path.join()` - [x] 6.16 E2E regression `test/cli-e2e/issue-1212-spec-drop.test.ts`: reproduces #1212 (apply blocks, archive refuses, audit detects) and asserts #1250 behaviors (apply exit 1 + spec-driven hint) against the real CLI +- [x] 6.17 Proposal parity: malformed proposal (missing `## Why`) + valid delta → archive blocked in text AND `--json` modes (`archive_validation_failed`, nothing moved); proposal-only schema + malformed proposal → blocked; valid proposals unaffected ## 7. Release diff --git a/src/core/archive.ts b/src/core/archive.ts index 78c7458102..636db74076 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -242,22 +242,30 @@ export class ArchiveCommand { const validator = new Validator(); let hasValidationErrors = false; - // Validate proposal.md (informative only; human mode prints warnings) + // Validate proposal.md with the same rules as `openspec validate` + // (required sections + change-shape, minus the schema-gated delta + // requirement). Errors block the archive in both text and JSON mode so + // archive never syncs/moves a change that validate rejects; warnings + // stay informative in human mode. + const proposalReport = await validator.validateChangeProposal(changeDir); + const proposalErrors = proposalReport.issues.filter((issue) => issue.level === 'ERROR'); + if (proposalErrors.length > 0) { + hasValidationErrors = true; + if (!json) { + console.log(chalk.red(`\nValidation errors in proposal.md:`)); + for (const issue of proposalErrors) { + console.log(chalk.red(` ✗ ${issue.message}`)); + } + } + } if (!json) { - const changeFile = path.join(changeDir, 'proposal.md'); - try { - await fs.access(changeFile); - const changeReport = await validator.validateChange(changeFile); - // Proposal validation is informative only (do not block archive) - if (!changeReport.valid) { - console.log(chalk.yellow(`\nProposal warnings in proposal.md (non-blocking):`)); - for (const issue of changeReport.issues) { - const symbol = issue.level === 'ERROR' ? '⚠' : (issue.level === 'WARNING' ? '⚠' : 'ℹ'); - console.log(chalk.yellow(` ${symbol} ${issue.message}`)); - } + const proposalWarnings = proposalReport.issues.filter((issue) => issue.level !== 'ERROR'); + if (proposalWarnings.length > 0) { + console.log(chalk.yellow(`\nProposal warnings in proposal.md (non-blocking):`)); + for (const issue of proposalWarnings) { + const symbol = issue.level === 'WARNING' ? '⚠' : 'ℹ'; + console.log(chalk.yellow(` ${symbol} ${issue.message}`)); } - } catch { - // Change file doesn't exist, skip validation } } @@ -299,6 +307,7 @@ export class ArchiveCommand { } console.log(chalk.red('\nValidation failed. Please fix the errors before archiving.')); console.log(chalk.yellow('To skip validation (not recommended), use --no-validate flag.')); + process.exitCode = 1; return null; } } else if (json) { diff --git a/test/core/archive-spec-gate.test.ts b/test/core/archive-spec-gate.test.ts index bb02cb6a3e..9e1ae2279a 100644 --- a/test/core/archive-spec-gate.test.ts +++ b/test/core/archive-spec-gate.test.ts @@ -27,6 +27,8 @@ describe('archive spec-drop gate', () => { console.log = origLog; if (origXdg === undefined) delete process.env.XDG_DATA_HOME; else process.env.XDG_DATA_HOME = origXdg; + // Blocked archives set process.exitCode = 1; reset so the runner exits clean. + process.exitCode = undefined; vi.clearAllMocks(); await fs.rm(tempDir, { recursive: true, force: true }); }); @@ -109,6 +111,32 @@ describe('archive spec-drop gate', () => { expect(await isArchived('good')).toBe(true); }); + it('blocks a malformed proposal (missing ## Why) even with a valid delta spec', async () => { + const dir = await makeChange('badprop', { caps: ['cap'], deltaSpecs: ['cap'] }); + await fs.writeFile( + path.join(dir, 'proposal.md'), + '## What Changes\n- stuff\n\n## Capabilities\n### New Capabilities\n- `cap`: cap\n' + ); + await cmd.execute('badprop', { yes: true }); + expect(process.exitCode).toBe(1); + expect(await stillActive(dir)).toBe(true); + expect(await isArchived('badprop')).toBe(false); + }); + + it('blocks a malformed proposal in --json mode with archive_validation_failed', async () => { + const dir = await makeChange('badjson', { caps: ['cap'], deltaSpecs: ['cap'] }); + await fs.writeFile( + path.join(dir, 'proposal.md'), + '## What Changes\n- stuff\n\n## Capabilities\n### New Capabilities\n- `cap`: cap\n' + ); + await cmd.execute('badjson', { yes: true, json: true }); + expect(process.exitCode).toBe(1); + const out = (console.log as ReturnType).mock.calls.map((c) => c[0]).join('\n'); + expect(out).toContain('archive_validation_failed'); + expect(await stillActive(dir)).toBe(true); + expect(await isArchived('badjson')).toBe(false); + }); + it('proposal-only schema (no specs artifact) archives without specs', async () => { // project-local schema with no artifact generating under specs/ const schemaDir = path.join(tempDir, 'openspec', 'schemas', 'proposal-only'); @@ -126,4 +154,21 @@ describe('archive spec-drop gate', () => { expect(await isArchived('lighter')).toBe(true); expect(await stillActive(dir)).toBe(false); }); + + it('proposal-only schema with a malformed proposal is still blocked', async () => { + const schemaDir = path.join(tempDir, 'openspec', 'schemas', 'proposal-only'); + await fs.mkdir(path.join(schemaDir, 'templates'), { recursive: true }); + await fs.writeFile( + path.join(schemaDir, 'schema.yaml'), + 'name: proposal-only\nversion: 1\nartifacts:\n - id: proposal\n generates: proposal.md\n description: p\n template: proposal.md\n requires: []\n' + ); + await fs.writeFile(path.join(schemaDir, 'templates', 'proposal.md'), 'tpl'); + const dir = path.join(changesDir(), 'badlite'); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, '.openspec.yaml'), 'schema: proposal-only\ncreated: 2026-06-29\n'); + await fs.writeFile(path.join(dir, 'proposal.md'), '## What Changes\n- x\n'); + await cmd.execute('badlite', { yes: true }); + expect(await stillActive(dir)).toBe(true); + expect(await isArchived('badlite')).toBe(false); + }); }); diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 6b0905cba6..ab83b38ffd 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -51,6 +51,9 @@ describe('ArchiveCommand', () => { process.env.XDG_DATA_HOME = originalXdgDataHome; } + // Blocked archives set process.exitCode = 1; reset so the runner exits clean. + process.exitCode = undefined; + // Clear mocks vi.clearAllMocks(); From 7a4bfada0098d8b328be736d4d352420f409b622 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 2 Jul 2026 17:31:19 -0500 Subject: [PATCH 10/10] test(e2e): write a valid proposal in capstone journey 3 so the archive gate passes Journey 3 stubbed every non-spec artifact as '# / 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 --- test/cli-e2e/capstone-journeys.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/cli-e2e/capstone-journeys.test.ts b/test/cli-e2e/capstone-journeys.test.ts index 5b411cbe2b..df89e6222c 100644 --- a/test/cli-e2e/capstone-journeys.test.ts +++ b/test/cli-e2e/capstone-journeys.test.ts @@ -153,7 +153,9 @@ describe('capstone persona journeys (6.1)', () => { target, artifact.id === 'specs' ? '## ADDED Requirements\n\n### Requirement: Rate limits\nThe API SHALL rate-limit.\n\n#### Scenario: Limit hit\n- **WHEN** the limit is exceeded\n- **THEN** requests are rejected\n' - : `# ${artifact.id}\n\nDone.\n` + : artifact.id === 'proposal' + ? '## Why\n\nRate limiting protects the API from abusive clients and keeps latency predictable for everyone else.\n\n## What Changes\n\n- Add rate limits to the API.\n' + : `# ${artifact.id}\n\nDone.\n` ); }