Skip to content

refactor(claim-evidence): consume agent-eval runBoundedProcess; delete the private runner - #185

Merged
drewstone merged 3 commits into
mainfrom
refactor/consume-eval-bounded-process
Sep 1, 2026
Merged

refactor(claim-evidence): consume agent-eval runBoundedProcess; delete the private runner#185
drewstone merged 3 commits into
mainfrom
refactor/consume-eval-bounded-process

Conversation

@drewstone

@drewstone drewstone commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

The duplicate

src/claim-evidence.ts carried a private 32-line runBash, the only process spawner in this package's shipped source. It ran the claim grader's checks through execFile, which puts the check and every descendant it starts in the grader's process group.

A deadline therefore killed the shell alone. A check that puts its real work in the background — solver & wait — left that work running, holding the stdout and stderr pipes open, so the grader observed no result either. Three solver processes outlived their grading parent by five days; a grading loop hung twice on the same cause, for 8 hours and for 1.9 hours.

agent-eval owns the runner that closes exactly this (agent-eval #710, extracted for this reason). The private copy is deleted.

What changed

before after
spawn execFile in this package runBoundedProcess from agent-eval
process group the grader's the check's own, killed as a group
check body argv to execFile argv to the shared runner, still never shell text
killed run reports 0 possible forced non-zero upstream
deadline exit status 127 124, this file's own DEADLINE_EXIT_CODE

Both call sites moved: the bash -n -c parse that refuses a check before running it, and the bash -c execution. 32 lines deleted, 2 callers rewired, 1 private runner gone. No other spawner remains in src/ — swept with grep -rn "child_process\|execFile\|spawn("; the remaining hits are build scripts and tests.

The upstream gap this needed

runBoundedProcess could only be handed a command line for a shell to read, so bash -n -c <check> was not expressible: shell: 'bash' runs bash -c <check>, which executes it. The only alternative was quoting an author's untrusted check into a shell string — a hand-rolled quoter, which is the class of private copy this change exists to remove.

Filed as agent-eval #715, fixed by agent-eval #716 (args?: string[]), released as agent-eval 0.172.1. An adversarial review of that PR found two real correctness defects in it — the call could reject on a NUL byte, and envMode was not identical across the two spawn forms — both fixed before it merged. Peer range here moves to >=0.172.1 <0.173.0.

Also filed, out of scope here: agent-eval #717, localCommandRunner is a second spawnSync spawn site with none of these bounds; absorbing it needs stdin on BoundedProcessInput first.

The executor states its own conditions, instead of spelling them for a regex

The first commit restored the three runner-side conditions into the error text UNRUNNABLE_SIGNATURES was calibrated on, so the shared runner's fields kept classifying the way the old execFile Error text did. That works and it is the wrong shape — grading would depend on the words a reconstructed error happens to use, and a drift on either side turns an environment failure into contradicted silently. The reviewer audit named it; the second commit takes the durable fix instead of leaving it as a follow-up.

CheckExecution now carries killedBySignal and outputTruncated beside timedOut, and gradeFor reads all three before it reads the exit status. UNRUNNABLE_SIGNATURES stays what it was calibrated for — reading a failure out of what the CHECK printed. Nothing is manufactured: stderr carries the runner's own runnerError text and nothing else, and the exit status is the one the run produced.

This is why the release is a major, 13.0.0. Reading a CheckExecution needs no change; both fields are optional. Producing one does: an executor that kills a check on a caller's signal, or that stops keeping its output, must now say so, or gradeFor grades a fragment as a whole reading. check:version-bump calls any exported-shape change breaking, and it is paid for.

Behavior is pinned, not adapted

Every existing caller test passes with its fixtures untouched — 58 across claim-evidence.test.ts and claim-evidence-intake.test.ts. Grading is unchanged for a check that runs and finishes.

Three runner-side conditions are restored into the vocabulary UNRUNNABLE_SIGNATURES is calibrated on, because none of them is a verdict on the claim and the old execFile path communicated each one as Error text appended to stderr — the signature list was swept over 272 grade files produced that way:

  • a run the caller aborted — now killedBySignal;
  • a run whose capture overflowed maxBufferBytes — now outputTruncated. The old wrapper killed the process; the shared runner truncates, so grading the kept bytes could report contradicted for an expectation printed in the bytes nobody kept;
  • a run that could not be spawned — the runner's own runnerError text, which UNRUNNABLE_SIGNATURES already matches on ENOENT.

The first two are graded from the fields, before the exit status is read, so the expectation comparison is never reached on a run the grader could not observe. That is strictly safer than the old path, which could reach the comparison.

Three new tests, each proven non-vacuous

Every one was run against the deleted runner first:

test against the deleted runner
a backgrounded descendant is gone after the deadline failssleep 60 survives, expect(alive).toBe(false) reads true
an overflowed capture is not graded fails
an aborted check is not graded fails

Verification — the repo's exact CI chain, locally, on the rebased tree

gate result
pnpm install --frozen-lockfile clean
pnpm lint pass, 247 files
pnpm typecheck (src + contracts) pass
pnpm test with AGENT_KNOWLEDGE_RUN_NETWORK_TESTS=1 847 passed, 7 skipped, 0 failed (87 files)
pnpm build pass
pnpm verify:package pass — packed 13.0.0 against installed agent-eval 0.172.1, check:version-bump and check:api-surface included
pnpm verify:official-optimizers pass

main released 12.0.1 and then 12.0.2 mid-change; this branch is rebased onto 12.0.2 and re-verified from scratch on the rebased tree each time. 12.0.2 independently moved the agent-eval peer to >=0.172.1 <0.173.0, so this branch's package.json diff against main is now only the version bump.

Known and unchanged from before this PR: a bash -n parse pass that hits its own deadline still raises UncheckableClaimError saying the check does not parse. The old wrapper did the same (its timeout also produced a non-zero status), so this is not a regression, but it is a wrong verdict shape and worth a separate fix.

The dynamic import('@tangle-network/agent-eval') is deliberate and preserves the property the old code defended: a consumer that never grades a check still imports this module without loading the runner. Confirmed in the build — dist/inspect-*.js keeps the dynamic import and the type-only import is erased.

Not in this PR

discovery-lab/tools/oracle-jail.mjs is the lab-side copy of the same idea. It is out of scope here; the lab deletes it after the next agent-knowledge release ships this.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 87f47b7b

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-09-01T19:50:57Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 271.4s (2 bridge agents)
Total 271.4s

💰 Value — sound-with-nits

Deletes this package's only private process spawner and runs claim checks through agent-eval's group-killing runBoundedProcess, closing a real orphaned-descendant defect in the grain of the repo's own layering doctrine; ship.

  • What it does: verifyGradeableEvidence's two bash invocations (the bash -n -c parse refusal at src/claim-evidence.ts:411 and the bash -c execution at :417) now go through a new runCheckProcess (:515) that calls runBoundedProcess from @tangle-network/agent-eval instead of a private execFile wrapper. Concrete deltas: the check runs in its own process group and a deadline/abort SIGKILLs the whole group; a kille
  • Goals it achieves: Read from the code and its history: (1) close the descendant-kill defect — the old execFile path left a backgrounded solver & wait in the grader's process group, so a deadline killed the shell alone while grandchildren held the pipes open, hanging the grader (documented at :505-510 and reproduced by the new pgrep/kill(pid,0) test); (2) delete a private copy of a primitive the layering already as
  • Assessment: Good, and squarely in the grain. The package layering in AGENTS.md explicitly permits agent-knowledge → agent-eval and assigns shared run primitives to agent-eval; the old runBash was the only child_process spawner in shipped src (verified: rg over src/ leaves only comments in claim-evidence.ts and build scripts under scripts/). The change is behavior-preserving where it should be (argv delivery,
  • Better / existing approach: none — this is the right approach. Searched: (a) rg 'child_process|execFile|spawn(' across src/ and scripts/ — no other runner exists in this package to reuse, only build scripts; (b) verified the upstream primitive in the sibling checkout tangle-network-agent-eval-pr-716/src/bounded-process.ts owns the group-kill semantics this needs; (c) considered teaching gradeFor to consume structured runner
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error event without a message: {"type":"error","timestamp":1788292370346,"sessionID":"ses_fa177f1b3ffebJq0L46mwwbPqX","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_d99a26c9"}}}

