From c7c559b7aa216e1c3405de47662c626032353e24 Mon Sep 17 00:00:00 2001 From: Godopu Date: Mon, 14 Sep 2026 18:13:54 +0000 Subject: [PATCH] fix(issue-21): reviews 8f141362 N1 and 275fdc4f R4/R5 - clean refusal for a broken worktree, no init over existing files, check retained submodule repositories - N1 (planner-reviewer-claude-01) worktree_remove.sh: when `git status` fails in the worktree (e.g. its directory was deleted), judge_tree returned before METHOD was set and the summary line died under set -u with exit 1. METHOD now has its default before the function; the refusal is exit 3 with "nothing was changed", plus a `git worktree prune` hint when the directory is gone. Same for --dry-run. - R4 (planner-reviewer-codex-01) worktree_prepare.sh: a submodule path that is not initialised yet but already holds files made the clone fail, and the rollback's `deinit -f` then deleted those files. --submodules now refuses such a path up front (new exit 3, nothing changed), which also makes the rollback's premise true: after a failed attempt, everything under a not-yet-initialised path was created by that attempt. - R5 (planner-reviewer-codex-01) lib_py/worktree.py: `submodule foreach` visits initialised checkouts only, but a deinit'd submodule keeps its repository - stash and unpushed commits included - under the worktree's modules/, which the forced removal deletes. Every repository under modules/ (nested ones included) is now checked directly (`--git-dir`): commits on no remote (HEAD included) and refs/stash with its reflog. A directory that looks like a partial repository cannot be checked and refuses. - Also: the rollback loops are C-style (`seq 0 -1` counts down on BSD), and prepare warns that --base is ignored for an existing branch. Tests W6i (deleted worktree directory, with and without --dry-run), W6j and W6j2 (stash / unpushed commit in a deinit'd submodule's repository) and W6k (existing files in an uninitialised submodule path). Skill docs and the design doc describe the new refusal and the wider repository check. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017R3Eg7MEpSWnm1oB9ZPP6G --- .agents/skills/lib_py/worktree.py | 46 ++++++++++ .../skills/multi-agent-mux-create/SKILL.md | 3 +- .../scripts/worktree_prepare.sh | 26 +++++- .agents/skills/multi-agent-mux-stop/SKILL.md | 2 +- .../scripts/worktree_remove.sh | 7 +- docs/design/worktree-creator-model.md | 4 +- tests/test_issue21_phase2_loop_a.py | 88 +++++++++++++++++++ 7 files changed, 168 insertions(+), 8 deletions(-) diff --git a/.agents/skills/lib_py/worktree.py b/.agents/skills/lib_py/worktree.py index 100a5c3..df52ed5 100644 --- a/.agents/skills/lib_py/worktree.py +++ b/.agents/skills/lib_py/worktree.py @@ -122,6 +122,26 @@ def open_jobs(rows: List[dict], dirs: Iterable[str], exclude_ids: Iterable[str] return out +def _module_gitdirs(root: str): + """Repositories under a worktree's modules/ (nested ones included), and + directories that look like partial repositories (a repository marker but + not a checkable repository).""" + repos: List[str] = [] + partial: List[str] = [] + if not os.path.isdir(root): + return repos, partial + for dirpath, dirnames, filenames in os.walk(root): + if dirpath == root: + continue + if "HEAD" in filenames and "objects" in dirnames: + repos.append(dirpath) + dirnames[:] = [d for d in dirnames if d == "modules"] # nested submodules only + elif "HEAD" in filenames or "objects" in dirnames or "refs" in dirnames: + partial.append(dirpath) + dirnames[:] = [] + return repos, partial + + 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 / stashes: @@ -148,6 +168,32 @@ def submodule_findings(wt: str) -> Dict[str, object]: errors.append(r.stderr.strip() or f"{key} check failed (rc {r.returncode})") else: res[key] = r.stdout.strip() + # Every repository under modules/, whether its checkout is initialised or + # not (review R5): `submodule foreach` visits initialised checkouts only, + # but a deinit'd submodule keeps its repository - stash included - here, + # and the forced removal deletes it. A repository that cannot be checked + # is an error, never assumed safe. + repos, partial = _module_gitdirs(os.path.join(gd.stdout.strip(), "modules")) + extra: Dict[str, List[str]] = {"unpushed": [], "stashes": []} + for d in repos: + git = ["git", "--git-dir", d, "--work-tree", d] + u = subprocess.run(git + ["log", "--oneline", "HEAD", "--branches", "--not", "--remotes"], + capture_output=True, text=True) + if u.returncode != 0: + errors.append(f"{d}: cannot list its unpushed commits ({u.stderr.strip() or u.returncode})") + elif u.stdout.strip(): + extra["unpushed"] += [f"{d}: {line}" for line in u.stdout.strip().splitlines()] + s = subprocess.run(git + ["for-each-ref", "--format=%(refname)", "refs/stash"], capture_output=True, text=True) + if s.returncode != 0: + errors.append(f"{d}: cannot read refs/stash ({s.stderr.strip() or s.returncode})") + elif s.stdout.strip(): + g = subprocess.run(git + ["log", "-g", "--format=%gd %s", "refs/stash"], capture_output=True, text=True) + lines = g.stdout.strip().splitlines() if g.returncode == 0 and g.stdout.strip() else ["refs/stash"] + extra["stashes"] += [f"{d}: {line}" for line in lines] + errors += [f"{p}: looks like a partial repository and cannot be checked" for p in partial] + for key, lines in extra.items(): + if lines: + res[key] = "\n".join(x for x in [str(res[key]).strip(), *lines] if x) res["error"] = "; ".join(errors) return res diff --git a/.agents/skills/multi-agent-mux-create/SKILL.md b/.agents/skills/multi-agent-mux-create/SKILL.md index 7427a5d..372aff9 100644 --- a/.agents/skills/multi-agent-mux-create/SKILL.md +++ b/.agents/skills/multi-agent-mux-create/SKILL.md @@ -247,7 +247,8 @@ bash .agents/skills/multi-agent-mux-create/scripts/worktree_prepare.sh \ - **Submodules are opt-in** (`--submodules`). Initialisation writes `submodule..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=`: record it **before** delegating the implementation, then review with `run_loop.sh --worktree --base "$BASE" --review-only`. -- Exit codes: 0 ok, 1 git / submodule / create failure, 2 usage or path rule. +- A submodule path that is not initialised yet but already holds files refuses `--submodules` up front (exit 3, nothing changed): the clone would fail on it and a rollback could not tell those files from its own. +- Exit codes: 0 ok, 1 git / submodule / create failure, 2 usage or path rule, 3 refused to protect existing content (nothing changed). - create itself: a control-plane binding that cannot be computed is refused (exit 2) for a `--workspace` other than the control root and warned about for the control root; the immediate reconcile and the watchdog run the control root's code. ## Pitfalls diff --git a/.agents/skills/multi-agent-mux-create/scripts/worktree_prepare.sh b/.agents/skills/multi-agent-mux-create/scripts/worktree_prepare.sh index c70106e..b0c86c9 100755 --- a/.agents/skills/multi-agent-mux-create/scripts/worktree_prepare.sh +++ b/.agents/skills/multi-agent-mux-create/scripts/worktree_prepare.sh @@ -14,7 +14,8 @@ # modules/ trace that makes a plain `git worktree remove` refuse) -> optional # creator session registered in the control root's .mam -> summary with BASE=. # -# Exit codes: 0 ok, 1 git / submodule / create failure, 2 usage or path rule. +# Exit codes: 0 ok, 1 git / submodule / create failure, 2 usage or path rule, +# 3 refused to protect existing content (nothing changed). set -euo pipefail _script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -98,6 +99,9 @@ else fi mkdir -p "$(dirname "$WT")" if git -C "$MAIN" show-ref --verify --quiet "refs/heads/$BRANCH"; then + if [ -n "$BASE_REF" ]; then + echo "WARNING: branch '$BRANCH' already exists; --base '$BASE_REF' is ignored (the worktree checks the branch out as it is)" >&2 + fi git -C "$MAIN" worktree add "$WT" "$BRANCH" || exit 1 else git -C "$MAIN" worktree add -b "$BRANCH" "$WT" "${BASE_REF:-HEAD}" || exit 1 @@ -151,18 +155,34 @@ if [ -f "$WT/.gitmodules" ]; then fi done done < <(git config --file "$WT/.gitmodules" --get-regexp '^submodule\..*\.path$' 2>/dev/null || true) + # A submodule path that is not initialised yet but already holds files + # (review R4): git refuses to clone into it, and a rollback could not tell + # those files from its own - refuse before changing anything. With this, + # everything under a not-yet-initialised path after a failed attempt was + # created by that attempt, which is what the rollback relies on. + blocked=() + for ((i = 0; i < ${#sm_names[@]}; i++)); do + p="$WT/${sm_paths[$i]}" + if [ "${sm_pre[$i]}" != "1" ] && [ -e "$p" ] && [ -n "$(ls -A "$p" 2>/dev/null)" ]; then + blocked+=("${sm_paths[$i]}") + fi + done + if [ "${#blocked[@]}" -gt 0 ]; then + echo "REFUSED: submodule path(s) not initialised yet but not empty: ${blocked[*]}. Initialising would fail on them and a rollback could not tell their files from its own; move the files away (or drop --submodules). Nothing was changed." >&2 + exit 3 + fi if ! git -C "$WT" submodule update --init --recursive; then # A submodule cloned by this attempt leaves a checkout whose .git file # points into modules/: 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 + for ((i = 0; i < ${#sm_names[@]}; i++)); 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 if [ "$had_modules" != "1" ]; then rm -rf "$gd/modules"; fi - for i in $(seq 0 $(( ${#cfg_keys[@]} - 1 ))); do + for ((i = 0; i < ${#cfg_keys[@]}; i++)); do if [ "${cfg_had[$i]}" = "1" ]; then git -C "$MAIN" config "${cfg_keys[$i]}" "${cfg_vals[$i]}" else diff --git a/.agents/skills/multi-agent-mux-stop/SKILL.md b/.agents/skills/multi-agent-mux-stop/SKILL.md index 9e98ef0..b2a7738 100644 --- a/.agents/skills/multi-agent-mux-stop/SKILL.md +++ b/.agents/skills/multi-agent-mux-stop/SKILL.md @@ -121,7 +121,7 @@ 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, 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). +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) - checked in every repository under the worktree's `modules/`, including deinitialised submodules (their repository stays there) and nested ones; a repository that cannot be checked refuses. 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. diff --git a/.agents/skills/multi-agent-mux-stop/scripts/worktree_remove.sh b/.agents/skills/multi-agent-mux-stop/scripts/worktree_remove.sh index e2adf65..a632be1 100755 --- a/.agents/skills/multi-agent-mux-stop/scripts/worktree_remove.sh +++ b/.agents/skills/multi-agent-mux-stop/scripts/worktree_remove.sh @@ -11,7 +11,8 @@ # 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) and no stash. (4) and (5) run again right before the removal, after +# HEAD) and no stash - in every repository under the worktree's modules/, +# initialised or not. (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). @@ -84,10 +85,14 @@ 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. +METHOD="plain" # set before any early return below: the summary line reads it under set -u 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" + if [ ! -d "$WT" ]; then + echo " the directory no longer exists; if that is intended, run: git -C '$MAIN' worktree prune" >&2 + fi return 1 fi if [ -n "$dirty" ]; then diff --git a/docs/design/worktree-creator-model.md b/docs/design/worktree-creator-model.md index 9c54eb9..e6ead43 100644 --- a/docs/design/worktree-creator-model.md +++ b/docs/design/worktree-creator-model.md @@ -89,8 +89,8 @@ bash "$MAIN/.agents/skills/multi-agent-mux-stop/scripts/worktree_remove.sh" --pa - 준비·해체 헬퍼는 항목 4 로 구현했다(`worktree_prepare.sh`, `worktree_remove.sh`). - `.mam.env` 는 복사하지 않는다. creator 에는 제어 루트의 것을 `MAM_ENV_FILE` 로 준다. - `.venv` 는 제어 루트 것의 심링크다. 무시 규칙(`/.venv`, `.pytest_cache/`)은 linked worktree 가 공유하는 `$GIT_COMMON_DIR/info/exclude` 에 들어간다. 그래서 본 체크아웃에도 적용되며, 둘 다 본 체크아웃에서도 무시해야 하는 산출물이다. - - 서브모듈은 `--submodules` 로만 초기화한다. 초기화는 성공·실패와 무관하게 워크트리 git 디렉터리에 `modules/` 를 남기고, 그러면 git 이 force 없는 `worktree remove` 를 거부한다. prepare 는 실패한 시도가 만든 것만 되돌리고 exit 1 한다. 새 체크아웃은 git 디렉터리보다 먼저 deinit 해서 워크트리가 쓸 수 있는 체크아웃으로 남게 하고, 새 `submodule.*` 공유 설정 키는 지운다. 이전 초기화 상태와 기존 설정 값은 보존한다. - - 서브모듈을 초기화한 워크트리는 remove 가 수정, 원격에 없는 커밋(`HEAD` 포함 — 서브모듈은 detached HEAD 다), stash 를 확인한 뒤 `--force` 를 한 번만 써서 제거한다. 이 확인은 `--stop` 으로 세션을 정지한 뒤 제거 직전에 한 번 더 한다(정지 중에도 쓰기·커밋·stash 가 생길 수 있다). **사람이 `git worktree remove --force` 를 직접 쓰지 말 것.** 워크트리 전용 서브모듈 저장소와 미푸시 커밋이 함께 사라진다. + - 서브모듈은 `--submodules` 로만 초기화한다. 초기화는 성공·실패와 무관하게 워크트리 git 디렉터리에 `modules/` 를 남기고, 그러면 git 이 force 없는 `worktree remove` 를 거부한다. 초기화하지 않은 서브모듈 경로에 이미 파일이 있으면 아무것도 바꾸지 않고 exit 3 으로 거부한다. prepare 는 실패한 시도가 만든 것만 되돌리고 exit 1 한다. 새 체크아웃은 git 디렉터리보다 먼저 deinit 해서 워크트리가 쓸 수 있는 체크아웃으로 남게 하고, 새 `submodule.*` 공유 설정 키는 지운다. 이전 초기화 상태와 기존 설정 값은 보존한다. + - 서브모듈을 초기화한 워크트리는 remove 가 수정, 원격에 없는 커밋(`HEAD` 포함 — 서브모듈은 detached HEAD 다), stash 를 확인한 뒤 `--force` 를 한 번만 써서 제거한다. 이 확인은 초기화된 체크아웃뿐 아니라 워크트리 `modules/` 아래의 모든 저장소(deinit 된 서브모듈, 중첩 서브모듈 포함)에 직접 하며, 확인할 수 없는 저장소가 있으면 거부한다. 이 확인은 `--stop` 으로 세션을 정지한 뒤 제거 직전에 한 번 더 한다(정지 중에도 쓰기·커밋·stash 가 생길 수 있다). **사람이 `git worktree remove --force` 를 직접 쓰지 말 것.** 워크트리 전용 서브모듈 저장소와 미푸시 커밋이 함께 사라진다. ## 5. 필요한 변경 (Issue #21 작업 항목) diff --git a/tests/test_issue21_phase2_loop_a.py b/tests/test_issue21_phase2_loop_a.py index c7cd7e0..6a1ee73 100644 --- a/tests/test_issue21_phase2_loop_a.py +++ b/tests/test_issue21_phase2_loop_a.py @@ -516,3 +516,91 @@ def test_w6h_failed_retry_keeps_the_earlier_submodule_state(ctl_sub): assert (wt / "sm" / ".git").exists() and (wt / "sm" / "s.txt").exists() assert _git(main, "config", "--get", "submodule.sm.url").stdout.strip() == url_before assert _git(main, "config", "--get", "submodule.zbad.url", check=False).returncode != 0 + + +# --------------------------------------------------------------------------- +# reviews 8f141362 (N1) and 275fdc4f (R4, R5) +# --------------------------------------------------------------------------- + +def test_w6i_deleted_worktree_directory_is_refused_cleanly(ctl): + """N1: `git status` fails in a worktree whose directory is gone - the + helper refuses (exit 3) instead of dying on an unset METHOD.""" + main = ctl + wt = _wt(main) + assert _prepare(main, wt).returncode == 0 + shutil.rmtree(wt) + for extra in ([], ["--dry-run"]): + res = _run(main, REM, "--path", str(wt), *extra) + assert res.returncode == 3, res.stdout + res.stderr + assert "worktree prune" in res.stderr + assert "nothing was changed" in res.stdout + res.stderr + assert "unbound variable" not in res.stderr + assert str(wt) in _worktrees(main), "nothing was pruned or removed" + + +def _deinit_after(wt: Path, make_state) -> str: + """Put state into the initialised `sm`, then deinit it (no force); returns + the submodule's repository, which stays under the worktree's modules/.""" + make_state(wt / "sm") + gd = _git(wt / "sm", "rev-parse", "--absolute-git-dir").stdout.strip() + _git(wt, "submodule", "deinit", "-q", "sm") + assert not (wt / "sm" / ".git").exists() and os.path.isdir(gd) + return gd + + +def test_w6j_stash_in_a_deinitialised_submodule_repository_is_kept(ctl_sub): + """R5: foreach no longer visits a deinit'd submodule, but its repository - + and its stash - is still under modules/ and would be force-removed.""" + main = ctl_sub + wt = _wt(main) + assert _prepare(main, wt, "--submodules", env=FILE_OK).returncode == 0 + + def stash(sm): + (sm / "s.txt").write_text("valuable stashed work\n") + _git(sm, "stash", "push", "-q", "-m", "keep this work") + + gd = _deinit_after(wt, stash) + before = _git(gd, "rev-parse", "refs/stash").stdout.strip() + res = _run(main, REM, "--path", str(wt)) + assert res.returncode == 3 and "'stashes'" in res.stderr, res.stdout + res.stderr + assert _git(gd, "rev-parse", "refs/stash").stdout.strip() == before + + +def test_w6j2_unpushed_commit_in_a_deinitialised_submodule_repository_is_kept(ctl_sub): + main = ctl_sub + wt = _wt(main) + assert _prepare(main, wt, "--submodules", env=FILE_OK).returncode == 0 + + local = [] + + def commit(sm): + (sm / "new.txt").write_text("n\n") + _git(sm, "add", "new.txt") + _git(sm, "commit", "-q", "-m", "local only") + local.append(_git(sm, "rev-parse", "HEAD").stdout.strip()) + # record the new gitlink first: deinit refuses a submodule whose HEAD + # differs from it, and the top level must be clean for the scenario + _git(wt, "add", "sm") + _git(wt, "commit", "-q", "-m", "bump sm") + + gd = _deinit_after(wt, commit) + assert _git(wt, "status", "--porcelain", "--ignore-submodules=none").stdout == "" + res = _run(main, REM, "--path", str(wt)) + assert res.returncode == 3 and "'unpushed'" in res.stderr, res.stdout + res.stderr + assert _git(gd, "cat-file", "-e", local[0], check=False).returncode == 0 + + +def test_w6k_files_in_an_uninitialised_submodule_path_refuse_the_init(ctl_sub): + """R4: files that predate the attempt in a not-yet-initialised submodule + path refuse --submodules up front; nothing is changed or deleted.""" + main = ctl_sub + wt = _wt(main) + assert _prepare(main, wt).returncode == 0 + valuable = wt / "sm" / "user-work.txt" + valuable.write_text("existing user work\n") + res = _prepare(main, wt, "--submodules", env=FILE_OK) + assert res.returncode == 3 and "not empty" in res.stderr, res.stdout + res.stderr + assert valuable.read_text() == "existing user work\n" + gd = _git(wt, "rev-parse", "--absolute-git-dir").stdout.strip() + assert not os.path.exists(os.path.join(gd, "modules")) + assert _git(main, "config", "--get", "submodule.sm.url", check=False).returncode != 0