Skip to content

fix(source-control): claim unlocked worktrees created outside the helper - #3104

Merged
kyle-sexton merged 1 commit into
mainfrom
cursor/2882-worktree-lock-cfcf
Aug 23, 2026
Merged

fix(source-control): claim unlocked worktrees created outside the helper#3104
kyle-sexton merged 1 commit into
mainfrom
cursor/2882-worktree-lock-cfcf

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Closes #2882

Summary

worktree-create.sh already armed git worktree lock on the trees it created. A plain git worktree add did not, so concurrent sessions reached into those trees with no claim to read. This adds a check that reports unclaimed linked worktrees, a claim path with a session-distinct lock reason, an entry gate that surfaces a foreign live claim, and a PostToolUse hook that locks only the parsed add target after a Bash add, without rewriting helper-created reason strings.

Fix

  • scripts/worktree-claim.sh verbs: report (UNCLAIMED vs CLAIMED), claim / claim --all-unclaimed (session-distinct reason; never rewrites an existing reason; --all-unclaimed is a repair verb and preserves a lock-failure exit status), check-enter (foreign claim → exit 4 with the reason text; unclaimed → exit 3). Relative / . / symlink paths are canonicalized before matching porcelain.
  • hooks/worktree-add-claim-gate.sh is the PostToolUse sibling of the containment gate: after a Bash git worktree add it claims only the parsed add target, composing git -C and wrapper chdirs the same way the containment sibling does. echo git worktree add is not a git call. It does not use --all-unclaimed (two concurrent adds must not assign both trees to whichever hook runs first).
  • Documented check-enter passes --session-id "${CLAUDE_SESSION_ID}" (skill-markdown substitution). The Bash environment does not independently export that id.
  • /source-control:worktree status, audit, create, and cleanup now name the claim gate. source-control 0.55.4 → 0.55.5.

The lock still only prevents worktree remove / move / prune (git-worktree(1)); its value here is as a claim other agents can read.

Verification

From an unrelated cwd (/tmp), against throwaway fixtures:

  • worktree-claim.test.sh — 52/52. Includes check-enter . and a relative child path against a foreign claim (exit 4), and --all-unclaimed preserving a lock-failure status.
  • worktree-add-claim-gate.test.sh — 24/24. Echo does not lock a decoy; a sibling unclaimed tree is not stolen; git -C other-repo claims the other repo, not the payload-cwd tree.
  • nesting-invariant-ssot.test.sh — 18/18.
  • shellcheck -x on the new scripts — clean.
  • scripts/sync-plugin-options-docs.py --check — up to date.

Rebased onto origin/main (keeps the 0.55.4 de-slop changelog; this ships as 0.55.5).

Related

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

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


Security review — no findings

  • Gather context (PR diff, changed files)
  • Run skip-gate check
  • Review changed files for security-relevant issues
  • Adversarially validate any candidate findings
  • Post final findings

Reviewed the security-relevant surface of this PR: worktree-claim.sh and the new PostToolUse hook worktree-add-claim-gate.sh, plus the hooks.json wiring and plugin.json kill-switch addition.

No security issues found. Specifically checked and confirmed safe:

  • No injection surface. All git invocations pass arguments as argv (never through eval or unquoted interpolation into a shell string), so worktree paths and lock reasons parsed from git worktree list --porcelain can't break out into shell execution.
  • Session id is strictly validated (^[A-Za-z0-9._:-]{1,128}$ in worktree-claim.sh:110) before it's embedded in the lock reason or used in the reason_is_ours glob match (worktree-claim.sh:129-132) — no wildcard/glob metacharacters can reach that match, so a foreign session can't spoof ownership via a crafted id.
  • Ownership check is boundary-anchored on "session <sid> since" (not a bare prefix match), which correctly avoids the s1 vs s10 collision the code comments call out.
  • Fail-safe defaults. check-enter treats any lock reason as foreign when no session id can be resolved (no false "claimed" outcome), and worktree-add-claim-gate.sh fails open (exits 0) on missing jq, a missing claim script, or any claim-script error — consistent with this hook being advisory (a claim, not a write-mutex) rather than a blocking gate.
  • Trust boundary is unchanged from sibling hooks. cwd and session_id come from the harness's own PostToolUse payload (same trust model already used by worktree-add-containment-gate.sh), not from untrusted external input — this PR doesn't widen that boundary.
  • git_unlocated() explicitly unsets GIT_DIR/GIT_WORK_TREE/etc. before every -C invocation, preventing outer-repo environment bleed into the target repo resolution.

No pull_request_target/workflow_run usage, no permission-widening, and no unpinned dependencies in this diff — nothing here falls into zizmor's lane either.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

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


Code Review Complete

  • Load /review:code-review skill and skip-gate checks
  • Fetch PR diff (gh pr diff 3104)
  • Review scripts/worktree-claim.sh
  • Review hooks/worktree-add-claim-gate.sh
  • Review test files and doc/skill updates
  • Post findings as inline comments + summary

