Skip to content

Commit f0a9e90

Browse files
authored
fix(worktree): reconcile detached-HEAD worktrees, run prune on clean, clamp zero timeout (#6051)
reconcile() previously dropped any worktree whose git worktree list --porcelain block lacked a branch line (detached HEAD), making such entries permanently invisible to list/clean. The porcelain parser now flushes on the worktree <path> line alone and assigns a sentinel branch_name that is provably not a valid git ref, so it can never collide with a real (including foreign, non-zeph) branch (#5936). zeph worktree clean now runs git worktree prune once after removing stale entries, per spec-063 FR-CLEANUP-04 (#5937). DefaultGitRunner now clamps git_timeout_secs=0 to a 1-second floor internally, so the invariant holds regardless of caller; the two ad hoc .max(1) call sites and the bypassable derived Default impl are removed (#5939).
1 parent 5ed8ab4 commit f0a9e90

9 files changed

Lines changed: 561 additions & 37 deletions

File tree

CHANGELOG.md

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,31 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
110110
classification) is now constructed and registered as a tool executor, with its trust-snapshot
111111
`Arc` also threaded onto the `Agent` via `with_trust_snapshot`, in ACP and daemon — previously
112112
`invoke_skill` was effectively CLI-only (#5975).
113+
- `fix(worktree)`: `WorktreeManager::reconcile()` no longer silently drops detached-`HEAD`
114+
worktrees (#5936). `git worktree list --porcelain` emits a `detached` line instead of
115+
`branch refs/heads/<name>` for these worktrees; the porcelain parser previously only flushed a
116+
parsed block when both a `worktree <path>` line and a `branch` line were seen, so detached
117+
entries were discarded and became permanently invisible to `zeph worktree list`/`zeph worktree
118+
clean`. A block is now flushed whenever its `worktree <path>` line is seen, and detached
119+
entries get the new `zeph_worktree::DETACHED_BRANCH_SENTINEL` (`"(detached HEAD)"` — the
120+
embedded space makes it an invalid git ref name, so it can never collide with a real branch,
121+
including one on a worktree foreign to zeph) as their `branch_name`; `WorktreeManager::remove`
122+
skips the `git branch -D` step for this sentinel since there is no real branch to prune, while
123+
the `git worktree remove --force` path is unaffected (it operates on `path`, not
124+
`branch_name`).
125+
- `fix(worktree)`: `zeph worktree clean` now runs `git worktree prune` after removing stale
126+
entries, per spec-063 FR-CLEANUP-04 (#5937). Previously it only issued `git worktree remove
127+
--force` for entries discovered by `reconcile()`, leaving any stale administrative files (e.g.
128+
from a worktree directory deleted outside Zeph) in the git registry. Added
129+
`WorktreeManager::prune()`.
130+
- `fix(worktree)`: `DefaultGitRunner` now clamps `git_timeout_secs = 0` (and any sub-second
131+
timeout) to a 1-second floor inside `new()`/`with_timeout()` itself, per spec-063's NEVER
132+
invariant (#5939). Previously the clamp was duplicated ad hoc at two call sites
133+
(`src/runner.rs`, `src/commands/worktree.rs`) and the crate's own `Default` impl bypassed both,
134+
so `DefaultGitRunner::default()` (and any other future call site) could still construct a
135+
zero-timeout runner where every `git` invocation failed instantly. Both call-site `.max(1)`
136+
duplications were removed now that the crate enforces the invariant internally; the
137+
`git_timeout_secs` doc comment on `WorktreeConfig` was corrected to say so.
113138
- `fix(tools)`: `CompressedExecutor`, `ToolFilter`, and `Arc<ShellExecutor>` now forward the
114139
remaining cross-cutting `ToolExecutor` methods to their inner/wrapped executor instead of
115140
silently falling through to the trait's no-op defaults (#6012). `CompressedExecutor` now

crates/zeph-config/src/worktree.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,9 @@ pub struct WorktreeConfig {
7979
/// Applied to every `git` call issued by the worktree subsystem (e.g.
8080
/// `git worktree add`, `git fetch`, `git rev-parse`). Increase this value
8181
/// on repositories that are slow to clone or when running over high-latency
82-
/// network links. A value of `0` is treated as `1` at the call site.
82+
/// network links. A value of `0` is clamped to `1` second by
83+
/// [`DefaultGitRunner`](https://docs.rs/zeph-worktree/latest/zeph_worktree/git_runner/struct.DefaultGitRunner.html),
84+
/// not by any call site.
8385
pub git_timeout_secs: u64,
8486
}
8587

crates/zeph-worktree/src/git_runner.rs

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,24 +52,39 @@ pub trait GitRunner: Send + Sync {
5252
/// # Ok(())
5353
/// # }
5454
/// ```
55-
#[derive(Debug, Default, Clone)]
55+
#[derive(Debug, Clone)]
5656
pub struct DefaultGitRunner {
5757
timeout: Duration,
5858
}
5959

60+
/// Floor applied to every configured timeout so a `git_timeout_secs = 0`
61+
/// misconfiguration cannot produce an instantly-expiring command timeout
62+
/// (spec-063 NEVER: `git_timeout_secs = 0` is disallowed).
63+
const MIN_TIMEOUT: Duration = Duration::from_secs(1);
64+
6065
impl DefaultGitRunner {
6166
/// Creates a runner with the default 30-second command timeout.
6267
#[must_use]
6368
pub fn new() -> Self {
64-
Self {
65-
timeout: Duration::from_secs(30),
66-
}
69+
Self::with_timeout(Duration::from_secs(30))
6770
}
6871

6972
/// Creates a runner with a custom command timeout.
73+
///
74+
/// `timeout` is clamped to a minimum of one second — a zero (or
75+
/// sub-second) timeout would make every `git` invocation fail
76+
/// immediately, which is never the caller's intent.
7077
#[must_use]
7178
pub fn with_timeout(timeout: Duration) -> Self {
72-
Self { timeout }
79+
Self {
80+
timeout: timeout.max(MIN_TIMEOUT),
81+
}
82+
}
83+
}
84+
85+
impl Default for DefaultGitRunner {
86+
fn default() -> Self {
87+
Self::new()
7388
}
7489
}
7590

@@ -223,3 +238,29 @@ impl GitRunner for FakeGitRunner {
223238
})
224239
}
225240
}
241+
242+
#[cfg(test)]
243+
mod runner_tests {
244+
use super::*;
245+
246+
/// Regression test for #5939: `git_timeout_secs = 0` (surfaced as
247+
/// `Duration::ZERO`) must not produce a runner whose every `git`
248+
/// invocation times out instantly.
249+
#[test]
250+
fn with_timeout_clamps_zero_to_one_second() {
251+
let runner = DefaultGitRunner::with_timeout(Duration::ZERO);
252+
assert_eq!(runner.timeout, Duration::from_secs(1));
253+
}
254+
255+
#[test]
256+
fn with_timeout_preserves_values_above_the_floor() {
257+
let runner = DefaultGitRunner::with_timeout(Duration::from_mins(1));
258+
assert_eq!(runner.timeout, Duration::from_mins(1));
259+
}
260+
261+
#[test]
262+
fn default_and_new_are_never_zero() {
263+
assert_eq!(DefaultGitRunner::default().timeout, Duration::from_secs(30));
264+
assert_eq!(DefaultGitRunner::new().timeout, Duration::from_secs(30));
265+
}
266+
}

crates/zeph-worktree/src/handle.rs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,28 @@
33
44
use std::{path::PathBuf, time::SystemTime};
55

6+
/// Sentinel used for [`WorktreeHandle::branch_name`] when a worktree discovered
7+
/// via [`WorktreeManager::reconcile`][crate::WorktreeManager::reconcile] is on a
8+
/// detached `HEAD` rather than a branch (`git worktree list --porcelain` emits a
9+
/// `detached` line instead of `branch refs/heads/<name>` for these entries).
10+
///
11+
/// The embedded space makes this an invalid git ref name: `git
12+
/// check-ref-format` rejects any ref component containing a space (see
13+
/// `git help check-ref-format` — disallowed characters include space, `~`,
14+
/// `^`, `:`, `?`, `*`, `[`, `\`). `reconcile()` only ever populates
15+
/// `branch_name` from a `branch refs/heads/<name>` porcelain line, and git
16+
/// itself refuses to create a ref containing a space in the first place — so
17+
/// no real branch, including one on a worktree foreign to zeph (i.e. not
18+
/// created via [`WorktreeManager::create`][crate::WorktreeManager::create],
19+
/// which further restricts the subagent-id component to
20+
/// `^[A-Za-z0-9._-]+$`), can ever equal this sentinel. An earlier version of
21+
/// this constant, `"(detached)"`, lacked this property: parentheses are
22+
/// valid in git ref names, so a real branch literally named `(detached)`
23+
/// would have been indistinguishable from a detached-HEAD worktree, causing
24+
/// [`WorktreeManager::remove`][crate::WorktreeManager::remove] to silently
25+
/// skip pruning it (#5936 review finding).
26+
pub const DETACHED_BRANCH_SENTINEL: &str = "(detached HEAD)";
27+
628
/// A live record of a git worktree that [`WorktreeManager`][crate::WorktreeManager]
729
/// has created for a subagent.
830
///
@@ -14,6 +36,10 @@ pub struct WorktreeHandle {
1436
/// Absolute path on disk where the worktree was checked out.
1537
pub path: PathBuf,
1638
/// The git branch name created for this worktree.
39+
///
40+
/// For worktrees discovered by [`WorktreeManager::reconcile`][crate::WorktreeManager::reconcile]
41+
/// that are on a detached `HEAD`, this is [`DETACHED_BRANCH_SENTINEL`] rather
42+
/// than an actual branch name.
1743
pub branch_name: String,
1844
/// The resolved base ref used to create the branch.
1945
///

crates/zeph-worktree/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ pub mod sanitize;
5050

5151
pub use error::WorktreeError;
5252
pub use git_runner::{DefaultGitRunner, GitRunner};
53-
pub use handle::WorktreeHandle;
53+
pub use handle::{DETACHED_BRANCH_SENTINEL, WorktreeHandle};
5454
pub use manager::{WorktreeManager, probe_capabilities};
5555

5656
/// A [`WorktreeManager`] using the production [`DefaultGitRunner`].

0 commit comments

Comments
 (0)