Skip to content
Merged
4 changes: 2 additions & 2 deletions .claude/skills/add-e2e-test/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ const skipReason = requireServer("transform › transform e2e", ["transforms"]);
describe.skipIf(skipReason !== null)("transform e2e", () => {
```

- If any method the command under test calls needs a feature — look the command's `requires` up in `METHOD_REQUIREMENTS` (`packages/client/src/version/requirements.ts`), or read `help --json`'s `requires.features` — the suite must gate itself on those feature names. `requireServer(lane, features)` feeds the persisted server probe through `createServerProfile` and the production `checkFeatures` — the same check the CLI's preflight and the client's `require()` run — and returns the skip reason (or `null`). Its first argument is the lane label naming the describe or test the gate guards; an unmet gate appends it to `.gate-skips.<stack>.json`, which the closing block prints so a skipped lane is reported rather than counted as a passing one.
- If any method the command under test calls needs a feature — look the command's `requires` up in `METHOD_REQUIREMENTS` (`packages/client/src/version/requirements.ts`), or read `help --json`'s `requires.features` — the suite must gate itself on those feature names. `requireServer(lane, features)` feeds the persisted server probe through `createServerProfile` and the production `checkFeatures` — the same check the client asks of a fresh probe to explain a refusal — and returns the skip reason (or `null`). Its first argument is the lane label naming the describe or test the gate guards; an unmet gate appends it to `.gate-skips.<stack>.json`, which the closing block prints so a skipped lane is reported rather than counted as a passing one. A lane that pins how a server _without_ a feature answers gates on `requireServerWithout(lane, features)`, which skips — and reports — on a stack that has them all.
- The point is that a lane **passes or skips**, never fails, on a server that cannot satisfy the command. A suite that hits HTTP 402 or 404 because nobody gated it is a broken lane, not a real failure.
- Where every stack answers but the answer differs by generation — a drift site whose canonical JSON carries `null` on the servers that cannot report a field, a status code that changed, a run id that is numeric on newer servers — do not gate. Branch with `serverHas("<feature>")` and assert the exact outcome each generation produces, so the lane runs everywhere and pins both shapes:

Expand Down Expand Up @@ -164,7 +164,7 @@ Which package a schema belongs to is a boundary question, not a stylistic one. A
**5b. Assertion strictness.** Every assertion is exact:

- **Parsed payloads:** one `toEqual({ ...full expected... })` over the parsed object — never a sequence of `expect(parsed.id).toBe(...)`, `expect(parsed.name).toBe(...)`. The whole point of `parseJson` is that the structure is now in your hands; pinning each field individually leaves the rest untested and lets a regression that flips an unchecked field pass silently.
- **Exit codes:** `expect(result.exitCode).toBe(<n>)` with the exact integer. Never `.not.toBe(0)`. The taxonomy is `packages/client/src/errors.ts`: `ConfigError` → 2, `CapabilityError` (`packages/client/src/version/preflight-error.ts`) → 2, `AbortError` → 130, everything else (`HttpError`, `ValidationError`, `NetworkError`, `TimeoutError`, `UnknownError`) → 1. A test that says "non-zero" doesn't distinguish "the right error fired" from "any failure at all".
- **Exit codes:** `expect(result.exitCode).toBe(<n>)` with the exact integer. Never `.not.toBe(0)`. The taxonomy is `packages/client/src/errors.ts`: `ConfigError` → 2, `CapabilityError` (`packages/client/src/version/capability-error.ts`) → 2, `AbortError` → 130, everything else (`HttpError`, `ValidationError`, `NetworkError`, `TimeoutError`, `UnknownError`) → 1. A test that says "non-zero" doesn't distinguish "the right error fired" from "any failure at all".
- **Error strings:** the harness is never a TTY, so the CLI renders errors as a JSON envelope on stderr rather than plain text. Assert through `cliErrorMessage(result.stderr)` from `./cli-error`, which unwraps that envelope and hands back the human message:

```ts
Expand Down
17 changes: 6 additions & 11 deletions .claude/skills/add-resource-command/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ For a typical list/get pair on a new resource:

1. `packages/client/src/domain/<r>.ts` — the schema pair (`<Resource>` + `<Resource>Compact`). No fixture, no schema-parse unit test.
2. Updated `packages/client/src/index.ts` — re-export every value the domain file exports from the public barrel.
3. `packages/client/src/resources/<r>.ts` — `<r>Resource(transport)`, one method per endpoint, each opening with `await transport.require("<r>.<method>")`.
3. `packages/client/src/resources/<r>.ts` — `<r>Resource(transport)`, one method per endpoint.
4. Updated `packages/client/src/version/requirements.ts` — one `"<r>.<method>"` entry per method in `METHOD_REQUIREMENTS`, and a rule in `packages/client/src/version/features.ts` if the route is gated by a major or a token feature.
5. Updated `packages/client/src/client.ts` — compose the namespace onto the client as `<r>: <r>Resource(transport)`.
6. `packages/client/src/resources/<r>.test.ts` — the wire test: one `it` per method, asserting the exact URL, method, headers and body through `captureFetch`.
Expand Down Expand Up @@ -159,7 +159,6 @@ export function cardResource(transport: Transport) {
params: CardListParams = {},
options: RequestOptions = {},
): Promise<ListResult<Card>> {
await transport.require("card.list");
const data = await transport.requestParsed(CardApiList, "/api/card", {
...options,
query: { f: params.f, model_id: params.model_id },
Expand All @@ -169,13 +168,11 @@ export function cardResource(transport: Transport) {

/** Get one card by id. */
async function get(id: number, options: RequestOptions = {}): Promise<Card> {
await transport.require("card.get");
return transport.requestParsed(Card, `/api/card/${id}`, { ...options });
}

/** Create a card — a question, a model, or a metric — from a full card body. */
async function create(params: CardCreateInput, options: RequestOptions = {}): Promise<Card> {
await transport.require("card.create");
return transport.requestParsed(Card, "/api/card", { ...options, method: "POST", body: params });
}

Expand All @@ -185,7 +182,6 @@ export function cardResource(transport: Transport) {
params: CardUpdateInput,
options: RequestOptions = {},
): Promise<Card> {
await transport.require("card.update");
return transport.requestParsed(Card, `/api/card/${id}`, {
...options,
method: "PUT",
Expand All @@ -195,7 +191,6 @@ export function cardResource(transport: Transport) {

/** Archive (soft-delete) a card by id. Metabase models this as an update, not its own endpoint. */
async function archive(id: number, options: RequestOptions = {}): Promise<Card> {
await transport.require("card.archive");
return update(id, { archived: true }, options);
}

Expand All @@ -219,7 +214,7 @@ The nine conventions every method follows:
`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.
9. Every method opens with `await transport.require("<r>.<method>")`, keyed exactly as the method is reached on the client, and that key has an entry in `METHOD_REQUIREMENTS` (`packages/client/src/version/requirements.ts`): `[]` when every supported server answers the route, otherwise the feature names it needs, strictest first. `requirements.test.ts` fails a key the table lacks, a table key no method requires, and a method requiring a key from another resource's namespace. A method whose wire shape is selected by generation then reads `const { features } = await transport.server()` and parses with the reader from Step 1a.
9. Every method has an entry in `METHOD_REQUIREMENTS` (`packages/client/src/version/requirements.ts`), keyed exactly as the method is reached on the client: `[]` when every supported server answers the route, otherwise the feature names it needs, strictest first. A gated method is exported through `explain("<method>", method)`, where `const { explain, explainWalk, refuse, refuseAfterReading } = explainer(transport, "<r>")` from `packages/client/src/version/refusal.ts` opens the factory (take only the wrappers the resource uses), and a `<thing>Pages` method answering an async iterable through `explainWalk("<method>", method)`, so the server's rejection reads as the feature it lacks; nothing refuses on the table's word, and `explain-guard.test.ts` fails a gated method not wrapped exactly once. A parameter only some servers accept is handled by what an unsupporting server does with it: one it rejects (a 400 on an enum, a 404) is sent and named through `explain`'s third argument, a small named function beside the method that takes the method's arguments and returns `{ feature, fields }` entries — the feature, and the request `fields` the argument travels in as the server names them in a 400; one it ignores while a newer server requires it is sent on every server; one it drops or rewrites without a word is refused before the wire with `await transport.requireFeatures([...], options)` (a check by value reads `transport.preflightServer(options)` and refuses through `transport.refuseBeforeSending`, never by throwing), with a comment saying what the server would do. A method every call of which such a server would answer wrongly without a word (its whole answer misread, or its whole body dropped) is exported through `refuse("<method>", method)` instead of `explain`, with that comment beside it in the factory's return: `refuse` runs the method's whole `METHOD_REQUIREMENTS` entry through `transport.requireFeatures` before the call, so the method must end in optional `RequestOptions`. Such a method answering from a setting the session properties carry is exported through `refuseAfterReading("<method>", reader, method)` instead, where `reader` extends `SessionProperties` with the setting and `method` is a pure function of the properties read: the wrapper reads them afresh through `transport.probe`, judges the entry against the profile that read settled, and exposes a method taking only optional `RequestOptions`, so the call costs one request. `requirements.test.ts` fails a client method the table lacks and a table key no method answers to.

Convention 7 has a companion habit: when every path parameter on a resource is a numeric id, say so in a one-line comment at the top of the factory, so a reader knows the omission was decided rather than forgotten.

Expand Down Expand Up @@ -351,7 +346,7 @@ renderList(envelope, collectionItemView, ctx);

`collectForOutput` pulls only as far as the output byte budget can display, so an unbounded listing over a large collection costs a page or two rather than a full drain the cap then discards. It sizes each request through the `PageRequest` it hands the source — forward `max` and `pageSize` verbatim, or the budget cannot bound the walk.

Every command declares `requires`: the list of `"<r>.<method>"` keys its `run` body reaches (plus the methods of any helper it hands the client to), or `null` for a command that never touches a Metabase server. `packages/cli/src/commands/requires-guard.test.ts` reads the body and fails a declaration that differs from the calls in it, so the list is transcribed, not chosen. The preflight derives the features from `METHOD_REQUIREMENTS`, runs once per command against the profile's cached probe (or one live probe), and refuses with the client's own message and exit code 2; a command whose methods need nothing never preflights. `help --json` reports the derived `requires: { methods, features }`. Nothing on the command names a version.
Every command declares `requires`: the list of `"<r>.<method>"` keys its `run` body reaches (plus the methods of any helper it hands the client to), or `null` for a command that never touches a Metabase server. `packages/cli/src/commands/requires-guard.test.ts` reads the body and fails a declaration that differs from the calls in it, so the list is transcribed, not chosen. The CLI runs no check of its own: the client sends each call and explains a refusal by the method's features, with exit code 2. `help --json` reports the features derived from `METHOD_REQUIREMENTS` as `requires: { methods, features }`. Nothing on the command names a version.

The `<Resource>ListEnvelope` export is **mandatory**. It is consumed by JSON help (`--help --json`, via `outputSchema`) and by the matching e2e test (which imports it back to parse `--json` output). Do **not** redeclare the envelope shape inline anywhere. It is the _CLI's_ envelope — `{ data, returned, offset, limit?, total?, has_more, next_offset?, truncated? }`, declared in `packages/cli/src/output/types.ts` — and has nothing to do with the server's wire envelope, which stayed module-private in Step 2.

Expand Down Expand Up @@ -424,7 +419,7 @@ A comprehensive suite for a typical list/get pair covers, at minimum:

Assertions are exact at every level — these are not stylistic preferences, they are hard rules from CLAUDE.md and the `add-e2e-test` skill:

- **Exit codes** — always the exact integer (`toBe(0)`, `toBe(1)`, `toBe(2)`, `toBe(130)`). Never `.not.toBe(0)`. The `packages/client/src/errors.ts` taxonomy is fixed: `ConfigError`=2, `CapabilityError` (`packages/client/src/version/preflight-error.ts`)=2, `AbortError`=130, all others=1.
- **Exit codes** — always the exact integer (`toBe(0)`, `toBe(1)`, `toBe(2)`, `toBe(130)`). Never `.not.toBe(0)`. The `packages/client/src/errors.ts` taxonomy is fixed: `ConfigError`=2, `CapabilityError` (`packages/client/src/version/capability-error.ts`)=2, `AbortError`=130, all others=1.
- **Error strings** — always `toContain("<exact substring>")` or `toBe("<exact full string>")`. Never `toMatch(/.../i)`. Look the literal up in `packages/` and pin it. A regex with `\d+` or `.*` for a dynamic part is FAIL — build the expected string from the same source the production code consumed and assert with `toBe`.
- **Parsed payloads** — always one full `toEqual({ ... })`. Field-by-field `toBe` after `parseJson` is FAIL.

Expand Down Expand Up @@ -461,7 +456,7 @@ rg -n "/api/" packages/client/src/domain/<r>.ts && echo FAIL || echo OK # endp
rg -n "\.request(Parsed|Raw|Stream)\(" packages/client/src/resources/<r>.ts || echo "FAIL: no transport call — is this file doing anything?"
rg -n "\$\{[a-zA-Z_$][\w$]*\}" packages/client/src/resources/<r>.ts # every string interpolation: numeric id, or encodeURIComponent?
rg -n "<r>Resource" packages/client/src/client.ts || echo "FAIL: namespace not composed onto the client"
rg -n "transport\.require\(\"<r>\." packages/client/src/resources/<r>.ts || echo "FAIL: no require() — every method opens with one"
rg -n "explainer\(transport, \"<r>\"\)" packages/client/src/resources/<r>.ts || echo "CHECK: no explainer — right only when METHOD_REQUIREMENTS gates no <r> method"
rg -n "\"<r>\." packages/client/src/version/requirements.ts || echo "FAIL: no METHOD_REQUIREMENTS entries for <r>"
rg -n "\.major\s*(<|>|=)" packages/client/src/domain/<r>.ts packages/client/src/resources/<r>.ts && echo FAIL || echo OK # only version/features.ts compares a major

Expand Down Expand Up @@ -514,7 +509,7 @@ If either skill surfaces a structural issue (missing `.strip()`, a request built
- [ ] Closed enums pinned via `z.enum([...])` where the backend defines a closed set.
- [ ] Schema scope is query/agent-relevant fields only — no sync flags, fingerprints, audit timestamps, or other internal plumbing unless they drive an actual decision.
- [ ] No fixture or schema-parse unit test added.
- [ ] **Resource file** `packages/client/src/resources/<r>.ts` exports `<r>Resource(transport)`, holds every `/api/` path and every transport call, and follows all nine conventions — positional path params then params then options, Metabase's own field names, transport concerns in `options`, wire envelopes module-private, domain values returned, `ListResult<T>` for a non-paginated list, `encodeURIComponent` on every string path param, a doc comment per method, `await transport.require("<r>.<method>")` first in every method.
- [ ] **Resource file** `packages/client/src/resources/<r>.ts` exports `<r>Resource(transport)`, holds every `/api/` path and every transport call, and follows all nine conventions — positional path params then params then options, Metabase's own field names, transport concerns in `options`, wire envelopes module-private, domain values returned, `ListResult<T>` for a non-paginated list, `encodeURIComponent` on every string path param, a doc comment per method, a `METHOD_REQUIREMENTS` entry per method, and each gated method exported through exactly one of the `explain` / `explainWalk` / `refuse` / `refuseAfterReading` of `explainer(transport, "<r>")`.
- [ ] **Requirements** — every method has its `"<r>.<method>"` entry in `METHOD_REQUIREMENTS`; a gated route has a behaviour-named rule in `FEATURE_RULES` verified against the Metabase release branches; a shape that differs by generation has its `<Resource>WireV<N>` + converter + exported reader, with converter unit tests per generation.
- [ ] **Namespace composed** onto `packages/client/src/client.ts` as `<r>: <r>Resource(transport)`.
- [ ] **Wire test** `packages/client/src/resources/<r>.test.ts` asserts URL, method, headers and body for every method through `captureFetch`.
Expand Down
Loading
Loading