Repository navigation
Two seams from the architecture review: classified upstream failures + one gate for every agent turn - #183
Open
maxff77 wants to merge 20 commits into
Open
Two seams from the architecture review: classified upstream failures + one gate for every agent turn#183maxff77 wants to merge 20 commits into
maxff77 wants to merge 20 commits into
Conversation
The classifier (utils/upstream-error.js#describeUpstreamFailure) already knew why
the upstream failed, but the return path — {status:false, response:null} — threw
it away: all three callers answered 500/502 "Request failed", so an agentic
client could not tell "come back later" from "the proxy is broken".
Every failure exit now carries the verdict: no usable account -> 503, chat not
created -> 502, attempts exhausted with a non-200 -> 429|502, transport
exhausted -> 503. The shape is defined once (unclassifiedFailure), and the quota
predicate gains one clause: an error carrying an HTTP 429 counts as quota
exhaustion, so the account that served it stops being handed out.
Ticket 01 of .scratch/failure-classification. Decision: docs/adr/0001.
Co-Authored-By: Claude Code <noreply@anthropic.com>
The pre-response failure exit answered a mute 500 "Request failed" for every kind of upstream failure. It now states the verdict that travels with the failure: quota -> 429 insufficient_quota, overloaded -> 503 upstream_unavailable, transport -> 503, unclassified -> 502 upstream_error, with Retry-After only when the upstream supplied a wait. The wire translation moves into one adapter (openAIErrorShape) shared by the exception path (upstreamErrorShape) and this return path, so there is a single place that knows OpenAI's error vocabulary. Ticket 02 of .scratch/failure-classification. Co-Authored-By: Claude Code <noreply@anthropic.com>
The pre-response failure exit answered a mute 500 api_error for every kind of upstream failure. It now states the verdict: quota -> 429 rate_limit_error, overloaded -> 529 overloaded_error, transport -> 503, unclassified -> 502 api_error, with Retry-After only when the upstream supplied a wait. The catch and this exit now share one wire writer (writeAnthropicHttpFailure), so there is a single place that knows Anthropic's error vocabulary. The catch keeps its 500 default: what reaches it are our own exceptions and already classified challenges, not an opaque upstream non-200. Ticket 03 of .scratch/failure-classification. Co-Authored-By: Claude Code <noreply@anthropic.com>
Standards axis: - request.js comment no longer claims every HTTP 4xx/5xx leaves the account cooldown alone; the 429 exception is named. - One Anthropic error-type mapper (anthropicErrorType) shared by the wire writer and the mid-stream catch, instead of the same switch twice. - The completion handler's overloaded branch uses the shared shaper instead of building the same object by hand. - The wire translation (openAIErrorShape) moves from the chat controller into utils/upstream-error.js, beside both wire vocabularies, because the agent runtime needs it too and must not import a controller. Spec axis: - The verdict now survives the agent runtime's correction resend, which is the live site: the review pointed at the account-failover exit instead, and that one is unreachable. - rateLimitRetryAfterSeconds also reads an upstream Retry-After header (seconds) on an HTTP 429, so the HTTP channel stops dropping a wait the upstream did give. - CONTEXT.md sharpens Quota exhaustion: a rate limit and a spent daily allowance are one thing to the proxy, because it cannot tell them apart. Tests: +5 (1251 total, baseline re-blessed). Co-Authored-By: Claude Code <noreply@anthropic.com>
The two legacy handlers in the chat controller branched on `strict_agent_turn`, whose only producer in the tree was a test: every production request carrying tools is dispatched to the agent handler before reaching them. Their tool-call parser, native accumulator, tool-call delta writer, protocol-error gate, compensation retry and in-band error frame could not fire — while the largest block of tool-call coverage aimed at them instead of the live gate. Deleted: the `hasTools`-gated regions of both legacy handlers, the sentinel itself, the three hint builders and the local `requiresToolCall` that only fed them, and the three tests pinning the unreachable behaviour. The legacy functions keep only the no-tools path they still serve, with identical output, messages and statuses. Deliberate deviation from the ticket: `buildEmptyOutputRetryHint` and `appendRetryHintToRequestBody` stay. The ticket listed them as dying with the path, but the empty-output compensation retry is not `hasTools`-gated — a tools-less request that returns only reasoning still gets one retry — so both keep a live caller. The message on the non-stream retry branch that names a tool-call retry stream is left untouched on purpose: this change alters nothing observable. Verified: precondition measured (`strict_agent_turn` has no producer outside the deleted test), eslint clean, suite green with the count gate re-blessed at 1248/137 — exactly the three deleted tests. Co-Authored-By: Claude Code <noreply@anthropic.com>
Follow-up to 576be12, from the spec review of that commit. The review found that one deletion there was not behaviour-neutral, and it was right. The empty-output compensation retry in the non-stream handler rebuilt the native accumulator and re-parsed the retry round's text with no `hasTools` condition anywhere in that block — only the first round was gated. Measured: `parseToolCallsFromText('[TOOL CALL]{...}[END TOOL CALL]', { allowedToolNames: [] })` returns one call with `cleanedText: ''`, so a request that declared no tools at all, whose first round came back empty and whose retry returned a `[TOOL CALL]` block, was answered with `tool_calls` and `finish_reason: "tool_calls"`. The retry hint asks for exactly that block, so the loop closed on itself. New behaviour: without tools there are never tool calls; the protocol residue travels verbatim as content, which is what this same path's first round and the streaming twin already did. Pinned by a new test. Restoring the parse was the alternative and was rejected: it would preserve a defect in exchange for a diff that reads as "pure". The honest move is to declare the change, which the ticket now does, along with the second correction the review surfaced — the hint-append helper the ticket expected to die here has a live caller and stays, so all three copies survive into issues 05 and 07. The root cause is filed, not patched: the empty-output hint asks a tools-less request for a `[TOOL CALL]` block. Co-Authored-By: Claude Code <noreply@anthropic.com>
…eeded Ticket 02 of `.scratch/agent-turn-gate/`. The claim under test came from the August loop-unification plan: the non-stream handler builds `createUpstreamResponseFilter` once per request and its retry block rebuilds everything else, and that filter latches the response id it accepts — so a retry whose frames carry a different id would contribute nothing. Both halves of the mechanism check out. The latch is real and the retry block does not rebuild the filter. The trigger does not: `response.created` re-latches the filter to the new id, and every upstream generation opens with that frame (captured frames in sse.test.js and chat-challenge.test.js show it). Measured both ways before deciding: - retry with `response.created` and a different id → the call is recovered - retry with a different id and no `response.created` → every frame dropped The second shape is the old protocol, which the upstream no longer emits. Per the ticket's own checklist the fix is therefore not applied "just in case": closing as no-change-needed, with the measurement in the test file's header and in the ticket. What ships is the regression net over the re-latch, so a filter that stops accepting the new id — or a retry window that stops being rebuilt — turns red. Co-Authored-By: Claude Code <noreply@anthropic.com>
…arameter Standards review of 576be12, two of its four judgement calls. `normalizeOpenAIFinishReason` took `hasToolCalls` and short-circuited to `'tool_calls'`; with the tool paths gone both call sites passed a literal `false`, so the parameter and that branch were unreachable from live code. The parameter is gone and the two call sites pass only what they still decide. The `tool_use` alias still maps to `'tool_calls'` in the string table, so a response carrying that upstream reason reports what it reported before. The `has_tools` jsdoc on both legacy handlers still said "whether to enable tool-call parsing", which stopped being true the moment its value began routing to the agent handler instead. It now says so. Left alone, deliberately: the two stale retry-branch message strings (untouched to keep the diff behaviour-neutral, already noted in the ticket) and the duplicated retry shape the review flagged in both handlers — that one belongs to the gate work, which is set to restructure exactly there. Co-Authored-By: Claude Code <noreply@anthropic.com>
Ticket 03 of `.scratch/agent-turn-gate/`. The net that makes "same behaviour, one home" falsifiable instead of asserted: 16 scenarios × 4 handler cells, recorded from the current code and asserted from the recorded file. Every later ticket in the series diffs against this, and anything that moves must appear in the diff as a named change. What the baseline already measures, before any refactor touches it: - `tool_error` produces byte-identical hint text on all four cells (the OpenAI side spells the reason `invalid_tool_call`, the Anthropic side `tool_error`) — the merge case for the unified vocabulary. - `required_tool` produces *different* hints for the same condition, because one surface builds it from the shared map and the other from a local builder. - `good_call_with_broken_sibling` and `prose_with_tools`: the OpenAI cells retry, the Anthropic cells accept and ship the good call — the per-surface policy difference, now pinned by behaviour rather than by prose. - `delivered_round_then_empty`: the Anthropic stream makes 2 upstream sends where both other cells make 3, because it judges `empty` against text accumulated across attempts. The streaming/non-streaming twins disagree today; the corpus says so. The recorder lives in the dev-probes tooling and writes the baseline under tests/fixtures — test data beside the test, not in the probe directory. It refuses to write a hollow row, and `--check` re-runs and diffs without writing. Verified by hand, not by the author's word: 17/17 green, eslint clean, and a mutation of one cell's status turns exactly one assertion red — the test reads the baseline rather than agreeing with itself. `npm run test:bless` in the same change (1250 → 1267). Co-Authored-By: Claude Code <noreply@anthropic.com>
Both reviews of aff4e53, and one of their findings corrects a claim in that commit message. The corpus could not see the change it exists to protect. Spec review mutated the streaming Anthropic judgement from accumulated text to attempt-scoped text — the `empty`-scope unification this series is built around — and all 64 cells stayed byte-identical. Reproduced here before acting on it. The cause is structural, not a gap in the fixture set: whenever the two rules would disagree the accumulated text is non-empty, and at that same moment the loop's own compensation guard is evaluated over that same accumulated text, so it has already broken the loop or already spent its one post-text retry. Ticket 05 now expects an empty diff for that change, and the spec says why. Correction to aff4e53's message: it attributed `delivered_round_then_empty`'s 2-sends-against-3 to the `empty` scope. That row's divergence comes from the compensation guard reading the same accumulated text, not from the `empty` branch — which is exactly why the mutation above is silent. The rest closes what the standards review found: - `targets` was a per-family declaration nothing checked against observation, and one row declared two reasons while firing one — green by not looking. Now declared per cell, because the two modes of a family genuinely disagree (the non-stream Anthropic cell fires two reasons where the stream fires one), and an invariant requires the declared count to equal the observed hint count. Probed: declaring a reason that does not fire makes the recorder refuse to write and name the cell. - The hollow-row invariants existed twice, in the test and the recorder, and had already diverged — the copy deciding whether to write was not the copy deciding whether to pass. One shared function now. - The Anthropic hint is no longer normalised by stripping its `# Tool-call retry` header: that text travels to the model, so stripping it kept a model-facing contract out of the baseline. - Unused exports trimmed, a stale comment corrected (`droppedResult` is used), and the four runners share their options/ctx builders. Baseline re-recorded: 17/17 green, eslint clean, `--check` reproduces it. Co-Authored-By: Claude Code <noreply@anthropic.com>
Ticket 04 of `.scratch/agent-turn-gate/`. The OpenAI runtime clamped the attempt budget with a floor of 2, the two Anthropic loops with a floor of 1, so the same number meant two things: a caller asking for 1 got two upstream generations there and one here. In production both read the same config — itself clamped to [2, 6] — so only a per-request value could expose it, which is why the divergence survived: the only callers that pass one are tests. "Max attempts" now means the total upstream generations for one client request, counting the first. Floor 1, ceiling 6, one resolver in a new leaf module that also gives the gate a home for the next ticket. The config clamp is unchanged, so configuration still cannot ask for less than 2 — only a per-request value can, and now it is honoured. Proved red first: with the old expression restored, the budget-1 test fails and the other two pass; with the resolver it is green. The corpus is byte-identical, which is the expected result rather than a gap — no cell passes a budget, so nothing in it ever exercised the divergence. `chat-challenge.test.js` already passes `agent_turn_max_attempts: 1` and stays green, which says the phantom second attempt was invisible to what those tests assert. Co-Authored-By: Claude Code <noreply@anthropic.com>
Both reviews of d5f8ee5. Each found something the commit message got wrong. Spec review measured two input drifts the message's "nothing else moves" did not cover. Compared old against new across the edge inputs: a negative per-request value used to clamp up to 2 and now falls to the configured value, and an `Infinity` used to clamp down to 6 and now also falls to config. Neither is reachable — no production caller passes a per-request budget at all — but they are behaviour changes, so the docblock and the ticket now name them instead of leaving them to be discovered. It also showed the ticket's own expectation was false: the corpus pins the config value and no cell passes a budget, so the diff is empty rather than "the scenarios that asked for 1 attempt now make one". That is the correct answer, not a gap, and the ticket records it along with the fact that its `Blocked by: 03` went unmet — the behaviour is pinned by a standalone test proven red against the old floor. Standards review caught the overclaim: the docblock said "one meaning" while `AGENT_TURN_MAX_ATTEMPTS=1` still yields 2, because config keeps its own [2, 6] clamp — deliberately, per the ticket. The docblock now states the exact scope: the floor of 1 is reachable only through a per-request value. Also closes the gap spec review flagged in the route-seam test: the Anthropic surface has no per-request override, so there is no budget-1 case to run there. The new pin passes a stray override in the ctx and asserts it is ignored — the day that channel exists, this test forces the decision instead of inheriting it. Co-Authored-By: Claude Code <noreply@anthropic.com>
Ticket 05 of `.scratch/agent-turn-gate/`. The streaming Anthropic loop stops deciding for itself: it builds a snapshot of one attempt and calls `gate`, which returns accept-or-retry-with-a-reason. The module that holds the decision now also holds the reason vocabulary, the hint map and its builders, the append helper and the recovery-reason set. The corpus is byte-identical across all 64 cells — the expected result, and the reason the move was safe to attempt: every hint that travels to the model is frozen in the baseline, so a careless move shows up. In particular the `empty` scope change this ticket carries is invisible, as measured in ticket 03: the loop's own compensation guard reads the same accumulated text that makes the two rules disagree. Declared, because each is a real difference from what shipped: - The snapshot carries fourteen fields, not the twelve the ticket listed. `required_tool` is undecidable without knowing whether the request declared tools and whether its `tool_choice` demanded a call; both enter as precomputed booleans, so the snapshot stays data and the policy stays four named fields with no config read inside the module. - One log token is renamed: the rejection warn now prints `required_tool` where it printed `required`. Not client-visible. One test pinned the old string and was updated; what it guards — that dropped-frame names are logged even when another reason masks them — is unchanged. - `appendRetryHint` is unified on the deep-clone implementation. The Anthropic copy it replaces cloned the message objects shallowly and then wrote into the shared text part, so on an array-content message a second retry accumulated the previous hint. Corpus-invisible (the corpus uses string content), and the deep-clone shape is the one that does not mutate its caller. - `required_tool`'s hint text is the Anthropic one. The OpenAI wording for the same condition is a decision for ticket 07, noted in the module. - Deviation from the ticket's preamble, which says both reason-keyed maps move here: the exhausted-turn message map did not. Only the hint side has a caller after this ticket, and moving the other would land dead code that ticket 07 wires. The ticket now says so. New truth table at the gate seam: 44 tests over every token, every policy field in both directions, and the combinations the corpus does not carry. `npm run test:bless` in the same change (1271 → 1315). Co-Authored-By: Claude Code <noreply@anthropic.com>
Both reviews of f75ef0f. The message of that commit claimed the ticket had been amended to record that the exhausted-turn map did not move. It had not — the ticket was untouched, and the claim was false. The ticket now carries it, along with why: only the hint side has a caller after this ticket, so moving the other map would land dead code that ticket 07 wires. That correction is the reason this commit exists. Spec review also measured that the module is not a dependency leaf: requiring it pulls `logger.js`, `runtime-paths.js`, `tools.js` and `jwt-decode` through `agent-turn.js` and `tool-prompt.js`. The docblock no longer says "hoja"; it says what is true — `gate` is a pure function of its two arguments, and that is the purity that makes the truth table cheap. Two deltas in the unified `appendRetryHint` were real but unnamed, so they are named now: it adds the `else { last.content = hint }` branch the old Anthropic copy lacked (that copy left content which was neither string nor array untouched), and it returns a deep clone instead of `{...body, messages}`. Both are unreachable for the body shape this surface builds. Truth table extended where the review found a hole: `controlKind: 'blocked'` shares the `final` branch, so it gets its two rows (with text → accept, without → empty). Counts 1315 → 1317. Two cosmetic corrections the standards review caught: the local is `decision` now, not `verdict` (the stutter), and a comment no longer claims both Anthropic loops read `ANTHROPIC_GATE_POLICY` when only the streaming one does. The traps both reviews found are recorded where they will be stepped on — the controller export that silently becomes `undefined` when ticket 06 deletes the aliased imports, the optional `allowedToolNames` that silently drops the unknown-tool appendix, and `hasTools` defaulting to true when a snapshot omits it. Co-Authored-By: Claude Code <noreply@anthropic.com>
…gate Ticket 06 of `.scratch/agent-turn-gate/`. Both Anthropic loops now build the same fourteen-field snapshot and call `gate`; this one sets `callsDelivered: false`, because nothing has reached the client when it decides — that field is the whole difference between the two loops' decisions, and everything else about them is now shared. The four local hint builders and the local append helper die here: after this commit both loops take their text from the module, so the controller keeps only how it builds a snapshot from its own stream and how it states the verdict in its own wire vocabulary. The trap the ticket-05 review flagged is closed: the controller's `buildToolErrorRetryHint` export resolved through an aliased import, and deleting that import would have left it silently `undefined` while `tests/anthropic-tool-error-hints.test.js` still required it from the controller. The test now imports the function from where it honestly lives. `requiresToolCall` stays in the controller — it has five other callers across both snapshots and the delivery layer, so it was never a candidate for deletion. Corpus: unchanged on every Anthropic cell, byte for byte, which is the expected result and was measured; the recorder's diff at the time came only from the OpenAI surface's declared change, wired in the next commit. Its count baseline also lands there, because both tickets came out of one working tree. Co-Authored-By: Claude Code <noreply@anthropic.com>
Ticket 07 of `.scratch/agent-turn-gate/`, the last of the three surfaces. All three now build a snapshot of one attempt and call the same pure function; what each keeps is how it reads its own stream and how it states the verdict in its own wire vocabulary. Moved: the exhausted-turn message map joins the hint map in the module, keyed by the unified vocabulary (`tool_error` and `prose_with_tools` take the two halves the old `invalid_tool_call` covered; every message is byte-identical). The runtime's local evaluation, its local hint appender and `NON_RETRYABLE_FINISH_ REASONS` are gone — the terminal set is imported from the gate, one definition. The declared behaviour change, decided before the work started and recorded in the ticket: the retry hint unifies, so this surface's tool-error hint gains the appendix naming unknown tool names and invalid arguments. The corpus diff is exactly that and nothing else — six hint cells, each gaining only the appendix with its body unchanged, plus the target-token renames. No `status`, `sends`, `frames` or `delivered` cell moved. The gate gained one branch, and it belongs to both surfaces: the old rule "a finish the upstream already explained is accepted" had nowhere to live, because the Anthropic surfaces got it by falling through while this one had it first. It now sits after the two vetoes, so a terminal round with text-channel tool errors still vetoes and `required_tool` still retries — which is what Anthropic already did in production and this surface did not. Two residual deltas follow and are declared in the ticket. Checked separately because the corpus cannot see it: the corpus records status, delivered, hints, sends and frames, but not `finish_reason`. The terminal branch returns 'stop', so a truncated round reporting `"length"` now depends on `wireFinishReason` preferring the attempt's own terminal finish (normalising `max_tokens` to `length`). The max-token test asserts it and passes. Without that check this would have been a wire regression the net is blind to. Co-Authored-By: Claude Code <noreply@anthropic.com>
…t left Standards review of tickets 06 and 07. `OPENAI_LOCAL_HINTS` kept a `prose_with_tools` entry whose text is byte-identical to the builder the gate already serves for that token. A second home that agrees today and can drift tomorrow, with nothing watching it. Deleted, so the reason falls through to the module. The other two entries stay and now say why: `empty` and `required_tool` have this surface's own wording, and that difference is deliberate. Two comments named things that no longer exist: the controller's pointer at the deleted `gateRetryHintFor`, and `wireFinishReason`'s claim that the gate does not know the truncation tokens when it imports them to decide — it knows them and simply does not emit them, which is the distinction the comment now draws. Verified neutral: corpus byte-identical, agent-protocol 73, turn-cutoff 42, gate 46, eslint clean. Filed rather than fixed here: the fourteen-key snapshot literal is written twice in the controller and a third time in the runtime, so adding a field means four edits and the parity between the two Anthropic loops is asserted by hand, not by structure. That is ticket 09 — an extraction belongs in its own change, or a corpus diff stops telling you which of the two moves produced it. Co-Authored-By: Claude Code <noreply@anthropic.com>
Spec review of tickets 06 and 07, which found a regression neither commit declared. `orphanResidue` was switched off entirely once the protocol-recovery allowance was spent. The gate reads that field for two different things: the retry decision for `malformed_protocol`, where the allowance does govern — and that is what the deleted code did — and the decision to withhold the text on a native or cut acceptance, where the deleted code called `containsOrphanProtocolResidue` unconditionally. With the field off, a client could be handed protocol bytes that used to be held back. The residue is now measured raw, always, and the allowance travels as its own field (`protocolRecoverySpent`) that the gate consults only for the retry. Three truth-table rows pin it, including the one that proves suppression still fires with the allowance spent — without that row the fix would be as unguarded as the bug was. Two claims in aa12ec7's message were wrong and the ticket now records them. The corpus does record the wire finish reason, inside `delivered`: what is missing is a scenario that ends on a terminal finish, since the only literal in the corpus is `"stop"`. That is a coverage hole rather than a blind spot, and the difference matters for anyone relying on my earlier claim. And "differing only in `callsDelivered`" holds for the field list, not the values — the two Anthropic loops pass different tool-call and text inputs; the equivalence is in the decision, which is what the parity test asserts on the wire. Counts 1334 → 1337. Co-Authored-By: Claude Code <noreply@anthropic.com>
ADR 0002, ticket 08 of `.scratch/agent-turn-gate/`. It records what the seven preceding commits decided, in the places a reader will look for them: why the verdict has no `fail(code)`, why `callsDelivered` is what makes two loops with different inputs one decision, the four policy fields and the reason each exists rather than being flattened, where the gate stops and delivery starts, and why the `empty` scope unification is invisible — measured, not asserted. It also records two things this series got wrong on the way, because a decision record that omits them is a sales document: the terminal rule now sits explicitly in the gate and the OpenAI surface consequently retries a terminal round whose `tool_choice` went unsatisfied, where it used to deliver; and a spent protocol-recovery allowance stops the retry for malformed residue without stopping its suppression, a distinction a review found the hard way. The loop-unification plan in the lohari repository is marked superseded in part: its loop half stands, its "never harmonise the asymmetries" constraint is overruled by the four policy fields, and its stale counts are corrected. Not done here, and named in the ticket: the live agentic turn on the next environment. That is a deploy, and it gets its own run. Co-Authored-By: Claude Code <noreply@anthropic.com>
Author
|
Fork CI on
The upstream |
maxff77
changed the title
Upstream has no `docs/adr/` and no `CONTEXT.md` — its `docs/` holds two README screenshots. Shipping ours there would not be documentation, it would be imposing a convention on a maintainer who asked for neither, inside a PR about turn handling. Both decision records stay on disk as fork artifacts, excluded locally the way `.scratch/` already is. The one code reference to them is gone with them: a comment in the request module cited the ADR by path, which would have been a dead reference upstream. It now states the rule it was pointing at — an upstream `429` cools the account down with the default wait when the body carries none. No behaviour change: this commit untracks two documents and rewords one comment. Co-Authored-By: Claude Code <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two seams from the same architecture review
This branch carries two independent proposals, both from the 2026-10-09
architecture review of this repo. They are unrelated to each other — the first is
about what a client is told when a request fails, the second about how a turn is
judged — and each has its own decision record. They arrive together because they
come from one review and one author; a maintainer who wants them apart should say
so and they will be split.
Part 1 — Upstream failures reach the client classified, not as a blanket 500
The problem
When Qwen refused a request — quota exhaustion, an overloaded upstream, a dead
transport — the proxy answered
500/502with"Request failed", so an agenticclient could not tell "the proxy is broken, stop trying" from "upstream refused,
come back later". It retried into a wall, spending another account per attempt.
The classification already existed and was already computed.
sendChatRequestknew whether it had an HTTP response, which status, whether the transport had
given up and whether the account was out of quota — and then returned
{ status: false, response: null }, throwing the answer away. Four callersre-derived the meaning; three hard-coded
500.What this does
The verdict the request module already computes travels out with the failure, in
the one shape the classifier already defines, and each surface states it in its
own wire vocabulary: quota exhaustion →
429(insufficient_quota/rate_limit_error), overloaded upstream →529(overloaded_error) on theAnthropic surface and
503(upstream_unavailable) on the OpenAI one, transportinterruption →
503, unclassified →502.Retry-Afteris emitted only from await the upstream actually supplied, never invented.
Every failure exit of the request module now carries the verdict, so no caller
has to invent a default. Account-side effects are provably unchanged: transport
failures still accumulate toward cooldown, HTTP refusals still only warn — with
one accepted, recorded exception, an upstream
429now marks that accountquota-exhausted.
The one accepted side effect, recorded rather than discovered: an upstream
429now marks that account quota-exhausted, and with no wait in the body that is the
default one-hour cooldown. The decision record for this half lives in the fork
(this branch deliberately ships no
docs/— upstream has none, and a PR aboutturn handling is no place to introduce one).
Part 2 — One gate decides every agent turn
The problem
"Is this upstream attempt acceptable?" was answered in four places — the OpenAI
runtime and three loops inside the two controllers — in two reason vocabularies,
with attempt budgets that meant two different things. The rule that kept the
copies equal lived in comments: seventy-two of them said "twin of …, both parts
must change together", and where a comment could not be trusted, a test read the
source file as text and matched a literal.
The cost was paid by whoever fixed the next defect. A tool loop, a malformed tool
call, a round that should have been retried and was not: each was found once and
fixed once per surface and once per streaming mode, and a fix that landed in one
copy left the others shipping the old rule.
What this does
All three surfaces now build a snapshot of one attempt and call one pure
function.
src/utils/agent-turn-gate.jsowns the decision, the reasonvocabulary, both reason-keyed maps (retry hint and exhausted-turn message), the
hint builders, the append helper and the attempt budget. Each loop keeps what is
genuinely its own: its budget, its mutable flags, its delivery machinery, and how
it states the verdict in its own wire vocabulary.
The per-surface differences do not disappear — they stop being accidental. Four
of them become named policy fields with a documented reason:
proseWithToolsacceptBareFinaltoolErrorsBeforeRequiredrequired_toolfirsttoolErrorsVetoWithCallsThe last one is not decoration: an Anthropic client receives discrete
tool_useblocks and can act on what arrived, while an OpenAI client receives a
tool_callsarray it executes as a set, so a partial set is a silently wrongaction rather than a partial one.
Also in this part
strict_agent_turn's only producer in the tree was a test, so on everyproduction request carrying tools the legacy handlers ran with their tool
branches dead — while the largest block of tool-call coverage aimed at them.
compensation rebuilt the native accumulator and re-parsed the retry text with no
hasToolscondition anywhere in that block, so a request that declared no toolscould be answered with
tool_callsandfinish_reason: "tool_calls". The retryhint asks for exactly that block, so the loop closed on itself.
turned a caller's
1into two upstream generations while the Anthropic loopsapplied 1. Floor 1, ceiling 6, one resolver.
response id it accepts and is built once per request; the claim that this breaks
a retry turned out to be true of the mechanism and false of the trigger, because
response.createdre-latches and every generation opens with it. Recorded asno-change-needed with the measurement, per the ticket's own checklist.
tests/agent-turn-corpus.test.js) freezes theobservable outcome of sixteen scenarios across four handler cells — status,
delivered output, upstream sends, served frames, and the hint text that travels
to the model. It made the refactor falsifiable rather than asserted, and it
earned its keep immediately: it exposed that the corpus could not see the very
change it was built to protect, and that one row declared coverage it did not
have. Both are fixed.
Behaviour changes in this part, declared
hint gains the appendix naming unknown tool names and invalid arguments — the
only text that makes an invented name recoverable.
consequently retries a terminal round whose
tool_choicewent unsatisfied,where it used to deliver. The conservative rule is the one the Anthropic
surfaces already shipped.
length-truncated round still reports"length"to the client: the wirefinish reason comes from the attempt, not from the verdict.
Everything else is byte-identical: the corpus diff is six hint cells (each
gaining only that appendix) plus target-token renames.
The decision record for this half is fork-local for the same reason: this branch
ships no documentation tree.
Verification
1337 tests / 137 suites / 0 fail, count-gated;eslintclean.surfaces: two tool calls then a clean close, with zero attempt rejections.
verify / regression,verify / frontendand two ofthe three platform builds pass; the
linux-x64container job fails on a DockerHub network timeout, not on the code.
🤖 Generated with Claude Code