fix(source-control): harden prune_babysit_worktrees.py against orphaned state by kyle-sexton · Pull Request #1331 · melodic-software/claude-code-plugins

kyle-sexton · 2026-07-25T15:25:59Z

This was generated by AI during work-loop execution.

Closes #816

Summary

prune_babysit_worktrees.py had two related robustness gaps, both observed at queue-start prune
(#657, sweep-converted into #816, operator-ratified 2026-07-23):

A worktree directory left behind by a lock-blocked git worktree remove -- git's administrative
record dropped from .git/worktrees/, the directory itself surviving on disk, most commonly a
Windows file lock -- made every subsequent prune run error fatal: not a git repository on that
entry instead of self-healing.
The lock-blocked removal itself gave no signal: the residual directory was left with no report
and no cleanup attempt.

git_status failures now go through is_missing_repo_error, which distinguishes "this path is no
longer a valid git repository" from every other git failure (permission errors, gh network
issues, etc. still surface as real errors). An orphaned entry is self-healed by
drop_orphaned_worktree: it drops the stale worker-lease record for that key (by the time this
runs, main's active-lease check has already established it isn't a live/unexpired hold -- the
lease-only case with no matching directory stays manage_babysit_lease.py reap's job, unchanged)
and removes the residual directory only when it is empty and root-contained
(remove_empty_orphan_directory) -- an orphan's .git pointer could be corrupted or gone while real
uncommitted work still sits there, so a non-empty orphan is reported, never force-deleted. This is
reported via a new orphan_dropped row action, and does not flip the run's exit code, so the
same orphan stops re-erroring every cycle.

remove_worktree now verifies the directory actually left disk after a successful
git worktree remove (attempt_directory_removal -- safe to fully rmtree here, since git already
confirmed the directory was removable) and reports a still-locked directory via residual_directory
in the JSON row plus a stderr WARNING, instead of leaving a silent orphan for the next run to
stumble over (which the orphan_dropped path above then self-heals).

source-control 0.26.2 → 0.26.3 with the matching CHANGELOG entry.

Test plan

Every command below was run from the branch worktree; all passed.

python -m pytest tests/ in plugins/source-control/skills/babysit-prs/scripts/ -- 364 passed,
58 subtests passed (was 358 before this change; 6 new test classes covering the orphan-detection,
directory-removal, and self-heal paths, plus 2 new cases added to the existing
RemoveWorktreeIsHermetic class).
- MissingRepoErrorDetectionTests -- is_missing_repo_error fires on a real git status failure
  against a plain (non-git) directory, and not on an unrelated failure string.
- AttemptDirectoryRemovalTests -- already-gone path reports True; a surviving directory (with
  content) is fully removed; a removal blocked by a patched shutil.rmtree OSError (simulating the
  Windows-lock scenario portably, since CI runs on ubuntu-24.04) reports False without raising.
- RemoveEmptyOrphanDirectoryTests -- removes an empty directory under root; leaves a non-empty
  directory's content untouched; refuses a directory outside root (defense in depth, matching
  remove_worktree's own containment guard).
- DropOrphanedWorktreeTests -- drops the lease record and removes an empty orphan directory; is a
  no-op when no lease record exists; never deletes content from a non-empty orphan directory;
  refuses to remove an orphan directory outside root.
- RemoveWorktreeIsHermetic (extended) -- a clean removal reports residual_directory: False; a
  cleanup that stays blocked (via a patched attempt_directory_removal, since a real
  git worktree remove already clears the directory in this hermetic test environment) reports
  residual_directory: True.
- MainSelfHealsAnOrphanedWorktreeEntry -- end-to-end main() run against an orphaned directory
  plus its stale lease record: exit code 0, orphan_dropped action, both the directory and the
  lease record gone.
python -m py_compile prune_babysit_worktrees.py tests/test_prune_babysit_worktrees.py -- exit 0.

An advisor review (this repo's own reviewer-in-the-loop discipline) caught two real gaps in the
first pass before this PR was opened: (1) an unconditional rmtree on the orphan path would have
destroyed uncommitted work in a directory git never confirmed removable -- the issue text says
empty directory, and I'd drifted from that while implementing; fixed by adding
remove_empty_orphan_directory, gated on emptiness and root-containment, distinct from the
unconditional attempt_directory_removal used only after a git-confirmed removal. (2) whether a
lease-only orphan (no matching directory) needed its own sweep in this script -- confirmed via
grep that manage_babysit_lease.py reap already handles that case independently and already runs
adjacent to this script at queue start (SKILL.md runbook step 3), so no parallel mechanism was
added here.

Sweep-converted from lane sweep: triage — week of 7/20 (rolling paper-cut batch) #657; ratified as work-class: scoped (C3) 2026-07-23.
Version note: open PR fix(babysit-prs): casefold owner/repo/login compares in babysit scripts #1329 (fix(babysit-prs): casefold owner/repo/login compares in babysit scripts) also bumps source-control 0.26.2 → 0.26.3. Whichever of these two merges second
will conflict on the manifest version line and the CHANGELOG insert point -- resolve by rebase,
not force-merge.

🤖 Generated with Claude Code

…ed state A worktree directory left behind by a lock-blocked `git worktree remove` (administrative record dropped, directory surviving -- most commonly a Windows file lock) made every subsequent prune run error `fatal: not a git repository` on that entry instead of self-healing, and the lock-blocked removal itself left the residual directory with no signal. `git_status` failures now distinguish "no longer a valid git repository" from every other failure: an orphaned entry drops its stale worker-lease record and removes the residual directory only when empty and root-contained (never touching an orphan's contents, since git never confirmed it safe to discard), reported via a new `orphan_dropped` row action rather than flipping the run's exit code. `remove_worktree` now verifies the directory actually left disk after a successful removal and reports a still-locked directory via `residual_directory` plus a stderr warning instead of leaving a silent orphan. Closes #816 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

claude · 2026-07-25T15:26:13Z

Claude finished @kyle-sexton's task in 3m 20s —— View job

PR Review

Gather context and understand the request
Read changed files
Review main script changes (prune_babysit_worktrees.py)
Review test coverage (test_prune_babysit_worktrees.py)
Review manifest and changelog
Post findings

Summary

The logic is sound and the defensive programming is well-considered. The two-tier approach — unconditional rmtree after a git-confirmed removal vs. empty-only rmdir for an orphan git never confirmed safe — is the right design. Test coverage is thorough for the core paths. Three issues worth addressing before merge, one more minor note below.

Issues

1. Non-empty orphan silently exits 0 with no stderr signal — medium

prune_babysit_worktrees.py line 335–337

When the orphan directory has content, directory_removed is False in the JSON row but nothing is printed to stderr. The residual_directory case (line 354–360) does emit a WARNING: to stderr; a non-empty orphan is arguably more alarming (uncommitted work sitting in a directory that git no longer manages) but emits nothing. An operator running the script non-verbosely, piping JSON elsewhere, will miss it entirely.

# after row["action"] = "orphan_dropped"
if not row.get("directory_removed"):
    print(
        "WARNING: orphaned worktree directory is non-empty and was not removed "
        f"for {worktree.key}: {worktree.path}",
        file=sys.stderr,
    )

Fix this →

2. lease_path.unlink called while holding state_lock(lease_path) — worth a comment/assertion

prune_babysit_worktrees.py line 237

drop_orphaned_worktree calls lease_path.unlink(missing_ok=True) on the exact file that state_lock(lease_path) holds a lock on. On POSIX, unlinking a locked file is fine (the lock is on the inode). On Windows — the platform this PR specifically targets — if state_lock holds an exclusive file handle on lease_path itself (rather than a separate .lck sidecar), unlink will raise PermissionError, which propagates uncaught and sets exit_code = 1, defeating the self-healing entirely.

Whether this is safe depends on state_lock's implementation. If it uses a separate sidecar lock file, there's no issue. This is worth a brief comment documenting the assumption, so a future reader doesn't need to audit babysit_state.py to understand the safety:

# `state_lock` holds a sidecar lock, not an exclusive handle on lease_path
# itself, so unlinking lease_path while the lock is held is safe on Windows.
lease_path.unlink(missing_ok=True)

3. is_missing_repo_error depends on git's English-locale error message — low

prune_babysit_worktrees.py line 121

return NOT_A_GIT_REPO_MARKER in str(exc).lower()

If git runs under a non-English LC_MESSAGES locale, the error message is translated and this will never match — the orphan is then treated as an unknown error (exit code 1, no self-healing). The subprocess call in run_command doesn't force LANGUAGE=C. This is low-priority since CI and the target Windows environments are almost always English, but it is a latent portability gap. If run_command accepted an env override, passing LANGUAGE=C to the git status call would make this locale-independent.

Minor notes

remove_empty_orphan_directory return value is ambiguous on failure — informational only

The function returns False for both "intentionally skipped (non-empty)" and "removal failed (OSError)". drop_orphaned_worktree maps this to directory_removed: false either way. Callers can't distinguish the two cases without correlating the directory's contents. This is fine for the current use — both outcomes mean "directory still present, don't crash" — but if a future caller needs to distinguish, the API will need extending.

End-to-end coverage for non-empty orphan — informational only

MainSelfHealsAnOrphanedWorktreeEntry only exercises the clean (empty-dir, has-lease) path through main(). The non-empty orphan path is unit-tested via DropOrphanedWorktreeTests but not exercised end-to-end. Not a blocker, just a gap to note.

Everything else looks good

The two-tier deletion strategy (unconditional rmtree post-git-confirm vs. empty-only rmdir for orphan) is the right design. The distinction between attempt_directory_removal and remove_empty_orphan_directory is clearly documented and well-tested.
Symlink traversal is handled correctly: path.resolve() in remove_empty_orphan_directory follows links before the containment check, so a symlink planted inside root pointing outside can't escape the guard.
The TOCTOU in iterdir() → rmdir() is benign: if content appears between the check and the removal, rmdir() raises OSError, which is caught; the directory survives and directory_removed: false is reported.
Exit code 0 for orphan self-healing is the right call — keeping it non-zero would cause the same orphan to keep erroring on every run, which is what the PR is fixing.
CHANGELOG and version bump are consistent with the changes.

claude · 2026-07-25T15:26:22Z

Claude finished @kyle-sexton's task in 3m 40s —— View job

Security Review

Gather PR diff and changed files
Read and analyze prune_babysit_worktrees.py
Read and analyze test_prune_babysit_worktrees.py
Review plugin.json and CHANGELOG.md
Report findings

Findings

IMPORTANT — Orphan self-healing bypasses `--apply` dry-run gate

Confidence: HIGH

The normal worktree removal path is gated on args.apply:

if eligible and args.apply:
    removal_info = remove_worktree(worktree, root)

The orphan self-healing path is not:

row.update(drop_orphaned_worktree(worktree, lease_path, root))   # no --apply guard
row["action"] = "orphan_dropped"

prune_babysit_worktrees.py L325–337

--apply is the user's authorization boundary — it is the only mechanism by which an operator approves state-mutating changes (lease deletion, directory removal). An operator who intentionally runs prune without --apply to audit what would happen, gets silent mutations for any orphaned entry: drop_orphaned_worktree deletes the worker-lease JSON and calls remove_empty_orphan_directory on every such entry, unconditionally.

The upstream lease-check guard does bound the worst case (an active, unexpired lease with a non-matching token skips this path via keep_leased), but:

An expired or absent lease is deleted unconditionally even in dry-run mode.
If is_missing_repo_error produces a false positive (see next finding), a live worktree's lease could be silently deleted during what the caller believes is a dry run.
Automated callers (scheduled queue-start prune without --apply) would silently mutate state.

The CHANGELOG and PR description discuss the exit-code contract but do not explicitly document that orphan self-healing is intentionally --apply-exempt. If this is intentional, it should be documented and the decision cross-checked against how queue-start callers invoke the script (with or without --apply).

SUGGESTION — `is_missing_repo_error` matches the full exception string, including the file path

Confidence: PLAUSIBLE

run_command embeds the full argv in its error message:

raise RuntimeError(f"{' '.join(argv)} failed: {detail}")

So for a git status call the exception string is:

git -C /the/root/path/owner__repo__pr-1 status --short --branch failed: <stderr>

The check:

NOT_A_GIT_REPO_MARKER in str(exc).lower()

prune_babysit_worktrees.py L121

searches the entire string, including the --root value embedded in the command. If the root path contains the substring not a git repository (e.g., a developer's test environment at /home/user/not a git repository test/babysit), every git error for every worktree under that root — permission errors, lock contention, disk failures — would be misclassified as orphan state. The consequence is lease deletion and orphan-drop behavior instead of surfacing a real error (exit code remains 0; real failures are swallowed).

The fix is to match only against the stderr portion of the detail, which run_command already captures separately before building the exception message.

Non-findings (checked, confirmed safe)

No command injection: All subprocess calls go through run_command with an allowlist and argv list form; shell=False is the effective behavior.
Path containment in remove_worktree: The resolved.parents check on the resolved real path catches symlinks pointing outside root before attempt_directory_removal is reached.
Path containment in remove_empty_orphan_directory: Same pattern; also refuses to remove non-empty directories, protecting uncommitted work in a corrupted orphan.
drop_orphaned_worktree lease deletion is inside state_lock: The same lock that acquire/heartbeat/release hold serializes the lease unlink.
Lease-only orphans untouched: A stale lease with no matching directory is explicitly left to manage_babysit_lease.py reap, not claimed here.
No sensitive data in JSON output: Row fields (key, path, lease_dropped, directory_removed, residual_directory) contain no tokens or credentials.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d98b233c18

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

# Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21019fe414

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…cleanup The documented scoped cleanup (`--pr … --lease-token … --apply`) runs while the worker still holds its lease and releases it in the next orchestration step, but `drop_orphaned_worktree` unlinked the lease record unconditionally. When the orphan belonged to the caller, the matching token let control reach that unlink and deleted the caller's own live lease, so the following `release` failed with `lease does not exist` and ownership was dropped early. `preserve_lease` now scopes the drop to leases that are actually stale — no live hold, or a hold that expired. The `main` call site derives it from the same active-lease probe that authorized the run.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ddb9c4ab65

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

# Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md

claude · 2026-07-25T23:17:11Z

Claude encountered an error after 2s —— View job

I'll analyze this and get back to you.

claude · 2026-07-25T23:17:17Z

Claude encountered an error after 2s —— View job

I'll analyze this and get back to you.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfa0d490c4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…nished cleanup Two gaps in the orphan path, both leaving a worktree directory stranded at its deterministic one-per-PR path where a replacement can never be created. `git -C <path>` runs as if git had started in that directory, so upward discovery answers from an ancestor checkout whenever the worktree root sits inside one. `git status` then succeeds while saying nothing about the candidate, the entry reads as healthy, and an open PR sticks as `keep_open` while a closed one errors in `git worktree remove`. `is_orphaned_entry` now compares `rev-parse --show-toplevel` against the candidate path, so an ancestor's answer is an orphan just as surely as `fatal: not a git repository`. A non-empty orphan is deliberately never force-deleted, but the row still reported `dropped: true` and exited zero. It now reports `dropped: false` with `residual_directory: true` and a stderr warning, reusing the vocabulary `remove_worktree` already uses for its own surviving directory.

# Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md

claude · 2026-07-25T23:34:23Z

Claude encountered an error after 2s —— View job

I'll analyze this and get back to you.

claude · 2026-07-25T23:34:33Z

Claude encountered an error after 2s —— View job

I'll analyze this and get back to you.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 261f34e479

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

# Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md # plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py # plugins/source-control/skills/babysit-prs/scripts/tests/test_prune_babysit_worktrees.py

A removed orphan directory left the owning repository's worktree record in place, so the deterministic path still rejected `git worktree add` as "missing but already registered" while the report claimed a self-heal. The record path is read off the entry's own gitdir pointer and pruned there; where it cannot be known, the state is reported rather than assumed. Orphan detection also matched git's English "not a git repository" text, so on a localized machine every orphan surfaced as an unrelated error instead. The probe now pins LC_ALL=C through a new env_overrides seam parameter.

claude · 2026-07-26T02:05:35Z

Claude finished @kyle-sexton's task in 3m 49s —— View job

PR Review

Gather context and understand the request
Read changed files (prune_babysit_worktrees.py, test_prune_babysit_worktrees.py, babysit_util.py, plugin.json, CHANGELOG.md)
Review main script changes
Review test coverage
Review manifest and changelog
Post findings

Summary

This PR has evolved through several review cycles (initial Claude + security review, Codex P2 rounds) and has addressed its major findings well. The final shape is sound. Three items remain that are worth addressing before merge; one informational note below those.

Issues

1. Vacuous assertFalse(lease_path.exists()) in StaleWorktreeRegistrationTests — correctness gap

test_prune_babysit_worktrees.py line 345

The test constructs lease_path but never writes to it, so assertFalse(lease_path.exists()) is trivially True regardless of the code under test — orphan_registration_state never touches lease state. The assertion gives no coverage and reads as an implicit claim that the prune cleared the lease, which it didn't. The two real assertions (info == "pruned" and the worktree list check) are the meaningful ones; the lease assertion should be removed or replaced with one that actually verifies something reachable from orphan_registration_state.

# Remove this line — lease_path was never written and orphan_registration_state
# doesn't touch leases; this assertion is always True:
self.assertFalse(lease_path.exists())

Fix this →

2. state_lock(lease_path) + lease_path.unlink() missing sidecar comment — documentation gap (Windows portability)

prune_babysit_worktrees.py line 371

The original Claude review flagged this: lease_path.unlink(missing_ok=True) is called while holding state_lock(lease_path). On POSIX, unlinking a locked inode is fine. On Windows — the platform this PR targets — if state_lock holds a direct exclusive handle on lease_path (rather than a separate sidecar), unlink raises PermissionError, which propagates uncaught and sets exit_code = 1, defeating the self-healing for every Windows run. Whether the implementation is safe depends entirely on how state_lock acquires its lock. The fix from the original review still applies: a one-line comment documents the assumption so a future reader doesn't need to audit babysit_state.py:

# state_lock holds a sidecar file, not an exclusive handle on lease_path itself,
# so unlinking lease_path while the lock is held is safe on Windows.
lease_path.unlink(missing_ok=True)

If state_lock does NOT use a sidecar and this assumption is wrong, the bug is critical for Windows and needs a fix rather than a comment.

Fix this →

3. worktree_toplevel called twice per orphan entry — minor inefficiency

prune_babysit_worktrees.py line 376

main() calls is_orphaned_entry(worktree.path) → worktree_toplevel(path) to decide the entry is orphaned. Then drop_orphaned_worktree calls worktree_toplevel(worktree.path) again (line 376) to determine never_registered. That's two git subprocesses for the same query. The result from the first call could be threaded through: is_orphaned_entry already has the toplevel value — returning it (or exposing it as a named helper) would let drop_orphaned_worktree skip the second probe. Not a correctness issue — just an observable extra subprocess per orphan.

Minor notes

Type-b orphan + .git pointer edge case — informational only

orphan_registration_state returns "not-applicable" for never_registered=True regardless of whether registered_repo was resolved. A directory that is both inside an ancestor checkout (type-b orphan) AND was previously a proper linked worktree (so still has a readable .git pointer naming its owning repo) would skip the git worktree prune call and leave a stale $GIT_DIR/worktrees/ record behind — exactly the registration problem the registration_pruned field was added to catch. This scenario requires the babysit root to sit inside another git checkout, which the design explicitly discourages, so it's very theoretical. Not a blocker.

Everything else looks good

Two-tier deletion strategy: unconditional rmtree after git-confirmed removal vs. empty-only rmdir for orphan is the correct design. The distinction between attempt_directory_removal and remove_empty_orphan_directory is clearly documented and well-tested.
Locale pinning: C_LOCALE_ENV = {"LC_ALL": "C", "LANGUAGE": ""} correctly handles GNU gettext outranking LC_ALL. All orphan-detection probes that parse git output run under this env. ✓
Ancestor-checkout detection (is_orphaned_entry / worktree_toplevel): comparing --show-toplevel against the candidate path is the right approach; the alternative of relying on fatal: not a git repository alone misses the ancestor-answer case. ✓
--apply dry-run gate: orphan self-healing is now gated on args.apply like every other mutation. Flagless runs report action: drop_orphan with dropped: false and leave both the directory and the lease on disk. ✓
preserve_lease derivation: active_lease is not None at the callsite in main is correct — by that point the keep_leased guard has already filtered out leases whose token doesn't match, so a non-None active_lease means the caller's own token authorized the run. ✓
Stale registration path: registered_repo_from_gitdir_pointer → prune_repo_worktree_records correctly addresses the "missing but already registered" case the last Codex review caught. ✓
Path containment: root.resolve() not in resolved.parents in remove_empty_orphan_directory catches symlink traversal. TOCTOU between iterdir() and rmdir() is benign (content appearing between the check and the remove causes rmdir to raise, which is caught). ✓
Non-empty orphan reporting: dropped: false, residual_directory: true, and a stderr warning. Consistent vocabulary with remove_worktree's own residual-directory outcome. ✓
CHANGELOG and version bump: consistent with the changes; covers every behavioral delta added across all review-iteration commits. ✓

claude · 2026-07-26T02:05:37Z

Claude finished @kyle-sexton's task in 3m 34s —— View job

Security Review

Gather context and understand the request
Fetch PR diff
Read changed files (prune_babysit_worktrees.py, babysit_util.py)
Analyze new code paths for security issues
Post findings

Findings

IMPORTANT — `registered_repo_from_gitdir_pointer` → `prune_repo_worktree_records`: no containment check on the derived repository path

Confidence: HIGH

Every file-deletion and directory-removal function in this script has an explicit containment check against the babysit root. remove_worktree (line 338–342) refuses to remove anything outside root.resolve(). remove_empty_orphan_directory (line 239–241) does the same. prune_repo_worktree_records has no equivalent guard.

The path it receives — registered_repo — is derived by reading the orphan directory's .git file (line 250–276) and then passed without validation to git -C <registered_repo> worktree prune (line 291). A .git file containing gitdir: /victim/repo/.git/worktrees/name resolves to /victim/repo, and the script runs git worktree prune there — clearing stale worktree records from an arbitrary repository on the filesystem without any relationship to the babysit root.

Exploit path: An actor with write access to the babysit root (e.g., a compromised CI worker) creates a directory matching WORKTREE_RE, writes a crafted .git file into it, and waits for the next --apply prune run. The target repository's worktree records are pruned. Worktree records for directories that are temporarily unavailable (network mounts, snapshots) can be erased; if the process runs with elevated privilege, this extends to any git repository the user can reach.

git worktree prune does not trigger hooks and does not delete file content, so the blast radius is bounded — but the pattern is a textbook path-traversal that breaks the script's own stated containment policy. A two-line guard matching the existing ones would close it:

def prune_repo_worktree_records(repo: Path, root: Path) -> str:
    if root.resolve() not in repo.resolve().parents:
        return "outside-root"
    ...

Fix this →

SUGGESTION — `is_missing_repo_error` matches the full exception string including the command-line path argument

Confidence: PLAUSIBLE

This was flagged in the 2026-07-25 security review as PLAUSIBLE and remains unaddressed in HEAD. The exception message is constructed at babysit_util.py line 141 as f"{' '.join(argv)} failed: {detail}", so the string being searched at prune_babysit_worktrees.py line 203 is:

git -C /the/root/path/owner__repo__pr-1 rev-parse --show-toplevel failed: fatal: not a git repository

The worktree path component (owner__repo__pr-N) cannot contain spaces due to GitHub naming rules, so the marker cannot appear there. However the root path itself (args.root) is operator-supplied and unconstrained: a root at /home/dev/not a git repository/babysit would embed the marker in every exception string from any worktree_toplevel call, causing every git failure (permission errors, disk errors) for every worktree under that root to be misclassified as an orphan and self-healed under --apply, silently swallowing the real error and exit code 1.

The locale issue that motivated the fix is addressed by C_LOCALE_ENV; this is a separate concern about what substring is searched. The fix is to match only against proc.stderr before building the exception, or to export the stderr separately and pass it to the classifier.

Non-findings checked

No command injection: all subprocess calls go through run_command with an allowlist and argv-list form; the new env_overrides path ({**os.environ, **env_overrides}) merges only the hardcoded constant C_LOCALE_ENV = {"LC_ALL": "C", "LANGUAGE": ""} — no user-supplied values reach it in this PR.
--apply dry-run gate on orphan path: fixed per prior review; the orphan branch is now gated on args.apply with the same pattern as the normal removal path.
Active-lease preservation: fixed per prior review; preserve_lease=active_lease is not None prevents unlinking a lease the caller still holds.
Non-empty orphan reporting: fixed per prior review; dropped now reflects the actual directory_removed result and residual_directory: True is set with a stderr warning.
Ancestor-checkout false-negative: fixed by worktree_toplevel / is_orphaned_entry; rev-parse --show-toplevel compared against the candidate path catches ancestor-answered git status.
Stale $GIT_DIR/worktrees record: addressed by prune_repo_worktree_records (the path-containment gap above is the remaining concern here).
Symlink escape in remove_empty_orphan_directory: path.resolve() before resolved.parents correctly follows links.
Lease unlink under state_lock: confirmed inside the with state_lock(lease_path): context in main(); POSIX unlink-while-locked is safe for the sidecar-lock pattern.
No sensitive data in JSON output: row fields contain no tokens or credentials.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dff89b3443

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

`repositorys` tripped the spell-check gate.

claude · 2026-07-26T02:12:08Z

Claude finished @kyle-sexton's task in 6m 38s —— View job

PR Review

Gather context and understand the request
Read changed files (prune_babysit_worktrees.py, test_prune_babysit_worktrees.py, babysit_util.py)
Review main script changes
Review test coverage
Review prior findings — which are resolved, which remain open
Post findings

Summary

This PR has gone through thorough review-and-fix cycles, and most major issues have been resolved well. The two-tier deletion strategy, --apply gating, preserve_lease, ancestor-checkout detection via worktree_toplevel, stale-registration cleanup, locale pinning, and non-empty orphan reporting are all correct. Five items from prior review rounds remain open in HEAD (acf9eee).

Issues

1. prune_repo_worktree_records has no containment check on the derived repository path — medium (carried from security review)

prune_babysit_worktrees.py line 279

The prior security review flagged this as HIGH. The repo path passed here is derived from reading gitdir: <path> out of an orphan's .git file. An actor with write access to the babysit root could craft a .git file containing gitdir: /victim/repo/.git/worktrees/fake, causing git -C /victim/repo worktree prune to run on an arbitrary repository during an --apply run.

Important clarification on the prior review's suggested fix: root.resolve() not in repo.resolve().parents would reject every legitimate case — the main checkout is by design outside the worktree root. That check cannot be applied directly.

The bounded blast radius (git worktree prune only clears admin records, not file content) keeps this at medium rather than high. Mitigations short of a full containment check:

Validate registered_repo actually contains a .git/worktrees/ record for the orphan's name before pruning there, e.g. (registered_repo / ".git" / "worktrees" / worktree.path.name).exists(). This refuses a crafted path that would prune a victim repo where no record for this worktree exists.
Document the intentional trust in the docstring and note it was reviewed.

Fix this →

2. Vacuous assertFalse(lease_path.exists()) in StaleWorktreeRegistrationTests — correctness gap (carried from Claude review 2026-07-26)

test_prune_babysit_worktrees.py line 345

lease_path is derived from state_dir = tmp / "state", but state_dir is never created and no lease file is ever written. lease_path.exists() is trivially False regardless of the code under test — orphan_registration_state doesn't touch leases. The assertion gives no coverage and reads as a claim that the prune cleared the lease, which it didn't (it wasn't there to begin with).

# Remove — lease_path was never written, so this is always True:
self.assertFalse(lease_path.exists())

Fix this →

3. never_registered=True conflates "ancestor answers" with "was never a worktree" — low (flagged by Codex 2026-07-26)

prune_babysit_worktrees.py line 376

never_registered = worktree_toplevel(worktree.path) is not None

orphan_registration_state short-circuits on never_registered=True and returns "not-applicable" — no stale-registration warning, no prune attempt. This is correct for a plain directory under an ancestor checkout that was never a worktree.

It is incorrect when: the babysit root sits inside another git checkout, a previously-registered linked worktree loses its .git pointer, and the ancestor now answers worktree_toplevel. In that case never_registered=True silently skips the "unresolved" warning, even though the owning repository still holds a stale $GIT_DIR/worktrees/ record. A subsequent git worktree add at the deterministic path fails as "missing but already registered" with no prior warning.

The conservative fix: treat ancestor-answers-but-no-pointer as "unresolved" rather than "not-applicable":

ancestor_toplevel = worktree_toplevel(worktree.path)
# `never_registered` only when an ancestor answers AND the pointer is
# still readable — a path that had a pointer (and so was registered)
# but lost it is indistinguishable from one that was always bare;
# report it as "unresolved" rather than silently skipping.
never_registered = ancestor_toplevel is not None and registered_repo is not None

This scenario requires the babysit root to live inside another git checkout (discouraged by design) and a worktree to lose its .git file entirely, so it is low-priority — but the current "not-applicable" label in the CHANGELOG is a false claim for that sub-case.

Fix this →

4. Missing sidecar comment before lease_path.unlink in drop_orphaned_worktree — documentation gap (carried from Claude review 2026-07-25 and 2026-07-26)

prune_babysit_worktrees.py line 371

lease_path.unlink(missing_ok=True) is called while main() holds state_lock(lease_path). On POSIX, unlinking a locked inode is safe. On Windows — the platform this PR specifically targets — if state_lock holds a direct exclusive handle on lease_path itself (rather than a separate sidecar file), unlink raises PermissionError, propagates uncaught, and sets exit_code = 1, defeating the self-healing for every Windows run. Whether the implementation is safe depends entirely on state_lock's internals. A one-line comment documents the assumption so a future reader doesn't need to audit babysit_state.py:

# state_lock holds a sidecar file, not an exclusive handle on lease_path
# itself, so unlinking lease_path while the lock is held is safe on Windows.
lease_path.unlink(missing_ok=True)

If state_lock does NOT use a sidecar, this unlink must move outside the lock context.

Fix this →

Minor notes

worktree_toplevel called twice per orphan — informational (carried from prior Claude review)

is_orphaned_entry(worktree.path) in main calls worktree_toplevel, and then drop_orphaned_worktree calls it again on line 376 to derive never_registered. Two git subprocesses per orphan entry for the same query. The result from is_orphaned_entry's call could be threaded through as a return value or dedicated helper.

is_missing_repo_error searches the full exception string — informational (carried from security review, PLAUSIBLE)

prune_babysit_worktrees.py line 203

The exception message is git -C <path> rev-parse ... failed: <stderr>. If <path> (the operator's --root value) contains the substring not a git repository, permission errors and disk failures from worktree_toplevel would be misclassified as orphan state under --apply. The locale is pinned (good), but the path-in-string concern is separate. Low probability in practice; noted for completeness.

Everything else looks good

Ancestor-checkout detection via worktree_toplevel / is_orphaned_entry is correct and well-tested, including the test_a_directory_answered_by_an_ancestor_checkout_is_an_orphan real-repo test. ✓
--apply gate on orphan path and dry-run contract are both correctly implemented and verified by test_dry_run_reports_the_orphan_without_mutating_it. ✓
preserve_lease correctly prevents dropping an active caller lease; test_scoped_cleanup_leaves_the_authorizing_lease_for_its_caller verifies the failure case against the previous unconditional unlink. ✓
Two-tier deletion strategy: unconditional rmtree post-git-confirm vs. empty-only rmdir for orphan is correct. ✓
Locale pinning with C_LOCALE_ENV = {"LC_ALL": "C", "LANGUAGE": ""} is the right approach; clearing LANGUAGE to outrank GNU gettext is the necessary extra step. ✓
stale_registration warning fires correctly when registration_pruned is neither pruned nor not-applicable. ✓
Non-empty orphan reporting: dropped: false, residual_directory: true, stderr warning. ✓
Symlink containment in remove_empty_orphan_directory: path.resolve() before resolved.parents correctly follows links before the guard. ✓
CHANGELOG entry at [0.31.7] is comprehensive and accurately describes all behavioral changes across the review iterations. ✓

claude · 2026-07-26T02:12:15Z

Claude finished @kyle-sexton's task in 2m 58s —— View job

Security Review

Gather context and understand the request
Fetch PR diff
Read changed files (prune_babysit_worktrees.py, babysit_util.py)
Analyze new code paths for security issues
Post findings

Findings

IMPORTANT — `prune_repo_worktree_records` runs `git worktree prune` in an arbitrary repository with no containment check

Severity: IMPORTANT | Confidence: HIGH

This was flagged in the 2026-07-26T02:05:37Z security review and remains unaddressed in HEAD (acf9eee).

prune_babysit_worktrees.py line 279–294

def prune_repo_worktree_records(repo: Path) -> str:
    try:
        run(["git", "-C", str(repo), "worktree", "prune"])
    except (RuntimeError, OSError):
        return "failed"
    return "pruned"

The repo argument is derived from an untrusted .git file inside the orphaned directory (line 250–276). A .git file containing gitdir: /victim/repo/.git/worktrees/name resolves to /victim/repo, and the script then runs git -C /victim/repo worktree prune — operating on an arbitrary repository that has no relationship to the babysit root.

Every other file-system mutation in this script has a corresponding containment guard:

remove_worktree lines 338–342: if allowed_root not in resolved.parents: raise
remove_empty_orphan_directory lines 239–241: if root.resolve() not in resolved.parents: return False

prune_repo_worktree_records has no equivalent guard. The .git file is attacker-controlled whenever an adversary has write access to the babysit root (a CI worker process, a compromised job, a race between queue-start and directory creation).

Blast radius: git worktree prune itself does not delete file content — it removes entries from $GIT_DIR/worktrees/. But running it in an unrelated repository can erase worktree records for active CI jobs whose directories are temporarily unavailable (network mounts, snapshots, in-progress operations), prematurely dropping their ownership and causing git worktree add at those paths to fail as "already registered". In an environment with elevated CI privileges the same git binary could be configured to run hooks, though worktree prune does not invoke user hooks.

Fix: add the same two-line guard used by the other mutation functions, passing root through orphan_registration_state → prune_repo_worktree_records:

def prune_repo_worktree_records(repo: Path, root: Path) -> str:
    if root.resolve() not in repo.resolve().parents:
        return "outside-root"
    try:
        run(["git", "-C", str(repo), "worktree", "prune"])
    except (RuntimeError, OSError):
        return "failed"
    return "pruned"

Fix this →

SUGGESTION — `is_missing_repo_error` matches against the full exception string, including the operator-supplied root path

Severity: SUGGESTION | Confidence: PLAUSIBLE

This was flagged in both the 2026-07-25 and 2026-07-26 security reviews and remains in HEAD.

prune_babysit_worktrees.py line 190–203

The exception is built in babysit_util.py line 141 as f"{' '.join(argv)} failed: {detail}", so the full string passed to is_missing_repo_error is:

git -C /the/root/path/owner__repo__pr-1 rev-parse --show-toplevel failed: fatal: not a git repository

The .lower() search covers the entire string, including the root path. A root at /home/dev/not a git repository/babysit would match for every git failure against every worktree under that root, causing permission errors and disk failures to be silently classified as orphans (exit code stays 0; the actual error is swallowed). This is low-probability in production but could matter in developer environments with unusual directory naming.

The locale issue is now correctly addressed via C_LOCALE_ENV. The remaining gap is narrowing the search to proc.stderr before the exception is assembled, or restricting the match to the failed: <stderr> suffix of the exception string:

def is_missing_repo_error(exc: Exception) -> bool:
    msg = str(exc)
    # Match only in the stderr portion (after " failed: ") to avoid a false
    # positive when the root path itself contains the marker substring.
    suffix_marker = " failed: "
    idx = msg.find(suffix_marker)
    detail = msg[idx + len(suffix_marker):] if idx != -1 else msg
    return NOT_A_GIT_REPO_MARKER in detail.lower()

Fix this →

Non-findings checked

env_overrides in run_command: only C_LOCALE_ENV = {"LC_ALL": "C", "LANGUAGE": ""} is passed — a hardcoded constant, not user-supplied; no environment-variable injection surface.
No command injection: all subprocess calls go through run_command with allowed_executables=("git", "gh") and argv-list form; the new worktree prune call follows the same pattern.
Path containment in remove_worktree: unchanged from prior passing checks. ✓
Path containment in remove_empty_orphan_directory: path.resolve() before resolved.parents catches symlinks. TOCTOU between iterdir() and rmdir() is benign. ✓
--apply dry-run gate on orphan path: gated correctly on args.apply. ✓
Active-lease preservation: preserve_lease=active_lease is not None prevents unlinking a lease the caller still holds. ✓
Non-empty orphan reporting: residual_directory: True plus stderr warning. ✓
Locale pinning: C_LOCALE_ENV (LC_ALL=C, LANGUAGE="") correctly outranks GNU gettext. ✓
No sensitive data in JSON output: row fields contain no tokens or credentials. ✓

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: acf9eeeb35

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…achable Three defects in the previous commit's orphan handling: The emptiness guard counted a dangling .git gitfile as user work, so the one orphan whose owner is knowable never got removed and never reached the prune. A sole .git file is now unlinked; a .git directory is still never touched. An ancestor checkout answering for the path did not prove the path was never registered -- a nested linked worktree resolves to its ancestor once the pointer is lost, while the record survives. That case is unresolved now. A malformed pointer fails with "invalid gitfile format", not the missing-repository wording, so it re-raised as a hard error forever instead of healing. Marker set widened against git's real C-locale output.

# Conflicts: # plugins/source-control/CHANGELOG.md

claude · 2026-07-26T02:28:22Z

Claude finished @kyle-sexton's task in 3m 55s —— View job

PR Review

Gather context and understand the request
Read changed files at HEAD
Audit open findings from prior review rounds
Review main script (prune_babysit_worktrees.py)
Review test coverage (test_prune_babysit_worktrees.py)
Review utility changes (babysit_util.py)
Post findings

Summary

This PR has gone through extensive review-and-fix cycles (Claude + security + Codex P2, multiple iterations). The code has improved substantially with each round. The two-tier deletion strategy, --apply gating, preserve_lease, ancestor-checkout detection via worktree_toplevel, C_LOCALE_ENV locale pinning, .git gitfile removal before emptiness check, and the three-state orphan_registration_state design are all correctly implemented and well-tested.

Two issues remain open from prior rounds; one is confirmed safe with a missing documentation note.

Issues

1. prune_repo_worktree_records runs git worktree prune in a path from an untrusted .git file — medium (security)

prune_babysit_worktrees.py line 298–313

This was flagged in both security review rounds (HIGH, then IMPORTANT) and remains open in HEAD. registered_repo is derived by reading gitdir: <path> out of an orphan's .git file. Nothing validates that path before passing it to git -C <path> worktree prune. An actor with write access to the babysit root can craft a .git file pointing to an arbitrary repository on the filesystem.

The prior review's suggested root.resolve() not in repo.resolve().parents was correctly identified as unworkable (the main checkout is by design outside root). A workable guard that does not require threading root through: validate that registered_repo is structurally a git repository before pruning there:

def prune_repo_worktree_records(repo: Path) -> str:
    if not (repo / ".git").is_dir():
        return "failed"
    try:
        run(["git", "-C", str(repo), "worktree", "prune"])
    except (RuntimeError, OSError):
        return "failed"
    return "pruned"

A stronger guard — pass the worktree name through and require repo / ".git" / "worktrees" / worktree_name to exist before pruning — narrows it further to the specific record relationship, which is what registered_repo_from_gitdir_pointer already traced. Either is a meaningful step: git worktree prune has bounded blast radius (only drops admin records, not file content), but running it in an unrelated repository against active CI jobs can prematurely invalidate their registrations.

Fix this →

2. lease_path.unlink inside state_lock — safe, but the assumption is undocumented — low (documentation)

prune_babysit_worktrees.py line 390

This was flagged in three consecutive review rounds and is confirmed safe: state_lock opens path.with_suffix(f"{path.suffix}.lock") — a .lock sidecar — not the lease file itself (babysit_state.py line 116). Unlinking the .json lease while holding a lock on the .lock sidecar is safe on both POSIX and Windows. The concern is not correctness — it's that a future reader has to audit babysit_state.py to confirm it. A one-line comment closes that:

# state_lock holds lease_path.with_suffix(".lock"), not an exclusive handle
# on lease_path itself, so unlinking the .json while the .lock is held is
# safe on Windows.
lease_path.unlink(missing_ok=True)

Fix this →

Minor notes

is_missing_repo_error searches the full exception string — informational

prune_babysit_worktrees.py line 209

run_command builds the exception as f"{' '.join(argv)} failed: {detail}", so the full string includes the worktree path (and, transitively, the root path). If the operator's --root value itself contains one of the marker substrings (e.g. /home/dev/not a git repository/babysit), every git failure against any worktree under that root would be misclassified as an orphan, swallowing real errors. Locale is correctly pinned. This remains low-probability in practice. Narrowing the search to the " failed: " suffix (the stderr portion of the message) would eliminate it:

def is_missing_repo_error(exc: Exception) -> bool:
    msg = str(exc)
    idx = msg.find(" failed: ")
    detail = msg[idx + len(" failed: "):] if idx != -1 else msg
    return any(marker in detail.lower() for marker in NOT_A_WORKTREE_MARKERS)

Confirmed resolved since the last review round

The following issues from earlier rounds are confirmed fixed in HEAD and no longer require action:

Vacuous assertFalse(lease_path.exists()): the test class was fully restructured — test_prunes_the_owning_repositorys_record_via_the_gitdir_pointer is gone; StaleWorktreeRegistrationTests now contains test_self_heals_a_registered_orphan_end_to_end with real assertions. ✓
--apply dry-run gate: orphan self-healing is gated on args.apply; test_dry_run_reports_the_orphan_without_mutating_it verifies both the directory and lease survive a flagless run. ✓
preserve_lease derivation: active_lease is not None at the callsite correctly prevents dropping a token-matched live lease. ✓
Non-empty orphan reporting: dropped: false, residual_directory: true, stderr warning — confirmed in test_a_non_empty_orphan_is_reported_unfinished_not_dropped. ✓
Ancestor-checkout detection: worktree_toplevel + os.path.normcase comparison; real-repo test test_a_directory_answered_by_an_ancestor_checkout_is_an_orphan. ✓
Locale pinning: C_LOCALE_ENV = {"LC_ALL": "C", "LANGUAGE": ""} — clears LANGUAGE to outrank GNU gettext. ✓
.git gitfile unlinked before emptiness check: remove_empty_orphan_directory handles the single-gitfile case; the end-to-end test_self_heals_a_registered_orphan_end_to_end confirms git worktree add succeeds at the same path afterwards. ✓
Malformed gitfile detection: NOT_A_WORKTREE_MARKERS covers both not a git repository and invalid gitfile format; confirmed by test_a_corrupted_pointer_is_an_orphan_not_an_error. ✓
never_registered conflation: removed entirely; orphan_registration_state is pointer-only (pruned/failed/skipped/unresolved). ✓
Stale registration self-heal: orphan_registration_state docstring correctly states the three-state semantics and why ancestor-checkout answer doesn't prove "never registered". ✓

claude · 2026-07-26T02:28:28Z

Claude finished @kyle-sexton's task in 3m 50s —— View job

Security Review

Gather context and understand the request
Fetch PR diff and read changed files at HEAD (7d381ba)
Audit prune_babysit_worktrees.py for security issues
Audit babysit_util.py for security issues
Check whether open findings from prior reviews are resolved in HEAD
Report findings

Findings

IMPORTANT — `prune_repo_worktree_records` runs `git worktree prune` on an attacker-controlled path with no validation

Severity: IMPORTANT | Confidence: HIGH

This was flagged in both prior security review rounds and remains unaddressed in HEAD (7d381ba).

prune_babysit_worktrees.py L298–313

def prune_repo_worktree_records(repo: Path) -> str:
    try:
        run(["git", "-C", str(repo), "worktree", "prune"])
    ...

repo is derived from reading the orphan's .git gitfile at registered_repo_from_gitdir_pointer L269–295. A .git file is attacker-controlled whenever an adversary has write access to the babysit root (a compromised CI worker, a race between queue-start and directory creation). A file containing gitdir: /victim/repo/.git/worktrees/fake resolves to /victim/repo, and git -C /victim/repo worktree prune then runs on that repository — an arbitrary repository with no relationship to the babysit root.

Every other file-system mutation in this script has a containment check:

remove_worktree L357–362: allowed_root not in resolved.parents
remove_empty_orphan_directory L254–256: root.resolve() not in resolved.parents

prune_repo_worktree_records has no equivalent guard, and no validation that the derived path has any relationship to the babysit root.

Exploit path: Actor with write access to the babysit root creates owner__repo__pr-999/ (matches WORKTREE_RE) with a crafted .git file containing gitdir: /victim/repo/.git/worktrees/orphan. At queue-start --apply, is_orphaned_entry classifies it as orphaned (the path is not a real worktree), registered_repo_from_gitdir_pointer parses /victim/repo/.git/worktrees/orphan, locates .git in the path's parents, returns /victim/repo, and git -C /victim/repo worktree prune runs there.

Blast radius: git worktree prune does not delete file content — it removes entries from $GIT_DIR/worktrees/. In a CI environment with concurrent workers this can erase worktree registrations for jobs whose directories are temporarily unavailable (network mounts, in-progress ops), causing git worktree add at those paths to fail as "missing but already registered". The prior review correctly noted that the simple root containment check root.resolve() not in repo.resolve().parents can't be used because the main checkout is intentionally outside the babysit root. A workable guard: validate that registered_repo contains a .git/worktrees/<orphan-name> record before running prune there, which refuses a crafted path where no such record exists:

def prune_repo_worktree_records(repo: Path, worktree_name: str) -> str:
    if not (repo / ".git" / "worktrees" / worktree_name).exists():
        return "not-registered"
    try:
        run(["git", "-C", str(repo), "worktree", "prune"])
    ...

Fix this →

SUGGESTION — `is_missing_repo_error` searches the full exception string, including the operator-supplied root path

Severity: SUGGESTION | Confidence: PLAUSIBLE

This was flagged in both prior security review rounds and remains in HEAD.

prune_babysit_worktrees.py L196–210

The RuntimeError is built in babysit_util.py at L141 as f"{' '.join(argv)} failed: {detail}", so the string searched here is the full git -C <root>/<worktree-name> rev-parse --show-toplevel failed: <stderr>. If the operator's --root value contains the substring not a git repository (e.g., a developer running from /home/dev/not a git repository/babysit), every git failure for every worktree under that root — permission errors, disk failures, timeout — is misclassified as orphan state under --apply and self-healed instead of surfacing as a real error. The locale is correctly pinned via C_LOCALE_ENV; this is a separate concern about the path being part of the searched string. Fix: match only against the stderr suffix (after " failed: "):

def is_missing_repo_error(exc: Exception) -> bool:
    msg = str(exc)
    suffix_marker = " failed: "
    idx = msg.find(suffix_marker)
    detail = msg[idx + len(suffix_marker):] if idx != -1 else msg
    return any(marker in detail.lower() for marker in NOT_A_WORKTREE_MARKERS)

Non-findings — confirmed safe in HEAD

state_lock sidecar confirmed: state_lock opens path.with_suffix(f"{path.suffix}.lock") (babysit_state.py L116), which is a separate sidecar file. lease_path.unlink(missing_ok=True) while state_lock(lease_path) is held is safe on Windows — the lock is on the sidecar, not on lease_path itself. The prior documentation gap (missing comment) is a non-security documentation issue; it is now confirmed not a bug.
--apply dry-run gate: orphan self-healing correctly gated on args.apply at L541. Dry-run reports dropped: false without mutating state. ✓
preserve_lease: preserve_lease=active_lease is not None prevents dropping a caller's live lease. ✓
No command injection: all subprocess calls go through run_command with allowed_executables=("git", "gh") and argv-list form. ✓
env_overrides merge: only C_LOCALE_ENV = {"LC_ALL": "C", "LANGUAGE": ""} (a hardcoded constant) is passed — no user-supplied values reach it. ✓
Path containment in remove_worktree: allowed_root not in resolved.parents guard refuses out-of-root paths. ✓
Path containment in remove_empty_orphan_directory: root.resolve() not in resolved.parents correctly follows symlinks before the containment check. TOCTOU between iterdir() and rmdir() is benign. ✓
.git file vs directory distinction: children[0].is_file() check before unlinking correctly leaves .git directories (standalone repos) untouched; only removes the gitfile pointer. ✓
Non-empty orphan reporting: dropped: false, residual_directory: true, stderr warning. ✓
Locale pinning: C_LOCALE_ENV (LC_ALL=C, LANGUAGE="") correctly addresses GNU gettext outranking LC_ALL. ✓
No sensitive data in JSON output: row fields contain no tokens or credentials. ✓

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d381ba79b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…tration Restore the gitfile when the follow-up rmdir fails -- it is the only record of the owning repository, so dropping it on a lock made a retryable failure permanent. Derive the repository from the record's own worktrees/<name> structure so a bare-clone hub, whose common directory is hub.git and has no .git ancestor, is recoverable rather than silently unresolved. Verify the prune by re-reading `worktree list` instead of trusting the exit status: git keeps a locked record and still exits 0, so a locked orphan reported a repair that had not happened. Paths are compared resolved, since git prints POSIX separators and long names while the caller may hold a Windows 8.3 short name for the same directory.

claude · 2026-07-26T02:39:24Z

Claude finished @kyle-sexton's task in 5m 14s —— View job

PR Review

Gather context and understand the request
Read changed files at HEAD (1a3276e)
Audit open findings from prior review rounds
Review main script, tests, and utility changes
Post findings

Summary

The three items Codex P2 flagged on the last round are all correctly addressed in 1a3276e: the gitfile is restored when rmdir fails (preventing the next run from being stranded at unresolved), bare-hub registrations are recovered via the worktrees/ segment rather than a .git-named-ancestor search, and prune_repo_worktree_records now re-reads worktree list --porcelain post-prune rather than trusting the exit code — so a locked record is correctly reported as "failed" rather than as a completed repair. The worktree_toplevel double-call concern from earlier rounds is gone: never_registered was removed entirely and drop_orphaned_worktree no longer calls worktree_toplevel at all.

One security issue persists across every prior round and is still unresolved at HEAD. One documentation gap has been carried through four rounds and is now confirmed safe but still undocumented.

Issues

1. prune_repo_worktree_records still runs git worktree prune on an unvalidated path — medium (security, carried from all prior security review rounds)

prune_babysit_worktrees.py lines 337–338

run(["git", "-C", str(repo), "worktree", "prune"])
run(["git", "-C", str(repo), "worktree", "list", "--porcelain"])

repo is derived by reading gitdir: <path> from an orphaned directory's .git file — attacker-controlled whenever an adversary can write to the babysit root (a compromised CI worker). A crafted .git file containing gitdir: /victim/repo/.git/worktrees/fake causes git -C /victim/repo worktree prune to run on an arbitrary repository. The post-prune worktree list check (new in 1a3276e) correctly verifies whether the specific record survived, but it does not prevent the prune from running in an unrelated repo first.

The root.resolve() not in repo.resolve().parents containment check can't be used here (the main checkout is intentionally outside the babysit root). The workable guard — which the prior security reviews described — is to verify the orphan's record actually exists in the repo before running prune there. For a standard clone the record is at repo/.git/worktrees/<worktree-name>/; for a bare hub it is at repo/worktrees/<worktree-name>/:

def prune_repo_worktree_records(repo: Path, worktree_path: Path) -> str:
    # Validate the specific record exists before pruning: a crafted .git
    # gitfile naming an unrelated repo would have no record for this worktree.
    git_dir = repo / ".git" if (repo / ".git").is_dir() else repo
    if not (git_dir / "worktrees" / worktree_path.name).is_dir():
        return "failed"
    try:
        run(["git", "-C", str(repo), "worktree", "prune"])
        ...

Fix this →

2. Missing sidecar comment before lease_path.unlink(missing_ok=True) — low (documentation, carried from four prior rounds)

prune_babysit_worktrees.py line 429

lease_path.unlink(missing_ok=True) is called while main() holds state_lock(lease_path). This is now confirmed safe: state_lock opens path.with_suffix(f"{path.suffix}.lock") — a .lock sidecar, not lease_path itself — so unlinking the .json while the .lock is held is safe on Windows. The concern that's been raised four times is that a future reader has to audit babysit_state.py to confirm it. A one-line comment closes it:

# state_lock holds lease_path.with_suffix(".lock"), not an exclusive handle on
# lease_path itself — unlinking the .json while the .lock is held is safe on Windows.
lease_path.unlink(missing_ok=True)

Fix this →

Minor notes

is_missing_repo_error searches the full exception string — informational (carried from prior security reviews, PLAUSIBLE)

prune_babysit_worktrees.py line 209–210

The RuntimeError string includes the full command (with the root path): git -C /root/path/worktree rev-parse ... failed: fatal: not a git repository. If the operator's --root value itself contains one of the marker substrings, every git failure under that root would be misclassified as an orphan under --apply. Locale is correctly pinned; this is a separate path-in-string concern. Low probability in practice, but has persisted through multiple rounds.

Confirmed fixed in `1a3276e` (no prior review coverage)

Gitfile restored when rmdir fails: remove_empty_orphan_directory now captures the pointer bytes before unlinking, restores on OSError from rmdir, and re-raises to the outer except OSError: pass — leaving a retryable failure retryable rather than permanently unresolved. Test test_restores_the_pointer_when_the_directory_will_not_go verifies the restore path under a patched Path.rmdir. ✓
Bare-hub registration recovery: registered_repo_from_gitdir_pointer now derives the common directory from the worktrees/ segment of the recorded path rather than searching for a .git-named ancestor — hub.git/worktrees/<name> has no .git ancestor but does have the worktrees segment. Test test_recovers_a_bare_hubs_repository_from_the_pointer confirms the path. ✓
Locked record correctly reported: prune_repo_worktree_records now re-reads worktree list --porcelain after prune and returns "failed" when the target path still appears — git worktree prune exits 0 for a locked record while keeping it, so exit-code trust would have falsely reported a completed repair. Test test_a_locked_record_is_not_reported_as_pruned verifies this against a real locked worktree. ✓
worktree_toplevel double-call eliminated: drop_orphaned_worktree no longer calls worktree_toplevel to derive never_registered — that concept was removed entirely; orphan_registration_state derives its verdict from registered_repo_from_gitdir_pointer (a file read) only. ✓

Everything else looks good

remove_empty_orphan_directory logic: The inner-try / outer-catch structure (unlink pointer, re-list, rmdir; restore on failure, re-raise to the outer swallow) is correct across all cases: empty with no pointer, sole gitfile present, and rmdir blocked.
Ordering in drop_orphaned_worktree: registered_repo_from_gitdir_pointer runs before remove_empty_orphan_directory — the pointer is read while it still exists, then the directory goes, then the record is pruned. The docstring states this constraint explicitly. ✓
orphan_registration_state three-state semantics: skipped / pruned/failed / unresolved correctly models what is knowable from the path alone, with the docstring explaining why an ancestor-checkout answer doesn't prove "never registered." ✓
Test coverage for 1a3276e additions: test_self_heals_a_registered_orphan_end_to_end ends with git worktree add at the same deterministic path to confirm it is actually reusable — the proof that matters, not just the intermediate fields. ✓
CHANGELOG and version bump (0.31.8): consistent with all behavioral changes across the review iterations. ✓

claude · 2026-07-26T02:39:27Z

Claude finished @kyle-sexton's task in 3m 27s —— View job

Security Review

Gather context and understand the request
Fetch PR diff and read changed files at HEAD (1a3276ee)
Audit prune_babysit_worktrees.py for security issues
Audit babysit_util.py for security issues
Review whether prior security findings are resolved at HEAD
Report findings

Findings

IMPORTANT — `prune_repo_worktree_records` runs `git worktree prune` on an attacker-controlled path with no validation

Confidence: HIGH

This was flagged in every prior security review round (HIGH → IMPORTANT → IMPORTANT) and remains unaddressed at HEAD (1a3276ee).

prune_babysit_worktrees.py L321–352

def prune_repo_worktree_records(repo: Path, worktree_path: Path) -> str:
    try:
        run(["git", "-C", str(repo), "worktree", "prune"])
        listed = run(["git", "-C", str(repo), "worktree", "list", "--porcelain"])
    except (RuntimeError, OSError):
        return "failed"

repo is derived from registered_repo_from_gitdir_pointer(worktree.path) (L283–318), which reads the orphan directory's .git file and parses gitdir: <path>. A .git file containing gitdir: /victim/repo/.git/worktrees/fake causes registered_repo_from_gitdir_pointer to return /victim/repo, and git -C /victim/repo worktree prune then runs — in an arbitrary repository with no relationship to the babysit root.

The worktree list verification step does not mitigate this. The post-prune check looks for worktree_path (the babysit orphan's path) in the victim repo's worktree list output. The victim repo has no worktree at the orphan's path, so the loop finds no match and returns "pruned" — confirming a "successful" repair while git worktree prune has run on the wrong repository. Every crafted attack results in registration_pruned: "pruned" in the output row.

Exploit path: Any actor with write access to the babysit root creates a directory matching WORKTREE_RE (owner__repo__pr-N) containing a crafted .git file: gitdir: /victim/repo/.git/worktrees/orphan. At the next --apply prune run, is_orphaned_entry classifies it as orphaned (the toplevel doesn't match), drop_orphaned_worktree reads the .git pointer, and git -C /victim/repo worktree prune runs there — removing stale worktree registrations from $GIT_DIR/worktrees/.

Blast radius: git worktree prune does not delete file content, but it can erase worktree registrations for jobs whose directories are temporarily unavailable (network mounts, in-progress CI runs), causing git worktree add at those paths to fail as "missing but already registered". In environments where the CI user can reach system-wide git repositories (common on shared runners), the scope extends to any git repository on the filesystem.

Fix options — the prior review identified a workable guard that doesn't require threading root through. The minimal version validates that the repo path is structurally a git repository containing a record for this specific worktree before pruning:

def prune_repo_worktree_records(repo: Path, worktree_path: Path) -> str:
    # Validate the derived repo path before running git there: a crafted .git
    # file could name an arbitrary filesystem path with no relationship to the
    # babysit root. Check that the record for this worktree actually exists in
    # the repo before trusting the path enough to run git worktree prune there.
    worktree_name = worktree_path.name
    if not (repo / ".git" / "worktrees" / worktree_name).is_dir():
        return "failed"
    try:
        run(["git", "-C", str(repo), "worktree", "prune"])
        ...

Fix this →

SUGGESTION — `is_missing_repo_error` searches the full exception string including the operator-supplied `--root` path

Confidence: PLAUSIBLE

This was flagged in three prior security review rounds and remains unaddressed at HEAD.

prune_babysit_worktrees.py L196–210

The exception is built in babysit_util.py at L141 as f"{' '.join(argv)} failed: {detail}". The string searched in is_missing_repo_error therefore includes the full command — git -C /the/root/path/owner__repo__pr-1 rev-parse --show-toplevel failed: <stderr> — with the operator's --root path embedded. If that path contains not a git repository (e.g., a developer running from /home/dev/not a git repository/babysit), every git failure for every worktree under that root — permission errors, disk failures — is misclassified as orphan state and self-healed under --apply, swallowing the real error and exit code 1.

Locale is correctly pinned via C_LOCALE_ENV; this is a separate issue about what portion of the string is matched. The fix: search only the stderr suffix, not the full message:

def is_missing_repo_error(exc: Exception) -> bool:
    msg = str(exc)
    suffix_marker = " failed: "
    idx = msg.find(suffix_marker)
    detail = msg[idx + len(suffix_marker):] if idx != -1 else msg
    return any(marker in detail.lower() for marker in NOT_A_WORKTREE_MARKERS)

Non-findings — confirmed safe at HEAD

--apply dry-run gate: orphan self-healing is gated on args.apply at L580. Dry-run reports dropped: false without mutating state. ✓
preserve_lease: preserve_lease=active_lease is not None at L585 prevents dropping a caller's live lease. ✓
No command injection: all subprocess calls go through run_command with allowed_executables=("git", "gh") and argv-list form. ✓
env_overrides merge: only C_LOCALE_ENV = {"LC_ALL": "C", "LANGUAGE": ""} (a hardcoded constant) is ever passed — no user-supplied values reach it. ✓
Path containment in remove_worktree: allowed_root not in resolved.parents at L397–401 refuses out-of-root paths. ✓
Path containment in remove_empty_orphan_directory: root.resolve() not in resolved.parents at L255 correctly follows symlinks before the check. TOCTOU between iterdir() and rmdir() is benign. ✓
state_lock sidecar confirmed safe: babysit_state.py:116 opens path.with_suffix(f"{path.suffix}.lock") — a separate sidecar file, not an exclusive handle on lease_path itself. lease_path.unlink() while state_lock(lease_path) is held is safe on Windows. ✓
.git gitfile handling: children[0].is_file() check at L260 correctly leaves .git directories untouched; only removes gitfile pointers. ✓
Restore-on-rmdir-failure: the gitfile bytes are saved before unlink and written back if rmdir raises, so a locked-directory failure stays retryable rather than permanently losing the registration pointer. ✓
Prune verification via worktree list: correct for its stated purpose (catching locked records that prune keeps despite exiting 0) — the gap is in what repo path is passed to prune, not in the verification logic itself. ✓
Bare repository support: registered_repo_from_gitdir_pointer correctly identifies the common directory for bare-hub clones (where there is no .git-named ancestor) by walking the worktrees/ path structure. ✓
Locale pinning: C_LOCALE_ENV = {"LC_ALL": "C", "LANGUAGE": ""} correctly addresses GNU gettext outranking LC_ALL. ✓
Non-empty orphan reporting: dropped: false, residual_directory: true, stderr warning. ✓
No sensitive data in JSON output: row fields contain no tokens or credentials. ✓
ancestor checkout detection: worktree_toplevel + os.path.normcase comparison correctly classifies entries answered by an ancestor checkout as orphans. ✓

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a3276eeab

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

`git worktree prune` takes no path and drops every prunable record in the repository, so a scoped `--pr <one PR> --apply` orphan self-heal also discarded the administrative record of any unrelated worktree whose directory happened to be missing at that moment -- an unmounted share, a removable drive, a checkout mid-restore -- despite it being outside the requested scope. Reproduced on git 2.55.0.windows.3: register two worktrees, delete both directories, prune on behalf of one, and both records vanish. `prune_repo_worktree_records` now clears the record with `git worktree remove <path>`, which names its one target and behaves identically from a standard clone and from a bare hub. The deliberate consequence is that unrelated stale records are no longer swept up as a side effect: clearing those stays `git worktree prune`'s job, run by the operator or by `git gc`, not a decision a single-PR cleanup makes. The existing verify-by-`worktree list` rule is what keeps the swap honest in both directions, since `remove` exits nonzero both for a locked record (correctly `failed`) and for a record that is already gone (correctly `pruned`). `--force` is never passed, and -- because `remove`, unlike `prune`, deletes a worktree's contents -- a still-present directory now returns `skipped` instead of being handed to git, enforcing the caller's `directory_removed` precondition here rather than assuming it. Four regression tests: the sibling record survives a targeted clear (standard clone and bare hub, both verified failing against the repo-wide prune), an already-absent record reads `pruned`, and a surviving directory with uncommitted content is returned `skipped` untouched. Refs #816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude · 2026-07-26T03:37:02Z

Claude encountered an error after 0s —— View job

I'll analyze this and get back to you.

claude · 2026-07-26T03:37:11Z

Claude encountered an error after 0s —— View job

I'll analyze this and get back to you.

github-actions · 2026-07-26T03:37:16Z

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/30186445196
Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":454,"total_cost_usd":0}

Re-running the job, or pushing a new commit, will retry the review.

github-actions · 2026-07-26T03:37:34Z

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/30186445076
Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":393,"total_cost_usd":0}

Re-running the job, or pushing a new commit, will retry the review.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 77ebb64c99

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

No linked issue ## Summary Four stranded P2 review findings, each filed by `chatgpt-codex-connector` on a merged `source-control` PR and left unresolved, verdicted REAL by an independent auditor at `main`. All four are discharged here. A fifth thread on the same sweep (`babysit-prs/reference/safety.md:457`, from #1264) was verdicted ALREADY-FIXED by `72ee66e0` and carries **no change** in this PR. Filed line anchors are stale on several of these threads; every fix was made against the complaint text, not the anchor. ## Fix ### `exec-bit-check.sh` keys its candidate set on a new index *entry* (#1590) `git diff --cached --name-status` reports the same staged file as `A <path>` with rename/copy detection off and as `R<score> <old> <new>` / `C<score> <src> <dst>` with it on. The script read and discarded both pair forms, so whether a newly added shebang file staged `100644` got caught was a function of the consumer's `diff.renames` setting rather than of the staged content. A pair destination is now a candidate when its **source was `100755`** — the mode pairing that means the bit was *dropped*. The scan reads `git diff --cached --raw` rather than `--name-status` for exactly this reason: only the raw record (`:<srcmode> <dstmode> <srcsha> <dstsha> <status>`) carries the source mode. The existing `100644`-plus-shebang filter still does the rest. ### `prune_babysit_worktrees.py` restores the gitfile on every surviving path (#1331) Restoration was keyed on `rmdir` raising. Two other paths leave the directory standing: the rescan after the unlink can itself raise, and a file appearing between the unlink and the rmdir skips the removal *without raising at all*. Either way the directory outlived the only record of its owning repository, turning a retryable failure into a permanent `unresolved`. Restoration is now keyed on whether the removal actually happened (a `removed` flag, not a second `exists()` probe — a probe that transiently failed would skip the restore precisely when the directory survives), and the `Path.exists()` probe runs inside the guarded write. ### Two defects this PR's own first pass introduced, caught in review and fixed here Both were filed by `chatgpt-codex-connector` on this PR, both reproduced before fixing, both real. - **A source-mode-blind candidate set** (`exec-bit-check.sh`). Widening to every `R*`/`C*` destination reported a shebang file that is *deliberately* non-executable — a sourced library, a template — merely for being renamed, and `--fix` flipped it to `100755`. Nothing dropped a bit; the file is already tracked, outside the newly-added-only scope. Reproduced: a committed `100644` shebang plus `git mv` gives `:100644 100644 … R100`, and the pre-fix script listed the destination. Hence the source-mode gate above. - **An unguarded existence probe** (`prune_babysit_worktrees.py`). `pointer.exists()` sat in the `finally` *outside* the try guarding the write. `Path.exists()` re-raises an `OSError` whose errno is outside the ignored not-found family, so a permission denial on the very directory the block exists to rescue escaped the `finally` — replacing the original exception and leaving the pointer deleted, the exact loss the block prevents. On `main` this was contained because the probe sat inside an `except OSError` handler; moving it to `finally` uncontained it. The probe is now inside the guard. ### The conflict orchestrator runs base → head → push, in that order (#1355, two threads) Both threads edit the same push-contract bullet, so they land together. - `safety.md` requires the head check immediately before every push, but the base re-fetch — a network round trip — sat between that check and the push, re-opening the exact window the check closes. The contract is now a three-step numbered list with nothing between step 2 (head) and step 3 (push). - Both orchestrator head checks now spell `GH_REPO=<owner>/<repo>`. The bare `gh pr view <N>` had no target: the orchestrator's cwd is whatever the fleet run started from, never reliably the target repository. Deliberately **not** changed: the bare `gh pr view --json headRefOid` at `orchestration.md:513`. That one is in the **Conflict-Worker Contract**, whose cwd *is* the assigned worktree, and the worker contract's own rule offers `cd`-into-the-worktree and `GH_REPO` as alternatives. The finding scopes itself to "both orchestrator head checks". ### The `VALID (defer)` grounding rule states its no-tracker branch (#1633) **Narrower than filed.** The finding claims the missing branch "permanently blocks `full` mode"; it does not — `pull-request/SKILL.md` §Adapting to your environment and a `VALID (fix now)` reclassification both already escape it. The real defect is the *unstated branch*: the rule mandated filing a tracker item before the D5 reply and said nothing about the consumer with no tracker, even though the same skill documents a tracker as optional. That branch is now stated. The CHANGELOG entry was rewritten to the narrower framing rather than restating the overstated claim. **Surface scope, stated explicitly.** The branch is added to the three surfaces that state the *filing mandate*: the canonical `reference/review-discipline.md` §3 clause and its `pull-request/SKILL.md` and `pull-request/reference/monitor.md` restatements. `babysit-prs/reference/independent-resolution.md` also carries the `D4.6-deferral-grounding` tag and is deliberately left alone — it states what an already-chosen `VALID (defer)` must *show* (eligibility), not an instruction to file, so it has no dead end to branch out of. One rule, both directions: mandate ⇒ branch, eligibility criterion ⇒ no branch. `monitor.md` is **not** forced by `check-contract-clause-coverage.py` — verified by reverting only that file's hunk and re-running the gate, which still passes. It is included on the merits above. ## Verification Every fix was confirmed to stop the complaint reproducing, each with a pre-fix control proving the fixture discriminates. **#1590** — `git version 2.54.0.windows.1`. Same fixture, three configurations: | fixture | raw record | `origin/main` `--list` | this branch `--list` | | --- | --- | --- | --- | | copy, **control** (`diff.renames` unset) | `A dest.sh` | reported | reported | | copy, `diff.renames=copies` | `:100755 100644 … C095 src.sh dest.sh` | *(nothing)* | `dest.sh` | | rename, **default** config | `:100755 100644 … R100 src.sh moved.sh` | *(nothing)* | `moved.sh` | | rename off a **non-exec** source | `:100644 100644 … R100 lib.sh lib-moved.sh` | *(nothing)* | *(nothing)* | The control row is the discriminator: the identical `cp` produces `A` with copy detection off and `C095` with it on, and the destination stages `100644` in both. The last row is the regression guard — nothing dropped a bit there, so nothing is reported on either tree. Running this branch's `exec-bit-check.test.sh` against `origin/main`'s script fails exactly cases 50 and 52; against the intermediate source-mode-blind version it fails exactly case 59; against this branch, **59 cases, 0 failures**. Fixture-assertion cases 57/58 pass on every tree, which is what makes 59 a real discriminator rather than a broken fixture. Note for reviewers: the sibling `--fix -- <dest>` cases pass on *both* trees and are not discriminating — a pathspec naming only the destination breaks the rename pairing back to `A`. The defect is in unscoped detection (`--list` / `--probe` / `--fix --all`), which is what cases 50/52 cover. Case 56 is the other negative half: an ordinary rename that *kept* `100755`. **#1331** — both new tests run against `origin/main`'s module (branch tests, old code) **FAIL** on `assertTrue(pointer.is_file())`. The third test (the raising probe) **ERRORs** with an escaped `PermissionError` against the intermediate version, while its sibling passes there — the control that shows the new fixture targets the new defect. Against this branch the full suite is **45 tests, OK**. **#1355** — prose. Control: `origin/main`'s bullet textually places the base re-fetch after the head check and before the push ("Revalidate the base side in the same breath"). Current: numbered 1-Base / 2-Head / 3-Push with nothing between 2 and 3, and `GH_REPO=` on both orchestrator head checks (`:615`, `:681`). **#1633** — prose. Control: `origin/main` states the filing mandate on all three surfaces with no no-tracker branch. The cited escape hatch (`SKILL.md` §Adapting to your environment, line 36) was read and does say a work-item tracker is optional and that its absence must never block a phase. **Gates run locally from the worktree root, all green:** - `python scripts/check-contract-clause-coverage.py` — passed (4 canonical, 14 tagged restatements, 16 pointing surfaces) - `scripts/check-changelog-parity.sh` `--check` / `--check-order` / `--check-bump origin/main` - `scripts/check-contract-slice-prune.sh` `--check` / `--check-diff origin/main` - `scripts/check-shell-portability.sh origin/main`, `scripts/check-skill-portability.sh origin/main` - `scripts/check-changed-skills.sh origin/main` - `scripts/validate-plugins.sh` - `markdownlint-cli2` over all six changed markdown files — 0 errors - `shellcheck -x` over both changed shell files — clean - `scripts/run-ruff.sh check plugins/source-control` — all checks passed - all nine affected `source-control` suites from `scripts/affected-tests.sh origin/main`, plus the two suites the changed scripts own (`exec-bit-check.test.sh` 59/59, `test_prune_babysit_worktrees.py` 45/45) The prune test file's diff is **purely additive** vs `origin/main` — an editor format-on-write pass had rewrapped three untouched regions, and that drift was stripped so every hunk maps to a finding. `plugins/source-control/skills/babysit-loop/SKILL.md` is untouched and stays at 499 lines. Version `0.48.0` → `0.49.3`, renumbered above `main`'s current `0.49.2` after the merge. ## Related Refs #1590, #1331, #1355, #1633 — the merged PRs carrying the four review threads. Refs #1264 — the fifth thread on this sweep, verdicted ALREADY-FIXED (`72ee66e0`); no change here. Refs #1939 — owns the defects in `babysit_resolve_thread.py`, deliberately untouched by this PR. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

kyle-sexton mentioned this pull request Jul 25, 2026

source-control/babysit-prs: harden prune_babysit_worktrees.py against orphaned state #816

Closed

chatgpt-codex-connector Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py Outdated

kyle-sexton mentioned this pull request Jul 25, 2026

Lane telemetry: work-loop #1248

Closed

Merge remote-tracking branch 'origin/main' into drain-1331

21019fe

# Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md

chatgpt-codex-connector Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py Outdated

chatgpt-codex-connector Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py Outdated

Merge remote-tracking branch 'origin/main' into drain2-1331

bfa0d49

# Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md

chatgpt-codex-connector Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py Outdated

kyle-sexton added 2 commits July 25, 2026 19:27

Merge remote-tracking branch 'origin/main' into drain2-1331

261f34e

# Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md

chatgpt-codex-connector Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py Outdated

kyle-sexton mentioned this pull request Jul 26, 2026

Lane telemetry: babysit-loop #1214

Closed

kyle-sexton added 2 commits July 25, 2026 21:55

chatgpt-codex-connector Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py Outdated

fix(source-control): correct a misspelled test name

acf9eee

`repositorys` tripped the spell-check gate.

chatgpt-codex-connector Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py

Merge remote-tracking branch 'origin/main' into pr1331-work

7d381ba

# Conflicts: # plugins/source-control/CHANGELOG.md

chatgpt-codex-connector Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py Outdated

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py Outdated

chatgpt-codex-connector Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py Outdated

kyle-sexton merged commit 8a56fd3 into main Jul 26, 2026
27 checks passed

kyle-sexton deleted the fix/816-prune-babysit-worktrees-orphaned-state branch July 26, 2026 03:40

chatgpt-codex-connector Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread plugins/source-control/skills/babysit-prs/scripts/prune_babysit_worktrees.py

This was referenced Aug 6, 2026

Triage the remaining stranded P2 review findings from the post-merge sweep #1938

Closed

fix(source-control): discharge four stranded P2 review findings #2098

Merged

Uh oh!

Conversation

kyle-sexton commented Jul 25, 2026

Summary

Test plan

Related

Uh oh!

claude Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review

Summary

Issues

Minor notes

Everything else looks good

Uh oh!

claude Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review

Findings

IMPORTANT — Orphan self-healing bypasses --apply dry-run gate

SUGGESTION — is_missing_repo_error matches the full exception string, including the file path

Non-findings (checked, confirmed safe)

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

claude Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

claude Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review

Summary

Issues

Minor notes

Everything else looks good

Uh oh!

claude Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review

Findings

IMPORTANT — registered_repo_from_gitdir_pointer → prune_repo_worktree_records: no containment check on the derived repository path

SUGGESTION — is_missing_repo_error matches the full exception string including the command-line path argument

Non-findings checked

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

claude Bot commented Jul 25, 2026 •

edited

Loading

claude Bot commented Jul 25, 2026 •

edited

Loading

IMPORTANT — Orphan self-healing bypasses `--apply` dry-run gate

SUGGESTION — `is_missing_repo_error` matches the full exception string, including the file path

claude Bot commented Jul 25, 2026 •

edited

Loading

claude Bot commented Jul 25, 2026 •

edited

Loading

claude Bot commented Jul 25, 2026 •

edited

Loading

claude Bot commented Jul 25, 2026 •

edited

Loading

claude Bot commented Jul 26, 2026 •

edited

Loading

claude Bot commented Jul 26, 2026 •

edited

Loading

IMPORTANT — `registered_repo_from_gitdir_pointer` → `prune_repo_worktree_records`: no containment check on the derived repository path

SUGGESTION — `is_missing_repo_error` matches the full exception string including the command-line path argument

claude Bot commented Jul 26, 2026 •

edited

Loading

claude Bot commented Jul 26, 2026 •

edited

Loading

IMPORTANT — `prune_repo_worktree_records` runs `git worktree prune` in an arbitrary repository with no containment check

SUGGESTION — `is_missing_repo_error` matches against the full exception string, including the operator-supplied root path

claude Bot commented Jul 26, 2026 •

edited

Loading

claude Bot commented Jul 26, 2026 •

edited

Loading

IMPORTANT — `prune_repo_worktree_records` runs `git worktree prune` on an attacker-controlled path with no validation

SUGGESTION — `is_missing_repo_error` searches the full exception string, including the operator-supplied root path

claude Bot commented Jul 26, 2026 •

edited

Loading

Confirmed fixed in `1a3276e` (no prior review coverage)

claude Bot commented Jul 26, 2026 •

edited

Loading

IMPORTANT — `prune_repo_worktree_records` runs `git worktree prune` on an attacker-controlled path with no validation

SUGGESTION — `is_missing_repo_error` searches the full exception string including the operator-supplied `--root` path