From 9ac966286f1c3e9822c4c02cc8d0de29adb8bef9 Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Thu, 2 Jul 2026 02:35:59 +0100 Subject: [PATCH 1/9] core+app: foundational shapes for phases B-E MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CiSummary moves to model (domain rollup); AgentMode::Review (advisory pre-pass, never advances the gate); FindingSeverity/ReviewFinding; ConversationKind/ConversationItem (read-only GitHub context, distinct from ephemeral Comments per §0.4) - Review gains ci_summary, review_findings, conversation, last_reviewed_sha (all serde-default; legacy state loads clean) - AgentPrompts.review override; Config.notify_poll_secs; REVIEW_INSTRUCTION - bindings regenerated; FE exhaustive switches extended Co-Authored-By: Claude Fable 5 --- app/src-tauri/src/commands/mod.rs | 8 +- app/src-tauri/src/lib.rs | 6 + app/src/App.tsx | 12 +- app/src/bindings/AgentMode.ts | 2 +- app/src/bindings/AgentPrompts.ts | 6 +- app/src/bindings/Config.ts | 8 +- app/src/bindings/ConversationItem.ts | 52 ++++++ app/src/bindings/ConversationKind.ts | 6 + app/src/bindings/FindingSeverity.ts | 6 + app/src/bindings/Review.ts | 28 ++- app/src/bindings/ReviewFinding.ts | 40 +++++ app/src/components/AgentEditor.tsx | 7 +- app/src/test/fixtures.ts | 2 + crates/cockpit-core/src/adapters/github.rs | 29 ++-- crates/cockpit-core/src/config.rs | 30 ++++ crates/cockpit-core/src/gate.rs | 4 + crates/cockpit-core/src/kickoff.rs | 12 ++ crates/cockpit-core/src/model.rs | 159 ++++++++++++++++++ crates/cockpit-core/src/persist.rs | 4 + crates/cockpit-core/src/prompt.rs | 13 ++ crates/cockpit-core/src/restack.rs | 4 + crates/cockpit-core/src/store.rs | 4 + .../cockpit-core/tests/batch_fan_out_e2e.rs | 4 + crates/cockpit-core/tests/e2e_round_trip.rs | 4 + crates/cockpit-core/tests/fix_loop_e2e.rs | 4 + 25 files changed, 426 insertions(+), 28 deletions(-) create mode 100644 app/src/bindings/ConversationItem.ts create mode 100644 app/src/bindings/ConversationKind.ts create mode 100644 app/src/bindings/FindingSeverity.ts create mode 100644 app/src/bindings/ReviewFinding.ts diff --git a/app/src-tauri/src/commands/mod.rs b/app/src-tauri/src/commands/mod.rs index 3325442..0859741 100644 --- a/app/src-tauri/src/commands/mod.rs +++ b/app/src-tauri/src/commands/mod.rs @@ -18,8 +18,8 @@ use cockpit_core::config::Config; use cockpit_core::gate::Gated; use cockpit_core::kickoff::{self, KickoffResult}; use cockpit_core::model::{ - AgentMode, Anchor, Artifact, Comment, CommentId, CommentOrigin, DiffData, DiffSide, GateState, - PlanDoc, PrRef, Project, ProjectId, ProjectPlan, ProjectRef, Review, ReviewSource, + AgentMode, Anchor, Artifact, CiSummary, Comment, CommentId, CommentOrigin, DiffData, DiffSide, + GateState, PlanDoc, PrRef, Project, ProjectId, ProjectPlan, ProjectRef, Review, ReviewSource, }; use cockpit_core::plan_parser; use cockpit_core::restack; @@ -647,13 +647,13 @@ pub async fn fetch_ci_checks( state: State<'_, Arc>, app_handle: tauri::AppHandle, pr: String, -) -> Result { +) -> Result { use tauri::Emitter; let pr_ref = PrRef::new(&pr); let repo_slug = state.reviews.get(&pr_ref).and_then(|r| r.repo_slug.clone()); - let empty = github::CiSummary { + let empty = CiSummary { passed: 0, total: 0, failed: 0, diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 85b2936..0e7a261 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -167,6 +167,12 @@ pub fn run() { } "completed" } + AgentMode::Review => { + // Advisory pre-pass reviewer finished. Findings + // ingest lands in W1; for now this is a no-op stub + // that keeps the match exhaustive. + "completed" + } }; // Best-effort: if no frontend window is listening, the diff --git a/app/src/App.tsx b/app/src/App.tsx index da54b7f..2148c42 100644 --- a/app/src/App.tsx +++ b/app/src/App.tsx @@ -112,6 +112,11 @@ function completionNotification( title: "Plan Rework Complete", body: "Plan agent finished reworking", }; + case "Review": + return { + title: "Pre-review Complete", + body: `Pre-review finished on ${branch}`, + }; default: return assertNever(mode); } @@ -119,7 +124,12 @@ function completionNotification( /** Whether a mode's completion is keyed by a review's PR ref (not a project). */ function isReviewMode(mode: AgentMode): boolean { - return mode === "Fix" || mode === "Restack" || mode === "Implement"; + return ( + mode === "Fix" || + mode === "Restack" || + mode === "Implement" || + mode === "Review" + ); } /** diff --git a/app/src/bindings/AgentMode.ts b/app/src/bindings/AgentMode.ts index 9db18e2..5399a80 100644 --- a/app/src/bindings/AgentMode.ts +++ b/app/src/bindings/AgentMode.ts @@ -3,4 +3,4 @@ /** * Which mode the spawned agent runs in. */ -export type AgentMode = "Plan" | "Implement" | "Fix" | "Restack"; +export type AgentMode = "Plan" | "Implement" | "Fix" | "Restack" | "Review"; diff --git a/app/src/bindings/AgentPrompts.ts b/app/src/bindings/AgentPrompts.ts index aafa16f..717f781 100644 --- a/app/src/bindings/AgentPrompts.ts +++ b/app/src/bindings/AgentPrompts.ts @@ -25,4 +25,8 @@ fix: string | null, /** * Override for [`AgentMode::Restack`]. */ -restack: string | null, }; +restack: string | null, +/** + * Override for [`AgentMode::Review`]. + */ +review: string | null, }; diff --git a/app/src/bindings/Config.ts b/app/src/bindings/Config.ts index 2e5d254..8cf589b 100644 --- a/app/src/bindings/Config.ts +++ b/app/src/bindings/Config.ts @@ -71,4 +71,10 @@ skills_github: SkillsGithub | null, * * Controls whether the bridge runs and which binaries back each language. */ -lsp_servers: LspServers, }; +lsp_servers: LspServers, +/** + * Seconds between background board polls for review-request changes. + * + * `None` or `0` disables background board polling. The UI default is 90. + */ +notify_poll_secs: number | null, }; diff --git a/app/src/bindings/ConversationItem.ts b/app/src/bindings/ConversationItem.ts new file mode 100644 index 0000000..94d1530 --- /dev/null +++ b/app/src/bindings/ConversationItem.ts @@ -0,0 +1,52 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { ConversationKind } from "./ConversationKind"; +import type { DiffSide } from "./DiffSide"; + +/** + * A read-only GitHub conversation item shown alongside a review for context. + * + * This is deliberately **not** a [`Comment`]: it is external context pulled + * from GitHub, never cleared by the gate on `Reworked`. That §0.4 distinction + * keeps cockpit's own comments ephemeral while GitHub threads persist. + */ +export type ConversationItem = { +/** + * GitHub identifier for this item. + */ +id: string, +/** + * Which kind of conversation item this is. + */ +kind: ConversationKind, +/** + * Login of the author. + */ +author: string, +/** + * The item's body text. + */ +body: string, +/** + * Path the item is anchored to, for inline review comments. + */ +path: string | null, +/** + * Line the item is anchored to, for inline review comments. + */ +line: number | null, +/** + * Which side of the diff the item is anchored to, when inline. + */ +side: DiffSide | null, +/** + * Review state (e.g. `"APPROVED"`), for review submissions. + */ +state: string | null, +/** + * ISO-8601 creation timestamp as reported by GitHub. + */ +created_at: string, +/** + * Permalink to the item on GitHub. + */ +url: string | null, }; diff --git a/app/src/bindings/ConversationKind.ts b/app/src/bindings/ConversationKind.ts new file mode 100644 index 0000000..043b00f --- /dev/null +++ b/app/src/bindings/ConversationKind.ts @@ -0,0 +1,6 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * The kind of GitHub conversation item mirrored into a review for context. + */ +export type ConversationKind = "Review" | "ReviewComment" | "IssueComment"; diff --git a/app/src/bindings/FindingSeverity.ts b/app/src/bindings/FindingSeverity.ts new file mode 100644 index 0000000..edd85fb --- /dev/null +++ b/app/src/bindings/FindingSeverity.ts @@ -0,0 +1,6 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * Severity of a [`ReviewFinding`] produced by the advisory pre-pass reviewer. + */ +export type FindingSeverity = "Info" | "Warning" | "Critical"; diff --git a/app/src/bindings/Review.ts b/app/src/bindings/Review.ts index 0847e4d..b899aaa 100644 --- a/app/src/bindings/Review.ts +++ b/app/src/bindings/Review.ts @@ -1,12 +1,15 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { AgentRun } from "./AgentRun"; +import type { CiSummary } from "./CiSummary"; import type { Comment } from "./Comment"; +import type { ConversationItem } from "./ConversationItem"; import type { DiffData } from "./DiffData"; import type { DispatchSnapshot } from "./DispatchSnapshot"; import type { GateState } from "./GateState"; import type { IssueRef } from "./IssueRef"; import type { PrRef } from "./PrRef"; import type { ProjectId } from "./ProjectId"; +import type { ReviewFinding } from "./ReviewFinding"; import type { ReviewId } from "./ReviewId"; import type { ReviewSource } from "./ReviewSource"; @@ -109,4 +112,27 @@ project: ProjectId | null, * Overwritten on each dispatch (see [`DispatchSnapshot`]). `None` before * the first dispatch and for legacy data. */ -dispatch_snapshot?: DispatchSnapshot, }; +dispatch_snapshot?: DispatchSnapshot, +/** + * Rolled-up CI status for this review's PR, when fetched. `None` until a + * CI check read populates it, and for legacy data that predates the field. + */ +ci_summary?: CiSummary, +/** + * Advisory findings from the read-only pre-pass reviewer, if it has run. + * + * Empty by default; never advances the gate (see [`ReviewFinding`]). + */ +review_findings: Array, +/** + * Read-only GitHub conversation context mirrored for this review. + * + * Empty by default; not [`Comment`]s and never cleared by the gate. + */ +conversation: Array, +/** + * The SHA the advisory pre-pass reviewer last reviewed, for staleness. + * + * `None` before the first pre-pass and for legacy data. + */ +last_reviewed_sha?: string, }; diff --git a/app/src/bindings/ReviewFinding.ts b/app/src/bindings/ReviewFinding.ts new file mode 100644 index 0000000..4e919a5 --- /dev/null +++ b/app/src/bindings/ReviewFinding.ts @@ -0,0 +1,40 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { DiffSide } from "./DiffSide"; +import type { FindingSeverity } from "./FindingSeverity"; + +/** + * A single advisory finding from the read-only pre-pass reviewer + * ([`AgentMode::Review`]). + * + * Findings annotate the diff for the human reviewer; they never advance the + * gate and are deliberately not [`Comment`]s. + */ +export type ReviewFinding = { +/** + * Stable identifier for this finding within a review cycle. + */ +id: string, +/** + * How serious the finding is. + */ +severity: FindingSeverity, +/** + * Path relative to the repo root the finding refers to. + */ +path: string, +/** + * Inclusive start and end line the finding covers. + */ +range: [number, number], +/** + * Which side of the diff the range refers to. + */ +side: DiffSide, +/** + * Short one-line summary of the finding. + */ +title: string, +/** + * Longer explanation of why this is a finding. + */ +rationale: string, }; diff --git a/app/src/components/AgentEditor.tsx b/app/src/components/AgentEditor.tsx index c5ea60a..587137a 100644 --- a/app/src/components/AgentEditor.tsx +++ b/app/src/components/AgentEditor.tsx @@ -19,7 +19,7 @@ function assertNever(x: never): never { } /** The agent modes, in display order, as an exhaustive literal table. */ -const AGENT_MODES = ["Implement", "Plan", "Fix", "Restack"] as const; +const AGENT_MODES = ["Implement", "Plan", "Fix", "Restack", "Review"] as const; /** Human-facing label for an agent mode. */ function modeLabel(mode: AgentMode): string { @@ -32,6 +32,8 @@ function modeLabel(mode: AgentMode): string { return "Fix"; case "Restack": return "Restack"; + case "Review": + return "Pre-review"; default: return assertNever(mode); } @@ -48,6 +50,8 @@ function modeDescription(mode: AgentMode): string { return "Applies your diff-gate comments and pushes the rework."; case "Restack": return "Rebases a descendant PR after its base branch changed."; + case "Review": + return "Runs a read-only pre-pass that annotates the diff with advisory findings."; default: return assertNever(mode); } @@ -60,6 +64,7 @@ function toAgentMode(value: unknown): AgentMode | null { case "Plan": case "Fix": case "Restack": + case "Review": return value; default: return null; diff --git a/app/src/test/fixtures.ts b/app/src/test/fixtures.ts index 4ede98b..114abf9 100644 --- a/app/src/test/fixtures.ts +++ b/app/src/test/fixtures.ts @@ -48,6 +48,8 @@ export function makeReview(overrides: Partial = {}): Review { agent: null, repo_slug: "o/r", project: null, + review_findings: [], + conversation: [], }; return { ...base, ...overrides }; } diff --git a/crates/cockpit-core/src/adapters/github.rs b/crates/cockpit-core/src/adapters/github.rs index 35054d6..8dd926c 100644 --- a/crates/cockpit-core/src/adapters/github.rs +++ b/crates/cockpit-core/src/adapters/github.rs @@ -9,7 +9,9 @@ use tokio::io::AsyncWriteExt; use tokio::process::Command; use ts_rs::TS; -use crate::model::{Anchor, Comment, CommentId, CommentOrigin, DiffSide, IssueRef, PrRef}; +use crate::model::{ + Anchor, CiSummary, Comment, CommentId, CommentOrigin, DiffSide, IssueRef, PrRef, +}; // --------------------------------------------------------------------------- // Errors @@ -105,23 +107,6 @@ pub struct CiCheck { pub workflow: String, } -/// Rollup of a set of [`CiCheck`]s into pass/fail/pending counts. -/// -/// Drives the diff-gate CI badge. Neutral and skipped checks count as passing: -/// they do not indicate a failure and should not block or alarm the reviewer. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, TS)] -#[ts(export, export_to = "../../../app/src/bindings/")] -pub struct CiSummary { - /// Checks that passed (includes neutral/skipped/cancelled). - pub passed: u32, - /// Total number of checks. - pub total: u32, - /// Checks that failed. - pub failed: u32, - /// Checks still pending (queued or in progress). - pub pending: u32, -} - /// Classification of a single check's outcome, derived from its bucket/state. /// /// Kept private: the public surface is [`CiSummary`] via [`summarize`]. @@ -817,6 +802,10 @@ pub fn build_review_from_pr( repo_slug: slug, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } @@ -2010,6 +1999,10 @@ index 1111111..2222222 100644 repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } diff --git a/crates/cockpit-core/src/config.rs b/crates/cockpit-core/src/config.rs index f99068b..3a72e66 100644 --- a/crates/cockpit-core/src/config.rs +++ b/crates/cockpit-core/src/config.rs @@ -71,6 +71,9 @@ pub struct AgentPrompts { /// Override for [`AgentMode::Restack`]. #[serde(default)] pub restack: Option, + /// Override for [`AgentMode::Review`]. + #[serde(default)] + pub review: Option, } impl AgentPrompts { @@ -85,6 +88,7 @@ impl AgentPrompts { AgentMode::Plan => self.plan.as_deref(), AgentMode::Fix => self.fix.as_deref(), AgentMode::Restack => self.restack.as_deref(), + AgentMode::Review => self.review.as_deref(), }; value.filter(|s| !s.trim().is_empty()) } @@ -100,6 +104,7 @@ impl AgentPrompts { AgentMode::Plan => self.plan = value, AgentMode::Fix => self.fix = value, AgentMode::Restack => self.restack = value, + AgentMode::Review => self.review = value, } } } @@ -306,6 +311,13 @@ pub struct Config { /// Controls whether the bridge runs and which binaries back each language. #[serde(default)] pub lsp_servers: LspServers, + + // ----- Notifications ----- + /// Seconds between background board polls for review-request changes. + /// + /// `None` or `0` disables background board polling. The UI default is 90. + #[serde(default)] + pub notify_poll_secs: Option, } /// Default agent command value for serde deserialization. @@ -338,6 +350,7 @@ impl Default for Config { agent_prompts: AgentPrompts::default(), skills_github: None, lsp_servers: LspServers::default(), + notify_poll_secs: None, } } } @@ -475,6 +488,7 @@ mod tests { assert!(config.ide_command.is_none()); assert!(config.app_theme.is_none()); assert!(config.editor_theme.is_none()); + assert!(config.notify_poll_secs.is_none()); } #[test] @@ -494,6 +508,7 @@ mod tests { plan: None, fix: Some("custom fix".into()), restack: None, + review: Some("custom review".into()), }, skills_github: Some(SkillsGithub { owner: "acme".into(), @@ -507,6 +522,7 @@ mod tests { pyright_command: Some("pyright-langserver".into()), typescript_command: None, }, + notify_poll_secs: Some(90), }; let serialized = toml::to_string_pretty(&config).expect("serialize should succeed"); @@ -531,8 +547,13 @@ mod tests { deserialized.agent_prompts.restack, config.agent_prompts.restack ); + assert_eq!( + deserialized.agent_prompts.review, + config.agent_prompts.review + ); assert_eq!(deserialized.skills_github, config.skills_github); assert_eq!(deserialized.lsp_servers, config.lsp_servers); + assert_eq!(deserialized.notify_poll_secs, config.notify_poll_secs); } #[test] @@ -563,6 +584,7 @@ mod tests { agent_prompts: AgentPrompts::default(), skills_github: None, lsp_servers: LspServers::default(), + notify_poll_secs: None, }; // Save to a specific path (test helper). @@ -629,8 +651,11 @@ hook_port = 19876 .is_none() ); assert!(config.agent_prompts.for_mode(AgentMode::Restack).is_none()); + assert!(config.agent_prompts.for_mode(AgentMode::Review).is_none()); // skills_github must default to None when absent (migration). assert!(config.skills_github.is_none()); + // notify_poll_secs must default to None when absent (migration). + assert!(config.notify_poll_secs.is_none()); } #[test] @@ -695,11 +720,13 @@ path = "skills" plan: None, fix: Some("fix it".into()), restack: None, + review: Some("review it".into()), }; assert_eq!(prompts.for_mode(AgentMode::Implement), Some("build it")); assert_eq!(prompts.for_mode(AgentMode::Fix), Some("fix it")); assert_eq!(prompts.for_mode(AgentMode::Plan), None); assert_eq!(prompts.for_mode(AgentMode::Restack), None); + assert_eq!(prompts.for_mode(AgentMode::Review), Some("review it")); } #[test] @@ -709,6 +736,7 @@ path = "skills" plan: Some(String::new()), fix: None, restack: None, + review: None, }; // Whitespace-only and empty overrides fall back to builtin (None). assert_eq!(prompts.for_mode(AgentMode::Implement), None); @@ -888,6 +916,7 @@ hook_port = 19876 plan: Some("plan preamble".into()), fix: None, restack: Some("restack preamble".into()), + review: Some("review preamble".into()), }; let serialized = toml::to_string_pretty(&prompts).expect("serialize"); let deserialized: AgentPrompts = toml::from_str(&serialized).expect("deserialize"); @@ -895,5 +924,6 @@ hook_port = 19876 assert_eq!(deserialized.plan, prompts.plan); assert_eq!(deserialized.fix, prompts.fix); assert_eq!(deserialized.restack, prompts.restack); + assert_eq!(deserialized.review, prompts.review); } } diff --git a/crates/cockpit-core/src/gate.rs b/crates/cockpit-core/src/gate.rs index 3f0ef49..f79a9e0 100644 --- a/crates/cockpit-core/src/gate.rs +++ b/crates/cockpit-core/src/gate.rs @@ -335,6 +335,10 @@ mod tests { repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } diff --git a/crates/cockpit-core/src/kickoff.rs b/crates/cockpit-core/src/kickoff.rs index e5b578d..81e451b 100644 --- a/crates/cockpit-core/src/kickoff.rs +++ b/crates/cockpit-core/src/kickoff.rs @@ -198,6 +198,10 @@ fn build_review( repo_slug: None, project: project.cloned(), dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } @@ -779,6 +783,10 @@ mod tests { repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, }]; wire_children(&mut reviews); @@ -809,6 +817,10 @@ mod tests { repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, }; let prompt = assemble_implement_prompt(&review, &ProjectRef::new("proj-1"), None, None); diff --git a/crates/cockpit-core/src/model.rs b/crates/cockpit-core/src/model.rs index b102f5c..c9a8b9a 100644 --- a/crates/cockpit-core/src/model.rs +++ b/crates/cockpit-core/src/model.rs @@ -112,6 +112,9 @@ pub enum AgentMode { Fix, /// Rebase / resolve conflicts after a parent branch changed. Restack, + /// Read-only advisory pre-pass reviewer: annotates the diff with findings + /// and never advances the gate. + Review, } /// Where a comment originated. @@ -167,6 +170,30 @@ pub enum DiffSide { New, } +/// Severity of a [`ReviewFinding`] produced by the advisory pre-pass reviewer. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub enum FindingSeverity { + /// Informational note; no action required. + Info, + /// Worth a look, but not blocking. + Warning, + /// A serious issue the reviewer should address. + Critical, +} + +/// The kind of GitHub conversation item mirrored into a review for context. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub enum ConversationKind { + /// A PR review submission (approve / request-changes / comment). + Review, + /// An inline review comment anchored to a diff line. + ReviewComment, + /// A top-level issue-style comment on the PR. + IssueComment, +} + /// A location inside the current artifact that a [`Comment`] points to. /// /// Anchors are ephemeral — they reference the *current* artifact version only @@ -345,6 +372,80 @@ pub struct Project { pub plan: Option, } +/// Rollup of a set of [`CiCheck`]s into pass/fail/pending counts. +/// +/// Drives the diff-gate CI badge. Neutral and skipped checks count as passing: +/// they do not indicate a failure and should not block or alarm the reviewer. +// +// Lives here (not in the github adapter) as a pure domain rollup; the adapter's +// `summarize()` returns this type and `CiCheck` re-exports from `crate::model`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct CiSummary { + /// Checks that passed (includes neutral/skipped/cancelled). + pub passed: u32, + /// Total number of checks. + pub total: u32, + /// Checks that failed. + pub failed: u32, + /// Checks still pending (queued or in progress). + pub pending: u32, +} + +/// A single advisory finding from the read-only pre-pass reviewer +/// ([`AgentMode::Review`]). +/// +/// Findings annotate the diff for the human reviewer; they never advance the +/// gate and are deliberately not [`Comment`]s. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct ReviewFinding { + /// Stable identifier for this finding within a review cycle. + pub id: String, + /// How serious the finding is. + pub severity: FindingSeverity, + /// Path relative to the repo root the finding refers to. + pub path: PathBuf, + /// Inclusive start and end line the finding covers. + pub range: (u32, u32), + /// Which side of the diff the range refers to. + pub side: DiffSide, + /// Short one-line summary of the finding. + pub title: String, + /// Longer explanation of why this is a finding. + pub rationale: String, +} + +/// A read-only GitHub conversation item shown alongside a review for context. +/// +/// This is deliberately **not** a [`Comment`]: it is external context pulled +/// from GitHub, never cleared by the gate on `Reworked`. That §0.4 distinction +/// keeps cockpit's own comments ephemeral while GitHub threads persist. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct ConversationItem { + /// GitHub identifier for this item. + pub id: String, + /// Which kind of conversation item this is. + pub kind: ConversationKind, + /// Login of the author. + pub author: String, + /// The item's body text. + pub body: String, + /// Path the item is anchored to, for inline review comments. + pub path: Option, + /// Line the item is anchored to, for inline review comments. + pub line: Option, + /// Which side of the diff the item is anchored to, when inline. + pub side: Option, + /// Review state (e.g. `"APPROVED"`), for review submissions. + pub state: Option, + /// ISO-8601 creation timestamp as reported by GitHub. + pub created_at: String, + /// Permalink to the item on GitHub. + pub url: Option, +} + /// A single PR under review at the diff gate. /// /// Reviews form a DAG via `parents` / `children`, mirroring the Linear issue @@ -407,6 +508,27 @@ pub struct Review { #[serde(default)] #[ts(optional)] pub dispatch_snapshot: Option, + /// Rolled-up CI status for this review's PR, when fetched. `None` until a + /// CI check read populates it, and for legacy data that predates the field. + #[serde(default)] + #[ts(optional)] + pub ci_summary: Option, + /// Advisory findings from the read-only pre-pass reviewer, if it has run. + /// + /// Empty by default; never advances the gate (see [`ReviewFinding`]). + #[serde(default)] + pub review_findings: Vec, + /// Read-only GitHub conversation context mirrored for this review. + /// + /// Empty by default; not [`Comment`]s and never cleared by the gate. + #[serde(default)] + pub conversation: Vec, + /// The SHA the advisory pre-pass reviewer last reviewed, for staleness. + /// + /// `None` before the first pre-pass and for legacy data. + #[serde(default)] + #[ts(optional)] + pub last_reviewed_sha: Option, } // --------------------------------------------------------------------------- @@ -441,6 +563,10 @@ mod tests { repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } @@ -589,6 +715,39 @@ mod tests { assert_eq!(review.dispatch_snapshot, None); } + #[test] + fn review_deserializes_without_new_context_fields() { + // A Review payload predating `ci_summary`, `review_findings`, + // `conversation`, and `last_reviewed_sha` must load with those fields + // defaulted rather than erroring (`#[serde(default)]`). + let json = r#"{ + "id": "r-1", + "issue": "ISSUE-1", + "pr": "owner/repo#1", + "branch": "alejandro/legacy", + "base": "main", + "base_sha": "000", + "source": "Frontier", + "worktree": "/tmp/wt", + "gate_state": "Pending", + "diff": { "raw": "" }, + "head_sha": "abc", + "comments": [], + "parents": [], + "children": [], + "stale": false, + "agent": null, + "repo_slug": null, + "project": null + }"#; + + let review: Review = serde_json::from_str(json).expect("legacy Review should deserialize"); + assert_eq!(review.ci_summary, None); + assert!(review.review_findings.is_empty()); + assert!(review.conversation.is_empty()); + assert_eq!(review.last_reviewed_sha, None); + } + #[test] fn diff_line_anchor_deserializes_without_side() { // Anchors persisted before the `side` field default to `New`. diff --git a/crates/cockpit-core/src/persist.rs b/crates/cockpit-core/src/persist.rs index ea943d3..dd6cf8c 100644 --- a/crates/cockpit-core/src/persist.rs +++ b/crates/cockpit-core/src/persist.rs @@ -149,6 +149,10 @@ mod tests { repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } diff --git a/crates/cockpit-core/src/prompt.rs b/crates/cockpit-core/src/prompt.rs index b0b76ac..f02e280 100644 --- a/crates/cockpit-core/src/prompt.rs +++ b/crates/cockpit-core/src/prompt.rs @@ -41,6 +41,7 @@ pub fn builtin_intent(mode: AgentMode) -> &'static str { AgentMode::Implement => IMPLEMENT_INSTRUCTION, AgentMode::Fix => SCOPE_GUARD, AgentMode::Restack => RESTACK_INSTRUCTION, + AgentMode::Review => REVIEW_INSTRUCTION, } } @@ -62,6 +63,17 @@ Preserve the intent of both sides. \ Don't refactor unrelated code and don't weaken or delete tests. \ If a conflict is ambiguous, stop and say so."; +/// The builtin advisory-reviewer instruction, verbatim. +/// +/// Guides the read-only pre-pass reviewer ([`AgentMode::Review`]): it inspects +/// the diff against the intent and emits findings only — it never edits code +/// and never advances the gate. +const REVIEW_INSTRUCTION: &str = "\ +Review the diff above against the stated intent. \ +Output ONLY a JSON array of findings to the output path, in this shape: \ +[{\"severity\":\"Info|Warning|Critical\",\"path\":\"...\",\"line_start\":N,\"line_end\":N,\"side\":\"Old|New\",\"title\":\"...\",\"rationale\":\"...\"}]. \ +Do NOT edit any code; this pass is advisory only."; + /// Input bundle for rework prompt assembly. /// /// Collects references to the data needed to build a rework prompt, @@ -976,6 +988,7 @@ mod tests { AgentMode::Implement, AgentMode::Fix, AgentMode::Restack, + AgentMode::Review, ] { assert!( !builtin_intent(mode).is_empty(), diff --git a/crates/cockpit-core/src/restack.rs b/crates/cockpit-core/src/restack.rs index 08538c6..ac63d2a 100644 --- a/crates/cockpit-core/src/restack.rs +++ b/crates/cockpit-core/src/restack.rs @@ -317,6 +317,10 @@ mod tests { repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } diff --git a/crates/cockpit-core/src/store.rs b/crates/cockpit-core/src/store.rs index 4b74025..bff72fc 100644 --- a/crates/cockpit-core/src/store.rs +++ b/crates/cockpit-core/src/store.rs @@ -416,6 +416,10 @@ mod tests { repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } diff --git a/crates/cockpit-core/tests/batch_fan_out_e2e.rs b/crates/cockpit-core/tests/batch_fan_out_e2e.rs index fe45d42..fb6a60e 100644 --- a/crates/cockpit-core/tests/batch_fan_out_e2e.rs +++ b/crates/cockpit-core/tests/batch_fan_out_e2e.rs @@ -141,6 +141,10 @@ fn make_review(id: &str, branch: &str, worktree: PathBuf, project: &ProjectId) - repo_slug: None, project: Some(project.clone()), dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } diff --git a/crates/cockpit-core/tests/e2e_round_trip.rs b/crates/cockpit-core/tests/e2e_round_trip.rs index e6b503d..f04bf6d 100644 --- a/crates/cockpit-core/tests/e2e_round_trip.rs +++ b/crates/cockpit-core/tests/e2e_round_trip.rs @@ -42,6 +42,10 @@ fn make_test_review(worktree: PathBuf) -> Review { repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } diff --git a/crates/cockpit-core/tests/fix_loop_e2e.rs b/crates/cockpit-core/tests/fix_loop_e2e.rs index 0008034..45c72d8 100644 --- a/crates/cockpit-core/tests/fix_loop_e2e.rs +++ b/crates/cockpit-core/tests/fix_loop_e2e.rs @@ -171,6 +171,10 @@ fn make_review(worktree: PathBuf, branch: &str, head_sha: &str) -> Review { repo_slug: None, project: None, dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, } } From 2d629b02c0e4a6f16dafaf8eacdd1026bac3e39f Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Thu, 2 Jul 2026 02:36:11 +0100 Subject: [PATCH 2/9] core: stub diff_signals and findings modules for parallel wave Co-Authored-By: Claude Fable 5 --- crates/cockpit-core/src/diff_signals.rs | 2 ++ crates/cockpit-core/src/findings.rs | 2 ++ crates/cockpit-core/src/lib.rs | 2 ++ 3 files changed, 6 insertions(+) create mode 100644 crates/cockpit-core/src/diff_signals.rs create mode 100644 crates/cockpit-core/src/findings.rs diff --git a/crates/cockpit-core/src/diff_signals.rs b/crates/cockpit-core/src/diff_signals.rs new file mode 100644 index 0000000..7b26379 --- /dev/null +++ b/crates/cockpit-core/src/diff_signals.rs @@ -0,0 +1,2 @@ +//! Deterministic diff-derived review signals (size, risk paths, test +//! delta, weakening flags). Pure; populated in the next commit. diff --git a/crates/cockpit-core/src/findings.rs b/crates/cockpit-core/src/findings.rs new file mode 100644 index 0000000..cefb7b3 --- /dev/null +++ b/crates/cockpit-core/src/findings.rs @@ -0,0 +1,2 @@ +//! Advisory reviewer-agent findings: JSON contract parser. Pure; +//! populated in the next commit. diff --git a/crates/cockpit-core/src/lib.rs b/crates/cockpit-core/src/lib.rs index 0156c36..f6ec06b 100644 --- a/crates/cockpit-core/src/lib.rs +++ b/crates/cockpit-core/src/lib.rs @@ -15,6 +15,8 @@ pub mod model; pub mod adapters; pub mod dag; +pub mod diff_signals; +pub mod findings; pub mod gate; pub mod kickoff; pub mod persist; From 9c67b7817a372db499ecce89650a505c1e0ef328 Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Thu, 2 Jul 2026 02:41:51 +0100 Subject: [PATCH 3/9] docs: interaction-patterns research synthesis (round 2) + roadmap Phase F Verified competitor teardowns (Graphite taxonomy/downvote/rules model, CodeRabbit price band + review/rework collapse, the ~58% critical-bug ceiling), HCI evidence for the gated loop (four oversight surfaces, 2.1-steps-per-prompt chunking, zero-of-99 autonomy trust), the trust-miscalibration hazard binding our evidence/trajectory UI, and Cisco sizing mechanics. Locks 10 design decisions; adds roadmap Phase F. Co-Authored-By: Claude Fable 5 --- cockpit-docs/RESEARCH_INTERACTION_PATTERNS.md | 123 ++++++++++++++++++ cockpit-docs/ROADMAP.md | 24 ++++ 2 files changed, 147 insertions(+) create mode 100644 cockpit-docs/RESEARCH_INTERACTION_PATTERNS.md diff --git a/cockpit-docs/RESEARCH_INTERACTION_PATTERNS.md b/cockpit-docs/RESEARCH_INTERACTION_PATTERNS.md new file mode 100644 index 0000000..017641b --- /dev/null +++ b/cockpit-docs/RESEARCH_INTERACTION_PATTERNS.md @@ -0,0 +1,123 @@ +# Human-AI review interaction — verified research synthesis (round 2) + +*Compiled 2026-07-02 from a second deep-research pass (107 agents, 3-vote adversarial +verification per claim; competitor claims anchored to primary vendor sources because the +first pass's landscape findings failed verification). Companion to +[`RESEARCH_REVIEW_BOTTLENECK.md`](./RESEARCH_REVIEW_BOTTLENECK.md); drives the interaction +design decisions listed at the end and the roadmap amendments in [`ROADMAP.md`](./ROADMAP.md).* + +--- + +## 1. The competitive field — convergence, and the gap cockpit occupies + +- **Graphite (Diamond → "Graphite Agent")**: positions on *near-instant review latency* + ("every PR in seconds, not hours") and a *published noise metric* — per-comment downvotes + with a claimed <5% negative rate (dashboard example 3.5%). Findings are inline comments + categorized by a **seven-type taxonomy** (logic bug, edge case, security, accidentally + committed code, performance, quality/style, docs), most with one-click committable fixes; + steering is **org-level natural-language rules** (+ OWASP/Airbnb/Google/PEP templates), + not per-review configuration. [graphite.com/features/ai-reviews](https://graphite.com/features/ai-reviews) + — confidence: high (first-party; noise metric partly reflects low volume, ~0.62 comments/PR). +- **CodeRabbit**: the price band is **$24–48/user/mo** (free tier exists; local IDE/CLI + review is free but rate-limited). Its paid tiers make the reviewer *perform rework + itself* — one-click autofixes, docstrings, generated unit tests, merge-conflict + resolution, committed as follow-up PRs. + [coderabbit.ai/pricing](https://www.coderabbit.ai/pricing) — confidence: high/medium. +- **The ceiling argument (use this, never vendor rankings):** even in Greptile's own + vendor-favorable benchmark, the best AI reviewer caught **58% of critical bugs** — ~4 in + 10 missed by the *winner*; independent re-runs (Martian: best tool 53.5% recall; Augment; + Macroscope: 48%) land in the same band. No benchmark anywhere shows an AI reviewer near + 100% on critical bugs. [greptile.com/benchmarks](https://www.greptile.com/benchmarks) — + confidence: medium (vendor-run), directionally corroborated 3×. +- **Devin's MultiDevin** ships hierarchical *mission control for building*: one coordinator + scoping, delegating to ≤10 worker sessions, monitoring, merging. + [cognition.ai blog](https://cognition.ai/blog/devin-can-now-manage-devins) — confidence: high. + (Refuted 1-2: claims about steering child agents mid-task by message — do not cite.) + +**Strategic read:** the market is converging on always-on inline AI review with one-click +fixes — *collapsing the review/rework boundary*. Nobody verified occupies (a) an explicit +**human-gated review→rework loop**, or (b) **batch review mission control** — reviewing +fleets of agent PRs the way Devin builds with fleets. That is cockpit's lane, now with a +quantitative justification: AI review alone misses ~40%+ of critical bugs. + +## 2. HCI evidence — the gated architecture is how experts already work + +- **Four oversight surfaces** (interview study, n=17, FAccT 2026): a-priori control, + co-planning, real-time monitoring, post-hoc review — oversight is *preventative and + proactive*, not only after-the-fact. These map one-to-one onto cockpit: config/skills → + plan gate → agent timeline → diff gate. A tool offering only post-hoc diff review covers + one of four modes. [arXiv:2606.05391](https://arxiv.org/abs/2606.05391) — medium (n=17). +- **Experts control through plans, in small chunks** ("Professional Software Developers + Don't Vibe, They Control", UCSD+Cornell, Dec 2025): all 11 feature-building participants + used an explicit plan step — but **9 of 11 authored the plan themselves** (only 2 + approve-agent-drafts), and even 70-step plans were handed to the agent **~2.1 steps at a + time** (max 5–6), verifying between chunks. + [arXiv:2512.14012](https://arxiv.org/abs/2512.14012) — medium. + → Cockpit's plan gate must support **editing/authoring**, not just approve/reject; and + chunked checkpoints beat one monolithic approval. +- **Zero of 99 surveyed developers** consider agents safe for full autonomy; modification + rate of agent code ≈ half the time; dominant stance: "always reading the output and + steering." → Optimize the read-and-steer path, not an approve-all path. +- **Verification-first already happens in the wild** — developers treat passing tests as a + correctness proxy ("we don't even need to look at the source folder anymore"). Evidence of + *adoption*, not *safety*: surface test/check results as first-class signals, but never let + green silently substitute for the gate. + +## 3. The trust-miscalibration hazard (read before building any trust UI) + +Microsoft Research (3 user studies, Feb 2026): **raw step-by-step agent traces are +cumbersome and ineffective** for verifying agent work — and a *better-designed* oversight +interface cut error-finding time (g −0.65) while **inflating reviewer confidence without +improving accuracy** (g 0.18); confidence rose most precisely when errors were missed +(g 0.85). [arXiv:2602.16844](https://arxiv.org/abs/2602.16844) — medium (n=12, CUA domain). + +Implications for cockpit, binding until better evidence exists: +1. Never present the raw trajectory dump as the transparency mechanism (D2's compact + summary is the right call; the raw log stays one click away, not the default). +2. Any confidence/provenance display must be judged by **detection accuracy**, not by how + confident or fast it makes the reviewer feel. "Faster and more confident" is the + *failure mode* unless accuracy holds. +3. Guided reading aids should order attention **without pre-supplying the verdict** + (see priming, below). + +## 4. Code-reading science — concrete sizing mechanics + +Cisco/SmartBear study (2,500 reviews, 3.2M LOC, 2006 — old but the only verified sizing +data; medium confidence, correlational): +- Defect detection falls off sharply **above ~200 LOC per review** (guideline: under 200, + never above 400) — cockpit's small-stacked-PR dogfooding rule, now with a number. +- Reading **faster than ~450–500 LOC/hour** degrades detection (recommended <300/hr). +- Author **pre-annotation** correlates with near-zero defect density — attributed to forced + self-review, with a live rival explanation: **annotations prime reviewers and dull + criticism**. Agent-generated walkthroughs must guide *order*, not conclusions. +- Refuted 1-2: the popular "60–90 minute session ceiling" — do not build timer mechanics on it. + +## 5. Design decisions this research locks in (with the phase that owns each) + +| # | Decision | Basis | Owner | +|---|----------|-------|-------| +| 1 | AI findings are a **triage layer, never a verdict**; advisory pins, dismissible | §1 ceiling (≤58% critical catch) | Phase B2 (shipping) | +| 2 | Findings get a **category taxonomy chip** + **per-finding downvote** feeding a local noise metric | Graphite's field-tested mechanics | Phase F (new) | +| 3 | Steering is **amortized**: org/repo-level natural-language review rules fed into the pre-pass prompt, not per-review config | Graphite rules model; skills system already exists | Phase F (new) | +| 4 | Plan gate gains **direct plan editing/authoring** (human-authored is the 9/11 expert mode) | §2 | Phase F (new) | +| 5 | Plans execute in **chunked checkpoints** (~small step batches), not monolithic fan-out | §2 (2.1 steps/prompt) | Phase F (candidate, needs design) | +| 6 | Trajectory transparency = **compact summary default, raw log one click away** | §3 | Phase D2 (as designed) | +| 7 | Evidence strip stays **deterministic signals**; no model-generated confidence scores without an accuracy-validated design | §3 | Phase B1 (as designed) | +| 8 | Guided reading order (risk-sorted file tree) **without verdict annotations** | §4 priming | Phase F (new) | +| 9 | Size discipline surfaced: warn when a single review unit exceeds ~400 changed LOC ("consider splitting") | §4 | Phase C (fold into size chips) | +| 10 | Batch **review mission control** is the positioning: the coordinator view over N gated reviews — the unoccupied niche | §1 | Product framing | + +## Refuted / unusable claims (transparency) + +1. Devin child-agent steer-by-message/pause/terminate mechanics — refuted 1-2. +2. The 60–90-minute review-session ceiling — refuted 1-2. +3. Vendor benchmark *rankings* (Greptile 82% vs Graphite 6% etc.) — contradicted by + independent re-runs; only the ceiling argument survives. + +## Under-researched (verified-empty, not settled) + +Copilot code review / Cursor BugBot / Baz / Ellipsis / CodeAnt / Conductor / Terragon / +Sourcegraph Amp UX specifics; modern (2024–26) diff-presentation evidence (side-by-side vs +interleaved, semantic/AST diffs, reading tours); voice/spec-driven review; whether the +confidence-inflation hazard replicates on code diffs specifically. These produced no +verified claims — treat as open questions, not absence of activity. diff --git a/cockpit-docs/ROADMAP.md b/cockpit-docs/ROADMAP.md index bfc17c5..232d8d2 100644 --- a/cockpit-docs/ROADMAP.md +++ b/cockpit-docs/ROADMAP.md @@ -107,6 +107,30 @@ in A9). Next: - **E3. Linear description in intent** — kickoff reviews currently carry only the issue title; fetch the description for the intent panel and prompts. +## Phase F — interaction refinements (from research round 2) + +Locked in by [`RESEARCH_INTERACTION_PATTERNS.md`](./RESEARCH_INTERACTION_PATTERNS.md) +(2026-07-02). Not scheduled yet; sequenced after B–E land. + +- **F1. Finding taxonomy + downvote** — category chips on advisory findings (logic bug / + edge case / security / performance / …) and a per-finding thumbs-down feeding a local + noise metric (Graphite's field-tested trust mechanic). Extends `ReviewFinding`. +- **F2. Amortized steering rules** — org/repo-level natural-language review rules + (a `review-rules.md` alongside skills) injected into the pre-pass prompt; steering + configured once, not per review. +- **F3. Plan editing** — the plan gate gains direct authoring/editing (9/11 experts author + plans themselves; approve/reject-only serves the minority mode). +- **F4. Chunked plan checkpoints** — experts hand agents ~2 steps at a time; explore + splitting batch builds into gated step-chunks rather than one fan-out (needs design; must + not fork the Gated loop). +- **F5. Guided reading order** — risk-sorted file tree (signals-driven) that orders + attention without pre-supplying verdicts (priming hazard). +- **F6. Size discipline nudge** — ">400 changed LOC — consider splitting" on cards/evidence + strip (Cisco sizing data). +- **Binding constraint from the trust-miscalibration study**: no confidence scores or + polished provenance displays unless validated against detection *accuracy* — faster+more + confident with unchanged accuracy is the failure mode. + ## Deliberate non-goals (guardrails, restated) - **No auto-approve, no auto-merge, no batch-approve.** Every terminal action stays behind From d5f201ba87f6c917c0c92d0299a720550a1c475d Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Thu, 2 Jul 2026 02:42:04 +0100 Subject: [PATCH 4/9] core: file-at-rev adapters for full-file diff context git2 blob reads at a revision (binary/oversize/missing-path -> None, bad rev -> Err) and a gh api contents fallback with pure, tested response parsing. 512 KiB cap guards Monaco full-file rendering. Co-Authored-By: Claude Fable 5 --- Cargo.lock | 1 + crates/cockpit-core/Cargo.toml | 1 + crates/cockpit-core/src/adapters/git.rs | 103 +++++++++++ crates/cockpit-core/src/adapters/github.rs | 202 +++++++++++++++++++++ 4 files changed, 307 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index ac5cf3d..78fe9e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -572,6 +572,7 @@ name = "cockpit-core" version = "0.0.0" dependencies = [ "axum", + "base64 0.22.1", "dirs", "futures-util", "git2", diff --git a/crates/cockpit-core/Cargo.toml b/crates/cockpit-core/Cargo.toml index b116440..1bf922f 100644 --- a/crates/cockpit-core/Cargo.toml +++ b/crates/cockpit-core/Cargo.toml @@ -19,6 +19,7 @@ ts-rs.workspace = true toml = "1.1.2" dirs = "6.0.0" futures-util = { version = "0.3", default-features = false, features = ["sink"] } +base64 = "0.22" [dev-dependencies] tempfile.workspace = true diff --git a/crates/cockpit-core/src/adapters/git.rs b/crates/cockpit-core/src/adapters/git.rs index aeac802..c0a9f3c 100644 --- a/crates/cockpit-core/src/adapters/git.rs +++ b/crates/cockpit-core/src/adapters/git.rs @@ -193,6 +193,60 @@ pub fn diff_range(worktree: &Path, from: &str, to: &str) -> Result Result, Error> { + let repo = Repository::open(repo_dir)?; + let tree = repo.revparse_single(rev)?.peel_to_tree()?; + + let entry = match tree.get_path(Path::new(path)) { + Ok(entry) => entry, + // A missing path is a normal outcome (added/deleted file), not an error. + Err(e) if e.code() == git2::ErrorCode::NotFound => return Ok(None), + Err(e) => return Err(Error::Git2(e)), + }; + + let object = entry.to_object(&repo)?; + // The entry may resolve to a subtree or submodule rather than a file. + let Some(blob) = object.as_blob() else { + return Ok(None); + }; + + let content = blob.content(); + if content.len() > MAX_FULL_FILE_BYTES { + return Ok(None); + } + // Binary blobs are not rendered: the full-file view is text-only. A NUL byte + // is valid UTF-8, so it is checked separately from the UTF-8 decode below. + if content.contains(&0) { + return Ok(None); + } + match std::str::from_utf8(content) { + Ok(text) => Ok(Some(text.to_string())), + Err(_) => Ok(None), + } +} + /// Remove a worktree and clean up its directory. /// /// Accepts the branch name and flattens it with [`worktree_name`] to match @@ -858,6 +912,55 @@ mod tests { ); } + #[test] + fn file_at_rev_reads_content_across_revisions() { + let (repo, dir) = scratch_repo_with_file(); + // scratch_repo_with_file seeds base.txt with "line 1\n" on the initial + // commit. Add a second revision that changes the same file. + commit_file(&repo, "base.txt", b"line two\n", "edit base"); + + let at_head = file_at_rev(dir.path(), "HEAD", "base.txt").unwrap(); + assert_eq!(at_head.as_deref(), Some("line two\n"), "HEAD content"); + + let at_prev = file_at_rev(dir.path(), "HEAD~1", "base.txt").unwrap(); + assert_eq!(at_prev.as_deref(), Some("line 1\n"), "HEAD~1 content"); + } + + #[test] + fn file_at_rev_absent_path_is_none() { + let (_repo, dir) = scratch_repo_with_file(); + let result = file_at_rev(dir.path(), "HEAD", "does-not-exist.txt").unwrap(); + assert_eq!(result, None, "an absent path yields Ok(None), not an error"); + } + + #[test] + fn file_at_rev_bad_rev_errors() { + let (_repo, dir) = scratch_repo_with_file(); + let result = file_at_rev(dir.path(), "no-such-rev", "base.txt"); + assert!( + matches!(result, Err(Error::Git2(_))), + "a bad revision must error, not return None, got {result:?}" + ); + } + + #[test] + fn file_at_rev_binary_blob_is_none() { + let (repo, dir) = scratch_repo_with_file(); + // A blob with an embedded NUL is binary; the text-only view returns None. + commit_file(&repo, "bin.dat", b"abc\x00def\n", "add binary blob"); + let result = file_at_rev(dir.path(), "HEAD", "bin.dat").unwrap(); + assert_eq!(result, None, "binary blob should return None"); + } + + #[test] + fn file_at_rev_oversize_blob_is_none() { + let (repo, dir) = scratch_repo_with_file(); + let big = vec![b'x'; MAX_FULL_FILE_BYTES + 1]; + commit_file(&repo, "big.txt", &big, "add oversize blob"); + let result = file_at_rev(dir.path(), "HEAD", "big.txt").unwrap(); + assert_eq!(result, None, "blob over the cap should return None"); + } + #[test] fn prune_worktree_removes_worktree() { let (repo, dir) = scratch_repo(); diff --git a/crates/cockpit-core/src/adapters/github.rs b/crates/cockpit-core/src/adapters/github.rs index 8dd926c..b0c8e7a 100644 --- a/crates/cockpit-core/src/adapters/github.rs +++ b/crates/cockpit-core/src/adapters/github.rs @@ -4,6 +4,7 @@ //! identifier in generated branch names (e.g. `alejandro/nex-123-add-feature`), //! so cockpit links PR to issue by parsing the head branch. See `SPEC.md` S16. +use base64::Engine as _; use serde::{Deserialize, Serialize}; use tokio::io::AsyncWriteExt; use tokio::process::Command; @@ -1063,6 +1064,114 @@ pub async fn compare(repo_slug: &str, base_sha: &str, head_sha: &str) -> Result< Ok(String::from_utf8_lossy(&output.stdout).into_owned()) } +// --------------------------------------------------------------------------- +// Contents API (full-file fallback) +// --------------------------------------------------------------------------- + +/// Maximum file size served by [`contents_at`], in bytes. +/// +/// Mirrors [`crate::adapters::git::MAX_FULL_FILE_BYTES`]: the full-file view +/// feeds Monaco, so a blob past this cap yields `Ok(None)` and the UI stays on +/// the diff view. GitHub independently omits the body for blobs over 1 MiB. +pub const MAX_CONTENTS_BYTES: usize = 512 * 1024; + +/// Decode GitHub's base64 `content` field into UTF-8 text. +/// +/// GitHub wraps the base64 payload across lines, so all ASCII whitespace is +/// stripped before decoding. Returns `None` when the base64 is malformed or the +/// decoded bytes are not valid UTF-8 (the full-file view is text-only). +fn decode_base64_content(content: &str) -> Option { + let stripped: String = content + .chars() + .filter(|c| !c.is_ascii_whitespace()) + .collect(); + let bytes = base64::engine::general_purpose::STANDARD + .decode(stripped) + .ok()?; + String::from_utf8(bytes).ok() +} + +/// Parse a GitHub contents-API response body into optional file text. +/// +/// The response carries the file's bytes as base64 in `content` with an +/// `encoding` field. Pure and independently testable; [`contents_at`] wraps it +/// around the `gh` call. Returns: +/// - `Ok(None)` when `size` exceeds [`MAX_CONTENTS_BYTES`], when GitHub omits +/// the body for a large file (`content` empty with a non-zero `size`, as it +/// does for blobs over 1 MiB), when `encoding` is not `base64`, or when the +/// decoded bytes are not valid UTF-8 (the full-file view is text-only). +/// - `Ok(Some(text))` otherwise (an empty file with `size` 0 decodes to `""`). +fn parse_contents_response(json: &str) -> Result, Error> { + let value: serde_json::Value = + serde_json::from_str(json).map_err(|e| Error::ParseOutput(e.to_string()))?; + + let size = value + .get("size") + .and_then(serde_json::Value::as_u64) + .unwrap_or(0); + // Convert to usize for the cap comparison; a value too large for usize is, + // by definition, over the cap. + let size_bytes = usize::try_from(size).unwrap_or(usize::MAX); + if size_bytes > MAX_CONTENTS_BYTES { + return Ok(None); + } + + // GitHub sets `encoding` to "none" (with an empty body) for oversize blobs. + let encoding = value + .get("encoding") + .and_then(serde_json::Value::as_str) + .unwrap_or(""); + if encoding != "base64" { + return Ok(None); + } + + let content = value + .get("content") + .and_then(serde_json::Value::as_str) + .unwrap_or(""); + if content.is_empty() { + // An empty body with a non-zero size is GitHub omitting the content for + // a large file; an empty body with size 0 is a genuinely empty file. + if size > 0 { + return Ok(None); + } + return Ok(Some(String::new())); + } + + Ok(decode_base64_content(content)) +} + +/// Fetch a file's text at a git ref via the GitHub contents API (fallback). +/// +/// Runs `gh api repos//contents/?ref=` and decodes the base64 +/// body via [`parse_contents_response`]. This is the cross-repo fallback for the +/// full-file view when no local checkout is available. +/// +/// Returns `Ok(None)` when the file is absent at `ref_` (HTTP 404), too large +/// (over [`MAX_CONTENTS_BYTES`], or omitted by GitHub for blobs over 1 MiB), or +/// not UTF-8 text. Any other `gh` failure is an [`Error::GhCommand`]. +pub async fn contents_at(repo_slug: &str, ref_: &str, path: &str) -> Result, Error> { + let endpoint = format!("repos/{repo_slug}/contents/{path}?ref={ref_}"); + let output = Command::new("gh").args(["api", &endpoint]).output().await?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + // A 404 means the path does not exist at this ref (an added/deleted file + // viewed from the wrong side) — a normal outcome, not an error. `gh api` + // exposes no structured status code on failure, and this adapter has no + // status-code discrimination precedent, so we match the "HTTP 404" text + // gh prints to stderr. This is fragile: a change to gh's error wording + // would break the discrimination. + if stderr.contains("HTTP 404") { + return Ok(None); + } + return Err(Error::GhCommand(stderr.into_owned())); + } + + let stdout = String::from_utf8_lossy(&output.stdout); + parse_contents_response(&stdout) +} + // --------------------------------------------------------------------------- // Reviews (inline review comments) // --------------------------------------------------------------------------- @@ -2082,4 +2191,97 @@ index 1111111..2222222 100644 assert!(reviews[1].parents.is_empty()); assert!(reviews[1].children.is_empty()); } + + // -- decode_base64_content -- + + #[test] + fn decode_base64_strips_embedded_whitespace() { + // "hello world" is "aGVsbG8gd29ybGQ="; GitHub wraps the payload across + // lines, so an embedded newline must be stripped before decoding. + let wrapped = "aGVsbG8g\nd29ybGQ=\n"; + assert_eq!( + decode_base64_content(wrapped).as_deref(), + Some("hello world") + ); + } + + #[test] + fn decode_base64_invalid_utf8_is_none() { + // "//4=" decodes to bytes [0xFF, 0xFE], which are not valid UTF-8. + assert_eq!(decode_base64_content("//4="), None); + } + + #[test] + fn decode_base64_malformed_is_none() { + assert_eq!(decode_base64_content("not valid base64 @@@"), None); + } + + // -- parse_contents_response -- + + #[test] + fn parse_contents_decodes_body() { + let json = r#"{"encoding":"base64","size":11,"content":"aGVsbG8gd29ybGQ="}"#; + assert_eq!( + parse_contents_response(json).unwrap().as_deref(), + Some("hello world") + ); + } + + #[test] + fn parse_contents_wrapped_base64() { + // Mirrors GitHub's actual wire format, where `content` carries embedded + // newlines inside the JSON string. + let json = "{\"encoding\":\"base64\",\"size\":11,\"content\":\"aGVsbG8g\\nd29ybGQ=\\n\"}"; + assert_eq!( + parse_contents_response(json).unwrap().as_deref(), + Some("hello world") + ); + } + + #[test] + fn parse_contents_empty_file_is_empty_string() { + let json = r#"{"encoding":"base64","size":0,"content":""}"#; + assert_eq!(parse_contents_response(json).unwrap().as_deref(), Some("")); + } + + #[test] + fn parse_contents_oversize_is_none() { + let json = format!( + r#"{{"encoding":"base64","size":{},"content":"aGk="}}"#, + MAX_CONTENTS_BYTES + 1 + ); + assert_eq!(parse_contents_response(&json).unwrap(), None); + } + + #[test] + fn parse_contents_omitted_large_body_is_none() { + // GitHub omits the body for blobs over 1 MiB: encoding "none", empty + // content, non-zero size. + let json = r#"{"encoding":"none","size":2000000,"content":""}"#; + assert_eq!(parse_contents_response(json).unwrap(), None); + } + + #[test] + fn parse_contents_empty_body_nonzero_size_is_none() { + // A within-cap size but empty base64 body still maps to None: GitHub only + // sends an empty body when it has withheld the content. + let json = r#"{"encoding":"base64","size":42,"content":""}"#; + assert_eq!(parse_contents_response(json).unwrap(), None); + } + + #[test] + fn parse_contents_binary_is_none() { + // base64 "//4=" decodes to non-UTF-8 bytes; the text-only view returns None. + let json = r#"{"encoding":"base64","size":2,"content":"//4="}"#; + assert_eq!(parse_contents_response(json).unwrap(), None); + } + + #[test] + fn parse_contents_malformed_json_errors() { + let result = parse_contents_response("not json at all"); + assert!( + matches!(result, Err(Error::ParseOutput(_))), + "malformed JSON must be a ParseOutput error, got {result:?}" + ); + } } From bd38dd015f4115d4589f15f60e0c1cf77e777050 Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Thu, 2 Jul 2026 02:46:33 +0100 Subject: [PATCH 5/9] =?UTF-8?q?core:=20advisory=20findings=20contract=20?= =?UTF-8?q?=E2=80=94=20parser,=20review=20prompt,=20findings=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tolerant JSON-array extraction (string-aware balanced scan, junk-wrapped agent output survives), skip-malformed entries, stable content-hashed ids, 200-finding cap. assemble_review_prompt mirrors the plan prompt sections with a golden; findings_file_path shares the plan slug sanitizer. Co-Authored-By: Claude Fable 5 --- crates/cockpit-core/src/config.rs | 79 +++- crates/cockpit-core/src/findings.rs | 402 +++++++++++++++++- crates/cockpit-core/src/prompt.rs | 245 ++++++++++- .../tests/golden/review_prompt.txt | 24 ++ 4 files changed, 741 insertions(+), 9 deletions(-) create mode 100644 crates/cockpit-core/tests/golden/review_prompt.txt diff --git a/crates/cockpit-core/src/config.rs b/crates/cockpit-core/src/config.rs index 3a72e66..3b7df67 100644 --- a/crates/cockpit-core/src/config.rs +++ b/crates/cockpit-core/src/config.rs @@ -450,17 +450,50 @@ pub fn plans_dir() -> Result { /// project id with filesystem-hostile characters replaced by `-` so an /// arbitrary Linear project id or ad-hoc name yields a safe single filename. pub fn plan_file_path(project_id: &str) -> Result { - let slug: String = project_id + let slug = path_slug(project_id, "plan"); + Ok(plans_dir()?.join(format!("{slug}.md"))) +} + +/// Return the directory that holds advisory reviewer findings files +/// (`/findings`). +/// +/// Findings are the JSON arrays written by the read-only pre-pass reviewer +/// ([`crate::model::AgentMode::Review`]); each PR's findings are one file under +/// this directory, parsed back with [`crate::findings::parse_findings`]. Like +/// [`plans_dir`], this only resolves the path — it does not create the +/// directory. +pub fn findings_dir() -> Result { + Ok(cockpit_home()?.join("findings")) +} + +/// Return the path to the findings JSON file for the PR identified by `pr`. +/// +/// The convention is `/findings/.json`, where `` is +/// `pr` with filesystem-hostile characters replaced by `-` (the same +/// sanitization [`plan_file_path`] applies), so an arbitrary PR reference such +/// as `owner/repo#42` yields a safe single filename. Like [`plan_file_path`], +/// this only resolves the path and does not create the directory. +pub fn findings_file_path(pr: &str) -> Result { + let slug = path_slug(pr, "findings"); + Ok(findings_dir()?.join(format!("{slug}.json"))) +} + +/// Turn an arbitrary id into a single filesystem-safe filename stem. +/// +/// Every non-alphanumeric character becomes `-`. An id that reduces to only +/// separators (or is empty) falls back to `fallback`, so we never emit an empty +/// or dotfile-only name. Shared by [`plan_file_path`] and [`findings_file_path`] +/// so both use identical sanitization. +fn path_slug(id: &str, fallback: &str) -> String { + let slug: String = id .chars() .map(|c| if c.is_ascii_alphanumeric() { c } else { '-' }) .collect(); - // Guard against an all-separator or empty id producing a dotfile-only name. - let slug = if slug.trim_matches('-').is_empty() { - "plan".to_owned() + if slug.trim_matches('-').is_empty() { + fallback.to_owned() } else { slug - }; - Ok(plans_dir()?.join(format!("{slug}.md"))) + } } /// Return the path to `/config.toml`. @@ -789,6 +822,40 @@ path = "skills" }); } + #[test] + fn findings_file_path_uses_findings_dir_and_slug() { + temp_env::with_var("COCKPIT_HOME", Some("/tmp/cockpit-home-test"), || { + let path = findings_file_path("PR-1").expect("should resolve"); + assert_eq!( + path, + PathBuf::from("/tmp/cockpit-home-test/findings/PR-1.json") + ); + }); + } + + #[test] + fn findings_file_path_sanitizes_hostile_ids() { + temp_env::with_var("COCKPIT_HOME", Some("/tmp/cockpit-home-test"), || { + // A `owner/repo#42` PR ref must collapse to one safe filename. + let path = findings_file_path("owner/repo#42").expect("should resolve"); + assert_eq!( + path, + PathBuf::from("/tmp/cockpit-home-test/findings/owner-repo-42.json") + ); + }); + } + + #[test] + fn findings_file_path_falls_back_for_empty_slug() { + temp_env::with_var("COCKPIT_HOME", Some("/tmp/cockpit-home-test"), || { + let path = findings_file_path("###").expect("should resolve"); + assert_eq!( + path, + PathBuf::from("/tmp/cockpit-home-test/findings/findings.json") + ); + }); + } + #[test] fn written_plan_file_parses_into_doc() { // End-to-end for the ingestion convention: a planner-written markdown diff --git a/crates/cockpit-core/src/findings.rs b/crates/cockpit-core/src/findings.rs index cefb7b3..3d88c0a 100644 --- a/crates/cockpit-core/src/findings.rs +++ b/crates/cockpit-core/src/findings.rs @@ -1,2 +1,400 @@ -//! Advisory reviewer-agent findings: JSON contract parser. Pure; -//! populated in the next commit. +//! Advisory reviewer-agent findings: the JSON contract parser. +//! +//! The read-only pre-pass reviewer ([`crate::model::AgentMode::Review`]) is +//! instructed to emit a JSON array of findings (see +//! [`crate::prompt::assemble_review_prompt`] and the `REVIEW_INSTRUCTION` +//! contract). This module turns that agent output into typed +//! [`ReviewFinding`]s. +//! +//! Parsing is deliberately **tolerant**: agents routinely wrap their output in +//! prose or Markdown code fences, and individual entries can be sloppy. The +//! parser therefore extracts the first bracket region that parses as a JSON +//! array, skips entries it cannot make sense of, and fills defaults for the +//! forgiving fields rather than failing the whole batch. The one hard failure +//! is "no array at all", so a caller can log that the reviewer produced nothing +//! usable. + +use std::fmt::Write as _; +use std::path::PathBuf; + +use serde_json::{Map, Value}; +use sha2::{Digest, Sha256}; + +use crate::model::{DiffSide, FindingSeverity, ReviewFinding}; + +/// Upper bound on the number of findings returned from a single parse. +/// +/// A misbehaving agent could emit thousands of entries; capping keeps memory +/// and UI bounded. Valid entries beyond this count are truncated (dropped). +const MAX_FINDINGS: usize = 200; + +/// Errors from parsing reviewer findings output. +#[derive(Debug, thiserror::Error)] +pub enum Error { + /// The reviewer output contained no bracket region that parsed as a JSON + /// array. Distinct from a found-but-empty array, which is `Ok(vec![])`. + #[error("no JSON array found in reviewer findings output")] + NoArrayFound, +} + +/// Parse an agent's reviewer output into a list of [`ReviewFinding`]s. +/// +/// The input may contain arbitrary prose or code fences around the array; the +/// first bracket region that parses as a JSON array is used (see +/// [`extract_findings_array`]). Malformed entries are skipped rather than +/// failing the parse: +/// +/// - an entry that is not a JSON object, or that lacks a non-empty `path` or +/// `title`, is dropped; +/// - an unknown or missing `severity` becomes [`FindingSeverity::Warning`]; +/// - a missing or unknown `side` becomes [`DiffSide::New`]; +/// - a range whose `line_end` precedes `line_start` is swapped; +/// - a missing line number defaults to `0`. +/// +/// Each returned finding gets a stable id of the form `f{index}-{hash8}`, where +/// `index` is its position in the returned list and `hash8` is the first eight +/// hex digits of `sha256("{path}|{start}-{end}|{title}")`. The same input parses +/// to the same ids every time. +/// +/// Returns [`Error::NoArrayFound`] only when no JSON array can be located; a +/// located-but-empty array yields `Ok(vec![])`. At most [`MAX_FINDINGS`] +/// findings are returned. +pub fn parse_findings(json: &str) -> Result, Error> { + let array = extract_findings_array(json).ok_or(Error::NoArrayFound)?; + + let mut findings = Vec::new(); + for value in &array { + if findings.len() >= MAX_FINDINGS { + break; + } + // The id index is the finding's final position, so re-parsing the same + // input reproduces the same ids. + if let Some(finding) = value_to_finding(value, findings.len()) { + findings.push(finding); + } + } + Ok(findings) +} + +/// Extract the first bracket region that parses as a JSON array. +/// +/// Scans for each `[` and, using a string-aware balanced-bracket walk +/// ([`scan_balanced`]), finds its matching `]`. The enclosed slice is then +/// tried as a JSON array; the first slice that parses wins. This tolerates +/// leading/trailing prose and Markdown code fences without a fragile regex. +/// +/// Byte scanning is safe for UTF-8: the only bytes acted on (`[`, `]`, `"`, +/// `\`) are ASCII, and multi-byte code points never contain those byte values, +/// so slice boundaries always fall on char boundaries. +/// +/// Heuristic note: the first *parseable* JSON array is assumed to be the +/// findings array. A stray JSON array embedded in prose ahead of the real +/// findings would be picked instead; in practice the reviewer emits only the +/// findings array (optionally fenced), so this holds. +fn extract_findings_array(text: &str) -> Option> { + let bytes = text.as_bytes(); + let mut i = 0; + while i < bytes.len() { + if bytes[i] == b'[' { + if let Some(end) = scan_balanced(bytes, i) { + // `i` and `end` index ASCII `[`/`]`, so this range is valid. + if let Ok(array) = serde_json::from_str::>(&text[i..=end]) { + return Some(array); + } + } + } + i += 1; + } + None +} + +/// Find the index of the `]` that closes the `[` at `start`. +/// +/// Walks bytes tracking string context (honoring `\` escapes) so brackets and +/// quotes inside JSON string values are ignored. Returns `None` if the array is +/// never closed. +fn scan_balanced(bytes: &[u8], start: usize) -> Option { + let mut depth: usize = 0; + let mut in_string = false; + let mut escaped = false; + + for (offset, &b) in bytes.iter().enumerate().skip(start) { + if in_string { + if escaped { + escaped = false; + } else if b == b'\\' { + escaped = true; + } else if b == b'"' { + in_string = false; + } + continue; + } + match b { + b'"' => in_string = true, + b'[' => depth += 1, + b']' => { + depth -= 1; + if depth == 0 { + return Some(offset); + } + } + _ => {} + } + } + None +} + +/// Convert a single JSON value into a [`ReviewFinding`], or `None` if it is too +/// malformed to represent (not an object, or missing a `path`/`title`). +fn value_to_finding(value: &Value, index: usize) -> Option { + let obj = value.as_object()?; + + let path = obj.get("path").and_then(Value::as_str)?.trim(); + if path.is_empty() { + return None; + } + let title = obj.get("title").and_then(Value::as_str)?.trim(); + if title.is_empty() { + return None; + } + + let range = read_range(obj); + let severity = parse_severity(obj.get("severity").and_then(Value::as_str)); + let side = parse_side(obj.get("side").and_then(Value::as_str)); + let rationale = obj + .get("rationale") + .and_then(Value::as_str) + .unwrap_or("") + .trim() + .to_owned(); + + let id = finding_id(index, path, range, title); + + Some(ReviewFinding { + id, + severity, + path: PathBuf::from(path), + range, + side, + title: title.to_owned(), + rationale, + }) +} + +/// Read the `(line_start, line_end)` range, defaulting missing values to `0` +/// and swapping so `start <= end`. +fn read_range(obj: &Map) -> (u32, u32) { + let start = read_line(obj, "line_start"); + let end = read_line(obj, "line_end"); + let (start, end) = match (start, end) { + (Some(s), Some(e)) => (s, e), + (Some(s), None) => (s, s), + (None, Some(e)) => (e, e), + (None, None) => (0, 0), + }; + if end < start { + (end, start) + } else { + (start, end) + } +} + +/// Read a single line-number field, clamping out-of-`u32`-range values. +fn read_line(obj: &Map, key: &str) -> Option { + obj.get(key) + .and_then(Value::as_u64) + .map(|n| u32::try_from(n).unwrap_or(u32::MAX)) +} + +/// Map a raw severity string to a [`FindingSeverity`], case-insensitively. +/// +/// Anything unrecognized or absent becomes [`FindingSeverity::Warning`] — the +/// safe middle default for an advisory note. +fn parse_severity(raw: Option<&str>) -> FindingSeverity { + match raw.map(|s| s.trim().to_ascii_lowercase()).as_deref() { + Some("info") => FindingSeverity::Info, + Some("critical") => FindingSeverity::Critical, + _ => FindingSeverity::Warning, + } +} + +/// Map a raw side string to a [`DiffSide`], case-insensitively. +/// +/// Anything other than `"old"` (including absent) becomes [`DiffSide::New`], +/// matching the model's own default. +fn parse_side(raw: Option<&str>) -> DiffSide { + match raw.map(|s| s.trim().to_ascii_lowercase()).as_deref() { + Some("old") => DiffSide::Old, + _ => DiffSide::New, + } +} + +/// Build the stable id `f{index}-{hash8}` for a finding. +/// +/// `hash8` is the first eight hex digits (four bytes) of +/// `sha256("{path}|{start}-{end}|{title}")`, so identical content hashes +/// identically across parses. +fn finding_id(index: usize, path: &str, range: (u32, u32), title: &str) -> String { + let mut hasher = Sha256::new(); + hasher.update(format!("{path}|{}-{}|{title}", range.0, range.1).as_bytes()); + let digest = hasher.finalize(); + + // INVARIANT: write! into a String is infallible. + let mut hex = String::with_capacity(8); + for byte in digest.iter().take(4) { + write!(hex, "{byte:02x}").unwrap(); + } + format!("f{index}-{hex}") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn happy_path_parses_all_fields() { + let json = r#"[ + {"severity":"Critical","path":"src/main.rs","line_start":10,"line_end":15,"side":"New","title":"Missing error handling","rationale":"The result is ignored."}, + {"severity":"Info","path":"src/lib.rs","line_start":42,"line_end":42,"side":"Old","title":"Nit","rationale":"Prefer as_str."} + ]"#; + + let findings = parse_findings(json).expect("should parse"); + assert_eq!(findings.len(), 2); + + let first = &findings[0]; + assert_eq!(first.severity, FindingSeverity::Critical); + assert_eq!(first.path, PathBuf::from("src/main.rs")); + assert_eq!(first.range, (10, 15)); + assert_eq!(first.side, DiffSide::New); + assert_eq!(first.title, "Missing error handling"); + assert_eq!(first.rationale, "The result is ignored."); + assert!(first.id.starts_with("f0-")); + + let second = &findings[1]; + assert_eq!(second.severity, FindingSeverity::Info); + assert_eq!(second.side, DiffSide::Old); + assert!(second.id.starts_with("f1-")); + } + + #[test] + fn junk_wrapped_array_is_extracted() { + // Prose plus a fenced code block, exactly the shape agents emit. + let json = "Here are the findings I spotted [see the diff]:\n\ + ```json\n\ + [{\"severity\":\"Warning\",\"path\":\"a.rs\",\"line_start\":1,\"line_end\":2,\"title\":\"x\",\"rationale\":\"y\"}]\n\ + ```\n\ + Let me know if you want more detail."; + + let findings = parse_findings(json).expect("should parse through the junk"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].path, PathBuf::from("a.rs")); + assert_eq!(findings[0].range, (1, 2)); + } + + #[test] + fn malformed_entries_are_skipped() { + // Entry 1 is a bare string, entry 2 lacks a path, entry 3 is valid. + let json = r#"[ + "not an object", + {"severity":"Info","title":"no path here","line_start":1,"line_end":1}, + {"severity":"Warning","path":"ok.rs","line_start":3,"line_end":4,"title":"kept","rationale":"r"} + ]"#; + + let findings = parse_findings(json).expect("should parse"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].title, "kept"); + // The surviving finding is numbered from the output position, not input. + assert!(findings[0].id.starts_with("f0-")); + } + + #[test] + fn unknown_severity_defaults_to_warning() { + let json = r#"[{"severity":"catastrophic","path":"a.rs","line_start":1,"line_end":1,"title":"t","rationale":"r"}]"#; + let findings = parse_findings(json).expect("should parse"); + assert_eq!(findings[0].severity, FindingSeverity::Warning); + } + + #[test] + fn missing_severity_defaults_to_warning() { + let json = r#"[{"path":"a.rs","line_start":1,"line_end":1,"title":"t","rationale":"r"}]"#; + let findings = parse_findings(json).expect("should parse"); + assert_eq!(findings[0].severity, FindingSeverity::Warning); + } + + #[test] + fn missing_side_defaults_to_new() { + let json = r#"[{"severity":"Info","path":"a.rs","line_start":1,"line_end":1,"title":"t","rationale":"r"}]"#; + let findings = parse_findings(json).expect("should parse"); + assert_eq!(findings[0].side, DiffSide::New); + } + + #[test] + fn swapped_range_is_normalized() { + let json = r#"[{"severity":"Info","path":"a.rs","line_start":20,"line_end":5,"title":"t","rationale":"r"}]"#; + let findings = parse_findings(json).expect("should parse"); + assert_eq!(findings[0].range, (5, 20)); + } + + #[test] + fn no_array_is_an_error() { + let json = "I could not find any issues worth reporting."; + assert!(matches!(parse_findings(json), Err(Error::NoArrayFound))); + } + + #[test] + fn empty_array_is_ok_and_empty() { + let json = "No problems found: []"; + let findings = parse_findings(json).expect("empty array is not an error"); + assert!(findings.is_empty()); + } + + #[test] + fn findings_are_truncated_at_the_cap() { + let mut entries = Vec::new(); + for n in 0..(MAX_FINDINGS + 50) { + entries.push(format!( + r#"{{"severity":"Info","path":"f{n}.rs","line_start":1,"line_end":1,"title":"t{n}","rationale":"r"}}"# + )); + } + let json = format!("[{}]", entries.join(",")); + + let findings = parse_findings(&json).expect("should parse"); + assert_eq!(findings.len(), MAX_FINDINGS); + } + + #[test] + fn ids_are_stable_across_reparses() { + let json = r#"[ + {"severity":"Critical","path":"src/main.rs","line_start":10,"line_end":15,"title":"a","rationale":"r"}, + {"severity":"Info","path":"src/lib.rs","line_start":1,"line_end":2,"title":"b","rationale":"r"} + ]"#; + + let first = parse_findings(json).expect("parse 1"); + let second = parse_findings(json).expect("parse 2"); + + let ids_first: Vec<&str> = first.iter().map(|f| f.id.as_str()).collect(); + let ids_second: Vec<&str> = second.iter().map(|f| f.id.as_str()).collect(); + assert_eq!(ids_first, ids_second); + + // Id shape: f{index}-{8 hex}. + for (i, finding) in first.iter().enumerate() { + let (prefix, hash) = finding.id.split_once('-').expect("id has a dash"); + assert_eq!(prefix, format!("f{i}")); + assert_eq!(hash.len(), 8); + assert!(hash.chars().all(|c| c.is_ascii_hexdigit())); + } + } + + #[test] + fn id_hash_tracks_content() { + // Same content in two entries but different paths must hash differently. + let json = r#"[ + {"path":"a.rs","line_start":1,"line_end":1,"title":"same","rationale":"r"}, + {"path":"b.rs","line_start":1,"line_end":1,"title":"same","rationale":"r"} + ]"#; + let findings = parse_findings(json).expect("should parse"); + let hash_a = findings[0].id.split_once('-').expect("dash").1; + let hash_b = findings[1].id.split_once('-').expect("dash").1; + assert_ne!(hash_a, hash_b); + } +} diff --git a/crates/cockpit-core/src/prompt.rs b/crates/cockpit-core/src/prompt.rs index f02e280..da2ed40 100644 --- a/crates/cockpit-core/src/prompt.rs +++ b/crates/cockpit-core/src/prompt.rs @@ -8,7 +8,7 @@ use std::fmt::Write; use sha2::{Digest, Sha256}; -use crate::model::{AgentMode, Anchor, Artifact, Comment, PlanDoc}; +use crate::model::{AgentMode, Anchor, Artifact, Comment, DiffData, PlanDoc}; use crate::skills::Skill; /// The scope-guard text, verbatim from `SPEC.md` §9. @@ -339,6 +339,97 @@ const PLAN_FORMAT: &str = "\ - "; +/// Input bundle for advisory-review prompt assembly. +/// +/// Collects the data the read-only pre-pass reviewer ([`AgentMode::Review`]) +/// needs: the PR intent, the diff to inspect, and where to write its findings. +/// This mirrors [`PlanInput`]'s shape — there are no comments, because the +/// reviewer produces findings rather than addressing them. +pub struct ReviewInput<'a> { + /// PR title, rendered first in the Intent section. + pub title: &'a str, + /// PR description / body, rendered after the title when non-empty. + pub body: &'a str, + /// The issue reference this review implements (e.g. `NEX-123`). + pub issue: &'a str, + /// Optional user-authored preamble, injected **verbatim** right after the + /// Intent section. `None` or empty is omitted (identical to builtin-only). + /// This is the [`AgentMode::Review`] override seam. + pub custom_preamble: Option<&'a str>, + /// The diff the reviewer inspects. + pub diff: &'a DiffData, + /// Absolute path the reviewer must write its findings JSON to. + /// + /// When set, an explicit "Output" section instructs the agent to write the + /// findings array to this exact path so cockpit can read and parse it back + /// with [`crate::findings::parse_findings`]. `None` omits the section (e.g. + /// for tests that only check the instruction shape). + pub output_path: Option<&'a std::path::Path>, + /// Pre-filtered project skills to inject as conventions. + /// + /// Pass an empty slice to omit the conventions section. + pub skills: &'a [Skill], +} + +/// Assemble a deterministic advisory-review prompt per `SPEC.md` §9. +/// +/// Sections appear in fixed order: +/// 1. Intent (title, then body when non-empty, then issue) +/// (then the custom preamble, verbatim; omitted when None/empty) +/// 2. Current artifact (the diff) +/// 3. Instruction (the read-only reviewer contract, [`REVIEW_INSTRUCTION`]) +/// 4. Output (the findings file path; omitted when no path is supplied) +/// 5. Project conventions (skills) +/// +/// Same inputs always produce the same output. This is the read-only +/// counterpart of [`assemble_plan_prompt`]: it carries no comments because the +/// reviewer emits findings rather than reworking against them. +pub fn assemble_review_prompt(input: &ReviewInput<'_>) -> AssembledPrompt { + // INVARIANT: all writeln!/write! calls target a String, whose fmt::Write + // impl is infallible — unwrap() cannot panic. + let mut text = String::new(); + + // §1 — Intent (title, body, issue) + writeln!(text, "## Intent\n").unwrap(); + writeln!(text, "{}\n", input.title).unwrap(); + if !input.body.trim().is_empty() { + writeln!(text, "{}\n", input.body).unwrap(); + } + writeln!(text, "Issue: {}\n", input.issue).unwrap(); + + // §1b — Custom preamble (verbatim override), fixed position after Intent. + write_custom_preamble(&mut text, input.custom_preamble); + + // §2 — Current artifact (the diff) + writeln!(text, "## Current Artifact\n").unwrap(); + writeln!(text, "{}\n", input.diff.raw).unwrap(); + + // §3 — Instruction + writeln!(text, "## Instruction\n").unwrap(); + writeln!(text, "{REVIEW_INSTRUCTION}\n").unwrap(); + + // §4 — Output (only when a destination path is supplied) + if let Some(path) = input.output_path { + writeln!(text, "## Output\n").unwrap(); + writeln!( + text, + "Write ONLY the JSON array of findings to `{}`. Output nothing else to that file.\n", + path.display() + ) + .unwrap(); + } + + // §5 — Project conventions (skills) + let conventions = crate::skills::format_for_prompt(input.skills); + if !conventions.is_empty() { + text.push_str(&conventions); + } + + let hash = sha256_hex(&text); + + AssembledPrompt { text, hash } +} + /// Render an anchor as a human-readable location string. /// /// When the artifact is a plan, `PlanStep` anchors are enriched with the @@ -1084,4 +1175,156 @@ mod tests { assert_eq!(result.hash, base.hash); } } + + // --------------------------------------------------------------- + // Advisory-review prompt tests + // --------------------------------------------------------------- + + /// A helper diff artifact shared by the review-prompt tests. + fn review_diff() -> DiffData { + DiffData { + raw: "diff --git a/src/main.rs b/src/main.rs\n--- a/src/main.rs\n+++ b/src/main.rs\n@@ -1,3 +1,4 @@\n+fn added() {}".into(), + } + } + + #[test] + fn review_prompt_golden() { + let diff = review_diff(); + let output = std::path::Path::new("/tmp/cockpit-test/findings/owner-repo-42.json"); + let input = ReviewInput { + title: "Add the widget system", + body: "Implements the base widget trait and a button widget.", + issue: "NEX-123", + custom_preamble: None, + diff: &diff, + output_path: Some(output), + skills: &[], + }; + + let result = assemble_review_prompt(&input); + let expected = include_str!("../tests/golden/review_prompt.txt"); + assert_eq!(result.text, expected, "review-prompt golden file mismatch"); + } + + #[test] + fn review_prompt_deterministic() { + let diff = review_diff(); + let input = ReviewInput { + title: "t", + body: "b", + issue: "NEX-1", + custom_preamble: None, + diff: &diff, + output_path: None, + skills: &[], + }; + let a = assemble_review_prompt(&input); + let b = assemble_review_prompt(&input); + assert_eq!(a.hash, b.hash, "same inputs must produce same hash"); + assert_eq!(a.hash.len(), 64, "SHA-256 hex digest is 64 chars"); + assert!( + a.hash.chars().all(|c| c.is_ascii_hexdigit()), + "hash must be hex" + ); + } + + #[test] + fn review_prompt_omits_output_when_no_path() { + let diff = review_diff(); + let input = ReviewInput { + title: "t", + body: "b", + issue: "NEX-1", + custom_preamble: None, + diff: &diff, + output_path: None, + skills: &[], + }; + let result = assemble_review_prompt(&input); + assert!( + !result.text.contains("## Output"), + "no output path should omit the Output section" + ); + } + + #[test] + fn review_prompt_omits_empty_body_line() { + let diff = review_diff(); + let with_body = assemble_review_prompt(&ReviewInput { + title: "t", + body: "some body", + issue: "NEX-1", + custom_preamble: None, + diff: &diff, + output_path: None, + skills: &[], + }); + let no_body = assemble_review_prompt(&ReviewInput { + title: "t", + body: " ", + issue: "NEX-1", + custom_preamble: None, + diff: &diff, + output_path: None, + skills: &[], + }); + assert!(with_body.text.contains("some body")); + assert!(!no_body.text.contains("some body")); + // A blank/whitespace body must not add stray content around the issue. + assert!(no_body.text.contains("## Intent\n\nt\n\nIssue: NEX-1\n")); + } + + #[test] + fn review_prompt_sections_in_fixed_order() { + let diff = review_diff(); + let output = std::path::Path::new("/tmp/findings/x.json"); + let verbatim = "Focus on error handling paths."; + let skills = vec![Skill { + name: "error-handling".into(), + description: "Error conventions".into(), + tags: vec![], + body: "Use thiserror in core crates.".into(), + path: PathBuf::from("error-handling/SKILL.md"), + source: crate::skills::SkillSource::Local, + }]; + let input = ReviewInput { + title: "Add widgets", + body: "Body text", + issue: "NEX-9", + custom_preamble: Some(verbatim), + diff: &diff, + output_path: Some(output), + skills: &skills, + }; + let result = assemble_review_prompt(&input); + + let intent = result.text.find("## Intent").expect("intent missing"); + let custom = result + .text + .find("## Custom Instructions") + .expect("custom missing"); + let artifact = result + .text + .find("## Current Artifact") + .expect("artifact missing"); + let instruction = result + .text + .find("## Instruction") + .expect("instruction missing"); + let output_pos = result.text.find("## Output").expect("output missing"); + let conventions = result + .text + .find("## Project Conventions") + .expect("conventions missing"); + + assert!( + intent < custom + && custom < artifact + && artifact < instruction + && instruction < output_pos + && output_pos < conventions, + "review sections must appear in the fixed order" + ); + assert!(result.text.contains(verbatim), "preamble injected verbatim"); + } } diff --git a/crates/cockpit-core/tests/golden/review_prompt.txt b/crates/cockpit-core/tests/golden/review_prompt.txt new file mode 100644 index 0000000..b033e68 --- /dev/null +++ b/crates/cockpit-core/tests/golden/review_prompt.txt @@ -0,0 +1,24 @@ +## Intent + +Add the widget system + +Implements the base widget trait and a button widget. + +Issue: NEX-123 + +## Current Artifact + +diff --git a/src/main.rs b/src/main.rs +--- a/src/main.rs ++++ b/src/main.rs +@@ -1,3 +1,4 @@ ++fn added() {} + +## Instruction + +Review the diff above against the stated intent. Output ONLY a JSON array of findings to the output path, in this shape: [{"severity":"Info|Warning|Critical","path":"...","line_start":N,"line_end":N,"side":"Old|New","title":"...","rationale":"..."}]. Do NOT edit any code; this pass is advisory only. + +## Output + +Write ONLY the JSON array of findings to `/tmp/cockpit-test/findings/owner-repo-42.json`. Output nothing else to that file. + From e0fb250fc8f3c0ab565ac66ea9b225a1dd8c6573 Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Thu, 2 Jul 2026 02:50:03 +0100 Subject: [PATCH 6/9] =?UTF-8?q?core:=20deterministic=20diff=20signal=20eng?= =?UTF-8?q?ine=20=E2=80=94=20size,=20risk=20paths,=20test=20delta,=20weake?= =?UTF-8?q?ning?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pure single-pass unified-diff analysis: size classes at documented boundaries, priority-ordered per-file risk flags, assertion counting, and seven test-weakening detectors with jump targets. 16 tests + regenerated bindings. Co-Authored-By: Claude Fable 5 --- app/src/bindings/DiffSignals.ts | 38 + app/src/bindings/RiskFlag.ts | 6 + app/src/bindings/RiskPath.ts | 15 + app/src/bindings/SizeClass.ts | 7 + app/src/bindings/TestDelta.ts | 18 + app/src/bindings/WeakeningFlag.ts | 34 + app/src/bindings/WeakeningKind.ts | 6 + crates/cockpit-core/src/diff_signals.rs | 987 +++++++++++++++++++++++- 8 files changed, 1109 insertions(+), 2 deletions(-) create mode 100644 app/src/bindings/DiffSignals.ts create mode 100644 app/src/bindings/RiskFlag.ts create mode 100644 app/src/bindings/RiskPath.ts create mode 100644 app/src/bindings/SizeClass.ts create mode 100644 app/src/bindings/TestDelta.ts create mode 100644 app/src/bindings/WeakeningFlag.ts create mode 100644 app/src/bindings/WeakeningKind.ts diff --git a/app/src/bindings/DiffSignals.ts b/app/src/bindings/DiffSignals.ts new file mode 100644 index 0000000..37a3255 --- /dev/null +++ b/app/src/bindings/DiffSignals.ts @@ -0,0 +1,38 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { RiskPath } from "./RiskPath"; +import type { SizeClass } from "./SizeClass"; +import type { TestDelta } from "./TestDelta"; +import type { WeakeningFlag } from "./WeakeningFlag"; + +/** + * The full deterministic signal set for one diff. + */ +export type DiffSignals = { +/** + * Total added lines. + */ +additions: number, +/** + * Total removed lines. + */ +deletions: number, +/** + * Number of files the diff touches (one per `diff --git` header). + */ +files_changed: number, +/** + * Size bucket derived from additions + deletions. + */ +size_class: SizeClass, +/** + * Test-surface movement. + */ +test_delta: TestDelta, +/** + * Risky files, at most one flag per file. + */ +risk_paths: Array, +/** + * Suspected test-weakening flags. + */ +weakening: Array, }; diff --git a/app/src/bindings/RiskFlag.ts b/app/src/bindings/RiskFlag.ts new file mode 100644 index 0000000..1c18bd7 --- /dev/null +++ b/app/src/bindings/RiskFlag.ts @@ -0,0 +1,6 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * A category of change that warrants extra reviewer scrutiny. + */ +export type RiskFlag = "Migration" | "Lockfile" | "CiConfig" | "Auth" | "GithubDir" | "Dependency"; diff --git a/app/src/bindings/RiskPath.ts b/app/src/bindings/RiskPath.ts new file mode 100644 index 0000000..d2213c9 --- /dev/null +++ b/app/src/bindings/RiskPath.ts @@ -0,0 +1,15 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { RiskFlag } from "./RiskFlag"; + +/** + * A single file flagged as risky, paired with the reason. + */ +export type RiskPath = { +/** + * Why this path is risky. + */ +flag: RiskFlag, +/** + * Repo-relative path of the flagged file. + */ +path: string, }; diff --git a/app/src/bindings/SizeClass.ts b/app/src/bindings/SizeClass.ts new file mode 100644 index 0000000..533a805 --- /dev/null +++ b/app/src/bindings/SizeClass.ts @@ -0,0 +1,7 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * Coarse size bucket for a diff, by total changed lines (additions + + * deletions): `S` < 50, `M` < 200, `L` < 600, `Xl` >= 600. + */ +export type SizeClass = "S" | "M" | "L" | "Xl"; diff --git a/app/src/bindings/TestDelta.ts b/app/src/bindings/TestDelta.ts new file mode 100644 index 0000000..e681dbb --- /dev/null +++ b/app/src/bindings/TestDelta.ts @@ -0,0 +1,18 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * How the diff moved the test surface: files touched and assertion churn. + */ +export type TestDelta = { +/** + * Number of distinct test files the diff touched. + */ +test_files_changed: number, +/** + * Assertion-bearing lines added across the diff. + */ +assertions_added: number, +/** + * Assertion-bearing lines removed across the diff. + */ +assertions_removed: number, }; diff --git a/app/src/bindings/WeakeningFlag.ts b/app/src/bindings/WeakeningFlag.ts new file mode 100644 index 0000000..c60db8a --- /dev/null +++ b/app/src/bindings/WeakeningFlag.ts @@ -0,0 +1,34 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { DiffSide } from "./DiffSide"; +import type { WeakeningKind } from "./WeakeningKind"; + +/** + * A single suspected test-weakening, with a jump target into the diff. + * + * `line`/`side` point at the offending line: the New side for additions + * (e.g. [`WeakeningKind::IgnoreAdded`]), the Old side for deletions + * (e.g. [`WeakeningKind::DeletedAssertion`]). File-level flags + * ([`WeakeningKind::DeletedTestFile`], [`WeakeningKind::SnapshotRewrite`]) + * point at the first removed line. + */ +export type WeakeningFlag = { +/** + * What kind of weakening this is. + */ +kind: WeakeningKind, +/** + * Repo-relative path of the file. + */ +path: string, +/** + * Line number of the offending line on [`Self::side`]. + */ +line: number, +/** + * Which side of the diff [`Self::line`] refers to. + */ +side: DiffSide, +/** + * The trimmed offending line, capped at ~120 characters. + */ +excerpt: string, }; diff --git a/app/src/bindings/WeakeningKind.ts b/app/src/bindings/WeakeningKind.ts new file mode 100644 index 0000000..88d624e --- /dev/null +++ b/app/src/bindings/WeakeningKind.ts @@ -0,0 +1,6 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * The kind of test-weakening a [`WeakeningFlag`] reports. + */ +export type WeakeningKind = "DeletedAssertion" | "IgnoreAdded" | "OrTrue" | "SkipOrTodo" | "DeletedTestFn" | "DeletedTestFile" | "SnapshotRewrite"; diff --git a/crates/cockpit-core/src/diff_signals.rs b/crates/cockpit-core/src/diff_signals.rs index 7b26379..51b9732 100644 --- a/crates/cockpit-core/src/diff_signals.rs +++ b/crates/cockpit-core/src/diff_signals.rs @@ -1,2 +1,985 @@ -//! Deterministic diff-derived review signals (size, risk paths, test -//! delta, weakening flags). Pure; populated in the next commit. +//! Deterministic diff-derived review signals: size class, change risk paths, +//! test delta, and test-weakening flags. +//! +//! Everything here is a **pure** function of a unified diff string. No I/O, no +//! git, no network — the engine walks the diff once and classifies lines with +//! documented heuristics. The output ([`DiffSignals`]) is what the diff gate +//! surfaces to the reviewer so they can spend attention where it matters. +//! +//! Line/side on each [`WeakeningFlag`] is a jump target: additions carry their +//! New-side line number, deletions carry their Old-side line number, matching +//! the diff-side convention used elsewhere in the crate (see +//! `adapters::github::validate_comment_in_diff` for the same hunk-walking shape, +//! reimplemented locally here to avoid coupling to the adapter). + +use std::path::PathBuf; + +use serde::{Deserialize, Serialize}; +use ts_rs::TS; + +use crate::model::DiffSide; + +// --------------------------------------------------------------------------- +// Public types +// --------------------------------------------------------------------------- + +/// Coarse size bucket for a diff, by total changed lines (additions + +/// deletions): `S` < 50, `M` < 200, `L` < 600, `Xl` >= 600. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub enum SizeClass { + /// Small: fewer than 50 changed lines. + S, + /// Medium: 50..200 changed lines. + M, + /// Large: 200..600 changed lines. + L, + /// Extra-large: 600 or more changed lines. + Xl, +} + +/// A category of change that warrants extra reviewer scrutiny. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub enum RiskFlag { + /// A database/schema migration (`migration`/`migrations/` in path, or `.sql`). + Migration, + /// A dependency lockfile (machine-generated churn). + Lockfile, + /// A CI pipeline definition (workflows, `ci.yml`, GitLab CI, Jenkinsfile). + CiConfig, + /// Authentication / secret material (`auth`, `credential`, `secret`, `token`). + Auth, + /// Any other file under `.github/` not already classified as [`RiskFlag::CiConfig`]. + GithubDir, + /// A package manifest whose declared dependencies may have changed. + Dependency, +} + +/// A single file flagged as risky, paired with the reason. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct RiskPath { + /// Why this path is risky. + pub flag: RiskFlag, + /// Repo-relative path of the flagged file. + pub path: PathBuf, +} + +/// How the diff moved the test surface: files touched and assertion churn. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct TestDelta { + /// Number of distinct test files the diff touched. + pub test_files_changed: u32, + /// Assertion-bearing lines added across the diff. + pub assertions_added: u32, + /// Assertion-bearing lines removed across the diff. + pub assertions_removed: u32, +} + +/// The kind of test-weakening a [`WeakeningFlag`] reports. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub enum WeakeningKind { + /// An assertion line was removed from a test file. + DeletedAssertion, + /// An `#[ignore]` attribute was added. + IgnoreAdded, + /// A `|| true` short-circuit was added (silences a failing command). + OrTrue, + /// A skip/todo/focus marker was added (`.skip`, `.todo`, `.only`, `xit`, …). + SkipOrTodo, + /// A test function opener was removed from a test file. + DeletedTestFn, + /// A whole test file was deleted (pure deletions to `/dev/null`). + DeletedTestFile, + /// A snapshot file was substantially rewritten (large removal). + SnapshotRewrite, +} + +/// A single suspected test-weakening, with a jump target into the diff. +/// +/// `line`/`side` point at the offending line: the New side for additions +/// (e.g. [`WeakeningKind::IgnoreAdded`]), the Old side for deletions +/// (e.g. [`WeakeningKind::DeletedAssertion`]). File-level flags +/// ([`WeakeningKind::DeletedTestFile`], [`WeakeningKind::SnapshotRewrite`]) +/// point at the first removed line. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct WeakeningFlag { + /// What kind of weakening this is. + pub kind: WeakeningKind, + /// Repo-relative path of the file. + pub path: PathBuf, + /// Line number of the offending line on [`Self::side`]. + pub line: u32, + /// Which side of the diff [`Self::line`] refers to. + pub side: DiffSide, + /// The trimmed offending line, capped at ~120 characters. + pub excerpt: String, +} + +/// The full deterministic signal set for one diff. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct DiffSignals { + /// Total added lines. + pub additions: u32, + /// Total removed lines. + pub deletions: u32, + /// Number of files the diff touches (one per `diff --git` header). + pub files_changed: u32, + /// Size bucket derived from additions + deletions. + pub size_class: SizeClass, + /// Test-surface movement. + pub test_delta: TestDelta, + /// Risky files, at most one flag per file. + pub risk_paths: Vec, + /// Suspected test-weakening flags. + pub weakening: Vec, +} + +// --------------------------------------------------------------------------- +// Entry point +// --------------------------------------------------------------------------- + +/// Compute [`DiffSignals`] from a unified diff in a single pass. +/// +/// The diff is expected in `git diff` format: each file preceded by a +/// `diff --git a/… b/…` header, `--- `/`+++ ` path headers, and +/// `@@ -a,b +c,d @@` hunk headers (omitted counts default to 1). Old/new line +/// numbers are tracked per hunk so weakening flags carry a real jump target. +/// +/// Assertion counting is content-based across *all* files (not just test +/// files): the [`is_assertion_line`] heuristic is purely textual, matching the +/// literal signal definition. Weakening flags that would create refactor noise +/// (deleted assertions / deleted test functions) are restricted to test files; +/// see [`removed_weakening_kind`]. +pub fn compute_diff_signals(diff: &str) -> DiffSignals { + let mut builder = SignalBuilder::default(); + + for line in diff.lines() { + if line.starts_with("diff --git ") { + builder.start_file(); + continue; + } + + // Path headers only appear before the first hunk. Gating on `!in_hunk` + // disambiguates them from removed lines like `--- foo` inside a hunk. + if !builder.in_hunk { + if let Some(value) = line.strip_prefix("--- ") { + builder.old_path = parse_file_header_path(value); + continue; + } + if let Some(value) = line.strip_prefix("+++ ") { + builder.new_path = parse_file_header_path(value); + builder.new_is_dev_null = builder.new_path.is_none(); + builder.classify_file(); + continue; + } + } + + if let Some((old_start, new_start)) = parse_hunk_header(line) { + builder.old_line = old_start; + builder.new_line = new_start; + builder.in_hunk = true; + continue; + } + + if !builder.in_hunk { + // Preamble lines (`index …`, mode changes, binary markers). + continue; + } + + if let Some(content) = line.strip_prefix('+') { + builder.add_line(content); + builder.new_line += 1; + } else if let Some(content) = line.strip_prefix('-') { + builder.remove_line(content); + builder.old_line += 1; + } else if line.starts_with('\\') { + // `\ No newline at end of file` — a marker, not a real line. + } else { + // Context line (leading space, or a stray empty line): both advance. + builder.old_line += 1; + builder.new_line += 1; + } + } + + builder.finish() +} + +// --------------------------------------------------------------------------- +// Single-pass builder +// --------------------------------------------------------------------------- + +/// Minimum removed lines for a `.snap` rewrite to count as [`WeakeningKind::SnapshotRewrite`]. +const SNAPSHOT_MIN_REMOVED: u32 = 30; + +/// Accumulates signals across one walk of a diff. Per-file fields reset on each +/// `diff --git`; global counters persist. +#[derive(Default)] +struct SignalBuilder { + // Global counters. + additions: u32, + deletions: u32, + files_changed: u32, + test_files_changed: u32, + assertions_added: u32, + assertions_removed: u32, + risk_paths: Vec, + weakening: Vec, + + // Current-file state. + file_open: bool, + old_path: Option, + new_path: Option, + canonical: Option, + is_test: bool, + is_snapshot: bool, + new_is_dev_null: bool, + file_added: u32, + file_removed: u32, + first_removed_old_line: Option, + /// Per-line weakening buffered so a whole-file flag can supersede it. + file_weakening: Vec, + + // Current-hunk state. + in_hunk: bool, + old_line: u32, + new_line: u32, +} + +impl SignalBuilder { + /// Begin a new file: flush the previous one, then reset per-file state. + fn start_file(&mut self) { + self.flush_file(); + self.file_open = true; + self.files_changed += 1; + self.old_path = None; + self.new_path = None; + self.canonical = None; + self.is_test = false; + self.is_snapshot = false; + self.new_is_dev_null = false; + self.file_added = 0; + self.file_removed = 0; + self.first_removed_old_line = None; + self.file_weakening.clear(); + self.in_hunk = false; + } + + /// Classify the current file once both path headers are known: whether it + /// is a test/snapshot file and which single risk flag (if any) it carries. + fn classify_file(&mut self) { + // Prefer the new-side path; fall back to the old side for deletions + // whose new side is `/dev/null`. + let Some(path) = self.new_path.clone().or_else(|| self.old_path.clone()) else { + return; + }; + self.is_test = is_test_file(&path); + self.is_snapshot = is_snapshot_file(&path); + if self.is_test { + self.test_files_changed += 1; + } + if let Some(flag) = classify_risk(&path) { + self.risk_paths.push(RiskPath { + flag, + path: PathBuf::from(&path), + }); + } + self.canonical = Some(PathBuf::from(&path)); + } + + /// Record an added line (content already stripped of its `+`). + fn add_line(&mut self, content: &str) { + self.additions += 1; + self.file_added += 1; + if is_assertion_line(content) { + self.assertions_added += 1; + } + if let Some(kind) = added_weakening_kind(content) { + self.file_weakening.push(WeakeningFlag { + kind, + path: self.canonical.clone().unwrap_or_default(), + line: self.new_line, + side: DiffSide::New, + excerpt: make_excerpt(content), + }); + } + } + + /// Record a removed line (content already stripped of its `-`). + fn remove_line(&mut self, content: &str) { + self.deletions += 1; + self.file_removed += 1; + if self.first_removed_old_line.is_none() { + self.first_removed_old_line = Some(self.old_line); + } + if is_assertion_line(content) { + self.assertions_removed += 1; + } + if let Some(kind) = removed_weakening_kind(content, self.is_test, self.is_snapshot) { + self.file_weakening.push(WeakeningFlag { + kind, + path: self.canonical.clone().unwrap_or_default(), + line: self.old_line, + side: DiffSide::Old, + excerpt: make_excerpt(content), + }); + } + } + + /// Finalize the current file: emit a whole-file weakening flag if one + /// applies (which supersedes the per-line flags to avoid noise), otherwise + /// flush the buffered per-line flags. + fn flush_file(&mut self) { + if !self.file_open { + return; + } + self.file_open = false; + + if let Some(path) = self.canonical.clone() { + // Deleted test file: a test file that is pure deletions to /dev/null. + if self.is_test && self.new_is_dev_null && self.file_added == 0 && self.file_removed > 0 + { + self.weakening.push(WeakeningFlag { + kind: WeakeningKind::DeletedTestFile, + line: self.first_removed_old_line.unwrap_or(1), + side: DiffSide::Old, + excerpt: make_excerpt(&path.display().to_string()), + path, + }); + self.file_weakening.clear(); + return; + } + // Snapshot rewrite: a large removal in a snapshot file, where the + // removed count is at least the threshold and at least double the + // added count (i.e. content was discarded, not merely regenerated). + if self.is_snapshot + && self.file_removed >= SNAPSHOT_MIN_REMOVED + && self.file_removed >= self.file_added.saturating_mul(2) + { + self.weakening.push(WeakeningFlag { + kind: WeakeningKind::SnapshotRewrite, + line: self.first_removed_old_line.unwrap_or(1), + side: DiffSide::Old, + excerpt: make_excerpt(&path.display().to_string()), + path, + }); + self.file_weakening.clear(); + return; + } + } + + self.weakening.append(&mut self.file_weakening); + } + + /// Consume the builder and produce the final signals. + fn finish(mut self) -> DiffSignals { + self.flush_file(); + let total = self.additions.saturating_add(self.deletions); + DiffSignals { + additions: self.additions, + deletions: self.deletions, + files_changed: self.files_changed, + size_class: classify_size(total), + test_delta: TestDelta { + test_files_changed: self.test_files_changed, + assertions_added: self.assertions_added, + assertions_removed: self.assertions_removed, + }, + risk_paths: self.risk_paths, + weakening: self.weakening, + } + } +} + +// --------------------------------------------------------------------------- +// Heuristics +// --------------------------------------------------------------------------- + +/// Bucket a total changed-line count: `S` < 50, `M` < 200, `L` < 600, else `Xl`. +fn classify_size(total: u32) -> SizeClass { + match total { + 0..=49 => SizeClass::S, + 50..=199 => SizeClass::M, + 200..=599 => SizeClass::L, + _ => SizeClass::Xl, + } +} + +/// A path is a test file when it contains `test` (covers `tests/`, `__tests__`, +/// `*_test.rs`, `*.test.ts(x)`) or a `.spec.` segment (`*.spec.*`). The `test` +/// substring is intentionally broad per the signal definition; it can match +/// non-test paths that happen to contain "test", which is accepted as a +/// low-cost false positive. +fn is_test_file(path: &str) -> bool { + let lower = path.to_ascii_lowercase(); + lower.contains("test") || lower.contains(".spec.") +} + +/// A path is a snapshot file when it ends in `.snap` or lives under a +/// `__snapshots__` directory. +fn is_snapshot_file(path: &str) -> bool { + path.ends_with(".snap") || path.contains("__snapshots__") +} + +/// A changed line asserts when it mentions `assert` (Rust `assert!`/`assert_eq!`/ +/// `assert_ne!`/`debug_assert*`), `expect(` (TS/vitest/jest), or a `.toBe`/ +/// `.toEqual` matcher. +fn is_assertion_line(content: &str) -> bool { + content.contains("assert") + || content.contains("expect(") + || content.contains(".toBe") + || content.contains(".toEqual") +} + +/// The single risk flag for a path, in priority order (each file flags once). +/// +/// `Migration` and `Lockfile` outrank `CiConfig`, which outranks `Auth`, which +/// outranks the catch-all `GithubDir`, which outranks `Dependency`. The order +/// resolves overlaps (e.g. `.github/workflows/ci.yml` is `CiConfig`, not +/// `GithubDir`). +fn classify_risk(path: &str) -> Option { + let lower = path.to_ascii_lowercase(); + let name = file_name(path); + + if lower.contains("migration") || lower.ends_with(".sql") { + return Some(RiskFlag::Migration); + } + + const LOCKFILES: [&str; 6] = [ + "Cargo.lock", + "package-lock.json", + "pnpm-lock.yaml", + "yarn.lock", + "uv.lock", + "poetry.lock", + ]; + if LOCKFILES.contains(&name) { + return Some(RiskFlag::Lockfile); + } + + if lower.contains(".github/workflows/") + || name == "ci.yml" + || lower.contains(".gitlab-ci") + || name == "Jenkinsfile" + { + return Some(RiskFlag::CiConfig); + } + + if has_auth_indicator(path) { + return Some(RiskFlag::Auth); + } + + if lower.contains(".github/") { + return Some(RiskFlag::GithubDir); + } + + if name == "Cargo.toml" || name == "package.json" { + return Some(RiskFlag::Dependency); + } + + None +} + +/// Whether a path names auth/secret material. +/// +/// `credential`, `secret`, and `token` match as plain (case-insensitive) +/// substrings — they have no notable collision words, so plurals and camelCase +/// are covered. `auth` is matched unless immediately followed by `o`, which +/// filters the `autho…` stem (`author`, `authored`, `authorize`) while keeping +/// `auth.rs`, `auth_token`, `authService`, and `authentication`. As a +/// documented limitation, `authorization`/`authorize` share that stem and so do +/// not flag. +fn has_auth_indicator(path: &str) -> bool { + let lower = path.to_ascii_lowercase(); + if lower.contains("credential") || lower.contains("secret") || lower.contains("token") { + return true; + } + for (idx, _) in lower.match_indices("auth") { + if lower.as_bytes().get(idx + 4).copied() != Some(b'o') { + return true; + } + } + false +} + +/// The weakening kind for an added line, if any (first match wins). +fn added_weakening_kind(content: &str) -> Option { + if content.contains("#[ignore") { + return Some(WeakeningKind::IgnoreAdded); + } + if content.contains("|| true") { + return Some(WeakeningKind::OrTrue); + } + if is_skip_or_todo(content) { + return Some(WeakeningKind::SkipOrTodo); + } + None +} + +/// Skip/todo/focus markers. `.only(` is included: focusing a single test +/// silently skips the rest of the suite, which weakens it. +fn is_skip_or_todo(content: &str) -> bool { + const MARKERS: [&str; 7] = [ + ".skip(", + ".todo(", + "it.skip", + "describe.skip", + "xit(", + "xdescribe(", + ".only(", + ]; + MARKERS.iter().any(|marker| content.contains(marker)) +} + +/// The weakening kind for a removed line, if any. +/// +/// Restricted to non-snapshot test files: removing an assertion or a test +/// function opener only counts as weakening inside a hand-written test file, so +/// ordinary production refactors do not generate noise. +fn removed_weakening_kind( + content: &str, + is_test: bool, + is_snapshot: bool, +) -> Option { + if !is_test || is_snapshot { + return None; + } + if is_test_fn_opening(content.trim()) { + return Some(WeakeningKind::DeletedTestFn); + } + if is_assertion_line(content) { + return Some(WeakeningKind::DeletedAssertion); + } + None +} + +/// Whether a trimmed line opens a test: a Rust `#[test]` attribute, a +/// `fn test_…` opener, or a JS `it(`/`test(` opener. +fn is_test_fn_opening(trimmed: &str) -> bool { + trimmed.contains("#[test]") + || trimmed.starts_with("fn test_") + || trimmed.starts_with("it(") + || trimmed.starts_with("test(") +} + +/// Trim a line and cap it at ~120 characters for use as a flag excerpt. +fn make_excerpt(content: &str) -> String { + let trimmed = content.trim(); + if trimmed.chars().count() <= 120 { + trimmed.to_string() + } else { + trimmed.chars().take(120).collect() + } +} + +/// The final path component (basename), or the whole path if it has no `/`. +fn file_name(path: &str) -> &str { + path.rsplit('/').next().unwrap_or(path) +} + +// --------------------------------------------------------------------------- +// Local unified-diff header parsing +// --------------------------------------------------------------------------- + +/// Extract the repo-relative path from a `---`/`+++` header value, stripping the +/// `a/`/`b/` prefix and trailing tab metadata. Returns `None` for `/dev/null`. +fn parse_file_header_path(value: &str) -> Option { + let token = value.split('\t').next().unwrap_or(value).trim(); + if token == "/dev/null" { + return None; + } + let stripped = token + .strip_prefix("a/") + .or_else(|| token.strip_prefix("b/")) + .unwrap_or(token); + Some(stripped.to_string()) +} + +/// Parse a hunk header (`@@ -a,b +c,d @@`) into `(old_start, new_start)`. +/// +/// Returns `None` for any non-hunk line. Counts are ignored here (line tracking +/// derives lengths from the body), so only the two start numbers are returned. +fn parse_hunk_header(line: &str) -> Option<(u32, u32)> { + let rest = line.strip_prefix("@@ ")?; + let close = rest.find(" @@")?; + let spec = &rest[..close]; + + let mut parts = spec.split_whitespace(); + let old = parts.next()?.strip_prefix('-')?; + let new = parts.next()?.strip_prefix('+')?; + Some((parse_hunk_start(old)?, parse_hunk_start(new)?)) +} + +/// Parse the start line from a hunk range component (`a,b` or `a`). +fn parse_hunk_start(component: &str) -> Option { + let start = match component.split_once(',') { + Some((start, _)) => start, + None => component, + }; + start.parse().ok() +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + + /// A diff that adds `n` lines to a plain (non-test) file. + fn diff_adding(n: u32) -> String { + let mut s = String::new(); + s.push_str("diff --git a/data.txt b/data.txt\n"); + s.push_str("--- a/data.txt\n"); + s.push_str("+++ b/data.txt\n"); + s.push_str(&format!("@@ -0,0 +1,{n} @@\n")); + for i in 0..n { + s.push_str(&format!("+row {i}\n")); + } + s + } + + /// A minimal diff that touches (one context + one added line) `path`. + fn touch(path: &str) -> String { + format!( + "diff --git a/{path} b/{path}\n--- a/{path}\n+++ b/{path}\n@@ -1,1 +1,2 @@\n unchanged\n+changed\n" + ) + } + + #[test] + fn size_class_boundary_s_m() { + assert_eq!( + compute_diff_signals(&diff_adding(49)).size_class, + SizeClass::S + ); + let m = compute_diff_signals(&diff_adding(50)); + assert_eq!(m.size_class, SizeClass::M); + assert_eq!(m.additions, 50); + assert_eq!(m.deletions, 0); + } + + #[test] + fn size_class_boundary_m_l() { + assert_eq!( + compute_diff_signals(&diff_adding(199)).size_class, + SizeClass::M + ); + assert_eq!( + compute_diff_signals(&diff_adding(200)).size_class, + SizeClass::L + ); + } + + #[test] + fn size_class_boundary_l_xl() { + assert_eq!( + compute_diff_signals(&diff_adding(599)).size_class, + SizeClass::L + ); + assert_eq!( + compute_diff_signals(&diff_adding(600)).size_class, + SizeClass::Xl + ); + } + + #[test] + fn risk_flags_by_path() { + let cases: [(&str, RiskFlag); 6] = [ + ("db/migrations/001_init.sql", RiskFlag::Migration), + ("Cargo.lock", RiskFlag::Lockfile), + (".github/workflows/ci.yml", RiskFlag::CiConfig), + ("src/auth.rs", RiskFlag::Auth), + (".github/dependabot.yml", RiskFlag::GithubDir), + ("Cargo.toml", RiskFlag::Dependency), + ]; + for (path, expected) in cases { + let signals = compute_diff_signals(&touch(path)); + assert_eq!( + signals.risk_paths.len(), + 1, + "path {path} should yield exactly one risk flag" + ); + assert_eq!(signals.risk_paths[0].flag, expected, "flag for {path}"); + assert_eq!(signals.risk_paths[0].path, PathBuf::from(path)); + } + } + + #[test] + fn author_rs_does_not_flag_auth() { + let signals = compute_diff_signals(&touch("src/author.rs")); + assert!( + signals.risk_paths.is_empty(), + "author.rs must not flag Auth (the `autho` stem is excluded)" + ); + } + + #[test] + fn test_delta_counts_assertions_and_files() { + let diff = concat!( + "diff --git a/tests/alpha.rs b/tests/alpha.rs\n", + "--- a/tests/alpha.rs\n", + "+++ b/tests/alpha.rs\n", + "@@ -1,4 +1,5 @@\n", + " fn setup() {}\n", + "+ assert_eq!(1, 1);\n", + "+ assert!(ok());\n", + "- assert_ne!(2, 3);\n", + " fn teardown() {}\n", + "diff --git a/src/widget.test.ts b/src/widget.test.ts\n", + "--- a/src/widget.test.ts\n", + "+++ b/src/widget.test.ts\n", + "@@ -1,2 +1,3 @@\n", + " describe('w', () => {});\n", + "+ expect(v).toBe(1);\n", + " done();\n", + ); + let s = compute_diff_signals(diff); + assert_eq!(s.test_delta.test_files_changed, 2); + assert_eq!(s.test_delta.assertions_added, 3); + assert_eq!(s.test_delta.assertions_removed, 1); + } + + #[test] + fn weakening_deleted_assertion() { + let diff = concat!( + "diff --git a/tests/foo.rs b/tests/foo.rs\n", + "--- a/tests/foo.rs\n", + "+++ b/tests/foo.rs\n", + "@@ -1,4 +1,3 @@\n", + " fn test_thing() {\n", + " let x = compute();\n", + "- assert_eq!(x, 42);\n", + " }\n", + ); + let s = compute_diff_signals(diff); + assert_eq!(s.weakening.len(), 1); + let w = &s.weakening[0]; + assert_eq!(w.kind, WeakeningKind::DeletedAssertion); + assert_eq!(w.side, DiffSide::Old); + assert_eq!(w.line, 3); + assert_eq!(w.path, PathBuf::from("tests/foo.rs")); + assert_eq!(w.excerpt, "assert_eq!(x, 42);"); + } + + #[test] + fn weakening_ignore_added() { + let diff = concat!( + "diff --git a/src/lib.rs b/src/lib.rs\n", + "--- a/src/lib.rs\n", + "+++ b/src/lib.rs\n", + "@@ -10,2 +10,3 @@\n", + " mod m {\n", + "+#[ignore]\n", + " }\n", + ); + let s = compute_diff_signals(diff); + assert_eq!(s.weakening.len(), 1); + let w = &s.weakening[0]; + assert_eq!(w.kind, WeakeningKind::IgnoreAdded); + assert_eq!(w.side, DiffSide::New); + assert_eq!(w.line, 11); + assert_eq!(w.excerpt, "#[ignore]"); + } + + #[test] + fn weakening_or_true() { + let diff = concat!( + "diff --git a/scripts/run.sh b/scripts/run.sh\n", + "--- a/scripts/run.sh\n", + "+++ b/scripts/run.sh\n", + "@@ -1,2 +1,3 @@\n", + " #!/bin/sh\n", + "+cargo check || true\n", + " echo ok\n", + ); + let s = compute_diff_signals(diff); + assert_eq!(s.weakening.len(), 1); + let w = &s.weakening[0]; + assert_eq!(w.kind, WeakeningKind::OrTrue); + assert_eq!(w.side, DiffSide::New); + assert_eq!(w.line, 2); + assert_eq!(w.excerpt, "cargo check || true"); + } + + #[test] + fn weakening_skip_todo_and_only() { + let skip = concat!( + "diff --git a/src/a.test.ts b/src/a.test.ts\n", + "--- a/src/a.test.ts\n", + "+++ b/src/a.test.ts\n", + "@@ -1,2 +1,3 @@\n", + " describe('a', () => {\n", + "+ it.skip('wip', () => {});\n", + " });\n", + ); + let s = compute_diff_signals(skip); + assert_eq!(s.weakening.len(), 1); + assert_eq!(s.weakening[0].kind, WeakeningKind::SkipOrTodo); + assert_eq!(s.weakening[0].side, DiffSide::New); + assert_eq!(s.weakening[0].line, 2); + + let only = concat!( + "diff --git a/src/b.test.ts b/src/b.test.ts\n", + "--- a/src/b.test.ts\n", + "+++ b/src/b.test.ts\n", + "@@ -1,2 +1,3 @@\n", + " describe('b', () => {\n", + "+ it.only('focus', () => {});\n", + " });\n", + ); + let s2 = compute_diff_signals(only); + assert_eq!(s2.weakening.len(), 1); + assert_eq!(s2.weakening[0].kind, WeakeningKind::SkipOrTodo); + } + + #[test] + fn weakening_deleted_test_fn() { + let diff = concat!( + "diff --git a/tests/bar.rs b/tests/bar.rs\n", + "--- a/tests/bar.rs\n", + "+++ b/tests/bar.rs\n", + "@@ -1,4 +1,3 @@\n", + " fn helper() {}\n", + " mod inner {}\n", + "-fn test_old() {}\n", + " fn other() {}\n", + ); + let s = compute_diff_signals(diff); + assert_eq!(s.weakening.len(), 1); + let w = &s.weakening[0]; + assert_eq!(w.kind, WeakeningKind::DeletedTestFn); + assert_eq!(w.side, DiffSide::Old); + assert_eq!(w.line, 3); + assert_eq!(w.excerpt, "fn test_old() {}"); + } + + #[test] + fn weakening_deleted_test_file() { + let diff = concat!( + "diff --git a/tests/gone.rs b/tests/gone.rs\n", + "deleted file mode 100644\n", + "index 89abcde..0000000\n", + "--- a/tests/gone.rs\n", + "+++ /dev/null\n", + "@@ -1,4 +0,0 @@\n", + "-#[test]\n", + "-fn test_a() {\n", + "- assert!(true);\n", + "-}\n", + ); + let s = compute_diff_signals(diff); + assert_eq!( + s.weakening.len(), + 1, + "whole-file deletion collapses to a single flag" + ); + let w = &s.weakening[0]; + assert_eq!(w.kind, WeakeningKind::DeletedTestFile); + assert_eq!(w.side, DiffSide::Old); + assert_eq!(w.line, 1); + assert_eq!(w.path, PathBuf::from("tests/gone.rs")); + } + + #[test] + fn weakening_snapshot_rewrite() { + // 30 removed, 5 added: >= threshold and >= 2x added -> fires. + let mut big = String::new(); + big.push_str("diff --git a/src/__snapshots__/foo.snap b/src/__snapshots__/foo.snap\n"); + big.push_str("--- a/src/__snapshots__/foo.snap\n"); + big.push_str("+++ b/src/__snapshots__/foo.snap\n"); + big.push_str("@@ -1,30 +1,5 @@\n"); + for i in 0..30 { + big.push_str(&format!("-old {i}\n")); + } + for i in 0..5 { + big.push_str(&format!("+new {i}\n")); + } + let s = compute_diff_signals(&big); + assert_eq!(s.weakening.len(), 1); + assert_eq!(s.weakening[0].kind, WeakeningKind::SnapshotRewrite); + assert_eq!(s.weakening[0].side, DiffSide::Old); + + // 10 removed: below the 30-line threshold -> no flag. + let mut small = String::new(); + small.push_str("diff --git a/src/__snapshots__/bar.snap b/src/__snapshots__/bar.snap\n"); + small.push_str("--- a/src/__snapshots__/bar.snap\n"); + small.push_str("+++ b/src/__snapshots__/bar.snap\n"); + small.push_str("@@ -1,10 +1,1 @@\n"); + for i in 0..10 { + small.push_str(&format!("-old {i}\n")); + } + small.push_str("+new 0\n"); + let s2 = compute_diff_signals(&small); + assert!( + s2.weakening.is_empty(), + "10 removed is below the snapshot threshold" + ); + } + + #[test] + fn clean_refactor_has_no_weakening() { + let diff = concat!( + "diff --git a/src/util.rs b/src/util.rs\n", + "--- a/src/util.rs\n", + "+++ b/src/util.rs\n", + "@@ -1,3 +1,3 @@\n", + " fn add(a: i32, b: i32) -> i32 {\n", + "- a + b\n", + "+ a.wrapping_add(b)\n", + " }\n", + ); + let s = compute_diff_signals(diff); + assert!(s.weakening.is_empty()); + assert_eq!(s.additions, 1); + assert_eq!(s.deletions, 1); + assert_eq!(s.files_changed, 1); + assert_eq!(s.size_class, SizeClass::S); + } + + #[test] + fn removed_assertion_in_non_test_file_is_not_weakening() { + let diff = concat!( + "diff --git a/src/main.rs b/src/main.rs\n", + "--- a/src/main.rs\n", + "+++ b/src/main.rs\n", + "@@ -1,4 +1,3 @@\n", + " fn main() {\n", + "- assert!(ready());\n", + " run();\n", + " }\n", + ); + let s = compute_diff_signals(diff); + assert!( + s.weakening.is_empty(), + "removing an assertion outside a test file is not weakening" + ); + // Counting is content-based across all files, so it still tallies. + assert_eq!(s.test_delta.assertions_removed, 1); + assert_eq!(s.test_delta.test_files_changed, 0); + } + + #[test] + fn empty_diff_is_zeroed() { + let s = compute_diff_signals(""); + assert_eq!(s.additions, 0); + assert_eq!(s.deletions, 0); + assert_eq!(s.files_changed, 0); + assert_eq!(s.size_class, SizeClass::S); + assert_eq!( + s.test_delta, + TestDelta { + test_files_changed: 0, + assertions_added: 0, + assertions_removed: 0, + } + ); + assert!(s.risk_paths.is_empty()); + assert!(s.weakening.is_empty()); + } +} From eb8d02adf38b3fe41894d1afbca54a9314aeda59 Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Thu, 2 Jul 2026 03:00:30 +0100 Subject: [PATCH 7/9] app: wire evidence, pre-review, and full-file commands - get_evidence: deterministic signals + CI rollup (trajectory slot ready for Phase D) - pre_review: explicit reviewer-agent spawn (AgentMode::Review); findings ingested on completion, file deleted after transport, gate state never touched; findings cleared when the head advances - get_file_pair: worktree git2 reads with gh contents fallback and honest full:false degradation Co-Authored-By: Claude Fable 5 --- app/src-tauri/src/commands/mod.rs | 300 +++++++++++++++++++++++- app/src-tauri/src/lib.rs | 104 +++++++- app/src/bindings/CommandRun.ts | 18 ++ app/src/bindings/EvidenceSummary.ts | 26 ++ app/src/bindings/FilePair.ts | 24 ++ crates/cockpit-core/src/diff_signals.rs | 33 ++- crates/cockpit-core/src/model.rs | 18 ++ 7 files changed, 517 insertions(+), 6 deletions(-) create mode 100644 app/src/bindings/CommandRun.ts create mode 100644 app/src/bindings/EvidenceSummary.ts create mode 100644 app/src/bindings/FilePair.ts diff --git a/app/src-tauri/src/commands/mod.rs b/app/src-tauri/src/commands/mod.rs index 0859741..3ef5d80 100644 --- a/app/src-tauri/src/commands/mod.rs +++ b/app/src-tauri/src/commands/mod.rs @@ -15,11 +15,13 @@ use cockpit_core::adapters::agent::SpawnConfig; use cockpit_core::adapters::github::{self, MirrorResult, ReviewEvent, SubmitReviewResult}; use cockpit_core::adapters::linear; use cockpit_core::config::Config; +use cockpit_core::diff_signals::{EvidenceSummary, compute_diff_signals}; use cockpit_core::gate::Gated; use cockpit_core::kickoff::{self, KickoffResult}; use cockpit_core::model::{ AgentMode, Anchor, Artifact, CiSummary, Comment, CommentId, CommentOrigin, DiffData, DiffSide, - GateState, PlanDoc, PrRef, Project, ProjectId, ProjectPlan, ProjectRef, Review, ReviewSource, + FilePair, GateState, PlanDoc, PrRef, Project, ProjectId, ProjectPlan, ProjectRef, Review, + ReviewSource, }; use cockpit_core::plan_parser; use cockpit_core::restack; @@ -146,6 +148,171 @@ pub async fn get_interdiff( Ok(DiffData { raw }) } +/// Return the review-time evidence bundle for a PR (B1). +/// +/// Bundles the deterministic diff signals, the review's CI rollup, and the +/// commands the agent ran into one [`EvidenceSummary`] so the diff gate can show +/// what changed, whether CI is green, and what the agent executed without three +/// separate round-trips. +/// +/// The diff can be large, so [`compute_diff_signals`] runs on the blocking pool +/// via [`tokio::task::spawn_blocking`] (only the owned diff string — `Send` — +/// crosses the boundary). `agent_ran` is empty for now; Phase D fills it from the +/// agent trajectory. +#[tauri::command] +pub async fn get_evidence( + state: State<'_, Arc>, + pr: String, +) -> Result { + let pr_ref = PrRef::new(&pr); + let review = state.reviews.get(&pr_ref).ok_or_else(|| CommandError { + message: format!("Review not found: {pr}"), + })?; + + let ci = review.ci_summary; + let raw = review.diff.raw; + let signals = tokio::task::spawn_blocking(move || compute_diff_signals(&raw)) + .await + .map_err(|e| CommandError { + message: format!("diff signal task panicked: {e}"), + })?; + + Ok(EvidenceSummary { + signals, + ci, + // Phase D fills this from the agent trajectory; empty for now. + agent_ran: vec![], + }) +} + +/// Return the full text of a single file on both sides of a review's diff (B4). +/// +/// Feeds the diff gate's optional full-file view. The two revisions are resolved +/// preferring pinned SHAs (truthful across force-pushes): the base is +/// `base_sha` when set, else the base branch name; the head is `head_sha` when +/// set, else `HEAD` for a local read or the head branch name for a GitHub read. +/// +/// Content is read locally with +/// [`git::file_at_rev`](cockpit_core::adapters::git::file_at_rev) (off the async +/// runtime, since `git2` is blocking) when a usable local repo dir exists — a +/// cockpit-managed worktree present on disk, or the shared checkout for a +/// same-repo PR (`repo_slug` absent). Otherwise, for an imported PR with a +/// `repo_slug`, it falls back to +/// [`github::contents_at`](cockpit_core::adapters::github::contents_at). +/// +/// See [`combine_file_pair`] for how the two per-side results become the +/// returned [`FilePair`] and when `full` is `false`. +#[tauri::command] +pub async fn get_file_pair( + state: State<'_, Arc>, + pr: String, + path: String, +) -> Result { + let pr_ref = PrRef::new(&pr); + let review = state.reviews.get(&pr_ref).ok_or_else(|| CommandError { + message: format!("Review not found: {pr}"), + })?; + + let config = Config::load()?; + let repo_path = config + .repo_path + .clone() + .unwrap_or_else(|| PathBuf::from(".")); + + // Resolve the base revision: a pinned SHA when known, else the base branch. + let base_rev = if review.base_sha.is_empty() { + review.base.clone() + } else { + review.base_sha.clone() + }; + + // A usable local repo dir: a managed worktree present on disk, or — only for + // a same-repo PR (`repo_slug` absent) — the shared checkout. A cross-repo PR + // (`repo_slug` set) whose worktree still points at `repo_path` is the wrong + // repo, so it routes to the GitHub fallback instead. + let local_dir: Option = if is_managed_worktree(&review.worktree, &repo_path) { + review.worktree.exists().then(|| review.worktree.clone()) + } else if review.repo_slug.is_none() && repo_path.exists() { + Some(repo_path.clone()) + } else { + None + }; + + if let Some(dir) = local_dir { + // Head revision for a local read: a pinned SHA when known, else `HEAD` + // (the worktree branch tip). + let head_rev = if review.head_sha.is_empty() { + "HEAD".to_string() + } else { + review.head_sha.clone() + }; + let pair = tokio::task::spawn_blocking(move || { + let original = cockpit_core::adapters::git::file_at_rev(&dir, &base_rev, &path); + let modified = cockpit_core::adapters::git::file_at_rev(&dir, &head_rev, &path); + combine_file_pair(original, modified) + }) + .await + .map_err(|e| CommandError { + message: format!("file-pair task panicked: {e}"), + })?; + return Ok(pair); + } + + // Imported PR with no usable local dir: read via GitHub contents. + if let Some(repo_slug) = review.repo_slug.as_deref() { + // Head ref for a GitHub read: a pinned SHA when known, else the head + // branch name. + let head_ref = if review.head_sha.is_empty() { + review.branch.clone() + } else { + review.head_sha.clone() + }; + let original = github::contents_at(repo_slug, &base_rev, &path).await; + let modified = github::contents_at(repo_slug, &head_ref, &path).await; + return Ok(combine_file_pair(original, modified)); + } + + // Neither a local dir nor a repo slug: nothing to read — fall back. + Ok(FilePair { + original: String::new(), + modified: String::new(), + full: false, + }) +} + +/// Combine the two per-side file-content reads into a [`FilePair`]. +/// +/// `Err` on either side means the content could not be determined, so the pair +/// is reported as not-full (`full: false`) and the frontend falls back to the +/// diff fragments. `Ok(None)` on a side means the file is legitimately absent +/// there — an added or deleted file, or (indistinguishably) a blob past the +/// adapter's size cap — and maps to an empty string; but when BOTH sides are +/// absent there is nothing to show, so the pair is not-full. Any side that +/// loaded makes the pair `full`. +fn combine_file_pair( + original: Result, E>, + modified: Result, E>, +) -> FilePair { + let not_full = FilePair { + original: String::new(), + modified: String::new(), + full: false, + }; + match (original, modified) { + (Ok(orig), Ok(modi)) => match (orig, modi) { + // Both sides legitimately absent: nothing to render. + (None, None) => not_full, + (orig, modi) => FilePair { + original: orig.unwrap_or_default(), + modified: modi.unwrap_or_default(), + full: true, + }, + }, + // Could not determine one or both sides. + _ => not_full, + } +} + /// Add an anchored comment to a review at the diff gate. /// /// Creates an ephemeral [`Comment`] with a `DiffLine` anchor and @@ -417,6 +584,137 @@ async fn try_spawn_agent( )) } +/// Run the advisory read-only pre-pass reviewer over a PR's diff (B2). +/// +/// This is an explicit user action. It spawns an [`AgentMode::Review`] agent +/// that inspects the diff against the PR intent and writes a JSON findings array +/// to [`config::findings_file_path`](cockpit_core::config::findings_file_path); +/// the Stop-hook completion handler ingests that file onto the review (see +/// `ingest_review_findings` in `lib.rs`). +/// +/// The pre-pass is advisory: it NEVER touches the gate state (Review mode never +/// transitions). It refuses to start when an agent is already attached so a +/// reviewer cannot race an in-flight fix/restack/implement agent. An imported +/// PR's worktree is materialized first via [`ensure_worktree_for_review`]. On a +/// successful spawn the running agent is attached and any stale findings from a +/// previous pre-pass are cleared so the UI shows "agent working" cleanly. +#[tauri::command] +pub async fn pre_review( + state: State<'_, Arc>, + app_handle: tauri::AppHandle, + pr: String, +) -> Result { + let pr_ref = PrRef::new(&pr); + let review = state.reviews.get(&pr_ref).ok_or_else(|| CommandError { + message: format!("Review not found: {pr}"), + })?; + + // Refuse to start a second agent for this review: the advisory reviewer must + // not race an in-flight fix/restack/implement agent. + if review.agent.is_some() { + return Err(CommandError { + message: format!("Review {pr} already has a running agent; wait for it to finish"), + }); + } + + // Materialize a usable worktree for imported PRs (a no-op for managed ones). + let worktree = ensure_worktree_for_review(&state, &pr_ref).await?; + + // Re-read after materialization, which may have updated the worktree path. + let review = state.reviews.get(&pr_ref).ok_or_else(|| CommandError { + message: format!("Review not found: {pr}"), + })?; + + // Resolve (and ensure the parent dir of) the findings output path so the + // reviewer's write succeeds; the completion handler reads it back. + let findings_path = cockpit_core::config::findings_file_path(&pr)?; + if let Some(parent) = findings_path.parent() { + std::fs::create_dir_all(parent)?; + } + + // Assemble the review prompt: intent from the PR title/body/issue, the + // Review-mode custom preamble, and skills relevant to the diff — mirroring + // dispatch_fix_agent's structure. A config load failure falls back to no + // preamble (builtin instruction). + let preamble = Config::load().ok().and_then(|c| { + c.agent_prompts + .for_mode(AgentMode::Review) + .map(str::to_owned) + }); + let skills = cockpit_core::skills::relevant_for_diff(&review.diff.raw); + let review_input = cockpit_core::prompt::ReviewInput { + title: &review.title, + body: &review.body, + issue: review.issue.as_str(), + custom_preamble: preamble.as_deref(), + diff: &review.diff, + output_path: Some(&findings_path), + skills: &skills, + }; + let assembled = cockpit_core::prompt::assemble_review_prompt(&review_input); + + // Spawn the reviewer (keyed by PR ref so completion ingests the right + // review). On success, attach the agent and clear any stale findings from a + // previous pre-pass — atomically, so the UI never shows old findings under a + // running agent. The gate state is left UNTOUCHED (Invariant 5). + let agent_run = + try_spawn_review_agent(&state, &app_handle, &pr, &pr_ref, &worktree, &assembled) + .await + .map_err(|e| CommandError { + message: format!("failed to spawn reviewer agent: {e}"), + })?; + state.reviews.update(&pr_ref, |r| { + r.review_findings.clear(); + r.agent = Some(agent_run); + }); + + state.reviews.get(&pr_ref).ok_or_else(|| CommandError { + message: format!("Review not found: {pr}"), + }) +} + +/// Attempt to spawn the advisory pre-pass reviewer ([`AgentMode::Review`]). +/// +/// Mirrors [`try_spawn_agent`] but in Review mode: spawns the agent in the +/// review's worktree, keyed by the PR ref, and wires stdout streaming to the +/// frontend. Factored out so the caller can map a spawn failure to a +/// [`CommandError`]. +async fn try_spawn_review_agent( + state: &AppState, + app_handle: &tauri::AppHandle, + pr: &str, + pr_ref: &PrRef, + worktree: &std::path::Path, + prompt: &cockpit_core::prompt::AssembledPrompt, +) -> Result { + let config = Config::load().map_err(|e| format!("config: {e}"))?; + let spawn_config = SpawnConfig::from_config(&config); + let hook_url = format!("http://127.0.0.1:{}/hook/stop", config.hook_port); + + let spawn_result = cockpit_core::adapters::agent::spawn_agent( + worktree, + prompt, + AgentMode::Review, + pr_ref.as_str(), + &state.sessions, + &hook_url, + &spawn_config, + ) + .await + .map_err(|e| format!("spawn: {e}"))?; + + let stream_ctx = crate::streaming::StreamContext { + object_id: pr.to_string(), + mode: AgentMode::Review, + completion_tx: state.completion_tx.clone(), + }; + Ok(crate::streaming::start_stream_forwarding( + spawn_result, + app_handle.clone(), + stream_ctx, + )) +} + // --------------------------------------------------------------------------- // Worktree materialization // --------------------------------------------------------------------------- diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 0e7a261..38abb5d 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -168,10 +168,11 @@ pub fn run() { "completed" } AgentMode::Review => { - // Advisory pre-pass reviewer finished. Findings - // ingest lands in W1; for now this is a no-op stub - // that keeps the match exhaustive. - "completed" + // Advisory pre-pass reviewer finished: ingest its + // findings file onto the review. This NEVER touches + // the gate state — the pre-pass is read-only + // (Invariant 5). + ingest_review_findings(&app_state_ref, &event.object_id) } }; @@ -259,6 +260,9 @@ pub fn run() { commands::open_review, commands::get_review_diff, commands::get_interdiff, + commands::get_evidence, + commands::get_file_pair, + commands::pre_review, commands::add_comment, commands::request_changes, commands::mirror_comments, @@ -508,6 +512,94 @@ fn ingest_plan_output(state: &AppState, project_id: &ProjectId) -> &'static str outcome } +/// Ingest the advisory reviewer's findings after an [`AgentMode::Review`] +/// completion and return the outcome label for the `"agent-completed"` payload. +/// +/// The read-only pre-pass reviewer writes a JSON findings array to +/// [`config::findings_file_path`](cockpit_core::config::findings_file_path), +/// keyed by the PR ref used at spawn (the completion event's `object_id`). This +/// reads that file, parses it with +/// [`findings::parse_findings`](cockpit_core::findings::parse_findings), stores +/// the result on the review, and always clears the review's running agent handle. +/// +/// Every failure mode is non-fatal (Invariant 1) and maps to `"failed"`: no +/// review resolves for the object id, the path cannot be resolved, the file is +/// missing or unreadable, or the parse returns +/// [`Error::NoArrayFound`](cockpit_core::findings::Error::NoArrayFound). A +/// successful parse (including a located-but-empty array) stores the findings and +/// returns `"completed"`. +/// +/// INVARIANT: this NEVER touches `gate_state`. The advisory pre-pass is +/// read-only and never advances the gate (Invariant 5). +/// +/// The findings file is a transport, not a store: it is deleted after ingest — +/// findings now live on the [`Review`] and in persistence. +fn ingest_review_findings(state: &AppState, object_id: &str) -> &'static str { + let Some(pr_ref) = resolve_review_pr(state, object_id) else { + // No stored review resolved for this object id — nothing to ingest. + return "failed"; + }; + + // Read + parse the reviewer's findings file (keyed by the PR ref used at + // spawn). Any failure yields `None`; a successful parse yields the findings. + let path = cockpit_core::config::findings_file_path(object_id); + let parsed = match &path { + Ok(path) => match std::fs::read_to_string(path) { + Ok(raw) => match cockpit_core::findings::parse_findings(&raw) { + Ok(findings) => Some(findings), + Err(e) => { + eprintln!( + "ingest_review_findings: parse failed for {}: {e}", + path.display() + ); + None + } + }, + Err(e) => { + eprintln!( + "ingest_review_findings: read failed for {}: {e}", + path.display() + ); + None + } + }, + Err(e) => { + eprintln!("ingest_review_findings: findings path for {object_id}: {e}"); + None + } + }; + + let outcome = if parsed.is_some() { + "completed" + } else { + "failed" + }; + + // Always clear the running agent; store the findings on a successful parse. + // INVARIANT: gate_state is never touched here (read-only pre-pass). + state.reviews.update(&pr_ref, |r| { + r.agent = None; + if let Some(findings) = parsed { + r.review_findings = findings; + } + }); + + // The findings file is a transport, not a store — delete it after ingest. + // Best-effort: a delete failure (other than a missing file) is logged but + // never changes the outcome. + if let Ok(path) = &path + && let Err(e) = std::fs::remove_file(path) + && e.kind() != std::io::ErrorKind::NotFound + { + eprintln!( + "ingest_review_findings: remove {} failed: {e}", + path.display() + ); + } + + outcome +} + /// Resolve a review's [`PrRef`] from a completion event's `object_id`. /// /// The object id may be a [`PrRef`] string (the Fix/Restack path keys sessions @@ -558,6 +650,10 @@ async fn refresh_review_diff(state: &AppState, pr_ref: &PrRef) { if let Ok(raw_diff) = diff_result { state.reviews.update(pr_ref, |review| { review.diff = DiffData { raw: raw_diff }; + // The advisory findings were anchored to the previous diff/head; once + // the diff is replaced (e.g. after a Fix landed a commit) they no + // longer line up, so drop them. + review.review_findings.clear(); }); } } diff --git a/app/src/bindings/CommandRun.ts b/app/src/bindings/CommandRun.ts new file mode 100644 index 0000000..70e8987 --- /dev/null +++ b/app/src/bindings/CommandRun.ts @@ -0,0 +1,18 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * A single command an agent ran during its work, paired with its outcome. + * + * Defined here in the evidence module so [`EvidenceSummary`] can carry it now; + * Phase D's trajectory module reuses this type to summarize what an agent + * executed (there is no separate trajectory module yet). + */ +export type CommandRun = { +/** + * The command line the agent ran. + */ +command: string, +/** + * Whether the command exited successfully. + */ +ok: boolean, }; diff --git a/app/src/bindings/EvidenceSummary.ts b/app/src/bindings/EvidenceSummary.ts new file mode 100644 index 0000000..0ff4aa8 --- /dev/null +++ b/app/src/bindings/EvidenceSummary.ts @@ -0,0 +1,26 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { CiSummary } from "./CiSummary"; +import type { CommandRun } from "./CommandRun"; +import type { DiffSignals } from "./DiffSignals"; + +/** + * The review-time evidence bundle: deterministic diff signals, the CI rollup, + * and the commands the agent ran. + * + * Assembled per review so the diff gate can show, in one place, what changed + * (the [`DiffSignals`]), whether CI is green (the [`CiSummary`]), and what the + * agent actually executed (the [`CommandRun`]s). + */ +export type EvidenceSummary = { +/** + * Deterministic diff-derived signals for the review. + */ +signals: DiffSignals, +/** + * Rolled-up CI status, when a CI check has populated it. + */ +ci: CiSummary | null, +/** + * Commands the agent ran; empty until Phase D fills it from the trajectory. + */ +agent_ran: Array, }; diff --git a/app/src/bindings/FilePair.ts b/app/src/bindings/FilePair.ts new file mode 100644 index 0000000..0331ed5 --- /dev/null +++ b/app/src/bindings/FilePair.ts @@ -0,0 +1,24 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * The full text of a single file on both sides of a review's diff. + * + * Feeds the diff gate's optional full-file view (Monaco). `full` is `true` only + * when the pair was resolved: a side that is legitimately absent (an added or + * deleted file) is an empty string but still counts as resolved. `full` is + * `false` when the content could not be determined on either side, signalling + * the frontend to fall back to the diff fragments. + */ +export type FilePair = { +/** + * File text at the base revision (empty when absent on that side). + */ +original: string, +/** + * File text at the head revision (empty when absent on that side). + */ +modified: string, +/** + * Whether the pair resolved (so the full-file view can be shown). + */ +full: boolean, }; diff --git a/crates/cockpit-core/src/diff_signals.rs b/crates/cockpit-core/src/diff_signals.rs index 51b9732..6f09573 100644 --- a/crates/cockpit-core/src/diff_signals.rs +++ b/crates/cockpit-core/src/diff_signals.rs @@ -17,7 +17,7 @@ use std::path::PathBuf; use serde::{Deserialize, Serialize}; use ts_rs::TS; -use crate::model::DiffSide; +use crate::model::{CiSummary, DiffSide}; // --------------------------------------------------------------------------- // Public types @@ -140,6 +140,37 @@ pub struct DiffSignals { pub weakening: Vec, } +/// A single command an agent ran during its work, paired with its outcome. +/// +/// Defined here in the evidence module so [`EvidenceSummary`] can carry it now; +/// Phase D's trajectory module reuses this type to summarize what an agent +/// executed (there is no separate trajectory module yet). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct CommandRun { + /// The command line the agent ran. + pub command: String, + /// Whether the command exited successfully. + pub ok: bool, +} + +/// The review-time evidence bundle: deterministic diff signals, the CI rollup, +/// and the commands the agent ran. +/// +/// Assembled per review so the diff gate can show, in one place, what changed +/// (the [`DiffSignals`]), whether CI is green (the [`CiSummary`]), and what the +/// agent actually executed (the [`CommandRun`]s). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct EvidenceSummary { + /// Deterministic diff-derived signals for the review. + pub signals: DiffSignals, + /// Rolled-up CI status, when a CI check has populated it. + pub ci: Option, + /// Commands the agent ran; empty until Phase D fills it from the trajectory. + pub agent_ran: Vec, +} + // --------------------------------------------------------------------------- // Entry point // --------------------------------------------------------------------------- diff --git a/crates/cockpit-core/src/model.rs b/crates/cockpit-core/src/model.rs index c9a8b9a..4d66d7d 100644 --- a/crates/cockpit-core/src/model.rs +++ b/crates/cockpit-core/src/model.rs @@ -416,6 +416,24 @@ pub struct ReviewFinding { pub rationale: String, } +/// The full text of a single file on both sides of a review's diff. +/// +/// Feeds the diff gate's optional full-file view (Monaco). `full` is `true` only +/// when the pair was resolved: a side that is legitimately absent (an added or +/// deleted file) is an empty string but still counts as resolved. `full` is +/// `false` when the content could not be determined on either side, signalling +/// the frontend to fall back to the diff fragments. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct FilePair { + /// File text at the base revision (empty when absent on that side). + pub original: String, + /// File text at the head revision (empty when absent on that side). + pub modified: String, + /// Whether the pair resolved (so the full-file view can be shown). + pub full: bool, +} + /// A read-only GitHub conversation item shown alongside a review for context. /// /// This is deliberately **not** a [`Comment`]: it is external context pulled From 18f6c48755273575497a0bea1934e51b81e69786 Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Thu, 2 Jul 2026 03:19:25 +0100 Subject: [PATCH 8/9] =?UTF-8?q?app(fe):=20Phase=20B=20=E2=80=94=20evidence?= =?UTF-8?q?=20strip,=20advisory=20pins,=20pre-review,=20full-file=20view?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EvidenceStrip (CI/size/test-delta/risk/weakening chips with jump-to-hunk), advisory finding pins on parallel zone machinery (dashed severity rail, local dismiss, never counted as comments), Pre-review button, full-file toggle with identity line maps + per-head cache, TS signal mirror with Rust-fixture parity tests. Co-Authored-By: Claude Fable 5 --- app/src/app.css | 12 + app/src/components/DiffView.tsx | 516 +++++++++++++++++++++- app/src/components/EvidenceStrip.test.tsx | 84 ++++ app/src/components/EvidenceStrip.tsx | 339 ++++++++++++++ app/src/diff-parser.test.ts | 34 ++ app/src/diff-parser.ts | 24 + app/src/lib/diff-signals.test.ts | 126 ++++++ app/src/lib/diff-signals.ts | 166 +++++++ app/src/store.test.ts | 128 ++++++ app/src/store.ts | 99 +++++ 10 files changed, 1516 insertions(+), 12 deletions(-) create mode 100644 app/src/components/EvidenceStrip.test.tsx create mode 100644 app/src/components/EvidenceStrip.tsx create mode 100644 app/src/lib/diff-signals.test.ts create mode 100644 app/src/lib/diff-signals.ts diff --git a/app/src/app.css b/app/src/app.css index 5be3bff..e39ef47 100644 --- a/app/src/app.css +++ b/app/src/app.css @@ -237,3 +237,15 @@ .inline-comment-line-glyph { border-left: 3px solid var(--color-state-in-review); } + +/* Advisory review-finding rails in the glyph margin (B2). Dashed to read as + distinct from the solid human-comment accent, colored by severity. */ +.finding-line-info { + border-left: 3px dashed var(--color-muted-foreground); +} +.finding-line-warning { + border-left: 3px dashed var(--color-warning); +} +.finding-line-critical { + border-left: 3px dashed var(--color-danger); +} diff --git a/app/src/components/DiffView.tsx b/app/src/components/DiffView.tsx index bf57ba6..6ebb51a 100644 --- a/app/src/components/DiffView.tsx +++ b/app/src/components/DiffView.tsx @@ -31,14 +31,20 @@ import type { MirrorResult } from "../bindings/MirrorResult"; import type { Anchor } from "../bindings/Anchor"; import type { DiffSide } from "../bindings/DiffSide"; import type { CiSummary } from "../bindings/CiSummary"; +import type { CiCheck } from "../bindings/CiCheck"; import { summarizeChecks, ciState, parseCiUpdate } from "@/lib/ci"; import type { ReviewEvent } from "../bindings/ReviewEvent"; import type { SubmitReviewResult } from "../bindings/SubmitReviewResult"; +import type { EvidenceSummary } from "../bindings/EvidenceSummary"; +import type { FilePair } from "../bindings/FilePair"; +import type { ReviewFinding } from "../bindings/ReviewFinding"; +import type { FindingSeverity } from "../bindings/FindingSeverity"; import { parseDiff, extractFilePaths, fragmentToReal, realToFragment, + identityLineMap, } from "../diff-parser"; import type { FileDiff } from "../diff-parser"; import { elapsedSince } from "@/lib/relative-time"; @@ -50,6 +56,7 @@ import { Badge } from "@/components/ui/badge"; import { GatePill } from "./GatePill"; import { IntentPanel } from "./IntentPanel"; import { AddressedRequests } from "./AddressedRequests"; +import { EvidenceStrip } from "./EvidenceStrip"; import { SubmitReviewControl } from "./SubmitReviewControl"; import { cn } from "@/lib/utils"; import { invoke } from "@tauri-apps/api/core"; @@ -63,6 +70,7 @@ import { Send, AlertTriangle, Bot, + BotMessageSquare, Hash, GitBranch, CheckCircle2, @@ -72,6 +80,7 @@ import { Layers, Check, GitMerge, + X, } from "lucide-react"; // --------------------------------------------------------------------------- @@ -471,6 +480,128 @@ function InlineCommentThread({ ); } +// --------------------------------------------------------------------------- +// FindingPin -- advisory review findings, rendered in Monaco view zones +// --------------------------------------------------------------------------- + +/** Presentation for a finding severity: dot / border / text color + label. */ +interface SeverityMeta { + readonly label: string; + readonly dot: string; + readonly border: string; + readonly text: string; + /** The Monaco glyph-margin class that draws the dashed severity rail. */ + readonly glyph: string; +} + +function severityMeta(severity: FindingSeverity): SeverityMeta { + switch (severity) { + case "Info": + return { + label: "Info", + dot: "bg-muted-foreground", + border: "border-muted-foreground/50", + text: "text-muted-foreground", + glyph: "finding-line-info", + }; + case "Warning": + return { + label: "Warning", + dot: "bg-warning", + border: "border-warning/60", + text: "text-warning", + glyph: "finding-line-warning", + }; + case "Critical": + return { + label: "Critical", + dot: "bg-danger", + border: "border-danger/60", + text: "text-danger", + glyph: "finding-line-critical", + }; + default: + return assertNever(severity); + } +} + +/** + * An advisory finding from the read-only pre-pass reviewer, rendered inline as a + * Monaco view zone. Deliberately distinct from a human [`InlineCommentThread`]: + * a dashed severity-colored border plus a severity label, and a dismiss control. + * Findings never count toward the Request Changes requirement. + */ +function FindingPin({ + finding, + onDismiss, +}: { + readonly finding: ReviewFinding; + readonly onDismiss: () => void; +}) { + const meta = severityMeta(finding.severity); + return ( +
+
+ + + +
+
{finding.title}
+
+ {finding.rationale} +
+
+ ); +} + +/** A portal entry for a finding zone (mirrors {@link PortalEntry} for comments). */ +interface FindingPortalEntry { + readonly key: string; + readonly domNode: HTMLDivElement; + readonly finding: ReviewFinding; +} + +/** + * Estimate a finding zone's pixel height from its rationale, so the view zone + * reserves enough room. A rough character-per-line wrap is good enough; Monaco + * clips gracefully if the estimate is short. + */ +function findingZoneHeight(finding: ReviewFinding): number { + const perLine = 88; + const rationaleLines = finding.rationale + .split("\n") + .reduce((n, l) => n + Math.max(1, Math.ceil(l.length / perLine)), 0); + return 52 + rationaleLines * 18; +} + // --------------------------------------------------------------------------- // DiffView // --------------------------------------------------------------------------- @@ -497,6 +628,12 @@ export function DiffView({ const submitGithubReview = useAppStore((s) => s.submitGithubReview); const fetchInterdiff = useAppStore((s) => s.fetchInterdiff); + // -- Phase B store actions (evidence / pre-review / full-file) -- + const fetchEvidence = useAppStore((s) => s.fetchEvidence); + const preReview = useAppStore((s) => s.preReview); + const fetchFilePair = useAppStore((s) => s.fetchFilePair); + const listCiChecks = useAppStore((s) => s.listCiChecks); + // -- D10: interdiff (changes since the last review dispatch) -- // Only a reworked review with a dispatch snapshot has a meaningful interdiff. const hasInterdiff = @@ -504,10 +641,35 @@ export function DiffView({ const [interdiff, setInterdiff] = useState(null); const [diffSource, setDiffSource] = useState<"interdiff" | "full">("full"); + // Whether the interdiff is the active view (it exempts the full-file toggle). + const interdiffActive = diffSource === "interdiff" && interdiff !== null; + // The raw diff currently shown: the interdiff when selected and available, // otherwise the full review diff. - const activeDiffRaw = - diffSource === "interdiff" && interdiff !== null ? interdiff.raw : diff.raw; + const activeDiffRaw = interdiffActive ? interdiff.raw : diff.raw; + + // -- B1/B3: review-time evidence bundle (refetched on head change) -- + const [evidence, setEvidence] = useState(null); + // Raw CI checks (for the evidence strip's failing-job name), kept alongside + // the summarized badge; populated from the initial fetch and `ci-updated`. + const [ciChecks, setCiChecks] = useState([]); + + // -- B2: advisory pre-pass reviewer + component-local finding dismissals -- + const [preReviewing, setPreReviewing] = useState(false); + const [dismissedFindings, setDismissedFindings] = useState< + ReadonlySet + >(new Set()); + const [findingPortals, setFindingPortals] = useState< + readonly FindingPortalEntry[] + >([]); + + // -- B4: Hunks vs Full-file view + the resolved full-file pair -- + const [viewMode, setViewMode] = useState<"hunks" | "full">("hunks"); + const [fullPair, setFullPair] = useState(null); + + // Bumped on each jump-to-line request so the apply effect re-runs even when + // the target file is already selected. + const [jumpNonce, setJumpNonce] = useState(0); // -- Diff parsing -- const fileDiffs = useMemo(() => parseDiff(activeDiffRaw), [activeDiffRaw]); @@ -574,6 +736,9 @@ export function DiffView({ const zoneIdsRef = useRef([]); const originalZoneIdsRef = useRef([]); const domNodeCacheRef = useRef>(new Map()); + const findingZoneIdsRef = useRef([]); + const originalFindingZoneIdsRef = useRef([]); + const findingNodeCacheRef = useRef>(new Map()); const glyphDecorRef = useRef(null); const commentDecorRef = @@ -582,7 +747,18 @@ export function DiffView({ useRef(null); const originalCommentDecorRef = useRef(null); + const findingDecorRef = + useRef(null); + const originalFindingDecorRef = + useRef(null); const lspAttachmentRef = useRef(null); + // Pending jump-to-line request (from evidence-strip weakening chips), applied + // once the target file's editor + line map are ready. + const pendingJumpRef = useRef<{ + readonly path: string; + readonly side: DiffSide; + readonly line: number; + } | null>(null); // -- Derived -- const currentFileDiff = useMemo( @@ -590,19 +766,51 @@ export function DiffView({ [fileDiffs, selectedFile], ); - // Keep the current file diff reachable from the (mount-time) Monaco mouse + // -- B4: full-file view is active only when the pair resolved and the + // interdiff is not showing (the interdiff is exempt from full-file). -- + const fullFileActive = + viewMode === "full" && + !interdiffActive && + fullPair !== null && + fullPair.full; + + // The file diff the editor + zones actually consume. In full-file mode this is + // the complete file text with an identity line map, so the comment/zone code + // path (fragmentToReal / realToFragment) works unchanged on real lines. + const effectiveFileDiff = useMemo(() => { + if (fullFileActive && fullPair !== null) { + return { + path: selectedFile, + original: fullPair.original, + modified: fullPair.modified, + lineMap: identityLineMap(fullPair.original, fullPair.modified), + }; + } + return currentFileDiff; + }, [fullFileActive, fullPair, selectedFile, currentFileDiff]); + + // Keep the effective file diff reachable from the (mount-time) Monaco mouse // handlers, which capture nothing else from render scope. Used to map a // clicked fragment line to its real file line for the comment anchor (D1). - const currentFileDiffRef = useRef(currentFileDiff); + const currentFileDiffRef = useRef(effectiveFileDiff); useEffect(() => { - currentFileDiffRef.current = currentFileDiff; - }, [currentFileDiff]); + currentFileDiffRef.current = effectiveFileDiff; + }, [effectiveFileDiff]); const commentsForFile = useMemo( () => fileComments(review.comments, selectedFile), [review.comments, selectedFile], ); + // -- B2: advisory findings for the current file, minus dismissed ones. -- + const findingsForFile = useMemo( + () => + review.review_findings.filter( + (f) => f.path === selectedFile && !dismissedFindings.has(f.id), + ), + [review.review_findings, selectedFile, dismissedFindings], + ); + const hasLocalComments = useMemo( () => review.comments.some((c) => isLocalOrigin(c.origin)), [review.comments], @@ -731,6 +939,7 @@ export function DiffView({ glyphDecorRef.current = modified.createDecorationsCollection([]); commentDecorRef.current = modified.createDecorationsCollection([]); + findingDecorRef.current = modified.createDecorationsCollection([]); // Hover: show "+" glyph on the hovered line modified.onMouseMove((e) => { @@ -790,6 +999,8 @@ export function DiffView({ originalGlyphDecorRef.current = original.createDecorationsCollection([]); originalCommentDecorRef.current = original.createDecorationsCollection([]); + originalFindingDecorRef.current = + original.createDecorationsCollection([]); original.onMouseMove((e) => { if (originalGlyphDecorRef.current == null) return; @@ -876,7 +1087,7 @@ export function DiffView({ if (anchorSide(c.anchor) !== side) continue; const range = anchorRange(c.anchor); if (range === null) continue; - const fragLine = realToFragment(currentFileDiff, side, range[1]); + const fragLine = realToFragment(effectiveFileDiff, side, range[1]); if (fragLine === undefined) continue; const arr = commentsByLine.get(fragLine) ?? []; arr.push(c); @@ -933,7 +1144,8 @@ export function DiffView({ }); // Display + anchor use the real file line; placement used the fragment. - const realLine = fragmentToReal(currentFileDiff, side, line) ?? line; + const realLine = + fragmentToReal(effectiveFileDiff, side, line) ?? line; zoneIds.current.push(zoneId); portals.push({ key, @@ -996,12 +1208,118 @@ export function DiffView({ }, [ editorReady, commentsForFile, - currentFileDiff, + effectiveFileDiff, effectiveActiveComment, selectedFile, diffMode, ]); + // -- B2: sync advisory finding pins as their OWN view zones + a dashed + // severity rail, kept entirely separate from the comment machinery. Finding + // zones use dedicated zone-id arrays, DOM-node cache, and decoration + // collections keyed by finding id, so they never collide with a human comment + // on the same line — a finding and a comment on one line simply stack. -- + useEffect(() => { + if ( + !editorReady || + diffEditorRef.current == null || + monacoRef.current == null + ) { + return; + } + const monaco = monacoRef.current; + + const syncSide = ( + ed: MonacoEditorNs.IStandaloneCodeEditor, + side: DiffSide, + zoneIds: { current: string[] }, + decor: MonacoEditorNs.IEditorDecorationsCollection | null, + ): { + readonly portals: FindingPortalEntry[]; + readonly usedKeys: Set; + } => { + const portals: FindingPortalEntry[] = []; + const usedKeys = new Set(); + const decorations: MonacoEditorNs.IModelDeltaDecoration[] = []; + + ed.changeViewZones((accessor) => { + for (const id of zoneIds.current) { + accessor.removeZone(id); + } + zoneIds.current = []; + + for (const finding of findingsForFile) { + if (finding.side !== side) continue; + const fragLine = realToFragment( + effectiveFileDiff, + side, + finding.range[1], + ); + if (fragLine === undefined) continue; + + const key = `finding-${finding.id}`; + usedKeys.add(key); + let domNode = findingNodeCacheRef.current.get(key); + if (domNode == null) { + domNode = document.createElement("div"); + domNode.style.zIndex = "10"; + findingNodeCacheRef.current.set(key, domNode); + } + + const zoneId = accessor.addZone({ + afterLineNumber: fragLine, + heightInPx: findingZoneHeight(finding), + domNode, + suppressMouseDown: false, + }); + zoneIds.current.push(zoneId); + portals.push({ key, domNode, finding }); + + decorations.push({ + range: new monaco.Range(fragLine, 1, fragLine, 1), + options: { + isWholeLine: true, + glyphMarginClassName: severityMeta(finding.severity).glyph, + }, + }); + } + }); + + decor?.set(decorations); + return { portals, usedKeys }; + }; + + const modified = diffEditorRef.current.getModifiedEditor(); + const original = diffEditorRef.current.getOriginalEditor(); + modified.updateOptions({ glyphMargin: true }); + original.updateOptions({ glyphMargin: true }); + + const newResult = syncSide( + modified, + "New", + findingZoneIdsRef, + findingDecorRef.current, + ); + const oldResult = syncSide( + original, + "Old", + originalFindingZoneIdsRef, + originalFindingDecorRef.current, + ); + + const usedKeys = new Set([ + ...newResult.usedKeys, + ...oldResult.usedKeys, + ]); + for (const cachedKey of findingNodeCacheRef.current.keys()) { + if (!usedKeys.has(cachedKey)) { + findingNodeCacheRef.current.delete(cachedKey); + } + } + + setFindingPortals([...newResult.portals, ...oldResult.portals]); + }, [editorReady, findingsForFile, effectiveFileDiff, diffMode]); + // -- LSP: attach a language client to the modified (right-hand) model -- // Runs once the editor is ready and whenever the selected file (and thus its // language) changes. Only languages with a configured server (typescript / @@ -1104,11 +1422,17 @@ export function DiffView({ console.error("fetch_ci_checks failed", e); }); + // Raw checks (for the evidence strip's failing-job name). Best-effort. + void listCiChecks(prRef).then((checks) => { + if (!cancelled) setCiChecks(checks); + }); + // Live updates: the backend pushes the full checks list via `ci-updated`. const unlisten = listen("ci-updated", (event) => { const update = parseCiUpdate(event.payload); if (update === null || update.pr !== prRef) return; setCiSummary(summarizeChecks(update.checks)); + setCiChecks(update.checks); }); return () => { @@ -1117,7 +1441,71 @@ export function DiffView({ f(); }); }; - }, [prRef]); + }, [prRef, listCiChecks]); + + // -- B1/B3: fetch the evidence bundle on entry and on each head change. -- + const reviewHead = review.head_sha; + useEffect(() => { + let cancelled = false; + void fetchEvidence(reviewPr).then((ev) => { + if (!cancelled) setEvidence(ev); + }); + return () => { + cancelled = true; + }; + }, [reviewPr, reviewHead, fetchEvidence]); + + // -- B4: resolve the full-file pair when Full-file mode is active. The pair is + // reset on file/mode switch so stale content never flashes; the store memoizes + // by `pr:path:head` so re-entry (and a return to the same file) is cheap. -- + useEffect(() => { + if (viewMode !== "full" || interdiffActive || selectedFile === "") { + setFullPair(null); + return; + } + let cancelled = false; + setFullPair(null); + void fetchFilePair(reviewPr, selectedFile).then((pair) => { + if (!cancelled) setFullPair(pair); + }); + return () => { + cancelled = true; + }; + }, [ + viewMode, + interdiffActive, + selectedFile, + reviewPr, + reviewHead, + fetchFilePair, + ]); + + // -- B3: apply a pending jump-to-line once the target file's editor + line + // map are ready (from evidence-strip weakening chips). -- + useEffect(() => { + const jump = pendingJumpRef.current; + if (jump === null) return; + if (jump.path !== selectedFile) return; + if (!editorReady || diffEditorRef.current === null) return; + const frag = realToFragment(effectiveFileDiff, jump.side, jump.line); + pendingJumpRef.current = null; + if (frag === undefined) return; + const ed = + jump.side === "New" + ? diffEditorRef.current.getModifiedEditor() + : diffEditorRef.current.getOriginalEditor(); + ed.revealLineInCenter(frag); + ed.setPosition({ lineNumber: frag, column: 1 }); + }, [selectedFile, effectiveFileDiff, editorReady, jumpNonce]); + + const handleJumpTo = useCallback( + (path: string, side: DiffSide, line: number) => { + pendingJumpRef.current = { path, side, line }; + setSelectedFile(path); + setJumpNonce((n) => n + 1); + }, + [], + ); const ciBadgeState = useMemo( () => (ciSummary !== null ? ciState(ciSummary) : "none"), @@ -1159,6 +1547,18 @@ export function DiffView({ } }, [approveReview, review.pr]); + // -- B2: run the advisory read-only pre-pass reviewer. Advisory only — never + // touches the gate; refuses while another agent is attached (enforced core- + // side too). The header's "Agent working" line keys off review.agent. -- + const handlePreReview = useCallback(async () => { + setPreReviewing(true); + try { + await preReview(review.pr); + } finally { + setPreReviewing(false); + } + }, [preReview, review.pr]); + // -- D2: merge is a guarded, confirmed side effect (Invariant 5 / §9). -- const handleMerge = useCallback(async () => { const confirmed = window.confirm( @@ -1376,6 +1776,45 @@ export function DiffView({ + {/* Hunks / Full-file toggle (B4). The interdiff is exempt, so the + toggle hides while the interdiff is the active view. */} + {!interdiffActive && ( +
+ + +
+ )} + {/* Diff-source toggle: interdiff vs full diff (D10). */} {hasInterdiff && (
@@ -1477,6 +1916,22 @@ export function DiffView({ Agent + {/* Pre-review (B2) — advisory read-only pre-pass; any source while + InReview. Disabled while an agent is attached (the header's + "Agent working" line reflects the running Review agent). */} + {review.gate_state === "InReview" && ( + + )} + {/* Restack — explicit user action; only when the review is stale. Operates only on the review's own branch (Invariant 5 / §9). */} {review.stale && ( @@ -1612,6 +2067,15 @@ export function DiffView({ /> )} + {/* ----------------------------------------------------------------- */} + {/* Evidence strip — deterministic review signals (B3) */} + {/* ----------------------------------------------------------------- */} + + {/* ----------------------------------------------------------------- */} {/* Addressed requests — read-only interdiff history (D10) */} {/* ----------------------------------------------------------------- */} @@ -1881,10 +2345,20 @@ export function DiffView({ {/* Monaco Diff Editor */}
+ {/* B4: full-file was requested but could not be resolved — fall back + to the changed hunks with a subtle note. */} + {viewMode === "full" && + !interdiffActive && + fullPair !== null && + !fullPair.full && ( +
+ Full file unavailable — showing changed hunks +
+ )} {selectedFile !== "" ? ( + createPortal( + { + setDismissedFindings((prev) => { + const next = new Set(prev); + next.add(entry.finding.id); + return next; + }); + }} + />, + entry.domNode, + entry.key, + ), + )}
diff --git a/app/src/components/EvidenceStrip.test.tsx b/app/src/components/EvidenceStrip.test.tsx new file mode 100644 index 0000000..2e41026 --- /dev/null +++ b/app/src/components/EvidenceStrip.test.tsx @@ -0,0 +1,84 @@ +import { describe, it, expect, vi } from "vitest"; +import { render, screen, fireEvent } from "@testing-library/react"; +import { EvidenceStrip } from "./EvidenceStrip"; +import type { EvidenceSummary } from "../bindings/EvidenceSummary"; +import type { CiCheck } from "../bindings/CiCheck"; +import { makeCheck } from "../test/fixtures"; + +/** An evidence bundle exercising every chip kind. */ +function makeEvidence(overrides: Partial = {}): EvidenceSummary { + return { + signals: { + additions: 120, + deletions: 30, + files_changed: 4, + size_class: "M", + test_delta: { + test_files_changed: 1, + assertions_added: 2, + assertions_removed: 1, + }, + risk_paths: [{ flag: "Migration", path: "db/migrations/001_init.sql" }], + weakening: [ + { + kind: "DeletedAssertion", + path: "tests/foo.rs", + line: 3, + side: "Old", + excerpt: "assert_eq!(x, 42);", + }, + ], + }, + ci: { passed: 3, total: 4, failed: 1, pending: 0 }, + agent_ran: [{ command: "cargo test", ok: true }], + ...overrides, + }; +} + +describe("EvidenceStrip", () => { + it("renders nothing when evidence is null", () => { + const { container } = render(); + expect(container).toBeEmptyDOMElement(); + }); + + it("renders a chip for each evidence kind", () => { + const checks: CiCheck[] = [ + makeCheck({ name: "build", workflow: "Nightly", bucket: "fail" }), + ]; + render(); + + // CI rollup + the failing workflow name. + expect(screen.getByText("3/4")).toBeInTheDocument(); + expect(screen.getByText("CI")).toBeInTheDocument(); + expect(screen.getByText("Nightly")).toBeInTheDocument(); + // Size chip. + expect(screen.getByText("M")).toBeInTheDocument(); + expect(screen.getByText("4 files")).toBeInTheDocument(); + // Test delta. + expect(screen.getByText("Tests")).toBeInTheDocument(); + expect(screen.getByText("+2")).toBeInTheDocument(); + // Risk path. + expect(screen.getByText("Migration")).toBeInTheDocument(); + // Weakening flag. + expect(screen.getByText("Deleted assertion")).toBeInTheDocument(); + // Agent command. + expect(screen.getByText("cargo test")).toBeInTheDocument(); + }); + + it("omits the CI chip when the bundle has no CI", () => { + render(); + expect(screen.queryByText("CI")).not.toBeInTheDocument(); + // The size chip is still present. + expect(screen.getByText("M")).toBeInTheDocument(); + }); + + it("jumps to the offending hunk when a weakening chip is clicked", () => { + const onJumpTo = vi.fn(); + render(); + + fireEvent.click( + screen.getByRole("button", { name: /Deleted assertion/ }), + ); + expect(onJumpTo).toHaveBeenCalledWith("tests/foo.rs", "Old", 3); + }); +}); diff --git a/app/src/components/EvidenceStrip.tsx b/app/src/components/EvidenceStrip.tsx new file mode 100644 index 0000000..f0344c8 --- /dev/null +++ b/app/src/components/EvidenceStrip.tsx @@ -0,0 +1,339 @@ +/** + * Evidence strip: one glanceable row of deterministic review signals rendered + * above the diff (B3). + * + * Surfaces the [`EvidenceSummary`] the backend assembles per review — CI rollup, + * test delta, diff size, risk paths, suspected test-weakening, and the commands + * the agent ran — as compact chips. Each chip carries a status dot AND a text + * label (never color alone) and uses tabular-nums mono for its telemetry. + * Weakening chips are actionable: clicking one jumps the diff to the offending + * hunk via {@link EvidenceStripProps.onJumpTo}. + * + * Renders nothing when `evidence` is null so a failed/absent evidence fetch + * degrades gracefully. + */ + +import type { EvidenceSummary } from "../bindings/EvidenceSummary"; +import type { CiSummary } from "../bindings/CiSummary"; +import type { CiCheck } from "../bindings/CiCheck"; +import type { RiskFlag } from "../bindings/RiskFlag"; +import type { SizeClass } from "../bindings/SizeClass"; +import type { WeakeningKind } from "../bindings/WeakeningKind"; +import type { DiffSide } from "../bindings/DiffSide"; +import { checkOutcome } from "@/lib/ci"; +import { cn } from "@/lib/utils"; +import { Gauge, ShieldAlert, TriangleAlert } from "lucide-react"; + +// --------------------------------------------------------------------------- +// Props +// --------------------------------------------------------------------------- + +interface EvidenceStripProps { + /** The evidence bundle; when null the strip renders nothing. */ + readonly evidence: EvidenceSummary | null; + /** + * The raw CI checks DiffView already fetches, used only to name the failing + * workflow/job on the CI chip. The x/y count comes from `evidence.ci`. + */ + readonly ciChecks?: readonly CiCheck[]; + /** + * Jump the diff to a line, side-aware. Wired for weakening chips so a + * suspected weakening jumps straight to its hunk. + */ + readonly onJumpTo?: (path: string, side: DiffSide, line: number) => void; +} + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function assertNever(x: never): never { + throw new Error(`unreachable: ${String(x)}`); +} + +/** Semantic tone driving a chip's status-dot color. */ +type ChipTone = "pass" | "fail" | "pending" | "warning" | "neutral"; + +function toneDotClass(tone: ChipTone): string { + switch (tone) { + case "pass": + return "bg-success"; + case "fail": + return "bg-danger"; + case "pending": + return "bg-warning"; + case "warning": + return "bg-warning"; + case "neutral": + return "bg-muted-foreground"; + default: + return assertNever(tone); + } +} + +/** Human label for a diff size bucket. */ +function sizeLabel(size: SizeClass): string { + switch (size) { + case "S": + return "S"; + case "M": + return "M"; + case "L": + return "L"; + case "Xl": + return "XL"; + default: + return assertNever(size); + } +} + +/** Larger diffs read as more caution-worthy. */ +function sizeTone(size: SizeClass): ChipTone { + switch (size) { + case "S": + case "M": + return "neutral"; + case "L": + return "warning"; + case "Xl": + return "fail"; + default: + return assertNever(size); + } +} + +/** Human label for a risk-path flag. */ +function riskLabel(flag: RiskFlag): string { + switch (flag) { + case "Migration": + return "Migration"; + case "Lockfile": + return "Lockfile"; + case "CiConfig": + return "CI config"; + case "Auth": + return "Auth"; + case "GithubDir": + return ".github"; + case "Dependency": + return "Dependency"; + default: + return assertNever(flag); + } +} + +/** Human label for a suspected test-weakening. */ +function weakeningLabel(kind: WeakeningKind): string { + switch (kind) { + case "DeletedAssertion": + return "Deleted assertion"; + case "IgnoreAdded": + return "#[ignore] added"; + case "OrTrue": + return "|| true added"; + case "SkipOrTodo": + return "Skip / only"; + case "DeletedTestFn": + return "Deleted test fn"; + case "DeletedTestFile": + return "Deleted test file"; + case "SnapshotRewrite": + return "Snapshot rewrite"; + default: + return assertNever(kind); + } +} + +// --------------------------------------------------------------------------- +// Chip +// --------------------------------------------------------------------------- + +function Chip({ + tone, + title, + onClick, + children, +}: { + readonly tone: ChipTone; + readonly title?: string | undefined; + readonly onClick?: (() => void) | undefined; + readonly children: React.ReactNode; +}) { + const dot = ( +