fix(doctor): detect missing native binary during runtime checks

[michaelkillgta]

Fixes #405

Summary

When the native binary is missing next to the npm wrapper (ZERO_SKIP_DOWNLOAD=1, failed postinstall, etc.), zero doctor previously gave users a false sense that setup was fine — or a generic wrapper error that did not look like a doctor report — while zero exec and zero providers list failed with the real cause.

Root cause

bin/zero.js is the only entry point for npm-installed zero. When the native binary is absent, the wrapper exits before spawning Go. The old Go-side runtimeCheck (os.Executable() + os.Stat()) could never observe this state: if Go is running, the binary already exists.

Why the fix lives in the wrapper

The missing binary is detected in bin/zero.js at existsSync(nativePath) — before any Go process starts. The wrapper must surface a doctor-shaped failure for zero doctor so the diagnostic command reports the actual problem instead of sending users to debug provider config or network issues.

CLI compatibility

The wrapper mirrors parseDoctorArgs and writeDoctorHelp from the Go doctor command:

Invocation	Output	Exit
zero doctor	Text doctor report on stdout	1
zero doctor --json	doctor.Report JSON on stdout	1
zero doctor --help / help / -h	Help text on stdout	0
zero doctor --bogus	[zero] unknown doctor flag "..." on stderr	2
All other commands	Unchanged generic missing-binary error	1

Only valid doctor invocations use the missing-native-binary fallback. Help and invalid flags behave like the real CLI.

Tests

Added/updated wrapper tests in internal/npmwrapper/npmwrapper_test.go:

• TestNodeWrapperDoctorReportsMissingNativeBinaryAsDoctorFail — text report on stdout, not stderr
• TestNodeWrapperDoctorJSONReportsMissingNativeBinaryAsJSONFail — JSON schema + stdout-only
• TestNodeWrapperDoctorHelpShowsUsage — --help, help, -h
• TestNodeWrapperDoctorRejectsUnknownFlag — unknown flag error on stderr, exit 2
• TestNodeWrapperDoctorWithFlagsStillReportsDoctorFail — --connectivity still hits fallback
• Existing TestNodeWrapperReportsMissingNativeBinary unchanged for non-doctor commands

Test plan

• go test ./internal/npmwrapper/... -count=1
• git diff --check upstream/main

Summary by CodeRabbit

• Bug Fixes

  • When native binaries are missing, doctor now reliably returns a doctor-shaped failure report (stdout) including runtime.go, a remedy that instructs running postinstall.mjs, and correct exit codes (--help exit 0, invalid flags exit 2).
  • doctor --json outputs valid JSON with ok: false and a single failing runtime.go check; generic fallback messaging remains unchanged for non-doctor runs.
  • Bun recovery guidance is consistent between the generic missing-binary output and doctor.

• Tests

  • Added/expanded CLI and output contract tests for doctor (standard/JSON/help/unknown flags) and Bun recovery copy consistency.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 48b16119-169f-4bbe-883f-4118bb6ddf03

📥 Commits

Reviewing files that changed from the base of the PR and between a5d9086 and 6a26a74.

📒 Files selected for processing (2)

• bin/zero.js
• internal/npmwrapper/npmwrapper_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• bin/zero.js
• internal/npmwrapper/npmwrapper_test.go

---

Walkthrough

The wrapper now routes missing-binary doctor invocations to doctor-specific help, JSON, usage-error, or text failure output, while other commands keep the generic missing-binary message. Tests cover exit codes, stdout/stderr separation, and Bun recovery text.

Changes

Doctor-shaped wrapper error report

Layer / File(s)	Summary
Doctor missing-binary branching bin/zero.js	When the native binary is missing and the first argument is doctor, emits doctor help, JSON, or text failure output, rejects invalid doctor flags, and keeps non-doctor commands on the generic missing-binary stderr path; Bun detection is centralized with a simulation override.
Wrapper missing-binary and Bun recovery tests internal/npmwrapper/npmwrapper_test.go	Adds shared fixture helpers and coverage for doctor failure output, doctor --json, doctor --help, invalid doctor flags, valid doctor flags, and Bun recovery messaging shared between the generic and doctor missing-binary paths.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• Gitlawb/zero#88: Also changes the Node CLI wrapper and its missing-native-binary handling.
• Gitlawb/zero#418: Modifies the same missing-native-binary error path in bin/zero.js.
• Gitlawb/zero#452: Updates the Bun-related missing-native-binary messaging that this PR reuses in both wrapper paths.

