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

crates/merry-runtime/src/permission.rs

@@ -7,6 +7,7 @@
 
 use crate::{PathAccess, ProcessActionIntent};
 use merry_core::{CoreError, PendingToolCall, ToolName};
+use merry_llm::FinishReason;
 use serde::{Deserialize, Serialize};
 use std::{future::Future, pin::Pin};
 use thiserror::Error;
@@ -617,9 +618,22 @@ pub enum PermissionAdmissionError {
     /// Model-backed review failed before producing a decision.
     #[error("permission review failed: {message}")]
     ReviewFailed { message: String },
-    /// Model-backed review returned unsupported output.
+    /// Model-backed review returned output that violates the review contract.
     #[error("permission review output is invalid: {message}")]
     InvalidReviewOutput { message: String },
+    /// The reviewer ran out of output tokens before it could answer.
+    ///
+    /// This is an output-budget failure rather than a contract violation: the
+    /// reviewer never reached the review contract, so runtime policy may retry
+    /// it with a larger budget or hand the decision to the host instead of
+    /// recording the reviewer as non-compliant.
+    #[error(
+        "permission review ran out of output tokens before answering: reviewer finished with {finish_reason:?} instead of stop"
+    )]
+    ReviewOutputTruncated {
+        /// Finish reason reported by the provider.
+        finish_reason: FinishReason,
+    },
     /// The optional human fallback could not accept or await a response.
     #[error("human permission review is unavailable: {message}")]
     HumanReviewUnavailable { message: String },

crates/merry-runtime/src/permission/review.rs

@@ -30,8 +30,8 @@ use crate::model_completion::{
 };
 use crate::model_config::ModelProviderConfig;
 use merry_llm::{
-    GenerationConfig, ModelContent, ModelError, ModelMessage, ModelMessageRole, ModelName,
-    ModelProvider, ModelRequest, ModelStreamContext,
+    FinishReason, GenerationConfig, ModelContent, ModelError, ModelMessage, ModelMessageRole,
+    ModelName, ModelProvider, ModelRequest, ModelStreamContext, ReasoningEffort,
 };
 use serde::Deserialize;
 use std::sync::Arc;
@@ -40,7 +40,21 @@ use std::sync::Arc;
 pub(crate) const PERMISSION_REVIEW_SCHEMA_VERSION: &str = "permission_review.v1";
 
 /// Maximum output tokens reserved for one permission review response.
