dirge-agent 0.13.0

//! Bounded in-loop LLM critic (F6 tier 3).
//!
//! When a `critic_provider` is configured, the verifier gate can escalate
//! from cheap signals to a single LLM judgement at the finalization
//! boundary: given the user's request and the work done this run, is the
//! task actually complete and correct? If the critic says no, its
//! concrete issues are injected as a follow-up and the loop continues;
//! otherwise the run finalizes. Bounded to one call per run (the caller
//! enforces this) and OFF unless a critic provider is configured — so it
//! never adds latency or cost to a default session.
//!
//! The actual LLM call is a [`CriticFn`] callback (mirrors
//! `compression::SummarizeFn`) built in the provider layer; this module
//! owns the prompt, the verdict parsing, and the loop-message wiring so
//! they're unit-testable without a model.

use std::future::Future;
use std::pin::Pin;
use std::sync::Arc;

use super::message::{LoopMessage, UserMessage};
use super::verifier::VerificationStatus;

/// One-shot critic call: takes a fully-built prompt, returns the model's
/// raw verdict text. Mirrors `compression::SummarizeFn` so the provider
/// layer can build it from any configured model.
pub type CriticFn = Arc<
    dyn Fn(String) -> Pin<Box<dyn Future<Output = anyhow::Result<String>> + Send>> + Send + Sync,
>;

/// Tag prefixed onto the critic's injected follow-up message. The agent
/// loop re-enters it as a user-role message (so the model acts on it); the
/// UI keys on this tag to render it under a distinct `<critic>` handle and
/// color instead of the user's. Shared so producer and renderer agree.
pub const CRITIC_TAG: &str = "[critic]";

/// System preamble for the critic: establishes its role and a calibrated —
/// not trigger-happy — stance. Passed as the LLM system prompt by
/// `build_critic_fn` so the model knows what it is BEFORE it sees the
/// transcript. The response FORMAT lives in [`build_prompt`] instead —
/// right next to the material being judged.
///
/// dirge-bedj: the stance was over-aggressive ("be skeptical", everything
/// "NOT complete") and constraint-blind, so it demanded actions the agent
/// was explicitly told not to take (e.g. pushing). It now (a) respects the
/// agent's own instructions and (b) blocks only on concrete, in-scope gaps.
pub const CRITIC_PREAMBLE: &str = "\
You are a code-review critic for an autonomous coding agent. You are given the instructions and \
constraints the assistant operates under, plus a transcript of what it just did to satisfy the \
user's request. Judge ONLY whether the task is actually complete and correct within those \
constraints — not style.\n\
\n\
Hard rules:\n\
- RESPECT the assistant's instructions. NEVER flag the absence of an action the instructions \
forbid or defer (e.g. if it was told not to push/commit/deploy, do NOT ask it to). Treat anything \
the instructions place out of scope as correctly omitted.\n\
- Block only on CONCRETE, in-scope incompleteness with evidence (e.g. the user asked for X and X \
is missing; a change was made but never built/tested when verification was expected).\n\
- A tool result tagged `[DENIED]` (or whose text begins `Permission denied` / `Auto-approval \
denied`) is a PERMISSION block, not a failure to fix. Treat that capability as out of scope: \
never demand the assistant retry it, route around it, or accomplish the blocked action some \
other way. Judge the rest of the work as if that action were correctly deferred to the user.\n\
- A block marked `[CONTEXT COMPACTION — REFERENCE ONLY]` (or a `## Active Task` lifted from one) \
describes ALREADY-COMPLETED prior work — never treat it as an outstanding requirement. Judge only \
the latest request and the transcript.\n\
- Do NOT invent new requirements, scope, or \"nice to haves\". If you are unsure, PASS — a false \
block wastes a whole turn.";

/// Response-format instruction. Kept in the user prompt (not the system
/// preamble) so the verdict shape sits directly beside the transcript.
const CRITIC_FORMAT: &str = "\
Respond in EXACTLY this format and nothing else:\n\
On the first line, either `VERDICT: COMPLETE` or `VERDICT: INCOMPLETE`.\n\
If INCOMPLETE, follow with a short bullet list of the specific, concrete, in-scope issues to fix.";

