Conversation
# Conflicts: # codex-rs/tui/src/custom_terminal.rs # codex-rs/tui/src/diff_render.rs # codex-rs/tui/src/render/renderable.rs
This reverts commit 3d97baa.
WalkthroughThis PR introduces a custom status line rendering system for the TUI with Git and Kubernetes context awareness, adds persistent per-workspace state management, updates the default model to gpt-5.1-codex, refactors GitHub Actions workflows, and restructures Nix build configuration. It includes CLI git SHA embedding, file-locking improvements, and extends configuration for TUI customization. Changes
Sequence DiagramsequenceDiagram
participant App
participant StatusOverlay as StatusLineOverlay
participant BGTask as Background Tasks
participant GitDetect as Git Detection
participant KubeDetect as Kube Detection
participant ChatWidget
participant FrameRequester
App->>StatusOverlay: new() with config flag
activate StatusOverlay
StatusOverlay->>StatusOverlay: init with defaults
App->>StatusOverlay: bootstrap(tokens, messages)
StatusOverlay->>StatusOverlay: sync_model, refresh_environment
StatusOverlay->>BGTask: spawn_background_tasks()
activate BGTask
BGTask->>GitDetect: collect_status_line_git_snapshot
BGTask->>KubeDetect: detect_kube_context_async
deactivate BGTask
GitDetect-->>App: AppEvent::StatusLineGit(snapshot)
KubeDetect-->>App: AppEvent::StatusLineKubeContext(context)
App->>ChatWidget: update_statusline_git(snapshot)
App->>ChatWidget: update_statusline_kube_context(context)
App->>FrameRequester: request_frame()
FrameRequester->>ChatWidget: render with overlay
ChatWidget->>StatusOverlay: layout(area)
ChatWidget->>StatusOverlay: render_status_line(area)
deactivate StatusOverlay
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
Summary of ChangesHello @zcg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a suite of enhancements aimed at improving the user experience and operational transparency of the Codex TUI and CLI. Key changes include a dynamic and informative status line within the TUI, persistent user preferences for models and MCP servers, and better version traceability for the CLI. These updates collectively make the tool more powerful and user-friendly by providing critical context and maintaining user settings seamlessly. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: a custom status line in the TUI, which displays useful context like Git branch and Kubernetes context. It also adds workspace-scoped state persistence. The implementation is quite extensive, especially in the tui crate, with a lot of new modules for the status line. The refactoring of the chat composer layout is a good improvement for maintainability. The addition of the Git commit SHA to the CLI version is also a nice touch for traceability.
However, there are a few critical and medium severity issues that need to be addressed. There are unresolved merge conflicts in some test snapshot files. There's also a potential regression in error handling for image pasting, and some inconsistencies in AI model naming that could cause issues.
| --- | ||
| source: tui/src/chatwidget/tests.rs | ||
| <<<<<<< HEAD | ||
| ======= | ||
| assertion_line: 562 | ||
| >>>>>>> main | ||
| expression: "format!(\"{buf:?}\")" | ||
| --- | ||
| Buffer { |
There was a problem hiding this comment.
This file contains merge conflict markers (<<<<<<< HEAD, =======, >>>>>>> main). Please resolve these conflicts before merging.
|
|
||
| const RATE_LIMIT_WARNING_THRESHOLDS: [f64; 3] = [75.0, 90.0, 95.0]; | ||
| const NUDGE_MODEL_SLUG: &str = "gpt-5.1-codex-mini"; | ||
| const NUDGE_MODEL_SLUG: &str = "gpt-5-codex-mini"; |
There was a problem hiding this comment.
There seems to be an inconsistency in the model names used. In core/src/config/mod.rs, OPENAI_DEFAULT_MODEL is updated to gpt-5.1-codex. However, in this file, NUDGE_MODEL_SLUG is changed from gpt-5.1-codex-mini to gpt-5-codex-mini. Another change on line 2076 checks for gpt-5-codex instead of gpt-5.1-codex for high reasoning effort warnings.
Could you please verify the correct model names and ensure they are used consistently across the codebase to avoid issues with model nudges and warnings?
| if let Ok((path, info)) = paste_image_to_temp_png() { | ||
| self.attach_image(path, info.width, info.height, info.encoded_format.label()); | ||
| } |
There was a problem hiding this comment.
The error handling for paste_image_to_temp_png has been removed. Previously, if pasting an image failed, a warning was logged and an error message was displayed to the user. Now, it fails silently. This could be confusing for users if pasting an image doesn't work and they don't know why. It would be better to restore the error handling.
match paste_image_to_temp_png() {
Ok((path, info)) => {
self.attach_image(path, info.width, info.height, info.encoded_format.label());
}
Err(err) => {
tracing::warn!("failed to paste image: {err}");
self.add_to_history(history_cell::new_error_event(format!(
"Failed to paste image: {err}",
)));
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (21)
codex-rs/cli/build.rs (1)
1-38: Robust git SHA resolution with graceful fallbackThe build script correctly prefers
CODEX_BUILD_GIT_SHA, falls back togit rev-parse HEAD, and degrades to"unknown"without panicking, then exposesCODEX_CLI_GIT_SHAfor use in the binary—this is a solid, failure-tolerant setup. As a minor incremental-build improvement, you could also track changes to the env var:fn main() { - println!("cargo:rerun-if-changed=build.rs"); + println!("cargo:rerun-if-changed=build.rs"); + println!("cargo:rerun-if-env-changed=CODEX_BUILD_GIT_SHA");so toggling
CODEX_BUILD_GIT_SHAalone will retrigger the build script.flake.nix (3)
17-17: Pin the nightly toolchain to a specific date for reproducibility.Using
nightly.latestmeans the Rust toolchain version changes over time, which can cause non-reproducible builds or unexpected breakages when the nightly channel updates.Consider pinning to a specific nightly date:
- rustToolchain = pkgs.rust-bin.nightly.latest.default; + rustToolchain = pkgs.rust-bin.nightly."2024-11-01".default;
35-38: Inconsistent hash formats in outputHashes.Line 36 uses SRI format (
sha256-...) while line 37 uses base32 format. Both work, but using a consistent format improves readability.outputHashes = { "ratatui-0.29.0" = "sha256-HBvT5c8GsiCxMffNjJGLmHnvG77A6cqEL+1ARurBXho="; - "crossterm-0.28.1" = "0vzgpvbri4m4qydkj50ch468az7myy04qh5z2n500p1f4dysv87a"; + "crossterm-0.28.1" = "sha256-XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX="; };You can convert the base32 hash to SRI format using
nix hash to-sri --type sha256 <base32-hash>.
97-106: Consider clearer variable naming for app wrappers.The variable
codexAppwrapscodex-tui, which is confusing since it sounds like the main app. Consider renaming for clarity.apps = let codexCliApp = flake-utils.lib.mkApp { drv = codex-cli; }; - codexApp = flake-utils.lib.mkApp { drv = codex-tui; }; + codexTuiApp = flake-utils.lib.mkApp { drv = codex-tui; }; in { codex = codexCliApp; codex-cli = codexCliApp; - codex-tui = codexApp; + codex-tui = codexTuiApp; default = codexCliApp; };codex-rs/core/src/message_history.rs (2)
222-262: Consider logging unlock failures in the read path.The unlock results are ignored with
let _ = FileExt::unlock(&file)at lines 231, 244, and 251. While less critical than exclusive lock failures (since shared locks permit concurrent readers), a failed unlock could still prevent future exclusive locks (writes) from succeeding.Consider logging unlock failures to aid debugging if lock-related issues occur in production.
Apply this pattern to the three unlock sites:
- let _ = FileExt::unlock(&file); + if let Err(e) = FileExt::unlock(&file) { + tracing::warn!(error = %e, "failed to unlock history file after read error"); + }
223-252: Consider optimizing lookup for large offsets.The current implementation reads and parses every line from the start up to the target offset. For large history files, this could be inefficient when looking up recent entries.
If performance becomes an issue, consider:
- Maintaining an index file with byte offsets
- Using a more sophisticated data structure
- Or accepting the current O(n) behavior if history files remain small
codex-rs/core/src/config/mod.rs (1)
61-65: Be aware of tests gated on OPENAI_DEFAULT_MODELBumping
OPENAI_DEFAULT_MODELto"gpt-5.1-codex"will cause tests likebinary_size_transcript_snapshot(which early‑returns unless the default is"gpt-5-codex") to be skipped. That looks intentional per the test comment, but worth confirming that you no longer expect that snapshot to run under the new default.codex-rs/tui/src/statusline/state.rs (1)
27-288: StatusLineState and RunTimer logic look correct, with a couple of small polish opportunitiesThe state machine around
start_task/complete_task,set_idle_run_state, andsnapshot_for_renderis coherent and integrates cleanly withFrameRequesterandRunTimer. Timer pause/resume semantics and queued‑message / interrupt‑hint propagation intoStatusLineRunStateare sound.Two minor polish ideas you could consider later:
set_session_idis currently a no‑op; either wire it into the snapshot (e.g., for future display) or document it as intentionally unused to avoid confusion.- If
TokenUsageInfo.model_context_windowcan be more accurate than the config hint over time, you might want to cache it intoself.context_window_hintwhen present, so subsequent updates without a window still use the most recent value.Neither is blocking; the current behavior is consistent.
Also applies to: 290-340
codex-rs/tui/src/chatwidget/tests.rs (2)
2828-2988: Status overlay layout tests are valuable; consider tightening assumptions about popup heightThe two new tests around cursor alignment and composer padding/margins under an active status overlay are quite thorough and should prevent a lot of subtle regressions in layout and styling.
One small robustness improvement for
typed_input_preserves_padding_and_margin_with_status_overlay:
- The test assumes there is at least one padding row and one margin row (
margin_y = popup_rect.y + 1), but only asserts thatpopup_rect.height >= 1. Ifcomposer_layout_for_testsever returns apopup_rectthat’s only a single row tall, indexing atmargin_ywill end up outside that rectangle (and potentially out of bounds if the pane is tight).You could either:
- assert
popup_rect.height >= 2, or- compute the margin row from
popup_rectand/orpane_areamore defensively (e.g.,let margin_y = (popup_rect.y + 1).min(area.y + area.height - 1)).Not urgent, but it would make the test more future‑proof against layout refactors.
2994-3077: VT100 snapshot test nicely exercises statusline environment, but uses a global devspace override
chatwidget_exec_and_status_layout_vt100_snapshotnow seeds the status overlay with deterministic devspace/hostname/AWS/kube context, which is great for stabilizing the snapshot.Minor consideration:
set_devspace_override_for_testsandclear_devspace_override_for_testsrely on a global, mutex‑protected override. Because Rust tests can run in parallel, there’s a theoretical window betweenset_*andclear_*where a different test could observe the override. The current usage is very short‑lived and probably fine, but if flakiness ever shows up you may want to:
- gate this test with
-- --test-threads=1(or a serialization helper), or- move the override entirely behind
StatusLineOverlay::newparameters and avoid the global for this path.Functionally, the test is well structured.
codex-rs/tui/src/chatwidget.rs (5)
229-240: Status overlay wiring inChatWidgetlooks coherent; consider documenting invariantsThe addition of
status_rendererinChatWidgetInit, thestatus_overlay: Option<StatusLineOverlay>field, and the helper methods (update_statusline_git,update_statusline_kube_context,set_status_renderer,status_line_mut) are all consistently guarded onstatus_overlay.is_some()and keep the header in sync viaset_status_header. From this file it appears:
StatusLineOverlay::new(...)is the only place that can yieldNone, and every later use treatsstatus_overlayas truly optional.set_status_headeris now the single source of truth for both the bottom-pane status indicator and the run header shown by the overlay, which is good for consistency.It would help future maintainers if you briefly documented the expected invariant, e.g. whether
status_overlayisSomewheneverconfig.tui_custom_statuslineis enabled and whether it can legitimately flip betweenSomeandNoneat runtime, since a lot of callers now branch on that option.Also applies to: 250-299, 341-368, 369-375
377-387: Keep status overlay state aligned with session + token + queue stateThe new overlay hooks in:
on_session_configured(set session id, sync model, spawn background tasks),set_token_info(propagate token usage tooverlay.update_tokens),on_undo_started(mirror interrupt visibility and header), andrefresh_queued_user_messages(mirror queued messages into the overlay)all look internally consistent and side-effect free when
status_overlayisNone. This should keep the overlay’s session/model/tokens/queue view in step with the bottom pane.One thing to watch:
overlay.spawn_background_tasks()is now called both inon_session_configuredand after task completion; if that method isn’t idempotent, you might end up with duplicated background workers over a long session. If it already self-guards, no change is needed; otherwise consider making it idempotent or centralizing its call site.Also applies to: 538-555, 791-803, 1817-1827
902-950: Status overlay integration for exec/patch/MCP flows is consistent; minor duplication onlyThe new overlay interactions on command and patch flows:
handle_exec_begin_now/handle_mcp_begin_nowresume the timer and set a specific run header viaexec_status_label/tool_status_label.handle_exec_end_nowandhandle_patch_apply_end_nowreset the run header to"Working"and, when there are no more running commands or a patch has completed, calloverlay.refresh_git()so the git snapshot matches the new workspace state.handle_exec_approval_nowandhandle_apply_patch_approval_nowset approval-specific run headers and also emit the newNotification::EditApprovalRequestedvariant as appropriate.handle_mcp_end_nowresets the header but deliberately does not refresh git (reasonable if MCP calls are not expected to touch the repo).This all lines up: the overlay’s run header is a concise reflection of what the system is currently waiting on, and git context refreshing is only done in places that actually change the tree.
If you see more of these lifecycle hooks proliferate, consider pulling the “set header + maybe refresh git” pattern into a tiny helper (e.g.,
set_idle_header_and_refresh_git_if_idle()) to avoid subtle drift between handlers, but there’s no immediate correctness issue here.Also applies to: 952-967, 969-1004, 1012-1097
1099-1179: Constructors correctly bootstrap the overlay; consider avoiding double work on empty queuesBoth
ChatWidget::newandChatWidget::new_from_existingnow:
- clone
frame_requesterso both the widget andBottomPanehave handles,- build a
StatusLineOverlaywithconfig, a clonedFrameRequester,app_event_tx, and the optionalstatus_renderer,- initialize
status_overlayon the struct, and- immediately call
overlay.bootstrap(&config, token_info.clone(), queued_messages)followed byrefresh_queued_user_messages().Given that
queued_user_messagesandtoken_infoare empty at construction time, the bootstrap call is effectively a no‑op today;refresh_queued_user_messages()will also push an empty queue into both the bottom pane and overlay.That’s fine functionally, but if you ever add heavier work to
bootstrap(e.g., initial git scan) you may want to:
- guard the call behind “any initial state to sync?” or
- let
on_session_configuredown heavyweight initialization, with constructors only wiring references.Also applies to: 1181-1265
2885-2908:BottomPaneWithOverlaynicely isolates layout; double‑check future changes tohas_active_viewWrapping the bottom pane in
BottomPaneWithOverlayand exposing it viabottom_pane_renderable()is a clean way to:
- hide the bottom‑pane status indicator when an overlay is active (
overlayis filtered out whenbottom_pane.has_active_view()is true),- let
StatusLineOverlay::layoutchoosepane_area,run_pill_area, andstatus_line_area, and- keep
desired_heightconsistent by addingStatusLineOverlay::reserved_rows()only when an overlay is actually being rendered and there’s no active view.The render and cursor logic both respect the computed
StatusLineLayout, and the use ofClearbefore drawing overlay regions avoids visual overlap with previous frames.The only subtle invariant is that
bottom_pane_renderable()uses the value ofhas_active_view()at the time it’s called to decide whetheroverlayisSome, whileoverlay_layout()recomputeshas_active_view()for the layout call. As long asBottomPanecan’t mutate its active‑view flag during a single render pass (which is true in this architecture), this stays consistent; just keep that in mind if you ever introduce side‑effectful rendering or background UI updates.Also applies to: 2926-2984
codex-rs/tui/src/bottom_pane/chat_composer.rs (2)
139-153:fill_rect_with_styleis straightforward; confirmBuffer::cell_mutsemantics match your ratatui version
fill_rect_with_styledoes the expected thing—fills a rect with spaces and the provided style, with early exit on zero-sized rects. It’s used to:
- paint the top padding and composer background with
user_message_style, and- initialize the footer and bottom margin rows with default style before delegating to overlays/footers.
A couple of minor points:
- This runs on every render over the composer region; the areas are small, so the double loop is fine, but if you later need to optimize, you could switch to iterating over a slice of the buffer if ratatui exposes region iterators.
- The
if let Some(cell) = buf.cell_mut((x, y))pattern assumescell_mutreturns anOption<&mut Cell>. Some ratatui versions exposecell_mut/get_mutas infallible. Please double‑check this against the ratatui version you’re pinned to and adjust (or gate via a small extension trait) if the signature differs, otherwise this will be a type error.Also applies to: 1687-1693
717-839: File-search popup: image attachment path is careful about UTF‑8 and token boundariesThe changes in
handle_key_event_with_file_popupand the newis_image_pathhelper add a nice quality-of-life improvement:
- When a file-search selection ends with
.png/.jpg/.jpeg, you now attempt to treat it as an image:
- verify dimensions via
image::image_dimensions,- compute token boundaries around the cursor using the same whitespace-based logic as
insert_selected_path,- remove the current
@tokenslice, and- attach an image placeholder plus a trailing space, with a label derived from the extension.
- If dimension probing fails or the path doesn’t look like an image, you fall back to
insert_selected_pathand keep the existing behavior.- All slicing is guarded via
clamp_to_char_boundary, so even with multi-byte tokens thestart_idx..end_idxrange stays on valid UTF‑8 boundaries.This is a good reuse of the existing token-boundary approach and should feel natural in the UI. If you see further duplication creep between this and
insert_selected_path, you could consider a small shared helper that returns(start_idx, end_idx)for the active token, but it’s not urgent.Also applies to: 841-845, 948-997
codex-rs/tui/src/statusline/mod.rs (2)
398-419: Consider tightening the attempts limit.The
attempts > 8guard is conservative sincedegrade_run_capsuleonly defines 2 degradation operations. The loop will exit after at most 2 successful degradations anyway, making the high limit unnecessary (though harmless).attempts += 1; - if attempts > 8 { + if attempts > 3 { return truncate_line_to_width(Line::from(spans), target_width); }
1059-1073: Consider documenting the distinction fromtruncate_text.This function uses single-character "…" (U+2026) while
truncate_textintext_formatting.rsuses "...". The choice is appropriate for the space-constrained status line, but a brief doc comment explaining the rationale would help maintainers understand why both exist.+/// Truncates text to a maximum number of graphemes, appending "…" if truncated. +/// Uses single-character ellipsis for space efficiency in the status line, +/// unlike `truncate_text` which uses "..." for general text formatting. fn truncate_graphemes(text: &str, max_graphemes: usize) -> String {codex-rs/tui/src/statusline/overlay.rs (2)
108-129: Consider logging errors from background tasks.The fire-and-forget spawning pattern is appropriate for background refresh, but task failures are silently ignored. Consider adding error logging to aid debugging if git or kube detection fails unexpectedly.
handle.spawn(async move { let snapshot = collect_status_line_git_snapshot(cwd).await; + if snapshot.is_none() { + tracing::debug!("git status collection returned no data"); + } tx.send(AppEvent::StatusLineGit(snapshot)); });
353-371: Simplistic YAML parsing may miss edge cases.The line-by-line parsing for
current-context:works for typical kubeconfig files but could fail with quoted values, multi-line strings, or unusual formatting. Consider using a YAML parser likeserde_yamlfor robustness, or document this limitation.For typical kubeconfig files, this works. If edge cases become problematic, consider:
// Using serde_yaml for robust parsing #[derive(Deserialize)] struct KubeConfigPartial { #[serde(rename = "current-context")] current_context: Option<String>, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (37)
codex-rs/Cargo.lockis excluded by!**/*.lockcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_ctrl_c_interrupt.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_ctrl_c_quit.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_ctrl_c_then_esc_hint.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_esc_hint_backtrack.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_esc_hint_from_overlay.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_hidden_while_typing.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_overlay_then_external_esc_hint.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shortcut_overlay.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_context_running.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_default.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__queued_messages_visible_when_status_hidden_snapshot.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_and_composer_fill_height_without_bottom_padding.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_and_composer_include_bottom_padding.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_and_queued_messages_snapshot.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_1.snapis excluded by!**/*.snapcodex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_2.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_idle_h3.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h2.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h3.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chatwidget_exec_and_status_layout_vt100_snapshot.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chatwidget_tall.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__rate_limit_switch_prompt_popup.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_band_snapshot.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_active.snapis excluded by!**/*.snapcodex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snapis excluded by!**/*.snapcodex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_manual.snapis excluded by!**/*.snapcodex-rs/tui/src/statusline/snapshots/codex_tui__statusline__tests__statusline_narrow_40.snapis excluded by!**/*.snapcodex-rs/tui/src/statusline/snapshots/codex_tui__statusline__tests__statusline_wide_80.snapis excluded by!**/*.snapflake.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.github/workflows/codex-cli-bundles.yml(1 hunks).github/workflows/rust-ci.yml(0 hunks)codex-rs/Cargo.toml(1 hunks)codex-rs/cli/Cargo.toml(1 hunks)codex-rs/cli/build.rs(1 hunks)codex-rs/cli/src/main.rs(2 hunks)codex-rs/core/Cargo.toml(1 hunks)codex-rs/core/src/config/mod.rs(7 hunks)codex-rs/core/src/config/types.rs(1 hunks)codex-rs/core/src/lib.rs(1 hunks)codex-rs/core/src/message_history.rs(3 hunks)codex-rs/core/src/workspace_state.rs(1 hunks)codex-rs/protocol/src/protocol.rs(1 hunks)codex-rs/tui/Cargo.toml(1 hunks)codex-rs/tui/src/app.rs(4 hunks)codex-rs/tui/src/app_backtrack.rs(1 hunks)codex-rs/tui/src/app_event.rs(2 hunks)codex-rs/tui/src/bottom_pane/chat_composer.rs(9 hunks)codex-rs/tui/src/bottom_pane/footer.rs(3 hunks)codex-rs/tui/src/bottom_pane/mod.rs(2 hunks)codex-rs/tui/src/chatwidget.rs(32 hunks)codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chatwidget_exec_and_status_layout_vt100_snapshot.snap.new(1 hunks)codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__model_reasoning_selection_popup.snap.new(1 hunks)codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__rate_limit_switch_prompt_popup.snap.new(1 hunks)codex-rs/tui/src/chatwidget/tests.rs(7 hunks)codex-rs/tui/src/lib.rs(1 hunks)codex-rs/tui/src/status/mod.rs(1 hunks)codex-rs/tui/src/statusline/mod.rs(1 hunks)codex-rs/tui/src/statusline/overlay.rs(1 hunks)codex-rs/tui/src/statusline/palette.rs(1 hunks)codex-rs/tui/src/statusline/skins/mod.rs(1 hunks)codex-rs/tui/src/statusline/state.rs(1 hunks)customization-plan.md(1 hunks)flake.nix(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/rust-ci.yml
🧰 Additional context used
🧬 Code graph analysis (7)
codex-rs/tui/src/status/mod.rs (2)
codex-rs/tui/src/status/format.rs (2)
line_display_width(95-99)truncate_line_to_width(101-147)codex-rs/tui/src/status/helpers.rs (1)
format_directory_display(140-161)
codex-rs/tui/src/statusline/skins/mod.rs (1)
codex-rs/tui/src/statusline/mod.rs (5)
text(1005-1010)render(71-71)render(346-348)render_run_pill(73-78)render_run_pill(350-357)
codex-rs/tui/src/statusline/palette.rs (3)
codex-rs/tui/src/render/highlight.rs (1)
style(52-57)codex-rs/protocol/src/protocol.rs (2)
default(1076-1087)default(1173-1180)codex-rs/tui/src/statusline/mod.rs (1)
default(210-219)
codex-rs/core/src/config/mod.rs (1)
codex-rs/core/src/config/types.rs (3)
default(323-329)default(340-342)default(358-363)
codex-rs/tui/src/bottom_pane/mod.rs (2)
codex-rs/core/tests/common/test_codex.rs (1)
test(203-205)codex-rs/tui/src/custom_terminal.rs (1)
area(70-72)
codex-rs/tui/src/chatwidget/tests.rs (1)
codex-rs/tui/src/statusline/overlay.rs (3)
clear_devspace_override_for_tests(398-400)set_devspace_override_for_tests(393-395)layout(184-229)
codex-rs/tui/src/statusline/overlay.rs (4)
codex-rs/tui/src/text_formatting.rs (1)
truncate_text(79-103)codex-rs/core/src/git_info.rs (1)
collect_git_info(57-109)codex-rs/tui/src/statusline/mod.rs (5)
new(323-330)new(438-459)line(1454-1464)text(1005-1010)default(210-219)codex-rs/tui/src/statusline/state.rs (1)
with_renderer(48-67)
🪛 Gitleaks (8.29.0)
flake.nix
[high] 25-25: Sourcegraph is a code search and navigation engine.
(sourcegraph-access-token)
[high] 30-30: Sourcegraph is a code search and navigation engine.
(sourcegraph-access-token)
🔇 Additional comments (74)
codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__rate_limit_switch_prompt_popup.snap.new (1)
1-6: Snapshot metadata and popup text look consistent and focusedThe snapshot cleanly captures just the popup’s user-facing prompt and ties it to the correct test source and expression; nothing here appears brittle or problematic for future maintenance.
codex-rs/protocol/src/protocol.rs (1)
395-400: TMPDIR guard refactor looks correct and preserves behaviorThe nested
ifstructure cleanly preserves the original intent:$TMPDIRis added only whenexclude_tmpdir_env_varisfalse, the variable exists, and it is non-empty. No correctness or security regressions are apparent.codex-rs/cli/Cargo.toml (1)
5-5: Build script wiring for CLI git SHA is correct
build = "build.rs"cleanly hooks the new build script into this crate without altering public interfaces or dependency behavior; no issues found.codex-rs/cli/src/main.rs (1)
39-44: CLI_VERSION wiring and test coverage look solidThe const
CLI_VERSIONcorrectly incorporates the crate version and embedded SHA, is properly referenced viaversion = CLI_VERSIONin the clap attributes, and theversion_flag_reports_commit_shatest gives good protection against regressions in how the version string is exposed.Also applies to: 52-54, 736-741
flake.nix (3)
2-6: LGTM!The input definitions are standard and appropriate for a Rust project with Nix flakes.
23-32: LGTM!Source dependencies are properly pinned with specific commits and hashes, ensuring reproducible builds. The static analysis warnings for these lines are false positives—these are standard git commit SHAs, not secrets.
74-74: Verify the homepage URL matches this repository.The homepage metadata points to
github.com/sourcegraph/codex, but this PR is ingithub.com/zcg/codex. If this is a fork intended to diverge, consider updating the homepage to reflect the correct repository.Also applies to: 85-85
codex-rs/core/src/message_history.rs (1)
30-30: LGTM!The fs2::FileExt import is correctly placed and necessary for the new file locking implementation.
codex-rs/core/Cargo.toml (2)
87-87: All imports correctly use the new underscore naming convention.The verification confirms that all code references have been properly updated to use
codex_windows_sandboxinstead ofcodex-windows-sandbox. The only remaining reference to the hyphenated name is in a comment atcodex-rs/core/src/sandboxing/mod.rs:144, which does not affect functionality. The dependency name change has been correctly propagated throughout the codebase.
86-86: fs2 version 0.4.3 is correct and current. The latest stable version of the Rust crate fs2 is 0.4.3. The dependency addition is using an up-to-date version.codex-rs/tui/src/app_backtrack.rs (1)
350-350: LGTM! Clean integration point for status renderer.The addition of the
status_renderer: Nonefield toChatWidgetInitfollows the optional, layered approach described in the PR's customization plan. Initializing toNonemaintains backward compatibility and keeps the status line feature opt-in.codex-rs/tui/src/bottom_pane/footer.rs (4)
75-77: Clear documentation of new default-hidden behavior.The updated comment effectively explains the intentional design decision to hide the context meter by default in ShortcutSummary mode.
82-82: Implements hidden-by-default footer in summary mode.Returning an empty vector in
ShortcutSummarymode effectively hides the footer during normal operation, which aligns with the PR's goal of keeping the UI cleaner.
217-220: Context meter intentionally disabled.The function signature change to return
Option<Line>and the no-op implementation (returningNone) clearly indicate the context meter feature has been intentionally disabled. The unused parameter is appropriately prefixed with_.
466-485: Good test coverage for context meter hiding.The test ensures the context meter text doesn't appear even when
context_window_percentis provided, confirming the expected behavior change..github/workflows/codex-cli-bundles.yml (5)
1-11: LGTM! Clean workflow structure.The workflow metadata is well-defined with appropriate trigger conditions and concurrency control to prevent redundant builds.
19-28: Good platform coverage with appropriate runners.The matrix covers the essential platforms (Linux x86_64, macOS x86_64, and macOS ARM64) with correct runner selections.
38-49: Comprehensive Nix directory caching.The cache configuration covers all relevant Nix directories across both Unix and macOS platforms, with appropriate fallback keys.
61-65: Build command structure looks correct.The Nix build command uses appropriate flags (
--print-build-logs,--log-format raw,--no-link,--print-out-paths) and targets the correct package path with matrix system substitution.
54-59: Manual verification required: CACHIX_AUTH_TOKEN secret access restricted.The verification script encountered a permission error (HTTP 403) when attempting to check the repository secrets, preventing automated confirmation of whether
CACHIX_AUTH_TOKENis configured. You need to manually verify that the secret is properly set in the repository settings:
- Navigate to repository Settings → Secrets and variables → Actions
- Confirm
CACHIX_AUTH_TOKENexists and is properly configured for thejoshsymondsCachix cachecustomization-plan.md (1)
1-36: Excellent documentation for maintaining custom TUI changes.This playbook provides clear guidance on the layering strategy, automation approach, and testing requirements. The "Layer, Don't Fork" philosophy and the concrete workflow checklist will help maintain clean separation from upstream changes.
Key strengths:
- Clear separation between upstream and custom code
- Emphasis on config-flag-based feature activation
- Practical automation guidance with git rerere and patch management
- Comprehensive testing strategy including snapshot tests
codex-rs/tui/src/statusline/palette.rs (2)
5-37: Well-defined color palette with appropriate linting allowances.The color constants appear to follow the Catppuccin color scheme (based on names like LAVENDER, MAUVE, ROSEWATER), which provides a cohesive visual design. The
clippy::disallowed_methodsallowance is appropriate for direct RGB color definitions.The
#[allow(dead_code)]onSURFACE0suggests it's reserved for future use or conditionally used—this is acceptable for a palette module.
39-43: Queue preview style appropriately styled.The combination of
SUBTEXT0color withITALIC | DIMmodifiers creates a visually distinct style for queue previews, helping differentiate them from active content.codex-rs/tui/src/lib.rs (1)
69-69: LGTM! Clean module integration.The statusline module declaration follows the existing crate structure and enables the new status line rendering system introduced in this PR.
codex-rs/core/src/lib.rs (1)
86-86: Workspace state module verified and approved.The verification confirms that
workspace_statemodule is properly implemented atcodex-rs/core/src/workspace_state.rswith a well-defined public API (WorkspaceState, WorkspaceMcpServerState structs, and load_workspace_state, persist_model_selection, persist_mcp_enabled functions) and existing test coverage.codex-rs/tui/Cargo.toml (1)
50-50: Hostname dependency is properly imported and actively used.Verification confirms the hostname crate is correctly imported in
codex-rs/tui/src/statusline/overlay.rs(line 17) and invoked at line 294 to fetch the system hostname, which is then displayed in the status line. The integration is complete and functioning as intended.codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__model_reasoning_selection_popup.snap.new (1)
1-13: New snapshot file needs acceptance.This
.snap.newfile represents a pending snapshot that needs to be reviewed and accepted. Runcargo insta reviewto accept the snapshot orcargo insta acceptto accept all pending snapshots.codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chatwidget_exec_and_status_layout_vt100_snapshot.snap.new (1)
1-20: New snapshot file needs acceptance.This
.snap.newfile captures the new status line overlay with Git, model, and Kubernetes context rendering. Runcargo insta reviewto accept after verifying the expected output matches the implementation.codex-rs/tui/src/app_event.rs (2)
11-11: LGTM!Import properly added for the new
StatusLineGitSnapshottype.
45-48: LGTM!New
AppEventvariants for status line updates follow the existing enum patterns with clear documentation comments. UsingOptionappropriately handles the case when Git/Kubernetes context is unavailable.codex-rs/tui/src/status/mod.rs (1)
8-10: LGTM!These re-exports properly expose formatting utilities needed by the status line rendering system while maintaining crate-level visibility.
codex-rs/tui/src/bottom_pane/mod.rs (2)
121-123: LGTM!Simple predicate method that exposes whether a view is active, enabling the status line overlay to make layout decisions.
264-267: LGTM!Test helper follows the existing pattern of exposing internal state for testing purposes, properly gated behind
#[cfg(test)].codex-rs/tui/src/app.rs (4)
210-210: LGTM!Setting
status_renderer: Nonefor the fresh session initialization is consistent with the pattern across all ChatWidgetInit constructions in production code.
234-234: LGTM!Consistent with the other initialization paths.
379-379: LGTM!Consistent with the other initialization paths for
NewSessionevent handling.
479-486: LGTM!Event handlers for the new status line events follow the established pattern: update the widget state and schedule a frame redraw. The handling is clean and consistent with similar event handlers in this match block.
codex-rs/core/src/config/types.rs (1)
345-370: Tuicustom_statuslinedefaults are wired consistentlyThe new
custom_statuslinefield, its serde default (Tui::default_custom_statusline) and the manualDefaultimpl forTuiare consistent and keep behavior well-defined when[tui]is missing or partial. No issues from a config/serde perspective.codex-rs/tui/src/statusline/skins/mod.rs (1)
1-26: CustomStatusLineRenderer is a clean pass‑through wrapperThe renderer correctly delegates to
render_status_lineandrender_status_run_pill, keeping all layout logic centralized. Zero-sized,Debug+Defaultare appropriate here.codex-rs/core/src/config/mod.rs (3)
159-163: Newtui_custom_statuslinefield on Config is well‑placedAdding
tui_custom_statusline: boolalongsidetui_notificationsonConfigcleanly exposes the UI toggle at the core layer. Public visibility and doc comment match how it’s consumed from the TUI.
1165-1174: Config→TUI wiring fortui_custom_statuslineis correctResolving
tui_custom_statuslineviacfg.tui.as_ref().map(|t| t.custom_statusline).unwrap_or_else(|| Tui::default().custom_statusline)ensures:
- explicit
[tui].custom_statuslineis honored,- bare
[tui]section gets the serde default (true),- missing
[tui]falls back toTui::default()(true).This is consistent with the new
Tuidefaults.
2850-3148: Precedence fixture expectations updated correctly for new fieldThe precedence tests now include
tui_custom_statusline: truein the expectedConfiginstances. This keeps the “full struct equality” assertions robust against the new field and matches the defaulting behavior fromTui::default().codex-rs/tui/src/statusline/state.rs (1)
342-399: Context‑window math and timer snapshot behavior are well‑specified and tested
context_percent_remaininguses a fixed 12k baseline, saturating arithmetic, and clamps to[0,100], which matches the expectations incontext_snapshot_matches_status_values.RunTimer::snapshotplusRunTimerSnapshot::elapsed_atare validated byrun_timer_snapshot_advances_in_real_seconds, and the use ofsaturating_duration_sinceavoids underflow. This all looks correct.Also applies to: 401-447
codex-rs/core/src/workspace_state.rs (1)
16-113: WorkspaceState persistence is straightforward and resilientThe SHA‑1–based
workspace_state_path, gracefulNotFoundhandling, and “warn + default” behavior on TOML parse errors give you robust, non‑disruptive per‑workspace state. Pruning empty MCP entries before write and using a temp‑file + persist pattern avoids stale/partial writes. The public helpers (persist_model_selection,persist_mcp_enabled) have clear semantics and are covered by the test. No issues from a correctness standpoint.codex-rs/tui/src/chatwidget/tests.rs (3)
4-8: New statusline imports match later usageBringing
CustomStatusLineRenderer,StatusLineOverlay,StatusLineRenderer, and the style types into scope is consistent with the added helpers and tests; there’s no unused or mismatched import here.Also applies to: 59-60
264-307: ChatWidget test harness now cleanly wires in StatusLineOverlay
make_chatwidget_with_configand its wrappers (make_chatwidget_manual,make_chatwidget_manual_with_custom_statusline) correctly:
- share a
FrameRequesterbetween the bottom pane and the status overlay,- gate the custom renderer on
cfg.tui_custom_statusline,- bootstrap the overlay once and then refresh queued messages.
This gives tests a single, consistent construction path for both configurations (with and without custom statusline) and keeps the struct literal for
ChatWidgetup to date (including the newstatus_overlayandstatus_rendererinit path).
848-873: ESC interrupt test exercises both bottom‑pane and statusline reset
esc_interrupt_resets_status_indicator_and_statuslineis a good end‑to‑end check: it asserts the emittedOp::Interrupt, verifies the bottom pane clears its running/indicator state, and also inspects the statusline snapshot to confirm the label returns to"Ready when you are". This should catch regressions in how ESC wiring interacts with the overlay.codex-rs/tui/src/chatwidget.rs (3)
496-512: Centralize task teardown so the overlay can’t get “stuck” in a running stateThe task lifecycle is now split across several handlers:
on_task_startedsetsbottom_pane.set_task_running(true), hides the old status indicator when an overlay is active, and callsoverlay.start_task("Working").on_task_completeclears running state, callsoverlay.complete_task(), and (re)spawns background tasks.halt_running_task(used for Esc/Ctrl‑C while a task is running) clears bottom‑pane state, cancels running commands, callsoverlay.complete_task(), and submitsOp::Interrupt.finalize_turn(used fromon_errorandon_interrupted_turn) clears bottom‑pane state and running commands, but does not touch the overlay.If codex-core ever ends a turn via
ErrororTurnAbortedwithout also emitting aTaskComplete,finalize_turnwill run but the overlay will never seecomplete_task(), so its timer/run-pill could remain in a “running” state even though the bottom pane has stopped.It would be more robust to funnel all “task has ended” flows through a single helper that:
- resets bottom‑pane flags,
- clears
running_commandsandstream_controller, and- calls
overlay.complete_task()when present.You could either:
- move the overlay cleanup into
finalize_turn()and haveon_task_complete/halt_running_taskcall that helper, or- have
finalize_turn()explicitly delegate to a newfinish_task_ui()that owns all shared teardown, including the overlay.[ suggest_recommended_refactor ]
Also applies to: 514-537, 612-621, 623-672, 2618-2644
1267-1300: ESC/Ctrl‑C behavior for running tasks is clearer; ESC short‑circuits appropriatelyThe changes to input handling:
- Treat
Ctrl‑Vas “paste image if possible, otherwise no-op”, which simplifies error handling for failed image pastes.- Intercept
Escpresses whilebottom_pane.is_task_running()and callhalt_running_task()instead of letting the composer consume Esc as UI chrome.- Have
on_ctrl_cdelegate tohalt_running_task()when a task is running, and only fall back to shutdown when idle.
halt_running_task()centralizes the “user‑initiated interrupt” behavior by:
- clearing the Ctrl‑C hint and interrupt hint,
- resetting
is_task_runningandrunning_commands,- turning off overlay interrupt hints and calling
overlay.complete_task(), and- sending
Op::Interruptplus a redraw.This is a good separation between “cancel this turn” and “shut down the app”, and keeps the overlay and bottom pane in sync for user‑driven interrupts.
Also applies to: 2618-2644
1817-1828: Queued messages are now surfaced coherently to both bottom pane and overlay
refresh_queued_user_messagesnow:
- rebuilds a
Vec<String>of queued texts,- passes a clone to
bottom_pane.set_queued_user_messages, and- forwards the original vector into
overlay.set_queued_messageswhen present.The small clone is negligible here and ensures both the composer UI and status overlay share an identical view of queued inputs. No issues.
codex-rs/tui/src/bottom_pane/chat_composer.rs (3)
120-137: New layout abstraction (ComposerRenderLayout) is sound and keeps rendering/cursor math consistentThe introduction of:
- explicit layout constants (
TOP_PADDING_HEIGHT,BOTTOM_PADDING_HEIGHT,BOTTOM_MARGIN_HEIGHT,TEXTAREA_RIGHT_MARGIN,FOOTER_SPACING_HEIGHT),- the
ComposerRenderLayoutstruct, and- shared helpers
desired_height,render_layout,layout_areas, andcursor_posgives a single source of truth for how the composer occupies its rectangle. In particular:
desired_heightandrender_layoutnow agree on how many rows go to text vs. footer vs. padding/margin, withtext_heightclamped to at least 1 and grown to fill any slack in the providedarea.height.cursor_posuses the samelayout_areasas rendering, which matches thecomposer_cursor_aligns_with_text_rowtest and prevents drift between visual and logical cursor positions.- The
WidgetRefimplementation delegates text rendering strictly totextarea_rect, while the padding/footers are handled viaComposerRenderLayout, so the same plan drives both drawing and hit-testing.Overall this is a solid refactor; the structure is clear and the constants make future tweaks straightforward.
Also applies to: 193-213, 215-303, 304-316, 326-331, 1681-1759, 1762-1774
1844-2056: Layout-focused tests and snapshot harness give strong coverage of the new rendering planThe new/updated tests around the composer layout:
composer_cursor_aligns_with_text_rowvalidates thatcursor_poslands on the same row as the rendered prompt glyph›.composer_renders_bottom_padding_row,composer_leaves_transparent_margin_below_padding, andfooter_hint_row_is_separated_from_composercollectively verify:
- top/bottom padding rows share the same background as the input row,
- the row(s) used for footer/margin are blank in terms of glyphs, and
- the margin sits visually below the composer, not merged into it.
snapshot_composer_stateand the various footer-mode snapshot tests now exercise theWidgetRefpath (render_widget_ref) and usefooter_height + footer_spacing + BOTTOM_MARGIN_HEIGHT + 8to size the terminal, giving deterministic snapshots aligned with the new layout.These tests sharply reduce the risk of subtle regressions in cursor placement or footer spacing; they’re a good complement to the refactored layout code.
Also applies to: 2079-2083, 2601-2603, 2628-2634
2490-2522: Image and large-paste placeholder handling remains robust with new layoutThe existing behavior for:
- large pastes (using
[Pasted Content N chars]placeholders),- image attachments (with
[file.png WxH]placeholders),- deleting placeholders (including duplicates and multi-byte text before/after), and
- flushing paste bursts into either real text or placeholders
is all still well-covered by tests like:
handle_paste_large_uses_placeholder_and_replaces_on_submit,test_multiple_pastes_submission,test_placeholder_deletion/test_partial_placeholder_deletion,- and the image-specific tests at the bottom of the file.
Given the layout refactor didn’t need to special-case these behaviors in rendering, it’s good to see this test surface untouched but still passing through the new
WidgetRefpath. No issues here.Also applies to: 2950-3025, 3077-3112, 3149-3198, 3200-3229
codex-rs/tui/src/statusline/mod.rs (13)
1-48: LGTM! Well-organized module structure and imports.The module structure is clean with clear separation between public re-exports and internal palette imports. The sub-module organization (
overlay,palette,skins,state) provides good encapsulation.
50-69: LGTM! Clear constant definitions for visual elements.The Powerline glyphs, progress bar characters, and icon arrays are well-defined. The hash-based icon selection approach provides visual variety without requiring persistent mapping.
70-115: LGTM! Clean trait design with useful helper functions.The
StatusLineRenderertrait withSend + Syncbounds enables flexible renderer injection while maintaining thread safety. The helper functions provide a concise DSL for building styled spans.
117-240: LGTM! Well-structured snapshot types with safe arithmetic.The snapshot hierarchy is clean with proper separation of concerns. Using
saturating_addandsaturating_duration_sinceinRunTimerSnapshot::elapsed_atcorrectly handles edge cases without panicking. The#[allow(dead_code)]annotations suggest planned future use for the unused fields.
242-255: LGTM! Clear duration formatting logic.The three-tier formatting (seconds, minutes+seconds, hours+minutes+seconds) provides appropriate granularity for different durations with consistent zero-padding.
257-340: LGTM! Well-designed enums for progressive degradation.The variant enums and
DegradeOpprovide a clear vocabulary for the degradation system.EnvironmentInclusionwith its factory methods cleanly tracks which environment segments to include.
437-459: LGTM! Sensible default rendering configuration.The initial render model configuration prioritizes essential information (path, model, context bar, git status) while keeping tokens hidden until needed. The environment inclusion is correctly derived from the snapshot data availability.
601-624: LGTM! Robust width calculation with proper overflow handling.Using
checked_subprevents underflow, and the exact-width requirement integrates well with the degradation loop. Thedebug_assertprovides development-time validation without release overhead.
626-897: LGTM! Well-structured Powerline segment rendering.The left/right segment rendering maintains visual symmetry with appropriate chevron bridging. The environment segments in
collect_right_segmentsare ordered logically, and truncation is applied consistently to prevent overflow.
949-986: LGTM! Thoughtful context bar implementation with graceful degradation.The multiple width checks ensure the bar degrades gracefully at small sizes. The color thresholds (green ≤60%, yellow ≤80%, peach ≤92%, red >92%) provide intuitive visual feedback for context usage.
1144-1166: LGTM! Clean token count formatting with suffix optimization.The logic correctly handles negative values and provides human-readable suffixes. The ".0" suffix removal keeps the display compact.
1206-1485: LGTM! Comprehensive test coverage with snapshot testing.The test suite covers formatting edge cases, degradation behavior, and renderer parity. The
sample_snapshot()fixture provides rich test data, and snapshot tests will catch visual regressions effectively.
474-476: The project uses Rust 1.90.0 (specified in rust-toolchain.toml), which is well beyond the 1.76.0 requirement for let chains. The code is compatible with the project's Rust toolchain and no changes are needed.codex-rs/tui/src/statusline/overlay.rs (8)
1-41: LGTM! Clean module structure and type definitions.The imports are well-organized, and
StatusLineLayoutprovides a clear structure for layout regions. TheStatusLineOverlaystruct appropriately encapsulates state, event sender, and working directory.
43-72: LGTM! Clean feature-gated construction with sensible defaults.The layout constants are well-documented, and
RESERVED_ROWSbeing a compile-time constant is efficient. The feature flag check (tui_custom_statusline) provides clean opt-in behavior.
74-97: LGTM! Well-structured initialization and synchronization.The
bootstrapmethod provides a clear initialization sequence, andrefresh_environmentcorrectly handles synchronous environment variable detection.
184-229: LGTM! Robust layout calculation with proper bounds checking.The minimum height check prevents rendering in cramped spaces, and saturating arithmetic ensures no overflow. The status line anchoring to the bottom row is correctly calculated.
241-262: LGTM! Clean status label generation with consistent truncation.The label generators handle edge cases (empty commands, empty server names) and apply consistent 40-character truncation for display uniformity.
264-295: LGTM! Comprehensive environment detection with sensible fallbacks.The AWS profile detection correctly handles both
AWS_PROFILEandAWS_VAULT, and the hostname detection provides system fallback when the environment variable is unset. The test override mechanism for devspace enables controlled testing.
373-385: LGTM! Proper kubeconfig path resolution with sensible trimming.The
KUBECONFIGenvironment variable handling correctly supports multiple colon-separated paths. Thetrim_kube_contextfunction appropriately extracts the cluster name from ARN-style contexts.
402-491: LGTM! Thorough test coverage for layout and rendering.The
overlay_for_testshelper provides clean test setup, and the tests verify critical layout invariants (margin positioning, blank cells). The comprehensive cell style assertions inrender_leaves_blank_margin_rowensure margin transparency.
| eventsource-stream = "0.2.3" | ||
| futures = { version = "0.3", default-features = false } | ||
| http = "1.3.1" | ||
| hostname = "0.4.0" |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM!
The hostname crate addition is properly placed in alphabetical order and enables the hostname detection feature for the status line rendering system.
🌐 Web query:
What is the latest version of the hostname Rust crate?
💡 Result:
The latest version is hostname 0.4.1 (released April 8, 2025). [1][2]
Sources:
[1] lib.rs — hostname crate (release list).
[2] docs.rs — hostname 0.4.1 documentation.
Update hostname dependency to the latest patch version (0.4.1)
The hostname crate has been updated to 0.4.1 as of April 8, 2025. Update the Cargo.toml entry from hostname = "0.4.0" to hostname = "0.4.1" to incorporate any bug fixes or improvements in the newer patch release.
🤖 Prompt for AI Agents
In codex-rs/Cargo.toml at line 124, the hostname dependency is pinned to
"0.4.0"; update that line to "hostname = \"0.4.1\"" to use the latest patch
release, then run cargo update -p hostname (or cargo build) to refresh the
lockfile and verify the project compiles.
| match FileExt::try_lock_exclusive(&history_file) { | ||
| Ok(()) => { | ||
| // While holding the exclusive lock, write the full line. | ||
| history_file.write_all(line.as_bytes())?; | ||
| history_file.flush()?; | ||
| let write_result = history_file | ||
| .write_all(line.as_bytes()) | ||
| .and_then(|_| history_file.flush()); | ||
| let unlock_result = FileExt::unlock(&history_file); | ||
| write_result?; | ||
| unlock_result?; | ||
| return Ok(()); | ||
| } | ||
| Err(std::fs::TryLockError::WouldBlock) => { | ||
| Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { | ||
| std::thread::sleep(RETRY_SLEEP); | ||
| } | ||
| Err(e) => return Err(e.into()), | ||
| Err(err) => return Err(err), | ||
| } |
There was a problem hiding this comment.
Handle unlock failure more carefully after successful write.
If the write succeeds but unlock fails (line 125), the function returns an error while leaving the file locked exclusively. This could block all subsequent operations on the history file until the process exits.
Consider:
- Logging the unlock failure but returning success since the write completed
- Or attempting to recover by retrying the unlock
- At minimum, add a warning log before returning the unlock error so the failure mode is observable
Apply this diff to log unlock failures while preserving write success:
Ok(()) => {
let write_result = history_file
.write_all(line.as_bytes())
.and_then(|_| history_file.flush());
- let unlock_result = FileExt::unlock(&history_file);
write_result?;
- unlock_result?;
+ if let Err(e) = FileExt::unlock(&history_file) {
+ tracing::warn!(error = %e, "failed to unlock history file after write; lock may persist until process exit");
+ }
return Ok(());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| match FileExt::try_lock_exclusive(&history_file) { | |
| Ok(()) => { | |
| // While holding the exclusive lock, write the full line. | |
| history_file.write_all(line.as_bytes())?; | |
| history_file.flush()?; | |
| let write_result = history_file | |
| .write_all(line.as_bytes()) | |
| .and_then(|_| history_file.flush()); | |
| let unlock_result = FileExt::unlock(&history_file); | |
| write_result?; | |
| unlock_result?; | |
| return Ok(()); | |
| } | |
| Err(std::fs::TryLockError::WouldBlock) => { | |
| Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { | |
| std::thread::sleep(RETRY_SLEEP); | |
| } | |
| Err(e) => return Err(e.into()), | |
| Err(err) => return Err(err), | |
| } | |
| match FileExt::try_lock_exclusive(&history_file) { | |
| Ok(()) => { | |
| let write_result = history_file | |
| .write_all(line.as_bytes()) | |
| .and_then(|_| history_file.flush()); | |
| write_result?; | |
| if let Err(e) = FileExt::unlock(&history_file) { | |
| tracing::warn!(error = %e, "failed to unlock history file after write; lock may persist until process exit"); | |
| } | |
| return Ok(()); | |
| } | |
| Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { | |
| std::thread::sleep(RETRY_SLEEP); | |
| } | |
| Err(err) => return Err(err), | |
| } |
| async fn git_status_porcelain(cwd: &Path) -> Option<(bool, Option<i64>, Option<i64>)> { | ||
| let output = Command::new("git") | ||
| .args(["status", "--porcelain=2", "--branch"]) | ||
| .current_dir(cwd) | ||
| .output() | ||
| .await | ||
| .ok()?; | ||
| if !output.status.success() { | ||
| return None; | ||
| } | ||
| let text = String::from_utf8_lossy(&output.stdout); | ||
| let mut dirty = false; | ||
| let mut ahead = None; | ||
| let mut behind = None; | ||
| for line in text.lines() { | ||
| if !line.starts_with('#') { | ||
| dirty = true; | ||
| continue; | ||
| } | ||
| if let Some(rest) = line.strip_prefix("# branch.ab ") { | ||
| let mut parts = rest.split_whitespace(); | ||
| if let Some(ahead_part) = parts.next() { | ||
| ahead = ahead_part | ||
| .strip_prefix('+') | ||
| .and_then(|s| s.parse::<i64>().ok()); | ||
| } | ||
| if let Some(behind_part) = parts.next() { | ||
| behind = behind_part | ||
| .strip_prefix('-') | ||
| .and_then(|s| s.parse::<i64>().ok()); | ||
| } | ||
| } | ||
| } | ||
| Some((dirty, ahead, behind)) | ||
| } |
There was a problem hiding this comment.
Add timeout to prevent indefinite hanging.
The git status command runs without a timeout. If git hangs (e.g., on network filesystems, large repos, or lock contention), this could block the async runtime indefinitely. The codebase already has run_git_command_with_timeout in codex_core::git_info.rs that could be adapted, or use tokio::time::timeout.
+use tokio::time::timeout;
+use std::time::Duration;
+
+const GIT_COMMAND_TIMEOUT: Duration = Duration::from_secs(5);
+
async fn git_status_porcelain(cwd: &Path) -> Option<(bool, Option<i64>, Option<i64>)> {
- let output = Command::new("git")
+ let output = timeout(
+ GIT_COMMAND_TIMEOUT,
+ Command::new("git")
.args(["status", "--porcelain=2", "--branch"])
.current_dir(cwd)
.output()
- .await
- .ok()?;
+ )
+ .await
+ .ok()?
+ .ok()?;
if !output.status.success() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn git_status_porcelain(cwd: &Path) -> Option<(bool, Option<i64>, Option<i64>)> { | |
| let output = Command::new("git") | |
| .args(["status", "--porcelain=2", "--branch"]) | |
| .current_dir(cwd) | |
| .output() | |
| .await | |
| .ok()?; | |
| if !output.status.success() { | |
| return None; | |
| } | |
| let text = String::from_utf8_lossy(&output.stdout); | |
| let mut dirty = false; | |
| let mut ahead = None; | |
| let mut behind = None; | |
| for line in text.lines() { | |
| if !line.starts_with('#') { | |
| dirty = true; | |
| continue; | |
| } | |
| if let Some(rest) = line.strip_prefix("# branch.ab ") { | |
| let mut parts = rest.split_whitespace(); | |
| if let Some(ahead_part) = parts.next() { | |
| ahead = ahead_part | |
| .strip_prefix('+') | |
| .and_then(|s| s.parse::<i64>().ok()); | |
| } | |
| if let Some(behind_part) = parts.next() { | |
| behind = behind_part | |
| .strip_prefix('-') | |
| .and_then(|s| s.parse::<i64>().ok()); | |
| } | |
| } | |
| } | |
| Some((dirty, ahead, behind)) | |
| } | |
| use tokio::time::timeout; | |
| use std::time::Duration; | |
| const GIT_COMMAND_TIMEOUT: Duration = Duration::from_secs(5); | |
| async fn git_status_porcelain(cwd: &Path) -> Option<(bool, Option<i64>, Option<i64>)> { | |
| let output = timeout( | |
| GIT_COMMAND_TIMEOUT, | |
| Command::new("git") | |
| .args(["status", "--porcelain=2", "--branch"]) | |
| .current_dir(cwd) | |
| .output() | |
| ) | |
| .await | |
| .ok()? | |
| .ok()?; | |
| if !output.status.success() { | |
| return None; | |
| } | |
| let text = String::from_utf8_lossy(&output.stdout); | |
| let mut dirty = false; | |
| let mut ahead = None; | |
| let mut behind = None; | |
| for line in text.lines() { | |
| if !line.starts_with('#') { | |
| dirty = true; | |
| continue; | |
| } | |
| if let Some(rest) = line.strip_prefix("# branch.ab ") { | |
| let mut parts = rest.split_whitespace(); | |
| if let Some(ahead_part) = parts.next() { | |
| ahead = ahead_part | |
| .strip_prefix('+') | |
| .and_then(|s| s.parse::<i64>().ok()); | |
| } | |
| if let Some(behind_part) = parts.next() { | |
| behind = behind_part | |
| .strip_prefix('-') | |
| .and_then(|s| s.parse::<i64>().ok()); | |
| } | |
| } | |
| } | |
| Some((dirty, ahead, behind)) | |
| } |
| inherit cargoLock; | ||
| cargoSha256 = cargoVendorSha; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if both cargoLock and cargoSha256 are needed.
When using cargoLock with outputHashes, cargoSha256 is typically unnecessary as the lock file handles dependency pinning. Setting both may be redundant or could cause conflicts depending on the nixpkgs version.
🌐 Web query:
Nix buildRustPackage cargoLock vs cargoSha256
💡 Result:
Short answer
-
cargoSha256 (aka cargoHash/cargoHash SRI) — a single hash of the vendored crate tarball. Use it when all dependencies are crates.io (no git/path deps) and you let buildRustPackage fetch+vendor them. Typical workflow: set cargoSha256 = lib.fakeSha256, build, copy the failure hash. [1][2]
-
cargoLock — provide a Cargo.lock (or its contents) so Nix vendors exactly what's listed and you supply per-crate output hashes in cargoLock.outputHashes. Use this when you have git/path dependencies or need to pin the exact lockfile (e.g. local or git deps), or when you want to avoid re-calculating one global cargoSha256 after every lockfile change. You must list each crate’s hash in outputHashes (or let the build fail with fake hashes and copy them). [1][3]
When to pick which
- Prefer cargoSha256 for simple projects with only crates.io crates (easier: single hash). [1][2]
- Use cargoLock when the lockfile contains git/path deps or you want deterministic vendoring tied exactly to Cargo.lock — required for git deps. Also handy in flakes or when committing Cargo.lock into repo. [1][3]
Practical tips
- To discover correct cargoSha256/cargoHash: use lib.fakeSha256 / lib.fakeHash, run the build, and copy the hash from the error. [1]
- For cargoLock with many git deps, you’ll need to populate cargoLock.outputHashes; you can start with fake hashes and copy actual ones from the failed build output. Tools/docs show examples. [3]
- cargoDepsName can make the vendored tarball name stable across versions (so the cargo hash is invariant). [1]
Sources
- Nixpkgs rust docs (buildRustPackage, cargoSha256, cargoLock). [1]
- Nixpkgs user guide / examples mentioning cargoSha256 and workflow. [2]
- Blog / how-to showing cargoLock with git deps and outputHashes example. [3]
I need more specific information about whether using both cargoLock and cargoSha256 together actually causes issues.
Based on the official Nixpkgs documentation, the original review comment's concern is valid and well-founded.
The cargoLock argument to buildRustPackage can be used "in place of" cargo{Sha256,Hash}—not alongside it. When using cargoLock, it "does not require a Cargo hash" and retrieves all dependencies as fixed-output derivations. The official Nix documentation demonstrates this with a concrete example: the PR migrating bitwarden_rs explicitly commented out the cargoSha256 line when adding cargoLock, showing they are mutually exclusive approaches, not complementary settings.
Remove cargoSha256 = cargoVendorSha; and rely solely on cargoLock with proper outputHashes.
When using cargoLock, the per-crate hashes in outputHashes handle dependency pinning—cargoSha256 becomes redundant and should be omitted entirely. The lock file provides the determinism that cargoSha256 would otherwise provide.
🤖 Prompt for AI Agents
In flake.nix around lines 52-53, remove the mutually-exclusive cargoSha256
assignment: drop "cargoSha256 = cargoVendorSha;" and rely solely on the provided
cargoLock and its outputHashes for dependency pinning; ensure outputHashes
contains the required per-crate hashes (or update outputHashes as needed) so
buildRustPackage uses cargoLock without specifying cargoSha256.
2 participants
External (non-OpenAI) Pull Request Requirements
Before opening this Pull Request, please read the dedicated "Contributing" markdown file or your PR may be closed:
https://github.com/openai/codex/blob/main/docs/contributing.md
If your PR conforms to our contribution guidelines, replace this text with a detailed and high quality description of your changes.
Include a link to a bug report or enhancement request.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.