fix: address review — list per-key types, batch sep, hasOwnProperty, request_id in msg

8 items.

### Concerns

1. **Two-level `;` separator in batchCreate aggregated errors.** Inner
   per-field errors join with `; `, outer per-item errors used `; `
   too — the same separator at two levels was ambiguous. Outer
   separator is now ` | ` so callers can split reliably:
   `items[0]: a; b | items[2]: c; d`. JSDoc updated.

2. **`list()` per-key type validation.** Was permissive (any
   string-or-number accepted for any key). Per the OpenAPI spec only
   `limit` is numeric; `cursor`/`status`/`q`/`sort`/dates are strings.
   Tightened to per-key `expectedType` so `cursor: 42` is rejected
   instead of silently coerced to `"42"`. Updated existing test;
   added new test for numeric values on string-typed fields.

3. **`mapQurlsField` `Object.prototype.hasOwnProperty.call`.** Replaced
   the bare `in` operator (walks prototype chain by spec) with
   `hasOwnProperty.call`. `response.json()` produces plain objects
   today, but defends against future consumers feeding pre-parsed
   bodies from arbitrary sources.

5. **`AbortSignal.timeout` per-attempt budget documented.** Added a
   block comment explaining the deliberate per-attempt-not-total
   semantics (so a slow-then-failed attempt doesn't poison the next
   retry's deadline) plus the worst-case total bound.

6. **`Quota.usage.active_qurls_percent` field-level JSDoc.** Documented
   the `null = unlimited plan` semantic on the field itself so IDE
   hover catches it without forcing readers back to the PR description.

### Nits

- **`request_id` in shape-guard error message string.** Embedded
  `[request_id=...]` in the detail message in addition to
  `.requestId` — a stack trace pasted into a support ticket now
  carries the correlation handle without a follow-up round-trip.
  Existing test extended to assert the new suffix.

### Coverage

- **`qurls: null` with `access_tokens` already present.** Exercises
  the `qurls && ...` truthy short-circuit branches in mapQurlsField.
  Defends against a future refactor that flips presence-vs-truthy
  and surfaces `qurls: null` on a typed QURL.

### No action

- **#4 `requireString` helper extraction.** Reviewer marked optional
  ("tiny refactor"). Skipping — the duplicated typeof guard appears
  in three small spots and pulling it out adds an indirection layer.
- **HTTP-date `Retry-After` support.** Already filed as #61.
- **`extend()` narrower-than-`update()` type relationship.** Reviewer
  noted as informational. Already documented in extend()'s JSDoc.

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

Author: justin-layerv

src/client.test.ts

@@ -314,6 +314,48 @@ describe("QURLClient", () => {
     expect(result.access_tokens).toBeUndefined();
   });
 
+  it("handles qurls: null with access_tokens already present (truthy short-circuit)", async () => {
+    // Exercises the `qurls && ...` branches in mapQurlsField. With
+    // qurls being null (not absent), the rename branch is entered
+    // (presence check passes), but the truthy short-circuit means
+    // we keep access_tokens as-is and drop the wire-format key.
+    // Defends against a future refactor that flips presence-vs-truthy
+    // and accidentally surfaces qurls: null on a typed QURL.
+    const accessTokens = [
+      {
+        qurl_id: "at_existing",
+        status: "active",
+        one_time_use: false,
+        max_sessions: 5,
+        session_duration: 3600,
+        use_count: 0,
+        created_at: "2026-03-10T10:00:00Z",
+        expires_at: "2026-04-10T10:00:00Z",
+      },
+    ];
+    const fetch = mockFetch({
+      status: 200,
+      body: {
+        data: {
+          resource_id: "r_dual",
+          target_url: "https://example.com",
+          status: "active",
+          qurl_count: 1,
+          created_at: "2026-03-10T10:00:00Z",
+          qurls: null,
+          access_tokens: accessTokens,
+        },
+      },
+    });
+
+    const client = createClient(fetch);
+    const result = await client.get("r_dual");
+
+    expect(result.access_tokens).toEqual(accessTokens);
+    // Wire-format key stripped from consumer-facing object.
+    expect((result as unknown as { qurls?: unknown }).qurls).toBeUndefined();
+  });
+
   it("preserves empty qurls: [] as access_tokens: []", async () => {
     // Defense against a future refactor flipping the truthiness check
     // in mapQurlsField from `"qurls" in raw` to `if (raw.qurls)`. An
@@ -3324,10 +3366,10 @@ describe("QURLClient", () => {
     });
     const client = createClient(fetch);
 
-    for (const [key, value] of [
-      ["q", ["foo", "bar"]],
-      ["sort", { fieldName: "created_at" }],
-      ["cursor", true],
+    for (const [key, value, expectedType] of [
+      ["q", ["foo", "bar"], "string"],
+      ["sort", { fieldName: "created_at" }, "string"],
+      ["cursor", true, "string"],
     ] as const) {
       const error = await client
         .list({ [key]: value } as unknown as Parameters<typeof client.list>[0])
@@ -3336,7 +3378,34 @@ describe("QURLClient", () => {
       expect((error as ValidationError).code).toBe("client_validation");
       const detail = (error as ValidationError).detail;
       expect(detail).toContain(key);
-      expect(detail).toContain("must be a string or number");
+      // Per-key type enforcement: cursor / q / sort are strings,
+      // not numbers — `cursor: 42` would silently coerce without
+      // this guard.
+      expect(detail).toContain(`must be a ${expectedType}`);
+    }
+    expect(fetch).not.toHaveBeenCalled();
+  });
+
+  it("list rejects numeric values for string-typed filter fields (e.g. cursor: 42)", async () => {
+    // Per-key type enforcement: only `limit` is numeric per the
+    // OpenAPI spec. `cursor: 42`, `status: 1`, etc. would silently
+    // coerce to "42" / "1" through the previous broad
+    // string-or-number guard. Now they fail fast with a per-key
+    // ValidationError pointing at the expected type.
+    const fetch = mockFetch({
+      status: 200,
+      body: { data: [], meta: { has_more: false } },
+    });
+    const client = createClient(fetch);
+
+    for (const key of ["cursor", "status", "q", "sort"] as const) {
+      const error = await client
+        .list({ [key]: 42 } as unknown as Parameters<typeof client.list>[0])
+        .catch((e: unknown) => e as ValidationError);
+      expect(error).toBeInstanceOf(ValidationError);
+      const detail = (error as ValidationError).detail;
+      expect(detail).toContain(key);
+      expect(detail).toContain("must be a string");
     }
     expect(fetch).not.toHaveBeenCalled();
   });
@@ -4098,6 +4167,10 @@ describe("QURLClient", () => {
     expect(error).toBeInstanceOf(ValidationError);
     expect((error as ValidationError).code).toBe("unexpected_response");
     expect((error as ValidationError).requestId).toBe("req_shape_guard_correlation");
+    // request_id also lands in the message string itself so a stack
+    // trace pasted into a support ticket carries the correlation
+    // handle without a follow-up round-trip.
+    expect((error as ValidationError).detail).toContain("[request_id=req_shape_guard_correlation]");
   });
 
   it("batch create surfaces ValidationError on unexpected 400 response shape", async () => {

src/client.ts

@@ -523,18 +523,21 @@ export class QURLClient {
    * Maps the API's "qurls" field to "access_tokens" on the SDK type.
    * Uses destructuring to avoid mutation and unsafe casts.
    *
-   * **Why `"qurls" in raw` (not `if (raw.qurls)`):** an empty array
-   * `qurls: []` is truthy as a value but the rename must still happen
-   * — consumers expect `access_tokens: []` (empty list of tokens), not
-   * `qurls: []` slipping through. The `in` operator triggers on
-   * presence regardless of value, so empty / null / undefined `qurls`
+   * **Why `hasOwnProperty.call` (not `if (raw.qurls)`):** an empty
+   * array `qurls: []` is truthy as a value but the rename must still
+   * happen — consumers expect `access_tokens: []` (empty list of
+   * tokens), not `qurls: []` slipping through. The presence check
+   * triggers regardless of value, so empty / null / undefined `qurls`
    * all enter the rename branch and the wire-format key gets stripped.
-   * Walks the prototype chain by spec, but `response.json()` produces
-   * plain objects so this is safe in practice. Locked in by the test
+   * Belt-and-suspenders form: `Object.prototype.hasOwnProperty.call`
+   * confines the check to own-properties (the bare `in` operator walks
+   * the prototype chain). `response.json()` already produces plain
+   * objects, but if the SDK ever consumes pre-parsed bodies from
+   * arbitrary sources, this stays safe. Locked in by the test
    * `"preserves empty qurls: [] as access_tokens: []"`.
    */
   private mapQurlsField(raw: QURL & { qurls?: AccessToken[] }): QURL {
-    if (!("qurls" in raw)) return raw;
+    if (!Object.prototype.hasOwnProperty.call(raw, "qurls")) return raw;
     const { qurls, ...rest } = raw;
     if (qurls && !rest.access_tokens) {
       return { ...rest, access_tokens: qurls };
@@ -585,8 +588,12 @@ export class QURLClient {
     // correlation handle on the error path, mirroring the success-
     // path propagation in batchCreate(). The field is optional —
     // older API versions without `meta.request_id` simply produce
-    // an error without it, same as today.
+    // an error without it, same as today. Embedded in BOTH the
+    // .requestId property AND the message string so a stack trace
+    // pasted into a support ticket carries the correlation handle
+    // without a follow-up round-trip.
     const requestId = envelope.meta?.request_id;
+    const requestIdSuffix = requestId !== undefined ? ` [request_id=${requestId}]` : "";
 
     // `Number.isInteger` rejects NaN, Infinity, and floats. Combined
     // with `>= 0`, this rules out pathological proxy bodies (e.g.
@@ -602,7 +609,7 @@ export class QURLClient {
       !Array.isArray(result.results)
     ) {
       throw unexpectedResponseError(
-        `Unexpected response shape from POST /v1/qurls/batch${statusSuffix}`,
+        `Unexpected response shape from POST /v1/qurls/batch${statusSuffix}${requestIdSuffix}`,
         requestId,
       );
     }
@@ -611,7 +618,7 @@ export class QURLClient {
       throw unexpectedResponseError(
         `Unexpected response shape from POST /v1/qurls/batch${statusSuffix}: ` +
           `counts/results length mismatch (succeeded=${result.succeeded}, ` +
-          `failed=${result.failed}, results.length=${result.results.length})`,
+          `failed=${result.failed}, results.length=${result.results.length})${requestIdSuffix}`,
         requestId,
       );
     }
@@ -622,7 +629,7 @@ export class QURLClient {
       const reason = batchItemResultValidationReason(result.results[i]);
       if (reason !== null) {
         throw unexpectedResponseError(
-          `Unexpected response shape from POST /v1/qurls/batch${statusSuffix}: results[${i}] ${reason}`,
+          `Unexpected response shape from POST /v1/qurls/batch${statusSuffix}: results[${i}] ${reason}${requestIdSuffix}`,
           requestId,
         );
       }
@@ -659,8 +666,11 @@ export class QURLClient {
    * **Client-side validation:** All items are validated before any network
    * request is made (target_url scheme, label length, max_sessions range,
    * tag constraints). All failures are collected into a single
-   * `ValidationError` with per-index attribution (`"items[0]: ...; items[3]: ..."`),
-   * so callers see every problem in one throw instead of fix-re-run-repeat.
+   * `ValidationError` with per-index attribution
+   * (`"items[0]: ... | items[3]: ..."`) — items are separated by ` | ` and
+   * field-level errors within an item by `; ` so callers can split the
+   * detail message reliably. Every problem surfaces in one throw instead
+   * of fix-re-run-repeat.
    *
    * Throws `ValidationError` client-side (`status: 0`, `code: "client_validation"`)
    * when `items` is empty or exceeds 100, or when the HTTP 400 response body
@@ -722,7 +732,13 @@ export class QURLClient {
       }
     }
     if (perItemErrors.length > 0) {
-      throw clientValidationError(`batchCreate: ${perItemErrors.join("; ")}`);
+      // Outer separator differs from `validateCreateInput`'s inner
+      // `"; "` so callers can split the message reliably:
+      //   `items[0]: a; b | items[2]: c; d`
+      // Without this asymmetry, the same `; ` at both levels makes
+      // it ambiguous which separator is "next item" vs "next field
+      // within item". Mirrors the JSDoc example shape.
+      throw clientValidationError(`batchCreate: ${perItemErrors.join(" | ")}`);
     }
     // 400 carries per-item errors (see rawRequest JSDoc). Use rawRequest
     // directly (not `this.request`) so we can read `meta.request_id`
@@ -801,23 +817,23 @@ export class QURLClient {
     // typing can't prevent callers from spreading untyped objects with extra
     // properties, and String(value) on an unexpected array/object would emit
     // "[object Object]" as a query param.
+    // Per the OpenAPI spec, only `limit` is numeric; every other
+    // filter field is a string. Enforce per-key type expectations
+    // so untyped-JS callers passing `cursor: 42` (or worse,
+    // `q: ["foo", "bar"]`) get a structured ValidationError instead
+    // of silently coercing to `"42"` / `"foo,bar"`.
+    const NUMERIC_LIST_KEYS: ReadonlySet<keyof ListInput> = new Set(["limit"]);
     for (const key of LIST_PARAM_KEYS) {
       const value = input[key];
       // Drop null/undefined and empty strings — an empty string from
       // an untyped JS caller would otherwise produce `?status=&q=`
       // garbage that the API might interpret as an explicit empty
-      // filter. Non-zero numerics are preserved (serialize as their
-      // string form); `limit` is validated separately above.
+      // filter. `limit` is validated separately above.
       if (value === null || value === undefined || value === "") continue;
-      // Reject non-string/number values explicitly. `String(["a","b"])`
-      // produces `"a,b"` (which accidentally matches `status`'s CSV
-      // form but means nothing for `q` / `sort` / dates), and
-      // `String({})` produces `"[object Object]"` — both leak garbage
-      // through the type system from untyped-JS callers. Surface a
-      // structured ValidationError instead.
-      if (typeof value !== "string" && typeof value !== "number") {
+      const expectedType = NUMERIC_LIST_KEYS.has(key) ? "number" : "string";
+      if (typeof value !== expectedType) {
         throw clientValidationError(
-          `${key}: must be a string or number (got ${value === null ? "null" : typeof value})`,
+          `${key}: must be a ${expectedType} (got ${value === null ? "null" : typeof value})`,
         );
       }
       params.set(key, String(value));
@@ -1101,6 +1117,14 @@ export class QURLClient {
 
       let response: Response;
       try {
+        // `timeout` is intentionally a per-attempt budget, not a total-
+        // request budget. With maxRetries=3 and timeout=30s, a slow
+        // upstream + retries can run up to ~120s total. The per-
+        // attempt cap matches `fetch()` semantics elsewhere and lets
+        // each retry get a fresh window — a half-completed slow
+        // attempt shouldn't poison the next try's deadline. Total
+        // request bound: roughly `timeout * (maxRetries + 1) +
+        // sum(retryDelay)`.
         response = await this.fetchFn(url, {
           method,
           headers,

src/types.ts

@@ -295,6 +295,12 @@ export interface Quota {
   usage?: {
     qurls_created?: number;
     active_qurls?: number;
+    /**
+     * Active qURLs as a percentage of the plan's `max_active_qurls`,
+     * or `null` when the plan is unlimited (no denominator). Callers
+     * doing arithmetic must guard the null case — `usage.active_qurls_percent * 0.01`
+     * yields `NaN` if the plan is unlimited.
+     */
     active_qurls_percent?: number | null;
     total_accesses?: number;
   };