review

# review Non-deterministic architectural check that complements the deterministic gate (`bun run check` — typecheck, lint, format, unit tests, skill lint). Determinism catches typos; this catches drift. ## When to invoke - After finishing any non-trivial change, before claiming "done." - On user `/review`. ## Inputs 1. Diff scope: - `git diff` (uncommitted) AND - `git diff main...HEAD` if on a feature branch. - If outside a git repo, review uncommitted changes only. 2. `CLAUDE.md`. Read both before forming any judgment. All other rules required for this review are embedded in this skill below — do not depend on any external doc. ## Project layering (memorize) A bun workspace with two packages. `packages/client` (`@metabase/client`) is the private Metabase API client, built by `tsdown` to a `dist/` its `exports` map names; `packages/cli` (`@metabase/cli`) is the publishable CLI built on it, and inlines the client at build time. The e2e tier lives at the repo root under `tests/e2e/` and drives the built binary; unit tests sit beside their source under `packages/<p>/src/**/*.test.ts`. ``` packages/client/src/ index.ts public barrel: client factory, error taxonomy, Credential, parseJson client.ts createClient — composes the resource namespaces onto one transport resources/ one file per Metabase resource; every /api/ path and transport call http/ the HTTP boundary — transport.ts, errors.ts, network-error.ts, response-shape.ts, sanitize.ts, retry.ts, oauth.ts testing/ the shipped test doubles — fake-client.ts, fetch-capture.ts auth/ OAuth login flow: credential.ts, pkce.ts, callback-server.ts, oauth-*.ts version/ tag.ts, probe.ts, known-range.ts (KNOWN_RANGE), profile.ts (ServerProfile), features.ts (FEATURE_RULES — the only file comparing a Metabase major), requirements.ts (METHOD_REQUIREMENTS), requirement-check.ts (checkFeatures), capability-error.ts, refusal.ts (explainer → explain / explainWalk / refuse / refuseAfterReading, explainRefusal) domain/ Metabase API resource Zod schemas + inferred types errors.ts MetabaseError taxonomy, toMetabaseError, isFileNotFoundError, errorMessage, isNonInterruptFailure list.ts ListResult<T> = { data, total } — what a one-response list method returns url.ts normalizeUrl, displayUrl, assertEndpointOrigin — the only home for new URL() json.ts parseJson — the only home for JSON.parse poll.ts pollUntil — the wait-loop home (http/retry.ts drives its own backoff wait) paginate.ts, signal.ts, predicates.ts, json-pointer.ts packages/cli/src/ cli.ts entrypoint; only file allowed to call process.exit main.ts root citty command, lazy subCommands commands/ CLI shell — defineMetabaseCommand, flags, prompts, orchestration core/ pure logic, no CLI deps core/auth/ credential storage + verification core/config.ts flag → env → stored resolver core/env.ts readEnv — every MB_* env name, no raw process.env reads elsewhere core/user-agent.ts USER_AGENT, the CLI's wire identity core/npm-registry.ts the upgrade check's dist-tags lookup — the CLI's only fetch() core/system-ca.ts trustSystemCa — the only node:tls in the repo, a CLI prerogative output/ presentation; takes typed values; renders text or JSON runtime/ platform glue: stdin, csv, yaml, citty helpers ``` Allowed import direction — the package boundary first, then within each package: - **Nothing in the client may import from the CLI** — not by an `@metabase/cli` specifier, not by a relative path that escapes the client's own src tree. The reverse is expected: the CLI consumes the client as `@metabase/client/<path>`. - The client's dependency budget is `zod` + `node:` builtins. Root devDependencies hoist, so the resolver will not reject a third dependency — a production client file importing anything else is FAIL even though it type-checks. - `domain/*` imports only `zod`, sibling `domain/*` files and — for a reader that selects a wire schema by generation (`<resource><Endpoint>Schema(features)`) — `import type { Features } from "../version/features"`. A value import from `version/`, or anything else from elsewhere in either package, is FAIL. - `resources/*` is imported by `client.ts` and by sibling `resources/*` files, and by nothing else. A `domain/`, `http/`, `auth/` or `version/` file importing a resource is FAIL — the direction runs one way, and the permitted importers are an allowlist, so a directory added later is not exempt for having arrived after the rule. - `commands/*` may import `core/*`, `output/*`, `runtime/*`, and any `@metabase/client/*` path. - `core/*` may import `runtime/*` and `@metabase/client/*`. Never `commands/`, never `output/`. - `output/*` may import `runtime/*`, `core/*` except `core/auth/`, and `@metabase/client/*` **except `@metabase/client/http/`** — presentation consumes the error taxonomy from `@metabase/client/errors`, not the HTTP boundary. Never `commands/`. - `runtime/*` may import `@metabase/client/*` (typed errors are the project's pattern: `runtime/body.ts` uses `parseJson` and `ConfigError`, `runtime/yaml.ts` uses `ValidationError`). It must not import from `commands/`, `core/`, or `output/`. ## Domain-file contract Every Metabase API resource in `packages/client/src/domain/<resource-singular>.ts` exports exactly two things per resource — the client's agent-facing contract: 1. **`<Resource>`** — `z.object({ ... }).loose()` for the full API shape, with the type inferred via `z.infer`. `.loose()` is the default so Metabase API additions don't break the parse — tighten over time, don't preempt. (Zod 4 — `.passthrough()` is deprecated; use `.loose()`.) 2. **`<Resource>Compact`** — `<Resource>.pick({ ... }).strip()` projection with its inferred type. The trailing `.strip()` is **mandatory**: `.pick()` on a `.loose()` parent inherits the loose catchall and silently passes every API field through at parse time, which means the "compact" projection isn't actually compact. A diff that lands a Compact missing `.strip()` is FAIL — quote `<Resource>.pick({...})` without trailing `.strip()`. Presentation is CLI-owned, never the client's: the `<resource>View` (`ResourceView<T>` with `compactPick: <Resource>Compact` and `tableColumns: ColumnDef<T>[]`) lives in `packages/cli/src/output/views/<r>.ts`, with any `format:` cell helper colocated there. A `ResourceView`, a `ColumnDef`, a `tableColumns` list, or a terminal-presentation `format:` helper appearing in `packages/client/src/domain/` is FAIL. Inlining a column list inside a command is FAIL. Naming: `<Resource>` is PascalCase (`Card`); the CLI-side `<resource>View` is camelCase (`cardView`). `<Resource>` is the _resource name_, not the file name — a file may host multiple resources (e.g. `domain/user.ts` → `CurrentUser` + `CurrentUserCompact`); the pair contract holds per-resource. Adding a resource is additive: a new `packages/client/src/domain/<r>.ts` for the pair, `packages/client/src/resources/<r>.ts` for the methods, and `packages/cli/src/output/views/<r>.ts` for the view — consumed by commands as `client.<r>.<method>(…)`, `@metabase/client/domain/<r>` and `../../output/views/<r>`. A schema declared in `packages/cli` instead — or a command importing one by a relative path into the client's src tree — is FAIL. Editing existing commands to wire the schema is FAIL — commands import what they need. Do **not** require a fixture + parse-test pair — that is a tautology against Zod itself. The schema is contract-tested by the e2e tier when a command consuming it runs against the live API. A new or modified API resource without both exports, or that omits `.loose()` on the full schema, or that omits `.strip()` after `.pick()` on the Compact, or that hand-rolls `Array<Record<string, unknown>>` / inline `as { ... }` casts in commands instead of consuming `z.infer<typeof Schema>`, is a structural failure. `Record<string, unknown>` is forbidden in command files. The single exemption is `packages/cli/src/output/projection.ts` — the canonical structurally-untyped envelope for dot-path `--fields` projection over already-validated payloads. ## Resource-method contract `packages/client/src/resources/<r>.ts` exports `<r>Resource(transport)` and holds every `/api/` path the client sends. `packages/client/src/client.ts` composes each namespace onto the client, so a resource file with no line in `client.ts` is unreachable and FAIL. Eight conventions bind every method: 1. `client.<resource>.<method>(...)`, namespace named after the API resource (`mb.card`, singular). The resource is what Metabase calls the thing, not what it calls the route: the file and the namespace are `snippet` even though the endpoint is `/api/native-query-snippet`, because the domain schema, the CLI noun and the Metabase concept all read `snippet`. 2. Path parameters positional, then params, then options: `update(id, params, options?)`. 3. Params use Metabase's own field names verbatim — `f`, `model_id`, `include_inactive`. **No mapping layer, no renaming.** 4. Transport concerns (`signal`, `timeoutMs`, `retries`) live in the trailing `options`, never in params. 5. Wire envelope schemas are module-private to `resources/`; they never appear in `domain/`. 6. Methods return domain values, never wire envelopes. Non-paginated lists return `ListResult<T>` = `{ data, total }`. 7. A string path parameter always goes through `encodeURIComponent`. 8. Every method carries the endpoint's description as a doc comment. A method violating any of the eight is FAIL — quote the signature or the path template. Convention 7 is the one with a silent failure mode: an un-escaped string interpolated into a path template lets a `/` or `?` in a setting key or collection ref rewrite the request. A new or changed method comes with its wire test in `packages/client/src/resources/<r>.test.ts` — one `it` per method, asserting URL, verb, headers and body in a single `toEqual` over `capture.calls` from `@metabase/client/testing/fetch-capture`. That test is what pins the path and query vocabulary; a method landed without one is FAIL. ## Version contract `packages/client/src/version/features.ts` is the only production file that compares a Metabase major (`major-comparison-guard.test.ts` scans both packages); `profile.ts` places a version and `tag.ts` parses one. A `.major` comparison or a literal major used as a decision anywhere else is FAIL. Feature names describe a behaviour (`transformTargetTableId`), never a version (`transformV61Shape` is FAIL), and every version-gated rule must flip somewhere inside `KNOWN_RANGE` — `features.test.ts` enforces it, so a diff that raises `KNOWN_RANGE.min` without deleting the rules that went constant is FAIL. Every method in `resources/*.ts` has a matching `METHOD_REQUIREMENTS` entry under its own namespace, strictest feature first (`requirements.test.ts`). A gated method not exported through exactly one of the resource's `explain`, `explainWalk`, `refuse` or `refuseAfterReading` (`explainer(transport, "<ns>")`) is FAIL (`explain-guard.test.ts`). A method refusing before the wire is FAIL unless the refusal guards a parameter an unsupporting server would drop or rewrite without a word, asked through `transport.requireFeatures` or a value check on `transport.preflightServer()` whose refusal is thrown through `transport.refuseBeforeSending` (a refusal thrown directly, or a value check reading `verifiedServer`, is FAIL: the caller could not skip it), or the method is one whose every call such a server would answer wrongly without a word, exported through `refuse` and ending in optional `RequestOptions`, or through `refuseAfterReading` with a reader when it answers from a setting the session properties carry (a `refuse`-wrapped method that reads the session properties itself pays a second request); either way with a comment saying what the server would do; a parameter the server rejects belongs in `explain`'s parameter callback, with the request fields it travels in. A method that chooses what to send from `transport.server()`'s `features` is FAIL — that profile may be a stale cache; send what every server accepts, or decide against `verifiedServer`. A method without an entry, or a command under `packages/cli/src/commands/` declaring `requires` that differs from the `client.<ns>.<method>(` calls in its body (`requires-guard.test.ts`), is FAIL. A `defineMetabaseCommand` declaring a version floor or a token feature is FAIL — the features are derived from `requires`, never declared. Version-driven drift is a `<Resource>WireV<N>` schema plus a converter, selected by a feature and handed to `requestParsed` through an exported `<resource><Endpoint>Schema(features): z.ZodType<Resource>` reader. The canonical schema is the newest server's shape; a field an older server cannot report is `.nullable()`. FAIL: `.optional()` meaning "an older server omits it"; a converter mapping two distinguishable wire states onto one canonical state; a `z.union` of generation shapes parsed by trial (runtime shape detection); a converter that leaves a wire-only field on the canonical value; a discriminated union of generations as a method's public return type; version prose in `packages/cli/skill-data/` where a `requires:` frontmatter key or a `<!-- requires: … -->` … `<!-- /requires -->` fence belongs (markers on their own lines, never inside a table). ## Forbidden patterns and their permitted homes CLAUDE.md declares these as a **separate policy per package**, applied to production source only (`*.test.ts` files are exempt). A pattern's permitted home in one package says nothing about the other: `JSON.parse` has a home in the client and none in the CLI, `fetch` has a home in each but at different paths. The e2e-side restrictions below are part of this review too. ### The client — every file under `packages/client/src/` | Pattern | Permitted only in | | ---------------------------------------------------------------------------------------------- | --------------------------------------- | | bare `fetch(`, `globalThis.fetch`, third-party HTTP libs (`got`/`axios`/`node-fetch`/`undici`) | `http/**` | | `new URL(` | `http/**`, `url.ts` | | `JSON.parse(` | `json.ts` | | `setTimeout` inside an `await` wait loop | `poll.ts`, `http/retry.ts` | | `process.stdout.write`, `process.stderr.write` | nowhere — the client owns no output | | `process.exit(` | nowhere — the client never ends the run | | `@clack/prompts`, `@clack/core` | nowhere — prompting is the CLI's job | | `fs`, `fs/promises`, `node:fs`, `node:fs/promises` | nowhere — the client touches no files | | `child_process`, `node:child_process` | nowhere — the client spawns nothing | | a `.ts` extension in an import specifier | nowhere | | any dependency beyond `zod` and `node:` builtins | nowhere | | an import resolving into `packages/cli` | nowhere | ### The CLI — every file under `packages/cli/src/` | Pattern | Permitted only in |