review: anonymize author identity, calibrate severity, surface subprocess errors

Settings.toml

@@ -97,3 +97,8 @@ worktree_dir = "review_trees"
 timeout_seconds = 7200
 max_retries = 3
 ignore_files = ["MAINTAINERS", ".mailmap", ".gitignore", "LICENSES/"]
+# Strip author-reputation cues (Signed-off-by/Reviewed-by and other identity
+# trailers, the author header, and e-mail addresses) from the commit-message
+# region of the patch the model sees. The diff body, e-mail routing, the
+# stored patch, and the public report are unaffected. On by default; set to false to show author identity.
+# anonymize_authors = false

src/bin/review.rs

@@ -336,14 +336,41 @@ async fn main() -> Result<()> {
                                     String::new()
                                 };
 
+                            // Anonymize the author-reputation cues in the
+                            // model's view only (diff body, patch application,
+                            // routing, and the stored/public patch are
+                            // untouched). Off unless `[review] anonymize_authors`.
+                            let anonymize = settings.review.anonymize_authors;
+                            let author = if anonymize { None } else { p.author.clone() };
+                            let diff = if anonymize {
+                                sashiko::patch::anonymize_patch_text(&p.diff)
+                            } else {
+                                p.diff.clone()
+                            };
+                            let git_show = patch_shows.get(&p.index).cloned().map(|s| {
+                                if anonymize {
+                                    sashiko::patch::anonymize_patch_text(&s)
+                                } else {
+                                    s
+                                }
+                            });
+                            let commit_message_full =
+                                patch_messages.get(&p.index).cloned().map(|s| {
+                                    if anonymize {
+                                        sashiko::patch::anonymize_patch_text(&s)
+                                    } else {
+                                        s
+                                    }
+                                });
+
                             json!({
                                 "subject": p.subject,
-                                "author": p.author,
+                                "author": author,
                                 "date_string": date_str,
-                                "diff": p.diff,
+                                "diff": diff,
                                 "commit_id": patch_shas.get(&p.index).cloned(),
-                                "git_show": patch_shows.get(&p.index).cloned(),
-                                "commit_message_full": patch_messages.get(&p.index).cloned()
+                                "git_show": git_show,
+                                "commit_message_full": commit_message_full
                             })
                         })
                         .collect();

src/bin/sashiko-cli.rs

@@ -1585,9 +1585,16 @@ async fn handle_local(
             drop(stdin);
         }
 
-        // Stream stderr for progress in a background task
+        // Stream stderr for progress, while keeping a bounded tail of the
+        // subprocess's log lines. The progress UI consumes these lines, so
+        // without the tail a failed review would collapse to a bare "no output"
+        // message with its actual cause (a stage error, an auth/quota failure)
+        // discarded. On failure we replay the tail so the run explains itself.
         let stderr = child.stderr.take();
         let stderr_handle = tokio::spawn(async move {
+            use std::collections::VecDeque;
+            const STDERR_TAIL_LINES: usize = 100;
+            let mut tail: VecDeque<String> = VecDeque::with_capacity(STDERR_TAIL_LINES);
             if let Some(stderr) = stderr {
                 use tokio::io::{AsyncBufReadExt, BufReader};
                 let reader = BufReader::new(stderr);
@@ -1609,8 +1616,13 @@ async fn handle_local(
                         eprint_phase(4, 4, "Review complete.");
                         eprintln!();
                     }
+                    if tail.len() == STDERR_TAIL_LINES {
+                        tail.pop_front();
+                    }
+                    tail.push_back(line);
                 }
             }
+            tail
         });
 
         // Capture stdout
