From fe4de02c92912a046bfd515f507e17b808f859f5 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 7 Sep 2026 08:56:19 -0500 Subject: [PATCH 1/6] feat(sync): fold delta specs without archiving the change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `archive` folds a change's deltas into `specs/` and moves the change directory in one command, so the fold can only happen at the moment the change is finished. On a team that reviews before merging, that moment is after the pull request closes — which leaves CI nothing it can assert during review. The only property expressible today is "nothing is left unarchived", and that is violated by design for the whole life of every open PR (#1683). Separate the two, additively: - `openspec sync [change]` folds delta specs into the main specs and moves nothing. The merge engine already supported this — re-applying a folded delta is the no-op `specs-apply` calls the early-sync pattern — so `archive` afterwards behaves exactly as before. - `openspec sync --check` asserts `shipped => folded` over the working tree. A change that has not claimed to be shipped passes for free, so green is the resting state and red means a real mistake. It reads only files on disk, so pre-commit, pre-push and CI run one command and agree. - `status: proposed | shipped` becomes an optional field in a change's `.openspec.yaml`. Absent means proposed, which is what a change under `changes/` has always meant; nothing writes it on the author's behalf. - `openspec sync --ship` sets the field and folds in one working-tree diff, so no intermediate commit claims a change is shipped while the specs say otherwise. - `openspec list --status ` filters by the field. The lifecycle column and the JSON `lifecycle` key appear only once some change in the root declares one. Folded-ness is decided by running the merge builder and seeing it apply zero operations — the same predicate archive uses to decide it has nothing to write. Not a byte comparison against the rebuilt output: the rebuild normalizes blank lines, so a hand-formatted main spec would compare unequal while being perfectly in sync. Sharing archive's own predicate is what stops the checker and the doer from drifting (#1112). Two deliberate limits keep this additive rather than a second lifecycle. Sync never deletes a spec: retiring a capability stays with `archive`, behind the `retire_capabilities` marker and its rollback-safe deletion, and sync reports the case and names archive. Sync never examines archived changes: their deltas are history and later changes supersede them, so re-applying a months-old delta over everything that followed is a merge conflict, not a drift check. Metadata that mentions `status` but cannot be honored is reported as undetermined rather than rounded to proposed — the fail-open direction would let a change that declared itself shipped, and then had its metadata broken, silently stop being checked. Two archive helpers are exported so sync enumerates the same active changes and asks the same retirement question; archive's behavior is unchanged. Credit: the diagnosis, the `shipped => folded` framing, and the argument that a checker reimplementing the doer eventually disagrees with it are from Matan Bendix Shenhav's proposal in #1683. Co-Authored-By: Claude Opus 5 --- skills/openspec-apply-change/SKILL.md | 2 +- skills/openspec-archive-change/SKILL.md | 2 +- skills/openspec-bulk-archive-change/SKILL.md | 2 +- skills/openspec-continue-change/SKILL.md | 2 +- skills/openspec-explore/SKILL.md | 2 +- skills/openspec-ff-change/SKILL.md | 2 +- skills/openspec-new-change/SKILL.md | 2 +- skills/openspec-onboard/SKILL.md | 2 +- skills/openspec-propose/SKILL.md | 2 +- skills/openspec-sync-specs/SKILL.md | 2 +- skills/openspec-update-change/SKILL.md | 2 +- skills/openspec-verify-change/SKILL.md | 2 +- src/cli/index.ts | 32 +- src/core/archive.ts | 7 +- src/core/change-metadata/schema.ts | 12 + src/core/completions/command-registry.ts | 34 + src/core/list.ts | 47 +- src/core/sync.ts | 735 ++++++++++++++++++ .../templates/workflows/store-selection.ts | 2 +- src/utils/change-metadata.ts | 173 +++++ .../core/completions/command-registry.test.ts | 1 + test/core/list.test.ts | 77 ++ test/core/sync.test.ts | 481 ++++++++++++ .../templates/skill-templates-parity.test.ts | 72 +- 24 files changed, 1641 insertions(+), 56 deletions(-) create mode 100644 src/core/sync.ts create mode 100644 test/core/sync.test.ts diff --git a/skills/openspec-apply-change/SKILL.md b/skills/openspec-apply-change/SKILL.md index 098f63fecb..21bd424e2c 100644 --- a/skills/openspec-apply-change/SKILL.md +++ b/skills/openspec-apply-change/SKILL.md @@ -11,7 +11,7 @@ metadata: Implement tasks from an OpenSpec change. -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. **Input**: Optionally specify a change name (e.g., `/openspec-apply-change add-auth`). If omitted, check if it can be inferred from conversation context. If vague or ambiguous you MUST prompt for available changes. diff --git a/skills/openspec-archive-change/SKILL.md b/skills/openspec-archive-change/SKILL.md index 5f34ed53a7..0fe97ea943 100644 --- a/skills/openspec-archive-change/SKILL.md +++ b/skills/openspec-archive-change/SKILL.md @@ -11,7 +11,7 @@ metadata: Archive a completed change in the experimental workflow. -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. `` is the spec directory relative to `specs/` (for example, `user-auth` or `identity/user-auth`). Preserve the full path from each delta spec when resolving its main spec. diff --git a/skills/openspec-bulk-archive-change/SKILL.md b/skills/openspec-bulk-archive-change/SKILL.md index 252e1dd155..f210e1214d 100644 --- a/skills/openspec-bulk-archive-change/SKILL.md +++ b/skills/openspec-bulk-archive-change/SKILL.md @@ -13,7 +13,7 @@ Archive multiple completed changes in a single operation. This skill allows you to batch-archive changes, handling spec conflicts intelligently by checking the codebase to determine what's actually implemented. -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. `` is the spec directory relative to `specs/` (for example, `user-auth` or `identity/user-auth`). Preserve the full path from each delta spec when resolving its main spec. diff --git a/skills/openspec-continue-change/SKILL.md b/skills/openspec-continue-change/SKILL.md index 5991b06891..30e2cf5f05 100644 --- a/skills/openspec-continue-change/SKILL.md +++ b/skills/openspec-continue-change/SKILL.md @@ -11,7 +11,7 @@ metadata: Continue working on a change by creating the next artifact. -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. **Input**: Optionally specify a change name. If omitted, check if it can be inferred from conversation context. If vague or ambiguous you MUST prompt for available changes. diff --git a/skills/openspec-explore/SKILL.md b/skills/openspec-explore/SKILL.md index b597796fb1..fbe6ba0494 100644 --- a/skills/openspec-explore/SKILL.md +++ b/skills/openspec-explore/SKILL.md @@ -15,7 +15,7 @@ Enter explore mode. Think deeply. Visualize freely. Follow the conversation wher **This is a stance, not a workflow.** There are no fixed steps, no required sequence, no mandatory outputs. You're a thinking partner helping the user explore. -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. --- diff --git a/skills/openspec-ff-change/SKILL.md b/skills/openspec-ff-change/SKILL.md index 72a95620bb..986e7d2905 100644 --- a/skills/openspec-ff-change/SKILL.md +++ b/skills/openspec-ff-change/SKILL.md @@ -11,7 +11,7 @@ metadata: Fast-forward through artifact creation - generate everything needed to start implementation in one go. -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. **Input**: The user's request should include a change name (kebab-case) OR a description of what they want to build. diff --git a/skills/openspec-new-change/SKILL.md b/skills/openspec-new-change/SKILL.md index 9aea11d391..35f9b7b5e9 100644 --- a/skills/openspec-new-change/SKILL.md +++ b/skills/openspec-new-change/SKILL.md @@ -11,7 +11,7 @@ metadata: Start a new change using the experimental artifact-driven approach. -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. **Input**: The user's request should include a change name (kebab-case) OR a description of what they want to build. diff --git a/skills/openspec-onboard/SKILL.md b/skills/openspec-onboard/SKILL.md index fb3f13bec7..aefee7f0c0 100644 --- a/skills/openspec-onboard/SKILL.md +++ b/skills/openspec-onboard/SKILL.md @@ -11,7 +11,7 @@ metadata: Guide the user through their first complete OpenSpec workflow cycle. This is a teaching experience—you'll do real work in their codebase while explaining each step. -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. --- diff --git a/skills/openspec-propose/SKILL.md b/skills/openspec-propose/SKILL.md index 2d5709dfe1..fd7757f007 100644 --- a/skills/openspec-propose/SKILL.md +++ b/skills/openspec-propose/SKILL.md @@ -25,7 +25,7 @@ When the user is ready to implement, they must start the apply workflow explicit --- -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. **Input**: The user's request should include a change name (kebab-case) OR a description of what they want to build. diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index d12d56b857..ac6b8de945 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -13,7 +13,7 @@ Sync delta specs from a change to main specs. This is an **agent-driven** operation - you will read delta specs and directly edit main specs to apply the changes. This allows intelligent merging (e.g., adding a scenario without copying the entire requirement). -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. `` is the spec directory relative to `specs/` (for example, `user-auth` or `identity/user-auth`). Preserve the full path from each delta spec when resolving its main spec. diff --git a/skills/openspec-update-change/SKILL.md b/skills/openspec-update-change/SKILL.md index 24c9f88367..6bdaec2ada 100644 --- a/skills/openspec-update-change/SKILL.md +++ b/skills/openspec-update-change/SKILL.md @@ -11,7 +11,7 @@ metadata: Revise a change's existing planning artifacts and keep them coherent. Never edit code. -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. **Input**: Optionally specify a change name. If omitted, check if it can be inferred from conversation context. If vague or ambiguous you MUST prompt for available changes. diff --git a/skills/openspec-verify-change/SKILL.md b/skills/openspec-verify-change/SKILL.md index 2165a6a910..e32eacbd31 100644 --- a/skills/openspec-verify-change/SKILL.md +++ b/skills/openspec-verify-change/SKILL.md @@ -11,7 +11,7 @@ metadata: Verify that an implementation matches the change artifacts (specs, tasks, design). -**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. +**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run `openspec store list --json` to discover registered store ids, then pass `--store ` on the commands that read or write specs and changes (`new change`, `status`, `instructions`, `list`, `show`, `validate`, `sync`, `archive`, `doctor`, `context`, `schemas`, `view`). Once selected, treat `--store ` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run `openspec status --change "" --json --store ""`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local `openspec/` root. **Input**: Optionally specify a change name. If omitted, check if it can be inferred from conversation context. If vague or ambiguous you MUST prompt for available changes. diff --git a/src/cli/index.ts b/src/cli/index.ts index 75324cfa38..f7bbbf8ecc 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -19,6 +19,7 @@ import { } from '../core/version-check.js'; import { ListCommand } from '../core/list.js'; import { ArchiveCommand, type ArchiveOptions } from '../core/archive.js'; +import { SyncCommand, type SyncOptions } from '../core/sync.js'; import { ViewCommand } from '../core/view.js'; import { resolveRootForCommand, toRootOutput } from '../core/root-selection.js'; import { registerSpecCommand } from '../commands/spec.js'; @@ -356,10 +357,11 @@ program .option('--specs', 'List specs instead of changes') .option('--changes', 'List changes explicitly (default)') .option('--sort ', 'Sort order: "recent" (default) or "name"', 'recent') + .option('--status ', 'Only list changes in this lifecycle state: proposed|shipped') .option('--json', 'Output as JSON (for programmatic use)') .option('--store ', STORE_OPTION_DESCRIPTION) .addOption(hiddenStorePathOption()) - .action(async (options?: { specs?: boolean; changes?: boolean; sort?: string; json?: boolean; store?: string; storePath?: string }) => { + .action(async (options?: { specs?: boolean; changes?: boolean; sort?: string; status?: string; json?: boolean; store?: string; storePath?: string }) => { try { const root = await resolveRootForCommand(options ?? {}, { json: options?.json, @@ -374,9 +376,17 @@ program const listCommand = new ListCommand(); const mode: 'changes' | 'specs' = options?.specs ? 'specs' : 'changes'; const sort = options?.sort === 'name' ? 'name' : 'recent'; + // Rejected rather than ignored: a typo would otherwise silently list + // everything, which reads as "no change has that state". + if (options?.status !== undefined && options.status !== 'proposed' && options.status !== 'shipped') { + throw new Error( + `Unknown --status '${options.status}'. Use 'proposed' or 'shipped'.` + ); + } await listCommand.execute(root.path, mode, { sort, json: options?.json, + ...(options?.status ? { status: options.status as 'proposed' | 'shipped' } : {}), ...(options?.json ? { root: toRootOutput(root) } : {}), }); } catch (error) { @@ -495,6 +505,26 @@ program } }); +program + .command('sync [change-name]') + .description('Fold a change\'s spec deltas into the main specs without archiving it') + .option('--check', 'Report shipped changes whose deltas are not in the main specs; write nothing') + .option('--ship', 'Mark the named change `status: shipped` before folding it') + .option('-y, --yes', 'Sync even when the change still has incomplete tasks') + .option('--no-validate', 'Skip validation (not recommended)') + .option('--json', 'Output as JSON (for hooks and CI)') + .option('--store ', STORE_OPTION_DESCRIPTION) + .addOption(hiddenStorePathOption()) + .action(async (changeName?: string, options?: SyncOptions) => { + try { + const syncCommand = new SyncCommand(); + await syncCommand.execute(changeName, options); + } catch (error) { + failWithError(error, { enabled: options?.json, fallbackCode: 'sync_error' }); + process.exit(1); + } + }); + registerSpecCommand(program); registerConfigCommand(program); registerSchemaCommand(program); diff --git a/src/core/archive.ts b/src/core/archive.ts index 888a6135a6..eb76bdfadc 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -158,7 +158,12 @@ async function decideSpecOutcome( return built.counts.removed > 0 ? 'retire' : 'write'; } -async function listActiveChangeNames(changesDir: string): Promise { +/** + * Every change directory directly under `changes/`, excluding the archive. + * Exported so `openspec sync` enumerates the same set archive does - the two + * commands must never disagree about which changes are active. + */ +export async function listActiveChangeNames(changesDir: string): Promise { try { const entries = await fs.readdir(changesDir, { withFileTypes: true }); return entries diff --git a/src/core/change-metadata/schema.ts b/src/core/change-metadata/schema.ts index 3644160052..7d706b0ec1 100644 --- a/src/core/change-metadata/schema.ts +++ b/src/core/change-metadata/schema.ts @@ -46,6 +46,18 @@ export const ChangeMetadataSchema = z.object({ // tree - only from git - so it is the author's call, not an inference from the // shape of a delta. retire_capabilities: z.boolean().optional(), + // Where the change sits in its own lifecycle, as data rather than as a + // directory position. Optional and absent by default: a change with no + // `status` is `proposed`, which is what every change in `changes/` has always + // meant. Declaring `shipped` says "these deltas belong in `specs/` now", and + // is what `openspec sync --check` gates on - so a proposed change passes the + // gate for free and red means a real mistake, instead of a check that is red + // for the whole life of an open PR (#1683). + // + // Nothing writes this field on its own: `openspec new change` does not emit + // it, and `archive` neither reads nor stamps it. A project that never opts in + // never sees it. + status: z.enum(['proposed', 'shipped']).optional(), }); export type ChangeMetadata = z.infer; diff --git a/src/core/completions/command-registry.ts b/src/core/completions/command-registry.ts index 95e550a7ff..d9292cec01 100644 --- a/src/core/completions/command-registry.ts +++ b/src/core/completions/command-registry.ts @@ -73,6 +73,12 @@ export const COMMAND_REGISTRY: CommandDefinition[] = [ takesValue: true, values: ['recent', 'name'], }, + { + name: 'status', + description: 'Only list changes in this lifecycle state', + takesValue: true, + values: ['proposed', 'shipped'], + }, COMMON_FLAGS.json, COMMON_FLAGS.store, ], @@ -191,6 +197,34 @@ export const COMMAND_REGISTRY: CommandDefinition[] = [ COMMON_FLAGS.store, ], }, + { + name: 'sync', + description: "Fold a change's spec deltas into the main specs without archiving it", + acceptsPositional: true, + positionalType: 'change-id', + positionals: [{ name: 'change-name', type: 'change-id', optional: true }], + flags: [ + { + name: 'check', + description: 'Report shipped changes whose deltas are not in the main specs; write nothing', + }, + { + name: 'ship', + description: 'Mark the named change `status: shipped` before folding it', + }, + { + name: 'yes', + short: 'y', + description: 'Sync even when the change still has incomplete tasks', + }, + { + name: 'no-validate', + description: 'Skip validation (not recommended)', + }, + COMMON_FLAGS.json, + COMMON_FLAGS.store, + ], + }, { name: 'status', description: 'Display artifact completion status for a change', diff --git a/src/core/list.ts b/src/core/list.ts index f6b6faf2f8..eb608136ca 100644 --- a/src/core/list.ts +++ b/src/core/list.ts @@ -5,18 +5,27 @@ import { readFileSync, type Dirent } from 'fs'; import { MarkdownParser } from './parsers/markdown-parser.js'; import type { RootOutput } from './root-selection.js'; import { discoverSpecFiles } from '../utils/spec-discovery.js'; +import { readChangeStatus, type ChangeStatus } from '../utils/change-metadata.js'; interface ChangeInfo { name: string; completedTasks: number; totalTasks: number; lastModified: Date; + /** + * Only set when the change's `.openspec.yaml` declares `status` itself. Left + * undefined otherwise so a project that never opts in sees no new column and + * no new JSON key. + */ + status?: ChangeStatus; } interface ListOptions { sort?: 'recent' | 'name'; json?: boolean; root?: RootOutput; + /** Filter to changes in this lifecycle state. Undeclared counts as `proposed`. */ + status?: ChangeStatus; } function isMissingPathError(error: unknown): boolean { @@ -96,7 +105,7 @@ function formatRelativeTime(date: Date): string { export class ListCommand { async execute(targetPath: string = '.', mode: 'changes' | 'specs' = 'changes', options: ListOptions = {}): Promise { - const { sort = 'recent', json = false, root } = options; + const { sort = 'recent', json = false, root, status: statusFilter } = options; if (mode === 'changes') { const changesDir = path.join(targetPath, 'openspec', 'changes'); @@ -120,17 +129,35 @@ export class ListCommand { const changes: ChangeInfo[] = []; for (const changeDir of changeDirs) { - const progress = await getTaskProgressForChange(changesDir, changeDir, targetPath); const changePath = path.join(changesDir, changeDir); + // Undeclared reads as `proposed`, which is what a change under + // `changes/` has always meant. Metadata that cannot be read is left + // unfiltered rather than guessed at - `openspec sync --check` and + // `openspec status` are where a broken file gets reported. + const marker = readChangeStatus(changePath); + if (statusFilter && !marker.invalidReason && marker.status !== statusFilter) { + continue; + } + const progress = await getTaskProgressForChange(changesDir, changeDir, targetPath); const lastModified = await getLastModified(changePath); changes.push({ name: changeDir, completedTasks: progress.completed, totalTasks: progress.total, - lastModified + lastModified, + ...(marker.declared ? { status: marker.status } : {}) }); } + if (changes.length === 0) { + if (json) { + console.log(JSON.stringify({ changes: [], ...(root ? { root } : {}) }, null, 2)); + } else { + console.log(`No changes with status '${statusFilter}' found.`); + } + return; + } + // Sort by preference (default: recent first) if (sort === 'recent') { changes.sort((a, b) => b.lastModified.getTime() - a.lastModified.getTime()); @@ -145,7 +172,11 @@ export class ListCommand { completedTasks: c.completedTasks, totalTasks: c.totalTasks, lastModified: c.lastModified.toISOString(), - status: c.totalTasks === 0 ? 'no-tasks' : c.completedTasks === c.totalTasks ? 'complete' : 'in-progress' + // `status` here has always meant task progress. The lifecycle state is + // a different axis and gets its own key, emitted only when the change + // declares one, so existing consumers see byte-identical output. + status: c.totalTasks === 0 ? 'no-tasks' : c.completedTasks === c.totalTasks ? 'complete' : 'in-progress', + ...(c.status ? { lifecycle: c.status } : {}) })); console.log(JSON.stringify({ changes: jsonOutput, ...(root ? { root } : {}) }, null, 2)); return; @@ -155,11 +186,17 @@ export class ListCommand { console.log('Changes:'); const padding = ' '; const nameWidth = Math.max(...changes.map(c => c.name.length)); + const anyLifecycleDeclared = changes.some(c => c.status !== undefined); for (const change of changes) { const paddedName = change.name.padEnd(nameWidth); const status = formatTaskStatus({ total: change.totalTasks, completed: change.completedTasks }); const timeAgo = formatRelativeTime(change.lastModified); - console.log(`${padding}${paddedName} ${status.padEnd(12)} ${timeAgo}`); + // Only rendered when some change in this root declares a lifecycle + // state, so the default listing is unchanged for everyone else. + const lifecycle = anyLifecycleDeclared + ? ` ${(change.status ?? 'proposed').padEnd(8)}` + : ''; + console.log(`${padding}${paddedName}${lifecycle} ${status.padEnd(12)} ${timeAgo}`); } return; } diff --git a/src/core/sync.ts b/src/core/sync.ts new file mode 100644 index 0000000000..fb5d805af7 --- /dev/null +++ b/src/core/sync.ts @@ -0,0 +1,735 @@ +/** + * Standalone spec sync. + * + * `archive` does two separable jobs in one command: it folds a change's deltas + * into `openspec/specs/`, and it declares the change finished by moving its + * directory. Welding the two means the fold can only happen at the moment the + * move happens - which, on a team that reviews before merging, is after the + * pull request closes. So a CI check for "the specs match what shipped" has + * nothing it can assert during review: every open PR has an unarchived change + * by definition, and a check phrased as "is everything archived?" is red as its + * resting state, from a change's first commit to its last (#1683). + * + * This module unwelds them, additively: + * + * - `openspec sync ` folds one change's deltas now, leaving the change + * where it is. `archive` still moves it later, and re-applying an already + * folded delta is a no-op the merge builder has supported all along (the + * "early-sync pattern" `specs-apply` names in ADDED, MODIFIED, REMOVED and + * RENAMED alike), so nothing about the archive step changes. + * - `openspec sync --check` asserts `shipped => folded` over the working tree. + * A change that has not declared itself shipped passes for free, so green is + * the resting state and red means a real mistake. The predicate is a pure + * function of files on disk - no VCS history, no timing - so a pre-commit + * hook, a pre-push hook and CI can all run the same command and get the same + * verdict. + * + * Two things this deliberately does NOT do, both for the same reason - they + * would turn an additive command into a second lifecycle: + * + * 1. **It never deletes a spec.** When a change's REMOVED entries take a + * capability's last requirement, `archive` retires the capability and + * deletes its main spec, gated on the author's `retire_capabilities` marker + * and wrapped in a displace-verify-delete dance that can roll back. Sync + * reports that case and points at `archive` instead of reimplementing the + * one irreversible operation in the system. + * 2. **It never checks archived changes.** Once a change is archived its deltas + * are history, and later changes supersede them; re-applying a five-month-old + * delta on top of everything that came after it is not a drift check, it is + * a merge conflict waiting to be written back over current text. The checked + * set is exactly the active changes that declare `status: shipped`, which is + * bounded and drains itself as those changes archive. + * + * Credit: the diagnosis, the `shipped => folded` framing, and the argument that + * a checker which reimplements the doer eventually disagrees with it are all + * from Matan Bendix Shenhav's proposal in #1683. + */ + +import { promises as fs } from 'fs'; +import path from 'path'; +import chalk from 'chalk'; +import { Validator } from './validation/validator.js'; +import { getTaskProgressForChange, formatTaskStatus } from '../utils/task-progress.js'; +import { + emitStoreRootBanner, + isRootSelectionError, + resolveOpenSpecRoot, + toRootOutput, + withStoreFlag, + isStoreSelectedRoot, + type ResolvedOpenSpecRoot, +} from './root-selection.js'; +import { + findSpecUpdates, + buildUpdatedSpec, + writeUpdatedSpec, + type SpecUpdate, +} from './specs-apply.js'; +import { isRetirableSpec, listActiveChangeNames } from './archive.js'; +import { + readChangeStatus, + writeChangeStatus, + METADATA_FILENAME, + type ChangeStatus, +} from '../utils/change-metadata.js'; +import { FileSystemUtils } from '../utils/file-system.js'; +import { folderStyleNameProblem } from './id.js'; + +// ----------------------------------------------------------------------------- +// Types +// ----------------------------------------------------------------------------- + +export interface SyncOptions { + /** Report what is unfolded and exit non-zero, without writing anything. */ + check?: boolean; + /** Set `status: shipped` on the change before folding, in one working-tree diff. */ + ship?: boolean; + /** Proceed past incomplete tasks without asking. */ + yes?: boolean; + /** Commander sets this to false for `--no-validate`. */ + validate?: boolean; + json?: boolean; + store?: string; + storePath?: string; +} + +export interface SyncSpecReport { + /** Capability id relative to the specs root, e.g. `billing/invoices`. */ + capability: string; + /** True when re-applying this delta would still change the main spec. */ + pending: boolean; + counts: { added: number; modified: number; removed: number; renamed: number }; +} + +export interface SyncChangeReport { + change: string; + status: ChangeStatus; + /** True when every delta this change carries is already in the main specs. */ + folded: boolean; + specs: SyncSpecReport[]; + warnings: string[]; + /** + * Reasons this change cannot be folded by `sync` at all: a delta the merge + * refuses, a retirement only `archive` can perform, or metadata whose + * `status` could not be determined. + */ + blockers: string[]; +} + +export interface SyncResult { + /** True for `--check`: nothing was written. */ + checked: boolean; + /** True when every examined change is folded and no change is blocked. */ + clean: boolean; + changes: SyncChangeReport[]; + totals: { added: number; modified: number; removed: number; renamed: number }; +} + +/** + * Carries the same `diagnostic` envelope RootSelectionError and StoreError do, + * so the CLI's shared failure plumbing prints the `Fix:` line and JSON callers + * get one status object - without this class joining their hierarchy. + */ +export class SyncBlockedError extends Error { + readonly diagnostic: { + severity: 'error'; + code: string; + message: string; + fix?: string; + }; + + constructor(code: string, message: string, fix?: string) { + super(message); + this.name = 'SyncBlockedError'; + this.diagnostic = { + severity: 'error', + code, + message, + ...(fix ? { fix } : {}), + }; + } +} + +// ----------------------------------------------------------------------------- +// Evaluation +// ----------------------------------------------------------------------------- + +interface PreparedSpec { + update: SpecUpdate; + rebuilt: string; + counts: SyncSpecReport['counts']; +} + +interface Evaluation { + report: SyncChangeReport; + /** Only the specs that still need writing. Empty when the change is folded. */ + writes: PreparedSpec[]; +} + +function sumCounts(counts: SyncSpecReport['counts']): number { + return counts.added + counts.modified + counts.removed + counts.renamed; +} + +/** + * Decide whether a change's deltas are already in the main specs, by running the + * merge builder and looking at how much it had to do. + * + * "Folded" is `buildUpdatedSpec` applying zero operations - the *same* predicate + * `archive` uses to decide it has nothing to write ("Every operation was already + * synced: rewriting the file would only churn normalization differences into + * it"). Deliberately not a byte-comparison against the rebuilt output: the + * rebuild normalizes blank lines, so a main spec that a human formatted by hand + * would compare unequal while being perfectly in sync, and the gate would be red + * for a change nobody made. Sharing archive's own predicate is also what keeps + * checker and doer from drifting apart, which is the failure #1112 describes. + */ +async function evaluateChange( + changeName: string, + changeDir: string, + mainSpecsDir: string +): Promise { + const status = readChangeStatus(changeDir); + const report: SyncChangeReport = { + change: changeName, + status: status.status, + folded: true, + specs: [], + warnings: [], + blockers: [], + }; + + if (status.invalidReason) { + // Undetermined is not "proposed". A change whose metadata broke would + // otherwise pass a gate whose whole job is noticing that kind of rot. + report.folded = false; + report.blockers.push( + `Could not read the change's lifecycle status from ${METADATA_FILENAME}: ${status.invalidReason}` + ); + return { report, writes: [] }; + } + + let updates: SpecUpdate[]; + try { + updates = await findSpecUpdates(changeDir, mainSpecsDir); + } catch (error) { + report.folded = false; + report.blockers.push( + `Could not read this change's delta specs: ${ + error instanceof Error ? error.message : String(error) + }` + ); + return { report, writes: [] }; + } + + const writes: PreparedSpec[] = []; + for (const update of updates) { + let built: Awaited>; + try { + built = await buildUpdatedSpec(update, changeName, { silent: true }); + } catch (error) { + report.folded = false; + report.blockers.push( + `${update.id}: ${error instanceof Error ? error.message : String(error)}` + ); + continue; + } + + report.warnings.push(...built.warnings); + + // The one case sync hands back to archive. When a change's REMOVED entries + // take a capability's last requirement, the spec that would be written has + // no requirements and cannot be validated - archive's answer is to delete + // the file, which needs the author's `retire_capabilities` marker and a + // rollback-safe deletion. Reimplementing that here would put the system's + // only irreversible operation behind a second, less careful door. + if ( + update.exists && + built.counts.removed > 0 && + built.noRequirementBlocks && + (await isRetirableSpec(update.id, built.rebuilt)) + ) { + report.folded = false; + report.blockers.push( + `${update.id}: this change removes the capability's last requirement. ` + + `Retiring a capability deletes its spec, which only openspec archive does. ` + + `Archive the change instead of syncing it.` + ); + continue; + } + + const pending = sumCounts(built.counts) > 0; + report.specs.push({ capability: update.id, pending, counts: built.counts }); + if (pending) { + report.folded = false; + writes.push({ update, rebuilt: built.rebuilt, counts: built.counts }); + } + } + + if (report.blockers.length > 0) report.folded = false; + return { report, writes }; +} + +// ----------------------------------------------------------------------------- +// Command +// ----------------------------------------------------------------------------- + +export class SyncCommand { + async execute(changeName?: string, options: SyncOptions = {}): Promise { + const json = !!options.json; + + let root: ResolvedOpenSpecRoot; + try { + root = await resolveOpenSpecRoot({ + ...(options.store !== undefined ? { store: options.store } : {}), + ...(options.storePath !== undefined ? { storePath: options.storePath } : {}), + }); + } catch (error) { + if (json && isRootSelectionError(error)) { + this.printJsonFailure(undefined, { + code: error.diagnostic.code, + message: error.diagnostic.message, + ...(error.diagnostic.fix ? { fix: error.diagnostic.fix } : {}), + }); + return; + } + throw error; + } + + if (json) { + try { + const result = await this.run(changeName, options, root, true); + if (!result) return; + console.log( + JSON.stringify({ sync: result, root: toRootOutput(root) }, null, 2) + ); + if (!result.clean) process.exitCode = 1; + } catch (error) { + this.printJsonFailure(root, toDiagnostic(error)); + } + return; + } + + emitStoreRootBanner(root); + await this.run(changeName, options, root, false); + } + + private printJsonFailure( + root: ResolvedOpenSpecRoot | undefined, + diagnostic: { code: string; message: string; fix?: string } + ): void { + console.log( + JSON.stringify( + { + sync: null, + ...(root ? { root: toRootOutput(root) } : {}), + status: [{ severity: 'error', ...diagnostic }], + }, + null, + 2 + ) + ); + process.exitCode = 1; + } + + private async run( + changeName: string | undefined, + options: SyncOptions, + root: ResolvedOpenSpecRoot, + json: boolean + ): Promise { + const changesDir = root.changesDir; + const mainSpecsDir = root.specsDir; + + for (const [allowedDirectory, managedDir] of [ + [root.path, changesDir], + [root.path, mainSpecsDir], + ] as const) { + try { + FileSystemUtils.assertPathWithin(allowedDirectory, managedDir); + } catch { + throw new SyncBlockedError( + 'sync_path_outside_root', + `Refusing to sync through a path outside the OpenSpec root: ${managedDir}` + ); + } + } + + const check = !!options.check; + if (options.ship && check) { + throw new SyncBlockedError( + 'sync_ship_with_check', + '--ship writes to the change and --check writes nothing; pass one or the other.' + ); + } + if (options.ship && !changeName) { + throw new SyncBlockedError( + 'sync_ship_needs_change', + '--ship needs the change to mark shipped.', + withStoreFlag(root, 'openspec sync --ship') + ); + } + + const targets = changeName + ? [await this.resolveNamedChange(changeName, changesDir, root)] + : await this.shippedChanges(changesDir); + + if (targets.length === 0) { + const result: SyncResult = { + checked: check, + clean: true, + changes: [], + totals: { added: 0, modified: 0, removed: 0, renamed: 0 }, + }; + if (!json) { + console.log( + check + ? 'No change declares `status: shipped`; nothing to check.' + : 'No change declares `status: shipped`; nothing to sync. ' + + 'Name a change to sync it directly, or add `status: shipped` to its ' + + `${METADATA_FILENAME}.` + ); + } + return result; + } + + // `--ship` before evaluation, so the fold that follows is measured against + // the state the commit will carry: the field flip and the spec write land + // in one working-tree diff, and no intermediate commit can claim a change + // is shipped while its deltas are not in the specs. + if (options.ship) { + writeChangeStatus(path.join(changesDir, targets[0]), 'shipped'); + if (!json) console.log(`Marked '${targets[0]}' as shipped.`); + } + + const evaluations: Evaluation[] = []; + for (const name of targets) { + evaluations.push( + await evaluateChange(name, path.join(changesDir, name), mainSpecsDir) + ); + } + + return check + ? this.reportCheck(evaluations, root, json) + : this.applyFolds(evaluations, changesDir, root, options, json); + } + + /** A named change has to exist, exactly as archive requires. */ + private async resolveNamedChange( + changeName: string, + changesDir: string, + root: ResolvedOpenSpecRoot + ): Promise { + const problem = folderStyleNameProblem(changeName, 'Change name'); + if (problem) throw new SyncBlockedError('sync_change_name_invalid', problem); + + const changeDir = path.join(changesDir, changeName); + try { + const stat = await fs.lstat(changeDir); + if (stat.isSymbolicLink()) { + throw new SyncBlockedError( + 'sync_change_symlink', + `Change '${changeName}' is a symbolic link. Replace it with a real directory before syncing.` + ); + } + if (!stat.isDirectory()) throw new Error('not a directory'); + } catch (error) { + if (error instanceof SyncBlockedError) throw error; + const available = await listActiveChangeNames(changesDir); + throw new SyncBlockedError( + 'sync_change_not_found', + available.length > 0 + ? `Change '${changeName}' not found. Available changes: ${available.join(', ')}` + : `Change '${changeName}' not found. No active changes exist in this root.`, + withStoreFlag(root, 'openspec list') + ); + } + return changeName; + } + + /** + * Every active change that declares `status: shipped`, plus every change + * whose status could not be read at all. + * + * The second half is what keeps the gate honest. Skipping an unreadable + * `.openspec.yaml` here would be the fail-open direction: a change that + * declared itself shipped and then had its metadata broken would silently + * stop being checked. `evaluateChange` turns it into a named blocker. + */ + private async shippedChanges(changesDir: string): Promise { + const names = await listActiveChangeNames(changesDir); + return names.filter((name) => { + const status = readChangeStatus(path.join(changesDir, name)); + return status.invalidReason !== undefined || status.status === 'shipped'; + }); + } + + private reportCheck( + evaluations: Evaluation[], + root: ResolvedOpenSpecRoot, + json: boolean + ): SyncResult { + const changes = evaluations.map((evaluation) => evaluation.report); + const clean = changes.every((change) => change.folded); + const result: SyncResult = { + checked: true, + clean, + changes, + totals: { added: 0, modified: 0, removed: 0, renamed: 0 }, + }; + + if (json) { + if (!clean) process.exitCode = 1; + return result; + } + + if (clean) { + console.log( + `✓ ${changes.length} shipped change(s) are folded into the main specs.` + ); + return result; + } + + // Titled for both shapes of failure it reports: a shipped change whose + // deltas are not in the main specs, and a change whose lifecycle status + // could not be determined at all. + console.log(chalk.red('Sync check failed:\n')); + for (const change of changes) { + if (change.folded) continue; + console.log(` ${change.change}`); + for (const spec of change.specs) { + if (!spec.pending) continue; + const { added, modified, removed, renamed } = spec.counts; + const parts = [ + added ? `+${added}` : '', + modified ? `~${modified}` : '', + removed ? `-${removed}` : '', + renamed ? `→${renamed}` : '', + ].filter(Boolean); + console.log(` ${spec.capability}: ${parts.join(' ')} not applied`); + } + for (const blocker of change.blockers) { + console.log(chalk.yellow(` ${blocker}`)); + } + } + const fixable = changes.filter( + (change) => !change.folded && change.blockers.length === 0 + ); + if (fixable.length > 0) { + console.log( + `\nRun ${withStoreFlag(root, 'openspec sync')} to fold them, then commit the result.` + ); + } + process.exitCode = 1; + return result; + } + + private async applyFolds( + evaluations: Evaluation[], + changesDir: string, + root: ResolvedOpenSpecRoot, + options: SyncOptions, + json: boolean + ): Promise { + const skipValidation = options.validate === false; + + const blocked = evaluations.filter( + (evaluation) => evaluation.report.blockers.length > 0 + ); + if (blocked.length > 0) { + const first = blocked[0]; + throw new SyncBlockedError( + 'sync_change_blocked', + `Cannot sync '${first.report.change}': ${first.report.blockers[0]}`, + blocked.length > 1 + ? `${blocked.length} changes are blocked; run openspec sync --check for the full list.` + : undefined + ); + } + + // Same guards archive runs before it writes a spec, in the same order. + for (const { report } of evaluations) { + const changeDir = path.join(changesDir, report.change); + if (!skipValidation) { + await this.assertDeltaSpecsValid(report.change, changeDir, root, json); + } + await this.assertTasksComplete(report.change, changesDir, options, root, json); + } + + if (!json) { + for (const { report } of evaluations) { + for (const warning of report.warnings) { + console.log(chalk.yellow(`⚠️ Warning: ${warning}`)); + } + } + } + + // Every rebuilt spec is validated before any of them is written, so a late + // failure leaves every target unchanged rather than half the tree folded. + if (!skipValidation) { + const validator = new Validator(); + for (const { report, writes } of evaluations) { + for (const write of writes) { + const specReport = await validator.validateSpecContent( + write.update.id, + write.rebuilt + ); + if (specReport.valid) continue; + const details = specReport.issues + .filter((issue) => issue.level === 'ERROR') + .map((issue) => issue.message) + .join('; '); + throw new SyncBlockedError( + 'sync_spec_validation_failed', + `The spec '${write.update.id}' would be rebuilt into an invalid state by ` + + `change '${report.change}': ${details}. No files were changed.`, + `Run ${withStoreFlag(root, `openspec validate ${write.update.id}`)} after fixing the change deltas.` + ); + } + } + } + + const totals = { added: 0, modified: 0, removed: 0, renamed: 0 }; + let wroteAny = false; + for (const { writes } of evaluations) { + for (const write of writes) { + await writeUpdatedSpec(write.update, write.rebuilt, write.counts, { + silent: json, + ...(isStoreSelectedRoot(root) ? { displayPath: write.update.target } : {}), + }); + wroteAny = true; + totals.added += write.counts.added; + totals.modified += write.counts.modified; + totals.removed += write.counts.removed; + totals.renamed += write.counts.renamed; + } + } + + // Re-evaluate rather than assume. Folding is idempotent for a single + // change, but two shipped changes can disagree about the same requirement - + // one adding what the other removes - and a fold that does not settle would + // otherwise report success while `--check` immediately after went red. The + // merge builder catches the destructive shapes of that disagreement on its + // own (a MODIFIED that would drop a scenario, an ADDED whose content + // differs), so what reaches here is the non-convergent rest, and naming it + // is better than looping on it. + const unsettled: string[] = []; + for (const { report } of evaluations) { + const after = await evaluateChange( + report.change, + path.join(changesDir, report.change), + root.specsDir + ); + if (!after.report.folded) unsettled.push(report.change); + } + if (unsettled.length > 0) { + throw new SyncBlockedError( + 'sync_did_not_converge', + `Specs were written, but these changes still report unfolded deltas: ` + + `${unsettled.join(', ')}. Two shipped changes are claiming the same ` + + `requirement in ways that cannot both hold.`, + 'Reconcile the conflicting deltas, then rerun.' + ); + } + + const changes = evaluations.map((evaluation) => ({ + ...evaluation.report, + folded: true, + })); + if (!json) { + if (wroteAny) { + console.log( + `Totals: + ${totals.added}, ~ ${totals.modified}, - ${totals.removed}, → ${totals.renamed}` + ); + console.log('Specs updated successfully.'); + } else { + console.log('Specs already in sync; no files changed.'); + } + } + + return { + checked: false, + clean: true, + changes, + totals, + }; + } + + /** + * Archive's delta validation, restricted to what sync needs. Sync only ever + * runs on a change that has deltas to fold, so the `skip_specs` reconciliation + * archive performs (a change declaring it has no deltas, but carrying files) + * has nothing to decide here - `findSpecUpdates` already found the files. + */ + private async assertDeltaSpecsValid( + changeName: string, + changeDir: string, + root: ResolvedOpenSpecRoot, + json: boolean + ): Promise { + const report = await new Validator().validateChangeDeltaSpecs(changeDir); + if (report.valid) return; + + if (!json) { + console.log(chalk.red(`\nValidation errors in change delta specs:`)); + for (const issue of report.issues) { + if (issue.level === 'ERROR') console.log(chalk.red(` ✗ ${issue.message}`)); + else if (issue.level === 'WARNING') console.log(chalk.yellow(` ⚠ ${issue.message}`)); + } + } + throw new SyncBlockedError( + 'sync_validation_failed', + `Validation failed for change '${changeName}'. No files were changed.`, + `Run ${withStoreFlag(root, `openspec validate ${changeName}`)} for details, fix the errors, or rerun with --no-validate.` + ); + } + + /** + * Folding a change whose tasks are unfinished writes requirements into + * `specs/` that nothing implements yet - the exact drift the gate exists to + * prevent, arriving through the gate's own command. Blocks rather than warns, + * because sync is designed to run unattended in a hook. + */ + private async assertTasksComplete( + changeName: string, + changesDir: string, + options: SyncOptions, + root: ResolvedOpenSpecRoot, + json: boolean + ): Promise { + const progress = await getTaskProgressForChange( + changesDir, + changeName, + path.resolve(changesDir, '..', '..') + ); + const incomplete = Math.max(progress.total - progress.completed, 0); + if (incomplete === 0) return; + + if (options.yes) { + if (!json) { + console.log( + `Warning: ${incomplete} incomplete task(s) in '${changeName}'. Continuing due to --yes flag.` + ); + } + return; + } + + if (!json) console.log(`Task status: ${formatTaskStatus(progress)}`); + throw new SyncBlockedError( + 'sync_tasks_incomplete', + `${incomplete} incomplete task(s) in '${changeName}'. Syncing now would write ` + + `requirements into the main specs that nothing implements yet.`, + `Complete the tasks, or rerun with ${withStoreFlag(root, `openspec sync ${changeName} --yes`)}.` + ); + } +} + +function toDiagnostic(error: unknown): { code: string; message: string; fix?: string } { + if (error instanceof SyncBlockedError) { + const { severity: _severity, ...rest } = error.diagnostic; + return rest; + } + return { + code: 'sync_error', + message: error instanceof Error ? error.message : String(error), + }; +} diff --git a/src/core/templates/workflows/store-selection.ts b/src/core/templates/workflows/store-selection.ts index dfa132c3e5..771269e6ef 100644 --- a/src/core/templates/workflows/store-selection.ts +++ b/src/core/templates/workflows/store-selection.ts @@ -4,4 +4,4 @@ * Interpolated into every workflow's instructions so generated skills * consistently teach how to target a registered store with `--store `. */ -export const STORE_SELECTION_GUIDANCE = `**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run \`openspec store list --json\` to discover registered store ids, then pass \`--store \` on the commands that read or write specs and changes (\`new change\`, \`status\`, \`instructions\`, \`list\`, \`show\`, \`validate\`, \`archive\`, \`doctor\`, \`context\`, \`schemas\`, \`view\`). Once selected, treat \`--store \` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run \`openspec status --change "" --json --store ""\`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local \`openspec/\` root.`; +export const STORE_SELECTION_GUIDANCE = `**Store selection:** If the user names a store (a store is a standalone OpenSpec repo registered on this machine) or the work lives in one, run \`openspec store list --json\` to discover registered store ids, then pass \`--store \` on the commands that read or write specs and changes (\`new change\`, \`status\`, \`instructions\`, \`list\`, \`show\`, \`validate\`, \`sync\`, \`archive\`, \`doctor\`, \`context\`, \`schemas\`, \`view\`). Once selected, treat \`--store \` as sticky for the rest of the workflow. Every unscoped example of those commands below is shorthand: before running it, append the flag. For example, run \`openspec status --change "" --json --store ""\`, not the unscoped form shown below. Other commands do not take the flag. Hints printed by commands already carry the flag; keep it on follow-ups. Without a store, commands act on the nearest local \`openspec/\` root.`; diff --git a/src/utils/change-metadata.ts b/src/utils/change-metadata.ts index 1f31a44596..8c2dd5c331 100644 --- a/src/utils/change-metadata.ts +++ b/src/utils/change-metadata.ts @@ -343,3 +343,176 @@ function readBooleanMarker( } return { declared: false }; } + +/** + * Where a change sits in its own lifecycle. + * + * `proposed` is the resting state and needs no declaration: it is what every + * change under `changes/` has always meant, so a project that never opts in + * reads as proposed everywhere. + */ +export type ChangeStatus = 'proposed' | 'shipped'; + +export interface ChangeStatusMarker { + /** The change's state. `proposed` unless the metadata explicitly says otherwise. */ + status: ChangeStatus; + /** True only when `.openspec.yaml` sets `status` itself. */ + declared: boolean; + /** + * Set when the state could not be determined: the metadata file exists but + * cannot be read, does not parse, or fails the contract the rest of the CLI + * enforces. Callers must never round this to `proposed` - that is the + * direction that lets a shipped change slip past `openspec sync --check`. + */ + invalidReason?: string; +} + +/** + * Non-throwing read of the `status` field, with the same metadata contract the + * boolean markers above enforce: the file has to parse under + * ChangeMetadataSchema and name a schema that both passes `listSchemas` + * membership and actually resolves. + * + * The one difference is what an unreadable file means. A boolean marker that + * cannot be honored falls back to "not declared", which is the safe direction + * for `skip_specs` and `retire_capabilities` - both authorize an action, so + * withholding them does less. `status` gates a *check*, so the safe direction + * is the opposite: undetermined must stay undetermined and be reported, or a + * change whose metadata broke would quietly pass the gate that exists to + * notice exactly that kind of rot. + */ +export function readChangeStatus(changeDir: string): ChangeStatusMarker { + let raw: string; + try { + raw = fs.readFileSync(path.join(changeDir, METADATA_FILENAME), 'utf-8'); + } catch (err) { + if ((err as NodeJS.ErrnoException)?.code === 'ENOENT') { + // No metadata at all is the ordinary case for changes authored before + // the file existed, and for every change that never opts in. + return { status: 'proposed', declared: false }; + } + const message = err instanceof Error ? err.message : String(err); + return undetermined(`the metadata file cannot be read (${message})`); + } + + let parsed: unknown; + try { + parsed = yaml.parse(raw); + } catch { + // Anchored so a comment like "# set status: shipped once merged" does not + // turn an unrelated YAML problem into an undetermined state. + const mentioned = /^\s*(['"]?)status\1\s*:/m.test(raw); + return mentioned + ? undetermined('the file is not valid YAML') + : { status: 'proposed', declared: false }; + } + + const result = ChangeMetadataSchema.safeParse(parsed); + if (result.success) { + if (result.data.status === undefined) { + return { status: 'proposed', declared: false }; + } + // Checked only when the field is declared, exactly as the boolean markers + // do: a broken schema on an ordinary change is `openspec status`'s problem + // to report, not this reader's. + try { + const projectRoot = path.resolve(changeDir, '../../..'); + if (!listSchemas(projectRoot).includes(result.data.schema)) { + return undetermined(`schema: unknown schema '${result.data.schema}'`); + } + resolveSchema(result.data.schema, projectRoot); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + return undetermined(message); + } + return { status: result.data.status, declared: true }; + } + + // Key presence, not value: `status: shiped` must surface as undetermined + // rather than silently reading as proposed. Metadata that is broken for some + // unrelated reason, on a change that never mentions `status`, is simply not + // declared - the same restraint the boolean markers show. + const mentioned = + typeof parsed === 'object' && parsed !== null && 'status' in parsed; + if (mentioned) { + const first = result.error.issues[0]; + const where = first.path.length > 0 ? `${first.path.join('.')}: ` : ''; + return undetermined(`${where}${first.message}`); + } + return { status: 'proposed', declared: false }; +} + +/** + * A state that could not be determined, with its reason made safe to print. + * Same treatment as `unhonorable` above: every reason quotes something the + * author wrote, and callers print it straight to a terminal. + */ +function undetermined(reason: string): ChangeStatusMarker { + return { + status: 'proposed', + declared: false, + invalidReason: reason.replace(/[\u0000-\u001f\u007f]/g, '?'), + }; +} + +/** + * Set `status` in a change's `.openspec.yaml`, preserving every other field and + * the file's own formatting. + * + * Edits the parsed document rather than rewriting it from the validated object, + * so comments and key order survive - `writeChangeMetadata` would flatten both, + * and this file is hand-authored. + */ +export function writeChangeStatus(changeDir: string, status: ChangeStatus): void { + const metaPath = path.join(changeDir, METADATA_FILENAME); + let raw: string; + try { + raw = fs.readFileSync(metaPath, 'utf-8'); + } catch (err) { + const ioError = err instanceof Error ? err : new Error(String(err)); + throw new ChangeMetadataError( + (err as NodeJS.ErrnoException)?.code === 'ENOENT' + ? `No ${METADATA_FILENAME} in this change, so there is nothing to set status on. ` + + `Create the change with openspec new change, or add the file by hand.` + : `Failed to read metadata: ${ioError.message}`, + metaPath, + ioError + ); + } + + let doc: ReturnType; + try { + doc = yaml.parseDocument(raw); + if (doc.errors.length > 0) throw new Error(doc.errors[0].message); + } catch (err) { + const parseError = err instanceof Error ? err : new Error(String(err)); + throw new ChangeMetadataError( + `Invalid YAML in metadata file: ${parseError.message}`, + metaPath, + parseError + ); + } + + doc.set('status', status); + + // The edited document still has to satisfy the contract every reader + // enforces, or this would be a way to write metadata the CLI then rejects. + const check = ChangeMetadataSchema.safeParse(doc.toJS()); + if (!check.success) { + throw new ChangeMetadataError( + `Invalid metadata: ${check.error.message}`, + metaPath + ); + } + + try { + fs.writeFileSync(metaPath, doc.toString(), 'utf-8'); + } catch (err) { + const ioError = err instanceof Error ? err : new Error(String(err)); + throw new ChangeMetadataError( + `Failed to write metadata: ${ioError.message}`, + metaPath, + ioError + ); + } +} diff --git a/test/core/completions/command-registry.test.ts b/test/core/completions/command-registry.test.ts index c3ada9fb79..6e48e66b5e 100644 --- a/test/core/completions/command-registry.test.ts +++ b/test/core/completions/command-registry.test.ts @@ -174,6 +174,7 @@ describe('command completion registry', () => { 'schemas', 'show', 'status', + 'sync', 'validate', 'view', ]); diff --git a/test/core/list.test.ts b/test/core/list.test.ts index 5b23a5d712..4144edec3f 100644 --- a/test/core/list.test.ts +++ b/test/core/list.test.ts @@ -187,4 +187,81 @@ Regular text that should be ignored expect(logOutput.some(line => line.includes('no-tasks') && line.includes('No tasks'))).toBe(true); }); }); + + describe('lifecycle status', () => { + async function change(name: string, metadata?: string): Promise { + const dir = path.join(tempDir, 'openspec', 'changes', name); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, 'tasks.md'), '- [x] 1.1 Done\n'); + if (metadata !== undefined) { + await fs.writeFile(path.join(dir, '.openspec.yaml'), metadata); + } + } + + it('renders no lifecycle column when no change declares one', async () => { + await change('a'); + await change('b', 'schema: spec-driven\n'); + + await new ListCommand().execute(tempDir, 'changes'); + + // A project that never opts in must see byte-identical output. + expect(logOutput.join('\n')).not.toContain('proposed'); + expect(logOutput.join('\n')).not.toContain('shipped'); + }); + + it('renders the column once any change declares one', async () => { + await change('a'); + await change('b', 'schema: spec-driven\nstatus: shipped\n'); + + await new ListCommand().execute(tempDir, 'changes'); + + const text = logOutput.join('\n'); + expect(text).toContain('shipped'); + // An undeclared change reads as proposed rather than blank. + expect(text).toContain('proposed'); + }); + + it('filters to shipped changes', async () => { + await change('a'); + await change('b', 'schema: spec-driven\nstatus: shipped\n'); + + await new ListCommand().execute(tempDir, 'changes', { status: 'shipped' }); + + const text = logOutput.join('\n'); + expect(text).toContain('b'); + expect(text).not.toMatch(/^\s+a\s/m); + }); + + it('counts an undeclared change as proposed when filtering', async () => { + await change('a'); + await change('b', 'schema: spec-driven\nstatus: shipped\n'); + + await new ListCommand().execute(tempDir, 'changes', { status: 'proposed' }); + + const text = logOutput.join('\n'); + expect(text).toContain('a'); + expect(text).not.toContain('shipped'); + }); + + it('says so when a filter matches nothing', async () => { + await change('a'); + + await new ListCommand().execute(tempDir, 'changes', { status: 'shipped' }); + + expect(logOutput).toEqual(["No changes with status 'shipped' found."]); + }); + + it('emits the lifecycle key in JSON only when declared', async () => { + await change('a'); + await change('b', 'schema: spec-driven\nstatus: shipped\n'); + + await new ListCommand().execute(tempDir, 'changes', { json: true, sort: 'name' }); + + const payload = JSON.parse(logOutput.join('\n')); + expect(payload.changes[0]).not.toHaveProperty('lifecycle'); + expect(payload.changes[1].lifecycle).toBe('shipped'); + // The pre-existing `status` key still means task progress. + expect(payload.changes[1].status).toBe('complete'); + }); + }); }); diff --git a/test/core/sync.test.ts b/test/core/sync.test.ts new file mode 100644 index 0000000000..6b0be98972 --- /dev/null +++ b/test/core/sync.test.ts @@ -0,0 +1,481 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import os from 'os'; +import { SyncCommand } from '../../src/core/sync.js'; +import { ArchiveCommand } from '../../src/core/archive.js'; +import { readChangeStatus, writeChangeStatus } from '../../src/utils/change-metadata.js'; + +vi.mock('@inquirer/prompts', () => ({ + select: vi.fn(), + confirm: vi.fn(), +})); + +const MAIN_SPEC = `# api Specification + +## Purpose +The API surface exposed to clients, and the rules requests are admitted under. + +## Requirements + +### Requirement: Rate limiting +The API SHALL reject requests above the configured rate. + +#### Scenario: Over the limit +- **WHEN** a client exceeds the rate +- **THEN** the API responds 429 +`; + +const ADDED_DELTA = `## ADDED Requirements + +### Requirement: Request tracing +The API SHALL attach a trace id to every response. + +#### Scenario: Traced response +- **WHEN** a request is served +- **THEN** the response carries a trace id +`; + +describe('SyncCommand', () => { + let tempDir: string; + let sync: SyncCommand; + const originalConsoleLog = console.log; + const originalExitCode = process.exitCode; + const originalXdgDataHome = process.env.XDG_DATA_HOME; + let logged: string[]; + + const changesDir = (): string => path.join(tempDir, 'openspec', 'changes'); + const specsDir = (): string => path.join(tempDir, 'openspec', 'specs'); + const output = (): string => logged.join('\n'); + + /** A complete, valid change with one ADDED delta against `api`. */ + async function makeChange( + name: string, + options: { + status?: string; + delta?: string; + tasks?: string; + metadata?: string; + } = {} + ): Promise { + const dir = path.join(changesDir(), name); + await fs.mkdir(path.join(dir, 'specs', 'api'), { recursive: true }); + await fs.writeFile( + path.join(dir, '.openspec.yaml'), + options.metadata ?? + `schema: spec-driven\n${options.status ? `status: ${options.status}\n` : ''}` + ); + await fs.writeFile( + path.join(dir, 'proposal.md'), + '## Why\nThe API needs request tracing, and today nothing correlates calls.\n\n' + + '## What Changes\n- Add request tracing to the API surface.\n' + ); + await fs.writeFile( + path.join(dir, 'tasks.md'), + options.tasks ?? '## 1. Work\n- [x] 1.1 Done\n' + ); + await fs.writeFile( + path.join(dir, 'specs', 'api', 'spec.md'), + options.delta ?? ADDED_DELTA + ); + return dir; + } + + async function mainSpec(): Promise { + return fs.readFile(path.join(specsDir(), 'api', 'spec.md'), 'utf-8'); + } + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-sync-test-')); + process.chdir(tempDir); + // Keep root resolution off any real store registry on the host. + process.env.XDG_DATA_HOME = path.join(tempDir, 'xdg-data'); + + await fs.mkdir(path.join(specsDir(), 'api'), { recursive: true }); + await fs.mkdir(path.join(changesDir(), 'archive'), { recursive: true }); + await fs.writeFile(path.join(tempDir, 'openspec', 'project.md'), '# Demo\n'); + await fs.writeFile(path.join(specsDir(), 'api', 'spec.md'), MAIN_SPEC); + + logged = []; + console.log = vi.fn((...args: unknown[]) => { + logged.push(args.map(String).join(' ')); + }); + process.exitCode = undefined; + sync = new SyncCommand(); + }); + + afterEach(async () => { + console.log = originalConsoleLog; + process.exitCode = originalExitCode; + if (originalXdgDataHome === undefined) delete process.env.XDG_DATA_HOME; + else process.env.XDG_DATA_HOME = originalXdgDataHome; + vi.clearAllMocks(); + try { + await fs.rm(tempDir, { recursive: true, force: true }); + } catch { + // Ignore cleanup errors. + } + }); + + describe('--check is green at rest', () => { + it('passes a proposed change without examining its deltas', async () => { + await makeChange('add-tracing'); + + await sync.execute(undefined, { check: true }); + + expect(process.exitCode).toBeUndefined(); + // The whole point of #1683: an open change is the resting state, not a + // failure, so the gate must not go red for one. + expect(output()).toContain('nothing to check'); + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + + it('passes when no change declares a status at all', async () => { + await makeChange('a'); + await makeChange('b'); + + await sync.execute(undefined, { check: true }); + + expect(process.exitCode).toBeUndefined(); + }); + }); + + describe('--check fails only on a real mistake', () => { + it('reports a shipped change whose deltas are not in the main specs', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + + await sync.execute(undefined, { check: true }); + + expect(process.exitCode).toBe(1); + expect(output()).toContain('add-tracing'); + expect(output()).toContain('api'); + expect(output()).toContain('openspec sync'); + // A check writes nothing, ever. + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + + it('passes the same change once it is folded', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + + await sync.execute(undefined, { yes: true }); + process.exitCode = undefined; + await sync.execute(undefined, { check: true }); + + expect(process.exitCode).toBeUndefined(); + expect(await mainSpec()).toContain('Request tracing'); + }); + }); + + describe('folding', () => { + it('applies a shipped change and leaves it in changes/', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + + await sync.execute(undefined, { yes: true }); + + expect(await mainSpec()).toContain('### Requirement: Request tracing'); + // Unlike archive, nothing moves: the change is still open for review. + await expect( + fs.stat(path.join(changesDir(), 'add-tracing')) + ).resolves.toBeTruthy(); + }); + + it('is idempotent: a second run writes nothing', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + + await sync.execute(undefined, { yes: true }); + const afterFirst = await mainSpec(); + logged = []; + await sync.execute(undefined, { yes: true }); + + expect(await mainSpec()).toBe(afterFirst); + expect(output()).toContain('already in sync'); + }); + + it('folds a named change regardless of its status', async () => { + // `openspec sync ` is the deterministic counterpart of the + // agent-driven `/opsx:sync` workflow, and predates any lifecycle field. + await makeChange('add-tracing'); + + await sync.execute('add-tracing', {}); + + expect(await mainSpec()).toContain('Request tracing'); + }); + + it('leaves archive able to run afterwards, changing nothing further', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + await sync.execute(undefined, { yes: true }); + const afterSync = await mainSpec(); + + await new ArchiveCommand().execute('add-tracing', { yes: true }); + + // The early-sync pattern: re-applying a folded delta is a no-op, so the + // archive step is unchanged by having synced first. + expect(await mainSpec()).toBe(afterSync); + await expect( + fs.stat(path.join(changesDir(), 'archive')) + ).resolves.toBeTruthy(); + }); + + it('stops checking a change once it is archived', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + await sync.execute(undefined, { yes: true }); + await new ArchiveCommand().execute('add-tracing', { yes: true }); + + logged = []; + process.exitCode = undefined; + await sync.execute(undefined, { check: true }); + + // Archived deltas are history: re-applying them on top of everything + // that came later is a merge conflict, not a drift check. + expect(process.exitCode).toBeUndefined(); + expect(output()).toContain('nothing to check'); + }); + }); + + describe('guards', () => { + it('refuses a change with incomplete tasks', async () => { + await makeChange('add-tracing', { + status: 'shipped', + tasks: '## 1. Work\n- [ ] 1.1 Not done\n', + }); + + await expect(sync.execute('add-tracing', {})).rejects.toThrow( + /incomplete task/i + ); + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + + it('proceeds past incomplete tasks with --yes', async () => { + await makeChange('add-tracing', { + status: 'shipped', + tasks: '## 1. Work\n- [ ] 1.1 Not done\n', + }); + + await sync.execute('add-tracing', { yes: true }); + + expect(await mainSpec()).toContain('Request tracing'); + }); + + it('refuses a delta that fails validation, writing nothing', async () => { + await makeChange('add-tracing', { + status: 'shipped', + // An ADDED requirement with no scenario is what validate rejects. + delta: + '## ADDED Requirements\n\n### Requirement: Request tracing\n' + + 'The API SHALL attach a trace id.\n', + }); + + await expect(sync.execute('add-tracing', { yes: true })).rejects.toThrow( + /Validation failed/ + ); + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + + it('never deletes a spec: a retirement is handed to archive', async () => { + await makeChange('retire-limits', { + status: 'shipped', + delta: + '## REMOVED Requirements\n\n### Requirement: Rate limiting\n' + + '**Reason**: Moved to the gateway.\n**Migration**: Configure the gateway.\n', + }); + + await expect(sync.execute('retire-limits', { yes: true })).rejects.toThrow( + /openspec archive/ + ); + // The one irreversible operation in the system stays behind archive's + // authorization marker and its rollback-safe deletion. + await expect( + fs.stat(path.join(specsDir(), 'api', 'spec.md')) + ).resolves.toBeTruthy(); + }); + + it('reports a retirement in --check without offering sync as the fix', async () => { + await makeChange('retire-limits', { + status: 'shipped', + delta: + '## REMOVED Requirements\n\n### Requirement: Rate limiting\n' + + '**Reason**: Moved to the gateway.\n**Migration**: Configure the gateway.\n', + }); + + await sync.execute(undefined, { check: true }); + + expect(process.exitCode).toBe(1); + expect(output()).toContain('openspec archive'); + expect(output()).not.toContain('Run openspec sync to fold them'); + }); + }); + + describe('undetermined status fails closed', () => { + it('reports a change whose status value is not a known state', async () => { + await makeChange('add-tracing', { + metadata: 'schema: spec-driven\nstatus: shiped\n', + }); + + await sync.execute(undefined, { check: true }); + + // Rounding this to `proposed` is the fail-open direction: a change that + // declared itself shipped and then had its metadata broken would + // silently stop being checked. + expect(process.exitCode).toBe(1); + expect(output()).toContain('add-tracing'); + expect(output()).toContain('lifecycle status'); + }); + + it('reports a change whose metadata is not valid YAML but names status', async () => { + await makeChange('add-tracing', { + metadata: 'schema: spec-driven\nstatus: [unclosed\n', + }); + + await sync.execute(undefined, { check: true }); + + expect(process.exitCode).toBe(1); + expect(output()).toContain('not valid YAML'); + }); + + it('ignores broken metadata that never mentions status', async () => { + await makeChange('add-tracing', { metadata: 'schema: [unclosed\n' }); + + await sync.execute(undefined, { check: true }); + + // Not this gate's problem to report; `openspec status` and `validate` + // already fail on it, and claiming it here would be noise. + expect(process.exitCode).toBeUndefined(); + }); + }); + + describe('--ship', () => { + it('sets the field and folds in one run', async () => { + const dir = await makeChange('add-tracing'); + + await sync.execute('add-tracing', { ship: true }); + + expect(readChangeStatus(dir).status).toBe('shipped'); + expect(await mainSpec()).toContain('Request tracing'); + }); + + it('is refused alongside --check', async () => { + await makeChange('add-tracing'); + + await expect( + sync.execute('add-tracing', { ship: true, check: true }) + ).rejects.toThrow(/one or the other/); + }); + + it('is refused without a change name', async () => { + await makeChange('add-tracing'); + + await expect(sync.execute(undefined, { ship: true })).rejects.toThrow( + /needs the change/ + ); + }); + }); + + describe('JSON output', () => { + it('reports a clean check and exits 0', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + await sync.execute(undefined, { yes: true }); + logged = []; + process.exitCode = undefined; + + await sync.execute(undefined, { check: true, json: true }); + + const payload = JSON.parse(output()); + expect(payload.sync.clean).toBe(true); + expect(payload.sync.checked).toBe(true); + expect(payload.sync.changes[0].change).toBe('add-tracing'); + expect(process.exitCode).toBeUndefined(); + }); + + it('reports a dirty check and exits 1', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + + await sync.execute(undefined, { check: true, json: true }); + + const payload = JSON.parse(output()); + expect(payload.sync.clean).toBe(false); + expect(payload.sync.changes[0].specs[0].counts.added).toBe(1); + expect(process.exitCode).toBe(1); + }); + + it('emits one status document for a blocked run', async () => { + await makeChange('add-tracing', { + status: 'shipped', + tasks: '## 1. Work\n- [ ] 1.1 Not done\n', + }); + + await sync.execute('add-tracing', { json: true }); + + const payload = JSON.parse(output()); + expect(payload.sync).toBeNull(); + expect(payload.status[0].code).toBe('sync_tasks_incomplete'); + expect(process.exitCode).toBe(1); + }); + }); + + describe('errors', () => { + it('names the available changes when the change does not exist', async () => { + await makeChange('add-tracing'); + + await expect(sync.execute('nope', {})).rejects.toThrow(/add-tracing/); + }); + }); +}); + +describe('readChangeStatus / writeChangeStatus', () => { + let tempDir: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-status-test-')); + await fs.mkdir(path.join(tempDir, 'openspec', 'changes', 'c'), { + recursive: true, + }); + }); + + afterEach(async () => { + try { + await fs.rm(tempDir, { recursive: true, force: true }); + } catch { + // Ignore cleanup errors. + } + }); + + const changeDir = (): string => path.join(tempDir, 'openspec', 'changes', 'c'); + + it('reads an absent metadata file as proposed and undeclared', () => { + const marker = readChangeStatus(changeDir()); + expect(marker).toEqual({ status: 'proposed', declared: false }); + }); + + it('reads an absent status field as proposed and undeclared', async () => { + await fs.writeFile( + path.join(changeDir(), '.openspec.yaml'), + 'schema: spec-driven\n' + ); + expect(readChangeStatus(changeDir())).toEqual({ + status: 'proposed', + declared: false, + }); + }); + + it('preserves comments and key order when setting status', async () => { + const original = + '# hand-authored\nschema: spec-driven\ncreated: 2026-09-07\n'; + await fs.writeFile(path.join(changeDir(), '.openspec.yaml'), original); + + writeChangeStatus(changeDir(), 'shipped'); + + const written = await fs.readFile( + path.join(changeDir(), '.openspec.yaml'), + 'utf-8' + ); + expect(written).toContain('# hand-authored'); + expect(written.indexOf('schema:')).toBeLessThan(written.indexOf('created:')); + expect(readChangeStatus(changeDir()).status).toBe('shipped'); + }); + + it('refuses to stamp a change with no metadata file', () => { + expect(() => writeChangeStatus(changeDir(), 'shipped')).toThrow( + /nothing to set status on/ + ); + }); +}); diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 3f309bdaa3..9bb02d5376 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -38,46 +38,46 @@ import { import { STORE_SELECTION_GUIDANCE } from '../../../src/core/templates/workflows/store-selection.js'; const EXPECTED_FUNCTION_HASHES: Record = { - getExploreSkillTemplate: '6315fcc5c2eb848963bc8bca4c23e657412a99608e610daee59fb4e58cd21fd4', - getNewChangeSkillTemplate: 'eabd1e895c5881dcb17dcbaa3fb26098dd59e8eacb318e400820b4dc811ef781', - getContinueChangeSkillTemplate: '012136f6411a99c8fa228e2f9444cb64b0a89e0f56fdeac2fe03b2f5bee0c5d7', - getApplyChangeSkillTemplate: 'd1e7d5ceb85193c0964057dbb88e9651526754bd33f84020e2440ff0621d5dbb', - getFfChangeSkillTemplate: 'efa6a70c111b18b61a7720250b9622afa9a212fb64edf609cf80e2182a9bdf8c', - getSyncSpecsSkillTemplate: 'b099e2ff31859c9b10d928066e662524f9aad9ecf2be12fceacb732d718c4146', - getOnboardSkillTemplate: '3a836faae463d88c289a1c129cb7ee556a563b7e53e1a52a4711ff152a3b51f7', - getOpsxExploreCommandTemplate: 'b4706a5b8fd280f7929eea610ecc9d41676b2d2dd6653d259cbbc2bfe01813d9', - getOpsxNewCommandTemplate: 'f2d30e569798a4c92ba932859d6ba4e0ad10e18feccbade1cfee0957597b3463', - getOpsxContinueCommandTemplate: 'e50e50266efa1b8e64ff9b6274ee8254f0a240d6adc1b862d126e2f1c9d3a559', - getOpsxApplyCommandTemplate: 'e3579ac78f2e2c75fa3d3a7ac7dc3e49c395e96f7323398f0f041d94f8de9bb0', - getOpsxFfCommandTemplate: '21132fc9c6d3b3ab2d2295d6bbd72d1e0052eb35ea1be0258c8b1ab3e200c4db', - getArchiveChangeSkillTemplate: '56bfada1a5f35a127791b70de9d428a75b5aedd1584d6c9803a1ecb1fd1b4a23', - getBulkArchiveChangeSkillTemplate: '93875998cade5322d95b43299fba794bc1da754e917dd63a770406386a6d295d', - getOpsxSyncCommandTemplate: '0d2427efb79986e8fff3f96bd075a739c80d45eb29159fae717e950030da8202', - getVerifyChangeSkillTemplate: '223b7ffd99299a7d430e13092b9a0a3421b39f0d3217232f46c39d79b5f619ff', - getOpsxArchiveCommandTemplate: '9f973c819b11620985b03322945f0e0a92a02a2ef455b94e74482f5e6292ac5d', - getOpsxOnboardCommandTemplate: 'ee99aa99252c602720fbb8c63fb3ac438a5bd4e952fd961ddf1ae956cbfc2c8f', - getOpsxBulkArchiveCommandTemplate: '9fa8cdebe2f5667ebfc37bdc023396762c59d5b038c771dac2d8fd2c19e2627b', - getOpsxVerifyCommandTemplate: '1efcf7eff0671f48e9d9420f50865c563dd3079ee60f8c380bb7a90dd0102696', - getOpsxProposeSkillTemplate: '9c0fbf0137151bd03ec30c45180f83daec96e8976ceaf517c63147f84b803446', - getOpsxProposeCommandTemplate: 'b3c145f541dcc13d9859eae8f7bedbe4553371477ed2c5ac07a4a80f82c46f52', + getExploreSkillTemplate: '5866ff47a73ae83523c9d6846fa0524c060cc0b10408b4e0d7f5ed7a1d1b3055', + getNewChangeSkillTemplate: 'e32224f45cfadf3d8ca74e0f0eb27ffb826f9508c0a30ff597846c7f49016aed', + getContinueChangeSkillTemplate: '20ddf6b8131bedcf8cb64925062f37eedd649836f680e5de49705f09f4961824', + getApplyChangeSkillTemplate: '12ed95c079bbbe2f9f114854b7cac59b5ff93236292ba680c883ac91a3e8a8d4', + getFfChangeSkillTemplate: 'ce6c292cce0e26aee23f31fd1a61fa1869d5f9ad11f114f2d8fbc2071308bfa7', + getSyncSpecsSkillTemplate: 'c135a7a1f636a154d74caa54327fab04cfabfb3549e2d82b31776f443010e52f', + getOnboardSkillTemplate: 'f91615491da1bc1b08505eee28b199acbf7041fb6e6b841adeed50ac8138b2d7', + getOpsxExploreCommandTemplate: 'f052e24ca5efa83bbecd26090d1536878888f9752883046bae57f74e771fcbc3', + getOpsxNewCommandTemplate: '243cef36241e576095ebb41108b5624c624af97a1f877b26e1a95fd7082db8a7', + getOpsxContinueCommandTemplate: 'b6d7b6336659b35e69e657afa58cfa1601aedb8543b0380302d1a2467149ca5c', + getOpsxApplyCommandTemplate: '061b9e26ee81414a547f7920dd91b16a052b8e59e4aa767cf9103047386e7e74', + getOpsxFfCommandTemplate: 'a7e65e4eb0941bd4f2231347fd7080784d55c04cee21fa1691b477dfabb40e9e', + getArchiveChangeSkillTemplate: '49d670ca69a4dc48f5db8a18005abf08f43000666e70963e21f2c46cca46e543', + getBulkArchiveChangeSkillTemplate: '310ba62a8c3769925ebe7b12f46c27a1a93446ddcfa3903ec8085aaf2da24b31', + getOpsxSyncCommandTemplate: 'bf2e18a079e6a60a341d2eac5574396a5796cfe9f6cc503fb5635d42f91323dd', + getVerifyChangeSkillTemplate: 'ededd72f473fdde4e0466c10d41f81a3b7f3bd4f3d3da8d351b4fe7d0a2b56b6', + getOpsxArchiveCommandTemplate: 'ad3a85d957429374cb61b7e77760d51e1cb41fd86a1efb53d33c67c30c265aec', + getOpsxOnboardCommandTemplate: 'eadf07b8ae5215923cc8e398d0cfae6f5e319e337e37df74df287dda8e664e46', + getOpsxBulkArchiveCommandTemplate: '02a1176fa3183704c0975ece62ff690af9c8338a45d71134fbd0e92daf0bdf77', + getOpsxVerifyCommandTemplate: '555fe9158d31fde1806484711713633b392fcfc27e6ba69722385a7f5a2c2cca', + getOpsxProposeSkillTemplate: '9ba481f5710a67cf83861ce9a9fbb0e0be1bc1e73a2fe5901813e464b67bf079', + getOpsxProposeCommandTemplate: '3cc80f65739184446a04842a4fdb3c0209eb5ce2825201909a0e599012a1a9cc', getFeedbackSkillTemplate: 'dabeb5e825b9349abc8156c3e7b8608f27987912a6d9bf47ef29addde6138133', - getUpdateChangeSkillTemplate: '7dc8abc6f64c58bf34d7581ed4ab095a3b7a53cb372349bee2d840db58622819', - getOpsxUpdateCommandTemplate: 'e2388521b22f92f74561df9a0c2f98e1fa4d265af93b5ba26f42fb47a6c5bfed', + getUpdateChangeSkillTemplate: '45c1e97de46c52cafef9d5e12a5ed96fe4264d8c978404b8e3c3ee519067c18b', + getOpsxUpdateCommandTemplate: '3a52e1495abbb6b9f50efb6651b95fd53fd1c4fd329deb828e2962cc26e24843', }; const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { - 'openspec-explore': 'dd84af68d3c93b40659dcdd8d383423b25b443cacdc4b514cd70614ae10c5cac', - 'openspec-new-change': 'ec4529beef978e34634a6f7286fab55d68fad8fb374dceb45691d52caab33fbb', - 'openspec-continue-change': 'bb6194a16c54891cdb253678e8f70ce53b2af86735243980f366ce551d37e42e', - 'openspec-apply-change': '81ea96d9fa6ec8536cd23c1fe561ed28e1cc1cad0a8ceb700588e08974cc0e49', - 'openspec-ff-change': '31355250514bce51b16ff37ee2b833bc9d475cd0dbd4b1f68fe2041694575623', - 'openspec-sync-specs': 'd933d8856584d6c1253de91e652e7aee9e85c77ad4d3531f6476f79d84e6e5e8', - 'openspec-archive-change': '7c65053d674ba4e1e20e2bf73ba7e5a7f94baef2eaa9b33cee48d4cadea51b7a', - 'openspec-bulk-archive-change': '2039b9ecf6e64339dffe0e16272507a386d9fe326f419ff758315aa736fdd96c', - 'openspec-verify-change': 'af9be013dcbe8c6d8f6d9ab10c893fbd03f4c62933c384d82f63894dd0ceb84f', - 'openspec-onboard': 'f6f59476acaf5e4d65dbb180da4cef62432612f3cecf207d471a951295e2003a', - 'openspec-propose': 'e358b45102a88082cf20f5c4441cba02533724ad6eef8ed15ba174e3496cb6ed', - 'openspec-update-change': '586547406aca94422dfeb3ffedce6c01049429b743f57ce829baa79ebc714d51', + 'openspec-explore': '2fa2c1d9d1e7b535a133b67b45cef7ca11d6cc769ace264b2711c36027151a9d', + 'openspec-new-change': '8e22baadbbf38ef5394c1ca598a1b49f1eba4e63f2dc6604c445f0dffa1b46d3', + 'openspec-continue-change': 'bf32daae9ac4e904be15dd40865f68a9c3370172ff4279ec32fde46e9aaca845', + 'openspec-apply-change': 'c8fc28e38f519156c61dc17db44dafdc4abecfdb814d83c801744e64a2197482', + 'openspec-ff-change': 'd95ffb998e62398c33193151f4f5e8a984b7c47f903776dd9628bd343c16ca3a', + 'openspec-sync-specs': '1b8fca8224cc6e2617dc225e54b5f35dddd67f0d773c79f7e84eeb6a4b82c304', + 'openspec-archive-change': '2f7628e22644ade9e5e59b0173044a6259a2ca63445e7ea1bb9e4384c407753a', + 'openspec-bulk-archive-change': 'f22045c417d243e6b2cf5c4bf3b6007b4e3bf27bf28de95a2c79cf97b9354924', + 'openspec-verify-change': 'd06e831bfe5d78979fdccebde4e615ced13f7c5c485b3bd6ab1ce90465b968f7', + 'openspec-onboard': 'c21e21a894e7a6dd03849846c04ce932bcd6ee835acc6712fa675cc938b3d104', + 'openspec-propose': 'c2696908254ee21b6f4500a05cb27ff9912a9d5d675e6799d3ee67fe69164285', + 'openspec-update-change': 'fc1a4b93e7310b0b30249c1b22e7bf0eb84dd5c523f0831d236be0067a0f8410', }; // Intentionally excludes getFeedbackSkillTemplate: this list only models templates From de03d4c11fbac7a99b916d4b35e6b34d2ef8dcbc Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 7 Sep 2026 08:56:25 -0500 Subject: [PATCH 2/6] docs(sync): document openspec sync and the CI gate Adds the `openspec sync` reference to both CLI docs, documents `list --status`, and gives the team workflow guide the section it was missing: how to enforce in CI that the specs describe what shipped, without a gate that is red for the whole life of every pull request. Also carries the dogfooded change and its capability specs. Co-Authored-By: Claude Opus 5 --- .changeset/sync-lifecycle-status.md | 11 ++ docs-lab/reference/cli.md | 96 ++++++++++++ docs/cli.md | 99 +++++++++++- docs/team-workflow.md | 32 ++++ .../add-standalone-spec-sync/.openspec.yaml | 2 + .../add-standalone-spec-sync/proposal.md | 69 +++++++++ .../specs/cli-list/spec.md | 29 ++++ .../specs/cli-sync/spec.md | 142 ++++++++++++++++++ .../changes/add-standalone-spec-sync/tasks.md | 25 +++ 9 files changed, 504 insertions(+), 1 deletion(-) create mode 100644 .changeset/sync-lifecycle-status.md create mode 100644 openspec/changes/add-standalone-spec-sync/.openspec.yaml create mode 100644 openspec/changes/add-standalone-spec-sync/proposal.md create mode 100644 openspec/changes/add-standalone-spec-sync/specs/cli-list/spec.md create mode 100644 openspec/changes/add-standalone-spec-sync/specs/cli-sync/spec.md create mode 100644 openspec/changes/add-standalone-spec-sync/tasks.md diff --git a/.changeset/sync-lifecycle-status.md b/.changeset/sync-lifecycle-status.md new file mode 100644 index 0000000000..78f7733879 --- /dev/null +++ b/.changeset/sync-lifecycle-status.md @@ -0,0 +1,11 @@ +--- +"@fission-ai/openspec": minor +--- + +Add `openspec sync`, which folds a change's delta specs into the main specs without archiving it, and an optional `status: proposed | shipped` field in a change's `.openspec.yaml`. + +`openspec sync --check` gates on one property: a change that claims to be shipped has its deltas in `specs/`. A proposed change passes for free, so the check is green as its resting state and red only on a real mistake — unlike a check for "is everything archived?", which is red for the whole life of every open pull request. It reads only files on disk, so a pre-commit hook, a pre-push hook and CI run the same command and agree. + +`openspec list --status ` filters changes by that field. + +Everything here is opt-in and inert by default. The `status` field is absent unless a project writes it, nothing generates it, and `archive` is unchanged. diff --git a/docs-lab/reference/cli.md b/docs-lab/reference/cli.md index e9e4eb14fd..85d6a2ec62 100644 --- a/docs-lab/reference/cli.md +++ b/docs-lab/reference/cli.md @@ -22,6 +22,7 @@ | [`openspec show`](#openspec-show) | Print a change or spec, as markdown or JSON. | | [`openspec view`](#openspec-view) | One-screen dashboard of specs and changes. | | [`openspec validate`](#openspec-validate) | Check changes and specs for structural issues. | +| [`openspec sync`](#openspec-sync) | Fold a change's delta specs into the main specs, without archiving it. | | [`openspec archive`](#openspec-archive) | Move a completed change to the archive and update the main specs. | **Workflows and schemas** @@ -410,6 +411,7 @@ Rows come from `openspec/changes/` and `openspec/specs/` under the resolved root | `--specs` | List specs instead of changes. | | `--changes` | List changes. This is the default. | | `--sort ` | `recent` (last modified first) or `name`. Default: `recent`. Specs always sort by name. | +| `--status ` | Only list changes in this lifecycle state: `proposed` or `shipped`. A change with no `status` in its `.openspec.yaml` counts as `proposed`. | | `--json` | Print JSON instead of the table. | | `--store ` | Use a registered store as the OpenSpec root instead of the current project. | @@ -867,6 +869,100 @@ exit $validationExit These custom views keep the full report's keys but omit clean items. They are neither complete full-v1 reports nor the versioned `--report findings` shape. +## openspec sync + +Folds a change's delta specs into the main specs, without archiving the change. + +```bash +openspec sync add-rate-limit # fold one change now; nothing moves +openspec sync add-rate-limit --ship # mark it shipped and fold it, in one diff +openspec sync # fold every change declaring status: shipped +openspec sync --check # exit 1 if a shipped change has unfolded deltas +``` + +`archive` folds and moves in one step, so the fold can only happen at the moment the +change is finished. `sync` separates them: the specs can be brought up to date while +the change is still open, and CI can check that they are. + +**Arguments** + +| Argument | What it is | +|---|---| +| `change-name` | The change to sync. Omitted, every change declaring `status: shipped` | + +**Options** + +| Flag | Effect | +|---|---| +| `--check` | Report shipped changes with unfolded deltas and exit 1. Writes nothing. | +| `--ship` | Set `status: shipped` on the named change, then fold it. | +| `-y, --yes` | Sync even when the change has incomplete tasks. | +| `--no-validate` | Skip validation. | +| `--json` | Print a structured result instead of text. | +| `--store ` | Use a registered store as the OpenSpec root. | + +**The lifecycle field** + +A change may declare where it sits, in its `.openspec.yaml`: + +```yaml +schema: spec-driven +status: shipped +``` + +Optional and absent by default. No `status` means `proposed`, which is what a change +under `changes/` has always meant. Nothing writes the field on its own. + +**The gate** + +`openspec sync --check` asserts that a change claiming to be shipped has its deltas in +`specs/`. A proposed change passes for free, so green is the resting state: + +``` +✓ 1 shipped change(s) are folded into the main specs. +``` + +and red names both the gap and the fix: + +``` +Sync check failed: + + add-rate-limit + api: +1 not applied + +Run openspec sync to fold them, then commit the result. +``` + +It reads only files on disk — no VCS history, no timing — so a pre-commit hook, a +pre-push hook and CI run the same command and agree. + +**Output** + +``` +Applying changes to openspec/specs/api/spec.md: + + 1 added +Totals: + 1, ~ 0, - 0, → 0 +Specs updated successfully. +``` + +Running it again reports `Specs already in sync; no files changed.` — and so does +`openspec archive` afterwards, because re-applying a folded delta is a no-op. + +**What it will not do** + +Sync never deletes a spec. When a change's `REMOVED` entries take a capability's last +requirement, retiring it deletes the file, which stays with `openspec archive` behind +the `retire_capabilities` marker. Sync reports the case and names archive instead. + +Sync also never examines archived changes: their deltas are history, superseded by +whatever came after. + +**Exit codes** + +- `0`: the specs were folded, or `--check` found nothing wrong. +- `1`: `--check` found an unfolded shipped change, validation failed, tasks were + incomplete, or the change was not found. + ## openspec archive Moves a completed change to the archive and updates the main specs. diff --git a/docs/cli.md b/docs/cli.md index e74bc60f2d..05d908514b 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -13,7 +13,7 @@ The OpenSpec CLI (`openspec`) provides terminal commands for project setup, vali | **Personal worksets** | `workset create`, `workset list`, `workset open`, `workset remove` | Keep and open personal, local working views in your tool | | **Browsing** | `list`, `view`, `show` | Explore changes and specs | | **Validation** | `validate` | Check changes and specs for issues | -| **Lifecycle** | `archive` | Finalize completed changes | +| **Lifecycle** | `sync`, `archive` | Fold delta specs into the main specs, and finalize completed changes | | **Workflow** | `new change`, `status`, `instructions`, `templates`, `schemas` | Artifact-driven workflow support | | **Schemas** | `schema init`, `schema fork`, `schema validate`, `schema which` | Create and manage custom workflows | | **Config** | `config` | View and modify settings | @@ -443,6 +443,7 @@ openspec list [options] | `--specs` | List specs instead of changes | | `--changes` | List changes (default) | | `--sort ` | Sort by `recent` (default) or `name` | +| `--status ` | Only list changes in this lifecycle state: `proposed` or `shipped`. A change whose `.openspec.yaml` has no `status` counts as `proposed` | | `--json` | Output as JSON | **Examples:** @@ -626,6 +627,102 @@ Validating add-dark-mode... ## Lifecycle Commands +### `openspec sync` + +Fold a change's delta specs into the main specs, without archiving the change. + +``` +openspec sync [change-name] [options] +``` + +`archive` does two things at once: it folds a change's deltas into `openspec/specs/` +and it moves the change folder. `sync` does only the first, so the specs can be +brought up to date while the change is still open for review — and so CI can check +that they are. + +**Arguments:** + +| Argument | Required | Description | +|----------|----------|-------------| +| `change-name` | No | Change to sync. Omitted, `sync` acts on every change that declares `status: shipped` | + +**Options:** + +| Option | Description | +|--------|-------------| +| `--check` | Report shipped changes whose deltas are not in the main specs and exit 1. Writes nothing | +| `--ship` | Set `status: shipped` on the named change, then fold it — one command, one diff | +| `-y, --yes` | Sync even when the change still has incomplete tasks | +| `--no-validate` | Skip validation (not recommended) | +| `--json` | Structured output for hooks and CI | +| `--store ` | Use a registered store as the OpenSpec root | + +**The lifecycle field.** A change's `.openspec.yaml` may declare where it sits: + +```yaml +schema: spec-driven +status: shipped # or: proposed +``` + +The field is optional and absent by default. A change with no `status` is +`proposed`, which is what every change under `changes/` has always meant, so a +project that never opts in is unaffected. Nothing writes the field on its own — +not `openspec new change`, not `archive`. + +**The CI gate.** `openspec sync --check` asserts one property: *a change that +claims to be shipped has its deltas in `specs/`*. A proposed change passes for +free, so the check is green as its resting state and red only on a real mistake — +unlike "is everything archived?", which is red for the entire life of every open +PR. It reads only files on disk, so a pre-commit hook, a pre-push hook and CI run +the same command and reach the same verdict. + +```bash +# CI, pre-commit, pre-push — same command +openspec sync --check +``` + +**Examples:** + +```bash +# Fold one change's deltas now; the change stays where it is +openspec sync add-rate-limit + +# Mark it shipped and fold it in a single commit +openspec sync add-rate-limit --ship + +# Fold every change that declares status: shipped +openspec sync + +# Gate: exits 1 if any shipped change has unfolded deltas +openspec sync --check + +# Which changes have claimed to be shipped but aren't archived yet +openspec list --status shipped +``` + +**What it does:** + +1. Validates the change's delta specs (unless `--no-validate`) +2. Refuses a change with incomplete tasks, unless `--yes` — folding a change + nothing implements yet writes requirements into `specs/` that aren't true +3. Validates every rebuilt spec before writing any of them, so a late failure + leaves the whole tree unchanged +4. Writes the updated main specs. Nothing moves; nothing is deleted + +**What it deliberately does not do:** + +- **It never deletes a spec.** When a change's `REMOVED` entries take a + capability's last requirement, retiring that capability deletes its `spec.md`. + That stays with `openspec archive`, behind the `retire_capabilities` marker. + `sync` reports the case and points you there. +- **It never checks archived changes.** Archived deltas are history, and later + changes supersede them. `--check` looks only at active changes that declare + `status: shipped` — a set that drains itself as those changes archive. + +**Syncing early does not change archiving.** Re-applying a delta that is already +in the main specs is a no-op, so `openspec archive` afterwards reports +`Specs already in sync` and moves the folder exactly as it always did. + ### `openspec archive` Archive a completed change and merge delta specs into main specs. diff --git a/docs/team-workflow.md b/docs/team-workflow.md index 76c83817b8..2d9b60eb7d 100644 --- a/docs/team-workflow.md +++ b/docs/team-workflow.md @@ -55,6 +55,38 @@ Archiving folds a change's deltas into your main `openspec/specs/` and moves the Pick one and be consistent. Either way, `/opsx:archive` checks that tasks are complete and offers to sync first, so nothing merges half-finished by accident. +## Enforcing it in CI + +The obvious CI check — "nothing is left unarchived" — doesn't work, because it's red for the whole life of every PR. An open change sits in `changes/`, unarchived, precisely because it isn't finished. A gate that is red as its resting state is one everyone learns to ignore. + +`openspec sync --check` is the check that works. It asks a different question: **does anything that claims to be shipped still have deltas missing from `specs/`?** A change that hasn't made that claim passes for free, so green is the resting state and red means a real mistake. + +```yaml +# .github/workflows/specs.yml +- run: npx openspec sync --check +``` + +The claim is one line in the change's `.openspec.yaml`: + +```yaml +schema: spec-driven +status: shipped +``` + +The everyday shape of it: + +1. Open the PR. The change is `proposed` (the default — nothing to write). The gate is green. +2. When the work is done and reviewed, mark it shipped and fold its deltas in one step: + ```bash + openspec sync add-rate-limit --ship + ``` + That sets `status: shipped` and writes the deltas into `specs/` in a single commit, so no intermediate commit claims a change shipped while the specs say otherwise. +3. Merge. Archive whenever you like afterwards — re-applying a delta that's already folded is a no-op, so `openspec archive` behaves exactly as it always did. + +The check is a pure function of the files on disk, so the same command works as a pre-commit hook, a pre-push hook, and the CI gate, and all three agree. + +`openspec list --status shipped` shows which changes have made the claim but aren't archived yet. + ## Two people, parallel changes Because changes are separate folders, they don't collide: diff --git a/openspec/changes/add-standalone-spec-sync/.openspec.yaml b/openspec/changes/add-standalone-spec-sync/.openspec.yaml new file mode 100644 index 0000000000..2e24cfa4fa --- /dev/null +++ b/openspec/changes/add-standalone-spec-sync/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-07 diff --git a/openspec/changes/add-standalone-spec-sync/proposal.md b/openspec/changes/add-standalone-spec-sync/proposal.md new file mode 100644 index 0000000000..9058621715 --- /dev/null +++ b/openspec/changes/add-standalone-spec-sync/proposal.md @@ -0,0 +1,69 @@ +# Let a change's specs be folded before it is archived + +## Why + +`archive` does two separable jobs in one command. It folds a change's deltas into +`openspec/specs/`, and it declares the change finished by moving its directory. +Welding them means the fold can only happen at the moment the move happens, which +on a team that reviews before merging is after the pull request closes. + +So a team that wants CI to assert "the specs describe what shipped" has nothing to +assert during review. The only property expressible today is "nothing is left +unarchived", and that is violated by design for the entire life of every open PR: +the change sits in `changes/`, unarchived, precisely because it is not finished. +A gate that is red as its resting state is one everyone learns to ignore, and it +masks the real failures underneath (#1683). + +The fix is to make the check conditional on the change's own claim — not "is +everything archived?" but "does anything claiming to be shipped still have deltas +missing from the specs?" A proposed change passes for free, so green is the +resting state and red means a real mistake. + +## What Changes + +- **`openspec sync [change]`** folds delta specs into the main specs without + archiving. The merge engine already supports this: re-applying a folded delta + is a no-op it names the "early-sync pattern", so `archive` afterwards behaves + exactly as it always did. +- **`openspec sync --check`** asserts `shipped ⇒ folded` over the working tree and + exits 1 with the offending changes named. A pure function of files on disk, so + a pre-commit hook, a pre-push hook and CI run one command and agree. +- **`status: proposed | shipped`** becomes an optional field in a change's + `.openspec.yaml`. Absent means `proposed`, which is what a change under + `changes/` has always meant. Nothing writes it: not `new change`, not `archive`. +- **`openspec sync --ship`** sets the field and folds in one working-tree + diff, so no intermediate commit claims a change is shipped while the specs say + otherwise. +- **`openspec list --status `** filters by the field, and renders a + lifecycle column only when some change in the root declares one. + +Two deliberate limits, both to keep this additive rather than a second lifecycle: + +- **Sync never deletes a spec.** Retiring a capability is the one irreversible + operation in the system; it stays with `archive`, behind the + `retire_capabilities` marker and its rollback-safe deletion. Sync reports the + case and names archive. +- **Sync never examines archived changes.** Their deltas are history and later + changes supersede them; re-applying a months-old delta over everything that + came after is a merge conflict, not a drift check. The checked set is the + active changes declaring `shipped`, which drains itself as they archive. + +"Folded" is decided by running the merge builder and seeing that it applied zero +operations — the same predicate `archive` uses to decide it has nothing to write. +Not a byte-comparison of the rebuilt output: the rebuild normalizes blank lines, +so a hand-formatted main spec would compare unequal while being perfectly in +sync. Sharing archive's own predicate is also what stops the checker and the doer +from drifting apart (#1112). + +## Impact + +- Affected specs: `cli-sync` (ADDED), `cli-list` (MODIFIED: filtering) +- Affected code: `src/core/sync.ts` (new), `src/core/list.ts`, + `src/utils/change-metadata.ts`, `src/core/change-metadata/schema.ts`, + `src/cli/index.ts`, `src/core/completions/command-registry.ts`, + `src/core/archive.ts` (two helpers exported, no behavior change) +- Affected docs: `docs/cli.md`, `docs/team-workflow.md`, + `docs-lab/reference/cli.md` + +Credit: the diagnosis and the `shipped ⇒ folded` framing are from Matan Bendix +Shenhav's proposal in #1683. diff --git a/openspec/changes/add-standalone-spec-sync/specs/cli-list/spec.md b/openspec/changes/add-standalone-spec-sync/specs/cli-list/spec.md new file mode 100644 index 0000000000..e80d7cc220 --- /dev/null +++ b/openspec/changes/add-standalone-spec-sync/specs/cli-list/spec.md @@ -0,0 +1,29 @@ +## ADDED Requirements + +### Requirement: Lifecycle Status Filtering +The command SHALL be able to filter changes by their declared lifecycle state, and +SHALL surface that state without changing the output of a project that has never +declared one. + +#### Scenario: Filtering by state +- **WHEN** `openspec list --status shipped` is executed +- **THEN** only changes declaring `status: shipped` SHALL be listed +- **AND** `--status proposed` SHALL list every change that declares `proposed` or + declares no status at all + +#### Scenario: An unknown state is rejected +- **WHEN** `--status` is given a value other than `proposed` or `shipped` +- **THEN** the command SHALL exit 1 naming the accepted values +- **AND** SHALL NOT list every change as though the filter matched nothing + +#### Scenario: No lifecycle output without a declaration +- **WHEN** no change in the root declares a `status` +- **THEN** the human listing SHALL render no lifecycle column +- **AND** the JSON output SHALL carry no lifecycle key + +#### Scenario: The lifecycle appears once any change declares one +- **WHEN** at least one change declares a `status` +- **THEN** the human listing SHALL render a lifecycle column, showing `proposed` + for changes that declare nothing +- **AND** the JSON output SHALL carry a `lifecycle` key for the declaring changes + only, leaving the existing `status` key meaning task progress diff --git a/openspec/changes/add-standalone-spec-sync/specs/cli-sync/spec.md b/openspec/changes/add-standalone-spec-sync/specs/cli-sync/spec.md new file mode 100644 index 0000000000..5854223a1d --- /dev/null +++ b/openspec/changes/add-standalone-spec-sync/specs/cli-sync/spec.md @@ -0,0 +1,142 @@ +# Sync Command Specification + +## Purpose + +The `openspec sync` command SHALL fold a change's delta specs into the main specs +without archiving the change, and SHALL provide a check that a change claiming to +be shipped has its deltas present in the main specs. + +## ADDED Requirements + +### Requirement: Lifecycle Status Field +A change SHALL be able to declare its lifecycle state as data in its +`.openspec.yaml`, using an optional `status` field whose value is `proposed` or +`shipped`. A change that does not declare one SHALL be treated as `proposed`. + +#### Scenario: Undeclared status reads as proposed +- **WHEN** a change's `.openspec.yaml` has no `status` field, or the change has no + metadata file at all +- **THEN** every reader SHALL treat the change as `proposed` +- **AND** no command SHALL write the field on the change's behalf + +#### Scenario: A status that cannot be determined is not rounded to proposed +- **WHEN** a change's metadata mentions `status` but cannot be honored, because the + file does not parse, carries an unknown value, or names a schema that does not + resolve +- **THEN** the state SHALL be reported as undetermined with its reason +- **AND** `openspec sync --check` SHALL fail rather than pass the change + +#### Scenario: Broken metadata that never mentions status is left alone +- **WHEN** a change's metadata cannot be honored and does not mention `status` +- **THEN** the change SHALL read as `proposed` +- **AND** `openspec sync --check` SHALL NOT report it + +### Requirement: Folding Delta Specs +The command SHALL apply a change's delta specs to the main specs, leaving the +change directory where it is. + +#### Scenario: Folding a named change +- **WHEN** `openspec sync ` is executed +- **THEN** each delta under the change's `specs/` SHALL be applied to its main spec +- **AND** the change directory SHALL NOT be moved +- **AND** the change's declared status SHALL NOT affect whether it is folded + +#### Scenario: Folding every shipped change +- **WHEN** `openspec sync` is executed with no change name +- **THEN** every active change declaring `status: shipped` SHALL be folded +- **AND** a change declaring no status SHALL NOT be folded + +#### Scenario: Folding is idempotent +- **WHEN** `openspec sync` is run against a change whose deltas are already in the + main specs +- **THEN** no file SHALL be written +- **AND** the command SHALL report that the specs are already in sync + +#### Scenario: Archiving after a sync is unaffected +- **WHEN** a change is folded by `openspec sync` and later archived +- **THEN** `openspec archive` SHALL apply zero operations and write no spec file +- **AND** the change SHALL be moved to the archive as it always was + +### Requirement: Shipped Changes Are Folded +The command SHALL provide a check that asserts one property over the working tree: +every change claiming to be shipped has its deltas present in the main specs. + +#### Scenario: A proposed change passes for free +- **WHEN** `openspec sync --check` is executed and no active change declares + `status: shipped` +- **THEN** the command SHALL exit 0 +- **AND** SHALL write no file + +#### Scenario: A shipped change with unfolded deltas fails the check +- **WHEN** `openspec sync --check` is executed and an active change declaring + `status: shipped` has a delta that is not in its main spec +- **THEN** the command SHALL exit 1 +- **AND** SHALL name the change and each capability whose delta is unapplied +- **AND** SHALL name the command that folds them +- **AND** SHALL write no file + +#### Scenario: Folded-ness is decided by the merge builder +- **WHEN** deciding whether a change's deltas are present in the main specs +- **THEN** the decision SHALL be that re-applying the delta produces zero applied + operations, which is the same predicate the archive command uses to decide it + has nothing to write +- **AND** SHALL NOT be a byte comparison against a rebuilt spec + +#### Scenario: Archived changes are never examined +- **WHEN** `openspec sync --check` is executed +- **THEN** only active changes SHALL be examined +- **AND** a change that has been archived SHALL NOT be checked + +### Requirement: Sync Never Deletes A Spec +The command SHALL NOT delete a main spec under any circumstance. Retiring a +capability remains the archive command's operation. + +#### Scenario: A retirement is handed to archive +- **WHEN** a change's REMOVED entries would take a capability's last requirement +- **THEN** `openspec sync` SHALL refuse to fold that change +- **AND** SHALL name `openspec archive` as the command that performs a retirement +- **AND** the main spec file SHALL remain on disk + +#### Scenario: The check reports a retirement without offering sync as the fix +- **WHEN** `openspec sync --check` finds a shipped change that would retire a + capability +- **THEN** the command SHALL exit 1 naming the retirement +- **AND** SHALL NOT tell the user to run `openspec sync` + +### Requirement: Guards Before Writing +The command SHALL run the same guards the archive command runs before it writes a +main spec. + +#### Scenario: Delta specs are validated +- **WHEN** a change's delta specs fail validation and `--no-validate` was not passed +- **THEN** the command SHALL refuse the change and write no file + +#### Scenario: Incomplete tasks block the fold +- **WHEN** a change has incomplete tasks and `--yes` was not passed +- **THEN** the command SHALL refuse the change and write no file +- **AND** SHALL name the rerun that proceeds anyway + +#### Scenario: Every rebuilt spec is validated before any is written +- **WHEN** any rebuilt spec would fail validation +- **THEN** no spec file SHALL be written at all + +#### Scenario: A fold that does not settle is named +- **WHEN** two shipped changes claim the same requirement in ways that cannot both + hold, so re-evaluating after the write still reports unfolded deltas +- **THEN** the command SHALL name the changes involved +- **AND** SHALL NOT retry the fold + +### Requirement: Shipping In One Diff +The command SHALL offer to set a change's status and fold it in a single run, so +that no intermediate commit claims a change is shipped while its deltas are absent +from the main specs. + +#### Scenario: Marking a change shipped and folding it +- **WHEN** `openspec sync --ship` is executed +- **THEN** the change's `.openspec.yaml` SHALL be set to `status: shipped` +- **AND** its deltas SHALL be folded in the same run +- **AND** the metadata file's comments and key order SHALL be preserved + +#### Scenario: Ship is refused where it cannot apply +- **WHEN** `--ship` is passed with `--check`, or with no change name +- **THEN** the command SHALL refuse and say which flag combination is valid diff --git a/openspec/changes/add-standalone-spec-sync/tasks.md b/openspec/changes/add-standalone-spec-sync/tasks.md new file mode 100644 index 0000000000..56ee1301ef --- /dev/null +++ b/openspec/changes/add-standalone-spec-sync/tasks.md @@ -0,0 +1,25 @@ +## 1. Lifecycle field + +- [x] 1.1 Add optional `status: proposed | shipped` to `ChangeMetadataSchema` +- [x] 1.2 Add `readChangeStatus`, failing closed on metadata it cannot honor +- [x] 1.3 Add `writeChangeStatus`, preserving comments and key order + +## 2. Sync command + +- [x] 2.1 Add `src/core/sync.ts` with the fold and the `--check` predicate +- [x] 2.2 Run archive's guards before writing: validation, task completion, + rebuilt-spec validation +- [x] 2.3 Refuse retirements and name `openspec archive` instead +- [x] 2.4 Re-evaluate after writing so a non-convergent pair is named, not looped on +- [x] 2.5 Register the CLI command and its completion entry + +## 3. List filter + +- [x] 3.1 Add `--status `, counting an undeclared change as `proposed` +- [x] 3.2 Render the lifecycle column and the JSON `lifecycle` key only when declared + +## 4. Docs and verification + +- [x] 4.1 Document `openspec sync` and `list --status` +- [x] 4.2 Add the CI section to the team workflow guide +- [x] 4.3 Tests covering the gate, the guards, and the archive interaction From 7b542c247ef35c84e6bb56ebae8bcfce1f17efca Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 7 Sep 2026 09:18:01 -0500 Subject: [PATCH 3/6] fix(sync): address review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `--ship` stamped `status: shipped` before the guards ran, so a change refused for incomplete tasks or failing validation was left claiming to be shipped with its deltas absent — the exact state the flag exists to prevent, with the gate red until the metadata was hand-edited back. The stamp now happens after every guard passes and before the writes. - A failed write part way through a multi-capability fold left some main specs updated and others not. The previous bytes of every target are captured and restored on failure, and the error names anything that could not be put back. Simpler than archive's equivalent: sync only writes, so there is no retirement to undo and no move to unwind. - `openspec list --specs --status shipped` accepted the flag and listed every spec unfiltered. The combination is rejected. - A change whose lifecycle status could not be determined matched both `--status proposed` and `--status shipped`. A filter is a claim of membership, and membership cannot be established for it, so it now matches neither — it stays visible in the unfiltered listing, and `openspec sync --check` is where the broken file gets named. - Docs said `--ship` lands both edits "in a single commit". OpenSpec never runs git; the wording now says one command, commit the result. Co-Authored-By: Claude Opus 5 --- docs-lab/reference/cli.md | 2 +- docs/cli.md | 4 +- docs/team-workflow.md | 2 +- src/cli/index.ts | 6 +++ src/core/list.ts | 13 +++-- src/core/sync.ts | 97 ++++++++++++++++++++++++++++++---- test/commands/sync-cli.test.ts | 63 ++++++++++++++++++++++ test/core/list.test.ts | 25 +++++++++ test/core/sync.test.ts | 70 ++++++++++++++++++++++++ 9 files changed, 263 insertions(+), 19 deletions(-) create mode 100644 test/commands/sync-cli.test.ts diff --git a/docs-lab/reference/cli.md b/docs-lab/reference/cli.md index 85d6a2ec62..ccba3825ee 100644 --- a/docs-lab/reference/cli.md +++ b/docs-lab/reference/cli.md @@ -875,7 +875,7 @@ Folds a change's delta specs into the main specs, without archiving the change. ```bash openspec sync add-rate-limit # fold one change now; nothing moves -openspec sync add-rate-limit --ship # mark it shipped and fold it, in one diff +openspec sync add-rate-limit --ship # mark it shipped and fold it, in one set of changes openspec sync # fold every change declaring status: shipped openspec sync --check # exit 1 if a shipped change has unfolded deltas ``` diff --git a/docs/cli.md b/docs/cli.md index 05d908514b..d8ca06d5b3 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -651,7 +651,7 @@ that they are. | Option | Description | |--------|-------------| | `--check` | Report shipped changes whose deltas are not in the main specs and exit 1. Writes nothing | -| `--ship` | Set `status: shipped` on the named change, then fold it — one command, one diff | +| `--ship` | Set `status: shipped` on the named change, then fold it — both land in one set of file changes for you to commit | | `-y, --yes` | Sync even when the change still has incomplete tasks | | `--no-validate` | Skip validation (not recommended) | | `--json` | Structured output for hooks and CI | @@ -687,7 +687,7 @@ openspec sync --check # Fold one change's deltas now; the change stays where it is openspec sync add-rate-limit -# Mark it shipped and fold it in a single commit +# Mark it shipped and fold it, so both land in one commit when you make it openspec sync add-rate-limit --ship # Fold every change that declares status: shipped diff --git a/docs/team-workflow.md b/docs/team-workflow.md index 2d9b60eb7d..6b240fa18a 100644 --- a/docs/team-workflow.md +++ b/docs/team-workflow.md @@ -80,7 +80,7 @@ The everyday shape of it: ```bash openspec sync add-rate-limit --ship ``` - That sets `status: shipped` and writes the deltas into `specs/` in a single commit, so no intermediate commit claims a change shipped while the specs say otherwise. + That sets `status: shipped` and writes the deltas into `specs/` in one command, so both land in the same set of file changes for you to commit together. OpenSpec never runs git itself — commit the result as usual. 3. Merge. Archive whenever you like afterwards — re-applying a delta that's already folded is a no-op, so `openspec archive` behaves exactly as it always did. The check is a pure function of the files on disk, so the same command works as a pre-commit hook, a pre-push hook, and the CI gate, and all three agree. diff --git a/src/cli/index.ts b/src/cli/index.ts index f7bbbf8ecc..9867b00805 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -383,6 +383,12 @@ program `Unknown --status '${options.status}'. Use 'proposed' or 'shipped'.` ); } + // A lifecycle state belongs to a change, not a spec, so the flag has + // nothing to filter in specs mode. Silently ignoring it would print the + // full spec list as though the filter had matched everything. + if (options?.status !== undefined && mode === 'specs') { + throw new Error('--status filters changes and cannot be combined with --specs.'); + } await listCommand.execute(root.path, mode, { sort, json: options?.json, diff --git a/src/core/list.ts b/src/core/list.ts index eb608136ca..9e60b4692d 100644 --- a/src/core/list.ts +++ b/src/core/list.ts @@ -131,11 +131,16 @@ export class ListCommand { for (const changeDir of changeDirs) { const changePath = path.join(changesDir, changeDir); // Undeclared reads as `proposed`, which is what a change under - // `changes/` has always meant. Metadata that cannot be read is left - // unfiltered rather than guessed at - `openspec sync --check` and - // `openspec status` are where a broken file gets reported. + // `changes/` has always meant. + // + // A change whose metadata cannot be honored matches NEITHER filter. A + // filter is a claim of membership, and membership cannot be + // established here - listing it under both `--status proposed` and + // `--status shipped` states something false in one of the two. It stays + // visible in the unfiltered listing, and `openspec sync --check` is + // where the broken file gets named. const marker = readChangeStatus(changePath); - if (statusFilter && !marker.invalidReason && marker.status !== statusFilter) { + if (statusFilter && (marker.invalidReason || marker.status !== statusFilter)) { continue; } const progress = await getTaskProgressForChange(changesDir, changeDir, targetPath); diff --git a/src/core/sync.ts b/src/core/sync.ts index fb5d805af7..60de3d72f1 100644 --- a/src/core/sync.ts +++ b/src/core/sync.ts @@ -392,15 +392,6 @@ export class SyncCommand { return result; } - // `--ship` before evaluation, so the fold that follows is measured against - // the state the commit will carry: the field flip and the spec write land - // in one working-tree diff, and no intermediate commit can claim a change - // is shipped while its deltas are not in the specs. - if (options.ship) { - writeChangeStatus(path.join(changesDir, targets[0]), 'shipped'); - if (!json) console.log(`Marked '${targets[0]}' as shipped.`); - } - const evaluations: Evaluation[] = []; for (const name of targets) { evaluations.push( @@ -588,10 +579,32 @@ export class SyncCommand { } } + // Every guard has passed, so this is the first point at which the change is + // known to be foldable. Stamping earlier would leave the working tree in the + // exact state this flag exists to prevent: a change claiming `shipped` while + // its deltas are absent from the specs, with the gate red until someone + // hand-edits the metadata back. + if (options.ship) { + const shipped = evaluations[0].report; + writeChangeStatus(path.join(changesDir, shipped.change), 'shipped'); + shipped.status = 'shipped'; + if (!json) console.log(`Marked '${shipped.change}' as shipped.`); + } + + const pending = evaluations.flatMap(({ writes }) => writes); + // Sync applies one change across several capabilities, so a failure part + // way through the loop would leave some main specs folded and others not. + // Re-running would finish the job - the fold is idempotent - but a tree + // nobody asked for is not a state to hand back, so the previous bytes are + // restored instead. Simpler than archive's equivalent because sync only + // ever writes: there is no retirement to undo and no directory move to + // unwind. + const snapshots = await captureTargets(pending.map((write) => write.update.target)); + const totals = { added: 0, modified: 0, removed: 0, renamed: 0 }; let wroteAny = false; - for (const { writes } of evaluations) { - for (const write of writes) { + try { + for (const write of pending) { await writeUpdatedSpec(write.update, write.rebuilt, write.counts, { silent: json, ...(isStoreSelectedRoot(root) ? { displayPath: write.update.target } : {}), @@ -602,6 +615,15 @@ export class SyncCommand { totals.removed += write.counts.removed; totals.renamed += write.counts.renamed; } + } catch (error) { + const restoreFailure = await restoreTargets(snapshots); + throw new SyncBlockedError( + 'sync_write_failed', + `Could not write the main specs: ${ + error instanceof Error ? error.message : String(error) + }.${restoreFailure ? ` ${restoreFailure}` : ' No spec was left partly folded.'}`, + restoreFailure ? 'Restore the named files from git, then rerun.' : undefined + ); } // Re-evaluate rather than assume. Folding is idempotent for a single @@ -723,6 +745,59 @@ export class SyncCommand { } } +interface TargetSnapshot { + target: string; + /** The bytes that were there, or undefined when the file did not exist. */ + content?: Buffer; +} + +/** Read the current bytes of each target so a failed write can be undone. */ +async function captureTargets(targets: string[]): Promise { + return Promise.all( + targets.map(async (target) => { + try { + return { target, content: await fs.readFile(target) }; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { target }; + throw error; + } + }) + ); +} + +/** + * Put every captured target back the way it was, in reverse order. + * + * Returns a sentence naming what could not be restored, or undefined when the + * tree is back to its original state. Never throws: it runs inside a failure + * path, and losing the original error to a rollback error would hide the cause. + */ +async function restoreTargets( + snapshots: TargetSnapshot[] +): Promise { + const failed: string[] = []; + for (const snapshot of [...snapshots].reverse()) { + try { + if (snapshot.content === undefined) { + // The file did not exist before this run, so the rollback is removing + // whatever was created. A missing file is already the desired state. + await fs.unlink(snapshot.target).catch((error) => { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + }); + } else { + // Written in place, exactly as writeUpdatedSpec does, so a symlinked or + // hard-linked spec keeps the semantics it had before the fold. + await fs.writeFile(snapshot.target, snapshot.content); + } + } catch { + failed.push(snapshot.target); + } + } + return failed.length > 0 + ? `These specs could not be restored and may hold partly folded content: ${failed.join(', ')}.` + : undefined; +} + function toDiagnostic(error: unknown): { code: string; message: string; fix?: string } { if (error instanceof SyncBlockedError) { const { severity: _severity, ...rest } = error.diagnostic; diff --git a/test/commands/sync-cli.test.ts b/test/commands/sync-cli.test.ts new file mode 100644 index 0000000000..3bcaacf056 --- /dev/null +++ b/test/commands/sync-cli.test.ts @@ -0,0 +1,63 @@ +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'; +import { cleanupTempPath } from '../helpers/temp-cleanup.js'; + +/** + * Flag handling that lives in the CLI layer rather than in the command class, + * so it can only be exercised through the real argument parser. + */ +describe('openspec sync / list --status (CLI surface)', () => { + let tempDir: string; + let env: NodeJS.ProcessEnv; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-sync-cli-')); + await fs.mkdir(path.join(tempDir, 'openspec', 'changes'), { recursive: true }); + await fs.mkdir(path.join(tempDir, 'openspec', 'specs'), { recursive: true }); + await fs.writeFile(path.join(tempDir, 'openspec', 'project.md'), '# Demo\n'); + // Keep root resolution off any store registry on the host machine. + env = { ...process.env, XDG_DATA_HOME: path.join(tempDir, 'xdg-data') }; + }); + + afterEach(async () => { + await cleanupTempPath(tempDir); + }); + + it('rejects --status combined with --specs', async () => { + const result = await runCLI(['list', '--specs', '--status', 'shipped'], { + cwd: tempDir, + env, + }); + + // Silently ignoring it would print the full spec list as though the filter + // had matched everything. + expect(result.exitCode).toBe(1); + expect(`${result.stdout}${result.stderr}`).toContain('cannot be combined with --specs'); + }); + + it('rejects an unknown --status value', async () => { + const result = await runCLI(['list', '--status', 'bogus'], { + cwd: tempDir, + env, + }); + + expect(result.exitCode).toBe(1); + expect(`${result.stdout}${result.stderr}`).toContain("Unknown --status 'bogus'"); + }); + + it('exits 0 from sync --check when nothing declares a status', async () => { + const result = await runCLI(['sync', '--check'], { cwd: tempDir, env }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain('nothing to check'); + }); + + it('registers sync in --help', async () => { + const result = await runCLI(['--help'], { cwd: tempDir, env }); + + expect(result.stdout).toContain('sync'); + }); +}); diff --git a/test/core/list.test.ts b/test/core/list.test.ts index 4144edec3f..a994a73f49 100644 --- a/test/core/list.test.ts +++ b/test/core/list.test.ts @@ -243,6 +243,31 @@ Regular text that should be ignored expect(text).not.toContain('shipped'); }); + it('excludes a change whose status cannot be determined from either filter', async () => { + await change('a', 'schema: spec-driven\nstatus: shiped\n'); + await change('b', 'schema: spec-driven\nstatus: shipped\n'); + + await new ListCommand().execute(tempDir, 'changes', { status: 'shipped' }); + const shippedOnly = logOutput.join('\n'); + logOutput = []; + await new ListCommand().execute(tempDir, 'changes', { status: 'proposed' }); + const proposedOnly = logOutput.join('\n'); + + // A filter is a claim of membership; an undetermined change belongs to + // neither list rather than to both. + expect(shippedOnly).toContain('b'); + expect(shippedOnly).not.toMatch(/^\s+a\s/m); + expect(proposedOnly).toBe("No changes with status 'proposed' found."); + }); + + it('still lists an undetermined change when no filter is given', async () => { + await change('a', 'schema: spec-driven\nstatus: shiped\n'); + + await new ListCommand().execute(tempDir, 'changes'); + + expect(logOutput.join('\n')).toContain('a'); + }); + it('says so when a filter matches nothing', async () => { await change('a'); diff --git a/test/core/sync.test.ts b/test/core/sync.test.ts index 6b0be98972..19c8bae006 100644 --- a/test/core/sync.test.ts +++ b/test/core/sync.test.ts @@ -353,6 +353,35 @@ describe('SyncCommand', () => { expect(await mainSpec()).toContain('Request tracing'); }); + it('does not stamp the change when a guard refuses it', async () => { + const dir = await makeChange('add-tracing', { + tasks: '## 1. Work\n- [ ] 1.1 Not done\n', + }); + + await expect( + sync.execute('add-tracing', { ship: true }) + ).rejects.toThrow(/incomplete task/i); + + // Stamping before the guards would leave the tree in the exact state + // --ship exists to prevent: shipped, with its deltas absent. + expect(readChangeStatus(dir).status).toBe('proposed'); + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + + it('does not stamp the change when its deltas fail validation', async () => { + const dir = await makeChange('add-tracing', { + delta: + '## ADDED Requirements\n\n### Requirement: Request tracing\n' + + 'The API SHALL attach a trace id.\n', + }); + + await expect( + sync.execute('add-tracing', { ship: true }) + ).rejects.toThrow(/Validation failed/); + + expect(readChangeStatus(dir).status).toBe('proposed'); + }); + it('is refused alongside --check', async () => { await makeChange('add-tracing'); @@ -412,6 +441,47 @@ describe('SyncCommand', () => { }); }); + describe('write failures leave no partly folded tree', () => { + it('restores every spec it had already written', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + // A second capability, so the run writes more than one file and a + // failure on the later one can strand the earlier one. + await fs.mkdir(path.join(changesDir(), 'add-tracing', 'specs', 'billing'), { + recursive: true, + }); + await fs.writeFile( + path.join(changesDir(), 'add-tracing', 'specs', 'billing', 'spec.md'), + '## ADDED Requirements\n\n### Requirement: Invoice totals\n' + + 'The system SHALL total invoices in the account currency.\n\n' + + '#### Scenario: Totalling\n- **WHEN** an invoice is issued\n' + + '- **THEN** its total is in the account currency\n' + ); + + const before = await mainSpec(); + const real = fs.writeFile; + let writes = 0; + const spy = vi + .spyOn(fs, 'writeFile') + .mockImplementation(async (...args: Parameters) => { + // Let the first spec through, fail the second, then let the + // rollback's own writes succeed. + if (++writes === 2) throw new Error('ENOSPC: no space left on device'); + return real(...args); + }); + + await expect(sync.execute(undefined, { yes: true })).rejects.toThrow( + /Could not write the main specs/ + ); + spy.mockRestore(); + + expect(await mainSpec()).toBe(before); + // The spec this run would have created must not be left behind either. + await expect( + fs.stat(path.join(specsDir(), 'billing', 'spec.md')) + ).rejects.toThrow(); + }); + }); + describe('errors', () => { it('names the available changes when the change does not exist', async () => { await makeChange('add-tracing'); From 51b031813aa1cddc6a1b1fac017916c0718e8ea6 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 7 Sep 2026 09:33:34 -0500 Subject: [PATCH 4/6] fix(sync): stamp the lifecycle field last, after the specs are correct MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round. `--ship` still wrote `status: shipped` before the spec writes, so a write that failed — or a fold that did not converge — reverted the specs and left the field set. Rather than teach every failure path to take the metadata back with it, and rely on none of them forgetting, the stamp now happens after the writes and after the convergence check. The failure is removed instead of compensated for. The reverse order is harmless and self-correcting: a fold that lands without the stamp is a proposed change whose deltas happen to already be in the specs, which the gate ignores, and rerunning `--ship` folds nothing and sets the field. Also stop forwarding the host environment into the new CLI test. `runCLI` merges `process.env` itself and isolates `XDG_CONFIG_HOME` unless the caller passes one explicitly, so spreading the host env made a developer's real config directory count as an explicit override. Co-Authored-By: Claude Opus 5 --- src/core/sync.ts | 30 ++++++++++++++++++------------ test/commands/sync-cli.test.ts | 6 ++++-- test/core/sync.test.ts | 24 ++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/core/sync.ts b/src/core/sync.ts index 60de3d72f1..c71e92c63d 100644 --- a/src/core/sync.ts +++ b/src/core/sync.ts @@ -579,18 +579,6 @@ export class SyncCommand { } } - // Every guard has passed, so this is the first point at which the change is - // known to be foldable. Stamping earlier would leave the working tree in the - // exact state this flag exists to prevent: a change claiming `shipped` while - // its deltas are absent from the specs, with the gate red until someone - // hand-edits the metadata back. - if (options.ship) { - const shipped = evaluations[0].report; - writeChangeStatus(path.join(changesDir, shipped.change), 'shipped'); - shipped.status = 'shipped'; - if (!json) console.log(`Marked '${shipped.change}' as shipped.`); - } - const pending = evaluations.flatMap(({ writes }) => writes); // Sync applies one change across several capabilities, so a failure part // way through the loop would leave some main specs folded and others not. @@ -653,6 +641,24 @@ export class SyncCommand { ); } + // Stamped last, once the specs on disk are known to be correct. Writing the + // field any earlier means every later failure - a write that cannot + // complete, a fold that does not settle - has to remember to take the + // metadata back with it, and the one that forgets leaves a change claiming + // `shipped` with its deltas absent, which is the state this flag exists to + // prevent. Ordering removes the failure rather than compensating for it. + // + // The reverse order is harmless and self-correcting: a fold that lands + // without the stamp is a proposed change whose deltas happen to already be + // in the specs, which the gate ignores, and rerunning `--ship` folds + // nothing and stamps the field. + if (options.ship) { + const shipped = evaluations[0].report; + writeChangeStatus(path.join(changesDir, shipped.change), 'shipped'); + shipped.status = 'shipped'; + if (!json) console.log(`Marked '${shipped.change}' as shipped.`); + } + const changes = evaluations.map((evaluation) => ({ ...evaluation.report, folded: true, diff --git a/test/commands/sync-cli.test.ts b/test/commands/sync-cli.test.ts index 3bcaacf056..b4d76d8f91 100644 --- a/test/commands/sync-cli.test.ts +++ b/test/commands/sync-cli.test.ts @@ -18,8 +18,10 @@ describe('openspec sync / list --status (CLI surface)', () => { await fs.mkdir(path.join(tempDir, 'openspec', 'changes'), { recursive: true }); await fs.mkdir(path.join(tempDir, 'openspec', 'specs'), { recursive: true }); await fs.writeFile(path.join(tempDir, 'openspec', 'project.md'), '# Demo\n'); - // Keep root resolution off any store registry on the host machine. - env = { ...process.env, XDG_DATA_HOME: path.join(tempDir, 'xdg-data') }; + // Only the override: runCLI merges process.env itself, and forwarding a + // host XDG_CONFIG_HOME would count as an explicit one, sending the CLI to + // the developer's real config directory instead of runCLI's isolated one. + env = { XDG_DATA_HOME: path.join(tempDir, 'xdg-data') }; }); afterEach(async () => { diff --git a/test/core/sync.test.ts b/test/core/sync.test.ts index 19c8bae006..eff3c78026 100644 --- a/test/core/sync.test.ts +++ b/test/core/sync.test.ts @@ -382,6 +382,30 @@ describe('SyncCommand', () => { expect(readChangeStatus(dir).status).toBe('proposed'); }); + it('does not stamp the change when the spec write fails', async () => { + const dir = await makeChange('add-tracing'); + const real = fs.writeFile; + const spy = vi + .spyOn(fs, 'writeFile') + .mockImplementation(async (...args: Parameters) => { + if (String(args[0]).endsWith(path.join('specs', 'api', 'spec.md'))) { + throw new Error('ENOSPC: no space left on device'); + } + return real(...args); + }); + + await expect(sync.execute('add-tracing', { ship: true })).rejects.toThrow( + /Could not write the main specs/ + ); + spy.mockRestore(); + + // The field is stamped only once the specs on disk are correct, so a + // failed write cannot leave a change claiming shipped with its deltas + // absent. + expect(readChangeStatus(dir).status).toBe('proposed'); + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + it('is refused alongside --check', async () => { await makeChange('add-tracing'); From eda3dd798c1d1b13e096db407c43f4137a9ffd85 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 7 Sep 2026 09:44:42 -0500 Subject: [PATCH 5/6] fix(sync): write the lifecycle field atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round, addressed differently than suggested. The finding was that `writeChangeStatus` can throw after the specs are folded and convergence confirmed, and asked for the fold to be rolled back. Rolling the specs back would be the wrong trade: it discards correct, validated work because a one-line metadata write failed, and the resulting state — folded specs on a change still marked `proposed` — is already the benign, self-correcting one the code comment names. The gate ignores proposed changes, and rerunning `--ship` folds nothing and sets the field. The half of the finding that does have teeth is a partially written `.openspec.yaml`. A direct write that fails partway truncates the file, and that file carries the change's `schema:` — losing it breaks every command that reads the change, not just the field being set. So the write now goes through a sibling temp file and an atomic rename: the file is either the old content or the new one, never half of either. This protects every caller rather than only the sync path. Failure is injected through a module mock rather than filesystem permissions, because chmod does not constrain root and does not exist on Windows. Co-Authored-By: Claude Opus 5 --- src/utils/change-metadata.ts | 16 ++++- test/utils/change-status-atomic-write.test.ts | 65 +++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 test/utils/change-status-atomic-write.test.ts diff --git a/src/utils/change-metadata.ts b/src/utils/change-metadata.ts index 8c2dd5c331..42f236fd15 100644 --- a/src/utils/change-metadata.ts +++ b/src/utils/change-metadata.ts @@ -505,9 +505,23 @@ export function writeChangeStatus(changeDir: string, status: ChangeStatus): void ); } + // Written through a sibling temp file and renamed into place. A direct + // write that fails partway (ENOSPC, a full disk, a killed process) truncates + // the file, and this one carries the change's `schema:` - losing it breaks + // every command that reads the change, not just the field being set. The + // rename is atomic on the same filesystem, so the file is either the old + // content or the new one. + const tempPath = `${metaPath}.openspec-status-${process.pid}-${Date.now()}`; try { - fs.writeFileSync(metaPath, doc.toString(), 'utf-8'); + fs.writeFileSync(tempPath, doc.toString(), 'utf-8'); + fs.renameSync(tempPath, metaPath); } catch (err) { + try { + fs.unlinkSync(tempPath); + } catch { + // Nothing to clean up, or it cannot be removed; the original file is + // intact either way, which is the property that matters here. + } const ioError = err instanceof Error ? err : new Error(String(err)); throw new ChangeMetadataError( `Failed to write metadata: ${ioError.message}`, diff --git a/test/utils/change-status-atomic-write.test.ts b/test/utils/change-status-atomic-write.test.ts new file mode 100644 index 0000000000..a7589c1cde --- /dev/null +++ b/test/utils/change-status-atomic-write.test.ts @@ -0,0 +1,65 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { promises as fsp } from 'fs'; +import path from 'path'; +import os from 'os'; + +/** + * `writeChangeStatus` writes through a sibling temp file and renames it into + * place. A direct write that fails partway truncates `.openspec.yaml`, and that + * file carries the change's `schema:` — losing it breaks every command that + * reads the change, not only the field being set. + * + * Injected through a module mock rather than filesystem permissions: chmod does + * not constrain root and does not exist on Windows, so a permissions-based test + * would not run on two of the three CI legs. + */ +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + default: actual, + writeFileSync: (...args: Parameters) => { + if (String(args[0]).includes('.openspec-status-')) { + throw new Error('ENOSPC: no space left on device'); + } + return actual.writeFileSync(...args); + }, + }; +}); + +const { writeChangeStatus } = await import('../../src/utils/change-metadata.js'); + +describe('writeChangeStatus durability', () => { + let tempDir: string; + const changeDir = (): string => path.join(tempDir, 'openspec', 'changes', 'c'); + const metaPath = (): string => path.join(changeDir(), '.openspec.yaml'); + + beforeEach(async () => { + tempDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'openspec-atomic-')); + await fsp.mkdir(changeDir(), { recursive: true }); + }); + + afterEach(async () => { + await fsp.rm(tempDir, { recursive: true, force: true }).catch(() => {}); + }); + + it('leaves the original file intact when the write fails', async () => { + const original = '# hand-authored\nschema: spec-driven\ncreated: 2026-09-07\n'; + await fsp.writeFile(metaPath(), original); + + expect(() => writeChangeStatus(changeDir(), 'shipped')).toThrow(/ENOSPC/); + + expect(await fsp.readFile(metaPath(), 'utf-8')).toBe(original); + }); + + it('leaves no temp file behind when the write fails', async () => { + await fsp.writeFile(metaPath(), 'schema: spec-driven\n'); + + expect(() => writeChangeStatus(changeDir(), 'shipped')).toThrow(); + + const strays = (await fsp.readdir(changeDir())).filter((name) => + name.includes('.openspec-status-') + ); + expect(strays).toEqual([]); + }); +}); From 679d3f7bbbd1fd180e2bb6497f5d166fa97ce573 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 7 Sep 2026 10:14:52 -0500 Subject: [PATCH 6/6] fix(sync): fold each change against the live tree, and gate the check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hardening round. Four defects, all reproduced before being fixed. **Two shipped changes touching one capability lost a fold.** `applyFolds` evaluated every change against the pre-write baseline and then wrote them all, so each rebuilt body was a whole file derived from the original spec and the second write erased the first — silently, while the console reported both as applied. The changes did not conflict; the batch read a stale baseline. Archive never had this because it takes one change per invocation. Sync now folds one change at a time, re-deriving each against the specs as they are at that moment. **Two capability ids resolving to one file overwrote each other.** A capability directory may deliberately be a symlink, so this is a shape the trust model allows rather than an accident. Archive refuses it outright; sync wrote both and lost one. Archive's check is now shared by both, so they cannot disagree about which trees they will write. **`sync --check` was green for a change whose delta the writer refuses.** The check only asked "is what was discovered folded?", and `discoverSpecFiles` does not walk `specs/spec.md`, so a change whose only delta sat there certified as clean while archive and the sync writer both refused the same tree (#1385). Delta validation now runs inside the evaluation, so it runs on the check path too — and it asks archive's own question about whether a change has deltas at all, so a zero-delta change gets the same answer from both commands. **`--ship` could fold and then fail forever.** A change with no `.openspec.yaml` had its specs written and its stamp refused, and the rerun failed in the same place, so the ordering's usual self-correction did not apply. Checked up front now. Also: `--no-validate` requires `--yes`, matching archive's refusal to skip validation without an explicit answer; the rollback no longer "restores" targets it never wrote (a false data-loss alarm) and refuses to clobber a file something else changed mid-run; and `test/core/sync.test.ts` restores the process working directory before removing its temp tree, which Windows locks. Nineteen tests added, each mutation-verified. Co-Authored-By: Claude Opus 5 --- .changeset/sync-lifecycle-status.md | 2 + docs-lab/reference/cli.md | 2 +- docs/cli.md | 7 +- docs/commands.md | 2 + docs/glossary.md | 4 +- .../add-standalone-spec-sync/proposal.md | 9 +- src/cli/index.ts | 2 +- src/core/archive.ts | 147 +++--- src/core/completions/command-registry.ts | 2 +- src/core/sync.ts | 422 ++++++++++++------ test/core/sync.test.ts | 410 ++++++++++++++++- 11 files changed, 812 insertions(+), 197 deletions(-) diff --git a/.changeset/sync-lifecycle-status.md b/.changeset/sync-lifecycle-status.md index 78f7733879..513f06bf15 100644 --- a/.changeset/sync-lifecycle-status.md +++ b/.changeset/sync-lifecycle-status.md @@ -9,3 +9,5 @@ Add `openspec sync`, which folds a change's delta specs into the main specs with `openspec list --status ` filters changes by that field. Everything here is opt-in and inert by default. The `status` field is absent unless a project writes it, nothing generates it, and `archive` is unchanged. + +Designed by [@ixxie](https://github.com/ixxie) in [#1683](https://github.com/Fission-AI/OpenSpec/issues/1683) — the diagnosis that `archive` welds a state transition to a text merge, `shipped ⇒ folded` as a predicate over the working tree, and the standalone `sync` that makes it checkable. This ships a smaller, additive subset of that proposal. diff --git a/docs-lab/reference/cli.md b/docs-lab/reference/cli.md index ccba3825ee..229efdc5a8 100644 --- a/docs-lab/reference/cli.md +++ b/docs-lab/reference/cli.md @@ -895,7 +895,7 @@ the change is still open, and CI can check that they are. | Flag | Effect | |---|---| | `--check` | Report shipped changes with unfolded deltas and exit 1. Writes nothing. | -| `--ship` | Set `status: shipped` on the named change, then fold it. | +| `--ship` | Fold the named change, then set `status: shipped` on it. If the fold fails, the field is not set. | | `-y, --yes` | Sync even when the change has incomplete tasks. | | `--no-validate` | Skip validation. | | `--json` | Print a structured result instead of text. | diff --git a/docs/cli.md b/docs/cli.md index d8ca06d5b3..7725dad59a 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -651,7 +651,7 @@ that they are. | Option | Description | |--------|-------------| | `--check` | Report shipped changes whose deltas are not in the main specs and exit 1. Writes nothing | -| `--ship` | Set `status: shipped` on the named change, then fold it — both land in one set of file changes for you to commit | +| `--ship` | Fold the named change, then set `status: shipped` on it — both land in one set of file changes for you to commit. If the fold fails, the field is not set | | `-y, --yes` | Sync even when the change still has incomplete tasks | | `--no-validate` | Skip validation (not recommended) | | `--json` | Structured output for hooks and CI | @@ -669,6 +669,11 @@ The field is optional and absent by default. A change with no `status` is project that never opts in is unaffected. Nothing writes the field on its own — not `openspec new change`, not `archive`. +If the fold fails — validation, incomplete tasks, a retirement, a write error — +the field is not set. `--ship` writes `status: shipped` only after the specs are +correct, so a failed run never leaves a change claiming to be shipped with its +deltas absent. + **The CI gate.** `openspec sync --check` asserts one property: *a change that claims to be shipped has its deltas in `specs/`*. A proposed change passes for free, so the check is green as its resting state and red only on a real mistake — diff --git a/docs/commands.md b/docs/commands.md index 7546dae82d..84ad92fe25 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -444,6 +444,8 @@ AI: Verifying add-dark-mode... **Optional command.** Merge delta specs from a change into main specs. Archive will prompt to sync if needed, so you typically don't need to run this manually. +> Not the same as the CLI's `openspec sync`. This one is the agent doing the merge in your session. `openspec sync` is a deterministic terminal command that does the same fold without a model, and carries the `--check` gate for CI — see [CLI](cli.md#openspec-sync). + **Syntax:** ``` /opsx:sync [change-name] diff --git a/docs/glossary.md b/docs/glossary.md index 345125f38a..790b550fb5 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -38,7 +38,9 @@ Terms are grouped by topic, then alphabetized within each group. **Archive.** The act of finishing a change. Its delta specs merge into the main specs, and the change folder moves to `openspec/changes/archive/YYYY-MM-DD-/`. After archiving, your specs describe the new reality. See [Concepts](concepts.md#archive). -**Sync.** Merging a change's delta specs into the main specs *without* archiving the change. Usually automatic (archive offers to do it), but available on its own as `/opsx:sync` for long-running changes. See [Commands](commands.md#opsxsync). +**Sync.** Merging a change's delta specs into the main specs *without* archiving the change. Usually automatic (archive offers to do it). Available on its own two ways: `/opsx:sync`, where the agent does the merge ([Commands](commands.md#opsxsync)), and `openspec sync`, the deterministic CLI command ([CLI](cli.md#openspec-sync)). + +**Shipped / proposed.** A change may declare its lifecycle state as `status: proposed | shipped` in its `.openspec.yaml`. The field is optional and absent by default; no `status` means `proposed`. `openspec sync --check` gates on it — a change that claims to be shipped must have its deltas in the main specs — which makes the specs enforceable in CI without a check that is red for the whole life of every PR. See [OpenSpec on a Team](team-workflow.md#enforcing-it-in-ci). ## Workflow and commands diff --git a/openspec/changes/add-standalone-spec-sync/proposal.md b/openspec/changes/add-standalone-spec-sync/proposal.md index 9058621715..4b1d22f723 100644 --- a/openspec/changes/add-standalone-spec-sync/proposal.md +++ b/openspec/changes/add-standalone-spec-sync/proposal.md @@ -65,5 +65,10 @@ from drifting apart (#1112). - Affected docs: `docs/cli.md`, `docs/team-workflow.md`, `docs-lab/reference/cli.md` -Credit: the diagnosis and the `shipped ⇒ folded` framing are from Matan Bendix -Shenhav's proposal in #1683. +Credit: the design is Matan Bendix Shenhav's, from #1683 and his implementation +#1684. His: the diagnosis, `shipped ⇒ folded` as a tree predicate (V), the +checker-versus-doer argument (IV), the standalone idempotent `sync` (III), status +as data (I and II), and shipping in one working-tree diff (VI). This change takes +a smaller, additive subset — no mode, no layout change, no migration — and +decides folded-ness by archive's zero-operations predicate rather than his +byte-identical regeneration. diff --git a/src/cli/index.ts b/src/cli/index.ts index 9867b00805..9c5175c57f 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -515,7 +515,7 @@ program .command('sync [change-name]') .description('Fold a change\'s spec deltas into the main specs without archiving it') .option('--check', 'Report shipped changes whose deltas are not in the main specs; write nothing') - .option('--ship', 'Mark the named change `status: shipped` before folding it') + .option('--ship', 'Fold the named change, then mark it `status: shipped`') .option('-y, --yes', 'Sync even when the change still has incomplete tasks') .option('--no-validate', 'Skip validation (not recommended)') .option('--json', 'Output as JSON (for hooks and CI)') diff --git a/src/core/archive.ts b/src/core/archive.ts index eb76bdfadc..e526d31630 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -821,35 +821,57 @@ async function fingerprintSpecInputs(update: SpecUpdate): Promise { return `${await fingerprintPath(update.source)}\n${await fingerprintPath(update.target)}`; } -async function mutationTargetIdentity(mutation: SpecMutation): Promise { +async function specTargetIdentity(target: string): Promise { try { - const stat = await fs.stat(mutation.update.target, { bigint: true }); + const stat = await fs.stat(target, { bigint: true }); return `${stat.dev}:${stat.ino}`; } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - const parent = path.dirname(mutation.update.target); + const parent = path.dirname(target); const realParent = await fs.realpath(parent).catch(() => path.resolve(parent)); - return `missing:${path.join(realParent, path.basename(mutation.update.target))}`; + return `missing:${path.join(realParent, path.basename(target))}`; } throw error; } } -async function assertDistinctMutationTargets(mutations: SpecMutation[]): Promise { +/** + * Refuse a run in which two capability ids resolve to the SAME file. + * + * `resolveTrustedSpecPath` deliberately permits a capability directory to be a + * symlink (monorepos point one at another), so two ids aliasing one spec is a + * shape the trust model allows rather than an exotic accident. Writing both in + * sequence is last-writer-wins: one capability's fold is silently destroyed and + * the other's requirements are filed under the wrong name. + * + * Shared with `openspec sync`, which writes the same targets - the two commands + * must not differ on which trees they are willing to write. + */ +export async function assertDistinctSpecTargets( + entries: Array<{ id: string; target: string }>, + action: string +): Promise { const owners = new Map(); - for (const mutation of mutations) { - const identity = await mutationTargetIdentity(mutation); + for (const entry of entries) { + const identity = await specTargetIdentity(entry.target); const existing = owners.get(identity); if (existing !== undefined) { throw new Error( - `Spec updates for '${existing}' and '${mutation.update.id}' resolve to the same target ` + - `${identity}. Replace the capability alias or combine the deltas before archiving.` + `Spec updates for '${existing}' and '${entry.id}' resolve to the same target ` + + `${identity}. Replace the capability alias or combine the deltas before ${action}.` ); } - owners.set(identity, mutation.update.id); + owners.set(identity, entry.id); } } +async function assertDistinctMutationTargets(mutations: SpecMutation[]): Promise { + await assertDistinctSpecTargets( + mutations.map(({ update }) => ({ id: update.id, target: update.target })), + 'archiving' + ); +} + async function captureSpecSnapshots(mutations: SpecMutation[]): Promise { return Promise.all( mutations.map(async ({ update, outcome, rebuilt }) => { @@ -1055,6 +1077,66 @@ async function finalizeRetirementBackups( } } +/** + * Whether a change carries spec deltas that must be validated before its specs + * are folded into `openspec/specs/`. + * + * A `spec.md` at the `specs/` root is never merged, so archiving a change that + * has one drops its content whether or not it carries delta headers (#1385). + * Its existence alone forces validation, which reports it and blocks the run. A + * directory named `spec.md` is a normal capability folder, so only a regular + * file counts. + * + * A change that declares `skip_specs` must not carry any file under `specs/` - + * validate reports that as a conflict, so this has to run the same check + * instead of skipping validation because the files happen to have no delta + * headers. A marker that cannot be honored (skip_specs mentioned but the + * metadata fails the shared shape, or names a schema that does not resolve) + * also forces validation, so every caller and validate always agree about the + * marker. Unreadable specs/ fails closed into validation too. + * + * An UNMARKED zero-delta change returns false - a gap that predates the marker, + * kept here so `openspec sync` inherits archive's exact answer rather than a + * stricter one of its own. + * + * Exported so `archive`, `sync`, and anything else that folds deltas ask one + * question rather than three that drift. + */ +export async function changeHasDeltaSpecsToValidate(changeDir: string): Promise { + const changeSpecsDir = path.join(changeDir, 'specs'); + const rootSpecStat = await fs.stat(path.join(changeSpecsDir, 'spec.md')).catch(() => null); + let hasDeltaSpecs = rootSpecStat?.isFile() === true; + + if (!hasDeltaSpecs) { + const marker = readSkipSpecsMarker(changeDir); + if (marker.invalidReason) { + hasDeltaSpecs = true; + } else if (marker.declared) { + let specsDirHasFiles = true; + try { + specsDirHasFiles = await hasAnyFileUnder(changeSpecsDir); + } catch { + // fall through with true: let validation surface the conflict + } + hasDeltaSpecs = specsDirHasFiles; + } + } + + for (const { specFile } of hasDeltaSpecs ? [] : await discoverSpecFiles(changeSpecsDir)) { + try { + const content = await fs.readFile(specFile, 'utf-8'); + // Case-insensitive to match the delta parser, so a lowercase header + // routes through the same delta validation that validate runs. + if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/im.test(content)) { + hasDeltaSpecs = true; + break; + } + } catch {} + } + + return hasDeltaSpecs; +} + export class ArchiveCommand { async execute(changeName?: string, options: ArchiveOptions = {}): Promise { const json = !!options.json; @@ -1222,50 +1304,7 @@ export class ArchiveCommand { } // Validate delta-formatted spec files under the change directory if present - const changeSpecsDir = path.join(changeDir, 'specs'); - // A spec.md at the specs/ root is never merged, so archiving a change - // that has one drops its content whether or not it carries delta headers - // (#1385). Its existence alone must run validation, which reports it and - // blocks the archive. A directory named spec.md is a normal capability - // folder, so only a regular file counts. - const rootSpecStat = await fs.stat(path.join(changeSpecsDir, 'spec.md')).catch(() => null); - let hasDeltaSpecs = rootSpecStat?.isFile() === true; - // A change that declares skip_specs must not carry any file under - // specs/ — validate reports that as a conflict, so archive has to run - // the same check instead of skipping validation because the files - // happen to have no delta headers. A marker that cannot be honored - // (skip_specs mentioned but the metadata fails the shared shape, or - // names a schema that does not resolve) also - // forces validation, so archive and validate always agree about the - // marker. Unreadable specs/ fails closed into validation too. (An - // UNMARKED zero-delta change still archives with only non-blocking - // proposal warnings — a gap that predates the marker and is left - // unchanged here.) - if (!hasDeltaSpecs) { - const marker = readSkipSpecsMarker(changeDir); - if (marker.invalidReason) { - hasDeltaSpecs = true; - } else if (marker.declared) { - let specsDirHasFiles = true; - try { - specsDirHasFiles = await hasAnyFileUnder(changeSpecsDir); - } catch { - // fall through with true: let validation surface the conflict - } - hasDeltaSpecs = specsDirHasFiles; - } - } - for (const { specFile } of hasDeltaSpecs ? [] : await discoverSpecFiles(changeSpecsDir)) { - try { - const content = await fs.readFile(specFile, 'utf-8'); - // Case-insensitive to match the delta parser, so a lowercase header - // routes through the same delta validation that validate runs. - if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/im.test(content)) { - hasDeltaSpecs = true; - break; - } - } catch {} - } + const hasDeltaSpecs = await changeHasDeltaSpecsToValidate(changeDir); if (hasDeltaSpecs) { // No mainSpecsDir here on purpose: the scenario-loss check standalone // validate runs (#1477) is the same one buildUpdatedSpec enforces a few diff --git a/src/core/completions/command-registry.ts b/src/core/completions/command-registry.ts index d9292cec01..5e23ec2234 100644 --- a/src/core/completions/command-registry.ts +++ b/src/core/completions/command-registry.ts @@ -210,7 +210,7 @@ export const COMMAND_REGISTRY: CommandDefinition[] = [ }, { name: 'ship', - description: 'Mark the named change `status: shipped` before folding it', + description: 'Fold the named change, then mark it `status: shipped`', }, { name: 'yes', diff --git a/src/core/sync.ts b/src/core/sync.ts index c71e92c63d..4e1f49fa07 100644 --- a/src/core/sync.ts +++ b/src/core/sync.ts @@ -40,9 +40,21 @@ * set is exactly the active changes that declare `status: shipped`, which is * bounded and drains itself as those changes archive. * - * Credit: the diagnosis, the `shipped => folded` framing, and the argument that - * a checker which reimplements the doer eventually disagrees with it are all - * from Matan Bendix Shenhav's proposal in #1683. + * Credit: this design is Matan Bendix Shenhav's, from his proposal #1683 and his + * implementation #1684, which he closed himself. No code from it is reused here. + * His, not ours: the diagnosis above; `shipped => folded` as a tree predicate + * evaluable at every tier (his decision V); the argument that a checker which + * reimplements the doer eventually disagrees with it (IV); the standalone + * idempotent `sync` (III); status as data rather than directory position (I and + * II); and setting the field and folding in one working-tree diff (VI, his + * `ship`). + * + * One deliberate divergence. His IV decides folded-ness by byte-identical + * regeneration; this module uses archive's zero-operations predicate instead, + * because the rebuild normalizes blank lines - a hand-formatted main spec would + * compare unequal while being perfectly in sync, and the gate would be red for a + * change nobody made. Same goal as IV, reached by sharing the doer's own + * predicate rather than comparing its output. */ import { promises as fs } from 'fs'; @@ -65,7 +77,12 @@ import { writeUpdatedSpec, type SpecUpdate, } from './specs-apply.js'; -import { isRetirableSpec, listActiveChangeNames } from './archive.js'; +import { + assertDistinctSpecTargets, + changeHasDeltaSpecsToValidate, + isRetirableSpec, + listActiveChangeNames, +} from './archive.js'; import { readChangeStatus, writeChangeStatus, @@ -82,7 +99,11 @@ import { folderStyleNameProblem } from './id.js'; export interface SyncOptions { /** Report what is unfolded and exit non-zero, without writing anything. */ check?: boolean; - /** Set `status: shipped` on the change before folding, in one working-tree diff. */ + /** + * Fold the change, then set `status: shipped` on it - one working-tree diff. + * The stamp is last on purpose: a failed write or a non-convergent fold must + * not leave the field claiming shipped with the deltas absent. + */ ship?: boolean; /** Proceed past incomplete tasks without asking. */ yes?: boolean; @@ -186,7 +207,8 @@ function sumCounts(counts: SyncSpecReport['counts']): number { async function evaluateChange( changeName: string, changeDir: string, - mainSpecsDir: string + mainSpecsDir: string, + validate = true ): Promise { const status = readChangeStatus(changeDir); const report: SyncChangeReport = { @@ -208,6 +230,52 @@ async function evaluateChange( return { report, writes: [] }; } + // Run BEFORE the fold, and on the `--check` path too. + // + // The gate promises `shipped => folded`, and a delta the merge would refuse is + // not folded and never will be. Leaving this to the write path made `--check` + // certify as clean a change whose only delta sat at `specs/spec.md`, which + // `discoverSpecFiles` does not walk (#1385): zero updates found, nothing + // pending, green - while `openspec sync` and `openspec archive` both refused + // the same tree. A gate that is green on a silently dropped requirement is + // worse than no gate. + // + // Whether a change HAS deltas to validate is archive's own question, asked + // through its own function, so a zero-delta change is treated identically by + // both commands. + if (validate) { + let hasDeltas: boolean; + try { + hasDeltas = await changeHasDeltaSpecsToValidate(changeDir); + } catch (error) { + report.folded = false; + report.blockers.push( + `Could not read this change's delta specs: ${ + error instanceof Error ? error.message : String(error) + }` + ); + return { report, writes: [] }; + } + if (hasDeltas) { + // No mainSpecsDir, matching archive: the scenario-loss check standalone + // validate runs (#1477) is the same one buildUpdatedSpec enforces below, + // and reporting it here would relabel that failure. + const deltaReport = await new Validator().validateChangeDeltaSpecs(changeDir); + if (!deltaReport.valid) { + report.folded = false; + for (const issue of deltaReport.issues) { + if (issue.level === 'ERROR') report.blockers.push(issue.message); + } + // A report that is invalid with no ERROR issue would otherwise pass + // silently while claiming to have blocked. + if (report.blockers.length === 0) { + report.blockers.push(`Delta specs for '${changeName}' failed validation.`); + } + return { report, writes: [] }; + } + } + } + let updates: SpecUpdate[]; try { updates = await findSpecUpdates(changeDir, mainSpecsDir); @@ -369,10 +437,41 @@ export class SyncCommand { ); } + // Archive refuses to skip validation without an explicit answer, because + // skipping it can write a spec that would never have validated. Sync is + // unattended by design, so there is no prompt to give - `--yes` is the + // answer, exactly as archive's own JSON path requires. + if (options.validate === false && !options.yes && !options.check) { + throw new SyncBlockedError( + 'sync_confirmation_required', + 'Skipping validation can fold a spec that would never have validated, so it needs confirmation.', + withStoreFlag(root, `openspec sync ${changeName ?? ''} --no-validate --yes`) + ); + } + const targets = changeName ? [await this.resolveNamedChange(changeName, changesDir, root)] : await this.shippedChanges(changesDir); + // Checked before the fold, not after it. `writeChangeStatus` refuses a + // change with no `.openspec.yaml`, and discovering that only once the specs + // are written leaves a fold that is never stamped - and a rerun that fails + // in exactly the same place, so the ordering's usual self-correction does + // not apply. + if (options.ship) { + const metaPath = path.join(changesDir, targets[0], METADATA_FILENAME); + try { + await fs.access(metaPath); + } catch { + throw new SyncBlockedError( + 'sync_ship_no_metadata', + `Change '${targets[0]}' has no ${METADATA_FILENAME}, so there is no file to record ` + + `\`status: shipped\` in. No specs were folded.`, + `Create the change with openspec new change, or add ${METADATA_FILENAME} by hand, then rerun.` + ); + } + } + if (targets.length === 0) { const result: SyncResult = { checked: check, @@ -395,13 +494,18 @@ export class SyncCommand { const evaluations: Evaluation[] = []; for (const name of targets) { evaluations.push( - await evaluateChange(name, path.join(changesDir, name), mainSpecsDir) + await evaluateChange( + name, + path.join(changesDir, name), + mainSpecsDir, + options.validate !== false + ) ); } return check ? this.reportCheck(evaluations, root, json) - : this.applyFolds(evaluations, changesDir, root, options, json); + : this.applyFolds(evaluations, changesDir, mainSpecsDir, root, options, json); } /** A named change has to exist, exactly as archive requires. */ @@ -517,6 +621,7 @@ export class SyncCommand { private async applyFolds( evaluations: Evaluation[], changesDir: string, + mainSpecsDir: string, root: ResolvedOpenSpecRoot, options: SyncOptions, json: boolean @@ -537,74 +642,126 @@ export class SyncCommand { ); } - // Same guards archive runs before it writes a spec, in the same order. + // Delta validation already ran inside `evaluateChange`, on the check path + // too, so a blocked change never reaches here. Task completion is the one + // guard that is about the change rather than about its deltas, and it has + // no bearing on whether the tree satisfies the gate - so it gates the write + // and deliberately does not make `--check` red. for (const { report } of evaluations) { - const changeDir = path.join(changesDir, report.change); - if (!skipValidation) { - await this.assertDeltaSpecsValid(report.change, changeDir, root, json); - } await this.assertTasksComplete(report.change, changesDir, options, root, json); } - if (!json) { - for (const { report } of evaluations) { - for (const warning of report.warnings) { - console.log(chalk.yellow(`⚠️ Warning: ${warning}`)); - } - } - } + // Fold ONE CHANGE AT A TIME, rebuilding each against the specs as they are + // on disk at that moment. + // + // Evaluating every change up front and then writing them all would rebuild + // each one from the same pre-write baseline, so two shipped changes adding + // different requirements to the same capability would each produce a spec + // containing only their own - and the second write would erase the first, + // silently, while the console reported both as applied. That is not a + // conflict between the changes; they compose fine. It is the batch reading + // a stale baseline. `archive` never had the bug because it takes one change + // per invocation, and folding sequentially is how sync inherits that. + // + // Every target written across the whole run is captured first, so a failure + // on the third change still puts the first two back rather than handing + // back a tree nobody asked for. + const totals = { added: 0, modified: 0, removed: 0, renamed: 0 }; + const snapshots: TargetSnapshot[] = []; + // What this run last wrote to each target, so the rollback can tell its own + // output apart from a concurrent edit it must not clobber. + const wrote = new Map(); + let wroteAny = false; - // Every rebuilt spec is validated before any of them is written, so a late - // failure leaves every target unchanged rather than half the tree folded. - if (!skipValidation) { - const validator = new Validator(); - for (const { report, writes } of evaluations) { - for (const write of writes) { - const specReport = await validator.validateSpecContent( - write.update.id, - write.rebuilt - ); - if (specReport.valid) continue; - const details = specReport.issues - .filter((issue) => issue.level === 'ERROR') - .map((issue) => issue.message) - .join('; '); + try { + for (const evaluation of evaluations) { + const changeName = evaluation.report.change; + // Re-evaluated against the current tree rather than reusing the plan + // built before the previous change was folded. + const current = await evaluateChange( + changeName, + path.join(changesDir, changeName), + mainSpecsDir, + !skipValidation + ); + if (current.report.blockers.length > 0) { throw new SyncBlockedError( - 'sync_spec_validation_failed', - `The spec '${write.update.id}' would be rebuilt into an invalid state by ` + - `change '${report.change}': ${details}. No files were changed.`, - `Run ${withStoreFlag(root, `openspec validate ${write.update.id}`)} after fixing the change deltas.` + 'sync_change_blocked', + `Cannot sync '${changeName}': ${current.report.blockers[0]}` ); } - } - } + evaluation.report.specs = current.report.specs; + evaluation.report.warnings = current.report.warnings; + if (current.writes.length === 0) continue; + + // Two capability ids can resolve to the SAME file - a symlinked + // capability directory is explicitly allowed by the trust model, and a + // case-variant id aliases on a case-insensitive filesystem. Writing + // both in sequence is last-writer-wins, which loses one fold and files + // the other's requirements under the wrong name. Archive refuses this + // outright; sync uses archive's own check so the two agree on which + // trees they will write. + await assertDistinctSpecTargets( + current.writes.map(({ update }) => ({ id: update.id, target: update.target })), + 'syncing' + ); - const pending = evaluations.flatMap(({ writes }) => writes); - // Sync applies one change across several capabilities, so a failure part - // way through the loop would leave some main specs folded and others not. - // Re-running would finish the job - the fold is idempotent - but a tree - // nobody asked for is not a state to hand back, so the previous bytes are - // restored instead. Simpler than archive's equivalent because sync only - // ever writes: there is no retirement to undo and no directory move to - // unwind. - const snapshots = await captureTargets(pending.map((write) => write.update.target)); + // Validated before any of THIS change's specs is written, so a late + // failure inside one change leaves that change wholly unapplied. + if (!skipValidation) { + const validator = new Validator(); + for (const write of current.writes) { + const specReport = await validator.validateSpecContent( + write.update.id, + write.rebuilt + ); + if (specReport.valid) continue; + const details = specReport.issues + .filter((issue) => issue.level === 'ERROR') + .map((issue) => issue.message) + .join('; '); + throw new SyncBlockedError( + 'sync_spec_validation_failed', + `The spec '${write.update.id}' would be rebuilt into an invalid state by ` + + `change '${changeName}': ${details}.`, + `Run ${withStoreFlag(root, `openspec validate ${write.update.id}`)} after fixing the change deltas.` + ); + } + } - const totals = { added: 0, modified: 0, removed: 0, renamed: 0 }; - let wroteAny = false; - try { - for (const write of pending) { - await writeUpdatedSpec(write.update, write.rebuilt, write.counts, { - silent: json, - ...(isStoreSelectedRoot(root) ? { displayPath: write.update.target } : {}), - }); - wroteAny = true; - totals.added += write.counts.added; - totals.modified += write.counts.modified; - totals.removed += write.counts.removed; - totals.renamed += write.counts.renamed; + if (!json) { + for (const warning of current.report.warnings) { + console.log(chalk.yellow(`⚠️ Warning: ${warning}`)); + } + } + + for (const write of current.writes) { + if (!wrote.has(write.update.target)) { + snapshots.push(await captureTarget(write.update.target)); + } + await writeUpdatedSpec(write.update, write.rebuilt, write.counts, { + silent: json, + ...(isStoreSelectedRoot(root) ? { displayPath: write.update.target } : {}), + }); + wrote.set(write.update.target, write.rebuilt); + wroteAny = true; + totals.added += write.counts.added; + totals.modified += write.counts.modified; + totals.removed += write.counts.removed; + totals.renamed += write.counts.renamed; + } } } catch (error) { - const restoreFailure = await restoreTargets(snapshots); + const restoreFailure = await restoreTargets(snapshots, wrote); + if (error instanceof SyncBlockedError) { + throw new SyncBlockedError( + error.diagnostic.code, + `${error.message}${ + restoreFailure ? ` ${restoreFailure}` : ' No spec was left partly folded.' + }`, + restoreFailure ? 'Restore the named files from git, then rerun.' : error.diagnostic.fix + ); + } throw new SyncBlockedError( 'sync_write_failed', `Could not write the main specs: ${ @@ -614,29 +771,31 @@ export class SyncCommand { ); } - // Re-evaluate rather than assume. Folding is idempotent for a single - // change, but two shipped changes can disagree about the same requirement - - // one adding what the other removes - and a fold that does not settle would - // otherwise report success while `--check` immediately after went red. The - // merge builder catches the destructive shapes of that disagreement on its - // own (a MODIFIED that would drop a scenario, an ADDED whose content - // differs), so what reaches here is the non-convergent rest, and naming it - // is better than looping on it. + // Re-evaluate rather than assume. Sequential folding removes the stale + // baseline, but two shipped changes can still genuinely disagree - one + // adding a requirement the other removes - and such a pair never settles. + // The merge builder catches the destructive shapes on its own (a MODIFIED + // that would drop a scenario, an ADDED whose content differs), so what + // reaches here is the non-convergent rest, and naming it beats looping. const unsettled: string[] = []; for (const { report } of evaluations) { const after = await evaluateChange( report.change, path.join(changesDir, report.change), - root.specsDir + mainSpecsDir, + !skipValidation ); if (!after.report.folded) unsettled.push(report.change); } if (unsettled.length > 0) { + const restoreFailure = await restoreTargets(snapshots, wrote); throw new SyncBlockedError( 'sync_did_not_converge', - `Specs were written, but these changes still report unfolded deltas: ` + + `These changes still report unfolded deltas after a fold: ` + `${unsettled.join(', ')}. Two shipped changes are claiming the same ` + - `requirement in ways that cannot both hold.`, + `requirement in ways that cannot both hold.${ + restoreFailure ? ` ${restoreFailure}` : ' The main specs were left unchanged.' + }`, 'Reconcile the conflicting deltas, then rerun.' ); } @@ -682,35 +841,6 @@ export class SyncCommand { }; } - /** - * Archive's delta validation, restricted to what sync needs. Sync only ever - * runs on a change that has deltas to fold, so the `skip_specs` reconciliation - * archive performs (a change declaring it has no deltas, but carrying files) - * has nothing to decide here - `findSpecUpdates` already found the files. - */ - private async assertDeltaSpecsValid( - changeName: string, - changeDir: string, - root: ResolvedOpenSpecRoot, - json: boolean - ): Promise { - const report = await new Validator().validateChangeDeltaSpecs(changeDir); - if (report.valid) return; - - if (!json) { - console.log(chalk.red(`\nValidation errors in change delta specs:`)); - for (const issue of report.issues) { - if (issue.level === 'ERROR') console.log(chalk.red(` ✗ ${issue.message}`)); - else if (issue.level === 'WARNING') console.log(chalk.yellow(` ⚠ ${issue.message}`)); - } - } - throw new SyncBlockedError( - 'sync_validation_failed', - `Validation failed for change '${changeName}'. No files were changed.`, - `Run ${withStoreFlag(root, `openspec validate ${changeName}`)} for details, fix the errors, or rerun with --no-validate.` - ); - } - /** * Folding a change whose tasks are unfinished writes requirements into * `specs/` that nothing implements yet - the exact drift the gate exists to @@ -757,51 +887,81 @@ interface TargetSnapshot { content?: Buffer; } -/** Read the current bytes of each target so a failed write can be undone. */ -async function captureTargets(targets: string[]): Promise { - return Promise.all( - targets.map(async (target) => { - try { - return { target, content: await fs.readFile(target) }; - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { target }; - throw error; - } - }) - ); +/** Read the current bytes of a target so a failed write can be undone. */ +async function captureTarget(target: string): Promise { + try { + return { target, content: await fs.readFile(target) }; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { target }; + throw error; + } } /** - * Put every captured target back the way it was, in reverse order. + * Put every target that this run actually changed back the way it was, in + * reverse order. + * + * Two things it will not do, both mirroring `archive`'s rollback: * - * Returns a sentence naming what could not be restored, or undefined when the + * - **It does not touch a target whose bytes already match the snapshot.** The + * write that failed is usually the one that never landed, and "restoring" an + * unchanged file only to fail on a read-only one produced a false "may hold + * partly folded content" alarm about a file nothing had written. + * - **It does not overwrite content this run did not produce.** A target whose + * bytes match neither the snapshot nor what was written was changed by + * something else while the fold was running; clobbering it would destroy an + * edit to save a rollback. It is reported instead. + * + * Returns a sentence naming what could not be put back, or undefined when the * tree is back to its original state. Never throws: it runs inside a failure * path, and losing the original error to a rollback error would hide the cause. */ async function restoreTargets( - snapshots: TargetSnapshot[] + snapshots: TargetSnapshot[], + wrote: Map ): Promise { const failed: string[] = []; + const foreign: string[] = []; for (const snapshot of [...snapshots].reverse()) { try { + const current = await fs.readFile(snapshot.target).catch((error) => { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; + throw error; + }); + if (snapshot.content === undefined) { - // The file did not exist before this run, so the rollback is removing - // whatever was created. A missing file is already the desired state. - await fs.unlink(snapshot.target).catch((error) => { - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; - }); - } else { - // Written in place, exactly as writeUpdatedSpec does, so a symlinked or - // hard-linked spec keeps the semantics it had before the fold. - await fs.writeFile(snapshot.target, snapshot.content); + // The file did not exist before this run. + if (current === undefined) continue; + if (current.toString() !== wrote.get(snapshot.target)) { + foreign.push(snapshot.target); + continue; + } + await fs.unlink(snapshot.target); + continue; } + + if (current !== undefined && current.equals(snapshot.content)) continue; + if (current !== undefined && current.toString() !== wrote.get(snapshot.target)) { + foreign.push(snapshot.target); + continue; + } + // Written in place, exactly as writeUpdatedSpec does, so a symlinked or + // hard-linked spec keeps the semantics it had before the fold. + await fs.writeFile(snapshot.target, snapshot.content); } catch { failed.push(snapshot.target); } } - return failed.length > 0 - ? `These specs could not be restored and may hold partly folded content: ${failed.join(', ')}.` - : undefined; + + const problems = [ + failed.length > 0 + ? `These specs could not be restored and may hold partly folded content: ${failed.join(', ')}.` + : '', + foreign.length > 0 + ? `These specs changed underneath this run and were left as they are: ${foreign.join(', ')}.` + : '', + ].filter(Boolean); + return problems.length > 0 ? problems.join(' ') : undefined; } function toDiagnostic(error: unknown): { code: string; message: string; fix?: string } { diff --git a/test/core/sync.test.ts b/test/core/sync.test.ts index eff3c78026..22ca9df72b 100644 --- a/test/core/sync.test.ts +++ b/test/core/sync.test.ts @@ -5,6 +5,10 @@ import os from 'os'; import { SyncCommand } from '../../src/core/sync.js'; import { ArchiveCommand } from '../../src/core/archive.js'; import { readChangeStatus, writeChangeStatus } from '../../src/utils/change-metadata.js'; +import { + writeStoreMetadataState, + writeStoreRegistryState, +} from '../../src/core/store/foundation.js'; vi.mock('@inquirer/prompts', () => ({ select: vi.fn(), @@ -42,6 +46,7 @@ describe('SyncCommand', () => { const originalConsoleLog = console.log; const originalExitCode = process.exitCode; const originalXdgDataHome = process.env.XDG_DATA_HOME; + const originalCwd = process.cwd(); let logged: string[]; const changesDir = (): string => path.join(tempDir, 'openspec', 'changes'); @@ -56,10 +61,15 @@ describe('SyncCommand', () => { delta?: string; tasks?: string; metadata?: string; + /** Capability id relative to `specs/`, e.g. `platform/session-layout`. */ + capability?: string; } = {} ): Promise { const dir = path.join(changesDir(), name); - await fs.mkdir(path.join(dir, 'specs', 'api'), { recursive: true }); + const capability = options.capability ?? 'api'; + await fs.mkdir(path.join(dir, 'specs', ...capability.split('/')), { + recursive: true, + }); await fs.writeFile( path.join(dir, '.openspec.yaml'), options.metadata ?? @@ -75,7 +85,7 @@ describe('SyncCommand', () => { options.tasks ?? '## 1. Work\n- [x] 1.1 Done\n' ); await fs.writeFile( - path.join(dir, 'specs', 'api', 'spec.md'), + path.join(dir, 'specs', ...capability.split('/'), 'spec.md'), options.delta ?? ADDED_DELTA ); return dir; @@ -86,7 +96,12 @@ describe('SyncCommand', () => { } beforeEach(async () => { - tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-sync-test-')); + // realpath'd: a Windows runner can hand back an 8.3 short path while the + // CLI canonicalizes to the long form, and macOS /var resolves to + // /private/var - both make a root read as outside itself. + tempDir = await fs.realpath( + await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-sync-test-')) + ); process.chdir(tempDir); // Keep root resolution off any real store registry on the host. process.env.XDG_DATA_HOME = path.join(tempDir, 'xdg-data'); @@ -105,6 +120,10 @@ describe('SyncCommand', () => { }); afterEach(async () => { + // Before the rm: Windows locks the process working directory, so removing + // a tree we are standing inside fails and leaks it, leaving the next + // describe running from a deleted path. + process.chdir(originalCwd); console.log = originalConsoleLog; process.exitCode = originalExitCode; if (originalXdgDataHome === undefined) delete process.env.XDG_DATA_HOME; @@ -216,6 +235,110 @@ describe('SyncCommand', () => { ).resolves.toBeTruthy(); }); + it('folds a nested capability into the same nested path', async () => { + const nested = path.join(specsDir(), 'platform', 'session-layout'); + await fs.mkdir(nested, { recursive: true }); + await fs.writeFile( + path.join(nested, 'spec.md'), + '# session-layout Specification\n\n## Purpose\n' + + 'How sessions are laid out across the platform surface.\n\n' + + '## Requirements\n\n### Requirement: Session store\n' + + 'The platform SHALL persist sessions.\n\n' + + '#### Scenario: Persisted\n- **WHEN** a session is created\n- **THEN** it is persisted\n' + ); + await makeChange('evict-sessions', { + status: 'shipped', + capability: 'platform/session-layout', + delta: + '## ADDED Requirements\n\n### Requirement: Session eviction\n' + + 'The platform SHALL evict idle sessions.\n\n#### Scenario: Idle session\n' + + '- **WHEN** a session idles out\n- **THEN** it is evicted\n', + }); + + await sync.execute(undefined, { yes: true }); + + expect(await fs.readFile(path.join(nested, 'spec.md'), 'utf-8')).toContain( + 'Session eviction' + ); + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + + it('folds a MODIFIED delta and reports folded afterwards', async () => { + await makeChange('retry-after', { + status: 'shipped', + delta: + '## MODIFIED Requirements\n\n### Requirement: Rate limiting\n' + + 'The API SHALL reject requests above the configured rate, with a Retry-After header.\n\n' + + '#### Scenario: Over the limit\n- **WHEN** a client exceeds the rate\n' + + '- **THEN** the API responds 429 with Retry-After\n', + }); + + await sync.execute(undefined, { yes: true }); + logged = []; + process.exitCode = undefined; + await sync.execute(undefined, { check: true }); + + expect(await mainSpec()).toContain('Retry-After'); + expect(process.exitCode).toBeUndefined(); + }); + + it('folds a RENAMED delta and counts it as a rename', async () => { + await makeChange('rename-limits', { + status: 'shipped', + delta: + '## RENAMED Requirements\n\n- FROM: `### Requirement: Rate limiting`\n' + + '- TO: `### Requirement: Request throttling`\n', + }); + + await sync.execute(undefined, { check: true, json: true }); + // The only path that increments `renamed`. + expect(JSON.parse(output()).sync.changes[0].specs[0].counts).toEqual({ + added: 0, + modified: 0, + removed: 0, + renamed: 1, + }); + + logged = []; + process.exitCode = undefined; + await sync.execute(undefined, { yes: true }); + + const folded = await mainSpec(); + expect(folded).toContain('### Requirement: Request throttling'); + expect(folded).not.toContain('### Requirement: Rate limiting'); + }); + + it('folds every shipped change and leaves proposed ones alone', async () => { + await makeChange('a-tracing', { status: 'shipped' }); + await makeChange('b-billing', { + status: 'shipped', + capability: 'billing', + delta: + '## ADDED Requirements\n\n### Requirement: Invoice totals\n' + + 'The system SHALL total invoices in the account currency.\n\n' + + '#### Scenario: Totalling\n- **WHEN** an invoice is issued\n' + + '- **THEN** its total is in the account currency\n', + }); + await makeChange('c-proposed'); + + await sync.execute(undefined, { yes: true }); + + expect(await mainSpec()).toContain('Request tracing'); + expect( + await fs.readFile(path.join(specsDir(), 'billing', 'spec.md'), 'utf-8') + ).toContain('Invoice totals'); + expect(output()).toContain('Totals: + 2'); + }); + + it('creates the specs tree when the project has none yet', async () => { + await fs.rm(specsDir(), { recursive: true, force: true }); + await makeChange('add-tracing', { status: 'shipped' }); + + await sync.execute(undefined, { yes: true }); + + expect(await mainSpec()).toContain('### Requirement: Request tracing'); + }); + it('stops checking a change once it is archived', async () => { await makeChange('add-tracing', { status: 'shipped' }); await sync.execute(undefined, { yes: true }); @@ -232,6 +355,133 @@ describe('SyncCommand', () => { }); }); + describe('folding several changes in one run', () => { + /** A second change adding a different requirement to the SAME capability. */ + async function secondChange(name: string): Promise { + const dir = path.join(changesDir(), name); + await fs.mkdir(path.join(dir, 'specs', 'api'), { recursive: true }); + await fs.writeFile( + path.join(dir, '.openspec.yaml'), + 'schema: spec-driven\nstatus: shipped\n' + ); + await fs.writeFile( + path.join(dir, 'proposal.md'), + '## Why\nThe API needs audit logging, and today nothing records calls.\n\n' + + '## What Changes\n- Add audit logging to the API surface.\n' + ); + await fs.writeFile(path.join(dir, 'tasks.md'), '## 1. Work\n- [x] 1.1 Done\n'); + await fs.writeFile( + path.join(dir, 'specs', 'api', 'spec.md'), + '## ADDED Requirements\n\n### Requirement: Audit logging\n' + + 'The API SHALL record every call in the audit log.\n\n' + + '#### Scenario: Logged call\n- **WHEN** a request is served\n' + + '- **THEN** the audit log gains an entry\n' + ); + } + + it('keeps both folds when two shipped changes touch one capability', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + await secondChange('add-audit'); + + await sync.execute(undefined, { yes: true }); + + // Evaluating both against the same pre-write baseline and then writing + // them in sequence makes the second write erase the first: each rebuilt + // body is a whole file derived from the original spec. The changes do not + // conflict, so losing one is pure data loss. + const spec = await mainSpec(); + expect(spec).toContain('Request tracing'); + expect(spec).toContain('Audit logging'); + expect(spec).toContain('Rate limiting'); + }); + + it('is green afterwards for every change it folded', async () => { + await makeChange('add-tracing', { status: 'shipped' }); + await secondChange('add-audit'); + await sync.execute(undefined, { yes: true }); + + logged = []; + process.exitCode = undefined; + await sync.execute(undefined, { check: true }); + + expect(process.exitCode).toBeUndefined(); + }); + + // fs.symlink needs Developer Mode or elevation on a Windows runner. + it.skipIf(process.platform === 'win32')( + 'refuses when two capability ids resolve to the same file', + async () => { + // A capability directory may deliberately be a symlink, so two ids + // aliasing one spec is a shape the trust model allows. Writing both in + // sequence is last-writer-wins: one fold is destroyed and the other's + // requirements are filed under the wrong capability. + await makeChange('add-tracing', { status: 'shipped' }); + await fs.mkdir(path.join(changesDir(), 'add-tracing', 'specs', 'apiv2'), { + recursive: true, + }); + await fs.writeFile( + path.join(changesDir(), 'add-tracing', 'specs', 'apiv2', 'spec.md'), + '## ADDED Requirements\n\n### Requirement: Audit logging\n' + + 'The API SHALL record every call in the audit log.\n\n' + + '#### Scenario: Logged call\n- **WHEN** a request is served\n' + + '- **THEN** the audit log gains an entry\n' + ); + await fs.symlink('api', path.join(specsDir(), 'apiv2'), 'dir'); + + await expect(sync.execute('add-tracing', { yes: true })).rejects.toThrow( + /resolve to the same target/ + ); + expect(await mainSpec()).toBe(MAIN_SPEC); + } + ); + }); + + describe('the check path sees what the writer would refuse', () => { + it('fails a shipped change whose delta specs do not validate', async () => { + await makeChange('add-tracing', { + status: 'shipped', + delta: + '## ADDED Requirements\n\n### Requirement: Request tracing\n' + + 'The API SHALL attach a trace id.\n', + }); + + await sync.execute(undefined, { check: true }); + + // The gate promises `shipped => folded`. A delta the merge would refuse + // is not folded and never will be, so certifying it clean is a false + // green on the one surface teams wire into CI. + expect(process.exitCode).toBe(1); + expect(output()).toContain('at least one scenario'); + }); + + it('fails a shipped change whose only delta sits at the specs root', async () => { + // `discoverSpecFiles` does not walk `specs/spec.md`, so the change looks + // like it has nothing to fold while its requirement is silently dropped + // (#1385). Archive and the sync writer both refuse this tree. + const dir = await makeChange('add-tracing', { status: 'shipped' }); + await fs.rm(path.join(dir, 'specs', 'api'), { recursive: true }); + await fs.writeFile(path.join(dir, 'specs', 'spec.md'), ADDED_DELTA); + + await sync.execute(undefined, { check: true }); + + expect(process.exitCode).toBe(1); + expect(output()).toContain('specs/spec.md'); + }); + + it('passes a shipped change that declares it has no deltas', async () => { + // Archive treats a zero-delta change as fine; sync must give the same + // answer rather than a stricter one of its own. + const dir = await makeChange('add-tracing', { + metadata: 'schema: spec-driven\nstatus: shipped\nskip_specs: true\n', + }); + await fs.rm(path.join(dir, 'specs'), { recursive: true }); + + await sync.execute(undefined, { check: true }); + + expect(process.exitCode).toBeUndefined(); + }); + }); + describe('guards', () => { it('refuses a change with incomplete tasks', async () => { await makeChange('add-tracing', { @@ -266,7 +516,7 @@ describe('SyncCommand', () => { }); await expect(sync.execute('add-tracing', { yes: true })).rejects.toThrow( - /Validation failed/ + /must include at least one scenario/ ); expect(await mainSpec()).toBe(MAIN_SPEC); }); @@ -305,6 +555,54 @@ describe('SyncCommand', () => { }); }); + describe('--no-validate', () => { + it('needs --yes, the way archive needs an answer', async () => { + await makeChange('add-tracing', { + status: 'shipped', + delta: + '## ADDED Requirements\n\n### Requirement: Request tracing\n' + + 'The API SHALL attach a trace id.\n', + }); + + await expect( + sync.execute('add-tracing', { validate: false }) + ).rejects.toThrow(/needs confirmation/); + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + + it('folds a delta validation would refuse, once confirmed', async () => { + await makeChange('add-tracing', { + status: 'shipped', + // The same scenario-less ADDED the validation guard rejects. + delta: + '## ADDED Requirements\n\n### Requirement: Request tracing\n' + + 'The API SHALL attach a trace id.\n', + }); + + await sync.execute('add-tracing', { validate: false, yes: true }); + + expect(await mainSpec()).toContain('### Requirement: Request tracing'); + }); + }); + + describe('a fold that does not settle', () => { + it('refuses to report success when two shipped changes cannot both hold', async () => { + const conflicting = (discriminator: string): string => + '## MODIFIED Requirements\n\n### Requirement: Rate limiting\n' + + `The API SHALL reject requests above the configured rate, per ${discriminator}.\n\n` + + '#### Scenario: Over the limit\n- **WHEN** a client exceeds the rate\n' + + `- **THEN** the API responds 429 with a per-${discriminator} message\n`; + await makeChange('a-widen', { status: 'shipped', delta: conflicting('API key') }); + await makeChange('b-narrow', { status: 'shipped', delta: conflicting('IP address') }); + + // Reporting success would have `--check`, run immediately after, go red + // for a fold that just claimed to have succeeded. + await expect(sync.execute(undefined, { yes: true })).rejects.toThrow( + /still report unfolded deltas/ + ); + }); + }); + describe('undetermined status fails closed', () => { it('reports a change whose status value is not a known state', async () => { await makeChange('add-tracing', { @@ -377,7 +675,7 @@ describe('SyncCommand', () => { await expect( sync.execute('add-tracing', { ship: true }) - ).rejects.toThrow(/Validation failed/); + ).rejects.toThrow(/must include at least one scenario/); expect(readChangeStatus(dir).status).toBe('proposed'); }); @@ -406,6 +704,19 @@ describe('SyncCommand', () => { expect(await mainSpec()).toBe(MAIN_SPEC); }); + it('refuses before folding when there is no metadata file to stamp', async () => { + const dir = await makeChange('add-tracing'); + await fs.rm(path.join(dir, '.openspec.yaml')); + + await expect(sync.execute('add-tracing', { ship: true })).rejects.toThrow( + /no \.openspec\.yaml/ + ); + + // Folding first and discovering the missing file afterwards leaves a + // fold that is never stamped, and a rerun that fails in the same place. + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + it('is refused alongside --check', async () => { await makeChange('add-tracing'); @@ -450,6 +761,25 @@ describe('SyncCommand', () => { expect(process.exitCode).toBe(1); }); + it('names a blocked change rather than showing it as having no specs', async () => { + await makeChange('retire-limits', { + status: 'shipped', + delta: + '## REMOVED Requirements\n\n### Requirement: Rate limiting\n' + + '**Reason**: Moved to the gateway.\n**Migration**: Configure the gateway.\n', + }); + + await sync.execute(undefined, { check: true, json: true }); + + // Structurally unlike a dirty change: no spec entries at all, so a CI + // consumer reading `specs` alone would read this as clean. + const change = JSON.parse(output()).sync.changes[0]; + expect(change.folded).toBe(false); + expect(change.specs).toEqual([]); + expect(change.blockers[0]).toContain('openspec archive'); + expect(process.exitCode).toBe(1); + }); + it('emits one status document for a blocked run', async () => { await makeChange('add-tracing', { status: 'shipped', @@ -506,6 +836,47 @@ describe('SyncCommand', () => { }); }); + describe('stores', () => { + it("folds the selected store's specs and leaves the working directory alone", async () => { + const storeRoot = path.join(tempDir, 'stores', 'team-context'); + const storeSpec = path.join(storeRoot, 'openspec', 'specs', 'api', 'spec.md'); + const changeDir = path.join(storeRoot, 'openspec', 'changes', 'add-tracing'); + await fs.mkdir(path.join(storeRoot, 'openspec', 'specs', 'api'), { recursive: true }); + await fs.mkdir(path.join(storeRoot, 'openspec', 'changes', 'archive'), { + recursive: true, + }); + await fs.mkdir(path.join(changeDir, 'specs', 'api'), { recursive: true }); + await fs.writeFile( + path.join(storeRoot, 'openspec', 'config.yaml'), + 'schema: spec-driven\n' + ); + await fs.writeFile(storeSpec, MAIN_SPEC); + await fs.writeFile( + path.join(changeDir, '.openspec.yaml'), + 'schema: spec-driven\nstatus: shipped\n' + ); + await fs.writeFile( + path.join(changeDir, 'proposal.md'), + '## Why\nThe API needs request tracing, and today nothing correlates calls.\n\n' + + '## What Changes\n- Add request tracing to the API surface.\n' + ); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '## 1. Work\n- [x] 1.1 Done\n'); + await fs.writeFile(path.join(changeDir, 'specs', 'api', 'spec.md'), ADDED_DELTA); + await writeStoreMetadataState(storeRoot, { version: 1, id: 'team-context' }); + await writeStoreRegistryState({ + version: 1, + stores: { 'team-context': { backend: { type: 'git', local_path: storeRoot } } }, + }); + + await sync.execute(undefined, { yes: true, store: 'team-context' }); + + expect(await fs.readFile(storeSpec, 'utf-8')).toContain('Request tracing'); + // The working directory's own project has no shipped change; nothing + // there may be touched by a store-scoped run. + expect(await mainSpec()).toBe(MAIN_SPEC); + }); + }); + describe('errors', () => { it('names the available changes when the change does not exist', async () => { await makeChange('add-tracing'); @@ -567,6 +938,35 @@ describe('readChangeStatus / writeChangeStatus', () => { expect(readChangeStatus(changeDir()).status).toBe('shipped'); }); + it('leaves the state undetermined when the declared schema does not resolve', async () => { + await fs.writeFile( + path.join(changeDir(), '.openspec.yaml'), + 'schema: no-such-schema\nstatus: shipped\n' + ); + + // Distinct from a bad status value: the field parses, the schema does not + // resolve, and rounding that to `proposed` is the fail-open direction. + const marker = readChangeStatus(changeDir()); + + expect(marker.invalidReason).toContain('no-such-schema'); + expect(marker.declared).toBe(false); + }); + + it('replaces a status that is already set, in place', async () => { + await fs.writeFile( + path.join(changeDir(), '.openspec.yaml'), + '# hand-authored\nschema: spec-driven\nstatus: proposed\ncreated: 2026-09-07\n' + ); + + writeChangeStatus(changeDir(), 'shipped'); + + // Replaced, not appended: a duplicate `status` key would make the file + // parse differently in yaml and in a hand-reading author's head. + expect(await fs.readFile(path.join(changeDir(), '.openspec.yaml'), 'utf-8')).toBe( + '# hand-authored\nschema: spec-driven\nstatus: shipped\ncreated: 2026-09-07\n' + ); + }); + it('refuses to stamp a change with no metadata file', () => { expect(() => writeChangeStatus(changeDir(), 'shipped')).toThrow( /nothing to set status on/