Conversation
Replace the Ink and React terminal UI with an OpenTUI + Solid implementation while preserving Zero's chat/provider flows. Add a persistent header for provider, model, mode, and usage totals; add an optional todo rail fed by update_plan with Ctrl+T and /todo toggles; add a slash-command popup for /provider, /plan, /todo, /help, and /exit. Update agent/provider plumbing to surface usage and plan updates, retry OpenAI-compatible streams without stream_options when unsupported, and lazy-load the TUI with the Solid transform for source execution. Switch the build to a non-exe Bun bundle under dist and remove the old Ink/React dependencies and components.
Resolve conflicts from main's plan-mode, debug, provider-error, and test additions by porting them into the OpenTUI/Solid implementation. Keep the non-exe OpenTUI build path while preserving main's test script, plan-mode system prompt, tool schema conversion, tools/debug toggles, and improved provider error handling.
Handle OpenTUI paste events and add a Windows Ctrl+V clipboard fallback for terminals that send the control key directly. Make Ctrl+C clear the composer or back out of provider screens before exiting the TUI.
Add /clear to reset the TUI transcript buffer. Read the displayed and CLI version from package metadata instead of hard-coding it.
Move the transcript to a native OpenTUI scrollbox so new output sticks to the bottom, mouse wheel scrolling works, and the scroll indicator is native instead of hand-rolled. Add a resizable todo rail with mouse movement enabled, a narrower default width, a larger max width, and hover feedback on the resize edge. Rework the TUI framing with compact panel borders, a prompt frame, reclaimed vertical space, and a compact zero header mark with version/model/usage metadata. Add explicit /build and /default paths back out of plan mode while preserving /plan as a toggle.
Collaborator
|
Thanks for putting this together — porting main's plan-mode, debug, and provider-error work forward into the new stack instead of just reverting it was the right call, and a few pieces here are clear wins regardless of where we land (more below). Before we go deeper on the UI, I want to flag that this PR quietly settles two decisions we hadn't actually made yet, and I'd like us to make them on purpose rather than by merge:
Both are reasonable options, but I'm not sold on taking them as a package, and the second one is the one I keep snagging on. A full-screen panel app breaks native terminal copy-paste and scrollback, which I think is a core part of how this tool should feel. I'd like to keep the transcript scrollback-native. What I'd keep no matter what
These would be great as a separate PR we can merge now — none of it depends on the TUI rewrite. UI regressions I'd want addressed before any TUI swap
Other things to decide consciously
Net: let's split out the plumbing wins so they can land now, and treat the TUI direction (append-vs-panes, Ink-vs-OpenTUI) as its own call before we commit the rewrite. Happy to prototype an append-style version with the same scroll/usage/todo wins so we can compare. |
Collaborator
Author
|
Thanks, this was the right framing. I split the mergeable plumbing/UI wins out of this OpenTUI rewrite into #4 so they can be reviewed against main without forcing the render-stack or transcript-model decision. This PR can stay focused on the larger Ink vs OpenTUI / append vs pane discussion. |
jatmn
changed the title
jatmn
marked this pull request as ready for review
June 1, 2026 02:43
| setScrollOffset(0); | ||
| } | ||
| }, [messages.length]); | ||
| export function App(props: { onExit: () => void }) { |
Collaborator
There was a problem hiding this comment.
I checked out this branch and the mergeable pieces in #4, and want to leave one focused comment on the OpenTUI direction (rather than a long inline review — this PR is in the right state as a draft).
Recommendation: keep this as draft, merge #4 first.
Reasoning:
-
The regressions you already listed in the PR body are the right ones to weigh — the loss of the Ink markdown/code path, the loss of tool-call collapse/expand + smart summaries +
[more], the braille header, the dropped splash/logo, and the alt-screen-pane scrollback breakage. Each of those is a real UX downgrade and a faithful representation of how the TUI currently works. Inverting them in a 1,400-line rewrite is the kind of regression that's easy to underestimate until users hit it. -
The render-stack decision is bigger than this PR can carry.
src/tui/App.tsxon this branch is 1,151 lines in a single file. That's a maintainability cliff before the user-experience questions are even settled. The decision to swap Ink for OpenTUI + Solid is a commitment to a different concurrency model (signals vs. React's reconciler), a different JSX runtime (Babel toolchain + the Solid plugin you wired up insrc/index.ts:3), a different distribution story (theensureSolidTransformPlugin()call suggests a custom build), and a different terminal-IO model (Kitty keyboard protocol, mouse-movement events). Each is a fork in the road that should be made explicitly by the project owner before more code accumulates against a possibly-rejected direction. -
Split out agent plumbing from OpenTUI branch #4 captures the actual wins. The agent-loop
onUsage/onPlanUpdatecallbacks, thestream_options.include_usageretry, the cumulative-usage header, the todo rail, and the removal of the broken manual scroll — all of these are improvements to the current stack that don't depend on the render-stack decision. I posted 11 inline findings on Split out agent plumbing from OpenTUI branch #4 (one critical multi-turn text-drop bug, one streaming 401-prefix bug, anisUnsupportedStreamOptionsErrorfalse-positive, and a few smaller issues). Once those are addressed, Split out agent plumbing from OpenTUI branch #4 should merge cleanly. -
A few things this branch does that Split out agent plumbing from OpenTUI branch #4 doesn't, worth recording if/when the OpenTUI direction is approved:
- Resizable todo rail width (this branch:
minTodoRailWidth=18/maxTodoRailWidth=65) - Kitty keyboard protocol support
- Mouse-movement events enabled
- Package version surfaced in the header
process.once('SIGINT', exit)clean shutdown wiring that the Ink version lacks- Proper paste handling (
usePaste+normalizePastedText) — the Ink version drops pasted newlines
These are good ideas, but each needs the render-stack decision to be made first so they can be ported (or not) deliberately.
- Resizable todo rail width (this branch:
-
Build is currently broken on this branch (
bun run buildfails —react-devtools-corenot resolvable, plus the Solid toolchain may needbun add -dof new deps). That's worth noting as a follow-up regardless of which direction is taken.
Closing thought: this is a well-scoped draft and your "known review concerns before merge" list is the right framing. I'd suggest converting this PR to a discussion issue (or closing it in favor of an issue titled "Evaluate OpenTUI + Solid migration" with this PR linked as the working branch) so the decision is made on its own merits, separate from any merge-blocker. The plumbing in #4 then has a clean path to land without waiting on a 1,400-line TUI rewrite to be either blessed or rejected.
| ); | ||
| } | ||
|
|
||
| function ZeroLogo(props: { compact?: boolean }) { |
Collaborator
There was a problem hiding this comment.
Re-review of Restore TUI rendering affordances (8ef78c0).
Checked out the latest and walked through the four new commits (Fix terminal paste and cancel shortcuts → Add clear command and build version header → Refine OpenTUI layout and transcript behavior → Restore TUI rendering affordances). Substantive progress.
What got restored (the things I flagged in the PR body)
- ✅ Figlet ZERO logo — both compact and full variants (App.tsx:841-870), replaced the braille header.
- ✅ Tool-call collapse/expand +
[more]/[less]—<ToolCall>component (App.tsx:1082-1148) is back, with[x]/[>]status marks, one-linetoolSummary(...)label, and toggles wired via OpenTUI's realonMouseDownhandler. Unlike the Ink version on main, the click handler actually works here because OpenTUI's<text>element supports mouse events. - ✅ Splash / empty-session —
<EmptySession />fallback inside the<Show when={messages.length > 0}>(App.tsx:911). - ✅ Sticky-bottom native scroll —
<scrollbox stickyScroll stickyStart="bottom">(App.tsx:905) gives a real "always pinned to bottom" feel without the brokenscrollOffsetslicing from main. - ✅ Resizable todo rail — with
onMouseDragfor live width adjustment. Nice. - ✅ Clear command + version header — operational polish.
The commit message is accurate; this is genuinely a restoration effort, not just a re-shuffle.
What I want to flag for the next round
1. The two provider-error fixes from my PR #4 review aren't in this branch.
I confirmed by running the test suite: applying PR #4's regression tests (tests/openai-provider.test.ts) against the current pr3_2 source gives 22 pass, 2 fail:
does not retry unrelated unknown-parameter errors— fails becausesrc/providers/openai.ts:30still uses the broaderrorText.includes('stream_options') || … 'unknown parameter' || …keyword list. The fix from Split out agent plumbing from OpenTUI branch #4's r2 is a single regex:/stream[ _-]?options?/.categorizes mid-stream authentication errors— fails because the streaming catch atsrc/providers/openai.ts:196is still the oldthrow new Error(\Provider returned error during streaming: ${message}`);. The fix from #4's r2 isthrow formatProviderCreateError(error);`.
Both are one-line changes. The classification helper formatProviderCreateError is already defined on this branch (line 201) — it's just not called from the streaming catch. These fixes would also make the OpenTUI TUI's toFriendlyError mapper at App.tsx:39 produce the same helpful "Authentication failed - check your API key" message that the Ink TUI now produces.
2. The tc.name === 'update_plan' special case in the agent loop is back in a new shape.
Looking at src/agent/loop.ts on this branch — does it still have the old hard-coded if (tc.name === 'update_plan') check, or has it been ported to the Tool.onAfterExecute pattern from PR #4 r2? If the former, this branch is going to fork from #4's fix the moment #4 merges. Worth a rebase / merge of #4 before more code accumulates against the unfixed shape.
3. Build is presumably still broken on this branch.
bun run build requires the react-devtools-core import that ink makes — and this branch is on OpenTUI + Solid, not Ink, so that may not apply here. But the new @opentui/core / @opentui/solid packages presumably have their own build-time concerns (native binaries per the PR body). Worth running bun run build here and confirming before merge consideration.
Bigger-picture question
The restoration work in this PR is real, and the OpenTUI affordances (mouse-driven rail resizing, sticky-bottom scroll, native mouse events on text) are genuine wins that the Ink version doesn't have. But the OpenTUI vs Ink decision is still undecided at the project level, and #4 was based on a recommendation to keep this draft until that decision is made.
Has the team reached a decision? If yes, this branch has matured enough to be the merge target. If no, the most useful next step is probably to rebase this onto #4 (so the agent-loop fixes flow both ways) and then leave the branch as the proof-of-concept for the OpenTUI direction without merging.
Collaborator
Author
|
ink issues is a sticky one.. because that's remnants related to #4, which needs to land first so i can properly deteather it after |
This was referenced Jun 2, 2026
This was referenced Jun 12, 2026
gnanam1990
added a commit
that referenced
this pull request
Jun 18, 2026
… scanner over-redaction (#249) * redaction: catch compound secret keys + non-bearer auth schemes; stop scanner over-redaction Hardens the secret-redaction last line of defense after an audit of internal/redaction + internal/secrets. 1. Compound secret keys are now redacted. IsSensitiveKey was exact-match only, so db_password, session_secret, stripe_secret_key, auth_token, csrf_token, ssh_private_key, backup_api_key, etc. passed through in cleartext. A new keyLooksSensitive applies conservative structural heuristics (secret / password / passphrase / credential / apikey segments; a singular trailing "_token"; api_key / private_key pairs). Critically, the agent's token-COUNT fields (max_tokens, prompt_tokens, token_count) and ordinary *_key fields (primary_key, public_key, cache_key) are deliberately NOT redacted. 2. Authorization headers with non-bearer/basic schemes were leaking. The header pattern now also covers token, apikey, digest, negotiate, oauth, and aws4-hmac-sha256. 3. The secrets scanner over-redacted ordinary text: its prefixed patterns (sk-, gh*, AIza, AKIA, xox, github_pat, jwt) lacked the word boundaries that internal/redaction already uses, so kebab-case words like "task-management-and-coordination" matched as a fake key. Added \b anchors. Design note: a free-text "key: value" colon pattern was prototyped but dropped -- it mangled "file.go:line: message" output (a filename's "secret" segment looked like a sensitive key), the same over-redaction class fix #3 addresses. The structured paths (RedactValue for maps/structs, plus assign/JSON/query forms in RedactString) are covered; free-text colon-form remains out of scope. Tests: compound-key + token-count-safety matrix, auth-scheme redaction, the file:line guard, and scanner boundary positives/negatives. * redaction: redact the full auth-header value for multi-part schemes The broadened header pattern captured only the first token after the scheme, so parameterized schemes (Digest "…, response=…", AWS4-HMAC-SHA256 "…, Signature=…") left the actual secret param visible. Redact the entire value to end of line; the scheme name is kept. Adds tests with realistic Digest + AWS SigV4 headers. Addresses review feedback.
gnanam1990
added a commit
that referenced
this pull request
Jun 27, 2026
Addresses the correctness findings from the PR review. self-report (#1): drop behavior-describing phrases ("fall back to", "placeholder value", bare "best guess", "as a fallback", "without proper") that also match legitimate final answers; keep first-person/uncertainty admissions only, since these are matched without a context guard. self-report (#5): scan every occurrence of each inability stem so an early success-negation ("could not find any examples") no longer masks a later genuine admission with the same stem ("could not implement it"). continuation cue (#6): require a trailing colon AND an action lead-in on the final clause; stop flagging recommendations, plain summary colons, and sign-offs. Still catches the mid-line "...Let me check the config:". gate order (#8): check the self-report admission BEFORE pending-plan, so an admitted-impossible task downgrades immediately with the accurate reason instead of burning continue-nudges. pending plan (#3): treat a pending/in_progress update_plan item as a NUDGE-only weak signal -- it no longer forces INCOMPLETE on its own (a completed run that left stale plan bookkeeping is trusted). Only a continuation cue or a self-report admission finalizes INCOMPLETE. max-turns (#4): a run cut off at the MaxTurns ceiling now finalizes INCOMPLETE under the gate instead of being reported as success. exec json/cron (#2, #7): for -o json, emit the terminal done with exit 4 on an incomplete run (final() pre-emits a success done:0 for json that would otherwise mask it); emit an error event -- not just a warning -- so the cron failure extractor can recover the reason. Deferred (noted on the PR): acceptance-only-when-mutated (#9, cost) and reusing tools.normalizePlanStatus (#10, would widen scope to internal/tools). Tests: add TestContinuationCueMatching and TestMaxTurnsCutoffIsIncompleteUnderGate, extend TestSelfReportedIncompletionMatching with the #1/#5 cases, and replace the in_progress=>incomplete test with TestPendingPlanAloneDoesNotForceIncomplete. make build / go vet / make lint / go test ./... -race all green.
gnanam1990
added a commit
that referenced
this pull request
Jun 27, 2026
… on no-tool-call / self-reported-incomplete turns) (#325) * fix(agent): don't end a run as success on a no-tool-call turn mid-task A turn that produced text but no tool call was always accepted as the final answer, so the loop reported success even when the model stopped mid-task (e.g. ended on "...Let me check the SSH configuration:" with plan steps still pending). Add an opt-in completion gate (Options.RequireCompletionSignal): when a turn has no tool call and work clearly remains -- pending update_plan items, or the message ends on a continuation cue -- re-prompt the model to continue instead of finalizing. Bounded by maxContinueNudges and still by MaxTurns/the deadline; once the budget is spent the run finalizes as INCOMPLETE (Result.Incomplete) rather than success. Default off, so the interactive path is byte-identical. Genuine single-turn completions (no pending plan, no cue) still finalize as success. Covered by internal/agent/completion_gate_test.go. * feat(exec): report stalled headless runs as INCOMPLETE (exit 4) Enable the agent completion gate for headless exec (RequireCompletionSignal) and map Result.Incomplete to run_end status "incomplete" with a new exit code 4, so a run that stalled mid-task (model stopped without a tool call while work remained, continue budget exhausted) is no longer reported as success. Interactive callers are unaffected. * fix(agent): downgrade self-reported non-completion; advisory task-grounded acceptance Reduce -- not eliminate -- false-success on headless runs. Two DETERMINISTIC, unit-tested gates (with the plan gate from the prior commit): (a) self-report downgrade: if the final message admits the model guessed or could not meet the objective, finalize INCOMPLETE (exit 4), never success. Inability is matched by first-person STEMS generalized over verb/tense ("I cannot/can't/could not/am unable to/do not have/unable to ...") plus guess/fallback/uncertainty phrases, with a guard so success-y negations ("could not find any issues", "cannot reproduce") are not misread. (b7bc0b8's plan gate already forces INCOMPLETE on pending/in_progress update_plan items at termination.) (b) task-grounded acceptance is ADVISORY, not a guarantee. When --self-correct is on it demands one bounded acceptance pass that re-derives the task's stated criterion and runs a concrete check, discouraging three false-success patterns (well-formed==correct, existing-tests-pass==objective-met, result==baseline-it-was-told-to-beat). But it is a prompt: a model that ignores it and confidently claims "PASS, all requirements met" still slips, because ZERO has no general oracle to verify correctness against a task's hidden criterion. Empirically (TB-2, qwen3-coder:480b) this reliably catches admissions and incomplete plans and REDUCES false-success, but a confident false PASS on a model-ceiling task is a residual, fundamental gap -- not a tuning miss. Default off (RequireCompletionSignal); interactive callers are byte-identical. Covered by internal/agent/{acceptance_gate_test.go,completion_gate_test.go}. * feat(exec): surface the INCOMPLETE reason in run_end and logs When a headless run finalizes as INCOMPLETE, include Result.IncompleteReason in the session error event and a stderr warning so an honestly-incomplete run (e.g. "the final message admits the objective was not met") is debuggable rather than an opaque exit 4. * fix(agent): harden completion gate per PR review Addresses the correctness findings from the PR review. self-report (#1): drop behavior-describing phrases ("fall back to", "placeholder value", bare "best guess", "as a fallback", "without proper") that also match legitimate final answers; keep first-person/uncertainty admissions only, since these are matched without a context guard. self-report (#5): scan every occurrence of each inability stem so an early success-negation ("could not find any examples") no longer masks a later genuine admission with the same stem ("could not implement it"). continuation cue (#6): require a trailing colon AND an action lead-in on the final clause; stop flagging recommendations, plain summary colons, and sign-offs. Still catches the mid-line "...Let me check the config:". gate order (#8): check the self-report admission BEFORE pending-plan, so an admitted-impossible task downgrades immediately with the accurate reason instead of burning continue-nudges. pending plan (#3): treat a pending/in_progress update_plan item as a NUDGE-only weak signal -- it no longer forces INCOMPLETE on its own (a completed run that left stale plan bookkeeping is trusted). Only a continuation cue or a self-report admission finalizes INCOMPLETE. max-turns (#4): a run cut off at the MaxTurns ceiling now finalizes INCOMPLETE under the gate instead of being reported as success. exec json/cron (#2, #7): for -o json, emit the terminal done with exit 4 on an incomplete run (final() pre-emits a success done:0 for json that would otherwise mask it); emit an error event -- not just a warning -- so the cron failure extractor can recover the reason. Deferred (noted on the PR): acceptance-only-when-mutated (#9, cost) and reusing tools.normalizePlanStatus (#10, would widen scope to internal/tools). Tests: add TestContinuationCueMatching and TestMaxTurnsCutoffIsIncompleteUnderGate, extend TestSelfReportedIncompletionMatching with the #1/#5 cases, and replace the in_progress=>incomplete test with TestPendingPlanAloneDoesNotForceIncomplete. make build / go vet / make lint / go test ./... -race all green.
This was referenced Jul 3, 2026
anandh8x
added a commit
that referenced
this pull request
Jul 4, 2026
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.
anandh8x
added a commit
that referenced
this pull request
Jul 5, 2026
* feat(tui): actionable hints on provider/model errors 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. * feat(tools): budget bash output with head+tail truncation 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. * fix(gemini): retry 401 with refreshed OAuth token 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. * fix(errhint): gate classification on provider-origin marker 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. * feat(cli): actionable hint on text-mode exec provider errors 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. * fix(tools): set Result.Truncated for budgeted bash output 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. * perf(tui): reuse one LSP manager across prompts 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). * perf(tui): accumulate streamingText as []byte (O(1) append) 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. * perf(tui): coalesce streamed text deltas to one frame 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. * fix(agent): raise tool-failure stop threshold 4->6 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. * fix(agent): more reconnect retries with jitter; narrow to transport errors #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. * fix(agent): reduce stall retries 2->1 to bound stuck-session time 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. * fix(tui): serialize coalescer forwarding to preserve message order 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. * feat(tui): accept /compact now 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. * feat(tui): add /retry, /edit, /copy, /export commands /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. * feat(tools): separator-insensitive tool_search matching 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. * feat(tools): signal ask_user dismissal distinctly from a blank field 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)'. * feat(tui): fuzzy ranking in model/session pickers 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). * perf(agent): cache per-tool schema render across turns 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. * perf(agent): calibrate compaction estimate against real prompt tokens 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. * style: gofmt the new agent test files The CI gofmt check flagged the #22/#25 test files (struct-field and switch alignment). No behavior change. * feat(agent): turn-based plan staleness reminder The stale-plan nudge only fired after 10 tool calls since the last update_plan, so a plan that drifts stale across many low-tool-call turns (one call per turn) took many turns to catch — the 'plan set turn 1 stays pending forever' case. Add a turn-count complement: after stalePlanTurnThreshold (8) turns without an update AND with items still pending, fire the same one-shot reminder. Gated on pending items so a fully-completed plan is never nagged; the existing tool-call trigger is unchanged. (The roadmap's fuzzy 'tool calls unrelated to plan items' relevance match is deliberately omitted — too false-positive-prone.) * feat(agent): preserve recent-edits summary across compaction Compaction preserved the plan, loaded skills, tool schemas, and project instructions, but not file edits — so after the editing turns were elided the model knew it had changed a file only vaguely, and would re-read to rediscover its own footprint. Extract write_file/edit_file targets from the elided middle with a one-line note from each tool result, merge them across repeated compactions (newer note per path wins), and carry them in the preserved-state JSON (capped at 20 paths / 160 bytes per note). * feat(tui): workspace-scoped /resume + fewer event reads /resume listed sessions from every project globally, and read the full event file for each of them to test for resumable content. Filter the picker (and /resume latest) by the current workspace Cwd, checked BEFORE the per-session read so a large global history no longer pays N full file reads to build one workspace's list; also skip zero-event sessions via metadata without any read. Sessions with no recorded Cwd (older runs) and an unknown current workspace stay visible so nothing is hidden that can't be confidently placed. Explicit /resume <id> still resolves any session. * feat(tui): warn on model switch that drops staged images Staged images were silently dropped at submit when the active model had no vision support — the user only learned after sending. Chips already render above the composer (renderAttachmentChips); add an immediate amber warning line to the /model switch status (both same-provider and cross-provider paths) when images are staged and the newly selected model can't accept them, so the drop is surfaced at switch time. * feat(tui): inline recovery affordance on setup errors A first-run setup error rendered as a bare red 'error: <text>' with no pointer to the fix. Append a faint, stage-tailored recovery line (edit the endpoint/name/key above then Enter, or pick another model) that names only keys the stage's footer confirms, so the guidance is always accurate. Empty for stages whose footer already makes recovery obvious. * fix: address CodeRabbit review (5 findings) - 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. * test(tui): skip export file-mode assertion on Windows 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. * fix(tui): align /resume latest filters + case-insensitive workspace match 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. * fix(tui): /retry resends the last prompt's image/PDF attachments /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. * fix: address jatmn review (reconnect 5xx, /edit attachments, edit-cap, 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. * fix(tools): bound bash output capture in memory, not just model-visible 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.
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current status
Draft exploration PR for the larger TUI direction only.
The mergeable pieces called out in review have been split into #4:
onUsageandonPlanUpdatestream_options.include_usageretry fallbackupdate_planThat means this PR should no longer be treated as the path for those incremental wins. #4 can be reviewed and merged independently against
main.What this PR is now for
This branch remains a place to evaluate the bigger product/architecture questions from review:
Those decisions should be made explicitly before this rewrite is considered mergeable.
Implemented in this branch
/provider,/clear,/plan,/build,/default,/todo,/tools,/debug,/debug-mode,/help, and/exit.update_plan.mainplan-mode, debug, provider-error, tool schema, and test behavior into the OpenTUI implementation.Known review concerns before merge
[more]affordance.Validation snapshot
Last validation on this branch before the split:
bun installbun run tsc --noEmitbun test ./tests --timeout 15000-> 20 passbun run buildbun dist\index.js --version->0.1.0This still needs hands-on interactive terminal testing across terminal sizes, mouse/scroll behavior, providers, and real agent sessions.
Relationship to #4
#4 is the recommended cleanup path for the reviewer's "keep no matter what" items. This PR should remain draft until the team decides whether to continue with OpenTUI/Solid and the pane-based transcript model.