fix: preserve tty when forwarding codex

[ndycode]

Summary

• Preserve inherited stdout/stderr for terminal-attached forwarded Codex runs.
• Keep output capture for non-TTY forwarded runs so unsupported-model fallback can still inspect errors.
• Add wrapper regression coverage for the no-capture path.

Verification

• node --check scripts/codex.js
• npm install --ignore-scripts
• npx vitest run test/codex-bin-wrapper.test.ts (blocked: esbuild spawn EPERM in local Windows sandbox)

Notes

• npm ci was also blocked locally by Windows spawn EPERM during lifecycle/rebuild cleanup.

Fixes #436

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr fixes tty passthrough for forwarded codex runs by introducing shouldCaptureForwardedCodexOutput, which skips piping when both stdout and stderr are tty-attached, and falls back to capture mode for non-tty (ci/pipe) runs so unsupported-model retries can still inspect stderr. it also hardens spawn() with a try/catch to handle synchronous launch failures (e.g. windows EPERM) and adds two explicit-override integration tests.

Confidence Score: 5/5

safe to merge — all remaining findings are P2 style/coverage gaps with no runtime correctness risk

the logic is sound: tty detection uses !== true to safely handle windows undefined isTTY, the two explicit-override tests cover the primary regression scenarios, and the spawn try/catch is a net improvement. the only gaps are the untested tty auto-detect fallback branch and the minor failLaunch/no-capture stderr inconsistency, neither of which causes incorrect retries or data loss in practice

no files require blocking attention; test/codex-bin-wrapper.test.ts could add a test omitting the override to exercise the tty auto-detect path

Important Files Changed

Filename	Overview
scripts/codex.js	adds shouldCaptureForwardedCodexOutput to skip piping when stdin/stdout are TTY, and wraps spawn() in try/catch for synchronous-failure resilience; minor inconsistency where failLaunch still mutates stderr in no-capture mode
test/codex-bin-wrapper.test.ts	adds two integration tests for explicit capture-override paths; TTY auto-detect fallback branch in shouldCaptureForwardedCodexOutput remains untested

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[forwardToRealCodex] --> B[shouldCaptureForwardedCodexOutput]
    B --> C{CODEX_MULTI_AUTH_CAPTURE_FORWARD_OUTPUT}
    C -- "1" --> D[captureOutput = true]
    C -- "0" --> E[captureOutput = false]
    C -- unset --> F{stdout.isTTY === true AND stderr.isTTY === true}
    F -- yes --> E
    F -- no/undefined Windows safe --> D
    D --> G[spawn with stdio: inherit,pipe,pipe]
    E --> H[spawn with stdio: inherit]
    G --> I[buffer stdout+stderr, write to process streams]
    H --> J[child inherits parent TTY, no buffering]
    I --> K[result.output = buffered text]
    J --> L[result.output = empty string]
    K --> M{resolveUnsupportedModelRetryTarget}
    L --> N[retry skipped by design]
    M -- match --> O[retry with fallback model]
    M -- no match --> P[return exitCode]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: scripts/codex.js
Line: 301-306

Comment:
**`failLaunch` leaks into `result.output` even in no-capture mode**

when `captureOutput` is `false` and `spawn()` throws synchronously (e.g. windows EPERM during rebuild), `failLaunch` still appends to `stderr` and `finalize` resolves with `output: \`${stdout}\n${stderr}\`.trim()`. this means `resolveUnsupportedModelRetryTarget` receives a non-empty `result.output` on a spawn error in the no-capture path, breaking the "no-capture output stays empty by design" invariant. in practice, the spawn-error message won't match any unsupported-model pattern, so no incorrect retry fires — but it's a silent contract hole worth closing, especially on windows where EPERM spawns are possible per the PR notes.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/codex-bin-wrapper.test.ts
Line: 1262

Comment:
**missing vitest coverage for TTY auto-detect path**

both new tests set `CODEX_MULTI_AUTH_CAPTURE_FORWARD_OUTPUT` explicitly (`"1"` and `"0"`), so the `else` branch in `shouldCaptureForwardedCodexOutput` — the `process.stdout.isTTY !== true || process.stderr.isTTY !== true` fallback — has zero test coverage. a test that omits the override env var entirely and asserts `captureOutput` behaviour (e.g. via the attempt counter or retry message) would cover that path. the TTY auto-detect branch is the main new production behaviour in this PR.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (2): Last reviewed commit: "docs: clarify no-capture retry behavior" | Re-trigger Greptile}

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

📝 Walkthrough

Walkthrough

this pr fixes issue #436 by restoring TTY compatibility in the codex wrapper. it adds conditional output capture via CODEX_MULTI_AUTH_CAPTURE_FORWARD_OUTPUT env var with automatic TTY-based fallback: when disabled (0), the forwarded real codex process inherits stdio fully; when enabled or on TTY detection, it pipes stdout/stderr for output capture and retry logic.

Changes

Cohort / File(s)	Summary
Output-capture conditional forwarding scripts/codex.js	Introduces shouldCaptureForwardedCodexOutput flag controlled by CODEX_MULTI_AUTH_CAPTURE_FORWARD_OUTPUT env var (1/0) with automatic TTY fallback. Updates forwardToRealCodexOnce to accept options object, spawn with either inherited ("inherit") or piped (["inherit","pipe","pipe"]) stdio depending on capture decision. Buffers stdout/stderr only when capturing. Adds guarded spawn error handling via try/catch.
No-capture forwarding test test/codex-bin-wrapper.test.ts	New test verifying that when CODEX_MULTI_AUTH_CAPTURE_FORWARD_OUTPUT=0, wrapper forwards failing terminal-sensitive command without retry: asserts exit code 1, single invocation, error text in output, no retry message.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Notes

