Skip to content

feat(cli): add --json output to doctor#320

Merged
jrusso1020 merged 2 commits intoheygen-com:mainfrom
Dylanwooo:feat/doctor-json-output
May 7, 2026
Merged

feat(cli): add --json output to doctor#320
jrusso1020 merged 2 commits intoheygen-com:mainfrom
Dylanwooo:feat/doctor-json-output

Conversation

@Dylanwooo
Copy link
Copy Markdown
Contributor

Summary

hyperframes doctor is one of the first commands users and agents run when something's off — remote bug reports (e.g. #294, #316, #317) typically include a doctor screenshot, which is painful to parse programmatically.

Every other CLI command that reports state already supports --json (info, lint, compositions, catalog, benchmark, validate, capture). This brings doctor in line so:

  • CI pipelines can gate on hyperframes doctor --json without scraping terminal output (exits 1 when any check fails)
  • AI agents consuming diagnostic data get structured input
  • Bug report tooling can attach machine-readable doctor output

Human output is unchanged. This is purely additive.

Output schema

{
  "ok": false,
  "platform": "darwin",
  "arch": "arm64",
  "checks": [
    { "name": "Version",  "ok": true,  "detail": "0.4.4 (latest)" },
    { "name": "FFmpeg",   "ok": true,  "detail": "ffmpeg version 8.1 Copyright (c) 2000-2026 …" },
    { "name": "Docker",   "ok": false, "detail": "Not found",
      "hint": "https://docs.docker.com/get-docker/" },
    { "name": "Docker running", "ok": false, "detail": "Not running",
      "hint": "Start Docker Desktop or run: sudo systemctl start docker" }
  ],
  "_meta": {
    "version": "0.4.4",
    "latestVersion": "0.4.4",
    "updateAvailable": false
  }
}

Uses the existing withMeta() helper so the _meta envelope matches other --json commands.

Design notes

  • Preserved check order. outcomes[] is populated in the same order checks run, so JSON consumers see the same ordering as the human view (Linux-only /dev/shm slots into the middle).
  • Optional hint field. Only emitted when a check has a hint, matching the human mode which only prints hints on failures.
  • Non-zero exit on --json failure. process.exitCode = 1 when any check fails so hyperframes doctor --json || handle_failure works as a CI gate. Human mode still exits 0 (unchanged behavior).
  • No new dependencies. Reuses withMeta() from utils/updateCheck.js.

Test plan

  • bunx oxlint packages/cli/src/commands/doctor.ts — clean
  • bunx oxfmt --check packages/cli/src/commands/doctor.ts — clean
  • Pre-commit typecheck hook passes
  • bunx tsx packages/cli/src/cli.ts doctor — human output unchanged
  • bunx tsx packages/cli/src/cli.ts doctor --json — valid JSON, correct schema
  • bunx tsx packages/cli/src/cli.ts doctor --help--json listed, new example appears
  • Exit code 1 when checks fail, 0 when all pass (CI-gate verified)

Follow-ups (out of scope)

The doctor docs page could be updated to mention --json alongside other --json-aware commands. Happy to do that in a follow-up PR if merged.

