feat: add Go M0 provider config and TUI foundations#52
Conversation
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughThis PR migrates agent types and streaming to internal/zeroruntime, adds CollectStreamWithOptions with live callbacks, updates the agent run loop to use zeroruntime collection and tool-definition mapping, implements an OpenAI-compatible streaming provider, extends configuration resolution and provider-command loading (with redaction and platform termination), and adds a Bubble Tea TUI with tests. ChangesZeroruntime Type and Stream Contract
Agent Loop Integration and Tool Schema
Providers, Config, CLI, and TUI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/zeroruntime/helpers.go (1)
40-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSurface cancellation in collected error state.
This branch returns partial output with an empty
Error, so cancellation can be interpreted as success by callers that only inspectCollectedStream.Error.💡 Proposed fix
case <-ctx.Done(): appendOpenToolCalls(&collected, toolCallOrder, pendingToolCalls) + collected.Error = ctx.Err().Error() return collected🤖 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/zeroruntime/helpers.go` around lines 40 - 42, The cancellation branch currently returns a partial CollectedStream (variable collected) with no error, which can be misinterpreted as success; update the ctx.Done() branch in the function containing appendOpenToolCalls(&collected, toolCallOrder, pendingToolCalls) so that before returning you set collected.Error (or the appropriate error field on the CollectedStream) to ctx.Err() (or a wrapped/contextualized error indicating cancellation) to surface cancellation to callers; ensure you only mutate the existing collected struct and keep appendOpenToolCalls usage unchanged.
🤖 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/zeroruntime/helpers.go`:
- Around line 40-42: The cancellation branch currently returns a partial
CollectedStream (variable collected) with no error, which can be misinterpreted
as success; update the ctx.Done() branch in the function containing
appendOpenToolCalls(&collected, toolCallOrder, pendingToolCalls) so that before
returning you set collected.Error (or the appropriate error field on the
CollectedStream) to ctx.Err() (or a wrapped/contextualized error indicating
cancellation) to surface cancellation to callers; ensure you only mutate the
existing collected struct and keep appendOpenToolCalls usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bac90b9e-710e-4168-8716-665ca1933622
📒 Files selected for processing (7)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/app.gointernal/zeroruntime/helpers.gointernal/zeroruntime/provider_test.gointernal/zeroruntime/types.go
Vasanthdev2004
changed the title
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
zeroruntimeis now the single source of truth.agent.Message,agent.Provider,agent.ToolCall,agent.Usageare all type aliases forzeroruntime.*(internal/agent/types.go:8-12), and the loop is built onzeroruntime.SeedMessages+zeroruntime.CollectStreamWithOptions(internal/agent/loop.go:36-50). Stream event names were also normalised to kebab-case (internal/zeroruntime/types.go:18-20) and locked behind a name-matches-contract test (TestStreamEventNamesMatchProviderContract,internal/zeroruntime/provider_test.go:46-58). ThecollectTurn/pendingToolCall/buildToolCallstriplet from #51 has been replaced by a single, well-tested collector withensurePendingToolCallhandling missing-Start resilience.- OpenAI provider is built for real SSE.
provider.stream(internal/providers/openai/provider.go:42-116) usesbufio.Scannerwith a 16 MB max line buffer, a privatetoolStatethat buffers per-index tool calls until bothidandnamearrive, and emits the correct start → delta(s) → end → done sequence. Three targeted tests prove the ordering (TestStreamCompletionBuffersToolArgsUntilIDAndNameArrive,TestStreamCompletionTracksMultipleToolCallsByIndex,TestStreamCompletionDoesNotHangOnEOFWithOpenToolCall). Usage parsing handlesprompt_tokens_details.cached_tokens, and thesendEventhelper drops non-error events onctx.Done(with a non-blocking last-chance send for errors), so cancellation can't deadlock the consumer. - Error classification + redaction is layered.
classifyError(provider.go:159-172) maps 401/403/429/4xx toauth error:,rate limit error:,provider request error:prefixes; everything else isprovider error:.redactdoes a literal replace of the configuredapiKeyand a word-by-word sweep forBearer <token>(tested inTestStreamCompletionClassifiesHTTPErrorsAndRedactsToken). Same redaction discipline is applied ininternal/config/redact.go(regexes forsk-...,api[_-]?key=,Bearer ...) and exercised byTestResolveRedactsSecretsFromErrors/TestLoadProviderCommandFailureIncludesExitAndRedactsOutput. - Config resolver is genuinely layered and well-typed.
Resolve(internal/config/resolver.go:8-46) walksUserConfigPath→ProjectConfigPath→ProviderCommand→ env → overrides in that order, withmergeProfilefilling only empty fields so later layers additively extend.ProviderProfile.UnmarshalJSONaccepts both camelCase (apiKey,model) and snake_case (api_key,model_id,base_url) field names (internal/config/types.go:39-65), so a TypeScript-style config continues to parse. Env fallback (OPENAI_API_KEY/OPENAI_BASE_URL/OPENAI_MODEL/ZERO_PROVIDER) auto-promotes a custombase_urltoProviderKindOpenAICompatible(resolver.go:81-93), and the sameisOfficialOpenAIBaseURLcheck is applied both ways so anopenai-compatibleprovider can't accidentally point at the official URL. - Provider command is process-managed and bounded.
LoadProviderCommandruns the configured shell command with a 5-secondtime.NewTimertimeout, kills the process group on POSIX (Setpgid: true+syscall.Kill(-pid, SIGKILL),internal/config/process_posix.go) or the whole process tree on Windows (taskkill /T /F,internal/config/process_windows.go). TestTestLoadProviderCommandTimeoutasserts the wall-clock elapsed stays under 7 s. The shell invocation pre-calls Windows commands that start with a leading quote (command.go:53-56), and the stderr output is funnelled throughredactSecretsbefore being included in the error. - TUI is a clean Bubble Tea shell.
model.gofollows the Elm pattern with atranscriptreducer (internal/tui/transcript.go:48-71) that produces a new slice on every action, a separateparseCommandfor/help /clear /exit /tools /permissions(internal/tui/commands.go:21-46), and anOnToolCall/OnToolResultwrapper inrunAgent(internal/tui/model.go:158-181) that records rows without breaking the user's callbacks. Esc clears the input +pendingflag; Ctrl+C quits viatea.Quit. The PR body is honest that the TUI isn't wired to CLI startup yet — the next PR can route no-argzerototui.Runonce the resolver+provider wiring lands. - Agent loop got simpler with the unification.
loop.go:23-99is now ~80 lines instead of 100, and the test file grew (TestRunAdvertisesRuntimeToolDefinitions,loop_test.go:108-149) to cover thetools.Schema→map[string]anyconversion viaschemaToRuntimeMap/propertyToRuntimeMap(loop.go:147-184). This is a worthwhile test — without it, a regression in the schema mapper would silently strip tool descriptions before the provider sees them. - Three test files give the resolver / command / provider serious coverage.
resolver_test.go(7 tests) covers layer precedence, active profile selection, env fallback, official-URL normalisation, validation errors, and redaction.command_test.go(5 tests) covers success, exit-code-with-redaction, timeout wall-clock, invalid JSON, and missing model.provider_test.go(11 tests) covers request shape, model requirement, empty auth/tools, text+usage+done, tool-args buffering, multi-tool tracking, error classification, malformed JSON, context cancel, and EOF-without-DONE.
Observations (non-blocking)
-
CodeRabbit's nitpick is correct:
CollectStreamWithOptionsdoesn't surface cancellation.internal/zeroruntime/helpers.go:40-42returnscollectedwithError == ""whenctx.Done()fires, so a caller that only inspectsCollectedStream.Errorinterprets a cancelled stream as success. One-line fix:collected.Error = ctx.Err().Error()before theappendOpenToolCalls+ return. Worth addressing in this PR before it ships — the agent loop's check oncollected.Error != ""(internal/agent/loop.go:46-50) would then propagate the cancellation as a proper error to the caller, and the TUI /zero execCLI would render a useful "context canceled" line instead of an emptyFinalAnswer. -
The OpenAI provider
redactis approximate forBearertokens. It splits on whitespace and looks for a word equal-folding toBearer(with optional trailing:), then replaces the next word. That works forAuthorization: Bearer sk-...in normal log lines, but a token likeBearer\nsk-...(rare but possible) or abearerheader value buried mid-string would slip through. For a v1 this is fine; a follow-up using a single regex pass ((?i)\bBearer\s+\S+) would be tighter. -
redact.go's prefix preservation has a gap. The prefix list is{"apiKey=", "api_key=", "api-key=", "Bearer "}, but the regex capturesapi[_-]?keyfollowed by a delimiter (which also matchesapi key=...with a space). If the input isapi key=secret, the regex matches but no prefix in the list matches, so the match is fully replaced with[REDACTED]and the originalapi key=is lost. Adding"api key="to the prefix list closes it. -
Provider command timeout is a hard-coded 5 s.
LoadProviderCommand(internal/config/command.go:10) uses a package constant. A custom provider that does any IO (e.g. fetches a token) could exceed that. Worth making it configurable viaResolveOptions.ProviderCommandTimeout(or similar) so users on slow systems don't have to fork the code. -
The TUI doesn't show running tool calls.
model.runAgent(internal/tui/model.go:158-181) recordsOnToolCallrows into a localrowsslice and only flushes them when the agent returns. While a long-running tool (e.g.bashwith the full 10-minute timeout) is in flight, the user seesassistant: working...and no indication of which tool is running. The fix is to surface the rows incrementally: either via intermediateagentResponseMsgflushes or by switching thetea.Cmdto a stream that yields after each tool. For an M0 TUI this is acceptable; flag it before the TUI replaceszero execas the default entry point. -
TUI transcript has no upper bound. A 100-turn session would append hundreds of rows;
View()rebuilds the full string on every render, and the row slice grows unboundedly. Bubble Tea'sviewportcomponent handles scrollback, but the currentmodel.godoesn't use one. A max-row cap (e.g. 1000) with truncation, or aviewportmigration, is the right next step. -
runAgentwraps the user'sOnToolCall/OnToolResultcallbacks, but the fullToolResult.Outputis rendered into arowToolResult. Aread_fileof a 10 MB log file would dump 10 MB into a single row, which then has to fit in the transcript and re-render on every keystroke. The same row-cap fix in #6 would help, or the TUI could truncate the renderedOutputto a fixed number of bytes with a… (truncated)suffix and the user could expand it via a hotkey. -
applyEnvoverrides theactiveProviderfromOPENAI_*env. When onlyOPENAI_API_KEYis set (withoutOPENAI_BASE_URLorOPENAI_MODEL), the code returns early (resolver.go:79-83), which is correct. But when an env profile is created, it hasName = cfg.ActiveProvider— i.e. it inherits whatever the user set viaZERO_PROVIDERor earlier layers. If the user has a custom-named provider in their config file and then setsOPENAI_API_KEYin the environment, the env key gets bolted onto the custom-named provider rather than creating a separateopenaiprofile. Probably the right call, but worth a comment in the code so the merge behaviour is explicit. -
No migration note for the kebab-case event rename.
#47–#51usedtool_call_start(snake_case) and#52switches totool-call-start(kebab-case). Any third-party provider or test that was constructingStreamEvent{Type: "tool_call_start", ...}will break. The test inTestStreamEventNamesMatchProviderContractwill catch a future rename, but a one-line note in the PR description (or a CHANGELOG entry) would be useful since the rename spans three event types and the previous event names were in the publiczeroruntimetypes. -
offlineProviderininternal/cli/app.gostill useszeroruntime.MessageRoleUser— confirmed in the diff. The CLI'srunExecnow receives a real resolver-built provider, soofflineProvideris dead code in this PR (the CLI doesn't wire it up). A quickgit grepcheck or a//nolintwill confirm whether anything still references it; if not, removing it (or moving it under a//go:build offline_testtag) is a clean follow-up. -
go.modtoolchain bump togo1.24.13is fine, but worth checking downstream. Anyone building Zero on a Go version < 1.24.2 (i.e. before this PR) will now get ago.modtoolchain mismatch. Thetoolchaindirective is the right answer, but a one-line note in the PR description ("requires Go 1.24.2+") and a CI matrix pin to 1.24.2 minimum would make the floor explicit.
No blockers. The CodeRabbit nitpick (#1) is the one I'd want to see fixed before merge — it's a one-line change that closes a real cancellation-surfaces-as-success bug. The rest are correctness / UX polish for the follow-up PRs (TUI wiring, real-provider command timeout, transcript scrollback).
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/config/resolver.go`:
- Around line 67-69: Trim src.ActiveProvider before checking emptiness and only
copy a non-empty trimmed value into dst.ActiveProvider: replace the current if
src.ActiveProvider != "" { dst.ActiveProvider =
strings.TrimSpace(src.ActiveProvider) } with code that does v :=
strings.TrimSpace(src.ActiveProvider); if v != "" { dst.ActiveProvider = v } and
make the same change for the second occurrence around the src.ActiveProvider
handling (lines ~158-160) so whitespace-only values no longer overwrite a valid
dst.ActiveProvider.
In `@internal/config/types.go`:
- Around line 77-79: The current assignments (e.g., profile.BaseURL =
strings.TrimSpace(firstNonEmpty(raw.BaseURL, raw.BaseURLSnake))) pass untrimmed
raw fields into firstNonEmpty so whitespace-only values can win over valid
aliases; change to trim each candidate before selecting the first non-empty one
(e.g., call strings.TrimSpace on raw.BaseURL and raw.BaseURLSnake before calling
firstNonEmpty) and apply the same change for APIKey/APIKeySnake and
Model/ModelID so that firstNonEmpty operates on trimmed strings rather than on
potential whitespace-only inputs.
In `@internal/providers/openai/provider.go`:
- Line 107: Replace the plain defer response.Body.Close() with an explicit
deferred closure that handles the Close() error to satisfy errcheck; e.g. after
the HTTP call that assigns response, use defer func() { if err :=
response.Body.Close(); err != nil { /* log or ignore intentionally */ } }() so
the returned error is consumed (reference: response.Body.Close in
internal/providers/openai/provider.go).
In `@internal/providers/openai/tool_state.go`:
- Around line 78-87: The closeOpen finalization loop should skip any tool calls
that lack a name to avoid emitting a StreamEventToolCallStart with an empty
ToolName; update the guard around the call so that it also checks call.name ==
"" (or add an explicit check before sending the start event) and continue when
the name is empty, referencing the same loop where call, call.started and
sendEvent(ctx, events, zeroruntime.StreamEvent{...}) are used to prevent
emitting a start event for nameless calls.
In `@internal/tui/model.go`:
- Around line 102-105: The Esc key handler clears input and unsets m.pending
without cancelling an active agent run, and the prompt-start code (e.g., the
method that begins a run—look for startPrompt/runPrompt or where a new prompt
run is initiated) doesn’t guard against an existing pending run; modify KeyEsc
handling to signal cancellation of the active run (e.g., set/close a cancel
channel or cancel a context tied to the current run) instead of just clearing
m.pending, and update the prompt-start path to check m.pending (or attempt to
acquire a run lock) before starting a new run so it returns early when a run is
already in progress; ensure the run sets m.pending = true at start and clears it
only when the run fully terminates (or on cancel) to prevent interleaved runs.
- Around line 210-215: In runAgent the options.OnToolResult handler appends a
transcriptRow using result.Status directly, which can be empty; change the
handler (options.OnToolResult closure) to normalize an empty status to "ok"
before formatting (e.g. if result.Status == "" then use "ok") so the
transcriptRow for rowToolResult always shows a consistent status value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 000ed0cd-446f-4640-a069-d1051486e3c9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
go.modinternal/config/command.gointernal/config/command_test.gointernal/config/process_posix.gointernal/config/process_windows.gointernal/config/redact.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/tool_state.gointernal/providers/openai/types.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/options.gointernal/tui/run.gointernal/tui/transcript.go
✅ Files skipped from review due to trivial changes (2)
- internal/tui/options.go
- internal/tui/model_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
internal/zeroruntimethe canonical Go provider/message/event contract and route the Go agent loop through it.Validation
go test ./...bun run typecheckbun test ./tests --timeout 15000— 279 pass, 0 failbun run buildbun run smoke:buildbun run build:gobun run smoke:goNotes
zero.exeon Windows.zerototui.Runonce this foundation lands.Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Reliability
Tests