fix(anthropic): canonicalize system prompts with the shared stripper - #767
Conversation
The adapter carried its own unanchored `re.sub` for the billing header.
It runs before `canonicalize_system_messages()`, so whatever it removed
was already gone when the shared, line-anchored stripper saw the
messages -- and that pass cannot restore it.
Unanchored means it matched anywhere, including inside a user's own
sentence:
in: "Explain what x-anthropic-billing-header: means in HTTP terms."
out: "Explain what "
It was also case-sensitive where the shared stripper is not, so the two
stages disagreed in both directions on the same input.
Replacing it with `canonicalize_system_prompt()` leaves one definition of
the pattern, so the Anthropic path picks up any future stripper for free
-- the point of the registry added in waybarrios#528.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1c36039 to
77ae906
Compare
Thump604
left a comment
There was a problem hiding this comment.
The source change at 77ae9065 matches the narrow fix agreed in #524: reuse the shared line-anchored canonicalizer instead of deleting mid-sentence prompt text. I checked the real adapter, shared helper and server preparation call path; I found no further source defect in this change. The new tests exercise the real adapter and shared helper, without adding speculative strippers or a benchmark.
One CI wiring item before landing: tests/test_anthropic_adapter_canonicalization.py is absent from the workflow's explicit test lists. I checked run 33886240337, including the Linux 3.12 and Apple 3.11 logs: the existing adapter tests execute, but these 15 new cases do not. The green checks therefore do not cover them yet.
Please add that filename beside tests/test_anthropic_adapter.py in the existing no-MLX unit-test invocation. That is the only remaining request from me; no additional behavior, framework or model benchmark.
|
@janhilgard just added a missing test into the CI settings. Now it is ready to merge. |
|
Thanks both — and thanks @waybarrios for wiring it in directly rather than bouncing it back. I verified the wiring executes rather than assuming the green checks meant it, since that is precisely what the checks did not mean before with individual ids present, e.g. @Thump604 — you caught the same gap on #690, and both times it was a new test file that CI never saw. That is a repeatable footgun rather than two coincidences: a green PR proves nothing about a file nobody listed. I have started running the exact CI invocation locally before pushing rather than only Nothing further from me on this one. |
Thump604
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 505c9a19. The source remains the narrow shared-canonicalizer fix, and the follow-up commit adds the new test file to the existing Linux matrix invocation. I independently checked run 33890662441: all 15 canonicalization cases execute and pass on Python 3.10, 3.11, 3.12, and 3.13; all ten checks are green.
The prior CI-wiring request is resolved. I found no remaining blocker in this PR.
Follow-up to #524, requested in that thread. Replaces the Anthropic adapter's inline billing-header regex with the shared
canonicalize_system_prompt()from #528.The defect
anthropic_to_openai()carried its own pattern:It is unanchored, so it matches anywhere on a line — including the middle of a user's own sentence. And it runs before
_prepare_chat_messages()callscanonicalize_system_messages(), so the shared pass cannot restore what it already deleted:The two also disagreed on case, in the opposite direction: the shared stripper is
(?im)-anchored and removes a capitalised standalone header; the inline one did not.x-anthropic-billing-header: …on its own lineX-Anthropic-Billing-Header: …on its own lineOnly the second row was a cache-hit question, and on current
mainthe shared pass already covers it, so that half is not a live defect. The third row is user-visible data loss, and it is the reason for this change.The fix
One definition of the pattern instead of two, which is what the registry in #528 was for: the Anthropic path now picks up any future stripper without a second edit. The
reimport is no longer needed in the adapter and is dropped.Tests
tests/test_anthropic_adapter_canonicalization.py, 15 cases across four groups:Authorization:header, prose mentioning billing headersEvery case drives the real
anthropic_to_openai()and, where the two-stage behaviour matters, the realcanonicalize_system_messages()— no reimplementation of either.Mutation-checked. Restoring the inline regex fails 5 of the 15:
Verification
blackclean. No benchmark here: this is a correctness fix on a code path, and the cache-hit claim I originally made for it did not survive checking.