@@ -1619,12 +1631,17 @@ async fn handle_local(
             .await
             .context("Failed to wait for review subprocess")?;
 
-        let _ = stderr_handle.await;
+        let stderr_tail = stderr_handle.await.unwrap_or_default();
 
         let exit_code = output.status.code().unwrap_or(1);
         let stdout = String::from_utf8_lossy(&output.stdout).to_string();
 
         if stdout.trim().is_empty() {
+            // The subprocess failed before emitting a result; replay its log
+            // tail so the actual cause is visible rather than swallowed.
+            for line in &stderr_tail {
+                eprintln!("{line}");
+            }
             return Err(anyhow::anyhow!(
                 "Review subprocess produced no output (exit code: {})",
                 exit_code

src/patch.rs

@@ -400,10 +400,173 @@ pub fn extract_email(author: &str) -> String {
     author.trim().to_string()
 }
 
+/// Author-identity trailer lines removed from the model's view of a commit
+/// message when `[review] anonymize_authors` is enabled. Matched
+/// case-insensitively against each line after trimming leading whitespace
+/// (`git show` indents commit-message bodies, so trailers appear as
+/// "    Signed-off-by:"). The trailing colon is load-bearing: it keeps prose
+/// lines such as "From the previous discussion" from being mistaken for
+/// trailers.
+///
+/// `Fixes:` is deliberately absent: it carries a SHA and a description of
+/// the prior commit being fixed, which is technical provenance the model
+/// needs to trace code history, not an identity cue.
+const IDENTITY_LINE_PREFIXES: &[&str] = &[
+    "from:",
+    "author:",
+    "signed-off-by:",
+    "co-developed-by:",
+    "co-authored-by:",
+    "reviewed-by:",
+    "acked-by:",
+    "tested-by:",
+    "reported-by:",
+    "suggested-by:",
+    "cc:",
+];
+
+/// Byte offset at which the diff body begins (the first line starting with
+/// `diff --git `), or `text.len()` if there is none. Everything from this
+/// offset onward is the diff and is never anonymized.
+fn diff_body_start(text: &str) -> usize {
+    const MARKER: &str = "diff --git ";
+    if text.starts_with(MARKER) {
+        return 0;
+    }
+    match text.find(&format!("\n{MARKER}")) {
+        Some(pos) => pos + 1,
+        None => text.len(),
+    }
+}
+
+/// Strip author-reputation cues from the commit-message region of patch text,
+/// for the model's view only. Drops identity trailer lines (Signed-off-by,
+/// Reviewed-by, Author, Cc, ...) and redacts e-mail addresses. The diff
+/// body (everything from the first `diff --git` line onward) is left
+/// byte-for-byte unchanged, so the code under review and the model's quotes are
+/// unaffected. This runs only while assembling the model's input; the patch as
+/// stored in the database, applied to the worktree, and quoted in the public
+/// report is untouched. Removing the author-reputation cue ("X wrote this, so
+/// it must be right") is the point: see `[review] anonymize_authors`.
+pub fn anonymize_patch_text(text: &str) -> String {
+    static EMAIL_RE: OnceLock<Regex> = OnceLock::new();
+    let email_re = EMAIL_RE.get_or_init(|| {
+        Regex::new(r"<?[A-Za-z0-9._%+\-]+@[A-Za-z0-9.\-]+\.[A-Za-z]{2,}>?").unwrap()
+    });
+
+    let (message_region, diff_body) = text.split_at(diff_body_start(text));
+
+    let mut out = String::with_capacity(text.len());
+    for line in message_region.split_inclusive('\n') {
+        let content = line.strip_suffix('\n').unwrap_or(line);
+        let trimmed = content.trim_start().to_ascii_lowercase();
+        if IDENTITY_LINE_PREFIXES.iter().any(|p| trimmed.starts_with(p)) {
+            continue;
+        }
+        out.push_str(&email_re.replace_all(line, "<redacted>"));
+    }
+    out.push_str(diff_body);
+    out
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
 
+    #[test]
+    fn test_anonymize_strips_trailers_and_preserves_diff_body() {
+        let input = "mm: fix the thing\n\
+\n\
+This rewrites the allocator to avoid a leak.\n\
+\n\
+Signed-off-by: Marc Zyngier <maz@kernel.org>\n\
+Reviewed-by: Will Deacon <will@kernel.org>\n\
+Cc: stable@vger.kernel.org\n\
+Fixes: 0123456789ab (\"mm: introduce the thing\")\n\
+---\n\
+ mm/foo.c | 2 +-\n\
+ 1 file changed, 1 insertion(+), 1 deletion(-)\n\
+\n\
+diff --git a/mm/foo.c b/mm/foo.c\n\
+index 1234..5678 100644\n\
+--- a/mm/foo.c\n\
++++ b/mm/foo.c\n\
+@@ -1,3 +1,3 @@\n\
+-\told(); /* Cc: looks like a trailer but is code */\n\
++\tnew(buf@host.example_stays_in_diff);\n";
+        let out = anonymize_patch_text(input);
+
+        // Identity trailers and the names/emails they carry are gone.
+        assert!(!out.contains("Signed-off-by"));
+        assert!(!out.contains("Reviewed-by"));
+        assert!(!out.contains("Marc Zyngier"));
+        assert!(!out.contains("Will Deacon"));
+        assert!(
+            !out.lines()
+                .any(|l| l.trim_start().to_ascii_lowercase().starts_with("cc:"))
+        );
+        // Fixes: is technical provenance, not an identity cue; it survives.
+        assert!(out.contains("Fixes: 0123456789ab"));
+        // Subject and prose survive.
+        assert!(out.contains("mm: fix the thing"));
+        assert!(out.contains("This rewrites the allocator"));
+        // The diff body is byte-for-byte unchanged, including its trailer-like
+        // and email-like text inside code.
+        let in_body = &input[input.find("diff --git").unwrap()..];
+        let out_body = &out[out.find("diff --git").unwrap()..];
+        assert_eq!(in_body, out_body);
+        assert!(out.contains("buf@host.example_stays_in_diff"));
+    }
+
+    #[test]
+    fn test_anonymize_redacts_emails_in_kept_prose() {
+        let input = "Reported by alice@example.com after testing.\n";
+        let out = anonymize_patch_text(input);
+        assert!(!out.contains("alice@example.com"));
+        assert!(out.contains("<redacted>"));
+        assert!(out.contains("Reported by"));
+        assert!(out.contains("after testing."));
+    }
+
+    #[test]
+    fn test_anonymize_handles_git_show_indented_trailers() {
+        let input = "commit 0123456789abcdef0123456789abcdef01234567\n\
+Author: Marc Zyngier <maz@kernel.org>\n\
+Date:   Mon May 1 00:00:00 2026 +0000\n\
+\n\
+    KVM: arm64: fix the thing\n\
+\n\
+    Body of the message.\n\
+\n\
+    Signed-off-by: Marc Zyngier <maz@kernel.org>\n\
+\n\
+diff --git a/x b/x\n\
+@@ -1 +1 @@\n\
+-a\n\
++b\n";
+        let out = anonymize_patch_text(input);
+        assert!(!out.contains("Author: Marc"));
+        assert!(!out.contains("maz@kernel.org"));
+        assert!(!out.contains("Signed-off-by"));
+        // The commit hash line, subject, and body survive.
+        assert!(out.contains("commit 0123456789abcdef"));
+        assert!(out.contains("KVM: arm64: fix the thing"));
+        assert!(out.contains("Body of the message."));
+        // Diff preserved.
+        assert!(out.contains("diff --git a/x b/x"));
+        assert!(out.contains("+b"));
+    }
+
+    #[test]
+    fn test_anonymize_message_without_diff() {
+        let input = "Subject line\n\nBody.\n\nAcked-by: Someone <s@e.org>\n";
+        let out = anonymize_patch_text(input);
+        assert!(!out.contains("Acked-by"));
+        assert!(!out.contains("s@e.org"));
+        assert!(out.contains("Subject line"));
+        assert!(out.contains("Body."));
+    }
+
     #[test]
     fn test_extract_email() {
         assert_eq!(

src/settings.rs

@@ -308,6 +308,15 @@ pub struct ReviewSettings {
     pub max_files_touched: usize,
     #[serde(default)]
     pub ignore_files: Vec<String>,
+    /// When true, strip author-reputation cues (identity trailers such as
+    /// Signed-off-by/Reviewed-by, the author header, and e-mail addresses)
+    /// from the commit-message region of the patch the model sees. The diff
+    /// body, e-mail routing, MAINTAINERS handling, the stored patch, and the
+    /// public report are all unaffected. Defaults to on, so the model judges a
+    /// patch on its merits and never sees author identity unless an operator
+    /// explicitly sets this to false.
+    #[serde(default = "default_anonymize_authors")]
+    pub anonymize_authors: bool,
     #[serde(default = "default_email_policy_path")]
     pub email_policy_path: String,
     /// Maximum cumulative non-cached tokens (uncached input + output) across all turns in a
@@ -357,6 +366,10 @@ fn default_email_policy_path() -> String {
     "email_policy.toml".to_string()
 }
 
+fn default_anonymize_authors() -> bool {
+    true
+}
+
 fn default_log_level() -> String {
     "info".to_string()
 }

src/worker/prompts.rs

@@ -318,7 +318,7 @@ You are the lead reviewer validating consolidated concerns. You will be given a
 2. CRITICAL RULE: To discard a concern as a false positive, you MUST find concrete proof that explicitly invalidates the concern's reasoning. If you cannot find definitive proof that the concern is a false positive, it must be reported as a finding. If you're not sure about something and it's critical in the reasoning validation, make it obvious: if X is possible, then problem Y can occur. Always try to validate if X is possible yourself.
 3. If context from subsequent patches in the series is provided, check if the concern is fixed later in the series. If so, discard it. But don't trust any promises in the commit message if they can't be verified (e.g. something will be fixed by subsequent patches in the series - if you can't prove that it's indeed fixed, report it as a bug).
 4. When referring to other patches within this series in your explanation, DO NOT use git hashes (they are ephemeral/unstable). Instead, refer to them by their patch subject (e.g., 'commit \"mm: fix allocation\"'). Existing historical commits in the tree should still be referenced by their standard hash.
-5. Assign a severity (low, medium, high, critical) to each remaining valid finding and explain the reasoning. Be rigorous in filtering out verifiable noise, but accurately report real logic flaws and edge cases.
+5. Assign a severity (low, medium, high, critical) to each remaining valid finding. Do not pick the label directly. First reason through the three axes defined in severity.md and state their values at the start of the finding's `severity_explanation` so the label is auditable: (a) Reachability: who can reach the buggy code (unprivileged, privileged, guest, internal, or unreachable). (b) Consequence: what happens if it triggers (crash, data-corruption, info-leak, perf, commit-msg-only, or style). (c) Confidence: how sure you are the bug is real and reachable (low, medium, or high, reserving high for a concrete reproducible path with every precondition named). Then collapse the three axes into the single label using severity.md's rules: an unreachable, commit-msg-only, or style finding is at most low however damaging it would be if reachable, and low confidence caps the label at medium. A finding is critical or high only when a damaging consequence, real reachability, and medium-or-better confidence all hold. Be rigorous in filtering out verifiable noise, but accurately report real logic flaws and edge cases.
 6. If the problem did exist in the code before the patch was applied, say it explicitly: 'This problem wasn't introduced by this patch, but...'. Discard low- and medium-severity pre-existing problems, report only high- and critical severity issues.
 7. SPECIFICITY REQUIREMENT: Every finding MUST cite the exact function name(s), file path(s), line number(s) when known, and triggering conditions where the bug manifests. Vague descriptions like 'potential overflow in ring buffer calculations' are insufficient. State precisely which variable overflows, in which function, and under what input conditions. Do not invent line numbers; use `line: null` when the exact line is not known.
 8. Carry forward the `locations` from the validated concern into each finding. If you gather better evidence, replace vague locations with the most precise file/function_or_symbol/line/code_snippet/why_this_location_matters locations you verified."

third_party/prompts/kernel/severity.md

@@ -6,6 +6,42 @@ critical issues must be critical, high issues must be very damaging.
 Use Medium as default and lower/raise depending on the "Question to ask" answer and examples.
 Use the following definitions and examples:
 
+## Calibrate the label through three axes
+
+Do not jump straight to a label. First reason through three independent
+axes, then collapse them into one of the four levels below. State all three
+axis values and the collapse in the finding's explanation so the label is
+auditable. Separating "how bad if true" from "how reachable" from "how sure"
+is what keeps the label honest: a finding that is `unprivileged` +
+`crash` + `high` confidence is genuinely Critical, while one that is
+`internal` + `crash` + `low` confidence is not.
+
+- **Reachability** (who can actually reach the buggy code): one of
+  `unprivileged`, `privileged`, `guest`, `internal`, or
+  `unreachable`. An attacker-reachable path raises severity, while a path no
+  one can trigger caps it.
+- **Consequence** (what happens if it triggers): one of `crash`,
+  `data-corruption`, `info-leak`, `perf`, `commit-msg-only`, or `style`.
+  This is the "how bad if true" axis on its own.
+- **Confidence** (how sure you are the bug is real and reachable): `low`,
+  `medium`, or `high`. Reserve `high` for a concrete reproducible path with
+  every precondition named. If you are speculating about whether the
+  triggering condition can occur, say so and lower confidence.
+
+Collapse the axes into the label, applying these rules in order:
+
+- An `unreachable` reachability or a `commit-msg-only`/`style` consequence is
+  at most Low, no matter how damaging the consequence would be if it were
+  reachable. A bug nobody can hit is not Critical.
+- `low` confidence caps the label at Medium. Still raise the finding, but do
+  not inflate an unproven hypothesis to High or Critical. If the uncertainty
+  is itself the point, state precisely what would confirm it.
+- Critical or High requires all three of: a damaging consequence (crash,
+  data-corruption, info-leak, or a security vulnerability), real
+  reachability (unprivileged, privileged, or guest), and `medium`-or-better confidence.
+- Otherwise pick Medium (recoverable, or a cold-path resource issue) or Low
+  per the definitions below.
+
 ## Critical
 - **Definition**: Issues that cause data loss, memory corruptions or security vulnerabilities.
 - **Question to ask**: Is it actually better for system to crash rather then keep working? If yes, it's a critical issue.