-const PERMISSION_REVIEW_MAX_OUTPUT_TOKENS: u64 = 512;
+///
+/// The budget covers the reviewer's hidden reasoning tokens as well as the
+/// answer, so it stays an order of magnitude above the size of one review JSON
+/// object. A ceiling that only fits the answer turns an ordinary reasoning pass
+/// into a truncated response whose finish reason is not [`FinishReason::Stop`],
+/// which the review contract rejects.
+const PERMISSION_REVIEW_MAX_OUTPUT_TOKENS: u64 = 2048;
+
+/// Reasoning effort requested for one permission review response.
+///
+/// Review is a bounded classification over recorded evidence, not an
+/// open-ended task, so reviewer thinking is capped at the lowest standard
+/// provider effort: enough to weigh risk and authorization, while leaving the
+/// output budget for the answer the review contract requires.
+const PERMISSION_REVIEW_REASONING_EFFORT: &str = "low";
 
 pub(crate) struct ModelBackedPermissionAdmissionSource {
     provider: Arc<dyn ModelProvider>,
@@ -52,9 +66,12 @@ impl ModelBackedPermissionAdmissionSource {
     pub(crate) fn from_config(
         config: ModelProviderConfig,
     ) -> Result<Self, PermissionAdmissionError> {
+        let reasoning_effort = ReasoningEffort::new(PERMISSION_REVIEW_REASONING_EFFORT)
+            .map_err(map_permission_model_request_error)?;
         let generation_config =
             GenerationConfig::new(Some(PERMISSION_REVIEW_MAX_OUTPUT_TOKENS), false)
-                .map_err(map_permission_model_request_error)?;
+                .map_err(map_permission_model_request_error)?
+                .with_reasoning_effort(Some(reasoning_effort));
         Ok(Self {
             provider: config.provider(),
             model: config.model().clone(),
@@ -282,10 +299,8 @@ fn map_permission_review_completion_error(error: ModelCompletionError) -> Permis
         ModelCompletionError::ToolCallRequested => PermissionAdmissionError::InvalidReviewOutput {
             message: "permission review model must not request tools".to_owned(),
         },
-        ModelCompletionError::NonStopFinish { .. } => {
-            PermissionAdmissionError::InvalidReviewOutput {
-                message: "permission review completed without stop finish reason".to_owned(),
-            }
+        ModelCompletionError::NonStopFinish { finish_reason } => {
+            classify_non_stop_review_finish(finish_reason)
         }
         ModelCompletionError::NotSingleText => PermissionAdmissionError::InvalidReviewOutput {
             message: "permission review stop output must contain exactly one text item".to_owned(),
@@ -297,3 +312,26 @@ fn map_permission_review_completion_error(error: ModelCompletionError) -> Permis
         }
     }
 }
+
+/// Classifies a reviewer response that ended for a reason other than a stop.
+///
+/// Only output the reviewer itself controls is a review-contract violation.
+/// Running out of output tokens, a provider-side failure, and a provider
+/// safety filter all mean the review never reached the contract, so they stay
+/// distinguishable from invalid reviewer output and remain recoverable by
+/// runtime policy instead of reading as a broken reviewer.
+fn classify_non_stop_review_finish(finish_reason: FinishReason) -> PermissionAdmissionError {
+    match finish_reason {
+        FinishReason::Length => PermissionAdmissionError::ReviewOutputTruncated { finish_reason },
+        FinishReason::Blocked => PermissionAdmissionError::ReviewFailed {
+            message: "permission review response was blocked by the provider's safety filter"
+                .to_owned(),
+        },
+        FinishReason::Error => PermissionAdmissionError::ReviewFailed {
+            message: "provider reported a failed permission review response".to_owned(),
+        },
+        other => PermissionAdmissionError::InvalidReviewOutput {
+            message: format!("permission review finished with {other:?} instead of stop"),
+        },
+    }
+}

crates/merry-runtime/src/permission/tests/review.rs

@@ -7,17 +7,40 @@ use crate::permission::review::{
 };
 use crate::permission::{
     ModelBackedPermissionAdmissionSource, PermissionAdmissionContext, PermissionAdmissionError,
-    PermissionAdmissionSource, PermissionReviewRisk, PermissionUserAuthorization,
-    permission_request_from_call,
+    PermissionAdmissionSource, PermissionRequest, PermissionReviewRisk,
+    PermissionUserAuthorization, permission_request_from_call,
 };
 use merry_llm::{
     FinishReason, ModelEvent, ModelName, ModelOutput, ModelResponse, ModelRetryPolicy,
-    testing::FakeModelProvider,
+    ReasoningEffort, testing::FakeModelProvider,
 };
 use serde_json::json;
 use std::sync::Arc;
 use tokio_util::sync::CancellationToken;
 
+/// Builds a reviewer source backed by one scripted provider response.
+fn review_source(provider: Arc<FakeModelProvider>) -> ModelBackedPermissionAdmissionSource {
+    ModelBackedPermissionAdmissionSource::from_config(ModelProviderConfig::new(
+        provider,
+        ModelName::new("fake/reviewer").expect("model name should be valid"),
+        ModelRetryPolicy::default(),
+    ))
+    .expect("review source should build")
+}
+
+/// Builds the permission request every reviewer test reviews.
+fn review_request() -> PermissionRequest {
+    permission_request_from_call(
+        &call(json!({
+            "reason": "Confirm the endpoint is reachable",
+            "requested": { "network": true },
+            "for_action": { "command": "curl -sI https://example.com", "cwd": null }
+        })),
+        Vec::new(),
+    )
+    .expect("request should parse")
+}
+
 #[test]
 fn model_review_parser_maps_approve_and_deny() {
     let approved = parse_permission_review_model_output(
@@ -106,25 +129,11 @@ async fn model_review_request_declares_the_accepted_schema_version() {
             None,
         ),
     })]));
-    let source = ModelBackedPermissionAdmissionSource::from_config(ModelProviderConfig::new(
-        provider.clone(),
-        ModelName::new("fake/reviewer").expect("model name should be valid"),
-        ModelRetryPolicy::default(),
-    ))
-    .expect("review source should build");
-    let request = permission_request_from_call(
-        &call(json!({
-            "reason": "Confirm the endpoint is reachable",
-            "requested": { "network": true },
-            "for_action": { "command": "curl -sI https://example.com", "cwd": null }
-        })),
-        Vec::new(),
-    )
-    .expect("request should parse");
+    let source = review_source(provider.clone());
 
     source
         .review(
-            request,
+            review_request(),
             PermissionAdmissionContext::new(CancellationToken::new()),
         )
         .await
@@ -147,3 +156,118 @@ async fn model_review_request_declares_the_accepted_schema_version() {
         "user prompt must declare the schema version the parser accepts"
     );
 }
