Conversation
Encoding chat history that contained a part with missing or unrecognized data received from the server trapped in JSONEncoder on the next turn (TemplateChat unary and streaming, Chat streaming). Chat.sendMessage hit fatalError in ModelContent.init when the response contained code execution parts. A proto duration with an exponent in its fractional part (e.g. "1.5e10s") overflowed Int32 while decoding. Also add tests feeding malformed and unexpected template responses. Refs #16728
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses crashes related to unrecognized data or code execution parts in chat history, as well as malformed duration decoding. It adds support for ExecutableCodePart and CodeExecutionResultPart in ModelContent, prevents crashes when encoding nil data in InternalPart, and restricts ProtoDuration nanoseconds to ASCII digits to avoid overflow. The review feedback suggests replacing fatalError() with assertionFailure and logging when encountering unrecognized Part types to prevent production crashes.
Addresses review feedback: replace the fatalError() in ModelContent.init(role:parts:) for unrecognized Part types with an AILog error and skip the part. Part is a public protocol that apps can conform to, and the internal ErrorPart (returned for failed image conversions) also reached this branch, so both are runtime-reachable paths. ErrorPart is handled explicitly with its own message code.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the SDK's robustness against unexpected or malformed server responses. It prevents crashes by safely skipping and logging unsupported or failed part types (such as code execution parts) during ModelContent conversion, and avoids JSONEncoder traps by conditionally encoding optional data in InternalPart. Additionally, it hardens ProtoDuration decoding by validating that nanoseconds consist only of digits to prevent integer overflow. Comprehensive unit tests are added to verify these behaviors. The feedback suggests optimizing the digit validation check in ProtoDuration.swift to use a direct range check, avoiding the overhead of Unicode property lookups.
Addresses review feedback on #16756: adds test cases for non-ASCII numerics (Arabic-Indic digit, vulgar fraction, superscript) and an ASCII digit with a combining mark, and clarifies that the existing isASCII && isNumber check only admits ASCII 0-9. The suggested ("0"..."9").contains($0) range check was not adopted because it also accepts grapheme clusters such as "1\u{301}".
| case let errorPart as ErrorPart: | ||
| AILog.error( | ||
| code: .modelContentPartConversionFailed, | ||
| "Skipping a part that failed to convert to model content: \(errorPart.error)" | ||
| ) |
There was a problem hiding this comment.
This was already broken before this PR but now has a different problem. Before it would call fatalError on images that can't be converted. I think the original approach (before InternalPart was added for thought signatures) was to call [ModelContent].throwIfError().
I think now passing an invalid image (e.g., UIImage() or CIImage.empty(), whose partsValue returns [ErrorPart(ImageConversionError...)] in PartsRepresentable+Image.swift) to model.generateContent("Describe this", invalidImage) silently drops the image and only sends the text prompt to the backend, or send an empty parts: [] array if the image was the only part. That's probably better than crashing but depends on whether it happens during development or in production.
Potentially we could keep the ErrorPart (something like a .error(ErrorPart) case on InternalPart.OneOfData) so throwIfError() can still throw ImageConversionError when making a request. I see it's also not called in TemplateGenerativeModel so that's another hole.
Feel free to add a TODO since it'd probably balloon the PR to address it here. As a stop-gap solution, could maybe add an assertionFailure so it'll still crash during development but only log during production (I'd need to double check that assertionFailure gets compiled out of release builds though).
There was a problem hiding this comment.
Good catch. It turned out smaller than expected, so I fixed it here in ccf2d7d:
OneOfDatais only exhaustively switched on in thepartsgetter andencode(to:), so I added an internal.error(ErrorPart)case.ModelContent.initkeeps theErrorPartagain, sothrowIfError()works.throwIfError()is now also called when building template requests (which coversTemplateChat) and incountTokens. It wraps the error, sogenerateContent("Describe this", UIImage())throwsGenerateContentError.internalError(underlying: ImageConversionError.couldNotConvertToJPEG)without sending a request.- Since
ModelContent.partscan now return anErrorPart, encoding one throws anEncodingError, and==compares error descriptions instead of callingfatalError. LiveSession.sendContentcan't throw, so a message with an invalid image now fails to encode and is logged and dropped as a whole, instead of being sent without the image.
On assertionFailure: it has no effect at -O, so release builds would only log. But SPM and CocoaPods compile FirebaseAI with the app's configuration, so it would trap in developers' Debug builds and unit tests on a failure that depends on app input rather than an SDK invariant. With the error thrown again, I left it out.
An image that can't be converted to JPEG (for example, `UIImage()` or `CIImage.empty()`) produces an internal `ErrorPart`. Skipping it in `ModelContent.init` meant `generateContent` silently sent the prompt without the image, or an empty `parts` array. Keep the `ErrorPart` as an internal `InternalPart.OneOfData.error` case so `throwIfError()` rejects the request again, wrapped in `GenerateContentError.internalError(underlying:)` to match the documented `Throws:`. Call `throwIfError()` when building template requests, which covers `TemplateChat`, and in `countTokens`; neither checked before. `ModelContent.parts` can now return the `ErrorPart`, so make its encoding throw an `EncodingError` and its `==` compare error descriptions instead of calling `fatalError()`. A `LiveSession` message with such a part now fails to encode and is logged instead of being sent without the image.
This branch has not been deployed
2 participants
Context: #16728. A malformed server payload crashed every released version of
another SDK. Server prompt templates keep the model, prompt, schema and config
on the server, so the client has to handle any response shape. This PR audits
response decoding for traps and fixes the three crashes it found. It also
removes the remaining
fatalError()paths inModelContent.init(role:parts:).Hardened code paths
InternalPart.encode(to:). Parts with missing or unrecognized data(for example, a part type the backend adds later) decode with
data == nil.TemplateChat(unary and streaming) andChat(streaming) keep these partsin
history. On the next turn, encoding the history ranOptional.none.encode(to:)and thenencoder.container(keyedBy:), whichhits a
preconditionFailureinJSONEncoder. The fix skips encodingdatawhen it isnil, sothoughtandthoughtSignaturestillround-trip.
ModelContent.init(role:parts:). Every part type it didn't handle fellthrough to
fatalError():ExecutableCodePartandCodeExecutionResultPart.Chat.sendMessagerebuilds the model turn with this initializer, so a unary chat response
containing code execution parts crashed. The fix adds the two missing
cases.
Partconformance defined by the app. It is now logged(
modelContentUnsupportedPartType) and skipped.ErrorPartthat an image produces when it can't be convertedto JPEG (for example,
UIImage()orCIImage.empty()). It is now kept asan internal
.errorcase, sothrowIfError()works again:generateContent,generateContentStream,Chat,countTokensandTemplateChatthrowGenerateContentError.internalError(underlying: ImageConversionError…)without sending a request.
throwIfError()wasn't called for templaterequests or
countTokensbefore. Encoding anErrorPartnow throws anEncodingError, and==no longer traps, sinceModelContent.partscanreturn one.
ProtoDurationdecoding (LiveGoAway.timeLeft). The fractional partwent through
Double("0.\(nanos)"), which accepts exponents. A value like"1.5e10s"then overflowedInt32(...). The fix requires the fractionalpart to be digits only; anything else throws the existing
DecodingError.The audit also checked these paths and found no trap: unknown enum values (they
already degrade to raw values), empty or missing
candidatesandparts(accessors use
.first), integer overflow and wrong types (they throwDecodingError), malformed SSE lines and malformed error bodies (they throw).Tests
These tests trapped before the fix (verified locally):
TemplateChatTests.testSendMessage_unrecognizedPartInResponse_nextTurnSucceedsTemplateChatTests.testSendMessageStream_unrecognizedPartInResponse_nextTurnSucceedsInternalPartTests.testEncodeUnsupportedPart_doesNotCrashChatTests.testSendMessage_unary_codeExecution_appendsHistoryProtoDurationTests.testDecodeProtoDuration_nonDigitNanosecondsThese tests cover the other
ModelContentchanges. Onmain, each one hits afatalError()inModelContent.init(role:parts:)or inErrorPart:PartTests.testModelContent_unsupportedPartType_isSkippedAndLoggedPartTests.testModelContent_errorPart_isKeptAndThrowIfErrorThrowsPartTests.testModelContent_errorPart_encodingThrowsPartTests.testModelContent_errorPart_equalityPartTests.testErrorPart_encodingAndDecodingThrowGenerativeModelVertexAITests.testGenerateContent_failure_invalidImageGenerativeModelVertexAITests.testGenerateContentStream_failure_imageConversionErrorGenerativeModelVertexAITests.testCountTokens_failure_imageConversionErrorTemplateChatTests.testSendMessage_imageConversionError_throwsWithoutSendingTemplateChatTests.testSendMessageStream_imageConversionError_throwsWithoutSendingThese are coverage-only tests in
TemplateGenerativeModelTests; they alreadypass on
main:are non-integer
A small
GenerativeModelTestUtil.httpRequestHandler(body:statusCode:)helperserves inline response bodies.
Testing
xcodebuild -scheme FirebaseAILogicUnit test -destination 'platform=macOS'with mock responses from
scripts/update_vertexai_responses.sh: 413tests, 0 failures, 15 skipped.
Notes
as
{}(or{"thoughtSignature": ...}) instead of crashing. The backendmay reject an empty part with an error. A follow-up could drop such parts
from outgoing history; this PR leaves that out to keep the change minimal.
LiveSession.sendContentcan't throw. A message containing an image thatfails to convert now fails to encode and is logged
(
liveSessionFailedToEncodeClientMessage) and dropped, instead of crashing.