feat: make Go runtime the app path - #71
Merged
Merged
Conversation
Vasanthdev2004
marked this pull request as ready for review
June 5, 2026 11:07
Contributor
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an internal Go update-check package and wires ChangesGo-native & Compatibility Transition
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 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: 8
🧹 Nitpick comments (3)
internal/update/update.go (2)
148-171: 💤 Low valueConsider documenting or removing
data:endpoint support.Lines 149-151 support
data:URLs, presumably for testing. This is production code but the feature isn't documented in the Options struct or function comments. Either document this capability or move it behind a test-only interface.🤖 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/update/update.go` around lines 148 - 171, The fetchRelease function currently accepts "data:" URLs via fetchDataRelease (see fetchRelease and fetchDataRelease), which is undocumented; either document this behavior on the public Options struct or function comment (document that Options can accept data: endpoints for testing and link to fetchDataRelease), or remove/limit it to tests by moving fetchDataRelease behind a test-only build tag or exposing it only in test helpers; update comments and public API accordingly so the presence of "data:" endpoint support is explicit and not surprising in production code.
87-132: 💤 Low valueConsider validating timeout is non-negative.
Lines 97-100 default a zero timeout to
DefaultTimeout, but negative timeouts are silently ignored (line 101 checks> 0). This could lead to unexpected behavior if a caller mistakenly passes a negative duration.💡 Suggested validation
timeout := options.Timeout + if timeout < 0 { + return Result{}, fmt.Errorf("timeout cannot be negative") + } if timeout == 0 { timeout = DefaultTimeout }🤖 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/update/update.go` around lines 87 - 132, The Check function currently treats zero timeout specially but silently ignores negative durations; validate Options.Timeout early in Check (before applying DefaultTimeout) and return a clear error if timeout is negative. Locate the timeout handling in Check (referencing Options.Timeout, DefaultTimeout and the context.WithTimeout block) and add a guard that checks if options.Timeout < 0 and returns a descriptive error (e.g., "timeout must be non-negative") so callers cannot pass negative durations that are silently ignored.internal/cli/update.go (1)
16-44: 💤 Low valueConsider using a more generic error writer.
Line 19 calls
writeExecUsageError, which is semantically tied to the exec command. While the code reuse works, it's misleading when debugging update command errors. Consider renamingwriteExecUsageErrortowriteUsageErroror introducing a dedicatedwriteUpdateUsageError.This is a minor naming nit and doesn't affect functionality.
🤖 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/update.go` around lines 16 - 44, The call to writeExecUsageError inside runUpdate is misleadingly named for the update command; replace it with a generic usage-error writer (either rename writeExecUsageError to writeUsageError or add a new writeUpdateUsageError wrapper that forwards to the existing implementation) and update runUpdate to call the new generic function instead of writeExecUsageError (affects the invocation in runUpdate and any callers if you choose to rename the original function). Ensure the new function signature and behavior match writeExecUsageError so existing error handling and return values remain 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.
Inline comments:
In `@internal/cli/app_test.go`:
- Around line 243-285: The current TestRunUpdateCheckTextAndJSON covers only the
happy path; add unit tests using runWithDeps and appDeps (injecting checkUpdate)
to cover: 1) missing --check flag (call runWithDeps([]string{"update"}, ...) and
assert non-success and stderr mentions "--check"), 2) checkUpdate error path
(inject checkUpdate returning error like "network failure", assert non-success
and stderr contains the error), 3) help output
(runWithDeps([]string{"update","--help"}, ...) and assert stdout contains
"--check"), 4) no-update case (inject update.Result with UpdateAvailable:false
and assert stdout shows "up to date"), and 5) JSON completeness (when using
--json assert all update.Result fields CurrentVersion, LatestVersion,
ReleaseURL, TagName and UpdateAvailable are present and correct); use the
existing TestRunUpdateCheckTextAndJSON, runWithDeps, appDeps, and checkUpdate
function shape to implement these additional tests.
In `@internal/cli/exec_parse.go`:
- Around line 69-86: Extend the test TestRunExecHelpDocumentsM1Flags to assert
that the legacy compatibility flags from writeExecHelp are documented: add
checks that the help output includes the "--profile <profile>" flag (for
modelProfile) and the "-r, --reasoning-effort <effort>" flag (for
reasoningEffort); update the test's want list in internal/cli/exec_test.go to
include those exact strings so the help documentation assertions cover legacy
flags handled in exec_parse.go (modelProfile and reasoningEffort).
In `@internal/update/update_test.go`:
- Around line 1-62: The test suite only covers happy paths; add unit tests
exercising failure branches of Check and Format: add tests that call Check with
a Fetch that returns an error to cover network failures and context cancellation
(use context.WithTimeout to trigger deadline and ensure Check respects ctx), a
Fetch returning invalid JSON/malformed Release or Release with empty
TagName/empty HTMLURL to hit the tag_name and HTMLURL fallback logic in Check, a
Fetch returning HTTP error status simulation to cover non-2xx handling, and
tests for Format when UpdateAvailable is false to assert the "up to date"
message; target the Check, Format, and ResolveEndpoint behaviors referenced in
update.go so each error branch (missing tag_name, invalid version strings, data:
endpoint, context timeout) has an explicit test.
In `@internal/update/update.go`:
- Line 199: Update the two fmt.Errorf calls that currently produce
capitalized/error-punctuated messages (the one using "Invalid update endpoint
%q. Use a full URL or an owner/repo slug like %s." and the similar message
around repository validation) to follow Go error conventions: make the first
word lowercase and remove terminal punctuation. For example, change to a message
like `invalid update endpoint %q: use a full URL or an owner/repo slug like %s`
(and apply the same lowercase/no-punctuation pattern to the other fmt.Errorf
usage referencing the repository/value variables).
- Around line 76-85: CompareSemver currently calls parseSemver which can panic
via NormalizeVersionTag; refactor parseSemver to return ([]int, error) instead
of panicking and update CompareSemver to return (int, error) so callers receive
an error for invalid input; specifically change parseSemver to validate and
propagate errors from NormalizeVersionTag, handle the error in CompareSemver
(return 0, err) and only perform the comparison when both parses succeed, and
update any call sites of CompareSemver to handle the new error return.
- Around line 49-66: ResolveEndpoint currently panics on invalid input; change
its signature to return (string, error) instead of string, remove the panic and
return a descriptive error (e.g. fmt.Errorf("invalid update endpoint %q: use
full URL or owner/repo like %s", value, repository)) when url.ParseRequestURI
fails or scheme is empty, and keep the existing behavior of returning
Endpoint(repository) or Endpoint(value) on success; alternatively, if external
callers shouldn't use it, make ResolveEndpoint private and have public callers
use the existing private resolveEndpoint that already returns (string, error).
Ensure all callers of ResolveEndpoint are updated to handle the returned error.
- Line 162: The deferred call to response.Body.Close() is currently unchecked;
wrap it in a deferred closure that captures and checks the error from
response.Body.Close() and logs or prints a warning using the existing logger (or
fmt) so failures closing the response body in update.go are not silently
ignored; locate the defer response.Body.Close() in the function that performs
the HTTP request (the variable response) and replace it with a defer func() { if
err := response.Body.Close(); err != nil { /* log or warn via existing logger */
} } to handle the error.
- Around line 68-74: NormalizeVersionTag currently panics on invalid input;
change it to return an error instead (or delegate to the existing
normalizeVersionTag) so the public API does not crash the CLI: replace the panic
behavior in NormalizeVersionTag with an error-returning flow (e.g., return
(string, error)) or simply call the private normalizeVersionTag and propagate
its error, ensuring callers handle the error rather than allowing a panic from
NormalizeVersionTag.
---
Nitpick comments:
In `@internal/cli/update.go`:
- Around line 16-44: The call to writeExecUsageError inside runUpdate is
misleadingly named for the update command; replace it with a generic usage-error
writer (either rename writeExecUsageError to writeUsageError or add a new
writeUpdateUsageError wrapper that forwards to the existing implementation) and
update runUpdate to call the new generic function instead of writeExecUsageError
(affects the invocation in runUpdate and any callers if you choose to rename the
original function). Ensure the new function signature and behavior match
writeExecUsageError so existing error handling and return values remain
unchanged.
In `@internal/update/update.go`:
- Around line 148-171: The fetchRelease function currently accepts "data:" URLs
via fetchDataRelease (see fetchRelease and fetchDataRelease), which is
undocumented; either document this behavior on the public Options struct or
function comment (document that Options can accept data: endpoints for testing
and link to fetchDataRelease), or remove/limit it to tests by moving
fetchDataRelease behind a test-only build tag or exposing it only in test
helpers; update comments and public API accordingly so the presence of "data:"
endpoint support is explicit and not surprising in production code.
- Around line 87-132: The Check function currently treats zero timeout specially
but silently ignores negative durations; validate Options.Timeout early in Check
(before applying DefaultTimeout) and return a clear error if timeout is
negative. Locate the timeout handling in Check (referencing Options.Timeout,
DefaultTimeout and the context.WithTimeout block) and add a guard that checks if
options.Timeout < 0 and returns a descriptive error (e.g., "timeout must be
non-negative") so callers cannot pass negative durations that are silently
ignored.
🪄 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: 24564877-5436-4f26-b331-891c93ef22ec
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (76)
README.mdbin/zero.tsdocs/NPM_WRAPPER_SMOKE.mddocs/PERFORMANCE.mdinternal/cli/app.gointernal/cli/app_test.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_test.gointernal/cli/extensions.gointernal/cli/extensions_test.gointernal/cli/mcp_tools.gointernal/cli/observability.gointernal/cli/observability_test.gointernal/cli/update.gointernal/update/update.gointernal/update/update_test.gopackage.jsonscripts/npm-wrapper.tsscripts/package-release.tsscripts/perf-bench.tssrc/cli/index.tssrc/index.tssrc/tui/AddProvider.tsxsrc/tui/App.tsxsrc/tui/CommandSuggestions.tsxsrc/tui/DebugErrorPanel.tsxsrc/tui/DiffCard.tsxsrc/tui/LiveDot.tsxsrc/tui/Logo.tsxsrc/tui/MessageRenderer.tsxsrc/tui/ModelPicker.tsxsrc/tui/ProviderPicker.tsxsrc/tui/Spinner.tsxsrc/tui/StartupScreen.tsxsrc/tui/ThemePicker.tsxsrc/tui/ToolApprovalPanel.tsxsrc/tui/ToolCallRenderer.tsxsrc/tui/Transcript.tsxsrc/tui/TuiHeader.tsxsrc/tui/TuiPromptBox.tsxsrc/tui/TuiShell.tsxsrc/tui/TuiStatusBar.tsxsrc/tui/commands.tssrc/tui/highlighter.tssrc/tui/index.tsxsrc/tui/model-selection.tssrc/tui/startup/CommandChips.tsxsrc/tui/startup/Header.tsxsrc/tui/startup/PromptBox.tsxsrc/tui/startup/ZeroLogo.tsxsrc/tui/startup/theme.tssrc/tui/terminal-background.tssrc/tui/theme.tssrc/tui/types.tssrc/update/check.tssrc/zero-plugins/index.tssrc/zero-plugins/loader.tssrc/zero-plugins/manifest.tssrc/zero-plugins/types.tstests/build-scripts.test.tstests/cli-version.test.tstests/headless-exec.test.tstests/perf-bench.test.tstests/tui-command-registry.test.tstests/tui-model-selection.test.tstests/tui-shell-render.test.tstests/tui-theme.test.tstests/update-check.test.tstests/zero-config-cli.test.tstests/zero-doctor-cli.test.tstests/zero-hooks.test.tstests/zero-mcp-client.test.tstests/zero-mcp-permissions.test.tstests/zero-plugins.test.tstests/zero-search-cli.test.ts
💤 Files with no reviewable changes (53)
- src/tui/Transcript.tsx
- tests/tui-theme.test.ts
- src/tui/CommandSuggestions.tsx
- src/tui/ProviderPicker.tsx
- src/tui/Logo.tsx
- src/tui/types.ts
- src/tui/startup/CommandChips.tsx
- src/tui/index.tsx
- tests/tui-command-registry.test.ts
- tests/zero-plugins.test.ts
- tests/tui-model-selection.test.ts
- tests/update-check.test.ts
- tests/zero-mcp-permissions.test.ts
- src/zero-plugins/types.ts
- src/tui/terminal-background.ts
- tests/zero-doctor-cli.test.ts
- src/tui/TuiShell.tsx
- src/tui/App.tsx
- src/tui/DiffCard.tsx
- src/tui/startup/theme.ts
- tests/tui-shell-render.test.ts
- tests/cli-version.test.ts
- src/zero-plugins/manifest.ts
- src/update/check.ts
- src/tui/TuiPromptBox.tsx
- src/zero-plugins/loader.ts
- src/tui/startup/Header.tsx
- src/tui/ToolCallRenderer.tsx
- src/tui/theme.ts
- tests/zero-config-cli.test.ts
- src/tui/model-selection.ts
- src/tui/ToolApprovalPanel.tsx
- src/tui/Spinner.tsx
- src/tui/startup/PromptBox.tsx
- tests/zero-hooks.test.ts
- src/tui/TuiHeader.tsx
- src/zero-plugins/index.ts
- src/tui/LiveDot.tsx
- tests/zero-mcp-client.test.ts
- src/cli/index.ts
- tests/zero-search-cli.test.ts
- tests/headless-exec.test.ts
- src/tui/commands.ts
- src/tui/MessageRenderer.tsx
- src/index.ts
- src/tui/startup/ZeroLogo.tsx
- src/tui/highlighter.ts
- src/tui/DebugErrorPanel.tsx
- src/tui/ModelPicker.tsx
- src/tui/AddProvider.tsx
- src/tui/TuiStatusBar.tsx
- src/tui/ThemePicker.tsx
- src/tui/StartupScreen.tsx
gnanam1990
requested changes
Jun 5, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Blockers
- This PR is currently not mergeable: GitHub reports
mergeStateStatus=DIRTYagainstmain, so it needs a rebase/conflict fix before it can be safely queued. - The Zero Review check failed on head
7a355783b276, and the bot comment reportsgit diff --checkfailure. Please fix the diff hygiene issue and rerun the check. - The PR review decision is still
CHANGES_REQUESTED; CodeRabbit posted unresolved actionable comments around the update API/CLI test coverage and public API error handling. Please verify the still-valid items and either fix them or document why they no longer apply before rereview.
Non-blocking context
- The platform smoke jobs and performance smoke are passing, and CodeRabbit status currently shows success/skipped, but the failed Zero Review plus dirty merge state still block this from merge order.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/update/update.go`:
- Around line 204-210: normalizeVersionTag currently uses atoi(match[n]) which
panics on oversized numeric components; change it to safely parse each capture
with strconv.ParseUint(match[n], 10, 32) (or ParseInt with 64 bits and then
check bounds) and return an error if parsing fails or the value overflows, then
use the parsed uint32/int values when building the formatted string; update
references in normalizeVersionTag to remove atoi calls and handle parse errors
instead of allowing a panic.
🪄 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: cc98afa3-9ca6-4081-a66b-3724f359abde
📒 Files selected for processing (9)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_test.gointernal/cli/update.gointernal/update/update.gointernal/update/update_test.gotests/zero-hooks.test.ts
💤 Files with no reviewable changes (1)
- tests/zero-hooks.test.ts
✅ Files skipped from review due to trivial changes (1)
- internal/cli/exec.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cli/update.go
- internal/cli/exec_parse.go
- internal/cli/app.go
- internal/cli/exec_test.go
Collaborator
Author
|
@gnanam1990 blockers from the earlier review are addressed on latest head bc10b6d:\n\n- Merged latest main into feat/go-runtime-takeover, so the PR is no longer DIRTY.\n- Zero Review is now passing.\n- CodeRabbit's update API/test coverage comments were fixed, including the follow-up semver overflow case, and CodeRabbit has approved.\n- Current GitHub checks are green: ubuntu/macOS/windows smoke, Performance Smoke, Zero Review, CodeRabbit.\n\nPlease re-review when you get a chance so the stale CHANGES_REQUESTED state can clear. |
gnanam1990
approved these changes
Jun 5, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Approved latest head bc10b6d98907.
I rechecked the prior blockers against this commit:
- The dirty merge/diff-hygiene blocker is gone;
git diff --checkpasses. - The update semver overflow issue is fixed with checked parsing plus oversized-version regressions.
- The update API now returns errors instead of panicking on invalid input, handles response body close errors, documents
data:endpoint behavior, and validates negative timeouts. - The Go runtime takeover wiring, npm wrapper move, legacy flags, and
mcp listcompatibility path all look consistent with the current branch direction.
Validation run locally from a clean worktree:
git diff --check origin/main...HEAD✅go test -count=1 ./internal/update ./internal/cli✅go test -count=1 -p 1 ./...✅bun install --frozen-lockfile✅bun run typecheck✅bun test ./tests --timeout 15000✅bun run build✅bun run smoke:build✅bun run smoke:go✅- built
./zero --helpand./zero update --helpsmoke checks ✅
All GitHub checks are also green at review time.
This was referenced Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
bun run devnow runsgo run ./cmd/zero, and the npm wrapper only launches the built native binary.src/toscripts/, updates release packaging, and removes the TypeScript fallback tosrc/index.ts.zero update --check [--json]to Go and adds compatibility coverage for legacy exec/search/MCP CLI shapes.package.jsonandbun.lock.Validation
bun run typecheckgo test ./...bun test ./tests --timeout 15000bun run buildbun run smoke:buildbun run smoke:goSummary by CodeRabbit
New Features
Documentation
Chores
Removed