mirror of
https://github.com/hanzoai/dev.git
synced 2026-10-25 03:32:32 +00:00
feat(auto-review): reuse worktree and pass turn context
This commit is contained in:
Generated
+2
@@ -1166,6 +1166,7 @@ dependencies = [
|
||||
"code-apply-patch",
|
||||
"code-browser",
|
||||
"code-file-search",
|
||||
"code-git-tooling",
|
||||
"code-mcp-client",
|
||||
"code-mcp-types",
|
||||
"code-otel",
|
||||
@@ -1225,6 +1226,7 @@ dependencies = [
|
||||
"tree-sitter-bash",
|
||||
"url",
|
||||
"uuid",
|
||||
"walkdir",
|
||||
"which",
|
||||
"wildmatch",
|
||||
"windows-sys 0.61.2",
|
||||
|
||||
@@ -1585,6 +1585,9 @@ fn run_git_command<const N: usize>(args: [&str; N]) -> Option<String> {
|
||||
if !output.status.success() {
|
||||
return None;
|
||||
}
|
||||
if args.iter().any(|arg| matches!(*arg, "pull" | "checkout" | "merge" | "apply")) {
|
||||
code_core::review_coord::bump_snapshot_epoch();
|
||||
}
|
||||
String::from_utf8(output.stdout)
|
||||
.ok()
|
||||
.map(|text| text.trim_end().to_string())
|
||||
|
||||
@@ -3,6 +3,7 @@ use std::time::{Duration, Instant};
|
||||
use code_common::elapsed::format_duration;
|
||||
use code_core::protocol::ReviewContextMetadata;
|
||||
use code_core::protocol::ReviewOutputEvent;
|
||||
use code_core::review_coord::{bump_snapshot_epoch, try_acquire_lock};
|
||||
use code_git_tooling::GhostCommit;
|
||||
|
||||
use crate::AutoTurnAgentsAction;
|
||||
@@ -219,6 +220,7 @@ pub struct AutoResolveState {
|
||||
pub last_review: Option<ReviewOutputEvent>,
|
||||
pub last_fix_message: Option<String>,
|
||||
pub last_reviewed_commit: Option<String>,
|
||||
pub snapshot_epoch: Option<u64>,
|
||||
}
|
||||
|
||||
impl AutoResolveState {
|
||||
@@ -242,6 +244,7 @@ impl AutoResolveState {
|
||||
last_review: None,
|
||||
last_fix_message: None,
|
||||
last_reviewed_commit: None,
|
||||
snapshot_epoch: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -325,6 +328,8 @@ pub struct AutoDriveController {
|
||||
pub pending_stop_message: Option<String>,
|
||||
pub last_completion_explanation: Option<String>,
|
||||
pub phase: AutoRunPhase,
|
||||
// Non-cloneable guard is kept separately; controller stays Clone.
|
||||
pub review_lock: Option<std::sync::Arc<code_core::review_coord::ReviewGuard>>,
|
||||
}
|
||||
|
||||
impl AutoDriveController {
|
||||
@@ -377,14 +382,24 @@ impl AutoDriveController {
|
||||
|
||||
pub fn on_resume_from_manual(&mut self) {
|
||||
self.apply_phase(AutoRunPhase::Active);
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
|
||||
pub fn on_begin_review(&mut self, diagnostics_pending: bool) {
|
||||
self.apply_phase(AutoRunPhase::AwaitingReview { diagnostics_pending });
|
||||
// Acquire global review lock; if busy or error, fall back to Active to avoid overlap.
|
||||
self.review_lock = try_acquire_lock("auto-drive-review", std::path::Path::new("."))
|
||||
.ok()
|
||||
.flatten()
|
||||
.map(std::sync::Arc::new);
|
||||
if self.review_lock.is_none() {
|
||||
self.apply_phase(AutoRunPhase::Active);
|
||||
}
|
||||
}
|
||||
|
||||
pub fn on_complete_review(&mut self) {
|
||||
self.apply_phase(AutoRunPhase::Active);
|
||||
self.review_lock = None;
|
||||
}
|
||||
|
||||
pub fn on_transient_failure(&mut self, backoff_ms: u64) {
|
||||
@@ -430,10 +445,26 @@ impl AutoDriveController {
|
||||
self.apply_phase(AutoRunPhase::AwaitingDiagnostics { coordinator_waiting: true });
|
||||
}
|
||||
(AutoRunPhase::AwaitingDiagnostics { .. } | AutoRunPhase::Active, PhaseTransition::BeginReview { diagnostics_pending }) => {
|
||||
if self.review_lock.is_none() {
|
||||
self.review_lock = code_core::review_coord::try_acquire_lock(
|
||||
"auto-drive-review",
|
||||
std::path::Path::new("."),
|
||||
)
|
||||
.ok()
|
||||
.flatten()
|
||||
.map(std::sync::Arc::new);
|
||||
if self.review_lock.is_none() {
|
||||
// Unable to secure the global lock; stay active to avoid overlapping reviews.
|
||||
self.apply_phase(AutoRunPhase::Active);
|
||||
return TransitionEffects { effects, phase_changed: old_phase != self.phase };
|
||||
}
|
||||
}
|
||||
|
||||
self.apply_phase(AutoRunPhase::AwaitingReview { diagnostics_pending: *diagnostics_pending });
|
||||
}
|
||||
(AutoRunPhase::AwaitingReview { .. }, PhaseTransition::CompleteReview) => {
|
||||
self.apply_phase(AutoRunPhase::Active);
|
||||
self.review_lock = None;
|
||||
}
|
||||
(_, PhaseTransition::TransientFailure { backoff_ms }) => {
|
||||
if self.phase.is_active() {
|
||||
@@ -445,6 +476,7 @@ impl AutoDriveController {
|
||||
}
|
||||
(_, PhaseTransition::Stop) => {
|
||||
self.apply_phase(AutoRunPhase::Idle);
|
||||
self.review_lock = None;
|
||||
}
|
||||
_ => {
|
||||
}
|
||||
@@ -758,6 +790,7 @@ impl AutoDriveController {
|
||||
self.elapsed_override = elapsed_override;
|
||||
self.pending_stop_message = pending_stop_message;
|
||||
self.last_completion_explanation = last_completion_explanation;
|
||||
self.review_lock = None;
|
||||
self.phase = if self.phase.is_active() {
|
||||
AutoRunPhase::Active
|
||||
} else {
|
||||
|
||||
@@ -119,6 +119,8 @@ serial_test = "3.2.0"
|
||||
pretty_assertions = { workspace = true }
|
||||
tokio-test = { workspace = true }
|
||||
wiremock = { workspace = true }
|
||||
code-git-tooling = { path = "../git-tooling" }
|
||||
walkdir = { workspace = true }
|
||||
|
||||
[package.metadata.cargo-shear]
|
||||
ignored = ["openssl-sys"]
|
||||
|
||||
@@ -0,0 +1,50 @@
|
||||
# Auto-review / Auto-resolve Workflow (Exec, TUI, Auto-drive)
|
||||
|
||||
This note summarizes how automated reviews flow across the CLI layers and what invariants keep snapshots fresh and non-overlapping.
|
||||
|
||||
## Overview
|
||||
- **Exec (/review + auto-resolve):** Captures a ghost snapshot, acquires the shared review lock, runs review, and may loop fixes → follow-up review until clean or limits hit.
|
||||
- **TUI (interactive + background):** Triggers reviews, renders progress, and now shares the same lock + epoch rules so background reviews cannot overlap or reuse stale snapshots.
|
||||
- **Auto-drive:** Orchestrates turns; when it triggers reviews/auto-resolve it also takes the global lock and relies on the same snapshot epoch to reject stale follow-ups.
|
||||
|
||||
## Core states
|
||||
- **Review lock (file-based):** Only one review pipeline (exec/TUI/auto-drive) may run at a time. If lock acquisition fails, the review is skipped/deferred.
|
||||
- **Snapshot epoch (monotonic counter):** Bumped on every git mutation we control (ghost snapshots, git helpers, worktree/remote ops). Follow-up reviews compare epochs and abort if the world moved.
|
||||
- **Auto-resolve phases:**
|
||||
- `PendingFix` → apply patch
|
||||
- `AwaitingFix` → wait for model output
|
||||
- `WaitingForReview` → capture fresh snapshot, re-review
|
||||
- Abort when identical snapshot, stale base, or epoch advanced
|
||||
|
||||
## Ghost snapshots
|
||||
- Ghost commits are taken via `CreateGhostCommitOptions` **with** `post_commit_hook(&|| bump_snapshot_epoch())` so every snapshot invalidates prior review targets.
|
||||
- Stored metadata (commit hash + epoch) is compared before follow-up reviews; mismatch stops the loop.
|
||||
|
||||
## Lock + epoch rules
|
||||
- Acquire the shared lock before sending a review or follow-up. Release on completion/stop/reset.
|
||||
- If lock busy: skip/defer rather than overlap.
|
||||
- If `current_snapshot_epoch` differs from the recorded epoch for the review context, abort and recapture.
|
||||
|
||||
## Git mutations: use the helpers
|
||||
- Use `chatwidget::run_git_command` (TUI) or the auto-drive git helper; both bump epoch on mutating verbs (`pull`, `checkout`, `merge`, `apply`).
|
||||
- Worktree/remote management goes through `core::git_worktree`, which bumps epoch on remove/add/remote updates.
|
||||
- Ghost snapshots already bump epoch via the post-commit hook.
|
||||
- New git touchpoints must either call these helpers or call `bump_snapshot_epoch()` immediately after a successful mutation. The `git_mutation_guard` test enforces this for TUI/auto-drive.
|
||||
|
||||
## Cleanup
|
||||
- Session worktree cleanup acquires the shared lock with retries; successful removal bumps the epoch so stale reviews are invalidated. If lock stays busy, cleanup defers to a later run.
|
||||
|
||||
## Failure handling
|
||||
- Follow-up review skipped when:
|
||||
- lock unavailable
|
||||
- snapshot identical to last reviewed commit
|
||||
- base snapshot no longer ancestor of HEAD
|
||||
- epoch advanced (stale snapshot)
|
||||
- Apply/patch failure + resume must recapture; epoch drift detects staleness.
|
||||
|
||||
## What to remember when extending
|
||||
- Add new git mutations via the shared helpers or bump the epoch manually.
|
||||
- If you add a new review entrypoint, acquire the shared lock.
|
||||
- When capturing snapshots, record the epoch and base commit; compare before re-reviewing.
|
||||
- If you must bypass the guardrails, document the reason and keep the code path read-only.
|
||||
|
||||
@@ -6,6 +6,8 @@ use std::path::{Path, PathBuf};
|
||||
use tokio::fs::OpenOptions;
|
||||
use tokio::process::Command;
|
||||
|
||||
use crate::review_coord::bump_snapshot_epoch;
|
||||
|
||||
/// Returns the `/.../.code/branches/<worktree>` root when `path` resides inside a branch worktree.
|
||||
pub fn branch_worktree_root(path: &Path) -> Option<PathBuf> {
|
||||
use std::ffi::OsStr;
|
||||
@@ -150,6 +152,34 @@ pub async fn setup_worktree(
|
||||
let mut worktree_path = code_dir.join(&effective_branch);
|
||||
let reuse_allowed = base_ref.is_none();
|
||||
if worktree_path.exists() {
|
||||
if let Some(base) = base_ref {
|
||||
// Reset existing worktree to the requested base instead of deleting it
|
||||
// so caches (e.g., target/) are preserved.
|
||||
let reset = Command::new("git")
|
||||
.current_dir(&worktree_path)
|
||||
.args(["reset", "--hard", base])
|
||||
.output()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to reset existing worktree: {}", e))?;
|
||||
if !reset.status.success() {
|
||||
let stderr = String::from_utf8_lossy(&reset.stderr);
|
||||
return Err(format!("Failed to reset existing worktree: {stderr}"));
|
||||
}
|
||||
let clean = Command::new("git")
|
||||
.current_dir(&worktree_path)
|
||||
.args(["clean", "-fd"])
|
||||
.output()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to clean existing worktree: {}", e))?;
|
||||
if !clean.status.success() {
|
||||
let stderr = String::from_utf8_lossy(&clean.stderr);
|
||||
return Err(format!("Failed to clean existing worktree: {stderr}"));
|
||||
}
|
||||
bump_snapshot_epoch();
|
||||
record_worktree_in_session(git_root, &worktree_path).await;
|
||||
return Ok((worktree_path, effective_branch));
|
||||
}
|
||||
|
||||
if reuse_allowed {
|
||||
// Re-use existing worktree for speed when it does not need to be
|
||||
// pinned to a specific snapshot.
|
||||
@@ -157,16 +187,20 @@ pub async fn setup_worktree(
|
||||
return Ok((worktree_path, effective_branch));
|
||||
}
|
||||
|
||||
// Snapshot-specific worktrees must reflect the requested base ref, so
|
||||
// remove any stale directory before recreating.
|
||||
let _ = Command::new("git")
|
||||
// Fallback: remove if we cannot reuse
|
||||
if let Ok(out) = Command::new("git")
|
||||
.arg("worktree")
|
||||
.arg("remove")
|
||||
.arg("--force")
|
||||
.arg(worktree_path.to_str().unwrap())
|
||||
.current_dir(git_root)
|
||||
.output()
|
||||
.await;
|
||||
.await
|
||||
{
|
||||
if out.status.success() {
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let mut args = vec![
|
||||
@@ -189,6 +223,10 @@ pub async fn setup_worktree(
|
||||
.await
|
||||
.map_err(|e| format!("Failed to create git worktree: {}", e))?;
|
||||
|
||||
if output.status.success() {
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
|
||||
if !output.status.success() {
|
||||
let stderr = String::from_utf8_lossy(&output.stderr);
|
||||
// If the branch already exists, generate a unique name and retry once.
|
||||
@@ -228,6 +266,7 @@ pub async fn setup_worktree(
|
||||
let retry_err = String::from_utf8_lossy(&retry.stderr);
|
||||
return Err(format!("Failed to create worktree: {}", retry_err));
|
||||
}
|
||||
bump_snapshot_epoch();
|
||||
record_worktree_in_session(git_root, &worktree_path).await;
|
||||
} else {
|
||||
return Err(format!("Failed to create worktree: {}", stderr));
|
||||
@@ -241,6 +280,94 @@ pub async fn setup_worktree(
|
||||
Ok((worktree_path, effective_branch))
|
||||
}
|
||||
|
||||
/// Prepare (or create) a reusable worktree pinned to `base_ref`, keeping
|
||||
/// gitignored build outputs between runs. When the worktree already exists we
|
||||
/// simply reset and clean it; otherwise we create it detached at the requested
|
||||
/// commit. Returns the worktree path.
|
||||
pub async fn prepare_reusable_worktree(
|
||||
git_root: &Path,
|
||||
worktree_name: &str,
|
||||
base_ref: &str,
|
||||
keep_gitignored: bool,
|
||||
) -> Result<PathBuf, String> {
|
||||
// Global location: ~/.code/working/<repo_name>/branches
|
||||
let repo_name = git_root
|
||||
.file_name()
|
||||
.and_then(|s| s.to_str())
|
||||
.unwrap_or("repo");
|
||||
let mut code_dir = dirs::home_dir().unwrap_or_else(|| PathBuf::from("."));
|
||||
code_dir = code_dir
|
||||
.join(".code")
|
||||
.join("working")
|
||||
.join(repo_name)
|
||||
.join("branches");
|
||||
tokio::fs::create_dir_all(&code_dir)
|
||||
.await
|
||||
.map_err(|e| format!("Failed to create .code/branches directory: {}", e))?;
|
||||
|
||||
let worktree_path = code_dir.join(worktree_name);
|
||||
|
||||
if worktree_path.exists() {
|
||||
// Reset to requested snapshot and clean tracked files; keep gitignored
|
||||
// outputs (e.g., target/) when desired.
|
||||
let reset = Command::new("git")
|
||||
.current_dir(&worktree_path)
|
||||
.args(["reset", "--hard", base_ref])
|
||||
.output()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to reset reusable worktree: {}", e))?;
|
||||
if !reset.status.success() {
|
||||
let stderr = String::from_utf8_lossy(&reset.stderr);
|
||||
return Err(format!("Failed to reset reusable worktree: {stderr}"));
|
||||
}
|
||||
|
||||
let clean_args = if keep_gitignored {
|
||||
vec!["clean", "-fd"]
|
||||
} else {
|
||||
vec!["clean", "-fdx"]
|
||||
};
|
||||
let clean = Command::new("git")
|
||||
.current_dir(&worktree_path)
|
||||
.args(&clean_args)
|
||||
.output()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to clean reusable worktree: {}", e))?;
|
||||
if !clean.status.success() {
|
||||
let stderr = String::from_utf8_lossy(&clean.stderr);
|
||||
return Err(format!("Failed to clean reusable worktree: {stderr}"));
|
||||
}
|
||||
|
||||
bump_snapshot_epoch();
|
||||
record_worktree_in_session(git_root, &worktree_path).await;
|
||||
return Ok(worktree_path);
|
||||
}
|
||||
|
||||
// Create detached worktree at the snapshot commit.
|
||||
let output = Command::new("git")
|
||||
.current_dir(git_root)
|
||||
.args([
|
||||
"worktree",
|
||||
"add",
|
||||
"--detach",
|
||||
worktree_path
|
||||
.to_str()
|
||||
.ok_or_else(|| "Invalid worktree path".to_string())?,
|
||||
base_ref,
|
||||
])
|
||||
.output()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to create reusable worktree: {}", e))?;
|
||||
|
||||
if !output.status.success() {
|
||||
let stderr = String::from_utf8_lossy(&output.stderr);
|
||||
return Err(format!("Failed to create reusable worktree: {stderr}"));
|
||||
}
|
||||
|
||||
bump_snapshot_epoch();
|
||||
record_worktree_in_session(git_root, &worktree_path).await;
|
||||
Ok(worktree_path)
|
||||
}
|
||||
|
||||
/// Append the created worktree to a per-process session file so the TUI can
|
||||
/// clean it up on exit without touching worktrees from other processes.
|
||||
async fn record_worktree_in_session(git_root: &Path, worktree_path: &Path) {
|
||||
@@ -287,6 +414,7 @@ pub async fn ensure_local_default_remote(
|
||||
let stderr = String::from_utf8_lossy(&update.stderr).trim().to_string();
|
||||
return Err(format!("Failed to set {remote_name} URL: {stderr}"));
|
||||
}
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
}
|
||||
_ => {
|
||||
@@ -300,6 +428,7 @@ pub async fn ensure_local_default_remote(
|
||||
let stderr = String::from_utf8_lossy(&add.stderr).trim().to_string();
|
||||
return Err(format!("Failed to add {remote_name}: {stderr}"));
|
||||
}
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -338,6 +467,7 @@ pub async fn ensure_local_default_remote(
|
||||
.map_err(|e| format!("Failed to update {remote_ref}: {e}"))?;
|
||||
if update.status.success() {
|
||||
metadata.remote_ref = Some(format!("{remote_name}/{base}"));
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -462,6 +592,7 @@ async fn _ensure_origin_remote(git_root: &Path) -> Result<(), String> {
|
||||
if !add.status.success() {
|
||||
return Err("failed to add origin".to_string());
|
||||
}
|
||||
bump_snapshot_epoch();
|
||||
let _ = Command::new("git")
|
||||
.current_dir(git_root)
|
||||
.args(["remote", "set-head", "origin", "-a"])
|
||||
|
||||
@@ -30,6 +30,7 @@ pub mod context_timeline;
|
||||
pub mod acp;
|
||||
pub mod custom_prompts;
|
||||
pub mod debug_logger;
|
||||
pub mod review_coord;
|
||||
mod environment_context;
|
||||
mod reasoning;
|
||||
pub mod retention;
|
||||
|
||||
@@ -0,0 +1,287 @@
|
||||
use serde::{Deserialize, Serialize};
|
||||
use std::fs::{self, File, OpenOptions};
|
||||
use std::io::{Read, Write};
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::time::{SystemTime, UNIX_EPOCH};
|
||||
|
||||
const LOCK_FILENAME: &str = "review.lock";
|
||||
const EPOCH_FILENAME: &str = "snapshot.epoch";
|
||||
|
||||
#[derive(Debug, Serialize, Deserialize)]
|
||||
pub struct ReviewLockInfo {
|
||||
pub pid: u32,
|
||||
pub started_at: u64,
|
||||
pub intent: String,
|
||||
pub git_head: Option<String>,
|
||||
pub snapshot_epoch: u64,
|
||||
}
|
||||
|
||||
pub struct ReviewGuard {
|
||||
lock_path: PathBuf,
|
||||
}
|
||||
|
||||
fn state_dir() -> std::io::Result<PathBuf> {
|
||||
let mut dir = crate::config::find_code_home()?;
|
||||
dir.push("state");
|
||||
dir.push("review");
|
||||
fs::create_dir_all(&dir)?;
|
||||
Ok(dir)
|
||||
}
|
||||
|
||||
fn epoch_path() -> std::io::Result<PathBuf> {
|
||||
let mut p = state_dir()?;
|
||||
p.push(EPOCH_FILENAME);
|
||||
Ok(p)
|
||||
}
|
||||
|
||||
fn lock_path() -> std::io::Result<PathBuf> {
|
||||
let mut p = state_dir()?;
|
||||
p.push(LOCK_FILENAME);
|
||||
Ok(p)
|
||||
}
|
||||
|
||||
fn read_epoch() -> u64 {
|
||||
if let Ok(p) = epoch_path() {
|
||||
if let Ok(text) = fs::read_to_string(p) {
|
||||
if let Ok(v) = text.trim().parse::<u64>() {
|
||||
return v;
|
||||
}
|
||||
}
|
||||
}
|
||||
0
|
||||
}
|
||||
|
||||
fn write_epoch(val: u64) -> std::io::Result<()> {
|
||||
let p = epoch_path()?;
|
||||
fs::write(p, val.to_string())
|
||||
}
|
||||
|
||||
pub fn bump_snapshot_epoch() {
|
||||
let current = read_epoch();
|
||||
let _ = write_epoch(current.saturating_add(1));
|
||||
}
|
||||
|
||||
pub fn current_snapshot_epoch() -> u64 {
|
||||
read_epoch()
|
||||
}
|
||||
|
||||
fn git_head(cwd: &Path) -> Option<String> {
|
||||
let output = std::process::Command::new("git")
|
||||
.current_dir(cwd)
|
||||
.args(["rev-parse", "HEAD"])
|
||||
.output()
|
||||
.ok()?;
|
||||
if !output.status.success() {
|
||||
return None;
|
||||
}
|
||||
Some(String::from_utf8_lossy(&output.stdout).trim().to_string())
|
||||
}
|
||||
|
||||
pub fn try_acquire_lock(intent: &str, cwd: &Path) -> std::io::Result<Option<ReviewGuard>> {
|
||||
let lock_path = lock_path()?;
|
||||
let file = OpenOptions::new()
|
||||
.write(true)
|
||||
.create_new(true)
|
||||
.open(&lock_path);
|
||||
|
||||
match file {
|
||||
Ok(mut f) => {
|
||||
let info = ReviewLockInfo {
|
||||
pid: std::process::id(),
|
||||
started_at: SystemTime::now()
|
||||
.duration_since(UNIX_EPOCH)
|
||||
.unwrap_or_default()
|
||||
.as_secs(),
|
||||
intent: intent.to_string(),
|
||||
git_head: git_head(cwd),
|
||||
snapshot_epoch: current_snapshot_epoch(),
|
||||
};
|
||||
let body = serde_json::to_string_pretty(&info).unwrap_or_default();
|
||||
let _ = f.write_all(body.as_bytes());
|
||||
Ok(Some(ReviewGuard { lock_path }))
|
||||
}
|
||||
Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => Ok(None),
|
||||
Err(err) => Err(err),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn read_lock_info() -> Option<ReviewLockInfo> {
|
||||
let path = lock_path().ok()?;
|
||||
let mut buf = String::new();
|
||||
File::open(path).ok()?.read_to_string(&mut buf).ok()?;
|
||||
serde_json::from_str(&buf).ok()
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn pid_alive(pid: u32) -> bool {
|
||||
// Safety: kill with signal 0 performs permission/aliveness check only
|
||||
let res = unsafe { libc::kill(pid as libc::pid_t, 0) };
|
||||
if res == 0 {
|
||||
true
|
||||
} else {
|
||||
// ESRCH => no such process; EPERM => process exists but not permitted
|
||||
let err = std::io::Error::last_os_error()
|
||||
.raw_os_error()
|
||||
.unwrap_or(libc::ESRCH);
|
||||
err != libc::ESRCH
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(not(unix))]
|
||||
fn pid_alive(_pid: u32) -> bool {
|
||||
// Best-effort: assume alive to avoid clobbering valid locks on non-Unix platforms
|
||||
true
|
||||
}
|
||||
|
||||
/// Remove a stale review lock if the recorded pid is no longer running.
|
||||
/// Returns true if a stale lock was cleared.
|
||||
pub fn clear_stale_lock_if_dead() -> std::io::Result<bool> {
|
||||
let info = match read_lock_info() {
|
||||
Some(i) => i,
|
||||
None => return Ok(false),
|
||||
};
|
||||
if pid_alive(info.pid) {
|
||||
return Ok(false);
|
||||
}
|
||||
if let Ok(path) = lock_path() {
|
||||
let _ = fs::remove_file(path);
|
||||
return Ok(true);
|
||||
}
|
||||
Ok(false)
|
||||
}
|
||||
|
||||
impl Drop for ReviewGuard {
|
||||
fn drop(&mut self) {
|
||||
let _ = fs::remove_file(&self.lock_path);
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use tempfile::TempDir;
|
||||
|
||||
#[test]
|
||||
fn lock_contention_and_release() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
std::env::set_var("CODE_HOME", dir.path());
|
||||
let cwd = dir.path();
|
||||
|
||||
let g1 = try_acquire_lock("test", cwd).unwrap();
|
||||
assert!(g1.is_some());
|
||||
let g2 = try_acquire_lock("test2", cwd).unwrap();
|
||||
assert!(g2.is_none());
|
||||
drop(g1);
|
||||
let g3 = try_acquire_lock("test3", cwd).unwrap();
|
||||
assert!(g3.is_some());
|
||||
drop(g3);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn epoch_bump_changes_value() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
std::env::set_var("CODE_HOME", dir.path());
|
||||
let e0 = current_snapshot_epoch();
|
||||
bump_snapshot_epoch();
|
||||
let e1 = current_snapshot_epoch();
|
||||
assert!(e1 > e0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lock_records_snapshot_epoch_and_updates_after_bump() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
std::env::set_var("CODE_HOME", dir.path());
|
||||
let cwd = dir.path();
|
||||
|
||||
bump_snapshot_epoch();
|
||||
let current = current_snapshot_epoch();
|
||||
let guard = try_acquire_lock("first", cwd).unwrap().expect("lock available");
|
||||
let info = read_lock_info().expect("lock info present");
|
||||
assert_eq!(info.snapshot_epoch, current);
|
||||
drop(guard);
|
||||
|
||||
bump_snapshot_epoch();
|
||||
let next = current_snapshot_epoch();
|
||||
assert!(next > current);
|
||||
let guard2 = try_acquire_lock("second", cwd).unwrap().expect("lock reacquired");
|
||||
let info2 = read_lock_info().expect("lock info present");
|
||||
assert_eq!(info2.snapshot_epoch, next);
|
||||
drop(guard2);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lock_info_survives_epoch_bump_for_stale_detection() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
std::env::set_var("CODE_HOME", dir.path());
|
||||
let cwd = dir.path();
|
||||
|
||||
let guard = try_acquire_lock("stale-check", cwd).unwrap().expect("lock available");
|
||||
let initial = read_lock_info().expect("lock info present");
|
||||
bump_snapshot_epoch();
|
||||
let now = current_snapshot_epoch();
|
||||
|
||||
// The on-disk lock should still show the snapshot_epoch captured at acquisition time,
|
||||
// allowing callers to detect that the world has moved on while the lock holder runs.
|
||||
let still_recorded = read_lock_info().expect("lock info present");
|
||||
assert_eq!(still_recorded.snapshot_epoch, initial.snapshot_epoch);
|
||||
assert!(now > initial.snapshot_epoch);
|
||||
drop(guard);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lock_contention_across_components() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
std::env::set_var("CODE_HOME", dir.path());
|
||||
let cwd = dir.path();
|
||||
|
||||
let exec_lock = try_acquire_lock("exec-review", cwd).unwrap();
|
||||
assert!(exec_lock.is_some());
|
||||
let tui_lock = try_acquire_lock("tui-review", cwd).unwrap();
|
||||
assert!(tui_lock.is_none(), "TUI should be blocked while exec holds lock");
|
||||
drop(exec_lock);
|
||||
let auto_lock = try_acquire_lock("auto-drive-review", cwd).unwrap();
|
||||
assert!(auto_lock.is_some());
|
||||
drop(auto_lock);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn stale_epoch_detected_after_git_mutation() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
std::env::set_var("CODE_HOME", dir.path());
|
||||
let cwd = dir.path();
|
||||
|
||||
let before = current_snapshot_epoch();
|
||||
let guard = try_acquire_lock("exec", cwd).unwrap().unwrap();
|
||||
let captured = read_lock_info().unwrap().snapshot_epoch;
|
||||
assert_eq!(captured, before);
|
||||
|
||||
bump_snapshot_epoch(); // simulate git mutation while lock holder runs
|
||||
let after = current_snapshot_epoch();
|
||||
assert!(after > captured);
|
||||
|
||||
// A follow-up caller comparing epochs would notice the mismatch.
|
||||
assert_ne!(captured, after);
|
||||
drop(guard);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apply_failure_resume_requires_fresh_epoch() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
std::env::set_var("CODE_HOME", dir.path());
|
||||
let cwd = dir.path();
|
||||
|
||||
let initial = current_snapshot_epoch();
|
||||
// Snapshot taken at initial epoch
|
||||
let snapshot_epoch = initial;
|
||||
// Simulate apply/patch failure causing repo mutation guard to bump
|
||||
bump_snapshot_epoch();
|
||||
let resumed = current_snapshot_epoch();
|
||||
assert!(resumed > snapshot_epoch);
|
||||
// A resume that still uses the old snapshot must be treated as stale
|
||||
assert_ne!(snapshot_epoch, resumed);
|
||||
let guard = try_acquire_lock("resume-review", cwd).unwrap().unwrap();
|
||||
let info = read_lock_info().unwrap();
|
||||
assert_eq!(info.snapshot_epoch, resumed);
|
||||
drop(guard);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,76 @@
|
||||
//! Guardrail to catch new direct `git` invocations in TUI/auto-drive without epoch bumping.
|
||||
//!
|
||||
//! This is intentionally simple: it scans source files for `Command::new("git")` and
|
||||
//! fails if they appear outside a small allow-list of files that already route through
|
||||
//! epoch-bumping helpers. When you add a new mutating git call, either use the shared
|
||||
//! helper that bumps `snapshot epoch` or extend the allow-list with a justification.
|
||||
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
|
||||
const TARGET_DIRS: &[&str] = &[
|
||||
"code-rs/tui/src",
|
||||
"code-rs/code-auto-drive-core/src",
|
||||
];
|
||||
|
||||
// Files allowed to host direct git invocations. They must also demonstrate an
|
||||
// epoch bump reference (either direct `bump_snapshot_epoch` or a documented helper).
|
||||
const ALLOW_LIST: &[(&str, &[&str])] = &[
|
||||
// chatwidget uses run_git_command (bumps epoch) and ghost snapshots with post_commit_hook.
|
||||
("code-rs/tui/src/chatwidget.rs", &["bump_snapshot_epoch", "run_git_command"]),
|
||||
// get_git_diff is read-only; no epoch bump required, still allowed.
|
||||
("code-rs/tui/src/get_git_diff.rs", &[]),
|
||||
// auto_coordinator bumps epoch on mutating verbs inside run_git_command.
|
||||
(
|
||||
"code-rs/code-auto-drive-core/src/auto_coordinator.rs",
|
||||
&["bump_snapshot_epoch", "run_git_command"],
|
||||
),
|
||||
// controller currently no direct git calls; kept to avoid churn, require no markers.
|
||||
("code-rs/code-auto-drive-core/src/controller.rs", &[]),
|
||||
];
|
||||
|
||||
#[test]
|
||||
fn new_git_invocations_must_use_epoch_helpers() {
|
||||
let mut offenders = Vec::new();
|
||||
|
||||
for dir in TARGET_DIRS {
|
||||
let walker = walkdir::WalkDir::new(dir).into_iter();
|
||||
for entry in walker.filter_map(Result::ok).filter(|e| e.file_type().is_file()) {
|
||||
let path = entry.path();
|
||||
let path_str = path.to_string_lossy();
|
||||
if !path_str.ends_with(".rs") {
|
||||
continue;
|
||||
}
|
||||
let contents = match fs::read_to_string(path) {
|
||||
Ok(c) => c,
|
||||
Err(_) => continue,
|
||||
};
|
||||
if !contents.contains("Command::new(\"git\")") {
|
||||
continue;
|
||||
}
|
||||
if let Some((_, markers)) = ALLOW_LIST
|
||||
.iter()
|
||||
.find(|(p, _)| path_str.ends_with(p))
|
||||
{
|
||||
// If the allowlisted file performs mutations, enforce presence of epoch markers.
|
||||
if !markers.is_empty()
|
||||
&& markers.iter().all(|m| !contents.contains(m))
|
||||
{
|
||||
offenders.push(format!(
|
||||
"{} (allowlisted but missing epoch marker(s): {:?})",
|
||||
path_str, markers
|
||||
));
|
||||
}
|
||||
continue;
|
||||
}
|
||||
offenders.push(path_str.to_string());
|
||||
}
|
||||
}
|
||||
|
||||
if !offenders.is_empty() {
|
||||
panic!(
|
||||
"Found new direct git invocations without epoch bumping: {:?}. Use the shared helpers or add an allow-list entry with justification.",
|
||||
offenders
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,89 @@
|
||||
use code_core::review_coord::{bump_snapshot_epoch, current_snapshot_epoch, read_lock_info, try_acquire_lock};
|
||||
use code_git_tooling::{create_ghost_commit, CreateGhostCommitOptions};
|
||||
use std::process::Command;
|
||||
use tempfile::TempDir;
|
||||
|
||||
// Integration-style coverage of lock contention and stale-epoch handling across components.
|
||||
#[test]
|
||||
fn lock_contention_and_epoch_refresh_across_components() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
std::env::set_var("CODE_HOME", dir.path());
|
||||
let cwd = dir.path();
|
||||
|
||||
// Component A acquires the global lock and captures the epoch.
|
||||
let guard_a = try_acquire_lock("component-a", cwd).unwrap().expect("lock available");
|
||||
let info_a = read_lock_info().expect("lock info present");
|
||||
let epoch_a = current_snapshot_epoch();
|
||||
assert_eq!(info_a.snapshot_epoch, epoch_a);
|
||||
|
||||
// Component B should be blocked while A holds the lock.
|
||||
let guard_b_blocked = try_acquire_lock("component-b", cwd).unwrap();
|
||||
assert!(guard_b_blocked.is_none());
|
||||
|
||||
// Simulate a git mutation while A holds the lock; epoch advances.
|
||||
bump_snapshot_epoch();
|
||||
let epoch_after = current_snapshot_epoch();
|
||||
assert!(epoch_after > epoch_a);
|
||||
|
||||
// Release A, then acquire with B; B should see the newer epoch.
|
||||
drop(guard_a);
|
||||
let guard_b = try_acquire_lock("component-b", cwd).unwrap().expect("lock available");
|
||||
let info_b = read_lock_info().expect("lock info present");
|
||||
assert_eq!(info_b.snapshot_epoch, epoch_after);
|
||||
drop(guard_b);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ghost_commit_bumps_epoch_and_stale_resume_is_detectable() {
|
||||
let code_home = TempDir::new().unwrap();
|
||||
std::env::set_var("CODE_HOME", code_home.path());
|
||||
|
||||
let repo_dir = TempDir::new().unwrap();
|
||||
let repo = repo_dir.path();
|
||||
|
||||
// Init repo and base commit
|
||||
assert!(Command::new("git")
|
||||
.current_dir(repo)
|
||||
.args(["init", "--initial-branch=main"])
|
||||
.status()
|
||||
.unwrap()
|
||||
.success());
|
||||
assert!(Command::new("git")
|
||||
.current_dir(repo)
|
||||
.args(["config", "user.email", "[email protected]"])
|
||||
.status()
|
||||
.unwrap()
|
||||
.success());
|
||||
assert!(Command::new("git")
|
||||
.current_dir(repo)
|
||||
.args(["config", "user.name", "Tester"])
|
||||
.status()
|
||||
.unwrap()
|
||||
.success());
|
||||
std::fs::write(repo.join("file.txt"), "hello") .unwrap();
|
||||
assert!(Command::new("git")
|
||||
.current_dir(repo)
|
||||
.args(["add", "file.txt"])
|
||||
.status()
|
||||
.unwrap()
|
||||
.success());
|
||||
assert!(Command::new("git")
|
||||
.current_dir(repo)
|
||||
.args(["commit", "-m", "base"])
|
||||
.status()
|
||||
.unwrap()
|
||||
.success());
|
||||
|
||||
let before = current_snapshot_epoch();
|
||||
let options = CreateGhostCommitOptions::new(repo).post_commit_hook(&|| bump_snapshot_epoch());
|
||||
let _ghost = create_ghost_commit(&options).expect("ghost commit");
|
||||
let after = current_snapshot_epoch();
|
||||
assert!(after > before, "ghost commit should bump epoch");
|
||||
|
||||
// Simulate apply/patch failure + resume bumping the epoch
|
||||
let captured_epoch = after;
|
||||
bump_snapshot_epoch();
|
||||
let resumed_epoch = current_snapshot_epoch();
|
||||
assert!(resumed_epoch > captured_epoch, "resume must see newer epoch");
|
||||
assert_ne!(captured_epoch, resumed_epoch, "stale snapshot should be detectable");
|
||||
}
|
||||
@@ -65,6 +65,7 @@ pub(crate) struct EventProcessorWithHumanOutput {
|
||||
reasoning_started: bool,
|
||||
raw_reasoning_started: bool,
|
||||
last_message_path: Option<PathBuf>,
|
||||
last_turn_diff: Option<String>,
|
||||
|
||||
/// If true, stop after the first TaskComplete event (default exec mode).
|
||||
/// Auto Drive sessions keep running across multiple turns, so they leave
|
||||
@@ -99,6 +100,7 @@ impl EventProcessorWithHumanOutput {
|
||||
reasoning_started: false,
|
||||
raw_reasoning_started: false,
|
||||
last_message_path,
|
||||
last_turn_diff: None,
|
||||
stop_on_task_complete,
|
||||
}
|
||||
} else {
|
||||
@@ -118,6 +120,7 @@ impl EventProcessorWithHumanOutput {
|
||||
reasoning_started: false,
|
||||
raw_reasoning_started: false,
|
||||
last_message_path,
|
||||
last_turn_diff: None,
|
||||
stop_on_task_complete,
|
||||
}
|
||||
}
|
||||
@@ -203,7 +206,8 @@ impl EventProcessor for EventProcessorWithHumanOutput {
|
||||
// does not surface them alongside the human-readable transcript.
|
||||
}
|
||||
EventMsg::TaskStarted => {
|
||||
// Ignore.
|
||||
// Reset per-turn diff cache so we only print new diffs once.
|
||||
self.last_turn_diff = None;
|
||||
}
|
||||
EventMsg::TaskComplete(TaskCompleteEvent { last_agent_message }) => {
|
||||
if let Some(output_file) = self.last_message_path.as_deref() {
|
||||
@@ -525,6 +529,11 @@ impl EventProcessor for EventProcessorWithHumanOutput {
|
||||
}
|
||||
}
|
||||
EventMsg::TurnDiff(TurnDiffEvent { unified_diff }) => {
|
||||
if self.last_turn_diff.as_deref() == Some(&unified_diff) {
|
||||
// Suppress duplicate turn diffs; they are sometimes streamed multiple times.
|
||||
return CodexStatus::Running;
|
||||
}
|
||||
self.last_turn_diff = Some(unified_diff.clone());
|
||||
ts_println!(self, "{}", "turn diff:".style(self.magenta));
|
||||
println!("{unified_diff}");
|
||||
}
|
||||
|
||||
+185
-23
@@ -25,6 +25,7 @@ use code_core::config::Config;
|
||||
use code_core::config::ConfigOverrides;
|
||||
use code_core::git_info::get_git_repo_root;
|
||||
use code_core::git_info::recent_commits;
|
||||
use code_core::review_coord::{bump_snapshot_epoch, current_snapshot_epoch, try_acquire_lock};
|
||||
use code_core::protocol::AskForApproval;
|
||||
use code_core::protocol::Event;
|
||||
use code_core::protocol::EventMsg;
|
||||
@@ -38,8 +39,6 @@ use code_protocol::models::ResponseItem;
|
||||
use code_protocol::protocol::SessionSource;
|
||||
use code_ollama::DEFAULT_OSS_MODEL;
|
||||
use code_protocol::config_types::SandboxMode;
|
||||
use once_cell::sync::Lazy;
|
||||
use tokio::sync::Mutex as AsyncMutex;
|
||||
use event_processor_with_human_output::EventProcessorWithHumanOutput;
|
||||
use event_processor_with_json_output::EventProcessorWithJsonOutput;
|
||||
use event_processor::handle_last_message;
|
||||
@@ -302,6 +301,7 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
let mut review_outputs: Vec<code_core::protocol::ReviewOutputEvent> = Vec::new();
|
||||
let mut final_review_snapshot: Option<code_core::protocol::ReviewSnapshotInfo> = None;
|
||||
let mut review_runs: u32 = 0;
|
||||
let mut last_review_epoch: Option<u64> = None;
|
||||
let max_auto_resolve_attempts: u32 = config.auto_drive.auto_resolve_review_attempts.get();
|
||||
let mut auto_resolve_state: Option<AutoResolveState> = review_request.as_ref().and_then(|req| {
|
||||
if config.tui.review_auto_resolve {
|
||||
@@ -315,6 +315,8 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
None
|
||||
}
|
||||
});
|
||||
let mut auto_resolve_fix_guard: Option<code_core::review_coord::ReviewGuard> = None;
|
||||
let mut auto_resolve_followup_guard: Option<code_core::review_coord::ReviewGuard> = None;
|
||||
// Base snapshot captured at the start of auto-resolve; each review snapshot is parented to this.
|
||||
let mut auto_resolve_base_snapshot: Option<GhostCommit> = None;
|
||||
let stop_on_task_complete = auto_drive_goal.is_none() && auto_resolve_state.is_none();
|
||||
@@ -539,11 +541,30 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
}
|
||||
|
||||
// Send the prompt.
|
||||
let mut _review_guard: Option<code_core::review_coord::ReviewGuard> = None;
|
||||
|
||||
// Clear stale review lock in case a prior process crashed.
|
||||
let _ = code_core::review_coord::clear_stale_lock_if_dead();
|
||||
|
||||
let _initial_prompt_task_id = if let Some(mut review_request) = review_request.clone() {
|
||||
let _review_lock = REVIEW_LOCK.lock().await;
|
||||
// Cross-process review coordination
|
||||
match try_acquire_lock("review", &config.cwd) {
|
||||
Ok(Some(g)) => _review_guard = Some(g),
|
||||
Ok(None) => {
|
||||
eprintln!("Another review is already running; skipping this /review.");
|
||||
return Ok(());
|
||||
}
|
||||
Err(err) => {
|
||||
eprintln!("Warning: could not acquire review lock: {err}");
|
||||
}
|
||||
}
|
||||
|
||||
if auto_resolve_state.is_some() {
|
||||
if auto_resolve_base_snapshot.is_none() {
|
||||
auto_resolve_base_snapshot = capture_auto_resolve_snapshot(&config.cwd, None, "auto-resolve base snapshot");
|
||||
if let Some(state) = auto_resolve_state.as_mut() {
|
||||
state.snapshot_epoch = Some(current_snapshot_epoch());
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(base) = auto_resolve_base_snapshot.as_ref() {
|
||||
@@ -561,6 +582,9 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
}
|
||||
}
|
||||
|
||||
// Capture baseline epoch after any snapshot creation so we don't trip on our own bumps.
|
||||
last_review_epoch = Some(current_snapshot_epoch());
|
||||
|
||||
let event_id = conversation.submit(Op::Review { review_request }).await?;
|
||||
info!("Sent /review with event ID: {event_id}");
|
||||
event_id
|
||||
@@ -587,13 +611,28 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
// Handle review auto-resolve: chain follow-up reviews when enabled.
|
||||
match &event.msg {
|
||||
EventMsg::ExitedReviewMode(event) => {
|
||||
// Any review that just finished should release follow-up locks.
|
||||
auto_resolve_followup_guard = None;
|
||||
// Release the global review lock as soon as the review finishes so follow-up
|
||||
// auto-resolve steps can acquire it.
|
||||
_review_guard = None;
|
||||
review_runs = review_runs.saturating_add(1);
|
||||
if let Some(output) = event.review_output.as_ref() {
|
||||
review_outputs.push(output.clone());
|
||||
}
|
||||
if let Some(snapshot) = event.snapshot.as_ref() {
|
||||
final_review_snapshot = Some(snapshot.clone());
|
||||
}
|
||||
if let Some(output) = event.review_output.as_ref() {
|
||||
review_outputs.push(output.clone());
|
||||
}
|
||||
if let Some(snapshot) = event.snapshot.as_ref() {
|
||||
final_review_snapshot = Some(snapshot.clone());
|
||||
// detect stale snapshot epoch
|
||||
if let Some(start_epoch) = last_review_epoch {
|
||||
let current_epoch = current_snapshot_epoch();
|
||||
if current_epoch != start_epoch {
|
||||
eprintln!("Snapshot epoch changed during review; aborting auto-resolve and requiring restart.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
continue;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Surface review result to the parent CLI via stderr; avoid injecting
|
||||
// synthetic user turns into the /review sub-agent conversation.
|
||||
@@ -669,27 +708,77 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
}
|
||||
EventMsg::TaskComplete(TaskCompleteEvent { last_agent_message }) => {
|
||||
if let Some(state_snapshot) = auto_resolve_state.clone() {
|
||||
let current_epoch = current_snapshot_epoch();
|
||||
match state_snapshot.phase {
|
||||
AutoResolvePhase::PendingFix { review } => {
|
||||
if auto_resolve_fix_guard.is_none() {
|
||||
auto_resolve_fix_guard = try_acquire_lock("auto-resolve-fix", &config.cwd).ok().flatten();
|
||||
}
|
||||
if auto_resolve_fix_guard.is_none() {
|
||||
eprintln!("Auto-resolve: another review is running; skipping fix.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
auto_resolve_fix_guard = None;
|
||||
auto_resolve_followup_guard = None;
|
||||
continue;
|
||||
}
|
||||
if let Some(state) = auto_resolve_state.as_mut() {
|
||||
state.phase = AutoResolvePhase::AwaitingFix {
|
||||
review: review.clone(),
|
||||
};
|
||||
state.snapshot_epoch = Some(current_epoch);
|
||||
}
|
||||
dispatch_auto_fix(&conversation, &review).await?;
|
||||
}
|
||||
AutoResolvePhase::AwaitingFix { .. } => {
|
||||
// Fix phase complete; release fix guard so follow-up can take the review lock
|
||||
auto_resolve_fix_guard = None;
|
||||
if let Some(state) = auto_resolve_state.as_mut() {
|
||||
state.last_fix_message = last_agent_message.clone();
|
||||
state.phase = AutoResolvePhase::WaitingForReview;
|
||||
}
|
||||
if auto_resolve_followup_guard.is_none() {
|
||||
auto_resolve_followup_guard =
|
||||
try_acquire_lock("auto-resolve-followup", &config.cwd).ok().flatten();
|
||||
}
|
||||
if auto_resolve_followup_guard.is_none() {
|
||||
eprintln!("Auto-resolve: another review is running; stopping follow-up review.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
auto_resolve_fix_guard = None;
|
||||
auto_resolve_followup_guard = None;
|
||||
continue;
|
||||
}
|
||||
if let Some(base) = auto_resolve_base_snapshot.as_ref() {
|
||||
if !base_is_ancestor_of_head(&config.cwd, base.id()) {
|
||||
eprintln!("Auto-resolve: base snapshot is no longer an ancestor of HEAD; stopping to avoid stale review.");
|
||||
if !head_is_ancestor_of_base(&config.cwd, base.id()) {
|
||||
eprintln!("Auto-resolve: base snapshot no longer matches current HEAD; stopping to avoid stale review.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
auto_resolve_followup_guard = None;
|
||||
auto_resolve_fix_guard = None;
|
||||
continue;
|
||||
}
|
||||
// stale epoch check
|
||||
if let Some(state) = auto_resolve_state.as_ref() {
|
||||
if let Some(baseline) = state.snapshot_epoch {
|
||||
if current_epoch > baseline {
|
||||
eprintln!("Auto-resolve: snapshot epoch advanced; aborting follow-up review.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
auto_resolve_followup_guard = None;
|
||||
auto_resolve_fix_guard = None;
|
||||
continue;
|
||||
}
|
||||
}
|
||||
if state.metadata.as_ref().and_then(|m| m.commit.as_ref()).is_some()
|
||||
&& state.snapshot_epoch.is_none()
|
||||
{
|
||||
eprintln!("Auto-resolve: snapshot epoch advanced; aborting follow-up review.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
continue;
|
||||
}
|
||||
}
|
||||
match capture_snapshot_against_base(
|
||||
&config.cwd,
|
||||
base,
|
||||
@@ -703,15 +792,16 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
eprintln!("Auto-resolve: follow-up snapshot is identical to last reviewed commit; ending loop to avoid duplicate review.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
auto_resolve_followup_guard = None;
|
||||
auto_resolve_fix_guard = None;
|
||||
continue;
|
||||
}
|
||||
|
||||
if let Some(state) = auto_resolve_state.as_mut() {
|
||||
state.last_reviewed_commit = Some(snap.id().to_string());
|
||||
state.snapshot_epoch = Some(current_snapshot_epoch());
|
||||
}
|
||||
|
||||
let _review_lock = REVIEW_LOCK.lock().await;
|
||||
let _review_lock = REVIEW_LOCK.lock().await;
|
||||
let followup_request = build_followup_review_request(
|
||||
&state_snapshot,
|
||||
&config.cwd,
|
||||
@@ -720,6 +810,7 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
Some(base.id()),
|
||||
)
|
||||
.await;
|
||||
last_review_epoch = Some(current_snapshot_epoch());
|
||||
let _ = conversation
|
||||
.submit(Op::Review {
|
||||
review_request: followup_request,
|
||||
@@ -730,6 +821,8 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
eprintln!("Auto-resolve: failed to capture follow-up snapshot or no diff detected; stopping auto-resolve.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
auto_resolve_followup_guard = None;
|
||||
auto_resolve_fix_guard = None;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -741,12 +834,27 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
state.phase = AutoResolvePhase::WaitingForReview;
|
||||
}
|
||||
if let Some(base) = auto_resolve_base_snapshot.as_ref() {
|
||||
if !base_is_ancestor_of_head(&config.cwd, base.id()) {
|
||||
eprintln!("Auto-resolve: base snapshot is no longer an ancestor of HEAD; stopping to avoid stale review.");
|
||||
if !head_is_ancestor_of_base(&config.cwd, base.id()) {
|
||||
eprintln!("Auto-resolve: base snapshot no longer matches current HEAD; stopping to avoid stale review.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
continue;
|
||||
}
|
||||
let current_epoch = current_snapshot_epoch();
|
||||
if let Some(state) = auto_resolve_state.as_ref() {
|
||||
if state
|
||||
.metadata
|
||||
.as_ref()
|
||||
.and_then(|m| m.commit.as_ref())
|
||||
.is_some()
|
||||
&& current_epoch > state.attempt as u64
|
||||
{
|
||||
eprintln!("Auto-resolve: snapshot epoch advanced; aborting follow-up review.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
continue;
|
||||
}
|
||||
}
|
||||
match capture_snapshot_against_base(
|
||||
&config.cwd,
|
||||
base,
|
||||
@@ -775,6 +883,7 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
Some(base.id()),
|
||||
)
|
||||
.await;
|
||||
last_review_epoch = Some(current_snapshot_epoch());
|
||||
let _ = conversation
|
||||
.submit(Op::Review {
|
||||
review_request: followup_request,
|
||||
@@ -785,6 +894,8 @@ pub async fn run_main(cli: Cli, code_linux_sandbox_exe: Option<PathBuf>) -> anyh
|
||||
eprintln!("Auto-resolve: failed to capture follow-up snapshot or no diff detected; stopping auto-resolve.");
|
||||
auto_resolve_state = None;
|
||||
auto_resolve_base_snapshot = None;
|
||||
auto_resolve_followup_guard = None;
|
||||
auto_resolve_fix_guard = None;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1154,11 +1265,17 @@ fn capture_auto_resolve_snapshot(
|
||||
parent: Option<&str>,
|
||||
message: &'static str,
|
||||
) -> Option<GhostCommit> {
|
||||
let mut options = CreateGhostCommitOptions::new(cwd).message(message);
|
||||
let mut options = CreateGhostCommitOptions::new(cwd)
|
||||
.message(message)
|
||||
.post_commit_hook(&|| bump_snapshot_epoch());
|
||||
if let Some(parent) = parent {
|
||||
options = options.parent(parent);
|
||||
}
|
||||
create_ghost_commit(&options).ok()
|
||||
let snap = create_ghost_commit(&options).ok();
|
||||
if snap.is_some() {
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
snap
|
||||
}
|
||||
|
||||
fn snapshot_parent_diff_paths(cwd: &Path, parent: &str, head: &str) -> Option<Vec<String>> {
|
||||
@@ -1256,6 +1373,7 @@ fn capture_snapshot_against_base(
|
||||
if diff_paths.is_empty() {
|
||||
return None;
|
||||
}
|
||||
bump_snapshot_epoch();
|
||||
Some((snapshot, diff_paths))
|
||||
}
|
||||
|
||||
@@ -1274,6 +1392,30 @@ fn strip_scope_from_prompt(prompt: &str) -> String {
|
||||
filtered.join("\n")
|
||||
}
|
||||
|
||||
/// Remove lines that pin the review to specific commit hashes so follow-up
|
||||
/// reviews can safely re-scope to the newest snapshot.
|
||||
fn strip_commit_mentions(prompt: &str, commits: &[&str]) -> String {
|
||||
prompt
|
||||
.lines()
|
||||
.filter(|line| {
|
||||
let trimmed = line.trim();
|
||||
if trimmed
|
||||
.to_ascii_lowercase()
|
||||
.contains("analyze only changes made in commit")
|
||||
{
|
||||
return false;
|
||||
}
|
||||
for c in commits {
|
||||
if !c.is_empty() && trimmed.contains(c) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
true
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
fn should_skip_followup(last_reviewed_commit: Option<&str>, next_snapshot: &GhostCommit) -> bool {
|
||||
match last_reviewed_commit {
|
||||
Some(prev) => prev == next_snapshot.id(),
|
||||
@@ -1281,12 +1423,17 @@ fn should_skip_followup(last_reviewed_commit: Option<&str>, next_snapshot: &Ghos
|
||||
}
|
||||
}
|
||||
|
||||
static REVIEW_LOCK: Lazy<AsyncMutex<()>> = Lazy::new(|| AsyncMutex::new(()));
|
||||
|
||||
fn base_is_ancestor_of_head(cwd: &Path, base_commit: &str) -> bool {
|
||||
/// Returns true if the current HEAD is an ancestor of `base_commit`.
|
||||
///
|
||||
/// Ghost snapshots are created as children of the then-current HEAD. That means
|
||||
/// HEAD should be an ancestor of the snapshot immediately after creation. If
|
||||
/// HEAD moves later (new commits, rebases, etc.) it may no longer be an
|
||||
/// ancestor, which indicates the snapshot is stale relative to the live branch
|
||||
/// we plan to patch against.
|
||||
fn head_is_ancestor_of_base(cwd: &Path, base_commit: &str) -> bool {
|
||||
let output = std::process::Command::new("git")
|
||||
.current_dir(cwd)
|
||||
.args(["merge-base", "--is-ancestor", base_commit, "HEAD"])
|
||||
.args(["merge-base", "--is-ancestor", "HEAD", base_commit])
|
||||
.output();
|
||||
|
||||
match output {
|
||||
@@ -1323,6 +1470,21 @@ async fn build_followup_review_request(
|
||||
metadata = updated.metadata.unwrap_or_default();
|
||||
}
|
||||
|
||||
// Strip lingering references to earlier commits so follow-up /review scopes to
|
||||
// the freshly captured snapshot instead of the original hash baked into the
|
||||
// user prompt.
|
||||
let mut commit_ids: Vec<&str> = Vec::new();
|
||||
if let Some(last) = state.last_reviewed_commit.as_deref() {
|
||||
commit_ids.push(last);
|
||||
}
|
||||
if let Some(meta_commit) = metadata.commit.as_deref() {
|
||||
commit_ids.push(meta_commit);
|
||||
}
|
||||
if let Some(parent) = parent_commit {
|
||||
commit_ids.push(parent);
|
||||
}
|
||||
prompt = strip_commit_mentions(&prompt, &commit_ids);
|
||||
|
||||
// Ensure commit metadata matches the snapshot we will review.
|
||||
if let Some(snapshot) = snapshot {
|
||||
metadata.commit = Some(snapshot.id().to_string());
|
||||
@@ -1771,7 +1933,7 @@ use tokio::sync::Mutex as AsyncMutex;
|
||||
.output()
|
||||
.unwrap();
|
||||
|
||||
assert!(base_is_ancestor_of_head(temp.path(), &c1));
|
||||
assert!(head_is_ancestor_of_base(temp.path(), &c1));
|
||||
|
||||
// move HEAD back to check false case
|
||||
std::process::Command::new("git")
|
||||
@@ -1779,7 +1941,7 @@ use tokio::sync::Mutex as AsyncMutex;
|
||||
.args(["checkout", "HEAD~1"])
|
||||
.output()
|
||||
.unwrap();
|
||||
assert!(!base_is_ancestor_of_head(temp.path(), "deadbeef"));
|
||||
assert!(!head_is_ancestor_of_base(temp.path(), "deadbeef"));
|
||||
}
|
||||
|
||||
fn test_config(code_home: &Path) -> Config {
|
||||
|
||||
@@ -24,6 +24,7 @@ pub struct CreateGhostCommitOptions<'a> {
|
||||
pub message: Option<&'a str>,
|
||||
pub force_include: Vec<PathBuf>,
|
||||
pub parent: Option<&'a str>,
|
||||
pub post_commit_hook: Option<&'a dyn Fn()>,
|
||||
}
|
||||
|
||||
impl<'a> CreateGhostCommitOptions<'a> {
|
||||
@@ -34,6 +35,7 @@ impl<'a> CreateGhostCommitOptions<'a> {
|
||||
message: None,
|
||||
force_include: Vec::new(),
|
||||
parent: None,
|
||||
post_commit_hook: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -49,6 +51,12 @@ impl<'a> CreateGhostCommitOptions<'a> {
|
||||
self
|
||||
}
|
||||
|
||||
/// Registers a hook to run after the ghost commit is created.
|
||||
pub fn post_commit_hook(mut self, hook: &'a dyn Fn()) -> Self {
|
||||
self.post_commit_hook = Some(hook);
|
||||
self
|
||||
}
|
||||
|
||||
/// Supplies the entire force-include path list at once.
|
||||
pub fn force_include<I>(mut self, paths: I) -> Self
|
||||
where
|
||||
@@ -140,6 +148,10 @@ pub fn create_ghost_commit(
|
||||
Some(commit_env.as_slice()),
|
||||
)?;
|
||||
|
||||
if let Some(hook) = options.post_commit_hook {
|
||||
hook();
|
||||
}
|
||||
|
||||
Ok(GhostCommit::new(commit_id, parent_ref))
|
||||
}
|
||||
|
||||
@@ -376,6 +388,41 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
/// Verifies the optional post-commit hook runs after snapshot creation.
|
||||
fn post_commit_hook_runs_after_creation() -> Result<(), GitToolingError> {
|
||||
use std::sync::atomic::{AtomicUsize, Ordering};
|
||||
static CALLS: AtomicUsize = AtomicUsize::new(0);
|
||||
|
||||
let temp = tempfile::tempdir()?;
|
||||
let repo = temp.path();
|
||||
init_test_repo(repo);
|
||||
|
||||
std::fs::write(repo.join("tracked.txt"), "contents\n")?;
|
||||
run_git_in(repo, &["add", "tracked.txt"]);
|
||||
run_git_in(
|
||||
repo,
|
||||
&[
|
||||
"-c",
|
||||
"user.name=Tester",
|
||||
"-c",
|
||||
"[email protected]",
|
||||
"commit",
|
||||
"-m",
|
||||
"initial",
|
||||
],
|
||||
);
|
||||
|
||||
fn hook() {
|
||||
CALLS.fetch_add(1, Ordering::SeqCst);
|
||||
}
|
||||
|
||||
let options = CreateGhostCommitOptions::new(repo).post_commit_hook(&hook);
|
||||
let _ = create_ghost_commit(&options)?;
|
||||
assert_eq!(CALLS.load(Ordering::SeqCst), 1);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
/// Rejects force-included paths that escape the repository.
|
||||
fn create_ghost_commit_rejects_force_include_parent_path() {
|
||||
|
||||
+277
-30
@@ -250,6 +250,7 @@ use code_core::protocol::PatchApplyEndEvent;
|
||||
use code_core::protocol::TaskCompleteEvent;
|
||||
use code_core::protocol::TokenUsage;
|
||||
use code_core::protocol::TurnDiffEvent;
|
||||
use code_core::review_coord::{bump_snapshot_epoch, try_acquire_lock, ReviewGuard};
|
||||
use code_core::ConversationManager;
|
||||
use code_core::codex::compact::COMPACTION_CHECKPOINT_MESSAGE;
|
||||
use crate::bottom_pane::{
|
||||
@@ -715,6 +716,8 @@ async fn run_fast_forward_merge(state: &MergeRepoState) -> Result<(), String> {
|
||||
));
|
||||
}
|
||||
|
||||
bump_snapshot_epoch();
|
||||
|
||||
let worktree_remove = Command::new("git")
|
||||
.current_dir(&state.git_root)
|
||||
.args(["worktree", "remove"])
|
||||
@@ -1312,6 +1315,12 @@ struct BackgroundReviewState {
|
||||
snapshot: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
||||
pub(crate) enum TurnOrigin {
|
||||
User,
|
||||
Developer,
|
||||
}
|
||||
|
||||
pub(crate) struct ChatWidget<'a> {
|
||||
app_event_tx: AppEventSender,
|
||||
code_op_tx: UnboundedSender<Op>,
|
||||
@@ -1351,6 +1360,12 @@ pub(crate) struct ChatWidget<'a> {
|
||||
// at once into scrollback so the history contains a single message.
|
||||
// Cache of the last finalized assistant message to suppress immediate duplicates
|
||||
last_assistant_message: Option<String>,
|
||||
// Cache of the last user text we submitted (for context passing to review/resolve agents)
|
||||
last_user_message: Option<String>,
|
||||
// Cache of the last developer/system note we injected (hidden messages)
|
||||
last_developer_message: Option<String>,
|
||||
pending_turn_origin: Option<TurnOrigin>,
|
||||
current_turn_origin: Option<TurnOrigin>,
|
||||
// Tracks whether lingering running exec/tool cells have been cleared for the
|
||||
// current turn. Reset on TaskStarted; set after the first assistant message
|
||||
// (delta or final) arrives, which is more reliable than TaskComplete.
|
||||
@@ -1397,6 +1412,9 @@ pub(crate) struct ChatWidget<'a> {
|
||||
auto_resolve_attempts_baseline: u32,
|
||||
turn_had_code_edits: bool,
|
||||
background_review: Option<BackgroundReviewState>,
|
||||
auto_review_baseline: Option<GhostCommit>,
|
||||
review_guard: Option<ReviewGuard>,
|
||||
background_review_guard: Option<ReviewGuard>,
|
||||
processed_auto_review_agents: HashSet<String>,
|
||||
// New: coordinator-provided hints for the next Auto turn
|
||||
pending_turn_descriptor: Option<TurnDescriptor>,
|
||||
@@ -2170,6 +2188,8 @@ impl From<OrderKey> for OrderKeySnapshot {
|
||||
|
||||
// Global guard to prevent overlapping background screenshot captures and to rate-limit them
|
||||
static BG_SHOT_IN_FLIGHT: Lazy<AtomicBool> = Lazy::new(|| AtomicBool::new(false));
|
||||
static BACKGROUND_REVIEW_LOCKS: Lazy<Mutex<HashMap<String, code_core::review_coord::ReviewGuard>>> =
|
||||
Lazy::new(|| Mutex::new(HashMap::new()));
|
||||
static BG_SHOT_LAST_START_MS: Lazy<AtomicU64> = Lazy::new(|| AtomicU64::new(0));
|
||||
static MERGE_LOCKS: Lazy<Mutex<HashMap<PathBuf, Arc<tokio::sync::Mutex<()>>>>> =
|
||||
Lazy::new(|| Mutex::new(HashMap::new()));
|
||||
@@ -5299,6 +5319,10 @@ impl ChatWidget<'_> {
|
||||
rate_limit_secondary_next_reset_at: None,
|
||||
content_buffer: String::new(),
|
||||
last_assistant_message: None,
|
||||
last_user_message: None,
|
||||
last_developer_message: None,
|
||||
pending_turn_origin: None,
|
||||
current_turn_origin: None,
|
||||
cleared_lingering_execs_this_turn: true,
|
||||
exec: ExecState {
|
||||
running_commands: HashMap::new(),
|
||||
@@ -5346,6 +5370,9 @@ impl ChatWidget<'_> {
|
||||
auto_resolve_attempts_baseline: config.auto_drive.auto_resolve_review_attempts.get(),
|
||||
turn_had_code_edits: false,
|
||||
background_review: None,
|
||||
auto_review_baseline: None,
|
||||
review_guard: None,
|
||||
background_review_guard: None,
|
||||
processed_auto_review_agents: HashSet::new(),
|
||||
pending_turn_descriptor: None,
|
||||
pending_auto_turn_config: None,
|
||||
@@ -5618,6 +5645,10 @@ impl ChatWidget<'_> {
|
||||
rate_limit_secondary_next_reset_at: None,
|
||||
content_buffer: String::new(),
|
||||
last_assistant_message: None,
|
||||
last_user_message: None,
|
||||
last_developer_message: None,
|
||||
pending_turn_origin: None,
|
||||
current_turn_origin: None,
|
||||
cleared_lingering_execs_this_turn: true,
|
||||
exec: ExecState {
|
||||
running_commands: HashMap::new(),
|
||||
@@ -5681,6 +5712,9 @@ impl ChatWidget<'_> {
|
||||
auto_resolve_attempts_baseline: config.auto_drive.auto_resolve_review_attempts.get(),
|
||||
turn_had_code_edits: false,
|
||||
background_review: None,
|
||||
auto_review_baseline: None,
|
||||
review_guard: None,
|
||||
background_review_guard: None,
|
||||
processed_auto_review_agents: HashSet::new(),
|
||||
pending_turn_descriptor: None,
|
||||
pending_auto_turn_config: None,
|
||||
@@ -6674,6 +6708,8 @@ impl ChatWidget<'_> {
|
||||
|
||||
match input_result {
|
||||
InputResult::Submitted(text) => {
|
||||
self.pending_turn_origin = Some(TurnOrigin::User);
|
||||
self.last_user_message = Some(text.clone());
|
||||
if self.auto_state.should_show_goal_entry() {
|
||||
let trimmed = text.trim();
|
||||
if trimmed.is_empty() {
|
||||
@@ -10007,7 +10043,8 @@ impl ChatWidget<'_> {
|
||||
);
|
||||
let repo_path = self.config.cwd.clone();
|
||||
let started_at = request.started_at;
|
||||
let result = create_ghost_commit(&CreateGhostCommitOptions::new(repo_path.as_path()));
|
||||
let result = create_ghost_commit(&CreateGhostCommitOptions::new(repo_path.as_path())
|
||||
.post_commit_hook(&|| bump_snapshot_epoch()));
|
||||
let elapsed = started_at.elapsed();
|
||||
let snapshot = self.finalize_ghost_snapshot(request, result, elapsed);
|
||||
snapshot
|
||||
@@ -10192,7 +10229,7 @@ impl ChatWidget<'_> {
|
||||
tokio::spawn(async move {
|
||||
let handle = tokio::task::spawn_blocking(move || {
|
||||
let options = CreateGhostCommitOptions::new(repo_path.as_path());
|
||||
create_ghost_commit(&options)
|
||||
create_ghost_commit(&options.post_commit_hook(&|| bump_snapshot_epoch()))
|
||||
});
|
||||
tokio::pin!(handle);
|
||||
|
||||
@@ -10883,6 +10920,12 @@ impl ChatWidget<'_> {
|
||||
Err(msg.to_string())
|
||||
}
|
||||
} else {
|
||||
if args_vec
|
||||
.iter()
|
||||
.any(|arg| matches!(arg.as_str(), "pull" | "checkout" | "merge" | "apply"))
|
||||
{
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
parser(String::from_utf8_lossy(&output.stdout).to_string())
|
||||
}
|
||||
}
|
||||
@@ -11701,6 +11744,7 @@ impl ChatWidget<'_> {
|
||||
// ToolEnd to avoid the earlier regression where we finalized
|
||||
// after every tool call.
|
||||
self.turn_had_code_edits = false;
|
||||
self.current_turn_origin = self.pending_turn_origin.take();
|
||||
self.cleared_lingering_execs_this_turn = false;
|
||||
self.ensure_lingering_execs_cleared();
|
||||
|
||||
@@ -11727,6 +11771,16 @@ impl ChatWidget<'_> {
|
||||
.update_status_text("waiting for model".to_string());
|
||||
tracing::info!("[order] EventMsg::TaskStarted id={}", id);
|
||||
|
||||
// Capture a baseline snapshot for this turn so background auto review only
|
||||
// covers changes made during the turn, not pre-existing local edits.
|
||||
self.auto_review_baseline = None;
|
||||
if self.config.tui.auto_review_enabled {
|
||||
match self.capture_auto_turn_commit("auto review baseline snapshot", None) {
|
||||
Ok(commit) => self.auto_review_baseline = Some(commit),
|
||||
Err(err) => tracing::warn!("failed to capture auto review baseline: {err}"),
|
||||
}
|
||||
}
|
||||
|
||||
// Don't add loading cell - we have progress in the input area
|
||||
// self.add_to_history(history_cell::new_loading_cell("waiting for model".to_string()));
|
||||
|
||||
@@ -13076,6 +13130,7 @@ impl ChatWidget<'_> {
|
||||
if self.auto_resolve_enabled() {
|
||||
self.auto_resolve_handle_review_exit(review_event.review_output.clone());
|
||||
}
|
||||
self.review_guard = None;
|
||||
let hint = self.active_review_hint.take();
|
||||
let prompt = self.active_review_prompt.take();
|
||||
match review_event.review_output {
|
||||
@@ -16522,11 +16577,17 @@ Have we met every part of this goal and is there no further work to do?"#
|
||||
let parent_id = parent.map(|commit| commit.id().to_string());
|
||||
return stub(message, parent_id);
|
||||
}
|
||||
let mut options = CreateGhostCommitOptions::new(self.config.cwd.as_path()).message(message);
|
||||
let mut options = CreateGhostCommitOptions::new(self.config.cwd.as_path())
|
||||
.message(message)
|
||||
.post_commit_hook(&|| bump_snapshot_epoch());
|
||||
if let Some(parent_commit) = parent {
|
||||
options = options.parent(parent_commit.id());
|
||||
}
|
||||
create_ghost_commit(&options)
|
||||
let result = create_ghost_commit(&options.post_commit_hook(&|| bump_snapshot_epoch()));
|
||||
if result.is_ok() {
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
result
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-helpers"))]
|
||||
@@ -24803,6 +24864,8 @@ Have we met every part of this goal and is there no further work to do?"#
|
||||
use code_core::protocol::InputItem;
|
||||
|
||||
let mut ordered = Vec::new();
|
||||
let preface_cache = preface.clone();
|
||||
let agent_cache = agent_text.clone();
|
||||
if !preface.trim().is_empty() {
|
||||
ordered.push(InputItem::Text { text: preface });
|
||||
}
|
||||
@@ -24819,6 +24882,20 @@ Have we met every part of this goal and is there no further work to do?"#
|
||||
ordered_items: ordered,
|
||||
suppress_persistence: false,
|
||||
};
|
||||
let mut cache = String::new();
|
||||
if !preface_cache.trim().is_empty() {
|
||||
cache.push_str(preface_cache.trim());
|
||||
}
|
||||
if !agent_cache.trim().is_empty() {
|
||||
if !cache.is_empty() {
|
||||
cache.push('\n');
|
||||
}
|
||||
cache.push_str(agent_cache.trim());
|
||||
}
|
||||
if !cache.is_empty() {
|
||||
self.last_developer_message = Some(cache);
|
||||
}
|
||||
self.pending_turn_origin = Some(TurnOrigin::Developer);
|
||||
self.submit_user_message(msg);
|
||||
}
|
||||
|
||||
@@ -25470,18 +25547,64 @@ Have we met every part of this goal and is there no further work to do?"#
|
||||
}
|
||||
}
|
||||
|
||||
async fn run_background_review(config: Config, app_event_tx: AppEventSender) {
|
||||
let outcome = async {
|
||||
async fn run_background_review(
|
||||
config: Config,
|
||||
app_event_tx: AppEventSender,
|
||||
base_snapshot: Option<GhostCommit>,
|
||||
turn_context: Option<String>,
|
||||
) {
|
||||
// Best-effort: clean up any stale lock left by a cancelled review process.
|
||||
let _ = code_core::review_coord::clear_stale_lock_if_dead();
|
||||
|
||||
// If another review is in-flight, skip quietly and report a non-fatal status.
|
||||
let review_guard = match try_acquire_lock("background-review", &config.cwd) {
|
||||
Ok(Some(g)) => g,
|
||||
Ok(None) => {
|
||||
app_event_tx.send(AppEvent::BackgroundReviewFinished {
|
||||
worktree_path: std::path::PathBuf::new(),
|
||||
branch: String::new(),
|
||||
has_findings: false,
|
||||
findings: 0,
|
||||
summary: Some("Auto review skipped: another review is already running.".to_string()),
|
||||
error: None,
|
||||
agent_id: None,
|
||||
snapshot: None,
|
||||
});
|
||||
return;
|
||||
}
|
||||
Err(err) => {
|
||||
app_event_tx.send(AppEvent::BackgroundReviewFinished {
|
||||
worktree_path: std::path::PathBuf::new(),
|
||||
branch: String::new(),
|
||||
has_findings: false,
|
||||
findings: 0,
|
||||
summary: None,
|
||||
error: Some(format!("could not acquire review lock: {err}")),
|
||||
agent_id: None,
|
||||
snapshot: None,
|
||||
});
|
||||
return;
|
||||
}
|
||||
};
|
||||
|
||||
let app_event_tx_clone = app_event_tx.clone();
|
||||
let outcome = async move {
|
||||
let guard = review_guard;
|
||||
let git_root = code_core::git_worktree::get_git_root_from(&config.cwd)
|
||||
.await
|
||||
.map_err(|e| format!("failed to detect git root: {e}"))?;
|
||||
|
||||
let snapshot = task::spawn_blocking({
|
||||
let repo_path = config.cwd.clone();
|
||||
let base_snapshot = base_snapshot.clone();
|
||||
move || {
|
||||
let options = CreateGhostCommitOptions::new(repo_path.as_path())
|
||||
.message("auto review snapshot");
|
||||
create_ghost_commit(&options)
|
||||
let mut options = CreateGhostCommitOptions::new(repo_path.as_path())
|
||||
.message("auto review snapshot")
|
||||
.post_commit_hook(&|| bump_snapshot_epoch());
|
||||
if let Some(base) = base_snapshot.as_ref() {
|
||||
options = options.parent(base.id());
|
||||
}
|
||||
create_ghost_commit(&options.post_commit_hook(&|| bump_snapshot_epoch()))
|
||||
}
|
||||
})
|
||||
.await
|
||||
@@ -25489,14 +25612,18 @@ async fn run_background_review(config: Config, app_event_tx: AppEventSender) {
|
||||
.and_then(|res| res.map_err(|e| format!("failed to capture snapshot: {e}")))?;
|
||||
|
||||
let snapshot_id = snapshot.id().to_string();
|
||||
let branch_name = format!("auto-review-{}", Utc::now().format("%Y%m%d-%H%M%S"));
|
||||
let (worktree_path, branch) = code_core::git_worktree::setup_worktree(
|
||||
bump_snapshot_epoch();
|
||||
// Reuse a single detached worktree for auto-review to avoid churn; keep
|
||||
// gitignored build outputs for faster incremental builds.
|
||||
let worktree_path = code_core::git_worktree::prepare_reusable_worktree(
|
||||
&git_root,
|
||||
&branch_name,
|
||||
Some(snapshot_id.as_str()),
|
||||
"auto-review-reuse",
|
||||
snapshot_id.as_str(),
|
||||
true,
|
||||
)
|
||||
.await
|
||||
.map_err(|e| format!("failed to create worktree: {e}"))?;
|
||||
.map_err(|e| format!("failed to prepare worktree: {e}"))?;
|
||||
let branch = "auto-review-reuse".to_string();
|
||||
|
||||
// Ensure Codex models are invoked via the `code-` CLI shim so they exist on PATH.
|
||||
fn ensure_code_prefix(model: &str) -> String {
|
||||
@@ -25511,10 +25638,15 @@ async fn run_background_review(config: Config, app_event_tx: AppEventSender) {
|
||||
let review_model = ensure_code_prefix(&config.review_model);
|
||||
|
||||
// Use the /review entrypoint so upstream wiring (model defaults, review formatting) stays intact.
|
||||
let review_prompt = format!(
|
||||
let mut review_prompt = format!(
|
||||
"/review Analyze only changes made in commit {snapshot_id}. Identify critical bugs, regressions, security/performance/concurrency risks or incorrect assumptions. Provide actionable feedback and references to the changed code; ignore minor style or formatting nits."
|
||||
);
|
||||
|
||||
if let Some(context) = turn_context {
|
||||
review_prompt.push_str("\n\n");
|
||||
review_prompt.push_str(&context);
|
||||
}
|
||||
|
||||
let mut manager = code_core::AGENT_MANAGER.write().await;
|
||||
let agent_id = manager
|
||||
.create_agent_with_options(
|
||||
@@ -25535,13 +25667,15 @@ async fn run_background_review(config: Config, app_event_tx: AppEventSender) {
|
||||
.await;
|
||||
drop(manager);
|
||||
|
||||
app_event_tx.send(AppEvent::BackgroundReviewStarted {
|
||||
app_event_tx_clone.send(AppEvent::BackgroundReviewStarted {
|
||||
worktree_path: worktree_path.clone(),
|
||||
branch: branch.clone(),
|
||||
agent_id: Some(agent_id.clone()),
|
||||
snapshot: Some(snapshot_id.clone()),
|
||||
});
|
||||
|
||||
// Keep the lock alive for the duration of the agent by parking it in a
|
||||
// static map keyed by agent id; cleared on BackgroundReviewFinished.
|
||||
insert_background_lock(&agent_id, guard);
|
||||
Ok((worktree_path, branch, agent_id, snapshot_id))
|
||||
}
|
||||
.await;
|
||||
@@ -25560,6 +25694,20 @@ async fn run_background_review(config: Config, app_event_tx: AppEventSender) {
|
||||
}
|
||||
}
|
||||
|
||||
fn insert_background_lock(agent_id: &str, guard: code_core::review_coord::ReviewGuard) {
|
||||
if let Ok(mut map) = BACKGROUND_REVIEW_LOCKS.lock() {
|
||||
map.insert(agent_id.to_string(), guard);
|
||||
}
|
||||
}
|
||||
|
||||
fn release_background_lock(agent_id: &Option<String>) {
|
||||
if let Some(id) = agent_id {
|
||||
if let Ok(mut map) = BACKGROUND_REVIEW_LOCKS.lock() {
|
||||
map.remove(id);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
static AUTO_REVIEW_STUB: once_cell::sync::Lazy<std::sync::Mutex<Option<Box<dyn FnMut() + Send>>>> =
|
||||
once_cell::sync::Lazy::new(|| std::sync::Mutex::new(None));
|
||||
@@ -25623,8 +25771,9 @@ impl Drop for AutoReviewStubGuard {
|
||||
TextEmphasis,
|
||||
TextTone,
|
||||
};
|
||||
use code_core::parse_command::ParsedCommand;
|
||||
use code_core::protocol::OrderMeta;
|
||||
use code_core::parse_command::ParsedCommand;
|
||||
use code_core::protocol::OrderMeta;
|
||||
use code_core::review_coord::{bump_snapshot_epoch, current_snapshot_epoch, try_acquire_lock, ReviewGuard};
|
||||
use code_core::protocol::{
|
||||
AskForApproval,
|
||||
AgentMessageEvent,
|
||||
@@ -28517,6 +28666,40 @@ impl ChatWidget<'_> {
|
||||
}
|
||||
}
|
||||
|
||||
fn turn_context_block(&self) -> Option<String> {
|
||||
let mut lines: Vec<String> = Vec::new();
|
||||
let mut any = false;
|
||||
lines.push("<context>".to_string());
|
||||
lines.push("Below are the most recent messages related to this code change.".to_string());
|
||||
if let Some(user) = self.last_user_message.as_ref().filter(|s| !s.trim().is_empty()) {
|
||||
any = true;
|
||||
lines.push(format!("<user>{}</user>", user.trim()));
|
||||
}
|
||||
if let Some(dev) = self
|
||||
.last_developer_message
|
||||
.as_ref()
|
||||
.filter(|s| !s.trim().is_empty())
|
||||
{
|
||||
any = true;
|
||||
lines.push(format!("<developer>{}</developer>", dev.trim()));
|
||||
}
|
||||
if let Some(assistant) = self
|
||||
.last_assistant_message
|
||||
.as_ref()
|
||||
.filter(|s| !s.trim().is_empty())
|
||||
{
|
||||
any = true;
|
||||
lines.push(format!("<assistant>{}</assistant>", assistant.trim()));
|
||||
}
|
||||
lines.push("</context>".to_string());
|
||||
|
||||
if any {
|
||||
Some(lines.join("\n"))
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
fn auto_resolve_should_block_auto_resume(&self) -> bool {
|
||||
match self.auto_resolve_state.as_ref().map(|state| &state.phase) {
|
||||
Some(AutoResolvePhase::PendingFix { .. })
|
||||
@@ -28674,6 +28857,18 @@ impl ChatWidget<'_> {
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Pass the full structured output so the resolving agent sees file paths and line ranges.
|
||||
if let Ok(raw_json) = serde_json::to_string_pretty(review) {
|
||||
preface.push_str("\n\nFull review JSON (includes file paths and line ranges):\n");
|
||||
preface.push_str(&raw_json);
|
||||
}
|
||||
|
||||
if let Some(context) = self.turn_context_block() {
|
||||
preface.push_str("\n\n");
|
||||
preface.push_str(&context);
|
||||
}
|
||||
|
||||
self.auto_resolve_notice("Auto-resolve: asking the agent to verify and address the review findings.");
|
||||
self.submit_hidden_text_message_with_preface(
|
||||
"Is this a real issue introduced by our changes? If so, please fix and resolve all similar issues.".to_string(),
|
||||
@@ -28712,6 +28907,11 @@ impl ChatWidget<'_> {
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(context) = self.turn_context_block() {
|
||||
preface.push_str("\n\n");
|
||||
preface.push_str(&context);
|
||||
}
|
||||
|
||||
self.auto_resolve_notice("Auto-resolve: requesting status JSON from the agent.");
|
||||
self.submit_hidden_text_message_with_preface("Auto-resolve status check".to_string(), preface);
|
||||
}
|
||||
@@ -28725,6 +28925,10 @@ impl ChatWidget<'_> {
|
||||
preface.push_str("\n\nOutstanding findings:\n");
|
||||
preface.push_str(&summary);
|
||||
}
|
||||
if let Some(context) = self.turn_context_block() {
|
||||
preface.push_str("\n\n");
|
||||
preface.push_str(&context);
|
||||
}
|
||||
self.auto_resolve_notice("Auto-resolve: asking the agent to continue working on the findings.");
|
||||
self.submit_hidden_text_message_with_preface("Please continue".to_string(), preface);
|
||||
}
|
||||
@@ -29197,14 +29401,24 @@ impl ChatWidget<'_> {
|
||||
if !self.turn_had_code_edits {
|
||||
return;
|
||||
}
|
||||
if matches!(self.current_turn_origin, Some(TurnOrigin::Developer)) {
|
||||
return;
|
||||
}
|
||||
if self.is_review_flow_active() {
|
||||
return;
|
||||
}
|
||||
|
||||
self.launch_background_review();
|
||||
if self.auto_review_baseline.is_none() {
|
||||
if let Ok(commit) = self.capture_auto_turn_commit("auto review baseline snapshot", None) {
|
||||
self.auto_review_baseline = Some(commit);
|
||||
}
|
||||
}
|
||||
|
||||
let baseline = self.auto_review_baseline.take();
|
||||
self.launch_background_review(baseline);
|
||||
}
|
||||
|
||||
fn launch_background_review(&mut self) {
|
||||
fn launch_background_review(&mut self, base_snapshot: Option<GhostCommit>) {
|
||||
// Record state immediately to avoid duplicate launches when multiple
|
||||
// TaskComplete events fire in quick succession.
|
||||
self.turn_had_code_edits = false;
|
||||
@@ -29223,8 +29437,10 @@ impl ChatWidget<'_> {
|
||||
|
||||
let config = self.config.clone();
|
||||
let app_event_tx = self.app_event_tx.clone();
|
||||
let base_snapshot_for_task = base_snapshot.clone();
|
||||
let turn_context = self.turn_context_block();
|
||||
tokio::spawn(async move {
|
||||
run_background_review(config, app_event_tx).await;
|
||||
run_background_review(config, app_event_tx, base_snapshot_for_task, turn_context).await;
|
||||
});
|
||||
}
|
||||
|
||||
@@ -29439,6 +29655,10 @@ impl ChatWidget<'_> {
|
||||
agent_id: Option<String>,
|
||||
snapshot: Option<String>,
|
||||
) {
|
||||
// Clear flags up front so subsequent auto reviews can start even if this finishes with an error
|
||||
self.background_review = None;
|
||||
self.background_review_guard = None;
|
||||
release_background_lock(&agent_id);
|
||||
let mut developer_note: Option<String> = None;
|
||||
let snapshot_note = snapshot
|
||||
.as_ref()
|
||||
@@ -29486,11 +29706,22 @@ impl ChatWidget<'_> {
|
||||
});
|
||||
base
|
||||
} else {
|
||||
format!(
|
||||
"Auto review in '{}' completed with no blocking issues ({}).",
|
||||
branch,
|
||||
worktree_path.display()
|
||||
)
|
||||
if let Some(text) = summary.as_ref().and_then(|s| {
|
||||
let t = s.trim();
|
||||
if t.is_empty() {
|
||||
None
|
||||
} else {
|
||||
Some(t.to_string())
|
||||
}
|
||||
}) {
|
||||
text
|
||||
} else {
|
||||
format!(
|
||||
"Auto review in '{}' completed with no blocking issues ({}).",
|
||||
branch,
|
||||
worktree_path.display()
|
||||
)
|
||||
}
|
||||
};
|
||||
|
||||
self.push_background_tail(message.clone());
|
||||
@@ -29591,6 +29822,8 @@ impl ChatWidget<'_> {
|
||||
self.config.auto_drive.auto_resolve_review_attempts = limit;
|
||||
}
|
||||
}
|
||||
|
||||
self.background_review = None;
|
||||
}
|
||||
|
||||
fn update_review_settings_model_row(&mut self) {
|
||||
@@ -30013,8 +30246,18 @@ impl ChatWidget<'_> {
|
||||
user_facing_hint: hint,
|
||||
metadata,
|
||||
};
|
||||
|
||||
self.submit_op(Op::Review { review_request });
|
||||
match try_acquire_lock("review", &self.config.cwd) {
|
||||
Ok(Some(guard)) => {
|
||||
self.review_guard = Some(guard);
|
||||
self.submit_op(Op::Review { review_request });
|
||||
}
|
||||
Ok(None) => {
|
||||
self.push_background_tail("Review skipped: another review is already running.".to_string());
|
||||
}
|
||||
Err(err) => {
|
||||
self.push_background_tail(format!("Review skipped: could not acquire review lock ({err})"));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn is_review_flow_active(&self) -> bool {
|
||||
@@ -32204,6 +32447,10 @@ impl ChatWidget<'_> {
|
||||
self.layout.last_history_viewport_height.set(viewport_height);
|
||||
self.layout.last_max_scroll.set(max_scroll);
|
||||
|
||||
// scroll_offset is bottom‑anchored; Paragraph expects top‑anchored scroll.
|
||||
let clamped_offset = self.layout.scroll_offset.min(max_scroll);
|
||||
let scroll_from_top = max_scroll.saturating_sub(clamped_offset);
|
||||
|
||||
let detail_has_focus = self.agents_terminal.focus() == AgentsTerminalFocus::Detail;
|
||||
let detail_border_color = if detail_has_focus {
|
||||
crate::colors::border_focused()
|
||||
@@ -32218,7 +32465,7 @@ impl ChatWidget<'_> {
|
||||
Paragraph::new(lines)
|
||||
.block(history_block)
|
||||
.wrap(Wrap { trim: false })
|
||||
.scroll((self.layout.scroll_offset.min(max_scroll), 0))
|
||||
.scroll((scroll_from_top, 0))
|
||||
.render(right_area, buf);
|
||||
|
||||
if hint_height == 1 {
|
||||
|
||||
+29
-2
@@ -36,6 +36,7 @@ use code_protocol::config_types::SandboxMode;
|
||||
use std::fs::OpenOptions;
|
||||
use std::io;
|
||||
use std::path::{Path, PathBuf};
|
||||
use code_core::review_coord::{bump_snapshot_epoch, try_acquire_lock};
|
||||
use std::sync::Once;
|
||||
use std::sync::OnceLock;
|
||||
use tracing_appender::non_blocking;
|
||||
@@ -844,10 +845,36 @@ fn reclaim_worktrees_from_file(path: &std::path::Path, label: &str) {
|
||||
eprintln!("Cleaning remaining worktrees for {} ({}).", label, entries.len());
|
||||
for (git_root, worktree) in entries {
|
||||
let Some(wt_str) = worktree.to_str() else { continue };
|
||||
let _ = Command::new("git")
|
||||
|
||||
// Retry a few times if the global review lock is busy.
|
||||
let mut acquired = None;
|
||||
for attempt in 0..3 {
|
||||
acquired = try_acquire_lock("tui-worktree-cleanup", &git_root)
|
||||
.ok()
|
||||
.flatten();
|
||||
if acquired.is_some() {
|
||||
break;
|
||||
}
|
||||
std::thread::sleep(std::time::Duration::from_millis(150 * (attempt + 1)));
|
||||
}
|
||||
|
||||
let Some(_lock) = acquired else {
|
||||
eprintln!(
|
||||
"Deferring cleanup of {} — review lock busy; will retry on next shutdown",
|
||||
worktree.display()
|
||||
);
|
||||
continue;
|
||||
};
|
||||
|
||||
if let Ok(out) = Command::new("git")
|
||||
.current_dir(&git_root)
|
||||
.args(["worktree", "remove", wt_str, "--force"])
|
||||
.output();
|
||||
.output()
|
||||
{
|
||||
if out.status.success() {
|
||||
bump_snapshot_epoch();
|
||||
}
|
||||
}
|
||||
let _ = std::fs::remove_dir_all(&worktree);
|
||||
}
|
||||
let _ = std::fs::remove_file(path);
|
||||
|
||||
Reference in New Issue
Block a user