zeroline TUI: fix slash palette, token readout, @ files, ! bash#138
Merged
Conversation
The zeroline home footer advertised affordances the code didn't keep. Fixes:
- '/' palette: autocomplete.go suppressed a bare '/' ('/' alone showed nothing). Now a bare '/' lists the full command palette (capped at 8). The footer's '/ commands' works.
- Token readout: the zeroline header was built without Cost/TotalTokens, so the bars always showed $0.00 and no token total. zerolineHeader() now threads m.usageTracker.Summary() (TotalCost + TotalTokens, with the unpriced-token fallback); the bottom bar shows a cumulative 'tok <N>' segment (zeroline.Header gains TotalTokens; humanTokens formats it).
- '@ files': '@' input now drives a workspace file picker in the existing overlay (trailingAtToken/fileSuggestions/replaceTrailingAtToken; bounded, skips .git/node_modules/vendor/dist/hidden). Selecting inserts the @path, preserving preceding prompt text. Previously '@…' was sent to the agent as a chat prompt.
- '! bash': '!cmd' now runs as a shell escape (commandBash) — executed async with a 30s timeout in the workspace, output shown in the transcript. Previously '!cmd' was sent to the agent as chat.
- '1-5 theme': left as-is — it works when the input is empty (the home screen's state); gating on empty input keeps digits typable in prompts.
Tests: bare-/ palette, @-token detection/replacement, file listing+filter+skip-dirs, !cmd parsing, header usage. build/vet/-race/full-suite + GOOS=windows green; no new deps.
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughAdds ChangesTUI Shell Escape, File Completion, and Token Metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/tui/autocomplete.go (1)
185-187: ⚡ Quick winClarify intentional error swallowing.
The linter flags returning
nilwhenerr != nil. For a file picker, skipping inaccessible entries is reasonable, but the intent isn't obvious to future readers. A brief comment cements that this is deliberate.♻️ Suggested clarification
_ = filepath.WalkDir(cwd, func(path string, d fs.DirEntry, err error) error { if err != nil { - return nil + return nil // skip inaccessible entries; continue walking }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/autocomplete.go` around lines 185 - 187, The early return that swallows errors at the "if err != nil { return nil }" site in internal/tui/autocomplete.go should be made explicit: add a short comment above that conditional explaining that inaccessible files are intentionally skipped by the file picker (e.g., "intentionally ignore inaccessible entries; skip rather than surface error"), and optionally convert this to a debug log call if you want traceability; leave the code path and behavior in the surrounding function (the file-picker/autocomplete routine) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/model.go`:
- Around line 860-869: The commandBash branch currently invokes
runBashEscape(m.cwd, cmdText) without checking permissions; add a permission
gate using m.permissionMode (or equivalent) so that commandBash is only executed
when permissionMode==Auto or when Ask is accepted by the user, otherwise
reject/queue the command and append a system transcript message; update the
commandBash case in internal/tui/model.go to consult m.permissionMode and call
runBashEscape only after explicit allow, and add unit tests covering Auto
(runs), Ask (prompts and runs only on accept), and Unsafe (never runs)
behaviors, referencing the commandBash branch and runBashEscape for placement
and test targets.
In `@internal/zeroline/render.go`:
- Around line 1120-1129: Add a table-driven unit test for the humanTokens
function (e.g., TestHumanTokens) that verifies negative clamping and
rounding/boundary behavior: include cases for 0 -> "0", -1 -> "0", 999 -> "999",
1000 -> "1k", 1234 -> "1.2k", 999999 -> "1000k", and at least one very large
value (e.g., 1500000 or math.MaxInt32/MaxInt64) to ensure no panic and correct
formatting; iterate cases, call humanTokens(n), and assert equality with the
expected string for each case.
---
Nitpick comments:
In `@internal/tui/autocomplete.go`:
- Around line 185-187: The early return that swallows errors at the "if err !=
nil { return nil }" site in internal/tui/autocomplete.go should be made
explicit: add a short comment above that conditional explaining that
inaccessible files are intentionally skipped by the file picker (e.g.,
"intentionally ignore inaccessible entries; skip rather than surface error"),
and optionally convert this to a debug log call if you want traceability; leave
the code path and behavior in the surrounding function (the
file-picker/autocomplete routine) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e52750c-d51b-4d52-8d85-0e81bf4a371b
📒 Files selected for processing (7)
internal/tui/autocomplete.gointernal/tui/command_bash.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/tui_fixes_test.gointernal/tui/zeroline_view.gointernal/zeroline/render.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/autocomplete.go (1)
182-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBounded walk is not actually bounded by traversed nodes.
visitedincrements only for files, so a repo with many directories can still be fully traversed per keystroke, causing UI stalls. Count every visited entry (or at least directories too), and stop at>= maxVisited.Suggested fix
- visited := 0 + visited := 0 const maxVisited = 4000 _ = filepath.WalkDir(cwd, func(path string, d fs.DirEntry, err error) error { if err != nil { return nil } - if visited > maxVisited || len(out) >= maxCommandSuggestions { + visited++ + if visited >= maxVisited || len(out) >= maxCommandSuggestions { return fs.SkipAll } if d.IsDir() { name := d.Name() if path != cwd && (name == ".git" || name == "node_modules" || name == "vendor" || name == "dist" || strings.HasPrefix(name, ".")) { return fs.SkipDir } return nil } - visited++ rel, relErr := filepath.Rel(cwd, path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/autocomplete.go` around lines 182 - 199, The WalkDir callback in internal/tui/autocomplete.go only increments visited for files, so directories aren’t counted and the traversal can exceed maxVisited; update the callback used by filepath.WalkDir to increment visited for every DirEntry visit (i.e., move or add visited++ at the top of the closure) and change the stopping check to use >= maxVisited (and still check len(out) >= maxCommandSuggestions) so the walk is actually bounded; adjust logic around d.IsDir() to return fs.SkipDir after counting the entry when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/tui/autocomplete.go`:
- Around line 182-199: The WalkDir callback in internal/tui/autocomplete.go only
increments visited for files, so directories aren’t counted and the traversal
can exceed maxVisited; update the callback used by filepath.WalkDir to increment
visited for every DirEntry visit (i.e., move or add visited++ at the top of the
closure) and change the stopping check to use >= maxVisited (and still check
len(out) >= maxCommandSuggestions) so the walk is actually bounded; adjust logic
around d.IsDir() to return fs.SkipDir after counting the entry when appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 419f061a-3586-467e-8832-a102c65a72aa
📒 Files selected for processing (1)
internal/tui/autocomplete.go
CodeRabbit major: the !cmd shell escape ran bash -c directly with no permission check, bypassing the sandbox. It now runs ONLY in unsafe permission mode; in auto/ask it is not executed and the user is told to relaunch with --skip-permissions-unsafe. Adds TestBashEscapeGatedByPermissionMode (auto/ask gated, unsafe runs). CodeRabbit minor: adds TestHumanTokens covering negative clamp, 0, sub-1k, 1k trim, fractional, and the 999999->1000k rounding edge.
Collaborator
Author
|
Both fixed:
-race + full suite + Windows green. @coderabbitai @Vasanthdev2004 review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read key files from the PR branch
echo "=== command_bash.go ==="
cat internal/tui/command_bash.go
echo ""
echo "=== commands.go ==="
cat internal/tui/commands.go
echo ""
echo "=== model.go (relevant sections) ==="
cat internal/tui/model.go
echo ""
echo "=== zeroline/render.go ==="
cat internal/zeroline/render.go
echo ""
echo "=== tui_fixes_test.go ==="
cat internal/tui/tui_fixes_test.go
echo ""
echo "=== Any new test files for humanTokens or permission mode ==="
fd -e go -e _test.go . internal/ | xargs grep -l "TestBashEscapeGated\|TestHumanTokens" 2>/dev/null
echo ""
echo "=== Permission mode type/constants ==="
rg -n "PermissionMode\|permissionMode\|SkipPermissions\|unsafe" --type go -C2Length of output: 94893 🏁 Script executed: #!/bin/bash
echo "=== zeroline/tokens_test.go ==="
cat internal/zeroline/tokens_test.go
echo ""
echo "=== agent PermissionMode constants ==="
rg -n "PermissionMode" --type go -C2 internal/agent/
echo ""
echo "=== typeRunes helper (used in test) ==="
rg -n "func typeRunes" --type go -C5
echo ""
echo "=== contains helper (used in test) ==="
rg -n "func contains" internal/tui/ --type go -C3
echo ""
echo "=== autocomplete.go (full, for trailingAtToken / replaceTrailingAtToken / fileSuggestions) ==="
cat internal/tui/autocomplete.goLength of output: 24106
Safety gate (
One minor runtime note (not a blocker): Everything else ( [approve] |
Vasanthdev2004
requested changes
Jun 8, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Finding:
internal/tui/autocomplete.go:184-198:fileSuggestionsstill does not actually bound the directory walk.visitedis checked before it is incremented, and it only increments after directory handling, so directory entries are not counted. In a repo with a large nested directory tree,@file suggestions can still traverse far more thanmaxVisitedentries on each keystroke and stall the TUI.
Required fix:
- Increment
visitedfor everyWalkDirentry after the error check. - Stop on
visited >= maxVisitedorlen(out) >= maxCommandSuggestions. - Keep the existing skip-dir behavior for
.git,node_modules,vendor,dist, and hidden directories.
CI smoke is green, but this runtime responsiveness issue should be fixed before merge.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/zeroline/tokens_test.go (1)
14-16: 💤 Low valueConsider adding rounding boundary tests.
The existing cases (1234→"1.2k", 1500→"1.5k") don't reveal the rounding strategy at boundaries. Adding tests for values like 1245, 1250, 1255 would explicitly verify whether
humanTokensuses truncation, round-half-up, or round-half-to-even.For example:
{1245, "1.2k"}, // or "1.3k" depending on strategy {1250, "1.3k"}, // round-half-up, or "1.2k" if truncation {1255, "1.3k"},Not critical since the function presumably works, but documenting the strategy helps maintainability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/zeroline/tokens_test.go` around lines 14 - 16, Add explicit rounding-boundary test cases to tokens_test.go to document humanTokens' rounding strategy: add inputs like 1245, 1250, 1255 (and possibly 1249/1251) with expected string outputs that match the current implementation behavior; inspect the humanTokens function to determine whether it uses truncation, round-half-up, or bankers rounding and set the expected values accordingly (e.g., if humanTokens currently rounds half-up then {1245,"1.2k"}, {1250,"1.3k"}, {1255,"1.3k"}), and include these cases alongside the existing {1234,"1.2k"} and {1500,"1.5k"} entries so the test suite documents the chosen rounding rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/zeroline/tokens_test.go`:
- Around line 5-23: Extend the TestHumanTokens table-driven cases to include
values >= 1_000_000 to assert the function humanTokens remains in "k" notation
and trims ".0k"; e.g., add cases for 1_000_000 expecting "1000k" and 5_000_000
expecting "5000k" (optionally include a non-whole thousand like 1_000_100
expecting "1000.1k") so the tests cover the M-scale boundaries and the ".0k"
stripping behavior.
---
Nitpick comments:
In `@internal/zeroline/tokens_test.go`:
- Around line 14-16: Add explicit rounding-boundary test cases to tokens_test.go
to document humanTokens' rounding strategy: add inputs like 1245, 1250, 1255
(and possibly 1249/1251) with expected string outputs that match the current
implementation behavior; inspect the humanTokens function to determine whether
it uses truncation, round-half-up, or bankers rounding and set the expected
values accordingly (e.g., if humanTokens currently rounds half-up then
{1245,"1.2k"}, {1250,"1.3k"}, {1255,"1.3k"}), and include these cases alongside
the existing {1234,"1.2k"} and {1500,"1.5k"} entries so the test suite documents
the chosen rounding rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 591cc24e-1d87-498e-95e8-3c144a6b12f0
📒 Files selected for processing (3)
internal/tui/model.gointernal/tui/tui_fixes_test.gointernal/zeroline/tokens_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/tui_fixes_test.go
Review fixes for #138: - fileSuggestions now counts every WalkDir entry (directories included), not just files, and stops at the budget; a directory-heavy tree could otherwise be traversed in full each keystroke and stall the TUI. Extracted fileSuggestionsBounded so the per-keystroke budget is unit-tested. - humanTokens switches to "M" notation past 1,000,000 instead of rendering "1000k"+ on large-context models; tests extended to the M boundaries.
Collaborator
Author
|
@Vasanthdev2004 addressed in a263d03. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/autocomplete_test.go (1)
231-258: ⚡ Quick winAdd assertions for
vendor,dist, and generic hidden-directory skips.This test currently proves
.gitandnode_modules, but the file-picker contract also excludesvendor,dist, and dot-prefixed directories. Adding those assertions would better lock in expected behavior.🧪 Suggested test extension
func TestFileSuggestionsMatchesAndSkipsVCSDirs(t *testing.T) { @@ mustWrite("cmd/server/main.go") mustWrite(".git/config") // hidden VCS dir: must be skipped mustWrite("node_modules/dep/index.js") // dependency dir: must be skipped + mustWrite("vendor/mod/file.go") // dependency dir: must be skipped + mustWrite("dist/app.js") // build output dir: must be skipped + mustWrite(".cache/state.json") // hidden dir: must be skipped @@ all := suggestionTokens(fileSuggestions(root, "")) for _, name := range all { - if strings.Contains(name, ".git/") || strings.Contains(name, "node_modules/") { + if strings.Contains(name, ".git/") || + strings.Contains(name, "node_modules/") || + strings.Contains(name, "vendor/") || + strings.Contains(name, "dist/") || + strings.Contains(name, "@.") || strings.Contains(name, "/.") { t.Fatalf("walk must skip VCS/dependency dirs, got %q", name) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/autocomplete_test.go` around lines 231 - 258, TestFileSuggestionsMatchesAndSkipsVCSDirs currently only asserts .git and node_modules are skipped; extend it to also create entries under vendor/, dist/, and another dot-prefixed directory (e.g. .cache/) using the existing mustWrite helper, then call fileSuggestions(root, "") -> suggestionTokens and assert none of the returned names contain "vendor/", "dist/", or any ".<name>/" dot-prefixed directory (or specifically ".cache/"); update failure messages to mention which extra directory was unexpectedly present. Use the same helper functions referenced in the test (mustWrite, fileSuggestions, suggestionTokens, contains) so the new assertions align with the current test structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tui/autocomplete_test.go`:
- Around line 231-258: TestFileSuggestionsMatchesAndSkipsVCSDirs currently only
asserts .git and node_modules are skipped; extend it to also create entries
under vendor/, dist/, and another dot-prefixed directory (e.g. .cache/) using
the existing mustWrite helper, then call fileSuggestions(root, "") ->
suggestionTokens and assert none of the returned names contain "vendor/",
"dist/", or any ".<name>/" dot-prefixed directory (or specifically ".cache/");
update failure messages to mention which extra directory was unexpectedly
present. Use the same helper functions referenced in the test (mustWrite,
fileSuggestions, suggestionTokens, contains) so the new assertions align with
the current test structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 17adc687-3b25-48dc-9b0a-0f0fbc242f5d
📒 Files selected for processing (4)
internal/tui/autocomplete.gointernal/tui/autocomplete_test.gointernal/zeroline/render.gointernal/zeroline/tokens_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/zeroline/tokens_test.go
The "!cmd" shell escape is gated behind unsafe permission mode, but the
interactive TUI hardcoded Ask and shift+tab only cycles auto<->ask, so unsafe
was unreachable and the escape was dead code. Worse, the gate's own remediation
message ("Relaunch with --skip-permissions-unsafe") pointed at a flag the
interactive launch did not parse: "zero --skip-permissions-unsafe" fell through
to the unknown-command path.
Honor --skip-permissions-unsafe at interactive launch (both "zero" and
"zero zeroline"), mirroring exec, so unsafe mode -- and the ! escape -- is
reachable via an explicit opt-in. Thread the resolved mode through
runInteractiveTUI(WithSkin); the default stays Ask. Document the flag in
"zero --help" and add launch tests for both entrypoints.
Collaborator
Author
|
Pushed Problem: gating
So the advertised "! bash" footer printed a remediation pointing at a flag that didn't work. Fix: honor
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/app_test.go (1)
271-274: 💤 Low valueMissing assertion for
AgentOptions.PermissionModein zeroline test.The companion test
TestRunSkipPermissionsUnsafeLaunchesTUIInUnsafeMode(lines 233-238) asserts bothPermissionModeandAgentOptions.PermissionMode. This test only checks the former. Adding the assertion keeps test coverage consistent and guards against future regressions where the two could diverge.🧪 Proposed fix
if launchedOptions.PermissionMode != agent.PermissionModeUnsafe { t.Fatalf("PermissionMode = %q, want %q", launchedOptions.PermissionMode, agent.PermissionModeUnsafe) } + if launchedOptions.AgentOptions.PermissionMode != agent.PermissionModeUnsafe { + t.Fatalf("AgentOptions.PermissionMode = %q, want %q", launchedOptions.AgentOptions.PermissionMode, agent.PermissionModeUnsafe) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/app_test.go` around lines 271 - 274, Add the missing assertion verifying the nested AgentOptions.PermissionMode in the zeroline test: after the existing check of launchedOptions.PermissionMode, also assert that launchedOptions.AgentOptions.PermissionMode == agent.PermissionModeUnsafe (mirror what TestRunSkipPermissionsUnsafeLaunchesTUIInUnsafeMode does) so both the top-level PermissionMode and the nested AgentOptions.PermissionMode are checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cli/app_test.go`:
- Around line 271-274: Add the missing assertion verifying the nested
AgentOptions.PermissionMode in the zeroline test: after the existing check of
launchedOptions.PermissionMode, also assert that
launchedOptions.AgentOptions.PermissionMode == agent.PermissionModeUnsafe
(mirror what TestRunSkipPermissionsUnsafeLaunchesTUIInUnsafeMode does) so both
the top-level PermissionMode and the nested AgentOptions.PermissionMode are
checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 24adeb9f-cdc0-49eb-bc83-e49067be2aaf
📒 Files selected for processing (3)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/zeroline.go
…bash Now that --skip-permissions-unsafe makes the "!" escape reachable on every platform, hardcoding "bash -c" left it broken on stock Windows (no bash on PATH) and anywhere bash is not at a predictable path -- the advertised "! bash" footer would just print an exec error. Resolve the shell the same way the agent bash tool does: cmd.exe /d /s /c on Windows, /bin/sh -c elsewhere. Add a platform test asserting the command is wrapped correctly.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/command_bash.go (1)
38-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize CRLF before rendering shell output.
On Windows,
cmd.exeemits\r\n; trimming only\nleaves stray\rcharacters in the transcript (echo hibecomeshi\r). That makes the new platform-shell path render oddly even though execution succeeds.Proposed fix
- out, err := cmd.CombinedOutput() - text := strings.TrimRight(string(out), "\n") + out, err := cmd.CombinedOutput() + text := strings.ReplaceAll(string(out), "\r\n", "\n") + text = strings.TrimRight(text, "\n")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/command_bash.go` around lines 38 - 40, The output normalization currently does strings.TrimRight(string(out), "\n") which leaves stray '\r' on Windows; update the handling after cmd.CombinedOutput() so you first normalize CRLF to LF (e.g., replace "\r\n" with "\n") and then trim trailing newlines before rendering; locate the code around the CombinedOutput call using the out variable in internal/tui/command_bash.go (the cmd.CombinedOutput() call and the text := strings.TrimRight(...) line) and replace the trim-only approach with a CRLF-normalize-then-trim approach so shell output like "hi\r\n" becomes "hi\n" (no stray '\r') before rendering.
🧹 Nitpick comments (1)
internal/tui/tui_fixes_test.go (1)
32-33: ⚡ Quick winAssert the actual opt-in flag in the gated-path message.
This still passes if the system message stops telling users how to enable
!cmd. Since the recovery path is part of the UX contract here, assert--skip-permissions-unsafeexplicitly.Suggested test tightening
- if msg := lastSystemText(updated.(model)); !strings.Contains(msg, "disabled") || !strings.Contains(msg, "unsafe") { - t.Fatalf("%s mode: expected a gate message naming unsafe, got %q", mode, msg) + if msg := lastSystemText(updated.(model)); !strings.Contains(msg, "disabled") || !strings.Contains(msg, "--skip-permissions-unsafe") { + t.Fatalf("%s mode: expected gate message to explain the unsafe opt-in flag, got %q", mode, msg) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/tui_fixes_test.go` around lines 32 - 33, The test currently checks the system message mentions "disabled" and "unsafe" but doesn't assert the exact opt-in flag; update the assertion in tui_fixes_test.go (where lastSystemText(updated.(model)) is used and the variable mode is logged) to also require that the message contains the literal "--skip-permissions-unsafe" so the gated-path recovery instruction is preserved; modify the conditional that now tests for "disabled" and "unsafe" to include a check for "--skip-permissions-unsafe" and fail the test if it's missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/tui/command_bash.go`:
- Around line 38-40: The output normalization currently does
strings.TrimRight(string(out), "\n") which leaves stray '\r' on Windows; update
the handling after cmd.CombinedOutput() so you first normalize CRLF to LF (e.g.,
replace "\r\n" with "\n") and then trim trailing newlines before rendering;
locate the code around the CombinedOutput call using the out variable in
internal/tui/command_bash.go (the cmd.CombinedOutput() call and the text :=
strings.TrimRight(...) line) and replace the trim-only approach with a
CRLF-normalize-then-trim approach so shell output like "hi\r\n" becomes "hi\n"
(no stray '\r') before rendering.
---
Nitpick comments:
In `@internal/tui/tui_fixes_test.go`:
- Around line 32-33: The test currently checks the system message mentions
"disabled" and "unsafe" but doesn't assert the exact opt-in flag; update the
assertion in tui_fixes_test.go (where lastSystemText(updated.(model)) is used
and the variable mode is logged) to also require that the message contains the
literal "--skip-permissions-unsafe" so the gated-path recovery instruction is
preserved; modify the conditional that now tests for "disabled" and "unsafe" to
include a check for "--skip-permissions-unsafe" and fail the test if it's
missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e96de89-df78-4ac5-bdb9-a26a9b4f75f8
📒 Files selected for processing (2)
internal/tui/command_bash.gointernal/tui/tui_fixes_test.go
Vasanthdev2004
approved these changes
Jun 8, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the latest PR branch manually.
Verdict: APPROVE.
What I checked:
!cmdshell escape is now gated behind explicit unsafe permission mode, so it does not bypass the normal ask/auto safety posture.zero --skip-permissions-unsafeandzero zeroline --skip-permissions-unsafecorrectly thread unsafe mode into the TUI.- The
@filepicker is bounded by visited filesystem entries and normalizes inserted paths to forward slashes. - Slash palette, usage token readout, and human token formatting have reasonable tests.
Validation run locally:
go test ./internal/tui ./internal/cli ./internal/zerolinego test ./internal/tui -run 'Test(Bash|Escape|FileSuggestions|BareSlash|ParseCommand|ZerolineHeader)'go test ./internal/cli -run 'Test.*Unsafe|Test.*Zeroline|Test.*Help'go test ./...
No blocking findings from my pass. CI smoke is green; CodeRabbit was still pending when I checked.
This was referenced Jun 8, 2026
Merged
Merged
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
zeroline TUI: fix slash palette, token readout, @ files, ! bash
The zeroline home footer advertised six affordances but the code didn't keep four of them. Found via a multi-agent audit of
internal/tui+internal/zeroline; each fix is verified against the code path.Fixes
/command palette —autocomplete.goexplicitly suppressed a bare/(you had to type/hfirst), yet the footer says "/ commands". Now a bare/lists the full palette (capped at 8).Cost/TotalTokens, so the bars always read$ 0.00with no token total.zerolineHeader()now threadsusageTracker.Summary()(cost + cumulative tokens, with the unpriced fallback); the bottom bar gains atok <N>segment.@ files—@now drives a workspace file picker in the existing suggestion overlay (bounded walk; skips.git/node_modules/vendor/dist/hidden). Selecting inserts the@path, preserving any preceding prompt text. Previously@…was silently sent to the agent as chat.! bash—!cmdnow runs as a shell escape (async, 30s timeout, in the workspace), output shown in the transcript. Previously!cmdwas sent to the agent as chat.1-5 theme— left as-is: it works when the input is empty (the home screen), and gating on empty input keeps digits typable in prompts.Testing
bare-
/palette,@-token detection/replacement, file listing+filter+skip-dirs,!cmdparsing, header usage; all existing autocomplete tests still pass. build / vet /go test ./.../-race/ Windows green. No new deps.Summary by CodeRabbit
New Features
Bug Fixes
Tests