[codex] Add tmux-aware OSC 9 notifications#17836
Conversation
a1f3a20 to
9938ca0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9938ca0a1c
ℹ️ 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".
c7b17c0 to
97c0aea
Compare
fcoury-oai
left a comment
There was a problem hiding this comment.
Code overall looks good. The only thing is we might have a regression for WezTerm where I pointed it out. Can you add a test to check and fix it if found to be the case?
|
I also tested it with Ghostty and tmux and it worked even without the |
bddbe55 to
a9543e5
Compare
|
Question: did you changes introduce the error you are fixing on the base PR? The code looks good and I was able to replicate the behavior. I don't think you should submit this PR stacked on the other test fix. I would like to approve this PR with the test error and handle the other one separately. Would that be possible? |
I'll restack and we can see! |
a9543e5 to
2d0cf2f
Compare
b42352c to
5dfc788
Compare
@fcoury-oai looks like this is all green now. |
5dfc788 to
8a45c89
Compare
Co-authored-by: Codex <noreply@openai.com>
4b4d9b3 to
dc3936d
Compare
Summary
Stack
Why
Tmux does not forward OSC 9 notifications directly; the sequence has to be wrapped in tmux's DCS passthrough envelope. Codex also had local notification heuristics that could miss supported terminals when running under tmux, even though codex-terminal-detection already knows how to attribute tmux sessions to the client terminal.
Validation
just fmtcargo test -p codex-tui(currently blocked by an unrelated existing compile error inapp-server/src/message_processor.rs:754referencingconnection_idout of scope; not caused by this change)