feat(cli): add "testsprite doctor" environment diagnostic#183
Merged
Conversation
One-shot preflight: checks CLI version, Node runtime, profile, API endpoint, credentials, live connectivity (GET /me), and verify-skill install. Prints an OK/WARN/FAIL report and exits non-zero when any check fails, so it gates a CI step or agent preflight. Reuses the real resolution helpers; the API key is never printed. Fixes TestSprite#73
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new ChangesDoctor Command Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DoctorCommand
participant runDoctor
participant HttpClient
participant Output
DoctorCommand->>runDoctor: resolveCommonOptions + invoke
runDoctor->>runDoctor: build checklist
runDoctor->>HttpClient: GET /me for connectivity
HttpClient-->>runDoctor: response or ApiError
runDoctor->>Output: renderDoctor(report)
Output-->>DoctorCommand: printed report
runDoctor-->>DoctorCommand: DoctorReport or CLIError(exitCode 1)
Related issues: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/commands/doctor.test.ts (2)
65-106: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo test for
--debug/verbose logging excluding the API key.The healthy-path suite covers text and JSON output but has no case exercising
debug: truealongside a real API key to confirm debug logging never surfaces it. The referenced documentation explicitly calls out this requirement separately from the general "never print" case.🤖 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 `@src/commands/doctor.test.ts` around lines 65 - 106, Add a healthy-path test in runDoctor that runs with debug enabled and a real API key, then assert the combined stdout/stderr still does not contain the secret. Reuse the existing runDoctor, makeCapture, and healthyDeps helpers from doctor.test.ts, and keep the assertion focused on debug/verbose output specifically so the masking behavior is covered separately from the current “never prints” case.Source: Path instructions
122-176: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFailure tests don't consistently assert the exit code.
Only the first failing-check test (Line 116) checks
rejectiontoMatchObject({ exitCode: 1 }). The invalid-endpoint (Lines 122-133), rejected-key (135-149), non-auth-error (151-163), and outdated-Node (165-176) tests only assertinstanceOf(CLIError), without pinning the exit code. Given the path instructions require exit codes to map to the documented table for every error path, these tests should each also assertexitCode: 1to guard against a future ad-hoc exit code creeping into one specific check path.Example fix (repeat for each failing test)
expect(rejection).toBeInstanceOf(CLIError); + expect(rejection).toMatchObject({ exitCode: 1 });As per path instructions, "Exit-code mapping: error paths must map to the documented exit code (see DOCUMENTATION.md / the exit-code table); don't introduce ad-hoc codes."
🤖 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 `@src/commands/doctor.test.ts` around lines 122 - 176, The failing-path tests in doctor.test.ts are missing the documented exit-code assertion, so update each of the affected runDoctor rejection checks to also verify the CLIError carries exitCode: 1. Use the existing rejection variables in the invalid endpoint, rejected API key, non-auth /me error, and outdated Node cases, and add the same exit-code expectation already used in the first failing-check test so all error paths are pinned to the required mapping.Source: Path instructions
src/commands/doctor.ts (1)
127-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiscards the specific validation reason from
assertValidEndpointUrl.The catch block replaces the thrown error's actual message (which distinguishes an unparsable URL from an unsupported scheme, per
client-factory.ts) with a generic templated string. Since doctor's purpose is diagnosing setup issues, surfacing the original message would give users more actionable detail.♻️ Proposed fix
function checkEndpoint(apiUrl: string): DoctorCheck { try { assertValidEndpointUrl(apiUrl); return { name: 'API endpoint', status: 'ok', detail: apiUrl }; - } catch { + } catch (error) { return { name: 'API endpoint', status: 'fail', - detail: `"${apiUrl}" is not a valid http(s) URL`, + detail: error instanceof Error ? error.message : `"${apiUrl}" is not a valid http(s) URL`, }; } }🤖 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 `@src/commands/doctor.ts` around lines 127 - 138, The checkEndpoint helper is swallowing the specific error from assertValidEndpointUrl and replacing it with a generic message. Update the catch in checkEndpoint to capture the thrown error and use its actual message in the fail detail, so doctor reports the exact validation reason instead of only saying the URL is invalid. Keep the existing API endpoint status shape and preserve the success path unchanged.
🤖 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 `@src/commands/doctor.test.ts`:
- Around line 92-105: The JSON report path in runDoctor is not covered by the
existing API-key masking assertion, so a leak in --output json could go
unnoticed. Extend the doctor test that parses JSON output to verify the raw
serialized stdout and/or parsed report does not contain the API key value from
writeProfile. Use the existing helpers in src/commands/doctor.test.ts,
especially runDoctor, makeCapture, and DoctorReport, and assert that secret
masking is applied in the JSON serialization path as well as the text renderer.
In `@src/commands/doctor.ts`:
- Around line 31-32: The Node.js validation in checkNodeVersion is too coarse
because it only checks the major version via MIN_NODE_MAJOR, so it accepts
unsupported 20.x, 21.x, and 23.x releases. Update the doctor command’s version
check to compare full semver versions against the documented minimums (20.19+,
22.13+, or 24+), and adjust the logic in checkNodeVersion to reject any release
below those thresholds instead of relying on major-only comparison.
- Around line 253-273: The request-timeout handling in
resolveCommonOptions/parseRequestTimeoutFlag silently ignores invalid values
instead of using the shared validation path. Update doctor.ts to reuse the
shared --request-timeout parser from client-factory.ts so malformed,
non-numeric, or non-positive values raise the same validation error as other
commands, while still returning the parsed milliseconds for valid input.
---
Nitpick comments:
In `@src/commands/doctor.test.ts`:
- Around line 65-106: Add a healthy-path test in runDoctor that runs with debug
enabled and a real API key, then assert the combined stdout/stderr still does
not contain the secret. Reuse the existing runDoctor, makeCapture, and
healthyDeps helpers from doctor.test.ts, and keep the assertion focused on
debug/verbose output specifically so the masking behavior is covered separately
from the current “never prints” case.
- Around line 122-176: The failing-path tests in doctor.test.ts are missing the
documented exit-code assertion, so update each of the affected runDoctor
rejection checks to also verify the CLIError carries exitCode: 1. Use the
existing rejection variables in the invalid endpoint, rejected API key, non-auth
/me error, and outdated Node cases, and add the same exit-code expectation
already used in the first failing-check test so all error paths are pinned to
the required mapping.
In `@src/commands/doctor.ts`:
- Around line 127-138: The checkEndpoint helper is swallowing the specific error
from assertValidEndpointUrl and replacing it with a generic message. Update the
catch in checkEndpoint to capture the thrown error and use its actual message in
the fail detail, so doctor reports the exact validation reason instead of only
saying the URL is invalid. Keep the existing API endpoint status shape and
preserve the success path unchanged.
🪄 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: 907a5170-c290-498b-ac9e-9d89f15154bd
⛔ Files ignored due to path filters (1)
test/__snapshots__/help.snapshot.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (3)
src/commands/doctor.test.tssrc/commands/doctor.tssrc/index.ts
Comment on lines
+253
to
+273
| function resolveCommonOptions(command: Command): CommonOptions { | ||
| const globals = command.optsWithGlobals() as Partial<CommonOptions> & { | ||
| requestTimeout?: string; | ||
| }; | ||
| return { | ||
| profile: globals.profile ?? 'default', | ||
| output: globals.output ?? 'text', | ||
| endpointUrl: globals.endpointUrl, | ||
| debug: globals.debug ?? false, | ||
| verbose: globals.verbose ?? false, | ||
| dryRun: globals.dryRun ?? false, | ||
| requestTimeoutMs: parseRequestTimeoutFlag(globals.requestTimeout), | ||
| }; | ||
| } | ||
|
|
||
| function parseRequestTimeoutFlag(raw: string | undefined): number | undefined { | ||
| if (raw === undefined) return undefined; | ||
| const seconds = Number(raw); | ||
| if (!Number.isFinite(seconds) || seconds <= 0) return undefined; | ||
| return Math.round(seconds * 1000); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether request-timeout parsing/validation already exists elsewhere and whether the 1-600 range is enforced anywhere
rg -n "requestTimeout|request-timeout" src --type=ts -C3
ast-grep run --pattern 'function resolveCommonOptions($_) { $$$ }' --lang typescript src
ast-grep run --pattern 'function parseRequestTimeoutFlag($_) { $$$ }' --lang typescript srcRepository: TestSprite/testsprite-cli
Length of output: 37473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the doctor command and the shared timeout parser/validation.
ast-grep outline src/commands/doctor.ts --view expanded
echo '--- doctor.ts around imports/helpers ---'
sed -n '1,340p' src/commands/doctor.ts
echo '--- shared request-timeout parser in client-factory.ts ---'
sed -n '190,245p' src/lib/client-factory.ts
echo '--- request-timeout option definition in src/index.ts ---'
sed -n '48,66p' src/index.tsRepository: TestSprite/testsprite-cli
Length of output: 15215
Reuse the shared --request-timeout parser
doctor.ts should call the shared parser from src/lib/client-factory.ts instead of silently treating malformed values as “unset”. The 600s ceiling is already enforced downstream in makeHttpClient; the gap here is that non-numeric or <= 0 --request-timeout input falls back to the default 120s instead of raising the same validation error as the other commands.
🤖 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 `@src/commands/doctor.ts` around lines 253 - 273, The request-timeout handling
in resolveCommonOptions/parseRequestTimeoutFlag silently ignores invalid values
instead of using the shared validation path. Update doctor.ts to reuse the
shared --request-timeout parser from client-factory.ts so malformed,
non-numeric, or non-positive values raise the same validation error as other
commands, while still returning the parsed milliseconds for valid input.
Source: Path instructions
- Node check reuses the CLI runtime guard (shouldRejectNodeVersion) instead of a hardcoded major floor, so the verdict matches what the entrypoint enforces at startup; the precise 20.19+/22.13+/24+ engines are enforced by npm engine-strict. - --request-timeout raises a validation error on malformed input instead of silently defaulting, matching the other commands. - The --output json test asserts the API key never appears in the JSON path (distinct from the text renderer already covered).
Contributor
Author
|
Addressed the review:
|
zeshi-du
approved these changes
Jul 6, 2026
zeshi-du
left a comment
zeshi-du
left a comment
Contributor
There was a problem hiding this comment.
Reviewed: scoped, tests included, CI green across the matrix.
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
2 participants
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.
Adds
testsprite doctor: a one-shot environment diagnostic.Runs a fixed checklist and prints an OK/WARN/FAIL report: CLI version, Node.js runtime, active profile, API endpoint, credentials, live connectivity (GET /me + key validity), and whether the verify skill is installed. Exits non-zero when any check FAILS, so it gates a CI step or an agent preflight (testsprite doctor && testsprite test run ).
Every check reuses the same helpers the real commands use (loadConfig, assertValidEndpointUrl, makeHttpClient, isVerifySkillInstalled), so the report reflects what a subsequent command resolves. The API key is never printed (credentials check confirms presence only).
Fixes #73
Summary by CodeRabbit
doctorcommand to diagnose CLI setup and environment health.