Suggested reviewers: Vasanthdev2004, gnanam1990

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately reflects the main change: doctor now detects a missing native binary during runtime checks.
Linked Issues check	✅ Passed	The wrapper now makes zero doctor fail on a missing native binary and preserves the requested help, JSON, and error behaviors.
Out of Scope Changes check	✅ Passed	The Bun recovery refactor and test helpers support the doctor/missing-binary fix and do not introduce unrelated product changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

internal/tui/model.go (1)

777-796: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated os.Executable() resolution across CLI and TUI.

This 4-line "resolve exePath, default to empty on error" block is now duplicated verbatim in internal/cli/observability.go and here. Given the existing LookupExecutable field already establishes a "caller resolves, doctor verifies" pattern, this is consistent — but a tiny shared helper (e.g. doctor.ResolveExecutablePath()) would remove the duplication if a third caller shows up later.

🤖 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 777 - 796, The os.Executable() resolution
logic is duplicated between the TUI doctor setup and the CLI observability path,
so extract the “resolve executable path and fall back to empty on error” block
into a shared helper such as doctor.ResolveExecutablePath() and use it from both
call sites. Keep the existing doctor.Options wiring in internal/tui/model.go and
the corresponding CLI code unchanged except for calling the shared helper, so
the caller-resolves/doctor-verifies pattern stays consistent.

internal/doctor/doctor_test.go (1)

553-574: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optionally assert Details["verified"] in the warn test.

The test correctly guards Status != StatusPass/Warn, but doesn't assert check.Details["verified"] == false like the fail-path test does at Line 526. Not blocking since the status assertion already covers the regression this test targets.

