Skip to content

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

Open
samson910022 wants to merge 21 commits into
mainfrom
fix/external-tool-streaming-errors
Open

samson910022 wants to merge 21 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

BLOCKING:
(none)

SHOULD_FIX:
(none)

NITS:

  • src/routes/interactions.ts:377: The redundant assignment if (parentSessionId) { promptSubmitted = true; } immediately before awaitWithAbort(...) and promptSubmitted = true; is slightly repetitive, though functionally safe for preventing duplicate turns on continuation retries.

Safety Invariants Audit

  1. Secret & Key Hygiene: Verified no secrets, .env files, internal paths, or credentials committed or printed in logs. Error logging formats metadata and sanitized messages only (toErrorMessage, readDataMessage).
  2. Auth Discipline: Auth verification middleware is placed before bodyParser, rejecting unauthenticated payloads with 401 without consuming or parsing request bodies. Trailing slash normalization cleanly preserves auth bypass for /health, /health/details, /metrics, and / root health probes.
  3. Wire Shape & Streaming Framing: Keepalive SSE framing is fixed to : keepalive\n without phantom event dispatch. All 4 routes strictly preserve their respective streaming protocol requirements (Chat [DONE], Responses SSE response.failed + [DONE], Messages event: error with open block closure and no [DONE], Interactions type: 'error' without [DONE]).
  4. Backend Concurrency & Lifecycle: Fast-failing invalid models before acquiring the sequential request lock() prevents queue starvation. In-flight getProvidersList request coalescing prevents cache stampedes, and backend reconnection invalidation listeners clean up properly via ctx.dispose().
  5. Tool Safety & Sandboxing: Single authoritative snapshot resolution avoids false malformed_external_tool_call errors while maintaining tool isolation and strict output stripping when tools are declared.
  6. CI & Packaging: Dockerfile and workflow backend dependencies are aligned to 1.18.35, runtime NODE_ENV=production is asserted, and undici is patched to 8.12.3. No test imports index.ts.

api_compat

API Compatibility Review

Summary

This is a large, multi-phase hardening PR touching all four streaming routes, the collector, error handling, retry logic, and CI pins. I'll focus on wire-shape compliance, converter integrity, session chaining, reasoning mapping, model aliases, collector semantics, retry policy, and documentation.


BLOCKING

B1. Interactions stream error framing emits data: prefix instead of bare JSON — wire shape violation
src/routes/interactions.ts outer catch (near end of file):

res.write(`data: ${JSON.stringify({ type: 'error', error: transformed.error })}\n\n`);

