fix(reviewer): show the model the schema instead of naming it #86
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/gllm-show-the-schema"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #85.
What was happening
Two reviews on
brooktrails/infraposted a JSON blob as the review body:verdictandverdict_descriptivedecoded correctly beside it,finish_reasonwasstop, no truncation, no prompt override. The model put an invented JSON payload into thebodystring.Why
v0.41.2 (#84) gave the gllm path this closing instruction:
The model has never seen that schema. gllm compiles it into a decoding grammar and does not put it in the model's context — unlike tool calling, where the provider renders the tool definition,
descriptiontext and all. The instruction named an object the model had no way to know, so it invented a plausiblesubmit_reviewpayload. The grammar still forced the real keys at the top level, so the invention landed insidebody.The previous instruction ("use the submit_review tool") was wrong in a way the model ignored — it wrote prose, and the grammar shaped it correctly. The new one is more directive about JSON, so the model took the schema name seriously. Distribution matches:
The fix
Render the schema into the prompt,
gllmSchemaapplied, so the text the model reads and the grammar its output is decoded against cannot describe different objects. That subsumes the matching-names property the old instruction was built around — there are no longer two names to keep in agreement.submissiondrops to the tool sentence alone; JSON mode no longer has a per-pass line of its own.Also states in the
bodydescription that the field holds US-English Markdown prose and not a nested payload — the failure written as a rule, in the one place all three backends read.Measured against the live endpoint
inference.brooktrails.org,mistral-small-4, same schema and diff, counting bodies that start with{:Quality moves too. The schema
descriptiontext reaches this path for the first time, including the body-as-scratchpad contract, and the bodies go from one-line boilerplate:to reasoning about the actual diff:
Notes
bodydescription, which is the intended reach of that change.map[string]any+encoding/jsonsorts keys). The intended emission order survives only inrequired, which is a[]string.x-gllm-orderedis what would make it binding; still a follow-up.docs/gllm-schema-notes.mdgains a section on why the schema is sent twice.Tests
TestJSONSchemaPromptsCarryTheirSchema— every pass's prompt against its builder.TestGLLMPromptSchemaMatchesRequest— the prompt's schema against the schema in the wire request it was sent with (replaces the name-matching version).TestJSONSchemaReviewPromptShowsTierVerdicts— the enum shown matches the tier.TestToolModePromptsUnchangedunmodified and passing.gofmt,go vet,go build ./...,go test -race ./...all clean.🤖 Generated with Claude Code
v0.41.2 gave the gllm path a closing instruction that reads "Respond with a single JSON object conforming to the submit_review schema." The model has never seen that schema. gllm compiles it into a decoding grammar and does not put it in the model's context — unlike tool calling, where the provider renders the definition, descriptions and all. So the instruction named an object the model had no way to know, and it filled the gap by inventing a plausible submit_review payload. The grammar still forced the real keys at the top level, so the invention landed inside the body string. Two reviews reached Forgejo as {"review_id": "auto-pr-226", "verdict": "approved", "summary": ...} with verdict and verdict_descriptive decoding correctly beside it and finish_reason=stop. Nothing in the pipeline was broken; the model was answering a question it could not have answered. 2 of the 4 gllm reviews on 0.41.2 came out this way, against 0 of the 17 on every version before it. Render the schema into the prompt instead, gllmSchema applied, so the text the model reads and the grammar its output is decoded against cannot describe different objects. That subsumes the matching-names property the old instruction was built around: there are no longer two names to keep in agreement. submission drops to the tool sentence alone, since JSON mode no longer has a per-pass line of its own. Against mistral-small-4 the old prompt reproduced the nested JSON in 3 of 4 samples and the new one in 0 of 14. Review quality moves too — the schema descriptions reach this path for the first time, and the body-as-scratchpad contract they carry is visible in the output, which goes from one-line boilerplate to actual reasoning about the diff. Also say plainly in the body description that the field holds US-English Markdown prose and not a nested payload. That is the failure stated as a rule, in the one place all three backends read. Tool mode is untouched: same sentences, byte for byte, and Anthropic and vLLM continue to get their schemas via tool definitions. They do see the reworded body description, which is the intended reach of that change.Automated review by pr-reviewer v0.41.2 | Safety Check | Mistral Small | tracking id
r-67eebe-216b39This is an AI-generated review and may contain mistakes.
Status: ✅ Completed
✅ Verdict: LGTM — All changes are safe, well-tested, and address the reported issue without introducing new risks. No security concerns, breaking changes, or accidentally committed files detected.
No issues found. Changes are safe and properly tested.