Skip to content

Commit cfaf9c1

Browse files
authored
refactor: Cut dead code paths (#438)
## Summary - remove the legacy debug-only `/rollout` and `/test-approval` slash commands from the TUI command surface and their dead handlers/tests - trim compatibility-only `login_status` and `auth_manager` fields from the Nori onboarding args so the Nori path only carries live data - delete the orphaned top-level update modules and stale snapshot, and refresh `codex-rs/tui/docs.md` so it documents the Nori update path that actually ships in `nori` ## Why This PR is a dead-code cleanup pass focused on the ACP-backed Nori CLI. It removes compatibility baggage and uncompiled upstream leftovers from `nori-tui`, which keeps the branch net-negative and reduces accidental complexity in the code that matters for the `nori` binary. ## Validation - `RUSTC_WRAPPER= cargo test -p nori-tui --manifest-path codex-rs/Cargo.toml` - `RUSTC_WRAPPER= cargo build --bin nori --manifest-path codex-rs/Cargo.toml` - `RUSTC_WRAPPER= cargo test -p tui-pty-e2e --manifest-path codex-rs/Cargo.toml` - `RUSTC_WRAPPER= just fix -p nori-tui` - `just fmt` - launched `./target/debug/nori --agent elizacp --skip-trust-directory` in an isolated tmux session, submitted `hello`, observed an ElizACP reply, and verified the prompt returned
1 parent 1c9f96f commit cfaf9c1

10 files changed

Lines changed: 47 additions & 777 deletions

File tree

codex-rs/tui/docs.md

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ The auto-worktree startup flow branches on the `AutoWorktree` enum (see `@/codex
3939
| `Ask` | After TUI init, in `run_ratatui_app()` | Sets `pending_worktree_ask = true`, deferred to a TUI popup shown after onboarding but before `App::run()` |
4040
| `Off` | N/A | Skips worktree creation entirely |
4141

42-
The `Ask` popup is implemented by `nori::worktree_ask::run_worktree_ask_popup()`, a standalone mini-app screen (same pattern as `update_prompt.rs`) that runs its own event loop before the main `App`. It presents two options ("Yes, create a worktree" / "No, continue without a worktree") and returns a boolean. If the user confirms, `setup_auto_worktree()` is called and config is reloaded with the new cwd via `load_config_or_exit()`. Ctrl-C, Escape, and the "No" option all skip worktree creation. On failure, the TUI continues with the original cwd.
42+
The `Ask` popup is implemented by `nori::worktree_ask::run_worktree_ask_popup()`, a standalone mini-app screen using the same pre-`App` event-loop pattern as `nori::update_prompt` in release builds. It presents two options ("Yes, create a worktree" / "No, continue without a worktree") and returns a boolean. If the user confirms, `setup_auto_worktree()` is called and config is reloaded with the new cwd via `load_config_or_exit()`. Ctrl-C, Escape, and the "No" option all skip worktree creation. On failure, the TUI continues with the original cwd.
4343

4444
The main event loop in `app/mod.rs` processes:
4545

@@ -386,8 +386,6 @@ The fork context flows through `ChatWidgetInit.fork_context` -> `spawn_agent()`
386386

387387
**Session context injection:** Both `spawn_acp_agent()` and `spawn_acp_agent_resume()` in `chatwidget/agent.rs` set `AcpBackendConfig.session_context` to the contents of `@/codex-rs/tui/session_context.md` (loaded at compile time via `include_str!`). This tells the ACP agent that it is running inside the nori CLI and provides a source-code URL for self-referential questions. The context is prepended (without `SUMMARY_PREFIX` framing) to the first user prompt only and then consumed (see `@/codex-rs/acp/docs.md` for the hook context injection mechanism).
388388

389-
Debug-only commands (not shown in help): `/rollout`, `/test-approval`
390-
391389
The `/logout` command is only available when the `login` feature is enabled. The `/config` command requires the `nori-config` feature.
392390

393391

@@ -843,10 +841,10 @@ The `--dangerously-bypass-approvals-and-sandbox` flag (alias: `--yolo`) works in
843841

844842
**Update Checking:**
845843

846-
The TUI uses Nori-specific update checking via files in `@/codex-rs/tui/src/nori/`:
847-
- `update_action.rs`: Update action handling
848-
- `updates.rs`: Version checking against GitHub releases
849-
- `update_prompt.rs`: User prompting for updates
844+
The TUI uses Nori-specific update checking via the modules in `@/codex-rs/tui/src/nori/`:
845+
- `nori/update_action.rs`: Update action handling
846+
- `nori/updates.rs`: Version checking against GitHub releases
847+
- `nori/update_prompt.rs`: User prompting for updates
850848

851849
**Error Reporting:**
852850

codex-rs/tui/src/chatwidget/key_handling.rs

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -221,54 +221,6 @@ impl ChatWidget {
221221
SlashCommand::Mcp => {
222222
self.open_mcp_servers_popup();
223223
}
224-
SlashCommand::Rollout => {
225-
if let Some(path) = self.rollout_path() {
226-
self.add_info_message(
227-
format!("Current rollout path: {}", path.display()),
228-
None,
229-
);
230-
} else {
231-
self.add_info_message("Rollout path is not available yet.".to_string(), None);
232-
}
233-
}
234-
SlashCommand::TestApproval => {
235-
use codex_core::protocol::EventMsg;
236-
use std::collections::HashMap;
237-
238-
use codex_core::protocol::ApplyPatchApprovalRequestEvent;
239-
use codex_core::protocol::FileChange;
240-
241-
self.app_event_tx.send(AppEvent::CodexEvent(Event {
242-
id: "1".to_string(),
243-
// msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
244-
// call_id: "1".to_string(),
245-
// command: vec!["git".into(), "apply".into()],
246-
// cwd: self.config.cwd.clone(),
247-
// reason: Some("test".to_string()),
248-
// }),
249-
msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent {
250-
call_id: "1".to_string(),
251-
turn_id: "turn-1".to_string(),
252-
changes: HashMap::from([
253-
(
254-
PathBuf::from("/tmp/test.txt"),
255-
FileChange::Add {
256-
content: "test".to_string(),
257-
},
258-
),
259-
(
260-
PathBuf::from("/tmp/test2.txt"),
261-
FileChange::Update {
262-
unified_diff: "+test\n-test2".to_string(),
263-
move_path: None,
264-
},
265-
),
266-
]),
267-
reason: None,
268-
grant_root: Some(PathBuf::from("/tmp")),
269-
}),
270-
}));
271-
}
272224
SlashCommand::SwitchSkillset => {
273225
self.handle_switch_skillset_command();
274226
}

codex-rs/tui/src/chatwidget/tests/part2.rs

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -31,42 +31,6 @@ fn slash_undo_sends_op() {
3131
}
3232
}
3333

34-
#[test]
35-
fn slash_rollout_displays_current_path() {
36-
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual();
37-
let rollout_path = PathBuf::from("/tmp/codex-test-rollout.jsonl");
38-
chat.current_rollout_path = Some(rollout_path.clone());
39-
40-
chat.dispatch_command(SlashCommand::Rollout);
41-
42-
let cells = drain_insert_history(&mut rx);
43-
assert_eq!(cells.len(), 1, "expected info message for rollout path");
44-
let rendered = lines_to_single_string(&cells[0]);
45-
assert!(
46-
rendered.contains(&rollout_path.display().to_string()),
47-
"expected rollout path to be shown: {rendered}"
48-
);
49-
}
50-
51-
#[test]
52-
fn slash_rollout_handles_missing_path() {
53-
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual();
54-
55-
chat.dispatch_command(SlashCommand::Rollout);
56-
57-
let cells = drain_insert_history(&mut rx);
58-
assert_eq!(
59-
cells.len(),
60-
1,
61-
"expected info message explaining missing path"
62-
);
63-
let rendered = lines_to_single_string(&cells[0]);
64-
assert!(
65-
rendered.contains("not available"),
66-
"expected missing rollout path message: {rendered}"
67-
);
68-
}
69-
7034
#[test]
7135
fn slash_first_prompt_shows_initial_prompt() {
7236
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual();

codex-rs/tui/src/lib.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -445,8 +445,6 @@ async fn run_ratatui_app(
445445
show_trust_screen: should_show_trust_screen,
446446
skip_welcome: cli.skip_welcome,
447447
skip_trust_directory: cli.skip_trust_directory,
448-
login_status,
449-
auth_manager: auth_manager.clone(),
450448
config: initial_config.clone(),
451449
},
452450
&mut tui,

codex-rs/tui/src/nori/onboarding/onboarding_screen.rs

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,6 @@
77
//! The flow is designed to be used instead of the default Codex onboarding
88
//! screen when building with Nori branding.
99
10-
use std::path::PathBuf;
11-
use std::sync::Arc;
12-
13-
use codex_core::AuthManager;
1410
use codex_core::config::Config;
1511
use codex_core::git_info::get_git_repo_root;
1612
use color_eyre::eyre::Result;
@@ -23,8 +19,8 @@ use ratatui::prelude::Widget;
2319
use ratatui::style::Color;
2420
use ratatui::widgets::Clear;
2521
use ratatui::widgets::WidgetRef;
22+
use std::path::PathBuf;
2623

27-
use crate::LoginStatus;
2824
use crate::onboarding::TrustDirectorySelection;
2925
use crate::onboarding::onboarding_screen::KeyboardHandler;
3026
use crate::onboarding::onboarding_screen::StepState;
@@ -56,12 +52,6 @@ pub(crate) struct NoriOnboardingScreenArgs {
5652
pub skip_welcome: bool,
5753
/// Whether to skip the trust directory prompt (--skip-trust-directory flag).
5854
pub skip_trust_directory: bool,
59-
/// Current login status (unused in Nori but kept for API compatibility).
60-
#[allow(dead_code)]
61-
pub login_status: LoginStatus,
62-
/// Auth manager (unused in Nori but kept for API compatibility).
63-
#[allow(dead_code)]
64-
pub auth_manager: Arc<AuthManager>,
6555
/// Application configuration.
6656
pub config: Config,
6757
}
@@ -90,8 +80,6 @@ impl NoriOnboardingScreen {
9080
show_trust_screen,
9181
skip_welcome,
9282
skip_trust_directory,
93-
login_status: _,
94-
auth_manager: _,
9583
config,
9684
} = args;
9785

@@ -375,11 +363,30 @@ pub(crate) async fn run_nori_onboarding_app(
375363
#[cfg(test)]
376364
mod tests {
377365
use super::*;
366+
use codex_core::config::Config;
367+
use codex_core::config::ConfigOverrides;
368+
use codex_core::config::ConfigToml;
378369

379370
#[test]
380371
fn nori_step_implements_required_traits() {
381372
// This test verifies the NoriStep enum implements all required traits.
382373
// The compilation of this test is the actual verification.
383374
let _welcome = NoriStep::Welcome(NoriWelcomeWidget::new());
384375
}
376+
377+
#[test]
378+
fn nori_onboarding_args_only_require_live_fields() -> std::io::Result<()> {
379+
let _args = NoriOnboardingScreenArgs {
380+
show_trust_screen: true,
381+
skip_welcome: false,
382+
skip_trust_directory: false,
383+
config: Config::load_from_base_config_with_overrides(
384+
ConfigToml::default(),
385+
ConfigOverrides::default(),
386+
std::env::temp_dir(),
387+
)?,
388+
};
389+
390+
Ok(())
391+
}
385392
}

codex-rs/tui/src/slash_command.rs

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,6 @@ pub enum SlashCommand {
3333
Logout,
3434
Quit,
3535
Exit,
36-
Rollout,
37-
TestApproval,
3836
SwitchSkillset,
3937
Fork,
4038
}
@@ -63,8 +61,6 @@ impl SlashCommand {
6361
SlashCommand::Mcp => "manage MCP server connections",
6462
SlashCommand::Login => "log in to the current agent",
6563
SlashCommand::Logout => "show logout instructions",
66-
SlashCommand::Rollout => "print the rollout file path",
67-
SlashCommand::TestApproval => "test approval request",
6864
SlashCommand::SwitchSkillset => "switch between available skillsets",
6965
SlashCommand::Fork => "rewind conversation to a previous message",
7066
}
@@ -102,14 +98,11 @@ impl SlashCommand {
10298
| SlashCommand::FirstPrompt
10399
| SlashCommand::Quit
104100
| SlashCommand::Exit => true,
105-
SlashCommand::Rollout => true,
106-
SlashCommand::TestApproval => true,
107101
}
108102
}
109103

110104
fn is_visible(self) -> bool {
111105
match self {
112-
SlashCommand::Rollout | SlashCommand::TestApproval => cfg!(debug_assertions),
113106
#[cfg(not(feature = "login"))]
114107
SlashCommand::Logout => false,
115108
_ => true,
@@ -290,6 +283,28 @@ mod tests {
290283
);
291284
}
292285

286+
#[test]
287+
fn legacy_debug_commands_are_not_exposed() {
288+
let commands = built_in_slash_commands();
289+
290+
assert!(
291+
commands.iter().all(|(name, _)| *name != "rollout"),
292+
"/rollout should not be visible in commands list"
293+
);
294+
assert!(
295+
commands.iter().all(|(name, _)| *name != "test-approval"),
296+
"/test-approval should not be visible in commands list"
297+
);
298+
assert!(
299+
"rollout".parse::<SlashCommand>().is_err(),
300+
"/rollout should not parse as a slash command"
301+
);
302+
assert!(
303+
"test-approval".parse::<SlashCommand>().is_err(),
304+
"/test-approval should not parse as a slash command"
305+
);
306+
}
307+
293308
#[test]
294309
fn browse_parses_from_string() {
295310
let cmd: SlashCommand = "browse".parse().expect("/browse should parse from string");

codex-rs/tui/src/snapshots/nori_tui__update_prompt__tests__update_prompt_modal.snap

Lines changed: 0 additions & 13 deletions
This file was deleted.

codex-rs/tui/src/update_action.rs

Lines changed: 0 additions & 101 deletions
This file was deleted.

0 commit comments

Comments
 (0)