mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat(doctor): report editor and pager environment (#27081)
## Background This was prompted by [#26858](https://github.com/openai/codex/issues/26858), where the attached doctor report did not include the editor selection and I had to [ask which editor was in use](https://github.com/openai/codex/issues/26858#issuecomment-4653829891) before investigating the external-editor newline issue. Capturing these variables in doctor makes that context available up front in future reports. `codex doctor` is intended to capture enough local context to diagnose startup and terminal behavior, but it did not report the environment variables that select an external editor or configure command pagers. The TUI [prefers `VISUAL` over `EDITOR`](https://github.com/openai/codex/blob/56554904babcaacf4444a2cc90716880837dff7c/codex-rs/tui/src/external_editor.rs#L31-L38), so missing or unexpected values can explain why the external-editor shortcut fails or launches the wrong command. Pager values are also useful inherited-shell context even though [unified exec normalizes its effective pager variables to `cat`](https://github.com/openai/codex/blob/56554904babcaacf4444a2cc90716880837dff7c/codex-rs/core/src/unified_exec/process_manager.rs#L60-L70). These variables can contain arbitrary command arguments or inline environment assignments. The human report is local, but `codex doctor --json` may be attached to feedback, so the machine-readable report should not include their raw contents. ## What Changed - Report `VISUAL` and `EDITOR` in the system environment details, using `not set` when either variable is absent. - Report inherited `PAGER`, `GIT_PAGER`, `GH_PAGER`, and `LESS` values when present. - Preserve full values in local human output while reducing these fields to `set` or `not set` in redacted JSON output. - Add structured check, JSON-redaction, rendered-output, and snapshot coverage. ## How to Test 1. From `codex-rs`, run Codex with explicit editor and pager variables: ```sh env VISUAL='code --wait' EDITOR=vim PAGER='less -R' GIT_PAGER=delta GH_PAGER=less LESS=-FRX \ cargo run -p codex-cli --bin codex -- doctor --no-color ``` 2. Confirm the `system` details show the full values for all six variables. 3. Unset the pager variables and rerun the command. Confirm pager rows are omitted while missing editor variables are shown as `not set`. 4. Run the same configured environment with `doctor --json`. Confirm each configured editor or pager field is reported as `set` and none of the raw commands or arguments appear in the JSON. Targeted tests: - `just test -p codex-cli` (279 tests passed)
This commit is contained in:
committed by
GitHub
Unverified
parent
feca160da4
commit
0473a5cc52
@@ -682,7 +682,7 @@ fn structured_json_details(details: &[String]) -> (BTreeMap<String, JsonDetailVa
|
||||
notes.push(redacted);
|
||||
continue;
|
||||
}
|
||||
let value = value.to_string();
|
||||
let value = json_detail_value(key, value);
|
||||
match structured.get_mut(key) {
|
||||
Some(existing) => existing.push(value),
|
||||
None => {
|
||||
@@ -693,6 +693,21 @@ fn structured_json_details(details: &[String]) -> (BTreeMap<String, JsonDetailVa
|
||||
(structured, notes)
|
||||
}
|
||||
|
||||
fn json_detail_value(key: &str, value: &str) -> String {
|
||||
if matches!(
|
||||
key,
|
||||
"VISUAL" | "EDITOR" | "PAGER" | "GIT_PAGER" | "GH_PAGER" | "LESS"
|
||||
) && !value.eq_ignore_ascii_case("not set")
|
||||
{
|
||||
// Editor and pager configuration can contain arbitrary arguments or
|
||||
// inline environment assignments. Keep full values local to human output
|
||||
// because the JSON report may be attached to feedback.
|
||||
"set".to_string()
|
||||
} else {
|
||||
value.to_string()
|
||||
}
|
||||
}
|
||||
|
||||
fn run_sync_check(
|
||||
label: &'static str,
|
||||
progress: Arc<dyn DoctorProgress>,
|
||||
@@ -3226,6 +3241,18 @@ mod tests {
|
||||
overall_status: CheckStatus::Warning,
|
||||
codex_version: "0.0.0".to_string(),
|
||||
checks: vec![
|
||||
DoctorCheck::new(
|
||||
"system.environment",
|
||||
"system",
|
||||
CheckStatus::Ok,
|
||||
"OS language en-US",
|
||||
)
|
||||
.detail("VISUAL: code --wait")
|
||||
.detail("EDITOR: env AWS_ACCESS_KEY_ID=AKIAEXAMPLE vim")
|
||||
.detail("PAGER: env PRIVATE_PAGER_VALUE=pager-secret less")
|
||||
.detail("GIT_PAGER: delta")
|
||||
.detail("GH_PAGER: less")
|
||||
.detail("LESS: -FRX"),
|
||||
DoctorCheck::new(
|
||||
"mcp.config",
|
||||
"mcp",
|
||||
@@ -3260,8 +3287,35 @@ mod tests {
|
||||
assert!(!redacted.contains("user:pass"));
|
||||
assert!(!redacted.contains("x=abc"));
|
||||
assert!(!redacted.contains("sk-live-secret"));
|
||||
assert!(!redacted.contains("AKIAEXAMPLE"));
|
||||
assert!(!redacted.contains("pager-secret"));
|
||||
assert!(!redacted.contains("code --wait"));
|
||||
assert!(redacted.contains("https://example.com/mcp"));
|
||||
assert_eq!(json["checks"].is_object(), true);
|
||||
assert_eq!(
|
||||
json["checks"]["system.environment"]["details"]["VISUAL"],
|
||||
"set"
|
||||
);
|
||||
assert_eq!(
|
||||
json["checks"]["system.environment"]["details"]["EDITOR"],
|
||||
"set"
|
||||
);
|
||||
assert_eq!(
|
||||
json["checks"]["system.environment"]["details"]["PAGER"],
|
||||
"set"
|
||||
);
|
||||
assert_eq!(
|
||||
json["checks"]["system.environment"]["details"]["GIT_PAGER"],
|
||||
"set"
|
||||
);
|
||||
assert_eq!(
|
||||
json["checks"]["system.environment"]["details"]["GH_PAGER"],
|
||||
"set"
|
||||
);
|
||||
assert_eq!(
|
||||
json["checks"]["system.environment"]["details"]["LESS"],
|
||||
"set"
|
||||
);
|
||||
assert_eq!(json["checks"]["mcp.config"]["id"], "mcp.config");
|
||||
assert_eq!(
|
||||
json["checks"]["mcp.config"]["details"]["OPENAI_API_KEY"],
|
||||
|
||||
@@ -1133,7 +1133,13 @@ mod tests {
|
||||
"OS language en-US",
|
||||
)
|
||||
.detail("os: macOS 15.0")
|
||||
.detail("os language: en-US"),
|
||||
.detail("os language: en-US")
|
||||
.detail("VISUAL: code --wait")
|
||||
.detail("EDITOR: vim")
|
||||
.detail("PAGER: less -R")
|
||||
.detail("GIT_PAGER: delta")
|
||||
.detail("GH_PAGER: less")
|
||||
.detail("LESS: -FRX"),
|
||||
DoctorCheck::new(
|
||||
"runtime.provenance",
|
||||
"runtime",
|
||||
@@ -1246,6 +1252,12 @@ Environment
|
||||
✓ system en-US
|
||||
os macOS 15.0
|
||||
OS language en-US
|
||||
VISUAL code --wait
|
||||
EDITOR vim
|
||||
PAGER less -R
|
||||
GIT_PAGER delta
|
||||
GH_PAGER less
|
||||
LESS -FRX
|
||||
✓ runtime running local build on darwin-arm64
|
||||
✓ install consistent
|
||||
managed by npm: no · bun: no · package root —
|
||||
|
||||
@@ -62,6 +62,12 @@ fn system_details(parsed: &[ParsedDetail]) -> Vec<HumanDetail> {
|
||||
push_row_if_present(&mut out, parsed, "LC_ALL", "LC_ALL");
|
||||
push_row_if_present(&mut out, parsed, "LC_CTYPE", "LC_CTYPE");
|
||||
push_row_if_present(&mut out, parsed, "LANG", "LANG");
|
||||
push_row_if_present(&mut out, parsed, "VISUAL", "VISUAL");
|
||||
push_row_if_present(&mut out, parsed, "EDITOR", "EDITOR");
|
||||
push_row_if_present(&mut out, parsed, "PAGER", "PAGER");
|
||||
push_row_if_present(&mut out, parsed, "GIT_PAGER", "GIT_PAGER");
|
||||
push_row_if_present(&mut out, parsed, "GH_PAGER", "GH_PAGER");
|
||||
push_row_if_present(&mut out, parsed, "LESS", "LESS");
|
||||
push_remaining(
|
||||
&mut out,
|
||||
parsed,
|
||||
@@ -73,6 +79,12 @@ fn system_details(parsed: &[ParsedDetail]) -> Vec<HumanDetail> {
|
||||
"LC_ALL",
|
||||
"LC_CTYPE",
|
||||
"LANG",
|
||||
"VISUAL",
|
||||
"EDITOR",
|
||||
"PAGER",
|
||||
"GIT_PAGER",
|
||||
"GH_PAGER",
|
||||
"LESS",
|
||||
],
|
||||
&[],
|
||||
);
|
||||
|
||||
+6
@@ -13,6 +13,12 @@ Environment
|
||||
✓ system en-US
|
||||
os macOS 15.0
|
||||
OS language en-US
|
||||
VISUAL code --wait
|
||||
EDITOR vim
|
||||
PAGER less -R
|
||||
GIT_PAGER delta
|
||||
GH_PAGER less
|
||||
LESS -FRX
|
||||
✓ runtime running local build on darwin-arm64
|
||||
✓ install consistent
|
||||
managed by npm: no · bun: no · package root —
|
||||
|
||||
@@ -4,6 +4,9 @@ use std::env;
|
||||
use super::DoctorCheck;
|
||||
use super::LOCALE_ENV_VARS;
|
||||
|
||||
const EDITOR_ENV_VARS: &[&str] = &["VISUAL", "EDITOR"];
|
||||
const PAGER_ENV_VARS: &[&str] = &["PAGER", "GIT_PAGER", "GH_PAGER", "LESS"];
|
||||
|
||||
#[derive(Clone, Debug, Default, Eq, PartialEq)]
|
||||
struct SystemCheckInputs {
|
||||
os: String,
|
||||
@@ -11,6 +14,8 @@ struct SystemCheckInputs {
|
||||
os_version: String,
|
||||
os_language: Option<String>,
|
||||
locale_env: BTreeMap<String, String>,
|
||||
editor_env: BTreeMap<String, String>,
|
||||
pager_env: BTreeMap<String, String>,
|
||||
}
|
||||
|
||||
impl SystemCheckInputs {
|
||||
@@ -24,12 +29,30 @@ impl SystemCheckInputs {
|
||||
.map(|value| ((*name).to_string(), value))
|
||||
})
|
||||
.collect();
|
||||
let editor_env = EDITOR_ENV_VARS
|
||||
.iter()
|
||||
.map(|name| {
|
||||
let value = env::var_os(name)
|
||||
.map(|value| value.to_string_lossy().into_owned())
|
||||
.unwrap_or_else(|| "not set".to_string());
|
||||
((*name).to_string(), value)
|
||||
})
|
||||
.collect();
|
||||
let pager_env = PAGER_ENV_VARS
|
||||
.iter()
|
||||
.filter_map(|name| {
|
||||
env::var_os(name)
|
||||
.map(|value| ((*name).to_string(), value.to_string_lossy().into_owned()))
|
||||
})
|
||||
.collect();
|
||||
Self {
|
||||
os: info.to_string(),
|
||||
os_type: info.os_type().to_string(),
|
||||
os_version: info.version().to_string(),
|
||||
os_language: sys_locale::get_locale(),
|
||||
locale_env,
|
||||
editor_env,
|
||||
pager_env,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -54,6 +77,16 @@ fn system_check_from_inputs(inputs: SystemCheckInputs) -> DoctorCheck {
|
||||
details.push(format!("{name}: {value}"));
|
||||
}
|
||||
}
|
||||
for name in EDITOR_ENV_VARS {
|
||||
if let Some(value) = inputs.editor_env.get(*name) {
|
||||
details.push(format!("{name}: {value}"));
|
||||
}
|
||||
}
|
||||
for name in PAGER_ENV_VARS {
|
||||
if let Some(value) = inputs.pager_env.get(*name) {
|
||||
details.push(format!("{name}: {value}"));
|
||||
}
|
||||
}
|
||||
|
||||
let summary = inputs
|
||||
.os_language
|
||||
@@ -76,20 +109,46 @@ mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn system_check_reports_os_language_and_locale_env() {
|
||||
fn system_check_reports_os_language_locale_editor_and_pager_env() {
|
||||
let mut locale_env = BTreeMap::new();
|
||||
locale_env.insert("LANG".to_string(), "en_US.UTF-8".to_string());
|
||||
let editor_env = BTreeMap::from([
|
||||
("EDITOR".to_string(), "vim".to_string()),
|
||||
("VISUAL".to_string(), "code --wait".to_string()),
|
||||
]);
|
||||
let pager_env = BTreeMap::from([
|
||||
("GH_PAGER".to_string(), "less".to_string()),
|
||||
("GIT_PAGER".to_string(), "delta".to_string()),
|
||||
("LESS".to_string(), "-FRX".to_string()),
|
||||
("PAGER".to_string(), "less -R".to_string()),
|
||||
]);
|
||||
let check = system_check_from_inputs(SystemCheckInputs {
|
||||
os: "macOS 15.0".to_string(),
|
||||
os_type: "macos".to_string(),
|
||||
os_version: "15.0".to_string(),
|
||||
os_language: Some("en-US".to_string()),
|
||||
locale_env,
|
||||
editor_env,
|
||||
pager_env,
|
||||
});
|
||||
|
||||
assert_eq!(check.summary, "OS language en-US");
|
||||
assert!(check.details.contains(&"os language: en-US".to_string()));
|
||||
assert!(check.details.contains(&"LANG: en_US.UTF-8".to_string()));
|
||||
assert_eq!(
|
||||
check.details,
|
||||
vec![
|
||||
"os: macOS 15.0",
|
||||
"os type: macos",
|
||||
"os version: 15.0",
|
||||
"os language: en-US",
|
||||
"LANG: en_US.UTF-8",
|
||||
"VISUAL: code --wait",
|
||||
"EDITOR: vim",
|
||||
"PAGER: less -R",
|
||||
"GIT_PAGER: delta",
|
||||
"GH_PAGER: less",
|
||||
"LESS: -FRX",
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -100,13 +159,24 @@ mod tests {
|
||||
os_version: "unknown".to_string(),
|
||||
os_language: None,
|
||||
locale_env: BTreeMap::new(),
|
||||
editor_env: BTreeMap::from([
|
||||
("EDITOR".to_string(), "not set".to_string()),
|
||||
("VISUAL".to_string(), "not set".to_string()),
|
||||
]),
|
||||
pager_env: BTreeMap::new(),
|
||||
});
|
||||
|
||||
assert_eq!(check.summary, "OS language unavailable");
|
||||
assert!(
|
||||
check
|
||||
.details
|
||||
.contains(&"os language: unavailable".to_string())
|
||||
assert_eq!(
|
||||
check.details,
|
||||
vec![
|
||||
"os: Linux",
|
||||
"os type: linux",
|
||||
"os version: unknown",
|
||||
"os language: unavailable",
|
||||
"VISUAL: not set",
|
||||
"EDITOR: not set",
|
||||
]
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user