From 25f4be3359ad923ac1917a1cc1a7995943c05f7a Mon Sep 17 00:00:00 2001 From: Godopu Date: Mon, 14 Sep 2026 20:11:18 +0000 Subject: [PATCH] fix(issue-21): review ec50fbe7 R6 - the unpushed check covers HEAD and every local ref (tags included, stash excluded) in both submodule checks Both unpushed checks in lib_py/worktree.py - `submodule foreach` over the initialised checkouts and the direct `--git-dir` walk over the repositories under the worktree's modules/ - used `HEAD --branches --not --remotes`. A commit made on a submodule's detached HEAD and kept only by a tag is reachable from neither, the top-level status stays clean, and worktree_remove.sh then deleted the repository with its single --force (exit 0). Both checks now share one rev list, UNPUSHED_REVS = HEAD --exclude=refs/stash --all --not --remotes: HEAD and every local ref (branches, tags, notes, ...) minus remote-tracking refs. refs/stash stays out of it because the stash check (stash list / reflog) owns it, so a stash is reported once, as 'stashes'. As intended, a local-only note is now protected too. The stash, error, partial repository and dirty checks are unchanged. Tests W6l (tag-only commit; initialised submodule and deinit'd repository) and W6m (annotated tag). The helper header, the stop SKILL.md and the design doc state the wider rule. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017R3Eg7MEpSWnm1oB9ZPP6G --- .agents/skills/lib_py/worktree.py | 18 +++++--- .agents/skills/multi-agent-mux-stop/SKILL.md | 2 +- .../scripts/worktree_remove.sh | 4 +- docs/design/worktree-creator-model.md | 2 +- tests/test_issue21_phase2_loop_a.py | 43 +++++++++++++++++++ 5 files changed, 59 insertions(+), 10 deletions(-) diff --git a/.agents/skills/lib_py/worktree.py b/.agents/skills/lib_py/worktree.py index df52ed5..b6ee9c1 100644 --- a/.agents/skills/lib_py/worktree.py +++ b/.agents/skills/lib_py/worktree.py @@ -24,6 +24,10 @@ import sys from typing import Dict, Iterable, List, Optional OPEN_STATUSES = ("pending", "running") +# The commits a forced removal would lose (review R6): reachable from HEAD or +# from any local ref - branches, tags, notes, ... - except refs/stash (the +# stash check covers it, reflog included), and from no remote-tracking ref. +UNPUSHED_REVS = ("HEAD", "--exclude=refs/stash", "--all", "--not", "--remotes") def _git(cwd: str, *args: str) -> subprocess.CompletedProcess: @@ -146,10 +150,12 @@ 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: 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.""" + submodule repositories. The unpushed check covers HEAD and every local + ref except refs/stash (UNPUSHED_REVS): a submodule is on a detached HEAD + after `submodule update`, so its commits may be on no branch, and a commit + kept only by a tag (review R6) or a note is lost with the repository just + the same. Stashes live in refs/stash and its reflog; the stash check owns + them.""" gd = _git(wt, "rev-parse", "--absolute-git-dir") if gd.returncode != 0 or not gd.stdout.strip(): return {"trace": False, "dirty": "", "unpushed": "", "stashes": "", @@ -159,7 +165,7 @@ def submodule_findings(wt: str) -> Dict[str, object]: if not res["trace"]: return res checks = (("dirty", "git status --porcelain"), - ("unpushed", "git log --oneline HEAD --branches --not --remotes"), + ("unpushed", "git log --oneline " + " ".join(UNPUSHED_REVS)), ("stashes", "git stash list")) errors = [] for key, cmd in checks: @@ -177,7 +183,7 @@ def submodule_findings(wt: str) -> Dict[str, object]: 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"], + u = subprocess.run(git + ["log", "--oneline", *UNPUSHED_REVS], capture_output=True, text=True) if u.returncode != 0: errors.append(f"{d}: cannot list its unpushed commits ({u.stderr.strip() or u.returncode})") diff --git a/.agents/skills/multi-agent-mux-stop/SKILL.md b/.agents/skills/multi-agent-mux-stop/SKILL.md index b2a7738..87ea10f 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) - 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. +5. inside submodules: no modification, no commit reachable from `HEAD` or any local ref that is on no remote (a submodule sits on a detached HEAD; a commit kept only by a tag or a note is protected too) 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 a632be1..4537197 100755 --- a/.agents/skills/multi-agent-mux-stop/scripts/worktree_remove.sh +++ b/.agents/skills/multi-agent-mux-stop/scripts/worktree_remove.sh @@ -10,8 +10,8 @@ # session under it unless --stop; (3) no open job of those sessions (the # 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 - in every repository under the worktree's modules/, +# commit reachable from HEAD or any local ref (tags included - a submodule is +# on a detached HEAD) that is on no remote, 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 diff --git a/docs/design/worktree-creator-model.md b/docs/design/worktree-creator-model.md index e6ead43..d8a9229 100644 --- a/docs/design/worktree-creator-model.md +++ b/docs/design/worktree-creator-model.md @@ -90,7 +90,7 @@ bash "$MAIN/.agents/skills/multi-agent-mux-stop/scripts/worktree_remove.sh" --pa - `.mam.env` 는 복사하지 않는다. creator 에는 제어 루트의 것을 `MAM_ENV_FILE` 로 준다. - `.venv` 는 제어 루트 것의 심링크다. 무시 규칙(`/.venv`, `.pytest_cache/`)은 linked worktree 가 공유하는 `$GIT_COMMON_DIR/info/exclude` 에 들어간다. 그래서 본 체크아웃에도 적용되며, 둘 다 본 체크아웃에서도 무시해야 하는 산출물이다. - 서브모듈은 `--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` 를 직접 쓰지 말 것.** 워크트리 전용 서브모듈 저장소와 미푸시 커밋이 함께 사라진다. + - 서브모듈을 초기화한 워크트리는 remove 가 수정, HEAD 와 모든 로컬 ref(tag·notes 포함)에서 도달하는, 원격에 없는 커밋(서브모듈은 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 6a1ee73..6b274c7 100644 --- a/tests/test_issue21_phase2_loop_a.py +++ b/tests/test_issue21_phase2_loop_a.py @@ -604,3 +604,46 @@ def test_w6k_files_in_an_uninitialised_submodule_path_refuse_the_init(ctl_sub): 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 + + +# --------------------------------------------------------------------------- +# review ec50fbe7 (R6): a submodule commit kept only by a tag +# --------------------------------------------------------------------------- + +def _tag_only_commit(sm: Path, annotated: bool = False) -> str: + """Commit on the submodule's detached HEAD, keep it only by a tag, go back + to the recorded commit (the top level stays clean). Returns the commit.""" + orig = _git(sm, "rev-parse", "HEAD").stdout.strip() + (sm / "tagged.txt").write_text("kept only by a tag\n") + _git(sm, "add", "tagged.txt") + _git(sm, "commit", "-q", "-m", "tag-only work") + local = _git(sm, "rev-parse", "HEAD").stdout.strip() + _git(sm, "tag", *(["-a", "-m", "keep"] if annotated else []), "saved-work") + _git(sm, "checkout", "-q", orig) + return local + + +def _remove_refuses_tag_only_commit(main: Path, annotated: bool, deinit: bool) -> None: + wt = _wt(main) + assert _prepare(main, wt, "--submodules", env=FILE_OK).returncode == 0 + local = _tag_only_commit(wt / "sm", annotated) + gd = _git(wt / "sm", "rev-parse", "--absolute-git-dir").stdout.strip() + assert _git(wt, "status", "--porcelain", "--ignore-submodules=none").stdout == "" + if deinit: + _git(wt, "submodule", "deinit", "-q", "sm") + res = _run(main, REM, "--path", str(wt)) + assert res.returncode == 3 and "'unpushed'" in res.stderr, res.stdout + res.stderr + assert os.path.isdir(gd) + assert _git(gd, "rev-parse", "saved-work^{commit}").stdout.strip() == local + + +@pytest.mark.parametrize("deinit", [False, True]) +def test_w6l_tag_only_submodule_commit_is_kept(ctl_sub, deinit): + """R6: HEAD and --branches miss a commit only a tag reaches; both checks + (initialised checkout via foreach, retained repository via --git-dir) use + the same rev list and refuse.""" + _remove_refuses_tag_only_commit(ctl_sub, annotated=False, deinit=deinit) + + +def test_w6m_annotated_tag_only_submodule_commit_is_kept(ctl_sub): + _remove_refuses_tag_only_commit(ctl_sub, annotated=True, deinit=False)