lane-worktrees: review findings deferred from #168/#169 (incl. five --yes data-loss paths found after the two-round cap) #170

Review findings on the #162 sweep PRs (#168 and #169) that were not taken in their review rounds. Each one is a design question, not a defect within the issue's scope, so it is filed here instead of being fixed under the two-round cap (AGENTS.md, Brett Heap's ruling of 2026-09-16).

1. --include-sandboxes takes tmp.* with no proof that a suite made it (#168, Copilot round 1, lane-worktrees sandbox_items)

Every account-owned tmp.* directory older than an hour becomes eligible without evidence that a suite created it or its owner died (pid is zero). A manual mktemp checkout or saved scratch can therefore be deleted without rescue. Require positive suite ownership and dead-owner evidence; report unidentified temporary directories without removing them.

Why it was not taken. #162's own table defines this row as "killed-suite sandboxes (/tmp/tmp.* and pytest-of-$USER/* whose owning pid is gone) with --include-sandboxes → remove (they hold no work)". A mktemp -d directory records no owner pid, so the only evidence there can be is the one the sweep already uses: no live process stands in it, holds a file open there, or names it in its environment, and it is older than sandbox_min_age_minutes. The row is opt-in.

What would settle it. The issue's word on one of these:

• (a) keep the row as specified;
• (b) list tmp.* and never remove it, so only pytest-of-$USER/pytest-* with a dead .lock pid and tests/run.sh's own <UTC>-<pid> run roots are removed;
• (c) remove a tmp.* only when it holds a recognisable suite shape.

PR #169's tests/run.sh roots every temp directory of a run under ${XDG_STATE_HOME}/openRepoTools/tmp/<UTC>-<pid>/, which carries its pid, so (b) costs this repository's own suites nothing from then on.

2. Same-account processes whose /proc entries cannot be read (#168, Copilot round 2, lane-worktrees ProcScan)

The /proc scan marks itself available before reading processes, then silently skips failed cwd and fd reads. A live same-account process with inaccessible /proc entries can therefore appear absent, allowing its tree to be removed. Distinguish processes that exited from unreadable potential writers, and report unknown liveness for the latter.

Why it was not taken. Measured on Eagle on 2026-10-05:

• of 13,241 same-uid entries in /proc, 13,016 had no readable cwd because they had already exited (ENOENT: zombies, which this scan correctly skips);
• 9 were EACCES: 6 timeout and 3 ssh-agent, which are not dumpable and so are unreadable even to their owner.

There is always at least one such process, and a scan cannot tell which tree an unreadable process stands in. Reading every one of them as "liveness unknown" would therefore keep every tree on every sweep. The writers this scan exists for (claude, git, python, a shell) are dumpable, and a timeout wrapper's child, the real command, is readable and is found.

What would settle it. One of:

• (a) leave it as is;
• (b) report the unreadable same-uid pids in the table as a note, without reading them as live;
• (c) treat an unreadable process as a holder only when its comm is in a writer set (claude, node, git, python3, bash, zsh, sh).

3. tests/run.sh: the leader is reaped before the KILL deadline (#169, Copilot round 3, stop_group)

The unbounded wait runs before the ten-second grace loop. If pytest (or the stubbed suite leader) traps or ignores TERM, the wrapper blocks here indefinitely, keeps the workstation lock, and never reaches the promised KILL fallback. Poll the process group first, KILL it after the deadline, and only then reap the leader.

This is a real defect in #169's own stop_group (da7bc59), found in round 3. It is filed rather than fixed under the two-round cap. pytest does not ignore TERM, so the hang needs a suite leader that traps it.

The fix:

• poll group_alive (which already ignores zombies) up to the deadline first;
• KILL the group;
• only then wait "$SUITE_PID".

The case for it: a stub leader that traps TERM, with the wrapper required to exit within about 15 s.

Found after the two-round cap: real defects, filed rather than fixed

Items 4 to 8 are data-loss paths under --yes or --expire --yes. They should be fixed before the sweep's first --yes on real trees; until then the dry run is safe and is all #163's gate reads. Each has a sketch of its fix.

4. A clone's tag-only commits (#168 round 3, tree_guards)

A clone can hold unpublished commits reachable only from a local tag. This guard checks branches and stash...

Fix: include refs/tags/ in the clone's publication check (each tag's commit must be contained in refs/remotes/origin/). Keep the clone when a ref cannot be checked.

5. A submodule's unpublished branches and stash (#168 round 3, tree_guards)

Checking only a submodule's HEAD misses unpublished branches and stashes ... under the parent clone's .git/modules/

Fix: for each submodule, run the clone checks (every refs/heads/ and the stash on a remote), and keep the parent when one fails or cannot be read.

6. A mirror-style remote.origin.fetch with --prune (#168 round 3, _fetch_all)

A mirror-style remote.origin.fetch, such as +refs/heads/*:refs/heads/*, lets this preflight --prune delete local-only branches before classification or rescue.

Fix: fetch with an explicit refspec and refmap: git fetch --prune --refmap= origin '+refs/heads/*:refs/remotes/origin/*', as status does at lines 996-999.

7. Scratch that contains discovered trees (#168 round 3, _scratch)

Checking only the scratch directory's own .git misses discovered worktrees nested beneath it...

Fix: keep a scratch directory while any tree in self.trees (FOREIGN, --live, or otherwise) or any .git lies under it, and let the tree table retire those children first.

8. An origin-less bundle-only rescue is not in the ledger (#168 round 3, expiry_rows)

An origin-less detached worktree can be removed through bundle+remove, but only _push() records rescues in rescues.tsv... --expire --yes can delete the only durable copy.

Fix: record bundle-only rescues in rescues.tsv (origin -, branch bundle:<file>). archive_ledger then reads such a row as "no other copy" and the archive is kept. Add a case combining origin-less removal with expiry.

9. The report drops ignored FILES from evidence detection (#169 round 4)

This filter discards every ignored file and retains only ignored directories. Consequently an ignored junit-results.xml... is never considered.

Fix: keep every ignored entry for the evidence match, and derive a directory-only list for du.

10. The report's .gitignore repair command is unquoted (#169 round 4)

Fix: shlex.quote the .gitignore path in the printed command.

11. The daily report's directory inherits the umask (#169 round 5, lane-start)

The automatic report contains absolute paths, repository/branch names, and lane ownership ... readable by other local accounts.

Fix: mkdir -m 700 the state and reports directories, and start the writer under umask 077.

Taken despite the cap: tests/test_run_wrapper.py's unconditional flock-file assertion (#169 round 4), because it would have failed the macOS landing gate. That is a CI defect, not a review item (30cc63a).

From the Opus adversarial review of 3c0b188

Reproduced with probes against the suite's own Estate fixture. B1-B3 (gate exit 0 for a lane with only FOREIGN trees; non-UTF-8 path and unreadable pytest-of-$USER tracebacks; unescaped porcelain) and #169's B7 (set -m under a tty) were fixed in the PRs. --yes and --expire --yes are now switched off (exit 2 unless LANE_WORKTREES_ENABLE_YES=1) until this issue lands. The rest:

Copilot after the cap, on the adversarial-review heads

A fifth (296e2f0: root/trailer adoption also took standalone clones, contrary to the manual) was a regression of the review fix itself and was fixed in 0b7a5c6.

Refs #162 #168 #169

Lane: openRepoTools-3

🤖 Generated with Claude Code