Repository navigation
fix: eliminate inline system prompts — all prompts via temp files (#568) - #569
Conversation
…rompts Inline --append-system-prompt and --system-prompt flags broke when passed through tmux send-keys with multi-line prompts containing code blocks. All system prompts are now written to temp files at /tmp/genie-prompts/ and passed via --append-system-prompt-file or --system-prompt-file. When both systemPrompt and systemPromptFile are set, they are merged into a single temp file. Extra --append-system-prompt-file args are also merged. Closes #568
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 significantly enhances the robustness of system prompt handling by moving away from inline arguments to a temporary file-based approach. This change directly addresses and resolves a critical issue where complex, multi-line prompts with special characters were being corrupted by shell escaping mechanisms, leading to application crashes. By centralizing prompt content into temporary files, the system ensures reliable and consistent delivery of prompts, improving overall stability and agent performance, especially for those utilizing rich, code-block-heavy instructions. 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 successfully addresses a critical shell escaping issue by switching from inline system prompts to temporary files. The implementation is a solid step forward. However, I've identified a few areas for improvement. There's a resource leak as temporary files are not cleaned up, a bug in parsing multiple prompt file arguments, and some minor deviations from best practices regarding code style and test hygiene. My review includes specific suggestions to address these points and enhance the robustness and maintainability of the code.
| if (params.systemPrompt) { | ||
| // Write built-in prompt to temp file — avoids shell escaping of complex content | ||
| const { mkdirSync, writeFileSync, readFileSync } = require('node:fs'); | ||
| const { join } = require('node:path'); | ||
| const dir = '/tmp/genie-prompts'; | ||
| mkdirSync(dir, { recursive: true }); | ||
| const ts = Date.now().toString(36); | ||
| const promptFile = join(dir, `${params.role || 'agent'}-${ts}.md`); | ||
|
|
||
| // If there is also a systemPromptFile (user agent), merge both | ||
| let content = params.systemPrompt; | ||
| if (params.systemPromptFile) { | ||
| content = readFileSync(params.systemPromptFile, 'utf-8') + '\n\n' + content; | ||
| } | ||
|
|
||
| // If extraArgs has --append-system-prompt-file, merge that too | ||
| if (params.extraArgs) { | ||
| const fileIdx = params.extraArgs.indexOf('--append-system-prompt-file'); | ||
| if (fileIdx !== -1 && params.extraArgs[fileIdx + 1]) { | ||
| content = content + '\n\n' + readFileSync(params.extraArgs[fileIdx + 1], 'utf-8'); | ||
| // Remove the extra arg since we merged it | ||
| params.extraArgs.splice(fileIdx, 2); | ||
| } | ||
| } | ||
|
|
||
| writeFileSync(promptFile, content); | ||
| const flag = params.promptMode === 'system' ? '--system-prompt-file' : '--append-system-prompt-file'; | ||
| parts.push(flag, escapeShellArg(promptFile)); | ||
| } else if (params.systemPromptFile) { | ||
| const flag = params.promptMode === 'system' ? '--system-prompt-file' : '--append-system-prompt-file'; | ||
| parts.push(flag, escapeShellArg(params.systemPromptFile)); | ||
| } else if (params.systemPrompt) { | ||
| const flag = params.promptMode === 'system' ? '--system-prompt' : '--append-system-prompt'; | ||
| parts.push(flag, escapeShellArg(params.systemPrompt)); | ||
| } |
There was a problem hiding this comment.
This new implementation for handling system prompts has a few issues I'd recommend addressing:
-
Resource Leak: Temporary files are created but never deleted. This will lead to an accumulation of files in the temp directory, which is a resource leak. A cleanup mechanism is crucial. I'd suggest modifying the
LaunchCommandinterface to return the path of the temporary file, making the caller responsible for cleanup after the command finishes execution. -
Bug with
extraArgs: The logic to merge--append-system-prompt-filefromextraArgsonly handles the first occurrence due to usingindexOf. If multiple flags are passed, the subsequent ones will be missed. This should be a loop to handle all occurrences. -
Hardcoded Temp Directory: The path
/tmpis hardcoded. It's better to useos.tmpdir()for portability across different operating systems. You'll need toimport { tmpdir } from 'node:os';. -
Code Style:
require()calls are made inside the function. It's a better practice to use top-level ES moduleimportstatements forfs,path, andosfor consistency and clarity.
Here's a suggested implementation that addresses points 2, 3, and 4 (while I've used require to make the suggestion self-contained, I recommend moving them to top-level imports). The resource leak (point 1) will require changes to the LaunchCommand interface, which is outside the scope of this suggestion but should be addressed.
if (params.systemPrompt) {
// Write built-in prompt to temp file — avoids shell escaping of complex content
const { mkdirSync, writeFileSync, readFileSync } = require('node:fs');
const { join } = require('node:path');
const { tmpdir } = require('node:os');
const dir = join(tmpdir(), 'genie-prompts');
mkdirSync(dir, { recursive: true });
const ts = Date.now().toString(36);
const promptFile = join(dir, `${params.role || 'agent'}-${ts}.md`);
// If there is also a systemPromptFile (user agent), merge both
let content = params.systemPrompt;
if (params.systemPromptFile) {
content = readFileSync(params.systemPromptFile, 'utf-8') + '\n\n' + content;
}
// If extraArgs has --append-system-prompt-file, merge that too
if (params.extraArgs) {
let fileIdx;
while ((fileIdx = params.extraArgs.indexOf('--append-system-prompt-file')) !== -1) {
if (fileIdx + 1 < params.extraArgs.length) {
content = content + '\n\n' + readFileSync(params.extraArgs[fileIdx + 1], 'utf-8');
// Remove the flag and filename
params.extraArgs.splice(fileIdx, 2);
} else {
// Malformed, just remove the flag
params.extraArgs.splice(fileIdx, 1);
}
}
}
writeFileSync(promptFile, content);
const flag = params.promptMode === 'system' ? '--system-prompt-file' : '--append-system-prompt-file';
parts.push(flag, escapeShellArg(promptFile));
} else if (params.systemPromptFile) {
const flag = params.promptMode === 'system' ? '--system-prompt-file' : '--append-system-prompt-file';
parts.push(flag, escapeShellArg(params.systemPromptFile));
}| it('merges systemPromptFile and systemPrompt into one temp file', () => { | ||
| const fs = require('node:fs'); | ||
| const testFile = '/tmp/genie-prompts/test-agents.md'; | ||
| fs.mkdirSync('/tmp/genie-prompts', { recursive: true }); | ||
| fs.writeFileSync(testFile, 'User agent instructions'); | ||
|
|
||
| const result = buildClaudeCommand({ | ||
| provider: 'claude', | ||
| team: 'work', | ||
| role: 'implementor', | ||
| systemPromptFile: testFile, | ||
| systemPrompt: 'Built-in prompt', | ||
| }); | ||
| expect(result.command).toContain('--append-system-prompt-file'); | ||
| // Should reference the NEW temp file, not the original | ||
| expect(result.command).toContain('/tmp/genie-prompts/implementor-'); | ||
|
|
||
| // Verify merged content | ||
| const match = result.command.match(/\/tmp\/genie-prompts\/implementor-[^']+/); | ||
| expect(match).toBeTruthy(); | ||
| const content = fs.readFileSync(match![0], 'utf-8'); | ||
| expect(content).toContain('User agent instructions'); | ||
| expect(content).toContain('Built-in prompt'); | ||
| }); |
There was a problem hiding this comment.
This test, and others in this file, create files and directories in /tmp/genie-prompts but do not clean them up. This creates side effects that can pollute the test environment and leave garbage on the file system.
Additionally, I noticed there's missing test coverage for the new logic that merges --append-system-prompt-file from extraArgs in buildClaudeCommand.
I recommend the following:
- Use test lifecycle hooks like
afterEachorafterAllto clean up any files and directories created during the test run. You can use a unique temporary directory for each test run usingfs.mkdtempto make cleanup easier. - Add new test cases to verify the
extraArgsmerging logic, especially a case with multiple--append-system-prompt-fileflags to ensure it's handled correctly.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e8d711325
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const dir = '/tmp/genie-prompts'; | ||
| mkdirSync(dir, { recursive: true }); | ||
| const ts = Date.now().toString(36); | ||
| const promptFile = join(dir, `${params.role || 'agent'}-${ts}.md`); |
There was a problem hiding this comment.
Using params.role directly in join(dir, ${params.role || 'agent'}-${ts}.md) allows path traversal and absolute-path escapes when the role contains /, .., or starts with /. Agent names are only validated as non-empty elsewhere, so a role like ../../pwn causes prompt files to be written outside /tmp/genie-prompts (e.g. /pwn-<ts>.md), which is an unintended arbitrary file write and can also make worker startup fail on permission errors.
Useful? React with 👍 / 👎.
1 participant
Summary
Kills inline
--append-system-prompt/--system-promptflags entirely. ALL system prompts now go through--append-system-prompt-file/--system-prompt-fileusing temp files at/tmp/genie-prompts/.Fixes #568 — leader pane crashing because 2850-char multi-line prompt with bash code blocks was corrupted by shell escaping + tmux send-keys.
Root Cause
buildClaudeCommand()passed built-in agent prompts inline viaescapeShellArg()which wraps in single quotes. Multi-line prompts with nested quotes, backticks, and$()patterns get double-escaped through tmux send-keys, corrupting the command and crashing Claude Code on startup.Fix
One code path: when
systemPromptis set, write to/tmp/genie-prompts/<role>-<timestamp>.mdand use--append-system-prompt-file. If there are additional prompt files (leader's wish context), merge into one file.Test plan
bun run typecheckpasses--append-system-promptor--system-promptin codebasegenie team create --wish→ leader stays alive