Skip to content

Commit 9a43f00

Browse files
authored
fix: make the worktree conflict guard fail closed; unpin the installer tests (#226)
Two of the three defects in #225, all one root cause: helpers resolving paths under $HOME/Code/PyAutoLabs, which no cloud/web/CI session has. worktree_check_conflict FAILED OPEN. worktree_list_claimed returned 0 with empty output when active.md could not be resolved under $PYAUTO_MAIN, so the guard could not tell "nothing claims this repo" from "I could not read the registry" — and answered the former. start_dev step 6 and start_library step 1 both document this call and act on its answer, so a session whose roots are not at the default path got a green light it never earned. Reproduced during #224: a conflict check was recorded as clean having read nothing. Split into worktree_registry_path so the listing can signal failure in its exit code; the guard now reports CANNOT VERIFY with the paths it tried and returns 3. --allow-missing-registry proceeds unguarded and says so, never by default. test_missing_active_md_yields_no_claims asserted exactly the fail-open behaviour, so it is rewritten to the corrected contract with the reason recorded — it was pinning the defect. The sibling worktree_claim_is_stale is documented fail-open by design and is untouched: it resolves active.md itself. Both fail toward safety. test_skill_install.py: two tests depended on the checkout being NAMED PyAutoBrain one level under PYAUTO_ROOT (default bin/../..). They now pin a fixture root, testing the installer rather than the checkout layout. This is what made the ship_library fallback gate spuriously RED on #224. pytest tests/ is now 331 passed with nothing ignored — green in a cloud session. Closes #225
1 parent f518b24 commit 9a43f00

5 files changed

Lines changed: 107 additions & 7 deletions

File tree

bin/worktree.sh

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -306,13 +306,23 @@ worktree_remove() {
306306
# Used by conflict-guard steps. A task is "claimed" if its entry has a
307307
# `worktree:` field; legacy non-worktree tasks are emitted with worktree-path
308308
# set to the literal string "-".
309-
worktree_list_claimed() {
309+
# worktree_registry_path
310+
# Echoes the resolved PyAutoMind/active.md, or returns 3 (printing nothing) when
311+
# it cannot be found. Split out so a caller can tell "nothing is claimed" apart
312+
# from "I could not read the registry" — see worktree_check_conflict.
313+
worktree_registry_path() {
310314
local active="$PYAUTO_MAIN/PyAutoMind/active.md"
311315
# Back-compat: fall back to the pre-rename PyAutoPrompt/ path if present.
312316
[[ -f "$active" ]] || active="$PYAUTO_MAIN/PyAutoPrompt/active.md"
313-
if [[ ! -f "$active" ]]; then
314-
return 0
315-
fi
317+
[[ -f "$active" ]] || return 3
318+
printf '%s\n' "$active"
319+
}
320+
321+
worktree_list_claimed() {
322+
local active
323+
# Returns 3, NOT 0, when the registry is unreadable — empty output must never
324+
# be mistaken for "nothing is claimed".
325+
active="$(worktree_registry_path)" || return 3
316326
awk '
317327
# Claim rows are buffered and flushed at the end of each task entry:
318328
# `worktree:` may appear either side of the `repos:` block, so it is not
@@ -363,13 +373,41 @@ worktree_list_claimed() {
363373
' "$active"
364374
}
365375

366-
# worktree_check_conflict <task-name> <repo1> [repo2 ...]
376+
# worktree_check_conflict [--allow-missing-registry] <task-name> <repo1> [repo2 ...]
367377
# Exits 0 if none of the requested repos are claimed by a different task.
368378
# Exits 1 and prints the conflicts to stderr otherwise.
379+
# Exits 3 when the registry cannot be resolved — see below.
380+
#
381+
# THIS GUARD FAILS CLOSED. It used to return 0 when `active.md` could not be
382+
# found, which meant a cloud/web/CI session (where the roots are not under the
383+
# default $HOME/Code/PyAutoLabs) got "no conflict" from a guard that had read
384+
# nothing. That is worse than no guard, because the workflow documents this
385+
# call and the skills act on its answer: two sessions could each be told the
386+
# same repo was free. It now reports the failure and returns non-zero, so the
387+
# caller stops instead of proceeding on a green light it never earned.
369388
worktree_check_conflict() {
389+
local allow_missing=0
390+
if [[ "${1:-}" == "--allow-missing-registry" ]]; then
391+
allow_missing=1
392+
shift
393+
fi
370394
local task="$1"
371395
shift
372396
local want repo existing_task existing_repo existing_branch existing_wt rc=0
397+
398+
if ! worktree_registry_path >/dev/null; then
399+
if (( allow_missing )); then
400+
echo "worktree_check_conflict: no registry under \$PYAUTO_MAIN — proceeding UNGUARDED (--allow-missing-registry)" >&2
401+
return 0
402+
fi
403+
echo "worktree_check_conflict: CANNOT VERIFY — no active.md found." >&2
404+
echo " tried: $PYAUTO_MAIN/PyAutoMind/active.md" >&2
405+
echo " $PYAUTO_MAIN/PyAutoPrompt/active.md" >&2
406+
echo " PYAUTO_MAIN=${PYAUTO_MAIN:-<unset>}" >&2
407+
echo " Set PYAUTO_MAIN to the directory holding your PyAutoMind checkout," >&2
408+
echo " or pass --allow-missing-registry to proceed without the guard." >&2
409+
return 3
410+
fi
373411
for want in "$@"; do
374412
while IFS=$'\t' read -r existing_task existing_repo existing_branch existing_wt; do
375413
if [[ "$existing_repo" == "$want" && "$existing_task" != "$task" ]]; then

skills/WORKFLOW.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,13 @@ Task worktrees keep parallel work isolated (`PyAutoBrain/bin/worktree.sh`):
223223
**dev workflow's own git mechanics** — feature-development work, **not** Build.
224224
Build is reached only for the release/packaging step (PyPI/tags/notebooks).
225225

226+
`worktree_check_conflict` **fails closed**: it reads `active.md` under
227+
`$PYAUTO_MAIN` (default `$HOME/Code/PyAutoLabs`) and exits `3` with
228+
`CANNOT VERIFY` when that registry cannot be resolved, instead of reporting
229+
"no conflict" from a read that never happened (#225). In `web-github` / `ci-only`
230+
environments set `PYAUTO_MAIN` to the directory holding the PyAutoMind checkout,
231+
or pass `--allow-missing-registry` to proceed knowingly unguarded.
232+
226233
## Repo → GitHub owner mapping
227234

228235
<!-- repos_sync:begin -->

skills/start_dev/reference.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,8 @@ PyAutoMind. Shared organ boundary and the execution-environment model are in
105105

106106
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.
107107

108+
**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.
109+
108110
Then, for each affected repo, also run:
109111
```bash
110112
git -C <repo_path> branch --sort=-committerdate | head -5

tests/test_skill_install.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,33 @@ def test_local_skill_links_resolve():
4545
assert broken == []
4646

4747

48+
def _pyauto_root(tmp_path):
49+
"""A fixture PYAUTO_ROOT whose `PyAutoBrain/` is this checkout.
50+
51+
Without this the installer falls back to `DEFAULT_PYAUTO_ROOT`
52+
(`bin/../..`, i.e. this repo's grandparent) and looks for
53+
`<grandparent>/PyAutoBrain/skills`. That resolves only when the checkout
54+
happens to be *named* `PyAutoBrain` and sits one level under the root —
55+
true on a laptop, false for a clone at any other path (a cloud session
56+
cloning to `pyautobrain` finds nothing, so the Brain skills are never
57+
scanned and the assertions below have nothing to assert on).
58+
59+
Pinning the root makes these tests depend on the installer's behaviour
60+
rather than on where the repo happens to be checked out. Same pattern as
61+
`test_invalid_codex_name_does_not_suppress_claude_surfaces` below.
62+
"""
63+
root = tmp_path / "PyAutoLabs"
64+
root.mkdir(parents=True, exist_ok=True)
65+
(root / "PyAutoBrain").symlink_to(BRAIN_HOME, target_is_directory=True)
66+
return root
67+
68+
4869
def test_installer_keeps_commands_and_installs_both_skill_homes(tmp_path):
4970
claude_home = tmp_path / "claude"
5071
codex_home = tmp_path / "codex"
5172
env = os.environ | {
5273
"HOME": str(tmp_path / "home"),
74+
"PYAUTO_ROOT": str(_pyauto_root(tmp_path)),
5375
"CLAUDE_HOME": str(claude_home),
5476
"CODEX_HOME": str(codex_home),
5577
}
@@ -85,6 +107,7 @@ def test_installer_preserves_non_symlink_destinations(tmp_path):
85107
marker.write_text("keep\n")
86108
env = os.environ | {
87109
"HOME": str(tmp_path / "home"),
110+
"PYAUTO_ROOT": str(_pyauto_root(tmp_path)),
88111
"CLAUDE_HOME": str(claude_home),
89112
"CODEX_HOME": str(codex_home),
90113
}

tests/test_worktree_conflict_guard.py

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,37 @@ def test_entry_claiming_no_repos_yields_no_claims(tmp_path):
200200
assert _claims(tmp_path, NO_CLAIMS) == []
201201

202202

203-
def test_missing_active_md_yields_no_claims(tmp_path):
203+
def test_missing_active_md_reports_failure_rather_than_no_claims(tmp_path):
204+
"""A missing registry is NOT the same as "nothing is claimed".
205+
206+
This test previously asserted the opposite — `returncode == 0` with empty
207+
output — and that pinned behaviour turned out to BE the defect (#225).
208+
`worktree_check_conflict` consumes this function, so returning 0 meant a
209+
session whose roots are not under `$PYAUTO_MAIN` got "no conflict" from a
210+
guard that had read nothing. Empty output must never be mistaken for a
211+
clean registry, so the listing now signals the failure in its exit code.
212+
"""
204213
proc = _run(tmp_path, None, "worktree_list_claimed")
214+
assert proc.returncode != 0
215+
assert proc.stdout == "" # still emits no bogus rows
216+
217+
218+
def test_conflict_guard_fails_closed_when_the_registry_is_missing(tmp_path):
219+
"""The defect, at the level the skills actually call.
220+
221+
Documented in `start_dev` step 6 and `start_library` step 1, and its answer
222+
decides whether a task may start — so "I could not check" has to stop the
223+
caller, not wave it through.
224+
"""
225+
proc = _run(tmp_path, None, "worktree_check_conflict some-task PyAutoFit")
226+
assert proc.returncode != 0
227+
assert "CANNOT VERIFY" in proc.stderr
228+
assert "PYAUTO_MAIN" in proc.stderr # names what to set
229+
230+
231+
def test_conflict_guard_can_be_forced_past_a_missing_registry(tmp_path):
232+
"""The escape hatch is explicit and loud — never the default."""
233+
proc = _run(tmp_path, None,
234+
"worktree_check_conflict --allow-missing-registry t PyAutoFit")
205235
assert proc.returncode == 0
206-
assert proc.stdout == ""
236+
assert "UNGUARDED" in proc.stderr

0 commit comments

Comments
 (0)