fix: address review — request_id on shape-guard, target_url dedup, retry/index hardening

6 items.

### Load-bearing

1. **`request_id` propagated to shape-guard ValidationError.**
   Operators debugging "unexpected response" tickets need the
   server-side correlation ID on the *error* path too, not just
   the success/passthrough returns. Added an optional
   `request_id` parameter to `unexpectedResponseError` and
   threaded `envelope.meta?.request_id` through every
   shape-guard branch in `validateBatchCreateResponse`. New
   regression test asserts the requestId surfaces on
   ValidationError.

### Correctness

2. **De-duplicated target_url validation.**
   `requireValidTargetUrl` and `requireMaxLength(target_url, ...)`
   both ran under `collect()`, both ran their own typeof guard,
   and a non-string target_url produced two "must be a string"
   messages in the aggregated detail. Inlined the length check
   into `requireValidTargetUrl` (priority order: type → scheme
   → length, fail-fast within the field). The collect-all loop
   in validateCreateInput is unchanged at the field level.

3. **`batchItemResultValidationReason` index check tightened.**
   Was `typeof e.index !== "number"`, which let NaN, Infinity,
   negatives, and floats through. `Number.isInteger(...) && ... >= 0`
   matches the same posture used by the count integer guards on
   the surrounding shape guard.

4. **`parseRetryAfter` rejects trailing garbage.**
   `parseInt("60abc", 10)` returns 60 — the SDK would have
   silently honored a malformed Retry-After header. Added a
   digit-only regex pre-check so any deviation surfaces via the
   existing debug log instead. New test asserts a 429 with a
   `60abc` header produces an error with `.retryAfter ===
   undefined` and the debug log fires.

### JSDoc / clarity

5. **`mapQurlsField`: explained why `"qurls" in raw` (not
   truthy)** — empty array `qurls: []` is truthy as a value
   but the rename must still happen so consumers see
   `access_tokens: []`. Locked in by the existing test.

6. **`MintInput`: explained why mutual-exclusion stays
   runtime-only** — `ExtendInput` requires *exactly one* of two
   fields (clean `T1 | T2`); `MintInput` allows *at most one*
   (zero is valid; server applies 24h default), which would need
   a noisier three-arm union. Runtime check catches "both
   provided" at minimal cost.

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

Author: justin-layerv

src/client.test.ts

@@ -2020,6 +2020,56 @@ describe("QURLClient", () => {
     }
   });
 
