diff --git a/crates/merry-runtime/src/permission.rs b/crates/merry-runtime/src/permission.rs index 871e649c..94ba82b4 100644 --- a/crates/merry-runtime/src/permission.rs +++ b/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 }, diff --git a/crates/merry-runtime/src/permission/review.rs b/crates/merry-runtime/src/permission/review.rs index 9ec4ae67..2c9b34da 100644 --- a/crates/merry-runtime/src/permission/review.rs +++ b/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, @@ -52,9 +66,12 @@ impl ModelBackedPermissionAdmissionSource { pub(crate) fn from_config( config: ModelProviderConfig, ) -> Result { + 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"), + }, + } +} diff --git a/crates/merry-runtime/src/permission/tests/review.rs b/crates/merry-runtime/src/permission/tests/review.rs index 7f67e29d..588abb4a 100644 --- a/crates/merry-runtime/src/permission/tests/review.rs +++ b/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) -> 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}" + ); + } +} diff --git a/examples/config.toml b/examples/config.toml index a3166af5..382ed78b 100644 --- a/examples/config.toml +++ b/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]