diff --git a/.claude/skills/implement-issue/SKILL.md b/.claude/skills/implement-issue/SKILL.md index 6d12c83d..0cf306ac 100644 --- a/.claude/skills/implement-issue/SKILL.md +++ b/.claude/skills/implement-issue/SKILL.md @@ -319,21 +319,41 @@ next person to do issue work cleans up the last one's mess automatically. # looks unmerged forever. PR state is the only authoritative signal. merged=$(gh pr list --state merged --limit 200 --json headRefName -q '.[].headRefName') git worktree list | awk 'NR>1 {print $1}' | while read -r wt; do + # Directory gone = PHANTOM (`prunable`), not detached. `git -C` would fail + # and the detached guard below would silently swallow it. + [ -d "$wt" ] || { echo "PHANTOM (needs unsandboxed prune): $wt"; continue; } b=$(git -C "$wt" branch --show-current 2>/dev/null) [ -n "$b" ] || continue # detached: leave alone echo "$merged" | grep -qx "$b" || continue # not merged: leave alone - if [ -n "$(git -C "$wt" status --porcelain -uno)" ]; then # tracked edits: never auto-delete - echo "KEEP (uncommitted changes): $wt"; continue + dirty=$(git -C "$wt" status --porcelain -uno) + if [ -n "$dirty" ]; then # tracked edits: never auto-delete + if [ -z "$(printf '%s\n' "$dirty" | grep -v '^ D ')" ]; then + echo "CARCASS (failed prune, deletions only): $wt" # see below -- not real edits + else + echo "KEEP (uncommitted changes): $wt" + fi + continue fi - git worktree remove "$wt" && git branch -D "$b" + echo "PRUNE: $wt ($b)" # report; do NOT remove here done git fetch origin --prune ``` -Two guards that matter: never remove a worktree with **uncommitted tracked +**Report the `PRUNE` list; do not act on it from here.** `git worktree remove` +is sandbox-denied — it deletes the working tree *first* and then fails on the +`.git/worktrees/` unlink, destroying ~393 tracked files and leaving a +carcass that no later prune can clear (`git worktree prune` is denied too). +Emit one `!`-prefixed command covering every `PRUNE`, `CARCASS` and `PHANTOM` +for the maintainer to paste, exactly as `sweep-prs` Step 3 does. See +`docs/agents/local-agent-environment.md`, "git worktree remove is denied too". + +Three guards that matter: never remove a worktree with **uncommitted tracked changes** — report it and let a human decide (one such worktree held a -375-line module that existed nowhere else) — and never touch a **detached or -locked** worktree, which is usually another agent's live session. +375-line module that existed nowhere else); never touch a **detached or +locked** worktree, which is usually another agent's live session; and never +mistake a **carcass** for either. A dirty set that is *entirely* ` D` lines is +this bug's own wreckage, not someone's work — 13 of them accumulated before +anyone read the diff. Then `git fetch origin main` — `using-git-worktrees`' git fallback branches from the current local `HEAD`, not `origin/main`, so a stale local checkout @@ -747,10 +767,26 @@ net is upstream, not this section. something's wrong. 2. Remove the worktree — via `ExitWorktree action=remove discard_changes=true` - if the session is still in it, or `git worktree remove ` from the - main repo root for a `.worktrees/`-created one. + if the session is still in it. That is the harness doing it, so it is not + sandboxed and it works. + + If the session has already left, **emit one `!`-prefixed command that + removes the worktree and force-deletes the branch together**, in that + order — the branch delete has to ride the same deferred command: git + refuses `git branch -D` while the worktree registration persists, and the + command below is what clears the registration: + + ```bash + # Emit this; do not execute it. It must run unsandboxed. + git worktree remove --force && git branch -D + ``` + + Running `git worktree remove` from a sandboxed Bash half-deletes the + worktree and then fails (see Step 4), so the agent must not run it either. -3. Force-delete the local branch and prune stale remote refs: +3. In-session only — when item 2 completed via `ExitWorktree`, the + registration is gone and `git branch -D` is safe. Force-delete the local + branch and prune stale remote refs: ```bash git branch -D diff --git a/.claude/skills/sweep-prs/SKILL.md b/.claude/skills/sweep-prs/SKILL.md index 7517b701..6950b31d 100644 --- a/.claude/skills/sweep-prs/SKILL.md +++ b/.claude/skills/sweep-prs/SKILL.md @@ -91,16 +91,30 @@ merged=$(gh pr list --state merged --limit 200 --json headRefName -q '.[].headRe owned=$(cd ~ && claude agents --json 2>/dev/null | jq -r '.[].cwd') git worktree list | awk 'NR>1 {print $1}' | while read -r wt; do + # A registration whose directory is gone is a PHANTOM, not a detached HEAD: + # `git -C` fails, so $b comes back empty and the detached branch below would + # swallow it. `git worktree list` tags these `prunable`. + [ -d "$wt" ] || { echo "PHANTOM (prunable, needs unsandboxed prune): $wt"; continue; } b=$(git -C "$wt" branch --show-current 2>/dev/null) [ -n "$b" ] || { echo "SKIP (detached): $wt"; continue; } if echo "$owned" | grep -qF "$wt"; then echo "SKIP (live session): $b"; continue fi - if [ -n "$(git -C "$wt" status --porcelain -uno)" ]; then - echo "SKIP (uncommitted changes): $b"; continue + dirty=$(git -C "$wt" status --porcelain -uno) + if [ -n "$dirty" ]; then + # A worktree whose dirty set is ENTIRELY deletions is not someone's work + # in progress -- it is a carcass left by a `git worktree remove` that the + # sandbox killed halfway (see below). Say so, or the sweep reports its own + # wreckage back as a backlog of stranded edits. + if [ -z "$(printf '%s\n' "$dirty" | grep -v '^ D ')" ]; then + echo "CARCASS (failed prune, $(printf '%s\n' "$dirty" | grep -c .) deletions): $wt ($b)" + else + echo "SKIP (uncommitted changes): $b" + fi + continue fi if echo "$merged" | grep -qx "$b"; then - echo "PRUNE: $b"; continue + echo "PRUNE: $wt ($b)"; continue fi age=$(( ($(date +%s) - $(git -C "$wt" log -1 --format=%ct)) / 60 )) if [ "$age" -lt 30 ]; then @@ -124,15 +138,49 @@ done ### 3. Act -**`PRUNE`** — the PR merged: +**`PRUNE`** — the PR merged. **Do not run `git worktree remove` yourself.** +The sandbox denies the `.git/worktrees/` unlink that removal ends with, +and removal deletes the working tree *before* it gets there, so a run from +here does not fail cleanly — it destroys ~393 tracked files and leaves a +carcass that can never be pruned again. `git worktree prune` is denied by the +same unlink **and exits 0 while failing**, so there is no in-sandbox recovery +either — and no exit status you can trust. This is not a hypothetical: three +sweeps did exactly that to 13 worktrees before it was diagnosed. See +`docs/agents/local-agent-environment.md`, "git worktree remove is denied too". + +Collect every `PRUNE`, `CARCASS` and `PHANTOM` instead, and emit **one** +command for the maintainer to paste with a `!` prefix, which runs unsandboxed: ```bash -git worktree remove "$wt" && git branch -D "$b" +# Emit this; do not execute it. It must run from a NON-worktree-isolated +# session -- an isolated one refuses the `cd` to the shared checkout. +cd /Users/johanzander/GitHub/bess-manager && for wt in ; do + b=$(git -C "$wt" symbolic-ref --short HEAD 2>/dev/null) + git worktree remove --force "$wt" + [ -n "$b" ] && git branch -D "$b" +done; git worktree prune; git worktree list | wc -l ``` -Force-delete is expected, not a warning sign: squash-merge means the -branch's commits never become reachable from `main`, so `git branch -d`'s -ancestry check always refuses. +`` is the space-separated list of full worktree paths from the reported +`PRUNE`/`CARCASS`/`PHANTOM` lines. Report the path, not the branch name: `git +worktree list` yields paths, and a sibling worktree (`../bess-manager-feature/`) +does not live under `.claude/worktrees/`, so the branch name alone cannot +reconstruct the path to remove. + +The trailing `git worktree prune` is what clears any `PHANTOM`, whose +directory is already gone so `remove` has nothing to work with. `[ -n "$b" ]` +rather than `&&` because a phantom yields no branch name. + +`--force` is required for a `CARCASS` (its own damage reads as uncommitted +changes) and harmless for a clean `PRUNE`. Force-deleting the *branch* is +expected too, and is a separate thing: squash-merge means the branch's commits +never become reachable from `main`, so `git branch -d`'s ancestry check always +refuses. + +Before listing a `CARCASS`, confirm its branch is genuinely spent — the PR +merged, and any commits past the merged head are already in `origin/main`. +A carcass has no recoverable working-tree content by definition (deletions +only), but the *branch* may still hold commits that never landed. **`OPEN`** — in report-only mode (the default), print the row from the table below that this PR matches and take no action. In `--all` mode, or when the diff --git a/docs/agents/local-agent-environment.md b/docs/agents/local-agent-environment.md index 7752ab4a..eb33df01 100644 --- a/docs/agents/local-agent-environment.md +++ b/docs/agents/local-agent-environment.md @@ -52,11 +52,14 @@ frontend/node_modules` first) rather than installing through it. Re-running **An agent should run end-to-end without approving anything that the sandbox already contains.** Prompts are the cost, not the safety: a stalled autonomous run is a guaranteed loss, while anything the sandbox bounds is recoverable. -`rm`, `git reset --hard`, `rebase`, `merge`, `git branch -D`, `git worktree -remove` all run unattended. **`git push` also runs unattended, in every -spelling including `--force`** — not because pushing is safe, but because the -refs worth protecting are protected on GitHub itself. See "Pushing is guarded -server-side" below. +`rm`, `git reset --hard`, `rebase`, `merge`, `git branch -D` all run +unattended. **`git worktree remove`/`prune` are the one exception to that +list** — the sandbox does not make them safe, it makes them destructively +unsafe (see "What stays denied" below): they delete the working tree before +failing on the denied `.git/worktrees/` unlink. **`git push` also runs +unattended, in every spelling including `--force`** — not because pushing is +safe, but because the refs worth protecting are protected on GitHub itself. See +"Pushing is guarded server-side" below. What still asks is one closed list, and every entry is there because the **sandbox cannot contain it** — it bounds the filesystem, not the network, and @@ -83,7 +86,7 @@ deletion, which is a real bypass. When the choice is between a false prompt and a hole, take the prompt. The one case where that trade flips is `deny`, which has no override: an over-broad deny **blocks** documented work rather than prompting for it, which is why `git prune` has no twin (it caught `git worktree prune`, -and Step 4 prunes in a loop) and why the stash twins are per-verb. +which the Step 4 loop used to run) and why the stash twins are per-verb. Denied outright: the shared podman VM (`machine rm`, `system reset`) and every mutating `git stash` form, **including `git -C stash …`**. That last @@ -327,6 +330,11 @@ must match **nothing**. from the actual syscall rather than guessed from a command string. That is why `rm -rf` needs no prompt: outside the repo it cannot create *or* unlink — the macOS profile denies `file-write-create` and `file-write-unlink` in one rule. +**`git worktree remove`/`prune` are the one destructive exception**: their +denied write is the `.git/worktrees/` unlink, which comes *after* the +working tree is already gone, so the sandbox does not bound their damage — it +causes it. The bullets below are the measured record; never call either verb +from sandboxed Bash. **`allowWrite` must name the repo root, and that is the whole trick.** Writes are `allowOnly` minus `denyWithinAllow`, and the built-in `allowOnly` is only @@ -344,6 +352,54 @@ primitive for writes (reads have one, which is why `allowRead` differs), so no - **Create worktrees with `EnterWorktree`, never `git worktree add` from Bash** — the harness is not sandboxed; the Bash form writes `.git/config` and `.git/worktrees`, both denied. *(measured)* +- **`git worktree remove` is denied too, and unlike `add` it fails + DESTRUCTIVELY.** Removal deletes the working tree *first*, then unlinks + `.git/worktrees/` — and that unlink is the denied one: + + ``` + error: failed to delete '.../.claude/worktrees/backlogger': Operation not permitted + error: failed to delete '.git/worktrees/backlogger': Operation not permitted + ``` + + By then it has already deleted several hundred tracked files. It does not + roll back. What is left is a **carcass**: a registered worktree whose + `git status` is a few hundred ` D` lines and nothing else. That reads as + "uncommitted changes" to every later prune, so the worktree is now + permanently unprunable *by the failure itself* — re-running the removal + hits the no-`--force` refusal instead, and `--force` re-hits the denial. + + Because the filename set is identical in every worktree, so is APFS's + readdir order, so every carcass loses the **same** ~393 paths (`core/`, + `frontend/`, `bess_manager/`, `pyproject.toml`, `Dockerfile`, …). Identical + damage across many worktrees is the signature — do not read it as a + coincidence or as real edits. *(measured — 13 carcasses accumulated over + three sweeps before anyone noticed)* + + The denial is precisely on the `.git/worktrees/` unlink, **not** on the + working tree: `rm -rf .claude/worktrees/` from Bash succeeds. So an + agent can always destroy the files and never the registration. Do not + half-do it — that converts a carcass into a `prunable` phantom, which still + needs the same unsandboxed fix. *(measured)* + +- **`git worktree prune` is denied by the same unlink, and it EXITS 0 while + failing.** *(measured)* This is the nastier of the two: + + ``` + $ git worktree prune -v; echo "exit=$?" + Removing worktrees/backlogger: gitdir file points to non-existent location + error: failed to delete '.../.git/worktrees/backlogger': Operation not permitted + exit=0 + ``` + + The entry survives and `git worktree list` keeps showing it, now tagged + `prunable`. `remove` at least exits 255; `prune` reports success, so + `git worktree prune && echo done` prints `done` having done nothing. Never + infer from its exit status — re-check `git worktree list`. + + **So there is no in-sandbox path to removing a worktree**, by either verb. + It has to run unsandboxed — the maintainer pastes it with a `!` prefix, or + the harness does it via `ExitWorktree` (which only ever covers the session's + own `EnterWorktree` worktree, not a pre-existing one). - **`git checkout -b origin/` fails**, because recording the upstream writes `.git/config` — and it fails *after* creating the branch, so the branch exists while the command reports an error and leaves you on the @@ -362,6 +418,29 @@ primitive for writes (reads have one, which is why `allowRead` differs), so no ` rather than re-pushing, and don't read the message as a failed push. Use a plain `git push origin `; nothing in this repo's flow needs the upstream recorded. *(measured)* +- **`git branch -D` prints a `.git/config` error and deletes the branch + anyway**, exiting 0 with the noise on stderr: + + ``` + $ git branch -D worktree-backlogger; echo "exit=$?" + error: could not lock config file .../.git/config + warning: update of config-file failed + Deleted branch worktree-backlogger (was 6c70a77d). + exit=0 + ``` + + Delete completed the part that matters: refs are not denied (next bullet), + so the ref is gone. The config write it wanted was to drop the branch's + `[branch ""]` stanza — and **nothing was left behind**, verified by + grepping `.git/config` afterwards, because the branch never had a stanza to + drop. That is the general case under this sandbox rather than luck: writing + one requires `git push -u` or `checkout -b --track`, both of which are + denied by the two bullets above, so branches created here have no stanza. + A branch predating the sandbox could still have one; whether `-D` then + strands it is untested. + + Either way, do not re-run the delete on seeing the error and do not report + the branch as still present — check `git branch --list `. *(measured)* - **`.git/objects`, refs and the index are NOT denied**, so commit, branch, reset and reflog work normally. *(measured — this is the one that matters)* - The agent-config files are denied individually — `.claude/settings.json`, @@ -568,12 +647,15 @@ The full procedure, including the staged-changes variant, is in There is no cwd-conditional behaviour left, so nothing changes when a session enters a worktree. -**Don't add a prompt where git already refuses.** `git branch -D` and plain -`git worktree remove` are deliberately not in the `ask` list: git itself blocks -the dangerous case (it won't delete a branch checked out in another worktree, -and won't remove a worktree holding uncommitted or untracked files). A second -prompt there buys nothing and costs a stall on every run — `implement-issue` -Step 4's prune loop alone would have hit ~24 of them. +**Don't add a prompt where git already refuses.** `git branch -D` is +deliberately not in the `ask` list: git refuses to delete a branch checked out +in another worktree, so nothing a prompt would add is real. `git worktree +remove`/`prune` are not in the list either, but for a different reason — a +prompt cannot make them safe, because it does not un-deny the sandboxed +`.git/worktrees/` unlink that corrupts an otherwise-clean worktree +mid-removal (see "What stays denied" above). The guard is that the skills never +call them from sandboxed Bash: they report `PRUNE`/`CARCASS`/`PHANTOM` and emit +a `!`-prefixed command for the maintainer instead. **GitHub now refuses the dangerous pushes, which is the same reasoning one layer out.** `--force`, `--force-with-lease` and `+refspec` all used to ask,