🎯 Usefulness — sound

Deletes the package's only private process spawner and rewires both call sites to the shared agent-eval runner that was extracted upstream precisely for this consumer; the pinned version exists on npm with the needed argv support, and the defect it closes (deadline misses background descendants, gra

  • Integration: Fully wired and reachable. verifyGradeableEvidence is exported public API (src/index.ts:9, api-surface.json:772) and its two spawn call sites (src/claim-evidence.ts:411,417) both moved to runCheckProcess, which calls agent-eval's runBoundedProcess (claim-evidence.ts:519-531). The peer pin >=0.172.1 <0.173.0 resolves: I pulled the published 0.172.1 tarball from npm and confirmed dist/bounded-proces
  • Fit with existing patterns: Exactly the codebase's grain, not a competing pattern. The layering diagram in AGENTS.md has agent-knowledge importing agent-eval, and agent-eval's own changelog (0.171.1 entry) names agent-knowledge's claim grader as the motive for extracting runBoundedProcess and the first consumer of the argv form — the shared runner exists because of this caller. A grep for child_process|execFile|spawn( across
  • Real-world viability: Verified live in this sandbox against the installed 0.172.1, not just on paper. (1) Group-kill: ran the exact shape from the incident — bash -c 'echo $$; sleep 60 & echo $!; wait' with a 300ms deadline — and got exit 124, killedByTimeout true, resolution in 303ms, pgrep showing an empty process group, and the descendant pid confirmed dead via kill(pid,0). This is the exact path that hung the gradi
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 runnerDiagnosis manufactures synthetic execFile-era error strings to feed a regex [maintenance] ``

