review: don't let a brace-literal in the model's prose shadow the real JSON - #79
Open
tarekziade wants to merge 1 commit into
Open
review: don't let a brace-literal in the model's prose shadow the real JSON#79tarekziade wants to merge 1 commit into
tarekziade wants to merge 1 commit into
Conversation
…l JSON
peft#3482 published the model's raw reply as the review summary — reasoning
prose followed by the review JSON verbatim, escaped quotes and all — with
event downgraded to COMMENT and zero inline comments.
Cause: GLM-5.2 narrated before answering, and one narration line quoted
`_caches = {}`. `_extract_json`'s raw_decode-at-every-`{` pass returned the
first decodable object, and `{}` decodes fine, so the real payload later in
the same reply was never seen. Empty summary + no comments then tripped the
prose-salvage fallback, which published the whole reply (and peeled the same
`{}` out of the narration, mangling it).
Split extraction into `_json_candidates` (the same three passes, now yielding
each object with the span it occupies) and `_select_json`, which prefers the
first object carrying one of the caller's contract keys, then the first
non-empty object, and only then a bare `{}`. Callers pass REVIEW_JSON_KEYS /
TASK_JSON_KEYS. `_prose_outside_json` now peels the object `_extract_json`
actually chose, so a brace-literal in prose survives intact when the salvage
path does legitimately fire.
Verified by replaying the real peft#3482 reply: old path returned `{}`, new
path returns the review with event=APPROVE and no literal \" in the summary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@askserge review this fix |
![sergereview[bot]](https://avatars.githubusercontent.com/in/3673521?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤗 Serge says:
This review failed and posted nothing usable. Body replaced after the fact.
The model (moonshotai/Kimi-K2.6) serialized a tool call into its message content as raw chat-template special tokens instead of the structured tool_calls field, and serge published that markup as the review body. No actual review was produced — please disregard and re-run.
Root cause and fix: #81. Task: 738183d9a7d5444d96b7811bd7651b8d.
tarekziade
added a commit
that referenced
this pull request
Jul 28, 2026
## What happened [This review](#79 (review)) was published on #79 with a body of nothing but Kimi's raw tool-call markup and zero inline comments: ``` 🤗 **Serge** says: <|tool_calls_section_begin|><|tool_call_begin|>functions.read_file:6<|tool_call_argument_begin|> <|tool_call_end|><|tool_calls_section_end|> ``` ## Root cause — three defects compounding 1. **The provider leaked a tool call as text.** On turn 8, `moonshotai/Kimi-K2.6` via the HF Router serialized its tool call into `message.content` using chat-template special tokens, left `message.tool_calls` empty, and returned `finish_reason: "stop"`. `_run_agentic_loop` took the `not chat.tool_calls` branch and treated it as the final answer. `_needs_final_salvage` didn't fire either — the content wasn't blank and the finish reason wasn't `length`. 2. **`_extract_json` accepted the tool arguments as the review.** Its third pass (`raw_decode` at every `{`) parsed the leaked call's own argument object, `{"path": "reviewbot/reviewer.py", "start_line": 248, "end_line": 280}`, so no `_UnparseableLLMOutput` was raised. 3. **The empty-summary salvage published the markup.** With `summary=""` and no comments, the stub-JSON fallback called `_prose_outside_json`, which stripped the JSON and returned the surrounding special tokens as the summary. `publish_review` posted it. Reproduced byte-for-byte against the persisted job row before fixing. ## Scope Scanning the prod jobs DB, **2 of 22 Kimi jobs** hit defect 1; no other model did. The other one was a `/tasks` run that ended `no_fix` (degraded, nothing published). ## The fix Each defect is fixed at its own layer, so no single one has to hold alone. **`llm_client`** — recover text-serialized tool calls into real `ToolCall` objects and strip the markup from the content, so the turn stays a tool turn and the model gets the file it asked for. Covers the streaming and buffered paths (they converge on one place). Structured `tool_calls` still wins whenever the provider sends them. Tolerant of a truncated tail; drops a call whose id isn't recognizable rather than inventing a tool name. **`_extract_json`** — new optional `require_any_key`. The review caller passes `("summary", "comments", "event")` and the task callers pass `("title", "body", "patch")`, so incidental JSON can't pass as a result. **Default behaviour is unchanged** for any caller that doesn't opt in. **`publish_review`** — raises the new `EmptyReviewError` instead of posting a review with no summary and no inline comments, and a summary that is only leaked markup counts as no summary. A markup-only summary that arrives *with* real inline comments still publishes, but renders the `(no overall summary provided)` line rather than the tokens. All three publish paths report the failure rather than swallowing it: - webhook auto-publish → marks the job `error` and posts a failure comment - `run_review` → posts a comment in the thread - the UI publish endpoint → `409 empty_review`, so the human can edit the summary and retry A legitimate review that quotes a special token (`<|endoftext|>` while discussing a tokenizer) is deliberately left untouched — the check only rejects text with no substance left, it never rewrites text that has some. ## Tests 29 new tests; 596 pass, `make format` clean. - `TextToolCallRecoveryTests` — the exact prod payload, multiple calls, prose around the markup, truncated tail, unrecognizable id, both HTTP paths, structured-wins - `LeakedTextToolCallLoopTests` — end-to-end through the *real* client with mocked HTTP: the leaked turn executes `read_file`, the output comes back as a `tool` message, the loop reaches a proper review - `ExtractJsonRequiredKeysTests` — the leaked arguments are rejected as a review but still accepted without the filter - `ModelMarkupOnlyTests`, `EmptyReviewGateTests` — the publish gate, including the quoted-special-token case 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@askserge review this fix |
Contributor
There was a problem hiding this comment.
🤗 Serge says:
Clean, well-tested fix for a real production bug. The extraction logic is now split into candidate generation and selection, with contract keys disambiguating brace-literals in prose from the actual payload. All three call sites pass the right key tuples, and _prose_outside_json peels the same object that _extract_json consumed. Tests cover the regression, task contract, stub fallback, and prose-peeling alignment. No issues found.
serge v0.1.0 · model: moonshotai/Kimi-K2.6 · 4 LLM turns · 4 tool calls · 90.5s · 39141 in / 16249 out tokens
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.
The bug
peft#3482 review rendered as the model's raw reply: chain-of-thought prose, then the review JSON verbatim (
\"fix HiRA sequential merge,\"and friends visible on the page).eventcame outCOMMENTinstead of theAPPROVEthe model actually returned, and 0 inline comments.Cause
GLM-5.2 narrated before answering, and one narration line quoted a brace-literal:
_extract_json's third pass ranraw_decodeat every{and returned the first decodable object.{}decodes fine, so the real payload sitting at the end of the same reply was never reached. Emptysummary+ nocommentsthen tripped the prose-salvage fallback inrun_review, which published the whole reply — and_prose_outside_jsonpeeled that same{}, which is why the page shows`_caches =with the braces gone.Confirmed against the stored reply from the prod job DB: old code →
{}.Fix
_json_candidates(same three passes — whole text, fenced blocks,raw_decodeat every{— now yielding each object with the span it occupies) and_select_json, which picks:{}.REVIEW_JSON_KEYS = ("summary", "event", "comments")andTASK_JSON_KEYS = ("patch", "title", "body"), passed at all three call sites (review parse, task plan parse, normalizer validation)._prose_outside_jsonnow peels the object_extract_jsonactually chose, so when the salvage path does legitimately fire (a genuine stub reply), a brace-literal quoted in the prose survives intact.Behaviour with no
expect_keysis unchanged except that a bare{}no longer wins over a later non-empty object.Verification
Replayed the real peft#3482 reply through the new path:
summary= the actual review,event=APPROVE, no literal\", salvage fallback does not fire.Tests:
ExtractJsonExpectKeysTests(8 cases, incl. the_caches = {}regression, the task contract, and stub-still-reaches-salvage) plus 2 inProseOutsideJsonTests. Full suite: 502 passed.ruff format/ruff checkclean.🤖 Generated with Claude Code