feat: cli doctor command#363
Open
calvinbrewer wants to merge 1 commit intomainfrom
Open
Conversation
🦋 Changeset detectedLatest commit: 07d8f79 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a comprehensive Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as stash doctor
participant Registry as Check Registry
participant Runner as Check Runner
participant Cache as DoctorCache
participant Checks as Individual Checks
participant Report as Report Renderer
participant Output as Human/JSON Output
User->>CLI: stash doctor [--json] [--only category] [--skip-db]
CLI->>CLI: Parse flags & cwd
CLI->>Cache: buildCache(cwd)
Cache->>Cache: Memoize: package.json, stashConfig, encryptClient, token, integration, hasTypeScript
CLI->>Registry: Load CHECKS array
CLI->>Runner: runChecks(CHECKS, context)
loop For each check in order
Runner->>Checks: Check.run({cache, flags, cwd})
alt Check has unmet dependency
Checks-->>Runner: {status: 'skip', reason: 'dependency failed'}
else Check runs
Checks->>Cache: Call memoized getters as needed
Cache-->>Checks: Return cached/computed results
Checks-->>Runner: {status: 'pass'|'fail'|'skip', message, fixHint?, details?, cause?}
end
Runner->>Runner: Catch exceptions → fail result
Runner->>Runner: Record outcome + update summary
end
Runner-->>CLI: Report {cliVersion, timestamp, summary, outcomes}
CLI->>Report: Select formatter (json/human)
Report->>Output: Render report
Output-->>User: Display results
CLI->>CLI: Compute exit code (0/1/2) from report
CLI-->>User: Exit with code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 10
🧹 Nitpick comments (14)
packages/cli/src/commands/doctor/__tests__/checks/env-database-url.test.ts (1)
1-3: Addimport 'dotenv/config'at the top.This test reads and mutates
process.env.DATABASE_URL. The save/restore pattern is correct, but the repo convention is to load dotenv in env-touching test files so behaviour is consistent across local/CI environments.♻️ Proposed change
+import 'dotenv/config' import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { envDatabaseUrl } from '../../checks/env/database-url.js' import type { CheckContext } from '../../types.js'As per coding guidelines: "Import dotenv/config at the top of test files that need environment variables".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/__tests__/checks/env-database-url.test.ts` around lines 1 - 3, Add "import 'dotenv/config'" as the first import in the test file so dotenv is loaded before any test code runs; update the tests in the env-database-url.test file (which import envDatabaseUrl and use process.env.DATABASE_URL) to include this top-level import so environment variables behave consistently across local/CI, keeping the existing save/restore pattern intact.packages/cli/src/commands/doctor/__tests__/checks/project-node-version.test.ts (1)
27-33: Consider adding a fail-path test by stubbingprocess.versions.The current test only covers the happy path implicitly via the runtime engine. A second case that temporarily overrides
process.versions.node(e.g., viavi.stubGlobalor property descriptor) would lock in the< 22failure branch and the associatedfixHint/message. Optional, but cheap insurance.♻️ Sketch
+ it('fails when Node major < 22', async () => { + const original = process.versions.node + Object.defineProperty(process.versions, 'node', { value: '20.0.0', configurable: true }) + try { + const result = await projectNodeVersion.run(ctx) + expect(result.status).toBe('fail') + } finally { + Object.defineProperty(process.versions, 'node', { value: original, configurable: true }) + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/__tests__/checks/project-node-version.test.ts` around lines 27 - 33, Add a failing-path unit test that stubs process.versions.node to a pre-22 value so the projectNodeVersion.run(ctx) executes the failure branch; for example, in the same describe block use vi.stubGlobal or Object.defineProperty to temporarily set process.versions.node = '18.0.0' (and restore after) then await projectNodeVersion.run(ctx) and assert result.status === 'fail' and that result.message and/or result.fixHint contain the expected failure text. Locate this change around the existing test that calls projectNodeVersion.run to mirror the happy-path and ensure the stub is cleaned up after the spec.packages/cli/src/bin/stash.ts (1)
99-99: Minor:--onlyhelp text wording is slightly inconsistent."Only run one category" reads as singular, but the parser accepts a comma-separated list. Consider:
Proposed wording
- --only <category> Only run one category: project, config, auth, env, database, integration (comma-separated) + --only <categories> Restrict to one or more categories (comma-separated): project, config, auth, env, database, integration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/bin/stash.ts` at line 99, Update the help text for the --only flag to reflect that multiple categories may be provided (comma-separated) rather than a single one; change the description that currently reads "Only run one category: project, config, auth, env, database, integration (comma-separated)" to wording such as "Only run specified categories (comma-separated): project, config, auth, env, database, integration" so the flag name --only and its accepted input format are consistent.packages/cli/src/commands/doctor/checks/project/package-json.ts (1)
20-30: Optional: distinguish read errors from parse errors.
readFileSyncerrors (e.g.EACCES,EISDIR) currently surface as"package.json is not valid JSON", which is misleading. The original error survives incause, but the user-visiblemessage/fixHintwould point in the wrong direction. Consider splitting:Proposed refactor
- try { - JSON.parse(readFileSync(pkgPath, 'utf-8')) - } catch (cause) { - return { - status: 'fail', - message: 'package.json is not valid JSON', - fixHint: 'Fix the JSON syntax errors in package.json.', - details: { path: pkgPath }, - cause, - } - } + let raw: string + try { + raw = readFileSync(pkgPath, 'utf-8') + } catch (cause) { + return { + status: 'fail', + message: 'package.json could not be read', + fixHint: 'Check file permissions on package.json.', + details: { path: pkgPath }, + cause, + } + } + try { + JSON.parse(raw) + } catch (cause) { + return { + status: 'fail', + message: 'package.json is not valid JSON', + fixHint: 'Fix the JSON syntax errors in package.json.', + details: { path: pkgPath }, + cause, + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/checks/project/package-json.ts` around lines 20 - 30, The current catch lumps readFileSync and JSON.parse failures together; change the logic to first call readFileSync(pkgPath, 'utf-8') in its own try/catch and return a read-specific failure (use pkgPath and the caught cause) when reading fails (EACCES, EISDIR, ENOENT), then separately try JSON.parse(...) and on parse errors return a parse-specific failure message and fixHint; preserve the original caught error in the details.cause in both cases so callers can inspect the underlying error.packages/cli/src/commands/doctor/format/json.ts (1)
7-27: Add a comment documenting the contract thatdetailsmust never contain secrets.
renderJsoncorrectly drops thecausefield (which can carry stack traces and paths), but passesresult.detailsthrough verbatim. While all current checks appropriately include only non-sensitive data indetails(paths, version numbers, workspace IDs as identifiers, permission flags, counts), a brief comment in this file would help future check authors maintain this contract. Note thatDATABASE_URLand credentials are correctly omitted fromdetails—connection errors are attached tocause(whichrenderJsonstrips), notdetails.As per coding guidelines: "Do NOT log plaintext; the library never logs plaintext by design and logs should never leak sensitive data".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/format/json.ts` around lines 7 - 27, Add a short comment above the renderJson function documenting the contract that result.details must never contain secrets or plaintext credentials (e.g., DATABASE_URL, passwords, tokens) and should only include non-sensitive diagnostic values (paths, versions, IDs, permission flags, counts); note that renderJson intentionally strips result.cause but will serialize result.details verbatim so check authors must sanitize sensitive data before setting result.details.packages/cli/src/commands/doctor/checks/database/eql-installed.ts (1)
13-13: Optional: include amessagefor the skip path for consistency.Line 11 returns
{ status: 'skip', message: '--skip-db' }, but this branch returns{ status: 'skip' }with no message. A short reason (e.g.'no stash config') would help when results are rendered, especially in JSON output where a bareskipis opaque. Thedatabase.connectsdependency likely already short-circuits this path, but defensive consistency is cheap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/checks/database/eql-installed.ts` at line 13, The skip branch in eql-installed.ts currently returns { status: 'skip' } with no message; update the branch that checks result.ok (the early return when result.ok is false) to return a consistent object including a message (e.g. { status: 'skip', message: 'no stash config' } or similar) so that callers rendering database.check results (the same shape returned by this module) always have a descriptive message for skip statuses; locate the check that reads "if (!result.ok) return { status: 'skip' }" and change it to include the brief message.packages/cli/src/commands/doctor/__tests__/checks/config-encryption-client-loadable.test.ts (1)
5-27: Consider extractingctxWithinto a shared test helper.The
ctxWithfactory is duplicated across at leastconfig-encryption-client-loadable.test.tsandconfig-stash-config-present.test.ts(and likely other check tests). When new fields are added toCheckContext/CheckCache, every copy must be updated in lockstep. A small__tests__/helpers/ctx.tswith a configurable factory (e.g., taking partial cache overrides) would reduce drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/__tests__/checks/config-encryption-client-loadable.test.ts` around lines 5 - 27, Extract the duplicated ctxWith factory into a shared test helper (e.g., export function ctxWith from a new __tests__/helpers/ctx.ts) that returns a CheckContext and accepts optional overrides for the cache and top-level fields (use a Partial<CheckContext> and Partial<CheckCache> pattern) so tests can pass only the fields they need; then replace local ctxWith copies in tests like config-encryption-client-loadable.test.ts and config-stash-config-present.test.ts with imports of the shared ctxWith and call it with specific overrides (e.g., encryptClient override) to preserve existing behavior while avoiding duplicated factories.packages/cli/src/commands/doctor/checks/env/dotenv-files.ts (1)
5-10: Optional: consider broader.envcandidate coverage.The candidate list covers the common dev variants but omits
.env.production,.env.test, and.env.production.local. Since this check isinfoseverity and the fix hint already disclaims it for shell/CI setups, expanding the list would only marginally improve detection — flagging as a nit in case prod-deployed projects also runstash doctor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/checks/env/dotenv-files.ts` around lines 5 - 10, Update the CANDIDATES constant in dotenv-files.ts to include additional common environment file names so the check covers production and test variants: add '.env.production', '.env.production.local', '.env.test', and optionally '.env.test.local' to the existing CANDIDATES array; keep the constant name CANDIDATES and the rest of the logic unchanged since this is an info-level check.packages/cli/src/commands/doctor/checks/config/encryption-client-loadable.ts (1)
9-35: Consider exhaustive matching onresult.reason.The code handles all four failure reasons (
no-config,file-missing,import-failed,no-export) and the success case, but the trailing return block (lines 27–33) acts as a catch-all that assumes any remaining case is a missing export. If a new failure reason is added to theEncryptClientLoadResultunion type later, it would silently render as a "missing export" message with a misleadingfixHint. Using a discriminatedswitchstatement with exhaustive case coverage would surface such gaps at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/checks/config/encryption-client-loadable.ts` around lines 9 - 35, The if/else chain in the run function is treating unknown failure reasons as "no-export"; refactor run (in encryption-client-loadable.ts) to use a discriminated switch on result.reason (handle 'no-config', 'file-missing', 'import-failed', and 'no-export' explicitly) and add a default branch that enforces exhaustiveness (assign to a never-typed variable or throw an Error) so the compiler will surface any newly added EncryptClientLoadResult reasons instead of silently showing the wrong fixHint; preserve the existing success path that checks result.ok and the current return payloads for each handled case.packages/cli/src/commands/doctor/index.ts (1)
31-36: Consider gating--fixat flag-parse time.
parseDoctorFlagshappily acceptsfix, andrunDoctorthen errors and exits 1. Because--fixisn't implemented, you could either reject it during parsing (with a clear message that includes the suggested workaround) or omit it from the parsed shape entirely until it lands. Current behavior is fine but the dead path is easy to forget once a real implementation is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/index.ts` around lines 31 - 36, The doctor command currently accepts --fix in parseDoctorFlags but then bails in runDoctor when flags.fix is truthy; update parseDoctorFlags to either (a) reject the --fix flag at parse time by validating and throwing/returning a clear error message that includes the suggested manual command, or (b) remove/omit the fix property from the parsed flags shape so callers never see flags.fix; locate parseDoctorFlags and the flags definition and implement the chosen approach so runDoctor no longer needs the early exit for flags.fix.packages/cli/src/commands/doctor/format/human.ts (1)
71-96: Minor: simplify the title-emit branching.The
if (!verbose && result.status === 'pass') { push(titleLine); continue }followed by an unconditionallines.push(titleLine(outcome))reads as duplicated logic — the title is always pushed; only the detail trailer is conditional. Flattening reduces the chance of future divergence.♻️ Suggested cleanup
for (const outcome of outcomes) { const { check, result } = outcome - const showDetail = - result.status === 'fail' || - (verbose && (result.status !== 'pass' || check.severity !== 'info')) - - if (!verbose && result.status === 'pass') { - lines.push(titleLine(outcome)) - continue - } lines.push(titleLine(outcome)) + const showDetail = + result.status === 'fail' || + (verbose && (result.status !== 'pass' || check.severity !== 'info')) + if (!showDetail) continue if (result.fixHint && showDetail) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/format/human.ts` around lines 71 - 96, The titleLine is being pushed twice due to an early continue followed by an unconditional push; simplify by emitting titleLine(outcome) exactly once at the top of the loop and then conditionally emitting the detail blocks: compute showDetail as you already do (using result.status, verbose, and check.severity), then after lines.push(titleLine(outcome)) skip any detail output when (!verbose && result.status === 'pass') or simply guard each detail block (fixHint and cause) with showDetail/verbose checks; remove the early continue and the duplicate push to flatten the branching.packages/cli/src/commands/doctor/checks/database/eql-version.ts (1)
11-20: Bundled-version regex is tightly coupled to the SQL artifact's exact text.
/SELECT\s+'(eql-[^']+)'/iwill silently start returningundefinedifeql_v2.version()is ever rewritten (e.g. wrapped, multi-line return, computed string). The check already degrades gracefully (passwith “bundled version not detectable”), but that means a real version-drift bug would be invisible.Consider either (a) pinning the bundled version in a small generated constant alongside the SQL, or (b) parsing more defensively and emitting a
warnif the bundled SQL is found but no version can be extracted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/checks/database/eql-version.ts` around lines 11 - 20, The regex in bundledEqlVersion() that extracts the bundled eql version is fragile and may silently return undefined if the SQL artifact text changes; update bundledEqlVersion() (which calls loadBundledEqlSql()) to detect when loadBundledEqlSql() returns content but the regex fails and then emit a visible warning (instead of silently returning undefined) indicating the bundled SQL was loaded but its version could not be parsed; alternatively add a small generated constant near the SQL artifact that pins the expected version and fall back to that constant if the regex cannot parse it (reference bundledEqlVersion, loadBundledEqlSql and the eql_v2.version() callsite so reviewers can locate the change).packages/cli/src/commands/doctor/context.ts (1)
57-60: Optional: preserve the underlying stash-config failure reason.
tryLoadStashConfigdistinguishesnot-found,import-failed, andinvalid(perconfig/index.ts), but they all collapse intoreason: 'no-config'here. Individual checks recover the detail by callingcache.stashConfig()directly, so this isn't a bug — but the'no-config'discriminant is misleading when the file actually exists and just failed to import or validate. Consider either renaming to'config-unavailable'or forwarding the underlying reason.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/context.ts` around lines 57 - 60, The current doctor context collapses all tryLoadStashConfig failures into { ok: false, reason: 'no-config' } which loses the specific failure type; update the return to preserve or surface the underlying failure by inspecting configResult (from tryLoadStashConfig) and returning its specific reason (e.g., 'not-found' | 'import-failed' | 'invalid') or change the discriminant to a more accurate token like 'config-unavailable' and include configResult.reason as a detail; adjust any callers that expect 'no-config' accordingly (notably places that call cache.stashConfig()) so they can handle the specific reasons.packages/cli/src/commands/doctor/runner.ts (1)
26-41: Optional: distinguish missing-dep from failed-dep.Both branches return the same
depId, so a typo independsOn(or a filtered-out dep when running with--only) produces a skip message that reads identically to a real dependency failure. Differentiating these helps debug check-registration mistakes without changing user behavior on the happy path.♻️ Possible refactor
function dependencyFailure( check: Check, byId: ReadonlyMap<string, CheckResult>, -): string | undefined { +): { depId: string; reason: 'missing' | 'not-pass' } | undefined { if (!check.dependsOn) return undefined for (const depId of check.dependsOn) { const depResult = byId.get(depId) - if (!depResult) { - return depId - } - if (depResult.status !== 'pass') { - return depId - } + if (!depResult) return { depId, reason: 'missing' } + if (depResult.status !== 'pass') return { depId, reason: 'not-pass' } } return undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/doctor/runner.ts` around lines 26 - 41, In dependencyFailure, distinguish a missing dependency from a failing dependency so callers can render different messages: change the function (dependencyFailure) to return a discriminated result (e.g., an object like { id: string, kind: 'missing' | 'failed' } or a prefixed string such as "missing:<id>" vs "failed:<id>") instead of just the depId, update callers that consume dependencyFailure to handle the two cases (render different skip vs dependency-failed messages), and ensure the function still returns undefined for no dependency issues; specifically adjust the branches where byId.get(depId) is falsy to return the "missing" variant and where depResult.status !== 'pass' to return the "failed" variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/doctor/__tests__/checks/database-connects.test.ts`:
- Around line 1-4: This test file is missing the dotenv loader so DATABASE_URL
from a .env isn't available; add import 'dotenv/config' as the very first import
in packages/cli/src/commands/doctor/__tests__/checks/database-connects.test.ts
(before the current imports of vitest and databaseConnects) so the
it.skipIf(!process.env.DATABASE_URL)(...) gate works correctly and real-DB cases
aren't silently skipped when env vars come from a .env file.
- Around line 52-57: The unreachable-DB test can hang because DNS may resolve
"nope"; update the test to target a guaranteed-unroutable address (e.g. use
ctxFor('postgres://10.255.255.1:5432/postgres') or another RFC1918 non-routable
IP) and enforce a short timeout for the test (use Vitest's per-test timeout or
set a short connect timeout when invoking databaseConnects.run). Additionally,
add a short connectionTimeoutMillis in the real implementation
(checks/database/connects.ts where the pg.Client is constructed) so
databaseConnects uses that setting by default and the doctor check won't stall
users.
In
`@packages/cli/src/commands/doctor/__tests__/checks/database-eql-installed.test.ts`:
- Around line 1-3: The test relies on process.env.DATABASE_URL (used in the
integration assertion around lines 43–51) but doesn't load environment files;
add the dotenv/config import at the top of the test file so developer .env
values are loaded before the test runs—i.e., insert an import 'dotenv/config' as
the first statement in
packages/cli/src/commands/doctor/__tests__/checks/database-eql-installed.test.ts
above the existing imports referencing databaseEqlInstalled and CheckContext.
In `@packages/cli/src/commands/doctor/checks/auth/authenticated.ts`:
- Around line 23-31: The fixHint currently hardcodes "Run: stash auth login"
even for transport/unknown errors; update the object returned in
authenticated.ts so fixHint is conditional on isNotLoggedIn (only suggest "Run:
stash auth login" when isNotLoggedIn is true) and for other cases omit the hint
or provide a generic suggestion (e.g., check network/inspect result.cause) so
that non-login failures (identified by code and probeCredentials behavior)
aren’t misdirected; change the logic that builds the return object where
isNotLoggedIn, code, and result.cause are used to set message/fixHint.
In `@packages/cli/src/commands/doctor/checks/auth/workspace-id-matches-config.ts`:
- Around line 16-21: The early return in workspace-id-matches-config.ts
currently returns { status: 'pass', message: 'CS_WORKSPACE_CRN not set —
skipping' } when crn is unset; change the status to 'skip' so the check is
recorded as skipped (i.e., return { status: 'skip', message: ... }). Locate the
early-return/guard in the function handling the workspace ID check (the block
that begins with if (!crn)) and modify only the status string from 'pass' to
'skip' while keeping the existing message and structure.
In `@packages/cli/src/commands/doctor/context.ts`:
- Around line 90-103: The call to client.getEncryptConfig() can throw and
currently escapes the structured result contract; update the
EncryptClientLoadResult type in types.ts to add a new failure variant (e.g., {
ok: false, reason: 'getEncryptConfigFailed', error: unknown }) and then wrap the
getEncryptConfig() invocation inside a try/catch in the loadEncryptClient
implementation so that any thrown error is caught and you return the new
structured failure instead of letting the exception propagate into memoAsync;
ensure cache.encryptClient() continues to return the structured
EncryptClientLoadResult so runCheck receives an actionable failure.
- Around line 113-133: The JSON formatter currently emits check result details
verbatim (renderJson), which causes sensitive token claims (e.g., workspaceId,
subject, token, services) added by auth checks (auth-authenticated,
workspace-id-matches-config) to be printed; update the JSON rendering pipeline
to sanitize details before output by stripping or redacting those fields (at
minimum workspaceId and subject, plus any nested token/services) in renderJson()
so the JSON output never contains plaintext token claims, or alternatively
perform the same sanitization when the auth checks build their details objects
(auth-authenticated.ts / workspace-id-matches-config.ts) to ensure details do
not include raw token claims. Ensure the sanitizer targets keys "workspaceId",
"subject", "token", and "services" and replaces values with a fixed redaction
marker (e.g., "[REDACTED]") or omits them entirely.
In `@packages/cli/src/commands/doctor/index.ts`:
- Around line 73-82: The current parsing of values.only into rawOnly and only
silently discards invalid tokens (rawOnly, only, isCategory) so an empty only
array causes the program to run all checks contrary to intent; update the logic
after building only to detect when values.only was provided but only.length ===
0 and then either (a) print an error/warning listing allowed categories (derive
allowed names from CheckCategory union or isCategory's accepted set) and exit
with non-zero, or (b) return a CLI error informing the user their tokens were
invalid and showing the list of valid categories, ensuring you reference
values.only, rawOnly and isCategory in the message so users can correct typos.
In `@packages/cli/src/commands/doctor/types.ts`:
- Around line 65-71: The JSDoc for the tableCount property is stale: it says the
value can be `undefined` when there are no tables, but the type is `number` and
the loader (`getEncryptConfig()` path in context.ts) returns 0 for no tables.
Update the JSDoc on the tableCount field to state that it is 0 when no tables
are present (keep the type as `number`), and confirm no other code expects
`undefined` for tableCount; if any code does, convert those checks to compare
against 0 instead.
In `@packages/cli/src/commands/wizard/agent/interface.ts`:
- Around line 484-501: The current debug block in the wizard agent (the
session.debug conditional that calls p.log.warn with the serialized message) may
log sensitive plaintext from fields like error_details, result, and
last_assistant_message; update this block to sanitize or omit sensitive content
before logging by either redacting those fields (e.g., replace values with
"<REDACTED>" or only include lengths), or log only safe metadata (subtype,
is_error, stop_reason) and optionally a boolean/size indicator for
result/error_details; ensure you modify the code that builds the object passed
to p.log.warn (the JSON.stringify payload) so it never includes raw
error_details, result, or last_assistant_message values even when session.debug
is true.
---
Nitpick comments:
In `@packages/cli/src/bin/stash.ts`:
- Line 99: Update the help text for the --only flag to reflect that multiple
categories may be provided (comma-separated) rather than a single one; change
the description that currently reads "Only run one category: project, config,
auth, env, database, integration (comma-separated)" to wording such as "Only run
specified categories (comma-separated): project, config, auth, env, database,
integration" so the flag name --only and its accepted input format are
consistent.
In
`@packages/cli/src/commands/doctor/__tests__/checks/config-encryption-client-loadable.test.ts`:
- Around line 5-27: Extract the duplicated ctxWith factory into a shared test
helper (e.g., export function ctxWith from a new __tests__/helpers/ctx.ts) that
returns a CheckContext and accepts optional overrides for the cache and
top-level fields (use a Partial<CheckContext> and Partial<CheckCache> pattern)
so tests can pass only the fields they need; then replace local ctxWith copies
in tests like config-encryption-client-loadable.test.ts and
config-stash-config-present.test.ts with imports of the shared ctxWith and call
it with specific overrides (e.g., encryptClient override) to preserve existing
behavior while avoiding duplicated factories.
In `@packages/cli/src/commands/doctor/__tests__/checks/env-database-url.test.ts`:
- Around line 1-3: Add "import 'dotenv/config'" as the first import in the test
file so dotenv is loaded before any test code runs; update the tests in the
env-database-url.test file (which import envDatabaseUrl and use
process.env.DATABASE_URL) to include this top-level import so environment
variables behave consistently across local/CI, keeping the existing save/restore
pattern intact.
In
`@packages/cli/src/commands/doctor/__tests__/checks/project-node-version.test.ts`:
- Around line 27-33: Add a failing-path unit test that stubs
process.versions.node to a pre-22 value so the projectNodeVersion.run(ctx)
executes the failure branch; for example, in the same describe block use
vi.stubGlobal or Object.defineProperty to temporarily set process.versions.node
= '18.0.0' (and restore after) then await projectNodeVersion.run(ctx) and assert
result.status === 'fail' and that result.message and/or result.fixHint contain
the expected failure text. Locate this change around the existing test that
calls projectNodeVersion.run to mirror the happy-path and ensure the stub is
cleaned up after the spec.
In
`@packages/cli/src/commands/doctor/checks/config/encryption-client-loadable.ts`:
- Around line 9-35: The if/else chain in the run function is treating unknown
failure reasons as "no-export"; refactor run (in encryption-client-loadable.ts)
to use a discriminated switch on result.reason (handle 'no-config',
'file-missing', 'import-failed', and 'no-export' explicitly) and add a default
branch that enforces exhaustiveness (assign to a never-typed variable or throw
an Error) so the compiler will surface any newly added EncryptClientLoadResult
reasons instead of silently showing the wrong fixHint; preserve the existing
success path that checks result.ok and the current return payloads for each
handled case.
In `@packages/cli/src/commands/doctor/checks/database/eql-installed.ts`:
- Line 13: The skip branch in eql-installed.ts currently returns { status:
'skip' } with no message; update the branch that checks result.ok (the early
return when result.ok is false) to return a consistent object including a
message (e.g. { status: 'skip', message: 'no stash config' } or similar) so that
callers rendering database.check results (the same shape returned by this
module) always have a descriptive message for skip statuses; locate the check
that reads "if (!result.ok) return { status: 'skip' }" and change it to include
the brief message.
In `@packages/cli/src/commands/doctor/checks/database/eql-version.ts`:
- Around line 11-20: The regex in bundledEqlVersion() that extracts the bundled
eql version is fragile and may silently return undefined if the SQL artifact
text changes; update bundledEqlVersion() (which calls loadBundledEqlSql()) to
detect when loadBundledEqlSql() returns content but the regex fails and then
emit a visible warning (instead of silently returning undefined) indicating the
bundled SQL was loaded but its version could not be parsed; alternatively add a
small generated constant near the SQL artifact that pins the expected version
and fall back to that constant if the regex cannot parse it (reference
bundledEqlVersion, loadBundledEqlSql and the eql_v2.version() callsite so
reviewers can locate the change).
In `@packages/cli/src/commands/doctor/checks/env/dotenv-files.ts`:
- Around line 5-10: Update the CANDIDATES constant in dotenv-files.ts to include
additional common environment file names so the check covers production and test
variants: add '.env.production', '.env.production.local', '.env.test', and
optionally '.env.test.local' to the existing CANDIDATES array; keep the constant
name CANDIDATES and the rest of the logic unchanged since this is an info-level
check.
In `@packages/cli/src/commands/doctor/checks/project/package-json.ts`:
- Around line 20-30: The current catch lumps readFileSync and JSON.parse
failures together; change the logic to first call readFileSync(pkgPath, 'utf-8')
in its own try/catch and return a read-specific failure (use pkgPath and the
caught cause) when reading fails (EACCES, EISDIR, ENOENT), then separately try
JSON.parse(...) and on parse errors return a parse-specific failure message and
fixHint; preserve the original caught error in the details.cause in both cases
so callers can inspect the underlying error.
In `@packages/cli/src/commands/doctor/context.ts`:
- Around line 57-60: The current doctor context collapses all tryLoadStashConfig
failures into { ok: false, reason: 'no-config' } which loses the specific
failure type; update the return to preserve or surface the underlying failure by
inspecting configResult (from tryLoadStashConfig) and returning its specific
reason (e.g., 'not-found' | 'import-failed' | 'invalid') or change the
discriminant to a more accurate token like 'config-unavailable' and include
configResult.reason as a detail; adjust any callers that expect 'no-config'
accordingly (notably places that call cache.stashConfig()) so they can handle
the specific reasons.
In `@packages/cli/src/commands/doctor/format/human.ts`:
- Around line 71-96: The titleLine is being pushed twice due to an early
continue followed by an unconditional push; simplify by emitting
titleLine(outcome) exactly once at the top of the loop and then conditionally
emitting the detail blocks: compute showDetail as you already do (using
result.status, verbose, and check.severity), then after
lines.push(titleLine(outcome)) skip any detail output when (!verbose &&
result.status === 'pass') or simply guard each detail block (fixHint and cause)
with showDetail/verbose checks; remove the early continue and the duplicate push
to flatten the branching.
In `@packages/cli/src/commands/doctor/format/json.ts`:
- Around line 7-27: Add a short comment above the renderJson function
documenting the contract that result.details must never contain secrets or
plaintext credentials (e.g., DATABASE_URL, passwords, tokens) and should only
include non-sensitive diagnostic values (paths, versions, IDs, permission flags,
counts); note that renderJson intentionally strips result.cause but will
serialize result.details verbatim so check authors must sanitize sensitive data
before setting result.details.
In `@packages/cli/src/commands/doctor/index.ts`:
- Around line 31-36: The doctor command currently accepts --fix in
parseDoctorFlags but then bails in runDoctor when flags.fix is truthy; update
parseDoctorFlags to either (a) reject the --fix flag at parse time by validating
and throwing/returning a clear error message that includes the suggested manual
command, or (b) remove/omit the fix property from the parsed flags shape so
callers never see flags.fix; locate parseDoctorFlags and the flags definition
and implement the chosen approach so runDoctor no longer needs the early exit
for flags.fix.
In `@packages/cli/src/commands/doctor/runner.ts`:
- Around line 26-41: In dependencyFailure, distinguish a missing dependency from
a failing dependency so callers can render different messages: change the
function (dependencyFailure) to return a discriminated result (e.g., an object
like { id: string, kind: 'missing' | 'failed' } or a prefixed string such as
"missing:<id>" vs "failed:<id>") instead of just the depId, update callers that
consume dependencyFailure to handle the two cases (render different skip vs
dependency-failed messages), and ensure the function still returns undefined for
no dependency issues; specifically adjust the branches where byId.get(depId) is
falsy to return the "missing" variant and where depResult.status !== 'pass' to
return the "failed" variant.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bebecf3-1a1a-4618-93d6-fd741f576896
📒 Files selected for processing (81)
.changeset/cli-doctor-command.mdpackages/cli/src/__tests__/installer.test.tspackages/cli/src/bin/stash.tspackages/cli/src/commands/db/status.tspackages/cli/src/commands/db/validate.tspackages/cli/src/commands/doctor/__tests__/checks/auth-authenticated.test.tspackages/cli/src/commands/doctor/__tests__/checks/auth-workspace-id-matches-config.test.tspackages/cli/src/commands/doctor/__tests__/checks/config-encryption-client-loadable.test.tspackages/cli/src/commands/doctor/__tests__/checks/config-stash-config-present.test.tspackages/cli/src/commands/doctor/__tests__/checks/database-connects.test.tspackages/cli/src/commands/doctor/__tests__/checks/database-eql-installed.test.tspackages/cli/src/commands/doctor/__tests__/checks/env-cs-client-credentials.test.tspackages/cli/src/commands/doctor/__tests__/checks/env-database-url.test.tspackages/cli/src/commands/doctor/__tests__/checks/env-dotenv-files.test.tspackages/cli/src/commands/doctor/__tests__/checks/integration-drizzle-kit-installed.test.tspackages/cli/src/commands/doctor/__tests__/checks/project-node-version.test.tspackages/cli/src/commands/doctor/__tests__/checks/project-package-json.test.tspackages/cli/src/commands/doctor/__tests__/checks/project-stack-installed.test.tspackages/cli/src/commands/doctor/__tests__/flags.test.tspackages/cli/src/commands/doctor/__tests__/format-human.test.tspackages/cli/src/commands/doctor/__tests__/format-json.test.tspackages/cli/src/commands/doctor/__tests__/runner.test.tspackages/cli/src/commands/doctor/checks/auth/authenticated.tspackages/cli/src/commands/doctor/checks/auth/workspace-id-matches-config.tspackages/cli/src/commands/doctor/checks/config/database-url-set.tspackages/cli/src/commands/doctor/checks/config/encryption-client-exists.tspackages/cli/src/commands/doctor/checks/config/encryption-client-has-tables.tspackages/cli/src/commands/doctor/checks/config/encryption-client-loadable.tspackages/cli/src/commands/doctor/checks/config/stash-config-present.tspackages/cli/src/commands/doctor/checks/config/stash-config-valid.tspackages/cli/src/commands/doctor/checks/database/connects.tspackages/cli/src/commands/doctor/checks/database/eql-installed.tspackages/cli/src/commands/doctor/checks/database/eql-version.tspackages/cli/src/commands/doctor/checks/database/role-permissions.tspackages/cli/src/commands/doctor/checks/env/cs-client-credentials.tspackages/cli/src/commands/doctor/checks/env/cs-workspace-crn.tspackages/cli/src/commands/doctor/checks/env/database-url.tspackages/cli/src/commands/doctor/checks/env/dotenv-files.tspackages/cli/src/commands/doctor/checks/integration/drizzle-kit-installed.tspackages/cli/src/commands/doctor/checks/integration/drizzle-migrations-dir.tspackages/cli/src/commands/doctor/checks/integration/supabase-grants.tspackages/cli/src/commands/doctor/checks/project/cli-installed.tspackages/cli/src/commands/doctor/checks/project/integration-detected.tspackages/cli/src/commands/doctor/checks/project/node-version.tspackages/cli/src/commands/doctor/checks/project/package-json.tspackages/cli/src/commands/doctor/checks/project/stack-installed.tspackages/cli/src/commands/doctor/checks/project/typescript.tspackages/cli/src/commands/doctor/context.tspackages/cli/src/commands/doctor/format/human.tspackages/cli/src/commands/doctor/format/json.tspackages/cli/src/commands/doctor/index.tspackages/cli/src/commands/doctor/lib/package-manager.tspackages/cli/src/commands/doctor/lib/package.tspackages/cli/src/commands/doctor/registry.tspackages/cli/src/commands/doctor/runner.tspackages/cli/src/commands/doctor/types.tspackages/cli/src/commands/init/providers/drizzle.tspackages/cli/src/commands/init/providers/supabase.tspackages/cli/src/commands/init/steps/authenticate.tspackages/cli/src/commands/schema/build.tspackages/cli/src/commands/wizard/__tests__/agent-sdk.test.tspackages/cli/src/commands/wizard/__tests__/commandments.test.tspackages/cli/src/commands/wizard/__tests__/detect.test.tspackages/cli/src/commands/wizard/__tests__/format.test.tspackages/cli/src/commands/wizard/__tests__/gateway-messages.test.tspackages/cli/src/commands/wizard/__tests__/health-checks.test.tspackages/cli/src/commands/wizard/__tests__/hooks.test.tspackages/cli/src/commands/wizard/__tests__/interface.test.tspackages/cli/src/commands/wizard/__tests__/wizard-tools.test.tspackages/cli/src/commands/wizard/agent/errors.tspackages/cli/src/commands/wizard/agent/fetch-prompt.tspackages/cli/src/commands/wizard/agent/hooks.tspackages/cli/src/commands/wizard/agent/interface.tspackages/cli/src/commands/wizard/health-checks/index.tspackages/cli/src/commands/wizard/lib/format.tspackages/cli/src/commands/wizard/lib/gather.tspackages/cli/src/commands/wizard/lib/prerequisites.tspackages/cli/src/commands/wizard/lib/types.tspackages/cli/src/commands/wizard/tools/wizard-tools.tspackages/cli/src/config/index.tspackages/cli/src/lib/auth-state.ts
Comment on lines
+1
to
+4
| import { describe, expect, it } from 'vitest' | ||
| import { databaseConnects } from '../../checks/database/connects.js' | ||
| import type { CheckContext } from '../../types.js' | ||
|
|
There was a problem hiding this comment.
Add import 'dotenv/config' — this test reads process.env.DATABASE_URL.
Per coding guidelines for **/__tests__/**/*.test.ts: "Import dotenv/config at the top of test files that need environment variables". Without it, the it.skipIf(!process.env.DATABASE_URL)(...) gate silently skips the real-DB case whenever DATABASE_URL lives only in a .env file rather than the shell.
Proposed fix
+import 'dotenv/config'
import { describe, expect, it } from 'vitest'
import { databaseConnects } from '../../checks/database/connects.js'
import type { CheckContext } from '../../types.js'As per coding guidelines: "Import dotenv/config at the top of test files that need environment variables".
📝 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.
Suggested change
| import { describe, expect, it } from 'vitest' | |
| import { databaseConnects } from '../../checks/database/connects.js' | |
| import type { CheckContext } from '../../types.js' | |
| import 'dotenv/config' | |
| import { describe, expect, it } from 'vitest' | |
| import { databaseConnects } from '../../checks/database/connects.js' | |
| import type { CheckContext } from '../../types.js' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/doctor/__tests__/checks/database-connects.test.ts`
around lines 1 - 4, This test file is missing the dotenv loader so DATABASE_URL
from a .env isn't available; add import 'dotenv/config' as the very first import
in packages/cli/src/commands/doctor/__tests__/checks/database-connects.test.ts
(before the current imports of vitest and databaseConnects) so the
it.skipIf(!process.env.DATABASE_URL)(...) gate works correctly and real-DB cases
aren't silently skipped when env vars come from a .env file.
Comment on lines
+52
to
+57
| it('fails against an unreachable database', async () => { | ||
| const result = await databaseConnects.run( | ||
| ctxFor('postgres://nope:1/postgres'), | ||
| ) | ||
| expect(result.status).toBe('fail') | ||
| }) |
There was a problem hiding this comment.
Unreachable-DB test may hang on networks that resolve nope.
The check's pg.Client has no connectionTimeoutMillis set, so this test depends on DNS returning ENOTFOUND for nope quickly. Networks with captive DNS (or some CI runners) may resolve it and then stall on TCP connect, causing flaky timeouts. Consider a guaranteed-unreachable target like a non-routable IP with an explicit short timeout, or run with vitest's per-test timeout:
Proposed fix
- it('fails against an unreachable database', async () => {
+ it('fails against an unreachable database', async () => {
const result = await databaseConnects.run(
- ctxFor('postgres://nope:1/postgres'),
+ // 198.51.100.0/24 is reserved (TEST-NET-2) and guaranteed unroutable.
+ ctxFor('postgres://user:pw@198.51.100.1:1/postgres'),
)
expect(result.status).toBe('fail')
- })
+ }, 10_000)(Separately, the underlying databaseConnects implementation could benefit from a short connectionTimeoutMillis so doctor doesn't stall users — worth flagging in checks/database/connects.ts.)
📝 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.
Suggested change
| it('fails against an unreachable database', async () => { | |
| const result = await databaseConnects.run( | |
| ctxFor('postgres://nope:1/postgres'), | |
| ) | |
| expect(result.status).toBe('fail') | |
| }) | |
| it('fails against an unreachable database', async () => { | |
| const result = await databaseConnects.run( | |
| // 198.51.100.0/24 is reserved (TEST-NET-2) and guaranteed unroutable. | |
| ctxFor('postgres://user:pw@198.51.100.1:1/postgres'), | |
| ) | |
| expect(result.status).toBe('fail') | |
| }, 10_000) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/doctor/__tests__/checks/database-connects.test.ts`
around lines 52 - 57, The unreachable-DB test can hang because DNS may resolve
"nope"; update the test to target a guaranteed-unroutable address (e.g. use
ctxFor('postgres://10.255.255.1:5432/postgres') or another RFC1918 non-routable
IP) and enforce a short timeout for the test (use Vitest's per-test timeout or
set a short connect timeout when invoking databaseConnects.run). Additionally,
add a short connectionTimeoutMillis in the real implementation
(checks/database/connects.ts where the pg.Client is constructed) so
databaseConnects uses that setting by default and the doctor check won't stall
users.
Comment on lines
+1
to
+3
| import { describe, expect, it } from 'vitest' | ||
| import { databaseEqlInstalled } from '../../checks/database/eql-installed.js' | ||
| import type { CheckContext } from '../../types.js' |
There was a problem hiding this comment.
Missing dotenv/config import for env-dependent test.
This test reads process.env.DATABASE_URL to gate and drive the integration assertion at lines 43-51. Per coding guidelines, test files that need environment variables should import dotenv/config at the top so a developer-supplied .env is honored when running the suite locally.
Proposed fix
+import 'dotenv/config'
import { describe, expect, it } from 'vitest'
import { databaseEqlInstalled } from '../../checks/database/eql-installed.js'
import type { CheckContext } from '../../types.js'As per coding guidelines: "Import dotenv/config at the top of test files that need environment variables".
📝 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.
Suggested change
| import { describe, expect, it } from 'vitest' | |
| import { databaseEqlInstalled } from '../../checks/database/eql-installed.js' | |
| import type { CheckContext } from '../../types.js' | |
| import 'dotenv/config' | |
| import { describe, expect, it } from 'vitest' | |
| import { databaseEqlInstalled } from '../../checks/database/eql-installed.js' | |
| import type { CheckContext } from '../../types.js' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cli/src/commands/doctor/__tests__/checks/database-eql-installed.test.ts`
around lines 1 - 3, The test relies on process.env.DATABASE_URL (used in the
integration assertion around lines 43–51) but doesn't load environment files;
add the dotenv/config import at the top of the test file so developer .env
values are loaded before the test runs—i.e., insert an import 'dotenv/config' as
the first statement in
packages/cli/src/commands/doctor/__tests__/checks/database-eql-installed.test.ts
above the existing imports referencing databaseEqlInstalled and CheckContext.
Comment on lines
+23
to
+31
| return { | ||
| status: 'fail', | ||
| message: isNotLoggedIn | ||
| ? 'Not authenticated' | ||
| : `Failed to resolve CipherStash credentials${code ? ` (${code})` : ''}`, | ||
| fixHint: 'Run: stash auth login', | ||
| details: code ? { code } : undefined, | ||
| cause: result.cause, | ||
| } |
There was a problem hiding this comment.
fixHint: 'Run: stash auth login' is misleading for non-login failures.
When code is neither NOT_AUTHENTICATED nor MISSING_WORKSPACE_CRN, the failure is a transport/unknown error (per probeCredentials in packages/cli/src/lib/auth-state.ts), and cause is preserved. Telling those users to run stash auth login will likely be unhelpful — they may already be logged in. Consider gating the hint on isNotLoggedIn.
🔧 Proposed fix
return {
status: 'fail',
message: isNotLoggedIn
? 'Not authenticated'
: `Failed to resolve CipherStash credentials${code ? ` (${code})` : ''}`,
- fixHint: 'Run: stash auth login',
+ fixHint: isNotLoggedIn ? 'Run: stash auth login' : undefined,
details: code ? { code } : undefined,
cause: result.cause,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/doctor/checks/auth/authenticated.ts` around lines
23 - 31, The fixHint currently hardcodes "Run: stash auth login" even for
transport/unknown errors; update the object returned in authenticated.ts so
fixHint is conditional on isNotLoggedIn (only suggest "Run: stash auth login"
when isNotLoggedIn is true) and for other cases omit the hint or provide a
generic suggestion (e.g., check network/inspect result.cause) so that non-login
failures (identified by code and probeCredentials behavior) aren’t misdirected;
change the logic that builds the return object where isNotLoggedIn, code, and
result.cause are used to set message/fixHint.
Comment on lines
+16
to
+21
| if (!crn) { | ||
| return { | ||
| status: 'pass', | ||
| message: 'CS_WORKSPACE_CRN not set — skipping', | ||
| } | ||
| } |
There was a problem hiding this comment.
Use status: 'skip' instead of 'pass' when the env var is unset.
The message says "skipping" but the status is pass, which inflates summary.pass and is semantically misleading — the check didn't actually verify anything. Use skip so the human/JSON renderers and counts reflect reality.
🔧 Proposed fix
const crn = process.env.CS_WORKSPACE_CRN
if (!crn) {
return {
- status: 'pass',
- message: 'CS_WORKSPACE_CRN not set — skipping',
+ status: 'skip',
+ message: 'CS_WORKSPACE_CRN not set',
}
}📝 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.
Suggested change
| if (!crn) { | |
| return { | |
| status: 'pass', | |
| message: 'CS_WORKSPACE_CRN not set — skipping', | |
| } | |
| } | |
| if (!crn) { | |
| return { | |
| status: 'skip', | |
| message: 'CS_WORKSPACE_CRN not set', | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/doctor/checks/auth/workspace-id-matches-config.ts`
around lines 16 - 21, The early return in workspace-id-matches-config.ts
currently returns { status: 'pass', message: 'CS_WORKSPACE_CRN not set —
skipping' } when crn is unset; change the status to 'skip' so the check is
recorded as skipped (i.e., return { status: 'skip', message: ... }). Locate the
early-return/guard in the function handling the workspace ID check (the block
that begins with if (!crn)) and modify only the status string from 'pass' to
'skip' while keeping the existing message and structure.
Comment on lines
+90
to
+103
| const encryptConfig = client.getEncryptConfig() as | ||
| | { tables?: Record<string, unknown> } | ||
| | undefined | ||
|
|
||
| const tableCount = encryptConfig?.tables | ||
| ? Object.keys(encryptConfig.tables).length | ||
| : 0 | ||
|
|
||
| return { | ||
| ok: true, | ||
| resolvedPath, | ||
| config: configResult.config, | ||
| tableCount, | ||
| } |
There was a problem hiding this comment.
Wrap getEncryptConfig() in try/catch — an exception here defeats the structured-result contract.
loadEncryptClient returns structured { ok: false, reason } for every other failure mode, but a throw from client.getEncryptConfig() propagates out, the rejection gets cached by memoAsync, and every check that calls cache.encryptClient() ends up as a generic "threw an unexpected error" fail in runCheck (runner.ts L18–22). The user loses the actionable signal that the encryption client is the culprit.
🛡️ Proposed fix
Add a new variant to EncryptClientLoadResult in types.ts:
| { ok: false; reason: 'no-export'; resolvedPath: string }
+ | { ok: false; reason: 'getEncryptConfig-threw'; resolvedPath: string; cause: unknown }
| {Then guard the call:
- const encryptConfig = client.getEncryptConfig() as
- | { tables?: Record<string, unknown> }
- | undefined
-
- const tableCount = encryptConfig?.tables
- ? Object.keys(encryptConfig.tables).length
- : 0
+ let encryptConfig: { tables?: Record<string, unknown> } | undefined
+ try {
+ encryptConfig = client.getEncryptConfig() as
+ | { tables?: Record<string, unknown> }
+ | undefined
+ } catch (cause) {
+ return { ok: false, reason: 'getEncryptConfig-threw', resolvedPath, cause }
+ }
+
+ const tableCount = encryptConfig?.tables
+ ? Object.keys(encryptConfig.tables).length
+ : 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/doctor/context.ts` around lines 90 - 103, The call
to client.getEncryptConfig() can throw and currently escapes the structured
result contract; update the EncryptClientLoadResult type in types.ts to add a
new failure variant (e.g., { ok: false, reason: 'getEncryptConfigFailed', error:
unknown }) and then wrap the getEncryptConfig() invocation inside a try/catch in
the loadEncryptClient implementation so that any thrown error is caught and you
return the new structured failure instead of letting the exception propagate
into memoAsync; ensure cache.encryptClient() continues to return the structured
EncryptClientLoadResult so runCheck receives an actionable failure.
Comment on lines
+113
to
+133
| const token = memoAsync(async () => { | ||
| const result = await probeCredentials() | ||
| if (!result.ok) return result | ||
| // probeCredentials only returns ok: true after getToken() resolves, but it | ||
| // doesn't surface the token shape. Re-invoke to capture the claims for | ||
| // downstream checks that need workspaceId/services. | ||
| try { | ||
| const auth = (await import('@cipherstash/auth')).default | ||
| const tokenResult = await auth.AutoStrategy.detect().getToken() | ||
| const token: TokenInfo = { | ||
| workspaceId: tokenResult.workspaceId, | ||
| subject: tokenResult.subject, | ||
| issuer: tokenResult.issuer, | ||
| services: tokenResult.services, | ||
| } | ||
| return { ok: true as const, token } | ||
| } catch (cause) { | ||
| const code = (cause as { code?: string } | null)?.code | ||
| return { ok: false as const, code, cause } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm token claims aren't surfaced verbatim by the doctor formatters or by checks via details/cause.
rg -nP -C3 --type=ts '\b(subject|workspaceId|issuer|services|TokenInfo)\b' packages/cli/src/commands/doctorRepository: cipherstash/stack
Length of output: 11812
🏁 Script executed:
find packages/cli -type f -name "*format*" | head -20Repository: cipherstash/stack
Length of output: 293
🏁 Script executed:
rg -nP --type=ts '\b(details|subject|workspaceId)\b' packages/cli/src -A2 -B2 | grep -E '(format|human|json)' -A5 -B5 | head -100Repository: cipherstash/stack
Length of output: 7279
🏁 Script executed:
# Look for format/human.ts and format/json.ts specifically
fd -t f 'format' packages/cli/src --type f | rg '(human|json)\.ts$'Repository: cipherstash/stack
Length of output: 43
🏁 Script executed:
find packages/cli/src/commands/doctor -type f -name "*.ts" | grep -E '(format|human|json)' | grep -v testRepository: cipherstash/stack
Length of output: 222
🏁 Script executed:
# Look for the format directory and files in doctor command
ls -la packages/cli/src/commands/doctor/format/ 2>/dev/null || echo "Directory doesn't exist, searching..."
fd 'format' packages/cli/src/commands/doctor --type f --glob '*.ts' | grep -v testRepository: cipherstash/stack
Length of output: 356
🏁 Script executed:
cat -n packages/cli/src/commands/doctor/format/json.tsRepository: cipherstash/stack
Length of output: 984
🏁 Script executed:
cat -n packages/cli/src/commands/doctor/format/human.tsRepository: cipherstash/stack
Length of output: 4707
JSON formatter outputs token claims (subject, workspaceId) from check details without redaction.
The renderJson() function outputs details: result.details verbatim (line 21 of format/json.ts). The auth checks (auth-authenticated.ts, workspace-id-matches-config.ts) already include token claims in their details objects—specifically workspaceId and subject. When stash doctor --json is used, these claims will be emitted to stdout unredacted.
The human formatter does not expose the details field, so it is compliant. However, the JSON output violates the coding guideline: "Do NOT log plaintext; the library never logs plaintext by design and logs should never leak sensitive data." Consider masking or omitting sensitive fields from check result details before they reach the JSON formatter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/doctor/context.ts` around lines 113 - 133, The JSON
formatter currently emits check result details verbatim (renderJson), which
causes sensitive token claims (e.g., workspaceId, subject, token, services)
added by auth checks (auth-authenticated, workspace-id-matches-config) to be
printed; update the JSON rendering pipeline to sanitize details before output by
stripping or redacting those fields (at minimum workspaceId and subject, plus
any nested token/services) in renderJson() so the JSON output never contains
plaintext token claims, or alternatively perform the same sanitization when the
auth checks build their details objects (auth-authenticated.ts /
workspace-id-matches-config.ts) to ensure details do not include raw token
claims. Ensure the sanitizer targets keys "workspaceId", "subject", "token", and
"services" and replaces values with a fixed redaction marker (e.g.,
"[REDACTED]") or omits them entirely.
Comment on lines
+73
to
+82
| const rawOnly = values.only?.trim() | ||
| const only: CheckCategory[] = [] | ||
| if (rawOnly) { | ||
| for (const part of rawOnly.split(',')) { | ||
| const trimmed = part.trim() | ||
| if (trimmed && isCategory(trimmed)) { | ||
| only.push(trimmed) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent drop of invalid --only categories falls back to running everything.
If a user passes --only=foo (typo) or --only=bar,baz, every value is filtered out, only becomes [], and the predicate at lines 38–41 then runs all checks — the opposite of the user's intent. Consider warning (or erroring out non-zero) when none of the provided tokens are valid categories, and surfacing the allowed list.
🔧 Sketch
const rawOnly = values.only?.trim()
const only: CheckCategory[] = []
+ const invalid: string[] = []
if (rawOnly) {
for (const part of rawOnly.split(',')) {
const trimmed = part.trim()
- if (trimmed && isCategory(trimmed)) {
- only.push(trimmed)
- }
+ if (!trimmed) continue
+ if (isCategory(trimmed)) only.push(trimmed)
+ else invalid.push(trimmed)
}
}
+ if (invalid.length > 0) {
+ p.log.warn(
+ `Ignoring unknown --only categories: ${invalid.join(', ')}. ` +
+ `Valid categories: ${CATEGORIES.join(', ')}.`,
+ )
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/doctor/index.ts` around lines 73 - 82, The current
parsing of values.only into rawOnly and only silently discards invalid tokens
(rawOnly, only, isCategory) so an empty only array causes the program to run all
checks contrary to intent; update the logic after building only to detect when
values.only was provided but only.length === 0 and then either (a) print an
error/warning listing allowed categories (derive allowed names from
CheckCategory union or isCategory's accepted set) and exit with non-zero, or (b)
return a CLI error informing the user their tokens were invalid and showing the
list of valid categories, ensuring you reference values.only, rawOnly and
isCategory in the message so users can correct typos.
Comment on lines
+65
to
+71
| | { | ||
| ok: true | ||
| resolvedPath: string | ||
| config: ResolvedStashConfig | ||
| /** `undefined` when the client resolves but `getEncryptConfig()` returns no tables. */ | ||
| tableCount: number | ||
| } |
There was a problem hiding this comment.
Stale JSDoc on tableCount.
The comment says undefined when the client resolves but getEncryptConfig() returns no tables, but the field is typed number and context.ts (lines 94–96) returns 0 in that case. Either tighten the type to number | undefined and update the loader, or fix the comment to reflect the 0 semantics. The latter is almost certainly what you want, since checks that count tables shouldn't have to handle two empty representations.
📝 Suggested fix
| {
ok: true
resolvedPath: string
config: ResolvedStashConfig
- /** `undefined` when the client resolves but `getEncryptConfig()` returns no tables. */
+ /** `0` when the client resolves but `getEncryptConfig()` returns no tables. */
tableCount: number
}📝 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.
Suggested change
| | { | |
| ok: true | |
| resolvedPath: string | |
| config: ResolvedStashConfig | |
| /** `undefined` when the client resolves but `getEncryptConfig()` returns no tables. */ | |
| tableCount: number | |
| } | |
| | { | |
| ok: true | |
| resolvedPath: string | |
| config: ResolvedStashConfig | |
| /** `0` when the client resolves but `getEncryptConfig()` returns no tables. */ | |
| tableCount: number | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/doctor/types.ts` around lines 65 - 71, The JSDoc
for the tableCount property is stale: it says the value can be `undefined` when
there are no tables, but the type is `number` and the loader
(`getEncryptConfig()` path in context.ts) returns 0 for no tables. Update the
JSDoc on the tableCount field to state that it is 0 when no tables are present
(keep the type as `number`), and confirm no other code expects `undefined` for
tableCount; if any code does, convert those checks to compare against 0 instead.
Comment on lines
+484
to
501
| if (session.debug) { | ||
| p.log.warn( | ||
| `[debug] Result message: ${JSON.stringify( | ||
| { | ||
| subtype: message.subtype, | ||
| is_error: message.is_error, | ||
| error: message.error, | ||
| error_details: message.error_details, | ||
| result: message.result?.slice(0, 500), | ||
| last_assistant_message: | ||
| message.last_assistant_message?.slice(0, 500), | ||
| stop_reason: message.stop_reason, | ||
| }, | ||
| null, | ||
| 2, | ||
| )}`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Debug logging may leak sensitive data
The debug logs output error_details, result, and last_assistant_message which could contain:
- File contents from agent Read operations
- Environment variable values
- Database connection strings
- Other plaintext user data
While this is gated behind session.debug, it violates the coding guideline: "Do NOT log plaintext; the library never logs plaintext by design and logs should never leak sensitive data".
Consider either:
- Redacting sensitive fields before logging
- Logging only metadata (subtype, is_error, stop_reason) without content
- Adding a warning that debug mode may log sensitive information
As per coding guidelines, **/*.{ts,tsx,js,jsx}: Do NOT log plaintext; the library never logs plaintext by design and logs should never leak sensitive data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/agent/interface.ts` around lines 484 - 501,
The current debug block in the wizard agent (the session.debug conditional that
calls p.log.warn with the serialized message) may log sensitive plaintext from
fields like error_details, result, and last_assistant_message; update this block
to sanitize or omit sensitive content before logging by either redacting those
fields (e.g., replace values with "<REDACTED>" or only include lengths), or log
only safe metadata (subtype, is_error, stop_reason) and optionally a
boolean/size indicator for result/error_details; ensure you modify the code that
builds the object passed to p.log.warn (the JSON.stringify payload) so it never
includes raw error_details, result, or last_assistant_message values even when
session.debug is true.
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
1 participant
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.
Add
stash doctorcommand — a single read-only diagnostic that checks the health of a CipherStash install across project state, config, auth, environment, database, and ORM integration. Prints a categorised human report by default, or--jsonfor a stable machine-readable shape;--only <category>narrows the run and--skip-dbavoids DB-opening checks.--fixis reserved for a follow-up PR.Summary by CodeRabbit
New Features
stash doctorcommand for diagnosing project health across configuration, authentication, environment, database, and ORM integration.--jsonfor machine-readable output,--only <category>to filter checks, and--skip-dbto skip database operations.Bug Fixes & Refactoring