runnerDiagnosis (src/claim-evidence.ts:490-499) reconstructs strings like "AbortError: ..." and "ERR_CHILD_PROCESS_STDIO_MAXBUFFER: ..." so UNRUNNABLE_SIGNATURES (:208) keeps classifying them as unrunnable. This couples this file to upstream field semantics via hand-written pseudo-error text; if either side drifts, an environment failure could silently read as 'contradicted'. The durable fix is for gradeFor to consume structured fields (timedOut/outputTruncated/aborted) rather than regexing rest


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260901T195720Z

…e the private runner

The claim grader spawned its own checks through `execFile`, which puts the
check and every descendant it starts in the GRADER's process group. A deadline
therefore killed the shell alone: a check that backgrounds its real work —
`solver & wait` — left that work running, holding the stdout and stderr pipes
open, so the grader observed no result either. Three solver processes outlived
their grading parent by five days, and a grading loop hung twice on the same
cause, for 8 hours and for 1.9 hours.

agent-eval owns the runner that closes this. `runBoundedProcess` gives the
check its own process group and kills the group, so a deadline reaches the
whole tree, and it forces a non-zero status on a killed run so a SIGKILLed
child that closes with 0 cannot read as a pass.

The 32-line private `runBash` is deleted. Both call sites — the `bash -n`
parse and the execution — go through the shared runner, and the check body now
reaches bash as an argument vector rather than as shell text, so no quoting
stands between an author's check and the interpreter. That needed `args` on
`runBoundedProcess`, added upstream in agent-eval 0.172.1.

