Code Review

This pull request introduces a dedicated connection timeout for LLM streams, distinct from the existing silent stream timeout. It adds a connectTimeoutMs configuration option and updates the streaming logic to track provider progress, applying the connection timeout until the first progress event is received. The implementation includes a new failOnTimeout utility to wrap async iterables and race them against timeout signals. Testing coverage is expanded with new cases for connection timeout failures and session status transitions. One review comment identifies a potential unhandled promise rejection when racing the stream iterator against the timeout, suggesting a catch handler for the iterator's next promise.

Opus second-pass review (product layer / three questions / five locks)

Conclusion: approve

Implementation stays inside the GPT-X-locked D shape: connect-phase timeout split from silent-stream timeout, allowlist-gated provider progress flag, force-failure path via failOnTimeout wrapper hitting processor.halt(). No drift into boot validation, no status reconcile watchdog, no churn on session.abort from PR #556.

Three questions

Could this be even less? 293 lines across llm.ts + 2 test files. Core change is ~70 lines (constant + state + failOnTimeout + isProviderProgressEvent); the rest is targeted tests. Promise.race(iterator.next(), timeoutFailure) plus double timeoutError() sync-check after the race is necessary to plug the race window between setTimeout firing and the next iterator pull resolving — cannot shrink without losing correctness. Check.

Is what remains good enough?

• CONNECT_STREAM_TIMEOUT_MS = 30_000 default; connectTimeoutMs?: number is a test-only hook (not surfaced via config), matches GPT-X "no provider override in v1" red line.
• isProviderProgressEvent allowlist excludes start and finish per GPT-X-tightened spec. Includes text-*, reasoning-*, tool-input-*, tool-call, tool-result, tool-error — only events that prove the provider is actually producing content/state.
• failConnectTimeout() writes timeoutError, rejects the timeoutFailure promise, then calls ctrl.abort(). The failOnTimeout wrapper double-checks timeoutError() before AND after the Promise.race to plug the window where setTimeout fires concurrent with iterator.next() resolving. Stream throws → Effect.runDrain returns failure → processor.halt() triggers → assistant info.error written → session status flips to idle.
• After first valid progress event, providerProgressed = true flips currentTimeoutMs() to streamTimeoutMs. Existing silent-stream timeout path (10min default) preserved.
• Re-arm semantics during hasAwaitingQuestion preserved via the existing arm() re-entry — pending-question soft cancel from PR #556 is not broken.

Check.

Is it reassuring? Five-lock acceptance traced against tests:

1. connect timeout produces stream failure not success drain — explicit test asserting Exit.isFailure(exit)).toBe(true) plus responseCanceled + requestAborted both fire. This is the lock GPT-X put hardest weight on; the test name is locked to the semantic to keep future maintainers honest.
2. Processor writes info.error + flips status idle — covered by processor-effect.test.ts additions (+65 LOC). I did not deep-read those but the test names need to assert this chain explicitly; please confirm in engineering final.
3. First progress event disarms connect timeout — covered indirectly by silent stream timeout cancels provider response body promptly rewritten to use waitProgressThenSilentStreamingRequest fixture: provider emits text-delta first, then goes silent, only streamTimeoutMs (20ms) fires, exit stays success.
4. Awaiting question pauses connect timeout, fires failure after answer — covered by the reworked does not count awaiting question blockers as silent provider timeout test: connect timeout suspended for 90ms while awaiting, request not aborted; after answer, connect timeout fires and exit is failure. PR #556 soft-cancel pending-question semantic preserved.
5. Live repro of alibaba-coding-plan-cn + glm-5 — flagged for manual verification in PR body. Not asserted in CI.

Findings

P3-1 (rename request, not blocking): The test does not count awaiting question blockers as silent provider timeout no longer matches its current behavior. The first half still asserts that awaiting-question re-arm prevents fire, but the second half now asserts that connect timeout DOES fire (as failure) after the answer is sent. Suggest renaming to connect timeout pauses during awaiting question and fires failure after answer once this PR lands, or splitting into two tests. Not a blocker — behavior is correct, name is the artifact lag.

P3-2 (style nit, not blocking): failOnTimeout wrapper's return() method calls void iterator.return?.().catch(() => {}) without awaiting. This is intentional to avoid hanging on a provider iterator that never resolves cleanup. Worth a one-line code comment noting the design intent: "abort signal is the real cancellation path; iterator.return() is best-effort protocol courtesy that we do not block on."

P3-3 (Gemini bot finding, defer to GPT-X engineering final): Gemini left an unresolved review thread suggesting void nextPromise.catch(() => {}) before the Promise.race to suppress unhandled-rejection warnings when the late AbortError lands after the timeout has already rejected. I think this is a legitimate small improvement (Node will log unhandled rejection warnings even when behavior is correct); GPT-X should weigh whether to fix in this PR or defer.

Approve

