fix(mt#4281): Record the timeout that was never recorded, and stop the SDK retrying blind

[minsky-ai]

Summary

mt#1897 has been open since 2026-05 asking a question its own substrate cannot answer: how often does runReview actually time out? This is that task's phase 1. It does not answer the question — it makes the question answerable. Two independent blind spots, both verified against main@b7a936316 rather than inherited from the parent spec.

Blind spot 1 — the unrecovered-timeout path persisted nothing

providers.ts pushed "timeout-unrecovered" onto a local array one statement before throw, so it died with the stack frame. That string appeared exactly once in the entire service — written, never read.

All three non-test recordReviewTiming call sites sit downstream of that throw:

site	path	reachable on an unrecovered timeout?
review-worker.ts:379	routing-skip (pre-model)	no
review-worker.ts:443	concurrent-inflight skip (pre-model)	no
review-finalize.ts:168	post-model, all four terminal paths	no — the throw goes past finalize

Measured consequence: 30 days of review_timing contained zero rows carrying it, while this repo produced at least five unrecovered timeouts on 2026-08-18 alone (PRs #3095, #3098, #3108, #3113, #3109). So every percentile in mt#1897 — including the p99 its "the cap is already 2× p99" conclusion rests on — is bounded to the recovered class. That bound is now annotated on the parent spec at the figures themselves.

Fixed by carrying the partial timing out on the error and writing one row at the runReview boundary. A non-enumerable symbol property rather than a field on TimeoutError, which lives in with-timeout.ts and is shared with merge-state-sweeper.ts — reviewer timing has no business in its shape. The boundary rethrows: this adds a row, it does not recover, and review_error still surfaces exactly as before.

Class-not-instance. The tool-loop catch prompted this, but callOpenAIWithClient has a second model-call site — the notools single-turn path — with its own withTimeout and its own success-only timing block, losing the same data the same way. It was found because the first draft of these tests omitted tools and silently took that branch. Both are fixed, and the test table now pins both so neither stands in for the other.

Blind spot 2 — the OpenAI SDK was retrying invisibly inside the 120s budget

The client was new OpenAI({ apiKey: config.providerApiKey }) — no timeout, no maxRetries.

Verified against the installed openai@4.104.0, not the master README, which documents v6. That mismatch is the point: the docs I first reached describe a different major version, so the installed source is the citation throughout.

• index.d.ts: [opts.maxRetries=2], [opts.timeout=10 minutes]
• core.js#shouldRetry: retries on 408, 409, 429, and any ≥ 500, plus connection errors
• index.d.ts line 49, unprompted: "Note that request timeouts are retried by default, so in a worst-case scenario you may wait much longer than this timeout before the promise succeeds or fails."

Two consequences. The SDK's own 10-minute timeout is unreachable — the 120s withTimeout wrapper always fires first, so APIConnectionTimeoutError is never observed. And a 429 retry chain was indistinguishable from one stuck call: up to three attempts plus backoff run inside a single chat.completions.create(), emitting nothing this service logs. From outside, the wrapper simply fires at 120s.

Both values are now pinned to what we were already inheriting, and a wrapping fetch logs any retryable response with the SDK's own attempt number — read off the x-stainless-retry-count header it already stamps (maxRetries - retriesRemaining), rather than derived. Absent header logs null rather than a fabricated number, because an invented attempt count reads as measured.

Explicitly not a tuning move. Retry policy is a Class B guarantee trade under the July 2026 cost audit, owned by mt#2718 and mt#3526, needing a measured before/after and principal sign-off. Pinning an inherited value is behaviour-preserving; changing it would not be. A test asserts the constants equal the defaults they replace, so a future change to either is a deliberate act rather than drift. The 10-minute timeout is deliberately left above the wrapper — lowering it would start it firing.

A figure this revises

The July 2026 cost audit computes "up to 24" calls per review, over the two application-level retry layers. The SDK layer sits underneath both and multiplies by up to 3, so the real worst-case HTTP attempt count is ~72. No concurrency or rate-limit reasoning should use 24 as an upper bound until the new instrumentation says otherwise.

Why this is a hypothesis-tester, not a hypothesis

Blind spot 2 is a strong candidate for the residual mt#1897 could not place — it predicts the three properties that survived that task's falsification round (failures at ~2× p99, i.e. off the distribution rather than in its tail; time-clustering, 15 of 25 on one day; a monotone rise with concurrency) and explains why status.openai.com read operational throughout, since per-key 429s are not a status-page incident. It is labelled inferred and is asserted nowhere in the code or the spec. This PR ships the instrumentation that can test it. The two facts it acts on are directly verified and each independently justifies the change.

Key Changes

• providers.ts — attachPartialTiming / extractPartialTiming; the TIMEOUT_UNRECOVERED constant; the carrier attached at both throw sites; createReviewerOpenAIClient with pinned timeout/maxRetries; withSdkRetryVisibility + readSdkAttempt; isSdkRetryableStatus verified against the installed core.js.
• review-timing.ts — recordUnrecoveredReviewTiming, the third timing shape. Token fields are omitted, not zeroed: a review that never returned usage has unknown spend, and zeroes would understate cost in the same aggregate the cost audit reads.
• review-worker.ts — the runReview boundary catch: record, then rethrow.
• unrecovered-timing.test.ts — new. Deliberately not in review-worker.test.ts, which open PR test(mt#1263): Add runReview sanitize wiring integration tests #774 rewrites (its only two files are that test and eslint.config.js; this PR touches neither).

No new review_timing column — retryOutcomes is already string[], so the contract-propagation gate does not fire.

Testing

Every seam is a real parameter — callOpenAIWithClient takes its client, recordUnrecoveredReviewTiming takes its timingRecorder, withSdkRetryVisibility takes its baseFetch — so no test here patches a module import.

Execution evidence:

$ bun --cwd services/reviewer test --preload ../../tests/setup.ts --timeout=15000 src/unrecovered-timing.test.ts
 39 pass
 0 fail
 63 expect() calls
Ran 39 tests across 1 file. [111.00ms]

$ bun --cwd services/reviewer test --preload ../../tests/setup.ts --timeout=20000
 2116 pass
 0 fail
 4628 expect() calls
Ran 2116 tests across 77 files. [4.59s]

Acceptance tests, by the spec's own numbering:

• AT1 (one row carrying timeout-unrecovered) — writes exactly ONE row carrying timeout-unrecovered.
• AT2 (negative control: the error still propagates) — NEGATIVE CONTROL: the error still propagates, on both call paths. A test asserting only the row would pass against an implementation that swallowed the failure, which would be worse than the bug.
• AT3 (no double-write) — structural, and stated as such rather than claimed as a test: the boundary catch rethrows, so control never reaches finalizeReviewSuccess / finalizeReviewError, and writeMainPathTiming cannot run on this path. The two writes are mutually exclusive by control flow.
• AT4 (partial latencies survive) — partial latencies are PERSISTED, not dropped to an empty array.
• AT5 (SDK retry visible, with a negative control) — a 429 ... is reported, reports the SDK's own 1-based attempt number, the FIRST attempt reads as 1, reports null rather than fabricating a number, plus NEGATIVE CONTROL: a clean 200 reports nothing. Without the last, an always-firing callback would pass the others and make every successful call look like a retry.
• AT6 (explicit client config) — PINS maxRetries and timeout instead of inheriting SDK defaults.

Negative control — tool-loop attach reverted to a bare throw err:

(fail) ... > tool-use loop > an unrecovered TimeoutError arrives carrying timeout-unrecovered
(fail) ... > tool-use loop > NEGATIVE CONTROL: a non-timeout throw records no timeout outcome
 34 pass
 2 fail

Exactly the two tool-loop tests fail while the notools ones still pass — so the paths are independently covered rather than one standing in for both. (The notools half got its control for free: it was discovered by those tests failing before it was fixed.)

Production wiring, caller direction — helper unit tests are not evidence of a caller:

$ grep -rn "recordUnrecoveredReviewTiming" services/reviewer/src/ --include='*.ts' | grep -v '\.test\.ts'
services/reviewer/src/review-timing.ts:84:export async function recordUnrecoveredReviewTiming(
services/reviewer/src/review-worker.ts:62:  recordUnrecoveredReviewTiming,
services/reviewer/src/review-worker.ts:885:    await recordUnrecoveredReviewTiming({

Typecheck: 0 errors across 8 projects. Lint: 0 errors, 0 warnings, 3783 files.

Live verification

UNVERIFIED — with the reason, per the rule that a runtime you did not attempt is a skipped step rather than a substitution.

The behaviour added here fires only on failures that cannot be induced on demand against production: an unrecovered 120s toolloop timeout, and a 429/5xx from OpenAI. There is no safe way to force either from a session, and forcing them would degrade the live reviewer for every other PR in flight. The substitute available — a stub transport and a stub client — is what the tests above use, and it validates the LOGIC, not its reachability in the real runtime. Note the negative control does not close this gap either: it proves the probe can fail, never that it is observing the right system.

Post-deploy both become observable without being inducible, and this is what to check:

1. select * from review_timing where 'timeout-unrecovered' = any(retry_outcomes) — expected to stay empty until the next unrecovered timeout, then become non-empty for the first time in the table's history. That transition is the real proof.
2. openai.sdk_retryable_response in the reviewer's runtime logs — expected on the next 429/5xx, carrying status and attempt.

Neither is a deploy-health signal, so neither is satisfied by the deploy succeeding.

Deploy verification

Touches deployed source under services/reviewer/src/**. After merge: deployment_wait-for-latest with notBefore set to the merge timestamp, then assert the health body's service identity is minsky-reviewer — not merely a 200 (mt#3148). No new external-system integration: same OpenAI API, same credential, no new scope, permission, or webhook, so gate (n)'s live-exercise class does not apply.

Scope boundary

Out of scope, with owners named: MAX_TOOL_ROUNDS and retry policy as cost levers (mt#2718; mt#3526 specifically for the round cap, where 83% of calls terminate at the 10-round limit); the Responses API migration (mt#1897's decide phase); Braintrust spans (mt#1885, which instruments only the success path and shares the blind spot this closes).

mt#2119 (per-review correlation ID) touches the same logging call sites — callReviewerWithRetry, callOpenAIWithClient, withTimeout — and is complementary rather than overlapping: it adds correlation to lines that already exist, this adds a line that did not. The new retry log is materially less useful without it when the whole problem is concurrent reviews interleaving.

[minsky-reviewer]

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 405K prompt, 7K completion | Duration: 158s
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

---

Strong work on closing the unrecovered-timeout blind spot and surfacing SDK retries; tests are thoughtful and negative controls are good. However, two blocking concerns need resolution before merge. (1) createReviewerOpenAIClient unconditionally defaults its baseFetch to the global fetch and always sets the client fetch option. In runtimes without a global fetch, this will throw at construction time; previously the SDK would use its own fetch. Make the wrapper conditional or supply a known fetch implementation. (2) attachPartialTiming uses Object.defineProperty on arbitrary thrown values; if the error object is frozen/non‑extensible, this will throw and mask the original failure on a hot path. Guard with Object.isExtensible or catch-and-fall-back so instrumentation can never interfere with error propagation. With these fixed, the PR should be ready to land.

Findings

• [BLOCKING] services/reviewer/src/providers.ts:1847 — createReviewerOpenAIClient hard-depends on global fetch — may throw at runtime in environments without it
  createReviewerOpenAIClient defaults its baseFetch parameter to the global fetch and always installs fetch: withSdkRetryVisibility(baseFetch, …) on the OpenAI client (services/reviewer/src/providers.ts:1847-1864). In Node runtimes without a global fetch (common pre-18, or when polyfills are absent in the deployed container), evaluating the default parameter will raise ReferenceError: fetch is not defined at client construction time. Previously the SDK supplied its own fetch internally when not overridden. Suggested fix: make baseFetch optional (no default), resolve const f = baseFetch ?? (globalThis.fetch as any | undefined), and only set the client's fetch option when f is defined; otherwise, allow the SDK's default to apply. Alternatively, import a known fetch implementation explicitly for the service runtime.
• [BLOCKING] services/reviewer/src/providers.ts:181 — attachPartialTiming may throw if the error object is non-extensible, masking the original failure
  attachPartialTiming unconditionally calls Object.defineProperty(err, PARTIAL_TIMING, …) (services/reviewer/src/providers.ts:181-207). If a thrown value is an object that is non-extensible (e.g., frozen or sealed by a dependency, or an SDK error that uses Object.freeze), defineProperty will throw a TypeError. In the model-call catch sites, this would raise a second error and mask the original cause, changing control flow on a hot path. Suggested fix: guard with if (!Object.isExtensible(err)) return err; or wrap the defineProperty in a try/catch that falls back to returning the original err on failure so instrumentation never interferes with propagation.

Spec verification

Criterion	Status	Evidence
An unrecovered TimeoutError on the model path persists exactly one review_timing row carrying retry_outcomes containing 'timeout-unrecovered', the partial per_round_latencies_ms accumulated before the throw, and the timeout_count. The error still propagates — this adds a record, it must not swallow the failure or alter control flow.	Met	services/reviewer/src/review-worker.ts:856-872 — wraps the model-call block in a try and on catch calls recordUnrecoveredReviewTiming(...) with partialTiming: extractPartialTiming(err) and then immediately throw err. TIMEOUT_UNRECOVERED is pushed in the model loop (providers.ts:1217-1224) and carried via attachPartialTiming. New test unrecovered-timing.test.ts asserts the negative control (error still propagates) and that timeout-unrecovered is present.
Exactly one row per failed review: the new write does not double-write with review-finalize.ts:168 on any path where both could run.	Met	The new write occurs in runReviewBody's catch (services/reviewer/src/review-worker.ts:856-872), which then rethrows. Because finalizeReviewSuccess/finalizeReviewError are only reached after the model returns (post-output), this catch prevents control from reaching any finalize path on throw. The test unrecovered-timing.test.ts verifies a single write via injected timingRecorder (no double-write observed).
The OpenAI client sets timeout and maxRetries explicitly rather than inheriting SDK defaults, so the retry budget is a stated value and not an inherited one. The chosen values are recorded with their reasoning; maxRetries is not silently left at 2.	Met	services/reviewer/src/providers.ts:1769-1810,1847-1864 — introduces OPENAI_SDK_MAX_RETRIES = 2 and OPENAI_SDK_TIMEOUT_MS = 10*60*1000 with a rationale block, and createReviewerOpenAIClient(...) passes them to new OpenAI({ maxRetries, timeout, ... }). callOpenAI now uses createReviewerOpenAIClient (providers.ts:1847-1854). Tests unrecovered-timing.test.ts assert the pinned values equal the defaults they replace.
An SDK-level retry is observable — a retry that happens inside a single chat.completions.create() call emits a log line or a counter naming the attempt number and the triggering status (429 / 5xx / connection). Today it emits nothing.	Met	services/reviewer/src/providers.ts:1695-1767 — adds withSdkRetryVisibility wrapper over fetch that checks isSdkRetryableStatus and logs openai.sdk_retryable_response including status, target, and a 1-based attempt from the SDK header (readSdkAttempt). createReviewerOpenAIClient installs this wrapper in the client fetch. Tests under SDK retry visibility in services/reviewer/src/unrecovered-timing.test.ts:216-383 verify 429 is reported, 200 is not, attempt numbers are interpreted correctly, and responses are passed through.
A 429 / rate-limit response is distinguishable in the record from a slow model call. These are opposite causes with opposite fixes, and the current instrumentation renders them identically.	Met	withSdkRetryVisibility logs retryable statuses (including 429) with an explicit event: "openai.sdk_retryable_response" and the status code (services/reviewer/src/providers.ts:1754-1764,1860-1864). This surfaces 429s distinctly in logs; combined with recordUnrecoveredReviewTiming adding 'timeout-unrecovered' to retryOutcomes, operator data can now separate slow calls (no retryable logs) from rate-limited ones (429 logs). Verified by tests unrecovered-timing.test.ts:270-297.
mt#1897's own percentile claims are annotated with the class they are bounded to (recovered-only), so the next reader does not inherit them as whole-population figures.	Unverifiable	The criterion refers to updating mt#1897's spec/doc. The repository diff cannot carry changes to that external spec, and the fetched reference for mt#1897 is TRUNCATED in this review context. I cannot verify from the diff whether those annotations were added. Marking Unverifiable per the Referenced Task Specs contract.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
providers.TIMEOUT_UNRECOVERED	function	services/reviewer/src/providers.ts:1217 — pushed into retryOutcomes on timeout, services/reviewer/src/unrecovered-timing.test.ts:32 — referenced in tests	Adopted	Constant used within provider implementation and tests; no external wiring required.

Documentation impact

• no-update-needed — No public API or user-facing docs changed; instrumentation and internal logging only. The change pins SDK defaults explicitly but does not alter behavior (tests assert parity). No docs under docs/ appear to cover reviewer provider internals; nothing to update or invalidate.

[minsky-reviewer]

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

---

Follow-up verification round: both prior BLOCKING findings persist in the current diff. (1) createReviewerOpenAIClient still defaults its baseFetch to the global fetch and always sets the ClientOptions.fetch, which will throw ReferenceError in runtimes without a global fetch rather than letting the SDK’s internal fetch apply. Make the wrapper conditional or resolve fetch from globalThis and omit the option when undefined. (2) attachPartialTiming still calls Object.defineProperty on arbitrary error objects; on non‑extensible (frozen/sealed) errors this will throw and mask the original failure. Guard with Object.isExtensible or wrap in try/catch and no-op on failure. Otherwise the spec criteria look met and tests are sound; address these two hot‑path robustness issues before merge.

Findings

• [BLOCKING] services/reviewer/src/providers.ts:1915 — createReviewerOpenAIClient still hard-depends on global fetch — ReferenceError risk remains
  createReviewerOpenAIClient(apiKey: string, baseFetch: FetchLike = fetch) still defaults its baseFetch parameter to the global fetch and unconditionally installs fetch: withSdkRetryVisibility(baseFetch, …) on the OpenAI client. In Node runtimes without a global fetch (common pre-18, or stripped/polyfill-missing images), evaluating the default parameter will throw ReferenceError: fetch is not defined at client construction time — a regression from allowing the SDK's internal fetch to apply when not overridden. This was the prior round's BLOCKING concern and is not addressed.

Suggested fix (one of):

• Make baseFetch optional (no default), resolve const f = (baseFetch ?? (globalThis as any).fetch) as FetchLike | undefined, and only set the client's fetch option when f is defined; otherwise, let the SDK default apply.
• Or import and pass a known fetch implementation for the service runtime.
• [BLOCKING] services/reviewer/src/providers.ts:187 — attachPartialTiming still unguarded — defineProperty on non‑extensible errors can throw and mask the original failure
  attachPartialTiming still unconditionally calls Object.defineProperty(err, PARTIAL_TIMING, …) when err is an object. If the thrown value is non‑extensible (frozen/sealed or an SDK error class that freezes instances), defineProperty will throw a TypeError. On the hot model-call catch path this would raise a second error and mask/replace the original timeout or HTTP error, altering control flow. The added tests only cover happy cases and do not exercise frozen errors.

Fix: make the carrier strictly non‑interfering by guarding or catching:

• Guard: if (!Object.isExtensible(err)) return err;
• Or wrap defineProperty in try/catch and fall back to returning the original err on failure. Add a test with attachPartialTiming(Object.freeze(new Error("boom")), timing) proving it does not throw.

Spec verification

Criterion	Status	Evidence
An unrecovered TimeoutError on the model path persists exactly one review_timing row carrying retry_outcomes containing 'timeout-unrecovered', the partial per_round_latencies_ms accumulated before the throw, and the timeout_count. The error still propagates — this adds a record, it must not swallow the failure or alter control flow.	Met	services/reviewer/src/review-worker.ts:856-885 — wraps the model-call block in a try/catch; on catch calls recordUnrecoveredReviewTiming({ …, partialTiming: extractPartialTiming(err) }) and then throw err. TIMEOUT_UNRECOVERED is pushed in the model loop (services/reviewer/src/providers.ts:1217-1224) and carried via attachPartialTiming (providers.ts:180-213).
Exactly one row per failed review: the new write does not double-write with review-finalize.ts:168 on any path where both could run.	Met	services/reviewer/src/review-worker.ts:856-885 — the boundary catch records and immediately rethrows, preventing control from reaching any finalize path; finalize writes are post-model only. New tests in services/reviewer/src/unrecovered-timing.test.ts assert a single write via injected recorder.
The OpenAI client sets timeout and maxRetries explicitly rather than inheriting SDK defaults, so the retry budget is a stated value and not an inherited one. The chosen values are recorded with their reasoning; maxRetries is not silently left at 2.	Met	services/reviewer/src/providers.ts:1859-1921 — introduces OPENAI_SDK_MAX_RETRIES and OPENAI_SDK_TIMEOUT_MS with rationale and passes them in createReviewerOpenAIClient(), which is then used by callOpenAI (providers.ts:1961-1972). Tests under createReviewerOpenAIClient in services/reviewer/src/unrecovered-timing.test.ts:333-394 pin parity.
An SDK-level retry is observable — a retry that happens inside a single chat.completions.create() call emits a log line or a counter naming the attempt number and the triggering status (429 / 5xx / connection). Today it emits nothing.	Met	services/reviewer/src/providers.ts:1768-1858 — adds withSdkRetryVisibility and readSdkAttempt; createReviewerOpenAIClient installs it and logs openai.sdk_retryable_response with status/target/attempt. Tests SDK retry visibility in services/reviewer/src/unrecovered-timing.test.ts:216-333 verify 429 is logged, 200 is not, attempts are interpreted, and pass-through behavior.
A 429 / rate-limit response is distinguishable in the record from a slow model call. These are opposite causes with opposite fixes, and the current instrumentation renders them identically.	Met	providers.ts:1813-1836 — withSdkRetryVisibility calls the logger only for retryable statuses (incl. 429). Combined with timeout-unrecovered persistence, operators can now disambiguate slow/no-error from explicit 429/5xx bursts. Verified by tests at unrecovered-timing.test.ts:270-297.
mt#1897's own percentile claims are annotated with the class they are bounded to (recovered-only), so the next reader does not inherit them as whole-population figures.	Unverifiable	This is an external spec/doc change to mt#1897. The repository diff cannot carry it, and the fetched mt#1897 reference in this review is TRUNCATED; cannot verify from code. Reporting Unverifiable per Referenced Task Specs contract.

Documentation impact

• no-update-needed — Instrumentation-only change: persists a previously-lost timeout outcome and logs SDK retryable responses. No public API or user-facing behavior is added or removed; OpenAI client settings are pinned to current defaults (parity asserted by tests). No docs in this repo describe provider internals; nothing to update or invalidate.

[minsky-reviewer]

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

---

Re-verification complete. The prior BLOCKING issues were addressed by the latest commit: (1) attachPartialTiming now safely guards against non-extensible errors and catches defineProperty failures, ensuring the original error is never masked; (2) createReviewerOpenAIClient no longer hard-depends on a bare global fetch, resolving via globalThis and allowing the SDK to supply its own shims. The unrecovered-timeout path now carries partial timing out on the error, runReview records it and rethrows, and the OpenAI client’s timeout/maxRetries are explicitly pinned with added SDK-retry visibility via a wrapped fetch. Tests exercise both call paths and the retry-visibility behavior. I find no new critical defects introduced by these fixes. Event: APPROVE.

Spec verification

Criterion	Status	Evidence
An unrecovered TimeoutError on the model path persists exactly one review_timing row carrying retry_outcomes containing 'timeout-unrecovered', the partial per_round_latencies_ms accumulated before the throw, and the timeout_count. The error still propagates — this adds a record, it must not swallow the failure or alter control flow.	Met	services/reviewer/src/review-worker.ts:532-562 — runReviewBody wraps model call and, on catch, calls recordUnrecoveredReviewTiming({... partialTiming: extractPartialTiming(err) ...}) then throw err; services/reviewer/src/providers.ts:154-224, 246-267 — attachPartialTiming + extractPartialTiming carrier; services/reviewer/src/providers.ts:1246-1255 — toolloop catch stamps TIMEOUT_UNRECOVERED and attaches partial timing; services/reviewer/src/unrecovered-timing.test.ts:112-210, 247-324 — tests assert carriage, propagation, and partial latencies.
Exactly one row per failed review: the new write does not double-write with review-finalize.ts:168 on any path where both could run.	Met	services/reviewer/src/review-worker.ts:532-562 — boundary catch records timing then throw err; control does not proceed to finalizeReview* paths below; acceptance test in services/reviewer/src/unrecovered-timing.test.ts:172-210 asserts error still propagates to caller (no finalize), ensuring mutual exclusivity by control flow.
The OpenAI client sets timeout and maxRetries explicitly rather than inheriting SDK defaults, so the retry budget is a stated value and not an inherited one. The chosen values are recorded with their reasoning; maxRetries is not silently left at 2.	Met	services/reviewer/src/providers.ts:1738-1771 — OPENAI_SDK_MAX_RETRIES = 2, OPENAI_SDK_TIMEOUT_MS = 10*60*1000 with rationale comment; services/reviewer/src/providers.ts:1918-1965 — createReviewerOpenAIClient passes maxRetries and timeout explicitly; services/reviewer/src/providers.ts:1966-1973 — call site switches to createReviewerOpenAIClient; services/reviewer/src/unrecovered-timing.test.ts:439-458 asserts pinned values on the constructed client and that constants match SDK defaults.
An SDK-level retry is observable — a retry that happens inside a single chat.completions.create() call emits a log line or a counter naming the attempt number and the triggering status (429 / 5xx / connection). Today it emits nothing.	Met	services/reviewer/src/providers.ts:1855-1916 — withSdkRetryVisibility wraps fetch and calls callback on retryable statuses; services/reviewer/src/providers.ts:1989-2010 — client installs wrapped fetch that logs openai.sdk_retryable_response with status, target, and attempt (1-based from x-stainless-retry-count parsed by readSdkAttempt at 1800-1852); services/reviewer/src/unrecovered-timing.test.ts:330-420 asserts 429 is reported, 200 is not, and attempt parsing behavior.
A 429 / rate-limit response is distinguishable in the record from a slow model call. These are opposite causes with opposite fixes, and the current instrumentation renders them identically.	Met	services/reviewer/src/providers.ts:1989-2010 — logs openai.sdk_retryable_response including status and 1-based attempt; services/reviewer/src/unrecovered-timing.test.ts:330-388 verifies 429 emits a signal while 200 does not, making rate-limit retries observable vs. slow-but-clean calls.
mt#1897's own percentile claims are annotated with the class they are bounded to (recovered-only), so the next reader does not inherit them as whole-population figures.	Unverifiable	This criterion refers to annotating mt#1897’s spec outside this repo diff. The Referenced Task Specs section shows mt#1897 content was fetched but TRUNCATED; whether the annotation landed cannot be confirmed from the current diff. Therefore this is Unverifiable from in-repo changes alone.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
createReviewerOpenAIClient	function	services/reviewer/src/providers.ts:1966 — used at call site to construct the OpenAI client	Adopted	Exported for reuse and tests; primary production consumer is the call site in the same module. Tests also construct clients via this helper for verification.

Documentation impact

• no-update-needed — Behavioral changes are internal instrumentation and SDK client configuration being made explicit. No user-facing routes, flags, or CLI changed. I scanned code comments and test names for any new externally-observable behavior beyond logs and timing persistence; none require docs updates.

[minsky-reviewer]

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

---

Re-verification complete. The prior BLOCKING issues called out in earlier rounds were addressed: attachPartialTiming now guards non-extensible errors and never masks the original failure; createReviewerOpenAIClient no longer hard-depends on a bare global fetch and cleanly defers to SDK shims when absent. The unrecovered-timeout path now carries partial timing on the error, runReview records it and rethrows, and SDK retry visibility is added via a wrapped fetch with pinned timeout/maxRetries matching current defaults. Tests exercise both call paths and retry-visibility behavior. I find no new critical defects introduced by these fixes. Event: APPROVE.

Spec verification

Criterion	Status	Evidence
An unrecovered TimeoutError on the model path persists exactly one review_timing row carrying retry_outcomes containing 'timeout-unrecovered', the partial per_round_latencies_ms accumulated before the throw, and the timeout_count. The error still propagates — this adds a record, it must not swallow the failure or alter control flow.	Met	services/reviewer/src/providers.ts:1248-1261 — on timeout, pushes TIMEOUT_UNRECOVERED and throws attachPartialTiming(err, { roundLatenciesMs, timeoutCount, retryOutcomes }); services/reviewer/src/review-worker.ts:860-898 — boundary catch calls recordUnrecoveredReviewTiming({... partialTiming: extractPartialTiming(err) ...}) then throw err (propagates error).
Exactly one row per failed review: the new write does not double-write with review-finalize.ts:168 on any path where both could run.	Met	services/reviewer/src/review-worker.ts:860-898 — row is written inside the catch and the error is immediately rethrown; since finalize paths execute only on success, double-write is structurally excluded.
The OpenAI client sets timeout and maxRetries explicitly rather than inheriting SDK defaults, so the retry budget is a stated value and not an inherited one. The chosen values are recorded with their reasoning; maxRetries is not silently left at 2.	Met	services/reviewer/src/providers.ts:1732-1758 — OPENAI_SDK_MAX_RETRIES = 2, OPENAI_SDK_TIMEOUT_MS = 10*60*1000 with rationale; services/reviewer/src/providers.ts:1891-1907 — createReviewerOpenAIClient applies pinned values; services/reviewer/src/providers.ts:1918-1924 — callsite switched to createReviewerOpenAIClient.
An SDK-level retry is observable — a retry that happens inside a single chat.completions.create() call emits a log line or a counter naming the attempt number and the triggering status (429 / 5xx / connection). Today it emits nothing.	Met	services/reviewer/src/providers.ts:1824-1889 — withSdkRetryVisibility wraps fetch and on retryable statuses logs openai.sdk_retryable_response with status, target, and 1-based attempt read via readSdkAttempt; services/reviewer/src/providers.ts:1889-1907 — wrapper installed on OpenAI client.
A 429 / rate-limit response is distinguishable in the record from a slow model call. These are opposite causes with opposite fixes, and the current instrumentation renders them identically.	Met	services/reviewer/src/providers.ts:1865-1889 — wrapper logs status and attempt only on retryable statuses (including 429); services/reviewer/src/providers.ts:1798-1822 — isSdkRetryableStatus mirrors SDK logic so slow 200s are silent; tests at services/reviewer/src/unrecovered-timing.test.ts:330-379 assert 429 logs and 200 does not.
mt#1897's own percentile claims are annotated with the class they are bounded to (recovered-only), so the next reader does not inherit them as whole-population figures.	Unverifiable	Referenced Task Specs → mt#1897 is TRUNCATED; while it contains a CORRECTION section, the full spec (and the specific figure annotations) are cut. Cannot confirm annotation placement from this diff.

Documentation impact

• no-update-needed — The PR adds internal observability and persistence: recording unrecovered timeouts to review_timing and logging SDK retry attempts. No user-facing APIs, commands, or documented behavior changed; no files under docs/ were modified. Checked code paths: services/reviewer/src/providers.ts, review-worker.ts, and review-timing.ts — all internal service logic.