From e33ebb4860227935b41131026758c2336a53bbc8 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 10 Apr 2026 18:29:04 -0700 Subject: [PATCH] fix: restore deterministic project trust lookups Re-enable hook discovery on Windows so the trust-gating tests cover the real behavior instead of skipping it. Avoid alias-expanded project maps that can let a configured symlink alias satisfy the canonical project lookup; keep exact/case-normalized matching deterministic instead. Co-authored-by: Codex --- codex-rs/config/src/config_toml.rs | 35 ++++++++++---- codex-rs/core/src/codex_tests.rs | 2 - codex-rs/core/src/config/config_tests.rs | 24 ++++++++++ codex-rs/core/src/config_loader/mod.rs | 60 +++++++++++++----------- codex-rs/core/src/config_loader/tests.rs | 59 +++++++++++++++++++++++ codex-rs/hooks/src/engine/mod.rs | 11 ----- 6 files changed, 140 insertions(+), 51 deletions(-) diff --git a/codex-rs/config/src/config_toml.rs b/codex-rs/config/src/config_toml.rs index 4170dbafb3..ec87fbec27 100644 --- a/codex-rs/config/src/config_toml.rs +++ b/codex-rs/config/src/config_toml.rs @@ -658,16 +658,11 @@ impl ConfigToml { /// Resolves the cwd to an existing project, or returns None if ConfigToml /// does not contain a project corresponding to cwd or a git repo for cwd pub fn get_active_project(&self, resolved_cwd: &Path) -> Option { - let mut projects = HashMap::new(); - for (key, project_config) in self.projects.clone().unwrap_or_default() { - for normalized_key in normalized_project_lookup_keys(Path::new(&key)) { - projects.insert(normalized_key, project_config.clone()); - } - } + let projects = self.projects.as_ref()?; for normalized_cwd in normalized_project_lookup_keys(resolved_cwd) { - if let Some(project_config) = projects.get(&normalized_cwd) { - return Some(project_config.clone()); + if let Some(project_config) = project_config_for_lookup_key(projects, &normalized_cwd) { + return Some(project_config); } } @@ -676,8 +671,10 @@ impl ConfigToml { // worktrees inherit trust from the main project. if let Some(repo_root) = resolve_root_git_project_for_trust(resolved_cwd) { for normalized_repo_root in normalized_project_lookup_keys(&repo_root) { - if let Some(project_config_for_root) = projects.get(&normalized_repo_root) { - return Some(project_config_for_root.clone()); + if let Some(project_config_for_root) = + project_config_for_lookup_key(projects, &normalized_repo_root) + { + return Some(project_config_for_root); } } } @@ -733,6 +730,24 @@ fn normalize_project_lookup_key(key: String) -> String { } } +fn project_config_for_lookup_key( + projects: &HashMap, + lookup_key: &str, +) -> Option { + if let Some(project_config) = projects.get(lookup_key) { + return Some(project_config.clone()); + } + + let mut normalized_matches: Vec<_> = projects + .iter() + .filter(|(key, _)| normalize_project_lookup_key((*key).clone()) == lookup_key) + .collect(); + normalized_matches.sort_by(|(left, _), (right, _)| left.cmp(right)); + normalized_matches + .first() + .map(|(_, project_config)| (**project_config).clone()) +} + pub fn validate_reserved_model_provider_ids( model_providers: &HashMap, ) -> Result<(), String> { diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 550b606c40..f18d2d3983 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -5622,7 +5622,6 @@ async fn unified_exec_rejects_escalated_permissions_when_policy_not_on_request() pretty_assertions::assert_eq!(output, expected); } -#[cfg_attr(windows, ignore)] #[tokio::test] async fn session_start_hooks_only_load_from_trusted_project_layers() -> std::io::Result<()> { let temp = tempfile::tempdir()?; @@ -5657,7 +5656,6 @@ async fn session_start_hooks_only_load_from_trusted_project_layers() -> std::io: Ok(()) } -#[cfg_attr(windows, ignore)] #[tokio::test] async fn session_start_hooks_require_project_trust_without_config_toml() -> std::io::Result<()> { let temp = tempfile::tempdir()?; diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index b7a6a18457..22a6eb7eb9 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -5136,6 +5136,30 @@ trust_level = "trusted" Ok(()) } +#[cfg(unix)] +#[test] +fn active_project_does_not_match_configured_alias_for_canonical_cwd() -> anyhow::Result<()> { + let tmp = tempdir()?; + let project_root = tmp.path().join("project"); + let alias_root = tmp.path().join("project_alias"); + std::fs::create_dir_all(&project_root)?; + std::os::unix::fs::symlink(&project_root, &alias_root)?; + + let config = ConfigToml { + projects: Some(HashMap::from([( + alias_root.to_string_lossy().to_string(), + ProjectConfig { + trust_level: Some(TrustLevel::Trusted), + }, + )])), + ..Default::default() + }; + + assert_eq!(config.get_active_project(&project_root), None); + + Ok(()) +} + #[test] fn test_set_default_oss_provider() -> std::io::Result<()> { let temp_dir = TempDir::new()?; diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 63d59df2eb..5dec3462a2 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -564,29 +564,35 @@ impl ProjectTrustDecision { impl ProjectTrustContext { fn decision_for_dir(&self, dir: &AbsolutePathBuf) -> ProjectTrustDecision { for dir_key in normalized_project_trust_keys(dir.as_path()) { - if let Some(trust_level) = self.projects_trust.get(&dir_key).copied() { + if let Some((trust_key, trust_level)) = + project_trust_for_lookup_key(&self.projects_trust, &dir_key) + { return ProjectTrustDecision { trust_level: Some(trust_level), - trust_key: dir_key, + trust_key, }; } } for project_root_key in &self.project_root_lookup_keys { - if let Some(trust_level) = self.projects_trust.get(project_root_key).copied() { + if let Some((trust_key, trust_level)) = + project_trust_for_lookup_key(&self.projects_trust, project_root_key) + { return ProjectTrustDecision { trust_level: Some(trust_level), - trust_key: project_root_key.clone(), + trust_key, }; } } if let Some(repo_root_lookup_keys) = self.repo_root_lookup_keys.as_ref() { for repo_root_key in repo_root_lookup_keys { - if let Some(trust_level) = self.projects_trust.get(repo_root_key).copied() { + if let Some((trust_key, trust_level)) = + project_trust_for_lookup_key(&self.projects_trust, repo_root_key) + { return ProjectTrustDecision { trust_level: Some(trust_level), - trust_key: repo_root_key.clone(), + trust_key, }; } } @@ -669,16 +675,7 @@ async fn project_trust_context( let projects_trust = projects .into_iter() - .flat_map(|(key, project)| { - project - .trust_level - .into_iter() - .flat_map(move |trust_level| { - normalized_project_trust_key_strs(&key) - .into_iter() - .map(move |normalized_key| (normalized_key, trust_level)) - }) - }) + .filter_map(|(key, project)| project.trust_level.map(|trust_level| (key, trust_level))) .collect(); Ok(ProjectTrustContext { @@ -714,17 +711,6 @@ fn normalized_project_trust_keys(path: &Path) -> Vec { } } -fn normalized_project_trust_key_strs(path: &str) -> Vec { - let path = Path::new(path); - if path.is_absolute() { - normalized_project_trust_keys(path) - } else { - vec![normalize_project_trust_lookup_key( - path.to_string_lossy().to_string(), - )] - } -} - fn normalize_project_trust_lookup_key(key: String) -> String { if cfg!(windows) { key.to_ascii_lowercase() @@ -733,10 +719,28 @@ fn normalize_project_trust_lookup_key(key: String) -> String { } } /// Convert the path to the canonical trust key we store and compare against. -/// On Windows this also normalizes case so alias paths still match. +/// On Windows this also normalizes case so lookups remain case-insensitive. pub fn project_trust_key(project_path: &Path) -> String { normalized_project_trust_key(project_path) } + +fn project_trust_for_lookup_key( + projects_trust: &std::collections::HashMap, + lookup_key: &str, +) -> Option<(String, TrustLevel)> { + if let Some(trust_level) = projects_trust.get(lookup_key).copied() { + return Some((lookup_key.to_string(), trust_level)); + } + + let mut normalized_matches: Vec<_> = projects_trust + .iter() + .filter(|(key, _)| normalize_project_trust_lookup_key((*key).clone()) == lookup_key) + .collect(); + normalized_matches.sort_by(|(left, _), (right, _)| left.cmp(right)); + normalized_matches + .first() + .map(|(key, trust_level)| ((**key).clone(), **trust_level)) +} /// Takes a `toml::Value` parsed from a config.toml file and walks through it, /// resolving any `AbsolutePathBuf` fields against `base_dir`, returning a new /// `toml::Value` with the same shape but with paths resolved. diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 060efeb599..b1e6ab1703 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -1202,6 +1202,65 @@ async fn project_layers_disabled_when_untrusted_or_unknown() -> std::io::Result< Ok(()) } +#[cfg(unix)] +#[tokio::test] +async fn project_trust_does_not_match_configured_alias_for_canonical_cwd() -> std::io::Result<()> { + let tmp = tempdir()?; + let project_root = tmp.path().join("project"); + let alias_root = tmp.path().join("project_alias"); + tokio::fs::create_dir_all(project_root.join(".codex")).await?; + tokio::fs::write(project_root.join(".git"), "gitdir: here").await?; + tokio::fs::write( + project_root.join(".codex").join(CONFIG_TOML_FILE), + "foo = \"project\"\n", + ) + .await?; + std::os::unix::fs::symlink(&project_root, &alias_root)?; + + let codex_home = tmp.path().join("home"); + tokio::fs::create_dir_all(&codex_home).await?; + tokio::fs::write( + codex_home.join(CONFIG_TOML_FILE), + toml::to_string(&ConfigToml { + projects: Some(HashMap::from([( + alias_root.to_string_lossy().to_string(), + ProjectConfig { + trust_level: Some(TrustLevel::Trusted), + }, + )])), + ..Default::default() + }) + .expect("serialize config"), + ) + .await?; + + let layers = load_config_layers_state( + &codex_home, + Some(AbsolutePathBuf::from_absolute_path(&project_root)?), + &[] as &[(String, TomlValue)], + LoaderOverrides::default(), + CloudRequirementsLoader::default(), + ) + .await?; + + let project_layers: Vec<_> = layers + .get_layers( + super::ConfigLayerStackOrdering::HighestPrecedenceFirst, + /*include_disabled*/ true, + ) + .into_iter() + .filter(|layer| matches!(layer.name, super::ConfigLayerSource::Project { .. })) + .collect(); + assert_eq!(project_layers.len(), 1); + assert!( + project_layers[0].disabled_reason.is_some(), + "configured aliases must not collapse into the canonical project key" + ); + assert_eq!(layers.effective_config().get("foo"), None); + + Ok(()) +} + #[tokio::test] async fn cli_override_can_update_project_local_mcp_server_when_project_is_trusted() -> std::io::Result<()> { diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index f91fee24c9..b6ae4a365e 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -80,17 +80,6 @@ impl ClaudeHooksEngine { }; } - if cfg!(windows) { - return Self { - handlers: Vec::new(), - warnings: vec![ - "Disabled `codex_hooks` for this session because `hooks.json` lifecycle hooks are not supported on Windows yet." - .to_string(), - ], - shell, - }; - } - let _ = schema_loader::generated_hook_schemas(); let discovered = discovery::discover_handlers(config_layer_stack); Self {