From 569ff6a1c400bd514ff79f5f1050a684dc3afde3 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 11 May 2026 12:53:15 +0200 Subject: [PATCH] extension: move git attribution into an extension (#21738) ## Why Git commit attribution is prompt policy, not session orchestration. After #21737 adds the extension-registry seam, this moves that prompt-only behavior out of `codex-core` so `Session` can consume extension-contributed prompt fragments instead of owning a one-off policy path itself. Before this PR, `Session` injected the trailer instruction directly from `codex-core` ([session assembly](https://github.com/openai/codex/blob/a57a747eb667753118217b8bb47dfd1fff88cbde/codex-rs/core/src/session/mod.rs#L2733-L2739), [helper module](https://github.com/openai/codex/blob/a57a747eb667753118217b8bb47dfd1fff88cbde/codex-rs/core/src/commit_attribution.rs#L1-L33)). This branch moves that same responsibility into [`codex-git-attribution`](https://github.com/openai/codex/blob/b5029a67360fe5c948aa849d4cf65fd2597ebaae/codex-rs/ext/git-attribution/src/lib.rs#L14-L100). ## What changed - Added the `codex-git-attribution` extension crate. - Snapshot `CodexGitCommit` plus `commit_attribution` at thread start, then contribute the developer-policy fragment through the extension registry. - Register the extension in app-server thread extensions. - Remove the old `codex-core` helper module and direct `Session` injection path. This keeps the existing behavior intact: the prompt is only contributed when `CodexGitCommit` is enabled, blank attribution still disables the trailer, and the default remains `Codex `. ## Stack - Stacked on #21737. --- codex-rs/Cargo.lock | 11 ++ codex-rs/Cargo.toml | 2 + codex-rs/app-server/Cargo.toml | 1 + codex-rs/app-server/src/extensions.rs | 6 +- codex-rs/core/src/commit_attribution.rs | 33 ---- codex-rs/core/src/commit_attribution_tests.rs | 43 ----- codex-rs/core/src/lib.rs | 1 - codex-rs/core/src/session/mod.rs | 8 - codex-rs/ext/git-attribution/BUILD.bazel | 6 + codex-rs/ext/git-attribution/Cargo.toml | 21 +++ codex-rs/ext/git-attribution/src/lib.rs | 149 ++++++++++++++++++ 11 files changed, 195 insertions(+), 86 deletions(-) delete mode 100644 codex-rs/core/src/commit_attribution.rs delete mode 100644 codex-rs/core/src/commit_attribution_tests.rs create mode 100644 codex-rs/ext/git-attribution/BUILD.bazel create mode 100644 codex-rs/ext/git-attribution/Cargo.toml create mode 100644 codex-rs/ext/git-attribution/src/lib.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index bf1d7be1c8..f600b38d6d 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1902,6 +1902,7 @@ dependencies = [ "codex-feedback", "codex-file-search", "codex-file-watcher", + "codex-git-attribution", "codex-git-utils", "codex-hooks", "codex-login", @@ -2918,6 +2919,16 @@ dependencies = [ "tracing", ] +[[package]] +name = "codex-git-attribution" +version = "0.0.0" +dependencies = [ + "codex-core", + "codex-extension-api", + "codex-features", + "pretty_assertions", +] + [[package]] name = "codex-git-utils" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 41ba8c69ba..ae7ef5dc3f 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -46,6 +46,7 @@ members = [ "execpolicy", "execpolicy-legacy", "ext/extension-api", + "ext/git-attribution", "external-agent-migration", "external-agent-sessions", "keyring-store", @@ -162,6 +163,7 @@ codex-file-system = { path = "file-system" } codex-exec-server = { path = "exec-server" } codex-execpolicy = { path = "execpolicy" } codex-extension-api = { path = "ext/extension-api" } +codex-git-attribution = { path = "ext/git-attribution" } codex-external-agent-migration = { path = "external-agent-migration" } codex-external-agent-sessions = { path = "external-agent-sessions" } codex-experimental-api-macros = { path = "codex-experimental-api-macros" } diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index bb26b5bd7a..7a38a75424 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -41,6 +41,7 @@ codex-extension-api = { workspace = true } codex-external-agent-migration = { workspace = true } codex-external-agent-sessions = { workspace = true } codex-features = { workspace = true } +codex-git-attribution = { workspace = true } codex-git-utils = { workspace = true } codex-file-watcher = { workspace = true } codex-hooks = { workspace = true } diff --git a/codex-rs/app-server/src/extensions.rs b/codex-rs/app-server/src/extensions.rs index 8fa5e3f693..9449f8dcad 100644 --- a/codex-rs/app-server/src/extensions.rs +++ b/codex-rs/app-server/src/extensions.rs @@ -5,5 +5,9 @@ use codex_extension_api::ExtensionRegistry; use codex_extension_api::ExtensionRegistryBuilder; pub(crate) fn thread_extensions() -> Arc> { - Arc::new(ExtensionRegistryBuilder::::new().build()) + Arc::new( + ExtensionRegistryBuilder::::new() + .with_extension(codex_git_attribution::extension()) + .build(), + ) } diff --git a/codex-rs/core/src/commit_attribution.rs b/codex-rs/core/src/commit_attribution.rs deleted file mode 100644 index 8df9661431..0000000000 --- a/codex-rs/core/src/commit_attribution.rs +++ /dev/null @@ -1,33 +0,0 @@ -const DEFAULT_ATTRIBUTION_VALUE: &str = "Codex "; - -fn build_commit_message_trailer(config_attribution: Option<&str>) -> Option { - let value = resolve_attribution_value(config_attribution)?; - Some(format!("Co-authored-by: {value}")) -} - -pub(crate) fn commit_message_trailer_instruction( - config_attribution: Option<&str>, -) -> Option { - let trailer = build_commit_message_trailer(config_attribution)?; - Some(format!( - "When you write or edit a git commit message, ensure the message ends with this trailer exactly once:\n{trailer}\n\nRules:\n- Keep existing trailers and append this trailer at the end if missing.\n- Do not duplicate this trailer if it already exists.\n- Keep one blank line between the commit body and trailer block." - )) -} - -fn resolve_attribution_value(config_attribution: Option<&str>) -> Option { - match config_attribution { - Some(value) => { - let trimmed = value.trim(); - if trimmed.is_empty() { - None - } else { - Some(trimmed.to_string()) - } - } - None => Some(DEFAULT_ATTRIBUTION_VALUE.to_string()), - } -} - -#[cfg(test)] -#[path = "commit_attribution_tests.rs"] -mod tests; diff --git a/codex-rs/core/src/commit_attribution_tests.rs b/codex-rs/core/src/commit_attribution_tests.rs deleted file mode 100644 index 8b2ce27e1f..0000000000 --- a/codex-rs/core/src/commit_attribution_tests.rs +++ /dev/null @@ -1,43 +0,0 @@ -use super::build_commit_message_trailer; -use super::commit_message_trailer_instruction; -use super::resolve_attribution_value; - -#[test] -fn blank_attribution_disables_trailer_prompt() { - assert_eq!(build_commit_message_trailer(Some("")), None); - assert_eq!(commit_message_trailer_instruction(Some(" ")), None); -} - -#[test] -fn default_attribution_uses_codex_trailer() { - assert_eq!( - build_commit_message_trailer(/*config_attribution*/ None).as_deref(), - Some("Co-authored-by: Codex ") - ); -} - -#[test] -fn resolve_value_handles_default_custom_and_blank() { - assert_eq!( - resolve_attribution_value(/*config_attribution*/ None), - Some("Codex ".to_string()) - ); - assert_eq!( - resolve_attribution_value(Some("MyAgent ")), - Some("MyAgent ".to_string()) - ); - assert_eq!( - resolve_attribution_value(Some("MyAgent")), - Some("MyAgent".to_string()) - ); - assert_eq!(resolve_attribution_value(Some(" ")), None); -} - -#[test] -fn instruction_mentions_trailer_and_omits_generated_with() { - let instruction = commit_message_trailer_instruction(Some("AgentX ")) - .expect("instruction expected"); - assert!(instruction.contains("Co-authored-by: AgentX ")); - assert!(instruction.contains("exactly once")); - assert!(!instruction.contains("Generated-with")); -} diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 93acda7854..3ec34694cb 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -27,7 +27,6 @@ mod agent; mod attestation; mod codex_delegate; mod command_canonicalization; -mod commit_attribution; pub mod config; pub mod connectors; pub mod context; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 6d28b9959c..af36fda487 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -16,7 +16,6 @@ use crate::agent::agent_status_from_event; use crate::agent::status::is_final; use crate::attestation::AttestationProvider; use crate::build_available_skills; -use crate::commit_attribution::commit_message_trailer_instruction; use crate::compact; use crate::config::ManagedFeatures; use crate::config::resolve_tool_suggest_config_from_layer_stack; @@ -2712,13 +2711,6 @@ impl Session { { developer_sections.push(plugin_instructions.render()); } - if turn_context.features.enabled(Feature::CodexGitCommit) - && let Some(commit_message_instruction) = commit_message_trailer_instruction( - turn_context.config.commit_attribution.as_deref(), - ) - { - developer_sections.push(commit_message_instruction); - } for contributor in self.services.extensions.context_contributors() { for fragment in contributor.contribute( &self.services.session_extension_data, diff --git a/codex-rs/ext/git-attribution/BUILD.bazel b/codex-rs/ext/git-attribution/BUILD.bazel new file mode 100644 index 0000000000..0cb1ab5764 --- /dev/null +++ b/codex-rs/ext/git-attribution/BUILD.bazel @@ -0,0 +1,6 @@ +load("//:defs.bzl", "codex_rust_crate") + +codex_rust_crate( + name = "git-attribution", + crate_name = "codex_git_attribution", +) diff --git a/codex-rs/ext/git-attribution/Cargo.toml b/codex-rs/ext/git-attribution/Cargo.toml new file mode 100644 index 0000000000..edf8c07196 --- /dev/null +++ b/codex-rs/ext/git-attribution/Cargo.toml @@ -0,0 +1,21 @@ +[package] +edition.workspace = true +license.workspace = true +name = "codex-git-attribution" +version.workspace = true + +[lib] +name = "codex_git_attribution" +path = "src/lib.rs" +doctest = false + +[lints] +workspace = true + +[dependencies] +codex-core = { workspace = true } +codex-extension-api = { workspace = true } +codex-features = { workspace = true } + +[dev-dependencies] +pretty_assertions = { workspace = true } diff --git a/codex-rs/ext/git-attribution/src/lib.rs b/codex-rs/ext/git-attribution/src/lib.rs new file mode 100644 index 0000000000..d4b711d555 --- /dev/null +++ b/codex-rs/ext/git-attribution/src/lib.rs @@ -0,0 +1,149 @@ +use std::sync::Arc; + +use codex_core::config::Config; +use codex_extension_api::CodexExtension; +use codex_extension_api::ContextContributor; +use codex_extension_api::ExtensionData; +use codex_extension_api::ExtensionRegistryBuilder; +use codex_extension_api::PromptFragment; +use codex_extension_api::ThreadStartContributor; +use codex_features::Feature; + +const DEFAULT_ATTRIBUTION_VALUE: &str = "Codex "; + +/// Prompt-only extension that contributes the configured git-attribution instruction. +#[derive(Clone, Copy, Debug, Default)] +pub struct GitAttributionExtension; + +impl GitAttributionExtension { + /// Creates an extension instance. + pub fn new() -> Self { + Self + } +} + +impl ContextContributor for GitAttributionExtension { + fn contribute( + &self, + _session_store: &ExtensionData, + thread_store: &ExtensionData, + ) -> Vec { + let Some(config_store) = thread_store.get::() else { + return Vec::new(); + }; + if !config_store.enabled { + return Vec::new(); + } + build_instruction(config_store.prompt.as_deref()) + .map(PromptFragment::developer_policy) + .into_iter() + .collect() + } +} + +#[derive(Clone, Debug, Default)] +struct GitAttributionConfig { + enabled: bool, + prompt: Option, +} + +impl ThreadStartContributor for GitAttributionExtension { + fn contribute( + &self, + config: &Config, + _session_store: &ExtensionData, + thread_store: &ExtensionData, + ) { + thread_store.insert(GitAttributionConfig { + enabled: config.features.enabled(Feature::CodexGitCommit), + prompt: config.commit_attribution.clone(), + }); + } +} + +impl CodexExtension for GitAttributionExtension { + fn install(self: Arc, registry: &mut ExtensionRegistryBuilder) { + registry.thread_start_contributor(self.clone()); + registry.prompt_contributor(self); + } +} + +/// Creates a shared git-attribution extension instance. +pub fn extension() -> Arc { + Arc::new(GitAttributionExtension::new()) +} + +fn build_commit_message_trailer(config_attribution: Option<&str>) -> Option { + let value = resolve_attribution_value(config_attribution)?; + Some(format!("Co-authored-by: {value}")) +} + +fn build_instruction(config_attribution: Option<&str>) -> Option { + let trailer = build_commit_message_trailer(config_attribution)?; + Some(format!( + "When you write or edit a git commit message, ensure the message ends with this trailer exactly once:\n{trailer}\n\nRules:\n- Keep existing trailers and append this trailer at the end if missing.\n- Do not duplicate this trailer if it already exists.\n- Keep one blank line between the commit body and trailer block." + )) +} + +fn resolve_attribution_value(config_attribution: Option<&str>) -> Option { + match config_attribution { + Some(value) => { + let trimmed = value.trim(); + if trimmed.is_empty() { + None + } else { + Some(trimmed.to_string()) + } + } + None => Some(DEFAULT_ATTRIBUTION_VALUE.to_string()), + } +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + + use super::build_commit_message_trailer; + use super::build_instruction; + use super::resolve_attribution_value; + + #[test] + fn blank_attribution_disables_trailer_prompt() { + assert_eq!(build_commit_message_trailer(Some("")), None); + assert_eq!(build_instruction(Some(" ")), None); + } + + #[test] + fn default_attribution_uses_codex_trailer() { + assert_eq!( + build_commit_message_trailer(/*config_attribution*/ None).as_deref(), + Some("Co-authored-by: Codex ") + ); + } + + #[test] + fn resolve_value_handles_default_custom_and_blank() { + assert_eq!( + resolve_attribution_value(/*config_attribution*/ None), + Some("Codex ".to_string()) + ); + assert_eq!( + resolve_attribution_value(Some("MyAgent ")), + Some("MyAgent ".to_string()) + ); + assert_eq!( + resolve_attribution_value(Some("MyAgent")), + Some("MyAgent".to_string()) + ); + assert_eq!(resolve_attribution_value(Some(" ")), None); + } + + #[test] + fn instruction_mentions_trailer_and_omits_generated_with() { + let instruction = + build_instruction(Some("AgentX ")).expect("instruction expected"); + assert!(instruction.contains("Co-authored-by: AgentX ")); + assert!(instruction.contains("exactly once")); + assert!(!instruction.contains("Generated-with")); + } +}