• the new test at test/codex-bin-wrapper.test.ts only covers the disabled-capture (0) path. the automatic TTY detection fallback (!process.stdout.isTTY) needs explicit regression coverage: add a test that forces CODEX_MULTI_AUTH_CAPTURE_FORWARD_OUTPUT=undefined and verifies piped stdio gets applied when stdin/stdout/stderr are non-TTY.

• windows edge case: process.stdout.isTTY behaves differently on windows (often undefined in non-console contexts). verify that the fallback !process.stdout.isTTY correctly defaults to capture mode on windows subprocess execution, preventing silent failures on ci/cd.

• no concurrency risk visible here (synchronous spawn, single process forwarding), but if concurrent codex invocations can race on retry logic, confirm the capture-decision flag doesn't leak state across processes.

• at scripts/codex.js: error handling via try/catch on spawn is good, but verify that if spawn itself throws (e.g., ENOENT for real codex binary), the wrapper exits cleanly and doesn't swallow the original error message.

Suggested labels

bug

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The PR description covers the summary and verification steps performed, but omits required validation checklist items and incomplete risk assessment sections from the template.	Add the validation checklist (npm run lint, typecheck, npm test, etc.), complete the risk/rollback section, and confirm docs/governance updates if user-visible behavior changed.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title follows conventional commits format with lowercase imperative summary under 72 chars, directly aligned to the fix for TTY preservation issue.
Linked Issues check	✅ Passed	Changes directly address #436 by conditionally inheriting stdio for TTY runs while preserving capture for non-TTY fallback logic.
Out of Scope Changes check	✅ Passed	All changes are scoped to TTY forwarding behavior: conditional stdio handling in scripts/codex.js and new regression test in test/codex-bin-wrapper.test.ts.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-436-tty-forwarding

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/issue-436-tty-forwarding

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/codex.js`:
- Around line 256-265: The auto-tty fallback in
shouldCaptureForwardedCodexOutput only reads process.stdout/stderr globals
making that branch hard to exercise in tests — add a unit/integration test in
test/codex-bin-wrapper.test.ts that sets
env.CODEX_MULTI_AUTH_CAPTURE_FORWARD_OUTPUT="1" to exercise the explicit capture
path (ensure the wrapper still captures and triggers the retry path), and update
tests to cover both "1" and "0" overrides; also add a one-line comment above
shouldCaptureForwardedCodexOutput explaining that process.stdout.isTTY may be
undefined on Windows for child processes and that treating undefined as non-tty
(i.e., capture) is intentional to preserve the unsupported-model retry behavior.

In `@test/codex-bin-wrapper.test.ts`:
- Around line 1234-1261: Add a symmetric regression test that asserts the
explicit "1" branch preserves capture and triggers the gpt-5.5→gpt-5.4 retry:
duplicate the existing gpt-5.5→gpt-5.4 retry spec but set
CODEX_MULTI_AUTH_CAPTURE_FORWARD_OUTPUT: "1" in the runWrapper env, keep using
createCustomFakeCodexBin/runWrapper/combinedOutput, and assert
readFileSync(join(stateDir, "attempt.txt"), "utf8") === "2" and output contains
"Retrying with gpt-5.4"; this protects the behavior of
shouldCaptureForwardedCodexOutput in scripts/codex.js from regressions that
invert the explicit "1" branch.
- Around line 1234-1261: The test "can forward without capturing child stdio for
terminal-sensitive Codex runs" leaves CODEX_HOME unpinned causing it to pick up
the host ~/.codex and produce non-deterministic shadow-home behavior; update the
test (where runWrapper and createWrapperFixture are used and the env vars
CODEX_MULTI_AUTH_TEST_STATE_DIR, CODEX_MULTI_AUTH_REAL_CODEX_BIN,
CODEX_MULTI_AUTH_CAPTURE_FORWARD_OUTPUT are set) to set a deterministic
CODEX_HOME in extraEnv (similar to the sibling test that sets CODEX_HOME:
originalHome) so the wrapper will not fall back to join(HOME, ".codex") and
avoid createCompatibilityCodexHome shadowing and potential Windows EBUSY/EPERM
races; ensure the new env key is added to the runWrapper invocation rather than
relying on host HOME forwarding through buildWrapperEnv/WRAPPER_ENV_ALLOWLIST.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 64fd62f9-d806-4f78-a753-7cd6fcb1f471

📥 Commits

Reviewing files that changed from the base of the PR and between 326c054 and 449e099.

📒 Files selected for processing (2)

• scripts/codex.js
• test/codex-bin-wrapper.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (1)

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

• test/codex-bin-wrapper.test.ts

🔇 Additional comments (1)

scripts/codex.js (1)

267-340: stdio switch looks correct; spawn failure path is sound.

the three things i look for here all check out:

• synchronous spawn throw → failLaunch → return at scripts/codex.js:311 prevents the later child.stdout?.on/child.once from dereferencing an undefined child.
• async error + close both funneled through finalize, guarded by settled. no double-resolve, no hanging promise.
• when captureOutput === false, stdio: "inherit" inherits stdin too, which is exactly what the real codex needs to keep its raw-mode tty setup. matches issue #436.

one minor smell: in the no-capture branch the local stderr accumulator at scripts/codex.js:277 is only ever written by failLaunch, and that same message is already emitted via console.error. the output field returned to forwardToRealCodex will therefore be either empty or just the launch-failure line, which correctly causes resolveUnsupportedModelRetryTarget to short-circuit — intended behavior, but worth a one-line comment so future readers don't "helpfully" wire the buffer back up and silently re-enable retries on tty runs.