feat(cli): add doctor diagnostics and help snapshots#43
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds ChangesCLI doctor diagnostics command and help system
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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
🤖 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 `@packages/cli/src/commands/doctor.ts`:
- Around line 152-160: The diagnostics probe in doctor.ts currently returns the
full `result` from `ctx.api.invoke` (the `/requests` GET) as `details`, which
may leak sensitive payloads; change the probe to call `ctx.api.invoke` as it
does now but replace `details: result` in the returned object with a sanitized
summary (e.g., HTTP status, total/count of items, truncated or masked request
IDs, and any non-sensitive timestamps) and ensure any user/request fields are
omitted or redacted before returning; update the code around the invocation and
the return statement that references `result` so only this minimal metadata is
exposed.
In `@packages/cli/src/runtime.ts`:
- Around line 156-166: The help flag (-h/--help) is being interpreted as a
positional token because renderHelp calls parseCliArguments(argv) directly; to
fix, strip known help flags from argv before calling parseCliArguments in
renderHelp so that -h/--help are not included in commandTokens, then continue to
call resolveCommand and renderCommandHelp/renderCommandGroupHelp as before
(update renderHelp to filter argv for "-h" and "--help" prior to
parseCliArguments).
In `@packages/cli/test/help.test.ts`:
- Around line 6-14: The test helper renderHelp currently returns only the first
captured stdout entry (stdout[0]) which misses multi-line/multi-write help
output; update renderHelp to join all captured output entries (e.g., using
stdout.join("\n") or similar) and return that combined string (falling back to
"" when empty) so snapshots receive the full help output; modify the code around
the stdout array and the return in the renderHelp function accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecd9e62a-6885-4c90-80ff-a2a3ee56cf0c
⛔ Files ignored due to path filters (1)
packages/cli/test/__snapshots__/help.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
docs/reference/developer/cli.mdxpackages/cli/README.mdpackages/cli/src/commands/doctor.tspackages/cli/src/commands/system.tspackages/cli/src/config.tspackages/cli/src/runtime.tspackages/cli/src/types.tspackages/cli/test/doctor.test.tspackages/cli/test/e2e/parity-guard.e2e.test.tspackages/cli/test/help.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/cli/test/help.test.tspackages/cli/test/e2e/parity-guard.e2e.test.tspackages/cli/src/commands/system.tspackages/cli/src/types.tspackages/cli/test/doctor.test.tspackages/cli/src/config.tspackages/cli/src/runtime.tspackages/cli/src/commands/doctor.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/cli/test/help.test.tspackages/cli/test/e2e/parity-guard.e2e.test.tspackages/cli/src/commands/system.tspackages/cli/src/types.tspackages/cli/test/doctor.test.tspackages/cli/src/config.tspackages/cli/src/runtime.tspackages/cli/src/commands/doctor.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/cli/test/help.test.tspackages/cli/test/e2e/parity-guard.e2e.test.tspackages/cli/test/doctor.test.ts
🔇 Additional comments (8)
packages/cli/test/doctor.test.ts (1)
1-110: LGTM!packages/cli/test/help.test.ts (1)
16-33: LGTM!packages/cli/test/e2e/parity-guard.e2e.test.ts (1)
7-7: LGTM!docs/reference/developer/cli.mdx (1)
19-23: LGTM!Also applies to: 53-69, 100-100, 104-108
packages/cli/README.md (1)
20-20: LGTM!Also applies to: 44-47, 56-60
packages/cli/src/types.ts (1)
107-108: LGTM!packages/cli/src/config.ts (1)
13-131: LGTM!packages/cli/src/commands/system.ts (1)
7-10: LGTM!
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
TL;DR — Adds a dsar doctor preflight that probes API-URL config, /status, and an authenticated read; expands --help to per-command and group output backed by golden snapshots; documents exit codes.
Key changes
- Doctor diagnostics command —
dsar doctorchains config →/status→ authenticated probe → migration/adapter checks and exits non-zero when any check fails. - Per-command and group help —
dsar <command> --helpanddsar requests --helproute viaparseCliArguments+resolveCommand, falling back to a prefix-match group listing. - Help snapshots — golden snapshots for every registered command, the global help, and the
requestsgroup. - Diagnostic exit envelope — runtime adds an
{ ok: false, data, meta }envelope for commands that report failure via result rather than throw, gated by a new optionalCommandDefinition.isSuccessfulResultpredicate. - Config plumbing —
parseCliInputacceptsallowMissingApiUrlsodoctorcan report a missing URL as a check instead of throwing;parseFlagsis renamed and exported asparseCliArguments. - Docs — CLI MDX and CLI README gain Diagnostics and Exit Codes sections.
Summary ｜ 11 files ｜ 1 commit ｜ base: main ← fix/13-cli-doctor-help
Doctor diagnostics
Before: No first-party CLI command to verify CLI/runtime wiring.
After:dsar doctorruns config →/status→ authenticatedGET /requests?limit=1→ migration/adapter checks, with downstream checks short-circuiting toskipwhen an upstream check did not pass.
The probe is conservative: a missing token surfaces as warn, not fail; backend surfaces that don't yet exist (migration freshness, adapter health) surface as warn/skip instead of pretending to validate. Failure of any check returns exit 1 via the new isSuccessfulResult predicate. Tests exercise the missing-URL, happy-path, and bad-token branches against the in-process runtime.
packages/cli/src/commands/doctor.ts · packages/cli/src/runtime.ts · packages/cli/src/types.ts · packages/cli/test/doctor.test.ts
Help routing and snapshots
Before:
--helponly rendered the global command listing.
After:dsar <command> --helpresolves to a per-command panel;dsar requests --helplists every command whose usage starts withrequests; unmatched prefixes print "No commands matched."
Golden snapshots cover the global help, every registered command, and the requests group, giving a single-file diff when descriptions or usage tokens change.
packages/cli/src/runtime.ts · packages/cli/test/help.test.ts · packages/cli/test/__snapshots__/help.test.ts.snap
Config and runtime plumbing
Before:
parseCliInputalways threw on missing API URL during arg parsing.
After: Callers can passallowMissingApiUrl: true(the runtime sets this only when the top-level token isdoctor) so the diagnostic surface can report a missing URL as a failed check instead of an exception.
isDoctorRequest is evaluated after the optional interactive wizard returns, so wizard-selected doctor runs also benefit from the bypass.
packages/cli/src/config.ts · packages/cli/src/runtime.ts · packages/cli/src/commands/system.ts
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Sanitized the authenticated probe
detailsto{ probe: "GET /requests?limit=1" }so the response payload is no longer echoed in diagnostic output. - Stripped
--help/-hfrom argv viawithoutHelpFlagsbeforeparseCliArgumentsso help flags no longer leak intocommandTokens. - Replaced the hardcoded
isDoctorRequestruntime check with a declarativeCommandDefinition.allowMissingApiUrlfield, resolved from the matched command beforeparseCliInput. - Introduced
CommandDefinition.formatTextResultand aformatDoctorReportformatter so text-mode doctor output renders a readable per-check summary instead of pretty-printed JSON. - Renamed
test/doctor.test.tstotest/e2e/doctor.e2e.test.tsand switched the test harness to join the full stdout buffer rather than indexing[0]. - Added
-hparity snapshots covering global, command, and group help, plus a text-mode doctor rendering test.
Claude Opus ｜ 𝕏
|
Updated after review:
Validation passed locally:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/reference/developer/cli.mdx`:
- Line 60: Replace the vague phrase "persistence-backed read path through the
authenticated probe" with a clearer description of what is being verified;
update the sentence in docs/reference/developer/cli.mdx to something like
"backend database read operations via the authenticated request" or "that the
runtime can query persisted data when authenticated" (choose the one that
matches intent) so readers immediately understand it verifies authenticated
reads against persisted storage.
In `@packages/cli/test/e2e/doctor.e2e.test.ts`:
- Around line 22-28: The parseDoctorEnvelope function currently uses a raw type
assertion JSON.parse(output) as DoctorEnvelope which bypasses runtime
validation; replace that with parsing to unknown then validate with a runtime
type guard (e.g., implement isDoctorEnvelope(obj): obj is DoctorEnvelope) to
check required fields/types and throw a clear Error when validation fails so
tests fail with a helpful message; update parseDoctorEnvelope to call
JSON.parse(output) to get unknown, run isDoctorEnvelope on the result, return
the narrowed DoctorEnvelope when valid, otherwise throw an Error describing the
malformed response.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc877b93-2a1e-4e6c-bcfa-2d114b86d216
📒 Files selected for processing (7)
docs/reference/developer/cli.mdxpackages/cli/README.mdpackages/cli/src/commands/doctor.tspackages/cli/src/runtime.tspackages/cli/src/types.tspackages/cli/test/e2e/doctor.e2e.test.tspackages/cli/test/help.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/cli/test/e2e/doctor.e2e.test.tspackages/cli/test/help.test.tspackages/cli/src/types.tspackages/cli/src/runtime.tspackages/cli/src/commands/doctor.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/cli/test/e2e/doctor.e2e.test.tspackages/cli/test/help.test.tspackages/cli/src/types.tspackages/cli/src/runtime.tspackages/cli/src/commands/doctor.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/cli/test/e2e/doctor.e2e.test.tspackages/cli/test/help.test.ts
🔇 Additional comments (20)
packages/cli/test/e2e/doctor.e2e.test.ts (1)
1-21: LGTM!Also applies to: 30-146
packages/cli/test/help.test.ts (1)
1-51: LGTM!docs/reference/developer/cli.mdx (3)
19-23: LGTM!
107-110: LGTM!
103-103: ⚡ Quick winNo changes needed. The documented diagnostic failure envelope format
{ ok: false, data, meta: { command } }is correct and matches the implementation inpackages/cli/src/runtime.ts(lines 45-52). The unusual pairing ofok: falsewithdatainstead oferroris intentional—it allows the doctor command to return partial diagnostic results even when checks fail.packages/cli/README.md (4)
20-20: LGTM!
44-44: LGTM!
46-47: LGTM!
57-60: LGTM!packages/cli/src/types.ts (1)
97-112: LGTM!packages/cli/src/commands/doctor.ts (6)
1-36: LGTM!
38-73: LGTM!
75-101: LGTM!
103-175: LGTM!
177-220: LGTM!
222-257: LGTM!packages/cli/src/runtime.ts (4)
15-81: LGTM!
83-174: LGTM!
232-291: LGTM!
292-302: LGTM!
devcool20
force-pushed
the
fix/13-cli-doctor-help
branch
from
3fcf26a to
22d3914
Compare
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Reworded the diagnostics bullet in
docs/reference/developer/cli.mdxfrom "persistence-backed read path through the authenticated probe" to "runtime database read operations through the authenticated request probe". - Replaced the raw
JSON.parse(output) as DoctorEnvelopeassertion inparseDoctorEnvelopewithisDoctorEnvelope/isDoctorCheckruntime type guards that throw a descriptive error on malformed output.
Claude Opus ｜ 𝕏
|
Maintainer review + update: the original scope was solid (doctor command, help snapshots, exit codes, honest warn/skip for missing backend surfaces), and the diff is clean from a supply-chain standpoint (no new deps, no workflow changes). Since we only merge full fixes, I pushed follow-up commits completing issue #13:
Verified: full backend (100), cli (159), and persistence (23) suites pass; ultracite, tsdoc, error-docs, docs-lint, alpha-warning all clean. Note for merge sequencing: this touches the same CLI parity/help files as #44, so whichever lands second needs a trivial rebase. |
There was a problem hiding this comment.
ℹ️ No critical issues — one minor, defer-able suggestion inline.
Reviewed changes — the backend diagnostics endpoint added since the prior pullfrog review (22d3914) and the wiring that turns dsar doctor's migration/adapter checks from always-warn/skip into real pass/warn/fail results.
- Add operator-scoped
GET /status/diagnostics— new OpenAPI group entry andRouteDefinitioninpackages/backend/src/routes/status.ts, gated byrequirePrincipalKinds(["operator","service"]), returning migration freshness, per-adapter health, andpersistence.reachable. Per-adapterhealthCheck()/diagnostics()failures are caught and reported asdown/error-details rather than failing the whole request. - Expose migration freshness from persistence —
readMigrationStatus(sql)andcurrentPersistenceMigrationStatus()inpackages/internals/persistence, plus an optionalmigrationStatus?()onPersistenceServiceand newPersistenceMigrationStatus/PersistenceMigrationInfotypes. - Wire
dsar doctorto the endpoint —fetchDiagnostics+ runtime type guards (isDiagnosticsData,unwrapDiagnostics);checkMigrationsnow fails on stale migrations andcheckAdaptersfails/warns on down/degraded adapters, with graceful warn/skip degradation when the endpoint 404s on older backends. - Tests + docs — backend
status.test.ts(403 subject / 200 operator), tenant-isolation probe, OpenAPI snapshot, a CLI stale-migration fail test, paritycompositeCommandRoutesexclusion, and updated CLI docs/README/changeset.
ℹ️ A permission error on /status/diagnostics degrades differently than a missing endpoint
The auth probe (GET /requests?limit=1) has no principal-kind gate, so any authenticated actor — including a subject principal — passes it, then hits 403 on the operator-scoped diagnostics endpoint. Only a 404 is treated as "endpoint unavailable" (warn/skip); every other status, 403 included, becomes a hard fail on both the migrations and adapters checks. The net effect is that a non-operator token makes dsar doctor fail as if the backend were broken, rather than signalling a permissions gap.
Technical details
# 403 on diagnostics is a hard fail, not a graceful degrade
## Affected sites
- `packages/cli/src/commands/doctor.ts:117-118` — `isUnavailableDiagnosticsError` matches only `/\(404\)/`.
- `packages/cli/src/commands/doctor.ts:281-286` and `:318-323` — non-404 errors become `fail` for migrations/adapters.
- `packages/backend/src/routes/requests/core.ts:145-148` — `GET /requests` (the auth probe) is tenant-scoped only, no `requirePrincipalKinds`, so subject tokens pass the probe.
- `packages/backend/src/routes/status.ts:61-65` — `/status/diagnostics` requires `operator`/`service`, so those same tokens get 403.
## Required outcome
- A token that authenticates but lacks operator/service scope should not make `doctor` look like the backend is broken. A `403` should be distinguishable from a genuine backend/persistence failure.
## Suggested approach (optional)
- Treat `403` like the unavailable case (skip/warn with a "requires operator or service access" message) rather than a hard fail, e.g. broaden the predicate to also match `(403)` or key off the parsed status.
## Open questions for the human
- Is `dsar doctor` intended to be operator-only? If so, a hard fail on `403` may be acceptable and this is purely a messaging nicety.Claude Opus ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the single commit since the prior pullfrog review (092aa96), which makes dsar doctor degrade gracefully when the configured token lacks operator/service access to /status/diagnostics, directly resolving the round-4 informational finding.
- Treat
401/403diagnostics errors aswarn— newisForbiddenDiagnosticsError(regex/\((?:401|403)\)/) is checked ahead of the general non-404fail branch in bothunavailableMigrationCheck(newly extracted) andunavailableAdapterCheck, returning awarnwith a message that points the user to rerun with an operator or service token. Because awarndoes not incrementsummary.failed, the doctor exit code stays0. - Add e2e coverage —
warns instead of failing when diagnostics are forbidden for the tokenstubs/status/diagnosticsto403(other routes hit the real in-process runtime) and asserts exit0with bothpersistence.migrationsandadapters.healthreported aswarn.
Claude Opus ｜ 𝕏
|
DX follow-up pushed in 378240b: |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the single commit since the prior pullfrog review (ceb2d66), which surfaces command-specific flags in dsar <command> --help.
- Add optional
CommandDefinition.flagHelp— areadonly string[]of preformatted flag lines, rendered by the newrenderCommandFlagLineshelper inruntime.tsas a "Command flags:" block above the global flags. The helper returns an empty array when the field is absent, so commands without command-specific flags render unchanged. - Populate
flagHelponauditTailCommand— documents--request(required) and--interval(default 2000). These match the flags the command actually reads; the test-only--max-pollsbound is intentionally left undocumented. - Snapshot coverage — the
audit tail --helpgolden snapshot gains the new block, andit.each(allCommands)exercises the rendering for every command with-h/--helpparity asserted by equality.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@packages/backend/src/http-api/groups/status.ts`:
- Around line 53-55: The persistence status schema in the diagnostics response
is hardcoded to reachable true, so update the status model in the group/status
definitions to allow a boolean value instead of a literal. Then fix the status
handler in routes/status to catch failures from the migration status call and
return a reachable false response with error details rather than failing the
entire endpoint.
In `@packages/backend/src/routes/status.ts`:
- Around line 66-69: The diagnostics handler currently lets
`services.repos.persistence.migrationStatus?.()` fail the whole `status` route,
so update the migration path in `status.ts` to catch errors the same way the
adapter health checks do and return a persistence status object with `reachable:
false` plus error details on failure. Also update the `persistence` schema in
`http-api/groups/status.ts` from `Schema.Literal(true)` to `Schema.Boolean` so
the endpoint can represent both healthy and failed persistence states.
- Around line 77-79: The health check in status handling drops the failure
details because the catch in the adapter health flow ignores the error. Update
the `adapter.healthCheck().pipe(Effect.catch(...))` path in `status.ts` so the
handler captures the error and returns a down result with `details` populated,
matching the diagnostics catch behavior and allowing `toDetails` to surface the
message in the response.
In `@packages/backend/test/routes/status.test.ts`:
- Around line 44-83: Add coverage in status.test.ts for the diagnostics error
paths in the handler: the current tests only verify the forbidden subject case
and the healthy adapter path. Extend the status route tests around
makeRuntime()/handler to assert that a failing adapter healthCheck() is reported
as down, and add a separate test for the diagnostics() failure catch path if
that branch is reachable from the same diagnostics flow. Use the existing
DiagnosticsEnvelope expectations and the adapter fixture setup (for example the
notifications fixture) so the new tests clearly exercise the adapterDown
fallback and error handling in the diagnostics route.
In `@packages/cli/test/e2e/doctor.e2e.test.ts`:
- Around line 165-281: Add e2e coverage in doctor.e2e.test.ts for the missing
adapter classification branches in checkRegisteredAdapters. Introduce helper
fetch fixtures alongside makeRuntimeFetch, such as a degraded adapter response
and a down adapter response, then add tests under describe("doctor command")
that call runDoctorJson with E2E_API_TOKEN and assert adapters.health returns
warn for degraded and fail for down, with the expected exitCode values 0 and 1.
Use the existing runDoctorJson, findCheck, and makeRuntimeFetch symbols to keep
the new cases consistent with the current suite.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fa9e194-3b2f-44b2-8135-b199544b64f5
⛔ Files ignored due to path filters (2)
packages/backend/test/__snapshots__/openapi.test.ts.snapis excluded by!**/*.snappackages/cli/test/__snapshots__/help.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
.changeset/cli-doctor-diagnostics.mddocs/reference/developer/cli.mdxpackages/backend/src/http-api/groups/status.tspackages/backend/src/routes/status.tspackages/backend/test/e2e/tenant-isolation.test.tspackages/backend/test/routes/status.test.tspackages/cli/README.mdpackages/cli/src/commands/audit-tail.tspackages/cli/src/commands/doctor.tspackages/cli/src/runtime.tspackages/cli/src/types.tspackages/cli/test/e2e/doctor.e2e.test.tspackages/cli/test/help.test.tspackages/cli/test/parity.test.tspackages/internals/persistence/src/index.tspackages/internals/persistence/src/services/persistence.tspackages/internals/persistence/src/services/persistence/migrations.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/backend/src/http-api/groups/status.tspackages/cli/test/help.test.tspackages/internals/persistence/src/index.tspackages/backend/test/e2e/tenant-isolation.test.tspackages/cli/src/types.tspackages/backend/src/routes/status.tspackages/backend/test/routes/status.test.tspackages/internals/persistence/src/services/persistence/migrations.tspackages/cli/test/e2e/doctor.e2e.test.tspackages/cli/src/commands/audit-tail.tspackages/internals/persistence/src/services/persistence.tspackages/cli/src/commands/doctor.tspackages/cli/src/runtime.tspackages/cli/test/parity.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/backend/src/http-api/groups/status.tspackages/cli/test/help.test.tspackages/internals/persistence/src/index.tspackages/backend/test/e2e/tenant-isolation.test.tspackages/cli/src/types.tspackages/backend/src/routes/status.tspackages/backend/test/routes/status.test.tspackages/internals/persistence/src/services/persistence/migrations.tspackages/cli/test/e2e/doctor.e2e.test.tspackages/cli/src/commands/audit-tail.tspackages/internals/persistence/src/services/persistence.tspackages/cli/src/commands/doctor.tspackages/cli/src/runtime.tspackages/cli/test/parity.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/cli/test/help.test.tspackages/backend/test/e2e/tenant-isolation.test.tspackages/backend/test/routes/status.test.tspackages/cli/test/e2e/doctor.e2e.test.tspackages/cli/test/parity.test.ts
**/index.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid barrel files (index files that re-export everything)
Files:
packages/internals/persistence/src/index.ts
🪛 markdownlint-cli2 (0.22.1)
.changeset/cli-doctor-diagnostics.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (15)
packages/cli/src/commands/doctor.ts (2)
222-263: LGTM!Also applies to: 265-335, 337-423, 424-472, 480-509
117-121: 🩺 Stability & AvailabilityCheck the API error message format before relying on these regexes. If HTTP errors no longer include status codes in parentheses, these guards will stop classifying unavailable/forbidden diagnostics responses.
packages/cli/src/runtime.ts (1)
15-81: LGTM!Also applies to: 111-133, 150-184, 242-301
packages/cli/src/commands/audit-tail.ts (1)
230-233: LGTM!packages/cli/test/e2e/doctor.e2e.test.ts (1)
1-163: LGTM!packages/cli/test/help.test.ts (1)
6-14: LGTM!Also applies to: 33-50
packages/cli/README.md (1)
20-20: LGTM!Also applies to: 44-47, 57-61
docs/reference/developer/cli.mdx (1)
19-24: LGTM!Also applies to: 53-77, 107-115
packages/cli/src/types.ts (1)
97-103: LGTM!Also applies to: 114-117
packages/internals/persistence/src/index.ts (1)
2-8: LGTM!Also applies to: 9-12
packages/internals/persistence/src/services/persistence.ts (1)
54-55: LGTM!Also applies to: 72-80, 134-138, 1427-1427
packages/internals/persistence/src/services/persistence/migrations.ts (1)
43-64: LGTM!Also applies to: 98-117, 196-231
packages/backend/test/e2e/tenant-isolation.test.ts (1)
1462-1467: LGTM!packages/cli/test/parity.test.ts (1)
8-8: LGTM!Also applies to: 29-32
packages/backend/src/routes/status.ts (1)
66-69: 🎯 Functional CorrectnessCheck
currentPersistenceMigrationStatus()against the diagnosticsmigrationsschema. If the fallback shape differs, the status route can fail schema validation.
| const health = yield* adapter | ||
| .healthCheck() | ||
| .pipe(Effect.catch(() => Effect.succeed(adapterDown))); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Health check error details are silently lost.
The Effect.catch(() => Effect.succeed(adapterDown)) ignores the error parameter. When healthCheck() fails, the error message is not included in the response, making it harder to diagnose why an adapter is down. The diagnostics catch handler (lines 81-87) does capture errors — the asymmetry is notable.
🔍 Proposed fix to preserve health check error details
const health = yield* adapter
.healthCheck()
- .pipe(Effect.catch(() => Effect.succeed(adapterDown)));
+ .pipe(
+ Effect.catch((error) =>
+ Effect.succeed({
+ details: { healthCheckError: messageFromError(error) },
+ ok: false,
+ status: "down" as const,
+ })
+ )
+ );This way health.details is populated and flows into the response via toDetails.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const health = yield* adapter | |
| .healthCheck() | |
| .pipe(Effect.catch(() => Effect.succeed(adapterDown))); | |
| const health = yield* adapter | |
| .healthCheck() | |
| .pipe( | |
| Effect.catch((error) => | |
| Effect.succeed({ | |
| details: { healthCheckError: messageFromError(error) }, | |
| ok: false, | |
| status: "down" as const, | |
| }) | |
| ) | |
| ); |
🤖 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 `@packages/backend/src/routes/status.ts` around lines 77 - 79, The health check
in status handling drops the failure details because the catch in the adapter
health flow ignores the error. Update the
`adapter.healthCheck().pipe(Effect.catch(...))` path in `status.ts` so the
handler captures the error and returns a down result with `details` populated,
matching the diagnostics catch behavior and allowing `toDetails` to surface the
message in the response.
|
hey @KayleeWilliams, are you working on this currently?? let me know if any fix is needed! |
Summary of Diagnostics & Test Coverage FixesI've resolved the outstanding review issues regarding the
|
There was a problem hiding this comment.
Important
The backend now reports persistence outages gracefully (persistence.reachable: false + error), but the CLI half of the same commit can't parse that shape — a real outage surfaces as a generic "did not match expected shape" error instead of the backend's actual message. One inline comment.
Reviewed changes — the single commit a917ea2 since the prior pullfrog review (378240b), which reworks the backend diagnostics schema to represent persistence failure, hardens the endpoint's error handling, and adds test coverage (addresses CodeRabbit's round-7 findings).
- Made
persistencefailure representable —packages/backend/src/http-api/groups/status.tschanges the diagnosticspersistenceschema fromSchema.Literal(true)toSchema.Booleanplus an optionalerrorstring. - Graceful persistence-failure handling —
packages/backend/src/routes/status.tswraps the migration-status effect inEffect.catch, returning empty migrations +persistence.reachable: falsewith the error message while keeping the endpoint at200;adapterDownnow carriesdetails: undefinedand the diagnostics-failure fallback setsversion: undefined. - Test coverage — backend
status.test.tsgains persistence-failure and adapter-failure cases; CLIdoctor.e2e.test.tsadds degraded-adapter (warn, exit0) and down-adapter (fail, exit1) fixtures; OpenAPI snapshot updated to the newpersistenceshape. - Unrelated type churn —
packages/internals/persistence/src/services/persistence/webhook-secret-encryption.tsaddsas Uint8Arraywidening casts aroundcryptocalls (runtime no-op,Bufferis aUint8Array), likely a typecheck fix pulled in with this commit.
⚠️ Persistence-outage details are discarded by the CLI
The backend change in this commit makes a persistence outage a first-class, well-formed 200 response — empty migrations, persistence.reachable: false, and a human-readable error. The CLI never gets to use any of it.
Because the CLI's diagnostics type and guard still require persistence.reachable === true, the whole payload is rejected as malformed:
- the migrations check fails with
Diagnostics response did not match expected shape.instead of the backend's real error (e.g.Database connection failed) - the adapters check fails the same way, even though the adapter statuses in the payload are valid
The exit code is still 1, so this isn't a correctness break, but the operator sees a message that reads like a CLI/version mismatch rather than a database outage — the exact scenario the backend change was written to surface. No CLI test exercises the reachable: false path.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/backend/src/routes/status.ts (1)
14-14: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHealth check error details are still silently lost.
adapterDownnow explicitly setsdetails: undefined, and the health check catch at line 99 (Effect.catch(() => Effect.succeed(adapterDown))) still ignores the error parameter. WhenhealthCheck()fails butdiagnostics()succeeds, the adapter is reported as"down"with no explanation. The diagnostics catch (lines 100-108) correctly captures errors — the asymmetry persists.🔍 Proposed fix to preserve health check error details
const health = yield* adapter .healthCheck() - .pipe(Effect.catch(() => Effect.succeed(adapterDown))); + .pipe( + Effect.catch((error) => + Effect.succeed({ + ...adapterDown, + details: { healthCheckError: messageFromError(error) }, + }) + ) + );This flows through
toDetailsat line 115 ashealth.details→details.healthin the response.🤖 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 `@packages/backend/src/routes/status.ts` at line 14, The health-check failure path in status.ts is still dropping useful error details: `adapterDown` is initialized with `details: undefined`, and the `Effect.catch` around `healthCheck()` ignores the error value before returning `adapterDown`. Update the `healthCheck()` catch branch to capture the thrown error and populate `details` consistently with the existing diagnostics error handling, so `toDetails` can surface it through `health.details` in the response.
🤖 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.
Duplicate comments:
In `@packages/backend/src/routes/status.ts`:
- Line 14: The health-check failure path in status.ts is still dropping useful
error details: `adapterDown` is initialized with `details: undefined`, and the
`Effect.catch` around `healthCheck()` ignores the error value before returning
`adapterDown`. Update the `healthCheck()` catch branch to capture the thrown
error and populate `details` consistently with the existing diagnostics error
handling, so `toDetails` can surface it through `health.details` in the
response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b080b12-73b0-4056-bad9-ee7c357f1e07
⛔ Files ignored due to path filters (1)
packages/backend/test/__snapshots__/openapi.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
packages/backend/src/http-api/groups/status.tspackages/backend/src/routes/status.tspackages/backend/test/routes/status.test.tspackages/cli/src/commands/doctor.tspackages/cli/test/e2e/doctor.e2e.test.tspackages/internals/persistence/src/services/persistence/webhook-secret-encryption.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/backend/test/routes/status.test.tspackages/cli/test/e2e/doctor.e2e.test.tspackages/backend/src/http-api/groups/status.tspackages/backend/src/routes/status.tspackages/cli/src/commands/doctor.tspackages/internals/persistence/src/services/persistence/webhook-secret-encryption.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/backend/test/routes/status.test.tspackages/cli/test/e2e/doctor.e2e.test.tspackages/backend/src/http-api/groups/status.tspackages/backend/src/routes/status.tspackages/cli/src/commands/doctor.tspackages/internals/persistence/src/services/persistence/webhook-secret-encryption.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/backend/test/routes/status.test.tspackages/cli/test/e2e/doctor.e2e.test.ts
🔇 Additional comments (10)
packages/backend/src/http-api/groups/status.ts (1)
54-55: LGTM!packages/backend/src/routes/status.ts (1)
67-89: LGTM!packages/backend/test/routes/status.test.ts (1)
49-150: LGTM!packages/cli/src/commands/doctor.ts (1)
117-119: LGTM!packages/cli/test/e2e/doctor.e2e.test.ts (3)
7-131: LGTM!
165-231: LGTM!
309-374: LGTM!packages/internals/persistence/src/services/persistence/webhook-secret-encryption.ts (3)
116-118: LGTM!
138-147: LGTM!
160-170: LGTM!
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the single commit 53d4dca since the prior pullfrog review (a917ea2), which closes the round-7 open risk where the CLI could not parse the backend's persistence-outage payload, and captures adapter health-check error details on the backend.
- CLI consumes
persistence.reachable: false—packages/cli/src/commands/doctor.tswidensDiagnosticsData.persistenceto{ error?: string; reachable: boolean }, relaxesisDiagnosticsDatato accept a booleanreachablewith optional-stringerror, and hascheckMigrationsshort-circuit onreachable === falsetofailwith the backend's actual error message instead of the generic shape-mismatch error. - Backend captures health-check errors —
packages/backend/src/routes/status.tsreplaces the staticadapterDownconstant (and itsAdapterHealthimport) with anEffect.catchthat recordsdetails: { error }onhealthCheck()failure, so the real error flows throughtoDetailsintodetails.health. - Test coverage — CLI
doctor.e2e.test.tsadds amakeOutagePersistenceFetchfixture asserting exit1,persistence.migrationsfailing withPersistence outage: Database connection failed., andadapters.healthstill passing; backendstatus.test.tsasserts the captureddetails.healtherror.
The round-7 finding is addressed exactly as suggested — the type, guard, and checkMigrations branch line up with the backend's 200-with-reachable: false contract, and the new e2e fixture locks in the behaviour. The backend and CLI halves of the outage path are now coherent.
Claude Opus ｜ 𝕏
2 participants
Linked Issue
Closes #13.
Implemented:
dsar doctorconfig/runtime/auth/persistence reachability checks--output jsonretained for automationCompleted in follow-up commits on this PR (maintainer update):
GET /status/diagnosticsbackend endpoint exposing schema migration freshness and per-adapter healthdsar doctornow fails on stale migrations or down adapters, warns on degraded adapters, and keeps graceful warn/skip behavior against older backendsdsar: minor)Description
Adds an initial
dsar doctorcommand for CLI/runtime diagnostics and expands CLI help coverage.This includes:
/statusreachability checkdsar <command> --helpdsar requests --helpWith the diagnostics endpoint added in the follow-up commits, this PR now fully closes #13.
##Screenshots for validation
Validation