🤖 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/doctor/doctor_test.go` around lines 553 - 574, The warn-path test
for runtime.go should also verify the details payload, not just the status. In
TestRuntimeCheckWarnsWhenExecutablePathUnset, assert that
check.Details["verified"] is false, matching the existing fail-path coverage and
using the same report.Check("runtime.go") result to confirm the warning state is
represented consistently.

🤖 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 `@internal/doctor/doctor_test.go`:
- Around line 553-574: The warn-path test for runtime.go should also verify the
details payload, not just the status. In
TestRuntimeCheckWarnsWhenExecutablePathUnset, assert that
check.Details["verified"] is false, matching the existing fail-path coverage and
using the same report.Check("runtime.go") result to confirm the warning state is
represented consistently.

In `@internal/tui/model.go`:
- Around line 777-796: The os.Executable() resolution logic is duplicated
between the TUI doctor setup and the CLI observability path, so extract the
“resolve executable path and fall back to empty on error” block into a shared
helper such as doctor.ResolveExecutablePath() and use it from both call sites.
Keep the existing doctor.Options wiring in internal/tui/model.go and the
corresponding CLI code unchanged except for calling the shared helper, so the
caller-resolves/doctor-verifies pattern stays consistent.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fe1377c7-4290-4bd1-96ac-4f7b1675ed7b

📥 Commits

Reviewing files that changed from the base of the PR and between bdb1b0e and 81d37c5.

📒 Files selected for processing (4)

• internal/cli/observability.go
• internal/doctor/doctor.go
• internal/doctor/doctor_test.go
• internal/tui/model.go

[Vasanthdev2004]

Request changes — right instinct, but this fixes the wrong layer (and CI is red)

Thanks @michaelkillgta — #405 is a real papercut (doctor pointing users away from the actual cause), and the PR is thoughtful: clear comments, a fail-closed default, four focused tests. But as designed I don't think it fixes the reported bug, and it's currently failing CI.

1. The check can't observe the condition it targets (blocker)

The npm wrapper bin/zero.js (unchanged by this PR; guard already on main via #418) checks existsSync(nativePath) for every subcommand — including doctor — and on a missing binary prints "No native binary found" and exit 1 before it ever spawns the Go binary. So:

• In #405's exact repro (ZERO_SKIP_DOWNLOAD=1 … zero doctor), the Go doctor code never runs — the new StatusFail branch can't fire.
• When Go doctor does run, os.Executable() is the process executing it, so os.Stat on it is a tautology → always StatusPass.
• The only reachable non-pass is StatusWarn when os.Executable() itself errors — not the reported condition.

So a Go-side runtimeCheck architecturally can't see "the native binary is missing" (it can't be running when the binary is absent). The fix belongs in the JS wrapper's doctor path — e.g. a preflight, or have the wrapper pass doctor the expected nativePath so it can stat a path other than its own running image. If I'm missing a flow where zero doctor reaches Go with the binary absent, point me at it.

2. CI red — not gofmt-clean (blocker)

Smoke (ubuntu-latest) → Check formatting fails; I reproduced it — gofmt -l flags all four changed files (misaligned Details map literals in doctor.go). gofmt -w the four. (Windows/macOS smoke passing is misleading — the format gate is ubuntu-only.)

Minor

The os.Executable()→empty-on-error block is duplicated in observability.go and model.go, and no test covers the real call-site wiring (every test injects the fields).

Verified locally on Windows: build clean, internal/doctor tests pass — but they validate a mechanism that doesn't map to the real failure mode. Happy to re-review once detection moves to the wrapper layer and CI's green.

[gnanam1990]

VERDICT: needs-changes

REGRESSION RISK: medium

The change replaces the old unconditional StatusPass in runtimeCheck with a real os.Stat of the running binary path. The intent is correct (detect a missing native binary), but the implementation cannot observe the condition it targets: when the native binary is missing, bin/zero.js exits with "No native binary found" before spawning the Go process, so zero doctor never reaches the Go code. When Go doctor does run, os.Executable() returns the path of the currently executing binary, so os.Stat on it is a tautology that always passes. The only reachable non-pass state is StatusWarn when os.Executable() itself errors — which is not the reported condition (#405). The fix belongs in the JS wrapper's doctor path (e.g. have the wrapper pass the expected nativePath so Go doctor can stat a path other than its own running image), or a preflight check in the wrapper itself.

BUILD / TEST

• go build ./... — pass
• go vet ./internal/doctor/... ./internal/cli/... — pass
• gofmt -l on all four changed files — clean (the "style: run gofmt" commit addressed the gofmt blocker from the prior review)
• go test ./internal/doctor/... -run TestRuntimeCheck — all four tests pass, but they validate a mechanism that does not map to the real failure mode (every test injects the fields rather than exercising the real call-site wiring)
• CI status for the latest commit (b527640) could not be confirmed — the check-runs API returned empty for this commit, suggesting CI may not have re-run after the latest push. The prior commit's CI was red on the Ubuntu formatting gate, which is now fixed.

CONTRIBUTING

Author is a community contributor (michaelkillgta). The PR links Fixes #405, which has a bug label but no issue-approved label. Per CONTRIBUTING.md, the issue-approved gate applies to community PRs. Not met. Process violation, but reviewing the code regardless as instructed.

FINDINGS

1. Blocker — internal/doctor/doctor.go:127-170 — the check cannot observe the condition it targets. os.Executable() returns the path of the running Go binary, which by definition exists (it is executing). os.Stat on it is always StatusPass. When the binary is actually missing, the JS wrapper (bin/zero.js, unchanged by this PR) exits before the Go process starts, so the new StatusFail branch is unreachable for the reported scenario. The fix needs to either (a) move detection to the JS wrapper's doctor path, or (b) have the wrapper pass the expected nativePath to the Go doctor so it can stat a path other than its own image.

2. Minor — internal/cli/observability.go:67-70 and internal/tui/model.go:777-790 — duplicated os.Executable() resolution. The 4-line "resolve exe path, empty on error" block is duplicated across CLI and TUI call sites. Consider a shared helper. No test covers the real call-site wiring (every test injects the fields).

EXISTING REVIEWS

• Vasanthdev2004 (CHANGES_REQUESTED on 81d37c5): Identified both blockers — (1) the check being a tautology / unreachable for the reported condition, and (2) CI red from gofmt. Two new commits have since been pushed (c1dd620 "style: remove trailing blank line" and b527640 "style: run gofmt"). Blocker #2 (gofmt) is now resolved — verified clean on the current head. Blocker #1 (architectural) is NOT addressed — bin/zero.js is not in the changed files, and the os.Executable() tautology remains. The review is partially stale (gofmt fixed) but the primary blocker is still valid on the current head.
• coderabbitai (APPROVED + 2 nitpicks): Flagged the duplicated os.Executable() resolution. Valid, trivial. The approval did not catch the architectural blocker — the check cannot observe the missing-binary condition because the JS wrapper exits first.

BOTTOM LINE

The gofmt blocker is fixed, but the architectural blocker remains: a Go-side os.Executable() stat cannot detect a missing native binary because the binary is either running (tautology) or absent (Go never starts); the fix needs to move to the JS wrapper layer.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

bin/zero.js (2)

59-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Hardcoded doctor report text can silently drift from the real doctor.Format output.

This block re-implements the Go-side report shape (header, Overall: fail, [fail] runtime.go - ...) entirely in JS, independent of internal/doctor/doctor.go's renderer (context snippet 2/3). If the check ID, message wording, or "Overall:" formatting changes on the Go side, this wrapper fallback will silently diverge and no longer match what users see when the binary is present — undermining the goal of consistent doctor UX.

Consider adding a lightweight cross-check (e.g., a test asserting the literal strings here match constants/messages defined in doctor.go) so future changes to the Go check don't quietly break this fallback.

🤖 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 `@bin/zero.js` around lines 59 - 89, The doctor fallback text in zero.js is
hardcoded and can drift from the Go renderer, so keep it aligned with
internal/doctor/doctor.go. Update the zero doctor branch to use shared constants
or add a test that compares the JS fallback strings against the doctor.Format
output and runtime.go check message so any future wording or format changes fail
fast. Reference the isDoctor branch and the zero doctor report block when making
the change.

---

78-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Bun remedy block duplicates the existing generic-bail logic.

This ranByBun / trustedDependencies remedy text appears to replicate structure already used by the pre-existing generic wrapper error (implied by the shared ranByBun/postinstallScript variables). Extracting this into a shared helper (e.g., bunRemedyText()) would avoid two copies drifting apart when the messaging is updated.

🤖 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 `@bin/zero.js` around lines 78 - 84, The Bun remedy text is duplicated in the
postinstall error path, which risks the trustedDependencies messaging drifting
from the generic bail logic. Extract the `ranByBun`/trustedDependencies message
assembly into a shared helper such as `bunRemedyText()` and have the existing
error formatting use that helper wherever the Bun-specific remedy is appended.
Keep the shared text in one place so both the generic wrapper error and the
Bun-specific branch stay consistent.

internal/npmwrapper/npmwrapper_test.go (1)

331-337: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Missing explicit exit-code assertion.

Unlike TestNodeWrapperDoctorReportsMissingNativeBinaryAsDoctorFail, this test only checks err == nil and doesn't assert exitErr.ExitCode() == 1. A non-1 nonzero exit (e.g. a Node crash) would pass this check without being caught.

✅ Proposed fix

 	if err == nil {
 		t.Fatalf("doctor --connectivity should not exit 0 when binary missing: %s", output)
 	}
+	exitErr, ok := err.(*exec.ExitError)
+	if !ok || exitErr.ExitCode() != 1 {
+		t.Fatalf("doctor --connectivity err = %v, want exit 1; output: %s", err, output)
+	}
 	got := string(output)

🤖 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/npmwrapper/npmwrapper_test.go` around lines 331 - 337, The test in
the doctor connectivity case only checks that command.CombinedOutput returns a
non-nil error, so it can miss the wrong failure mode. Update this assertion path
in TestNodeWrapperDoctor… to also inspect the returned error as an ExitError and
verify the exit code is exactly 1, matching the behavior already checked in
TestNodeWrapperDoctorReportsMissingNativeBinaryAsDoctorFail. Use the existing
command.CombinedOutput result and the test’s error handling to fail when the
process exits nonzero for any reason other than the expected doctor failure.