The Interactions wire contract (documented in the system prompt and the route's own comment: "no [DONE]; errors arrive as {type:'error'} like /v1/messages") requires event: error SSE framing, not a bare data: line. A bare data: line without an event: field is parsed by SSE clients as a message event, not an error event. This breaks clients that listen for event: error (the Anthropic/Gemini convention this route mirrors).

Expected:

res.write(`event: error\ndata: ${JSON.stringify({ type: 'error', error: transformed.error })}\n\n`);

src/routes/interactions.ts: outer .catch() handler

B2. Messages stream — open blocks not closed before error throw paths inside the retry loop
src/routes/messages.ts: The closeOpenBlocks() helper is defined at the top of the handler, but the multiple throw paths inside the retry loop (e.g., throw normalizeBackendError(recoveryRecord['error']), throw new Error('Stream ended without valid terminal'), throw normalizeBackendError(collectedR['error'])) do not call closeOpenBlocks() before throwing. The outer catch does call it (based on the truncated diff pattern), but if the catch is the lock() inner catch that re-throws, blocks may remain open when the error SSE is emitted. The PR description explicitly claims "Open text and thinking blocks are cleanly closed via content_block_stop before emitting event: error" — this must be verified for every throw path that can occur after textBlockOpen or thinkingBlockOpen is set to true.

Since the diff is truncated I cannot confirm the outer catch calls closeOpenBlocks(). If it does not, this is a blocking wire-shape violation for the Messages route (Anthropic clients expect content_block_stop before event: error).

src/routes/messages.ts: all throw paths after block-open state is set

B3. Chat route outer catch writes data: error after headersSent but never writes data: [DONE]
src/routes/chat.ts:1283-1295 (outer catch):

res.write(`data: ${JSON.stringify({ error: transformed.error })}\n\n`);

Per the chat non-stream wire shape, errors after headers are sent should include data: [DONE]\n\n to signal stream termination. The inner catch (around line 1245) also writes an error data: line and calls res.end() but does not write [DONE]. Chat SSE clients (OpenAI SDK) expect data: [DONE] as the terminal marker. Without it, clients may hang waiting for more data even though res.end() closes the TCP stream — this is fragile with reverse proxies that buffer.

src/routes/chat.ts:1245-1252 and src/routes/chat.ts:1283-1295


SHOULD_FIX

S1. unsentSuffix now returns '' on divergence — silent data loss
src/routes/chat.ts and src/routes/messages.ts: The unsentSuffix function was changed from returning the full snapshot text on divergence to returning ''. The comment says "A divergent snapshot is not a safe replay" — this is correct for preventing duplication, but it means that if the raw stream dropped characters (which is the exact scenario recovery is for), the client receives no recovery text at all. Combined with the new throw new Error('Stream ended without valid terminal') guard, this may cause unnecessary failures on streams where the snapshot is valid but diverged due to a dropped delta. Consider at minimum logging a warning when divergence is detected.

src/routes/chat.ts:607 and src/routes/messages.ts:660

S2. isTransientUpstreamError now treats HTTP 401 as retryable
src/errors/upstream.ts:178-182: The 4xx filter explicitly allows 401, 402, 408, 429 through to the transient check, and then line 228 returns true for status 401. This means authentication failures are retried up to maxAttempts times. The auth-failure short-circuit regex (invalid[_\s]?api[_\s]?key|unauthorized|...) only fires if the message string matches — a 401 with a non-matching message body (e.g., {"error":"token expired"}) will be retried. The PR description says "strict 4xx filtering (recognizing HTTP 408 Request Timeout alongside 401, 402, and 429 as retryable)" but the original code's comment explicitly says "Genuine auth failures must surface immediately." This is a behavioral regression for misconfigured API keys.

src/errors/upstream.ts:175-182 and src/errors/upstream.ts:226-228

S3. Provider cache ensureBackend call inside getProvidersList may deadlock under lock()
src/proxy.ts:195-197: When MANAGE_BACKEND: true, getProvidersList now calls ensureBackend(config). If getProvidersList is called from within lock() (which it is in the responses/interactions routes for model resolution), and ensureBackend itself acquires any shared resource or takes significant time, this could cause the process-global lock to be held for the entire backend spawn duration, blocking all other routes. The PR moves model resolution outside lock() for chat and messages but the diff is truncated — verify responses and interactions also moved resolution outside lock().

src/proxy.ts:195

S4. Interactions continuation retry creates blank sessions when parentSessionId is set
src/routes/interactions.ts:370-380: When parentSessionId is set and a retry occurs, the code sets sessionId = parentSessionId and promptParams.path.id = sessionId but does not reset promptSubmitted = false (it's only reset in the !parentSessionId branch). This means on retry with a parent session, the prompt is re-submitted only if promptSubmitted is still false from a failed first attempt. If the first attempt's promptWithTimeout succeeded but the poll failed, promptSubmitted is true, and the retry will skip the prompt call entirely and go straight to polling — which will return the same (possibly failed) response. The if (!(parentSessionId && promptSubmitted)) guard at line 385 is the culprit.

src/routes/interactions.ts:385-390

S5. toAnthropicErrorType maps 413 to request_too_large — not a standard Anthropic error type
src/converters/anthropic.ts:403: Anthropic's API documentation does not define request_too_large as a valid error type. The standard type for oversized requests is invalid_request_error. Clients parsing the type field may not recognize this value.

src/converters/anthropic.ts:403

S6. Keepalive single-newline framing may not be SSE-spec compliant
All routes: Changed from ': keepalive\n\n' to ': keepalive\n'. The SSE specification (W3C EventSource) requires events to be terminated by a blank line (\n\n). A comment line (:) followed by a single \n without a subsequent blank line means the event buffer is not dispatched — the comment is accumulated but the next real event's data: line may be concatenated with the pending buffer. While this "eliminates phantom empty SSE events" as claimed, it may cause the next real event to be delayed or malformed in strict SSE parsers. The correct fix for phantom events is ': keepalive\n\n' (comment + blank line = dispatches empty event, which is harmless) or ensuring the comment is on its own line within a properly terminated block.

src/routes/chat.ts:523, src/routes/messages.ts:588, src/routes/responses.ts (not in truncated diff), src/routes/interactions.ts:434


NITS

N1. src/errors/http.ts:34: isAnthropicMessagesPath splits on ? and lowercases — this is fine but the function is also used in src/proxy.ts auth middleware where req.originalUrl may contain query strings. Consider documenting this is intentional.

N2. src/proxy.ts:300: resolveRequestedModel now throws with statusCode = 404 (was 400). This is correct per the checklist (400 model_not_found + availableModels), but the status code is now 404. Verify docs/api-reference.md is updated to reflect 404 for unknown models (the checklist says 400).

N3. CHANGELOG.md: The undici security entry references CVE numbers from 2026 (e.g., CVE-2026-111497). These appear to be future-dated. If this is intentional (the project operates in a future timeline), fine; otherwise verify these are real CVE identifiers.

N4. src/routes/interactions.ts:329: promptParams.body.messageID = makeId('msg_') — this sets a messageID on the prompt body. Verify the backend API accepts this field; if not, it's silently ignored but adds noise.

N5. Dockerfile: ENV NODE_ENV=production is set in the runtime stage — good. Consider also setting it in the builder stage if npm ci behavior should differ (though --omit=dev is already used).


Test Gap Analysis

  • Keepalive framing: No test verifies that the single-newline comment format doesn't break SSE parsing. tests/stream-phase2.test.js should include an SSE parser that validates comment-only lines don't corrupt subsequent events.
  • Interactions outer catch wire shape: No test appears to verify the outer .catch() handler emits proper event: error SSE framing (it currently emits bare data:).
  • Messages closeOpenBlocks coverage: Need to verify tests cover the case where textBlockOpen=true and an error is thrown mid-stream — the content_block_stop must precede the error event.
  • 401 retry behavior: tests/stream-retries.test.js should verify that a 401 with a non-standard message body is not retried (or document that it intentionally is).
  • unsentSuffix divergence: No test covers the case where raw stream text diverges from the snapshot — the new empty-string return should be tested to ensure it doesn't cause 'Stream ended without valid terminal' errors unnecessarily.
  • Cross-protocol model 404: Verify that the 404 status code from resolveRequestedModel is correctly mapped to not_found_error on Messages and model_not_found on Chat/Responses.

VERDICT: NEEDS_CHANGES

Blocking issues:

  1. Interactions outer catch emits bare data: instead of event: error SSE framing
  2. Messages stream may not close open blocks before all error throw paths
  3. Chat stream error paths missing data: [DONE] terminal marker

These three items break the documented wire shapes for their respective protocols. The should-fix items (especially S2 retrying 401s and S6 keepalive framing) also carry significant compatibility risk.

docs_config

VERDICT: APPROVE

BLOCKING

None. The configuration matrix and wire contracts remain fully intact:

  • No new environment variables or config short keys were introduced; the six-way matrix (.env.example, config.json.example, Dockerfile, docker-compose.yml, index.ts, docs/configuration.md) remains in sync.
  • Known drifts between runtime (index.ts) and library (src/config/proxy-config.ts) are untouched.
  • DISABLE_TOOLS resolution logic remains unaffected.
  • The four streaming wire error shapes (Chat, Responses, Messages, Interactions) strictly adhere to protocol requirements without stray [DONE] events or corrupted framing.

SHOULD_FIX

None.

NITS

  • CHANGELOG.md:16: The undici security entry mentions "Node.js October 2026 Security Releases (October 21, 2026)" and multiple CVE-2026-* identifiers. If this was intended to reference the 2024 or 2025 Node.js/undici security releases (e.g., CVE-2024-xxxx), consider fixing the year typo.

Files that must change to close the matrix gap

None (no configuration matrix gaps found).

FINAL_VERDICT: COMMENT

Session: response_id=resp_EybbasKBAbvMrfcPufPt6Ac,resp_req_vrtx_011CgJuzzc2J8ZBfyu1P6nEj,resp_lCXbaoa1N-LgrfcP0biIiQ8 (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 Re-review(2026-10-23;編輯原留言)

Review target: 17d1f131bf026ad9ab4b985c936d78150c7226eb(前次 96d4dec3e60f 之後新增 7 commits;PR 30 changed files,+5,766 / −746)。
FINAL VERDICT: NEEDS_CHANGES — 目前不建議 Merge。

重新獨立審閱本輪修改、Chat / Messages / Responses / Interactions 四種路由、collector、session/重試邏輯、對應測試與最新 AI Review Bot 評語;另比對 pinned upstream OpenCode v1.18.35 的 session persistence semantics 及相關官方 API 文件。最新 CI 全綠(npm ci、typecheck、build、Jest、Docker build + /health smoke、ShellCheck、AI-bot lint),AI Review workflow 亦成功執行。以下發現來自原始碼路徑與可構造的事件序列;沒有宣稱已實測真實第三方 provider 故障。

✅ 前次 P1 已實際解決的部分

  1. Poll partial/error 被當完整答案:已修正 Chat、Messages、Responses 的 proxy-mode streaming / non-stream,以及 Interactions 的 stream + non-stream。Poll partial:true 或 error != null 現在 fail-closed,新增 tests/stream-phase2.test.js:577-746 的跨路由回歸。src/routes/chat.ts:677-695 · src/routes/messages.ts:744-762 · src/routes/responses.ts:1249-1271 · src/routes/interactions.ts:385-393
  2. 單次 continuation retry 重複提交的風險:新版 Responses 與 Interactions 對已存在 parent session 的錯誤採保守 fail-closed;在 prompt 發出前將 promptSubmitted 標記為可能已提交,不再無條件重送同一輪。tests/stream-retries.test.js:627-1068 補了 SSE terminal 503、prompt rejection、non-stream 等案例,驗證一個 HTTP request 中只 prompt 一次、parent 不會被刪。這是有效的安全改善;但跨 HTTP request 的 idempotency 尚未處理(下方 P1-2)。
  3. 此前已修的 collector snapshot/delta 交錯、stale raw tool artifacts、文字 divergence fail-closed、poll cut fail-closed、provider-cache reconnect epoch guard:實作與回歸仍保留;Responses 的 whitespace delta、hosted-search snapshot 另外補了專項測試(tests/stream-phase2.test.js:748-870)。
  4. Backend retry 連線修復:本輪重試路徑加入 active-model 恢復,backend reconnect listener 可透過 ctx.dispose() 移除;新增 tests/stream-retries.test.js:1123-1400 驗證。已確認 CI 覆蓋並通過。

🚫 P1 — 仍需修正的兩個跨回合資料正確性風險

P1-1. Continuation 的 recovery poll 未限定『本次 user turn』,可能把上一回合回答當成新回合成功結果。

  • src/stream/collector.ts:213-298 的 pollForAssistantResponse(sessionId, ...) 從 session.messages 尾端往前找任意最近一則 role === 'assistant',見到舊 finish:'stop' 就回 {complete:true,content:...};完全沒有驗證它是這次新 prompt 產生的 assistant、其 parentID、新 messageID 或基準 cursor/時間。
  • src/routes/responses.ts:661-672 的 previous_response_id 會重用 parent session,並在 src/routes/responses.ts:1273-1338 同時啟動 SSE collector 與 prompt。若 SSE 提前 cut、first-delta timeout / noData,recovery poll 可能在本輪 assistant 尚未寫入前先讀到上一回合已完成的 assistant,將舊內容 replaySnapshotText()、轉成成功 collected,再走 response.completed,同時把新的 response ID 綁到同一 session。
  • Interactions continuation 也共用 parent session,src/routes/interactions.ts:371-393 的 poll 沒有回合 ID 校驗;若新 turn 尚無 assistant 而 poll 先觀察到舊完整 assistant,同樣可能回舊 output_text。

明確重現序列: Parent session 含 user1 → assistant1('A1',finish:'stop');提交 user2 後,本輪 assistant 尚未出現,SSE 對 user2 回 cut/noData。此時 session.messages 是 [user1,assistant1,user2],collector 會略過 user2,撿到 assistant1 並立即標為 complete,不會回 partial:true,故前次新增的 partial guard 也攔不住。請新增 stateful regression 確認 Responses streaming / Interactions continuation 在這個序列必須等待本輪、或 fail-closed,絕不回 A1 的 successful terminal。

修法: 將預期 user message ID(本輪 promptParams.body.messageID)或 per-turn cursor 明確傳進 poll,僅接受屬於本輪的 assistant(OpenCode assistant 有 parentID 指向 user),不允許降級到 parent session 的上一輪完整訊息。注意不能只用 Date.now() 的 1 秒過濾窗;請測同一秒內的連續請求與慢速新回合。

P1-2. Continuation 在遇到已持久化的 503 後,『下一個 HTTP request』仍可能重複寫入 user turn;單次 request fail-closed 不等於跨請求冪等。

  • src/routes/responses.ts:775-787 與 src/routes/interactions.ts:322-330 的 messageID 是每次新 HTTP request 都重新隨機產生。而 previous_response_id / previous_interaction_id 仍指向同一 parent session;這次的 promptSubmitted 只存在於當次 handler,對下一個同內容 request 不生效。
  • Pinned upstream OpenCode v1.18.35 session/prompt.ts:635-661、OpenCode v1.18.35 session/prompt.ts:1036-1070 的 createUserMessage() 在推論前呼叫 sessions.updateMessage(info) 和 sessions.updatePart(part) 寫入訊息與 parts;因此 provider 在訊息持久化後才失敗 503 是合理且可構造的路徑。Gateway 回 503 但保留 parent;若客戶端以同一 previous_response_id 重試整個 HTTP request,新的 msg_... 會再新增相同 user turn,污染歷史,也可能重複觸發有副作用的工具。
  • 非串流 Responses 在 src/routes/responses.ts:1735-1829 對 continuation 503 fail-closed 正確,但仍回 HTTP 503,OpenAI 官方 Python/Node SDK 對符合條件的 5xx 請求預設會重試(Python retries / Node retries);因此這不只是使用者手動重試的理論問題。Streaming SSE 已收到 200 後的錯誤 event 則不能籠統視為 SDK 自動 retry,請分開測。

必要回歸: 第 1 個 HTTP request 將 user2 持久化後 session.prompt 失敗 503;客戶端重送相同 payload、相同 previous_response_id 的第 2 個 HTTP request;assert parent session 中 user2 只出現一次(包括 parts)、且沒有重複副作用。覆蓋 Responses non-stream、Interactions,以及用戶端 request timeout/connection drop 的模糊提交狀態。建議方案:在 parent 的固定 checkpoint 上 fork 出 request-owned child session(成功才保存 chaining state,失敗可丟棄),或有伺服器端持久 idempotency key + backend commit/reconcile;若無法安全重試,必須明確定義如何讓呼叫端在重送前辨識已提交 turn,不能只靠 handler-local promptSubmitted。

P2 — 非阻擋但應追蹤

A. Interactions 與官方 Gemini SSE 並非相同 wire format。 src/routes/interactions.ts:439-478 的所有事件只寫 data: {type: ...},錯誤同樣 data: {type:'error'}(src/routes/interactions.ts:537-566);但 Google 官方 Interactions streaming 文件 使用 named event: interaction.created / event: step.delta / event: error、payload event_type,且另有 steps 與 done event。Bot 指出 error 缺 event: error,就『直接接 Google SDK』而言有真實相容性依據;但 repo docs/api-reference.md:225-240 明示目前是 Gemini-compatible thin layer,其既有單元測試也驗證自訂 data: {type:...}。請確認支援的是自家 thin-layer contract、還是直接官方 SDK;前者建議在文件清楚註明 wire 差異,後者須另做完整協定對齊。這不宜被錯誤描述成『本次改動導致既有 wire regression』。
B. isTransientUpstreamError 的泛化 401 retry。 src/errors/upstream.ts:170-229 在 status=401 且 message 非 invalid api key / unauthorized 等固定 regex 時仍會 retry;像 token expired 這類真憑證錯誤可浪費 backoff。建議以明確 provider-signature 或 isRetryable 區分可重試的免費額度 / auth 問題,補 401 負向案例。
C. SSE comment keepalive。 : keepalive\n 會送 bytes,但完整 comment-only SSE block 一般是 : keepalive\n\n;符合規格的 EventSource 不會因此產生可見的空資料事件。建議以支援的客戶端與 reverse proxy 端到端測試鎖定選擇,不因 Bot 的『phantom event』描述直接判為功能錯誤。

最新 AI Review Bot 意見逐項整合

  • gateway_safety: APPROVE、docs_config: APPROVE:安全 middleware 順序、工具隔離、Docker/CI backend pin、cache listener dispose 等已核對;與上方新發現的 cross-turn poll / cross-request idempotency 不矛盾。
  • Bot api_compat B1 Interactions event: error:依 Google 官方規格有效,但此專案目前刻意採 thin-layer data contract;列 P2 範圍決策,非直接認定本次硬化 regression。
  • Bot B2 Messages 錯誤時未關閉 block:不成立;src/routes/messages.ts:1138-1209 的內、外層 catch 在 event: error 前都會 closeOpenBlocks(),另有 mid-stream 錯誤測試。
  • Bot B3 Chat 錯誤後沒有 [DONE]:成功路徑有 [DONE],錯誤路徑是 error payload + res.end(),tests/stream-phase2.test.js:232-251 有明確 contract。『必定 SDK hang』缺實證,不能單因無 [DONE] 當 blocker;可再補官方 SDK exception behavior 測試。
  • Bot S4 continuation retry 會建立空 session:現行邏輯刻意保留 parent 並 fail-closed,該敘述錯;真正問題是 下一個 HTTP request 可能重複提交。
  • Bot S5 Anthropic 413 mapping:官方 Claude API errors 明確定義 413 → request_too_large,目前實作正確,不應改成 invalid_request_error。
  • Bot Responses diff truncated / docs 404 不一致 / 2026 security bulletin 未來日期:屬檢視不足或過時資訊;Responses error 會送 response.failed + [DONE],docs/api-reference.md:423-433 已記錄 404,2026-10-21 的官方安全通告至 2026-10-23 已是過去事件。

合併門檻(避免反覆 review/fix)

維持 NEEDS_CHANGES。請在同一修補輪次補兩個重點測試與處理:(1) parent session 上的 stale-assistant poll:跨回合只接受本輪 assistant;(2) failed continuation 後由客戶端重新發 HTTP 的真正跨請求 idempotency。 另保留既有所有 partial、tool-call、proxy fallback、CI regression;檢查官方 SDK retry 行為。兩項 P1 經 stateful 驗證修復後,再做最終 merge gate;P2 不需為了這個 PR 無限擴張範圍。

— This comment is updated in place and supersedes the previous review of 96d4dec3e60f.

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