Add Gemini buddy example#190
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new development example, gemini-buddy-mcp, which implements an MCP service wrapping the Gemini CLI to maintain multi-turn conversation state server-side. The feedback highlights several critical areas for improvement: resolving potential character corruption by explicitly setting UTF-8 encoding on child process streams, avoiding the security risk of hardcoding API keys in the Dockerfile, sanitizing user messages containing newlines to preserve the transcript structure, and addressing potential race conditions from concurrent requests on the in-memory session store.
| let stdout = ""; | ||
| let stderr = ""; | ||
| child.stdout.on("data", (d) => { stdout += d; }); | ||
| child.stderr.on("data", (d) => { stderr += d; }); |
There was a problem hiding this comment.
When reading from child.stdout and child.stderr, the data chunks are emitted as Buffer objects. Concatenating them directly to a string (stdout += d) implicitly converts each buffer chunk to a string. If a multi-byte character (such as an emoji or non-ASCII character) is split across chunk boundaries, this implicit conversion will result in corrupted characters (e.g., ``).
To prevent this, set the encoding of the streams to 'utf8' before listening to the 'data' event, which ensures that multi-byte characters are decoded correctly across chunk boundaries.
| let stdout = ""; | |
| let stderr = ""; | |
| child.stdout.on("data", (d) => { stdout += d; }); | |
| child.stderr.on("data", (d) => { stderr += d; }); | |
| let stdout = ""; | |
| let stderr = ""; | |
| child.stdout.setEncoding("utf8"); | |
| child.stderr.setEncoding("utf8"); | |
| child.stdout.on("data", (d) => { stdout += d; }); | |
| child.stderr.on("data", (d) => { stderr += d; }); |
| # Dev-only: replace <API_KEY> with a Google AI Studio free-tier key before `make kind`. | ||
| # Keep the edit local so it is never committed: | ||
| # git update-index --skip-worktree development/examples/gemini-buddy-mcp/Dockerfile | ||
| ENV GEMINI_API_KEY=<API_KEY> |
There was a problem hiding this comment.
Hardcoding API keys in the Dockerfile (even with instructions to use git update-index --skip-worktree) is a security risk. The key can easily be baked into the Docker image layers, making it retrievable by anyone with access to the image, and it is prone to accidental commits if the local git configuration is reset or cloned elsewhere.
Instead, GEMINI_API_KEY should be passed as a runtime environment variable to the container (e.g., via Kubernetes secrets, Docker run flags, or local compose/mesh configuration) and not defined with a default placeholder in the Dockerfile.
| function render(turns) { | ||
| return turns.map((t) => `${t.role === "user" ? "User" : "Buddy"}: ${t.text}`).join("\n"); | ||
| } |
There was a problem hiding this comment.
If a user message contains newlines, the rendered transcript will contain lines that are not prefixed with User: or Buddy:. This violates the assumption in the system prompt (You'll get the conversation so far on stdin, lines prefixed 'User:' and 'Buddy:') and can confuse the model.
Consider sanitizing or replacing newlines in the message text to keep the transcript structure clean and predictable.
function render(turns) {
return turns.map((t) => {
const role = t.role === "user" ? "User" : "Buddy";
const cleanText = t.text.replace(/\\r?\\n/g, " ");
return `${role}: ${cleanText}`;
}).join("\\n");
}| async ({ message, session_id }) => { | ||
| const id = session_id ?? "default"; | ||
| const turns = transcript(id); | ||
| turns.push({ role: "user", text: message }); | ||
| try { | ||
| const reply = await runGemini(render(turns)); | ||
| turns.push({ role: "assistant", text: reply }); | ||
| return { content: [{ type: "text", text: reply }] }; | ||
| } catch (err) { | ||
| turns.pop(); // drop the turn we couldn't answer so it can be retried | ||
| return { content: [{ type: "text", text: String(err?.message ?? err) }], isError: true }; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Since the server handles requests asynchronously and stores session history in a shared in-memory sessions Map, concurrent requests for the same session_id can lead to race conditions. If two chat requests for the same session are processed concurrently, both will read the same initial transcript, append their respective messages, and call runGemini in parallel. This can result in corrupted history, duplicate turns, or out-of-order messages.
While this is a development example, it is worth noting that in a production environment, you should serialize requests per session (e.g., using a promise chain or queue per session ID) or use a persistent, transactional database.
No description provided.