golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

enhance(#3884): failure is a value — strict argv, and --pick that signals absence (#3922)

Browse Source 
* test(#3884): failing-first coverage for strict argv and absence-signalling --pick

ADR-3473 §8.4 says failure is a value. Three families currently encode failure as
success, and this commit pins each one RED before the fix lands.

Measured on this tree, 2026-08-26:

  gsd-tools generate-slug "test" --pick nonexistent
    -> empty stdout, exit 0                                     (#3365)

  gsd-tools audit-open --pick nonexistent_field
    -> dumps the entire human-readable audit report, exit 0

  gsd-tools generate-slug "Hello World" --raw --pick bogus
    -> prints "hello-world", another field's value, exit 0

  gsd-tools query state.planned-phase 3        (positional, no --phase)
    -> exit 0; STATE.md's "Phase: 2 of 5 (Widget Support)" is overwritten to
       "Phase: null - READY TO EXECUTE" and the frontmatter gains a corrupted
       current_phase_name                                        (#3358)

tests/pick-flag.test.cjs:27 previously asserted the #3365 defect as the contract
("returns empty string for missing field", success === true). That assertion is
replaced by the required behavior rather than deleted.

The new parseNamedArgs block calls the spec-object signature that does not exist
yet, so it fails today by construction. The 11 existing behavior-lock tests are
left untouched here; they are corrected in the implementation commit.

C1/C4 assert at the consumer's output - STATE.md's bytes - per ADR-3180
Decision 4(b). A unit assertion on the parser would have passed throughout this
defect's life.

Design:      .gsd/phase/feat-3884-failure-is-a-value/40-design.md
Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md

Refs #3884

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* enhance(#3884): failure is a value — strict argv, and --pick that signals absence

Implements ADR-3473 §8.4. Absence, emptiness and failure stop being interchangeable
ways to say "I could not answer".

parseNamedArgs (src/command-arg-projection.cts)
  Takes a spec object with a REQUIRED `positionals: number | 'rest'` and returns the
  hub's Result shape instead of a bare Record. Declaring the positional arity is what
  makes #3358's call site unrepresentable rather than merely detectable: an unrecognized
  flag or a token past the declared boundary is now InvalidArgs, naming the offending
  token and listing the accepted flags. The legacy positional-array call shape throws
  a TypeError — an internal invariant violation per ADR-3473 Decision 2, so a stale
  hand-written .cjs call site fails loudly instead of destructuring undefined off a
  Result. parseNamedArgsOrExit projects a failure onto the caller's error(); it is a
  projection over the one parser, not a second parser.

  Measured before, against a STATE.md with a populated phase-2 block:
    query state.planned-phase 3        (positional, no --phase)
    -> exit 0; "Phase: 2 of 5 (Widget Support)" overwritten to
       "Phase: null - READY TO EXECUTE", frontmatter gains a corrupted
       current_phase_name
  After: exit 1, `unexpected positional argument "3"`, STATE.md byte-identical.
  The flag form is unchanged and still updates STATE.md.

--pick <field> (gsd-core/bin/gsd-tools.cjs)
  extractField returns {found,value}, and the pick block no longer shares one catch
  between "output was not JSON" and "field was absent". An absent field exits 1 with
  pick_field_absent, naming the field and the keys that do exist; non-JSON output exits 1
  with pick_output_not_json instead of dumping the command's entire output. A field that
  is PRESENT with value null, '', 0 or false still prints at exit 0 — that is an answer,
  not a failure, and it is what keeps `--pick count` printing 0 on a fresh project.

  Measured before: `audit-open --pick nonexistent_field` printed the whole human-readable
  audit report at exit 0, and `generate-slug X --raw --pick bogus` printed "hello-world" —
  a different field's value, confidently, at exit 0.

  ADR-3409 Decision 7 explicitly deferred this contract fix to #3473; this is it. The
  sub-issue's "returns 0 when the count is zero OR absent" wording is superseded by the
  ADR rule it implements: zero prints 0, absence exits non-zero. Defaulting absence to 0
  would demote "could not answer" to "the answer is zero" — the hazard
  docs/how-to/resolve-unreachable-guard-findings.md already warns against.

Guard ledger (ADR-3473 Decision 6)
  scripts/lint-unreachable-guard-drift.cjs Detector A is RETIRED. Its premise — that a
  `--pick ... || echo` arm can never fire — is now false, so the shape it forbade is the
  correct idiom and keeping it would forbid the fix. Detector B (glob-consuming cat/ls,
  a nullglob mechanism this change does not touch) is retained in full, as are the shared
  scanner, the escape-marker parser and the baseline. Net: -1 detector, 0 added. The file
  is not deleted.

Call-site audit
  45 prompt-layer --pick invocations, every one a plain X=$(...) assignment — none in an
  if test, && chain, or a pipeline whose status is consumed, and no shell block in
  workflows/commands/agents/references sets -e. Of the 13 (command, field) pairs the
  prompt layer reads, 10 are always present; the 3 sometimes-absent ones each sit behind
  a prior found/existence check. No ADR-3409-class "field the command never produces"
  remains.

Design:      .gsd/phase/feat-3884-failure-is-a-value/40-design.md
Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md

Refs #3884

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#3884): escape untrusted tokens in diagnostics, and cover five unpinned rows

Two review findings, both fixed here rather than recorded as limits.

1. A newline in an untrusted token forged a second stderr line.

   Before, plain-text mode:
     $ gsd-tools query state.planned-phase $'foo\nError: forged second line'
     Error: unexpected positional argument "foo
     Error: forged second line"

   After:
     Error: unexpected positional argument "foo\nError: forged second line"

   --json-errors mode was never affected — io.error runs that payload through
   JSON.stringify. Plain-text mode writes 'Error: ' + message verbatim, and the
   three new InvalidArgs reasons plus the two new --pick diagnostics all
   interpolate a token that comes straight from argv.

   Fixed with ONE shared helper, formatDiagnosticToken (src/io.cts), applied at
   every interpolation site — not a copy per site. It is deliberately NOT
   applied inside error() itself: several callers in this tree emit intentional
   multi-line diagnostics, and escaping newlines there would mangle them.

   The available-top-level-keys list needed the same treatment for a reason the
   review did not anticipate: `frontmatter get <file>` reads an ARBITRARY user
   document and echoes that document's own keys into the diagnostic. Verified
   reachable — a frontmatter key containing a newline reaches the key list — so
   formatKeyForDiagnosticList is guarding a live path, not a hypothetical one.
   Ordinary keys still render plain and unquoted; a fix that merely dropped the
   key would also have passed a "one line" assertion, so the test pins the
   escaped key's presence too.

2. Five behavior-table rows were implemented but nothing pinned them:
   B7  a dotted path that dies partway
   B9  bracket syntax on a non-array
   B10 a negative array index, in and out of range
   B14 a JSON root that is not an object
   B17 an @file: payload over 50KB

   B17 is the load-bearing one. output() writes @file:<path> instead of inline
   JSON past 50000 characters, and --pick resolves that BEFORE parsing; with no
   test, a future reordering of those two steps turns every large result into a
   false pick_output_not_json. The fixture seeds 1200 phase directories and
   measures the payload at 62474 characters, asserting the spill actually
   happened rather than assuming it.

Refs #3884

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#3884): correct the strict-argv surface against a full verification run

The first full run came back with 90 failures across 12 files, none in the new
tests. They were the argv surface telling me what it actually is. Ten root
causes; each classified before anything was changed.

I over-implemented, and that is reverted.

  ADR-3473 §8.4 says parseNamedArgs rejects "unrecognized and positional
  tokens". It says nothing about a value flag whose value is missing. Making
  that an error was my design decision, not the rule, and it broke a
  deliberately recorded contract: `--prd` with no value resolving to null
  (tests/init.test.cjs emptyPrdValueIsFalsyAndTreatedAsAbsent, row B5;
  tests/section-manifest-init-facts.test.cjs "flag-shaped value"). The
  "requires a value" branch is deleted outright rather than kept behind an
  option — an unused strictness mode is speculative generality. Unknown-flag
  and unexpected-positional rejection, which is what §8.4 actually mandates,
  is unchanged.

--wave needed a third flag kind the original design did not anticipate.

  `--wave N` is documented (commands/gsd/execute-phase.md:4,48) and the
  shipped workflow reconstructs and passes it (execute-phase.md:84), while
  #2932 records token-PRESENCE semantics: the CLI cares only that the flag
  appeared, and the value belongs to the workflow layer. That is neither a
  boolean flag nor a value flag, so `optionalValueFlags` now exists —
  presence-only in `data`, and the validation cursor consumes a following
  non-flag token so it is not reported as a stray positional. Every other
  declared boolean flag was checked against every argument-hint and prose
  usage in commands/, workflows/, agents/ and docs/; `--wave` is the only one
  of this shape.

Five tests were pinning forms that never worked.

  tests/adr857-core-without-capabilities.test.cjs passed
  `init plan-phase --phase 01-stub`, but the documented form is positional
  (docs/CLI-TOOLS.md:776) and the handler reads args[2] — which for that form
  is the literal string "--phase". Measured on the pre-fix build against a
  real .planning/phases/01-stub/ directory:

    init plan-phase 01-stub          -> phase_found=true
    init plan-phase --phase 01-stub  -> phase_found=false

  The test asserted only exit 0 and key presence, so it had been green while
  proving nothing about phase resolution. Corrected to the documented form and
  strengthened to assert phase_found === true. Same class in state.test.cjs
  (`--plan-count`, a flag that does not exist; the real one is `--plans`),
  milestone-archive.test.cjs (`init new-milestone --json`, silently ignored),
  and concurrency-safety.test.cjs (a bare positional field name whose
  OR-assertion passed because a whole-document dump happens to contain the
  substring it looked for).

Six handlers had no argv validation at all — the same #3358 shape this phase
exists to close, found while fixing the rest: init verify-work / phase-op /
review / todos / remove-workspace read args[2] with nothing checking the rest,
and validate health read --repair/--backfill through a bare args.includes()
scan that bypassed the parser entirely. All now go through the seam, so the
flag has one owner.

tests/init-debug.test.cjs rows C4/C5 asserted that an unrecognized flag must
NOT fail. That is the behavior §8.4 removes, and Decision 8 says a caller's
local expectation does not override §8, so they are inverted and renamed —
a test still called "ignores an unrecognized flag" while asserting rejection
would be its own defect. Row C6's point is its PWNED canary; that assertion is
kept verbatim and only its exit-status expectation changed, because the
hostile token is now rejected rather than absorbed.

The blast-radius estimate in 40-design.md is corrected rather than quietly
left wrong. get_impact reported MEDIUM / 8 symbols upstream, and that was
accurate for what the graph can see — parseNamedArgs's callers. It cannot see
that those callers' handlers accept argv shapes wider than the code reading
args[2] suggests, which is where the real surface was.

Refs #3884

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#3884): withdraw the validate-health tightening, finish the A2/A3 revert

Second full run: 46 failures, down from 90. Four causes, two of them mine.

Reverted `validate health` entirely — it was scope creep, and it broke a real flag.

  ~30 of the 46 read `unknown flag "--json"; accepted: --repair, --backfill`.
  The previous commit routed `validate health` through the parser on the
  reasoning that a flag should have one owner. That was wrong twice over:
  §8.4 names parseNamedArgs and count queries, and `validate health` was never
  a parseNamedArgs call site — it read its flags, just not through the parser,
  so it had no silent-drop defect to fix. Tightening it omitted `--json`, which
  the health-diagnostic suites use heavily. The handler is now byte-for-behaviour
  back to its pre-branch form. `validate context` stays converted: it genuinely
  was a call site, and its `--json` is now declared rather than read by a second
  `args.includes` scan.

  The five handlers that had NO validation at all — init verify-work / phase-op /
  review / todos / remove-workspace — stay fixed. Those read args[2] with nothing
  checking the rest, which is the #3358 shape this phase owns.

Finished the A2/A3 revert. Three tests still encoded the deleted
"a value flag with a missing value is an error" rule, including one added by the
previous commit for that rule. All three now assert the reverted null contract,
and the ones whose titles said "rejected" are renamed — a test named for a
contract it no longer asserts is its own defect.

`--wave=` and `--wave --weird` are correctly rejected. Neither is documented in
commands/gsd/execute-phase.md, gsd-core/workflows/execute-phase.md or docs/, and
neither is emitted by the shipped prompt layer, so both are unrecognized tokens
that §8.4 mandates rejecting. `doesNotConsumeFollowingFlagAsWaveValue` keeps the
property it exists for — asserted directly now, at the parser, that `--wave` does
not swallow a following flag as its value — and only its exit-status expectation
changed.

A contradiction inside this branch, surfaced by the audit and resolved the safe way.

  Two pre-existing #3573 tests call `state begin-phase '2'` and
  `state planned-phase '2'` with a bare positional, relying on the old permissive
  parser to ignore it. This branch's own #3358 regression test requires that exact
  argv to be REJECTED. The two are mutually exclusive.

  Widening the router to accept a bare positional — mirroring complete-phase —
  would have silently re-opened #3358, and was verified to do exactly that: with
  the widened router, `query state.planned-phase 3` returned exit 0 and wrote
  current_phase_name again. It is reverted. docs/CLI-TOOLS.md:116 and
  docs/COMMANDS.md:2192 document only the `--phase N` form for both verbs, so the
  two #3573 tests move to it. Their assertions were never about the call shape —
  only that total_phases survives the resync — and both still pass.

  complete-phase is untouched: its bare positional IS documented, and it keeps the
  dynamic boundary and the negative-space note that record why.

The audit that produced this is in the PR body: for every handler whose declaration
changed, the flags it reads anywhere in its body, the flags the shipped surface
documents, and the shapes the suite passes, compared. The `--json` miss was a
pattern, not an accident — declaring a handler's flags from its parseNamedArgs call
alone misses whatever it reads elsewhere.

Refs #3884

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* chore(#3884): backfill the changeset PR number

Refs #3884

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-08-27 00:12:13 -04:00
committed by GitHub
parent 878f25025c
commit e20744eacb
30 changed files with 1905 additions and 427 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
docs/CLI-TOOLS.md
View File

@@ -25,13 +25,49 @@ node gsd-tools.cjs <command> [args] [--raw] [--cwd <path>]
**Global flags (CJS):**
| Flag | Description |
| -------------- | ---------------------------------------------------------------------------- |
| `--raw` | Machine-readable output (JSON or plain text, no formatting) |
| `--cwd <path>` | Override working directory (for sandboxed subagents) |
| `--ws <name>` | Workstream context for `.planning/workstreams/<name>` paths |
| Flag | Description |
| ------------------- | ---------------------------------------------------------------------------- |
| `--raw` | Machine-readable output (JSON or plain text, no formatting) |
| `--cwd <path>` | Override working directory (for sandboxed subagents) |
| `--ws <name>` | Workstream context for `.planning/workstreams/<name>` paths |
| `--pick <field>` | Extract one field from a command's JSON output — see [`--pick <field>` contract](#--pick-field-contract) below |
---
### `--pick <field>` contract
`--pick <field>` runs `<command>` as normal, parses its stdout as JSON, and
extracts one field by name (dotted paths and `[N]` array indices are
supported, e.g. `a.b.c`, `directories[-1]`). As of ADR-3473 §8.4 / #3884, the
three possible outcomes are distinguished **by exit code**, never by an
ambiguous empty string:
| Outcome | stdout | stderr | Exit code |
| --- | --- | --- | --- |
| Field present | The field's value, coerced to a string | (none) | `0` |
| Field absent (missing key, out-of-range index, dotted path partially missing, or a non-object JSON root) | empty | Diagnostic naming the field and the available top-level keys (or the actual JSON root type) | `1` (`pick_field_absent`) |
| Command output is not JSON (including `--raw` output, which is plain text/human-readable, not JSON) | empty | Diagnostic saying the output was not JSON | `1` (`pick_output_not_json`) |
A `null` or empty-string (`''`) field value is a real answer, not an absence
— it still prints (an empty line) at exit **0**. Only the *absence of the
field itself* is a failure. This is why `--raw` and `--pick` are, in
practice, mutually exclusive: `--raw` output is not JSON, so combining them
always hits `pick_output_not_json`.
**This replaces the previous behavior.** Before #3884, an absent field (or
non-JSON output) silently printed an empty string at exit `0` — indistinguishable
from a field that genuinely held `null` or `''`. That coercion is gone. The
common shell idiom
```bash
X=$(gsd_run query some.command --pick some_field 2>/dev/null) || X=default
```
now works as written: the `|| X=default` arm fires exactly when the field
could not be resolved, and never fires merely because the resolved value
happens to be empty.
---
## State Commands

77
docs/FEATURES.md
View File

@@ -197,6 +197,7 @@
- [Machine-Readable State Contract (`.planning/state.json`)](#166-machine-readable-state-contract-planningstatejson)
- [Stated Failing Direction](#167-stated-failing-direction)
- [Runtime Identity](#168-runtime-identity)
- ["Failure Is a Value" — Strict Argv Rejection and the `--pick` Absence Contract](#3884-failure-is-a-value--strict-argv-rejection-and-the---pick-absence-contract)
---
@@ -3661,6 +3662,82 @@ _Generated by `scripts/gen-features.cjs` — add a fragment under `docs/features
---
### 3884. "Failure Is a Value" — Strict Argv Rejection and the `--pick` Absence Contract
**Purpose:** ADR-3473 §8.4 states the rule directly: absence, emptiness, and
failure are three different things, and a routine that cannot tell them
apart eventually reports the wrong one. Before this change, `gsd-tools` had
two silent instances of exactly that collapse.
**Half one — a stray positional corrupted state, silently (#3358).**
`parseNamedArgs` read only the flags it recognized and dropped everything
else — an unrecognized `--flag` or an extra positional argument (for
example, a stray phase number appended after `state.planned-phase`) was
silently discarded rather than rejected. The caller's own positional read
(`args[2]`, etc.) still worked, so the command ran anyway, on the wrong
phase, and overwrote the previously-current phase block with no error at
all. The fix makes `parseNamedArgs(args, spec)` return the command-routing
hub's own `Result` shape (`{ok:true,data} | {ok:false,kind:'InvalidArgs',...}`)
and requires every call site to declare `positionals: number | 'rest'` — the
count of leading argv slots the caller itself reads directly. An unknown
flag or an unexpected positional past that boundary is now a loud,
non-zero-exit `InvalidArgs` failure instead of a token quietly falling on
the floor. A duplicate flag, a negative-number value (`--plans -1`), and a
documented free-text tail (`init quick <description>`) are deliberately
left alone — none of them are the defect this closes, and forbidding them
would just break working call sites for no gain.
**Half two — `--pick` on an absent field answered `''` at exit 0, exactly
like a present-but-empty one (#3365).** `--pick <field>` extracted one field
from a command's JSON output, but a missing key, an out-of-range array
index, a partially-missing dotted path, or non-JSON output (including a
`--raw` command's output) all rendered the same way: empty stdout, exit
`0`. That is indistinguishable from a field that genuinely holds `null` or
`''` — a real answer. The shell idiom `X=$(… --pick F) || X=default` could
therefore never observe the failure it was written to react to; only a typo
in the verb name would ever make it exit non-zero. `--pick` now exits `1`
with a diagnostic on stderr (`pick_field_absent` naming the field and the
available top-level keys, or `pick_output_not_json` when the output could
not be parsed as JSON at all) whenever the field cannot be resolved. A
present field's value — including `0`, `false`, `null`, and `''` — is
unchanged: those are answers, not failures, and remain exit `0`. See
[CLI-TOOLS.md's `--pick <field>` contract](CLI-TOOLS.md#--pick-field-contract)
for the full outcome table and [json-errors.md](json-errors.md) for the two
new reason codes.
**Why not just default to zero for an absent count.** The sub-issue's own
Done-when checkbox suggested treating an absent field the same as a
zero-valued one. That is rejected on the merits: it demotes *"I could not
answer"* to *"the answer is zero"*, which would make a count-gated shell
guard fire unconditionally on the very projects that could never resolve
the count in the first place — the opposite of what a gate is for.
**Consequence for `scripts/lint-unreachable-guard-drift.cjs`.** That guard's
Detector A existed specifically because the old `--pick` behavior made a
`--pick … || echo <default>` line's fallback arm permanently unreachable.
Once `--pick` exits non-zero on absence, that premise is false and the
shape the detector forbade becomes the *correct* idiom — so Detector A was
retired rather than kept. Detector B (the unrelated `cat`/`ls`-over-a-glob
nullglob hazard) is untouched. See
[Resolve unreachable-guard findings, Shape A](how-to/resolve-unreachable-guard-findings.md#shape-a---pick-with-an--echo-fallback-resolved-upstream-3884).
**Known limits:**
- A value token beginning with `--` still cannot be passed to a declared
value flag (`--summary "--force is now default"` now fails loudly instead
of silently dropping the value) — strictly better, but no `--flag=value`
escape was added.
- `--pick` still cannot distinguish an absent field from a `null` one *on
stdout alone* — the distinction is carried entirely by exit code.
- The ~10 `gsd-tools.cjs` call sites of `parseNamedArgs` get no
compile-time check (that file is hand-written JavaScript, not `.cts`);
enforcement there is the runtime throw on a stale legacy call shape plus
behavioral tests.
- This phase does not sweep every routine in `gsd-core` for `Result`
conformance — it applies the rule to the argument-projection seam and the
`--pick` extractor it names, not the whole codebase.
---
_Generated by `scripts/gen-features.cjs` — add a fragment under `docs/features/` and run `--write`._
<!-- FEATURES:END -->

2
docs/INVENTORY.md
View File

@@ -493,7 +493,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`.
| `codex-agent-toml.cjs` | Typed IR (genuine leaf) for `~/.codex/agents/<agent>.toml` — `parseCodexAgentToml`/`renderCodexAgentToml` round-trip byte-identically; `stripModel`/`stripReasoningEffort` remove exactly one targeted line; `scanTomlLines`/`stripBOM`/`findDeveloperInstructionsBlockRange`/`unquoteTomlValue` are the lenient reader primitives moved here from `agent-install-check.cjs` (#3242 Phase 2); consumed by the Codex `.toml` sync (`commands.cjs cmdEffortSyncCodex`, ADR-2313 D7, #3243) |
| `command-aliases.cjs` | Alias/subcommand metadata for manifest-backed family routers |
| `commonjs-marker.cjs` | Ownership-guarded `{"type":"commonjs"}` marker used to pin GSD's staged `.js` scripts to CommonJS; exports `classifyMarker` (absent/gsd-owned/foreign, fail-closed), `ensureCommonJsMarker`, and `removeCommonJsMarker` so install and uninstall share one predicate and never touch a user-authored `package.json` (#2544) |
| `command-arg-projection.cjs` | Typed flag and positional argument projection helpers shared across command-family routers |
| `command-arg-projection.cjs` | Strict, `Result`-returning flag and positional argument projection helpers shared across command-family routers — `parseNamedArgs` rejects unrecognized flags and stray positionals instead of silently dropping them (ADR-3473 §8.4, #3884). Generated from `src/command-arg-projection.cts` |
| `command-roster.cjs` | Read-only discovery of canonical `commands/gsd/*.md` command stems for runtime artifact conversion and namespace rewrites |
| `command-routing-hub.cjs` | Pure-result dispatch hub that centralizes mode decision (SDK vs CJS), error taxonomy, and no-throw contract for all command-family routers (#3788) |
| `commands.cjs` | Misc CLI commands (slug, timestamp, todos, scaffolding, stats) |

77
docs/features/failure-is-a-value-argv-and-pick-contracts.md Normal file
View File

@@ -0,0 +1,77 @@
---
id: 3884
title: "Failure Is a Value" — Strict Argv Rejection and the `--pick` Absence Contract
group: v1.7.0 Features
---
**Purpose:** ADR-3473 §8.4 states the rule directly: absence, emptiness, and
failure are three different things, and a routine that cannot tell them
apart eventually reports the wrong one. Before this change, `gsd-tools` had
two silent instances of exactly that collapse.
**Half one — a stray positional corrupted state, silently (#3358).**
`parseNamedArgs` read only the flags it recognized and dropped everything
else — an unrecognized `--flag` or an extra positional argument (for
example, a stray phase number appended after `state.planned-phase`) was
silently discarded rather than rejected. The caller's own positional read
(`args[2]`, etc.) still worked, so the command ran anyway, on the wrong
phase, and overwrote the previously-current phase block with no error at
all. The fix makes `parseNamedArgs(args, spec)` return the command-routing
hub's own `Result` shape (`{ok:true,data} | {ok:false,kind:'InvalidArgs',...}`)
and requires every call site to declare `positionals: number | 'rest'` — the
count of leading argv slots the caller itself reads directly. An unknown
flag or an unexpected positional past that boundary is now a loud,
non-zero-exit `InvalidArgs` failure instead of a token quietly falling on
the floor. A duplicate flag, a negative-number value (`--plans -1`), and a
documented free-text tail (`init quick <description>`) are deliberately
left alone — none of them are the defect this closes, and forbidding them
would just break working call sites for no gain.
**Half two — `--pick` on an absent field answered `''` at exit 0, exactly
like a present-but-empty one (#3365).** `--pick <field>` extracted one field
from a command's JSON output, but a missing key, an out-of-range array
index, a partially-missing dotted path, or non-JSON output (including a
`--raw` command's output) all rendered the same way: empty stdout, exit
`0`. That is indistinguishable from a field that genuinely holds `null` or
`''` — a real answer. The shell idiom `X=$(… --pick F) || X=default` could
therefore never observe the failure it was written to react to; only a typo
in the verb name would ever make it exit non-zero. `--pick` now exits `1`
with a diagnostic on stderr (`pick_field_absent` naming the field and the
available top-level keys, or `pick_output_not_json` when the output could
not be parsed as JSON at all) whenever the field cannot be resolved. A
present field's value — including `0`, `false`, `null`, and `''` — is
unchanged: those are answers, not failures, and remain exit `0`. See
[CLI-TOOLS.md's `--pick <field>` contract](CLI-TOOLS.md#--pick-field-contract)
for the full outcome table and [json-errors.md](json-errors.md) for the two
new reason codes.
**Why not just default to zero for an absent count.** The sub-issue's own
Done-when checkbox suggested treating an absent field the same as a
zero-valued one. That is rejected on the merits: it demotes *"I could not
answer"* to *"the answer is zero"*, which would make a count-gated shell
guard fire unconditionally on the very projects that could never resolve
the count in the first place — the opposite of what a gate is for.
**Consequence for `scripts/lint-unreachable-guard-drift.cjs`.** That guard's
Detector A existed specifically because the old `--pick` behavior made a
`--pick … || echo <default>` line's fallback arm permanently unreachable.
Once `--pick` exits non-zero on absence, that premise is false and the
shape the detector forbade becomes the *correct* idiom — so Detector A was
retired rather than kept. Detector B (the unrelated `cat`/`ls`-over-a-glob
nullglob hazard) is untouched. See
[Resolve unreachable-guard findings, Shape A](how-to/resolve-unreachable-guard-findings.md#shape-a---pick-with-an--echo-fallback-resolved-upstream-3884).
**Known limits:**
- A value token beginning with `--` still cannot be passed to a declared
value flag (`--summary "--force is now default"` now fails loudly instead
of silently dropping the value) — strictly better, but no `--flag=value`
escape was added.
- `--pick` still cannot distinguish an absent field from a `null` one *on
stdout alone* — the distinction is carried entirely by exit code.
- The ~10 `gsd-tools.cjs` call sites of `parseNamedArgs` get no
compile-time check (that file is hand-written JavaScript, not `.cts`);
enforcement there is the runtime throw on a stale legacy call shape plus
behavioral tests.
- This phase does not sweep every routine in `gsd-core` for `Result`
conformance — it applies the rule to the argument-projection seam and the
`--pick` extractor it names, not the whole codebase.

60
docs/how-to/resolve-unreachable-guard-findings.md
View File

@@ -13,7 +13,7 @@ acknowledging is the right answer. For *why* the invariant exists, see
```
unreachable-guard-drift: NEW unreachable shell-guard shape(s) found in the prompt layer.
gsd-core/workflows/ship.md:312 --pick STATUS=$(gsd_run query verification.status "$D" --pick status 2>/dev/null || echo "")
gsd-core/workflows/ship.md:312 cat cat .planning/phases/*-*/*-SUMMARY.md
```
Each line is `file:line`, the token that matched, and the offending source line.
@@ -37,58 +37,65 @@ outcomes and only one of them is good news.
|---|---|---|
| `ok_no_violations` | 0 | Clean. Every scanned file passed. |
| `ok_baseline_updated` | 0 | You ran `--update`; the baseline was rewritten. |
| `fail_fresh_violation` | 1 | A new instance of one of the two shapes. **Fix it** — see below. |
| `fail_fresh_violation` | 1 | A new instance of Shape B (Shape A was retired upstream, #3884 — see below). **Fix it** — see below. |
| `fail_stale_entry` | 1 | A baseline entry matched fewer occurrences than it acknowledges. Either a site was migrated (good — re-record) or only *some* copies were (finish the job). |
| `fail_malformed_marker` | 1 | A `# gsd-scan-ignore:` whose reason names no issue or URL. Not a violation — a broken exemption. |
| `fail_baseline_load` | 1 | The baseline file is missing, empty, not JSON, or structurally wrong. The guard **could not look**; this is not a clean run. |
## Shape A — `--pick` with an `|| echo` fallback
## Shape A — `--pick` with an `|| echo` fallback (resolved upstream, #3884)
**This shape is no longer a finding.** As of #3884 (ADR-3473 §8.4), `--pick
<field>` exits **non-zero** — not `0` — when the field is absent, so the
guard no longer flags a line carrying both `--pick` and `|| echo`; see the
[`--pick <field>` contract](../CLI-TOOLS.md#--pick-field-contract) for the
full three-outcome table. The example below is kept for historical context
(this page previously taught the workaround for the defect), and because the
shape it shows is now the **correct, idiomatic** way to write this:
```bash
# BROKEN — the fallback can never fire
# Previously BROKEN (pre-#3884): the fallback could never fire, because an
# absent field printed '' at exit 0. As of #3884 this now works as written —
# `|| echo "false"` fires exactly when `active` cannot be resolved.
AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null || echo "false")
```
`--pick` renders a missing or absent field as the **empty string and exits 0**.
`gsd_run` passes that exit code straight through, so `||` only ever fires on a
typo in the verb name — never on the field absence you wrote it for.
Test it yourself before assuming a field exists:
You can confirm the new behavior directly:
```bash
node gsd-core/bin/gsd-tools.cjs query phases.list --pick a_field_that_does_not_exist; echo "exit=$?"
```
That prints nothing and exits `0`.
That now prints nothing on stdout, a diagnostic on stderr, and exits `1`
(`pick_field_absent`) — not `0`.
**Fix — test the value, not the exit code:**
**The two-line workaround this page used to prescribe is no longer
required, but remains harmless:**
```bash
AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null)
AUTO_MODE="${AUTO_MODE:-false}"
```
`${VAR:-default}` is exactly the empty-or-unset test, and unlike
`[ -z "$VAR" ] && VAR=default` it cannot abort a `set -e` shell when the value
is non-empty.
It still works exactly as before — `--pick` still prints `''` at exit `0`
when the field is present but `null` or empty (that is an answer, not a
failure; see the contract's negative space), so `${VAR:-default}` still
resolves those cases the same way it always did. It is simply no longer the
*only* reachable way to supply a default: the single-line `|| echo` idiom at
the top of this section now works too.
**When the safe direction is "do nothing", compare against the literal instead**
and add no default at all:
**A count that returns zero is still not the same as a count that could not
be resolved** — this half of the contract is unchanged by #3884 and remains
the reason a `:-0` default on an absent field would be a bug:
```bash
PRIOR_SUMMARIES=$(gsd_run query phases.list --type summaries --pick count 2>/dev/null)
if [ "$PRIOR_SUMMARIES" = "0" ]; then WALKING_SKELETON=true; fi
```
Here a `:-0` default would be a bug: it turns "the query could not answer" into
"there are zero summaries" and fires the gate unconditionally. Only a literal
`0` should act; anything else correctly does nothing.
**If the field does not exist at all, repoint the query — do not paper over it.**
The Walking Skeleton gate read `--pick summaries_total`, a field `phases.list`
has never produced under any flag combination, so the gate had never fired on
any project. The fix was to ask the owner that *does* answer it
(`--type summaries --pick count`), not to default the empty away.
`phases.list --type summaries --pick count` always returns a real integer
(never absent), so this comparison is safe as written; a query that *can*
return an absent field must still not paper over that with a `:-0` default —
repoint the query to one that actually answers, the same guidance as before.
## Shape B — a glob whose command succeeds on zero matches
@@ -183,6 +190,7 @@ the file most likely to grow the next copy.
## Related
- [ADR-3409](../adr/3409-unreachable-shell-guard-arms.md) — the invariant, the measurements behind both detectors, and the alternatives rejected
- [ADR-3409](../adr/3409-unreachable-shell-guard-arms.md) — the invariant, the measurements behind both detectors (one since retired), and the alternatives rejected
- [ADR-3180](../adr/3180-planning-semantic-model-single-owner.md) — the ratchet and whole-repo-discovery mechanism this guard reuses
- [CLI-TOOLS.md's `--pick <field>` contract](../CLI-TOOLS.md#--pick-field-contract) — the #3884 fix that resolved Shape A upstream and retired its detector
- [Resolve edge-coverage findings](resolve-edge-coverage-findings.md) · [Resolve prohibition findings](resolve-prohibition-findings.md) — sibling "the loop surfaced something, here is what to do with it" pages

7
docs/json-errors.md
View File

@@ -193,6 +193,13 @@ text (unstable).
| `usage` | Version flag (`--version`, `-v`) which gsd-tools never accepts |
| `usage` | Top-level no-args invocation (usage text) |
### `--pick <field>` errors (ADR-3473 §8.4, #3884)
| Code | When emitted |
|------|-------------|
| `pick_field_absent` | `--pick <field>` names a field that does not exist in the command's JSON output (missing key, out-of-range index, a partially-missing dotted path, or a non-object JSON root) — see [CLI-TOOLS.md's `--pick` contract](CLI-TOOLS.md#--pick-field-contract) |
| `pick_output_not_json` | `--pick <field>` is combined with a command whose output is not JSON (including `--raw` output) |
### Config errors (`config-get`, `config-set`, `config-ensure-section`)
| Code | When emitted |