This PR closes the gap where a plain git worktree add produced an unlocked, unclaimable tree. The design (report/claim/check-enter verbs, a session-distinct reason string, the "never rewrite an existing reason" invariant, and a PostToolUse hook to catch Bash-tool adds) is sound and the exit-code contract is clean. I posted two inline findings:

  1. plugins/source-control/scripts/worktree-claim.sh (lines 204–221, 420–425)find_worktree_index string-compares the caller's path against git's absolute porcelain worktree paths with no canonicalization. Since git accepts relative worktree paths (and this PR's own context/create.md update tells callers to reuse the same, possibly-relative, <path> for a later check-enter <path>), a relative path — or one that only differs via symlink resolution — fails to match, and do_check_enter falls into its "not a worktree path, nothing to consult" branch, silently returning exit 0 (allow). That's a fail-open on exactly the case check-enter exists to catch: entry into a tree carrying a live foreign claim. This is the one I'd want resolved before merge.
  2. plugins/source-control/hooks/worktree-add-claim-gate.sh (lines 76–88) — lower severity: when a batch (claim --all-unclaimed) has one worktree successfully claimed and a different one concurrently claimed by another session in the same run, the hook's message branch only checks for "lane active" in $out first, so the "already carries a live claim" notice for the losing worktree is silently dropped. No safety impact (the foreign reason is never rewritten), just a lost notification in a mixed-outcome batch.

Everything else — the never-rewrite guarantee, the s1/s10 session-anchoring, the helper-reason preservation, the kill switch, the doc/README/CHANGELOG updates, the version bump — checked out from reading the code. The verification numbers in the PR description (45/45, 18/18, etc.) are author-claimed; I did not independently execute the test suites in this sandbox (git subprocess execution is not available to this review lane), so I read the test files instead and they do exercise the stated ACs, just not the relative-path/symlink case above.
· branch cursor/2882-worktree-lock-cfcf

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Last security-reviewed head: ab3278c3a8c90204ad1c0e5d49ff31aedc67400c. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a78d3d3a38

ℹ️ 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".

Comment thread plugins/source-control/skills/worktree/SKILL.md Outdated
Comment thread plugins/source-control/hooks/worktree-add-claim-gate.sh Outdated
Comment thread plugins/source-control/hooks/worktree-add-claim-gate.sh Outdated
Comment thread plugins/source-control/hooks/worktree-add-claim-gate.sh
Comment thread plugins/source-control/scripts/worktree-claim.sh
Comment thread plugins/source-control/scripts/worktree-claim.sh Outdated
Comment thread plugins/source-control/scripts/worktree-claim.sh
Comment thread plugins/source-control/hooks/worktree-add-claim-gate.sh Outdated
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 1m 37s —— View job


Code Review — skipped (no code-relevant changes since last review)

  • Load /review:code-review skill / gather context
  • Fetch PR diff and changed files
  • Diff the new commit against the last-reviewed head
  • Post findings

The only commit since the last review (a78d3d3, reviewed above) is a7418fe, which changes the mode bits on two test files from 100644 to 100755 (worktree-add-claim-gate.test.sh, worktree-claim.test.sh) — zero content diff, confirmed via git show --stat. There's nothing new here to review.

For completeness, here's where the substantive findings from the prior passes stand — all against unchanged code (plugins/source-control/scripts/worktree-claim.sh, plugins/source-control/hooks/worktree-add-claim-gate.sh):

  • Still open, confirmed independently: worktree-claim.sh#L410-L412if ! lock_one ...; then rc=$?; fi. Bash collapses the exit status of a !-negated pipeline to 0/1 before $? is readable, so rc=$? inside that then branch always assigns 0, never lock_one's real code (e.g. 4 for a concurrent foreign claim). do_claim_all therefore always returns 0, so the hook's rc -eq 4 branch (worktree-add-claim-gate.sh:84) is dead code and a losing-race worktree in a batch claim never gets its "already claimed" notice surfaced. (Matches the codex finding on worktree-claim.sh:411 and the prior claude review comment on worktree-add-claim-gate.sh:88.)
  • Still open: find_worktree_index (worktree-claim.sh:204-221) does no path canonicalization, so a relative or symlink-differing path argument to check-enter silently falls through to the "not a worktree, allow" branch (exit 0) even against a foreign-claimed tree — the exact case this gate exists to catch.
  • Still open: worktree-add-claim-gate.sh:56 claims via --all-unclaimed, which locks every currently-unlocked linked worktree in the repo, not just the one the triggering command created — a second session's own concurrently-created worktree gets claimed under the first session's id.
  • Still open: worktree-add-claim-gate.sh:57-58 passes the PostToolUse payload cwd straight through as --repo-dir, which doesn't account for git -C <path> worktree add ... or a cd inside the command — the wrong repo can be targeted.
  • Still open: worktree-add-claim-gate.sh:44-46 gates purely on substring matches (git, worktree, add) against the raw command string, so echo 'git worktree add ...' or a comment/heredoc containing that text also triggers a claim.

None of these are new — they're carried over from the existing review threads on this PR and none of the touched lines changed in a7418fe. Flagging here only so they stay visible if the review-relevance gate resets before they're addressed.

@kyle-sexton kyle-sexton self-assigned this Aug 23, 2026
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

No description provided.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Taking over from expired work-item-lease held by session MELO-LAP-001-triage-202608230449 (1h TTL, no renewal or further activity observed since acquisition; current time 2026-08-23T07:12Z is well past expiry). Proceeding under lane babysit-loop, instance melo-lap-001-merge-20260823T045045Z.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

This was generated by AI during triage.

Triage: verified — rework needed before merge

Checked out the branch (cursor/2882-worktree-lock-cfcf) into an isolated worktree and re-ran the
claimed test suite plus a direct read of the flagged logic. The diff implements the described
claim/report/check-enter mechanism, but it does not fully do what it claims, and it currently has a
merge conflict against main (mergeable: CONFLICTING).

Confirmed independently (not just relaying the bot review threads):

  • worktree-claim.test.sh does not pass 45/45 on this platform — 24 of 45 cases fail here,
    concentrated in exactly the check-enter / foreign-claim scenarios the earlier code-review
    comments flagged: find_worktree_index compares the caller's path against git's porcelain
    worktree paths with no canonicalization (no realpath/symlink resolution), so a path-form
    mismatch falls through to the "not a worktree, allow" branch — a fail-open on the exact case the
    gate exists to catch.
  • Reproduced the do_claim_all return-code bug directly: if ! lock_one ...; then rc=$?; fi
    always assigns rc=0 in bash regardless of lock_one's real exit status (verified with a
    minimal repro: a function returning 4, negated in an if, $? inside then reads back 0).
    So a losing/foreign claim during claim --all-unclaimed is silently swallowed — the
    PostToolUse hook's "already claimed" branch is dead code.
  • --all-unclaimed claims every currently-unlocked linked worktree in the repo when the
    PostToolUse hook fires, not just the one the triggering git worktree add created — a second
    session's own concurrently-created worktree can get claimed under the first session's id.

