fix(acp): preserve streamed usage on timeout

[bingran-you]

Problem

Fixes #933.

ACP native usage was only recorded after session/prompt returned a final PromptResult. When a prompt hit BenchFlow's wall-clock or idle timeout, the prompt task was cancelled before that return path, so the session could have no usage snapshots and cleanup finalized the rollout as usage_source=unavailable with total_tokens=0.

That is wrong for agents that already stream cumulative usage through session/update: timed-out runs can have real token usage available before cancellation, and BenchFlow should preserve that best-effort lower bound instead of dropping it.

Fix

• Teach ACPSession.handle_update() to normalize and record usage telemetry carried by session/update notifications.
• Support both nested usage payloads and top-level ACP usage fields, so agents/protocol extensions can send either shape.
• Preserve the existing unknown-update behavior: unknown updates still do not produce trajectory snapshots unless they actually carried usage telemetry.
• Collect native ACP usage during Rollout.cleanup() before disconnect() clears the live session, so timeout/error paths that skipped the normal execution commit still promote the latest session snapshot.
• Tighten the timeout-cleanup regression test so it uses the real Rollout.disconnect() ordering and verifies _session is cleared after usage is preserved.
• Refresh mcp in uv.lock from 1.27.2 to 1.28.1 after CI pip-audit flagged CVE-2026-59950; this is a lock-only security update required for the same PR checks to pass.

Regression coverage

• test_acp_session_records_usage_from_session_update covers update-time usage capture.
• test_cleanup_collects_update_usage_after_timed_out_prompt covers the production cleanup path so streamed usage is collected before disconnect clears the session.
• Existing cumulative-delta/native-provider precedence tests continue to pass.
• MCP session wiring tests pass after the mcp lock refresh.

Validation

Local validation on current head 3bacd9e42cb92de585ffaa35ffa6c9db281a4e7e:

• uv sync --extra dev --extra sandbox-daytona --locked
• uv run python -m pytest tests/test_native_acp_usage.py tests/test_acp.py::TestIdleTimeoutDiagnostics::test_wall_clock_timeout_records_terminal_diagnostic tests/test_acp.py::TestIdleTimeoutDiagnostics::test_wall_clock_timeout_with_pending_tool_stays_partial -q -> 10 passed
• uv run python -m pytest tests/test_native_acp_usage.py tests/test_acp.py -q -> 91 passed
• uv run python -m pytest tests/test_acp_mcp_servers.py tests/test_task_config.py::test_task_config_accepts_mcp_tool_filter_surface -> 16 passed on earlier same-PR head after the mcp lock refresh
• uv run ruff check .
• uv run ruff check src/benchflow/rollout/__init__.py tests/test_native_acp_usage.py
• uv run ruff format --check src/benchflow/rollout/__init__.py tests/test_native_acp_usage.py
• uv run ty check src/
• git diff --check
• uv export --locked --extra dev --extra sandbox-daytona --extra sandbox-modal --format requirements-txt --no-hashes --output-file /tmp/req-audit-pr934-exact && uv run --with pip-audit pip-audit -r /tmp/req-audit-pr934-exact --no-deps --disable-pip --ignore-vuln GHSA-537c-gmf6-5ccf -> no known vulnerabilities found on earlier same-PR head after the lock refresh

Current-head GitHub validation:

• test passed
• pip-audit passed
• manifest-parity / parity passed
• integration-light / detect-scope passed
• integration-scope / detect-scope passed
• integration-light / rollout-smoke passed
• integration-scope matrix/final-review jobs skipped by scope

Artifact validation from run 29922386377:

• Downloaded integration-light-jobs and validated with .agents/skills/benchflow-experiment-review/scripts/validate_run_artifacts.py /tmp/pr934-artifacts.6gAhD8/integration-light-jobs --json.
• Result: healthy 1/1 rollout, ACP + LLM trajectories present, rollout-level results.jsonl training-ready, reward present, 323,363 tokens, 17 tool calls, 27 ACP events, and 18/18 LLM responses with usage.

Notes

This is intentionally narrow. It does not invent a new partial-usage source type or claim exact provider-cost accounting for every cancellation path. It preserves trusted native ACP usage when an ACP agent has already provided it before timeout, and leaves provider-response telemetry authoritative when available.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 54a6c0829d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[bingran-you]

Users Simulation automation review (2026-07-22): blocked on current head 62c4900ce7024e9bf94879c96ed9f53ee4f6763e.

The narrow diff and current CI look good, but the core bug is not actually fixed on the production cleanup path:

