feat: token-efficient system prompt delivery for CLI agents#596
Conversation
Separate system prompt from user message text when spawning CLI agents. For Claude Code, use --append-system-prompt flag to deliver system context as a proper system prompt instead of embedding it in the user message. For other agents, fall back to embedding in the prompt (no equivalent flag). On Windows, text-only messages to Claude Code now use raw stdin instead of stream-json API format wrapping. Stream-json is still used when images are present (required for base64 transport). Changes: - Add appendSystemPrompt field to ProcessConfig (renderer, preload, global types) - Add supportsAppendSystemPrompt capability flag to AgentCapabilities - Route system prompt delivery in process:spawn handler (args vs embed fallback) - Update all 3 renderer call sites to pass system prompt separately - Add hasImages param to getStdinFlags for conditional stream-json stdin - Update tests for new spawn config shape and stdin flag behavior
📝 WalkthroughWalkthroughAdded a new AgentCapabilities flag Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (hook/store)
participant IPC as Main IPC Handler
participant AgentMgr as Agent Capabilities
participant Spawn as Process Spawn
Renderer->>Renderer: build prompt, substitute maestro system prompt -> appendSystemPrompt?
Renderer->>IPC: spawn({ prompt, appendSystemPrompt, images?, isSshSession, supportsStreamJsonInput })
IPC->>AgentMgr: check capabilities.supportsAppendSystemPrompt
alt supportsAppendSystemPrompt == true
IPC->>Spawn: spawn args + "--append-system-prompt <redacted>", prompt (user)
Spawn->>Spawn: agent receives separate system prompt arg
else
IPC->>IPC: effectivePrompt = embed appendSystemPrompt into prompt
IPC->>Spawn: spawn args, prompt = effectivePrompt
Spawn->>Spawn: agent receives combined prompt
end
sequenceDiagram
participant Caller as Renderer call-site
participant Helper as spawnHelpers
participant Platform as Platform detection
Caller->>Helper: getStdinFlags({isSshSession, supportsStreamJsonInput, hasImages})
Helper->>Platform: detect Windows?
Platform-->>Helper: platform result
alt Windows && !isSsh && supportsStreamJsonInput && hasImages
Helper-->>Caller: { sendPromptViaStdin: true, sendPromptViaStdinRaw: false }
else if Windows && !isSsh
Helper-->>Caller: { sendPromptViaStdin: false, sendPromptViaStdinRaw: true }
else
Helper-->>Caller: { sendPromptViaStdin: false, sendPromptViaStdinRaw: false }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
- Add supportsAppendSystemPrompt to capabilities test expected keys - Update inline wizard tests: text-only messages now use raw stdin instead of stream-json on Windows (no images = no stream-json)
…ages param - Redact --append-system-prompt value from logged args and fullCommand to avoid leaking large system prompt content into Sentry logs - Add logger.warn when system prompt is dropped because effectivePrompt is empty on the fallback (non-append-system-prompt) path - Make hasImages required in getStdinFlags to prevent silent default behavior; add explicit false at all non-image call sites
Greptile SummaryThis PR improves token efficiency for CLI agents by separating the Maestro system prompt from the user message at spawn time. For Claude Code, the system prompt is now delivered via Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[process:spawn IPC\nwith appendSystemPrompt] --> B{agent.supportsAppendSystemPrompt?}
B -- No --> C{effectivePrompt non-empty?}
C -- Yes --> D[Embed: prepend system prompt\nto user message]
C -- No --> E[Use system prompt\nas sole content]
B -- Yes --> F{isWindows AND\nnot SSH session?}
F -- Yes --> G[Write system prompt\nto temp file\nmaestro-sysprompt-id.txt]
G --> H[Pass --append-system-prompt-file\nwith temp path\nSchedule 30s cleanup]
F -- No --> I[Pass --append-system-prompt\ninline in args]
D --> J[Spawn agent with\neffectivePrompt]
E --> J
H --> J
I --> J
J --> K{SSH session?}
K -- Yes --> L[Prompt delivered via\nSSH stdin script]
K -- No --> M{Windows local?}
M -- Yes, images --> N[sendPromptViaStdin\nstream-json format]
M -- Yes, no images --> O[sendPromptViaStdinRaw\nraw text stdin]
M -- No --> P[Prompt in CLI args]
Reviews (3): Last reviewed commit: "style: fix prettier formatting in proces..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/stores/agentStore.ts (1)
299-345:⚠️ Potential issue | 🟠 MajorMirror the Windows stdin routing in queued spawns.
useInputProcessingnow setssendPromptViaStdin/sendPromptViaStdinRaw, butprocessQueuedItemstill doesn't. Once a turn enters the queue, Windows falls back to the old argv-based path: text-only follow-ups stop using raw stdin, and image-bearing Claude/factory-droid turns never opt into stream-json stdin. That makes queued behavior diverge from immediate sends and can regress on long/quoted prompts or image delivery.💡 Suggested fix
import { gitService } from '../services/git'; import { filterYoloArgs } from '../utils/agentArgs'; +import { getStdinFlags } from '../utils/spawnHelpers'; @@ const hasImages = item.images && item.images.length > 0; const hasText = item.text && item.text.trim(); const isImageOnlyMessage = item.type === 'message' && hasImages && !hasText; + const isSshSession = + !!session.sshRemoteId || !!session.sessionSshRemoteConfig?.enabled; @@ console.log('[processQueuedItem] Spawning agent with queued message:', { @@ args: spawnArgs, }); + + const stdinFlags = getStdinFlags({ + isSshSession, + supportsStreamJsonInput: agent.capabilities?.supportsStreamJsonInput ?? false, + hasImages, + }); await window.maestro.process.spawn({ @@ prompt: effectivePrompt, images: hasImages ? item.images : undefined, appendSystemPrompt, + ...stdinFlags, agentSessionId: tabAgentSessionId ?? undefined, @@ await window.maestro.process.spawn({ + ...getStdinFlags({ + isSshSession, + supportsStreamJsonInput: agent.capabilities?.supportsStreamJsonInput ?? false, + hasImages: false, + }), sessionId: targetSessionId, toolType: session.toolType, cwd: session.cwd, command: commandToUse, args: spawnArgs, prompt: substitutedPrompt, appendSystemPrompt: appendSystemPromptForCommand,Also applies to: 406-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/stores/agentStore.ts` around lines 299 - 345, processQueuedItem is not passing the stdin-routing flags set by useInputProcessing, causing queued turns to fall back to argv-based prompts; update the spawn call inside processQueuedItem to include sendPromptViaStdin and sendPromptViaStdinRaw (and any other flags useInputProcessing computes) using the same logic/values you use for immediate sends so image-bearing Claude/factory-droid and raw-stdin flows behave identically for queued messages; apply the same change to the equivalent spawn block around lines 406-447 so both queued spawn sites mirror useInputProcessing decisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/types/index.ts`:
- Line 771: The AgentCapabilities definition in the renderer hook is missing the
required supportsAppendSystemPrompt flag and types/index.ts marks it optional;
update the hook file (useAgentCapabilities.ts) to add
supportsAppendSystemPrompt: boolean to the AgentCapabilities interface and
include it in DEFAULT_CAPABILITIES, and change the declaration in
src/renderer/types/index.ts so supportsAppendSystemPrompt is a non-optional
boolean to match the main-process canonical type.
---
Outside diff comments:
In `@src/renderer/stores/agentStore.ts`:
- Around line 299-345: processQueuedItem is not passing the stdin-routing flags
set by useInputProcessing, causing queued turns to fall back to argv-based
prompts; update the spawn call inside processQueuedItem to include
sendPromptViaStdin and sendPromptViaStdinRaw (and any other flags
useInputProcessing computes) using the same logic/values you use for immediate
sends so image-bearing Claude/factory-droid and raw-stdin flows behave
identically for queued messages; apply the same change to the equivalent spawn
block around lines 406-447 so both queued spawn sites mirror useInputProcessing
decisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a00f751-6fc6-4a36-99fd-3ab7582a8ccf
📒 Files selected for processing (16)
src/__tests__/main/agents/capabilities.test.tssrc/__tests__/renderer/services/inlineWizardConversation.test.tssrc/__tests__/renderer/stores/agentStore.test.tssrc/__tests__/renderer/utils/spawnHelpers.test.tssrc/main/agents/capabilities.tssrc/main/ipc/handlers/process.tssrc/main/preload/process.tssrc/renderer/components/Wizard/services/conversationManager.tssrc/renderer/global.d.tssrc/renderer/hooks/agent/useAgentExecution.tssrc/renderer/hooks/agent/useMergeTransferHandlers.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/services/inlineWizardConversation.tssrc/renderer/stores/agentStore.tssrc/renderer/types/index.tssrc/renderer/utils/spawnHelpers.ts
- Use system prompt as sole content instead of dropping it when effectivePrompt is empty on the fallback path - Log systemPromptDelivery method and effectivePromptLength in spawn info log for debugging embedded vs cli-arg delivery - Add supportsAppendSystemPrompt to renderer hook AgentCapabilities interface and DEFAULT_CAPABILITIES for type consistency
- Local supported agent: --append-system-prompt in CLI args, clean prompt - Local unsupported agent: system prompt embedded in user message - Local unsupported + empty prompt: system prompt used as sole content - SSH supported agent: --append-system-prompt in remote exec script - SSH unsupported agent: system prompt embedded in SSH stdin - No appendSystemPrompt: flag not added, prompt unchanged - Fix useAgentCapabilities test fixture for new capability field
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/__tests__/main/ipc/handlers/process.test.ts (2)
1963-1990: Verify edge case:undefinedvs empty string forprompt.The test uses
prompt: ''(empty string) to verify the "no user message" branch. However, the handler conditionelse if (effectivePrompt)would also be falsy forundefined. Consider adding a test case withprompt: undefinedto ensure both cases are covered.💡 Additional test case
it('should use system prompt as sole content when prompt is undefined for unsupported agents', async () => { const mockAgent = { id: 'codex', name: 'Codex', requiresPty: false, capabilities: { supportsAppendSystemPrompt: false, }, }; mockAgentDetector.getAgent.mockResolvedValue(mockAgent); mockProcessManager.spawn.mockReturnValue({ pid: 12345, success: true }); const handler = handlers.get('process:spawn'); await handler!({} as any, { sessionId: 'session-1', toolType: 'codex', cwd: '/home/user/project', command: 'codex', args: [], // prompt: undefined (not provided) appendSystemPrompt: 'You are Maestro system prompt content', }); const spawnCall = mockProcessManager.spawn.mock.calls[0][0]; expect(spawnCall.prompt).toBe('You are Maestro system prompt content'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/process.test.ts` around lines 1963 - 1990, Add a new test that mirrors the existing "should use system prompt as sole content when no user prompt for unsupported agents" but omits the prompt field (i.e., leave prompt undefined) to exercise the branch where effectivePrompt is undefined; use the same mockAgent (id 'codex'), mockAgentDetector.getAgent mockResolvedValue, and mockProcessManager.spawn return, call the handlers.get('process:spawn') handler with no prompt property and appendSystemPrompt set, then assert that the spawnCall.prompt (from mockProcessManager.spawn.mock.calls[0][0]) equals the appendSystemPrompt string.
1882-1891: Consider extracting duplicatemockSshRemotefixture.This
mockSshRemoteobject duplicates the one defined at line 1158. For test maintainability, consider extracting it to a shared constant at the top of the describe block or in a test fixtures file.♻️ Suggested refactor
describe('process IPC handlers', () => { let handlers: Map<string, Function>; + // Shared SSH remote fixture for tests + const mockSshRemote = { + id: 'remote-1', + name: 'Dev Server', + host: 'dev.example.com', + port: 22, + username: 'devuser', + privateKeyPath: '~/.ssh/id_ed25519', + enabled: true, + remoteEnv: { REMOTE_VAR: 'remote-value' }, + };Then remove the duplicate declarations in
SSH remote executionandappendSystemPrompt deliverydescribe blocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/process.test.ts` around lines 1882 - 1891, Extract the duplicated mockSshRemote fixture into a single shared constant and reuse it in both tests: define a top-level const mockSshRemote = { id: 'remote-1', name: 'Dev Server', host: 'dev.example.com', port: 22, username: 'devuser', privateKeyPath: '~/.ssh/id_ed25519', enabled: true, remoteEnv: {} } inside the outer describe (or a fixtures file) and remove the duplicate inline object declarations from the "SSH remote execution" and "appendSystemPrompt delivery" describe blocks so both tests reference the same mockSshRemote constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/main/ipc/handlers/process.test.ts`:
- Around line 1963-1990: Add a new test that mirrors the existing "should use
system prompt as sole content when no user prompt for unsupported agents" but
omits the prompt field (i.e., leave prompt undefined) to exercise the branch
where effectivePrompt is undefined; use the same mockAgent (id 'codex'),
mockAgentDetector.getAgent mockResolvedValue, and mockProcessManager.spawn
return, call the handlers.get('process:spawn') handler with no prompt property
and appendSystemPrompt set, then assert that the spawnCall.prompt (from
mockProcessManager.spawn.mock.calls[0][0]) equals the appendSystemPrompt string.
- Around line 1882-1891: Extract the duplicated mockSshRemote fixture into a
single shared constant and reuse it in both tests: define a top-level const
mockSshRemote = { id: 'remote-1', name: 'Dev Server', host: 'dev.example.com',
port: 22, username: 'devuser', privateKeyPath: '~/.ssh/id_ed25519', enabled:
true, remoteEnv: {} } inside the outer describe (or a fixtures file) and remove
the duplicate inline object declarations from the "SSH remote execution" and
"appendSystemPrompt delivery" describe blocks so both tests reference the same
mockSshRemote constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5710295d-fe93-4c09-bcbd-0cf6469e6179
📒 Files selected for processing (1)
src/__tests__/main/ipc/handlers/process.test.ts
|
@greptile re-review |
…imit On Windows, CreateProcess has a ~32K char limit. Large system prompts passed via --append-system-prompt could exceed this. Use --append-system-prompt-file with a temp file for Windows local execution. SSH sessions are exempt since the command runs inside a stdin script on the remote host, not through the Windows CLI. The temp file is cleaned up 5 seconds after spawn (agent reads it at startup).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/process.ts`:
- Around line 492-499: The current guard prevents building a stream-json payload
on image-only turns because it requires effectivePrompt; update the logic in the
block around stdinInput, hasImages, and
agent?.capabilities?.supportsStreamJsonInput so that when hasImages is true and
the agent supports stream-json you always call buildStreamJsonMessage using
(effectivePrompt ?? '') as the text field (e.g.,
buildStreamJsonMessage(effectivePrompt ?? '', config.images!)), append a
trailing '\n', and still ensure sshArgs includes '--input-format',
'stream-json'; remove the effectivePrompt truthiness check so image-only turns
include images for stream-json agents.
- Around line 229-233: The Windows temp system-prompt file is keyed only by
config.sessionId causing cross-spawn races and silent unlink failures; change
creation to use a unique per-spawn filename (e.g., include process.pid +
timestamp or a UUID) when assigning systemPromptTempFile and add it to
finalArgs, ensure the cleanup routine is tied to that specific spawn (not a
shared session key) and only swallow ENOENT when unlinking while reporting any
other fs errors via the project's error-reporting utility (e.g.,
captureException/processLogger.error); update both occurrences (the write and
the unlink/cleanup logic around systemPromptTempFile) and reference the symbol
names systemPromptTempFile, config.sessionId, finalArgs so the per-spawn
lifecycle is deterministic and non-silent on real failures.
- Around line 225-245: Resolve the SSH remote before deciding how to deliver the
system prompt: move or change the logic that sets isSshSession (currently using
config.sessionSshRemoteConfig?.enabled) to use the resolved result from
getSshRemoteConfig() (or explicitly call getSshRemoteConfig() and await it
before the appendSystemPrompt branch), and if SSH was requested but
getSshRemoteConfig() fails, either fail fast with an error or treat it as a
non-local session consistently; then update the branches that build finalArgs
(the --append-system-prompt-file path using systemPromptTempFile for Windows and
the inline --append-system-prompt path) to base their choice on the resolved SSH
state so the Windows temp-file workaround is used only when truly local and not
when SSH was intended but unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dce40b82-d49a-44cb-b9e3-66b84c25dce5
📒 Files selected for processing (2)
src/__tests__/main/ipc/handlers/process.test.tssrc/main/ipc/handlers/process.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/main/ipc/handlers/process.test.ts
|
Good to go |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/main/ipc/handlers/process.ts (3)
623-631:⚠️ Potential issue | 🟠 MajorSurface temp-file cleanup failures.
Line 625 swallows every
unlinkSyncerror. Any non-ENOENTfailure leaves a prompt-bearing temp file behind with zero signal. Ignore onlyENOENT, and report the rest.🔧 Minimal fix
-import { addBreadcrumb } from '../../utils/sentry'; +import { addBreadcrumb, captureException } from '../../utils/sentry'; ... try { fs.unlinkSync(systemPromptTempFile!); - } catch { - // Ignore - temp dir cleanup will handle it + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + captureException(error, { + operation: 'system-prompt-tempfile-cleanup', + sessionId: config.sessionId, + tempFile: systemPromptTempFile, + }); + } }As per coding guidelines "Do not silently swallow errors with try-catch blocks that only log" and "Use Sentry utilities for explicit reporting".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/process.ts` around lines 623 - 631, The temp-file cleanup currently swallows all unlinkSync errors; update the setTimeout cleanup block that references systemPromptTempFile and fs.unlinkSync to only ignore ENOENT, and for any other error capture/report it (e.g., call Sentry.captureException(err) and/or processLogger.error with context about systemPromptTempFile) so failures are surfaced instead of silently swallowed.
498-503:⚠️ Potential issue | 🟠 MajorDon't require text to send stream-json images over SSH.
Line 500 still drops image-only turns for stream-json agents. With
appendSystemPromptseparated,effectivePromptcan be empty, this block is skipped, and the file-based image path is disabled for these agents, so the remote spawn sends no images.🔧 Minimal fix
- if (hasImages && effectivePrompt && agent?.capabilities?.supportsStreamJsonInput) { + if (hasImages && agent?.capabilities?.supportsStreamJsonInput) { // Stream-json agent (Claude Code): embed images in the stdin message - stdinInput = buildStreamJsonMessage(effectivePrompt, config.images!) + '\n'; + stdinInput = buildStreamJsonMessage(effectivePrompt ?? '', config.images!) + '\n';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/process.ts` around lines 498 - 503, The current branch skips sending stream-json image payloads when effectivePrompt is empty; change the condition so that when hasImages and agent?.capabilities?.supportsStreamJsonInput are true you always construct a stream-json stdin message (use effectivePrompt || '' when calling buildStreamJsonMessage) and set stdinInput to that + '\n', rather than requiring effectivePrompt to be truthy; keep the existing check that adds '--input-format' to sshArgs unchanged and still reference config.images when building the message (symbols: effectivePrompt, hasImages, agent?.capabilities?.supportsStreamJsonInput, stdinInput, buildStreamJsonMessage, sshArgs, config.images).
225-248:⚠️ Potential issue | 🟠 MajorResolve SSH before choosing the Windows system-prompt delivery path.
Line 225 uses
sessionSshRemoteConfig.enabledas a proxy for “this spawn will be remote”. IfgetSshRemoteConfig()later returns no config, this handler falls back to a local Windows spawn after already choosing inline--append-system-prompt, so the temp-file workaround and the local shell path are both skipped. Please branch on the resolved remote, or fail fast when SSH was requested but unavailable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/process.ts` around lines 225 - 248, The code currently uses config.sessionSshRemoteConfig?.enabled to decide Windows vs inline system-prompt delivery before the actual SSH resolution; change that to branch on the resolved SSH config (the value returned by getSshRemoteConfig or the resolvedRemote variable used later) instead of the raw config flag, and if SSH was requested but getSshRemoteConfig() returns null/undefined, fail fast (throw/return an error) so you never take the local-Windows temp-file path incorrectly; update the logic around isSshSession, systemPromptTempFile and finalArgs (the --append-system-prompt-file vs --append-system-prompt branch) to use the resolved SSH boolean or error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/ipc/handlers/process.ts`:
- Around line 623-631: The temp-file cleanup currently swallows all unlinkSync
errors; update the setTimeout cleanup block that references systemPromptTempFile
and fs.unlinkSync to only ignore ENOENT, and for any other error capture/report
it (e.g., call Sentry.captureException(err) and/or processLogger.error with
context about systemPromptTempFile) so failures are surfaced instead of silently
swallowed.
- Around line 498-503: The current branch skips sending stream-json image
payloads when effectivePrompt is empty; change the condition so that when
hasImages and agent?.capabilities?.supportsStreamJsonInput are true you always
construct a stream-json stdin message (use effectivePrompt || '' when calling
buildStreamJsonMessage) and set stdinInput to that + '\n', rather than requiring
effectivePrompt to be truthy; keep the existing check that adds '--input-format'
to sshArgs unchanged and still reference config.images when building the message
(symbols: effectivePrompt, hasImages,
agent?.capabilities?.supportsStreamJsonInput, stdinInput,
buildStreamJsonMessage, sshArgs, config.images).
- Around line 225-248: The code currently uses
config.sessionSshRemoteConfig?.enabled to decide Windows vs inline system-prompt
delivery before the actual SSH resolution; change that to branch on the resolved
SSH config (the value returned by getSshRemoteConfig or the resolvedRemote
variable used later) instead of the raw config flag, and if SSH was requested
but getSshRemoteConfig() returns null/undefined, fail fast (throw/return an
error) so you never take the local-Windows temp-file path incorrectly; update
the logic around isSshSession, systemPromptTempFile and finalArgs (the
--append-system-prompt-file vs --append-system-prompt branch) to use the
resolved SSH boolean or error path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44775e96-dd0e-4934-a5e7-18617dd73683
📒 Files selected for processing (2)
src/__tests__/main/ipc/handlers/process.test.tssrc/main/ipc/handlers/process.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/main/ipc/handlers/process.test.ts
Code Review — PR #596@jSydorowicz21 — solid PR overall. Clean separation of system prompt from user message, good test coverage, and sensible Windows temp-file workaround. Three issues worth addressing: 🔴 Critical:
|
- Pass sendPromptViaStdin/sendPromptViaStdinRaw to both spawn calls in
agentStore processQueuedItem (message and command branches)
- Add sendPromptViaStdin/sendPromptViaStdinRaw to ProcessConfig interface
in preload/process.ts for type safety
- Replace bare catch {} in temp file cleanup with ENOENT-only ignore
and captureException for unexpected errors
- cue-executor: capture sshStdinScript from wrapSpawnWithSsh and write it to stdin for SSH large-prompt mode (was silently dropping prompts) - global.d.ts: add supportsAppendSystemPrompt to both AgentCapabilities interfaces for type safety through the preload bridge - process.ts: move temp file cleanup to creation site with 30s safety net so the file is removed even if spawn throws
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/ipc/handlers/process.ts (2)
516-520: Consider future-proofing the image-only turn handling.The condition at line 518 requires
effectivePromptto be truthy. If an image-only turn arrives with an empty prompt and no system prompt, the stream-json payload won't be built for stream-json agents.As noted in the review discussion, this is currently safe because
DEFAULT_IMAGE_ONLY_PROMPTis always set by the renderer. However, a defensive check usingeffectivePrompt ?? ''would make this more robust against future changes.♻️ Optional defensive improvement
-if (hasImages && effectivePrompt && agent?.capabilities?.supportsStreamJsonInput) { +if (hasImages && agent?.capabilities?.supportsStreamJsonInput) { // Stream-json agent (Claude Code): embed images in the stdin message - stdinInput = buildStreamJsonMessage(effectivePrompt, config.images!) + '\n'; + stdinInput = buildStreamJsonMessage(effectivePrompt ?? '', config.images!) + '\n';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/process.ts` around lines 516 - 520, The current branch that builds the stream-json payload short-circuits when effectivePrompt is falsy, so for image-only turns the payload isn't created for stream-json agents; update the logic around effectivePrompt/hasImages/agent?.capabilities?.supportsStreamJsonInput in the handler (references: effectivePrompt, hasImages, agent?.capabilities?.supportsStreamJsonInput, buildStreamJsonMessage, stdinInput) to use a defensive default (e.g. pass effectivePrompt ?? '' into buildStreamJsonMessage and set stdinInput from effectivePrompt ?? '') so image-only turns still produce a stream-json stdin message.
244-250: AddsessionIdto the Sentry exception context.Per the reviewer's recommendation in PR objectives, including
sessionIdin thecaptureExceptioncontext would improve debuggability for problematic temp file cleanup failures.♻️ Suggested improvement
captureException( cleanupErr instanceof Error ? cleanupErr : new Error(String(cleanupErr)), { context: 'systemPromptTempFile cleanup (safety)', file: tempFileToClean, + sessionId: config.sessionId, } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/process.ts` around lines 244 - 250, The Sentry capture call for temp file cleanup (captureException) is missing the sessionId in its context; update the context object passed to captureException (in the process.ts handler where cleanupErr and tempFileToClean are handled) to also include sessionId so the payload contains { context: 'systemPromptTempFile cleanup (safety)', file: tempFileToClean, sessionId: sessionId } (use the existing variable that holds the current session id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/ipc/handlers/process.ts`:
- Around line 516-520: The current branch that builds the stream-json payload
short-circuits when effectivePrompt is falsy, so for image-only turns the
payload isn't created for stream-json agents; update the logic around
effectivePrompt/hasImages/agent?.capabilities?.supportsStreamJsonInput in the
handler (references: effectivePrompt, hasImages,
agent?.capabilities?.supportsStreamJsonInput, buildStreamJsonMessage,
stdinInput) to use a defensive default (e.g. pass effectivePrompt ?? '' into
buildStreamJsonMessage and set stdinInput from effectivePrompt ?? '') so
image-only turns still produce a stream-json stdin message.
- Around line 244-250: The Sentry capture call for temp file cleanup
(captureException) is missing the sessionId in its context; update the context
object passed to captureException (in the process.ts handler where cleanupErr
and tempFileToClean are handled) to also include sessionId so the payload
contains { context: 'systemPromptTempFile cleanup (safety)', file:
tempFileToClean, sessionId: sessionId } (use the existing variable that holds
the current session id).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35a7b46d-d9eb-453f-b325-1ff9854ad630
📒 Files selected for processing (1)
src/main/ipc/handlers/process.ts
|
@greptile give this a re-review |
| } | ||
| } | ||
| }, 30_000); | ||
| finalArgs = [...finalArgs, '--append-system-prompt-file', systemPromptTempFile]; |
There was a problem hiding this comment.
--append-system-prompt-file is not a documented Claude Code flag
The Windows code path passes --append-system-prompt-file <tempfilepath> to Claude Code, but this flag is not documented anywhere in the codebase and does not appear to be a real Claude Code CLI flag. The capability comment (supportsAppendSystemPrompt: true, // --append-system-prompt flag) only references --append-system-prompt.
If Claude Code does not support --append-system-prompt-file, it will either:
- Fail to start with an "unknown option" error (breaking all Windows Claude Code sessions), or
- Silently ignore the flag (delivering no system prompt to the agent)
The unit test at process.test.ts:1922 only checks that the arg is present in the spawned args array — it does not actually invoke the Claude Code binary to verify the flag is accepted.
Before merging, please confirm that --append-system-prompt-file is a supported Claude Code CLI flag (e.g., by checking claude --help output or the Claude Code changelog). If it is not supported, consider alternatives such as sending the system prompt via a prepended stdin message (before the user prompt), similar to how the SSH path inlines it.
| const appendPromptIdx = finalArgs.indexOf('--append-system-prompt'); | ||
| const argsToLog = | ||
| appendPromptIdx !== -1 | ||
| ? [ | ||
| ...finalArgs.slice(0, appendPromptIdx + 1), | ||
| `<${finalArgs[appendPromptIdx + 1]?.length ?? 0} chars>`, | ||
| ...finalArgs.slice(appendPromptIdx + 2), | ||
| ] | ||
| : finalArgs; |
There was a problem hiding this comment.
--append-system-prompt-file args
finalArgs.indexOf('--append-system-prompt') uses exact element matching on the array. On Windows, the args contain '--append-system-prompt-file' (a distinct string), so appendPromptIdx will always be -1 and argsToLog will equal finalArgs — including the temp file path.
The system prompt content itself is not in the args (it's in the file), so there is no sensitive data being logged. However, it may be worth extending the check to also redact the file path on Windows for consistency:
const appendPromptIdx =
finalArgs.indexOf('--append-system-prompt') !== -1
? finalArgs.indexOf('--append-system-prompt')
: finalArgs.indexOf('--append-system-prompt-file');
|
@pedramamini good to go, two remaining greptile feedbacks are confirm the flag exists (it does) and potentially redact the path but I believe that's important info |
2 participants
Summary
--append-system-promptflag (proper system prompt) instead of embedding in user messageChanges
appendSystemPromptfield to ProcessConfig (renderer types, preload, global.d.ts)supportsAppendSystemPromptcapability flag to AgentCapabilities (true for Claude Code, false for others)process:spawnhandler: CLI arg for supported agents, embed fallback for othershasImagesparam togetStdinFlagsfor conditional stream-json stdin on WindowsTest plan
--append-system-promptappears in spawn args, user message is cleannpm run lint)Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation