Skip to content

fix(bench): honor absolute Terminal-Bench venv paths#619

Merged
drewstone merged 2 commits into
mainfrom
fix/terminal-bench-absolute-venv-475
Jul 25, 2026
Merged

fix(bench): honor absolute Terminal-Bench venv paths#619
drewstone merged 2 commits into
mainfrom
fix/terminal-bench-absolute-venv-475

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Summary

  • resolve isolated virtual environments through one shared package-relative-or-absolute path helper
  • use the same resolved environment for Terminal-Bench Python and CLI execution
  • prove the installed package can load and judge through an external absolute environment

Checks

  • reproduced the installed-package ENOENT before the fix
  • pnpm --dir bench exec tsx --test src/benchmarks/terminal-bench.test.mts (2/2)
  • pnpm --dir bench typecheck:public
  • pnpm --dir bench verify:package:local-runtime
  • node --check bench/scripts/verify-packed-consumer.mjs
  • git diff --check
  • git merge-tree --write-tree origin/main HEAD

Closes #475

@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 — afac0aca

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-25T00:32:16Z

@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

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 70.4s (2 bridge agents)
Total 70.4s

💰 Value — sound

Makes the named-venv path resolver honor absolute TERMINAL_BENCH_VENV paths via one shared helper, fixing installed-package consumers and matching the existing resolveBenchPython pattern — no better approach or duplication found.

  • What it does: Before, venvPythonAt did join(benchRoot, venvDir, 'bin', 'python') and terminal-bench's tb path was a separate join(benchRoot, venvDir, 'bin', 'tb') — both unconditionally anchored to the package root, so an absolute TERMINAL_BENCH_VENV got concatenated under benchRoot and ENOENT'd for an installed consumer. After, both go through a new shared venvBinAt(venvDir, name) = join(resolve(benchRoot, v
  • Goals it achieves: (1) Let an installed @tangle-network/agent-bench consumer point Terminal-Bench's heavy venv at an arbitrary absolute location instead of being forced to nest it under the package. (2) Unify the python and tb resolution through one helper so they can't drift again (the old code had tb hand-rolled with a raw join, bypassing venvPythonAt). (3) Prove the fix survives the pack→install→consume path,
  • Assessment: Good change, in the grain of the codebase. _harness.ts already had the exact precedent: resolveBenchPython (line 67-77) treats AGENT_BENCH_PYTHON as absolute-required, and the named-venv concept (venvPythonAt, used by commit0.ts:36 and terminal-bench.ts:37) already existed for relative isolated venvs. Extending the same helper to honor absolute paths is the minimal, consistent move — and because t
  • Better / existing approach: none — this is the right approach. Searched: rg for venvPythonAt/venvBinAt/venvBin/TERMINAL_BENCH_VENV across bench/src and bench/scripts (only _harness.ts defines them; commit0.ts and terminal-bench.ts consume). resolveBenchPython is the existing absolute-path precedent for the SHARED venv only — it does not cover named venvs, so there is no reusable equivalent being reinvented. venvBin(name) at
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

Routes Terminal-Bench python+cli resolution through one shared path.resolve-based helper so absolute TERMINAL_BENCH_VENV overrides are honored for installed consumers, fixing the installed-package ENOENT.

  • Integration: Fully reachable and exercised. terminal-bench.ts is a published subpath export (@tangle-network/agent-bench/benchmarks/terminal-bench). The new terminal-bench-absolute-venv.mjs consumer in verify-packed-consumer.mjs:169-182 drives createTerminalBenchAdapter().loadTasks + .judge through an absolute external venv end-to-end (terminalBenchPython at terminal-bench.ts:37 feeds loadRows' runVenvPython;
  • Fit with existing patterns: Matches the established pattern exactly. resolveBenchPython (_harness.ts:67-77) already supports an absolute override for the SHARED .venv via AGENT_BENCH_PYTHON (with an isAbsolute check); this PR extends the same idea to NAMED isolated venvs via TERMINAL_BENCH_VENV / COMMIT0_VENV, using path.resolve consistently. No competing mechanism exists. Crucially, it unifies python and tb resolution onto
  • Real-world viability: Handles the three input shapes correctly: relative venvDir (resolve(benchRoot, rel) preserves source-tree behavior), absolute venvDir (resolve returns it as-is — the fix), and the default '.venv-terminal-bench' (unchanged). Empty-string edge case resolves to benchRoot/bin/python, but that input is opt-in via env var so not realistic. The fake python/tb stubs written into the absolute venv (verify-
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅


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 · 20260725T004011Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — afac0aca

Review health 100/100 · Reviewer score 86/100 · Confidence 70/100 · 3 findings (3 low)

glm: Correctness 86 · Security 86 · Testing 86 · Architecture 86

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

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

🟡 LOW Mock tb uses ESM import in extensionless file, requires Node 22.7+ module detection — bench/scripts/verify-packed-consumer.mjs

The mock tb script uses import { mkdirSync, writeFileSync } from 'node:fs' in an extensionless file with no package.json ancestor. This relies on Node's automatic module syntax detection (default since Node 22.7.0, still flagged on Node 20.x). CI uses Node 22 (latest, which is 22.7+) so it passes, but the repo's engines field declares >=20.19.0 — a developer on Node 20 would get SyntaxError: Cannot use import statement outside a module. Fix: either use require('node:fs') (detect-module would then classify the file as CJS, working on all Node versions), or write a package.json with {"type":"module"} into terminalBenchBinDir. The mock python ([line 65](https://github.com/tangle-network/agent-runtime/blob/afac0acaafe34202d03fd5803330c5d2f4c38d86/bench/scripts/verify-packed-consu

🟡 LOW venvBin vs venvBinAt use different path primitives without a stated reason — bench/src/benchmarks/_harness.ts

venvBin (shared .venv, line 96-99) still uses join(benchRoot,'.venv','bin',name) while the new venvBinAt (line 89-91) uses resolve. Both are correct for their callers today (shared .venv is always package-owned and relative), but a future reader may copy the join variant for a named/overridable venv and reintroduce the absolute-path bug. Consider having venvBin delegate to venvBinAt('.venv', name) so there is a single resolution path, or add a one-line comment on venvBin noting it is intentionally join-only because the

🟡 LOW venvBinAt has no dedicated unit test — bench/src/benchmarks/_harness.ts

The new exported helper venvBinAt is covered only by the heavier verify-packed-consumer.mjs integration script, not by a fast unit test in _harness.test.mts. A one-line assertion (venvBinAt('/abs/venv','tb') === '/abs/venv/bin/tb' and venvBinAt('.venv-x','python') === join(benchRoot,'.venv-x','bin','python')) would lock in the join-vs-resolve contract cheaply and catch regressions without spinning a packed-consumer scratch env.


tangletools · 2026-07-25T00:46:48Z · trace

tangletools
tangletools previously approved these changes Jul 25, 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.

✅ Approved — 3 non-blocking findings — afac0aca

Full multi-shot audit completed 2/2 planned shots over 3 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-25T00:46:48Z · immutable 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.

✅ Auto-approved drewstone PR — 8c3279e8

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-25T01:08:09Z

@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

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 83.7s (2 bridge agents)
Total 83.7s

💰 Value — sound

Lets installed consumers point Terminal-Bench (and commit0) at an external absolute venv instead of ENOENT-ing under the package root, by routing all named-venv executable resolution through one shared helper.

  • What it does: Introduces venvBinAt(venvDir, name) in _harness.ts:89 that does join(resolve(benchRoot, venvDir), 'bin', name) — so an ABSOLUTE venvDir resolves to itself (external env) while a relative one stays package-owned. Refactors venvPythonAt and venvBin to delegate to it, and rewrites terminal-bench.ts:38 (terminalBenchBin) to call venvBinAt instead of its own inline join(benchRoot, venvDir, 'bin', 'tb')
  • Goals it achieves: (1) Fix a real bug: when @tangle-network/agent-bench is npm-installed (not run from source) there is no .venv-terminal-bench inside node_modules, so a consumer setting TERMINAL_BENCH_VENV=/srv/terminal-bench got benchRoot+/srv/terminal-bench/bin/python → ENOENT. (2) Remove the duplicated join(benchRoot, venvDir, 'bin', ...) that terminal-bench.ts had reimplemented inline alongside the harness. Net
  • Assessment: Good change, in the grain of the codebase. The harness file (_harness.ts:1-17) explicitly exists to centralize the venv/process/report plumbing so adapters don't copy-paste path logic, and this PR removes exactly such a copy-paste (the inline terminalBenchBin join). It extends an already-established pattern: resolveBenchPython already accepted an absolute override for the shared interpreter, so gi
  • Better / existing approach: none — this is the right approach. Checked _harness.ts for an existing absolute-aware resolver: resolveBenchPython (lines 67-77) handles it for the SHARED interpreter only, and deliberately THROWS on relative paths because it has no package-relative default; the named-venv helpers legitimately need a package-relative default (.venv-terminal-bench / .venv-commit0), so silently treating relative-as-
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

Fixes a real ENOENT for installed consumers by routing venv executable resolution through path.resolve so absolute TERMINAL_BENCH_VENV paths are honored, consolidating the python and tb CLI path computations into one shared helper.

  • Integration: Directly on the live execution paths. venvBinAt is consumed by terminalBenchBin() (terminal-bench.ts:38) which feeds judge() (line 166), and venvPythonAt (now delegating to venvBinAt) feeds loadTasks/loadRows (line 104) and preflight (line 117). commit0.ts:36 also benefits via venvPythonAt as a free side-effect. The shared venvBin('.venv',...) path used by swebench/others is preserved (resolve(ben
  • Fit with existing patterns: Matches the codebase grain precisely. _harness.ts header (lines 1-17) declares itself the ONE place for venv/path/Docker plumbing, and this change removes the duplication where terminal-bench computed its tb path via inline join while venvPythonAt used a separate join — now both route through the single venvBinAt using resolve. No competing or alternative path-resolution pattern exists; resolve is
  • Real-world viability: resolve(benchRoot, venvDir) correctly handles both cases: relative stays package-owned (benchRoot/.venv-x/bin/x, verified identical to old join output), absolute roots at the given path. The happy path for installed consumers (absolute external venv) is exactly the case that was broken and is now fixed and CI-proven. Minor non-material wrinkle: the preflight diagnostic string (terminal-bench.ts:11
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅


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 · 20260725T013057Z

@drewstone
drewstone merged commit 7047f3c into main Jul 25, 2026
3 checks passed
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.

fix(agent-bench): support absolute TERMINAL_BENCH_VENV paths

2 participants