review

name	review
description	Strict end-of-task review. Use before marking any non-trivial task done. Returns axis-by-axis pass/fail; any FAIL blocks completion. Trigger on user /review or end-of-task self-check.

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, capabilities.ts, preflight-error.ts
  domain/                   Metabase API resource Zod schemas + inferred types
  errors.ts                 MetabaseError taxonomy, toMetabaseError, isFileNotFoundError, errorMessage
  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 + semver + 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 and sibling domain/* files. Nothing from elsewhere in either package.
• 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.

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, semver, node: builtins	nowhere
an import resolving into packages/cli	nowhere

The CLI — every file under packages/cli/src/

Pattern	Permitted only in
process.exit(	cli.ts
process.stderr.write	cli.ts, output/**
process.stdout.write	output/**
JSON.parse(	nowhere — use parseJson from @metabase/client/json
new URL(	nowhere — use the helpers in @metabase/client/url
bare fetch(, globalThis.fetch, third-party HTTP libs	core/npm-registry.ts (not a Metabase endpoint)
setTimeout inside an await wait loop	nowhere — use pollUntil from @metabase/client/poll
child_process (spawn, exec, etc.)	runtime/process.ts
@clack/prompts, @clack/core	output/prompt.ts
Record<string, unknown>	nowhere under commands/ — the walker scopes this rule to command files; output/projection.ts is the canonical untyped envelope and sits outside that scope
an import of @metabase/client/http/** from output/**	nowhere — presentation uses @metabase/client/errors
a .ts extension in an import specifier	nowhere
process.env["MB_*"], process.env["METABASE_*"]	core/env.ts — CLI env vars go through readEnv. A CLAUDE.md rule, not walker-enforced; judge it by reading, not by trusting the gate
an /api/… path literal ("/api/ or )

The last two are absolute: neither has a sanctioned home anywhere in the CLI. A diff that lands a command naming a path or driving the transport is FAIL.

What those two patterns bound is a path literal and a transport call, not every client function that can open a socket. commands/auth/login.ts calls tryDiscoverMetadata from @metabase/client/http/oauth, which fetches the OAuth discovery document before any credential exists and therefore before a client can be constructed; that one is sanctioned. A second such call from commands/ is FAIL — the pattern rules cannot see it, so review is the only thing that can.

Two rules bind both packages but come from elsewhere, so judge them repo-wide rather than per-package:

• console.{log,warn,error,info,debug} — forbidden everywhere, including output/** and tests/. .oxlintrc.json sets no-console: error with no overrides; write through output/render.ts instead.
• Platform env vars (XDG_CONFIG_HOME, APPDATA, npm_config_prefix) are read where they are used — core/paths.ts, core/install-method.ts, core/auth/temp-config-home.ts. readEnv owns the MB_/METABASE_ family only; do not flag the others.

The test tree — every file under the repo-root tests/, outside the walker but still reviewed here

Pattern	Permitted only in
process.exit(, process.std*.write	tests/e2e/setup/**
/api/testing/snapshot, /api/testing/restore	tests/e2e/setup/** — a beforeEach there restores before every test
fetch( against Metabase	tests/e2e/setup/** (bootstrap, oauth harness)
child_process / execa	tests/e2e/run-cli.ts, tests/e2e/setup/**, scripts/**
Hand-written ENOENT shape check (error.code === "ENOENT")	nowhere — use isFileNotFoundError from @metabase/client/errors
Inline JSON.parse(...) ; Schema.parse(...)	nowhere — use parseJson(raw, Schema, { source }) from @metabase/client/json

Type strictness rules (always)

• No as cast in any form: as X, as unknown as, as never, as any. Use type guards or Zod .parse.
• No any in declared types.
• No Record<string, unknown> used as a typed value (it is allowed only inside the structurally-untyped output envelope).
• No ! non-null assertions.
• No // @ts-ignore, // @ts-nocheck, // @ts-expect-error (paired comment with // reason: and // FIXME(date) only).
• Every cross-network value passes through Zod .parse before downstream code touches it.
• Imports are extensionless. Type-only imports use import type.

Test-parity rules

• New packages/cli/src/commands/<noun>/<verb>.ts ⇒ a tests/e2e/<noun>.e2e.test.ts exists and exercises that subcommand end-to-end via runCli. Adding a new command without an e2e test is FAIL.
• New packages/cli/src/commands/<noun>/<verb>.ts (any leaf) ⇒ packages/cli/src/runtime/command-help.test.ts's literal ALL_COMMANDS list includes the new path. Renaming or deleting a command without updating the list is FAIL.
• New or changed method in packages/client/src/resources/<r>.ts ⇒ packages/client/src/resources/<r>.test.ts covers it with a wire assertion over capture.calls (URL, verb, headers, body) built on captureFetch from @metabase/client/testing/fetch-capture. A method with no wire test is FAIL — nothing else pins the path or the query vocabulary at the unit tier.
• New packages/client/src/domain/<r>.ts ⇒ no fixture-and-parse-test pair required. A unit test of the form Schema.parse(fixture).toEqual(fixture) is a tautology against Zod and is FAIL if introduced. The schema's contract is tested by the e2e tier hitting the live API.
• New pure helper anywhere in packages/client/src/ or in the CLI's core/, output/, runtime/ ⇒ a colocated unit test (<name>.test.ts beside <name>.ts, inside the same package) imports from it.
• New helper with infinite input space (URL parsing, JSON parsing, projection, byte cap, polling) ⇒ a property test (fast-check).

E2E test contract

• E2E tests drive the built packages/cli/dist/cli.mjs through runCli from tests/e2e/run-cli.ts. Direct execa, spawn, or child_process calls inside tests/e2e/**/*.e2e.test.ts are FAIL.
• E2E tests read admin credentials via readBootstrap() from tests/e2e/bootstrap-data.ts and seeded entity ids via SEEDED from tests/e2e/seed/seeded.ts. Inline fetch to Metabase, hard-coded API keys, hard-coded entity ids, or calls to /api/setup from a test are FAIL.
• The Bootstrap Zod schema and BOOTSTRAP_FILE_PATH constant live only in tests/e2e/bootstrap-data.ts. Re-declaring either in tests/e2e/setup/bootstrap.ts (or anywhere else) is FAIL — the writer imports from the reader to prevent shape drift.
• --json output assertions parse through the schema imported from the package that owns it: @metabase/client/domain/<r> for a resource, ../../packages/cli/src/commands/<noun>/<verb> for a command-shaped output (e.g. LoginResult, AuthStatus, <Resource>ListEnvelope). Re-declaring a z.object({...}) in a test that mirrors either export is FAIL.
• Each test or test-group gets its own XDG_CONFIG_HOME via mkTempConfigHome(). Sharing config home across tests that mutate credentials is FAIL.
• A suite whose command declares non-baseline capabilities gates itself with requireServer("<lane>", {...}) from tests/e2e/server-gate.ts, passing a lane label naming the describe or test the gate guards so an unmet gate is reported in the closing coverage block. A suite that fails instead of skipping on a server that cannot satisfy the command is FAIL.

