Move performance benchmark to Go#87
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. |
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. 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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughMigrates the perf benchmark from Bun/TypeScript to Go: adds internal/perfbench library and a cmd/zero-perf-bench CLI, replaces Bun scripts and artifact helpers, updates npm scripts and CI to run the Go CLI, and adds tests and docs reflecting the new flow. ChangesPerformance Benchmark System Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 (1)
docs/PERFORMANCE.md (1)
12-14: PERFORMANCE.md RSS collection matchesperfbenchimplementation.
perfbenchreads/proc/self/statmviareadLinuxRSSMb()and uses it when available; otherwisereadHarnessMemoryMb()falls back toruntime.ReadMemStats()and reportsstats.Sys(MB). Consider tweaking wording from generic “Go runtime memory” to “MemStats.Sys” for precision.🤖 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 `@docs/PERFORMANCE.md` around lines 12 - 14, The wording in PERFORMANCE.md is imprecise: update the sentence that currently says “Go runtime memory” to explicitly state that the fallback uses runtime.ReadMemStats() and reports MemStats.Sys (MB); reference the existing functions readLinuxRSSMb() and readHarnessMemoryMb() so the doc matches perfbench’s behavior and mentions MemStats.Sys as the reported metric.
🤖 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 `@cmd/zero-perf-bench/main.go`:
- Line 193: The error string in the return statement currently uses "Unknown
option: %s" which violates Go error string capitalization conventions; update
the fmt.Errorf call (the line that returns fmt.Errorf("Unknown option: %s",
arg)) to use a lowercase leading word ("unknown option: %s") so the error starts
with a lowercase letter while preserving the argument formatting.
In `@internal/perfbench/perfbench.go`:
- Line 384: Change the error string returned in the fmt.Errorf call that
references binaryName to follow Go convention: make it start lowercase and
remove trailing punctuation. Replace the current message "No %s binary found.
Run `bun run build` before running the performance benchmark." with a lowercase,
non-terminated sentence such as "no %s binary found; run `bun run build` before
running the performance benchmark" in the fmt.Errorf(...) that returns the
error.
---
Nitpick comments:
In `@docs/PERFORMANCE.md`:
- Around line 12-14: The wording in PERFORMANCE.md is imprecise: update the
sentence that currently says “Go runtime memory” to explicitly state that the
fallback uses runtime.ReadMemStats() and reports MemStats.Sys (MB); reference
the existing functions readLinuxRSSMb() and readHarnessMemoryMb() so the doc
matches perfbench’s behavior and mentions MemStats.Sys as the reported metric.
🪄 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: e87c9b80-3f8e-4d82-acd4-50d0671bc904
📒 Files selected for processing (13)
.github/workflows/ci.ymlREADME.mdcmd/zero-perf-bench/main.gocmd/zero-perf-bench/main_test.godocs/NPM_WRAPPER_SMOKE.mddocs/PERFORMANCE.mdinternal/perfbench/perfbench.gointernal/perfbench/perfbench_test.gopackage.jsonscripts/artifact.tsscripts/perf-bench.tstests/build-scripts.test.tstests/perf-bench.test.ts
💤 Files with no reviewable changes (4)
- tests/perf-bench.test.ts
- scripts/perf-bench.ts
- scripts/artifact.ts
- tests/build-scripts.test.ts
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review: PR #87 — Move performance benchmark to Go (anandh8x / Anandan)
URL: #87
State: OPEN (as of analysis)
Author: anandh8x (Anandan)
Size: +1092 / -927 lines (13 files)
Branch: feat/go-perf-bench
Review Date: 2026-06 (current session)
Reviewer context: Gnanam (runtime core owner per WORK_SPLIT_PRD.md)
Ownership Fit (WORK_SPLIT_PRD v3)
Strong match for Anandan's lane:
- "performance and release checks"
- "CI as product verification"
- "package and binary smoke tests"
- "install/update/release artifact verification"
Explicitly called out in the split as Anandan's responsibility (alongside #84 update verification, #81 sandbox platform reporting, build smoke, checksums, etc.).
This PR delivers user-runnable behavior: new zero-perf-bench binary + bun run perf:smoke / perf:bench wired to the real Go binary (not in-process TS).
Does not touch Gnanam's core contracts (provider events, permission events, session schema, stream-json, agent protocols, config/redaction). It measures the external binary. Good boundary.
Summary of Change
- Introduces
internal/perfbenchpackage (pure measurement harness):Run(ctx, Options)→Resultwith structuredMetrics(NumericStats: min/median/avg/p95/max for cold start, first output, drain, RSS, RSS delta).- Platform capture (OS/Arch/GoVersion).
- Warning evaluation against configurable thresholds (env-overridable:
ZERO_PERF_*). - JSON output (SchemaVersion: 3 — bumped because harness is now Go).
- Linux: reads RSS from
/proc/self/statm; fallback toruntime.MemStats. - Helper to resolve cold-start vs first-output commands (probes the built
zerobinary with a minimal prompt).
- New CLI entry:
cmd/zero-perf-bench/main.go+ tests. Separate from maincmd/zero(correct — smoke/perf is tooling). - CI updates: switches performance smoke from old
scripts/perf-bench.ts --citobun run perf:smoke --ci. - Cleanup: deletes large chunks of old TS perf scripts + tests that assumed the legacy runtime.
- Docs: PERFORMANCE.md, NPM_WRAPPER_SMOKE.md, README layout notes updated.
- Threshold defaults look reasonable (cold 300ms, first-output 500ms, RSS 256MB harness).
Code Quality & Go Idioms
Positive:
- Clean separation:
perfbenchpackage is testable in isolation (go test ./internal/perfbench). - Good stats helpers (sorting + rounding for p95 etc.).
- Context propagation, error returns.
- JSON structs are well-tagged and versioned.
- Platform + command capture for reproducibility.
- Tests include end-to-end minimal benchmark + warning classification + helper process.
Areas to examine / suggestions (pre-merge):
- Measurement fidelity for "first output": The harness execs the binary and watches for first stdout. Confirm it correctly simulates a real user prompt (not just
--versionor empty). The PR description says it probes the agent — good, but the exact prompt/args used inResolveFirstOutputCommandshould be visible and perhaps overridable for future "realistic task" benches. - RSS on non-Linux: The fallback to
runtime.MemStats.Allocor similar is noted; document the approximation clearly (current docs update helps). - Thresholds as product contract: These are now part of the "platform/release contract" Anandan owns. If TUI or runtime later wants to surface perf budgets, coordinate the schema (currently internal + JSON report).
- Large TS deletion: Welcome. The perf script rework now treats the Go binary as a black box (correct post-#78 legacy removal). Make sure
bun run perf:smokestill works from a fresh npm-installed zero (the smoke checklist update is mentioned). - CodeRabbit "Changes requested": Review the specific CodeRabbit feedback on the PR page before merge (it was present at time of analysis).
Tests & Verification
PR claims (and CI runs):
go test ./internal/perfbench ./cmd/zero-perf-bench- Full
go test ./... bun run perf:bench --iterations=1 ...(with warn thresholds relaxed for CI)bun run perf:smoke --cibun run build+ smoke:build
Recommendation: Before approval, run locally:
go test ./internal/perfbench -run TestRunMinimalBenchmarkEndToEnd -count=1
bun run perf:smoke --ciAlso exercise the new binary directly after go run ./cmd/zero-release build or equivalent.
Alignment with Broader Goals (PRD + Split)
- Contributes to "Performance" non-functional reqs in PRD (TTFT, cold start targets).
- Supports Anandan's DoD requirement: "CI jobs that prove real Zero commands work after installation" + performance/release checks.
- Post-legacy-TS world (#78), measuring the Go binary is the only correct thing.
Overall Recommendation
Approve after addressing CodeRabbit feedback (if any blocking) and confirming measurement prompt realism + cross-platform RSS docs.
This is exactly the kind of platform/product-verification work Anandan is supposed to own. Clean boundary with runtime (Gnanam) and TUI (Vasanth).
Status suggestion: LGTM with minor polish on docs/threshold provenance + the first-output command visibility.
Review generated as part of "analyse the repo PRs except gnanam1990 PRs".
Files: reviews/pr-87-anandan-go-perf-bench.md
Review: PR #87 — Move performance benchmark to Go (anandh8x / Anandan)URL: #87 Ownership Fit (WORK_SPLIT_PRD v3)Strong match for Anandan's lane:
Explicitly called out in the split as Anandan's responsibility (alongside #84 update verification, #81 sandbox platform reporting, build smoke, checksums, etc.). This PR delivers user-runnable behavior: new Does not touch Gnanam's core contracts (provider events, permission events, session schema, stream-json, agent protocols, config/redaction). It measures the external binary. Good boundary. Summary of Change
Code Quality & Go IdiomsPositive:
Areas to examine / suggestions (pre-merge):
Tests & VerificationPR claims (and CI runs):
Recommendation: Before approval, run locally: go test ./internal/perfbench -run TestRunMinimalBenchmarkEndToEnd -count=1
bun run perf:smoke --ciAlso exercise the new binary directly after Alignment with Broader Goals (PRD + Split)
Overall RecommendationApprove after addressing CodeRabbit feedback (if any blocking) and confirming measurement prompt realism + cross-platform RSS docs. This is exactly the kind of platform/product-verification work Anandan is supposed to own. Clean boundary with runtime (Gnanam) and TUI (Vasanth). Status suggestion: LGTM with minor polish on docs/threshold provenance + the first-output command visibility. |
Re-review: PR #87 — Move performance benchmark to Go (anandh8x)Updated: 2026-06-06 (after feedback commits) Thanks for the follow-ups. The two recent commits ("Address perf benchmark review feedback" + "Fix perf benchmark pipe drain order") address several of the points from the initial review. What was addressed
Remaining / new observations
RecommendationLGTM — ready to merge. The important measurement correctness fix (pipe drain) landed, previous nits were polished, and CodeRabbit is happy. This delivers exactly the "performance and release checks" / "CI as product verification" work called out for Anandan in the WORK_SPLIT_PRD. Minor future follow-ups (not blocking):
Great work moving the harness to Go and cleaning up the old TS bits. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-reviewed after the feedback commits (pipe drain order fix + docs/error message polish).
All previous concerns addressed. Tests pass, clean boundary with runtime contracts, good platform/perf work per the WORK_SPLIT_PRD.
LGTM. Approving.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review complete after feedback commits (pipe drain fix + polish).
LGTM. Approving as runtime owner — clean platform/perf work, no impact on runtime contracts, tests pass, CodeRabbit green.
2 participants
Summary:
Notes:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes / Chores
Documentation
Tests