Grading is unchanged for a check that runs and finishes; every existing caller
test passes with its fixtures untouched. Three runner-side conditions are
restored into the vocabulary `UNRUNNABLE_SIGNATURES` is calibrated on, because
none of them is a verdict on the claim: a run the caller aborted, a run whose
capture overflowed `maxBufferBytes`, and a run that could not be spawned. A
deadline now reports 124, this file's own `DEADLINE_EXIT_CODE`, where the old
wrapper reported 127.

Three tests are added for what the move buys, each of which fails against the
deleted runner: a backgrounded descendant is gone after the deadline, an
overflowed capture is not graded, and an aborted check is not graded.
…, not from prose

The first commit restored three runner-side conditions into the error text
`UNRUNNABLE_SIGNATURES` was calibrated on, so the shared runner's fields would
keep classifying the way the old `execFile` wrapper's `Error` text did. That
works, and it is the wrong shape: it makes this file's grading depend on the
words a reconstructed error happens to use, and a drift on either side turns an
environment failure into `contradicted` silently.

`CheckExecution` now carries `killedBySignal` and `outputTruncated` beside
`timedOut`, and `gradeFor` reads all three before it reads the exit status.
Neither new field is an observation of the claim: the first says the caller
withdrew the run, the second says the executor stopped keeping the output
before the check stopped printing, so a comparison would answer about a
fragment and could refute a claim the check established.

`UNRUNNABLE_SIGNATURES` stays what it was calibrated for — reading a failure
out of what the CHECK printed. It no longer has to recognise conditions the
executor caused and can simply state. Nothing is manufactured now: `stderr`
carries the runner's own `runnerError` text and nothing else, and the exit
status is the one the run actually produced.

Raised by the PR reviewer audit as the durable fix; it is right, so it is
taken here rather than left as a follow-up.
@tangletools

Copy link
Copy Markdown
Contributor

❌ Needs Work — 87f47b7b

