From 4ecbd6e815b2c6ff29657a422256ebbd5b69c4a4 Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Fri, 3 Jul 2026 02:43:12 +0100 Subject: [PATCH 1/2] feat: pre-review recap contract (core + app) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let the advisory pre-review agent produce a /visual-recap-style briefing that cockpit can render, gated by diff size / rendered-UI touch. core: - config: recaps_dir() + recap_dir_for_pr(&PrRef) (findings slug scheme); RECAP_MDX_FILE const shared by prompt/ingest/read. - diff_signals: should_run_review_skills(signals, diff) — true for L/XL or a diff touching {tsx,jsx,css,scss,vue,svelte,html}; unit-tested. - prompt: ReviewInput gains pipeline_skills + recap_dir; assemble_review_prompt appends a Visual Recap section after Output only when both are set. Inactive output is byte-identical to before (asserted), so review_prompt.txt and review_prompt_panel.txt are UNCHANGED; added review_prompt_recap.txt for the active section. - model: Review.recap_sha (Option, serde default, `string | null` binding). Deliberately NOT cleared on refresh. - recap: new module reading recap.mdx + *.excalidraw scenes (sorted, 2 MiB cap, fail-open) into RecapPayload/RecapScene. app: - start_pre_review: create+authorize recap dir, resolve in_review_pipeline skills (fail-open), compute the gate, pass both into the prompt. - ingest_review_findings: set recap_sha = current head_sha when a non-empty recap.mdx exists; recap files are never deleted. refresh preserves recap_sha (extracted clear_head_anchored_annotations helper, unit-tested). - get_review_recap command (registered) → RecapPayload. - regenerated bindings; updated Review fixture. Co-Authored-By: Claude Fable 5 --- Cargo.lock | 2 + app/src-tauri/Cargo.toml | 4 + app/src-tauri/src/commands/mod.rs | 119 ++++++++- app/src-tauri/src/lib.rs | 208 ++++++++++++++- app/src/bindings/RecapPayload.ts | 25 ++ app/src/bindings/RecapScene.ts | 16 ++ app/src/bindings/Review.ts | 13 +- app/src/test/fixtures.ts | 1 + crates/cockpit-core/src/adapters/github.rs | 2 + crates/cockpit-core/src/config.rs | 65 ++++- crates/cockpit-core/src/diff_signals.rs | 80 ++++++ crates/cockpit-core/src/gate.rs | 1 + crates/cockpit-core/src/kickoff.rs | 3 + crates/cockpit-core/src/lib.rs | 1 + crates/cockpit-core/src/model.rs | 11 + crates/cockpit-core/src/persist.rs | 1 + crates/cockpit-core/src/prompt.rs | 245 +++++++++++++++++- crates/cockpit-core/src/recap.rs | 206 +++++++++++++++ crates/cockpit-core/src/restack.rs | 1 + crates/cockpit-core/src/store.rs | 1 + .../cockpit-core/tests/batch_fan_out_e2e.rs | 1 + crates/cockpit-core/tests/e2e_round_trip.rs | 1 + crates/cockpit-core/tests/fix_loop_e2e.rs | 1 + .../tests/golden/review_prompt_recap.txt | 46 ++++ 24 files changed, 1040 insertions(+), 14 deletions(-) create mode 100644 app/src/bindings/RecapPayload.ts create mode 100644 app/src/bindings/RecapScene.ts create mode 100644 crates/cockpit-core/src/recap.rs create mode 100644 crates/cockpit-core/tests/golden/review_prompt_recap.txt diff --git a/Cargo.lock b/Cargo.lock index 78fe9e4..36d19ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -563,6 +563,8 @@ dependencies = [ "tauri-plugin-dialog", "tauri-plugin-notification", "tauri-plugin-opener", + "temp-env", + "tempfile", "tokio", "uuid", ] diff --git a/app/src-tauri/Cargo.toml b/app/src-tauri/Cargo.toml index e257b04..2db5a41 100644 --- a/app/src-tauri/Cargo.toml +++ b/app/src-tauri/Cargo.toml @@ -24,5 +24,9 @@ objc2 = "0.6" objc2-app-kit = { version = "0.3", features = ["NSApplication", "NSImage", "NSRunningApplication"] } objc2-foundation = { version = "0.3", features = ["NSData"] } +[dev-dependencies] +tempfile.workspace = true +temp-env = { version = "0.3.6", features = ["async_closure"] } + [build-dependencies] tauri-build.workspace = true diff --git a/app/src-tauri/src/commands/mod.rs b/app/src-tauri/src/commands/mod.rs index 27850cf..342d8a6 100644 --- a/app/src-tauri/src/commands/mod.rs +++ b/app/src-tauri/src/commands/mod.rs @@ -396,6 +396,37 @@ pub async fn get_trajectory_summary(pr: String) -> Result>, + 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 sha = review.recap_sha.clone(); + let dir = cockpit_core::config::recap_dir_for_pr(&pr_ref)?; + let payload = tokio::task::spawn_blocking(move || cockpit_core::recap::read_recap(&dir, sha)) + .await + .map_err(|e| CommandError { + message: format!("recap read task panicked: {e}"), + })?; + Ok(payload) +} + /// 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 @@ -956,6 +987,51 @@ fn file_extension(path: &str) -> Option<&str> { name.rsplit_once('.').map(|(_, ext)| ext) } +/// Resolve the review-pipeline skills as `(name, description)` pairs. +/// +/// Reads the discovered skills and keeps those flagged into the review pipeline +/// ([`Skill::in_review_pipeline`](cockpit_core::skills::Skill::in_review_pipeline)). +/// FAIL-OPEN: any discovery error yields an empty list so the recap is skipped +/// rather than blocking the review (Invariant §0.1). Order follows discovery +/// (deterministic) so the prompt stays stable. +fn resolve_pipeline_skills() -> Vec<(String, String)> { + match cockpit_core::skills::discover_installed_skills() { + Ok(skills) => skills + .into_iter() + .filter(|s| s.in_review_pipeline) + .map(|s| (s.name, s.description)) + .collect(), + Err(e) => { + eprintln!("start_pre_review: skill discovery failed (recap skipped): {e}"); + Vec::new() + } + } +} + +/// Resolve and create the recap directory for `pr_ref`, returning it on success. +/// +/// Returns `None` (recap skipped) when the path cannot be resolved or the +/// directory cannot be created — a recap is best-effort and never fails the +/// review. +fn prepare_recap_dir(pr_ref: &PrRef) -> Option { + match cockpit_core::config::recap_dir_for_pr(pr_ref) { + Ok(dir) => match std::fs::create_dir_all(&dir) { + Ok(()) => Some(dir), + Err(e) => { + eprintln!( + "start_pre_review: create recap dir {} failed: {e}", + dir.display() + ); + None + } + }, + Err(e) => { + eprintln!("start_pre_review: recap dir path for {pr_ref}: {e}"); + None + } + } +} + /// Spawn the advisory read-only pre-pass reviewer for a PR (shared core of B2). /// /// This is the single spawn path used by both the explicit [`pre_review`] @@ -1030,6 +1106,23 @@ pub(crate) async fn start_pre_review( })?; let lenses = review_lenses(&signals, &review.diff.raw); + // Resolve the review-pipeline skills and the lens gate to decide whether to + // ask the reviewer for a visual recap. Skill resolution is FAIL-OPEN: any + // discovery error yields an empty list and the recap is simply skipped — + // skills must never block the review (Invariant §0.1). + let pipeline_skills = resolve_pipeline_skills(); + let want_recap = !pipeline_skills.is_empty() + && cockpit_core::diff_signals::should_run_review_skills(&signals, &review.diff.raw); + // When the gate passes, create the recap dir up front so the agent's writes + // land, and pass it into the prompt (its presence is what activates the + // recap section). A path/creation failure degrades to "no recap" rather than + // failing the review. + let recap_dir = if want_recap { + prepare_recap_dir(&pr_ref) + } else { + None + }; + let review_input = cockpit_core::prompt::ReviewInput { title: &review.title, body: &review.body, @@ -1039,6 +1132,8 @@ pub(crate) async fn start_pre_review( lenses: &lenses, output_path: Some(&findings_path), skills: &skills, + pipeline_skills: &pipeline_skills, + recap_dir: recap_dir.as_deref(), }; let assembled = cockpit_core::prompt::assemble_review_prompt(&review_input); @@ -1046,11 +1141,19 @@ pub(crate) async fn start_pre_review( // 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}"), - })?; + let agent_run = try_spawn_review_agent( + state, + app_handle, + pr, + &pr_ref, + &worktree, + &assembled, + recap_dir.as_deref(), + ) + .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); @@ -1074,6 +1177,7 @@ async fn try_spawn_review_agent( pr_ref: &PrRef, worktree: &std::path::Path, prompt: &cockpit_core::prompt::AssembledPrompt, + recap_dir: Option<&Path>, ) -> Result { let config = Config::load().map_err(|e| format!("config: {e}"))?; // The findings file lives outside the worktree; a headless session can't @@ -1088,6 +1192,11 @@ async fn try_spawn_review_agent( .apply_permission_mode(config.agent_permission_mode, Some(&approve_url)) .with_extra_dir(&findings_dir) .allow_write_under(&findings_dir); + // The recap dir (when the gate opted in) also lives outside the worktree, so + // pre-authorize the agent's recap.mdx / *.excalidraw writes there the same way. + if let Some(dir) = recap_dir { + spawn_config = spawn_config.with_extra_dir(dir).allow_write_under(dir); + } // Run the advisory pre-review on the configured override model when set (a // faster/cheaper tier trades reviewer depth for latency); empty falls back // to the CLI's default. This applies to the read-only reviewer only. diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 4e834a5..4f93468 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -329,6 +329,7 @@ pub fn run() { commands::fetch_conversation, commands::get_evidence, commands::get_trajectory_summary, + commands::get_review_recap, commands::get_file_pair, commands::pre_review, commands::add_comment, @@ -653,9 +654,18 @@ fn ingest_review_findings(state: &AppState, object_id: &str) -> &'static str { "failed" }; + // Detect a fresh recap: the pre-review agent writes recap.mdx into the PR's + // recap dir when the lens gate ran the review-pipeline skills (keyed by the + // same object id used for findings). Unlike findings, recap files are a + // durable artifact the Briefing reads — they are NEVER deleted here. The + // filesystem probe is done before the lock so no I/O runs under it. + let recap_present = recap_mdx_present(object_id); + // Always clear the running agent; store the findings and distilled intent on // a successful parse. Intent is only overwritten when present (a legacy - // bare-array output keeps the previous intent). + // bare-array output keeps the previous intent). When a recap was produced, + // pin its sha to the review's current head so the Briefing can compute + // staleness by comparing against later pushes. // INVARIANT: gate_state is never touched here (read-only pre-pass). state.reviews.update(&pr_ref, |r| { r.agent = None; @@ -665,6 +675,9 @@ fn ingest_review_findings(state: &AppState, object_id: &str) -> &'static str { r.distilled_intent = Some(intent); } } + if recap_present { + r.recap_sha = Some(r.head_sha.clone()); + } }); // The findings file is a transport, not a store — delete it after ingest. @@ -683,6 +696,24 @@ fn ingest_review_findings(state: &AppState, object_id: &str) -> &'static str { outcome } +/// Whether a non-empty `recap.mdx` exists in the recap dir for `object_id`. +/// +/// The recap dir is keyed by the same object id the pre-review was spawned with +/// (the PR ref). A missing dir/file, a read error, or a whitespace-only file all +/// yield `false` — a recap is only counted when it has real content. Best-effort +/// and non-fatal (Invariant §0.1). +fn recap_mdx_present(object_id: &str) -> bool { + let pr_ref = PrRef::new(object_id); + let Ok(dir) = cockpit_core::config::recap_dir_for_pr(&pr_ref) else { + return false; + }; + let path = dir.join(cockpit_core::config::RECAP_MDX_FILE); + match std::fs::read_to_string(&path) { + Ok(contents) => !contents.trim().is_empty(), + Err(_) => false, + } +} + /// 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 @@ -733,11 +764,11 @@ async fn refresh_review_diff(state: &AppState, pr_ref: &PrRef) { // The advisory findings + distilled intent 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/read on changed - // content mislead). - state.reviews.update(pr_ref, |review| { - review.review_findings.clear(); - review.distilled_intent = None; - }); + // content mislead). `recap_sha` is deliberately preserved — see + // [`clear_head_anchored_annotations`]. + state + .reviews + .update(pr_ref, clear_head_anchored_annotations); if let Ok(raw_diff) = diff_result { state.reviews.update(pr_ref, |review| { @@ -746,6 +777,19 @@ async fn refresh_review_diff(state: &AppState, pr_ref: &PrRef) { } } +/// Clear the review annotations that were anchored to the previous head. +/// +/// The advisory findings and distilled intent are pinned to a specific diff/head +/// and mislead once the head moves, so a refresh drops them. The recap pointer +/// (`recap_sha`) is DELIBERATELY left intact: the Briefing derives recap +/// staleness by comparing it to the new head, so clearing it would hide the +/// "regenerate" nudge. Kept as a named helper so this invariant is unit-tested +/// without needing to drive the network-backed [`refresh_review_diff`]. +fn clear_head_anchored_annotations(review: &mut Review) { + review.review_findings.clear(); + review.distilled_intent = None; +} + /// Extract the PR number from a PR URL or reference string. /// /// Handles formats like `https://github.com/owner/repo/pull/42` and @@ -1341,4 +1385,156 @@ mod tests { assert_eq!(summary, "ls"); assert!(!summary.ends_with('…')); } + + // ----------------------------------------------------------------- + // Recap ingest + refresh (PR 3) + // ----------------------------------------------------------------- + + /// Build a minimal review for the recap tests, with the given PR and head. + fn recap_test_review(pr: &str, head_sha: &str) -> Review { + use cockpit_core::model::{DiffData, IssueRef, ReviewId}; + Review { + id: ReviewId::new("r-recap"), + issue: IssueRef::new("NEX-1"), + pr: PrRef::new(pr), + title: "t".into(), + body: "b".into(), + branch: "alejandro/recap".into(), + base: "main".into(), + base_sha: "000".into(), + source: ReviewSource::Frontier, + worktree: std::path::PathBuf::from("/tmp/wt-recap"), + gate_state: GateState::InReview, + diff: DiffData { raw: String::new() }, + head_sha: head_sha.into(), + comments: vec![], + parents: vec![], + children: vec![], + stale: false, + agent: None, + repo_slug: None, + project: None, + dispatch_snapshot: None, + ci_summary: None, + review_findings: vec![], + conversation: vec![], + last_reviewed_sha: None, + distilled_intent: None, + recap_sha: None, + } + } + + /// Build an [`AppState`] with a throwaway completion channel + broker. + fn recap_test_state() -> AppState { + let (tx, _rx) = tokio::sync::broadcast::channel(16); + let broker = cockpit_core::hook_server::PermissionBroker::new(); + AppState::new_with_completion_tx(tx, broker) + } + + #[test] + fn ingest_sets_recap_sha_when_recap_mdx_present() { + let dir = tempfile::tempdir().expect("tempdir"); + temp_env::with_var("COCKPIT_HOME", Some(dir.path().as_os_str()), || { + let state = recap_test_state(); + let pr = "owner/repo#7"; + state.reviews.insert(recap_test_review(pr, "headsha7")); + + // Write a non-empty recap.mdx into the PR's recap dir. + let recap_dir = + cockpit_core::config::recap_dir_for_pr(&PrRef::new(pr)).expect("recap dir"); + std::fs::create_dir_all(&recap_dir).expect("mkdir"); + std::fs::write( + recap_dir.join(cockpit_core::config::RECAP_MDX_FILE), + "# Recap\n\nBody.", + ) + .expect("write recap.mdx"); + + ingest_review_findings(&state, pr); + + let review = state.reviews.get(&PrRef::new(pr)).expect("review present"); + assert_eq!( + review.recap_sha.as_deref(), + Some("headsha7"), + "recap_sha must pin to the review's current head" + ); + }); + } + + #[test] + fn ingest_skips_recap_sha_when_recap_mdx_empty() { + let dir = tempfile::tempdir().expect("tempdir"); + temp_env::with_var("COCKPIT_HOME", Some(dir.path().as_os_str()), || { + let state = recap_test_state(); + let pr = "owner/repo#8"; + state.reviews.insert(recap_test_review(pr, "headsha8")); + + // A whitespace-only recap.mdx counts as "nothing produced". + let recap_dir = + cockpit_core::config::recap_dir_for_pr(&PrRef::new(pr)).expect("recap dir"); + std::fs::create_dir_all(&recap_dir).expect("mkdir"); + std::fs::write( + recap_dir.join(cockpit_core::config::RECAP_MDX_FILE), + " \n\t", + ) + .expect("write recap.mdx"); + + ingest_review_findings(&state, pr); + + let review = state.reviews.get(&PrRef::new(pr)).expect("review present"); + assert!( + review.recap_sha.is_none(), + "an empty recap.mdx must not set recap_sha" + ); + }); + } + + #[test] + fn ingest_skips_recap_sha_when_no_recap_dir() { + let dir = tempfile::tempdir().expect("tempdir"); + temp_env::with_var("COCKPIT_HOME", Some(dir.path().as_os_str()), || { + let state = recap_test_state(); + let pr = "owner/repo#10"; + state.reviews.insert(recap_test_review(pr, "headsha10")); + + // No recap dir written at all. + ingest_review_findings(&state, pr); + + let review = state.reviews.get(&PrRef::new(pr)).expect("review present"); + assert!(review.recap_sha.is_none(), "no recap dir → no recap_sha"); + }); + } + + #[test] + fn clear_head_anchored_annotations_preserves_recap_sha() { + use cockpit_core::model::{DiffSide, FindingSeverity, ReviewFinding}; + let mut review = recap_test_review("owner/repo#9", "headsha9"); + review.review_findings = vec![ReviewFinding { + id: "f1".into(), + severity: FindingSeverity::Info, + path: std::path::PathBuf::from("src/a.rs"), + range: (1, 2), + side: DiffSide::New, + title: "t".into(), + rationale: "r".into(), + area: None, + }]; + review.distilled_intent = Some("intent".into()); + review.recap_sha = Some("headsha9".into()); + + clear_head_anchored_annotations(&mut review); + + assert!( + review.review_findings.is_empty(), + "findings cleared on refresh" + ); + assert!( + review.distilled_intent.is_none(), + "intent cleared on refresh" + ); + assert_eq!( + review.recap_sha.as_deref(), + Some("headsha9"), + "recap_sha must survive a refresh so staleness stays derivable" + ); + } } diff --git a/app/src/bindings/RecapPayload.ts b/app/src/bindings/RecapPayload.ts new file mode 100644 index 0000000..8cdcacc --- /dev/null +++ b/app/src/bindings/RecapPayload.ts @@ -0,0 +1,25 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { RecapScene } from "./RecapScene"; + +/** + * The recap artifacts read from a PR's recap directory, for the Briefing tab. + * + * A read never fails loudly: a missing directory or file yields + * `mdx: None, scenes: []` (see [`read_recap`]). + */ +export type RecapPayload = { +/** + * The `recap.mdx` briefing body (GitHub-flavored markdown), or `None` when + * the file is absent or empty. + */ +mdx: string | null, +/** + * Excalidraw scenes discovered in the recap directory, sorted by name. + */ +scenes: Array, +/** + * The head SHA the recap was generated at (the review's `recap_sha`), for + * staleness. Supplied by the caller from persisted review state; `None` + * before any recap. The frontend compares this to the review's `head_sha`. + */ +sha: string | null, }; diff --git a/app/src/bindings/RecapScene.ts b/app/src/bindings/RecapScene.ts new file mode 100644 index 0000000..6fe60ac --- /dev/null +++ b/app/src/bindings/RecapScene.ts @@ -0,0 +1,16 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * A single Excalidraw scene the pre-review agent produced. + */ +export type RecapScene = { +/** + * Scene name: the file's basename with the `.excalidraw` extension removed + * (e.g. `architecture` for `architecture.excalidraw`). Used as a stable + * tab label / React key by the Briefing viewer. + */ +name: string, +/** + * The raw scene JSON, ready to hand to the Excalidraw viewer. + */ +json: string, }; diff --git a/app/src/bindings/Review.ts b/app/src/bindings/Review.ts index 29a70b8..698a28a 100644 --- a/app/src/bindings/Review.ts +++ b/app/src/bindings/Review.ts @@ -145,4 +145,15 @@ last_reviewed_sha: string | null, * invites self-reinforcement). `None` before the first pre-pass and for * legacy data. */ -distilled_intent: string | null, }; +distilled_intent: string | null, +/** + * The head SHA the rendered pre-review recap was generated at, for + * staleness. `None` before any recap and for legacy data. + * + * The recap files themselves live on disk (see [`crate::recap`]); only this + * pointer is persisted. The Briefing compares it to [`Self::head_sha`]: a + * match means the recap is fresh, a mismatch (or `None`) means stale, so the + * tab can nudge a regenerate. Deliberately NOT cleared when the head moves + * (unlike `review_findings` / `distilled_intent`) so staleness is derivable. + */ +recap_sha: string | null, }; diff --git a/app/src/test/fixtures.ts b/app/src/test/fixtures.ts index 29af6fe..f4997d1 100644 --- a/app/src/test/fixtures.ts +++ b/app/src/test/fixtures.ts @@ -59,6 +59,7 @@ export function makeReview(overrides: Partial = {}): Review { ci_summary: null, last_reviewed_sha: null, distilled_intent: null, + recap_sha: null, }; return { ...base, ...overrides }; } diff --git a/crates/cockpit-core/src/adapters/github.rs b/crates/cockpit-core/src/adapters/github.rs index fa4b2d9..167c1ad 100644 --- a/crates/cockpit-core/src/adapters/github.rs +++ b/crates/cockpit-core/src/adapters/github.rs @@ -944,6 +944,7 @@ pub fn build_review_from_pr( conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } @@ -2664,6 +2665,7 @@ index 1111111..2222222 100644 conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } diff --git a/crates/cockpit-core/src/config.rs b/crates/cockpit-core/src/config.rs index fa84d25..045c3fc 100644 --- a/crates/cockpit-core/src/config.rs +++ b/crates/cockpit-core/src/config.rs @@ -9,7 +9,7 @@ use std::path::PathBuf; use serde::{Deserialize, Serialize}; use ts_rs::TS; -use crate::model::AgentMode; +use crate::model::{AgentMode, PrRef}; // --------------------------------------------------------------------------- // Error @@ -553,6 +553,40 @@ pub fn findings_file_path(pr: &str) -> Result { Ok(findings_dir()?.join(format!("{slug}.json"))) } +/// Filename of the markdown briefing the pre-review agent writes into a PR's +/// recap directory. +/// +/// Shared by the prompt (which instructs the agent to write this file), the +/// ingest step (which checks it for freshness), and the read command (which +/// serves it to the Briefing tab) so all three agree on one name. +pub const RECAP_MDX_FILE: &str = "recap.mdx"; + +/// Return the directory that holds pre-review recap artifacts +/// (`/recaps`). +/// +/// Each PR's recap lives in its own subdirectory under here (see +/// [`recap_dir_for_pr`]): a `recap.mdx` briefing plus zero or more +/// `*.excalidraw` scene files, written by the advisory pre-review agent when the +/// lens gate opts it into the review-pipeline skills. Like [`findings_dir`], this +/// only resolves the path — it does not create the directory. +pub fn recaps_dir() -> Result { + Ok(cockpit_home()?.join("recaps")) +} + +/// Return the recap directory for the PR identified by `pr`. +/// +/// The convention is `/recaps//`, where `` is the PR +/// ref with filesystem-hostile characters replaced by `-` (the same +/// sanitization [`findings_file_path`] applies), so an arbitrary PR reference +/// such as `owner/repo#42` yields a safe single directory name. Unlike findings +/// (one file per PR), a recap is a *directory* because it may hold several +/// `.excalidraw` scenes alongside `recap.mdx`. Like [`findings_file_path`], this +/// only resolves the path and does not create the directory. +pub fn recap_dir_for_pr(pr: &PrRef) -> Result { + let slug = path_slug(pr.as_str(), "recap"); + Ok(recaps_dir()?.join(slug)) +} + /// Turn an arbitrary id into a single filesystem-safe filename stem. /// /// Every non-alphanumeric character becomes `-`. An id that reduces to only @@ -976,6 +1010,35 @@ path = "skills" }); } + #[test] + fn recap_dir_for_pr_uses_recaps_dir_and_slug() { + temp_env::with_var("COCKPIT_HOME", Some("/tmp/cockpit-home-test"), || { + let path = recap_dir_for_pr(&PrRef::new("PR-1")).expect("should resolve"); + assert_eq!(path, PathBuf::from("/tmp/cockpit-home-test/recaps/PR-1")); + }); + } + + #[test] + fn recap_dir_for_pr_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 directory name, + // matching the findings slug scheme. + let path = recap_dir_for_pr(&PrRef::new("owner/repo#42")).expect("should resolve"); + assert_eq!( + path, + PathBuf::from("/tmp/cockpit-home-test/recaps/owner-repo-42") + ); + }); + } + + #[test] + fn recap_dir_for_pr_falls_back_for_empty_slug() { + temp_env::with_var("COCKPIT_HOME", Some("/tmp/cockpit-home-test"), || { + let path = recap_dir_for_pr(&PrRef::new("###")).expect("should resolve"); + assert_eq!(path, PathBuf::from("/tmp/cockpit-home-test/recaps/recap")); + }); + } + #[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/diff_signals.rs b/crates/cockpit-core/src/diff_signals.rs index 6f09573..0745223 100644 --- a/crates/cockpit-core/src/diff_signals.rs +++ b/crates/cockpit-core/src/diff_signals.rs @@ -241,6 +241,44 @@ pub fn compute_diff_signals(diff: &str) -> DiffSignals { builder.finish() } +// --------------------------------------------------------------------------- +// Review-skills lens gate +// --------------------------------------------------------------------------- + +/// Changed-file extensions that indicate rendered UI, used by +/// [`should_run_review_skills`]. A diff touching any of these is a visual change +/// worth a briefing even when it is small. +const RENDERED_UI_EXTS: [&str; 7] = ["tsx", "jsx", "css", "scss", "vue", "svelte", "html"]; + +/// Whether the advisory pre-review should also invoke the review-pipeline skills +/// (the visual-recap contract) for this diff. +/// +/// Deterministic and a pure function of the diff: `true` when the diff is large +/// ([`SizeClass::L`] or [`SizeClass::Xl`]) OR touches rendered UI (a changed +/// file whose extension is in [`RENDERED_UI_EXTS`]). This makes the UI promise +/// — "run this skill when the diff is large or touches rendered UI" — literally +/// true. Callers pair a `true` result with a non-empty pipeline-skill list +/// before asking the reviewer for a recap. +pub fn should_run_review_skills(signals: &DiffSignals, diff: &str) -> bool { + matches!(signals.size_class, SizeClass::L | SizeClass::Xl) || touches_rendered_ui(diff) +} + +/// Whether any changed file in the diff has a rendered-UI extension. +fn touches_rendered_ui(diff: &str) -> bool { + crate::skills::changed_files_from_diff(diff) + .iter() + .any(|path| has_rendered_ui_ext(path)) +} + +/// Whether `path`'s extension is a rendered-UI extension (case-insensitive). +fn has_rendered_ui_ext(path: &str) -> bool { + let name = file_name(path); + name.rsplit_once('.').is_some_and(|(_, ext)| { + let ext = ext.to_ascii_lowercase(); + RENDERED_UI_EXTS.contains(&ext.as_str()) + }) +} + // --------------------------------------------------------------------------- // Single-pass builder // --------------------------------------------------------------------------- @@ -995,6 +1033,48 @@ mod tests { assert_eq!(s.test_delta.test_files_changed, 0); } + // -- should_run_review_skills (lens gate) ----------------------------- + + #[test] + fn review_skills_gate_fires_on_large_diff() { + // A large (L) diff to a non-UI file trips the size trigger alone. + let diff = diff_adding(200); + let signals = compute_diff_signals(&diff); + assert_eq!(signals.size_class, SizeClass::L); + assert!(should_run_review_skills(&signals, &diff)); + } + + #[test] + fn review_skills_gate_fires_on_xl_diff() { + let diff = diff_adding(600); + let signals = compute_diff_signals(&diff); + assert_eq!(signals.size_class, SizeClass::Xl); + assert!(should_run_review_skills(&signals, &diff)); + } + + #[test] + fn review_skills_gate_fires_on_small_ui_diff() { + // A tiny (S) diff still fires when it touches rendered UI. + for path in ["src/App.tsx", "src/a.jsx", "styles/x.css", "ui/y.scss"] { + let diff = touch(path); + let signals = compute_diff_signals(&diff); + assert_eq!(signals.size_class, SizeClass::S, "{path} should be small"); + assert!( + should_run_review_skills(&signals, &diff), + "{path} touches rendered UI and must trip the gate" + ); + } + } + + #[test] + fn review_skills_gate_stays_shut_on_small_non_ui_diff() { + // A tiny backend-only diff must not trip the gate. + let diff = touch("src/main.rs"); + let signals = compute_diff_signals(&diff); + assert_eq!(signals.size_class, SizeClass::S); + assert!(!should_run_review_skills(&signals, &diff)); + } + #[test] fn empty_diff_is_zeroed() { let s = compute_diff_signals(""); diff --git a/crates/cockpit-core/src/gate.rs b/crates/cockpit-core/src/gate.rs index a38d937..1e6e291 100644 --- a/crates/cockpit-core/src/gate.rs +++ b/crates/cockpit-core/src/gate.rs @@ -340,6 +340,7 @@ mod tests { conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } diff --git a/crates/cockpit-core/src/kickoff.rs b/crates/cockpit-core/src/kickoff.rs index 1c0fe71..f2ebf47 100644 --- a/crates/cockpit-core/src/kickoff.rs +++ b/crates/cockpit-core/src/kickoff.rs @@ -206,6 +206,7 @@ fn build_review( conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } @@ -837,6 +838,7 @@ mod tests { conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, }]; wire_children(&mut reviews); @@ -872,6 +874,7 @@ mod tests { conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, }; let prompt = assemble_implement_prompt(&review, &ProjectRef::new("proj-1"), None, None); diff --git a/crates/cockpit-core/src/lib.rs b/crates/cockpit-core/src/lib.rs index 2634d9f..2bebe04 100644 --- a/crates/cockpit-core/src/lib.rs +++ b/crates/cockpit-core/src/lib.rs @@ -22,6 +22,7 @@ pub mod kickoff; pub mod persist; pub mod plan_parser; pub mod prompt; +pub mod recap; pub mod restack; pub mod skills; pub mod store; diff --git a/crates/cockpit-core/src/model.rs b/crates/cockpit-core/src/model.rs index f4e9395..9f0f043 100644 --- a/crates/cockpit-core/src/model.rs +++ b/crates/cockpit-core/src/model.rs @@ -559,6 +559,16 @@ pub struct Review { /// legacy data. #[serde(default)] pub distilled_intent: Option, + /// The head SHA the rendered pre-review recap was generated at, for + /// staleness. `None` before any recap and for legacy data. + /// + /// The recap files themselves live on disk (see [`crate::recap`]); only this + /// pointer is persisted. The Briefing compares it to [`Self::head_sha`]: a + /// match means the recap is fresh, a mismatch (or `None`) means stale, so the + /// tab can nudge a regenerate. Deliberately NOT cleared when the head moves + /// (unlike `review_findings` / `distilled_intent`) so staleness is derivable. + #[serde(default)] + pub recap_sha: Option, } // --------------------------------------------------------------------------- @@ -598,6 +608,7 @@ mod tests { conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } diff --git a/crates/cockpit-core/src/persist.rs b/crates/cockpit-core/src/persist.rs index 6d01f8d..3baf3ee 100644 --- a/crates/cockpit-core/src/persist.rs +++ b/crates/cockpit-core/src/persist.rs @@ -154,6 +154,7 @@ mod tests { conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } diff --git a/crates/cockpit-core/src/prompt.rs b/crates/cockpit-core/src/prompt.rs index 794f7b9..c71b616 100644 --- a/crates/cockpit-core/src/prompt.rs +++ b/crates/cockpit-core/src/prompt.rs @@ -9,6 +9,7 @@ use std::sync::LazyLock; use sha2::{Digest, Sha256}; +use crate::config::RECAP_MDX_FILE; use crate::model::{AgentMode, Anchor, Artifact, Comment, DiffData, PlanDoc}; use crate::skills::Skill; @@ -428,6 +429,23 @@ pub struct ReviewInput<'a> { /// /// Pass an empty slice to omit the conventions section. pub skills: &'a [Skill], + /// Review-pipeline skills the agent should *invoke by name* to produce a + /// visual recap, as `(name, description)` pairs. + /// + /// These are distinct from [`Self::skills`]: skills are injected as + /// conventions text, whereas these are Claude Code skills already available + /// in the agent's environment (e.g. `"visual-recap"`) that the recap section + /// tells it to run against the diff. An empty slice omits the recap section + /// entirely. Callers resolve these from the configured review pipeline. + pub pipeline_skills: &'a [(String, String)], + /// Absolute directory the recap outputs (`recap.mdx` + `*.excalidraw`) must + /// be written to, present only when the lens gate + /// ([`crate::diff_signals::should_run_review_skills`]) passed. + /// + /// `None` omits the recap section, so the emitted prompt is byte-identical to + /// output produced before the recap contract existed. The section is emitted + /// only when this is `Some` AND [`Self::pipeline_skills`] is non-empty. + pub recap_dir: Option<&'a std::path::Path>, } /// Assemble a deterministic advisory-review prompt per `SPEC.md` §9. @@ -439,7 +457,10 @@ pub struct ReviewInput<'a> { /// 3. Instruction (the read-only reviewer contract; single-pass or the /// subagent-panel variant depending on `input.lenses` — see /// [`render_review_instruction`]) -/// 4. Output (the findings file path; omitted when no path is supplied) +/// 4. Output (the findings file path; omitted when no path is supplied), then +/// the Visual Recap section (emitted only when `recap_dir` is `Some` AND +/// `pipeline_skills` is non-empty — see [`write_recap_section`]; otherwise +/// the output is byte-identical to a prompt with no recap contract) /// 5. Project conventions (skills) /// /// Same inputs always produce the same output. This is the read-only @@ -481,6 +502,9 @@ pub fn assemble_review_prompt(input: &ReviewInput<'_>) -> AssembledPrompt { .unwrap(); } + // §4b — Visual recap (only when the gate passed and pipeline skills exist). + write_recap_section(&mut text, input.pipeline_skills, input.recap_dir); + // §5 — Project conventions (skills) let conventions = crate::skills::format_for_prompt(input.skills); if !conventions.is_empty() { @@ -492,6 +516,62 @@ pub fn assemble_review_prompt(input: &ReviewInput<'_>) -> AssembledPrompt { AssembledPrompt { text, hash } } +/// Write the visual-recap instruction section, if the recap contract is active. +/// +/// Emits nothing — keeping the prompt byte-identical to the pre-recap output — +/// unless `recap_dir` is `Some` (the lens gate passed) AND `skills` is non-empty +/// (there is a pipeline skill to invoke). When active it names each pipeline +/// skill for the agent to invoke against the diff and points every output at the +/// absolute recap directory. The text is fixed and deterministic: the only +/// variable parts are the skill list (caller order) and the directory path. +fn write_recap_section( + text: &mut String, + skills: &[(String, String)], + recap_dir: Option<&std::path::Path>, +) { + let Some(dir) = recap_dir else { + return; + }; + if skills.is_empty() { + return; + } + + // INVARIANT: all writeln!/write! target a String, whose fmt::Write is infallible. + writeln!(text, "## Visual Recap\n").unwrap(); + writeln!( + text, + "After writing your findings, produce a visual briefing for the human reviewer. \ +Invoke each of these review-pipeline skills by name — they are Claude Code skills already \ +available in your environment — against this PR's diff:\n" + ) + .unwrap(); + for (name, description) in skills { + if description.trim().is_empty() { + writeln!(text, "- `{name}`").unwrap(); + } else { + writeln!(text, "- `{name}`: {description}").unwrap(); + } + } + writeln!(text).unwrap(); + writeln!(text, "Write all recap outputs into `{}`:", dir.display()).unwrap(); + writeln!( + text, + "- `{RECAP_MDX_FILE}` — the briefing as GitHub-flavored markdown (headings, tables, fenced diff blocks)." + ) + .unwrap(); + writeln!( + text, + "- any number of `*.excalidraw` scene files for diagrams a skill produces.\n" + ) + .unwrap(); + writeln!( + text, + "Ground the recap in the real diff only; do not invent changes. Redact any secrets. \ +If a skill produces nothing worth showing, write nothing — do not create placeholder files.\n" + ) + .unwrap(); +} + /// Render an anchor as a human-readable location string. /// /// When the artifact is a plan, `PlanStep` anchors are enriched with the @@ -1264,6 +1344,8 @@ mod tests { lenses: &[], output_path: Some(output), skills: &[], + pipeline_skills: &[], + recap_dir: None, }; let result = assemble_review_prompt(&input); @@ -1293,6 +1375,8 @@ mod tests { lenses: &["security", "frontend"], output_path: Some(output), skills: &[], + pipeline_skills: &[], + recap_dir: None, }; let result = assemble_review_prompt(&input); @@ -1317,6 +1401,8 @@ mod tests { lenses: &[], output_path: None, skills: &[], + pipeline_skills: &[], + recap_dir: None, }); // The single-pass step 2 must not DIRECT any subagent fan-out. (The // shared exploration budget mentions "per subagent when spawned" @@ -1337,6 +1423,8 @@ mod tests { lenses: &["security", "backend"], output_path: None, skills: &[], + pipeline_skills: &[], + recap_dir: None, }); assert!( panel.text.contains("Spawn AT MOST 2 parallel subagents"), @@ -1361,6 +1449,8 @@ mod tests { lenses: &[], output_path: None, skills: &[], + pipeline_skills: &[], + recap_dir: None, }; let a = assemble_review_prompt(&input); let b = assemble_review_prompt(&input); @@ -1384,6 +1474,8 @@ mod tests { lenses: &[], output_path: None, skills: &[], + pipeline_skills: &[], + recap_dir: None, }; let result = assemble_review_prompt(&input); assert!( @@ -1404,6 +1496,8 @@ mod tests { lenses: &[], output_path: None, skills: &[], + pipeline_skills: &[], + recap_dir: None, }); let no_body = assemble_review_prompt(&ReviewInput { title: "t", @@ -1414,6 +1508,8 @@ mod tests { lenses: &[], output_path: None, skills: &[], + pipeline_skills: &[], + recap_dir: None, }); assert!(with_body.text.contains("some body")); assert!(!no_body.text.contains("some body")); @@ -1444,6 +1540,8 @@ mod tests { lenses: &[], output_path: Some(output), skills: &skills, + pipeline_skills: &[], + recap_dir: None, }; let result = assemble_review_prompt(&input); @@ -1476,4 +1574,149 @@ mod tests { ); assert!(result.text.contains(verbatim), "preamble injected verbatim"); } + + // --------------------------------------------------------------- + // Visual-recap section tests + // --------------------------------------------------------------- + + /// The review-pipeline skills used by the recap tests. + fn recap_skills() -> Vec<(String, String)> { + vec![( + "visual-recap".to_owned(), + "Turn the diff into a visual briefing".to_owned(), + )] + } + + #[test] + fn review_prompt_recap_golden() { + // Golden for the ACTIVE recap contract: a single-pass review that also + // instructs the agent to invoke the review-pipeline skills and write + // their outputs into the recap dir. This golden was added DELIBERATELY + // alongside the two prior review goldens (which stay byte-identical, + // proving the recap section is inert when inactive). + let diff = review_diff(); + let output = std::path::Path::new("/tmp/cockpit-test/findings/owner-repo-42.json"); + let recap_dir = std::path::Path::new("/tmp/cockpit-test/recaps/owner-repo-42"); + let skills = recap_skills(); + 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, + lenses: &[], + output_path: Some(output), + skills: &[], + pipeline_skills: &skills, + recap_dir: Some(recap_dir), + }; + + let result = assemble_review_prompt(&input); + let expected = include_str!("../tests/golden/review_prompt_recap.txt"); + assert_eq!(result.text, expected, "review-prompt recap golden mismatch"); + } + + #[test] + fn review_prompt_recap_inactive_is_byte_identical() { + // The recap section must be inert unless BOTH the gate passed (recap_dir + // Some) AND there are pipeline skills. Each half alone leaves the prompt + // byte-identical to a prompt with no recap contract at all. + let diff = review_diff(); + let output = std::path::Path::new("/tmp/cockpit-test/findings/owner-repo-42.json"); + let recap_dir = std::path::Path::new("/tmp/cockpit-test/recaps/owner-repo-42"); + let skills = recap_skills(); + + let base = assemble_review_prompt(&ReviewInput { + title: "t", + body: "b", + issue: "NEX-1", + custom_preamble: None, + diff: &diff, + lenses: &[], + output_path: Some(output), + skills: &[], + pipeline_skills: &[], + recap_dir: None, + }); + + // Skills present but gate failed (no recap dir): omitted. + let gate_failed = assemble_review_prompt(&ReviewInput { + title: "t", + body: "b", + issue: "NEX-1", + custom_preamble: None, + diff: &diff, + lenses: &[], + output_path: Some(output), + skills: &[], + pipeline_skills: &skills, + recap_dir: None, + }); + assert_eq!(gate_failed.text, base.text); + assert_eq!(gate_failed.hash, base.hash); + + // Gate passed but no pipeline skills: omitted. + let no_skills = assemble_review_prompt(&ReviewInput { + title: "t", + body: "b", + issue: "NEX-1", + custom_preamble: None, + diff: &diff, + lenses: &[], + output_path: Some(output), + skills: &[], + pipeline_skills: &[], + recap_dir: Some(recap_dir), + }); + assert_eq!(no_skills.text, base.text); + assert_eq!(no_skills.hash, base.hash); + } + + #[test] + fn review_prompt_recap_section_sits_after_output_before_conventions() { + let diff = review_diff(); + let output = std::path::Path::new("/tmp/findings/x.json"); + let recap_dir = std::path::Path::new("/tmp/recaps/x"); + let pipeline = recap_skills(); + let conventions = 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::CockpitStore, + in_review_pipeline: false, + }]; + let input = ReviewInput { + title: "Add widgets", + body: "Body", + issue: "NEX-9", + custom_preamble: None, + diff: &diff, + lenses: &[], + output_path: Some(output), + skills: &conventions, + pipeline_skills: &pipeline, + recap_dir: Some(recap_dir), + }; + let result = assemble_review_prompt(&input); + + let output_pos = result.text.find("## Output").expect("output missing"); + let recap_pos = result + .text + .find("## Visual Recap") + .expect("recap section missing"); + let conventions_pos = result + .text + .find("## Project Conventions") + .expect("conventions missing"); + assert!( + output_pos < recap_pos && recap_pos < conventions_pos, + "recap must sit between Output and Project Conventions" + ); + // The skill is named for invocation and the recap dir is an absolute path. + assert!(result.text.contains("`visual-recap`")); + assert!(result.text.contains("/tmp/recaps/x")); + assert!(result.text.contains("recap.mdx")); + } } diff --git a/crates/cockpit-core/src/recap.rs b/crates/cockpit-core/src/recap.rs new file mode 100644 index 0000000..1b0bbdc --- /dev/null +++ b/crates/cockpit-core/src/recap.rs @@ -0,0 +1,206 @@ +//! On-disk reading of pre-review recap artifacts for the Briefing tab. +//! +//! The advisory pre-review agent writes a `recap.mdx` briefing (plus zero or +//! more `*.excalidraw` scene files) into a PR's recap directory (see +//! [`crate::config::recap_dir_for_pr`]) when the lens gate opts it into the +//! review-pipeline skills. This module reads those files back into a serializable +//! payload the frontend renders. It is pure disk I/O with no gate +//! interaction — reading a recap never mutates review state. +//! +//! See [`RecapPayload`] for the serialized shape sent to the frontend. + +use std::path::Path; + +use serde::{Deserialize, Serialize}; +use ts_rs::TS; + +use crate::config::RECAP_MDX_FILE; + +/// Maximum size, in bytes, of a single `.excalidraw` scene file that will be +/// read into a payload. Larger scenes are skipped (and logged) rather than +/// streamed into the IPC boundary, which would bloat the frontend. +const MAX_SCENE_BYTES: u64 = 2 * 1024 * 1024; + +/// Extension (without the dot) of an Excalidraw scene file. +const SCENE_EXT: &str = "excalidraw"; + +/// A single Excalidraw scene the pre-review agent produced. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct RecapScene { + /// Scene name: the file's basename with the `.excalidraw` extension removed + /// (e.g. `architecture` for `architecture.excalidraw`). Used as a stable + /// tab label / React key by the Briefing viewer. + pub name: String, + /// The raw scene JSON, ready to hand to the Excalidraw viewer. + pub json: String, +} + +/// The recap artifacts read from a PR's recap directory, for the Briefing tab. +/// +/// A read never fails loudly: a missing directory or file yields +/// `mdx: None, scenes: []` (see [`read_recap`]). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, TS)] +#[ts(export, export_to = "../../../app/src/bindings/")] +pub struct RecapPayload { + /// The `recap.mdx` briefing body (GitHub-flavored markdown), or `None` when + /// the file is absent or empty. + pub mdx: Option, + /// Excalidraw scenes discovered in the recap directory, sorted by name. + pub scenes: Vec, + /// The head SHA the recap was generated at (the review's `recap_sha`), for + /// staleness. Supplied by the caller from persisted review state; `None` + /// before any recap. The frontend compares this to the review's `head_sha`. + pub sha: Option, +} + +/// Read the recap artifacts from `dir`, attaching the caller-supplied `sha`. +/// +/// Reads `recap.mdx` (returned in `mdx` when present and non-empty) and every +/// `*.excalidraw` scene (sorted by name; each capped at [`MAX_SCENE_BYTES`], +/// oversized ones skipped and logged). A missing directory or unreadable entry +/// degrades to an empty result rather than an error — the Briefing simply shows +/// nothing. This is a read: it never mutates any gate or review state. +/// +/// `sha` is threaded through unchanged into [`RecapPayload::sha`] so the caller +/// (which holds the review's persisted `recap_sha`) can populate staleness +/// without this module reaching into review state. +pub fn read_recap(dir: &Path, sha: Option) -> RecapPayload { + RecapPayload { + mdx: read_mdx(dir), + scenes: read_scenes(dir), + sha, + } +} + +/// Read `recap.mdx` from `dir`, returning its contents when present and +/// non-empty (after trimming). Any read error yields `None`. +fn read_mdx(dir: &Path) -> Option { + let path = dir.join(RECAP_MDX_FILE); + match std::fs::read_to_string(&path) { + Ok(contents) if !contents.trim().is_empty() => Some(contents), + _ => None, + } +} + +/// Read every `*.excalidraw` scene in `dir`, sorted by name. +/// +/// Oversized scenes (> [`MAX_SCENE_BYTES`]) and unreadable entries are skipped; +/// a missing directory yields an empty vector. +fn read_scenes(dir: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(dir) else { + return Vec::new(); + }; + + let mut scenes = Vec::new(); + for entry in entries.flatten() { + let path = entry.path(); + if !path.is_file() { + continue; + } + // Only `*.excalidraw` files are scenes; skip recap.mdx and anything else. + let is_scene = path + .extension() + .is_some_and(|ext| ext.eq_ignore_ascii_case(SCENE_EXT)); + if !is_scene { + continue; + } + + // Skip oversized scenes rather than push megabytes across IPC. + match entry.metadata() { + Ok(meta) if meta.len() > MAX_SCENE_BYTES => { + eprintln!( + "read_recap: skipping oversized scene {} ({} bytes > {MAX_SCENE_BYTES})", + path.display(), + meta.len() + ); + continue; + } + Ok(_) => {} + Err(e) => { + eprintln!("read_recap: metadata for {} failed: {e}", path.display()); + continue; + } + } + + let Ok(json) = std::fs::read_to_string(&path) else { + continue; + }; + // The scene name is the basename without the `.excalidraw` extension. + let name = path + .file_stem() + .map(|s| s.to_string_lossy().into_owned()) + .unwrap_or_default(); + scenes.push(RecapScene { name, json }); + } + + scenes.sort_by(|a, b| a.name.cmp(&b.name)); + scenes +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn missing_dir_yields_empty_payload() { + let dir = tempfile::tempdir().expect("tempdir"); + // Point at a subdirectory that does not exist. + let missing = dir.path().join("recaps").join("owner-repo-1"); + let payload = read_recap(&missing, None); + assert_eq!(payload.mdx, None); + assert!(payload.scenes.is_empty()); + assert_eq!(payload.sha, None); + } + + #[test] + fn mdx_only_is_read_and_sha_threaded() { + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join(RECAP_MDX_FILE), "# Recap\n\nBody.").expect("write mdx"); + + let payload = read_recap(dir.path(), Some("headsha".into())); + assert_eq!(payload.mdx.as_deref(), Some("# Recap\n\nBody.")); + assert!(payload.scenes.is_empty()); + assert_eq!(payload.sha.as_deref(), Some("headsha")); + } + + #[test] + fn empty_mdx_is_treated_as_absent() { + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join(RECAP_MDX_FILE), " \n\t").expect("write mdx"); + let payload = read_recap(dir.path(), None); + assert_eq!(payload.mdx, None); + } + + #[test] + fn mdx_and_two_scenes_are_read_sorted() { + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join(RECAP_MDX_FILE), "# Recap").expect("write mdx"); + // Deliberately write in reverse order to prove sorting by name. + std::fs::write(dir.path().join("zeta.excalidraw"), "{\"z\":1}").expect("scene z"); + std::fs::write(dir.path().join("alpha.excalidraw"), "{\"a\":1}").expect("scene a"); + // A non-scene file must be ignored. + std::fs::write(dir.path().join("notes.txt"), "ignore me").expect("txt"); + + let payload = read_recap(dir.path(), None); + assert_eq!(payload.mdx.as_deref(), Some("# Recap")); + assert_eq!(payload.scenes.len(), 2); + assert_eq!(payload.scenes[0].name, "alpha"); + assert_eq!(payload.scenes[0].json, "{\"a\":1}"); + assert_eq!(payload.scenes[1].name, "zeta"); + assert_eq!(payload.scenes[1].json, "{\"z\":1}"); + } + + #[test] + fn oversized_scene_is_skipped() { + let dir = tempfile::tempdir().expect("tempdir"); + // One in-bounds scene, one over the 2 MiB cap. + std::fs::write(dir.path().join("small.excalidraw"), "{}").expect("small scene"); + let big = "x".repeat((MAX_SCENE_BYTES + 1) as usize); + std::fs::write(dir.path().join("huge.excalidraw"), big).expect("huge scene"); + + let payload = read_recap(dir.path(), None); + assert_eq!(payload.scenes.len(), 1, "oversized scene must be skipped"); + assert_eq!(payload.scenes[0].name, "small"); + } +} diff --git a/crates/cockpit-core/src/restack.rs b/crates/cockpit-core/src/restack.rs index 06d6252..7badc31 100644 --- a/crates/cockpit-core/src/restack.rs +++ b/crates/cockpit-core/src/restack.rs @@ -322,6 +322,7 @@ mod tests { conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } diff --git a/crates/cockpit-core/src/store.rs b/crates/cockpit-core/src/store.rs index dc42bdc..2195535 100644 --- a/crates/cockpit-core/src/store.rs +++ b/crates/cockpit-core/src/store.rs @@ -421,6 +421,7 @@ mod tests { conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_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 cca437c..73f10cd 100644 --- a/crates/cockpit-core/tests/batch_fan_out_e2e.rs +++ b/crates/cockpit-core/tests/batch_fan_out_e2e.rs @@ -146,6 +146,7 @@ fn make_review(id: &str, branch: &str, worktree: PathBuf, project: &ProjectId) - conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } diff --git a/crates/cockpit-core/tests/e2e_round_trip.rs b/crates/cockpit-core/tests/e2e_round_trip.rs index 808f30d..320fa0f 100644 --- a/crates/cockpit-core/tests/e2e_round_trip.rs +++ b/crates/cockpit-core/tests/e2e_round_trip.rs @@ -47,6 +47,7 @@ fn make_test_review(worktree: PathBuf) -> Review { conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } diff --git a/crates/cockpit-core/tests/fix_loop_e2e.rs b/crates/cockpit-core/tests/fix_loop_e2e.rs index 2f890cc..560e98d 100644 --- a/crates/cockpit-core/tests/fix_loop_e2e.rs +++ b/crates/cockpit-core/tests/fix_loop_e2e.rs @@ -176,6 +176,7 @@ fn make_review(worktree: PathBuf, branch: &str, head_sha: &str) -> Review { conversation: vec![], last_reviewed_sha: None, distilled_intent: None, + recap_sha: None, } } diff --git a/crates/cockpit-core/tests/golden/review_prompt_recap.txt b/crates/cockpit-core/tests/golden/review_prompt_recap.txt new file mode 100644 index 0000000..08e936e --- /dev/null +++ b/crates/cockpit-core/tests/golden/review_prompt_recap.txt @@ -0,0 +1,46 @@ +## 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 + +Act as a read-only advisory reviewer for the pull request above. Work in three steps. + +1. Distill the INTENT. In 2-4 sentences, state what this PR actually does — judged from the title, body, and diff — and call out any gap between what it claims and what it changes. + +2. Review the DIFF against that intent in a single general pass, covering correctness, tests, and anything the change puts at risk. + +Review the DIFF, not the repository. Read only files in the diff plus directly referenced definitions. Budget: about 25 tool calls total (about 15 per subagent when spawned). Do not run builds or test suites. When verification would need a deep chain of reads, report the finding as a Warning with your assumption stated in the rationale instead of chasing it. Aim to complete within 5 minutes. + +3. Output ONE JSON OBJECT to the output path, and nothing else, in this shape: + {"intent":"...","findings":[{"severity":"Info|Warning|Critical","path":"...","line_start":N,"line_end":N,"side":"Old|New","title":"...","rationale":"...","area":"frontend|backend|infra|security|general"}]} + Set each finding's "area" to the lens that produced it. This pass is advisory only: do NOT edit any code. + +## Output + +Write ONLY the JSON review object to `/tmp/cockpit-test/findings/owner-repo-42.json`. Output nothing else to that file. + +## Visual Recap + +After writing your findings, produce a visual briefing for the human reviewer. Invoke each of these review-pipeline skills by name — they are Claude Code skills already available in your environment — against this PR's diff: + +- `visual-recap`: Turn the diff into a visual briefing + +Write all recap outputs into `/tmp/cockpit-test/recaps/owner-repo-42`: +- `recap.mdx` — the briefing as GitHub-flavored markdown (headings, tables, fenced diff blocks). +- any number of `*.excalidraw` scene files for diagrams a skill produces. + +Ground the recap in the real diff only; do not invent changes. Redact any secrets. If a skill produces nothing worth showing, write nothing — do not create placeholder files. + From e7d7dd03f349e817ad70457b078d23dca1e36ec1 Mon Sep 17 00:00:00 2001 From: Alejandro Perez Date: Fri, 3 Jul 2026 02:50:26 +0100 Subject: [PATCH 2/2] =?UTF-8?q?core:=20recap=20review=20MINORs=20=E2=80=94?= =?UTF-8?q?=20symlink-safe=20scene=20cap,=20local=20prompt=20sanitization?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/cockpit-core/src/prompt.rs | 16 +++++++++++++++- crates/cockpit-core/src/recap.rs | 14 +++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/crates/cockpit-core/src/prompt.rs b/crates/cockpit-core/src/prompt.rs index c71b616..c1e80d7 100644 --- a/crates/cockpit-core/src/prompt.rs +++ b/crates/cockpit-core/src/prompt.rs @@ -516,6 +516,14 @@ pub fn assemble_review_prompt(input: &ReviewInput<'_>) -> AssembledPrompt { AssembledPrompt { text, hash } } +/// Collapse all whitespace runs (including newlines) to single spaces. +/// +/// Used on text injected into single-line prompt positions so multi-line +/// input can never fabricate additional markdown structure. +fn flatten_ws(s: &str) -> String { + s.split_whitespace().collect::>().join(" ") +} + /// Write the visual-recap instruction section, if the recap contract is active. /// /// Emits nothing — keeping the prompt byte-identical to the pre-recap output — @@ -546,7 +554,13 @@ available in your environment — against this PR's diff:\n" ) .unwrap(); for (name, description) in skills { - if description.trim().is_empty() { + // Defense-in-depth: skill names/descriptions come from SKILL.md + // frontmatter. The parser is line-based so newlines shouldn't reach + // here, but flatten locally so a list entry can never break out of + // this section regardless of upstream parsing. + let name = flatten_ws(name); + let description = flatten_ws(description); + if description.is_empty() { writeln!(text, "- `{name}`").unwrap(); } else { writeln!(text, "- `{name}`: {description}").unwrap(); diff --git a/crates/cockpit-core/src/recap.rs b/crates/cockpit-core/src/recap.rs index 1b0bbdc..35a38a5 100644 --- a/crates/cockpit-core/src/recap.rs +++ b/crates/cockpit-core/src/recap.rs @@ -107,7 +107,9 @@ fn read_scenes(dir: &Path) -> Vec { } // Skip oversized scenes rather than push megabytes across IPC. - match entry.metadata() { + // `fs::metadata` (not `DirEntry::metadata`) so a symlinked scene is + // measured at its target — the same file `read_to_string` follows. + match std::fs::metadata(&path) { Ok(meta) if meta.len() > MAX_SCENE_BYTES => { eprintln!( "read_recap: skipping oversized scene {} ({} bytes > {MAX_SCENE_BYTES})", @@ -126,6 +128,16 @@ fn read_scenes(dir: &Path) -> Vec { let Ok(json) = std::fs::read_to_string(&path) else { continue; }; + // Defense-in-depth: the metadata gate above raced the read; re-check + // the bytes actually loaded. + if json.len() as u64 > MAX_SCENE_BYTES { + eprintln!( + "read_recap: skipping oversized scene {} ({} bytes > {MAX_SCENE_BYTES})", + path.display(), + json.len() + ); + continue; + } // The scene name is the basename without the `.excalidraw` extension. let name = path .file_stem()