From 0744f9a84c2334deaf31aff6e976b5a608797bd5 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Thu, 23 Jul 2026 15:01:52 +0200 Subject: [PATCH 01/14] feat(cli): add appkit doctor command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diagnose whether an AppKit app's declared Databricks resources are actually usable, beyond the startup env-var check. Registered as `appkit doctor`. Three layers per resource: - auth: validate DATABRICKS_HOST, then currentUser.me() (once, app-wide) - config: offline env-var presence check - existence: live per-type probe — control-plane .get() for warehouse, serving, genie, job, volume, vector index, uc_function; a real SELECT 1 connection for Lakebase/postgres. Errors are classified (NOT_FOUND, INVALID_VALUE, ACCESS_DENIED) with clean one-line messages. Actionable hints translate opaque failures into the fix: expired/missing credentials to the right `databricks auth login`, a serving endpoint keyed by id to its name, and a Lakebase auth failure to the PGUSER/identity mismatch. Reaches the Databricks SDK / @databricks/appkit only through a runtime import in databricks-client.ts, keeping the SDK-free shared package free of the dependency and degrading gracefully when it is absent. Output is a friendly list (errors first; plugin/type + reason shown only on rows needing attention) or --json; exit code is non-zero on any error so it can gate CI. Signed-off-by: Galymzhan --- .../shared/src/cli/commands/doctor/README.md | 53 +++ .../commands/doctor/checks-existence.test.ts | 333 +++++++++++++++++ .../cli/commands/doctor/checks-existence.ts | 346 ++++++++++++++++++ .../src/cli/commands/doctor/checks.test.ts | 184 ++++++++++ .../shared/src/cli/commands/doctor/checks.ts | 197 ++++++++++ .../cli/commands/doctor/databricks-client.ts | 96 +++++ .../shared/src/cli/commands/doctor/index.ts | 43 +++ .../src/cli/commands/doctor/report.test.ts | 132 +++++++ .../shared/src/cli/commands/doctor/report.ts | 101 +++++ .../commands/doctor/resolve-targets.test.ts | 180 +++++++++ .../cli/commands/doctor/resolve-targets.ts | 140 +++++++ .../src/cli/commands/doctor/run.test.ts | 84 +++++ .../shared/src/cli/commands/doctor/run.ts | 88 +++++ .../shared/src/cli/commands/doctor/types.ts | 63 ++++ packages/shared/src/cli/index.ts | 2 + 15 files changed, 2042 insertions(+) create mode 100644 packages/shared/src/cli/commands/doctor/README.md create mode 100644 packages/shared/src/cli/commands/doctor/checks-existence.test.ts create mode 100644 packages/shared/src/cli/commands/doctor/checks-existence.ts create mode 100644 packages/shared/src/cli/commands/doctor/checks.test.ts create mode 100644 packages/shared/src/cli/commands/doctor/checks.ts create mode 100644 packages/shared/src/cli/commands/doctor/databricks-client.ts create mode 100644 packages/shared/src/cli/commands/doctor/index.ts create mode 100644 packages/shared/src/cli/commands/doctor/report.test.ts create mode 100644 packages/shared/src/cli/commands/doctor/report.ts create mode 100644 packages/shared/src/cli/commands/doctor/resolve-targets.test.ts create mode 100644 packages/shared/src/cli/commands/doctor/resolve-targets.ts create mode 100644 packages/shared/src/cli/commands/doctor/run.test.ts create mode 100644 packages/shared/src/cli/commands/doctor/run.ts create mode 100644 packages/shared/src/cli/commands/doctor/types.ts diff --git a/packages/shared/src/cli/commands/doctor/README.md b/packages/shared/src/cli/commands/doctor/README.md new file mode 100644 index 000000000..b746c6f38 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/README.md @@ -0,0 +1,53 @@ +# `appkit doctor` + +Diagnoses whether an AppKit app's declared Databricks resources are actually +usable — going beyond the startup env-var presence check to probe existence and +reachability against the live workspace. + +## The check model — three layers + +For each declared resource, doctor runs the offline `config` layer then the live +`existence` layer; `auth` runs once up front. It stops at the first hard failure +so the reported problem is the *root* cause, not a symptom. + +| Layer | Question | How | +| ------------ | ----------------------------------------------------- | ---------------------------------------------------------- | +| `auth` | Can we authenticate to the workspace at all? | validate `DATABRICKS_HOST` is a real URL, then `currentUser.me()` — once, app-wide; a failure skips the live layer | +| `config` | Are the resource's field env vars **present**? | offline presence check of `process.env` (presence only — see note) | +| `existence` | Does the resource exist and is it reachable? | cheapest per-type live probe (`warehouses.get`, `servingEndpoints.get`, …); Lakebase runs a real `SELECT 1` | + +`config` checks env-var presence only; whether a value points at a real resource +is the `existence` layer's job. (`DATABRICKS_HOST` is the exception — `auth` +validates it structurally, since a bad host means no client can be built.) + +## Files + +- `types.ts` — the contract: layers, statuses, `ResourceTarget`, `DoctorReport`. +- `resolve-targets.ts` — reads `appkit.plugins.json` → `ResourceTarget[]`. +- `databricks-client.ts` — the sole SDK seam: dynamic `import()` of the SDK / + `@databricks/appkit`, builds a `WorkspaceClient` and Lakebase pool, graceful + fallback when uninstalled. +- `checks.ts` — `checkAuth`, `checkConfig`, `checkExistence`. +- `checks-existence.ts` — per-type existence probe dispatch + error classifier. +- `run.ts` — orchestration: auth once → per resource (config → existence). +- `report.ts` — human table + `--json`, and the CI-gating exit code. +- `index.ts` — the Commander command + flags. + +## Flags + +- `--profile ` — Databricks CLI profile to authenticate with. +- `--json` — machine-readable report. + +Exit code is non-zero if auth or any resource is in an `error` state, so +`appkit doctor` can gate CI / pre-deploy. + +Checks run as the identity that runs doctor (the developer locally, the app in +deployment). + +## Existence coverage + +Control-plane `.get()` for `sql_warehouse`, `serving_endpoint`, `genie_space`, +`job`, `volume`, `vector_search_index`, and `uc_function`; a real `SELECT 1` +connection for `postgres`/Lakebase. Other types report `skipped`. + +Requires live credentials (a configured Databricks profile/token). diff --git a/packages/shared/src/cli/commands/doctor/checks-existence.test.ts b/packages/shared/src/cli/commands/doctor/checks-existence.test.ts new file mode 100644 index 000000000..f621ef5b7 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/checks-existence.test.ts @@ -0,0 +1,333 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { runExistenceProbe, toThreeLevelVolumeName } from "./checks-existence"; +import type { ResourceTarget } from "./types"; + +// Mock the SDK bridge so the postgres probe can be driven without a real +// Lakebase connection. `getLakebasePool` returns a fake pool per test. +const { mockGetLakebasePool, endSpy } = vi.hoisted(() => ({ + mockGetLakebasePool: vi.fn(), + endSpy: vi.fn(async () => {}), +})); +vi.mock("./databricks-client", () => ({ + AppkitNotInstalledError: class AppkitNotInstalledError extends Error {}, + getLakebasePool: mockGetLakebasePool, +})); + +function target(overrides: Partial = {}): ResourceTarget { + return { + type: "sql_warehouse", + resourceKey: "sql-warehouse", + alias: "SQL Warehouse", + plugin: "analytics", + requiredPermission: "CAN_USE", + required: true, + envVars: ["DATABRICKS_WAREHOUSE_ID"], + fieldValues: { id: "wh-123" }, + ...overrides, + }; +} + +/** Builds an SDK-like error carrying a statusCode (as ApiError does). */ +function apiError(statusCode: number, message = "boom"): Error { + return Object.assign(new Error(message), { statusCode }); +} + +describe("runExistenceProbe — sql_warehouse", () => { + it("ok when the warehouse exists and is RUNNING", async () => { + const client = { + warehouses: { get: async () => ({ state: "RUNNING" }) }, + }; + const r = await runExistenceProbe(client, target()); + expect(r.status).toBe("ok"); + }); + + it("warns when the warehouse exists but is STOPPED", async () => { + const client = { + warehouses: { get: async () => ({ state: "STOPPED" }) }, + }; + const r = await runExistenceProbe(client, target()); + expect(r.status).toBe("warn"); + expect(r.code).toBe("WAREHOUSE_NOT_RUNNING"); + }); + + it("errors NOT_FOUND on a 404", async () => { + const client = { + warehouses: { + get: async () => { + throw apiError(404); + }, + }, + }; + const r = await runExistenceProbe(client, target()); + expect(r.status).toBe("error"); + expect(r.code).toBe("NOT_FOUND"); + }); + + it("errors INVALID_VALUE on a 400 with a clean message", async () => { + const client = { + warehouses: { + get: async () => { + throw Object.assign( + new Error( + 'Response from server (Bad Request) {"error_code":"INVALID_PARAMETER_VALUE","message":"bogus is not a valid endpoint id."}', + ), + { statusCode: 400, errorCode: "INVALID_PARAMETER_VALUE" }, + ); + }, + }, + }; + const r = await runExistenceProbe(client, target()); + expect(r.status).toBe("error"); + expect(r.code).toBe("INVALID_VALUE"); + // The clean inner message is extracted, not the whole JSON blob. + expect(r.detail).toContain("not a valid endpoint id"); + expect(r.detail).not.toContain("error_code"); + }); + + it("errors ACCESS_DENIED on a 403", async () => { + const client = { + warehouses: { + get: async () => { + throw apiError(403); + }, + }, + }; + const r = await runExistenceProbe(client, target()); + expect(r.status).toBe("error"); + expect(r.code).toBe("ACCESS_DENIED"); + }); + + it("skips when the id field is missing", async () => { + const client = { warehouses: { get: async () => ({}) } }; + const r = await runExistenceProbe(client, target({ fieldValues: {} })); + expect(r.status).toBe("skipped"); + expect(r.code).toBe("MISSING_FIELD"); + }); +}); + +describe("runExistenceProbe — job", () => { + it("errors on a non-integer job id", async () => { + const client = { jobs: { get: async () => ({}) } }; + const r = await runExistenceProbe( + client, + target({ type: "job", fieldValues: { id: "not-a-number" } }), + ); + expect(r.status).toBe("error"); + expect(r.code).toBe("INVALID_ID"); + }); + + it("ok for a valid integer job id", async () => { + const client = { jobs: { get: async () => ({}) } }; + const r = await runExistenceProbe( + client, + target({ type: "job", fieldValues: { id: "42" } }), + ); + expect(r.status).toBe("ok"); + }); +}); + +describe("runExistenceProbe — unsupported type", () => { + it("skips NOT_IMPLEMENTED for a type with no probe", async () => { + const r = await runExistenceProbe( + {}, + target({ type: "secret", fieldValues: {} }), + ); + expect(r.status).toBe("skipped"); + expect(r.code).toBe("NOT_IMPLEMENTED"); + }); +}); + +describe("runExistenceProbe — postgres (Lakebase)", () => { + afterEach(() => { + mockGetLakebasePool.mockReset(); + endSpy.mockClear(); + }); + + function pgTarget(overrides: Partial = {}): ResourceTarget { + return target({ + type: "postgres", + requiredPermission: "CAN_CONNECT_AND_CREATE", + fieldValues: { host: "ep.example.com", endpointPath: "projects/x" }, + ...overrides, + }); + } + + it("ok when SELECT 1 succeeds, and closes the pool", async () => { + const query = vi.fn(async () => ({ rows: [{ "?column?": 1 }] })); + mockGetLakebasePool.mockResolvedValue({ query, end: endSpy }); + + const r = await runExistenceProbe({}, pgTarget()); + expect(r.status).toBe("ok"); + expect(query).toHaveBeenCalledWith("SELECT 1"); + expect(endSpy).toHaveBeenCalled(); // pool always closed + }); + + it("errors CONNECTION_FAILED when the query throws, and still closes", async () => { + const query = vi.fn(async () => { + throw new Error("ECONNREFUSED 10.0.0.1:5432"); + }); + mockGetLakebasePool.mockResolvedValue({ query, end: endSpy }); + + const r = await runExistenceProbe({}, pgTarget()); + expect(r.status).toBe("error"); + expect(r.code).toBe("CONNECTION_FAILED"); + expect(r.detail).toContain("ECONNREFUSED"); + expect(endSpy).toHaveBeenCalled(); + }); + + it("adds an OAuth/PGUSER hint on 'password authentication failed'", async () => { + const query = vi.fn(async () => { + throw new Error( + 'password authentication failed for user "galymzhan.zhangazy%40databricks.com"', + ); + }); + mockGetLakebasePool.mockResolvedValue({ query, end: endSpy }); + + const r = await runExistenceProbe({}, pgTarget()); + expect(r.status).toBe("error"); + expect(r.code).toBe("CONNECTION_FAILED"); + // The raw error stays in detail; the actionable guidance is a separate hint. + expect(r.detail).toMatch(/password authentication failed/i); + expect(r.hint).toMatch(/OAuth token as the password/i); + expect(r.hint).toMatch(/PGUSER/); + }); + + it("does NOT add the auth hint for unrelated connection errors", async () => { + const query = vi.fn(async () => { + throw new Error("ECONNREFUSED 10.0.0.1:5432"); + }); + mockGetLakebasePool.mockResolvedValue({ query, end: endSpy }); + + const r = await runExistenceProbe({}, pgTarget()); + expect(r.hint).toBeUndefined(); + }); + + it("skips with MISSING_FIELD when neither host nor endpoint resolved", async () => { + const r = await runExistenceProbe({}, pgTarget({ fieldValues: {} })); + expect(r.status).toBe("skipped"); + expect(r.code).toBe("MISSING_FIELD"); + // Pool was never created. + expect(mockGetLakebasePool).not.toHaveBeenCalled(); + }); +}); + +// These use the REAL first-party manifest field keys (e.g. vector-search's +// camelCase `indexName`) — the case that previously slipped through as a silent +// skip. Each supported probe gets a happy path + a missing-field skip. +describe("runExistenceProbe — per-type coverage with real manifest keys", () => { + it("serving_endpoint: ok via `name`", async () => { + const client = { servingEndpoints: { get: async () => ({}) } }; + const r = await runExistenceProbe( + client, + target({ + type: "serving_endpoint", + fieldValues: { name: "my-endpoint" }, + }), + ); + expect(r.status).toBe("ok"); + }); + + it("serving_endpoint: hints id-vs-name when configured by id and the probe fails", async () => { + const client = { + servingEndpoints: { + get: async () => { + throw Object.assign(new Error("not found"), { statusCode: 404 }); + }, + }, + }; + const r = await runExistenceProbe( + client, + target({ type: "serving_endpoint", fieldValues: { id: "abc-123" } }), + ); + expect(r.status).toBe("error"); + expect(r.hint).toMatch(/looked up by name/i); + }); + + it("serving_endpoint: no id-vs-name hint when configured by name", async () => { + const client = { + servingEndpoints: { + get: async () => { + throw Object.assign(new Error("not found"), { statusCode: 404 }); + }, + }, + }; + const r = await runExistenceProbe( + client, + target({ type: "serving_endpoint", fieldValues: { name: "my-ep" } }), + ); + expect(r.status).toBe("error"); + expect(r.hint).toBeUndefined(); + }); + + it("genie_space: ok via `id`", async () => { + const client = { genie: { getSpace: async () => ({}) } }; + const r = await runExistenceProbe( + client, + target({ type: "genie_space", fieldValues: { id: "01ef" } }), + ); + expect(r.status).toBe("ok"); + }); + + it("volume: ok via `path` (real manifest key)", async () => { + const client = { volumes: { read: async () => ({}) } }; + const r = await runExistenceProbe( + client, + target({ + type: "volume", + fieldValues: { path: "/Volumes/main/default/files" }, + }), + ); + expect(r.status).toBe("ok"); + }); + + it("uc_function: ok via `name`", async () => { + const client = { functions: { get: async () => ({}) } }; + const r = await runExistenceProbe( + client, + target({ type: "uc_function", fieldValues: { name: "cat.sch.fn" } }), + ); + expect(r.status).toBe("ok"); + }); + + it("vector_search_index: probes via camelCase `indexName` (regression: bug #1)", async () => { + const getIndex = vi.fn(async () => ({})); + const client = { vectorSearchIndexes: { getIndex } }; + const r = await runExistenceProbe( + client, + target({ + type: "vector_search_index", + fieldValues: { indexName: "main.default.idx", endpointName: "ep" }, + }), + ); + expect(r.status).toBe("ok"); + expect(getIndex).toHaveBeenCalledWith({ index_name: "main.default.idx" }); + }); + + it("vector_search_index: skips MISSING_FIELD when index name absent", async () => { + const client = { vectorSearchIndexes: { getIndex: async () => ({}) } }; + const r = await runExistenceProbe( + client, + target({ type: "vector_search_index", fieldValues: {} }), + ); + expect(r.status).toBe("skipped"); + expect(r.code).toBe("MISSING_FIELD"); + }); +}); + +describe("toThreeLevelVolumeName", () => { + it("passes through a dotted 3-level name", () => { + expect(toThreeLevelVolumeName("main.default.files")).toBe( + "main.default.files", + ); + }); + + it("extracts from a /Volumes path", () => { + expect(toThreeLevelVolumeName("/Volumes/main/default/files/sub/x")).toBe( + "main.default.files", + ); + }); + + it("returns null for an unparseable value", () => { + expect(toThreeLevelVolumeName("just-a-name")).toBeNull(); + }); +}); diff --git a/packages/shared/src/cli/commands/doctor/checks-existence.ts b/packages/shared/src/cli/commands/doctor/checks-existence.ts new file mode 100644 index 000000000..25526ad19 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/checks-existence.ts @@ -0,0 +1,346 @@ +/** + * Layer: existence — per-resource-type probes that prove a declared resource + * exists and is reachable via the cheapest read the SDK offers. + * + * The client is typed structurally (not via the SDK) so `shared` stays SDK-free. + */ + +import { + AppkitNotInstalledError, + getLakebasePool, + type LakebasePoolHandle, +} from "./databricks-client"; +import type { LayerResult, ResourceTarget } from "./types"; + +interface DoctorWorkspaceClient { + warehouses: { + get: (r: { id: string }) => Promise<{ state?: string }>; + }; + servingEndpoints: { + get: (r: { name: string }) => Promise; + }; + genie: { + getSpace: (r: { space_id: string }) => Promise; + }; + jobs: { + get: (r: { job_id: number }) => Promise; + }; + volumes: { + read: (r: { name: string }) => Promise; + }; + vectorSearchIndexes: { + getIndex: (r: { index_name: string }) => Promise; + }; + functions: { + get: (r: { name: string }) => Promise; + }; +} + +type ExistenceProbe = ( + client: DoctorWorkspaceClient, + target: ResourceTarget, +) => Promise; + +// The SDK's ApiError carries statusCode/errorCode; we read them off the caught +// error structurally rather than importing the class, keeping `shared` SDK-free. +function statusCodeOf(err: unknown): number | undefined { + if (err && typeof err === "object" && "statusCode" in err) { + const code = (err as { statusCode?: unknown }).statusCode; + if (typeof code === "number") return code; + } + return undefined; +} + +function errorCodeOf(err: unknown): string | undefined { + if (err && typeof err === "object" && "errorCode" in err) { + const code = (err as { errorCode?: unknown }).errorCode; + if (typeof code === "string" && code.length > 0) return code; + } + return undefined; +} + +// The SDK message often embeds a JSON blob (`Response from server (...) +// {"message":"..."}`); pull the inner `message` out so doctor prints one clean +// line, not a dump. +function cleanMessage(err: unknown): string { + const raw = err instanceof Error ? err.message : String(err); + const m = raw.match(/"message"\s*:\s*"([^"]+)"/); + return m ? m[1] : raw; +} + +function classifyError(err: unknown, target: ResourceTarget): LayerResult { + const status = statusCodeOf(err); + const errorCode = errorCodeOf(err); + const message = cleanMessage(err); + + if (status === 404 || errorCode === "RESOURCE_DOES_NOT_EXIST") { + return { + layer: "existence", + status: "error", + code: "NOT_FOUND", + detail: `${target.type} not found — check the configured id/name (${message})`, + }; + } + // A malformed id/name often comes back as 400 rather than 404. + if (status === 400 || errorCode === "INVALID_PARAMETER_VALUE") { + return { + layer: "existence", + status: "error", + code: "INVALID_VALUE", + detail: `invalid ${target.type} id/name: ${message}`, + }; + } + if (status === 403 || errorCode === "PERMISSION_DENIED") { + return { + layer: "existence", + status: "error", + code: "ACCESS_DENIED", + detail: `access denied reading ${target.type} — the identity may lack visibility (${message})`, + }; + } + return { + layer: "existence", + status: "error", + code: "PROBE_FAILED", + detail: `failed to read ${target.type}: ${message}`, + }; +} + +/** Normalizes a field key for comparison: manifests use camelCase (`indexName`) + * while scaffold defaults use snake_case (`index_name`), so match case- and + * separator-insensitively. */ +function normalizeKey(key: string): string { + return key.toLowerCase().replace(/[_-]/g, ""); +} + +/** Looks up a resolved field value by any of the accepted key spellings. */ +function field(target: ResourceTarget, ...names: string[]): string | null { + const wanted = new Set(names.map(normalizeKey)); + for (const [key, value] of Object.entries(target.fieldValues)) { + if (wanted.has(normalizeKey(key)) && value.length > 0) return value; + } + return null; +} + +function missingField(fieldName: string): LayerResult { + return { + layer: "existence", + status: "skipped", + code: "MISSING_FIELD", + detail: `cannot probe existence: no resolved value for "${fieldName}"`, + }; +} + +const probeWarehouse: ExistenceProbe = async (client, target) => { + const id = field(target, "id"); + if (!id) return missingField("id"); + try { + const wh = await client.warehouses.get({ id }); + const state = wh.state; + if (state && state !== "RUNNING") { + return { + layer: "existence", + status: "warn", + code: "WAREHOUSE_NOT_RUNNING", + detail: `warehouse exists but is ${state} (will cold-start on first query)`, + }; + } + return { layer: "existence", status: "ok" }; + } catch (err) { + return classifyError(err, target); + } +}; + +const probeServing: ExistenceProbe = async (client, target) => { + const name = field(target, "name"); + // Serving endpoints are looked up by name. If the manifest instead keys this + // resource by `id`, we still probe with that value but flag the likely cause + // when it fails, since the API only accepts a name. + const idOnly = name === null ? field(target, "id") : null; + const value = name ?? idOnly; + if (!value) return missingField("name"); + try { + await client.servingEndpoints.get({ name: value }); + return { layer: "existence", status: "ok" }; + } catch (err) { + const result = classifyError(err, target); + if (idOnly && result.status === "error") { + result.hint = + "Serving endpoints are looked up by name, but this resource is configured by id. Set DATABRICKS_SERVING_ENDPOINT_NAME to the endpoint's name."; + } + return result; + } +}; + +const probeGenie: ExistenceProbe = async (client, target) => { + const spaceId = field(target, "id"); + if (!spaceId) return missingField("id"); + try { + await client.genie.getSpace({ space_id: spaceId }); + return { layer: "existence", status: "ok" }; + } catch (err) { + return classifyError(err, target); + } +}; + +const probeJob: ExistenceProbe = async (client, target) => { + const raw = field(target, "id"); + if (!raw) return missingField("id"); + const jobId = Number(raw); + if (!Number.isInteger(jobId)) { + return { + layer: "existence", + status: "error", + code: "INVALID_ID", + detail: `job id is not a valid integer: "${raw}"`, + }; + } + try { + await client.jobs.get({ job_id: jobId }); + return { layer: "existence", status: "ok" }; + } catch (err) { + return classifyError(err, target); + } +}; + +const probeVolume: ExistenceProbe = async (client, target) => { + // The read API wants the 3-level name (catalog.schema.volume), but the env + // value is usually a /Volumes/... path. + const raw = field(target, "path", "name"); + if (!raw) return missingField("path"); + const name = toThreeLevelVolumeName(raw); + if (!name) { + return { + layer: "existence", + status: "error", + code: "INVALID_NAME", + detail: `cannot derive a 3-level volume name from "${raw}"`, + }; + } + try { + await client.volumes.read({ name }); + return { layer: "existence", status: "ok" }; + } catch (err) { + return classifyError(err, target); + } +}; + +const probeVectorIndex: ExistenceProbe = async (client, target) => { + const name = field(target, "indexName", "index_name", "name"); + if (!name) return missingField("indexName"); + try { + await client.vectorSearchIndexes.getIndex({ index_name: name }); + return { layer: "existence", status: "ok" }; + } catch (err) { + return classifyError(err, target); + } +}; + +const probeFunction: ExistenceProbe = async (client, target) => { + const name = field(target, "name"); + if (!name) return missingField("name"); + try { + await client.functions.get({ name }); + return { layer: "existence", status: "ok" }; + } catch (err) { + return classifyError(err, target); + } +}; + +// Lakebase authenticates with an OAuth token as the Postgres password, so +// "password authentication failed" almost never means a wrong password — it +// usually means PGUSER doesn't match the token's identity. +function lakebaseAuthHint(message: string): string | undefined { + if (/password authentication failed/i.test(message)) { + return ( + "Lakebase uses an OAuth token as the password, so this usually means the" + + " PGUSER/role doesn't match your identity. Check PGUSER is your exact" + + " login (the literal `user@domain`, not URL-encoded)." + ); + } + return undefined; +} + +// Lakebase has no cheap control-plane `.get()`, so existence is proven by a +// real connection + `SELECT 1` (exercising endpoint resolution, OAuth token +// mint, TLS, and reachability — the same path the app uses). +const probePostgres: ExistenceProbe = async (client, target) => { + if (!field(target, "endpointPath") && !field(target, "host")) { + return missingField("host/endpoint"); + } + + let pool: LakebasePoolHandle | null = null; + try { + pool = await getLakebasePool(client); + await pool.query("SELECT 1"); + return { layer: "existence", status: "ok" }; + } catch (err) { + if (err instanceof AppkitNotInstalledError) { + return { + layer: "existence", + status: "skipped", + code: "APPKIT_NOT_INSTALLED", + detail: err.message, + }; + } + const message = cleanMessage(err); + return { + layer: "existence", + status: "error", + code: "CONNECTION_FAILED", + detail: `could not connect to Lakebase Postgres: ${message}`, + hint: lakebaseAuthHint(message), + }; + } finally { + if (pool) { + try { + await pool.end(); + } catch { + // best-effort close + } + } + } +}; + +/** + * Normalizes a volume reference to the SDK's 3-level `catalog.schema.volume` + * name, accepting either an already-dotted name or a `/Volumes/c/s/v/...` path. + */ +export function toThreeLevelVolumeName(raw: string): string | null { + const trimmed = raw.trim(); + if (/^[^./\s]+\.[^./\s]+\.[^./\s]+$/.test(trimmed)) return trimmed; + + const m = trimmed.match(/^\/Volumes\/([^/]+)\/([^/]+)\/([^/]+)/); + if (m) return `${m[1]}.${m[2]}.${m[3]}`; + + return null; +} + +// Types not listed (secret, uc_connection, database, app, experiment) have no +// probe and fall through to NOT_IMPLEMENTED in runExistenceProbe. +const PROBES: Record = { + sql_warehouse: probeWarehouse, + serving_endpoint: probeServing, + genie_space: probeGenie, + job: probeJob, + volume: probeVolume, + vector_search_index: probeVectorIndex, + uc_function: probeFunction, + postgres: probePostgres, +}; + +export async function runExistenceProbe( + client: unknown, + target: ResourceTarget, +): Promise { + const probe = PROBES[target.type]; + if (!probe) { + return { + layer: "existence", + status: "skipped", + code: "NOT_IMPLEMENTED", + detail: `existence check not implemented for ${target.type}`, + }; + } + return probe(client as DoctorWorkspaceClient, target); +} diff --git a/packages/shared/src/cli/commands/doctor/checks.test.ts b/packages/shared/src/cli/commands/doctor/checks.test.ts new file mode 100644 index 000000000..34bc109a6 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/checks.test.ts @@ -0,0 +1,184 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { checkAuth, checkConfig, validateHost } from "./checks"; +import type { ResourceTarget } from "./types"; + +const { mockGetServiceClient } = vi.hoisted(() => ({ + mockGetServiceClient: vi.fn(), +})); +vi.mock("./databricks-client", () => ({ + SdkNotInstalledError: class SdkNotInstalledError extends Error {}, + getServiceClient: mockGetServiceClient, +})); + +function target(overrides: Partial = {}): ResourceTarget { + return { + type: "sql_warehouse", + resourceKey: "sql-warehouse", + alias: "SQL Warehouse", + plugin: "analytics", + requiredPermission: "CAN_USE", + required: true, + envVars: ["DOCTOR_TEST_ENV"], + fieldValues: {}, + ...overrides, + }; +} + +describe("checkConfig", () => { + const saved = { ...process.env }; + + beforeEach(() => { + delete process.env.DOCTOR_TEST_ENV; + delete process.env.DOCTOR_TEST_ENV_2; + }); + + afterEach(() => { + process.env = { ...saved }; + }); + + it("errors when a required resource's env var is unset", async () => { + const result = await checkConfig(target()); + expect(result.status).toBe("error"); + expect(result.code).toBe("ENV_MISSING"); + }); + + it("warns (not errors) when an optional resource's env var is unset", async () => { + const result = await checkConfig(target({ required: false })); + expect(result.status).toBe("warn"); + expect(result.code).toBe("ENV_MISSING_OPTIONAL"); + }); + + it("passes an unfilled-looking value (existence check is the authority)", async () => { + // Placeholder-looking values are NOT flagged here by design — the live + // existence check decides whether a set value is real. + process.env.DOCTOR_TEST_ENV = "your_sql_warehouse_id"; + const result = await checkConfig(target()); + expect(result.status).toBe("ok"); + }); + + it("treats an empty string as missing", async () => { + process.env.DOCTOR_TEST_ENV = " "; + const result = await checkConfig(target()); + expect(result.status).toBe("error"); + expect(result.code).toBe("ENV_MISSING"); + }); + + it("passes when all env vars are set to real-looking values", async () => { + process.env.DOCTOR_TEST_ENV = "abc123def456"; + const result = await checkConfig(target()); + expect(result.status).toBe("ok"); + }); + + it("passes when a resource declares no env vars", async () => { + const result = await checkConfig(target({ envVars: [] })); + expect(result.status).toBe("ok"); + }); + + it("reports all missing env vars when several are unset", async () => { + const result = await checkConfig( + target({ envVars: ["DOCTOR_TEST_ENV", "DOCTOR_TEST_ENV_2"] }), + ); + expect(result.status).toBe("error"); + expect(result.detail).toContain("DOCTOR_TEST_ENV"); + expect(result.detail).toContain("DOCTOR_TEST_ENV_2"); + }); +}); + +describe("validateHost", () => { + it("accepts an unset host (SDK auth chain takes over)", () => { + expect(validateHost(undefined)).toBeNull(); + expect(validateHost("")).toBeNull(); + }); + + it("accepts a real workspace URL", () => { + expect(validateHost("https://foo.cloud.databricks.com")).toBeNull(); + expect(validateHost("https://adb-123.4.azuredatabricks.net")).toBeNull(); + }); + + it("rejects the unfilled template placeholder https://...", () => { + expect(validateHost("https://...")).toMatch(/placeholder/i); + }); + + it("rejects a non-URL value", () => { + expect(validateHost("not-a-url")).toMatch(/not a valid URL/i); + }); + + it("rejects a non-http(s) scheme", () => { + expect(validateHost("ftp://foo.databricks.com")).toMatch(/http/i); + }); +}); + +describe("checkAuth", () => { + const savedHost = process.env.DATABRICKS_HOST; + + afterEach(() => { + mockGetServiceClient.mockReset(); + if (savedHost === undefined) delete process.env.DATABRICKS_HOST; + else process.env.DATABRICKS_HOST = savedHost; + }); + + it("ok + returns the client when currentUser.me() succeeds", async () => { + process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; + const client = { currentUser: { me: async () => ({ userName: "sp-1" }) } }; + mockGetServiceClient.mockResolvedValue({ client }); + + const { result, client: returned } = await checkAuth({}); + expect(result.status).toBe("ok"); + expect(result.code).toBe("AUTH_OK"); + expect(result.detail).toContain("sp-1"); + expect(returned).toBe(client); // handed to the live layers + }); + + it("errors HOST_INVALID before touching the SDK", async () => { + process.env.DATABRICKS_HOST = "https://..."; + const { result, client } = await checkAuth({}); + expect(result.status).toBe("error"); + expect(result.code).toBe("HOST_INVALID"); + expect(client).toBeUndefined(); + expect(mockGetServiceClient).not.toHaveBeenCalled(); + }); + + it("errors SDK_NOT_INSTALLED when the bridge reports the SDK is absent", async () => { + process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; + const { SdkNotInstalledError } = await import("./databricks-client"); + mockGetServiceClient.mockRejectedValue(new SdkNotInstalledError()); + + const { result } = await checkAuth({}); + expect(result.status).toBe("error"); + expect(result.code).toBe("SDK_NOT_INSTALLED"); + }); + + it("errors AUTH_FAILED on any other auth error", async () => { + process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; + mockGetServiceClient.mockRejectedValue(new Error("something odd")); + + const { result } = await checkAuth({}); + expect(result.status).toBe("error"); + expect(result.code).toBe("AUTH_FAILED"); + expect(result.detail).toContain("something odd"); + expect(result.hint).toBeUndefined(); // unrecognized failure → no guess + }); + + it("hints to reauthenticate on an expired/failed CLI token (real SDK message)", async () => { + process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; + mockGetServiceClient.mockRejectedValue( + new Error( + "default auth: databricks-cli: cannot get access token: Command failed: databricks auth token", + ), + ); + + const { result } = await checkAuth({ profile: "prod" }); + expect(result.hint).toMatch(/expired|reauthenticate/i); + expect(result.hint).toContain("databricks auth login --profile prod"); + }); + + it("hints about a missing profile (real SDK message)", async () => { + process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; + mockGetServiceClient.mockRejectedValue( + new Error("resolve: ~/.databrickscfg has no nope profile configured"), + ); + + const { result } = await checkAuth({}); + expect(result.hint).toMatch(/Profile not found/i); + }); +}); diff --git a/packages/shared/src/cli/commands/doctor/checks.ts b/packages/shared/src/cli/commands/doctor/checks.ts new file mode 100644 index 000000000..815f4e0e0 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/checks.ts @@ -0,0 +1,197 @@ +/** + * The `auth` and `config` layer checks. (The `existence` layer's per-type + * probes live in `checks-existence.ts`.) + */ + +import { runExistenceProbe } from "./checks-existence"; +import { getServiceClient, SdkNotInstalledError } from "./databricks-client"; +import type { + AuthCheckResult, + DoctorOptions, + LayerResult, + ResourceTarget, +} from "./types"; + +/** The auth result plus the resolved client (present only on success), which + * the orchestrator hands to the live layers so they don't rebuild it. */ +export interface AuthOutcome { + result: AuthCheckResult; + client?: unknown; +} + +interface CurrentUserClient { + currentUser: { + me: () => Promise<{ id?: string; userName?: string }>; + }; +} + +/** + * Validates `DATABRICKS_HOST` before we hand it to the SDK, so an unfilled + * placeholder (e.g. `https://...`) gets a clear message instead of the SDK's + * opaque "cannot configure default credentials" error. Returns an error + * message, or null when the host is acceptable (or unset — then the SDK's own + * auth chain takes over). + * + * @internal exported for unit testing. + */ +export function validateHost(host: string | undefined): string | null { + if (host === undefined || host.trim().length === 0) return null; + + let url: URL; + try { + url = new URL(host); + } catch { + return `DATABRICKS_HOST is not a valid URL: "${host}"`; + } + + if (url.protocol !== "https:" && url.protocol !== "http:") { + return `DATABRICKS_HOST must be an http(s) URL: "${host}"`; + } + + // Placeholders like "https://..." parse as a URL but have a hostname with no + // real (dotted, alphanumeric) label. + const hostname = url.hostname; + const hasRealLabel = /[a-z0-9]/i.test(hostname) && hostname.includes("."); + if (!hasRealLabel || /^[.\-_]+$/.test(hostname)) { + return `DATABRICKS_HOST looks like an unfilled placeholder: "${host}"`; + } + + return null; +} + +/** + * Layer: auth. Runs once, app-wide; a failure short-circuits every resource's + * existence check (they all need the client). + */ +export async function checkAuth(options: DoctorOptions): Promise { + const hostError = validateHost(process.env.DATABRICKS_HOST); + if (hostError) { + return { + result: { + status: "error", + code: "HOST_INVALID", + detail: hostError, + host: process.env.DATABRICKS_HOST, + profile: options.profile, + }, + }; + } + + try { + const { client } = await getServiceClient(options.profile); + const me = await (client as CurrentUserClient).currentUser.me(); + const who = me.userName ?? me.id ?? "unknown"; + + return { + client, + result: { + status: "ok", + code: "AUTH_OK", + detail: `authenticated as ${who}`, + host: process.env.DATABRICKS_HOST, + profile: options.profile, + }, + }; + } catch (err) { + if (err instanceof SdkNotInstalledError) { + return { + result: { + status: "error", + code: "SDK_NOT_INSTALLED", + detail: err.message, + profile: options.profile, + }, + }; + } + const detail = err instanceof Error ? err.message : String(err); + return { + result: { + status: "error", + code: "AUTH_FAILED", + detail: `failed to authenticate to the workspace: ${detail}`, + hint: authFailureHint(detail, options.profile), + host: process.env.DATABRICKS_HOST, + profile: options.profile, + }, + }; + } +} + +/** + * Infers guidance for the common auth failures, which the SDK surfaces as + * opaque strings. Patterns are matched against real SDK messages (a missing + * profile, an expired CLI token, and no credentials at all), most specific + * first. Returns the command that fixes each, or undefined for the unknown. + */ +function authFailureHint( + message: string, + profile?: string, +): string | undefined { + const loginCmd = profile + ? `databricks auth login --profile ${profile}` + : "databricks auth login"; + + // "resolve: ~/.databrickscfg has no profile configured" + if ( + /has no .* profile configured|profile .* (does not exist|not found)/i.test( + message, + ) + ) { + return `Profile not found in ~/.databrickscfg. Run \`${loginCmd}\`, or pass an existing profile via --profile.`; + } + // Expired/failed CLI token: "cannot get access token", "databricks auth + // token", "refresh token is invalid", "reauthenticate". + if ( + /cannot get access token|refresh token|reauthenticate|databricks auth token|token .*expired/i.test( + message, + ) + ) { + return `Your login has expired or the token could not be fetched. Run \`${loginCmd}\` to reauthenticate.`; + } + // No credentials resolved by any auth method. + if ( + /cannot configure default credentials|default auth|no .*credentials/i.test( + message, + ) + ) { + return `No credentials found. Run \`${loginCmd}\`, or set a profile via --profile / DATABRICKS_CONFIG_PROFILE.`; + } + return undefined; +} + +/** + * Layer: config. Offline check that each declared env var is present. Presence + * only — whether a set value points at a real resource is the existence layer's + * job, so this never guesses at placeholder values. + */ +export async function checkConfig( + target: ResourceTarget, +): Promise { + const missing: string[] = []; + + for (const envVar of target.envVars) { + const value = process.env[envVar]; + if (value === undefined || value.trim().length === 0) { + missing.push(envVar); + } + } + + if (missing.length > 0) { + return { + layer: "config", + status: target.required ? "error" : "warn", + code: target.required ? "ENV_MISSING" : "ENV_MISSING_OPTIONAL", + detail: `${target.required ? "required" : "optional"} resource is missing env var(s): ${missing.join(", ")}`, + }; + } + + return { layer: "config", status: "ok" }; +} + +/** Layer: existence. Dispatches to the per-type probe in `checks-existence.ts`. */ +export async function checkExistence( + target: ResourceTarget, + client: unknown, +): Promise { + return runExistenceProbe(client, target); +} diff --git a/packages/shared/src/cli/commands/doctor/databricks-client.ts b/packages/shared/src/cli/commands/doctor/databricks-client.ts new file mode 100644 index 000000000..8084f9654 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/databricks-client.ts @@ -0,0 +1,96 @@ +/** + * The single seam where `appkit doctor` crosses into the Databricks SDK. + * + * The CLI lives in the SDK-free `shared` package, so it reaches the SDK via a + * runtime `import(...)` — keeping `shared` free of the dependency and degrading + * gracefully when it's absent. All Databricks-touching check code goes through + * this module; nothing else in the doctor command imports the SDK. + */ + +/** Raised when `@databricks/sdk-experimental` is not resolvable at runtime. */ +export class SdkNotInstalledError extends Error { + constructor() { + super( + "The 'doctor' command requires the Databricks SDK (a dependency of @databricks/appkit). Please install @databricks/appkit to run connection checks.", + ); + this.name = "SdkNotInstalledError"; + } +} + +export interface ServiceClientHandle { + /** WorkspaceClient, typed as unknown to keep `shared` SDK-free. */ + client: unknown; +} + +function isModuleNotFound(err: unknown): boolean { + return ( + err instanceof Error && + (err.message.includes("Cannot find module") || + err.message.includes("Cannot find package") || + (err as NodeJS.ErrnoException).code === "ERR_MODULE_NOT_FOUND") + ); +} + +/** Env var the Databricks SDK's unified auth reads to select a CLI profile. */ +const CONFIG_PROFILE_ENV = "DATABRICKS_CONFIG_PROFILE"; + +/** Constructs a `WorkspaceClient` via the SDK's unified-auth chain. `profile` + * is applied through the env var the SDK reads. */ +export async function getServiceClient( + profile?: string, +): Promise { + if (profile) { + // Permanent for this process — fine for a one-shot CLI, but note it if this + // is ever reused in a test or long-lived context. + process.env[CONFIG_PROFILE_ENV] = profile; + } + + let sdk: typeof import("@databricks/sdk-experimental"); + try { + sdk = await import("@databricks/sdk-experimental"); + } catch (err) { + if (isModuleNotFound(err)) { + throw new SdkNotInstalledError(); + } + throw err; + } + + const client = new sdk.WorkspaceClient({}); + return { client }; +} + +/** Raised when `@databricks/appkit` (needed for the Lakebase probe) is absent. */ +export class AppkitNotInstalledError extends Error { + constructor() { + super( + "The Lakebase connection check requires @databricks/appkit. Please install it to run this check.", + ); + this.name = "AppkitNotInstalledError"; + } +} + +/** Minimal `pg.Pool`-shaped handle. The caller owns it and must call `end()`. */ +export interface LakebasePoolHandle { + query: (sql: string) => Promise; + end: () => Promise; +} + +/** Builds a Lakebase pool via `createLakebasePool`. Connection settings + * (`PGHOST`, `LAKEBASE_ENDPOINT`, …) come from the environment; we inject only + * the resolved workspace client for OAuth token minting. */ +export async function getLakebasePool( + client: unknown, +): Promise { + let appkit: typeof import("@databricks/appkit"); + try { + appkit = await import("@databricks/appkit"); + } catch (err) { + if (isModuleNotFound(err)) { + throw new AppkitNotInstalledError(); + } + throw err; + } + + const pool = appkit.createLakebasePool({ workspaceClient: client as never }); + return pool as unknown as LakebasePoolHandle; +} diff --git a/packages/shared/src/cli/commands/doctor/index.ts b/packages/shared/src/cli/commands/doctor/index.ts new file mode 100644 index 000000000..dfaf1dcea --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/index.ts @@ -0,0 +1,43 @@ +import process from "node:process"; +import { Command } from "commander"; +import { exitCodeFor, printReport, printReportJson } from "./report"; +import { runDoctor } from "./run"; +import type { DoctorOptions } from "./types"; + +async function runDoctorCommand(options: DoctorOptions): Promise { + const report = await runDoctor(options); + + if (options.json) { + printReportJson(report); + } else { + printReport(report); + } + + process.exit(exitCodeFor(report)); +} + +export const doctorCommand = new Command("doctor") + .description( + "Diagnose an AppKit app's Databricks resources: authentication, config, and resource existence/reachability", + ) + .option( + "-m, --manifest ", + "Path to the resolved template manifest", + "appkit.plugins.json", + ) + .option("-p, --profile ", "Databricks CLI profile to authenticate with") + .option("--json", "Output the diagnostic report as JSON") + .addHelpText( + "after", + ` +Examples: + $ appkit doctor + $ appkit doctor --profile my-profile + $ appkit doctor --json`, + ) + .action((opts: DoctorOptions) => + runDoctorCommand(opts).catch((err) => { + console.error(err); + process.exit(1); + }), + ); diff --git a/packages/shared/src/cli/commands/doctor/report.test.ts b/packages/shared/src/cli/commands/doctor/report.test.ts new file mode 100644 index 000000000..7cc666ef8 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/report.test.ts @@ -0,0 +1,132 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { exitCodeFor, printReport, printReportJson } from "./report"; +import type { DoctorReport, ResourceCheckResult } from "./types"; + +function report(overrides: Partial = {}): DoctorReport { + return { + auth: { status: "ok" }, + resources: [], + summary: { ok: 0, warn: 0, error: 0, skipped: 0 }, + ...overrides, + }; +} + +/** Runs `fn` with console.log captured; returns the printed lines. */ +function capture(fn: () => void): string[] { + const lines: string[] = []; + const spy = vi.spyOn(console, "log").mockImplementation((msg?: unknown) => { + lines.push(String(msg)); + }); + try { + fn(); + return lines; + } finally { + spy.mockRestore(); + } +} + +function res( + status: ResourceCheckResult["status"], + type: string, +): ResourceCheckResult { + return { + target: { + type, + resourceKey: type, + alias: type, + plugin: "p", + requiredPermission: "X", + required: true, + envVars: [], + fieldValues: {}, + }, + status, + layers: [], + }; +} + +describe("printReport ordering", () => { + afterEach(() => vi.restoreAllMocks()); + + it("prints resources most-severe first (error, warn, skipped, ok)", () => { + const lines: string[] = []; + vi.spyOn(console, "log").mockImplementation((msg?: unknown) => { + lines.push(String(msg)); + }); + + // Deliberately supplied in non-severity order. + const report: DoctorReport = { + auth: { status: "ok" }, + resources: [ + res("warn", "warned"), + res("ok", "okay"), + res("error", "errored"), + res("skipped", "skip"), + ], + summary: { ok: 1, warn: 1, error: 1, skipped: 1 }, + }; + + printReport(report); + + const order = ["errored", "warned", "skip", "okay"].map((t) => + lines.findIndex((l) => l.includes(t)), + ); + // Every row was printed and appears in strictly increasing (severity) order. + expect(order.every((i) => i >= 0)).toBe(true); + expect(order).toEqual([...order].sort((a, b) => a - b)); + }); + + it("summary shows only non-zero categories", () => { + const lines = capture(() => + printReport( + report({ summary: { ok: 3, warn: 0, error: 0, skipped: 0 } }), + ), + ); + expect(lines.some((l) => l === "3 ok")).toBe(true); + expect(lines.some((l) => /warning|error|skipped/.test(l))).toBe(false); + }); + + it("folds an auth error into the summary error count", () => { + const lines = capture(() => + printReport( + report({ + auth: { status: "error", detail: "bad creds" }, + summary: { ok: 0, warn: 0, error: 0, skipped: 0 }, + }), + ), + ); + // Auth failure counts as an error even though no resource errored. + expect(lines.some((l) => l.startsWith("1 error"))).toBe(true); + expect(lines.some((l) => /Fix authentication first/.test(l))).toBe(true); + }); +}); + +describe("exitCodeFor", () => { + it("is 0 when auth ok and no resource errors", () => { + expect( + exitCodeFor( + report({ summary: { ok: 2, warn: 1, error: 0, skipped: 1 } }), + ), + ).toBe(0); + }); + + it("is 1 when auth failed", () => { + expect(exitCodeFor(report({ auth: { status: "error" } }))).toBe(1); + }); + + it("is 1 when any resource errored", () => { + expect( + exitCodeFor( + report({ summary: { ok: 0, warn: 0, error: 1, skipped: 0 } }), + ), + ).toBe(1); + }); +}); + +describe("printReportJson", () => { + it("emits the full report as parseable JSON", () => { + const r = report({ summary: { ok: 1, warn: 0, error: 0, skipped: 0 } }); + const lines = capture(() => printReportJson(r)); + expect(JSON.parse(lines.join("\n"))).toEqual(r); + }); +}); diff --git a/packages/shared/src/cli/commands/doctor/report.ts b/packages/shared/src/cli/commands/doctor/report.ts new file mode 100644 index 000000000..65cadc3da --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/report.ts @@ -0,0 +1,101 @@ +/** + * Rendering for `appkit doctor` — turns a {@link DoctorReport} into either a + * human-readable console report or JSON, matching the `--json` convention used + * by `plugin list` / `plugin validate`. + */ + +import type { CheckStatus, DoctorReport } from "./types"; + +function glyph(status: CheckStatus): string { + switch (status) { + case "ok": + return "✓"; + case "warn": + return "⚠"; + case "error": + return "✗"; + default: + return "•"; + } +} + +/** Row display order: most severe first (stable within a status). */ +const DISPLAY_ORDER: Record = { + error: 0, + warn: 1, + skipped: 2, + ok: 3, +}; + +/** Shows only the non-zero categories. */ +function summaryLine(counts: { + ok: number; + warn: number; + error: number; + skipped: number; +}): string { + const parts: string[] = []; + if (counts.error) + parts.push(`${counts.error} error${counts.error > 1 ? "s" : ""}`); + if (counts.warn) + parts.push(`${counts.warn} warning${counts.warn > 1 ? "s" : ""}`); + if (counts.ok) parts.push(`${counts.ok} ok`); + if (counts.skipped) parts.push(`${counts.skipped} skipped`); + return parts.length > 0 ? parts.join(", ") : "nothing to check"; +} + +export function printReport(report: DoctorReport): void { + const { auth, resources, summary } = report; + + console.log(""); + console.log("Databricks AppKit — connection check"); + if (auth.profile) console.log(` profile ${auth.profile}`); + if (auth.host) console.log(` workspace ${auth.host}`); + console.log(""); + + console.log(`${glyph(auth.status)} Auth ${auth.detail ?? auth.status}`); + if (auth.hint) console.log(` hint: ${auth.hint}`); + console.log(""); + + if (resources.length === 0) { + console.log("No resources declared."); + } else { + console.log("Resources"); + const ordered = [...resources].sort( + (a, b) => DISPLAY_ORDER[a.status] - DISPLAY_ORDER[b.status], + ); + for (const r of ordered) { + const { target } = r; + // The `plugin · type` attribution only earns its place when a row needs + // fixing (tells you where to go); on a pass the alias alone is cleaner. + const suffix = + r.status !== "ok" ? ` ${target.plugin} · ${target.type}` : ""; + console.log(` ${glyph(r.status)} ${target.alias}${suffix}`); + for (const layer of r.layers) { + if (layer.status === "ok") continue; + if (layer.detail) console.log(` ↳ ${layer.detail}`); + if (layer.hint) console.log(` hint: ${layer.hint}`); + } + } + } + console.log(""); + + // Fold auth failure into the counts so it doesn't read as "0 errors" just + // because resources weren't probed. + const authError = auth.status === "error" ? 1 : 0; + console.log(summaryLine({ ...summary, error: summary.error + authError })); + if (authError) { + console.log("Fix authentication first, then re-run to check resources."); + } + console.log(""); +} + +export function printReportJson(report: DoctorReport): void { + console.log(JSON.stringify(report, null, 2)); +} + +/** Non-zero if auth or any resource errored, so `appkit doctor` can gate CI. */ +export function exitCodeFor(report: DoctorReport): number { + if (report.auth.status === "error") return 1; + return report.summary.error > 0 ? 1 : 0; +} diff --git a/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts b/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts new file mode 100644 index 000000000..aee758e84 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts @@ -0,0 +1,180 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { + resolveTargetsFromCwd, + targetsFromManifestFile, +} from "./resolve-targets"; + +function makeTempDir(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), "doctor-targets-")); +} + +function cleanDir(dir: string): void { + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { + // best effort + } +} + +const MANIFEST = { + version: "2.0", + plugins: { + analytics: { + resources: { + required: [ + { + type: "sql_warehouse", + resourceKey: "sql-warehouse", + alias: "SQL Warehouse", + permission: "CAN_USE", + fields: { id: { env: "DATABRICKS_WAREHOUSE_ID" } }, + }, + ], + optional: [], + }, + }, + agents: { + resources: { + required: [], + optional: [ + { + type: "serving_endpoint", + resourceKey: "serving", + alias: "Chat model", + permission: "CAN_QUERY", + fields: { name: { env: "DATABRICKS_SERVING_ENDPOINT_NAME" } }, + }, + ], + }, + }, + server: { resources: { required: [], optional: [] } }, + }, +}; + +describe("targetsFromManifestFile", () => { + const dirs: string[] = []; + afterEach(() => { + for (const d of dirs) cleanDir(d); + dirs.length = 0; + }); + + function writeManifest(obj: unknown): string { + const dir = makeTempDir(); + dirs.push(dir); + const p = path.join(dir, "appkit.plugins.json"); + fs.writeFileSync(p, JSON.stringify(obj)); + return p; + } + + it("flattens required and optional resources across plugins", () => { + const targets = targetsFromManifestFile(writeManifest(MANIFEST)); + expect(targets).toHaveLength(2); + + const warehouse = targets.find((t) => t.type === "sql_warehouse"); + expect(warehouse).toMatchObject({ + type: "sql_warehouse", + resourceKey: "sql-warehouse", + alias: "SQL Warehouse", + plugin: "analytics", + requiredPermission: "CAN_USE", + required: true, + envVars: ["DATABRICKS_WAREHOUSE_ID"], + }); + + const serving = targets.find((t) => t.type === "serving_endpoint"); + expect(serving).toMatchObject({ + plugin: "agents", + required: false, + envVars: ["DATABRICKS_SERVING_ENDPOINT_NAME"], + }); + }); + + it("excludes value-default and platform-injected fields from envVars (regression: bug #3)", () => { + const targets = targetsFromManifestFile( + writeManifest({ + version: "2.0", + plugins: { + lakebase: { + resources: { + required: [ + { + type: "postgres", + resourceKey: "pg", + alias: "Postgres", + permission: "CAN_CONNECT_AND_CREATE", + fields: { + endpointPath: { env: "LAKEBASE_ENDPOINT", origin: "cli" }, + host: { + env: "PGHOST", + localOnly: true, + origin: "platform", + }, + port: { + env: "PGPORT", + value: "5432", + localOnly: true, + origin: "platform", + }, + sslmode: { + env: "PGSSLMODE", + value: "require", + localOnly: true, + origin: "platform", + }, + }, + }, + ], + optional: [], + }, + }, + }, + }), + ); + const pg = targets.find((t) => t.type === "postgres"); + // Only the user-supplied endpoint is presence-checked; value-default and + // platform-injected fields must not be flagged as missing. + expect(pg?.envVars).toEqual(["LAKEBASE_ENDPOINT"]); + }); + + it("returns an empty list for a manifest with no resources", () => { + const targets = targetsFromManifestFile( + writeManifest({ version: "2.0", plugins: { server: {} } }), + ); + expect(targets).toEqual([]); + }); + + it("throws on invalid JSON", () => { + const dir = makeTempDir(); + dirs.push(dir); + const p = path.join(dir, "appkit.plugins.json"); + fs.writeFileSync(p, "{ not json"); + expect(() => targetsFromManifestFile(p)).toThrow(/Failed to parse/); + }); +}); + +describe("resolveTargetsFromCwd", () => { + const dirs: string[] = []; + afterEach(() => { + for (const d of dirs) cleanDir(d); + dirs.length = 0; + }); + + it("returns an empty list when no manifest is present", () => { + const dir = makeTempDir(); + dirs.push(dir); + expect(resolveTargetsFromCwd(dir)).toEqual([]); + }); + + it("reads the manifest from the given cwd", () => { + const dir = makeTempDir(); + dirs.push(dir); + fs.writeFileSync( + path.join(dir, "appkit.plugins.json"), + JSON.stringify(MANIFEST), + ); + expect(resolveTargetsFromCwd(dir)).toHaveLength(2); + }); +}); diff --git a/packages/shared/src/cli/commands/doctor/resolve-targets.ts b/packages/shared/src/cli/commands/doctor/resolve-targets.ts new file mode 100644 index 000000000..a4886d383 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/resolve-targets.ts @@ -0,0 +1,140 @@ +/** + * Reads the app's resolved template manifest (`appkit.plugins.json`) and + * flattens each plugin's declared resources into the flat {@link ResourceTarget} + * shape the checks consume. Offline and SDK-free — parses the same file + * `appkit plugin list` reads. + */ + +import fs from "node:fs"; +import path from "node:path"; +import type { ResourceTarget } from "./types"; + +export const DEFAULT_MANIFEST_FILE = "appkit.plugins.json"; + +interface ManifestField { + env?: string; + /** Static default value baked into the manifest. */ + value?: string; + /** Computed origin: how the value is supplied (see the manifest schema). */ + origin?: "user" | "platform" | "static" | "cli"; + /** Only generated into the local .env; the platform injects it at deploy. */ + localOnly?: boolean; +} + +interface ManifestResource { + type: string; + resourceKey: string; + alias?: string; + permission: string; + fields?: Record; +} + +interface ManifestPlugin { + resources?: { + required?: ManifestResource[]; + optional?: ManifestResource[]; + }; +} + +interface TemplateManifest { + plugins?: Record; +} + +/** A field's env var is only the developer's to supply when it has no static + * default and isn't platform-injected at deploy time (origin "platform" / + * localOnly). Those the config layer must not flag as missing. */ +function isUserSuppliedEnv(field: ManifestField): boolean { + if (field.value !== undefined) return false; + if (field.origin === "platform" || field.origin === "static") return false; + if (field.localOnly) return false; + return true; +} + +/** Env vars the config layer should presence-check (i.e. user-supplied ones). */ +function envVarsOf(resource: ManifestResource): string[] { + const fields = resource.fields ?? {}; + const envs: string[] = []; + for (const field of Object.values(fields)) { + if (field?.env && isUserSuppliedEnv(field)) envs.push(field.env); + } + return envs; +} + +/** Resolves each field's value from the environment, keyed by manifest field + * name. Unset/empty fields are omitted so probes can tell provided from absent. */ +function fieldValuesOf(resource: ManifestResource): Record { + const fields = resource.fields ?? {}; + const values: Record = {}; + for (const [fieldName, field] of Object.entries(fields)) { + const env = field?.env; + if (!env) continue; + const value = process.env[env]; + if (value !== undefined && value.trim().length > 0) { + values[fieldName] = value; + } + } + return values; +} + +function toTarget( + plugin: string, + resource: ManifestResource, + required: boolean, +): ResourceTarget { + return { + type: resource.type, + resourceKey: resource.resourceKey, + alias: resource.alias ?? resource.resourceKey, + plugin, + requiredPermission: resource.permission, + required, + envVars: envVarsOf(resource), + fieldValues: fieldValuesOf(resource), + }; +} + +/** @throws if the file cannot be read or parsed. */ +export function targetsFromManifestFile( + manifestPath: string, +): ResourceTarget[] { + let raw: string; + try { + raw = fs.readFileSync(manifestPath, "utf-8"); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to read manifest file ${manifestPath}: ${msg}`); + } + + let data: TemplateManifest; + try { + data = JSON.parse(raw) as TemplateManifest; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to parse manifest file ${manifestPath}: ${msg}`); + } + + const targets: ResourceTarget[] = []; + for (const [pluginName, plugin] of Object.entries(data.plugins ?? {})) { + const resources = plugin.resources ?? {}; + for (const resource of resources.required ?? []) { + targets.push(toTarget(pluginName, resource, true)); + } + for (const resource of resources.optional ?? []) { + targets.push(toTarget(pluginName, resource, false)); + } + } + return targets; +} + +/** Returns an empty list if the manifest is absent — an app may legitimately + * declare no resources. */ +export function resolveTargetsFromCwd( + cwd: string = process.cwd(), + manifestFile: string = DEFAULT_MANIFEST_FILE, +): ResourceTarget[] { + const manifestPath = path.resolve(cwd, manifestFile); + if (!fs.existsSync(manifestPath)) { + return []; + } + return targetsFromManifestFile(manifestPath); +} diff --git a/packages/shared/src/cli/commands/doctor/run.test.ts b/packages/shared/src/cli/commands/doctor/run.test.ts new file mode 100644 index 000000000..5de53668d --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/run.test.ts @@ -0,0 +1,84 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { runDoctor } from "./run"; + +/** + * Tests for the doctor orchestrator. The auth layer reaches the Databricks SDK, + * so these mock the bridge to keep the orchestration assertions deterministic + * and offline. + */ +vi.mock("./databricks-client", () => ({ + SdkNotInstalledError: class SdkNotInstalledError extends Error {}, + // Default: a client whose currentUser.me() succeeds. + getServiceClient: vi.fn(async () => ({ + client: { currentUser: { me: async () => ({ userName: "app-sp" }) } }, + })), +})); + +describe("runDoctor", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("reports ok auth by default", async () => { + const report = await runDoctor({}); + expect(report.auth.status).toBe("ok"); + expect(report.auth.code).toBe("AUTH_OK"); + }); + + it("resolves no resources when cwd has no manifest, with an empty summary", async () => { + // Point cwd at a dir guaranteed to have no appkit.plugins.json. + const spy = vi.spyOn(process, "cwd").mockReturnValue("/nonexistent-doctor"); + const report = await runDoctor({}); + expect(report.resources).toEqual([]); + expect(report.summary).toEqual({ ok: 0, warn: 0, error: 0, skipped: 0 }); + spy.mockRestore(); + }); + + it("still resolves and offline-checks resources when auth fails", async () => { + const { getServiceClient } = await import("./databricks-client"); + vi.mocked(getServiceClient).mockRejectedValueOnce(new Error("no creds")); + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "doctor-run-")); + fs.writeFileSync( + path.join(dir, "appkit.plugins.json"), + JSON.stringify({ + version: "2.0", + plugins: { + analytics: { + resources: { + required: [ + { + type: "sql_warehouse", + resourceKey: "sql-warehouse", + alias: "SQL Warehouse", + permission: "CAN_USE", + fields: { id: { env: "DOCTOR_RUN_MISSING_ENV" } }, + }, + ], + optional: [], + }, + }, + }, + }), + ); + const spy = vi.spyOn(process, "cwd").mockReturnValue(dir); + delete process.env.DOCTOR_RUN_MISSING_ENV; + + const report = await runDoctor({}); + + expect(report.auth.status).toBe("error"); + // Resource was still resolved and offline-checked despite auth failure. + expect(report.resources).toHaveLength(1); + expect(report.resources[0].status).toBe("error"); + const configLayer = report.resources[0].layers.find( + (l) => l.layer === "config", + ); + expect(configLayer?.status).toBe("error"); + + spy.mockRestore(); + fs.rmSync(dir, { recursive: true, force: true }); + }); +}); diff --git a/packages/shared/src/cli/commands/doctor/run.ts b/packages/shared/src/cli/commands/doctor/run.ts new file mode 100644 index 000000000..b65c1da9c --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/run.ts @@ -0,0 +1,88 @@ +/** + * Orchestration for `appkit doctor`. + * + * Resolves the resources the app declares, runs the app-wide auth check once, + * then per resource runs the offline `config` layer followed by the live + * `existence` layer. Rolls everything up into a {@link DoctorReport}. + */ + +import { checkAuth, checkConfig, checkExistence } from "./checks"; +import { resolveTargetsFromCwd } from "./resolve-targets"; +import type { + CheckStatus, + DoctorOptions, + DoctorReport, + LayerResult, + ResourceCheckResult, + ResourceTarget, +} from "./types"; + +const STATUS_SEVERITY: Record = { + ok: 0, + skipped: 1, + warn: 2, + error: 3, +}; + +function worst(a: CheckStatus, b: CheckStatus): CheckStatus { + return STATUS_SEVERITY[b] > STATUS_SEVERITY[a] ? b : a; +} + +/** Returns an empty list when no manifest is present — an app may legitimately + * declare no resources, which doctor reports rather than treating as an error. */ +export async function resolveTargets( + options: DoctorOptions, +): Promise { + return resolveTargetsFromCwd(process.cwd(), options.manifest); +} + +async function checkResource( + target: ResourceTarget, + // undefined = auth failed, so the live existence layer is skipped. + client: unknown | undefined, +): Promise { + const layers: LayerResult[] = []; + let rolled: CheckStatus = "ok"; + + const configResult = await checkConfig(target); + layers.push(configResult); + rolled = worst(rolled, configResult.status); + // A hard config failure (missing id) makes the existence probe meaningless. + if (configResult.status === "error") { + return { target, status: rolled, layers }; + } + + if (client === undefined) { + layers.push({ + layer: "existence", + status: "skipped", + code: "AUTH_UNAVAILABLE", + detail: "skipped because workspace authentication failed", + }); + rolled = worst(rolled, "skipped"); + } else { + const result = await checkExistence(target, client); + layers.push(result); + rolled = worst(rolled, result.status); + } + + return { target, status: rolled, layers }; +} + +export async function runDoctor(options: DoctorOptions): Promise { + const { result: auth, client } = await checkAuth(options); + + const summary = { ok: 0, warn: 0, error: 0, skipped: 0 }; + const resources: ResourceCheckResult[] = []; + + // Resources are resolved and config-checked even when auth failed, so a bad + // connection still surfaces config problems instead of hiding them all. + const targets = await resolveTargets(options); + for (const target of targets) { + const result = await checkResource(target, client); + resources.push(result); + summary[result.status] += 1; + } + + return { auth, resources, summary }; +} diff --git a/packages/shared/src/cli/commands/doctor/types.ts b/packages/shared/src/cli/commands/doctor/types.ts new file mode 100644 index 000000000..3f70d92f6 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/types.ts @@ -0,0 +1,63 @@ +/** Type definitions for the `appkit doctor` command. */ + +/** Layers run per resource, in order; a hard failure short-circuits the rest. */ +export type CheckLayer = "auth" | "config" | "existence"; + +export type CheckStatus = "ok" | "warn" | "error" | "skipped"; + +export interface LayerResult { + layer: CheckLayer; + status: CheckStatus; + /** The raw finding (what happened). */ + detail?: string; + /** Optional inferred guidance, rendered on its own line below `detail`. */ + hint?: string; + /** Machine-readable code for `--json` consumers (e.g. `NOT_FOUND`). */ + code?: string; +} + +export interface ResourceTarget { + type: string; + resourceKey: string; + /** Human-readable label. */ + alias: string; + plugin: string; + /** Declared permission level; shown for context, not checked. */ + requiredPermission: string; + /** Mandatory (vs optional) for the app. */ + required: boolean; + envVars: string[]; + /** Field values resolved from `process.env`, keyed by manifest field name + * (e.g. `id`, `name`). Unset fields are omitted. */ + fieldValues: Record; +} + +export interface ResourceCheckResult { + target: ResourceTarget; + /** Worst status across all layers run for this resource. */ + status: CheckStatus; + layers: LayerResult[]; +} + +export interface AuthCheckResult { + status: CheckStatus; + detail?: string; + /** Optional inferred guidance, rendered on its own line below `detail`. */ + hint?: string; + code?: string; + host?: string; + profile?: string; +} + +export interface DoctorReport { + auth: AuthCheckResult; + resources: ResourceCheckResult[]; + summary: { ok: number; warn: number; error: number; skipped: number }; +} + +export interface DoctorOptions { + /** Path to the resolved template manifest (defaults to appkit.plugins.json). */ + manifest?: string; + profile?: string; + json?: boolean; +} diff --git a/packages/shared/src/cli/index.ts b/packages/shared/src/cli/index.ts index aa60157c8..53398b0e1 100644 --- a/packages/shared/src/cli/index.ts +++ b/packages/shared/src/cli/index.ts @@ -6,6 +6,7 @@ import { fileURLToPath } from "node:url"; import { Command } from "commander"; import { codemodCommand } from "./commands/codemod/index.js"; import { docsCommand } from "./commands/docs.js"; +import { doctorCommand } from "./commands/doctor/index.js"; import { generateTypesCommand } from "./commands/generate-types.js"; import { lintCommand } from "./commands/lint.js"; import { pluginCommand } from "./commands/plugin/index.js"; @@ -28,5 +29,6 @@ cmd.addCommand(lintCommand); cmd.addCommand(docsCommand); cmd.addCommand(pluginCommand); cmd.addCommand(codemodCommand); +cmd.addCommand(doctorCommand); await cmd.parseAsync(); From 99ec29467e29204f0e329fa5cbae8167de61d943 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Mon, 27 Jul 2026 14:17:34 +0200 Subject: [PATCH 02/14] refactor(cli): simplify appkit doctor command Run existence probes concurrently via Promise.all (independent network reads; input order preserved), cache DATABRICKS_HOST in a local, and drop a redundant `unknown | undefined` parameter type. Signed-off-by: Galymzhan --- .../commands/doctor/checks-existence.test.ts | 6 ++-- .../cli/commands/doctor/checks-existence.ts | 28 +++++++----------- .../shared/src/cli/commands/doctor/checks.ts | 29 ++++++++----------- .../cli/commands/doctor/databricks-client.ts | 13 ++------- .../shared/src/cli/commands/doctor/report.ts | 6 ++-- .../cli/commands/doctor/resolve-targets.ts | 12 +++----- .../shared/src/cli/commands/doctor/run.ts | 22 ++++++-------- .../shared/src/cli/commands/doctor/types.ts | 7 ++--- 8 files changed, 45 insertions(+), 78 deletions(-) diff --git a/packages/shared/src/cli/commands/doctor/checks-existence.test.ts b/packages/shared/src/cli/commands/doctor/checks-existence.test.ts index f621ef5b7..154d418f4 100644 --- a/packages/shared/src/cli/commands/doctor/checks-existence.test.ts +++ b/packages/shared/src/cli/commands/doctor/checks-existence.test.ts @@ -211,9 +211,9 @@ describe("runExistenceProbe — postgres (Lakebase)", () => { }); }); -// These use the REAL first-party manifest field keys (e.g. vector-search's -// camelCase `indexName`) — the case that previously slipped through as a silent -// skip. Each supported probe gets a happy path + a missing-field skip. +// These use the real first-party manifest field keys (e.g. vector-search's +// camelCase `indexName`). Each supported probe gets a happy path + a +// missing-field skip. describe("runExistenceProbe — per-type coverage with real manifest keys", () => { it("serving_endpoint: ok via `name`", async () => { const client = { servingEndpoints: { get: async () => ({}) } }; diff --git a/packages/shared/src/cli/commands/doctor/checks-existence.ts b/packages/shared/src/cli/commands/doctor/checks-existence.ts index 25526ad19..6e2636a7d 100644 --- a/packages/shared/src/cli/commands/doctor/checks-existence.ts +++ b/packages/shared/src/cli/commands/doctor/checks-existence.ts @@ -41,8 +41,8 @@ type ExistenceProbe = ( target: ResourceTarget, ) => Promise; -// The SDK's ApiError carries statusCode/errorCode; we read them off the caught -// error structurally rather than importing the class, keeping `shared` SDK-free. +// Read statusCode/errorCode off the SDK's ApiError structurally, keeping +// `shared` SDK-free. function statusCodeOf(err: unknown): number | undefined { if (err && typeof err === "object" && "statusCode" in err) { const code = (err as { statusCode?: unknown }).statusCode; @@ -59,9 +59,8 @@ function errorCodeOf(err: unknown): string | undefined { return undefined; } -// The SDK message often embeds a JSON blob (`Response from server (...) -// {"message":"..."}`); pull the inner `message` out so doctor prints one clean -// line, not a dump. +// The SDK message often embeds a JSON blob; pull the inner `message` out so +// doctor prints one clean line, not a dump. function cleanMessage(err: unknown): string { const raw = err instanceof Error ? err.message : String(err); const m = raw.match(/"message"\s*:\s*"([^"]+)"/); @@ -106,9 +105,8 @@ function classifyError(err: unknown, target: ResourceTarget): LayerResult { }; } -/** Normalizes a field key for comparison: manifests use camelCase (`indexName`) - * while scaffold defaults use snake_case (`index_name`), so match case- and - * separator-insensitively. */ +/** Normalizes a field key so camelCase (`indexName`) and snake_case + * (`index_name`) spellings match. */ function normalizeKey(key: string): string { return key.toLowerCase().replace(/[_-]/g, ""); } @@ -153,9 +151,8 @@ const probeWarehouse: ExistenceProbe = async (client, target) => { const probeServing: ExistenceProbe = async (client, target) => { const name = field(target, "name"); - // Serving endpoints are looked up by name. If the manifest instead keys this - // resource by `id`, we still probe with that value but flag the likely cause - // when it fails, since the API only accepts a name. + // The API only accepts a name; if configured by id we probe with it anyway + // and flag the likely cause on failure. const idOnly = name === null ? field(target, "id") : null; const value = name ?? idOnly; if (!value) return missingField("name"); @@ -247,9 +244,6 @@ const probeFunction: ExistenceProbe = async (client, target) => { } }; -// Lakebase authenticates with an OAuth token as the Postgres password, so -// "password authentication failed" almost never means a wrong password — it -// usually means PGUSER doesn't match the token's identity. function lakebaseAuthHint(message: string): string | undefined { if (/password authentication failed/i.test(message)) { return ( @@ -262,8 +256,7 @@ function lakebaseAuthHint(message: string): string | undefined { } // Lakebase has no cheap control-plane `.get()`, so existence is proven by a -// real connection + `SELECT 1` (exercising endpoint resolution, OAuth token -// mint, TLS, and reachability — the same path the app uses). +// real connection + `SELECT 1`. const probePostgres: ExistenceProbe = async (client, target) => { if (!field(target, "endpointPath") && !field(target, "host")) { return missingField("host/endpoint"); @@ -316,8 +309,7 @@ export function toThreeLevelVolumeName(raw: string): string | null { return null; } -// Types not listed (secret, uc_connection, database, app, experiment) have no -// probe and fall through to NOT_IMPLEMENTED in runExistenceProbe. +// Unlisted types fall through to NOT_IMPLEMENTED in runExistenceProbe. const PROBES: Record = { sql_warehouse: probeWarehouse, serving_endpoint: probeServing, diff --git a/packages/shared/src/cli/commands/doctor/checks.ts b/packages/shared/src/cli/commands/doctor/checks.ts index 815f4e0e0..a3ff8ddcd 100644 --- a/packages/shared/src/cli/commands/doctor/checks.ts +++ b/packages/shared/src/cli/commands/doctor/checks.ts @@ -29,10 +29,7 @@ interface CurrentUserClient { * Validates `DATABRICKS_HOST` before we hand it to the SDK, so an unfilled * placeholder (e.g. `https://...`) gets a clear message instead of the SDK's * opaque "cannot configure default credentials" error. Returns an error - * message, or null when the host is acceptable (or unset — then the SDK's own - * auth chain takes over). - * - * @internal exported for unit testing. + * message, or null when the host is acceptable or unset. */ export function validateHost(host: string | undefined): string | null { if (host === undefined || host.trim().length === 0) return null; @@ -64,14 +61,15 @@ export function validateHost(host: string | undefined): string | null { * existence check (they all need the client). */ export async function checkAuth(options: DoctorOptions): Promise { - const hostError = validateHost(process.env.DATABRICKS_HOST); + const host = process.env.DATABRICKS_HOST; + const hostError = validateHost(host); if (hostError) { return { result: { status: "error", code: "HOST_INVALID", detail: hostError, - host: process.env.DATABRICKS_HOST, + host, profile: options.profile, }, }; @@ -88,7 +86,7 @@ export async function checkAuth(options: DoctorOptions): Promise { status: "ok", code: "AUTH_OK", detail: `authenticated as ${who}`, - host: process.env.DATABRICKS_HOST, + host, profile: options.profile, }, }; @@ -110,7 +108,7 @@ export async function checkAuth(options: DoctorOptions): Promise { code: "AUTH_FAILED", detail: `failed to authenticate to the workspace: ${detail}`, hint: authFailureHint(detail, options.profile), - host: process.env.DATABRICKS_HOST, + host, profile: options.profile, }, }; @@ -118,10 +116,9 @@ export async function checkAuth(options: DoctorOptions): Promise { } /** - * Infers guidance for the common auth failures, which the SDK surfaces as - * opaque strings. Patterns are matched against real SDK messages (a missing - * profile, an expired CLI token, and no credentials at all), most specific - * first. Returns the command that fixes each, or undefined for the unknown. + * Infers guidance for common auth failures, which the SDK surfaces as opaque + * strings. Patterns are matched most specific first; returns the fixing command + * or undefined for an unrecognized failure. */ function authFailureHint( message: string, @@ -139,8 +136,7 @@ function authFailureHint( ) { return `Profile not found in ~/.databrickscfg. Run \`${loginCmd}\`, or pass an existing profile via --profile.`; } - // Expired/failed CLI token: "cannot get access token", "databricks auth - // token", "refresh token is invalid", "reauthenticate". + // Expired/failed CLI token. if ( /cannot get access token|refresh token|reauthenticate|databricks auth token|token .*expired/i.test( message, @@ -160,9 +156,8 @@ function authFailureHint( } /** - * Layer: config. Offline check that each declared env var is present. Presence - * only — whether a set value points at a real resource is the existence layer's - * job, so this never guesses at placeholder values. + * Layer: config. Offline presence check of each declared env var; whether a set + * value points at a real resource is the existence layer's job. */ export async function checkConfig( target: ResourceTarget, diff --git a/packages/shared/src/cli/commands/doctor/databricks-client.ts b/packages/shared/src/cli/commands/doctor/databricks-client.ts index 69b2347f7..ac393ecdf 100644 --- a/packages/shared/src/cli/commands/doctor/databricks-client.ts +++ b/packages/shared/src/cli/commands/doctor/databricks-client.ts @@ -40,14 +40,10 @@ export async function getServiceClient( profile?: string, ): Promise { if (profile) { - // Permanent for this process — fine for a one-shot CLI, but note it if this - // is ever reused in a test or long-lived context. process.env[CONFIG_PROFILE_ENV] = profile; } - // Untyped dynamic import: `shared` has no dependency on the SDK (kept - // SDK-free on purpose), so a `typeof import(...)` annotation would fail to - // resolve at compile time. We narrow to the one call we make below. + // Narrowed to the one call we make; `shared` has no static SDK dependency. let sdk: { WorkspaceClient: new (opts: Record) => unknown }; try { sdk = (await import("@databricks/sdk-experimental")) as typeof sdk; @@ -84,11 +80,8 @@ export interface LakebasePoolHandle { export async function getLakebasePool( client: unknown, ): Promise { - // `shared` intentionally has no dependency on `@databricks/appkit`, and CI - // typechecks it without appkit's `dist` built — so a static import specifier - // would fail to resolve at compile time. Going through a variable specifier - // keeps TS from resolving it statically; the module is an optional peer - // resolved (and guarded) at runtime. + // A variable specifier stops TS from statically resolving `@databricks/appkit`, + // which `shared` has no dependency on; it's an optional peer resolved at runtime. const appkitPkg = "@databricks/appkit"; let appkit: { createLakebasePool: (cfg: Record) => unknown; diff --git a/packages/shared/src/cli/commands/doctor/report.ts b/packages/shared/src/cli/commands/doctor/report.ts index 65cadc3da..ceb89f0bf 100644 --- a/packages/shared/src/cli/commands/doctor/report.ts +++ b/packages/shared/src/cli/commands/doctor/report.ts @@ -66,8 +66,7 @@ export function printReport(report: DoctorReport): void { ); for (const r of ordered) { const { target } = r; - // The `plugin · type` attribution only earns its place when a row needs - // fixing (tells you where to go); on a pass the alias alone is cleaner. + // Only attribute plugin · type on rows that need fixing. const suffix = r.status !== "ok" ? ` ${target.plugin} · ${target.type}` : ""; console.log(` ${glyph(r.status)} ${target.alias}${suffix}`); @@ -80,8 +79,7 @@ export function printReport(report: DoctorReport): void { } console.log(""); - // Fold auth failure into the counts so it doesn't read as "0 errors" just - // because resources weren't probed. + // Fold auth failure into the counts so it doesn't read as "0 errors". const authError = auth.status === "error" ? 1 : 0; console.log(summaryLine({ ...summary, error: summary.error + authError })); if (authError) { diff --git a/packages/shared/src/cli/commands/doctor/resolve-targets.ts b/packages/shared/src/cli/commands/doctor/resolve-targets.ts index 5b3bcd551..935e8bf1a 100644 --- a/packages/shared/src/cli/commands/doctor/resolve-targets.ts +++ b/packages/shared/src/cli/commands/doctor/resolve-targets.ts @@ -40,9 +40,8 @@ interface TemplateManifest { plugins?: Record; } -/** A field's env var is only the developer's to supply when it has no static - * default and isn't platform-injected at deploy time (origin "platform" / - * localOnly). Those the config layer must not flag as missing. */ +/** A field's env var is the developer's to supply only when it has no static + * default and isn't platform-injected at deploy time. */ function isUserSuppliedEnv(field: ManifestField): boolean { if (field.value !== undefined) return false; if (field.origin === "platform" || field.origin === "static") return false; @@ -60,11 +59,8 @@ function envVarsOf(resource: ManifestResource): string[] { return envs; } -/** Resolves each field's value, keyed by manifest field name. Prefers the - * environment, then falls back to a static `value` default in the manifest, so - * a field configured either way reaches the probe (matching what the config - * layer treats as configured). Unset/empty fields are omitted so probes can - * tell provided from absent. */ +/** Resolves each field's value keyed by manifest field name, preferring the + * environment over a static `value` default. Unset/empty fields are omitted. */ function fieldValuesOf(resource: ManifestResource): Record { const fields = resource.fields ?? {}; const values: Record = {}; diff --git a/packages/shared/src/cli/commands/doctor/run.ts b/packages/shared/src/cli/commands/doctor/run.ts index b65c1da9c..8d9a0ab6a 100644 --- a/packages/shared/src/cli/commands/doctor/run.ts +++ b/packages/shared/src/cli/commands/doctor/run.ts @@ -28,18 +28,10 @@ function worst(a: CheckStatus, b: CheckStatus): CheckStatus { return STATUS_SEVERITY[b] > STATUS_SEVERITY[a] ? b : a; } -/** Returns an empty list when no manifest is present — an app may legitimately - * declare no resources, which doctor reports rather than treating as an error. */ -export async function resolveTargets( - options: DoctorOptions, -): Promise { - return resolveTargetsFromCwd(process.cwd(), options.manifest); -} - async function checkResource( target: ResourceTarget, // undefined = auth failed, so the live existence layer is skipped. - client: unknown | undefined, + client: unknown, ): Promise { const layers: LayerResult[] = []; let rolled: CheckStatus = "ok"; @@ -47,7 +39,7 @@ async function checkResource( const configResult = await checkConfig(target); layers.push(configResult); rolled = worst(rolled, configResult.status); - // A hard config failure (missing id) makes the existence probe meaningless. + // A hard config failure makes the existence probe meaningless. if (configResult.status === "error") { return { target, status: rolled, layers }; } @@ -77,9 +69,13 @@ export async function runDoctor(options: DoctorOptions): Promise { // Resources are resolved and config-checked even when auth failed, so a bad // connection still surfaces config problems instead of hiding them all. - const targets = await resolveTargets(options); - for (const target of targets) { - const result = await checkResource(target, client); + // Probes are independent reads, so run them concurrently; Promise.all + // preserves input order, keeping the report deterministic. + const targets = resolveTargetsFromCwd(process.cwd(), options.manifest); + const results = await Promise.all( + targets.map((target) => checkResource(target, client)), + ); + for (const result of results) { resources.push(result); summary[result.status] += 1; } diff --git a/packages/shared/src/cli/commands/doctor/types.ts b/packages/shared/src/cli/commands/doctor/types.ts index 3f70d92f6..de3bc3701 100644 --- a/packages/shared/src/cli/commands/doctor/types.ts +++ b/packages/shared/src/cli/commands/doctor/types.ts @@ -8,9 +8,8 @@ export type CheckStatus = "ok" | "warn" | "error" | "skipped"; export interface LayerResult { layer: CheckLayer; status: CheckStatus; - /** The raw finding (what happened). */ detail?: string; - /** Optional inferred guidance, rendered on its own line below `detail`. */ + /** Inferred guidance for fixing the finding. */ hint?: string; /** Machine-readable code for `--json` consumers (e.g. `NOT_FOUND`). */ code?: string; @@ -27,8 +26,7 @@ export interface ResourceTarget { /** Mandatory (vs optional) for the app. */ required: boolean; envVars: string[]; - /** Field values resolved from `process.env`, keyed by manifest field name - * (e.g. `id`, `name`). Unset fields are omitted. */ + /** Resolved field values keyed by manifest field name; unset fields omitted. */ fieldValues: Record; } @@ -42,7 +40,6 @@ export interface ResourceCheckResult { export interface AuthCheckResult { status: CheckStatus; detail?: string; - /** Optional inferred guidance, rendered on its own line below `detail`. */ hint?: string; code?: string; host?: string; From 93315f66df1e026b08fbaf0c977fa1dd387ea2f9 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Wed, 29 Jul 2026 10:48:21 +0200 Subject: [PATCH 03/14] feat(shared): improve appkit doctor auth diagnostics and report readability - Add --env-file flag to load an explicit env file (e.g. .env.local), overriding the auto-loaded .env so doctor checks the app's real env. - Add --detail flag to show the full raw SDK error; by default the report shows a short "authentication failed" headline plus an actionable hint. - Make auth hints action-first and reference the profile/host in use: classify unreachable-host (DNS/TLS) failures, recover the SDK-resolved profile so the login hint targets the right one, and surface it in the header. - Collapse auth-skipped resources into a single line, keep rows with real findings, and nudge toward --detail at the end. Signed-off-by: Galymzhan --- packages/shared/package.json | 1 + .../shared/src/cli/commands/doctor/README.md | 22 +++++ .../src/cli/commands/doctor/checks.test.ts | 80 ++++++++++++++++++- .../shared/src/cli/commands/doctor/checks.ts | 70 +++++++++++----- .../src/cli/commands/doctor/index.test.ts | 45 +++++++++++ .../shared/src/cli/commands/doctor/index.ts | 29 ++++++- .../src/cli/commands/doctor/report.test.ts | 26 +++++- .../shared/src/cli/commands/doctor/report.ts | 62 ++++++++++++-- .../shared/src/cli/commands/doctor/types.ts | 11 +++ pnpm-lock.yaml | 3 + 10 files changed, 315 insertions(+), 34 deletions(-) create mode 100644 packages/shared/src/cli/commands/doctor/index.test.ts diff --git a/packages/shared/package.json b/packages/shared/package.json index 7045336fc..fa6c7f7a7 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -43,6 +43,7 @@ "@standard-schema/spec": "1.1.0", "@clack/prompts": "1.0.1", "commander": "12.1.0", + "dotenv": "16.6.1", "zod": "4.3.6" } } diff --git a/packages/shared/src/cli/commands/doctor/README.md b/packages/shared/src/cli/commands/doctor/README.md index b746c6f38..e5ef12b98 100644 --- a/packages/shared/src/cli/commands/doctor/README.md +++ b/packages/shared/src/cli/commands/doctor/README.md @@ -20,6 +20,28 @@ so the reported problem is the *root* cause, not a symptom. is the `existence` layer's job. (`DATABRICKS_HOST` is the exception — `auth` validates it structurally, since a bad host means no client can be built.) +### Auth outcomes + +Host and credentials aren't resolved by doctor — an empty `WorkspaceClient({})` +defers to the SDK's unified-auth chain (explicit env, then the selected +`~/.databrickscfg` profile, then OAuth). `--profile` is forwarded via +`DATABRICKS_CONFIG_PROFILE`; with neither host nor profile set, the SDK falls +back to the default profile. Doctor emits: + +| Code | When | Detected | +| ---- | ---- | -------- | +| `HOST_INVALID` | `DATABRICKS_HOST` set but malformed / placeholder | offline, before any network | +| `SDK_NOT_INSTALLED` | Databricks SDK not resolvable | client build | +| `AUTH_OK` | `currentUser.me()` succeeded | live | +| `AUTH_FAILED` | `me()` threw | live | + +`AUTH_FAILED` carries an action-first `hint` inferred from the SDK message +(workspace unreachable, profile not found, expired login / no credentials). Each +hint says what to *run* and to confirm the profile/host actually in use — a +failure can mean the wrong target, not just a stale token. `detail` is the short +headline `authentication failed`; the full SDK message is kept in `raw` and shown +only with `--detail` (always in `--json`). The report labels hints `Hint:`. + ## Files - `types.ts` — the contract: layers, statuses, `ResourceTarget`, `DoctorReport`. diff --git a/packages/shared/src/cli/commands/doctor/checks.test.ts b/packages/shared/src/cli/commands/doctor/checks.test.ts index 34bc109a6..81bfe2b77 100644 --- a/packages/shared/src/cli/commands/doctor/checks.test.ts +++ b/packages/shared/src/cli/commands/doctor/checks.test.ts @@ -155,7 +155,9 @@ describe("checkAuth", () => { const { result } = await checkAuth({}); expect(result.status).toBe("error"); expect(result.code).toBe("AUTH_FAILED"); - expect(result.detail).toContain("something odd"); + // detail stays a short headline; the raw message is carried separately. + expect(result.detail).toBe("authentication failed"); + expect(result.raw).toContain("something odd"); expect(result.hint).toBeUndefined(); // unrecognized failure → no guess }); @@ -168,8 +170,10 @@ describe("checkAuth", () => { ); const { result } = await checkAuth({ profile: "prod" }); - expect(result.hint).toMatch(/expired|reauthenticate/i); + // Action-first: tells the user what to run and to confirm the target, + // without redundantly repeating the profile name (it's in the command). expect(result.hint).toContain("databricks auth login --profile prod"); + expect(result.hint).toMatch(/confirm the profile\/host/i); }); it("hints about a missing profile (real SDK message)", async () => { @@ -179,6 +183,76 @@ describe("checkAuth", () => { ); const { result } = await checkAuth({}); - expect(result.hint).toMatch(/Profile not found/i); + expect(result.hint).toContain("databricks auth login"); + expect(result.hint).toMatch(/--profile/i); + }); + + it("reports the profile from DATABRICKS_CONFIG_PROFILE when --profile is unset", async () => { + process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; + process.env.DATABRICKS_CONFIG_PROFILE = "from-env"; + const client = { currentUser: { me: async () => ({ userName: "u" }) } }; + mockGetServiceClient.mockResolvedValue({ client }); + try { + const { result } = await checkAuth({}); + expect(result.profile).toBe("from-env"); + } finally { + delete process.env.DATABRICKS_CONFIG_PROFILE; + } + }); + + it("mines the profile the SDK used when none was passed, so the login hint targets it", async () => { + process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; + delete process.env.DATABRICKS_CONFIG_PROFILE; + mockGetServiceClient.mockRejectedValue( + new Error( + "default auth: databricks-cli: cannot get access token: the refresh token is invalid. To reauthenticate, run\n $ databricks auth login --profile DEFAULT", + ), + ); + + // No --profile given, but the SDK fell back to DEFAULT — the hint must name it. + const { result } = await checkAuth({}); + expect(result.hint).toContain("databricks auth login --profile DEFAULT"); + // Not the bare, profile-less command (which would reauth the wrong profile). + expect(result.hint).not.toContain("`databricks auth login`"); + }); + + it("hints that the workspace is unreachable on a network error", async () => { + process.env.DATABRICKS_HOST = "https://wrong.cloud.databricks.com"; + mockGetServiceClient.mockRejectedValue( + new Error("getaddrinfo ENOTFOUND wrong.cloud.databricks.com"), + ); + + const { result } = await checkAuth({}); + expect(result.hint).toMatch(/host is correct and reachable/i); + expect(result.hint).toMatch(/DATABRICKS_HOST/); + }); + + it("hints to log in and confirm the target when creds fail with nothing set", async () => { + delete process.env.DATABRICKS_HOST; + delete process.env.DATABRICKS_CONFIG_PROFILE; + mockGetServiceClient.mockRejectedValue( + new Error("default auth: cannot configure default credentials"), + ); + + const { result } = await checkAuth({}); + // No profile/host known → generic login command + confirm-the-target note. + expect(result.hint).toContain("databricks auth login"); + expect(result.hint).toMatch( + /confirm the profile\/host is the one you intend/i, + ); + }); + + it("keeps detail short and carries the full message in raw for --detail/--json", async () => { + process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; + const sprawling = `first line of the failure\n${"x".repeat(300)}`; + mockGetServiceClient.mockRejectedValue(new Error(sprawling)); + + const { result } = await checkAuth({}); + // Human report headline is always the short, constant string... + expect(result.detail).toBe("authentication failed"); + // ...while the full multi-line error is preserved verbatim in raw. + expect(result.raw).toBe(sprawling); + expect(result.raw).toContain("first line of the failure"); + expect(result.raw).toContain("x".repeat(300)); }); }); diff --git a/packages/shared/src/cli/commands/doctor/checks.ts b/packages/shared/src/cli/commands/doctor/checks.ts index a3ff8ddcd..bd5a2afa9 100644 --- a/packages/shared/src/cli/commands/doctor/checks.ts +++ b/packages/shared/src/cli/commands/doctor/checks.ts @@ -62,6 +62,9 @@ export function validateHost(host: string | undefined): string | null { */ export async function checkAuth(options: DoctorOptions): Promise { const host = process.env.DATABRICKS_HOST; + // What the report shows as the identity source: an explicit --profile, else + // the env var the SDK would pick up, so "which profile?" is never a mystery. + const profile = options.profile ?? process.env.DATABRICKS_CONFIG_PROFILE; const hostError = validateHost(host); if (hostError) { return { @@ -70,7 +73,7 @@ export async function checkAuth(options: DoctorOptions): Promise { code: "HOST_INVALID", detail: hostError, host, - profile: options.profile, + profile, }, }; } @@ -87,7 +90,7 @@ export async function checkAuth(options: DoctorOptions): Promise { code: "AUTH_OK", detail: `authenticated as ${who}`, host, - profile: options.profile, + profile, }, }; } catch (err) { @@ -97,60 +100,83 @@ export async function checkAuth(options: DoctorOptions): Promise { status: "error", code: "SDK_NOT_INSTALLED", detail: err.message, - profile: options.profile, + profile, }, }; } - const detail = err instanceof Error ? err.message : String(err); + const raw = err instanceof Error ? err.message : String(err); + // If we didn't know the profile up front, recover the one the SDK actually + // resolved (embedded in its error) and mark it "(resolved)" so the header + // reveals which profile was used — otherwise a default fallback is invisible. + const sdkProfile = raw.match(/--profile\s+(\S+)/)?.[1]; + const shownProfile = + profile ?? (sdkProfile ? `${sdkProfile} (resolved)` : undefined); return { result: { status: "error", code: "AUTH_FAILED", - detail: `failed to authenticate to the workspace: ${detail}`, - hint: authFailureHint(detail, options.profile), + // Keep the headline short and let the hint explain the fix; the raw SDK + // message is carried separately for `--detail` / `--json`. + detail: "authentication failed", + hint: authFailureHint(raw, profile, host), host, - profile: options.profile, + profile: shownProfile, + raw, }, }; } } /** - * Infers guidance for common auth failures, which the SDK surfaces as opaque - * strings. Patterns are matched most specific first; returns the fixing command - * or undefined for an unrecognized failure. + * Suggests a next action for common auth failures. Hints are phrased as + * something to *do* (not a restatement of the error) and, since a failure may + * mean the wrong profile/host is in play, tell the user to confirm which + * profile/host they're targeting. Patterns are matched most specific first; + * returns undefined for an unrecognized failure. */ function authFailureHint( message: string, profile?: string, + host?: string, ): string | undefined { - const loginCmd = profile - ? `databricks auth login --profile ${profile}` + // The SDK resolves a profile even when the user gave none (falling back to + // DEFAULT / DATABRICKS_CONFIG_PROFILE), and its error embeds that name (e.g. + // "databricks auth login --profile DEFAULT"). Prefer the explicit profile, + // then the one the SDK actually used, so the hint points at the profile that + // truly failed — not a bare `databricks auth login` that reauths the wrong one. + const usedProfile = profile ?? message.match(/--profile\s+(\S+)/)?.[1]; + const loginCmd = usedProfile + ? `databricks auth login --profile ${usedProfile}` : "databricks auth login"; - - // "resolve: ~/.databrickscfg has no profile configured" + // The profile/host in use is already shown in the report header and in the + // login command, so hints don't repeat it — they just nudge the user to + // sanity-check the target rather than blindly re-logging-in. + // Network-level failure: the host is unreachable (bad workspace URL, DNS, + // offline, or TLS). Matched first — it's more specific than a credential + // problem and needs a different fix. if ( - /has no .* profile configured|profile .* (does not exist|not found)/i.test( + /ENOTFOUND|EAI_AGAIN|ECONNREFUSED|ECONNRESET|ETIMEDOUT|getaddrinfo|certificate|self.signed|unable to verify|TLS/i.test( message, ) ) { - return `Profile not found in ~/.databrickscfg. Run \`${loginCmd}\`, or pass an existing profile via --profile.`; + return "Check that the workspace host is correct and reachable (verify DATABRICKS_HOST or the profile's host, and that you're online)."; } - // Expired/failed CLI token. + // "resolve: ~/.databrickscfg has no profile configured" if ( - /cannot get access token|refresh token|reauthenticate|databricks auth token|token .*expired/i.test( + /has no .* profile configured|profile .* (does not exist|not found)/i.test( message, ) ) { - return `Your login has expired or the token could not be fetched. Run \`${loginCmd}\` to reauthenticate.`; + return `Run \`${loginCmd}\`, or pass an existing profile via --profile.`; } - // No credentials resolved by any auth method. + // Expired/failed CLI token, or no credentials resolved by any method — both + // point at the same next action: log in and verify the target. if ( - /cannot configure default credentials|default auth|no .*credentials/i.test( + /cannot get access token|refresh token|reauthenticate|databricks auth token|token .*expired|cannot configure default credentials|default auth|no .*credentials/i.test( message, ) ) { - return `No credentials found. Run \`${loginCmd}\`, or set a profile via --profile / DATABRICKS_CONFIG_PROFILE.`; + return `Run \`${loginCmd}\` and confirm the profile/host is the one you intend.`; } return undefined; } diff --git a/packages/shared/src/cli/commands/doctor/index.test.ts b/packages/shared/src/cli/commands/doctor/index.test.ts new file mode 100644 index 000000000..3e72dec60 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/index.test.ts @@ -0,0 +1,45 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { loadEnvFile } from "./index"; + +function makeTempDir(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), "doctor-envfile-")); +} + +describe("loadEnvFile", () => { + const dirs: string[] = []; + const savedKeys = ["DOCTOR_ENVFILE_A", "DOCTOR_ENVFILE_B"]; + + afterEach(() => { + for (const d of dirs) fs.rmSync(d, { recursive: true, force: true }); + dirs.length = 0; + for (const k of savedKeys) delete process.env[k]; + }); + + function writeEnv(contents: string): string { + const dir = makeTempDir(); + dirs.push(dir); + const p = path.join(dir, ".env.local"); + fs.writeFileSync(p, contents); + return p; + } + + it("loads values from the given env file into process.env", () => { + loadEnvFile(writeEnv("DOCTOR_ENVFILE_A=from-file\n")); + expect(process.env.DOCTOR_ENVFILE_A).toBe("from-file"); + }); + + it("overrides an already-set value (so it beats the auto-loaded .env)", () => { + process.env.DOCTOR_ENVFILE_B = "from-default-env"; + loadEnvFile(writeEnv("DOCTOR_ENVFILE_B=from-explicit-file\n")); + expect(process.env.DOCTOR_ENVFILE_B).toBe("from-explicit-file"); + }); + + it("throws a clear error when the file does not exist", () => { + expect(() => loadEnvFile("/no/such/.env.local")).toThrow( + /env file not found/, + ); + }); +}); diff --git a/packages/shared/src/cli/commands/doctor/index.ts b/packages/shared/src/cli/commands/doctor/index.ts index dfaf1dcea..e85f37d79 100644 --- a/packages/shared/src/cli/commands/doctor/index.ts +++ b/packages/shared/src/cli/commands/doctor/index.ts @@ -1,16 +1,33 @@ +import fs from "node:fs"; import process from "node:process"; import { Command } from "commander"; +import dotenv from "dotenv"; import { exitCodeFor, printReport, printReportJson } from "./report"; import { runDoctor } from "./run"; import type { DoctorOptions } from "./types"; +/** + * Loads an explicit env file into `process.env`, overriding the `.env` the CLI + * auto-loads at startup so doctor checks the same environment the app runs with. + * @throws if the file is missing — an explicit `--env-file` that doesn't exist + * is a mistake worth surfacing, not silently ignoring. + */ +export function loadEnvFile(envFile: string): void { + if (!fs.existsSync(envFile)) { + throw new Error(`env file not found: ${envFile}`); + } + dotenv.config({ path: envFile, override: true }); +} + async function runDoctorCommand(options: DoctorOptions): Promise { + if (options.envFile) loadEnvFile(options.envFile); + const report = await runDoctor(options); if (options.json) { printReportJson(report); } else { - printReport(report); + printReport(report, options.detail); } process.exit(exitCodeFor(report)); @@ -26,6 +43,14 @@ export const doctorCommand = new Command("doctor") "appkit.plugins.json", ) .option("-p, --profile ", "Databricks CLI profile to authenticate with") + .option( + "-e, --env-file ", + "Load this env file before checking (overrides the auto-loaded .env), e.g. .env.local", + ) + .option( + "-d, --detail", + "Show full underlying error messages (raw SDK output) for failures", + ) .option("--json", "Output the diagnostic report as JSON") .addHelpText( "after", @@ -33,6 +58,8 @@ export const doctorCommand = new Command("doctor") Examples: $ appkit doctor $ appkit doctor --profile my-profile + $ appkit doctor --env-file .env.local + $ appkit doctor --detail $ appkit doctor --json`, ) .action((opts: DoctorOptions) => diff --git a/packages/shared/src/cli/commands/doctor/report.test.ts b/packages/shared/src/cli/commands/doctor/report.test.ts index 7cc666ef8..8c978c664 100644 --- a/packages/shared/src/cli/commands/doctor/report.test.ts +++ b/packages/shared/src/cli/commands/doctor/report.test.ts @@ -76,6 +76,31 @@ describe("printReport ordering", () => { expect(order).toEqual([...order].sort((a, b) => a - b)); }); + it("hides raw error by default, shows it with detail=true", () => { + const withRaw = report({ + auth: { + status: "error", + detail: "authentication failed", + hint: "Run `databricks auth login`.", + raw: "default auth: databricks-cli: cannot get access token: ", + }, + summary: { ok: 0, warn: 0, error: 0, skipped: 0 }, + }); + + // Default: headline + hint, no raw blob, plus a nudge toward --detail. + const plain = capture(() => printReport(withRaw)); + expect(plain.some((l) => l.includes("authentication failed"))).toBe(true); + expect(plain.some((l) => l.includes("Hint:"))).toBe(true); + expect(plain.some((l) => l.includes(""))).toBe(false); + expect(plain.some((l) => /Run with --detail/.test(l))).toBe(true); + + // --detail: raw blob is surfaced under a "details:" block; no nudge. + const detailed = capture(() => printReport(withRaw, true)); + expect(detailed.some((l) => l.includes("details:"))).toBe(true); + expect(detailed.some((l) => l.includes(""))).toBe(true); + expect(detailed.some((l) => /Run with --detail/.test(l))).toBe(false); + }); + it("summary shows only non-zero categories", () => { const lines = capture(() => printReport( @@ -97,7 +122,6 @@ describe("printReport ordering", () => { ); // Auth failure counts as an error even though no resource errored. expect(lines.some((l) => l.startsWith("1 error"))).toBe(true); - expect(lines.some((l) => /Fix authentication first/.test(l))).toBe(true); }); }); diff --git a/packages/shared/src/cli/commands/doctor/report.ts b/packages/shared/src/cli/commands/doctor/report.ts index ceb89f0bf..ad39fe4b8 100644 --- a/packages/shared/src/cli/commands/doctor/report.ts +++ b/packages/shared/src/cli/commands/doctor/report.ts @@ -4,7 +4,36 @@ * by `plugin list` / `plugin validate`. */ -import type { CheckStatus, DoctorReport } from "./types"; +import type { CheckStatus, DoctorReport, ResourceCheckResult } from "./types"; + +/** + * True when a resource's *only* finding is that its existence probe was skipped + * because auth failed — i.e. it has no independent problem worth its own row. + * Such rows are collapsed into one summary line. + */ +function isOnlyAuthSkipped(r: ResourceCheckResult): boolean { + if (r.status !== "skipped") return false; + const hasAuthSkip = r.layers.some( + (l) => l.layer === "existence" && l.code === "AUTH_UNAVAILABLE", + ); + if (!hasAuthSkip) return false; + // Collapse only when auth-skip is the *sole* finding — any other non-ok layer + // (e.g. a missing env var) is actionable and keeps the row on its own line. + return r.layers.every( + (l) => + l.status === "ok" || + (l.layer === "existence" && l.code === "AUTH_UNAVAILABLE"), + ); +} + +/** Left-pads every line of `text` by `spaces`, for indented detail blocks. */ +function indent(text: string, spaces: number): string { + const pad = " ".repeat(spaces); + return text + .split("\n") + .map((l) => `${pad}${l}`) + .join("\n"); +} function glyph(status: CheckStatus): string { switch (status) { @@ -44,7 +73,7 @@ function summaryLine(counts: { return parts.length > 0 ? parts.join(", ") : "nothing to check"; } -export function printReport(report: DoctorReport): void { +export function printReport(report: DoctorReport, detail = false): void { const { auth, resources, summary } = report; console.log(""); @@ -54,7 +83,12 @@ export function printReport(report: DoctorReport): void { console.log(""); console.log(`${glyph(auth.status)} Auth ${auth.detail ?? auth.status}`); - if (auth.hint) console.log(` hint: ${auth.hint}`); + if (auth.hint) console.log(`\n Hint: ${auth.hint}`); + // The full underlying error is hidden by default (the hint explains the fix); + // `--detail` surfaces it for debugging. + if (detail && auth.raw) { + console.log(`\n Details:\n${indent(auth.raw, 4)}`); + } console.log(""); if (resources.length === 0) { @@ -64,7 +98,14 @@ export function printReport(report: DoctorReport): void { const ordered = [...resources].sort( (a, b) => DISPLAY_ORDER[a.status] - DISPLAY_ORDER[b.status], ); - for (const r of ordered) { + // When auth failed, every resource's existence probe is auth-skipped. Those + // rows carry no new information, so collapse the ones with no *other* + // finding into a single line — but keep any that also have a real problem + // (e.g. a missing env var), which is actionable regardless of auth. + const authSkipped = ordered.filter(isOnlyAuthSkipped); + const shown = ordered.filter((r) => !isOnlyAuthSkipped(r)); + + for (const r of shown) { const { target } = r; // Only attribute plugin · type on rows that need fixing. const suffix = @@ -73,17 +114,24 @@ export function printReport(report: DoctorReport): void { for (const layer of r.layers) { if (layer.status === "ok") continue; if (layer.detail) console.log(` ↳ ${layer.detail}`); - if (layer.hint) console.log(` hint: ${layer.hint}`); + if (layer.hint) console.log(` Hint: ${layer.hint}`); } } + + if (authSkipped.length > 0) { + console.log( + ` • ${authSkipped.length} resource${authSkipped.length > 1 ? "s" : ""} not checked (auth failed)`, + ); + } } console.log(""); // Fold auth failure into the counts so it doesn't read as "0 errors". const authError = auth.status === "error" ? 1 : 0; console.log(summaryLine({ ...summary, error: summary.error + authError })); - if (authError) { - console.log("Fix authentication first, then re-run to check resources."); + // Point at --detail when there's more to show and it wasn't requested. + if (!detail && auth.raw) { + console.log("\nRun with --detail for full error output."); } console.log(""); } diff --git a/packages/shared/src/cli/commands/doctor/types.ts b/packages/shared/src/cli/commands/doctor/types.ts index de3bc3701..2f2e8ce07 100644 --- a/packages/shared/src/cli/commands/doctor/types.ts +++ b/packages/shared/src/cli/commands/doctor/types.ts @@ -44,6 +44,9 @@ export interface AuthCheckResult { code?: string; host?: string; profile?: string; + /** Full underlying error (e.g. the raw SDK message). Shown only with + * `--detail` or in `--json`; the human report relies on `detail` + `hint`. */ + raw?: string; } export interface DoctorReport { @@ -57,4 +60,12 @@ export interface DoctorOptions { manifest?: string; profile?: string; json?: boolean; + /** Show full underlying error messages (raw SDK output) in the human report. */ + detail?: boolean; + /** + * Path to an env file to load before checking (e.g. `.env.local`). Its values + * override the `.env` the CLI auto-loads at startup, so doctor checks the same + * environment the app runs with. + */ + envFile?: string; } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c06f7db67..6ecae34ed 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -563,6 +563,9 @@ importers: commander: specifier: 12.1.0 version: 12.1.0 + dotenv: + specifier: 16.6.1 + version: 16.6.1 zod: specifier: 4.3.6 version: 4.3.6 From 463560c449f3a93a28f025bacf714d78d03feb80 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Wed, 29 Jul 2026 10:57:13 +0200 Subject: [PATCH 04/14] test(shared): fix doctor report detail-block assertion Match the capitalized "Details:" label used in the rendered report. Signed-off-by: Galymzhan --- packages/shared/src/cli/commands/doctor/report.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/shared/src/cli/commands/doctor/report.test.ts b/packages/shared/src/cli/commands/doctor/report.test.ts index 8c978c664..3449de8f6 100644 --- a/packages/shared/src/cli/commands/doctor/report.test.ts +++ b/packages/shared/src/cli/commands/doctor/report.test.ts @@ -94,9 +94,9 @@ describe("printReport ordering", () => { expect(plain.some((l) => l.includes(""))).toBe(false); expect(plain.some((l) => /Run with --detail/.test(l))).toBe(true); - // --detail: raw blob is surfaced under a "details:" block; no nudge. + // --detail: raw blob is surfaced under a "Details:" block; no nudge. const detailed = capture(() => printReport(withRaw, true)); - expect(detailed.some((l) => l.includes("details:"))).toBe(true); + expect(detailed.some((l) => l.includes("Details:"))).toBe(true); expect(detailed.some((l) => l.includes(""))).toBe(true); expect(detailed.some((l) => /Run with --detail/.test(l))).toBe(false); }); From 12191f8935c64b02d11ea584576a8821e0e9edfc Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Thu, 30 Jul 2026 13:22:21 +0200 Subject: [PATCH 05/14] feat(shared): make doctor DAB-aware with wiring checks and refined report Extend `appkit doctor` to understand Databricks Asset Bundles and to render a cleaner, single-list report. - Classify each resource's provenance from databricks.yml + app.yaml (bundle.ts): external (probed live) vs bundle-managed (created on deploy, reported as such rather than a false NOT_FOUND). - Add an offline three-file wiring check (checks-wiring.ts) that spans app.yaml <-> databricks.yml <-> plugin env vars, catching gaps the official validators miss: VALUEFROM_UNBOUND, BUNDLE_REF_MISSING, and ENV_UNWIRED. A wiring error gates the exit code for pre-deploy CI. - Filter to plugins marked requiredByTemplate: true. - Redesign the report: one flat, severity-sorted checklist (no titled sub-sections), every row sharing one shape (glyph + label, indented detail, hint set off by blank lines). Reserve colour for actionable tokens (cyan ids/code spans, bold env vars) via picocolors, which auto-disables for non-TTY / NO_COLOR. - Shorten and standardise error messages: drop redundant snake_case types and SDK noise, quote the value you'd act on, and give missing env vars an actionable hint. - Add --env-file and --detail flags. Signed-off-by: Galymzhan --- packages/shared/package.json | 1 + .../shared/src/cli/commands/doctor/README.md | 72 ++++- .../src/cli/commands/doctor/bundle.test.ts | 99 +++++++ .../shared/src/cli/commands/doctor/bundle.ts | 174 +++++++++++ .../commands/doctor/checks-existence.test.ts | 13 +- .../cli/commands/doctor/checks-existence.ts | 34 ++- .../cli/commands/doctor/checks-wiring.test.ts | 128 +++++++++ .../src/cli/commands/doctor/checks-wiring.ts | 82 ++++++ .../shared/src/cli/commands/doctor/checks.ts | 13 +- .../src/cli/commands/doctor/report.test.ts | 59 ++++ .../shared/src/cli/commands/doctor/report.ts | 272 ++++++++++++++---- .../commands/doctor/resolve-targets.test.ts | 73 ++++- .../cli/commands/doctor/resolve-targets.ts | 18 +- .../src/cli/commands/doctor/run.test.ts | 1 + .../shared/src/cli/commands/doctor/run.ts | 38 ++- .../shared/src/cli/commands/doctor/types.ts | 33 +++ pnpm-lock.yaml | 3 + 17 files changed, 1028 insertions(+), 85 deletions(-) create mode 100644 packages/shared/src/cli/commands/doctor/bundle.test.ts create mode 100644 packages/shared/src/cli/commands/doctor/bundle.ts create mode 100644 packages/shared/src/cli/commands/doctor/checks-wiring.test.ts create mode 100644 packages/shared/src/cli/commands/doctor/checks-wiring.ts diff --git a/packages/shared/package.json b/packages/shared/package.json index fa6c7f7a7..0ea7644da 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -44,6 +44,7 @@ "@clack/prompts": "1.0.1", "commander": "12.1.0", "dotenv": "16.6.1", + "picocolors": "1.1.1", "zod": "4.3.6" } } diff --git a/packages/shared/src/cli/commands/doctor/README.md b/packages/shared/src/cli/commands/doctor/README.md index e5ef12b98..f2db6dfc7 100644 --- a/packages/shared/src/cli/commands/doctor/README.md +++ b/packages/shared/src/cli/commands/doctor/README.md @@ -20,6 +20,64 @@ so the reported problem is the *root* cause, not a symptom. is the `existence` layer's job. (`DATABRICKS_HOST` is the exception — `auth` validates it structurally, since a bad host means no client can be built.) +## Resource provenance: external vs bundle-managed + +Each resource is one of two kinds, which changes what doctor does with it: + +- **external** — referenced by id/name (`${var.*}` or a literal in + `databricks.yml`). It should exist now, so the live `existence` probe applies. +- **bundle-managed** — created by this bundle on deploy + (`${resources...*}`). It doesn't exist until `bundle deploy`, so + probing it would be a false `NOT_FOUND`; doctor reports it as *will be created + on deploy* instead. + +Provenance comes from `bundle.ts`, which reads `databricks.yml` + `app.yaml` and +classifies each binding, then overlays `origin` onto the manifest-sourced +targets. Apps with no `databricks.yml` behave exactly as before (everything +treated as external). + +### Wiring check (`checks-wiring.ts`) + +An offline, auth-independent check of the three-file join that +`databricks bundle validate` can't see (it doesn't know `app.yaml`↔plugin +wiring): + +- `VALUEFROM_UNBOUND` — an `app.yaml` `valueFrom: ` matches no + `databricks.yml` binding (the env var would never be injected). +- `BUNDLE_REF_MISSING` — a `${resources...*}` binding references a + resource not declared in the bundle (deploy would fail). +- `ENV_UNWIRED` (warn) — a plugin needs an env var no `app.yaml` entry provides. + +A wiring `error` gates the exit code, so `appkit doctor` catches deploy-breaking +misconfiguration pre-deploy. + +## Report rendering (`report.ts`) + +One flat, severity-sorted checklist — no titled sub-sections (which read as +inconsistent next to the header-less `Auth` row). `Auth` is always the first +row; every resource and wiring finding follows in the same list, sorted +most-severe first. Bundle-managed resources appear as *will be created on +deploy* rather than a probe result. + +Optimised for a quiet happy path: a healthy resource is just a green tick and +its name — no plugin/type attribution, no per-layer output. Detail (and a +`Hint:`, offset by a blank line so it sits apart from the error) appears only +beneath rows that aren't `ok`. There's no header block; the profile in play is +shown only when auth *fails* (attached under the auth row, where it's the first +thing to sanity-check). + +Colour (via `picocolors`) uses one tight palette: + +- **glyphs** carry status — green `✓` / yellow `⚠` / red `✗` / dim `•`; +- **cyan** marks a literal token you'd type or reference — a `"quoted"` id or + binding name, or a `` `backticked` `` code/command span; +- **bold** marks a `SCREAMING_SNAKE` env-var name (bold + cyan when it sits + inside a code span); +- resource *names* are left unstyled — only the id/code you'd act on is coloured. + +`picocolors` auto-disables for non-TTY output and honours `NO_COLOR`, so +piped/CI output is plain text. + ### Auth outcomes Host and credentials aren't resolved by doctor — an empty `WorkspaceClient({})` @@ -46,22 +104,28 @@ only with `--detail` (always in `--json`). The report labels hints `Hint:`. - `types.ts` — the contract: layers, statuses, `ResourceTarget`, `DoctorReport`. - `resolve-targets.ts` — reads `appkit.plugins.json` → `ResourceTarget[]`. +- `bundle.ts` — reads `databricks.yml` + `app.yaml` for binding provenance + (external vs bundle-managed) and the env↔binding wiring map. - `databricks-client.ts` — the sole SDK seam: dynamic `import()` of the SDK / `@databricks/appkit`, builds a `WorkspaceClient` and Lakebase pool, graceful fallback when uninstalled. - `checks.ts` — `checkAuth`, `checkConfig`, `checkExistence`. - `checks-existence.ts` — per-type existence probe dispatch + error classifier. -- `run.ts` — orchestration: auth once → per resource (config → existence). -- `report.ts` — human table + `--json`, and the CI-gating exit code. +- `checks-wiring.ts` — offline three-file join / bundle-ref consistency check. +- `run.ts` — orchestration: auth once → overlay origin → per resource + (config → existence, skipping bundle-managed) → wiring check. +- `report.ts` — runtime / deploy sections, `--json`, exit code. - `index.ts` — the Commander command + flags. ## Flags - `--profile ` — Databricks CLI profile to authenticate with. +- `--env-file ` — load an env file (e.g. `.env.local`) before checking. +- `--detail` — show full underlying error messages. - `--json` — machine-readable report. -Exit code is non-zero if auth or any resource is in an `error` state, so -`appkit doctor` can gate CI / pre-deploy. +Exit code is non-zero if auth, any resource, or any wiring finding is in an +`error` state, so `appkit doctor` can gate CI / pre-deploy. Checks run as the identity that runs doctor (the developer locally, the app in deployment). diff --git a/packages/shared/src/cli/commands/doctor/bundle.test.ts b/packages/shared/src/cli/commands/doctor/bundle.test.ts new file mode 100644 index 000000000..5f9ef0aaf --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/bundle.test.ts @@ -0,0 +1,99 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { originForEnvVars, readBundleInfo } from "./bundle"; + +function tmp(files: Record): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "doctor-bundle-")); + for (const [name, content] of Object.entries(files)) { + fs.writeFileSync(path.join(dir, name), content); + } + return dir; +} + +const BUNDLE = ` +resources: + apps: + app: + name: my-app + resources: + - name: sql-warehouse # external: \${var.*} + sql_warehouse: + id: \${var.sql_warehouse_id} + permission: CAN_USE + - name: report-job # bundle-managed: \${resources.*} + job: + id: \${resources.jobs.report.id} + permission: CAN_MANAGE_RUN + jobs: + report: + name: report +`; + +const APP_YAML = ` +env: + - name: DATABRICKS_WAREHOUSE_ID + valueFrom: sql-warehouse + - name: DATABRICKS_JOB_REPORT + valueFrom: report-job +`; + +describe("readBundleInfo", () => { + const dirs: string[] = []; + afterEach(() => { + for (const d of dirs) fs.rmSync(d, { recursive: true, force: true }); + dirs.length = 0; + }); + + it("returns present:false when no databricks.yml exists", () => { + const dir = tmp({}); + dirs.push(dir); + const info = readBundleInfo(dir); + expect(info.present).toBe(false); + expect(info.bindings.size).toBe(0); + }); + + it("classifies external (var) vs bundle-managed (resources ref) bindings", () => { + const dir = tmp({ "databricks.yml": BUNDLE, "app.yaml": APP_YAML }); + dirs.push(dir); + const info = readBundleInfo(dir); + expect(info.present).toBe(true); + expect(info.bindings.get("sql-warehouse")?.origin).toBe("external"); + const job = info.bindings.get("report-job"); + expect(job?.origin).toBe("bundle-managed"); + expect(job?.ref).toEqual({ type: "jobs", key: "report" }); + }); + + it("records declared bundle resources and the app.yaml env→binding map", () => { + const dir = tmp({ "databricks.yml": BUNDLE, "app.yaml": APP_YAML }); + dirs.push(dir); + const info = readBundleInfo(dir); + expect(info.declaredResources.has("jobs.report")).toBe(true); + expect(info.envToBinding.get("DATABRICKS_WAREHOUSE_ID")).toBe( + "sql-warehouse", + ); + }); +}); + +describe("originForEnvVars", () => { + const dirs: string[] = []; + afterEach(() => { + for (const d of dirs) fs.rmSync(d, { recursive: true, force: true }); + dirs.length = 0; + }); + + it("resolves origin by walking env var → binding → origin", () => { + const dir = tmp({ "databricks.yml": BUNDLE, "app.yaml": APP_YAML }); + dirs.push(dir); + const info = readBundleInfo(dir); + expect(originForEnvVars(["DATABRICKS_WAREHOUSE_ID"], info)).toBe( + "external", + ); + expect(originForEnvVars(["DATABRICKS_JOB_REPORT"], info)).toBe( + "bundle-managed", + ); + // Unknown env / no bundle → undefined (caller treats as external). + expect(originForEnvVars(["NOPE"], info)).toBeUndefined(); + }); +}); diff --git a/packages/shared/src/cli/commands/doctor/bundle.ts b/packages/shared/src/cli/commands/doctor/bundle.ts new file mode 100644 index 000000000..7eb66c3b8 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/bundle.ts @@ -0,0 +1,174 @@ +/** + * Reads the Databricks Asset Bundle (`databricks.yml`) and `app.yaml` to learn + * two things doctor can't get from `appkit.plugins.json`: + * + * 1. **Provenance** — whether each app resource binding references an *existing* + * resource (`${var.*}` or a literal → `external`) or one this bundle + * *creates* (`${resources...*}` → `bundle-managed`). A + * bundle-managed resource doesn't exist until `bundle deploy`, so doctor + * must not probe it (that would be a false NOT_FOUND). + * + * 2. **Wiring** — the join key that ties the three files together is the + * binding *name*: `app.yaml`'s `valueFrom: ` must match a + * `databricks.yml` binding `name`, which an AppKit plugin then reads via an + * env var. We surface the pieces so the wiring check can validate the join. + * + * SDK-free; pure YAML parsing. Absent files degrade to empty results. + */ + +import fs from "node:fs"; +import path from "node:path"; +import yaml from "js-yaml"; +import type { ResourceOrigin } from "./types"; + +export const DEFAULT_BUNDLE_FILE = "databricks.yml"; +export const DEFAULT_APP_YAML_FILE = "app.yaml"; + +/** A `${resources...}` reference — a bundle-created resource. */ +const RESOURCES_REF = /\$\{resources\.([^.]+)\.([^.}]+)\.[^}]+\}/; + +/** One app resource binding from `resources.apps..resources[]`. */ +export interface BundleBinding { + /** The binding name — the join key with `app.yaml` `valueFrom`. */ + name: string; + /** The typed sub-key that names the kind (`sql_warehouse`, `genie_space`, …). */ + type?: string; + origin: ResourceOrigin; + /** For bundle-managed bindings: the `.` it references, if parseable. */ + ref?: { type: string; key: string }; +} + +export interface BundleInfo { + /** Bindings under every app, keyed by binding name. */ + bindings: Map; + /** `app.yaml` env var name → binding name (from `valueFrom`). */ + envToBinding: Map; + /** Declared bundle resource keys, as `"."`, for ref-integrity. */ + declaredResources: Set; + /** True when a `databricks.yml` was found and parsed. */ + present: boolean; +} + +interface AppResourceBlock { + name?: string; + [k: string]: unknown; +} +interface BundleAppBlock { + resources?: AppResourceBlock[]; +} +interface BundleDoc { + resources?: { + apps?: Record; + [otherType: string]: Record | undefined; + }; +} +interface AppYamlDoc { + env?: Array<{ name?: string; valueFrom?: string }>; +} + +/** The typed sub-key of a binding is its single non-`name` object property. */ +function bindingType(block: AppResourceBlock): string | undefined { + for (const [k, v] of Object.entries(block)) { + if (k === "name") continue; + if (v && typeof v === "object") return k; + } + return undefined; +} + +/** Classifies a binding's origin by scanning its field values for a + * `${resources.*}` reference (bundle-managed) vs anything else (external). */ +function classifyBinding(block: AppResourceBlock): { + origin: ResourceOrigin; + ref?: { type: string; key: string }; +} { + const type = bindingType(block); + const typed = type ? block[type] : undefined; + if (typed && typeof typed === "object") { + for (const value of Object.values(typed as Record)) { + if (typeof value !== "string") continue; + const m = value.match(RESOURCES_REF); + if (m) { + return { origin: "bundle-managed", ref: { type: m[1], key: m[2] } }; + } + } + } + return { origin: "external" }; +} + +function readYaml(filePath: string): T | null { + if (!fs.existsSync(filePath)) return null; + try { + return (yaml.load(fs.readFileSync(filePath, "utf-8")) ?? {}) as T; + } catch { + return null; + } +} + +/** + * Parses `databricks.yml` (+ `app.yaml`) into {@link BundleInfo}. Returns an + * empty-but-`present:false` result when no bundle is found, so callers can treat + * "no bundle" as "everything external" without special-casing. + */ +export function readBundleInfo( + cwd: string = process.cwd(), + bundleFile: string = DEFAULT_BUNDLE_FILE, + appYamlFile: string = DEFAULT_APP_YAML_FILE, +): BundleInfo { + const bindings = new Map(); + const envToBinding = new Map(); + const declaredResources = new Set(); + + const doc = readYaml(path.resolve(cwd, bundleFile)); + if (!doc) { + return { bindings, envToBinding, declaredResources, present: false }; + } + + // Every declared bundle resource, as "." (apps included). + for (const [type, group] of Object.entries(doc.resources ?? {})) { + if (!group || typeof group !== "object") continue; + for (const key of Object.keys(group)) + declaredResources.add(`${type}.${key}`); + } + + // App resource bindings across every app in the bundle. + for (const app of Object.values(doc.resources?.apps ?? {})) { + for (const block of app.resources ?? []) { + if (!block?.name) continue; + const { origin, ref } = classifyBinding(block); + bindings.set(block.name, { + name: block.name, + type: bindingType(block), + origin, + ref, + }); + } + } + + const appYaml = readYaml(path.resolve(cwd, appYamlFile)); + for (const entry of appYaml?.env ?? []) { + if (entry?.name && entry.valueFrom) { + envToBinding.set(entry.name, entry.valueFrom); + } + } + + return { bindings, envToBinding, declaredResources, present: true }; +} + +/** + * Resolves the origin for a target given its env vars, by walking + * env var → binding name → binding origin. Returns undefined when the bundle + * has nothing to say (so the target stays external by default). + */ +export function originForEnvVars( + envVars: string[], + info: BundleInfo, +): ResourceOrigin | undefined { + if (!info.present) return undefined; + for (const env of envVars) { + const bindingName = info.envToBinding.get(env); + if (!bindingName) continue; + const binding = info.bindings.get(bindingName); + if (binding) return binding.origin; + } + return undefined; +} diff --git a/packages/shared/src/cli/commands/doctor/checks-existence.test.ts b/packages/shared/src/cli/commands/doctor/checks-existence.test.ts index 154d418f4..c338a520f 100644 --- a/packages/shared/src/cli/commands/doctor/checks-existence.test.ts +++ b/packages/shared/src/cli/commands/doctor/checks-existence.test.ts @@ -63,7 +63,7 @@ describe("runExistenceProbe — sql_warehouse", () => { expect(r.code).toBe("NOT_FOUND"); }); - it("errors INVALID_VALUE on a 400 with a clean message", async () => { + it("errors INVALID_VALUE on a 400, quoting the value (no type/blob leak)", async () => { const client = { warehouses: { get: async () => { @@ -76,12 +76,17 @@ describe("runExistenceProbe — sql_warehouse", () => { }, }, }; - const r = await runExistenceProbe(client, target()); + // The target has an id, so we own the wording: quote the value, don't echo + // the SDK sentence, don't leak the raw type or JSON blob. + const r = await runExistenceProbe( + client, + target({ fieldValues: { id: "wh-123" } }), + ); expect(r.status).toBe("error"); expect(r.code).toBe("INVALID_VALUE"); - // The clean inner message is extracted, not the whole JSON blob. - expect(r.detail).toContain("not a valid endpoint id"); + expect(r.detail).toContain('"wh-123"'); expect(r.detail).not.toContain("error_code"); + expect(r.detail).not.toContain("sql_warehouse"); }); it("errors ACCESS_DENIED on a 403", async () => { diff --git a/packages/shared/src/cli/commands/doctor/checks-existence.ts b/packages/shared/src/cli/commands/doctor/checks-existence.ts index 6e2636a7d..6d2f5f929 100644 --- a/packages/shared/src/cli/commands/doctor/checks-existence.ts +++ b/packages/shared/src/cli/commands/doctor/checks-existence.ts @@ -67,26 +67,40 @@ function cleanMessage(err: unknown): string { return m ? m[1] : raw; } +/** The resource's configured identifier (id / name / path / host), for short + * messages. The row label already names the *type*, so details never repeat it + * — they only surface the value you'd act on, quoted so the report highlights + * it. */ +function displayId(target: ResourceTarget): string | null { + return field(target, "id", "name", "path", "index_name", "indexName", "host"); +} + function classifyError(err: unknown, target: ResourceTarget): LayerResult { const status = statusCodeOf(err); const errorCode = errorCodeOf(err); const message = cleanMessage(err); + const id = displayId(target); + const quoted = id ? `"${id}"` : null; if (status === 404 || errorCode === "RESOURCE_DOES_NOT_EXIST") { return { layer: "existence", status: "error", code: "NOT_FOUND", - detail: `${target.type} not found — check the configured id/name (${message})`, + detail: quoted ? `${quoted} not found` : "not found", }; } - // A malformed id/name often comes back as 400 rather than 404. + // A malformed id/name often comes back as 400 rather than 404. We own the + // wording (quoting the value so it highlights) rather than echoing the SDK's + // sentence, which repeats "invalid" and leaks the raw type. if (status === 400 || errorCode === "INVALID_PARAMETER_VALUE") { return { layer: "existence", status: "error", code: "INVALID_VALUE", - detail: `invalid ${target.type} id/name: ${message}`, + detail: quoted + ? `${quoted} is not a valid id/name` + : `invalid id/name: ${message}`, }; } if (status === 403 || errorCode === "PERMISSION_DENIED") { @@ -94,14 +108,18 @@ function classifyError(err: unknown, target: ResourceTarget): LayerResult { layer: "existence", status: "error", code: "ACCESS_DENIED", - detail: `access denied reading ${target.type} — the identity may lack visibility (${message})`, + detail: quoted + ? `no permission to read ${quoted}` + : "no permission to read it", }; } return { layer: "existence", status: "error", code: "PROBE_FAILED", - detail: `failed to read ${target.type}: ${message}`, + detail: quoted + ? `read failed for ${quoted}: ${message}` + : `read failed: ${message}`, }; } @@ -125,7 +143,7 @@ function missingField(fieldName: string): LayerResult { layer: "existence", status: "skipped", code: "MISSING_FIELD", - detail: `cannot probe existence: no resolved value for "${fieldName}"`, + detail: `no ${fieldName} configured — can't probe`, }; } @@ -189,7 +207,7 @@ const probeJob: ExistenceProbe = async (client, target) => { layer: "existence", status: "error", code: "INVALID_ID", - detail: `job id is not a valid integer: "${raw}"`, + detail: `"${raw}" is not a valid job id (expected an integer)`, }; } try { @@ -211,7 +229,7 @@ const probeVolume: ExistenceProbe = async (client, target) => { layer: "existence", status: "error", code: "INVALID_NAME", - detail: `cannot derive a 3-level volume name from "${raw}"`, + detail: `"${raw}" is not a valid volume path (expected /Volumes/catalog/schema/volume or catalog.schema.volume)`, }; } try { diff --git a/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts b/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts new file mode 100644 index 000000000..84620f7c5 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, it } from "vitest"; +import type { BundleInfo } from "./bundle"; +import { checkWiring } from "./checks-wiring"; +import type { ResourceTarget } from "./types"; + +function info(overrides: Partial = {}): BundleInfo { + return { + bindings: new Map(), + envToBinding: new Map(), + declaredResources: new Set(), + present: true, + ...overrides, + }; +} + +function target(overrides: Partial = {}): ResourceTarget { + return { + type: "sql_warehouse", + resourceKey: "sql-warehouse", + alias: "SQL Warehouse", + plugin: "analytics", + requiredPermission: "CAN_USE", + required: true, + envVars: [], + fieldValues: {}, + ...overrides, + }; +} + +describe("checkWiring", () => { + it("returns nothing when no bundle is present", () => { + expect(checkWiring(info({ present: false }), [])).toEqual([]); + }); + + it("flags an app.yaml valueFrom that matches no binding", () => { + const findings = checkWiring( + info({ envToBinding: new Map([["DATABRICKS_WAREHOUSE_ID", "sql-wh"]]) }), + [], + ); + expect(findings).toHaveLength(1); + expect(findings[0].code).toBe("VALUEFROM_UNBOUND"); + expect(findings[0].status).toBe("error"); + }); + + it("flags a bundle-managed ref to an undeclared resource", () => { + const findings = checkWiring( + info({ + bindings: new Map([ + [ + "job", + { + name: "job", + type: "job", + origin: "bundle-managed", + ref: { type: "jobs", key: "ghost" }, + }, + ], + ]), + declaredResources: new Set(), // jobs.ghost NOT declared + }), + [], + ); + expect(findings.some((f) => f.code === "BUNDLE_REF_MISSING")).toBe(true); + }); + + it("passes a bundle-managed ref that resolves to a declared resource", () => { + const findings = checkWiring( + info({ + bindings: new Map([ + [ + "job", + { + name: "job", + type: "job", + origin: "bundle-managed", + ref: { type: "jobs", key: "report" }, + }, + ], + ]), + declaredResources: new Set(["jobs.report"]), + }), + [], + ); + expect(findings.some((f) => f.code === "BUNDLE_REF_MISSING")).toBe(false); + }); + + it("warns when a plugin needs an env var app.yaml doesn't provide", () => { + const findings = checkWiring( + info({ + // app.yaml provides one env, bindings exist for it. + envToBinding: new Map([["DATABRICKS_WAREHOUSE_ID", "sql-warehouse"]]), + bindings: new Map([ + [ + "sql-warehouse", + { + name: "sql-warehouse", + type: "sql_warehouse", + origin: "external", + }, + ], + ]), + }), + [target({ envVars: ["DATABRICKS_WAREHOUSE_ID", "DATABRICKS_MISSING"] })], + ); + const unwired = findings.find((f) => f.code === "ENV_UNWIRED"); + expect(unwired?.status).toBe("warn"); + expect(unwired?.label).toBe("DATABRICKS_MISSING"); + }); + + it("warns even when app.yaml has NO env block (used plugin, zero wiring)", () => { + // The 'works locally, crashes on deploy' case: analytics is used and its + // env var is set locally via .env, but app.yaml provides nothing, so the + // deployed container has no way to inject it. + const findings = checkWiring( + info({ present: true }), // empty bindings + empty envToBinding + [ + target({ + alias: "SQL Warehouse", + envVars: ["DATABRICKS_WAREHOUSE_ID"], + }), + ], + ); + const unwired = findings.find((f) => f.code === "ENV_UNWIRED"); + expect(unwired?.status).toBe("warn"); + expect(unwired?.label).toBe("DATABRICKS_WAREHOUSE_ID"); + expect(unwired?.detail).toMatch(/unset in the deployed app/i); + }); +}); diff --git a/packages/shared/src/cli/commands/doctor/checks-wiring.ts b/packages/shared/src/cli/commands/doctor/checks-wiring.ts new file mode 100644 index 000000000..af0633f49 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/checks-wiring.ts @@ -0,0 +1,82 @@ +/** + * Layer: wiring (offline, deploy-declaration). Validates the three-file join + * that `databricks bundle validate` can't see, because it spans `app.yaml` and + * the AppKit plugin layer: + * + * - every `app.yaml` `valueFrom: ` must match a `databricks.yml` binding + * `name` — otherwise the platform has nothing to inject and the env var the + * plugin reads stays empty at runtime; + * - every bundle-managed `${resources...*}` binding must reference a + * resource actually declared in the bundle — otherwise deploy fails. + * + * Runs regardless of auth; these are declaration problems, not connectivity. + */ + +import type { BundleInfo } from "./bundle"; +import type { ResourceTarget, WiringFinding } from "./types"; + +/** + * @param info parsed bundle + app.yaml wiring + * @param targets resolved resource targets (for env-var context in messages) + */ +export function checkWiring( + info: BundleInfo, + targets: ResourceTarget[], +): WiringFinding[] { + if (!info.present) return []; + + const findings: WiringFinding[] = []; + + // 1. Each app.yaml valueFrom must resolve to a real binding. + for (const [envVar, bindingName] of info.envToBinding) { + if (!info.bindings.has(bindingName)) { + findings.push({ + status: "error", + code: "VALUEFROM_UNBOUND", + label: envVar, + detail: `app.yaml binds it to "${bindingName}", which databricks.yml doesn't declare`, + hint: `Add a resource named "${bindingName}" under resources.apps..resources, or fix the valueFrom to match an existing binding.`, + }); + } + } + + // 2. Each bundle-managed binding must reference a declared bundle resource. + for (const binding of info.bindings.values()) { + if (binding.origin !== "bundle-managed" || !binding.ref) continue; + const ref = `${binding.ref.type}.${binding.ref.key}`; + if (!info.declaredResources.has(ref)) { + findings.push({ + status: "error", + code: "BUNDLE_REF_MISSING", + label: binding.name, + detail: `references \${resources.${ref}.*}, but no resources.${binding.ref.type}.${binding.ref.key} is declared in the bundle`, + hint: `Declare resources.${binding.ref.type}.${binding.ref.key} in databricks.yml, or point the binding at an existing resource.`, + }); + } + } + + // 3. A used plugin needs an env var that no app.yaml entry provides. Locally + // the value comes from .env, but .env isn't uploaded — on deploy the platform + // only injects what app.yaml maps. So a used plugin whose env var has no + // app.yaml entry will be *unset in the deployed container* and crash at + // runtime, even though local checks are green. This is the headline + // "works locally, breaks on deploy" case, so it fires whenever the app is a + // bundle (databricks.yml present) — including when app.yaml has no env block + // at all, which is the most broken state, not a "doesn't use injection" one. + for (const target of targets) { + for (const envVar of target.envVars) { + if (!info.envToBinding.has(envVar)) { + findings.push({ + status: "warn", + code: "ENV_UNWIRED", + label: envVar, + detail: + "has no app.yaml entry — it will be unset in the deployed app", + hint: `Add to app.yaml: \`{ name: ${envVar}, valueFrom: }\`, and declare that binding in databricks.yml.`, + }); + } + } + } + + return findings; +} diff --git a/packages/shared/src/cli/commands/doctor/checks.ts b/packages/shared/src/cli/commands/doctor/checks.ts index bd5a2afa9..2e9e47a42 100644 --- a/packages/shared/src/cli/commands/doctor/checks.ts +++ b/packages/shared/src/cli/commands/doctor/checks.ts @@ -118,7 +118,7 @@ export async function checkAuth(options: DoctorOptions): Promise { // Keep the headline short and let the hint explain the fix; the raw SDK // message is carried separately for `--detail` / `--json`. detail: "authentication failed", - hint: authFailureHint(raw, profile, host), + hint: authFailureHint(raw, profile), host, profile: shownProfile, raw, @@ -137,7 +137,6 @@ export async function checkAuth(options: DoctorOptions): Promise { function authFailureHint( message: string, profile?: string, - host?: string, ): string | undefined { // The SDK resolves a profile even when the user gave none (falling back to // DEFAULT / DATABRICKS_CONFIG_PROFILE), and its error embeds that name (e.g. @@ -198,11 +197,19 @@ export async function checkConfig( } if (missing.length > 0) { + const plural = missing.length > 1; + const names = missing.join(", "); return { layer: "config", status: target.required ? "error" : "warn", code: target.required ? "ENV_MISSING" : "ENV_MISSING_OPTIONAL", - detail: `${target.required ? "required" : "optional"} resource is missing env var(s): ${missing.join(", ")}`, + // The env var name(s) do the work — the report bolds SCREAMING_SNAKE + // names. Optional resources say so: a missing optional is a warn, not a + // blocker. + detail: target.required + ? `${names} ${plural ? "are" : "is"} not set` + : `${names} ${plural ? "are" : "is"} not set (optional)`, + hint: `Set ${plural ? "them" : "it"} in your .env (local) and wire ${plural ? "them" : "it"} through app.yaml + databricks.yml for deploy.`, }; } diff --git a/packages/shared/src/cli/commands/doctor/report.test.ts b/packages/shared/src/cli/commands/doctor/report.test.ts index 3449de8f6..457d1a423 100644 --- a/packages/shared/src/cli/commands/doctor/report.test.ts +++ b/packages/shared/src/cli/commands/doctor/report.test.ts @@ -6,6 +6,7 @@ function report(overrides: Partial = {}): DoctorReport { return { auth: { status: "ok" }, resources: [], + wiring: [], summary: { ok: 0, warn: 0, error: 0, skipped: 0 }, ...overrides, }; @@ -63,6 +64,7 @@ describe("printReport ordering", () => { res("error", "errored"), res("skipped", "skip"), ], + wiring: [], summary: { ok: 1, warn: 1, error: 1, skipped: 1 }, }; @@ -111,6 +113,45 @@ describe("printReport ordering", () => { expect(lines.some((l) => /warning|error|skipped/.test(l))).toBe(false); }); + it("renders bundle-managed resources and wiring findings in the flat list", () => { + const external = res("ok", "sql_warehouse"); // origin undefined ⇒ runtime + const managed = res("skipped", "job"); + managed.target.origin = "bundle-managed"; + managed.target.alias = "Report Job"; + managed.layers = [ + { layer: "existence", status: "skipped", code: "BUNDLE_MANAGED" }, + ]; + + const lines = capture(() => + printReport( + report({ + resources: [external, managed], + wiring: [ + { + status: "error", + code: "VALUEFROM_UNBOUND", + label: "SOME_ENV", + detail: "app.yaml binds X to Y, no such binding", + }, + ], + summary: { ok: 1, warn: 0, error: 0, skipped: 1 }, + }), + ), + ); + const out = lines.join("\n"); + // No titled sub-sections: a single flat, severity-sorted checklist. + expect(out).not.toContain("Runtime connectivity"); + expect(out).not.toContain("Deploy declaration"); + // A bundle-managed resource is shown as deploy-created, not probed. + expect(out).toContain("will be created on deploy"); + expect(out).toContain("Report Job"); + // The wiring error sorts to the top (most severe), above the ok resource. + const wiringIdx = lines.findIndex((l) => l.includes("no such binding")); + const okIdx = lines.findIndex((l) => l.includes("sql_warehouse")); + expect(wiringIdx).toBeGreaterThanOrEqual(0); + expect(wiringIdx).toBeLessThan(okIdx); + }); + it("folds an auth error into the summary error count", () => { const lines = capture(() => printReport( @@ -145,6 +186,24 @@ describe("exitCodeFor", () => { ), ).toBe(1); }); + + it("is 1 when a wiring finding errored (gates pre-deploy)", () => { + expect( + exitCodeFor( + report({ + summary: { ok: 0, warn: 0, error: 0, skipped: 0 }, + wiring: [ + { + status: "error", + code: "VALUEFROM_UNBOUND", + label: "X", + detail: "x", + }, + ], + }), + ), + ).toBe(1); + }); }); describe("printReportJson", () => { diff --git a/packages/shared/src/cli/commands/doctor/report.ts b/packages/shared/src/cli/commands/doctor/report.ts index ad39fe4b8..4f8030662 100644 --- a/packages/shared/src/cli/commands/doctor/report.ts +++ b/packages/shared/src/cli/commands/doctor/report.ts @@ -2,9 +2,29 @@ * Rendering for `appkit doctor` — turns a {@link DoctorReport} into either a * human-readable console report or JSON, matching the `--json` convention used * by `plugin list` / `plugin validate`. + * + * Design goals: one flat, severity-sorted checklist (no titled sub-sections), + * where every row shares the same shape — `glyph label`, with detail and an + * optional `Hint:` indented beneath, and expanded rows set apart by blank lines. + * Show as little as possible on the happy path (a fine resource is just a green + * tick and its name); reveal detail only where there's something to act on. + * Colour is sparing and consistent — glyphs carry status (green/red/yellow), + * and the actionable token (a quoted id / code span in cyan, an env var in bold) + * is highlighted so it stands out from prose. picocolors auto-disables when + * output isn't a TTY (and honours NO_COLOR), so piped/CI output stays plain. */ -import type { CheckStatus, DoctorReport, ResourceCheckResult } from "./types"; +import pc from "picocolors"; +import type { + CheckStatus, + DoctorReport, + ResourceCheckResult, + WiringFinding, +} from "./types"; + +/** Column at which a row's label starts (`" ✓ "` → 5), used to align the + * detail/hint lines beneath it. */ +const SUB_INDENT = " "; /** * True when a resource's *only* finding is that its existence probe was skipped @@ -35,16 +55,58 @@ function indent(text: string, spaces: number): string { .join("\n"); } +/** A status glyph, coloured by severity. */ function glyph(status: CheckStatus): string { switch (status) { case "ok": - return "✓"; + return pc.green("✓"); case "warn": - return "⚠"; + return pc.yellow("⚠"); case "error": - return "✗"; + return pc.red("✗"); default: - return "•"; + return pc.dim("•"); + } +} + +/** + * Highlights the actionable tokens in a prose detail/hint line. One tight + * palette: cyan = a literal token you type or reference — a "quoted" id/binding + * name or a `backticked` code/command span (the delimiters are stripped); bold = + * a SCREAMING_SNAKE env-var name. Resource *names* aren't highlighted — only the + * id/code you'd act on is. Nesting is safe: bold's reset (22) doesn't clear the + * cyan (39), so an env var inside a code span stays bold *and* cyan. Purely + * cosmetic — a no-op when colour is disabled. + */ +function highlight(text: string): string { + return text + .replace(/`([^`]+)`/g, (_, code) => pc.cyan(code)) + .replace(/"([^"]+)"/g, (_, id) => pc.cyan(id)) + .replace(/\b[A-Z][A-Z0-9]*(?:_[A-Z0-9]+)+\b/g, (id) => pc.bold(id)); +} + +/** A sub-line beneath a row (detail or hint), consistently indented. */ +function subLine(text: string): void { + console.log(`${SUB_INDENT}${text}`); +} + +/** + * Prints the expanded body shared by every non-ok row: each indented `detail` + * line, then (if any) a `Hint:` set off by a blank line above it. Keeps + * resource, wiring, and auth rows visually identical. Multiple details/hints (a + * resource can trip more than one layer) are grouped into one block. Callers own + * the surrounding blank lines (see {@link printReport}), so a body never emits a + * trailing blank of its own. + */ +function printRowBody( + details: Array, + hints: Array, +): void { + for (const d of details) if (d) subLine(highlight(d)); + const realHints = hints.filter((h): h is string => Boolean(h)); + if (realHints.length > 0) { + console.log(""); + for (const h of realHints) subLine(`${pc.dim("Hint:")} ${highlight(h)}`); } } @@ -56,7 +118,7 @@ const DISPLAY_ORDER: Record = { ok: 3, }; -/** Shows only the non-zero categories. */ +/** Shows only the non-zero categories, coloured by severity. */ function summaryLine(counts: { ok: number; warn: number; @@ -65,73 +127,163 @@ function summaryLine(counts: { }): string { const parts: string[] = []; if (counts.error) - parts.push(`${counts.error} error${counts.error > 1 ? "s" : ""}`); + parts.push(pc.red(`${counts.error} error${counts.error > 1 ? "s" : ""}`)); if (counts.warn) - parts.push(`${counts.warn} warning${counts.warn > 1 ? "s" : ""}`); - if (counts.ok) parts.push(`${counts.ok} ok`); - if (counts.skipped) parts.push(`${counts.skipped} skipped`); - return parts.length > 0 ? parts.join(", ") : "nothing to check"; + parts.push( + pc.yellow(`${counts.warn} warning${counts.warn > 1 ? "s" : ""}`), + ); + if (counts.ok) parts.push(pc.green(`${counts.ok} ok`)); + if (counts.skipped) parts.push(pc.dim(`${counts.skipped} skipped`)); + return parts.length > 0 ? parts.join(pc.dim(", ")) : "nothing to check"; } -export function printReport(report: DoctorReport, detail = false): void { - const { auth, resources, summary } = report; +/** Prints the leading `Auth` check row (always first). On failure, appends the + * target profile, a hint, and — with `--detail` — the raw SDK error. */ +function printAuthRow(report: DoctorReport, detail: boolean): void { + const { auth } = report; + console.log(` ${glyph(auth.status)} Auth — ${auth.detail ?? auth.status}`); + // Happy path: just the row. On failure, expand with the target profile, hint, + // and (with --detail) the raw error — the caller frames it with blank lines. + if (auth.status !== "error") return; - console.log(""); - console.log("Databricks AppKit — connection check"); - if (auth.profile) console.log(` profile ${auth.profile}`); - if (auth.host) console.log(` workspace ${auth.host}`); - console.log(""); - - console.log(`${glyph(auth.status)} Auth ${auth.detail ?? auth.status}`); - if (auth.hint) console.log(`\n Hint: ${auth.hint}`); + // The profile in play is only interesting when auth failed — it's the first + // thing to sanity-check — so attach it here rather than in a header. + if (auth.profile) { + subLine(`${pc.dim("profile:")} ${pc.cyan(auth.profile)}`); + } + if (auth.hint) { + console.log(""); + subLine(`${pc.dim("Hint:")} ${highlight(auth.hint)}`); + } // The full underlying error is hidden by default (the hint explains the fix); // `--detail` surfaces it for debugging. if (detail && auth.raw) { - console.log(`\n Details:\n${indent(auth.raw, 4)}`); + console.log(""); + subLine(pc.dim("Details:")); + console.log(pc.dim(indent(auth.raw, SUB_INDENT.length + 2))); } - console.log(""); +} - if (resources.length === 0) { - console.log("No resources declared."); - } else { - console.log("Resources"); - const ordered = [...resources].sort( - (a, b) => DISPLAY_ORDER[a.status] - DISPLAY_ORDER[b.status], +/** + * Prints one resource row. On the happy path this is just a tick and the + * resource name — no plugin/type attribution, no layer noise. Non-ok rows + * append their actionable layer details (and hints, offset by a blank line) + * beneath. Bundle-managed resources are noted as deploy-created, not probed. + */ +function printResourceRow(r: ResourceCheckResult): void { + const { target } = r; + if (target.origin === "bundle-managed") { + console.log( + ` ${pc.dim("⧗")} ${target.alias} ${pc.dim("will be created on deploy")}`, + ); + return; + } + console.log(` ${glyph(r.status)} ${target.alias}`); + const problems = r.layers.filter((l) => l.status !== "ok"); + if (problems.length > 0) { + printRowBody( + problems.map((l) => l.detail), + problems.map((l) => l.hint), ); - // When auth failed, every resource's existence probe is auth-skipped. Those - // rows carry no new information, so collapse the ones with no *other* - // finding into a single line — but keep any that also have a real problem - // (e.g. a missing env var), which is actionable regardless of auth. - const authSkipped = ordered.filter(isOnlyAuthSkipped); - const shown = ordered.filter((r) => !isOnlyAuthSkipped(r)); - - for (const r of shown) { - const { target } = r; - // Only attribute plugin · type on rows that need fixing. - const suffix = - r.status !== "ok" ? ` ${target.plugin} · ${target.type}` : ""; - console.log(` ${glyph(r.status)} ${target.alias}${suffix}`); - for (const layer of r.layers) { - if (layer.status === "ok") continue; - if (layer.detail) console.log(` ↳ ${layer.detail}`); - if (layer.hint) console.log(` Hint: ${layer.hint}`); - } - } - - if (authSkipped.length > 0) { - console.log( - ` • ${authSkipped.length} resource${authSkipped.length > 1 ? "s" : ""} not checked (auth failed)`, - ); - } } +} + +/** Prints a wiring finding with the same shape as a resource row: + * `glyph label` on the first line, then the indented detail and hint. */ +function printWiringRow(f: WiringFinding): void { + console.log(` ${glyph(f.status)} ${highlight(f.label)}`); + printRowBody([f.detail], [f.hint]); +} + +/** A unified printable row. `expanded` rows (those with detail/hint beneath the + * glyph line) get a blank line on either side so they read as their own + * paragraph; plain `ok` rows pack together. */ +interface Row { + status: CheckStatus; + expanded: boolean; + print: () => void; +} + +export function printReport(report: DoctorReport, detail = false): void { + const { resources, wiring, summary, auth } = report; + + // One flat, severity-sorted list — no titled sub-blocks. Auth is always the + // first row; every resource and wiring finding follows in the same list, so + // the report reads as a single consistent checklist rather than a mix of + // headed and headless sections. + const rows: Row[] = []; + + // Auth leads and never sorts. It expands only on failure. + rows.push({ + status: auth.status, + expanded: auth.status === "error", + print: () => printAuthRow(report, detail), + }); + + // When auth failed, external resources are all auth-skipped; those carry no + // new information, so collapse the ones whose *only* finding is the auth-skip + // into a single line (keeping any that also have a real, actionable problem). + const authSkipped = resources.filter(isOnlyAuthSkipped); + const checked: Row[] = []; + for (const r of resources) { + if (isOnlyAuthSkipped(r)) continue; + checked.push({ + status: r.status, + // Bundle-managed rows are a single line ("will be created on deploy"); + // every other non-ok row shows detail beneath, so it's expanded. + expanded: r.status !== "ok" && r.target.origin !== "bundle-managed", + print: () => printResourceRow(r), + }); + } + if (authSkipped.length > 0) { + checked.push({ + status: "skipped", + expanded: false, + print: () => + console.log( + ` ${pc.dim("•")} ${pc.dim( + `${authSkipped.length} resource${authSkipped.length > 1 ? "s" : ""} not checked (auth failed)`, + )}`, + ), + }); + } + for (const f of wiring) { + checked.push({ + status: f.status, + expanded: true, + print: () => printWiringRow(f), + }); + } + + // Sort everything after auth by severity; auth stays pinned at the top. + checked.sort((a, b) => DISPLAY_ORDER[a.status] - DISPLAY_ORDER[b.status]); + rows.push(...checked); + + // Print with a blank line separating any expanded row from its neighbours, so + // multi-line findings are set apart while consecutive ok rows stay compact. console.log(""); + rows.forEach((row, i) => { + const prev = rows[i - 1]; + if (i > 0 && (row.expanded || prev.expanded)) console.log(""); + row.print(); + }); - // Fold auth failure into the counts so it doesn't read as "0 errors". + // Fold auth failure and wiring findings into the counts so nothing that + // affects the exit code reads as "0 errors". const authError = auth.status === "error" ? 1 : 0; - console.log(summaryLine({ ...summary, error: summary.error + authError })); + const wiringErrors = wiring.filter((w) => w.status === "error").length; + const wiringWarns = wiring.filter((w) => w.status === "warn").length; + console.log(""); + console.log( + summaryLine({ + ...summary, + error: summary.error + authError + wiringErrors, + warn: summary.warn + wiringWarns, + }), + ); // Point at --detail when there's more to show and it wasn't requested. if (!detail && auth.raw) { - console.log("\nRun with --detail for full error output."); + console.log(pc.dim("\nRun with --detail for full error output.")); } console.log(""); } @@ -140,8 +292,10 @@ export function printReportJson(report: DoctorReport): void { console.log(JSON.stringify(report, null, 2)); } -/** Non-zero if auth or any resource errored, so `appkit doctor` can gate CI. */ +/** Non-zero if auth, any resource, or any wiring finding errored, so + * `appkit doctor` can gate CI / pre-deploy. */ export function exitCodeFor(report: DoctorReport): number { if (report.auth.status === "error") return 1; - return report.summary.error > 0 ? 1 : 0; + if (report.summary.error > 0) return 1; + return report.wiring.some((w) => w.status === "error") ? 1 : 0; } diff --git a/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts b/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts index c16c41e1f..1c433d7d5 100644 --- a/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts +++ b/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts @@ -23,6 +23,7 @@ const MANIFEST = { version: "2.0", plugins: { analytics: { + requiredByTemplate: true, resources: { required: [ { @@ -37,6 +38,7 @@ const MANIFEST = { }, }, agents: { + requiredByTemplate: true, resources: { required: [], optional: [ @@ -50,7 +52,10 @@ const MANIFEST = { ], }, }, - server: { resources: { required: [], optional: [] } }, + server: { + requiredByTemplate: true, + resources: { required: [], optional: [] }, + }, }, }; @@ -92,12 +97,76 @@ describe("targetsFromManifestFile", () => { }); }); + it("checks only requiredByTemplate plugins when any are marked", () => { + // analytics is marked (used in the app); agents is discovered-but-unused. + const targets = targetsFromManifestFile( + writeManifest({ + version: "2.0", + plugins: { + analytics: { + requiredByTemplate: true, + resources: { + required: [ + { + type: "sql_warehouse", + resourceKey: "sql-warehouse", + permission: "CAN_USE", + fields: { id: { env: "DATABRICKS_WAREHOUSE_ID" } }, + }, + ], + }, + }, + agents: { + // no requiredByTemplate → discovered but not used → skipped + resources: { + optional: [ + { + type: "serving_endpoint", + resourceKey: "serving", + permission: "CAN_QUERY", + fields: { name: { env: "DATABRICKS_SERVING_ENDPOINT_NAME" } }, + }, + ], + }, + }, + }, + }), + ); + expect(targets).toHaveLength(1); + expect(targets[0].plugin).toBe("analytics"); + }); + + it("returns nothing when no plugin is marked requiredByTemplate", () => { + // A manifest whose plugins are all discovered-but-unused → nothing to check. + const targets = targetsFromManifestFile( + writeManifest({ + version: "2.0", + plugins: { + analytics: { + resources: { + required: [ + { + type: "sql_warehouse", + resourceKey: "sql-warehouse", + permission: "CAN_USE", + fields: { id: { env: "DATABRICKS_WAREHOUSE_ID" } }, + }, + ], + }, + }, + }, + }), + ); + expect(targets).toEqual([]); + }); + it("excludes value-default and platform-injected fields from envVars (regression: bug #3)", () => { const targets = targetsFromManifestFile( writeManifest({ version: "2.0", plugins: { lakebase: { + requiredByTemplate: true, resources: { required: [ { @@ -146,6 +215,7 @@ describe("targetsFromManifestFile", () => { version: "2.0", plugins: { demo: { + requiredByTemplate: true, resources: { required: [ { @@ -178,6 +248,7 @@ describe("targetsFromManifestFile", () => { version: "2.0", plugins: { demo: { + requiredByTemplate: true, resources: { required: [ { diff --git a/packages/shared/src/cli/commands/doctor/resolve-targets.ts b/packages/shared/src/cli/commands/doctor/resolve-targets.ts index 935e8bf1a..e2e56fff6 100644 --- a/packages/shared/src/cli/commands/doctor/resolve-targets.ts +++ b/packages/shared/src/cli/commands/doctor/resolve-targets.ts @@ -34,6 +34,13 @@ interface ManifestPlugin { required?: ManifestResource[]; optional?: ManifestResource[]; }; + /** + * True when the plugin is actually used by the app (imported and passed to + * `createApp`). `plugin sync` discovers *every* plugin shipped by installed + * packages but only marks the used ones; doctor checks only those, so an + * unused built-in doesn't produce phantom "missing env var" errors. + */ + requiredByTemplate?: boolean; } interface TemplateManifest { @@ -115,8 +122,17 @@ export function targetsFromManifestFile( throw new Error(`Failed to parse manifest file ${manifestPath}: ${msg}`); } + // `plugin sync` writes a resource entry for *every* plugin the installed + // packages ship (a catalog for `apps init`), not just the ones this app uses. + // Only plugins actually wired into `createApp` are marked + // `requiredByTemplate`, so check exactly those — otherwise doctor reports + // phantom "missing env var" errors for plugins the app never imports. + const selected = Object.entries(data.plugins ?? {}).filter( + ([, plugin]) => plugin.requiredByTemplate === true, + ); + const targets: ResourceTarget[] = []; - for (const [pluginName, plugin] of Object.entries(data.plugins ?? {})) { + for (const [pluginName, plugin] of selected) { const resources = plugin.resources ?? {}; for (const resource of resources.required ?? []) { targets.push(toTarget(pluginName, resource, true)); diff --git a/packages/shared/src/cli/commands/doctor/run.test.ts b/packages/shared/src/cli/commands/doctor/run.test.ts index 5de53668d..73ef804d2 100644 --- a/packages/shared/src/cli/commands/doctor/run.test.ts +++ b/packages/shared/src/cli/commands/doctor/run.test.ts @@ -48,6 +48,7 @@ describe("runDoctor", () => { version: "2.0", plugins: { analytics: { + requiredByTemplate: true, resources: { required: [ { diff --git a/packages/shared/src/cli/commands/doctor/run.ts b/packages/shared/src/cli/commands/doctor/run.ts index 8d9a0ab6a..c965470b3 100644 --- a/packages/shared/src/cli/commands/doctor/run.ts +++ b/packages/shared/src/cli/commands/doctor/run.ts @@ -1,12 +1,17 @@ /** * Orchestration for `appkit doctor`. * - * Resolves the resources the app declares, runs the app-wide auth check once, - * then per resource runs the offline `config` layer followed by the live - * `existence` layer. Rolls everything up into a {@link DoctorReport}. + * Resolves the resources the app declares, overlays bundle provenance, runs the + * app-wide auth check once, then per resource runs the offline `config` layer + * followed by the live `existence` layer — except for bundle-managed resources, + * whose existence can't be probed before deploy. Separately runs the offline + * wiring check (deploy-declaration consistency). Rolls everything into a + * {@link DoctorReport}. */ +import { originForEnvVars, readBundleInfo } from "./bundle"; import { checkAuth, checkConfig, checkExistence } from "./checks"; +import { checkWiring } from "./checks-wiring"; import { resolveTargetsFromCwd } from "./resolve-targets"; import type { CheckStatus, @@ -44,6 +49,18 @@ async function checkResource( return { target, status: rolled, layers }; } + // A bundle-managed resource doesn't exist until `bundle deploy`, so probing it + // would be a false NOT_FOUND. Report it as deploy-managed instead. + if (target.origin === "bundle-managed") { + layers.push({ + layer: "existence", + status: "skipped", + code: "BUNDLE_MANAGED", + detail: "created by this bundle on deploy — not probed", + }); + return { target, status: worst(rolled, "skipped"), layers }; + } + if (client === undefined) { layers.push({ layer: "existence", @@ -69,9 +86,18 @@ export async function runDoctor(options: DoctorOptions): Promise { // Resources are resolved and config-checked even when auth failed, so a bad // connection still surfaces config problems instead of hiding them all. + const cwd = process.cwd(); + const targets = resolveTargetsFromCwd(cwd, options.manifest); + + // Overlay bundle provenance (Phase 1) and gather wiring info (Phase 2). + const bundle = readBundleInfo(cwd); + for (const target of targets) { + const origin = originForEnvVars(target.envVars, bundle); + if (origin) target.origin = origin; + } + // Probes are independent reads, so run them concurrently; Promise.all // preserves input order, keeping the report deterministic. - const targets = resolveTargetsFromCwd(process.cwd(), options.manifest); const results = await Promise.all( targets.map((target) => checkResource(target, client)), ); @@ -80,5 +106,7 @@ export async function runDoctor(options: DoctorOptions): Promise { summary[result.status] += 1; } - return { auth, resources, summary }; + const wiring = checkWiring(bundle, targets); + + return { auth, resources, wiring, summary }; } diff --git a/packages/shared/src/cli/commands/doctor/types.ts b/packages/shared/src/cli/commands/doctor/types.ts index 2f2e8ce07..c6967fbfc 100644 --- a/packages/shared/src/cli/commands/doctor/types.ts +++ b/packages/shared/src/cli/commands/doctor/types.ts @@ -5,6 +5,17 @@ export type CheckLayer = "auth" | "config" | "existence"; export type CheckStatus = "ok" | "warn" | "error" | "skipped"; +/** + * Where a resource's value comes from, per the bundle (`databricks.yml`): + * - `external` — an existing resource referenced by id/name (`${var.*}` or a + * literal). It should exist now, so the live probe applies. + * - `bundle-managed`— created by this same bundle (`${resources...*}`); + * it doesn't exist until `bundle deploy`, so we validate the + * declaration instead of probing. + * Undefined ⇒ external (no bundle info, or an AppKit app with no databricks.yml). + */ +export type ResourceOrigin = "external" | "bundle-managed"; + export interface LayerResult { layer: CheckLayer; status: CheckStatus; @@ -28,6 +39,8 @@ export interface ResourceTarget { envVars: string[]; /** Resolved field values keyed by manifest field name; unset fields omitted. */ fieldValues: Record; + /** Provenance from the bundle; undefined ⇒ treated as external. */ + origin?: ResourceOrigin; } export interface ResourceCheckResult { @@ -49,9 +62,29 @@ export interface AuthCheckResult { raw?: string; } +/** + * A finding from the offline three-file wiring check (Phase 2): does each + * `app.yaml` `valueFrom` bind to a real databricks.yml binding, and does each + * bundle-managed `${resources.*}` reference resolve to a declared bundle + * resource. Independent of auth — it's a deploy-declaration check, not a live one. + */ +export interface WiringFinding { + status: CheckStatus; + /** Machine-readable code (e.g. `VALUEFROM_UNBOUND`, `BUNDLE_REF_MISSING`). */ + code: string; + /** Short row label (the env var or binding at fault), so a wiring row renders + * with the same shape as a resource row: `glyph label` then indented detail. */ + label: string; + detail: string; + hint?: string; +} + export interface DoctorReport { auth: AuthCheckResult; + /** Live connectivity checks for external resources. */ resources: ResourceCheckResult[]; + /** Deploy-declaration findings: bundle-managed resources + wiring consistency. */ + wiring: WiringFinding[]; summary: { ok: number; warn: number; error: number; skipped: number }; } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6ecae34ed..b4e202b2e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -566,6 +566,9 @@ importers: dotenv: specifier: 16.6.1 version: 16.6.1 + picocolors: + specifier: 1.1.1 + version: 1.1.1 zod: specifier: 4.3.6 version: 4.3.6 From d8a4cbd79bdb080dbbf5536eaa4e659fd49dccf7 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Thu, 30 Jul 2026 13:30:41 +0200 Subject: [PATCH 06/14] test(shared): strip ANSI codes in doctor report test capture CI forces colour on, so picocolors wraps the summary line in escape codes; the exact-match and startsWith assertions failed against `\x1b[32m3 ok\x1b[39m`. Strip ANSI in the capture helper so assertions are colour-agnostic in any environment. Signed-off-by: Galymzhan --- packages/shared/src/cli/commands/doctor/report.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/shared/src/cli/commands/doctor/report.test.ts b/packages/shared/src/cli/commands/doctor/report.test.ts index 457d1a423..65c39d13f 100644 --- a/packages/shared/src/cli/commands/doctor/report.test.ts +++ b/packages/shared/src/cli/commands/doctor/report.test.ts @@ -12,11 +12,16 @@ function report(overrides: Partial = {}): DoctorReport { }; } -/** Runs `fn` with console.log captured; returns the printed lines. */ +// biome-ignore lint/suspicious/noControlCharactersInRegex: matching ANSI escapes +const ANSI = /\x1b\[[0-9;]*m/g; + +/** Runs `fn` with console.log captured; returns the printed lines with any ANSI + * colour codes stripped, so assertions are colour-agnostic (picocolors emits + * codes when CI forces colour / a TTY is present, and none otherwise). */ function capture(fn: () => void): string[] { const lines: string[] = []; const spy = vi.spyOn(console, "log").mockImplementation((msg?: unknown) => { - lines.push(String(msg)); + lines.push(String(msg).replace(ANSI, "")); }); try { fn(); From f955f067a28994fe6e7a607b60ceec1e7d7231a7 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Thu, 30 Jul 2026 13:46:08 +0200 Subject: [PATCH 07/14] =?UTF-8?q?fix(shared):=20address=20doctor=20review?= =?UTF-8?q?=20=E2=80=94=20declare=20js-yaml,=20fail=20on=20bad=20YAML,=20d?= =?UTF-8?q?rop=20--manifest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Act on code-review findings for the doctor command: - Declare js-yaml + @types/js-yaml in shared's package.json. bundle.ts imported js-yaml but only resolved via pnpm hoisting / being bundled into appkit — a fragile packaging gap. Declare it like dotenv/picocolors. - Surface malformed bundle YAML instead of swallowing it. readYaml now throws on a present-but-unparseable databricks.yml/app.yaml rather than returning null (which set present:false, skipping all wiring checks and reporting a false all-clear on deploy-breaking config). Absent files still degrade gracefully. - Remove the --manifest flag. Multiple plugin manifests is a non-scenario (appkit.plugins.json is generated at a fixed path), and the flag opened two footguns: a mistyped path silently checked nothing, and it could desync from the fixed bundle-file paths. Dropping it closes both. - Document the known limitation that used non-GA plugins aren't checked (plugin sync strips requiredByTemplate for non-GA), with a fast-follow note, in both the filter comment and the README. - Comment the deliberate, unrestored process.env mutation in getServiceClient (safe in a one-shot CLI). Adds tests for the malformed-YAML throw. Signed-off-by: Galymzhan --- packages/shared/package.json | 2 ++ .../shared/src/cli/commands/doctor/README.md | 15 +++++++++++++++ .../src/cli/commands/doctor/bundle.test.ts | 17 +++++++++++++++++ .../shared/src/cli/commands/doctor/bundle.ts | 13 +++++++++++-- .../cli/commands/doctor/databricks-client.ts | 5 +++++ .../shared/src/cli/commands/doctor/index.ts | 5 ----- .../src/cli/commands/doctor/resolve-targets.ts | 6 ++++++ packages/shared/src/cli/commands/doctor/run.ts | 2 +- .../shared/src/cli/commands/doctor/types.ts | 2 -- pnpm-lock.yaml | 6 ++++++ 10 files changed, 63 insertions(+), 10 deletions(-) diff --git a/packages/shared/package.json b/packages/shared/package.json index 0ea7644da..e155769ae 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -26,6 +26,7 @@ }, "devDependencies": { "@types/express": "4.17.23", + "@types/js-yaml": "4.0.9", "@types/json-schema": "7.0.15", "@types/node": "25.2.3", "@types/ws": "8.18.1" @@ -44,6 +45,7 @@ "@clack/prompts": "1.0.1", "commander": "12.1.0", "dotenv": "16.6.1", + "js-yaml": "4.2.0", "picocolors": "1.1.1", "zod": "4.3.6" } diff --git a/packages/shared/src/cli/commands/doctor/README.md b/packages/shared/src/cli/commands/doctor/README.md index f2db6dfc7..d22d34423 100644 --- a/packages/shared/src/cli/commands/doctor/README.md +++ b/packages/shared/src/cli/commands/doctor/README.md @@ -20,6 +20,21 @@ so the reported problem is the *root* cause, not a symptom. is the `existence` layer's job. (`DATABRICKS_HOST` is the exception — `auth` validates it structurally, since a bad host means no client can be built.) +## Which plugins are checked + +`plugin sync` writes `appkit.plugins.json` cataloguing *every* plugin the +installed packages ship (for `apps init`), and marks the ones actually wired +into `createApp` with `requiredByTemplate: true`. Doctor checks exactly those, so +an unused built-in doesn't produce phantom "missing env var" errors. + +> **Known limitation — non-GA plugins aren't checked yet.** `plugin sync` strips +> `requiredByTemplate` for beta/experimental plugins (its step 6b), so a *used* +> non-GA plugin (e.g. the agents plugin requiring a serving endpoint) is +> currently skipped. A proper fix needs a usage signal that survives sync +> regardless of stability tier (e.g. a separate `used` marker written before the +> strip); that's a `plugin sync` change tracked as a fast-follow. Until then, +> doctor covers GA plugins wired into your app. + ## Resource provenance: external vs bundle-managed Each resource is one of two kinds, which changes what doctor does with it: diff --git a/packages/shared/src/cli/commands/doctor/bundle.test.ts b/packages/shared/src/cli/commands/doctor/bundle.test.ts index 5f9ef0aaf..6fa9d1535 100644 --- a/packages/shared/src/cli/commands/doctor/bundle.test.ts +++ b/packages/shared/src/cli/commands/doctor/bundle.test.ts @@ -54,6 +54,23 @@ describe("readBundleInfo", () => { expect(info.bindings.size).toBe(0); }); + it("throws on malformed databricks.yml instead of treating it as absent", () => { + // A present-but-unparseable bundle is a deploy-breaking error; swallowing it + // would let doctor skip all wiring checks and report a false all-clear. + const dir = tmp({ "databricks.yml": "resources: [ this: is: not: valid" }); + dirs.push(dir); + expect(() => readBundleInfo(dir)).toThrow(/databricks\.yml/); + }); + + it("throws on malformed app.yaml", () => { + const dir = tmp({ + "databricks.yml": BUNDLE, + "app.yaml": "env:\n - name: X\n valueFrom: bad-indent", + }); + dirs.push(dir); + expect(() => readBundleInfo(dir)).toThrow(/app\.yaml/); + }); + it("classifies external (var) vs bundle-managed (resources ref) bindings", () => { const dir = tmp({ "databricks.yml": BUNDLE, "app.yaml": APP_YAML }); dirs.push(dir); diff --git a/packages/shared/src/cli/commands/doctor/bundle.ts b/packages/shared/src/cli/commands/doctor/bundle.ts index 7eb66c3b8..ce603fdfb 100644 --- a/packages/shared/src/cli/commands/doctor/bundle.ts +++ b/packages/shared/src/cli/commands/doctor/bundle.ts @@ -95,12 +95,21 @@ function classifyBinding(block: AppResourceBlock): { return { origin: "external" }; } +/** + * Reads and parses a YAML file. Returns `null` only when the file is *absent* + * (a legitimate "no bundle" state). A file that exists but fails to parse is a + * real, deploy-breaking error — swallowing it would let doctor skip every + * wiring check and treat bundle-managed resources as external, reporting a + * false all-clear — so it's rethrown with a clear message. + * @throws if `filePath` exists but contains invalid YAML. + */ function readYaml(filePath: string): T | null { if (!fs.existsSync(filePath)) return null; try { return (yaml.load(fs.readFileSync(filePath, "utf-8")) ?? {}) as T; - } catch { - return null; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to parse ${path.basename(filePath)}: ${msg}`); } } diff --git a/packages/shared/src/cli/commands/doctor/databricks-client.ts b/packages/shared/src/cli/commands/doctor/databricks-client.ts index ac393ecdf..783f05307 100644 --- a/packages/shared/src/cli/commands/doctor/databricks-client.ts +++ b/packages/shared/src/cli/commands/doctor/databricks-client.ts @@ -40,6 +40,11 @@ export async function getServiceClient( profile?: string, ): Promise { if (profile) { + // Deliberate, unrestored mutation: the SDK's unified auth reads the profile + // from this env var, and there's no per-call config seam for it. Safe here + // because doctor is a one-shot CLI that exits after a single run — this is + // not a long-lived process where a lingering profile could leak between + // operations. process.env[CONFIG_PROFILE_ENV] = profile; } diff --git a/packages/shared/src/cli/commands/doctor/index.ts b/packages/shared/src/cli/commands/doctor/index.ts index e85f37d79..4daa3316b 100644 --- a/packages/shared/src/cli/commands/doctor/index.ts +++ b/packages/shared/src/cli/commands/doctor/index.ts @@ -37,11 +37,6 @@ export const doctorCommand = new Command("doctor") .description( "Diagnose an AppKit app's Databricks resources: authentication, config, and resource existence/reachability", ) - .option( - "-m, --manifest ", - "Path to the resolved template manifest", - "appkit.plugins.json", - ) .option("-p, --profile ", "Databricks CLI profile to authenticate with") .option( "-e, --env-file ", diff --git a/packages/shared/src/cli/commands/doctor/resolve-targets.ts b/packages/shared/src/cli/commands/doctor/resolve-targets.ts index e2e56fff6..a0a6fb8e1 100644 --- a/packages/shared/src/cli/commands/doctor/resolve-targets.ts +++ b/packages/shared/src/cli/commands/doctor/resolve-targets.ts @@ -127,6 +127,12 @@ export function targetsFromManifestFile( // Only plugins actually wired into `createApp` are marked // `requiredByTemplate`, so check exactly those — otherwise doctor reports // phantom "missing env var" errors for plugins the app never imports. + // + // KNOWN LIMITATION: `plugin sync` strips `requiredByTemplate` for non-GA + // plugins (its step 6b), so a *used* beta/experimental plugin (e.g. agents + // requiring a serving endpoint) is currently NOT checked here. Fixing this + // needs a usage signal that survives sync regardless of stability — see the + // doctor README. Documented and deferred rather than papered over. const selected = Object.entries(data.plugins ?? {}).filter( ([, plugin]) => plugin.requiredByTemplate === true, ); diff --git a/packages/shared/src/cli/commands/doctor/run.ts b/packages/shared/src/cli/commands/doctor/run.ts index c965470b3..307ec91c6 100644 --- a/packages/shared/src/cli/commands/doctor/run.ts +++ b/packages/shared/src/cli/commands/doctor/run.ts @@ -87,7 +87,7 @@ export async function runDoctor(options: DoctorOptions): Promise { // Resources are resolved and config-checked even when auth failed, so a bad // connection still surfaces config problems instead of hiding them all. const cwd = process.cwd(); - const targets = resolveTargetsFromCwd(cwd, options.manifest); + const targets = resolveTargetsFromCwd(cwd); // Overlay bundle provenance (Phase 1) and gather wiring info (Phase 2). const bundle = readBundleInfo(cwd); diff --git a/packages/shared/src/cli/commands/doctor/types.ts b/packages/shared/src/cli/commands/doctor/types.ts index c6967fbfc..25f2e32ff 100644 --- a/packages/shared/src/cli/commands/doctor/types.ts +++ b/packages/shared/src/cli/commands/doctor/types.ts @@ -89,8 +89,6 @@ export interface DoctorReport { } export interface DoctorOptions { - /** Path to the resolved template manifest (defaults to appkit.plugins.json). */ - manifest?: string; profile?: string; json?: boolean; /** Show full underlying error messages (raw SDK output) in the human report. */ diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b4e202b2e..2e45b03b5 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -566,6 +566,9 @@ importers: dotenv: specifier: 16.6.1 version: 16.6.1 + js-yaml: + specifier: 4.2.0 + version: 4.2.0 picocolors: specifier: 1.1.1 version: 1.1.1 @@ -576,6 +579,9 @@ importers: '@types/express': specifier: 4.17.23 version: 4.17.23 + '@types/js-yaml': + specifier: 4.0.9 + version: 4.0.9 '@types/json-schema': specifier: 7.0.15 version: 7.0.15 From ce641bfc6c6a5a421380359786d9d4bae78bb849 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Thu, 30 Jul 2026 13:57:33 +0200 Subject: [PATCH 08/14] fix(shared): pass doctor profile via SDK config, not process.env mutation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior code set process.env.DATABRICKS_CONFIG_PROFILE to apply an explicit --profile — a hidden, unrestored global side effect. The SDK's own Config exposes a first-class `profile` field, so pass it straight into new WorkspaceClient({ profile }) instead. No env mutation, no leak past the call, and it correctly overrides an ambient DATABRICKS_CONFIG_PROFILE. Verified against the real SDK. Signed-off-by: Galymzhan --- .../cli/commands/doctor/databricks-client.ts | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/packages/shared/src/cli/commands/doctor/databricks-client.ts b/packages/shared/src/cli/commands/doctor/databricks-client.ts index 783f05307..cdf886944 100644 --- a/packages/shared/src/cli/commands/doctor/databricks-client.ts +++ b/packages/shared/src/cli/commands/doctor/databricks-client.ts @@ -31,23 +31,14 @@ function isModuleNotFound(err: unknown): boolean { ); } -/** Env var the Databricks SDK's unified auth reads to select a CLI profile. */ -const CONFIG_PROFILE_ENV = "DATABRICKS_CONFIG_PROFILE"; - -/** Constructs a `WorkspaceClient` via the SDK's unified-auth chain. `profile` - * is applied through the env var the SDK reads. */ +/** Constructs a `WorkspaceClient` via the SDK's unified-auth chain. An explicit + * `profile` is passed through the SDK's own `Config.profile` field — a per-call + * seam, so we never mutate `process.env` (which would leak the profile beyond + * this call and race with anything else reading the environment). When omitted, + * the SDK falls back to its normal chain (env, then the default profile). */ export async function getServiceClient( profile?: string, ): Promise { - if (profile) { - // Deliberate, unrestored mutation: the SDK's unified auth reads the profile - // from this env var, and there's no per-call config seam for it. Safe here - // because doctor is a one-shot CLI that exits after a single run — this is - // not a long-lived process where a lingering profile could leak between - // operations. - process.env[CONFIG_PROFILE_ENV] = profile; - } - // Narrowed to the one call we make; `shared` has no static SDK dependency. let sdk: { WorkspaceClient: new (opts: Record) => unknown }; try { @@ -59,7 +50,7 @@ export async function getServiceClient( throw err; } - const client = new sdk.WorkspaceClient({}); + const client = new sdk.WorkspaceClient(profile ? { profile } : {}); return { client }; } From f2d313902628a3d0d0c60aa070000c35125daaad Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Fri, 31 Jul 2026 10:16:47 +0200 Subject: [PATCH 09/14] fix(shared): redact secrets from doctor report and --json Address a security finding: sensitive data could reach captured output. - Strip URL userinfo (user:pass@) from DATABRICKS_HOST at capture, so credentials embedded in the host never reach the report or --json. Validation still runs on the raw value. - Gate auth.raw in --json behind --detail, matching the human report. A bare --json omits the raw SDK error (CI commonly captures --json); --json --detail opts back in. Also folds in pending doctor cleanups the tree already carried and that these changes build on: an errorMessage() helper in a new utils.ts, and STATUS_SEVERITY / AUTH_UNAVAILABLE_CODE constants shared via types.ts to stop run.ts and report.ts drifting. Adds tests for host sanitization and the --json raw gate (92 total). Signed-off-by: Galymzhan --- .../shared/src/cli/commands/doctor/README.md | 8 +- .../shared/src/cli/commands/doctor/bundle.ts | 54 ++++--- .../commands/doctor/checks-existence.test.ts | 12 +- .../cli/commands/doctor/checks-existence.ts | 37 +++-- .../cli/commands/doctor/checks-wiring.test.ts | 3 - .../src/cli/commands/doctor/checks-wiring.ts | 15 +- .../src/cli/commands/doctor/checks.test.ts | 45 ++++-- .../shared/src/cli/commands/doctor/checks.ts | 100 ++++++------- .../cli/commands/doctor/databricks-client.ts | 18 +-- .../shared/src/cli/commands/doctor/index.ts | 8 +- .../src/cli/commands/doctor/report.test.ts | 27 ++++ .../shared/src/cli/commands/doctor/report.ts | 138 +++++++----------- .../commands/doctor/resolve-targets.test.ts | 6 +- .../cli/commands/doctor/resolve-targets.ts | 29 ++-- .../shared/src/cli/commands/doctor/run.ts | 64 +++----- .../shared/src/cli/commands/doctor/types.ts | 53 +++---- .../shared/src/cli/commands/doctor/utils.ts | 6 + 17 files changed, 297 insertions(+), 326 deletions(-) create mode 100644 packages/shared/src/cli/commands/doctor/utils.ts diff --git a/packages/shared/src/cli/commands/doctor/README.md b/packages/shared/src/cli/commands/doctor/README.md index d22d34423..09cb4309d 100644 --- a/packages/shared/src/cli/commands/doctor/README.md +++ b/packages/shared/src/cli/commands/doctor/README.md @@ -113,7 +113,11 @@ back to the default profile. Doctor emits: hint says what to *run* and to confirm the profile/host actually in use — a failure can mean the wrong target, not just a stale token. `detail` is the short headline `authentication failed`; the full SDK message is kept in `raw` and shown -only with `--detail` (always in `--json`). The report labels hints `Hint:`. +only with `--detail` — in both the human report and `--json` (i.e. `--json` +alone omits `raw`, since it can carry sensitive detail and CI often captures it; +`--json --detail` opts back in). `DATABRICKS_HOST` is stored with any embedded +`user:pass@` userinfo stripped, so credentials never reach the report or JSON. +The report labels hints `Hint:`. ## Files @@ -124,7 +128,7 @@ only with `--detail` (always in `--json`). The report labels hints `Hint:`. - `databricks-client.ts` — the sole SDK seam: dynamic `import()` of the SDK / `@databricks/appkit`, builds a `WorkspaceClient` and Lakebase pool, graceful fallback when uninstalled. -- `checks.ts` — `checkAuth`, `checkConfig`, `checkExistence`. +- `checks.ts` — `checkAuth`, `checkConfig`. - `checks-existence.ts` — per-type existence probe dispatch + error classifier. - `checks-wiring.ts` — offline three-file join / bundle-ref consistency check. - `run.ts` — orchestration: auth once → overlay origin → per resource diff --git a/packages/shared/src/cli/commands/doctor/bundle.ts b/packages/shared/src/cli/commands/doctor/bundle.ts index ce603fdfb..1d450cdf1 100644 --- a/packages/shared/src/cli/commands/doctor/bundle.ts +++ b/packages/shared/src/cli/commands/doctor/bundle.ts @@ -1,17 +1,12 @@ /** - * Reads the Databricks Asset Bundle (`databricks.yml`) and `app.yaml` to learn - * two things doctor can't get from `appkit.plugins.json`: + * Reads `databricks.yml` + `app.yaml` for two things doctor can't get from + * `appkit.plugins.json`: * - * 1. **Provenance** — whether each app resource binding references an *existing* - * resource (`${var.*}` or a literal → `external`) or one this bundle - * *creates* (`${resources...*}` → `bundle-managed`). A - * bundle-managed resource doesn't exist until `bundle deploy`, so doctor - * must not probe it (that would be a false NOT_FOUND). - * - * 2. **Wiring** — the join key that ties the three files together is the - * binding *name*: `app.yaml`'s `valueFrom: ` must match a - * `databricks.yml` binding `name`, which an AppKit plugin then reads via an - * env var. We surface the pieces so the wiring check can validate the join. + * 1. **Provenance** — whether each binding references an existing resource + * (`${var.*}`/literal → `external`) or one this bundle creates + * (`${resources...*}` → `bundle-managed`). + * 2. **Wiring** — the binding `name` joins the three files: `app.yaml`'s + * `valueFrom: ` must match a `databricks.yml` binding `name`. * * SDK-free; pure YAML parsing. Absent files degrade to empty results. */ @@ -20,6 +15,7 @@ import fs from "node:fs"; import path from "node:path"; import yaml from "js-yaml"; import type { ResourceOrigin } from "./types"; +import { errorMessage } from "./utils"; export const DEFAULT_BUNDLE_FILE = "databricks.yml"; export const DEFAULT_APP_YAML_FILE = "app.yaml"; @@ -75,9 +71,11 @@ function bindingType(block: AppResourceBlock): string | undefined { return undefined; } -/** Classifies a binding's origin by scanning its field values for a - * `${resources.*}` reference (bundle-managed) vs anything else (external). */ +/** Classifies a binding by its typed sub-key and origin: scanning its field + * values for a `${resources.*}` reference (bundle-managed) vs anything else + * (external). */ function classifyBinding(block: AppResourceBlock): { + type?: string; origin: ResourceOrigin; ref?: { type: string; key: string }; } { @@ -88,28 +86,30 @@ function classifyBinding(block: AppResourceBlock): { if (typeof value !== "string") continue; const m = value.match(RESOURCES_REF); if (m) { - return { origin: "bundle-managed", ref: { type: m[1], key: m[2] } }; + return { + type, + origin: "bundle-managed", + ref: { type: m[1], key: m[2] }, + }; } } } - return { origin: "external" }; + return { type, origin: "external" }; } /** - * Reads and parses a YAML file. Returns `null` only when the file is *absent* - * (a legitimate "no bundle" state). A file that exists but fails to parse is a - * real, deploy-breaking error — swallowing it would let doctor skip every - * wiring check and treat bundle-managed resources as external, reporting a - * false all-clear — so it's rethrown with a clear message. - * @throws if `filePath` exists but contains invalid YAML. + * Reads and parses a YAML file. Returns `null` when the file is absent (a + * legitimate "no bundle" state); throws on invalid YAML, since silently + * ignoring it would let doctor report a false all-clear. */ function readYaml(filePath: string): T | null { if (!fs.existsSync(filePath)) return null; try { return (yaml.load(fs.readFileSync(filePath, "utf-8")) ?? {}) as T; } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - throw new Error(`Failed to parse ${path.basename(filePath)}: ${msg}`); + throw new Error( + `Failed to parse ${path.basename(filePath)}: ${errorMessage(err)}`, + ); } } @@ -132,21 +132,19 @@ export function readBundleInfo( return { bindings, envToBinding, declaredResources, present: false }; } - // Every declared bundle resource, as "." (apps included). for (const [type, group] of Object.entries(doc.resources ?? {})) { if (!group || typeof group !== "object") continue; for (const key of Object.keys(group)) declaredResources.add(`${type}.${key}`); } - // App resource bindings across every app in the bundle. for (const app of Object.values(doc.resources?.apps ?? {})) { for (const block of app.resources ?? []) { if (!block?.name) continue; - const { origin, ref } = classifyBinding(block); + const { type, origin, ref } = classifyBinding(block); bindings.set(block.name, { name: block.name, - type: bindingType(block), + type, origin, ref, }); diff --git a/packages/shared/src/cli/commands/doctor/checks-existence.test.ts b/packages/shared/src/cli/commands/doctor/checks-existence.test.ts index c338a520f..7fe559d2b 100644 --- a/packages/shared/src/cli/commands/doctor/checks-existence.test.ts +++ b/packages/shared/src/cli/commands/doctor/checks-existence.test.ts @@ -2,8 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { runExistenceProbe, toThreeLevelVolumeName } from "./checks-existence"; import type { ResourceTarget } from "./types"; -// Mock the SDK bridge so the postgres probe can be driven without a real -// Lakebase connection. `getLakebasePool` returns a fake pool per test. +// Mock the SDK bridge so the postgres probe runs without a real connection. const { mockGetLakebasePool, endSpy } = vi.hoisted(() => ({ mockGetLakebasePool: vi.fn(), endSpy: vi.fn(async () => {}), @@ -76,8 +75,6 @@ describe("runExistenceProbe — sql_warehouse", () => { }, }, }; - // The target has an id, so we own the wording: quote the value, don't echo - // the SDK sentence, don't leak the raw type or JSON blob. const r = await runExistenceProbe( client, target({ fieldValues: { id: "wh-123" } }), @@ -191,7 +188,6 @@ describe("runExistenceProbe — postgres (Lakebase)", () => { const r = await runExistenceProbe({}, pgTarget()); expect(r.status).toBe("error"); expect(r.code).toBe("CONNECTION_FAILED"); - // The raw error stays in detail; the actionable guidance is a separate hint. expect(r.detail).toMatch(/password authentication failed/i); expect(r.hint).toMatch(/OAuth token as the password/i); expect(r.hint).toMatch(/PGUSER/); @@ -211,14 +207,10 @@ describe("runExistenceProbe — postgres (Lakebase)", () => { const r = await runExistenceProbe({}, pgTarget({ fieldValues: {} })); expect(r.status).toBe("skipped"); expect(r.code).toBe("MISSING_FIELD"); - // Pool was never created. expect(mockGetLakebasePool).not.toHaveBeenCalled(); }); }); -// These use the real first-party manifest field keys (e.g. vector-search's -// camelCase `indexName`). Each supported probe gets a happy path + a -// missing-field skip. describe("runExistenceProbe — per-type coverage with real manifest keys", () => { it("serving_endpoint: ok via `name`", async () => { const client = { servingEndpoints: { get: async () => ({}) } }; @@ -294,7 +286,7 @@ describe("runExistenceProbe — per-type coverage with real manifest keys", () = expect(r.status).toBe("ok"); }); - it("vector_search_index: probes via camelCase `indexName` (regression: bug #1)", async () => { + it("vector_search_index: probes via camelCase `indexName`", async () => { const getIndex = vi.fn(async () => ({})); const client = { vectorSearchIndexes: { getIndex } }; const r = await runExistenceProbe( diff --git a/packages/shared/src/cli/commands/doctor/checks-existence.ts b/packages/shared/src/cli/commands/doctor/checks-existence.ts index 6d2f5f929..71e29112e 100644 --- a/packages/shared/src/cli/commands/doctor/checks-existence.ts +++ b/packages/shared/src/cli/commands/doctor/checks-existence.ts @@ -11,6 +11,10 @@ import { type LakebasePoolHandle, } from "./databricks-client"; import type { LayerResult, ResourceTarget } from "./types"; +import { errorMessage } from "./utils"; + +/** A passing existence check. Shared, never mutated by callers. */ +const EXISTENCE_OK: LayerResult = { layer: "existence", status: "ok" }; interface DoctorWorkspaceClient { warehouses: { @@ -41,8 +45,7 @@ type ExistenceProbe = ( target: ResourceTarget, ) => Promise; -// Read statusCode/errorCode off the SDK's ApiError structurally, keeping -// `shared` SDK-free. +// Read statusCode/errorCode off the SDK's ApiError structurally. function statusCodeOf(err: unknown): number | undefined { if (err && typeof err === "object" && "statusCode" in err) { const code = (err as { statusCode?: unknown }).statusCode; @@ -62,15 +65,13 @@ function errorCodeOf(err: unknown): string | undefined { // The SDK message often embeds a JSON blob; pull the inner `message` out so // doctor prints one clean line, not a dump. function cleanMessage(err: unknown): string { - const raw = err instanceof Error ? err.message : String(err); + const raw = errorMessage(err); const m = raw.match(/"message"\s*:\s*"([^"]+)"/); return m ? m[1] : raw; } /** The resource's configured identifier (id / name / path / host), for short - * messages. The row label already names the *type*, so details never repeat it - * — they only surface the value you'd act on, quoted so the report highlights - * it. */ + * messages. */ function displayId(target: ResourceTarget): string | null { return field(target, "id", "name", "path", "index_name", "indexName", "host"); } @@ -90,9 +91,7 @@ function classifyError(err: unknown, target: ResourceTarget): LayerResult { detail: quoted ? `${quoted} not found` : "not found", }; } - // A malformed id/name often comes back as 400 rather than 404. We own the - // wording (quoting the value so it highlights) rather than echoing the SDK's - // sentence, which repeats "invalid" and leaks the raw type. + // A malformed id/name often comes back as 400 rather than 404. if (status === 400 || errorCode === "INVALID_PARAMETER_VALUE") { return { layer: "existence", @@ -161,7 +160,7 @@ const probeWarehouse: ExistenceProbe = async (client, target) => { detail: `warehouse exists but is ${state} (will cold-start on first query)`, }; } - return { layer: "existence", status: "ok" }; + return EXISTENCE_OK; } catch (err) { return classifyError(err, target); } @@ -169,14 +168,14 @@ const probeWarehouse: ExistenceProbe = async (client, target) => { const probeServing: ExistenceProbe = async (client, target) => { const name = field(target, "name"); - // The API only accepts a name; if configured by id we probe with it anyway - // and flag the likely cause on failure. + // The API only accepts a name; if configured by id, probe with it anyway and + // flag the likely cause on failure. const idOnly = name === null ? field(target, "id") : null; const value = name ?? idOnly; if (!value) return missingField("name"); try { await client.servingEndpoints.get({ name: value }); - return { layer: "existence", status: "ok" }; + return EXISTENCE_OK; } catch (err) { const result = classifyError(err, target); if (idOnly && result.status === "error") { @@ -192,7 +191,7 @@ const probeGenie: ExistenceProbe = async (client, target) => { if (!spaceId) return missingField("id"); try { await client.genie.getSpace({ space_id: spaceId }); - return { layer: "existence", status: "ok" }; + return EXISTENCE_OK; } catch (err) { return classifyError(err, target); } @@ -212,7 +211,7 @@ const probeJob: ExistenceProbe = async (client, target) => { } try { await client.jobs.get({ job_id: jobId }); - return { layer: "existence", status: "ok" }; + return EXISTENCE_OK; } catch (err) { return classifyError(err, target); } @@ -234,7 +233,7 @@ const probeVolume: ExistenceProbe = async (client, target) => { } try { await client.volumes.read({ name }); - return { layer: "existence", status: "ok" }; + return EXISTENCE_OK; } catch (err) { return classifyError(err, target); } @@ -245,7 +244,7 @@ const probeVectorIndex: ExistenceProbe = async (client, target) => { if (!name) return missingField("indexName"); try { await client.vectorSearchIndexes.getIndex({ index_name: name }); - return { layer: "existence", status: "ok" }; + return EXISTENCE_OK; } catch (err) { return classifyError(err, target); } @@ -256,7 +255,7 @@ const probeFunction: ExistenceProbe = async (client, target) => { if (!name) return missingField("name"); try { await client.functions.get({ name }); - return { layer: "existence", status: "ok" }; + return EXISTENCE_OK; } catch (err) { return classifyError(err, target); } @@ -284,7 +283,7 @@ const probePostgres: ExistenceProbe = async (client, target) => { try { pool = await getLakebasePool(client); await pool.query("SELECT 1"); - return { layer: "existence", status: "ok" }; + return EXISTENCE_OK; } catch (err) { if (err instanceof AppkitNotInstalledError) { return { diff --git a/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts b/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts index 84620f7c5..ee2b0d405 100644 --- a/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts +++ b/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts @@ -108,9 +108,6 @@ describe("checkWiring", () => { }); it("warns even when app.yaml has NO env block (used plugin, zero wiring)", () => { - // The 'works locally, crashes on deploy' case: analytics is used and its - // env var is set locally via .env, but app.yaml provides nothing, so the - // deployed container has no way to inject it. const findings = checkWiring( info({ present: true }), // empty bindings + empty envToBinding [ diff --git a/packages/shared/src/cli/commands/doctor/checks-wiring.ts b/packages/shared/src/cli/commands/doctor/checks-wiring.ts index af0633f49..91e398192 100644 --- a/packages/shared/src/cli/commands/doctor/checks-wiring.ts +++ b/packages/shared/src/cli/commands/doctor/checks-wiring.ts @@ -15,10 +15,6 @@ import type { BundleInfo } from "./bundle"; import type { ResourceTarget, WiringFinding } from "./types"; -/** - * @param info parsed bundle + app.yaml wiring - * @param targets resolved resource targets (for env-var context in messages) - */ export function checkWiring( info: BundleInfo, targets: ResourceTarget[], @@ -55,14 +51,9 @@ export function checkWiring( } } - // 3. A used plugin needs an env var that no app.yaml entry provides. Locally - // the value comes from .env, but .env isn't uploaded — on deploy the platform - // only injects what app.yaml maps. So a used plugin whose env var has no - // app.yaml entry will be *unset in the deployed container* and crash at - // runtime, even though local checks are green. This is the headline - // "works locally, breaks on deploy" case, so it fires whenever the app is a - // bundle (databricks.yml present) — including when app.yaml has no env block - // at all, which is the most broken state, not a "doesn't use injection" one. + // 3. A used plugin's env var with no app.yaml entry: set locally via .env, but + // .env isn't uploaded, so it's unset in the deployed container — the headline + // "works locally, breaks on deploy" case. for (const target of targets) { for (const envVar of target.envVars) { if (!info.envToBinding.has(envVar)) { diff --git a/packages/shared/src/cli/commands/doctor/checks.test.ts b/packages/shared/src/cli/commands/doctor/checks.test.ts index 81bfe2b77..136310388 100644 --- a/packages/shared/src/cli/commands/doctor/checks.test.ts +++ b/packages/shared/src/cli/commands/doctor/checks.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { checkAuth, checkConfig, validateHost } from "./checks"; +import { checkAuth, checkConfig, sanitizeHost, validateHost } from "./checks"; import type { ResourceTarget } from "./types"; const { mockGetServiceClient } = vi.hoisted(() => ({ @@ -49,8 +49,6 @@ describe("checkConfig", () => { }); it("passes an unfilled-looking value (existence check is the authority)", async () => { - // Placeholder-looking values are NOT flagged here by design — the live - // existence check decides whether a set value is real. process.env.DOCTOR_TEST_ENV = "your_sql_warehouse_id"; const result = await checkConfig(target()); expect(result.status).toBe("ok"); @@ -108,6 +106,28 @@ describe("validateHost", () => { }); }); +describe("sanitizeHost", () => { + it("passes through an unset or credential-free host unchanged", () => { + expect(sanitizeHost(undefined)).toBeUndefined(); + expect(sanitizeHost("https://foo.cloud.databricks.com")).toBe( + "https://foo.cloud.databricks.com", + ); + }); + + it("strips embedded userinfo (user:pass@) so it can't leak", () => { + const cleaned = sanitizeHost( + "https://user:secret@workspace.databricks.com", + ); + expect(cleaned).not.toContain("secret"); + expect(cleaned).not.toContain("user"); + expect(cleaned).toContain("workspace.databricks.com"); + }); + + it("leaves a non-URL value as-is (nothing to strip)", () => { + expect(sanitizeHost("not-a-url")).toBe("not-a-url"); + }); +}); + describe("checkAuth", () => { const savedHost = process.env.DATABRICKS_HOST; @@ -129,6 +149,17 @@ describe("checkAuth", () => { expect(returned).toBe(client); // handed to the live layers }); + it("stores the sanitized host (no embedded credentials) in the result", async () => { + process.env.DATABRICKS_HOST = + "https://user:secret@foo.cloud.databricks.com"; + const client = { currentUser: { me: async () => ({ userName: "sp-1" }) } }; + mockGetServiceClient.mockResolvedValue({ client }); + + const { result } = await checkAuth({}); + expect(result.host).not.toContain("secret"); + expect(result.host).toContain("foo.cloud.databricks.com"); + }); + it("errors HOST_INVALID before touching the SDK", async () => { process.env.DATABRICKS_HOST = "https://..."; const { result, client } = await checkAuth({}); @@ -170,8 +201,6 @@ describe("checkAuth", () => { ); const { result } = await checkAuth({ profile: "prod" }); - // Action-first: tells the user what to run and to confirm the target, - // without redundantly repeating the profile name (it's in the command). expect(result.hint).toContain("databricks auth login --profile prod"); expect(result.hint).toMatch(/confirm the profile\/host/i); }); @@ -209,10 +238,9 @@ describe("checkAuth", () => { ), ); - // No --profile given, but the SDK fell back to DEFAULT — the hint must name it. const { result } = await checkAuth({}); expect(result.hint).toContain("databricks auth login --profile DEFAULT"); - // Not the bare, profile-less command (which would reauth the wrong profile). + // Not the bare command, which would reauth the wrong profile. expect(result.hint).not.toContain("`databricks auth login`"); }); @@ -235,7 +263,6 @@ describe("checkAuth", () => { ); const { result } = await checkAuth({}); - // No profile/host known → generic login command + confirm-the-target note. expect(result.hint).toContain("databricks auth login"); expect(result.hint).toMatch( /confirm the profile\/host is the one you intend/i, @@ -248,9 +275,7 @@ describe("checkAuth", () => { mockGetServiceClient.mockRejectedValue(new Error(sprawling)); const { result } = await checkAuth({}); - // Human report headline is always the short, constant string... expect(result.detail).toBe("authentication failed"); - // ...while the full multi-line error is preserved verbatim in raw. expect(result.raw).toBe(sprawling); expect(result.raw).toContain("first line of the failure"); expect(result.raw).toContain("x".repeat(300)); diff --git a/packages/shared/src/cli/commands/doctor/checks.ts b/packages/shared/src/cli/commands/doctor/checks.ts index 2e9e47a42..822369050 100644 --- a/packages/shared/src/cli/commands/doctor/checks.ts +++ b/packages/shared/src/cli/commands/doctor/checks.ts @@ -3,7 +3,6 @@ * probes live in `checks-existence.ts`.) */ -import { runExistenceProbe } from "./checks-existence"; import { getServiceClient, SdkNotInstalledError } from "./databricks-client"; import type { AuthCheckResult, @@ -11,9 +10,9 @@ import type { LayerResult, ResourceTarget, } from "./types"; +import { errorMessage } from "./utils"; -/** The auth result plus the resolved client (present only on success), which - * the orchestrator hands to the live layers so they don't rebuild it. */ +/** The auth result plus the resolved client (present only on success). */ export interface AuthOutcome { result: AuthCheckResult; client?: unknown; @@ -26,10 +25,27 @@ interface CurrentUserClient { } /** - * Validates `DATABRICKS_HOST` before we hand it to the SDK, so an unfilled - * placeholder (e.g. `https://...`) gets a clear message instead of the SDK's - * opaque "cannot configure default credentials" error. Returns an error - * message, or null when the host is acceptable or unset. + * Strips URL userinfo (`user:pass@`) from a host, so credentials someone + * embedded in `DATABRICKS_HOST` never reach the report or `--json`. Returns the + * input unchanged when it has no userinfo or isn't a parseable URL. + */ +export function sanitizeHost(host: string | undefined): string | undefined { + if (host === undefined) return undefined; + try { + const url = new URL(host); + if (!url.username && !url.password) return host; + url.username = ""; + url.password = ""; + return url.toString(); + } catch { + return host; + } +} + +/** + * Validates `DATABRICKS_HOST` before the SDK sees it, so an unfilled placeholder + * gets a clear message instead of the SDK's opaque credentials error. Returns an + * error message, or null when the host is acceptable or unset. */ export function validateHost(host: string | undefined): string | null { if (host === undefined || host.trim().length === 0) return null; @@ -45,8 +61,7 @@ export function validateHost(host: string | undefined): string | null { return `DATABRICKS_HOST must be an http(s) URL: "${host}"`; } - // Placeholders like "https://..." parse as a URL but have a hostname with no - // real (dotted, alphanumeric) label. + // Placeholders like "https://..." parse but have no real dotted label. const hostname = url.hostname; const hasRealLabel = /[a-z0-9]/i.test(hostname) && hostname.includes("."); if (!hasRealLabel || /^[.\-_]+$/.test(hostname)) { @@ -61,11 +76,14 @@ export function validateHost(host: string | undefined): string | null { * existence check (they all need the client). */ export async function checkAuth(options: DoctorOptions): Promise { - const host = process.env.DATABRICKS_HOST; - // What the report shows as the identity source: an explicit --profile, else - // the env var the SDK would pick up, so "which profile?" is never a mystery. + // Validate the raw value, but only ever store/report the sanitized one so + // credentials embedded in the URL (user:pass@) never reach the report/--json. + const rawHost = process.env.DATABRICKS_HOST; + const host = sanitizeHost(rawHost); + // The identity source shown in the report: explicit --profile, else the env + // var the SDK would pick up. const profile = options.profile ?? process.env.DATABRICKS_CONFIG_PROFILE; - const hostError = validateHost(host); + const hostError = validateHost(rawHost); if (hostError) { return { result: { @@ -104,21 +122,19 @@ export async function checkAuth(options: DoctorOptions): Promise { }, }; } - const raw = err instanceof Error ? err.message : String(err); - // If we didn't know the profile up front, recover the one the SDK actually - // resolved (embedded in its error) and mark it "(resolved)" so the header - // reveals which profile was used — otherwise a default fallback is invisible. + const raw = errorMessage(err); + // When no profile was given up front, recover the one the SDK resolved (it's + // embedded in the error) and mark it "(resolved)". const sdkProfile = raw.match(/--profile\s+(\S+)/)?.[1]; + const usedProfile = profile ?? sdkProfile; const shownProfile = profile ?? (sdkProfile ? `${sdkProfile} (resolved)` : undefined); return { result: { status: "error", code: "AUTH_FAILED", - // Keep the headline short and let the hint explain the fix; the raw SDK - // message is carried separately for `--detail` / `--json`. detail: "authentication failed", - hint: authFailureHint(raw, profile), + hint: authFailureHint(raw, usedProfile), host, profile: shownProfile, raw, @@ -128,31 +144,22 @@ export async function checkAuth(options: DoctorOptions): Promise { } /** - * Suggests a next action for common auth failures. Hints are phrased as - * something to *do* (not a restatement of the error) and, since a failure may - * mean the wrong profile/host is in play, tell the user to confirm which - * profile/host they're targeting. Patterns are matched most specific first; - * returns undefined for an unrecognized failure. + * Suggests a next action for common auth failures, matched most specific first. + * Returns undefined for an unrecognized failure. + * + * `profile` is the explicit --profile, else the one the SDK resolved (DEFAULT / + * DATABRICKS_CONFIG_PROFILE) and embedded in its error, so the login hint + * targets what actually failed. */ function authFailureHint( message: string, profile?: string, ): string | undefined { - // The SDK resolves a profile even when the user gave none (falling back to - // DEFAULT / DATABRICKS_CONFIG_PROFILE), and its error embeds that name (e.g. - // "databricks auth login --profile DEFAULT"). Prefer the explicit profile, - // then the one the SDK actually used, so the hint points at the profile that - // truly failed — not a bare `databricks auth login` that reauths the wrong one. - const usedProfile = profile ?? message.match(/--profile\s+(\S+)/)?.[1]; - const loginCmd = usedProfile - ? `databricks auth login --profile ${usedProfile}` + const loginCmd = profile + ? `databricks auth login --profile ${profile}` : "databricks auth login"; - // The profile/host in use is already shown in the report header and in the - // login command, so hints don't repeat it — they just nudge the user to - // sanity-check the target rather than blindly re-logging-in. - // Network-level failure: the host is unreachable (bad workspace URL, DNS, - // offline, or TLS). Matched first — it's more specific than a credential - // problem and needs a different fix. + // Network-level failure (unreachable host, DNS, TLS). Matched first — more + // specific than a credential problem and needs a different fix. if ( /ENOTFOUND|EAI_AGAIN|ECONNREFUSED|ECONNRESET|ETIMEDOUT|getaddrinfo|certificate|self.signed|unable to verify|TLS/i.test( message, @@ -160,7 +167,6 @@ function authFailureHint( ) { return "Check that the workspace host is correct and reachable (verify DATABRICKS_HOST or the profile's host, and that you're online)."; } - // "resolve: ~/.databrickscfg has no profile configured" if ( /has no .* profile configured|profile .* (does not exist|not found)/i.test( message, @@ -168,8 +174,7 @@ function authFailureHint( ) { return `Run \`${loginCmd}\`, or pass an existing profile via --profile.`; } - // Expired/failed CLI token, or no credentials resolved by any method — both - // point at the same next action: log in and verify the target. + // Expired/failed token, or no credentials resolved: same next action. if ( /cannot get access token|refresh token|reauthenticate|databricks auth token|token .*expired|cannot configure default credentials|default auth|no .*credentials/i.test( message, @@ -203,9 +208,6 @@ export async function checkConfig( layer: "config", status: target.required ? "error" : "warn", code: target.required ? "ENV_MISSING" : "ENV_MISSING_OPTIONAL", - // The env var name(s) do the work — the report bolds SCREAMING_SNAKE - // names. Optional resources say so: a missing optional is a warn, not a - // blocker. detail: target.required ? `${names} ${plural ? "are" : "is"} not set` : `${names} ${plural ? "are" : "is"} not set (optional)`, @@ -215,11 +217,3 @@ export async function checkConfig( return { layer: "config", status: "ok" }; } - -/** Layer: existence. Dispatches to the per-type probe in `checks-existence.ts`. */ -export async function checkExistence( - target: ResourceTarget, - client: unknown, -): Promise { - return runExistenceProbe(client, target); -} diff --git a/packages/shared/src/cli/commands/doctor/databricks-client.ts b/packages/shared/src/cli/commands/doctor/databricks-client.ts index cdf886944..2ca549d1d 100644 --- a/packages/shared/src/cli/commands/doctor/databricks-client.ts +++ b/packages/shared/src/cli/commands/doctor/databricks-client.ts @@ -1,10 +1,7 @@ /** - * The single seam where `appkit doctor` crosses into the Databricks SDK. - * - * The CLI lives in the SDK-free `shared` package, so it reaches the SDK via a - * runtime `import(...)` — keeping `shared` free of the dependency and degrading - * gracefully when it's absent. All Databricks-touching check code goes through - * this module; nothing else in the doctor command imports the SDK. + * The single seam where `appkit doctor` crosses into the Databricks SDK. The + * SDK-free `shared` package reaches it via a runtime `import(...)`, degrading + * gracefully when it's absent. */ /** Raised when `@databricks/sdk-experimental` is not resolvable at runtime. */ @@ -32,14 +29,11 @@ function isModuleNotFound(err: unknown): boolean { } /** Constructs a `WorkspaceClient` via the SDK's unified-auth chain. An explicit - * `profile` is passed through the SDK's own `Config.profile` field — a per-call - * seam, so we never mutate `process.env` (which would leak the profile beyond - * this call and race with anything else reading the environment). When omitted, - * the SDK falls back to its normal chain (env, then the default profile). */ + * `profile` is passed through `Config.profile` rather than mutating + * `process.env`, so it doesn't leak beyond this call. */ export async function getServiceClient( profile?: string, ): Promise { - // Narrowed to the one call we make; `shared` has no static SDK dependency. let sdk: { WorkspaceClient: new (opts: Record) => unknown }; try { sdk = (await import("@databricks/sdk-experimental")) as typeof sdk; @@ -77,7 +71,7 @@ export async function getLakebasePool( client: unknown, ): Promise { // A variable specifier stops TS from statically resolving `@databricks/appkit`, - // which `shared` has no dependency on; it's an optional peer resolved at runtime. + // an optional peer that `shared` has no dependency on. const appkitPkg = "@databricks/appkit"; let appkit: { createLakebasePool: (cfg: Record) => unknown; diff --git a/packages/shared/src/cli/commands/doctor/index.ts b/packages/shared/src/cli/commands/doctor/index.ts index 4daa3316b..4d95f49b4 100644 --- a/packages/shared/src/cli/commands/doctor/index.ts +++ b/packages/shared/src/cli/commands/doctor/index.ts @@ -7,10 +7,8 @@ import { runDoctor } from "./run"; import type { DoctorOptions } from "./types"; /** - * Loads an explicit env file into `process.env`, overriding the `.env` the CLI - * auto-loads at startup so doctor checks the same environment the app runs with. - * @throws if the file is missing — an explicit `--env-file` that doesn't exist - * is a mistake worth surfacing, not silently ignoring. + * Loads an explicit env file into `process.env`, overriding the auto-loaded + * `.env`. Throws if the file is missing. */ export function loadEnvFile(envFile: string): void { if (!fs.existsSync(envFile)) { @@ -25,7 +23,7 @@ async function runDoctorCommand(options: DoctorOptions): Promise { const report = await runDoctor(options); if (options.json) { - printReportJson(report); + printReportJson(report, options.detail); } else { printReport(report, options.detail); } diff --git a/packages/shared/src/cli/commands/doctor/report.test.ts b/packages/shared/src/cli/commands/doctor/report.test.ts index 65c39d13f..32431fad6 100644 --- a/packages/shared/src/cli/commands/doctor/report.test.ts +++ b/packages/shared/src/cli/commands/doctor/report.test.ts @@ -217,4 +217,31 @@ describe("printReportJson", () => { const lines = capture(() => printReportJson(r)); expect(JSON.parse(lines.join("\n"))).toEqual(r); }); + + it("strips the raw SDK error by default (matches the --detail gate)", () => { + const r = report({ + auth: { + status: "error", + detail: "authentication failed", + raw: "default auth: cannot get access token: ", + }, + }); + const parsed = capture(() => printReportJson(r)); + const out = JSON.parse(parsed.join("\n")) as DoctorReport; + expect(out.auth.raw).toBeUndefined(); + expect(parsed.join("\n")).not.toContain(""); + }); + + it("includes raw when detail=true (opt-in)", () => { + const r = report({ + auth: { + status: "error", + detail: "authentication failed", + raw: "default auth: cannot get access token: ", + }, + }); + const parsed = capture(() => printReportJson(r, true)); + const out = JSON.parse(parsed.join("\n")) as DoctorReport; + expect(out.auth.raw).toContain(""); + }); }); diff --git a/packages/shared/src/cli/commands/doctor/report.ts b/packages/shared/src/cli/commands/doctor/report.ts index 4f8030662..5d0410962 100644 --- a/packages/shared/src/cli/commands/doctor/report.ts +++ b/packages/shared/src/cli/commands/doctor/report.ts @@ -1,48 +1,35 @@ /** - * Rendering for `appkit doctor` — turns a {@link DoctorReport} into either a - * human-readable console report or JSON, matching the `--json` convention used - * by `plugin list` / `plugin validate`. - * - * Design goals: one flat, severity-sorted checklist (no titled sub-sections), - * where every row shares the same shape — `glyph label`, with detail and an - * optional `Hint:` indented beneath, and expanded rows set apart by blank lines. - * Show as little as possible on the happy path (a fine resource is just a green - * tick and its name); reveal detail only where there's something to act on. - * Colour is sparing and consistent — glyphs carry status (green/red/yellow), - * and the actionable token (a quoted id / code span in cyan, an env var in bold) - * is highlighted so it stands out from prose. picocolors auto-disables when - * output isn't a TTY (and honours NO_COLOR), so piped/CI output stays plain. + * Rendering for `appkit doctor` — turns a {@link DoctorReport} into a + * human-readable console report or JSON. */ import pc from "picocolors"; -import type { - CheckStatus, - DoctorReport, - ResourceCheckResult, - WiringFinding, +import { + AUTH_UNAVAILABLE_CODE, + type CheckStatus, + type DoctorReport, + type ResourceCheckResult, + STATUS_SEVERITY, + type WiringFinding, } from "./types"; -/** Column at which a row's label starts (`" ✓ "` → 5), used to align the - * detail/hint lines beneath it. */ +/** Column at which a row's label starts, to align detail/hint lines beneath. */ const SUB_INDENT = " "; /** - * True when a resource's *only* finding is that its existence probe was skipped - * because auth failed — i.e. it has no independent problem worth its own row. - * Such rows are collapsed into one summary line. + * True when a resource's only finding is an auth-skipped existence probe, so it + * can be collapsed into one summary line. */ function isOnlyAuthSkipped(r: ResourceCheckResult): boolean { if (r.status !== "skipped") return false; const hasAuthSkip = r.layers.some( - (l) => l.layer === "existence" && l.code === "AUTH_UNAVAILABLE", + (l) => l.layer === "existence" && l.code === AUTH_UNAVAILABLE_CODE, ); if (!hasAuthSkip) return false; - // Collapse only when auth-skip is the *sole* finding — any other non-ok layer - // (e.g. a missing env var) is actionable and keeps the row on its own line. return r.layers.every( (l) => l.status === "ok" || - (l.layer === "existence" && l.code === "AUTH_UNAVAILABLE"), + (l.layer === "existence" && l.code === AUTH_UNAVAILABLE_CODE), ); } @@ -70,13 +57,9 @@ function glyph(status: CheckStatus): string { } /** - * Highlights the actionable tokens in a prose detail/hint line. One tight - * palette: cyan = a literal token you type or reference — a "quoted" id/binding - * name or a `backticked` code/command span (the delimiters are stripped); bold = - * a SCREAMING_SNAKE env-var name. Resource *names* aren't highlighted — only the - * id/code you'd act on is. Nesting is safe: bold's reset (22) doesn't clear the - * cyan (39), so an env var inside a code span stays bold *and* cyan. Purely - * cosmetic — a no-op when colour is disabled. + * Highlights actionable tokens in a detail/hint line: cyan for a "quoted" id or + * `backticked` code span (delimiters stripped), bold for a SCREAMING_SNAKE env + * var. Nesting is safe — bold's reset (22) doesn't clear the cyan (39). */ function highlight(text: string): string { return text @@ -91,12 +74,8 @@ function subLine(text: string): void { } /** - * Prints the expanded body shared by every non-ok row: each indented `detail` - * line, then (if any) a `Hint:` set off by a blank line above it. Keeps - * resource, wiring, and auth rows visually identical. Multiple details/hints (a - * resource can trip more than one layer) are grouped into one block. Callers own - * the surrounding blank lines (see {@link printReport}), so a body never emits a - * trailing blank of its own. + * Prints the expanded body of a non-ok row: each indented `detail` line, then + * any `Hint:` set off by a blank line. Callers own the surrounding blank lines. */ function printRowBody( details: Array, @@ -110,14 +89,6 @@ function printRowBody( } } -/** Row display order: most severe first (stable within a status). */ -const DISPLAY_ORDER: Record = { - error: 0, - warn: 1, - skipped: 2, - ok: 3, -}; - /** Shows only the non-zero categories, coloured by severity. */ function summaryLine(counts: { ok: number; @@ -142,12 +113,8 @@ function summaryLine(counts: { function printAuthRow(report: DoctorReport, detail: boolean): void { const { auth } = report; console.log(` ${glyph(auth.status)} Auth — ${auth.detail ?? auth.status}`); - // Happy path: just the row. On failure, expand with the target profile, hint, - // and (with --detail) the raw error — the caller frames it with blank lines. if (auth.status !== "error") return; - // The profile in play is only interesting when auth failed — it's the first - // thing to sanity-check — so attach it here rather than in a header. if (auth.profile) { subLine(`${pc.dim("profile:")} ${pc.cyan(auth.profile)}`); } @@ -155,8 +122,6 @@ function printAuthRow(report: DoctorReport, detail: boolean): void { console.log(""); subLine(`${pc.dim("Hint:")} ${highlight(auth.hint)}`); } - // The full underlying error is hidden by default (the hint explains the fix); - // `--detail` surfaces it for debugging. if (detail && auth.raw) { console.log(""); subLine(pc.dim("Details:")); @@ -165,10 +130,9 @@ function printAuthRow(report: DoctorReport, detail: boolean): void { } /** - * Prints one resource row. On the happy path this is just a tick and the - * resource name — no plugin/type attribution, no layer noise. Non-ok rows - * append their actionable layer details (and hints, offset by a blank line) - * beneath. Bundle-managed resources are noted as deploy-created, not probed. + * Prints one resource row: a tick and the name on the happy path, actionable + * layer details beneath otherwise. Bundle-managed resources are noted as + * deploy-created, not probed. */ function printResourceRow(r: ResourceCheckResult): void { const { target } = r; @@ -195,9 +159,8 @@ function printWiringRow(f: WiringFinding): void { printRowBody([f.detail], [f.hint]); } -/** A unified printable row. `expanded` rows (those with detail/hint beneath the - * glyph line) get a blank line on either side so they read as their own - * paragraph; plain `ok` rows pack together. */ +/** A unified printable row. `expanded` rows get a blank line on either side; + * plain `ok` rows pack together. */ interface Row { status: CheckStatus; expanded: boolean; @@ -207,30 +170,25 @@ interface Row { export function printReport(report: DoctorReport, detail = false): void { const { resources, wiring, summary, auth } = report; - // One flat, severity-sorted list — no titled sub-blocks. Auth is always the - // first row; every resource and wiring finding follows in the same list, so - // the report reads as a single consistent checklist rather than a mix of - // headed and headless sections. + // One flat, severity-sorted list. Auth leads and never sorts. const rows: Row[] = []; - - // Auth leads and never sorts. It expands only on failure. rows.push({ status: auth.status, expanded: auth.status === "error", print: () => printAuthRow(report, detail), }); - // When auth failed, external resources are all auth-skipped; those carry no - // new information, so collapse the ones whose *only* finding is the auth-skip - // into a single line (keeping any that also have a real, actionable problem). - const authSkipped = resources.filter(isOnlyAuthSkipped); + // When auth failed, resources whose only finding is the auth-skip carry no + // new information, so collapse them into a single line. + const authSkipped: ResourceCheckResult[] = []; const checked: Row[] = []; for (const r of resources) { - if (isOnlyAuthSkipped(r)) continue; + if (isOnlyAuthSkipped(r)) { + authSkipped.push(r); + continue; + } checked.push({ status: r.status, - // Bundle-managed rows are a single line ("will be created on deploy"); - // every other non-ok row shows detail beneath, so it's expanded. expanded: r.status !== "ok" && r.target.origin !== "bundle-managed", print: () => printResourceRow(r), }); @@ -255,12 +213,11 @@ export function printReport(report: DoctorReport, detail = false): void { }); } - // Sort everything after auth by severity; auth stays pinned at the top. - checked.sort((a, b) => DISPLAY_ORDER[a.status] - DISPLAY_ORDER[b.status]); + // Most severe first (descending severity); stable within a status. + checked.sort((a, b) => STATUS_SEVERITY[b.status] - STATUS_SEVERITY[a.status]); rows.push(...checked); - // Print with a blank line separating any expanded row from its neighbours, so - // multi-line findings are set apart while consecutive ok rows stay compact. + // A blank line sets any expanded row apart from its neighbours. console.log(""); rows.forEach((row, i) => { const prev = rows[i - 1]; @@ -268,11 +225,15 @@ export function printReport(report: DoctorReport, detail = false): void { row.print(); }); - // Fold auth failure and wiring findings into the counts so nothing that - // affects the exit code reads as "0 errors". + // Fold auth and wiring findings into the counts so nothing that affects the + // exit code reads as "0 errors". const authError = auth.status === "error" ? 1 : 0; - const wiringErrors = wiring.filter((w) => w.status === "error").length; - const wiringWarns = wiring.filter((w) => w.status === "warn").length; + let wiringErrors = 0; + let wiringWarns = 0; + for (const w of wiring) { + if (w.status === "error") wiringErrors += 1; + else if (w.status === "warn") wiringWarns += 1; + } console.log(""); console.log( summaryLine({ @@ -288,8 +249,17 @@ export function printReport(report: DoctorReport, detail = false): void { console.log(""); } -export function printReportJson(report: DoctorReport): void { - console.log(JSON.stringify(report, null, 2)); +/** + * Emits the report as JSON. The raw SDK error (`auth.raw`) can carry sensitive + * detail, so — matching the human report's `--detail` gate — it's stripped + * unless `detail` is set. `--json --detail` opts back into the full error. + */ +export function printReportJson(report: DoctorReport, detail = false): void { + const emitted = + detail || report.auth.raw === undefined + ? report + : { ...report, auth: { ...report.auth, raw: undefined } }; + console.log(JSON.stringify(emitted, null, 2)); } /** Non-zero if auth, any resource, or any wiring finding errored, so diff --git a/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts b/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts index 1c433d7d5..63d558494 100644 --- a/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts +++ b/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts @@ -160,7 +160,7 @@ describe("targetsFromManifestFile", () => { expect(targets).toEqual([]); }); - it("excludes value-default and platform-injected fields from envVars (regression: bug #3)", () => { + it("excludes value-default and platform-injected fields from envVars", () => { const targets = targetsFromManifestFile( writeManifest({ version: "2.0", @@ -203,8 +203,6 @@ describe("targetsFromManifestFile", () => { }), ); const pg = targets.find((t) => t.type === "postgres"); - // Only the user-supplied endpoint is presence-checked; value-default and - // platform-injected fields must not be flagged as missing. expect(pg?.envVars).toEqual(["LAKEBASE_ENDPOINT"]); }); @@ -224,8 +222,6 @@ describe("targetsFromManifestFile", () => { alias: "WH", permission: "CAN_USE", fields: { - // Configured by a baked-in value, not an env var — the - // existence probe must still receive it. id: { env: "SOME_UNSET_ENV", value: "baked-in-id" }, }, }, diff --git a/packages/shared/src/cli/commands/doctor/resolve-targets.ts b/packages/shared/src/cli/commands/doctor/resolve-targets.ts index a0a6fb8e1..35e56cdb8 100644 --- a/packages/shared/src/cli/commands/doctor/resolve-targets.ts +++ b/packages/shared/src/cli/commands/doctor/resolve-targets.ts @@ -8,6 +8,7 @@ import fs from "node:fs"; import path from "node:path"; import type { ResourceTarget } from "./types"; +import { errorMessage } from "./utils"; export const DEFAULT_MANIFEST_FILE = "appkit.plugins.json"; @@ -15,7 +16,6 @@ interface ManifestField { env?: string; /** Static default value baked into the manifest. */ value?: string; - /** Computed origin: how the value is supplied (see the manifest schema). */ origin?: "user" | "platform" | "static" | "cli"; /** Only generated into the local .env; the platform injects it at deploy. */ localOnly?: boolean; @@ -110,29 +110,26 @@ export function targetsFromManifestFile( try { raw = fs.readFileSync(manifestPath, "utf-8"); } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - throw new Error(`Failed to read manifest file ${manifestPath}: ${msg}`); + throw new Error( + `Failed to read manifest file ${manifestPath}: ${errorMessage(err)}`, + ); } let data: TemplateManifest; try { data = JSON.parse(raw) as TemplateManifest; } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - throw new Error(`Failed to parse manifest file ${manifestPath}: ${msg}`); + throw new Error( + `Failed to parse manifest file ${manifestPath}: ${errorMessage(err)}`, + ); } - // `plugin sync` writes a resource entry for *every* plugin the installed - // packages ship (a catalog for `apps init`), not just the ones this app uses. - // Only plugins actually wired into `createApp` are marked - // `requiredByTemplate`, so check exactly those — otherwise doctor reports - // phantom "missing env var" errors for plugins the app never imports. - // - // KNOWN LIMITATION: `plugin sync` strips `requiredByTemplate` for non-GA - // plugins (its step 6b), so a *used* beta/experimental plugin (e.g. agents - // requiring a serving endpoint) is currently NOT checked here. Fixing this - // needs a usage signal that survives sync regardless of stability — see the - // doctor README. Documented and deferred rather than papered over. + // `plugin sync` catalogues every plugin the installed packages ship, marking + // only those wired into `createApp` with `requiredByTemplate`. Check exactly + // those, else doctor reports phantom "missing env var" errors for unimported + // plugins. + // KNOWN LIMITATION: sync strips `requiredByTemplate` for non-GA plugins, so a + // used beta/experimental plugin is not checked here yet. See the README. const selected = Object.entries(data.plugins ?? {}).filter( ([, plugin]) => plugin.requiredByTemplate === true, ); diff --git a/packages/shared/src/cli/commands/doctor/run.ts b/packages/shared/src/cli/commands/doctor/run.ts index 307ec91c6..2fd3ea08d 100644 --- a/packages/shared/src/cli/commands/doctor/run.ts +++ b/packages/shared/src/cli/commands/doctor/run.ts @@ -1,34 +1,21 @@ -/** - * Orchestration for `appkit doctor`. - * - * Resolves the resources the app declares, overlays bundle provenance, runs the - * app-wide auth check once, then per resource runs the offline `config` layer - * followed by the live `existence` layer — except for bundle-managed resources, - * whose existence can't be probed before deploy. Separately runs the offline - * wiring check (deploy-declaration consistency). Rolls everything into a - * {@link DoctorReport}. - */ +/** Orchestration for `appkit doctor`. */ import { originForEnvVars, readBundleInfo } from "./bundle"; -import { checkAuth, checkConfig, checkExistence } from "./checks"; +import { checkAuth, checkConfig } from "./checks"; +import { runExistenceProbe } from "./checks-existence"; import { checkWiring } from "./checks-wiring"; import { resolveTargetsFromCwd } from "./resolve-targets"; -import type { - CheckStatus, - DoctorOptions, - DoctorReport, - LayerResult, - ResourceCheckResult, - ResourceTarget, +import { + AUTH_UNAVAILABLE_CODE, + type CheckStatus, + type DoctorOptions, + type DoctorReport, + type LayerResult, + type ResourceCheckResult, + type ResourceTarget, + STATUS_SEVERITY, } from "./types"; -const STATUS_SEVERITY: Record = { - ok: 0, - skipped: 1, - warn: 2, - error: 3, -}; - function worst(a: CheckStatus, b: CheckStatus): CheckStatus { return STATUS_SEVERITY[b] > STATUS_SEVERITY[a] ? b : a; } @@ -49,8 +36,8 @@ async function checkResource( return { target, status: rolled, layers }; } - // A bundle-managed resource doesn't exist until `bundle deploy`, so probing it - // would be a false NOT_FOUND. Report it as deploy-managed instead. + // A bundle-managed resource doesn't exist until deploy, so probing would be a + // false NOT_FOUND. if (target.origin === "bundle-managed") { layers.push({ layer: "existence", @@ -65,12 +52,12 @@ async function checkResource( layers.push({ layer: "existence", status: "skipped", - code: "AUTH_UNAVAILABLE", + code: AUTH_UNAVAILABLE_CODE, detail: "skipped because workspace authentication failed", }); rolled = worst(rolled, "skipped"); } else { - const result = await checkExistence(target, client); + const result = await runExistenceProbe(client, target); layers.push(result); rolled = worst(rolled, result.status); } @@ -82,31 +69,26 @@ export async function runDoctor(options: DoctorOptions): Promise { const { result: auth, client } = await checkAuth(options); const summary = { ok: 0, warn: 0, error: 0, skipped: 0 }; - const resources: ResourceCheckResult[] = []; - // Resources are resolved and config-checked even when auth failed, so a bad - // connection still surfaces config problems instead of hiding them all. + // Resolve and config-check resources even when auth failed, so a bad + // connection still surfaces config problems instead of hiding them. const cwd = process.cwd(); const targets = resolveTargetsFromCwd(cwd); - // Overlay bundle provenance (Phase 1) and gather wiring info (Phase 2). const bundle = readBundleInfo(cwd); for (const target of targets) { const origin = originForEnvVars(target.envVars, bundle); if (origin) target.origin = origin; } - // Probes are independent reads, so run them concurrently; Promise.all - // preserves input order, keeping the report deterministic. - const results = await Promise.all( + // Probes are independent reads; Promise.all preserves order for a + // deterministic report. + const resources = await Promise.all( targets.map((target) => checkResource(target, client)), ); - for (const result of results) { - resources.push(result); + for (const result of resources) { summary[result.status] += 1; } - const wiring = checkWiring(bundle, targets); - - return { auth, resources, wiring, summary }; + return { auth, resources, wiring: checkWiring(bundle, targets), summary }; } diff --git a/packages/shared/src/cli/commands/doctor/types.ts b/packages/shared/src/cli/commands/doctor/types.ts index 25f2e32ff..7b38e82fc 100644 --- a/packages/shared/src/cli/commands/doctor/types.ts +++ b/packages/shared/src/cli/commands/doctor/types.ts @@ -6,13 +6,27 @@ export type CheckLayer = "auth" | "config" | "existence"; export type CheckStatus = "ok" | "warn" | "error" | "skipped"; /** - * Where a resource's value comes from, per the bundle (`databricks.yml`): - * - `external` — an existing resource referenced by id/name (`${var.*}` or a - * literal). It should exist now, so the live probe applies. - * - `bundle-managed`— created by this same bundle (`${resources...*}`); - * it doesn't exist until `bundle deploy`, so we validate the - * declaration instead of probing. - * Undefined ⇒ external (no bundle info, or an AppKit app with no databricks.yml). + * The existence-layer `code` marking a probe skipped because auth failed. Set in + * `run.ts` and matched in `report.ts` to collapse those resources into one line; + * shared so the two sides can't drift. + */ +export const AUTH_UNAVAILABLE_CODE = "AUTH_UNAVAILABLE"; + +/** + * Severity ranking for a status. Used both to roll up the worst status across + * layers and to sort report rows most-severe-first (descending severity). + */ +export const STATUS_SEVERITY: Record = { + ok: 0, + skipped: 1, + warn: 2, + error: 3, +}; + +/** + * Where a resource's value comes from: `external` (exists now, so probe it) or + * `bundle-managed` (created by this bundle on deploy, so not probed). + * Undefined ⇒ external. */ export type ResourceOrigin = "external" | "bundle-managed"; @@ -39,7 +53,6 @@ export interface ResourceTarget { envVars: string[]; /** Resolved field values keyed by manifest field name; unset fields omitted. */ fieldValues: Record; - /** Provenance from the bundle; undefined ⇒ treated as external. */ origin?: ResourceOrigin; } @@ -57,23 +70,16 @@ export interface AuthCheckResult { code?: string; host?: string; profile?: string; - /** Full underlying error (e.g. the raw SDK message). Shown only with - * `--detail` or in `--json`; the human report relies on `detail` + `hint`. */ + /** Full underlying error, shown only with `--detail` or in `--json`. */ raw?: string; } -/** - * A finding from the offline three-file wiring check (Phase 2): does each - * `app.yaml` `valueFrom` bind to a real databricks.yml binding, and does each - * bundle-managed `${resources.*}` reference resolve to a declared bundle - * resource. Independent of auth — it's a deploy-declaration check, not a live one. - */ +/** A finding from the offline three-file wiring check. */ export interface WiringFinding { status: CheckStatus; /** Machine-readable code (e.g. `VALUEFROM_UNBOUND`, `BUNDLE_REF_MISSING`). */ code: string; - /** Short row label (the env var or binding at fault), so a wiring row renders - * with the same shape as a resource row: `glyph label` then indented detail. */ + /** The env var or binding at fault. */ label: string; detail: string; hint?: string; @@ -81,9 +87,7 @@ export interface WiringFinding { export interface DoctorReport { auth: AuthCheckResult; - /** Live connectivity checks for external resources. */ resources: ResourceCheckResult[]; - /** Deploy-declaration findings: bundle-managed resources + wiring consistency. */ wiring: WiringFinding[]; summary: { ok: number; warn: number; error: number; skipped: number }; } @@ -91,12 +95,9 @@ export interface DoctorReport { export interface DoctorOptions { profile?: string; json?: boolean; - /** Show full underlying error messages (raw SDK output) in the human report. */ + /** Show full underlying error messages in the human report. */ detail?: boolean; - /** - * Path to an env file to load before checking (e.g. `.env.local`). Its values - * override the `.env` the CLI auto-loads at startup, so doctor checks the same - * environment the app runs with. - */ + /** Env file to load before checking (e.g. `.env.local`); overrides the .env + * the CLI auto-loads at startup. */ envFile?: string; } diff --git a/packages/shared/src/cli/commands/doctor/utils.ts b/packages/shared/src/cli/commands/doctor/utils.ts new file mode 100644 index 000000000..f628c879e --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/utils.ts @@ -0,0 +1,6 @@ +/** Small shared helpers for the `appkit doctor` command. */ + +/** Extracts a human-readable message from an unknown thrown value. */ +export function errorMessage(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} From bbe3b24d8eaea8aac62e7e6037be8e99ab1ee313 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Fri, 31 Jul 2026 10:47:01 +0200 Subject: [PATCH 10/14] fix(shared): error (not warn) on an unwired REQUIRED env var MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ENV_UNWIRED was always a warning, but exitCodeFor gates only on errors — so a required plugin's env var with no app.yaml entry (set locally via .env, unset in the deployed container) exited 0, the exact works-locally-breaks-on-deploy case the wiring check exists to catch. Gate on target.required: error for required (fails the exit code, blocks pre-deploy CI), warn for optional. Tests cover both. Signed-off-by: Galymzhan --- .../cli/commands/doctor/checks-wiring.test.ts | 17 +++++++++++++---- .../src/cli/commands/doctor/checks-wiring.ts | 11 +++++++---- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts b/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts index ee2b0d405..49973158e 100644 --- a/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts +++ b/packages/shared/src/cli/commands/doctor/checks-wiring.test.ts @@ -84,7 +84,7 @@ describe("checkWiring", () => { expect(findings.some((f) => f.code === "BUNDLE_REF_MISSING")).toBe(false); }); - it("warns when a plugin needs an env var app.yaml doesn't provide", () => { + it("warns for an OPTIONAL plugin's env var that app.yaml doesn't provide", () => { const findings = checkWiring( info({ // app.yaml provides one env, bindings exist for it. @@ -100,25 +100,34 @@ describe("checkWiring", () => { ], ]), }), - [target({ envVars: ["DATABRICKS_WAREHOUSE_ID", "DATABRICKS_MISSING"] })], + [ + target({ + required: false, + envVars: ["DATABRICKS_WAREHOUSE_ID", "DATABRICKS_MISSING"], + }), + ], ); const unwired = findings.find((f) => f.code === "ENV_UNWIRED"); expect(unwired?.status).toBe("warn"); expect(unwired?.label).toBe("DATABRICKS_MISSING"); }); - it("warns even when app.yaml has NO env block (used plugin, zero wiring)", () => { + it("errors for a REQUIRED plugin's unwired env var, gating the exit code", () => { + // The "works locally, breaks on deploy" case: a required env is set via + // local .env but has no app.yaml entry, so it's unset in the deployed + // container — a guaranteed break, so it must be an error, not a warning. const findings = checkWiring( info({ present: true }), // empty bindings + empty envToBinding [ target({ + required: true, alias: "SQL Warehouse", envVars: ["DATABRICKS_WAREHOUSE_ID"], }), ], ); const unwired = findings.find((f) => f.code === "ENV_UNWIRED"); - expect(unwired?.status).toBe("warn"); + expect(unwired?.status).toBe("error"); expect(unwired?.label).toBe("DATABRICKS_WAREHOUSE_ID"); expect(unwired?.detail).toMatch(/unset in the deployed app/i); }); diff --git a/packages/shared/src/cli/commands/doctor/checks-wiring.ts b/packages/shared/src/cli/commands/doctor/checks-wiring.ts index 91e398192..7e6fd87d7 100644 --- a/packages/shared/src/cli/commands/doctor/checks-wiring.ts +++ b/packages/shared/src/cli/commands/doctor/checks-wiring.ts @@ -53,16 +53,19 @@ export function checkWiring( // 3. A used plugin's env var with no app.yaml entry: set locally via .env, but // .env isn't uploaded, so it's unset in the deployed container — the headline - // "works locally, breaks on deploy" case. + // "works locally, breaks on deploy" case. For a *required* resource this + // guarantees a broken deploy, so it's an error that gates the exit code; for + // an optional one it's a warning. for (const target of targets) { for (const envVar of target.envVars) { if (!info.envToBinding.has(envVar)) { findings.push({ - status: "warn", + status: target.required ? "error" : "warn", code: "ENV_UNWIRED", label: envVar, - detail: - "has no app.yaml entry — it will be unset in the deployed app", + detail: target.required + ? "has no app.yaml entry — it will be unset in the deployed app" + : "has no app.yaml entry — the optional resource will be unset in the deployed app", hint: `Add to app.yaml: \`{ name: ${envVar}, valueFrom: }\`, and declare that binding in databricks.yml.`, }); } From 5a2568e37703590900fd1042a61340c9fa542b90 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Fri, 31 Jul 2026 10:53:34 +0200 Subject: [PATCH 11/14] fix(shared): bound doctor's live calls with a 10s timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No probe, currentUser.me(), or Lakebase SELECT 1 had a deadline, so a reachable-but-unresponsive endpoint would hang doctor forever — and as a CI gate, the job would never return. Add withTimeout (Promise.race against a 10s wall-clock deadline) around each existence probe and the auth me() call. It can't cancel the underlying request, but it stops doctor *waiting* so the report returns: a timed-out probe becomes a PROBE_TIMEOUT error row, a timed-out auth becomes an auth failure. The timer is always cleared, so a fast result leaves nothing pending and the CLI exits promptly. Lakebase connection setup was already bounded (connectionTimeoutMillis: 10s); this covers a hung query on an established connection too. Tests: withTimeout unit tests (fake timers), a hung-probe run test, and a hung-auth test. Signed-off-by: Galymzhan --- .../shared/src/cli/commands/doctor/README.md | 8 +++ .../src/cli/commands/doctor/checks.test.ts | 17 ++++++ .../shared/src/cli/commands/doctor/checks.ts | 8 ++- .../src/cli/commands/doctor/run.test.ts | 57 +++++++++++++++++++ .../shared/src/cli/commands/doctor/run.ts | 12 +++- .../src/cli/commands/doctor/utils.test.ts | 57 +++++++++++++++++++ .../shared/src/cli/commands/doctor/utils.ts | 32 +++++++++++ 7 files changed, 188 insertions(+), 3 deletions(-) create mode 100644 packages/shared/src/cli/commands/doctor/utils.test.ts diff --git a/packages/shared/src/cli/commands/doctor/README.md b/packages/shared/src/cli/commands/doctor/README.md index 09cb4309d..7e26227f0 100644 --- a/packages/shared/src/cli/commands/doctor/README.md +++ b/packages/shared/src/cli/commands/doctor/README.md @@ -20,6 +20,14 @@ so the reported problem is the *root* cause, not a symptom. is the `existence` layer's job. (`DATABRICKS_HOST` is the exception — `auth` validates it structurally, since a bad host means no client can be built.) +Every live call (`auth`'s `currentUser.me()` and each `existence` probe) is +bounded by a 10s wall-clock deadline (`withTimeout`). A reachable-but-unresponsive +endpoint must never hang doctor — it's a CI gate that has to return — so a +timeout surfaces as an error (`PROBE_TIMEOUT`, or an auth failure) rather than a +hung process. The race can't cancel the underlying request, but it stops doctor +*waiting* on it. (Lakebase connections are separately bounded at 10s by the +pool's `connectionTimeoutMillis`.) + ## Which plugins are checked `plugin sync` writes `appkit.plugins.json` cataloguing *every* plugin the diff --git a/packages/shared/src/cli/commands/doctor/checks.test.ts b/packages/shared/src/cli/commands/doctor/checks.test.ts index 136310388..238da7ba5 100644 --- a/packages/shared/src/cli/commands/doctor/checks.test.ts +++ b/packages/shared/src/cli/commands/doctor/checks.test.ts @@ -192,6 +192,23 @@ describe("checkAuth", () => { expect(result.hint).toBeUndefined(); // unrecognized failure → no guess }); + it("fails (not hangs) when currentUser.me() never responds", async () => { + vi.useFakeTimers(); + process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; + // Client resolves, but the live me() call never settles. + mockGetServiceClient.mockResolvedValue({ + client: { currentUser: { me: () => new Promise(() => {}) } }, + }); + + const authPromise = checkAuth({}); + await vi.advanceTimersByTimeAsync(10_000); + const { result } = await authPromise; + expect(result.status).toBe("error"); + expect(result.code).toBe("AUTH_FAILED"); + expect(result.raw).toMatch(/timed out/i); + vi.useRealTimers(); + }); + it("hints to reauthenticate on an expired/failed CLI token (real SDK message)", async () => { process.env.DATABRICKS_HOST = "https://foo.cloud.databricks.com"; mockGetServiceClient.mockRejectedValue( diff --git a/packages/shared/src/cli/commands/doctor/checks.ts b/packages/shared/src/cli/commands/doctor/checks.ts index 822369050..18c6e1a53 100644 --- a/packages/shared/src/cli/commands/doctor/checks.ts +++ b/packages/shared/src/cli/commands/doctor/checks.ts @@ -10,7 +10,7 @@ import type { LayerResult, ResourceTarget, } from "./types"; -import { errorMessage } from "./utils"; +import { errorMessage, withTimeout } from "./utils"; /** The auth result plus the resolved client (present only on success). */ export interface AuthOutcome { @@ -98,7 +98,11 @@ export async function checkAuth(options: DoctorOptions): Promise { try { const { client } = await getServiceClient(options.profile); - const me = await (client as CurrentUserClient).currentUser.me(); + // Bound the live call so an unresponsive workspace can't hang the CLI; a + // timeout throws and is reported as an auth failure below. + const me = await withTimeout( + (client as CurrentUserClient).currentUser.me(), + ); const who = me.userName ?? me.id ?? "unknown"; return { diff --git a/packages/shared/src/cli/commands/doctor/run.test.ts b/packages/shared/src/cli/commands/doctor/run.test.ts index 73ef804d2..b109f5a92 100644 --- a/packages/shared/src/cli/commands/doctor/run.test.ts +++ b/packages/shared/src/cli/commands/doctor/run.test.ts @@ -17,6 +17,11 @@ vi.mock("./databricks-client", () => ({ })), })); +// Real probes by default; individual tests override to simulate a hung probe. +vi.mock("./checks-existence", async (importActual) => ({ + ...(await importActual()), +})); + describe("runDoctor", () => { afterEach(() => { vi.restoreAllMocks(); @@ -82,4 +87,56 @@ describe("runDoctor", () => { spy.mockRestore(); fs.rmSync(dir, { recursive: true, force: true }); }); + + it("bounds a hung probe with a timeout instead of hanging forever", async () => { + vi.useFakeTimers(); + const existence = await import("./checks-existence"); + // A probe that never settles — the reachable-but-unresponsive endpoint case. + vi.spyOn(existence, "runExistenceProbe").mockReturnValue( + new Promise(() => {}), + ); + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "doctor-hang-")); + fs.writeFileSync( + path.join(dir, "appkit.plugins.json"), + JSON.stringify({ + version: "2.0", + plugins: { + analytics: { + requiredByTemplate: true, + resources: { + required: [ + { + type: "sql_warehouse", + resourceKey: "sql-warehouse", + alias: "SQL Warehouse", + permission: "CAN_USE", + fields: { id: { env: "DOCTOR_HANG_ENV" } }, + }, + ], + optional: [], + }, + }, + }, + }), + ); + const spy = vi.spyOn(process, "cwd").mockReturnValue(dir); + process.env.DOCTOR_HANG_ENV = "wh-123"; // config passes → reach existence + + const runPromise = runDoctor({}); + // Fire the deadline; without the timeout this promise would never resolve. + await vi.advanceTimersByTimeAsync(10_000); + const report = await runPromise; + + const existenceLayer = report.resources[0].layers.find( + (l) => l.layer === "existence", + ); + expect(existenceLayer?.status).toBe("error"); + expect(existenceLayer?.code).toBe("PROBE_TIMEOUT"); + + delete process.env.DOCTOR_HANG_ENV; + spy.mockRestore(); + fs.rmSync(dir, { recursive: true, force: true }); + vi.useRealTimers(); + }); }); diff --git a/packages/shared/src/cli/commands/doctor/run.ts b/packages/shared/src/cli/commands/doctor/run.ts index 2fd3ea08d..c5e4a9de3 100644 --- a/packages/shared/src/cli/commands/doctor/run.ts +++ b/packages/shared/src/cli/commands/doctor/run.ts @@ -15,6 +15,7 @@ import { type ResourceTarget, STATUS_SEVERITY, } from "./types"; +import { errorMessage, TimeoutError, withTimeout } from "./utils"; function worst(a: CheckStatus, b: CheckStatus): CheckStatus { return STATUS_SEVERITY[b] > STATUS_SEVERITY[a] ? b : a; @@ -57,7 +58,16 @@ async function checkResource( }); rolled = worst(rolled, "skipped"); } else { - const result = await runExistenceProbe(client, target); + // A reachable-but-unresponsive endpoint must not hang doctor, so bound the + // probe; a timeout becomes an error row rather than a hung process. + const result = await withTimeout(runExistenceProbe(client, target)).catch( + (err): LayerResult => ({ + layer: "existence", + status: "error", + code: err instanceof TimeoutError ? "PROBE_TIMEOUT" : "PROBE_FAILED", + detail: `probe did not complete: ${errorMessage(err)}`, + }), + ); layers.push(result); rolled = worst(rolled, result.status); } diff --git a/packages/shared/src/cli/commands/doctor/utils.test.ts b/packages/shared/src/cli/commands/doctor/utils.test.ts new file mode 100644 index 000000000..b17ccdc60 --- /dev/null +++ b/packages/shared/src/cli/commands/doctor/utils.test.ts @@ -0,0 +1,57 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + DEFAULT_CHECK_TIMEOUT_MS, + errorMessage, + TimeoutError, + withTimeout, +} from "./utils"; + +describe("errorMessage", () => { + it("returns an Error's message", () => { + expect(errorMessage(new Error("boom"))).toBe("boom"); + }); + + it("stringifies a non-Error value", () => { + expect(errorMessage("plain")).toBe("plain"); + expect(errorMessage(42)).toBe("42"); + }); +}); + +describe("withTimeout", () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it("resolves with the value when the promise settles in time", async () => { + await expect(withTimeout(Promise.resolve("done"), 1000)).resolves.toBe( + "done", + ); + }); + + it("propagates the promise's own rejection unchanged", async () => { + await expect( + withTimeout(Promise.reject(new Error("real failure")), 1000), + ).rejects.toThrow("real failure"); + }); + + it("rejects with TimeoutError when the promise hangs past the deadline", async () => { + vi.useFakeTimers(); + // A promise that never settles — the classic reachable-but-unresponsive case. + const hung = new Promise(() => {}); + const raced = withTimeout(hung, 10_000); + const assertion = expect(raced).rejects.toBeInstanceOf(TimeoutError); + await vi.advanceTimersByTimeAsync(10_000); + await assertion; + }); + + it("defaults to the shared deadline", async () => { + vi.useFakeTimers(); + const hung = new Promise(() => {}); + const raced = withTimeout(hung); + const assertion = expect(raced).rejects.toThrow( + new RegExp(`${DEFAULT_CHECK_TIMEOUT_MS}ms`), + ); + await vi.advanceTimersByTimeAsync(DEFAULT_CHECK_TIMEOUT_MS); + await assertion; + }); +}); diff --git a/packages/shared/src/cli/commands/doctor/utils.ts b/packages/shared/src/cli/commands/doctor/utils.ts index f628c879e..65d10e164 100644 --- a/packages/shared/src/cli/commands/doctor/utils.ts +++ b/packages/shared/src/cli/commands/doctor/utils.ts @@ -4,3 +4,35 @@ export function errorMessage(err: unknown): string { return err instanceof Error ? err.message : String(err); } + +/** Default per-check wall-clock deadline (ms). A reachable-but-unresponsive + * endpoint must never hang doctor — it's a CI gate that has to return. */ +export const DEFAULT_CHECK_TIMEOUT_MS = 10_000; + +/** Thrown when a check exceeds its deadline. */ +export class TimeoutError extends Error { + constructor(ms: number) { + super(`timed out after ${ms}ms`); + this.name = "TimeoutError"; + } +} + +/** + * Races `promise` against a wall-clock deadline, rejecting with + * {@link TimeoutError} if it isn't settled in time. This bounds doctor even + * when the underlying SDK/driver call has no cancellation of its own — we can't + * abort the request, but we stop *waiting* on it so the report still returns. + * The timer is always cleared so a fast result leaves nothing pending. + */ +export function withTimeout( + promise: Promise, + ms = DEFAULT_CHECK_TIMEOUT_MS, +): Promise { + let timer: ReturnType; + const deadline = new Promise((_, reject) => { + timer = setTimeout(() => reject(new TimeoutError(ms)), ms); + }); + return Promise.race([promise, deadline]).finally(() => + clearTimeout(timer), + ) as Promise; +} From b7e54b165374eac6e1ab2a2223f0fe33361ee4c8 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Fri, 31 Jul 2026 11:00:58 +0200 Subject: [PATCH 12/14] fix(shared): make doctor summary authoritative + add exitCode to report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit summary counted only resources; printReport folded auth + wiring in at render time but never wrote it back, so printReportJson emitted the unfolded counts. A --json consumer reading summary.error === 0 missed auth failures and wiring errors (e.g. a required ENV_UNWIRED) — the exact CI-gate case. Build summary over everything with a status (resources + auth + wiring) once in runDoctor, and add a top-level exitCode as the unambiguous pass/fail signal. printReport now renders the summary verbatim (no re-folding, so human and --json agree) and exitCodeFor just reads the field. Moves the aggregation coverage to run.test.ts. Signed-off-by: Galymzhan --- .../shared/src/cli/commands/doctor/README.md | 6 +++ .../src/cli/commands/doctor/report.test.ts | 51 +++++-------------- .../shared/src/cli/commands/doctor/report.ts | 27 +++------- .../src/cli/commands/doctor/run.test.ts | 23 +++++++-- .../shared/src/cli/commands/doctor/run.ts | 18 ++++--- .../shared/src/cli/commands/doctor/types.ts | 8 +++ 6 files changed, 64 insertions(+), 69 deletions(-) diff --git a/packages/shared/src/cli/commands/doctor/README.md b/packages/shared/src/cli/commands/doctor/README.md index 7e26227f0..a2ddae53b 100644 --- a/packages/shared/src/cli/commands/doctor/README.md +++ b/packages/shared/src/cli/commands/doctor/README.md @@ -154,6 +154,12 @@ The report labels hints `Hint:`. Exit code is non-zero if auth, any resource, or any wiring finding is in an `error` state, so `appkit doctor` can gate CI / pre-deploy. +The report's `summary` counts *everything* with a status — resources, the auth +check, and wiring findings — so a `--json` consumer can trust +`summary.error === 0` to mean "nothing failed". The report also carries a +top-level `exitCode` (0/1) as the single unambiguous pass/fail signal. Both are +computed once in `runDoctor`, so `--json` and the human report never disagree. + Checks run as the identity that runs doctor (the developer locally, the app in deployment). diff --git a/packages/shared/src/cli/commands/doctor/report.test.ts b/packages/shared/src/cli/commands/doctor/report.test.ts index 32431fad6..b8909a2db 100644 --- a/packages/shared/src/cli/commands/doctor/report.test.ts +++ b/packages/shared/src/cli/commands/doctor/report.test.ts @@ -8,6 +8,7 @@ function report(overrides: Partial = {}): DoctorReport { resources: [], wiring: [], summary: { ok: 0, warn: 0, error: 0, skipped: 0 }, + exitCode: 0, ...overrides, }; } @@ -71,6 +72,7 @@ describe("printReport ordering", () => { ], wiring: [], summary: { ok: 1, warn: 1, error: 1, skipped: 1 }, + exitCode: 1, }; printReport(report); @@ -157,57 +159,28 @@ describe("printReport ordering", () => { expect(wiringIdx).toBeLessThan(okIdx); }); - it("folds an auth error into the summary error count", () => { + it("renders the authoritative summary verbatim (no re-folding)", () => { + // summary is built in runDoctor and already includes auth + wiring; the + // renderer must print it as-is, not recompute. const lines = capture(() => printReport( report({ auth: { status: "error", detail: "bad creds" }, - summary: { ok: 0, warn: 0, error: 0, skipped: 0 }, + summary: { ok: 0, warn: 0, error: 1, skipped: 0 }, + exitCode: 1, }), ), ); - // Auth failure counts as an error even though no resource errored. expect(lines.some((l) => l.startsWith("1 error"))).toBe(true); }); }); describe("exitCodeFor", () => { - it("is 0 when auth ok and no resource errors", () => { - expect( - exitCodeFor( - report({ summary: { ok: 2, warn: 1, error: 0, skipped: 1 } }), - ), - ).toBe(0); - }); - - it("is 1 when auth failed", () => { - expect(exitCodeFor(report({ auth: { status: "error" } }))).toBe(1); - }); - - it("is 1 when any resource errored", () => { - expect( - exitCodeFor( - report({ summary: { ok: 0, warn: 0, error: 1, skipped: 0 } }), - ), - ).toBe(1); - }); - - it("is 1 when a wiring finding errored (gates pre-deploy)", () => { - expect( - exitCodeFor( - report({ - summary: { ok: 0, warn: 0, error: 0, skipped: 0 }, - wiring: [ - { - status: "error", - code: "VALUEFROM_UNBOUND", - label: "X", - detail: "x", - }, - ], - }), - ), - ).toBe(1); + // The exit code is computed once in runDoctor and stored on the report; + // exitCodeFor just surfaces it. (Aggregation is covered in run.test.ts.) + it("returns the report's exitCode field", () => { + expect(exitCodeFor(report({ exitCode: 0 }))).toBe(0); + expect(exitCodeFor(report({ exitCode: 1 }))).toBe(1); }); }); diff --git a/packages/shared/src/cli/commands/doctor/report.ts b/packages/shared/src/cli/commands/doctor/report.ts index 5d0410962..ef06a43cf 100644 --- a/packages/shared/src/cli/commands/doctor/report.ts +++ b/packages/shared/src/cli/commands/doctor/report.ts @@ -225,23 +225,10 @@ export function printReport(report: DoctorReport, detail = false): void { row.print(); }); - // Fold auth and wiring findings into the counts so nothing that affects the - // exit code reads as "0 errors". - const authError = auth.status === "error" ? 1 : 0; - let wiringErrors = 0; - let wiringWarns = 0; - for (const w of wiring) { - if (w.status === "error") wiringErrors += 1; - else if (w.status === "warn") wiringWarns += 1; - } + // `summary` already counts auth + wiring (built authoritatively in run.ts), + // so render it directly — no re-folding, and it matches what --json emits. console.log(""); - console.log( - summaryLine({ - ...summary, - error: summary.error + authError + wiringErrors, - warn: summary.warn + wiringWarns, - }), - ); + console.log(summaryLine(summary)); // Point at --detail when there's more to show and it wasn't requested. if (!detail && auth.raw) { console.log(pc.dim("\nRun with --detail for full error output.")); @@ -262,10 +249,8 @@ export function printReportJson(report: DoctorReport, detail = false): void { console.log(JSON.stringify(emitted, null, 2)); } -/** Non-zero if auth, any resource, or any wiring finding errored, so - * `appkit doctor` can gate CI / pre-deploy. */ +/** The report's exit code (non-zero if anything errored), so `appkit doctor` + * can gate CI / pre-deploy. Computed in `runDoctor`; this reads the field. */ export function exitCodeFor(report: DoctorReport): number { - if (report.auth.status === "error") return 1; - if (report.summary.error > 0) return 1; - return report.wiring.some((w) => w.status === "error") ? 1 : 0; + return report.exitCode; } diff --git a/packages/shared/src/cli/commands/doctor/run.test.ts b/packages/shared/src/cli/commands/doctor/run.test.ts index b109f5a92..621e4d7c3 100644 --- a/packages/shared/src/cli/commands/doctor/run.test.ts +++ b/packages/shared/src/cli/commands/doctor/run.test.ts @@ -33,12 +33,29 @@ describe("runDoctor", () => { expect(report.auth.code).toBe("AUTH_OK"); }); - it("resolves no resources when cwd has no manifest, with an empty summary", async () => { - // Point cwd at a dir guaranteed to have no appkit.plugins.json. + it("counts the auth check in the summary (no resources ⇒ ok auth = 1 ok)", async () => { const spy = vi.spyOn(process, "cwd").mockReturnValue("/nonexistent-doctor"); const report = await runDoctor({}); expect(report.resources).toEqual([]); - expect(report.summary).toEqual({ ok: 0, warn: 0, error: 0, skipped: 0 }); + // Auth ok, no resources/wiring → summary reflects the auth check alone. + expect(report.summary).toEqual({ ok: 1, warn: 0, error: 0, skipped: 0 }); + expect(report.exitCode).toBe(0); + spy.mockRestore(); + }); + + it("folds an auth failure into summary.error and exitCode (the --json gap)", async () => { + const { getServiceClient } = await import("./databricks-client"); + vi.mocked(getServiceClient).mockRejectedValueOnce(new Error("no creds")); + // No manifest ⇒ no resources; only the failed auth contributes. + const spy = vi.spyOn(process, "cwd").mockReturnValue("/nonexistent-doctor"); + + const report = await runDoctor({}); + + expect(report.auth.status).toBe("error"); + // The bug this guards: a --json consumer reading summary.error must see 1, + // not 0, and exitCode must be non-zero. + expect(report.summary.error).toBe(1); + expect(report.exitCode).toBe(1); spy.mockRestore(); }); diff --git a/packages/shared/src/cli/commands/doctor/run.ts b/packages/shared/src/cli/commands/doctor/run.ts index c5e4a9de3..6702e909d 100644 --- a/packages/shared/src/cli/commands/doctor/run.ts +++ b/packages/shared/src/cli/commands/doctor/run.ts @@ -78,8 +78,6 @@ async function checkResource( export async function runDoctor(options: DoctorOptions): Promise { const { result: auth, client } = await checkAuth(options); - const summary = { ok: 0, warn: 0, error: 0, skipped: 0 }; - // Resolve and config-check resources even when auth failed, so a bad // connection still surfaces config problems instead of hiding them. const cwd = process.cwd(); @@ -96,9 +94,17 @@ export async function runDoctor(options: DoctorOptions): Promise { const resources = await Promise.all( targets.map((target) => checkResource(target, client)), ); - for (const result of resources) { - summary[result.status] += 1; - } + const wiring = checkWiring(bundle, targets); + + // The summary counts *everything* with a status — resources, the auth check, + // and wiring findings — so a --json consumer reading summary.error can trust + // it, and the human/JSON outputs share one source of truth. + const summary = { ok: 0, warn: 0, error: 0, skipped: 0 }; + for (const result of resources) summary[result.status] += 1; + summary[auth.status] += 1; + for (const finding of wiring) summary[finding.status] += 1; + + const exitCode = summary.error > 0 ? 1 : 0; - return { auth, resources, wiring: checkWiring(bundle, targets), summary }; + return { auth, resources, wiring, summary, exitCode }; } diff --git a/packages/shared/src/cli/commands/doctor/types.ts b/packages/shared/src/cli/commands/doctor/types.ts index 7b38e82fc..acca790d2 100644 --- a/packages/shared/src/cli/commands/doctor/types.ts +++ b/packages/shared/src/cli/commands/doctor/types.ts @@ -89,7 +89,15 @@ export interface DoctorReport { auth: AuthCheckResult; resources: ResourceCheckResult[]; wiring: WiringFinding[]; + /** + * Authoritative counts across *everything* that has a status — resources, + * the auth check, and wiring findings — not just resources. A `--json` + * consumer can trust `summary.error === 0` to mean "nothing failed". + */ summary: { ok: number; warn: number; error: number; skipped: number }; + /** Process exit code (0 ok, 1 if anything errored). The single unambiguous + * pass/fail signal for programmatic consumers. */ + exitCode: number; } export interface DoctorOptions { From 121ce81e12c3fe1d47f403688dbb84d37996dd54 Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Fri, 31 Jul 2026 11:06:00 +0200 Subject: [PATCH 13/14] fix(shared): guard checkResource so one throw can't crash the report The probe's own .catch covered a probe rejection, but checkResource had no top-level guard: an unexpected throw (a config-layer error, a synchronous throw in probe dispatch, anything else) rejected Promise.all and lost the entire report to a stack trace. Wrap each resource in checkResourceSafe, mapping any throw to a PROBE_EXCEPTION error row so the run always resolves and the rest of the report survives. Test verifies a synchronous probe throw becomes a row rather than crashing (confirmed it fails without the guard). Signed-off-by: Galymzhan --- .../src/cli/commands/doctor/run.test.ts | 49 +++++++++++++++++++ .../shared/src/cli/commands/doctor/run.ts | 30 +++++++++++- 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/packages/shared/src/cli/commands/doctor/run.test.ts b/packages/shared/src/cli/commands/doctor/run.test.ts index 621e4d7c3..d93bb5eb6 100644 --- a/packages/shared/src/cli/commands/doctor/run.test.ts +++ b/packages/shared/src/cli/commands/doctor/run.test.ts @@ -156,4 +156,53 @@ describe("runDoctor", () => { fs.rmSync(dir, { recursive: true, force: true }); vi.useRealTimers(); }); + + it("maps an unexpected throw to a PROBE_EXCEPTION row (never crashes the report)", async () => { + const existence = await import("./checks-existence"); + // A synchronous throw — before withTimeout wraps it — so it bypasses the + // probe's own .catch and would otherwise reject Promise.all. + vi.spyOn(existence, "runExistenceProbe").mockImplementation(() => { + throw new Error("kaboom"); + }); + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "doctor-throw-")); + fs.writeFileSync( + path.join(dir, "appkit.plugins.json"), + JSON.stringify({ + version: "2.0", + plugins: { + analytics: { + requiredByTemplate: true, + resources: { + required: [ + { + type: "sql_warehouse", + resourceKey: "sql-warehouse", + alias: "SQL Warehouse", + permission: "CAN_USE", + fields: { id: { env: "DOCTOR_THROW_ENV" } }, + }, + ], + optional: [], + }, + }, + }, + }), + ); + const spy = vi.spyOn(process, "cwd").mockReturnValue(dir); + process.env.DOCTOR_THROW_ENV = "wh-123"; // config passes → reach existence + + // Must resolve (not reject) — the whole report survives one bad resource. + const report = await runDoctor({}); + expect(report.resources).toHaveLength(1); + expect(report.resources[0].status).toBe("error"); + const layer = report.resources[0].layers.find( + (l) => l.code === "PROBE_EXCEPTION", + ); + expect(layer?.detail).toContain("kaboom"); + + delete process.env.DOCTOR_THROW_ENV; + spy.mockRestore(); + fs.rmSync(dir, { recursive: true, force: true }); + }); }); diff --git a/packages/shared/src/cli/commands/doctor/run.ts b/packages/shared/src/cli/commands/doctor/run.ts index 6702e909d..ef2a8e787 100644 --- a/packages/shared/src/cli/commands/doctor/run.ts +++ b/packages/shared/src/cli/commands/doctor/run.ts @@ -75,6 +75,34 @@ async function checkResource( return { target, status: rolled, layers }; } +/** + * Runs {@link checkResource} behind a guaranteed boundary: any unexpected throw + * (a probe's own `.catch` handles the common case, but this covers config + * errors and anything else) becomes a single error row instead of rejecting the + * `Promise.all` and losing the entire report to a stack trace. + */ +async function checkResourceSafe( + target: ResourceTarget, + client: unknown, +): Promise { + try { + return await checkResource(target, client); + } catch (err) { + return { + target, + status: "error", + layers: [ + { + layer: "existence", + status: "error", + code: "PROBE_EXCEPTION", + detail: `unexpected error while checking: ${errorMessage(err)}`, + }, + ], + }; + } +} + export async function runDoctor(options: DoctorOptions): Promise { const { result: auth, client } = await checkAuth(options); @@ -92,7 +120,7 @@ export async function runDoctor(options: DoctorOptions): Promise { // Probes are independent reads; Promise.all preserves order for a // deterministic report. const resources = await Promise.all( - targets.map((target) => checkResource(target, client)), + targets.map((target) => checkResourceSafe(target, client)), ); const wiring = checkWiring(bundle, targets); From b64f14eb7b4b443fc762ee87b3b5f0468e78938b Mon Sep 17 00:00:00 2001 From: Galymzhan Date: Fri, 31 Jul 2026 13:26:02 +0200 Subject: [PATCH 14/14] fix(shared): silence Lakebase connector's raw error dump in doctor The Lakebase pool's default error-only logger dumps the raw SDK ApiError (stack + full response blob) to stderr on a failed token fetch, burying doctor's clean one-line classification. Pass logger: { error: false } so doctor owns the failure output. Reorder createLakebasePool so a caller-supplied logger overrides the appkit default. Also trims the Lakebase auth hint and prunes redundant tests. Signed-off-by: Galymzhan --- packages/appkit/src/connectors/lakebase/index.ts | 4 +--- .../src/cli/commands/doctor/checks-existence.ts | 2 +- .../shared/src/cli/commands/doctor/checks.test.ts | 11 ----------- .../src/cli/commands/doctor/databricks-client.ts | 9 ++++++++- packages/shared/src/cli/commands/doctor/index.test.ts | 7 +------ .../src/cli/commands/doctor/resolve-targets.test.ts | 10 ---------- 6 files changed, 11 insertions(+), 32 deletions(-) diff --git a/packages/appkit/src/connectors/lakebase/index.ts b/packages/appkit/src/connectors/lakebase/index.ts index 17d7491f6..8de651471 100644 --- a/packages/appkit/src/connectors/lakebase/index.ts +++ b/packages/appkit/src/connectors/lakebase/index.ts @@ -13,11 +13,9 @@ import { createLogger } from "../../logging/logger"; * @returns PostgreSQL pool with appkit integration */ export function createLakebasePool(config?: Partial): Pool { - const logger = createLogger("connectors:lakebase"); - return createLakebasePoolBase({ + logger: createLogger("connectors:lakebase"), ...config, - logger, }); } diff --git a/packages/shared/src/cli/commands/doctor/checks-existence.ts b/packages/shared/src/cli/commands/doctor/checks-existence.ts index 71e29112e..49e6473fe 100644 --- a/packages/shared/src/cli/commands/doctor/checks-existence.ts +++ b/packages/shared/src/cli/commands/doctor/checks-existence.ts @@ -266,7 +266,7 @@ function lakebaseAuthHint(message: string): string | undefined { return ( "Lakebase uses an OAuth token as the password, so this usually means the" + " PGUSER/role doesn't match your identity. Check PGUSER is your exact" + - " login (the literal `user@domain`, not URL-encoded)." + " login." ); } return undefined; diff --git a/packages/shared/src/cli/commands/doctor/checks.test.ts b/packages/shared/src/cli/commands/doctor/checks.test.ts index 238da7ba5..040579a17 100644 --- a/packages/shared/src/cli/commands/doctor/checks.test.ts +++ b/packages/shared/src/cli/commands/doctor/checks.test.ts @@ -61,17 +61,6 @@ describe("checkConfig", () => { expect(result.code).toBe("ENV_MISSING"); }); - it("passes when all env vars are set to real-looking values", async () => { - process.env.DOCTOR_TEST_ENV = "abc123def456"; - const result = await checkConfig(target()); - expect(result.status).toBe("ok"); - }); - - it("passes when a resource declares no env vars", async () => { - const result = await checkConfig(target({ envVars: [] })); - expect(result.status).toBe("ok"); - }); - it("reports all missing env vars when several are unset", async () => { const result = await checkConfig( target({ envVars: ["DOCTOR_TEST_ENV", "DOCTOR_TEST_ENV_2"] }), diff --git a/packages/shared/src/cli/commands/doctor/databricks-client.ts b/packages/shared/src/cli/commands/doctor/databricks-client.ts index 2ca549d1d..fdf5457b0 100644 --- a/packages/shared/src/cli/commands/doctor/databricks-client.ts +++ b/packages/shared/src/cli/commands/doctor/databricks-client.ts @@ -85,6 +85,13 @@ export async function getLakebasePool( throw err; } - const pool = appkit.createLakebasePool({ workspaceClient: client }); + // Silence Lakebase's own logger. It defaults to an error-only console logger + // that dumps the raw SDK ApiError (stack + full response blob) to stderr on a + // failed token fetch. doctor classifies and prints that failure itself, so the + // library's dump is just noise on top of our clean one-line report. + const pool = appkit.createLakebasePool({ + workspaceClient: client, + logger: { error: false }, + }); return pool as unknown as LakebasePoolHandle; } diff --git a/packages/shared/src/cli/commands/doctor/index.test.ts b/packages/shared/src/cli/commands/doctor/index.test.ts index 3e72dec60..d23027a7e 100644 --- a/packages/shared/src/cli/commands/doctor/index.test.ts +++ b/packages/shared/src/cli/commands/doctor/index.test.ts @@ -10,7 +10,7 @@ function makeTempDir(): string { describe("loadEnvFile", () => { const dirs: string[] = []; - const savedKeys = ["DOCTOR_ENVFILE_A", "DOCTOR_ENVFILE_B"]; + const savedKeys = ["DOCTOR_ENVFILE_B"]; afterEach(() => { for (const d of dirs) fs.rmSync(d, { recursive: true, force: true }); @@ -26,11 +26,6 @@ describe("loadEnvFile", () => { return p; } - it("loads values from the given env file into process.env", () => { - loadEnvFile(writeEnv("DOCTOR_ENVFILE_A=from-file\n")); - expect(process.env.DOCTOR_ENVFILE_A).toBe("from-file"); - }); - it("overrides an already-set value (so it beats the auto-loaded .env)", () => { process.env.DOCTOR_ENVFILE_B = "from-default-env"; loadEnvFile(writeEnv("DOCTOR_ENVFILE_B=from-explicit-file\n")); diff --git a/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts b/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts index 63d558494..3988c82c9 100644 --- a/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts +++ b/packages/shared/src/cli/commands/doctor/resolve-targets.test.ts @@ -298,14 +298,4 @@ describe("resolveTargetsFromCwd", () => { dirs.push(dir); expect(resolveTargetsFromCwd(dir)).toEqual([]); }); - - it("reads the manifest from the given cwd", () => { - const dir = makeTempDir(); - dirs.push(dir); - fs.writeFileSync( - path.join(dir, "appkit.plugins.json"), - JSON.stringify(MANIFEST), - ); - expect(resolveTargetsFromCwd(dir)).toHaveLength(2); - }); });