Remove legacy TypeScript runtime#78
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. |
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughSwitches perf benchmark from TTFT/in-process sampling to spawn-based binary "first-output" + process-drain measurement with harness RSS; updates README/project layout to Go-native paths, removes several TypeScript barrel/type re-exports and small re-exports, and updates perf tests/docs/CLI/env flags to the new metrics and schemaVersion 2. ChangesTypeScript-to-Go migration + Perf-first-output redesign Surface & barrel export removals
Docs & README
Perf thresholds, types & CLI/env flags
Spawn-based first-output measurement implementation
Perf tests updated
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.
🧹 Nitpick comments (2)
scripts/perf-bench.ts (2)
367-373: 💤 Low valueConsider extracting shared resolution logic.
resolveTtftCommandandresolveColdStartCommandare identical. If they're expected to diverge (different flags, prompts), keeping them separate is fine. Otherwise, a small helper likeresolveZeroBinaryCommand(args: string[])avoids duplication.🔧 Optional consolidation
-async function resolveColdStartCommand(): Promise<string[]> { - if (await Bun.file(zeroArtifactPath).exists()) { - return [zeroArtifactPath, '--version']; - } - - throw new Error(`No ${zeroArtifactName} binary found. Run \`bun run build\` before running the performance benchmark.`); -} - -async function resolveTtftCommand(): Promise<string[]> { - if (await Bun.file(zeroArtifactPath).exists()) { - return [zeroArtifactPath, '--version']; - } - - throw new Error(`No ${zeroArtifactName} binary found. Run \`bun run build\` before running the performance benchmark.`); -} +async function resolveZeroBinaryCommand(): Promise<string[]> { + if (await Bun.file(zeroArtifactPath).exists()) { + return [zeroArtifactPath, '--version']; + } + + throw new Error(`No ${zeroArtifactName} binary found. Run \`bun run build\` before running the performance benchmark.`); +} + +const resolveColdStartCommand = resolveZeroBinaryCommand; +const resolveTtftCommand = resolveZeroBinaryCommand;🤖 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 `@scripts/perf-bench.ts` around lines 367 - 373, Both resolveTtftCommand and resolveColdStartCommand contain identical logic; extract that shared logic into a helper like resolveZeroBinaryCommand(args: string[]) that checks Bun.file(zeroArtifactPath).exists(), throws the same Error if missing, and returns [zeroArtifactPath, ...args] on success, then have resolveTtftCommand call resolveZeroBinaryCommand(['--version']) and resolveColdStartCommand call resolveZeroBinaryCommand([]) or their respective args to remove duplication while preserving behavior.
402-437: ⚡ Quick winRSS metrics now measure the benchmark harness, not the spawned agent.
With the spawn-based approach,
process.memoryUsage().rsscaptures this process's memory, not the child Go binary's. TheagentRssMb/agentRssDeltaMbnames suggest otherwise.If the goal is measuring harness overhead (buffered stdout/stderr), consider renaming to
harnessRssMbor adding a brief comment. If spawned-process memory is desired, that would require platform-specific tooling (e.g.,/proc/[pid]/statmon Linux).🤖 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 `@scripts/perf-bench.ts` around lines 402 - 437, The RSS values returned by measureTtft currently call readRssMb() (measuring the benchmark harness process) but are named agentRssMb/agentRssDeltaMb which is misleading; rename those return properties to harnessRssMb and harnessRssDeltaMb (and update any callers/tests) or, if you prefer not to rename, add a clear comment above measureTtft and next to readRssMb() stating that the RSS is for the harness process (not the spawned child) and that measuring the child would require platform-specific code (e.g., /proc/[pid]/statm on Linux); update all references to agentRssMb/agentRssDeltaMb accordingly to keep names and docs consistent.
🤖 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.
Nitpick comments:
In `@scripts/perf-bench.ts`:
- Around line 367-373: Both resolveTtftCommand and resolveColdStartCommand
contain identical logic; extract that shared logic into a helper like
resolveZeroBinaryCommand(args: string[]) that checks
Bun.file(zeroArtifactPath).exists(), throws the same Error if missing, and
returns [zeroArtifactPath, ...args] on success, then have resolveTtftCommand
call resolveZeroBinaryCommand(['--version']) and resolveColdStartCommand call
resolveZeroBinaryCommand([]) or their respective args to remove duplication
while preserving behavior.
- Around line 402-437: The RSS values returned by measureTtft currently call
readRssMb() (measuring the benchmark harness process) but are named
agentRssMb/agentRssDeltaMb which is misleading; rename those return properties
to harnessRssMb and harnessRssDeltaMb (and update any callers/tests) or, if you
prefer not to rename, add a clear comment above measureTtft and next to
readRssMb() stating that the RSS is for the harness process (not the spawned
child) and that measuring the child would require platform-specific code (e.g.,
/proc/[pid]/statm on Linux); update all references to agentRssMb/agentRssDeltaMb
accordingly to keep names and docs consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7eda09cb-505b-4085-bd79-7d6de8a9c65e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (102)
README.mdindex.tspackage.jsonscripts/perf-bench.tssrc/agent/loop.tssrc/agent/prompts.tssrc/config/loader.tssrc/config/manager.tssrc/config/provider.tssrc/config/types.tssrc/providers/anthropic.tssrc/providers/base.tssrc/providers/gemini.tssrc/providers/openai.tssrc/providers/types.tssrc/tools/apply_patch.tssrc/tools/base.tssrc/tools/bash.tssrc/tools/edit-file.tssrc/tools/edit_file.tssrc/tools/glob.tssrc/tools/grep.tssrc/tools/index.tssrc/tools/list-directory.tssrc/tools/plan.tssrc/tools/read-file.tssrc/tools/read_file.tssrc/tools/registry.tssrc/tools/types.tssrc/tools/write_file.tssrc/version.tssrc/zero-config-inspection/index.tssrc/zero-config-inspection/inspection.tssrc/zero-config-inspection/types.tssrc/zero-doctor/index.tssrc/zero-doctor/report.tssrc/zero-doctor/types.tssrc/zero-hooks/audit.tssrc/zero-hooks/config.tssrc/zero-hooks/index.tssrc/zero-hooks/types.tssrc/zero-mcp/config.tssrc/zero-mcp/index.tssrc/zero-mcp/manager.tssrc/zero-mcp/permissions.tssrc/zero-mcp/tools.tssrc/zero-mcp/transport.tssrc/zero-mcp/types.tssrc/zero-model-registry/cost.tssrc/zero-model-registry/index.tssrc/zero-model-registry/profiles.tssrc/zero-model-registry/registry.tssrc/zero-model-registry/types.tssrc/zero-provider-runtime/index.tssrc/zero-provider-runtime/resolver.tssrc/zero-provider-runtime/types.tssrc/zero-redaction/index.tssrc/zero-redaction/redactor.tssrc/zero-redaction/types.tssrc/zero-runtime/context.tssrc/zero-runtime/index.tssrc/zero-runtime/types.tssrc/zero-search/index.tssrc/zero-search/search.tssrc/zero-search/session-index.tssrc/zero-search/types.tssrc/zero-sessions/exec-session.tssrc/zero-sessions/index.tssrc/zero-sessions/store.tssrc/zero-sessions/types.tssrc/zero-stream-json/index.tssrc/zero-stream-json/protocol.tssrc/zero-stream-json/types.tssrc/zero-usage/index.tssrc/zero-usage/tracker.tssrc/zero-usage/types.tstests/agent-loop.test.tstests/anthropic-provider.test.tstests/config-loader.test.tstests/file-tools.test.tstests/foundation-tools.test.tstests/gemini-provider.test.tstests/openai-provider.test.tstests/perf-bench.test.tstests/plan.test.tstests/provider-base.test.tstests/registry.test.tstests/tool-base.test.tstests/tool-schema.test.tstests/zero-config-inspection.test.tstests/zero-doctor.test.tstests/zero-exec-session.test.tstests/zero-hooks.test.tstests/zero-model-registry.test.tstests/zero-provider-runtime.test.tstests/zero-redaction.test.tstests/zero-runtime.test.tstests/zero-search-index.test.tstests/zero-search.test.tstests/zero-session-events.test.tstests/zero-stream-json.test.tstests/zero-usage.test.ts
💤 Files with no reviewable changes (99)
- index.ts
- src/zero-doctor/index.ts
- src/zero-mcp/index.ts
- src/config/types.ts
- tests/zero-doctor.test.ts
- src/tools/write_file.ts
- src/zero-config-inspection/types.ts
- tests/plan.test.ts
- src/agent/prompts.ts
- src/providers/anthropic.ts
- tests/config-loader.test.ts
- src/zero-search/types.ts
- src/tools/plan.ts
- tests/zero-provider-runtime.test.ts
- tests/zero-hooks.test.ts
- tests/zero-stream-json.test.ts
- src/zero-mcp/manager.ts
- tests/zero-usage.test.ts
- tests/file-tools.test.ts
- src/providers/types.ts
- src/version.ts
- src/zero-usage/types.ts
- src/zero-provider-runtime/types.ts
- src/config/manager.ts
- tests/registry.test.ts
- src/zero-model-registry/types.ts
- tests/openai-provider.test.ts
- tests/tool-schema.test.ts
- tests/zero-config-inspection.test.ts
- tests/zero-search-index.test.ts
- tests/zero-exec-session.test.ts
- tests/tool-base.test.ts
- tests/anthropic-provider.test.ts
- src/tools/read-file.ts
- src/zero-usage/tracker.ts
- src/zero-provider-runtime/index.ts
- src/zero-mcp/config.ts
- src/zero-sessions/exec-session.ts
- src/zero-model-registry/cost.ts
- src/zero-stream-json/index.ts
- src/zero-runtime/types.ts
- src/config/provider.ts
- src/agent/loop.ts
- src/zero-stream-json/types.ts
- src/tools/edit-file.ts
- src/tools/bash.ts
- tests/zero-redaction.test.ts
- src/zero-usage/index.ts
- src/zero-hooks/audit.ts
- tests/foundation-tools.test.ts
- src/providers/base.ts
- src/zero-mcp/tools.ts
- src/tools/index.ts
- src/zero-config-inspection/inspection.ts
- tests/zero-model-registry.test.ts
- src/zero-provider-runtime/resolver.ts
- src/zero-redaction/types.ts
- package.json
- tests/gemini-provider.test.ts
- src/tools/edit_file.ts
- src/zero-config-inspection/index.ts
- src/tools/types.ts
- src/zero-mcp/permissions.ts
- src/zero-runtime/context.ts
- src/tools/base.ts
- tests/zero-runtime.test.ts
- src/zero-runtime/index.ts
- src/zero-stream-json/protocol.ts
- src/zero-model-registry/profiles.ts
- src/tools/grep.ts
- src/zero-hooks/config.ts
- src/zero-search/session-index.ts
- src/tools/apply_patch.ts
- src/config/loader.ts
- src/zero-hooks/index.ts
- src/tools/list-directory.ts
- src/tools/read_file.ts
- src/zero-sessions/types.ts
- src/zero-mcp/transport.ts
- src/tools/glob.ts
- tests/zero-session-events.test.ts
- src/zero-sessions/index.ts
- src/zero-search/index.ts
- src/zero-doctor/types.ts
- src/zero-redaction/redactor.ts
- src/tools/registry.ts
- src/zero-mcp/types.ts
- tests/zero-search.test.ts
- src/providers/gemini.ts
- src/zero-sessions/store.ts
- tests/agent-loop.test.ts
- src/zero-search/search.ts
- src/providers/openai.ts
- src/zero-model-registry/registry.ts
- src/zero-doctor/report.ts
- src/zero-model-registry/index.ts
- src/zero-redaction/index.ts
- tests/provider-base.test.ts
- src/zero-hooks/types.ts
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Blockers
-
The performance benchmark no longer measures the metric names it reports.
scripts/perf-bench.tsnow resolves the TTFT command tozero --versionand measures first stdout/stderr output from that process. The RSS values also come from the Bun benchmark harness process viaprocess.memoryUsage().rss, not the spawned Go agent. However the summary and docs still label these as TTFT and agent RSS, anddocs/PERFORMANCE.mdstill describes TTFT as the first streamed text event from a deterministic provider plus memory as the in-process agent path. Please either restore a Go-native deterministic agent/stream benchmark, or rename/update the metric/schema/docs to describe binary first-output latency and harness RSS accurately. -
The README project layout is stale after this removal. PR #78 deletes the
src/agent,src/providers,src/tools, andsrc/zero-*runtime trees, but the README still listssrc/as the project layout with the transitional TS agent loop, providers, tools, sessions, usage, doctor, stream-json, and other modules. Please update that section to the current Go layout (cmd/*,internal/*, remainingscripts/*, and remainingtests/*) so users do not follow deleted paths.
Validation
I checked the PR branch and a temporary merge onto current origin/main.
git diff --checkgo test -count=1 -p 1 ./...bun install --frozen-lockfilebun run typecheckbun test ./tests --timeout 15000bun run buildbun run smoke:buildbun run smoke:gobun run perf:bench --iterations 1 --warmup 0 --json
The local gates pass and the merge simulation applies cleanly. The request is for correctness of user-facing docs and performance metric semantics before merging.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
- The deletion is the right call now that the Go runtime is the source of truth. The Go side already has full coverage of the M0–M6 surface (agent loop, providers, tools, mcp, hooks, sessions, search, redaction, runtime, model-registry, doctor, config-inspection, usage, stream-json), and the TS modules were dead weight that confused the contribution path. Removing them at the same time as adding the Go versions is exactly the right sequencing.
- Dependency cleanup is real.
execa,openai, andzodare gone frompackage.jsonandbun.lock(-57 lines in the lockfile, no moreopenai/zod/@sec-ant/readable-stream/@sindresorhus/merge-streams/cross-spawn/figures/get-stream/human-signals/is-plain-obj/is-stream/is-unicode-supported/isexe/npm-run-path/parse-ms/path-key/pretty-ms/shebang-command/shebang-regex/signal-exit/strip-final-newline/unicorn-magic/which/ws/yoctocolors). That is a smaller install footprint, a faster CI cold start, and one less attack surface for supply-chain review. scripts/perf-bench.tsis rewritten to measure what matters. The old benchmark ran an in-process TS agent with a stubImmediateTextProvider; that proved the TS loop was fast, not that the shipped binary is fast. The newmeasureTtftspawns the built Go binary, strips provider env vars viaofflineBenchmarkEnv, and times the first byte of stdout/stderr. That is a real cold-start + first-byte measurement of the production binary in a way that can run in CI without network access.resolveTtftCommandandresolveColdStartCommandboth fail loudly if the artifact is missing, so the perf job will surface a missingbun run buildstep instead of silently timing abun --versioncall.- The regression test prevents reintroduction.
tests/perf-bench.test.tsreadsscripts/perf-bench.tsand asserts it does not contain../src/agent/loopor../src/providers/types. That is a one-line migration safeguard; if a future PR imports the legacy runtime back into perf-bench, the test fails before the perf numbers become misleading. index.tsis gone. Theconsole.log("Hello via Bun!")placeholder was misleading after the Go CLI took over. The npm wrapper path is nowbin/zero.ts→scripts/npm-wrapper.ts→ native binary, with thepackage.jsonmoduleandbinfields pointing at the wrapper, so removing the root placeholder is the right cleanup.src/version.tsis also gone. The Go binary owns the version string via thevar version = "dev"linker injection (scripts/build.tsinjects-X main.version=...), so the TS version module was redundant.- README example path is updated.
zero exec "explain src/agent/loop.ts and suggest one improvement"becomeszero exec "explain internal/agent/loop.go and suggest one improvement", which matches the new home of the loop. Small but important for new contributors following the example. - CI does not need a follow-up.
.github/workflows/ci.ymlalready runsbun run test:go,bun run test,bun run typecheck,bun run build,bun run smoke:build,bun run smoke:go, andbun run scripts/perf-bench.ts --ci --output dist/perf/perf-bench.json. All of those still work with the new perf-bench; no workflow edits are required. package.jsonbecomesdevDependencies-only.execa,openai,zodare removed, and there are nodependenciesleft. That is the cleanest possible npm state for a project whose npm package is a thin wrapper around a Go binary.
Observations (non-blocking)
-
ttftMsis no longer "time to first text" in the LLM-stream sense. The newmeasureTtftruns<binary> --version, which exits immediately after writing a single short line. The metric is now "cold-start + first-byte" of the binary, not "time to first model token". A future benchmark that wants a real TTFT would need to either (a) call a--smoke-streamsubcommand that streams a canned response, or (b) hit a fixed offline endpoint. Consider renamingttftMstostartupMsorfirstByteMs(and updatingformatPerfSummaryand the JSON schema) so the metric name matches what it measures. The new name would also be more honest in the threshold config (ttftP95MsbecomesfirstByteP95Ms). -
streamOverheadMssemantics shifted with the rewrite. Previously it wasfirstTextAt - providerFirstByteAt(the gap between the provider starting to stream and the agent emitting text). It is nowfinishedAt - observedFirstOutputAt, i.e., the time from the first byte to process exit. For--versionthat is a few milliseconds; it is still collected, but the number is no longer comparable to the old metric. If the new metric is kept, document it inPerfBenchResult's docstring. -
offlineBenchmarkEnvis a hardcoded env-var deletion list. A future provider (e.g., aMISTRAL_API_KEYorBEDROCK_*env) would not be stripped, so a future--version-style smoke that reads those vars could behave differently in CI vs. dev. A more robust pattern is to start from{ PATH: process.env.PATH, NO_COLOR: '1' }and explicitly add only what the binary needs, instead of cloningprocess.envand deleting entries. The current approach is fine for the immediate need, but the comment should note that adding new provider env vars requires updating this list. -
resolveColdStartCommandandresolveTtftCommandare 80% duplicate. Both callBun.file(zeroArtifactPath).exists()and throw the sameNo ${zeroArtifactName} binary founderror. A small helper likeresolveArtifactPath()returning the validated path (or throwing) would DRY this and ensure the error message stays consistent. Cosmetic. -
The regression test is a string check on source text.
expect(source).not.toContain('../src/agent/loop')will also fail if a comment, docstring, or string literal contains that exact substring. For a one-off migration safeguard that is acceptable, but if anyone refactors perf-bench and adds a doc comment that mentions the legacy runtime for context, the test breaks. A more robust check would be to importperf-benchand assert it does not exportrunAgent(or to scan for^import .* from '\.\./src/regex with proper escaping). -
PerfBenchResultaddsttftCommandbut the oldcoldStartCommandis still typedstring[]only. Acommanddescription for both would be cleaner. Minor; the text formatter already prints both. -
The legacy TS source had a
default exportfromsrc/zero-runtime/index.tsand a few other modules. No one outside the deleted files referenced those exports, so the deletion is safe. The Go side has the corresponding types ininternal/zeroruntime/(kebab-case stream events,agent.Message,agent.Provider, etc.), which the npm wrapper never needs to touch because the Go binary is the entry point. -
No mention of removing
AGENTS.MDreferences to the TS API surface.AGENTS.MD(the Bun guidance file) is a generic Bun cookbook and does not mention Zero-specific code paths, so it is unaffected. If any internal docs indocs/reference the TS API surface, a follow-up grep is worth running. From the diff,docs/INSTALL.md,docs/PRD.md, etc. were not touched, which suggests they do not reference the TS surface. -
bun.lockstill hasbun-typesand the@types/bunblock. Those are dev-time, not runtime, so they should stay. The deletion is correctly limited todependencies. -
The PR description mentions
bun run smoke:buildandbun run smoke:goin the validation list. Both of those scripts likely depend on the built Go binary, so thebun run buildstep must run first. The validation list is in the right order; the perf-benchttftCommanddefault also assumesbun run buildhas produceddist/zero/zero(.exe). -
scripts/perf-bench.tsno longer imports any TS runtime, but it still imports fromnode:fs/promises,node:path, andBun. That is the right surface for a build-script that only manipulates the filesystem and spawns a child. No leftoverexeca,zod, oropenaireferences in the new code. -
tsconfig.jsonis not updated. The"include"(or lack thereof) still applies tosrc/, but withsrc/removed, TypeScript will only typecheckbin/,scripts/, andtests/. Worth verifying thatbun run typecheckstill passes — the validation list includes it, so it should. A small"include"array intsconfig.jsonwould make the new scope explicit and prevent accidental reintroduction ofsrc/references. -
No deprecation notice for downstream consumers. If any external tool imports
zero/agent/loopor another deleted path from the npm package, the nextbun installwill fail to resolve. For an unreleased CLI this is fine; for a published package a major-version bump and aCHANGELOG.mdentry would be required. The PR description notes this is a Go-first rewrite, so consumers are expected to switch to the native binary. -
The
providerFirstByteAtandfirstTextAtlocals are gone, along with theImmediateTextProviderclass. If a future benchmark wants to bring back real-provider TTFT, the previous test coverage of the immediate provider is also gone. That is a deliberate trade-off; the old test only verified a synthetic path. If a real offline smoke provider is added later, it should land with its own test. -
formatPerfSummaryadds aTTFT command:line. ThecoldStartCommandis rendered above it andttftCommandis rendered with the sameformatCommandhelper, which is consistent. A small consistency win: the JSON shape (PerfBenchResult) and the text shape should agree on field names; currentlymetrics.ttftMsexists in both, but the command itself was added asttftCommandeverywhere. Good. -
measureTtftusesPromise.all([child.exited, ...])to await exit and read both streams concurrently. If the binary hangs (e.g., a provider call without offline env), the streams never resolve. There is no timeout on the benchmark itself. A futurerunPerfBenchcould add a per-iterationAbortControllerso a hung binary fails the perf job instead of blocking CI for the default 6-hour GitHub Actions timeout. -
The
streamOverheadMsvalue is computed but unused byformatPerfSummary. The text format printsttftMsandcoldStartMsonly. Either dropstreamOverheadMsfrom the result (it is noise on--version) or add a one-line summary that prints it. Right now it is collected and forgotten. -
The PR removes
src/zero-runtime(the canonical TS runtime types) but the Go equivalent ininternal/zeroruntime/is untouched. That is correct — the Go runtime is the source of truth, and the npm wrapper does not need TS runtime types. Worth a one-lineAGENTS.MDorCONTRIBUTING.mdnote that TS code should not redefine runtime types. -
No new test asserts that the npm wrapper still resolves to a native target. The wrapper has its own tests in
tests/build-scripts.test.ts(from the file list). If those cover theresolveNpmWrapperTargethappy path, the deletion of legacy code does not regress them; if not, a follow-up is worth adding. The PR validation list does not call them out explicitly, so I assumebun test ./testscovers them. -
git diff --check origin/main...HEADis in the validation list but the diff is large (103 files, 13k lines). On a typical CI runner that check is fast; on a developer machine the diff stats are large enough thatgit diff --checkis the cheapest guard. Good to see it included.
No blockers. The deletion is thorough, the perf-bench rewrite is more honest than what it replaced, the regression test is small and correct, and the dependency cleanup is a real win. The two follow-ups I would prioritize next: (1) rename ttftMs/ttftCommand/ttftP95Ms to firstByte* so the metric name matches what is actually measured, and (2) add a per-iteration timeout in runPerfBench so a hung binary fails fast in CI.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/perf-bench.test.ts (1)
52-75: ⚡ Quick winCover
ZERO_PERF_FIRST_OUTPUT_WARN_MSenv parsing explicitly.This test validates the renamed CLI flag, but not the renamed env key for first-output thresholds. Add one env-only assertion to lock that contract and prevent silent regressions.
Suggested test extension
it('parses CLI and environment overrides', () => { @@ expect(options.failOnWarning).toBe(true); + + const envOnly = parsePerfBenchArgs([], { + ZERO_PERF_FIRST_OUTPUT_WARN_MS: '610', + }); + expect(envOnly.thresholds.firstOutputP95Ms).toBe(610); });As per coding guidelines,
tests/**/*.tsshould provide deterministic and meaningful coverage for provider/runtime flow decisions.🤖 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/perf-bench.test.ts` around lines 52 - 75, The test parsePerfBenchArgs coverage is missing an env-only check for the renamed ZERO_PERF_FIRST_OUTPUT_WARN_MS key; update tests/perf-bench.test.ts to include an additional invocation of parsePerfBenchArgs (or extend the existing call) that supplies ZERO_PERF_FIRST_OUTPUT_WARN_MS in the env map and assert that the returned options.thresholds.firstOutputP95Ms equals that value (similar to existing assertions for ZERO_PERF_COLD_START_WARN_MS and ZERO_PERF_HARNESS_RSS_WARN_MB), ensuring the parsing logic in parsePerfBenchArgs correctly picks up the env-only variable.
🤖 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 `@scripts/perf-bench.ts`:
- Around line 442-445: measureFirstOutput currently sets harnessRssMb to
rssAfter (end-state RSS) which contradicts its "peak" semantics and the CLI
gating on metrics.harnessRssMb.max; update measureFirstOutput to either (A)
sample process.memoryUsage().rss periodically during the measured window and set
harnessRssMb to the max observed RSS (use symbols rssBefore, rssAfter, and the
measurement loop inside measureFirstOutput to compute a peak) or (B) change the
metric name/CLI/messages to reflect "end-state RSS" (rename harnessRssMb and any
references to metrics.harnessRssMb.max, help text, and warnings) so the reported
value matches the current two-sample measurement; pick one approach and update
all identifiers and messaging consistently.
---
Nitpick comments:
In `@tests/perf-bench.test.ts`:
- Around line 52-75: The test parsePerfBenchArgs coverage is missing an env-only
check for the renamed ZERO_PERF_FIRST_OUTPUT_WARN_MS key; update
tests/perf-bench.test.ts to include an additional invocation of
parsePerfBenchArgs (or extend the existing call) that supplies
ZERO_PERF_FIRST_OUTPUT_WARN_MS in the env map and assert that the returned
options.thresholds.firstOutputP95Ms equals that value (similar to existing
assertions for ZERO_PERF_COLD_START_WARN_MS and ZERO_PERF_HARNESS_RSS_WARN_MB),
ensuring the parsing logic in parsePerfBenchArgs correctly picks up the env-only
variable.
🪄 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: d5731b45-4a74-4a17-b71f-7821013bbc0a
📒 Files selected for processing (5)
README.mddocs/PERFORMANCE.mddocs/WORK_SPLIT_PRD.mdscripts/perf-bench.tstests/perf-bench.test.ts
✅ Files skipped from review due to trivial changes (2)
- README.md
- docs/WORK_SPLIT_PRD.md
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approved after rereview.
The previous blockers are fixed on current head acf1905:
- Performance benchmark/docs now describe binary first-output and harness end RSS semantics instead of provider TTFT or agent RSS.
- README project layout now points contributors at the Go runtime layout under
cmd/,internal/, remainingscripts/,tests/, anddocs/.
Validation run locally on a detached PR head:
git diff --check origin/main...HEADgo test -count=1 -p 1 ./...bun install --frozen-lockfilebun run typecheckbun test ./tests --timeout 15000bun run buildbun run smoke:buildbun run smoke:gobun run perf:bench --iterations 1 --warmup 0 --json
Review: PR #78 — Remove legacy TypeScript runtime (Vasanthdev2004 / Vasanth)URL: #78 (MERGED) Ownership & Strategic FitVasanth (TUI + overall Go-native direction) appropriately drove the final "the Go runtime is now the source of truth" step.
Approved by Anandan + gnanam1990 + CodeRabbit at merge time. What Was Removed / Changed
Impact on Shared Contracts (Runtime Owner View)
Review Notes
Verdict (Post-Merge)Correct and necessary. This PR (combined with the preceding Go M0-M5 ports by both Vasanth and Anandan, and Gnanam's runtime contracts) marks the end of the migration era and the beginning of the "mature daily-driver" phase described in the v3 WORK_SPLIT. The project is now in a much healthier state for parallel ownership:
Excellent milestone PR. |
Non-gnanam1990 PR Reviews (Zero repo)Context: Analysis of https://github.com/Gitlawb/zero.git PRs, excluding all PRs authored by gnanam1990 (Gnanam / runtime core owner per WORK_SPLIT_PRD.md). Date of analysis: 2026-06 (active sprint) Team lanes (from docs/WORK_SPLIT_PRD.md):
Currently Open (at time of review)
Key Recently Merged (platform + migration, non-gnanam)
Other (older / external)
Overall Observations (as Runtime Owner)
How to Use These Reviews
Next suggested reviews (when new activity appears):
|
3 participants
Summary
src/now that the Go implementation is the source of truth.scripts/perf-bench.tsso perf smoke no longer imports the TS agent loop and instead probes the built Go binary offline.execa,openai,zod) and update the README example path tointernal/agent/loop.go.Validation
bun run typecheckbun test ./tests --timeout 15000go test ./...bun run buildbun run smoke:buildbun run smoke:gobun run perf:smokegit diff --cached --checkNotes
Remaining TypeScript is intentionally limited to the npm wrapper, build/release scripts, and their Bun tests. The Go runtime remains covered by
go test ./....Summary by CodeRabbit
Removals
Refactor
Documentation
Chores