test: address cr round 1 + simplify findings on contract test

cr round 1 (non-blocking) + /simplify findings:

- Extract mockFetch + createClient to src/test-helpers.ts so both
  client.test.ts and contract.test.ts import a single fixture.
  tsconfig.json excludes the new file from the build output so it
  doesn't ship in dist/.
- Regen script: replace the `CONTENT=$(git show …)` capture with a
  direct `git show` stream inside the heredoc. Avoids bash command-
  substitution's trailing-newline strip and ARG_MAX edges on large
  specs.
- Regen script: run `git fetch --quiet origin` before `rev-parse`
  so `origin/main` resolves to the live upstream tip rather than
  a stale local ref.
- Multi-page listAll contract case: original test asserted only
  the first page; the new case mocks two pages and asserts both
  calls hit GET /v1/qurls (covers the pagination path properly).
- `assertSdkCallMatches` takes a `callIndex` param for the multi-
  page case and guards against a future SDK method calling fetch
  without an `init` object (clearer error than a bare access).
- Trim narrative-only comment explaining a lint workaround that
  would rot if the rule changed.
- Add CONTRIBUTING note: new public SDK methods require a matching
  contract case (alias methods included, so alias rewires can't
  silently slip past).

Snapshot-staleness detector (cr round 1 suggestion #3) filed as a
follow-up issue — scope-appropriate since it's a new CI workflow
rather than a change to this test.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Author: justin-layerv

CONTRIBUTING.md

@@ -44,6 +44,11 @@ was last validated against. If the regenerated snapshot breaks the
 contract test, that's the signal: qurl-service changed an endpoint the
 SDK depends on, and the SDK needs to be updated in lockstep.
 
+**Adding a new public SDK method?** Add a matching case to
+`src/contract.test.ts` asserting the exact `(verb, path-template)` the
+method calls. Alias methods (e.g. `extend` → `update`) get their own
+case so an alias rewire can't silently slip past.
+
 ## Pull Requests
 
 1. Fork the repo and create a branch from `main`

scripts/update-openapi-snapshot.sh

@@ -22,15 +22,25 @@ if [ ! -d "$QURL_SERVICE_DIR/.git" ]; then
   exit 2
 fi
 
+# Refresh remote refs so `origin/main` (the default) points at the current
+# upstream tip, not whatever was last fetched locally. Without this, a
+# stale checkout would silently pin the snapshot to an old SHA. Harmless
+# when the caller passes an explicit SHA or tag.
+git -C "$QURL_SERVICE_DIR" fetch --quiet origin
+
 # Resolve ref → commit SHA so the snapshot header is deterministic even if
 # the caller passed a branch name that moves later.
 SHA=$(git -C "$QURL_SERVICE_DIR" rev-parse "$REF^{commit}")
 SRC_FILE="api/openapi.yaml"
 
-# Fetch the file at the resolved SHA (doesn't require checkout). Piping
-# through cat keeps the script composable if someone redirects output.
-CONTENT=$(git -C "$QURL_SERVICE_DIR" show "${SHA}:${SRC_FILE}")
-
+# Write to a sibling tempfile and `mv` into place so a Ctrl-C mid-write
+# can't leave the committed snapshot truncated. `mv` on the same
+# filesystem is atomic; the trap cleans up the tempfile if the script
+# aborts before the rename. Streaming `git show` directly into the
+# heredoc (rather than capturing into a shell variable) avoids trailing-
+# newline truncation and ARG_MAX/memory edges on very large specs.
+TMP="${OUT}.tmp.$$"
+trap 'rm -f "$TMP"' EXIT
 {
   echo "# Snapshot of qurl-service/api/openapi.yaml — canonical API contract."
   echo "# This file is vendored (not auto-generated) so the contract test runs"
@@ -39,7 +49,8 @@ CONTENT=$(git -C "$QURL_SERVICE_DIR" show "${SHA}:${SRC_FILE}")
   echo "# Source: layervai/qurl-service@${SHA}"
   echo "# Regenerate with: scripts/update-openapi-snapshot.sh"
   echo "#"
-  printf '%s\n' "$CONTENT"
-} > "$OUT"
+  git -C "$QURL_SERVICE_DIR" show "${SHA}:${SRC_FILE}"
+} > "$TMP"
+mv "$TMP" "$OUT"
 
 echo "wrote $OUT (source: qurl-service@${SHA:0:12})"

