fix(runtime): classify exhausted Codex edge rejection - #3960
Conversation
|
I reviewed this PR at exact head Spec: NO-GO — 1×P1 + 2×P2
Positives: the transport test proves the fourth attempt and exact 2/10/30 delays; existing abort tests cover both signal sources; HTTP 403/request ID and bounded/redacted diagnostics remain preserved. Standards: 1×P1 + 1×P2 + 1 judgment P3
No architecture-boundary breach or actionable duplication was found. Other checks: What I did not check: full local suite beyond the checks noted. Gate: exact head has P1/P2 findings and therefore cannot be approved or merged until the 403 override, HTML detection, exhaustion stamping, and template/AI disclosure issues are fixed.
|
e65e7c0 to
b9e95c8
Compare
|
Thanks for the exact-head review. I addressed all in-scope P1/P2 findings in head �9e95c812: (1) the exhausted-edge sentinel is now trusted only from the internally created OpenAiCodexEdgeRejectionError/Maka model-failure provenance, so arbitrary provider server_error or spoofed 403 payloads remain Auth; (2) parseable JSON bodies win even when Content-Type is incorrectly ext/html; (3) the sentinel is stamped only after all three replayable edge retries are consumed, while non-replayable requests retain the normal HTTP error. Regression coverage now includes each boundary. I also restored the required PR template sections, declared substantive AI use, and added the Generated-by: Codex trailer. CI was green before this push; the new head is ready for re-review. There are no inline threads to resolve. |
|
The updated head is �9e95c812; all review findings are covered by the new transport/classification tests and the required PR template/AI disclosure is present. No inline review threads exist on this PR; only the automated summary comment required a follow-up. |
Astro-Han
left a comment
There was a problem hiding this comment.
Adds a dedicated internal sentinel for the case where Codex's edge returns HTML 403 across the whole bounded retry budget, and teaches the classifier to read it as ProviderUnavailable instead of Auth, so an edge/WAF incident stops telling users to re-authenticate (#3959).
The problem is real and the direction is right — a 403 from a WAF is not an authorization answer, and classifying it as one sends users to fix a credential that was never broken. The retry budget, the retry-after handling, and the provenance idea are all sound instincts. Three things need to change before I can approve, and two of them are in the trust boundary this PR introduces.
P1 — the sentinel is stamped from the retry counter rather than from what actually failed (subscription-model-fetch.ts:217). Once three edge retries are consumed, any subsequent failure on that request is stamped as an exhausted edge rejection, and the real provider code is discarded. An expired token on attempt 4 gets reported as a provider outage and retried. Details and the one-conjunct fix are inline.
P1 — the provenance check is bypassable two independent ways, both introduced in this diff: the PROVIDER_UNAVAILABLE_PROVIDER_CODES entry at :40 grants the classification to exactly the payloads the trust check rejected, and the trust function's own model_failure clause at :796 accepts a forgeable code that normalizeProviderFailure copies straight out of the provider's payload. Both inline. I want to be clear that this is not a hypothetical-attacker objection: the practical case is a user-configured OpenAI-compatible proxy, and the consequence is bounded. It matters because a guard that is defeated in the same diff that adds it will be relied on as sound by the next change.
P2 — the exhausted signal is handed to a second retry authority, which restarts the budget it was meant to end. ProviderUnavailable with no retry-after returns {retryable: true} from providerRetryMetadata (:248-258), and ai-sdk-backend.ts:2583 retries up to MAX_PROVIDER_ATTEMPTS_PER_STEP = 10. Each outer attempt re-enters the inner 2/10/30 s edge budget. A persistent edge block therefore turns a 45-second terminal failure into roughly forty requests against an actively-rejecting WAF over several minutes before the turn ends. It is abortable and it does not corrupt anything, so this is not a blocker — but the transport already owns the retry budget for this exact condition, and the sentinel's whole meaning is "that budget is spent". Consider returning retryable: false, or carrying a retryAfterMs that reflects what has already been spent.
Three P3s, none blocking:
durableProviderErrorClass(:457-463) now passesProviderUnavailablethrough for every classification, not just this one. Needed for the Codex case, but it also changes the durable diagnostic for structuredserver_errorpayloads whose status lives in a nested source. Telemetry class only, no runtime behavior.- The cross-module contract is four bare string literals plus an error
namein two more places, none imported from a shared export. Worth a single exported constant. Note that the classification does not depend on the provider's message text, which is good — but it does depend on the body being HTML-shaped (subscription-model-fetch.ts:268-277). If Codex's edge ever emits a JSON body with an HTML content type, theJSON.parsepre-check returnsfalse, no retry happens at all, and #3959 is unfixed for that shape. I could not verify what the edge actually emits; that claim rests on the issue's analysis. - The
PROVIDER_UNAVAILABLE_PROVIDER_CODEScheck moved below the/^5\d\d$/check (:757-766). Both branches return the same class, so it is a no-op reorder adding diff noise to a carefully ordered function. Drop it.
Ungraded note on the body: it says the sentinel is emitted "only after all three replayable edge retries have actually been consumed", which is true, but the implication that the final failure is an edge rejection is not — that is the P1 above.
Also worth coordinating with #3812, which touches the same classifier. They merge cleanly and do not miscompose today, but after both land providerRetryMetadata answers "retryable?" from three independent authorities: the classified class, a raw HTTP status range, and an isRetryable flag walked from the cause chain. Once #3812 exists, this PR could arguably be implemented entirely in the transport by setting that flag on its own error, with no classifier change at all. Worth picking one seam before both land.
AI use: Claude Code (Opus) produced the initial findings; I then independently re-verified each graded one against the PR head b9e95c8 — reading the throw site and openAiCodexHttpError, the classifier's branch ordering, isTrustedCodexEdgeRejection, and the normalizeProviderFailure → providerFailureSummary chain that populates the envelope's code. The second bypass at :796 is from that re-verification, not from the initial pass. No scenario was reproduced at runtime. The reviewer of record reviewed and accepted this.
简体中文
为 "Codex 边缘在整个有界重试预算内持续返回 HTML 403" 这一情形引入专用内部哨兵,并让分类器把它读作 ProviderUnavailable 而非 Auth,使边缘/WAF 事故不再提示用户重新认证(#3959)。
问题是真的,方向也对——WAF 返回的 403 不是一个授权答复,把它当作授权答复会让用户去修一个从未损坏的凭据。重试预算、retry-after 处理和"出处校验"的思路都是好的直觉。Approve 前有三处需要改,其中两处就在本 PR 引入的信任边界上。
P1 —— 哨兵按重试计数器打戳,而非按真正失败的东西打戳(subscription-model-fetch.ts:217)。三次边缘重试用尽后,该请求上任何后续失败都会被打成"边缘拒绝已耗尽",真实的 provider code 被丢弃。第 4 次尝试上过期的令牌会被报成服务商故障并继续重试。细节与只需一个合取项的修法见行内。
P1 —— 出处校验有两条独立的绕过路径,且都由本 diff 引入::40 的 PROVIDER_UNAVAILABLE_PROVIDER_CODES 条目恰好把分类授予了信任检查已经拒绝的那些载荷;而信任函数自身 :796 的 model_failure 分支接受一个可伪造的 code,该 code 由 normalizeProviderFailure 直接从服务商载荷里抄过来。两条均见行内。需要说明:这不是"假想攻击者"式的反对——现实情形是用户自配的 OpenAI 兼容代理,后果也是有界的。它重要是因为:一个在引入它的同一 diff 里就被击穿的守卫,会被下一次改动当作可靠前提来依赖。
P2 —— 把"预算已耗尽"的信号交给了第二个重试权威,而后者重启了它本要终结的预算。 无 retry-after 的 ProviderUnavailable 在 providerRetryMetadata(:248-258)返回 {retryable: true},而 ai-sdk-backend.ts:2583 最多重试 MAX_PROVIDER_ATTEMPTS_PER_STEP = 10 次,每次外层尝试又重新进入内层 2/10/30 秒的边缘预算。因此持续性的边缘封锁会把一次 45 秒的终态失败,变成数分钟内对一个正在主动拒绝的 WAF 发起约四十次请求。它可被中止、不破坏任何状态,所以不阻塞——但传输层本就为这一条件持有重试预算,而哨兵的全部含义正是"该预算已经花完"。可考虑返回 retryable: false,或携带一个反映已花费预算的 retryAfterMs。
三条 P3,均不阻塞:
durableProviderErrorClass(:457-463)现在对所有ProviderUnavailable分类放行,而不只是本情形。Codex 这条路径需要它,但它同时改变了那些状态藏在嵌套源里的结构化server_error载荷的持久诊断分类。仅影响遥测类别,不影响运行时行为。- 跨模块契约是四个裸字符串字面量,外加两处错误
name,都没有从共享导出引入。值得抽一个导出常量。要肯定的是:分类不依赖服务商的可读消息文本;但它依赖响应体呈 HTML 形状(subscription-model-fetch.ts:268-277)。若 Codex 边缘某天以 HTML content-type 返回 JSON 体,JSON.parse预检返回false,将完全不再重试,#3959 对该形状仍未修复。我无法核实边缘实际输出什么,这一点依赖 issue 的分析。 PROVIDER_UNAVAILABLE_PROVIDER_CODES检查被移到了/^5\d\d$/检查之后(:757-766)。两个分支返回同一类别,因此是空操作重排,给一个精心排序的函数增加了 diff 噪声。建议撤掉。
关于正文的未定级说明:文中称哨兵"仅在三次可重放边缘重试全部真正耗尽后"才发出,这是对的;但由此暗示最终失败就是一次边缘拒绝,则不成立——即上面那条 P1。
另外建议与 #3812 协调,它改动同一个分类器。两者能干净合并,今天也不会错误组合;但两个都落地之后,providerRetryMetadata 将从三个独立权威回答"是否可重试":分类出的类别、原始 HTTP 状态区间,以及从 cause 链上走出来的 isRetryable 标志。一旦 #3812 存在,本 PR 其实可以完全在传输层实现——在自己构造的错误上设置该标志,完全不动分类器。建议在两者落地前先选定一个接缝。
|
This is a synthesis of the independent bounded freshness review by @Sol-404ARE at exact head What I checked myself:
Findings from Sol's review (file:line anchored): Standards — GO (0 hard violations)
Old Standards closures: AI-use, template, title/branch, and runtime boundary are now closed. Spec — NO-GO: 1×P1
Old Spec closures: JSON 403 What I did not judge:
Gate: the rebase itself is not pure tree-equivalent, and one P1 (plus one judgment P3 naming) remains; the PR cannot be considered merge-ready until the terminal sentinel also checks the final response's edge fact and the naming is corrected.
|
b9e95c8 to
af00398
Compare
|
Addressed the latest exact-head review in |
Generated-by: Codex
af00398 to
f0e751a
Compare
|
Rebased the review-complete repair onto current |
|
This is a synthesis of the independent bounded freshness review by @Sol-404ARE at exact head What I checked myself:
Findings from Sol's review (file:line anchored): Spec — NO-GO: 1×P1
Old closures: the previous P1 (final stamp must also check current response's edge fact) is now closed — the new code requires both budget exhausted and Standards — GO (0 hard violations)
Other checks: What I did not judge:
Gate: the rebase is not pure tree-equivalent and one P1
|
Astro-Han
left a comment
There was a problem hiding this comment.
All three graded findings are closed, each at the seam rather than at the symptom.
P1 (sentinel stamped from the retry counter) — subscription-model-fetch.ts:220 now requires edgeRetry === edgeRetryDelaysMs.length && isTransientOpenAiCodexEdgeRejection(response, detail), so an expired token on attempt 4 is no longer reported as an edge outage. That was the one that mattered most: it made the fix reproduce the shape of the bug it exists to fix, in the other direction.
P1 (two bypasses of the provenance check) — both gone. The PROVIDER_UNAVAILABLE_PROVIDER_CODES entry is removed, and isTrustedCodexEdgeRejection is now only the cause-chain walk for an Error named OpenAiCodexEdgeRejectionError. The forgeable model_failure clause that accepted a provider-supplied code is deleted, so the guard is no longer defeated inside the diff that adds it.
P2 (the exhausted signal handed to a second retry authority) — providerRetryMetadata short-circuits with retryable: false ahead of the generic ProviderUnavailable branch, and the comment states why. A persistent edge block now ends at roughly 45 seconds instead of fanning out to about forty requests against an actively-rejecting WAF.
P3 (durableProviderErrorClass) — tightened further than I suggested: ProviderUnavailable passes through only when isTrustedCodexEdgeRejection(facts), so structured server_error payloads keep their previous durable class.
One P3 remains, non-blocking: the cross-module contract is still string literals. OPENAI_CODEX_EDGE_REJECTION_CODE exists at provider-error-classification.ts:38 but isn't exported, and subscription-model-fetch.ts:352 plus the OpenAiCodexEdgeRejectionError name are still spelled out by hand in both modules — two producers and two consumers agreeing by spelling.
Still worth coordinating with #3812 before both land (it's currently red), since afterwards providerRetryMetadata answers "retryable?" from three independent authorities.
AI-assisted review: I verified each fix against the current head myself — the sentinel conjunct, the removal of both bypasses, the short-circuit ordering in providerRetryMetadata, and the tightened durableProviderErrorClass condition. No tests run. AI review is not independent human review.
简体中文
三条定级发现全部关闭,且都修在接缝上。哨兵现在要求"重试预算耗尽且本次失败确为瞬态边缘拒绝",不再因为计数器耗尽就把第 4 次尝试上的过期令牌误报成边缘故障——那一条最要紧,它让修复以相反方向复现了它要修的 bug 的形状。两条绕过路径均已删除,信任检查只剩 cause chain 上的真实出处。providerRetryMetadata 在通用分支之前短路返回 retryable: false,持续边缘封锁在约 45 秒终止,而非扩散成约四十次请求。durableProviderErrorClass 收得比我建议的更紧。
剩一条 P3 不阻塞:跨模块契约仍靠字面量拼写一致,OPENAI_CODEX_EDGE_REJECTION_CODE 未导出。
Summary
Fixes #3959
Preserve the distinction between Codex JSON authorization failures and transient HTML edge rejections after the bounded retry budget is exhausted. The transport emits a dedicated internal sentinel only when all three replayable edge retries have actually been consumed and the terminal response is still the same HTML edge rejection. Parseable JSON wins over a misleading
Content-Type, non-replayable bodies are never stamped as exhausted edge failures, and provider classification trusts only the internally constructed error provenance.The exhausted sentinel is
ProviderUnavailablefor presentation but non-retryable at the outer model loop, because the transport has already spent its complete 2/10/30-second retry budget.Verification
@maka/core,@maka/storage,@maka/mcp,@maka/computer-use, and@maka/runtimenode --test "packages/runtime/dist/__tests__/subscription-model-fetch.test.js" "packages/runtime/dist/__tests__/provider-error-classification.test.js"(36 passed)Regression coverage includes the original 2/10/30 second HTML edge retry budget, JSON auth with an incorrect HTML content type, non-replayable HTML responses, three HTML retries followed by a JSON auth failure, spoofed sentinel codes, JSON 403
server_errorremaining Auth, and the trusted exhausted-edge diagnostic projection.AI use
Select exactly one:
Tool(s) and scope: Codex authored the implementation, tests, and review follow-up; the human contributor reviewed the changes and owns the submission.
Checklist
Does this PR entail a change in behavior?