Repository navigation
fix(setup): show non-interactive setup results in a terminal - #10546
Conversation
Codex Review Summary: ❌ FailedThis 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. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
 [Medium impact] The change affects terminal output for the setup command. Fix the test's inherited Findings
Fix with agent prompt### Issue 1
tests/boundary/cli/cliSetup.test.ts:383-388
If a developer has `CODEX_HOME` set, this new `--yes` test writes to that Codex directory instead of the temporary home. `runSetupInTty` inherits the variable, and Codex path selection gives it precedence over `HOME`. Setup can change the developer's real REA registration and leave it changed after the test fails its temporary-path assertion.
Set `CODEX_HOME` in the shared helper, preferably by reusing `isolatedSetupEnvironment(home)`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryShows setup plans or result summaries when stdout is a terminal and no format was explicitly selected.
Reviews (1) · Last reviewed commit: "fix(setup): show non-interactive setup r..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { exitCode, output } = await runSetupInTty(home, [ | ||
| "--yes", | ||
| "--client", | ||
| "codex", | ||
| "--skill=false", | ||
| ]); |
There was a problem hiding this comment.
If a developer has CODEX_HOME set, this new --yes test writes to that Codex directory instead of the temporary home. runSetupInTty inherits the variable, and Codex path selection gives it precedence over HOME. Setup can change the developer's real REA registration and leave it changed after the test fails its temporary-path assertion.
Set CODEX_HOME in the shared helper, preferably by reusing isolatedSetupEnvironment(home).
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/boundary/cli/cliSetup.test.ts
Line: 383-388
Comment:
**Test changes real Codex settings**
If a developer has `CODEX_HOME` set, this new `--yes` test writes to that Codex directory instead of the temporary home. `runSetupInTty` inherits the variable, and Codex path selection gives it precedence over `HOME`. Setup can change the developer's real REA registration and leave it changed after the test fails its temporary-path assertion.
Set `CODEX_HOME` in the shared helper, preferably by reusing `isolatedSetupEnvironment(home)`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid, thanks, and it affected the existing TTY dry-run tests too. runSetupInTty now spreads the file's isolatedSetupEnvironment(home) (HOME, USERPROFILE, CODEX_HOME and the XDG bases) over the inherited environment. With CODEX_HOME=/nonexistent/... set in the parent, both TTY tests failed before this change and all 30 pass after it.
Setup's result is agent-only, so incur hides it when stdout is a terminal and no format was chosen. Only an interactive setup or a fully interactive TTY dry run rendered a summary, so `rea setup --yes` printed nothing. Render the dry run plan or the existing result summary under incur's own hidden-output condition instead. Fixes #10531 Supersedes #2185 Co-Authored-By: balkan16 <[email protected]> Co-Authored-By: gensokyo-dev <[email protected]>
ea7d5c0 to
60ada9b
Compare
Summary
rea setup --yesin a terminal applied its changes and printed nothing. Setup's result is agent-only, which incur hides when stdout is a TTY and no--formatis given. REA rendered a summary only for an interactive setup, or for a dry run when stdin, stdout and stderr were all terminals. It now renders the dry-run plan or the existing result summary whenever incur hides the result.Fixes #10531 (split out of #2164). Based on #2185 by balkan16 (co-author), reduced to one renderer under one hidden-output check.
Change and scope
setupCommands.ts: after a non-interactive setup, render under incur's own condition (process.stdout.isTTY === true && !formatExplicit). That covers--yes, a setup that needs confirmation because stdin is redirected, and a dry run with redirected stdin. Main's dry-run renderer, and the remediation it now shows, are kept.needs_human/ not-completed renderer and theSetupHostchanges. The existing result renderer already shows the remediation.Contribution scope: ordinary fix within existing boundaries; maintainer scope decision not applicable.
Evidence and regression coverage
cliSetup.test.tsrunssetup --yes --client codex --skill=falseunder node-pty and expects "Restart Codex to load REA." plus the written config. It fails on base and passes with the fix. The TTY dry-run tests now use the samerunSetupInTtyhelper, which applies the file'sisolatedSetupEnvironment(includingCODEX_HOME). Before that review fix, both TTY tests failed when the parent hadCODEX_HOMEset.Validation performed
npm run test:focused -- tests/boundary/cli/cliSetup.test.ts, after rebasing onto current main: 30 passed.npm run check:fast(pre-push): passed.oxfmt --check: clean.Compatibility, safety, and release
--formatis unchanged.Author readiness
Human owner: N0zoM1z0 (PR author, exempt from the declaration per CONTRIBUTING.md).
Supersedes #2185.