Skip to content

fix(permission): budget permission review output and classify its failures - #57

Merged
locez merged 1 commit into
mainfrom
fix/permission-review-output-budget
Sep 15, 2026
Merged

locez merged 1 commit into
mainfrom
fix/permission-review-output-budget

Conversation

@locez

@locez locez commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Fixes the "AI review unavailable: permission review output is invalid: permission review completed without stop finish reason" escalation loop reported for session a4daeba5-7108-47b9-80ba-7eb9b9b592d0.

Root cause

Permission review asked the reviewer for at most 512 output tokens. On a reasoning reviewer those tokens are billed together with hidden reasoning, so the response ended as response.incomplete long before the review JSON arrived. complete_single_text only accepts a stop finish, so the truncation surfaced as InvalidReviewOutput, and model_then_human escalated to the human dialog.

Evidence from that session: the reviewer stream (19:36:03.184 -> .646) contains only response.reasoning_text.delta events, ends in response.incomplete, and never reaches the review schema; across the log, all 35 truncated reviews are the reasoning reviewer while all 50 responses from the non-reasoning reviewer completed.

Changes

  • crates/merry-runtime/src/permission/review.rs: reviewer budget 512 -> 2048, and the reviewer request now asks for a low reasoning effort so hidden thinking cannot consume the whole allowance.
  • crates/merry-runtime/src/permission.rs: new typed PermissionAdmissionError::ReviewOutputTruncated { finish_reason }; InvalidReviewOutput now means only that the reviewer returned output outside the review contract.
  • crates/merry-runtime/src/permission/review.rs: classify_non_stop_review_finish splits Length (truncated), Blocked/Error (failed review with the provider cause), and everything else (contract violation).
  • examples/config.toml: documents that runtime owns the reviewer request shape, that review effort is independent of the primary reasoning_effort, and that the role should point at a provider accepting a reasoning effort.

The reviewer request shape stays owned by runtime and provider-neutral; no provider wire type or policy crosses a layer boundary, and the primary step request is unchanged (its reasoning_effort remains optional and configuration-driven).

Verification

  • cargo fmt --all --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test --all (all targets ok)
  • New tests: reviewer request carries the wide budget and low effort; Length maps to ReviewOutputTruncated; Blocked/Error stay failed reviews.

Not verified: no live model call was made, so the endpoint behavior of low effort is untested; the change is documented in examples/config.toml as the mitigation for that.

Follow-ups not in this change

  • Retry a truncated review once with a larger budget instead of escalating immediately.
  • Constrain reviewer rationale length in the system prompt.
  • Make reviewer reasoning effort configurable through [models.approval_review] if a provider needs it omitted.

…lures

Permission review asked the reviewer for at most 512 output tokens. On a
reasoning reviewer those tokens are billed with hidden reasoning, so the
response ended as `response.incomplete` long before the review JSON, and the
runtime reported the truncation as invalid reviewer output. Sessions using a
reasoning model as the approval reviewer therefore escalated nearly every
permission request to the human fallback.

Raise the reviewer budget to one order of magnitude above a review JSON,
request the lowest standard reasoning effort so thinking cannot consume the
whole allowance, and keep the two knobs documented where a reader looks for
them. The reviewer request shape stays owned by runtime and independent of the
primary provider's reasoning_effort, which examples/config.toml now states.

Split the non-stop finish classification so a response the reviewer never
controlled reads as a failed or truncated review instead of a contract
violation:

- Length -> new ReviewOutputTruncated { finish_reason }
- Blocked/Error -> ReviewFailed with the provider cause
- anything else -> InvalidReviewOutput

InvalidReviewOutput now means only that the reviewer returned output outside
the review contract, which lets runtime policy retry or escalate a truncated
review without parsing error text.

Verification: cargo fmt --all --check; cargo clippy --all-targets
--all-features -- -D warnings; cargo test --all (all targets ok).
@locez
locez merged commit ca479e2 into main Sep 15, 2026
8 checks passed
@locez
locez deleted the fix/permission-review-output-budget branch September 15, 2026 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant