fix(pi): close native inspection review gaps

This commit is contained in:
SaladDay
2026-08-02 16:40:45 +00:00
parent d2ad7995a3
commit 9fffc8853d
4 changed files with 331 additions and 52 deletions
+5
View File
@@ -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"]),
];
+189 -27
View File
@@ -404,25 +404,81 @@ fn rust_sources(root: &Path) -> Vec<PathBuf> {
output
}
fn module_path_attribute(attrs: &[Attribute]) -> Option<PathBuf> {
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<PathBuf>,
definitely_explicit: bool,
}
fn collect_path_module_targets(items: &[Item], source_path: &Path, output: &mut Vec<PathBuf>) {
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::<Meta, Token![,]>::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<PathBuf>,
) {
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:?}"
);
}
}
+83 -25
View File
@@ -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<String, JavaScriptSpreadValue>;
/// 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<Map<String, Value>, 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<Map<String, Value>, 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
}
})))
);
}
}
+54
View File
@@ -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");