fix: self-test findings across adapters, engine, graph, commands and handoff

[datj9]

Summary

Fixes found by running loomgraph against itself (self-test). 31 commits across five parallel slices, merged into one branch.

What changed

Adapters — kill the process group so timeoutSec actually aborts a run; name the missing binary instead of a generic ENOENT; clamp negative/NaN costs; validate the codex sandbox env and fail a verifier whose sandbox is broken; match verifier pass strings on word boundaries.

Engine / budget — gate the node-run ceiling before every dispatch and re-check the budget on every retry attempt; make the three ceilings mutually exclusive; fail the run when a template reference cannot be resolved; interpolate a human node's question.

Graph validation — reject unknown template node references, and reject adapter on command and human nodes, at validate time.

Commands — events rejects an unknown --kind instead of printing nothing; resume rejects --answer for unknown or non-paused node ids; report validates --visibility at runtime and publishes only the report, not its parent directory; report HTML declares charset and language.

Handoff — rewrite the machine hostname out of a bundle (and wire the hostname through so the rewrite is not inert); files.txt is repo-relative only and never drops entries silently; push exits 1 when the bundle directory is missing; --expires is validated as a real date; scanner catches the auth-header, .netrc and auth.json shapes it missed, treats a bare key assignment as a credential, and the assignment rule is linear so it no longer fires on prose.

Docs — README updated to match the landed behavior.

Test plan

• npm test green (2316 insertions include tests for every fix above)
• npm run lint / typecheck green
• Manual: run a handoff bundle and confirm hostname + credential redaction
• Manual: confirm a run aborts at timeoutSec and the child process group dies

🤖 Generated with Claude Code

[datj9]

Review — verified locally

npm run typecheck clean, npm test green (25 files / 445 tests) on fix/self-test-findings at 3a5b6de. Read the full non-test diff (~1760 lines) across adapters, core, commands and handoff.

The shape of this is good: every fix is narrow, every non-obvious choice carries a comment explaining the failure it prevents, and the regex hardening (bounded scheme quantifier, anchored prefix) fixes real ReDoS with concrete measurements. The budget reservation counter (reservedNodeRuns) is the right fix for the concurrent-batch overshoot, and prepareDispatch correctly moves interpolation ahead of the reservation so a TemplateError charges no node run.

What I'd fix before merge is concentrated in one place: the four new/loosened scanner rules are too broad, and the scanner is fail-closed. Each false positive below makes lg-handoff scan exit 2 and push refuse, on ordinary text.

Blocking — verified by running scanText against this branch

"I edited .claude/settings.local.json to allow it"  => residual-local-hostname
"check the .env.local file"                          => residual-local-hostname
"Password changed"                                   => netrc-credentials
"login failed"                                       => netrc-credentials
'"key": "value"'                                     => env-assignment
'the config uses "key": "id" for lookups'            => env-assignment
scanText("handed off by user on the box", {username:"user"}) => residual-username

1. residual-local-hostname fires on every *.local.* filename. src/handoff/scan.ts — the pattern \b[A-Za-z0-9](...)?\.local\b cannot tell a Bonjour hostname from settings.local.json, .env.local, next.config.local.ts. This repo's own workflow mentions settings.local.json, so a handoff of a session about the harness self-blocks. Anchor it: require .local to end the token (\.local(?![A-Za-z0-9._-])) and require the label to look like a hostname rather than a filename stem — or drop the rule and rely on residual-hostname, which now covers the real case with the actual hostname.

2. netrc-credentials's second alternative fires on two-word prose. ^\s*(?:login|password)\s+\S+\s*$ matches any standalone line of exactly login <word> or password <word> — Password changed, login failed, login required. The comment argues prose has "more than one word after the keyword"; two-word lines are the counterexample, and they are common in a transcript or a quoted git log. Tighten the value instead: require something credential-shaped (length ≥ 8, or containing a non-lowercase-letter), the same trick the auth-header rule already uses successfully.

3. Adding bare KEY to env-assignment catches every JSON "key" field. The 8-char minimum only guards the unquoted branch; the quoted branch "[^"\s]+" has no length floor, so "key": "id" fires. Any transcript that pastes JSON with a key field is blocked. Either apply the same minimum length to the quoted branch, or restrict bare KEY to the unquoted/whitespace-delimited shape you actually needed for .netrc/auth.json. Side note: with KEY in the alternation, API[_-]?KEY is now redundant.

4. residual-username self-fires when the OS account is literally user. replaceUsernameToken rewrites the username to the literal user; identityRules then flags every standalone user token. On a container, a CI runner, or any box where whoami is user, every bundle reports a finding it created itself and can never be pushed. Skip the rule when username equals the placeholder — and consider the same for other placeholder-colliding accounts (admin, runner, ubuntu are all ≥3 chars and appear in prose).

Non-blocking

