fix(providers/pi): tolerate prose preamble in structured-output responses#1440
Merged
fix(providers/pi): tolerate prose preamble in structured-output responses#1440
Conversation
📝 WalkthroughWalkthroughtryParseStructuredOutput now attempts JSON.parse on cleaned text, and if that fails performs a forward-scan from the first Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Collaborator
Author
PR Review Summary (multi-agent)Six specialist reviews ran in parallel: code-reviewer, docs-impact, pr-test-analyzer, comment-analyzer, silent-failure-hunter, code-simplifier. Type-design-analyzer skipped (no new types). Critical Issues (1)
Three converging mitigations were proposed:
The suffix check is the strongest fix because it actually prevents the wrong parse rather than just observing it. Important Issues (2)
Suggestions (3)
Strengths
VerdictNEEDS FIX — critical issue is the silent-wrong-parse risk. The mitigations are small (a suffix check + a regression test); they don't alter the conservative-ladder design. Once that lands, this is ready to merge. Recommended Actions
|
Wirasm
added a commit
that referenced
this pull request
Apr 27, 2026
- Drop the redundant `firstBrace !== lastBrace` guard on Tier 3. When
the only `{` past position 0 is the same one Tier 2 just tried, Tier
3 will re-run JSON.parse on the same input and fail identically — one
redundant call accepted for simpler control flow. (CodeRabbit cleanup)
- Tighten the Tier 1 comment to drop the model name list (will date)
and tighten the Tier 2 comment to scope its claim to "preamble +
flat JSON" rather than overgeneralized "nested JSON". (CodeRabbit
comment-quality cleanup)
- Replace the bare `// Fall through...` comments with `// fall through`
inside the catch blocks. Necessary because eslint's `no-empty` rule
flags bare `catch {}` blocks. (lint compliance)
- Add a regression test for `{` inside a string value that pins the
Tier 2 → Tier 3 cascade composition: lastIndexOf lands inside the
string brace, JSON.parse rejects the resulting `{ inside","ok":true}`
fragment, Tier 3 forward-scans to the JSON object's outer `{` and
parses cleanly. Note: the preamble must not itself contain `{`,
otherwise Tier 3 lands on it instead of on the JSON object — covered
in the test comment so the constraint isn't lost. (CodeRabbit test
coverage)
- Update `packages/docs-web/src/content/docs/getting-started/ai-assistants.md:378`
— the previous "degrades cleanly when the model emits prose" claim is
now partially false (preamble-only patterns are recovered). New text
enumerates the three handled forms (bare, fenced, prose-preamble)
and clarifies the trailing-text-interleaved case still degrades.
(CodeRabbit docs-impact)
- Add a CHANGELOG entry under `## [Unreleased] / Fixed`.
CodeRabbit's "critical" suffix-check suggestion (`cleaned.slice(brace)
.trimEnd().endsWith('}')`) is not applied. JSON.parse already rejects
trailing non-whitespace, so any successful parse from a `{`-prefixed
slice already ends with `}` after trim — the check is tautological in
all cases the function actually reaches it. The actual concern (a
self-contained `{...}` fragment in preamble that happens to be valid
JSON, with no real answer in the response) wouldn't be caught by the
suffix check either, since the fragment IS a valid JSON object that
ends with `}`. Real defense against that case would require schema
validation, which lives at the call site (executor's
`structured_output_missing` warning path), not in this parser.
Tests: 40 pass / 0 fail. Lint, format, type-check all clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/community/pi/event-bridge.test.ts`:
- Around line 404-410: Add a regression test to ensure tryParseStructuredOutput
still returns undefined when valid-looking JSON appears in a postamble
brace-containing snippet: create a new test case (e.g., "returns undefined when
model wraps JSON in prose with brace-containing postamble") that uses prose like
'Here is the JSON you requested:\n{"ok":true}\nAlso note: {"foo":"bar"}' and
assert expect(tryParseStructuredOutput(prose)).toBeUndefined(); place this
alongside the existing tests around tryParseStructuredOutput to ensure the
parser doesn't erroneously pick up the trailing brace fragment.
In `@packages/providers/src/community/pi/event-bridge.ts`:
- Around line 186-194: The Tier 2 fallback currently slices at lastBrace and may
return a trailing JSON object from responses that include a prior JSON (e.g.,
'{"ok":true}\nFor example: {"verdict":"review"}'), violating the conservative
failure mode; change the logic in the block that computes lastBrace/cleaned so
you only attempt JSON.parse(cleaned.slice(lastBrace)) when the prefix before
lastBrace is empty or contains only whitespace/newlines/separators (i.e.,
cleaned.slice(0, lastBrace).trim() === ''), otherwise skip Tier 2 and return
undefined; update the try/catch around JSON.parse to remain but do not silently
accept a parsed object when the prefix is non-empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1219932-73d9-47ae-ab35-d3d0a1f4dad2
📒 Files selected for processing (4)
CHANGELOG.mdpackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/providers/src/community/pi/event-bridge.test.tspackages/providers/src/community/pi/event-bridge.ts
✅ Files skipped from review due to trivial changes (2)
- packages/docs-web/src/content/docs/getting-started/ai-assistants.md
- CHANGELOG.md
…nses
Pi has no SDK-level JSON-mode parameter and Minimax's Anthropic-compat
proxy doesn't expose response_format support, so reasoning models like
Minimax M2.7 routinely "think out loud" before emitting the JSON we
asked for, e.g.:
"Now I have all the inputs. Let me evaluate the three gates:
**Gate A — Direction alignment**: ...
{\"verdict\":\"review\",\"direction_alignment\":\"aligned\",...}"
The previous tryParseStructuredOutput stripped fences then JSON.parse'd
the whole string, which fails the moment a single character of prose
appears before the {. The maintainer-review-pr workflow's gate node was
hitting this on ~70% of runs, propagating to 7-of-10 silent failures
in a sequential review batch this afternoon.
Add two preamble-tolerant fallback tiers:
Tier 1 (existing): clean JSON.parse — fast path for compliant models
Tier 2 (new): scan backward to last `{`, parse from there — flat
JSON with reasoning preamble (Minimax M2.7 case)
Tier 3 (new): scan forward to first `{`, parse from there —
nested JSON with preamble (Tier 2 lands inside a
child object and fails)
The existing "trailing-prose-too" failure case (preamble + JSON +
postamble) still returns undefined — handling it would require brace-
depth tracking and isn't worth the cost. The two new tiers cover the
failure mode actually observed in production.
Tested against the real Minimax preamble pattern and a synthetic
preamble + nested-JSON case. 39 event-bridge tests pass.
No SDK changes, no Pi extensions, no Minimax API dependencies. Pure
Archon-side parser hardening — backward-compatible and benefits every
verbose-preamble model routed through Pi, not just Minimax.
- Drop the redundant `firstBrace !== lastBrace` guard on Tier 3. When
the only `{` past position 0 is the same one Tier 2 just tried, Tier
3 will re-run JSON.parse on the same input and fail identically — one
redundant call accepted for simpler control flow. (CodeRabbit cleanup)
- Tighten the Tier 1 comment to drop the model name list (will date)
and tighten the Tier 2 comment to scope its claim to "preamble +
flat JSON" rather than overgeneralized "nested JSON". (CodeRabbit
comment-quality cleanup)
- Replace the bare `// Fall through...` comments with `// fall through`
inside the catch blocks. Necessary because eslint's `no-empty` rule
flags bare `catch {}` blocks. (lint compliance)
- Add a regression test for `{` inside a string value that pins the
Tier 2 → Tier 3 cascade composition: lastIndexOf lands inside the
string brace, JSON.parse rejects the resulting `{ inside","ok":true}`
fragment, Tier 3 forward-scans to the JSON object's outer `{` and
parses cleanly. Note: the preamble must not itself contain `{`,
otherwise Tier 3 lands on it instead of on the JSON object — covered
in the test comment so the constraint isn't lost. (CodeRabbit test
coverage)
- Update `packages/docs-web/src/content/docs/getting-started/ai-assistants.md:378`
— the previous "degrades cleanly when the model emits prose" claim is
now partially false (preamble-only patterns are recovered). New text
enumerates the three handled forms (bare, fenced, prose-preamble)
and clarifies the trailing-text-interleaved case still degrades.
(CodeRabbit docs-impact)
- Add a CHANGELOG entry under `## [Unreleased] / Fixed`.
CodeRabbit's "critical" suffix-check suggestion (`cleaned.slice(brace)
.trimEnd().endsWith('}')`) is not applied. JSON.parse already rejects
trailing non-whitespace, so any successful parse from a `{`-prefixed
slice already ends with `}` after trim — the check is tautological in
all cases the function actually reaches it. The actual concern (a
self-contained `{...}` fragment in preamble that happens to be valid
JSON, with no real answer in the response) wouldn't be caught by the
suffix check either, since the fragment IS a valid JSON object that
ends with `}`. Real defense against that case would require schema
validation, which lives at the call site (executor's
`structured_output_missing` warning path), not in this parser.
Tests: 40 pass / 0 fail. Lint, format, type-check all clean.
… footgun
CodeRabbit flagged a real correctness issue: a backward scan from the last
`{` silently returns the wrong JSON object when the response contains a
brace-bearing example after the real payload (e.g. `{"actual":1}\n
For example: {"x":2}` parses the trailing example instead of failing).
This breaks the conservative-failure contract callers rely on.
Tier 2 was always strictly worse than Tier 3 anyway: every preamble pattern
Tier 2 handled, Tier 3 handles too, and Tier 3 doesn't have the
multi-fragment hazard. Dropping Tier 2 entirely removes the footgun and
keeps the parser simple (clean parse → forward scan → undefined).
Tests updated:
- Renamed/clarified existing tests to reference the surviving forward-scan
tier instead of the deleted backward-scan tier (no behavior change for
the cases they cover — preamble has no extra braces).
- Added a regression test for the brace-postamble footgun: input with a
trailing example like `{"actual":"value"}\nFor example: {"verdict":"review"}`
must return undefined under the new contract.
Wirasm
force-pushed
the
fix/pi-parse-prose-preamble
branch
from
3272784 to
bf77ac9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/providers/src/community/pi/event-bridge.ts (1)
186-196: Add debug telemetry for successful Tier 2 recovery.When heuristic parsing succeeds, a debug event with
preambleLengthwould make recovery behavior observable in production.As per coding guidelines: "Structured logging with Pino: use event naming format `{domain}.{action}_{state}` (standard states: _started, _completed, _failed, _validated, _rejected) ..."♻️ Proposed patch
const firstBrace = cleaned.indexOf('{'); if (firstBrace > 0) { try { - return JSON.parse(cleaned.slice(firstBrace)); + const parsed = JSON.parse(cleaned.slice(firstBrace)); + getLog().debug( + { preambleLength: firstBrace }, + 'pi.structured_output_recovery_completed' + ); + return parsed; } catch { // fall through } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/event-bridge.ts` around lines 186 - 196, Add a debug telemetry/event log inside the Tier 2 recovery success path (inside the try block that does JSON.parse(cleaned.slice(firstBrace))) that emits a structured debug event following the Pino naming convention {domain}.{action}_{state} (e.g., "pi.recovery_completed" or "eventBridge.tier2_recovered_completed") and include a field preambleLength set to firstBrace (and optionally the cleaned length or snippet metadata). Place the log call immediately before the return so it only fires on successful JSON.parse, using the existing logger/telemetry mechanism in this module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/providers/src/community/pi/event-bridge.ts`:
- Around line 186-196: Add a debug telemetry/event log inside the Tier 2
recovery success path (inside the try block that does
JSON.parse(cleaned.slice(firstBrace))) that emits a structured debug event
following the Pino naming convention {domain}.{action}_{state} (e.g.,
"pi.recovery_completed" or "eventBridge.tier2_recovered_completed") and include
a field preambleLength set to firstBrace (and optionally the cleaned length or
snippet metadata). Place the log call immediately before the return so it only
fires on successful JSON.parse, using the existing logger/telemetry mechanism in
this module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8807fef1-d35e-4d42-816d-6a6d07aa116c
📒 Files selected for processing (4)
CHANGELOG.mdpackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/providers/src/community/pi/event-bridge.test.tspackages/providers/src/community/pi/event-bridge.ts
✅ Files skipped from review due to trivial changes (1)
- packages/docs-web/src/content/docs/getting-started/ai-assistants.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/providers/src/community/pi/event-bridge.test.ts
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
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
tryParseStructuredOutputreturns undefined, the gate'sverdictfield is unreadable, everywhen:branch fails-closed, and no review comment is posted.tryParseStructuredOutput. Tier 2 scans backward to the last{(handles flat JSON after preamble — the M2.7 case). Tier 3 scans forward to the first{(handles nested JSON after preamble, missed by Tier 2).Why this approach over the alternatives
I researched four options before settling on this one. Summary of why each alternative was rejected:
response_formatvia Pibefore_provider_requestextensionresponse_formatin the payload; even if injected viaonPayload, Minimax's Anthropic-compat proxy atapi.minimax.io/anthropicis not publicly documented to honor the field. Unverified dependency.agent_endagent_end. The only extension hooks (before_provider_request,after_provider_response) operate on HTTP payloads, not decoded text. Confirmed via local inspection of~/.npm-global/.../pi-coding-agent/dist/.tryParseStructuredOutput(this PR)provider: claudeoverride on the gate nodeoutput_formatagainst a verbose model stays broken.Option C addresses the root cause in the parser, so the fix benefits every Pi-routed model (Qwen, DeepSeek, Llama variants, future models) — not just Minimax.
Validation Evidence (required)
parses preamble + trailing flat JSON (Minimax M2.7 reasoning-model pattern)— exercises Tier 2 with the actual prose pattern observed today.parses preamble + trailing nested JSON via tier-3 forward scan— exercises Tier 3, where Tier 2 fails because the last{is inside a child object.returns undefined when model wraps JSON in prose with trailing text— this case (preamble + JSON + postamble) still returns undefined; updated test name and comment to clarify why the new tiers don't catch it.Security Impact (required)
Compatibility / Migration
Human Verification (required)
json …) — Tier 1 strips fences and parses. Unchanged.{inside a string value —lastIndexOf('{')finds the inner one, Tier 2 fails, Tier 3 recovers via forward scan.{}) —firstBrace === lastBrace, so Tier 3 is skipped (correct — no second scan needed). Tier 1 already parsed it.[]— Tier 1 parses cleanly; Tiers 2/3 don't fire (indexOf('{') === -1).archonis bun-linked to source.Side Effects / Blast Radius (required)
@archon/providers(Pi event-bridge). The function is called from one place —provider.ts'sagent_endhandler — and is only invoked whenoutput_formatis set on the workflow node. Workflows without structured output are unaffected.undefined). Callers handleundefinedby emitting adag.structured_output_missingwarning and degrading; with this fix, that warning fires less often.Rollback Plan (required)
git revert <merge-sha>— single commit, ~10 net LOC, fully reversible.{as if it were the structured output), downstreamwhen:conditions would route to wrong branches. Mitigated by the conservative ladder — Tier 1 only fires on full-text parse, Tiers 2/3 only fire after Tier 1 fails. Extra brace inside a string value is the only way to get a wrong parse, and the order (last-then-first) makes that a very narrow case.Risks and Mitigations
{characters embedded in string values (e.g., aregex_pattern: "{name}"field) could trigger Tier 2 to parse from the wrong{. Tier 3 falls back to first{and would parse correctly.Linked Issue
Summary by CodeRabbit
Bug Fixes
Tests
Documentation