polish v2: smoothness & delight#484
Conversation
Classify a failed turn's provider error into auth / rate-limit / connectivity / model-not-found / context-overflow and render a one-line next step below the red error row, instead of dumping an identical raw blob for every failure mode. The classifier lives in a new internal/errhint package so both the TUI error row and the CLI exec path can share it (TUIHint references slash commands, CLIHint references zero subcommands). Classification is a conservative string heuristic keyed off providerio.ClassifiedError's prefixes plus lower-level DNS/TLS/timeout/context-length signatures, since the numeric HTTP status is gone by the time the error reaches a UI surface.
bash was the one tool that returned command output with no byte cap — 'cat large.log' or a verbose test run could dump megabytes into context and force compaction, while every other read/search tool already applies a budget. Cap stdout and stderr at 96KB each, keeping the head and tail of an oversized stream (build/test failures usually land at the tail) and dropping the middle behind a marker that points at redirect-to-file + read_file. Record raw_bytes/emitted_bytes/estimated_tokens/truncated in Meta like the other tools. Sandbox-denial and shell-issue detection still run on the full raw output before budgeting.
|
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 error hinting and Gemini OAuth retry wiring, TUI transcript and streaming updates, command and export flows, picker ranking, bash output truncation, agent compaction and guardrail changes, and reconnect/backoff adjustments. ChangesCross-cutting runtime and UI updates
Runtime output and agent control
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/errhint/errhint.go`:
- Around line 35-60: The Classify function is matching overly broad substrings,
which can mislabel local failures as provider errors. Tighten the checks in
Classify by either restricting matches to known provider error patterns or
adding a provider-origin gate before returning Auth, RateLimit, ContextOverflow,
ModelNotFound, or Connectivity. Focus on the containsAny cases in Classify and
make sure hints are only attached when the error clearly comes from the
provider.
🪄 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
Run ID: e1f5d682-7e37-41a7-8f37-63907617ad0b
📒 Files selected for processing (6)
internal/errhint/errhint.gointernal/errhint/errhint_test.gointernal/tui/error_hint_test.gointernal/tui/model.gointernal/tui/rendering.gointernal/tui/transcript.go
The Gemini provider used providerio.SendWithRetry (429/503/529 only), so an expired OAuth token surfaced as a terminal 'API key not valid' auth error while OpenAI and Anthropic silently recovered via SendWithAuthRetry's force-refresh-and-replay on 401. Plumb OAuthResolver through gemini.Options/Provider and the factory (exactly as the other two providers already do) and switch the stream send to SendWithAuthRetry. With a nil resolver (API-key users) behavior is unchanged.
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/tools/bash.go`:
- Around line 160-181: The bash result handling in Result construction is
dropping truncation state even though budgetBashOutput marks meta["truncated"]
as true. Update the return paths in the bash execution flow in
internal/tools/bash.go so Result.Truncated is set whenever output is truncated,
including both the error and success cases; use the existing budgetBashOutput
and formatBashOutputWithShellHint paths to locate all Result literals and either
copy the meta flag into the struct or have budgetBashOutput return the
truncation boolean directly.
🪄 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
Run ID: 4cc89ca1-e27c-4c20-8162-b106238a5925
📒 Files selected for processing (5)
internal/providers/factory.gointernal/providers/gemini/provider.gointernal/providers/gemini/provider_test.gointernal/tools/bash.gointernal/tools/bash_budget_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/tools/bash_budget_test.go
Address CodeRabbit review on #1: agentResponseMsg.err can also carry local failures (a tool's 'permission denied', a 'file does not exist', a config error), and broad substrings would have attached a bogus /provider or /model hint to those. Classify now returns Unknown unless the message carries a provider marker the provider layer always attaches (auth error: / rate limit error: / provider error: / provider request error: / provider stream error:). Local errors never draw a provider hint; provider errors sub-classify as before. Added local-failure test cases proving they stay Unknown.
zero exec printed provider failures as a bare '[zero] <raw error>' with no next step. Append a one-line hint (reusing the shared errhint classifier) for recognized provider failures — 'run `zero auth`', 'run `zero doctor`', etc. The provider-origin gate means non-provider codes (sandbox_error, mcp_error) and JSON/stream-json output are untouched.
Address CodeRabbit review on #3: budgetBashOutput recorded truncation in meta["truncated"] but never set the Result.Truncated struct field, so consumers reading the struct (like glob/web_fetch/read_minified do) would miss truncated bash results. Return the bool from budgetBashOutput and set Result.Truncated on every return path.
runAgentWithOptions built a fresh lsp.NewManager per run and shut it down when the run returned, so gopls (and every other language server) cold-started on the first edit of every turn — 200ms-2s of latency the manager's own design (long-lived, reused servers) exists to avoid. Build one manager per session in newModel (cheap: servers start lazily on first Check) and reuse it across runs; the SelfCorrector still gets a fresh checker wrapping the shared manager each run. Torn down with a short deadline in quit(). Runs fall back to a per-run manager when the session manager is absent (cwd unknown, or a directly-built test model).
m.streamingText += msg.delta was O(len) per delta -> O(n²) across a long generation (a 10k-token code gen allocates hundreds of MB of intermediate strings and the UI gets progressively laggier). Accumulate into a []byte with append (O(1) amortized) and read via streamingTextString(). A []byte rather than strings.Builder because the TUI model is copied by value on every Update, which would trip strings.Builder's copy check; []byte is nil-safe and copies cleanly like the other slice fields.
Every OnText delta was its own tea.Msg, so a fast provider (100+ tok/s) drove 100+ full Update->View cycles per second, each re-parsing the growing markdown — visible stutter over SSH and wasted CPU. Batch agentTextMsg deltas at the runtime sink over a ~16ms frame and forward them as a single message, decoupling render rate from token rate. Any non-text message flushes pending text first so ordering with tool-call / reasoning / row messages is preserved; the final agentResponseMsg (a tea.Cmd return, not a sink message) is safe because the model already drops deltas for an inactive runID.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/tui/model.go (1)
268-274: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider applying the same byte-buffer treatment to
streamingReasoning.
streamingReasoning(line 274) still accumulates via+=on a plainstring(seeagentReasoningMsghandling), which reintroduces the exact O(n²) growth pattern this PR just eliminated forstreamingText. Long chain-of-thought traces from reasoning-heavy models could hit the same wall-time overhead this cohort is meant to fix.♻️ Sketch of the same pattern applied to reasoning
- streamingReasoning string // live provider reasoning for the current segment + streamingReasoning []byte // live provider reasoning for the current segment; see streamingReasoningString()Not blocking this PR, but worth a follow-up given the precedent just set here.
🤖 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 268 - 274, The `streamingReasoning` field in `internal/tui/model.go` still grows via repeated string concatenation in the `agentReasoningMsg` flow, which brings back the same O(n²) accumulation problem already fixed for `streamingText`. Apply the same byte-buffer approach used for `streamingText`: change `streamingReasoning` to a `[]byte`, add a helper for reading it back as a string, and update the reasoning append path to append bytes instead of using `+=`. Use the existing `streamingText`/`streamingTextString()` pattern as the reference for where to wire this in.internal/tui/coalesce_test.go (1)
37-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPotential CI flakiness: the "still buffered" assertion depends on wall-clock timing.
sendarms a real 16mstime.AfterFuncon the first delta. Line 42'slen(got) != 0check assumes the timer hasn't fired yet — usually true, but a GC/scheduler stall >16ms between the sends andsnapshot()will flush early and fail the assertion nondeterministically. Consider making the coalesce interval injectable (or the timer a test seam) so this path is deterministic.🤖 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/coalesce_test.go` around lines 37 - 44, The coalescing test in coalesce_test.go is timing-dependent because agentTextMsg buffering in send() relies on a real time.AfterFunc, so the “still buffered” assertion can fail nondeterministically. Make the coalesce interval or timer behavior injectable through the coalescer/send path and use that test seam in the test so the snapshot assertion is deterministic. Reference the send method and the buffered-delta assertion in the coalesce test when wiring the fix.
🤖 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/coalesce.go`:
- Around line 41-80: The forwarding path in textCoalescer can race because send
and flush both drain under c.mu but call c.forward after unlocking, allowing
buffered text to overtake a concurrent non-text message. Update
textCoalescer.send and textCoalescer.flush to serialize all forward calls with a
separate lock or equivalent ordering guard, and apply it around the
drain+forward flow so agentTextMsg output from the timer or run switch cannot be
delivered after the intervening non-text msg.
---
Nitpick comments:
In `@internal/tui/coalesce_test.go`:
- Around line 37-44: The coalescing test in coalesce_test.go is timing-dependent
because agentTextMsg buffering in send() relies on a real time.AfterFunc, so the
“still buffered” assertion can fail nondeterministically. Make the coalesce
interval or timer behavior injectable through the coalescer/send path and use
that test seam in the test so the snapshot assertion is deterministic. Reference
the send method and the buffered-delta assertion in the coalesce test when
wiring the fix.
In `@internal/tui/model.go`:
- Around line 268-274: The `streamingReasoning` field in `internal/tui/model.go`
still grows via repeated string concatenation in the `agentReasoningMsg` flow,
which brings back the same O(n²) accumulation problem already fixed for
`streamingText`. Apply the same byte-buffer approach used for `streamingText`:
change `streamingReasoning` to a `[]byte`, add a helper for reading it back as a
string, and update the reasoning append path to append bytes instead of using
`+=`. Use the existing `streamingText`/`streamingTextString()` pattern as the
reference for where to wire this in.
🪄 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
Run ID: f493d773-b06e-4728-a786-91a3af324212
📒 Files selected for processing (10)
internal/tui/coalesce.gointernal/tui/coalesce_test.gointernal/tui/flush_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/rendering_lime_test.gointernal/tui/run.gointernal/tui/syntax_highlight_test.gointernal/tui/transcript_selection.gointernal/tui/working_status_test.go
guardrails.go halted a run after 4 consecutive same-error tool failures. A corrective hint fires at 2, and a model iterating on a genuinely tricky edit can legitimately fail a couple more times after the hint while converging — stopping at 4 cut those runs short. Raise to 6; the streak still resets the instant the tool succeeds or hits a different error, so only true same-error loops are affected.
…rrors #19: bump maxStreamReconnects 2->4 and add up-to-50% jitter on the exponential backoff (capped at 8s), so a multi-second network blip is ridden out instead of killing the run on a 2s hiccup, and concurrent runs (swarms, cron fleets) don't reconnect in lockstep. #34: drop the '502'/'503' substring matches from shouldReconnect. 503 already exhausted providerio.SendWithRetry (retrying here is a redundant double-retry) and 502 is non-idempotent by providerio's rule (the POST may have been processed). Only genuine transport failures — where no response was received — reconnect now.
A no-output stream stall is detected only after the full stream idle timeout (~5min) elapses, and each retry can idle again — so 2 retries left an interactive session frozen for ~15min. Drop to 1 retry: keeps the common single-hiccup recovery while bounding the worst case to ~2x the idle timeout.
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/agent/reconnect.go (1)
110-129: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBlock HTTP 504 before the generic timeout match.
Line 112 says 504 must not be retried, but Line 123 still makes
"504 Gateway Timeout"reconnectable via"timeout". Add an HTTP-status guard before the transport substring loop, then update the test expectation.Proposed fix
msg := strings.ToLower(err.Error()) if isContextLimitError(msg) || isImageRejectionError(err) { return false } + for _, needle := range []string{ + "500 internal server error", + "502 bad gateway", + "503 service unavailable", + "504 gateway timeout", + "http 500", + "http 502", + "http 503", + "http 504", + "status 500", + "status 502", + "status 503", + "status 504", + } { + if strings.Contains(msg, needle) { + return false + } + } // Transport-level disconnects only. HTTP 5xx statuses are deliberately NOT🤖 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/agent/reconnect.go` around lines 110 - 129, Block HTTP 504 explicitly before the transport substring checks in reconnect logic, since the generic timeout match in the reconnect matcher can still incorrectly treat “504 Gateway Timeout” as reconnectable. Update the reconnect decision path in the reconnect helper so it short-circuits on 504 before the substring loop in the function that scans the needles, then adjust the associated test expectations to assert 504 is not retried.
🤖 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/agent/reconnect.go`:
- Around line 110-129: Block HTTP 504 explicitly before the transport substring
checks in reconnect logic, since the generic timeout match in the reconnect
matcher can still incorrectly treat “504 Gateway Timeout” as reconnectable.
Update the reconnect decision path in the reconnect helper so it short-circuits
on 504 before the substring loop in the function that scans the needles, then
adjust the associated test expectations to assert 504 is not retried.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05be15ad-178a-4e8c-91e1-e96681c6b673
📒 Files selected for processing (3)
internal/agent/guardrails.gointernal/agent/reconnect.gointernal/agent/reconnect_test.go
Address CodeRabbit review on #7: flush() and the run-switch path drained the text buffer under c.mu but called forward() after releasing it, so a timer-fired text flush could race a concurrent non-text send() and land after the tool-call/reasoning message it should precede. Hold c.mu across drain AND forward (drainAndForwardLocked), so whoever holds the lock delivers atomically and the other caller blocks until it is done — text can never overtake a following non-text message. Added a concurrency stress test (timer racing an inline send) that passes under -race.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Went through all ten commits and they hold up. Each one's a real thing: bash was the only tool without an output cap, Gemini was the only OAuth provider that couldn't refresh an expired token (you'd get a dead "API key not valid" while OpenAI/Anthropic silently retry), streamingText really was doing O(n²) string appends, and the per-run LSP manager cold-started gopls on the first edit of every prompt. One commit per item, each with a test, so it's easy to reason about.
A few small things, none blocking:
- The 16ms text coalescer calls
forward()outside the mutex, so in theory the flush timer and a non-text emit can reorder and render a trailing narration fragment after the tool card it should sit before. The window is nanoseconds wide and the committed transcript comes fromresult.FinalAnswer, not the streamed buffer, so it's cosmetic — worth a code comment, not worth holding this. - The bash commit message says shell-issue detection runs on the raw pre-truncation output, but it actually runs on the budgeted text. Harmless (head+tail keeps the first/last error lines the detector looks for), just an inaccurate note.
- The "N bytes omitted" marker undercounts slightly. Cosmetic.
Approving — good pass.
Bare /compact already triggers compaction, but /compact now — what users reach for when the context gauge climbs — hit the usage-error branch. Accept the 'now' keyword and advertise it in the command usage.
/retry resends the last prompt; /edit recalls it into the composer to tweak and resend — both read a new lastPrompt field captured verbatim (pre-expansion) in launchPrompt. /copy puts the last answer on the clipboard via the existing copy machinery; /export writes a plain role-prefixed transcript to a file (timestamped default, or a given path). Real gaps for SSH users with no mouse select.
tool_search ranked deferred tools by exact substring, so a model that
typed 'webfetch' (dropping the underscore) matched nothing and looped.
Add a separator-squashing fallback ('web_fetch' -> 'webfetch') scored
below an exact substring match, so precise queries still rank first.
# Conflicts: # internal/tui/commands.go
Empty answers all rendered as '(no answer provided)', so the model couldn't tell a wholesale dismissal (user closed the prompt without answering anything) from one field left blank amid real answers — and might invent a default. FormatAskUserAnswers now flags a full dismissal up front as a skip and marks individual empties '(left blank)' vs '(skipped)'.
The pickers filtered by flat strings.Contains with no ranking, so finding 'sonnet 4.5' among 50+ models meant an exact prefix or scrolling. Rank matches (exact < prefix < contains < subsequence, reusing fuzzySubsequenceGap) so the closest match lands on top, and 'snt45' now matches 'Sonnet 4.5'. Groups stay contiguous — ordered by their best match, never split — so the grouped model picker still renders one header per provider. Covers both /model and /resume (shared commandPicker).
partitionTools re-ran the recursive schema->map conversion (schemaToRuntimeMap) for every tool on every turn — redundant, since a tool's advertised name/description/schema is stable for the run. Add a per-run definition cache keyed by tool name (nil-safe; the plain partitionTools entrypoint and tests pass nil for fresh renders). The partitioning itself (visibility, deferral, ordering) still recomputes every turn — it must, because a tool's deferred state can flip mid-run (swarm tools un-defer once a swarm is active). Only the expensive schema render is memoized. tool_search is excluded by its callers (dynamic description) so it never poisons the cache.
The byte/4 heuristic (ApproxTextTokens) over-counts code-heavy content ~15-20%, so with triggerRatio=0.7 compaction fired at ~60% of true capacity — premature summarizer calls that degrade quality. Each turn now folds (rawEstimate, provider InputTokens) into an EMA calibration ratio, clamped per-sample to a sane band; maybeCompact and the reactive path scale their estimates by it. Turn 1 uses the raw estimate (no data yet); later turns compact near real capacity.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providers/factory.go (1)
25-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc: OAuthResolver now covers Gemini too.
Line 27 still says the resolver is "Applied to the OpenAI and Anthropic providers", but this very PR wires it into the
config.ProviderKindGooglebranch (line 94). Future maintainers will read this as Gemini being excluded. Fold Google in.📝 Proposed doc fix
// OAuthResolver, when set, lets the provider authenticate model calls with an // OAuth bearer token (preferred over the API key). nil => API-key auth only. - // Applied to the OpenAI and Anthropic providers. + // Applied to the OpenAI, Anthropic, and Gemini providers. OAuthResolver providerio.TokenResolver🤖 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/factory.go` around lines 25 - 28, The comment on OAuthResolver is stale because it still says the token resolver is applied only to OpenAI and Anthropic, but the factory now also uses it for Google/Gemini via the config.ProviderKindGoogle branch. Update the documentation on OAuthResolver in factory.go to explicitly include Google/Gemini alongside OpenAI and Anthropic, keeping the wording aligned with the provider selection logic in the provider factory.
🧹 Nitpick comments (3)
internal/tui/lsp_manager_test.go (1)
25-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing coverage for the non-nil shutdown path.
Only the nil-manager guard is tested. Consider adding a case with a real
lspManager(e.g. fromnewModelwith aCwd) to exerciseshutdownLSPManager's actualShutdown(ctx)call and timeout behavior.🤖 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/lsp_manager_test.go` around lines 25 - 31, Add test coverage for the non-nil shutdown path in shutdownLSPManager, not just the zero-value model guard. Extend TestShutdownLSPManagerNilSafe or add a new test that creates a real model via newModel with a valid Cwd, ensures lspManager is initialized, and then calls shutdownLSPManager to verify the underlying Shutdown(ctx) path is exercised. Include assertions that cover the timeout behavior and that the shutdown completes without panicking or leaking errors when lspManager is present.internal/tools/bash_budget_test.go (1)
29-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a stderr-only truncation case.
Current tests only exercise oversized stdout; no test verifies
truncatedflips true when only stderr exceeds the budget (errTruncbranch of the||). Logic is trivial, but a quick added case would close the gap.🤖 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/tools/bash_budget_test.go` around lines 29 - 61, Add a stderr-only truncation test for budgetBashOutput to cover the errTrunc branch of the truncation logic. Extend TestBudgetBashOutputTruncatesHeadAndTail or add a new test that passes a small stdout value and an oversized stderr value, then assert truncated is true, the stderr output is budgeted/truncated as expected, and the meta fields reflect truncation and emitted/raw byte counts correctly.internal/tui/retry_edit_test.go (1)
10-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing positive-path coverage for
/retry.Tests cover the no-op path (no prior prompt) and
/editrecall, but nothing exercises/retryactually resendingm.lastPromptvialaunchPrompt(the happy path shown in the downstreammodel.gohandler). Worth adding a test that setslastPrompt, sends/retry, and asserts a run is launched with that prompt.🤖 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/retry_edit_test.go` around lines 10 - 46, Add positive-path test coverage for the `/retry` command in the retry/edit tests: alongside TestRetryWithoutPriorPromptIsNoOp and TestEditRecallsLastPrompt, add a case that sets model.lastPrompt, enters `/retry`, and verifies the model handler actually resends that prompt by invoking launchPrompt rather than producing the no-op transcript. Use the existing newModel, Update, and model command handling path to assert the launched run reflects the saved lastPrompt.
🤖 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/agent/loop.go`:
- Around line 177-179: The calibration sample is being taken from an early
request snapshot, so `requestTokenEstimate` should not be reused across a
reactive compaction retry. Update `internal/agent/loop.go` so the estimate is
recomputed from the final `messages`/`exposed` state immediately before calling
`compactor.calibrate(...)`, using the logic around `estimateTokens` and
`estimateToolDefTokens` rather than the pre-retry captured value. This ensures
the calibration input matches the request that actually completed and keeps the
EMA aligned with the final prompt size.
In `@internal/errhint/errhint.go`:
- Around line 61-77: The numeric status-code checks in errhint.go are too broad
because containsAny() matches bare substrings like 401, 403, 429, and 529, which
can misclassify unrelated provider messages. Update the logic in the errhint
matcher so these status codes are only recognized with boundary-aware matching
or separated into a dedicated status-code check, while keeping the existing Auth
and RateLimit keyword handling in the errhint classifier intact.
In `@internal/tools/bash.go`:
- Around line 384-397: The truncation message built in truncateHeadTail has a
stray closing bracket at the end of the marker string. Update the marker text in
truncateHeadTail so it is grammatically consistent and does not end with an
unmatched bracket, keeping the output shown by bash clean and readable.
In `@internal/tui/model.go`:
- Around line 3929-3938: The `commandRetry` path in `model.go` is missing the
same launch guards used by `commandPrompt`, so `/retry` can start a run while
`m.compactInFlight` or `m.exiting` is true. Update the `commandRetry` handling
to mirror the `commandPrompt` checks before calling
`m.launchPrompt(m.lastPrompt)`, and return a system transcript message when
retry is blocked due to compaction or shutdown in progress.
In `@internal/tui/transcript_export.go`:
- Around line 60-81: The export path in handleExportCommand is writing
transcripts with overly permissive file mode, which can expose sensitive session
contents to other local users. Update the os.WriteFile call in
model.handleExportCommand to use a user-only permission mode instead of
world/group-readable permissions, while keeping the existing path resolution and
error handling unchanged. Refer to handleExportCommand and its os.WriteFile
invocation when making the change.
---
Outside diff comments:
In `@internal/providers/factory.go`:
- Around line 25-28: The comment on OAuthResolver is stale because it still says
the token resolver is applied only to OpenAI and Anthropic, but the factory now
also uses it for Google/Gemini via the config.ProviderKindGoogle branch. Update
the documentation on OAuthResolver in factory.go to explicitly include
Google/Gemini alongside OpenAI and Anthropic, keeping the wording aligned with
the provider selection logic in the provider factory.
---
Nitpick comments:
In `@internal/tools/bash_budget_test.go`:
- Around line 29-61: Add a stderr-only truncation test for budgetBashOutput to
cover the errTrunc branch of the truncation logic. Extend
TestBudgetBashOutputTruncatesHeadAndTail or add a new test that passes a small
stdout value and an oversized stderr value, then assert truncated is true, the
stderr output is budgeted/truncated as expected, and the meta fields reflect
truncation and emitted/raw byte counts correctly.
In `@internal/tui/lsp_manager_test.go`:
- Around line 25-31: Add test coverage for the non-nil shutdown path in
shutdownLSPManager, not just the zero-value model guard. Extend
TestShutdownLSPManagerNilSafe or add a new test that creates a real model via
newModel with a valid Cwd, ensures lspManager is initialized, and then calls
shutdownLSPManager to verify the underlying Shutdown(ctx) path is exercised.
Include assertions that cover the timeout behavior and that the shutdown
completes without panicking or leaking errors when lspManager is present.
In `@internal/tui/retry_edit_test.go`:
- Around line 10-46: Add positive-path test coverage for the `/retry` command in
the retry/edit tests: alongside TestRetryWithoutPriorPromptIsNoOp and
TestEditRecallsLastPrompt, add a case that sets model.lastPrompt, enters
`/retry`, and verifies the model handler actually resends that prompt by
invoking launchPrompt rather than producing the no-op transcript. Use the
existing newModel, Update, and model command handling path to assert the
launched run reflects the saved lastPrompt.
🪄 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
Run ID: 5f0bf2a0-5d7b-4e89-9774-7d59e39fbab5
📒 Files selected for processing (45)
internal/agent/compaction.gointernal/agent/compaction_calibrate_test.gointernal/agent/compaction_preserve.gointernal/agent/compaction_recent_edits_test.gointernal/agent/guardrails.gointernal/agent/loop.gointernal/agent/partition_cache_test.gointernal/agent/plan_staleness_test.gointernal/agent/reconnect.gointernal/agent/reconnect_test.gointernal/cli/exec.gointernal/cli/exec_provider_hint_test.gointernal/errhint/errhint.gointernal/errhint/errhint_test.gointernal/providers/factory.gointernal/providers/gemini/provider.gointernal/providers/gemini/provider_test.gointernal/tools/ask_user.gointernal/tools/ask_user_format_test.gointernal/tools/bash.gointernal/tools/bash_budget_test.gointernal/tools/tool_search.gointernal/tools/tool_search_test.gointernal/tui/coalesce.gointernal/tui/coalesce_test.gointernal/tui/commands.gointernal/tui/compact_now_test.gointernal/tui/error_hint_test.gointernal/tui/flush_test.gointernal/tui/lsp_manager_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/picker.gointernal/tui/picker_fuzzy_test.gointernal/tui/rendering.gointernal/tui/rendering_lime_test.gointernal/tui/retry_edit_test.gointernal/tui/run.gointernal/tui/session_controls.gointernal/tui/syntax_highlight_test.gointernal/tui/transcript.gointernal/tui/transcript_export.gointernal/tui/transcript_export_test.gointernal/tui/transcript_selection.gointernal/tui/working_status_test.go
- loop.go: recompute the compaction calibration estimate at calibration
time, not request-build time, so a reactive-compaction reissue that
shrinks the request feeds the EMA the sample that actually completed.
- errhint: match bare HTTP status codes (401/403/429/529) only at digit
boundaries, so an incidental number ('4290ms', 'id 14015') isn't
mis-bucketed as auth/rate-limit.
- bash: drop the stray unmatched ']' in the output-truncation marker.
- tui /retry: apply the same exiting/compactInFlight guards commandPrompt
has, so a retry can't race compactResultMsg's rewrite of session state.
- tui /export: write the transcript 0o600 (it may contain echoed
secrets), not world/group-readable 0o644.
os.WriteFile ignores Unix permission bits on Windows and Stat reports 0666, so the 0o600 assertion failed there. The production 0o600 is honored on Linux/macOS (where it matters); guard the check with runtime.GOOS so Windows smoke passes.
|
@coderabbitai review the commits that u havent checked before |
|
✅ Action performedReview 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/tui/image_attach.go (1)
302-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor grammar: "1 staged image(s)" reads awkwardly for singular count.
When exactly one image is staged, the message renders as
⚠ 1 staged image(s) will be dropped — ..., which looks like unfinished pluralization logic in a user-facing warning.✏️ Optional pluralization fix
func (m model) visionDropWarning() string { if len(m.pendingImages) == 0 || m.modelSupportsVisionTUI() { return "" } + noun := "images" + if len(m.pendingImages) == 1 { + noun = "image" + } - return fmt.Sprintf("⚠ %d staged image(s) will be dropped — %s has no vision support.", - len(m.pendingImages), displayValue(m.modelName, "the active model")) + return fmt.Sprintf("⚠ %d staged %s will be dropped — %s has no vision support.", + len(m.pendingImages), noun, displayValue(m.modelName, "the active model")) }🤖 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/image_attach.go` around lines 302 - 313, The warning text in visionDropWarning uses awkward pluralization for a single staged image, so update the message formatting to choose singular vs plural based on len(m.pendingImages). Keep the existing logic in model.visionDropWarning and displayValue, but change the user-facing string so it renders naturally for 1 image and multiple images without “image(s)”.
🤖 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/session.go`:
- Around line 394-401: Windows workspace matching should not fail when the same
path differs only by casing. Update sessionMatchesWorkspace to compare the
cleaned sessionCwd and workspaceCwd case-insensitively on Windows, or normalize
the stored cwd in the session creation path so both values use the same casing
before matching. Use the sessionMatchesWorkspace helper and the cwd values set
from options.Cwd/os.Getwd() to locate the fix.
- Around line 374-388: The latest resumable session shortcut is too permissive
because latestResumableInWorkspace() only checks workspace membership and can
return entries that newSessionPicker() would hide. Update
latestResumableInWorkspace() in model to apply the same resumable filters used
by the picker, skipping zero-event and empty/failed runs before returning a
match, so /resume latest stays aligned with the picker behavior.
---
Nitpick comments:
In `@internal/tui/image_attach.go`:
- Around line 302-313: The warning text in visionDropWarning uses awkward
pluralization for a single staged image, so update the message formatting to
choose singular vs plural based on len(m.pendingImages). Keep the existing logic
in model.visionDropWarning and displayValue, but change the user-facing string
so it renders naturally for 1 image and multiple images without “image(s)”.
🪄 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
Run ID: afe4ca59-9ec0-4322-aa9e-6c02e08a06eb
📒 Files selected for processing (15)
internal/agent/loop.gointernal/errhint/errhint.gointernal/errhint/errhint_test.gointernal/tools/bash.gointernal/tui/command_center.gointernal/tui/image_attach.gointernal/tui/model.gointernal/tui/onboarding.gointernal/tui/onboarding_affordance_test.gointernal/tui/resume_scope_test.gointernal/tui/retry_edit_test.gointernal/tui/session.gointernal/tui/transcript_export.gointernal/tui/transcript_export_test.gointernal/tui/vision_drop_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/tui/vision_drop_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/tui/transcript_export_test.go
- internal/errhint/errhint.go
- internal/tui/transcript_export.go
- internal/errhint/errhint_test.go
- internal/tools/bash.go
- internal/tui/model.go
…atch on Windows Address CodeRabbit review on #21: - latestResumableInWorkspace now applies the SAME filters as the picker (skip zero-event metadata and empty/failed runs), so /resume latest can't land on a session the picker intentionally hides. - sessionMatchesWorkspace compares paths case-insensitively on Windows (its filesystem is), so the same workspace spelled with different casing no longer hides resumable sessions; other platforms stay case-sensitive.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve attachments when retrying the previous prompt
internal/tui/model.go:3952
/retrynow relaunches onlym.lastPrompt, butlastPromptstores just the text while the original submit path consumes PDF text (consumePendingDocuments) and passes staged images viaturnImagesbefore clearing both attachment queues. If a vision or document-backed prompt fails and the user runs/retry, the retried request silently loses the image/PDF context even though the command is documented as resending the last prompt, so the model answers a different task. Please carry the submitted attachments/document preamble with the remembered prompt, or make/retryrefuse/clearly warn when the last prompt depended on attachments. -
[P3] Complete CodeRabbit's request to update the OAuthResolver provider comment
internal/providers/factory.go:27
CodeRabbit's earlier request is still valid: this comment saysOAuthResolveris applied only to OpenAI and Anthropic, but the current factory also passes it togemini.Newin the Google provider branch. Since this PR is specifically adding Gemini OAuth retry wiring, please update the provider-factory documentation so future auth work does not rely on a stale provider boundary.
/retry relaunched only lastPrompt's text, but launchPrompt clears the pending image/document queues once a turn is sent. A vision- or PDF-backed prompt that failed would silently retry as text-only and answer a different task. Snapshot the consumed attachments at launch and re-stage them on /retry so launchPrompt rebuilds an identical request (document preamble, images, and the submit-time vision re-check). startNewSession drops the snapshot so a post-/new /retry cannot leak the previous session's attachments. Also correct the OAuthResolver factory comment, which omitted the Gemini provider it is now passed to.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the updates. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P2] Complete CodeRabbit's request to keep 504 out of reconnect retries
internal/agent/reconnect.go:123
CodeRabbit's 504 reconnect concern is still valid on the current head. The new reconnect comment says HTTP 500/502/504 responses are non-idempotent and must not be replayed because the completion POST may already have been processed, butshouldReconnectstill matches the generic"timeout"substring, so an error like504 Gateway Timeoutreturns true and reissues the same completion request. Please add an explicit HTTP-status guard before the transport substring loop and updateTestShouldReconnectClassificationso 504 stays non-reconnectable. -
[P2] Preserve attachments when editing and resending the previous prompt
internal/tui/model.go:3974
The/editcommand recalls onlym.lastPromptinto the composer, while the image/PDF snapshot captured for that same last prompt is left out of the pending attachment queues. After a vision or document-backed prompt,/retrynow correctly re-stagesm.lastImages/m.lastDocuments, but/editfollowed by Enter silently sends a text-only version of the edited prompt. That makes the documented "edit and resend" flow answer a different task whenever the original prompt depended on attachments. Please either re-stage/show the remembered attachments for/edit, or warn/refuse when the previous prompt had attachments that cannot be carried through the edit flow. -
[P2] Keep recently re-edited files in the preserved edit summary
internal/agent/compaction_preserve.go:462
The recent-edit preservation cap can drop a file that was just edited again.recentEditsrecords path order only the first time a file appears,mergeSkillEntrieskeeps an older preserved path in its old position even when a fresh edit updates its note, andcapRecentEditsthen keeps only the tail of that old order. In a long editing session, once more than 20 distinct files are tracked, re-editing an early file can still leave it outside the capped tail, so the next compaction omits exactly the file the model most recently touched. Please move paths to the newest position when a fresh edit arrives, and add a regression covering a capped list where an old path is edited again. -
[P3] Complete CodeRabbit's request to make the coalescer batching test deterministic
internal/tui/coalesce_test.go:41
The test still starts the real 16mstime.AfterFuncand then asserts that nothing has been forwarded before the manualc.flush(). If the scheduler or GC pauses the test goroutine for longer than that interval between the sends and the snapshot, the timer goroutine can legally flush the buffered text first and make this test fail nondeterministically. Please add a test seam for the coalescer interval/timer or otherwise structure this assertion so it does not depend on beating a wall-clock timer.
…, coalescer test) - reconnect: exclude HTTP 5xx (500/502/503/504) before the transport substring match so "504 Gateway Timeout" no longer slips through on the generic "timeout" needle. A gateway timeout is non-idempotent — the completion POST may already have reached the model — so replaying the connect risked duplicate billable work. Reuses a digit-boundary status matcher exported from errhint (HasStatusCode). Test asserts 504/500 stay non-reconnectable. - tui /edit: re-stage the remembered image/PDF snapshot alongside the recalled prompt text, mirroring the /retry fix — editing a vision- or document-backed prompt no longer silently resends a text-only version. - compaction recent-edits: order edits by LAST edit, not first, and move a re-touched path to the newest position on merge, so the maxRecentEdits tail cap keeps the file the model most recently edited instead of dropping it. Adds mergeRecentEdits (edit-specific) and a capped-list regression. - coalescer test: inject the frame timer via afterFunc so TestCoalescerBatchesDeltas proves buffering deterministically instead of racing the real 16ms wall-clock timer.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the updates. I rechecked the changed paths and found an issue that still needs to be addressed.
Findings
- [P2] Bound bash output before it enters memory
internal/tools/bash.go:127
The new 96KB bash budget is applied only afterexec.Cmdhas already written all stdout/stderr into unboundedbytes.Buffers and after both buffers are converted to strings. A command likecaton a huge file can still grow the Zero process until it stalls or OOMs beforebudgetBashOutputever runs, even though the PR description says bash stdout/stderr are capped. Please cap the capture writer or stream through a bounded buffer before handing stdout/stderr to the formatter, so the tool is protected from large output as well as from large model-visible output.
…le text Previously stdout/stderr streamed into unbounded bytes.Buffers and were only truncated to the 96 KiB budget AFTER the command finished, so a runaway command (cat huge.log, yes) could grow Zero's memory until it stalled or OOMed before budgetBashOutput ever ran. Replace the capture with boundedBuffer, an io.Writer that keeps only the head+tail each stream will surface and discards the middle as it arrives, while counting the true total for the truncation marker. Memory is now bounded to ~head+2×tail per stream regardless of output size. truncateHeadTail is refactored onto a total-aware core so the string path (and its tests) are unchanged, and budgetBashCapture reports the true raw_bytes even though only a bounded slice was held. Adds boundedBuffer/capture unit tests and an end-to-end test that streams ~5× the budget and asserts bounded emission with an accurate raw_bytes.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the updates. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@Vasanthdev2004 LGTM
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-approving after the latest round (my earlier approval went stale on the new pushes).
I did an independent pass over the parts that worried me most — the core-loop changes where green CI and unit tests can still hide a hang, a leak, or dropped output. All of it holds up:
- streaming — the coalescer keeps delta order (the lock spans drain+forward) and the committed final answer stays authoritative, so no dropped or reordered text; the flush timer is reused, no per-turn goroutine/timer leak. The
[]byteaccumulator is copy-on-read, no aliasing. - compaction — the EMA ratio is div-by-zero-safe and clamped to
[0.5, 2.0], so a bad turn can't push compaction to fire way early or never; the preserved recent-edits block is hard-capped (20 paths, short notes) with no bypass. - reconnect/stall/tool-failure — jitter can't go negative or unbounded, retry counts are capped,
#34's narrowing doesn't strand a recoverable stream, and the 4→6 tool-failure stop has no off-by-one. - bash budget — capture is genuinely bounded in memory now (not just the model-visible slice), truncation is UTF-8-safe and keeps the failure tail where build/test errors live.
- LSP reuse — built once per session, nil-safe, torn down in
quit(), and cwd is process-fixed so/newand/resumedon't point it at the wrong root. - errhint/Gemini — the origin gate fails closed (a local error never draws a provider hint) and the 401 retry is single-shot with a rebuilt body, no loop.
On the whole PR: it's a lot to land at once (55 files), but it's the right kind of a lot — every item is its own commit with its own tests, each is a real wall-time or UX win rather than churn, and it's already been through three rounds with jatmn. Worth merging.
One coordination note: the bash output cap here (#3) covers the same ground as #492, so that one can be closed or folded once this lands.
Can't merge it myself — over to kevin.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
LGTM — approving. Ran an independent regression pass on top of the existing reviews.
Local gate (checked out polish/v2-smoothness):
go build ./...✅ ·go vet ./...✅ ·gofmt -lclean ✅go test ./... -race→ 74 packages, 0 failures, 0 races ✅
Deep-dived the streaming coalescer (internal/tui/coalesce.go) since it's the one item with new concurrency + a 16ms buffer. Traced the worst case — the final ≤16ms text batch losing the race to agentResponseMsg and getting dropped as a stale-run delta — and confirmed it's safe: on the success path the final answer is committed from the runtime's complete final rowAssistant in msg.rows, which supersedes the streamed streamingText, so a dropped tail only affects the last live frame. The error path keeps streamingText as a best-effort partial, where a 16ms truncation is immaterial. The mutex-held-across-forward design also correctly prevents streamed text from reordering around non-text (tool/reasoning) messages. The "harmless late flush" comment holds.
Everything else: the reconnect narrowing (502/503/504) and the bash memory-bound cap — the P2s from the earlier rounds — are addressed, and the full race suite passing gives me confidence the core-loop changes are clean. One-commit-per-item plus near-total test coverage made this easy to reason about despite the size.
Nothing blocking from me. The two remaining CodeRabbit inline notes (errhint.go:81, bash.go:181) are minor quick-wins — optional.
4 participants
Polish Roadmap v2 — the "smoothness & delight" pass. Where the prior context-economy PR cut per-turn token overhead, this PR cuts wall-time overhead and wires Zero's existing diagnostic machinery to the live failure points where users actually land.
Each item lands as its own commit so they can be reviewed (or reverted) independently. Every roadmap claim was spot-verified against the code before implementation. (Item numbers below are roadmap IDs, not GitHub issues.)
Shipped
Error recovery
#1Actionable provider/model error hints — classify a failed turn's provider error (auth / rate-limit / connectivity / model-not-found / context-overflow) and render a one-line next step under the red error row, instead of an identical raw blob for every failure. New sharedinternal/errhintpackage, gated on a provider-origin marker so local errors never draw a bogus hint.#16Gemini 401 auth-retry — plumbOAuthResolverthrough the Gemini provider + factory and switch toSendWithAuthRetry, so Gemini OAuth users recover from an expired token (force-refresh + replay on 401) instead of a terminal "API key not valid". Parity with OpenAI/Anthropic.#20CLI exec provider error hint — append a one-linezero auth/zero doctorhint to text-modezero execprovider errors, reusing the#1classifier.Per-prompt overhead
#3bash output budget — cap bash stdout/stderr at 96KB each with head+tail truncation (build/test failures live at the tail), recording raw/emitted/truncated in Meta +Result.Truncatedlike the other tools. bash was the only tool with no byte cap.#5reuse LSP manager across prompts — build onelsp.Managerper session (servers start lazily) instead of a fresh one per run, so gopls stays warm between turns instead of cold-starting on the first edit of every prompt.#7coalesce streamed text deltas — batchOnTextdeltas over ~16ms at the runtime sink so render rate decouples from token rate; a fast provider no longer drives 100+ Update→View cycles/sec.#10streamingText O(1) append — accumulate live assistant text as[]byteinstead ofstring +=, killing the O(n²) allocation on long generations.Reliability / recovery tuning
#17tool-failure stop 4→6 — a corrective hint fires at 2 failures; give a model iterating on a tricky edit more room before halting. Streak still resets on success or a different error.#19reconnect backoff + jitter —maxStreamReconnects2→4 with up-to-50% jitter (capped 8s), riding out a multi-second blip and de-synchronizing swarm/cron reconnects.#33stall retries 2→1 — a no-output stall is only detected after the full ~5min idle timeout, so 2 retries froze a session ~15min; one retry bounds it to ~2× idle.#34narrow reconnect matching — drop502/503substring matches (503 already exhaustedSendWithRetry; 502 is non-idempotent), reconnect only on genuine transport failures.#32(stream max-turns final answer) was already implemented —finalAnswerAfterMaxTurnsforwardsOnText/OnReasoningtoday; no change needed.Flow friction (commands)
#18/compact now — accept thenowkeyword (bare/compactalready triggered;/compact nowused to error).#12/retry and /edit — resend the last prompt, or recall it into the composer to tweak and resend.#23/copy and /export — copy the last answer to the clipboard; write a plain-text transcript to a file. Real gaps for SSH users with no mouse-select.Agent-quality polish
#31separator-insensitive tool_search —webfetchnow matchesweb_fetch(squash-separators fallback ranked below exact substring), so a model that drops the underscore doesn't loop.#30ask_user skip signal — a wholesale prompt dismissal is flagged as a skip up front (vs a single(left blank)field), so the model doesn't invent a default.Already satisfied by current code (no change):
#13(? shortcutsalready advertised by the composer idle-hint; the "allow?on a non-empty composer" idea would hijack typing "?"),#28(OnContextis never wired, soMeasureContextalready never runs per-turn;/contextcomputes on demand),#32(max-turns answer already streams).Merged latest
main(resolves the #478 conflict); no force-push.More flow polish
#11fuzzy picker ranking —/modeland/resumenow rank matches (exact < prefix < contains < subsequence) instead of a flat substring filter;snt45matchesSonnet 4.5. Groups stay contiguous so the grouped model picker keeps one header per provider.Already satisfied / held:
#27(turn-end already nilslineAgesviaresetStreamingFade+streamingTextvia#10);#24held for decision — its remaining win (sharing image bytes) would breakTestCopyMessagesDeepCopiesImageBytes, a deliberately-tested anti-aliasing invariant.Perf + agent-quality (round 3)
#22cache per-tool schema render — partitionTools no longer re-runs the recursive schema→map conversion for every tool every turn; a per-run cache keyed by tool name skips it. Partitioning stays fresh each turn (deferred state can flip mid-run).#25calibrate compaction estimate — fold each turn's (raw estimate, providerInputTokens) into an EMA ratio so compaction triggers near true capacity instead of ~15% early on code-heavy history.#26turn-based plan staleness — the stale-plan nudge now also fires after 8 turns without an update while items are pending (not just 10 tool calls), catching a plan that drifts stale across many low-tool-call turns. The roadmap's fuzzy relevance-match is omitted (too false-positive-prone).Round 4 (flow + agent-quality)
#35preserve recent edits across compaction — carry write_file/edit_file targets + a one-line note per file in the preserved-state block (capped 20 paths), so after compaction the model knows what it changed without re-reading.#21workspace-scoped /resume — filter the picker (and/resume latest) to the current workspace before the per-session event read (no more N full file reads for a global history); skip zero-event sessions via metadata. Empty-Cwd/older sessions stay visible.#14image-drop warning on model switch — chips already render above the composer; add an immediate amber warning when switching to a non-vision model with images staged (was silently dropped at submit).Held (needs registry/concurrency work — maintainer decision):
#15async MCP connect —tools.Registryhas no mutex, so background registration races the agent loop's per-turnregistry.All(), and it breaks the deferral-gate timing invariant. Not a safe drop-in.Summary by CodeRabbit
New Features
Bug Fixes