fix(cloaking): string-format system prompt silently dropped, causing lost user instructions#1990
fix(cloaking): string-format system prompt silently dropped, causing lost user instructions#1990shellus wants to merge 2 commits intorouter-for-me:mainfrom
Conversation
…tructionsWithMode checkSystemInstructionsWithMode only handled array-format system prompts. When a client sends a string-format system prompt (e.g. "system": "You are a helpful assistant"), the user's prompt was silently dropped because the code only iterated over system.IsArray() elements. This caused two issues: 1. The user's system prompt was lost — the AI never received it. 2. The cloaked payload contained only 2 system blocks (billing + agent identity) instead of the expected 3+, which some upstreams validate. Add an else-if branch for gjson.String to convert the string into an array element with cache_control, appended after the billing and agent blocks. Empty strings are skipped to avoid injecting a blank block.
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 addresses a critical bug in the system prompt handling mechanism where user-provided system instructions, when supplied as a simple string, were being inadvertently discarded. This oversight resulted in AI models not receiving their intended persona or instructions and could cause downstream API validation errors. The changes introduce a robust parsing and conversion process for string-based system prompts, ensuring they are correctly integrated into the request payload and preserving the user's intended model behavior, thereby enhancing the reliability and correctness of API interactions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 correctly addresses an issue where string-format system prompts were being dropped. The fix involves converting string prompts to the expected array format, and the addition of comprehensive test cases is a great way to ensure correctness and prevent regressions. I have a couple of suggestions to improve the implementation. One is to use the standard library for JSON escaping to make the code more robust and secure. The other is to strengthen an assertion in one of the new tests.
| escaped := strings.ReplaceAll(system.String(), `\`, `\\`) | ||
| escaped = strings.ReplaceAll(escaped, `"`, `\"`) | ||
| escaped = strings.ReplaceAll(escaped, "\n", `\n`) | ||
| escaped = strings.ReplaceAll(escaped, "\r", `\r`) | ||
| escaped = strings.ReplaceAll(escaped, "\t", `\t`) | ||
| result += `,{"type":"text","text":"` + escaped + `","cache_control":{"type":"ephemeral"}}` |
There was a problem hiding this comment.
Manually escaping strings for JSON is error-prone and can miss edge cases, potentially leading to malformed JSON or security vulnerabilities. The current implementation handles common characters like \, ", \n, \r, and \t, but it doesn't cover all control characters that must be escaped according to the JSON specification (e.g., \b, \f).
Using json.Marshal from the standard library is a safer and more robust approach as it handles all necessary escaping automatically.
| escaped := strings.ReplaceAll(system.String(), `\`, `\\`) | |
| escaped = strings.ReplaceAll(escaped, `"`, `\"`) | |
| escaped = strings.ReplaceAll(escaped, "\n", `\n`) | |
| escaped = strings.ReplaceAll(escaped, "\r", `\r`) | |
| escaped = strings.ReplaceAll(escaped, "\t", `\t`) | |
| result += `,{"type":"text","text":"` + escaped + `","cache_control":{"type":"ephemeral"}}` | |
| escapedBytes, _ := json.Marshal(system.String()) | |
| result += `,{"type":"text","text":` + string(escapedBytes) + `,"cache_control":{"type":"ephemeral"}}` |
There was a problem hiding this comment.
Good catch. Replaced manual escaping with json.Marshal — it handles all control characters per the JSON spec. Fixed in d2474c0.
| if !strings.Contains(got, "line1") || !strings.Contains(got, "line2") { | ||
| t.Fatalf("system[2] should preserve multiline content, got: %s", got) | ||
| } |
There was a problem hiding this comment.
The current assertion only checks for the presence of substrings (line1, line2), which is a bit weak. For a more robust test, it's better to assert that the entire string matches the expected value exactly. This will catch any unintended changes to whitespace, special characters, or their ordering.
const want = "line1\nline2\t\"quoted\""
if got != want {
t.Fatalf("system[2] text = %q, want %q", got, want)
}There was a problem hiding this comment.
Agreed, strengthened to exact match. Fixed in d2474c0.
…st assertion Address code review feedback: - Replace manual string escaping with json.Marshal for robustness - Use exact string match in special chars test instead of substring check
shellus
changed the title
|
Closing this PR in favor of #1992, which has already been merged. The same fix is now on main. |
2 participants
Problem
checkSystemInstructionsWithModeonly handles array-format system prompts ("system": [...]). The Anthropic API also accepts string-format system prompts ("system": "text"), which many third-party clients use (OpenAI SDK adapters, custom chat applications, etc.).When a string-format system prompt is received, the existing code skips the
system.IsArray()branch and the user's prompt is silently dropped. The resulting cloaked payload contains only 2 system blocks (billing header + agent identity), missing the user's original content.This causes:
Fix
Add an
else if system.Type == gjson.Stringbranch incheckSystemInstructionsWithModeto convert the string prompt into a proper array element (withcache_control) appended after the billing and agent blocks. Empty strings are skipped to avoid injecting a blank block.Tests
Added 5 test cases covering: