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/app/src-tauri/src/commands/mod.rs b/app/src-tauri/src/commands/mod.rs index 3325442..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, Comment, CommentId, CommentOrigin, DiffData, DiffSide, GateState, - PlanDoc, PrRef, Project, ProjectId, ProjectPlan, ProjectRef, Review, ReviewSource, + AgentMode, Anchor, Artifact, CiSummary, Comment, CommentId, CommentOrigin, DiffData, DiffSide, + 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 // --------------------------------------------------------------------------- @@ -647,13 +945,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..a554378 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -167,6 +167,13 @@ pub fn run() { } "completed" } + AgentMode::Review => { + // 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) + } }; // Best-effort: if no frontend window is listening, the @@ -253,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, @@ -502,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 @@ -549,6 +647,13 @@ async fn refresh_review_diff(state: &AppState, pr_ref: &PrRef) { None => github::pr_diff(pr_number).await, }; + // The advisory findings were anchored to the previous diff/head; the head + // has already moved by the time a refresh is requested, so drop them even + // when the re-fetch fails (stale pins on changed content mislead). + state.reviews.update(pr_ref, |review| { + review.review_findings.clear(); + }); + if let Ok(raw_diff) = diff_result { state.reviews.update(pr_ref, |review| { review.diff = DiffData { raw: raw_diff }; 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/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/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/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/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/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/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/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/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/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/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 = ( +