Skip to content

fix(doctor): exit nonzero on failed checks#23

Open
NianJiuZst wants to merge 4 commits into
Tencent:mainfrom
NianJiuZst:codex/doctor-fails-nonzero
Open

fix(doctor): exit nonzero on failed checks#23
NianJiuZst wants to merge 4 commits into
Tencent:mainfrom
NianJiuZst:codex/doctor-fails-nonzero

Conversation

@NianJiuZst

@NianJiuZst NianJiuZst commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

bsk doctor rendered individual checks as ok, FAIL, or N/A, but the top-level command discarded those results with .map(|_| ()). As long as collecting and printing the report itself did not encounter an I/O/serialization error, the process returned exit code 0—even when one or more checks explicitly reported FAIL.

That makes the command unreliable for every non-interactive consumer. Shell scripts using set -e, installers validating a setup, CI smoke tests, and monitoring tools all interpreted an unhealthy BrowserSkill installation as healthy unless they separately parsed human text or JSON fields.

How This Fix Works

  • Add a single has_failures verdict helper that treats only CheckStatus::Fail as unsuccessful. NotApplicable remains informational and does not fail the command.
  • Keep rendering the complete human or JSON report exactly as before, then return exit code 1 if any active check failed.
  • Add a CliError::RenderedExit path for commands that have already emitted their full result but need a command-specific status code. This avoids printing a duplicate error and avoids misclassifying a failed health check as a daemon/RPC protocol error.
  • Preserve exit code 0 when every check is either ok or N/A.
  • Update end-to-end tests for disconnected extensions, unreachable/non-daemon PIDs, and daemon metadata PID mismatches to assert both the structured report and the new non-zero status.

User Impact

bsk doctor is now safe to use as a health gate:

if bsk doctor; then
  echo "BrowserSkill is healthy"
else
  echo "BrowserSkill needs attention"
fi

Users still receive the complete diagnostic table or JSON array, including all repair hints. The only behavior change is that an actual FAIL now produces exit code 1, while N/A alone does not.

CI Baseline Compatibility

This branch also contains the one-line Biome 2.4 formatting update required by the current lockfile. upstream/main currently fails its own Frontend CI job on this pre-existing vitest.config.ts formatting mismatch; including the minimal formatter output allows this PR's workflow to pass lint and reach the remaining checks.

Validation

  • cargo fmt --check
  • cargo clippy --workspace --all-targets --locked -- -D warnings
  • cargo test --workspace --locked
  • End-to-end status_cmd suite verifies exit code 1 while preserving valid human/JSON doctor output.
  • Unit coverage verifies that FAIL changes the verdict, N/A does not, and the rendered-only exit path has no fabricated protocol code.
  • node --test scripts/*.test.mjs
  • pnpm lint
  • pnpm --filter @browser-skill/extension compile
  • pnpm ext:test (35 files, 373 tests)
  • pnpm ext:build

CI Reliability Follow-up

GitHub Actions exposed a pre-existing test-only port allocation race: integration helpers probed an ephemeral port, released it, and then asked the daemon to bind the same number, allowing another process to claim it in between. The resulting Address already in use failure was unrelated to the functional changes. The PR now lets each daemon bind port 0 directly and reads the assigned address from its handle, removing the TOCTOU window across all affected integration helpers.

@NianJiuZst NianJiuZst marked this pull request as ready for review July 10, 2026 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant