Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
212 changes: 212 additions & 0 deletions code-rs/core/src/review_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,21 @@ impl AutoReviewRunStore {
&self,
diff_fingerprint: &str,
excluded_run_id: Option<Uuid>,
) -> Option<AutoReviewDuplicateMatch> {
self.find_duplicate_by_fingerprint_excluding_with_target(
diff_fingerprint,
excluded_run_id,
None,
None,
)
}

pub fn find_duplicate_by_fingerprint_excluding_with_target(
&self,
diff_fingerprint: &str,
excluded_run_id: Option<Uuid>,
active_branch: Option<&str>,
active_head: Option<&str>,
) -> Option<AutoReviewDuplicateMatch> {
let fingerprint = diff_fingerprint.trim();
if fingerprint.is_empty() {
Expand All @@ -543,6 +558,7 @@ impl AutoReviewRunStore {
| AutoReviewRunStatus::Superseded
)
})
.filter(|run| duplicate_target_is_reusable(run, active_branch, active_head))
.max_by(|left, right| {
duplicate_priority(left)
.cmp(&duplicate_priority(right))
Expand Down Expand Up @@ -894,6 +910,40 @@ fn duplicate_disposition(run: &AutoReviewRun) -> AutoReviewDuplicateDisposition
}
}

fn duplicate_target_is_reusable(
run: &AutoReviewRun,
active_branch: Option<&str>,
active_head: Option<&str>,
) -> bool {
if active_head.and_then(non_empty_str).is_none() {
return true;
}

!target_is_known_stale(auto_review_target_applicability(
run,
active_branch,
active_head,
))
}

pub fn auto_review_freshness_for_agent_status(
is_terminal: bool,
elapsed_secs: Option<u64>,
seconds_since_last_activity: Option<u64>,
stale_secs: u64,
) -> AutoReviewFreshness {
if is_terminal {
return AutoReviewFreshness::Current;
}
if seconds_since_last_activity.is_some_and(|seconds| seconds >= stale_secs) {
return AutoReviewFreshness::Inactive;
}
if elapsed_secs.is_some_and(|elapsed| elapsed >= LEDGER_LONG_ELAPSED_SECS) {
return AutoReviewFreshness::LongRunning;
}
AutoReviewFreshness::Current
}

pub fn auto_review_dir(scope: &Path) -> io::Result<PathBuf> {
Ok(scoped_review_state_dir(scope)?.join(AUTO_REVIEW_DIR))
}
Expand Down Expand Up @@ -1522,6 +1572,8 @@ mod tests {
use serial_test::serial;
use tempfile::TempDir;

const AUTO_REVIEW_TEST_STALE_SECS: u64 = 5 * 60;

fn set_code_home(path: &Path) {
// SAFETY: tests run serially and isolate CODE_HOME within a temp dir per test.
unsafe { std::env::set_var("CODE_HOME", path); }
Expand Down Expand Up @@ -1879,6 +1931,127 @@ mod tests {
);
}

#[test]
#[serial]
fn duplicate_lookup_ignores_stale_terminal_target_match() {
let code_home = TempDir::new().unwrap();
let repo = TempDir::new().unwrap();
set_code_home(code_home.path());
let mut store = AutoReviewRunStore::open(repo.path()).unwrap();

let stale_id = Uuid::new_v4();
let mut stale = AutoReviewRun::new(stale_id, AutoReviewRunSource::Tui, 1);
stale.diff_fingerprint = Some("diff:abc".to_string());
stale.branch = Some("auto-review".to_string());
stale.snapshot_commit = Some("aaaaaaaaaaaaaaaa".to_string());
stale.finding_count = 1;
stale.mark_status(AutoReviewRunStatus::Completed, 2);
store.upsert(stale).unwrap();

assert!(
store
.find_duplicate_by_fingerprint_excluding_with_target(
"diff:abc",
None,
Some("feature"),
Some("bbbbbbbbbbbbbbbb"),
)
.is_none()
);
assert_eq!(
store
.find_duplicate_by_fingerprint("diff:abc")
.expect("fallback duplicate")
.run_id,
stale_id
);
}

#[test]
#[serial]
fn duplicate_lookup_reuses_matching_head_terminal_target_match() {
let code_home = TempDir::new().unwrap();
let repo = TempDir::new().unwrap();
set_code_home(code_home.path());
let mut store = AutoReviewRunStore::open(repo.path()).unwrap();

let run_id = Uuid::new_v4();
let mut run = AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1);
run.diff_fingerprint = Some("diff:abc".to_string());
run.branch = Some("feature".to_string());
run.head_at_launch = Some("bbbbbbbbbbbbbbbb".to_string());
run.finding_count = 1;
run.mark_status(AutoReviewRunStatus::Completed, 2);
store.upsert(run).unwrap();

let duplicate = store
.find_duplicate_by_fingerprint_excluding_with_target(
"diff:abc",
None,
Some("feature"),
Some("bbbbbbbbbbbbbbbb"),
)
.expect("duplicate");
assert_eq!(duplicate.run_id, run_id);
assert_eq!(
duplicate.disposition,
AutoReviewDuplicateDisposition::ReuseTerminal
);
}

#[test]
#[serial]
fn duplicate_lookup_does_not_adopt_snapshotting_fingerprint_match() {
let code_home = TempDir::new().unwrap();
let repo = TempDir::new().unwrap();
set_code_home(code_home.path());
let mut store = AutoReviewRunStore::open(repo.path()).unwrap();

let run_id = Uuid::new_v4();
let mut run = AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1);
run.diff_fingerprint = Some("diff:abc".to_string());
run.mark_status(AutoReviewRunStatus::Snapshotting, 2);
store.upsert(run).unwrap();

