diff --git a/.gitattributes b/.gitattributes index d7e61f3e..a48edec6 100644 --- a/.gitattributes +++ b/.gitattributes @@ -17,6 +17,21 @@ # The umbrella also re-covers git-hooks/lib/*.sh already caught by *.sh; harmless. git-hooks/** text eol=lf +# BUG-039: the other extensionless-but-text groups in the tree, same reasoning +# as git-hooks/** above. `dotf init`'s templates are go:embed'd into the CLI +# binary at build time, so a CRLF checkout here means every repo `dotf init` +# scaffolds inherits a CRLF Makefile/gitignore/claude-md on a Windows build. +# Golden-corpus fixtures under tests/golden/ are compared byte-for-byte against +# script output, so a CRLF checkout would also break the comparison itself, not +# just the shebang class of failure. A regression test (tests/gitattributes- +# eol.bats) asserts every tracked extensionless text file resolves an explicit +# eol, so the next one added outside these groups fails loudly instead of +# silently depending on which OS someone happens to be checking out on. +cli/internal/initrepo/templates/** text eol=lf +tests/golden/** text eol=lf +ssh/config text eol=lf +LICENSE text eol=lf + # Windows shells: native tooling expects CRLF. *.ps1 text eol=crlf *.psm1 text eol=crlf diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e72a8281..db9832f7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -52,7 +52,12 @@ repos: - id: sdd-spec-gate name: SDD spec-gate (Tier 4) - entry: ./scripts/check-spec-gate.sh + # BUG-061: routed through the prepush adapter, not check-spec-gate.sh + # directly -- the adapter resolves this branch's live PR (labels/body/ + # author) via `gh` when one exists, so archive-on-merge is credited + # locally instead of only after push. Falls through to the gate with no + # PR context (today's behaviour) when there is none yet. + entry: ./scripts/spec-gate-prepush.sh args: ['--base-ref', 'origin/main', '--head-ref', 'HEAD'] language: script pass_filenames: false diff --git a/docs/lessons.md b/docs/lessons.md index c4c95ca2..99d57d13 100644 --- a/docs/lessons.md +++ b/docs/lessons.md @@ -2235,6 +2235,30 @@ The blast radius was also wider than the one function: because `compile-harness. **Tags**: `git`, `github`, `workflow`, `worktree` +### [2026-08-12] A "multi-call binary" bug report can name the wrong mechanism — verify the dispatch, not just the symptom + +**Context**: BUG-054, fixing `tests/install-dotf.bats`'s busy-binary fixture so the ETXTBSY swap path it claims to exercise is actually reached. The filed issue diagnosed the root cause precisely — `sleep` copied to a file named `dotf` exits immediately instead of sleeping on a multi-call coreutils build — and proposed a fix: `exec -a sleep "$0" 30` to hand the copy the argv[0] the dispatcher expects while the file on disk keeps the name `install_dotf` needs to swap. + +**Problem**: the proposed fix does not work on this machine, and the reason is that "multi-call binary" names a shape, not a single mechanism. GNU coreutils' traditional multi-call dispatch reads argv[0], which is exactly what `exec -a` controls — but this system's `sleep` is `uutils coreutils` (the Rust reimplementation), confirmed by `sleep --version`. Direct testing (`/proc/$PID/cmdline` right after `exec -a sleep ... &`) showed the override reaching the process correctly, yet the copy still refused with `unknown program 'dotf'`. Invoking the SAME bytes at their original resolved path dispatched correctly regardless of argv[0]; invoking the copy at a different path failed regardless of argv[0]. That is only consistent with dispatch keyed on the executable's own resolved path (`current_exe()`/`/proc/self/exe`), not on argv[0] at all — the opposite axis from what the issue assumed and what `exec -a` can influence. + +**Solution**: stop trying to satisfy whichever dispatch mechanism a given coreutils build uses, and stand in a binary that has no multi-call dispatch to satisfy in the first place. A copy of `bash`, driven by `"$DEST/dotf" -c 's=$SECONDS; while (( SECONDS - s < 30 )); do :; done'`, busy-spins using only shell builtins — no forked or exec'd child, so the copied ELF's own text image stays open for the whole duration regardless of what its filename is or which coreutils flavor is on the host. Verified before wiring into the test: copying bash to a renamed file and running it directly reproduced ETXTBSY on a second `cp` onto it while busy, confirming the mechanism independent of any dispatch behavior. + +**Rule**: a bug report's stated root cause is a hypothesis until it reproduces on the machine actually running the fix, even when the diagnosis reads as obviously correct and even when it comes with a plausible-looking patch. "Multi-call binary" covers at least two different dispatch strategies (argv[0]-keyed, as GNU coreutils and busybox use; resolved-path-keyed, as observed here in uutils coreutils) that look identical from a bug report's black-box symptom but demand opposite fixes. When a fixture needs to hold a *real* executable text image open/busy without depending on a specific tool's argv0-vs-path dispatch behavior, prefer a binary with no multi-call ambiguity at all (a shell, driven by a builtin-only blocking loop) over trying to satisfy whichever mechanism the host happens to implement. + +**Tags**: `testing`, `bash`, `coreutils`, `debugging` + +### [2026-08-12] "Weaker locally, CI catches it" is not safe when local and CI share the same script + +**Context**: BUG-061, fixing a spec-gate false negative where a PR that correctly archived its spec in the same change could not be pushed locally — `check-spec-gate.sh`'s archive-on-merge credit only fires when `SDD_PR_BODY` names a closing keyword, and that variable is empty on every local pre-push run by design. The filed issue offered three fix options; option 2 read "credit an archive move unconditionally in the pre-push tier... weaker, but the #397 protection against a gratuitous archive-move dodging the gate still holds in CI." + +**Problem**: option 2 was implemented as stated — locally (empty `SDD_PR_BODY`), any spec that genuinely transitioned from active-at-base to archived-at-head was credited toward the LOC-threshold's "spec folder touched" requirement, without needing a closing-issue link. Two existing regression tests (`tests/check-spec-gate.bats`, the #397 pair) immediately went red: a PR bundling 60 LOC of unrelated production code with a genuine archive of an unrelated spec now passed locally, exactly the "gratuitous archive-move dodging the gate" #397 already closed. The flaw in the reasoning: "CI catches it" only holds when local and CI run *different* logic. Here they run the *same* `check-spec-gate.sh`, gated only by whether `SDD_PR_BODY` happens to be set — so a check made unconditionally permissive "when there's no PR body" is exactly as permissive in CI whenever CI itself has no PR body to supply (or, more subtly, it normalizes local runs to a genuinely weaker invariant than the one the script's own tests pin, which is a contradiction the tests exist to catch). A diff-only heuristic cannot distinguish #854's legitimate author (archiving the spec they are actually implementing) from #397's attacker (archiving something unrelated to bulk-legitimize other code) — their diffs are structurally identical, and no amount of "was this transition genuine" narrowing closes that gap without the PR-body linkage the local run doesn't have. + +**Solution**: discard the local-credit approach entirely and implement option 1 instead — `scripts/spec-gate-prepush.sh`, a wrapper mirroring the existing CI adapter (`spec-gate-pr.sh`) that resolves the current branch's live PR via `gh pr view` (no token needed locally; the developer's own `gh auth`) and forwards real `SDD_LABELS`/`SDD_PR_BODY`/`SDD_PR_AUTHOR` to the unmodified gate. Unlike its CI sibling, it falls THROUGH to running the gate with no PR context on any resolution failure (no `gh`, no `jq`, unauthenticated, no PR open yet) rather than failing closed — "no PR context" is the ordinary local baseline here, not a stale-data hazard to guard against. `check-spec-gate.sh` itself needed no change beyond a message pointing at the wrapper and the manual `SDD_PR_BODY=...` override. + +**Rule**: when a fix option is phrased as "loosen X here, Y elsewhere still catches it," check first whether "elsewhere" is actually a different code path or just the same code path under different environment variables — if a script is shared between an advisory local tier and an authoritative CI tier, any relaxation gated only on which env vars happen to be set is live everywhere those vars happen to be unset, not only in the tier the fix was aimed at. The repo's own existing regression tests caught this on the first test run after implementing the "obvious" reading of the option — proof that re-running the FULL adjacent test suite (not just new tests for the change at hand) before trusting an implementation is itself the guard. + +**Tags**: `testing`, `shell`, `sdd`, `ci` + ### [2026-08-12] "Looks like a known bug class" is a hypothesis, not a finding — reproduce before you fix **Context**: HARNESS-070 (deploy convergence, #843/#869/#828). The session's brief carried live evidence: `dotf doctor` flagging 4 deployed skills (`computer-use`, `find-skills`, `orca-cli`, `orchestration`) as symlinks, labeled as a "BUG-100" regression — the historical, closed issue #100 about this repo's own deploy strategy fighting `agy`'s filesystem layout. diff --git a/scripts/check-spec-gate.sh b/scripts/check-spec-gate.sh index 2d6983e2..2ddb6eef 100755 --- a/scripts/check-spec-gate.sh +++ b/scripts/check-spec-gate.sh @@ -35,8 +35,12 @@ BASE_REF="" HEAD_REF="" EXPLAIN=0 # Path to the open-issue feed for the advisory adjacency report (HARNESS-063). -# Empty on every local run: the fetch needs a token, so it lives in the workflow -# and this script stays offline for the pre-push hook (#854). +# Empty on every local run: fetching every open issue needs a token, so this +# feed is populated by the workflow only -- this script itself never fetches +# anything. (Distinct from BUG-061/#854's PR-body/labels/author resolution: +# that is a single PR read, needs no token, and is handled by the separate +# scripts/spec-gate-prepush.sh wrapper on the local pre-push tier -- see its +# header for why it lives outside this script rather than inside it.) ADJACENCY_ISSUES="" usage() { @@ -715,6 +719,12 @@ cat >&2 </dev/null 2>&1 || _run_gate "$@" +command -v jq >/dev/null 2>&1 || _run_gate "$@" + +if ! meta=$(gh pr view --json labels,body,author 2>/dev/null); then + # No PR for this branch yet, or the developer is unauthenticated. Both are + # the ordinary local baseline, not an error this wrapper reports on. + _run_gate "$@" +fi + +# `// ""` on both scalars: gh returns JSON null for an empty body, and the +# literal string "null" reaching the gate would match headings and label +# substrings that were never there. +SDD_LABELS=$(printf '%s' "$meta" | jq -r '[.labels[].name] | join(",")') +SDD_PR_BODY=$(printf '%s' "$meta" | jq -r '.body // ""') +SDD_PR_AUTHOR=$(printf '%s' "$meta" | jq -r '.author.login // ""') +export SDD_LABELS SDD_PR_BODY SDD_PR_AUTHOR + +_run_gate "$@" diff --git a/tests/gitattributes-eol.bats b/tests/gitattributes-eol.bats new file mode 100644 index 00000000..30497065 --- /dev/null +++ b/tests/gitattributes-eol.bats @@ -0,0 +1,81 @@ +#!/usr/bin/env bats +# BUG-039: git-hooks/** got an explicit `text eol=lf` rule after a CRLF +# checkout disabled the GUARD-001 dispatcher on Windows (BUG-068) -- the shebang +# became "#!/usr/bin/env bash\r" and every hook died "No such file or +# directory". That fix covered one group of extensionless files. This is the +# CLASS-LEVEL guard: every tracked extensionless text file, repo-wide, must +# resolve an explicit eol, so the next one added outside an already-covered +# group (cli/internal/initrepo/templates/**, tests/golden/**, ssh/config, +# LICENSE) fails loudly here instead of depending on which OS happens to check +# it out. + +setup() { + REPO="$BATS_TEST_DIRNAME/.." +} + +# Tracked files whose basename has no '.' -- the shape .gitattributes' *.sh/ +# *.bash/*.bats extension-keyed rules cannot match by construction, and the +# reason every file in this class needs an explicit rule of its own. +_extensionless_tracked_files() { + git -C "$REPO" ls-files | awk -F/ '{n=$NF; if (n !~ /\./) print $0}' +} + +@test "every tracked extensionless file has an explicit eol (or is declared binary)" { + cd "$REPO" || return 1 + local files + files=$(_extensionless_tracked_files) + # A fixture-drift guard on the guard itself: if this ever finds NOTHING, + # the class the test protects has vanished from the repo and the test + # would pass vacuously -- the exact failure mode this whole PR is about. + [ -n "$files" ] || { + printf 'No tracked extensionless files found. Either the repo layout\n' >&2 + printf 'changed (update this test) or something upstream is broken.\n' >&2 + return 1 + } + + # One process for the whole set (not one `check-attr` per file): --stdin + # emits ": : " per attribute per path, two lines per + # file for the two attributes requested here. + local -A eol_of text_of + local line path rest attr val + while IFS= read -r line; do + path="${line%%: *}" + rest="${line#*: }" + attr="${rest%%: *}" + val="${rest#*: }" + case "$attr" in + eol) eol_of["$path"]="$val" ;; + text) text_of["$path"]="$val" ;; + esac + done < <(printf '%s\n' "$files" | git check-attr --stdin text eol) + + # "unspecified" eol is only acceptable when text itself is explicitly + # unset (`binary`) -- a deliberate escape hatch for a future extensionless + # fixture that genuinely is binary, not a way to silence this guard for a + # text file someone forgot to cover. + local unresolved=() p + for p in "${!eol_of[@]}"; do + if [ "${eol_of[$p]}" = "unspecified" ] && [ "${text_of[$p]:-}" != "unset" ]; then + unresolved+=("$p") + fi + done + + if [ ${#unresolved[@]} -ne 0 ]; then + printf 'Tracked extensionless files with no explicit eol -- a Windows\n' >&2 + printf 'checkout CRLFs their shebang or fixture comparison (BUG-039/BUG-068):\n' >&2 + printf '%s\n' "${unresolved[@]}" | sort | while IFS= read -r f; do printf ' %s\n' "$f" >&2; done + printf '\nAdd an explicit `text eol=lf` (or `binary`) rule in .gitattributes.\n' >&2 + return 1 + fi +} + +@test "the five GUARD-001 hook dispatchers resolve eol=lf specifically" { + # AC1: distinct from the class guard above -- this pins the exact value, + # not just "something explicit", for the dispatchers whose CRLF breakage + # (BUG-068) is what motivated the class guard in the first place. + local f + for f in pre-commit pre-push commit-msg post-checkout prepare-commit-msg; do + run git -C "$REPO" check-attr eol -- "git-hooks/$f" + [[ "$output" == *": eol: lf" ]] + done +} diff --git a/tests/install-dotf.bats b/tests/install-dotf.bats index 70406405..97b71da8 100644 --- a/tests/install-dotf.bats +++ b/tests/install-dotf.bats @@ -97,11 +97,26 @@ EOF mkdir -p "$DEST" # A shell script never triggers ETXTBSY (the kernel does not hold it as an # executable text image), so stand in a real ELF and keep it running. - cp "$(command -v sleep)" "$DEST/dotf" + # `sleep` itself does NOT work for this on every coreutils build (BUG-054): + # on a multi-call `coreutils` binary the applet dispatch can key off the + # RESOLVED EXECUTABLE PATH rather than argv[0] (observed with uutils + # coreutils -- `exec -a sleep` changes argv[0] but the copy still dispatches + # by its own path and refuses "unknown program"), so no argv[0] trick + # recovers it once the file has been copied to a path named "dotf". A copy + # of `bash` sidesteps the whole dispatch question: it is never a multi-call + # binary, and a builtin busy-loop (no forked/exec'd child) keeps THIS COPY's + # own executable text image open for the swap to contend with. + cp "$(command -v bash)" "$DEST/dotf" chmod 0755 "$DEST/dotf" - "$DEST/dotf" 30 & + "$DEST/dotf" -c 's=$SECONDS; while (( SECONDS - s < 30 )); do :; done' & BUSY_PID=$! + # A fixture that failed to hold the binary busy must fail loudly here, at + # setup, instead of silently degrading the rest of the test into a check of + # the ordinary replace-a-file path (BUG-054). + sleep 0.2 + kill -0 "$BUSY_PID" + run install_dotf "$VERSION" "$DEST" "$BASE" [ "$status" -eq 0 ] diff --git a/tests/spec-gate-adjacency.bats b/tests/spec-gate-adjacency.bats index 54d9e074..3531d47a 100644 --- a/tests/spec-gate-adjacency.bats +++ b/tests/spec-gate-adjacency.bats @@ -138,8 +138,13 @@ _write_849_feed() { } @test "adjacency: with no flag the output is byte-identical to the previous version" { - # Characterization test (#672): the offline pre-push path (#854) must be - # untouched. Compares against the script as it stands on origin/main. + # Characterization test (#672): adding --adjacency-issues support did not + # silently change the script's existing offline output when the flag goes + # unused. #854/BUG-061 later added a documented local escape hatch + # (SDD_PR_BODY) to the LOC-gate failure message -- an intentional, in-scope + # change to this exact path, not a regression -- so that block is stripped + # from both sides before comparing; everything else must still match + # origin/main byte for byte. if ! git -C "$BATS_TEST_DIRNAME/.." rev-parse --verify origin/main >/dev/null 2>&1; then skip "origin/main not fetched in this environment" fi @@ -154,7 +159,14 @@ _write_849_feed() { SDD_PR_BODY="Closes #850" \ run "$SCRIPTS_DIR/check-spec-gate.sh" --base-ref main --head-ref feature --explain [ "$status" -eq "$baseline_status" ] - [ "$output" = "$baseline_output" ] + + _strip_escape_hatch() { + sed '/^ If this already archived a spec/,/^ be set by hand for a one-off check/d' | cat -s + } + local stripped_current stripped_baseline + stripped_current=$(printf '%s\n' "$output" | _strip_escape_hatch) + stripped_baseline=$(printf '%s\n' "$baseline_output" | _strip_escape_hatch) + [ "$stripped_current" = "$stripped_baseline" ] } @test "adjacency: --help documents the flag" { diff --git a/tests/spec-gate-prepush-real.bats b/tests/spec-gate-prepush-real.bats new file mode 100644 index 00000000..85a35575 --- /dev/null +++ b/tests/spec-gate-prepush-real.bats @@ -0,0 +1,80 @@ +#!/usr/bin/env bats +# Real-dependency sibling of spec-gate-prepush.bats (see tests/stub-real-pairing.bats). +# +# The stub suite proves what we ASKED `gh` to do. It cannot prove that `gh` +# accepts it -- and the likeliest way scripts/spec-gate-prepush.sh breaks in +# production is a --json field name that is wrong or renamed upstream, which a +# stub answering any invocation will never surface. That is the BUG-055 shape +# exactly: 15 green cases over a branch the real tool rejected. +# +# `gh pr view --json ` validates field names locally, BEFORE it +# resolves a repository or a branch's PR, so the contract can be pinned with +# the real binary and no token, no network and no live PR. + +# A real-dependency suite that quietly skips is a green proving nothing +# (BUG-055, #807). So a missing precondition is FATAL under CI -- where the +# workflow supplies the token -- and only relaxed on a dev machine. +_need() { + [ -z "${CI:-}" ] && skip "$1" + printf 'real-dependency precondition unmet in CI: %s\n' "$1" >&2 + return 1 +} + +setup() { + command -v gh >/dev/null || _need "gh is not installed" + + # Scoped to THIS suite rather than exporting GH_TOKEN for the whole bats + # run: every other suite keeps the auth state it has today, so none of them + # can start making real API calls as a side effect of this file. + [ -n "${DOTF_TEST_GH_TOKEN:-}" ] && export GH_TOKEN="$DOTF_TEST_GH_TOKEN" + + # An UNAUTHENTICATED gh exits at the auth check before it ever validates + # --json field names. Without this precondition the field tests below assert + # nothing while still reporting green -- which is precisely how spec-gate- + # pr-real.bats's equivalent test passed locally (authenticated) and failed + # in CI (not) on its first run. + gh auth status >/dev/null 2>&1 || _need "gh is not authenticated" + + SCRIPTS_DIR="$BATS_TEST_DIRNAME/../scripts" + OUTSIDE="/tmp/bats_specgateprepush_real_$$_${BATS_TEST_NUMBER:-0}" + mkdir -p "$OUTSIDE" + # Nothing here may resolve to a repository: these tests must fail on repo + # resolution, never on a real API call. + unset GH_REPO GITHUB_REPOSITORY +} + +teardown() { + cd / || true + rm -rf "$OUTSIDE" +} + +@test "real gh accepts every --json field the adapter asks for, with no PR number" { + cd "$OUTSIDE" || return 1 + run gh pr view --json labels,body,author + # It must fail (there is no repo here) but NOT because of the field names. + [ "$status" -ne 0 ] + [[ "$output" != *"Unknown JSON field"* ]] +} + +@test "the field check is a detector, not a tautology" { + cd "$OUTSIDE" || return 1 + run gh pr view --json labels,body,author,notAFieldName + [ "$status" -ne 0 ] + [[ "$output" == *"Unknown JSON field"* ]] + [[ "$output" == *"notAFieldName"* ]] +} + +@test "a real gh failure falls through to the gate instead of failing closed" { + cd "$OUTSIDE" || return 1 + # Real gh, real failure (no repo to resolve, so no PR to find). Unlike + # spec-gate-pr.sh (which must fail closed in CI to avoid a stale event- + # payload replay, BUG-066), this adapter has no such fallback to guard + # against: it runs the gate with no PR context, same as it would with no + # `gh` installed at all. + run "$SCRIPTS_DIR/spec-gate-prepush.sh" --base-ref origin/main --head-ref HEAD + # No git repo here either, so check-spec-gate.sh itself now fails (exit 2, + # "Not in a git repo") -- proof the adapter reached it and ran the gate + # rather than swallowing the gh failure into some exit code of its own. + [ "$status" -eq 2 ] + [[ "$output" == *"Not in a git repo"* ]] +} diff --git a/tests/spec-gate-prepush.bats b/tests/spec-gate-prepush.bats new file mode 100644 index 00000000..f084b195 --- /dev/null +++ b/tests/spec-gate-prepush.bats @@ -0,0 +1,170 @@ +#!/usr/bin/env bats +# Tests for scripts/spec-gate-prepush.sh (BUG-061). +# +# The defect this exists to prevent: check-spec-gate.sh's archive-on-merge +# credit only fires when SDD_PR_BODY names a closing keyword, and that variable +# is empty on every local pre-push run by design. A PR that correctly archived +# its spec in the same change therefore could not be pushed -- the LOC gate saw +# no active-spec touch and rejected a change the author had done exactly right. +# +# Unlike scripts/spec-gate-pr.sh (its CI sibling, which fails CLOSED on a bad +# live read to avoid deciding on a stale event-payload replay -- BUG-066), this +# adapter falls THROUGH to running the gate with no PR context on any failure +# to resolve one. "No PR context" is the ordinary local baseline, not an error +# condition, so a missing `gh`/`jq`, an unauthenticated `gh`, or no PR open yet +# for the branch must all degrade to today's exact behaviour rather than block. + +setup() { + SCRIPTS_DIR="$BATS_TEST_DIRNAME/../scripts" + FIX="/tmp/bats_specgateprepush_$$_${BATS_TEST_NUMBER:-0}" + mkdir -p "$FIX/bin" + + # The adapter execs check-spec-gate.sh from its OWN directory, so the fake + # gate must sit beside a COPY of the adapter -- putting it on PATH would not + # be reached, and that is the property we want to keep true. + cp "$SCRIPTS_DIR/spec-gate-prepush.sh" "$FIX/bin/spec-gate-prepush.sh" + ADAPTER="$FIX/bin/spec-gate-prepush.sh" + + # Present even under a restricted PATH: the adapter's `#!/usr/bin/env bash` + # shebang has `env` look `bash` up on PATH, and `dirname`/`cat` are the two + # external tools the adapter itself shells out to (SCRIPT_DIR resolution, + # the --help text). The missing-gh/missing-jq tests below deliberately + # shrink PATH to just this directory, so all three must live here too, or + # they would fail on "command not found" rather than on the thing under test. + for _tool in bash dirname cat; do + ln -sf "$(command -v "$_tool")" "$FIX/bin/$_tool" + done + + GATE_LOG="$FIX/gate.log" + export GATE_LOG + + # Gate stub: records the forwarded argv and the three env vars it was handed. + cat > "$FIX/bin/check-spec-gate.sh" <<'STUB' +#!/usr/bin/env bash +{ + printf 'ARGS=[%s]\n' "$*" + printf 'SDD_LABELS=[%s]\n' "${SDD_LABELS-}" + printf 'SDD_PR_AUTHOR=[%s]\n' "${SDD_PR_AUTHOR-}" + printf 'SDD_PR_BODY=[%s]\n' "${SDD_PR_BODY-}" +} >> "$GATE_LOG" +exit "${STUB_GATE_RC:-0}" +STUB + chmod +x "$FIX/bin/check-spec-gate.sh" + + # gh stub: no --pr number is ever passed by this adapter, so record the + # exact argv to prove that. STUB_PR_JSON is the answer, STUB_GH_RC forces + # the no-PR-for-this-branch failure path. + cat > "$FIX/bin/gh" <<'STUB' +#!/usr/bin/env bash +printf 'GH_ARGS=[%s]\n' "$*" >> "$GH_LOG" +if [[ "${STUB_GH_RC:-0}" -ne 0 ]]; then + printf 'no pull requests found for branch\n' >&2 + exit "$STUB_GH_RC" +fi +printf '%s' "$STUB_PR_JSON" +STUB + chmod +x "$FIX/bin/gh" + + GH_LOG="$FIX/gh.log" + export GH_LOG + PATH="$FIX/bin:$PATH" + export PATH + export STUB_PR_JSON='{"labels":[],"body":null,"author":{"login":"mlorentedev"}}' +} + +teardown() { + cd / || true + rm -rf "$FIX" +} + +@test "spec-gate-prepush: derives labels, body and author from the live gh read" { + export STUB_PR_JSON='{"labels":[{"name":"skip-archive"},{"name":"bug"}],"body":"## Archive skip rationale\n\nThe spec stays active on purpose.","author":{"login":"mlorentedev"}}' + run "$ADAPTER" --base-ref origin/main --head-ref HEAD --explain + [ "$status" -eq 0 ] + grep -qF 'SDD_LABELS=[skip-archive,bug]' "$GATE_LOG" + grep -qF 'SDD_PR_AUTHOR=[mlorentedev]' "$GATE_LOG" + grep -qF 'Archive skip rationale' "$GATE_LOG" +} + +@test "spec-gate-prepush: resolves the CURRENT branch's PR, no --pr number involved" { + run "$ADAPTER" --base-ref origin/main --head-ref HEAD + [ "$status" -eq 0 ] + grep -qF -- '--json labels,body,author' "$GH_LOG" + run grep -c -- '-p ' "$GH_LOG" + [ "$output" -eq 0 ] +} + +@test "spec-gate-prepush: forwards every argument to the gate verbatim" { + run "$ADAPTER" --base-ref origin/main --head-ref HEAD --explain \ + --adjacency-issues /tmp/adjacency.tsv + [ "$status" -eq 0 ] + grep -qF 'ARGS=[--base-ref origin/main --head-ref HEAD --explain --adjacency-issues /tmp/adjacency.tsv]' "$GATE_LOG" +} + +@test "spec-gate-prepush: no PR for this branch falls through, gate still runs with no PR context" { + export STUB_GH_RC=1 + run "$ADAPTER" --base-ref origin/main --head-ref HEAD + [ "$status" -eq 0 ] + grep -qF 'SDD_LABELS=[]' "$GATE_LOG" + grep -qF 'SDD_PR_BODY=[]' "$GATE_LOG" + grep -qF 'SDD_PR_AUTHOR=[]' "$GATE_LOG" +} + +@test "spec-gate-prepush: missing gh falls through without invoking it" { + # A restricted PATH, not just deleting the stub: the real system `gh` + # (further down the inherited PATH) must not be reachable either, or this + # test would pass by accident against a tool that answers for real. + rm -f "$FIX/bin/gh" + PATH="$FIX/bin" run "$ADAPTER" --base-ref origin/main --head-ref HEAD + [ "$status" -eq 0 ] + [ ! -f "$GH_LOG" ] + grep -qF 'SDD_LABELS=[]' "$GATE_LOG" +} + +@test "spec-gate-prepush: missing jq falls through without ever calling gh" { + # $FIX/bin is self-contained -- no jq in it, and the adapter needs nothing + # else from PATH before it decides to fall through (the gate stub is a + # script invoked by absolute path via `exec`, not looked up on PATH). + cat > "$FIX/bin/gh" <<'STUB' +#!/usr/bin/env bash +printf 'GH_ARGS=[%s]\n' "$*" >> "$GH_LOG" +exit 1 +STUB + chmod +x "$FIX/bin/gh" + PATH="$FIX/bin" run "$ADAPTER" --base-ref origin/main --head-ref HEAD + [ "$status" -eq 0 ] + [ ! -f "$GH_LOG" ] + grep -qF 'SDD_LABELS=[]' "$GATE_LOG" +} + +@test "spec-gate-prepush: a null body and no labels become empty, not the string null" { + export STUB_PR_JSON='{"labels":[],"body":null,"author":{"login":"dependabot[bot]"}}' + run "$ADAPTER" --base-ref origin/main --head-ref HEAD + [ "$status" -eq 0 ] + grep -qF 'SDD_LABELS=[]' "$GATE_LOG" + grep -qF 'SDD_PR_BODY=[]' "$GATE_LOG" + grep -qF 'SDD_PR_AUTHOR=[dependabot[bot]]' "$GATE_LOG" +} + +@test "spec-gate-prepush: propagates the gate verdict instead of masking it" { + export STUB_GATE_RC=1 + run "$ADAPTER" --base-ref origin/main --head-ref HEAD + [ "$status" -eq 1 ] +} + +@test "spec-gate-prepush: --help exits 0 and documents the fall-through" { + run "$ADAPTER" --help + [ "$status" -eq 0 ] + [[ "$output" == *"Falls through"* ]] +} + +@test "the pre-commit config wires sdd-spec-gate through this adapter, not the gate directly" { + # Guards the wiring itself: check-spec-gate.sh alone cannot resolve a live + # PR, so if this entry ever points back at it directly, BUG-061 is back -- + # silently, since nothing else would fail (bitacora-reconcile's lesson: + # config logic unreachable by tests goes red-and-silent). + run grep -A8 'id: sdd-spec-gate' "$BATS_TEST_DIRNAME/../.pre-commit-config.yaml" + [ "$status" -eq 0 ] + [[ "$output" == *"entry: ./scripts/spec-gate-prepush.sh"* ]] + [[ "$output" != *"entry: ./scripts/check-spec-gate.sh"* ]] +} diff --git a/tests/stub-real-pairing.bats b/tests/stub-real-pairing.bats index b6375c16..72b16a80 100644 --- a/tests/stub-real-pairing.bats +++ b/tests/stub-real-pairing.bats @@ -23,6 +23,13 @@ setup() { # reason. Add a row only when a real test genuinely cannot exist; prefer writing # the sibling. # +# Single-sourced: both exempt() and the "no stale entries" test below read this +# one list, rather than each keeping its own copy. They used to be two +# independently-maintained copies, and had already drifted — `bitacora-rollout` +# was exempted here but missing from the stale-entries loop, so an exemption +# going stale for that one suite specifically would have gone undetected. That +# is the exact failure mode this file exists to catch, reproduced inside itself. +# # bitacora-reconcile stubs `gh` — a real run mutates the live GitHub project board # bitacora-rollout stubs `gh` — same; a real run adds items to the live board. This # suite is a worked example of the limitation BUG-055 names: #884 was @@ -36,22 +43,70 @@ setup() { # shell-profile stubs `zsh`/`bash` timing probes — a real run measures this machine, not a fixture # skills-pipeline stubs the deploy targets — a real run writes into the caller's own $HOME # vault-health stubs `hive` — a real run needs the daemon and a live vault +# vault-health-golden stubs `obsidian` (from tests/golden/vault-health/lib.sh, not the +# .bats file itself — see #892) — same rationale as vault-health: a +# real run needs the AppImage and a live vault # vault-maintenance-weekly stubs `cron`/`hive` — a real run installs a crontab entry +EXEMPT_SUITES="bitacora-reconcile bitacora-rollout board-pickup guard-memory-sink hermes-setup +install-dotf shell-profile skills-pipeline vault-health vault-health-golden vault-maintenance-weekly" + exempt() { - case "$1" in - bitacora-reconcile|bitacora-rollout|board-pickup|guard-memory-sink|hermes-setup|install-dotf|\ - shell-profile|skills-pipeline|vault-health|vault-maintenance-weekly) return 0 ;; - *) return 1 ;; - esac + local base + for base in $EXEMPT_SUITES; do + [ "$base" = "$1" ] && return 0 + done + return 1 } # A suite "stubs a binary" when it makes something executable and puts its # directory on PATH — the shape that shadows a real tool for the suite's own # process. Deliberately structural: it matches intent, not a naming convention. -stubs_a_binary() { +_stubs_a_binary_in_file() { grep -q 'chmod +x' "$1" && grep -qE 'PATH="?\$' "$1" } +# Resolves `. "$VAR/name"` / `source "$VAR/name"` lines to a path under tests/, +# one level of same-file variable substitution deep — the shape every golden +# corpus suite uses: `HERE="$BATS_TEST_DIRNAME/golden/x"; . "$HERE/lib.sh"`. +# Deliberately one level: this resolves what THE SUITE sources, not what a +# sourced library goes on to source itself. +_sourced_test_libs() { + local f="$1" line name val target rest + local -A vars=() + + while read -r name val; do + vars["$name"]="$val" + done < <( + # shellcheck disable=SC2016 # the $BATS_TEST_DIRNAME is a literal + # pattern matched against the source file's text, not an expansion here. + grep -oE '^[[:space:]]*[A-Za-z_][A-Za-z0-9_]*="\$BATS_TEST_DIRNAME/[^"]*"' "$f" | + sed -E 's#^[[:space:]]*([A-Za-z_][A-Za-z0-9_]*)="\$BATS_TEST_DIRNAME/(.*)"$#\1 \2#' + ) + + while IFS= read -r line; do + target=$(printf '%s' "$line" | grep -oE '"\$[A-Za-z_][A-Za-z0-9_]*/[^"]*"' | tr -d '"') + [ -n "$target" ] || continue + name="${target#\$}"; name="${name%%/*}" + rest="${target#*/}" + if [ "$name" = "BATS_TEST_DIRNAME" ]; then + printf '%s/%s\n' "$TESTS" "$rest" + elif [ -n "${vars[$name]:-}" ]; then + printf '%s/%s/%s\n' "$TESTS" "${vars[$name]}" "$rest" + fi + done < <(grep -E '(^|[^A-Za-z_])(\.|source)[[:space:]]+"\$[A-Za-z_]' "$f") +} + +stubs_a_binary() { + _stubs_a_binary_in_file "$1" && return 0 + local lib + while IFS= read -r lib; do + [ -f "$lib" ] || continue + case "$lib" in "$TESTS"/*) ;; *) continue ;; esac + _stubs_a_binary_in_file "$lib" && return 0 + done < <(_sourced_test_libs "$1") + return 1 +} + @test "every suite that stubs a binary either pairs with a real test or is a declared exemption" { unpaired=() for f in "$TESTS"/*.bats; do @@ -77,8 +132,7 @@ stubs_a_binary() { # cover to a suite that has since grown a real sibling, or to one that no # longer exists at all. stale=() - for base in bitacora-reconcile board-pickup guard-memory-sink hermes-setup install-dotf \ - shell-profile skills-pipeline vault-health vault-maintenance-weekly; do + for base in $EXEMPT_SUITES; do [ -f "$TESTS/$base.bats" ] || { stale+=("$base (suite gone)"); continue; } [ -f "$TESTS/$base-real.bats" ] && stale+=("$base (now has a real sibling)") done @@ -98,3 +152,17 @@ stubs_a_binary() { [ "$status" -eq 0 ] [ -f "$TESTS/precommit-fallback-real.bats" ] } + +@test "the source-following is enforceable: vault-health-golden stubs obsidian only via its sourced lib.sh" { + # Guards the #892 fix itself. The .bats file greps clean on its own -- the + # stubbing lives entirely in tests/golden/vault-health/lib.sh -- so this + # pins that stubs_a_binary only catches it by following the `. "$HERE/..."` + # line. Without this pin, the source-following could silently regress (e.g. + # a refactor changes the variable name) and both tests above would pass + # vacuously again, exactly the failure #892 reported. + run _stubs_a_binary_in_file "$TESTS/vault-health-golden.bats" + [ "$status" -ne 0 ] + + run stubs_a_binary "$TESTS/vault-health-golden.bats" + [ "$status" -eq 0 ] +}