Split out agent plumbing from OpenTUI branch

[jatmn]

Summary

Splits the mergeable agent/provider plumbing and Ink UI improvements out of #3 without taking the OpenTUI/Solid rewrite, alt-screen pane model, dependency changes, or build-output changes.

This follows the review guidance on #3: keep the useful pieces that do not depend on the TUI rewrite, and leave the render-stack/transcript-model decision for a separate discussion.

What changed

• Added onUsage and onPlanUpdate callbacks to the agent loop.
• Clears and publishes the current plan at the start of each agent run, then republishes it after update_plan tool calls.
• Requests streamed token usage from OpenAI-compatible providers with stream_options.include_usage.
• Retries once without stream_options when a compatible provider rejects that parameter.
• Adds cumulative prompt/completion/total token usage to the existing Ink header.
• Adds a lightweight Ink todo rail fed by update_plan, with /todo and Ctrl+T toggles.
• Removes the old manual transcript slicing/scroll-offset behavior so the Ink transcript renders directly and terminal scrollback remains useful.
• Keeps the existing Ink/React UI stack, figlet splash, markdown/code rendering path, and tool-call renderer.
• Fixes a few strict TypeScript issues in existing Ink components and restores Shiki highlighting via token-to-ANSI rendering.

Validation

• bun run tsc --noEmit
• bun test ./tests --timeout 15000 -> 23 pass
• bun run build
• git diff --check

Notes

This intentionally does not include the OpenTUI/Solid implementation from #3. That PR can stay as the place to discuss whether Zero should move from Ink to OpenTUI and whether the transcript should become a full-screen pane UI.

[Vasanthdev2004]

Critical bug — multi-turn assistant text is silently dropped.

The new callbacks here are wired up correctly, but the onText handler at the unchanged L202-216 (and onToolCall at L218-225) interact badly on the second LLM turn.

Trace for "ask the agent to do something that requires a tool":

1. Turn 1: setMessages (L192-196) appends an empty assistant message and sets streamingMessageIndex to its index.
2. Text deltas stream into that assistant message via onText (L202-216). Good.
3. onToolCall (L218-225) appends a tool-call message and sets streamingMessageIndex = null.
4. The agent loop's turn 2 yields text events for the final answer.
5. onText reads streamingMessageIndex from closure → it's null. The fallback newMessages.length - 1 points at the just-appended tool-call message (not assistant). The type guard at L208 fails, so every text chunk is silently dropped.
6. The agent loop's finalAnswer still gets set (via currentText accumulation), so runAgent returns normally — but the TUI shows no final-answer text to the user.

I verified the agent loop itself yields turn-2 text correctly (the new agent-loop.test.ts test confirms). The bug is purely on the App.tsx consumer side.

Fix sketch: don't reset streamingMessageIndex in onToolCall. Track the assistant message by a stable id (or by tag) rather than array index, so subsequent text deltas continue writing to the right row.

[Vasanthdev2004]

Bug — mid-stream 401/rate-limit errors lose the friendly prefix.

The new formatProviderCreateError (L202-215) is wired up at the create-call site (L100-104) and correctly produces "Provider authentication error (check your API key): …" or "Provider rate limit error: …". But this streaming catch block rewraps the error inline and bypasses the helper, producing a flat "Provider returned error during streaming: …" message.

toFriendlyError in App.tsx:24-45 then tries to match substrings like 'auth', 'unauthorized', 'rate', 'quota' against this wrapped string. The inner message does survive (the inner substring is preserved), so the substring matcher in toFriendlyError actually still works in the common case. But the user's first read of the error string is uninformative, and the doubled "Provider returned error during streaming: Provider returned error: …" prefix leaks when the create-call path also fired (e.g. the create succeeded and the stream then 401'd on a follow-up chunk — see the related isUnsupportedStreamOptionsError retry path at L96-105, which produces a chain).

Fix: replace the inline rewrap with throw formatProviderCreateError(error); so this path uses the same categorization as the create call. One line.

Also: please add a test in tests/openai-provider.test.ts that asserts a mid-stream { error: { message: 'Incorrect API key' } } chunk produces an error message containing "Provider authentication error" (or at least "authentication").

[Vasanthdev2004]

False-positive risk — the keyword list is overbroad.

