diff --git a/src-tauri/src/commands/config.rs b/src-tauri/src/commands/config.rs index a6c88c224..3950b1356 100644 --- a/src-tauri/src/commands/config.rs +++ b/src-tauri/src/commands/config.rs @@ -2292,7 +2292,9 @@ fn scope_fault_code(fault: crate::config::replica_identity::ReplicaConfigFault) use crate::config::replica_identity::ReplicaConfigFault; match fault { ReplicaConfigFault::ConfigUnreadable => ScopeFaultCode::ConfigUnreadable, - ReplicaConfigFault::ConfigNotJson => ScopeFaultCode::ConfigNotJson, + ReplicaConfigFault::ConfigNotJson | ReplicaConfigFault::DuplicateJsonMember => { + ScopeFaultCode::ConfigNotJson + } ReplicaConfigFault::ConfigNotObject => ScopeFaultCode::ConfigNotObject, ReplicaConfigFault::IdentityMissing => ScopeFaultCode::IdentityMissing, ReplicaConfigFault::IdentityMismatch => ScopeFaultCode::IdentityMismatch, @@ -2554,6 +2556,12 @@ fn enumerate_profile_assignment_targets( &replica_dir, ) { Ok(read) => read, + Err(( + crate::config::replica_identity::ReplicaConfigFault::DuplicateJsonMember, + message, + )) => { + return Err(message); + } Err((fault, _message)) => { // #1941 - an unprovable identity inside the scope means membership // cannot be established: keep the diagnostic visible and refuse to @@ -13967,13 +13975,14 @@ mod tests { "dev-rust", locked_tooling("B", "agent-0"), ); - // The lenient anchor validation accepts a duplicate key; the strict - // reader rejects it, so the failure is the strict read's own. + // Duplicate admission rejects the anchor before the legacy strict reader. + let config_path = anchor.join("config.json"); std::fs::write( - anchor.join("config.json"), + &config_path, br#"{"identity":"../../_agent_dev-rust","tooling":{},"tooling":{}}"#, ) .expect("break anchor"); + let original = std::fs::read(&config_path).unwrap(); let settings = selection_api_settings(&fixture); let error = match super::enumerate_profile_assignment_targets( &settings, @@ -13984,7 +13993,12 @@ mod tests { Ok(_) => panic!("a strictly unparseable anchor must fail the enumeration"), Err(error) => error, }; - assert!(error.contains("Target replica '"), "{error}"); + assert!(error.contains("duplicateJsonMember:"), "{error}"); + assert!(error.contains(config_path.to_str().unwrap()), "{error}"); + assert!(error.contains("repeated JSON object members"), "{error}"); + assert!(!error.contains("tooling"), "{error}"); + assert!(!error.contains("../../_agent_dev-rust"), "{error}"); + assert_eq!(std::fs::read(&config_path).unwrap(), original); } #[test] diff --git a/src-tauri/src/config/agent_config.rs b/src-tauri/src/config/agent_config.rs index e590102db..4e4e1ccc5 100644 --- a/src-tauri/src/config/agent_config.rs +++ b/src-tauri/src/config/agent_config.rs @@ -270,13 +270,26 @@ fn activity_only_update(path: &Path, at_rfc3339: &str) -> Result { /// unparseable, or carries no `tooling.lastAgentMessageAt`. Reads the greatest /// landed valid state/legacy timestamp; invalid legacy-only data stays visible. pub fn read_last_agent_message_at(repo_path: &str) -> Option { + read_last_agent_message_at_checked(repo_path).ok().flatten() +} + +pub fn read_last_agent_message_at_checked(repo_path: &str) -> Result, String> { let dir = Path::new(repo_path).join(crate::config::agent_local_dir_name().as_str()); // Read only landed state and tracked legacy stamps. A pending activity // intent must never be advertised before its state publish. let mut maximum: Option<(chrono::DateTime, String)> = None; + let mut texts = Vec::new(); for path in [dir.join("config.json"), state_file_path(&dir)] { - let value = std::fs::read(&path) - .ok() + let bytes = std::fs::read(&path).ok(); + if let Some(bytes) = &bytes { + check_configuration_json(&path, bytes)?; + } + texts.push( + bytes + .as_ref() + .and_then(|bytes| String::from_utf8(bytes.clone()).ok()), + ); + let value = bytes .and_then(|bytes| serde_json::from_slice::(&bytes).ok()) .and_then(|value| { value @@ -293,9 +306,20 @@ pub fn read_last_agent_message_at(repo_path: &str) -> Option { } } } - maximum - .map(|(_, value)| value) - .or_else(|| read_agent_local_config(&dir).and_then(|cfg| cfg.tooling.last_agent_message_at)) + if let Some((_, value)) = maximum { + return Ok(Some(value)); + } + let mut texts = texts.into_iter(); + let decisions = texts.next().flatten(); + let state = texts.next().flatten(); + if decisions.as_ref().is_some_and(|text| !typed_ok(text)) { + return Ok(None); + } + let config = merge_config_texts(decisions, state.filter(|text| typed_ok(text))) + .ok() + .flatten() + .and_then(|value| serde_json::from_value::(value).ok()); + Ok(config.and_then(|cfg| cfg.tooling.last_agent_message_at)) } // ── State file loader (#2786, C1) ─────────────────────────────────────────── @@ -1029,15 +1053,22 @@ pub(crate) mod load_probe_harness { /// a duplicate known field is still a rejection and never collapses to the /// last value through `Value`. A state file that fails it is ignored. pub fn read_agent_local_config(dir: &Path) -> Option { + read_agent_local_config_checked(dir).ok().flatten() +} + +pub fn read_agent_local_config_checked(dir: &Path) -> Result, String> { #[cfg(test)] load_probe::record_load(dir); - let decisions = read_config_text(&dir.join("config.json")); + let decisions = read_config_text_checked(&dir.join("config.json"))?; + let state = read_config_text_checked(&state_file_path(dir))?; if decisions.as_ref().is_some_and(|text| !typed_ok(text)) { - return None; + return Ok(None); } - let state = read_config_text(&state_file_path(dir)).filter(|text| typed_ok(text)); - let value = merge_config_texts(decisions, state).ok()??; - serde_json::from_value(value).ok() + let state = state.filter(|text| typed_ok(text)); + let Some(value) = merge_config_texts(decisions, state).ok().flatten() else { + return Ok(None); + }; + Ok(serde_json::from_value(value).ok()) } /// #2786 (C1) - the raw JSON the loader serves: `config.json` in `dir` with @@ -1050,8 +1081,8 @@ pub fn read_agent_local_config_json(dir: &Path) -> Result std::path::PathBuf { +pub(super) fn state_file_path(dir: &Path) -> std::path::PathBuf { dir.join(crate::config::instance_artifacts::CONFIG_STATE_TARGET_NAME) } -fn read_config_text(path: &Path) -> Option { - std::fs::read_to_string(path).ok() +pub(super) fn check_configuration_json(path: &Path, bytes: &[u8]) -> Result<(), String> { + super::local_config_io::check_configuration_json(path, bytes) +} + +fn read_config_text_checked(path: &Path) -> Result, String> { + let Ok(bytes) = std::fs::read(path) else { + return Ok(None); + }; + check_configuration_json(path, &bytes)?; + Ok(String::from_utf8(bytes).ok()) } /// Merge the two texts. `config.json` that is not JSON is an error; a state @@ -1153,6 +1192,8 @@ fn overlay_state( // ── Config pair entry point (#2807, C2) ───────────────────────────────────── type JsonMap = serde_json::Map; +pub(super) use super::local_config_io::ConfigWriteError as AgentConfigWriteError; + /// #2807 (C2) - the one writer of an agent's config pair: `decisions` is the /// tracked `config.json`, and the state file is the one beside it. `mutate` /// gets `(decisions, state)`; a decision-only writer ignores the second map. @@ -1184,7 +1225,29 @@ pub(super) fn update_existing_agent_config_with_publish( where F: FnOnce(&mut JsonMap, &mut JsonMap) -> Result<(), String>, { - update_agent_config_with_activity_and_publish( + update_existing_agent_config_with_publish_classified(decisions, mutate, publish) + .map_err(|error| error.to_string()) +} + +pub(super) fn update_existing_agent_config_classified( + decisions: &Path, + mutate: F, +) -> Result<(), AgentConfigWriteError> +where + F: FnOnce(&mut JsonMap, &mut JsonMap) -> Result<(), String>, +{ + update_existing_agent_config_with_publish_classified(decisions, mutate, None) +} + +pub(super) fn update_existing_agent_config_with_publish_classified( + decisions: &Path, + mutate: F, + publish: Option>, +) -> Result<(), AgentConfigWriteError> +where + F: FnOnce(&mut JsonMap, &mut JsonMap) -> Result<(), String>, +{ + update_agent_config_with_activity_and_publish_classified( decisions, production_journal_dir().as_deref(), &|_: &str| {}, @@ -1303,6 +1366,28 @@ fn update_agent_config_with_activity_and_publish( activity: Option<(&str, &std::cell::Cell)>, publish: Option>, ) -> Result<(), String> +where + F: FnOnce(&mut JsonMap, &mut JsonMap) -> Result<(), String>, +{ + update_agent_config_with_activity_and_publish_classified( + decisions, + journal_dir, + on_stage, + mutate, + activity, + publish, + ) + .map_err(|error| error.to_string()) +} + +fn update_agent_config_with_activity_and_publish_classified( + decisions: &Path, + journal_dir: Option<&Path>, + on_stage: &dyn Fn(&str), + mutate: F, + activity: Option<(&str, &std::cell::Cell)>, + publish: Option>, +) -> Result<(), AgentConfigWriteError> where F: FnOnce(&mut JsonMap, &mut JsonMap) -> Result<(), String>, { @@ -1342,7 +1427,7 @@ where activity, ); match publish { - None => crate::config::local_config_io::update_config_pair_guarded( + None => crate::config::local_config_io::update_config_pair_guarded_classified( args.0, args.1, args.2, @@ -1354,7 +1439,7 @@ where args.7, args.8, ), - Some(publish) => crate::config::local_config_io::update_config_pair_inner( + Some(publish) => crate::config::local_config_io::update_config_pair_inner_classified( args.0, args.1, Some(args.2), @@ -1703,6 +1788,78 @@ fn upsert_config( #[cfg(test)] mod tests { + #[test] + fn e2992_duplicate_json_agent_readers_and_classified_writer_preserve_bytes() { + for side in 0..2 { + let temp = loader_fixture(Some(serde_json::json!({})), Some(serde_json::json!({}))); + let d = temp.path().join("config.json"); + let st = state_file_path(temp.path()); + let paths = [&d, &st]; + std::fs::write( + paths[side], + br#"{"unknown-canary":"value-canary","unknown-canary":0}"#, + ) + .unwrap(); + let originals = [std::fs::read(&d).unwrap(), std::fs::read(&st).unwrap()]; + for error in [ + read_agent_local_config_checked(temp.path()).unwrap_err(), + read_agent_local_config_json(temp.path()).unwrap_err(), + ] { + assert!(error.contains("duplicateJsonMember:")); + assert!(error.contains(paths[side].to_str().unwrap())); + assert!(!error.contains("canary")); + } + let called = std::cell::Cell::new(false); + let error = update_existing_agent_config_classified(&d, |_, _| { + called.set(true); + Ok(()) + }) + .unwrap_err(); + assert!( + matches!(&error, AgentConfigWriteError::DuplicateJsonMember(message) + if message.contains(paths[side].to_str().unwrap())) + ); + assert!(!called.get()); + assert_eq!(std::fs::read(&d).unwrap(), originals[0]); + assert_eq!(std::fs::read(&st).unwrap(), originals[1]); + } + for name in [ + "config.json", + crate::config::instance_artifacts::CONFIG_STATE_TARGET_NAME, + ] { + let repo = tempfile::tempdir().unwrap(); + let dir = repo + .path() + .join(crate::config::agent_local_dir_name().as_str()); + std::fs::create_dir(&dir).unwrap(); + let path = dir.join(name); + let bytes = br#"{"unknown-canary":0,"unknown-canary":"value-canary"}"#; + std::fs::write(&path, bytes).unwrap(); + let error = + read_last_agent_message_at_checked(repo.path().to_str().unwrap()).unwrap_err(); + assert!(error.contains(path.to_str().unwrap())); + assert!(!error.contains("canary")); + assert_eq!(std::fs::read(path).unwrap(), bytes); + } + let temp = loader_fixture(Some(serde_json::json!({})), None); + let error = + update_existing_agent_config_classified(&temp.path().join("config.json"), |_, _| { + Err("duplicateJsonMember: ordinary callback".to_string()) + }) + .unwrap_err(); + assert!(matches!(error, AgentConfigWriteError::Ordinary(_))); + std::fs::write(temp.path().join("config.json"), b"{").unwrap(); + assert!(read_agent_local_config_checked(temp.path()) + .unwrap() + .is_none()); + let native_error = serde_json::from_str::("{").unwrap_err(); + assert!(native_error.is_eof()); + assert_eq!( + read_agent_local_config_json(temp.path()).unwrap_err(), + native_error.to_string() + ); + } + use super::*; use std::path::PathBuf; @@ -2505,14 +2662,10 @@ mod tests { duplicate, ) .expect("seed state"); - assert_eq!( - read_agent_local_config(temp.path()) - .expect("config") - .tooling - .last_coding_agent - .as_deref(), - Some("tracked") - ); + assert!(read_agent_local_config_checked(temp.path()) + .unwrap_err() + .contains("duplicateJsonMember:")); + assert!(read_agent_local_config(temp.path()).is_none()); } /// #2786 C1 E7e: the raw wrapper filters the STATE file through the typed @@ -2526,14 +2679,16 @@ mod tests { .expect("some") }; - // (1) a tracked duplicate is last wins, with no state file. + // (1) a tracked duplicate is rejected before projection. let temp = loader_fixture(None, None); std::fs::write( temp.path().join("config.json"), r#"{"tooling":{"lastCodingAgent":"a","lastCodingAgent":"b"}}"#, ) .expect("seed decisions"); - assert_eq!(raw(temp.path())["tooling"]["lastCodingAgent"], "b"); + assert!(read_agent_local_config_json(temp.path()) + .unwrap_err() + .contains("duplicateJsonMember:")); // (2) an unrelated malformed tracked field is tolerated. let temp = loader_fixture( @@ -2561,7 +2716,13 @@ mod tests { ] { let temp = loader_fixture(Some(tracked.clone()), None); std::fs::write(temp.path().join(state_name), state).expect("seed state"); - assert_eq!(raw(temp.path())["tooling"], tracked["tooling"], "leg {leg}"); + if leg.starts_with('4') { + assert!(read_agent_local_config_json(temp.path()) + .unwrap_err() + .contains("duplicateJsonMember:")); + } else { + assert_eq!(raw(temp.path())["tooling"], tracked["tooling"], "leg {leg}"); + } } // (5) control: a well-formed state file for the same key does overlay. diff --git a/src-tauri/src/config/coding_agent_profiles.rs b/src-tauri/src/config/coding_agent_profiles.rs index beec975a7..95558d2fa 100644 --- a/src-tauri/src/config/coding_agent_profiles.rs +++ b/src-tauri/src/config/coding_agent_profiles.rs @@ -46,13 +46,53 @@ pub struct ProfileSelectionResolution { pub resolution: ProfileResolution, } -fn read_json_object(path: &Path) -> Option { - let content = std::fs::read_to_string(path).ok()?; - serde_json::from_str::(&content).ok() +fn read_json_object_checked(path: &Path) -> Result, String> { + let Ok(bytes) = std::fs::read(path) else { + return Ok(None); + }; + crate::config::agent_config::check_configuration_json(path, &bytes)?; + Ok(serde_json::from_slice::(&bytes).ok()) } /// #2786 (C1) - through the shared loader, so a state key such as /// `profileContentHash` is served from the state file first. +fn read_tooling_config_checked(agent_dir: &Path) -> Result, String> { + let read = |path: &Path| -> Result, String> { + let Ok(bytes) = std::fs::read(path) else { + return Ok(None); + }; + crate::config::agent_config::check_configuration_json(path, &bytes)?; + Ok(String::from_utf8(bytes).ok()) + }; + let decisions = read(&agent_dir.join("config.json"))?; + let state = read(&crate::config::agent_config::state_file_path(agent_dir))?; + let decisions = match decisions { + Some(text) => match serde_json::from_str::(&text) { + Ok(value) => Some(value), + Err(_) => return Ok(None), + }, + None => None, + }; + let state = state.and_then(|text| { + serde_json::from_str::(&text).ok()?; + serde_json::from_str::(&text).ok() + }); + if decisions.is_none() && state.is_none() { + return Ok(None); + } + Ok(Some( + crate::config::agent_config::overlay_agent_local_state( + decisions.unwrap_or_else(|| serde_json::json!({})), + state, + ), + )) +} + +fn read_tooling_string_checked(agent_dir: &Path, key: &str) -> Result, String> { + Ok(read_tooling_config_checked(agent_dir)? + .and_then(|value| value.get("tooling")?.get(key)?.as_str().map(str::to_string))) +} + fn read_tooling_string(agent_dir: &Path, key: &str) -> Option { crate::config::agent_config::read_agent_local_config_json(agent_dir) .ok()?? @@ -235,46 +275,69 @@ fn validate_authoritative_matrix_dir(matrix_dir: &Path) -> Result Result<(PathBuf, PathBuf), String> { - let replica_dir = canonical_existing_dir(replica_dir, "Room replica")?; + validated_replica_origin_classified(replica_dir).map_err(|(_, message)| message) +} + +fn validated_replica_origin_classified( + replica_dir: &Path, +) -> Result<(PathBuf, PathBuf), (crate::config::replica_identity::ReplicaConfigFault, String)> { + let ordinary = |message| { + ( + crate::config::replica_identity::ReplicaConfigFault::LocationInvalid, + message, + ) + }; + let replica_dir = canonical_existing_dir(replica_dir, "Room replica").map_err(ordinary)?; let dir_name = replica_dir .file_name() .and_then(|name| name.to_str()) - .ok_or_else(|| format!("Room replica '{}' has no valid name", replica_dir.display()))?; + .ok_or_else(|| format!("Room replica '{}' has no valid name", replica_dir.display())) + .map_err(ordinary)?; if !dir_name.starts_with("__agent_") { - return Err(format!( + return Err(ordinary(format!( "Room replica '{}' must be named '__agent_'", replica_dir.display() - )); + ))); } - let wg_dir = replica_dir.parent().ok_or_else(|| { - format!( - "Room replica '{}' has no parent room", - replica_dir.display() - ) - })?; + let wg_dir = replica_dir + .parent() + .ok_or_else(|| { + format!( + "Room replica '{}' has no parent room", + replica_dir.display() + ) + }) + .map_err(ordinary)?; let wg_name = wg_dir .file_name() .and_then(|name| name.to_str()) .unwrap_or(""); if !crate::config::entity_prefix::has_entity_prefix(wg_name) { - return Err(format!( + return Err(ordinary(format!( "Room replica '{}' is not inside a `room-*` or legacy `wg-*` Room directory", replica_dir.display() - )); + ))); } - let persisted_identity = - read_json_object(&replica_dir.join("config.json")).and_then(|config| { + let persisted_identity = read_json_object_checked(&replica_dir.join("config.json")) + .map_err(|message| { + ( + crate::config::replica_identity::ReplicaConfigFault::DuplicateJsonMember, + message, + ) + })? + .and_then(|config| { config .get("identity") .and_then(|value| value.as_str()) .map(str::to_string) }); - let identity = crate::config::replica_identity::validate_or_repair_wg_replica_identity( - &replica_dir, - persisted_identity.as_deref(), - )?; - let origin = validate_authoritative_matrix_dir(&identity.matrix_dir)?; + let identity = + crate::config::replica_identity::validate_or_repair_wg_replica_identity_classified( + &replica_dir, + persisted_identity.as_deref(), + )?; + let origin = validate_authoritative_matrix_dir(&identity.matrix_dir).map_err(ordinary)?; Ok((replica_dir, origin)) } @@ -331,19 +394,32 @@ pub(crate) fn agent_name_from_dir(path: &Path) -> Option { } fn origin_matrix_dir_for_launch_path(launch_path: &Path) -> Result, String> { + origin_matrix_dir_for_launch_path_classified(launch_path).map_err(|(_, message)| message) +} + +fn origin_matrix_dir_for_launch_path_classified( + launch_path: &Path, +) -> Result, (crate::config::replica_identity::ReplicaConfigFault, String)> { let Some(dir_name) = launch_path.file_name().and_then(|name| name.to_str()) else { return Ok(None); }; if dir_name.starts_with("_agent_") { - return validate_authoritative_matrix_dir(launch_path).map(Some); + return validate_authoritative_matrix_dir(launch_path) + .map(Some) + .map_err(|message| { + ( + crate::config::replica_identity::ReplicaConfigFault::LocationInvalid, + message, + ) + }); } if !dir_name.starts_with("__agent_") { return Ok(None); } - validated_replica_origin(launch_path).map(|(_, origin)| Some(origin)) + validated_replica_origin_classified(launch_path).map(|(_, origin)| Some(origin)) } fn normalize_profile_from_source( @@ -373,8 +449,26 @@ pub struct ReplicaProfileRead { } pub fn read_replica_profile_result(launch_path: &Path) -> ReplicaProfileRead { - let v2 = read_tooling_string(launch_path, "profile"); - let legacy = read_tooling_string(launch_path, "instanceProfileOverride"); + read_replica_profile_result_checked(launch_path).unwrap_or(ReplicaProfileRead { + profile: None, + warning: None, + }) +} + +pub fn read_replica_profile_result_checked( + launch_path: &Path, +) -> Result { + let config = read_tooling_config_checked(launch_path)?; + let field = |key: &str| { + config + .as_ref()? + .get("tooling")? + .get(key)? + .as_str() + .map(str::to_string) + }; + let v2 = field("profile"); + let legacy = field("instanceProfileOverride"); let v2_normalized = v2.as_deref().and_then(normalize_profile_letter); let legacy_normalized = legacy.as_deref().and_then(normalize_profile_letter); let warning = match (&v2_normalized, &legacy_normalized) { @@ -386,10 +480,26 @@ pub fn read_replica_profile_result(launch_path: &Path) -> ReplicaProfileRead { )), _ => None, }; - ReplicaProfileRead { + Ok(ReplicaProfileRead { profile: v2_normalized.or(legacy_normalized), warning, - } + }) +} + +pub fn read_replica_profile_checked(launch_path: &Path) -> Result, String> { + Ok(read_replica_profile_result_checked(launch_path)?.profile) +} + +pub fn read_replica_current_coding_agent_checked( + launch_path: &Path, +) -> Result, String> { + read_tooling_string_checked(launch_path, "currentCodingAgent") +} + +pub fn read_replica_profile_content_hash_checked( + launch_path: &Path, +) -> Result, String> { + read_tooling_string_checked(launch_path, "profileContentHash") } pub fn read_replica_profile(launch_path: &Path) -> Option { @@ -869,6 +979,21 @@ pub(crate) fn selection_state_from_value(config: &Value, identity: &str) -> Repl /// #1939 - strict read-only view of `replica_dir`'s stored selection. Uses the /// existing strict reader (bounded path-safe read, duplicate-free JSON, object /// and identity validation); every strict-read failure becomes `Invalid`. +pub fn read_replica_selection_state_checked( + replica_dir: &Path, +) -> Result { + use crate::config::replica_identity::{ + read_wg_replica_config_read_only_classified, ReplicaConfigFault, + }; + match read_wg_replica_config_read_only_classified(replica_dir) { + Ok((config, identity)) => Ok(selection_state_from_value(&config, &identity.identity)), + Err((ReplicaConfigFault::DuplicateJsonMember, message)) => Err(message), + Err((_, error)) => Ok(ReplicaSelectionState::Invalid { + diagnostic: format!("{}: {}", replica_dir.display(), error), + }), + } +} + pub fn read_replica_selection_state(replica_dir: &Path) -> ReplicaSelectionState { match crate::config::replica_identity::read_wg_replica_config_read_only(replica_dir) { Ok((config, identity)) => selection_state_from_value(&config, &identity.identity), @@ -988,6 +1113,7 @@ pub fn read_replica_selection_default( )) } }; + crate::config::agent_config::check_configuration_json(&config_path, &bytes)?; let config: Value = serde_json::from_slice(&bytes).map_err(|error| { format!( "Matrix config {} is not valid JSON: {}", @@ -1320,8 +1446,8 @@ pub(crate) fn read_source_selection_state( > { use crate::config::settings::{IdentityReferenceError, SourceSelectionState}; let target = read_source_selection_target(settings, agent_path)?; + let raw = identity_reference_value(&target.decisions, &target.physical_revision[0])?; let physical_revision = target.physical_revision; - let raw = identity_reference_value(&physical_revision[0])?; let tooling = raw.get("tooling"); if tooling.is_some_and(|value| !value.is_object()) { return Err(IdentityReferenceError::Conflict); @@ -1356,17 +1482,21 @@ pub(crate) fn read_source_selection_state( } fn identity_reference_value( + path: &Path, physical: &crate::config::settings::IdentityReferencePhysicalState, ) -> Result { use crate::config::settings::{IdentityReferenceError, IdentityReferencePhysicalState}; match physical { IdentityReferencePhysicalState::Absent => Ok(serde_json::json!({})), IdentityReferencePhysicalState::Bytes { bytes, .. } => { + if &IdentityReferencePhysicalState::from_bytes(bytes.clone()) != physical { + return Err(IdentityReferenceError::Conflict); + } + crate::config::agent_config::check_configuration_json(path, bytes) + .map_err(IdentityReferenceError::DuplicateJsonMember)?; let value: Value = serde_json::from_slice(bytes).map_err(|_| IdentityReferenceError::Conflict)?; - if !value.is_object() - || &IdentityReferencePhysicalState::from_bytes(bytes.clone()) != physical - { + if !value.is_object() { return Err(IdentityReferenceError::Conflict); } Ok(value) @@ -1457,13 +1587,14 @@ fn patch_identity_companion( #[allow(dead_code)] pub(crate) fn plan_identity_reference_patch( before: [crate::config::settings::IdentityReferencePhysicalState; 2], + paths: [&Path; 2], source: &crate::config::settings::SourceRef, mappings: &[crate::config::settings::SourceIdentityMapping], ) -> Result { use crate::config::settings::IdentityReferenceError; let mut after = [ - identity_reference_value(&before[0])?, - identity_reference_value(&before[1])?, + identity_reference_value(paths[0], &before[0])?, + identity_reference_value(paths[1], &before[1])?, ]; for (side_index, side) in after.iter_mut().enumerate() { let Some(tooling) = side.get_mut("tooling") else { @@ -1521,15 +1652,18 @@ pub(crate) fn prepare_identity_reference_patch( crate::config::settings::IdentityReferencePacket, crate::config::settings::IdentityReferenceError, > { - use crate::config::settings::IdentityReferencePhysicalState; + use crate::config::settings::{IdentityReferenceError, IdentityReferencePhysicalState}; + let state_path = crate::config::agent_config::state_file_path(decisions.parent().ok_or( + IdentityReferenceError::InvalidPlan("PAIR target has no parent"), + )?); crate::config::agent_config::prepare_and_reserve_identity_reference_pair_guarded( decisions, request, - |decisions, state| { + |decisions_map, state_map| { // Cleanup has already run under the same PAIR locks. Patch only // companions in these maps; never overwrite the loader's cleanup // with snapshots taken before lock acquisition. - let physical = [decisions.clone(), state.clone()].map(|map| { + let physical = [decisions_map.clone(), state_map.clone()].map(|map| { serde_json::to_vec(&map).map(IdentityReferencePhysicalState::from_bytes) }); let [decisions_before, state_before] = physical; @@ -1538,16 +1672,17 @@ pub(crate) fn prepare_identity_reference_patch( decisions_before.map_err(|e| e.to_string())?, state_before.map_err(|e| e.to_string())?, ], + [decisions, state_path.as_path()], source, mappings, ) .map_err(|e| e.to_string())?; let [decisions_after, state_after] = patch.after; - *decisions = decisions_after + *decisions_map = decisions_after .as_object() .ok_or("invalid decisions object")? .clone(); - *state = state_after + *state_map = state_after .as_object() .ok_or("invalid state object")? .clone(); @@ -2110,10 +2245,14 @@ pub fn resolve_profile_selection( .as_ref() .map(|validated| validated.origin_matrix_dir.as_path()); let agent_name = launch_path.and_then(agent_name_from_dir); - let instance_read = launch_path.map(read_replica_profile_result); + let instance_read = launch_path + .map(read_replica_profile_result_checked) + .transpose()?; let instance_profile_override = instance_read.as_ref().and_then(|read| read.profile.clone()); let origin_default_profile = origin_matrix_dir - .and_then(|path| read_tooling_string(path, "defaultProfile")) + .map(|path| read_tooling_string_checked(path, "defaultProfile")) + .transpose()? + .flatten() .and_then(|value| normalize_profile_letter(&value)); let agent_default_profile = agent_name .as_ref() @@ -2125,7 +2264,7 @@ pub fn resolve_profile_selection( }) .and_then(|value| normalize_profile_letter(value)); let requested_profile_input = requested_profile.map(str::to_string); - let mut resolution = resolve_profile( + let mut resolution = resolve_profile_checked( settings, ProfileResolutionRequest { coding_agent_id, @@ -2134,7 +2273,7 @@ pub fn resolve_profile_selection( requested_profile, requested_profile_authoritative: false, }, - ); + )?; if let Some(warning) = instance_read.and_then(|read| read.warning) { resolution.warnings.push(warning); } @@ -2268,8 +2407,188 @@ pub fn resolve_profile( } } +pub fn resolve_profile_checked( + settings: &AppSettings, + request: ProfileResolutionRequest<'_>, +) -> Result { + let mut warnings = Vec::new(); + + let launch_path = request.launch_path; + let agent_name = request + .agent_matrix_name + .map(str::to_string) + .or_else(|| launch_path.and_then(agent_name_from_dir)); + + let instance_override = match launch_path { + Some(path) => { + let read = read_replica_profile_result_checked(path)?; + if let Some(warning) = read.warning { + warnings.push(warning); + } + normalize_profile_from_source(read.profile, &mut warnings, "instance override") + } + None => None, + }; + let origin_default = match launch_path + .map(origin_matrix_dir_for_launch_path_classified) + .transpose() + { + Ok(Some(Some(origin))) => normalize_profile_from_source( + read_tooling_string_checked(&origin, "defaultProfile")?, + &mut warnings, + "origin default", + ), + Ok(_) => None, + Err(( + crate::config::replica_identity::ReplicaConfigFault::DuplicateJsonMember, + message, + )) => return Err(message), + Err((_, message)) => { + warnings.push(format!("Ignoring origin default profile: {}", message)); + None + } + }; + + let explicit = request.requested_profile.and_then(|letter| { + normalize_profile_from_source(Some(letter.to_string()), &mut warnings, "launch request") + }); + + let agent_default = agent_name + .as_ref() + .and_then(|name| { + settings + .coding_agent_profiles + .default_profile_by_agent + .get(name) + }) + .and_then(|letter| { + normalize_profile_from_source(Some(letter.clone()), &mut warnings, "agent default") + }); + + // #1635-2 D2: a wake dispatch request outranks the replica pin for that + // spawn only; every other caller keeps the historical ranking (instance + // override wins over the explicit request). + let requested_profile = if request.requested_profile_authoritative { + explicit + .or(instance_override) + .or(origin_default) + .or(agent_default) + .unwrap_or_else(|| "A".to_string()) + } else { + instance_override + .or(explicit) + .or(origin_default) + .or(agent_default) + .unwrap_or_else(|| "A".to_string()) + }; + + let mut fallback_chain = Vec::new(); + let mut effective_profile = "A".to_string(); + let mut effective_cell = empty_profile_cell(); + + for letter in fallback_letters_from(&requested_profile) { + fallback_chain.push(letter.clone()); + let cell = if letter == "A" { + cell_for_letter(settings, request.coding_agent_id, &letter) + .unwrap_or_else(empty_profile_cell) + } else if let Some(cell) = cell_for_letter(settings, request.coding_agent_id, &letter) { + cell + } else { + continue; + }; + effective_profile = letter; + effective_cell = cell; + break; + } + + let fallback_applied = effective_profile != requested_profile; + Ok(ProfileResolution { + requested_profile, + effective_profile, + fallback_chain, + fallback_applied, + cell: effective_cell, + warnings, + }) +} + #[cfg(test)] mod tests { + #[test] + fn e2992_duplicate_json_checked_profiles_reject_before_explicit_fallback() { + for origin in [false, true] { + for side in 0..2 { + let fx = selection_fixture(); + write_config(&fx.replica, &seed_selection_config("{}")); + write_config(&fx.matrix, "{}"); + let dir = if origin { &fx.matrix } else { &fx.replica }; + let d = dir.join("config.json"); + let st = crate::config::agent_config::state_file_path(dir); + std::fs::write(&st, b"{}").unwrap(); + let paths = [&d, &st]; + std::fs::write( + paths[side], + br#"{"unknown-canary":"value-canary","unknown-canary":0}"#, + ) + .unwrap(); + let before = [std::fs::read(&d).unwrap(), std::fs::read(&st).unwrap()]; + let request = ProfileResolutionRequest { + coding_agent_id: "codex", + launch_path: Some(&fx.replica), + agent_matrix_name: None, + requested_profile: Some("A"), + requested_profile_authoritative: true, + }; + for (resolver, error) in [ + ( + "resolve_profile_checked", + resolve_profile_checked(&fx.settings, request).unwrap_err(), + ), + ( + "resolve_profile_selection", + resolve_profile_selection( + &fx.settings, + Some(&fx.replica), + "codex", + Some("A"), + ) + .unwrap_err(), + ), + ] { + assert!(error.contains("duplicateJsonMember:")); + assert!(error.contains(paths[side].to_str().unwrap()), + "origin={origin} side={side} resolver={resolver} expected_physical_file={:?} returned_error={error:?} expected_canonical={:?} expected_metadata={:?}", + paths[side], std::fs::canonicalize(paths[side]), std::fs::symlink_metadata(paths[side])); + assert!(!error.contains("canary")); + } + if !origin && side == 0 { + assert!(matches!(validated_replica_origin_classified(&fx.replica), + Err((crate::config::replica_identity::ReplicaConfigFault::DuplicateJsonMember, _)))); + } + assert_eq!(std::fs::read(&d).unwrap(), before[0]); + assert_eq!(std::fs::read(&st).unwrap(), before[1]); + } + } + let fx = selection_fixture(); + write_config(&fx.replica, &seed_selection_config("{}")); + let request = || ProfileResolutionRequest { + coding_agent_id: "codex", + launch_path: Some(&fx.replica), + agent_matrix_name: None, + requested_profile: Some("B"), + requested_profile_authoritative: false, + }; + assert_eq!( + resolve_profile_checked(&fx.settings, request()).unwrap(), + resolve_profile(&fx.settings, request()) + ); + write_config(&fx.replica, "{"); + assert_eq!( + resolve_profile_checked(&fx.settings, request()).unwrap(), + resolve_profile(&fx.settings, request()) + ); + } + use super::*; use crate::config::settings::{AgentConfig, ProfileCellConfig, ProfileSlotConfig}; use std::collections::BTreeMap; @@ -4713,10 +5032,76 @@ mod transition_reference_tests { ) } + #[test] + fn e2992_duplicate_json_identity_reference_paths_and_original_slots() { + for side in 0..2 { + let temp = tempfile::tempdir().unwrap(); + let d = temp.path().join("config.json"); + let st = temp.path().join("config.state.no-git.json"); + let paths = [d.as_path(), st.as_path()]; + let (source, mut before, mappings) = fixture(); + before[side] = IdentityReferencePhysicalState::from_bytes( + br#"{"unknown-canary":"value-canary","unknown-canary":0}"#.to_vec(), + ); + for index in 0..2 { + if let IdentityReferencePhysicalState::Bytes { bytes, .. } = &before[index] { + std::fs::write(paths[index], bytes).unwrap(); + } + } + let target = SourceSelectionTarget { + root: temp.path().to_path_buf(), + decisions: d.clone(), + state: st.clone(), + reservation: temp + .path() + .join("config-identity-reservation.state.no-git.json"), + physical_revision: before.clone(), + }; + let request = crate::config::settings::IdentityReferenceRequest { + operation_id: "duplicate-admission".into(), + source_physical_key: "source-key".into(), + mappings_digest: "a".repeat(64), + before_protected: [ + crate::config::settings::IdentityReferenceProtectedRevision::Absent, + crate::config::settings::IdentityReferenceProtectedRevision::Absent, + ], + }; + let prepared = + prepare_identity_reference_patch(&d, &request, &source, &mappings, &|| Ok(())) + .unwrap_err(); + assert!(!target.reservation.exists()); + let errors = [ + prepared, + identity_reference_value(paths[side], &before[side]).unwrap_err(), + plan_identity_reference_patch(before.clone(), paths, &source, &mappings) + .unwrap_err(), + source_original_slots(&target).unwrap_err(), + source_target_is_affected(&target, &source, &mappings).unwrap_err(), + ]; + for error in errors { + assert!( + matches!(&error, crate::config::settings::IdentityReferenceError::DuplicateJsonMember(message) if message.contains("duplicateJsonMember:") && message.contains(paths[side].to_str().unwrap()) && message.contains("repeated JSON object members")) + ); + assert!(!error.to_string().contains("canary")); + } + for index in 0..2 { + if let IdentityReferencePhysicalState::Bytes { bytes, .. } = &before[index] { + assert_eq!(std::fs::read(paths[index]).unwrap(), *bytes); + } + } + assert_eq!(target.physical_revision, before); + } + } + #[test] fn transition_reference_batch_two_cid_pid_only_default_lock_history_unknown() { + let temp = tempfile::tempdir().unwrap(); + let decisions_path = temp.path().join("config.json"); + let state_path = temp.path().join("config.state.no-git.json"); + let paths = [decisions_path.as_path(), state_path.as_path()]; let (source, before, mappings) = fixture(); - let patch = plan_identity_reference_patch(before.clone(), &source, &mappings).unwrap(); + let patch = + plan_identity_reference_patch(before.clone(), paths, &source, &mappings).unwrap(); assert_eq!(patch.before, before); assert_eq!( patch.after[0].pointer("/tooling/configurationRef/calculatedId"), @@ -4736,8 +5121,8 @@ mod transition_reference_tests { Some(&serde_json::json!("cid1:new-a")) ); let mut expected = [ - identity_reference_value(&before[0]).unwrap(), - identity_reference_value(&before[1]).unwrap(), + identity_reference_value(paths[0], &before[0]).unwrap(), + identity_reference_value(paths[1], &before[1]).unwrap(), ]; for (side, pointer, cid, pid) in [ (0, "/tooling/configurationRef", "cid1:new-a", "pid1:new-a"), @@ -4773,6 +5158,10 @@ mod transition_reference_tests { #[test] fn transition_reference_disabled_removed_and_other_source_preserve_every_field() { + let temp = tempfile::tempdir().unwrap(); + let decisions_path = temp.path().join("config.json"); + let state_path = temp.path().join("config.state.no-git.json"); + let paths = [decisions_path.as_path(), state_path.as_path()]; let (source, before, mut mappings) = fixture(); mappings[0].unavailable = true; mappings[1].new_calculated_id = None; @@ -4780,11 +5169,11 @@ mod transition_reference_tests { .profiles .insert("B".into(), (Some("pid1:old-c".into()), None)); let original = [ - identity_reference_value(&before[0]).unwrap(), - identity_reference_value(&before[1]).unwrap(), + identity_reference_value(paths[0], &before[0]).unwrap(), + identity_reference_value(paths[1], &before[1]).unwrap(), ]; assert_eq!( - plan_identity_reference_patch(before.clone(), &source, &mappings) + plan_identity_reference_patch(before.clone(), paths, &source, &mappings) .unwrap() .after, original @@ -4792,7 +5181,7 @@ mod transition_reference_tests { let mut other = source; other.context_root = "other-instance".into(); assert_eq!( - plan_identity_reference_patch(before, &other, &mappings) + plan_identity_reference_patch(before, paths, &other, &mappings) .unwrap() .after, original @@ -4801,6 +5190,10 @@ mod transition_reference_tests { #[test] fn transition_reference_stale_profile_conflict_leaves_input_physical_unchanged() { + let temp = tempfile::tempdir().unwrap(); + let decisions_path = temp.path().join("config.json"); + let state_path = temp.path().join("config.state.no-git.json"); + let paths = [decisions_path.as_path(), state_path.as_path()]; let (source, before, mut mappings) = fixture(); let original = before.clone(); mappings[2].profiles.insert( @@ -4808,7 +5201,7 @@ mod transition_reference_tests { (Some("pid1:stale".into()), Some("pid1:new-c".into())), ); assert!(matches!( - plan_identity_reference_patch(before.clone(), &source, &mappings), + plan_identity_reference_patch(before.clone(), paths, &source, &mappings), Err(crate::config::settings::IdentityReferenceError::Conflict) )); assert_eq!(before, original); @@ -4816,13 +5209,17 @@ mod transition_reference_tests { #[test] fn transition_reference_metadata_only_and_absent_sides_remain_unchanged() { + let temp = tempfile::tempdir().unwrap(); + let decisions_path = temp.path().join("config.json"); + let state_path = temp.path().join("config.state.no-git.json"); + let paths = [decisions_path.as_path(), state_path.as_path()]; let (source, before, _) = fixture(); let original = [ - identity_reference_value(&before[0]).unwrap(), - identity_reference_value(&before[1]).unwrap(), + identity_reference_value(paths[0], &before[0]).unwrap(), + identity_reference_value(paths[1], &before[1]).unwrap(), ]; assert_eq!( - plan_identity_reference_patch(before, &source, &[]) + plan_identity_reference_patch(before, paths, &source, &[]) .unwrap() .after, original @@ -4831,7 +5228,7 @@ mod transition_reference_tests { IdentityReferencePhysicalState::Absent, IdentityReferencePhysicalState::Absent, ]; - let patch = plan_identity_reference_patch(absent.clone(), &source, &[]).unwrap(); + let patch = plan_identity_reference_patch(absent.clone(), paths, &source, &[]).unwrap(); assert_eq!(patch.before, absent); assert_eq!(patch.after, [serde_json::json!({}), serde_json::json!({})]); } @@ -4864,8 +5261,8 @@ pub(crate) fn source_original_slots( } Ok(()) }; - let decisions = identity_reference_value(&target.physical_revision[0])?; - let state = identity_reference_value(&target.physical_revision[1])?; + let decisions = identity_reference_value(&target.decisions, &target.physical_revision[0])?; + let state = identity_reference_value(&target.state, &target.physical_revision[1])?; if let Some(tooling) = decisions.get("tooling") { capture(SourceOriginalSlot::Decision, tooling)?; if let Some(default) = tooling.get("replicaSelectionDefault") { @@ -4914,10 +5311,15 @@ pub(crate) fn source_target_is_affected( source: &crate::config::settings::SourceRef, mappings: &[crate::config::settings::SourceIdentityMapping], ) -> Result { - let patch = plan_identity_reference_patch(target.physical_revision.clone(), source, mappings)?; + let patch = plan_identity_reference_patch( + target.physical_revision.clone(), + [&target.decisions, &target.state], + source, + mappings, + )?; let original = [ - identity_reference_value(&target.physical_revision[0])?, - identity_reference_value(&target.physical_revision[1])?, + identity_reference_value(&target.decisions, &target.physical_revision[0])?, + identity_reference_value(&target.state, &target.physical_revision[1])?, ]; Ok(patch.after != original) } diff --git a/src-tauri/src/config/local_config_io.rs b/src-tauri/src/config/local_config_io.rs index a35b583db..249b9e046 100644 --- a/src-tauri/src/config/local_config_io.rs +++ b/src-tauri/src/config/local_config_io.rs @@ -15,6 +15,10 @@ fn local_config_write_lock() -> &'static Mutex<()> { /// the data lives on disk under the file sidecar and every reader re-reads it. /// So a panic inside one mutate closure must not block every later config /// write in the process until restart, which `map_err` on poison did. +pub(super) fn check_configuration_json(path: &Path, bytes: &[u8]) -> Result<(), String> { + crate::path_identity::validate_ac_configuration_json(path, bytes) +} + fn lock_local_config_writes() -> MutexGuard<'static, ()> { local_config_write_lock() .lock() @@ -262,6 +266,7 @@ where let mut root = if path.exists() { let content = std::fs::read_to_string(path) .map_err(|e| format!("Failed to read {}: {}", path.display(), e))?; + check_configuration_json(path, content.as_bytes())?; let parsed: Value = serde_json::from_str(&content) .map_err(|e| format!("Failed to parse {}: {}", path.display(), e))?; if !parsed.is_object() { @@ -415,6 +420,7 @@ where /// The production PAIR boundary. Pending deltas are computed on private maps /// before cleanup or the caller's split-marker policy can change either file. #[allow(clippy::too_many_arguments)] +#[allow(dead_code)] // P52: retain legacy wrapper during checked migration. pub(crate) fn update_config_pair_guarded( decisions: &Path, state: &Path, @@ -430,7 +436,38 @@ pub(crate) fn update_config_pair_guarded( where F: FnOnce(&mut Map, &mut Map) -> Result<(), String>, { - update_config_pair_inner( + update_config_pair_guarded_classified( + decisions, + state, + reservation, + state_keys, + nonpending_preflight, + cleanup, + on_stage, + mutate, + finish, + activity, + ) + .map_err(|error| error.to_string()) +} + +#[allow(clippy::too_many_arguments)] +pub(super) fn update_config_pair_guarded_classified( + decisions: &Path, + state: &Path, + reservation: &Path, + state_keys: &[&str], + nonpending_preflight: Option<&dyn Fn() -> Result<(), String>>, + cleanup: Option>, + on_stage: &dyn Fn(&str), + mutate: F, + finish: ConfigPairFinish<'_>, + activity: Option<(&str, &Cell)>, +) -> Result<(), ConfigWriteError> +where + F: FnOnce(&mut Map, &mut Map) -> Result<(), String>, +{ + update_config_pair_inner_classified( decisions, state, Some(reservation), @@ -446,6 +483,7 @@ where } #[allow(clippy::too_many_arguments)] +#[allow(dead_code)] // P52: retain legacy wrapper during checked migration. pub(super) fn update_config_pair_inner( decisions: &Path, state: &Path, @@ -459,6 +497,39 @@ pub(super) fn update_config_pair_inner( nonpending_preflight: Option<&dyn Fn() -> Result<(), String>>, publish: ConfigPublisher<'_>, ) -> Result<(), String> +where + F: FnOnce(&mut Map, &mut Map) -> Result<(), String>, +{ + update_config_pair_inner_classified( + decisions, + state, + reservation, + state_keys, + cleanup, + on_stage, + mutate, + finish, + activity, + nonpending_preflight, + publish, + ) + .map_err(|error| error.to_string()) +} + +#[allow(clippy::too_many_arguments)] +pub(super) fn update_config_pair_inner_classified( + decisions: &Path, + state: &Path, + reservation: Option<&Path>, + state_keys: &[&str], + cleanup: Option>, + on_stage: &dyn Fn(&str), + mutate: F, + finish: ConfigPairFinish<'_>, + activity: Option<(&str, &Cell)>, + nonpending_preflight: Option<&dyn Fn() -> Result<(), String>>, + publish: ConfigPublisher<'_>, +) -> Result<(), ConfigWriteError> where F: FnOnce(&mut Map, &mut Map) -> Result<(), String>, { @@ -475,7 +546,7 @@ where .map_err(|e| format!("Failed to create {}: {}", parent.display(), e))?; } if let Some(path) = reservation { - validate_pair_paths(decisions, state, path).map_err(|e| e.to_string())?; + validate_pair_paths(decisions, state, path).map_err(ConfigWriteError::from)?; } let _decisions_lock = acquire_config_file_write_lock(decisions, CONFIG_LOCK_TIMEOUT)?; // Every reservation publisher takes decisions first. Keep this lease while @@ -483,8 +554,8 @@ where // Protect state/reservation artifacts before creating their sidecars, even // when a later read fails. Pending or unreadable packets never preflight. if let Some(path) = reservation { - if read_pair_reservation(path) - .map_err(|e| e.to_string())? + if read_pair_reservation([decisions, state], path) + .map_err(ConfigWriteError::from)? .is_none() { if let Some(preflight) = nonpending_preflight { @@ -499,9 +570,9 @@ where .transpose()?; if let Some(path) = reservation { if let Some((mut packet, mut physical)) = - read_pair_reservation(path).map_err(|e| e.to_string())? + read_pair_reservation([decisions, state], path).map_err(ConfigWriteError::from)? { - validate_pair_binding(decisions, state, &packet).map_err(|e| e.to_string())?; + validate_pair_binding(decisions, state, &packet).map_err(ConfigWriteError::from)?; if let Some((input, landed)) = activity { landed.set( land_pair_activity( @@ -513,28 +584,28 @@ where input, &|_| Ok(()), ) - .map_err(|e| e.to_string())?, + .map_err(ConfigWriteError::from)?, ); return Ok(()); } - let current = read_pair_tuple(decisions, state).map_err(|e| e.to_string())?; - packet.recognize(¤t).map_err(|e| e.to_string())?; - let mut d = current[0].map().map_err(|e| e.to_string())?; - let mut s = current[1].map().map_err(|e| e.to_string())?; + let current = read_pair_tuple(decisions, state).map_err(ConfigWriteError::from)?; + packet.recognize(¤t).map_err(ConfigWriteError::from)?; + let mut d = current[0].map().map_err(ConfigWriteError::from)?; + let mut s = current[1].map().map_err(ConfigWriteError::from)?; mutate(&mut d, &mut s)?; let proposed = [ prepared_pair_image( ¤t[0], - ¤t[0].map().map_err(|e| e.to_string())?, + ¤t[0].map().map_err(ConfigWriteError::from)?, &d, ) - .map_err(|e| e.to_string())?, + .map_err(ConfigWriteError::from)?, prepared_pair_image( ¤t[1], - ¤t[1].map().map_err(|e| e.to_string())?, + ¤t[1].map().map_err(ConfigWriteError::from)?, &s, ) - .map_err(|e| e.to_string())?, + .map_err(ConfigWriteError::from)?, ]; if proposed == current { return Ok(()); @@ -542,28 +613,32 @@ where // Structural changes, history, selection and unknown fields all // remain frozen. Only the exact timestamp leaf may move forward. if proposed[0] != current[0] - || protected_revision(&proposed[1], true).map_err(|e| e.to_string())? - != protected_revision(¤t[1], true).map_err(|e| e.to_string())? + || protected_revision(&proposed[1], true).map_err(ConfigWriteError::from)? + != protected_revision(¤t[1], true).map_err(ConfigWriteError::from)? { // Absent state permits only its timestamp-only overlay. if proposed[0] != current[0] || current[1] != PhysicalState::Absent - || !timestamp_only_overlay(&proposed[1]).map_err(|e| e.to_string())? + || !timestamp_only_overlay(&proposed[1]).map_err(ConfigWriteError::from)? { - return Err("targetTransitionPending: PAIR is reserved".into()); + return Err(ConfigWriteError::Ordinary( + "targetTransitionPending: PAIR is reserved".to_string(), + )); } } let input = timestamp_value(&proposed[1]) - .map_err(|e| e.to_string())? + .map_err(ConfigWriteError::from)? .ok_or_else(|| { "targetTransitionPending: timestamp cannot be deleted".to_string() })?; if timestamp_value(¤t[1]) - .map_err(|e| e.to_string())? + .map_err(ConfigWriteError::from)? .as_deref() .is_some_and(|old| timestamp_cmp(&input, old).is_ok_and(|order| order.is_lt())) { - return Err("targetTransitionPending: timestamp cannot decrease".into()); + return Err(ConfigWriteError::Ordinary( + "targetTransitionPending: timestamp cannot decrease".to_string(), + )); } land_pair_activity( decisions, @@ -574,14 +649,16 @@ where &input, &|_| Ok(()), ) - .map_err(|e| e.to_string())?; + .map_err(ConfigWriteError::from)?; return Ok(()); } } let both = || format!("{} and {}", decisions.display(), state.display()); let read = |path: &Path| { - read_pair_side(path).map_err(|e| format!("Config pair {} blocked: {}", both(), e)) + read_pair_side(path).map_err(|e| { + e.contextualize(|message| format!("Config pair {} blocked: {}", both(), message)) + }) }; let mut decisions_map = read(decisions)?; let mut state_map = read(state)?; @@ -611,6 +688,7 @@ where ["after_caller_state_publish", "after_caller_decisions_write"], publish, ) + .map_err(ConfigWriteError::Ordinary) } /// Exact private disk image. Absence is distinct from an existing empty object. @@ -623,6 +701,11 @@ pub(crate) enum PhysicalState { Bytes { sha256: String, bytes: Vec }, } +#[cfg(test)] +thread_local! { + static E2992_CANARY_MAP_CALLS: Cell = const { Cell::new(0) }; +} + impl PhysicalState { #[cfg_attr(not(test), allow(dead_code))] // P10: inactive until P11 activation. pub(crate) fn from_bytes(bytes: Vec) -> Self { @@ -635,6 +718,13 @@ impl PhysicalState { #[cfg_attr(not(test), allow(dead_code))] // P10: inactive until P11 activation. fn map(&self) -> Result, PreparedPairError> { + #[cfg(test)] + if let Self::Bytes { bytes, .. } = self { + const CANARY: &[u8] = br#", "opaque":"e2992-parse-canary""#; + if bytes.windows(CANARY.len()).any(|part| part == CANARY) { + E2992_CANARY_MAP_CALLS.with(|count| count.set(count.get() + 1)); + } + } match self { Self::Absent => Ok(Map::new()), Self::Bytes { bytes, .. } => { @@ -711,6 +801,7 @@ impl PreparedConfigPairPlan { pub(crate) enum PreparedPairError { InvalidPlan(&'static str), Preparation(String), + DuplicateJsonMember(String), Conflict, Pending, Io(String), @@ -720,7 +811,9 @@ impl std::fmt::Display for PreparedPairError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::InvalidPlan(reason) => write!(f, "invalidPreparedPair: {reason}"), - Self::Preparation(reason) => write!(f, "preparedPairRejected: {reason}"), + Self::Preparation(reason) | Self::DuplicateJsonMember(reason) => { + write!(f, "preparedPairRejected: {reason}") + } Self::Conflict => { f.write_str("preparedPairConflict: current tuple is not an authorized stage") } @@ -734,6 +827,50 @@ impl std::fmt::Display for PreparedPairError { impl std::error::Error for PreparedPairError {} +#[derive(Debug, PartialEq, Eq)] +pub(super) enum ConfigWriteError { + DuplicateJsonMember(String), + Ordinary(String), +} + +impl std::fmt::Display for ConfigWriteError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::DuplicateJsonMember(message) | Self::Ordinary(message) => f.write_str(message), + } + } +} + +impl From for ConfigWriteError { + fn from(message: String) -> Self { + Self::Ordinary(message) + } +} + +impl ConfigWriteError { + fn contextualize(self, f: impl FnOnce(String) -> String) -> Self { + match self { + Self::DuplicateJsonMember(message) => Self::DuplicateJsonMember(f(message)), + Self::Ordinary(message) => Self::Ordinary(f(message)), + } + } +} + +impl From for ConfigWriteError { + fn from(error: PreparedPairError) -> Self { + match error { + PreparedPairError::DuplicateJsonMember(reason) => Self::DuplicateJsonMember( + PreparedPairError::DuplicateJsonMember(reason).to_string(), + ), + ordinary @ (PreparedPairError::InvalidPlan(_) + | PreparedPairError::Preparation(_) + | PreparedPairError::Conflict + | PreparedPairError::Pending + | PreparedPairError::Io(_)) => Self::Ordinary(ordinary.to_string()), + } + } +} + #[cfg_attr(not(test), allow(dead_code))] // P10: inactive until P11 activation. fn prepared_pair_image( previous: &PhysicalState, @@ -841,6 +978,7 @@ fn execute_unreserved_config_pair_inner( ) -> Result<(), PreparedPairError> { let _writer = ConfigWriterActive::enter("execute_prepared_config_pair") .map_err(PreparedPairError::Preparation)?; + check_configuration_pair_images([decisions, state], plan)?; plan.validate()?; let resolve = |path: &Path| -> Result { let parent = path @@ -877,16 +1015,11 @@ fn execute_unreserved_config_pair_inner( .transpose() .map_err(PreparedPairError::Io)?; if let Some(path) = reservation { - if read_pair_reservation(path)?.is_some() { + if read_pair_reservation([decisions.as_path(), state.as_path()], path)?.is_some() { return Err(PreparedPairError::Pending); } } - let read = || -> Result<[PhysicalState; 2], PreparedPairError> { - Ok([ - read_config_pair_physical(&decisions)?, - read_config_pair_physical(&state)?, - ]) - }; + let read = || read_pair_tuple(&decisions, &state); let current: [PhysicalState; 2] = read()?; let start = plan .stages @@ -1376,7 +1509,11 @@ impl PairReservation { ]) } - fn validate(&self) -> Result<(), PreparedPairError> { + fn validate(&self, paths: [&Path; 2]) -> Result<(), PreparedPairError> { + for (path, image) in paths.into_iter().zip(&self.before_tuple) { + check_configuration_physical(path, image)?; + } + check_configuration_pair_images(paths, &self.plan_images)?; self.plan_images.validate()?; if self.version != 2 || self.protected_policy_version != 1 @@ -1470,14 +1607,42 @@ impl PairReservation { } } +fn check_configuration_physical( + path: &Path, + image: &PhysicalState, +) -> Result<(), PreparedPairError> { + if let PhysicalState::Bytes { bytes, .. } = image { + if PhysicalState::from_bytes(bytes.clone()) != *image { + return Err(PreparedPairError::InvalidPlan("physical digest mismatch")); + } + check_configuration_json(path, bytes).map_err(PreparedPairError::DuplicateJsonMember)?; + } + Ok(()) +} + +fn check_configuration_pair_images( + paths: [&Path; 2], + plan: &PreparedConfigPairPlan, +) -> Result<(), PreparedPairError> { + for tuple in plan.stages() { + for side in 0..2 { + check_configuration_physical(paths[side], &tuple[side])?; + } + } + Ok(()) +} + fn read_pair_tuple( decisions: &Path, state: &Path, ) -> Result<[PhysicalState; 2], PreparedPairError> { - Ok([ + let tuple = [ read_config_pair_physical(decisions)?, read_config_pair_physical(state)?, - ]) + ]; + check_configuration_physical(decisions, &tuple[0])?; + check_configuration_physical(state, &tuple[1])?; + Ok(tuple) } fn pair_target_identity(decisions: &Path, state: &Path) -> Result { @@ -1519,7 +1684,7 @@ fn validate_pair_binding( state: &Path, packet: &PairReservation, ) -> Result<(), PreparedPairError> { - packet.validate()?; + packet.validate([decisions, state])?; if packet.physical_target_identity != pair_target_identity(decisions, state)? { return Err(PreparedPairError::Conflict); } @@ -1560,6 +1725,7 @@ fn validate_pair_paths( } fn read_pair_reservation( + paths: [&Path; 2], path: &Path, ) -> Result, PreparedPairError> { let physical = read_config_pair_physical(path)?; @@ -1568,27 +1734,35 @@ fn read_pair_reservation( }; let packet: PairReservation = serde_json::from_slice(bytes) .map_err(|_| PreparedPairError::InvalidPlan("invalid reservation packet"))?; - packet.validate()?; + packet.validate(paths)?; Ok(Some((packet, physical))) } fn publish_pair_reservation( + paths: [&Path; 2], path: &Path, physical: &mut PhysicalState, packet: &PairReservation, ) -> Result<(), PreparedPairError> { - publish_pair_reservation_guarded(path, physical, packet, &PairTargetGuard::new(&|| Ok(()))) + publish_pair_reservation_guarded( + paths, + path, + physical, + packet, + &PairTargetGuard::new(&|| Ok(())), + ) } #[cfg_attr(not(test), allow(dead_code))] fn publish_pair_reservation_guarded( + paths: [&Path; 2], path: &Path, physical: &mut PhysicalState, packet: &PairReservation, guard: &PairTargetGuard<'_>, ) -> Result<(), PreparedPairError> { guard.check()?; - packet.validate()?; + packet.validate(paths)?; let mut bytes = serde_json::to_vec_pretty(packet).map_err(|e| PreparedPairError::Io(e.to_string()))?; bytes.push(b'\n'); @@ -1652,7 +1826,7 @@ where let _reservation = acquire_config_file_write_lock(reservation, CONFIG_LOCK_TIMEOUT) .map_err(PreparedPairError::Io)?; guard.check()?; - let existing = read_pair_reservation(reservation)?; + let existing = read_pair_reservation([decisions, state], reservation)?; guard.check()?; let current = read_pair_tuple(decisions, state)?; let before = existing @@ -1669,6 +1843,7 @@ where } let plan = prepare_config_pair_plan_inner(before, cleanup, mutate)?; guard.check()?; + check_configuration_pair_images([decisions, state], &plan)?; let images = protected_plan(&plan)?; let candidate = PairReservation { version: 2, @@ -1685,7 +1860,7 @@ where activity_intent: None, plan_images: plan, }; - candidate.validate()?; + candidate.validate([decisions, state])?; candidate.activity_max(¤t, None)?; if let Some((mut packet, mut physical)) = existing { validate_pair_binding(decisions, state, &packet)?; @@ -1711,7 +1886,13 @@ where } candidate.recognize(¤t)?; let mut physical = PhysicalState::Absent; - publish_pair_reservation_guarded(reservation, &mut physical, &candidate, &guard)?; + publish_pair_reservation_guarded( + [decisions, state], + reservation, + &mut physical, + &candidate, + &guard, + )?; guard.check()?; Ok(candidate) } @@ -1761,6 +1942,7 @@ fn reconcile_pair_activity_guarded( guard: &PairTargetGuard<'_>, ) -> Result<(), PreparedPairError> { guard.check()?; + packet.validate([decisions, state])?; let Some(intent) = packet.activity_intent.clone() else { return Ok(()); }; @@ -1777,7 +1959,7 @@ fn reconcile_pair_activity_guarded( packet.activity_intent = None; guard.check()?; packet.recognize(&read_pair_tuple(decisions, state)?)?; - publish_pair_reservation_guarded(reservation, physical, packet, guard) + publish_pair_reservation_guarded([decisions, state], reservation, physical, packet, guard) } fn land_pair_activity( @@ -1807,7 +1989,7 @@ fn land_pair_activity( after_timestamp: maximum.clone(), floor_before: packet.timestamp_floor.clone(), }); - publish_pair_reservation(reservation, physical, packet)?; + publish_pair_reservation([decisions, state], reservation, physical, packet)?; on_stage("activity_intent_durable")?; publish_prepared_pair_image(state, ¤t[1], &after)?; on_stage("activity_state_durable")?; @@ -1877,7 +2059,8 @@ pub(crate) fn verify_reserved_config_pair_guarded( let _reservation = acquire_config_file_write_lock(reservation, CONFIG_LOCK_TIMEOUT) .map_err(PreparedPairError::Io)?; guard.check()?; - let (packet, _) = read_pair_reservation(reservation)?.ok_or(PreparedPairError::Conflict)?; + let (packet, _) = read_pair_reservation([decisions, state], reservation)? + .ok_or(PreparedPairError::Conflict)?; validate_pair_binding(decisions, state, &packet)?; if packet.operation_id != operation_id || packet.plan_digest != plan_digest { return Err(PreparedPairError::Conflict); @@ -1933,8 +2116,8 @@ fn execute_reserved_config_pair_with_validation( let _reservation = acquire_config_file_write_lock(reservation, CONFIG_LOCK_TIMEOUT) .map_err(PreparedPairError::Io)?; guard.check()?; - let (mut packet, mut physical) = - read_pair_reservation(reservation)?.ok_or(PreparedPairError::Conflict)?; + let (mut packet, mut physical) = read_pair_reservation([decisions, state], reservation)? + .ok_or(PreparedPairError::Conflict)?; validate_pair_binding(decisions, state, &packet)?; if packet.operation_id != operation_id { return Err(PreparedPairError::Conflict); @@ -1949,7 +2132,14 @@ fn execute_reserved_config_pair_with_validation( )?; guard.check()?; let start = packet.recognize(&read_pair_tuple(decisions, state)?)?; - confirm_pair_activity_floor_guarded(state, reservation, &mut packet, &mut physical, &guard)?; + confirm_pair_activity_floor_guarded( + decisions, + state, + reservation, + &mut packet, + &mut physical, + &guard, + )?; for stage in start + 1..5 { guard.check()?; let current = read_pair_tuple(decisions, state)?; @@ -1985,6 +2175,7 @@ fn execute_reserved_config_pair_with_validation( return Err(PreparedPairError::Conflict); } confirm_pair_activity_floor_guarded( + decisions, state, reservation, &mut packet, @@ -1994,7 +2185,14 @@ fn execute_reserved_config_pair_with_validation( on_stage(stage)?; } } - confirm_pair_activity_floor_guarded(state, reservation, &mut packet, &mut physical, &guard)?; + confirm_pair_activity_floor_guarded( + decisions, + state, + reservation, + &mut packet, + &mut physical, + &guard, + )?; guard.check()?; if packet.recognize(&read_pair_tuple(decisions, state)?)? != 4 { return Err(PreparedPairError::Conflict); @@ -2005,6 +2203,7 @@ fn execute_reserved_config_pair_with_validation( #[cfg_attr(not(test), allow(dead_code))] fn confirm_pair_activity_floor_guarded( + decisions: &Path, state: &Path, reservation: &Path, packet: &mut PairReservation, @@ -2012,13 +2211,13 @@ fn confirm_pair_activity_floor_guarded( guard: &PairTargetGuard<'_>, ) -> Result<(), PreparedPairError> { guard.check()?; - let floor = merge_monotonic_activity(&[ - packet.timestamp_floor.clone(), - timestamp_value(&read_config_pair_physical(state)?)?, - ])?; + let captured = read_config_pair_physical(state)?; + check_configuration_physical(state, &captured)?; + let floor = + merge_monotonic_activity(&[packet.timestamp_floor.clone(), timestamp_value(&captured)?])?; if floor != packet.timestamp_floor { packet.timestamp_floor = floor; - publish_pair_reservation_guarded(reservation, physical, packet, guard)?; + publish_pair_reservation_guarded([decisions, state], reservation, physical, packet, guard)?; } Ok(()) } @@ -2141,7 +2340,8 @@ pub(crate) fn release_reserved_config_pair_guarded( if proof.target_id != pair_target_identity(decisions, state)? { return Err(PreparedPairError::Conflict); } - let Some((mut packet, mut physical)) = read_pair_reservation(reservation)? else { + let Some((mut packet, mut physical)) = read_pair_reservation([decisions, state], reservation)? + else { let current = read_pair_tuple(decisions, state)?; guard.check()?; return Ok(current); @@ -2602,19 +2802,18 @@ fn publish_coordination_bytes_guarded( /// #2786 (C1) - one side of the pair as a map: absent is empty, anything that /// is not a readable JSON object is an error. -fn read_pair_side(path: &Path) -> Result, String> { +fn read_pair_side(path: &Path) -> Result, ConfigWriteError> { let content = match std::fs::read_to_string(path) { Ok(content) => content, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(Map::new()), - Err(e) => return Err(format!("Failed to read {}: {}", path.display(), e)), + Err(e) => return Err(format!("Failed to read {}: {}", path.display(), e).into()), }; + check_configuration_json(path, content.as_bytes()) + .map_err(ConfigWriteError::DuplicateJsonMember)?; match serde_json::from_str::(&content) { Ok(Value::Object(map)) => Ok(map), - Ok(_) => Err(format!( - "Local config {} must be a JSON object", - path.display() - )), - Err(e) => Err(format!("Failed to parse {}: {}", path.display(), e)), + Ok(_) => Err(format!("Local config {} must be a JSON object", path.display()).into()), + Err(e) => Err(format!("Failed to parse {}: {}", path.display(), e).into()), } } @@ -5630,6 +5829,226 @@ mod pair_activity_tests { assert_eq!(p22_stages(&directory), stages); } + #[test] + fn e2992_duplicate_json_pair_original_and_embedded_images_before_parse() { + for side in 0..2 { + for slot in 0usize..7 { + assert_pair_image_duplicate_rejected(side, slot); + } + } + } + + fn assert_pair_image_duplicate_rejected(side: usize, slot: usize) { + let fixture = Fixture::new(false); + for path in [fixture.d(), fixture.s()] { + let mut value: Value = serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); + value["opaque"] = json!("e2992-parse-canary"); + fixture.write(&path, value); + } + let request = fixture.request(); + let mut packet = fixture.reserve(&request).unwrap(); + let d = fixture.d(); + let st = fixture.s(); + let paths = [d.as_path(), st.as_path()]; + let stage = slot.saturating_sub(2); + let valid = match &packet.plan_images.stages[stage][side] { + PhysicalState::Bytes { bytes, .. } => bytes, + PhysicalState::Absent => panic!("present fixture"), + }; + let closing = valid.iter().rposition(|byte| *byte == b'}').unwrap(); + let mut duplicate = valid[..closing].to_vec(); + duplicate.extend_from_slice(br#", "opaque":"e2992-parse-canary"}"#); + assert_eq!( + serde_json::from_slice::(&duplicate).unwrap(), + serde_json::from_slice::(valid).unwrap() + ); + let image = PhysicalState::from_bytes(duplicate.clone()); + match slot { + 0 => std::fs::write(paths[side], &duplicate).unwrap(), + 1 => packet.before_tuple[side] = image, + _ => packet.plan_images.stages[slot - 2][side] = image, + } + let envelope = serde_json::to_vec(&packet).unwrap(); + std::fs::write(fixture.r(), &envelope).unwrap(); + let originals = [std::fs::read(&d).unwrap(), std::fs::read(&st).unwrap()]; + E2992_CANARY_MAP_CALLS.with(|count| count.set(0)); + let error = + execute_reserved_config_pair(&d, &st, &fixture.r(), &request.operation_id).unwrap_err(); + assert!( + matches!(&error, PreparedPairError::DuplicateJsonMember(message) if message.contains("duplicateJsonMember:") && message.contains(paths[side].to_str().unwrap()) && message.contains("repeated JSON object members")) + ); + assert!(!error.to_string().contains("canary")); + let called = Cell::new(false); + let classified = update_config_pair_guarded_classified( + &d, + &st, + &fixture.r(), + &[], + None, + Some(&|_, _| { + called.set(true); + Ok(()) + }), + &|_| { + called.set(true); + }, + |_, _| { + called.set(true); + Ok(()) + }, + &|_| { + called.set(true); + Ok(()) + }, + None, + ) + .unwrap_err(); + assert!( + matches!(&classified, ConfigWriteError::DuplicateJsonMember(message) + if message.contains(paths[side].to_str().unwrap())) + ); + assert!(!called.get()); + assert_eq!(E2992_CANARY_MAP_CALLS.with(Cell::get), 0); + assert_eq!(std::fs::read(&d).unwrap(), originals[0]); + assert_eq!(std::fs::read(&st).unwrap(), originals[1]); + assert_eq!(std::fs::read(fixture.r()).unwrap(), envelope); + if slot != 0 { + let mut physical = PhysicalState::from_bytes(envelope.clone()); + let before = packet.clone(); + let error = reconcile_pair_activity_guarded( + &d, + &st, + &fixture.r(), + &mut packet, + &mut physical, + &PairTargetGuard::new(&|| Ok(())), + ) + .unwrap_err(); + assert!(matches!(error, PreparedPairError::DuplicateJsonMember(_))); + assert_eq!(packet, before); + assert_eq!(std::fs::read(&d).unwrap(), originals[0]); + assert_eq!(std::fs::read(&st).unwrap(), originals[1]); + assert_eq!(E2992_CANARY_MAP_CALLS.with(Cell::get), 0); + assert_eq!(std::fs::read(fixture.r()).unwrap(), envelope); + } + } + + #[test] + fn e2992_duplicate_json_pair_ordinary_callbacks_and_envelope_controls() { + let fixture = Fixture::new(false); + let request = fixture.request(); + let mut packet = fixture.reserve(&request).unwrap(); + let mut physical = PhysicalState::from_bytes(std::fs::read(fixture.r()).unwrap()); + reconcile_pair_activity_guarded( + &fixture.d(), + &fixture.s(), + &fixture.r(), + &mut packet, + &mut physical, + &PairTargetGuard::new(&|| Ok(())), + ) + .unwrap(); + execute_reserved_config_pair( + &fixture.d(), + &fixture.s(), + &fixture.r(), + &request.operation_id, + ) + .unwrap(); + for invalid in 0..6 { + let fixture = Fixture::new(false); + let request = fixture.request(); + let packet = fixture.reserve(&request).unwrap(); + let mut envelope = serde_json::to_value(&packet).unwrap(); + match invalid { + 0 => envelope["unknown"] = json!(true), + 1 => envelope["version"] = json!(1), + 2 => envelope["planDigest"] = json!("wrong"), + 3 => envelope["protectedImages"] = json!([]), + 4 => envelope["timestampFloor"] = json!("invalid timestamp"), + _ => envelope["activityIntent"] = json!({"invalid": true}), + } + let bytes = serde_json::to_vec(&envelope).unwrap(); + std::fs::write(fixture.r(), &bytes).unwrap(); + let before = fixture.tuple(); + let error = + read_pair_reservation([&fixture.d(), &fixture.s()], &fixture.r()).unwrap_err(); + assert!(!matches!(error, PreparedPairError::DuplicateJsonMember(_))); + assert_eq!(fixture.tuple(), before); + assert_eq!(std::fs::read(fixture.r()).unwrap(), bytes); + } + for side in 0..2 { + let fixture = Fixture::new(false); + let d = fixture.d(); + let st = fixture.s(); + let paths = [&d, &st]; + std::fs::write( + paths[side], + br#"{"unknown-canary":"secret-canary","unknown-canary":0}"#, + ) + .unwrap(); + let original = [std::fs::read(&d).unwrap(), std::fs::read(&st).unwrap()]; + let called = Cell::new(false); + let error = update_config_pair_inner_classified( + &d, + &st, + None, + &[], + Some(&|_, _| { + called.set(true); + Ok(()) + }), + &|_| { + called.set(true); + }, + |_, _| { + called.set(true); + Ok(()) + }, + &|_| { + called.set(true); + Ok(()) + }, + None, + None, + &|_, _| { + called.set(true); + Ok(()) + }, + ) + .unwrap_err(); + assert!( + matches!(&error, ConfigWriteError::DuplicateJsonMember(message) + if message.contains(paths[side].to_str().unwrap()) && message.contains("repeated JSON object members")) + ); + assert!(!error.to_string().contains("canary")); + assert!(!called.get()); + assert_eq!(std::fs::read(&d).unwrap(), original[0]); + assert_eq!(std::fs::read(&st).unwrap(), original[1]); + } + let fixture = Fixture::new(false); + let error = prepare_config_pair_plan_inner(fixture.tuple(), None, |_, _| { + Err("duplicateJsonMember: callback remains ordinary".into()) + }) + .unwrap_err(); + assert!(matches!(error, PreparedPairError::Preparation(_))); + let error = update_config_pair_inner_classified( + &fixture.d(), + &fixture.s(), + None, + &[], + None, + &|_| {}, + |_, _| Err("duplicateJsonMember: ordinary callback".into()), + &|_| Ok(()), + None, + None, + &publish_temp_config, + ) + .unwrap_err(); + assert!(matches!(error, ConfigWriteError::Ordinary(_))); + } + use serde_json::json; const T1: &str = "2026-10-08T01:00:00Z"; @@ -5674,7 +6093,10 @@ mod pair_activity_tests { read_pair_tuple(&self.d(), &self.s()).unwrap() } fn packet(&self) -> PairReservation { - read_pair_reservation(&self.r()).unwrap().unwrap().0 + read_pair_reservation([&self.d(), &self.s()], &self.r()) + .unwrap() + .unwrap() + .0 } fn request(&self) -> PairReservationRequest { PairReservationRequest { @@ -5741,7 +6163,8 @@ mod pair_activity_tests { let _d = acquire_config_file_write_lock(&self.d(), CONFIG_LOCK_TIMEOUT).unwrap(); let _s = acquire_config_file_write_lock(&self.s(), CONFIG_LOCK_TIMEOUT).unwrap(); let _r = acquire_config_file_write_lock(&self.r(), CONFIG_LOCK_TIMEOUT).unwrap(); - let (mut packet, mut physical) = read_pair_reservation(&self.r())?.unwrap(); + let (mut packet, mut physical) = + read_pair_reservation([&self.d(), &self.s()], &self.r())?.unwrap(); land_pair_activity( &self.d(), &self.s(), diff --git a/src-tauri/src/config/local_overlay.rs b/src-tauri/src/config/local_overlay.rs index fe30a9578..751bd77cf 100644 --- a/src-tauri/src/config/local_overlay.rs +++ b/src-tauri/src/config/local_overlay.rs @@ -389,6 +389,29 @@ impl LocalSettingsOverlay { } /// The same loader over captured physical bytes, without a second IO read. + #[allow(clippy::too_many_arguments)] + #[allow(dead_code)] // P52: prepared for P45 captured-source admission. + pub(crate) fn from_overlay_bytes_paired_checked( + base: &mut Value, + contents: &[u8], + ineligible_disk: &[&str], + ineligible_legacy: &[&str], + derived: &[DerivedIdClosure], + pairs: &[PairedLeaf], + overlay_path: &std::path::Path, + check: fn(&std::path::Path, &[u8]) -> Result<(), String>, + ) -> Result { + check(overlay_path, contents)?; + Ok(Self::from_overlay_bytes_paired( + base, + contents, + ineligible_disk, + ineligible_legacy, + derived, + pairs, + )) + } + pub(crate) fn from_overlay_bytes_paired( base: &mut Value, contents: &[u8], @@ -738,6 +761,42 @@ fn walk_creating_object_mut<'a>( #[cfg(test)] mod tests { + #[test] + fn e2992_duplicate_json_overlay_rejects_before_shadowing() { + let mut base = serde_json::json!({"x": 1}); + let before = base.clone(); + let temp = tempfile::tempdir().unwrap(); + let overlay_path = temp.path().join("settings.local.json"); + let path = overlay_path.as_path(); + let error = LocalSettingsOverlay::from_overlay_bytes_paired_checked( + &mut base, + br#"{"x":2,"unknown-canary":"value-canary","unknown-canary":0}"#, + &[], + &[], + &[], + &[], + path, + crate::path_identity::validate_ac_configuration_json, + ) + .unwrap_err(); + assert!(error.contains("duplicateJsonMember:")); + assert!(error.contains("settings.local.json")); + assert!(!error.contains("canary")); + assert_eq!(base, before); + LocalSettingsOverlay::from_overlay_bytes_paired_checked( + &mut base, + br#"{"x":2}"#, + &[], + &[], + &[], + &[], + path, + crate::path_identity::validate_ac_configuration_json, + ) + .unwrap(); + assert_eq!(base["x"], 2); + } + use super::*; use serde_json::json; diff --git a/src-tauri/src/config/replica_identity.rs b/src-tauri/src/config/replica_identity.rs index 86907ed9c..d0eaa178d 100644 --- a/src-tauri/src/config/replica_identity.rs +++ b/src-tauri/src/config/replica_identity.rs @@ -22,6 +22,7 @@ pub struct WgReplicaIdentity { pub enum ReplicaConfigFault { ConfigUnreadable, ConfigNotJson, + DuplicateJsonMember, ConfigNotObject, IdentityMissing, IdentityMismatch, @@ -409,6 +410,8 @@ pub fn read_wg_replica_config_read_only_classified( "Room replica config failed a bounded path-safe read".to_string(), ) })?; + crate::path_identity::validate_ac_configuration_json(&config_path, &bytes) + .map_err(|message| (ReplicaConfigFault::DuplicateJsonMember, message))?; let config = crate::path_identity::parse_json_no_duplicates(&bytes).map_err(|_| { ( ReplicaConfigFault::ConfigNotJson, @@ -428,30 +431,43 @@ pub fn read_wg_replica_config_read_only_classified( Ok((config, identity)) } +pub(super) use super::agent_config::AgentConfigWriteError as ReplicaRepairError; + pub fn read_and_repair_wg_replica_config( replica_dir: &Path, required_context_prefix: &[&str], ) -> Result<(Value, WgReplicaIdentity), String> { + read_and_repair_wg_replica_config_classified(replica_dir, required_context_prefix) + .map_err(|error| error.to_string()) +} + +pub(super) fn read_and_repair_wg_replica_config_classified( + replica_dir: &Path, + required_context_prefix: &[&str], +) -> Result<(Value, WgReplicaIdentity), ReplicaRepairError> { let config_path = replica_dir.join("config.json"); let mut repaired_identity: Option = None; // #2816 (C3) - the returned value is the decisions object as published, // captured inside the guard: no re-read and no state overlay. let mut published: Option = None; - crate::config::agent_config::update_existing_agent_config(&config_path, |obj, _state| { - let mut value = Value::Object(std::mem::take(obj)); - let identity = - repair_wg_replica_config_value(replica_dir, &mut value, required_context_prefix)?; - let final_obj = value.as_object_mut().ok_or_else(|| { - format!( - "Room replica config {} must be a JSON object", - config_path.display() - ) - })?; - *obj = std::mem::take(final_obj); - repaired_identity = Some(identity); - published = Some(Value::Object(obj.clone())); - Ok(()) - })?; + crate::config::agent_config::update_existing_agent_config_classified( + &config_path, + |obj, _state| { + let mut value = Value::Object(std::mem::take(obj)); + let identity = + repair_wg_replica_config_value(replica_dir, &mut value, required_context_prefix)?; + let final_obj = value.as_object_mut().ok_or_else(|| { + format!( + "Room replica config {} must be a JSON object", + config_path.display() + ) + })?; + *obj = std::mem::take(final_obj); + repaired_identity = Some(identity); + published = Some(Value::Object(obj.clone())); + Ok(()) + }, + )?; let config = published.ok_or_else(|| format!("Failed to repair {}", config_path.display()))?; let identity = repaired_identity .ok_or_else(|| format!("Failed to repair identity for {}", replica_dir.display()))?; @@ -518,6 +534,53 @@ pub(crate) mod strict_read_probe { #[cfg(test)] mod tests { + #[test] + fn e2992_duplicate_json_replica_repair_and_read_only_reason() { + for side in 0..2 { + let temp = setup_replica(".ac"); + let replica = temp + .path() + .join("AgentsCommander_ac/.ac/wg-2-dev-team/__agent_tech-lead"); + let d = replica.join("config.json"); + let st = replica.join(crate::config::instance_artifacts::CONFIG_STATE_TARGET_NAME); + std::fs::write(&d, br#"{"identity":"../../_agent_tech-lead"}"#).unwrap(); + std::fs::write(&st, b"{}").unwrap(); + let paths = [&d, &st]; + std::fs::write( + paths[side], + br#"{"unknown-canary":"value-canary","unknown-canary":0}"#, + ) + .unwrap(); + let originals = [std::fs::read(&d).unwrap(), std::fs::read(&st).unwrap()]; + if side == 0 { + let (fault, message) = + read_wg_replica_config_read_only_classified(&replica).unwrap_err(); + assert_eq!( + fault, + ReplicaConfigFault::DuplicateJsonMember, + "side={side} physical_file={d:?} actual_fault={fault:?} message={message:?} canonical={:?} component_chain={:?} ancestors={:?}", + std::fs::canonicalize(&d), + crate::path_identity::verify_component_chain_reason(&replica), + d.ancestors() + .map(|path| (path.to_path_buf(), std::fs::symlink_metadata(path))) + .collect::>() + ); + assert!(message.contains(d.to_str().unwrap())); + assert!(!message.contains("canary")); + } + let error = + read_and_repair_wg_replica_config_classified(&replica, WG_REPLICA_REQUIRED_CONTEXT) + .unwrap_err(); + assert!( + matches!(&error, ReplicaRepairError::DuplicateJsonMember(message) + if message.contains("duplicateJsonMember:") && message.contains(paths[side].to_str().unwrap())) + ); + assert!(!error.to_string().contains("canary")); + assert_eq!(std::fs::read(&d).unwrap(), originals[0]); + assert_eq!(std::fs::read(&st).unwrap(), originals[1]); + } + } + use super::*; #[test] diff --git a/src-tauri/src/config/root_agent.rs b/src-tauri/src/config/root_agent.rs index e4a91b0a9..d0bc4b5f9 100644 --- a/src-tauri/src/config/root_agent.rs +++ b/src-tauri/src/config/root_agent.rs @@ -1218,6 +1218,7 @@ pub fn ensure_root_agent_dir() -> Result { pub(crate) fn ensure_root_agent_dir_at(root_dir: &Path) -> Result<(), String> { validate_root_agent_root_path(root_dir)?; + crate::config::agent_config::read_agent_local_config_checked(root_dir)?; crate::commands::entity_creation::create_agent_matrix_layout(root_dir).map_err( |(sub, e)| { format!( @@ -2425,14 +2426,23 @@ fn context_array_matches(arr: &[Value], expected: &[&str]) -> bool { } pub fn read_last_coding_agent(root_dir: &str) -> Option { + read_last_coding_agent_checked(root_dir).ok().flatten() +} + +pub fn read_last_coding_agent_checked(root_dir: &str) -> Result, String> { // #2786 (C1) - the raw loader with field-level extraction, as before C1: // an unrelated malformed field must not hide `lastCodingAgent`. - crate::config::agent_config::read_agent_local_config_json(Path::new(root_dir)) - .ok()?? - .get("tooling")? - .get("lastCodingAgent")? - .as_str() - .map(ToString::to_string) + Ok( + crate::config::agent_config::read_agent_local_config_json(Path::new(root_dir))?.and_then( + |value| { + value + .get("tooling")? + .get("lastCodingAgent")? + .as_str() + .map(ToString::to_string) + }, + ), + ) } fn paths_equivalent(left: &Path, right: &Path) -> bool { @@ -2460,6 +2470,65 @@ fn display_path(path: &Path) -> String { #[cfg(test)] mod tests { + #[test] + fn e2992_duplicate_json_root_admits_both_files_before_layout() { + for side in 0..2 { + let temp = tempfile::tempdir().unwrap(); + let d = temp.path().join("config.json"); + let st = temp + .path() + .join(crate::config::instance_artifacts::CONFIG_STATE_TARGET_NAME); + std::fs::write(&d, b"{}").unwrap(); + std::fs::write(&st, b"{}").unwrap(); + let paths = [&d, &st]; + std::fs::write( + paths[side], + br#"{"unknown-canary":"value-canary","unknown-canary":0}"#, + ) + .unwrap(); + let before = [std::fs::read(&d).unwrap(), std::fs::read(&st).unwrap()]; + for error in [ + read_last_coding_agent_checked(temp.path().to_str().unwrap()).unwrap_err(), + merge_root_agent_config(&d).unwrap_err(), + ensure_root_agent_dir_at(temp.path()).unwrap_err(), + ] { + assert!(error.contains("duplicateJsonMember:")); + assert!(error.contains(paths[side].to_str().unwrap())); + assert!(!error.contains("canary")); + } + assert_eq!(std::fs::read(&d).unwrap(), before[0]); + assert_eq!(std::fs::read(&st).unwrap(), before[1]); + assert!(!temp.path().join("skills").exists()); + assert!(!temp.path().join("messaging").exists()); + assert!(!temp.path().join("Role.md").exists()); + } + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join(ROOT_AGENT_DIR_NAME); + std::fs::create_dir(&root).unwrap(); + let d = root.join("config.json"); + let st = root.join(crate::config::instance_artifacts::CONFIG_STATE_TARGET_NAME); + std::fs::write(&d, b"{").unwrap(); + std::fs::write(&st, b"{}").unwrap(); + let native_error = serde_json::from_str::("{").unwrap_err(); + assert!(native_error.is_eof()); + let error = ensure_root_agent_dir_at(&root).unwrap_err(); + assert_eq!( + error, + format!( + "Config pair {} and {} blocked: Failed to parse {}: {}", + d.display(), + st.display(), + d.display(), + native_error + ) + ); + assert!(root.join("skills").is_dir()); + assert!(root.join("messaging").is_dir()); + assert!(root.join("Role.md").is_file()); + assert_eq!(std::fs::read(&d).unwrap(), b"{"); + assert_eq!(std::fs::read(&st).unwrap(), b"{}"); + } + use super::*; #[test] diff --git a/src-tauri/src/config/sessions_persistence.rs b/src-tauri/src/config/sessions_persistence.rs index 0210b7d68..36ee49d0e 100644 --- a/src-tauri/src/config/sessions_persistence.rs +++ b/src-tauri/src/config/sessions_persistence.rs @@ -1900,6 +1900,19 @@ impl PersistedSession { } } +/// Read the startup selection without purge, archive, save or settings IO. +#[allow(dead_code)] // P52: prepared for P53 startup admission. +pub(crate) fn read_startup_sessions_for_admission( + config_dir: &Path, + settings: &super::settings::AppSettings, +) -> Result, String> { + let paths = session_retention_project_paths(settings)?; + let roots = normalize_project_roots(&paths); + let sessions = + load_sessions_from_path_with_settings(&config_dir.join("sessions.json"), Some(settings)); + Ok(partition_orphaned_sessions(sessions, &roots).0) +} + fn load_sessions_from_path(path: &Path) -> Vec { load_sessions_from_path_with_settings(path, None) } @@ -5983,6 +5996,80 @@ async fn write_start_fresh_and_persist_to_dir_result_core( #[cfg(test)] mod tests { + #[test] + fn e2992_duplicate_json_startup_sessions_admission_is_read_only() { + let temp = tempfile::tempdir().unwrap(); + let active = temp.path().join("active"); + let archived = temp.path().join("archived"); + let settings = crate::config::settings::AppSettings { + project_paths: vec![active.to_string_lossy().to_string()], + archived_project_paths: vec![archived.to_string_lossy().to_string()], + ..Default::default() + }; + let config = temp.path().join("config"); + assert!( + super::read_startup_sessions_for_admission(&config, &settings) + .unwrap() + .is_empty() + ); + assert!(!config.exists()); + std::fs::create_dir(&config).unwrap(); + let path = config.join("sessions.json"); + let rows = vec![ + PersistedSession { + name: "active".into(), + working_directory: active.to_string_lossy().to_string(), + ..Default::default() + }, + PersistedSession { + name: "archived".into(), + working_directory: archived.to_string_lossy().to_string(), + ..Default::default() + }, + PersistedSession { + name: "Root".into(), + is_root_agent: true, + working_directory: "missing-root".into(), + ..Default::default() + }, + PersistedSession { + name: "orphan".into(), + working_directory: "missing-orphan".into(), + ..Default::default() + }, + ]; + let bytes = serde_json::to_vec(&rows).unwrap(); + std::fs::write(&path, &bytes).unwrap(); + let selected = super::read_startup_sessions_for_admission(&config, &settings).unwrap(); + assert_eq!( + selected + .iter() + .map(|row| row.name.as_str()) + .collect::>(), + vec!["active", "archived", "Root"] + ); + let expected = super::partition_orphaned_sessions( + super::load_sessions_from_path_with_settings(&path, Some(&settings)), + &super::normalize_project_roots( + &super::session_retention_project_paths(&settings).unwrap(), + ), + ) + .0; + assert_eq!( + serde_json::to_value(selected).unwrap(), + serde_json::to_value(expected).unwrap() + ); + assert_eq!(std::fs::read(&path).unwrap(), bytes); + assert_eq!(std::fs::read_dir(&config).unwrap().count(), 1); + std::fs::write(&path, b"{").unwrap(); + assert!( + super::read_startup_sessions_for_admission(&config, &settings) + .unwrap() + .is_empty() + ); + assert_eq!(std::fs::read(&path).unwrap(), b"{"); + } + use super::{ append_orphan_archive_record, clear_blocked_menu_and_persist_to_dir, clear_user_input_transitions_and_persist_to_dir_result, diff --git a/src-tauri/src/config/teams.rs b/src-tauri/src/config/teams.rs index c28c0d4d4..78089fa79 100644 --- a/src-tauri/src/config/teams.rs +++ b/src-tauri/src/config/teams.rs @@ -3,7 +3,8 @@ use thiserror::Error; use crate::config::ac_root::{existing_ac_root, find_ac_root_segment, has_ac_root}; use crate::config::replica_identity::{ - agent_bare_name_from_ref, read_and_repair_wg_replica_config, WG_REPLICA_REQUIRED_CONTEXT, + agent_bare_name_from_ref, read_and_repair_wg_replica_config_classified, ReplicaRepairError, + WG_REPLICA_REQUIRED_CONTEXT, }; /// #280 §3.4 — record whether the missing-config one-shot INFO has already @@ -474,25 +475,49 @@ pub fn resolve_wg_coordinator_replica( ac_root: &Path, wg_dir: &Path, ) -> Option { - let project = ac_root.parent()?.file_name()?.to_str()?.to_string(); - let wg_name = wg_dir.file_name()?.to_str()?.to_string(); - let team = crate::config::entity_prefix::strip_entity_prefix(&wg_name) - .and_then(|s| s.split_once('-').map(|(_, rest)| rest.to_string()))?; - - let team_dir = ac_root.join(format!("_team_{}", team)); - let team_config: serde_json::Value = std::fs::read_to_string(team_dir.join("config.json")) + resolve_wg_coordinator_replica_checked(ac_root, wg_dir) .ok() - .and_then(|raw| serde_json::from_str(&raw).ok())?; - let coordinator_ref = team_config.get("coordinator").and_then(|c| c.as_str())?; - let coordinator_name = agent_bare_name_from_ref(coordinator_ref).ok()?; + .flatten() +} + +pub fn resolve_wg_coordinator_replica_checked( + ac_root: &Path, + wg_dir: &Path, +) -> Result, String> { + let Some((project, wg_name, team)) = (|| { + let project = ac_root.parent()?.file_name()?.to_str()?.to_string(); + let wg_name = wg_dir.file_name()?.to_str()?.to_string(); + let team = crate::config::entity_prefix::strip_entity_prefix(&wg_name) + .and_then(|s| s.split_once('-').map(|(_, rest)| rest.to_string()))?; + Some((project, wg_name, team)) + })() else { + return Ok(None); + }; + let config_path = ac_root.join(format!("_team_{}", team)).join("config.json"); + let Ok(bytes) = std::fs::read(&config_path) else { + return Ok(None); + }; + crate::path_identity::validate_ac_configuration_json(&config_path, &bytes)?; + let Ok(team_config) = serde_json::from_slice::(&bytes) else { + return Ok(None); + }; + let Some(coordinator_ref) = team_config.get("coordinator").and_then(|c| c.as_str()) else { + return Ok(None); + }; + let Ok(coordinator_name) = agent_bare_name_from_ref(coordinator_ref) else { + return Ok(None); + }; if !ac_root .join(format!("_agent_{}", coordinator_name)) .is_dir() { - return None; + return Ok(None); } - for replica_entry in std::fs::read_dir(wg_dir).ok()?.flatten() { + let Ok(entries) = std::fs::read_dir(wg_dir) else { + return Ok(None); + }; + for replica_entry in entries.flatten() { let replica_dir = replica_entry.path(); if !replica_dir.is_dir() { continue; @@ -501,36 +526,50 @@ pub fn resolve_wg_coordinator_replica( Some(n) if n.starts_with("__agent_") => n, _ => continue, }; - let Ok((_config, identity)) = - read_and_repair_wg_replica_config(&replica_dir, WG_REPLICA_REQUIRED_CONTEXT) - else { - continue; + let identity = match read_and_repair_wg_replica_config_classified( + &replica_dir, + WG_REPLICA_REQUIRED_CONTEXT, + ) { + Ok((_config, identity)) => identity, + Err(ReplicaRepairError::Ordinary(_)) => continue, + Err(ReplicaRepairError::DuplicateJsonMember(message)) => return Err(message), }; if identity.agent_name == coordinator_name { - return Some(WgCoordinatorReplica { + return Ok(Some(WgCoordinatorReplica { project, team, wg_name, agent_name: identity.agent_name, replica_dir, - }); + })); } } - None + Ok(None) } pub fn verified_wg_coordinator_target( target: &str, project_paths: &[String], ) -> Option { + verified_wg_coordinator_target_checked(target, project_paths) + .ok() + .flatten() +} + +pub fn verified_wg_coordinator_target_checked( + target: &str, + project_paths: &[String], +) -> Result, String> { let (Some(project), local) = split_project_prefix(target) else { - return None; + return Ok(None); }; if !is_valid_wg_local_shape(local) { - return None; + return Ok(None); } - let (wg_name, agent_name) = local.split_once('/')?; + let Some((wg_name, agent_name)) = local.split_once('/') else { + return Ok(None); + }; for (project_name, project_dir) in enumerate_project_dirs(project_paths) { if project_name != project { @@ -543,14 +582,14 @@ pub fn verified_wg_coordinator_target( if !wg_dir.is_dir() { continue; } - if let Some(resolved) = resolve_wg_coordinator_replica(&ac_root, &wg_dir) { + if let Some(resolved) = resolve_wg_coordinator_replica_checked(&ac_root, &wg_dir)? { if resolved.agent_name == agent_name { - return Some(resolved); + return Ok(Some(resolved)); } } } - None + Ok(None) } // Privileged PTY-input routing is intentionally separate from broad message @@ -784,6 +823,7 @@ fn read_identity_json( String, > { let (bytes, identity) = crate::path_identity::read_bounded_regular(path, 1024 * 1024)?; + crate::path_identity::validate_ac_configuration_json(path, &bytes)?; let value = crate::path_identity::parse_json_no_duplicates(&bytes)?; if !value.is_object() { return Err("sender_identity_invalid".to_string()); @@ -1784,8 +1824,12 @@ pub fn can_communicate(from: &str, to: &str, teams: &[DiscoveredTeam]) -> bool { /// Discover all teams from all known project paths. /// Scans settings.project_paths (and immediate children) for Project AC Root `_team_*/config.json`. pub fn discover_teams() -> Vec { + discover_teams_checked().unwrap_or_default() +} + +pub fn discover_teams_checked() -> Result, String> { let settings = crate::config::settings::load_settings(); - discover_teams_from_project_paths(&settings.project_paths) + discover_teams_from_project_paths_checked(&settings.project_paths) } /// CLI coordinator discovery fails closed on an unreadable registered source. @@ -1802,6 +1846,12 @@ pub(crate) fn discover_teams_for_cli() -> Vec { /// Scan a captured registered-project slice without loading or saving settings. pub(crate) fn discover_teams_from_project_paths(project_paths: &[String]) -> Vec { + discover_teams_from_project_paths_checked(project_paths).unwrap_or_default() +} + +pub(crate) fn discover_teams_from_project_paths_checked( + project_paths: &[String], +) -> Result, String> { let mut teams = Vec::new(); for repo_path in project_paths { @@ -1838,7 +1888,7 @@ pub(crate) fn discover_teams_from_project_paths(project_paths: &[String]) -> Vec "[teams] discover_teams: entering project_dir='{}'", project_dir.display() ); - discover_teams_in_project(&project_dir, &mut teams); + discover_teams_in_project_filtered_checked(&project_dir, &mut teams, |_| true)?; log::trace!( "[teams] discover_teams: project_dir='{}' produced {} team(s)", project_dir.display(), @@ -1852,10 +1902,11 @@ pub(crate) fn discover_teams_from_project_paths(project_paths: &[String]) -> Vec teams.len(), project_paths.len() ); - teams + Ok(teams) } /// Discover teams in a single project directory. +#[allow(dead_code)] // P52: retain legacy wrapper until P54/P55 migration. fn discover_teams_in_project(project_dir: &Path, teams: &mut Vec) { discover_teams_in_project_filtered(project_dir, teams, |_| true); } @@ -1866,8 +1917,17 @@ pub(crate) fn discover_teams_in_project_filtered( teams: &mut Vec, accept_team: impl Fn(&Path) -> bool, ) { + let _ = discover_teams_in_project_filtered_checked(project_dir, teams, accept_team); +} + +pub(crate) fn discover_teams_in_project_filtered_checked( + project_dir: &Path, + teams: &mut Vec, + accept_team: impl Fn(&Path) -> bool, +) -> Result<(), String> { + let mut discovered = Vec::new(); let Some(ac_root) = existing_ac_root(project_dir) else { - return; + return Ok(()); }; let project_folder = project_dir @@ -1878,7 +1938,7 @@ pub(crate) fn discover_teams_in_project_filtered( let entries = match std::fs::read_dir(&ac_root) { Ok(e) => e, - Err(_) => return, + Err(_) => return Ok(()), }; for entry in entries.flatten() { @@ -1945,6 +2005,7 @@ pub(crate) fn discover_teams_in_project_filtered( continue; } }; + crate::path_identity::validate_ac_configuration_json(&config_path, raw.as_bytes())?; let parsed: serde_json::Value = match serde_json::from_str(&raw) { Ok(v) => v, Err(e) => { @@ -1993,7 +2054,7 @@ pub(crate) fn discover_teams_in_project_filtered( .as_ref() .and_then(|r| resolve_agent_path(&ac_root, r)); - teams.push(DiscoveredTeam { + discovered.push(DiscoveredTeam { name: team_name, project: project_folder.clone(), agent_names, @@ -2001,7 +2062,7 @@ pub(crate) fn discover_teams_in_project_filtered( coordinator_name, coordinator_path, }); - let pushed = teams.last().expect("just pushed"); + let pushed = discovered.last().expect("just pushed"); log::debug!( "[teams] discovered team — project='{}' team='{}' coord_name={:?} coord_path={:?} agent_count={}", pushed.project, @@ -2011,6 +2072,8 @@ pub(crate) fn discover_teams_in_project_filtered( pushed.agent_names.len() ); } + teams.extend(discovered); + Ok(()) } /// Reuse the existing classified test-only junction spawn site. @@ -2034,6 +2097,201 @@ pub(crate) fn create_test_junction(link: &Path, target: &Path) { #[cfg(test)] mod tests { + #[test] + fn e2992_duplicate_json_coordinator_checked_and_atomic_discovery() { + for side in 0..3 { + let (tmp, paths) = make_coordinator_fixture(false); + let ac_root = tmp.path().join("proj-a").join(".ac"); + let room = ac_root.join("wg-1-dev-team"); + let replica = room.join("__agent_tech-lead"); + let d = replica.join("config.json"); + let st = replica.join(crate::config::instance_artifacts::CONFIG_STATE_TARGET_NAME); + let team = ac_root.join("_team_dev-team").join("config.json"); + std::fs::write(&st, b"{}").unwrap(); + let files = [&d, &st, &team]; + std::fs::write( + files[side], + br#"{"unknown-canary":"value-canary","unknown-canary":0}"#, + ) + .unwrap(); + let before: Vec<_> = files + .iter() + .map(|path| std::fs::read(path).unwrap()) + .collect(); + for (resolver, error) in [ + ( + "resolve_wg_coordinator_replica_checked", + resolve_wg_coordinator_replica_checked(&ac_root, &room).unwrap_err(), + ), + ( + "verified_wg_coordinator_target_checked", + verified_wg_coordinator_target_checked( + "proj-a:wg-1-dev-team/tech-lead", + &paths, + ) + .unwrap_err(), + ), + ] { + assert!(error.contains("duplicateJsonMember:")); + assert!(error.contains(files[side].to_str().unwrap()), + "side={side} resolver={resolver} expected_physical_path={:?} returned_error={error:?}", files[side]); + assert!(!error.contains("canary")); + } + for (path, bytes) in files.iter().zip(before) { + assert_eq!(std::fs::read(path).unwrap(), bytes); + } + if side == 2 { + let mut destination = vec![DiscoveredTeam { + name: "existing".into(), + project: "existing".into(), + agent_names: vec![], + agent_paths: vec![], + coordinator_name: None, + coordinator_path: None, + }]; + assert!(discover_teams_in_project_filtered_checked( + &tmp.path().join("proj-a"), + &mut destination, + |_| true + ) + .is_err()); + assert_eq!(destination.len(), 1); + assert_eq!(destination[0].name, "existing"); + assert!(discover_teams_from_project_paths_checked(&paths).is_err()); + } + } + // Collapsed valid PAIR stages keep the coordinator pending. Corrupt one + // captured configuration image at a time without using a publisher that + // would correctly refuse the duplicate fixture. + for side in 0..2 { + for slot in 0..7 { + assert_pending_coordinator_duplicate_rejected(side, slot); + } + } + for invalid in [ + None, + Some("{"), + Some("[]"), + Some(r#"{"identity":"../../_agent_wrong"}"#), + ] { + let (tmp, _) = make_coordinator_fixture(false); + let ac_root = tmp.path().join("proj-a/.ac"); + let room = ac_root.join("wg-1-dev-team"); + let d = room.join("__agent_tech-lead/config.json"); + match invalid { + Some(bytes) => std::fs::write(&d, bytes).unwrap(), + None => std::fs::remove_file(&d).unwrap(), + } + assert!(resolve_wg_coordinator_replica_checked(&ac_root, &room) + .unwrap() + .is_none()); + } + let (io_tmp, _) = make_coordinator_fixture(false); + let io_root = io_tmp.path().join("proj-a/.ac"); + let io_room = io_root.join("wg-1-dev-team"); + let io_path = io_room.join("__agent_tech-lead/config.json"); + std::fs::remove_file(&io_path).unwrap(); + std::fs::create_dir(&io_path).unwrap(); + assert!(resolve_wg_coordinator_replica_checked(&io_root, &io_room) + .unwrap() + .is_none()); + let (tmp, _) = make_coordinator_fixture(false); + let ac_root = tmp.path().join("proj-a/.ac"); + assert!( + resolve_wg_coordinator_replica_checked(&ac_root, &ac_root.join("wg-1-dev-team")) + .unwrap() + .is_some() + ); + } + + fn assert_pending_coordinator_duplicate_rejected(side: usize, slot: usize) { + use crate::config::settings::{ + identity_reference_protected_revision, IdentityReferencePhysicalState, + }; + use sha2::{Digest, Sha256}; + let (tmp, paths) = make_coordinator_fixture(false); + let ac_root = tmp.path().join("proj-a").join(".ac"); + let room = ac_root.join("wg-1-dev-team"); + let replica = room.join("__agent_tech-lead"); + let (d, st, reservation) = crate::config::settings::identity_reference_pair_paths(&replica); + std::fs::write(&st, b"{}").unwrap(); + let files = [&d, &st]; + let original = [std::fs::read(&d).unwrap(), std::fs::read(&st).unwrap()]; + let tuple = original + .clone() + .map(IdentityReferencePhysicalState::from_bytes); + let protected = identity_reference_protected_revision(&tuple).unwrap(); + let protected_images = vec![protected; 5]; + let mappings = "a".repeat(64); + let digest = format!( + "{:x}", + Sha256::digest(serde_json::to_vec(&(1u32, &protected_images, &mappings)).unwrap()) + ); + let resolved = files.map(|path| { + let value = std::fs::canonicalize(path.parent().unwrap()) + .unwrap() + .join(path.file_name().unwrap()) + .to_string_lossy() + .into_owned(); + if cfg!(windows) { + value.to_lowercase() + } else { + value + } + }); + let target = format!( + "{:x}", + Sha256::digest(serde_json::to_vec(&resolved).unwrap()) + ); + let mut envelope = serde_json::json!({ + "version": 2, "operationId": "pending-coordinator", "sourcePhysicalKey": "fixture", + "planDigest": digest, "physicalTargetIdentity": target, "protectedPolicyVersion": 1, + "mappingsDigest": mappings, "beforeTuple": tuple, "protectedImages": protected_images, + "changedSides": [false, false], "timestampFloor": null, "activityIntent": null, + "planImages": {"stages": vec![tuple.clone(); 5]}, + }); + let control = serde_json::to_vec(&envelope).unwrap(); + std::fs::write(&reservation, &control).unwrap(); + assert!(resolve_wg_coordinator_replica_checked(&ac_root, &room) + .unwrap() + .is_none()); + assert_eq!(std::fs::read(&reservation).unwrap(), control); + assert_eq!(std::fs::read(&d).unwrap(), original[0]); + assert_eq!(std::fs::read(&st).unwrap(), original[1]); + let closing = original[side] + .iter() + .rposition(|byte| *byte == 0x7d) + .unwrap(); + let mut bytes = original[side][..closing].to_vec(); + if closing > 1 { + bytes.push(b','); + } + bytes.extend_from_slice(br#""unknown-canary":"secret-canary","unknown-canary":0"#); + bytes.push(0x7d); + let image = serde_json::to_value(IdentityReferencePhysicalState::from_bytes(bytes.clone())) + .unwrap(); + match slot { + 0 => std::fs::write(files[side], bytes).unwrap(), + 1 => envelope["beforeTuple"][side] = image, + _ => envelope["planImages"]["stages"][slot - 2][side] = image, + } + let packet = serde_json::to_vec(&envelope).unwrap(); + std::fs::write(&reservation, &packet).unwrap(); + let captured = [std::fs::read(&d).unwrap(), std::fs::read(&st).unwrap()]; + for error in [ + resolve_wg_coordinator_replica_checked(&ac_root, &room).unwrap_err(), + verified_wg_coordinator_target_checked("proj-a:wg-1-dev-team/tech-lead", &paths) + .unwrap_err(), + ] { + assert!(error.contains("duplicateJsonMember:")); + assert!(error.contains(files[side].to_str().unwrap())); + assert!(!error.contains("canary")); + } + assert_eq!(std::fs::read(&d).unwrap(), captured[0]); + assert_eq!(std::fs::read(&st).unwrap(), captured[1]); + assert_eq!(std::fs::read(&reservation).unwrap(), packet); + } + use super::*; #[test] diff --git a/src-tauri/src/path_identity.rs b/src-tauri/src/path_identity.rs index cbf071c07..baed300f9 100644 --- a/src-tauri/src/path_identity.rs +++ b/src-tauri/src/path_identity.rs @@ -1612,6 +1612,17 @@ pub fn scan_json_document(bytes: &[u8]) -> Result { }) } +/// Check duplicate members only; callers must still apply their native parser. +pub(crate) fn validate_ac_configuration_json(path: &Path, bytes: &[u8]) -> Result<(), String> { + if scan_json_document(bytes).is_ok_and(|document| document.duplicate_keys) { + return Err(format!( + "duplicateJsonMember: configuration file {} contains repeated JSON object members. Remove duplicate members from the same object and restart/retry. The file was left unchanged.", + path.display() + )); + } + Ok(()) +} + /// Parse JSON while rejecting duplicate object keys at every depth. pub fn parse_json_no_duplicates(bytes: &[u8]) -> Result { let document = scan_json_document(bytes)?; @@ -3056,3 +3067,39 @@ fn terminal_snapshot_atomic_publish_is_stably_unsupported_on_other_unix() { ); assert_eq!(result, Err("atomic_publish_unsupported".to_string())); } + +#[cfg(test)] +mod e2992_configuration_admission_tests { + use super::*; + + #[test] + fn e2992_duplicate_json_decoded_members_and_object_boundaries() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("config.json"); + for bytes in [ + br#"{"canary-key":"canary-value","canary-key":0}"#.as_slice(), + br#"{"x":0,"\u0078":1}"#, + br#"{"array":[{"unknown":0,"unknown":1}]}"#, + br#"{"parent":{},"parent":{}}"#, + ] { + std::fs::write(&path, bytes).unwrap(); + let message = validate_ac_configuration_json(&path, bytes).unwrap_err(); + assert!(message.starts_with("duplicateJsonMember:")); + assert!(message.contains(path.to_str().unwrap())); + assert!(message.contains("repeated JSON object members")); + assert!(!message.contains("canary")); + assert_eq!(std::fs::read(&path).unwrap(), bytes); + } + for bytes in [ + br#"{"a":{"x":0},"b":{"x":1}}"#.as_slice(), + br#"{"a":[{"x":0},{"x":1}]}"#, + br#"{"a.b":0,"a":{"b":1}}"#, + ] { + validate_ac_configuration_json(&path, bytes).unwrap(); + assert!(serde_json::from_slice::(bytes).is_ok()); + } + let malformed = b"{"; + validate_ac_configuration_json(&path, malformed).unwrap(); + assert!(serde_json::from_slice::(malformed).is_err()); + } +}