fix(gemini): preserve thought_signature on stored tool calls; retry burst 429s; enable compaction - #1256
fix(gemini): preserve thought_signature on stored tool calls; retry burst 429s; enable compaction#1256adminwat wants to merge 6 commits into
Conversation
Root cause of the recurring HTTP 400 on the *second* message of a Gemini session: Function call is missing a thought_signature in functionCall parts. ... function call `default_api:websearch`, position 10. The signature was ingested and streamed correctly (ToolUseSignature -> ToolCall.thought_signature), but every turn loop then hand-built the persisted ContentBlock::ToolUse, and three of the four copies hardcoded `thought_signature: None`: jcode-tui/src/tui/app/turn.rs:333,401 (interrupt paths) jcode-tui/src/tui/app/turn.rs:1087 (main TUI path - every turn) jcode-app-core/src/agent/turn_streaming_mpsc.rs:1081 Only turn_loops.rs:785 carried it through. So the signature was captured, then discarded the moment the assistant message was stored: turn 2 replayed a fully-unsigned history and the backend rejected it. Retrying could never help, because the stored history was already stripped. Rather than fix four copies and leave the fifth to rot, the construction moves to ToolCall::to_tool_use_block(), the single place a completed tool call becomes a stored block. Duplication is what let this diverge. This is the real fix for the failure that PR 1jehuang#1 (branch fix/gemini-missing-thought-signature-recovery) only mitigated: that downgrade-to-text retry salvages an already-stripped history, but with signatures preserved it should now rarely be reached. Tests: three regression tests in jcode-message-types covering signature preservation, session-JSON round-trip (resumed sessions hit the same 400 if the field is lost on disk), and clean omission for providers that do not use signatures. Falsified by restoring `None`: 2 of 3 fail. jcode-app-core has 9 pre-existing unrelated failures, identical with and without this change (verified via stash). fmt clean; jcode-tui clippy warning count unchanged at 48.
supports_compaction() gates the entire compaction block in Agent::messages_for_provider, including the emergency hard-compact and payload truncation that fire at the 95% critical threshold. These three runtimes returned a hardcoded false and implement no native_compact, so they had no compaction of any kind: a long session grew unchecked until the provider rejected the prompt, with nothing able to rescue it. Unlike OpenAI there was no config escape hatch either. None of the three has native server-side compaction to conflict with, and all three inherit the default complete_simple(), which is what generate_compaction_artifact falls back to for the text summary. So opting in gives them the full engine: proactive/semantic folding, hard compact and emergency truncation. Latent rather than observed today only because no Gemini session on the test machine had yet exceeded 50 messages; Gemini advertises a 1M window, so the failure arrives late and all at once.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5489f9c89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // disabled the entire compaction block in `Agent::messages_for_provider` | ||
| // — including the emergency hard-compact and payload truncation at the | ||
| // critical threshold — leaving these sessions with no safety net at all. | ||
| true |
There was a problem hiding this comment.
When a Cursor session is large enough to compact, the generic compactor sends the selected history through complete_simple, which reaches build_cli_prompt; that function retains only the final 120,000 characters (MAX_PROMPT_CHARS, lines 28 and 96-105). The compaction manager nevertheless marks every selected message as covered by the resulting summary, so an oversized compaction prompt silently omits the earliest conversation while hiding all of those original messages from subsequent requests. Keep this disabled or provide a chunked/non-truncating summary path for Cursor.
Useful? React with 👍 / 👎.
| if status == reqwest::StatusCode::TOO_MANY_REQUESTS && attempt < MAX_429_RETRIES { | ||
| let delay = Duration::from_millis(1500u64.saturating_mul(1u64 << attempt)); |
There was a problem hiding this comment.
When Code Assist returns a persistent 429 for exhausted quota or billing limits, this condition retries solely by status and ignores the error body and any server retry interval. It therefore issues five doomed requests and delays the actionable error by 46.5 seconds; conversely, a transient limit with a retry interval longer than that can exhaust all attempts prematurely. Classify the body or honor the server-provided retry delay before entering this burst-retry loop.
Useful? React with 👍 / 👎.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let delay = Duration::from_millis(1500u64.saturating_mul(1u64 << attempt)); | ||
| jcode_base::logging::warn(&format!( | ||
| "Gemini {} hit burst 429 (attempt {}/{}); retrying in {:?}", | ||
| method, | ||
| attempt + 1, | ||
| MAX_429_RETRIES, | ||
| delay | ||
| )); | ||
| attempt += 1; | ||
| tokio::time::sleep(delay).await; |
There was a problem hiding this comment.
The new 429 handling retries only after fixed 1.5/3/6/12/24-second delays and never reads Retry-After. When the provider directs the client to wait longer, all six attempts can be consumed before requests are permitted again. A response with Retry-After: 60 exhausts the final attempt at about 46.6 seconds, causing the turn to fail even though waiting for the server-provided deadline would allow a later retry.
Artifacts
- Runs an injected in-module test against the parent and current revisions with a local 429 endpoint that sends Retry-After: 60, exercising the actual post_json retry path; it demonstrates the compared behavior.
- Captured command output for the parent revision shows one local HTTP 429 Too Many Requests response with Retry-After: 60 and one request at 30 ms; before the change there was no 429 retry.
- Captured command output for the current revision shows six local HTTP 429 Too Many Requests responses despite Retry-After: 60, with the final attempt at 46.568 seconds; the retry budget is exhausted before provider availability.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-gemini-runtime/src/lib.rs
Line: 401-410
Comment:
**Honor Retry-After Timing**
The new 429 handling retries only after fixed 1.5/3/6/12/24-second delays and never reads `Retry-After`. When the provider directs the client to wait longer, all six attempts can be consumed before requests are permitted again. A response with `Retry-After: 60` exhausts the final attempt at about 46.6 seconds, causing the turn to fail even though waiting for the server-provided deadline would allow a later retry.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| // No native server-side compaction exists for this provider, so jcode's | ||
| // own summary compaction is the only thing standing between a long | ||
| // session and a hard context-limit rejection. Returning `false` here | ||
| // disabled the entire compaction block in `Agent::messages_for_provider` | ||
| // — including the emergency hard-compact and payload truncation at the | ||
| // critical threshold — leaving these sessions with no safety net at all. | ||
| true | ||
| } |
There was a problem hiding this comment.
Enabling compaction for Gemini exposes OpenAI-native compaction state to a conversion path that cannot replay it. Gemini and Antigravity omit the encrypted compaction block entirely, while Cursor replaces it with a fixed marker. After switching providers, the request loses the compacted earlier conversation and contains only the active suffix, so the model can no longer use that prior context.
Artifacts
- The authored executable creates isolated before and after worktrees, injects focused runtime tests, and captures the provider conversion results; it demonstrates the exact exercised source path.
- The executed `HEAD^` run shows all three providers had compaction disabled while the same Gemini, Antigravity, and Cursor conversions respectively omit or reduce the OpenAI-native block; the unsafe conversion existed but was not enabled by this PR.
- The executed `HEAD` run shows PR fix(gemini): preserve thought_signature on stored tool calls; retry burst 429s; enable compaction #1256 enables compaction and the focused tests pass with `gemini contents=[]`, `antigravity contents=[]`, and Cursor's placeholder-only prompt; the enabled path loses the compacted context.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-gemini-runtime/src/lib.rs
Line: 1096-1103
Comment:
**Keep Switched-Provider Context**
Enabling compaction for Gemini exposes OpenAI-native compaction state to a conversion path that cannot replay it. Gemini and Antigravity omit the encrypted compaction block entirely, while Cursor replaces it with a fixed marker. After switching providers, the request loses the compacted earlier conversation and contains only the active suffix, so the model can no longer use that prior context.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…ty) like burst 429s
…onId Gemini/Claude-CLI/Grok emit StreamEvent::SessionId with the *provider* resume handle. The mpsc turn loop re-broadcast it as ServerEvent::SessionId, which the TUI treats as the jcode session id and stores in remote_session_id. Every later reload/reconnect then ran 'jcode --resume <provider-id>', the server found no such session and created a fresh empty one, and the user saw their entire conversation vanish while the transcript sat intact on disk.
| let transient = | ||
| status == reqwest::StatusCode::TOO_MANY_REQUESTS || status.is_server_error(); | ||
| if transient && attempt < MAX_429_RETRIES { |
There was a problem hiding this comment.
If Code Assist completes a generateContent request before responding with a 5xx, this loop sends the identical POST again without an idempotency or deduplication key. The provider can run and bill the generation twice while jcode discards the first result; onboardUser uses the same helper and has the same replay behavior. Retry only failures where no response was received, or use the provider-supported idempotency mechanism.
Artifacts
- The executed shell harness starts a local Code Assist-compatible server, returns a 500 then 200 for generateContent, and records each received request, demonstrating the precise retry contract.
- Combined output from executing the local harness shows HTTP 500 followed by HTTP 200, identical request body and user prompt ID, and no idempotency or deduplication headers, confirming an unkeyed replay.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-gemini-runtime/src/lib.rs
Line: 403-405
Comment:
**Prevent Duplicate Generations**
If Code Assist completes a `generateContent` request before responding with a 5xx, this loop sends the identical POST again without an idempotency or deduplication key. The provider can run and bill the generation twice while jcode discards the first result; `onboardUser` uses the same helper and has the same replay behavior. Retry only failures where no response was received, or use the provider-supported idempotency mechanism.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…at a bounded size Every turn re-sends the whole transcript. With Gemini's 1M window and the 0.80 trigger, a session could reach ~800k tokens per request before folding; one real session hit 371k/turn (1.3 MB JSON) with compaction never firing. [compaction] max_context_tokens bounds the budget in the single set_budget funnel so every provider path (agent, TUI, model switch) honors it. 0 = off.
| /// Update the token budget (e.g., when model changes) | ||
| pub fn set_budget(&mut self, budget: usize) { | ||
| self.token_budget = budget; | ||
| self.token_budget = Self::capped_budget(&self.compaction_config, budget); |
There was a problem hiding this comment.
When [compaction] max_context_tokens changes during a running session, the existing compaction manager continues to use the value captured when it was created. After the global configuration reloads from 50,000 to 10,000, calling set_budget(1_000_000) still permits 50,000 tokens for that session, while a newly created manager uses 10,000. This can leave a long-running session 40,000 tokens above an operator's newly lowered context and cost limit.
Artifacts
- The initial run shows that a manager constructed with a 50,000-token limit applies that limit.
- The reload run shows the existing manager remains at 50,000 while a new manager uses the reloaded 10,000 limit.
- The focused Rust harness keeps one manager alive across the configuration reload and compares it with a new manager.
Compaction reload harness manifest
- The manifest defines the isolated executable used to exercise the reload behavior.
- The runner executes and records both the initial and reloaded configuration cases.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-base/src/compaction.rs
Line: 245
Comment:
**Refresh Compaction Cap**
When `[compaction] max_context_tokens` changes during a running session, the existing compaction manager continues to use the value captured when it was created. After the global configuration reloads from 50,000 to 10,000, calling `set_budget(1_000_000)` still permits 50,000 tokens for that session, while a newly created manager uses 10,000. This can leave a long-running session 40,000 tokens above an operator's newly lowered context and cost limit.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.1 participant
Three fixes on top of v0.84.0 for Gemini "works on turn 1, fails on turn 2":
turn_streaming_mpsc.rs,turn.rs). The signature was captured fromStreamEvent::ToolUseSignaturebut the persistedContentBlock::ToolUsewas hand-built withthought_signature: Nonein the mpsc/TUI paths (onlyturn_loops.rscarried it). Turn 2 replayed an unsigned history and Gemini 3.x returned HTTP 400Function call is missing a thought_signature. Construction now goes throughToolCall::to_tool_use_block(). Three regression tests injcode-message-types; falsified by restoringNone(2 of 3 fail).supports_compaction() == falseskipped even the emergency hard-compact).Verified on a daemon-hosted mpsc session: two sequential tool calls on
gemini-2.5-pro(server-upgraded togemini-3.1-pro-preview), persisted history carriesthought_signatureon every tool call, zerostream_error.