feat: advance Go-first TUI and npm wrapper - #57
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. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughAdds an npm wrapper and Bun-executable CLI entrypoint; updates package/release wiring and tests; adds exec --max-turns parsing and propagation; refactors TUI slash-command parsing to a centralized registry with help/footer, handler helpers, model/plan helpers, and expanded tests. Changesnpm wrapper entrypoint and target resolution
CLI exec --max-turns
TUI command system refactor
Sequence Diagram(s) sequenceDiagram
participant User
participant TUI
participant Parser
participant CommandRegistry
User->>TUI: submit input
TUI->>Parser: parseCommand(input)
Parser->>CommandRegistry: lookup name/aliases
CommandRegistry-->>Parser: definition or not found
Parser-->>TUI: parsedCommand (prompt / unknown / resolved)
TUI->>TUI: handleSubmit -> append transcript or run agent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
🧹 Nitpick comments (2)
tests/build-scripts.test.ts (1)
113-149: ⚡ Quick winAdd a no-target test to cover the final safety branch.
You verify native-first and TS-fallback, but not the “nothing available” outcome (
null). Adding that case locks down the wrapper’s final decision path.Suggested test case
describe('npm wrapper entrypoint', () => { @@ it('prefers the Go binary and keeps the TS CLI as a local fallback', () => { @@ }); + + it('returns null when neither native nor TS fallback target exists', () => { + const target = resolveNpmWrapperTarget({ + root: join('repo'), + platform: 'linux', + bunPath: 'bun', + args: ['--version'], + exists: () => false, + }); + + expect(target).toBeNull(); + }); });As per coding guidelines, tests under
tests/**/*.tsshould provide meaningful coverage of safety decisions and runtime flows.🤖 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 `@tests/build-scripts.test.ts` around lines 113 - 149, Add a unit test that exercises the final safety branch of resolveNpmWrapperTarget by simulating no available targets and asserting it returns null; specifically, after the existing native-first and typescript-fallback checks, call resolveNpmWrapperTarget with platform 'win32', bunPath 'bun', args ['--version'] and an exists function that always returns false (or a Set that contains neither the native binary nor the TS entry), then expect the result to be null to lock down the "nothing available" outcome.internal/tui/model.go (1)
387-389: ⚡ Quick winDerive footer commands from the registry to prevent drift.
At Line 388, the footer command list is hardcoded while command metadata now lives in
commandDefinitions. This will eventually diverge during renames/additions; generate footer commands from registry data (optionally with a small allowlist for brevity).Suggested refactor
func commandFooterText() string { - return "/help /model /provider /context /tools /permissions /clear /exit Esc clear Ctrl+C quit" + featured := map[commandKind]bool{ + commandHelp: true, + commandModel: true, + commandProvider: true, + commandContext: true, + commandTools: true, + commandPermissions: true, + commandClear: true, + commandExit: true, + } + parts := make([]string, 0, len(commandDefinitions)+2) + for _, command := range commandDefinitions { + if featured[command.kind] { + parts = append(parts, command.name) + } + } + parts = append(parts, "Esc clear", "Ctrl+C quit") + return strings.Join(parts, " ") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model.go` around lines 387 - 389, Replace the hardcoded footer string in commandFooterText with a generated list built from the command registry (commandDefinitions): iterate the registry keys (or a small allowlist of the most common commands) to produce the short names, map them into the display tokens (e.g. "/name" and special tokens like "Esc clear" / "Ctrl+C quit" kept literal), join with double spaces to match current spacing, and return that string; ensure commandFooterText falls back to the original hardcoded string if commandDefinitions is empty or missing.
🤖 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 `@src/npm-wrapper.ts`:
- Around line 60-63: Update the error message emitted when target == null to
reflect that neither the native binary nor the TypeScript fallback is available:
change the stderr.write call that currently says "[zero] No native zero binary
found..." to a clearer message that mentions both the native binary and the TS
fallback are unavailable and suggests running `bun run build` (or another
recovery step); modify the string passed to stderr.write in the block
referencing target, options.stderr, and stderr.write so callers see the accurate
failure cause.
- Around line 66-73: The runNpmWrapper function currently calls
Bun.spawn(target.command, ...) unguarded and can throw synchronously; wrap the
Bun.spawn call in a try/catch so synchronous spawn failures are caught and the
function returns a clean numeric exit code (e.g., Promise.resolve(1) or
equivalent) instead of crashing; also update the error/message emitted when
target is null in resolveNpmWrapperTarget/runNpmWrapper to correctly state that
neither the native binary nor src/index.ts was found (instead of the misleading
"No native zero binary found…"), and ensure you still await/return child.exited
when spawn succeeds.
---
Nitpick comments:
In `@internal/tui/model.go`:
- Around line 387-389: Replace the hardcoded footer string in commandFooterText
with a generated list built from the command registry (commandDefinitions):
iterate the registry keys (or a small allowlist of the most common commands) to
produce the short names, map them into the display tokens (e.g. "/name" and
special tokens like "Esc clear" / "Ctrl+C quit" kept literal), join with double
spaces to match current spacing, and return that string; ensure
commandFooterText falls back to the original hardcoded string if
commandDefinitions is empty or missing.
In `@tests/build-scripts.test.ts`:
- Around line 113-149: Add a unit test that exercises the final safety branch of
resolveNpmWrapperTarget by simulating no available targets and asserting it
returns null; specifically, after the existing native-first and
typescript-fallback checks, call resolveNpmWrapperTarget with platform 'win32',
bunPath 'bun', args ['--version'] and an exists function that always returns
false (or a Set that contains neither the native binary nor the TS entry), then
expect the result to be null to lock down the "nothing available" outcome.
🪄 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: 79d9063e-af1e-4907-8e3e-2497290fb244
📒 Files selected for processing (9)
bin/zero.tsdocs/NPM_WRAPPER_SMOKE.mdinternal/tui/commands.gointernal/tui/model.gointernal/tui/model_test.gopackage.jsonscripts/package-release.tssrc/npm-wrapper.tstests/build-scripts.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/cli/exec_test.go (1)
55-80: ⚡ Quick winAdd explicit regression coverage for
0and empty--max-turnsinput.This path is easy to regress. Add cases for
0and--max-turns=so CLI behavior is explicit and enforced.Suggested test additions
func TestRunExecRejectsInvalidMaxTurnsBeforeRuntime(t *testing.T) { for _, tc := range []struct { value string want string }{ {value: "nope", want: "invalid --max-turns"}, {value: "-1", want: "invalid --max-turns"}, + {value: "0", want: "invalid --max-turns"}, } { t.Run(tc.value, func(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer @@ }) } + + t.Run("equals-empty", func(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + + exitCode := Run([]string{"exec", "--max-turns=", "hello"}, &stdout, &stderr) + if exitCode != exitUsage { + t.Fatalf("expected exit code %d, got %d", exitUsage, exitCode) + } + if got := stderr.String(); !strings.Contains(got, "--max-turns requires a value") { + t.Fatalf("expected empty-value max-turns error, got %q", got) + } + }) }🤖 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 55 - 80, Add regression cases for "0" and the empty `--max-turns=` input to TestRunExecRejectsInvalidMaxTurnsBeforeRuntime: extend the test table with entries for value "0" and value "" (expecting the same "invalid --max-turns" message), and adjust how the test builds args so an empty value is passed as the single token "--max-turns=" instead of separate flag+value; keep using Run(...) and asserting exitUsage, stdout empty, and stderr contains the expected message.internal/tui/model_catalog.go (1)
23-30: ⚡ Quick winRender
/model listentries in a stable order.
modelListText()currently renders whatever orderRegistry.List(...)returns, which can make catalog output jump between runs. Sorting before rendering keeps CLI UX predictable and avoids brittle downstream assertions.Proposed patch
import ( "fmt" + "sort" "strings" "github.com/Gitlawb/zero/internal/modelregistry" ) @@ - for _, model := range registry.List(modelregistry.ListOptions{}) { + models := registry.List(modelregistry.ListOptions{}) + sort.Slice(models, func(i, j int) bool { + if models[i].Provider == models[j].Provider { + return models[i].ID < models[j].ID + } + return models[i].Provider < models[j].Provider + }) + for _, model := range models { marker := " " if activeID != "" && model.ID == activeID { marker = "*" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model_catalog.go` around lines 23 - 30, modelListText currently iterates registry.List(modelregistry.ListOptions{}) and appends in whatever order is returned; make the output stable by sorting the returned slice before rendering (e.g., sort.SliceStable on the slice returned by registry.List using a deterministic key like model.ID or model.DisplayName). Locate the slice produced by registry.List in modelListText, apply the sort using sort.SliceStable(models, func(i,j int) bool { return models[i].ID < models[j].ID }) (or DisplayName) and then proceed to build lines (preserving the activeID check and marker logic).internal/tui/model_test.go (1)
279-283: ⚡ Quick winAvoid hard-coding evolving catalog entries in this TUI behavior test.
Asserting
"claude-sonnet-4.5"and"gemini-2.5-pro"couples this test to registry contents rather than/model listbehavior. This will fail on catalog updates even when command handling is correct.Proposed patch
- for _, want := range []string{"Active model: gpt-4.1", "provider: openai", "Available models", "* gpt-4.1", "claude-sonnet-4.5", "gemini-2.5-pro"} { + for _, want := range []string{"Active model: gpt-4.1", "provider: openai", "Available models", "* gpt-4.1"} { if !transcriptContains(next.transcript, want) { t.Fatalf("expected model transcript to contain %q, got %#v", want, next.transcript) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model_test.go` around lines 279 - 283, The test currently hard-codes specific catalog model names which will break when the registry changes; update the assertions in the test around transcriptContains/next.transcript so they verify behavior not exact entries: assert the presence of "Available models" and the provider and active-model lines (e.g., "Active model:" and "provider:"), and replace exact name checks like "claude-sonnet-4.5" and "gemini-2.5-pro" with a more general assertion that at least one model line exists (e.g., any transcript line matching the model-list pattern such as lines starting with "* " or non-empty model entries), using transcriptContains and scanning next.transcript to confirm the list format rather than specific model names.
🤖 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/exec.go`:
- Around line 75-77: The CLI currently accepts --max-turns 0 but then silently
ignores it because exec.go only forwards options.maxTurns when > 0
(overrides.MaxTurns = options.maxTurns), so either validate and reject zero at
parse time or propagate an explicit zero; to match current behavior prefer
rejecting zero: add validation where options.maxTurns is parsed/validated (the
code that sets options.maxTurns) to return an error if value == 0, and/or in the
command run flow check options.maxTurns == 0 and return a user-facing error;
reference options.maxTurns and overrides.MaxTurns (and the flag parsing function
that populates options.maxTurns) when adding this validation so the explicit
zero is not silently ignored.
---
Nitpick comments:
In `@internal/cli/exec_test.go`:
- Around line 55-80: Add regression cases for "0" and the empty `--max-turns=`
input to TestRunExecRejectsInvalidMaxTurnsBeforeRuntime: extend the test table
with entries for value "0" and value "" (expecting the same "invalid
--max-turns" message), and adjust how the test builds args so an empty value is
passed as the single token "--max-turns=" instead of separate flag+value; keep
using Run(...) and asserting exitUsage, stdout empty, and stderr contains the
expected message.
In `@internal/tui/model_catalog.go`:
- Around line 23-30: modelListText currently iterates
registry.List(modelregistry.ListOptions{}) and appends in whatever order is
returned; make the output stable by sorting the returned slice before rendering
(e.g., sort.SliceStable on the slice returned by registry.List using a
deterministic key like model.ID or model.DisplayName). Locate the slice produced
by registry.List in modelListText, apply the sort using sort.SliceStable(models,
func(i,j int) bool { return models[i].ID < models[j].ID }) (or DisplayName) and
then proceed to build lines (preserving the activeID check and marker logic).
In `@internal/tui/model_test.go`:
- Around line 279-283: The test currently hard-codes specific catalog model
names which will break when the registry changes; update the assertions in the
test around transcriptContains/next.transcript so they verify behavior not exact
entries: assert the presence of "Available models" and the provider and
active-model lines (e.g., "Active model:" and "provider:"), and replace exact
name checks like "claude-sonnet-4.5" and "gemini-2.5-pro" with a more general
assertion that at least one model line exists (e.g., any transcript line
matching the model-list pattern such as lines starting with "* " or non-empty
model entries), using transcriptContains and scanning next.transcript to confirm
the list format rather than specific model names.
🪄 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: 95687138-0d7b-4911-a2e6-7ce22a83a15d
📒 Files selected for processing (7)
internal/cli/app.gointernal/cli/exec.gointernal/cli/exec_test.gointernal/tui/model.gointernal/tui/model_catalog.gointernal/tui/model_test.gointernal/tui/plan_command.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/model.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
- The npm wrapper is the right native-first shape.
bin/zero.tscomputes the package root fromimport.meta.urland delegates torunNpmWrapper, whilesrc/npm-wrapper.tsprefers the built nativezero/zero.exeand falls back tosrc/index.tsfor local dev. The latest commits also fixed the no-target message and synchronousBun.spawnpanic path;tests/build-scripts.test.tsnow covers native-first, TS fallback, no-target, and spawn-throws behavior. - Release packaging now includes the wrapper files.
scripts/package-release.tsstagesbin/zero.tsandsrc/npm-wrapper.tsalongside the native binary, README, package.json, and VERSION, so the npmbinentry no longer points at a file missing from release archives. - TUI command parsing moved from ad hoc switches to a registry.
commandDefinitionsgives each slash command a name, aliases, usage, group, description, and kind;parseCommand,resolveCommand,listCommandNames, andformatCommandHelpLinesall flow through that single source./quitand/debug-modealiases are covered in tests. - Footer/help drift was addressed.
commandFooterText()is now generated fromcommandDefinitionswith a featured-kind allowlist and a fallback constant for empty input. This directly closes the earlier CodeRabbit footer comment while keeping the footer intentionally shorter than the full registry. - The Go TUI now has useful shell-only status commands.
/provider,/model,/model list,/context,/config,/debug, and/planall append system rows without starting an agent run. Tests assert each command stays local (cmd == nil) and that context/provider/model/plan state is rendered. /plancorrectly reuses the existing update-plan tool state.planTextlooks upupdate_plan, type-asserts a smallCurrentPlan() []tools.PlanIteminterface, and renders item status/content/notes. That keeps the TUI coupled to the existing tool contract instead of creating a second plan store.--max-turnsreaches config overrides.parseExecArgsparses--max-turns/--max-turns=<n>,runExeccopies positive values intoconfig.Overrides.MaxTurns, andTestRunExecMaxTurnsReachesConfigOverridesproves the value reachesresolveConfigbefore provider construction.
Observations (non-blocking)
-
CodeRabbit's
--max-turns 0finding is real.parseExecMaxTurnsaccepts zero because it only rejects< 0, butrunExeconly forwardsoptions.maxTurnswhen it is> 0. Sozero exec --max-turns 0 hellosilently behaves exactly like no override. Either reject zero asinvalid --max-turnsor intentionally propagate zero; rejecting is the safer CLI behavior. -
The flag-shaped-value parser gap from #54 still exists.
nextFlagValuestill accepts the next token as a value even when it starts with-, sozero exec --max-turns --cwd /tmp helloreportsinvalid --max-turns "--cwd"instead of--max-turns requires a value, andzero exec --prompt --model gpt-4can still misparse prompt text. Not introduced here, but this PR adds another flag that goes through the same helper, so the surface area grows. -
/model listalways loadsmodelregistry.DefaultRegistry(). That's fine for the current catalog shell, but it means a future custom registry passed throughtui.Optionscannot affect/model list. If provider factory starts accepting injected registries in tests or plugins, the TUI will need a registry field for models too. -
The model-list order is stable already. CodeRabbit suggested sorting
modelListText, butRegistry.Listfrom #56 iterates theregistry.modelsslice, not a map, so it preservesDefaultModelEntries()order. Sorting by ID/provider would change the curated display order rather than fixing nondeterminism. A short comment inmodelListTextwould make that clear. -
The
/model listtest intentionally couples to the catalog.TestModelCommandShowsActiveModelWithoutRunningAgentassertsclaude-sonnet-4.5andgemini-2.5-pro. That is brittle if the catalog changes, but it also proves the TUI lists cross-provider catalog entries. If catalog churn becomes frequent, keep one behavior test generic and move the exact model assertions to modelregistry tests only. -
No duplicate command-name/alias test.
resolveCommandlinearly scans names and aliases; if a future command reuses/quitor/debug-mode, the first one silently wins. A small test that builds a map fromcommandDefinitionsand fails on duplicate names/aliases would lock the registry invariant. -
shellOnlyCommandTextis clear but intentionally temporary./doctor,/search,/theme, and/input-styleare registered but not wired. The placeholder text is honest; just make sure these commands get follow-up issues so they don't become permanent dead menu items.
No blockers. The wrapper path, TUI command registry, generated footer/help, plan display, and --max-turns override are all good Go-first CLI/TUI progress. The only bug worth fixing before merge is --max-turns 0 being accepted and ignored; everything else is polish or follow-up scope.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Reviewed current head 14dc4e7.
No blockers found. The latest commit fixes the --max-turns 0 regression path and adds explicit coverage for 0 and --max-turns=. The npm wrapper path is native-first with a TS fallback, release packaging includes the wrapper files, and the Go TUI command registry/status commands are covered.
Local validation passed: bun install --frozen-lockfile, go test ./..., bun run typecheck, bun test ./tests --timeout 15000 (290 pass), bun run build, bun run smoke:build, bun run build:go, bun run smoke:go, and git diff --check origin/main...HEAD.
3 participants
Summary
Tests
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
Chores