fix(eval): make execution boundaries authoritative - #2928
Conversation
Keep normal Harbor and Pier completion under the framework task timeout while representing cancellation, used-trial abort, and unused-trial termination as explicit bounded phases. Allow relay settlement before TERM/KILL escalation and persist structured cleanup evidence so indeterminate outcomes remain attributable. Validated with focused fake-clock lifecycle tests for the removed 20-second completion deadline, preparation cancellation, and used-trial abort. Generated-by: Codex
Use the shared Harbor/Pier relay as the single boundary for task working-directory discovery, subject process settlement, transport delivery, and bounded infrastructure evidence. Remove executor containerCwd authority and workspace capture files, prefer observed terminal execution over simultaneous cancellation, and make peer-close handling idempotent without persisting raw stderr or credentials. Forward the benchmark-specific 120-second settlement budget through the existing Eval shim to the Runtime Host client without changing Runtime Host authority or polling. Replace remaining provider cwd literals on this path with the relay-confirmed task cwd. Validated with focused Node lifecycle, external subject, and provider-admission tests; Python relay contract and portable transport/race tests; @maka/eval typecheck; Biome; and git diff checks. Generated-by: Codex
Adapt the shared Harbor/Pier relay to their merged process stream with an exact-one bounded result frame, while retaining only structured diagnostics and removing raw external process logs. Make OS-signal teardown single-owner and deadline-bounded, preserve Harbor timeout and observed terminal facts, and clean staging/control artifacts idempotently. Restrict structured external results to the canonical bundled wrapper, require the explicit Maka host settlement budget in both benchmark specs, and remove the superseded bounded-log path. Validated with @maka/eval typecheck/build, 20 focused Node tests, 17 Python relay tests (2 GNU setsid cases skipped on macOS), Biome, and git diff --check. Generated-by: Codex
Refuse to start a subject when the relay cannot publish its process-group control file. Distinguish setup failure from subject exit, require valid scope evidence before verification, and retain only structured failure evidence. Validated with focused command, full relay lifecycle, external subject, typecheck, build, and formatting checks. Generated-by: Codex
d7a404a to
02876b3
Compare
|
I exercised this PR’s problem space in a live Terminal-Bench 2.1 run and observed several concrete failures on the pre-#2928 Eval relay implementation:
I excluded all affected cells from scoring and retried only infra-failed or indeterminate cells. The repeated failures show that retries alone do not make the pre-#2928 execution boundary reliable. AI assistance disclosure: Codex helped diagnose the failures, prepare the local EPIPE fix and regression test, and draft this report. I verified the referenced attempt artifacts and failure traces. |
M4n5ter
left a comment
There was a problem hiding this comment.
English review
The direction of this PR is sound and directly addresses failures observed in real benchmark runs. I found two issues that remain tightly coupled to the PR's stated goals of race-safe cancellation, trustworthy teardown evidence, and reliable bounded result transport.
1. Cancellation and teardown cleanliness are modeled as the same fact
Locations:
packages/eval/src/harness-executor.ts:117-202packages/eval/src/harness-executor.ts:783-826packages/eval/src/runner.ts:283-305
When the host aborts while verification is pending, onAbort starts teardown but does not record cancellation as a semantic attempt outcome. If the supervisor exits after TERM, clean becomes true, verification continues, and the attempt may be persisted as infra_failed or even completed rather than indeterminate.
I reproduced the latter case end to end: after AbortSignal fired during verification, the attempt was stored as completed with score: 1 and became non-replaceable.
The same state conflation affects escalation: trialExitEvidence() marks every non-completion exit as clean, including outcome: "killed". A SIGKILL only proves that the Python supervisor exited. It does not prove that Harbor completed environment.stop(delete=true) or removed the container and staged control/credential files.
This conflicts directly with the PR's central contract: host cancellation must remain distinguishable, and teardown evidence must describe what was actually confirmed.
Suggested direction:
- Model normal completion, cancellation source, and cleanup outcome as separate facts.
- If host cancellation occurs before verification has reached a confirmed terminal result, return an
indeterminateattempt even when resource cleanup succeeds. - Do not treat SIGKILL as clean unless a host-owned cleanup path independently confirms resource removal.
- Add regressions for cancellation during verification and TERM timeout followed by SIGKILL.
2. The bounded stream classifier drops valid large terminal records
Location:
packages/eval/src/harbor-external-subject.ts:422-450
classifyStream() clears buffered once an unterminated line exceeds 1 MiB. The remainder of that line is then not valid JSON, so its terminal marker is lost.
This affects a frozen, supported path rather than only malformed output. Pi's JSON-mode agent_end record contains the complete messages[] payload. I reproduced the issue with the PR's built wrapper:
- provider admission succeeded;
- the child exited with code 0;
- it emitted a valid 2,097,220-byte
agent_endrecord; - the wrapper returned
status: "failed"withpi execution failed.
The previous implementation accepted up to 16 MiB of trajectory output, so this is a regression introduced by the new bounded classifier.
Suggested direction:
- Incrementally extract only the bounded top-level terminal fields required by each profile while continuing to hash/count the complete stream.
- If an oversized record cannot be classified safely, report an explicit infrastructure diagnostic instead of silently turning it into a subject failure.
- Add an oversized
agent_endregression test.
Validation
I independently ran:
- Eval build and typecheck — passed
- Node Eval tests — 23 passed
- Python relay tests — 21 passed
- Biome and
git diff --check— passed - Focused reproductions for both issues above — reproduced
I intentionally excluded concerns outside this PR's stated threat model and normal benchmark paths.
简体中文审查
这个 PR 的方向是正确的,也确实针对真实 benchmark 中观察到的故障进行了修复。不过,我确认了两个仍然直接影响 PR 核心目标的问题:取消竞态安全、teardown 证据可信,以及有界结果传输的可靠性。
1. 取消语义与资源清理成功被建模成了同一个事实
位置:
packages/eval/src/harness-executor.ts:117-202packages/eval/src/harness-executor.ts:783-826packages/eval/src/runner.ts:283-305
当 host 在 verifier 等待期间发出取消时,onAbort 会启动 teardown,但不会把“attempt 已被取消”记录成独立的语义事实。如果 supervisor 在 TERM 后退出,clean 会变成 true,verification 继续执行,最终 attempt 可能被保存为 infra_failed,甚至 completed,而不是 indeterminate。
我端到端复现了后一种情况:在 verification 期间触发 AbortSignal 后,attempt 仍被保存为 completed、score: 1,并成为不可替换结果。
同一状态混淆也影响升级路径:trialExitEvidence() 会把所有非 completion 阶段的退出都标记为 clean,包括 outcome: "killed"。SIGKILL 只能证明 Python supervisor 已退出,不能证明 Harbor 已经完成 environment.stop(delete=true),也不能证明容器及暂存的控制文件和凭据文件已经删除。
这直接违反了 PR 的核心契约:host cancellation 必须保持可区分,teardown evidence 必须只描述已经确认的事实。
建议修复方向:
- 分别建模正常完成、取消来源和 cleanup outcome。
- 如果 host cancellation 发生时 verification 尚未确认终态,即使资源清理成功,也应返回
indeterminateattempt。 - 除非 host-owned cleanup 路径能够独立确认资源已删除,否则 SIGKILL 不能被视为 clean。
- 增加 verification 期间取消,以及 TERM 超时后升级到 SIGKILL 的回归测试。
2. 有界流分类器会丢弃合法的大型终态记录
位置:
packages/eval/src/harbor-external-subject.ts:422-450
当一个尚未结束的单行超过 1 MiB 时,classifyStream() 会直接清空 buffered。之后只剩该行的尾部,无法再解析成合法 JSON,终态 marker 因此丢失。
这会影响已经冻结并明确支持的正常路径,而不只是异常输出。Pi JSON mode 的 agent_end 记录包含完整的 messages[]。我使用本 PR 构建出的 wrapper 复现了问题:
- provider admission 成功;
- child exit code 为 0;
- 输出合法的 2,097,220-byte
agent_end; - wrapper 最终返回
status: "failed"和pi execution failed。
旧实现允许最高 16 MiB trajectory,因此这是新有界分类器引入的回归。
建议修复方向:
- 在继续对完整流进行 hash 和 byte count 的同时,增量提取各 profile 所需的、有界的顶层终态字段。
- 如果 oversized record 无法安全分类,应报告明确的 infrastructure diagnostic,而不能静默转成 subject failure。
- 增加 oversized
agent_end回归测试。
验证
我独立执行了:
- Eval build 和 typecheck:通过
- Node Eval tests:23 项通过
- Python relay tests:21 项通过
- Biome 和
git diff --check:通过 - 上述两个问题的针对性复现:均已复现
我有意排除了不属于本 PR 威胁模型以及正常 benchmark 路径的问题。
AI disclosure / AI 披露: This review was prepared with assistance from OpenAI Codex, manually reviewed by me, and I take responsibility for its conclusions. / 本次审查由 OpenAI Codex 辅助完成,已经我人工复核,其结论由我负责。
Track host cancellation, verified terminal state, and finalization evidence as independent facts. Attempts cancelled before verification settles now remain replaceable even when TERM cleanup succeeds, while SIGKILL records only supervisor termination rather than confirmed Harbor cleanup. Validated with focused verifier-cancellation and TERM-to-KILL lifecycle tests. Generated-by: Codex
Restore the frozen 16 MiB JSONL record contract without persisting raw output. Valid large Pi terminal records remain classifiable, while records beyond the bound continue to be hashed and counted and produce explicit infrastructure evidence instead of a subject failure. Validated with 2 MiB Pi settlement and 17 MiB oversized-record wrapper tests. Generated-by: Codex
|
Addressed both findings in
The wrapper now preserves the previous 16 MiB per-record contract while continuing to hash and count the complete stream without persisting raw stdout or stderr.
I did not introduce a general incremental JSON parser because the existing frozen contract already defines a bounded 16 MiB record. Keeping that explicit bound is smaller and avoids a second provider-specific parsing path. Focused validation:
Could you please re-review the latest head, AI assistance disclosure: Codex materially authored the fixes, tests, commit messages, and this response. I reviewed the resulting diff and focused validation. 简体中文对照已在
Wrapper 恢复了此前每条 record 最高 16 MiB 的合同,同时继续对完整 stream 计算 hash 和 byte count,并且不持久化 raw stdout 或 stderr。
我没有引入通用增量 JSON parser,因为当前冻结合同已经定义了有界的 16 MiB record。保留这一明确上限更简单,也避免建立第二套 provider-specific parsing path。 聚焦验证:
请重新审查最新 head AI 协助披露:Codex 实质性参与了修复、测试、commit message 和本回复的编写。我已复核最终 diff 和聚焦验证结果。 |
M4n5ter
left a comment
There was a problem hiding this comment.
English follow-up review
Thanks for addressing the original findings. The large terminal-record issue is resolved, and cancellation during verification now correctly produces an indeterminate attempt. One production lifecycle contract gap remains.
Production teardown cannot currently produce the new confirmed outcome
Locations:
packages/eval/src/harness-executor.ts:827-857packages/eval/harbor/run_trial.py:141-165packages/eval/src/runner.ts:319-325
waitForTrial() now treats a supervisor exit after TERM as confirmed only when it exits with code 0. However, the production supervisor does not currently have that exit behavior.
On SIGTERM, run_trial.py cancels its main task. Harbor's Trial.run() performs _finalize() in finally and then re-raises CancelledError. Consequently, even when teardown completes successfully, the uncaught cancellation causes the Python supervisor to exit with code 1.
I reproduced this through the real run_trial.main() signal path with a run_trial() stub that records completion from its finally block:
teardown-finally-ran
cleaned=True
exit_code=1
This affects a normal supported path:
subject returns infra_failed
→ runner skips verification
→ executor sends TERM during finalization
→ Harbor completes teardown, but the supervisor exits 1
→ cleanup is classified as terminated rather than confirmed
→ the original infra_failed result becomes indeterminate
The new lifecycle tests do not cover this production behavior because their fake Node supervisors explicitly call process.exit(0) from the SIGTERM handler.
This is directly within the PR's goal of making teardown evidence authoritative, so I think it should be resolved here rather than deferred.
Suggested direction:
- Give host-requested teardown an explicit success outcome after
run_trial()has unwound and finalized successfully. - Preserve a nonzero outcome when teardown itself fails; do not generally treat every TERM exit 1 as clean.
- Exercise the real
run_trial.pyTERM path in a regression test. - Verify that a subject
infra_failedresult remainsinfra_failedafter confirmed cleanup.
A secondary attribution issue remains: cancellation during verification now correctly becomes indeterminate, but runner.ts describes it as executor cleanup did not settle even when the artifact reports outcome: "confirmed". This wording does not need to be independently merge-blocking, but it would be clearer for the executor outcome to distinguish host cancellation from unconfirmed cleanup.
简体中文复审
感谢处理上一轮审查意见。大型终态记录问题已经解决,verification 期间发生 cancellation 也会正确产生 indeterminate attempt。目前还剩一个生产生命周期契约缺口。
生产 teardown 当前无法产生新增逻辑要求的 confirmed outcome
位置:
packages/eval/src/harness-executor.ts:827-857packages/eval/harbor/run_trial.py:141-165packages/eval/src/runner.ts:319-325
waitForTrial() 现在只有在 supervisor 收到 TERM 后以 code 0 退出时,才会确认 cleanup。不过,生产 supervisor 当前不具备这一退出语义。
收到 SIGTERM 后,run_trial.py 会取消 main task。Harbor 的 Trial.run() 会在 finally 中执行 _finalize(),然后重新抛出 CancelledError。因此,即使 teardown 已经成功完成,未捕获的 cancellation 仍会令 Python supervisor 以 code 1 退出。
我通过真实的 run_trial.main() 信号路径进行了复现,并让 run_trial() stub 在 finally 中记录 cleanup 已完成:
teardown-finally-ran
cleaned=True
exit_code=1
这会影响正常且受支持的路径:
subject 返回 infra_failed
→ runner 跳过 verification
→ executor 在 finalization 阶段发送 TERM
→ Harbor 完成 teardown,但 supervisor exit 1
→ cleanup 被分类为 terminated,而不是 confirmed
→ 原本确定的 infra_failed 被降为 indeterminate
新增 lifecycle tests 没有覆盖这一生产行为,因为其中的 fake Node supervisor 会在 SIGTERM handler 中显式调用 process.exit(0)。
这个问题直接属于本 PR “让 teardown evidence authoritative”的目标,因此我认为应在当前 PR 中解决,而不是留作 follow-up。
建议修复方向:
- 当 host-requested teardown 中的
run_trial()已经成功完成 unwind 和 finalization 后,提供明确的成功 outcome。 - teardown 本身失败时仍应保持非零 outcome,不能笼统地把所有 TERM exit 1 当成 clean。
- 使用真实的
run_trial.pyTERM 路径增加回归测试。 - 验证 subject
infra_failed在 cleanup 得到确认后仍保持infra_failed。
另外还存在一个次要的归因问题:verification 期间 cancellation 现在会正确成为 indeterminate,但即使 artifact 显示 outcome: "confirmed",runner.ts 仍将原因描述为 executor cleanup did not settle。这个措辞问题本身不必单独阻塞合并,但如果 executor outcome 能区分 host cancellation 与 cleanup 未确认,语义会更准确。
AI disclosure / AI 披露: This follow-up review was prepared with assistance from OpenAI Codex, manually reviewed by me, and I take responsibility for its conclusions. / 本次复审由 OpenAI Codex 辅助完成,已经我人工复核,其结论由我负责。
Translate only host-requested cancellation after Trial unwind into a zero supervisor exit so the executor can confirm successful teardown. Cleanup exceptions and other cancellation sources remain nonzero. Carry host cancellation and unconfirmed cleanup as explicit executor outcome causes so persisted attempts describe why they are indeterminate without inferring control state from artifacts. Validated with real run_trial SIGTERM subprocess tests and focused Eval lifecycle coverage. Generated-by: Codex
|
Addressed the remaining lifecycle contract gap in
This closes the producer/consumer contract without treating arbitrary TERM exit 1 as confirmed.
The runner maps those causes directly instead of inferring control state from cleanup artifacts:
A subject Validation:
Could you please re-review the latest head, AI assistance disclosure: Codex materially authored the fix, tests, commit message, and this response. I reviewed the resulting diff and focused validation. 简体中文对照已在
这使 producer/consumer 合同闭合,同时不会将任意 TERM exit 1 误判为 confirmed。
runner 会直接映射这些原因,不再从 cleanup artifact 反向推断控制状态:
subject 的 验证:
请重新审查最新 head AI 协助披露:Codex 实质性参与了修复、测试、commit message 和本回复的编写。我已复核最终 diff 和聚焦验证结果。 |
M4n5ter
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head. The teardown confirmation contract and cancellation attribution are now correctly closed, and the focused reproductions, Eval test suite, typecheck, and CI all pass. Approving.
Summary
Make the shared Harbor/Pier Eval execution boundary authoritative, bounded, and race-safe.
Full benchmark runs exposed coupled infrastructure failures: 15 verifiers were cancelled by Eval's private 20-second completion deadline despite task verifier timeouts of 3600–12000 seconds; 11 completed Maka executions were classified indeterminate when the Runtime Host exceeded its default 15-second settlement budget under 24-group load; and cancellation could write to a closed relay socket, causing EPIPE, runner crashes, or stale writer locks. The frozen Terminal-Bench 2.1 revision also contains task images whose Docker WORKDIR is
/app/personal-site,/workspace, or/app/dclm, disproving/appas an executor authority.This PR fixes those failures at the existing shared Eval seam:
hostSettlementTimeoutMs: 120000, forwarded through the existing Eval shim to the Runtime Host client.Runtime Host remains the sole Maka terminal-result authority. No Runtime implementation, polling, public protocol, framework-specific executor, full-evaluation result, or VM configuration is included.
The framing nonce and PID file are correctness mechanisms for normal merged-stream noise and lifecycle control, not a security boundary against an actively malicious same-UID/root child. That stronger threat model requires a separate sandbox/UID/PID-namespace or trusted-sidecar infrastructure change.
AI contribution disclosure: Codex materially authored the implementation, tests, analysis, commit messages, and this PR description. Claude and K3-256k performed read-only adversarial reviews. A human contributor must review the final diff, provenance, behavior, and submission decision before merge.
Verification
npm --workspace @maka/eval run typecheck— passednpm --workspace @maka/eval run build— passedsetsid --waitgit diff --check— passedNot run: full evaluation, paid VM execution, repository-wide tests, full-repository build, or real Harbor/Pier container lifecycle.
Review focus
containerCwd, task-workspace harness scratch, raw process log, credential value, or duplicate execution authority remains on the changed path.Checklist
Does this PR entail a change in behavior?