fix(#3996): classify Gemini API 400s into safe, actionable categories - #4017
fix(#3996): classify Gemini API 400s into safe, actionable categories#4017aheritier wants to merge 3 commits into
Conversation
aheritier
added
area/providers/gemini
d0e681d to
fe025fe
Compare
fe025fe to
f931b11
Compare
54bf686 to
f931b11
Compare
Diagnose the observed google/gemini-2.5-flash-image HTTP 400 without dumping raw requests/prompts/schemas. RequestShape summarizes a built GenerateContentConfig as booleans/counts/fixed enums only (modalities, tool/toolconfig presence, structured-output presence, thinking config, API surface), safe to log at Debug and to hand to bug reports. Track apiSurface (gemini_api/vertex_ai/gateway) on Client so the shape can distinguish backends. Replace the old per-tool debug log in CreateChatCompletionStream, which logged full Tool structs including function names/descriptions/parameter schemas, with the bounded shape. Trace the Split Diff local-only classification from the actual request-building boundary rather than just this package: the regression scans this package (where the request is serialized) plus pkg/tools (where the []tools.Tool list handed to every provider is built) and pkg/runtime (where messages/tools are assembled before any provider call) for any reference to the TUI-local Split Diff toggle, and finds none. MCP and other dynamically registered tools are outside the reach of a static scan; that boundary is documented on the test.
Introduce chat.SanitizeDisplayName and chat.TruncateUTF8Bytes with the canonical MaxSanitizedFieldBytes (128) field bound. Provider-supplied display names are untrusted model output: the sanitizer neutralizes control characters, path separators, traversal-like sequences, and angle brackets (so a name can never forge an XML/tag boundary in prompts), and the truncation helper enforces byte bounds without splitting multi-byte runes. These helpers are shared infrastructure: the Gemini 400 classifier uses them to bound diagnostic fields, and later generated-media commits use them for every persisted or displayed metadata field.
google.golang.org/genai's newAPIError returns APIError by value (its Error() method has a value receiver), so wrapGeminiError's previous errors.AsType[*genai.APIError] type parameter never matched. Every Gemini API error — including the observed empty-body 400 — passed through unwrapped: no *modelerrors.StatusError, no status code for the retry loop, no classification. Fix the type parameter to the value type. Add a bounded RequestRejectionCategory, derived only from keyword matches against Google's own documented request field names in the SDK's Message (never Details or request content), and APIRejectionError to render an action-oriented message in its place. This message reaches the user through the existing modelerrors.FormatError -> runtime ErrorEvent -> TUI AddErrorMessage seam unchanged — no new plumbing needed — replacing what was previously just "HTTP 400: " with no actionable content. Preserve a bounded, sanitized rendering of the SDK's own Message alongside the category/hint instead of replacing it outright: general model-error classification (pkg/modelerrors) identifies context overflow and retryable transient function-response errors (issue #2683) by matching known phrases against err.Error(), and dropping Message broke both for Gemini 400s. An auth-shaped 400 (e.g. "API key not valid") isn't affected by either classifier — a *StatusError short-circuits before modelerrors' phrase-based fallback ever inspects it — but dropping Message there still cost the user the provider's own informative text, leaving only a generic "request shape" hint; that is a loss of visible context, not a classifier failure. sanitizeAPIErrorMessage bounds the result, collapses control characters/newlines to a single line, rewrites '{'/'}' to '('/')' so a JSON-object-shaped envelope embedded in Message can't later be re-parsed by modelerrors.StatusError.Error and have its fields substitute for this package's own category/hint, and redacts any recognizable credential via portcullis.Redact before it is echoed; Details (the raw response body) is still never surfaced. Truncate the sanitized message via pkg/chat.TruncateUTF8Bytes rather than duplicating a local copy of it: that helper is already available in pkg/chat, so there is no standalone-build reason left to keep a package-local truncateUTF8Bytes. Cover the classification chain (overflow, transient retry, auth) and the JSON-envelope-neutralization/redaction/bounding contract with new tests; pkg/chat.TruncateUTF8Bytes's own multi-byte boundary conditions are covered by its existing tests in pkg/chat.
f931b11 to
c17dfaf
Compare
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Reviewed the #4017-only delta (three-dot vs media-input-modalities, head c17dfaf). CI green for this SHA (9 success / 3 release-only skipped). Locally: build/vet/lint clean, -race green on gemini/chat/modelerrors/runtime, and 7/7 -overlay mutants killed (pointer→value revert, dropped Message, no brace neutralization, no redaction, no bound, non-400 wrapping, keyword reorder) — the tests are load-bearing.
Verified the modelerrors text-matching pitfalls: the original genai.APIError stays reachable via errors.As, Message text still flows through StatusError.Error(), #2683's transient 400 stays retryable, and Gemini overflow 400s still hit IsContextOverflowError → compaction. Details is never rendered; RequestShape logs only bools/counts/enums.
No blocking findings. Approve-equivalent once un-drafted.
[should-fix] diagnostics_test.go:260-333 — TestSplitDiffView_NeverReferencedInToolOrRequestBuildingSource greps pkg/tools and pkg/runtime source for split_diff variants. It exercises no code in this PR and will fail on any future legitimate use of that substring elsewhere with a misleading message. Suggest recording the investigation conclusion in the PR/issue text and dropping the cross-package scan.
[optional] wrap.go:93 — the 300-byte bound is also what modelerrors classifies against; a ≥301-byte prefix before "input token count" loses overflow classification (verified). Real Gemini messages are short, so edge-only — consider a larger bound or a note on the constant.
[optional] classify.go — genai.APIError.Status and Details[].reason / fieldViolations[].field are structural, value-free signals that could precede keyword matching (e.g. API_KEY_INVALID instead of other). The doc comment also says keywords are "only documented field names", but the last group is generic English.
[optional] pkg/chat/display_name.go — SanitizeDisplayName/MaxSanitizedFieldBytes are unused at this head and their comments reference pkg/runtime/harness.go <role> blocks and a runtime MIME sanitizer that don't exist yet. Fine for the stack, dangling if merged alone.
[optional] wrap.go:38-39,62-63 — in the SDK's fallback paths Message (not Details) carries the raw body; the sanitizer covers it, only the comment is off.
1 participant
What
Classifies Gemini API 400 responses into safe, actionable categories and surfaces consistent display names and diagnostics.
Why
Turns opaque Gemini request failures into useful user-facing guidance without exposing unsafe provider details.
Validation
task test; Gemini classification, diagnostics, wrapping, and display-name tests.Test instructions
Run:
task testSend an intentionally invalid Gemini request. Expected: the failure is reported in a safe, actionable category with a useful display name and diagnostic guidance, rather than exposing a raw or misleading HTTP 400 response.