From a73237ecf4513586161ffc20edb2856a9194ab6e Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Fri, 5 Jun 2026 08:36:24 -0400 Subject: [PATCH] Add lazy auto-review detail retrieval --- code-rs/core/src/codex/streaming.rs | 128 +++++++- code-rs/core/src/openai_tools.rs | 82 +++++ code-rs/core/src/review_store.rs | 457 +++++++++++++++++++++++++++- 3 files changed, 664 insertions(+), 3 deletions(-) diff --git a/code-rs/core/src/codex/streaming.rs b/code-rs/core/src/codex/streaming.rs index 4166c97a2cd..56273257181 100644 --- a/code-rs/core/src/codex/streaming.rs +++ b/code-rs/core/src/codex/streaming.rs @@ -31,7 +31,10 @@ use crate::protocol::McpListToolsResponseEvent; use crate::protocol::TaskLifecycleEvent; use crate::protocol::TaskLifecyclePhase; use crate::protocol::TaskOriginKind; -use crate::review_store::{AutoReviewLedgerOptions, AutoReviewRunStore}; +use crate::review_store::{ + default_auto_review_detail_max_bytes, hard_auto_review_detail_max_bytes, + AutoReviewLedgerOptions, AutoReviewRunStore, +}; use code_app_server_protocol::AuthMode as AppAuthMode; use code_protocol::models::ContentItem; use code_protocol::models::ResponseItem; @@ -5073,6 +5076,7 @@ async fn handle_function_call( "gh_run_wait" => handle_gh_run_wait(sess, &ctx, arguments).await, "kill" => handle_kill(sess, &ctx, arguments).await, "code_bridge" | "code_bridge_subscription" => handle_code_bridge(sess, &ctx, arguments).await, + "auto_review_detail" => handle_auto_review_detail(sess, &ctx, arguments).await, TOOL_SEARCH_TOOL_NAME | LEGACY_SEARCH_TOOL_BM25_TOOL_NAME => { let arguments = match serde_json::from_str::(&arguments) { Ok(arguments) => arguments, @@ -5360,6 +5364,128 @@ async fn handle_declare_worktree_decision( } } +#[derive(Debug, serde::Deserialize)] +struct AutoReviewDetailArgs { + run_id: String, + #[serde(default)] + finding_id: Option, + #[serde(default)] + max_bytes: Option, +} + +async fn handle_auto_review_detail( + sess: &Session, + ctx: &ToolCallCtx, + arguments: String, +) -> ResponseInputItem { + let args = match serde_json::from_str::(&arguments) { + Ok(args) => args, + Err(err) => { + return ResponseInputItem::FunctionCallOutput { + call_id: ctx.call_id.clone(), + output: FunctionCallOutputPayload { + body: code_protocol::models::FunctionCallOutputBody::Text( + serde_json::json!({ + "success": false, + "error": { + "code": "invalid_arguments", + "message": format!("invalid auto_review_detail arguments: {err}"), + } + }) + .to_string(), + ), + success: Some(false), + }, + }; + } + }; + let run_id = match Uuid::parse_str(&args.run_id) { + Ok(run_id) => run_id, + Err(err) => { + return ResponseInputItem::FunctionCallOutput { + call_id: ctx.call_id.clone(), + output: FunctionCallOutputPayload { + body: code_protocol::models::FunctionCallOutputBody::Text( + serde_json::json!({ + "success": false, + "error": { + "code": "invalid_run_id", + "run_id": args.run_id, + "message": format!("invalid auto review run_id: {err}"), + } + }) + .to_string(), + ), + success: Some(false), + }, + }; + } + }; + let cwd = sess.cwd.clone(); + let finding_id = args.finding_id.clone(); + let max_bytes = args.max_bytes; + let lookup = tokio::task::spawn_blocking(move || { + let store = AutoReviewRunStore::open_existing_read_only(&cwd).map_err(|err| { + serde_json::json!({ + "code": "store_unavailable", + "message": format!("failed to open auto review run store: {err}"), + }) + })?; + let Some(store) = store else { + return Err(serde_json::json!({ + "code": "store_missing", + "message": "auto review run store is not available for this workspace", + })); + }; + store + .read_detail(run_id, finding_id.as_deref(), max_bytes) + .map_err(|err| serde_json::to_value(err).unwrap_or_else(|ser_err| { + serde_json::json!({ + "code": "detail_error", + "message": format!("failed to serialize auto review detail error: {ser_err}"), + }) + })) + }) + .await; + + let body = match lookup { + Ok(Ok(detail)) => serde_json::json!({ + "success": true, + "detail": detail, + "defaults": { + "default_max_bytes": default_auto_review_detail_max_bytes(), + "hard_max_bytes": hard_auto_review_detail_max_bytes(), + } + }), + Ok(Err(error)) => serde_json::json!({ + "success": false, + "error": error, + "defaults": { + "default_max_bytes": default_auto_review_detail_max_bytes(), + "hard_max_bytes": hard_auto_review_detail_max_bytes(), + } + }), + Err(err) => serde_json::json!({ + "success": false, + "error": { + "code": "lookup_join_error", + "message": format!("auto review detail lookup task failed: {err}"), + } + }), + }; + let success = body + .get("success") + .and_then(serde_json::Value::as_bool) + .unwrap_or(false); + ResponseInputItem::FunctionCallOutput { + call_id: ctx.call_id.clone(), + output: FunctionCallOutputPayload { + body: code_protocol::models::FunctionCallOutputBody::Text(body.to_string()), + success: Some(success), + }, + } +} + async fn handle_dynamic_tool_call( sess: &Session, ctx: &ToolCallCtx, diff --git a/code-rs/core/src/openai_tools.rs b/code-rs/core/src/openai_tools.rs index daad5ff98f0..8e619b57e1b 100644 --- a/code-rs/core/src/openai_tools.rs +++ b/code-rs/core/src/openai_tools.rs @@ -1308,6 +1308,7 @@ pub fn get_openai_tools( tools.push(create_kill_tool()); tools.push(create_gh_run_wait_tool()); tools.push(create_bridge_tool()); + tools.push(create_auto_review_detail_tool()); if config.web_search_request { let search_content_types = match config.web_search_tool_type { @@ -1501,6 +1502,49 @@ pub fn create_gh_run_wait_tool() -> OpenAiTool { }) } +pub fn create_auto_review_detail_tool() -> OpenAiTool { + let mut properties = BTreeMap::new(); + properties.insert( + "run_id".to_string(), + JsonSchema::String { + description: Some( + "Stable Auto Review run id from the auto_review_ledger.".to_string(), + ), + allowed_values: None, + }, + ); + properties.insert( + "finding_id".to_string(), + JsonSchema::String { + description: Some( + "Optional stable finding id such as f1 or f2. If omitted, returns bounded whole-run detail." + .to_string(), + ), + allowed_values: None, + }, + ); + properties.insert( + "max_bytes".to_string(), + JsonSchema::Number { + description: Some( + "Optional maximum response size in bytes; clamped to the built-in safe limit." + .to_string(), + ), + }, + ); + OpenAiTool::Function(ResponsesApiTool { + name: "auto_review_detail".to_string(), + description: "Retrieve bounded Auto Review run or finding details from durable sidecars using run ids shown in the Auto Review ledger. Read-only; does not change review disposition." + .to_string(), + strict: false, + parameters: JsonSchema::Object { + properties, + required: Some(vec!["run_id".to_string()]), + additional_properties: Some(false.into()), + }, + }) +} + pub fn create_bridge_tool() -> OpenAiTool { let mut properties = BTreeMap::new(); @@ -1644,11 +1688,40 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", ], ); } + #[test] + fn auto_review_detail_tool_requires_only_run_id() { + let tool = create_auto_review_detail_tool(); + let OpenAiTool::Function(ResponsesApiTool { + name, + parameters, + .. + }) = tool + else { + panic!("expected function tool"); + }; + + assert_eq!(name, "auto_review_detail"); + let JsonSchema::Object { + properties, + required, + additional_properties, + } = parameters + else { + panic!("expected object parameters"); + }; + assert_eq!(required, Some(vec!["run_id".to_string()])); + assert_eq!(additional_properties, Some(false.into())); + assert!(properties.contains_key("run_id")); + assert!(properties.contains_key("finding_id")); + assert!(properties.contains_key("max_bytes")); + } + #[test] fn test_web_search_defaults_to_external_access_enabled() { let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); @@ -1811,6 +1884,7 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", ], ); @@ -1845,6 +1919,7 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", ], ); @@ -1878,6 +1953,7 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", ], ); @@ -1950,6 +2026,7 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", "test_server/do_something_cool", ], @@ -2076,6 +2153,7 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", "test_server/do_something_cool", ], @@ -2204,6 +2282,7 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", "dash/search", ], @@ -2281,6 +2360,7 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", "dash/paginate", ], @@ -2359,6 +2439,7 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", "dash/tags", ], @@ -2435,6 +2516,7 @@ mod tests { "kill", "gh_run_wait", "code_bridge", + "auto_review_detail", "web_search", "dash/value", ], diff --git a/code-rs/core/src/review_store.rs b/code-rs/core/src/review_store.rs index 50c7671722d..e52457b6754 100644 --- a/code-rs/core/src/review_store.rs +++ b/code-rs/core/src/review_store.rs @@ -1,4 +1,4 @@ -use crate::protocol::ReviewOutputEvent; +use crate::protocol::{ReviewFinding, ReviewOutputEvent}; use crate::review_coord::scoped_review_state_dir; use crate::review_coord::scoped_review_state_dir_path; use serde::{Deserialize, Serialize}; @@ -17,6 +17,9 @@ const SCHEMA_VERSION: u32 = 1; const DEFAULT_MAX_RUNS: usize = 500; const MAX_FINDING_DIGESTS: usize = 25; const MAX_FINDING_DIGEST_TITLE_CHARS: usize = 160; +const AUTO_REVIEW_DETAIL_DEFAULT_MAX_BYTES: usize = 12_000; +const AUTO_REVIEW_DETAIL_HARD_MAX_BYTES: usize = 64_000; +const AUTO_REVIEW_DETAIL_MAX_FINDINGS: usize = 10; const DEFAULT_LEDGER_MAX_BYTES: usize = 2_400; const DEFAULT_LEDGER_MAX_RUNS: usize = 5; const LEDGER_RECENT_ACTIONABLE_SECS: u64 = 24 * 60 * 60; @@ -276,6 +279,63 @@ pub struct AutoReviewDuplicateMatch { pub prompt_token_estimate: Option, } +#[derive(Debug, Clone, Serialize, PartialEq)] +pub struct AutoReviewDetailResponse { + pub run_id: Uuid, + #[serde(skip_serializing_if = "Option::is_none")] + pub finding_id: Option, + pub status: AutoReviewRunStatus, + pub source: AutoReviewRunSource, + #[serde(skip_serializing_if = "Option::is_none")] + pub branch: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub snapshot_commit: Option, + pub detail_kind: AutoReviewDetailKind, + pub finding_count: usize, + pub omitted_findings: usize, + pub content: String, + pub bytes: usize, + pub original_bytes: usize, + pub max_bytes: usize, + pub truncated: bool, + #[serde(skip_serializing_if = "Option::is_none")] + pub sidecar_path: Option, +} + +#[derive(Debug, Clone, Copy, Serialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum AutoReviewDetailKind { + Run, + Finding, +} + +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum AutoReviewDetailErrorCode { + InvalidFindingId, + MissingFinding, + MissingRun, + MissingSidecar, + InvalidSidecar, +} + +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +pub struct AutoReviewDetailError { + pub code: AutoReviewDetailErrorCode, + pub run_id: Uuid, + #[serde(skip_serializing_if = "Option::is_none")] + pub finding_id: Option, + pub message: String, +} + +impl std::fmt::Display for AutoReviewDetailError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.message) + } +} + +impl std::error::Error for AutoReviewDetailError {} + #[derive(Debug, Clone)] pub struct AutoReviewLedgerOptions { pub max_bytes: usize, @@ -333,6 +393,21 @@ impl AutoReviewRunStore { Self::open_in_dir(root).map(Some) } + pub fn open_existing_read_only(scope: &Path) -> io::Result> { + let root = auto_review_dir_path(scope)?; + if !root.exists() { + return Ok(None); + } + let runs_path = root.join(RUNS_FILENAME); + let runs = read_runs_file(&runs_path)?; + Ok(Some(Self { + outputs_dir: root.join(OUTPUTS_DIR), + root, + runs_path, + runs, + })) + } + pub fn open_in_dir(root: PathBuf) -> io::Result { fs::create_dir_all(&root)?; let outputs_dir = root.join(OUTPUTS_DIR); @@ -583,11 +658,219 @@ impl AutoReviewRunStore { serde_json::from_str(&text).map_err(io::Error::other) } + pub fn read_detail( + &self, + run_id: Uuid, + finding_id: Option<&str>, + max_bytes: Option, + ) -> Result { + let run = self.runs.get(&run_id).ok_or_else(|| AutoReviewDetailError { + code: AutoReviewDetailErrorCode::MissingRun, + run_id, + finding_id: finding_id.map(str::to_string), + message: format!("auto review run {run_id} was not found in the durable run ledger"), + })?; + let sidecar_path = run + .output_path + .clone() + .unwrap_or_else(|| self.output_path_for(run_id)); + let text = fs::read_to_string(&sidecar_path).map_err(|err| AutoReviewDetailError { + code: if err.kind() == io::ErrorKind::NotFound { + AutoReviewDetailErrorCode::MissingSidecar + } else { + AutoReviewDetailErrorCode::InvalidSidecar + }, + run_id, + finding_id: finding_id.map(str::to_string), + message: if err.kind() == io::ErrorKind::NotFound { + format!( + "auto review detail sidecar for run {run_id} is not available at {}", + sidecar_path.display() + ) + } else { + format!( + "failed to read auto review detail sidecar for run {run_id} at {}: {err}", + sidecar_path.display() + ) + }, + })?; + let output = serde_json::from_str::(&text).map_err(|err| { + AutoReviewDetailError { + code: AutoReviewDetailErrorCode::InvalidSidecar, + run_id, + finding_id: finding_id.map(str::to_string), + message: format!( + "failed to parse auto review detail sidecar for run {run_id} at {}: {err}", + sidecar_path.display() + ), + } + })?; + let max_bytes = normalize_detail_max_bytes(max_bytes); + let detail_kind = if finding_id.is_some() { + AutoReviewDetailKind::Finding + } else { + AutoReviewDetailKind::Run + }; + let content = match finding_id { + Some(finding_id) => { + let index = parse_finding_id(finding_id).ok_or_else(|| AutoReviewDetailError { + code: AutoReviewDetailErrorCode::InvalidFindingId, + run_id, + finding_id: Some(finding_id.to_string()), + message: format!( + "invalid auto review finding id {finding_id:?}; expected a stable id like f1" + ), + })?; + let finding = output.findings.get(index).ok_or_else(|| AutoReviewDetailError { + code: AutoReviewDetailErrorCode::MissingFinding, + run_id, + finding_id: Some(finding_id.to_string()), + message: format!( + "auto review finding {finding_id} was not found for run {run_id}; this run has {} finding(s)", + output.findings.len() + ), + })?; + format_finding_detail(finding_id, finding) + } + None => format_run_detail(&output), + }; + let original_bytes = content.len(); + let (content, truncated) = truncate_detail_content(content, max_bytes); + let bytes = content.len(); + let omitted_findings = match finding_id { + Some(_) => output.findings.len().saturating_sub(1), + None => output.findings.len().saturating_sub(AUTO_REVIEW_DETAIL_MAX_FINDINGS), + }; + let findings_capped = finding_id.is_none() && omitted_findings > 0; + Ok(AutoReviewDetailResponse { + run_id, + finding_id: finding_id.map(str::to_string), + status: run.status, + source: run.source, + branch: run.branch.clone(), + snapshot_commit: run.snapshot_commit.clone(), + detail_kind, + finding_count: output.findings.len(), + omitted_findings, + content, + bytes, + original_bytes, + max_bytes, + truncated: truncated || findings_capped, + sidecar_path: Some(sidecar_path), + }) + } + pub fn compact_ledger(&self, options: AutoReviewLedgerOptions) -> Option { compact_ledger_from_runs(self.runs.values(), options) } } +pub fn default_auto_review_detail_max_bytes() -> usize { + AUTO_REVIEW_DETAIL_DEFAULT_MAX_BYTES +} + +pub fn hard_auto_review_detail_max_bytes() -> usize { + AUTO_REVIEW_DETAIL_HARD_MAX_BYTES +} + +fn normalize_detail_max_bytes(max_bytes: Option) -> usize { + max_bytes + .unwrap_or(AUTO_REVIEW_DETAIL_DEFAULT_MAX_BYTES) + .clamp(1, AUTO_REVIEW_DETAIL_HARD_MAX_BYTES) +} + +fn parse_finding_id(finding_id: &str) -> Option { + let number = finding_id.strip_prefix('f')?.parse::().ok()?; + number.checked_sub(1) +} + +fn format_run_detail(output: &ReviewOutputEvent) -> String { + let mut sections = Vec::new(); + sections.push(format!( + "overall_correctness={} confidence={}", + output.overall_correctness, output.overall_confidence_score + )); + if !output.overall_explanation.trim().is_empty() { + sections.push(format!( + "overall_explanation:\n{}", + output.overall_explanation.trim() + )); + } + if output.findings.is_empty() { + sections.push("findings: none".to_string()); + } else { + let mut findings = String::from("findings:"); + for (idx, finding) in output + .findings + .iter() + .take(AUTO_REVIEW_DETAIL_MAX_FINDINGS) + .enumerate() + { + let finding_id = format!("f{}", idx + 1); + findings.push('\n'); + findings.push_str(&format_finding_detail(&finding_id, finding)); + } + if output.findings.len() > AUTO_REVIEW_DETAIL_MAX_FINDINGS { + findings.push_str(&format!( + "\n... omitted {} additional finding(s); request a specific finding_id for full detail", + output.findings.len() - AUTO_REVIEW_DETAIL_MAX_FINDINGS + )); + } + sections.push(findings); + } + sections.join("\n\n") +} + +fn format_finding_detail(finding_id: &str, finding: &ReviewFinding) -> String { + format!( + "finding_id={finding_id} priority={} confidence={} location={}:{}-{}\ntitle: {}\nbody:\n{}", + finding.priority, + finding.confidence_score, + finding.code_location.absolute_file_path.display(), + finding.code_location.line_range.start, + finding.code_location.line_range.end, + finding.title.trim(), + finding.body.trim() + ) +} + +fn truncate_detail_content(content: String, max_bytes: usize) -> (String, bool) { + if content.len() <= max_bytes { + return (content, false); + } + let marker = "\n... truncated auto review detail ...\n"; + if max_bytes <= marker.len() { + return (marker.chars().take(max_bytes).collect(), true); + } + let keep = max_bytes - marker.len(); + let head_budget = keep / 2; + let tail_budget = keep - head_budget; + let head_end = floor_char_boundary(&content, head_budget); + let tail_start = ceil_char_boundary(&content, content.len().saturating_sub(tail_budget)); + let mut truncated = String::new(); + truncated.push_str(&content[..head_end]); + truncated.push_str(marker); + truncated.push_str(&content[tail_start..]); + (truncated, true) +} + +fn floor_char_boundary(text: &str, mut index: usize) -> usize { + index = index.min(text.len()); + while index > 0 && !text.is_char_boundary(index) { + index -= 1; + } + index +} + +fn ceil_char_boundary(text: &str, mut index: usize) -> usize { + index = index.min(text.len()); + while index < text.len() && !text.is_char_boundary(index) { + index += 1; + } + index +} + fn duplicate_priority(run: &AutoReviewRun) -> u8 { if run.status.is_adoptable_duplicate() { return 4; @@ -742,7 +1025,7 @@ where lines.push(format!( "" )); - lines.push("Auto Review state for this repo. Listed runs are active or target-applicable by default; diagnostics may include stale/detached history that is not an instruction to re-review or fix. Treat run ids as stable references for future detail lookup. Freshness describes run recency; target describes whether findings match the active checkout.".to_string()); + lines.push("Auto Review state for this repo. Listed runs are active or target-applicable by default; diagnostics may include stale/detached history that is not an instruction to re-review or fix. Treat run ids as stable references for future detail lookup with the auto_review_detail tool. Freshness describes run recency; target describes whether findings match the active checkout.".to_string()); if let Some(diagnostics) = diagnostics { lines.push(diagnostics); } @@ -1262,6 +1545,22 @@ mod tests { } } + fn review_finding(idx: usize, body: &str) -> ReviewFinding { + ReviewFinding { + title: format!("finding title {idx}"), + body: body.to_string(), + confidence_score: 0.9, + priority: idx as i32, + code_location: ReviewCodeLocation { + absolute_file_path: PathBuf::from(format!("/repo/src/file{idx}.rs")), + line_range: ReviewLineRange { + start: idx as u32, + end: idx as u32 + 1, + }, + }, + } + } + fn finding_digest(idx: usize) -> AutoReviewFindingDigest { AutoReviewFindingDigest { finding_id: format!("f{idx}"), @@ -1329,6 +1628,142 @@ mod tests { assert_eq!(run.summary_digest.as_deref(), Some("summary")); } + #[test] + #[serial] + fn detail_lookup_returns_bounded_run_detail() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let run_id = Uuid::new_v4(); + let mut run = AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1); + run.branch = Some("feature".to_string()); + run.snapshot_commit = Some("abcdef1234567890".to_string()); + run.mark_status(AutoReviewRunStatus::Completed, 2); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + store.upsert(run).unwrap(); + store.write_output(run_id, &review_output()).unwrap(); + + let detail = store.read_detail(run_id, None, Some(2_000)).unwrap(); + + assert_eq!(detail.run_id, run_id); + assert_eq!(detail.detail_kind, AutoReviewDetailKind::Run); + assert_eq!(detail.status, AutoReviewRunStatus::Completed); + assert_eq!(detail.branch.as_deref(), Some("feature")); + assert!(detail.content.contains("overall_correctness=incorrect")); + assert!(detail.content.contains("finding_id=f1")); + assert!(!detail.truncated); + assert!(detail.bytes <= detail.max_bytes); + } + + #[test] + #[serial] + fn detail_lookup_returns_specific_finding_by_stable_id() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let run_id = Uuid::new_v4(); + let mut output = review_output(); + output.findings = vec![review_finding(1, "first body"), review_finding(2, "second body")]; + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + store + .upsert(AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1)) + .unwrap(); + store.write_output(run_id, &output).unwrap(); + + let detail = store.read_detail(run_id, Some("f2"), Some(2_000)).unwrap(); + + assert_eq!(detail.finding_id.as_deref(), Some("f2")); + assert_eq!(detail.detail_kind, AutoReviewDetailKind::Finding); + assert_eq!(detail.finding_count, 2); + assert_eq!(detail.omitted_findings, 1); + assert!(detail.content.contains("title: finding title 2")); + assert!(detail.content.contains("second body")); + assert!(!detail.content.contains("first body")); + } + + #[test] + #[serial] + fn detail_lookup_truncates_long_content_with_metadata() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let run_id = Uuid::new_v4(); + let mut output = review_output(); + output.findings = vec![review_finding(1, &"x".repeat(4_000))]; + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + store + .upsert(AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1)) + .unwrap(); + store.write_output(run_id, &output).unwrap(); + + let detail = store.read_detail(run_id, Some("f1"), Some(300)).unwrap(); + + assert!(detail.truncated); + assert!(detail.content.contains("truncated auto review detail")); + assert!(detail.original_bytes > detail.bytes); + assert!(detail.bytes <= 300); + } + + #[test] + #[serial] + fn detail_lookup_reports_missing_sidecar() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let run_id = Uuid::new_v4(); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + store + .upsert(AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1)) + .unwrap(); + + let err = store.read_detail(run_id, None, None).unwrap_err(); + + assert_eq!(err.code, AutoReviewDetailErrorCode::MissingSidecar); + assert!(err.message.contains("sidecar")); + } + + #[test] + #[serial] + fn detail_lookup_reports_missing_and_invalid_finding_ids() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let run_id = Uuid::new_v4(); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + store + .upsert(AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1)) + .unwrap(); + store.write_output(run_id, &review_output()).unwrap(); + + let invalid = store.read_detail(run_id, Some("finding-1"), None).unwrap_err(); + assert_eq!(invalid.code, AutoReviewDetailErrorCode::InvalidFindingId); + + let missing = store.read_detail(run_id, Some("f2"), None).unwrap_err(); + assert_eq!(missing.code, AutoReviewDetailErrorCode::MissingFinding); + } + + #[test] + #[serial] + fn compact_ledger_omits_verbose_sidecar_detail() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let run_id = Uuid::new_v4(); + let mut run = AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 10); + run.mark_status(AutoReviewRunStatus::Completed, 20); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + store.upsert(run).unwrap(); + let mut output = review_output(); + output.findings = vec![review_finding(1, "VERY_VERBOSE_FINDING_BODY_SHOULD_STAY_LAZY")]; + store.write_output(run_id, &output).unwrap(); + + let ledger = store.compact_ledger(ledger_options(30)).expect("ledger"); + + assert!(ledger.contains(&run_id.to_string())); + assert!(!ledger.contains("VERY_VERBOSE_FINDING_BODY_SHOULD_STAY_LAZY")); + assert!(!ledger.contains("overall_correctness")); + } + #[test] #[serial] fn run_store_rejects_output_for_unknown_run() { @@ -2038,6 +2473,24 @@ mod tests { assert!(!code_home.path().join("state").join("review").exists()); } + #[test] + #[serial] + fn open_existing_read_only_does_not_create_outputs_dir() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let root = auto_review_dir_path(repo.path()).unwrap(); + fs::create_dir_all(&root).unwrap(); + write_runs_file(&root.join(RUNS_FILENAME), std::iter::empty()).unwrap(); + + let store = AutoReviewRunStore::open_existing_read_only(repo.path()) + .unwrap() + .expect("store"); + + assert_eq!(store.root(), root.as_path()); + assert!(!root.join(OUTPUTS_DIR).exists()); + } + #[test] #[serial] fn compact_ledger_honors_byte_cap() {