feat(improvement): widen failure-distiller evidence so tracebacks survive to the proposer#555
Conversation
…cebacks survive The default generationFailureDistiller clipped judge notes at 400 chars and errors at 200 — an executable judge's traceback/failing assertion arrived at the proposer as a stub. 1500 keeps a real traceback intact (error bound 500); the shape of the findings is unchanged. Doc references to the old ~400-char digest updated.
…ce widths + main's claim field)
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — d4fcf3dd
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.
tangletools · auto-approval · reason: drewstone_author · 2026-07-15T15:44:13Z
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 2 (2 low) |
| Heuristic | 2.1s |
| Duplication | 0.0s |
| Interrogation | 219.1s (2 bridge agents) |
| Total | 221.2s |
💰 Value — sound
Widens the default generation-failure distiller's truncation bounds (judge notes 400→1500 chars, errors 200→500) so executable-judge tracebacks survive to the reflective proposer; small, in-grain, and the giant file list is just an origin/main merge.
- What it does: In src/improvement/improve.ts:299-313,
generationFailureDistillernow slices judge notes at 1500 chars (was 400) and cell errors at 500 (was 200), keeps theclaimfield that came in from main's merge, and leaves the finding shape, the CAP of 12 failures, and the 0.999 pass threshold unchanged. src/improvement/raw-trace-distiller.ts:6,14 doc references to the old ~400-char digest are updated. T - Goals it achieves: Executable judges put tracebacks and failing assertions in notes; at 400 chars those arrived at the next proposal round as stubs, leaving the GEPA-style reflective proposer trace-blind (code comment at improve.ts:299-301). Widening lets a real traceback survive while the 12-finding cap keeps prompt growth bounded (worst case ~12 × 2k chars). Companion to agent-eval#373's trace-evidence fix.
- Assessment: Good on its merits. It is a two-line behavior change with a documented rationale, no shape change, no new dependencies, and it sits exactly where the existing design intends: the default distiller is deliberately dependency-free and replaceable via
opts.analyzeGeneration(improve.ts:265-270). Existing test improve.test.ts:550 exercises the distiller→proposer plumbing. The bounds chosen (1500) ma - Better / existing approach: Checked for an existing equivalent:
rawTraceDistiller(src/improvement/raw-trace-distiller.ts) already feeds full raw traces to the proposer, but it is the heavier opt-in meta-harness mode (rawTraceEvidenceflag, improve.ts:107-115) requiring agentic diagnosis; widening the lightweight default digest is complementary, not duplicative. No shared truncation helper/constant exists to reuse (searc - Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound
A small, correctly-placed constants change (400→1500 notes, 200→500 error) in the default failure distiller, cleanly merge-resolved with main's new claim field — real tracebacks now survive to the proposer.
- Integration: Fully reachable: generationFailureDistiller (improve.ts:271) is the DEFAULT analyzeGeneration producer selected at improve.ts:614 for every improve() call that doesn't opt into raw-trace mode or the memory surface. Its output (notes/claim/error) feeds the gepaProposer/skillOptProposer that every prompt/skills improvement run uses. The claim field (added by main, not this PR) is consumed by memoryC
- Fit with existing patterns: In-grain: generationFailureDistiller IS the established ACE-style findings path — the only alternative is the opt-in rawTraceDistiller (meta-harness mode), which is a deliberate companion, not a competitor. Widening bounds here is the minimal correct lever, not a new abstraction. Doc comments in raw-trace-distiller.ts and improve.ts were updated to match the new 1500-char figure.
- Real-world viability: Holds up on edge paths: the loop continues before any push when there is no error AND composite>=0.999 (improve.ts:298), so a finding always carries a real failure signal; when notes is empty but error present, claim falls back to a scenario-message; when both empty, claim is omitted via the conditional spread (improve.ts:312). Worst-case findings payload is 12 (CAP) x 1500 = ~18k chars, trivially
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🔎 Heuristic Signals
🟡 Cruft: console debug added bench/scripts/run-package-tests.mjs
+console.log(
package tests passed: ${tests.length}/${tests.length} TypeScript files + Pier bridge)
🟡 Cruft: magic number added bench/pier_agents/tangle_candidate.py
timeout_seconds = self._contract.timeout_ms / 1000
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.
|
| State | Detail |
|---|---|
| Interrupted | max run seconds |
No review verdict was produced for this run. Trigger a fresh review on the current PR head if the PR is still open.
tangletools · #555 · model: kimi-for-coding · updated 2026-07-15T17:59:58Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — d4fcf3dd
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.
tangletools · auto-approval · reason: drewstone_author · 2026-07-15T18:07:10Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 4 (1 medium-concern, 2 low, 1 weak-concern) |
| Heuristic | 3.0s |
| Duplication | 0.0s |
| Interrogation | 225.5s (2 bridge agents) |
| Total | 228.5s |
💰 Value — sound-with-nits
Widens the default failure distiller's evidence bounds (notes 400→1500 chars, errors 200→500) so real tracebacks reach the proposer — a small, correct, in-grain fix — but the PR vehicle also carries ~1.1k lines of unrelated ablation-suite examples hidden inside a commit labeled 'merge origin/main',
- What it does: In generationFailureDistiller (src/improvement/improve.ts:271-321), the per-finding
notesclip goes from 400 to 1500 chars and theerrorclip from 200 to 500 (also applied to theclaimfallback at line 307, keeping main's claim field). Doc comments in improve.ts:107 and raw-trace-distiller.ts:6,14 are updated to match. Shape of findings, CAP=12 finding limit (line 274), and sort order are un - Goals it achieves: Make the default GEPA-style reflection loop trace-aware: executable judges put their traceback/failing assertion in notes, and at 400 chars those arrived at the proposer as stubs, so the proposer was 'trace-blind'. 1500 chars keeps a real traceback intact while CAP=12 still bounds worst-case prompt growth to ~18k chars. The companion rawTraceDistiller (full filesystem traces) already exists for th
- Assessment: The core change is good on its merits. The rationale (tracebacks don't fit in 400 chars) is concrete and documented inline; the wider bounds are coherent with the existing CAP=12 finding cap; the
analyzeGenerationoption remains the full-replacement seam, so hardcoding a better default instead of adding config knobs matches the codebase's grain (raw-trace-distiller.ts is the complementary heavyw - Better / existing approach: None for the titled change — widening the existing default distiller's bounds is the right lever; callers needing more already have opts.analyzeGeneration / rawTraceDistiller (src/improvement/raw-trace-distiller.ts), and I found no other clipping site that needed the same fix (grep for slice(0,400/200) across src/improvement shows only this site, now updated). For the PR itself, the better packagi
- Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound
A minimal, correct widening of the default failure-distiller's character bounds (notes 400→1500, error 200→500) so real tracebacks survive to the prompt proposer — on the hot path, no competing pattern, nothing dead.
- Integration: Fully reachable.
generationFailureDistiller(improve.ts:271) is the DEFAULTanalyzeGenerationfor everyimprove()call that doesn't opt intorawTraceContextor use the memory surface — wired at improve.ts:614. ThememoryGenerationDistillerwraps it (improve.ts:329). Theclaimfield it now emits (merged from main) is consumed downstream bybuild-prompts.ts:26(f.claimrendered into - Fit with existing patterns: Perfect fit — it's a parameter bound change on the existing, established distiller, not a new abstraction. The alternative
rawTraceDistilleris explicitly opt-in (rawTraceContext: true) and serves a different philosophy (raw filesystem traces vs compressed findings); no competition. Doc comments in raw-trace-distiller.ts were updated in lockstep (400→1500) to stay accurate. - Real-world viability: Holds up trivially.
.slice(0, N)is pure string truncation — widening bounds cannot break. Max payload is 1500 chars × 12 findings (CAP at improve.ts:274) = ~18k chars, negligible in any LLM context window. Edge cases all handled: empty notes →claimfalls back to error-derived string (improve.ts:307); empty notes + no error → conditional spread omitsclaim; clean generation → static seed fa - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🔎 Heuristic Signals
🟡 Cruft: console debug added bench/scripts/run-package-tests.mjs
+console.log(
package tests passed: ${tests.length}/${tests.length} TypeScript files + Pier bridge)
🟡 Cruft: magic number added bench/pier_agents/tangle_candidate.py
timeout_seconds = self._contract.timeout_ms / 1000
💰 Value Audit
🟠 ~1,100 lines of untitled ablation-suite examples ride inside a commit labeled 'merge origin/main' [proportion] ``
Commit d4fcf3d ('merge origin/main into feat/gepa-trace-evidence', single parent 52951cc — not a real merge) adds examples/ablation-suite/aisdk-env.ts (+279), multi-contract-env.ts (+284), run-aisdk.ts (+153), run-multi-contract.ts (+189), extract-contracts.mjs (+50) and fixtures. Verified these exist ONLY on this branch (git log --all shows d4fcf3d as the sole commit; files absent from origin/main). They appear nowhere in the PR title/body, and GitHub's 187-file diff makes them unreviewable. Sp
🟡 Branch conflicts with current main and its snapshot-merge downgrades recent main contract changes [maintenance] ``
git merge-tree --write-tree origin/main HEAD conflicts (README.md and others). The d4fcf3d snapshot merged an older main, so relative to current origin/main the branch's tree downgrades #546-#557 work (candidate-execution schemaVersion 2→1, removed conversation subpath export in tsup.config.ts, removed verify-primeintellect-live.mjs — visible in git diff origin/main..HEAD). A real rebase/merge against current main is needed before landing; in a repo moving this fast the clean fix is to reduce th
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.
❌ Needs Work —
|
| opencode-kimi | glm | aggregate | |
|---|---|---|---|
| Readiness | 0 | 14 | 0 |
| Confidence | 95 | 95 | 95 |
| Correctness | 0 | 14 | 0 |
| Security | 0 | 14 | 0 |
| Testing | 0 | 14 | 0 |
| Architecture | 0 | 14 | 0 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 8/8 planned shots over 218 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 218 changed files. Global verifier still owns final merge decision.
Blocking
🔴 HIGH Root-run git against candidate-owned .git executes core.fsmonitor hooks (uid-65532 sandbox escape) — bench/pier_agents/tangle_candidate.py
prepare_identity chown -R's the workspace roots (including .git) to the candidate uid (process_boundary.py:100-103), so candidate code can write .git/config during run(). After the run, this function executes
git -c core.hooksPath=/dev/null reset --mixed,add -A -- .,rm --cached,diff --cached --quiet, andcommitas user="root". hooksPath is the only config neutralized; candidate_git_env (workspace_boundary.py:111-122) sets safe.directory but never core.fsmonitor. A candidate who setsgit config core.fsmonitor <script>gets that script executed as root by the evaluator'sgit add -A -- .(index refresh invokes the fsmonitor hook). The repo's own TS side treats this as a live vector: git-materialize.ts:472-478 forces `-c core.fsmonitor=false -c core.untrackedCache=false -c pro
🔴 HIGH docs/api/index.md stale — CI docs:check gate fails at head — docs/api/index.md
Committed docs say '~400-char distilled findings' but head source src/improvement/improve.ts:107 says '~1500-char' (changed by commit 52951cc, which did not regen docs). Reproduced at d4fcf3d:
pnpm run docs:apiproduces a 3-hunk diff, sogit diff --exit-code -- docs/apiindocs:check(ci.yml:38-39, runs on every PR to main) fails. Two more stale hunks same root cause: line 13288 links applyImprovementWinnerToProfile at improve.ts:466 (actual 469); line 13317 links improve() at improve.ts:529 (actual 532). Fix: runpnpm run docs:apiand commit
Other
🟠 MEDIUM Candidate-owned .git/config is trusted by evaluator git operations, enabling fsmonitor/filter code execution as root on older git — bench/pier_agents/tangle_candidate.py
prepare_identity (process_boundary.py:102) runs
chown -R 65532:65532 task_root, recursively transferring ownership of the entire workspace INCLUDING .git/ to the untrusted candidate uid. After the candidate exits, _restore_profile_and_capture_solution runsgit add -Aas root (line 721) with only-c core.hooksPath=/dev/nulloverriding config. The candidate can setcore.fsmonitor=/tmp/evilor[filter "x"] clean=/tmp/evil+* filter=xin.gitattributesin the now-candidate-owned.git/config.git add -Ainvokes the fsmonitor program and/or clean filters, executing candidate-controlled code as ROOT inside the container. On git >= 2.40.1, g
🟠 MEDIUM .sidecar metadata in the task root is staged into the candidate solution commit — bench/pier_agents/tangle_candidate.py
git add -A -- .has no exclusion for.sidecar. The workspace identity check explicitly tolerates{task_root}/.sidecarexisting (workspace_boundary.py:28-33 prune with allow_task_metadata=True, used for the task root at tangle_candidate.py:552-557), and candidate_contract.py:33 reserves the name precisely because the harness may place it in workspace roots (runtime tests plant.sidecarwith 'unsigned'/'hidden executable bytes': tests/candidate-execution-prepare.test.ts:606, tests/candidate-execution-core.test.ts:218). If.sidecarexists at capture time — a state this code is written to accept — its contents are committed and appear in the base..HEAD patch as candidate work, contaminating the graded solution with harness metadata. Fix: exclude it, e.g. `git add -A -- . ':(exclude).
🟠 MEDIUM Isolated-user execution path (setpriv watchdog, uid kill loop, chown, uid-in-use probe) has no test coverage — bench/pier_agents/tangle_candidate_test.py
Every end-to-end test subclasses TangleCandidateAgent with ISOLATE_CANDIDATE_USER=False, which (a) makes prepare_identity return before the chown/uid-probe block (process_boundary.py:89-107) and (b) selects the non-isolated
timeout-based runner (process_boundary.py:209-231). The production runner (lines 232-321: setsid+setpriv launch, watchdog TERM/KILL escalation, atomic marker via mv -fT, per-uid /proc kill loop) and the isolated branch of kill_and_wait are exercised only by test_process_stop_acknowledges_candidate_identity_is_empty, which covers just the trivial no-process case. The security boundary — including the exact root-privilege
🟠 MEDIUM Grader pinned in the signed execution plan is never enforced at execution time — src/candidate-execution/outcome-evidence.ts
prepare.ts:294 pins
grader: task.grader(name/version/artifact) into the signed execution plan, but execute.ts:391-399 passesoptions.graderstraight through and this function runs/binds only that port. benchmark-grader.ts:60-72 verifies the implementation bytes against the port's own artifact, and outcome-evidence.ts:251-255 records the ACTUAL grader in the receipt — nothing anywhere compares the used grader tostate.executionPlan.value.material.grader(grep formaterial.graderin src returns only the evidence write). Every other plan-pinned value is drift-checked (resolveContainer 'resolved container source drifted', resolveModel 'model resolver drifted', reservation limits/network), so a miswired evaluator can grade with a different grader than the plan declares and still finali
🟠 MEDIUM 1500-char notes never reach the default prompt/skills proposers — renderer truncates claim to 300 — src/improvement/improve.ts
The new comment (lines 299-301) justifies the cap raise as keeping 'a real traceback / failing assertion intact' so the proposer is no longer 'trace-blind'. But for the two default LLM-reflection surfaces this is not realized: gepaProposer (surface 'prompt', the default) and skillOptProposer (surface 'skills') both render findings through renderAnalystEvidence in @tangle-network/agent-eval, which reads only f.claim and emits truncate(claim, 300) — notes/error fields are dropped entirely and claim is cut to 300 chars. Verified in node_modules/@tangle-network/agent-eval/dist/chunk-HQPHZGL6.js:2190-2215 and its use in gepaProposer (:2429) and skillOptProposer (
🟠 MEDIUM defineLeaderboard resolveCostModel throws on multiple served models — behavior change for multi-model cells — src/runtime/define-leaderboard.ts
The new resolveCostModel collects served models from ALL iterations via spec.resolveModel and throws if >1 distinct model is found:
defineLeaderboard(${spec.name}): one cell reported multiple served models. The old code called observeModel per-iteration without this check. A cell that legitimately routes to different model snapshots across iterations (e.g. provider-side load balancing, fallback routing) will now throw mid-run and abort the entire leaderboard. If multi-model cells are expected in any deployment, this hard-fail is too strict — consider logging a warning and picking the majority model instead of throwing.
🟠 MEDIUM sandboxCheckRunner: candidate can override print() to forge the check verdict despite the nonce — src/runtime/structural-rollout.ts
The nonce (randomBytes(8).toString('hex')) is embedded in the scaffold's print() call AFTER the candidate code runs at module level. A candidate that does
import builtins; builtins.print = lambda *a: __import__('sys').stdout.write('SRCK-<learned-nonce> official=3/3 authored=2/2\n')would intercept the scaffold's print, learn the real nonce from the argument, and emit a forged summary. The regex (line 297) matches the first occurrence in stdout, so the forged line wins. Mitigating factors: (1) the candidate runs in a sandbox by design, (2) the harness-verified score (surface.score) is authoritative — visible checks only drive selection, not final scoring,
🟡 LOW GNU-only shell primitives without capability probes; busybox userland misclassifies timeouts — bench/pier_agents/process_boundary.py
The watchdog renames the marker with
mv -fT(GNU-only; busybox mv lacks -T). Other scripts rely onrealpath -e,stat -c %h/%a,find -printf '%P', andtimeout --signal/--kill-after(workspace_boundary.py:43-60, process_boundary.py:221-223), but prepare_identity probes onlysetsidandsetpriv(process_boundary.py:96-97). The signed contract pins only platform (linux/amd64|arm64), not userland; on a busybox-based image the mv -fT failure means the timeout marker is never created, so a killed candidate is recorded as termination kind 'exit' 137 and NonZeroAgentExitCodeError instead of kind 'timeout' + asyncio.TimeoutError (tangle_candidate.py:889-907) — silently wrong trial metadata. Fix: probe the required userland in prepare_identity (command -v realpath/stat/mv/timeout/find) w
🟡 LOW Non-isolated runner (test mode) uses bare timeout without process-group kill, leaving orphan candidates — bench/pier_agents/process_boundary.py
When ISOLATE_CANDIDATE_USER=False (test/local mode), the runner uses
timeout --signal=TERM --kill-after="${grace}s" "${deadline}s" env "$@".timeoutonly kills the direct child process, not the process group. A candidate that forks background workers would leave them running after timeout fires. kill_and_wait is a no-op in this mode (line 133-134). This is test-only today (production uses ISOLATE_CANDIDATE_USER=True with setsid+uid-sweep), but if the non-isolated path is ever used for real evaluation, orphaned processes could corrupt results. The isolated branch correctly uses setsid for session isolation. Low severity given current usage.
🟡 LOW config.uid interpolated directly into grep regex pattern without escaping — bench/pier_agents/process_boundary.py
Line 98 builds
f"! grep -l '^Uid:[[:space:]]*{config.uid}[[:space:]]' /proc/[0-9]*/status"by interpolating config.uid into a grep regex. config.uid is int-typed so it's always numeric today, but this is a regex-injection landmine: if the field ever becomes string-typed (e.g., to support named UIDs), metacharacters would be interpreted by grep's regex engine. The kill_and_wait path (line 170) correctly passes uid via TARGET_UID env and reads it as a shell variable — apply the same pattern here for consistency and
🟡 LOW Evaluator trace env vars leaked to candidate process via inherited environment — bench/pier_agents/tangle_candidate.py
process_env (line 767-773) includes TANGLE_CANDIDATE_EXECUTION_ID, TANGLE_CANDIDATE_BUNDLE_DIGEST, TANGLE_CANDIDATE_EXECUTION_PLAN_DIGEST, TANGLE_CANDIDATE_MATERIALIZATION_RECEIPT_DIGEST, and TANGLE_TRACE_RUN_ID. The wrapper script inherits this environment and passes it to the candidate via
env "$@"(env merges with inherited env, doesn't replace). The candidate can read all of these via /proc/self/environ. This may be intentional for trace correlation, but if evals assume the candidate doesn't know its execution identity or bundle digest, this leaks it. The signed env (self._contract.env) is correctly passed viaenv "$@"as explicit assignme
🟡 LOW Generic-exception cleanup path skips remaining cleanups and masks the original error — bench/pier_agents/tangle_candidate.py
In the non-cancellation exception path, kill_and_wait, _remove_instruction_file, _restore_profile_and_capture_solution, and remove_control_files are awaited sequentially with no per-step error capture; if kill_and_wait raises, the profile is never restored (candidate-tampered AGENTS.md etc. survives into Pier's verifier view) and the original exception is replaced by the cleanup error (only context chains it). The CancelledError branch (lines 805-845) already implements the correct collect-and-continue pattern with cleanup_errors; apply the same there. Fix: wrap each of the four cleanups in try/except collecting errors, then re-raise the origi
🟡 LOW observedElapsedMs shape differs between cancelled and exit/timeout metadata — bench/pier_agents/tangle_candidate.py
On cancellation the metadata is
termination: {kind: 'cancelled', observedElapsedMs: elapsed}(nested), while the exit/timeout path emitstermination: {kind, ...}plus a top-levelobservedElapsedMs(lines 889-897). Consumers reading metadata['observedElapsedMs'] uniformly (as the timeout/exit shape implies) get None for cancelled trials. Fix: hoist observedElapsedMs to top level in the cancelled branch.
🟡 LOW verify_pier_execution_identity has zero test coverage — image/platform binding is stubbed out in all tests — bench/pier_agents/tangle_candidate_test.py
The test agent subclass overrides _verify_pier_execution_identity with a no-op
del environment(line 79-80), so the real implementation in workspace_boundary.py:207-236 (which checks environment.task_env_config.docker_image against the signedimage@indexDigest, validates OS==linux, and verifies uname architecture) is NEVER exercised. If the Pier attribute namedocker_imagechanges, ortask_env_config.os.valuedoesn't match expectations, every production run would fail with a confusing error and no test would catch it. The pinned-image comparison is a core security control (proves the container matches the signed immutable image). Add a
🟡 LOW hev-eval.mts silently returns empty string on API HTTP errors — bench/src/hev-eval.mts
if (!res.ok) return ''swallows HTTP errors (429, 500, auth failures) without logging. All tasks would score 0 with no indication that the API was down rather than the model failing. Impact: a misconfigured API key or rate-limiting produces a silent 0% pass rate indistinguishable from model incompetence. Fix: throw on non-OK responses (like hev-improve.mts line 40 does), or at minimum log the error to stderr.
🟡 LOW Near-verbatim code duplication between hev-structural.mts and mbpp-structural.mts — bench/src/mbpp-structural.mts
The docker semaphore (~10 lines), jailed runner (~55 lines), container reaper (~8 lines), and sign test (~15 lines) are duplicated almost identically from hev-structural.mts. The author acknowledges this ('kept self-contained on purpose — the HumanEval rig is frozen post-verification'), but any bug fix or Docker version compatibility change must be applied in two places. Impact: maintainability debt; if one copy diverges, MBPP results may not be directly comparable to HumanEval results. Fix: extract the shared docker-infrastructure into a module imported by both scripts; keep the task-specific logic (prompt construction, test generation, dataset loading) in each file.
🟡 LOW Substring matching for exception type detection could theoretically false-positive — bench/src/pier-agent.ts
Lines 428 and 437 use
type?.includes('AgentTimeout')andtype?.includes('Cancelled')to classify termination from Pier's exception_info. If a future Pier exception name contains these substrings incidentally (e.g., 'NotCancelledError'), it would be misclassified. Impact: low — Pier's exception types are currently well-defined and these are the only matching patterns. Fix: use exact equality or regex anchors (e.g., /^(Agent)?Timeout/, /^Cancelled$/).
🟡 LOW pier-task-outcome.ts git child process receives full process.env minus GIT_* — bench/src/pier-task-outcome.ts
The git function copies all of process.env (filtering only GIT_* prefixed vars) into the child environment. While candidate code is never executed by these git operations (patch is applied via
git apply --cached), sensitive evaluator variables (API keys, tokens) are exposed to the git process. Impact: low — git itself doesn't log env vars and the operations are evaluator-trusted, but defense-in-depth suggests passing a minimal environment. Fix: pass only the specific environment variables git needs (HOME, PATH, LC_ALL, LANG) plus the controlled GIT_* and extraEnvironment entries.
🟡 LOW stopAndCapture receives an already-aborted signal on the plain executor-error path, contradicting its documented contract — src/candidate-execution/execute.ts
When executor.execute rejects before the deadline, line 586 aborts the controller and stopAndCapture (line 590-606) is then invoked with that pre-aborted signal. types.ts:504-506 documents the stop signal as 'Aborted at the frozen execution deadline or evaluator cleanup deadline' — neither holds here. recover.ts:57-58 does the same. The module's own test (tests/candidate-execution-execute.test.ts:1138-1142) codifies that executors must still teardown on an aborted signal in the timeout case, so behavior is intention
🟡 LOW verifyCandidateCode writes new objects into the shared repository object database during verification — src/candidate-execution/git-materialize.ts
For 'git-patch' candidates,
git apply --cached(line 43-48) andwrite-tree(line 49-55) run with only GIT_INDEX_FILE redirected to a temp file; new blob/tree objects are written into the resolved repository's real $GIT_DIR/objects. The repository port contract (types.ts:66-69) implies resolving an already-present local object store, and the sibling verifyTaskOutcomePatch (same file, [lines 143-147](https://github.com/tangle-network/agent-runtime/blob/d4fcf3ddc424aa4ab91243e03668f6cfacf1ccd9/src/
🟡 LOW Archive decode memory budget rejects workspaces the capture side's own limits permit — src/candidate-execution/workspace-archive.ts
Decode-side, retained entry bytes are capped at
maxArchiveBytes - archiveBytes(archive buffer + decoded entries must jointly fit maxArchiveBytes). Capture-side, captureAgentCandidateWorkspace allows maxTotalFileBytes (default 256MB) of files plus a maxRepositoryBundleBytes (default 128MB) git bundle inside a maxArchiveBytes (512MB) archive. A task workspace of e.g. 200MB files + 100MB bundle produces a ~300MB archive that captures fine, then materializeAgentCandidateWorkspace rejects it at line 435/454/466 because 300MB retained entries exceed 512-300=212MB. Effective repo-carrying workspace ceiling is ~128MB of files with default limits, hal
🟡 LOW No test pins the new 1500/500 truncation caps — src/improvement/improve.test.ts
The behavior change ships with no test update: grep across improve.test.ts, raw-trace-distiller.test.ts, and profile-diff-proposer.test.ts finds no assertion on note/error truncation length (nothing broke, but nothing guards the new contract either). Given the change exists specifically to preserve tracebacks, a cheap test — judge notes of ~1000 chars survive into the finding, notes >1500 are clipped — would pin the intent. (Vitest would not execute in this sandbox — silent exit 1 on pre-existing unrelated test files too — so this is a coverage observation, not a verified failure.)
🟡 LOW Comment 'Errors stay tighter' is misleading — errors were also raised (200→500) — src/improvement/improve.ts
The added comment says 'Errors stay tighter (they are usually one-line)', but the diff raises error slices from 200 to 500 in both places (line 307 claim fallback, line 313 error field). Errors are only 'tighter' relative to the new 1500 notes cap, not tighter than before. Nit: reword to 'Errors stay tighter than notes (500 vs 1500)' or similar.
🟡 LOW Slice caps are unnamed magic numbers — src/improvement/improve.ts
The 1500 (notes) and 500 (error) slice limits are inline literals duplicated across the notes/claim/error fields and referenced again in doc comments (improve.ts:107, raw-trace-distiller.ts:6,14). If one site drifts the comment becomes a lie again (this very PR is fixing exactly that drift). Hoist to named consts (e.g. NOTES_CAP=1500, ERROR_CAP=500) referenced by both the slices and the comments. Non-blocking; current values are internally consistent.
🟡 LOW Merge resolution discarded branch's pre-merge versions of index.ts and delivery.ts — src/intelligence/delivery.ts
At the branch parent 52951cc, delivery.ts had blob 1ef3cc24 and index.ts had blob 26fbd2ad — both DIFFERENT from main's versions (0192e55a and f81bf8f2 respectively). The commit d4fcf3d resolved both to main's versions (head blobs match base blobs exactly). The commit message ('keep 1500/500 evidence widths + main's claim field') confirms this was deliberate for evidence-width fields, but if the branch had other intentional intelligence-delivery changes (e.g., the older delivery.ts lacked capabilities/agentProfileDiffs/agentProfile fields that main added), confirm those were meant to be superseded by main rather than merged alongside. This is not a diff defect — it is a merge-strategy note for the PR author.
🟡 LOW verifyAgentImprovementProposal does not validate surface for non-ship proposals — src/intelligence/improvement-cycle.ts
Verification checks kind/schemaVersion/digests/profile-binding, but
surfaceis only exercised insideassertMeasuredProfileBindingwhengateDecision === 'ship'(via measuredWinnerProfile, which throws on unknown surfaces). A hand-crafted non-ship proposal withsurface: 'garbage'passes verify. Impact is minimal (surface only drives the winner-binding), but the trust-boundary verifier should reject unknown surfaces unconditionally for forward-compat when new surfaces are added.
🟡 LOW Failed runs export with OTLP span status OK — src/intelligence/index.ts
The run span is built with
flatOtelSpan, which hardcodesstatus: { code: 1 }(STATUS_CODE_OK) — a run withoutcome.success === falseor a recordederrorstill ships an OK span; failure exists only inerror.type/error.message/tangle.outcome.successattributes. Any downstream reader keying on standard OTLP span status (collectors, dashboards, SLO queries) will count failed runs as successful. Fix: set status ERROR (code 2) when the record carries an error or explicitsuccess === false.
🟡 LOW Removed OTLP endpoint env vars can silently reroute spans to the default public plane — src/intelligence/index.ts
resolveEndpoint(INTELLIGENCE_OTLP_ENDPOINT / OTEL_EXPORTER_OTLP_ENDPOINT) was deleted;otlpEndpointnow always derives from baseUrl/TANGLE_INTELLIGENCE_URL, defaulting tohttps://intelligence.tangle.tools/v1/otlp. A deployment configured only via the old env vars with TANGLE_API_KEY set will, after upgrade, ship spans to the default public plane instead of its custom collector — a data-routing change, not a no-op. Acceptable for an @experimental 0.x module, but it should be called out in release notes/changelog; grep confirms no in-repo references remain.
🟡 LOW Unguarded Buffer.byteLength silently kills telemetry in non-Node runtimes — src/intelligence/index.ts
The module otherwise guards non-Node environments (
typeof process !== 'undefined'checks for env vars), butBuffer.byteLengthis called unguarded. In a browser/edge runtime without a Buffer polyfill this throws ReferenceError; because both export paths wrap it in try/catch, the failure is swallowed and telemetry silently never ships even when fully configured with apiKey + baseUrl. Fix: compute UTF-8 length without Buffer (e.g.new TextEncoder().encode(s).length) or guard ontypeof Buffer !== 'undefined'.
🟡 LOW onProposals never fires on demotion-to-empty and suppresses re-promotion — src/intelligence/with-intelligence.ts
if (proposals.length === 0) returnmeans a refresh that demotes all diffs (non-empty → empty) never notifies the consumer, andlastSignalkeeps the stale signature — if the same diff set is later re-promoted,sig === lastSignalsuppresses the callback, so a consumer tracking the promoted set via onProposals misses both the demotion and the re-promotion. Fix: fire with[]on the non-empty→empty transition and resetlastSignalwhen the set empties.
🟡 LOW Shot dispatched against files with zero net PR diff (pre-existing main code) — src/primeintellect/package.ts
All 7 assigned files (package.ts, traces.ts, runner.ts, validation.ts, types.ts, index.ts, primeintellect.test.ts) have an EMPTY diff between the declared PR base 5a799d9 and head d4fcf3d (verified: per-file
git diff= 0 lines; blob SHAs identical base-vs-head). These files were introduced by PR #553, whose merge commit on main is exactly 5a799d9 (the PR base). PR #555's head is a merge of origin/main into feat/gepa-trace-evidence; it pulls primeintellect onto the feature branch unchanged. Impact: this shot flags +1639 lines because they appear when diffing against the feature branch's own pre-merge tip (52951cc), but relative to the PR's declared base the primeintellect module is pre-existing, already-shipped main code and not part of PR #555's contribution. No action required for merge
🟡 LOW Task JSON delivered via env var caps prompt/metadata size — src/primeintellect/package.ts
The generated harness passes TANGLE_PRIME_TASK_JSON and TANGLE_PRIME_MCP_SERVERS_JSON as environment variables to run_program. exec env limits (~2MB total on Linux) mean a task whose prompt contains image_url data-URLs or large message-array history will fail the rollout at spawn. Failure is loud (spawn error → trace error → failureMode), not silent, so this is an operational constraint worth documenting in the generated README rather than a correctness bug.
🟡 LOW VERSION regex falsely rejects valid semver with prerelease+build — src/primeintellect/package.ts
/^\d+.\d+.\d+(?:[-+][a-zA-Z0-9.-]+)?$/ allows only ONE of prerelease or build metadata: '1.0.0-rc.1+build.5' is valid semver but rejected, while '1.0.0-.' (junk) passes and fails later at hatchling build with a worse error. Fix: /^(?:-(?:0|[1-9]\d*|\w[\w-])(?:.(?:0|[1-9]\d|\w[\w-])))?(?:+[\w-]+(?:.[\w-]+)*)?$/ or delegate to a semver lib. Impact: package creation throws a confusing error for legitimate versions.
🟡 LOW writePrimeIntellectPackage reports failure after successful install if backup cleanup fails — src/primeintellect/package.ts
If
rm(backup, {recursive:true})throws after the new package was renamed into place, the catch block removestemporary(already renamed — force:true makes it a no-op) and rethrows, so the caller sees an error even though the new package is fully installed and the old one sits in a.backupsibling. Data-safe but misleading; wrap the cleanup rm in its own try or log-and-continue.
🟡 LOW failureMode interpolates unbounded error message — src/primeintellect/traces.ts
primeintellect:${errors[0].type}:${errors[0].message}embeds the raw provider/framework error message with no length or newline bound. Prime error messages can carry long upstream diagnostics; failureMode then pollutes RunRecord-based reporters and any TSV/JSONL aggregation. Fix: truncate to ~200 chars and strip newlines (the full error remains in the trace itself).
🟡 LOW Stale/typo'd comment in dispatch wiring — src/runtime/define-leaderboard.ts
The comment reads '...and a the cost receipt records the spec-resolved served model' — grammar artifact from the observeModel → paid-call receipt rewrite. Cosmetic only.
🟡 LOW Check program piped as a single shell argument — large candidates can hit exec-channel arg limits — src/runtime/structural-rollout.ts
printf '%s' '${b64}' | base64 -d | ${python} -inlines the entire base64'd check program (candidate + scaffold) as one shell argument. A very large candidate (tens of KB, e.g. a model emitting a long module) inflates ~4/3x and can exceed sandbox exec arg/string ceilings, failing the run ascrashedand silently ranking that candidate lowest — a selection bias against long-but-valid candidates, not just an error. The b64 alphabet is quote-safe so there is no injection risk. Inherited from the proven rigs' pattern, so acceptable, but a stdin/file-based channel (box.writeFile +python3 /tmp/x.py) would remove the ceiling.
🟡 LOW filterAuthoredAsserts balanced() uses a single counter for all bracket types — accepts mismatched nesting — src/runtime/structural-rollout.ts
The balanced() function increments/decrements a single counter for (), [], and {} combined. An assert like
assert f([1)] == 1(mismatched nesting) passes the balance check (d goes 1→2→1→0). For the intended purpose (rejecting truncated/malformed single-line asserts) this is an acceptable approximation, but it's technically incorrect bracket matching.
🟡 LOW structuralRollout budget requirement (k + repairRounds + 1) is documented but not enforced — src/runtime/structural-rollout.ts
The comment says 'pass at least k + repairRounds + 1 so the samples, repairs, and the check-author consult all admit.' If the caller passes a smaller budget, shot() and consult() return null on pool starvation. The strategy handles this by breaking loops (line 444:
if (!out) break), but a caller with budget=3 and k=5/repairRounds=2 would silently get 3 samples and 0 repairs — no authored checks either — without any warning. Consider a runtime assertion or at minimum a console.warn when budget < k + repairRounds + 1.
🟡 LOW reconcile() throws after open.delete, permanently leaking the reservation in pool counters — src/runtime/supervise/budget.ts
The new
usdCapped && spent.usdKnown === falsethrow fires afteropen.delete(ticket.id)(line 191) but before the reserved amounts are released (lines 223-239). When it fires, the ticket is un-tracked yet its reserved tokens/usd/iterations stay inreservedTokens/reservedUsd/reservedIterationsforever — the pool's free balance is permanently degraded andassertNoOpenTicketscannot see it. This mirrors the pre-existing over-spend throws at [lines 203-219](https://github.com/tangle-network/agent-runtime/blob/d4fcf
🟡 LOW Math.min/Math.max spread over unbounded span array — src/runtime/supervise/trajectory-recorder.ts
Math.min(...spans.map(s => s.startedAt))/Math.max(...)spreads the span array as function arguments; JS engines throw RangeError (stack overflow) around ~65k-125k arguments depending on engine. Worker tool-span counts are realistically far below that, so this is theoretical today, but a pathological TraceSource (e.g. a long sandbox box session) would crash the settle-time analyst instead of degrading. Fix: reduce loop instead of spread.
🟡 LOW trajectory-recorder Math.min/max spread can overflow call stack on very large traces — src/runtime/supervise/trajectory-recorder.ts
Math.min(...spans.map((span) => span.startedAt))andMath.max(...spans.map((span) => span.endedAt ?? span.startedAt))use the spread operator on arrays of arbitrary size. For traces with >~100k spans this hits the V8 argument limit (~65536 on most engines) and throws RangeError. Use a reduce-based min/max instead:spans.reduce((m, s) => Math.min(m, s.startedAt), Infinity).
tangletools · 2026-07-15T19:42:55Z · trace
tangletools
left a comment
There was a problem hiding this comment.
❌ 2 Blocking Findings — d4fcf3dd
Full multi-shot audit completed 8/8 planned shots over 218 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 218 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-07-15T19:42:55Z · immutable trace
The previous reconciliation with main (d4fcf3d) copied main's tree in a single-parent commit, so the PR's merge-base stayed at 2b18029 and the PR diff showed 218 files of main's own history as if this branch authored them. This commit records the merge properly: tree = origin/main (0179509) plus this branch's real contribution (examples/ablation-suite + the 1500/500 failure-distiller evidence widths in src/improvement).
typecheck:examples runs with noUncheckedIndexedAccess; the aisdk and multi-contract rigs indexed per-task vectors and task specs without narrowing, so pnpm typecheck failed at head.
… a test - hoist the 1500/500 slice bounds to DISTILLED_NOTES_MAX_CHARS / DISTILLED_ERROR_MAX_CHARS so the caps and the doc comments that cite them cannot drift apart - pin the contract in improve.test.ts: a traceback-sized note survives intact, a runaway note clips at exactly 1500, a cell error clips at 500 - regenerate docs/api so docs:check passes at head (the committed docs still described the ~400-char caps)
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 72bbfec3
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.
tangletools · auto-approval · reason: drewstone_author · 2026-07-15T20:09:35Z
|
Addressed at head Why the review saw 218 files: the branch's prior reconciliation with main ( HIGH — root-run git executes candidate HIGH — docs/api stale, docs:check fails at head: real, caused by this branch ( MEDIUM — 1500-char notes never reach the default prompt/skills proposers (renderAnalystEvidence claim→300): the wide evidence travels the trial-trace path, not the findings digest: the paired agent-eval PR (tangle-network/agent-eval#373) threads per-scenario judge notes (≤1500) and worst-rep emitted output (≤2000) through LOW — no test pins the 1500/500 caps; "Errors stay tighter" comment; magic numbers: fixed — caps hoisted to Remaining findings (grader plan-pinning, defineLeaderboard multi-model throw, structural-rollout nonce, process_boundary/watchdog items, intelligence OTLP/status items, primeintellect items): all pre-existing main code with zero net diff in this PR — not addressed here. Verification: |
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 4 (1 low, 3 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 222.7s (2 bridge agents) |
| Total | 222.7s |
💰 Value — sound-with-nits
Widens the GEPA failure-distiller evidence caps (notes 400→1500, errors 200→500) so tracebacks survive to the proposer, with named constants and a pinning test — correct and in-grain; the PR also quietly bundles ~1000 lines of unrelated ablation-suite example rigs, which are themselves fine but off-
- What it does: In src/improvement/improve.ts:310-322 the default generationFailureDistiller now slices judge notes at 1500 chars and cell errors (plus the error-derived
claimfallback) at 500 chars, via named constants DISTILLED_NOTES_MAX_CHARS/DISTILLED_ERROR_MAX_CHARS, instead of the previous hardcoded 400/200. A new test in src/improvement/improve.test.ts:588-643 pins all three behaviors (sub-cap note passe - Goals it achieves: Make the default improvement loop's reflection trace-aware: a real Python traceback or failing assertion (~1000 chars) previously never survived the 400/200 clipping, so the prompt proposer was diagnosing failures from a digest too small to contain the actual error. After merge, executable-judge notes reach the proposer intact, while the distilled path stays bounded (the rawTraceContext meta-harne
- Assessment: Good on its merits. The core change is the minimal correct fix: it keeps the ACE-style bounded digest (rather than collapsing the distinction with rawTraceDistiller, which exists precisely for full-trace evidence), names the caps, documents why the bound exists (clipping mid-traceback leaves the proposer trace-blind), and pins the exact clipping behavior with a realistic test including an intact-t
- Better / existing approach: None for the core change — I checked for existing equivalents: raw-trace-distiller.ts is the deliberate no-summarization alternative, not a duplicate; the caps could be made configurable, but the codebase's own grain (rawTraceContext for full traces, bounded digest otherwise) argues two fixed regimes is the right shape, and the companion agent-eval#373 covers the other side. For the examples, they
- Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound-with-nits
The core fix (widening the default failure-distiller's evidence caps so tracebacks survive to the proposer) is correct and on the live default path; the bundled ablation rigs fit the established example pattern but carry machine-specific default paths.
- Integration: Strong.
generationFailureDistilleris the DEFAULTanalyzeGenerationproducer for everyimprove()call that does not override it (improve.ts:614-623 — selected unlessrawTraceContext/memory surface/ explicit override). The widened caps (notes 400→1500, error 200→500) therefore take effect automatically for all standard prompt/code improvement loops; nothing needs to opt in. The new ablation - Fit with existing patterns: Excellent for the distiller change — it does not introduce a new pattern, it tunes the existing one (named constants
DISTILLED_NOTES_MAX_CHARS/DISTILLED_ERROR_MAX_CHARSreplace magic numbers, with a pinning test at improve.test.ts:588).rawTraceDistilleris the complementary meta-harness alternative, not a competitor. The ablation examples mirror the existingrun-X.ts+X-env.ts+ README - Real-world viability: Holds up. The new test (improve.test.ts:588) deliberately exercises the realistic case the PR targets: a ~1000-char traceback note that must survive whole, a runaway note clipped at exactly 1500, and a cell error clipped to ≤500 — so the bound is proven, not assumed. The
claimfallback (what actually reaches the proposer prompt) now uses the same 500-char error bound, so a failing assertion/trac - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🔎 Heuristic Signals
🟡 Cruft: magic number added src/improvement/improve.test.ts
- // A realistic executable-judge note (~1000 chars) must survive whole; a
💰 Value Audit
🟡 PR bundles ~1000 lines of unrelated ablation-suite rigs under a distiller-evidence title [proportion] ``
git diff origin/main...HEAD shows 11 files: the 20-line distiller change + test (the stated goal) AND aisdk-env.ts (282), multi-contract-env.ts (276), run-aisdk.ts (153), run-multi-contract.ts (189), extract-contracts.mjs (50), and fixtures — a web-search-ablation example suite with no relation to GEPA trace evidence. The examples are in-grain (same AgenticSurface pattern as the existing 8 envs in examples/ablation-suite/) and additive, so nothing here blocks merge — but a reviewer reading the t
🎯 Usefulness Audit
🟡 aisdk-env.ts ships a machine-specific default template path no one else has [ergonomics] ``
examples/ablation-suite/aisdk-env.ts:30defaultsTEMPLATEto/tmp/claude-1000/-home-drew-code-blueprint-agent/8b62a3c4.../scratchpad/aisdk-probe. It IS overridable viaAISDK_TEMPLATEand fails loudly inensureTemplate()if the dir lacksnode_modules/ai, so it won't silently misbehave — consistent with the existing rigs that also require external setup (e.g. EVAL=verkit). Worth a one-line README note on how to build the template (pnpm add ai@7 zod typescript) so the rig is reproducibl
🟡 extract-contracts.mjs hardcodes an absolute blueprint-agent source path [ergonomics] ``
examples/ablation-suite/extract-contracts.mjs:11points at/home/drew/code/blueprint-agent/scripts/experiments/.... This is a one-shot provenance generator whose OUTPUT is already committed (contracts.all.json / contracts.json), so it does not need to re-run for the rigs to work — but as a reusable tool it is author-only. Consider noting it is a regenerate-only script, or parameterizing the source root. No effect on the runnable rigs.
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.
❌ Needs Work —
|
| glm | kimi-code | aggregate | |
|---|---|---|---|
| Readiness | 0 | 0 | 0 |
| Confidence | 75 | 75 | 75 |
| Correctness | 0 | 0 | 0 |
| Security | 0 | 0 | 0 |
| Testing | 0 | 0 | 0 |
| Architecture | 0 | 0 | 0 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 3/3 planned shots over 11 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 11 changed files. Global verifier still owns final merge decision.
Blocking
🟣 CRITICAL Graders written into the worker workspace in plaintext — the 'hidden' answer is readable, defeating the multi-contract task — examples/ablation-suite/multi-contract-env.ts
runGrader writes Buffer.from(c.graderB64,'base64') to join(dir,'.vb_grader',c.testFile) and never removes it. dir IS the worker's workspace; the worker has read_file (line 208: join(dir, args.path) with no path guard) and bash (line 216: cat). I decoded committed graders: e.g. contracts.json[0] finch grader docstring states verbatim 'endpoint path /employer/directory, auth Authorization: Bearer <access_token>, version header Finch-API-Version: (REQUIRED; missing -> 400), response envelo
🔴 HIGH check.ts (the 'compiler is the judge' grader) is agent-writable and never restored at score time — examples/ablation-suite/aisdk-env.ts
open() writes
check.tsinto the worker's workspace once;write_file(line 229) accepts any relative path includingcheck.ts; compile() (line 112) and score() (line 246) type-check whatever check.ts currently contains. A worker that writescheck.ts=export {}(or deletes its import of solution) makes tsc pass and scores 1 without producing the current API — breaking the
🔴 HIGH Hidden graders written into the agent's own workspace — oracle leak contaminates the no-search control — examples/ablation-suite/multi-contract-env.ts
runGrader writes each decoded grader to
join(dir, '.vb_grader', c.testFile)— the samedirthe worker operates in — whileread_file(line 208) andbash(line 214) are ON in BOTH arms with unrestricted access to that dir. I decoded the graders: they state the full CURRENT contract in plaintext (POST /flags?v=2,api_keyin JSON body,EXPECTED_TOKEN = "phc_live_project_token_abc123", pagination rules). After the first run_tests call, a no-search worker canbash ls -a/ `read_file .vb_gr
🔴 HIGH bash tool inherits process.env (incl. TANGLE_API_KEY) and has no network sandbox — NO-SEARCH arm can self-search — examples/ablation-suite/multi-contract-env.ts
execFileSync('bash', ['-lc', cmd], { cwd: dir, timeout: 60_000, stdio: [...] }) — no env override, so the worker inherits the runner's full process.env. The runner requires TANGLE_API_KEY (run-multi-contract.ts:25-31 exits if unset). I simulated this: bash -lc 'echo $TANGLE_API_KEY' returns the key verbatim. The NO-SEARCH control arm can therefore run
curl -X POST https://router.tangle.tools/v1/search -H "authorization: Bearer $TANGLE_API_KEY" -d '{"query":"pipedrive v2 auth","provider":"you"}'and get the identical you.com results the +search arm's web_search tool returns — or simply curl vendor docs. Arm isolation is broken: both arms have web access. Fix: pass an explicit env scrub of TANGLE_API_KEY/ROUTER_* and either disable outbound network (network namespace / unset http_proxy) or
Other
🟠 MEDIUM Default AISDK_TEMPLATE is a hardcoded path in the author's personal /tmp scratchpad — examples/ablation-suite/aisdk-env.ts
TEMPLATEdefaults to/tmp/claude-1000/-home-drew-code-blueprint-agent/8b62a3c4-97a5-46cd-92bf-5a4f3bf95f75/scratchpad/aisdk-probe— an absolute path from another tool's session scratchpad on one machine, in /tmp (wiped on reboot). The prebuilt template's contents (ai@7, zod, typescript, the 'proven tsc invocation') exist nowhere in this repo, so no one else — and no CI — can run the env as committed; ensureTemplate() throws, so it fails closed, but the example is non-reproducible by default. Fix: add a small setup script (package.json + pnpm install + the tsc args) that materializes the template into a repo-relative or XDG-cache path, and default TEMPLATE to that.
🟠 MEDIUM Stream task passes with a bare stub — never requires streamText or the renamed method — examples/ablation-suite/aisdk-env.ts
The stream task's check is
export const _r: Response = handler(). Any stub —export function handler(): Response { return new Response('x') }— type-checks and scores 1 without importing streamText or discovering the renamed Response method (the env has no noUnusedLocals, so even the starter's unused import is fine). aiSdkTasks assigns this task to every other index (i % SDK_TASKS.length), so half the tasks measure nothing: both arms pass them with a v4-agnostic stub, diluting the measured search delta toward 0 and falsifying the header's claim that each task is 'a structurally-required rename ... cannot be completed without the current name'. Fix: require a real streamText usage in the check (e.g. check.ts imports and calls handler then reads a property only present on the SDK's Res
🟠 MEDIUM aisdk-env: symlinked node_modules makes the v7 type definitions readable — no-search arm may hit the ceiling without search — examples/ablation-suite/aisdk-env.ts
symlinkSync(join(TEMPLATE,'node_modules'), join(dir,'node_modules')) plus the read_file tool means the worker can read node_modules/ai/dist/*.d.ts and see the renamed
inputSchemafield directly — solving the task by reading the installed types, with neither recall nor web_search. Unlike the multi-contract grader leak (which spells the answer in English), this still requires reading TS type defs, but a senior-engineer-prompted model will try this immediately. The likely outcome is the no-search arm aces the task, triggering the >=0.6 'weak candidate' branch (run-aisdk.ts:120) and concluding search doesn't help — a possibly-correct but under-powered conclusion. Worth confirming with a no-search pilot; if it aces, hide the .d.ts (only expose the compiler via run_tests).
🟠 MEDIUM bash tool in both arms has unrestricted network — no-search arm can curl vendor docs — examples/ablation-suite/multi-contract-env.ts
The ABLATION DELTA comment claims additive isolation ('only one can also search'), but the
bashtool runsbash -lcwith the host's network in BOTH arms. The no-search worker cancurl https://docs.stripe.com/api-v2-overview(or any vendor doc) and recover every current contract — it has web access in everything but name. The measured delta is then 'web_search tool vs curl', not 'search vs no web', undercutting the ablation's construct validity. Fix: in the no-search arm, drop the bash tool or run it under a network-blocked sandbox/proxy; document whichever regime is enforced.
🟠 MEDIUM 'paired bootstrap CI' is actually a Wald normal-approximation on paired binary diffs — poor coverage at small n — examples/ablation-suite/run-multi-contract.ts
The comment says 'deterministic jackknife-style spread as a conservative CI proxy', but the code computes mean(d) ± 1.96*sqrt(var(d)/n) — a Wald CI on paired 0/1 differences. Wald coverage for paired binary data at n=5-20 (the gate is both.length<5) is notoriously anti-conservative (true coverage well below nominal). No bootstrap resampling is performed. Identical copy in run-aisdk.ts:91. Fix: either an exact paired permutation/McNemar exact interval, or an actual bootstrap (BCa) on the paired differences with a fixed seed.
🟠 MEDIUM Failed-retry cost is silently dropped and hardest tasks are excised — biases both cost and resolve — examples/ablation-suite/run-multi-contract.ts
On empty/error runs the loop retries up to 3x but only
usd += r.usdof the SUCCESSFUL attempt is accumulated; failed attempts' router spend is lost (runAgentic has already paid). Cost is under-reported by up to 3x for flaky tasks. Separately, if all 3 attempts fail the task is pushed as valid:false and filtered out ofboth(line 137) — systematically dropping the hardest tasks from pairing and biasing resolve toward the easy middle. Fix: accumulate cost from every attempt (catch the throw, read .usd if available), and report the invalid rate as a first-class result rather than silently trimming.
🟠 MEDIUM Verdict tests two CIs and wins if EITHER excludes 0 — doubles the false-positive rate — examples/ablation-suite/run-multi-contract.ts
ci.lo > 0 || sci.lo > 0 ? 'SEARCH HELPS — CI excludes 0.' : .... With two independent 95% CIs checked disjunctively, the family-wise false-positive rate is ~1-(0.95)^2 ≈ 9.8%, not 5% — and it is not the pre-registered single primary outcome. run-aisdk.ts:151 correctly tests ONE CI. Fix: pre-register a single primary (resolve delta), report score delta as secondary, apply a Bonferroni/Holm correction if both are load-bearing.
🟡 LOW read_file/write_file path traversal (no workspace containment check) — examples/ablation-suite/aisdk-env.ts
join(dir, String(args.path))with no resolve+startsWith(dir) guard lets../..escape the workspace in both new envs. In multi-contract-env.ts:203/209 this adds nothing (bash already grants full FS), but aisdk-env has no bash tool, so this is the only FS escape — a worker can read/write files outside its tmp workspace with the runner's privileges (this is also the channel for the check.ts overwrite in finding #2). Low severity for a local eval harness. Fix:const p = resolve(dir, String(args.path)); if (!p.startsWith(dir + sep)) return 'path escapes workspace'.
🟡 LOW Fixture signatures polluted with literal \n\n and a trailing backtick (all 19 entries) — examples/ablation-suite/extract-contracts.mjs
The regex
def fn\([^\n)]*\)[^\n]*captures across the TS sources' escaped\nsequences and the closing backtick of the surrounding string/template literal. Verified: every entry in contracts.all.json (19/19) and contracts.json (10/10) ends with\\n\\n\`` (e.g."def is_feature_enabled(...) -> bool\n\n`"). multi-contract-env.ts:258 interpolates${c.signature}inside a code span in the worker prompt, so every contract line renders with a literal\n\nand a stray backtick breaking the markdown. Models will usually recover, but it's systematic prompt pollution in a measurement instrument. Fix: strip the capture at the return annotation ([^\`\\n]*then trim trailing\n`/backticks) and regenerate the fixtures.
🟡 LOW extract-contracts.mjs hardcodes a machine-specific absolute path — cannot regenerate anywhere else — examples/ablation-suite/extract-contracts.mjs
const VB = '/home/drew/code/blueprint-agent/scripts/experiments/scenarios/verticals/web-grounded'. The script is regen-only (output is committed) so this doesn't affect runtime, but anyone else trying to refresh contracts.all.json gets a silent ENOENT on readdirSync(VB). Fix: make VB an env var (VB_ROOT) with a clear error if unset, mirroring AISDK_TEMPLATE's pattern.
🟡 LOW extract-contracts.mjs hardcodes a path into a separate private checkout — examples/ablation-suite/extract-contracts.mjs
const VB = '/home/drew/code/blueprint-agent/scripts/experiments/scenarios/verticals/web-grounded'— an absolute path to a different repo on the author's machine, so the generator cannot run anywhere else (its output is committed, which limits the blast radius, but the fixtures are then unregenerable/unauditable by anyone else). Fix: take the VB dir from argv or an env var and fail with a usage message.
🟡 LOW Committed signatures carry a stray trailing backtick from the extract regex — examples/ablation-suite/fixtures/multi-contract/contracts.json
Every signature looks like 'def get_directory(base_url: str, access_token: str) -> list[dict]\n\n
' — the trailing backtick comes from extract-contracts.mjs:37new RegExp('def ' + fn + '\([^\\n)]\)[^\\n]')` capturing the markdown backtick that wraps the seed in the source .ts file. These are fed verbatim to the worker in multiContractTasks' function list (multi-contract-env.ts:248), so the worker sees a stray backtick on every signature. Cosmetic but noisy. Fix: strip trailing non-alphanumeric noise when building the JSON, or anchor the regex.
🟡 LOW dirsByHandle is dead state and its size-based id collides after closes — examples/ablation-suite/multi-contract-env.ts
The module-level
dirsByHandlemap is set in open() and deleted in close() but never read anywhere (dir travels via handle.ctx). The handle id embedsdirsByHandle.size, which is not monotonic: open A → id …:0, open B → id …:1, close A (size 1), open C → id …:1 duplicates B's id, and B's later close deletes C's entry. Harmless today because the map is unused, but it's dead code plus a latent id-collision if anything ever consumes handle ids or the map. Fix: drop the map, or use a monotonic counter for the id suffix.
🟡 LOW youSearch has no fetch timeout — a slow router stalls the worker's whole turn — examples/ablation-suite/multi-contract-env.ts
fetch('https://router.tangle.tools/v1/search', {...}) with no AbortController/timeout and no res.ok check. A hung router connection blocks until the TCP/HTTP default drops (minutes), eating the worker's refine budget while the surface appears hung. The bash tool has a 60s timeout; web_search does not. Identical code in aisdk-env.ts:128. Fix: AbortSignal.timeout(15_000) and throw on !res.ok.
🟡 LOW 'Paired bootstrap' CI is neither bootstrap nor jackknife — it's a plain z-interval — examples/ablation-suite/run-multi-contract.ts
The header (line 11) and comment advertise a 'paired bootstrap 95% CI' / 'deterministic jackknife-style spread', but pairedDeltaCI computes mean ± 1.96·SE on the paired deltas — a normal approximation with no resampling (the comment's premise that 'no random seed is available in this env' is wrong; Math.random exists and can be seeded). On binary resolve outcomes at n≥20 the normal approx is coarse, and the mislabel overstates the statistical rigor this suite advertises (the pre-registered text in multi-contract-env.ts:16 also promises 'paired bootstrap'). Same mislabel in run-aisdk.ts. Fix: implement an actual seeded paired bootstrap or relabel
tangletools · 2026-07-15T20:27:45Z · trace
tangletools
left a comment
There was a problem hiding this comment.
❌ 4 Blocking Findings — 72bbfec3
Full multi-shot audit completed 3/3 planned shots over 11 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 11 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-07-15T20:27:45Z · immutable trace
The aisdk/multi-contract search-ablation rigs are unrelated to this PR's titled change (failure-distiller evidence widths) and drew blocking review findings of their own (grader files readable from the worker workspace, agent-writable check.ts at score time, bash inheriting TANGLE_API_KEY in the no-search arm). They live on feat/web-search-ablation-rigs now, where those findings can be addressed as their own change; this PR carries exactly the evidence-width contribution.
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 01eebf98
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.
tangletools · auto-approval · reason: drewstone_author · 2026-07-15T20:54:36Z
|
Re the 4 blocking findings on This PR now diffs as exactly its titled contribution: Verification at |
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 1 (1 low) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 58.4s (2 bridge agents) |
| Total | 58.4s |
💰 Value — sound
Widens the default failure distiller's evidence caps (notes 400→1500, errors 200→500) as named, documented constants so tracebacks reach the proposer; minimal, in-grain, no existing equivalent short-circuited — ship.
- What it does: In src/improvement/improve.ts:268-322, the default
generationFailureDistillernow clips joined judge notes at DISTILLED_NOTES_MAX_CHARS=1500 (was a magic 400) and cell error strings / the error-derivedclaimfallback at DISTILLED_ERROR_MAX_CHARS=500 (was 200). It also keeps theclaimfield that arrived from main's merge (claim = notes, orScenario X failed: <error>when no notes). Doc comm - Goals it achieves: The self-improvement loop's default path (ACE-style distilled findings) was trace-blind: a real traceback or failing-assertion note from an executable judge is typically >400 chars, so clipping at 400/200 meant the prompt proposer never saw the actual failure evidence and had to guess. After this, realistic tracebacks (~1-1.5KB) survive whole to the proposer, making reflection-driven proposals evi
- Assessment: Good on its merits. The change is three lines of behavior plus constants, docs, and a pinning test — proportional to the goal. Named constants with rationale comments match the repo's documented-invariants style; the test asserts exact cap behavior rather than vibes. Verified the wider values propagate correctly:
memoryGenerationDistiller(improve.ts:335) delegates to `generationFailureDistiller - Better / existing approach: Checked for an existing mechanism that already does this:
rawTraceDistiller(raw-trace-distiller.ts:74+) is the no-summarize alternative, but it requires a durable runDir and agentic grep/cat diagnosis — a heavier mode, not a substitute for the default lightweight digest, so keeping the default distiller but widening its caps is the right complement rather than forcing everyone onto raw-trace. S - Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound
Widens the default failure-distiller's notes/error caps (400→1500, 200→500) into named constants so real tracebacks survive to the proposer; a minimal, in-grain fix on a hot default path.
- Integration: Fully reachable: generationFailureDistiller (improve.ts:281) is the default analyzeGeneration selected at improve.ts:623 when no custom analyzer/rawTraceContext is set, and is reused transitively by memoryGenerationDistiller (improve.ts:338). Both new constants are consumed at improve.ts:313,316,322 — no dead surface. The companion doc in raw-trace-distiller.ts:6,14 was updated to cite the new ~15
- Fit with existing patterns: Fits the established pattern exactly. The default distiller is documented as deliberately dependency-free with hardcoded reasonable bounds (improve.ts:275-280); callers wanting control already pass a custom analyzeGeneration or use rawTraceDistiller. Extracting magic numbers into named, commented constants and widening them is precisely the grain. The separate bench analyzer (trata-gepa.mts:302) s
- Real-world viability: Trivially robust: it is purely widening string slices, so there are no concurrency, ordering, or edge-input concerns. The post-distill CAP of 12 findings (improve.ts:328) still bounds total proposer context at worst ~12×1500≈18k chars, well within budget. The claim-fallback path (notes OR error-derived) stays coherent and at the wider bound. No regression surface introduced.
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🔎 Heuristic Signals
🟡 Cruft: magic number added src/improvement/improve.test.ts
- // A realistic executable-judge note (~1000 chars) must survive whole; a
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.
Part of the GEPA trace-evidence fix (companion: tangle-network/agent-eval#373). The default generation-failure distiller clipped judge notes to 400 chars and errors to 200 — a real traceback/failing assertion never survived to the prompt proposer, leaving reflection trace-blind. Widened to 1500/500 and merged with main's
claimfield (kept at the wider bound).tsc clean; improvement suite 33/33.
🤖 Generated with Claude Code