Polish model status and reasoning streams#190
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds reasoning_content stream emission and test; records provider Name/BaseURL through onboarding, wizard, and CLI; centralizes provider display labels; implements paste-preview-aware composer editing, wrapped-line visual cursor, autocomplete integration, and mouse-wheel composer hit-testing with tests. ChangesOpenAI reasoning streaming
Provider setup UI, wizard, CLI, and display
Autocomplete integration
Composer, visual cursor, mouse, and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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.
🧹 Nitpick comments (2)
internal/providers/openai/provider_test.go (1)
210-227: ⚡ Quick winConsider adding test cases for mixed reasoning and regular content.
The current test verifies reasoning content in isolation. Consider adding test coverage for:
- A delta containing both
reasoning_contentandcontent(to verify both are emitted and in the correct order)- Empty
reasoning_content(to confirm it's not emitted, consistent with line 230's check)This would catch potential ordering or emission edge cases.
🤖 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/providers/openai/provider_test.go` around lines 210 - 227, Update TestStreamCompletionEmitsReasoningContentDeltas to add cases that exercise mixed and empty fields: inside the handler passed to newTestProvider, emit additional SSE messages including one delta with both "reasoning_content" and "content" in the same payload (e.g. `{"choices":[{"delta":{"reasoning_content":"Think ","content":"Hello"}}]}`) and another delta with an empty "reasoning_content" (`"reasoning_content":""`), then collect events via collectProviderEvents and assert that the concatenated text preserves the expected ordering ("Think Hello..." where reasoning text appears before content from the same delta) and that empty reasoning_content produces no output; keep existing assertions but extend them to verify both mixed-field ordering and suppression of empty reasoning_content in TestStreamCompletionEmitsReasoningContentDeltas.internal/tui/command_center.go (1)
234-234: ⚡ Quick winConsider using
displayValuefor target provider consistency.The compaction messages display
TargetProvider(line 435) which is set here to the bare provider kind. For UX consistency with the updated model/mode switch confirmations (lines 273, 403), consider showing the profile name with kind fallback.♻️ Proposed consistency fix
At line 234:
if guarded, text, requested := m.requestCompactionBeforeModelSwitch(modelSwitchCompactionRequest{ TargetModel: target.modelID, - TargetProvider: string(metadata.ProviderKind), + TargetProvider: displayValue(nextProfile.Name, string(metadata.ProviderKind)), TargetContextWindow: modelContextWindow(target.modelID),At line 360:
if guarded, text, requested := m.requestCompactionBeforeModelSwitch(modelSwitchCompactionRequest{ TargetModel: entry.ID, - TargetProvider: string(metadata.ProviderKind), + TargetProvider: displayValue(nextProfile.Name, string(metadata.ProviderKind)), TargetContextWindow: modelContextWindow(entry.ID),Also applies to: 360-360
🤖 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_center.go` at line 234, The TargetProvider is currently set to the raw provider kind; change the assignment for TargetProvider to use the displayValue helper so the UI shows the profile name with kind as a fallback (e.g., call displayValue(metadata.ProfileName, metadata.ProviderKind)) instead of string(metadata.ProviderKind); update any other sites setting TargetProvider the same way (the symbol to change is TargetProvider and the inputs are metadata.ProfileName and metadata.ProviderKind) to keep compaction and switch confirmation messages consistent with the model/mode UI.
🤖 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/providers/openai/provider_test.go`:
- Around line 210-227: Update TestStreamCompletionEmitsReasoningContentDeltas to
add cases that exercise mixed and empty fields: inside the handler passed to
newTestProvider, emit additional SSE messages including one delta with both
"reasoning_content" and "content" in the same payload (e.g.
`{"choices":[{"delta":{"reasoning_content":"Think ","content":"Hello"}}]}`) and
another delta with an empty "reasoning_content" (`"reasoning_content":""`), then
collect events via collectProviderEvents and assert that the concatenated text
preserves the expected ordering ("Think Hello..." where reasoning text appears
before content from the same delta) and that empty reasoning_content produces no
output; keep existing assertions but extend them to verify both mixed-field
ordering and suppression of empty reasoning_content in
TestStreamCompletionEmitsReasoningContentDeltas.
In `@internal/tui/command_center.go`:
- Line 234: The TargetProvider is currently set to the raw provider kind; change
the assignment for TargetProvider to use the displayValue helper so the UI shows
the profile name with kind as a fallback (e.g., call
displayValue(metadata.ProfileName, metadata.ProviderKind)) instead of
string(metadata.ProviderKind); update any other sites setting TargetProvider the
same way (the symbol to change is TargetProvider and the inputs are
metadata.ProfileName and metadata.ProviderKind) to keep compaction and switch
confirmation messages consistent with the model/mode UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a22b2a6b-7acf-4d32-b572-0e2b62e7c985
📒 Files selected for processing (4)
internal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/types.gointernal/tui/command_center.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/tui/provider_wizard_test.go (1)
322-442: ⚡ Quick winAdd regression coverage for IP endpoint names and remote
http://rejection.This test already validates the custom endpoint flow well; adding cases for
127.0.0.1-style endpoint naming and non-localhttp://validation would lock in the expected behavior and prevent regressions in profile-label derivation and transport safety.🤖 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/provider_wizard_test.go` around lines 322 - 442, Add regression cases to TestProviderWizardCustomCompatibleProviderCollectsEndpointAndModel: (1) exercise an IP-style endpoint (e.g. "https://127.0.0.1/v1" or "https://127.0.0.1:1234/v1") by filling the endpoint, advancing through name/model/save and assert captured.Name equals the IP host (e.g. "127.0.0.1" or "127.0.0.1:1234") and captured.BaseURL equals the provided URL; (2) exercise a non-local insecure endpoint (e.g. "http://api.example.com/v1") by entering that endpoint and asserting the wizard does not advance from providerWizardStepEndpoint and the view contains the validation error about insecure/non-HTTPS endpoints; add these as subtests inside the same test (or new focused tests) reusing the same model creation hook (NewProvider capture) and the existing provider wizard flow functions (openProviderWizardForTest, providerWizardProviderIndex, providerWizardStepEndpoint, providerWizardStepName, providerWizardStepModel) so the assertions verify name derivation and HTTP rejection regressions.internal/cli/setup_test.go (1)
137-178: ⚡ Quick winAdd an explicit test for CLI-over-selection precedence.
This test covers fallback from
SetupSelection, but not the new contract where CLI-providedname/baseURLshould override selection values when both are present. Add a sibling case with conflicting values and assert the persisted profile uses CLI inputs.🤖 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/setup_test.go` around lines 137 - 178, Add a sibling test to TestSaveSetupProviderStoresCustomEndpointSelection that verifies CLI-provided name/baseURL override the selection: call saveSetupProvider again (or add another test function) with a tui.SetupSelection containing CatalogID "custom-openai-compatible" and Name/BaseURL/Model set to some values, but pass conflicting CLI values via the saveSetupProvider inputs (i.e., set the CLI name and BaseURL different from the selection); then assert result.Provider.Name and result.Provider.BaseURL (and persisted cfg.Providers[0]) equal the CLI-provided values, not the selection's values; use the same helpers (appDeps, saveSetupProvider, readFileConfig, setupSaveOptions) and mirror existing assertions for Model/ActiveProvider consistency.
🤖 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/provider_wizard.go`:
- Around line 1018-1024: The function providerWizardNameFromBaseURL currently
treats IP hosts like domain names and returns the penultimate token (e.g.,
"127.0.0.1" → "0"); change it to detect IP addresses using
net.ParseIP(parsed.Hostname()) and, when it's an IP, return a stable sanitized
host-derived name (for example, replace dots with hyphens or replace
non-alphanumeric characters with '-' and optionally prefix with "ip-") instead
of splitting on dots; keep the existing trimming of "api." and "gateway." and
the domain-splitting logic for non-IP hosts.
- Around line 988-999: providerWizardEndpointError currently allows plain http
for any host; update it to only permit http for local loopback hosts and require
https otherwise: after parsing the URL use parsed.Hostname() (to strip ports)
and treat as local if it equals "localhost" or "127.0.0.1" or "::1" (or you can
add other loopback names if desired); if parsed.Scheme == "http" and the
hostname is not a recognized loopback, return an error like "endpoint URL must
use https:// unless it's a local loopback (localhost, 127.0.0.1, ::1)"; keep
existing checks for empty/invalid URL and for allowed schemes. Ensure the
function name providerWizardEndpointError and the use of parsed.Hostname() are
updated accordingly.
---
Nitpick comments:
In `@internal/cli/setup_test.go`:
- Around line 137-178: Add a sibling test to
TestSaveSetupProviderStoresCustomEndpointSelection that verifies CLI-provided
name/baseURL override the selection: call saveSetupProvider again (or add
another test function) with a tui.SetupSelection containing CatalogID
"custom-openai-compatible" and Name/BaseURL/Model set to some values, but pass
conflicting CLI values via the saveSetupProvider inputs (i.e., set the CLI name
and BaseURL different from the selection); then assert result.Provider.Name and
result.Provider.BaseURL (and persisted cfg.Providers[0]) equal the CLI-provided
values, not the selection's values; use the same helpers (appDeps,
saveSetupProvider, readFileConfig, setupSaveOptions) and mirror existing
assertions for Model/ActiveProvider consistency.
In `@internal/tui/provider_wizard_test.go`:
- Around line 322-442: Add regression cases to
TestProviderWizardCustomCompatibleProviderCollectsEndpointAndModel: (1) exercise
an IP-style endpoint (e.g. "https://127.0.0.1/v1" or
"https://127.0.0.1:1234/v1") by filling the endpoint, advancing through
name/model/save and assert captured.Name equals the IP host (e.g. "127.0.0.1" or
"127.0.0.1:1234") and captured.BaseURL equals the provided URL; (2) exercise a
non-local insecure endpoint (e.g. "http://api.example.com/v1") by entering that
endpoint and asserting the wizard does not advance from
providerWizardStepEndpoint and the view contains the validation error about
insecure/non-HTTPS endpoints; add these as subtests inside the same test (or new
focused tests) reusing the same model creation hook (NewProvider capture) and
the existing provider wizard flow functions (openProviderWizardForTest,
providerWizardProviderIndex, providerWizardStepEndpoint, providerWizardStepName,
providerWizardStepModel) so the assertions verify name derivation and HTTP
rejection regressions.
🪄 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: b77075ab-8a25-4b48-bdf7-4ff48f9b10e2
📒 Files selected for processing (10)
internal/cli/setup.gointernal/cli/setup_test.gointernal/tui/onboarding.gointernal/tui/onboarding_test.gointernal/tui/options.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_discovery.gointernal/tui/provider_wizard_test.gointernal/tui/rendering_lime_test.gointernal/tui/view.go
✅ Files skipped from review due to trivial changes (1)
- internal/tui/rendering_lime_test.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/model.go (1)
1124-1159: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove unreachable hint-rendering branches.
The
hintvariable is declared on line 1132 but never assigned a non-empty value, making all the conditional hint-formatting branches unreachable:
- Lines 1135–1140 (multiline composer hint)
- Lines 1150–1152 (argument hint with right-aligned hint)
- Lines 1156–1158 (general hint)
Since the PR intentionally removes the "esc stop" pending hint, these dead branches should be removed to avoid confusion.
♻️ Proposed cleanup
// composerLine renders the borderless composer. func (m model) composerLine(width int) string { input := m.input if m.suggestionsActive() && (!m.suggestionsAreFiles || fileSuggestionOnlyInput(m.input.Value())) { input.SetValue("") input.Placeholder = "" input.CursorEnd() } - hint := "" if m.composerActive && strings.Contains(m.composer.text, "\n") { line := renderComposerState(m.composer, m.input.Prompt, width) - if hint == "" { - return line - } - lines := strings.Split(line, "\n") - lines[len(lines)-1] = joinHeaderLine(fitStyledLine(lines[len(lines)-1], width-lipgloss.Width(hint)-2), hint, width) - return strings.Join(lines, "\n") + return line } argumentHint := commandArgumentHintForInput(input.Value()) if argumentHint != "" && input.Position() != len([]rune(input.Value())) { argumentHint = "" } if argumentHint != "" { input.Width = 0 - line := commandArgumentHintComposerLine(input, argumentHint) - if hint == "" { - return fitStyledLine(line, width) - } - return joinHeaderLine(fitStyledLine(line, width-lipgloss.Width(hint)-2), hint, width) + return fitStyledLine(commandArgumentHintComposerLine(input, argumentHint), width) } - line := input.View() - if hint == "" { - return fitStyledLine(line, width) - } - return joinHeaderLine(fitStyledLine(line, width-lipgloss.Width(hint)-2), hint, width) + return fitStyledLine(input.View(), width) }🤖 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/model.go` around lines 1124 - 1159, The composerLine function declares a never-set variable hint and contains dead branches that format lines around that hint; remove the unused hint variable and the conditional branches that reference it in composerLine, simplifying the control flow to directly return the rendered strings (use renderComposerState/commandArgumentHintComposerLine/input.View as appropriate) and remove lipgloss.Width(hint) adjustments—i.e., delete the multiline-hint block that uses renderComposerState, drop the right-aligned-argument-hint branch, and remove the final hint-aware joinHeaderLine usages so the function returns the fitted lines (fitStyledLine or joinHeaderLine without hint logic) and compiles cleanly.
🧹 Nitpick comments (1)
internal/tui/rendering_lime_test.go (1)
796-820: ⚡ Quick winConsider edge case coverage for endpoint-derived provider names.
The test verifies the happy path where "minimax" is successfully derived from
https://api.minimax.io/v1. Consider adding test cases for edge scenarios:
- Empty or missing
BaseURL- Non-standard URLs (localhost, IP addresses, malformed)
- Fallback behavior when domain extraction fails
This would ensure the derivation logic handles unusual custom endpoint configurations gracefully.
🤖 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/rendering_lime_test.go` around lines 796 - 820, Add additional test cases to TestGenericCustomProviderDisplayUsesEndpointName to cover edge cases for endpoint-derived provider names: create subtests that call newModel with Options where ProviderProfile.BaseURL is empty, a localhost URL (e.g., "http://localhost:8000"), an IP address URL (e.g., "http://127.0.0.1:8080"), and a malformed URL (e.g., "ht!tp://bad"), then assert the behavior of m.titleBar(120) and m.statusLine(100) for each—verify expected fallback labels (e.g., use ProviderProfile.CatalogID or ProviderName when extraction fails) and ensure no panics occur; reference the existing assertions against plainRender(t, m.titleBar(120)) and plainRender(t, m.statusLine(100)) as the way to validate outputs.
🤖 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/model.go`:
- Around line 1124-1159: The composerLine function declares a never-set variable
hint and contains dead branches that format lines around that hint; remove the
unused hint variable and the conditional branches that reference it in
composerLine, simplifying the control flow to directly return the rendered
strings (use renderComposerState/commandArgumentHintComposerLine/input.View as
appropriate) and remove lipgloss.Width(hint) adjustments—i.e., delete the
multiline-hint block that uses renderComposerState, drop the
right-aligned-argument-hint branch, and remove the final hint-aware
joinHeaderLine usages so the function returns the fitted lines (fitStyledLine or
joinHeaderLine without hint logic) and compiles cleanly.
---
Nitpick comments:
In `@internal/tui/rendering_lime_test.go`:
- Around line 796-820: Add additional test cases to
TestGenericCustomProviderDisplayUsesEndpointName to cover edge cases for
endpoint-derived provider names: create subtests that call newModel with Options
where ProviderProfile.BaseURL is empty, a localhost URL (e.g.,
"http://localhost:8000"), an IP address URL (e.g., "http://127.0.0.1:8080"), and
a malformed URL (e.g., "ht!tp://bad"), then assert the behavior of
m.titleBar(120) and m.statusLine(100) for each—verify expected fallback labels
(e.g., use ProviderProfile.CatalogID or ProviderName when extraction fails) and
ensure no panics occur; reference the existing assertions against plainRender(t,
m.titleBar(120)) and plainRender(t, m.statusLine(100)) as the way to validate
outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 92dff4ca-d4cd-466e-9388-15bfca2d7d56
📒 Files selected for processing (3)
internal/tui/model.gointernal/tui/model_test.gointernal/tui/rendering_lime_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/setup_test.go (1)
180-221: ⚡ Quick winConsider testing partial CLI override scenarios.
This test verifies that both
nameandbaseURLCLI options override the selection values. Consider adding tests for partial overrides:
- CLI provides only
name(should use CLI name + selection baseURL)- CLI provides only
baseURL(should use selection name + CLI baseURL)This would ensure the fallback/merge behavior is correct for all CLI usage patterns.
🤖 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/setup_test.go` around lines 180 - 221, Add two new unit tests alongside TestSaveSetupProviderCLIOptionsOverrideCustomEndpointSelection to cover partial CLI overrides: one where setupSaveOptions supplies only name (e.g., name: "cli-name" and baseURL: "" or zero value) and assert saveSetupProvider(appDeps..., tui.SetupSelection{BaseURL:"https://selection.example/v1", Name:"selection-name", Model:"selection-model", CatalogID:"custom-openai-compatible"}, setupSaveOptions{name:"cli-name"}) returns Provider.Name == "cli-name", Provider.BaseURL == "https://selection.example/v1", Provider.Model == "selection-model" and the stored config reflects those values; and another where setupSaveOptions supplies only baseURL (e.g., baseURL: "https://cli.example/v1") and assert the result uses selection Name with CLI baseURL (Provider.Name == "selection-name", Provider.BaseURL == "https://cli.example/v1", Provider.Model == "selection-model") and the config is stored accordingly; use the same helper functions (saveSetupProvider, readFileConfig, appDeps, tui.SetupSelection, setupSaveOptions) and t.TempDir pattern as the existing test.
🤖 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/setup_test.go`:
- Around line 180-221: Add two new unit tests alongside
TestSaveSetupProviderCLIOptionsOverrideCustomEndpointSelection to cover partial
CLI overrides: one where setupSaveOptions supplies only name (e.g., name:
"cli-name" and baseURL: "" or zero value) and assert
saveSetupProvider(appDeps...,
tui.SetupSelection{BaseURL:"https://selection.example/v1",
Name:"selection-name", Model:"selection-model",
CatalogID:"custom-openai-compatible"}, setupSaveOptions{name:"cli-name"})
returns Provider.Name == "cli-name", Provider.BaseURL ==
"https://selection.example/v1", Provider.Model == "selection-model" and the
stored config reflects those values; and another where setupSaveOptions supplies
only baseURL (e.g., baseURL: "https://cli.example/v1") and assert the result
uses selection Name with CLI baseURL (Provider.Name == "selection-name",
Provider.BaseURL == "https://cli.example/v1", Provider.Model ==
"selection-model") and the config is stored accordingly; use the same helper
functions (saveSetupProvider, readFileConfig, appDeps, tui.SetupSelection,
setupSaveOptions) and t.TempDir pattern as the existing test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9af7a8e7-ea82-4bf7-bf03-c8e76a9fca25
📒 Files selected for processing (3)
internal/cli/setup_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/provider_wizard.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mouse.go`:
- Around line 163-186: The mouseOverComposer hit-test in model.mouseOverComposer
uses the full footerView to compute coordinates but rendering clips the footer
when alt-screen/height is small, causing wheel events to be misattributed;
update mouseOverComposer to compute its top/height from the exact clipped footer
slice used by rendering (i.e., apply the same prefix/truncation logic used when
drawing the footer so composerTop is based on the rendered footerLines, not the
full footerView), keep using viewLines(composerBox(...)) for composer geometry,
and adjust the footerTop calculation to include the clipped-prefix offset; then
add a tiny-height regression test in internal/tui/scroll_test.go that sets a
small terminal height where the footer is truncated and verifies wheel events
over the visible footer do not get treated as over the composer.
🪄 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: 8d067752-a214-4253-8708-792cc6830c74
📒 Files selected for processing (6)
internal/tui/composer.gointernal/tui/composer_test.gointernal/tui/model.gointernal/tui/mouse.gointernal/tui/rendering_lime_test.gointernal/tui/scroll_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/tui/composer_test.go (1)
169-171: 💤 Low valueConsider using a positive assertion form for clarity.
The condition
len(next.composerPastePreviews) != 0is logically correct but slightly indirect. A positive form would be more idiomatic:- if len(next.composerPastePreviews) != 0 { - t.Fatal("paste preview should clear after deleting pasted block") - } + if got := len(next.composerPastePreviews); got > 0 { + t.Fatalf("paste preview should clear after deleting pasted block, got %d previews", got) + }This makes the expected condition (zero previews) more explicit and adds diagnostic output.
🤖 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/composer_test.go` around lines 169 - 171, The test assertion in internal/tui/composer_test.go should use a positive check: replace the negative condition on next.composerPastePreviews with an explicit equality check (len(next.composerPastePreviews) == 0) and emit a diagnostic message showing the actual length when it fails (e.g., use t.Fatalf or t.Fatalf-like call that prints the unexpected length) so the expectation and failure info are clearer.internal/tui/scroll_test.go (2)
58-76: ⚡ Quick winDocument the "clipped footer" assumption or verify it.
The test name suggests it's validating behavior when the footer/status is clipped at
height: 3, but the test doesn't explicitly verify that clipping actually occurs. It assumes that wheel events atY: m.height-1in a clipped layout won't affect the composer.Consider adding:
- A comment explaining the layout assumption (e.g., "// At height=3, footer is clipped; Y=2 should not hit composer region")
- Or an assertion that verifies the expected layout state before testing the wheel event
This would make the test's coverage more explicit and guard against silently testing unintended behavior if layout logic changes.
🤖 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/scroll_test.go` around lines 58 - 76, Test assumes footer is clipped at height=3 but doesn't verify it: in TestMouseWheelOnClippedFooterStatusDoesNotMoveComposerCursor, either add a brief comment documenting the assumption (e.g., "At height=3 the footer/status is clipped; Y=2 targets footer area") or, better, assert the layout state before sending the wheel event by checking the model's rendered/footer height or a boolean that indicates clipping (use m.height, m.height-1 and current layout methods/state); ensure the assertion confirms the footer is clipped (or that the composer region does not include Y==m.height-1) so the subsequent mouse wheel check against currentComposerState().cursor is explicitly valid.
35-56: ⚡ Quick winConsider documenting or computing the Y coordinate assumption.
The test uses
Y: 14assuming it falls within the composer region atheight: 20, but this magic number depends on internal layout logic. If the composer position changes, the test might silently stop testing the intended behavior.Consider either:
- Adding a comment explaining the layout assumption (e.g., "// Y=14 targets the wrapped composer, which renders at lines 11-15 at height=20")
- Computing Y from known layout constants if available
This would make the test's intent clearer and more maintainable.
🤖 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/scroll_test.go` around lines 35 - 56, The test TestMouseWheelOverWrappedComposerMovesComposerCursor relies on a magic Y coordinate (Y: 14) to target the wrapped composer; update the test to make this intent explicit by either adding a brief comment explaining the layout assumption (e.g., that with m.height = 20 the composer renders around that Y) or compute Y dynamically from known layout values (derive the composer top/bottom line from the same layout logic/constants used by the model and use that computed Y when calling m.Update), referencing the test function name and the m.Update(tea.MouseMsg{...}) call so reviewers can find and verify the change.
🤖 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/composer_test.go`:
- Around line 169-171: The test assertion in internal/tui/composer_test.go
should use a positive check: replace the negative condition on
next.composerPastePreviews with an explicit equality check
(len(next.composerPastePreviews) == 0) and emit a diagnostic message showing the
actual length when it fails (e.g., use t.Fatalf or t.Fatalf-like call that
prints the unexpected length) so the expectation and failure info are clearer.
In `@internal/tui/scroll_test.go`:
- Around line 58-76: Test assumes footer is clipped at height=3 but doesn't
verify it: in TestMouseWheelOnClippedFooterStatusDoesNotMoveComposerCursor,
either add a brief comment documenting the assumption (e.g., "At height=3 the
footer/status is clipped; Y=2 targets footer area") or, better, assert the
layout state before sending the wheel event by checking the model's
rendered/footer height or a boolean that indicates clipping (use m.height,
m.height-1 and current layout methods/state); ensure the assertion confirms the
footer is clipped (or that the composer region does not include Y==m.height-1)
so the subsequent mouse wheel check against currentComposerState().cursor is
explicitly valid.
- Around line 35-56: The test
TestMouseWheelOverWrappedComposerMovesComposerCursor relies on a magic Y
coordinate (Y: 14) to target the wrapped composer; update the test to make this
intent explicit by either adding a brief comment explaining the layout
assumption (e.g., that with m.height = 20 the composer renders around that Y) or
compute Y dynamically from known layout values (derive the composer top/bottom
line from the same layout logic/constants used by the model and use that
computed Y when calling m.Update), referencing the test function name and the
m.Update(tea.MouseMsg{...}) call so reviewers can find and verify the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 30a8e5d7-4a74-4c70-9efc-19308dac22ef
📒 Files selected for processing (5)
internal/tui/composer.gointernal/tui/composer_test.gointernal/tui/model.gointernal/tui/mouse.gointernal/tui/scroll_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/tui/mouse.go
- internal/tui/composer.go
- internal/tui/model.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/composer.go (1)
272-274:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClearing all paste previews on file mention deletion may be unintended.
When a completed file mention is deleted via backspace, all paste previews are cleared (
m.composerPastePreviews = nil). If the user has pasted content elsewhere in the composer, those previews are lost. Consider usingdeleteComposerRangeWithPastePreviewsinstead to preserve unrelated previews:Proposed fix
} else if nextState, ok := deleteCompletedFileMentionBefore(state); ok && !m.suggestionsActive() { - m.setComposerState(nextState) - m.composerPastePreviews = nil + // deleteCompletedFileMentionBefore returns the state after deletion; + // compute the deleted range to update previews properly + deletedRunes := len([]rune(state.text)) - len([]rune(nextState.text)) + _, nextPreviews := deleteComposerRangeWithPastePreviews(state, m.composerPastePreviews, nextState.cursor, nextState.cursor+deletedRunes) + m.setComposerState(nextState) + m.composerPastePreviews = nextPreviews🤖 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/composer.go` around lines 272 - 274, The code currently clears all paste previews when a completed file mention is deleted (the branch that calls deleteCompletedFileMentionBefore and then sets m.composerPastePreviews = nil); instead, call the helper deleteComposerRangeWithPastePreviews to remove only the paste previews associated with the deleted mention and preserve unrelated previews. Concretely, replace the m.composerPastePreviews = nil line with a call to deleteComposerRangeWithPastePreviews(state, nextState) (or the appropriate signature) after m.setComposerState(nextState), ensuring the branch that checks deleteCompletedFileMentionBefore(state) && !m.suggestionsActive() uses that helper to update previews rather than nuking m.composerPastePreviews.
🤖 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/composer.go`:
- Around line 272-274: The code currently clears all paste previews when a
completed file mention is deleted (the branch that calls
deleteCompletedFileMentionBefore and then sets m.composerPastePreviews = nil);
instead, call the helper deleteComposerRangeWithPastePreviews to remove only the
paste previews associated with the deleted mention and preserve unrelated
previews. Concretely, replace the m.composerPastePreviews = nil line with a call
to deleteComposerRangeWithPastePreviews(state, nextState) (or the appropriate
signature) after m.setComposerState(nextState), ensuring the branch that checks
deleteCompletedFileMentionBefore(state) && !m.suggestionsActive() uses that
helper to update previews rather than nuking m.composerPastePreviews.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 16df044f-33ac-4dde-b6a5-d4d23e04f8c1
📒 Files selected for processing (2)
internal/tui/composer.gointernal/tui/composer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/composer_test.go
|
@coderabbitai review the latest commits that u havent reviewed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the list of commits on the PR
gh pr view 190 --repo Gitlawb/zero --json commits --jq '.commits[] | {oid: .oid, message: .messageHeadline, authoredDate: .authoredDate}'
echo "---REVIEWS---"
# Get existing reviews
gh pr reviews 190 --repo Gitlawb/zero 2>/dev/null || echo "no reviews command"
echo "---REVIEW COMMENTS---"
# Get existing review comments count
gh api repos/Gitlawb/zero/pulls/190/reviews --jq '.[].submitted_at' 2>/dev/null | head -20Length of output: 2162 🏁 Script executed: #!/bin/bash
# Get PR files changed
gh pr view 190 --repo Gitlawb/zero --json files --jq '.files[].path'Length of output: 749 🏁 Script executed: #!/bin/bash
# Get diff for the last two commits
echo "=== Commit: Disable textinput ctrl-v paste binding (a397dc2) ==="
gh api repos/Gitlawb/zero/commits/a397dc2c7b99937e2b3ed83b8b0b16523f6dad85 --jq '.files[] | "File: \(.filename)\n+\(.additions) -\(.deletions)\n\(.patch)"'
echo ""
echo "=== Commit: Fix composer paste preview deletion (641eca2) ==="
gh api repos/Gitlawb/zero/commits/641eca21192dc553e6dae19a2fb509812da367a2 --jq '.files[] | "File: \(.filename)\n+\(.additions) -\(.deletions)\n\(.patch)"'Length of output: 15666
Commit
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/autocomplete_test.go (1)
633-744: ⚡ Quick winAdd one cursor-local paste-preview file-mention test.
These additions validate the end-of-input
@guidepath well. Please add one case with multiple mentions (for example,compare@oldwith@guide``) and cursor-local completion/dismiss after a pasted block, so the new range-based replacement path is covered under cursor-local conditions too.🤖 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 633 - 744, Add a new test named TestFileSuggestionMultipleMentionsCursorLocalKeepsPastePreviewCollapsed that follows the existing pattern (uses newModel, mustWriteFile, plainRender, m.Update, m.composerValue, m.composerBox, m.suggestionsActive, m.suggestionsAreFiles) but exercises the cursor-local, range-based replacement path with multiple file mentions: paste the long text (as in other tests), then insert " compare `@old` with `@guide`" (so there are two `@-mentions`), assert file suggestions are active when at each mention, perform a cursor-local completion for the first mention (simulate completing only `@old` via tea.KeyEnter) and verify composerValue has that single mention replaced while pasted content remains collapsed, then move the cursor to the second mention (simulate moving focus to that mention) and dismiss the suggestion with tea.KeyEsc and verify the second mention is removed, composerValue ends in a single trailing space, suggestions are closed, and the rendered view (plainRender(m.composerBox(96))) shows the collapsed paste preview without any "SECRET_TAIL" and contains the expected preview fragments; use the same assertion styles and messages as the nearby tests.
🤖 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 633-744: Add a new test named
TestFileSuggestionMultipleMentionsCursorLocalKeepsPastePreviewCollapsed that
follows the existing pattern (uses newModel, mustWriteFile, plainRender,
m.Update, m.composerValue, m.composerBox, m.suggestionsActive,
m.suggestionsAreFiles) but exercises the cursor-local, range-based replacement
path with multiple file mentions: paste the long text (as in other tests), then
insert " compare `@old` with `@guide`" (so there are two `@-mentions`), assert file
suggestions are active when at each mention, perform a cursor-local completion
for the first mention (simulate completing only `@old` via tea.KeyEnter) and
verify composerValue has that single mention replaced while pasted content
remains collapsed, then move the cursor to the second mention (simulate moving
focus to that mention) and dismiss the suggestion with tea.KeyEsc and verify the
second mention is removed, composerValue ends in a single trailing space,
suggestions are closed, and the rendered view (plainRender(m.composerBox(96)))
shows the collapsed paste preview without any "SECRET_TAIL" and contains the
expected preview fragments; use the same assertion styles and messages as the
nearby tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a9e54a4-78d5-4bf5-88d6-40a26dd6de7b
📒 Files selected for processing (4)
internal/tui/autocomplete.gointernal/tui/autocomplete_test.gointernal/tui/composer.gointernal/tui/model.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/model.go
- internal/tui/composer.go
|
@CodeRabbit can u do a full re review now, checking unchecked commits only is fine |
|
✅ Action performedFull review finished. |
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/providers/openai/provider_test.go (1)
210-227: ⚡ Quick winConsider adding test coverage for mixed reasoning and regular content.
The test correctly verifies that
reasoning_contentdeltas are emitted as text events. To strengthen coverage, consider adding a test case where a single chunk contains bothreasoning_contentandcontentto verify that reasoning is emitted before regular content.🧪 Example test case for mixed content
func TestStreamCompletionEmitsReasoningBeforeRegularContent(t *testing.T) { provider := newTestProvider(t, func(w http.ResponseWriter, r *http.Request) { writeSSE(w, `{"choices":[{"delta":{"reasoning_content":"Thinking. ","content":"Answer."}}]}`) writeSSE(w, `[DONE]`) }) events := collectProviderEvents(t, provider) textEvents := eventsOfType(events, zeroruntime.StreamEventText) if len(textEvents) != 2 { t.Fatalf("text events = %d, want 2 (reasoning + content)", len(textEvents)) } if textEvents[0].Content != "Thinking. " { t.Fatalf("first text event = %q, want reasoning content", textEvents[0].Content) } if textEvents[1].Content != "Answer." { t.Fatalf("second text event = %q, want regular content", textEvents[1].Content) } }🤖 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/providers/openai/provider_test.go` around lines 210 - 227, Add a new unit test to cover mixed reasoning and regular content by creating a test (e.g., TestStreamCompletionEmitsReasoningBeforeRegularContent) that uses the same test harness (newTestProvider, writeSSE) to emit a single SSE chunk containing both "reasoning_content" and "content" in the same delta, then collect events via collectProviderEvents and assert there are two text events with the first event's Content equal to the reasoning text and the second equal to the regular content; this verifies the provider emits reasoning_content before regular content without changing the existing TestStreamCompletionEmitsReasoningContentDeltas test.internal/tui/scroll_test.go (2)
68-75: ⚡ Quick winAssert chat-scroll routing in the clipped-footer wheel test.
The test currently verifies only that the composer cursor stays unchanged. Add an assertion that
chatScrollOffsetchanges, so it also proves the wheel event was routed to chat scrolling (not swallowed).💡 Suggested assertion addition
if got := next.currentComposerState().cursor; got != startCursor { t.Fatalf("composer cursor = %d, want unchanged end cursor %d", got, startCursor) } + if next.chatScrollOffset != chatWheelScrollLines { + t.Fatalf("chatScrollOffset = %d, want %d", next.chatScrollOffset, chatWheelScrollLines) + } }🤖 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/scroll_test.go` around lines 68 - 75, The test only checks the composer cursor but must also assert that the wheel event changed chat scrolling; before calling m.Update record the starting chatScrollOffset (e.g., startOffset := m.chatScrollOffset), then after the update and casting updated.(model) assert that next.chatScrollOffset != startOffset (or assert the expected delta direction if known) to prove the wheel was routed to chat scrolling rather than swallowed.
35-56: ⚡ Quick winAvoid hard-coding the wheel hit row in the composer-wheel test.
Y: 14is layout-coupled and can become fragile when footer/composer rendering changes. Derive a row that actually satisfiesmouseOverComposerin the test setup.💡 Suggested test hardening
func TestMouseWheelOverWrappedComposerMovesComposerCursor(t *testing.T) { @@ - updated, cmd := m.Update(tea.MouseMsg{Button: tea.MouseButtonWheelUp, Y: 14}) + y := -1 + for row := 0; row < m.height; row++ { + if m.mouseOverComposer(tea.MouseMsg{Button: tea.MouseButtonWheelUp, Y: row}) { + y = row + break + } + } + if y < 0 { + t.Fatal("expected a visible composer row for wheel hit-testing") + } + updated, cmd := m.Update(tea.MouseMsg{Button: tea.MouseButtonWheelUp, Y: y})🤖 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/scroll_test.go` around lines 35 - 56, The test TestMouseWheelOverWrappedComposerMovesComposerCursor hard-codes Y:14 which is layout-coupled; instead compute a Y that actually hits the composer by deriving it from the model. After creating m and setting sizes, pick a hitRow by scanning y from 0..m.height-1 and using the existing mouseOverComposer helper (or model.mouseOverComposer(y)) to find the first y that returns true, then pass that hitRow in the tea.MouseMsg to m.Update; keep the rest of the assertions the same so the test no longer depends on a magic row number.internal/tui/onboarding.go (1)
797-805: ⚡ Quick winConsider allowing spaces in provider names.
The current filtering prevents users from entering multi-word names like "OpenAI Production" or "My Custom Provider". While "short label" suggests conciseness, it doesn't require single-word names. The CLI already handles spaces via
setupCommandArgquoting (line 371), and the config stores names as plain strings.🔧 Proposed fix to allow spaces
func (m *model) appendSetupName(runes []rune) { for _, r := range runes { - if r < 32 || r == 127 || unicode.IsSpace(r) { + if r < 32 || r == 127 { continue } m.setup.name += string(r) } m.setup.err = "" }🤖 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/onboarding.go` around lines 797 - 805, The appendSetupName function currently rejects any unicode.IsSpace characters which prevents multi-word provider names; update appendSetupName to allow normal space characters (e.g., r == ' ') while still filtering out control chars (r < 32) and DEL (r == 127) — keep the logic that skips other whitespace categories if desired, or simply remove the unicode.IsSpace check and only exclude r < 32 and r == 127; ensure this change remains compatible with setupCommandArg usage elsewhere (so names containing spaces are preserved and stored as plain strings).
🤖 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/providers/openai/provider_test.go`:
- Around line 210-227: Add a new unit test to cover mixed reasoning and regular
content by creating a test (e.g.,
TestStreamCompletionEmitsReasoningBeforeRegularContent) that uses the same test
harness (newTestProvider, writeSSE) to emit a single SSE chunk containing both
"reasoning_content" and "content" in the same delta, then collect events via
collectProviderEvents and assert there are two text events with the first
event's Content equal to the reasoning text and the second equal to the regular
content; this verifies the provider emits reasoning_content before regular
content without changing the existing
TestStreamCompletionEmitsReasoningContentDeltas test.
In `@internal/tui/onboarding.go`:
- Around line 797-805: The appendSetupName function currently rejects any
unicode.IsSpace characters which prevents multi-word provider names; update
appendSetupName to allow normal space characters (e.g., r == ' ') while still
filtering out control chars (r < 32) and DEL (r == 127) — keep the logic that
skips other whitespace categories if desired, or simply remove the
unicode.IsSpace check and only exclude r < 32 and r == 127; ensure this change
remains compatible with setupCommandArg usage elsewhere (so names containing
spaces are preserved and stored as plain strings).
In `@internal/tui/scroll_test.go`:
- Around line 68-75: The test only checks the composer cursor but must also
assert that the wheel event changed chat scrolling; before calling m.Update
record the starting chatScrollOffset (e.g., startOffset := m.chatScrollOffset),
then after the update and casting updated.(model) assert that
next.chatScrollOffset != startOffset (or assert the expected delta direction if
known) to prove the wheel was routed to chat scrolling rather than swallowed.
- Around line 35-56: The test
TestMouseWheelOverWrappedComposerMovesComposerCursor hard-codes Y:14 which is
layout-coupled; instead compute a Y that actually hits the composer by deriving
it from the model. After creating m and setting sizes, pick a hitRow by scanning
y from 0..m.height-1 and using the existing mouseOverComposer helper (or
model.mouseOverComposer(y)) to find the first y that returns true, then pass
that hitRow in the tea.MouseMsg to m.Update; keep the rest of the assertions the
same so the test no longer depends on a magic row number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 35a14bd7-c181-4118-b73d-211795eead84
📒 Files selected for processing (22)
internal/cli/setup.gointernal/cli/setup_test.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/types.gointernal/tui/autocomplete.gointernal/tui/autocomplete_test.gointernal/tui/command_center.gointernal/tui/composer.gointernal/tui/composer_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/mouse.gointernal/tui/onboarding.gointernal/tui/onboarding_test.gointernal/tui/options.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_discovery.gointernal/tui/provider_wizard_test.gointernal/tui/rendering_lime_test.gointernal/tui/scroll_test.gointernal/tui/view.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: APPROVED
Findings: none blocking. I reviewed the current head (81930bc) across the reasoning-content streaming changes, custom provider onboarding/wizard persistence, provider display normalization, composer paste-preview/autocomplete behavior, and mouse-wheel composer hit-testing. The earlier CodeRabbit concerns around non-local HTTP endpoints and clipped-footer wheel hit-testing are addressed in this head.
Validation performed locally:
git diff --check origin/main...HEADgo test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smoke
Remote status is also green: CI smoke checks, Performance Smoke, Zero Review, and CodeRabbit are passing.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review: model status + reasoning streams
Nice polish overall — I reviewed the streaming, composer/mouse, and onboarding/wizard layers with verification. Build is clean and go test ./... passes. The composer and onboarding work is well-built and safe (details below). Requesting changes for one streaming issue; the rest are minor/nits.
🟠 Should fix — reasoning content leaks into persisted message history
internal/providers/openai/provider.go:232 emits reasoning_content deltas as StreamEventText:
if choice.Delta.ReasoningContent != "" {
sendEvent(ctx, events, zeroruntime.StreamEvent{Type: zeroruntime.StreamEventText, Content: choice.Delta.ReasoningContent})
}CollectStream folds every StreamEventText into collected.Text (internal/zeroruntime/helpers.go:113), and the agent loop then uses collected.Text as the persisted assistant message content (internal/agent/loop.go:210, replayed to the model on every subsequent turn) and as the final answer (loop.go:247).
This diverges from the repo's own contract for the other two provider adapters, which keep reasoning out of the answer/message text:
- the gemini adapter explicitly skips thought parts —
// Skip thought summary parts: their text is reasoning, not the answer.(internal/providers/gemini/provider.go:240); - the anthropic adapter routes thinking to
ReasoningBlocks, surfaced on the done event, never intocollected.Text.
There's already a dedicated channel for this — collected.ReasoningBlocks / Message.Reasoning (loop.go:214) — which this adapter bypasses.
Consequence: on multi-turn / tool-using runs (the normal agent path), the model's chain-of-thought is re-sent as committed assistant content every turn (token bloat + the model treating its scratch reasoning as prior output), and the saved answer becomes reasoning + answer concatenated.
Fix: show reasoning live, but route it through a separate event/channel so it does not enter collected.Text / the persisted message Content — mirror the gemini (skip) or anthropic (ReasoningBlocks, Provider: "openai") pattern. The new TestStreamCompletionEmitsReasoningContentDeltas asserts the concatenated behavior, so it should be updated to assert reasoning does not land in the answer text (and add a mixed reasoning_content+content delta case).
🟡 Minor — onboarding/setup
- Endpoint validation accepts embedded credentials and private/metadata hosts over https (
internal/tui/provider_wizard.go:~989,providerWizardEndpointError):https://user:pass@host/v1andhttps://169.254.169.254/v1both pass. Discovery is safely skipped for custom endpoints (no SSRF) and the provider label strips userinfo (no leak there), but auser:pass@base URL is persisted in plaintext and sent at request time. Recommend rejectingparsed.User != nil. - CLI
zero setup <provider> --base-url <url>skips the wizard's validation (internal/cli/setup.go): an invalid/ill-formed URL is written to config and only fails later at request time. Share the wizard's validation (orproviderio.NormalizeBaseURL) on this path.
Nits
- Dead code after the paste-preview refactor:
deleteCompletedFileMentionBefore,deleteComposerLineBefore,deleteComposerLineAfter(internal/tui/composer.go) have no callers — delete them. mouseOverComposer(internal/tui/mouse.go:163) only boundsmsg.Y, notmsg.X, so a wheel scroll far left/right of the composer box still moves the cursor; it also locates the composer row by matching the rendered string within the footer (brittle). Bound X and/or expose the row offset directly.
Verified safe (high-value checks)
- No SSRF during onboarding discovery — custom (user-supplied-URL) providers skip discovery entirely; built-in providers discover against their hardcoded default base URL.
- No credential leak via the provider display label — derived from
parsed.Hostname(), which strips anyuser:pass@. - Composer is panic/rune-safe — cursor math is on
[]runewith clamps throughout; the collapsed paste preview keeps the full bytes for submission and the cursor can't land inside a hidden preview (verified by stress testing — no panics, no buffer corruption). - Config writes preserve other config (read-merge-write, atomic temp+rename, key file 0600); CLI override precedence (
--name/--base-urlwin over saved) is correct.
Verdict: Solid work — composer and onboarding are well-built and the SSRF/credential-leak checks come out clean. The one item worth fixing before merge is the reasoning-into-history leak, since it contradicts how the two sibling provider adapters in this repo handle thinking; the rest can be follow-ups. Happy to re-review quickly.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review — reasoning leak fixed ✅
Thanks for 38da8f3. I verified the fix end-to-end:
- New dedicated
StreamEventReasoningtype (with a comment that it must never be folded into answer text or persisted as assistant content). - The OpenAI adapter now emits
reasoning_contentasStreamEventReasoning, notStreamEventText. CollectStreamWithOptionsroutes it to a newOnReasoningcallback and never appends it tocollected.Text— so it can no longer reach the persisted messageContent(loop.go:210) or the final answer (loop.go:247).OnReasoningis wired through agentOptionsinto the TUI, so reasoning is still shown live (the feature) while staying out of history — matching the gemini (skip) / anthropic (ReasoningBlocks) contract.- Good regression coverage:
TestRunDoesNotPersistReasoningAsAssistantTextasserts answer-onlyFinalAnswer/messageContentwith reasoning delivered viaOnReasoning, plus provider tests for "no text events" and reasoning-before-content ordering.
gofmt/go vet clean, go test ./... passes. Approving.
The earlier minors are fine as follow-ups whenever convenient: reject user:pass@ / validate scheme in the endpoint check (and share that validation with the zero setup --base-url CLI path), drop the now-unused composer helpers (deleteCompletedFileMentionBefore, deleteComposerLineBefore/After), and bound mouseOverComposer on msg.X. Nice work.
3 participants
Summary:
Notes:
Tests:
Summary by CodeRabbit
New Features
UI
Behavior
Tests