fix(llm-client): retry Cloudflare edge timeouts 522 and 524

[drewstone]

The retry classifier's RETRYABLE_STATUS set stopped at {429, 502, 503, 504}. Router deployments serve through Cloudflare, whose edge returns 522 (origin connect timeout) and 524 (origin response timeout) for exactly the transient class 504 covers. Measured cost: a GEPA benchmark run died at its baseline phase from one 524 on a long non-streaming glm call — the error escaped the retry loop and killed the comparison.

This adds 522 and 524 to the set and a focused classification test (retryable gateway statuses, the two Cloudflare timeouts, deterministic 4xx, and the foreign-error numeric-status path).

Verified: 19/19 in tests/llm-transient-status.test.ts + tests/judge-retry.test.ts, typecheck clean.

🤖 Generated with Claude Code

[tangletools]

✅ Auto-approved drewstone PR — fa8469b6

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

_{tangletools · auto-approval · reason: drewstone_author · 2026-08-19T06:27:15Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Coverage	2 of 2 lenses (value, usefulness)
Concerns	2 (2 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	134.3s (2 bridge agents)
Total	134.3s

💰 Value — sound-with-nits

Adds Cloudflare edge timeouts 522/524 to the LLM retry allowlist (with tests) — correct, minimal, in-grain; only stale docstrings that still enumerate the old set.

• What it does: Adds HTTP 522 and 524 to RETRYABLE_STATUS in src/llm-client.ts:355, so isTransientLlmError (and thus both callLlm's retry loop and withJudgeRetry, which share that classifier) treats Cloudflare edge origin-connect/response timeouts as transient and retries them instead of surfacing them as fatal. Adds tests/llm-transient-status.test.ts covering gateway statuses, the two Cloudflare codes, d
• Goals it achieves: Makes a transient Cloudflare edge timeout (522/524) recoverable instead of killing the run. The classifier is the single shared retry predicate (src/llm-client.ts:393 isTransientLlmError; judge-retry.ts:84 defaults to it), so one set addition fixes every LLM call path — the observed GEPA baseline death on a long non-streaming glm call would now retry and succeed.
• Assessment: Good. It is exactly in the grain: the allowlist was already an explicit Set, so extending it (not switching to a range) preserves the conservative intent of not retrying ambiguous 5xx like 500/501/505. 522/524 genuinely map to the same transient origin-timeout class as 504, and the test asserts the deterministic-4xx boundary holds. Scope is proportionate: one constant, one focused test file.
• Better / existing approach: Searched for an existing retry-status classifier: src/adapters/http.ts:146 and src/hosted/client.ts:135 use a range classifier status >= 500 || 408 || 429, which already covers 522/524 — but those are separate sandbox/hosted HTTP dispatch paths, not the LLM retry classifier, and they are not reusable here without changing the explicit-allowlist design that callLlm/judge-retry deliberately share.
• Model: opencode/deepseek/deepseek-v4-pro
• Bridge attempts: 3
• Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error; opencode/zai-coding-plan/glm-5.2: opencode: opencode error

🎯 Usefulness — sound

A two-element data change to the single retry classifier that every LLM call path in the package already routes through, fixing a measured benchmark-killing failure; no new surface, no competing mechanism.

• Integration: Immediately and fully reachable. RETRYABLE_STATUS is consumed at the inline !res.ok retry branch of callLlm (src/llm-client.ts:724), inside classifyTransient for both the LlmCallError path (src/llm-client.ts:398) and the foreign-error numeric-status path (src/llm-client.ts:403); isTransientLlmError is exported from the package entry (src/index.ts:937) and is the default isRetryable for withJudgeRe
• Fit with existing patterns: Extends the established pattern in place — one Set, consulted by the one classifier, documented as the package-wide retry authority in the isTransientLlmError docstring (src/llm-client.ts:382-392). No parallel retry mechanism is introduced; the comment explaining 522/524 as the Cloudflare-edge analog of 504 matches the file's existing comment style (compare the TRANSIENT_ERROR_PATTERNS rationale a
• Real-world viability: A Set literal change with no new state or control flow; concurrency-safe by construction. New statuses flow through the existing guards that make retry safe under pressure: maximumAttempts cap (default 3, src/llm-client.ts:335-346), retry-after header parsing (src/llm-client.ts:729-730), backoff ceiling (src/llm-client.ts:425-427), and the cross-attempt deadlineMs check (src/llm-client.ts:726). Wo
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

🎯 Usefulness Audit

🟡 Sibling Python client keeps its own retry set without 522/524 [problem-fit] ``

clients/python/src/agent_eval_rpc/hosted.py:278 defines _RETRYABLE_STATUSES = {408, 429, 500, 502, 503, 504} in its HTTP retry loop (line 429). If the hosted RPC endpoint also sits behind the same Cloudflare edge, the exact failure this PR fixes (one 522/524 escaping the retry loop) still kills Python wire-protocol clients. Not a blocker for this PR — it fixes the substrate where the failure was measured — but a one-line follow-up in the Python client would close the same class there.

💰 Value Audit

🟡 Two docstrings still enumerate the old {429,502,503,504} set [maintenance] ``

src/judge-retry.ts:36 documents any LlmCallError with status in {429,502,503,504} and src/llm-client.ts:5 header says retry on 429 + 5xx gateway errors (502/503/504); both are now stale and understate the retryable set this change extends. The new comment at src/llm-client.ts:352-354 is correct, but the change should have refreshed the two pre-existing enumerations so no doc names an outdated set.

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260819T062957Z}

[tangletools]

✅ No Blockers — fa8469b6

Review health 100/100 · Reviewer score 83/100 · Confidence 70/100 · 4 findings (4 low)

	opencode GLM 5.2	opencode DeepSeek v4 Pro	verifier:deepseek-flash	aggregate
Readiness	83	89	92	83
Confidence	70	70	95	70
Correctness	83	89	95	83
Security	83	89	100	83
Testing	83	89	88	83
Architecture	83	89	92	83

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision. | Verified the core change: RETRYABLE_STATUS at src/llm-client.ts:355 now includes 522/524, and both consumers (classifier at :398/:403 and the callLlm retry loop at :724) read the same constant, so behavior is consistent and bounded by deadlineMs at :726. All 7 reviewer findings are low-severity and accurate; the two overlaps (doc drift, test duplicatio

🟡 LOW Deterministic slow-origin 524 now burns the full attempt budget — src/llm-client.ts

A call that legitimately exceeds the Cloudflare origin window will hit 524 on every attempt, so failure now takes up to 3 x DEFAULT_TIMEOUT_MS + backoff and can re-trigger origin-side generation work up to 3 times, versus failing on attempt 1 before. Mitigations exist (deadlineMs cross-attempt budget, maximumAttempts, TANGLE_LLM_TIMEOUT_MS) and the same tradeoff was already accepted for 504, so this is an informational tradeoff, not a regression.

🟡 LOW File-header doc still enumerates retry statuses as (502/503/504) — src/llm-client.ts

The module docblock says 'Exponential-backoff retry on 429 + 5xx gateway errors (502/503/504)' while RETRYABLE_STATUS at line 355 now includes 522/524. Cosmetic drift inside the changed file. Fix: update the parenthetical to '(502/503/504/522/524)' or drop the enumeration so it cannot drift again.

🟡 LOW Classifier coverage duplicates four assertions already in the colocated suite — tests/llm-transient-status.test.ts

The 429/503 true and 400/401 false cases duplicate src/llm-client.test.ts:970-974. The PR's actual delta (522/524, foreign .status, 502/504, 404/422) is additive and correct, so this is redundancy, not a defect. Optional: merge the new cases into the existing 'llm-client — isTransientLlmError classification' describe block.

🟡 LOW No retry-loop-level test that a 522 response triggers an actual retry — tests/llm-transient-status.test.ts

All cases test the classifier in isolation. The callLlm retry loop (src/llm-client.ts:723-732) reads the same RETRYABLE_STATUS constant the classifier uses, so behavior is transitively pinned, but a two-response mock (522 then 200 via the existing mockFetch helper in src/llm-client.test.ts:362) would lock the end-to-end retry behavior against future loop refactors that bypass the constant.

---

_{tangletools · 2026-08-19T06:35:10Z · trace}

[tangletools]

✅ Approved — 4 non-blocking findings — fa8469b6

Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision. | Verified the core change: RETRYABLE_STATUS at src/llm-client.ts:355 now includes 522/524, and both consumers (classifier at :398/:403 and the callLlm retr

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-08-19T06:35:10Z · immutable trace}

[drewstone]

@tangletools review now

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Coverage	2 of 2 lenses (value, usefulness)
Concerns	4 (4 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	172.7s (2 bridge agents)
Total	172.7s

💰 Value — sound-with-nits

Adds Cloudflare edge timeouts 522/524 to the LLM retry-status set (plus a callback 500 diagnostic) — correct, minimal, and in the right shared spot.

• What it does: Two small fixes: (1) adds HTTP 522/524 (Cloudflare origin connect/response timeouts) to the RETRYABLE_STATUS set in src/llm-client.ts:355 that the retry classifier consults, so a long non-streaming LLM call that draws a 522/524 from the edge is retried like a 504 instead of escaping the loop; (2) changes the external-optimizer callback's evaluation-failed branch from catch {} to catch (error)
• Goals it achieves: Prevents a single transient Cloudflare edge timeout from killing a whole benchmark/eval run at its baseline phase — the failure is retried with backoff rather than surfacing as a fatal error. The callback change makes a failed evaluation's 500 self-describing, so the caller gets the thrown detail instead of a blind debugging round. Both serve the repo's 'fail loud, no silent zeros' doctrine.
• Assessment: Sound and in-grain. RETRYABLE_STATUS is the single shared classifier: callLlm retries off it directly (src/llm-client.ts:724) and withJudgeRetry defaults to isTransientLlmError (src/judge-retry.ts:84), which resolves to the same set (src/llm-client.ts:398). One-line extension therefore propagates to every retry path with no new machinery, and 522/524 are genuinely transient — the exact class
• Better / existing approach: None materially better — the change correctly extends the existing shared classifier rather than reinventing one, and an explicit set is the right grain (a >=500 range check would over-retry 501/505/521). I checked for an existing equivalent: the only other retry-status constant is _RETRYABLE_STATUSES = frozenset({408,429,500,502,503,504}) in clients/python/src/agent_eval_rpc/hosted.py:278, bu
• Model: opencode/deepseek/deepseek-v4-pro
• Bridge attempts: 3
• Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error; opencode/zai-coding-plan/glm-5.2: opencode: opencode error

🎯 Usefulness — sound

A two-line fix to the package's single LLM retry classifier that puts Cloudflare edge timeouts 522/524 on the same retried path as 504, plus a diagnostic improvement to the external-optimizer callback's failure response — both live on paths the PR's own incident exercised.

• Integration: Fully reachable now, not ahead of a caller. RETRYABLE_STATUS gates two live retry points in callLlm (src/llm-client.ts:723-732 for the !res.ok branch, src/llm-client.ts:907-914 for the catch loop) and is the default classifier for withJudgeRetry (src/judge-retry.ts:84, consumed by src/judge-panel.ts:138), so every LLM call — judges, campaigns, GEPA/skillopt evaluation — picks up 522/524 retry imme
• Fit with existing patterns: Extends the established pattern in grain: the explicit RETRYABLE_STATUS set with a rationale comment is how this classifier has always grown (src/llm-client.ts:352-355), and isTransientLlmError is documented as the package-wide retry classifier (src/llm-client.ts:389-391). The sibling status loops in src/hosted/client.ts:135 and src/adapters/http.ts:146 serve different surfaces (hosted service, wi
• Real-world viability: 522/524 are the same transient class as 504, and the retry path honors retry-after, backoff, attempt caps, and the deadline guard, so a repeated edge timeout still terminates rather than hanging a campaign — the same guarantees 504 already gets. The foreign-error numeric-status path (src/llm-client.ts:402-403) is also covered, so errors crossing a non-LlmCallError boundary classify correctly. The
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

🎯 Usefulness Audit

🟡 Fix covers 522/524 but not the rest of Cloudflare's 52x/edge family [robustness] ``

The PR's own rationale (router deployments serve through Cloudflare, so edge statuses replace origin statuses) applies equally to 520, 521, 523, and 525; those still fall out of RETRYABLE_STATUS (src/llm-client.ts:355) and would kill a benchmark run exactly like the measured 524 did. The repo's two sibling retry loops already use the broader res.status >= 500 rule (src/hosted/client.ts:135, src/adapters/http.ts:146). Either add the remaining Cloudflare codes or leave as-is since only 522/524 w

💰 Value Audit

🟡 Two doc comments now understate the retryable status set [maintenance] ``

src/judge-retry.ts:36 documents the default classifier as 'any LlmCallError with status in {429,502,503,504}' and src/llm-client.ts:5 lists '(502/503/504)'. Both omit 522/524 now. Given this repo's doc-discipline, update both to keep the enumerated set accurate.

🟡 New test file partially overlaps the existing classifier test block [maintenance] ``

tests/llm-transient-status.test.ts restates status classification already covered by describe('isTransientLlmError classification') in src/llm-client.test.ts:970-975 (429/503/400/401). It does add real new coverage (502, 504, 522, 524, 404, 422, and the foreign-error numeric-status path), so it is not redundant — but extending the existing block instead of a parallel file would keep one home for classifier tests.

🟡 Two unrelated fixes bundled under one retry PR [proportion] ``

The callback 500-diagnostic change (commit d947bd0) is a separate concern from the 522/524 retry fix (fa8469b) and is not mentioned in the PR title/body. Both are small and coherent so this does not gate shipping, but they read as one PR about two things; splitting would keep the retry fix reviewably atomic.

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260819T064210Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Coverage	2 of 2 lenses (value, usefulness)
Concerns	2 (2 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	204.1s (2 bridge agents)
Total	204.1s

💰 Value — sound-with-nits

Extends the single package-wide retry-status set with Cloudflare edge timeouts 522/524 (plus a bounded error-detail improvement in the optimizer callback) — a minimal, in-grain fix verified passing 25/25 locally; only doc-string staleness and a partial Cloudflare-family question remain.

• What it does: Two deltas. (1) Adds HTTP 522 and 524 to RETRYABLE_STATUS (src/llm-client.ts:355), the one classifier used by callLlm's inline retry (src/llm-client.ts:724) and isTransientLlmError (src/llm-client.ts:398), which withJudgeRetry defaults to (src/judge-retry.ts:84) — so both retry loops now treat Cloudflare edge timeouts like 504. (2) The external-optimizer loopback callback now returns the thrown er
• Goals it achieves: Long non-streaming LLM calls served through Cloudflare-fronted router deployments survive transient edge timeouts instead of dying on first occurrence — protecting multi-hour benchmark/optimization runs from a single 522/524. Secondarily, when an evaluation inside the optimizer callback does fail, the optimizer process sees the cause without a blind debugging round. Matches the repo's fail-loud do
• Assessment: Good on its merits. The codebase already funnels every LLM retry decision through one Set; extending it by two members is the minimal, correct-grain fix — no parallel classifier created, and the judge path inherits the fix for free via withJudgeRetry's default predicate. The new classification test (tests/llm-transient-status.test.ts) covers the retryable set, the two new statuses, deterministic 4
• Better / existing approach: None — this is the right approach. Searched for existing equivalents: src/hosted/client.ts:135 and src/adapters/http.ts:146 use 'status >= 500 || 408 || 429' but govern different transports (hosted ingest, HTTP dispatch) and already cover 522/524 — nothing to reuse for the LLM path and nothing duplicated. One could argue for switching RETRYABLE_STATUS to a '>= 500' predicate to match those two and
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

Adds the two Cloudflare edge timeout statuses (522/524) to the existing retryable-status set and surfaces the thrown detail in a failed-evaluation 500 body — both coherent, in-grain, and already reachable by live callers.

• Integration: Reachable through every existing retry path. RETRYABLE_STATUS is the single source of truth: it is checked directly on the HTTP status in callLlm's !res.ok branch (src/llm-client.ts:724) and again via isTransientLlmError (src/llm-client.ts:398,403), which the catch-loop retry (src/llm-client.ts:909) and withJudgeRetry (src/judge-retry.ts:84, wired through judge-panel.ts:138) both call. No dead sur
• Fit with existing patterns: Fits the existing pattern exactly: a single RETRYABLE_STATUS set, extended in place, with a focused classifier test mirroring the existing isTransientLlmError describe block in src/llm-client.test.ts:946. The doc comment on the set and the package header already name 429 + 5xx gateway errors as retryable, so 522/524 are a natural widening, not a competing mechanism. The callback 500-body change al
• Real-world viability: Holds up. 522 (origin connect timeout) and 524 (origin response timeout) are genuinely transient Cloudflare edge conditions and the same class as 504, so backoff-and-retry is correct behavior, not a happy-path-only fix. The foreign-error numeric-status path (src/llm-client.ts:402-403) already decodes a bare status on non-LlmCallError transports, so the test's 'edge timeout' case reflects a real pa
• Model: opencode/deepseek/deepseek-v4-pro
• Bridge attempts: 3
• Bridge warning: opencode/zai-coding-plan/glm-5.2: opencode: opencode error; opencode/kimi-for-coding/k2p7: opencode: opencode error

💰 Value Audit

🟡 Two docstrings still enumerate the old {429,502,503,504} set [maintenance] ``

src/judge-retry.ts:36 documents the default predicate as retrying 'any LlmCallError with status in {429,502,503,504}' and the src/llm-client.ts:5 header says '429 + 5xx gateway errors (502/503/504)'. Both now contradict the code. Trivial follow-up: update both to mention 522/524, or drop the enumeration in favor of pointing at RETRYABLE_STATUS so it cannot drift again.

🟡 Cloudflare edge family only partially covered [better-architecture] ``

Cloudflare also emits 520 (origin returned unknown error), 521 (origin down), 523, 525 — all plausibly transient for a proxied origin, and the sibling classifiers at src/hosted/client.ts:135 and src/adapters/http.ts:146 retry them via 'status >= 500'. The PR's own argument (transient exactly like 504) applies most strongly to 522/524, so the scoped fix is defensible, but if another edge status kills a run the same one-line extension will be needed. Not a blocker; note for the reviewer.

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260819T064523Z}

[tangletools]

✅ No Blockers — d947bd0c

Review health 100/100 · Reviewer score 65/100 · Confidence 80/100 · 22 findings (22 low)

	opencode GLM 5.2	opencode DeepSeek v4 Pro	opencode DeepSeek v4 Flash	aggregate
Readiness	65	83	68	65
Confidence	80	80	80	80
Correctness	65	83	68	65
Security	65	83	68	65
Testing	65	83	68	65
Architecture	65	83	68	65

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision.

🟡 LOW Durable refusal artifact still lacks the failure detail — src/campaign/external-optimizer-callback.ts

The observe({kind:'refusal', reason:'evaluation-failed', ...}) record carries no error detail, while the diagnostic now lives only on the transient HTTP response. Per the repo's experiment doctrine (refusals live inside artifacts), if the optimizer child discards the 500 body the retained artifact still cannot explain the failure. The refusal variant of ExternalOptimizerEvaluationObservation (external-optimizer-contracts.ts:224-231) already uses optional fields, so an optional detail field is an additive follow-up. Not a defect in this diff's intent.

🟡 LOW Harness-internal error detail is relayed to the benchmarked external optimizer over the wire — src/campaign/external-optimizer-callback.ts

The 500 body now embeds up to 400 chars of the harness's own evaluate() error. The recipient holds the bearer token (the GEPA/SkillOpt subprocess the harness spawned with callbackUrl/callbackToken), so the boundary is loopback+token, but the external optimizer is the experiment's subject, not a trusted party. If an evaluate() rejection message embeds a credential (provider URL with embedded key, traceback with env var names), it is disclosed to that subprocess. The 400-char slice bounds but does not redact. Consider redacting common secret patterns before sending, or documenting the trust boundary. Also note the durable observe() refusal record ([line 183-189](https://github.com/tangle-network/agent-eval/blob/d947bd0c94ab2985c4155ee14aa273e73e99067d/src/campaign/external-optimizer-callback

🟡 LOW No test covers the 400-char cap or non-Error throws — src/campaign/external-optimizer-callback.ts

The new lifecycle test asserts the happy path (Error with message appears in body, status 500) and passes. The truncation bound, a thrown non-Error value, and a thrown null/undefined/Symbol are untested. Cheap to add alongside the existing 'carries the thrown detail' test; low risk since all these paths degrade safely, but they are the exact surface the change introduces.

🟡 LOW Non-Error throws produce useless diagnostics — src/campaign/external-optimizer-callback.ts

If evaluate throws undefined or a plain object, String(error) yields 'undefined' or '[object Object]', defeating the diagnostic the change adds. Prefer error instanceof Error ? error.message : String(error) (or a small format helper). Not a regression — the prior code dropped the detail entirely — but the intended value is lost for non-Error throws.

🟡 LOW String(error) degrades for non-Error thrown values and can throw on a Symbol — src/campaign/external-optimizer-callback.ts

String(Symbol()) throws TypeError, which escapes the inner catch into the outer catch (line 231) and yields the opaque 'evaluation failed' — the exact outcome this PR removes. A thrown plain object stringifies to '[object Object]', losing the diagnostic entirely. Both paths are graceful (no crash, outer 500), but they partially defeat the fix's intent for edge cases. Prefer error instanceof Error ? error.message : String(error) before the slice. Also, slice(0,400) cuts by UTF-16 code unit and can split a surrogate pair (JSON stays valid because JSON.stringify escapes it); cosmetic only.

🟡 LOW Thrown detail echoed to caller without scrubbing — src/campaign/external-optimizer-callback.ts

sendJsonIfOpen(response, 500, { error: evaluation failed: ${String(error).slice(0, 400)} }) forwards the raw message of whatever args.evaluate throws. String(error) omits the stack (good), but the message can embed candidate text, downstream origin URLs, or secrets if the host's evaluate wraps an LLM/router error chain. Surface is loopback-only (listenLocal binds 127.0.0.1) and bearer-authenticated, so practical risk is low and this is the PR's stated intent. If evaluate may be third-party-supplied or wrap untrusted errors, scrub known-sensitive fields before echoing.

🟡 LOW Thrown detail forwarded without redaction — src/campaign/external-optimizer-callback.ts

String(error) can embed provider URLs, keys, or filesystem paths from whatever evaluate throws; the message forwards them verbatim. Exposure is bounded: loopback-only bind (external-optimizer-http.ts:6 binds 127.0.0.1) and Bearer-token auth checked before evaluate runs (line 140). Note the sibling model-call contract explicitly marks error text 'safe to retain and return to the child process' (external-optimizer-contracts.ts:152-153), so the codebase treats returned error text as a trust decision; this path skips that filter. Acceptable in the loopback+token trust domain.

🟡 LOW Truncation and non-Error throwables untested — src/campaign/external-optimizer-callback.ts

The added test covers only the Error-instance happy path. The 400-char slice cap and String() of a non-Error throw (e.g. object whose toString throws, which escapes to the outer catch at line 231 and degrades to the old opaque 500) are uncovered. Minor test gap; behavior is safe by inspection.

🟡 LOW 524 retries can triple wall-clock on a genuinely slow origin — src/llm-client.ts

524 means the origin did not answer within Cloudflare's edge window (~100s); if the model is legitimately slow rather than the edge connection being flaky, each retry re-hits the same 100s edge ceiling, so 3 attempts ≈ 300s + backoff before the final throw instead of failing at ~100s. Acceptable and consistent with the existing 504/timeout policy; callers that want fail-fast should lower maximumAttempts or set deadlineMs. Not a blocker — noted so the tradeoff is explicit.

🟡 LOW File-header comment still enumerates the old retry set — src/llm-client.ts

Header reads 'Exponential-backoff retry on 429 + 5xx gateway errors (502/503/504)' while RETRYABLE_STATUS (line 355) now includes 522/524. Impact: the doc closest to the change under-reports behavior; repo doctrine says update it with the change. Fix: extend the parenthetical to '(502/503/504 + Cloudflare edge 522/524)'.

🟡 LOW Header doc still enumerates only 502/503/504 as retryable — src/llm-client.ts

Module docstring line 5 says 'Exponential-backoff retry on 429 + 5xx gateway errors (502/503/504).' after the set now includes 522/524. The two new codes are Cloudflare-edge 5xx-class and behave like 504, so the doc is now incomplete. Fix: update the enumeration to mention 502/503/504/522/524. Cosmetic only; no behavior impact.

🟡 LOW Stale header docstring omits 522/524 — src/llm-client.ts

The module header docstring still says 'Exponential-backoff retry on 429 + 5xx gateway errors (502/503/504)' after the RETRYABLE_STATUS set was extended to include 522 and 524 at line 355. The inline comment at 352-354 was updated, so this is an inconsistency only, not a behavior bug. Fix: append 522/524 to the parenthetical, e.g. '(502/503/504/522/524)'. No functional risk.

🟡 LOW 400-char truncation bound untested — tests/campaign/external-optimizer-lifecycle.test.ts

The implementation truncates with String(error).slice(0, 400), but the test throws a 27-char message, so the slice is never exercised. A second case throwing a >400-char message and asserting body.error.length caps at 400 + prefix would guard the bound and prevent an accidental unbounded leak regression. Impact: low — bound is a safety cap, not behavior the test claims to name.

🟡 LOW 400-char truncation of the carried detail is untested — tests/campaign/external-optimizer-lifecycle.test.ts

The production fix truncates the detail to String(error).slice(0, 400) (external-optimizer-callback.ts:193), but the test only uses a short message, so truncation behavior (and the malformed/oversized-detail path) is unverified. Add a case with a >400-char thrown message asserting the body is capped. Low: the 500 status and containment assertions are already correct.

🟡 LOW callback server close is not in a finally block — tests/campaign/external-optimizer-lifecycle.test.ts

await callback.close() runs only after the response.json() and status assertions. If postEvaluation or response.json() throws, the listening server on the ephemeral port is never closed and the event loop stays alive. Sibling tests in the same file use try/finally (line 157-178) for the proxy equivalent. Fix: wrap in try/finally like the existing pattern.

🟡 LOW callback.close() not in finally — tests/campaign/external-optimizer-lifecycle.test.ts

If response.json() at line 50 throws (non-JSON body), callback.close() is skipped and the server leaks for the rest of the run. Matches the sibling tests' convention in this file (only the proxy test at line 157 uses try/finally), and process exit cleans up, so impact is minimal. Fix: wrap from postEvaluation onward in try/finally.

🟡 LOW 500 deliberately non-retryable is unpinned — tests/llm-transient-status.test.ts

RETRYABLE_STATUS (src/llm-client.ts:355) excludes 500 while retrying 502/503/504/522/524 — a deliberate but surprising choice with no explanatory comment at the constant and no pinning test in either test file. Someone 'completing' the set to 5xx would silently start retrying persistent origin bugs. Fix: add 500 to the deterministic-status it.each list, which documents intent at zero cost.

🟡 LOW Classification coverage for one pure function is split across two test files — tests/llm-transient-status.test.ts

This file re-tests isTransientLlmError statuses (429/502/503/504, 400/401) already asserted in src/llm-client.test.ts:946-989. Genuinely new coverage: 522/524 (the PR fix), 404/422 negatives, and the foreign-Error-with-numeric-status path. Because the two files are maintained independently, a future edit to RETRYABLE_STATUS (src/llm-client.ts:355) must be mirrored in both, and a mismatch would only surface when each file runs. Suggest folding the new status cases into the existing 'llm-client — isTransientLlmError classification' describe block, or deleting the overlapping assertions here.

🟡 LOW Foreign numeric-status branch has no negative test — tests/llm-transient-status.test.ts

classifyTransient (src/llm-client.ts:403) reads a numeric status off non-LlmCallError errors and returns true only when it is in RETRYABLE_STATUS. The new file pins only the positive case ({status: 524} -> true). No test anywhere (checked src/llm-client.test.ts:946-989) asserts a foreign error with a deterministic status is NOT retried — e.g. Object.assign(new Error('validation failed'), { status: 422 }). A regression like typeof status === 'number' without the set check would pass the entire suite while causing HTTP-library 4xx errors to be retried. Fix: add it.each([400, 401, 404, 422])('never retries foreign deterministic status %i', ...) constructing Object.assign(new Error('deterministic'), { status }) and expecting false.

🟡 LOW No end-to-end retry test for 522/524 through callLlm — tests/llm-transient-status.test.ts

The file tests the classifier only. The live retry path (src/llm-client.ts:724) checks RETRYABLE_STATUS.has(res.status) directly, and src/llm-client.test.ts's 'retry semantics' block (lines 703-833) covers 429/503/502/400 but not 522/524, so no test drives a mocked 522/524 HTTP response through callLlm to a subsequent success. Because both paths read the same constant the classifier test pins set membership transitively, and the loop mechanics are shared with the covered 503 case — so residual risk is a coupling break (e.g. the loop switching to a literal like status >= 500). Fix: add one mocked-fetch case retries Cloudflare 522 then succeeds next t

🟡 LOW No integration coverage for the actual retry fix — tests/llm-transient-status.test.ts

The file validates only the pure classifier. The fix's real behavior is the retry decision in callLlm's !res.ok branch (src/llm-client.ts:723-732), which reads RETRYABLE_STATUS directly and is already integration-tested for 429/503/502/400 in src/llm-client.test.ts:703+. A 522/524 integration case (mock fetch returning 522 then 200, asserting the call recovers) was not added, so a regression that desyncs the !res.ok check from the classifier set would pass this file while breaking actual edge-timeout retries. Low risk today because both paths share the single RETRYABLE_STATUS set.

🟡 LOW Overlaps existing LlmCallError status coverage — tests/llm-transient-status.test.ts

429/503/400/401 are already asserted in src/llm-client.test.ts:970-975. Redundant but harmless; the new 522/524 and foreign-status cases are the real net coverage.

---

_{tangletools · 2026-08-19T06:50:14Z · trace}

[tangletools]

✅ Approved — 22 non-blocking findings — d947bd0c

Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-08-19T06:50:14Z · immutable trace}

[tangletools]

✅ No Blockers — d947bd0c

Review health 100/100 · Reviewer score 71/100 · Confidence 80/100 · 11 findings (11 low)

	opencode GLM 5.2	opencode DeepSeek v4 Pro	aggregate
Readiness	71	83	71
Confidence	80	80	80
Correctness	71	83	71
Security	71	83	71
Testing	71	83	71
Architecture	71	83	71

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision.

🟡 LOW Evaluation error detail disclosed to the optimizer caller — src/campaign/external-optimizer-callback.ts

The 500 body now echoes the raw thrown detail from args.evaluate back to the requesting client. args.evaluate is host-controlled code whose errors may embed sensitive content (upstream LLM error bodies, prompt/response text, internal paths, or credentials surfaced in an exception message). Mitigations are real: the server binds to 127.0.0.1 (listenLocal), the endpoint requires the shared bearer token, and the payload is capped at 400 chars, so exposure is limited to the authenticated optimizer counterparty. This is an acceptable diagnostics-vs-disclosure tradeoff, but consider sanitizing known secret patterns or logging the full detail server-side via observe() while returning a shorter public summary if the evaluate callback may surface provider credentials in error text.

🟡 LOW Evaluation-failure detail is not persisted in the refusal observation artifact — src/campaign/external-optimizer-callback.ts

The observe() call at lines 183-189 records only {kind:'refusal', reason:'evaluation-failed', candidate, candidateHash, exampleId}; the thrown detail added to the 500 body at line 192-194 never reaches the durable artifact. The refusal variant of ExternalOptimizerEvaluationObservation (external-optimizer-contracts.ts:224-231) has no detail field. The diff's own comment says 'The thrown detail is the only diagnostic for a failed evaluation' — yet if the optimizer process discards the HTTP error body

🟡 LOW No test for the 400-character truncation bound — src/campaign/external-optimizer-callback.ts

The new test (tests/campaign/external-optimizer-lifecycle.test.ts:41-55) asserts the short message round-trips, but nothing exercises the .slice(0, 400) bound with an oversized message. A regression that drops the slice or moves the cap to 400 KB would pass the suite. Fix: add a case throwing an Error with a >400-char message and assert body.error.length stays within the expected bound while still starting with 'evaluation failed: '.

🟡 LOW Module header retry doc omits 522/524 — src/llm-client.ts

Header comment still reads 'Exponential-backoff retry on 429 + 5xx gateway errors (502/503/504)'. The parenthetical enumeration is now incomplete after RETRYABLE_STATUS gained 522/524 at line 355. Given this repo's doc-discipline rule, update the parenthetical to (502/503/504/522/524) or drop it. Adjacent stale doc of the same kind exists at src/judge-retry.ts:36 ('{429,502,503,504}') but that file is outside this shot.

🟡 LOW No end-to-end callLlm retry test for 522/524 — src/llm-client.ts

The new tests pin isTransientLlmError classification only. The callLlm retry loop reads RETRYABLE_STATUS.has(res.status) directly at line 724 — separate code from the tested classifier — and existing loop tests (src/llm-client.test.ts:704 'retries on 429 with Retry-After', :724 'retries on 503 gateway') do not include a 522/524 case driving 524-then-200 through a mocked fetch. Both read the same constant so wiring risk is minimal, but one e2e case would pin it. Fix: add a case to the existing 'retry semantics' block mirroring the 503 test.

🟡 LOW Remaining Cloudflare edge codes (520/521/523/525) still escape the retry loop — src/llm-client.ts

The change's own rationale — router deployments serve through Cloudflare, so edge statuses replace origin statuses — applies equally to 520 (origin unknown error) and 521 (origin down), which are outage/transient-class and currently kill a run exactly like 524 did before this PR; the repo's sibling retry loops (src/hosted/client.ts:135, src/adapters/http.ts:146) already use the broader >=500 rule. Scope decision, not a defect: only the two observed timeout codes were added, and 523/525 are config-class where retry is doubtful. Consider adding 520/521 in a follow-up.

🟡 LOW 400-char truncation bound of the error detail is untested — tests/campaign/external-optimizer-lifecycle.test.ts

The source caps the embedded detail at String(error).slice(0, 400) (src/campaign/external-optimizer-callback.ts:193), but the test only exercises a short message. A companion case with an oversized error message would pin the cap so a future edit cannot silently grow the response or drop the bound. Optional hardening; not blocking.

🟡 LOW Assert status before parsing the JSON body — tests/campaign/external-optimizer-lifecycle.test.ts

Line 50 reads await response.json() before line 53 asserts response.status === 500. If the callback returns a non-500 status with a non-JSON body, the test fails with an opaque JSON parse error instead of pointing at the status mismatch. Move expect(response.status).toBe(500) above the .json() call (then re-parse via response.clone().json() if needed). Cosmetic only; the test currently passes 6/6 and correctly guards the source change.

🟡 LOW No try/finally around callback.close() on the failure path — tests/campaign/external-optimizer-lifecycle.test.ts

If postEvaluation or response.json() rejects, callback.close() never runs and the loopback server leaks for the process lifetime. Matches sibling callback tests (lines 24-37, 78-88) which also skip try/finally, while the proxy test at line 157 does use try/finally. Fix: wrap lines 49-51 in try/finally with await callback.close() in the f

🟡 LOW Coverage overlaps existing isTransientLlmError suite in src/llm-client.test.ts — tests/llm-transient-status.test.ts

src/llm-client.test.ts:946-987 already has a describe block for isTransientLlmError covering 429/503 true, 400/401 false, error-pattern matching, cause recursion, and non-Error inputs. This new file re-tests the same classifier on a status-only axis (502/504/522/524 true, 404/422 false). Not a defect — the new cases (522/524, foreign .status) are genuinely additive — but future RETRYABLE_STATUS changes now require edits in two test files. Consider folding these status-table cases into the existing suite or leaving a cross-reference comment.

🟡 LOW Partial duplication of existing classification tests — tests/llm-transient-status.test.ts

The it.each cases for 429/503 (retried) and 400/401 (not retried) restate assertions already present at src/llm-client.test.ts:971-974. The net-new coverage is 502/504, the Cloudflare 522/524 pair, and the foreign-error status branch. Impact: none on correctness; slight maintenance cost if the classifier changes and two suites must be updated. Fix (optional): keep this file scoped to the 522/524 and foreign-status cases, or fold the duplicates into the existing describe block in src/llm-client.test.ts.

---

_{tangletools · 2026-08-19T06:51:48Z · trace}