Review health 100/100 · Reviewer score 55/100 · Confidence 85/100 · 18 findings (1 high, 3 medium, 14 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 55 73 60 55
Confidence 85 85 85 85
Correctness 55 73 60 55
Security 55 73 60 55
Testing 55 73 60 55
Architecture 55 73 60 55

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision.

Blocking

🔴 HIGH Abort landing at the execution-phase boundary grades a caller teardown as a claim refutation ('contradicted' instead of 'unrunnable') — src/claim-evidence.ts

agent-eval 0.172.1's aborted-before-spawn path returns BOTH runnerError ('aborted before spawn: the AbortSignal was already aborted') and killedBySignal: true (bounded-process.ts:179-190). runnerDiagnosis checks runnerError first, so the token 'AbortError' never lands in stderr, and 'aborted before spawn' matches no UNRUNNABLE_SIGNATURES entry; gradeFor's nonzero-exit path then returns 'contradicted'. Reproduced against the pinned runner: input {exitCode:137, stderr:'aborted before spawn: the AbortSignal was already aborted'} -> verdict 'contradicted'. Exposure: verifyGradeableEvidence runs a syntax phase then an execution phase with the same options.signal; if the signal aborts after the syntax spawn completes but before the execution spawn (teardown race in a grading loop passing one sig

Other

🟠 MEDIUM Abort test races the cold agent-eval import and fails intermittently — src/claim-evidence.test.ts

The 100ms setTimeout abort (line 698-699) races the first cold dynamic import of '@tangle-network/agent-eval' inside runCheckProcess (claim-evidence.ts:519, measured 175-186ms cold), plus the bash -n syntax spawn. When the signal aborts before that first spawn, runBoundedProcess returns its pre-aborted result (exit 137), the syntax check at claim-evidence.ts:411 is non-zero, and verifyGradeableEvidence throws UncheckableClaimError at claim-evidence.ts:415 instead of returning VerifiedGradeableEvidence. Reproduced: vitest run src/claim-evidence.test.ts -t "caller aborted" failed 3/6 and 5/5 across two batches with exactly that error; a full-file run also

🟠 MEDIUM Pre-aborted AbortSignal grades 'contradicted' instead of 'unrunnable' — src/claim-evidence.ts

runnerDiagnosis checks result.runnerError before result.killedBySignal. For an AbortSignal already aborted at call time, runBoundedProcess (agent-eval@0.172.1) returns killedBySignal:true AND runnerError:'aborted before spawn: the AbortSignal was already aborted'. runnerDiagnosis therefore returns the runnerError string, which lacks the 'AbortError' token that UNRUNNABLE_SIGNATURES matches. gradeFor then sees exitCode 137 with no matching signature and returns 'contradicted', falsely refuting the claim. The old execFile path produced an AbortError whose stringified name 'AbortError' matched the signature -> 'unrunnable'. Empirically reproduced: stderr 'aborted before spawn: the AbortSignal was already aborted' -> UNRUNNABLE_SIGNATURES.test() === false. Fix: reorder so killedBySignal is che

🟠 MEDIUM Pre-spawn abort grades the claim contradicted on a run that never executed — src/claim-evidence.ts

For an already-aborted signal runBoundedProcess returns { exitCode: 137, killedBySignal: true, runnerError: 'aborted before spawn: the AbortSignal was already aborted' }. runnerDiagnosis checks result.runnerError first and returns its text, which contains no UNRUNNABLE_SIGNATURES token ('AbortError' is not in it). Because exitCode is already 137 (nonzero), the exitCode === 0 && diagnosis !== undefined forcing at line 534 does not apply. gradeFor then takes the exitCode!==0 branch -> refutation() -> no signature match -> verdict contradicted. Reproduced: gradeFor({rung:4,check:'echo "done=$((0 + 1))"',expect:'done=1'}, {exitCode:137,stdout:'',stderr:'aborted befo

🟡 LOW Output cap semantics changed from per-stream to combined without documentation — CHANGELOG.md

The old execFile wrapper capped stdout and stderr EACH at maxBufferBytes (options.maxBufferBytes ?? 1MB); runBoundedProcess caps COMBINED stdout+stderr at maxOutputBytes (verified in bounded-process source: outputBytes across both streams, one cap). A check emitting ~700KB to stdout and ~700KB to stderr previously graded normally and now truncates and grades unrunnable. This is a conservative direction (unrunnable, never a false pass) and matches the changelog's 'capture overflowed maxBufferBytes' framing, but the trigger boundary changed silently; a check that used to pass on large-but-under-1MB-per-stream output now refuses. Worth one line in the changelog or a comment, not a blocker.

🟡 LOW Pre-aborted signal is mislabeled as invalid shell syntax, not an abort — CHANGELOG.md

CHANGELOG claims a 'run the caller aborted' is surfaced in the UNRUNNABLE_SIGNATURES vocabulary. Verified against the installed runner: when options.signal is already aborted at call time, runBoundedProcess returns runnerError 'aborted before spawn: the AbortSignal was already aborted' with exitCode 137 and killedBySignal=true (empirically reproduced). runnerDiagnosis (claim-evidence.ts:495) returns that text before the AbortError branch, and the text matches no UNRUNNABLE_SIGNATURES token, so the syntax pre-check throws UncheckableClaimError with INVALID_SHELL_SYNTAX_NOTE — an abort reported as a syntax error. It fails loud (never a wrong verdict), so eval validity is intact, but the error message misleads the claim author and the changelog's blanket statement overstates this edge. Fix in

🟡 LOW 200ms budget is shared with the bash -n syntax pre-check — src/claim-evidence.test.ts

verifyGradeableEvidence runs bash -n -c <check> and the real execution through the same options, so timeoutMs:200 applies to both. If bash -n ever exceeds 200ms on a loaded CI host, runCheckProcess returns a nonzero/timedOut syntax result and verifyGradeableEvidence throws UncheckableClaimError before grading, making the test fail spuriously (timedOut/grade assertions never reached). bash -n on a 30-char script is normally single-digit ms, so this is a low-probability flake, not a product bug. Consider a separate, larger syntax budget or a higher timeoutMs with a shorter-than-sleep work body.

🟡 LOW Abort test does not pin timedOut===undefined, so an abort misread as a deadline passes — src/claim-evidence.test.ts

The test asserts exitCode!==0 and verdict==='unrunnable' but not that execution.timedOut is undefined. If runCheckProcess ever mapped a caller abort to killedByTimeout (the old execFile runner distinguished them via error.name!=='AbortError'), the grade would silently change from the AbortError-signature unrunnable to DEADLINE_NOTE unrunnable and this test would still pass, since both lattice members are 'unrunnable'. I verified the current behavior is correct (timedOut unset on abort), but the regression is unguarded. Fix: add expect(verified.execution.timedOut).toBeUndefined().

🟡 LOW No test for the pre-aborted / runnerError abort path — src/claim-evidence.test.ts

The abort test aborts at 100ms via setTimeout, so the syntax check (bash -n) completes and the execution spawn aborts mid-run — the killedBySignal path, not the runnerError path. No test calls verifyGradeableEvidence with a signal already aborted at call time, which is exactly the case that now misgrades. Add a case with controller.abort() before the call asserting grade.verdict === 'unrunnable'.

🟡 LOW Survivor poll assumes orphaned descendants are reaped within 5s; flakes under non-reaping pid 1 — src/claim-evidence.test.ts

After the group SIGKILL, the reparented sleep is a zombie until init reaps it, and both pgrep -g and kill(pid,0) see zombies. On GitHub Actions ubuntu VMs init reaps promptly (verified: passes in 376ms), but in containerized local dev where node itself is pid 1 and does not reap SIGCHLD, the 5s polling loop exhausts and both survivor assertions fail. Failure is loud, not silent, and the assertion direction (dead) is correct — this is an environment-limited flake vector, not a wrong claim. Optional hardening: treat zombie states as dead when polling (e.g. read /proc//stat), or document the container caveat next to the skipIf.

🟡 LOW failing test leaks a live sleep 60 orphan — src/claim-evidence.test.ts

If the fix regresses, the deadline misses the descendant and the test leaves a 60s sleep running after the assertions fail (the assertion failures do not kill the group). This is inherent to testing exactly this defect, so it is acceptable, but a teardown that kills the leader's group would prevent orphan accumulation during a red CI loop.

🟡 LOW group-empty assertion is vacuous if the runner wraps the shell — src/claim-evidence.test.ts

expect(survivors).toEqual([]) reads the group by pgid $$, which only equals the process-group id if runBoundedProcess spawns bash as the group leader. I confirmed it does under a detached/own-group spawn, but if that contract ever changed (e.g. a wrapper process becomes leader), pgrep -g <$$> would return [] immediately and the group assertion passes without exercising the group-kill. The regression is still guarded by the process.kill(descendant,0) check at line 664, which is the real protection, so this only weakens the group-level assertion rather than hiding the defect.

🟡 LOW pidsInGroup depends on pgrep (procps), unguarded on POSIX hosts — src/claim-evidence.test.ts

it.skipIf(process.platform === 'win32') (line 623) does not guard against a POSIX host that lacks pgrep; execFile('pgrep', ...) then rejects with code 'ENOENT' (≠ 1) at line 613, failing the test with an error unrelated to the SUT. CI on ubuntu-latest ships procps so this is latent, but a contributor on a minimal container (e.g. alpine devcontainer) gets a hard failure. Fix: treat ENOENT as a skip or fall back to reading /proc, or gate the whole describe on a capability probe.

🟡 LOW Output cap is now combined stdout+stderr; overflowed checks run to the deadline instead of being killed — src/claim-evidence.ts

execFile's maxBuffer capped stdout and stderr separately and killed the child on overflow; runBoundedProcess caps stdout+stderr combined (maxOutputBytes) and keeps draining, discarding bytes, without killing. Two behavior deltas: (1) a check writing ~600KB to each stream graded verified under the old per-stream 1 MiB cap and now truncates (combined >1 MiB) and is forced to exit 127 -> unrunnable; the verdict change is safe-direction but contradicts the changelog's 'Grading is unchanged for a check that runs and finishes'. (2) a check that spews output is no longer terminated at the cap — it runs until the 30s deadline, so a noisy check occupies a grading slot far longer than before. Consider documenting the combined-cap change and/or passing a larger explicit cap. Low severity; verdict

🟡 LOW Phase-1 runner failures are reported to authors as 'does not parse under bash' — src/claim-evidence.ts

Any nonzero syntax-phase exit throws UncheckableClaimError prefixed INVALID_SHELL_SYNTAX_NOTE. A missing bashPath (stderr 'Error: spawn ... ENOENT'), a NUL byte in the check (TypeError ERR_INVALID_ARG_VALUE), or a pre-call aborted signal all produce notes telling the author to rewrite a check that bash never saw. Reproduced: runCheckProcess with bashPath '/no/such/bash-binary' returns exit 127 + ENOENT, which phase 1 converts to 'the recorded check does not parse under bash ... Error: spawn /no/such/bash-binary ENOENT'. Fix: when the phase-1 stderr carries a runnerDiagnosis rather than bash's own diagnostic, refuse with a runner-failure note instead of the syntax note.

🟡 LOW Spawn failures only grade unrunnable when the error text hits a signature token — src/claim-evidence.ts

The changelog claims 'a run that could not be spawned' is now reported in the UNRUNNABLE_SIGNATURES vocabulary. That holds for runnerError text containing ENOENT/EACCES, but not for other spawn failures: this shot's own test runs hit Error: spawn bash EAGAIN (process limit), which contains no signature token, so a spawn failure of that kind grades contradicted — a claim verdict on a check that never ran. The old execFile wrapper had identical behavior (it appended the same String(err)), so this is not a regression, but the new explicit runner-diagnosis layer is the natural place to close it: prepend or append a signature-matching token to runnerError text, or add EAGAIN/E2BIG to UNRUNNABLE_SIGNATURES. Low severity given reachability requires resource exhaustion at spawn time.

🟡 LOW Truncation diagnosis reports captured UTF-16 code units as 'bytes' — src/claim-evidence.ts

The message interpolates result.stdout.length + result.stderr.length — JavaScript string lengths — as 'the N bytes captured'. For multibyte output the number understates the byte count, though it is internally consistent with the runner's own length-based cap accounting. Diagnostic-note accuracy only; fix by dropping the number or naming it characters.

🟡 LOW Verdict correctness now depends on agent-eval's runnerError prose containing a signature word — src/claim-evidence.ts

CheckExecution has no first-class field for 'the runner, not the check, failed', so runCheckProcess folds runner failures into exit 127 + stderr text and relies on UNRUNNABLE_SIGNATURES matching that text. Two of the three runnerError strings in 0.172.1 contain a signature word (spawn ENOENT; the args+shell refusal does not but is unreachable here), and finding 1 shows the one that does not silently becomes 'contradicted'. Any future agent-eval release that rewords a runnerError breaks the lattice again with no type-level signal. Fix: propagate runner failures structurally (a refusal flag on CheckExecution, or map any runnerError to an explicit unrunnable note) instead of by string calibration.


tangletools · 2026-09-01T20:06:02Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 1 Blocking Finding — 87f47b7b

Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 5 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-09-01T20:06:02Z · immutable trace

@drewstone
drewstone force-pushed the refactor/consume-eval-bounded-process branch from 87f47b7 to 26bdede Compare September 1, 2026 20:06
tangletools
tangletools previously approved these changes Sep 1, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 26bdede4

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-09-01T20:11:08Z

`verifyGradeableEvidence` raised `UncheckableClaimError` on any non-zero status
from the `bash -n` pass, so a parse killed at its deadline, or by the caller's
own signal, blamed the author for a command bash never finished reading. It is
the same shape every other rule in this file exists to refuse: a budget, or a
caller's teardown, reported as a verdict on the claim.

Measured on the pinned runner before the fix: a signal already aborted at call
time produced "the recorded check does not parse under bash".

A stopped parse is now graded rather than raised, and `gradeFor` reports it
`unrunnable` from the field that says what stopped it. A parse that really ran
and failed still raises, because a command that cannot run anywhere is a
record-time defect the author must fix.

That also removes a race the audit measured in the abort test: the first
dynamic import of the runner takes 175-186ms cold, so a 100ms abort could land
on the parse rather than the execution. Both landings now reach the same
verdict, so the test does not have to beat the import. A second test aborts
before the call with no timer at all, which is the pre-abort path the runner
reports with both `killedBySignal` and a `runnerError`.

Found by the PR reviewer audit, whose blocking finding — an abort graded
`contradicted` — was already closed by grading from the field instead of from
error prose. Reproduced both against HEAD before acting.
@drewstone

Copy link
Copy Markdown
Contributor Author

The blocking finding was raised against 87f47b7b and is already closed on HEAD — by the commit that followed it, not by a patch on top.

Blocker: an abort graded contradicted. The audit is exactly right about 87f47b7b. That commit reconstructed the old execFile error text so UNRUNNABLE_SIGNATURES would keep classifying, and runBoundedProcess's pre-abort result carries BOTH runnerError and killedBySignal — so the runnerError text won, AbortError never reached stderr, and gradeFor refuted the claim. Reproduced against the pinned runner, and the fix verified beside it:

gradeFor(ev, { exitCode: 137, stderr: 'aborted before spawn: …', killedBySignal: true })
  -> { verdict: 'unrunnable', note: 'the check was stopped by the caller before it finished …' }
gradeFor(ev, { exitCode: 137, stderr: 'aborted before spawn: …' })     // no field, the old shape
  -> { verdict: 'contradicted' }

26bdede deleted runnerDiagnosis entirely. CheckExecution carries killedBySignal and outputTruncated beside timedOut, and gradeFor reads all three before the exit status. Nothing is reconstructed, so the ordering bug the finding describes has nowhere left to live. The audit's own weak-concern on the earlier run named this as the durable fix; it is taken rather than deferred.

The medium about the abort test racing the cold import was a real defect, and a deeper one than a flaky test. Chasing it found that verifyGradeableEvidence raised UncheckableClaimError on ANY non-zero status from the bash -n pass. So an abort landing on the parse — which the measured 175-186ms cold import makes likely against a 100ms timer — reported "the recorded check does not parse under bash" about a command bash never finished reading. Reproduced end to end with a pre-aborted signal.

d36576a fixes the cause: a parse the executor stopped is graded, not raised, and gradeFor reports it unrunnable. A parse that really ran and failed still raises. Both abort landings now reach the same verdict, so the test no longer has to beat the import, and a second test aborts before the call with no timer at all.

Verified on HEAD d36576a: pnpm lint, pnpm typecheck, pnpm build, pnpm check:api-surface, pnpm verify:package (13.0.0, check:version-bump included), pnpm verify:official-optimizers — all pass. pnpm test with AGENT_KNOWLEDGE_RUN_NETWORK_TESTS=1: 848 passed, 7 skipped, 0 failed. src/claim-evidence.test.ts run 8 times back to back: 55 of 55 every time, no flake.

The version is 13.0.0, not 12.1.0: check:version-bump calls any exported-shape change breaking, and CheckExecution gained two fields. Reading one needs no change; producing one does, and the changelog says so.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — d36576af

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-09-01T20:21:59Z

@drewstone
drewstone merged commit 23980bd into main Sep 1, 2026
2 checks passed
@drewstone
drewstone deleted the refactor/consume-eval-bounded-process branch September 1, 2026 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants