From 9fffc8853dce4d60dc816e4a849deabaa672e823 Mon Sep 17 00:00:00 2001 From: SaladDay Date: Sun, 2 Aug 2026 16:40:45 +0000 Subject: [PATCH] fix(pi): close native inspection review gaps --- scripts/pi-transport-capture.mjs | 5 + src-tauri/src/architecture_tests.rs | 216 ++++++++++++++++++++++++---- src-tauri/src/pi_config/mod.rs | 108 ++++++++++---- src-tauri/src/pi_config/native.rs | 54 +++++++ 4 files changed, 331 insertions(+), 52 deletions(-) diff --git a/scripts/pi-transport-capture.mjs b/scripts/pi-transport-capture.mjs index c17b996d1..cda83ec91 100644 --- a/scripts/pi-transport-capture.mjs +++ b/scripts/pi-transport-capture.mjs @@ -322,6 +322,11 @@ function captureCompatSpread(label, baseValue, overlayValue) { const compatEdgeCases = [ captureCompatSpread("ascii-string-to-string", "ab", "cd"), captureCompatSpread("astral-string-to-object", "😀", { named: true }), + captureCompatSpread("astral-string-fully-overridden", "😀", { + 0: "repaired-high", + 1: "repaired-low", + named: true, + }), captureCompatSpread("string-to-array", "ab", ["first", "second"]), ]; diff --git a/src-tauri/src/architecture_tests.rs b/src-tauri/src/architecture_tests.rs index 87718db65..b6d06cf82 100644 --- a/src-tauri/src/architecture_tests.rs +++ b/src-tauri/src/architecture_tests.rs @@ -404,25 +404,81 @@ fn rust_sources(root: &Path) -> Vec { output } -fn module_path_attribute(attrs: &[Attribute]) -> Option { - attrs.iter().find_map(|attribute| { - if !attribute.path().is_ident("path") { - return None; - } - let Meta::NameValue(value) = &attribute.meta else { - return None; - }; - let syn::Expr::Lit(expression) = &value.value else { - return None; - }; - let Lit::Str(path) = &expression.lit else { - return None; - }; - Some(PathBuf::from(path.value())) - }) +#[derive(Debug, Default)] +struct ModulePathOptions { + explicit: Vec, + definitely_explicit: bool, } -fn collect_path_module_targets(items: &[Item], source_path: &Path, output: &mut Vec) { +impl ModulePathOptions { + fn default_path_is_possible(&self) -> bool { + !self.definitely_explicit + } +} + +fn collect_conditional_module_paths( + meta: &Meta, + activation: CfgTruth, + output: &mut ModulePathOptions, +) { + match meta { + Meta::NameValue(value) if value.path.is_ident("path") => { + let syn::Expr::Lit(expression) = &value.value else { + return; + }; + let Lit::Str(path) = &expression.lit else { + return; + }; + if activation != CfgTruth::False { + output.explicit.push(PathBuf::from(path.value())); + output.definitely_explicit |= activation == CfgTruth::True; + } + } + Meta::List(list) if list.path.is_ident("cfg_attr") => { + let Ok(items) = list + .parse_args_with(syn::punctuated::Punctuated::::parse_terminated) + else { + return; + }; + let mut items = items.iter(); + let Some(predicate) = items.next() else { + return; + }; + let activation = activation.and(production_cfg_truth(predicate)); + for attribute in items { + collect_conditional_module_paths(attribute, activation, output); + } + } + _ => {} + } +} + +fn module_path_options(attrs: &[Attribute]) -> ModulePathOptions { + let mut output = ModulePathOptions::default(); + for attribute in attrs { + collect_conditional_module_paths(&attribute.meta, CfgTruth::True, &mut output); + } + output.explicit.sort(); + output.explicit.dedup(); + output +} + +fn default_submodule_directory(source_path: &Path) -> PathBuf { + let parent = source_path + .parent() + .expect("Rust source has a parent directory"); + match source_path.file_stem().and_then(|stem| stem.to_str()) { + Some("lib" | "main" | "mod") | None => parent.to_path_buf(), + Some(stem) => parent.join(stem), + } +} + +fn collect_module_targets( + items: &[Item], + source_path: &Path, + inline_modules: &[String], + output: &mut Vec, +) { for item in items { let Item::Mod(module) = item else { continue; @@ -430,16 +486,37 @@ fn collect_path_module_targets(items: &[Item], source_path: &Path, output: &mut if is_cfg_test(&module.attrs) { continue; } - if let Some(relative) = module_path_attribute(&module.attrs) { - output.push( - source_path - .parent() - .expect("Rust source has a parent directory") - .join(relative), - ); - } if let Some((_, nested)) = &module.content { - collect_path_module_targets(nested, source_path, output); + let mut nested_modules = inline_modules.to_vec(); + nested_modules.push(module.ident.to_string()); + collect_module_targets(nested, source_path, &nested_modules, output); + continue; + } + + let options = module_path_options(&module.attrs); + let module_directory = default_submodule_directory(source_path); + let inline_directory = inline_modules + .iter() + .fold(module_directory, |directory, module| directory.join(module)); + let explicit_base = if inline_modules.is_empty() { + source_path + .parent() + .expect("Rust source has a parent directory") + .to_path_buf() + } else { + inline_directory.clone() + }; + let default_path_is_possible = options.default_path_is_possible(); + output.extend( + options + .explicit + .into_iter() + .map(|relative| explicit_base.join(relative)), + ); + if default_path_is_possible { + let module_name = module.ident.to_string(); + output.push(inline_directory.join(format!("{module_name}.rs"))); + output.push(inline_directory.join(module_name).join("mod.rs")); } } } @@ -472,7 +549,7 @@ fn inherited_module_owners( continue; } let mut targets = Vec::new(); - collect_path_module_targets(&syntax.items, path, &mut targets); + collect_module_targets(&syntax.items, path, &[], &mut targets); for target in targets { let Ok(target) = fs::canonicalize(target) else { continue; @@ -836,4 +913,89 @@ fn architecture_scanner_negative_fixtures_prove_each_guard_fires() { {expected_kind}: {violations:?}" ); } + + let conditional_path_root = + tempfile::tempdir().expect("temp conditional-path architecture tree"); + let composer = conditional_path_root + .path() + .join("src/pi_config/composer.rs"); + let shared = conditional_path_root.path().join("src/shared/helper.rs"); + fs::create_dir_all(composer.parent().expect("composer parent")).expect("create pi_config"); + fs::create_dir_all(shared.parent().expect("shared parent")).expect("create shared"); + fs::write( + &composer, + r#" + #[cfg_attr(not(test), path = "../shared/helper.rs")] + mod helper; + "#, + ) + .expect("write conditional-path parent"); + fs::write( + &shared, + r#" + use crate::pi_config::gateway::PiGatewayApiFamily; + fn production_escape_attempt() { + save_provider(); + } + "#, + ) + .expect("write conditional-path child"); + let (violations, _) = scan_production_tree( + conditional_path_root.path(), + &conditional_path_root.path().join("src"), + ); + for expected_kind in ["cross_layer_import", "forbidden_provider_symbol"] { + assert!( + violations.iter().any(|violation| { + violation.kind == expected_kind && violation.path.ends_with("src/shared/helper.rs") + }), + "production cfg_attr(path) modules must inherit composer ownership and trigger \ + {expected_kind}: {violations:?}" + ); + } + + let transitive_path_root = tempfile::tempdir().expect("temp transitive-path architecture tree"); + let composer = transitive_path_root + .path() + .join("src/pi_config/composer.rs"); + let helper = transitive_path_root.path().join("src/shared/helper.rs"); + let leaf = transitive_path_root + .path() + .join("src/shared/helper/leaf.rs"); + fs::create_dir_all(composer.parent().expect("composer parent")).expect("create pi_config"); + fs::create_dir_all(helper.parent().expect("helper parent")).expect("create shared"); + fs::create_dir_all(leaf.parent().expect("leaf parent")).expect("create helper module"); + fs::write( + &composer, + r#" + #[path = "../shared/helper.rs"] + mod helper; + "#, + ) + .expect("write transitive parent"); + fs::write(&helper, "mod leaf;").expect("write transitive child"); + fs::write( + &leaf, + r#" + use crate::pi_config::gateway::PiGatewayApiFamily; + fn production_escape_attempt() { + save_provider(); + } + "#, + ) + .expect("write transitive leaf"); + let (violations, _) = scan_production_tree( + transitive_path_root.path(), + &transitive_path_root.path().join("src"), + ); + for expected_kind in ["cross_layer_import", "forbidden_provider_symbol"] { + assert!( + violations.iter().any(|violation| { + violation.kind == expected_kind + && violation.path.ends_with("src/shared/helper/leaf.rs") + }), + "ordinary descendants of #[path] modules must inherit composer ownership and \ + trigger {expected_kind}: {violations:?}" + ); + } } diff --git a/src-tauri/src/pi_config/mod.rs b/src-tauri/src/pi_config/mod.rs index a47eb74bd..d8f0ac931 100644 --- a/src-tauri/src/pi_config/mod.rs +++ b/src-tauri/src/pi_config/mod.rs @@ -4,6 +4,7 @@ //! Pi's shared files and from the proxy data plane. Callers must use the //! typed model resolver rather than reimplementing provider/model inheritance. +use indexmap::IndexMap; use serde_json::{Map, Value}; mod composer; @@ -24,6 +25,14 @@ const PI_COMPAT_NESTED_SPREAD_KEYS: [&str; 3] = [ #[derive(Debug, Clone, Copy, PartialEq, Eq)] struct PiCompatMergeError; +#[derive(Debug, Clone, PartialEq)] +enum JavaScriptSpreadValue { + Json(Value), + LoneSurrogate, +} + +type JavaScriptSpreadMap = IndexMap; + /// Mirror pinned Pi's `mergeCompat` JavaScript object-spread semantics. /// /// Arrays expose numeric enumerable properties, strings expose character @@ -47,8 +56,8 @@ fn merge_pi_compat( return Ok(base); } - let mut merged = javascript_object_spread(base.as_ref())?; - merged.extend(javascript_object_spread(Some(&overlay))?); + let mut merged = javascript_object_spread(base.as_ref()); + merged.extend(javascript_object_spread(Some(&overlay))); for key in PI_COMPAT_NESTED_SPREAD_KEYS { let base_value = javascript_property(base.as_ref(), key); @@ -56,12 +65,19 @@ fn merge_pi_compat( if base_value.is_some_and(javascript_is_object) || overlay_value.is_some_and(javascript_is_object) { - let mut nested = javascript_object_spread(base_value)?; - nested.extend(javascript_object_spread(overlay_value)?); - merged.insert(key.to_string(), Value::Object(nested)); + let mut nested = javascript_object_spread(base_value); + nested.extend(javascript_object_spread(overlay_value)); + merged.insert( + key.to_string(), + JavaScriptSpreadValue::Json(Value::Object(finish_javascript_object_spread( + nested, + )?)), + ); } } - Ok(Some(Value::Object(merged))) + Ok(Some(Value::Object(finish_javascript_object_spread( + merged, + )?))) } fn javascript_truthy(value: &Value) -> bool { @@ -81,29 +97,48 @@ fn javascript_property<'a>(value: Option<&'a Value>, key: &str) -> Option<&'a Va value.and_then(Value::as_object)?.get(key) } -fn javascript_object_spread( - value: Option<&Value>, -) -> Result, PiCompatMergeError> { - let spread = match value { - Some(Value::Object(object)) => object.clone(), +fn javascript_object_spread(value: Option<&Value>) -> JavaScriptSpreadMap { + match value { + Some(Value::Object(object)) => object + .iter() + .map(|(key, value)| (key.clone(), JavaScriptSpreadValue::Json(value.clone()))) + .collect(), Some(Value::Array(values)) => values .iter() .enumerate() - .map(|(index, value)| (index.to_string(), value.clone())) + .map(|(index, value)| { + ( + index.to_string(), + JavaScriptSpreadValue::Json(value.clone()), + ) + }) .collect(), - Some(Value::String(value)) => { - if value.chars().any(|character| character.len_utf16() != 1) { - return Err(PiCompatMergeError); - } - value - .chars() - .enumerate() - .map(|(index, value)| (index.to_string(), Value::String(value.to_string()))) - .collect() - } - Some(Value::Null | Value::Bool(_) | Value::Number(_)) | None => Map::new(), - }; - Ok(spread) + Some(Value::String(value)) => value + .encode_utf16() + .enumerate() + .map(|(index, unit)| { + let value = char::from_u32(u32::from(unit)) + .map(|character| { + JavaScriptSpreadValue::Json(Value::String(character.to_string())) + }) + .unwrap_or(JavaScriptSpreadValue::LoneSurrogate); + (index.to_string(), value) + }) + .collect(), + Some(Value::Null | Value::Bool(_) | Value::Number(_)) | None => IndexMap::new(), + } +} + +fn finish_javascript_object_spread( + spread: JavaScriptSpreadMap, +) -> Result, PiCompatMergeError> { + spread + .into_iter() + .map(|(key, value)| match value { + JavaScriptSpreadValue::Json(value) => Ok((key, value)), + JavaScriptSpreadValue::LoneSurrogate => Err(PiCompatMergeError), + }) + .collect() } #[cfg(test)] @@ -155,4 +190,27 @@ mod compat_spread_tests { Err(PiCompatMergeError) ); } + + #[test] + fn compat_spread_checks_surrogates_after_later_properties_override_them() { + assert_eq!( + merge_pi_compat( + Some(json!({"chatTemplateKwargs": "😀"})), + Some(json!({ + "chatTemplateKwargs": { + "0": "repaired-high", + "1": "repaired-low", + "named": true + } + })), + ), + Ok(Some(json!({ + "chatTemplateKwargs": { + "0": "repaired-high", + "1": "repaired-low", + "named": true + } + }))) + ); + } } diff --git a/src-tauri/src/pi_config/native.rs b/src-tauri/src/pi_config/native.rs index 4f270300b..884ad09ba 100644 --- a/src-tauri/src/pi_config/native.rs +++ b/src-tauri/src/pi_config/native.rs @@ -813,6 +813,60 @@ mod tests { )); } + #[test] + fn public_inspection_accepts_surrogates_overridden_before_the_final_result() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("models.json"); + fs::write( + &path, + r#"{"providers":{"surrogate":{ + "api":"openai-responses", + "baseUrl":"https://compat.example/v1", + "apiKey":"literal", + "compat":{"chatTemplateKwargs":"😀"}, + "models":[{"id":"m"}], + "modelOverrides":{"m":{"compat":{"chatTemplateKwargs":{ + "0":"repaired-high", + "1":"repaired-low", + "named":true + }}}} +}}}"#, + ) + .expect("write"); + + let inspection = + PiNativeInspectionService::inspect_entry(&path, "surrogate", &BTreeMap::new()) + .unwrap() + .expect("provider"); + assert_eq!( + inspection.diagnostic.managed_assessment, + PiManagedAssessment::Manageable + ); + assert_eq!( + inspection.diagnostic.composition_status, + PiCompositionStatus::Composed + ); + assert_eq!( + inspection.diagnostic.gateway_status, + PiGatewayStatus::Proxyable + ); + assert!(!inspection + .diagnostic + .reasons + .iter() + .any(|reason| reason.code == PiReasonCode::UnrepresentableCompat)); + assert_eq!( + inspection.composition.models[0].compat, + Some(json!({ + "chatTemplateKwargs": { + "0": "repaired-high", + "1": "repaired-low", + "named": true + } + })) + ); + } + #[test] fn managed_rejection_does_not_control_raw_composition() { let temp = tempfile::tempdir().expect("tempdir");