feat: wire Go CLI to real provider flow#54
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new ChangesCLI exec command and DI refactoring
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI.Run/runExec
participant Parser as parseExecArgs
participant Workspace as resolveWorkspaceRoot
participant Prompt as resolveExecPrompt
participant Config as resolveConfig
participant Factory as providers.New
participant Agent as agent.Run
participant Writer as execEventWriter
CLI->>Parser: parse flags & args
Parser->>Workspace: resolve --cwd
Parser->>Prompt: build prompt (inline + file)
CLI->>Config: resolveConfig(workspace)
Config-->>CLI: provider profile
CLI->>Factory: New(profile, Options)
Factory-->>CLI: provider
CLI->>Agent: Run(prompt, registry, provider)
Agent->>Writer: stream completion events
Writer-->>CLI: stdout/stderr or NDJSON
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.
Actionable comments posted: 6
🧹 Nitpick comments (2)
internal/config/resolver_test.go (1)
169-187: ⚡ Quick winAdd a regression case for active-provider-without-profiles.
The new test at Line 170 only covers fully empty config. Please also cover
activeProviderset with zero providers, so misconfiguration handling stays explicit.Suggested test
func TestResolveAllowsNoConfiguredProviders(t *testing.T) { @@ } + +func TestResolveRejectsActiveProviderWithoutConfiguredProfiles(t *testing.T) { + path := writeConfig(t, `{"activeProvider":"ghost","providers":[]}`) + + _, err := Resolve(ResolveOptions{ProjectConfigPath: path, Env: map[string]string{}}) + if err == nil { + t.Fatal("Resolve() error = nil, want missing active provider error") + } + if !strings.Contains(err.Error(), `active provider "ghost" not found`) { + t.Fatalf("error = %q, want active provider missing message", err.Error()) + } +}🤖 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/config/resolver_test.go` around lines 169 - 187, Add a new unit test (e.g., TestResolveActiveProviderWithoutProfiles) that calls Resolve(ResolveOptions{Env: map[string]string{"ACTIVE_PROVIDER": "foo"}}) with no configured providers and asserts that Resolve returns an error (or at least does not set a valid active provider) and that resolved.ActiveProvider is empty, resolved.Providers is empty, resolved.Provider equals the zero-value ProviderProfile, and resolved.MaxTurns equals defaultMaxTurns; reference the Resolve function, ResolveOptions type, ActiveProvider field, ProviderProfile type, and defaultMaxTurns in the test to locate and validate the behavior.internal/cli/exec_test.go (1)
20-43: ⚡ Quick winAdd a regression test for
exec --helpcombined with extra args.Please add a case like
zero exec --help --model m1asserting help output and no runtime/provider execution. This protects the parser behavior from silently regressing.🤖 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/cli/exec_test.go` around lines 20 - 43, Add a regression case to TestRunExecHelpDocumentsM1Flags that calls Run with extra args (e.g., Run([]string{"exec","--help","--model","m1"}, &stdout, &stderr)) and asserts the same behavior as the existing help-only case: exit code 0, stdout contains the help flags (-f/--file, -m/--model, etc.), and stderr is empty; use the same stdout/stderr buffers and checks around Run (the Run function is the CLI entry used here) so the parser shows help without invoking runtime/provider execution.
🤖 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/cli/app.go`:
- Around line 187-198: Update the hardcoded usage string printed in the exec
command help (the fmt.Fprint call in internal/cli/app.go) to reflect that the
prompt is optional and that input can come from --file or --prompt flags; change
the usage line from "zero exec [flags] <prompt>" to something like "zero exec
[flags] [<prompt>]" or "zero exec [flags] [prompt]" and add/mention the --prompt
flag in the Flags section alongside -f, --file so the help matches actual parser
behavior in the exec command implementation.
In `@internal/cli/exec.go`:
- Around line 98-114: The pre-run writer.runStart call can set writer.err (e.g.,
broken stdout/stderr) but the code proceeds to call agent.Run; detect writer.err
immediately after writer.runStart (and after writer.warning if
options.skipPermissionsUnsafe) and short-circuit by returning exitCrash instead
of invoking agent.Run. Update the control flow around writer.runStart /
options.skipPermissionsUnsafe to check writer.err and return early, referencing
writer.runStart, writer.err, and agent.Run to locate the change.
- Around line 135-140: The help flag currently only returns when it's the sole
arg, causing "exec --help --model x" to be treated as a prompt; change the
help-handling branch in the argument parsing switch (the case handling arg ==
"-h" || arg == "--help" || arg == "help") to treat any occurrence of a help flag
as a request for help: immediately return options, true, nil (or set a dedicated
showHelp flag on options and return after parsing) instead of appending to
options.promptParts; update references in the same parsing function where
options.promptParts is modified so help never falls through to execute provider
calls.
- Around line 287-299: The JSON branch in writeExecProviderError ignores errors
from writeJSONLine and always returns exitProvider; change it to capture and
check the error returned by each writeJSONLine call (the two calls that write
the "error" and "done" JSON objects) and if any write returns a non-nil error,
return exitCrash instead of exitProvider; otherwise, keep returning
exitProvider. Ensure you reference writeExecProviderError, writeJSONLine,
exitProvider and exitCrash when applying the change.
In `@internal/config/paths.go`:
- Line 30: Trim whitespace from the ZERO_PROVIDER_COMMAND environment variable
before storing it in the configuration so whitespace-only values become empty;
update the assignment for ProviderCommand (the struct field) to use
strings.TrimSpace(os.Getenv("ZERO_PROVIDER_COMMAND")) so later logic treating
empty vs non-empty commands behaves correctly.
In `@internal/config/resolver.go`:
- Around line 183-185: The current code returns success when providers is empty
regardless of activeName, which hides config errors; update the providers-empty
branch to check if activeName (or activeProvider) is set and, if so, return an
error instead of nil success; specifically modify the block that returns
[]ProviderProfile{}, ProviderProfile{}, nil to return a descriptive error when
activeName != "" (otherwise keep returning the empty slice and zero-value
ProviderProfile with nil error) so callers can surface misconfigured active
provider names.
---
Nitpick comments:
In `@internal/cli/exec_test.go`:
- Around line 20-43: Add a regression case to TestRunExecHelpDocumentsM1Flags
that calls Run with extra args (e.g.,
Run([]string{"exec","--help","--model","m1"}, &stdout, &stderr)) and asserts the
same behavior as the existing help-only case: exit code 0, stdout contains the
help flags (-f/--file, -m/--model, etc.), and stderr is empty; use the same
stdout/stderr buffers and checks around Run (the Run function is the CLI entry
used here) so the parser shows help without invoking runtime/provider execution.
In `@internal/config/resolver_test.go`:
- Around line 169-187: Add a new unit test (e.g.,
TestResolveActiveProviderWithoutProfiles) that calls Resolve(ResolveOptions{Env:
map[string]string{"ACTIVE_PROVIDER": "foo"}}) with no configured providers and
asserts that Resolve returns an error (or at least does not set a valid active
provider) and that resolved.ActiveProvider is empty, resolved.Providers is
empty, resolved.Provider equals the zero-value ProviderProfile, and
resolved.MaxTurns equals defaultMaxTurns; reference the Resolve function,
ResolveOptions type, ActiveProvider field, ProviderProfile type, and
defaultMaxTurns in the test to locate and validate the behavior.
🪄 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 Plus
Run ID: 8d6341af-0c4d-4a42-abb7-9ab7ecda0629
📒 Files selected for processing (10)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/exec.gointernal/cli/exec_test.gointernal/config/paths.gointernal/config/paths_test.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/providers/factory.gointernal/providers/factory_test.go
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. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
I rechecked current head 83f7b8d9870b2b84aee95fc7a295c6aab85e7b89 locally. The Go test suite is green, but there are still runtime/config blockers that should be fixed before merge.
Findings:
internal/cli/exec.go:135:zero exec --help --model testis still parsed as a prompt because help only short-circuits when it is the only exec arg. I reproduced this with a direct CLI probe: instead of printing help, it continued into provider setup and failed with a provider/auth error. Help should win whenever-h/--helpis present in the exec args, even with other flags.internal/config/resolver.go:182: an explicit active provider with no provider profiles is silently accepted as an empty provider config. For example,ZERO_PROVIDER=ghostwith no matching provider ends up as the generic “No provider configured” path instead of reporting that the active provider is invalid/missing. IfactiveNameis non-empty andprovidersis empty,normalizeProvidersshould return an error.internal/config/paths.go:30:ZERO_PROVIDER_COMMANDis passed through without trimming. WithZERO_PROVIDER_COMMAND=' ', the resolver attempts to execute a blank command and surfacesprovider command returned empty JSON. Trim the env value before assigning/using it so whitespace-only values behave like unset.internal/cli/exec.go:98: startup JSON/warning write failures are not checked until afteragent.Run, so a broken/unwritable output stream can still trigger the provider call and spend tokens before the crash is returned. Checkwriter.errimmediately afterrunStart/warningand before entering the agent loop.internal/cli/exec.go:287: the JSON provider-error path ignoreswriteJSONLinefailures and still returnsexitProvider. If either JSON error/done write fails, this should returnexitCrash, consistent with the rest of the writer error handling.
Validation run:
go test ./internal/cli ./internal/config ./internal/providerspassedgo test ./...passedgit diff --check origin/main...HEADpassed- Direct CLI probes reproduced the help parsing and whitespace provider-command issues above
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/cli/exec_test.go (1)
377-379:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid panic-prone payload assertions in test validation.
Line 377 and Line 378 can panic on payload shape changes; prefer checked assertions so failures stay deterministic and readable.
Proposed patch
- messages := gotBody["messages"].([]any) - lastMessage := messages[len(messages)-1].(map[string]any) - if lastMessage["content"] != "hello provider" { + messages, ok := gotBody["messages"].([]any) + if !ok || len(messages) == 0 { + t.Fatalf("messages = %#v, want non-empty []any", gotBody["messages"]) + } + lastMessage, ok := messages[len(messages)-1].(map[string]any) + if !ok { + t.Fatalf("last message = %#v, want map[string]any", messages[len(messages)-1]) + } + if lastMessage["content"] != "hello provider" { t.Fatalf("last provider message = %#v, want prompt", lastMessage) }🤖 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/cli/exec_test.go` around lines 377 - 379, The test currently uses bare type assertions on gotBody["messages"] and lastMessage which can panic if the JSON shape changes; replace them with safe, checked assertions (e.g., use the "ok" comma-ok form or testing helpers like require/ assert) to verify that gotBody["messages"] is a []any, that len(messages) > 0, and that the last element is a map[string]any before accessing "content"; then assert the content equals "hello provider" with a descriptive failure message. Target the variables/messages handling in exec_test.go (gotBody, messages, lastMessage) and change the assertions to fail deterministically rather than panic.
🧹 Nitpick comments (1)
internal/cli/exec_test.go (1)
334-341: ⚡ Quick winAssert request method and endpoint path in the provider integration test.
This handler currently accepts any route/method, so a request-routing regression can still pass this test. Add explicit assertions for method and path suffix.
Proposed patch
- var gotAuth string + var gotAuth string + var gotMethod string + var gotPath string var gotBody map[string]any server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotMethod = r.Method + gotPath = r.URL.Path gotAuth = r.Header.Get("Authorization") if err := json.NewDecoder(r.Body).Decode(&gotBody); err != nil { t.Fatalf("decode provider request: %v", err) } _, _ = io.WriteString(w, "data: {\"choices\":[{\"delta\":{\"content\":\"provider ok\"}}]}\n\n") _, _ = io.WriteString(w, "data: [DONE]\n\n") })) @@ if gotAuth != "Bearer sk-local" { t.Fatalf("Authorization = %q, want project config token", gotAuth) } + if gotMethod != http.MethodPost { + t.Fatalf("method = %q, want %q", gotMethod, http.MethodPost) + } + if !strings.HasSuffix(gotPath, "/chat/completions") { + t.Fatalf("path = %q, want suffix /chat/completions", gotPath) + }Also applies to: 371-373
🤖 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/cli/exec_test.go` around lines 334 - 341, The test HTTP handler used in exec_test.go accepts any method/path—add assertions inside the httptest server handler to check r.Method equals the expected method (e.g., "POST") and that r.URL.Path or r.URL.String() has the expected suffix (the provider endpoint) before decoding the body; update both occurrences (the handler around gotAuth/gotBody at the shown snippet and the similar handler at lines ~371-373) to fail the test (t.Fatalf) when method or path mismatches so request-routing regressions are caught.
🤖 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/cli/exec_test.go`:
- Around line 377-379: The test currently uses bare type assertions on
gotBody["messages"] and lastMessage which can panic if the JSON shape changes;
replace them with safe, checked assertions (e.g., use the "ok" comma-ok form or
testing helpers like require/ assert) to verify that gotBody["messages"] is a
[]any, that len(messages) > 0, and that the last element is a map[string]any
before accessing "content"; then assert the content equals "hello provider" with
a descriptive failure message. Target the variables/messages handling in
exec_test.go (gotBody, messages, lastMessage) and change the assertions to fail
deterministically rather than panic.
---
Nitpick comments:
In `@internal/cli/exec_test.go`:
- Around line 334-341: The test HTTP handler used in exec_test.go accepts any
method/path—add assertions inside the httptest server handler to check r.Method
equals the expected method (e.g., "POST") and that r.URL.Path or r.URL.String()
has the expected suffix (the provider endpoint) before decoding the body; update
both occurrences (the handler around gotAuth/gotBody at the shown snippet and
the similar handler at lines ~371-373) to fail the test (t.Fatalf) when method
or path mismatches so request-routing regressions are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2136131-e895-4eb7-948b-21ab783c3447
📒 Files selected for processing (7)
internal/cli/app.gointernal/cli/exec.gointernal/cli/exec_test.gointernal/config/paths.gointernal/config/paths_test.gointernal/config/resolver.gointernal/config/resolver_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/config/paths_test.go
- internal/config/resolver_test.go
- internal/cli/exec.go
- internal/config/paths.go
- internal/cli/app.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approved current head 608c1d38a8649405e78af9867fa72e57c8c95207.
I rechecked the requested-change items from my prior review. The exec help path no longer falls through to provider execution, pre-run writer errors now short-circuit before agent.Run, JSON provider-error writes return exitCrash on I/O failure, ZERO_PROVIDER_COMMAND is trimmed, active-provider-without-profiles now errors explicitly, and exec help text matches optional prompt/--prompt behavior. CodeRabbit has no new actionable comments and CI is green.
Validation run:
git diff --check origin/main...HEADgo test ./internal/cli ./internal/config ./internal/providersgo test ./...env -u OPENAI_API_KEY -u OPENAI_MODEL -u OPENAI_BASE_URL -u ZERO_PROVIDER -u ZERO_PROVIDER_COMMAND go run ./cmd/zero exec --help --model testXDG_CONFIG_HOME=/private/tmp/zero-pr54-empty-config ZERO_PROVIDER=ghost go run ./cmd/zero exec helloconfirmed explicit missing active-provider errorXDG_CONFIG_HOME=/private/tmp/zero-pr54-blank-command-config ZERO_PROVIDER_COMMAND=" " go run ./cmd/zero exec helloconfirmed whitespace provider command behaves as unsetPATH=/Users/kratos/.bun/bin:$PATH bun install --frozen-lockfilePATH=/Users/kratos/.bun/bin:$PATH bun run typecheckPATH=/Users/kratos/.bun/bin:$PATH bun test ./tests --timeout 15000(279 pass / 0 fail)PATH=/Users/kratos/.bun/bin:$PATH bun run buildPATH=/Users/kratos/.bun/bin:$PATH bun run smoke:buildPATH=/Users/kratos/.bun/bin:$PATH bun run build:goPATH=/Users/kratos/.bun/bin:$PATH bun run smoke:go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
appDepsis a clean DI seam for the CLI.appDeps{ getwd, resolveConfig, newProvider, runTUI }(internal/cli/app.go:25-31) plusfillAppDeps(internal/cli/app.go:104-119) means every test can swap in a fake for any subset of the four collaborators without reaching for monkey-patching. The test surface —TestRunNoArgsLaunchesTUIWithNilProviderWhenNoProviderConfigured,TestRunNoArgsLaunchesTUIWithResolvedProviderMetadata,TestRunNoArgsReportsConfigErrorsWithoutLaunchingTUI,TestRunCommandsDoNotLaunchTUI— exercises all four branches and proves the seam doesn't leak (TUI is only invoked whenbuildProviderresolves, the rightProviderName/ModelNameflow intotui.Options, andassertCoreRegistryconfirms the 9-tool core set is wired).zerowith no args launches the TUI,zero execandzero -proute to the same headless runner. Thecase "-p" | "--prompt"handler inapp.go:72-78rewrites intoexecArgsand delegates torunExec, so there's exactly one prompt-parser / provider-construction path.TestRunPromptFlagRoutesToExecRunner(internal/cli/exec_test.go:185-208) verifies the stdout/stderr/exit-code are bit-identical betweenexec hello zeroand-p hello zero.parseExecArgshandles every flag form the help text advertises.-f/--file,-m/--model,-C/--cwd,-o/--output-format text|json,--prompt,--skip-permissions-unsafe, and the--terminator that flushes the rest as positional prompt parts (internal/cli/exec.go:81-152). Both--flag valueand--flag=valueare supported. The--terminator is a nice touch forzero exec -- "--file foo.txt"style invocations.- Text and JSON output share the same callback set with no logic duplication.
execEventWriter(internal/cli/exec.go:155-269) routestextto stdout deltas in text mode and to{"type":"text","delta":...}lines in JSON mode; tool calls/results/usage get a symmetric split. The error sticky-flag (writer.erris set on first write failure, all subsequent writes are no-ops) means a closed pipe can't crash mid-stream —TestRunExecJSONRunStartWriteFailureSkipsAgentandTestRunExecJSONProviderErrorWriteFailureReturnsCrashcover both the early (run_start) and late (provider_error) write-failure paths. resolveWorkspaceRootandresolveExecPromptdo real validation before runtime.--cwdrejects non-existent paths and files-not-directories with exit 2 (exec.go:172-187, tested byTestRunExecRejectsInvalidCwdBeforeRuntime).--filerejects missing/empty files with a clear error. The agent runtime is never invoked when these validations fail — verified by thestrings.Contains(... "Go agent runtime ready")negative-assertion in both tests.paths.goadds properos.UserConfigDir()integration with platform-aware test setup.DefaultResolveOptions(workspaceRoot)(internal/config/paths.go:18-44) returns the user config path ($XDG_CONFIG_HOME/zero/config.jsonon Linux,$HOME/Library/Application Support/zero/config.jsonon macOS,%APPDATA%/zero/config.jsonon Windows) plus the project-level$workspaceRoot/.zero/config.json, andsetUserConfigRoot(paths_test.go:140-158) overrides the right env var perruntime.GOOSso the same test runs on all three platforms.- Resolver change reorders env vs. provider-command correctly.
applyEnvnow runs beforeLoadProviderCommand(internal/config/resolver.go:21-37), so a provider command can override env-set keys —TestResolveProviderCommandOverridesEnvProviderFieldsprovesOPENAI_API_KEY=sk-env+ commandapiKey=sk-command→sk-commandwins. The default-ActiveProviderchange (removed the implicitopenai) is the right semantic: zero-config should mean "no provider", not "synthetic OpenAI profile". - Provider factory is a single switch, ready for Anthropic/Gemini adapters to drop in.
providers.New(internal/providers/factory.go:30-46) currently routes bothopenaiandopenai-compatibletoopenai.Newand returnsunsupported provider kindfor anything else. The factory options struct carriesUserAgent(zero/<version>) and a*http.Clientfor test injection — theTestNewCreatesOpenAIProviderWithFactoryOptionstest uses acaptureTransportto assert the request URL,Authorizationheader, andUser-Agentheader all flow through. - End-to-end exec test against a real
httptest.Serveris the right shape.TestRunExecUsesProjectConfigAndOpenAICompatibleProvider(exec_test.go:425-520) writes a.zero/config.jsonwithprovider_kind: openai-compatible, runsRun(["exec", "--cwd", root, "hello provider"]), and asserts: exit 0, stdout equals the SSE-decodedprovider ok,Authorization: Bearer sk-local,POST /v1/chat/completions,model: local-model, and the user prompt is the last message. This catches the most common wiring regression (auth header, model name, message ordering) in one shot. - Exit-code differentiation is solid.
exitSuccess=0,exitCrash=1,exitUsage=2,exitProvider=3(exec.go:14-19). Provider failures get code 3 in both text and JSON mode (with{"type":"done","exit_code":3}in JSON), which lets shell scripts distinguish "no provider configured" from "usage error" from "agent crashed".
Observations (non-blocking)
-
nextFlagValuedoesn't reject flag-shaped values.internal/cli/exec.go:158-164returns the next arg as the value as long as it's not empty, even if it starts with-. Sozero exec --prompt --model gpt-4parses asprompt = "--model", thengpt-4becomes a positional prompt part, giving the user"--model gpt-4"as the prompt with no error. The same gap was closed inscripts/build.tsfrom #53 (parseBuildArgsrejects flag-shaped values viarequireValue); the same one-liner fix would close it here. Worth a follow-up test likeparseBuildArgs(['-o', '-h'])from #53'stests/build-scripts.test.ts:88-91. -
No
--askmode forzero exec.agent.PermissionModehas three values (auto,ask,unsafeperinternal/agent/types.go:14-19), but the CLI exposes only--skip-permissions-unsafeto flip the defaultautointounsafe. The middle tier (ask) — which would be the right default for an interactive user typing prompts into a CLI one-shot — is unreachable. A--permission-mode auto|ask|unsafeflag (or aZERO_PERMISSION_MODEenv) would be the natural complement to the existing config plumbing. -
buildProvider's(nil, nil)"no provider" sentinel is asymmetric withrunExec. The TUI acceptsProvider: niland is expected to surface a "no provider configured" panel;runExecreturnsexitProvider=3with the message "No provider configured. Set OPENAI_MODEL/OPENAI_API_KEY or add .zero/config.json.". A reader ofapp.gohas to follow the control flow to discover which callers tolerate nil. A typedOptionalProvider(or an explicitprovider.IsZero()predicate onProviderProfile) would make the intent self-documenting. -
fillAppDepssilently falls back to production implementations. If a test sets onlyrunTUIandresolveConfig, the other three fields are filled withos.Getwd,config.Resolve(which reads the filesystem and may shell out), andproviders.New(which callsopenai.New). The current tests avoid this by always supplying all four deps, but a future test that forgets one would silently hit real I/O. A test that injects onlyrunTUIand expects an error would surprise the next reader. Either document the pattern ("always inject all four deps") or use a constructor that requires all four. -
The resolver's removed default-
ActiveProvideris a silent behavior change. Old code:ActiveProvider: string(ProviderKindOpenAI)meant an emptycfgresolved to a synthetic OpenAI profile built fromOPENAI_API_KEY/OPENAI_MODEL. New code: empty cfg returnsResolvedConfig{}and the runner reports "no provider configured". This is the right change (no more implicit OpenAI assumption), but any user with env-only config (no~/.config/zero/config.jsonor.zero/config.json) and who upgraded silently loses their setup. The PR body is honest about the scope but doesn't call this out — a one-line note in the PR description or a CHANGELOG entry would prevent a support churn spike. -
--promptat the top level uses a string-append hack.execArgs := append([]string{"--prompt", args[1]}, args[2:]...)(app.go:75-76) is fine but the call then re-entersrunExecwithargs[1:] = execArgs, which means the help text path insiderunExecwon't see"exec"asargs[0](it sees"--prompt"instead). It works becauseparseExecArgsdoesn't checkargs[0], but it would be cleaner to either (a) handle the flag inline inrunWithDepsand build theexecOptionsdirectly, or (b) make the top-level--promptroute through the sameparseExecArgsby reconstructingargsto start withexec. Functionally identical, just a code-shape concern. -
resolveExecPromptjoins inline and file prompts with\n\neven though the help says "Read prompt text from a file". The help line-f, --file <path> Read prompt text from a file(app.go:179-180) reads as "this is the prompt", butresolveExecPrompt(exec.go:190-221) concatenatesinline + "\n\n" + filewhen both are provided.TestRunExecAssemblesInlineAndFilePromptRelativeToCwddocuments the join, but the help text doesn't. Either change the help to say "Append prompt text from a file" or document the behavior with a short example in the help output. -
truncateForStatusproduces a 203-character string but the limit is 200.exec.go:294-300builds a 200-char compact string then appends"...", giving 203 chars total. Trivial, but if the result is fed into a fixed-width terminal or a downstream column-width check, the 3-char overflow could misalign formatting. Either drop the prefix to 197 or rename the constant to make the intent clear. -
JSON event type naming is snake_case; zeroruntime stream events are kebab-case.
{"type":"run_start",...},{"type":"tool_call",...},{"type":"tool_result",...}(exec.go:218-264) are snake_case; PR #52'szeroruntimecontract usestool-call-start/tool-call-delta/tool-call-end(kebab-case). These are different APIs (CLI exec output vs. agent stream), so the inconsistency isn't wrong, but anyone reading both will trip on it. Worth a one-line comment in the event-emitter code explaining the two namespaces. -
No test for what happens when the OpenAI provider returns a real 4xx/5xx.
TestRunExecJSONProviderErrorWriteFailureReturnsCrash(exec_test.go:151-173) tests the case where stderr write fails after a config error. There's no test where the provider itself returns an error (e.g. 401 from thehttptest.Server). ATestRunExecSurfacesProviderAPIErrorthat has the mock SSE server return401 Unauthorizedand asserts the user sees a clearprovider_errorevent + exit code 3 would close the gap. -
runExecdoesn't surfaceOnUsagein text mode, but in JSON mode theusageevent is emitted after the finaltextdelta — which means a long stream may not get usage until after the user has already seen the answer. The order isrun_start→text*→final→doneperTestRunExecJSONOutputsNDJSONEvents. If the provider sends usage mid-stream (some do, for prompt-cache prefill accounting), the current code buffers it. The contract isn't wrong, but a docstring onexecEventWriter.usagewould help the next reader understand the timing. -
appDeps.runTUIreceives acontext.Background().runInteractiveTUI(app.go:122-165) andrunExec(exec.go:43-176) both passcontext.Background()todeps.runTUI/agent.Run. There's no way for a caller (smoke test, CI, wrapper script) to wire in a cancellation context or deadline. Acontext.Contextparameter onRun(withcontext.Background()as the default incmd/zero/main.go) would let downstream tests exercise cancellation. -
--modeloverride doesn't go through the model registry. The flag setsoverrides.Provider.Model(exec.go:68-72), which the resolver then merges. But the registry (#56) hasDefaultModelID = "gpt-4.1"and aRequire(pattern)that validates against known models + aliases. If the user passes--model typo-of-4-1, the resolver accepts it, the provider tries to send it to the API, and the API 4xxs. A registry lookup inrunExec(or in the resolver'sapplyOverrides) would catch this earlier with a friendlier error. -
paths_test.godoesn't test the directory-rejection path under the macOS or Windows env override branches.TestDefaultResolveOptionsRejectsDirectoryConfigPathsusest.Runto parameterize by user/project, but thesetUserConfigRoothelper covers all three platforms. A test that setsHOMEon darwin + a directory at$HOME/zero/config.jsonand asserts the same error message would lock in the platform behavior beyond theruntime.GOOSswitch. -
runInteractiveTUIexits 0 on successful TUI return, but there's no signal that the TUI ran vs. immediately bailed. The testTestRunNoArgsLaunchesTUIWithResolvedProviderMetadatareturns 0 for a successful TUI and theTestRunCommandsDoNotLaunchTUItest returns whateverrunTUIreturned (default 0). If the TUI panics inside its Bubble Tea loop, the test harness would surface it as a Go panic but the CLI exit code would be 1 (Go's default panic exit). Adefer recover()wrapper arounddeps.runTUIwould convert TUI panics into a clean error report. Minor.
No blockers. The appDeps DI seam, the unified prompt-routing path, the resolver-rewrite with the right env/command ordering, the JSON/text split in execEventWriter, and the end-to-end httptest.Server test are all genuine upgrades to the M1 Go path. The nextFlagValue flag-shaped-value gap (#1) is the only one I'd want to see fixed in this PR — it's a one-liner and matches the pattern already established in #53's scripts/build.ts. The rest are polish items for follow-up PRs (--ask mode, registry-aware model validation, TUI context plumbing, error-panic recovery).
# Conflicts: # internal/cli/app.go
3 participants
Summary
zeroinstead of showing helpzero execto resolved config plus the real OpenAI-compatible provider pathopenai/openai-compatibleNotes
This keeps the scope focused on the real Go CLI flow. It intentionally defers stream-json, sessions, Anthropic/Gemini, MCP/plugins, and model registry UI to follow-up PRs.
The implementation was split with subagents across config/provider factory, TUI bootstrap, and headless exec, then integrated in this branch.
Validation
go test ./...bun run typecheckbun test ./tests --timeout 15000(279 pass)bun run buildbun run smoke:buildbun run build:gobun run smoke:goSummary by CodeRabbit
New Features
Documentation
Tests
Chores