Agent Brief

Type: Bug
Summary: Finish the worktree-claim mechanism this PR adds so foreign-claim detection and
batch claiming behave as documented, and resolve the merge conflict against main.

Current behavior of the diff:
worktree-claim.sh adds report / claim / claim --all-unclaimed / check-enter verbs, and
worktree-add-claim-gate.sh runs claim --all-unclaimed as a PostToolUse hook after a Bash-tool
git worktree add. As written: (1) check-enter's foreign-claim detection can fail open when the
caller's path isn't in exactly the same textual form git's porcelain output uses (no path
canonicalization before comparison); (2) do_claim_all's bash negation (if ! lock_one; then rc=$?; fi) discards the real exit code of a failed/foreign lock_one call, so batch claims never
report a losing race; (3) the batch-claim hook locks every unclaimed linked worktree repo-wide
rather than scoping to the worktree(s) the triggering command actually created, which can claim
another session's freshly-created (and not-yet-locked) tree under the wrong session id.

Desired behavior:

  • check-enter's path-to-worktree matching resolves both the caller-supplied path and git's
    reported worktree paths to a common canonical form (symlinks and relative-vs-absolute
    differences included) before comparison, so a foreign claim is detected regardless of the path
    form the caller passes.
  • A failed or foreign-claim outcome from an individual worktree lock attempt is preserved through
    batch claiming, so the caller (and the PostToolUse hook) can distinguish "everything claimed
    cleanly" from "one or more worktrees in the batch were already claimed by someone else."
  • The claim gate that fires after a git worktree add only claims the worktree(s) that command
    created, not every currently-unlocked linked worktree in the repository.
  • The branch merges cleanly against current main.

Key interfaces:

  • find_worktree_index() (or its path-matching logic) — needs canonical-path comparison, not raw
    string comparison of caller vs. git-porcelain paths.
  • do_claim_all() — needs to propagate a per-worktree failure/foreign-claim status instead of
    losing it through the ! cmd; then rc=$? bash pattern.
  • The claim-gate hook's invocation of the claim script — needs to target only the newly-created
    worktree path(s) it can determine from the triggering tool call, not --all-unclaimed against
    the whole repo.

