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/.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..d9a2765782 --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/proposal.md @@ -0,0 +1,86 @@ +## Why + +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. + +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. + +Full analysis, reproductions, and decisions are in [`design.md`](./design.md). + +## 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). 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 +- [#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) — 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 + +- [#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..40f60bbc99 --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/specs/cli-archive/spec.md @@ -0,0 +1,90 @@ +## 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. 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 + +- **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: 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` +- **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..e13cc69536 --- /dev/null +++ b/openspec/changes/prevent-silent-spec-drop/tasks.md @@ -0,0 +1,69 @@ +## 1. Deterministic CLI gate (the guarantee — do first) + +### 1a. Capability extraction +- [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 +- [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) +- [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) +- [x] 1.10 Set `apply.requires: [specs, tasks]` in `schemas/spec-driven/schema.yaml`; update tests asserting the old `[tasks]` gate + +- [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 + +- [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) + +- [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 +- [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) + +- [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) + +- [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 + +- [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()` +- [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 + +- [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) diff --git a/schemas/spec-driven/schema.yaml b/schemas/spec-driven/schema.yaml index 45f61e222b..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 @@ -146,7 +157,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/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 4690f6f6b5..e50621afef 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -1,6 +1,10 @@ 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, toRootOutput, @@ -18,6 +22,7 @@ interface ExecuteOptions { all?: boolean; changes?: boolean; specs?: boolean; + archived?: boolean; type?: string; strict?: boolean; json?: boolean; @@ -45,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, { @@ -178,12 +189,123 @@ export class ValidateCommand { await this.validateByType(root, type, itemName, opts); } + /** + * 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, + requiresDelta: (changeDir: string) => boolean, + changeDir: string, + strict: boolean + ): Promise { + const reports = [await validator.validateChangeProposal(changeDir)]; + if (requiresDelta(changeDir)) { + reports.push( + ...(await Promise.all([ + validator.validateChangeDeltaSpecs(changeDir), + validator.validateChangeCapabilityCoverage(changeDir), + ])) + ); + } + 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; + const valid = strict ? errors === 0 && warnings === 0 : errors === 0; + 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') { 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 +365,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..636db74076 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 { @@ -241,56 +242,56 @@ export class ArchiveCommand { const validator = new Validator(); let hasValidationErrors = false; - // Validate proposal.md (informative only; human mode prints warnings) - 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}`)); - } + // 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}`)); } - } catch { - // Change file doesn't exist, skip validation } } - - // 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 {} + if (!json) { + 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 {} - 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}`)); } } } @@ -306,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/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/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, 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/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/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..93ce12604e 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -1,9 +1,10 @@ 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'; 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, @@ -99,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); } @@ -273,6 +304,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/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` ); } 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'); + }); +}); 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/artifact-workflow.test.ts b/test/commands/artifact-workflow.test.ts index a286422a82..ea9473bab1 100644 --- a/test/commands/artifact-workflow.test.ts +++ b/test/commands/artifact-workflow.test.ts @@ -434,15 +434,30 @@ 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('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 () => { @@ -624,7 +639,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/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 new file mode 100644 index 0000000000..de31e2f1d7 --- /dev/null +++ b/test/commands/validate-schema-aware.test.ts @@ -0,0 +1,130 @@ +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\nA sufficiently long why section so proposal validation passes for this test change.\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'); + }); + + // 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'); + 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..9e1ae2279a --- /dev/null +++ b/test/core/archive-spec-gate.test.ts @@ -0,0 +1,174 @@ +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; + // 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 }); + }); + + 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('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'); + 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); + }); + + 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 977508929c..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(); @@ -74,7 +77,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 +100,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 +286,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 +296,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 +315,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 +407,14 @@ The system will log all events. await fs.mkdir(changeSpecDir, { recursive: true }); // Create valid spec in change - const specContent = `# Test Capability Spec - -## Purpose -This is a test capability specification. - -## Requirements + const specContent = `## ADDED 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 +806,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 +839,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/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/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 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([]); + }); +}); 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