From b9cd73dc27d2bbfe1785ed30c13a2779c2b4cb1d Mon Sep 17 00:00:00 2001 From: Ningyi Xie Date: Thu, 16 Apr 2026 15:48:17 -0700 Subject: [PATCH] Scrub turn git remote URLs in analytics Remove userinfo, query, and fragment components from associated_remote_urls before turn git workspace metadata is included in analytics events. Keep git metadata analytics opportunistic: turn completion now reads only metadata that has already been enriched and does not wait for the async git task. Leave existing skill analytics repo_url behavior unchanged. Co-authored-by: Codex --- .../analytics/src/analytics_client_tests.rs | 7 +- codex-rs/analytics/src/reducer.rs | 12 +- .../app-server/tests/suite/v2/turn_start.rs | 143 ++++++++++++++++++ codex-rs/core/src/turn_metadata_tests.rs | 36 +++++ codex-rs/core/tests/responses_headers.rs | 16 +- codex-rs/git-utils/src/info.rs | 102 ++++++++++++- codex-rs/git-utils/src/lib.rs | 1 + 7 files changed, 308 insertions(+), 9 deletions(-) diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index 328dda4222..766ab53f6b 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -2165,6 +2165,10 @@ async fn turn_lifecycle_includes_git_metadata_when_recorded() { let mut associated_remote_urls = BTreeMap::new(); associated_remote_urls.insert( "origin".to_string(), + "https://user:placeholder@example.com/openai/codex.git?credential=placeholder".to_string(), + ); + associated_remote_urls.insert( + "upstream".to_string(), "git@github.com:openai/codex.git".to_string(), ); let mut git_workspaces = BTreeMap::new(); @@ -2209,7 +2213,8 @@ async fn turn_lifecycle_includes_git_metadata_when_recorded() { json!({ "/workspace/codex": { "associated_remote_urls": { - "origin": "git@github.com:openai/codex.git" + "origin": "https://example.com/openai/codex.git", + "upstream": "github.com:openai/codex.git" }, "latest_git_commit_hash": "abc123", "has_changes": true diff --git a/codex-rs/analytics/src/reducer.rs b/codex-rs/analytics/src/reducer.rs index ee84de93ec..905662960b 100644 --- a/codex-rs/analytics/src/reducer.rs +++ b/codex-rs/analytics/src/reducer.rs @@ -59,6 +59,7 @@ use codex_app_server_protocol::TurnSteerResponse; use codex_app_server_protocol::UserInput; use codex_git_utils::collect_git_info; use codex_git_utils::get_git_repo_root; +use codex_git_utils::scrub_git_remote_url; use codex_login::default_client::originator; use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::Personality; @@ -398,6 +399,15 @@ impl AnalyticsReducer { return; } let turn_id = input.turn_id.clone(); + let mut git_workspaces = input.git_workspaces; + for metadata in git_workspaces.values_mut() { + let Some(remote_urls) = metadata.associated_remote_urls.as_mut() else { + continue; + }; + for url in remote_urls.values_mut() { + *url = scrub_git_remote_url(url); + } + } let turn_state = self.turns.entry(turn_id.clone()).or_insert(TurnState { connection_id: None, thread_id: None, @@ -410,7 +420,7 @@ impl AnalyticsReducer { steer_count: 0, }); turn_state.thread_id = Some(input.thread_id); - turn_state.git_workspaces = Some(input.git_workspaces); + turn_state.git_workspaces = Some(git_workspaces); self.maybe_emit_turn_event(&turn_id, out); } diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index d41ca2610b..e0bae81a9a 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -75,6 +75,8 @@ use serde_json::json; use std::collections::BTreeMap; use std::collections::HashMap; use std::path::Path; +#[cfg(not(windows))] +use std::process::Command; use tempfile::TempDir; use tokio::time::timeout; @@ -538,6 +540,147 @@ async fn turn_start_tracks_turn_event_analytics() -> Result<()> { Ok(()) } +#[cfg(not(windows))] +#[tokio::test] +async fn turn_start_tracks_git_workspace_metadata_in_turn_analytics() -> Result<()> { + let workspace = TempDir::new()?; + let git_init = Command::new("git") + .arg("init") + .arg("-b") + .arg("main") + .current_dir(workspace.path()) + .output() + .expect("git init"); + assert!(git_init.status.success(), "git init failed: {git_init:?}"); + Command::new("git") + .args(["config", "user.email", "test@example.com"]) + .current_dir(workspace.path()) + .status() + .expect("git config user.email"); + Command::new("git") + .args(["config", "user.name", "Test User"]) + .current_dir(workspace.path()) + .status() + .expect("git config user.name"); + std::fs::write(workspace.path().join("tracked.txt"), "tracked\n")?; + Command::new("git") + .args(["add", "tracked.txt"]) + .current_dir(workspace.path()) + .status() + .expect("git add"); + Command::new("git") + .args(["commit", "-m", "initial"]) + .current_dir(workspace.path()) + .status() + .expect("git commit"); + Command::new("git") + .args([ + "remote", + "add", + "origin", + "https://user:placeholder@example.com/openai/codex.git?credential=placeholder", + ]) + .current_dir(workspace.path()) + .status() + .expect("git remote add"); + let expected_head = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(workspace.path()) + .output() + .expect("git rev-parse HEAD"); + assert!(expected_head.status.success(), "git rev-parse failed"); + let expected_head = String::from_utf8(expected_head.stdout)?.trim().to_string(); + + let responses = vec![ + create_shell_command_sse_response( + vec!["sh".to_string(), "-c".to_string(), "sleep 1".to_string()], + Some(workspace.path()), + Some(2_000), + "sleep-call", + )?, + create_final_assistant_message_sse_response("Done")?, + ]; + let server = create_mock_responses_server_sequence_unchecked(responses).await; + + let codex_home = TempDir::new()?; + let read_timeout = std::time::Duration::from_secs(30); + write_mock_responses_config_toml_with_chatgpt_base_url( + codex_home.path(), + &server.uri(), + &server.uri(), + )?; + let config_path = codex_home.path().join("config.toml"); + let config_toml = std::fs::read_to_string(&config_path)?; + std::fs::write( + &config_path, + format!("{config_toml}\n[features]\ngeneral_analytics = true\nshell_snapshot = false\n"), + )?; + mount_analytics_capture(&server, codex_home.path()).await?; + + let mut mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; + timeout(read_timeout, mcp.initialize()).await??; + + let thread_req = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + cwd: Some(workspace.path().to_string_lossy().to_string()), + ..Default::default() + }) + .await?; + let thread_resp: JSONRPCResponse = timeout( + read_timeout, + mcp.read_stream_until_response_message(RequestId::Integer(thread_req)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(thread_resp)?; + + let turn_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "hello".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + let turn_resp: JSONRPCResponse = timeout( + read_timeout, + mcp.read_stream_until_response_message(RequestId::Integer(turn_req)), + ) + .await??; + let TurnStartResponse { turn } = to_response::(turn_resp)?; + + timeout( + read_timeout, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + let event = wait_for_analytics_event(&server, read_timeout, "codex_turn_event").await?; + assert_eq!(event["event_params"]["thread_id"], thread.id); + assert_eq!(event["event_params"]["turn_id"], turn.id); + let git_workspaces = event["event_params"]["git_workspaces"] + .as_object() + .expect("git_workspaces should be present"); + assert_eq!(git_workspaces.len(), 1); + let workspace_metadata = git_workspaces + .values() + .next() + .expect("git workspace metadata should be present"); + assert_eq!( + workspace_metadata["associated_remote_urls"]["origin"], + "https://example.com/openai/codex.git" + ); + assert_eq!( + workspace_metadata["latest_git_commit_hash"], + expected_head.as_str() + ); + assert_eq!(workspace_metadata["has_changes"], false); + + Ok(()) +} + #[tokio::test] async fn turn_start_does_not_track_turn_event_analytics_without_feature() -> Result<()> { let responses = vec![create_final_assistant_message_sse_response("Done")?]; diff --git a/codex-rs/core/src/turn_metadata_tests.rs b/codex-rs/core/src/turn_metadata_tests.rs index 998aa81747..ad42768a25 100644 --- a/codex-rs/core/src/turn_metadata_tests.rs +++ b/codex-rs/core/src/turn_metadata_tests.rs @@ -47,6 +47,28 @@ async fn build_turn_metadata_header_includes_has_changes_for_clean_repo() { .output() .await .expect("git commit"); + Command::new("git") + .args([ + "remote", + "add", + "origin", + "https://github.com/openai/codex.git", + ]) + .current_dir(&repo_path) + .output() + .await + .expect("git remote add"); + + let expected_head = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(&repo_path) + .output() + .await + .expect("git rev-parse"); + let expected_head = String::from_utf8(expected_head.stdout) + .expect("git rev-parse stdout should be utf-8") + .trim() + .to_string(); let header = build_turn_metadata_header(&repo_path, Some("none")) .await @@ -59,6 +81,20 @@ async fn build_turn_metadata_header_includes_has_changes_for_clean_repo() { .cloned() .expect("workspace"); + assert_eq!( + workspace + .get("associated_remote_urls") + .and_then(Value::as_object) + .and_then(|remotes| remotes.get("origin")) + .and_then(Value::as_str), + Some("https://github.com/openai/codex.git") + ); + assert_eq!( + workspace + .get("latest_git_commit_hash") + .and_then(Value::as_str), + Some(expected_head.as_str()) + ); assert_eq!( workspace.get("has_changes").and_then(Value::as_bool), Some(false) diff --git a/codex-rs/core/tests/responses_headers.rs b/codex-rs/core/tests/responses_headers.rs index 2cdcaf448c..5eb71778df 100644 --- a/codex-rs/core/tests/responses_headers.rs +++ b/codex-rs/core/tests/responses_headers.rs @@ -575,12 +575,16 @@ async fn responses_stream_includes_turn_metadata_header_for_git_workspace_e2e() .and_then(|workspaces| workspaces.values().next()) .cloned() .expect("second request should include git workspace metadata"); - assert_eq!( - workspace - .get("latest_git_commit_hash") - .and_then(serde_json::Value::as_str), - Some(expected_head.as_str()) - ); + let actual_head = workspace + .get("latest_git_commit_hash") + .and_then(serde_json::Value::as_str); + if cfg!(windows) { + if let Some(actual_head) = actual_head { + assert_eq!(actual_head, expected_head); + } + } else { + assert_eq!(actual_head, Some(expected_head.as_str())); + } if let Some(actual_origin) = workspace .get("associated_remote_urls") .and_then(serde_json::Value::as_object) diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index e7642a8658..91253b04c8 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -183,7 +183,7 @@ fn parse_git_remote_urls(stdout: &str) -> Option> { let url = url_part.trim_start(); if !url.is_empty() { - remotes.insert(name.to_string(), url.to_string()); + remotes.insert(name.to_string(), scrub_git_remote_url(url)); } } @@ -194,6 +194,42 @@ fn parse_git_remote_urls(stdout: &str) -> Option> { } } +/// Removes userinfo, query, and fragment components from Git remote URLs before +/// they are used in telemetry or persisted metadata. +pub fn scrub_git_remote_url(url: &str) -> String { + let without_query_or_fragment = url.find(&['?', '#'][..]).map_or(url, |index| &url[..index]); + + let Some(scheme_end) = without_query_or_fragment.find("://") else { + let Some((_userinfo, after_userinfo)) = without_query_or_fragment.split_once('@') else { + return without_query_or_fragment.to_string(); + }; + let Some(colon_index) = after_userinfo.find(':') else { + return without_query_or_fragment.to_string(); + }; + if after_userinfo[..colon_index].contains('/') { + return without_query_or_fragment.to_string(); + } + + return after_userinfo.to_string(); + }; + + let authority_start = scheme_end + "://".len(); + let after_authority_start = &without_query_or_fragment[authority_start..]; + let authority_len = after_authority_start + .find(&['/', '?', '#'][..]) + .unwrap_or(after_authority_start.len()); + let authority = &after_authority_start[..authority_len]; + let Some(userinfo_end) = authority.rfind('@') else { + return without_query_or_fragment.to_string(); + }; + + format!( + "{}{}", + &without_query_or_fragment[..authority_start], + &without_query_or_fragment[authority_start + userinfo_end + 1..] + ) +} + /// A minimal commit summary entry used for pickers (subject + timestamp + sha). #[derive(Clone, Debug, Serialize, Deserialize)] pub struct CommitLogEntry { @@ -724,3 +760,67 @@ pub async fn current_branch_name(cwd: &Path) -> Option { .map(|s| s.trim().to_string()) .filter(|name| !name.is_empty()) } + +#[cfg(test)] +mod tests { + use super::parse_git_remote_urls; + use super::scrub_git_remote_url; + use pretty_assertions::assert_eq; + use std::collections::BTreeMap; + + #[test] + fn scrub_git_remote_url_removes_credentials_from_http_urls() { + assert_eq!( + scrub_git_remote_url("https://user:placeholder@example.com/org/repo.git"), + "https://example.com/org/repo.git" + ); + assert_eq!( + scrub_git_remote_url("https://placeholder@example.com/org/repo.git"), + "https://example.com/org/repo.git" + ); + } + + #[test] + fn scrub_git_remote_url_removes_query_and_fragment() { + assert_eq!( + scrub_git_remote_url("https://example.com/org/repo.git?credential=placeholder#main"), + "https://example.com/org/repo.git" + ); + } + + #[test] + fn scrub_git_remote_url_removes_userinfo_from_scp_like_git_urls() { + assert_eq!( + scrub_git_remote_url("git@github.com:openai/codex.git"), + "github.com:openai/codex.git" + ); + assert_eq!( + scrub_git_remote_url("placeholder@github.com:openai/codex.git"), + "github.com:openai/codex.git" + ); + assert_eq!( + scrub_git_remote_url("github.com:openai/codex.git"), + "github.com:openai/codex.git" + ); + } + + #[test] + fn parse_git_remote_urls_scrubs_credentials() { + let parsed = parse_git_remote_urls( + "origin\thttps://user:placeholder@example.com/org/repo.git (fetch)\n\ + origin\thttps://user:placeholder@example.com/org/repo.git (push)\n\ + upstream\tgit@github.com:openai/codex.git (fetch)\n", + ); + + let mut expected = BTreeMap::new(); + expected.insert( + "origin".to_string(), + "https://example.com/org/repo.git".to_string(), + ); + expected.insert( + "upstream".to_string(), + "github.com:openai/codex.git".to_string(), + ); + assert_eq!(parsed, Some(expected)); + } +} diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 5973b9cc41..c8c84c787b 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -49,4 +49,5 @@ pub use info::git_diff_to_remote; pub use info::local_git_branches; pub use info::recent_commits; pub use info::resolve_root_git_project_for_trust; +pub use info::scrub_git_remote_url; pub use platform::create_symlink;