Skip to content

feat: timeout diagnosability, wall recommendations, and duration telemetry (#11) - #12

Open
windaddict wants to merge 2 commits into
mainfrom
feat/timeout-telemetry-wall-estimate
Open

feat: timeout diagnosability, wall recommendations, and duration telemetry (#11)#12
windaddict wants to merge 2 commits into
mainfrom
feat/timeout-telemetry-wall-estimate

Conversation

@windaddict

@windaddict windaddict commented Aug 14, 2026

Copy link
Copy Markdown
Owner

Closes #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 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:

# Proposal Where
1 Measure and expose review phases telemetry.phases, received_any_bytes, ttfb_s
2 Payload/model-aware wall recommendations impasse_run.py estimate, wall_advice on every result
3 Preserve local duration telemetry metrics.jsonl + impasse_report.py performance
4 Concrete recovery advice ranked recovery options with exact commands
6 Resolve and display the actual model claude --output-format json envelope
7 Timeout tests with fake backends 49 new checks

Item 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

  • basis is always reported. heuristic is a shipped estimate padded for margin, not a measurement of your account; empirical is 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.
  • The timing store holds no artifact content, structurally. Writes are filtered to a key allowlist and a value sanitizer (finite numbers, booleans, ≤200-char strings, a bounded phase map). The one content-derived field, the digest, is withheld under --no-record/--raw. performance --forget deletes it; IMPASSE_NO_METRICS=1 disables it.
  • The byte signal is reported for exactly what it is — whether the CLI wrote anything, not whether the model made progress.

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 a RecursionError escape on untrusted backend JSON. Run record issue-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_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.

Gates

All three pass: tests/test_helpers.py, validate_schemas.py, ruff check scripts/ tests/.

🤖 Generated with Claude Code

windaddict and others added 2 commits August 14, 2026 16:11
@windaddict @claude
feat: timeout diagnosability, wall recommendations, duration telemetry (
9f26941 
#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>
@windaddict @claude
fix: apply the cross-provider review's findings on this change (#11)
a26b736 
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>
@windaddict windaddict mentioned this pull request Aug 15, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve Claude review reliability, timeout diagnostics, and ETA prediction

1 participant

@windaddict