From 1f38c61c75918e19068d3a14f8a9c9be9979608d Mon Sep 17 00:00:00 2001 From: Blomios Date: Thu, 10 Sep 2026 19:04:01 +0200 Subject: [PATCH] =?UTF-8?q?fix(application):=20writable=20roots=20Codex=20?= =?UTF-8?q?corrects=20sur=20projets=20non-IdeA=20quel=20que=20soit=20le=20?= =?UTF-8?q?mode=20de=20lancement=20=E2=80=94=20#309=20(QA=20verte)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cause: la politique de lancement Codex divergeait selon le chemin de composition. compile_codex_launch_policy ajoutait les roots extra (temp géré) AVANT de calculer le mode de sandbox, et le builder PTY (append_codex_pty_managed_temp_roots) n'appliquait QUE ces roots temp, ignorant project root et politique compilée ; sans permissions explicites, codex_permission_writable_roots renvoyait une liste vide, donc le premier lancement TUI partait sans --add-dir project root et rejetait les roots additionnels (« Error adding directories: Ignoring --add-dir … because the effective permissions do not allow additional writable roots ») ; un --add-dir préexistant dans spec.args n'était pas réconcilié, d'où des comportements différents entre first-run, stop/relaunch, CLI custom et délégation headless — invisible sur le projet IdeA lui-même du fait de ses permissions déjà configurées. fix: codex_permission_writable_roots retourne par défaut le project root quand aucune permission explicite n'est configurée (jamais vide si root non vide) ; compile_codex_launch_policy n'ajoute les roots extra qu'en mode workspace-write et reste vide en read-only (deny explicite ne gagne jamais project root ni temp géré) ; nouveau apply_codex_pty_launch_policy unique source de vérité PTY Codex — réécrit la valeur --sandbox existante ou l'ajoute, draine le --add-dir préexistant puis pose les roots compilés dédupliqués, composition identique pour first-run, relaunch, custom CLI et headless ; unconfigured_policy_writes_project_and_deduplicated_managed_temp, explicit_write_deny_never_gains_project_or_managed_temp_root, partial_allow_exposes_only_its_computed_rw_grant + tests lifecycle de réécriture d'args ajoutés. QA verte: cargo targeted tests OK, npm targeted/typecheck OK — reconfirmée par Git: git diff --check OK, cargo fmt --all -- --check OK, cargo test -p application --lib codex_permissions 3/3, cargo test -p application --lib lifecycle 48/48. fichiers .ideai/* (agents/test.md, chat-drafts.json, system-permissions.json, tickets/counter.json, tickets/index.json, tickets/308/, tickets/309/), attachments/ et scripts/ hors périmètre et préservés en worktree ; aucun push. Co-Authored-By: Claude Opus 4.8 --- crates/application/src/agent/lifecycle.rs | 147 ++++++++++++++++++-- crates/application/src/codex_permissions.rs | 83 ++++++++++- crates/application/tests/agent_lifecycle.rs | 20 ++- 3 files changed, 235 insertions(+), 15 deletions(-) diff --git a/crates/application/src/agent/lifecycle.rs b/crates/application/src/agent/lifecycle.rs index 2b80f260..faefe20e 100644 --- a/crates/application/src/agent/lifecycle.rs +++ b/crates/application/src/agent/lifecycle.rs @@ -37,9 +37,7 @@ use domain::{ use domain::live_state::WorkStatus; -use crate::codex_permissions::{ - codex_permission_writable_roots, codex_sandbox_mode, compile_codex_launch_policy, -}; +use crate::codex_permissions::{codex_permission_writable_roots, compile_codex_launch_policy}; use crate::error::AppError; use crate::layout::{persist_doc, resolve_doc}; use crate::managed_temp::{ @@ -1649,19 +1647,56 @@ fn append_codex_pty_model_overrides(profile: &AgentProfile, spec: &mut SpawnSpec } } -fn append_codex_pty_managed_temp_roots( +fn apply_codex_pty_launch_policy( profile: &AgentProfile, permissions: Option<&EffectivePermissions>, + network: Option, + project_root: &str, + run_dir: &str, managed_temp_roots: &[String], spec: &mut SpawnSpec, ) { - if managed_temp_roots.is_empty() || !is_codex_profile(profile) { + if !is_codex_profile(profile) { return; } - if codex_sandbox_mode(permissions, managed_temp_roots) != "workspace-write" { - return; + + let StructuredProviderLaunchPolicy::Codex { + sandbox_mode, + writable_roots, + .. + } = compile_codex_launch_policy( + project_root, + run_dir, + permissions, + network, + managed_temp_roots, + ); + + let mut index = 0; + let mut sandbox_seen = false; + while index < spec.args.len() { + if spec.args[index] == "--sandbox" && index + 1 < spec.args.len() { + spec.args[index + 1] = sandbox_mode.clone(); + sandbox_seen = true; + index += 2; + continue; + } + if spec.args[index] == "--add-dir" + && spec + .args + .get(index + 1) + .is_some_and(|root| root == project_root) + { + spec.args.drain(index..=index + 1); + continue; + } + index += 1; } - for root in managed_temp_roots { + if !sandbox_seen { + spec.args.push("--sandbox".to_owned()); + spec.args.push(sandbox_mode); + } + for root in writable_roots { if spec .args .windows(2) @@ -1670,7 +1705,7 @@ fn append_codex_pty_managed_temp_roots( continue; } spec.args.push("--add-dir".to_owned()); - spec.args.push(root.clone()); + spec.args.push(root); } } @@ -2626,9 +2661,12 @@ impl LaunchAgent { spec.sandbox = enrich_sandbox_plan_with_managed_temp_roots(spec.sandbox, &managed_temp_writable_roots); spec.sandbox = enrich_sandbox_plan_with_plugin_mcp_roots(spec.sandbox, &plugin_mcp_servers); - append_codex_pty_managed_temp_roots( + apply_codex_pty_launch_policy( &profile, effective_permissions.as_ref(), + network_permission, + input.project.root.as_str(), + run_dir.as_str(), &managed_temp_writable_roots, &mut spec, ); @@ -4630,6 +4668,95 @@ mod tests { use super::*; use domain::Skill; + fn codex_profile() -> AgentProfile { + crate::agent::reference_profiles() + .into_iter() + .find(|profile| profile.projector == Some(ProjectorKey::Codex)) + .expect("reference Codex profile") + } + + fn spawn_spec(args: Vec<&str>) -> SpawnSpec { + SpawnSpec { + command: "codex".to_owned(), + args: args.into_iter().map(str::to_owned).collect(), + cwd: ProjectPath::new("/project/.ideai/run/main").unwrap(), + env: Vec::new(), + context_plan: None, + sandbox: None, + } + } + + #[test] + fn codex_pty_first_launch_pairs_workspace_mode_with_project_and_temp_roots() { + let mut spec = spawn_spec(Vec::new()); + apply_codex_pty_launch_policy( + &codex_profile(), + None, + None, + "/project", + "/project/.ideai/run/main", + &["/managed/tmp".to_owned(), "/project".to_owned()], + &mut spec, + ); + + assert!(spec + .args + .windows(2) + .any(|pair| pair == ["--sandbox", "workspace-write"])); + assert_eq!( + spec.args + .windows(2) + .filter(|pair| pair == &["--add-dir", "/project"]) + .count(), + 1 + ); + assert_eq!( + spec.args + .windows(2) + .filter(|pair| pair == &["--add-dir", "/managed/tmp"]) + .count(), + 1 + ); + } + + #[test] + fn codex_pty_deny_replaces_advisory_workspace_root_without_temp_escape() { + use domain::permission::{ + resolve, Capability, Effect, PathScope, PermissionRule, PermissionSet, Posture, + }; + + let deny = PermissionRule::file( + Capability::Delete, + Effect::Deny, + PathScope::new(["**".to_owned()]).unwrap(), + ) + .unwrap(); + let permissions = + resolve(Some(&PermissionSet::new(vec![deny], Posture::Ask)), None).unwrap(); + let mut spec = spawn_spec(vec![ + "--sandbox", + "workspace-write", + "--add-dir", + "/project", + ]); + + apply_codex_pty_launch_policy( + &codex_profile(), + Some(&permissions), + None, + "/project", + "/project/.ideai/run/main", + &["/managed/tmp".to_owned()], + &mut spec, + ); + + assert!(spec + .args + .windows(2) + .any(|pair| pair == ["--sandbox", "read-only"])); + assert!(!spec.args.iter().any(|arg| arg == "--add-dir")); + } + #[allow(clippy::too_many_arguments)] fn compose_convention_file( project_root: &str, diff --git a/crates/application/src/codex_permissions.rs b/crates/application/src/codex_permissions.rs index ce59359d..a4c166cd 100644 --- a/crates/application/src/codex_permissions.rs +++ b/crates/application/src/codex_permissions.rs @@ -11,9 +11,10 @@ pub(crate) fn compile_codex_launch_policy( extra_writable_roots: &[String], ) -> StructuredProviderLaunchPolicy { let mut writable_roots = codex_permission_writable_roots(project_root, run_dir, permissions); - append_unique_paths(&mut writable_roots, extra_writable_roots); let sandbox_mode = codex_sandbox_mode(permissions, &writable_roots).to_owned(); - if sandbox_mode != "workspace-write" { + if sandbox_mode == "workspace-write" { + append_unique_paths(&mut writable_roots, extra_writable_roots); + } else { writable_roots.clear(); } @@ -30,7 +31,11 @@ pub(crate) fn codex_permission_writable_roots( permissions: Option<&EffectivePermissions>, ) -> Vec { let Some(permissions) = permissions else { - return Vec::new(); + return if project_root.is_empty() { + Vec::new() + } else { + vec![project_root.to_owned()] + }; }; let mut roots = Vec::new(); @@ -99,3 +104,75 @@ fn append_unique_path(target: &mut Vec, root: &str) { } target.push(root.to_owned()); } + +#[cfg(test)] +mod tests { + use super::*; + use domain::permission::{resolve, PathScope, PermissionRule, PermissionSet}; + + fn effective(rules: Vec, fallback: Posture) -> EffectivePermissions { + resolve(Some(&PermissionSet::new(rules, fallback)), None).unwrap() + } + + fn codex(policy: StructuredProviderLaunchPolicy) -> (String, Vec) { + let StructuredProviderLaunchPolicy::Codex { + sandbox_mode, + writable_roots, + .. + } = policy; + (sandbox_mode, writable_roots) + } + + #[test] + fn unconfigured_policy_writes_project_and_deduplicated_managed_temp() { + let (mode, roots) = codex(compile_codex_launch_policy( + "/project", + "/project/.ideai/run/main", + None, + None, + &["/managed/tmp".to_owned(), "/project".to_owned()], + )); + assert_eq!(mode, "workspace-write"); + assert_eq!(roots, vec!["/project", "/managed/tmp"]); + } + + #[test] + fn explicit_write_deny_never_gains_project_or_managed_temp_root() { + let deny = PermissionRule::file( + Capability::Write, + Effect::Deny, + PathScope::new(["**".to_owned()]).unwrap(), + ) + .unwrap(); + let permissions = effective(vec![deny], Posture::Ask); + let (mode, roots) = codex(compile_codex_launch_policy( + "/project", + "/run", + Some(&permissions), + None, + &["/managed/tmp".to_owned()], + )); + assert_eq!(mode, "read-only"); + assert!(roots.is_empty()); + } + + #[test] + fn partial_allow_exposes_only_its_computed_rw_grant() { + let allow = PermissionRule::file( + Capability::Write, + Effect::Allow, + PathScope::new(["generated/**".to_owned()]).unwrap(), + ) + .unwrap(); + let permissions = effective(vec![allow], Posture::Deny); + let (mode, roots) = codex(compile_codex_launch_policy( + "/project", + "/run", + Some(&permissions), + None, + &[], + )); + assert_eq!(mode, "workspace-write"); + assert_eq!(roots, vec!["/project/generated"]); + } +} diff --git a/crates/application/tests/agent_lifecycle.rs b/crates/application/tests/agent_lifecycle.rs index c9dda887..7a7622e6 100644 --- a/crates/application/tests/agent_lifecycle.rs +++ b/crates/application/tests/agent_lifecycle.rs @@ -4342,9 +4342,16 @@ async fn codex_network_env_is_projected_without_filesystem_permission_policy() { .contains("[sandbox_workspace_write]\nnetwork_access = true")), "network policy must still update Codex config without filesystem/bash policy" ); + let args = &pty.spawns()[0].args; assert!( - !pty.spawns()[0].args.contains(&"--sandbox".to_owned()), - "no filesystem/bash policy means no Codex sandbox args are folded" + args.windows(2) + .any(|pair| pair == ["--sandbox", "workspace-write"]), + "Codex runs from its isolated run dir, so an unconfigured project still needs an explicit workspace-write mode: {args:?}" + ); + assert!( + args.windows(2) + .any(|pair| pair == ["--add-dir", "/home/me/proj"]), + "the real project root must be writable on the first PTY spawn: {args:?}" ); assert_eq!( pty.spawns()[0] @@ -4396,6 +4403,15 @@ async fn codex_projection_forwards_network_env_to_structured_session_factory() { "structured launch output is populated" ); assert_eq!(factory.envs().len(), 1, "exactly one structured launch"); + assert_eq!( + factory.policies(), + vec![Some(domain::ports::StructuredProviderLaunchPolicy::Codex { + sandbox_mode: "workspace-write".to_owned(), + writable_roots: vec!["/home/me/proj".to_owned()], + network_access: true, + })], + "unconfigured structured Codex must receive the project root because cwd is the run dir" + ); assert_eq!( factory.envs()[0] .iter()