diff --git a/docs/ENGINE.md b/docs/ENGINE.md index f651bcc2a..b3a29a6fe 100644 --- a/docs/ENGINE.md +++ b/docs/ENGINE.md @@ -1735,7 +1735,16 @@ warning and the rest are kept. then `projects/` in the engine's own `CLAUDE_CONFIG_DIR` and in every listed account, so a session on another login is still found. - Folder-trust pre-acceptance writes to the session's own config directory - (above). Create a profile with `claude-profile prepare `: it makes the + (above). Codex is pre-trusted the same way (WI-1667): before a Codex + session starts, the engine records `trust_level = "trusted"` for that + session's cwd under `[projects.""]` in `$CODEX_HOME/config.toml` + (`~/.codex/config.toml` when `CODEX_HOME` is unset). The rewrite holds the + same lock the MCP bootstrap uses, leaves a config that does not parse + untouched, and does not overwrite a `trust_level` an operator already + set. `ENGINE_AGENT_QUIET_ONBOARDING=0` skips it, as it skips Claude's. + Codex's "Trust this folder?" screen is also a startup gate + (`approval.kind` `folder-trust`), so a session that still reaches it can + be answered with `session_answer`. Create a profile with `claude-profile prepare `: it makes the directory, links the main account's settings, skills and agents in, sets the auto-mode flag, and registers the MCP servers there by running `vogt-mcp-bootstrap` with `CLAUDE_CONFIG_DIR` set. Then log in once with diff --git a/engine/server/src/approval.rs b/engine/server/src/approval.rs index da440a0dc..fb63fc036 100644 --- a/engine/server/src/approval.rs +++ b/engine/server/src/approval.rs @@ -51,7 +51,7 @@ const BORDER: &[char] = &[ /// because a TUI may position words with cursor moves rather than spaces. static MENTIONS: Lazy = Lazy::new(|| { regex::bytes::Regex::new( - r"(?i)permission\s*required|always\s*allow|allow\s*once|do\s*you\s*want\s*to|would\s*you\s*like\s*to\s*(?:run|make|apply|allow|grant|send)|allow\s*(?:command|.{1,80}?to\s*run\s*tool)|press\s*\S+\s*to\s*confirm\s*or\s*\S+\s*to\s*cancel|field\s*\d+\s*/\s*\d+|needs\s*your\s*approval|approve\s*network|grant\s*these\s*permissions|send\s*input\s*to|automatically\s*den|requires\s*approval|trust\s*the\s*files|project\s*you\s*(?:created|trust)|external\s*claude\.md|outside\s*(?:of\s*)?(?:the\s*)?(?:current\s*)?working\s*director|\(y\)\s*es\s*/\s*\(n\)\s*o", + r"(?i)permission\s*required|always\s*allow|allow\s*once|do\s*you\s*want\s*to|would\s*you\s*like\s*to\s*(?:run|make|apply|allow|grant|send)|allow\s*(?:command|.{1,80}?to\s*run\s*tool)|press\s*\S+\s*to\s*confirm\s*or\s*\S+\s*to\s*cancel|field\s*\d+\s*/\s*\d+|needs\s*your\s*approval|approve\s*network|grant\s*these\s*permissions|send\s*input\s*to|automatically\s*den|requires\s*approval|trust\s*the\s*files|trust\s*this\s*folder|project\s*you\s*(?:created|trust)|external\s*claude\.md|outside\s*(?:of\s*)?(?:the\s*)?(?:current\s*)?working\s*director|\(y\)\s*es\s*/\s*\(n\)\s*o", ) .expect("approval prefilter compiles") }); @@ -60,7 +60,7 @@ static MENTIONS: Lazy = Lazy::new(|| { /// line of the rendered screen. static QUESTION: Lazy = Lazy::new(|| { Regex::new( - r"(?i)^(?:do you want to (?:proceed|make this edit|make these edits|create|run|allow|delete|overwrite|fetch|use|write|execute|apply)\b.*\?|would you like to (?:run|make|apply|allow)\b.*\?|allow (?:command|this command|edit|tool)\b.*\??|.*\brequires approval\b.*|do you trust the files in this folder\??|.*\bis this a project you (?:created or one you )?trust\b.*\??|allow external claude\.md(?: file)? imports\??)$", + r"(?i)^(?:do you want to (?:proceed|make this edit|make these edits|create|run|allow|delete|overwrite|fetch|use|write|execute|apply)\b.*\?|would you like to (?:run|make|apply|allow)\b.*\?|allow (?:command|this command|edit|tool)\b.*\??|.*\brequires approval\b.*|do you trust the files in this folder\??|trust this folder\??|.*\bis this a project you (?:created or one you )?trust\b.*\??|allow external claude\.md(?: file)? imports\??)$", ) .expect("approval question regex compiles") }); @@ -949,6 +949,41 @@ mod tests { assert!(d.options[0].selected && !d.options[1].selected); assert_eq!(d.options[1].label, "No, exit"); + // WI-1667: Codex's folder-trust gate (codex-cli 0.162, captured from + // the binary's TUI strings). The question is "Trust this folder?", + // the first option is preselected, and Enter alone chooses it — + // typing "1" and Enter quits the session. It is a startup gate, so + // session_answer can move the highlight and the change feed sees it. + let codex_trust = lines( + "Folder access\n\ + Trust this folder?\n\ + Codex can read, edit, and run files here, subject to your permission settings.\n\ + \n\ + ❯ 1. Trust and continue\n\ + 2. Open restricted\n", + ); + let d = detect(&codex_trust, &[]).expect("codex trust gate detected"); + assert_eq!(d.kind, "folder-trust"); + assert_eq!(d.question, "Trust this folder?"); + assert_eq!(d.options.len(), 2); + assert!(d.options[0].selected && !d.options[1].selected); + assert_eq!(d.options[0].label, "Trust and continue"); + assert_eq!(d.options[1].label, "Open restricted"); + assert!(mentions_approval(b"Trust this folder?")); + + // The screen reported from prod named the second option "Quit" + // rather than "Open restricted". Detection keys on the question, so + // that variant is the same gate. + let quit = lines( + "Folder access\n\ + Trust this folder?\n\ + ❯ 1. Trust and continue\n\ + 2. Quit\n", + ); + let d = detect(&quit, &[]).expect("quit variant detected"); + assert_eq!(d.kind, "folder-trust"); + assert_eq!(d.options[1].label, "Quit"); + let imports = lines( "Allow external CLAUDE.md file imports?\n\ This project's CLAUDE.md imports files outside the current working directory.\n\ diff --git a/engine/server/src/claude_config.rs b/engine/server/src/claude_config.rs index 5aaf71f5e..98ed5938b 100644 --- a/engine/server/src/claude_config.rs +++ b/engine/server/src/claude_config.rs @@ -531,6 +531,21 @@ impl Onboarding { .map(|dir| dir.join(".claude.json")) } + /// [`accept_codex_project`] for a Codex session about to start in + /// `cwd`, logging rather than failing: the session starts either way. + /// Codex reads `~/.codex/config.toml` (`CODEX_HOME` overrides it). + pub fn prepare_codex(&self, env: &[(String, String)], cwd: &str) { + if !self.enabled { + return; + } + let Some(path) = codex_config_path(env) else { + return; + }; + if let Err(e) = accept_codex_project(&path, cwd) { + tracing::info!(path = %path.display(), error = %e, "could not pre-trust the Codex folder; it may ask"); + } + } + /// [`accept_project`] for a Claude session about to start in `cwd`, /// logging rather than failing: the session starts either way. pub fn prepare(&self, env: &[(String, String)], cwd: &str) { @@ -599,6 +614,169 @@ pub fn accept_project(path: &Path, cwd: &str) -> std::io::Result { result.map(|()| true) } +/// Codex's config file: `$CODEX_HOME/config.toml` when the session sets it, +/// otherwise `$HOME/.codex/config.toml`. +fn codex_config_path(env: &[(String, String)]) -> Option { + if let Some((_, home)) = env.iter().rev().find(|(k, _)| k == "CODEX_HOME") { + return Some(PathBuf::from(home).join("config.toml")); + } + std::env::var_os("HOME").map(|home| PathBuf::from(home).join(".codex").join("config.toml")) +} + +/// A path Codex will accept as a `[projects.""]` key: absolute, UTF-8, +/// with no control characters and no quote. Anything else is left untrusted +/// rather than written into a config the session shares. +fn codex_project_key(cwd: &str) -> Option { + let path = Path::new(cwd); + if !path.is_absolute() || cwd.chars().any(|c| c.is_control() || c == '"') { + return None; + } + Some(cwd.to_string()) +} + +/// Record `trust_level = "trusted"` for `cwd` in Codex's `config.toml` +/// (WI-1667). +/// +/// Codex stops at "Trust this folder?" the first time it is started in a +/// directory it has not seen, and that screen is a startup gate a driven +/// session used to sit at — or exit from, when "1" was typed into a menu +/// whose first option Enter alone selects. The workspace the engine launches +/// into is one the operator trusts by deploying, so the session's own cwd is +/// recorded before it starts. +/// +/// The rewrite is the same shape the MCP bootstrap uses on this file: one +/// flock, a config that does not parse is left byte for byte, and the new +/// text is renamed over the old one. Only the one project table is added or +/// completed. An operator's `trust_level` of anything other than `trusted` +/// is left as they set it, and every other table is untouched. +pub fn accept_codex_project(path: &Path, cwd: &str) -> std::io::Result { + let Some(key) = codex_project_key(cwd) else { + return Ok(false); + }; + let parent = path.parent().unwrap_or_else(|| Path::new(".")); + std::fs::create_dir_all(parent)?; + let lock_path = parent.join(".config.toml.lock"); + let lock = std::fs::OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(&lock_path)?; + flock_exclusive(&lock)?; + + let existing = match std::fs::read(path) { + Ok(bytes) => bytes, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Vec::new(), + Err(e) => return Err(e), + }; + let text = String::from_utf8(existing.clone()).map_err(|_| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + "config.toml is not UTF-8; left alone", + ) + })?; + if !text.trim().is_empty() && toml::from_str::(&text).is_err() { + return Ok(false); + } + let Some(new) = codex_trust_edit(&text, &key) else { + return Ok(false); + }; + let tmp = parent.join(format!(".config.toml.vogt-{}", uuid::Uuid::new_v4())); + let result = (|| { + std::fs::write(&tmp, new.as_bytes())?; + if let Ok(meta) = std::fs::metadata(path) { + std::fs::set_permissions(&tmp, meta.permissions())?; + } + std::fs::rename(&tmp, path) + })(); + if result.is_err() { + let _ = std::fs::remove_file(&tmp); + } + result.map(|()| true) +} + +/// `LOCK_EX` on `file`, the same lock the MCP bootstrap holds while it +/// rewrites this config. +fn flock_exclusive(file: &std::fs::File) -> std::io::Result<()> { + let fd = std::os::unix::io::AsRawFd::as_raw_fd(file); + // SAFETY: `fd` is a live file descriptor and `LOCK_EX` only waits for + // the advisory lock on it. + let rc = unsafe { libc_flock(fd, 2) }; + if rc == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +unsafe fn libc_flock(fd: i32, op: i32) -> i32 { + libc::flock(fd, op) +} + +/// The text with `[projects.""]` trusted, or `None` when the project is +/// already present under any TOML spelling, or the edit would not parse. +/// +/// Presence is decided on the parsed document, not on one header spelling. +/// A literal-string key, a dotted key or an inline `[projects]` entry is the +/// same project, and appending a second table for it would make the file +/// invalid — Codex would then refuse to load its config at all, and an +/// operator's `trust_level` would be shadowed (WI-1667 review). +fn codex_trust_edit(text: &str, key: &str) -> Option { + let parsed: toml::Value = if text.trim().is_empty() { + toml::Value::Table(toml::Table::new()) + } else { + toml::from_str(text).ok()? + }; + if parsed + .get("projects") + .and_then(toml::Value::as_table) + .is_some_and(|projects| projects.contains_key(key)) + { + return None; + } + let header = format!("[projects.{}]", toml_basic_string(key)); + let mut body = text.to_string(); + if !body.is_empty() && !body.ends_with('\n') { + body.push('\n'); + } + if !body.is_empty() && !body.ends_with("\n\n") { + body.push('\n'); + } + body.push_str(&header); + body.push_str("\ntrust_level = \"trusted\"\n"); + // Never rename a file that does not parse, or that did not record the + // trust the caller asked for. + let written: toml::Value = toml::from_str(&body).ok()?; + let trusted = written + .get("projects") + .and_then(|p| p.get(key)) + .and_then(|entry| entry.get("trust_level")) + .and_then(toml::Value::as_str) + == Some("trusted"); + trusted.then_some(body) +} + +/// A TOML basic string for `value`, so the header key round-trips through +/// the parser. Rust's `Debug` writes some characters as `\u{..}`, which +/// TOML does not accept. +fn toml_basic_string(value: &str) -> String { + let mut out = String::from("\""); + for c in value.chars() { + match c { + '"' => out.push_str("\\\""), + '\\' => out.push_str("\\\\"), + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + c if c.is_control() => { + out.push_str(&format!("\\u{:04X}", u32::from(c))); + } + c => out.push(c), + } + } + out.push('"'); + out +} + #[cfg(test)] mod tests { use super::*; @@ -694,6 +872,84 @@ mod tests { assert!(none.contains("no accounts are configured"), "{none}"); } + #[test] + fn codex_trust_is_recorded_once_and_everything_else_is_kept() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("config.toml"); + std::fs::write( + &path, + "model = \"grok\"\n\n[projects.\"/w/other\"]\ntrust_level = \"trusted\"\n\n[mcp_servers.vogt]\ncommand = \"vogt-mcp\"\n", + ) + .unwrap(); + assert!(accept_codex_project(&path, "/w/repo").unwrap()); + let text = std::fs::read_to_string(&path).unwrap(); + let parsed: toml::Value = toml::from_str(&text).unwrap(); + assert_eq!(parsed["model"].as_str(), Some("grok")); + assert_eq!( + parsed["projects"]["/w/other"]["trust_level"].as_str(), + Some("trusted") + ); + assert_eq!( + parsed["projects"]["/w/repo"]["trust_level"].as_str(), + Some("trusted") + ); + assert_eq!( + parsed["mcp_servers"]["vogt"]["command"].as_str(), + Some("vogt-mcp") + ); + assert!(!accept_codex_project(&path, "/w/repo").unwrap()); + assert_eq!(std::fs::read_to_string(&path).unwrap(), text); + } + + #[test] + fn an_operator_trust_choice_and_a_broken_config_are_left_alone() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("config.toml"); + let chosen = "[projects.\"/w/repo\"]\ntrust_level = \"untrusted\"\n"; + std::fs::write(&path, chosen).unwrap(); + assert!(!accept_codex_project(&path, "/w/repo").unwrap()); + assert_eq!(std::fs::read_to_string(&path).unwrap(), chosen); + + let broken = dir.path().join("broken.toml"); + std::fs::write(&broken, "not = [toml").unwrap(); + assert!(!accept_codex_project(&broken, "/w/repo").unwrap()); + assert_eq!(std::fs::read(&broken).unwrap(), b"not = [toml"); + + let missing = dir.path().join("sub").join("config.toml"); + assert!(accept_codex_project(&missing, "/w/repo").unwrap()); + assert!(std::fs::read_to_string(&missing) + .unwrap() + .contains("trust_level = \"trusted\"")); + // A relative path, or one that cannot be a TOML key, is not written. + assert!(!accept_codex_project(&missing, "relative").unwrap()); + assert!(!accept_codex_project(&missing, "/w/\"quoted").unwrap()); + } + + #[test] + fn a_project_already_present_in_another_spelling_is_left_alone() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("config.toml"); + // Literal-string key, and an inline table: both name the same + // project the double-quoted header would. Appending a second table + // would make the file invalid. + for existing in [ + "[projects.'/w/repo']\ntrust_level = \"untrusted\"\n", + "[projects]\n\"/w/repo\" = { trust_level = \"untrusted\" }\n", + ] { + std::fs::write(&path, existing).unwrap(); + assert!( + !accept_codex_project(&path, "/w/repo").unwrap(), + "rewrote {existing:?}" + ); + assert_eq!(std::fs::read_to_string(&path).unwrap(), existing); + let parsed: toml::Value = toml::from_str(existing).unwrap(); + assert_eq!( + parsed["projects"]["/w/repo"]["trust_level"].as_str(), + Some("untrusted") + ); + } + } + #[test] fn the_session_config_dir_wins() { let onboarding = Onboarding { diff --git a/engine/server/src/sessions.rs b/engine/server/src/sessions.rs index 04919c562..d8e95331e 100644 --- a/engine/server/src/sessions.rs +++ b/engine/server/src/sessions.rs @@ -605,19 +605,21 @@ impl SessionRegistry { // Claude Code asks, per directory, whether to trust it and whether to // allow a CLAUDE.md that imports from outside it. Answer both before // it starts, so a woken or driven session does not sit at a modal. - if base_command - .as_deref() - .and_then(agent_cli::agent_name) - .as_deref() - == Some("claude") - { + let agent = base_command.as_deref().and_then(agent_cli::agent_name); + // Claude Code and Codex each ask, per directory, whether to trust + // it. Answer both before the CLI starts, so a woken or driven + // session does not sit at a modal (WI-917, WI-1667). + if matches!(agent.as_deref(), Some("claude" | "codex")) { let cwd = spec .cwd .clone() .unwrap_or_else(|| self.cfg.default_cwd.to_string_lossy().into_owned()); - self.cfg - .agent_onboarding - .prepare(spec.env.as_deref().unwrap_or_default(), &cwd); + let env = spec.env.as_deref().unwrap_or_default(); + if agent.as_deref() == Some("claude") { + self.cfg.agent_onboarding.prepare(env, &cwd); + } else { + self.cfg.agent_onboarding.prepare_codex(env, &cwd); + } } // Names need not be unique — duplicates are merely confusing, not invalid. diff --git a/engine/server/tests/integration.rs b/engine/server/tests/integration.rs index 68a3e280a..a3b61ee0a 100644 --- a/engine/server/tests/integration.rs +++ b/engine/server/tests/integration.rs @@ -3877,17 +3877,33 @@ async fn warm_attach_within_budget_stays_a_byte_exact_delta() { .await .unwrap(); let _ = collect_binary_until(&mut ws, b"before-cursor", Duration::from_secs(2)).await; + // The bytes arriving on the socket is not the ring having absorbed them: + // the reader commits the scrollback after the broadcast, so a cursor read + // in that gap still points before "before-cursor" and the resume then + // includes it (seen failing in CI on an otherwise unrelated change). Wait + // until the recorded position has moved past the priming output. let cursor = { - let detail: Value = client - .get(format!("{base}/api/sessions/{id}")) - .send() - .await - .unwrap() - .json() - .await - .unwrap(); - detail["scrollback_pos"].as_u64().unwrap() + let deadline = std::time::Instant::now() + Duration::from_secs(2); + loop { + let detail: Value = client + .get(format!("{base}/api/sessions/{id}")) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let pos = detail["scrollback_pos"].as_u64().unwrap(); + if pos > 0 || std::time::Instant::now() >= deadline { + break pos; + } + tokio::time::sleep(Duration::from_millis(20)).await; + } }; + assert!( + cursor > 0, + "the priming output never reached the scrollback" + ); ws.send(Message::Binary(b"after-cursor\n".to_vec().into())) .await .unwrap();