Skip to content

fix(stream): harden streaming external tool calls, error wire shapes, and 503 retries - #34

Merged
samson910022 merged 30 commits into
mainfrom
fix/external-tool-streaming-errors
Oct 24, 2026
Merged

samson910022 merged 30 commits into
mainfrom
fix/external-tool-streaming-errors

Conversation

@samson910022

Copy link
Copy Markdown
Owner

Summary

This PR addresses all known gateway issues identified during deployed container testing and deep-dive investigations, adapting best practices from /home/samson1357924/projects/magpie.

1. Phase 1 — Tool-Call Streaming Assembly & Authoritative Snapshots

  • Collector Re-architecture (src/stream/collector.ts): Replaces lossy delta-only stream accumulation with sequence-based per-part reconciliation between incremental deltas and full snapshots (message.part.updated part.text, message.updated info.parts).
  • Single Authoritative Source (src/tool-runtime/stream-source.ts): Resolves a single authoritative source group across Chat, Messages, and Responses, eliminating false malformed_external_tool_call errors caused by validating raw deltas and snapshots independently.
  • Targeted Single Snapshot Repair: On malformed raw markup, routes perform a single snapshot recovery poll and continue if valid.
  • Single-Pass Visible Text Derivation: Under shouldBufferExternalStream, visible text is stripped once from the authoritative source, completely preventing unclosed XML tags or raw tool-call JSON from leaking into client text deltas.
  • Router Reminder: Replaces invalid JSON placeholder {...} with valid JSON {} in buildExternalToolsReminder.

2. Phase 2 — SSE Framing, Block Closing & Stream Closure

  • Keepalive Framing: Fixed keepalive/heartbeat comments to single newline : comment\n (instead of \n\n), eliminating phantom empty SSE events across all routes.
  • Reverse Proxy Header: Added X-Accel-Buffering: no to all streaming route responses.
  • Anthropic Block Closing Before Errors (messages.ts): Open text and thinking blocks are cleanly closed via content_block_stop before emitting event: error.
  • Outer Catch Socket Closure: Outer route catch handlers across all four streaming routes detect unhandled rejections after res.headersSent and invoke res.end() with wire-compliant error framing.
  • Natural Stream Cut Detection: Collector finalizes natural event-stream iterator completion, flags premature cuts (cut: true), and clears all timers.
  • Wire Shape Integrity: Confirmed strict wire formats without stray [DONE] markers on Chat, Messages, or Interactions error streams.

3. Phase 3 — Body-Parser Error Handler, Auth-Before-Parse, & Hardening

  • Terminal Error Middleware (src/errors/http.ts, system.ts): Added pure classifier and 4-argument Express error handler mapping entity.parse.failed (400), entity.too.large (413), charset/encoding.unsupported (415), and entity.verify.failed (400) to protocol-shaped JSON errors without leaking stack traces or internal paths.
  • Auth Precedence (src/proxy.ts): Mounted auth verification before body parsing so unauthenticated requests are rejected with 401 immediately without consuming or parsing large/malformed payloads.
  • Hardening: Disabled X-Powered-By header and added ENV NODE_ENV=production in the runtime Dockerfile stage.
  • Explicit 404: resolveRequestedModel explicitly throws with statusCode = 404 and code = 'model_not_found'.

4. Phase 4 — 503 Streaming Retries, Concurrency & Caching

  • Streaming 503 Retries: Added transient 503 retry loops on Responses stream, Messages stream, and Interactions before any delta output.
  • Parent Session Protection: Continuation requests with previous_response_id preserve the parent multi-turn session on retry and delete only ephemeral attempt sessions.
  • Fail-Closed Enforcement: Enforced strict fail-closed termination after partial output has streamed.
  • Transient Normalization (src/errors/upstream.ts): Added data['statusCode'] and /endpoint is unavailable/i to isTransientUpstreamError.
  • Lock Separation (chat.ts, messages.ts): Moved model resolution outside the process-global lock(), allowing unknown model requests to fail fast with 404 in milliseconds without waiting on concurrent streaming requests.
  • Provider Caching (src/proxy.ts): Added 30s TTL cache for getProvidersList with error and backend reconnect invalidation.