🤖 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 `@bin/zero.js`:
- Around line 68-89: The missing-binary branch in bin/zero.js currently always
prints the human-readable doctor message and exits, which breaks the supported
zero doctor --json mode. Update the isDoctor handling so it detects argv entries
for --json and either emits a machine-readable error payload in the same
native-binary-missing path or explicitly rejects JSON mode with a clear CLI
error. Keep the fix localized around the doctor branch and the existing
postinstallScript/ranByBun logic so both plain-text and JSON doctor invocations
behave consistently.

---

Nitpick comments:
In `@bin/zero.js`:
- Around line 59-89: The doctor fallback text in zero.js is hardcoded and can
drift from the Go renderer, so keep it aligned with internal/doctor/doctor.go.
Update the zero doctor branch to use shared constants or add a test that
compares the JS fallback strings against the doctor.Format output and runtime.go
check message so any future wording or format changes fail fast. Reference the
isDoctor branch and the zero doctor report block when making the change.
- Around line 78-84: The Bun remedy text is duplicated in the postinstall error
path, which risks the trustedDependencies messaging drifting from the generic
bail logic. Extract the `ranByBun`/trustedDependencies message assembly into a
shared helper such as `bunRemedyText()` and have the existing error formatting
use that helper wherever the Bun-specific remedy is appended. Keep the shared
text in one place so both the generic wrapper error and the Bun-specific branch
stay consistent.

