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
8 changes: 8 additions & 0 deletions src/daemon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4088,6 +4088,14 @@ impl ActorDaemonCoordinator {
return Ok(None);
};

// Repo-creating commands (clone, init) have no meaningful carryover
// state — the target repo doesn't exist before the command runs, and the
// worktree hint may point to the CWD (a non-repo directory) rather than
// the newly created repo.
if matches!(command, "clone" | "init") {
return Ok(None);
}

let repo = discover_repository_in_path_no_git_exec(input.worktree)?;
let stable_heads = stable_carryover_heads_for_command(&repo, &input, &parsed)?;

Expand Down
89 changes: 86 additions & 3 deletions src/daemon/trace_normalizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,23 @@ impl<B: GitBackend> TraceNormalizer<B> {
.and_then(|pending| argv_primary_command(&pending.raw_argv))
.is_some_and(|command| matches!(command.as_str(), "clone" | "init"));

// For clone/init the root process's def_repo carries the newly created
// repo path. Child processes (remote-https, index-pack, rev-list, …)
// inherit the parent CWD and their def_repo reports that CWD — not the
// clone destination. Once we've captured the root def_repo (first
// arrival), skip subsequent child def_repo events entirely to prevent
// overwriting the correct worktree, family, and sid lookup maps.
if prefer_def_repo_target {
let already_saw_def_repo = self
.state
.pending
.get(root_sid)
.is_some_and(|pending| pending.saw_def_repo);
if already_saw_def_repo {
return Ok(None);
}
}

// Trace2 `def_repo.repo` may point at a common-dir `.git` path for worktrees.
// For normal in-repo commands we keep the start/cwd-derived worktree when available.
// For clone/init the `def_repo` target is the repo we actually created and must win.
Expand Down Expand Up @@ -921,10 +938,13 @@ impl<B: GitBackend> TraceNormalizer<B> {
};

let mut candidates = Vec::new();
if let Some(target) = target_from_argv.as_ref() {
// Prefer the def_repo target — it comes from git's own trace2
// event and is always an absolute path. The argv-derived target
// may be relative and resolve against an unrelated ancestor repo.
if let Some(target) = target_from_def_repo.as_ref() {
candidates.push(target.clone());
}
if let Some(target) = target_from_def_repo.as_ref() {
if let Some(target) = target_from_argv.as_ref() {
let duplicate = candidates.iter().any(|existing| existing == target);
if !duplicate {
candidates.push(target.clone());
Comment on lines +944 to 950
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Fallback worktree preference not updated to match swapped candidate ordering

The PR swaps the candidate ordering at lines 931–938 to prefer target_from_def_repo (always absolute) over target_from_argv (potentially relative), with an explicit comment (lines 924–930) explaining that def_repo should be preferred to avoid storing relative paths. However, the !resolved fallback at line 970 still uses target_from_argv.or(target_from_def_repo), which prefers the argv-derived target — directly contradicting the stated intent.

When both candidates fail common_dir_for_repo_path (e.g., the clone just finished but filesystem state is slightly delayed), the fallback stores the potentially-relative argv path into pending.worktree. Downstream, maybe_apply_familyless_clone_side_effect (src/daemon.rs:6403-6435) uses cmd.worktree to call apply_clone_notes_sync_side_effect, which runs git -C <path> — a relative path will fail to resolve from the daemon's unrelated CWD, causing clone notes sync to silently not happen.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Expand Down Expand Up @@ -960,7 +980,7 @@ impl<B: GitBackend> TraceNormalizer<B> {

if !resolved {
// Keep the best available worktree hint even when family resolution fails.
if let Some(target) = target_from_argv.or(target_from_def_repo) {
if let Some(target) = target_from_def_repo.or(target_from_argv) {
pending.worktree = Some(target);
}
if let Some((target, error)) = last_error {
Expand Down Expand Up @@ -2122,6 +2142,69 @@ mod tests {
);
}

#[test]
fn clone_child_def_repo_does_not_overwrite_root_worktree() {
// Real git trace2 output shows child processes (remote-https, index-pack)
// emit def_repo with the CWD as worktree, not the clone destination.
// The root process's def_repo has the correct newly-created repo path.
// Verify that child def_repo events don't clobber the root's worktree.
let backend = Arc::new(MockBackend::default());
let mut normalizer = TraceNormalizer::new(backend);
let temp = tempfile::tempdir().expect("create tempdir");
let cwd = temp.path().join("projects"); // non-repo CWD
let clone_dest = cwd.join("testing-git"); // the clone destination
fs::create_dir_all(clone_dest.join(".git")).expect("create clone git dir");

let root_sid = "20260327T000000.000000Z-Hdeadbeef-P00010000";
let child_sid = format!("{}/20260327T000000.000001Z-Hdeadbeef-P00010001", root_sid);

let start = serde_json::json!({
"event": "start",
"sid": root_sid,
"ts": 1,
"argv": ["git", "clone", "https://github.com/svarlamov/testing-git"]
// No worktree or cwd — matches real trace2 start from non-repo dir
});
// Root def_repo: correct clone destination
let root_def_repo = serde_json::json!({
"event": "def_repo",
"sid": root_sid,
"ts": 2,
"worktree": clone_dest
});
// Child def_repo from remote-https: reports CWD (parent), not destination
let child_def_repo = serde_json::json!({
"event": "def_repo",
"sid": child_sid,
"ts": 3,
"worktree": cwd
});
let exit = serde_json::json!({
"event": "exit",
"sid": root_sid,
"ts": 4,
"code": 0
});

assert!(normalizer.ingest_payload(&start).unwrap().is_none());
assert!(normalizer.ingest_payload(&root_def_repo).unwrap().is_none());
// Child def_repo must NOT overwrite the root worktree
assert!(
normalizer
.ingest_payload(&child_def_repo)
.unwrap()
.is_none()
);

let cmd = normalizer.ingest_payload(&exit).unwrap().unwrap();
assert_eq!(cmd.primary_command.as_deref(), Some("clone"));
assert_eq!(
cmd.worktree.as_ref(),
Some(&clone_dest),
"clone worktree should be the destination, not the parent CWD"
);
}

#[test]
fn no_repo_routes_to_global_scope() {
let backend = Arc::new(MockBackend::default());
Expand Down
160 changes: 160 additions & 0 deletions tests/notes_sync_regression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,166 @@ worktree_test_wrappers! {
}
}

// Regression test: clone with an absolute target path from a non-repo CWD.
// Exercises the side-effect target resolution path where the clone target is
// specified as an absolute path (common in scripted / CI workflows and when
// the user types `git clone URL /some/absolute/path`).
worktree_test_wrappers! {
fn notes_sync_clone_absolute_target_from_non_repo_cwd_fetches_authorship_notes() {
if TestRepo::git_mode() == GitTestMode::Hooks {
return;
}

let (local, upstream) = TestRepo::new_with_remote();

fs::write(local.path().join("abs-clone-seed.txt"), "seed\n")
.expect("failed to write seed file");
local
.git_og(&["add", "abs-clone-seed.txt"])
.expect("add should succeed");
local
.git_og(&["commit", "-m", "seed commit"])
.expect("seed commit should succeed");

let seed_sha = local
.git_og(&["rev-parse", "HEAD"])
.expect("rev-parse should succeed")
.trim()
.to_string();

local
.git_og(&[
"notes",
"--ref=ai",
"add",
"-m",
"abs-clone-note",
seed_sha.as_str(),
])
.expect("adding notes should succeed");
local
.git_og(&["push", "-u", "origin", "HEAD"])
.expect("pushing branch should succeed");
local
.git_og(&["push", "origin", "refs/notes/ai"])
.expect("pushing notes should succeed");

// Clone from a non-repo directory using an absolute target path.
let external_cwd = unique_temp_path("notes-sync-abs-target-cwd");
let _ = fs::remove_dir_all(&external_cwd);
fs::create_dir_all(&external_cwd).expect("failed to create external cwd");

let clone_dir = unique_temp_path("notes-sync-abs-target-clone");
let _ = fs::remove_dir_all(&clone_dir);
let clone_dir_str = clone_dir.to_string_lossy().to_string();
let upstream_str = upstream.path().to_string_lossy().to_string();

local
.git_from_working_dir(
&external_cwd,
&["clone", upstream_str.as_str(), clone_dir_str.as_str()],
)
.expect("clone with absolute target should succeed");

assert!(
clone_dir.exists(),
"clone target should exist at {}",
clone_dir.display()
);

let cloned_note = read_note_from_worktree(&clone_dir, &seed_sha, TestRepo::git_mode());
assert!(
cloned_note.is_some(),
"cloned repository should have fetched authorship notes for commit {} (absolute target from non-repo CWD)",
seed_sha
);
}
}

// Regression test: clone with NO explicit target directory (implicit target
// derived from the source URL/path). This is the common user scenario:
// cd ~/projects && git clone https://github.com/user/repo
// In trace2, the root process emits def_repo with the correct clone destination,
// but child processes (remote-https, index-pack) emit def_repo with the CWD as
// worktree. The normalizer must prefer the root def_repo and ignore children.
worktree_test_wrappers! {
fn notes_sync_clone_implicit_target_from_non_repo_cwd_fetches_authorship_notes() {
if TestRepo::git_mode() == GitTestMode::Hooks {
return;
}

let (local, upstream) = TestRepo::new_with_remote();

fs::write(local.path().join("implicit-seed.txt"), "seed\n")
.expect("failed to write seed file");
local
.git_og(&["add", "implicit-seed.txt"])
.expect("add should succeed");
local
.git_og(&["commit", "-m", "seed commit"])
.expect("seed commit should succeed");

let seed_sha = local
.git_og(&["rev-parse", "HEAD"])
.expect("rev-parse should succeed")
.trim()
.to_string();

local
.git_og(&[
"notes",
"--ref=ai",
"add",
"-m",
"implicit-clone-note",
seed_sha.as_str(),
])
.expect("adding notes should succeed");
local
.git_og(&["push", "-u", "origin", "HEAD"])
.expect("pushing branch should succeed");
local
.git_og(&["push", "origin", "refs/notes/ai"])
.expect("pushing notes should succeed");

// Clone from a non-repo CWD with NO explicit target — git derives the
// directory name from the source path (the upstream bare repo's basename).
let external_cwd = unique_temp_path("notes-sync-clone-implicit-cwd");
let _ = fs::remove_dir_all(&external_cwd);
fs::create_dir_all(&external_cwd).expect("failed to create external cwd");

let upstream_str = upstream.path().to_string_lossy().to_string();
// Derive the expected directory name the same way git does: basename of the source.
let expected_dir_name = Path::new(&upstream_str)
.file_name()
.expect("upstream path should have a filename")
.to_string_lossy()
.to_string();
// Strip .git suffix if present (matches git's behavior)
let expected_dir_name = expected_dir_name
.strip_suffix(".git")
.unwrap_or(&expected_dir_name);

local
.git_from_working_dir(&external_cwd, &["clone", upstream_str.as_str()])
.expect("clone with implicit target should succeed");

let clone_dir = external_cwd.join(expected_dir_name);
assert!(
clone_dir.exists(),
"implicit clone target should exist at {}",
clone_dir.display()
);

let cloned_note = read_note_from_worktree(&clone_dir, &seed_sha, TestRepo::git_mode());
assert!(
cloned_note.is_some(),
"cloned repository should have fetched authorship notes for commit {} (implicit target from non-repo CWD)",
seed_sha
);
}
}

worktree_test_wrappers! {
fn notes_sync_fetch_does_not_import_authorship_notes() {
let mode = TestRepo::git_mode();
Expand Down
Loading