feat: timeout diagnosability, wall recommendations, and duration telemetry (#11) - #12
Merged
Merged
Conversation
#11) A review that blew its --wall reported only "backend wall_timeout after 605s": no phase, no evidence the provider had ever responded, no resolved model, and no next step but a guess. The safety behavior was already right (a timeout was never reported as a pass) — what was missing was predictability and diagnosis. - Timeouts now carry `telemetry`: the phase timeline (consent -> spawn -> first byte -> exit -> validated), whether any bytes ever arrived, time to first byte, retry counts and the resolved model. The supervisor records first_byte_s/bytes_received on every path, timeouts included, so "the backend never spoke" is distinguishable from "it spoke, then stalled". - New `impasse_run.py estimate` (purely local; sends nothing, needs no consent) and a `wall_advice` block on every result, also printed to stderr before the send. `basis` says where the number came from: heuristic is a shipped estimate padded for margin, NOT a measurement of this account; empirical is fitted from >=5 of this machine's completed runs for that backend+model. - A local timing store (config_dir()/metrics.jsonl, 0600, newest 1000 rows) records every run that reached the backend, failures included. It holds no artifact content, structurally: writes are filtered to a field allowlist. The one content-derived field, the digest, is withheld under --no-record/--raw. New `impasse_report.py performance`; `--forget` deletes it, IMPASSE_NO_METRICS disables it. - Timeouts return ranked recovery options with exact commands, each stating what it changes — time budget, model, depth, scope, or the independence tier — plus reusable_result: false, since a timeout leaves nothing to resume. - The claude backend runs --output-format json and reads the review from the envelope's `result`, recording modelUsage/ttft_ms/session_id. Non-envelope stdout still parses as before, claiming no resolved model. Codex names no model in its event stream, so codex runs report requested/backend_default and never overstate. Two defects found while self-reviewing this change and fixed here: the timing store read its head rather than its tail (trimming would have discarded the newest rows and percentiles described the oldest), and the empirical fit could extrapolate a near-zero rate from small fast reviews into a too-short wall for a large artifact — it now falls back to the shipped estimate beyond observed sizes and says so. Item 5 of the issue (opt-in supervised chunking) is deferred: it changes the protocol rather than the runner and is outside the issue's own acceptance criteria. Designed, not built, in docs/proposals/supervised-chunking.md. Also fixes an unrelated pre-existing test bug: the resolve_codex_command ChatGPT.app case asserted a path suffix that a higher-priority Homebrew codex fails and a system-wide ChatGPT.app passed for the wrong reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dogfooding round: an Impasse review of the issue-#11 diff (codex, high effort, Fast mode) raised 11 findings. All were verified against the code and fixed. - F001 (critical) failure-as-success: a claude envelope marked `is_error` was only inspected inside `if exit_code != 0`, so an error envelope with a zero exit could have its `result` parsed and returned ok. Exit code and envelope are independent signals; both are now checked. - F002 the byte signal was overinterpreted. This run's own telemetry disproves the old wording: codex's first byte arrived at 0.053s (a thread-started event), which says nothing about model progress. The timeout messages, the RunResult/supervise docstrings and SKILL.md now state only what the signal shows and what it rules out. - F003 the metrics allowlist bounded keys but not value types or lengths, and `model_resolved` is backend-supplied. Values are now sanitized to finite numbers, booleans, 200-char strings, None, or a bounded phase map — which makes the "no artifact content" claim structural on both axes, and bounds a single row's size. - F005/F006/F011 recommendation correctness: history is now matched on effort and speed (a low-effort history must not size a high-effort review), the empirical estimate is floored at the observed p90 so a zero-rate fit cannot collapse it, and the docstring states each mode's exact claim instead of one that held in neither. - F007 timeout comparability is bounded 0.5x-2.0x rather than only below, and a recommendation clamped by the ceiling below a known-exceeded cap now says it is not expected to be enough. - F008 json.loads raises RecursionError, not ValueError, on deeply nested input; all three parsers on the untrusted-stdout path now catch it. - F009 the performance report raised TypeError on a null or non-numeric field; series are filtered to finite numbers and missing values render as an em dash. - F010 the version probe (20s timeout) ran before the deadline was set, so a review could exceed the cap --wall documents as total. The budget now starts first. F004 (head-vs-tail read of the timing store) was found independently while self-reviewing before the review returned, and was already fixed in the working tree when the reviewer flagged it against the submitted diff. 19 regression checks added. Reconciliation saved under the run record `issue-11-adversarial-review`: converged, 11 resolved, nothing escalated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#11) The commit that applied the first review's 11 findings (a26b736) had itself never been reviewed. Sent it back for an independent cross-provider review: 7 findings, all verified against the code, all fixed. The pattern worth recording: three of the seven were the SAME failure mode — a fix applied to the one path the original finding named, while sibling paths on the same hazard were left untouched. That is the characteristic risk of fixing from a findings list, and the reason to review a fix commit rather than trust it. - F002 RecursionError on untrusted reviewer stdout reached three more parsers. Round one hardened _claude_envelope and _codex_stream_meta and stopped; _unwrap_error, the codex JSONL scan in _extract_backend_error, and _parse_reviewer_json still caught only json.JSONDecodeError, which RecursionError does not subclass. All three classify it now. _parse_reviewer_json normalizes it to a JSONDecodeError so every future caller is covered, not just today's one (which already caught it — making that path the least exposed of the three). - F001 the metrics store's no-artifact-content guarantee is now per FIELD. The sanitizer bounded types and lengths but was shape-only, so a dict handed to a scalar field would have stored its KEYS verbatim. Values are typed by destination field; a dict on a scalar field is dropped. The absolute doc claim is replaced with the exact one: the guarantee binds CALLERS, and model_resolved / backend_version stay backend-controlled within 200 chars. - F003 --wall now covers the version probe. Round one moved deadline creation ahead of it but the probe kept a fixed 20s timeout and never received the budget. backend_version is bounded by min(20s, remaining) and skips when nothing is left. Teardown is deliberately NOT bounded by the wall — shrinking those joins risks leaking a live subprocess — so --wall help discloses the few seconds it may add. - F004 performance no longer pools incompatible histories. The effort/speed match landed in recommend_wall but the report bypassed it via pre-grouped rows=, so the number the operator SEES still mixed low- and high-effort runs. It groups on the same four keys the library fits on and labels each group's settings. - F005 NaN/Infinity crashed the report. "Filtered to finite numbers" was an isinstance check; both are floats and passed it, raising at the int() that formats them. _nums and _percentile test math.isfinite and exclude bools. - F006 OverflowError escaped record_metrics, which documents itself TOTAL, on the failure paths whose diagnosis it exists to serve. Ints are bounded before any float() and the handler catches ArithmeticError. - F007 two round-one checks did not pin their fixes. The floor test asserted >= 250 where the unfloored path already returned ~375. Replaced with a case where the floor binds: ~440s unfloored against an observed 800s p90. Proven by deleting `est = max(est, p90)` — the new check fails at 480 < 800 where the old one passed. 15 regression checks added, each verified to FAIL against code with its fix removed. Docs updated in the same change (security-model, glossary, --wall help, CHANGELOG). Reconciliation: /Users/johnknox/Library/Application Support/impasse/runs/ a26b736-fix-review/reconciliation-result.json — converged, 7 resolved, 0 escalated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBJxgqjWtXmstng2dGz7SG
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #11.
A review that blew its
--wallreported onlybackend wall_timeout after 605s— no phase, no evidence the provider had ever responded, no resolved model, and no next step but a guess. The safety behavior was already correct (a timeout was never reported as a pass); what was missing was predictability and diagnosis.What's implemented
Six of the issue's seven proposals, covering all six acceptance criteria:
telemetry.phases,received_any_bytes,ttfb_simpasse_run.py estimate,wall_adviceon every resultmetrics.jsonl+impasse_report.py performancerecoveryoptions with exact commands--output-format jsonenvelopeItem 5 (supervised chunking) is deferred — it changes the protocol rather than the runner and is outside the issue's own acceptance criteria. Designed, not built, in
docs/proposals/supervised-chunking.md, including the honesty problem that governs it: chunking buys completion at the cost of cross-cutting findings, and chunk agreement is not corroboration.Honesty properties worth reviewing closely
basisis always reported.heuristicis a shipped estimate padded for margin, not a measurement of your account;empiricalis fitted from ≥5 of this machine's completed runs at the same backend, model, effort and speed. A timeout is excluded from the duration fit (it records when we stopped waiting, not how long the review needed) but raises a floor, so a cap already exceeded is never re-recommended.--no-record/--raw.performance --forgetdeletes it;IMPASSE_NO_METRICS=1disables it.Dogfooded, and it paid
A cross-provider review of this change (codex,
--effort high, Fast mode; 24.9K-token diff, 239s against a 1860s recommended wall) raised 11 findings, all verified and fixed in the second commit — including a genuine failure-as-success path, an overclaiming timeout message that the run's own telemetry disproved, a metrics allowlist that bounded keys but not values, and aRecursionErrorescape on untrusted backend JSON. Run recordissue-11-adversarial-review: converged, 11 resolved, nothing escalated.One defect (head-vs-tail read of the timing store) was found independently during self-review before the reviewer returned, and was already fixed when it was flagged.
Also
Fixes an unrelated pre-existing test bug: the
resolve_codex_commandChatGPT.app case asserted a path suffix that a higher-priority Homebrewcodexfails and a system-wideChatGPT.apppassed for the wrong reason.Gates
All three pass:
tests/test_helpers.py,validate_schemas.py,ruff check scripts/ tests/.🤖 Generated with Claude Code