Skip to content

Commit 866cb61

Browse files
refactor(codex): centralize bundle scope policy
Make hook assertions scope-explicit and address review findings: assert_codex_bundle_contains_bin silently skipped hook validation when hooks/hooks.json was absent, letting global refresh tests pass even if hooks stopped rendering; and the global-vs-repo-local scope policy was scattered as ad-hoc conditionals across bundle writing, manifest/MCP mutation, and the doctor. - Add CodexBundlePolicy (include_hooks, mcp_args, mcp_env, hook_trust_config_path, include_memory_digest) consumed by the bundle writer, renderers, and doctor; collapse doctor_check_plugin's duplicated manifest/version/MCP checks into doctor_check_plugin_dir, which now warns if a repo-local bundle unexpectedly ships hooks. - Tests take an explicit CodexScope: global bundles must ship hooks/hooks.json, repo-local bundles must not; repo-local rendered bundles keep the placeholder and source-coverage checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 32b02ef commit 866cb61

2 files changed

Lines changed: 190 additions & 128 deletions

File tree

src/agents/codex.rs

Lines changed: 100 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,12 @@ impl AgentIntegration for CodexIntegration {
187187
eprintln!("\n\x1b[1mCodex CLI integration\x1b[0m");
188188
let local_plugin_dir = codex_repo_plugin_install_dir(&ctx.project_path);
189189
if local_plugin_dir.join(".codex-plugin/plugin.json").exists() {
190-
doctor_check_plugin_dir(dc, &local_plugin_dir, None);
190+
doctor_check_plugin_dir(
191+
dc,
192+
&local_plugin_dir,
193+
CodexBundlePolicy::for_scope(InstallScope::ProjectLocal),
194+
&ctx.home,
195+
);
191196
doctor_check_marketplace_entry(
192197
dc,
193198
&codex_repo_marketplace_path(&ctx.project_path),
@@ -406,15 +411,70 @@ fn uninstall_tracedecay_mcp_if_present(config_path: &Path) {
406411
}
407412
}
408413

414+
/// The scope contract for a rendered Codex plugin bundle, in one place.
415+
///
416+
/// A global bundle ships lifecycle hooks (declared in the manifest and
417+
/// recorded as trusted in the user-level `~/.codex/config.toml`), serves with
418+
/// the global DB enabled, and carries the memory digest. A repo-local bundle
419+
/// ships no hooks, serves the project path with no env, and stays free of
420+
/// user-profile state. The bundle writer, manifest/MCP renderers, and doctor
421+
/// all consume this type instead of re-encoding the scope as ad-hoc
422+
/// conditionals.
423+
#[derive(Debug, Clone, Copy)]
424+
struct CodexBundlePolicy {
425+
scope: InstallScope,
426+
}
427+
428+
impl CodexBundlePolicy {
429+
fn for_scope(scope: InstallScope) -> Self {
430+
Self { scope }
431+
}
432+
433+
/// Whether the bundle ships `hooks/hooks.json` and declares it in the
434+
/// plugin manifest.
435+
fn include_hooks(self) -> bool {
436+
self.scope == InstallScope::Global
437+
}
438+
439+
/// The `serve` args baked into the bundle's `.mcp.json`.
440+
fn mcp_args(self) -> serde_json::Value {
441+
match self.scope {
442+
InstallScope::Global => json!(["serve"]),
443+
InstallScope::ProjectLocal => json!(["serve", "--path", "."]),
444+
}
445+
}
446+
447+
/// The `env` baked into the bundle's `.mcp.json`; `None` strips the key.
448+
fn mcp_env(self) -> Option<serde_json::Value> {
449+
match self.scope {
450+
InstallScope::Global => Some(json!({ "TRACEDECAY_ENABLE_GLOBAL_DB": "1" })),
451+
InstallScope::ProjectLocal => None,
452+
}
453+
}
454+
455+
/// Where Codex records trust for this bundle's hooks — `None` for scopes
456+
/// that ship no hooks and therefore have no trust surface.
457+
fn hook_trust_config_path(self, home: &Path) -> Option<PathBuf> {
458+
self.include_hooks()
459+
.then(|| home.join(".codex/config.toml"))
460+
}
461+
462+
/// The memory digest rides only the global bundle.
463+
fn include_memory_digest(self) -> bool {
464+
self.scope == InstallScope::Global
465+
}
466+
}
467+
409468
fn install_codex_plugin_bundle(
410469
install_dir: &Path,
411470
tracedecay_bin: &str,
412471
scope: InstallScope,
413472
profile_home: &Path,
414473
) -> Result<()> {
415-
write_codex_plugin_bundle_base(install_dir, tracedecay_bin, scope)?;
474+
let policy = CodexBundlePolicy::for_scope(scope);
475+
write_codex_plugin_bundle_base(install_dir, tracedecay_bin, policy)?;
416476
install_codex_managed_skill_overlay(profile_home, install_dir)?;
417-
if scope != InstallScope::ProjectLocal {
477+
if policy.include_memory_digest() {
418478
let profile_root =
419479
crate::automation::skill_targets::profile_root_for_agent_home(profile_home);
420480
crate::automation::memory_digest::sync_memory_digest_export(
@@ -432,7 +492,11 @@ pub fn export_codex_plugin_artifact(
432492
output: &Path,
433493
tracedecay_bin: &str,
434494
) -> Result<crate::automation::skill_targets::SkillInstallSummary> {
435-
write_codex_plugin_bundle_base(output, tracedecay_bin, InstallScope::Global)?;
495+
write_codex_plugin_bundle_base(
496+
output,
497+
tracedecay_bin,
498+
CodexBundlePolicy::for_scope(InstallScope::Global),
499+
)?;
436500
crate::automation::skill_targets::export_native_skill_overlay(
437501
profile_root,
438502
crate::automation::skill_targets::SkillInstallTarget::Codex,
@@ -443,15 +507,15 @@ pub fn export_codex_plugin_artifact(
443507
fn write_codex_plugin_bundle_base(
444508
install_dir: &Path,
445509
tracedecay_bin: &str,
446-
scope: InstallScope,
510+
policy: CodexBundlePolicy,
447511
) -> Result<()> {
448512
if let Some(parent) = install_dir.parent() {
449513
std::fs::create_dir_all(parent).map_err(|e| TraceDecayError::Config {
450514
message: format!("failed to create {}: {e}", parent.display()),
451515
})?;
452516
}
453517
remove_codex_plugin_install(install_dir)?;
454-
write_codex_plugin_files(install_dir, tracedecay_bin, scope)
518+
write_codex_plugin_files(install_dir, tracedecay_bin, policy)
455519
}
456520

457521
fn install_codex_managed_skill_overlay(
@@ -469,13 +533,13 @@ fn install_codex_managed_skill_overlay(
469533
fn write_codex_plugin_files(
470534
install_dir: &Path,
471535
tracedecay_bin: &str,
472-
scope: InstallScope,
536+
policy: CodexBundlePolicy,
473537
) -> Result<()> {
474538
for (relative, contents) in codex_embedded_plugin_files() {
475539
let rendered = match relative {
476-
".codex-plugin/plugin.json" => codex_plugin_manifest(contents, scope)?,
477-
".mcp.json" => codex_plugin_mcp(contents, tracedecay_bin, scope)?,
478-
"hooks/hooks.json" if scope == InstallScope::ProjectLocal => continue,
540+
".codex-plugin/plugin.json" => codex_plugin_manifest(contents, policy)?,
541+
".mcp.json" => codex_plugin_mcp(contents, tracedecay_bin, policy)?,
542+
"hooks/hooks.json" if !policy.include_hooks() => continue,
479543
"hooks/hooks.json" => codex_plugin_hooks(contents, tracedecay_bin)?,
480544
_ => contents.to_string(),
481545
};
@@ -484,9 +548,9 @@ fn write_codex_plugin_files(
484548
Ok(())
485549
}
486550

487-
fn codex_plugin_manifest(raw: &str, scope: InstallScope) -> Result<String> {
551+
fn codex_plugin_manifest(raw: &str, policy: CodexBundlePolicy) -> Result<String> {
488552
let stamped = super::plugin_bundle::stamp_manifest_version(raw)?;
489-
if scope != InstallScope::ProjectLocal {
553+
if policy.include_hooks() {
490554
return Ok(stamped);
491555
}
492556

@@ -497,19 +561,16 @@ fn codex_plugin_manifest(raw: &str, scope: InstallScope) -> Result<String> {
497561
Ok(format!("{}\n", serde_json::to_string_pretty(&manifest)?))
498562
}
499563

500-
fn codex_plugin_mcp(raw: &str, tracedecay_bin: &str, scope: InstallScope) -> Result<String> {
501-
// Reuse the shared command rewrite, then layer Codex's scope-specific
502-
// args/env on top of the result.
564+
fn codex_plugin_mcp(raw: &str, tracedecay_bin: &str, policy: CodexBundlePolicy) -> Result<String> {
565+
// Reuse the shared command rewrite, then layer the policy's args/env on
566+
// top of the result.
503567
let stamped = super::plugin_bundle::set_mcp_command(raw, tracedecay_bin)?;
504568
let mut mcp: serde_json::Value = serde_json::from_str(&stamped)?;
505569
let server = &mut mcp["mcpServers"]["tracedecay"];
506-
match scope {
507-
InstallScope::Global => {
508-
server["args"] = json!(["serve"]);
509-
server["env"] = json!({ "TRACEDECAY_ENABLE_GLOBAL_DB": "1" });
510-
}
511-
InstallScope::ProjectLocal => {
512-
server["args"] = json!(["serve", "--path", "."]);
570+
server["args"] = policy.mcp_args();
571+
match policy.mcp_env() {
572+
Some(env) => server["env"] = env,
573+
None => {
513574
if let Some(object) = server.as_object_mut() {
514575
object.remove("env");
515576
}
@@ -1160,10 +1221,11 @@ fn uninstall_prompt_rules(agents_md: &Path) {
11601221
// ---------------------------------------------------------------------------
11611222

11621223
fn doctor_check_plugin(dc: &mut DoctorCounters, home: &Path) {
1224+
let global_policy = CodexBundlePolicy::for_scope(InstallScope::Global);
11631225
let cached_dirs = codex_plugin_cached_install_dirs(home);
11641226
if !cached_dirs.is_empty() {
11651227
for plugin_dir in cached_dirs {
1166-
doctor_check_plugin_dir(dc, &plugin_dir, Some(&home.join(".codex/config.toml")));
1228+
doctor_check_plugin_dir(dc, &plugin_dir, global_policy, home);
11671229
}
11681230
return;
11691231
}
@@ -1178,50 +1240,7 @@ fn doctor_check_plugin(dc: &mut DoctorCounters, home: &Path) {
11781240
return;
11791241
}
11801242

1181-
let manifest = load_json_file(&manifest_path);
1182-
if manifest.get("name").and_then(|value| value.as_str()) == Some("tracedecay") {
1183-
dc.pass(&format!(
1184-
"Codex plugin manifest present in {}",
1185-
manifest_path.display()
1186-
));
1187-
} else {
1188-
dc.fail(&format!(
1189-
"Codex plugin manifest at {} is not a tracedecay plugin",
1190-
manifest_path.display()
1191-
));
1192-
}
1193-
match manifest.get("version").and_then(|value| value.as_str()) {
1194-
Some(env!("CARGO_PKG_VERSION")) => dc.pass("Codex plugin version matches tracedecay"),
1195-
Some(version) => dc.warn(&format!(
1196-
"Codex plugin version {version} does not match tracedecay {} — run `tracedecay update-plugin`",
1197-
env!("CARGO_PKG_VERSION")
1198-
)),
1199-
None => dc.warn("Codex plugin manifest does not contain a version"),
1200-
}
1201-
1202-
let mcp_path = plugin_dir.join(".mcp.json");
1203-
let mcp = load_json_file(&mcp_path);
1204-
if mcp
1205-
.get("mcpServers")
1206-
.and_then(|servers| servers.get("tracedecay"))
1207-
.is_some()
1208-
{
1209-
dc.pass(&format!(
1210-
"Codex plugin MCP server registered in {}",
1211-
mcp_path.display()
1212-
));
1213-
} else {
1214-
dc.fail(&format!(
1215-
"Codex plugin MCP server missing in {} — run `tracedecay install --agent codex`",
1216-
mcp_path.display()
1217-
));
1218-
}
1219-
doctor_check_hooks(
1220-
dc,
1221-
&plugin_dir.join("hooks/hooks.json"),
1222-
&home.join(".codex/config.toml"),
1223-
);
1224-
1243+
doctor_check_plugin_dir(dc, &plugin_dir, global_policy, home);
12251244
doctor_check_marketplace_entry(
12261245
dc,
12271246
&codex_personal_marketplace_path(home),
@@ -1270,7 +1289,12 @@ fn doctor_check_marketplace_entry(
12701289
}
12711290
}
12721291

1273-
fn doctor_check_plugin_dir(dc: &mut DoctorCounters, plugin_dir: &Path, config_path: Option<&Path>) {
1292+
fn doctor_check_plugin_dir(
1293+
dc: &mut DoctorCounters,
1294+
plugin_dir: &Path,
1295+
policy: CodexBundlePolicy,
1296+
home: &Path,
1297+
) {
12741298
let manifest_path = plugin_dir.join(".codex-plugin/plugin.json");
12751299
let manifest = load_json_file(&manifest_path);
12761300
if manifest.get("name").and_then(|value| value.as_str()) == Some("tracedecay") {
@@ -1310,8 +1334,14 @@ fn doctor_check_plugin_dir(dc: &mut DoctorCounters, plugin_dir: &Path, config_pa
13101334
mcp_path.display()
13111335
));
13121336
}
1313-
if let Some(config_path) = config_path {
1314-
doctor_check_hooks(dc, &plugin_dir.join("hooks/hooks.json"), config_path);
1337+
let hooks_path = plugin_dir.join("hooks/hooks.json");
1338+
if let Some(config_path) = policy.hook_trust_config_path(home) {
1339+
doctor_check_hooks(dc, &hooks_path, &config_path);
1340+
} else if hooks_path.exists() {
1341+
dc.warn(&format!(
1342+
"repo-local Codex bundle unexpectedly ships lifecycle hooks in {} — run `tracedecay install --local --agent codex` to refresh it",
1343+
hooks_path.display()
1344+
));
13151345
}
13161346
}
13171347

0 commit comments

Comments
 (0)