diff --git a/bin/worktree.sh b/bin/worktree.sh index 04a442c..e2a2928 100644 --- a/bin/worktree.sh +++ b/bin/worktree.sh @@ -306,13 +306,23 @@ worktree_remove() { # Used by conflict-guard steps. A task is "claimed" if its entry has a # `worktree:` field; legacy non-worktree tasks are emitted with worktree-path # set to the literal string "-". -worktree_list_claimed() { +# worktree_registry_path +# Echoes the resolved PyAutoMind/active.md, or returns 3 (printing nothing) when +# it cannot be found. Split out so a caller can tell "nothing is claimed" apart +# from "I could not read the registry" — see worktree_check_conflict. +worktree_registry_path() { local active="$PYAUTO_MAIN/PyAutoMind/active.md" # Back-compat: fall back to the pre-rename PyAutoPrompt/ path if present. [[ -f "$active" ]] || active="$PYAUTO_MAIN/PyAutoPrompt/active.md" - if [[ ! -f "$active" ]]; then - return 0 - fi + [[ -f "$active" ]] || return 3 + printf '%s\n' "$active" +} + +worktree_list_claimed() { + local active + # Returns 3, NOT 0, when the registry is unreadable — empty output must never + # be mistaken for "nothing is claimed". + active="$(worktree_registry_path)" || return 3 awk ' # Claim rows are buffered and flushed at the end of each task entry: # `worktree:` may appear either side of the `repos:` block, so it is not @@ -363,13 +373,41 @@ worktree_list_claimed() { ' "$active" } -# worktree_check_conflict [repo2 ...] +# worktree_check_conflict [--allow-missing-registry] [repo2 ...] # Exits 0 if none of the requested repos are claimed by a different task. # Exits 1 and prints the conflicts to stderr otherwise. +# Exits 3 when the registry cannot be resolved — see below. +# +# THIS GUARD FAILS CLOSED. It used to return 0 when `active.md` could not be +# found, which meant a cloud/web/CI session (where the roots are not under the +# default $HOME/Code/PyAutoLabs) got "no conflict" from a guard that had read +# nothing. That is worse than no guard, because the workflow documents this +# call and the skills act on its answer: two sessions could each be told the +# same repo was free. It now reports the failure and returns non-zero, so the +# caller stops instead of proceeding on a green light it never earned. worktree_check_conflict() { + local allow_missing=0 + if [[ "${1:-}" == "--allow-missing-registry" ]]; then + allow_missing=1 + shift + fi local task="$1" shift local want repo existing_task existing_repo existing_branch existing_wt rc=0 + + if ! worktree_registry_path >/dev/null; then + if (( allow_missing )); then + echo "worktree_check_conflict: no registry under \$PYAUTO_MAIN — proceeding UNGUARDED (--allow-missing-registry)" >&2 + return 0 + fi + echo "worktree_check_conflict: CANNOT VERIFY — no active.md found." >&2 + echo " tried: $PYAUTO_MAIN/PyAutoMind/active.md" >&2 + echo " $PYAUTO_MAIN/PyAutoPrompt/active.md" >&2 + echo " PYAUTO_MAIN=${PYAUTO_MAIN:-}" >&2 + echo " Set PYAUTO_MAIN to the directory holding your PyAutoMind checkout," >&2 + echo " or pass --allow-missing-registry to proceed without the guard." >&2 + return 3 + fi for want in "$@"; do while IFS=$'\t' read -r existing_task existing_repo existing_branch existing_wt; do if [[ "$existing_repo" == "$want" && "$existing_task" != "$task" ]]; then diff --git a/skills/WORKFLOW.md b/skills/WORKFLOW.md index ec8e61f..c9d0af9 100644 --- a/skills/WORKFLOW.md +++ b/skills/WORKFLOW.md @@ -223,6 +223,13 @@ Task worktrees keep parallel work isolated (`PyAutoBrain/bin/worktree.sh`): **dev workflow's own git mechanics** — feature-development work, **not** Build. Build is reached only for the release/packaging step (PyPI/tags/notebooks). +`worktree_check_conflict` **fails closed**: it reads `active.md` under +`$PYAUTO_MAIN` (default `$HOME/Code/PyAutoLabs`) and exits `3` with +`CANNOT VERIFY` when that registry cannot be resolved, instead of reporting +"no conflict" from a read that never happened (#225). In `web-github` / `ci-only` +environments set `PYAUTO_MAIN` to the directory holding the PyAutoMind checkout, +or pass `--allow-missing-registry` to proceed knowingly unguarded. + ## Repo → GitHub owner mapping diff --git a/skills/start_dev/reference.md b/skills/start_dev/reference.md index fa62e16..d36bdb9 100644 --- a/skills/start_dev/reference.md +++ b/skills/start_dev/reference.md @@ -105,6 +105,8 @@ PyAutoMind. Shared organ boundary and the execution-environment model are in This prints one line per `(task, repo, branch, worktree_path)` quadruple currently registered in `active.md`. For each affected repo the new plan wants to touch, check whether a different task already claims it via a `worktree:` field. If so, flag it as a **hard conflict** — the new task cannot start until the other one ships. + **The guard fails closed (#225).** `worktree_check_conflict` resolves `active.md` under `$PYAUTO_MAIN` (default `$HOME/Code/PyAutoLabs`). When it cannot find the registry it exits **3** with `CANNOT VERIFY`, rather than reporting "no conflict" from a read that never happened — outside local-dev, set `PYAUTO_MAIN` to the directory holding your PyAutoMind checkout. `--allow-missing-registry` proceeds **unguarded** and says so; use it only when you have confirmed by other means that nothing else claims the repos. Exit `1` is a real conflict; `0` is genuinely clear. + Then, for each affected repo, also run: ```bash git -C branch --sort=-committerdate | head -5 diff --git a/tests/test_skill_install.py b/tests/test_skill_install.py index 8f31ea6..5dd3394 100644 --- a/tests/test_skill_install.py +++ b/tests/test_skill_install.py @@ -45,11 +45,33 @@ def test_local_skill_links_resolve(): assert broken == [] +def _pyauto_root(tmp_path): + """A fixture PYAUTO_ROOT whose `PyAutoBrain/` is this checkout. + + Without this the installer falls back to `DEFAULT_PYAUTO_ROOT` + (`bin/../..`, i.e. this repo's grandparent) and looks for + `/PyAutoBrain/skills`. That resolves only when the checkout + happens to be *named* `PyAutoBrain` and sits one level under the root — + true on a laptop, false for a clone at any other path (a cloud session + cloning to `pyautobrain` finds nothing, so the Brain skills are never + scanned and the assertions below have nothing to assert on). + + Pinning the root makes these tests depend on the installer's behaviour + rather than on where the repo happens to be checked out. Same pattern as + `test_invalid_codex_name_does_not_suppress_claude_surfaces` below. + """ + root = tmp_path / "PyAutoLabs" + root.mkdir(parents=True, exist_ok=True) + (root / "PyAutoBrain").symlink_to(BRAIN_HOME, target_is_directory=True) + return root + + def test_installer_keeps_commands_and_installs_both_skill_homes(tmp_path): claude_home = tmp_path / "claude" codex_home = tmp_path / "codex" env = os.environ | { "HOME": str(tmp_path / "home"), + "PYAUTO_ROOT": str(_pyauto_root(tmp_path)), "CLAUDE_HOME": str(claude_home), "CODEX_HOME": str(codex_home), } @@ -85,6 +107,7 @@ def test_installer_preserves_non_symlink_destinations(tmp_path): marker.write_text("keep\n") env = os.environ | { "HOME": str(tmp_path / "home"), + "PYAUTO_ROOT": str(_pyauto_root(tmp_path)), "CLAUDE_HOME": str(claude_home), "CODEX_HOME": str(codex_home), } diff --git a/tests/test_worktree_conflict_guard.py b/tests/test_worktree_conflict_guard.py index 2fb4587..c6f8f78 100644 --- a/tests/test_worktree_conflict_guard.py +++ b/tests/test_worktree_conflict_guard.py @@ -200,7 +200,37 @@ def test_entry_claiming_no_repos_yields_no_claims(tmp_path): assert _claims(tmp_path, NO_CLAIMS) == [] -def test_missing_active_md_yields_no_claims(tmp_path): +def test_missing_active_md_reports_failure_rather_than_no_claims(tmp_path): + """A missing registry is NOT the same as "nothing is claimed". + + This test previously asserted the opposite — `returncode == 0` with empty + output — and that pinned behaviour turned out to BE the defect (#225). + `worktree_check_conflict` consumes this function, so returning 0 meant a + session whose roots are not under `$PYAUTO_MAIN` got "no conflict" from a + guard that had read nothing. Empty output must never be mistaken for a + clean registry, so the listing now signals the failure in its exit code. + """ proc = _run(tmp_path, None, "worktree_list_claimed") + assert proc.returncode != 0 + assert proc.stdout == "" # still emits no bogus rows + + +def test_conflict_guard_fails_closed_when_the_registry_is_missing(tmp_path): + """The defect, at the level the skills actually call. + + Documented in `start_dev` step 6 and `start_library` step 1, and its answer + decides whether a task may start — so "I could not check" has to stop the + caller, not wave it through. + """ + proc = _run(tmp_path, None, "worktree_check_conflict some-task PyAutoFit") + assert proc.returncode != 0 + assert "CANNOT VERIFY" in proc.stderr + assert "PYAUTO_MAIN" in proc.stderr # names what to set + + +def test_conflict_guard_can_be_forced_past_a_missing_registry(tmp_path): + """The escape hatch is explicit and loud — never the default.""" + proc = _run(tmp_path, None, + "worktree_check_conflict --allow-missing-registry t PyAutoFit") assert proc.returncode == 0 - assert proc.stdout == "" + assert "UNGUARDED" in proc.stderr