fix(neo-tui): surface every Bug 3 silent-failure path missed by PR #14#15
Conversation
…ept opencode/ id prefix Oracle round-3 review of merged PR #14 (commit 45dbb57) caught four silent regressions that all violate the original Bug 3 contract ('if there is an error, say so'): 1. `Inbound::Response { success: false }` was matched to `{}` and dropped. Every backend command rejection - submit, cycle-model, abort, get-available-models - vanished without UI feedback. Now surfaces as a Role::Error chat bubble + Status::Error footer with the original `response.error` string when present, or a command-name fallback when the backend omitted the message. 2. `Inbound::ParseError { line, source }` only emitted a `tracing::warn!` line, which is invisible to a user running `senpi --neo` in a real terminal. The previous regression test `app_inbound_parse_error_does_not_disrupt_ui` actively locked the bad behavior in. Renamed to `app_inbound_parse_error_surfaces_to_chat_and_footer` and inverted the assertions; `apply_inbound` now pushes a Role::Error bubble with a truncated frame preview + flips the footer to 'protocol error' while still keeping the tracing line for log aggregation. 3. `senpi-neo-tui --theme opencode/dracula` (the exact form the README documents) failed with `reading theme json opencode/dracula: No such file or directory`. Root cause: `looks_like_theme_path` used `value.contains('/')` as the path heuristic, which ate every namespaced bundled id. Replaced the heuristic with an explicit path-indicator list (`Path::is_file` || starts with `~/`, `./`, `../`, or `/`) and added a registry-side `strip_prefix("opencode/")` so both `dracula` and `opencode/dracula` resolve to the same theme. 4. `Alt+T` was bound to `neo.theme.picker` in the default keymap and advertised in the README + help overlay, but the dispatcher had no match arm for it, so the chord silently fell through to the `Consumed` catch-all and the overlay never opened. Added a `neo.theme.picker` arm that builds a `ThemePickerOverlay` seeded with the active theme's name and returns the new `AppAction::OpenThemePicker` variant. TDD: each fix is locked by a new failing-then-passing test in `tests/app_loop.rs` (4 added covering Response success+failure shapes, ParseError surfacing, and Alt+T overlay open) and `tests/theme.rs` (`opencode_themes_are_resolvable_by_both_flat_id_and_opencode_prefix`). Total: 305 Rust tests passing (was 298), 0 failing, 1 ignored. `cargo clippy --all-targets -- -D warnings` and `cargo fmt -- --check` green, `npm run check` clean, 9 TS regression tests still pass. Refs #14
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/neo-tui/src/main.rs">
<violation number="1" location="packages/neo-tui/src/main.rs:71">
P2: Absolute path detection is Unix-specific; Windows absolute paths can be misclassified as theme IDs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| || value.starts_with("~/") | ||
| || value.starts_with("./") | ||
| || value.starts_with("../") | ||
| || value.starts_with('/') |
There was a problem hiding this comment.
P2: Absolute path detection is Unix-specific; Windows absolute paths can be misclassified as theme IDs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/neo-tui/src/main.rs, line 71:
<comment>Absolute path detection is Unix-specific; Windows absolute paths can be misclassified as theme IDs.</comment>
<file context>
@@ -54,6 +54,23 @@ struct Cli {
+ || value.starts_with("~/")
+ || value.starts_with("./")
+ || value.starts_with("../")
+ || value.starts_with('/')
+}
+
</file context>
| || value.starts_with('/') | |
| || Path::new(value).is_absolute() |
Oracle round-4 review of PR #15 (`cbcc9331`) found two more silent no-op paths in the picker plumbing: 1. `Ctrl+L` returned `AppAction::OpenModelPicker` and fired `Command::GetAvailableModels` to the backend, but never actually set `self.overlay = Some(Overlay::ModelPicker(_))`, so the advertised model picker just never appeared. Now opens the overlay immediately with the bundled `MODELS` constant; the backend response can refresh it later but the user sees a usable list right away. 2. The theme picker overlay emits `OverlayResult::Selected("neo.theme.set:<id>")` when the user confirms a row, but `execute_action` had no match arm for that prefix - so the dispatcher silently fell into the catch-all `Consumed` no-op. The overlay closed, no theme changed, no error was shown. Classic Bug-3 silent failure. Added an `apply_theme_selection` helper (kept `execute_action` under the 100-line clippy threshold) that calls `theme::load_by_id`, swaps `self.theme` on success, or pushes a `Role::Error` chat bubble plus footer `Status::Error` / `theme load failed` on failure (e.g. typo, stale config). Either way the user sees the result. Tests added (`packages/neo-tui/tests/app_loop.rs`): - `ctrl_l_actually_opens_the_model_picker_overlay` - locks the overlay-open contract. - `theme_picker_selection_applies_the_chosen_theme` - drives the `neo.theme.set:dracula` action and verifies the resolved `Token::Background` color flips to the dracula registry entry (display name comparison would be brittle - JSON `name` is "Dracula", registry id is `dracula`). - `theme_picker_selection_with_unknown_id_pushes_a_chat_error` - locks the no-silent-fail contract for unknown registry ids. Plus a new `#[doc(hidden)] pub fn execute_action_for_tests` lets integration tests drive a specific action id without re-implementing the keymap dispatch chain. Used only by the two new theme-set tests. Test count is now 308 passing (was 305 in #15), 0 failing, 1 ignored. `cargo clippy --package senpi-neo-tui --all-targets -- -D warnings` and `cargo fmt --package senpi-neo-tui -- --check` both green. Refs #14
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/neo-tui/src/main.rs">
<violation number="1" location="packages/neo-tui/src/main.rs:71">
P2: Absolute path detection is Unix-specific; Windows absolute paths can be misclassified as theme IDs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…agged Oracle round-5 audit of PR #15 found four more leak vectors of the user's original Bug 3 contract ("if there is an error, say so"): 1. `Inbound::Response { success: false }` and `Inbound::ParseError` were already wired in the earlier rounds, but the model picker (`Selected("neo.model.set:<id>")`) silently consumed the selection. Picking a model closed the overlay with no visible change and no backend traffic. Added `apply_model_selection` that pushes a Role::System chat note pointing the user at legacy senpi for runtime model switching until the SetModel/provider plumbing lands. 2. The slash menu, command palette, and /hotkeys overlay all advertised actions the neo TUI never wired - `app.session.*`, `app.tree.*`, `app.tree.filter.*`, `app.models.*`, `app.editor.external`, `app.message.followUp`/`dequeue`, `app.clipboard.pasteImage`, `neo.sidebar.toggle`, `neo.compact`, `neo.toggle_animations`. Selecting any of them was a silent no-op. Added `ADVERTISED_BUT_UNIMPLEMENTED_ACTIONS` set + a `note_unimplemented_action` helper that pushes a one-line "not yet wired in --neo" chat-system note. The chord is visibly accounted for. 3. `maybe_spawn_backend()` swallowed `RpcClient::spawn` failures with `.ok()`. A misconfigured `SENPI_NEO_BACKEND_BIN` booted as if no backend were attached - identical to demo mode - and the user only saw a generic "No backend process is connected." message after their first command, with the underlying `ClientError` text lost. Now returns `Result<Option<RpcClient>, String>`, and `drive()` immediately feeds the failure to `apply_inbound(Inbound::Error { ... })` before the first render so the user sees the actual spawn error in the chat + footer. 4. `spawn_writer` and `spawn_stdout_reader` exited silently on I/O errors (`write_all`, `flush`, `lines.next_line` returning `Err`). The user only saw the eventual child exit much later - the actual transport failure point was lost. Both now own a clone of `inbound_tx` and send `Inbound::Error` with the io error text before exiting. `spawn_writer` also now reports serialization failures via the same channel instead of continuing silently. Tests added (TDD red-then-green): - `model_picker_selection_pushes_visible_feedback` - locks the chat-note contract for `neo.model.set:<id>`. - `unimplemented_slash_command_visibly_notifies_user` - locks the not-yet-wired note for `app.session.new` (representative of the whole `ADVERTISED_BUT_UNIMPLEMENTED_ACTIONS` set). Test count: 310 passing (was 308 after round 4), 0 failing, 1 ignored. `cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` clean. Refactored two helpers (`apply_theme_selection`, `apply_model_selection`, `note_unimplemented_action`) out of `execute_action` to keep it under the clippy 100-line threshold. Refs #14, #15
…ound 6 Oracle's sixth review of PR #15 head (`35bbdece`) confirmed the spawn / writer / stdout-reader / picker / unimplemented-action fixes from round 5 but flagged six additional places where the "if there's an error, say so" contract still leaked through: 1. `app.editor.external` (Ctrl+G) returned `AppAction::ExternalEditor`, but `action_to_command` maps that variant to `None` and the run loop has no other handler - the keystroke produced zero visible effect. The dispatcher now also pushes a chat-system note explaining the in-buffer external editor is not yet wired in `senpi --neo`, while still returning the typed variant so the existing parity test continues to lock the dispatch contract. Extracted as `App::apply_external_editor_action` to keep `execute_action` under clippy's 100-line ceiling. 2. `app.suspend` (Ctrl+Z) was advertised in `assets/keymaps/default.json:45` and exposed via the slash menu / command palette, but missing from `ADVERTISED_BUT_UNIMPLEMENTED_ACTIONS`. Falling through to the catch-all `_ => AppAction::Consumed(...)` made it a silent no-op. Added to the list so the dispatcher now routes it through `note_unimplemented_action` and surfaces a "not yet wired" chat note. 3. `RpcEvent::MessageEnd { message }` ignored the `message.errorMessage` field. When the assistant turn fails, `agent-loop.ts::buildErrorAssistantMessage` attaches the provider error string to the final assistant message and ships it through `message_end`. The previous arm only dropped the empty bubble and flipped footer idle, silently discarding the error. Extracted `apply_message_end` helper that pops the empty placeholder bubble (existing behavior), then checks for `errorMessage` - if present, pushes `Role::Error` with footer `Status::Error` (`assistant error` label); otherwise falls back to the clean idle path. 4. `RpcEvent::CompactionEnd { aborted, error_message, will_retry, .. }` failure variants were dropped by the catch-all `_ => {}`. Compaction failures typically mean the summarization LLM rejected the request - the user has to know so they can manually compact, fork, or accept the larger context. Added a guarded match arm `if aborted || error_message.is_some()` that calls a new `apply_compaction_failure` helper. Successful compactions stay silent (would flood chat on every routine compaction). 5. `RpcEvent::AutoRetryEnd { success: false, attempt, final_error }` exhausted retries used to silently flip back to idle. Added a destructure-guard match arm `success: false` that calls `apply_auto_retry_failure`, which pushes a `Role::Error` message naming the attempt count and final error and flips the footer to `retry exhausted`. Successful retries stay silent so transient recoveries do not flood chat. 6. `crossterm::EventStream::next()` returning `Some(Err(_))` (terminal input pipe I/O error) and `None` (stream exhausted / TTY closed) were both silently consumed by `_ => {}` in `drive`'s match arm. The user got a silent freeze with no chat / footer indication of why their keystrokes stopped landing. Extracted `handle_terminal_event` helper plus a `TerminalEventOutcome` enum (Continue / Quit / Disconnected / BackendChannelClosed) so the drive loop can surface input-pipe errors as `Inbound::Error` and treat stream exhaustion as `Inbound::Disconnected` before breaking. The helper extraction also keeps `drive` under clippy's per-fn line ceiling. 7. `spawn_stderr_reader` used `while let Ok(Some(line))`, silently dropping any `Err(_)` from `lines.next_line()`. The child watcher eventually surfaces the exit, but until then the user had no idea why the diagnostic stream went quiet. The function now takes an `inbound_tx` clone and emits `Inbound::Error { stderr_tail: "backend stderr read failed: ..." }` immediately on transport read failure, matching the round-5 `spawn_writer` / `spawn_stdout_reader` shape. Refactor: extracted `apply_message_update_delta`, `apply_message_end`, `apply_compaction_failure`, `apply_auto_retry_failure`, `apply_external_editor_action`, `handle_terminal_event` helpers + the `TerminalEventOutcome` enum to keep `apply_event`, `execute_action`, and `drive` under clippy's `too_many_lines` ceiling and to express the new arms as typed unit-test surfaces. Tests added (TDD red-then-green): six regression tests in `tests/app_loop.rs` for Defects 1-5, plus two inverse-contract tests (`apply_inbound_compaction_end_success_does_not_disturb_chat`, `apply_inbound_auto_retry_end_success_does_not_disturb_chat`) that lock the "no chat noise on success" guarantee against future over-firing. Defects 6 and 7 live in spawned tokio tasks driven by external transports and are verified by code review + the helper extraction (`handle_terminal_event` returns a typed outcome enum, so the drive loop's match exhaustively names every failure path, and the stderr-reader refactor mirrors the round-5 stdout-reader / writer patterns). Existing tests untouched. Total Rust test count is now 318 passing (was 310 after round 5), 0 failing, 1 ignored. `cargo clippy --package senpi-neo-tui --all-targets -- -D warnings`, `cargo fmt --package senpi-neo-tui -- --check`, and `npm run check` (897 files) all green. Refs #14, #15.
Oracle's round-7 review of HEAD `f789df30` confirmed every round-6
fix and the three quality gates, but flagged one last Bug-3 silent
path: `tui.select.{up,down,pageUp,pageDown,confirm,cancel}` are
bound in the bundled keymap and exposed by the command palette, but
they only have a useful effect while an overlay is open. The
compositor's `synthesise_select_event` translates them to a
synthetic `KeyEvent` that the active overlay's raw handler consumes;
with no overlay open the dispatcher fell into the catch-all silent
`Consumed` arm and produced zero feedback. Selecting
`tui.select.up` from the palette closed it with nothing visible.
Bug 3 contract: every advertised chord that lands must produce
visible feedback. `note_unimplemented_action` would be misleading
here (these actions ARE wired - just only inside an overlay), so
the fix introduces a separate path.
Added:
- `OVERLAY_SCOPED_SELECT_ACTIONS` module-level constant with the
six `tui.select.*` ids.
- `is_overlay_scoped_select_action(id) -> bool` predicate.
- `App::note_overlay_only_action(action_id) -> AppAction` that
pushes a `Role::System` chat note explaining that the action
only takes effect while an overlay (slash menu, command palette,
model / theme picker, help) is open.
- Dispatcher arm in `execute_action` ordered BEFORE the existing
`is_advertised_unimplemented_action` predicate so the
overlay-scoping message wins over the generic "not yet wired"
message.
Regression test: `tui_select_action_outside_overlay_visibly_notifies_user`
in `tests/app_loop.rs` (asserts `AppAction::Consumed` plus a
`Role::System` chat message that names the action AND mentions
"overlay").
Total Rust test count is now 319 passing (was 318 after round 6),
0 failing, 1 ignored. `cargo clippy --package senpi-neo-tui --all-targets -- -D warnings`,
`cargo fmt --package senpi-neo-tui -- --check`, and `npm run check`
(897 files) all green.
Refs #14, #15
Oracle's eighth review of HEAD `8ac34409` confirmed every prior fix (rounds 1-7) and the three quality gates, then surfaced two more silent paths: 1. `neo.slash.open` (`/`) is bound in the bundled keymap and exposed by the command palette. The raw key path in `handle_key` opens the slash overlay only when the user types `/` with an empty Input-focus buffer (so mid-prompt `/` inserts literally). When the action was dispatched THROUGH `execute_action` (the palette path), there was no arm and it fell into the catch-all silent consume. Selecting `/help` from the palette closed the palette with no slash overlay and no chat note. New `App::open_slash_overlay` helper opens the overlay unconditionally - the palette explicitly picked the action, so the buffer-empty precondition no longer applies. Regression: `neo_slash_open_dispatched_from_palette_opens_slash_overlay`. 2. `tui.input.tab` (`tab`) is bound in the keymap and surfaced by the palette. `try_autocomplete_action` consumes it ONLY when an autocomplete popup is visible; outside that context it returned `None` and the dispatcher fell into the catch-all silent consume. New `App::note_autocomplete_only_action` helper pushes a chat-system note: "`tui.input.tab` only takes effect while an autocomplete popup is showing. Type `@` for path completion or `/` for slash commands first." Mirrors the `note_overlay_only_action` shape from round 7. Regression: `tui_input_tab_outside_autocomplete_visibly_notifies_user`. Cleanup (per Oracle's audit recommendation): removed the dead `"app.message.followUp"` entry from `ADVERTISED_BUT_UNIMPLEMENTED_ACTIONS`. The explicit `execute_action` arm at line 645 has always shadowed it - the list entry was audit-misleading. Replaced with a comment explaining the precedence so future auditors do not re-add it. Refactor (under clippy's 100-line ceiling): extracted `App::open_slash_overlay`, `App::apply_follow_up_action`, `App::apply_submit_action`, and `App::note_autocomplete_only_action` from `execute_action`. 1:1 with their inline equivalents, no behavior change. Oracle's exhaustive walk of `assets/keymaps/default.json` confirmed the only remaining class-(E) defect (raw-handler-only path) was `neo.slash.open`. No class-(F) orphans found. Every advertised default keybinding now routes to one of: real behavior, `note_unimplemented_action`, `note_overlay_only_action`, `note_autocomplete_only_action`, or an `apply_*` / `open_*` overlay/message helper. Total Rust test count is now 321 passing (was 319 after round 7), 0 failing, 1 ignored. `cargo clippy --package senpi-neo-tui --all-targets -- -D warnings`, `cargo fmt --package senpi-neo-tui -- --check`, and `npm run check` (897 files) all green. Refs #14, #15.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/neo-tui/changes.md">
<violation number="1" location="packages/neo-tui/changes.md:97">
P3: This round-8 changelog entry is duplicated, which creates redundant/confusing release notes for the same fixes.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| Total Rust test count is now 321 passing (was 319 after round 7). `cargo fmt --check`, `cargo clippy --all-targets -- -D warnings`, and `npm run check` (897 files) all green. | ||
|
|
||
| ## 2026-05-19 — Oracle round 8: `neo.slash.open` + `tui.input.tab` outside their contexts |
There was a problem hiding this comment.
P3: This round-8 changelog entry is duplicated, which creates redundant/confusing release notes for the same fixes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/neo-tui/changes.md, line 97:
<comment>This round-8 changelog entry is duplicated, which creates redundant/confusing release notes for the same fixes.</comment>
<file context>
@@ -74,6 +74,38 @@ Regression: `tui_select_action_outside_overlay_visibly_notifies_user`.
+
+Total Rust test count is now 321 passing (was 319 after round 7). `cargo fmt --check`, `cargo clippy --all-targets -- -D warnings`, and `npm run check` (897 files) all green.
+
+## 2026-05-19 — Oracle round 8: `neo.slash.open` + `tui.input.tab` outside their contexts
+
+Oracle's eighth review of the round-7 commit (`8ac34409`) confirmed the `tui.select.*` fix and all three gates, then flagged the next-most-similar leak: `neo.slash.open` was only handled in the raw key path (gated on Input focus + empty buffer for the mid-prompt `/` literal-insert behavior), so dispatching it through `execute_action` (i.e. via the command palette) fell into the catch-all silent consume. A follow-up classification audit also surfaced `tui.input.tab`, which `try_autocomplete_action` only consumes when an autocomplete popup is open — outside that context it landed in the catch-all too.
</file context>
Oracle's ninth review of HEAD `1e08e6b7` flagged the last
remaining Bug-3 silent path: `app.exit` with a non-empty input
buffer returned `AppAction::Consumed("tui.editor.deleteCharForward")`
WITHOUT actually performing a delete-char-forward. The label
string was emitted but the buffer never mutated. Selecting
`/quit` from the slash menu / command palette while drafting a
message silently closed the menu with no quit, no delete, no
chat feedback - a textbook Bug 3 violation.
The old branch was trying to mimic legacy senpi's
`Ctrl+D-on-non-empty-buffer = delete-char-forward` semantics,
but the implementation never actually performed the delete.
Tested behavior was empty-buffer Ctrl+D = Quit only; the
non-empty branch was unreached in tests and silently broken.
Fix: `app.exit` now always returns `AppAction::Quit`. The user
explicitly invoked exit - whether via Ctrl+D, /quit from the
slash menu, or selecting "app.exit" from the palette - and we
respect that. Users wanting the legacy keystroke semantic
(Ctrl+D mid-text deletes a character) can rebind Ctrl+D to
`tui.editor.deleteCharForward` in their keymap; that arm
already exists with the correct delete behavior.
Regression: `app_exit_dispatched_from_palette_quits_even_with_nonempty_buffer`
in `tests/app_loop.rs` drives `execute_action_for_tests("app.exit")`
with a populated input buffer and asserts `AppAction::Quit`.
The existing `ctrl_d_with_empty_input_resolves_to_app_exit`
continues to pass.
Existing tests untouched. Total Rust test count is now 322
passing (was 321 after round 8), 0 failing, 1 ignored.
`cargo clippy --package senpi-neo-tui --all-targets -- -D warnings`,
`cargo fmt --package senpi-neo-tui -- --check`, and
`npm run check` (897 files) all green.
Refs #14, #15
…gged
1. `app.thinking.toggle` (Ctrl+T) and `app.tools.expand` (Ctrl+O)
flipped `self.thinking_visible` / `self.tools_expanded` but
no render path consumed those fields, so the chord produced
zero visible effect. New `apply_thinking_visibility_toggle`
/ `apply_tools_expanded_toggle` helpers preserve the field
mutation (so wiring renderer hookups later is a one-line
change) and push a chat-system "not yet wired to chat
rendering" note so the chord visibly lands now.
2. Successful `cycle_model` / `set_model` / `cycle_thinking_level`
responses were silently dropped by `apply_inbound` (only the
`success: false` branch was wired). Pressing Ctrl+P or
Shift+Tab fired the RPC, the backend changed the model /
level, but the user saw no header / footer / chat update.
New `apply_response` extracted helper routes successful
responses to `apply_model_change_response` and
`apply_thinking_change_response`. Both update header +
footer AND push a chat note. Null data ("no other model
configured") now pushes an explanatory note so the chord
still produces visible feedback.
3. `app.model.cycleBackward` (Shift+Ctrl+P) silently cycled
FORWARD because `action_to_command` mapped both directions
to the same forward-only `Command::CycleModel` (the wire
protocol is next-only today). Now routes through
`note_unimplemented_action` so the user sees an explicit
"not yet wired" note instead of getting the wrong model.
Refactor: simplified `AppAction::CycleModel { forward: bool }`
to `AppAction::CycleModel` since the discriminator was always
discarded by `action_to_command` and only `forward: true` is
reachable. Backward cycling lands at `note_unimplemented_action`
in `execute_action` before producing the variant.
Test count: 322 -> 329 passing. Seven new tests cover the three
defects. Four existing tests updated to assert the new behavior.
One test replaced (action_to_command_maps_cycle_model_regardless_of_direction
-> action_to_command_maps_cycle_model_to_cycle_model_command).
`cargo fmt --check`, `cargo clippy --all-targets -- -D warnings`,
`cargo test -j 1`, and `npm run check` (897 files) all green.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/neo-tui/changes.md">
<violation number="1" location="packages/neo-tui/changes.md:157">
P3: The test accounting is inconsistent: this line says seven new tests but enumerates eight (and one is also listed as an existing updated test).</violation>
</file>
<file name="packages/neo-tui/src/app/mod.rs">
<violation number="1" location="packages/neo-tui/src/app/mod.rs:1091">
P2: `cycle_model` success handling updates model text but drops `thinkingLevel`, so thinking indicators can become stale after model changes.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| self.footer.model.clone_from(&display); | ||
| self.chat.push_system(format!("Model: {display}")); |
There was a problem hiding this comment.
P2: cycle_model success handling updates model text but drops thinkingLevel, so thinking indicators can become stale after model changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/neo-tui/src/app/mod.rs, line 1091:
<comment>`cycle_model` success handling updates model text but drops `thinkingLevel`, so thinking indicators can become stale after model changes.</comment>
<file context>
@@ -1004,6 +1026,92 @@ impl App {
+ let provider = model_obj.get("provider").and_then(serde_json::Value::as_str);
+ let display = provider.map_or_else(|| name.to_owned(), |provider| format!("{provider}/{name}"));
+ self.header.model.clone_from(&display);
+ self.footer.model.clone_from(&display);
+ self.chat.push_system(format!("Model: {display}"));
+ }
</file context>
| self.footer.model.clone_from(&display); | |
| self.chat.push_system(format!("Model: {display}")); | |
| self.footer.model.clone_from(&display); | |
| if let Some(level) = data.get("thinkingLevel").and_then(serde_json::Value::as_str) { | |
| self.header.thinking_level = Some(level.to_owned()); | |
| self.footer.thinking = Some(level.to_owned()); | |
| } | |
| self.chat.push_system(format!("Model: {display}")); |
|
|
||
| Refactor: simplified `AppAction::CycleModel { forward: bool }` → `AppAction::CycleModel` since the discriminator was always discarded by `action_to_command` and only `forward: true` is now reachable. Backward cycling never enters the variant; it lands at `note_unimplemented_action` in `execute_action`. | ||
|
|
||
| Total Rust test count is now 329 passing (was 322 after round 9); seven new tests cover the three defects (`ctrl_t_app_thinking_toggle_visibly_notifies_user`, `ctrl_o_app_tools_expand_visibly_notifies_user`, `shift_ctrl_p_app_model_cycle_backward_visibly_notifies_user`, `apply_inbound_cycle_model_success_response_updates_displays`, `apply_inbound_cycle_model_success_response_with_null_data_pushes_note`, `apply_inbound_set_model_success_response_updates_displays`, `apply_inbound_cycle_thinking_level_success_response_updates_displays`, `apply_inbound_cycle_thinking_level_success_response_with_null_data_pushes_note`). Four existing tests updated to assert the new behavior (`ctrl_p_dispatches_cycle_model`, `shift_ctrl_p_app_model_cycle_backward_visibly_notifies_user`, `shift_ctrl_p_does_not_open_palette_after_rebind`, `ctrl_shift_p_no_longer_opens_palette_after_rebind`). One existing test replaced (`action_to_command_maps_cycle_model_regardless_of_direction` → `action_to_command_maps_cycle_model_to_cycle_model_command`). `cargo fmt --check`, `cargo clippy --package senpi-neo-tui --all-targets -- -D warnings`, and `npm run check` (897 files) all green. |
There was a problem hiding this comment.
P3: The test accounting is inconsistent: this line says seven new tests but enumerates eight (and one is also listed as an existing updated test).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/neo-tui/changes.md, line 157:
<comment>The test accounting is inconsistent: this line says seven new tests but enumerates eight (and one is also listed as an existing updated test).</comment>
<file context>
@@ -143,3 +143,15 @@ Refactor: extracted `apply_message_update_delta`, `apply_compaction_failure`, `a
+
+Refactor: simplified `AppAction::CycleModel { forward: bool }` → `AppAction::CycleModel` since the discriminator was always discarded by `action_to_command` and only `forward: true` is now reachable. Backward cycling never enters the variant; it lands at `note_unimplemented_action` in `execute_action`.
+
+Total Rust test count is now 329 passing (was 322 after round 9); seven new tests cover the three defects (`ctrl_t_app_thinking_toggle_visibly_notifies_user`, `ctrl_o_app_tools_expand_visibly_notifies_user`, `shift_ctrl_p_app_model_cycle_backward_visibly_notifies_user`, `apply_inbound_cycle_model_success_response_updates_displays`, `apply_inbound_cycle_model_success_response_with_null_data_pushes_note`, `apply_inbound_set_model_success_response_updates_displays`, `apply_inbound_cycle_thinking_level_success_response_updates_displays`, `apply_inbound_cycle_thinking_level_success_response_with_null_data_pushes_note`). Four existing tests updated to assert the new behavior (`ctrl_p_dispatches_cycle_model`, `shift_ctrl_p_app_model_cycle_backward_visibly_notifies_user`, `shift_ctrl_p_does_not_open_palette_after_rebind`, `ctrl_shift_p_no_longer_opens_palette_after_rebind`). One existing test replaced (`action_to_command_maps_cycle_model_regardless_of_direction` → `action_to_command_maps_cycle_model_to_cycle_model_command`). `cargo fmt --check`, `cargo clippy --package senpi-neo-tui --all-targets -- -D warnings`, and `npm run check` (897 files) all green.
</file context>
| Total Rust test count is now 329 passing (was 322 after round 9); seven new tests cover the three defects (`ctrl_t_app_thinking_toggle_visibly_notifies_user`, `ctrl_o_app_tools_expand_visibly_notifies_user`, `shift_ctrl_p_app_model_cycle_backward_visibly_notifies_user`, `apply_inbound_cycle_model_success_response_updates_displays`, `apply_inbound_cycle_model_success_response_with_null_data_pushes_note`, `apply_inbound_set_model_success_response_updates_displays`, `apply_inbound_cycle_thinking_level_success_response_updates_displays`, `apply_inbound_cycle_thinking_level_success_response_with_null_data_pushes_note`). Four existing tests updated to assert the new behavior (`ctrl_p_dispatches_cycle_model`, `shift_ctrl_p_app_model_cycle_backward_visibly_notifies_user`, `shift_ctrl_p_does_not_open_palette_after_rebind`, `ctrl_shift_p_no_longer_opens_palette_after_rebind`). One existing test replaced (`action_to_command_maps_cycle_model_regardless_of_direction` → `action_to_command_maps_cycle_model_to_cycle_model_command`). `cargo fmt --check`, `cargo clippy --package senpi-neo-tui --all-targets -- -D warnings`, and `npm run check` (897 files) all green. | |
| Total Rust test count is now 329 passing (was 322 after round 9); seven new tests cover the three defects (`ctrl_t_app_thinking_toggle_visibly_notifies_user`, `ctrl_o_app_tools_expand_visibly_notifies_user`, `apply_inbound_cycle_model_success_response_updates_displays`, `apply_inbound_cycle_model_success_response_with_null_data_pushes_note`, `apply_inbound_set_model_success_response_updates_displays`, `apply_inbound_cycle_thinking_level_success_response_updates_displays`, `apply_inbound_cycle_thinking_level_success_response_with_null_data_pushes_note`). Four existing tests updated to assert the new behavior (`ctrl_p_dispatches_cycle_model`, `shift_ctrl_p_app_model_cycle_backward_visibly_notifies_user`, `shift_ctrl_p_does_not_open_palette_after_rebind`, `ctrl_shift_p_no_longer_opens_palette_after_rebind`). One existing test replaced (`action_to_command_maps_cycle_model_regardless_of_direction` → `action_to_command_maps_cycle_model_to_cycle_model_command`). `cargo fmt --check`, `cargo clippy --package senpi-neo-tui --all-targets -- -D warnings`, and `npm run check` (897 files) all green. |
…racle round 11)
Extensions emit `extension_ui_request` JSONL frames on stdout to
drive user-facing notifications (`method: "notify"`) and modal
dialogs (`select`, `confirm`, `input`, `editor`). The neo-tui
`Event` enum had no variant for them, so the `#[serde(other)]`
arm caught them as `Event::Other` and `apply_event` silently
discarded them. An extension could emit `notifyType: "error"`
with body "Command blocked" and the user saw absolutely nothing.
Added typed `RpcEvent::ExtensionUiRequest { method, message,
notify_type, title }` variant and new `apply_extension_ui_request`
helper that routes:
- `notify` → Role::Error + footer error state for
`notifyType: "error"`, Role::System otherwise.
- Dialog methods → Role::System "not yet wired" note naming
the method + title; the agent-side timeout auto-resolves
until dialog overlays land in a future iteration.
- Per-extension UI updates (`setStatus`, `setWidget`,
`setTitle`, `set_editor_text`) stay silent. These are
per-extension self-management traffic, not user-facing
errors. The inverse contract is locked by a regression test.
Test count: 329 -> 333 passing. Four new tests cover the four
routing cases (error notify, warning notify, dialog method,
silent setStatus).
See packages/coding-agent/docs/rpc.md → "Extension UI Requests"
for the wire contract.
`cargo fmt --check`, `cargo clippy --all-targets -- -D warnings`,
`cargo test -j 1`, and `npm run check` (897 files) all green.
…-silent-errors # Conflicts: # packages/coding-agent/CHANGELOG.md
|
머지 완료. Oracle 12라운드 끝에 VERIFIED 받고 11라운드 동안 닫은 Bug 3 silent path 30+ 개 요약:
테스트 카운트: 298 (PR #14 머지 시점) → 333 passing, 0 failing, 1 ignored. 원래 4개 버그(shift+enter in tmux / 한글 폭 / 백엔드 silent failure / idle vs answering) 전부 lock 됐고, "에러가났으면 났다 안났으면 안났다 전혀안되노" 컨트랙트도 advertised 키맵 + RPC 전부 cover. 남은 follow-up (별도 이슈):
|
Follow-up to #14. Oracle's second skeptical sweep over the merged commit
45dbb577found four more silent-failure leaks that all violate the original Bug 3 contract ("if there's an error, say so; if there isn't, say so"). Each fix is locked by a TDD-red-then-green regression test on top of the existing 298-test suite.Defects fixed
1.
Inbound::Response { success: false }was silently droppedapply_inboundmatchedInbound::Response(_) => {}and threw away every backend command rejection. The wire envelope already carriessuccess: bool+error: Option<String>(seepackages/neo-tui/src/rpc/envelope.rs::Response), so a backend-reported "submit failed" or "cycle_model failed" left the TUI looking healthy. Now pushes aRole::Errorchat bubble (with the original error string when present, command-name fallback otherwise) and flips the footer toStatus::Error/command failed. Successful responses still stay silent.Tests:
app_inbound_failed_response_surfaces_to_chat_and_footer,app_inbound_failed_response_without_error_message_still_surfaces,app_inbound_successful_response_does_not_disturb_chat_or_footer.2.
Inbound::ParseErroronly logged totracing::warn!A
ParseErrormeans the backend sent something the TUI can't decode. The previous code logged it and moved on, and the regression testapp_inbound_parse_error_does_not_disrupt_uiactively asserted that the UI stays untouched. Both behaviors flipped: the runtime now pushes a chat error with the parser message + truncated frame preview and the footer goes toprotocol error. Test renamed toapp_inbound_parse_error_surfaces_to_chat_and_footerwith inverted assertions.3.
senpi --neo -- --theme opencode/draculafailed withNo such file or directoryTwo compounding bugs:
dracula,nord, ...) but the README documents the namespaced form (opencode/dracula).main.rs::looks_like_theme_pathpreviously usedvalue.contains('/')as the path heuristic, so any namespaced id was routed toread_to_string.Fixed both:
load_by_idstrips theopencode/prefix on lookup, and the path heuristic now only triggers on explicit filesystem indicators (./,../,~/, absolute path, or an existing file). Test:opencode_themes_are_resolvable_by_both_flat_id_and_opencode_prefix.4.
Alt+T(neo.theme.picker) was a no-opThe chord was bound in the JSON keymap and advertised in the README + help overlay, but
execute_actionhad no match arm for it. The dispatcher silently fell into the catch-allConsumedno-op and the overlay never opened. AddedAppAction::OpenThemePickerplus the dispatch path that buildsThemePickerOverlay::new(&self.theme.name). Test:alt_t_dispatches_open_theme_picker_and_opens_overlay.Verification
cargo fmt --package senpi-neo-tui -- --checkclean,cargo clippy --package senpi-neo-tui --all-targets -- -D warningsclean.packages/coding-agent/test/suite/regressions/still pass 9/9 (keymap parity + arg forwarding).npm run check(biome + tsgo + smoke + web-ui) clean across all 897 files.Files
Refs #14.
Summary by cubic
Surfaces every remaining
neo-tuiBug 3 silent-failure path, completes picker, command, and exit UX, and surfaces extension UI notifications. Adds clear feedback for successful model/thinking changes and for non-rendered toggles.Response { success: false }andParseErrornow push chat errors and set the footer to error.message_end.errorMessage, failingcompaction_end, andauto_retry_end { success: false }push chat errors; success paths stay quiet.extension_ui_requestnow routes to chat.notifyshows a system note or a chat error (with footer error) fornotifyType: "error".select/confirm/input/editorpost a "not yet wired" note with the title.setStatus/setWidget/setTitle/set_editor_textstay silent.Ctrl+Lopens the model picker;Alt+Topens the theme picker; picking a theme applies it or shows an error; picking a model posts a visible system note.draculaandopencode/dracula; CLI only treats explicit filesystem indicators (./,../,~/, absolute, or an existing file) as paths.tui.select.*outside overlays andtui.input.taboutside autocomplete show notes;neo.slash.openfrom the palette opens the slash overlay.app.exitalways quits, even with a non-empty input buffer.cycle_model,set_model, andcycle_thinking_levelresponses update header/footer and push a chat note; missing data shows an explanatory note.app.thinking.toggleandapp.tools.expandnow show a "not yet wired to chat rendering" note so the chord visibly lands.app.model.cycleBackwardreports "not yet wired" instead of cycling forward.Written for commit f46f986. Summary will update on new commits. Review in cubic