fix: integration test for #9011 #9166
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bolinfest
force-pushed
the
pr9166
branch
2 times, most recently
from
e6d8dff to
67ca792
Compare
joshka-oai
approved these changes
Jan 13, 2026
Comment on lines
+82
to
+90
| if chunk.windows(4).any(|window| window == b"\x1b[6n") { | ||
| let _ = writer_tx.send(b"\x1b[1;1R".to_vec()).await; | ||
| } |
| output: String, | ||
| } | ||
|
|
||
| async fn run_codex_cli( |
Collaborator
There was a problem hiding this comment.
Would be good to have a small amount of inline / doc comments on this one to help give details about how it works.
Right now all I could really say is that there's a bunch of code that seems to work and codex understood it when it wrote it. I'd have to read every line to understand this code and then infer constraints / edge cases that are encoded here.
bolinfest
added a commit
that referenced
this pull request
Jan 13, 2026
The underlying issue is that when we encountered an error starting a conversation (any sort of error, though making `$CODEX_HOME/rules` a file rather than folder was the example in #8803), then we were writing the message to stderr, but this could be printed over by our UI framework so the user would not see it. In general, we disallow the use of `eprintln!()` in this part of the code for exactly this reason, though this was suppressed by an `#[allow(clippy::print_stderr)]`. This attempts to clean things up by changing `handle_event()` and `handle_tui_event()` to return a `Result<AppRunControl>` instead of a `Result<bool>`, which is a new type introduced in this PR (and depends on `ExitReason`, also a new type): ```rust #[derive(Debug)] pub(crate) enum AppRunControl { Continue, Exit(ExitReason), } #[derive(Debug, Clone)] pub enum ExitReason { UserRequested, Fatal(String), } ``` This makes it possible to exit the primary control flow of the TUI with richer information. This PR adds `ExitReason` to the existing `AppExitInfo` struct and updates `handle_app_exit()` to print the error and exit code `1` in the event of `ExitReason::Fatal`. I tried to create an integration test for this, but it was a bit involved, so I published it as a separate PR: #9166. For this PR, please have faith in my manual testing! Fixes #8803. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/9011). * #9166 * __->__ #9011
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds an integration test for the new behavior introduced in #9011. The work to create the test setup was substantial enough that I thought it merited a separate PR.
This integration test spawns
codexin TUI mode, which requires spawning a PTY to run successfully, so I had to introduce quite a bit of scaffolding inrun_codex_cli(). I was surprised to discover that we have not done this in our codebase before, so perhaps this should get moved to a common location so it can be reused.The test itself verifies that a malformed
rulesin$CODEX_HOMEprints a human-readable error message and exits nonzero.