isUnsupportedStreamOptionsError matches any 400/422 error whose body contains one of: 'stream_options', 'stream options', 'include_usage', 'unknown parameter', 'unsupported parameter', 'unrecognized'. The last three are generic OpenAI-compatible-API error phrasings that also fire for unrelated problems — unknown tool names, unsupported tool_choice values, invalid response_format, etc.

Confirmed by a probe test: a 400 with message "unknown parameter: tool_choice" triggered a second request without stream_options, which then re-threw the same error. The user gets a confusing second error and an extra network round-trip; the real problem (tool_choice) is masked.

Fix sketch: tighten to a single specific keyword match, or look for stream_options only in conjunction with the param name appearing near it. E.g.

const errorText = [...].join(' ').toLowerCase();
return statusAllowsRetry && /stream[ _-]?options?/.test(errorText);

Or, since this is the only OpenAI-specific quirk being handled, consider lifting the retry shape to the Provider interface so each provider declares its own optional request variants and "recoverable" predicate.

[Vasanthdev2004]

React anti-pattern — calling state setters inside another setter's updater.

onPlanUpdate: (nextPlan) => {
  setPlan((current) => {
    if (nextPlan.length > 0 && current.length === 0) {
      setTodoRailHidden(false);
      setTodoRailOpen(false);
    }
    return nextPlan;
  });
},

setPlan's updater is supposed to be a pure reducer. Under StrictMode (or any future bailout), the updater runs twice, scheduling setTodoRailHidden and setTodoRailOpen twice. Worse, setState-during-render-of-another-component is officially discouraged in React 19 — in some configurations it logs a warning, in others it can throw.

Fix sketch: pull the side effects out of the updater and into the outer callback scope:

onPlanUpdate: (nextPlan) => {
  if (nextPlan.length > 0 && plan.length === 0) {
    setTodoRailHidden(false);
    setTodoRailOpen(false);
  }
  setPlan(nextPlan);
},

This is the same pattern as the working onUsage callback directly above (L244-248), which uses the functional setter form correctly because it doesn't have additional side effects.

[Vasanthdev2004]

Double-prefix bug — chain: formatProviderCreateError(retryError) → outer streaming catch.

If the first call fails (e.g. truly invalid API key with 401) and the retry also fails with a categorized error, the user sees:

Provider returned error during streaming: Provider authentication error (check your API key): Incorrect API key provided

Two Provider… error: prefixes. Fix this together with the L197 finding (call formatProviderCreateError from the streaming catch too) — once both sites use the same helper, the chain becomes a single categorization and the rewrap prefix is gone.

[Vasanthdev2004]

Bug — the /todo command prints the wrong state in its confirmation message.

if (cmd === '/todo') {
  toggleTodoRail();
  setMessages((prev) => [
    ...prev,
    { type: 'system', content: `Todo rail ${showTodoRail ? 'hidden' : 'shown'}.` },
  ]);
  return;
}

toggleTodoRail runs (calling setTodoRailOpen / setTodoRailHidden setters), but showTodoRail (L122) is derived from the previous render's state. So the message printed to the user always describes the state before the toggle, not after. A user typing /todo to hide the rail sees "Todo rail shown." even though the rail is about to hide.

The Ctrl+T keybind (L136-138) doesn't print any message at all — so the two paths also behave asymmetrically.

Fix: compute the next state explicitly in the callback (read the current values from state via the functional setters and pass them through), or move the message-printing into toggleTodoRail and pass the next state in:

const toggleTodoRail = (): 'shown' | 'hidden' => {
  const willShow = !showTodoRail;
  if (showTodoRail) { setTodoRailOpen(false); setTodoRailHidden(true); }
  else              { setTodoRailHidden(false); setTodoRailOpen(true); }
  return willShow ? 'shown' : 'hidden';
};

Then /todo does toggleTodoRail() and uses the return value. Ctrl+T can call the same function and either print the message or not.

[Vasanthdev2004]

State machine bug — /todo to hide is silently overridden by the next plan arrival.

The auto-reveal logic at L252-254 (if (nextPlan.length > 0 && current.length === 0) { setTodoRailHidden(false); setTodoRailOpen(false); }) reopens the rail on the next runAgent call (because clearPlan empties the plan to length 0, then onPlanUpdate?.(getCurrentPlan()) at loop.ts:45 fires with [] to the App, then a moment later a real plan arrives with length > 0 and triggers the reopen).

