feat: add requestTransform for deterministic matching and recording#63
feat: add requestTransform for deterministic matching and recording#63iskhakovt wants to merge 2 commits intoCopilotKit:mainfrom
Conversation
bac282e to
0207739
Compare
…ording Optional requestTransform on MockServerOptions normalizes requests before fixture matching. When set, string comparisons use exact equality (===) instead of includes() for deterministic recorded-fixture replay. - matchFixture gets optional 4th parameter, threaded from all handlers - Recorder applies transform before building fixture match keys - 8 new tests cover transform behavior, backward compat, and predicate passthrough Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
0207739 to
958add3
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9694329 to
75d0dfa
Compare
commit: |
jpr5
left a comment
There was a problem hiding this comment.
Code Review — 7-Agent Standard CR
335 lines across 17 files reviewed by 7 specialized agents.
The feature design is sound and well-motivated — requestTransform solves a real problem with dynamic data in recorded fixtures. The implementation is mechanically correct across all 17 files, with consistent threading of the new parameter through every handler. Three items need addressing before merge.
Bugs
1. Docs claim "RegExp and predicate matching are unaffected" — RegExp IS affected
docs/record-replay.html states:
RegExp and predicate matching are unaffected
But the code in router.ts applies effectiveReq (the transformed request) to all matching criteria including regex — only predicates receive the original req. Your own test at line 125 ("regexp does not match when transform changes the text") proves this by asserting that regex fails when the transform changes the content.
Fix: Change to something like:
Only predicate matching is unaffected — predicates always receive the original (untransformed) request. All other match criteria (including RegExp) operate on the transformed request.
Also update the JSDoc on requestTransform in types.ts — it mentions matching but omits the effect on recording and the includes() → === behavioral switch.
2. Transform that throws crashes the handler with opaque "Internal error"
requestTransform is user-supplied code called unprotected in matchFixture():
const effectiveReq = requestTransform ? requestTransform(req) : req;If the transform throws (TypeError, user logic error, etc.), the exception propagates up through every handler into the HTTP response path. Users get a generic 500 "Internal error" with zero indication their transform was the problem.
In recorder.ts, it's worse — the upstream response has already been fetched but is lost when the transform throws during fixture-key building.
In WebSocket handlers, the matchFixture call is outside the JSON parse try/catch, so a throwing transform crashes the message handler.
Fix: Wrap the transform invocation in try/catch. Log the actual error with context identifying the transform as the source. Return null from matchFixture (no match) on failure, or create a small wrapper:
let effectiveReq: ChatCompletionRequest;
try {
effectiveReq = requestTransform ? requestTransform(req) : req;
} catch (err) {
// need logger access — either pass it in or wrap at call sites
return null;
}Since matchFixture doesn't have logger access, the wrapping may need to happen at call sites. Either approach works.
3. Transform that drops messages causes TypeError
If a user's transform returns an object without messages (e.g. { model: req.model }), getLastMessageByRole(effectiveReq.messages, "user") throws TypeError: Cannot read properties of undefined (reading 'length'). TypeScript interfaces provide no runtime enforcement.
Fix: Either validate the transform output at the top of matchFixture, or add a guard before getLastMessageByRole:
if (match.userMessage !== undefined) {
if (!effectiveReq.messages?.length) continue;
const msg = getLastMessageByRole(effectiveReq.messages, "user");
// ...
}Missing Test
4. No test for throwing transform
User-supplied code in a hot path with no error handling deserves a test that documents the contract — whether matchFixture propagates the exception or handles it gracefully. Either behavior is fine, but it should be tested and intentional, not accidental.
Design Discussion (non-blocking)
5. Identity transform silently changes matching semantics
The mere presence of ANY transform (even (r) => r) switches string matching from includes() to ===. A user adding a no-op transform expecting no behavioral change will find previously-matching fixtures stop matching. Worth considering whether exact-match should be a separate exactMatch?: boolean option rather than coupled to transform presence.
6. Transform can mutate original request
If a user writes req.messages = req.messages.filter(...) (mutating in place), effectiveReq === req and the "predicates receive original" contract is silently violated. The docs example uses spread correctly, but nothing enforces immutability. Consider documenting this prominently or using structuredClone(req) before passing to the transform.
7. Type duplication across 8+ locations
The 3 WebSocket handler files inline their own defaults type (6 copies) instead of referencing HandlerDefaults. recorder.ts also has its own inline type. Consider extracting:
export type WebSocketHandlerDefaults = HandlerDefaults & { model: string };
export type RequestTransform = (req: ChatCompletionRequest) => ChatCompletionRequest;This is a pre-existing pattern the PR inherits, not something it introduced — but since you're touching all these signatures anyway, it's a good time to clean it up.
What's Good
- Clean, consistent threading of the new parameter through all 15
matchFixturecall sites - Predicate isolation (receiving original
req) is a well-thought-out design choice - Dual application in recorder (normalize both at match time and record time) ensures round-trip consistency
- 8 router tests cover the core behavioral contract well: exact match, regex, embedding, backward compat, predicate isolation
- Documentation section with realistic code example
Problem
LLM prompts often contain dynamic data (timestamps, UUIDs, session IDs)
injected by upstream services. When recording fixtures, the match key
includes this dynamic data. On replay, the same prompt with different
timestamps doesn't match the stored fixture.
Example: Hindsight memory server injects
Event Date: 2026-03-30T13:19:38into extraction prompts. Each test run has a different timestamp, so
recorded fixtures never match on replay.
Solution
Add
requestTransformtoMockServerOptions— a function that normalizesrequests before both matching and recording:
Recording: saves the transformed match key — no timestamps in fixture.
Matching: transforms incoming request before comparison — same clean key.
When
requestTransformis set, string matching switches fromincludes(substring) to
===(exact equality). This prevents false positive matchesfrom shortened keys accidentally matching unrelated prompts. Without a
transform, existing
includesbehavior is preserved (backward compatible).Follows the Polly.js pattern of
composable request normalizers for deterministic snapshot matching.
Changes
types.ts: AddrequestTransformtoMockServerOptionsandHandlerDefaultsrouter.ts: Optional 4th param onmatchFixture, exact match when transform setserver.ts: Thread transform into defaultsrecorder.ts: Apply transform before match key extractiondefaults.requestTransformas 4th arg tomatchFixturedocs/record-replay.html: DocumentrequestTransformfeature