src/client.test.ts

@@ -11,30 +11,7 @@ import {
   TimeoutError,
   ValidationError,
 } from "./errors.js";
-
-function mockFetch(response: {
-  status: number;
-  body?: unknown;
-  headers?: Record<string, string>;
-}): typeof globalThis.fetch {
-  return vi.fn().mockResolvedValue({
-    ok: response.status >= 200 && response.status < 300,
-    status: response.status,
-    statusText: response.status === 200 ? "OK" : "Error",
-    headers: new Headers(response.headers ?? {}),
-    json: () => Promise.resolve(response.body),
-    text: () => Promise.resolve(JSON.stringify(response.body)),
-  } satisfies Partial<Response> as Response);
-}
-
-function createClient(fetchFn: typeof globalThis.fetch): QURLClient {
-  return new QURLClient({
-    apiKey: "lv_live_test",
-    baseUrl: "https://api.test.layerv.ai",
-    fetch: fetchFn,
-    maxRetries: 0,
-  });
-}
+import { mockFetch, createClient } from "./test-helpers.js";
 
 describe("QURLClient", () => {
   it("creates a QURL", async () => {

src/contract.test.ts

@@ -3,7 +3,7 @@ import { readFileSync } from "node:fs";
 import { fileURLToPath } from "node:url";
 import { dirname, join } from "node:path";
 import { parse as parseYaml } from "yaml";
-import { QURLClient } from "./client.js";
+import { mockFetch, createClient } from "./test-helpers.js";
 
 // Contract test: every SDK method must call the exact (verb, path) pair
 // that qurl-service's OpenAPI spec declares. Catches two failure modes:
@@ -56,12 +56,26 @@ function assertSdkCallMatches(
   fetchFn: typeof globalThis.fetch,
   expectedVerb: Verb,
   expectedTemplate: string,
+  callIndex: number = 0,
 ): void {
   const calls = (fetchFn as unknown as ReturnType<typeof vi.fn>).mock.calls;
-  if (calls.length === 0) {
-    throw new Error(`SDK made no fetch call; expected ${expectedVerb} ${expectedTemplate}`);
+  if (calls.length <= callIndex) {
+    throw new Error(
+      `SDK made ${calls.length} fetch call(s); expected at least ${callIndex + 1} ` +
+        `(looking for ${expectedVerb} ${expectedTemplate})`,
+    );
+  }
+  const [rawUrl, init] = calls[callIndex];
+  // Guard against a future SDK method calling fetch(url) with no options —
+  // today every path in client.ts passes an `init` object, but the check is
+  // cheap insurance against a regression with a clearer error than the
+  // undefined-access TypeError a bare access would throw.
+  if (!init) {
+    throw new Error(
+      `SDK called fetch without an init object; cannot verify method. ` +
+        `Expected ${expectedVerb} ${expectedTemplate}.`,
+    );
   }
-  const [rawUrl, init] = calls[0];
   const actualVerb = (init as RequestInit).method as Verb;
   const actualPath = new URL(rawUrl as string).pathname;
 
@@ -84,25 +98,15 @@ function assertSdkCallMatches(
   }
 }
 
-function mockOk(body: unknown = { data: {}, meta: {} }): typeof globalThis.fetch {
-  return vi.fn().mockResolvedValue({
-    ok: true,
-    status: 200,
-    statusText: "OK",
-    headers: new Headers(),
-    json: () => Promise.resolve(body),
-    text: () => Promise.resolve(JSON.stringify(body)),
-  } satisfies Partial<Response> as Response);
-}
+// `mockOk` + `client` wrap the shared helpers with contract-test defaults:
+// `status: 200` for `mockOk`, and the identifier `client` kept short since
+// every test in this file uses it once. Keeps the test bodies readable
+// without forking the underlying fixture — if a fixture ever changes
+// (e.g., the default baseUrl or apiKey), both test files move together.
+const mockOk = (body: unknown = { data: {}, meta: {} }): typeof globalThis.fetch =>
+  mockFetch({ status: 200, body });
 
-function client(fetchFn: typeof globalThis.fetch): QURLClient {
-  return new QURLClient({
-    apiKey: "lv_live_test",
-    baseUrl: "https://api.test.layerv.ai",
-    fetch: fetchFn,
-    maxRetries: 0,
-  });
-}
+const client = createClient;
 
 describe("OpenAPI contract", () => {
   it("snapshot parses and has paths", () => {
@@ -144,16 +148,45 @@ describe("OpenAPI contract", () => {
 
   it("listAll → GET /v1/qurls (via wrapped list)", async () => {
     const fetch = mockOk({ data: [], meta: { has_more: false } });
-    // Drive the generator to completion so `list` actually gets called.
-    // Using iterator-protocol `.next()` instead of `for…of` avoids
-    // declaring an unused loop binding that trips eslint.
+    // Drive the generator to completion so the underlying list() fires.
     const iter = client(fetch).listAll();
     while (!(await iter.next()).done) {
-      /* no-op — iteration itself is the contract being exercised. */
+      /* no-op */
     }
     assertSdkCallMatches(fetch, "GET", "/v1/qurls");
   });
 
+  it("listAll multi-page → every page hits GET /v1/qurls", async () => {
+    // Two-page mock so the test covers the pagination path — a single-page
+    // exit would not exercise that every subsequent fetch also targets the
+    // same endpoint. Custom fetch instead of mockOk so call N can return
+    // a different body than call 0.
+    const fetch = vi
+      .fn()
+      .mockImplementationOnce(async () => ({
+        ok: true,
+        status: 200,
+        statusText: "OK",
+        headers: new Headers(),
+        json: async () => ({ data: [], meta: { has_more: true, next_cursor: "c2" } }),
+        text: async () => "",
+      }))
+      .mockImplementationOnce(async () => ({
+        ok: true,
+        status: 200,
+        statusText: "OK",
+        headers: new Headers(),
+        json: async () => ({ data: [], meta: { has_more: false } }),
+        text: async () => "",
+      })) as unknown as typeof globalThis.fetch;
+    const iter = client(fetch).listAll();
+    while (!(await iter.next()).done) {
+      /* no-op */
+    }
+    assertSdkCallMatches(fetch, "GET", "/v1/qurls", 0);
+    assertSdkCallMatches(fetch, "GET", "/v1/qurls", 1);
+  });
+
   it("update → PATCH /v1/qurls/{id}", async () => {
     const fetch = mockOk({ data: { resource_id: "r_x" } });
     await client(fetch).update("r_x", { expires_in: "24h" });

src/test-helpers.ts

@@ -0,0 +1,32 @@
+import { vi } from "vitest";
+import { QURLClient } from "./client.js";
+
+// Shared test fixtures used by client.test.ts and contract.test.ts.
+// Keep this file test-only — tsconfig excludes *.test.ts but NOT
+// test-helpers.ts, so it would otherwise land in dist/. Filename +
+// tsconfig exclude in lockstep: if you rename this, update the
+// exclude pattern.
+
+export function mockFetch(response: {
+  status: number;
+  body?: unknown;
+  headers?: Record<string, string>;
+}): typeof globalThis.fetch {
+  return vi.fn().mockResolvedValue({
+    ok: response.status >= 200 && response.status < 300,
+    status: response.status,
+    statusText: response.status === 200 ? "OK" : "Error",
+    headers: new Headers(response.headers ?? {}),
+    json: () => Promise.resolve(response.body),
+    text: () => Promise.resolve(JSON.stringify(response.body)),
+  } satisfies Partial<Response> as Response);
+}
+
+export function createClient(fetchFn: typeof globalThis.fetch): QURLClient {
+  return new QURLClient({
+    apiKey: "lv_live_test",
+    baseUrl: "https://api.test.layerv.ai",
+    fetch: fetchFn,
+    maxRetries: 0,
+  });
+}

tsconfig.json

@@ -16,5 +16,5 @@
     "forceConsistentCasingInFileNames": true
   },
   "include": ["src/**/*.ts"],
-  "exclude": ["node_modules", "dist", "**/*.test.ts"]
+  "exclude": ["node_modules", "dist", "**/*.test.ts", "src/test-helpers.ts"]
 }