+  it("rejects Retry-After with trailing garbage (e.g. '60abc') as non-numeric", async () => {
+    // `parseInt("60abc", 10)` returns 60 — a malformed header would
+    // silently get honored without the digit-only pre-check. The strict
+    // regex makes the malformed-header path observable (debug log) and
+    // falls through to exponential backoff so the SDK doesn't act on
+    // a half-parsed value.
+    const fetch = mockFetch({
+      status: 200,
+      body: {
+        data: { plan: "growth", period_start: "2026-03-01", period_end: "2026-04-01" },
+      },
+      headers: { "Retry-After": "60abc" },
+    });
+    let lastDebug: string | undefined;
+    const client = new QURLClient({
+      apiKey: "lv_live_test",
+      baseUrl: "https://api.test.layerv.ai",
+      fetch,
+      maxRetries: 0,
+      debug: (msg) => {
+        lastDebug = msg;
+      },
+    });
+    // The 200 success path doesn't actually call parseRetryAfter
+    // (only 429 does). Use the QURLError construction path: a 429
+    // with malformed header surfaces .retryAfter === undefined.
+    const fetch429 = mockFetch({
+      status: 429,
+      body: { error: { title: "Rate Limited", status: 429, detail: "x", code: "rate_limited" } },
+      headers: { "Retry-After": "60abc" },
+    });
+    const client429 = new QURLClient({
+      apiKey: "lv_live_test",
+      baseUrl: "https://api.test.layerv.ai",
+      fetch: fetch429,
+      maxRetries: 0,
+      debug: (msg) => {
+        lastDebug = msg;
+      },
+    });
+    const err = (await client429.getQuota().catch((e: unknown) => e)) as QURLError;
+    expect(err).toBeInstanceOf(RateLimitError);
+    // Malformed Retry-After must NOT propagate — retryAfter stays undefined.
+    expect(err.retryAfter).toBeUndefined();
+    // Debug log fires so operators can spot the drift.
+    expect(lastDebug).toContain("non-numeric");
+    // Sanity-check the 200-path client wasn't broken by the Retry-After test.
+    await expect(client.getQuota()).resolves.toBeDefined();
+  });
+
   it("does not clamp Retry-After against RETRY_MAX_DELAY_MS (server directive wins)", async () => {
     // RFC 7231 §7.1.3 — when the server tells us 60s, retrying at 30s
     // (the exponential-backoff cap) just re-trips the rate limit.
@@ -4027,6 +4077,29 @@ describe("QURLClient", () => {
     expect(result.request_id).toBe("req_mixed207");
   });
 
+  it("batch create propagates request_id to shape-guard ValidationError", async () => {
+    // Operators debugging "unexpected response" tickets need the
+    // server-side correlation ID on the *error* path too — not just
+    // on the success/passthrough returns. Locks in that
+    // unexpectedResponseError carries `meta.request_id` through to
+    // QURLError.requestId.
+    const fetch = mockFetch({
+      status: 400,
+      body: {
+        data: { unexpected: "not a batch response" },
+        meta: { request_id: "req_shape_guard_correlation" },
+      },
+    });
+
+    const client = createClient(fetch);
+    const error = await client
+      .batchCreate({ items: [{ target_url: "https://example.com" }] })
+      .catch((e: unknown) => e as ValidationError);
+    expect(error).toBeInstanceOf(ValidationError);
+    expect((error as ValidationError).code).toBe("unexpected_response");
+    expect((error as ValidationError).requestId).toBe("req_shape_guard_correlation");
+  });
+
   it("batch create surfaces ValidationError on unexpected 400 response shape", async () => {
     // If the API ever returns HTTP 400 with a non-BatchCreateOutput body
     // (e.g., a top-level malformed-request error), batchCreate should

src/client.ts

@@ -133,13 +133,19 @@ function clientValidationError(detail: string): ValidationError {
  * server returned a body I can't interpret" (`"unexpected_response"`).
  * Uses `status: 0` because the offending HTTP status (400/207/etc.) isn't
  * the thing being reported — the shape mismatch is.
+ *
+ * Threads `request_id` through to {@link QURLError.requestId} when the
+ * server-side correlation ID is available — operators debugging
+ * "unexpected response" tickets need it on the *error* path too, not
+ * just on success/passthrough returns.
  */
-function unexpectedResponseError(detail: string): ValidationError {
+function unexpectedResponseError(detail: string, request_id?: string): ValidationError {
   return new ValidationError({
     status: 0,
     code: "unexpected_response",
     title: "Unexpected Response",
     detail,
+    request_id,
   });
 }
 
@@ -250,14 +256,13 @@ function requireValidTags(tags: string[] | null | undefined): void {
 const ALLOWED_URL_SCHEMES = ["http://", "https://"] as const;
 
 function requireValidTargetUrl(target_url: unknown): void {
-  // Split the type guard from the scheme check so the error message
-  // pinpoints the actual failure mode. A non-string `target_url` (a
-  // number, null, plain object) gets a "must be a string" message
-  // matching the rest of the validators (requireMaxLength,
-  // requireValidTags); only an actual string value with a bad scheme
-  // gets the "must start with http:// or https://" message. Without
-  // the split, an untyped-JS caller passing `null` would get a
-  // misleading "scheme" complaint.
+  // Three checks in priority order: type guard, scheme, length.
+  // Stop at the first failure so a non-string target_url doesn't
+  // produce both "must be a string" AND "must be ≤ 2048 characters"
+  // (the length check on a non-string would also typeof-fail and
+  // duplicate the type message). The collect-all loop in
+  // validateCreateInput runs each FIELD's validator independently —
+  // within target_url itself we want fail-fast.
   if (typeof target_url !== "string") {
     throw clientValidationError(
       `target_url: must be a string (got ${target_url === null ? "null" : typeof target_url})`,
@@ -269,6 +274,11 @@ function requireValidTargetUrl(target_url: unknown): void {
     const repr = JSON.stringify(target_url).slice(0, 40);
     throw clientValidationError(`target_url: must start with http:// or https:// (got ${repr})`);
   }
+  if (target_url.length > MAX_TARGET_URL) {
+    throw clientValidationError(
+      `target_url: must be ${MAX_TARGET_URL} characters or fewer (got ${target_url.length})`,
+    );
+  }
 }
 
 function validateCreateInput(input: CreateInput): void {
@@ -290,7 +300,6 @@ function validateCreateInput(input: CreateInput): void {
     }
   };
   collect(() => requireValidTargetUrl(input.target_url));
-  collect(() => requireMaxLength(input.target_url, "target_url", MAX_TARGET_URL));
   collect(() => requireMaxLength(input.label, "label", MAX_LABEL));
   collect(() => requireMaxLength(input.custom_domain, "custom_domain", MAX_CUSTOM_DOMAIN));
   collect(() => requireMaxSessionsInRange(input.max_sessions));
@@ -335,7 +344,14 @@ function validateCreateInput(input: CreateInput): void {
 function batchItemResultValidationReason(entry: unknown): string | null {
   if (!entry || typeof entry !== "object") return "not an object";
   const e = entry as Record<string, unknown>;
-  if (typeof e.index !== "number") return "missing/invalid 'index' (expected number)";
+  // `Number.isInteger` rejects NaN, Infinity, and floats; combined
+  // with `>= 0`, the check rules out pathological proxy bodies that
+  // would attribute to a useless index. `index` is informational, so
+  // this isn't a correctness hazard, but tightening the constraint
+  // matches the same posture as the count integer guards above.
+  if (!Number.isInteger(e.index) || (e.index as number) < 0) {
+    return "missing/invalid 'index' (expected non-negative integer)";
+  }
   if (e.success === true) {
     if (typeof e.resource_id !== "string") {
       return "success-branch missing required field 'resource_id' (expected string)";
@@ -506,6 +522,16 @@ 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`
+   * 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
+   * `"preserves empty qurls: [] as access_tokens: []"`.
    */
   private mapQurlsField(raw: QURL & { qurls?: AccessToken[] }): QURL {
     if (!("qurls" in raw)) return raw;
@@ -554,6 +580,13 @@ export class QURLClient {
     const result = envelope.data;
     const statusSuffix =
       envelope.http_status !== undefined ? ` (HTTP ${envelope.http_status})` : "";
+    // Thread the server's request_id into the shape-guard error so
+    // operators debugging "unexpected response" tickets have the
+    // 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.
+    const requestId = envelope.meta?.request_id;
 
     // `Number.isInteger` rejects NaN, Infinity, and floats. Combined
     // with `>= 0`, this rules out pathological proxy bodies (e.g.
@@ -570,6 +603,7 @@ export class QURLClient {
     ) {
       throw unexpectedResponseError(
         `Unexpected response shape from POST /v1/qurls/batch${statusSuffix}`,
+        requestId,
       );
     }
 
@@ -578,6 +612,7 @@ export class QURLClient {
         `Unexpected response shape from POST /v1/qurls/batch${statusSuffix}: ` +
           `counts/results length mismatch (succeeded=${result.succeeded}, ` +
           `failed=${result.failed}, results.length=${result.results.length})`,
+        requestId,
       );
     }
 
@@ -588,6 +623,7 @@ export class QURLClient {
       if (reason !== null) {
         throw unexpectedResponseError(
           `Unexpected response shape from POST /v1/qurls/batch${statusSuffix}: results[${i}] ${reason}`,
+          requestId,
         );
       }
     }
@@ -1206,17 +1242,18 @@ export class QURLClient {
     // TODO: parse HTTP-date format per RFC 7231 §7.1.3 — currently only
     // handles delta-seconds; HTTP-date headers fall through to the
     // exponential-backoff branch in `retryDelay`. Tracked in #61.
-    const seconds = parseInt(header, 10);
-    if (Number.isNaN(seconds)) {
-      // Surface non-numeric Retry-After values via debug so operators
-      // see drift if the upstream ever switches formats — without this,
-      // the silent fall-through to exponential backoff is invisible.
+    //
+    // Digit-only pre-check rather than `parseInt` alone: `parseInt("60abc")`
+    // returns `60` and would silently honor a malformed header. The
+    // strict check makes any deviation observable (debug log) instead.
+    const trimmed = header.trim();
+    if (!/^\d+$/.test(trimmed)) {
       this.log("Retry-After header was non-numeric, using exponential backoff", {
         header_value: header.slice(0, 100),
       });
       return undefined;
     }
-    return seconds;
+    return parseInt(trimmed, 10);
   }
 
   /**

src/types.ts

@@ -210,6 +210,15 @@ export interface UpdateInput {
  *
  * `expires_in` and `expires_at` are mutually exclusive — provide at most one.
  * If neither is specified, the link defaults to 24 hours from now.
+ *
+ * **Why this isn't a {@link ExtendInput}-style discriminated union:**
+ * `ExtendInput` requires *exactly one* of the two fields, which fits
+ * `T1 | T2` cleanly. `MintInput` allows *at most one* (zero is valid —
+ * the server applies the 24h default), which would need a three-arm
+ * union (`{ expires_in } | { expires_at } | { neither }`). The runtime
+ * check in {@link QURLClient.mintLink} catches the "both provided"
+ * case at minimal cost; the type-system gain isn't worth the noisier
+ * type for consumers who just want to omit both and accept the default.
  */
 export interface MintInput {
   /** Relative duration until expiration (e.g., `"5m"`, `"24h"`, `"7d"`). Mutually exclusive with `expires_at`. */