5. Phase 5 — CI Alignment & Documentation

  • Workflow Pins: Aligned .github/workflows/ai-fix.yml, ai-review.yml, and ai-scan.yml to opencode-ai@1.18.35 matching Dockerfile ARG OPENCODE_VERSION=1.18.35.
  • Automated Pin Verification: Extended tests/dockerfile-pin.test.js to validate workflow pins against PINNED_VERSION.
  • Release Documentation: Updated CHANGELOG.md with all changes.

Verification

  • npm run typecheck: Passed (0 errors)
  • npm run build: Clean build to dist/
  • npm test -- --runInBand: 36 test suites passed, 869 tests passed, 0 failures
  • Security & Hygiene: Zero secrets in diff, .gitignore preserved untouched, temporary test artifacts omitted.

…l tool calls

- Track per-part stream state with sequence-based snapshot and delta reconciliation
- Resolve a single authoritative stream source across Chat, Messages, and Responses
- Add targeted single snapshot repair when raw tool markup is malformed
- Derive visible assistant text in one pass under external tool buffering
- Replace invalid JSON placeholder in external tool reminder with valid JSON
- Add deterministic collector, source resolution, and route snapshot regression tests
…r termination

- Write bare keepalive comments without trailing newline to prevent phantom empty SSE events
- Add X-Accel-Buffering: no header across all streaming endpoints
- Close open Anthropic content and thinking blocks before emitting stream errors
- Ensure outer catch handlers across all four streaming routes close open sockets with res.end()
- Finalize natural event-stream iterator completion and detect cut streams in collector
- Preserve protocol-specific streaming error wire shapes without stray [DONE] markers
- Add comprehensive Phase 2 regression tests
…, and security hardening

- Add pure HTTP error classifier and terminal 4-argument Express error middleware
- Mount auth middleware before body parsing to block unauthenticated payload processing
- Disable Express X-Powered-By header and add NODE_ENV=production in runtime container
- Set explicit 404 status code and code in resolveRequestedModel
- Add comprehensive HTTP error, security non-leakage, and protocol-shaped wire tests
… fast-fail model resolution

- Add transient 503 retry loops on Responses stream, Messages stream, and Interactions before output
- Preserve multi-turn continuation parent sessions during stream retry
- Enforce fail-closed termination after partial output has streamed
- Recognize data.statusCode: 503 and Endpoint is unavailable in isTransientUpstreamError
- Cache provider models list for 30s with reconnect and error invalidation
- Resolve requested models before acquiring the global lock to fail fast on 404
- Add comprehensive streaming retry, lock-free fast fail, and provider caching tests
…ease changes

- Align opencode-ai pin in AI workflows to 1.18.35 matching Dockerfile
- Add automated verification in tests/dockerfile-pin.test.js
- Document all four phases of streaming, error, retry, and hardening fixes in CHANGELOG.md
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Verdict matrix

  • Roles: gateway_safety, api_compat, docs_config
  • FINAL_VERDICT: COMMENT

Sensitive paths

  • .github/workflows/ai-fix.yml
  • .github/workflows/ai-review.yml
  • .github/workflows/ai-scan.yml
  • Dockerfile
  • src/backend/manager.ts
  • src/errors/upstream.ts
  • src/proxy.ts
  • src/routes/chat.ts
  • src/routes/interactions.ts
  • src/routes/messages.ts
  • src/routes/responses.ts
  • src/routes/system.ts
  • src/stream/collector.ts
  • src/tool-runtime/router.ts
  • src/tool-runtime/stream-source.ts

Deterministic pre-checks

  • (none)

gateway_safety

VERDICT: APPROVE

The changes harden stream lifecycle management, external tool assembly, and error handling across all protocols while maintaining all [provider] safety invariants, tenant isolation, and wire protocol shapes.


BLOCKING

(None)


SHOULD_FIX

(None)


NITS

  • PR Description: The PR body references a local workstation filesystem path; contributors should avoid embedding local absolute user directory paths in PR descriptions or commit metadata to maintain metadata privacy.

