Uh oh!
There was an error while loading. Please reload this page.
Fix parse_response/2 marker leak + drive handle_ai_response/2 via real test - #562
Closed
mdon wants to merge 2 commits into
Closed
Fix parse_response/2 marker leak + drive handle_ai_response/2 via real test#562mdon wants to merge 2 commits into
mdon wants to merge 2 commits into
Conversation
The boundary regex for each requested field only included OTHER
requested markers as terminators. When a model emitted a marker
the caller didn't ask for, that block's content silently
appended to the preceding requested field's capture.
Real-world trigger: the projects prompt template referenced
`{{title}}` literally (no `title` variable was bound for a
template resource that only has name + description), so the AI
dutifully emitted `---TITLE---{{title}}` between `---NAME---`
and `---DESCRIPTION---`. The parser, asked for only `name` and
`description`, set name to "Mitarbeiter-Onboarding---TITLE---{{title}}"
because nothing in its boundary regex matched `---TITLE---`.
Fix: bound captures at any `---[A-Z0-9_]+---` marker, not just
the explicitly-requested ones. The character class matches what
`marker/1` already produces. The `i` flag still tolerates
lowercased markers in the response. Unrequested markers'
content is dropped silently — they're not in the requested
fields list, so the caller never gets them either way.
Regression test pins the exact real-world response we saw.Codex final-review findings on PR BeamLabEU#560: CONCERN: the original "handles OpenAI-shaped response map" test manually extracted message.content and then called parse_response/2. The test would still have passed against the pre-fix broken implementation because it never drove the new handle_ai_response/2 branch. Fixed by: - Exposing handle_ai_response/2 as @doc false def so the unit test can drive it directly (instead of going through the private path). - Rewrote the test to call Translation.handle_ai_response(map, fields) with a realistic OpenAI response shape. Now actually fails if the map-handling branch regresses. - Added two more handle_ai_response/2 tests covering the raw-binary legacy/stub path and the malformed-shape error path. CONCERN: the earlier implementation called PhoenixKitAI.Completion.extract_content/1, which raises UndefinedFunctionError in core's test env where the plugin isn't loaded. Replaced the cross-module call with an inline pattern match on the OpenAI shape (choices[0].message.content). The shape is stable (OpenAI-standard), so the inline match is safe; and removing the dep means the helper works in core's test env and in any host regardless of which PhoenixKitAI version they pin. NIT: the test comment now matches the test behavior.
5 tasks
mdon
commented
May 22, 2026
ContributorAuthor
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Follow-up to PR #560 — two commits that landed on
mdon/fix-translation-response-shapeafter the merge.1. Parser marker-leak bug (real bug, present on
devtoday)parse_response/2's field-capture boundary regex only terminated at REQUESTED markers. A model that emits a marker the caller didn't ask for silently rolls that block's content into the previous requested field.Real-world trigger: projects-side prompt template referenced
{{title}}literally (notitlevariable bound for a template resource that only has name + description), so the AI emitted---TITLE---{{title}}between---NAME---and---DESCRIPTION---. The parser, asked for onlynameanddescription, returned name as"Mitarbeiter-Onboarding---TITLE---{{title}}"— corrupted.Fix: widen the boundary regex to terminate at any
---[A-Z0-9_]+---marker (the character class matches whatmarker/1already produces). Unrequested markers' content is dropped silently — they're not in the requested fields list, so the caller never gets them either way.Verified end-to-end against real deepseek-v3.2: DE-DE and FR-FR translations of the Employee onboarding template were clean after this fix, dirty before.
2.
handle_ai_response/2testabilityCodex final-review of #560 caught that the original "OpenAI-shaped response map" test manually extracted
message.contentand only droveparse_response/2directly — it would still have passed against the pre-fix broken implementation. Reworked:handle_ai_response/2fromdefpto@doc false defso unit tests can drive it directly.PhoenixKitAI.Completion.extract_content/1call with an inline pattern-match on the OpenAI shape. Drops the cross-module dep — works in core's test env (plugin absent) and any host (plugin present, any version).Test plan
handle_ai_response/2tests cover the OpenAI map / raw binary / malformed shape branchesmix credo --strictclean