refactor: extract terminal and assistant boundaries#102
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR replaces alias-based assistant and provider types with package-owned LLM adapters, moves context budgeting and compaction into shared packages, centralizes lifecycle payload building, and refactors the terminal UI onto transcript, rendertext, panel, input, and extui abstractions. ChangesBoundary refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
+ Coverage 75.79% 77.40% +1.61%
==========================================
Files 235 259 +24
Lines 20547 21268 +721
==========================================
+ Hits 15574 16463 +889
+ Misses 3775 3601 -174
- Partials 1198 1204 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/assistant/context_compaction.go (1)
70-85:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRe-plan before honoring
FirstKeptEntryIDmutations.
compactionSummaryDecisioncan return a hook-selectedFirstKeptEntryID, but this block only patchesplan.FirstKeptEntryIDafter the core summary and the derived plan state were already built from the old split point. If an extension moves the cut later without also supplying a summary, the saved summary no longer matches the actual retained tail; even when the hook does provide a summary, the persisted counts andplan.FileOperationsstay stale.Please apply the mutation before summarization and recompute the derived plan fields, or have the before-compaction path return an already-adjusted
*compaction.Plan. A regression test where the hook only overridesFirstKeptEntryIDwould catch this.🤖 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/assistant/context_compaction.go` around lines 70 - 85, The hook-returned FirstKeptEntryID from compactionSummaryDecision must be applied before building the summary-derived plan so counts and FileOperations are consistent: change the flow so compactionSummaryDecision either returns an already-adjusted *compaction.Plan or, immediately after receiving (summary, decision, fromHook) but before computing derived plan fields, set plan.FirstKeptEntryID = decision.FirstKeptEntryID (when non-empty) and recompute any derived fields on that plan (e.g., persisted counts, FileOperations) so the subsequent call to runtime.appendCompaction receives a plan that matches the actual retained tail; update compactionSummaryDecision callers (and add a regression test where the hook only overrides FirstKeptEntryID) to verify correctness.internal/compaction/file_operations.go (1)
266-278:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't ignore
WriteStringresults in a file covered byerrcheck.This block drops every
strings.Builder.WriteStringreturn value. The repo guideline says to handle all error returns explicitly, and this helper can avoid error-returning APIs entirely by building lines andstrings.Joining once.Suggested rewrite
- builder := strings.Builder{} - builder.WriteString(strings.TrimSpace(summary)) - builder.WriteString("\n\n") - builder.WriteString(fileOperationsHeader) - for _, operation := range operations[:min(len(operations), maxFileOperations)] { - builder.WriteString("\n- ") - builder.WriteString(operation.Action) - builder.WriteString(": ") - builder.WriteString(operation.Path) - if operation.Tool != "" { - builder.WriteString(" (via ") - builder.WriteString(operation.Tool) - builder.WriteString(")") - } - } - - return strings.TrimSpace(builder.String()) + lines := []string{strings.TrimSpace(summary), "", fileOperationsHeader} + for _, operation := range operations[:min(len(operations), maxFileOperations)] { + line := "- " + operation.Action + ": " + operation.Path + if operation.Tool != "" { + line += " (via " + operation.Tool + ")" + } + lines = append(lines, line) + } + + return strings.TrimSpace(strings.Join(lines, "\n"))As per coding guidelines, "Never ignore errors; handle all error returns explicitly. The
errchecklinter withcheck-blank: trueis enabled and enforced."🤖 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/compaction/file_operations.go` around lines 266 - 278, The code block uses strings.Builder.WriteString calls and discards their returned errors (builder := strings.Builder{}, builder.WriteString(...)) which violates the errcheck rule; replace the incremental WriteString usage with a non-error-returning approach: build a slice of strings (e.g., parts) and append each line (including summary, fileOperationsHeader and each formatted operation using operation.Action, operation.Path, operation.Tool, and the maxFileOperations/min logic) then produce the final output with strings.Join(parts, "") or strings.Join(parts, "\n\n") so no WriteString return values are ignored and the logic around operations[:min(len(operations), maxFileOperations)] remains intact.Source: Coding guidelines
internal/terminal/render_runtime.go (1)
118-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset clears from column 0 instead of the window origin.
At Line 121,
clearWindowignoreswindow.X, so a reset for an offset window can wipe unrelated content on the left side of the frame.Proposed fix
func clearWindow(target rendertext.ContentSetter, window *extension.WindowState) { style := tcell.StyleDefault for row := 0; row < window.Height; row++ { - writeLine(target, window.Y+row, window.Width, "", style) + writeTextAt(target, window.X, window.Y+row, window.Width, "", style) } }🤖 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/terminal/render_runtime.go` around lines 118 - 122, clearWindow currently ignores window.X and clears from column 0, wiping content left of the window; update the writeLine call inside clearWindow to start at window.X and only clear window.Width columns (use the window.X origin from extension.WindowState when invoking writeLine for each row) so the reset affects only the window region rather than from column 0.
🧹 Nitpick comments (3)
internal/contextwindow/contributions.go (1)
81-92: ⚡ Quick winDocument the 1-indexed consecutive key requirement.
The
numericMapValuesfunction assumes numeric map keys are consecutive starting from"1". If a key is missing (e.g.,{"1": x, "3": y}without"2"), the function returns an empty slice. This behavior may be correct, but it's subtle and deserves a comment explaining the requirement.📝 Suggested documentation improvement
func numericMapValues(values map[string]any) []any { + // Returns an ordered list by parsing consecutive 1-indexed string keys ("1", "2", ...). + // Returns empty slice if any key in the sequence is missing. items := make([]any, 0, len(values)) for index := 1; index <= len(values); index++ {🤖 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/contextwindow/contributions.go` around lines 81 - 92, The numericMapValues function assumes map keys are numeric strings starting at "1" and consecutive (1-indexed) and returns an empty slice if any index is missing; add a clear doc comment above numericMapValues describing this requirement and its current behavior (including example like {"1":a,"3":b} -> returns empty slice) so callers understand the precondition and the function's fallback when a gap exists.internal/terminal/panel/model_test.go (1)
14-118: ⚡ Quick winConvert core panel behavior coverage to table-driven tests.
These cases are solid, but consolidating filter/selection/key/render scenarios into table-driven form will make regression expansion easier and reduce repetition.
As per coding guidelines,
**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs.🤖 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/terminal/panel/model_test.go` around lines 14 - 118, Several related unit tests (TestModelFiltersByAllItemFields, TestModelSelectionWrapsAndClamps, TestModelHandleKeyUsesInjectedBindings, TestModelRenderShowsSelectionHintsAndPosition) are repetitive and should be converted into table-driven tests; refactor these to a single table-driven test (or grouped table-driven subtests) that enumerates scenarios (filtering via AppendQueryRune, selection movement via MoveSelection/SetSelectedIndex, key handling via HandleKey with testBindings, and rendering via Render with RenderOptions) with expected outputs for FilteredItems, SelectedIndex/SelectedValue/SelectedItem, returned Action from HandleKey, and rendered lines, so new cases can be added by appending rows to the table rather than duplicating test functions.Source: Coding guidelines
internal/terminal/input/key.go (1)
52-77: 💤 Low valueConsider hoisting
specialKeysto a package-level variable.The
specialKeys()function allocates a new map on every call toComposerKeyEvent. Hoisting it to a package-levelvarwould avoid repeated allocations.However, the current approach is clearer and avoids any potential shared-state concurrency issues, so this is purely an optional optimization.
♻️ Optional optimization
-func specialKeys() map[tcell.Key]string { - return map[tcell.Key]string{ +var specialKeys = map[tcell.Key]string{ tcell.KeyEscape: "escape", tcell.KeyEnter: "enter", // ... rest of map - } }Then update line 19:
- key, ok := specialKeys()[event.Key()] + key, ok := specialKeys[event.Key()]🤖 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/terminal/input/key.go` around lines 52 - 77, Hoist the per-call allocation by replacing the specialKeys() function with a package-level read-only variable (e.g., specialKeysMap) initialized once with the same tcell.Key->string entries, and update any callers such as ComposerKeyEvent to use that variable instead of calling specialKeys(); because the map is only read after init it is safe for concurrent use, and this avoids repeated allocations while preserving behavior.
🤖 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/assistant/lifecyclepayload/lifecyclepayload.go`:
- Around line 361-373: CompactionPreparation currently dereferences plan
unconditionally and will panic if plan is nil; update CompactionPreparation to
guard for a nil plan and return a safe payload (at minimum include CWDKey and
SessionIDKey and empty/nil placeholders for the compaction-specific fields) so
building lifecycle payloads never panics, and also ensure call sites that pass
saved.Plan (the place that forwards saved.Plan into CompactionPreparation)
either check saved.Plan != nil before calling or rely on the new nil-safe
CompactionPreparation behavior.
In `@internal/assistant/provider_diagnostics_internal_test.go`:
- Around line 32-36: The subtest captures the range variable address which races
when using t.Parallel(); avoid taking &testCase directly. Inside the loop,
shadow the range variable (e.g., tc := testCase) and pass its address to
runProviderHookDiagnosticsTest (call runProviderHookDiagnosticsTest(t, &tc)) and
use tc in the t.Run closure so each parallel subtest gets a unique copy instead
of the range variable address.
In `@internal/compaction/plan.go`:
- Around line 456-458: The token estimator function countRunesAsTokens currently
uses byte length via len(strings.TrimSpace(text)), which miscounts multibyte
Unicode characters; update countRunesAsTokens to count Unicode runes instead
(e.g., use utf8.RuneCountInString or convert to a rune slice after trimming) so
the function returns the actual rune count for compaction boundaries and import
utf8 if needed.
In `@internal/provider/hooks.go`:
- Around line 41-48: Guard against a nil hook result before dereferencing
mutated: after calling request.OnProviderRequest(ctx, hookInput) check whether
mutated is nil (in addition to err) and, if so, return an appropriate
provider-scoped error (matching the existing pattern used for errors from
OnProviderRequest) instead of accessing mutated.Payload or mutated.Headers;
update the HookOutput return logic to only read mutated.Payload and
mutated.Headers when mutated != nil.
In `@internal/provider/provider_tool_calls_test.go`:
- Around line 101-102: The test currently calls providerResponseView(result)
before verifying the operation succeeded, which can panic and obscure the real
error; change the test to call require.NoError(t, err) immediately after the
operation that returns err, then call providerResponseView(result) only after
that assertion; update both places that call providerResponseView(result) (the
occurrences around the require.NoError checks) so the error is asserted first
and conversion happens afterward.
In `@internal/provider/tool_loop.go`:
- Around line 42-45: The error returned from request.ExecuteTools should be
wrapped with provider context using samber/oops before returning; locate the
call in function (or method) that invokes request.ExecuteTools with
toolCallsToLLM(calls) and replace the raw return of err with an oops wrapper
such as oops.In("provider").Code("tool_exec_failed").Wrapf(err, "failed to
execute tools for calls=%v", calls) (or similar message) so the returned error
retains domain and code context per the package's error style.
In `@internal/terminal/rendertext/text.go`:
- Around line 214-216: The top-border fill calculation uses RuneLen(suffix)
which miscomputes visible width for wide or combining glyphs; update the
calculation in the block creating the labeled top border to call Width(suffix)
instead of RuneLen(suffix) (i.e., replace RuneLen(suffix) with Width(suffix)
when computing fillWidth using innerWidth and suffix) so the repeat count
correctly reflects terminal display width; ensure the Width function is the one
used elsewhere in this package and adjust imports if necessary.
---
Outside diff comments:
In `@internal/assistant/context_compaction.go`:
- Around line 70-85: The hook-returned FirstKeptEntryID from
compactionSummaryDecision must be applied before building the summary-derived
plan so counts and FileOperations are consistent: change the flow so
compactionSummaryDecision either returns an already-adjusted *compaction.Plan
or, immediately after receiving (summary, decision, fromHook) but before
computing derived plan fields, set plan.FirstKeptEntryID =
decision.FirstKeptEntryID (when non-empty) and recompute any derived fields on
that plan (e.g., persisted counts, FileOperations) so the subsequent call to
runtime.appendCompaction receives a plan that matches the actual retained tail;
update compactionSummaryDecision callers (and add a regression test where the
hook only overrides FirstKeptEntryID) to verify correctness.
In `@internal/compaction/file_operations.go`:
- Around line 266-278: The code block uses strings.Builder.WriteString calls and
discards their returned errors (builder := strings.Builder{},
builder.WriteString(...)) which violates the errcheck rule; replace the
incremental WriteString usage with a non-error-returning approach: build a slice
of strings (e.g., parts) and append each line (including summary,
fileOperationsHeader and each formatted operation using operation.Action,
operation.Path, operation.Tool, and the maxFileOperations/min logic) then
produce the final output with strings.Join(parts, "") or strings.Join(parts,
"\n\n") so no WriteString return values are ignored and the logic around
operations[:min(len(operations), maxFileOperations)] remains intact.
In `@internal/terminal/render_runtime.go`:
- Around line 118-122: clearWindow currently ignores window.X and clears from
column 0, wiping content left of the window; update the writeLine call inside
clearWindow to start at window.X and only clear window.Width columns (use the
window.X origin from extension.WindowState when invoking writeLine for each row)
so the reset affects only the window region rather than from column 0.
---
Nitpick comments:
In `@internal/contextwindow/contributions.go`:
- Around line 81-92: The numericMapValues function assumes map keys are numeric
strings starting at "1" and consecutive (1-indexed) and returns an empty slice
if any index is missing; add a clear doc comment above numericMapValues
describing this requirement and its current behavior (including example like
{"1":a,"3":b} -> returns empty slice) so callers understand the precondition and
the function's fallback when a gap exists.
In `@internal/terminal/input/key.go`:
- Around line 52-77: Hoist the per-call allocation by replacing the
specialKeys() function with a package-level read-only variable (e.g.,
specialKeysMap) initialized once with the same tcell.Key->string entries, and
update any callers such as ComposerKeyEvent to use that variable instead of
calling specialKeys(); because the map is only read after init it is safe for
concurrent use, and this avoids repeated allocations while preserving behavior.
In `@internal/terminal/panel/model_test.go`:
- Around line 14-118: Several related unit tests
(TestModelFiltersByAllItemFields, TestModelSelectionWrapsAndClamps,
TestModelHandleKeyUsesInjectedBindings,
TestModelRenderShowsSelectionHintsAndPosition) are repetitive and should be
converted into table-driven tests; refactor these to a single table-driven test
(or grouped table-driven subtests) that enumerates scenarios (filtering via
AppendQueryRune, selection movement via MoveSelection/SetSelectedIndex, key
handling via HandleKey with testBindings, and rendering via Render with
RenderOptions) with expected outputs for FilteredItems,
SelectedIndex/SelectedValue/SelectedItem, returned Action from HandleKey, and
rendered lines, so new cases can be added by appending rows to the table rather
than duplicating test functions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de61690c-38a3-45b9-a723-4a0000957751
📒 Files selected for processing (196)
docs/refactoring/terminal-assistant-boundary-plan-2026-06-09.mdinternal/assistant/anthropic.gointernal/assistant/client.gointernal/assistant/client_adapter.gointernal/assistant/context_auto_compaction.gointernal/assistant/context_budget.gointernal/assistant/context_build.gointernal/assistant/context_compaction.gointernal/assistant/context_compaction_lifecycle.gointernal/assistant/context_compaction_lifecycle_internal_test.gointernal/assistant/context_contributors.gointernal/assistant/context_usage_led.gointernal/assistant/export_test.gointernal/assistant/lifecycle.gointernal/assistant/lifecycle_diagnostics.gointernal/assistant/lifecyclepayload/lifecyclepayload.gointernal/assistant/lifecyclepayload/lifecyclepayload_test.gointernal/assistant/llm_conversion.gointernal/assistant/llm_conversion_behavior_internal_test.gointernal/assistant/llm_conversion_internal_test.gointernal/assistant/maps.gointernal/assistant/provider_diagnostics_internal_test.gointernal/assistant/provider_hook_test_helpers_internal_test.gointernal/assistant/provider_hooks.gointernal/assistant/provider_hooks_internal_test.gointernal/assistant/runtime.gointernal/assistant/runtime_entries.gointernal/assistant/runtime_events.gointernal/assistant/runtime_events_test.gointernal/assistant/runtime_model.gointernal/assistant/runtime_persist.gointernal/assistant/runtime_test.gointernal/assistant/stream_events.gointernal/assistant/stream_events_internal_test.gointernal/assistant/test_message_helpers_internal_test.gointernal/assistant/token_estimate.gointernal/assistant/tool_executor.gointernal/assistant/tool_schema_internal_test.gointernal/assistant/usage_events.gointernal/compaction/doc.gointernal/compaction/file_operations.gointernal/compaction/file_operations_internal_test.gointernal/compaction/plan.gointernal/compaction/plan_internal_test.gointernal/contextwindow/budget.gointernal/contextwindow/budget_internal_test.gointernal/contextwindow/contributions.gointernal/contextwindow/contributions_internal_test.gointernal/contextwindow/doc.gointernal/contextwindow/test_helpers_internal_test.gointernal/contextwindow/tokens.gointernal/contextwindow/types.gointernal/contextwindow/usage.gointernal/contextwindow/usage_internal_test.gointernal/contextwindow/usage_led_internal_test.gointernal/llm/doc.gointernal/llm/llm_test.gointernal/llm/provider_types.gointernal/llm/test_helpers_test.gointernal/llm/tools.gointernal/llm/types.gointernal/provider/anthropic.gointernal/provider/anthropic_internal_test.gointernal/provider/anthropic_mapping_internal_test.gointernal/provider/anthropic_tools_test.gointernal/provider/client.gointernal/provider/client_test.gointernal/provider/empty_response_internal_test.gointernal/provider/events.gointernal/provider/hooks.gointernal/provider/http.gointernal/provider/messages.gointernal/provider/messages_internal_test.gointernal/provider/openai_chat.gointernal/provider/openai_chat_payload_internal_test.gointernal/provider/openai_responses.gointernal/provider/openai_responses_internal_test.gointernal/provider/provider_payload_hook_internal_test.gointernal/provider/provider_tool_calls_test.gointernal/provider/result.gointernal/provider/sse.gointernal/provider/test_helpers_internal_test.gointernal/provider/text_tool_calls.gointernal/provider/text_tool_calls_fields_internal_test.gointernal/provider/tool_loop.gointernal/provider/tool_loop_test.gointernal/provider/tool_registry_internal_test.gointernal/provider/tool_schema.gointernal/provider/tool_schema_internal_test.gointernal/provider/usage.gointernal/provider/usage_merge_internal_test.gointernal/provider/usage_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/async_events_test.gointernal/terminal/auth_commands.gointernal/terminal/auth_commands_internal_test.gointernal/terminal/autocomplete.gointernal/terminal/autocomplete_test.gointernal/terminal/bench_terminal_test.gointernal/terminal/cell_buffer.gointernal/terminal/clipboard_test.gointernal/terminal/code_highlight.gointernal/terminal/commands.gointernal/terminal/commands_internal_test.gointernal/terminal/compact_commands_test.gointernal/terminal/composer.gointernal/terminal/composer_buffer.gointernal/terminal/context_commands.gointernal/terminal/editor.gointernal/terminal/editor_test.gointernal/terminal/extension_events.gointernal/terminal/extension_events_test.gointernal/terminal/extui/doc.gointernal/terminal/extui/state.gointernal/terminal/extui/state_test.gointernal/terminal/focus.gointernal/terminal/focus_test.gointernal/terminal/input.gointernal/terminal/input/buffer.gointernal/terminal/input/buffer_test.gointernal/terminal/input/doc.gointernal/terminal/input/editor.gointernal/terminal/input/editor_test.gointernal/terminal/input/key.gointernal/terminal/input/key_test.gointernal/terminal/input/render.gointernal/terminal/input/render_test.gointernal/terminal/input_escape.gointernal/terminal/interrupt_test.gointernal/terminal/markdown.gointernal/terminal/markdown_table.gointernal/terminal/markdown_test.gointernal/terminal/message_layout.gointernal/terminal/message_line_cache.gointernal/terminal/message_render.gointernal/terminal/panel.gointernal/terminal/panel/input.gointernal/terminal/panel/model.gointernal/terminal/panel/model_test.gointernal/terminal/panel_actions.gointernal/terminal/panel_actions_test.gointernal/terminal/panel_keybindings.gointernal/terminal/panel_model.gointernal/terminal/panel_model_test.gointernal/terminal/panel_scoped_models.gointernal/terminal/panel_scoped_models_test.gointernal/terminal/panel_session.gointernal/terminal/panel_session_test.gointernal/terminal/panel_settings.gointernal/terminal/panel_settings_selection_test.gointernal/terminal/panel_settings_test.gointernal/terminal/panel_tree.gointernal/terminal/panel_tree_test.gointernal/terminal/panels.gointernal/terminal/prompt_cancel.gointernal/terminal/prompt_cancel_test.gointernal/terminal/prompt_history.gointernal/terminal/prompt_history_test.gointernal/terminal/prompt_queue.gointernal/terminal/prompt_queue_test.gointernal/terminal/prompt_response.gointernal/terminal/prompt_response_test.gointernal/terminal/prompt_send.gointernal/terminal/prompt_send_test.gointernal/terminal/prompt_submit.gointernal/terminal/render.gointernal/terminal/render_composer.gointernal/terminal/render_layout.gointernal/terminal/render_lines.gointernal/terminal/render_loader.gointernal/terminal/render_messages.gointernal/terminal/render_panel.gointernal/terminal/render_parity_test.gointernal/terminal/render_runtime.gointernal/terminal/render_test.gointernal/terminal/rendertext/buffer.gointernal/terminal/rendertext/doc.gointernal/terminal/rendertext/line.gointernal/terminal/rendertext/text.gointernal/terminal/rendertext/text_test.gointernal/terminal/runtime_buffers.gointernal/terminal/scoped_models.gointernal/terminal/selection.gointernal/terminal/session_panel.gointernal/terminal/session_setting_actions_test.gointernal/terminal/text.gointernal/terminal/text_format.gointernal/terminal/token_usage_export_test.gointernal/terminal/tool_blocks.gointernal/terminal/ui_text.gointernal/terminal/welcome.gointernal/transcript/role.gointernal/transcript/role_test.gointernal/transcript/tool_event.gointernal/transcript/tool_event_test.go
💤 Files with no reviewable changes (11)
- internal/terminal/panels.go
- internal/terminal/cell_buffer.go
- internal/assistant/context_contributors.go
- internal/assistant/token_estimate.go
- internal/terminal/editor_test.go
- internal/assistant/context_usage_led.go
- internal/terminal/text.go
- internal/terminal/editor.go
- internal/assistant/stream_events.go
- internal/terminal/ui_text.go
- internal/assistant/maps.go
omarluq
force-pushed
the
feat/transcript-boundary
branch
from
3569cb7 to
1f49eba
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/assistant/runtime_events.go`:
- Around line 14-25: Remove the redundant Payload: nil entry from the
lifecyclepayload.ProviderRequest construction passed to runtime.emit in
internal/assistant/runtime_events.go; update the literal used with
lifecyclepayload.ProviderRequest (the object built for
lifecyclepayload.ProviderRequestPayload) by deleting the Payload field so the
code relies on ProviderRequestPayload's mapsutil.CloneOrEmpty behavior, keeping
all other fields (Headers, API, ModelID, Provider, SessionID, ThinkingLevel,
Attempt) unchanged.
In `@internal/terminal/keyevent/keyevent.go`:
- Around line 11-15: The Rune function currently dereferences event without
checking for nil which can panic; add a nil-safety guard at the top of Rune
(check if event == nil) and return a zero rune (e.g., rune(0)) immediately if
nil, then proceed to call utf8.DecodeRuneInString(event.Str()) as before; update
the Rune function to perform this check to prevent panics when event is nil.
In `@internal/terminal/panel/input.go`:
- Around line 80-91: In Model.handleSearchKey, add a nil-check for the event
parameter before using its methods to avoid panics when event is nil;
specifically, at the start of handleSearchKey (the function defined as func
(model *Model) handleSearchKey(event *tcell.EventKey)), return early if event ==
nil (similar to the existing model nil-check) so calls to event.Key(),
keyevent.Rune(event), and the subsequent calls to BackspaceQuery() and
AppendQueryRune(...) are only executed when event is non-nil.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7471f4d5-f4bd-4cc3-b812-c593bc458b3b
📒 Files selected for processing (192)
internal/assistant/anthropic.gointernal/assistant/client.gointernal/assistant/client_adapter.gointernal/assistant/client_adapter_internal_test.gointernal/assistant/context_auto_compaction.gointernal/assistant/context_auto_compaction_clients_test.gointernal/assistant/context_auto_compaction_internal_extra_test.gointernal/assistant/context_auto_compaction_test.gointernal/assistant/context_budget.gointernal/assistant/context_budget_test.gointernal/assistant/context_build.gointernal/assistant/context_build_test.gointernal/assistant/context_compaction.gointernal/assistant/context_compaction_lifecycle.gointernal/assistant/context_compaction_lifecycle_internal_test.gointernal/assistant/context_compaction_lifecycle_test.gointernal/assistant/context_compaction_test.gointernal/assistant/context_contributors.gointernal/assistant/context_contributors_test.gointernal/assistant/context_overflow_compaction_test.gointernal/assistant/context_usage_led.gointernal/assistant/export_test.gointernal/assistant/lifecycle.gointernal/assistant/lifecycle_diagnostics.gointernal/assistant/lifecyclepayload/behavior_test.gointernal/assistant/lifecyclepayload/lifecyclepayload.gointernal/assistant/lifecyclepayload/lifecyclepayload_test.gointernal/assistant/llm_conversion.gointernal/assistant/llm_conversion_behavior_internal_test.gointernal/assistant/llm_conversion_internal_test.gointernal/assistant/maps.gointernal/assistant/provider_diagnostics_internal_test.gointernal/assistant/provider_hook_test_helpers_internal_test.gointernal/assistant/provider_hooks.gointernal/assistant/provider_hooks_internal_test.gointernal/assistant/provider_seam_hardening_test.gointernal/assistant/runtime.gointernal/assistant/runtime_entries.gointernal/assistant/runtime_events.gointernal/assistant/runtime_events_test.gointernal/assistant/runtime_lifecycle_test.gointernal/assistant/runtime_model.gointernal/assistant/runtime_test.gointernal/assistant/stream_events.gointernal/assistant/stream_events_internal_test.gointernal/assistant/test_message_helpers_internal_test.gointernal/assistant/token_estimate.gointernal/assistant/tool_diagnostics_test.gointernal/assistant/tool_executor.gointernal/assistant/tool_lifecycle_test.gointernal/assistant/tool_registry_test.gointernal/assistant/tool_schema_internal_test.gointernal/assistant/usage_events.gointernal/compaction/doc.gointernal/compaction/file_operations.gointernal/compaction/file_operations_internal_test.gointernal/compaction/plan.gointernal/compaction/plan_internal_test.gointernal/contextwindow/budget.gointernal/contextwindow/budget_internal_test.gointernal/contextwindow/contributions.gointernal/contextwindow/contributions_internal_test.gointernal/contextwindow/doc.gointernal/contextwindow/test_helpers_internal_test.gointernal/contextwindow/tokens.gointernal/contextwindow/types.gointernal/contextwindow/usage.gointernal/contextwindow/usage_internal_test.gointernal/contextwindow/usage_led_internal_test.gointernal/llm/clone.gointernal/llm/doc.gointernal/llm/llm_test.gointernal/llm/provider_types.gointernal/llm/test_helpers_test.gointernal/llm/tools.gointernal/llm/types.gointernal/model/clone.gointernal/provider/anthropic.gointernal/provider/anthropic_internal_test.gointernal/provider/anthropic_mapping_internal_test.gointernal/provider/anthropic_tools_test.gointernal/provider/client.gointernal/provider/client_test.gointernal/provider/empty_response_internal_test.gointernal/provider/events.gointernal/provider/events_internal_test.gointernal/provider/hooks.gointernal/provider/http.gointernal/provider/messages.gointernal/provider/messages_internal_test.gointernal/provider/openai_chat.gointernal/provider/openai_chat_payload_internal_test.gointernal/provider/openai_responses.gointernal/provider/openai_responses_internal_test.gointernal/provider/provider_payload_hook_internal_test.gointernal/provider/provider_tool_calls_test.gointernal/provider/result.gointernal/provider/sse.gointernal/provider/test_helpers_internal_test.gointernal/provider/text_tool_calls.gointernal/provider/text_tool_calls_fields_internal_test.gointernal/provider/tool_loop.gointernal/provider/tool_loop_test.gointernal/provider/tool_registry_internal_test.gointernal/provider/tool_schema.gointernal/provider/tool_schema_internal_test.gointernal/provider/usage.gointernal/provider/usage_merge_internal_test.gointernal/provider/usage_test.gointernal/terminal/app.gointernal/terminal/auth_commands.gointernal/terminal/auth_commands_internal_test.gointernal/terminal/autocomplete.gointernal/terminal/autocomplete_test.gointernal/terminal/commands.gointernal/terminal/compact_commands_test.gointernal/terminal/composer.gointernal/terminal/composer_buffer.gointernal/terminal/editor.gointernal/terminal/editor_test.gointernal/terminal/extension_events.gointernal/terminal/extension_events_test.gointernal/terminal/extui/doc.gointernal/terminal/extui/state.gointernal/terminal/extui/state_behavior_test.gointernal/terminal/extui/state_test.gointernal/terminal/focus.gointernal/terminal/focus_test.gointernal/terminal/input.gointernal/terminal/input/buffer.gointernal/terminal/input/buffer_test.gointernal/terminal/input/doc.gointernal/terminal/input/editing_behavior_test.gointernal/terminal/input/editor.gointernal/terminal/input/editor_test.gointernal/terminal/input/key.gointernal/terminal/input/key_test.gointernal/terminal/input/render.gointernal/terminal/input/render_test.gointernal/terminal/input_escape.gointernal/terminal/interrupt_test.gointernal/terminal/keybindings.gointernal/terminal/keyevent/keyevent.gointernal/terminal/message_render.gointernal/terminal/panel.gointernal/terminal/panel/input.gointernal/terminal/panel/input_behavior_test.gointernal/terminal/panel/model.gointernal/terminal/panel/model_test.gointernal/terminal/panel_actions.gointernal/terminal/panel_actions_test.gointernal/terminal/panel_keybindings.gointernal/terminal/panel_model.gointernal/terminal/panel_model_test.gointernal/terminal/panel_scoped_models.gointernal/terminal/panel_scoped_models_test.gointernal/terminal/panel_session.gointernal/terminal/panel_session_test.gointernal/terminal/panel_settings.gointernal/terminal/panel_settings_selection_test.gointernal/terminal/panel_settings_test.gointernal/terminal/panel_tree.gointernal/terminal/panel_tree_test.gointernal/terminal/panels.gointernal/terminal/prompt_cancel.gointernal/terminal/prompt_history.gointernal/terminal/prompt_history_test.gointernal/terminal/prompt_queue.gointernal/terminal/prompt_queue_test.gointernal/terminal/prompt_send_test.gointernal/terminal/prompt_submit.gointernal/terminal/render.gointernal/terminal/render_composer.gointernal/terminal/render_layout.gointernal/terminal/render_messages.gointernal/terminal/render_panel.gointernal/terminal/render_parity_test.gointernal/terminal/render_runtime.gointernal/terminal/render_test.gointernal/terminal/rendertext/behavior_test.gointernal/terminal/rendertext/format.gointernal/terminal/rendertext/renderer_screen_stub_test.gointernal/terminal/rendertext/test_helpers_test.gointernal/terminal/rendertext/text.gointernal/terminal/rendertext/text_test.gointernal/terminal/runtime_buffers.gointernal/terminal/scoped_models.gointernal/terminal/session_panel.gointernal/terminal/session_setting_actions_test.gointernal/terminal/tcell_v3.gointernal/terminal/token_usage.gointernal/terminal/tool_blocks.go
💤 Files with no reviewable changes (8)
- internal/assistant/maps.go
- internal/terminal/panels.go
- internal/terminal/editor.go
- internal/assistant/context_usage_led.go
- internal/terminal/editor_test.go
- internal/assistant/stream_events.go
- internal/assistant/token_estimate.go
- internal/assistant/context_contributors.go
✅ Files skipped from review due to trivial changes (12)
- internal/terminal/extui/doc.go
- internal/model/clone.go
- internal/llm/clone.go
- internal/assistant/context_contributors_test.go
- internal/compaction/doc.go
- internal/terminal/input/doc.go
- internal/provider/text_tool_calls_fields_internal_test.go
- internal/llm/doc.go
- internal/terminal/prompt_history_test.go
- internal/terminal/compact_commands_test.go
- internal/assistant/runtime_entries.go
- internal/contextwindow/doc.go
🚧 Files skipped from review as they are similar to previous changes (114)
- internal/assistant/test_message_helpers_internal_test.go
- internal/assistant/provider_hook_test_helpers_internal_test.go
- internal/terminal/input/render_test.go
- internal/contextwindow/test_helpers_internal_test.go
- internal/terminal/input/key_test.go
- internal/terminal/commands.go
- internal/terminal/panel_settings_selection_test.go
- internal/llm/tools.go
- internal/assistant/runtime_events_test.go
- internal/provider/tool_registry_internal_test.go
- internal/terminal/prompt_submit.go
- internal/terminal/panel_scoped_models_test.go
- internal/llm/test_helpers_test.go
- internal/terminal/panel_scoped_models.go
- internal/provider/openai_responses_internal_test.go
- internal/contextwindow/tokens.go
- internal/terminal/auth_commands_internal_test.go
- internal/terminal/panel_session_test.go
- internal/provider/http.go
- internal/assistant/llm_conversion_behavior_internal_test.go
- internal/provider/usage_merge_internal_test.go
- internal/terminal/autocomplete_test.go
- internal/terminal/prompt_cancel.go
- internal/terminal/panel_actions.go
- internal/llm/types.go
- internal/provider/usage.go
- internal/provider/empty_response_internal_test.go
- internal/provider/anthropic_tools_test.go
- internal/terminal/panel_tree_test.go
- internal/terminal/panel_model_test.go
- internal/terminal/input/buffer_test.go
- internal/assistant/stream_events_internal_test.go
- internal/terminal/panel_tree.go
- internal/terminal/prompt_history.go
- internal/assistant/context_budget.go
- internal/terminal/input/editor_test.go
- internal/terminal/input/buffer.go
- internal/terminal/render_panel.go
- internal/contextwindow/contributions_internal_test.go
- internal/terminal/input/editor.go
- internal/terminal/interrupt_test.go
- internal/provider/events.go
- internal/terminal/input/key.go
- internal/terminal/render_parity_test.go
- internal/assistant/context_compaction_lifecycle_internal_test.go
- internal/llm/provider_types.go
- internal/assistant/runtime_model.go
- internal/terminal/extui/state_test.go
- internal/assistant/tool_schema_internal_test.go
- internal/provider/client_test.go
- internal/provider/sse.go
- internal/terminal/render_messages.go
- internal/contextwindow/usage_led_internal_test.go
- internal/provider/openai_chat_payload_internal_test.go
- internal/llm/llm_test.go
- internal/terminal/panel.go
- internal/terminal/panel_actions_test.go
- internal/contextwindow/types.go
- internal/provider/anthropic_internal_test.go
- internal/terminal/panel_settings.go
- internal/provider/messages.go
- internal/terminal/composer.go
- internal/assistant/anthropic.go
- internal/provider/provider_tool_calls_test.go
- internal/compaction/file_operations.go
- internal/terminal/message_render.go
- internal/assistant/lifecycle_diagnostics.go
- internal/terminal/extension_events_test.go
- internal/assistant/provider_hooks_internal_test.go
- internal/assistant/usage_events.go
- internal/terminal/app.go
- internal/assistant/provider_hooks.go
- internal/provider/tool_loop.go
- internal/compaction/file_operations_internal_test.go
- internal/terminal/panel_session.go
- internal/assistant/client_adapter.go
- internal/terminal/prompt_send_test.go
- internal/terminal/render_layout.go
- internal/terminal/render.go
- internal/terminal/panel_model.go
- internal/assistant/context_compaction_lifecycle.go
- internal/provider/tool_schema_internal_test.go
- internal/provider/hooks.go
- internal/provider/openai_chat.go
- internal/terminal/autocomplete.go
- internal/provider/provider_payload_hook_internal_test.go
- internal/provider/openai_responses.go
- internal/provider/messages_internal_test.go
- internal/provider/result.go
- internal/assistant/tool_executor.go
- internal/assistant/llm_conversion_internal_test.go
- internal/compaction/plan_internal_test.go
- internal/provider/anthropic.go
- internal/assistant/llm_conversion.go
- internal/assistant/lifecyclepayload/lifecyclepayload_test.go
- internal/terminal/render_composer.go
- internal/provider/test_helpers_internal_test.go
- internal/terminal/input.go
- internal/terminal/focus_test.go
- internal/contextwindow/usage.go
- internal/terminal/panel/model.go
- internal/assistant/export_test.go
- internal/assistant/lifecycle.go
- internal/provider/usage_test.go
- internal/terminal/panel_keybindings.go
- internal/provider/client.go
- internal/assistant/lifecyclepayload/lifecyclepayload.go
- internal/contextwindow/contributions.go
- internal/terminal/composer_buffer.go
- internal/terminal/extension_events.go
- internal/assistant/context_build.go
- internal/contextwindow/usage_internal_test.go
- internal/provider/tool_schema.go
- internal/compaction/plan.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/assistant/context_compaction.go`:
- Around line 95-103: If a decision replaces plan, recompute plan.FileOperations
from the updated plan starting at plan.FirstKeptEntryIndex before calling
runtime.compactionSummary so file-operation context isn't lost; specifically,
after any assignment that overrides plan and before the call to
runtime.compactionSummary(...) re-derive the file operations from the plan
entries slice beginning at plan.FirstKeptEntryIndex (e.g., regenerate
FileOperations from plan.Entries[plan.FirstKeptEntryIndex:]) and set
plan.FileOperations accordingly so the summary/append flows see the correct
operations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27ab013c-321a-497e-b489-a4a5c0332fb6
📒 Files selected for processing (27)
internal/assistant/context_compaction.gointernal/assistant/lifecyclepayload/lifecyclepayload.gointernal/assistant/llm_conversion.gointernal/assistant/runtime_context.gointernal/assistant/runtime_events.gointernal/compaction/plan.gointernal/contextwindow/usage.gointernal/contextwindow/usage_convert.gointernal/llm/llm_test.gointernal/llm/provider_types.gointernal/llm/request.gointernal/llm/usage_merge.gointernal/model/message_filter.gointernal/model/messages.gointernal/provider/anthropic_mapping_internal_test.gointernal/provider/client.gointernal/provider/message_fixtures_internal_test.gointernal/provider/messages_internal_test.gointernal/provider/openai_chat_payload_internal_test.gointernal/provider/usage.gointernal/terminal/extui/state.gointernal/terminal/input/buffer.gointernal/terminal/keyevent/keyevent.gointernal/terminal/keyevent/keyevent_test.gointernal/terminal/panel/input.gointernal/terminal/panel/input_behavior_test.gointernal/terminal/panel/model.go
✅ Files skipped from review due to trivial changes (2)
- internal/provider/message_fixtures_internal_test.go
- internal/terminal/keyevent/keyevent.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/llm/provider_types.go
- internal/assistant/runtime_events.go
- internal/provider/openai_chat_payload_internal_test.go
- internal/provider/anthropic_mapping_internal_test.go
- internal/contextwindow/usage.go
- internal/terminal/input/buffer.go
- internal/terminal/panel/input.go
- internal/assistant/llm_conversion.go
- internal/terminal/panel/input_behavior_test.go
- internal/provider/usage.go
- internal/assistant/lifecyclepayload/lifecyclepayload.go
|
This was referenced Jun 12, 2026
Merged
Merged
This was referenced Jun 16, 2026
Merged
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
1 participant
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.
Summary
Validation