From d24e22b1564284b71e25872e7efc565e6c06e0b9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 28 Aug 2026 08:09:05 -0400 Subject: [PATCH] enhance(#3912): gsd-tools declares outcomes, pinned at v1 (#3983) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * enhance(#3912): gsd-tools declares outcomes, pinned at v1 ADR-3889 §4. Phase 6 already moved error()'s terminator onto the seam, so what remained was the declaration — and the pin that makes it invisible today. The census corrected two documented figures before any code changed. ERROR_REASON has exactly 25 members (the ADR and epic were right; an earlier note of mine claiming 23 was wrong and is corrected). And output({error}) is **64 sites across 9 files, not the 60 ADR-2980 ratified** — the module shape holds but the total drifted +4: frontmatter 7 not 6, phase 4 not 2, roadmap 3 not 2. That matters because this phase's criterion demands the pin be asserted over the enumerated population rather than sampled; asserting over a stale 60 would leave four sites unpinned while claiming full coverage, which is the shape of failure this epic exists to remove. The issue does not state the fact that shapes the design: output() never touches the exit code. Confirmed by reading it — it writes fd 1 and returns. So a declared outcome for those 64 sites had nowhere to be READ. The mapping was never the work; wiring somewhere for the declaration to land was. The seam already existed twice over. cli-exit.cts holds two globalThis-Symbol cells, each because the module is emitted to three locations and a module-level `let` would let instances disagree, and runMain already maps a code returned by main(). A third cell inherits that solution. output() records DEGRADED for any {error} payload — key-order agnostic, which is exactly why the "42 sites" figure undercounts — and runMain projects the cell only when main() returns nothing, so an explicit return still wins. error() maps its reason through a table over the closed 25-member enum, leaving all 278 call sites untouched; 226 of them pass no reason at all. The version gate lives in error(), NOT in projectOutcome: registered names are version-invariant there, so mapping a reason straight through would make USAGE project to 64 under v1 and break the pin on its first line. projectOutcome is left exactly as Phase 2 shipped it, DEGRADED's 0/80 asymmetry included. Proven rather than asserted. v1 is byte-identical across three real CLI paths — config-get plain, config-get --json-errors, and an output({error}) path — matching exit code and exact bytes against the pre-change build. Under GSD_EXIT_CONTRACT=v2 the same commands now exit 66 (CONFIG_KEY_NOT_FOUND -> NO_INPUT) and 80 (DEGRADED), both looked up through the registry. An anti-vacuity test pins that v1 and v2 genuinely differ for at least one reason, because without it a mapping where everything projects to 1 under both versions would satisfy every other assertion and the declaration would be theatre. A1 iterates all 25 enum members and A3 asserts over the measured 64-site population, so a 26th reason or a 65th site fails until it is given a mapping — the drift guard this phase needs, given ADR-2980's own count had drifted +4 unnoticed. Verification runs on the remote runner. Refs #3912 * fix(#3912): the outcome cell must never lower an exit code The remote run caught a fail-open that this phase introduced, in the phase whose entire purpose is removing fail-opens. `state validate --strict` on a missing STATE.md exited **0** where it must exit 1. Mechanism: `runMain` projected the pending outcome whenever `main()` returned void, and under v1 DEGRADED projects to 0 — so a `process.exitCode` already set non-zero by the command was clobbered down to success. Confirmed live against a fixture, before and after. This refutes a review conclusion recorded earlier in this phase, that the cell was "fail-closed and can never mask a failure as success". It could, and did. Recording that plainly so the assumption is not repeated: the cell's danger was never only that it might add a failure — it was that projecting it unconditionally overwrites whatever decision came before. Projection is now guarded: it may set a code only when none is set, and an already-non-zero exit code always wins. The full precedence — explicit `main()` return, then an existing non-zero exitCode, then the declared outcome — is written at the projection site. A regression test drives a void return with a pre-set non-zero code and a pending DEGRADED, and fails against the pre-fix build. The second failure was my test encoding the wrong contract, not a code defect. It asserted `output({found:false, error: undefined})` records DEGRADED because the KEY is present. `JSON.stringify` drops undefined, so the payload the user receives is `{"found":false}` — carrying no error at all, and calling that degraded would hand back exit 80 under v2 for output that reads as clean. The discriminator is a serializable error VALUE, not key presence. The test now pins `{error: undefined}` as explicitly NOT degraded, and the design doc's wording is tightened to match. Verification runs on the remote runner. Refs #3912 * docs(#3912): the versioned exit contract, and a flag defect the docs found Diataxis pass for Phase 8, plus a real fix that only surfaced because writing the how-to meant running its own examples. The docs. ADR-2980's "Revisit if" clause asked for exactly the versioned projection this phase provides, so it gets an amendment naming #3912 / ADR-3889 section 4 as that boundary: v1 stays 0 byte-for-byte, v2 projects DEGRADED to 80. The amendment also records the count drift rather than restating a stale figure — the ADR ratified 60 output({error}) sites in 9 modules; the AST-measured population is 64 across the same 9 (frontmatter 7 not 6, phase 4 not 2, roadmap 3 not 2). The pin is asserted over the enumerated 64. json-errors.md gains the outcome-declaration reference, including the precedence order a review pass got wrong and the suite refuted: an explicit main() return, then an already-set non-zero process.exitCode, then the declared outcome. Projection may only ever set a code, never lower one. A how-to is owed here and is written, not skipped. Under v1 nothing changes, so the audience is an operator opting into v2 and needing to know what the codes mean for a CI gate — a migration, which is how-to shaped. It covers turning v2 on, the code table, why 80 is "ran and reported a condition" rather than a crash, and how to split a gate that treats any non-zero as fatal. No tutorial: there is no new entry point to learn, and under the default contract a reader would be walked through observing nothing. The defect. Running the how-to's own Step 1 example returned $ gsd-tools --exit-contract=v2 state validate --strict Error: Unknown command: --exit-contract=v2 (exit 64) while the same flag trailing the subcommand worked and exited 80. The flag half-worked, by argv position. resolveContractVersion scans argv non-destructively, so the token survived into the dispatcher, which treats argv[2] as the command name. --json-errors had already solved precisely this at gsd-tools.cjs:4455, under a comment naming the hazard verbatim: "The argv splice must happen here too, otherwise the dispatcher below sees --json-errors as an unknown command." The later flag never got the same treatment. Fixed rather than documented around: the version is resolved first — which memoizes the cell and makes an invalid value throw early — and then every --exit-contract= token is spliced out of the dispatcher's argv copy. --exit-contract is now listed in TOP_LEVEL_USAGE, where it never was. The regression test pins leading position, trailing position, agreement between the two, and a loud failure on v3 rather than a silent fall back to v1. Neither review engine would have caught this: the defect is invisible in the diff, because the diff does not touch argv handling. It surfaced only from running the documentation's own example. Writing a how-to is an execution pass. Verification runs on the remote runner. Refs #3912 * fix(#3912): the flag splice has to run before the run-with-timeout return An isolated review of the previous commit found that the fix did not deliver what it claimed, and that two of its own tests were weak. All three findings reproduced by execution before any change was made. The fix was placed below a return. main() intercepts `run-with-timeout` at gsd-tools.cjs:4436 and returns from there — above both the --json-errors block and the --exit-contract splice added in the previous commit. So the flag still died in leading position for that one command: $ gsd-tools --exit-contract=v2 run-with-timeout 5 -- node -e "..." Error: Unknown command: run-with-timeout (exit 64, child never ran) The previous commit message and the test's describe-block both claimed position-independence unconditionally. That was an overclaim, not a gap left open, and it is the part worth naming: the fix was verified by hand on the commands I happened to think of, and `run-with-timeout` returns before the code I was verifying. Both global-flag blocks now run above the interception, with a comment naming it so a later edit cannot slide them back down. Moving --json-errors up fixes the identical pre-existing bug for that flag, verified failing beforehand (exit 1, sdk_unknown_command). Fixing the sibling is deliberate: same defect, same block, and a known-broken twin next to a fixed one is not a resting state. Two tests were not pulling their weight. The invalid-value test was vacuous — it passed against the pre-fix build, because `--exit-contract=v3` already exited 1 there and already printed the resolve error lazily through error() -> getContractVersion. Both its assertions held before the fix, so it pinned nothing. The real discriminator is that the pre-fix build emits BOTH "Unknown command: --exit-contract=v3" and the resolve error, while the fixed build emits only the latter; the test now asserts that absence. The leading-position and leading==trailing tests asserted proxies — "not 64", "no Unknown command", "the two agree" — none of which pin a value, and all of which would survive both positions being identically broken. With a .planning directory and no STATE.md, state-snapshot exits exactly 80 under v2 and 0 under v1 in both positions. Those numbers are pinned now. The multi-token case the descending splice loop exists for is covered too, and run-with-timeout has regression tests for both flags. The lesson is narrower than "test more". Hand-verifying the production behavior does not verify that the test would have caught its absence. The pre-fix binary has to be run against the test's own assertions. Investigated and deliberately not changed: splicing before --cwd parsing degrades one diagnostic from "Missing value for --cwd" to "Invalid --cwd: ", but that is pre-existing — verified on the pre-fix build via --json-errors, which already did it. This change joins the pattern rather than creating it, and both forms exit 64 on malformed input either way. Verification runs on the remote runner. Refs #3912 * chore(#3912): backfill changeset pr numbers to 3983 * test(#3912): pin the reason-table invariant as set equality, not a count A graph-backed review flagged the unchecked lookup in expectedErrorCode3912. Investigated by execution: the drift guard DOES hold — for an unmapped reason under v2 the production error() yields 1 while the table yields undefined, so the assertion fails. Not a correctness defect, and deliberately NOT made tolerant, since a tolerant lookup would destroy the guard. Two real problems remained. The guard asserted the wrong invariant: it counted the TABLE's keys at 25 rather than checking they match the ENUM's values, so a renamed member keeps the count at 25 and slips past, and a 26th member leaves the table at 25 and slips past too. Both were then caught only indirectly, by an undefined mismatch producing 'must exit undefined'. It is now a sorted set equality, so the failure names the specific missing or extra reason. And the comment above it described a '?? FAIL' fallback that does not exist anywhere in the function. It now states what the code actually does, verified by running it rather than by reading it. Refs #3912 --------- Co-authored-by: sim --- .changeset/quick-mice-hop.md | 5 + .changeset/sharp-foxes-tumble.md | 5 + docs/FEATURES.md | 64 +++ docs/README.md | 1 + ...load-carried-error-is-a-degraded-result.md | 32 ++ ...sd-tools-declares-outcomes-pinned-at-v1.md | 64 +++ docs/how-to/adopt-the-v2-exit-contract.md | 146 +++++ docs/json-errors.md | 74 ++- gsd-core/bin/gsd-tools.cjs | 67 ++- hooks/lib/cli-exit.js | 145 ++++- scripts/lib/cli-exit.cjs | 145 ++++- src/cli-exit.cts | 145 ++++- src/io.cts | 122 +++- tests/cli-exit.test.cjs | 289 ++++++++++ tests/io.test.cjs | 529 +++++++++++++++++- 15 files changed, 1746 insertions(+), 87 deletions(-) create mode 100644 .changeset/quick-mice-hop.md create mode 100644 .changeset/sharp-foxes-tumble.md create mode 100644 docs/features/gsd-tools-declares-outcomes-pinned-at-v1.md create mode 100644 docs/how-to/adopt-the-v2-exit-contract.md diff --git a/.changeset/quick-mice-hop.md b/.changeset/quick-mice-hop.md new file mode 100644 index 000000000..b83886915 --- /dev/null +++ b/.changeset/quick-mice-hop.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3983 +--- +**Pending-outcome cell no longer leaks across calls in one process.** A CLI run that calls `output()` with a payload-carried error and later returns cleanly, or that runs a second `main()` in the same process, could inherit a stale DEGRADED exit code (80 under the v2 exit contract) from an earlier declaration. The cell now follows last-write-wins semantics and is cleared on consumption. (#3912) diff --git a/.changeset/sharp-foxes-tumble.md b/.changeset/sharp-foxes-tumble.md new file mode 100644 index 000000000..98d344b0b --- /dev/null +++ b/.changeset/sharp-foxes-tumble.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3983 +--- +**Global flags now work in any argv position, including before `run-with-timeout`.** Passing `--exit-contract=` before the subcommand — `gsd-tools --exit-contract=v2 state validate` — failed with `Error: Unknown command: --exit-contract=v2`, because the token was read for version resolution but never removed from argv, so the dispatcher treated it as the command name. Separately, `gsd-tools --json-errors run-with-timeout ...` failed with `Unknown command: run-with-timeout` and never ran the child, because `run-with-timeout` is intercepted before the flag is stripped. Both flags are now resolved and stripped ahead of that interception, and `--exit-contract` is listed in `gsd-tools --help`. (#3912) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 2bf74730f..0c761450e 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -202,6 +202,7 @@ - [Runtime Marker Resolution, Derived Codex Sandbox, and In-Phase Short-Form Dependencies](#3897-runtime-marker-resolution-derived-codex-sandbox-and-in-phase-short-form-dependencies) - [The Raw Terminator Is Banned by Construction](#3910-the-raw-terminator-is-banned-by-construction) - [Hooks Declare Their Crash Policy](#3911-hooks-declare-their-crash-policy) + - [gsd-tools Declares Outcomes, Pinned at v1](#3912-gsd-tools-declares-outcomes-pinned-at-v1) - [Reachable Lint Rules and a Non-Destructive Quick-Task Append](#3951-reachable-lint-rules-and-a-non-destructive-quick-task-append) --- @@ -4028,6 +4029,69 @@ this vocabulary is layered over. --- +### 3912. gsd-tools Declares Outcomes, Pinned at v1 + +**Purpose:** Give every `gsd-tools` terminating path a declared outcome name, and project that +declaration through the versioned exit contract ([ADR-3889](../adr/3889-process-exit-contract.md) +§4) — without changing a single exit code for a caller that has not opted in. + +**Reference — what changed (ADR-3889 Phase 8, #3912):** + +- `error(message, reason)` now maps its `reason` argument onto a declared outcome name (`USAGE`, + `NO_INPUT`, `UNAVAILABLE`, `INTERNAL`, `FAIL`) via a fixed table closed over all 25 + `ERROR_REASON` members (`src/io.cts`'s `REASON_TO_OUTCOME`). Under the default contract version + `v1`, the declaration is recorded but `error()` still throws `ExitError(1)` unconditionally, + byte-identical to every prior release. Under `v2` (`--exit-contract=v2` / + `GSD_EXIT_CONTRACT=v2`), it throws `ExitError(projectOutcome(outcome, 'v2'))` instead — e.g. + `SDK_MISSING_ARG`/`SDK_UNKNOWN_COMMAND` project to `64` (`USAGE`), `CONFIG_KEY_NOT_FOUND` to `66` + (`NO_INPUT`). All 278 call sites are untouched; 226 pass no reason and default to `UNKNOWN` -> + `FAIL` -> exit `1` under both versions. +- `output()` now declares `DEGRADED` whenever its payload carries a **serializable** `error` value + (any key order — `{found:false, error}` counts the same as `{error, found:false}`). The + discriminator is survives-`JSON.stringify`, not mere key presence: `{ error: undefined }` does + **not** declare `DEGRADED`, because `JSON.stringify` drops an `undefined`-valued property before + the payload reaches the wire. +- A third `globalThis` cell (`src/cli-exit.cts`'s `PENDING_OUTCOME_KEY`) holds the pending declared + outcome between `output()` and `runMain`. Semantics: **last declaration wins, cleared on + consumption** — a later clean `output()` call in the same invocation undoes an earlier degraded + one, and `runMain` clears the cell on every exit so a second `runMain` in the same process never + inherits a stale declaration. +- **Precedence** for the code a void-returning `main()` ends up with, highest first: (1) an explicit + `main()` return, (2) a non-zero `process.exitCode` `main()` already set directly, (3) the pending + declared outcome, (4) otherwise `0`. Projection may only ever **set** a code, never **lower** one + — a review pass wrongly concluded the cell was fail-closed by construction; without rule 2, `state + validate --strict` briefly exited `0` where it must exit `1`. +- **`v1` is byte-identical.** `DEGRADED` projects to `0` under `v1` and to `80` + (`exitCodeFor('DEGRADED')`) under `v2` — that asymmetry is + [ADR-2980](../adr/2980-payload-carried-error-is-a-degraded-result.md)'s compatibility boundary, + deliberately preserved, not a bug to reconcile. + +**Explanation — why this is the shape it is:** + +ADR-2980 ratified `output({error})`'s exit-0 population on measured blast radius (`output` has 170 +direct callers) and Hyrum's Law grounds — a CLI exit code has no `/v2/` of its own, so normalizing +it in place would have broken every caller already treating exit `0` as a soft signal. Its own +"Revisit if" clause named the missing piece: *"a future `gsd-tools` major version provides a +compatibility boundary that a CLI exit code otherwise lacks."* ADR-3889 §4 built exactly that +boundary — a versioned projection selected by flag or env var, defaulting to today's behavior — and +this phase is what wires `error()` and `output()` onto it. Declaring an outcome is unconditional and +immediate; only its *projection* onto an integer is deferred behind the version switch, so the +population ADR-2980 ratified keeps exiting `0` until a caller explicitly asks for something else. + +The count matters here too: an AST re-measure for this phase found **64** `output({error})` call +sites across the same nine modules ADR-2980 named — not the 60 that ADR itself recorded, the drift +concentrated in `frontmatter.cts`, `phase.cts`, and `roadmap.cts`. The `v2` projection is asserted +over the enumerated 64, not a restated 60; see ADR-2980's amendment for the module-by-module +breakdown. + +See [Adopt the v2 exit contract](../how-to/adopt-the-v2-exit-contract.md) for how to opt in and what +it means for a CI gate, [`docs/json-errors.md`](../json-errors.md#outcome-declaration-and-the-versioned-exit-contract-adr-3889-4-3912) +for the full reference, and +[ADR-2980](../adr/2980-payload-carried-error-is-a-degraded-result.md) / +[ADR-3889](../adr/3889-process-exit-contract.md) for the decisions. + +--- + ### 3951. Reachable Lint Rules and a Non-Destructive Quick-Task Append **Purpose:** Make two ESLint rules cover the code they were written to govern, and stop diff --git a/docs/README.md b/docs/README.md index e3b3ab8c7..3840f31bd 100644 --- a/docs/README.md +++ b/docs/README.md @@ -36,6 +36,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [Diagnose which gsd-tools is running](how-to/diagnose-a-foreign-gsd-tools.md) — tell this package's tool apart from the predecessor's colliding binary and from a gsd-core too old to identify itself - [Resolve an ESLint glob-coverage finding](how-to/resolve-eslint-coverage-findings.md) — bring a source file that matches no lint rule under coverage, or record a reasoned exemption - [Resolve a raw-terminator finding](how-to/resolve-a-raw-terminator-finding.md) — pick `runMain`/`ExitError`, `terminateNow`, or `process.exitCode` for a `local/require-registered-exit` finding, and know the two allowlist entries and the rule's documented evasions +- [Adopt the v2 exit contract](how-to/adopt-the-v2-exit-contract.md) — turn on `gsd-tools`'s versioned exit-code projection, read the code table including what `80` (`DEGRADED`) means, and migrate a CI gate that treats any non-zero exit as fatal - [Read the statusline freshness marker](how-to/read-the-statusline-freshness-marker.md) — turn on `state ~N commits back`, and tell "STATE.md is fresh" apart from "freshness could not be established" - [Consume the planning snapshot](how-to/consume-the-planning-snapshot.md) — read `planning inspect` from a dashboard or harness, and tell "nothing to report" apart from "could not look" - [Consume the state contract](how-to/consume-the-state-contract.md) — read `.planning/state.json` from a workbench or editor extension, gate on the contract version, and tell "nothing to show" apart from "could not look" diff --git a/docs/adr/2980-payload-carried-error-is-a-degraded-result.md b/docs/adr/2980-payload-carried-error-is-a-degraded-result.md index 0b281d938..7fa45428f 100644 --- a/docs/adr/2980-payload-carried-error-is-a-degraded-result.md +++ b/docs/adr/2980-payload-carried-error-is-a-degraded-result.md @@ -182,3 +182,35 @@ long-lived observable behavior is to *document what is stable*, which is what th far smaller radius than the full 60. - A future `gsd-tools` major version provides a compatibility boundary that a CLI exit code otherwise lacks. + +## Amendment — 2026-08-27: the compatibility boundary now exists (#3912, ADR-3889 §4) + +The third bullet above is answered. [ADR-3889](3889-process-exit-contract.md) §4 gives `gsd-tools` +a versioned exit-contract projection (`v1`/`v2`, selected by `--exit-contract=`/ +`GSD_EXIT_CONTRACT=`), and [#3912](https://github.com/open-gsd/gsd-core/issues/3912) is the phase +that lands the projection this idiom's population feeds. + +- **`output()` now declares an outcome.** A payload carrying a *serializable* `error` key — any key + order, `error: undefined` excluded because `JSON.stringify` drops it before the wire — records + `DEGRADED` into the pending-outcome cell `runMain` reads (`src/cli-exit.cts`, `src/io.cts`). +- **Under `v1` — today's default — nothing here changes anything.** Every ratified site still exits + `0` byte-for-byte; the declaration is recorded but `projectOutcome` pins `DEGRADED -> 0` under + `v1` specifically so this ADR's Consequences ("no call site, exit code, or payload shape changes") + stays true. +- **Under `v2`, a caller that opts in gets a real signal.** `DEGRADED` projects to exit code `80` + (`exitCodeFor('DEGRADED')`, ADR-3889's registry) — non-zero, so `if ! cmd; then` finally trips on + this idiom, without moving a single call site or changing the payload shape Option 2/3 above + declined to touch. +- **The population this ADR ratifies is 64, not 60.** This ADR's own site count was measured by + brace-matching in 2026-08-09; an AST-based re-measure for #3912 finds the same **nine modules** + but **64** `output({error})` call sites, not 60 — the drift concentrates in + `src/frontmatter.cts` (7, not 6), `src/phase.cts` (4, not 2), and `src/roadmap.cts` (3, not 2). + Do not restate 60 as current: the `v2` `DEGRADED` projection above is asserted over the enumerated + **64**, and any future re-measure that finds a different count should amend this note rather than + silently correct the number back. + +This does not reopen the Decision above. The 64 sites are still unchanged, still exit `0` under the +default contract, and the richer named-field shape (`state update-progress`'s +`{"updated": false, "reason": …}`) is still the recommended shape for new code. What changes is that +a caller who explicitly asks for `v2` no longer has to parse the payload to notice a degraded +result — the exit code says so. diff --git a/docs/features/gsd-tools-declares-outcomes-pinned-at-v1.md b/docs/features/gsd-tools-declares-outcomes-pinned-at-v1.md new file mode 100644 index 000000000..741e2efb1 --- /dev/null +++ b/docs/features/gsd-tools-declares-outcomes-pinned-at-v1.md @@ -0,0 +1,64 @@ +--- +id: 3912 +title: gsd-tools Declares Outcomes, Pinned at v1 +group: v1.7.0 Features +--- + +**Purpose:** Give every `gsd-tools` terminating path a declared outcome name, and project that +declaration through the versioned exit contract ([ADR-3889](../adr/3889-process-exit-contract.md) +§4) — without changing a single exit code for a caller that has not opted in. + +**Reference — what changed (ADR-3889 Phase 8, #3912):** + +- `error(message, reason)` now maps its `reason` argument onto a declared outcome name (`USAGE`, + `NO_INPUT`, `UNAVAILABLE`, `INTERNAL`, `FAIL`) via a fixed table closed over all 25 + `ERROR_REASON` members (`src/io.cts`'s `REASON_TO_OUTCOME`). Under the default contract version + `v1`, the declaration is recorded but `error()` still throws `ExitError(1)` unconditionally, + byte-identical to every prior release. Under `v2` (`--exit-contract=v2` / + `GSD_EXIT_CONTRACT=v2`), it throws `ExitError(projectOutcome(outcome, 'v2'))` instead — e.g. + `SDK_MISSING_ARG`/`SDK_UNKNOWN_COMMAND` project to `64` (`USAGE`), `CONFIG_KEY_NOT_FOUND` to `66` + (`NO_INPUT`). All 278 call sites are untouched; 226 pass no reason and default to `UNKNOWN` -> + `FAIL` -> exit `1` under both versions. +- `output()` now declares `DEGRADED` whenever its payload carries a **serializable** `error` value + (any key order — `{found:false, error}` counts the same as `{error, found:false}`). The + discriminator is survives-`JSON.stringify`, not mere key presence: `{ error: undefined }` does + **not** declare `DEGRADED`, because `JSON.stringify` drops an `undefined`-valued property before + the payload reaches the wire. +- A third `globalThis` cell (`src/cli-exit.cts`'s `PENDING_OUTCOME_KEY`) holds the pending declared + outcome between `output()` and `runMain`. Semantics: **last declaration wins, cleared on + consumption** — a later clean `output()` call in the same invocation undoes an earlier degraded + one, and `runMain` clears the cell on every exit so a second `runMain` in the same process never + inherits a stale declaration. +- **Precedence** for the code a void-returning `main()` ends up with, highest first: (1) an explicit + `main()` return, (2) a non-zero `process.exitCode` `main()` already set directly, (3) the pending + declared outcome, (4) otherwise `0`. Projection may only ever **set** a code, never **lower** one + — a review pass wrongly concluded the cell was fail-closed by construction; without rule 2, `state + validate --strict` briefly exited `0` where it must exit `1`. +- **`v1` is byte-identical.** `DEGRADED` projects to `0` under `v1` and to `80` + (`exitCodeFor('DEGRADED')`) under `v2` — that asymmetry is + [ADR-2980](../adr/2980-payload-carried-error-is-a-degraded-result.md)'s compatibility boundary, + deliberately preserved, not a bug to reconcile. + +**Explanation — why this is the shape it is:** + +ADR-2980 ratified `output({error})`'s exit-0 population on measured blast radius (`output` has 170 +direct callers) and Hyrum's Law grounds — a CLI exit code has no `/v2/` of its own, so normalizing +it in place would have broken every caller already treating exit `0` as a soft signal. Its own +"Revisit if" clause named the missing piece: *"a future `gsd-tools` major version provides a +compatibility boundary that a CLI exit code otherwise lacks."* ADR-3889 §4 built exactly that +boundary — a versioned projection selected by flag or env var, defaulting to today's behavior — and +this phase is what wires `error()` and `output()` onto it. Declaring an outcome is unconditional and +immediate; only its *projection* onto an integer is deferred behind the version switch, so the +population ADR-2980 ratified keeps exiting `0` until a caller explicitly asks for something else. + +The count matters here too: an AST re-measure for this phase found **64** `output({error})` call +sites across the same nine modules ADR-2980 named — not the 60 that ADR itself recorded, the drift +concentrated in `frontmatter.cts`, `phase.cts`, and `roadmap.cts`. The `v2` projection is asserted +over the enumerated 64, not a restated 60; see ADR-2980's amendment for the module-by-module +breakdown. + +See [Adopt the v2 exit contract](../how-to/adopt-the-v2-exit-contract.md) for how to opt in and what +it means for a CI gate, [`docs/json-errors.md`](../json-errors.md#outcome-declaration-and-the-versioned-exit-contract-adr-3889-4-3912) +for the full reference, and +[ADR-2980](../adr/2980-payload-carried-error-is-a-degraded-result.md) / +[ADR-3889](../adr/3889-process-exit-contract.md) for the decisions. diff --git a/docs/how-to/adopt-the-v2-exit-contract.md b/docs/how-to/adopt-the-v2-exit-contract.md new file mode 100644 index 000000000..270eee930 --- /dev/null +++ b/docs/how-to/adopt-the-v2-exit-contract.md @@ -0,0 +1,146 @@ +# How to adopt the v2 exit contract + +`gsd-tools` runs, by default, under exit-contract **`v1`** — every observable exit code is +byte-identical to every prior release. **`v2`** is an opt-in projection, per +[ADR-3889](../adr/3889-process-exit-contract.md) §4, that turns a small set of previously +same-looking outcomes into distinct, non-zero exit codes a CI gate can branch on without parsing +stdout. This page covers how to turn it on, what the new codes mean, and how to migrate a gate that +today treats "any non-zero exit" as fatal. + +## Should you turn this on? + +Turn it on if a script or CI gate wraps `gsd-tools` and needs to tell "ran fine", "you called it +wrong", "there was nothing to find", "a prerequisite was missing", "it crashed", and "it ran but is +reporting a condition in its payload" apart **from the exit code alone**, instead of parsing JSON on +stdout to find out. If your caller only ever needs pass/fail, `v1` already gives you that — there is +nothing to adopt. + +## Step 1 — turn it on + +Either a flag or an environment variable activates `v2`. The flag beats the env var if both are +given in the same invocation. + +```bash +# Per-invocation (preferred in test code and one-off scripts): +node gsd-tools.cjs --exit-contract=v2 state validate --strict + +# Process-wide (preferred for CI and shell wrappers): +export GSD_EXIT_CONTRACT=v2 +gsd-tools state validate --strict +``` + +The flag works in any argv position — before the subcommand or after it — and `gsd-tools` strips +it before dispatch, exactly as it already does for `--json-errors`. + +Only the exact lowercase tokens `v1`/`v2` are accepted. Anything else present — a typo, `V2`, +`v3`, or an explicitly empty `--exit-contract=` — throws rather than silently falling back to `v1`; +a selector for a contract whose whole point is "nothing fails with success" must not itself fail +open. That throw surfaces as a stack trace at exit `1`, not as a tidy `USAGE` `64`, and that is +deliberate: the failure is *the version being unresolvable*, so there is no contract version yet +under which to project a code. Loud and coarse beats quiet and wrong. An empty +`GSD_EXIT_CONTRACT=` (nothing after the `=`) reads as **unset**, not as an explicit selection, so a +shell that exports it empty still gets `v1`. + +## Step 2 — read the code table + +`v2` projects a declared outcome through one generated registry +(`gsd-core/bin/lib/exit-code-registry.cjs`). Every code below is stable and machine-checked; do not +hardcode the integers in your own scripts — call `exitCodeFor(name)` if you are writing Node, or +just compare against the number after reading it here once. + +| Code | Name | Meaning | +|--:|---|---| +| `0` | (PASS) | The operation ran and its verdict is affirmative. | +| `1` | (FAIL) | The operation ran and its verdict is negative — the honest default when nothing more specific applies. | +| `64` | `USAGE` | Caller error — bad argv, unknown subcommand, missing required argument. | +| `66` | `NO_INPUT` | Ran; zero units were in scope, and that emptiness is known to be genuine. | +| `69` | `UNAVAILABLE` | Could not run — a prerequisite was absent, unreadable, or scope was never established. | +| `70` | `INTERNAL` | Self-failure — the run itself broke (crash, timeout, killed subprocess), not its inputs. | +| `80` | `DEGRADED` | Ran to completion and is **reporting a condition through its result payload**, not failing as a process. | + +`2` is reserved to the Claude Code hook-protocol deny and is never produced by `gsd-tools` itself — +you will not see it from a CLI invocation. + +## Step 3 — understand `80` specifically + +`80` (`DEGRADED`) is the one code most CI authors get wrong, because it is the one code where "ran +to completion" and "found a problem" are the same event. It fires when a command's JSON payload +carries a **serializable** `error` key — for example `gsd-tools state-snapshot` in a project with +no `STATE.md` returns `{"error": "STATE.md not found"}`. Under `v1` that exits `0`; under `v2` it +exits `80`. + +`80` is **not** a crash. The process did its job: it determined, correctly, that the artifact you +asked about is absent, unparseable, or that a required argument was missing — see +[ADR-2980](../adr/2980-payload-carried-error-is-a-degraded-result.md) for the full population this +covers (64 call sites across nine modules) and why it stayed a payload-carried signal rather than a +thrown fault. `70` (`INTERNAL`) is the code for an actual crash. Do not conflate the two: a gate +that maps `80` to "the tool is broken" will page someone for a condition the tool successfully +diagnosed. + +## Step 4 — migrate a gate that treats any non-zero exit as fatal + +The naive shell form, + +```sh +if ! gsd-tools state-snapshot > snap.json; then + echo "gsd-tools failed" >&2 + exit 1 +fi +``` + +is **correct as a fail-safe** under `v2` — every registered code is non-zero, so this can only turn +a false green red, never a red green (ADR-3889 §5). What it cannot do on its own is tell you *which* +non-zero condition fired, which matters if your policy is "treat `DEGRADED` as a soft warning but +still hard-fail on `USAGE`/`UNAVAILABLE`/`INTERNAL`": + +```sh +set +e +gsd-tools --exit-contract=v2 state-snapshot > snap.json +code=$? +set -e + +case "$code" in + 0) ;; # PASS + 80) echo "degraded result — inspect snap.json" >&2 ;; # ran, reported a condition — your call whether this gates the pipeline + 64|66|69|70) + echo "gsd-tools failed (exit $code)" >&2 + exit 1 + ;; + *) echo "gsd-tools failed (unrecognized exit $code)" >&2 + exit 1 + ;; +esac +``` + +Whether `DEGRADED` itself should gate your pipeline is a policy decision only you can make — the +contract's only promise is that `80` is distinguishable from `64`/`66`/`69`/`70`, not that it is +always safe to ignore. A gate that wants the old, coarser behavior (any non-zero is fatal, including +`80`) needs no case statement at all; the naive form above already does that correctly. + +## Four things that will surprise you + +1. **`--json-errors` is a different, orthogonal switch.** It governs whether `error()`'s stderr + envelope is JSON or plain text; it does nothing to `output()`'s exit code. You can run `v2` with + or without `--json-errors`. +2. **Precedence can surprise a caller reading only `output()`'s contract.** An explicit `main()` + return, or a non-zero `process.exitCode` a command set directly, always wins over a `DEGRADED` + declared earlier in the same invocation — projection can only raise a code, never lower one. See + [`docs/json-errors.md`](../json-errors.md#outcome-declaration-and-the-versioned-exit-contract-adr-3889-4-3912) + for the full precedence order. +3. **The declaration does not accumulate.** If a command calls `output()` more than once — a + diagnostic degraded payload followed by a clean final one — only the **last** call's declaration + is live when the process exits. +4. **`v1` and `v2` are the only recognized versions today**, and the default flips to `v2` at the + next major (ADR-3889 §4). Pin `--exit-contract=v1` explicitly in a script that must keep today's + codes indefinitely, rather than relying on the current default staying `v1` forever. + +## Related + +- [ADR-3889](../adr/3889-process-exit-contract.md) — the exit-code registry and the versioned + projection this page walks through +- [ADR-2980](../adr/2980-payload-carried-error-is-a-degraded-result.md) — why `output({error})` + exits `0` under `v1`, and the population `DEGRADED` covers under `v2` +- [`docs/json-errors.md`](../json-errors.md) — the full reference for both failure channels, + the error-code taxonomy, and the outcome-declaration precedence rules +- [Resolve a raw-terminator finding](resolve-a-raw-terminator-finding.md) — the sibling page for the + lint rule that keeps every termination path routed through this same seam diff --git a/docs/json-errors.md b/docs/json-errors.md index f6d1e3cb9..33f41ff12 100644 --- a/docs/json-errors.md +++ b/docs/json-errors.md @@ -110,10 +110,13 @@ $ echo $? This is a **ratified contract**, not an accident — see [ADR-2980](adr/2980-payload-carried-error-is-a-degraded-result.md) for the decision and the blast -radius that drove it. It applies to **60 call sites across nine modules** — `state`, `verify`, +radius that drove it. It applies to **64 call sites across nine modules** — `state`, `verify`, `workstream`, `frontmatter`, `commands`, `template`, `phase`, `roadmap`, and `gsd2-import`. (Issues #2966 and #2980 record this as "42 sites"; that figure counts only the sites where `error` -happens to be the object's first key. See ADR-2980 for why the real number is 60.) +happens to be the object's first key. ADR-2980 itself re-derived the population as "60" by +brace-matching; a further AST re-measure for [#3912](https://github.com/open-gsd/gsd-core/issues/3912) +found the true current count is 64 — the same nine modules, with `frontmatter`, `phase`, and +`roadmap` each having grown since. See ADR-2980's amendment for the breakdown.) ### Writing a correct caller @@ -169,6 +172,73 @@ $ gsd-tools state update-progress # STATE.md present, no Progress fie } ``` +## Outcome declaration and the versioned exit contract (ADR-3889 §4, #3912) + +Both failure channels above now **declare an outcome** on every terminating path, per +[ADR-3889](adr/3889-process-exit-contract.md). Declaration is unconditional; whether it changes the +observed exit code depends on which **exit-contract version** the process is running under. + +Turn on `v2` with either `--exit-contract=v2` or `GSD_EXIT_CONTRACT=v2` (a flag beats the env var if +both are given). Absent either, the process runs `v1` — today's default and, for every existing +caller, byte-identical to pre-#3912 behavior. See +[Adopt the v2 exit contract](how-to/adopt-the-v2-exit-contract.md) for a worked migration. + +### `error(message, reason)` + +`error()`'s `reason` argument now maps onto a declared outcome name (`USAGE`, `NO_INPUT`, +`UNAVAILABLE`, `INTERNAL`, or `FAIL`) via a fixed table over all 25 `ERROR_REASON` members. + +- **Under `v1`, the mapping is recorded but never projected.** `error()` still throws + `ExitError(1)` unconditionally, exactly as before — stderr and the exit code are byte-identical to + every prior release. +- **Under `v2`, the mapping is projected through the exit-code registry.** `error()` throws + `ExitError(exitCodeFor())` instead of a hardcoded `1` — so, for example, a call + with `ERROR_REASON.SDK_MISSING_ARG` or `ERROR_REASON.SDK_UNKNOWN_COMMAND` exits `64` (`USAGE`) + under `v2`, and one with `ERROR_REASON.CONFIG_KEY_NOT_FOUND` exits `66` (`NO_INPUT`). +- **Most call sites are unaffected either way.** 226 of the 278 `error()` call sites in the repo + pass no `reason` at all, defaulting to `ERROR_REASON.UNKNOWN`, which maps to the generic `FAIL` + outcome (exit `1`) under both versions. + +### `output({ error: … })` — a degraded result is also a declared outcome + +The degraded-result idiom above now declares the outcome `DEGRADED` whenever `output()`'s payload +carries a **serializable** `error` value — any key order, and regardless of that value's own +truthiness (`0`/`null`/`''` all count). The one exclusion: `{ error: undefined }` does **not** +declare `DEGRADED`, because `JSON.stringify` (the exact serializer `output()` uses) drops an +object property whose value is `undefined` before it ever reaches the wire — a payload built that +way reaches the caller as `{}`, with nothing to be degraded about. + +- **Under `v1`, `DEGRADED` projects to `0`** — deliberately: this is ADR-2980's compatibility + boundary, pinned so all 64 ratified sites keep exiting `0` byte-for-byte. +- **Under `v2`, `DEGRADED` projects to `80`** — looked up from the exit-code registry, never + hardcoded, so a future re-allocation of `DEGRADED`'s number cannot silently desync this doc from + the shipped table. + +### Precedence — what code a void-returning command actually exits with + +A command's `main()` can end up producing a code from more than one source. The order, highest +precedence first, is: + +1. **An explicit `main()` return** (a number or a registered outcome-name string) — always wins. +2. **A non-zero `process.exitCode` `main()` already set directly** before returning — wins over + anything declared through `output()`. This is what keeps `state validate --strict` correct: it + sets `process.exitCode = 1` itself on a missing `STATE.md`, and a `DEGRADED` declared earlier in + the same call must not clobber that `1` back down to `DEGRADED`'s `v1` projection of `0`. +3. **The declared outcome pending from `output()`** — consulted only when neither of the above set + anything. +4. Otherwise the process exits `0`. + +**Projection may only ever set a code, never lower one.** A prior review pass concluded the pending +declaration was fail-closed by construction; it was not — without rule 2 above, `state validate +--strict` briefly exited `0` on a case that must exit `1`. If you add a new call path that sets +`process.exitCode` directly, check it still wins over a later `output({error})` in the same +invocation. + +**The declaration does not accumulate across calls.** `output()`'s declaration follows +last-write-wins: a clean payload clears a prior `DEGRADED` declaration in the same invocation, and +`runMain` clears the cell on every exit regardless of which branch produced the final code, so a +later `runMain` call in the same process never inherits a stale declaration. + ## Error code taxonomy Codes are frozen constants in `gsd-core/bin/lib/core.cjs` under diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 8fca5cf02..d3e076371 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -258,7 +258,7 @@ try { process.exit(1); } -const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { ExitError, runMain, resolveContractVersion } = require('./lib/cli-exit.cjs'); const io = require('./lib/io.cjs'); const { error, ERROR_REASON, setJsonErrorMode, output, formatDiagnosticToken } = io; const projectRoot = require('./lib/project-root.cjs'); @@ -4318,7 +4318,7 @@ function runWithTimeout(argv) { // this string and HOST_COMMAND_ROUTERS/SKIP_ROOT_RESOLUTION are three // independently hand-maintained sites and nothing previously caught them // drifting apart when a query command was added to only one or two. -const TOP_LEVEL_USAGE = 'Usage: gsd-tools [args] [--raw] [--pick ] [--cwd ] [--project-dir ] [--ws ] [--json-errors]\n' + +const TOP_LEVEL_USAGE = 'Usage: gsd-tools [args] [--raw] [--pick ] [--cwd ] [--project-dir ] [--ws ] [--json-errors] [--exit-contract=]\n' + 'Commands: agent, agent-skills, assumption-delta, audit-open, audit-uat, check, check-commit, commit, commit-docs-guard, commit-to-subrepo, pr-subrepo, ' + 'config-ensure-section, config-get, config-new-project, config-path, config-set, migrate-config, normalize-test-command, ' + 'context-predicates, current-timestamp, detect-custom-files, docs-init, drift-guard, effort, extract-messages, find-phase, ' + @@ -4335,7 +4335,8 @@ const TOP_LEVEL_USAGE = 'Usage: gsd-tools [args] [--raw] [--pick Override working directory for project-root resolution\n' + ' --project-dir Explicit project root; skips the ancestor walk-up entirely (must already contain .planning/)\n' + ' --ws Override active workstream (or set GSD_WORKSTREAM)\n' + - ' --json-errors Emit structured JSON error objects on stderr (or set GSD_JSON_ERRORS=1)\n\n' + + ' --json-errors Emit structured JSON error objects on stderr (or set GSD_JSON_ERRORS=1)\n' + + ' --exit-contract= Exit-code contract version: v1 (default) or v2 (or set GSD_EXIT_CONTRACT)\n\n' + 'For command-specific argument requirements, invoke the command without args ' + '(e.g. `gsd-tools phase add`) — the resulting error lists what is required.'; @@ -4429,18 +4430,19 @@ function resolveMainWorktreeCwd(cwd, deps = {}) { async function main() { let args = process.argv.slice(2); - // #2351: run-with-timeout bounds a spawned command's wall clock portably - // (coreutils-independent). It MUST intercept HERE, before the global-flag - // parsing below — the wrapped command's argv is opaque and may itself contain - // --raw / --cwd / --pick that this dispatcher would otherwise consume. - { - let rwt = args; - if (rwt[0] === 'query') rwt = rwt.slice(1); - if (rwt[0] === 'run-with-timeout') { - // Return the child's exit code; runMain() maps it to process.exitCode. - return runWithTimeout(rwt.slice(1)); - } - } + // These two global-flag blocks (--json-errors, --exit-contract) MUST run + // BEFORE the run-with-timeout interception below. run-with-timeout treats + // args[0] (post `query` stripping) as the sentinel and otherwise passes the + // remaining argv straight to the wrapped child — it never reaches the + // dispatcher's "Unknown command" fallback, but a global flag left in LEADING + // position (e.g. `--exit-contract=v2 run-with-timeout ...`) would be spliced + // out too late if these ran after, since neither block currently exists + // below this point to consume it. Splicing here, before run-with-timeout's + // own argv slicing, is what keeps both flags position-independent for every + // command, run-with-timeout included. Do not move these back below the + // run-with-timeout block (#confirmed regression: leading --exit-contract=v2 + // and leading --json-errors both broke run-with-timeout when these blocks + // sat after it). // --json-errors / GSD_JSON_ERRORS=1: when active, error() emits structured // JSON ({ ok: false, reason: , message }) to stderr @@ -4460,6 +4462,41 @@ async function main() { setJsonErrorMode(true); } + // --exit-contract= / GSD_EXIT_CONTRACT: resolve FIRST, before the splice + // below, so an invalid value (e.g. `v3`, or an empty `--exit-contract=`) + // throws EARLY — matching the --json-errors block's own "detect early, + // before any flag parsing that can fire error()" rationale above. This also + // memoizes the resolved version into the shared contract-version cell so a + // later terminateNow()/runMain() call projects against it correctly. + // + // The argv splice must happen here too, otherwise the dispatcher below sees + // "--exit-contract=" as an unknown command when the flag is given in + // LEADING position (argv[0] is what the dispatcher treats as the command + // name). Splice EVERY occurrence, not just the first — findExitContractFlag + // only consults the first match, so a stray second token would otherwise + // survive into the dispatcher and reproduce the same "Unknown command". + resolveContractVersion({ argv: process.argv, env: process.env }); + for (let i = args.length - 1; i >= 0; i--) { + if (typeof args[i] === 'string' && args[i].startsWith('--exit-contract=')) { + args.splice(i, 1); + } + } + + // #2351: run-with-timeout bounds a spawned command's wall clock portably + // (coreutils-independent). It MUST intercept HERE, before the remaining + // flag parsing below — the wrapped command's argv is opaque and may itself + // contain --raw / --cwd / --pick that this dispatcher would otherwise + // consume. (--json-errors / --exit-contract are handled above this block, + // not below, precisely so they keep working with run-with-timeout.) + { + let rwt = args; + if (rwt[0] === 'query') rwt = rwt.slice(1); + if (rwt[0] === 'run-with-timeout') { + // Return the child's exit code; runMain() maps it to process.exitCode. + return runWithTimeout(rwt.slice(1)); + } + } + // Optional cwd override for sandboxed subagents running outside project root. let cwd = process.cwd(); const cwdEqArg = args.find(arg => arg.startsWith('--cwd=')); diff --git a/hooks/lib/cli-exit.js b/hooks/lib/cli-exit.js index c1f14ad48..a2e7788f6 100644 --- a/hooks/lib/cli-exit.js +++ b/hooks/lib/cli-exit.js @@ -150,6 +150,41 @@ function projectOutcome(outcome, version) { // exitCodeFor's own contract, which this function inherits verbatim). return exitCodeFor(outcome); } +/** + * Pending declared outcome (ADR-3889 §4, #3912): the outcome `output()` + * records when it detects a payload-carried error (`{ error }`, any key + * order) on a call that otherwise just returns — there is no thrown + * ExitError and no explicit `main()` return for `runMain` to project, so + * without this cell the declaration has nowhere to land. `runMain` reads it + * ONLY when `main()` itself returns no explicit code (void/undefined); an + * explicit number/string return always wins over whatever this cell holds. + * + * Held in a Symbol-keyed globalThis cell for the exact reason + * JSON_ERROR_MODE_KEY / CONTRACT_VERSION_KEY are (see their comments above): + * this module is emitted to three locations and thus loaded as independent + * module instances in any process that requires more than one, so a + * module-level variable would let those instances disagree about whether a + * degraded result was ever declared. + * + * LIFETIME (#3912 review fix): LAST DECLARATION WINS, CLEARED ON CONSUMPTION. + * This cell is NOT "was DEGRADED ever declared this process" — it is "is a + * degraded outcome pending RIGHT NOW". `output()` sets it to 'DEGRADED' on a + * payload-carried error and CLEARS it (`undefined`) on a clean payload, so a + * later clean `output()` call undoes an earlier degraded one in the same + * invocation. `runMain` clears it immediately after consuming it (in a + * `finally`, on both the pending-cell branch and the case where nothing was + * pending), so a second `runMain` in the same process starts clean. Without + * both halves the cell is monotonic for the life of the process: any later + * `main()` returning void would inherit a stale DEGRADED from an unrelated, + * earlier call — this is the leak #3912 review found and fixed. + */ +const PENDING_OUTCOME_KEY = Symbol.for('gsd.exit.pendingOutcome'); +function setPendingOutcome(v) { + globalThis[PENDING_OUTCOME_KEY] = v; +} +function getPendingOutcome() { + return globalThis[PENDING_OUTCOME_KEY]; +} const EXIT_CONTRACT_FLAG_PREFIX = '--exit-contract='; /** Scan argv for the FIRST `--exit-contract=` token; undefined if absent. */ function findExitContractFlag(argv) { @@ -223,11 +258,44 @@ class ExitError extends Error { * process.on('exit') cleanup still fires — this is precisely why runMain and * terminateNow are two different functions: drain-then-exit vs write-then- * terminate). main may be sync or async. Every arm below except the new - * string one is UNCHANGED from before ADR-3889 Phase 2: + * string one and the void/pending-cell one is UNCHANGED from before + * ADR-3889 Phase 2: * number return -> process.exitCode = it (unchanged) * string return -> NEW: process.exitCode = projectOutcome(result, getContractVersion()), * UNLESS that projection is the HOOK_DENY exit code (see * the refusal below — 2 may only be produced by terminateNow). + * void/undefined return -> NEW (#3912, ADR-3889 §4): an explicit return + * already handled above always wins, so this arm only + * runs when main() declared no outcome of its own. If + * the pending-outcome cell holds a value (currently only + * ever 'DEGRADED', set by io.cts's output() on a + * payload-carried error), project THAT through the + * current contract version — BUT ONLY when + * process.exitCode is not already a non-zero value. + * FULL PRECEDENCE ORDER for the code a void-returning + * main() ends up with: + * 1. An explicit number/string return from main() + * (handled in the arms above) — always wins. + * 2. A non-zero process.exitCode already set by main() + * itself before it returned (e.g. `state validate + * --strict`'s `emit()` setting 1 directly) — wins + * over the pending cell. + * 3. The pending-outcome cell's projection — used only + * when process.exitCode is still unset/0. + * 4. Otherwise process.exitCode stays 0 (default). + * This is a regression fix: unconditionally projecting + * the pending cell here used to CLOBBER an + * already-non-zero process.exitCode down to DEGRADED's + * v1 projection (0) — turning a real declared failure + * (e.g. `state validate --strict` against a missing + * STATE.md, which sets process.exitCode = 1 directly) + * into a false success. `runMain` must never LOWER an + * exit code that main() itself already raised. DEGRADED + * still projects to 0 under v1 when nothing else set a + * code — the same value this arm produced before this + * phase by doing nothing — so v1 behavior is + * byte-identical for every caller that never sets its + * own exit code. * thrown ExitError -> process.exitCode = err.code (+ stderr err.message if hasUserMessage && code!=0) (unchanged) * other throw -> when json-error mode is active, emits structured { ok:false, reason, message } * to stderr; otherwise writes raw stack trace. exit code = 1 in either case. (unchanged) @@ -236,30 +304,59 @@ function runMain(main) { Promise.resolve() .then(() => main()) .then((result) => { - if (typeof result === 'number') { - process.exitCode = result; - return; - } - if (typeof result === 'string') { - const projected = projectOutcome(result, getContractVersion()); - // ADR-3889 §3: exit code 2 (the hook-protocol deny) may - // ONLY be produced by terminateNow, never by runMain. runMain is - // drain-then-exit; a deny drained this way can be truncated on - // Windows, which is exactly why terminateNow (write-then-terminate) - // exists. Gated on the PROJECTED code, not on the literal string - // `'HOOK_DENY'`, so a future registry rename that still resolves to - // this code cannot slip past the guard. - if (projected === HOOK_DENY_CODE) { - process.stderr.write(`runMain: refusing to exit with code ${HOOK_DENY_CODE} — outcome ${JSON.stringify(result)} ` - + `projects to the ${HOOK_DENY_NAME} exit code, which is reserved to terminateNow. ` - + `A hook-protocol deny must be delivered write-then-terminate via terminateNow(${JSON.stringify(result)}, payload), ` - + 'never drain-then-exit via runMain — a drained deny can be truncated on Windows. ' - + 'This is a caller bug: runMain must not be given a main() that returns HOOK_DENY.\n'); - process.exitCode = exitCodeFor('INTERNAL'); + // Cleared on EVERY branch below, not only the pending-cell-consuming + // void arm: an explicit number/string return means main() declared + // its own outcome and the cell (if anything set it earlier in this + // same invocation) is now stale — leaving it set would leak into the + // NEXT runMain call in this process, reintroducing the #3912 leak one + // level up. "Cleared on consumption" therefore means "consumption of + // this runMain call", not just "consumption of the pending value". + try { + if (typeof result === 'number') { + process.exitCode = result; return; } - process.exitCode = projected; - return; + if (typeof result === 'string') { + const projected = projectOutcome(result, getContractVersion()); + // ADR-3889 §3: exit code 2 (the hook-protocol deny) may + // ONLY be produced by terminateNow, never by runMain. runMain is + // drain-then-exit; a deny drained this way can be truncated on + // Windows, which is exactly why terminateNow (write-then-terminate) + // exists. Gated on the PROJECTED code, not on the literal string + // `'HOOK_DENY'`, so a future registry rename that still resolves to + // this code cannot slip past the guard. + if (projected === HOOK_DENY_CODE) { + process.stderr.write(`runMain: refusing to exit with code ${HOOK_DENY_CODE} — outcome ${JSON.stringify(result)} ` + + `projects to the ${HOOK_DENY_NAME} exit code, which is reserved to terminateNow. ` + + `A hook-protocol deny must be delivered write-then-terminate via terminateNow(${JSON.stringify(result)}, payload), ` + + 'never drain-then-exit via runMain — a drained deny can be truncated on Windows. ' + + 'This is a caller bug: runMain must not be given a main() that returns HOOK_DENY.\n'); + process.exitCode = exitCodeFor('INTERNAL'); + return; + } + process.exitCode = projected; + return; + } + // result is undefined (void return): main declared no outcome itself. + // Fall back to the pending-outcome cell, if anything set it — but + // NEVER lower an exit code main() already raised on its own (see the + // precedence order in this function's doc comment above). Without + // this guard, a void-returning main() that set process.exitCode = 1 + // directly (e.g. `state validate --strict` on a missing STATE.md) + // would have that 1 clobbered down to DEGRADED's v1 projection (0) + // by a payload-carried error the SAME call also recorded via + // io.cts's output() — a real failure silently reported as success. + const pending = getPendingOutcome(); + if (typeof pending === 'string' && pending.length > 0 && !process.exitCode) { + process.exitCode = projectOutcome(pending, getContractVersion()); + } + } + finally { + // Cell is consumed exactly once per runMain call regardless of which + // branch above ran (see the cell's own doc comment — "last + // declaration wins, cleared on consumption") — so a later `runMain` + // in the same process never inherits this one's declaration. + setPendingOutcome(undefined); } }) .catch((err) => { @@ -458,4 +555,6 @@ module.exports = { resolveContractVersion, getContractVersion, terminateNow, + setPendingOutcome, + getPendingOutcome, }; diff --git a/scripts/lib/cli-exit.cjs b/scripts/lib/cli-exit.cjs index 81db88847..75850af30 100644 --- a/scripts/lib/cli-exit.cjs +++ b/scripts/lib/cli-exit.cjs @@ -148,6 +148,41 @@ function projectOutcome(outcome, version) { // exitCodeFor's own contract, which this function inherits verbatim). return exitCodeFor(outcome); } +/** + * Pending declared outcome (ADR-3889 §4, #3912): the outcome `output()` + * records when it detects a payload-carried error (`{ error }`, any key + * order) on a call that otherwise just returns — there is no thrown + * ExitError and no explicit `main()` return for `runMain` to project, so + * without this cell the declaration has nowhere to land. `runMain` reads it + * ONLY when `main()` itself returns no explicit code (void/undefined); an + * explicit number/string return always wins over whatever this cell holds. + * + * Held in a Symbol-keyed globalThis cell for the exact reason + * JSON_ERROR_MODE_KEY / CONTRACT_VERSION_KEY are (see their comments above): + * this module is emitted to three locations and thus loaded as independent + * module instances in any process that requires more than one, so a + * module-level variable would let those instances disagree about whether a + * degraded result was ever declared. + * + * LIFETIME (#3912 review fix): LAST DECLARATION WINS, CLEARED ON CONSUMPTION. + * This cell is NOT "was DEGRADED ever declared this process" — it is "is a + * degraded outcome pending RIGHT NOW". `output()` sets it to 'DEGRADED' on a + * payload-carried error and CLEARS it (`undefined`) on a clean payload, so a + * later clean `output()` call undoes an earlier degraded one in the same + * invocation. `runMain` clears it immediately after consuming it (in a + * `finally`, on both the pending-cell branch and the case where nothing was + * pending), so a second `runMain` in the same process starts clean. Without + * both halves the cell is monotonic for the life of the process: any later + * `main()` returning void would inherit a stale DEGRADED from an unrelated, + * earlier call — this is the leak #3912 review found and fixed. + */ +const PENDING_OUTCOME_KEY = Symbol.for('gsd.exit.pendingOutcome'); +function setPendingOutcome(v) { + globalThis[PENDING_OUTCOME_KEY] = v; +} +function getPendingOutcome() { + return globalThis[PENDING_OUTCOME_KEY]; +} const EXIT_CONTRACT_FLAG_PREFIX = '--exit-contract='; /** Scan argv for the FIRST `--exit-contract=` token; undefined if absent. */ function findExitContractFlag(argv) { @@ -221,11 +256,44 @@ class ExitError extends Error { * process.on('exit') cleanup still fires — this is precisely why runMain and * terminateNow are two different functions: drain-then-exit vs write-then- * terminate). main may be sync or async. Every arm below except the new - * string one is UNCHANGED from before ADR-3889 Phase 2: + * string one and the void/pending-cell one is UNCHANGED from before + * ADR-3889 Phase 2: * number return -> process.exitCode = it (unchanged) * string return -> NEW: process.exitCode = projectOutcome(result, getContractVersion()), * UNLESS that projection is the HOOK_DENY exit code (see * the refusal below — 2 may only be produced by terminateNow). + * void/undefined return -> NEW (#3912, ADR-3889 §4): an explicit return + * already handled above always wins, so this arm only + * runs when main() declared no outcome of its own. If + * the pending-outcome cell holds a value (currently only + * ever 'DEGRADED', set by io.cts's output() on a + * payload-carried error), project THAT through the + * current contract version — BUT ONLY when + * process.exitCode is not already a non-zero value. + * FULL PRECEDENCE ORDER for the code a void-returning + * main() ends up with: + * 1. An explicit number/string return from main() + * (handled in the arms above) — always wins. + * 2. A non-zero process.exitCode already set by main() + * itself before it returned (e.g. `state validate + * --strict`'s `emit()` setting 1 directly) — wins + * over the pending cell. + * 3. The pending-outcome cell's projection — used only + * when process.exitCode is still unset/0. + * 4. Otherwise process.exitCode stays 0 (default). + * This is a regression fix: unconditionally projecting + * the pending cell here used to CLOBBER an + * already-non-zero process.exitCode down to DEGRADED's + * v1 projection (0) — turning a real declared failure + * (e.g. `state validate --strict` against a missing + * STATE.md, which sets process.exitCode = 1 directly) + * into a false success. `runMain` must never LOWER an + * exit code that main() itself already raised. DEGRADED + * still projects to 0 under v1 when nothing else set a + * code — the same value this arm produced before this + * phase by doing nothing — so v1 behavior is + * byte-identical for every caller that never sets its + * own exit code. * thrown ExitError -> process.exitCode = err.code (+ stderr err.message if hasUserMessage && code!=0) (unchanged) * other throw -> when json-error mode is active, emits structured { ok:false, reason, message } * to stderr; otherwise writes raw stack trace. exit code = 1 in either case. (unchanged) @@ -234,30 +302,59 @@ function runMain(main) { Promise.resolve() .then(() => main()) .then((result) => { - if (typeof result === 'number') { - process.exitCode = result; - return; - } - if (typeof result === 'string') { - const projected = projectOutcome(result, getContractVersion()); - // ADR-3889 §3: exit code 2 (the hook-protocol deny) may - // ONLY be produced by terminateNow, never by runMain. runMain is - // drain-then-exit; a deny drained this way can be truncated on - // Windows, which is exactly why terminateNow (write-then-terminate) - // exists. Gated on the PROJECTED code, not on the literal string - // `'HOOK_DENY'`, so a future registry rename that still resolves to - // this code cannot slip past the guard. - if (projected === HOOK_DENY_CODE) { - process.stderr.write(`runMain: refusing to exit with code ${HOOK_DENY_CODE} — outcome ${JSON.stringify(result)} ` - + `projects to the ${HOOK_DENY_NAME} exit code, which is reserved to terminateNow. ` - + `A hook-protocol deny must be delivered write-then-terminate via terminateNow(${JSON.stringify(result)}, payload), ` - + 'never drain-then-exit via runMain — a drained deny can be truncated on Windows. ' - + 'This is a caller bug: runMain must not be given a main() that returns HOOK_DENY.\n'); - process.exitCode = exitCodeFor('INTERNAL'); + // Cleared on EVERY branch below, not only the pending-cell-consuming + // void arm: an explicit number/string return means main() declared + // its own outcome and the cell (if anything set it earlier in this + // same invocation) is now stale — leaving it set would leak into the + // NEXT runMain call in this process, reintroducing the #3912 leak one + // level up. "Cleared on consumption" therefore means "consumption of + // this runMain call", not just "consumption of the pending value". + try { + if (typeof result === 'number') { + process.exitCode = result; return; } - process.exitCode = projected; - return; + if (typeof result === 'string') { + const projected = projectOutcome(result, getContractVersion()); + // ADR-3889 §3: exit code 2 (the hook-protocol deny) may + // ONLY be produced by terminateNow, never by runMain. runMain is + // drain-then-exit; a deny drained this way can be truncated on + // Windows, which is exactly why terminateNow (write-then-terminate) + // exists. Gated on the PROJECTED code, not on the literal string + // `'HOOK_DENY'`, so a future registry rename that still resolves to + // this code cannot slip past the guard. + if (projected === HOOK_DENY_CODE) { + process.stderr.write(`runMain: refusing to exit with code ${HOOK_DENY_CODE} — outcome ${JSON.stringify(result)} ` + + `projects to the ${HOOK_DENY_NAME} exit code, which is reserved to terminateNow. ` + + `A hook-protocol deny must be delivered write-then-terminate via terminateNow(${JSON.stringify(result)}, payload), ` + + 'never drain-then-exit via runMain — a drained deny can be truncated on Windows. ' + + 'This is a caller bug: runMain must not be given a main() that returns HOOK_DENY.\n'); + process.exitCode = exitCodeFor('INTERNAL'); + return; + } + process.exitCode = projected; + return; + } + // result is undefined (void return): main declared no outcome itself. + // Fall back to the pending-outcome cell, if anything set it — but + // NEVER lower an exit code main() already raised on its own (see the + // precedence order in this function's doc comment above). Without + // this guard, a void-returning main() that set process.exitCode = 1 + // directly (e.g. `state validate --strict` on a missing STATE.md) + // would have that 1 clobbered down to DEGRADED's v1 projection (0) + // by a payload-carried error the SAME call also recorded via + // io.cts's output() — a real failure silently reported as success. + const pending = getPendingOutcome(); + if (typeof pending === 'string' && pending.length > 0 && !process.exitCode) { + process.exitCode = projectOutcome(pending, getContractVersion()); + } + } + finally { + // Cell is consumed exactly once per runMain call regardless of which + // branch above ran (see the cell's own doc comment — "last + // declaration wins, cleared on consumption") — so a later `runMain` + // in the same process never inherits this one's declaration. + setPendingOutcome(undefined); } }) .catch((err) => { @@ -456,4 +553,6 @@ module.exports = { resolveContractVersion, getContractVersion, terminateNow, + setPendingOutcome, + getPendingOutcome, }; diff --git a/src/cli-exit.cts b/src/cli-exit.cts index 4d925ec55..2f9c42bc2 100644 --- a/src/cli-exit.cts +++ b/src/cli-exit.cts @@ -141,6 +141,44 @@ function projectOutcome(outcome: unknown, version: unknown): number { return exitCodeFor(outcome); } +/** + * Pending declared outcome (ADR-3889 §4, #3912): the outcome `output()` + * records when it detects a payload-carried error (`{ error }`, any key + * order) on a call that otherwise just returns — there is no thrown + * ExitError and no explicit `main()` return for `runMain` to project, so + * without this cell the declaration has nowhere to land. `runMain` reads it + * ONLY when `main()` itself returns no explicit code (void/undefined); an + * explicit number/string return always wins over whatever this cell holds. + * + * Held in a Symbol-keyed globalThis cell for the exact reason + * JSON_ERROR_MODE_KEY / CONTRACT_VERSION_KEY are (see their comments above): + * this module is emitted to three locations and thus loaded as independent + * module instances in any process that requires more than one, so a + * module-level variable would let those instances disagree about whether a + * degraded result was ever declared. + * + * LIFETIME (#3912 review fix): LAST DECLARATION WINS, CLEARED ON CONSUMPTION. + * This cell is NOT "was DEGRADED ever declared this process" — it is "is a + * degraded outcome pending RIGHT NOW". `output()` sets it to 'DEGRADED' on a + * payload-carried error and CLEARS it (`undefined`) on a clean payload, so a + * later clean `output()` call undoes an earlier degraded one in the same + * invocation. `runMain` clears it immediately after consuming it (in a + * `finally`, on both the pending-cell branch and the case where nothing was + * pending), so a second `runMain` in the same process starts clean. Without + * both halves the cell is monotonic for the life of the process: any later + * `main()` returning void would inherit a stale DEGRADED from an unrelated, + * earlier call — this is the leak #3912 review found and fixed. + */ +const PENDING_OUTCOME_KEY = Symbol.for('gsd.exit.pendingOutcome'); + +function setPendingOutcome(v: unknown): void { + (globalThis as unknown as Record)[PENDING_OUTCOME_KEY] = v; +} + +function getPendingOutcome(): unknown { + return (globalThis as unknown as Record)[PENDING_OUTCOME_KEY]; +} + const EXIT_CONTRACT_FLAG_PREFIX = '--exit-contract='; /** Scan argv for the FIRST `--exit-contract=` token; undefined if absent. */ @@ -222,11 +260,44 @@ class ExitError extends Error { * process.on('exit') cleanup still fires — this is precisely why runMain and * terminateNow are two different functions: drain-then-exit vs write-then- * terminate). main may be sync or async. Every arm below except the new - * string one is UNCHANGED from before ADR-3889 Phase 2: + * string one and the void/pending-cell one is UNCHANGED from before + * ADR-3889 Phase 2: * number return -> process.exitCode = it (unchanged) * string return -> NEW: process.exitCode = projectOutcome(result, getContractVersion()), * UNLESS that projection is the HOOK_DENY exit code (see * the refusal below — 2 may only be produced by terminateNow). + * void/undefined return -> NEW (#3912, ADR-3889 §4): an explicit return + * already handled above always wins, so this arm only + * runs when main() declared no outcome of its own. If + * the pending-outcome cell holds a value (currently only + * ever 'DEGRADED', set by io.cts's output() on a + * payload-carried error), project THAT through the + * current contract version — BUT ONLY when + * process.exitCode is not already a non-zero value. + * FULL PRECEDENCE ORDER for the code a void-returning + * main() ends up with: + * 1. An explicit number/string return from main() + * (handled in the arms above) — always wins. + * 2. A non-zero process.exitCode already set by main() + * itself before it returned (e.g. `state validate + * --strict`'s `emit()` setting 1 directly) — wins + * over the pending cell. + * 3. The pending-outcome cell's projection — used only + * when process.exitCode is still unset/0. + * 4. Otherwise process.exitCode stays 0 (default). + * This is a regression fix: unconditionally projecting + * the pending cell here used to CLOBBER an + * already-non-zero process.exitCode down to DEGRADED's + * v1 projection (0) — turning a real declared failure + * (e.g. `state validate --strict` against a missing + * STATE.md, which sets process.exitCode = 1 directly) + * into a false success. `runMain` must never LOWER an + * exit code that main() itself already raised. DEGRADED + * still projects to 0 under v1 when nothing else set a + * code — the same value this arm produced before this + * phase by doing nothing — so v1 behavior is + * byte-identical for every caller that never sets its + * own exit code. * thrown ExitError -> process.exitCode = err.code (+ stderr err.message if hasUserMessage && code!=0) (unchanged) * other throw -> when json-error mode is active, emits structured { ok:false, reason, message } * to stderr; otherwise writes raw stack trace. exit code = 1 in either case. (unchanged) @@ -235,29 +306,57 @@ function runMain(main: () => number | string | void | Promise main()) .then((result) => { - if (typeof result === 'number') { process.exitCode = result; return; } - if (typeof result === 'string') { - const projected = projectOutcome(result, getContractVersion()); - // ADR-3889 §3: exit code 2 (the hook-protocol deny) may - // ONLY be produced by terminateNow, never by runMain. runMain is - // drain-then-exit; a deny drained this way can be truncated on - // Windows, which is exactly why terminateNow (write-then-terminate) - // exists. Gated on the PROJECTED code, not on the literal string - // `'HOOK_DENY'`, so a future registry rename that still resolves to - // this code cannot slip past the guard. - if (projected === HOOK_DENY_CODE) { - process.stderr.write( - `runMain: refusing to exit with code ${HOOK_DENY_CODE} — outcome ${JSON.stringify(result)} ` - + `projects to the ${HOOK_DENY_NAME} exit code, which is reserved to terminateNow. ` - + `A hook-protocol deny must be delivered write-then-terminate via terminateNow(${JSON.stringify(result)}, payload), ` - + 'never drain-then-exit via runMain — a drained deny can be truncated on Windows. ' - + 'This is a caller bug: runMain must not be given a main() that returns HOOK_DENY.\n', - ); - process.exitCode = exitCodeFor('INTERNAL'); + // Cleared on EVERY branch below, not only the pending-cell-consuming + // void arm: an explicit number/string return means main() declared + // its own outcome and the cell (if anything set it earlier in this + // same invocation) is now stale — leaving it set would leak into the + // NEXT runMain call in this process, reintroducing the #3912 leak one + // level up. "Cleared on consumption" therefore means "consumption of + // this runMain call", not just "consumption of the pending value". + try { + if (typeof result === 'number') { process.exitCode = result; return; } + if (typeof result === 'string') { + const projected = projectOutcome(result, getContractVersion()); + // ADR-3889 §3: exit code 2 (the hook-protocol deny) may + // ONLY be produced by terminateNow, never by runMain. runMain is + // drain-then-exit; a deny drained this way can be truncated on + // Windows, which is exactly why terminateNow (write-then-terminate) + // exists. Gated on the PROJECTED code, not on the literal string + // `'HOOK_DENY'`, so a future registry rename that still resolves to + // this code cannot slip past the guard. + if (projected === HOOK_DENY_CODE) { + process.stderr.write( + `runMain: refusing to exit with code ${HOOK_DENY_CODE} — outcome ${JSON.stringify(result)} ` + + `projects to the ${HOOK_DENY_NAME} exit code, which is reserved to terminateNow. ` + + `A hook-protocol deny must be delivered write-then-terminate via terminateNow(${JSON.stringify(result)}, payload), ` + + 'never drain-then-exit via runMain — a drained deny can be truncated on Windows. ' + + 'This is a caller bug: runMain must not be given a main() that returns HOOK_DENY.\n', + ); + process.exitCode = exitCodeFor('INTERNAL'); + return; + } + process.exitCode = projected; return; } - process.exitCode = projected; - return; + // result is undefined (void return): main declared no outcome itself. + // Fall back to the pending-outcome cell, if anything set it — but + // NEVER lower an exit code main() already raised on its own (see the + // precedence order in this function's doc comment above). Without + // this guard, a void-returning main() that set process.exitCode = 1 + // directly (e.g. `state validate --strict` on a missing STATE.md) + // would have that 1 clobbered down to DEGRADED's v1 projection (0) + // by a payload-carried error the SAME call also recorded via + // io.cts's output() — a real failure silently reported as success. + const pending = getPendingOutcome(); + if (typeof pending === 'string' && pending.length > 0 && !process.exitCode) { + process.exitCode = projectOutcome(pending, getContractVersion()); + } + } finally { + // Cell is consumed exactly once per runMain call regardless of which + // branch above ran (see the cell's own doc comment — "last + // declaration wins, cleared on consumption") — so a later `runMain` + // in the same process never inherits this one's declaration. + setPendingOutcome(undefined); } }) .catch((err: unknown) => { @@ -461,4 +560,6 @@ export = { resolveContractVersion, getContractVersion, terminateNow, + setPendingOutcome, + getPendingOutcome, }; diff --git a/src/io.cts b/src/io.cts index 3a2573440..c36059abe 100644 --- a/src/io.cts +++ b/src/io.cts @@ -14,7 +14,10 @@ import path from 'node:path'; import { platformWriteSync, platformEnsureDir } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import cliExitModule = require('./cli-exit.cjs'); -const { setJsonErrorMode, getJsonErrorMode, EXIT_ENVELOPE_REASON, ExitError } = cliExitModule; +const { + setJsonErrorMode, getJsonErrorMode, EXIT_ENVELOPE_REASON, ExitError, + setPendingOutcome, projectOutcome, getContractVersion, +} = cliExitModule; // ─── Temp-file helpers (needed by output()) ────────────────────────────────── @@ -144,7 +147,48 @@ function serializeForOutput(result: unknown): string { return JSON.stringify(result, null, 2); } +/** + * A payload-carried error, per ADR-2980's own definition (#3912, ADR-3889 + * §4): `result` is an object carrying a SERIALIZABLE `error` property, in + * ANY key order — `{ found: false, error }` counts exactly the same as + * `{ error, found: false }`. The discriminator is a serializable error + * value, NOT mere key presence: `result.error`'s own truthiness is + * irrelevant (falsy `0`/`null`/`''` all count), and neither is `result`'s + * prototype (a plain object literal is all any call site here ever + * passes) — but `error: undefined` does NOT count, because + * `JSON.stringify` (the exact serializer `serializeForOutput` uses to + * build the payload the caller actually receives) drops an object + * property whose value is `undefined` entirely. A payload built as + * `{ found: false, error: undefined }` therefore reaches the wire as + * `{"found":false}` — no error at all — and recording DEGRADED for it + * would be a false verdict: exit 80 under v2 for output the user sees as + * clean. `hasOwnProperty` alone is not enough to answer "does this payload + * declare an error"; it must also survive `JSON.stringify`. + */ +function isPayloadCarriedError(result: unknown): boolean { + return typeof result === 'object' && result !== null + && Object.prototype.hasOwnProperty.call(result, 'error') + && (result as { error?: unknown }).error !== undefined; +} + function output(result: unknown, raw: boolean, rawValue?: unknown): void { + // #3912 (ADR-3889 §4): a payload-carried error declares DEGRADED into the + // pending-outcome cell runMain reads. This is the ONLY new thing output() + // does — it still just writes fd 1 and returns; the exit code stays + // whatever it already was under v1 (DEGRADED projects to 0), and nothing + // here touches process.exitCode directly. + // + // LAST-WRITE-WINS (review fix): a clean (non-error-shaped) payload CLEARS + // the cell rather than leaving a prior degraded declaration in place. A + // handler that calls output() more than once per invocation — a + // diagnostic error payload followed by a clean final payload — must have + // its LATEST declaration win, not its first: the cell reflects "is a + // degraded outcome pending right now", not "was one ever declared". + if (isPayloadCarriedError(result)) { + setPendingOutcome('DEGRADED'); + } else { + setPendingOutcome(undefined); + } let data: string; if (raw && rawValue !== undefined) { // eslint-disable-next-line @typescript-eslint/no-base-to-string @@ -274,6 +318,73 @@ function formatDiagnosticToken(value: string): string { return JSON.stringify(value); } +/** + * Map an ERROR_REASON wire value onto a declared outcome name (#3912, + * ADR-3889 §4). Closed over the 25-member enum: every reason gets an + * explicit entry below, so a 26th member added without a mapping falls + * through to the `?? 'FAIL'` default rather than silently mis-projecting — + * and tests/A1 iterates `Object.values(ERROR_REASON)`, so that default is + * exactly what makes an unmapped addition visible instead of invisible. + * + * This function's result is ONLY consulted under v2 (see `error()` below) — + * it is deliberately never routed through `projectOutcome` under v1, which + * is what keeps the v1 pin intact (`projectOutcome` treats registered names + * as version-invariant, so e.g. USAGE would otherwise become 64 today). + * + * Each non-FAIL choice below is justified inline; `UNKNOWN` and anything + * with no clearly better fit stays `FAIL` — the honest default the design + * calls for, not a guess dressed up as a specific outcome. + */ +const REASON_TO_OUTCOME: Readonly> = Object.freeze({ + // Bad argv/subcommand/argument — the caller, not the run, is at fault. + [ERROR_REASON.CONFIG_INVALID_KEY]: 'USAGE', + [ERROR_REASON.SDK_UNKNOWN_COMMAND]: 'USAGE', + [ERROR_REASON.SDK_MISSING_ARG]: 'USAGE', + [ERROR_REASON.GRAPHIFY_INVALID_QUERY]: 'USAGE', + [ERROR_REASON.USAGE]: 'USAGE', + + // A specific, named thing does not exist / nothing was there to find — + // genuine, known emptiness rather than a broken prerequisite. + [ERROR_REASON.CONFIG_KEY_NOT_FOUND]: 'NO_INPUT', + [ERROR_REASON.SUMMARY_NO_PLANNING]: 'NO_INPUT', + [ERROR_REASON.WORKSTREAM_MODE_NONE_ACTIVE]: 'NO_INPUT', + + // A prerequisite is absent, unreadable, or otherwise not in a state the + // run could proceed from — distinct from NO_INPUT's genuine emptiness. + [ERROR_REASON.CONFIG_NO_FILE]: 'UNAVAILABLE', + [ERROR_REASON.CONFIG_PARSE_FAILED]: 'UNAVAILABLE', + [ERROR_REASON.PHASE_NOT_FOUND]: 'UNAVAILABLE', + [ERROR_REASON.PHASE_VERIFICATION_INCOMPLETE]: 'UNAVAILABLE', + [ERROR_REASON.PHASE_PLAN_COVERAGE_INCOMPLETE]: 'UNAVAILABLE', + // Its own docstring: "a marker exists but didn't resolve" — a broken + // prerequisite, not the "no marker anywhere" emptiness NONE_ACTIVE covers. + [ERROR_REASON.WORKSTREAM_MODE_MARKER_UNRESOLVED]: 'UNAVAILABLE', + [ERROR_REASON.GRAPHIFY_NO_GRAPH]: 'UNAVAILABLE', + // Its own docstring: "a NON-answer, distinct from a project that + // genuinely has zero completed phases yet" — UNAVAILABLE, not NO_INPUT. + [ERROR_REASON.ESTIMATE_PHASES_UNREADABLE]: 'UNAVAILABLE', + [ERROR_REASON.COMMIT_DOCS_GUARD_NOT_A_REPO]: 'UNAVAILABLE', + [ERROR_REASON.COMMIT_DOCS_GUARD_FOREIGN_HOOK]: 'UNAVAILABLE', + [ERROR_REASON.COMMIT_DOCS_GUARD_HOOKS_PATH_SET]: 'UNAVAILABLE', + // Its own docstring: "an absent field or non-JSON command output is a + // failure, never a demotion to an empty answer" — the field/output was + // supposed to be there and was not; a prerequisite of the query failed. + [ERROR_REASON.PICK_FIELD_ABSENT]: 'UNAVAILABLE', + [ERROR_REASON.PICK_OUTPUT_NOT_JSON]: 'UNAVAILABLE', + + // Self-failure: the run itself broke, not its inputs. + [ERROR_REASON.SDK_FAIL_FAST]: 'INTERNAL', + [ERROR_REASON.SECURITY_SCAN_FAILED]: 'INTERNAL', + + // No clearly better fit — the honest default, per design. + [ERROR_REASON.HOOKS_OPT_OUT]: 'FAIL', + [ERROR_REASON.UNKNOWN]: 'FAIL', +}); + +function outcomeForReason(reason: ErrorReasonValue): string { + return REASON_TO_OUTCOME[reason] ?? 'FAIL'; +} + function error(message: string, reason: ErrorReasonValue = ERROR_REASON.UNKNOWN, extra?: Record): never { if (getJsonErrorMode()) { const payload = JSON.stringify({ ok: false, reason, message, ...(extra || {}) }) + '\n'; @@ -281,6 +392,15 @@ function error(message: string, reason: ErrorReasonValue = ERROR_REASON.UNKNOWN, } else { writeAllSync(2, 'Error: ' + message + '\n'); } + // #3912 (ADR-3889 §4): the declaration is version-gated HERE, not inside + // projectOutcome — registered names are version-invariant there, so + // routing every reason through it unconditionally would change v1 exit + // codes today (e.g. USAGE -> 64) and break the pin. Under v1 the exit + // stays ExitError(1) unconditionally, byte-identical to every prior + // release; only v2 projects the declared outcome through the registry. + if (getContractVersion() === 'v2') { + throw new ExitError(projectOutcome(outcomeForReason(reason), 'v2')); + } // No message passed to ExitError: the stderr write above is already done, // byte-identical to the prior process.exit(1) behavior, and ExitError with // no message means runMain's catch adds nothing further to stderr. diff --git a/tests/cli-exit.test.cjs b/tests/cli-exit.test.cjs index 8c7c8463f..9cea026a3 100644 --- a/tests/cli-exit.test.cjs +++ b/tests/cli-exit.test.cjs @@ -1120,6 +1120,123 @@ describe('#3906: ambient GSD_EXIT_CONTRACT/--exit-contract wiring (acceptance cr }); }); +// ─── #3912 regression: `--exit-contract=` in LEADING argv position ────── +// +// resolveContractVersion() scans argv non-destructively (findExitContractFlag, +// gsd-core/bin/lib/cli-exit.cjs). Nothing previously spliced the flag out of +// the `gsd-tools` CLI dispatcher's own argv before it fell through to command +// dispatch — the `--json-errors` block did this splice for itself, but +// `--exit-contract=` never got the same treatment. The dispatcher treats +// argv[0] as the command name, so: +// `gsd-tools --exit-contract=v2 state validate --strict` (leading) died +// with "Unknown command: --exit-contract=v2" (exit 64) — the flag was never +// consumed and squatted on the command-name slot. +// `gsd-tools state-snapshot --exit-contract=v2` (trailing) worked, because +// the flag landed after the command name and never collided with dispatch. +describe('#3912: gsd-tools dispatcher splices --exit-contract= regardless of argv position', () => { + const GSD_TOOLS_BIN = path.resolve(__dirname, '../gsd-core/bin/gsd-tools.cjs'); + + function run(args, options = {}) { + return toLegacyResult(runNode([GSD_TOOLS_BIN, ...args], { timeoutMs: PROBE_TIMEOUT_MS, ...options })); + } + + // Fixture: a temp dir with a `.planning/` directory but no `STATE.md`, so + // `state-snapshot` takes the "STATE.md not found" branch — which the + // outcome cell resolves to exit 80 under v2 and exit 0 under v1. Pinning + // these exact numbers (rather than "not 64" / "leading == trailing") + // catches the case where both positions dispatch but land on the SAME + // wrong contract. + let fixtureDir; + afterEach(() => { + if (fixtureDir) { + cleanup(fixtureDir); + fixtureDir = undefined; + } + }); + function makeFixture() { + fixtureDir = createTempDir(); + fs.mkdirSync(path.join(fixtureDir, '.planning'), { recursive: true }); + return fixtureDir; + } + + test('leading position dispatches (no "Unknown command", not exit 64)', () => { + const r = run(['--exit-contract=v2', 'state', 'validate', '--strict']); + assert.notEqual(r.status, 64, `must not fall into the unknown-command path; stderr: ${r.stderr}`); + assert.ok( + !/Unknown command/.test(r.stderr), + `leading --exit-contract=v2 must not be treated as the command name; stderr: ${r.stderr}`, + ); + }); + + test('trailing position still dispatches (no regression)', () => { + const r = run(['state-snapshot', '--exit-contract=v2']); + assert.notEqual(r.status, 64, `must not regress into the unknown-command path; stderr: ${r.stderr}`); + assert.ok( + !/Unknown command/.test(r.stderr), + `trailing --exit-contract=v2 must keep dispatching; stderr: ${r.stderr}`, + ); + }); + + test('leading and trailing position agree on the SAME pinned exit code, per contract version', () => { + const dir = makeFixture(); + const leadingV2 = run(['--exit-contract=v2', 'state-snapshot', `--cwd=${dir}`]); + const trailingV2 = run(['state-snapshot', `--cwd=${dir}`, '--exit-contract=v2']); + const leadingV1 = run(['--exit-contract=v1', 'state-snapshot', `--cwd=${dir}`]); + const trailingV1 = run(['state-snapshot', `--cwd=${dir}`, '--exit-contract=v1']); + assert.equal(leadingV2.status, 80, `leading v2 must exit 80; stderr: ${leadingV2.stderr}`); + assert.equal(trailingV2.status, 80, `trailing v2 must exit 80; stderr: ${trailingV2.stderr}`); + assert.equal(leadingV1.status, 0, `leading v1 must exit 0; stderr: ${leadingV1.stderr}`); + assert.equal(trailingV1.status, 0, `trailing v1 must exit 0; stderr: ${trailingV1.stderr}`); + }); + + test('an invalid leading value (v3) fails loudly rather than silently defaulting to v1', () => { + const r = run(['--exit-contract=v3', 'state-snapshot']); + assert.notEqual(r.status, 80, 'an invalid contract version must not silently resolve to a valid v2 exit code'); + assert.ok( + /unrecognized exit-contract version/.test(r.stderr), + `expected the resolveContractVersion rejection message on stderr; got: ${r.stderr.slice(0, 300)}`, + ); + // Distinguishes this from the PRE-FIX build, where the leading flag was + // never spliced: pre-fix stderr contains BOTH "Unknown command: + // --exit-contract=v3" (from the dispatcher rejecting the leading token) + // AND the resolve error (raised lazily via error() -> getContractVersion + // later in the same run). Post-fix, the flag is spliced before dispatch, + // so only the resolve error appears. + assert.ok( + !/Unknown command/.test(r.stderr), + `leading --exit-contract=v3 must be spliced before dispatch, not treated as the command name; stderr: ${r.stderr}`, + ); + }); + + test('multiple --exit-contract= tokens: first match wins, no leftover token reaches the dispatcher', () => { + const dir = makeFixture(); + const r = run(['--exit-contract=v2', '--exit-contract=v1', 'state-snapshot', `--cwd=${dir}`]); + assert.equal(r.status, 80, `first-match (v2) must win; stderr: ${r.stderr}`); + assert.ok( + !/Unknown command/.test(r.stderr), + `every --exit-contract= token must be spliced, not just the first; stderr: ${r.stderr}`, + ); + }); + + test('leading --exit-contract=v2 does not break run-with-timeout dispatch (#3912 P1 regression)', () => { + const r = run(['--exit-contract=v2', 'run-with-timeout', '5', '--', 'node', '-e', 'process.exit(0)']); + assert.equal(r.status, 0, `child must run and exit 0, not die with Unknown command; stderr: ${r.stderr}`); + assert.ok( + !/Unknown command/.test(r.stderr), + `leading --exit-contract=v2 must not intercept run-with-timeout's own dispatch; stderr: ${r.stderr}`, + ); + }); + + test('leading --json-errors does not break run-with-timeout dispatch (#3912 P1 regression)', () => { + const r = run(['--json-errors', 'run-with-timeout', '5', '--', 'node', '-e', 'process.exit(0)']); + assert.equal(r.status, 0, `child must run and exit 0, not die with Unknown command; stderr: ${r.stderr}`); + assert.ok( + !/Unknown command/.test(r.stderr), + `leading --json-errors must not intercept run-with-timeout's own dispatch; stderr: ${r.stderr}`, + ); + }); +}); + describe('#3906: terminateNow', () => { function spawnTerminateNow(lines) { return toLegacyResult(runNode(['-e', lines.join('\n')], { timeoutMs: PROBE_TIMEOUT_MS })); @@ -1761,3 +1878,175 @@ describe('#3911: the --check guards for the new hooks/lib artifacts can actually assert.equal(restored.status, 0, `--check must pass again once the artifact is restored; stderr: ${restored.stderr}`); }); }); + +// ─── #3912 (ADR-3889 §4, P8): the pending-outcome cell ────────────────────── +// +// output()'s payload-carried-error detection lives in io.cts and is tested +// there; these tests exercise the cell + runMain projection mechanics that +// live in cli-exit.cts itself: precedence (test matrix row C5) and cross-copy +// parity (row C6). + +describe('#3912: pending-outcome cell (runMain precedence + parity)', () => { + // E1: for every registered outcome name and version, projectOutcome is a + // non-negative integer, and is 0 ONLY for PASS, or for DEGRADED under v1. + // The existing "every projection... is a non-negative integer" test above + // does not pin the ZERO-ONLY-FOR half of this; this test is what makes an + // accidental future zero for some OTHER outcome fail loudly. + test('E1 (fast-check): 0 is produced only by PASS, or by DEGRADED under v1', () => { + fc.assert( + fc.property( + fc.constantFrom('PASS', 'FAIL', ...REGISTERED_NAMES), + fc.constantFrom(...VERSIONS), + (outcome, version) => { + const result = projectOutcome(outcome, version); + assert.equal(Number.isInteger(result), true); + assert.ok(result >= 0); + if (result === 0) { + const isPass = outcome === 'PASS'; + const isDegradedV1 = outcome === 'DEGRADED' && version === 'v1'; + assert.ok( + isPass || isDegradedV1, + `outcome=${outcome} version=${version} projected to 0 but is neither PASS nor DEGRADED/v1`, + ); + } else { + assert.notEqual(outcome, 'PASS', 'PASS must always project to 0'); + } + }, + ), + { seed: 3912, numRuns: 300 }, + ); + }); + + // C5: an explicit main() return beats a recorded DEGRADED cell. Without + // this, a caller that both returns an explicit code AND had a stale + // DEGRADED left in the cell (e.g. from an earlier output({error}) call in + // the same process) would get the CELL's projection instead of its own + // explicit decision — exactly backwards from "the cell is a fallback for + // the absence of a decision". + test('C5: an explicit main() return wins over a pending DEGRADED cell', () => { + const script = [ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.setPendingOutcome('DEGRADED');`, + `c.runMain(() => 'PASS');`, + `setImmediate(() => {});`, + ].join('\n'); + // Under v2, DEGRADED alone would project to 80; PASS must win with 0. + const r = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, + env: { ...process.env, GSD_EXIT_CONTRACT: 'v2' }, + })); + assert.equal(r.status, 0, `explicit PASS return must win over the pending DEGRADED cell; stderr: ${r.stderr}`); + }); + + test('C5 (numeric arm): an explicit numeric return also wins over a pending DEGRADED cell', () => { + const script = [ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.setPendingOutcome('DEGRADED');`, + `c.runMain(() => 42);`, + `setImmediate(() => {});`, + ].join('\n'); + const r = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, + env: { ...process.env, GSD_EXIT_CONTRACT: 'v2' }, + })); + assert.equal(r.status, 42, `explicit numeric return must win over the pending DEGRADED cell; stderr: ${r.stderr}`); + }); + + test('a void return with NO pending outcome set leaves exit code untouched (v1 unchanged)', () => { + const script = [ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.runMain(() => undefined);`, + `setImmediate(() => {});`, + ].join('\n'); + const r = toLegacyResult(runNode(['-e', script], { timeoutMs: PROBE_TIMEOUT_MS })); + assert.equal(r.status, 0, `stderr: ${r.stderr}`); + }); + + test('a void return WITH a pending DEGRADED cell projects it under v1 (0) and v2 (80)', () => { + const script = [ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.setPendingOutcome('DEGRADED');`, + `c.runMain(() => undefined);`, + `setImmediate(() => {});`, + ].join('\n'); + const v1 = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, env: { ...process.env, GSD_EXIT_CONTRACT: 'v1' }, + })); + assert.equal(v1.status, 0, `stderr: ${v1.stderr}`); + const v2 = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, env: { ...process.env, GSD_EXIT_CONTRACT: 'v2' }, + })); + assert.equal(v2.status, 80, `stderr: ${v2.stderr}`); + }); + + // Regression (fail-open, found live in `state validate --strict` on a + // missing STATE.md): a void-returning main() that ALREADY set + // process.exitCode to a non-zero value itself (e.g. `emit()` setting 1 + // directly on the STATE.md-not-found early return) must have that value + // survive a pending DEGRADED cell — the cell must never LOWER an exit + // code main() itself already raised. Before the fix, this arm + // unconditionally overwrote process.exitCode with the cell's projection, + // clobbering an already-set 1 down to DEGRADED's v1 projection (0) — a + // real declared failure silently turned into success. + test('a void return with an ALREADY-SET non-zero exitCode beats a pending DEGRADED cell (regression)', () => { + const script = [ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.setPendingOutcome('DEGRADED');`, + `c.runMain(() => { process.exitCode = 1; });`, + `setImmediate(() => {});`, + ].join('\n'); + const v1 = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, env: { ...process.env, GSD_EXIT_CONTRACT: 'v1' }, + })); + assert.equal(v1.status, 1, `an already-set non-zero exitCode must not be clobbered down to DEGRADED's v1 projection (0); stderr: ${v1.stderr}`); + const v2 = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, env: { ...process.env, GSD_EXIT_CONTRACT: 'v2' }, + })); + assert.equal(v2.status, 1, `an already-set non-zero exitCode must not be clobbered by DEGRADED's v2 projection (80) either; stderr: ${v2.stderr}`); + }); + + // C6: extends the existing three-copy parity coverage (json-error-mode cell, + // tested above at "both copies of the exit module share one json-error-mode + // cell") to the pending-outcome cell, across all THREE emitted copies — + // built (gsd-core/bin/lib), scripts/lib, and hooks/lib — not just two. + test('C6: all three emitted copies of cli-exit share ONE pending-outcome cell', () => { + const r = toLegacyResult(runNode(['-e', [ + `const built = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `const scripts = require(${JSON.stringify(SCRIPTS_CLI_EXIT_PATH)});`, + `const hooks = require(${JSON.stringify(HOOKS_CLI_EXIT_PATH)});`, + `if (built === scripts || built === hooks || scripts === hooks) throw new Error('expected three distinct module instances');`, + `built.setPendingOutcome('DEGRADED');`, + `process.stdout.write(JSON.stringify({`, + ` viaBuilt: built.getPendingOutcome(),`, + ` viaScripts: scripts.getPendingOutcome(),`, + ` viaHooks: hooks.getPendingOutcome(),`, + `}));`, + ].join('\n')], { timeoutMs: PROBE_TIMEOUT_MS })); + assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); + assert.deepStrictEqual( + JSON.parse(r.stdout), + { viaBuilt: 'DEGRADED', viaScripts: 'DEGRADED', viaHooks: 'DEGRADED' }, + 'all three copies must read one shared cell — three independent module-level flags would diverge here', + ); + }); + + test('C6: all three copies also agree on the PROJECTED exit code via runMain', () => { + for (const version of VERSIONS) { + for (const modulePath of [BUILT_CLI_EXIT_PATH, SCRIPTS_CLI_EXIT_PATH, HOOKS_CLI_EXIT_PATH]) { + const r = toLegacyResult(runNode(['-e', [ + `const c = require(${JSON.stringify(modulePath)});`, + `c.setPendingOutcome('DEGRADED');`, + `c.runMain(() => undefined);`, + `setImmediate(() => {});`, + ].join('\n')], { + timeoutMs: PROBE_TIMEOUT_MS, env: { ...process.env, GSD_EXIT_CONTRACT: version }, + })); + const expected = version === 'v1' ? 0 : 80; + assert.equal( + r.status, expected, + `${modulePath} under ${version} expected ${expected}, got ${r.status}; stderr: ${r.stderr}`, + ); + } + } + }); +}); diff --git a/tests/io.test.cjs b/tests/io.test.cjs index ea7988814..8da2a2889 100644 --- a/tests/io.test.cjs +++ b/tests/io.test.cjs @@ -18,10 +18,15 @@ const os = require('node:os'); const fs = require('node:fs'); const io = require('../gsd-core/bin/lib/io.cjs'); -const { ExitError } = require('../gsd-core/bin/lib/cli-exit.cjs'); +const { + ExitError, resolveContractVersion, setPendingOutcome, getPendingOutcome, runMain, +} = require('../gsd-core/bin/lib/cli-exit.cjs'); +const { EXIT_CODES } = require('../gsd-core/bin/lib/exit-code-registry.cjs'); const { runNode } = require('./helpers/process-seam.cjs'); const { toLegacyResult } = require('./helpers/git-fixture.cjs'); const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const fc = require('./helpers/fast-check-setup.cjs'); +const ts = require('typescript'); function runScript(script) { return toLegacyResult(runNode(['-e', script], { timeoutMs: PROBE_TIMEOUT_MS })); @@ -563,3 +568,525 @@ describe('bug #1891: @file: resolution in gsd-tools.cjs', () => { }); }); } + +// ═══════════════════════════════════════════════════════════════════════════ +// #3912 (ADR-3889 §4, epic #3889 Phase 8) — gsd-tools declares outcomes, +// pinned at v1. See .gsd/phase/enhance-3912-gsd-tools-outcomes/40-design.md +// and 50-test-matrix.md. +// ═══════════════════════════════════════════════════════════════════════════ + +const CLI_EXIT_PATH_3912 = path.resolve(__dirname, '../gsd-core/bin/lib/cli-exit.cjs'); +const IO_PATH_3912 = path.resolve(__dirname, '../gsd-core/bin/lib/io.cjs'); +const REGISTERED_NAMES_3912 = EXIT_CODES.map((e) => e.name); +const CODE_FOR_3912 = new Map(EXIT_CODES.map((e) => [e.name, e.code])); +const VERSIONS_3912 = ['v1', 'v2']; + +/** + * Expected reason -> outcome mapping, mirroring src/io.cts's own + * REASON_TO_OUTCOME table. Kept as an independent, explicit table here + * (rather than importing the internal function) so the test is a real + * behavioral check against error()'s observable exit code, not a tautology + * that re-imports the thing it is meant to verify. + */ +const EXPECTED_REASON_OUTCOME_3912 = { + config_key_not_found: 'NO_INPUT', + config_no_file: 'UNAVAILABLE', + config_parse_failed: 'UNAVAILABLE', + config_invalid_key: 'USAGE', + sdk_fail_fast: 'INTERNAL', + sdk_unknown_command: 'USAGE', + sdk_missing_arg: 'USAGE', + phase_not_found: 'UNAVAILABLE', + phase_verification_incomplete: 'UNAVAILABLE', + phase_plan_coverage_incomplete: 'UNAVAILABLE', + summary_no_planning: 'NO_INPUT', + workstream_mode_none_active: 'NO_INPUT', + workstream_mode_marker_unresolved: 'UNAVAILABLE', + graphify_no_graph: 'UNAVAILABLE', + graphify_invalid_query: 'USAGE', + estimate_phases_unreadable: 'UNAVAILABLE', + hooks_opt_out: 'FAIL', + commit_docs_guard_not_a_repo: 'UNAVAILABLE', + commit_docs_guard_foreign_hook: 'UNAVAILABLE', + commit_docs_guard_hooks_path_set: 'UNAVAILABLE', + security_scan_failed: 'INTERNAL', + pick_field_absent: 'UNAVAILABLE', + pick_output_not_json: 'UNAVAILABLE', + usage: 'USAGE', + unknown: 'FAIL', +}; + +/** Expected exit code for `error(msg, reason)` under a given contract version. */ +function expectedErrorCode3912(reasonValue, version) { + if (version === 'v1') return 1; + const outcome = EXPECTED_REASON_OUTCOME_3912[reasonValue]; + if (outcome === 'FAIL') return 1; + return CODE_FOR_3912.get(outcome); +} + +describe('#3912 A1/B1: error() declares from ERROR_REASON, exhaustive over the 25-member enum', () => { + afterEach(() => { + resolveContractVersion({ argv: ['node', 'x'], env: {} }); // restore v1 default + }); + + // A1 — the acceptance criterion: EVERY member of ERROR_REASON, iterated + // from the enum itself (not a hand-picked subset), exits 1 under v1. A + // 26th member added to the enum without a table entry still exits 1 + // under v1 (v1 never consults the table at all); under v2, the table + // lookup for that member yields `undefined`, `CODE_FOR_3912.get(undefined)` + // yields `undefined`, and `err.code === expected` fails against the real + // code (1) for that reason. With the set-equality assertion below in + // place, that drift is caught first, with a message naming the specific + // missing/extra reason instead of a confusing "must exit undefined". + assert.deepEqual( + Object.keys(EXPECTED_REASON_OUTCOME_3912).sort(), + Object.values(io.ERROR_REASON).slice().sort(), + 'this table must cover exactly the ERROR_REASON enum values, no more, no less', + ); + for (const [key, reasonValue] of Object.entries(io.ERROR_REASON)) { + test(`v1: ERROR_REASON.${key} (${reasonValue}) exits 1`, () => { + resolveContractVersion({ argv: ['node', 'x'], env: {} }); // v1 + assert.throws( + () => io.error('msg', reasonValue), + (err) => err instanceof ExitError && err.code === 1, + `ERROR_REASON.${key} must exit 1 under v1`, + ); + }); + + test(`v2: ERROR_REASON.${key} (${reasonValue}) projects to its mapped outcome's registered code`, () => { + resolveContractVersion({ argv: ['node', 'x', '--exit-contract=v2'], env: {} }); + const expected = expectedErrorCode3912(reasonValue, 'v2'); + assert.throws( + () => io.error('msg', reasonValue), + (err) => err instanceof ExitError && err.code === expected, + `ERROR_REASON.${key} under v2 must exit ${expected}`, + ); + }); + } + + // A2 — the 226-site default: no reason argument at all -> UNKNOWN -> exit 1. + test('A2: error() with no reason argument exits 1 under v1 (defaults to UNKNOWN)', () => { + resolveContractVersion({ argv: ['node', 'x'], env: {} }); + assert.throws( + () => io.error('no reason given'), + (err) => err instanceof ExitError && err.code === 1, + ); + }); + + test('A2: error() with no reason argument stays FAIL (exit 1) under v2 too — UNKNOWN is not a specific outcome', () => { + resolveContractVersion({ argv: ['node', 'x', '--exit-contract=v2'], env: {} }); + assert.throws( + () => io.error('no reason given'), + (err) => err instanceof ExitError && err.code === 1, + ); + }); + + // B2 — spot-check the specific mappings the design calls out by name. + test('B2: SDK_MISSING_ARG / SDK_UNKNOWN_COMMAND / USAGE all reach USAGE (64) under v2', () => { + resolveContractVersion({ argv: ['node', 'x', '--exit-contract=v2'], env: {} }); + for (const key of ['SDK_MISSING_ARG', 'SDK_UNKNOWN_COMMAND', 'USAGE']) { + assert.throws( + () => io.error('msg', io.ERROR_REASON[key]), + (err) => err instanceof ExitError && err.code === 64, + `${key} must project to 64 under v2`, + ); + } + }); + + // B5 — the anti-vacuity test. Without this, a mapping where every reason + // projects to 1 under both versions would satisfy every row above. + test('B5 (anti-vacuity): v1 and v2 differ for at least one reason', () => { + resolveContractVersion({ argv: ['node', 'x'], env: {} }); + let v1Code; + try { io.error('msg', io.ERROR_REASON.SDK_MISSING_ARG); } catch (e) { v1Code = e.code; } + resolveContractVersion({ argv: ['node', 'x', '--exit-contract=v2'], env: {} }); + let v2Code; + try { io.error('msg', io.ERROR_REASON.SDK_MISSING_ARG); } catch (e) { v2Code = e.code; } + assert.equal(v1Code, 1); + assert.equal(v2Code, 64); + assert.notEqual(v1Code, v2Code, 'v1 and v2 must differ for at least one reason, or the declaration is decorative'); + }); +}); + +describe('#3912 A3-A5: output({error}) records DEGRADED — shape-exhaustive plus a real census', () => { + // A3/A4 — shape exhaustive: both key orders, and varying the error value's + // own type/truthiness (irrelevant to detection — presence of the key is + // what counts). + // The discriminator is a SERIALIZABLE error value, not mere key presence: + // every row here embeds its payload via `JSON.stringify(payload)` to build + // the child script's literal, and `JSON.stringify` drops any key whose + // value is `undefined` — so an `{ error: undefined }` row would silently + // arrive at `output()` with NO `error` key at all, making the row pass for + // the wrong reason (or, as originally written, fail outright: see the + // dedicated `{error: undefined}` case below, which constructs the object + // as source text instead so the key survives). + const shapes = [ + ['error first', { error: 'boom', found: false }], + ['error last (A4)', { found: false, error: 'boom' }], + ['error in the middle', { a: 1, error: 'boom', b: 2 }], + ['error value is falsy (0)', { found: false, error: 0 }], + ['error value is null', { found: false, error: null }], + ['error value is an object', { error: { code: 'X' }, found: false }], + ]; + for (const [label, payload] of shapes) { + test(`A3/A4 (${label}): exits 0 under v1 and is recorded as DEGRADED`, () => { + const script = ` + const io = require(${JSON.stringify(IO_PATH_3912)}); + const c = require(${JSON.stringify(CLI_EXIT_PATH_3912)}); + io.output(${JSON.stringify(payload)}, false); + process.stdout.write('|PENDING=' + c.getPendingOutcome()); + `; + const result = toLegacyResult(runNode(['-e', script], { timeoutMs: PROBE_TIMEOUT_MS })); + assert.strictEqual(result.status, 0, `stderr: ${result.stderr}`); + assert.ok(result.stdout.includes('|PENDING=DEGRADED'), `expected DEGRADED recorded; got: ${result.stdout}`); + }); + } + + // A3/A4 pin — `{ error: undefined }` is NOT degraded. The object literal is + // written as SOURCE TEXT here (not round-tripped through + // `JSON.stringify(payload)`), so the `error` key genuinely reaches + // `output()` with an `undefined` value. `JSON.stringify` (the serializer + // `output()` itself uses to build the payload the user actually receives) + // drops a key whose value is `undefined`, so the wire payload is + // `{"found":false}` — no error at all. Recording DEGRADED here would be a + // false verdict: exit 80 under v2 for output the user sees as clean. The + // discriminator is a SERIALIZABLE error value, not key presence. + test('A3/A4 pin: {error: undefined} is NOT recorded as DEGRADED (JSON.stringify drops it)', () => { + const script = ` + const io = require(${JSON.stringify(IO_PATH_3912)}); + const c = require(${JSON.stringify(CLI_EXIT_PATH_3912)}); + io.output({ found: false, error: undefined }, false); + process.stdout.write('|PENDING=' + c.getPendingOutcome()); + `; + const result = toLegacyResult(runNode(['-e', script], { timeoutMs: PROBE_TIMEOUT_MS })); + assert.strictEqual(result.status, 0, `stderr: ${result.stderr}`); + assert.ok( + !result.stdout.includes('|PENDING=DEGRADED'), + `{error: undefined} carries no serializable error and must not be degraded; got: ${result.stdout}`, + ); + }); + + // A5 — negative space: no `error` key at all must NOT be recorded as degraded. + test('A5: output() with no error key exits 0 and records NOTHING', () => { + const script = ` + const io = require(${JSON.stringify(IO_PATH_3912)}); + const c = require(${JSON.stringify(CLI_EXIT_PATH_3912)}); + io.output({ ok: true, value: 1 }, false); + process.stdout.write('|PENDING=' + c.getPendingOutcome()); + `; + const result = toLegacyResult(runNode(['-e', script], { timeoutMs: PROBE_TIMEOUT_MS })); + assert.strictEqual(result.status, 0, `stderr: ${result.stderr}`); + assert.ok(!result.stdout.includes('|PENDING=DEGRADED'), `must not record DEGRADED; got: ${result.stdout}`); + }); + + // A3 census — AST-based, not a text grep (local/no-source-grep bans + // readFileSync().includes()/.match()/etc on source; this parses an AST + // instead and never calls a string-search method on the source text). + // Counts every call to an identifier literally named `output` (the name + // every call site in this tree destructures it to — `const { output } = + // ioMod`) whose first argument is an object literal carrying an `error` + // property. This is the SHAPE the design measured, over the real tree, + // not a hand-picked subset — and it independently reproduces the design + // doc's per-file breakdown (frontmatter 7, phase 4, roadmap 3, state 25, + // verify 8, workstream 7, commands 5, template 3, gsd2-import 2 = 64), + // which is itself the corrected count over ADR-2980's stale 60. + test('A3 census: exactly 64 output({error}) call sites exist in src/, across the 9 modules the design measured', () => { + const SRC_ROOT = path.resolve(__dirname, '../src'); + + function listCtsFiles(dir) { + const out = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) out.push(...listCtsFiles(full)); + else if (entry.name.endsWith('.cts') && !entry.name.endsWith('.d.cts')) out.push(full); + } + return out; + } + + function hasErrorProp(objLit) { + return objLit.properties.some((p) => { + if (ts.isPropertyAssignment(p) || ts.isShorthandPropertyAssignment(p)) { + const name = p.name; + if (ts.isIdentifier(name)) return name.text === 'error'; + if (ts.isStringLiteral(name)) return name.text === 'error'; + } + return false; + }); + } + + const perFile = {}; + let total = 0; + for (const file of listCtsFiles(SRC_ROOT)) { + const text = fs.readFileSync(file, 'utf8'); + const sf = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + let count = 0; + (function visit(node) { + if (ts.isCallExpression(node)) { + let calleeName = null; + if (ts.isIdentifier(node.expression)) calleeName = node.expression.text; + else if (ts.isPropertyAccessExpression(node.expression) && ts.isIdentifier(node.expression.name)) { + calleeName = node.expression.name.text; + } + if (calleeName === 'output' && node.arguments.length > 0 && ts.isObjectLiteralExpression(node.arguments[0])) { + if (hasErrorProp(node.arguments[0])) count += 1; + } + } + ts.forEachChild(node, visit); + })(sf); + if (count > 0) perFile[path.basename(file)] = count; + total += count; + } + + assert.deepStrictEqual( + perFile, + { + 'commands.cts': 5, 'frontmatter.cts': 7, 'gsd2-import.cts': 2, 'phase.cts': 4, + 'roadmap.cts': 3, 'state.cts': 25, 'template.cts': 3, 'verify.cts': 8, 'workstream.cts': 7, + }, + `per-file output({error}) census drifted: ${JSON.stringify(perFile)}`, + ); + assert.strictEqual(total, 64, `enumerated output({error}) population drifted from the measured 64: got ${total}`); + }); +}); + +describe('#3912 C5/D: output({error}) DEGRADED reaches process.exitCode only through runMain', () => { + afterEach(() => { + setPendingOutcome(undefined); + resolveContractVersion({ argv: ['node', 'x'], env: {} }); + }); + + test('a real gsd-tools-shaped main() that calls output({error}) and returns nothing exits 0 under v1, 80 under v2', () => { + const script = ` + const io = require(${JSON.stringify(IO_PATH_3912)}); + const c = require(${JSON.stringify(CLI_EXIT_PATH_3912)}); + c.runMain(() => { + io.output({ found: false, error: 'not found' }, false); + return undefined; + }); + setImmediate(() => {}); + `; + const v1 = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, env: { ...process.env, GSD_EXIT_CONTRACT: 'v1' }, + })); + assert.strictEqual(v1.status, 0, `stderr: ${v1.stderr}`); + const v2 = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, env: { ...process.env, GSD_EXIT_CONTRACT: 'v2' }, + })); + assert.strictEqual(v2.status, 80, `stderr: ${v2.stderr}`); + }); + + test('an explicit main() return still wins over a DEGRADED output({error}) call in the same main()', () => { + const script = ` + const io = require(${JSON.stringify(IO_PATH_3912)}); + const c = require(${JSON.stringify(CLI_EXIT_PATH_3912)}); + c.runMain(() => { + io.output({ found: false, error: 'not found' }, false); + return 0; + }); + setImmediate(() => {}); + `; + const r = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, env: { ...process.env, GSD_EXIT_CONTRACT: 'v2' }, + })); + assert.strictEqual(r.status, 0, `an explicit 0 return must win over the DEGRADED cell; stderr: ${r.stderr}`); + }); +}); + +describe('review fix: pending-outcome cell lifetime (last-write-wins, cleared on consumption)', () => { + // These drive runMain/output IN-PROCESS (not via a subprocess), which is + // exactly the gap that let the leak through: every other #3912 test above + // spawns a fresh process per case, so a cell that is never cleared was + // unobservable. runMain mutates process.exitCode as a side effect, so each + // test saves/restores it to avoid corrupting the real node:test run's own + // exit code. + function waitForRunMain() { + // runMain resolves its outcome via a Promise.resolve().then().then() + // chain (microtasks); a macrotask tick guarantees both have drained. + return new Promise((resolve) => { setImmediate(resolve); }); + } + + afterEach(() => { + // Harmless test hygiene now that production also clears the cell on + // every runMain call and on every clean output() — this scrub is a + // belt-and-suspenders reset between test cases, not the mechanism that + // prevents the leak (that mechanism now lives in cli-exit.cts/io.cts). + setPendingOutcome(undefined); + resolveContractVersion({ argv: ['node', 'x'], env: {} }); + }); + + test('leak regression: output({error}) then a second void-returning runMain must NOT inherit stale DEGRADED', async () => { + resolveContractVersion({ argv: ['node', 'x', '--exit-contract=v2'], env: {} }); + const savedExitCode = process.exitCode; + try { + // First invocation declares DEGRADED via a payload-carried error and + // returns nothing — runMain projects it to 80 under v2. + runMain(() => { + io.output({ found: false, error: 'not found' }, false); + return undefined; + }); + await waitForRunMain(); + assert.strictEqual(process.exitCode, 80, 'first runMain should have projected the DEGRADED cell to 80'); + + // Second, unrelated invocation in the SAME process declares nothing + // and returns nothing. Before the fix, the cell was never cleared by + // runMain, so this would inherit the first call's stale DEGRADED and + // also exit 80 — the exact bug the reviewers found. + process.exitCode = undefined; + runMain(() => undefined); + await waitForRunMain(); + assert.strictEqual( + process.exitCode, + undefined, + 'a later void-returning runMain must not inherit a prior invocation\'s stale DEGRADED declaration', + ); + } finally { + process.exitCode = savedExitCode; + } + }); + + test('last-write-wins: output({error}) then output({ok:true}) in the SAME invocation is not degraded', async () => { + resolveContractVersion({ argv: ['node', 'x', '--exit-contract=v2'], env: {} }); + const savedExitCode = process.exitCode; + try { + runMain(() => { + io.output({ found: false, error: 'not found' }, false); + io.output({ ok: true }, false); + return undefined; + }); + await waitForRunMain(); + assert.strictEqual( + process.exitCode, + undefined, + 'a later clean output() must undo an earlier degraded one — exit code must stay untouched, not 80', + ); + } finally { + process.exitCode = savedExitCode; + } + }); + + test('consumption clears: after runMain consumes a pending outcome, the cell reads unset', async () => { + resolveContractVersion({ argv: ['node', 'x', '--exit-contract=v2'], env: {} }); + const savedExitCode = process.exitCode; + try { + runMain(() => { + io.output({ error: 'x' }, false); + return undefined; + }); + await waitForRunMain(); + assert.strictEqual(process.exitCode, 80); + assert.strictEqual(getPendingOutcome(), undefined, 'the cell must be cleared once runMain has consumed it'); + } finally { + process.exitCode = savedExitCode; + } + }); +}); + +describe('#3912 A6: error() stderr bytes are unchanged by this phase', () => { + afterEach(() => { + resolveContractVersion({ argv: ['node', 'x'], env: {} }); + }); + + test('plain mode: "Error: " bytes are identical regardless of reason or contract version', () => { + for (const version of VERSIONS_3912) { + resolveContractVersion({ argv: ['node', 'x', `--exit-contract=${version}`], env: {} }); + let caught; + const chunks = []; + const origWrite = fs.writeSync; + fs.writeSync = (fd, buf, offset, length) => { + if (fd !== 2) return origWrite(fd, buf, offset, length); + const slice = Buffer.isBuffer(buf) ? buf.subarray(offset, offset + length) : Buffer.from(String(buf)); + chunks.push(slice.toString('utf8')); + return slice.length; + }; + try { + io.setJsonErrorMode(false); + try { io.error('boundary case', io.ERROR_REASON.SDK_MISSING_ARG); } catch (e) { caught = e; } + } finally { + fs.writeSync = origWrite; + } + assert.ok(caught instanceof ExitError); + assert.strictEqual(chunks.join(''), 'Error: boundary case\n', `version=${version}`); + } + }); + + test('json mode: the stderr envelope is identical regardless of contract version (only the thrown exit code differs)', () => { + for (const version of VERSIONS_3912) { + resolveContractVersion({ argv: ['node', 'x', `--exit-contract=${version}`], env: {} }); + const chunks = []; + const origWrite = fs.writeSync; + fs.writeSync = (fd, buf, offset, length) => { + if (fd !== 2) return origWrite(fd, buf, offset, length); + const slice = Buffer.isBuffer(buf) ? buf.subarray(offset, offset + length) : Buffer.from(String(buf)); + chunks.push(slice.toString('utf8')); + return slice.length; + }; + let caught; + try { + io.setJsonErrorMode(true); + try { io.error('boundary case', io.ERROR_REASON.SDK_MISSING_ARG); } catch (e) { caught = e; } + } finally { + fs.writeSync = origWrite; + io.setJsonErrorMode(false); + } + assert.ok(caught instanceof ExitError); + assert.deepStrictEqual( + JSON.parse(chunks.join('').trim()), + { ok: false, reason: 'sdk_missing_arg', message: 'boundary case' }, + `version=${version}`, + ); + } + }); +}); + +describe('#3912 B1/B4/E1: projectOutcome-backed checks over the real registry', () => { + test('B1: every registered outcome name is reachable through error() via SOME reason, and matches the registry', () => { + // Sanity check that CODE_FOR_3912 (derived straight from the shipped + // registry) matches the pinned table cli-exit.test.cjs already asserts. + assert.strictEqual(CODE_FOR_3912.get('USAGE'), 64); + assert.strictEqual(CODE_FOR_3912.get('NO_INPUT'), 66); + assert.strictEqual(CODE_FOR_3912.get('UNAVAILABLE'), 69); + assert.strictEqual(CODE_FOR_3912.get('INTERNAL'), 70); + assert.strictEqual(CODE_FOR_3912.get('DEGRADED'), 80); + }); + + test('B4: any output({error}) site under v2 exits 80 (DEGRADED)', () => { + const script = ` + const io = require(${JSON.stringify(IO_PATH_3912)}); + const c = require(${JSON.stringify(CLI_EXIT_PATH_3912)}); + c.runMain(() => { io.output({ error: 'x' }, false); return undefined; }); + setImmediate(() => {}); + `; + const r = toLegacyResult(runNode(['-e', script], { + timeoutMs: PROBE_TIMEOUT_MS, env: { ...process.env, GSD_EXIT_CONTRACT: 'v2' }, + })); + assert.strictEqual(r.status, 80, `stderr: ${r.stderr}`); + }); + + // E1 lives primarily in tests/cli-exit.test.cjs (projectOutcome is defined + // there); this is the io-side control confirming REGISTERED_NAMES_3912 + // used by the reason-mapping table above matches the live registry. + test('registered names used by the reason-mapping table are exactly the live registry names', () => { + for (const outcome of Object.values(EXPECTED_REASON_OUTCOME_3912)) { + if (outcome === 'FAIL') continue; + assert.ok( + REGISTERED_NAMES_3912.includes(outcome), + `mapped outcome ${outcome} must be a registered exit-code name`, + ); + } + }); + + test('fast-check: E1 sanity — every mapped outcome/version pair used by error() yields a non-negative integer', () => { + const outcomes = [...new Set(Object.values(EXPECTED_REASON_OUTCOME_3912))]; + fc.assert( + fc.property( + fc.constantFrom(...outcomes), + fc.constantFrom(...VERSIONS_3912), + (outcome, version) => { + const code = outcome === 'FAIL' ? 1 : (version === 'v1' ? 1 : CODE_FOR_3912.get(outcome)); + assert.ok(Number.isInteger(code) && code >= 0); + }, + ), + { seed: 39120, numRuns: 100 }, + ); + }); +});