+
+#[tokio::test(flavor = "current_thread")]
+async fn model_review_request_keeps_thinking_modest_and_output_budget_wide() {
+    // Reviewers on reasoning models bill hidden reasoning tokens against the
+    // same ceiling as the answer. The reviewer request must therefore keep
+    // thinking at a low effort and leave the ceiling far above one review
+    // JSON, otherwise an ordinary reasoning pass is truncated before the
+    // reviewer can answer in the accepted schema.
+    let provider = Arc::new(FakeModelProvider::new(vec![Ok(ModelEvent::Completed {
+        response: ModelResponse::new(
+            vec![ModelOutput::text(
+                r#"{"schema_version":"permission_review.v1","decision":"approve","risk":"low","user_authorization":"high","rationale":"The exact command is grounded in the task."}"#,
+            )],
+            FinishReason::Stop,
+            None,
+        ),
+    })]));
+    let source = review_source(provider.clone());
+
+    source
+        .review(
+            review_request(),
+            PermissionAdmissionContext::new(CancellationToken::new()),
+        )
+        .await
+        .expect("review should be accepted");
+
+    let recorded = provider.recorded_requests();
+    let [model_request] = recorded.as_slice() else {
+        panic!("expected exactly one recorded review request");
+    };
+    let generation = model_request.generation();
+    assert_eq!(generation.max_output_tokens(), Some(2048));
+    assert_eq!(
+        generation.reasoning_effort().map(ReasoningEffort::as_str),
+        Some("low")
+    );
+}
+
+#[tokio::test(flavor = "current_thread")]
+async fn model_review_reports_a_truncated_answer_as_an_output_budget_failure() {
+    // A reviewer that runs out of output tokens never reaches the review
+    // schema. The failure must name the output budget rather than read as a
+    // reviewer that returned output outside the contract.
+    let provider = Arc::new(FakeModelProvider::new(vec![Ok(ModelEvent::Completed {
+        response: ModelResponse::new(
+            vec![ModelOutput::text(
+                r#"{"schema_version":"permission_review.v1","decision":"approve","#,
+            )],
+            FinishReason::Length,
+            None,
+        ),
+    })]));
+    let source = review_source(provider);
+
+    let error = source
+        .review(
+            review_request(),
+            PermissionAdmissionContext::new(CancellationToken::new()),
+        )
+        .await
+        .expect_err("a truncated review must not be accepted");
+
+    let message = error.to_string();
+    assert!(
+        matches!(
+            &error,
+            PermissionAdmissionError::ReviewOutputTruncated { finish_reason }
+                if *finish_reason == FinishReason::Length
+        ),
+        "message was {message}"
+    );
+    assert!(
+        message.contains("ran out of output tokens"),
+        "message should name the exhausted output budget: {message}"
+    );
+    assert!(message.contains("Length"), "message was {message}");
+}
+
+#[tokio::test(flavor = "current_thread")]
+async fn model_review_separates_provider_failures_from_invalid_reviewer_output() {
+    // A reviewer the provider never let answer is a failed review, not a
+    // contract violation. Keeping the two apart lets runtime policy retry or
+    // escalate instead of recording the reviewer as non-compliant.
+    for (finish_reason, expected) in [
+        (FinishReason::Blocked, "safety filter"),
+        (FinishReason::Error, "failed permission review response"),
+    ] {
+        let provider = Arc::new(FakeModelProvider::new(vec![Ok(ModelEvent::Completed {
+            response: ModelResponse::new(
+                vec![ModelOutput::text("no review decision")],
+                finish_reason,
+                None,
+            ),
+        })]));
+        let source = review_source(provider);
+
+        let error = source
+            .review(
+                review_request(),
+                PermissionAdmissionContext::new(CancellationToken::new()),
+            )
+            .await
+            .expect_err("a non-stop review must not be accepted");
+
+        assert!(
+            matches!(&error, PermissionAdmissionError::ReviewFailed { .. }),
+            "{finish_reason:?} should stay a failed review, got {error}"
+        );
+        assert!(
+            error.to_string().contains(expected),
+            "message was {error}, expected {expected}"
+        );
+    }
+}

examples/config.toml

@@ -270,6 +270,11 @@ model = "gpt-4.1-mini"
 [models.approval_review]
 # Optional model override for permission admission review. Omit this table to
 # review with [providers.default].model.
+#
+# Merry owns the reviewer request shape: every review asks for a low reasoning
+# effort and its own fixed output budget, independently of the primary
+# provider's reasoning_effort. Point this role at a provider that accepts a
+# reasoning effort, and prefer a small fast model over a deep-reasoning one.
 model = "gpt-4.1-mini"
 
 [providers.openai-compatible]