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 }, + ); + }); +});