/// Cap on the instructions/constraints block fed to the critic, so a large
/// system prompt (tool docs + project context) doesn't balloon the critic
/// call. Generous — the constraints that matter (AGENTS.md, prompt-mode
/// rules) sit early; a truncation note tells the critic more was elided.
const MAX_RULES_CHARS: usize = 16_000;

/// Drop the context-compaction summary from the critic's `rules`. The rules
/// are the agent's merged system prompt, built as `preamble + "\n\n" + history`
/// (`provider::spawn`), so the summary — a `[CONTEXT COMPACTION — REFERENCE
/// ONLY]` System message — always lands AFTER the genuine constraints.
/// Truncating at the marker keeps the real rules (identity, tool docs,
/// AGENTS.md, prompt-mode scope) and discards the stale summary, whose
/// `## Active Task` describes already-completed work the critic would
/// otherwise demand again (the stale-state bug). Returns the input unchanged
/// when no summary is present.
///
/// Shared with the sibling goal gate ([`super::goal`]), which feeds the same
/// merged system prompt to the same judge and needs the same protection.
pub(crate) fn strip_compaction_summary(rules: &str) -> &str {
    match rules.find(crate::agent::compression::COMPACTION_MARKER) {
        Some(idx) => rules[..idx].trim_end(),
        None => rules,
    }
}

/// Render the verification-status block for the critic prompt (dirge-6q3w).
/// Empty unless code was edited this run — that's the precondition that
/// keeps the critic from nagging about tests on a no-code-change turn.
/// When code WAS edited, it gives the critic the concrete signal the
/// cheap verifier gate already computed, plus a calibrated instruction so
/// it treats an unverified/red change as a real, in-scope gap rather than
/// inventing busywork.
fn verification_block(verification: Option<VerificationStatus>) -> &'static str {
    match verification {
        Some(VerificationStatus::Unverified) => {
            "\n\n--- verification status ---\n\
             Code was edited this run but no build/test/lint was detected. If one is runnable \
             here and not forbidden, flag the unverified change as a concrete gap and name the \
             command to run. This is a NUDGE, not a hard rule: if there is nothing to run, the \
             change isn't testable (docs, config, scaffolding), or the assistant already verified \
             another way and said so, treat it as COMPLETE — never force a test that can't be \
             run.\n--- end verification status ---"
        }
        Some(VerificationStatus::VerifiedRed) => {
            "\n\n--- verification status ---\n\
             Code was edited and the most recent build/test FAILED. Don't pass a red build — this \
             is INCOMPLETE — UNLESS the assistant explicitly said the failure is pre-existing, \
             expected, or unrelated to the change.\n--- end verification status ---"
        }
        Some(VerificationStatus::VerifiedGreen) => {
            "\n\n--- verification status ---\n\
             Code was edited and a build/test passed. Sanity-check only that the verification was \
             RELEVANT to the change (e.g. tests covering the edited area, not just an unrelated \
             build); don't manufacture extra requirements.\n--- end verification status ---"
        }
        // No code edited (precondition not met) or no gate configured →
        // add nothing, so the critic behaves exactly as before.
        Some(VerificationStatus::NoCodeEdited) | None => "",
    }
}

/// Build the critic prompt. `rules` is the assistant's own system prompt /
/// instructions (so the critic judges against the SAME constraints the
/// agent had — dirge-bedj), minus any compaction summary (see
/// [`strip_compaction_summary`]); `transcript` is what the agent did;
/// `verification` is the run's compile/lint/test signal (dirge-6q3w),
/// `None` when no verifier gate is configured. The role lives in
/// [`CRITIC_PREAMBLE`]; this carries the format + bodies.
pub fn build_prompt(
    rules: &str,
    transcript: &str,
    verification: Option<VerificationStatus>,
) -> String {
    let rules = strip_compaction_summary(rules).trim();
    let rules_block = if rules.is_empty() {
        "(no special constraints provided)".to_string()
    } else if rules.len() > MAX_RULES_CHARS {
        let head: String = rules.chars().take(MAX_RULES_CHARS).collect();
        format!("{head}\n…(instructions truncated)")
    } else {
        rules.to_string()
    };
    format!(
        "{CRITIC_FORMAT}\n\n\
         --- assistant instructions & constraints (judge within these; never demand a \
         forbidden/out-of-scope action) ---\n{rules_block}\n--- end instructions ---\n\n\
         --- transcript ---\n{transcript}\n--- end transcript ---{}",
        verification_block(verification)
    )
}

