opt(llm): thinking signature encode with channel footprint, close #1035#1066
opt(llm): thinking signature encode with channel footprint, close #1035#1066looplj merged 2 commits intorelease/v0.9.xfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of LLM thinking signatures by embedding a channel-specific identifier, or 'footprint,' into them. This change ensures that reasoning content and encrypted data are correctly attributed and processed, preventing potential conflicts or misinterpretations when messages are transformed or passed between different LLM channels or providers. The footprint is propagated through the request context and integrated into the encoding and decoding logic of various transformer types. Highlights
Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a channel footprint mechanism to disambiguate thinking signatures from different channels, which is a great enhancement for multi-channel routing. The implementation is comprehensive, touching many transformers to propagate the channel identity and using it to create a unique footprint.
I've found a few issues in the shared signature handling logic that could lead to incorrect behavior when decoding signatures, particularly with how wildcard footprints are handled. Additionally, there's a potential for a nil pointer dereference in one of the Gemini transformers.
My review includes suggestions to fix these issues and improve the robustness of the new footprinting feature.
| APIKey: apiKey, | ||
| HeaderKey: "x-goog-api-key", | ||
| } | ||
| apiKey := t.config.APIKeyProvider.Get(ctx) |
There was a problem hiding this comment.
This direct call to t.config.APIKeyProvider.Get(ctx) could cause a panic if APIKeyProvider is nil. While the current usage in channel_llm.go ensures it's not nil, it's safer for the transformer to be self-contained and robust against being initialized with a nil provider. Please add a nil check for t.config.APIKeyProvider before calling Get.
llm/transformer/shared/anthropic.go
Outdated
| if embeddedFootprint == "" { | ||
| if footprint != "" { | ||
| return nil | ||
| } | ||
| } else if embeddedFootprint != footprint { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The logic here for handling a wildcard footprint seems incorrect. If footprint is an empty string (wildcard), but the signature has an embeddedFootprint, this function will incorrectly return nil. This would prevent decoding of any footprinted signature when a specific footprint isn't required.
A better approach would be to only perform the footprint match when the provided footprint is not empty.
if footprint != "" && embeddedFootprint != footprint {
return nil
}
llm/transformer/shared/gemini.go
Outdated
| if embeddedFootprint == "" && footprint != "" { | ||
| return nil | ||
| } | ||
|
|
||
| if embeddedFootprint != "" && embeddedFootprint != footprint { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Similar to my comment on anthropic.go, the logic for handling a wildcard footprint in DecodeGeminiThoughtSignatureWithFootprint is incorrect. It will fail to decode a signature that has a footprint when the caller doesn't specify one (i.e., provides an empty string as a wildcard).
To fix this and align with the wildcard behavior, I suggest simplifying the validation logic. The check should only fail if a specific footprint is provided and it doesn't match the embeddedFootprint.
| if embeddedFootprint == "" && footprint != "" { | |
| return nil | |
| } | |
| if embeddedFootprint != "" && embeddedFootprint != footprint { | |
| return nil | |
| } | |
| if footprint != "" && embeddedFootprint != footprint { | |
| return nil | |
| } |
llm/transformer/shared/openai.go
Outdated
| if embeddedFootprint == "" && footprint != "" { | ||
| return nil | ||
| } | ||
|
|
||
| decoded := (*content)[len(OpenAIEncryptedContentPrefix):] | ||
| if embeddedFootprint != "" && embeddedFootprint != footprint { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This DecodeOpenAIEncryptedContentWithFootprint function has the same logical issue as the other decode functions in this PR regarding wildcard footprints. When footprint is an empty string, it should successfully decode a signature regardless of whether it has an embedded footprint. The current implementation will incorrectly fail in this scenario.
To ensure correct wildcard behavior, please simplify the validation to only enforce a match when a non-empty footprint is provided.
| if embeddedFootprint == "" && footprint != "" { | |
| return nil | |
| } | |
| decoded := (*content)[len(OpenAIEncryptedContentPrefix):] | |
| if embeddedFootprint != "" && embeddedFootprint != footprint { | |
| return nil | |
| } | |
| if footprint != "" && embeddedFootprint != footprint { | |
| return nil | |
| } |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AccountIdentity field to LLM transformer configurations across various platforms (Anthropic, Gemini, OpenAI Responses, and Codex). This identity, derived from the channel ID, is used to generate a unique 'footprint' for each request. This footprint is then propagated through the request context and included in HTTP request metadata, enabling more precise tracking and handling of provider-specific reasoning signatures and encrypted content during transformations, especially when switching between different LLM providers within the same session. The changes involve updating configuration structs, modifying request/response transformation logic to compute and utilize this footprint, and updating related test cases to reflect these new parameters and assertions.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a "channel footprint" to the LLM thinking signature encoding mechanism. This is a significant refactoring across many transformer files to make signature handling scope-aware, which should improve robustness when switching between different LLM providers. The changes are extensive and appear consistent with the new tests added. I've found one critical issue that could lead to a panic.
| APIKey: apiKey, | ||
| HeaderKey: "x-goog-api-key", | ||
| } | ||
| apiKey := t.config.APIKeyProvider.Get(ctx) |
There was a problem hiding this comment.
The nil check for t.config.APIKeyProvider was removed before calling .Get(ctx). If APIKeyProvider is nil, this will cause a panic. It's safer to restore the nil check to prevent a potential nil pointer dereference.
For example:
if t.config.APIKeyProvider != nil {
apiKey := t.config.APIKeyProvider.Get(ctx)
if apiKey != "" {
auth = &httpclient.AuthConfig{
Type: "api_key",
APIKey: apiKey,
HeaderKey: "x-goog-api-key",
}
}
}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Uh oh!
There was an error while loading. Please reload this page.