mirror of
https://github.com/openai/codex.git
synced 2026-09-08 15:50:34 +00:00
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 <noreply@openai.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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::<ThreadStartResponse>(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::<TurnStartResponse>(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")?];
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -183,7 +183,7 @@ fn parse_git_remote_urls(stdout: &str) -> Option<BTreeMap<String, String>> {
|
||||
|
||||
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<BTreeMap<String, String>> {
|
||||
}
|
||||
}
|
||||
|
||||
/// 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<String> {
|
||||
.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));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user