fix(source-control): report worktrees the pruner cannot map to a PR - #1390
Conversation
…555) `prune_babysit_worktrees.py` derives each worktree's PR identity from its directory name. A directory that did not match `<owner>__<repo>__pr-<number>` was dropped by `iter_worktrees` before the report was assembled -- not kept, not removed, not an error, simply absent. A caller reading the JSON to answer "is anything left to clean up?" got an empty list while merged PRs' worktrees sat on disk, and had to find and `git worktree remove` them by hand. Fixed with the issue's option (b) -- report the unmappable directory -- rather than option (a), deriving identity from `git worktree list` branch/remote metadata. (b) is a prerequisite of (a), not an alternative to it: this skill documents detached-HEAD operation as required whenever the PR branch is locked in a sibling checkout (`reference/worktrees.md`), and a detached worktree has no branch to map, so (a) still needs this fallback. Two further reasons (a) is the wrong shape here: - Identity feeds the worker-lease guard as well as the removal decision. The directory name is fixed for the worktree's lifetime; git working state is not. A network-derived identity that disagreed with what the worker wrote would make an active lease invisible and delete a live worker's worktree. - The only existing branch-to-PR mapper, `find_open_prs_for_head_ref`, filters to open PRs, and pruning targets merged/closed ones by definition -- so (a) means new `gh` plumbing, and would cost this suite the hermeticity its #438 regression guard deliberately established. Every directory under the root now appears in the report; an unmappable one is an `action: unrecognized` row carrying its path and reason, emitted in every mode including `--pr` -- scoping narrows which PRs are acted on, never which directories are accounted for. Such entries are never removed (identity is a precondition for the PR-state and lease checks that authorize removal) and do not fail the run, so a stray directory cannot break a dry run. Root listing is now sorted so the report is order-stable. `reference/worktrees.md` states the naming convention that was previously only implied by the helper's regex -- the drift the issue identifies as the root cause -- and what becomes of a directory that breaks it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
3620814 to
f36a5eb
Compare
|
Claude finished @kyle-sexton's task in 1m 54s —— View job Code Review — fix(source-control): report worktrees the pruner cannot map to a PR
Overall verdict: Approve. The fix is correct, minimal, and well-tested. No blockers. A few observations below worth considering. CorrectnessThe fix is sound.
Minor: the SecurityNo new risk surface. Unrecognized entries never reach Test CoverageAll three new tests are well-structured and hermetic:
One small gap: there's no test that passes The DocumentationThe VersioningPatch bump 0.26.4 → 0.26.5 is appropriate for a bug fix with no API additions. CHANGELOG entry is accurate and complete. |
|
Claude finished @kyle-sexton's task in 1m 57s —— View job Security Review
No security issues found. I reviewed all five changed files with a focus on the new code paths introduced by this PR. Here is what I checked and why each is clean: Path handling and traversal The Removal guard remains intact Unrecognized entries are never passed to No new shell execution surface
Static reason string, no user-controlled data in output fields
Symlink safety
No new information disclosure The |
…d worktree The report-don't-remove invariant is what makes emitting an `unrecognized` row safe at all, and `--apply` is the only mode that can delete anything. Until now that invariant held only by construction -- the `isinstance` check `continue`s before the apply branch -- which is one refactor away from a silent regression that deletes a directory the tool could not identify. Builds a real linked git worktree named `medley-1567`, runs with `--apply`, and asserts the directory survives, the row is `action: unrecognized`, `removed` is False, and the exit code is 0. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 18s —— View job Code Review — fix(source-control): report worktrees the pruner cannot map to a PR
Overall verdict: Approve. The previous review raised one gap; this synchronize closes it exactly. All findings from the first pass remain resolved; nothing new introduced. What changed in
|
|
Claude finished @kyle-sexton's task in 1m 45s —— View job Security Review — fix(source-control): report worktrees the pruner cannot map to a PR
No security issues found. I reviewed all five changed files against the diff ( Changed surface examined
What I checked and why each is cleanCommand injection / subprocess isolation. The new Path traversal / symlink risk. JSON output / injection. Information disclosure. The
Test file security. GitHub Actions / supply chain. No workflow files changed; no dependency pins altered. |
|
This was generated by AI during autonomous /work-items:work-loop execution. Review triage —
|
| # | Source | Finding | Classification | Evidence |
|---|---|---|---|---|
| 1 | Code review — Correctness / Minor | The worktrees JSON array is now a mixed schema; a consumer unconditionally reading row["key"] would KeyError on an unrecognized row. Asks to confirm no downstream parser assumes the old homogeneous shape. |
VALID — confirmed, no change required | Swept every reference to prune_babysit_worktrees in the repo. All call sites are prose invocations the model reads, not programmatic parsers: skills/babysit-prs/SKILL.md:379, reference/worktrees.md:49/56/63/70, reference/orchestration.md:620. None indexes a row, none pipes the output through jq, none names a per-row key. There is no downstream parser to break. |
| 2 | Code review — Test Coverage / One small gap | No test passes --apply alongside an unrecognized directory, so the "survives even under --apply" invariant is guarded only by construction. |
VALID — fixed | Correct. --apply is the only mode that can delete anything, so the report-don't-remove invariant is exactly what makes emitting an unrecognized row safe. Guarding it by construction alone (the isinstance continue preceding the apply branch) is one refactor away from silent regression. Added test_apply_mode_reports_but_never_removes_an_unrecognized_worktree in 3816bafdd2536fe892e5429a210253cc69e3970e, which builds a real linked git worktree named medley-1567, runs with --apply, and asserts the directory still exists, action: unrecognized, removed: False, exit 0. Suite green locally: 8/8 in test_prune_babysit_worktrees. |
| 3 | Security review | No security issues found. | Informational — acknowledged | Independently consistent with the diff: unrecognized entries continue before any run_command, so no subprocess is constructed for a directory that carries no PR identity; UNRECOGNIZED_REASON is a module-level constant, never derived from the directory name. |
Note on finding 1
Worth stating explicitly since it is a contract question rather than a defect: the heterogeneous
row shape is intended, not incidental. A row's action is the discriminator — unrecognized
rows carry path / action / reason / removed and deliberately have no key, because key
is the PR identity the tool could not derive. Emitting a placeholder key would reintroduce the
exact false-confidence #555 is about. Filed nothing to defer here: there is no affected consumer,
and a schema-versioning change would be scope creep on a bug fix.
Not adopted
Nothing. Both findings were non-blocking and both are resolved above — one by verification, one by
a new test.
…es-nonstandard-names # Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md
|
Claude finished @kyle-sexton's task in 1m 51s —— View job Code Review — fix(source-control): report worktrees the pruner cannot map to a PR
Overall verdict: Approve. Three commits, all clean. The fix is correct, minimal, and fully regression-guarded. This review found no issues not already addressed in prior passes. What this PR does
CorrectnessThe
The SecurityNo new risk surface. Previous security review confirmed this in detail; nothing in the final commit ( Test coverageAll four new tests are hermetic (no
The DocumentationThe new policy bullet in VersioningPatch bump Residual from prior reviews
No open findings. The PR is correct, minimal, safe, and fully regression-guarded. Ready to merge. |
|
Claude finished @kyle-sexton's task in 1m 12s —— View job Security Review — fix(source-control): report worktrees the pruner cannot map to a PR
No security issues found. I reviewed all five changed files for the security surfaces most likely to be affected by this kind of change. Details below. Changed surface examined
What I checked and why each is cleanCommand injection / subprocess isolation. Path traversal / out-of-root removal. JSON output / injection. Information disclosure.
Test file security. GitHub Actions / supply chain. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 893a11616a
ℹ️ 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".
The rationale for reporting an unrecognized worktree before the `--pr` filter claimed "no directory under the root goes unreported". That is false: a recognized worktree whose key does not match the target is dropped by that same filter without a row, and always has been. The real reason the unrecognized branch precedes the filter is narrower and is what the code actually relies on. An unrecognized entry carries no key, so it can never match a target key -- leaving it to the filter would hide it from every scoped run, permanently, which is the #555 silent drop in a different mode. A recognized non-target entry is merely outside the scope the caller declared via `target_pr` and remains visible in an unscoped run, so scoping it out loses no inventory. Behavior is unchanged; the comment and the changelog entry now describe the invariant the code enforces rather than a stronger one it does not.
|
Claude finished @kyle-sexton's task in 1m 35s —— View job Code Review — fix(source-control): report worktrees the pruner cannot map to a PR
Overall verdict: Approve. The latest commit ( New commit:
|
| Finding | Status |
|---|---|
Mixed-schema JSON array (key vs reason) — any downstream parser? |
Not blocking: confirmed all call sites are prose invocations; no programmatic parser in the repo. |
--apply test gap |
Fixed in 3816baf. |
| Code comment overstated the invariant | Fixed in 07fe7c0. |
| CHANGELOG entry overstated the invariant | Fixed in 07fe7c0. |
Codex P2: conforming non-target worktrees silently dropped under --pr |
Pre-existing intentional behavior, not introduced or worsened by this PR. |
| Security review (two passes) | No issues found. |
No open findings. The PR is correct, minimal, safe, and the documentation now precisely describes what the code enforces.
|
Claude finished @kyle-sexton's task in 1m 39s —— View job Security Review — fix(source-control): report worktrees the pruner cannot map to a PR
No security issues found. I reviewed all five changed files against the diff, focusing on the surfaces most likely to be affected. Details by area: Command injection / subprocess isolationThe new Path traversal / out-of-root removal
JSON output / injectionThe only externally-derived value in the unrecognized row is Information disclosureThe
|
# Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
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."
Re-running the job, or pushing a new commit, will retry the review. |
|
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."
Re-running the job, or pushing a new commit, will retry the review. |
This was generated by AI during autonomous /work-items:work-loop execution.
Closes #555
Summary
prune_babysit_worktrees.pyderives each worktree's PR identity from its directory name. Adirectory that did not match
<owner>__<repo>__pr-<number>was dropped byiter_worktreesbefore the report was assembled — not kept, not removed, not an error, simply absent. A caller
reading the JSON to answer "is anything left to clean up?" got an empty list while merged PRs'
worktrees sat on disk, and had to find and
git worktree removethem by hand.Fix shape: the issue's option (b) — report the unmappable directory — not option (a), deriving
identity from
git worktree listbranch/remote metadata. The issue frames these asalternatives; they are not. (b) is a prerequisite of (a):
required whenever the PR branch is locked in a sibling checkout (
reference/worktrees.md), so(a) still needs this fallback for the case the convention exists to cover.
fixed for the worktree's lifetime; git working state is not. A network-derived identity that
disagreed with what the worker wrote would make an active lease invisible and delete a live
worker's worktree.
find_open_prs_for_head_ref, filters to open PRs, and pruning targets merged/closed ones bydefinition — so (a) means new
ghplumbing, against the hermetic regression guard source-control: babysit worktree pruner hard-depends on ghq (undeclared, author-personal repo-layout tool) #438deliberately established.
(a) is therefore not foreclosed by this change and not merely deferred: it is a superset that
would sit on top of this fallback.
What changed:
<worktree-root>now appears in the report. An unmappable one is anaction: unrecognizedrow carrying its path and reason, emitted in every mode including--pr— an unrecognized entry has no key to match a target against, so leaving it to thatfilter would hide it from every scoped run permanently. (A recognized worktree for another PR
is still filtered out under
--pr, as it always has been: it is outside the scope the callerdeclared in
target_prand remains visible in an unscoped run.)checks that authorize removal) and never fail the run, so a stray directory cannot break a dry
run.
reference/worktrees.mdnow states the naming convention that was previously only implied bythe helper's regex — the drift the issue identifies as the root cause — and what becomes of a
directory that breaks it.
Test plan
Run from
plugins/source-control/skills/babysit-prs/scripts/:python -m unittest tests.test_prune_babysit_worktrees -v— 7 tests, OK (3 new, 4pre-existing).
python -m unittest discover -s .../scripts/tests -t .../scripts— 351tests, OK. No regressions.
New coverage (all hermetic — no
gh, no network):test_iter_worktrees_pairs_recognized_and_unrecognized_directories— a root holding both aconforming and a non-conforming directory yields one
Worktreeand oneUnrecognizedWorktree;a loose non-directory file is still ignored.
test_main_reports_a_non_conforming_worktree_instead_of_nothing— the source-control:babysit-prs: prune_babysit_worktrees.py silently skips non-standard worktree names (medley-<N> pattern) #555 regression itself:against a real on-disk git worktree named
medley-1567, the report now carries theunrecognizedrow instead of the pre-fix empty list, the directory survives, exit code 0.test_scoped_pr_mode_still_reports_unrecognized_directories—--pr owner/repo#1still reportsthe unrecognized directory.
Related
N/A
🤖 Generated with Claude Code