Checklist Verification Summary

  1. No secrets committed or printed: No .env, config.json, or token values present in code or test fixtures. All idempotency tenant scopes and payload hashes use SHA-256 digests (idempotencyDigest, canonicalJsonHash).
  2. Auth discipline: Preserved. Global auth executes prior to body parsing, rejecting unauthenticated requests before consuming or parsing payloads (src/proxy.ts:168-202). Multi-turn continuation states and idempotency scopes enforce strict per-tenant separation by API key digest (src/proxy.ts:462, src/idempotency/middleware.ts:503-519).
  3. Tool safety: External tool execution remains sandboxed; tool namespacing (external__*) is intact; no tool-output bodies or client payload text are logged in diagnostic logs.
  4. Bind/port matrix: Unchanged and consistent across runtime configurations.
  5. Backend lifecycle: Home isolation and spawn protections remain untouched. Clean lifecycle resource disposal added (ctx.dispose(), clearBackendReconnectListeners(), unreferenced cleanup timers).
  6. Error classification & transient matchers: isTransientUpstreamError safely accounts for HTTP 408 and unavailable endpoint strings while filtering non-retryable 4xx errors (src/errors/upstream.ts:166-171), fully backed by dedicated test suites (tests/backend-error-surface.test.js, tests/stream-retries.test.js).
  7. Log hygiene: Request parameter logging restricted to metadata (temperature, max_tokens, top_p, reasoning_effort); no Authorization headers or secret tokens logged.
  8. CI & Dependencies: Clean version alignment for [provider]-ai@1.18.35 across all GitHub Actions workflows matching Dockerfile ARG OPENCODE_VERSION; undici upgraded to 8.12.3; no invalid lint scripts or illegal index.ts test imports.

api_compat

API Compatibility Review

I'm reviewing this large PR against the multi-protocol wire-shape compatibility checklist. The diff is truncated, so I'll note where evidence is incomplete.


VERDICT: NEEDS_CHANGES


BLOCKING:

  1. src/errors/upstream.ts: 4xx filter reorders isRetryable check after strict 4xx rejection, potentially breaking retry semantics for provider-marked retryable 403/404 errors.
    src/errors/upstream.ts:164-168: The new code rejects all 4xx except 401/402/408/429 before checking isRetryable. Previously, isRetryable === true won over any status code. A provider SDK that marks a 403 or 404 as isRetryable: true (e.g., a transient permission propagation delay) will now be treated as permanent. This inverts the documented "provider-explicit retryable flag keeps winning (upstream-faithful)" contract.

    // NEW: 4xx filter runs BEFORE isRetryable
    if (typeof status === 'number' && status >= 400 && status < 500) {
      if (status !== 401 && status !== 402 && status !== 408 && status !== 429) {
        return false;  // <-- kills isRetryable=true for 403, 404, etc.
      }
    }
    // ... later ...
    if (record['isRetryable'] === true || data['isRetryable'] === true) return true;

    The old code had isRetryable first. If this reordering is intentional (to prevent retrying model_not_found 404s), the auth-failure check below it also becomes unreachable for 401 since the 4xx filter already allows 401 through — but the comment still says "Placed after isRetryable". The comment is now stale and misleading.

    Fix: Either restore isRetryable priority over 4xx (except the specific codes you want to hard-reject like 404 model_not_found), or explicitly document that provider isRetryable is no longer authoritative for non-allowlisted 4xx. At minimum, update the stale comment.

  2. src/proxy.ts:418: resolveRequestedModel now throws with statusCode = 404 but the chat route error handler (and potentially messages/interactions) may not map 404 to the correct wire shape for each protocol.
    src/proxy.ts:418: The error object gets statusCode = 404 and code = 'model_not_found'. For the Anthropic Messages route, a 404 must produce { type: 'error', error: { type: 'not_found_error', message: ... } }. The new toAnthropicErrorType function in src/converters/anthropic.ts correctly maps 404 → not_found_error, but I cannot verify from the truncated diff that the Messages route catch handler actually calls toAnthropicErrorType for this thrown error. If it falls through to a generic handler that uses invalid_request_error, the Anthropic wire shape is violated.

    Evidence needed: Confirm src/routes/messages.ts catch block uses toAnthropicErrorType(404) or equivalent for model resolution failures. The diff is truncated at this point.


