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
29 changes: 25 additions & 4 deletions codex-rs/app-server/src/request_processors/turn_processor.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use super::*;
use codex_auto_review::ReviewCoordination;
use codex_exec_server::LOCAL_ENVIRONMENT_ID;
use codex_protocol::protocol::AdditionalContextEntry as CoreAdditionalContextEntry;
use codex_protocol::protocol::AdditionalContextKind as CoreAdditionalContextKind;
Expand Down Expand Up @@ -355,7 +356,7 @@ impl TurnRequestProcessor {
Ok(environment_selections)
}

async fn auto_review_target_for_thread(thread: &CodexThread) -> AutoReviewRunTarget {
async fn auto_review_target_for_thread(&self, thread: &CodexThread) -> AutoReviewRunTarget {
let snapshot = thread.config_snapshot().await;
let environments = thread.environment_selections().await;
let cwd = match environments.as_slice() {
Expand All @@ -366,6 +367,19 @@ impl TurnRequestProcessor {
};
let git_info = collect_git_info(cwd.as_path()).await;
let repo_root = get_git_repo_root(cwd.as_path());
let worktree_path = repo_root.or_else(|| Some(cwd.as_path().to_path_buf()));
let snapshot_epoch = worktree_path.as_ref().and_then(|scope| {
match ReviewCoordination::for_scope(self.config.codex_home.as_ref(), scope)
.current_snapshot_epoch()
{
Ok(0) => None,
Ok(epoch) => Some(epoch),
Err(err) => {
tracing::warn!(error = %err, "failed to collect app-server auto review snapshot epoch");
None
}
}
});
let worktree_diff_fingerprint = get_worktree_diff_fingerprint(cwd.as_path()).await;

AutoReviewRunTarget {
Expand All @@ -374,7 +388,14 @@ impl TurnRequestProcessor {
.as_ref()
.and_then(|git| git.commit_hash.as_ref().map(|sha| sha.0.clone())),
base_sha: None,
worktree_path: repo_root.or_else(|| Some(cwd.as_path().to_path_buf())),
worktree_path,
snapshot_epoch,
snapshot_commit: git_info
.as_ref()
.and_then(|git| git.commit_hash.as_ref().map(|sha| sha.0.clone())),
head_at_launch: git_info
.as_ref()
.and_then(|git| git.commit_hash.as_ref().map(|sha| sha.0.clone())),
worktree_diff_fingerprint,
}
}
Expand Down Expand Up @@ -1359,7 +1380,7 @@ impl TurnRequestProcessor {
let AutoReviewSummaryReadParams { thread_id } = params;
let (_, thread) = self.load_thread(&thread_id).await?;
let active_review_target = CoreReviewTarget::UncommittedChanges;
let active_target = Self::auto_review_target_for_thread(thread.as_ref()).await;
let active_target = self.auto_review_target_for_thread(thread.as_ref()).await;
let store_scope = active_target
.worktree_path
.as_deref()
Expand Down Expand Up @@ -1445,7 +1466,7 @@ impl TurnRequestProcessor {

let (_, thread) = self.load_thread(&thread_id).await?;
let active_review_target = CoreReviewTarget::UncommittedChanges;
let active_target = Self::auto_review_target_for_thread(thread.as_ref()).await;
let active_target = self.auto_review_target_for_thread(thread.as_ref()).await;
let store_scope = active_target
.worktree_path
.as_deref()
Expand Down
3 changes: 3 additions & 0 deletions codex-rs/app-server/tests/suite/v2/review.rs
Original file line number Diff line number Diff line change
Expand Up @@ -941,6 +941,9 @@ fn sample_auto_review_run(
head_sha: None,
base_sha: None,
worktree_path: Some(worktree_path.to_path_buf()),
snapshot_epoch: None,
snapshot_commit: None,
head_at_launch: None,
worktree_diff_fingerprint: None,
},
review_target: CoreReviewTarget::UncommittedChanges,
Expand Down
77 changes: 74 additions & 3 deletions codex-rs/auto-review/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ impl AutoReviewStore {
for run in self.load_index()?.runs {
if run.run_id == superseded_by
|| run.target.worktree_diff_fingerprint.as_deref() != Some(fingerprint)
|| !duplicate_target_is_reusable(&run, active_branch, active_head)
|| !duplicate_target_matches_branch_head(&run, active_branch, active_head)
{
continue;
}
Expand Down Expand Up @@ -248,7 +248,54 @@ impl AutoReviewStore {
| AutoReviewRunStatus::Superseded
)
})
.filter(|run| duplicate_target_is_reusable(run, active_branch, active_head))
.filter(|run| duplicate_target_matches_branch_head(run, active_branch, active_head))
.filter_map(|run| {
let duplicate = AutoReviewDuplicateMatch {
run_id: run.run_id.clone(),
status: run.status.clone(),
disposition: duplicate_disposition(&run),
finding_count: run.findings.len(),
model: run.model.clone(),
};
is_eligible(&duplicate).then_some((run, duplicate))
})
.max_by(|left, right| {
duplicate_priority(&left.0)
.cmp(&duplicate_priority(&right.0))
.then_with(|| {
auto_review_run_sort_key(&left.0).cmp(&auto_review_run_sort_key(&right.0))
})
})
.map(|(_run, duplicate)| duplicate))
}

pub fn find_duplicate_by_fingerprint_with_target_proof_and_filter<F>(
&self,
diff_fingerprint: &str,
active_target: Option<&AutoReviewRunTarget>,
is_eligible: F,
) -> Result<Option<AutoReviewDuplicateMatch>>
where
F: Fn(&AutoReviewDuplicateMatch) -> bool,
{
let fingerprint = diff_fingerprint.trim();
if fingerprint.is_empty() {
return Ok(None);
}
Ok(self
.load_index_for_read()?
.runs
.into_iter()
.filter(|run| run.target.worktree_diff_fingerprint.as_deref() == Some(fingerprint))
.filter(|run| {
!matches!(
run.status,
AutoReviewRunStatus::Lost
| AutoReviewRunStatus::Skipped
| AutoReviewRunStatus::Superseded
)
})
.filter(|run| duplicate_target_is_reusable(run, active_target))
.filter_map(|run| {
let duplicate = AutoReviewDuplicateMatch {
run_id: run.run_id.clone(),
Expand Down Expand Up @@ -670,6 +717,12 @@ pub struct AutoReviewRunTarget {
pub base_sha: Option<String>,
pub worktree_path: Option<PathBuf>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub snapshot_epoch: Option<u64>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub snapshot_commit: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub head_at_launch: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub worktree_diff_fingerprint: Option<String>,
}

Expand All @@ -684,6 +737,15 @@ impl AutoReviewRunTarget {
if self.base_sha != active.base_sha {
return AutoReviewFreshness::Stale;
}
if active.snapshot_epoch.is_some() && self.snapshot_epoch != active.snapshot_epoch {
return AutoReviewFreshness::Stale;
}
if self.snapshot_commit.is_some()
&& active.snapshot_commit.is_some()
&& self.snapshot_commit != active.snapshot_commit
{
return AutoReviewFreshness::Stale;
}
if self.worktree_diff_fingerprint != active.worktree_diff_fingerprint {
return AutoReviewFreshness::Stale;
}
Expand Down Expand Up @@ -778,7 +840,7 @@ fn duplicate_disposition(run: &AutoReviewRun) -> AutoReviewDuplicateDisposition
}
}

fn duplicate_target_is_reusable(
fn duplicate_target_matches_branch_head(
run: &AutoReviewRun,
active_branch: Option<&str>,
active_head: Option<&str>,
Expand All @@ -794,6 +856,15 @@ fn duplicate_target_is_reusable(
run.target.head_sha.as_deref() == active_head.and_then(non_empty_str)
}

fn duplicate_target_is_reusable(
run: &AutoReviewRun,
active_target: Option<&AutoReviewRunTarget>,
) -> bool {
active_target.is_none_or(|active_target| {
run.target.freshness(active_target) == AutoReviewFreshness::Current
})
}

fn non_empty_str(value: &str) -> Option<&str> {
let value = value.trim();
(!value.is_empty()).then_some(value)
Expand Down
101 changes: 101 additions & 0 deletions codex-rs/auto-review/src/lib_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,66 @@ fn freshness_classifies_current_stale_and_detached() {
);
}

#[test]
fn freshness_treats_known_snapshot_epoch_mismatch_as_stale() {
let epoch_zero_active = AutoReviewRunTarget {
snapshot_epoch: None,
..sample_target("main", "head-2", "/repo")
};
let active = AutoReviewRunTarget {
snapshot_epoch: Some(2),
..sample_target("main", "head-2", "/repo")
};
let stale_epoch = AutoReviewRunTarget {
snapshot_epoch: Some(1),
..sample_target("main", "head-2", "/repo")
};
let legacy_without_epoch = sample_target("main", "head-2", "/repo");

assert_eq!(
legacy_without_epoch.freshness(&epoch_zero_active),
AutoReviewFreshness::Current
);
assert_eq!(stale_epoch.freshness(&active), AutoReviewFreshness::Stale);
assert_eq!(
legacy_without_epoch.freshness(&active),
AutoReviewFreshness::Stale
);
}

#[test]
fn freshness_treats_known_snapshot_commit_mismatch_as_stale() {
let active = AutoReviewRunTarget {
snapshot_commit: Some("snapshot-new".to_string()),
..sample_target("main", "head-2", "/repo")
};
let stale_snapshot = AutoReviewRunTarget {
snapshot_commit: Some("snapshot-old".to_string()),
..sample_target("main", "head-2", "/repo")
};

assert_eq!(
stale_snapshot.freshness(&active),
AutoReviewFreshness::Stale
);
}

#[test]
fn legacy_run_targets_without_snapshot_proof_still_deserialize() -> anyhow::Result<()> {
let target: AutoReviewRunTarget = serde_json::from_value(serde_json::json!({
"branch": "main",
"head_sha": "head-2",
"base_sha": "base-1",
"worktree_path": "/repo"
}))?;

assert_eq!(target.snapshot_epoch, None);
assert_eq!(target.snapshot_commit, None);
assert_eq!(target.head_at_launch, None);
assert_eq!(target.worktree_diff_fingerprint, None);
Ok(())
}

#[test]
fn summary_only_surfaces_current_findings() {
let active = sample_target("main", "head-2", "/repo");
Expand Down Expand Up @@ -800,6 +860,35 @@ fn duplicate_lookup_can_filter_stale_target_matches() -> anyhow::Result<()> {
Ok(())
}

#[test]
fn duplicate_lookup_with_target_proof_rejects_mismatched_snapshot_epoch() -> anyhow::Result<()> {
let codex_home = tempfile::tempdir()?;
let scope = tempfile::tempdir()?;
let store = AutoReviewStore::for_scope(codex_home.path(), scope.path());
store.save_run(&AutoReviewRun {
target: AutoReviewRunTarget {
snapshot_epoch: Some(1),
worktree_diff_fingerprint: Some("sha256:abc".to_string()),
..sample_target("main", "head-2", "/repo")
},
..sample_run("stale_duplicate", vec![sample_finding("f1", "Old")])
})?;
let active_target = AutoReviewRunTarget {
snapshot_epoch: Some(2),
worktree_diff_fingerprint: Some("sha256:abc".to_string()),
..sample_target("main", "head-2", "/repo")
};

let duplicate = store.find_duplicate_by_fingerprint_with_target_proof_and_filter(
"sha256:abc",
Some(&active_target),
|_| true,
)?;

assert_eq!(duplicate, None);
Ok(())
}

#[test]
fn duplicate_lookup_reports_pending_but_ignores_lost_skipped_and_superseded() -> anyhow::Result<()>
{
Expand Down Expand Up @@ -1067,6 +1156,9 @@ fn commit_review_targets_ignore_checkout_metadata_when_reopened() {
head_sha: Some("abc123".to_string()),
base_sha: Some("base-old".to_string()),
worktree_path: Some(PathBuf::from("/repo-old")),
snapshot_epoch: None,
snapshot_commit: None,
head_at_launch: None,
worktree_diff_fingerprint: Some("sha256:old".to_string()),
},
review_target: ReviewTarget::Commit {
Expand All @@ -1080,13 +1172,19 @@ fn commit_review_targets_ignore_checkout_metadata_when_reopened() {
head_sha: Some("abc123".to_string()),
base_sha: None,
worktree_path: None,
snapshot_epoch: None,
snapshot_commit: None,
head_at_launch: None,
worktree_diff_fingerprint: Some("sha256:new".to_string()),
};
let reopened_renamed_branch = AutoReviewRunTarget {
branch: Some("feature-renamed".to_string()),
head_sha: Some("abc123".to_string()),
base_sha: Some("base-new".to_string()),
worktree_path: Some(PathBuf::from("/repo-new")),
snapshot_epoch: None,
snapshot_commit: None,
head_at_launch: None,
worktree_diff_fingerprint: Some("sha256:new".to_string()),
};

Expand Down Expand Up @@ -1266,6 +1364,9 @@ fn sample_target(branch: &str, head_sha: &str, worktree_path: &str) -> AutoRevie
head_sha: Some(head_sha.to_string()),
base_sha: Some("base-1".to_string()),
worktree_path: Some(PathBuf::from(worktree_path)),
snapshot_epoch: None,
snapshot_commit: None,
head_at_launch: None,
worktree_diff_fingerprint: None,
}
}
Expand Down
2 changes: 1 addition & 1 deletion codex-rs/core/src/context/auto_review_awareness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ async fn build_auto_review_awareness_inner(
}

let active_review_target = ReviewTarget::UncommittedChanges;
let active_target = collect_auto_review_target(cwd, &active_review_target).await;
let active_target = collect_auto_review_target(codex_home, cwd, &active_review_target).await;
let store_scope = active_target.worktree_path.as_deref().unwrap_or(cwd);
let runs = AutoReviewStore::for_scope(codex_home, store_scope).list_runs()?;
if runs.is_empty()
Expand Down
3 changes: 3 additions & 0 deletions codex-rs/core/src/context/auto_review_awareness_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,9 @@ fn sample_target(branch: &str, head_sha: &str, worktree_path: &str) -> AutoRevie
head_sha: Some(head_sha.to_string()),
base_sha: Some("base-1".to_string()),
worktree_path: Some(PathBuf::from(worktree_path)),
snapshot_epoch: None,
snapshot_commit: None,
head_at_launch: None,
worktree_diff_fingerprint: None,
}
}
Expand Down
Loading
Loading