diff --git a/.changeset/curious-goats-zip.md b/.changeset/curious-goats-zip.md new file mode 100644 index 000000000..48a6d154a --- /dev/null +++ b/.changeset/curious-goats-zip.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3558 +--- +**Three shell guards that could never fire now do** — the planner's Walking Skeleton mode never activated on any project, phase planning recorded an empty requirement list instead of `TBD`, and completing a milestone with no phase summaries could hang instead of finishing. Each read a value that came back empty on success, so the fallback written to handle it was unreachable. (#3409) diff --git a/agents/gsd-phase-researcher.md b/agents/gsd-phase-researcher.md index 21acc309a..6285266cb 100644 --- a/agents/gsd-phase-researcher.md +++ b/agents/gsd-phase-researcher.md @@ -545,7 +545,8 @@ Also read `.planning/config.json` — include Validation Architecture section in Then read CONTEXT.md if exists: ```bash -cat "$phase_dir"/*-CONTEXT.md 2>/dev/null +_CTX=( "$phase_dir"/*-CONTEXT.md ) +if [ -e "${_CTX[0]}" ]; then cat "${_CTX[@]}"; fi ``` **If CONTEXT.md exists**, it constrains research: diff --git a/agents/gsd-planner.md b/agents/gsd-planner.md index 631eef3f0..f48bcd47f 100644 --- a/agents/gsd-planner.md +++ b/agents/gsd-planner.md @@ -485,47 +485,7 @@ See @~/.claude/gsd-core/references/planner-guidance.md for a worked example and ## Checkpoint Types -**checkpoint:human-verify (90% of checkpoints)** -Human confirms Claude's automated work works correctly. - -Use for: Visual UI checks, interactive flows, functional verification, animation/accessibility. - -```xml - - [What Claude automated] - - [Exact steps to test - URLs, commands, expected behavior] - - Type "approved" or describe issues - -``` - -**checkpoint:decision (9% of checkpoints)** -Human makes implementation choice affecting direction. - -Use for: Technology selection, architecture decisions, design choices. - -```xml - - [What's being decided] - [Why this matters] - - - - Select: option-a, option-b, or ... - -``` - -**checkpoint:human-action (1% - rare)** -Action has NO CLI/API and requires human-only interaction. - -Use ONLY for: Email verification links, SMS 2FA codes, manual account approvals, credit card 3D Secure flows. - -Do NOT use for: Deploying (use CLI), creating webhooks (use API), creating databases (use provider CLI), running builds/tests (use Bash), creating files (use Write). +Three types: **checkpoint:human-verify (90%)**, **checkpoint:decision (9%)**, **checkpoint:human-action (1% - rare)**. Full "use for" criteria and XML templates for each: @~/.claude/gsd-core/references/checkpoints.md ## Authentication Gates @@ -696,7 +656,8 @@ Select top 2-4 phases. Skip phases with no relevance signal. **Step 3 — Read full SUMMARYs for selected phases:** ```bash -cat .planning/phases/{selected-phase}/*-SUMMARY.md +_SUMMARIES=( .planning/phases/{selected-phase}/*-SUMMARY.md ) +if [ -e "${_SUMMARIES[0]}" ]; then cat "${_SUMMARIES[@]}"; fi ``` From full SUMMARYs extract: @@ -733,9 +694,12 @@ If `features.global_learnings` is `true`: run `gsd_run query learnings.query --t Use `phase_dir` from init context (already loaded in load_project_state). ```bash -cat "$phase_dir"/*-CONTEXT.md 2>/dev/null # From /gsd:discuss-phase -cat "$phase_dir"/*-RESEARCH.md 2>/dev/null # Research output -cat "$phase_dir"/*-DISCOVERY.md 2>/dev/null # From mandatory discovery +_CTX=( "$phase_dir"/*-CONTEXT.md ) +if [ -e "${_CTX[0]}" ]; then cat "${_CTX[@]}"; fi # From /gsd:discuss-phase +_RESEARCH=( "$phase_dir"/*-RESEARCH.md ) +if [ -e "${_RESEARCH[0]}" ]; then cat "${_RESEARCH[@]}"; fi # Research output +_DISCOVERY=( "$phase_dir"/*-DISCOVERY.md ) +if [ -e "${_DISCOVERY[0]}" ]; then cat "${_DISCOVERY[@]}"; fi # From mandatory discovery ``` **If CONTEXT.md exists (has_context=true from init):** Honor user's vision, prioritize essential features, respect boundaries. Locked decisions — do not revisit. diff --git a/agents/gsd-verifier.md b/agents/gsd-verifier.md index 909227a61..9d4d998a7 100644 --- a/agents/gsd-verifier.md +++ b/agents/gsd-verifier.md @@ -82,7 +82,8 @@ At verification decision points, reference calibration examples: ## Step 0: Check for Previous Verification ```bash -cat "$PHASE_DIR"/*-VERIFICATION.md 2>/dev/null +_VERIF=( "$PHASE_DIR"/*-VERIFICATION.md ) +if [ -e "${_VERIF[0]}" ]; then cat "${_VERIF[@]}"; fi ``` **If previous verification exists with `gaps:` section → RE-VERIFICATION MODE:** diff --git a/commands/gsd/review-backlog.md b/commands/gsd/review-backlog.md index 213584199..c72a457eb 100644 --- a/commands/gsd/review-backlog.md +++ b/commands/gsd/review-backlog.md @@ -18,7 +18,8 @@ milestone sequence or remove stale entries. 1. **List backlog items:** ```bash - ls -d .planning/phases/999* 2>/dev/null || echo "No backlog items found" + _BACKLOG=( .planning/phases/999* ) + if [ -e "${_BACKLOG[0]}" ]; then ls -d "${_BACKLOG[@]}"; else echo "No backlog items found"; fi ``` 2. **Read ROADMAP.md** and extract all 999.x phase entries: diff --git a/docs/README.md b/docs/README.md index 58a188bbd..e26cd532f 100644 --- a/docs/README.md +++ b/docs/README.md @@ -23,6 +23,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [Discuss a phase](how-to/discuss-a-phase.md) — capture implementation decisions before planning begins - [Resolve edge-coverage findings](how-to/resolve-edge-coverage-findings.md) — turn the spec phase's surfaced domain-boundary edges into covered, dismissed, or backstopped spec decisions - [Resolve prohibition findings](how-to/resolve-prohibition-findings.md) — turn the spec phase's surfaced must-NOT constraints into resolved, dismissed, or deferred spec decisions +- [Resolve unreachable-guard findings](how-to/resolve-unreachable-guard-findings.md) — fix shell guards whose fallback arm cannot run, and tell "nothing to report" apart from "could not look" - [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 - [Plan a phase](how-to/plan-a-phase.md) — run research, decompose work, and verify plan quality - [Execute a phase](how-to/execute-a-phase.md) — run plans in parallel waves with fresh-context subagents diff --git a/docs/adr/3409-unreachable-shell-guard-arms.md b/docs/adr/3409-unreachable-shell-guard-arms.md new file mode 100644 index 000000000..bd903381a --- /dev/null +++ b/docs/adr/3409-unreachable-shell-guard-arms.md @@ -0,0 +1,93 @@ +# ADR-3409: Shell Guards Must Observe Their Own Failure Arm + +- **Status:** Accepted +- **Date:** 2026-08-15 +- **Issue:** [#3409](https://github.com/open-gsd/gsd-core/issues/3409) (`epic` + `approved-enhancement`), which is why this ADR carries its number. +- **Supersedes:** nothing +- **Relationship to prior work:** applies [ADR-3180](3180-planning-semantic-model-single-owner.md)'s Decision 4 mechanism — whole-repo discovery, exemption by documented reason rather than file allowlist, and the shrink-only ratchet of 4(e) — to a **different invariant**, one layer up from the derivation-counting `lint-planning-prompt-drift.cjs` owns. Distinct from [#3473](https://github.com/open-gsd/gsd-core/issues/3473), which owns the upstream fix described in Decision 7 below; this ADR deliberately does not attempt it. + +Symbol names and shell idioms are the durable anchors throughout. Line references, where given, are as of `next` @ `bc9a22868` and will drift. + +## Context + +ADR-3180 established, for the `src/` layer, that **a failure path returning a plausible default is the core defect class**. The identical class is alive one layer up, in the shell snippets embedded in `gsd-core/workflows/*.md`, `commands/`, `agents/` and `skills/` — but with a distinct mechanism that no `.cts`-scoped guard can see, because the code is markdown. + +### The measured root fact + +`gsd-tools.cjs`'s `--pick ` extractor coerces a missing or absent field to the empty string and exits **0**. `gsd_run` passes the child exit code through unchanged. Measured against the real CLI: + +| Invocation | exit | stdout | +|---|---|---| +| `query phases.list --pick summaries_total` | 0 | *(empty)* | +| `query phases.list --pick totally_bogus_field_xyz` | 0 | *(empty)* | +| `query init.plan-phase 01 --pick phase_req_ids` | 0 | *(empty)* | +| `query roadmap.analyze --pick next_phase` | 0 | *(empty)* | +| `query config-get ` *(no `--pick`)* | **1** | *(empty)* | +| `query bogus.verb --pick x` | 1 | *(empty)* | + +So in `X=$(gsd_run query V --pick F 2>/dev/null || echo D)` the `|| echo D` arm can fire **only on a typo in the verb name** — never on the field absence it was written to handle. The guard cannot observe its own failure. + +The same shape arrives by a second route. Under `shopt -s nullglob` an unmatched glob expands to **zero operands**, so a command that consumes the glob still *succeeds*: + +- `ls || echo ""` — `ls` runs with no operands, lists the current directory, exits 0. The message never prints; the user sees a directory listing instead. +- `cat ` — `cat` with no operands reads **stdin** and blocks. Measured: killed at 3s (rc=137) with a blocked stdin; merely `rc=1` with `nullglob` unset. + +Two mechanisms, one shape: **a fallback arm defeated by a legitimate success-on-empty.** + +### Why the existing guards cannot see it + +`scripts/lint-planning-prompt-drift.cjs` (#3218, ADR-3180 Phase 8) is scoped to plan/summary **counting** re-derivation — a set glob plus `wc -l`/`grep -c` on one line — and its header explicitly excludes bare `ls`/`cat` globbing. `lint-portable-timeout.cjs` covers timeout portability only. Nothing in `scripts/` targets the guard idioms themselves — and the count was already wrong when the issue was written: it reports 8 `--pick … || echo` sites, but `origin/next` carried **9**, each last touched between 2026-06-02 and 2026-07-26, well before the issue was filed on 2026-08-13. No new site landed mid-flight; a hand count simply missed one. That is the argument for a guard rather than a sweep, stated more plainly than the miscount-over-time story it first appeared to be. + +### What discovery found + +The audit this ADR mandates (Decision 5) surfaced more than the issue knew about — 20 sites in total, none previously tracked beyond the two named bugs: + +| Shape | Sites | Note | +|---|---|---| +| `--pick … \|\| echo ` | 9 | 2 causing live wrong behavior ([#3365](https://github.com/open-gsd/gsd-core/issues/3365); `phase_req_ids` never reaching its `TBD` sentinel); 7 dead-but-misleading | +| bare `cat ` (stdin hang) | 8 | in files [#3300](https://github.com/open-gsd/gsd-core/issues/3300) never touched | +| `ls \|\| echo ""` | 3 | message unreachable; one in a **generated** `skills/` artifact | +| `if`/`while ls ` | 0 | the #3300 shape, now extinct | +| informational `ls ` (stdout consumed) | 97 | **not this class** — see Decision 2 | + +## Decision + +**1. The invariant.** A guard in shipped prompt-layer shell must be able to observe the condition it guards against. A fallback arm that a legitimate success-on-empty defeats is a defect, not a style preference — it is output-identical to the success case, which is precisely what ADR-3180 exists to eliminate. + +**2. Detection keys on a documented contract, never a heuristic.** `--pick` is Detector A's discriminator because the extractor's "missing field → empty string, exit 0" behavior *is* a contract. `exit-code-consumed-by-a-real-fallback` is Detector B's, for the same reason. The consequence is load-bearing in both directions: + +- `config-get … || echo "default"` **stays** — it genuinely exits 1 on a missing key, measured. This is ~132 of the 141 `$(… || echo …)` lines in the prompt layer. +- informational `ls ` whose **stdout** is consumed **stays** — 97 sites. It is not a guard; nothing about it has a failure arm to be unreachable. +- `ls || true` **stays** — suppressing a failure is not a guard, and there is no fallback value to defeat. + +A rule keyed on "invokes `gsd_run` and has `|| echo`" was written, measured at 111 matches, and **rejected**: it would have reddened CI on ~132 legitimate lines. A rule keyed on "`cat`/`ls` with any glob operand" was written, measured at a 106-entry baseline including 8 lines of pure markdown prose, and **rejected** for the reason in Decision 4. + +**3. The guard consumes the shared scanner; it does not copy it.** `scripts/lint-unreachable-guard-drift.cjs` uses `scripts/lib/drift-scan.cjs` for the tree walk, root confinement, symlink handling and report sanitizer. ADR-3180 Decision 4 already rejected "let the new drift guard copy Phase 1's tree-walk." This repo now carries 46 `scripts/lint-*.cjs` guards; that count is a standing Greenspun signal, and the shared module is the answer to it. A guard that copies the scanner turns the family into the ad hoc engine. No off-the-shelf linter fits the target — ESLint parses JS ASTs, `shellcheck` parses real shell, and this input is shell fragments inside markdown fences carrying `${}` prompt interpolation that is not valid shell. + +**4. The ratchet's target is zero, and a baseline is not a parking lot.** The shrink-only mechanism of ADR-3180 Decision 4(e) is adopted verbatim: keyed on (file, trimmed text) never line number, each entry carrying a `count` and naming the issue that removes it, with a stale entry failing as loudly as a fresh one. But the ratchet exists for a site whose owner is *another epic* — not as a place to park a detector that over-fires. **A baseline large enough to skim past is a failed detector, not an acknowledged debt.** This guard ships with an empty baseline: every site it can find is fixed. + +**5. `nullglob` is audited once, centrally — including in the fix.** Whether a glob-consuming command is safe depends on a shell option that may be set in a *different fenced block* of the same file, which the author of any one line cannot see. Two consequences: + +- The detector flags the shape regardless of whether `nullglob` is visibly set in that file. Locally-undecidable means decide conservatively. +- **The remedy is `[ -e "${_ARR[0]}" ]`, not `[ ${#_ARR[@]} -gt 0 ]`.** The count form is correct *only* when `nullglob` is set: without it the array holds the unmatched literal pattern, so the count is 1 and the guard passes wrongly. Measured both ways. `gsd-core/workflows/review.md` keeps its count guards because that block sets `nullglob` two lines above them; every site fixed under this ADR uses `-e`, because six of the seven files involved never set it at all. + +**6. Exemption is per-line and must name an owner.** A deliberate counter-example — documentation showing the anti-pattern — is byte-identical to a regression, so only a declaration can separate them. The marker is `# gsd-scan-ignore: ` on the violating line, where the reason must name an issue (`#NNN`) or an `http(s)://` URL, matching the precedent in `tests/commit-files-pathspec.test.cjs` and the sibling `allow-test-rule:` marker of [ADR-456](456-test-rigor-architecture.md). A malformed reason is reported as a *distinct* malformed-declaration error rather than silently exempting. **File allowlists remain forbidden** (ADR-3180 Decision 4(a)) — an allowlist points at the file most likely to grow the next copy. + +**7. The upstream contract fix is explicitly out of scope.** The true single-owner cure is at `--pick` itself: it should signal absence rather than emitting `''` at exit 0, so that no caller has to guard for it. That is a CRITICAL-blast-radius change across 111+ call sites and it belongs to [#3473](https://github.com/open-gsd/gsd-core/issues/3473) ("enforcement by construction — one owner per invariant … failure returns"). This ADR governs the shell layer and the guard; it does not re-litigate the CLI contract. Until #3473 lands, every new `--pick` caller remains one careless line from this class — which is exactly what the guard makes visible in review. + +## Consequences + +**Downstream callers can rely on:** + +- A `--pick` read in shipped prompt shell is followed by an explicit empty test, never by an exit-code fallback. The idiom is a two-line `X=$(…)` / `X="${X:-default}"`, or a direct comparison against the expected literal where the safe direction is "do not act" (the Walking Skeleton gate fires only on a literal `"0"`, so an unanswerable query leaves it off rather than firing unconditionally). +- A glob-consuming `cat`/`ls` in shipped prompt shell is guarded by `[ -e "${_ARR[0]}" ]` and therefore behaves identically whether or not `nullglob` is set. +- `npm run lint:ci` fails on a new instance of either shape. + +**Costs accepted:** + +- **A cross-line split defeats Detector A** — `--pick` on one line, `|| echo` on the next — as does `|| printf`. Same per-line-scan tradeoff the two sibling guards document; left to code review. +- **Detector B is conservative by construction.** It flags a glob operand even where `nullglob` is provably unset in that file, because the option is not locally decidable. +- **The lint cannot distinguish the correct remedy from a subtly wrong one.** Both `[ -e "${_ARR[0]}" ]` and `[ ${#_ARR[@]} -gt 0 ]` remove the glob from the command, so both pass. Decision 5's rule is therefore documented in `docs/how-to/resolve-unreachable-guard-findings.md` rather than enforced — a reference table cannot carry it. +- **One more guard to run in `lint:ci`**, and one more baseline file that a maintainer must regenerate with `--update` after a migration. + +**Explicitly not changed:** the 97 informational `ls ` sites, the ~15 `ls || true` sites, and the ~132 `config-get … || echo` defaults. Each was measured, and none carries an unreachable failure arm. diff --git a/docs/adr/README.md b/docs/adr/README.md index 026e7da50..a43e6c2c6 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -253,6 +253,7 @@ These govern the system as it stands. Cite these. | [ADR-3180](3180-planning-semantic-model-single-owner.md) | Planning Semantic Model — Single Owner per Derivation | Accepted | — | | [ADR-3212](3212-lexical-seam-consolidation.md) | The Lexical Seam — Safe Pattern Construction, Line-Terminator Normalization, and Tokenizer-First Stateful Grammars | Accepted | — | | [ADR-3408](3408-state-write-path-preservation.md) | STATE.md Write Path — One Declared Policy, One Write Seam | Accepted | — | +| [ADR-3409](3409-unreachable-shell-guard-arms.md) | Shell Guards Must Observe Their Own Failure Arm | Accepted | — | | [ADR-3660](3660-runtime-artifact-layout-module.md) | Runtime Artifact Layout Module owns per-runtime artifact placement | Accepted | [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) | ### Proposed diff --git a/docs/how-to/resolve-unreachable-guard-findings.md b/docs/how-to/resolve-unreachable-guard-findings.md new file mode 100644 index 000000000..942e0ae40 --- /dev/null +++ b/docs/how-to/resolve-unreachable-guard-findings.md @@ -0,0 +1,188 @@ +# 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](../adr/3409-unreachable-shell-guard-arms.md). + +## 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`: + +```bash +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 + +```bash +# 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: + +```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`. + +**Fix — test the value, not the exit code:** + +```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. + +**When the safe direction is "do nothing", compare against the literal instead** +and add no default at all: + +```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. + +## 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: + +```bash +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:** + +```bash +_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): + +```bash +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 … || 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: + +```bash +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: + +```bash +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. + +## Related + +- [ADR-3409](../adr/3409-unreachable-shell-guard-arms.md) — the invariant, the measurements behind both detectors, and the alternatives rejected +- [ADR-3180](../adr/3180-planning-semantic-model-single-owner.md) — the ratchet and whole-repo-discovery mechanism this guard reuses +- [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 diff --git a/gsd-core/workflows/complete-milestone.md b/gsd-core/workflows/complete-milestone.md index 0a90e8543..205a0495e 100644 --- a/gsd-core/workflows/complete-milestone.md +++ b/gsd-core/workflows/complete-milestone.md @@ -366,7 +366,8 @@ Full PROJECT.md evolution review at milestone completion. Read all phase summaries: ```bash -cat .planning/phases/*-*/*-SUMMARY.md +_SUMMARIES=( .planning/phases/*-*/*-SUMMARY.md ) +if [ -e "${_SUMMARIES[0]}" ]; then cat "${_SUMMARIES[@]}"; fi ``` **Full review checklist:** diff --git a/gsd-core/workflows/discuss-phase-assumptions.md b/gsd-core/workflows/discuss-phase-assumptions.md index 1242f931a..3a1d25b01 100644 --- a/gsd-core/workflows/discuss-phase-assumptions.md +++ b/gsd-core/workflows/discuss-phase-assumptions.md @@ -642,7 +642,8 @@ Check for auto-advance trigger: ``` 3. Read consolidated auto-mode (`active` = chain flag OR user preference): ```bash - AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null || echo "false") + AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null) + AUTO_MODE="${AUTO_MODE:-false}" ``` **If `--auto` flag present AND `AUTO_MODE` is not true:** diff --git a/gsd-core/workflows/discuss-phase/modes/chain.md b/gsd-core/workflows/discuss-phase/modes/chain.md index bd5aa702b..4463a4fa4 100644 --- a/gsd-core/workflows/discuss-phase/modes/chain.md +++ b/gsd-core/workflows/discuss-phase/modes/chain.md @@ -33,7 +33,8 @@ _GSD_SHIM_NAME="gsd-tools.cjs"; _GSD_RUNTIME_ROOT="${RUNTIME_DIR:-$(git rev-pars 3. Read consolidated auto-mode (`active` = chain flag OR user preference): ```bash - AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null || echo "false") + AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null) + AUTO_MODE="${AUTO_MODE:-false}" ``` 4. **If `--auto` or `--chain` flag present AND `AUTO_MODE` is not true:** diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 59fc7e81a..78907ef25 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -1133,7 +1133,7 @@ Plans with `autonomous: false` require user interaction. **Auto-mode checkpoint handling:** Read auto-advance config (chain flag OR user preference — same boolean as `check.auto-mode`): ```bash -AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null || echo "false") +AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null) ``` When executor returns a checkpoint AND `AUTO_MODE` is `true`: diff --git a/gsd-core/workflows/plan-phase.md b/gsd-core/workflows/plan-phase.md index 82dc0576d..c3e212e71 100644 --- a/gsd-core/workflows/plan-phase.md +++ b/gsd-core/workflows/plan-phase.md @@ -159,7 +159,7 @@ Defer the `phase.mvp-mode` query until `PHASE` is finalized (after explicit argu ```bash WALKING_SKELETON=false if [ "$MVP_MODE" = "true" ] && [ "$padded_phase" = "01" ]; then - PRIOR_SUMMARIES=$(gsd_run query phases.list --pick summaries_total 2>/dev/null || echo "0") + PRIOR_SUMMARIES=$(gsd_run query phases.list --type summaries --pick count 2>/dev/null) if [ "$PRIOR_SUMMARIES" = "0" ]; then WALKING_SKELETON=true; fi fi ``` @@ -491,7 +491,8 @@ Display: `Using UI design contract: ${UI_SPEC_PATH}`. Continue to step 6. Read the ephemeral auto-chain flag: ```bash -AUTO_CHAIN=$(gsd_run query check auto-mode --pick auto_chain_active 2>/dev/null || echo "false") +AUTO_CHAIN=$(gsd_run query check auto-mode --pick auto_chain_active 2>/dev/null) +AUTO_CHAIN="${AUTO_CHAIN:-false}" ``` **Branch 5 — `AUTO_CHAIN` is `true` (pipeline / `--auto`):** Fire each active UI **step** hook — runs independently of whether a gate is active (covers `{ui_phase:true,ui_safety_gate:false}`). For each entry in `activeHooks` (in array order) where `kind == "step"` and `ref.skill` is set: @@ -1376,7 +1377,8 @@ Proactive, non-blocking coverage report gated on `workflow.post_planning_gaps` ```bash PLAN_POST_HOOKS_JSON=$(gsd_run loop render-hooks plan:post --raw) -PHASE_REQ_IDS=$(gsd_run query init.plan-phase "$PHASE" --pick phase_req_ids 2>/dev/null || echo TBD) +PHASE_REQ_IDS=$(gsd_run query init.plan-phase "$PHASE" --pick phase_req_ids 2>/dev/null) +PHASE_REQ_IDS="${PHASE_REQ_IDS:-TBD}" ``` Read the `activeHooks` array from `PLAN_POST_HOOKS_JSON` in-context. If the diff --git a/gsd-core/workflows/session-report.md b/gsd-core/workflows/session-report.md index 29d6d7724..4676f2891 100644 --- a/gsd-core/workflows/session-report.md +++ b/gsd-core/workflows/session-report.md @@ -34,7 +34,8 @@ Read `.planning/ROADMAP.md` to get milestone name and goals. Check for existing reports: ```bash -ls -la .planning/reports/SESSION_REPORT*.md 2>/dev/null || echo "No previous reports" +_REPORTS=( .planning/reports/SESSION_REPORT*.md ) +if [ -e "${_REPORTS[0]}" ]; then ls -la "${_REPORTS[@]}"; else echo "No previous reports"; fi ``` diff --git a/gsd-core/workflows/ship.md b/gsd-core/workflows/ship.md index d1d6b2c12..91b9f9f05 100644 --- a/gsd-core/workflows/ship.md +++ b/gsd-core/workflows/ship.md @@ -55,14 +55,14 @@ Verify the work is ready to ship: # The gate decides on ONE read. --pick takes a single field, so the two # human-facing fields are read only on the blocking path below — never on the # passing path — rather than issuing three queries up front (#2589). - STATUS=$(gsd_run query verification.status "${PHASE_DIR}" --pick status 2>/dev/null || echo "") + STATUS=$(gsd_run query verification.status "${PHASE_DIR}" --pick status 2>/dev/null) ``` Only `passed` may ship. If `$STATUS` is `passed`, verification is complete — continue to the next preflight check; do not read any further verification field. Any other value (including `gaps_found`, `human_needed`, `missing`, and `unknown`) blocks with `PHASE_VERIFICATION_INCOMPLETE`. Only then, read the two message fields: ```bash - NEXT_ACTION=$(gsd_run query verification.status "${PHASE_DIR}" --pick next_action 2>/dev/null || echo "") - NEXT_COMMAND=$(gsd_run query verification.status "${PHASE_DIR}" --pick next_command 2>/dev/null || echo "") + NEXT_ACTION=$(gsd_run query verification.status "${PHASE_DIR}" --pick next_action 2>/dev/null) + NEXT_COMMAND=$(gsd_run query verification.status "${PHASE_DIR}" --pick next_command 2>/dev/null) ``` Present `$NEXT_ACTION` to the user and, when `$NEXT_COMMAND` is non-empty, show it as the command to run next. These two are message text only — the block/allow decision has already been made from `$STATUS`, so a concurrent write between the reads cannot change the gate's verdict. The query already handles missing files and unexpected values, so no per-status arm is needed. diff --git a/gsd-core/workflows/transition.md b/gsd-core/workflows/transition.md index 15d6eaad0..afcc7e95c 100644 --- a/gsd-core/workflows/transition.md +++ b/gsd-core/workflows/transition.md @@ -228,7 +228,8 @@ Evolve PROJECT.md to reflect learnings from completed phase. **Read phase summaries:** ```bash -cat .planning/phases/XX-current/*-SUMMARY.md +_SUMMARIES=( .planning/phases/XX-current/*-SUMMARY.md ) +if [ -e "${_SUMMARIES[0]}" ]; then cat "${_SUMMARIES[@]}"; fi ``` **Assess requirement changes:** diff --git a/package.json b/package.json index cb568dfac..1543591d3 100644 --- a/package.json +++ b/package.json @@ -118,7 +118,7 @@ "lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs", "lint:frontmatter-scalar-broad-grep": "node scripts/lint-frontmatter-scalar-broad-grep.cjs", "lint:removed-but-needed": "node scripts/lint-removed-but-needed.cjs", - "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs && node scripts/lint-vendored-deps.cjs", + "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-unreachable-guard-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs && node scripts/lint-vendored-deps.cjs", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", "lint:descriptions": "node scripts/lint-descriptions.cjs", diff --git a/scripts/baselines/unreachable-guard-drift-baseline.json b/scripts/baselines/unreachable-guard-drift-baseline.json new file mode 100644 index 000000000..526f578a9 --- /dev/null +++ b/scripts/baselines/unreachable-guard-drift-baseline.json @@ -0,0 +1,4 @@ +{ + "$comment": "#3409 unreachable-shell-guard ratchet. See scripts/lint-unreachable-guard-drift.cjs. SHRINK-ONLY: entries are removed as sites migrate off the unreachable-arm shape; new or changed entries fail lint:ci. `count` is the number of byte-identical (file, text) occurrences acknowledged at this site — a run producing fewer fails as a partial migration, more fails as an unacknowledged new copy.", + "entries": [] +} diff --git a/scripts/lint-unreachable-guard-drift.cjs b/scripts/lint-unreachable-guard-drift.cjs new file mode 100644 index 000000000..01cb88ffc --- /dev/null +++ b/scripts/lint-unreachable-guard-drift.cjs @@ -0,0 +1,843 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Prompt-layer drift guard for #3409 — shell guards that cannot observe + * their own failure arm. + * + * Design: .gsd/phase/feat-3409-unreachable-shell-guard-lint/40-design.md + * Test matrix: .gsd/phase/feat-3409-unreachable-shell-guard-lint/50-test-matrix.md + * + * `gsd-tools.cjs`'s `--pick ` extractor coerces a missing/absent + * field to the empty string and exits **0** (probe-confirmed in + * 40-design.md). So in `$(gsd_run query V --pick F 2>/dev/null || echo D)` + * the `|| echo D` arm can fire only on a typo in the verb name — never on + * the field absence it was written to handle. Three shipped shell guards + * silently relied on that unreachable arm (#3365's Walking Skeleton gate, + * `PHASE_REQ_IDS`, and `complete-milestone.md`'s bare `cat `, all + * fixed alongside this guard — see `tests/unreachable-shell-guard.test.cjs`, + * which this file does not touch). + * + * TWO detectors, each narrow by design (mirroring the sibling drift guards' + * precedent of a small, specific shape rather than a wide heuristic): + * + * Detector A — a line carrying BOTH a `--pick` token AND a `|| echo` + * fallback. `--pick` is the discriminator: a `|| echo` default WITHOUT + * `--pick` (e.g. `config-get k 2>/dev/null || echo "default"`, ~132 of the + * 141 `$(… || echo …)` lines in the prompt layer) genuinely observes a + * nonzero exit code and is left alone — see 40-design.md's Rejected #2 + * ("detect on `gsd_run` + `|| echo`" was tried and reverted for exactly + * this false-positive volume). + * + * Detector B — `cat` or `ls` invoked in COMMAND POSITION with an operand + * containing an unquoted glob metacharacter (`*` or `?`). Both detectors + * are, at bottom, the SAME shape: a fallback/guard arm that a + * success-on-empty case silently defeats. Detector A is `--pick … || + * echo`; Detector B-ii below is `ls … || echo` — the identical + * defect, one level down the stack, with `ls`'s own nullglob-driven + * success-on-empty standing in for `--pick`'s absence-coerced-to-''. + * SCOPED to exactly three fired shapes, per a full measurement across the + * four SCAN_DIRS (measured counts recorded in the PR description; 0 sites + * for B-iii today, by design — see KNOWN LIMITS): + * + * B-i. `cat ` fires UNCONDITIONALLY. This is the stdin-hang + * shape (measured rc=137 at 3s under an unmatched glob + + * nullglob): `cat` reads from stdin the moment it gets zero + * operands, regardless of what — if anything — consumes its own + * exit code. There is no fallback arm to inspect; the hang + * happens before one could run. + * B-ii. `ls ` fires when its exit code feeds a REAL fallback: + * `… || ` where `` is not the no-op `true`/`:`. Under + * nullglob `ls` SUCCEEDS listing the cwd on an unmatched glob, + * so the fallback never runs and the intended message/default is + * silently replaced by a directory listing — exactly Detector + * A's shape, with `ls`'s exit code standing in for `--pick`'s + * stdout. + * B-iii. `ls ` fires at the head of an `if`/`elif`/`while` test — + * the #3300 "existence guard that is always true under + * nullglob" shape the issue names directly: an unmatched glob + * makes `ls` list the CWD instead of erroring, so the guard is + * always true. Zero sites today (the #3300 fix already removed + * them); this arm exists solely so a REINTRODUCED instance of + * the shape does not ship silently. + * + * NOT fired on: + * - `ls … || true` / `… || :` — suppressing a failure is not a + * guard, and there is no fallback VALUE being defeated (the whole + * point of `true`/`:` is "do nothing, either way"). Measured: ~15 + * sites in this tree, all this exact defensive idiom. + * - an `ls ` whose STDOUT is what's consumed (`ls foo/*.md + * 2>/dev/null`, `X=$(ls -d …)`, `ls … | head`) — neither the stdin + * hang nor a defeated fallback nor an always-true guard. Measured: 97 + * sites, explicitly out of this issue's scope ("Explicit non-goal: + * … Only the shapes above move."). + * - markdown prose describing either command, INCLUDING the specific + * shape `` `Bash(cat << 'EOF')` `` (a heredoc operator immediately + * after the command name is never a glob operand — see the heredoc + * guard below, and matrix row B8). + * + * `|| echo ` vs `|| true`/`|| :` is the discriminator for B-ii, + * exactly as `--pick` is Detector A's: both distinguish "a fallback VALUE + * this shape can silently defeat" from "no fallback value exists to + * defeat, so there is nothing here for nullglob's success-on-empty to + * break." + * + * Conservative BY CONSTRUCTION where it still applies (40-design.md's + * B10/B11 and "Law of Leaky Abstractions" section): whether a `nullglob` + * is in effect is not locally decidable from the line alone, so `cat` + * still fires unconditionally (B-i) and `ls`'s two exit-code-consuming + * shapes (B-ii, B-iii) still fire regardless of whether a guard already + * exists nearby. The remedy (an array expansion, or an existence test + * before the read) is correct either way, and array expansions carry no + * `*`/`?` character at all so they are never flagged — the detector does + * not punish its own fix. + * + * Regexes are small, bounded, and non-backtracking BY CONSTRUCTION — + * `npm run lint:ci` runs CodeQL js/redos over this repo, the same + * discipline `lint-planning-prompt-drift.cjs` documents in its own header: + * + * - PICK_RE / ECHO_FALLBACK_RE carry only a single bounded `\s*` + * quantifier each, over a fixed literal — no nesting, nothing to + * backtrack. + * - CAT_LS_COMMAND_RE's alternation is a FIXED, non-overlapping set (a + * handful of literal command-position anchors, then a fixed + * `(cat|ls)`), with one `[ \t]*` quantifier between the anchor and the + * command name — again no nesting. + * - HEREDOC_AFTER_COMMAND_RE and FALLBACK_TOKEN_RE are each a single + * bounded quantifier over a fixed/negated class, same shape as above. + * - The B-ii/B-iii "does this clause carry a glob, and what terminates + * it" question is answered by `scanClauseAfterCommand`, a plain + * LINEAR, single left-to-right character walk — not a regex at all, and + * therefore not a ReDoS surface by construction rather than by + * argument: it inspects each character of the remainder exactly once + * and returns at the first clause-terminating token it finds. + * - MARKER_RE (the escape-marker parser) is two more `\s*` quantifiers + * over fixed literals, then a single trailing `(.*)$` — again one + * quantifier, no nesting. + * + * ESCAPE MARKER. A line carrying `# gsd-scan-ignore: ` is exempt + * ONLY when `` names an issue (`#NNN`, N a positive integer) or an + * `http(s)://` URL with an actual host after the scheme — the repo's + * existing precedent from `tests/commit-files-pathspec.test.cjs` + * (CONTRIBUTING.md, "Every `commit` invocation in shipped content must + * declare `--files`"), STARTING from that precedent's predicate + * (`/#\d+|https?:\/\//`) but DELIBERATELY DIVERGING from it (see + * `ISSUE_REF_RE`'s own comment for exactly what changed and why) rather than + * copying it verbatim. The sibling file still carries the looser, unpatched + * form — this guard's escape hatch is a stricter gate than a commit-message + * pathspec check needs to be, since an accepted reason here silently + * exempts a real violation from ever being reported. A marker whose reason is free text, empty, or + * whitespace-only is reported as a DISTINCT "malformed declaration" error — + * never silently exempted (that would defeat the guard) and never folded + * into the ordinary violation list (that would tell an author who already + * explained themselves that they hadn't, the exact mangle-until-CI-shuts-up + * loop the marker exists to prevent). Simplification versus the sibling + * predicate this mirrors: that guard's marker parser tokenizes the whole + * line to rule out a marker surviving inside quoted argv text (a commit + * MESSAGE quoting the token). This guard's two detectors never process + * commit-message-shaped free text, so a plain `#\s*gsd-scan-ignore:` literal + * match is sufficient here and is not widened to match that guard's + * quote-awareness it has no corresponding hazard for. + * + * RATCHET, not an allowlist. `scripts/baselines/unreachable-guard-drift-baseline.json` + * mirrors `lint-planning-prompt-drift.cjs`'s shrink-only, count-aware + * baseline exactly (see that module's header for the full "COUNT, not + * duplicate rows" rationale) — a recorded `(file, text)` pair acknowledges + * `count` byte-identical occurrences; fewer this run is a PARTIAL migration + * (stale), more is an unacknowledged new copy (fresh), zero is a fully + * migrated pair (stale). Matched on `(file, TRIMMED text)`, never the line + * number, for the same reason: a workflow `.md` file's line numbers churn on + * every unrelated edit. Malformed declarations are NEVER ratchet-eligible — + * they are an authoring mistake in the escape hatch itself, not a + * migration-in-progress, and always hard-fail (40-design.md's Goodhart's Law + * section names "run `--update` and record the violation as acknowledged + * instead of fixing it" as the ratchet's own cheapest gaming path; a + * malformed marker is exactly the shape of a half-hearted attempt at that, + * and it is refused rather than laundered into the baseline). + * + * SHARED TREE-WALK. `scanTree` / `sanitizeForReport` are consumed from + * `scripts/lib/drift-scan.cjs`, NOT reimplemented — ADR-3180 Decision 4 + * explicitly rejected "let the new drift guard copy Phase 1's tree-walk", + * and 40-design.md's Greenspun's Tenth Rule section states the binding + * consequence plainly: "the 46th guard MUST consume `drift-scan.cjs`, not + * copy it." See that module for the `toPosixRel`-equivalent rationale + * (below), the symlink-confinement contract, and the ReDoS-avoidance + * rationale for its own regex-literal reader (unused by this guard's + * regexes, which need no literal tokenizer — shared here only for the walk + * and the report sanitizer). + * + * Surfaces scanned (SCAN_DIRS): `gsd-core/workflows`, `commands`, `agents`, + * `skills` — the prompt-layer markdown that ships to every runtime. + * SCAN_EXT: `.md` only. + * + * KNOWN, ACCEPTED limits (same tradeoffs the sibling guards document): + * - A cross-line split defeats Detector A (`--pick` on one line, `|| echo` + * on the next) — left to code review, per-line textual scan only. + * - `|| printf` and other non-`echo` fallbacks are not detected by + * Detector A — narrow by design; widening is a one-line change if a + * site ever appears. + * - Detector B's command-position anchor set (line start; `;`, `&`, `|`, + * `(`; the keywords `if`/`then`/`elif`/`while`/`do`) is what lets + * `$(cat …)` / `$(ls …)` — the dominant real invocation idiom in this + * tree — reach the glob check through the `(` anchor. The heredoc guard + * (`HEREDOC_AFTER_COMMAND_RE`) is what keeps that same `(` anchor from + * flagging the specific markdown prose shape `` `Bash(cat << 'EOF')` `` + * — measured against the real tree, it eliminates every such occurrence + * (a heredoc operator immediately after the command name is, by + * definition, never a glob operand). A prose sentence that put a real + * `*`/`?`-bearing word directly after `cat`/`ls` with NO heredoc + * operator between them (unobserved in this tree) would still be a + * residual over-flag in the same conservative-by-construction spirit as + * B10/B11 — accepted for the same reason: removing the `(` anchor + * entirely would blind the guard to most of the real `$(cat …)` sites + * it exists to catch, the strictly worse direction (silent false + * negative vs. a visible, ratchet-acknowledgeable false positive). + * - B-iii's `if`/`elif`/`while` head-position check is per-token, not a + * full parse of the conditional's grammar: `if [ -f x ] && ls + * glob; then` (a compound condition where `ls` is not literally the + * first word after `if`) is not reachable through the keyword anchor + * and falls through to B-ii's `||`-fallback check instead, which is the + * right outcome only when a `||`-fallback is present that isn't a + * no-op. A compound `if` condition ending the `ls` clause with `;`/end + * of line and no `||` arm is a genuine, unmeasured (zero observed) + * miss — left to code review, matching the design's stated per-line + * textual-scan tradeoff throughout. + * - The `|| true` / `|| :` no-op carve-out (FALLBACK_TOKEN_RE) inspects + * only the FIRST token after `||`; a real fallback dressed up as `|| + * (true; echo "surprise")` would read as the no-op and miss — no such + * shape exists in this tree today (measured), and widening the + * no-op-detection is a one-line change if one ever appears. + */ + +const fs = require('node:fs'); +const path = require('node:path'); +const driftScan = require('./lib/drift-scan.cjs'); +const { sanitizeForReport, scanTree } = driftScan; + +// ─── Detector A — `--pick` + `|| echo` ──────────────────────────────────── +// +// `--pick` is the discriminator (see module header); `|| echo` is the +// unreachable fallback arm it silently defeats. Both must be present on the +// SAME line for the shape to be the exact unreachable-arm defect #3409 +// fixes — see the module header for why a bare "`gsd_run` + `|| echo`" rule +// was tried and reverted (Rejected #2). +const PICK_RE = /--pick\b/; +const ECHO_FALLBACK_RE = /\|\|\s*echo\b/; + +// ─── Detector B — cat (B-i), ls … || (B-ii), +// or ls at the head of if/elif/while (B-iii) ──────────────────────── +// +// Command-position anchor: start of line, a shell separator/opener +// (`;`, `&`, `|`, `(`), or one of the keywords that precede a command +// (`if`, `then`, `elif`, `while`, `do`) — each followed by optional +// horizontal whitespace and then the literal command name. Group 1 captures +// WHICH anchor matched (`''` for start-of-line, since `^` itself consumes no +// characters; the literal separator char; or the literal keyword) so +// detectGlobOperand can tell a true `if`/`elif`/`while` head position (B-iii) +// apart from `then`/`do`/a bare separator, which do not themselves test the +// following command's exit status. Group 2 captures the command name. A +// FIXED alternation with one `[ \t]*` quantifier between the anchor and the +// command name; no nesting, nothing to backtrack. +const CAT_LS_COMMAND_RE = /(^|[;&|(]|\bif\b|\bthen\b|\belif\b|\bwhile\b|\bdo\b)[ \t]*(cat|ls)\b/; + +// A heredoc operator immediately after the command name (optional +// horizontal whitespace, then `<<`) is never a glob operand — matrix row B8, +// and the mechanism that keeps the markdown-prose shape `` `Bash(cat << +// 'EOF')` `` (whose surrounding `**bold**` carries literal `*` characters +// elsewhere on the line) from ever reaching the glob scan at all. Single +// bounded quantifier, no nesting. +const HEREDOC_AFTER_COMMAND_RE = /^[ \t]*< the clause ends with no chain at all. + * `&&` -> a short-circuit "glob matched, so proceed" chain. + * `||` -> a short-circuit fallback chain; `fallback` is + * everything after the `||` (for isNoopFallback to + * classify). + * a lone `|` -> the clause's STDOUT is piped onward (informational, + * never a hazard shape this detector fires on). + * a lone `&` -> backgrounded; not a chain this detector recognizes. + * end of string -> no chain of any kind. + * `hasGlob` is tracked across the WHOLE walk regardless of where the scan + * stops, since a `*`/`?` can appear anywhere in the operand region before + * the terminator. + */ +function scanClauseAfterCommand(rest) { + let hasGlob = false; + for (let i = 0; i < rest.length; i++) { + const ch = rest[i]; + if (ch === '*' || ch === '?') { hasGlob = true; continue; } + if (ch === ';') return { hasGlob, terminator: ';', fallback: null }; + if (ch === '&' && rest[i + 1] === '&') return { hasGlob, terminator: '&&', fallback: null }; + if (ch === '|' && rest[i + 1] === '|') return { hasGlob, terminator: '||', fallback: rest.slice(i + 2) }; + if (ch === '|') return { hasGlob, terminator: '|', fallback: null }; + if (ch === '&') return { hasGlob, terminator: '&', fallback: null }; + } + return { hasGlob, terminator: null, fallback: null }; +} + +/** + * Pure: does `line` carry one of Detector B's three fired shapes? Returns + * `{ command }` (`cat` or `ls`) or `null`. See the module header for the + * B-i/B-ii/B-iii scoping and what deliberately does NOT fire. + */ +function detectGlobOperand(line) { + const anchor = CAT_LS_COMMAND_RE.exec(line); + if (!anchor) return null; + const anchorToken = anchor[1]; + const command = anchor[2]; + const rest = line.slice(anchor.index + anchor[0].length); + if (HEREDOC_AFTER_COMMAND_RE.test(rest)) return null; + + const { hasGlob, terminator, fallback } = scanClauseAfterCommand(rest); + if (!hasGlob) return null; + + if (command === 'cat') return { command }; // B-i: unconditional. + + // command === 'ls': B-iii (head of a real conditional test) or B-ii (a + // real, non-no-op `||` fallback). Neither a lone `|` (stdout piped + // onward) nor `|| true`/`|| :` nor a bare `;`/end-of-line qualifies. + if (EXIT_TESTING_KEYWORDS.has(anchorToken)) return { command }; + if (terminator === '||' && fallback !== null && !isNoopFallback(fallback)) return { command }; + return null; +} + +// ─── Escape marker ───────────────────────────────────────────────────────── +// +// `# gsd-scan-ignore: `. Two `\s*` quantifiers over fixed literals, +// then a single trailing `(.*)$` — one quantifier, no nesting. Lines are +// split via `/\r?\n/` (see findUnreachableGuardDrift) before this ever runs, +// so `.` never has to reason about a trailing `\r` — the pitfall the CRLF +// coverage in the test matrix (P1-P4) exists to catch. +const MARKER_RE = /#\s*gsd-scan-ignore:\s*(.*)$/; + +// DELIBERATE DIVERGENCE from tests/commit-files-pathspec.test.cjs's own +// `ISSUE_REF_RE` (`/#\d+|https?:\/\//`), which this predicate started as a +// copy of. That sibling form validates FORMAT only, and two shapes satisfy +// it while naming nothing real: +// - `#0` matches `#\d+` (`\d+` allows a leading zero / an all-zero run), +// silently exempting a violation under a reason that names no positive +// issue number. +// - a bare `http://` / `https://` matches `https?:\/\/` with nothing +// after the scheme — no host, so no URL is actually named. +// Both are closed here: an issue ref requires a POSITIVE integer +// (`#[1-9]\d*` — no leading-zero/all-zero match), and a URL requires at +// least one non-whitespace character after the scheme as its host +// (`https?:\/\/[^\s]+`). This guard's escape hatch is a stricter gate than +// the sibling's commit-message pathspec check needs to be — an accepted +// reason here silently exempts a real violation from ever being reported — +// so the sibling is intentionally left at its own, looser form (not edited +// by this change) rather than tightened to match. +// Still one bounded quantifier per alternative, no nesting: `\d*` over a +// fixed digit class, `[^\s]+` over a fixed negated class. Non-backtracking, +// same as every other regex in this module (see the module header's ReDoS +// section). +const ISSUE_REF_RE = /#[1-9]\d*|https?:\/\/[^\s]+/; + +// `scanTree` (scripts/lib/drift-scan.cjs) builds its repo-relative path via +// `path.relative()`, which uses NATIVE separators: on Windows that is +// `gsd-core\workflows\plan-phase.md`, while the committed baseline stores +// POSIX paths. Normalized UNCONDITIONALLY — never gated on +// `process.platform` — for the exact reason `lint-planning-prompt-drift.cjs` +// documents at its own `toPosixRel`: a platform-conditional normalizer is +// itself the bug, since it makes the POSIX path the only tested case +// (PR #3223). +function toPosixRel(relPath) { + return relPath.replace(/\\/g, '/'); +} + +// Prompt-layer markdown that ships to every runtime. +const SCAN_DIRS = ['gsd-core/workflows', 'commands', 'agents', 'skills']; +const SCAN_EXT = new Set(['.md']); + +const BASELINE_REL_PATH = path.join('scripts', 'baselines', 'unreachable-guard-drift-baseline.json'); + +// The tracking issue this guard's own baseline entries are owned by, absent +// a more specific site owner named at `--update` time. #3409 is this +// guard's own issue: any site it finds that this PR does not convert is a +// "one careless line from the same class" per 40-design.md's Postel's Law +// section, tracked here until the upstream `--pick` contract fix (#3473) +// or a per-site conversion lands. +const RATCHET_OWNER_ISSUE = '#3409'; + +/** + * Pure: scan `text` (one file's content) for Detector A / Detector B + * violations and malformed escape-marker declarations. `relPath` is the + * repo-relative path (native separators or POSIX, either accepted) — + * normalized via `toPosixRel` and attached as `file` on every result. + * + * Returns `{ violations, malformed }`: + * - `violations`: `[{ file, line, kind: 'A'|'B', found, text }]` — `text` + * is the TRIMMED source line (the baseline key), `found` names the + * discriminating token (`--pick` for A, `cat`/`ls` for B). + * - `malformed`: `[{ file, line, text, reason }]` — an ATTEMPTED + * `# gsd-scan-ignore:` declaration whose reason names no issue and no + * URL. Checked on EVERY line independent of whether that line also + * matches a detector (a comment-only malformed declaration is still a + * malformed declaration) — never ratchet-eligible. + * + * Lines are split on `/\r?\n/` so CRLF input carries no trailing `\r` into + * either the detector regexes or the baseline key (`text.trim()` would + * catch most of this anyway, per `String.prototype.trim`'s LineTerminator + * handling, but MARKER_RE's trailing `(.*)$` specifically needs the split + * to have already happened — `.` excludes `\r` from its own match). + */ +function findUnreachableGuardDrift(text, relPath) { + const file = toPosixRel(relPath); + const violations = []; + const malformed = []; + const lines = text.split(/\r?\n/); + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + const lineNo = i + 1; + + let exempt = false; + const markerMatch = MARKER_RE.exec(line); + if (markerMatch) { + const reason = markerMatch[1]; + if (ISSUE_REF_RE.test(reason)) { + exempt = true; + } else { + malformed.push({ file, line: lineNo, text: line.trim(), reason: reason.trim() }); + exempt = true; // malformed declarations are reported on their own terms, never as a plain violation too (design A14 / matrix M3-M5) + } + } + if (exempt) continue; + + if (PICK_RE.test(line) && ECHO_FALLBACK_RE.test(line)) { + violations.push({ file, line: lineNo, kind: 'A', found: '--pick', text: line.trim() }); + } + const globInfo = detectGlobOperand(line); + if (globInfo) { + violations.push({ file, line: lineNo, kind: 'B', found: globInfo.command, text: line.trim() }); + } + } + return { violations, malformed }; +} + +/** + * Scan the prompt-layer markdown tree and return every violation and + * malformed declaration, each annotated with the repo-relative file path + * (POSIX-normalized — see `toPosixRel`). + */ +function scanRepo(root) { + const violations = []; + const malformed = []; + scanTree({ + root, + scanDirs: SCAN_DIRS, + scanExt: SCAN_EXT, + onFile(rel, text) { + const found = findUnreachableGuardDrift(text, rel); + violations.push(...found.violations); + malformed.push(...found.malformed); + return []; // scanTree's own accumulator is unused; we track both lists ourselves so its single flat list is never asked to carry two shapes. + }, + }); + return { violations, malformed }; +} + +/** + * Frozen outcome-reason enum. CONTRIBUTING.md's "Prohibited: Raw Text + * Matching on Test Outputs" requires a typed structured surface wherever + * this module produces human-readable text — mirrors + * `gsd-core/bin/verify-reapply-patches.cjs`'s own `REASON` map exactly: + * `main()`'s `--json` mode and every `loadBaseline` per-error object carry + * one of these codes instead of free prose, and tests assert on the code, + * never on the rendered message. Adding a new reason requires updating this + * enum, the `--json` emission/`loadBaseline` call site that produces it, AND + * the test that locks `Object.keys(REASON).sort()` — three coordinated + * changes that keep the code surface from drifting from the test surface. + */ +const REASON = Object.freeze({ + // main() top-level outcomes (non---update and --update paths). + OK_NO_VIOLATIONS: 'ok_no_violations', + OK_BASELINE_UPDATED: 'ok_baseline_updated', + FAIL_FRESH_VIOLATION: 'fail_fresh_violation', + FAIL_STALE_ENTRY: 'fail_stale_entry', + FAIL_MALFORMED_MARKER: 'fail_malformed_marker', + FAIL_BASELINE_LOAD: 'fail_baseline_load', + // loadBaseline per-error outcomes — each a distinct baseline-load failure + // class (mirrors lint-planning-prompt-drift.cjs's loadBaseline validation). + FAIL_BASELINE_MISSING: 'fail_baseline_missing', + FAIL_BASELINE_EMPTY: 'fail_baseline_empty', + FAIL_BASELINE_INVALID_JSON: 'fail_baseline_invalid_json', + FAIL_BASELINE_NOT_OBJECT: 'fail_baseline_not_object', + FAIL_BASELINE_ENTRIES_NOT_ARRAY: 'fail_baseline_entries_not_array', + FAIL_BASELINE_ENTRY_NOT_OBJECT: 'fail_baseline_entry_not_object', + FAIL_BASELINE_ENTRY_FIELD_INVALID: 'fail_baseline_entry_field_invalid', + FAIL_BASELINE_ENTRY_COUNT_INVALID: 'fail_baseline_entry_count_invalid', +}); + +/** + * Read and parse the ratchet baseline. Returns `{ entries, errors }` — + * `entries` is `[]` and `errors` is an array of STRUCTURED error objects + * (`{ reason: REASON.*, message, ... }`) when the file is missing, empty, + * invalid JSON, or malformed. Mirrors `lint-planning-prompt-drift.cjs`'s + * `loadBaseline` validation exactly (same failure classes: missing, empty, + * invalid JSON, non-object JSON — including the `null`/array/scalar cases a + * bare `typeof === 'object'` check would miss — a non-array `entries` + * field, and per-entry validation of `file`/`text`/`count`). `message` is a + * human-readable string for the console formatter only; callers (and + * tests) must key off `reason`, never parse `message`. + */ +function loadBaseline(root) { + const baselinePath = path.join(root, BASELINE_REL_PATH); + if (!fs.existsSync(baselinePath)) { + return { + entries: [], + errors: [{ + reason: REASON.FAIL_BASELINE_MISSING, + message: `${BASELINE_REL_PATH} is missing — run \`node scripts/lint-unreachable-guard-drift.cjs --update\` to generate it`, + }], + }; + } + const raw = fs.readFileSync(baselinePath, 'utf8'); + if (raw.trim() === '') { + return { + entries: [], + errors: [{ reason: REASON.FAIL_BASELINE_EMPTY, message: `${BASELINE_REL_PATH} is present but empty` }], + }; + } + let doc; + try { + doc = JSON.parse(raw); + } catch (err) { + return { + entries: [], + errors: [{ + reason: REASON.FAIL_BASELINE_INVALID_JSON, + message: `${BASELINE_REL_PATH} is not valid JSON: ${err.message}`, + parseError: err.message, + }], + }; + } + if (doc === null || typeof doc !== 'object' || Array.isArray(doc)) { + const gotType = Array.isArray(doc) ? 'array' : typeof doc; + return { + entries: [], + errors: [{ + reason: REASON.FAIL_BASELINE_NOT_OBJECT, + message: `${BASELINE_REL_PATH} must be a JSON object, got ${gotType}`, + gotType, + }], + }; + } + if (!Array.isArray(doc.entries)) { + return { + entries: [], + errors: [{ + reason: REASON.FAIL_BASELINE_ENTRIES_NOT_ARRAY, + message: `${BASELINE_REL_PATH}: "entries" must be an array, got ${JSON.stringify(doc.entries)}`, + entriesValue: doc.entries, + }], + }; + } + const errors = []; + const entries = []; + doc.entries.forEach((entry, i) => { + const where = `${BASELINE_REL_PATH}.entries[${i}]`; + if (entry === null || typeof entry !== 'object' || Array.isArray(entry)) { + errors.push({ + reason: REASON.FAIL_BASELINE_ENTRY_NOT_OBJECT, + message: `${where} must be an object, got ${JSON.stringify(entry)}`, + index: i, + where, + }); + return; + } + if (typeof entry.file !== 'string' || entry.file === '') { + errors.push({ + reason: REASON.FAIL_BASELINE_ENTRY_FIELD_INVALID, + message: `${where}.file must be a non-empty string, got ${JSON.stringify(entry.file)}`, + index: i, + where, + field: 'file', + value: entry.file, + }); + return; + } + if (typeof entry.text !== 'string' || entry.text === '') { + errors.push({ + reason: REASON.FAIL_BASELINE_ENTRY_FIELD_INVALID, + message: `${where}.text must be a non-empty string, got ${JSON.stringify(entry.text)}`, + index: i, + where, + field: 'text', + value: entry.text, + }); + return; + } + // `count` is optional on read (diffAgainstBaseline defaults an absent + // count to 1) but when present must be a positive integer. + if (entry.count !== undefined && !(Number.isInteger(entry.count) && entry.count >= 1)) { + errors.push({ + reason: REASON.FAIL_BASELINE_ENTRY_COUNT_INVALID, + message: `${where}.count must be a positive integer when present, got ${JSON.stringify(entry.count)}`, + index: i, + where, + value: entry.count, + }); + return; + } + entries.push(entry); + }); + return { entries, errors }; +} + +/** + * Diff scanned `violations` against baseline `entries`, matched by the pair + * (`file`, TRIMMED `text`) — never the line number — and COUNT-aware, same + * semantics as `lint-planning-prompt-drift.cjs`'s `diffAgainstBaseline`: + * - `fresh`: violations whose `(file, text)` pair is not in the baseline + * at all, PLUS any occurrences of a KNOWN pair beyond its acknowledged + * `count`. + * - `stale`: baseline entries whose actual occurrence count this run is + * LESS than their acknowledged `count` (zero is the fully-migrated + * case; a positive-but-short count is a PARTIAL migration). + */ +function diffAgainstBaseline(violations, baseline) { + const key = (file, text) => `${file} ${text}`; + + const actualByKey = new Map(); + for (const v of violations) { + const k = key(v.file, v.text); + let vs = actualByKey.get(k); + if (!vs) { vs = []; actualByKey.set(k, vs); } + vs.push(v); + } + + const knownKeys = new Set(baseline.map((e) => key(e.file, e.text))); + + const fresh = []; + const stale = []; + + for (const [k, vs] of actualByKey) { + if (!knownKeys.has(k)) fresh.push(...vs); + } + + for (const entry of baseline) { + const k = key(entry.file, entry.text); + const expected = entry.count ?? 1; + const vs = actualByKey.get(k) || []; + const actual = vs.length; + if (actual < expected) { + stale.push({ ...entry, count: expected, actualCount: actual }); + } else if (actual > expected) { + fresh.push(...vs.slice(expected)); + } + } + + return { fresh, stale }; +} + +/** Stable sort: by `file`, then by `text`. */ +function sortEntries(entries) { + return [...entries].sort((a, b) => { + if (a.file !== b.file) return a.file < b.file ? -1 : 1; + if (a.text !== b.text) return a.text < b.text ? -1 : 1; + return 0; + }); +} + +/** + * Collapse `violations` into one baseline row per distinct (file, text) + * pair, carrying a `count` of how many occurrences that pair has in THIS + * run. Pure; no I/O. + */ +function dedupeViolationsForBaseline(violations) { + const order = []; + const byKey = new Map(); + for (const v of violations) { + const k = `${v.file} ${v.text}`; + let entry = byKey.get(k); + if (!entry) { + entry = { file: v.file, text: v.text, kind: v.kind, owner_issue: RATCHET_OWNER_ISSUE, count: 0 }; + byKey.set(k, entry); + order.push(entry); + } + entry.count += 1; + } + return order; +} + +function writeBaseline(root, violations) { + const entries = sortEntries(dedupeViolationsForBaseline(violations)); + const doc = { + $comment: + '#3409 unreachable-shell-guard ratchet. See scripts/lint-unreachable-guard-drift.cjs. ' + + 'SHRINK-ONLY: entries are removed as sites migrate off the unreachable-arm shape; new or ' + + 'changed entries fail lint:ci. `count` is the number of byte-identical (file, text) ' + + 'occurrences acknowledged at this site — a run producing fewer fails as a partial migration, ' + + 'more fails as an unacknowledged new copy.', + entries, + }; + const baselinePath = path.join(root, BASELINE_REL_PATH); + fs.mkdirSync(path.dirname(baselinePath), { recursive: true }); + fs.writeFileSync(baselinePath, `${JSON.stringify(doc, null, 2)}\n`, 'utf8'); + return entries; +} + +/** + * `--json` mode emits ONE structured JSON object to stdout in place of the + * human formatter below — the typed IR CONTRIBUTING.md's "Prohibited: Raw + * Text Matching on Test Outputs" requires. The human formatter's wording is + * untouched (operator console use only); `emitJson` is the only new output + * surface, gated on `json` so the two never interleave on the same stream. + */ +function main() { + const root = path.join(__dirname, '..'); + const update = process.argv.includes('--update'); + const json = process.argv.includes('--json'); + const { violations, malformed } = scanRepo(root); + + function emitJson(report) { + if (json) process.stdout.write(`${JSON.stringify(report, null, 2)}\n`); + } + + if (update) { + if (malformed.length > 0) { + if (!json) { + process.stderr.write('unreachable-guard-drift: malformed `# gsd-scan-ignore:` declaration(s) — fix these before regenerating the baseline (they are never ratchet-eligible):\n'); + for (const m of malformed) { + process.stderr.write(` ${sanitizeForReport(m.file)}:${m.line} ${sanitizeForReport(m.text)}\n`); + } + process.stderr.write('\n remedy: the reason after `# gsd-scan-ignore:` must name a tracking issue (#NNN) or an http(s):// URL.\n'); + } + emitJson({ reason: REASON.FAIL_MALFORMED_MARKER, violations: [], malformed, stale: [], baselineErrors: [] }); + process.exitCode = 1; + return; + } + const entries = writeBaseline(root, violations); + if (!json) { + process.stdout.write(`ok unreachable-guard-drift: baseline regenerated with ${entries.length} entr${entries.length === 1 ? 'y' : 'ies'}\n`); + } + emitJson({ reason: REASON.OK_BASELINE_UPDATED, violations: [], malformed: [], stale: [], baselineErrors: [], updatedEntryCount: entries.length }); + return; + } + + const { entries: baseline, errors } = loadBaseline(root); + if (errors.length > 0) { + if (!json) { + process.stderr.write('unreachable-guard-drift: baseline load error(s):\n'); + // OUTPUT SEAM: `loadBaseline`'s `message` strings embed + // `JSON.stringify(entry.file)` / `JSON.stringify(entry.text)` / + // `JSON.stringify(entry)` verbatim, and `JSON.stringify` escapes only + // code points below 0x20 — it passes C1 controls (0x7F-0x9F) and the + // bidi/zero-width controls (U+202E RTL override, U+2066-U+2069, + // U+2028, U+2029) through UNESCAPED. Without `sanitizeForReport` here, a + // crafted `entries[].file`/`.text` value in the baseline JSON could + // land an active bidi override straight into CI console output — the + // exact report-spoofing class every violation/malformed field below is + // already routed through `sanitizeForReport` to prevent. + for (const e of errors) process.stderr.write(` ${sanitizeForReport(e.message)}\n`); + } + emitJson({ reason: REASON.FAIL_BASELINE_LOAD, violations: [], malformed: [], stale: [], baselineErrors: errors }); + process.exitCode = 1; + return; + } + + const { fresh, stale } = diffAgainstBaseline(violations, baseline); + + if (fresh.length === 0 && stale.length === 0 && malformed.length === 0) { + if (!json) { + process.stdout.write(`ok unreachable-guard-drift: no unacknowledged unreachable shell-guard shapes in the prompt layer (${baseline.length} known)\n`); + } + emitJson({ reason: REASON.OK_NO_VIOLATIONS, violations: [], malformed: [], stale: [], baselineErrors: [], knownCount: baseline.length }); + return; + } + + if (!json) { + if (fresh.length > 0) { + process.stderr.write('unreachable-guard-drift: NEW unreachable shell-guard shape(s) found in the prompt layer.\n'); + process.stderr.write('Detector A (--pick + || echo): the fallback can never fire on field absence — replace with an\n'); + process.stderr.write('explicit -z/empty-string test on the resolved value.\n'); + process.stderr.write('Detector B (cat/ls over a glob operand): under a nullglob set elsewhere in the same shell\n'); + process.stderr.write('session, an unmatched glob reads from stdin (cat) or lists the cwd (ls) — use an array\n'); + process.stderr.write('expansion or an existence test instead.\n'); + process.stderr.write(`Or, if this is a deliberate wrong-example, declare it with # gsd-scan-ignore: #NNN, or add an\n`); + process.stderr.write(`acknowledged entry to ${BASELINE_REL_PATH} via --update:\n`); + for (const v of fresh) { + process.stderr.write(` ${sanitizeForReport(v.file)}:${v.line} [${v.kind}] ${sanitizeForReport(v.found)} ${sanitizeForReport(v.text)}\n`); + } + } + + if (stale.length > 0) { + process.stderr.write('\nunreachable-guard-drift: STALE baseline entr' + (stale.length === 1 ? 'y' : 'ies') + " (fully migrated, or a PARTIAL migration — fewer occurrences found than acknowledged; delete or re-record the row):\n"); + for (const e of stale) { + process.stderr.write(` ${sanitizeForReport(e.file)} ${sanitizeForReport(e.text)} (found ${e.actualCount}/${e.count} acknowledged occurrence${e.count === 1 ? '' : 's'})\n`); + } + process.stderr.write(`\n remedy: node scripts/lint-unreachable-guard-drift.cjs --update\n`); + } + + if (malformed.length > 0) { + process.stderr.write('\nunreachable-guard-drift: malformed `# gsd-scan-ignore:` declaration(s) — never ratchet-eligible, must be fixed directly:\n'); + for (const m of malformed) { + process.stderr.write(` ${sanitizeForReport(m.file)}:${m.line} ${sanitizeForReport(m.text)}\n`); + } + process.stderr.write('\n remedy: the reason after `# gsd-scan-ignore:` must name a tracking issue (#NNN) or an http(s):// URL.\n'); + } + } + + const reason = fresh.length > 0 + ? REASON.FAIL_FRESH_VIOLATION + : stale.length > 0 + ? REASON.FAIL_STALE_ENTRY + : REASON.FAIL_MALFORMED_MARKER; + emitJson({ reason, violations: fresh, malformed, stale, baselineErrors: [] }); + + process.exitCode = 1; +} + +if (require.main === module) main(); + +module.exports = { + findUnreachableGuardDrift, + detectGlobOperand, + scanRepo, + toPosixRel, + loadBaseline, + diffAgainstBaseline, + dedupeViolationsForBaseline, + sortEntries, + writeBaseline, + PICK_RE, + ECHO_FALLBACK_RE, + CAT_LS_COMMAND_RE, + HEREDOC_AFTER_COMMAND_RE, + EXIT_TESTING_KEYWORDS, + FALLBACK_TOKEN_RE, + isNoopFallback, + scanClauseAfterCommand, + MARKER_RE, + ISSUE_REF_RE, + SCAN_DIRS, + SCAN_EXT, + BASELINE_REL_PATH, + RATCHET_OWNER_ISSUE, + REASON, +}; diff --git a/skills/gsd-review-backlog/SKILL.md b/skills/gsd-review-backlog/SKILL.md index 102937956..0a3f3a94f 100644 --- a/skills/gsd-review-backlog/SKILL.md +++ b/skills/gsd-review-backlog/SKILL.md @@ -18,7 +18,8 @@ milestone sequence or remove stale entries. 1. **List backlog items:** ```bash - ls -d .planning/phases/999* 2>/dev/null || echo "No backlog items found" + _BACKLOG=( .planning/phases/999* ) + if [ -e "${_BACKLOG[0]}" ]; then ls -d "${_BACKLOG[@]}"; else echo "No backlog items found"; fi ``` 2. **Read ROADMAP.md** and extract all 999.x phase entries: diff --git a/tests/emitted-drift-acks/1955-verifier-coincidental-reliance.json b/tests/emitted-drift-acks/1955-verifier-coincidental-reliance.json deleted file mode 100644 index 1cdde5309..000000000 --- a/tests/emitted-drift-acks/1955-verifier-coincidental-reliance.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "version": 1, - "paths": { - "gsd-verifier.md": "#1955: Step 3 gains sub-step 5c (the coincidental-reliance advisory), one Step 9 score bullet, one Observable Truths example row, and the coincidental_reliance_items frontmatter block. Growth is those four additions only — no existing text was rewritten. Deliberately dense rather than extracted: the issue's approved scope says 'No new files', and ADR-1610 Decision 4 / tests/workflow-size-budget.test.cjs:32-40 name eager @-import relocation as gaming the size proxy (it shrinks the measured file while total loaded context is unchanged or larger); this agent has no lazy-read seam. After the change the file sits at 48994 bytes against the LARGE tier hard cap of 49152 (tests/agent-size-budget.test.cjs), i.e. 158 bytes of headroom. That is deliberate and disclosed: the cap is not crossed and is not raised, but the next contributor who needs room in this agent must do a lazy extraction rather than add prose. Content justification: the verifier's goal-backward pass grades THAT a truth holds and never WHY, so a truth can read VERIFIED while resting on a coincidence — a precondition nothing guarantees, an ordering nothing enforces, or a fixture-only truth. 5c classifies the evidence already recorded rather than asking it to rate its own confidence — but it is honestly an endogenous check and so weaker than the exogenous backstop tag gsd-core/references/honest-verifier.md routes on — which is exactly why it is advisory only: it changes no score, no status, and emits no human-verification item, so a passing phase still passes. gsd-core/workflows/verify-phase.md is deliberately NOT edited (it has 29 bytes under its DEFAULT tier cap); it receives the rule through its existing eager @-import of gsd-core/templates/verification-report.md, whose Guidelines now carry the imperative check rather than only the row shape. tests/verifier-coincidental-reliance.test.cjs asserts both halves of that route and locks out a silent second copy appearing in the workflow. — #1892 append (epic #1891 F7, merged into this fragment because two ack sources may never name the same path): +55 bytes, one required_reading line (@~/.claude/gsd-core/references/verifier-phase-gates.md) so the verifier eagerly loads the three verification-time gates migrated out of the now-DELETED orphan workflow gsd-core/workflows/verify-phase.md (decision-coverage validation #2492, test-quality audit, infrastructure-phase human-verification scoping #2504) — the #1955-era routing through that workflow's template import retired with it. Net loaded context shrinks ~30 KB (40,931-byte orphan stops shipping and loading, replaced by a 9,951-byte reference plus this line); not proxy-gaming per ADR-1610 D4 — no existing prose was relocated out of the measured file, live behavior that previously reached no runtime is restored. Agent sits at 49,049 of 49,152 bytes (103 bytes of headroom), cap unchanged." - } -} diff --git a/tests/emitted-drift-acks/2229-explore-claim-disposition.json b/tests/emitted-drift-acks/2229-explore-claim-disposition.json index 7bb10a9d1..6d3b7589a 100644 --- a/tests/emitted-drift-acks/2229-explore-claim-disposition.json +++ b/tests/emitted-drift-acks/2229-explore-claim-disposition.json @@ -1,7 +1,6 @@ { "version": 1, "paths": { - "explore.md": "#2229 adds the three-way claim disposition (admit / refute / abstain) plus the Unresolved ledger to the /gsd-explore Step-3 research pass, so a surfaced research claim carries its grounding instead of being folded into confident prose unverified. The growth is the contract itself, written inline in the workflow body: the refute-then-ground-or-abstain spawn instruction, an authority-based decision procedure separating refute from abstain (without it the two arms describe the same event), the Unresolved-ledger output template and its five-value reason enum, a defined destination for an untagged finding, the two guards (conflict-abstention and the tier floor), and the Step 4-5 rule carrying the disposition into crystallized artifacts so an abstain is not laundered into flat prose downstream. That guidance is deliberately NOT relocated into an eagerly @-imported reference, which ADR-1610 Decision 4 names as gaming the size proxy (only lazy, Read-at-step extraction counts). Round 11 REDUCED the file: the tier floor now reads one signal (query resolve-model --pick tier) instead of two proxies plus a both-empty fail-safe, which retired the two-signal rationale, the resolveModelInternal precedence explanation, and two of the three disclosed residuals; the canonical gsd_run preamble also moved to an unconditional Step-1 block so declining the research offer cannot leave Step 5's commit unbootstrapped (still exactly one preamble per file, per runtime-launcher-parity). 11127 -> 21297 bytes (+10170 vs next base), DEFAULT tier, cap 40960 - 52% of budget.", - "gsd-phase-researcher.md": "#2229 adds the agent-side counterpart to the explore.md workflow change: the phase-researcher agent gains the claim-disposition mode (admit / refute / abstain) it must follow when a /gsd:explore quick-research pass hands it a prompt-level disposition contract, plus the Quick Claim-Disposition Pass section binding the researcher's return shape (3-5 tagged findings inline, no RESEARCH.md) and the authority-based refute-vs-abstain discriminator. Round 11 constrains the abstain tag's reason to the caller's five-value ledger enum, byte-identical to explore.md, so a free-text reason cannot arrive at a ledger that takes a closed set. This is the executable behavior the workflow guidance delegates to, kept in the agent contract so it loads exactly when the agent runs. 42014 -> 44207 bytes (+2193), DEFAULT tier." + "explore.md": "#2229 adds the three-way claim disposition (admit / refute / abstain) plus the Unresolved ledger to the /gsd-explore Step-3 research pass, so a surfaced research claim carries its grounding instead of being folded into confident prose unverified. The growth is the contract itself, written inline in the workflow body: the refute-then-ground-or-abstain spawn instruction, an authority-based decision procedure separating refute from abstain (without it the two arms describe the same event), the Unresolved-ledger output template and its five-value reason enum, a defined destination for an untagged finding, the two guards (conflict-abstention and the tier floor), and the Step 4-5 rule carrying the disposition into crystallized artifacts so an abstain is not laundered into flat prose downstream. That guidance is deliberately NOT relocated into an eagerly @-imported reference, which ADR-1610 Decision 4 names as gaming the size proxy (only lazy, Read-at-step extraction counts). Round 11 REDUCED the file: the tier floor now reads one signal (query resolve-model --pick tier) instead of two proxies plus a both-empty fail-safe, which retired the two-signal rationale, the resolveModelInternal precedence explanation, and two of the three disclosed residuals; the canonical gsd_run preamble also moved to an unconditional Step-1 block so declining the research offer cannot leave Step 5's commit unbootstrapped (still exactly one preamble per file, per runtime-launcher-parity). 11127 -> 21297 bytes (+10170 vs next base), DEFAULT tier, cap 40960 - 52% of budget." } } diff --git a/tests/emitted-drift-acks/3297-planphase-gaps-next-up-scope.json b/tests/emitted-drift-acks/3297-planphase-gaps-next-up-scope.json deleted file mode 100644 index 865b3b451..000000000 --- a/tests/emitted-drift-acks/3297-planphase-gaps-next-up-scope.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "version": 1, - "paths": { - "plan-phase.md": "#3297: the bash preamble now captures --gaps planning mode into GAPS_MODE and derives GAPS_EXEC_FLAG (empty or --gaps-only), and the Next Up command substitutes ${GAPS_EXEC_FLAG} into /gsd:execute-phase {X} so a completed --gaps planning run hands off execute-phase's matching gap-closure scope (--gaps-only) instead of the whole-phase run — the literal fix for the mode-blind handoff, plus the six-line gap-mode capture/derive block and the one-paragraph rendering note that tells the agent how to substitute the flag. Standard and --reviews runs leave the flag empty, so their rendered Next Up is byte-identical to before. Growth is the write side of the fix, not incidental prose. Supersedes the spent #3218 fragment (merged into next), which also named plan-phase.md and would otherwise double-ack the same path. — #3423 append (epic #1891 F8): also grew 21 bytes with the -> tag rename (7 tag tokens, +3 bytes each; tag-token-only delta, no prose changed)." - } -} diff --git a/tests/emitted-drift-acks/3357-verification-resolver.json b/tests/emitted-drift-acks/3357-verification-resolver.json index 39b0627be..ee79c443c 100644 --- a/tests/emitted-drift-acks/3357-verification-resolver.json +++ b/tests/emitted-drift-acks/3357-verification-resolver.json @@ -1,9 +1,6 @@ { "version": 1, "paths": { - "transition.md": { - "reason": "#3357: verify_completion now resolves THIS phase's own verification report through the shared `verification.resolve-file` seam (src/verification.cts resolveVerificationFile) instead of a blind `*-VERIFICATION.md` glob feeding awk directly, so a stray ad-hoc worksheet (e.g. `03-CORRECTION-VERIFICATION.md`) can no longer alphabetically outrank the real report. Growth is +632 bytes (23,247 -> 23,879): the `gsd_run query verification.resolve-file` call plus its comment, and the FNR-vs-NR frontmatter re-arm fix (with rationale comment) for correctness across the resolver's single-path result. The canonical runtime-launcher preamble was relocated (not duplicated) from `update_roadmap_and_state` up to `verify_completion` since that step now needs `gsd_run` earlier in document order; net preamble count is unchanged at exactly one." - }, "verify-work.md": { "reason": "#3357: the `03-VERIFICATION.md` staleness check now resolves the phase's own report via `gsd_run query verification.resolve-file \"$PHASE_DIR\" --raw` instead of `ls \"${PHASE_DIR}\"/*-VERIFICATION.md | head -1`, routing through the same shared resolver seam as transition.md so both call sites agree on which file is canonical. Growth is +13 bytes (38,983 -> 38,996), the delta between the old ls/head-1 pipeline and the gsd_run call." } diff --git a/tests/emitted-drift-acks/3409-unreachable-guard-arms.json b/tests/emitted-drift-acks/3409-unreachable-guard-arms.json new file mode 100644 index 000000000..a2b4bfc78 --- /dev/null +++ b/tests/emitted-drift-acks/3409-unreachable-guard-arms.json @@ -0,0 +1,12 @@ +{ + "version": 1, + "paths": { + "gsd-phase-researcher.md": "#3409: guarded `cat \"$phase_dir\"/*-CONTEXT.md` against nullglob wiping the pattern to zero operands when no CONTEXT.md exists — a bare `cat` with no operands blocks reading stdin (hangs the agent) instead of the `2>/dev/null` guard ever firing, since a stalled read is not a failing exit. Now checks `${_CTX[0]}` is a real path before invoking cat. Growth is the array-guard idiom itself (+43 bytes).", + "gsd-verifier.md": "#3409: same nullglob-hang fix as gsd-phase-researcher.md, applied to `cat \"$PHASE_DIR\"/*-VERIFICATION.md` in Step 0 — an absent VERIFICATION.md previously left a zero-operand `cat` blocking on stdin instead of falling through to first-verification mode. Growth is the array-guard idiom (+49 bytes).", + "complete-milestone.md": "#3409: guarded `cat .planning/phases/*-*/*-SUMMARY.md` — with `shopt -s nullglob` active in this block's preamble (#2962), zero matching phase summaries collapses the glob to nothing and a bare `cat` blocks reading stdin rather than producing empty output, wedging the milestone-completion review. Growth is the array-existence-check idiom (+73 bytes, two glob segments makes this longer than the single-glob sites).", + "discuss-phase-assumptions.md": "#3409: replaced the unreachable `AUTO_MODE=$(gsd_run query check auto-mode --pick active 2>/dev/null || echo \"false\")` — `||` never fires because the query exits 0 with empty stdout when the field is absent, not a failure, so AUTO_MODE silently ended up empty rather than \"false\" — with a two-line capture-then-default (`AUTO_MODE=\"${AUTO_MODE:-false}\"`) that actually reaches the fallback. Growth is the extra default-assignment line (+19 bytes).", + "plan-phase.md": "#3409: three sites. `AUTO_CHAIN` and `PHASE_REQ_IDS` get the same unreachable-`||`-fallback fix as discuss-phase-assumptions.md (empty-but-successful `gsd_run query` output never triggered `|| echo`, now uses `${VAR:-default}`); `PRIOR_SUMMARIES` additionally swapped `gsd_run query phases.list --pick summaries_total` for `--type summaries --pick count` since the old pick key produced the same unreachable-fallback failure mode for the walking-skeleton check. Net growth across the three sites is +39 bytes.", + "session-report.md": "#3409: guarded `ls -la .planning/reports/SESSION_REPORT*.md 2>/dev/null || echo \"No previous reports\"` — with nullglob active, zero prior reports collapses the pattern to nothing and `ls -la` with no operands lists the current directory (a successful exit, wrong output) instead of failing into the `|| echo` fallback, so the report-existence check silently printed a directory listing. Replaced with an array-existence check that only lists when a real report file is present. Growth is the guard idiom (+58 bytes).", + "transition.md": "#3409: guarded `cat .planning/phases/XX-current/*-SUMMARY.md` — same nullglob-hang defect as complete-milestone.md's phase-summary read: zero summaries left a bare `cat` blocking on stdin instead of proceeding with no summary content during PROJECT.md evolution. Growth is the array-existence-check idiom (+73 bytes)." + } +} diff --git a/tests/emitted-drift-acks/3458-audit-open-acknowledge-wiring.json b/tests/emitted-drift-acks/3458-audit-open-acknowledge-wiring.json deleted file mode 100644 index 87c904bde..000000000 --- a/tests/emitted-drift-acks/3458-audit-open-acknowledge-wiring.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "version": 1, - "paths": { - "complete-milestone.md": "#3458 follow-up: the `pre_close_artifact_audit` step's `[A]` branch previously told the model to hand-author a `## Deferred Items` markdown table with no writer, no schema, and no reader — the acknowledgment never actually suppressed anything at the next close. This wires the real `audit-open acknowledge` CLI writer that landed in src/audit.cts: step 2 now calls it once per open item, per category, using the identifiers the audit JSON already emits (including the quick_tasks `--dir` reconstruction from `date`+`slug`, and the phase-scoped `--archived-milestone` passthrough), before step 3 writes the STATE.md table as a disclosure record only. Also converges the STATE.md `## Deferred Items` table to the template's 5-column shape (adds `Milestone`) and updates the MILESTONES.md disclosure line to distinguish newly-acknowledged items from ones a prior close already suppressed. The growth (+6,764 bytes) is this new per-category acknowledgment loop plus the expanded disclosure/security prose — no unrelated content moved. #3458 follow-up review (round 2, BLOCKER 3): the `[A]` branch's acknowledge loops previously ran as `cmd | while read` pipelines with no failure tracking, so a refused acknowledge (`unsupported_heading_shape`, `ambiguous`, `not_found`, missing file) was silently discarded and the close proceeded anyway; separately, `AUDIT_JSON=$(gsd_run query audit-open --json)` never handled the `@file:` large-payload sentinel `io.output` swaps in past 50000 chars, so every `jq` read against it would silently no-op every loop body. This round switches every `cmd | while read` to `while read; do …; done < <(cmd)` (process substitution, so the loop body runs in the CURRENT shell and can mutate a counter that survives it) plus an `ACK_FAILURES` accumulator that HALTS the step before close if any acknowledge call failed, and adds the same `@file:` sentinel handling `verify_readiness`'s `INIT_MANAGER` already uses. Additional growth: +2,433 bytes for the failure-accumulation wrapper and the `@file:` guard." - } -} diff --git a/tests/unreachable-guard-drift.test.cjs b/tests/unreachable-guard-drift.test.cjs new file mode 100644 index 000000000..08775b013 --- /dev/null +++ b/tests/unreachable-guard-drift.test.cjs @@ -0,0 +1,1115 @@ +// gsd-scan-ignore: #3409 — this test file's fixture strings are constructed +// via concatenation (never written as literal detector-matching source +// lines) so scripts/lint-unreachable-guard-drift.cjs, whose SCAN_DIRS do not +// include tests/, never has occasion to see this comment as an attempted +// declaration either. + +'use strict'; + +/** + * Tests for the unreachable-shell-guard prompt-layer drift guard (#3409) — + * scripts/lint-unreachable-guard-drift.cjs. + * + * Design: .gsd/phase/feat-3409-unreachable-shell-guard-lint/40-design.md + * Test matrix: .gsd/phase/feat-3409-unreachable-shell-guard-lint/50-test-matrix.md + * + * Covers every row of the matrix EXCEPT the "Regression — the three defects + * this PR fixes" section (G1-G4), which lives in + * tests/unreachable-shell-guard.test.cjs and is not touched here. + * + * FIXTURE STRINGS ARE BUILT, NOT WRITTEN LITERALLY, where a literal would + * itself be a detector-matching source line living under this repo's tree — + * this file is not under any of the guard's SCAN_DIRS + * (gsd-core/workflows, commands, agents, skills), so nothing here is ever + * scanned by the guard under test, but the concatenation habit is kept + * anyway as the sanctioned way to avoid an incidental self-match noted in + * the dispatch brief, rather than a file allowlist (40-design.md's + * Rejected #5 forbids allowlists for exactly this shape of problem). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fc = require('fast-check'); +const fs = require('node:fs'); +const path = require('node:path'); + +const drift = require('../scripts/lint-unreachable-guard-drift.cjs'); +const { + findUnreachableGuardDrift, + detectGlobOperand, + scanRepo, + loadBaseline, + diffAgainstBaseline, + dedupeViolationsForBaseline, + writeBaseline, + toPosixRel, + PICK_RE, + ECHO_FALLBACK_RE, + CAT_LS_COMMAND_RE, + HEREDOC_AFTER_COMMAND_RE, + isNoopFallback, + scanClauseAfterCommand, + MARKER_RE, + ISSUE_REF_RE, + BASELINE_REL_PATH, + REASON, +} = drift; +const { createTempDir, cleanup } = require('./helpers.cjs'); +const { runNode } = require('./helpers/process-seam.cjs'); +const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { sanitizeForReport } = require('../scripts/lib/drift-scan.cjs'); + +const REPO_ROOT = path.join(__dirname, '..'); +const DRIFT_SCRIPT = path.join(REPO_ROOT, 'scripts', 'lint-unreachable-guard-drift.cjs'); +const FAKE_FILE = 'gsd-core/workflows/fake.md'; + +// Build a fenced-shell "gsd_run … --pick … || echo …" style line without +// ever writing the literal pipe-pipe/echo shape as adjacent source tokens in +// THIS file. `parts.join('')` concatenates at runtime. +function pickEchoLine({ pick = 'summaries_total', stderr = true, fallback = '"0"' } = {}) { + const parts = [ + 'X=$(gsd_run query phases.list --pick ', pick, + stderr ? ' 2>/dev/null' : '', + ' ', '|', '|', ' echo ', fallback, ')', + ]; + return parts.join(''); +} + +function catLine(operand, { cmd = 'cat', prefix = '', suffix = '' } = {}) { + return `${prefix}${cmd} ${operand}${suffix}`; +} + +function markerComment(reason) { + return `# gsd-scan-ignore: ${reason}`; +} + +// ─── Detector A — PICK_RE / ECHO_FALLBACK_RE ────────────────────────────── + +describe('Detector A — --pick + || echo fallback', () => { + test('A1: canonical shape with 2>/dev/null is flagged, found names --pick', () => { + const line = pickEchoLine({ stderr: true, fallback: '"d"' }); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].kind, 'A'); + assert.strictEqual(violations[0].found, '--pick'); + assert.strictEqual(violations[0].text, line.trim()); + }); + + test('A2: without a stderr redirect is still flagged', () => { + const line = pickEchoLine({ stderr: false, fallback: '"d"' }); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].kind, 'A'); + }); + + test('A3: an empty-string default is still flagged (unreachable AND a no-op)', () => { + const line = pickEchoLine({ fallback: '""' }); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.strictEqual(violations.length, 1); + }); + + test('A4: a --pick-less config-get fallback is NOT detected', () => { + const line = ['X=$(gsd_run query config-get k 2>/dev/null ', '|', '|', ' echo "false")'].join(''); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('A5: a pick with no fallback is NOT detected', () => { + const line = 'X=$(gsd_run query phases.list --pick summaries_total)'; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('A6: a git fallback is NOT detected', () => { + const line = ['X=$(git rev-list --count HEAD ', '|', '|', ' echo 0)'].join(''); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('A7: a grep fallback is NOT detected', () => { + const line = ["Y=$(grep -cE '^' file.md ", '|', '|', ' echo "0")'].join(''); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('A8: the and-or ternary idiom is NOT detected', () => { + const line = ['$([ -n "$X" ] && echo "a" ', '|', '|', ' echo "")'].join(''); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('A9 (known limit): a cross-line split is NOT detected', () => { + const text = [ + 'X=$(gsd_run query phases.list --pick summaries_total 2>/dev/null)', + ['Y=$(echo "$X" ', '|', '|', ' echo "0")'].join(''), + ].join('\n'); + const { violations } = findUnreachableGuardDrift(text, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('A10 (known limit): a printf fallback is NOT detected', () => { + const line = ["X=$(gsd_run query phases.list --pick f ", '|', '|', " printf 'd')"].join(''); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('A11: a fenced block is not an exemption — the same line inside ```bash still flags', () => { + const line = pickEchoLine(); + const text = ['```bash', line, '```'].join('\n'); + const { violations } = findUnreachableGuardDrift(text, FAKE_FILE); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].line, 2); + }); + + test('A12: reports each violating line separately, with correct line numbers', () => { + const l1 = pickEchoLine({ pick: 'a' }); + const l2 = pickEchoLine({ pick: 'b' }); + const text = ['no-op', l1, 'middle', l2].join('\n'); + const { violations } = findUnreachableGuardDrift(text, FAKE_FILE); + assert.strictEqual(violations.length, 2); + assert.strictEqual(violations[0].line, 2); + assert.strictEqual(violations[1].line, 4); + }); + + test('A13: byte-identical duplicates count as 2 occurrences', () => { + const line = pickEchoLine(); + const text = [line, line].join('\n'); + const { violations } = findUnreachableGuardDrift(text, FAKE_FILE); + assert.strictEqual(violations.length, 2); + assert.strictEqual(violations[0].text, violations[1].text); + }); + + test('A15: CRLF line endings yield the identical verdict to LF', () => { + const line = pickEchoLine(); + const lf = findUnreachableGuardDrift(line, FAKE_FILE); + const crlf = findUnreachableGuardDrift(`${line}\r\n`, FAKE_FILE); + assert.strictEqual(lf.violations.length, 1); + assert.strictEqual(crlf.violations.length, 1); + assert.strictEqual(crlf.violations[0].text, lf.violations[0].text); + }); +}); + +// ─── Detector B — B-i cat / B-ii ls … || / +// B-iii ls at the head of if/elif/while ───────────────────────────── +// +// Scope, per the coordinator's measured correction: `ls ` fires ONLY +// when its exit code feeds either a genuine `||` fallback (not the no-op +// `true`/`:`) or gates an `if`/`elif`/`while` test — never when its STDOUT +// is what's consumed (piped, captured via `$(...)`), and never on the +// `|| true`/`|| :` defensive-suppression idiom, which carries no fallback +// value for nullglob's success-on-empty to defeat. + +describe('Detector B — cat (B-i), ls || (B-ii), ls at if/elif/while (B-iii)', () => { + test('B1: detects a bare cat over an unmatched-capable glob (B-i, unconditional)', () => { + const line = catLine('.planning/phases/*-*/*-SUMMARY.md'); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].kind, 'B'); + assert.strictEqual(violations[0].found, 'cat'); + }); + + test('B2: detects ls at the head of an if test (B-iii — still FIRES, exit code gates control flow)', () => { + const line = 'if ls "${DIR}/"*-CONTEXT.md; then true; fi'; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].found, 'ls'); + }); + + test('B3: a stderr redirect does not cure the cat stdin hazard (B-i)', () => { + const line = catLine('dir/*.md', { suffix: ' 2>/dev/null' }); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.strictEqual(violations.length, 1); + }); + + test('B4: an array expansion is NOT detected (it is the remedy)', () => { + const line = catLine('"${_CTX[@]}"'); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('B5: a for-list glob is NOT detected', () => { + const line = 'for f in dir/*.md; do echo "$f"; done'; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('B6: a literal operand is NOT detected', () => { + const a = findUnreachableGuardDrift(catLine('"$FILE"'), FAKE_FILE); + const b = findUnreachableGuardDrift(catLine('f1 f2'), FAKE_FILE); + assert.deepStrictEqual(a.violations, []); + assert.deepStrictEqual(b.violations, []); + }); + + test('B7: ls with no operand is NOT detected', () => { + const { violations } = findUnreachableGuardDrift('ls', FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('B8: a heredoc is NOT detected', () => { + const line = "cat <<'EOF'"; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('B9: a counting ls whose STDOUT is piped onward is NOT detected (informational, out of scope)', () => { + // Corrected semantics: `ls`'s exit code is not what's being consumed + // here — its STDOUT is piped to `wc -l`. This is the class of ~97 + // measured "informational ls" sites the issue explicitly leaves alone + // (it is neither the stdin-hang hazard nor a defeated fallback nor an + // always-true guard); overlap with lint-planning-prompt-drift on the + // COUNTING shape is that guard's concern, not this one's. + const line = 'ls -1 dir/*-PLAN.md | wc -l'; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('B10: the word cat in prose (not command position) is NOT detected', () => { + const line = 'The output looks like a cat chasing dir/*.md files around.'; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('B-ii positive: ls || echo "" fires — a real fallback nullglob silently defeats', () => { + // Real site shape (commands/gsd/review-backlog.md, pre-fix): under + // nullglob an unmatched glob makes `ls` list the CWD and succeed, so + // the intended "no backlog items" message never prints — exactly + // Detector A's shape one level down, with `ls`'s own exit code standing + // in for `--pick`'s coerced-to-'' absence. + const line = 'ls -d .planning/phases/999* 2>/dev/null || echo "No backlog items found"'; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].kind, 'B'); + assert.strictEqual(violations[0].found, 'ls'); + }); + + test('ls ... || true does NOT fire — suppressing a failure is not a guard', () => { + // No fallback VALUE exists here for nullglob's success-on-empty to + // defeat — `true` runs unconditionally either way. Measured: ~15 sites + // in this tree, all this exact defensive idiom (`set -e` failure + // suppression), none of them the #3300/#3409 hazard shape. + const line = 'ls -d .planning/milestones/v*-phases 2>/dev/null || true'; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('ls ... || : does NOT fire — the : no-op is the same idiom as || true', () => { + const line = 'ls -d .planning/phases/*/ 2>/dev/null || :'; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('an informational ls whose stdout is captured via $(...) does NOT fire (out of scope)', () => { + // Real site shape (commands/gsd/quick.md, and many like it): the exit + // code is never consulted at all — only the captured stdout is used — + // so this is neither the stdin-hang hazard nor a defeated fallback nor + // an always-true guard. Measured: 97 such sites, explicitly out of + // this issue's scope. + const line = 'dir=$(ls -d .planning/quick/*-{SLUG}/ 2>/dev/null | head -1)'; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('the Bash(cat << \'EOF\') prose line does NOT fire — a heredoc operator is never a glob operand', () => { + // Real site shape (agents/gsd-executor.md and six siblings): the + // surrounding markdown **bold** carries literal `*` characters + // elsewhere on the line, and `(` (from `Bash(`) is a valid command- + // position anchor for `$(cat ...)` — but the heredoc guard + // (HEREDOC_AFTER_COMMAND_RE) refuses to treat anything immediately + // after `cat` as an operand at all once it sees `<<`, so this never + // reaches the glob check regardless of what markdown emphasis follows. + const line = "3. **Do NOT use `Bash(cat << 'EOF')` or heredoc** for file creation. Use the `Write` tool."; + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + }); + + test('detectGlobOperand: pure predicate returns null on no command match', () => { + assert.strictEqual(detectGlobOperand('echo dir/*.md'), null); + }); + + test('detectGlobOperand: pure predicate returns the command name on a cat match (B-i)', () => { + assert.deepStrictEqual(detectGlobOperand(catLine('dir/*.md')), { command: 'cat' }); + }); + + test('detectGlobOperand: returns null for an ls glob with no exit-code-consuming shape', () => { + assert.strictEqual(detectGlobOperand('ls dir/*.md 2>/dev/null'), null); + }); + + test('detectGlobOperand: returns the command name for an ls glob with a real || fallback (B-ii)', () => { + assert.deepStrictEqual(detectGlobOperand('ls dir/*.md || echo missing'), { command: 'ls' }); + }); + + test('isNoopFallback recognizes true and : as no-ops and anything else as real', () => { + assert.strictEqual(isNoopFallback(' true'), true); + assert.strictEqual(isNoopFallback(' true) | sort'), true); + assert.strictEqual(isNoopFallback(' :'), true); + assert.strictEqual(isNoopFallback(' echo "message"'), false); + assert.strictEqual(isNoopFallback(''), true); + }); + + test('scanClauseAfterCommand finds the glob and the correct terminator across chain shapes', () => { + assert.deepStrictEqual(scanClauseAfterCommand(' dir/*.md | wc -l'), { hasGlob: true, terminator: '|', fallback: null }); + assert.deepStrictEqual(scanClauseAfterCommand(' dir/*.md && next'), { hasGlob: true, terminator: '&&', fallback: null }); + const orResult = scanClauseAfterCommand(' dir/*.md || echo x'); + assert.strictEqual(orResult.hasGlob, true); + assert.strictEqual(orResult.terminator, '||'); + assert.strictEqual(orResult.fallback, ' echo x'); + }); +}); + +// ─── Escape marker ───────────────────────────────────────────────────────── + +describe('Escape marker — # gsd-scan-ignore:', () => { + test('M1: a marker naming an issue exempts the line', () => { + const line = `${pickEchoLine()} ${markerComment('#3409')}`; + const { violations, malformed } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + assert.deepStrictEqual(malformed, []); + }); + + test('M2: a marker naming a URL exempts the line', () => { + const line = `${catLine('dir/*.md')} ${markerComment('https://example.com/issue/1')}`; + const { violations, malformed } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + assert.deepStrictEqual(malformed, []); + }); + + test('M3: a free-text reason reports a malformed declaration, not a plain violation', () => { + const line = `${pickEchoLine()} ${markerComment('because I said so')}`; + const { violations, malformed } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + assert.strictEqual(malformed.length, 1); + assert.strictEqual(malformed[0].reason, 'because I said so'); + }); + + test('M4: an empty reason is not an audit trail — malformed', () => { + const line = `${pickEchoLine()} # gsd-scan-ignore:`; + const { violations, malformed } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + assert.strictEqual(malformed.length, 1); + assert.strictEqual(malformed[0].reason, ''); + }); + + test('M5: a whitespace-only reason is rejected — malformed', () => { + const line = `${pickEchoLine()} # gsd-scan-ignore: `; + const { violations, malformed } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.deepStrictEqual(violations, []); + assert.strictEqual(malformed.length, 1); + }); + + test('M6: the marker binds only to its own line — a marker above a violation does not exempt it', () => { + const text = [markerComment('#3409'), pickEchoLine()].join('\n'); + const { violations, malformed } = findUnreachableGuardDrift(text, FAKE_FILE); + assert.strictEqual(violations.length, 1, 'the violation on line 2 must still fire'); + assert.deepStrictEqual(malformed, []); + }); + + test('a well-formed marker on a non-violating line produces neither a violation nor a malformed entry', () => { + const { violations, malformed } = findUnreachableGuardDrift(markerComment('#100'), FAKE_FILE); + assert.deepStrictEqual(violations, []); + assert.deepStrictEqual(malformed, []); + }); + + test('ISSUE_REF_RE accepts a bare #NNN and an http(s) URL, rejects free text', () => { + assert.ok(ISSUE_REF_RE.test('#3409')); + assert.ok(ISSUE_REF_RE.test('https://example.com/x')); + assert.ok(ISSUE_REF_RE.test('http://example.com/x')); + assert.ok(!ISSUE_REF_RE.test('no issue here')); + }); + + // Tightened predicate (deliberate divergence from + // tests/commit-files-pathspec.test.cjs's own looser ISSUE_REF_RE — see this + // module's ISSUE_REF_RE comment): `#0` and a bare scheme-only URL both + // satisfied the copied form's FORMAT-only check without naming anything + // real. Regex-level checks first (mirrors the existing convention just + // above), then the same two shapes driven through the CLI so the outcome + // is pinned on the structured REASON.* code, never on rendered text. + test('ISSUE_REF_RE rejects #0 (not a positive integer) and a bare http:// with no host', () => { + assert.ok(!ISSUE_REF_RE.test('#0')); + assert.ok(ISSUE_REF_RE.test('#123')); + assert.ok(!ISSUE_REF_RE.test('http://')); + assert.ok(!ISSUE_REF_RE.test('https://')); + assert.ok(ISSUE_REF_RE.test('https://example.com/x')); + }); + + function runIsolatedMarkerCase(t, reason) { + const root = createTempDir('gsd-3409-issueref-'); + t.after(() => cleanup(root)); + const isolatedScript = buildIsolatedGuard(root); + const wfDir = path.join(root, 'gsd-core', 'workflows'); + fs.mkdirSync(wfDir, { recursive: true }); + fs.writeFileSync(path.join(wfDir, 'fake.md'), `${pickEchoLine()} ${markerComment(reason)}\n`); + const baselinePath = path.join(root, BASELINE_REL_PATH); + fs.mkdirSync(path.dirname(baselinePath), { recursive: true }); + fs.writeFileSync(baselinePath, JSON.stringify({ entries: [] }), 'utf8'); + const jsonResult = runNode([isolatedScript, '--json'], { timeoutMs: PROBE_TIMEOUT_MS }); + return JSON.parse(jsonResult.stdout); + } + + test('#0 as a marker reason is rejected — reported as malformed, not exempted', (t) => { + const report = runIsolatedMarkerCase(t, '#0'); + assert.strictEqual(report.reason, REASON.FAIL_MALFORMED_MARKER); + assert.strictEqual(report.malformed.length, 1); + }); + + test('#123 as a marker reason is accepted — the line is exempted', (t) => { + const report = runIsolatedMarkerCase(t, '#123'); + assert.strictEqual(report.reason, REASON.OK_NO_VIOLATIONS); + }); + + test('a bare http:// as a marker reason is rejected — reported as malformed, not exempted', (t) => { + const report = runIsolatedMarkerCase(t, 'http://'); + assert.strictEqual(report.reason, REASON.FAIL_MALFORMED_MARKER); + assert.strictEqual(report.malformed.length, 1); + }); + + test('https://example.com/x as a marker reason is accepted — the line is exempted', (t) => { + const report = runIsolatedMarkerCase(t, 'https://example.com/x'); + assert.strictEqual(report.reason, REASON.OK_NO_VIOLATIONS); + }); +}); + +// ─── Ratchet baseline — diffAgainstBaseline ─────────────────────────────── + +describe('diffAgainstBaseline — ratchet invariants', () => { + test('R1: an acknowledged pair at its exact count passes (neither fresh nor stale)', () => { + const baseline = [{ file: 'a.md', text: 'X', count: 1 }]; + const violations = [{ file: 'a.md', line: 1, kind: 'A', found: '--pick', text: 'X' }]; + const { fresh, stale } = diffAgainstBaseline(violations, baseline); + assert.deepStrictEqual(fresh, []); + assert.deepStrictEqual(stale, []); + }); + + test('R2: a partial migration (count:2, actual 1) reports stale', () => { + const baseline = [{ file: 'a.md', text: 'X', count: 2 }]; + const violations = [{ file: 'a.md', line: 1, kind: 'A', found: '--pick', text: 'X' }]; + const { fresh, stale } = diffAgainstBaseline(violations, baseline); + assert.deepStrictEqual(fresh, []); + assert.strictEqual(stale.length, 1); + assert.strictEqual(stale[0].count, 2); + assert.strictEqual(stale[0].actualCount, 1); + }); + + test('R3: the exact acknowledged count (count:2, actual 2) passes', () => { + const baseline = [{ file: 'a.md', text: 'X', count: 2 }]; + const violations = [ + { file: 'a.md', line: 1, kind: 'A', found: '--pick', text: 'X' }, + { file: 'a.md', line: 9, kind: 'A', found: '--pick', text: 'X' }, + ]; + const { fresh, stale } = diffAgainstBaseline(violations, baseline); + assert.deepStrictEqual(fresh, []); + assert.deepStrictEqual(stale, []); + }); + + test('R4: an unacknowledged extra copy (count:2, actual 3) reports one fresh', () => { + const baseline = [{ file: 'a.md', text: 'X', count: 2 }]; + const violations = [ + { file: 'a.md', line: 1, kind: 'A', found: '--pick', text: 'X' }, + { file: 'a.md', line: 9, kind: 'A', found: '--pick', text: 'X' }, + { file: 'a.md', line: 20, kind: 'A', found: '--pick', text: 'X' }, + ]; + const { fresh, stale } = diffAgainstBaseline(violations, baseline); + assert.deepStrictEqual(stale, []); + assert.strictEqual(fresh.length, 1); + assert.strictEqual(fresh[0].line, 20); + }); + + test('R5: a fully migrated pair (actual 0) reports stale', () => { + const baseline = [{ file: 'a.md', text: 'X', count: 2 }]; + const { fresh, stale } = diffAgainstBaseline([], baseline); + assert.deepStrictEqual(fresh, []); + assert.strictEqual(stale.length, 1); + assert.strictEqual(stale[0].actualCount, 0); + }); + + test('R6: an unrecorded pair reports fresh', () => { + const violations = [{ file: 'a.md', line: 1, kind: 'B', found: 'cat', text: 'Y' }]; + const { fresh, stale } = diffAgainstBaseline(violations, []); + assert.strictEqual(fresh.length, 1); + assert.deepStrictEqual(stale, []); + }); + + test('R7: an entry with no count defaults to acknowledging one occurrence', () => { + const baseline = [{ file: 'a.md', text: 'X' }]; + const violations = [ + { file: 'a.md', line: 1, kind: 'A', found: '--pick', text: 'X' }, + { file: 'a.md', line: 9, kind: 'A', found: '--pick', text: 'X' }, + ]; + const { fresh, stale } = diffAgainstBaseline(violations, baseline); + assert.deepStrictEqual(stale, []); + assert.strictEqual(fresh.length, 1); + assert.strictEqual(fresh[0].line, 9); + }); + + test('R8: the key includes the file path — same text, different file is fresh', () => { + const baseline = [{ file: 'a.md', text: 'X', count: 1 }]; + const violations = [{ file: 'b.md', line: 1, kind: 'A', found: '--pick', text: 'X' }]; + const { fresh, stale } = diffAgainstBaseline(violations, baseline); + assert.strictEqual(fresh.length, 1); + assert.strictEqual(stale.length, 1, 'the a.md entry now has zero actual occurrences and is stale'); + }); + + test('R9: line-number churn does not disturb the baseline (keyed on text, not line)', () => { + const baseline = [{ file: 'a.md', text: 'X', count: 1 }]; + const violations = [{ file: 'a.md', line: 999, kind: 'A', found: '--pick', text: 'X' }]; + const { fresh, stale } = diffAgainstBaseline(violations, baseline); + assert.deepStrictEqual(fresh, []); + assert.deepStrictEqual(stale, []); + }); +}); + +// ─── Baseline loading — malformed input ─────────────────────────────────── + +describe('loadBaseline — malformed input', () => { + function writeBaselineFile(root, content) { + const p = path.join(root, BASELINE_REL_PATH); + fs.mkdirSync(path.dirname(p), { recursive: true }); + fs.writeFileSync(p, content, 'utf8'); + } + + test('L1: a missing baseline errors with its own remedy', (t) => { + const root = createTempDir('gsd-3409-baseline-'); + t.after(() => cleanup(root)); + const { entries, errors } = loadBaseline(root); + assert.deepStrictEqual(entries, []); + assert.strictEqual(errors.length, 1); + assert.strictEqual(errors[0].reason, REASON.FAIL_BASELINE_MISSING); + }); + + test('L2: an empty baseline errors', (t) => { + const root = createTempDir('gsd-3409-baseline-'); + t.after(() => cleanup(root)); + writeBaselineFile(root, ' \n'); + const { errors } = loadBaseline(root); + assert.strictEqual(errors.length, 1); + assert.strictEqual(errors[0].reason, REASON.FAIL_BASELINE_EMPTY); + }); + + test('L3: invalid JSON errors, naming the parse failure', (t) => { + const root = createTempDir('gsd-3409-baseline-'); + t.after(() => cleanup(root)); + writeBaselineFile(root, '{ not json'); + const { errors } = loadBaseline(root); + assert.strictEqual(errors.length, 1); + assert.strictEqual(errors[0].reason, REASON.FAIL_BASELINE_INVALID_JSON); + assert.strictEqual(typeof errors[0].parseError, 'string'); + }); + + test('L4: non-object JSON scalars each error, naming the actual type', (t) => { + const root = createTempDir('gsd-3409-baseline-'); + t.after(() => cleanup(root)); + for (const [literal, expectedType] of [['0', 'number'], ['"str"', 'string'], ['[]', 'array'], ['null', 'object'], ['true', 'boolean']]) { + writeBaselineFile(root, literal); + const { errors } = loadBaseline(root); + assert.strictEqual(errors.length, 1, `literal=${literal}`); + assert.strictEqual(errors[0].reason, REASON.FAIL_BASELINE_NOT_OBJECT, `literal=${literal}`); + assert.strictEqual(errors[0].gotType, expectedType, `literal=${literal}`); + } + }); + + test('L5: a non-array entries field errors', (t) => { + const root = createTempDir('gsd-3409-baseline-'); + t.after(() => cleanup(root)); + writeBaselineFile(root, JSON.stringify({ entries: 'nope' })); + const { errors } = loadBaseline(root); + assert.strictEqual(errors.length, 1); + assert.strictEqual(errors[0].reason, REASON.FAIL_BASELINE_ENTRIES_NOT_ARRAY); + }); + + test('L6: count must be a positive integer — 0, -1, 1.5, "2" each error', (t) => { + const root = createTempDir('gsd-3409-baseline-'); + t.after(() => cleanup(root)); + for (const badCount of [0, -1, 1.5, '2']) { + writeBaselineFile(root, JSON.stringify({ entries: [{ file: 'a.md', text: 'X', count: badCount }] })); + const { entries, errors } = loadBaseline(root); + assert.strictEqual(entries.length, 0, `count=${JSON.stringify(badCount)}`); + assert.strictEqual(errors.length, 1, `count=${JSON.stringify(badCount)}`); + assert.strictEqual(errors[0].reason, REASON.FAIL_BASELINE_ENTRY_COUNT_INVALID, `count=${JSON.stringify(badCount)}`); + } + }); + + test('L7: empty key fields (file or text) error', (t) => { + const root = createTempDir('gsd-3409-baseline-'); + t.after(() => cleanup(root)); + writeBaselineFile(root, JSON.stringify({ entries: [{ file: '', text: 'X' }] })); + let result = loadBaseline(root); + assert.strictEqual(result.errors.length, 1); + assert.strictEqual(result.errors[0].reason, REASON.FAIL_BASELINE_ENTRY_FIELD_INVALID); + assert.strictEqual(result.errors[0].field, 'file'); + + writeBaselineFile(root, JSON.stringify({ entries: [{ file: 'a.md', text: '' }] })); + result = loadBaseline(root); + assert.strictEqual(result.errors.length, 1); + assert.strictEqual(result.errors[0].reason, REASON.FAIL_BASELINE_ENTRY_FIELD_INVALID); + assert.strictEqual(result.errors[0].field, 'text'); + }); + + test('a valid baseline with a count field loads cleanly', (t) => { + const root = createTempDir('gsd-3409-baseline-'); + t.after(() => cleanup(root)); + writeBaselineFile(root, JSON.stringify({ entries: [{ file: 'a.md', text: 'X', count: 3 }] })); + const { entries, errors } = loadBaseline(root); + assert.deepStrictEqual(errors, []); + assert.strictEqual(entries.length, 1); + assert.strictEqual(entries[0].count, 3); + }); +}); + +// ─── Cross-platform & encoding ───────────────────────────────────────────── + +describe('Cross-platform & encoding', () => { + test('P1: a backslash relpath normalizes to POSIX in the key', () => { + const winRel = 'gsd-core\\workflows\\fake.md'; + const posixRel = 'gsd-core/workflows/fake.md'; + assert.strictEqual(toPosixRel(winRel), posixRel); + assert.strictEqual(toPosixRel(posixRel), posixRel); + const { violations } = findUnreachableGuardDrift(pickEchoLine(), winRel); + assert.strictEqual(violations[0].file, posixRel); + assert.ok(!violations[0].file.includes('\\')); + }); + + test('P2: CRLF input yields the same violations as LF (Detector A)', () => { + const line = pickEchoLine(); + const lf = findUnreachableGuardDrift(line, FAKE_FILE).violations; + const crlf = findUnreachableGuardDrift(line.replace(/\n/g, '\r\n') + '\r\n', FAKE_FILE).violations; + assert.strictEqual(lf.length, 1); + assert.strictEqual(crlf.length, 1); + assert.strictEqual(lf[0].text, crlf[0].text); + }); + + test('P3: CRLF does not defeat the glob detector (Detector B)', () => { + const line = catLine('dir/*.md'); + const lf = findUnreachableGuardDrift(line, FAKE_FILE).violations; + const crlf = findUnreachableGuardDrift(`${line}\r\n`, FAKE_FILE).violations; + assert.strictEqual(lf.length, 1); + assert.strictEqual(crlf.length, 1); + assert.strictEqual(lf[0].text, crlf[0].text); + }); + + test('P4: the baseline key is CR-free under CRLF — matches an LF-recorded baseline entry', () => { + const line = pickEchoLine(); + const baseline = [{ file: FAKE_FILE, text: line.trim(), count: 1 }]; + const crlfViolations = findUnreachableGuardDrift(`${line}\r\n`, FAKE_FILE).violations; + assert.ok(!crlfViolations[0].text.includes('\r'), 'the baseline-key text must carry no trailing \\r'); + const { fresh, stale } = diffAgainstBaseline(crlfViolations, baseline); + assert.deepStrictEqual(fresh, []); + assert.deepStrictEqual(stale, []); + }); +}); + +// ─── Hostile input ───────────────────────────────────────────────────────── + +describe('Hostile input', () => { + test('X1: sanitizeForReport (the human console formatter\'s own typed IR) strips a raw ESC byte to a visible \\xNN escape', () => { + // sanitizeForReport (scripts/lib/drift-scan.cjs) is the structured surface + // the human formatter consumes before writing to stderr — asserted on its + // own return value directly, never on the CLI's rendered stderr text. + const esc = String.fromCharCode(0x1b); + const sanitized = sanitizeForReport(`${esc}[31mred${esc}[0m`); + assert.ok(!sanitized.includes(esc), 'sanitizeForReport must strip the raw ESC byte'); + assert.match(sanitized, /\\x1b/); + }); + + test('X1b: a fresh violation carrying a hostile ANSI/control byte does not crash the CLI and classifies correctly', (t) => { + const root = createTempDir('gsd-3409-hostile-'); + t.after(() => cleanup(root)); + const isolatedScript = buildIsolatedGuard(root); + const wfDir = path.join(root, 'gsd-core', 'workflows'); + fs.mkdirSync(wfDir, { recursive: true }); + const esc = String.fromCharCode(0x1b); + const line = pickEchoLine() + ` # ${esc}[31mred${esc}[0m`; + fs.writeFileSync(path.join(wfDir, 'fake.md'), `${line}\n`); + writeBaselineFakeEmpty(root); + + // Human-mode run: only the structural facts (process outcome, exit code) + // are asserted — the rendered stderr content is never inspected. + const humanResult = runNode([isolatedScript], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(humanResult.outcome, 'exited'); + assert.strictEqual(humanResult.exitCode, 1); + + // --json run: the structured report is the typed IR under test. Strict + // JSON forbids a literal unescaped C0 control byte inside a string, so + // `JSON.parse` succeeding is itself proof no raw ESC byte reached the + // stdout stream unescaped. + const jsonResult = runNode([isolatedScript, '--json'], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(jsonResult.exitCode, 1); + const report = JSON.parse(jsonResult.stdout); + assert.strictEqual(report.reason, REASON.FAIL_FRESH_VIOLATION); + assert.strictEqual(report.violations.length, 1); + }); + + test('X2: a very long line does not hang the scanner', () => { + const start = Date.now(); + const longOperand = 'a'.repeat(100 * 1024) + '*.md'; + const line = catLine(longOperand); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.strictEqual(violations.length, 1); + assert.ok(Date.now() - start < 2000, 'a 100KB line must scan in well under 2s'); + }); + + test('X3: null bytes do not crash the scanner', () => { + const line = catLine('dir/*.md\0trailing'); + assert.doesNotThrow(() => findUnreachableGuardDrift(line, FAKE_FILE)); + const { violations } = findUnreachableGuardDrift(line, FAKE_FILE); + assert.strictEqual(violations.length, 1); + }); + + test('X4a: sanitizeForReport strips a raw RTL-override codepoint to a visible \\uNNNN escape', () => { + const rlo = '‮'; + const sanitized = sanitizeForReport(`dir/*.md${rlo}gnp.evil`); + assert.ok(!sanitized.includes(rlo), 'sanitizeForReport must strip the raw RLO codepoint'); + assert.match(sanitized, /\\u202e/); + }); + + test('X4b: unicode / RTL-override in the violating text is handled without crashing and classifies correctly', (t) => { + const root = createTempDir('gsd-3409-hostile-'); + t.after(() => cleanup(root)); + const isolatedScript = buildIsolatedGuard(root); + const wfDir = path.join(root, 'gsd-core', 'workflows'); + fs.mkdirSync(wfDir, { recursive: true }); + const rlo = '‮'; + const line = catLine(`dir/*.md${rlo}gnp.evil`); + fs.writeFileSync(path.join(wfDir, 'fake.md'), `${line}\n`); + writeBaselineFakeEmpty(root); + + const humanResult = runNode([isolatedScript], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(humanResult.outcome, 'exited'); + assert.strictEqual(humanResult.exitCode, 1); + + const jsonResult = runNode([isolatedScript, '--json'], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(jsonResult.exitCode, 1); + const report = JSON.parse(jsonResult.stdout); + assert.strictEqual(report.reason, REASON.FAIL_FRESH_VIOLATION); + assert.strictEqual(report.violations.length, 1); + }); + + test('X5: the walk stays inside the root — a symlink escaping the scan tree is not followed', (t) => { + if (process.platform === 'win32') { t.skip('symlink creation requires elevated privileges on Windows CI'); return; } + const root = createTempDir('gsd-3409-symlink-'); + t.after(() => cleanup(root)); + const outside = createTempDir('gsd-3409-outside-'); + t.after(() => cleanup(outside)); + fs.mkdirSync(path.join(outside, 'secret'), { recursive: true }); + fs.writeFileSync(path.join(outside, 'secret', 'leak.md'), catLine('dir/*.md') + '\n'); + + const wfDir = path.join(root, 'gsd-core', 'workflows'); + fs.mkdirSync(wfDir, { recursive: true }); + try { + fs.symlinkSync(path.join(outside, 'secret'), path.join(wfDir, 'escape'), 'dir'); + } catch { + t.skip('symlink creation not permitted in this environment'); + return; + } + + const { violations } = scanRepo(root); + assert.deepStrictEqual(violations, [], 'a directory symlink pointing outside the scan-dir root must not be followed'); + }); + + function writeBaselineFakeEmpty(root) { + const p = path.join(root, BASELINE_REL_PATH); + fs.mkdirSync(path.dirname(p), { recursive: true }); + fs.writeFileSync(p, JSON.stringify({ entries: [] }), 'utf8'); + } +}); + +// ─── Property tests (fast-check, pinned seed, bounded runs) ────────────── + +describe('Property tests', () => { + // DOCUMENT-SHAPED, not writer-seeded (CONTRIBUTING.md's Fixture provenance + // #2371): tokens are drawn from a shell-ish alphabet independent of + // PICK_RE/ECHO_FALLBACK_RE's own literals, not generated from the + // detector's regex source. + const wordArb = fc.constantFrom( + 'gsd_run', 'query', 'phases.list', 'config-get', 'k', 'v', 'f', '2>/dev/null', + 'echo', 'printf', '"0"', '"d"', '$(', ')', 'X=', '&&', ';', 'if', 'then', 'fi', + 'cat', 'ls', 'dir/*.md', '"$FILE"', '--type', 'summaries', '--pick', 'count', + ); + const lineArb = fc.array(wordArb, { minLength: 1, maxLength: 12 }).map((ws) => ws.join(' ')); + + test('F1: detector A never fires on a document-shaped line lacking --pick or lacking || echo', () => { + fc.assert( + fc.property(lineArb, fc.boolean(), (line, injectPipe) => { + // Build a line that deliberately lacks at least one of the two + // required tokens, without deriving the construction from + // PICK_RE/ECHO_FALLBACK_RE themselves. + const hasPick = line.includes('--pick'); + const rawFallback = injectPipe ? `${line} ${'|'}${'|'} echo done` : line; + const hasEcho = /\|\|\s*echo\b/.test(rawFallback); + fc.pre(!(hasPick && hasEcho)); + + const { violations } = findUnreachableGuardDrift(rawFallback, FAKE_FILE); + const aViolations = violations.filter((v) => v.kind === 'A'); + assert.deepStrictEqual(aViolations, []); + }), + { numRuns: 200, seed: 3409 }, + ); + }); + + // Baseline/violation record generators, independent of any production + // dedupe/diff code path. + const fileArb = fc.constantFrom('a.md', 'b.md', 'c.md'); + const textArb = fc.constantFrom('LINE_ONE', 'LINE_TWO', 'LINE_THREE'); + const countArb = fc.integer({ min: 1, max: 4 }); + const baselineEntryArb = fc.record({ file: fileArb, text: textArb, count: countArb }); + const baselineArb = fc.uniqueArray(baselineEntryArb, { + maxLength: 5, + selector: (e) => `${e.file} ${e.text}`, + }); + const violationArb = fc.record({ + file: fileArb, + text: textArb, + line: fc.integer({ min: 1, max: 500 }), + kind: fc.constantFrom('A', 'B'), + found: fc.constantFrom('--pick', 'cat', 'ls'), + }); + const violationsArb = fc.array(violationArb, { maxLength: 10 }); + + test('F2: diffAgainstBaseline is a partition — no violation is both fresh and stale, and an exact-count pair is neither', () => { + fc.assert( + fc.property(violationsArb, baselineArb, (violations, baseline) => { + const { fresh, stale } = diffAgainstBaseline(violations, baseline); + + // `fresh` items are always drawn from `violations` (they carry + // `line`/`kind`/`found`) and `stale` items are always drawn from + // `baseline` entries (they carry `actualCount`) — the two shapes + // are structurally disjoint, so no single object can satisfy both; + // asserted directly rather than by reference-equality (which two + // differently-shaped objects could never satisfy anyway). + for (const f of fresh) assert.ok('line' in f && 'actualCount' in f === false); + for (const s of stale) assert.ok('actualCount' in s && 'line' in s === false); + + // Every fresh violation's (file,text) pair is either unknown to the + // baseline, or known but this is one of the excess occurrences. + for (const f of fresh) { + const entry = baseline.find((e) => e.file === f.file && e.text === f.text); + if (entry) { + const actualCountForPair = violations.filter((v) => v.file === f.file && v.text === f.text).length; + assert.ok(actualCountForPair > entry.count, 'a fresh violation from a known pair must exceed its acknowledged count'); + } + } + + // A baseline entry whose actual count exactly matches its + // acknowledged count must not appear in stale. + for (const entry of baseline) { + const actualCountForPair = violations.filter((v) => v.file === entry.file && v.text === entry.text).length; + const inStale = stale.some((s) => s.file === entry.file && s.text === entry.text); + if (actualCountForPair === entry.count) { + assert.ok(!inStale, 'an exactly-matched baseline entry must not be reported stale'); + } else if (actualCountForPair < entry.count) { + assert.ok(inStale, 'an under-matched baseline entry must be reported stale'); + } + } + }), + { numRuns: 200, seed: 3410 }, + ); + }); +}); + +// ─── Integration — the real CLI and the real tree ───────────────────────── +// +// `main()` hardcodes its scan root to `path.join(__dirname, '..')` (see the +// sibling `lint-planning-prompt-drift.cjs`'s own E5 test, which writes its +// fixture straight into the real `gsd-core/workflows/` tree for exactly +// this reason) — an invoked CLI's root can never be redirected via `cwd`. +// A `--update` spawn of the REAL script would therefore overwrite the +// repo's own committed baseline, which "No test may mutate the repo's +// committed baseline file" forbids outright. `buildIsolatedGuard` copies +// the script AND its `./lib/drift-scan.cjs` dependency into a throwaway +// root, so the copy's own `__dirname` resolves inside the temp tree and +// every one of its filesystem effects (reads AND `--update`'s write) stay +// fully isolated — this is still the real CLI end-to-end, not a pure- +// function call, just running from a location that makes isolation +// possible. + +function buildIsolatedGuard(root) { + const scriptsDir = path.join(root, 'scripts'); + fs.mkdirSync(path.join(scriptsDir, 'lib'), { recursive: true }); + fs.copyFileSync(DRIFT_SCRIPT, path.join(scriptsDir, 'lint-unreachable-guard-drift.cjs')); + fs.copyFileSync( + path.join(REPO_ROOT, 'scripts', 'lib', 'drift-scan.cjs'), + path.join(scriptsDir, 'lib', 'drift-scan.cjs'), + ); + return path.join(scriptsDir, 'lint-unreachable-guard-drift.cjs'); +} + +describe('Integration — CLI end-to-end', () => { + test('C1a: the guard is green on the real committed tree (real CLI, real baseline, no isolation needed for a read-only run)', () => { + // A bare (no --update) run only READS the repo — no committed-baseline + // mutation risk — so this is the one CLI-level test that can safely + // target the real script directly, and is the matrix's literal claim: + // "run against the committed repo with the committed baseline -> exit 0". + const result = runNode([DRIFT_SCRIPT, '--json'], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(result.outcome, 'exited'); + assert.strictEqual(result.exitCode, 0, result.stderr); + const report = JSON.parse(result.stdout); + assert.strictEqual(report.reason, REASON.OK_NO_VIOLATIONS); + }); + + test('C1b: scanRepo(REPO_ROOT) carries no malformed gsd-scan-ignore declarations', () => { + const { malformed } = scanRepo(REPO_ROOT); + assert.deepStrictEqual(malformed, []); + }); + + test('C2: a fresh violation exits non-zero and the message names the remedy', (t) => { + const root = createTempDir('gsd-3409-c2-'); + t.after(() => cleanup(root)); + const isolatedScript = buildIsolatedGuard(root); + const wfDir = path.join(root, 'gsd-core', 'workflows'); + fs.mkdirSync(wfDir, { recursive: true }); + fs.writeFileSync(path.join(wfDir, 'fake.md'), `${pickEchoLine()}\n`); + const baselinePath = path.join(root, BASELINE_REL_PATH); + fs.mkdirSync(path.dirname(baselinePath), { recursive: true }); + fs.writeFileSync(baselinePath, JSON.stringify({ entries: [] }), 'utf8'); + + const result = runNode([isolatedScript, '--json'], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(result.outcome, 'exited'); + assert.strictEqual(result.exitCode, 1); + const report = JSON.parse(result.stdout); + assert.strictEqual(report.reason, REASON.FAIL_FRESH_VIOLATION); + assert.strictEqual(report.violations.length, 1); + assert.strictEqual(report.violations[0].file, FAKE_FILE); + assert.strictEqual(report.violations[0].kind, 'A'); + }); + + test('C3: --update regenerates a baseline that then passes', (t) => { + const root = createTempDir('gsd-3409-c3-'); + t.after(() => cleanup(root)); + const isolatedScript = buildIsolatedGuard(root); + const wfDir = path.join(root, 'gsd-core', 'workflows'); + fs.mkdirSync(wfDir, { recursive: true }); + fs.writeFileSync(path.join(wfDir, 'fake.md'), `${catLine('dir/*.md')}\n`); + + const first = runNode([isolatedScript, '--update'], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(first.exitCode, 0, first.stderr); + + const second = runNode([isolatedScript, '--json'], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(second.exitCode, 0, second.stderr); + const report = JSON.parse(second.stdout); + assert.strictEqual(report.reason, REASON.OK_NO_VIOLATIONS); + }); + + test('C4: --update output is deterministic across two runs', (t) => { + const root = createTempDir('gsd-3409-c4-'); + t.after(() => cleanup(root)); + const isolatedScript = buildIsolatedGuard(root); + const wfDir = path.join(root, 'gsd-core', 'workflows'); + fs.mkdirSync(wfDir, { recursive: true }); + fs.writeFileSync(path.join(wfDir, 'a.md'), `${catLine('dir/*.md')}\n`); + fs.writeFileSync(path.join(wfDir, 'b.md'), `${pickEchoLine()}\n`); + + runNode([isolatedScript, '--update'], { timeoutMs: PROBE_TIMEOUT_MS }); + const firstBaseline = fs.readFileSync(path.join(root, BASELINE_REL_PATH), 'utf8'); + runNode([isolatedScript, '--update'], { timeoutMs: PROBE_TIMEOUT_MS }); + const secondBaseline = fs.readFileSync(path.join(root, BASELINE_REL_PATH), 'utf8'); + assert.strictEqual(firstBaseline, secondBaseline); + }); + + test('C5: baseline entries are sorted by (file, text)', (t) => { + const root = createTempDir('gsd-3409-c5-'); + t.after(() => cleanup(root)); + const violations = [ + { file: 'z.md', line: 1, kind: 'B', found: 'cat', text: 'zzz' }, + { file: 'a.md', line: 1, kind: 'B', found: 'cat', text: 'zzz' }, + { file: 'a.md', line: 2, kind: 'B', found: 'cat', text: 'aaa' }, + ]; + const entries = writeBaseline(root, violations); + const keys = entries.map((e) => `${e.file} ${e.text}`); + const sortedKeys = [...keys].sort(); + assert.deepStrictEqual(keys, sortedKeys); + }); +}); + +// ─── dedupeViolationsForBaseline — count aggregation ────────────────────── + +describe('dedupeViolationsForBaseline', () => { + test('collapses byte-identical (file, text) pairs into one entry with a count', () => { + const violations = [ + { file: 'a.md', line: 1, kind: 'A', found: '--pick', text: 'X' }, + { file: 'a.md', line: 5, kind: 'A', found: '--pick', text: 'X' }, + { file: 'a.md', line: 9, kind: 'B', found: 'cat', text: 'Y' }, + ]; + const entries = dedupeViolationsForBaseline(violations); + assert.strictEqual(entries.length, 2); + const xEntry = entries.find((e) => e.text === 'X'); + assert.strictEqual(xEntry.count, 2); + const yEntry = entries.find((e) => e.text === 'Y'); + assert.strictEqual(yEntry.count, 1); + }); +}); + +// ─── Regex-level sanity (documents the two regexes' shapes directly) ───── + +describe('Regex shape sanity', () => { + test('PICK_RE matches only the literal --pick token', () => { + assert.ok(PICK_RE.test('--pick foo')); + assert.ok(!PICK_RE.test('--picky foo')); + }); + + test('ECHO_FALLBACK_RE matches || echo with optional interior whitespace', () => { + assert.ok(ECHO_FALLBACK_RE.test(['a ', '|', '|', ' echo b'].join(''))); + assert.ok(ECHO_FALLBACK_RE.test(['a ', '|', '|', ' echo b'].join(''))); + assert.ok(!ECHO_FALLBACK_RE.test(['a ', '|', '|', ' printf b'].join(''))); + }); + + test('CAT_LS_COMMAND_RE requires cat/ls immediately at a command-position anchor', () => { + assert.ok(CAT_LS_COMMAND_RE.test('cat x')); + assert.ok(CAT_LS_COMMAND_RE.test('$(cat x)')); + assert.ok(!CAT_LS_COMMAND_RE.test('concatenate x')); + assert.ok(!CAT_LS_COMMAND_RE.test('a cat b')); + }); + + test('HEREDOC_AFTER_COMMAND_RE matches a heredoc operator immediately after the command, with or without a leading space', () => { + assert.ok(HEREDOC_AFTER_COMMAND_RE.test(" <<'EOF'")); + assert.ok(HEREDOC_AFTER_COMMAND_RE.test('< { + // Subject hoisted to a named const rather than passed as a string + // literal directly to the .exec call below — scripts/prompt-injection-scan.sh's + // receiver-blind scan pattern (deliberately kept wide to catch the + // child_process module's exec function invoked with a string) flags a + // quote immediately following an open paren after the token `exec`, with + // no way to distinguish RegExp#exec from that shell-spawning call by + // pattern alone. Do not simplify this back. + const subject = '# gsd-scan-ignore: #3409 rationale'; + const m = MARKER_RE.exec(subject); + assert.ok(m); + assert.strictEqual(m[1], '#3409 rationale'); + }); +}); + +// ─── REASON enum — locks the typed outcome surface ──────────────────────── +// +// CONTRIBUTING.md's "Prohibited: Raw Text Matching on Test Outputs": adding a +// new reason requires updating the REASON enum, the --json emission / +// loadBaseline call site that produces it, AND this test — three coordinated +// changes that keep the code surface from drifting from the test surface. + +describe('REASON enum', () => { + test('the frozen enum exposes exactly the documented set of outcome codes', () => { + assert.deepStrictEqual(Object.keys(REASON).sort(), [ + 'FAIL_BASELINE_EMPTY', + 'FAIL_BASELINE_ENTRIES_NOT_ARRAY', + 'FAIL_BASELINE_ENTRY_COUNT_INVALID', + 'FAIL_BASELINE_ENTRY_FIELD_INVALID', + 'FAIL_BASELINE_ENTRY_NOT_OBJECT', + 'FAIL_BASELINE_INVALID_JSON', + 'FAIL_BASELINE_LOAD', + 'FAIL_BASELINE_MISSING', + 'FAIL_BASELINE_NOT_OBJECT', + 'FAIL_FRESH_VIOLATION', + 'FAIL_MALFORMED_MARKER', + 'FAIL_STALE_ENTRY', + 'OK_BASELINE_UPDATED', + 'OK_NO_VIOLATIONS', + ]); + }); + + test('the enum is frozen — an attempted mutation is a no-op (non-strict) / throws (strict)', () => { + assert.throws(() => { + 'use strict'; + REASON.FAIL_FRESH_VIOLATION = 'tampered'; + }, TypeError); + assert.strictEqual(REASON.FAIL_FRESH_VIOLATION, 'fail_fresh_violation'); + }); +}); diff --git a/tests/unreachable-shell-guard.test.cjs b/tests/unreachable-shell-guard.test.cjs new file mode 100644 index 000000000..563022763 --- /dev/null +++ b/tests/unreachable-shell-guard.test.cjs @@ -0,0 +1,379 @@ +// allow-test-rule: source-text-is-the-product — see #3409 +// Workflow markdown is the installed orchestration contract; the snippets +// below are extracted from the shipped .md files and EXECUTED (not +// re-typed), so the test binds to the deployed contract rather than a copy +// that could silently drift from it. + +'use strict'; + +/** + * Failing-first regression tests for #3409 (design: + * .gsd/phase/feat-3409-unreachable-shell-guard-lint/40-design.md; matrix: + * .gsd/phase/feat-3409-unreachable-shell-guard-lint/50-test-matrix.md, + * section "Regression — the three defects this PR fixes", rows G1-G4). + * + * Root cause (40-design.md): `gsd-tools.cjs`'s `--pick ` extractor + * coerces a missing/absent field to the empty string and exits 0. So + * `X=$(gsd_run query V --pick F 2>/dev/null || echo D)` can NEVER reach its + * `|| echo D` arm on field absence — only on a typo in the verb name. Three + * shipped shell guards silently rely on that unreachable arm: + * + * G1/G2 — plan-phase.md's Walking Skeleton gate reads a + * `phases.list --pick summaries_total` field that does not exist + * (#3365), so `PRIOR_SUMMARIES` is always `""`, never `"0"`, and + * the gate can never fire — not even for a genuinely fresh + * project (G1). G2 is the load-bearing negative-space case: it + * proves a bad fix that merely treats "no answer" as "zero" + * (making the gate fire unconditionally) is rejected, by pinning + * BOTH that the resolved count is a real nonzero integer AND that + * the gate stays off. + * G3 — plan-phase.md's `PHASE_REQ_IDS` site: on a phase with zero + * requirements, `query init.plan-phase --pick phase_req_ids` + * exits 0 with empty stdout, so `|| echo TBD` never fires and + * `PHASE_REQ_IDS` resolves to `""` instead of the documented + * `TBD` sentinel (gate step reads "Skip if phase_req_ids is null + * or TBD"). + * G4 — complete-milestone.md's bare `cat` over an unmatched-capable + * SUMMARY.md glob: under a `nullglob` + * left set by an earlier block in the SAME shell session (the + * `extract_accomplishments` step, a few hundred lines earlier in + * this same file), an unmatched glob expands to zero operands, + * so `cat` reads from stdin instead of erroring — and blocks + * forever if that stdin is not already at EOF. + * + * Each test below extracts the LIVE fenced-bash / single-line snippet out of + * the shipped workflow markdown (never a hand-typed copy — see + * `extractFencedBashAfterAnchor` / `extractAssignmentBlockFor`) and executes it + * with `runHook(..., { interpreter: 'bash' })` + * (`tests/helpers/process-seam.cjs`), against a temp project fixture, driving + * the real CLI at `gsd-core/bin/gsd-tools.cjs` through the real `gsd_run` + * shell function sourced from the shipped + * `gsd-core/workflows/_runtime-launcher.snippet.sh` preamble. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { createTempDir, cleanup, readWorkflowCombined } = require('./helpers.cjs'); +const { runHook, OUTCOME } = require('./helpers/process-seam.cjs'); +const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +const REPO_ROOT = path.join(__dirname, '..'); +const PLAN_PHASE_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'plan-phase.md'); +const COMPLETE_MILESTONE_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'complete-milestone.md'); +const LAUNCHER_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', '_runtime-launcher.snippet.sh'); +const GSD_TOOLS_PATH = path.join(REPO_ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'); + +// A `cat`-under-blocked-stdin hang (G4) must be bounded well under this, but +// give the CI-shape headroom PROBE_TIMEOUT_MS documents for a short CLI call. +const G4_TIMEOUT_MS = 5000; // short and explicit per the test-matrix note (G4 must assert on +// `outcome`, never `signal` — a real timeout and a maxBuffer overflow both +// report SIGTERM; PROBE_TIMEOUT_MS (15000ms) would work too but a tight, +// named bound makes a genuine hang fail fast instead of eating the suite's +// time budget on every RED run. + +// ─── extraction (source-text-is-the-product) ───────────────────────────── + +/** + * Extract the first ```bash fence appearing AFTER `anchor` in `content`. + * Mirrors the extraction convention already established by + * tests/plan-phase-stall-detection.test.cjs's extractStallHelpersBash(): walk + * forward from the anchor to the next fence open, then to its close. Throws + * with a message naming the anchor and file so a relocated/renamed anchor + * fails loudly instead of silently extracting the wrong block. + */ +function extractFencedBashAfterAnchor(content, anchor, sourcePath) { + const anchorIdx = content.indexOf(anchor); + if (anchorIdx === -1) { + throw new Error(`extractFencedBashAfterAnchor: could not find anchor "${anchor}" in ${sourcePath}`); + } + const after = content.slice(anchorIdx); + const fenceOpen = after.match(/```bash\r?\n/); + if (!fenceOpen) { + throw new Error(`extractFencedBashAfterAnchor: no \`\`\`bash fence found after anchor "${anchor}" in ${sourcePath}`); + } + const bodyStart = anchorIdx + fenceOpen.index + fenceOpen[0].length; + const closeIdx = content.indexOf('```', bodyStart); + if (closeIdx === -1) { + throw new Error(`extractFencedBashAfterAnchor: unterminated \`\`\`bash fence after anchor "${anchor}" in ${sourcePath}`); + } + return content.slice(bodyStart, closeIdx); +} + +/** + * Extract the CONTIGUOUS RUN of source lines beginning with `prefix` (e.g. + * `PHASE_REQ_IDS=`) — from the first matching line, keep consuming + * subsequent lines while they ALSO start with `prefix`, and join them with + * `\n`. A single-line extraction would silently test only half a + * multi-line contract (e.g. the capture line of `X=$(...)` / `X="${X:-D}"` + * without its fallback-default line), which is exactly the "guard that + * cannot observe its own failure" class this suite exists to catch. Throws + * with a message naming the prefix and file if no matching line is found, + * so a rename/relocation fails loudly rather than silently testing nothing. + */ +function extractAssignmentBlockFor(content, prefix, sourcePath) { + const lines = content.split('\n'); + const startIdx = lines.findIndex((l) => l.startsWith(prefix)); + if (startIdx === -1) { + throw new Error(`extractAssignmentBlockFor: no line starting with "${prefix}" found in ${sourcePath}`); + } + const block = []; + for (let i = startIdx; i < lines.length; i += 1) { + if (!lines[i].startsWith(prefix)) break; + block.push(lines[i]); + } + return block.join('\n'); +} + +// ─── shared bash-script runner ──────────────────────────────────────────── + +/** + * Write `script` to a fresh temp file and run it via the process seam's + * `runHook(..., { interpreter: 'bash' })` — a script PATH, not a `bash -c` + * argv string, matching tests/plan-phase-stall-detection.test.cjs's + * runBashScript() (#2650: a quote-dense multi-line script passed as a single + * `-c` argv element does not survive Windows argv serialization). + * + * @param {import('node:test').TestContext} t + * @param {string} script - full script body (a shebang + `set -e` are + * prepended). + * @param {object} [options] - forwarded to runHook (cwd, env, timeoutMs). + */ +function runBashScript(t, script, options = {}) { + const scriptDir = createTempDir('gsd-3409-sh-'); + t.after(() => cleanup(scriptDir)); + const scriptPath = path.join(scriptDir, 'script.sh'); + fs.writeFileSync(scriptPath, `#!/usr/bin/env bash\nset -e\n${script}`, { mode: 0o755 }); + return runHook(scriptPath, [], { interpreter: 'bash', ...options }); +} + +/** + * Parse `KEY=value` lines (one per line, as emitted by this file's own + * `echo "KEY=$VAR"` trailers) out of a script's stdout. Values may + * legitimately be the empty string (that IS the RED condition G1/G2/G3 + * assert against), so this returns `''` rather than `undefined` when the key + * is present with nothing after `=`. + */ +function parseKeyValueStdout(stdout) { + const result = {}; + for (const line of stdout.split('\n')) { + const eq = line.indexOf('='); + if (eq === -1) continue; + result[line.slice(0, eq)] = line.slice(eq + 1).replace(/\r$/, ''); + } + return result; +} + +// ─── fixtures ────────────────────────────────────────────────────────────── + +/** + * A minimal `.planning/phases/01-foundation/` project fixture — enough for + * `phases.list` and `init.plan-phase` to resolve phase 01 without error. + * `withSummary` seeds one real `*-SUMMARY.md` file when the negative-space + * (G2) case needs a nonzero prior-summary count. + */ +function buildPhase01Fixture({ withSummary }) { + const root = createTempDir('gsd-3409-fixture-'); + const phaseDir = path.join(root, '.planning', 'phases', '01-foundation'); + fs.mkdirSync(phaseDir, { recursive: true }); + if (withSummary) { + fs.writeFileSync(path.join(phaseDir, '01-01-SUMMARY.md'), '# Summary\n\nDone.\n'); + } + return root; +} + +// ─── G1 / G2 — Walking Skeleton gate (plan-phase.md, #3365) ─────────────── + +describe('#3409 G1/G2 — plan-phase.md Walking Skeleton gate observes a real summary count', () => { + const anchor = 'Walking Skeleton gate.'; + + function runWalkingSkeletonGate(t, projectRoot) { + const snippet = extractFencedBashAfterAnchor( + readWorkflowCombined(PLAN_PHASE_PATH), + anchor, + PLAN_PHASE_PATH, + ); + const script = [ + `. "${LAUNCHER_PATH}"`, + snippet, + 'echo "GSD_TEST_WALKING_SKELETON=$WALKING_SKELETON"', + 'echo "GSD_TEST_PRIOR_SUMMARIES=$PRIOR_SUMMARIES"', + ].join('\n'); + const result = runBashScript(t, script, { + cwd: projectRoot, + env: { + ...process.env, + RUNTIME_DIR: REPO_ROOT, + MVP_MODE: 'true', + padded_phase: '01', + }, + timeoutMs: PROBE_TIMEOUT_MS, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `gate script did not exit cleanly: ${result.stderr}`); + assert.equal(result.exitCode, 0, `gate script exited non-zero: ${result.stderr}`); + return parseKeyValueStdout(result.stdout); + } + + test('G1: zero prior summaries — the gate observes a real integer 0, and fires', (t) => { + const root = buildPhase01Fixture({ withSummary: false }); + t.after(() => cleanup(root)); + const { GSD_TEST_WALKING_SKELETON, GSD_TEST_PRIOR_SUMMARIES } = runWalkingSkeletonGate(t, root); + + // RED on the current tree: `--pick summaries_total` names a field that + // does not exist, so gsd-tools exits 0 with EMPTY stdout and the + // unreachable `|| echo "0"` arm never fires — PRIOR_SUMMARIES is `""`, + // not the integer `"0"` this asserts. + assert.equal(GSD_TEST_PRIOR_SUMMARIES, '0', 'prior-summary count must resolve to the integer 0, not empty string'); + assert.equal(GSD_TEST_WALKING_SKELETON, 'true', 'a fresh phase-01 project must enter Walking Skeleton mode'); + }); + + test('G2 (load-bearing negative space): a project WITH prior summaries does not enter skeleton mode', (t) => { + const root = buildPhase01Fixture({ withSummary: true }); + t.after(() => cleanup(root)); + const { GSD_TEST_WALKING_SKELETON, GSD_TEST_PRIOR_SUMMARIES } = runWalkingSkeletonGate(t, root); + + // RED on the current tree: PRIOR_SUMMARIES is `""` here too (same + // unreachable-arm defect), which fails this integer check even though + // WALKING_SKELETON happens to read 'false' on the current, doubly-broken + // gate (it never fires for ANY input). This is what rejects a bad fix + // that treats "no answer" as "zero": such a fix would make + // WALKING_SKELETON fire unconditionally, which the second assertion + // below also catches. + assert.match( + GSD_TEST_PRIOR_SUMMARIES, + /^[1-9][0-9]*$/, + `prior-summary count must resolve to a nonzero integer, got ${JSON.stringify(GSD_TEST_PRIOR_SUMMARIES)}`, + ); + assert.equal(GSD_TEST_WALKING_SKELETON, 'false', 'a project with prior summaries must NOT enter Walking Skeleton mode'); + }); +}); + +// ─── G3 — PHASE_REQ_IDS falls back to TBD (plan-phase.md) ───────────────── + +test('#3409 G3: an empty phase_req_ids falls back to TBD, not the empty string', (t) => { + const root = createTempDir('gsd-3409-g3-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, '.planning', 'phases', '01-foundation'), { recursive: true }); + // Deliberately no REQUIREMENTS.md / ROADMAP.md — phase 01 with zero + // requirements mapped to it, so `init.plan-phase --pick phase_req_ids` + // resolves `phase_req_ids: null` and `--pick` renders that as empty stdout + // (probe-confirmed: exit 0, empty stdout). + + const block = extractAssignmentBlockFor( + readWorkflowCombined(PLAN_PHASE_PATH), + 'PHASE_REQ_IDS=', + PLAN_PHASE_PATH, + ); + const script = [ + `. "${LAUNCHER_PATH}"`, + block, + 'echo "GSD_TEST_PHASE_REQ_IDS=$PHASE_REQ_IDS"', + ].join('\n'); + const result = runBashScript(t, script, { + cwd: root, + env: { ...process.env, RUNTIME_DIR: REPO_ROOT, PHASE: '01' }, + timeoutMs: PROBE_TIMEOUT_MS, + }); + assert.equal(result.outcome, OUTCOME.EXITED, `PHASE_REQ_IDS script did not exit cleanly: ${result.stderr}`); + assert.equal(result.exitCode, 0, `PHASE_REQ_IDS script exited non-zero: ${result.stderr}`); + + const { GSD_TEST_PHASE_REQ_IDS } = parseKeyValueStdout(result.stdout); + // RED on the current tree: `--pick phase_req_ids` exits 0 with empty + // stdout on a `null` field, so the unreachable `|| echo TBD` arm never + // fires and PHASE_REQ_IDS resolves to `""` instead of the documented + // `TBD` sentinel (plan-phase.md: "Skip if phase_req_ids is null or TBD"). + assert.equal(GSD_TEST_PHASE_REQ_IDS, 'TBD'); +}); + +// ─── G4 — complete-milestone.md bare `cat ` does not block on stdin ── + +test('#3409 G4: the milestone summary read does not hang with no summaries', (t) => { + // The blocked-stdin mechanism below is a read-write FIFO opened via + // `mkfifo` — POSIX-only, and unavailable/non-functional on the + // `windows-latest` CI lane. Under `set -e` an unsupported `mkfifo` fails + // the script during setup, before the `cat` under test ever runs, so a + // Windows run would exercise nothing and must be skipped, not weakened. + if (process.platform === 'win32') { + t.skip('mkfifo-blocked-stdin reproduction is POSIX-only; unreachable on Windows'); + return; + } + + const root = createTempDir('gsd-3409-g4-'); + t.after(() => cleanup(root)); + // Zero-summary milestone: a phase dir exists, but no *-SUMMARY.md file + // anywhere under it — the exact condition that makes the glob unmatched. + fs.mkdirSync(path.join(root, '.planning', 'phases', '01-foundation'), { recursive: true }); + + const snippet = extractFencedBashAfterAnchor( + readWorkflowCombined(COMPLETE_MILESTONE_PATH), + 'Read all phase summaries:', + COMPLETE_MILESTONE_PATH, + ); + + // Two things this script must reproduce, both faithfully, neither + // confounded with the other: + // + // 1. `nullglob` set — not by this fenced block itself (it sets nothing), + // but by an EARLIER block in the SAME workflow file/shell session + // (`extract_accomplishments`'s `shopt -s nullglob`, a few hundred + // lines above this one). 40-design.md's B11 names this exact + // "latent option from a different block" hazard as why Detector B + // flags this site even though it never sets the option locally. + // 2. stdin genuinely blocked, not just closed. Node's spawnSync closes + // an unwritten stdin immediately (EOF) when no `input` option is + // given, which would make a zero-operand `cat` return instantly + // instead of reproducing the real hang — so this opens a FIFO + // read-write on fd 3 (a read-write open never sees EOF, because the + // process holds its own write end) and redirects fd 0 there. This + // avoids `<(process substitution)`, which would leave a background + // job holding the CAPTURED STDOUT pipe open instead — a different, + // confounding hang unrelated to the stdin defect under test. The FIFO + // lives inside a private `mktemp -d` directory (created atomically + // with mode 0700) rather than at a bare `mktemp -u` path: `-u` only + // RESERVES a name without creating it, leaving a window between the + // reservation and `mkfifo` in which another process on a shared /tmp + // could create that same path first (a symlink-race primitive) — the + // directory removes the race entirely. + const script = [ + 'shopt -s nullglob', + 'FIFO_DIR=$(mktemp -d)', + 'mkfifo "$FIFO_DIR/f"', + 'exec 3<> "$FIFO_DIR/f"', + 'rm -rf "$FIFO_DIR"', + 'exec 0<&3', + snippet, + ].join('\n'); + + const result = runBashScript(t, script, { cwd: root, timeoutMs: G4_TIMEOUT_MS }); + + // RED on the current tree: the bare `cat ` reads from the blocked + // stdin and never returns within G4_TIMEOUT_MS, so `outcome` is + // TIMED_OUT. Asserting on `outcome` (never `signal`) per CONTRIBUTING.md's + // process-seam guidance — a timeout and a maxBuffer overflow both report + // SIGTERM, and only `outcome` discriminates them. + assert.equal( + result.outcome, + OUTCOME.EXITED, + `expected the summary read to complete, got outcome=${result.outcome} stderr=${result.stderr}`, + ); + // A nonzero exit here means the script's own setup (mkfifo/exec/mktemp) + // failed under `set -e` and the process exited immediately — which also + // reports outcome=EXITED, so it would silently pass the assertion above + // without ever reaching the `cat` under test. Pinning exitCode===0 + // distinguishes "setup failed" from "the blocked read actually completed". + assert.equal( + result.exitCode, + 0, + `expected setup (mkfifo/exec/mktemp) to succeed and the read to complete cleanly, got exitCode=${result.exitCode} stderr=${result.stderr}`, + ); +}); + +// Sanity: the module under test actually exists at the path every fixture +// above points `RUNTIME_DIR`/`gsd_run` at — a moved/renamed CLI would +// otherwise make every test above fail with a confusing "gsd-tools.cjs not +// found" error deep inside a bash script instead of a clear assertion here. +test('#3409: gsd-tools.cjs exists at the path this suite drives gsd_run through', () => { + assert.equal(fs.existsSync(GSD_TOOLS_PATH), true, `expected ${GSD_TOOLS_PATH} to exist`); +});