fix(router): correct text extraction and empty/regex handling in fixture matching#301
Merged
Merged
Conversation
commit: |
…parate user messages
Some SDKs (e.g. Microsoft Agent Framework's agent_framework_openai image
path) serialize a single multimodal user turn (prompt text + attachment)
into TWO consecutive user messages — a text-only one followed by an
attachment-only one (content: [{type: 'image_url', ...}]). The trailing
attachment-only message has no extractable text, so the naive last-user-
message lookup returned null and — since no fixture can key on empty text —
every such multimodal fixture missed (404/503).
getLastUserText now skips trailing user messages that carry no text and
uses the nearest preceding user message that does. This is deliberately
narrow: it only skips text-LESS user messages, so a genuine multi-message
user turn (each message has text) is unaffected and still matched on its
final message. Adds two router tests (image split + a text-bearing trailing
message stays the match target).
jpr5
force-pushed
the
fix/multimodal-split-user-message-matching
branch
from
July 16, 2026 05:24
69fab57 to
ca55f47
Compare
The userMessage and inputText matcher branches discarded the
null-vs-"" distinction at two layers, so a fixture keyed on an empty
body could never fire even though such fixtures load fine:
- getLastUserText skipped a user message whose text extracted to ""
(the `&& text !== ""` clause), conflating a present-but-empty text
turn with a text-less (attachment-only) split turn. It now skips
only the genuinely text-LESS (`null`) case — matching what its own
doc-comment already claimed — so `content: ""` is returned as "".
- The userMessage guard `if (!text) continue;` became
`if (text === null) continue;` so an empty-string body flows into
the exact / substring / regex match forms (all of which handle ""
correctly: exact "" === "", substring "".includes("") === true,
/^$/.test("") === true).
- The inputText guard `if (!embeddingInput) continue;` became
`if (embeddingInput === undefined) continue;` — embeddingInput is
`string | undefined`, undefined = absent, "" = a genuinely-empty
embedding request.
systemMessage is intentionally NOT changed. getSystemText returns ""
for BOTH "a present but empty system message" AND "no system message
at all", so it exposes no absent-vs-empty distinction at the request
level; allowing "" through would turn a `systemMessage: ""` / `/^$/`
fixture into a catch-all firing on every no-system-message request.
There is no shipped-fixture demand for it, so the falsy guard stays,
with a comment explaining the asymmetry and how to add it later.
Call sites checked (grep for the extractor guards):
- userMessage guard src/router.ts (matcher) — FIXED
- systemMessage guard src/router.ts (matcher) — UNCHANGED (see above)
- inputText guard src/router.ts (matcher) — FIXED
- model branch reads effective.model (required, defaulted ""),
no null-vs-"" semantics — untouched
- getLastUserText only caller is the userMessage branch
Fixtures audit (fixtures/ docs/fixtures/ examples/) finds zero
empty-string userMessage/systemMessage/inputText matchers, so no
shipped fixture changes behavior.
The four RegExp matcher branches each did `match.<field>.lastIndex = 0; match.<field>.test(text)` in place. For a `/g` or `/y` regex, `RegExp.prototype.test` advances `lastIndex` as a side effect, so a successful match left the caller's own RegExp object with a non-zero `lastIndex` — clobbering a positional scan the caller was running with the same object, and making a `/g` regex reused across fixtures / successive matchFixture calls match intermittently. Introduce a `regexTest(re, text)` helper that tests a fresh `new RegExp(re.source, re.flags)` clone: the caller's object is never touched, every test starts at index 0, and all flag semantics (i/m/s/u and the harmless-for-a-single-test g/y) are preserved. Four sites replaced (reset-line dropped, .test → regexTest): - userMessage src/router.ts - systemMessage src/router.ts (sm narrowed to RegExp in the else branch) - inputText src/router.ts - model src/router.ts Call-site check: `grep lastIndex src/router.ts` now shows only the helper's doc-comment reference — no code sets or reads `match.<field>.lastIndex` after matching, so the clone is the sole mediator and no other path depended on the in-place reset.
jpr5
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three related fixes to how the fixture matcher reads and compares message text. This started as ran's fix for split multimodal turns; while we were in the same predicate branches we cleaned up two adjacent bugs, so the PR now covers text extraction and empty/regex handling in fixture matching as one unit.
What's fixed
1. Multimodal turns split into separate messages (ran's original fix)
When a provider serializes one multimodal user turn as a text message followed by an attachment-only message (observed with MS Agent Framework's
agent_framework_openaiimage path), the trailing text-less message shadowed the real prompt, so every such fixture missed.getLastUserTextnow walks back to the nearest user message that actually carries text.2. Fixtures keyed on empty content couldn't match
The predicate guards treated a present-but-empty body (
"") the same as an absent one, souserMessage: ""andinputText: ""could never fire. The guards now distinguish truly-absent (null/undefined→ skip) from present-but-empty (""→ allowed to match).systemMessageis deliberately left unchanged:getSystemTextcan't tell absent from empty at the request level, so allowing""there would turnsystemMessage: ""into a catch-all that fires on every no-system request. That reasoning is commented inline next to the guard.3. Matching a regex mutated the caller's RegExp
.test()advanceslastIndexon/gand/yregexes. Fixtures hold caller-owned RegExp objects, so testing them in place clobbered the caller's positional state, and a/gmatcher reused across match calls would match intermittently. AddedregexTest(), which tests a fresh clone (same source + flags) and never touches the caller's object.Testing
Red-green for each behavior change: the empty-content and
lastIndexcases fail on the pre-fix code and pass after. Full suite green (4400+ tests), lint / typecheck / build clean. A fixtures audit confirms no shipped fixture relied on the old empty-matcher behavior, so there's no match-outcome regression.Merge note
No version bump in this PR — the release bump is handled separately.
Deferred (pre-existing, out of scope)
Reviewers noted a
byUniquePositiondiagnostic-label quirk inselectByTurnIndexand a few untested pre-existing branches (Tier-4 turn-index path, non-image endpoint-compat,getTextContentnon-string filtering). All are pre-existing and the served-fixture behavior is correct; tracked for a separate follow-up.