Repository navigation
fix: eliminate inline system prompts — all prompts via temp files (#568) #569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,7 +68,7 @@ export interface SpawnParams { | |
| resume?: string; | ||
| /** Path to a system prompt file (AGENTS.md). Emits --system-prompt-file or --append-system-prompt-file. */ | ||
| systemPromptFile?: string; | ||
| /** Inline system prompt text (for built-ins without an AGENTS.md file). Emits --append-system-prompt or --system-prompt. */ | ||
| /** Inline system prompt text (for built-ins without an AGENTS.md file). Written to temp file, emits --append-system-prompt-file or --system-prompt-file. */ | ||
| systemPrompt?: string; | ||
| /** How to inject the system prompt file: 'system' replaces CC default, 'append' adds to it. */ | ||
| promptMode?: 'system' | 'append'; | ||
|
|
@@ -232,12 +232,37 @@ export function buildClaudeCommand(params: SpawnParams): LaunchCommand { | |
|
|
||
| if (params.model) parts.push('--model', escapeShellArg(params.model)); | ||
|
|
||
| if (params.systemPromptFile) { | ||
| 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`); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Using Useful? React with 👍 / 👎. |
||
|
|
||
| // 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)); | ||
| } | ||
|
Comment on lines
+235
to
266
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new implementation for handling system prompts has a few issues I'd recommend addressing:
Here's a suggested implementation that addresses points 2, 3, and 4 (while I've used 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));
} |
||
|
|
||
| if (params.extraArgs) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test, and others in this file, create files and directories in
/tmp/genie-promptsbut 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-filefromextraArgsinbuildClaudeCommand.I recommend the following:
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.extraArgsmerging logic, especially a case with multiple--append-system-prompt-fileflags to ensure it's handled correctly.