fix(issue-21): PR #29 review R1-R3 - untracked-safe cleanliness checks; a worktree gets its own venv
R1/R2: `git status --porcelain` honours status.showUntrackedFiles, so a `no` setting in a submodule (or the worktree) hid untracked files from the checks that justify worktree_remove.sh's single --force and from the review snapshot guard (_dirty_submodules), which then approved content it never captured. All three checks (lib_py/worktree.py, worktree_remove.sh, runner._dirty_submodules) now pass --untracked-files=all. The source tree's own snapshot was already safe: `git add -A` ignores that setting. R3: worktree_prepare.sh no longer links .venv to the control root's. It creates the worktree's own venv with the control root's interpreter and installs requirements*.txt; a leftover link is refused (exit 1), a control root without an interpreter is a warning (VENV=skipped); VENV= is printed. Tests: w1 (real venv, kept on re-run), w1b (link refused / skipped), w6e parametrised over the hidden setting for the post-stop recheck, w6e2 (hidden untracked file in the submodule and in the worktree blocks the removal), l17 untracked-hidden / during-hidden (guard refuses before the first review; a mid-round hidden file makes the round stale). Each new case fails without the fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
This commit is contained in:
1 parent
90a289da44
commit
08db1da9d5
9 files changed
+107
-29
No files matched your search
@@ -712,9 +712,10 @@ class LoopRunner:
|
||||
def _dirty_submodules(self) -> Optional[List[str]]:
|
||||
"""Checked-out submodules (recursively) with uncommitted or untracked
|
||||
content; None when the check itself fails. Each submodule is asked
|
||||
directly with --ignore-submodules=none, so no submodule.*.ignore or
|
||||
diff.ignoreSubmodules setting can hide it."""
|
||||
script = ('s=$(git status --porcelain --ignore-submodules=none) || exit 1; '
|
||||
directly with --ignore-submodules=none and --untracked-files=all, so no
|
||||
submodule.*.ignore, diff.ignoreSubmodules or status.showUntrackedFiles
|
||||
setting can hide it."""
|
||||
script = ('s=$(git status --porcelain --untracked-files=all --ignore-submodules=none) || exit 1; '
|
||||
'[ -z "$s" ] || printf "%s\\n" "$displaypath"')
|
||||
r = self._git(self.worktree_root, "submodule", "foreach", "--recursive", "--quiet", script)
|
||||
if r.returncode != 0:
|
||||
|
||||
@@ -155,7 +155,10 @@ def submodule_findings(wt: str) -> Dict[str, object]:
|
||||
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."""
|
||||
them. --untracked-files=all: `status.showUntrackedFiles=no` in a
|
||||
submodule would otherwise hide its untracked files from the check (review
|
||||
R1), and display settings must not decide what a forced removal may
|
||||
delete."""
|
||||
gd = _git(wt, "rev-parse", "--absolute-git-dir")
|
||||
if gd.returncode != 0 or not gd.stdout.strip():
|
||||
return {"trace": False, "dirty": "", "unpushed": "", "stashes": "",
|
||||
@@ -164,7 +167,7 @@ def submodule_findings(wt: str) -> Dict[str, object]:
|
||||
"dirty": "", "unpushed": "", "stashes": "", "error": ""}
|
||||
if not res["trace"]:
|
||||
return res
|
||||
checks = (("dirty", "git status --porcelain"),
|
||||
checks = (("dirty", "git status --porcelain --untracked-files=all"),
|
||||
("unpushed", "git log --oneline " + " ".join(UNPUSHED_REVS)),
|
||||
("stashes", "git stash list"))
|
||||
errors = []
|
||||
|
||||
@@ -243,7 +243,7 @@ bash .agents/skills/multi-agent-mux-create/scripts/worktree_prepare.sh \
|
||||
[--creator-agent claude [--session <name>] [--role creator] [--herdr-session <s>] [--herdr-workspace <w>] [--onboard]] [--dry-run]
|
||||
```
|
||||
|
||||
- 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).
|
||||
- Creates the worktree next to the control root (a path **inside** it is refused), idempotently; creates the worktree's **own** `.venv` from the control root's `.venv/bin/python` and installs `requirements.txt` / `requirements-dev.txt` when present (never a link - a leftover link is refused with exit 1; no control-root interpreter is a warning and `VENV=skipped`); 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` 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`.
|
||||
|
||||
@@ -8,8 +8,8 @@
|
||||
# [--creator-agent <agent> [--session <name>] [--role <role>]
|
||||
# [--herdr-session <s>] [--herdr-workspace <w>] [--onboard]] [--dry-run]
|
||||
#
|
||||
# Steps: git worktree (idempotent) -> .venv link to the control root's ->
|
||||
# /.venv and .pytest_cache/ in the shared info/exclude -> submodules only with
|
||||
# Steps: git worktree (idempotent) -> the worktree's own .venv (control root
|
||||
# interpreter + requirements*.txt) -> /.venv and .pytest_cache/ in the shared info/exclude -> submodules only with
|
||||
# --submodules (a failed init is rolled back: it would otherwise leave a
|
||||
# modules/ trace that makes a plain `git worktree remove` refuse) -> optional
|
||||
# creator session registered in the control root's .mam -> summary with BASE=.
|
||||
@@ -79,7 +79,7 @@ fi
|
||||
if [ "$DRY_RUN" = "1" ]; then
|
||||
echo "[dry-run] control root: $MAIN"
|
||||
echo "[dry-run] worktree: $WT (branch $BRANCH${BASE_REF:+, base $BASE_REF})"
|
||||
echo "[dry-run] .venv link and shared info/exclude entries; submodules: $([ "$SUBMODULES" = 1 ] && echo init || echo skipped)"
|
||||
echo "[dry-run] own .venv (from $MAIN/.venv/bin/python + requirements*.txt) and shared info/exclude entries; submodules: $([ "$SUBMODULES" = 1 ] && echo init || echo skipped)"
|
||||
[ -n "$CREATOR_AGENT" ] && echo "[dry-run] creator: $CREATOR_AGENT as $SESSION (role $ROLE)"
|
||||
exit 0
|
||||
fi
|
||||
@@ -108,13 +108,30 @@ else
|
||||
fi
|
||||
fi
|
||||
|
||||
# 2. .venv: a link to the control root's, never a copy.
|
||||
if [ -e "$MAIN/.venv" ]; then
|
||||
if [ ! -e "$WT/.venv" ] && [ ! -L "$WT/.venv" ]; then
|
||||
ln -s "$MAIN/.venv" "$WT/.venv"
|
||||
elif [ "$(readlink "$WT/.venv" 2>/dev/null || true)" != "$MAIN/.venv" ]; then
|
||||
echo "WARNING: '$WT/.venv' exists and is not a link to '$MAIN/.venv'; left as it is" >&2
|
||||
# 2. .venv: the worktree's own environment, created with the control root's
|
||||
# interpreter and its requirements files, never a link (review R3: a link
|
||||
# aliases the control root's environment, and a `git add -A` in the
|
||||
# worktree once committed the link itself - see .gitignore's managed
|
||||
# block). Idempotent: an existing usable venv is kept.
|
||||
VENV_STATE="skipped"
|
||||
if [ -L "$WT/.venv" ]; then
|
||||
echo "ERROR: '$WT/.venv' is a symbolic link; a worktree needs its own venv - remove the link and re-run" >&2
|
||||
exit 1
|
||||
elif [ -x "$WT/.venv/bin/python" ]; then
|
||||
VENV_STATE="kept"
|
||||
elif [ -x "$MAIN/.venv/bin/python" ]; then
|
||||
"$MAIN/.venv/bin/python" -m venv "$WT/.venv" || { echo "ERROR: could not create '$WT/.venv'" >&2; exit 1; }
|
||||
req=()
|
||||
for f in requirements.txt requirements-dev.txt; do
|
||||
[ -f "$WT/$f" ] && req+=(-r "$WT/$f")
|
||||
done
|
||||
if [ "${#req[@]}" -gt 0 ]; then
|
||||
"$WT/.venv/bin/python" -m pip install -q "${req[@]}" \
|
||||
|| { echo "ERROR: pip install into '$WT/.venv' failed; fix it and re-run (the worktree is kept)" >&2; exit 1; }
|
||||
fi
|
||||
VENV_STATE="created"
|
||||
else
|
||||
echo "WARNING: '$MAIN/.venv/bin/python' not found; no venv created in the worktree" >&2
|
||||
fi
|
||||
|
||||
# 3. Ignore rules. A linked worktree's info/exclude is the shared
|
||||
@@ -228,6 +245,7 @@ echo "WORKTREE=$WT"
|
||||
echo "BRANCH=$BRANCH"
|
||||
echo "BASE=$BASE_SHA"
|
||||
echo "SUBMODULES=$SUB_STATE"
|
||||
echo "VENV=$VENV_STATE"
|
||||
[ -n "$CREATOR_AGENT" ] && echo "SESSION=$SESSION"
|
||||
echo "next: bash $SKILL_DIR/multi-agent-mux-loop/scripts/run_loop.sh --worktree '$WT' --base $BASE_SHA --review-only --creator <creator> --reviewer <reviewers> --task '...'"
|
||||
echo "then: bash $SKILL_DIR/multi-agent-mux-stop/scripts/worktree_remove.sh --path '$WT' --stop"
|
||||
@@ -120,7 +120,7 @@ Every check runs first and changes nothing; a refusal exits 3:
|
||||
1. a linked worktree of the control root, not locked (`git worktree unlock` first - git refuses even `--force` on a locked one);
|
||||
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`);
|
||||
4. a clean tree, submodules included (`status --porcelain --untracked-files=all --ignore-submodules=none` - a `status.showUntrackedFiles=no` setting cannot hide a file from a check that decides a forced removal);
|
||||
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.
|
||||
|
||||
@@ -84,11 +84,13 @@ 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 from what it saw and returns 1 on any finding. --untracked-files=all:
|
||||
# a status.showUntrackedFiles=no setting (worktree or submodule) must not hide
|
||||
# files from a check that decides a forced removal (review R1).
|
||||
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
|
||||
if ! dirty="$(git -C "$WT" status --porcelain --untracked-files=all --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
|
||||
|
||||
Reference in new issue
Block a user