Repository navigation
fix: preserve diagnostic text without secret redaction - #1179
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dd9b25f4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
soyeon-oai
left a comment
There was a problem hiding this comment.
Reviewed 2dd9b25 against 4096958. I am withholding approval for the existing terminal-control issue: #1179 (comment)
I reproduced the single-scan publication path through actual main() with an injected publishScanToCloud failure. For a synthetic error containing token=SYNTHETIC_TOKEN_123 followed by ESC[2J and a newline, both revisions return 2 with empty stdout; base replaces the message with [redacted], while this head writes the raw ESC and embedded newline to stderr. Important qualification: a token-free control emits raw controls on BOTH revisions, so this exposes an existing missing terminal-normalization boundary rather than introducing that weakness for every input.
Please preserve diagnostic text but apply diagnosticValue at the stderr display boundary (cli.ts:3128), as the batch path already does, and cover the equivalent direct error writes in other changed catch paths. Returned/persisted error data can remain unchanged. No need to restore credential redaction.
The focused original tests pass; the synthetic comparison demonstrates the missing control-character coverage. A real malformed-manifest attempt was blocked by the environment's /tmp ownership check before parsing, with no network calls; I did not bypass that check. No live service/model tests were run. I found no additional actionable issue in the full diff and relevant callers.
…-secret-redaction # Conflicts: # sdk/typescript/src/cli.ts
Summary
Preserve diagnostic text instead of replacing credential-shaped messages with
[redacted]. The wrapper's heuristic also hid ordinary failures such astoken budget exceededandbasic validation failed.Changes
CODEX_SECURITY_REDACT_LOGSand its documentation. Existing verbosity and log-level settings continue to control diagnostic detail.mainand retain its updated CLI help.Testing
Validation on
5b244b16after resolving the latestmainconflicts:12345and763770054: 3,383 passed, 51 skipped, zero failures in each run; 67,782 assertions across 158 files per run. Both ran after rebuilding the bundled plugin, using isolated private temporary directories.pnpm run types,pnpm run format,pnpm run build:ci, andpnpm run build:plugin: passed.git diff --check: passed.Risk and rollout
This intentionally changes the default behavior: credential-shaped diagnostic text is preserved regardless of verbosity.
CODEX_SECURITY_REDACT_LOGSis no longer supported and has no effect; no replacement setting or flag is introduced.Diagnostics and persisted or published summaries can contain sensitive information. Review output and artifacts before sharing or publishing them. This change does not add logging, session capture, or uploads, and cannot restore text already redacted upstream or in existing saved artifacts.
Public disclosure review