In `@internal/npmwrapper/npmwrapper_test.go`:
- Around line 331-337: The test in the doctor connectivity case only checks that
command.CombinedOutput returns a non-nil error, so it can miss the wrong failure
mode. Update this assertion path in TestNodeWrapperDoctor… to also inspect the
returned error as an ExitError and verify the exit code is exactly 1, matching
the behavior already checked in
TestNodeWrapperDoctorReportsMissingNativeBinaryAsDoctorFail. Use the existing
command.CombinedOutput result and the test’s error handling to fail when the
process exits nonzero for any reason other than the expected doctor failure.

🪄 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

Run ID: 2bafdb30-474b-4d9f-a932-b375c4bb6131

📥 Commits

Reviewing files that changed from the base of the PR and between b527640 and ed854d1.

📒 Files selected for processing (2)

• bin/zero.js
• internal/npmwrapper/npmwrapper_test.go

[jatmn]

I found a couple of issues that need to be addressed before this is ready.

Findings

• [P2] Complete CodeRabbit's request to preserve doctor --json
bin/zero.js:68
  CodeRabbit's latest review thread is still unresolved and still applies: the new missing-binary doctor branch treats every zero doctor ... invocation as text output. The Go command already supports zero doctor --json and writes the report as JSON, but with the native binary missing this wrapper path now exits before Go starts and prints the human-readable report to stderr with empty stdout. That breaks scripts or CI using the machine-readable doctor mode exactly when diagnostics are needed most. Please either emit a JSON-shaped failure for doctor --json in this fallback or reject JSON mode explicitly, and add wrapper coverage for that flag.

• [P3] Update the PR description to match the wrapper-layer fix
  PR description
  The current PR body still describes the older Go-side approach: it says the solution adds ExecutablePath and StatExecutable to doctor.Options, changes runtimeCheck, and adds TestRuntimeCheck... coverage. The current patch no longer touches those files; it only changes bin/zero.js and internal/npmwrapper/npmwrapper_test.go. Since prior review rejected the Go-side approach as the wrong layer, leaving that stale description and test plan makes it hard to tell what behavior this PR is actually asking maintainers to merge. Please update the summary, root cause, solution, and test plan to describe the current wrapper fallback.

[jatmn]

Thanks for the update. I rechecked the current wrapper-layer patch and found a couple of issues that need to be addressed before this is ready.

Findings

