web: de-flake fetchTracking redaction test (#1757)

[cliffhall]

Closes #1757

Problem

The secret-redaction tests in fetchTracking.test.ts prove a secret doesn't leak by asserting the serialized tracked entry doesn't contain it, e.g. expect(JSON.stringify(tracked[0])).not.toContain("shh"). Redaction works correctly, but each entry carries a random request id minted as ${timestamp}-${Math.random().toString(36).slice(2,11)}. That base36 tail can coincidentally contain a short secret substring — the id …sw7kshhf0 contains shh — so the whole-object substring check fails spuriously (and passes on re-run). Observed once on PR #1756's CI.

Fix

Add a serializedWithoutId(entry) helper that serializes the entry with id overwritten to undefined (JSON.stringify drops undefined-valued keys — no unused-var lint issue), and route all nine whole-entry .not.toContain(secret) leak checks through it.

The id is generated independently of any secret and can never legitimately carry one, so excluding it keeps every deterministic field (headers, url, request/response bodies) under the leak check while removing the only nondeterministic field. The stricter per-field redaction assertions (recorded.get("client_secret") === REDACTED, etc.) are unchanged. No production change.

Verification

Full npm run ci green (validate, ≥90 coverage gate, smoke, 456 Storybook tests). The fix is structural — the leak assertions no longer depend on the random id, so the flake can't recur.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 57s —— View job

---

Review: de-flake fetchTracking redaction test

Verified the fix against the source. This is a correct, well-scoped fix — nothing blocking.

• Read the diff and surrounding test file
• Verify the id generation claim against fetchTracking.ts
• Assess the fix approach and completeness (all 9 leak checks)
• Post review feedback

The root cause claim checks out

core/mcp/fetchTracking.ts:251 mints the id exactly as described:

const id = `${timestamp.getTime()}-${Math.random().toString(36).slice(2, 11)}`;

That base36 tail is up to 9 chars of [0-9a-z], so a short lowercase-alpha secret substring like "shh" really can appear there by chance, and a whole-object JSON.stringify(entry).not.toContain("shh") would then fail spuriously. The id is derived solely from the clock and Math.random() — it's the only field in FetchRequestEntryBase that carries nondeterministic alphabetic content, and it's provably independent of any request/response data. Excluding it from the leak check is sound.

The fix is correct and complete

• serializedWithoutId via { ...entry, id: undefined } is the right idiom — JSON.stringify omits undefined-valued keys, so no leftover "id" key and no unused-var lint issue. Cleaner than destructuring-and-discarding.
• All nine whole-entry .not.toContain(...) checks are routed through it; I don't see a stray JSON.stringify(tracked[0]) leak assertion left behind.
• Every other serialized field (url, requestHeaders, requestBody, responseHeaders, responseBody, error) stays under the check, and the stricter per-field assertions (recorded.get("client_secret") === REDACTED, lowerKeys(...)["authorization"], etc.) are untouched — so redaction coverage is not weakened.
• Switching tracked[0] → tracked[0]! matches the non-null-assertion style already used throughout the file.
• The comment block is excellent — it captures the why (the …sw7kshhf0 example) so a future reader won't "simplify" it back to the flaky form. No production code changed.

Minor, non-blocking observations

1. The id is the only volatile field, but worth stating why the others are safe. timestamp serializes to an ISO string (digits + T/Z/:/- only) and duration is a number, so neither can collide with an alpha secret. In practice only short lowercase secrets like "shh" were ever at risk — the longer fixtures ("live-access-token", "inspector-backend-token", "secret-auth-code", "pkce123", "sekret", "issued-secret", "leaked-on-error") contain hyphens or exceed 9 chars and can't be produced by the base36 tail anyway. Routing all nine through the helper uniformly is still the right call (robust and consistent), just noting the actual blast radius was narrow.

2. Alternative worth a mention (not a request): injecting a deterministic id — e.g. having the tracker accept an id factory, or stubbing Math.random in the test — would let the leak check keep asserting over the full serialized entry including id. The current approach trades that tiny bit of coverage on id for zero production surface change, which is the right trade-off for a test-only de-flake. Leaving it as-is is fine.

Verdict

LGTM. The flake is eliminated structurally (the leak assertions no longer depend on the random id), redaction coverage is preserved, and the change is test-only. Ship it.
· branch v2/1757-flaky-fetchtracking