Files
msd-core/docs/how-to/resolve-unreachable-guard-findings.md
Tom Boucher 1591454357 feat(#3409): reject shell guards that cannot observe their own failure arm (#3558)
* test(#3409): failing-first regression tests for unreachable shell guard arms

Drives the three live defects fail-first, executing the shipped workflow
snippets rather than a re-typed copy:

- G1/G2 plan-phase.md Walking Skeleton gate reads `--pick summaries_total`,
  a field that does not exist, so PRIOR_SUMMARIES is always "" and the gate
  has never fired (#3365). G2 is the load-bearing negative-space case: it
  rejects a fix that treats "no answer" as "zero" and fires unconditionally.
- G3 plan-phase.md PHASE_REQ_IDS resolves "" instead of the TBD sentinel on
  a phase with zero requirements.
- G4 complete-milestone.md's bare `cat <glob>` blocks on stdin under a
  nullglob left set by an earlier block (measured hang).

Skipped on Windows for G4 only: the FIFO-blocked-stdin mechanism is POSIX
only, and a weakened assertion there would pass vacuously.

Refs #3409

* fix(#3409): make nine shell guards observe their own failure arm

`--pick` coerces a missing field to empty string and exits 0, so the
`|| echo <default>` fallback after it fires only on a verb typo, never on
the field absence it was written for. Nine sites relied on that arm.

- plan-phase.md walking-skeleton gate: `--pick summaries_total` names a
  field that does not exist under any flag combination, so the gate has
  never fired on any project (#3365). Repointed at the existing single
  owner, `phases.list --type summaries --pick count`, which returns a real
  integer in every case including a project with no `.planning` directory.
  No new counter is added: a second one would duplicate the ownership
  ADR-3180 Decision 1 forbids. The gate now fires only on a literal "0",
  so an unanswerable query fails safe instead of entering skeleton mode.
- plan-phase.md phase_req_ids: now falls back to the documented TBD.
- The remaining seven convert to an explicit empty test.
- complete-milestone.md read all phase summaries through a bare
  `cat <glob>`; under a nullglob left set by an earlier block that is zero
  operands, so cat blocks on stdin. Guarded with the array shape the
  #3300 fix already established in review.md.

Refs #3409

* fix(#3409): guard eleven more globs that defeat their own fallback arm

The nullglob audit this issue asks for turned up the same class in files
#3300 never touched.

- Eight bare `cat <glob>` reads (transition, complete-milestone, planner x4,
  verifier, phase-researcher). With nullglob set that is zero operands, so
  cat reads stdin and blocks; measured rc=137 at 3s.
- Three `ls <glob> || echo "<message>"` sites (session-report,
  review-backlog and its generated skill). nullglob makes ls succeed
  listing the cwd, so the message never prints and the user gets a
  directory listing instead.

Guarded with `[ -e "${_ARR[0]}" ]` rather than `[ ${#_ARR[@]} -gt 0 ]`.
The count form is correct only when nullglob is set, and six of these
seven files never set it: without it the array holds the unmatched literal
pattern, so the count is 1 and the guard passes wrongly. `-e` is correct
in both worlds. review.md keeps its count guards — that block sets
nullglob two lines above them.

skills/gsd-review-backlog regenerated from commands/, never hand-edited.

Refs #3409

* feat(#3409): add the unreachable-shell-guard drift lint

A sibling of lint-planning-prompt-drift.cjs, consuming the shared
scripts/lib/drift-scan.cjs rather than copying it, wired into lint:ci.

Both detectors are one shape — a fallback arm defeated by a legitimate
success-on-empty:

- Detector A: `--pick` and `|| echo` on one line. `--pick` is the
  discriminator because "missing field renders empty at exit 0" is a
  documented CLI contract, not a heuristic. A rule keyed on gsd_run
  matched 111 lines, ~132 of them legitimate, and was rejected.
- Detector B: `cat <glob>` in command position, and `ls <glob>` whose
  exit code feeds a real fallback or an if/while head. Informational
  `ls <glob>` whose stdout is consumed (97 sites) and `|| true` failure
  suppression (~15) are not guards and never fire.

Shrink-only ratchet keyed on (file, trimmed text) with a per-pair count,
POSIX-normalized unconditionally so Windows CI cannot report everything
fresh and stale at once. Ships with a ZERO-entry baseline: every site it
can find is fixed. Exemption is the per-line `# gsd-scan-ignore: #NNN`
marker whose reason must name an issue or URL; a malformed reason reports
a distinct error rather than silently exempting. No file allowlists.

ADR-3409 records the invariant, the measurements behind both detectors,
and why the upstream `--pick` contract fix belongs to #3473.

Refs #3409

* fix(#3409): resolve review findings — typed surface, sanitized reports, tighter marker

Standards axis (blocker): the guard's tests asserted on human-readable
stdout/stderr and on free-form baseline-load prose, which CONTRIBUTING
prohibits by name. Added the typed surface it prescribes instead of
weakening the tests: a frozen REASON enum, a --json report mode,
structured loadBaseline errors, and a test locking Object.keys(REASON)
so a new reason stays three coordinated changes.

Security axis: sanitizeForReport covered every violation field but not
the baseline-load error path, which embeds raw JSON.stringify output --
that escapes nothing above 0x1f, so bidi and C1 controls reached CI logs
unfiltered. Routed through the sanitizer at the output seam.

Security axis: the scan-ignore marker accepted `#0` and a bare
`http://`. Tightened to a positive issue number and a URL with a host.
This diverges deliberately from the sibling in
tests/commit-files-pathspec.test.cjs, whose looser form was copied
verbatim; the header now records the divergence.

Security axis: G4 built its FIFO with `mktemp -u`, reserving a name
without creating it. Now created inside a `mktemp -d` directory.

Spec axis: ADR-3409 claimed a ninth site landed after the issue was
filed. git blame disproves it -- all nine predate it; the issue's hand
count missed one. Corrected. The design and test matrix still specified
B9 as a FLAG after implementation reversed it to PASS; both now record
the reversal and why.

Refs #3409

* docs(#3409): add the how-to for resolving unreachable-guard findings

Reference and Explanation are carried by ADR-3409; this is the
task-oriented quadrant CI cannot check for.

The page exists mainly for one thing the lint structurally cannot catch:
both `[ -e "${_ARR[0]}" ]` and `[ ${#_ARR[@]} -gt 0 ]` remove the glob
from the command and therefore both pass, but the count form is correct
only when nullglob is set — and nullglob is usually set in a different
block of the same file. A reference table cannot carry that; a how-to can.

Also documents the reason codes, so a reader can tell "nothing to report"
from "could not look".

No tutorial: this is a gate inside an existing CI loop, not a new entry
point a newcomer starts from.

Refs #3409

* fix(#3409): bring the touched prompt files back under their size gates

The remote run was red on 14 tests, all size/attribution, none of them
the regression suite.

- agents/gsd-planner.md was 194 chars over a 49152 cap enforced by four
  separate tests, each of which says the remedy is extraction, not a bump.
  It had 41 chars of headroom before this branch. Its `## Checkpoint
  Types` section was an unlinked, condensed duplicate of
  references/checkpoints.md, which already carries all three types and
  their XML shapes; the section now points there and keeps the three
  names and percentages inline. Net -969, margin 1010.
- gsd-core/workflows/execute-phase.md sat 2 chars under a comfortable
  margin assertion. Dropped the AUTO_MODE default: the `|| echo "false"`
  it replaced was unreachable, so the value was already sometimes empty
  on next, and its only consumer compares against `true`. Net -16.
  Left plan-phase.md's AUTO_CHAIN default alone -- that file names an
  explicit `false` branch, so empty would match neither branch.
- Acknowledged the seven prompt files that genuinely grew, one specific
  reason each. Five of those paths were already claimed by spent
  fragments identical to next, which blocks a second source naming the
  same path; removed just the colliding key from each, deleting the two
  that this emptied.

Refs #3409

* test(#3409): extract the whole PHASE_REQ_IDS block, not just its first line

G3 failed on the remote runner with '' !== 'TBD'. The test was wrong, not
the workflow.

The shipped contract is now two consecutive lines -- the capture and the
`${PHASE_REQ_IDS:-TBD}` default -- but the helper's `^PREFIX=.*$` regex
returns only the first match, so the test executed half the contract and
correctly observed the empty string. Renamed to extractAssignmentBlockFor
and taught it to consume the contiguous run of lines sharing the prefix.

The assertion is untouched: TBD is the right expectation, and weakening
it to accept the empty string would have reinstated exactly the class
this suite exists to catch -- a check that cannot observe the thing it
is checking.

extractFencedBashAfterAnchor is unaffected: it is fence-delimited rather
than line-anchored, so G1/G2/G4 still capture their full blocks.

Refs #3409

* chore(#3409): drop a spent ack fragment that collided on complete-milestone.md

#3458 landed on next while this branch was in flight and its fragment
claims complete-milestone.md, which this branch also grows. Two ack
sources may never name the same path.

Its entry is spent: the +9163 it explains is already absorbed at base, so
it can no longer clear anything, and the checker's own guidance for spent
entries is to delete them. Removing the key emptied the fragment, so the
file goes too -- an empty one signals nothing.

Refs #3409

* chore(#3409): backfill changeset pr number 3558

* test(#3409): hoist a regex subject out of exec() to clear the injection scan

CI's prompt-injection scan flagged `MARKER_RE.exec('# gsd-scan-ignore: ...')`.
The pattern `exec[[:space:]]*\(["']` is receiver-blind on purpose, so it
catches `require('child_process').exec('...')` -- and the scanner's own
header records that RegExp.prototype.exec is collateral, to be handled by
its allowlist.

Allowlisting the file would blind it to the real exec vector permanently,
so the subject is hoisted into a const instead: same assertion, scanner
left at full strength, no security surface widened.

Refs #3409

---------

Co-authored-by: sim <sim@local>
2026-08-15 21:09:05 -04:00

8.0 KiB

How to resolve unreachable-guard findings

npm run lint:ci failed with unreachable-guard-drift. That guard finds shell in shipped prompt files where a fallback arm cannot run — the command it guards succeeds even in the case the fallback was written for, so the guard is output-identical to the success path and silently does nothing.

This page covers reading the finding, fixing each shape, and the one case where acknowledging is the right answer. For why the invariant exists, see ADR-3409.

Read a finding

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 "")

Each line is file:line, the token that matched, and the offending source line. For a machine-readable form — useful in CI or when scripting a migration — run the guard with --json:

node scripts/lint-unreachable-guard-drift.cjs --json

That emits one object carrying reason, violations, malformed, stale, baselineErrors, and knownCount. The reason is a frozen enum code, so assert on it rather than on the human text.

Reason codes

Tell "nothing to report" apart from "could not look" — they are different outcomes and only one of them is good news.

reason Exit Meaning
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_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

# BROKEN — the fallback can never fire
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:

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.

Fix — test the value, not the exit code:

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.

When the safe direction is "do nothing", compare against the literal instead and add no default at all:

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.

Shape B — a glob whose command succeeds on zero matches

Under shopt -s nullglob an unmatched glob expands to zero operands, so the command still succeeds:

cat .planning/phases/*-*/*-SUMMARY.md          # zero operands -> cat reads STDIN and BLOCKS
ls -d .planning/phases/999* || echo "none"     # zero operands -> ls lists the CWD, exits 0, message never prints

The cat form is the worse one: it hangs rather than failing.

Fix — collect into an array and test for existence:

_SUMMARIES=( .planning/phases/*-*/*-SUMMARY.md )
if [ -e "${_SUMMARIES[0]}" ]; then cat "${_SUMMARIES[@]}"; fi

Use -e, not a count — the lint cannot catch this for you

This is the one thing on this page you must get right unaided, because both forms pass the lint (each removes the glob from the command):

if [ ${#_SUMMARIES[@]} -gt 0 ]; then   # correct ONLY if nullglob is set
if [ -e "${_SUMMARIES[0]}" ]; then     # correct either way

Without nullglob, an unmatched glob leaves the literal pattern as a single element, so the count is 1 and the count guard passes wrongly. Measured:

${#_A[@]} -gt 0 -e "${_A[0]}"
no nullglob, no match passes (wrong) skips
nullglob set, no match skips skips

nullglob is frequently set in a different fenced block of the same file, so you cannot tell from the line you are editing. Use -e and stop having to know.

gsd-core/workflows/review.md keeps count guards because that block sets nullglob two lines above them — correct in context, not a template to copy.

What does not fire

Deliberately, so the guard stays worth reading:

  • ls foo/*.md 2>/dev/null | head -1 and X=$(ls -d …) — stdout is consumed, not the exit code. Not a guard.
  • ls foo/*.md 2>/dev/null || true — suppressing a failure; there is no fallback value to defeat.
  • gsd_run query config-get <key> … || echo "default" — config-get genuinely exits 1 on a missing key, so its || works. Verify with node gsd-core/bin/gsd-tools.cjs query config-get no.such.key; echo $?.
  • for f in dir/*.md; do — the construct nullglob exists to make correct.

Acknowledge a finding you cannot fix yet

Only when the fix belongs to another tracked issue. Run:

node scripts/lint-unreachable-guard-drift.cjs --update

This rewrites scripts/baselines/unreachable-guard-drift-baseline.json, keyed on (file, trimmed text) with a per-pair count — never line numbers, so unrelated edits do not disturb it. The baseline is shrink-only: a stale entry fails as loudly as a new one, so it cannot quietly become a parking lot.

The guard ships with a zero-entry baseline. Growing it is a real decision, not a way to make the build green — if you find yourself running --update because the finding is inconvenient, you are turning a gate back into a suggestion. Fix the site instead.

Exempt a deliberate counter-example

Documentation that shows the anti-pattern is byte-identical to a regression, so it must declare itself, on the offending line:

cat .planning/phases/*/*-SUMMARY.md   # gsd-scan-ignore: #3409 counter-example for the docs

The reason must name an issue (#123) or an http(s):// URL. A free-text, empty, or whitespace-only reason reports fail_malformed_marker — a distinct error, so you are told which of the two problems you have. #0 and a bare http:// are rejected: an exemption with no real ledger never gets revisited.

There is no file allowlist and there will not be one — an allowlist points at the file most likely to grow the next copy.