/// Parse the critic's raw response into a verdict. `Some(issues)` means
/// the critic judged the work incomplete (with the issue text); `None`
/// means complete — or the response was empty/ambiguous, in which case we
/// fail OPEN (don't block finalization on a confused critic).
pub fn parse_verdict(response: &str) -> Option<String> {
    let trimmed = response.trim();
    if trimmed.is_empty() {
        return None;
    }
    // Look at the first non-empty line for the verdict token.
    let first = trimmed.lines().find(|l| !l.trim().is_empty()).unwrap_or("");
    let upper = first.to_ascii_uppercase();
    if upper.contains("INCOMPLETE") {
        // Everything after the first line is the issue list; fall back to
        // the whole response if the model put issues on the first line.
        let rest = trimmed
            .split_once('\n')
            .map(|(_, x)| x)
            .map(str::trim)
            .filter(|s| !s.is_empty())
            .unwrap_or(trimmed);
        Some(rest.to_string())
    } else {
        // "COMPLETE", or anything that isn't a clear INCOMPLETE → pass.
        None
    }
}

/// Run the critic over a run transcript. `rules` is the assistant's own
/// system prompt / instructions, passed so the critic judges within the
/// SAME constraints the agent had (dirge-bedj); `verification` is the
/// run's compile/lint/test signal so the critic can be pickier about
/// unverified changes (dirge-6q3w). Returns a one-element vec with a
/// [`CRITIC_TAG`]-prefixed follow-up message when the critic judged the
/// work incomplete; empty otherwise (complete, or the call errored — fail
/// open). Never panics on a critic error.
pub async fn run_critic(
    critic: &CriticFn,
    rules: &str,
    transcript: &str,
    verification: Option<VerificationStatus>,
) -> Vec<LoopMessage> {
    let prompt = build_prompt(rules, transcript, verification);
    let response = match critic(prompt).await {
        Ok(r) => r,
        Err(e) => {
            tracing::warn!(target: "dirge::critic", error = %e, "critic call failed; finalizing without it");
            return Vec::new();
        }
    };
    match parse_verdict(&response) {
        Some(issues) => vec![LoopMessage::User(UserMessage {
            content: format!(
                "{CRITIC_TAG} A review of your work found it may not be done yet. Address these \
                 before reporting complete, or explain why they don't apply (e.g. they're out of \
                 scope or something you were told not to do):\n{issues}"
            ),
        })],
        None => Vec::new(),
    }
}

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

    #[test]
    fn parse_complete_returns_none() {
        assert!(parse_verdict("VERDICT: COMPLETE").is_none());
        assert!(parse_verdict("verdict: complete\n(looks good)").is_none());
    }

    #[test]
    fn parse_incomplete_returns_issues() {
        let v = parse_verdict("VERDICT: INCOMPLETE\n- missing test\n- error path unhandled");
        let issues = v.expect("should be incomplete");
        assert!(issues.contains("missing test"));
        assert!(issues.contains("error path"));
    }

    #[test]
    fn parse_empty_or_ambiguous_fails_open() {
        assert!(parse_verdict("").is_none());
        assert!(parse_verdict("   \n  ").is_none());
        assert!(parse_verdict("I think it's probably fine?").is_none());
    }

    #[test]
    fn prompt_embeds_transcript_format_and_rules() {
        let p = build_prompt(
            "RULE: never push to remote.",
            "user asked X; assistant edited foo.rs",
            None,
        );
        assert!(p.contains("VERDICT: COMPLETE"));
        assert!(p.contains("VERDICT: INCOMPLETE"));
        assert!(p.contains("edited foo.rs"));
        // dirge-bedj: the agent's own constraints are included so the
        // critic judges within them.
        assert!(p.contains("never push to remote"), "rules must be embedded");
        assert!(
            p.to_lowercase().contains("forbidden") || p.to_lowercase().contains("out-of-scope"),
            "prompt must instruct the critic to respect constraints",
        );
    }

    #[test]
    fn empty_rules_render_a_placeholder_not_blank() {
        let p = build_prompt("", "did stuff", None);
        assert!(p.contains("no special constraints"));
    }

    /// dirge: the critic's `rules` is the agent's merged system prompt, which
    /// after a compaction carries the `[CONTEXT COMPACTION — REFERENCE ONLY]`
    /// summary describing ALREADY-COMPLETED prior work. The critic must judge
    /// against the agent's real constraints, not a stale summary's
    /// `## Active Task` — else it blocks finalization on superseded work
    /// (e.g. demanding an old "Phase 3" that's already done).
    #[test]
    fn build_prompt_drops_the_compaction_summary_from_rules() {
        let rules = format!(
            "RULE: never push to remote.\n\n{} \
             ## Active Task\nFinish Phase 3: wire the Janet loader and add tests.",
            crate::agent::compression::COMPACTION_MARKER,
        );
        let p = build_prompt(&rules, "user asked X; assistant edited foo.rs", None);
        // The agent's genuine constraint (it precedes the summary) survives…
        assert!(
            p.contains("never push to remote"),
            "real rules must survive"
        );
        // …but the stale summary's contents must NOT reach the critic.
        assert!(
            !p.contains("Active Task") && !p.contains("Phase 3") && !p.contains("Janet"),
            "the compaction summary must be stripped from the critic's rules",
        );
        assert!(
            !p.contains(crate::agent::compression::COMPACTION_MARKER),
            "the compaction marker itself must be stripped",
        );
    }

    /// Defense-in-depth: even if a summary block reaches the critic by some
    /// other path, the preamble tells it to discount reference-only material.
    #[test]
    fn preamble_discounts_reference_only_blocks() {
        let lower = CRITIC_PREAMBLE.to_ascii_lowercase();
        assert!(
            lower.contains("reference") || lower.contains("compaction"),
            "preamble must tell the critic to ignore reference-only/compaction blocks",
        );
    }

    #[test]
    fn build_prompt_caps_large_rules() {
        let huge = "x".repeat(MAX_RULES_CHARS + 5_000);
        let p = build_prompt(&huge, "t", None);
        assert!(p.contains("instructions truncated"));
        // The rules block is bounded (cap + the transcript/format scaffold,
        // well under the untruncated size).
        assert!(p.len() < MAX_RULES_CHARS + 4_000);
    }

    /// The system preamble states the critic's ROLE, keeps FORMAT out, and
    /// (dirge-bedj) instructs it to respect the agent's constraints.
    #[test]
    fn preamble_is_calibrated_and_constraint_aware() {
        let lower = CRITIC_PREAMBLE.to_ascii_lowercase();
        assert!(lower.contains("critic"), "preamble must name the role");
        assert!(!lower.contains("summarizer"));
        // Format lives in the prompt, not the system preamble.
        assert!(!CRITIC_PREAMBLE.contains("VERDICT:"));
        assert!(build_prompt("", "t", None).contains("VERDICT:"));
        // Must not demand forbidden actions, and must respect instructions.
        assert!(
            lower.contains("respect"),
            "must say to respect instructions"
        );
        assert!(
            lower.contains("never flag the absence") || lower.contains("forbid"),
            "must forbid demanding disallowed actions",
        );
        assert!(lower.contains("unsure"), "must keep the fail-open guidance");
    }

    // dirge-6q3w: verification-status block.

    /// No gate configured → prompt is byte-identical to the pre-feature
    /// behavior (no verification block at all).
    #[test]
    fn no_verification_status_adds_no_block() {
        let p = build_prompt("rules", "did stuff", None);
        assert!(!p.contains("verification status"));
    }

    /// Precondition: no code edited this run → no verification pressure,
    /// even though the gate is present. The critic shouldn't nag about
    /// tests on a read-only / Q&A turn.
    #[test]
    fn no_code_edited_adds_no_block() {
        let p = build_prompt("rules", "did stuff", Some(VerificationStatus::NoCodeEdited));
        assert!(!p.contains("verification status"));
    }

    #[test]
    fn unverified_block_pushes_to_run_a_check() {
        let p = build_prompt(
            "rules",
            "edited foo.rs",
            Some(VerificationStatus::Unverified),
        );
        assert!(p.contains("verification status"));
        let lower = p.to_lowercase();
        assert!(lower.contains("no build/test/lint was detected"));
        assert!(lower.contains("concrete"), "must frame it as a real gap");
    }

    /// The unverified block must stay a soft nudge with an explicit escape
    /// hatch, so the model never fabricates a test that can't be run.
    #[test]
    fn unverified_block_is_a_soft_nudge() {
        let p = build_prompt(
            "rules",
            "edited foo.rs",
            Some(VerificationStatus::Unverified),
        );
        let lower = p.to_lowercase();
        assert!(lower.contains("nudge"), "must call itself a nudge");
        assert!(
            lower.contains("isn't testable") || lower.contains("nothing to run"),
            "must offer a not-testable escape",
        );
        assert!(
            lower.contains("never force a test that can't be run"),
            "must forbid fabricating an unrunnable test",
        );
    }

    #[test]
    fn red_block_forbids_passing_a_red_build() {
        let p = build_prompt(
            "rules",
            "edited foo.rs",
            Some(VerificationStatus::VerifiedRed),
        );
        let lower = p.to_lowercase();
        assert!(lower.contains("failed"));
        assert!(lower.contains("incomplete"));
    }

    #[test]
    fn green_block_stays_calibrated() {
        let p = build_prompt(
            "rules",
            "edited foo.rs",
            Some(VerificationStatus::VerifiedGreen),
        );
        let lower = p.to_lowercase();
        assert!(lower.contains("passed"));
        // Must not manufacture new requirements on a green run.
        assert!(lower.contains("relevant"));
    }

    #[tokio::test]
    async fn run_critic_threads_verification_into_prompt() {
        use std::sync::Mutex;
        let seen: Arc<Mutex<String>> = Arc::new(Mutex::new(String::new()));
        let seen2 = seen.clone();
        let critic: CriticFn = Arc::new(move |prompt: String| {
            *seen2.lock().unwrap() = prompt;
            Box::pin(async { Ok("VERDICT: COMPLETE".to_string()) })
        });
        let _ = run_critic(
            &critic,
            "rules",
            "edited foo.rs",
            Some(VerificationStatus::Unverified),
        )
        .await;
        assert!(
            seen.lock().unwrap().contains("verification status"),
            "the verification signal must reach the critic prompt",
        );
    }

    #[tokio::test]
    async fn run_critic_injects_followup_when_incomplete() {
        let critic: CriticFn = Arc::new(|_prompt| {
            Box::pin(async { Ok("VERDICT: INCOMPLETE\n- the test was never run".to_string()) })
        });
        let msgs = run_critic(&critic, "rules", "did stuff", None).await;
        assert_eq!(msgs.len(), 1);
        let content = match &msgs[0] {
            LoopMessage::User(u) => &u.content,
            _ => panic!("expected user message"),
        };
        assert!(content.starts_with(CRITIC_TAG));
        assert!(content.contains("test was never run"));
    }

    #[tokio::test]
    async fn run_critic_passes_rules_into_prompt() {
        use std::sync::Mutex;
        let seen: Arc<Mutex<String>> = Arc::new(Mutex::new(String::new()));
        let seen2 = seen.clone();
        let critic: CriticFn = Arc::new(move |prompt: String| {
            *seen2.lock().unwrap() = prompt;
            Box::pin(async { Ok("VERDICT: COMPLETE".to_string()) })
        });
        let _ = run_critic(&critic, "RULE: do not deploy", "did stuff", None).await;
        assert!(
            seen.lock().unwrap().contains("do not deploy"),
            "the agent's constraints must reach the critic prompt",
        );
    }

    #[tokio::test]
    async fn run_critic_silent_when_complete() {
        let critic: CriticFn =
            Arc::new(|_p| Box::pin(async { Ok("VERDICT: COMPLETE".to_string()) }));
        assert!(
            run_critic(&critic, "rules", "did stuff", None)
                .await
                .is_empty()
        );
    }

    #[tokio::test]
    async fn run_critic_fails_open_on_error() {
        let critic: CriticFn = Arc::new(|_p| Box::pin(async { anyhow::bail!("provider down") }));
        assert!(
            run_critic(&critic, "rules", "did stuff", None)
                .await
                .is_empty(),
            "a critic error must not block finalization"
        );
    }
}