• [P2] Keep the text doctor report on stdout
  bin/zero.js:95
  The new missing-binary zero doctor fallback writes the doctor-shaped text report with console.error, so zero doctor > report.txt captures an empty file even though the normal Go doctor path writes doctor.Format(report) to stdout and uses the exit code to signal failure. The new regression test uses CombinedOutput, so it cannot catch this stream drift. Please write the text doctor report to stdout, like the JSON fallback and the regular doctor command do, while preserving exit code 1.

• [P3] Preserve doctor help and flag parsing in the wrapper fallback
  bin/zero.js:68
  The new branch treats every first-argument doctor invocation as a runtime diagnostic, so zero doctor --help and zero doctor help now return the missing-binary failure report instead of the doctor usage text, and zero doctor --json --bogus returns a runtime JSON report instead of the normal unknown-flag error. The Go runDoctor path handles help before running checks and rejects unknown flags, so the missing-binary wrapper should keep those contracts for doctor rather than treating help and invalid invocations as real diagnostics.

• [P3] Resolve the contribution-policy gate for the linked issue
  CONTRIBUTING.md
  This PR links Fixes #405, but that issue currently has no issue-approved label. CONTRIBUTING.md says community pull requests need an approved parent issue and that approval is shown by the issue-approved label. Please get the parent issue approved/labeled or have a maintainer explicitly waive that gate before continuing the code review cycle.

• [P3] Update the PR description to match the wrapper-layer fix
  PR description
  The current PR body still describes the older Go-side approach: adding ExecutablePath and StatExecutable to doctor.Options, changing runtimeCheck, and adding TestRuntimeCheck... coverage. The current patch only changes bin/zero.js and internal/npmwrapper/npmwrapper_test.go, and prior review rejected the Go-side approach as the wrong layer. Please update the summary, root cause, solution, and test plan so the PR describes the wrapper fallback that is actually being reviewed.

[michaelkillgta]

Hi! I've addressed the latest review comments and updated the implementation to preserve CLI behavior (stdout for doctor reports, help/invalid-flag handling, JSON support). I also noticed the mention of the \issue-approved\ requirement. Could a maintainer let me know whether issue #405 should receive that label for this PR, or whether you'd prefer a different process?

[jatmn]

Thanks for the update. I rechecked the current wrapper-layer patch and found a couple of issues that still need to be addressed before this is ready.

Findings

• [P1] Fix the failing Ubuntu smoke formatting check
  internal/npmwrapper/npmwrapper_test.go:1
  The current Smoke (ubuntu-latest) check is failing in the Check formatting step, and the job log reports gofmt needed on: internal/npmwrapper/npmwrapper_test.go. This is a changed file in this PR, so please run gofmt on the test file and push the formatting fix before the code/test results can be treated as clean.

• [P3] Resolve the contribution-policy gate for the linked issue
  CONTRIBUTING.md:19
  This PR links Fixes #405, but that issue still has no issue-approved label. CONTRIBUTING.md says community pull requests need an approved parent issue and that approval is shown by the issue-approved label. Please get the parent issue approved/labeled or have a maintainer explicitly waive that gate before continuing the code review cycle.

[michaelkillgta]

Thanks for the re-review.

P1 (gofmt): Addressed in \3082dfc\ (\style: gofmt npmwrapper doctor fallback tests). \gofmt -l internal/npmwrapper/npmwrapper_test.go\ is clean locally. The Ubuntu Smoke failure you saw should be resolved on that commit — could you re-run Smoke (ubuntu-latest) when you have a moment? (Fork workflow runs may need approval before checks appear on the latest push.)

P3 (issue-approved): Understood. Issue #405 still doesn't have the \issue-approved\ label. I'm happy to pause the code-review cycle until a maintainer labels #405 or explicitly waives the gate per \CONTRIBUTING.md. Please let me know which you prefer.

[jatmn]

Thanks for the update. I rechecked the current wrapper-layer patch and found a couple of issues that still need to be addressed before this is ready.

Findings

