feat(continue): typed continue-gate errors and batch skips instead of silent drops - #1084
feat(continue): typed continue-gate errors and batch skips instead of silent drops#1084JeremyJC67 wants to merge 2 commits into
Conversation
`benchflow continue` rejected any non-openhands run with a bare
"currently supports the 'openhands' agent only". True, but it left the
operator with no way to tell a *temporary* limit (no replay ingress for
this agent yet) from a *permanent* one (the run was never recorded, so
no ingress can ever help it), and no reason for the limit at all — which
is why it keeps being read as "continue only supports open models".
- `CONTINUE_SUPPORTED_AGENTS` is now the single source of truth; a future
ingress adds its agent there and nowhere else.
- `ContinueUnsupportedError` carries a `reason_code` and
`recoverable_in_principle`, so callers can triage without parsing prose.
`UnsupportedAgentError` and `MissingRecordingError` are its two cases.
- The message states the supported set, that the set is derived from the
replay wire protocol (proxy serves POST /v1/chat/completions; the
sandbox gets LLM_BASE_URL/LLM_API_KEY/LLM_MODEL, which only the
OpenHands template consumes), and whether *this* run is recoverable.
- The agent gate moved ahead of trajectory parsing so an unsupported run
missing `trajectory/llm_trajectory.jsonl` hears the permanent verdict
(subscription-auth: never continuable) instead of the agent complaint.
- The snapshot alternative is offered conditionally ("if ... was
captured"), never as a promise.
Refs benchflow-ai#1083 (proposed sequence, step 1).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…them `continue-batch` over a mixed sweep silently dropped every non-openhands timeout run at discovery, then, for anything that slipped past, recorded it as a plain failure — indistinguishable from a continuation that actually broke. An operator with 200 timed-out runs learned neither that part of the sweep was left behind nor why, and the batch's exit status blamed them for it. - `BatchContinueResult.skipped` / `.reason_code` / `.recoverable_in_principle` carry the gate's typed verdict, built by `BatchContinueResult.skip()` from the `ContinueUnsupportedError`. - `continue_batch` catches that error specifically, so one unsupported run costs only itself; the other 199 still run. - `summarize_batch` reports `skipped`/`skips` apart from `failed`/ `errors`. Skips do not set the batch's exit status. - `discover_timeout_run_folders` gained an optional `on_skip` observer, and `continue-batch` uses it to surface the runs discovery drops. Only timeout *candidates* are reported — a finished run from another agent was never a candidate and stays quiet. - The single-run path is untouched: `bench eval continue <folder>` on an unsupported run still prints the error and exits 1. Refs benchflow-ai#1083 (proposed sequence, step 1). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| except ContinueUnsupportedError as exc: | ||
| if on_skip is not None and is_timeout_run(folder): | ||
| on_skip(folder, exc) |
There was a problem hiding this comment.
🟡 Limit permits oversized batch summaries
With --limit, candidates collected by on_skip do not consume the cap. The command can report more timeout runs than requested.
Prompt for agents
Make discover_timeout_run_folders apply limit to the complete set of selected timeout candidates, including candidates delivered to on_skip, so continue-batch never summarizes more runs than requested. Preserve deterministic ordering and clarify whether invalid/non-timeout folders consume the cap. Add coverage for unsupported candidates before supported ones and an all-unsupported tree with a small limit.
Was this helpful? React with 👍 or 👎 to provide feedback.
| f"missing required artifact: {path} — record-replay needs the LLM " | ||
| "trajectory. Was this run captured with usage tracking enabled?" | ||
| ) | ||
| raise MissingRecordingError(path) |
There was a problem hiding this comment.
🟡 Unusable recordings receive incorrect triage
Empty or wholly malformed trajectories bypass MissingRecordingError, while is_file() marks them recoverable. Runs vanish silently or receive false recovery guidance.
Prompt for agents
Classify recording usability from parsed LLM exchanges rather than file existence alone. Empty or wholly malformed trajectory files need the same typed no-recording verdict as an absent file, including reason_code=no_llm_recording and recoverable_in_principle=false. Ensure unsupported-agent triage can distinguish a usable recording from a merely present file without losing the intended agent-first error, and add discovery/CLI tests for empty and all-malformed files.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b6e4ef5f2
ℹ️ 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".
| agent = str(config.get("agent") or result.get("agent") or "") | ||
| if agent not in CONTINUE_SUPPORTED_AGENTS: | ||
| raise UnsupportedAgentError( | ||
| agent=agent, has_recording=trajectory_path.is_file() |
There was a problem hiding this comment.
When a timed-out run has a zero-byte or entirely malformed llm_trajectory.jsonl, this existence check reports that an unsupported agent has a recoverable recording; for OpenHands, load_llm_exchanges instead raises a plain RunFolderError, which discovery silently swallows. Such files contain no replayable recording and are especially plausible when a run fails before its first model exchange, so they must produce no_llm_recording skips just like an absent file rather than disappearing from the batch summary or receiving an incorrect recovery verdict.
Useful? React with 👍 / 👎.
| # Single source of truth: a future replay ingress adds its agent here and | ||
| # nowhere else. The membership is *protocol*-derived, not a policy about open | ||
| # vs closed model weights — see ``_WHY_PROTOCOL`` below for the mechanism. | ||
| CONTINUE_SUPPORTED_AGENTS: frozenset[str] = frozenset({"openhands"}) |
There was a problem hiding this comment.
If a future replay ingress follows this constant's advertised one-line extension path, load_run_folder will accept the new agent, but build_rollout_config and the sandbox-proxy path in orchestrator.py still hard-code agent="openhands" when constructing the rollout and resolving provider environment. The accepted run would therefore launch OpenHands rather than its recorded agent and could produce a mislabeled continuation; either the orchestrator must dispatch using the accepted agent or support registration must include the corresponding ingress implementation rather than being controlled solely here.
Useful? React with 👍 / 👎.
|
|
||
| @pytest.mark.asyncio | ||
| async def test_mixed_batch_skips_unsupported_and_continues_the_rest(tmp_path): | ||
| """A batch of mostly-openhands runs continues; the odd ones out are skips.""" |
There was a problem hiding this comment.
This newly added regression test, along with the other new tests in test_batch.py and test_run_folder.py, uses behavioral prose or issue #1083 rather than naming the PR or commit it guards. Update each regression-test docstring with the required PR/commit attribution so future maintainers can trace the protected behavior.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
1 participant
Continue triage: say why a run can't be resumed, and don't let one unsupported run tax the batch
Implements items 2 and 3 of step 1 in the proposed sequence from #1083. No replay-protocol code is touched.
Why
bench eval continuerejected every non-openhandsrun withbenchflow continue currently supports the 'openhands' agent only. The statement is true but uninformative in three ways, which is why the limitation keeps getting read as "continue only supports open models":What's in scope here
A typed, self-explaining gate.
CONTINUE_SUPPORTED_AGENTSis now the single source of truth — a future ingress adds its agent there and nowhere else.ContinueUnsupportedErrorcarries areason_codeandrecoverable_in_principleso callers can triage without parsing prose;UnsupportedAgentErrorandMissingRecordingErrorare its two cases. Both remainRunFolderErrorsubclasses, so every existing catch site is unaffected.The message now states the supported set, that the set is derived from the replay wire protocol (the proxy serves
POST /v1/chat/completionsonly; the sandbox receivesLLM_BASE_URL/LLM_API_KEY/LLM_MODEL, which only the OpenHands template consumes; another wire would 404 and fall back to host credentials, burning a live run the artifacts would then mislabel as a replay), and whether this particular run is recoverable at all.That last point required moving the agent gate ahead of trajectory parsing. Previously a run that was both unsupported and unrecorded heard only the missing-artifact complaint. Now a subscription-auth run is told plainly that it can never be continued — before a protocol ingress ships and raises false hope.
Batch continue reports skips instead of scoring them as failures. One correction to the framing in #1083:
continue_batch._onealready wrapped each run, so a batch never aborted — the real defect was quieter.discover_timeout_run_foldersswallowed unsupported runs at discovery with a bareexcept RunFolderError: continue, so a sweep of 200 with 10 unsupported printedContinuing 190 timeout run(s)and exited 0 with the operator never learning the 10 existed; and anything that did reach the runner was recorded as a plain failure, indistinguishable from a continuation that actually broke.Now:
continue_batchcatches the typed error per run and records a skip;summarize_batchreportsskipped/skipsseparately fromfailed/errors, each skip carrying itsreasonandrecoverable_in_principle; skips do not set the batch's exit status.discover_timeout_run_foldersgained an optionalon_skipobserver (defaultNone; signature and return type unchanged) andcontinue-batchuses it to surface what discovery drops — only timeout candidates, so a finished run from another agent stays quiet in a mixed jobs dir.The single-run path is deliberately unchanged:
bench eval continue <folder>on an unsupported run still prints the error and exits 1. An explicit request should fail loudly; only a sweep should skip.Deferred
--snapshot-stagesonbench eval run(item 1 of step 1 in bench eval continue is limited by replay wire protocol, not model openness — and timed-out runs lose their world before they can be salvaged #1083). The underlying machinery —RolloutConfig.snapshot_stages, the stage taxonomy, stage capture, and the--keep-snapshotsexport — is not onmainyet (it is in feat(branch): composed snapshotting, stage-level branching, and bench eval ablate #1046), so the flag has no destination to thread to. It follows once that lands, and the error message's snapshot clause gains the flag name at the same time. The clause is phrased conditionally and names no flag today, so it makes no promise the build cannot keep.claude-agent-acpcontinue support (step 2 of bench eval continue is limited by replay wire protocol, not model openness — and timed-out runs lose their world before they can be salvaged #1083): an Anthropic Messages ingress, or the gateway-in-front redesign. Adding the agent toCONTINUE_SUPPORTED_AGENTSis then a one-line change on this branch's foundation.codex-acp(step 3);geminiscoped out, per the issue.Testing
10 new tests, each verified red before its change. Full suite 5895 passed / 94 skipped / 7 deselected against a pre-change baseline of 5885 / 94 / 7 — the delta is exactly the new tests.
ruff check,ruff format --check src tests tools, andty check src/all clean.tests/test_cli_docs_drift.pypasses unmodified; no CLI flag was added, so the docs change is prose only.Closes nothing on its own — #1083 stays open for the protocol work.