Skip to content

Commit d92c909

Browse files
authored
Fix migrated hook path rewriting (#20144)
## Summary - Rewrite migrated external-agent hook commands by replacing the full hook script path token instead of only the `.claude/hooks/` segment. - Preserve quoting around the full rewritten target path so script names with spaces, absolute paths, and shell operators/redirection continue to work. - Apply `.claude/settings.local.json` over `.claude/settings.json` for config, MCP, and plugin migration so local scope matches Claude settings precedence. - Skip legacy command markdown without `description` frontmatter, including README-style docs under `.claude/commands`. ## Root Cause The previous hook rewrite handled `.claude/hooks/` as a substring replacement. For absolute source commands, that left the original project-root prefix before the newly quoted `.codex/hooks` directory, producing invalid commands like `project/'project/.codex/hooks'/script.sh`. The migration also only used project `settings.json` for config/MCP/plugin decisions, so local settings such as `disabledMcpjsonServers` could be ignored even though Claude gives local settings higher precedence than project settings. ## Validation - `just fmt` - `cargo test -p codex-external-agent-migration` - `cargo test -p codex-app-server external_agent_config` - `just fix -p codex-external-agent-migration` - `just fix -p codex-app-server` - `git diff --check`
1 parent 5597925 commit d92c909

3 files changed

Lines changed: 521 additions & 85 deletions

File tree

codex-rs/app-server/src/config/external_agent_config.rs

Lines changed: 48 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ impl ExternalAgentConfigService {
252252
|| self.external_agent_home.join("settings.json"),
253253
|repo_root| repo_root.join(EXTERNAL_AGENT_DIR).join("settings.json"),
254254
);
255-
let settings = read_external_settings(&source_settings)?;
255+
let settings = effective_external_settings(&source_settings)?;
256256
let target_config = repo_root.map_or_else(
257257
|| self.codex_home.join("config.toml"),
258258
|repo_root| repo_root.join(".codex").join("config.toml"),
@@ -569,7 +569,7 @@ impl ExternalAgentConfigService {
569569
) -> io::Result<Option<JsonValue>> {
570570
if repo_root.is_some() && source_settings.is_none() {
571571
let home_settings = self.external_agent_home.join("settings.json");
572-
match read_external_settings(&home_settings) {
572+
match effective_external_settings(&home_settings) {
573573
Ok(settings) => Ok(settings),
574574
Err(err) => {
575575
tracing::warn!(
@@ -636,7 +636,7 @@ impl ExternalAgentConfigService {
636636
|cwd| cwd.join(EXTERNAL_AGENT_DIR).join("settings.json"),
637637
);
638638
let source_root = cwd.unwrap_or(self.external_agent_home.as_path());
639-
let import_sources = read_external_settings(&source_settings)?
639+
let import_sources = effective_external_settings(&source_settings)?
640640
.map(|settings| collect_marketplace_import_sources(&settings, source_root))
641641
.unwrap_or_default();
642642

@@ -697,9 +697,11 @@ impl ExternalAgentConfigService {
697697
|cwd| cwd.join(EXTERNAL_AGENT_DIR).join("settings.json"),
698698
);
699699
let source_root = cwd.unwrap_or(self.external_agent_home.as_path());
700-
let import_source = read_external_settings(&source_settings)?.and_then(|settings| {
701-
collect_marketplace_import_sources(&settings, source_root).remove(&marketplace_name)
702-
});
700+
let import_source =
701+
effective_external_settings(&source_settings)?.and_then(|settings| {
702+
collect_marketplace_import_sources(&settings, source_root)
703+
.remove(&marketplace_name)
704+
});
703705
let Some(import_source) = import_source else {
704706
outcome.failed_marketplaces.push(marketplace_name);
705707
outcome.failed_plugin_ids.extend(plugin_ids);
@@ -767,13 +769,9 @@ impl ExternalAgentConfigService {
767769
self.codex_home.join("config.toml"),
768770
)
769771
};
770-
if !source_settings.is_file() {
772+
let Some(settings) = effective_external_settings(&source_settings)? else {
771773
return Ok(());
772-
}
773-
774-
let raw_settings = fs::read_to_string(&source_settings)?;
775-
let settings: JsonValue = serde_json::from_str(&raw_settings)
776-
.map_err(|err| invalid_data_error(err.to_string()))?;
774+
};
777775
let migrated = build_config_from_external(&settings)?;
778776
if is_empty_toml_table(&migrated) {
779777
return Ok(());
@@ -822,7 +820,7 @@ impl ExternalAgentConfigService {
822820
};
823821
let settings = self.mcp_settings(
824822
repo_root.as_deref(),
825-
read_external_settings(&source_settings)?,
823+
effective_external_settings(&source_settings)?,
826824
)?;
827825
let migrated = build_mcp_config_from_external(
828826
self.source_root(repo_root.as_deref()).as_path(),
@@ -999,6 +997,43 @@ fn read_external_settings(path: &Path) -> io::Result<Option<JsonValue>> {
999997
Ok(Some(settings))
1000998
}
1001999

1000+
fn effective_external_settings(project_settings: &Path) -> io::Result<Option<JsonValue>> {
1001+
let mut effective = read_external_settings(project_settings)?;
1002+
let Some(settings_dir) = project_settings.parent() else {
1003+
return Ok(effective);
1004+
};
1005+
let local_settings = settings_dir.join("settings.local.json");
1006+
let local_settings = match read_external_settings(&local_settings) {
1007+
Ok(Some(local_settings)) => local_settings,
1008+
Ok(None) => return Ok(effective),
1009+
Err(err) if err.kind() == io::ErrorKind::InvalidData => return Ok(effective),
1010+
Err(err) => return Err(err),
1011+
};
1012+
if let Some(effective) = effective.as_mut() {
1013+
merge_json_settings(effective, &local_settings);
1014+
} else {
1015+
effective = Some(local_settings);
1016+
}
1017+
Ok(effective)
1018+
}
1019+
1020+
fn merge_json_settings(existing: &mut JsonValue, incoming: &JsonValue) {
1021+
match (existing, incoming) {
1022+
(JsonValue::Object(existing), JsonValue::Object(incoming)) => {
1023+
for (key, incoming_value) in incoming {
1024+
match existing.get_mut(key) {
1025+
Some(existing_value) => merge_json_settings(existing_value, incoming_value),
1026+
None => {
1027+
existing.insert(key.clone(), incoming_value.clone());
1028+
}
1029+
}
1030+
}
1031+
}
1032+
(existing, incoming) => {
1033+
*existing = incoming.clone();
1034+
}
1035+
}
1036+
}
10021037
fn extract_plugin_migration_details(
10031038
settings: &JsonValue,
10041039
source_root: &Path,

codex-rs/app-server/src/config/external_agent_config_tests.rs

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -707,6 +707,68 @@ async fn import_home_migrates_supported_config_fields_skills_and_agents_md() {
707707
);
708708
}
709709

710+
#[tokio::test]
711+
async fn import_home_config_uses_local_settings_over_project_settings() {
712+
let (_root, external_agent_home, codex_home) = fixture_paths();
713+
fs::create_dir_all(&external_agent_home).expect("create external agent home");
714+
fs::write(
715+
external_agent_home.join("settings.json"),
716+
r#"{"env":{"FOO":"project","PROJECT_ONLY":"yes"},"sandbox":{"enabled":false}}"#,
717+
)
718+
.expect("write project settings");
719+
fs::write(
720+
external_agent_home.join("settings.local.json"),
721+
r#"{"env":{"FOO":"local","LOCAL_ONLY":true},"sandbox":{"enabled":true}}"#,
722+
)
723+
.expect("write local settings");
724+
725+
service_for_paths(external_agent_home, codex_home.clone())
726+
.import(vec![ExternalAgentConfigMigrationItem {
727+
item_type: ExternalAgentConfigMigrationItemType::Config,
728+
description: String::new(),
729+
cwd: None,
730+
details: None,
731+
}])
732+
.await
733+
.expect("import");
734+
735+
assert_eq!(
736+
fs::read_to_string(codex_home.join("config.toml")).expect("read config"),
737+
"sandbox_mode = \"workspace-write\"\n\n[shell_environment_policy]\ninherit = \"core\"\n\n[shell_environment_policy.set]\nFOO = \"local\"\nLOCAL_ONLY = \"true\"\nPROJECT_ONLY = \"yes\"\n"
738+
);
739+
}
740+
741+
#[tokio::test]
742+
async fn import_home_config_ignores_invalid_local_settings() {
743+
let (_root, external_agent_home, codex_home) = fixture_paths();
744+
fs::create_dir_all(&external_agent_home).expect("create external agent home");
745+
fs::write(
746+
external_agent_home.join("settings.json"),
747+
r#"{"env":{"FOO":"project"},"sandbox":{"enabled":false}}"#,
748+
)
749+
.expect("write project settings");
750+
fs::write(
751+
external_agent_home.join("settings.local.json"),
752+
"{invalid json",
753+
)
754+
.expect("write local settings");
755+
756+
service_for_paths(external_agent_home, codex_home.clone())
757+
.import(vec![ExternalAgentConfigMigrationItem {
758+
item_type: ExternalAgentConfigMigrationItemType::Config,
759+
description: String::new(),
760+
cwd: None,
761+
details: None,
762+
}])
763+
.await
764+
.expect("import");
765+
766+
assert_eq!(
767+
fs::read_to_string(codex_home.join("config.toml")).expect("read config"),
768+
"[shell_environment_policy]\ninherit = \"core\"\n\n[shell_environment_policy.set]\nFOO = \"project\"\n"
769+
);
770+
}
771+
710772
#[tokio::test]
711773
async fn import_home_skips_empty_config_migration() {
712774
let (_root, external_agent_home, codex_home) = fixture_paths();
@@ -1144,6 +1206,67 @@ command = "allowed-server"
11441206
assert_eq!(config, expected);
11451207
}
11461208

1209+
#[tokio::test]
1210+
async fn import_repo_mcp_uses_local_settings_toggles_over_project_settings() {
1211+
let root = TempDir::new().expect("create tempdir");
1212+
let repo_root = root.path().join("repo");
1213+
let external_agent_home = root.path().join(EXTERNAL_AGENT_DIR);
1214+
fs::create_dir_all(repo_root.join(".git")).expect("create git dir");
1215+
fs::create_dir_all(repo_root.join(EXTERNAL_AGENT_DIR)).expect("create external agent dir");
1216+
fs::write(
1217+
repo_root.join(".mcp.json"),
1218+
r#"{
1219+
"mcpServers": {
1220+
"project-disabled": {"command": "project-disabled-server"},
1221+
"local-disabled": {"command": "local-disabled-server"},
1222+
"local-enabled": {"command": "local-enabled-server"}
1223+
}
1224+
}"#,
1225+
)
1226+
.expect("write mcp");
1227+
fs::write(
1228+
repo_root.join(EXTERNAL_AGENT_DIR).join("settings.json"),
1229+
r#"{
1230+
"enabledMcpjsonServers": ["project-disabled", "local-disabled"],
1231+
"disabledMcpjsonServers": ["project-disabled"]
1232+
}"#,
1233+
)
1234+
.expect("write project settings");
1235+
fs::write(
1236+
repo_root
1237+
.join(EXTERNAL_AGENT_DIR)
1238+
.join("settings.local.json"),
1239+
r#"{
1240+
"enabledMcpjsonServers": ["local-enabled", "local-disabled"],
1241+
"disabledMcpjsonServers": ["local-disabled"]
1242+
}"#,
1243+
)
1244+
.expect("write local settings");
1245+
1246+
service_for_paths(external_agent_home, root.path().join(".codex"))
1247+
.import(vec![ExternalAgentConfigMigrationItem {
1248+
item_type: ExternalAgentConfigMigrationItemType::McpServerConfig,
1249+
description: String::new(),
1250+
cwd: Some(repo_root.clone()),
1251+
details: None,
1252+
}])
1253+
.await
1254+
.expect("import");
1255+
1256+
let config: TomlValue = toml::from_str(
1257+
&fs::read_to_string(repo_root.join(".codex").join("config.toml")).expect("read config"),
1258+
)
1259+
.expect("parse config");
1260+
let expected: TomlValue = toml::from_str(
1261+
r#"
1262+
[mcp_servers.local-enabled]
1263+
command = "local-enabled-server"
1264+
"#,
1265+
)
1266+
.expect("parse expected config");
1267+
assert_eq!(config, expected);
1268+
}
1269+
11471270
#[tokio::test]
11481271
async fn import_repo_mcp_ignores_invalid_home_settings_when_repo_settings_missing() {
11491272
let root = TempDir::new().expect("create tempdir");
@@ -1286,6 +1409,64 @@ async fn detect_home_lists_enabled_plugins_from_settings() {
12861409
);
12871410
}
12881411

1412+
#[tokio::test]
1413+
async fn detect_home_plugins_uses_local_settings_over_project_settings() {
1414+
let (_root, external_agent_home, codex_home) = fixture_paths();
1415+
fs::create_dir_all(&external_agent_home).expect("create external agent home");
1416+
fs::write(
1417+
external_agent_home.join("settings.json"),
1418+
r#"{
1419+
"enabledPlugins": {
1420+
"formatter@acme-tools": true,
1421+
"legacy@acme-tools": true
1422+
},
1423+
"extraKnownMarketplaces": {
1424+
"acme-tools": {
1425+
"source": "acme-corp/external-agent-plugins"
1426+
}
1427+
}
1428+
}"#,
1429+
)
1430+
.expect("write project settings");
1431+
fs::write(
1432+
external_agent_home.join("settings.local.json"),
1433+
r#"{
1434+
"enabledPlugins": {
1435+
"formatter@acme-tools": false,
1436+
"deployer@acme-tools": true
1437+
}
1438+
}"#,
1439+
)
1440+
.expect("write local settings");
1441+
1442+
let items = service_for_paths(external_agent_home.clone(), codex_home)
1443+
.detect(ExternalAgentConfigDetectOptions {
1444+
include_home: true,
1445+
cwds: None,
1446+
})
1447+
.await
1448+
.expect("detect");
1449+
1450+
assert_eq!(
1451+
items,
1452+
vec![ExternalAgentConfigMigrationItem {
1453+
item_type: ExternalAgentConfigMigrationItemType::Plugins,
1454+
description: format!(
1455+
"Migrate enabled plugins from {}",
1456+
external_agent_home.join("settings.json").display()
1457+
),
1458+
cwd: None,
1459+
details: Some(MigrationDetails {
1460+
plugins: vec![PluginsMigration {
1461+
marketplace_name: "acme-tools".to_string(),
1462+
plugin_names: vec!["deployer".to_string(), "legacy".to_string()],
1463+
}],
1464+
..Default::default()
1465+
}),
1466+
}]
1467+
);
1468+
}
1469+
12891470
#[tokio::test]
12901471
async fn detect_repo_skips_plugins_that_are_already_configured_in_codex() {
12911472
let root = TempDir::new().expect("create tempdir");

0 commit comments

Comments
 (0)