5. The wall-clock ceiling is no longer a ceiling at 0. src/core/budget.ts — the doc says node-runs and wall-clock are both "projected forward at admission time", but only node-runs are. With maxWallClockSec: 0, elapsed (0) > 0 is false, so a batch is admitted. That is precisely the argument the same comment uses to keep maxUsd inclusive; wall-clock cannot be projected either. Either keep >= for wall-clock, or special-case 0.

6. The SIGKILL escalation can be disarmed before it fires. src/adapters/types.ts — on timeout, killGroup(SIGTERM) runs, the escalation timer is armed for 2s, then the finally block deletes the pid from liveChildPids. If the parent exits inside that 2s window, the exit handler no longer knows the pid and the unref'd escalation timer dies with the process — so a grandchild that ignored SIGTERM survives, which is the case the commit set out to kill. Keep the pid tracked until the escalation resolves, or have the exit handler drain a separate "pending kill" set.

7. Library-level signal handlers change host behavior. Same file — installLifecycleHandlersOnce registers SIGINT/SIGTERM handlers that call process.exit(130/143). Any embedding host (or a future graceful-checkpoint-on-interrupt path in execute) loses its chance to run: the exit is immediate and synchronous. Worth confining to the CLI entrypoint, or gating behind an opt-in so the adapter module stays a library.

8. report --publish --out foo.html publishes an artifact with no index.html. src/commands/report.ts — staging by basename(htmlPath) is right for the default (index.html), but with a custom --out the enclave artifact has no entry page. Copy it in as index.html regardless of the local filename.

Nits

• containsPassToken with pass: "" builds an empty regex that matches everything. Worth a guard (or a schema min(1)) so a blank pass: cannot turn a verifier into a no-op.
• writeBundle's excluded is assigned inside the key loop and would be overwritten if a second key ever needed sanitizing. Correct today; fragile if BundleFiles grows. Accumulate instead of assign.
• detached: true + process.kill(-pid) is a no-op on Windows; the fallback to process.kill(pid) handles it, but the process-group guarantee in the doc comment only holds on POSIX. Worth saying so.
• The unchecked items in the test plan are now partly verifiable: typecheck and test are green. The two manual items (a real timeout kill, hostname/credential redaction on a live bundle) are the ones still worth doing by hand.

Findings 1–4 are the merge blockers; the rest can land as follow-ups.

[datj9-reader]

Verdict: approve

Reviewed by re-running the branch and probing the two claims that matter most (secret scanner, concurrency ceiling) rather than reading them.

Verified by execution

• npx vitest run -> 445/445 pass, 25 files
• npx tsc --noEmit -> 0 errors
• Worktree clean at 82e2e9e

Concurrency ceiling actually holds. I built a graph fanning one entry node out to 10 agent nodes so all 10 become ready in the same batch, with maxNodeRuns: 3:

ceiling=3 fanout=10 agent_dispatches=3 peak_concurrent=2 nodeRuns=3

Exactly 3 dispatched, 7 never spawned. The reservation in engine.ts:274 (recordSpend(state, { nodeRuns: reservedNodeRuns + 1 }) before dispatch, decremented on result.attempts at :444) is what makes this work under a wide batch. This was the highest-risk change in the diff and it does what it claims.

Scanner: 28/30 adversarial cases. All 11 real credential shapes caught (Anthropic, OpenAI, GitHub, AWS, JWT, PEM, URL creds, bearer, netrc one-line and continuation, auth.json). All 7 prose false-positive guards stayed quiet, including Authorization: see the docs and password reset instructions. Identity backstop catches residual username/hostname on word boundaries but not inside words (dat fires, dataset does not).

rewritePaths -> scanText round-trip leaves zero findings:

cwd ${REPO_ROOT} and ${HOME}/x and ${HOME}\y ran on ${HOSTNAME}
post-rewrite findings: none

No ReDoS. Worst case 15ms on a 100k-char hyphen run; the other three pathological inputs were 1-2ms.

The 2 scanner misses, both narrow

1. Authorization: Bearer <lowercase-letters-only> does not fire. Needs a digit, uppercase, -, _ or = anywhere in the value to match. A hex token, UUID, base64 or Basic credential all fire, so a realistic token is caught; a token that is purely [a-z]+ is not.
2. Unquoted assignment values below 8 chars (API_TOKEN=abc1234) do not fire. The quoted form API_TOKEN="1234567" does.

Both look like deliberate false-positive tradeoffs and neither is worth blocking on. Worth a comment in SCAN_RULES naming the lowercase-only gap so a future widening is a decision rather than an accident.

Notes

• Graph validation correctly rejects unknown adapters with the node id in the message (node "n0": adapter: Invalid option: expected one of "claude"|"codex"|"opencode"), matching the "validation stays loud" rule.
• Commit authorship on all 33 commits resolves to datj9.