SHOULD_FIX:

  1. src/proxy.ts:454: storeResponseState signature change adds required checkpointId and tenantId parameters — all existing callers must be updated.
    The function now requires checkpointId (string, non-empty) and tenantId (string). Since the diff is truncated, I cannot verify all call sites in responses.ts, interactions.ts have been updated. If any caller passes undefined for checkpointId, storeResponseState silently returns false and the response state is lost, breaking previous_response_id continuation.

  2. src/routes/chat.ts: Model resolution moved outside lock() — race condition with ensureActiveModel.
    src/routes/chat.ts:~170-180: resolveRequestedModel now runs before lock(), but ensureActiveModel (which PUTs the model to the backend) presumably still runs inside lock(). If two concurrent requests resolve different models outside the lock, they'll both enter the lock sequentially, but the first one's ensureActiveModel PUT may be overwritten by the second before the first's stream completes. This was previously serialized. The PR description says this is intentional for fast 404 fail, but it may cause model-switching races for valid models.

  3. src/proxy.ts: createApp return type now includes ctx — verify CreateAppResult type is updated.
    src/proxy.ts:1333: return { app, client, ctx } and src/proxy.ts:1355: const { app, ctx } = createApp(config). The CreateAppResult interface (not shown in diff) must include ctx or TypeScript will error. The PR says typecheck passes, so this is likely fine, but the type definition change is not in the diff.

  4. src/errors/upstream.ts:216: New anchored pattern ^\s*endpoint is unavailable\.?\s*$ is very strict.
    This only matches if the entire message is "endpoint is unavailable". If the upstream wraps this in a longer message like "503: endpoint is unavailable", the anchored regex won't match. However, readStatusCode would catch the 503: prefix separately, so this may be fine for the status-code path. Just noting the tight coupling.

  5. src/idempotency/middleware.ts: SSE terminal event validation for Interactions expects interaction.completed but the wire shape spec says errors use {type:'error'} without [DONE].
    src/idempotency/middleware.ts:~230: isSuccessfulResponse for non-Responses endpoints checks for interaction.completed as the terminal SSE data event. This is correct for success, but if an error stream is captured (which would have {type:'error'} as terminal), isSuccessfulResponse returns false, and the record transitions to unknown for submitted prompts. This seems intentional but means error responses for submitted Interactions prompts are never replayable — they always require reconciliation. Document this limitation.

  6. src/proxy.ts: rejectRequestAndClose sets Connection: close and calls req.resume() — verify this doesn't interfere with Express's own connection management on keep-alive connections.
    Setting Connection: close on auth rejection is aggressive. For high-throughput scenarios with many keys, a single bad key closes the TCP connection for the next pipelined request too. This is a minor operational concern.

  7. src/proxy.ts: Content-Type validation middleware uses req.once('data', onData) for Transfer-Encoding without Content-Type — potential timing issue.
    src/proxy.ts:~220-240: If body-parser has already started consuming the stream (it hasn't, since this middleware runs before it), the data event would be missed. The ordering is correct here (this runs before bodyParser.json), but the pattern is fragile if middleware order changes.


NITS:

  1. CHANGELOG.md: CVE references (CVE-2026-*) appear to be future-dated (2026). These look like placeholder or speculative CVE IDs. If undici@8.12.3 is a real version addressing real CVEs, verify the CVE numbers are accurate. If these are fictional/projected, they should not appear in a public changelog.

  2. src/errors/http.ts:34: isAnthropicMessagesPath splits on ? and lowercases — this duplicates logic that could be shared with the content-type middleware's path normalization.

  3. src/idempotency/registry.ts: canonicalize uses recursive string concatenation which is O(n²) for deeply nested objects. For the expected payload sizes (chat/interaction requests), this is fine, but the MAX_CANONICAL_NODES = 1_000_000 limit is generous enough to allow slow canonicalization on adversarial inputs.

  4. src/backend/manager.ts: backendReconnectListeners is module-global, not per-app-instance. If multiple createApp instances exist (e.g., in tests), all apps receive all reconnect notifications. The clearBackendReconnectListeners export mitigates this for tests but is a footgun.

  5. Dockerfile: ENV NODE_ENV=production is set in the runtime stage — good practice, no issue.

  6. Workflow pins: All three workflows correctly pin to 1.18.35 matching the Dockerfile ARG. ✓


Test Gaps:

  • Truncated diff: Cannot verify test coverage for Messages route 404 wire shape, Interactions 503 retry loop, or storeResponseState callers with new signature.
  • isTransientUpstreamError reordering: The test file tests/backend-error-surface.test.js is listed as changed but not shown. Need to verify it covers the isRetryable: true + 403/404 scenario under the new ordering.
  • Idempotency: New test files (tests/idempotency.test.js, tests/continuation-fork.test.js) are listed but not shown. Cannot verify they cover the unknown → reconciliation → 503 idempotency_reconciliation_pending path.
  • Content-Type 415 rejection: tests/http-errors.test.js is listed but not shown. Need to verify it covers the Transfer-Encoding-without-Content-Type edge case.

Cross-Protocol Regression Risks:

  1. The resolveRequestedModel 400→404 status change is correct per the spec (model_not_found should be 404), but all four route error handlers must be verified to emit the right wire shape for 404. Only the chat route's handling is partially visible.
  2. The storeResponseState signature change is a breaking internal API change — if any route still calls the old 4-argument form, continuation chaining silently breaks (returns false).
  3. Moving model resolution outside lock() changes the concurrency model for chat and messages. While this enables fast 404, it means model resolution and session creation are no longer atomic, which could cause subtle issues if the provider list changes between resolution and session creation.

docs_config

Verdict: APPROVE

The PR maintains documentation consistency, respects the EN-first convention for root documentation and changelog entries, and does not alter or break the six-way config matrix (no new environment configuration keys were introduced; all idempotency and cache limits are in-memory constants).


BLOCKING

(None)


SHOULD_FIX

(None)


NITS

  • CHANGELOG.md:16-17: The Undici security release entry mentions Node.js October 2026 Security Releases (October 21, 2026) and several CVE-2026-* identifiers (likely a typo for 2024/2025 release dates).
  • docs/api-reference.md:305: Heading ### 🌊 流式输出与反向代理缓冲 / Streaming & Reverse Proxy Buffering puts Chinese first, whereas line 245 (### Continuation Idempotency and Retries / Continuation 幂等与重试) uses EN-first. Recommend making heading order consistent.

Config Matrix Status

  • Environment variables introduced / changed: None.
  • Files requiring matrix sync: None (no six-way matrix drift detected; .env.example, config.json.example, Dockerfile, docker-compose.yml, index.ts, and docs/configuration.md remain aligned).

FINAL_VERDICT: COMMENT

Session: response_id=resp_6W7caqDBPI331e8PqtnXsAM,resp_req_vrtx_011CgLkzFC3daC5etqdLYUnE,resp_aW7careYI8aZ0-kPx4Sj4A0 (chain via previous_response_id, 30min TTL). model=[model] provider=[provider].

samson910022 commented Oct 21, 2026 •

Copy link
Copy Markdown
Owner Author

PR #34 — Independent Deep Re-review(2026-10-24,更新原留言)

Reviewed head: 2b8dcbe24295cce733f42b1b664e429d3113a1d3(相較前次 d9df7ae2da10 新增 4 commits;PR 目前 41 changed files,+15,455 / −1,024)。
FINAL VERDICT: APPROVE (code-review merge gate; P2 follow-ups below are non-blocking).

本次從最新原始碼獨立複核前一輪 2 項 blocker、Idempotency-Key / in-memory Registry、Responses / Interactions child-session fork、collector 本輪 ID 限定、Anthropic SSE wire shape、HTTP error handling、相關測試與 GitHub Actions 實際日誌,再核對最新 AI Review Bot(gateway_safety: APPROVE、docs_config: APPROVE、api_compat: NEEDS_CHANGES)。沒有再找到足以阻擋合併的已確認程式碼問題。 這是 code-level review + 自動化測試證據,並非真實部署的故障注入 / SDK end-to-end 認證;此處 APPROVE 是此留言的審查結論,並未提交 GitHub formal approval review event。

✅ 前次 Blocking #1:Jest failed — RESOLVED

最新 CI run #40078760373 = SUCCESS。 unit-test job 的 npm ci、npm run typecheck、npm run build、npm test -- --runInBand 全部成功;38 / 38 suites、1,136 / 1,136 tests passed。Docker build + /health smoke、ShellCheck、ai-bot-lint 全部成功;AI Review Bot workflow 自身亦正常完成。

上次 tests/idempotency.test.js 的 required-tool reconciliation 單測將 session.messages 恰好 2 次的實作細節斷言,改成 >=2,但維持 sessionPrompt === 2、forcedPromptCalls === 1、兩次請求皆 503 pending 的語義斷言(tests/idempotency.test.js:2287-2345)。額外 poll 不等於額外 prompt 或工具副作用;此調整合理,非靠移除核心防護讓測試變綠。

✅ 前次 Blocking #2:Pre-submit 暫時性 503 被 sticky-cache — RESOLVED

目前 src/idempotency/middleware.ts:273-302 的 isDeterministicPreSubmitFailure() 只把確定性的 4xx(排除 408/429、以及 JSON retryable:true/ transient_error)視為可重播的已知失敗;status >=500 都不算確定。於 src/idempotency/middleware.ts:430-452,若 !record.submitted 且為 transient,會 abandon 並釋放 Key(不再快取整個 503);若已提交則仍標 unknown,只能 reconcile,不會重送 prompt。

這次補上 actual route-level 回歸(tests/idempotency.test.js:2468-2765),包含:(A) Responses + Interactions 第一次 session 建立就 503 → 同 Key / payload 第二次 200、跨兩次請求只 prompt 1 次;(B) deterministic 400/422 重播;(C) 已提交後 503 / 斷線只能 reconcile、不能重 prompt。符合上一輪指定的安全重試區別。

✅ 本 PR 的跨 HTTP continuation / streaming 核心行為複核

  • Key 語義符合已選定方案 1 + 3: src/idempotency/middleware.ts:493-614 的 scope = hashed tenant + normalized endpoint + parent ID + hashed Key;同 key 不同 canonical payload → 409;同 key 同 payload 的已完成回應完整 replay;in-flight → retryable 409;unknown retry → claim lease、沿用唯一 child/user message ID,並使用精確本輪終止訊息確認。無 Key 時不會把相同 payload 視為同一邏輯請求。Interactions 兩個 endpoint alias 共用 scope。
  • 不同 Key 可合法分支: src/routes/responses.ts:683-711、src/routes/interactions.ts:376-410,新 continuation fork parent session,不變更 parent transcript;同一 Key 的不明結果僅 poll 已記錄 child,絕不重新 fork / prompt;tests/continuation-fork.test.js、tests/idempotency.test.js 有對應 stateful 測試。
  • Stale assistant 防護: src/stream/collector.ts:267-302 的 session.messages poll 用 expectedCurrentUserMessageId 過濾 assistant 的 parentID,避免把上一回合完整的 A1 誤當本回合 A2;idempotent 完成還需 complete:true + nonempty assistant ID + matching user ID。
  • 可觀測限制已清楚文件化: Ledger 是每 app instance 的 in-memory state;完成記錄 TTL 預設 60 分鐘;unknown 不會 TTL 自動淘汰;多程序/restart 不保證 exactly-once。這與明確選定的非持久化語義一致,不能把它解讀成分散式 exactly-once 承諾。

⚠️ P2 / Follow-ups(非阻擋;建議小範圍獨立修補)

P2-A. Anthropic SSE 在 reasoning/text 多次交替時可能重用已關閉的 content_block index。 src/routes/messages.ts:603-653 的 ensureTextIndex() / ensureThinkingIndex() 對每種類型僅分配一次 index,即使 sendTextDelta() / sendReasoningDelta() 交替關閉對方的 open block,後續重新開啟同種類 block 仍重用舊 index。

具體序列(不宣稱此事件在主流模型每次都會出現):如果 OpenCode 推送 reasoning('A') → text('B') → reasoning('C'),路由會輸出 content_block_start(index:0, thinking) / stop(0)、start(index:1, text) / stop(1)、然後再次 start(index:0, thinking)。已關閉的 index 被重新啟用,不符合一般 Anthropic content array block 順序預期。現有新增測試只檢查單次 reasoning → text(tests/messages-anthropic.test.js:669-760)。建議重開 block 時分配 新的遞增 index(或明確限制多段交替並 fail closed),補 reasoning → text → reasoning / text → reasoning → text 斷言。因屬較少見的交錯事件形態,列 P2 而非對已修的主路徑再開 P1。

P2-B. Pre-submit 401/402 transient policy parity。 src/errors/upstream.ts:150-231 對非明確認證失敗的 upstream 401/402 可能視為可重試(例如 CreditsError),但 src/idempotency/middleware.ts:273-302 的 pre-submit classifier 未將 401/402 排除於 deterministic 4xx。如果模型提交之前的某個後端調用出現此類可恢復的 401/402,Key 仍可能被快取 60 分鐘。建議未來以統一、明確的 retryable 分類 policy 評估 pre-submit failure,並補 CreditsError 與真正 invalid-key 401 反例;不要粗暴地把所有 401 當 transient。這個狀況需在實際 pre-submit 路徑可重現才升為 blocker,目前未找到可證實的主流觸發路徑。

其他 nits:PR description 可移除本機路徑;SSE 純 comment keepalive 的單/雙換行選擇可用目標 SDK/反向代理端到端測試鎖定;根 README 的 Idempotency-Key / 404 說明現已與雙語 docs 同步。避免為這些非阻擋項目讓本 PR 持續膨脹。

🤝 對最新 AI Review Bot 的獨立複核

  • gateway_safety / docs_config 的 APPROVE:同意其已驗證的安全與設定面向。 注意 AI review workflow 成功僅代表工作流程執行成功,不是 bot 所有子角色都 APPROVE。
  • api_compat B1(isRetryable 優先序): 現在 400/403/404 等非 allowlisted 4xx 刻意先 fail fast,而且 tests/stream-retries.test.js:138-161 明確 assert isRetryable:true 的 400/404 仍不可重試。這是現在的安全政策,不是無意間破壞的回歸;是否要允許 provider-marked 403 transient 是獨立產品政策,不能未經定義就把 403/404 全面變可重試。註解也已表述「unless filtered out」。
  • api_compat B2(Messages 404 wire 未驗證):已確認。 src/routes/messages.ts:1190-1202 和外層 error catch 均呼叫 toAnthropicErrorType(),且 tests/http-errors.test.js:616-631 驗證不存在 model 回傳 HTTP 404 + Anthropic not_found_error,不是現存 blocker。
  • storeResponseState 新參數可能漏傳: 已核對 Responses stream/non-stream(src/routes/responses.ts:1969-1981 / src/routes/responses.ts:2388-2401)、Interactions stream/non-stream(src/routes/interactions.ts:600-645),都有 checkpoint ID + tenant ID;加上本輪 typecheck 全綠,未確認有舊簽名 caller。
  • model resolution outside lock 的 race: 僅 fail-fast resolution 提前;Chat/Messages session 操作仍在原 lock 範圍,ensureActiveModel 執行於 lock 內。Bot 描述「第二個請求必然在第一個 stream 結束前覆寫 active model」缺乏該執行序列證據,不能當作 blocker。
  • Interactions error SSE replay: middleware 會把已確認 terminal error 且已送完整 wire response 的 submitted request 快取為 failed 後重播;Bot 所說 submitted error 一律永遠 unknown 並不成立。
  • CVE 日期 / docs 名稱 / Content-Type gate / cache pruning:多為文件或未核實情境;2026-10-21 對本次 2026-10-24 審查不是未來日期。未將 diff truncation 當成程式碼失敗證據。

Final Merge Gate

APPROVE for merge from this independent code review, subject to ordinary branch protection / CI remaining green and no additional unreviewed commits. 前次兩個 blocking issues 都已解決;已做 source + tests cross-check,沒看到新的 P1。P2-A / P2-B 建議以獨立、窄範圍修補改善相容性,不需要再把已驗證的 idempotency / continuation 主路徑重新翻修。

— Updated in place: original PR Conversation comment #6324878715 (replaces prior review of d9df7ae2da10).

@samson910022
samson910022 merged commit e340259 into main Oct 24, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant