From 5bfd01bbe6d0b9563cfed7b706b4c7b43ae8f3bc Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 6 Sep 2026 15:18:18 -0700 Subject: [PATCH 1/5] refactor: improve error handling and validation in session JSON parsing --- docs/sessions.md | 6 +- src/session.rs | 336 +++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 286 insertions(+), 56 deletions(-) diff --git a/docs/sessions.md b/docs/sessions.md index 75390ec..2fa1370 100644 --- a/docs/sessions.md +++ b/docs/sessions.md @@ -47,7 +47,11 @@ The shape, not the version, discriminates the schema. An object-valued `dirs` ma Saves are atomic: `fleetcom` writes and syncs a private temporary file in the session directory, then renames it over the recipe. Recipes persist full command lines, which can embed secrets. [Security](README.md#security) documents the directory and file permissions. -The file is plain JSON and practical to edit by hand. Editing the `name` field changes which session the file claims to be: collision checks compare it, so a save under the old name will be refused. On load, the daemon removes control characters, trims surrounding whitespace, and limits group and display names to 64 characters. `Unassigned` maps to no group but remains a legal display name. Invalid JSON fails the entire load. Within valid JSON, `fleetcom` drops any member that matches neither entry form, including a non-string scalar, an object without a string `cmd`, or an object with a non-string `group` or `name`. +The file is plain JSON and practical to edit by hand. Editing the `name` field changes which session the file claims to be: collision checks compare it, so a save under the old name will be refused. A readable stored name still controls collision checks and picker labels when the command body is invalid or the version is unsupported. + +Loading validates the entire recipe before starting any commands. The root must be an object, every directory value must be an array, and every entry must be a command string or an object with a string `cmd`. Optional entry `group` and `name` fields, and the wrapped session's `name`, accept strings, `null`, or omission; other types fail. Unknown fields in wrapped metadata and entry objects are ignored. Empty maps, arrays, and strings are valid. Invalid JSON or any malformed field fails the whole load, leaves existing tasks intact, and preserves the recipe file. Schema errors identify the quoted directory key, the entry number (starting at 1), and the offending field where applicable. + +After validation, the daemon removes control characters, trims surrounding whitespace, and limits group and display names to 64 characters. `Unassigned` maps to no group but remains a legal display name. Missing directories and entries exceeding task or command limits are still skipped and counted in the load status. Spawn failures are reported separately; commands already started by a structurally valid recipe keep running. Commands with neither a group nor a name use the string form. String and object entries can appear in the same directory array. diff --git a/src/session.rs b/src/session.rs index cfb3f00..3fc0220 100644 --- a/src/session.rs +++ b/src/session.rs @@ -113,21 +113,25 @@ pub fn fingerprint_json(cfg: &SessionConfig) -> String { /// A top-level `version` must be an integer from 1 through [`FORMAT_VERSION`]; /// a missing version is interpreted as 1. fn from_json(text: &str) -> io::Result<(Option, SessionConfig)> { - let parsed = jzon::parse(text).map_err(|e| io::Error::other(e.to_string()))?; + let invalid = |message| io::Error::new(io::ErrorKind::InvalidData, message); + let parsed = jzon::parse(text).map_err(|e| invalid(e.to_string()))?; + if !parsed.is_object() { + return Err(invalid("session root: expected an object".into())); + } // Validate version metadata before detecting the schema shape. let version = &parsed["version"]; - if !version.is_null() { + if parsed.has_key("version") { match version.as_u64() { Some(n) if (1..=FORMAT_VERSION).contains(&n) => {} Some(n) if n > FORMAT_VERSION => { - return Err(io::Error::other(format!( + return Err(invalid(format!( "session format version {n} is newer than this fleetcom \ (supports {FORMAT_VERSION}); load it with a newer build" ))); } // Reject zero, fractional, negative, and non-numeric values. _ => { - return Err(io::Error::other(format!( + return Err(invalid(format!( "session format version {} is not one this fleetcom reads \ (supports {FORMAT_VERSION}); load it with a newer build", version.dump() @@ -136,7 +140,8 @@ fn from_json(text: &str) -> io::Result<(Option, SessionConfig)> { } } let (name, dirs, flat) = if parsed["dirs"].is_object() { - let name = parsed["name"].as_str().map(str::to_string); + let name = opt_str(&parsed["name"]) + .ok_or_else(|| invalid("session field \"name\": expected a string or null".into()))?; (name, &parsed["dirs"], false) } else { (None, &parsed, true) @@ -147,29 +152,57 @@ fn from_json(text: &str) -> io::Result<(Option, SessionConfig)> { if flat && dir == "version" { continue; } - // Ignore members that match neither supported entry form. - let entries = val - .members() - .filter_map(|m| { - if let Some(cmd) = m.as_str() { - return Some(SessionEntry { - cmd: cmd.to_string(), - group: None, - name: None, - }); - } - // Indexing a non-object yields Null, so malformed members drop here. - let cmd = m["cmd"].as_str()?.to_string(); - let group = opt_str(&m["group"])?; - let name = opt_str(&m["name"])?; - Some(SessionEntry { cmd, group, name }) - }) - .collect(); + if !val.is_array() { + return Err(invalid(format!("directory {dir:?}: expected an array"))); + } + let mut entries = Vec::new(); + for (index, member) in val.members().enumerate() { + if let Some(cmd) = member.as_str() { + entries.push(SessionEntry { + cmd: cmd.to_string(), + group: None, + name: None, + }); + continue; + } + let location = format!("directory {dir:?}, entry {}", index + 1); + if !member.is_object() { + return Err(invalid(format!( + "{location}: expected a command string or an object" + ))); + } + let cmd = member["cmd"] + .as_str() + .ok_or_else(|| invalid(format!("{location}, field \"cmd\": expected a string")))?; + let label = |field| { + opt_str(&member[field]).ok_or_else(|| { + invalid(format!( + "{location}, field {field:?}: expected a string or null" + )) + }) + }; + entries.push(SessionEntry { + cmd: cmd.to_string(), + group: label("group")?, + name: label("name")?, + }); + } cfg.insert(dir.to_string(), entries); } Ok((name, cfg)) } +/// Inspect wrapper identity without validating its version or command body: +/// an unloadable recipe still owns its name for listings and collision checks. +fn stored_name(text: &str) -> Option { + let parsed = jzon::parse(text).ok()?; + if parsed["dirs"].is_object() { + parsed["name"].as_str().map(str::to_string) + } else { + None + } +} + // --- fs surface: callers supply the root. The supervisor resolves it from the // connection's launch context; `sessions_dir` above is only its process-env // fallback. Tests point it at scratch dirs the same way. ----------------------- @@ -237,7 +270,7 @@ pub fn save_in(dir: &Path, name: &str, cfg: &SessionConfig) -> io::Result { - if let Ok((Some(stored), _)) = from_json(&text) + if let Some(stored) = stored_name(&text) && stored != trimmed { return Err(io::Error::new( @@ -271,10 +304,7 @@ pub fn list_in(dir: &Path) -> Vec { if p.extension().and_then(|s| s.to_str()) == Some("json") && let Some(stem) = p.file_stem().and_then(|s| s.to_str()) { - let stored = fs::read_to_string(&p) - .ok() - .and_then(|t| from_json(&t).ok()) - .and_then(|(name, _)| name); + let stored = fs::read_to_string(&p).ok().and_then(|t| stored_name(&t)); names.push(stored.unwrap_or_else(|| stem.to_string())); } } @@ -643,35 +673,156 @@ mod tests { assert_eq!(cfg["~/proj"], vec![e("vim")]); } - /// Malformed members are omitted rather than decoded into partial entries. #[test] - fn malformed_object_members_drop_without_error() { - let (_, cfg) = from_json( - r#"{"d": [ - {"group": "g"}, - {"cmd": 3}, - {"cmd": "x", "group": 5}, - {"cmd": "y", "name": 5}, - 42, - {"cmd": "bare"}, - {"cmd": "n", "group": null}, - {"cmd": "m", "name": null}, - {"cmd": "ok", "group": "api"}, - {"cmd": "named", "name": "web"}, - "plain" - ]}"#, - ) - .unwrap(); + fn rejects_invalid_roots_and_syntax() { + for text in ["null", "true", "42", r#""text""#, "[]"] { + let err = from_json(text).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::InvalidData, "{text}"); + assert_eq!(err.to_string(), "session root: expected an object"); + } assert_eq!( - cfg["d"], - vec![ - e("bare"), - e("n"), - e("m"), - ge("ok", "api"), - ne("named", "web"), - e("plain") - ] + from_json("{not json").unwrap_err().kind(), + io::ErrorKind::InvalidData + ); + } + + #[test] + fn rejects_non_array_directories_with_escaped_keys() { + let dir = "d\"\\\n"; + for value in ["null", "true", "42", r#""command""#, "{}"] { + let mut body = jzon::JsonValue::new_object(); + body.insert(dir, jzon::parse(value).unwrap()).unwrap(); + for recipe in [body.clone(), jzon::object! { "dirs": body }] { + let err = from_json(&recipe.dump()).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::InvalidData, "{recipe}"); + assert_eq!( + err.to_string(), + format!("directory {dir:?}: expected an array") + ); + } + } + for value in ["null", "true", "42", r#""command""#] { + let err = from_json(&format!(r#"{{"dirs": {value}}}"#)).unwrap_err(); + assert_eq!(err.to_string(), "directory \"dirs\": expected an array"); + } + } + + /// One invalid entry rejects the recipe, including preceding valid commands. + #[test] + fn rejects_malformed_entries_with_directory_position_and_field() { + let mut cases = Vec::new(); + for value in ["null", "true", "42", "[]"] { + cases.push(( + value.to_string(), + "expected a command string or an object".to_string(), + )); + } + cases.push(("{}".into(), "field \"cmd\": expected a string".into())); + for field in ["cmd", "group", "name"] { + for value in ["null", "true", "42", "[]", "{}"] { + if field != "cmd" && value == "null" { + continue; + } + let mut entry = jzon::object! { "cmd": "secret-command" }; + entry[field] = jzon::parse(value).unwrap(); + let expected = if field == "cmd" { + "a string" + } else { + "a string or null" + }; + cases.push(( + entry.dump(), + format!("field {field:?}: expected {expected}"), + )); + } + } + for (entry, expected) in cases { + let body = format!(r#"{{"d": ["valid-command", {entry}]}}"#); + for text in [body.clone(), format!(r#"{{"dirs": {body}}}"#)] { + let err = from_json(&text).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::InvalidData, "{text}"); + let separator = if expected.starts_with("field") { + ", " + } else { + ": " + }; + assert_eq!( + err.to_string(), + format!("directory \"d\", entry 2{separator}{expected}") + ); + } + } + } + + #[test] + fn rejects_invalid_wrapper_names_and_explicit_null_versions() { + for value in ["true", "42", "[]", "{}"] { + let err = from_json(&format!(r#"{{"name": {value}, "dirs": {{}}}}"#)).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert_eq!( + err.to_string(), + "session field \"name\": expected a string or null" + ); + } + for value in ["null", "-1", "1.5", "true", "[]", "{}"] { + for body in [r#""dirs": {}"#, r#""d": []"#] { + let err = from_json(&format!(r#"{{"version": {value}, {body}}}"#)).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert!( + err.to_string().contains(&format!("version {value}")), + "{err}" + ); + assert!(err.to_string().contains("supports 1"), "{err}"); + } + } + } + + #[test] + fn accepts_empty_recipes_and_dirs_named_directory() { + for text in [ + "{}", + r#"{"version": 1}"#, + r#"{"dirs": {}}"#, + r#"{"name": null, "dirs": {}}"#, + ] { + assert_eq!(from_json(text).unwrap(), (None, SessionConfig::new())); + } + let (_, cfg) = from_json(r#"{"dirs": [], "name": [""], "": []}"#).unwrap(); + assert_eq!( + cfg, + SessionConfig::from([ + ("dirs".into(), vec![]), + ("name".into(), vec![e("")]), + ("".into(), vec![]), + ]) + ); + } + + #[test] + fn preserves_authored_strings_nullable_labels_and_unknown_fields() { + let body = r#"{"d": ["", {"cmd": "bare"}, {"cmd": "n", "group": null}, + {"cmd": "m", "name": null}, {"cmd": "", "group": "", "name": ""}, + {"cmd": " echo x\n", "group": " api ", "name": "\tweb\t", "extra": false}]}"#; + for text in [ + body.to_string(), + format!(r#"{{"name": "", "dirs": {body}, "extra": false}}"#), + ] { + let (_, cfg) = from_json(&text).unwrap(); + assert_eq!( + cfg["d"], + vec![ + e(""), + e("bare"), + e("n"), + e("m"), + gne("", "", ""), + gne(" echo x\n", " api ", "\tweb\t"), + ] + ); + } + assert_eq!( + from_json(r#"{"name": "", "dirs": {}}"#).unwrap().0, + Some("".into()) ); } @@ -780,6 +931,45 @@ mod tests { assert_eq!(load_in(&dir, "a/b").unwrap(), first); } + /// Invalid bodies and future versions retain their stored identity. + #[test] + fn unloadable_wrappers_keep_picker_names_and_collision_protection() { + let dir = temp("session_invalid_identity"); + let file = dir.join("a_b.json"); + for text in [ + r#"{"name": "a/b", "dirs": {"d": ["valid", {"cmd": false}]}}"#, + r#"{"name": "a/b", "dirs": {"d": null}}"#, + r#"{"version": 2, "name": "a/b", "dirs": {}}"#, + ] { + fs::write(&file, text).unwrap(); + assert!(load_in(&dir, "a/b").is_err()); + assert_eq!(list_in(&dir), ["a/b"]); + let err = save_in(&dir, "a.b", &SessionConfig::new()).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::AlreadyExists); + assert_eq!(fs::read(&file).unwrap(), text.as_bytes()); + save_in(&dir, "a/b", &SessionConfig::new()).unwrap(); + assert!(load_in(&dir, "a/b").unwrap().is_empty()); + } + } + + #[test] + fn corrupt_and_nameless_recipes_keep_filename_fallback_and_allow_overwrite() { + let dir = temp("session_nameless_identity"); + for text in [ + "{not json", + "null", + r#"{"dirs": {}}"#, + r#"{"name": null, "dirs": {}}"#, + r#"{"name": false, "dirs": {}}"#, + r#"{"name": ["cmd"], "dirs": []}"#, + ] { + fs::write(dir.join("mine.json"), text).unwrap(); + assert_eq!(list_in(&dir), ["mine"]); + save_in(&dir, "mine", &SessionConfig::new()).unwrap(); + assert!(load_in(&dir, "mine").unwrap().is_empty()); + } + } + /// Flat-schema files load and list by filename stem. #[test] fn loads_and_lists_legacy_flat_schema_files() { @@ -1097,6 +1287,42 @@ mod tests { ); } + #[test] + fn recovery_listing_skips_invalid_shapes_and_retains_empty_recipes() { + let rec = temp("session_recovery_shapes"); + for (index, text) in [ + "null", + "[]", + r#"{"d": null}"#, + r#"{"d": ["valid", 42]}"#, + r#"{"dirs": {"d": [{"cmd": "x", "name": false}]}}"#, + r#"{"version": null, "dirs": {}}"#, + ] + .iter() + .enumerate() + { + fs::write(rec.join(format!("invalid-{index}.json")), text).unwrap(); + } + fs::write(rec.join("empty-flat.json"), "{}").unwrap(); + fs::write( + rec.join("empty-wrapped.json"), + r#"{"name": "empty", "dirs": {"d": []}}"#, + ) + .unwrap(); + let entries = list_recovery_in(&rec); + let summary: Vec<_> = entries + .iter() + .map(|e| (e.stem.as_str(), e.label.as_str(), e.tasks)) + .collect(); + assert_eq!( + summary, + [ + ("empty-wrapped", "empty", 0), + ("empty-flat", "empty-flat", 0) + ] + ); + } + /// Recovery loads reject empty, dotted, or path-shaped stems. #[test] fn load_recovery_in_loads_by_stem_and_rejects_traversal() { From c13786e0a8ce1cb5a8ecb5a54cbd258a998ab449 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 6 Sep 2026 15:19:28 -0700 Subject: [PATCH 2/5] test: add validation for malformed session and recovery loads --- src/supervisor_capture_tests.rs | 12 ++++---- src/supervisor_tests.rs | 49 +++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/src/supervisor_capture_tests.rs b/src/supervisor_capture_tests.rs index c8e9f4d..b256f7f 100644 --- a/src/supervisor_capture_tests.rs +++ b/src/supervisor_capture_tests.rs @@ -929,11 +929,11 @@ fn home_only_launch_env_targets_the_clients_dot_codex() { ); let codex_home = home.join(".codex"); std::fs::create_dir_all(&codex_home).unwrap(); - // A multi-line notify is Opaque: injection is suppressed only when - // the guard reads the client's config.toml through HOME. + // An empty argument cannot survive the chain transport: injection is + // suppressed only when the guard reads the client's config through HOME. std::fs::write( codex_home.join("config.toml"), - "notify = [\n \"/my/thing\",\n]\n", + "notify = [\"/my/thing\", \"\"]\n", ) .unwrap(); install_stub(&bin, "codex", &dir); @@ -1128,10 +1128,10 @@ fn unrepresentable_config_notify_suppresses_injection() { let (bin, runtime) = (dir.join("bin"), dir.join("run")); let codex_home = dir.join("codex_home"); std::fs::create_dir_all(&codex_home).unwrap(); - // A multi-line array is out of the line-based parser's reach. + // An empty argument cannot survive shell field splitting. std::fs::write( codex_home.join("config.toml"), - "notify = [\n \"/my/thing\",\n]\n", + "notify = [\"/my/thing\", \"\"]\n", ) .unwrap(); install_stub(&bin, "codex", &dir); @@ -1145,7 +1145,7 @@ fn unrepresentable_config_notify_suppresses_injection() { let argv = wait_argv(&mut s, &dir.join("argv")); assert!( !argv.iter().any(|a| a.contains("notify=")), - "fleetcom must not guess at an unparseable notify; argv: {argv:?}" + "fleetcom must preserve an unrepresentable notify; argv: {argv:?}" ); // The same route commented out is inert: the injection returns. diff --git a/src/supervisor_tests.rs b/src/supervisor_tests.rs index 022b5ab..79f0527 100644 --- a/src/supervisor_tests.rs +++ b/src/supervisor_tests.rs @@ -1678,6 +1678,55 @@ fn load_surfaces_parse_errors_instead_of_absence() { ); } +/// Both load paths validate every entry before admitting any recipe commands. +#[test] +fn malformed_session_and_recovery_loads_preserve_tasks_and_recipe_bytes() { + let dir = scratch("sess_schema_err"); + let config = dir.join("config"); + let sessions = config.join("sessions"); + let recovery = sessions.join("recovery"); + std::fs::create_dir_all(&recovery).unwrap(); + let text = r#"{"dirs": {".": ["sleep 32", {"cmd": "sleep 33", "name": false}]}}"#; + let mut s = sup_ctx(config_ctx(&config, dir.to_path_buf(), &[])); + spawn(&mut s, "sleep 31", dir.to_path_buf()); + let existing_id = first_id(&mut s); + s.drain(); + + for (file, command, subject) in [ + ( + sessions.join("broken.json"), + Command::LoadSession { + name: "broken".into(), + }, + "session 'broken'", + ), + ( + recovery.join("20260714-093015-11.json"), + Command::LoadRecovery { + stem: "20260714-093015-11".into(), + }, + "recovery snapshot '20260714-093015-11'", + ), + ] { + std::fs::write(&file, text).unwrap(); + s.apply(command); + assert_eq!(s.tasks.len(), 1, "{subject} must add no tasks"); + assert_eq!(s.tasks[0].id, existing_id); + assert_eq!(s.tasks[0].command, "sleep 31"); + let expected = format!( + "{subject} failed to load: directory \".\", entry 2, field \"name\": expected a string or null" + ); + let events = s.drain(); + assert!( + events + .iter() + .any(|event| matches!(event, Event::Status(message) if message == &expected)), + "{events:?}" + ); + assert_eq!(std::fs::read(&file).unwrap(), text.as_bytes()); + } +} + /// Missing recipes report "not found". #[test] fn load_missing_session_reads_as_not_found() { From f491b5145a7b2a5b80199f87c3ae482d4fc81557 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 6 Sep 2026 15:19:48 -0700 Subject: [PATCH 3/5] refactor: enhance config_notify_route for improved TOML parsing and error handling --- src/harness/codex.rs | 177 +++++++++++++++++++++++++++++++------------ 1 file changed, 127 insertions(+), 50 deletions(-) diff --git a/src/harness/codex.rs b/src/harness/codex.rs index 03e3942..63fa5a1 100644 --- a/src/harness/codex.rs +++ b/src/harness/codex.rs @@ -79,42 +79,83 @@ enum NotifyRoute { Opaque, } -/// Classify the `notify` route declared in `config.toml`. The parser is -/// deliberately line-based: duplicate assignments are ambiguous and produce -/// [`NotifyRoute::Opaque`]. +/// Read bare top-level keys until the first table. Unsupported syntax disables +/// injection: it may contain a notifier that this reader cannot preserve. fn config_notify_route(home: Option<&Path>) -> NotifyRoute { let Some(root) = home_root(home, ".codex") else { return NotifyRoute::Vacant; }; - let text = fs::read_to_string(root.join("config.toml")).unwrap_or_default(); - let mut values = text.lines().filter_map(notify_value); - let Some(value) = values.next() else { - return NotifyRoute::Vacant; + let text = match fs::read_to_string(root.join("config.toml")) { + Ok(text) => text, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return NotifyRoute::Vacant, + Err(_) => return NotifyRoute::Opaque, }; - if values.next().is_some() { - return NotifyRoute::Opaque; - } - route_for(value) -} - -/// Classify one notify assignment for the newline-delimited chain transport. -/// Newlines collide with the delimiter, empty elements disappear during shell -/// field splitting, and an empty array names no program. Each case is opaque. -fn route_for(value: &str) -> NotifyRoute { - match parse_notify_array(value) { - Some(argv) - if !argv.is_empty() && argv.iter().all(|a| !a.is_empty() && !a.contains('\n')) => + let mut route = None; + for line in text.lines().map(str::trim) { + if line.is_empty() || line.starts_with('#') { + continue; + } + // TOML cannot return to the root table after a table header. Values + // above it must be complete so a header inside a string cannot stop us. + if line.starts_with('[') { + break; + } + let Some((key, value)) = line.split_once('=') else { + return NotifyRoute::Opaque; + }; + let key = key.trim(); + if key.is_empty() + || !key + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b"_-".contains(&b)) { - NotifyRoute::Chain(argv) + return NotifyRoute::Opaque; + } + if key == "notify" { + if route.is_some() { + return NotifyRoute::Opaque; + } + let Some(argv) = parse_notify_array(value) else { + return NotifyRoute::Opaque; + }; + if argv + .iter() + .any(|a| a.is_empty() || a.contains(['\n', '\0'])) + { + return NotifyRoute::Opaque; + } + route = Some(if argv.is_empty() { + NotifyRoute::Vacant + } else { + NotifyRoute::Chain(argv) + }); + } else if !complete_value(value.trim()) { + return NotifyRoute::Opaque; } - _ => NotifyRoute::Opaque, } + route.unwrap_or(NotifyRoute::Vacant) } -/// Value after `=` of an uncommented bare `notify` assignment, or `None`. -fn notify_value(line: &str) -> Option<&str> { - let rest = line.trim_start().strip_prefix("notify")?; - rest.trim_start_matches([' ', '\t']).strip_prefix('=') +/// Recognize complete single-line values without interpreting unrelated settings. +/// Multiline strings, arrays, and inline tables are outside the reader's scope. +fn complete_value(value: &str) -> bool { + if value.starts_with("\"\"\"") || value.starts_with("'''") { + return false; + } + let tail = if let Some(rest) = value.strip_prefix('"') { + parse_basic_string(rest).map(|(_, tail)| tail) + } else if let Some(rest) = value.strip_prefix('\'') { + rest.find('\'').map(|i| &rest[i + 1..]) + } else if value.starts_with('[') { + return parse_notify_array(value).is_some(); + } else { + let scalar = value.split('#').next().unwrap_or_default().trim(); + return matches!(scalar, "true" | "false") || scalar.parse::().is_ok(); + }; + tail.is_some_and(|tail| { + let tail = tail.trim(); + tail.is_empty() || tail.starts_with('#') + }) } /// Parse a one-line TOML array of basic strings. Literal strings, non-string @@ -307,7 +348,7 @@ mod tests { "# notify = [\"/my/thing\"]\n", " # notify = [\"/my/thing\"]\n", "notify_extra = 1\n", - "notify\n", + "notify = []\n", ] { fs::write(&cfg, inert).unwrap(); let plan = Codex.instrument(&inv, &paths(), Some(&home)); @@ -345,19 +386,18 @@ mod tests { let cfg = home.join("config.toml"); let inv = Codex.detect("codex").unwrap(); for opaque in [ - // Multi-line array: the value ends mid-structure. - "notify = [\n \"/my/thing\",\n]\n", - // Literal strings are unsupported. - "notify = ['/my/thing']\n", - // Empty array: notify is routed, yet no program to chain. - "notify = []\n", + // Malformed TOML cannot identify an active route. + "notify = [\n", + "notify\n", + "notify = [1]\n", + "notify = [\"a\\u0000b\"]\n", // Empty element: the script's field split would drop it. "notify = [\"\"]\n", // Embedded newline: the chain encoding's delimiter. "notify = [\"a\\nb\"]\n", // Not an array. "notify = \"/my/thing\"\n", - // Two assignment lines (e.g. one inside a table): ambiguous. + // Duplicate top-level assignments are invalid TOML. "notify = [\"/a\"]\nnotify = [\"/b\"]\n", ] { fs::write(&cfg, opaque).unwrap(); @@ -395,6 +435,58 @@ mod tests { ); } + /// Unknown syntax must not be mistaken for an absent notifier. + #[test] + fn config_notify_route_declines_unsupported_root_syntax() { + let home = temp("codex_unknown_notify"); + for text in [ + "\"notify\" = [\"/hook\"]", + "'notify' = ['/hook']", + "notify = ['/hook']", + "notify = [\n \"/hook\",\n]", + "description = '''\n[other]\n'''\nnotify = [\"/hook\"]", + "description = \"\"\"\nnotify = [\"/quoted\"]\n\"\"\"", + "other = [\n \"value\",\n]\nnotify = [\"/hook\"]", + "other = { value = 1 }\nnotify = [\"/hook\"]", + "other.key = true\nnotify = [\"/hook\"]", + ] { + fs::write(home.join("config.toml"), text).unwrap(); + assert_eq!( + config_notify_route(Some(&home)), + NotifyRoute::Opaque, + "{text:?}" + ); + } + } + + #[test] + fn config_notify_route_stops_at_tables_after_complete_values() { + let home = temp("codex_root_notify"); + let preamble = "model = \"example\" # comment\nname = 'literal'\nenabled = true\nlimit = 42\nother = [\"a\", \"b\"]\n"; + for (root, expected) in [ + ("", NotifyRoute::Vacant), + ("notify = []\n", NotifyRoute::Vacant), + ( + "notify = [\"/hook\"]\n", + NotifyRoute::Chain(vec!["/hook".into()]), + ), + ] { + fs::write( + home.join("config.toml"), + format!("{preamble}{root}[other]\nnotify = [\"/ignored\"]\n"), + ) + .unwrap(); + assert_eq!(config_notify_route(Some(&home)), expected); + } + } + + #[test] + fn config_notify_route_skips_unreadable_config() { + let home = temp("codex_unreadable_notify"); + fs::create_dir(home.join("config.toml")).unwrap(); + assert_eq!(config_notify_route(Some(&home)), NotifyRoute::Opaque); + } + #[test] fn parse_notify_array_decodes_escapes_and_structure() { assert_eq!( @@ -449,21 +541,6 @@ mod tests { } } - /// `route_for` rejects parsed arrays that the chain transport would alter. - #[test] - fn route_for_refuses_untransportable_argv() { - assert_eq!( - route_for(r#" ["/x", "y"]"#), - NotifyRoute::Chain(vec!["/x".into(), "y".into()]) - ); - // Newline elements collide with the join delimiter; empty elements - // are dropped by sh field splitting; an empty array has no program. - assert_eq!(route_for(r#"["a\nb"]"#), NotifyRoute::Opaque); - assert_eq!(route_for(r#"[""]"#), NotifyRoute::Opaque); - assert_eq!(route_for("[]"), NotifyRoute::Opaque); - assert_eq!(route_for("garbage"), NotifyRoute::Opaque); - } - #[test] fn parse_capture_accepts_only_turn_complete_payloads() { let payload = format!( @@ -497,7 +574,7 @@ mod tests { NotifyRoute::Chain(vec!["/base/hook".to_string()]) ); - // Two assignment lines remain ambiguous. + // Duplicate top-level assignments are invalid TOML. fs::write(&cfg, "notify = [\"/a\"]\nnotify = [\"/b\"]\n").unwrap(); assert_eq!(config_notify_route(Some(&home)), NotifyRoute::Opaque); } From d8cb35fc206141424c498569a717e5d2b729b355 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 6 Sep 2026 15:19:53 -0700 Subject: [PATCH 4/5] refactor: improve codex approval logic to handle cases without numbered siblings --- src/harness/summary.rs | 14 +++++++++++--- src/harness/summary_tests.rs | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/harness/summary.rs b/src/harness/summary.rs index 378c8c4..a7e7347 100644 --- a/src/harness/summary.rs +++ b/src/harness/summary.rs @@ -400,15 +400,23 @@ fn codex_numbered_option(row: &str) -> bool { } /// Codex's approval modal: a selector row with an indented numbered sibling -/// below it, pinned to the last nine painted rows. A quoted menu retains the +/// adjacent to it, pinned to the last nine painted rows. A quoted menu retains the /// live composer below it, so any non-selector [`CODEX_PROMPT`] row after the /// selector suppresses the match. Suppression tests the glyph alone because /// modal detection must not reinterpret a live composer as quoted content. fn codex_approval(rows: &[String]) -> Option<(String, &'static str)> { let last = rows.iter().rposition(|r| !r.is_empty())?; let i = (last.saturating_sub(8)..=last).find(|&i| codex_menu_head(&rows[i]))?; - let sibling = rows[i + 1..].iter().find(|r| !r.is_empty())?; - if !(sibling.starts_with(' ') && codex_numbered_option(sibling)) { + // The last option has no numbered sibling below it. + let siblings = [ + rows[..i].iter().rev().find(|r| !r.is_empty()), + rows[i + 1..].iter().find(|r| !r.is_empty()), + ]; + if !siblings + .into_iter() + .flatten() + .any(|r| r.starts_with(' ') && codex_numbered_option(r)) + { return None; } rows[i + 1..] diff --git a/src/harness/summary_tests.rs b/src/harness/summary_tests.rs index 921dea4..527c5d7 100644 --- a/src/harness/summary_tests.rs +++ b/src/harness/summary_tests.rs @@ -1014,6 +1014,23 @@ fn codex_approval_modal_synthesizes_on_any_selection() { CodexSummary.live_preview(&on_second), Some(("awaiting approval".to_string(), "codex:approval-menu")) ); + + let on_last = rs(&[ + " 1. Yes, proceed (y)", + " 2. Yes, and don't ask again (p)", + "› 3. No (esc)", + "", + " Press enter to confirm or esc to cancel", + ]); + assert_eq!( + CodexSummary.live_preview(&on_last), + Some(("awaiting approval".to_string(), "codex:approval-menu")) + ); + for glyph in CODEX_PROMPT { + let mut quoted = on_last.clone(); + quoted.push(glyph.to_string()); + assert_eq!(CodexSummary.live_preview("ed), None, "{glyph}"); + } } /// A menu quoted in the conversation always has the live composer From c5ad4c55168e7af8ee1c0adde4e170f504e3d0e3 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 6 Sep 2026 15:23:07 -0700 Subject: [PATCH 5/5] refactor: update notifier configuration handling for improved clarity and behavior --- docs/agent-resume.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/agent-resume.md b/docs/agent-resume.md index 17dcccb..eb13a89 100644 --- a/docs/agent-resume.md +++ b/docs/agent-resume.md @@ -68,7 +68,7 @@ Codex does not let the caller choose an ID at launch. Both accepted forms instea After each turn, the notifier writes the `agent-turn-complete` JSON argument to `FLEETCOM_CAPTURE_FILE`; the harness reads `thread-id`. This captures in-TUI session changes after the resumed conversation completes a turn. -Replacing a configured notifier would change user behavior. When the effective Codex configuration contains a one-line `notify` array of non-empty basic strings, the capture script executes that notifier after writing the capture file. Its argv is carried in `FLEETCOM_NOTIFY_CHAIN`, joined by newlines, and the notification payload is appended. An empty, multiline, ambiguous, or unsupported `notify` value disables the injected override so the configured route remains unchanged. The line-based configuration reader checks `config.toml` and the profile selected by its first `profile = ...` assignment; the profile's notify assignment takes precedence. +Replacing a configured notifier would change user behavior. `fleetcom` reads bare top-level keys in `$CODEX_HOME/config.toml` until the first table header. A one-line `notify` array of non-empty basic strings is chained after the capture write. Its argv is carried in `FLEETCOM_NOTIFY_CHAIN`, joined by newlines, and the notification payload is appended. An absent setting or empty array lets capture run alone; empty arguments, newlines, and NUL cannot be transported and disable injection. ### `grok`