• Rollout.cleanup() calls await self.disconnect() before self._collect_native_acp_usage().
• disconnect() clears self._session, so streamed ACP usage captured before timeout is unavailable by the time cleanup tries to finalize native ACP usage.
• I reproduced this with a real Rollout.cleanup() probe on the PR head: the final metrics stayed {'usage_source': 'unavailable'} after a session update containing totalTokens=54. Moving collection before disconnect preserved usage_source='agent_native_acp' and total_tokens=54 in the sidecar control probe.
• The new regression test masks this because it replaces disconnect() with a no-op, so it never exercises the production ordering that drops _session.

Passing evidence: focused timeout/native tests passed, repo ruff/ty passed, shared CLI/SDK smoke passed, and GitHub test, pip-audit, manifest-parity, and integration-light / rollout-smoke are green. I also downloaded run 29904867853 and validated the integration-light-jobs artifact with .agents/skills/benchflow-experiment-review/scripts/validate_run_artifacts.py: healthy 1/1, ACP + LLM trajectories present, results.jsonl training-ready, reward present, 259,476 tokens, 14 tool calls, 24 ACP events, 15/15 LLM responses with usage, and timing metadata.

Thermo-nuclear review: no broad structural or 1k-line blocker; this is a lifecycle correctness blocker. Recommended fix: collect native ACP usage before disconnect() clears the session, or preserve the session reference through cleanup, and add a regression test that uses the real disconnect ordering.

Moving labels to status:blocked + review:changes-requested until the production timeout cleanup path preserves the streamed usage.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[bingran-you]

Users Simulation automation follow-up (2026-07-22): fixed the production cleanup blocker on current head 3bacd9e42cb92de585ffaa35ffa6c9db281a4e7e.

What changed:

• Rollout.cleanup() now calls _collect_native_acp_usage() before disconnect() clears _session, so streamed session/update usage survives timeout cleanup.
• The timeout-cleanup regression test now uses real Rollout.disconnect() ordering and asserts _session is cleared after usage is preserved.

Validation:

• Local: native/ACP pytest slice 91 passed; focused timeout slice 10 passed; ruff check .; touched-file ruff format --check; ty check src/; git diff --check.
• GitHub current-head checks: test, pip-audit, manifest-parity / parity, both detect-scope jobs, and integration-light / rollout-smoke passed; matrix/final-review jobs skipped by scope.
• Artifact audit: downloaded run 29922386377 integration-light-jobs artifact and validated it with benchflow-experiment-review: healthy 1/1, ACP + LLM trajectories present, rollout-level results.jsonl training-ready, reward present, 323,363 tokens, 17 tool calls, 27 ACP events, and 18/18 LLM responses with usage.

The direct code/config blocker from the previous review is resolved. Moving labels to status:ready + review:pending.

[bingran-you]

Users Simulation automation review (2026-07-24): blocked by artifact-fidelity gate on current head 3bacd9e42cb92de585ffaa35ffa6c9db281a4e7e.

What passes:

• The ACP timeout usage fix itself is verified. A real cleanup-order probe preserves native ACP usage before disconnect() clears _session.
• Current checks are green: test, pip-audit, manifest-parity / parity, integration-light / rollout-smoke, and detect-scope jobs.
• Focused sidecar validation passed: uv sync --extra dev --extra sandbox-daytona --locked, ACP/rollout/trajectory tests (143 passed), CLI/SDK regression tests (59 passed), ruff, format, ty, pip-audit, and CLI/SDK smoke.
• Exact-head integration-light-jobs run 29922386377 structurally passes benchflow-experiment-review: 1/1 healthy, ACP + LLM trajectories, one training-ready results.jsonl, reward 1.0, 323,363 tokens, 18/18 LLM responses with usage, 17 tool calls, and 281.2s timing.

Why this is still blocked:

• Manual trajectory review found benchmark leakage in the acceptance artifact. During the jax-computing-basics rollout, the agent read /app/reference/*.npy, compared its outputs against hidden reference arrays, used those answers to debug scan_rnn.npy and jit_mlp.npy, and then earned reward 1.0.
• That makes this exact artifact non-publishable for BenchFlow/Prime-RL acceptance even though the structural validator is healthy. The task artifact should be fixed or quarantined, then integration-light should be rerun with references hidden from the agent-visible workspace.
• GLM user-endpoint cost caveat persists: result.json.agent_result.cost_usd and price_source are null while token/timing telemetry is complete.

Thermo-nuclear review did not find a #934 structural blocker: the PR adds the usage collection at the correct lifecycle point and does not create a new >1k file or spaghetti branch.

Label sync: moved status:ready to status:blocked; kept review:pending because the code path still needs non-author human review after the artifact is replaced/quarantined.

AI-generated automation review posted on behalf of Bingran You.