• [P2] Preserve the Bun trust-command recovery path in the doctor fallback
  bin/zero.js:119
  The new missing-binary zero doctor fallback adds its own Bun-specific remedy, but it only tells Bun users to add trustedDependencies. Current main already updated the wrapper's missing-binary guidance to lead with bun pm trust @gitlawb/zero and bun pm -g trust @gitlawb/zero, with trustedDependencies only as the fallback for older Bun versions. This means Bun users who run the diagnostic command now get weaker, stale recovery advice than the generic missing-binary wrapper path. Please keep the new doctor-shaped report aligned with the current Bun guidance and add coverage for the Bun branch so it does not drift again.

• [P3] Resolve the contribution-policy gate for the linked issue
  CONTRIBUTING.md:19
  This PR still links Fixes #405, but that issue has no issue-approved label. CONTRIBUTING.md says community pull requests need an approved parent issue and that approval is shown by the issue-approved label. Please get the parent issue approved/labeled or have a maintainer explicitly waive that gate before continuing the code review cycle.

[jatmn]

Thanks for the update. I rechecked the current wrapper-layer patch and found a few issues that still need to be addressed before this is ready.

Findings

• [P1] Rebase on main and resolve the wrapper conflict first
  GitHub mergeability
  GitHub currently reports this branch as CONFLICTING / DIRTY, and the conflict is in bin/zero.js, which is the file this PR changes. Please rebase on the latest main branch and resolve that conflict before addressing the review findings below, so the missing-binary wrapper behavior is reviewed against the code that will actually be merged.

• [P2] Preserve the current Bun trust-command recovery guidance
  bin/zero.js:74
  The current PR head still builds the shared Bun recovery copy around trustedDependencies only, but current main already changed the missing-binary wrapper guidance to lead with bun pm trust @gitlawb/zero and bun pm -g trust @gitlawb/zero, with trustedDependencies as the fallback for older Bun versions. After rebasing, please keep both the generic missing-binary path and the new zero doctor fallback on that newer bun pm trust guidance, and keep the Bun regression coverage pointed at that current copy so the doctor path cannot drift again.

• [P3] Resolve the contribution-policy gate for the linked issue
  CONTRIBUTING.md:19
  This PR still links Fixes #405, but that issue has no issue-approved label. CONTRIBUTING.md says community pull requests need an approved parent issue and that approval is shown by the issue-approved label. Please get the parent issue approved/labeled or have a maintainer explicitly waive that gate before continuing the code review cycle.

[jatmn]

@Vasanthdev2004

Thanks for the update. I rechecked the current wrapper-layer patch and found one issue that still needs to be addressed before this is ready.

Findings

• [P3] Resolve the contribution-policy gate for the linked issue
  CONTRIBUTING.md:19
  This PR still links Fixes #405, but issue #405 has no issue-approved label. CONTRIBUTING.md says community pull requests need an approved parent issue and that approval is shown by the issue-approved label. Please get the parent issue approved/labeled or have a maintainer explicitly waive that gate before this is treated as ready.

[michaelkillgta]

Hi! My PR (#450) is now approved from a technical standpoint. The remaining blocker appears to be the issue-approved policy requirement. Would a maintainer be able to add the issue-approved label to #405, or let me know if this requirement can be waived for this contribution? Thanks!

[Vasanthdev2004]

Re-reviewing — my earlier request-changes was that this fixed the wrong layer, and that's resolved. The whole fix now lives in bin/zero.js, which is the only place that can actually observe a missing binary: the wrapper exits before the Go process ever starts, so nothing in the Go doctor could see it. The wrapper now special-cases zero doctor to print a doctor-shaped FAIL line for the runtime instead of the generic "no native binary" bail — which is exactly what #405 needed, since the complaint was that the one diagnostic command pointed users away from the real cause.

The stream handling is right, which is the thing that kept coming up: both the text report and the --json report go to stdout with exit 1 (so zero doctor > report.txt and --json | jq both work), unknown flags go to stderr with exit 2, and every non-doctor invocation keeps the original bail untouched. The Bun recovery leads with bun pm trust, matching main, and the test fixture captures stdout and stderr separately now, so it can catch the stream drift the old CombinedOutput test couldn't.

Marked #405 issue-approved — a doctor that greenlights a broken install is a real papercut, and this is the right fix. CI is green and it's mergeable.

Good to merge — over to kevin.