From 679d3f7bbbd1fd180e2bb6497f5d166fa97ce573 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 7 Sep 2026 10:14:52 -0500 Subject: [PATCH] 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 | 428 ++++++++++++------ test/core/sync.test.ts | 410 ++++++++++++++++- 11 files changed, 815 insertions(+), 200 deletions(-) diff --git a/.changeset/sync-lifecycle-status.md b/.changeset/sync-lifecycle-status.md index 78f77338..513f06bf 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 ccba3825..229efdc5 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 d8ca06d5..7725dad5 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 7546dae8..84ad92fe 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 345125f3..790b550f 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 90586217..4b1d22f7 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 9867b008..9c5175c5 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 eb76bdfa..e526d316 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 d9292cec..5e23ec22 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 c71e92c6..4e1f49fa 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}`)); - } - } - } - - // 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 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)); - + // 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; + 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; + 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_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' + ); + + // 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.` + ); + } + } + + 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. * - * Returns a sentence naming what could not be restored, or undefined when the + * Two things it will not do, both mirroring `archive`'s rollback: + * + * - **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 eff3c780..22ca9df7 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/