let duplicate = store
.find_duplicate_by_fingerprint("diff:abc")
.expect("duplicate");
assert_eq!(duplicate.run_id, run_id);
assert_eq!(
duplicate.disposition,
AutoReviewDuplicateDisposition::SupersedeTerminal
);
}

#[test]
#[serial]
fn duplicate_lookup_ignores_stale_in_flight_target_match() {
let code_home = TempDir::new().unwrap();
let repo = TempDir::new().unwrap();
set_code_home(code_home.path());
let mut store = AutoReviewRunStore::open(repo.path()).unwrap();

let run_id = Uuid::new_v4();
let mut run = AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1);
run.diff_fingerprint = Some("diff:abc".to_string());
run.branch = Some("auto-review".to_string());
run.snapshot_commit = Some("aaaaaaaaaaaaaaaa".to_string());
run.agent_id = Some("agent-live".to_string());
run.mark_status(AutoReviewRunStatus::Reviewing, 2);
store.upsert(run).unwrap();

assert!(
store
.find_duplicate_by_fingerprint_excluding_with_target(
"diff:abc",
None,
Some("feature"),
Some("bbbbbbbbbbbbbbbb"),
)
.is_none()
);
}

#[test]
#[serial]
fn supersede_by_fingerprint_omits_runs_with_findings_and_errors() {
Expand Down Expand Up @@ -2012,6 +2185,45 @@ mod tests {
assert!(loaded.get(newest).is_some());
}

#[test]
fn auto_review_freshness_classifier_marks_healthy_long_running_reviews() {
assert_eq!(
auto_review_freshness_for_agent_status(
false,
Some(LEDGER_LONG_ELAPSED_SECS),
Some(AUTO_REVIEW_TEST_STALE_SECS - 1),
AUTO_REVIEW_TEST_STALE_SECS,
),
AutoReviewFreshness::LongRunning
);
}

#[test]
fn auto_review_freshness_classifier_prefers_inactive_over_long_running() {
assert_eq!(
auto_review_freshness_for_agent_status(
false,
Some(LEDGER_LONG_ELAPSED_SECS + 60),
Some(AUTO_REVIEW_TEST_STALE_SECS),
AUTO_REVIEW_TEST_STALE_SECS,
),
AutoReviewFreshness::Inactive
);
}

#[test]
fn auto_review_freshness_classifier_keeps_terminal_current() {
assert_eq!(
auto_review_freshness_for_agent_status(
true,
Some(LEDGER_LONG_ELAPSED_SECS + 60),
Some(AUTO_REVIEW_TEST_STALE_SECS),
AUTO_REVIEW_TEST_STALE_SECS,
),
AutoReviewFreshness::Current
);
}

#[test]
#[serial]
fn compact_ledger_omits_idle_clean_runs() {
Expand Down
19 changes: 8 additions & 11 deletions code-rs/exec/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ use code_core::protocol::ReviewOutputEvent;
use code_core::protocol::ReviewRequest;
use code_core::protocol::TaskCompleteEvent;
use code_core::review_store::{
AutoReviewFreshness,
AutoReviewRun,
AutoReviewRunSource,
AutoReviewRunStatus,
AutoReviewRunStore,
auto_review_freshness_for_agent_status,
};
use code_protocol::models::ContentItem;
use code_protocol::models::ResponseItem;
Expand Down Expand Up @@ -1820,16 +1820,13 @@ impl AutoReviewTracker {
.or(run.owner_session_id);
run.model = agent.model.clone().or_else(|| run.model.clone());
run.token_count = agent.token_count.or(run.token_count);
run.freshness = if durable_status.is_terminal() {
AutoReviewFreshness::Current
} else if agent
.seconds_since_last_activity
.is_some_and(|seconds| seconds >= AUTO_REVIEW_STALE_SECS)
{
AutoReviewFreshness::Inactive
} else {
AutoReviewFreshness::Current
};
let elapsed_secs = Some(now.saturating_sub(run.started_at.unwrap_or(run.created_at)));
run.freshness = auto_review_freshness_for_agent_status(
durable_status.is_terminal(),
elapsed_secs,
agent.seconds_since_last_activity,
AUTO_REVIEW_STALE_SECS,
);
run.mark_status(durable_status, now);
if !durable_status.is_terminal() || agent.last_activity_at.is_some() {
run.mark_activity(now);
Expand Down
13 changes: 13 additions & 0 deletions code-rs/tui/src/app/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1859,6 +1859,19 @@ impl App<'_> {
);
}
}
AppEvent::BackgroundReviewAdopted { local_run_id, adopted_run_id, worktree_path, branch, agent_id, snapshot, owner_session_id } => {
if let AppState::Chat { widget } = &mut self.app_state {
widget.on_background_review_adopted(
local_run_id,
adopted_run_id,
worktree_path,
branch,
agent_id,
snapshot,
owner_session_id,
);
}
}
AppEvent::BackgroundReviewFinished { run_id, worktree_path, branch, has_findings, findings, summary, error, agent_id, snapshot, owner_session_id } => {
if let AppState::Chat { widget } = &mut self.app_state {
widget.on_background_review_finished_for_run(
Expand Down
9 changes: 9 additions & 0 deletions code-rs/tui/src/app_event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,15 @@ pub(crate) enum AppEvent {
snapshot: Option<String>,
owner_session_id: Option<Uuid>,
},
BackgroundReviewAdopted {
local_run_id: Uuid,
adopted_run_id: Uuid,
worktree_path: PathBuf,
branch: String,
agent_id: Option<String>,
snapshot: Option<String>,
owner_session_id: Option<Uuid>,
},
BackgroundReviewFinished {
run_id: Uuid,
worktree_path: PathBuf,
Expand Down
Loading