`doctor` is one of the first commands users and agents run when something
is off — remote bug reports (e.g. heygen-com#294, heygen-com#316, heygen-com#317) typically include a
doctor screenshot, which is painful to parse programmatically.

Every other CLI command that reports state already supports `--json`
(info, lint, compositions, catalog, benchmark, validate, capture). This
brings `doctor` in line with that convention so:

- CI pipelines can gate on `hyperframes doctor --json` (exit 1 on
  failure) without scraping terminal output
- AI agents consuming telemetry / diagnostic data get structured input
- Bug report tooling can attach machine-readable doctor output

Schema:

    {
      "ok": boolean,
      "platform": "darwin" | "linux" | "win32",
      "arch": "arm64" | "x64" | ...,
      "checks": [
        { "name": "FFmpeg", "ok": true, "detail": "ffmpeg version 8.1 …" },
        { "name": "Docker", "ok": false, "detail": "Not found",
          "hint": "https://docs.docker.com/get-docker/" }
      ],
      "_meta": { "version", "latestVersion", "updateAvailable" }
    }

Uses the existing `withMeta()` helper so the `_meta` envelope matches
other `--json` commands. Human output format is unchanged.
@miguel-heygen miguel-heygen self-requested a review April 18, 2026 16:08
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — direction is great and the change is well-scoped. A couple of things I'd like to see addressed before this merges:

1. Drop process.exitCode = 1 on check failure

Exit code should reflect whether the command succeeded, not whether the environment it inspected is healthy. doctor ran all its checks and produced valid output — that's a successful execution. Whether the result is "ok" is data in the payload, not a property of the run.

Two concrete problems with gating the exit code on allOk:

  • checkVersion poisons the CI-gate use case. It returns ok: false whenever a newer npm version is available, so any pipeline doing hyperframes doctor --json || fail will start failing the next time a new version is published, even if the environment is perfectly fine.
  • Asymmetry with human mode. Bare doctor exits 0 on failure; doctor --json exits 1. Surprising for anyone scripting against it.

Proposal: always exit 0 on successful execution (reserve non-zero for real errors — bad flags, JSON serialization failure, an uncaught exception in a check). Consumers who want to gate can do:

hyperframes doctor --json | jq -e '.ok' > /dev/null || handle_failure

Just as ergonomic, and no version-check footgun. Worth a one-liner in the doctor docs pointing at this pattern.

2. Lock the JSON schema with a test

The PR body publishes a schema that CI pipelines and AI agents are expected to parse. There's currently nothing preventing a future refactor from silently renaming a field, dropping hint, or reordering checks[]. A snapshot test — feed a known environment into the check runner and assert the JSON shape — would lock the contract cheaply. The current test plan is all manual.

Nice-to-haves (non-blocking)

  • Privacy: paths in detail (notably the Chrome check) typically include /Users/<name>/ or /home/<name>/. Same was true in human mode, but JSON output is designed to be pasted into bug reports and agent contexts. Consider $HOME redaction in --json mode.
  • Nit: import { freemem, platform } from "node:os" on line 9 sits below export const examples. Grouping imports at the top is the repo-wide convention.

Security-wise this is clean — all execSync calls are hardcoded, no user input reflected into shell or JSON.

Follows up on jrusso1020's review in heygen-com#320.

Exit code no longer gated on check health
---------------------------------------
`doctor --json` previously set exitCode=1 when any check failed. Two
problems:

- `checkVersion` returns ok:false whenever a newer npm version is
  available, so any pipeline using `hyperframes doctor --json || fail`
  would start failing the next time a new CLI version was published.
- Asymmetric with bare `doctor` which always exits 0.

Exit code now strictly reflects whether the command executed, not
whether the environment is healthy. Consumers who want to gate do:

    hyperframes doctor --json | jq -e '.ok' > /dev/null || handle_failure

Documented that pattern in docs/packages/cli.mdx.

Schema locked with a snapshot test
----------------------------------
Extracted `buildDoctorReport()` as a pure function and added
`doctor.test.ts` covering:

- top-level key set (any accidental rename/addition fails the test)
- shape of each CheckOutcome entry
- ok flag true/false semantics
- check-order preservation
- hint field: omitted when absent, preserved when present
- redact option both on and off

Any future refactor that silently breaks the documented JSON contract
will now fail CI.

$HOME redaction for JSON mode
-----------------------------
JSON output is explicitly designed to be pasted into bug reports and
agent contexts. Added `redactHome()` so the user's home directory is
replaced with the literal `$HOME` in `detail`/`hint` when --json is
set. Human mode is unchanged (shows real paths).

Import grouping
---------------
Moved `node:os` + `_examples` imports up with the rest so `export const
examples` no longer sits between imports.
@Dylanwooo
Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — all four points addressed in 3079e8c.

1. Exit code no longer gated on check health
You're right — the checkVersion poisoning of the CI-gate use case is a real bug. Dropped process.exitCode = 1 entirely. Exit code now strictly reflects whether the command executed. Added the jq -e '.ok' gating pattern to the doctor docs in docs/packages/cli.mdx.

2. Schema locked with snapshot test
Extracted buildDoctorReport() as a pure function and added packages/cli/src/commands/doctor.test.ts (13 tests). Locks the top-level key set, shape of each CheckOutcome, ok-flag semantics, check-order preservation, hint field behavior, and the redact option. Any future refactor that silently renames a field or drops hint will fail the test before it ships.

3. $HOME redaction
Added redactHome() — applied to detail/hint in --json mode only. Human mode still shows real paths (unchanged). Handles both POSIX $HOME and Windows %USERPROFILE%. Covered by 5 dedicated tests.

4. Import grouping
Moved node:os and _examples imports up with the rest; export const examples now sits after all imports.

Re-run locally:

  • bun run --filter @hyperframes/cli test -- doctor.test → 13 passed
  • hyperframes doctor --json exits 0 on failure, 0 on success
  • hyperframes doctor --json | jq -e '.ok' correctly gates on payload health

@Qodo-Free-For-OSS
Copy link
Copy Markdown

Hi, redactHome() replaces any occurrence of the HOME string, so it can corrupt paths where the home path is only a prefix of a longer directory name (e.g., /Users/alice2/... becomes $HOME2/...). This makes doctor --json output inaccurate/misleading for CI/agents consuming detail/hint.

Severity: remediation recommended | Category: correctness

How to fix: Boundary-aware HOME path redaction

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

redactHome() currently does a plain substring replacement (split(home).join("$HOME")). This can corrupt paths when HOME is a prefix of a different path segment (e.g. HOME=/Users/alice, string contains /Users/alice2/...), producing $HOME2/....

Issue Context

JSON mode (hyperframes doctor --json) enables redaction and emits detail/hint for downstream CI/agent consumption. Corrupting paths harms diagnostics quality.

Fix Focus Areas

  • packages/cli/src/commands/doctor.ts[191-209]
  • packages/cli/src/commands/doctor.test.ts[22-63]

What to change

  • Implement boundary-aware replacement so HOME is only replaced when it matches a full path prefix/component, e.g. the match is followed by a path separator (/ or \\) or end-of-string.
  • Consider normalizing home to remove trailing separators before matching.
  • Add a unit test that proves /Users/alice2/... (and Windows equivalent like C:\\Users\\carol2\\...) is not corrupted when HOME is /Users/alice / C:\\Users\\carol.

Found by Qodo code review. FYI, Qodo is free for open-source.

@jrusso1020
Copy link
Copy Markdown
Collaborator

Sorry for the delay here @Dylanwooo just getting a chance to look at this again

Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict

Approve. Re-review: every prior blocking concern is resolved cleanly, and the nice-to-haves landed too. Tests pass locally (13/13 in doctor.test.ts).

Resolution of prior concerns

  • Exit code on check failureprocess.exitCode = 1 is gone. The JSON branch carries an explicit comment (packages/cli/src/commands/doctor.ts:272–277) explaining the checkVersion poison case and pointing consumers at jq -e '.ok'. Same pattern is now documented in docs/packages/cli.mdx under "CI gating".
  • Schema lock testpackages/cli/src/commands/doctor.test.ts:69–79 asserts Object.keys(report).sort() === ["_meta", "arch", "checks", "ok", "platform"], plus targeted tests for ok truth value, check ordering, and conditional hint presence/absence. A future refactor that renames or drops a top-level field will fail this immediately.
  • $HOME redaction — implemented via the exported redactHome() (doctor.ts:196–200) plumbed through buildDoctorReport({ redact: true }). CLI passes redact: true only on the --json path; human mode keeps full paths since the local user wants to know where their stuff lives. Test coverage includes HOME/USERPROFILE fallback, multi-occurrence, no-op, and integration cases.
  • Import grouping — top-of-file imports are now contiguous.

Test Coverage

Solid for the size of the change. The pure-function refactor (buildDoctorReport, redactHome, CheckOutcome) is exactly what makes this testable, and the 13 tests cover the schema contract, the redaction path, and the boundary cases (hint-omitted, HOME-unset, USERPROFILE-fallback). I'd consider this enough — the individual checkX functions touch the host environment and are appropriately not unit-tested.

Nits / Future

  • doctor.ts:269allOk is now used only in the human path; could move it into the else block. Trivial, fine to leave.
  • The _meta envelope's inner shape (e.g. latestVersion shown in the PR body schema) is asserted only via expect.any(...). Not worth a stronger lock here — that contract belongs in withMeta's own tests.

Nice work on the iteration.

@jrusso1020 jrusso1020 merged commit 5212ed4 into heygen-com:main May 7, 2026
13 checks passed
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.

3 participants