steep/crates/steep-server/src/domain/review.rs

//! Domain types of a review: what the model answers with, and how it is read.

use std::str::FromStr;

use serde::{Deserialize, Deserializer};
use tracing::warn;

use crate::text::excerpt;

#[derive(Deserialize, Debug)]
pub struct ReviewResult {
    pub reviews: Vec<ReviewItem>,
    pub comment: String,
    pub cost: Option<f64>,
}

#[derive(Deserialize, Debug)]
pub struct ReviewItem {
    pub filename: String,
    pub line: Option<u64>,
    #[serde(default, deserialize_with = "deserialize_side")]
    pub side: Option<ReviewSide>,
    #[serde(default, deserialize_with = "deserialize_severity")]
    pub severity: Option<ReviewSeverity>,
    pub message: String,
}

/// Which version of the file a review comment is anchored on.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ReviewSide {
    /// The line was added by the pull request: `line` is a line number of the
    /// new version of the file.
    Added,
    /// The line was removed by the pull request: `line` is a line number of the
    /// old version of the file.
    Removed,
}

impl ReviewSide {
    /// Reads the side the model asked for, tolerating casing and synonyms.
    fn parse(raw: &str) -> Option<Self> {
        match raw.trim().to_ascii_lowercase().as_str() {
            "added" | "add" | "new" | "right" => Some(Self::Added),
            "removed" | "remove" | "deleted" | "delete" | "old" | "left" => Some(Self::Removed),
            _ => None,
        }
    }
}

/// What kind of problem a review reports.
///
/// The categories are the ones the prompt asks for; a review whose severity is
/// missing or unreadable falls back to [`ReviewSeverity::Maintainability`], the
/// least alarming bucket, instead of failing the whole review.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ReviewSeverity {
    /// Wrong behaviour.
    Bug,
    /// A vulnerability.
    Security,
    /// A resource problem.
    Performance,
    /// Everything else: readability, structure, naming, tests.
    Maintainability,
}

impl ReviewSeverity {
    /// Reads the severity the model asked for, tolerating casing and synonyms.
    fn parse(raw: &str) -> Option<Self> {
        match raw.trim().to_ascii_lowercase().as_str() {
            "bug" | "bugs" | "correctness" | "error" => Some(Self::Bug),
            "security" | "vulnerability" => Some(Self::Security),
            "performance" | "perf" => Some(Self::Performance),
            "maintainability" | "quality" | "style" => Some(Self::Maintainability),
            _ => None,
        }
    }

    /// Lower-case label used in the review markdown.
    pub fn label(self) -> &'static str {
        match self {
            Self::Bug => "bug",
            Self::Security => "security",
            Self::Performance => "performance",
            Self::Maintainability => "maintainability",
        }
    }
}

/// Reads the side the model asked for. An unreadable value is ignored instead of
/// failing the whole review: the side is then resolved from the changed lines.
fn deserialize_side<'de, D>(deserializer: D) -> Result<Option<ReviewSide>, D::Error>
where
    D: Deserializer<'de>,
{
    let raw = Option::<serde_json::Value>::deserialize(deserializer)?;

    Ok(raw
        .as_ref()
        .and_then(serde_json::Value::as_str)
        .and_then(ReviewSide::parse))
}

/// Reads the severity the model asked for, ignoring an unreadable value: the
/// review is then reported as [`ReviewSeverity::Maintainability`].
fn deserialize_severity<'de, D>(deserializer: D) -> Result<Option<ReviewSeverity>, D::Error>
where
    D: Deserializer<'de>,
{
    let raw = Option::<serde_json::Value>::deserialize(deserializer)?;

    Ok(raw
        .as_ref()
        .and_then(serde_json::Value::as_str)
        .and_then(ReviewSeverity::parse))
}

/// 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])
}

#[cfg(test)]
mod tests {
    use super::*;

    #[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 severities_are_read_from_the_model() {
        let result: ReviewResult = serde_json::from_str(
            r#"{
                "reviews": [
                    { "filename": "a", "line": 1, "severity": "Bug", "message": "x" },
                    { "filename": "b", "line": 2, "severity": "SECURITY", "message": "x" },
                    { "filename": "c", "line": 3, "severity": "perf", "message": "x" },
                    { "filename": "d", "line": 4, "severity": "banana", "message": "x" }
                ],
                "comment": ""
            }"#,
        )
        .unwrap();

        let severities = result
            .reviews
            .iter()
            .map(|review| review.severity)
            .collect::<Vec<_>>();

        assert_eq!(
            severities,
            vec![
                Some(ReviewSeverity::Bug),
                Some(ReviewSeverity::Security),
                Some(ReviewSeverity::Performance),
                None,
            ]
        );
    }
}