fix(issue-21): review ffb5055e R1-R3 - re-check the tree after --stop, refuse submodule stashes, roll a partial submodule init back to a usable checkout
Review ffb5055e (planner-reviewer-codex-01): - R1 [P1] worktree_remove.sh: the clean-tree and submodule checks ran only before --stop, so work a stopping session wrote (or committed, or stashed) was deleted by the forced removal. The checks are one function now, run before any change and again right before the removal; any finding then refuses with exit 3 (sessions already stopped, worktree kept), and the removal method follows that latest look. - R2 [P1] lib_py/worktree.py: a submodule stash is reachable from no branch and leaves the status clean, and the forced removal deletes the worktree's own submodule repositories. `git stash list` in every submodule is a separate finding (`stashes`), refused like dirty/unpushed; errors fail closed as before. - R3 [P2] worktree_prepare.sh: when one submodule cloned and a later one failed, the rollback deleted modules/ while the cloned checkout's .git file still pointed into it, and `git status` failed in the worktree. The rollback now undoes only this attempt: each submodule whose git dir did not exist before is deinitialised before its git dir is deleted; submodules initialised earlier are kept; shared submodule.* config values are restored exactly (new keys removed, pre-existing values put back - init and deinit both write .git/config). A final `git status` either confirms a usable checkout or prints what to inspect. Tests W6e (a write during stop keeps the worktree and the file), W6f (a submodule stash refuses), W6g (partial failure on a fresh worktree: usable, no modules/, no new config, then prepare and a plain removal work) and W6h (a failing retry keeps the earlier `sm` init and its config value). The skill docs and the design doc describe the recheck, the stash rule and the scoped rollback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017R3Eg7MEpSWnm1oB9ZPP6G
This commit is contained in:
GodopuandClaude Opus 5 committed
1 parent
7988db9adf
commit
bbbbb95ef6
7 files changed
+181
-42
No files matched your search
@@ -124,19 +124,23 @@ def open_jobs(rows: List[dict], dirs: Iterable[str], exclude_ids: Iterable[str]
|
||||
|
||||
def submodule_findings(wt: str) -> Dict[str, object]:
|
||||
"""trace: the worktree git dir holds modules/ (git then refuses a plain
|
||||
`worktree remove`, even after a failed init); dirty / unpushed: what a
|
||||
forced removal would destroy. `HEAD` is part of the unpushed check: a
|
||||
submodule is on a detached HEAD after `submodule update`, so commits made
|
||||
there are on no branch and `--branches --not --remotes` alone misses them."""
|
||||
`worktree remove`, even after a failed init); dirty / unpushed / stashes:
|
||||
what a forced removal would destroy - it deletes the worktree's own
|
||||
submodule repositories. `HEAD` is part of the unpushed check: a submodule
|
||||
is on a detached HEAD after `submodule update`, so commits made there are
|
||||
on no branch and `--branches --not --remotes` alone misses them. Stashes
|
||||
live in refs/stash (and its reflog), reachable from neither."""
|
||||
gd = _git(wt, "rev-parse", "--absolute-git-dir")
|
||||
if gd.returncode != 0 or not gd.stdout.strip():
|
||||
return {"trace": False, "dirty": "", "unpushed": "", "error": gd.stderr.strip() or "rev-parse failed"}
|
||||
return {"trace": False, "dirty": "", "unpushed": "", "stashes": "",
|
||||
"error": gd.stderr.strip() or "rev-parse failed"}
|
||||
res: Dict[str, object] = {"trace": os.path.isdir(os.path.join(gd.stdout.strip(), "modules")),
|
||||
"dirty": "", "unpushed": "", "error": ""}
|
||||
"dirty": "", "unpushed": "", "stashes": "", "error": ""}
|
||||
if not res["trace"]:
|
||||
return res
|
||||
checks = (("dirty", "git status --porcelain"),
|
||||
("unpushed", "git log --oneline HEAD --branches --not --remotes"))
|
||||
("unpushed", "git log --oneline HEAD --branches --not --remotes"),
|
||||
("stashes", "git stash list"))
|
||||
errors = []
|
||||
for key, cmd in checks:
|
||||
r = _git(wt, "submodule", "foreach", "--recursive", "--quiet", cmd)
|
||||
|
||||
@@ -244,7 +244,7 @@ bash .agents/skills/multi-agent-mux-create/scripts/worktree_prepare.sh \
|
||||
```
|
||||
|
||||
- Creates the worktree next to the control root (a path **inside** it is refused), idempotently; links `.venv` to the control root's; adds `/.venv` and `.pytest_cache/` to the shared `info/exclude` (a linked worktree's exclude file is `$GIT_COMMON_DIR/info/exclude`, so it applies to the main checkout too).
|
||||
- **Submodules are opt-in** (`--submodules`). Initialisation writes `submodule.<name>.url` to the shared `.git/config`, and any attempt - even a failed one - leaves a `modules/` directory that makes a plain `git worktree remove` refuse. A failed `--submodules` removes those traces and exits 1; re-run without `--submodules` to finish.
|
||||
- **Submodules are opt-in** (`--submodules`). Initialisation writes `submodule.<name>.url` to the shared `.git/config`, and any attempt - even a failed one - leaves a `modules/` directory that makes a plain `git worktree remove` refuse. A failed `--submodules` undoes only what that attempt created - new submodule checkouts are deinitialised before their git dirs are deleted, so the worktree stays a usable checkout; submodules initialised earlier and pre-existing `submodule.*` values are kept - and exits 1; re-run without `--submodules` to finish.
|
||||
- `--creator-agent` creates the creator through this skill with the control root's binding and `MAM_NO_SUBSCRIBER_DAEMON=1`; the row's `pane.cwd` is the worktree.
|
||||
- Prints `BASE=<sha>`: record it **before** delegating the implementation, then review with `run_loop.sh --worktree <WT> --base "$BASE" --review-only`.
|
||||
- Exit codes: 0 ok, 1 git / submodule / create failure, 2 usage or path rule.
|
||||
|
||||
@@ -130,21 +130,51 @@ if [ -f "$WT/.gitmodules" ]; then
|
||||
if [ "$SUBMODULES" = "1" ]; then
|
||||
gd="$(git -C "$WT" rev-parse --absolute-git-dir)"
|
||||
had_modules=0
|
||||
[ -e "$gd/modules" ] && had_modules=1
|
||||
new_keys=()
|
||||
while read -r key _; do
|
||||
if [ -e "$gd/modules" ]; then had_modules=1; fi
|
||||
# The state before this attempt, so a failure undoes only what it created
|
||||
# (review R3): per submodule its path and whether its git dir already
|
||||
# existed (an earlier init - kept), and the exact shared config values
|
||||
# (init and deinit both write the common .git/config). Indexed arrays only
|
||||
# (bash 3.2 has no associative ones).
|
||||
sm_names=(); sm_paths=(); sm_pre=(); cfg_keys=(); cfg_had=(); cfg_vals=()
|
||||
while read -r key path; do
|
||||
[ -n "$key" ] || continue
|
||||
name="${key#submodule.}"; name="${name%.path}"
|
||||
sm_names+=("$name"); sm_paths+=("$path")
|
||||
if [ -e "$gd/modules/$name" ]; then sm_pre+=(1); else sm_pre+=(0); fi
|
||||
for k in url active; do
|
||||
git -C "$MAIN" config --get "submodule.$name.$k" >/dev/null 2>&1 || new_keys+=("submodule.$name.$k")
|
||||
cfg_keys+=("submodule.$name.$k")
|
||||
if v="$(git -C "$MAIN" config --get "submodule.$name.$k" 2>/dev/null)"; then
|
||||
cfg_had+=(1); cfg_vals+=("$v")
|
||||
else
|
||||
cfg_had+=(0); cfg_vals+=("")
|
||||
fi
|
||||
done
|
||||
done < <(git config --file "$WT/.gitmodules" --get-regexp '^submodule\..*\.path$' 2>/dev/null || true)
|
||||
if ! git -C "$WT" submodule update --init --recursive; then
|
||||
[ "$had_modules" = "1" ] || rm -rf "$gd/modules"
|
||||
for k in ${new_keys[@]+"${new_keys[@]}"}; do
|
||||
git -C "$MAIN" config --unset-all "$k" 2>/dev/null || true
|
||||
# A submodule cloned by this attempt leaves a checkout whose .git file
|
||||
# points into modules/<name>: deinit it before its git dir goes, or the
|
||||
# worktree stops being a usable git checkout.
|
||||
for i in $(seq 0 $(( ${#sm_names[@]} - 1 ))); do
|
||||
if [ "${sm_pre[$i]}" = "1" ]; then continue; fi
|
||||
git -C "$WT" submodule deinit -q -f -- "${sm_paths[$i]}" >/dev/null 2>&1 || true
|
||||
if [ -f "$WT/${sm_paths[$i]}/.git" ]; then rm -f "$WT/${sm_paths[$i]}/.git"; fi
|
||||
rm -rf "$gd/modules/${sm_names[$i]}"
|
||||
done
|
||||
echo "ERROR: submodule initialisation failed; its traces (modules/, new submodule.* config keys) were removed so the worktree can still be removed without --force. Re-run without --submodules to finish preparing." >&2
|
||||
if [ "$had_modules" != "1" ]; then rm -rf "$gd/modules"; fi
|
||||
for i in $(seq 0 $(( ${#cfg_keys[@]} - 1 ))); do
|
||||
if [ "${cfg_had[$i]}" = "1" ]; then
|
||||
git -C "$MAIN" config "${cfg_keys[$i]}" "${cfg_vals[$i]}"
|
||||
else
|
||||
git -C "$MAIN" config --unset-all "${cfg_keys[$i]}" 2>/dev/null || true
|
||||
fi
|
||||
done
|
||||
if st_err="$(git -C "$WT" status --porcelain 2>&1 >/dev/null)"; then
|
||||
echo "ERROR: submodule initialisation failed; this attempt's traces (new submodule checkouts, their git dirs, new submodule.* config keys) were removed and earlier submodule state was kept, so the worktree stays usable and removable. Re-run without --submodules to finish preparing." >&2
|
||||
else
|
||||
echo "ERROR: submodule initialisation failed, and '$WT' is not a usable git checkout after the rollback: $st_err" >&2
|
||||
echo " inspect it with: git -C '$WT' submodule status - do not force-remove it before checking its submodules" >&2
|
||||
fi
|
||||
exit 1
|
||||
fi
|
||||
SUB_STATE="initialised"
|
||||
|
||||
@@ -121,9 +121,9 @@ Every check runs first and changes nothing; a refusal exits 3:
|
||||
2. running sessions under it only with `--stop` (stopped normally, conversations kept);
|
||||
3. no open job of those sessions - never cancelled automatically, they may belong to a running loop (the delegate job of a session `--stop` stops is cancelled by stop itself);
|
||||
4. a clean tree, submodules included (`status --porcelain --ignore-submodules=none`);
|
||||
5. inside submodules: no modification and no commit that is on no remote (`HEAD` included - a submodule sits on a detached HEAD).
|
||||
5. inside submodules: no modification, no commit that is on no remote (`HEAD` included - a submodule sits on a detached HEAD) and no stash (`refs/stash` is reachable from no branch and leaves the status clean).
|
||||
|
||||
Then it stops the sessions, re-checks the jobs and removes the worktree (the branch is kept): plain, or - when the worktree holds submodule state, which git refuses to remove otherwise - a **single `--force` justified by checks 4 and 5**. Do not run `git worktree remove --force` by hand on such a worktree: it deletes the worktree's own submodule repositories, unpushed commits included.
|
||||
Then it stops the sessions, re-checks the jobs, runs checks 4 and 5 **again** (a session that is being stopped can still write, commit or stash - any change refuses with exit 3, sessions already stopped) and removes the worktree (the branch is kept): plain, or - when the worktree holds submodule state, which git refuses to remove otherwise - a **single `--force` justified by checks 4 and 5**. Do not run `git worktree remove --force` by hand on such a worktree: it deletes the worktree's own submodule repositories, unpushed commits included.
|
||||
|
||||
Exit codes: 0 removed, 1 stop/git failure, 2 not a worktree of the control root, 3 refused (nothing changed). Sessions created in a removed worktree cannot be resumed until a worktree exists at that path again.
|
||||
|
||||
|
||||
@@ -11,7 +11,9 @@
|
||||
# delegate job of a session --stop will stop is cancelled by stop itself);
|
||||
# (4) clean tree, submodules included; (5) submodules: nothing modified and no
|
||||
# commit that is on no remote (HEAD included - a submodule is on a detached
|
||||
# HEAD). Removal is plain, or - when the worktree holds submodule state, which
|
||||
# HEAD) and no stash. (4) and (5) run again right before the removal, after
|
||||
# --stop: a session that is being stopped can still write, commit or stash.
|
||||
# Removal is plain, or - when the worktree holds submodule state, which
|
||||
# git refuses to remove without it - a single --force justified by (4) and (5).
|
||||
# Never run `git worktree remove --force` by hand on such a worktree.
|
||||
#
|
||||
@@ -79,27 +81,37 @@ if [ "$(printf '%s' "$JOBS" | jget 'len(d)')" != "0" ]; then
|
||||
echo " close one after checking it: python3 .agents/skills/multi-agent-mux-delegate-job/scripts/registry.py --registry-dir <dir> close --job <id> --request cancelled" >&2
|
||||
fi
|
||||
|
||||
# (4) clean, submodules included
|
||||
DIRTY="$(git -C "$WT" status --porcelain --ignore-submodules=none)"
|
||||
if [ -n "$DIRTY" ]; then
|
||||
refuse "the worktree has uncommitted changes:"
|
||||
printf '%s\n' "$DIRTY" | sed 's/^/ /' >&2
|
||||
fi
|
||||
|
||||
# (5) submodules
|
||||
SUB="$(wt_py submodules "$WT")"
|
||||
TRACE="$(printf '%s' "$SUB" | jget 'd["trace"]')"
|
||||
METHOD="plain"
|
||||
if [ "$TRACE" = "True" ]; then
|
||||
METHOD="single --force"
|
||||
for key in error dirty unpushed; do
|
||||
val="$(printf '%s' "$SUB" | jget "d['$key']")"
|
||||
if [ -n "$val" ]; then
|
||||
refuse "submodule check '$key' is not empty (a forced removal would destroy it):"
|
||||
printf '%s\n' "$val" | sed 's/^/ /' >&2
|
||||
fi
|
||||
done
|
||||
fi
|
||||
# (4) clean, submodules included, and (5) submodules. A function: it runs now,
|
||||
# before any change, and again right before the removal (review R1). It sets
|
||||
# METHOD from what it saw and returns 1 on any finding.
|
||||
judge_tree() {
|
||||
local found=0 dirty sub trace key val
|
||||
if ! dirty="$(git -C "$WT" status --porcelain --ignore-submodules=none 2>&1)"; then
|
||||
refuse "git status failed in the worktree: $dirty"
|
||||
return 1
|
||||
fi
|
||||
if [ -n "$dirty" ]; then
|
||||
refuse "the worktree has uncommitted changes:"
|
||||
printf '%s\n' "$dirty" | sed 's/^/ /' >&2
|
||||
found=1
|
||||
fi
|
||||
sub="$(wt_py submodules "$WT")"
|
||||
trace="$(printf '%s' "$sub" | jget 'd["trace"]')"
|
||||
METHOD="plain"
|
||||
if [ "$trace" = "True" ]; then
|
||||
METHOD="single --force"
|
||||
for key in error dirty unpushed stashes; do
|
||||
val="$(printf '%s' "$sub" | jget "d.get('$key', '')")"
|
||||
if [ -n "$val" ]; then
|
||||
refuse "submodule check '$key' is not empty (a forced removal would destroy it):"
|
||||
printf '%s\n' "$val" | sed 's/^/ /' >&2
|
||||
found=1
|
||||
fi
|
||||
done
|
||||
fi
|
||||
return "$found"
|
||||
}
|
||||
judge_tree || true
|
||||
|
||||
echo "checks: locked/sessions/jobs/clean/submodules -> $([ "$REFUSED" = 1 ] && echo refused || echo passed); removal method: $METHOD"
|
||||
if [ "$REFUSED" = "1" ]; then
|
||||
@@ -130,6 +142,14 @@ if [ "$STOP" = "1" ] && [ -n "$RUNNING_AGENTS" ]; then
|
||||
fi
|
||||
fi
|
||||
|
||||
# (6b) the tree again, right before removing it: the stopped sessions may have
|
||||
# written, committed or stashed while they stopped (review R1). The
|
||||
# removal method follows this latest look.
|
||||
if ! judge_tree; then
|
||||
echo "REFUSED: ${RUNNING:+the sessions were stopped, but }the worktree changed after the checks; it was not removed" >&2
|
||||
exit 3
|
||||
fi
|
||||
|
||||
# (7) remove; the branch is kept
|
||||
if [ "$METHOD" = "plain" ]; then
|
||||
git -C "$MAIN" worktree remove "$WT" || exit 1
|
||||
|
||||
Reference in new issue
Block a user