fix(doctor): set non-zero exit code when error-severity issues are found - #300
Merged
gfargo-horizon-agent[bot] merged 8 commits intoAug 2, 2026
Conversation
![gfargo-horizon-agent[bot]](https://avatars.githubusercontent.com/in/4083186?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
🔎 Agent review (kiro · sonnet) — LGTM
REVIEW: LGTM
RESOLVES: full
The PR correctly fixes the zero-exit bug by accumulating a worst-case exit code across all sites and setting process.exitCode after the loop. The literal 3 is replaced with ExitCode.ConfigError and tests cover the key paths.
3 concerns · 1 nit — 4 inline on the diff
Previously doctor always exited 0, making it unusable as a CI/scripting gate and silently breaking the health_check MCP tool (which derives doctor.ok from the exit code). Changes: - Import ExitCode from src/types.ts - Replace hardcoded process.exit(3) with process.exit(ExitCode.ConfigError) in the no-sites-configured guard - Add worstExit accumulator before the per-site loop - Add issueListToExitCode() helper mapping error-severity issues to exit codes with explicit precedence: Auth(5) > Network(4) > Generic(1) - Set process.exitCode = worstExit after the loop (not process.exit()) so --all-sites reports all sites before exiting - Safety net: connectionOk===false with no matching issue also yields NetworkError Closes #267
- Add DoctorIssue.code ('auth' | 'network' | 'generic'), set explicitly
at each push site instead of pattern-matching issue.message later.
- Distinguish network failures from REST API errors by exception type:
WpApiError means the server responded (so a 5xx is a generic error,
not a network error); anything else means fetch() never got a
response at all. This also fixes a real bug where connection-refused
and DNS failures were silently misclassified, since Bun's fetch()
doesn't put Node-style codes like ECONNREFUSED/ENOTFOUND in its
error message text.
- Use err.status === 401 (from WpApiError) instead of matching "401"/
"Unauthorized" in the message for auth detection.
- Replace the explicit precedence if-chain with Math.max(), since the
exit codes produced here (0/1/4/5) are already ordered by severity.
- Drop the connectionOk safety net in issueListToExitCode — the REST
adapter is always constructed, so the branch was unreachable.
- Switch doctor-exit-code.test.ts from node:child_process's spawnSync
to Bun.spawnSync: other unit test files replace the whole
node:child_process module process-wide via mock.module() without
restoring it, which left spawnSync undefined for this file depending
on load order and broke CI.
… tests The doctor exit-code tests simulated an unreachable site with a hardcoded 127.0.0.1:1 and a `.invalid` hostname. CI failed with all four "unreachable site" assertions receiving exit 0 instead of 4, while the exact same test/binary/bun-version combination passed reliably in isolation — pointing at CI's network layer not refusing that fixed port or resolving that reserved TLD the same way a local machine does. Replace both with a real TCP server bound to an OS-assigned loopback port and closed before use, guaranteeing ECONNREFUSED from the kernel with no DNS lookup or fixed port/hostname assumption involved.
Two network-based simulations (hardcoded low port + .invalid hostname, then an OS-assigned loopback port bound and closed before use) both passed locally but produced exit 0 in CI, meaning the REST connectivity check didn't fail the way we expected in that environment. Switch to a syntactically-invalid site URL instead: RestAdapter.apiUrl() throws synchronously from new URL() before any socket opens, so there's no network behavior for CI's environment to diverge on.
…site tests Three different real-network/DNS simulation techniques for the "unreachable site" doctor tests passed locally but produced exit 0 in CI, and the cause didn't point at any single layer under our control. Since doctor never calls process.exit() on this path (only process.exitCode), it's safe to invoke the command in-process and mock globalThis.fetch directly — the same technique already used by dry-run-honesty-behavior.test.ts — making the failure deterministic regardless of environment.
gfargo-horizon-agent
Bot
force-pushed
the
agent/localpress-1354-localpress-267-b-09-doctor-always-exits-
branch
from
August 2, 2026 17:24
0ddc5fd to
81cc23b
Compare
…e tests The in-process doctor tests mock globalThis.fetch to simulate an unreachable site. jSquash WASM codecs use fetch to initialize their binary on first load. If the mock is active when the module first loads, the WASM state is permanently broken in the module cache, poisoning the encoder-preflight tests that run later in the same bun test suite. Add a beforeAll that pre-warms all four jSquash codecs before any fetch mock is installed, ensuring the modules are fully initialized and subsequent encoder-preflight tests are not affected.
….exitCode pollution bun test exits with code 4 when any test sets process.exitCode to a non-zero value, even if it is later reset in afterEach. The in-process doctor tests were setting process.exitCode = 4 (NetworkError) via doctor's action handler, causing bun test itself to exit 4 despite 0 test failures. Fix: extract a test/fixtures/doctor-unreachable.ts harness that mocks globalThis.fetch before running doctor, then invoke it via Bun.spawnSync so the process.exitCode side-effect stays inside the subprocess. The test file now uses only Bun.spawnSync (no in-process invocations), matching the pattern already used by cli-error-handling.test.ts. The doctor.ts implementation is unchanged — this commit is test-only.
Contributor
Author
There was a problem hiding this comment.
🔎 Agent re-review (kiro · sonnet, delta) — LGTM
REVIEW: LGTM
RESOLVES: full
All diagnostic paths now set process.exitCode via a structured code field and issueListToExitCode helper; the hardcoded literal 3 is replaced with ExitCode.ConfigError; tests are correctly subprocess-isolated to avoid poisoning bun's test runner.
gfargo-horizon-agent
Bot
deleted the
agent/localpress-1354-localpress-267-b-09-doctor-always-exits-
branch
August 2, 2026 18:31
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
doctorpreviously always exited 0, even when the connection check failed. This made it unusable as a CI/scripting gate and silently broke thehealth_checkMCP tool, which derivesdoctor.okfrom the exit code.This fix sets a non-zero
process.exitCodewhen any error-severity issue is detected during adoctorrun. Exit code semantics follow the existingExitCodeenum insrc/types.ts.Why
Closes #267
Plane: OSS-1354
How
ExitCodeindoctor.ts(was missing; onlySiteConfigwas imported fromsrc/types.ts)process.exit(3)in the no-sites-configured guard withprocess.exit(ExitCode.ConfigError)worstExitaccumulator before the per-site loopissueListToExitCode(issues, connectionOk)helper with explicit precedence: Auth(5) > Network(4) > Generic(1)process.exitCode = worstExitafter the loop (notprocess.exit()) so--all-sitesreports every site before exitingconnectionOk === falsewith no matching issue still yieldsNetworkErrortest/unit/doctor-exit-code.test.tscovering: no-sites → exit 3, unreachable site → exit 4,--jsonoutput shape unchangedTesting
test/unit/doctor-exit-code.test.ts)🤖 Generated by the harbor agent loop. Reviewed by a human before merge.
Closes #267