delegate_gemini: fix 5 error-coverage gaps found in review - #20
Merged
Conversation
… always suggesting resume callGenerateContentOnce/callGenerateContent already tag network-layer failures with err.status (429/503 are the documented transient cases -- see client.js's own model-cascade, which already only retries on 429). Everything else -- a malformed request (400), auth/config problems (401/403, or no status at all e.g. "GEMINI_API_KEY is not set"), or "Gemini returned no candidates" from a safety/recitation block -- will reproduce identically on retry. The per-step failure message previously treated all of these the same way and always told the caller to resume, which is actively misleading for the non-transient cases. Added isTransientGeminiError() and branched the message accordingly.
… failure message; guard err.message for non-Error throws
…ax_steps below steps already completed Previously, resuming with a max_steps ceiling lower than checkpoint.stepsDone meant startStep > cappedSteps, so the loop body never ran -- it fell straight through to deleteCheckpoint() + the generic "stopped after reaching the step cap" message, discarding a checkpoint that had real completed work in it without ever surfacing that work or explaining what happened. Added an explicit guard right after the checkpoint state is established: if there's nothing new to do because the requested ceiling is already met/exceeded, leave the checkpoint alone (still resumable with a higher max_steps) and return the existing transcript with a message that says so plainly.
…pected throw doesn't lose checkpointed progress Unlike the geminiChat call (which has its own try/catch that checkpoints and returns a resumable failure), the loop over functionCalls -- fn.execute(), building the transcript entry, JSON.stringify(args) -- had no equivalent guard. Every current FUNCTIONS[].execute() happens to return a string today, so resultText.length never throws in practice, but nothing enforced that contract; a future function returning non-string (or a JSON.stringify throw on unexpected args shape) would escape runInvestigation entirely uncaught, land in tools.js's generic catch, and lose the run's runId/resume info even though the model's turn (and possibly several prior steps) had already completed. Added a defensive typeof guard on resultText and wrapped the whole per-call loop in a try/catch that mirrors the geminiChat failure path: checkpoint what's done and return a resumable failure instead of throwing past this function.
… throws max_steps had no floor check -- passing 0 or a negative number produced a nonsensical "(Investigation stopped after reaching the step cap of 0/-N without a final answer...)" instead of Gemini ever being called, with no indication the input itself was invalid. Zod's z.number().optional() accepts any finite number including 0/negatives/non-integers, so the guard has to be a runtime check. Also broadened the existing task-required guard's error message to mention this new check, and hardened `err.message` references to fall back to String(err) in case anything ever throws a non-Error value.
…consistency with delegate_gemini's hardening
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
1 participant
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.
Follow-up to PR #19. Found while auditing delegate_gemini's error paths for gaps (no live regression -- PR #19's fixes are working correctly), all fixed here:
1.
max_steps: 0(or negative) silently no-ops instead of erroringConfirmed live before this fix:
Gemini is never called; the loop's
for (step = startStep; step <= cappedSteps; ...)just never executes. Zod'sz.number().optional()has no floor. Fix: runtime validation intools.js—max_stepsmust be a positive integer, returned as a clearisErrorbeforerunInvestigationis ever called.2. Transient vs. permanent Gemini failures got the identical message
A 429/503 (genuinely worth resuming) and a 400/malformed-request/missing-
GEMINI_API_KEY/no-candidates-from-a-safety-block (will reproduce identically on retry) all produced the same"...resume_run_id to continue"message, actively steering callers toward a fix that won't work for the latter group. Fix: addedisTransientGeminiError()(429/503 only, matchingclient.js's own fallback-cascade logic) and branched the failure message accordingly.3. Resuming with
max_stepslower than steps already completed silently discarded the checkpointIf a checkpoint has
stepsDone = 5and you resume withmax_steps: 2,startStep (6) > cappedSteps (2)means the loop body never runs, but the code fell straight through todeleteCheckpoint(runId)+ a generic "reached the step cap" message — destroying 5 real completed steps with no way to recover them and no indication that's what happened. Fix: explicit guard right after checkpoint load — if there's no step budget left, leave the checkpoint alone and say so, with the existing transcript included.4. Function-execution block wasn't guarded the way the
geminiChatcall wasThe
geminiChat()call has its own try/catch that checkpoints and returns a resumable failure. The loop right after it (fn.execute()+ transcript building) didn't have the same protection — every current function happens to return a string today so it doesn't fire, but nothing enforced that, and any future violation would throw pastrunInvestigationentirely and land intools.js's generic catch with norunId, losing the run's resumability. Fix: added a defensivetypeof resultText !== "string"guard plus a wrapping try/catch that checkpoints and returns the same resumable-failure shape as ageminiChatfailure.5.
err.messageassumed every throw is a realErrorMinor robustness gap across both
delegate.jsandtools.js(includingDelegate_web_fetch) — everything currently thrown is a realError, but nothing guarded against a non-Errorthrow producing"Error: undefined". Fix:err?.message ?? String(err)everywhere a caught error's message is surfaced.Testing status
Unit-reasoned through the code paths (item 1 was reproduced live before the fix, verified fixed after). Items 2-4 are defensive/edge-case fixes for conditions that are rare or hard to force live (a genuine non-429/503 Gemini failure, a checkpoint-then-lower-max_steps resume, a function returning non-string). Recommend a spot-check of item 1 live post-merge; the others are lower-risk since they only add guards around existing behavior rather than changing the happy path.