Skip to content

Phase 5 core: contained sessions + single-benchmark climb (tsp first) - #29

Merged
renmengye merged 3 commits into
mainfrom
feat/phase5-climb
Aug 6, 2026
Merged

renmengye merged 3 commits into
mainfrom
feat/phase5-climb

Conversation

@renmengye

Copy link
Copy Markdown
Member

The first real climb machinery, scoped to ONE benchmark per Mengye's direction.

  • Session containment (decided): container_image on the harness wraps sessions in apptainer exec --containall --cleanenv — the session sees only the workspace, its per-run HOME (via --home, since apptainer silently refuses --env HOME — review-agent catch, verified against apptainer source), and the read-only claude binary. Key travels via APPTAINERENV_, never argv.
  • orchestrator.climb_once: baseline measured from the pre-session tree, session, scope check before candidate eval (an out-of-scope edit could be to the ruler itself — the doctored tree is never measured), candidate re-measured by the orchestrator, direction-aware threshold with finiteness guards. The agent's claim is never trusted.
  • Contained evaluator: the benchmark command executes agent-written code, so it gets session-grade containment (same apptainer wrapper); uncontained mode never sees the real HOME (the PAT-holding account) and process-group-kills its orphans on timeout.
  • Metric parsing is JSON-only — the fuzzy regex fallback read progress lines and prefixed metric names (review catch, demonstrated live) and is gone.

Pre-merge adversarial pass: 8 confirmed findings, all fixed with regression tests. The top three were live-run blockers: uncontained candidate eval = agent-code execution on the PAT account; --env HOME silently ignored = resume state on an evaporating tmpfs; no scope enforcement before eval = metric fraud via a doctored eval harness. 241 tests.

🤖 Generated with Claude Code

renmengye and others added 2 commits August 6, 2026 11:00
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pe-before-eval, finite metrics

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 6, 2026 •

Copy link
Copy Markdown

Advisory review — not an approval. Automated findings from autoresearch; a human code owner still owns this PR. Reply to any finding you disagree with, or add the opt-out label to silence future runs on this PR.

  • Baseline eval runs inside the git clone, so its artifacts can be misattributed to the agent (spurious scope-violation or eval junk in the PR) (medium confidence, src/autoresearch/orchestrator.py:93)
    SubprocessEvaluator.evaluate runs the benchmark with cwd=workspace, and climb_once calls it once on the pre-session tree. Any file the eval command writes into the working tree (result JSON, __pycache__, .pytest_cache, .venv, per-run output dirs) is present before the session and will be reported by changed_paths — which the docstring says the caller wires to git add -A + staged paths. Consequences: (a) a clean session can be rejected as scope-violation for paths the orchestrator itself created, and (b) if the target repo does not gitignore those artifacts they end up in the pushed commit. The code goes out of its way to move HOME out of the clone for exactly this reason ("eval cache/state artifacts must not masquerade as agent edits in the diff") but leaves CWD writes unaddressed. Note also that the candidate eval runs after the scope check, so anything it writes into the workspace is never scope-checked.
  • Contained evaluator binds a possibly relative workspace path; harness guards against exactly this (medium confidence, src/autoresearch/orchestrator.py:68)
    In the container branch the evaluator builds --bind f"{workspace}:{workspace}" and --pwd str(workspace) from the caller-supplied workspace without resolving it, while eval_home a few lines later uses workspace.resolve().parent. The harness explicitly resolves the workspace before building the bind (workspace = workspace.resolve()) and even returns config-error for a relative bind source, with the comment "bind sources must be absolute or apptainer fails at mount time". A relative or symlinked workspace passed to the evaluator therefore fails deep inside apptainer instead of being handled, and --pwd may point at a path that does not exist inside the container. Resolving once at the top of evaluate would make both paths consistent.
  • eval_home.mkdir OSError escapes as an unhandled exception instead of an eval-error outcome (medium confidence, src/autoresearch/orchestrator.py:88)
    eval_home.mkdir(parents=True, exist_ok=True) is outside any try/except, so a PermissionError/OSError (read-only parent, quota, stale NFS handle on the shared filesystem) propagates out of evaluate as a bare OSError. climb_once only catches EvalError, so this crashes the whole climb rather than returning outcome="eval-error" — inconsistent with the surrounding error discipline (OSError from Popen is already wrapped in EvalError). Same applies to the container_image path, where the directory is created but never used inside the container.
  • eval HOME is shared between baseline and candidate runs and never cleaned up (low confidence, src/autoresearch/orchestrator.py:87)
    eval_home is derived only from the workspace name and created with exist_ok=True, so the baseline eval and the candidate eval share the same HOME (and so do any later runs reusing the same workspace name). If the benchmark caches anything under $HOME (uv/pip caches are the benign case; a memoized result file is the harmful one), the candidate measurement can be served from state written during the baseline run, weakening the "orchestrator re-measures independently" guarantee. A fresh per-measurement directory (or explicit cleanup) would remove the coupling; nothing in the diff removes the directory either, so it accumulates alongside every workspace.
  • "last JSON line wins" metric parsing is shadowable by in-scope agent code (low confidence, src/autoresearch/orchestrator.py:126)
    _metric_from_output takes the metric from the LAST single-line JSON object on stdout. The eval command imports agent-modified, in-scope code (e.g. src/pilot/solvers/), so that code can emit a trailing metric line (an atexit handler prints after the eval's own summary) and win the parse without touching any out-of-scope path — the scope check cannot catch it, and CI re-running the same command reproduces the same number. The harness's _parse_result deliberately prefers the FIRST candidate object for the mirror-image reason ("a trailing look-alike substituting its fields"); the two modules take opposite stances. A stricter contract (metric only from a designated file/last-line-of-a-fenced marker, or eval run from a pinned tree outside agent scope) would close this; at minimum the asymmetry deserves a comment.

Apptainer flag semantics (--home src:dest under --containall, APPTAINERENV_ injection with --cleanenv, auto-creation of the /opt/agent bind point in a read-only SIF) cannot be verified from the provided context; the tests only exercise a fake apptainer binary, so they would pass even if the real flag behavior differed. I did not flag the harness/eval containment design itself, only the inconsistencies visible in the diff.

…ed drain, absolute binds

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@renmengye
renmengye merged commit c196574 into main Aug 6, 2026
6 checks passed
@renmengye
renmengye deleted the feat/phase5-climb branch August 6, 2026 15:26
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.

1 participant