Repository navigation
fix(proxy): stop 500ing when the Gemini CLI continues from a model turn - #1484
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Tara-ag
reviewed
Aug 22, 2026
Tara-ag
left a comment
Tara-ag
left a comment
Contributor
There was a problem hiding this comment.
Review Summary
This PR makes two targeted improvements to the Claude proxy system:
Changes
1. src/lib/proxy/geminiFormat.ts - Model Turn Continuation Support
- Adds
CONTINUATION_PROMPT = "Continue."constant - Enables Google's API to continue generation from the model's last turn when it appears as the final turn in conversation history
- Properly documented with JSDoc explaining the use case
2. test/continuous-test-suite-proxy.ts - Test Timeout Protection
- Introduces
CASE_TIMEOUT_MS = 180_000(3-minute timeout per test case) - Adds
withCaseTimeout()helper function wrapping all test executions - Prevents hanging tests from blocking the entire suite
- Critical for proxy tests that spawn child processes and talk to external APIs
Impact Assessment
Scope: Minimal - affects Gemini multi-turn conversation handling and test infrastructure only
Breaking Changes: None - both changes are additive only
Risk Level: Low
- The first change enhances multi-turn conversation capabilities without altering existing behavior
- The second change improves test reliability without affecting production code
Findings
✅ No CRITICAL issues found
✅ No MAJOR issues found
✅ No MINOR issues found
✅ No SUGGESTION-level improvements needed
The changes follow project conventions, add clear documentation, and improve functionality without introducing any regressions or breaking changes.
Decision
APPROVED - The PR is ready to merge.
Contributor
|
The changes in this PR are minimal and well-targeted:
No issues found. Both changes follow project conventions, add value without breaking existing functionality, and improve the overall robustness of the system. Decision: APPROVED ✅ |
Tara-ag
reviewed
Aug 22, 2026
Tara-ag
left a comment
Tara-ag
left a comment
Contributor
There was a problem hiding this comment.
Review Summary
This PR makes two targeted improvements to the Claude proxy system:
Changes
1. src/lib/proxy/geminiFormat.ts - Model Turn Continuation Support
- Adds
CONTINUATION_PROMPT = "Continue."constant - Enables Google's API to continue generation from the model's last turn when it appears as the final turn in conversation history
- Properly documented with JSDoc explaining the use case
2. test/continuous-test-suite-proxy.ts - Test Timeout Protection
- Introduces
CASE_TIMEOUT_MS = 180_000(3-minute timeout per test case) - Adds
withCaseTimeout()helper function wrapping all test executions - Prevents hanging tests from blocking the entire suite
- Critical for proxy tests that spawn child processes and talk to external APIs
Impact Assessment
Scope: Minimal - affects Gemini multi-turn conversation handling and test infrastructure only
Breaking Changes: None - both changes are additive only
Risk Level: Low
- The first change enhances multi-turn conversation capabilities without altering existing behavior
- The second change improves test reliability without affecting production code
Findings
✅ No CRITICAL issues found
✅ No MAJOR issues found
✅ No MINOR issues found
✅ No SUGGESTION-level improvements needed
The changes follow project conventions, add clear documentation, and improve functionality without introducing any regressions or breaking changes.
Decision
APPROVED - The PR is ready to merge.
Two defects, both found by chasing review threads on already-merged PRs rather
than by the threads themselves.
Continuing from a model turn failed at the door, every time. Google lets a
client send `contents` whose final entry is a model turn, and the Gemini CLI
does exactly that when continuing — there is no trailing user turn to become
`input.text`. `prompt` was therefore left "", and NeuroLink's stream() rejects
an empty input before contacting any provider:
[proxy:gemini] request failed: Stream options must include either
input.text, input.audio, or stt.audio
The review on #1468 found the neighbouring half of this — that the engine's
`slice(0, -1)` ate the final model turn — and #1480 answered it with a terminal
placeholder consumed by the slice. That fix is correct as far as it goes and is
kept. But a placeholder eaten by the slice does nothing about the prompt, and
no case ever sent a model-final request, so the 500 sat behind a finding that
looked closed. The multi-turn case added in #1480 covers [user, model, user]
only, which always has a user turn to promote.
Google's semantics for a model-final `contents` are "keep going", and the
chat-completions shape the engine translates into has no assistant-prefill to
express that. An explicit continuation instruction is the closest faithful
equivalent: the whole conversation still arrives as history, and the model is
told to continue it rather than handed an empty turn.
The suite could not have caught a hang, either. continuous-test-suite-proxy.ts
drives its own runner — it destructures recordTest/runSuite from defineSuite
and calls `test.fn()` directly — so it never passes through the harness's own
Promise.race per-case timeout at helpers/harness.ts:411. Any case that hung
hung the entire run, indistinguishable from slow work. Two review threads on
#1455 raised this against the atomic-write race case specifically; it was never
about that one case.
Every case is now bounded at 180s, and a breach is reported as a FAILURE rather
than the harness's `SKIP:`-prefixed default. That difference is deliberate: a
skip is right for a live-provider suite where a hung upstream is not the code's
fault, but every case here talks to a proxy this repo builds and spawns, so a
hang is a defect and must not go green.
Both proven by reverting:
prompt left "" ✗ continuing from a model turn answered 500 instead
of 200 — the door is failing the CLI's continue flow
CASE_TIMEOUT_MS = 1 Passed 49, Failed 25, exit 1 — and reported as
failures, not skips, which is the part that matters
given the harness downgrades abort-shaped messages
fixed 74 passed, 0 failed
murdore
force-pushed
the
fix/gemini-model-final-turn
branch
from
August 22, 2026 22:08
a8245a6 to
b77fb92
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Contributor
|
💬 SUGGESTION: Consider adding inline example showing CONTINUATION_PROMPT usage. While the JSDoc explains when CONTINUATION_PROMPT is used, adding a brief inline comment showing how this constant flows through parseGeminiRequest would help developers understand its practical application. For example, after line 67 where we set |
Contributor
Yama Review SummaryDecision: APPROVED ✅ This PR contains a solid bug fix for handling conversation continuation from model turns, along with appropriate test coverage and performance safeguards. Changes Reviewed:1. src/lib/proxy/geminiFormat.ts
2. test/continuous-test-suite-proxy.ts
Impact Analysis:
Code Quality:✅ Follows factory+registry pattern No issues found. This is a safe, well-tested bug fix. |
Tara-ag
approved these changes
Aug 22, 2026
Contributor
|
🎉 This PR is included in version 11.18.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
murdore
added a commit
that referenced
this pull request
Aug 22, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Eighteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a `runAllTests` they pass to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. That is not hypothetical: two of my own runs of continuous-test-suite-context died on a wall-clock limit this session with no per-case attribution, and this is why. `withCaseTimeout` moves to the shared harness rather than being copied per suite. #1484 added an identical local helper to the proxy suite while fixing the same bug there; the semantics here match it deliberately so the two cannot drift, and that suite is left alone to avoid conflicting with an open PR. 19 call sites across 18 suites. The one non-uniform case is autoresearch, which has two runners — the assertion in the conversion script caught that before anything was written, rather than silently converting one and leaving the other. Verified the bound actually fires, since a timeout helper that never triggers is worse than none: normal case returns its value untouched a hung case rejects at 301ms against a 300ms bound, naming the case the timer does not keep the process alive AND IT IMMEDIATELY CAUGHT A REAL HANG. `continuous-test-suite-bugfixes` now reports: ✗ proxy fallback: an idle stream is aborted without ambiguous replay exceeded 240000ms and was aborted — treat as a hang, not slowness That case previously hung silently inside a suite that reported 280 passed. The failure is pre-existing and intermittent; what changed is that it is now attributable instead of costing four minutes of wall-clock and no information. Folded in continuous-test-suite-proxy.ts, at its author's request now that #1484 has merged. Its local `withCaseTimeout` is deleted in favour of the shared one — two implementations that match today are two that can drift tomorrow, and there is now exactly one definition in the repo. The proxy suite keeps its own 180s bound rather than the shared 240s default, for the reason its author gave: every case there talks to a proxy this repo builds and spawns, not a live provider, so a hang is a defect and does not deserve the slack a remote endpoint gets. Checked the swallow hazard they flagged rather than assuming the shared wording is safe. `defineSuite` classifies a thrown error as SKIP when the message matches `isExpectedProviderError()`, and this message contains "aborted", which is the shape that can be swallowed. Ran the messages through it directly: "... exceeded 240000ms and was aborted — treat as a hang, not slowness" "... exceeded 180000ms and was aborted — treat as a hang, not slowness" "... exceeded 240000ms and was aborted" all three report as FAILURE, none are swallowed. A timeout that reported as a skip would be the exact false green this change exists to remove. Verified after folding: typecheck 4821 files 0 errors, eslint 0 errors, prettier clean, proxy suite 74 passed / 0 failed / 6 skipped, servers 41 passed / 0 failed, and exactly one `withCaseTimeout` definition repo-wide.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Eighteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a `runAllTests` they pass to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. That is not hypothetical: two of my own runs of continuous-test-suite-context died on a wall-clock limit this session with no per-case attribution, and this is why. `withCaseTimeout` moves to the shared harness rather than being copied per suite. #1484 added an identical local helper to the proxy suite while fixing the same bug there; the semantics here match it deliberately so the two cannot drift, and that suite is left alone to avoid conflicting with an open PR. 19 call sites across 18 suites. The one non-uniform case is autoresearch, which has two runners — the assertion in the conversion script caught that before anything was written, rather than silently converting one and leaving the other. Verified the bound actually fires, since a timeout helper that never triggers is worse than none: normal case returns its value untouched a hung case rejects at 301ms against a 300ms bound, naming the case the timer does not keep the process alive AND IT IMMEDIATELY CAUGHT A REAL HANG. `continuous-test-suite-bugfixes` now reports: ✗ proxy fallback: an idle stream is aborted without ambiguous replay exceeded 240000ms and was aborted — treat as a hang, not slowness That case previously hung silently inside a suite that reported 280 passed. The failure is pre-existing and intermittent; what changed is that it is now attributable instead of costing four minutes of wall-clock and no information. Folded in continuous-test-suite-proxy.ts, at its author's request now that #1484 has merged. Its local `withCaseTimeout` is deleted in favour of the shared one — two implementations that match today are two that can drift tomorrow, and there is now exactly one definition in the repo. The proxy suite keeps its own 180s bound rather than the shared 240s default, for the reason its author gave: every case there talks to a proxy this repo builds and spawns, not a live provider, so a hang is a defect and does not deserve the slack a remote endpoint gets. Checked the swallow hazard they flagged rather than assuming the shared wording is safe. `defineSuite` classifies a thrown error as SKIP when the message matches `isExpectedProviderError()`, and this message contains "aborted", which is the shape that can be swallowed. Ran the messages through it directly: "... exceeded 240000ms and was aborted — treat as a hang, not slowness" "... exceeded 180000ms and was aborted — treat as a hang, not slowness" "... exceeded 240000ms and was aborted" all three report as FAILURE, none are swallowed. A timeout that reported as a skip would be the exact false green this change exists to remove. Verified after folding: typecheck 4821 files 0 errors, eslint 0 errors, prettier clean, proxy suite 74 passed / 0 failed / 6 skipped, servers 41 passed / 0 failed, and exactly one `withCaseTimeout` definition repo-wide. Narrowed the globalThis.setTimeout patch in the proxy-fallback case, because bounding the cases exposed a cascade that the bound itself creates. `Promise.race` does not cancel the losing promise. When the harness bound fires on a hung case, that case KEEPS RUNNING — still inside its `try`, with its `finally` not yet reached. A case that has patched a global therefore leaks that patch to every case after it. In CI this showed as two failures rather than one: the proxy-fallback case timed out, and then ✗ CLI setup: --provider <p> --check takes the check-only path (no interactive prompt, no hang) which sits later in the same file, failed in its wake. One hang became two failures, and the second looked unrelated. The patch now matches on the delay and collapses only the fallback idle timeout, passing every other timer through untouched. That keeps the case testing exactly what it tested, makes the patch inert for other pending work in a 280-case shared process, and makes it harmless even in the window after an abandoned run. Proven both directions: an unrelated 400ms timer still waits 403ms, the targeted 120s timer collapses to 1ms. This does not explain why the case hangs in CI and passes everywhere else — that is still open, and it is not being papered over. What it removes is the mechanism by which one unexplained hang corrupts unrelated cases. The constant is mirrored locally with its provenance rather than exported from the product: if the product value ever changes, the collapse stops matching and the case fails loudly at the harness bound instead of passing for the wrong reason. bugfixes 280 passed / 0 failed after the change.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Eighteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a `runAllTests` they pass to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. That is not hypothetical: two of my own runs of continuous-test-suite-context died on a wall-clock limit this session with no per-case attribution, and this is why. `withCaseTimeout` moves to the shared harness rather than being copied per suite. #1484 added an identical local helper to the proxy suite while fixing the same bug there; the semantics here match it deliberately so the two cannot drift, and that suite is left alone to avoid conflicting with an open PR. 19 call sites across 18 suites. The one non-uniform case is autoresearch, which has two runners — the assertion in the conversion script caught that before anything was written, rather than silently converting one and leaving the other. Verified the bound actually fires, since a timeout helper that never triggers is worse than none: normal case returns its value untouched a hung case rejects at 301ms against a 300ms bound, naming the case the timer does not keep the process alive AND IT IMMEDIATELY CAUGHT A REAL HANG. `continuous-test-suite-bugfixes` now reports: ✗ proxy fallback: an idle stream is aborted without ambiguous replay exceeded 240000ms and was aborted — treat as a hang, not slowness That case previously hung silently inside a suite that reported 280 passed. The failure is pre-existing and intermittent; what changed is that it is now attributable instead of costing four minutes of wall-clock and no information. Folded in continuous-test-suite-proxy.ts, at its author's request now that #1484 has merged. Its local `withCaseTimeout` is deleted in favour of the shared one — two implementations that match today are two that can drift tomorrow, and there is now exactly one definition in the repo. The proxy suite keeps its own 180s bound rather than the shared 240s default, for the reason its author gave: every case there talks to a proxy this repo builds and spawns, not a live provider, so a hang is a defect and does not deserve the slack a remote endpoint gets. Checked the swallow hazard they flagged rather than assuming the shared wording is safe. `defineSuite` classifies a thrown error as SKIP when the message matches `isExpectedProviderError()`, and this message contains "aborted", which is the shape that can be swallowed. Ran the messages through it directly: "... exceeded 240000ms and was aborted — treat as a hang, not slowness" "... exceeded 180000ms and was aborted — treat as a hang, not slowness" "... exceeded 240000ms and was aborted" all three report as FAILURE, none are swallowed. A timeout that reported as a skip would be the exact false green this change exists to remove. Verified after folding: typecheck 4821 files 0 errors, eslint 0 errors, prettier clean, proxy suite 74 passed / 0 failed / 6 skipped, servers 41 passed / 0 failed, and exactly one `withCaseTimeout` definition repo-wide. Narrowed the globalThis.setTimeout patch in the proxy-fallback case, because bounding the cases exposed a cascade that the bound itself creates. `Promise.race` does not cancel the losing promise. When the harness bound fires on a hung case, that case KEEPS RUNNING — still inside its `try`, with its `finally` not yet reached. A case that has patched a global therefore leaks that patch to every case after it. In CI this showed as two failures rather than one: the proxy-fallback case timed out, and then ✗ CLI setup: --provider <p> --check takes the check-only path (no interactive prompt, no hang) which sits later in the same file, failed in its wake. One hang became two failures, and the second looked unrelated. The patch now matches on the delay and collapses only the fallback idle timeout, passing every other timer through untouched. That keeps the case testing exactly what it tested, makes the patch inert for other pending work in a 280-case shared process, and makes it harmless even in the window after an abandoned run. Proven both directions: an unrelated 400ms timer still waits 403ms, the targeted 120s timer collapses to 1ms. This does not explain why the case hangs in CI and passes everywhere else — that is still open, and it is not being papered over. What it removes is the mechanism by which one unexplained hang corrupts unrelated cases. The constant is mirrored locally with its provenance rather than exported from the product: if the product value ever changes, the collapse stops matching and the case fails loudly at the harness bound instead of passing for the wrong reason. bugfixes 280 passed / 0 failed after the change. Documented what this helper CANNOT bound, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free. Anything that blocks the loop is unprotected: `spawnSync`, `execSync`, a slow synchronous read. Measured rather than reasoned about — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. That matters here specifically. continuous-test-suite-bugfixes makes 20 `spawnSync` calls, 8 of them with a `timeout`, and `spawnSync`'s timeout is not a guarantee either: it sends `killSignal`, SIGTERM by default, then keeps waiting, so a child that ignores SIGTERM hangs it forever. Those cases were unboundable by construction and this change would have left them looking protected. Credit to cli-support-83, who found it from the second CI failure I sent them and is fixing the call sites in #1490. Also recorded on the helper: `Promise.race` does not cancel the losing promise, so an abandoned case keeps running with its `finally` unreached — which is the cascade that made one hang produce two CI failures, and the reason the proxy-fallback case's global patch is now scoped by delay.
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
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.
What this is
Two defects found while working the review threads on already-merged PRs — neither was found by a thread. Both were sitting behind findings that looked closed.
The Gemini door 500s on every continue
Google lets a client send
contentswhose final entry is a model turn, and the Gemini CLI does exactly that when continuing. There's no trailing user turn to becomeinput.text, sopromptwas left""— and NeuroLink'sstream()rejects an empty input before contacting any provider:This was missed twice over. The review on #1468 found the neighbouring half — that the engine's
slice(0, -1)ate the final model turn — and #1480 answered it with a terminal placeholder consumed by the slice. That fix is correct as far as it goes and is kept here. But a placeholder eaten by the slice does nothing about the prompt, and no case ever sent a model-final request, so the 500 sat behind a finding that read as resolved. The multi-turn case #1480 added covers[user, model, user]only — which always has a user turn to promote.Google's semantics for a model-final
contentsare "keep going". The chat-completions shape the engine translates into has no assistant-prefill to express that, so an explicit continuation instruction is the closest faithful equivalent: the whole conversation still arrives as history, and the model is told to continue rather than handed an empty turn.The suite could not have caught a hang
continuous-test-suite-proxy.tsdrives its own runner — it destructuresrecordTest/runSuitefromdefineSuiteand callstest.fn()directly — so it never passes through the harness's ownPromise.raceper-case timeout attest/helpers/harness.ts:411. Any case that hung hung the whole run, indistinguishable from slow work.Two threads on #1455 raised this against the atomic-write race case. It was never about that one case: all 74 were unbounded.
Every case is now bounded at 180s, and a breach reports as a FAILURE, not the harness's
SKIP:-prefixed default. That difference is deliberate — a skip is right for a live-provider suite where a hung upstream isn't the code's fault, but every case here talks to a proxy this repo builds and spawns, so a hang is a defect and must not go green.Proof
Both confirmed by reverting the fix:
The timeout's own message was checked against the skip-masking rule specifically: it contains the word "aborted", which is exactly the shape
isExpectedProviderError()can swallow. It reports as a failure — verified, not assumed.Gates
Review threads
This lands alongside a pass over all 30 unresolved threads on #1455, #1468 and #1480 — each verified against the current tree, answered on the thread, and resolved where settled. The two genuinely-open ones were the timeout pair, fixed here.
Second witness
Worth recording, because it's the strongest evidence this is a real user-facing path rather than a shape only a test would send.
A peer session working the model-turn fix independently tried to add an end-to-end case for the model-terminal shape and could not make it exercise the path — the capture upstream was never called. Unable to tell whether that was a further defect or a limitation of the capture harness, they pulled the test rather than ship one that skips, and logged it as an open question instead of guessing.
That symptom was this bug, one layer down:
contentsending on a model turn leavespromptempty,stream()rejects it before contacting any provider, so of course no upstream was ever reached. Two independent attempts to test this shape both hit the wall; neither run got a request through.Their call to report the ambiguity rather than guess at it is why it was still legible when I hit the same wall from the other side.