replace ContainerRuntime CLI to Bollard crate (default: docker socket)

limit tool result to open router tool response (with truncated info for
ai)

Hard kill if graceful shutdown is too long

crates/herald-server/src/bot_actions/review.rs

@@ -1,3 +1,5 @@
+use std::str::FromStr;
+
 use tracing::{info, instrument, warn};
 
 use crate::{
@@ -7,6 +9,7 @@ use crate::{
     metrics,
     open_router::{OpenRouterClient, Tool},
     sandbox::{Sandbox, SandboxConfig, agent},
+    text::excerpt,
 };
 
 #[instrument(skip(gitea_api, open_router_client, sandbox_config, tools, review_payload))]
@@ -59,7 +62,7 @@ pub async fn exec_review(
             .replace("{comment}", &review_payload.comment.body)
             .replace("{changes}", &changes);
 
-        let (message, cost) = run_sandboxed_review(
+        let (mut review_result, cost) = run_sandboxed_review(
             gitea_api,
             open_router_client,
             sandbox_config,
@@ -69,7 +72,6 @@ pub async fn exec_review(
         )
         .await?;
 
-        let mut review_result = serde_json::from_str::<ReviewResult>(&message)?;
         resolve_review_sides(&mut review_result, &changed_lines);
 
         review_result.cost = cost;
@@ -112,6 +114,10 @@ pub async fn exec_review(
 
 /// Runs the review inside a sandbox container, letting the model explore the
 /// repository with tools before answering.
+///
+/// The answer of the model is parsed by [`ReviewResult::from_str`], which the
+/// agent loop enforces: an answer that is not a review is sent back to the model
+/// for correction.
 async fn run_sandboxed_review(
     gitea_api: &GiteaAPI,
     open_router_client: &OpenRouterClient,
@@ -119,7 +125,7 @@ async fn run_sandboxed_review(
     tools: Vec<Tool>,
     review_payload: &ReviewPayload,
     bot_request: &str,
-) -> anyhow::Result<(String, Option<f64>)> {
+) -> anyhow::Result<(ReviewResult, Option<f64>)> {
     let repo_url = gitea_api.repo_clone_url(&review_payload.repository.full_name);
 
     let sandbox = Sandbox::create(
@@ -151,7 +157,7 @@ async fn run_sandboxed_review(
         "Sandboxed review finished"
     );
 
-    Ok((result.message, result.cost))
+    Ok((result.answer, result.cost))
 }
 
 fn review_result_to_markdown(review_result: &ReviewResult) -> String {
@@ -452,6 +458,61 @@ fn format_line_numbers(lines: &[u64]) -> String {
         .join(", ")
 }
 
+/// Number of characters of a model answer kept in the logs when it cannot be
+/// parsed.
+const MAX_LOGGED_ANSWER: usize = 500;
+
+impl FromStr for ReviewResult {
+    type Err = anyhow::Error;
+
+    /// Parses the review the model answered with.
+    ///
+    /// This is the contract the agent loop enforces: a rejected answer is sent
+    /// back to the model, with the reason, so that it can correct itself.
+    ///
+    /// The contract is a raw JSON object, but models sometimes wrap it in a
+    /// markdown code fence or surround it with a sentence: the object is then
+    /// extracted from the answer before failing, and the answer is logged so a
+    /// breach of the contract can be diagnosed.
+    fn from_str(message: &str) -> Result<Self, Self::Err> {
+        let error = match serde_json::from_str::<Self>(message) {
+            Ok(review_result) => return Ok(review_result),
+            Err(error) => error,
+        };
+
+        // A markdown code fence or a sentence around the object is tolerated, with
+        // a warning: the contract asks for a raw JSON object.
+        if let Some(json) = json_object(message)
+            && let Ok(review_result) = serde_json::from_str::<Self>(json)
+        {
+            warn!(
+                "Model answer is not a raw JSON object, it was extracted from the surrounding text"
+            );
+
+            return Ok(review_result);
+        }
+
+        // The reason of the rejection is logged along with the answer: without it,
+        // a broken answer is impossible to diagnose.
+        warn!(
+            answer = %excerpt(message, MAX_LOGGED_ANSWER),
+            reason = %error,
+            "Model answer is not the expected JSON"
+        );
+
+        anyhow::bail!("the answer is not valid JSON: {error}")
+    }
+}
+
+/// Returns the outermost `{...}` of an answer, which ignores a markdown code
+/// fence or any text around it.
+fn json_object(message: &str) -> Option<&str> {
+    let start = message.find('{')?;
+    let end = message.rfind('}')?;
+
+    (start < end).then(|| &message[start..=end])
+}
+
 /// Resolves the side each review is anchored on and drops the reviews that do
 /// not match a line the pull request changes.
 ///
@@ -824,6 +885,45 @@ mod tests {
         assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Removed));
     }
 
+    #[test]
+    fn a_raw_answer_is_parsed() {
+        let answer = r#"{"reviews": [], "comment": "ok"}"#;
+
+        assert_eq!(answer.parse::<ReviewResult>().unwrap().comment, "ok");
+    }
+
+    #[test]
+    fn a_fenced_answer_is_extracted() {
+        let answer = concat!(
+            "Voici ma review :\n",
+            "```json\n",
+            "{\"reviews\": [], \"comment\": \"rien à signaler\"}\n",
+            "```\n"
+        );
+
+        assert_eq!(
+            answer.parse::<ReviewResult>().unwrap().comment,
+            "rien à signaler"
+        );
+    }
+
+    #[test]
+    fn a_sentence_around_the_object_is_ignored() {
+        let answer = r#"Rien à signaler. {"reviews": [], "comment": "ok"} Bonne journée !"#;
+
+        assert_eq!(answer.parse::<ReviewResult>().unwrap().comment, "ok");
+    }
+
+    #[test]
+    fn an_answer_without_json_is_rejected() {
+        assert!("Je n'ai rien relevé.".parse::<ReviewResult>().is_err());
+    }
+
+    #[test]
+    fn an_answer_that_is_an_object_but_not_a_review_is_rejected() {
+        assert!(r#"{"message": "LGTM"}"#.parse::<ReviewResult>().is_err());
+    }
+
     #[test]
     fn odd_sides_from_the_model_are_tolerated() {
         let changed_lines = parse_changed_lines(DIFF);