Acceptance criteria:

  • worktree-claim.test.sh passes in full (including the foreign-claim / check-enter cases)
    when the target path is passed in a different form (relative, or through a symlink) than
    git's porcelain output uses.
  • A test demonstrates that when a batch claim attempt includes a worktree already locked by
    another session, the caller/hook receives a non-zero / foreign-claim signal for that
    worktree rather than silent success.
  • A test demonstrates the PostToolUse hook does not lock a worktree it did not itself create
    (e.g., a pre-existing unclaimed worktree from another session is left untouched by a
    different session's git worktree add).
  • The PR branch merges cleanly against current main.
  • shellcheck -x stays clean on the touched scripts.

Out of scope:

Priority set to priority: medium (assessed-default tier — no blocking/deadline signal found).
Work-class set to scoped — the fix is bounded to path-comparison, exit-code propagation, and
claim-gate scoping inside the already-touched files, but the wrong result on the foreign-claim
path is safety-adjacent (it's the failure mode the PR exists to prevent), so this isn't
mechanical.

@kyle-sexton kyle-sexton added priority: medium Real value, no hard deadline; normal backlog flow. work-class: scoped A briefed fix or small feature; blast radius bounded by the brief, tests exist. agent-ready Fully specified and briefed; eligible for autonomous pickup from the frontier. labels Aug 23, 2026
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Genuine merge conflict, confirmed via git merge-tree against current origin/main: plugins/source-control/.claude-plugin/plugin.json, plugins/source-control/CHANGELOG.md, plugins/source-control/skills/worktree/SKILL.md, plugins/source-control/skills/worktree/context/audit.md (and likely one or more of cleanup.md/create.md/status.md in the same dir, not fully enumerated). Root cause: several PRs landed on main since this branch was cut (e.g. #3116, #3165), bumping plugin.json's version and touching the same worktree skill files this PR also edits.

Note this PR also overlaps files with sibling PR #3108 (cursor/2891-deslop-shard4-cfcf) in this same batch, both touch plugins/source-control/skills/worktree/SKILL.md and the plugin's plugin.json/CHANGELOG.md, so whichever of the two merges first will reshape the other's conflict. Recommend resolving and merging one at a time, freshening the second afterward.

This session's git push capability to a PR branch is currently blocked by the harness permission layer, so no local resolution was pushed. Escalating for a human or push-capable session: git fetch origin && git checkout cursor/2882-worktree-lock-cfcf && git merge origin/main and resolve the conflicting files above.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

🔧 Lane claim — melo-lap-001-merge-20260823T045045Z (conflict-batch coordinator)

Claiming this PR to diagnose and attempt conflict resolution (/source-control:resolve-conflicts) against current main. Will push if resolution succeeds, or report the specific blocker if not. No merge will be performed by this pass.

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — skipped (no new security-relevant changes since last review)

  • Gather context (PR diff, changed files, custom instructions)
  • Invoke /review:security-review skill instructions
  • Diff current head against last-reviewed head
  • Confirm skip-gate condition
  • Post result

Skip gate hit: condition 3 — "this head already has a successful security review that still applies."

The security-relevant files in this PR — worktree-claim.sh, worktree-add-claim-gate.sh, and hooks.json — are byte-for-byte identical at the current head (495e395) to what they were at a78d3d3 (git diff a78d3d3a38e..495e3953f -- <those three files> is empty), which already received a clean security review (no findings) on 2026-08-21.

Everything that changed between a78d3d3 and 495e395 is a merge of origin/main into this branch, bringing in unrelated upstream commits (docs-hygiene, work-items, guardrails fixes, etc.) plus one no-op mode-bit commit already covered by a prior pass. Within this PR's own 13-file diff, the only content deltas since the last review are:

  • plugin.json — version bump (0.54.170.55.4) and cosmetic Unicode-escape normalization pulled from other merged PRs; the worktree_add_claim_gate_enabled option block itself is unchanged.
  • CHANGELOG.md / README.md / SKILL.md / context/audit.md / context/cleanup.md — merged-in release notes and docs for an unrelated feature (worktree cleanup's plugin-install-record reaping, fix(source-control): worktree teardown leaves project-scope plugin install records forever #3113) that landed on main via other PRs, not new content from this PR.

None of this touches the claim/lock logic, trust boundaries, or hook wiring reviewed previously, so there's no new security-relevant surface to assess. The prior findings (no security issues in the claim/lock mechanism) still stand.

Note: the correctness/logic issues raised in code review (path canonicalization in find_worktree_index, the do_claim_all exit-code bug, --all-unclaimed batch-claim scoping) are tracked separately in the code-review threads and the 2026-08-23 triage/agent-brief comment on this PR — those are correctness concerns, not in scope for this security lane.
· branch cursor/2882-worktree-lock-cfcf

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — skipped (no code-relevant changes since last review)

  • Load /review:code-review skill and skip-gate checks
  • Diff last-reviewed head (a7418fe) against current head (495e395)
  • Confirm the merge resolution itself is clean
  • Post findings

The only thing between the last-reviewed head (a7418fe) and the current head (495e395) is a merge of origin/main into this branch. git diff a7418fe 495e395 -- plugins/source-control/scripts/worktree-claim.sh plugins/source-control/hooks/worktree-add-claim-gate.sh is empty — those two files (and their test files) are byte-identical to what was already reviewed. Everything that changed is either merge-only noise from unrelated main PRs (outside this PR's scope) or the conflict resolution itself in files this PR touches: plugin.json (version 0.54.170.55.4, correctly renumbered above main's published 0.55.3), CHANGELOG.md (this PR's 0.55.4 entry placed above main's 0.55.00.55.3 entries, nothing dropped), and SKILL.md/context/audit.md (this PR's claim-liveness content composed alongside main's orphaned-plugin-install-record content). I checked all three for correctness: no leftover <<<<<<</=======/>>>>>>> markers anywhere under plugins/source-control/, the version/changelog ordering is consistent, and hooks.json's wiring for worktree-add-claim-gate.sh is intact. Nothing new to flag there.

Since the code itself is unchanged, the substantive findings from the prior passes on this PR still stand, unaddressed, at the same lines on 495e395 (this also matches the 2026-08-23 triage comment's independent repro):

  • worktree-claim.sh#L204-L221 / #L420-L425find_worktree_index string-compares the caller's path against git's absolute porcelain paths with no canonicalization. A relative path, a .-relative path, or one that only differs via symlink resolution fails to match, so check-enter falls into "not a worktree, allow" and silently returns exit 0 for a tree that in fact carries a live foreign claim — the exact case this gate exists to catch.
  • worktree-claim.sh#L410-L412if ! lock_one ...; then rc=$?; fi reads back 0/1 (bash's negated-pipeline collapse), never lock_one's real exit status, so do_claim_all always reports success even when a worktree in the batch was already claimed by another session.
  • worktree-add-claim-gate.sh#L56--all-unclaimed claims every currently-unlocked linked worktree in the repo, not just the one the triggering command created; a second session's own concurrently-created worktree can be claimed under the first session's id.
  • worktree-add-claim-gate.sh#L58 — the payload cwd is passed straight through as --repo-dir, which doesn't account for git -C <path> worktree add or a cd inside the command.
  • worktree-add-claim-gate.sh#L44-L46 — the trigger is a raw substring match (git, worktree, add) against the command string, so echo 'git worktree add ...' or a comment/heredoc containing that text also fires a claim.

Not re-filing these as new inline comments — they're already anchored on this thread from the prior passes (Codex and Claude, 2026-08-21) and independently reproduced in the 2026-08-23 triage comment, which also opened an Agent Brief with concrete acceptance criteria for fixing them. Flagging here only so they stay visible against the current head.

One thing worth noting for whoever picks up the fix: the 2026-08-23 07:42 comment reports a genuine merge conflict against main in plugin.json/CHANGELOG.md/SKILL.md/context/audit.md — that conflict appears to be exactly what commit 495e395 (the current head) resolves, and the resolution itself checks out clean per the review above.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Taking over from expired lane-claim held by instance melo-lap-001-merge-20260823T045045Z (expired 2026-08-23T08:12:36Z; current time 2026-08-23T18:29:11Z is well past expiry, no renewal or activity observed). Proceeding under lane babysit-loop, instance MELO-LAP-001-merge-202608231413-w3104.

@cursor
cursor Bot force-pushed the cursor/2882-worktree-lock-cfcf branch from 495e395 to efa532e Compare August 23, 2026 19:12
@kyle-sexton
kyle-sexton force-pushed the cursor/2882-worktree-lock-cfcf branch from efa532e to 6746622 Compare August 23, 2026 19:12
@kyle-sexton kyle-sexton reopened this Aug 23, 2026
@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — in progress

  • Gather context (PR diff, changed files, custom instructions)
  • Invoke /review:security-review skill instructions
  • Diff current head (ab3278c) against last-reviewed head (495e395) — real content changes to worktree-claim.sh and worktree-add-claim-gate.sh, so the skip gate does not apply; performing a full review
  • Verify a candidate finding (hook script file-mode regression) against the hook execution contract
  • Post result

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Releasing lane-claim MELO-LAP-001-merge-202608231413-w3104 — stopping this cycle without pushing or merging.

While preparing fixes for the 8 open review threads, the PR head SHA changed mid-task (pinned 495e3953f3b80a48cdef22c7fc3ce5a0ff46369c -> observed ab3278c3a8c90204ad1c0e5d49ff31aedc67400c, new commit titled "fix(source-control): claim unlocked worktrees created outside the helper", authored 2026-08-23T19:12:52Z) — an update that appears to already address the same findings (targeted PostToolUse claim instead of --all-unclaimed, check-enter path canonicalization, explicit --session-id). Per this lane's safety rule, local work is discarded rather than pushed on top of a moved head. This PR needs a fresh babysit-loop cycle to re-evaluate the current head against the 8 open review threads.

@cursor
cursor Bot force-pushed the cursor/2882-worktree-lock-cfcf branch from bb71af1 to 7ba0b3e Compare August 23, 2026 19:40
@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 3m 55s —— View job


Code Review Complete

  • Load /review:code-review skill and skip-gate checks
  • Diff origin/main...HEAD (bb71af1) for plugins/source-control/**
  • Re-read worktree-claim.sh and worktree-add-claim-gate.sh in full against the prior findings
  • Check test coverage (worktree-claim.test.sh, worktree-add-claim-gate.test.sh) for each fixed case
  • Check docs/CHANGELOG/README/plugin.json consistency
  • Post findings

No new findings. This head (bb71af1, on top of ab3278c) genuinely fixes every substantive issue raised across the earlier review passes on this PR. I read the code independently rather than relying on the author's summary — here's what I verified line-by-line:

  • Path canonicalizationworktree-claim.sh:120-142 (canonicalize_path) now resolves relative/./symlink paths (via pwd -P when the path exists) before both the porcelain-path parse (parse_worktrees) and the caller-supplied target (find_worktree_index). check-enter . and a relative child path against a foreign claim now correctly exit 4 instead of silently allowing — this closes the fail-open the earlier Codex/Claude threads flagged, and is exercised by worktree-claim.test.sh lines 109-122.
  • do_claim_all exit-code bugworktree-claim.sh:466-486 now captures lock_one's status directly (lock_one ...; st=$?) instead of the old if ! lock_one; then rc=$?; fi pattern that always read back 0. A comment at L473-475 explicitly documents why the old form was wrong. worktree-claim.test.sh lines 171-186 reproduce a real lock failure (chmod on the worktree admin dir) and assert a non-zero return survives the batch.
  • Batch-claim over-scoping — the hook no longer calls claim --all-unclaimed at all. worktree-add-claim-gate.sh parses the actual executed git worktree add command (reusing the containment gate's hook::bash_parse_segments / hook::git_resolve_index / hook::git_resolve_subcommand) and claims only the resolved target path(s). --all-unclaimed is now documented and used solely as an explicit repair verb. Verified against worktree-add-claim-gate.test.sh lines 119-127 (sibling unclaimed tree is not stolen) and 129-138 (git -C <other-repo> claims in the right repo, not the payload cwd).
  • cwd vs. actual target repo — same fix as above; collect_add walks -C flags and wrapper chdirs to compute base before resolving the target, rather than trusting the PostToolUse payload cwd directly.
  • Substring pre-filter (echo git worktree add ...) — the three substring checks at worktree-add-claim-gate.sh:45-47 are only a cheap prefilter now; the actual claim only fires after collect_add finds a real, executed git worktree add via the shared parser. echo git worktree add ... populates no CLAIM_TARGETS and is a no-op — worktree-add-claim-gate.test.sh lines 62-68.
  • Mixed-outcome batch notification droppedworktree-add-claim-gate.sh:222-262 now tracks claimed_any / foreign_any independently per target and composes both notices into additionalContext when a run has both outcomes, instead of the old single if/elif that could drop one.
  • Missing session id in the documented check-enter invocationSKILL.md now passes --session-id "${CLAUDE_SESSION_ID}" explicitly, with an explicit warning not to guess an id if the token survives unexpanded.

Also checked and clean:

  • No leftover merge-conflict markers anywhere under plugins/source-control/ (the branch's earlier CONFLICTING state against main is resolved on this head).
  • CHANGELOG.md, README.md (including the new worktree_add_claim_gate_enabled options-table row), and the context/*.md skill docs all match what the code actually does — no stale claims.
  • hooks.json wires the new PostToolUse hook on Bash correctly; plugin.json adds the worktree_add_claim_gate_enabled kill switch and bumps to 0.55.5 consistent with the changelog entry.
  • find_worktree_index's prefix match uses a /-boundary check ("$wt"/*), so it doesn't false-positive on sibling paths sharing a prefix (e.g. /tmp/foo vs /tmp/foobar).

Not independently verified in this sandbox: I could not execute worktree-claim.test.sh, worktree-add-claim-gate.test.sh, or shellcheck here — this review lane's Bash access is scoped to read-only git/gh inspection, and both a git fetch and a shellcheck invocation were rejected by the harness's approval gate. The 52/52, 24/24, and clean-shellcheck numbers in the PR description and in kyle-sexton's efa532e0/ab3278c comments are author-claimed; I instead read the test files in full (worktree-claim.test.sh, worktree-add-claim-gate.test.sh) and confirmed the assertions actually exercise the fixed code paths described above, rather than just trusting the pass counts.

No blocking findings on this head.
· branch cursor/2882-worktree-lock-cfcf

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 3m 1s —— View job


Code Review

  • Load /review:code-review skill and skip-gate checks
  • Diff current head (7ba0b3e) against the last-reviewed content (ab3278c / 495e395)
  • Re-read worktree-claim.sh and worktree-add-claim-gate.sh in full against the five findings raised across earlier passes
  • Cross-check the accompanying test files exercise each fix
  • Post findings

Skip gate: does not apply cleanly — git diff ab3278c 7ba0b3e -- plugins/source-control/scripts/worktree-claim.sh plugins/source-control/hooks/worktree-add-claim-gate.sh (plus their test files) is empty, so the code is unchanged since ab3278c, but no code-review-lane pass had actually completed against that content (the 19:14 security-review attempt on ab3278c was released mid-task before finishing, and the last completed code-review pass was against the stale 495e395). So this is a fresh full read of the current head rather than a re-affirmation of a prior pass.

I independently read (not just diffed) plugins/source-control/scripts/worktree-claim.sh and plugins/source-control/hooks/worktree-add-claim-gate.sh at the current head and checked each of the five findings raised by Codex and the earlier Claude passes on this PR:

  1. Path canonicalization for check-enterworktree-claim.sh#L120-L142 adds canonicalize_path (lexical ./../// collapse + pwd -P when the path exists), applied to both sides of the match: porcelain paths in parse_worktrees (L239) and the caller's path before find_worktree_index (L260, L375-377). worktree-claim.test.sh cases 23-28 exercise check-enter . and a relative child path against a foreign claim (exit 4) and against the owning session (exit 0) — this fix looks correct.
  2. do_claim_all swallowing lock_one's exit statusworktree-claim.sh#L466-L486 now calls lock_one unnegated and reads st=$? directly (the old if ! lock_one; then rc=$?; fi collapsed to 0/1). The test's chmod a-w on the worktree admin dir to force a lock failure and assert a non-zero --all-unclaimed exit (worktree-claim.test.sh L171-186) is a good regression guard for exactly this bug class.
  3. --all-unclaimed claiming every unlocked tree repo-wide from the PostToolUse hook — the hook no longer calls --all-unclaimed at all. worktree-add-claim-gate.sh#L226-L247 iterates CLAIM_TARGETS, calling claim <target> per parsed target. --all-unclaimed remains only as an explicit repair verb on the script itself. worktree-add-claim-gate.test.sh L119-127 confirms a sibling unclaimed tree is left alone.
  4. --repo-dir resolution not accounting for git -C/wrapper chdirscollect_add() in the hook (L121-L215) now composes hook::git_resolve_index's wrapper-dir list and a -C scan before the worktree subcommand, the same shared helpers worktree-add-containment-gate.sh already uses. worktree-add-claim-gate.test.sh L129-138 (git -C <other-repo>) confirms the target is claimed in the other repo, not the payload cwd.
  5. Substring pre-filter (echo 'git worktree add ...') manufacturing a claim — the git/worktree/add substring checks at L45-47 are now only a cheap pre-filter; the actual claim only fires after hook::bash_parse_segments + hook::git_resolve_index confirm an executed git worktree add (shared parser already used by the containment gate, unchanged by this PR). worktree-add-claim-gate.test.sh L62-68 confirms echo git worktree add ... does not lock a decoy tree.

Also spot-checked the previously-flagged SKILL.md gap: check-enter at SKILL.md#L140 now passes --session-id "${CLAUDE_SESSION_ID}", with a note explaining the Bash environment doesn't independently export that id.

Beyond the five carried-over findings, nothing new stood out in worktree-claim.sh, the hook, hooks.json's wiring, plugin.json's new worktree_add_claim_gate_enabled option, or the CHANGELOG/README updates — version bump (0.55.5) and changelog entry are consistent, and no leftover merge-conflict markers.

Not independently verified: I could not execute worktree-claim.test.sh / worktree-add-claim-gate.test.sh / shellcheck myself — this review lane's Bash access is scoped to gh pr diff/view/comment/review only, so I read the test files (summarized above) rather than running them. The 52/52 and 24/24 pass counts in the PR description, and the merge-conflict resolution report earlier in the thread, are author-claimed and unverified by me. Static reading of the test files shows they do target the right scenarios (relative/./symlink check-enter, lock-failure preservation, echo-decoy, sibling-tree, git -C), which is as much as this lane can confirm.

No blocking findings from this pass. The five findings raised by Codex and earlier Claude reviews on this PR all read as correctly addressed at the current head.
· branch cursor/2882-worktree-lock-cfcf

Claim non-helper worktrees with a session-distinct lock reason. The
PostToolUse hook claims only the parsed git worktree add target. check-enter
canonicalizes relative paths and takes --session-id.

Closes #2882

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@cursor
cursor Bot force-pushed the cursor/2882-worktree-lock-cfcf branch from 7ba0b3e to 9b5c0ad Compare August 23, 2026 19:54
@kyle-sexton
kyle-sexton merged commit a1cb3b3 into main Aug 23, 2026
50 checks passed
@kyle-sexton
kyle-sexton deleted the cursor/2882-worktree-lock-cfcf branch August 23, 2026 20:05
kyle-sexton added a commit that referenced this pull request Aug 23, 2026
…55.6)

Rebase the --keep-data uninstall guidance onto current main as 0.55.6 so it
does not collide with the 0.55.5 worktree-claim landing from #3104 / #2882.

Closes #3131
kyle-sexton added a commit that referenced this pull request Aug 23, 2026
…0.55.6) (#3211)

Closes #3131

## Summary

`/source-control:setup`'s babysit-config section warned operators not to
uninstall the plugin in order to reconfigure it, citing the
`pluginConfigs` entry that an uninstall drops — and stopped there.
`--keep-data` appeared nowhere in the plugin (`grep -rn 'keep-data'
plugins/source-control/` returned zero hits before this change).

That left a real gap for the operator who uninstalls for one of the
*other* legitimate reasons — troubleshooting, changing scopes,
reinstalling a version. Uninstalling from the last remaining scope
deletes `${CLAUDE_PLUGIN_DATA}` by default, and this plugin keeps
durable state there.

## Fix

One paragraph added to `skills/setup/SKILL.md`'s "Babysit config"
section, after the reconfigure bullets. It names the flag, says what the
directory holds, and — the part that took the most care — states the
resolution rung each worktree root must fall through before it lands
there.

**What the directory actually holds**, verified in-tree rather than
assumed:

| Path | Contents | Relocatable? |
|---|---|---|
| `${CLAUDE_PLUGIN_DATA}/state/babysit-prs` | queue state
(`babysit_state.py`, `queue-state.json`), worker leases
(`manage_babysit_lease.py`), feedback ledger
(`manage_feedback_ledger.py`) | No `userConfig` key relocates it |
| `${CLAUDE_PLUGIN_DATA}/worktrees` | babysit worktrees;
`/source-control:worktree` trees | Only at the last resolution rung —
see below |

**"Left unset" is necessary but not sufficient for `worktree_root`.**
`scripts/worktree-create.sh:20-32` resolves a root over five rungs, and
the plugin option is rung 3, below the target repository's
`melodic.worktreeroot` git config at rung 2. So
`/source-control:worktree create` lands in the data directory only when
*neither* resolves. `babysit_worktree_root` is the simpler case and does
fall back there whenever it is unset. Stating only "unset" would have
over-warned the operator who uses the recommended per-repo git key, and
the paragraph now distinguishes the two.

**The emphasis is on the losses that are actually irrecoverable.**
Babysit's own worktrees are ephemeral scratch that rebuild from GitHub;
the state directory and a `/source-control:worktree` tree holding
uncommitted work are not. An earlier draft had this backwards.

Follows `docs/conventions/plugin-data-report-keying/README.md` Rule 4
("state the uninstall fragility where the artifact is the only copy").

## Verification

| Gate | Result |
|---|---|
| `claude plugin validate plugins/source-control` | Validation passed |
| `scripts/check-changed-skills.sh` (setup) | PASS — 0 errors, 1
pre-existing soft-target warning |
| `scripts/check-changelog-parity.sh` `--check` / `--check-bump` /
`--check-order` / `--check-preserved` | PASS |
| `scripts/sync-plugin-options-docs.py --check` | up to date |
| `scripts/check-skill-count-claims.sh --check` | PASS |
| `scripts/check-skill-leaf-names.sh --check` | PASS |
| `scripts/check-cross-plugin-source-drift.sh --check` | PASS |
| `markdownlint-cli2` (both changed markdown files) | 0 issues |
| `typos` | clean |

**Doc claims re-fetched at rung 1, not taken from a repo snapshot.**
`curl https://code.claude.com/docs/en/plugins-reference.md` on
2026-08-23, 108305 bytes: *"By default, uninstalling from the last
remaining scope also deletes the plugin's `${CLAUDE_PLUGIN_DATA}`
directory. Use `--keep-data` to preserve it"*, and the `plugin
uninstall` flag table carries `--keep-data` spelled exactly that way.

**Rebased twice onto a moving `main`; now at 0.55.6.** `main` landed
#3108 during this work, which de-slopped every source-control
instruction surface under the repo's zero-tolerance em dash policy and
took `0.55.4`; the worktree-claim change from #3104 / #2882 then took
`0.55.5`. The added prose is written em dash free to match the rewritten
file it lands in (`grep -c '—'` over the changed `SKILL.md` returns 0).
Left unrebased, the version collision would have failed
`check-changelog-parity.sh` and the prose would have silently
reintroduced the marks #3108 had just removed.

**Independent review.** A fresh-context reviewer checked the diff
against the issue's acceptance criteria with the author's rationale
withheld. It raised one blocker (the `worktree_root` conditional was
false in both directions — it over-warned past `melodic.worktreeroot`
and under-warned by omitting `/worktree create` trees from the
consequence) and two should-fixes (state described as "lane and lease
state", which misattributes loop-lane telemetry that actually lives in a
GitHub tracking issue and survives an uninstall; and a "neither has
another copy" claim that contradicted the plugin's own
`reference/worktrees.md`). All three are fixed above; the reviewer's
nits on length and antecedent are applied too.

## Related

- Closes #3131 — F3 from the `/plugin-quality:audit` packet. The other
findings in that packet are out of scope here and tracked separately.
- Refs #3128 — F1 from the same run, shared with `work-items`.
- Refs #3212 — **filed from this work, and materially more dangerous
than the gap this PR closes.**
`scripts/reap-project-plugin-records.sh:238` runs `claude plugin
uninstall "$id" -s project` without `--keep-data`, non-interactively,
for every plugin id with a project-scope record keyed to a worktree
being torn down (`/source-control:worktree cleanup` Step 4b). For any
plugin whose project-scope record was its last remaining scope, that
call deletes *that* plugin's `${CLAUDE_PLUGIN_DATA}`. Verified directly
against the script. It is other plugins' data and a code change rather
than prose, so it sits outside #3131's prose-only acceptance criteria
and is tracked on its own rather than folded in here.
- **One adjacent surface deliberately left alone.**
`plugins/source-control/README.md:337-338` carries a parallel uninstall
warning, but it sits inside the `BEGIN GENERATED` block emitted by
`scripts/sync-plugin-options-docs.py:100-101` and shared verbatim by
every plugin README in the marketplace — a hand-edit is clobbered on the
next sync, and editing it here would have been the mistake. Adding
`--keep-data` to the generator is a one-line change that fixes roughly
forty READMEs at once; it is left out of this PR because it changes
every plugin's README rather than source-control's, not because it needs
a decision.

---
*Generated by [Claude
Code](https://claude.ai/code/session_01XtbWChCVfUWAv1Pi5Qk2hA)*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-ready Fully specified and briefed; eligible for autonomous pickup from the frontier. priority: medium Real value, no hard deadline; normal backlog flow. work-class: scoped A briefed fix or small feature; blast radius bounded by the brief, tests exist.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

source-control: worktrees created outside worktree-create.sh carry no lock, so concurrent sessions reach into each other's trees with no claim to check

2 participants