docs(mt#4996): Settle the 120s toolloop timeout: accept the cadence, record it at the constant

[minsky-ai]

Summary

mt#1897 named the cause of the reviewer's openai.chat.completions.create.toolloop 120s timeouts
but reached terminal DONE with its strategy half undelivered. mt#4996 carries that remainder:
choose the remedy (mt#1897 SC4), implement it or explicitly defer with justification (AT3), and add
a regression guard only if a timeout moves (AT4).

Chosen: option (3) — accept the cadence. Options (1) stall detection, (2) Responses API, and
(4) webhook-concurrency bound are declined; (2) is ceded to an existing owner rather than
dropped. No behaviour changes. What ships is a decision record in the two places a future reader
actually lands.

Why, from measurements re-derived this session

Every figure was queried live against review_timing, not inherited from the spec. Populations are
named because they differ.

Recovery, all rows (n = 8,993, 2026-05-25 → 2026-09-05): 134 timeout events, 115 reviews
carrying at least one, 1 unrecovered, all time — 99.25% event-level recovery.

Round latency, completing rounds only (n = 53,038): p50 7.7s · p95 38.2s · p99 62.3s · p99.9
105.0s · max 118.0s. Only 33 rounds (0.062%) exceed 110s.

1. Do not lower the cap. It sits above p99.9. Lowering it starts truncating legitimate work, and
   a truncated round has already spent its reasoning tokens and must restart. This is AT2's concern,
   measured over all history rather than one day.
2. Do not raise it. A round at the cap produced nothing. Every observed failure is at
   round=0; PR feat(mt#4954): Settle which root a hook's module path resolves to, and pin the invariant #3625 burned four consecutive 120s attempts across two retry layers and completed
   no round. More budget lengthens each failure and recovers nothing.
3. Option (1) is not a tune. The loop calls chat.completions.create non-streaming, so no
   byte arrives before the whole completion does — there is no time-to-first-token to budget
   against. Adopting it means converting the loop to streaming, alongside tool-call accumulation,
   the mt#2828/mt#2863 emission guards, and usage accounting.
4. The total being bought back is small. 134 events × 120s ≈ 4.5 hours of cumulative added
   latency across 103 days (~2.6 min/day).
5. The harmful case already pages. The single unrecovered row emitted
   reviewer-pre-submit-failure/v1 organically (mt#4881 SC6).
6. The choice does not depend on the unresolved cause. The per-request hang is inferred and
   architecturally unobservable to withSdkRetryVisibility (mem#1373). Options (1)/(2) are derived
   from it; option (3) rests only on the recovery rate and the completing-round distribution.
   Premise-independence is a reason to prefer it.

One correction to the handoff that queued this task (mt#5006). It called 2026-09-04 "the worst
day observed" with "5 of 6 recovered". 2026-08-18 was worse on both measures — 27 timeout events
across 15 of 103 reviews (14.6%) vs 09-04's 9 across 9 of 194 (4.6%) — and recovered 27 of 27.
Bursts recur roughly every 13 days (8 days in 103 carry ≥5 events), which is evidence for
accepting: the layers have absorbed a heavier burst than the one that reopened the question.

Key changes

Two files, 69 insertions, 2 deletions, all prose — no executable statement, constant, or config
value is touched.

• services/reviewer/src/providers.ts — a decision-record comment on the
  DEFAULT_MODEL_TIMEOUT_MS docblock: the measurement, why neither direction is right, why stall
  detection is unreachable from here, and the reopen triggers. It lives on the constant because
  mt#1897 re-opened this question three times and twice argued for raising the cap from percentiles
  that were artifacts of the cap itself (mem#1373).
• services/reviewer/README.md — a ### The 120s model timeout is settled subsection, plus two
  corrections to ### Network-call timeouts. One of those was wrong, not merely stale: the
  tuning advice told operators to lower reasoning_effort when model timeouts fire. A timing-out
  round is not a round that ran long, so that would cost review quality without reducing timeouts.

SC2 — coordination with mt#2718 and mt#3526

Outcome: no change needed, and no coordination debt created — nothing moves. MAX_TOOL_ROUNDS,
the retry policy, DEFAULT_MODEL_TIMEOUT_MS and DEFAULT_TOOLLOOP_RETRY_TIMEOUT_MS are all
untouched.

Read at source: the July 2026 reviewer-cost audit, the frame mt#2718 was built on. Two things bear
directly. Its §7 non-drivers already lists "Retries — 4.7% timeout, 1.6% full re-run. Real tail
risk, not a baseline driver" — an independent, earlier judgment from a cost investigation, on
different evidence, agreeing with this one. And its §7 stretch row already owns option (2):
"migrate Chat Completions → Responses API … 40–80% better cache utilization", effort L, status
future. So option (2) is not "never explored" — it is inventoried as an mt#2718 cost lever with
a stronger justification than the timeout question supplies. Ceded there; not duplicated here. (That
40–80% figure is the audit's relay of an OpenAI claim, not read at the vendor source by this pass —
strong-evidence, and not load-bearing, since the option is being ceded rather than adopted.)

mt#3526 governs the round budget — loops that complete but never call conclude_review. Changing it
would move this task's max-duration input, not the per-attempt cap. No conflict either direction.

Reopen triggers

Reopen the remedy question — do not re-derive the measurement — on any of, over a rolling 30 days:
≥2 timeout-unrecovered rows (baseline 1 in 103 days); event-level recovery below 95%
(baseline 99.25%); completing-round p99.9 crossing 115s (baseline 105.0s), the one condition that
would make the cap genuinely tight against real work.

Acceptance tests

AT1 and AT2 are not applicable — both describe a stall-detection mechanism, and none ships. That
is what AT3 exists to express. AT3 was amended in place rather than explained elsewhere: as
filed it read "no code changed", which taken literally forbids the documentation SC4's own
rationale asks for. The amendment preserves the original wording above it, states what actually
ships, and gives the basis. See mt#4996 ## Acceptance Tests.

Execution evidence:

No test file is added or modified — this PR changes prose only. The evidence the decision rests on
is the live measurement, so it is the run output pasted here.

AT3 / SC1 / SC4 — the decision and its basis, from review_timing (2026-09-05 ~04:00Z):

-- recovery, all rows
unrecovered_alltime | reviews_with_any_timeout | total_timing_rows | first_row  | last_row
                  1 |                      115 |              8993 | 2026-05-25 | 2026-09-05

-- round latency, completing rounds only (model-call reviews)
all_rounds | completing | at_or_over_cap | p50 | p95  | p99  | p999  | max   | >90s | >100s | >110s
     53122 |      53038 |             84 | 7.7 | 38.2 | 62.3 | 105.0 | 118.0 |  134 |    78 |    33

-- trend, model-call population (token instrumentation starts ~2026-07)
2026-07: 27 events / 1870 reviews (1.44%)
2026-08: 44 events / 3506 reviews (1.25%)
2026-09 (5d): 9 events / 388 reviews (2.32%, one burst in a short window)

SC4 also requires mt#1897 to be cross-referenced so a future burst does not reopen the question
from scratch: it is cited in the spec's ## DECISION, in the providers.ts docblock, and in the
README subsection.

SC2 — coordination record: written above and into the spec; no timeout, round budget, or retry
policy changed, so no before/after measurement is owed.

SC3 — regression guard: not applicable. It is conditional on a timeout value or budget changing,
and none does.

SC5 — verified against a real burst: the chosen remedy is the existing retry stack, and it has
been exercised by 8 real bursts / 134 events with 133 recoveries, including 27-of-27 on the heaviest
(2026-08-18). This is a stronger discharge than a synthetic reproduction would be.

Local checks (session workspace df3c8366): typecheck pass, 0 errors, 8 projects including
services/reviewer (validatedWorkspace confirmed as the session dir, not main); lint pass, 0
errors / 0 warnings across 4,388 files; prettier clean on both files.

Deploy verification:

Both changed files return true from isDeploySurfaceFile — verified by running the predicate over
this PR's actual changed-file list rather than recalling a pattern set:

$ bun -e 'import { isDeploySurfaceFile } from "./packages/domain/src/deployment/deploy-surface.ts";
  for (const f of process.argv.slice(1)) console.log(isDeploySurfaceFile(f), f);' \
  services/reviewer/src/providers.ts services/reviewer/README.md
true services/reviewer/src/providers.ts
true services/reviewer/README.md

So this is not [no-deploy-impact], comment-only though it is. After merge I will run
deployment_wait-for-latest against the reviewer service with notBefore set to the merge
timestamp and expectCommitSha set to the merge SHA, read buildIdentity, and assert the /health
body's service field is minsky-reviewer rather than accepting the status code. No external-system
integration changes, so no live-exercise beyond deploy health is owed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YT3CvxJhrcGeVCv8P3DGPD

[minsky-reviewer]

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 316K prompt, 3K completion | Duration: 59s
Mode: normal

Commands

• /review — request a fresh review

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Reviewed the documentation-only PR that records a decision to keep the 120s toolloop timeout and corrects prior operator guidance. No executable behavior changed; constants and retry logic remain untouched. The additions are well-argued and align with the task spec’s success criteria. I found two non-blocking issues: embedding point-in-time metrics in a code comment is brittle and will stale; consider summarizing invariants and linking to the canonical query, and clarify in README that while the env knob exists, production policy is not to tune it absent the named triggers. With those nits noted, the PR meets the spec and can merge.

Findings

• [NON-BLOCKING] services/reviewer/src/providers.ts:46 — Hard-coded live metrics in a source comment are brittle and will stale quickly
  The new docblock above DEFAULT_MODEL_TIMEOUT_MS embeds specific counts and distribution figures (e.g., “53,038 completing rounds… p99.9 105.0s… 133 of 134 recovered… 1 unrecovered in 103 days”). These will drift as soon as more data lands and risk becoming misleading over time. Consider summarizing the invariants (e.g., “cap > p99.9 over completing rounds; retries recover ≈99%+ of events; do not lower/raise without X triggers”) and linking to the canonical query/spec section instead of freezing point-in-time numerics in code. If you keep the figures, include an explicit as-of date (the README does; mirror that here) and note they are illustrative, with the durable reopen triggers listed separately.
• [NON-BLOCKING] services/reviewer/README.md:574 — Operator guidance could clarify that the timeout is configurable but should not be tuned in normal ops
  The new subsection strongly says “do not tune it,” which is fine as a policy, but the environment knob (REVIEWER_MODEL_TIMEOUT_MS) remains present and documented. To reduce confusion for operators, consider adding a one-line clarification that the env var exists for parity/testing but production policy is to leave it at 120s unless the named reopen triggers fire. This avoids a perceived contradiction between a visible knob and a “never change it” stance.

Spec verification

Criterion	Status	Evidence
One option above is chosen, with the choice and its basis recorded — citing the measurement, not re-deriving it.	Met	README adds “The 120s model timeout is settled — do not tune it (mt#4996)” selecting option (3) and cites concrete measurements; providers.ts docblock records the same. See services/reviewer/README.md:“### The 120s model timeout is settled …” and services/reviewer/src/providers.ts:46-81.
SC5 of mt#1897 is honored: no change to MAX_TOOL_ROUNDS or the retry policy ships without coordinating with mt#2718 and mt#3526. Record the coordination outcome, including “no change needed” if that is the answer.	Met	README subsection explicitly states no code changes and records coordination outcome; PR body likewise. No edits to MAX_TOOL_ROUNDS, retry policy, or timeouts appear in the diff (providers.ts only adds comments). Evidence: services/reviewer/src/providers.ts shows constants unchanged; README records no-change coordination.
If a timeout value or budget changes, a regression guard exists (mt#1897 AT4) keyed to the in-regime, completing-rounds percentiles — never to a population containing capped rows.	N/A	No timeout or budget changed in this PR (diff shows comments/README only), so AT4/SC3 does not apply.
If option (3) is chosen, the justification is written into this spec and mt#1897 is cross-referenced, so a future burst does not reopen the question from scratch.	Met	README subsection cites mt#4996 and mem#1373, describes measurement, justifications, and reopen triggers; providers.ts docblock cross-references mt#1897/mem#1373. See services/reviewer/README.md section and services/reviewer/src/providers.ts comment block.
The chosen remedy is verified against a real burst or a faithful reproduction — a hang, not merely a slow response.	Met	README and comment cite recovery across real bursts (e.g., 2026-08-18: 27/27 recovered) and 133/134 recovered total; although no code changes, verification is by historical production events, which meets SC5. Evidence text embedded in README and providers.ts comments.

Documentation impact

• updated-in-pr — This PR is documentation-only: it edits services/reviewer/README.md and adds an extended decision-record comment in services/reviewer/src/providers.ts. The README explicitly updates operator guidance under “Network-call timeouts” and adds a new subsection “The 120s model timeout is settled,” correcting prior advice. No executable behavior changed, but documented behavior/guidance did — and the docs were updated in this PR accordingly.
  Affected: services/reviewer/README.md

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Verification complete. The follow-up commit confines changes to documentation-only updates: a settled-decision subsection in services/reviewer/README.md and an expanded docblock on DEFAULT_MODEL_TIMEOUT_MS in services/reviewer/src/providers.ts. Both prior R1 non-blocking notes are addressed by explicitly marking the cited figures as a dated snapshot and clarifying that the knob remains configurable but should not be tuned in normal operation. No executable code, constants, or configuration were modified. Success criteria are satisfied: option (3) is chosen and recorded with measurements; coordination with mt#2718/mt#3526 is documented; regression guard is N/A since no timeout/budget changed; mt#1897 and mem#1373 are cross-referenced; and the chosen remedy is evidenced against real bursts. I find no new defects introduced by this commit. Event: APPROVE.

Spec verification

Criterion	Status	Evidence
One option above is chosen, with the choice and its basis recorded — citing the measurement, not re-deriving it.	Met	services/reviewer/README.md: “### The 120s model timeout is settled — do not tune it (mt#4996)” records option (3) with measurements; services/reviewer/src/providers.ts:46-88 docblock records the decision and cites figures as a dated snapshot.
SC5 of mt#1897 is honored: no change to MAX_TOOL_ROUNDS or the retry policy ships without coordinating with mt#2718 (cost) and mt#3526 (round budget). Record the coordination outcome, including "no change needed" if that is the answer.	Met	services/reviewer/README.md: the settled-decision section states no timeout, round budget, or retry policy changed and records the coordination outcome; the task spec’s SC2 text is mirrored in README prose.
If a timeout value or budget changes, a regression guard exists (mt#1897 AT4) keyed to the in-regime, completing-rounds percentiles — never to a population containing capped rows.	N/A	No timeout or budget change ships in this PR; README explicitly says no code changed and advises not to tune the knob. Therefore AT4/SC3 does not apply.
If option (3) is chosen, the justification is written into this spec and mt#1897 is cross-referenced, so a future burst does not reopen the question from scratch.	Met	Task spec body includes the full decision rationale; README section cites mt#4996 decision and mem#1373; providers.ts docblock references mt#1897/mem#1373. Cross-references present.
The chosen remedy is verified against a real burst or a faithful reproduction — a hang, not merely a slow response, since the two are what this whole investigation had to separate.	Met	services/reviewer/README.md settled section enumerates 134 timeout events with 133 recoveries and cites round-latency distribution and the 2026-08-18 burst. The remedy is the existing retry stack exercised by multiple real bursts; no new mechanism to reproduce needed.

Documentation impact

• updated-in-pr — This PR is purely documentation/prose: it adds a settled-decision section in services/reviewer/README.md and a decision-record docblock on DEFAULT_MODEL_TIMEOUT_MS in services/reviewer/src/providers.ts. It corrects prior tuning advice and records the chosen option (accept the cadence) with dated measurements. No executable behavior changed, and the docs were updated in this PR to reflect the decision.
  Affected: services/reviewer/README.md, services/reviewer/src/providers.ts

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Verification pass complete. The fix commit confines changes to documentation-only updates: an expanded decision-record docblock on DEFAULT_MODEL_TIMEOUT_MS in services/reviewer/src/providers.ts and a new settled-decision subsection plus tuning-guidance correction in services/reviewer/README.md. No executable code, constants, or configuration were modified; MAX_TOOL_ROUNDS, retry logic, and timeout values remain unchanged. Success criteria are satisfied: option (3) is chosen and justified with cited measurements; coordination with mt#2718/mt#3526 is recorded; regression guard is N/A since no budget changed; cross-references to mt#1897/mem#1373 are present; the remedy is evidenced against real bursts. I find no new defects introduced by this commit. Event: APPROVE.

Spec verification

Criterion	Status	Evidence
One option above is chosen, with the choice and its basis recorded — citing the measurement, not re-deriving it.	Met	services/reviewer/README.md:574-626 adds “The 120s model timeout is settled — do not tune it (mt#4996)” selecting option (3) and cites corpus-wide percentiles and recovery counts; services/reviewer/src/providers.ts:46-86 mirrors the decision and records figures as a dated snapshot.
SC5 of mt#1897 is honored: no change to MAX_TOOL_ROUNDS or the retry policy ships without coordinating with mt#2718 (cost) and mt#3526 (round budget). Record the coordination outcome, including "no change needed" if that is the answer.	Met	PR description and README subsection explicitly state no changes to MAX_TOOL_ROUNDS or retry policy; coordination outcome “no change needed” is recorded in the PR body under “SC2 — coordination with mt#2718 and mt#3526”. No code changes to those constants in services/reviewer/src/providers.ts (MAX_TOOL_ROUNDS remains 10; retry logic unchanged).
If a timeout value or budget changes, a regression guard exists (mt#1897 AT4) keyed to the in-regime, completing-rounds percentiles — never to a population containing capped rows.	N/A	No timeout or budget was changed. README and providers.ts emphasize no behavior/config change; only documentation/decision record added.
If option (3) is chosen, the justification is written into this spec and mt#1897 is cross-referenced, so a future burst does not reopen the question from scratch.	Met	services/reviewer/README.md:590-626 includes the justification, cites mem#1373 and mt#1897; providers.ts docblock at DEFAULT_MODEL_TIMEOUT_MS also references mt#4996 and mem#1373.
The chosen remedy is verified against a real burst or a faithful reproduction — a hang, not merely a slow response, since the two are what this whole investigation had to separate.	Met	README subsection summarizes recovery over 134 timeout events with 133 recovered, cites specific burst (2026-08-18) and PR #3625 sequence; providers.ts docblock repeats recovery evidence. No new tests required per amended ATs since option (3) ships no mechanism.

Documentation impact

• updated-in-pr — This PR is documentation-only: it adds a settled-decision subsection and corrects prior guidance in services/reviewer/README.md (lines ~574-626) and expands the docblock over DEFAULT_MODEL_TIMEOUT_MS in services/reviewer/src/providers.ts (lines ~46-86). Behavior and constants remain unchanged.
  Affected: services/reviewer/README.md

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Verification complete. This fix commit confines changes to documentation: an expanded decision-record docblock adjacent to DEFAULT_MODEL_TIMEOUT_MS in services/reviewer/src/providers.ts and a new settled-decision subsection (plus corrected tuning guidance) in services/reviewer/README.md. No executable code, constants, or retry/round budgets were modified. The task’s Success Criteria are satisfied: option (3) is explicitly chosen and justified with corpus-wide measurements; coordination with mt#2718/mt#3526 is recorded as “no change needed”; regression guard is N/A since no timeout/budget moved; mt#1897/mem#1373 are cross-referenced; and the chosen remedy is evidenced against real bursts. I find no new defects introduced by these changes. Event: APPROVE.

Spec verification

Criterion	Status	Evidence
One option above is chosen, with the choice and its basis recorded — citing the measurement, not re-deriving it.	Met	services/reviewer/README.md: "The 120s model timeout is settled — do not tune it (mt#4996)" subsection records option (3) and cites corpus-wide measurements (p50 7.7s, p99.9 105.0s, 133/134 recovered). services/reviewer/src/providers.ts:46-86 adds the decision-record docblock with the same basis.
SC5 of mt#1897 is honored: no change to MAX_TOOL_ROUNDS or the retry policy ships without coordinating with mt#2718 (cost) and mt#3526 (round budget). Record the coordination outcome, including "no change needed" if that is the answer.	Met	services/reviewer/README.md: in the settled-decision subsection, it explicitly states no code changes and defers the Responses API to mt#2718 as a cost lever; it does not alter MAX_TOOL_ROUNDS or retry policy. The PR body and spec also record "no change needed" coordination. No diffs touch those constants in services/reviewer/src/providers.ts beyond comments.
If a timeout value or budget changes, a regression guard exists (mt#1897 AT4) keyed to the in-regime, completing-rounds percentiles — never to a population containing capped rows.	N/A	No timeout or budget changed in this PR (comments and README only). DEFAULT_MODEL_TIMEOUT_MS remains 120_000; MAX_TOOL_ROUNDS untouched. Therefore AT4 does not apply.
If option (3) is chosen, the justification is written into this spec and mt#1897 is cross-referenced, so a future burst does not reopen the question from scratch.	Met	services/reviewer/README.md: the subsection cites mt#4996 and mem#1373 and explains why lower/raise are wrong; providers.ts docblock references mt#1897/mem#1373 rationale. Cross-references present and placed at the constant docblock to catch future readers.
The chosen remedy is verified against a real burst or a faithful reproduction — a hang, not merely a slow response, since the two are what this whole investigation had to separate.	Met	services/reviewer/README.md settled-decision subsection cites real-burst stats: 134 timeout events with 133 recovered; 2026-08-18 burst recovered 27/27; recovered retries landing in 2.7s–95.3s versus 120s capped attempts. providers.ts docblock repeats PR #3625 four-attempts evidence at round=0 supporting hang vs slowness.

Documentation impact

• updated-in-pr — This PR intentionally ships documentation-only changes: an expanded decision-record docblock on DEFAULT_MODEL_TIMEOUT_MS in services/reviewer/src/providers.ts and a new settled-decision subsection plus corrected tuning guidance in services/reviewer/README.md. No executable behavior changed, and the docs now reflect the settled 120s timeout posture and corrected advice.
  Affected: services/reviewer/README.md, services/reviewer/src/providers.ts