Remove Bun installer test dependency#91
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. |
Review: PR #91 — Remove Bun installer test dependency (anandh8x / Anandan)URL: #91 Ownership FitStrong match for Anandan's platform product owner lane (WORK_SPLIT_PRD v3):
This PR ports the installer script contract tests and no-network Unix install smoke from Bun/TS to Go, removes Bun/TS config and CI setup for installer tests, switches CI/release/perf smoke/docs/PR review to direct Go commands (go run ./cmd/zero-release *, go test, etc.), while keeping the npm wrapper as Node (with message pointing to Go build). Clean boundary: Core of this PR does not touch runtime contracts (agent, permissions, sessions, providers, stream-json, etc. that Gnanam owns). The runtime file diffs in the branch are from previously merged PRs. Touches release/packaging/CI/install smoke — exactly Anandan's remit. Fits "every PR must expose user-runnable behavior or concrete contract" (install smoke now in Go, CI proves commands post-install). Summary of Change
Validation (per PR): go test for installtest/npmwrapper/release/review/perfbench/cmd/zero-pr-review; go run zero-release build/smoke/package/verify; go run zero-perf-bench; node bin/zero.js --version; full go test ./...; git diff --check. Code Quality & TestsPositive:
Observations / minor suggestions (not blocking):
All Go tests for the touched packages (installtest, release, review, etc.) pass locally, build/smoke commands succeed, full Alignment with PRD / WORK_SPLIT / DoD
RecommendationApprove. This is exactly the platform work Anandan is DRI for: cleaning legacy Bun/TS installer test dep, porting smoke/contracts to Go, switching CI/release/perf to direct Go commands, updating docs/AGENTS. Delivers concrete, testable install/release behavior without network, fits "must produce platform behavior" rule. Clean, no runtime contract impact, tests pass, aligns with prior Anandan PRs (#86, #85, #84, #81, #88, #87). Minor future: continue simplifying remaining Bun refs if/when full TS removal for non-wrapper is ready; ensure cross-platform (Windows PS1 smoke) coverage stays strong. Great cleanup step toward mature Go-first platform. (Full prior detailed reviews for Anandan followed similar positive pattern with lane fit, boundary notes, and approve recs.) Review generated per user's request for PR 91. Local copy: reviews/pr-91-anandan-remove-bun-installer-tests.md |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Formal approval.
This is clean, high-value platform work for Anandan's lane (port installer smoke/contracts to Go, remove Bun dep from CI/release/perf paths, switch to direct Go commands, update docs/AGENTS).
- New internal/installtest tests the Unix/PS1 installer contracts + no-network smoke using fixtures (matches release naming, checksums, extract).
- Workflows simplified (no Bun setup for these; use go run ./cmd/zero-release * and go test).
- Fits DoD: tests added, go test, package smoke/release verification, produces runnable install behavior and CI proofs.
- No runtime contract impact (Gnanam's areas untouched in core change).
- CodeRabbit will likely approve; local validation (tests, build, smoke, perf) passes.
LGTM per detailed comment. Approving.
See #91 (comment)... (the posted detailed review) for full ownership fit, code notes, and minor suggestions.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Formal approval following detailed review.
This PR is a clean, high-value platform cleanup by Anandan:
- Ports installer contracts + no-network smoke tests to Go (new internal/installtest).
- Removes Bun/TS dep from CI, release, perf smoke, docs for these paths.
- Switches to direct Go commands (go run ./cmd/zero-release *, go test).
- Updates AGENTS.MD, smoke checklists, etc. appropriately.
- Keeps Node for npm wrapper only.
Fits Anandan's lane perfectly (install/release trust, smoke tests, CI verification, platform/release contract).
Delivers user-runnable behavior and CI proofs.
No impact on runtime contracts (Gnanam areas).
Tests (new + existing) pass locally; smoke/build/perf/package/verify steps succeed per PR validation and local runs.
LGTM. Approving.
Detailed analysis in prior comment: #91 (comment)
|
Linter diff in the way? Review this PR in Change Stack to focus on meaningful changes and expand context only when needed. 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 (1)
WalkthroughThis PR replaces Bun/TypeScript operational commands with Go-native commands across CI workflows and PR review checks, updates docs and npm wrapper messaging, trims the npm manifest, and adds Go tests validating Unix/PowerShell installer scripts including an offline Unix smoke test. ChangesBun-to-Go Runtime Migration
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: 1
🧹 Nitpick comments (1)
internal/installtest/install_scripts_test.go (1)
95-101: ⚡ Quick winAdd a checksum-mismatch smoke test to validate integrity enforcement.
Right now the suite checks checksum-related strings and a happy-path install, but it doesn’t assert that a bad checksum actually fails installation. Adding one negative-path test would harden the release-contract guarantee.
Also applies to: 161-163
🤖 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/installtest/install_scripts_test.go` around lines 95 - 101, Add a negative-path smoke test that ensures a bad checksum causes install to fail: after reading the archive bytes (archiveBytes := os.ReadFile(archivePath)) and writing the real checksum (sha256.Sum256), create a tampered checksum file (checksumPath) or overwrite it via writeFile with an incorrect hex value for archiveName, then run the installer invocation used elsewhere in this test suite and assert it returns a non-zero exit/error and emits a checksum-mismatch error string; mirror the pattern used for the happy-path test but expect failure and specific checksum error output to validate integrity enforcement.
🤖 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/installtest/install_scripts_test.go`:
- Around line 59-66: The test currently defaults releasePlatform to "linux" and
releaseArch to "x64" which masks unsupported OS/arch; change the logic around
the releasePlatform and releaseArch variables (the code using runtime.GOOS and
runtime.GOARCH) to explicitly handle known values and call t.Skipf(...) when
encountering any other OS or architecture instead of assigning a default. Locate
the block that sets releasePlatform and releaseArch in install_scripts_test.go,
add explicit switches or if/else branches for supported cases (e.g., "darwin" ->
"macos", "linux" -> "linux"; "amd64"/"x86_64" -> "x64", "arm64" -> "arm64") and
invoke t.Skipf with a clear message for unsupported runtime.GOOS or
runtime.GOARCH.
---
Nitpick comments:
In `@internal/installtest/install_scripts_test.go`:
- Around line 95-101: Add a negative-path smoke test that ensures a bad checksum
causes install to fail: after reading the archive bytes (archiveBytes :=
os.ReadFile(archivePath)) and writing the real checksum (sha256.Sum256), create
a tampered checksum file (checksumPath) or overwrite it via writeFile with an
incorrect hex value for archiveName, then run the installer invocation used
elsewhere in this test suite and assert it returns a non-zero exit/error and
emits a checksum-mismatch error string; mirror the pattern used for the
happy-path test but expect failure and specific checksum error output to
validate integrity enforcement.
🪄 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: 68aa54b3-ebc7-4650-9755-396fbce7b5fb
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/workflows/ci.yml.github/workflows/pr-auto-review.yml.github/workflows/release-artifacts.ymlAGENTS.MDREADME.mdbin/zero.jscmd/zero-pr-review/main.gocmd/zero-pr-review/main_test.godocs/INSTALL.mddocs/M1_HEADLESS_EXEC_PRD.mddocs/NPM_WRAPPER_SMOKE.mddocs/PERFORMANCE.mddocs/WORK_SPLIT_PRD.mdinternal/installtest/install_scripts_test.gointernal/npmwrapper/npmwrapper_test.gointernal/perfbench/perfbench.gointernal/review/review.gointernal/review/review_test.gopackage.jsontests/install-scripts.test.tstsconfig.json
💤 Files with no reviewable changes (5)
- cmd/zero-pr-review/main.go
- tests/install-scripts.test.ts
- tsconfig.json
- cmd/zero-pr-review/main_test.go
- package.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/installtest/install_scripts_test.go (1)
215-239:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPast review addressed; minor cleanup needed.
The switch-based platform/arch helpers with explicit
t.Skipffor unsupported values fully address the past review comment—great improvement.However, Line 231 includes
"x86_64"in the case, butruntime.GOARCHreturns"amd64"for x86-64 systems, never"x86_64". This case is unreachable and should be removed.🧹 Proposed fix
func unixReleaseArch(t *testing.T) string { t.Helper() switch runtime.GOARCH { - case "amd64", "x86_64": + case "amd64": return "x64" case "arm64": return "arm64" default: t.Skipf("Unix installer smoke does not support GOARCH=%s", runtime.GOARCH) return "" } }🤖 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/installtest/install_scripts_test.go` around lines 215 - 239, Remove the unreachable "x86_64" case from the unixReleaseArch helper: update the switch in unixReleaseArch (which currently checks runtime.GOARCH) to only match "amd64" and "arm64", leaving the default t.Skipf path unchanged; leave unixReleasePlatform alone. This cleans up dead code by deleting the "x86_64" case in the unixReleaseArch function.
🤖 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.
Duplicate comments:
In `@internal/installtest/install_scripts_test.go`:
- Around line 215-239: Remove the unreachable "x86_64" case from the
unixReleaseArch helper: update the switch in unixReleaseArch (which currently
checks runtime.GOARCH) to only match "amd64" and "arm64", leaving the default
t.Skipf path unchanged; leave unixReleasePlatform alone. This cleans up dead
code by deleting the "x86_64" case in the unixReleaseArch function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 003c1cb2-a913-473e-8232-e18d08816a0d
📒 Files selected for processing (1)
internal/installtest/install_scripts_test.go
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/npmwrapper/npmwrapper_test.go`:
- Around line 70-76: Tests create a 30s context for running the npm wrapper via
exec.CommandContext (ctx, node, wrapperPath, "--version") which is excessive;
change the timeout passed to context.WithTimeout in both test locations (the
blocks invoking exec.CommandContext/CombinedOutput and checking ctx.Err()) down
to a shorter value like 5–10s (e.g., 5*time.Second or 10*time.Second) so
failures surface faster, while keeping the existing ctx.Err() timeout checks and
error formatting intact.
🪄 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: 83c90f5d-a8cb-4fa2-912a-af2518c35782
📒 Files selected for processing (1)
internal/npmwrapper/npmwrapper_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Blockers
- None found.
Non-Blocking
- None.
Looks Good
- The latest head
61985989df4073dfac480a1c79ac6f72a4c879d5cleanly completes the Go-native installer/release validation migration: installer contract tests are now Go tests, CI/release/perf/review paths use direct Go commands, and the npm wrapper remains a minimal Node distribution shim. - The follow-up installer hardening is good: unsupported Unix OS/arch combinations skip explicitly instead of defaulting to Linux/x64, and the no-network installer smoke now includes a checksum-mismatch negative path.
- The npm wrapper tests now make the fixture package type explicit and assert subprocess timeouts, which keeps the Node ESM wrapper checks stable on the review host.
- The PR auto-review workflow keeps the write token in the trusted
actions/github-scriptposting step; PR-controlled validation steps only produce inert outcomes/body content.
Validation run locally on the latest PR head and on a latest-main test merge:
GOCACHE=/tmp/zero-go-cache go test -count=1 ./internal/installtest ./internal/npmwrapper ./internal/release ./internal/review ./cmd/zero-pr-review ./internal/perfbench— passed.GOCACHE=/tmp/zero-go-cache go test -count=1 -p 1 ./...— passed.GOCACHE=/tmp/zero-go-cache go run ./cmd/zero-release build— passed; builtzero.exe.GOCACHE=/tmp/zero-go-cache go run ./cmd/zero-release smoke— passed.node bin/zero.js --version— passed (zero 0.1.0).GOCACHE=/tmp/zero-go-cache go run ./cmd/zero-perf-bench --output dist/perf/perf-bench.json --ci— passed with no warnings.GOCACHE=/tmp/zero-go-cache go run ./cmd/zero-release packageandverify— passed; Windows x64 archive packaged and checksum verified.git diff --check— passed.- Latest-main test merge onto
b70fea7d6ffd4ef085aef71cb95997ce960d86e9merged cleanly and repeated the same validation successfully.
3 participants
Summary
Validation
Summary by CodeRabbit
Documentation
Chores
Tests
Style