diff --git a/docs/tdd/0068-format-and-lint-honors-config.md b/docs/tdd/0068-format-and-lint-honors-config.md new file mode 100644 index 0000000..ff644ba --- /dev/null +++ b/docs/tdd/0068-format-and-lint-honors-config.md @@ -0,0 +1,343 @@ +# TDD 0068: format-and-lint honors the repo's configuration (FR-21, issue #180) + +Status: draft +PRD refs: FR-21, FR-1, FR-2, NFR-4 +PRD-rev: f6ef178 +ADR constraints: 0004, 0005, 0006, 0010 +Supersedes: 0001 (the `format-and-lint` hook component only; 0001's bootstrap design stands) + +## Approach +FR-21 says the hook formats then lints edited files "when a linter is +configured (no-op otherwise)" and that failures are "surfaced into the +session for root-cause fixing". TDD 0001 says "no configured linter → +no-op". The shipped hook does neither (issue #180). Reproduced on 3.49.0 +with ruff 0.16.10: +- a lint-only ruff config caused a whole-file reformat; +- with no config, `--fix` deleted imports mid-edit; +- a blocking exit 2 put the diagnostics on stdout, so the agent saw only + a one-line summary. + +JS/TS, Rust and Go have the same class of problem. + +Rules, for every language: +1. **Discovery starts from the edited file.** It walks up from the file's + directory to the file's git toplevel, or to `/` when the file isn't in + a repo. It never starts from `$PWD`. +2. **Lint only when a linter is configured, and report only.** No + `--fix`. Diagnostics go to **stderr** on failure, with exit 2. +3. **Format only on opt-in.** + - Python and JS/TS need an explicit formatter config. + - Rust and Go use the language convention, but only where it can't + re-create #180: the file is in a git repo, and its committed version + is already formatter-clean (or the file is new to git). + - Rust is formatted through stdin, so `rustfmt` never touches child + modules. +4. **A missing tool or config means exit 0 and nothing written.** The + exception is a missing jq/python3, which is still a loud exit 2, + because the hook then can't read its input. + +`/bootstrap-project` writes the formatter opt-in on **every** path that +creates a formatter config. The README tells already-bootstrapped Python +repos to add `[tool.ruff.format]`. + +## Components & interfaces +`hooks/format-and-lint.sh`. Stdin parsing via jq → python3 is unchanged. +Internal helpers: + +- **`_tl_stop_dir `** prints `git -C rev-parse --show-toplevel`, + or `/`. +- **`_tl_find_up `** walks from `` up to the stop dir, + inclusive. It prints the first directory where ` ` returns + 0, with rc 0. If none match, it prints nothing and returns rc 1. +- **`_tl_json_has_key `** tests for a **top-level** + key: jq `has($k)`, falling back to python3 `k in json.load(f)`. Missing + file, parse error, or no parser → rc 1. A key that appears only inside + `devDependencies` does not count. +- **`_tl_local_bin `** prints the first + `node_modules/.bin/` found walking up from ``, or returns + rc 1. This replaces the `npx --no-install … --version` probes: no extra + process per edit. +- **`_tl_git_state `** prints one of three states: + - `outside` — not in a git work tree; + - `new` — in a work tree, but `git cat-file -e HEAD:` fails. This + covers untracked, intent-to-add, and unborn-HEAD files. + - `tracked` — `HEAD:` exists. + + `` = `git -C rev-parse --show-prefix` + basename, so a + symlinked directory path cannot mislead it. +- **`_tl_head_clean `** returns rc 0 iff + `_tl_git_state` is `tracked`, and the command — run as + `git show HEAD: | (cd && )` — exits 0 with + **empty stdout**. Any error counts as not-clean. + +**Python (`.py`)** +- No ruff on `PATH` → exit 0. +- **Config is per directory, in ruff's precedence:** `ruff.toml`, else + `.ruff.toml`, else a `pyproject.toml` containing a line matching + `^\[tool\.ruff[.\]]`, which excludes look-alikes such as + `[tool.ruffle]`. The first directory upward that has one wins, and the + chosen file is the only one inspected. A format-only config counts as + lint-configured too (ruff's default rules), as ruff itself would. No config → exit 0. +- **Format opt-in:** the chosen `pyproject.toml` has `^\[tool\.ruff\.format\]`, + or the chosen `ruff.toml` / `.ruff.toml` has `^\[format\]`. + - Then run `ruff format `, output discarded. + - Dotted top-level forms (`format.quote-style = …`) and `extend =` + inheritance are not recognised. That fails safe: no formatting. +- **Lint:** `ruff check --no-fix --output-format concise 1>&2`. + - rc ≠ 0 → exit 2. stderr ends with + `format-and-lint: ruff reported errors in (diagnostics above). Fix the root cause; do not suppress.` + +**JS/TS (`.js .jsx .ts .tsx .mjs .cjs`)** +- **Prettier config:** the first directory upward with `.prettierrc` or + `.prettierrc.*`, `prettier.config.*`, or a `package.json` where + `_tl_json_has_key … prettier` is true. + - If found **and** `_tl_local_bin prettier` resolves, run + `(cd && --write )`, output discarded. +- **ESLint config:** the first directory upward with `eslint.config.*` or + `.eslintrc*`, or a `package.json` where `_tl_json_has_key … eslintConfig` + is true. + - If found **and** `_tl_local_bin eslint` resolves, run + `(cd && ) 1>&2`, without `--fix`. + - rc ≠ 0 → exit 2 with the trailing line, naming eslint. +- A config without the tool installed → no-op. Today that case blocks + every edit. + +**Rust (`.rs`)** — the root is the first directory upward with +`Cargo.toml`. No root → exit 0. +- **Format** requires `rustfmt` on `PATH` and `_tl_git_state` ≠ + `outside`. It runs when the state is `new`, or when + `_tl_head_clean rustfmt --edition --check` passes. + 1. Run `(cd && rustfmt --edition < > )`. + 2. If rustfmt exits 0 and `` differs from the file, write it back + with `cat > `. This keeps the file's mode and inode. + Skip formatting entirely when the file is a symlink (`-L`). + + Stdin mode never recurses into `mod` children, and running in the + file's directory makes `rustfmt.toml` discovery match the file. + - `` is the root `Cargo.toml`'s `edition = "…"`, default `2021`. + - `edition.workspace = true` falls back to `2021`. If the style + differs, `_tl_head_clean` fails and nothing is formatted: it fails + safe. +- **Lint:** if `cargo` is on `PATH` and the debounce allows, run + `(cd && cargo clippy --quiet) 1>&2`. rc ≠ 0 → exit 2. + +**Go (`.go`)** — the root is the first directory upward with `go.mod`. No +root → exit 0. +- **Format** requires `gofmt` on `PATH` and a state ≠ `outside`. It runs + when the state is `new`, or when `_tl_head_clean gofmt -l` + passes. Then run `gofmt -w `, which formats one file only. + `gofmt -l` reads stdin, exits 0 either way, and prints + `` when unformatted. That is why "clean" means empty + stdout. +- **Lint** requires `golangci-lint` on `PATH`, a `.golangci.{yml,yaml,toml,json}` + in the **root or above** (where golangci-lint looks when run from the + root), and the debounce. Run + `(cd && golangci-lint run .//...) 1>&2`, where + `` is `.` for the root, which gives `./...`. rc ≠ 0 → exit 2. + +**Debounce:** the window and env var are unchanged +(`THROUGHLINE_LINT_DEBOUNCE`, 30s). It is now keyed by the project root, +not `$PWD`. + +**`skills/bootstrap-project/SKILL.md`** — the rule applies to **every** +path that writes a formatter config: greenfield, and brownfield "add the +default". +- **Python** `pyproject.toml` gets `[tool.ruff]`, `[tool.ruff.lint]` and + `[tool.ruff.format]`. +- **JS/TS** gets a `.prettierrc` and an `eslint.config.*`. +- **Rust/Go:** no config is needed. +- The prose describes the hook as: explicit opt-in for Python/JS, + HEAD-clean convention for Rust/Go, never auto-fix. + +**`README.md`** — the hook paragraph, plus an upgrade note: Python repos +bootstrapped before 3.50.0 have lint-only ruff config, so add +`[tool.ruff.format]` to keep formatting. + +## Data & state +No new files. The debounce marker is keyed by the project root. Rust +formatting writes a temp file and copies it back with `cat`, which +preserves the file's mode. + +## Sequencing / implementation plan +1. `tests/format-and-lint-hook.test.sh` with stub tools, registered in + `tests/implement-gate.test.sh`. +2. Rewrite `hooks/format-and-lint.sh`: the helpers, then the four + branches. +3. `skills/bootstrap-project/SKILL.md` (all formatter-writing paths) and + `README.md` (paragraph and upgrade note). + +## Failure modes & edge cases +**Real risks** +- **Previously bootstrapped Python repos stop formatting.** This is the + issue's own principle (lint config ≠ format opt-in). The README upgrade + note says how to opt in, and newly bootstrapped repos are covered. +- **Real-tool behaviour could differ from the stubs.** The runtime-verify + gate installs ruff, gofmt and rustfmt into a scratch directory and runs + real controls (observation 13). If the install fails, the gate reports + BLOCKED, never PASS. +- **A Cargo/Go project outside git is never formatted.** There is no HEAD + to prove the file was clean. This is the safe side of #180. + +**Overblown risks** +- **Cost of the extra `git show`.** It adds milliseconds per Rust/Go + edit, and only when the formatter is installed. +- **Nested projects.** The nearest config wins, which is ruff's own rule. + +**Unspoken risks** +- **Lint output now reaches the agent in full.** A large clippy run can + flood the session. That is the linter's real output; clippy and + golangci-lint stay debounced. +- **ESLint flat configs are detected by presence only.** A config that + ignores the file exits 0, which is correct. + +## Verification plan +- **Surface:** + - the hook's rc, stdout and stderr; + - the edited file's bytes; + - a call log written by the stub tools. +- **Harness:** + - Each case runs in a temp git repo, cleaned up by a trap (L-004). + - The hook gets `{"tool_input":{"file_path":""}}` on stdin and + runs as + `env -i HOME= PATH=:/usr/bin:/bin bash hooks/format-and-lint.sh`, + started from a directory **unrelated** to the file. + - Stubs log their argv and cwd to `/calls`: + - `ruff` and `rustfmt` act as formatters and rewrite the file, or + stdout, to a marker; + - lint stubs print `STUB-DIAG` to stdout and exit 1 when the file + contains `BAD`; + - `gofmt -l` copies real gofmt: rc 0, and prints `` + when its stdin contains `UNCLEAN`; + - `rustfmt --check` prints a diff and exits 1 on `UNCLEAN`. + - JS tools are stubbed as `node_modules/.bin/{prettier,eslint}`. +- **Observation points → expected (PASS):** + 1. **No config, Python** → rc 0; `calls` is empty; the file is + byte-identical; stdout and stderr are empty. + 2. **`[tool.ruff]` plus `[tool.ruff.lint]` only** → only + `check --no-fix --output-format concise `; the file is + unchanged; rc 0. + 3. **`[tool.ruff.format]` present** → `format`, then + `check --no-fix …`; the file is rewritten. + 4. **`ruff.toml` with `[format]` next to a `pyproject.toml` with no + format section** → formats, because `ruff.toml` wins. The reverse + (`pyproject.toml` with format plus a `ruff.toml` without it) → no + format. + 5. **Configured, file contains `BAD`** → rc 2; stderr contains + `STUB-DIAG` and `(diagnostics above)`; stdout is empty. + 6. **No `--fix`** in `calls` across every case, after asserting `calls` + is readable. + 7. **JS:** + - no config → no calls; + - `.prettierrc` with a local bin → `prettier --write` from the config + dir; + - a `package.json` with prettier only in `devDependencies` and no + config → **no** prettier call; + - `"prettier": {}` at the top level → a call; + - an `eslint.config.js` with the bin → `eslint `, no `--fix`; + - a config without a local bin → rc 0, no call. + 8. **Config above `$PWD`.** The config sits next to the file, and the + hook runs from an unrelated cwd → still detected (as in case 3). + 9. **Rust:** + - in `Cargo.toml` + git, a clean tracked `.rs` file → rustfmt + invoked as stdin mode with `--edition 2021`, cwd = the file's dir; + - HEAD `UNCLEAN` → no write; + - a new (untracked) file → formatted; + - outside git → no rustfmt; + - a `lib.rs` edit leaves a child `foo.rs` byte-identical; + - no `Cargo.toml` → no calls. + 10. **Go:** + - a clean tracked file → `gofmt -w`; + - HEAD `UNCLEAN` (stub rc 0 plus stdout) → **no** `gofmt -w`, + proving the guard reads stdout, not rc; + - no `.golangci.*` → no golangci-lint call; + - a `.golangci.yml` at the module root → called with `./...` from + the root; + - a config only in a subdirectory → not called. + 11. **Bootstrap** (text check; the reason is that it is skill prose an + eval can't execute): `skills/bootstrap-project/SKILL.md` names + `[tool.ruff.format]` in both the greenfield and brownfield-add + paths, with the file asserted readable first. + 12. **Regression:** + - a fully configured repo (ruff format and lint, prettier and + eslint) still formats and lints, minus `--fix`; + - with jq and python3 hidden → rc 2 and the existing "need jq or + python3" message. + 13. **Real tools** (runtime-verify gate only). The worker installs + ruff (venv), gofmt (Go tarball) and rustfmt (standalone) into the + scratchpad; if any install fails → BLOCKED. Then: + - issue #180 controls with real ruff: + - `[tool.ruff.format]` with `x=1` → `x = 1`; + - lint-only (case A) → unchanged, rc 0; + - no config (cases B and C) → unchanged, imports kept, rc 0; + - real gofmt: + - a dirty-HEAD tracked file → untouched; + - a clean-HEAD file with a new unformatted line → formatted; + - real rustfmt: + - the same two cases; + - plus a `lib.rs` edit leaving a dirty child module untouched. +- Every negated check first asserts that its input exists and is + non-empty (L-001, L-011). + +## Evaluation rubric +| Criterion | High-quality | Acceptable | Failing | +|---|---|---|---| +| requirement traceability | FR-21, FR-18, FR-43, FR-88, FR-41 each map to a named function, hook branch, or skill block | One mapping indirect but named | An in-scope requirement missing | +| interface concreteness | Every new helper (config discovery, tl_session_pid, lock fields, reason block) has args, stdout, rc pinned | One error return implicit | A reader cannot tell what a helper prints or returns | +| executable verification | Hook run via stdin in temp repos with stub tools and real ruff when present; skill blocks extracted and run; negated checks fail closed (L-001/L-011) | One check text-only with stated reason | A behaviour asserted only by grepping text | +| alternatives-analysis substance | Each mechanism names >=1 rejected alternative with reason (PWD discovery, any-ruff-config opt-in, --fix, $PPID lock, env-var reason) | One rejection thin | None | +| verification-plan actionability | Surface, observation points, PASS values named, incl. issue #180 three control cases | One fixture underspecified | Missing or non-actionable | +| scope-bound adherence | Each TDD <=8 files, <=500 body lines; estimates padded; exceptions declared | One justified exception | Over a bound with no exception | +| regression safety | Repos that DID configure tools format/lint as before; a live lock still refuses; existing evals stay green | One regression case only implied | A previously-working configured path is untested or broken | + +## Requirement traceability +| Requirement | Design element | +|---|---| +| FR-21 lint only when configured | per-language config discovery; obs 1, 2, 7, 10 | +| FR-21 no-op otherwise | missing tool/config → exit 0, no calls; obs 1, 7, 9 | +| FR-21 failures surfaced for root-cause fixing | stderr diagnostics, exit 2, no `--fix`; obs 5, 6 | +| FR-21 formats then lints | explicit opt-in (py/js); HEAD-clean convention via stdin (rs/go); obs 3, 4, 9, 10, 13 | +| FR-21 debounced | keyed by project root | +| FR-1 / FR-2 bootstrap configures tooling | every formatter-writing path includes the opt-in; obs 11; README upgrade note | +| NFR-4 honest failure | the agent sees the real diagnostics; a missing parser is still loud; real-tool controls or BLOCKED | + +## Dependencies considered +No new dependencies. + +Rejected: +- **Config discovery from `$PWD`.** That is today's bug. +- **Any ruff config as a format opt-in.** Issue case A. +- **Keeping `--fix`.** It deletes code mid-edit (case B), and FR-21 says + failures are surfaced for root-cause fixing. +- **A substring grep of `package.json`.** It matches `devDependencies`, + which would re-create #180. +- **`npx --no-install --version` probes.** They add one process + per edit, and `npx` can't tell "missing" from "lint failed". A local + `node_modules/.bin` lookup is free. +- **`rustfmt `.** It recurses into child modules, so it formats + lines the agent never touched. Stdin mode doesn't. +- **Formatting Rust/Go outside git.** There is no evidence the file was + clean, so it is skipped. + +## PRD conflicts surfaced (and resolution) +None. The design brings the hook back to FR-21's existing wording; +issue #180 is the trigger. Repos bootstrapped before this change lose +automatic Python formatting. They get an upgrade note rather than a +silent opt-in, because a lint-only config is not a format opt-in. + +## Decisions to promote (ADR candidates) +None. This corrects the implementation to match an existing requirement. + +## Touched files +- `hooks/format-and-lint.sh` — config discovery from the edited file, opt-in formatting, report-only lint to stderr +- `tests/format-and-lint-hook.test.sh` — stub-tool eval of every branch +- `tests/implement-gate.test.sh` — register the new eval +- `skills/bootstrap-project/SKILL.md` — every formatter-writing path includes the opt-in; hook prose +- `README.md` — hook paragraph and upgrade note + +## Expected diff size +- `hooks/format-and-lint.sh` — 240 lines +- `tests/format-and-lint-hook.test.sh` — 420 lines (exception: one cohesive stub-tool eval over four languages and 12 observation points) +- `tests/implement-gate.test.sh` — 12 lines +- `skills/bootstrap-project/SKILL.md` — 30 lines +- `README.md` — 20 lines +Total expected diff: 722 lines across 5 files. diff --git a/docs/tdd/0069-build-tdds-hardening.md b/docs/tdd/0069-build-tdds-hardening.md new file mode 100644 index 0000000..a07d00f --- /dev/null +++ b/docs/tdd/0069-build-tdds-hardening.md @@ -0,0 +1,342 @@ +# TDD 0069: `/build-tdds` hardening — a lock that holds, and the 0067 follow-ups + +Status: draft +PRD refs: FR-18, FR-43, FR-88, FR-41, NFR-4 +PRD-rev: f6ef178 +ADR constraints: 0005, 0006, 0011, 0013, 0015, 0016 + +## Approach +Two groups of fixes to the shipped `/build-tdds` (3.49.0). + +**1. The single-run lock never held (FR-18 / FR-43).** `tl_run_lock` +writes `$$`. The harness runs every Bash call in a fresh shell that exits +at once, so the recorded PID is dead by the next call, and any second run +"reclaims" it. This was observed in run 20261005-174458. The check-then- +write is also not atomic. The fix: +- record the **session process**, meaning the first long-lived ancestor + of the Bash shell, together with its **start time**; +- treat a lock as live only if that PID exists *with the same start + time*, which defeats PID reuse; +- acquire it atomically with noclobber (`mkdir` is not atomic on this + machine's uutils); +- let a second `/build-tdds` from the same session re-enter its own lock. + +**2. The 0067 review follow-ups.** 0067 is implemented, so its text is +not edited; this TDD records the changes. +- **(a)** Stream error text no longer reaches a shell command line. The + parent writes it to a file with the file-write tool, and a block reads + that file. +- **(b)** The implementer's rate-limit rule points to the + `credits_required` exception. +- **(c)** The live probe removes the harness session folders its headless + children create. +- **(d)** `tl_run_retry_begin` does what 0067 specified: it copies the + failed report itself and prints `report=` first. +- **(e)** `TL_BASE_SHA` is validated. + +## Components & interfaces +All of these live in `scripts/lib/run-record.sh` unless stated otherwise. + +**`tl_session_pid`** prints ` ` for the first ancestor of the +current shell whose command name (`ps -o comm=`) is not one of `bash`, +`sh`, `zsh`, `dash`, `env`, `timeout`, `nohup`, `script`, `sudo`, +`bwrap`, `firejail`, `flatpak-spawn`. +- `` = `ps -o lstart= -p `, with whitespace squeezed to + single spaces. +- The walk follows `ps -o ppid=`. +- PID 1 is never chosen: reaching it returns rc 1 with no output, as does +a `ps` failure. + +**Lock file** `docs/tdd/.implement-logs/.run.lock`. Its single line is +`pid= start=`. Liveness, `_tl_lock_live `: +- **Current format:** `kill -0 ` succeeds **and** the current + `lstart` of that PID equals ``. +- **Legacy bare-PID line:** `kill -0` only. + +**`tl_run_lock [owner]`**. `owner` defaults to `tl_session_pid`; +if that fails, rc 1 with +`run-record: cannot identify the session process; not locking`. +Every create below uses bash noclobber (`set -C`, `O_EXCL`), which this +machine validated as atomic. + +1. **No lock file.** Run `(set -C; printf '%s\n' "" > lock)`. + - If that fails, another run won the race: rc 1, + `run-record: lock held by a concurrent run`. +2. **Read the line.** If the file vanished (the owner unlocked), go + back to step 1. An empty or unparseable line means a writer is + mid-write. Retry 3× at 0.2 s. If it is still unreadable: rc 1, + `run-record: lock unreadable; held` (never treated as dead). +3. **Same owner** (same pid and same start) → rc 0. Re-entrant within a + session. +4. **Live other owner** → rc 1, + `run-record: lock held by live PID (started )`. +5. **Dead or reused owner → serialized reclaim.** + 1. Take a guard with `(set -C; : > lock.reclaim)`. + - If the guard exists and is older than 60 s, break it with an + atomic `mv lock.reclaim lock.reclaim.stale.$$` and retry once. Only + one breaker's `mv` succeeds. + - Otherwise rc 1, `run-record: lock reclaim in progress`. + 2. Re-read the lock. If it **equals** the stale line observed in + step 2, run `rm -f lock` and then step 1's create. + 3. Otherwise, someone else's fresh lock is there: remove the guard + and go back to step 2. + 4. Always remove the guard before returning. + + A slower reclaimer can never delete a fresh lock, because it compares + under the guard. Exactly one racer gets rc 0. + +**`tl_run_lock_reclaim `** stays as a thin alias of `tl_run_lock` +for callers. **`tl_run_unlock `** removes the file only when the +owner matches `tl_session_pid` or is dead (legacy bare-PID lines: only +when dead). Removing a dead owner's lock takes the same reclaim guard +and re-compares the line before `rm`. Otherwise it returns rc 1 and prints +`run-record: not the lock owner`. + +**`skills/implement/SKILL.md` step 2** becomes block `` +(input `TL_REPO`; it follows 0066's block contract). It runs +`tl_run_lock "$TL_REPO"` and prints `lock=acquired`, or the refusal on +stderr with rc 1, after which the skill stops. Every "unlock" in the +skill runs ``, which calls `tl_run_unlock`. + +**(a) Fall-back reason via a file.** Replaces rule 2's inline argument: +- The parent writes the raw error text with its file-write tool, never + a shell, to `/.fallback-reason.txt`. +- It then runs block ``. Inputs: + `TL_REPO`, `TL_RUN`, `TL_SLUG`, `TL_MODEL`, and `TL_REASON_FILE` + (which must equal that path). +- The block validates `TL_MODEL` against `^[A-Za-z0-9._:-]+$` (else + rc 2). +- It reads the file's first non-empty line, deletes bytes `\000–\037` + and `\177`, and caps the line at 300 chars. +- It calls + `tl_run_set_escalation … fell-back "$TL_MODEL" "dispatch error: "`. +- A missing or empty file is recorded as reason + `dispatch error: (no detail)`. +- The block deletes the file after reading, so a stale reason from an + earlier attempt can never be reused. + +**(b)** In step 7, the sentence "If worker exit or stderr matches +`rate.?limit|…` → paused" gains: "unless this TDD is `escalated` and the +text matches `credits_required|requires usage credits`; that is a +fall-back (Escalation check, rule 1 exception), not a pause." + +**(c) Probe cleanup** (`tests/live/escalation-probe.sh`). The EXIT trap +also removes the harness project folder created for the scratch dir: +`${CLAUDE_CONFIG_DIR:-$HOME/.claude}/projects/`. +- `` is `$(cd -P "$SCRATCH" && pwd)` with every `[^A-Za-z0-9]` + replaced by `-`, which matches the harness's encoding. +- It is removed only if all of these hold: + - `$SCRATCH` came from `mktemp -d`; + - `` starts with `-` and is at least 8 characters long; + - the path contains no `..`; + - the target is a directory and **not a symlink**. +- Nothing else under `projects/` is touched. + +**(d) `tl_run_retry_begin `** now: +1. resolves `tl_run_failed_report`; +2. copies it to `${report%.txt}.prev.txt` (the as-built name); +3. archives the verdicts and sets `building`, as before; +4. prints, in order: `report=`, `implementer_report=`, `archive=`. + +The `tl:retry-begin` block shrinks to the source lines plus one call. +**(e)** In `tl_escalation_fellback_check` and the `tl:escalation-verify` +block, a `` that doesn't match `^[0-9a-f]{7,64}$` → rc 2, with +`run-record: bad base sha`. + +## Data & state +- The lock line format changes. A legacy bare-PID lock is read with the + old liveness rule and is replaced on the next acquisition. +- New per-TDD file: `/.fallback-reason.txt`, written only + on a fall-back. + +## Sequencing / implementation plan +1. `tests/build-hardening.test.sh`, plus updated `tests/escalation.test.sh` + / `tests/run-record.test.sh` cases. Register the new eval in + `tests/implement-gate.test.sh`. +2. `run-record.sh`: `tl_session_pid`, the lock and unlock rewrite, + `tl_run_retry_begin` (d), and the base-sha validation (e). +3. `SKILL.md`: the `tl:lock` / `tl:unlock` blocks, (a), (b), and the + shrunken retry-begin block. +4. Probe cleanup (c). + +## Failure modes & edge cases +**Real risks** +- **The ancestor walk lands on the wrong process.** For example, a + terminal multiplexer or a harness wrapper that is itself short-lived. + Mitigation: the stop-list is explicit, and the eval drives it through + real nested shells. If it picks a process that later exits, the lock + merely reclaims early: the FR-43 behaviour, never a stuck lock. +- **The lock is left after a normal stop if the parent skips the unlock + block.** The owner is the live session, so a second run in *another* + session refuses, and the refusal names the PID. Closing the session + frees it. A run in the same session re-enters (step 2). +- **`ps -o lstart` format differs by platform.** Only equality is + compared, against the same machine's `ps`, so the format never needs + parsing. +- **Legacy lock files** from 3.49.0 contain the dead shell's PID, so they + reclaim immediately. That is correct. + +**Overblown risks** +- **A noclobber race on network filesystems.** `.implement-logs` is local. +- **The reason file is written by the parent model.** It is still model + output, but it never reaches a shell, and control bytes are stripped + before it enters the JSON sidecar. + +**Unspoken risks** +- **Grok Build's process tree is unverified.** If `tl_session_pid` finds + no long-lived ancestor there, it returns rc 1 and the skill stops with + the message. That is loud, not silent. A Grok session should observe + this before relying on it. +- **Probe cleanup depends on the harness's project-dir encoding.** If + that changes, the folder isn't found and nothing is deleted. The + failure is safe, and the folders pile up as before. + +## Verification plan +- **Surface:** the stdout, stderr and rc of the functions; the lock + file's contents; the stdout of extracted skill blocks; the filesystem + under a fake `CLAUDE_CONFIG_DIR`. +- **Harness:** a temp git repo with an initialized run. Blocks are run + with `TL_*` env only. +- **Observation points → expected (PASS):** + 1. **The walk skips shells.** Run + `python3 -c 'import subprocess,sys; subprocess.run(["bash","-c","bash -c tl_session_pid"])'`. + It prints the **python3** PID, which the eval knows from + `os.getpid()` written to a temp file, along with its non-empty start + time. This does not depend on the CI ancestor tree. With `PATH` + wrappers `bwrap`/`timeout` in the chain, it skips them too. + 2. **The lock outlives the Bash call.** A sleeper process is the owner + (`sleep 60 &`, passed explicitly). + - `tl_run_lock " "` → rc 0, with the file line + `pid= start=`. + - A second call with another owner → rc 1, naming the PID. + - The same owner again → rc 0 (re-entrant). + 3. **A dead owner is reclaimed.** Kill the sleeper, then the next lock + → rc 0 with the new owner (FR-43). + 4. **PID reuse is detected.** Rewrite the lock with the live sleeper's + PID but a different `start=`. The next lock with another owner + → rc 0. + 5. **Races have one winner.** + - 10 background `tl_run_lock` calls with distinct live owners against + an **absent** lock → exactly one rc 0, and the file names it. + - 10 against a **dead-owner** lock → exactly one rc 0, and the lock + is never deleted out from under that winner. The final line equals + the winner's, and no `lock.reclaim` is left behind. + 5b. **Partial writes are not stolen.** Pre-create an **empty** lock file + → `tl_run_lock` returns rc 1 `lock unreadable; held` after its + retries, and the file is untouched. A guard file older than 60 s is + cleared. + 6. **Unlock checks ownership.** + - `tl_run_unlock` by a non-owner with a live owner → rc 1, file + kept. + - By the owner → removed. + - A dead owner → removed. + 7. **Legacy locks.** A bare-PID lock of a dead PID → reclaimed. A + bare-PID lock of a live PID → refused. + 8. **The extracted `tl:lock` and `tl:unlock` blocks.** They run as two + `bash` children of one python3 parent (as in obs 1). The first + parent stays alive, blocked on a sleep, while a second parent tries. + - Both `tl:lock` runs print `lock=acquired`: the session re-enters. + - A different python3 parent while the first still holds the lock → + rc 1. + - `tl:unlock` from the owner removes the lock; from the non-owner + → rc 1, and the lock is kept. + 9. **(a) The reason block.** + - A reason file containing `boom "x" $(whoami) \ y`, then + ESC-`[31m`, then a second line → the sidecar + `escalation_reason` is exactly + `dispatch error: boom "x" $(whoami) \ y[31m`: control byte + stripped, no command executed, first line only. + - A missing file → `dispatch error: (no detail)`. + - After the block runs, the reason file no longer exists. + - `TL_MODEL='x;rm'` → rc 2. + 10. **(b) Text check.** Stated reason: this is dispatch-time prose + that an eval can't execute. The step-7 rate-limit sentence + contains `credits_required` (the file is asserted readable + first). + 11. **(c) Probe cleanup.** Run `tests/live/escalation-probe.sh` against + a stub `claude` (via `THROUGHLINE_PROBE_CLAUDE`) that creates + `/projects//x.jsonl` and emits a + minimal valid stream. + - After exit, that folder is gone. + - A sibling folder `projects/-keep-me` still exists. + - A scratch path whose encoded folder is a **symlink** is not + followed or removed. + 12. **(d) `tl:retry-begin` on a failed fixture.** The first line is + `report=…review.txt`, the second `implementer_report=…review.prev.txt` + (the copy exists), and the third `archive=…/retry-1`. The block + itself contains no `cp`. `tests/escalation.test.sh` [12] asserts + this three-line shape and order. + 13. **(e) Base sha.** `tl_escalation_fellback_check … 'HEAD;x' implementer` + → rc 2, `bad base sha`. A valid 40-hex sha → the previous + behaviour. + 14. **Regression.** All existing evals stay green, including 0067's + `escalation.test.sh` (with its retry-begin expectations updated as + in 12) and `run-record.test.sh` [E] (reclaim when the PID is dead). + +## Evaluation rubric +| Criterion | High-quality | Acceptable | Failing | +|---|---|---|---| +| requirement traceability | FR-21, FR-18, FR-43, FR-88, FR-41 each map to a named function, hook branch, or skill block | One mapping indirect but named | An in-scope requirement missing | +| interface concreteness | Every new helper (config discovery, tl_session_pid, lock fields, reason block) has args, stdout, rc pinned | One error return implicit | A reader cannot tell what a helper prints or returns | +| executable verification | Hook run via stdin in temp repos with stub tools and real ruff when present; skill blocks extracted and run; negated checks fail closed (L-001/L-011) | One check text-only with stated reason | A behaviour asserted only by grepping text | +| alternatives-analysis substance | Each mechanism names >=1 rejected alternative with reason (PWD discovery, any-ruff-config opt-in, --fix, $PPID lock, env-var reason) | One rejection thin | None | +| verification-plan actionability | Surface, observation points, PASS values named, incl. issue #180 three control cases | One fixture underspecified | Missing or non-actionable | +| scope-bound adherence | Each TDD <=8 files, <=500 body lines; estimates padded; exceptions declared | One justified exception | Over a bound with no exception | +| regression safety | Repos that DID configure tools format/lint as before; a live lock still refuses; existing evals stay green | One regression case only implied | A previously-working configured path is untested or broken | + +## Requirement traceability +| Requirement | Design element | +|---|---| +| FR-18 single-run lock | `tl_session_pid` owner + noclobber acquisition; obs 2, 5, 8 | +| FR-43 stale lock reclaimed; a live owner still blocks | start-time liveness, reclaim; obs 3, 4, 7 | +| FR-88 outcome recorded honestly | (a) reason via file; (e) base-sha validation; obs 9, 13 | +| FR-88 / FR-41 `credits_required` is a fall-back, not a pause | (b) step-7 pointer; obs 10 | +| FR-88 Retry report hand-off | (d) `tl_run_retry_begin`; obs 12 | +| NFR-4 | refusals name the owner; a missing session process stops loudly; the probe leaves no stray state | + +## Dependencies considered +No new dependencies. + +Rejected alternatives: +- **Record `$PPID` of the Bash call.** It was observed to be the harness + here, but any wrapper shell breaks it, and it is open to PID reuse. +- **Use `mkdir` as the mutex.** It is not atomic on this machine's uutils + coreutils 0.8.0; both racers get rc 0. Noclobber was validated as + atomic. +- **Publish with `ln tmp lock`.** It is atomic in the kernel, but + whether uutils `ln` pre-checks the target (the same TOCTOU as its + `mkdir`) is unverified. A noclobber create plus a reclaim guard needs + no new tool assumptions. +- **`flock(1)`.** It needs a held file descriptor across calls, and the + harness can't keep one open between Bash calls. +- **Pass the reason through an env var.** The parent would still + shell-quote untrusted text. +- **Leave probe sessions to accumulate.** They clutter `/resume` + history, and the cleanup is a one-folder `rm` with strict guards. + +## PRD conflicts surfaced (and resolution) +None. FR-18 and FR-43 already require a working lock. The shipped code +didn't meet them, which TDD 0062's eval missed because it used a fixed +PID. + +## Decisions to promote (ADR candidates) +None. These are corrections to existing requirements. + +## Touched files +- `scripts/lib/run-record.sh` — tl_session_pid, lock/unlock rewrite, retry_begin report copy, base-sha validation +- `skills/implement/SKILL.md` — tl:lock / tl:unlock blocks, fall-back reason file block, credits pointer, shrunken retry-begin block +- `tests/live/escalation-probe.sh` — remove the harness project folder for its scratch dir +- `tests/build-hardening.test.sh` — lock, reason, probe-cleanup, base-sha observations +- `tests/escalation.test.sh` — retry-begin expectations moved to the function +- `tests/run-record.test.sh` — lock line format in existing cases +- `tests/implement-gate.test.sh` — register the new eval + +## Expected diff size +- `scripts/lib/run-record.sh` — 190 lines +- `skills/implement/SKILL.md` — 90 lines +- `tests/live/escalation-probe.sh` — 30 lines +- `tests/build-hardening.test.sh` — 420 lines (exception: one cohesive eval over lock races, the reason block and the probe stub, 15 observation points) +- `tests/escalation.test.sh` — 40 lines +- `tests/run-record.test.sh` — 25 lines +- `tests/implement-gate.test.sh` — 12 lines +Total expected diff: 807 lines across 7 files.