Axes (output one row per axis)

For each axis below, output PASS, FAIL, or UNKNOWN. For every FAIL and UNKNOWN, quote the offending snippet with path:line and a one-sentence explanation.

1. Layering — direction rules above, judged against the package the file is in.
2. Type strictness — rules above.
3. Output discipline — console.*, process.exit, process.std*.write outside permitted homes. A file under packages/client/src/ writing to a stream or ending the process is FAIL regardless of path — the client has no permitted home for either.
4. Boundary discipline — JSON.parse, raw fetch/globalThis.fetch/third-party HTTP libraries (got/axios/node-fetch/undici)/new URL, child_process, polling setTimeout outside permitted homes. Read the per-package table — the CLI has no home for JSON.parse, new URL, or a wait loop, and delegates all three to the client. 4a. Package boundary — an import in packages/client/** that resolves into packages/cli (by @metabase/cli specifier or by a relative path escaping the client's src tree) is FAIL. A production client file importing anything beyond zod, semver, and node: builtins is FAIL. A CLI file reaching into the client by relative path instead of the @metabase/client/<path> specifier is FAIL. packages/cli/src/output/** importing @metabase/client/http/** is FAIL. Anything outside client.ts and resources/ importing resources/ is FAIL. 4b. Request placement — a file under packages/cli/src/commands/ that names an /api/… path literal, or calls requestParsed/requestRaw/requestStream/paginatePages, is FAIL. Quote the path template and name the resource method it belongs on.
5. Resource contract — every new or changed resource in packages/client/src/domain/ exports <Resource> (with .loose()) and <Resource>Compact (with — the trailing is mandatory), and its lives CLI-side in (a // in the domain file is FAIL); naming follows the convention; commands consume rather than or inline casts. Every new or changed method in satisfies all eight conventions above, is composed onto , and returns a domain value rather than the server's envelope.

Output format

| # | Axis | Status | Detail |
|---|------|--------|--------|
| 1 | Layering | PASS | |
| 2 | Type strictness | FAIL | packages/cli/src/commands/card/get.ts:12 — `as unknown as Card` |
| 3 | Output discipline | PASS | |
...

Then exactly one of:

• RESULT: PASS — task may be marked done.
• RESULT: FAIL (<n> failures, <m> unknowns) — fix issues above before marking done.

If FAIL, follow the table with a bulleted list of the next concrete actions to address each failure (one bullet per FAIL, with path:line).

Rules for the review itself

• Read-only. Do not modify any files.
• Do not run tsc, oxlint, oxfmt, or the test suites here. Those are separate; this skill is the architectural / non-deterministic layer.
• Do not green-light by default. If you cannot determine an axis from the diff, mark UNKNOWN with the reason — never PASS.
• Be ruthless. The point of this skill is to be strict where the type-checker can't be.