diff --git a/crates/ide-agent/src/config.rs b/crates/ide-agent/src/config.rs index af9882db0..ee9b45bfd 100644 --- a/crates/ide-agent/src/config.rs +++ b/crates/ide-agent/src/config.rs @@ -39,19 +39,31 @@ pub(crate) fn add_client_options(provider: &str, config: &mut Vec bool { - let selected = |category: &str| { - config.iter().find(|o| o.category.as_deref() == Some(category)).and_then(|o| o.current.as_deref()) - }; - selected("mode").is_some_and(is_bypass_mode) - || selected("permissions").is_some_and(|v| matches!(v.to_ascii_lowercase().as_str(), "on" | "true")) - || selected("approval").is_some_and(|v| v.eq_ignore_ascii_case("bypass")) + ["mode", "permissions", "approval"].iter().any(|category| { + config + .iter() + .find(|o| o.category.as_deref() == Some(category)) + .and_then(|o| o.current.as_deref()) + .is_some_and(|v| is_bypass(Some(category), v)) + }) } -/// The mode that approves everything without asking (`bypassPermissions`, `yolo`, `full-access`…), if the agent has one. -pub(crate) fn bypass_mode(options: &[AgentConfigOption]) -> Option<(&AgentConfigOption, &AgentConfigValue)> { - let option = options.iter().find(|o| o.category.as_deref() == Some("mode"))?; - let value = option.values.iter().find(|v| is_bypass_mode(&v.value))?; - Some((option, value)) +fn is_bypass(category: Option<&str>, value: &str) -> bool { + match category { + Some("mode") => is_bypass_mode(value), + Some("permissions") => matches!(value.to_ascii_lowercase().as_str(), "on" | "true"), + Some("approval") => value.eq_ignore_ascii_case("bypass"), + _ => false, + } +} + +/// Provider-wide offers are not user choices; a session's bypass belongs only to that session. +pub(crate) fn clear_cached_bypass(options: &mut [AgentConfigOption]) { + for option in options { + if option.current.as_deref().is_some_and(|v| is_bypass(option.category.as_deref(), v)) { + option.current = None; + } + } } fn is_bypass_mode(mode: &str) -> bool { @@ -295,7 +307,6 @@ mod tests { ]} })); assert!(bypass_approvals(&full_access)); - assert_eq!(bypass_mode(&full_access).map(|(_, v)| v.value.as_str()), Some("full-access")); let asking = options_of(&json!({ "modes": {"currentModeId": "ask", "availableModes": [ @@ -303,11 +314,13 @@ mod tests { ]} })); assert!(!bypass_approvals(&asking)); - assert_eq!( - bypass_mode(&asking).map(|(_, v)| v.value.as_str()), - Some("full-access"), - "the offered bypass mode stays available" - ); + let mut cached = full_access.clone(); + clear_cached_bypass(&mut cached); + assert_eq!(cached[0].current, None); + assert_eq!(cached[0].values, full_access[0].values, "bypass stays available as an explicit choice"); + let mut cached = asking.clone(); + clear_cached_bypass(&mut cached); + assert_eq!(cached, asking); let permissions = |current: &str| { options_of(&json!({"configOptions": [{ diff --git a/crates/ide-agent/src/lib.rs b/crates/ide-agent/src/lib.rs index 73e311b20..fe1f9dbb9 100644 --- a/crates/ide-agent/src/lib.rs +++ b/crates/ide-agent/src/lib.rs @@ -136,14 +136,7 @@ impl Shared { // Model-specific options are offered separately, never as provider-wide defaults. config.retain(|o| o.category.as_deref() != Some("thought_level")); config::add_client_options(provider, &mut config); - if let Some((o, v)) = config::bypass_mode(&config).map(|(o, v)| (o.id.clone(), v.value.clone())) - && let Some(o) = config.iter_mut().find(|c| c.id == o) - { - o.current = Some(v); - } - if let Some(o) = config.iter_mut().find(|o| o.id == config::CURSOR_APPROVAL) { - o.current = Some("bypass".into()); - } + config::clear_cached_bypass(&mut config); config } @@ -231,8 +224,10 @@ impl Shared { changed = true; } } + let mut c = c.to_vec(); + config::clear_cached_bypass(&mut c); if !c.is_empty() && known.config != c { - known.config = c.to_vec(); + known.config = c; changed = true; } } @@ -361,8 +356,11 @@ impl AgentManager { } fn with_connector(paths: AgentPaths, runtime: tokio::runtime::Handle, connector: Connector) -> Self { - let known = + let mut known: HashMap = std::fs::read(&paths.models_file).ok().and_then(|b| serde_json::from_slice(&b).ok()).unwrap_or_default(); + for offered in known.values_mut() { + config::clear_cached_bypass(&mut offered.config); + } let providers = RwLock::new(provider::load(paths.custom_agents_dir.as_deref())); let shared = Arc::new(Shared { paths, diff --git a/crates/ide-agent/src/tests.rs b/crates/ide-agent/src/tests.rs index e38114b23..668a462e8 100644 --- a/crates/ide-agent/src/tests.rs +++ b/crates/ide-agent/src/tests.rs @@ -1714,6 +1714,109 @@ fn usage_limits_and_cost_skip_what_is_malformed() { assert_eq!(usage.map(|u| u.cost), Some(None), "a negative cost is dropped, the context use kept"); } +#[tokio::test(flavor = "multi_thread")] +async fn cached_bypass_is_dropped_on_load_read_and_report_without_changing_task_choices() { + for (provider, category, bypass, safe) in [ + ("claude", "mode", "bypassPermissions", "default"), + ("codex", "mode", "full-access", "auto"), + ("cursor", "approval", "bypass", "ask"), + ("copilot", "permissions", "on", "off"), + ("custom", "mode", "https://example.com/modes#yolo", "ask"), + ] { + let tmp = tempfile::tempdir().unwrap(); + let agent = Arc::new(FakeAgent::default()); + let options = config::options_of(&json!({"configOptions": [{ + "id": category, "category": category, "currentValue": bypass, + "options": [{"value": safe}, {"value": bypass}] + }]})); + let models = tmp.path().join("models.json"); + std::fs::write(&models, serde_json::to_vec(&json!({provider: {"config": options}})).unwrap()).unwrap(); + let mgr = manager(tmp.path(), agent); + assert_eq!(mgr.shared.known.lock()[provider].config[0].current, None, "load: {provider}"); + // Also defend reads of in-memory data, without changing its offered values. + mgr.shared.known.lock().get_mut(provider).unwrap().config = options.clone(); + let read = mgr.shared.known_config(provider); + assert_eq!(read[0].current, None, "read: {provider}"); + assert_eq!(read[0].values, options[0].values); + mgr.shared.remember(provider, Some(&options), None, None); + assert_eq!(mgr.shared.known.lock()[provider].config[0].current, None, "report: {provider}"); + let disk: HashMap = serde_json::from_slice(&std::fs::read(models).unwrap()).unwrap(); + assert_eq!(disk[provider].config[0].current, None); + let mut safe_options = options.clone(); + safe_options[0].current = Some(safe.into()); + mgr.shared.remember(provider, Some(&safe_options), None, None); + assert_eq!(mgr.shared.known_config(provider)[0].current.as_deref(), Some(safe)); + assert_eq!(options[0].current.as_deref(), Some(bypass), "the live report stays intact"); + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn new_tasks_use_reported_modes_and_explicit_bypass_survives_restart() { + for (provider, safe, bypass) in [ + ("claude", "default", "bypassPermissions"), + ("codex", "auto", "full-access"), + ("cursor", "agent", "yolo"), + ("copilot", "interactive", "yolo"), + ("claude-acp", "default", "bypassPermissions"), + ] { + let tmp = tempfile::tempdir().unwrap(); + let agent = Arc::new(FakeAgent::default()); + *agent.extra_options.lock() = vec![json!({ + "id": "mode", "category": "mode", "currentValue": safe, + "options": [{"value": safe}, {"value": bypass}] + })]; + let mgr = manager(tmp.path(), agent.clone()); + let new = |config| NewAgentTask { + project_root: None, + prompt: text("again"), + provider: provider.into(), + config, + isolation: Isolation::Local, + base: None, + }; + let fresh = mgr.create(new(vec![])).await.unwrap(); + wait_for(&mgr, &fresh.id, AgentStatus::Ready).await; + let mode = |mgr: &AgentManager, id: &str| { + mgr.task(id).unwrap().state.lock().task.config.iter().find(|o| o.id == "mode").unwrap().current.clone() + }; + assert_eq!(mode(&mgr, &fresh.id).as_deref(), Some(safe), "{provider}"); + assert!(!config::bypass_approvals(&mgr.task(&fresh.id).unwrap().state.lock().task.config)); + if provider == "cursor" { + assert_eq!( + mgr.task(&fresh.id) + .unwrap() + .state + .lock() + .task + .config + .iter() + .find(|o| o.id == config::CURSOR_APPROVAL) + .unwrap() + .current + .as_deref(), + Some("ask") + ); + } + assert!(!agent.calls.lock().iter().any(|c| c.starts_with("session/set_"))); + let explicit = + mgr.create(new(vec![AgentConfigChoice { id: "mode".into(), value: bypass.into() }])).await.unwrap(); + wait_for(&mgr, &explicit.id, AgentStatus::Ready).await; + assert_eq!(mode(&mgr, &explicit.id).as_deref(), Some(bypass)); + assert!(!config::bypass_approvals(&mgr.shared.known_config(provider))); + mgr.shutdown(); + let restored = manager(tmp.path(), agent.clone()); + assert_eq!(mode(&restored, &explicit.id).as_deref(), Some(bypass)); + restored.prompt(&explicit.id, text("again"), false).await.unwrap(); + wait_for(&restored, &explicit.id, AgentStatus::Ready).await; + assert_eq!(mode(&restored, &explicit.id).as_deref(), Some(bypass)); + assert_eq!(mode(&restored, &fresh.id).as_deref(), Some(safe)); + let next = restored.create(new(vec![])).await.unwrap(); + wait_for(&restored, &next.id, AgentStatus::Ready).await; + assert_eq!(mode(&restored, &next.id).as_deref(), Some(safe)); + restored.shutdown(); + } +} + #[tokio::test(flavor = "multi_thread")] async fn cursor_approval_switch_resolves_pending_and_survives_restart() { let tmp = tempfile::tempdir().unwrap(); @@ -1746,13 +1849,13 @@ async fn cursor_approval_switch_resolves_pending_and_survives_restart() { mgr.prompt(&task.id, text("Fix again"), false).await.unwrap(); wait_for(&mgr, &task.id, AgentStatus::Ready).await; assert!(!agent.calls.lock().iter().any(|c| c.contains("bypass"))); - assert!(config::bypass_approvals(&mgr.shared.known_config("cursor"))); + assert!(!config::bypass_approvals(&mgr.shared.known_config("cursor"))); mgr.set_config(&task.id, config::CURSOR_APPROVAL, "ask".into()).await.unwrap(); mgr.prompt(&task.id, text("Fix once more"), false).await.unwrap(); wait_for(&mgr, &task.id, AgentStatus::NeedsInput).await; mgr.cancel(&task.id, None).unwrap(); wait_for(&mgr, &task.id, AgentStatus::Ready).await; - // A task created without choosing approvals still asks; the New Task page sends the default. + // A task created without choosing approvals still asks. let other = mgr.create(new()).await.unwrap(); wait_for(&mgr, &other.id, AgentStatus::NeedsInput).await; mgr.cancel(&other.id, None).unwrap(); diff --git a/e2e/agentApproval.spec.ts b/e2e/agentApproval.spec.ts index a742cf887..0ab43a684 100644 --- a/e2e/agentApproval.spec.ts +++ b/e2e/agentApproval.spec.ts @@ -148,3 +148,66 @@ for (const theme of ["dark", "light"] as const) { }); } } + +test("new tasks keep the displayed approval mode unless the user chooses Bypass", async ({ page }) => { + await page.goto("/"); + await page.getByRole("tree", { name: "Project" }).waitFor(); + await page.evaluate(async () => { + const path = "/src/features/agents/store.ts"; + const store = await import(path); + const ipcPath = "/src/platform/ipc.ts"; + const { commands } = await import(ipcPath); + commands.agentCreate = async (request: unknown) => { + (window as unknown as { taskRequest: unknown }).taskRequest = request; + return { status: "error", error: { kind: "invalid", message: "Verification only" } }; + }; + store.useAgents.setState({ + loaded: true, + providers: [ + { + id: "claude", + name: "Claude Code", + installed: true, + source: { kind: "builtin" }, + commands: [], + config: [ + { + id: "mode", + name: "Mode", + category: "mode", + current: "default", + via: "option", + values: [ + { value: "default", name: "Default" }, + { value: "bypassPermissions", name: "Bypass permissions" }, + ], + }, + ], + }, + ], + draft: store.savedDraft({}), + }); + store.openHome(); + }); + await expect(page.getByRole("button", { name: "Mode", exact: true })).toHaveText("Default"); + await page.getByRole("textbox").fill("Create a note"); + await page.getByRole("button", { name: "Start", exact: true }).click(); + await expect + .poll(() => page.evaluate(() => (window as unknown as { taskRequest: unknown }).taskRequest)) + .toMatchObject({ + provider: "claude", + config: [{ id: "mode", value: "default" }], + }); + await page.getByRole("button", { name: "Mode", exact: true }).click(); + await page.getByRole("option", { name: "Bypass permissions", exact: true }).click(); + await expect(page.getByRole("button", { name: "Mode", exact: true })).toHaveText("Bypass permissions"); + await page.getByRole("button", { name: "Start", exact: true }).click(); + await expect + .poll(() => page.evaluate(() => (window as unknown as { taskRequest: unknown }).taskRequest)) + .toMatchObject({ + config: [{ id: "mode", value: "bypassPermissions" }], + }); + expect(await page.evaluate(() => JSON.parse(localStorage.getItem("openidea.agents.draft.v2")!).config)).toEqual({ + mode: "bypassPermissions", + }); +}); diff --git a/src/features/agents/agents.test.ts b/src/features/agents/agents.test.ts index bba042b28..3603e582a 100644 --- a/src/features/agents/agents.test.ts +++ b/src/features/agents/agents.test.ts @@ -3,7 +3,6 @@ import type { AgentConfigOption, AgentEntry, AgentStatus, AgentTask } from "@/bi import { applyCompletion, atEdgeLine, - bypassDefaults, canSend, completionAt, isPathQuery, @@ -203,7 +202,8 @@ describe("agent composer", () => { "https://agentclientprotocol.com/protocol/session-modes#autopilot", ].map(modeIcon), ).toEqual(["lock", "edit", "todo", "run", "unlock", "edit", "eye", "lock", "run", "run"]); - expect(bypassDefaults([...config, permissions])).toEqual({ allow_all: "on" }); + expect(permissions.current).toBe("off"); + expect(approvals.current).toBe("ask"); expect(usageLabel({ used: 24_000, size: 200_000 })).toEqual({ short: "12%", detail: "24k / 200k tokens", diff --git a/src/features/agents/composerCommands.test.tsx b/src/features/agents/composerCommands.test.tsx index 9e3113835..9c3d884ad 100644 --- a/src/features/agents/composerCommands.test.tsx +++ b/src/features/agents/composerCommands.test.tsx @@ -33,7 +33,7 @@ import { defaultTaskProject } from "./project"; import { HomeEditor } from "./HomeEditor"; import composerStyles from "./Composer.module.css"; import tagStyles from "@/ui/InlineTag.module.css"; -import { HOME, updateComposer, useAgents } from "./store"; +import { HOME, savedDraft, startTask, updateComposer, useAgents } from "./store"; import { emptyDraft } from "./composerModel"; const command = (name: string, description: string, source: "command" | "skill" = "command"): AgentCommand => ({ @@ -264,3 +264,46 @@ describe("workspace task project", () => { await vi.dynamicImportSettled(); }); }); + +describe("new task permissions", () => { + it.each([ + ["claude", "mode", "default", "bypassPermissions"], + ["codex", "mode", "auto", "full-access"], + ["cursor", "approval", "ask", "bypass"], + ["copilot", "permissions", "off", "on"], + ["custom", "mode", "ask", "yolo"], + ])("keeps %s's offered default and sends an explicit Bypass choice", async (provider, category, safe, bypass) => { + const offered = { + ...claude, + id: provider, + config: [ + { + id: category, + name: category, + description: null, + category, + current: safe, + via: "option" as const, + values: [safe, bypass].map((value) => ({ value, name: value, description: null })), + }, + ], + }; + const draft = savedDraft({ provider }); + useAgents.setState({ providers: [offered], draft }); + updateComposer(HOME, { text: "Create a note" }); + await expect(startTask()).rejects.toThrow("stop here"); + expect(agentCreate).toHaveBeenLastCalledWith(expect.objectContaining({ config: [{ id: category, value: safe }] })); + useAgents.setState({ providers: [{ ...offered, config: [{ ...offered.config[0]!, current: null }] }] }); + await expect(startTask()).rejects.toThrow("stop here"); + expect(agentCreate).toHaveBeenLastCalledWith(expect.objectContaining({ config: [] })); + // Reloading the saved draft preserves the user's explicit permission choice. + useAgents.setState({ draft: savedDraft(JSON.parse(JSON.stringify({ ...draft, config: { [category]: bypass } }))) }); + await expect(startTask("worktree")).rejects.toThrow("stop here"); + expect(agentCreate).toHaveBeenLastCalledWith( + expect.objectContaining({ + isolation: "worktree", + config: [{ id: category, value: bypass }], + }), + ); + }); +}); diff --git a/src/features/agents/composerModel.ts b/src/features/agents/composerModel.ts index c9b9caf8e..0b07e1d96 100644 --- a/src/features/agents/composerModel.ts +++ b/src/features/agents/composerModel.ts @@ -198,20 +198,6 @@ export function modeIcon(mode: string): string { return "lock"; } -/** The settings that approve everything without asking (a bypass mode, or approvals), by option id. */ -export function bypassDefaults(config: readonly AgentConfigOption[]): Record { - const out: Record = {}; - const mode = modeOption(config); - const unlock = mode?.values.find((v) => modeIcon(v.value) === "unlock"); - if (mode && unlock) out[mode.id] = unlock.value; - const permissions = config.find((o) => o.category === "permissions"); - const allowAll = permissions?.values.find((v) => /^(on|true)$/i.test(v.value)); - if (permissions && allowAll) out[permissions.id] = allowAll.value; - const approval = config.find((o) => o.category === "approval"); - if (approval?.values.some((v) => v.value === "bypass")) out[approval.id] = "bypass"; - return out; -} - /** Shift+Tab: the mode after the current one. */ export function nextValue(option: AgentConfigOption, current = option.current): string | null { if (!option.values.length) return null; diff --git a/src/features/agents/store.ts b/src/features/agents/store.ts index 934e106e6..8ae912e57 100644 --- a/src/features/agents/store.ts +++ b/src/features/agents/store.ts @@ -20,7 +20,7 @@ import { call, commands, isTauri } from "@/platform/ipc"; import { editorService, notifications, notifyTabTitlesChanged, toolWindowService } from "@/workbench/contributions"; import { tx } from "@/platform/i18n"; import { confirm } from "@/workbench/messages"; -import { bypassDefaults, canSend, emptyDraft, toPrompt, type ComposerDraft, type FileRef } from "./composerModel"; +import { canSend, emptyDraft, toPrompt, type ComposerDraft, type FileRef } from "./composerModel"; import { withQuote } from "./conversation"; import { HOME_KEY, @@ -538,12 +538,8 @@ export async function startTask( providers.find((p) => p.id === draft.provider), draft, ); - const defaults = bypassDefaults(options); const config = options.flatMap((o) => { - const value = - o.category === "model" || o.category === "thought_level" - ? o.current - : (draft.config[o.id] ?? defaults[o.id] ?? null); + const value = o.current; return value !== null && o.values.some((v) => v.value === value) ? [{ id: o.id, value }] : []; }); const task = await call(