-
Notifications
You must be signed in to change notification settings - Fork 246
fix: enforce read-only mode for JSON line agents and pass customModel in CLI #563
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
base: 0.16.0-RC
Are you sure you want to change the base?
Changes from 2 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 |
|---|---|---|
|
|
@@ -22,9 +22,11 @@ const CLAUDE_ARGS = [ | |
| '--verbose', | ||
| '--output-format', | ||
| 'stream-json', | ||
| '--dangerously-skip-permissions', | ||
| ]; | ||
|
|
||
| // Permission bypass arg for Claude — skipped in read-only mode | ||
| const CLAUDE_YOLO_ARGS = ['--dangerously-skip-permissions']; | ||
|
|
||
| // Cached paths per agent type (resolved once at startup) | ||
| const cachedPaths: Map<string, string> = new Map(); | ||
|
|
||
|
|
@@ -192,6 +194,9 @@ async function spawnClaudeAgent( | |
| if (def?.readOnlyEnvOverrides) { | ||
| Object.assign(env, def.readOnlyEnvOverrides); | ||
| } | ||
| } else { | ||
| // Only bypass permissions in non-read-only mode | ||
| args.push(...CLAUDE_YOLO_ARGS); | ||
| } | ||
|
|
||
| if (agentSessionId) { | ||
|
|
@@ -355,7 +360,8 @@ async function spawnJsonLineAgent( | |
| cwd: string, | ||
| prompt: string, | ||
| agentSessionId?: string, | ||
| _readOnlyMode?: boolean | ||
| readOnlyMode?: boolean, | ||
| customModel?: string | ||
| ): Promise<AgentResult> { | ||
| return new Promise((resolve) => { | ||
| const env = buildExpandedEnv(); | ||
|
|
@@ -368,11 +374,29 @@ async function spawnJsonLineAgent( | |
| } | ||
| } | ||
|
|
||
| // Apply read-only mode env overrides from agent definition | ||
| if (readOnlyMode && def?.readOnlyEnvOverrides) { | ||
| Object.assign(env, def.readOnlyEnvOverrides); | ||
| } | ||
|
|
||
| // Build args from agent definition | ||
| const args: string[] = []; | ||
| if (def?.batchModePrefix) args.push(...def.batchModePrefix); | ||
| if (def?.batchModeArgs) args.push(...def.batchModeArgs); | ||
|
|
||
| // In read-only mode, filter out YOLO/bypass args from batchModeArgs | ||
| // (they override read-only flags). In normal mode, apply all batchModeArgs. | ||
| if (def?.batchModeArgs) { | ||
| if (readOnlyMode && def.yoloModeArgs?.length) { | ||
| const yoloSet = new Set(def.yoloModeArgs); | ||
| args.push(...def.batchModeArgs.filter((a) => !yoloSet.has(a))); | ||
| } else { | ||
| args.push(...def.batchModeArgs); | ||
| } | ||
| } | ||
|
Comment on lines
+386
to
+397
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. Gemini CLI may hang in read-only batch mode after For the Gemini CLI agent, if (readOnlyMode && def.yoloModeArgs?.length) {
const yoloSet = new Set(def.yoloModeArgs);
args.push(...def.batchModeArgs.filter((a) => !yoloSet.has(a)));
}
OpenCode faced the same problem and was fixed by keeping One option is to add a Gemini-specific |
||
|
|
||
| if (def?.jsonOutputArgs) args.push(...def.jsonOutputArgs); | ||
| if (readOnlyMode && def?.readOnlyArgs) args.push(...def.readOnlyArgs); | ||
| if (customModel && def?.modelArgs) args.push(...def.modelArgs(customModel)); | ||
|
|
||
| if (agentSessionId && def?.resumeArgs) { | ||
| args.push(...def.resumeArgs(agentSessionId)); | ||
|
|
@@ -477,6 +501,8 @@ export interface SpawnAgentOptions { | |
| agentSessionId?: string; | ||
| /** Run in read-only/plan mode (uses centralized agent definitions for provider-specific flags) */ | ||
| readOnlyMode?: boolean; | ||
| /** Custom model ID from agent config (e.g., 'github-copilot/gpt-5-mini') */ | ||
| customModel?: string; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -490,13 +516,14 @@ export async function spawnAgent( | |
| options?: SpawnAgentOptions | ||
| ): Promise<AgentResult> { | ||
| const readOnly = options?.readOnlyMode; | ||
| const customModel = options?.customModel; | ||
|
|
||
| if (toolType === 'claude-code') { | ||
| return spawnClaudeAgent(cwd, prompt, agentSessionId, readOnly); | ||
| } | ||
|
|
||
| if (hasCapability(toolType, 'usesJsonLineOutput')) { | ||
| return spawnJsonLineAgent(toolType, cwd, prompt, agentSessionId); | ||
| return spawnJsonLineAgent(toolType, cwd, prompt, agentSessionId, readOnly, customModel); | ||
| } | ||
|
|
||
| return { | ||
|
|
||
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.
Existing test asserts YOLO flag is still present in read-only mode
The removal of
--dangerously-skip-permissionsfromCLAUDE_ARGSand its relegation toCLAUDE_YOLO_ARGS(only added in non-read-only mode) is the correct core change of this PR. However, the existing test atsrc/__tests__/cli/services/agent-spawner.test.ts:1179explicitly asserts that--dangerously-skip-permissionsis still present whenreadOnlyMode: true:This assertion was written against the old behaviour where
--dangerously-skip-permissionslived inCLAUDE_ARGSand was therefore always included. After this PR it will no longer be present in read-only mode, so the test will fail. The assertion should be inverted:Additionally, a new assertion checking it is present in normal (non-read-only) mode would close the gap in coverage.