So: user types /todo to hide the rail, then submits a new prompt, then the agent starts a plan → rail pops back open even though the user asked for it hidden.

The user has no way to persistently hide the rail across turns. Two reasonable fixes:

1. Track an explicit "user has hidden the rail" preference in a ref or in localStorage and let it override the auto-reopen.
2. Drop the auto-reopen entirely; let users manually /todo to re-show when they want it.

[Vasanthdev2004]

UX regression — Ctrl+T fires even when the user is typing.

The old useInput had a if (!input) { … } guard wrapping the special-key handling (the arrow/PgUp/PgDn/Home/End scroll keys). The new code dropped the guard. Now Ctrl+T toggles the rail even when the user is mid-typing a path, an API key, or a slash command argument. If the user holds Ctrl while pressing any letter, this keybind can swallow other intended Ctrl-letter combinations too (Ctrl+C still works because it's checked first, but anything else Ctrl-prefixed will hit this branch first).

Fix: wrap the keybind in if (!input) like the original scroll keys, or check that no modifier other than Ctrl is set and that inputChar === 't' only when input is empty / a slash command.

A small bonus: the help text at L620 says "Ctrl+T todo" but doesn't mention /todo as the keyboard alternative for users who don't have a Ctrl key.

[Vasanthdev2004]

Refactor — Header and TodoRail should live in their own files.

These 84 lines of new presentational components are defined at the bottom of an already-714-line file. The neighboring Logo.tsx, Spinner.tsx, MessageRenderer.tsx, and ToolCallRenderer.tsx each follow the one-component-per-file convention. Moving Header and TodoRail to src/tui/Header.tsx and src/tui/TodoRail.tsx would:

• shrink App.tsx by ~85 lines (back under 630)
• let Header be unit-tested in isolation (it's a pure function of props)
• make the planStatusMark / planStatusColor helpers (L689-713) testable without booting Ink

No functional change. Just organization.

[Vasanthdev2004]

Layering — hard-coding a tool name in the agent loop.

if (tc.name === 'update_plan') { onPlanUpdate?.(getCurrentPlan()); } is a string match on a specific tool. If a future tool mutates module state (e.g. set_config, remember_fact), this code has to grow more if branches, and a rename of update_plan in src/tools/plan.ts silently breaks the onPlanUpdate pipeline with no compile error.

Alternative: add a onAfterExecute?: (result: string) => void field to the Tool interface in src/tools/types.ts. The loop can call tool.onAfterExecute?.(result) without knowing the tool's name. update_plan would declare its own callback in src/tools/plan.ts, where the special-casing belongs.

This is more of an "altitude" finding than a bug — not blocking — but the pattern won't scale.

[Vasanthdev2004]

Efficiency — highlightCode re-tokenizes on every call.

The new codeToTokensBase path runs the full Shiki tokenizer → hex→ANSI conversion on every invocation. MessageRenderer.tsx's useEffect calls it once per code block per render. During a streaming assistant message, the useEffect([content]) re-fires on every chunk, re-tokenizing every existing block. For a 500-line code block this is 50-150ms per chunk on the main thread.

Also: there's no cancellation in the consumer. If content changes mid-stream, an in-flight highlight from the previous render can finish and call setHighlightedBlocks with stale data, clobbering the newer result.

Fix: wrap highlightCode in a module-level Map<string, Promise<string>> keyed on ${lang}:${code.length}:hash(code). Cancel stale work with an AbortController inside the useEffect's cleanup.

Not blocking, but for any non-trivial code block this becomes the dominant cost during streaming.

[Vasanthdev2004]

Re-review of Fix PR review findings (bc0127b).

Checked out the new commit, restored the lockfile + package.json so react-devtools-core is actually installed, ran the full validation:

• bun test → 25 pass, 0 fail (was 23; +2 from the new regression tests for the streaming-auth and unrelated-unknown-parameter cases)
• bunx tsc --noEmit → clean
• bun run build → ✅ produces zero.exe, ./zero --version returns 0.1.0. The build is fixed — the lockfile is regenerated and the dep is installed.

Status of the 11 findings from the first review

All 11 are addressed, with concrete implementations and (where the bug was correctness-critical) regression tests:

#	Finding	Fix
1	Multi-turn text drop	streamingMessageIndexRef set synchronously, read in onText (App.tsx:212, 248)
2	Streaming 401 prefix lost	throw formatProviderCreateError(error) in streaming catch (openai.ts:189) — and a new test asserts the prefix
3	isUnsupportedStreamOptionsError overbroad	Tightened to /stream[ _-]?options?/ (openai.ts:29) — and a new test asserts non-retry on tool_choice 400
4	setState-in-setState-updater	onPlanUpdate now calls setPlan(nextPlan) directly with side effects via refs (App.tsx:256-263)
5	Double-prefix after retry	Subsumed by #2 — single categorization site
6	/todo stale message	toggleTodoRail now returns 'shown' | 'hidden' (App.tsx:398-411)
7	Todo-rail state race	New todoRailHiddenRef consulted in onPlanUpdate (App.tsx:257)
8	Ctrl+T mid-typing	Guarded with if (!input && …) (App.tsx:141); status-line updated to advertise both forms
9	Header/TodoRail in own files	Extracted to src/tui/Header.tsx and src/tui/TodoRail.tsx. App.tsx shrunk from 714 → 649 lines.
10	tc.name === 'update_plan' special case	New Tool.onAfterExecute hook (types.ts:8, loop.ts:208, plan.ts:63-65) — clean abstraction
11	highlightCode no memo / no cancel	Module-level highlightCache Map (highlighter.ts:5) + cancelled flag in MessageRenderer useEffect (MessageRenderer.tsx:32, 46-50)

The onAfterExecute abstraction in particular is exactly the altitude fix I was hoping for. Nicely done.

One new issue introduced by the fix

The new setPlanUpdateHandler cross-cutting plan-notification mechanism in plan.ts:78-82 + loop.ts:46-48, 226-228 has a closure-capture leak that breaks the "no plan updates across separate runAgent calls" property.

// loop.ts:46-48
const previousPlanUpdateHandler = setPlanUpdateHandler(() => {
  onPlanUpdate?.(getCurrentPlan());   // ← captures THIS call's onPlanUpdate
});
// ... later in finally:
setPlanUpdateHandler(previousPlanUpdateHandler);

The arrow function passed to setPlanUpdateHandler closes over the current call's onPlanUpdate. When a second runAgent call (without onPlanUpdate) runs and finishes, the finally block restores previousPlanUpdateHandler — which is the first call's closure, not undefined. So the second call's plan updates still fire the first call's callback, even though the first runAgent has long since returned.

I confirmed this with a probe test: two sequential runAgent calls (first with onPlanUpdate, second with {}) — the first call's callback fires when the second call's update_plan runs. The "restore" works, but it restores the wrong thing.

Fix sketch (one option): keep the handler as a stable indirection and re-point it each call:

// in plan.ts:
let currentHandler: (() => void) | undefined;
let onPlanUpdated = () => currentHandler?.();
export function setPlanUpdateHandler(handler: (() => void) | undefined) {
  currentHandler = handler;
}

// in loop.ts:
const previousHandler = setPlanUpdateHandler; // capture indirection, not closure
// ... finally:
previousHandler(undefined);

Or — simpler and my preferred version — drop setPlanUpdateHandler entirely and have the loop call onPlanUpdate?.(getCurrentPlan()) directly after tool?.onAfterExecute?.(result) in the toolPromises.map (loop.ts:208). onPlanUpdate is already a per-runAgent parameter; the global handler is unnecessary indirection. The onAfterExecute hook on the tool is the right abstraction; the global is extra surface area.

Minor leftover (not blocking)

configManager.getActiveProvider() is still called on every render at App.tsx:121 (was a pre-existing concern I noted in my first analysis). If the user changes providers via /provider, the header doesn't refresh until the next render trigger. Not a regression — same behavior as before this PR — but worth a follow-up.

Verdict

The r2 commit turns a "has a critical bug" PR into a "has one newly-introduced bug + everything else lands cleanly" PR. Fix the setPlanUpdateHandler leak (or, better, remove it as suggested above) and this is good to merge. The two regression tests already in the file should catch any future regression of #2 and #3.