Pending GPT-X engineering final pass and Gemini thread resolution.

Engineering review: one P2 issue before I can pass this.

• P2 packages/opencode/src/session/llm.ts:501: resetTimeout(event) currently clears and re-arms the timer for every fullStream event, even when isProviderProgressEvent(event) is false and providerProgressed is still false. That means start, finish, or any future unallowlisted housekeeping event can keep extending the connect timeout without any effective provider progress, which weakens the #554 contract and contradicts the explicit allowlist decision that start/finish do not count as progress.

  Suggested shape: before providerProgressed is true, ignore non-progress events for timer-reset purposes; after the first allowlisted progress event, continue resetting on every event to preserve the existing silent-stream timeout semantics.

  const progressed = isProviderProgressEvent(event)
  if (!providerProgressed && !progressed) return
  if (progressed) providerProgressed = true
  if (timeout) clearTimeout(timeout)
  arm()

  Please add a regression test showing a start-only or otherwise non-progress-only stream still fails on connectTimeoutMs without extending the timer.

The info.error + idle processor test is on the right layer, Gemini P3-3 is addressed, and the non-awaited return() cleanup is now documented. I will re-check after this timer-reset edge is patched.

Engineering review pass for 6295698b5.

The previous P2 is addressed: connect-phase non-progress events no longer reset the connect timer, while post-progress events still reset the existing silent-stream timeout. The new start-only regression covers the important edge case.

Verified locally:

• bun --cwd packages/opencode test test/session/llm.test.ts -t "timeout" --timeout 30000: 4 pass
• bun --cwd packages/opencode test test/session/processor-effect.test.ts -t "connect timeout writes assistant info.error and flips session_status idle" --timeout 10000: 1 pass
• bun --cwd packages/opencode typecheck: pass
• git diff --check: pass

No unresolved review threads found. CI is still running on the latest SHA, so keep waiting for the required checks before merge. Non-blocking closeout note: the PR body verification counts should be refreshed for the added regression test (llm.test.ts now has 19 tests and the full opencode run reported 2708 pass).

Approve in body: the implementation now matches the #554 boundary and does not expand into boot validation, provider overrides, status watchdogs, or the #556 abort API.

Engineering review pass for the rewritten history at 2ad40808f.

Commit granularity now matches the PawWork rule: PR branch commits are review-sized and independently explainable, while dev can still use squash merge.

Commit review:

• f89725343 fix(opencode): fail stalled llm connections before silent timeout: core implementation plus the minimal LLM failure-not-success-drain regression. Single fix intent.
• b29832c22 test(opencode): cover llm connect timeout edge cases: LLM phase-boundary coverage for post-progress silent-stream behavior, start-only non-progress events, and awaiting-question re-arm. Single test-expansion intent.
• 2ad40808f test(opencode): cover processor recovery after connect timeout: processor-level info.error + session_status idle recovery coverage. Single cross-layer regression intent.

Per-commit verification I reran by checkout:

• f89725343: bun --cwd packages/opencode typecheck pass; bun --cwd packages/opencode test test/session/llm.test.ts -t "connect timeout produces stream failure not success drain" --timeout 30000 pass.
• b29832c22: bun --cwd packages/opencode typecheck pass; bun --cwd packages/opencode test test/session/llm.test.ts -t "timeout" --timeout 30000 pass.
• 2ad40808f: bun --cwd packages/opencode typecheck pass; bun --cwd packages/opencode test test/session/processor-effect.test.ts -t "connect timeout writes assistant info.error and flips session_status idle" --timeout 10000 pass; bun --cwd packages/opencode test test/session/llm.test.ts -t "timeout" --timeout 30000 pass.

Also checked git diff --check dev...HEAD: pass, and review threads remain resolved.

Non-blocking nit: the awaiting-question LLM test name still says "silent provider timeout" even though the behavior now covers connect timeout pausing during an awaiting question and firing after answer. The behavior is correct and this does not block merge.

CI is still running on the latest SHA, so wait for required checks before merge.

Engineering re-check pass for 37b587daa after the CodeRabbit nitpick fix.

Confirmed the no-op catch was fixuped into commit 1 and preserves the original timeout promise for Promise.race([nextPromise, request.timeoutFailure]); it only observes early rejection warnings. The earlier nextPromise.catch(() => {}) remains in place. Commit 2 also now has the corrected awaiting-question test name.

Verified locally:

• On commit a4cd01b50: bun --cwd packages/opencode typecheck pass; bun --cwd packages/opencode test test/session/llm.test.ts -t "connect timeout produces stream failure not success drain" --timeout 30000 pass.
• On PR HEAD 37b587daa: bun --cwd packages/opencode test test/session/llm.test.ts --timeout 30000: 19 pass; git diff --check dev...HEAD: pass; worktree clean.

No engineering blockers from GPT-X. Wait for CI to finish before merge.