allod/tools

Watch

Fork 0

change: always isolate a -d change in its own worktree, and add 'change list' #125

Merged

vnprc merged 3 commits from agent/always-isolate into master

2026-07-28 03:57:16 +01:00

allod-agent commented

2026-07-28 03:05:56 +01:00

Member

Closes allod/tools#116
Refs allod/tools#115

Implements allod/strategy dev-plans/allod-change-always-isolate.md, inheriting contracts C1–C5 from dev-plans/concurrent-agent-workspace.md.

What changed

begin -d creates a worktree and an agent/<description> branch for every repo, protected or not, sited at ~/changes/<slug>-<description>-XXXXXX instead of /tmp (C1, C4). A protected-branches entry still names the base for the repos that have one; every repo without one bases on origin/HEAD. Protected repos keep their base selection unchanged but do pick up the two new preflights, since the plan applies them to every repo: an origin check, and a post-fetch verification that origin/<base> resolves — which turns a missing base from a raw 128 out of worktree add, with a temp directory already created, into exit 1 before anything is created.
begin without -d is unchanged: it prints the shared checkout path and creates nothing — no fetch, no branch, no directory. Protected repos still refuse it with exit 1 and a message naming -d. C4 removes protection as the isolation switch, not as a validation input, and an in-place commit is legitimate only on a repo's default branch, so if that branch is protected no in-place change in that checkout is legitimate at all.
begin writes the branch it created to allod-change-branch in the new worktree's private git dir (C3), for allod/tools#118's reader.
Failure handling for worktree add and for the handoff write removes the directory and prunes the admin entry, then reports rather than guesses: it never deletes agent/<description>, because under concurrency another agent may have created it between the pre-check and the add, and it checks whether the entry is actually gone rather than trusting worktree prune, which reports a failed delete and still exits 0.
New read-only allod change list [<repo-path>] prints one tab-separated row per linked worktree — repo, path, branch, state — enumerated from git rather than from a path, so it finds the /tmp worktrees that predate ~/changes. It reports the strongest blocker only: prunable, locked, detached, submodule, dirty, unpushed, clean. The binding contract is that clean is reported if and only if cleanup on that path would succeed, which means the negation of every one of cleanup's refusal sources rather than the two it names — a detached or locked worktree looks clean by the dirty and unpushed checks alone.
Plan correction: cleanup has six refusal sources, not the five the plan enumerates. git worktree remove also refuses a worktree containing a populated submodule, so one would have been reported clean and then died 128. Found by the review pass below and reproduced. list reports submodule for it; a gitlink whose directory holds no .git is unpopulated and removes fine, which is the state begin itself leaves behind, so both the fire and the no-fire case are asserted. No repo in the workspace uses submodules today, so this was latent.
Nothing is ever reclaimed implicitly. There is no --force, no --all, and no age heuristic, because no local signal distinguishes a dead agent from a working one. cleanup is untouched and still refuses everything it refused before.
README.md gains "Making a change" and "Reclaiming worktrees" under Workflow; change --help states the -d contract and the state words.

Risk

R2 Medium, as triaged in the plan. One CLI, one repo, one rewritten function plus one new read-only one. git worktree add never moves the shared checkout's HEAD, index, or working tree, which is what makes the flip safe to run while another agent is mid-write in the same repo, and that is asserted directly. begin still reaches the network only through ls-remote and fetch, both reads; publication risk is unchanged. Rollback is a straight revert: worktrees already created under ~/changes stay valid linked worktrees, record and submit operate on $PWD and work inside them unchanged, and cleanup still reclaims them.

Worth a human eye, in the plan's order: the -d-absent branch of change_begin, which must stay byte-for-byte frictionless for allod/memory and allod/strategy; the refusal to delete a branch begin cannot prove it created; and the list state words against cleanup's refusals, locked and detached in particular.

Validation

bash tests/allod-change.sh — 159 assertions, all passing, up from 79. New coverage: begin -d on an unprotected repo (the flip, previously untested); the in-place flow proven to create no worktree, no branch and no ~/changes at all, then to commit and push in place; both duplicate-description rails on the flipped path; the C3 handoff file and the linked-worktree git dir; both rollback paths through a mock git; the origin, origin/HEAD and post-fetch base preflights; one case per list state checked by running cleanup on the same worktree rather than by restating its refusals; and four precedence assertions, one per adjacent pair.

Every new assertion was checked against a sabotaged implementation, since a guard that cannot be shown to fail does not count. Sixteen sabotages — reordered state precedence, prunable computed from [[ -d ]] instead of git's annotation, dropped worktree prune, dropped handoff write, skipped base verification, restored unprotected passthrough, two-condition clean, a rollback that deletes the branch, a rollback that keeps the directory, no origin preflight, a list that prunes, a list that skips directory-less worktrees, /tmp siting, no submodule detection, submodule detection that fires on unpopulated gitlinks, and a list that prints no rows at all — were each caught by the intended assertion.

./allod change list against the live workspace reports both /tmp worktrees still registered against allod/tools, one clean and one dirty, which is the direct demonstration that enumeration is git-derived rather than path-derived.

Nothing here needs a rebuild to validate; the tests invoke the checkout directly and build their own repos under a temporary HOME. begin -d was never run against the live workspace.

Correction to the plan

The plan and concurrent-agent-workspace.md both record workspace_repo_default_branch as having a dead || echo "master" fallback that returns empty when origin/HEAD is unset, on the grounds that the || binds to a pipeline whose status is sed's. That is true only without pipefail. All five consumers — allod, pull-all, work-diff, flake-status, flake-update-cascade — set set -euo pipefail, so the pipeline's status is git's and the fallback does fire. The helper therefore does not return empty; it silently guesses master.

That inverts the hazard begin inherits: not a confusing origin/ error, but a repo quietly branched off the wrong base. lib/workspace.sh is still untouched here, as the plan scopes it, and begin instead asserts its own precondition — symbolic-ref refs/remotes/origin/HEAD must resolve — before falling through to the helper, which gives the contracted remote set-head repair message. The empty-base guard stays as defence.

The helper itself is now issue allod/tools#126, which also records how narrow the live exposure is: git 2.51.2 sets refs/remotes/origin/HEAD on clone and repairs it on any fetch, so the unresolvable state mostly survives in a repo that has never fetched. begin's assertion becomes redundant once allod/tools#126 lands and can be dropped then. Both dev plans carry the incorrect reading of the helper and should be corrected with it.

Not in this PR

The allod/memory updates (memory.md:49, git-workflow.md:9, :13, :28, allod.md:62, and a list entry) are a separate commit in a separate repo, gated on the human rebuild rather than on this merge: allod resolves to a nix store path, so until the rebuild the installed tool does not implement what the docs would describe.

Closes allod/tools#116 Refs allod/tools#115 Implements `allod/strategy` `dev-plans/allod-change-always-isolate.md`, inheriting contracts C1–C5 from `dev-plans/concurrent-agent-workspace.md`. ## What changed - `begin -d` creates a worktree and an `agent/<description>` branch for every repo, protected or not, sited at `~/changes/<slug>-<description>-XXXXXX` instead of `/tmp` (C1, C4). A `protected-branches` entry still names the base for the repos that have one; every repo without one bases on `origin/HEAD`. Protected repos keep their base selection unchanged but do pick up the two new preflights, since the plan applies them to every repo: an `origin` check, and a post-fetch verification that `origin/<base>` resolves — which turns a missing base from a raw 128 out of `worktree add`, with a temp directory already created, into exit 1 before anything is created. - `begin` without `-d` is unchanged: it prints the shared checkout path and creates nothing — no fetch, no branch, no directory. Protected repos still refuse it with exit 1 and a message naming `-d`. C4 removes protection as the isolation switch, not as a validation input, and an in-place commit is legitimate only on a repo's default branch, so if that branch is protected no in-place change in that checkout is legitimate at all. - `begin` writes the branch it created to `allod-change-branch` in the new worktree's private git dir (C3), for allod/tools#118's reader. - Failure handling for `worktree add` and for the handoff write removes the directory and prunes the admin entry, then reports rather than guesses: it never deletes `agent/<description>`, because under concurrency another agent may have created it between the pre-check and the add, and it checks whether the entry is actually gone rather than trusting `worktree prune`, which reports a failed delete and still exits 0. - New read-only `allod change list [<repo-path>]` prints one tab-separated row per linked worktree — repo, path, branch, state — enumerated from git rather than from a path, so it finds the `/tmp` worktrees that predate `~/changes`. It reports the strongest blocker only: `prunable`, `locked`, `detached`, `submodule`, `dirty`, `unpushed`, `clean`. The binding contract is that `clean` is reported if and only if `cleanup` on that path would succeed, which means the negation of every one of `cleanup`'s refusal sources rather than the two it names — a detached or locked worktree looks clean by the dirty and unpushed checks alone. - **Plan correction: `cleanup` has six refusal sources, not the five the plan enumerates.** `git worktree remove` also refuses a worktree containing a populated submodule, so one would have been reported `clean` and then died 128. Found by the review pass below and reproduced. `list` reports `submodule` for it; a gitlink whose directory holds no `.git` is unpopulated and removes fine, which is the state `begin` itself leaves behind, so both the fire and the no-fire case are asserted. No repo in the workspace uses submodules today, so this was latent. - Nothing is ever reclaimed implicitly. There is no `--force`, no `--all`, and no age heuristic, because no local signal distinguishes a dead agent from a working one. `cleanup` is untouched and still refuses everything it refused before. - `README.md` gains "Making a change" and "Reclaiming worktrees" under Workflow; `change --help` states the `-d` contract and the state words. ## Risk R2 Medium, as triaged in the plan. One CLI, one repo, one rewritten function plus one new read-only one. `git worktree add` never moves the shared checkout's HEAD, index, or working tree, which is what makes the flip safe to run while another agent is mid-write in the same repo, and that is asserted directly. `begin` still reaches the network only through `ls-remote` and `fetch`, both reads; publication risk is unchanged. Rollback is a straight revert: worktrees already created under `~/changes` stay valid linked worktrees, `record` and `submit` operate on `$PWD` and work inside them unchanged, and `cleanup` still reclaims them. Worth a human eye, in the plan's order: the `-d`-absent branch of `change_begin`, which must stay byte-for-byte frictionless for `allod/memory` and `allod/strategy`; the refusal to delete a branch `begin` cannot prove it created; and the `list` state words against `cleanup`'s refusals, `locked` and `detached` in particular. ## Validation `bash tests/allod-change.sh` — 159 assertions, all passing, up from 79. New coverage: `begin -d` on an unprotected repo (the flip, previously untested); the in-place flow proven to create no worktree, no branch and no `~/changes` at all, then to commit and push in place; both duplicate-description rails on the flipped path; the C3 handoff file and the linked-worktree git dir; both rollback paths through a mock git; the `origin`, `origin/HEAD` and post-fetch base preflights; one case per `list` state checked by running `cleanup` on the same worktree rather than by restating its refusals; and four precedence assertions, one per adjacent pair. Every new assertion was checked against a sabotaged implementation, since a guard that cannot be shown to fail does not count. Sixteen sabotages — reordered state precedence, `prunable` computed from `[[ -d ]]` instead of git's annotation, dropped `worktree prune`, dropped handoff write, skipped base verification, restored unprotected passthrough, two-condition `clean`, a rollback that deletes the branch, a rollback that keeps the directory, no `origin` preflight, a `list` that prunes, a `list` that skips directory-less worktrees, `/tmp` siting, no submodule detection, submodule detection that fires on unpopulated gitlinks, and a `list` that prints no rows at all — were each caught by the intended assertion. `./allod change list` against the live workspace reports both `/tmp` worktrees still registered against `allod/tools`, one `clean` and one `dirty`, which is the direct demonstration that enumeration is git-derived rather than path-derived. Nothing here needs a rebuild to validate; the tests invoke the checkout directly and build their own repos under a temporary `HOME`. `begin -d` was never run against the live workspace. ## Correction to the plan The plan and `concurrent-agent-workspace.md` both record `workspace_repo_default_branch` as having a dead `|| echo "master"` fallback that returns empty when `origin/HEAD` is unset, on the grounds that the `||` binds to a pipeline whose status is `sed`'s. That is true only without `pipefail`. All five consumers — `allod`, `pull-all`, `work-diff`, `flake-status`, `flake-update-cascade` — set `set -euo pipefail`, so the pipeline's status is git's and the fallback does fire. The helper therefore does not return empty; it silently guesses `master`. That inverts the hazard `begin` inherits: not a confusing `origin/` error, but a repo quietly branched off the wrong base. `lib/workspace.sh` is still untouched here, as the plan scopes it, and `begin` instead asserts its own precondition — `symbolic-ref refs/remotes/origin/HEAD` must resolve — before falling through to the helper, which gives the contracted `remote set-head` repair message. The empty-base guard stays as defence. The helper itself is now issue allod/tools#126, which also records how narrow the live exposure is: git 2.51.2 sets `refs/remotes/origin/HEAD` on clone and repairs it on any fetch, so the unresolvable state mostly survives in a repo that has never fetched. `begin`'s assertion becomes redundant once allod/tools#126 lands and can be dropped then. Both dev plans carry the incorrect reading of the helper and should be corrected with it. ## Not in this PR The `allod/memory` updates (`memory.md:49`, `git-workflow.md:9`, `:13`, `:28`, `allod.md:62`, and a `list` entry) are a separate commit in a separate repo, gated on the human rebuild rather than on this merge: `allod` resolves to a nix store path, so until the rebuild the installed tool does not implement what the docs would describe.

allod-agent added 1 commit

2026-07-28 03:05:56 +01:00

change: isolate every -d change in its own worktree, and add 'change list' d03cf99090

'begin -d' now creates a worktree and an agent/<description> branch for every
repo, protected or not, sited under ~/changes rather than /tmp. Without -d it
still prints the shared checkout path and creates nothing; protected repos
still refuse that, because protection stays a validation input even though it
is no longer the isolation switch.

'begin' also writes the branch it created to allod-change-branch in the new
worktree's private git dir, and rolls the worktree back when the add or that
write fails - without deleting a branch it cannot prove it created.

'change list' is a new read-only subcommand that enumerates linked worktrees
from git rather than from a path, so it finds the /tmp worktrees that predate
~/changes, and reports the one thing blocking each one's removal. It reports
'clean' if and only if 'change cleanup' on that path would succeed.

allod-agent added 1 commit

2026-07-28 03:07:18 +01:00

tests: do not swallow fixture failures in the origin/HEAD helper 339393a42c

'remote set-head origin -d' is silent on success, so redirecting its output
only hid a failure that would abort the run with no explanation - and the
fixture it builds is the whole point of the test that consumes it. Also drop a
redundant non-zero-exit check now that the same case asserts exit 1 exactly.

allod-agent commented

2026-07-28 03:07:23 +01:00

Author

Member

Read-only self-review pass over the diff. Two findings, both in the test harness, fixed in 339393a:

init_repo_no_origin_head redirected remote set-head origin -d to /dev/null 2>&1. That command is silent on success, so the redirect could only ever hide a failure — and under set -e the run would then abort with no explanation, in the one fixture whose whole purpose is the origin/HEAD precondition test.
The protected-repo no--d case carried both a bare non-zero-exit check and assert_status 1. Redundant; the weaker one is gone.

Still outstanding: the plan sets this PR's review depth at one pass by a different model, which this self-review is not. That pass has not been run.

Read-only self-review pass over the diff. Two findings, both in the test harness, fixed in 339393a: - `init_repo_no_origin_head` redirected `remote set-head origin -d` to `/dev/null 2>&1`. That command is silent on success, so the redirect could only ever hide a failure — and under `set -e` the run would then abort with no explanation, in the one fixture whose whole purpose is the `origin/HEAD` precondition test. - The protected-repo no-`-d` case carried both a bare non-zero-exit check and `assert_status 1`. Redundant; the weaker one is gone. Still outstanding: the plan sets this PR's review depth at one pass by a *different* model, which this self-review is not. That pass has not been run.

allod-agent referenced this pull request

2026-07-28 03:14:31 +01:00

workspace_repo_default_branch: resolve the default branch or fail, never guess 'master' #126

allod-agent referenced this pull request

2026-07-28 03:28:53 +01:00

allod change cleanup deletes a branch when has_unpushed_commits cannot tell whether it has unpushed commits #127

allod-agent added 1 commit

2026-07-28 03:39:54 +01:00

change list: report a populated submodule, which cleanup cannot remove 90d68dffa8

'git worktree remove' refuses a worktree containing a populated submodule, so
it is a sixth cleanup blocker beyond the five the dev plan enumerates. Without
this, such a worktree reported 'clean' and cleanup then died 128 - a
counterexample to the binding contract that 'clean' means cleanup will succeed.
A gitlink whose directory holds no '.git' is unpopulated and removes fine,
which is the state begin itself leaves behind, so detection must not fire on
every worktree of a repo that merely has submodules; both cases are asserted.

Also: the one-row assertion counted a blank line as a row, so it passed for a
list that printed nothing. And 'prunable' does not mean the directory is gone,
only that the link to it broke, so the README no longer implies there is
nothing left to look at before deleting one by hand.

allod-agent commented

2026-07-28 03:41:10 +01:00

Author

Member

The contract violation, confirmed and reproduced independently. cleanup has six refusal sources, not the five the dev plan enumerates: git worktree remove also refuses a worktree containing a populated submodule. Such a worktree passes every check list was making — empty status --porcelain, nothing unpushed, not locked, detached, or prunable — so list printed clean and advised allod change cleanup <path>, which then died 128 with fatal: working trees containing submodules cannot be moved or removed and left the worktree in place. That is a direct counterexample to the binding contract, stated absolutely in the code comment, the README, and this PR body.

Fixed by reporting rather than by weakening the claim. list gains a submodule state, ranked with the other structural blockers above dirty. The detection is deliberately narrow: a gitlink whose directory holds no .git is unpopulated and removes fine, and unpopulated is exactly what begin itself leaves behind, so firing on any gitlink would falsely block every worktree of any repo that merely has submodules. Both the fire and the no-fire case are asserted, and sabotaging the detection in either direction — removing it, or making it fire on unpopulated gitlinks — is caught by the intended assertion.

Latent rather than live: no repo under ~/work has a .gitmodules, and the failure is loud and non-destructive when hit. Worth fixing anyway, because the contract is the part of this work that outlives the bash implementation.

A weak assertion. "list prints one row per linked worktree" counted lines with printf '%s\n' "$CAPTURE_OUTPUT" | wc -l, which returns 1 for empty output too, so it passed for a list that printed nothing. The reviewer caught it by stubbing change_list_repo to a no-op: the suite still bit, but one assertion later. Now counted with grep -c ., and a zero-row list fails the assertion that claims to check it.

Two prose corrections.

prunable does not mean the directory is gone — it means the link to it broke. A worktree whose directory survives but whose .git file was deleted is also reported prunable, and git worktree prune clears the admin entry without deleting anything, so the README's old "the directory is gone; only the admin entry is left" could have invited an rm -rf over uncommitted work. Reworded, with the caution stated.

This PR body claimed protected-repo behaviour was "bit-identical". Not quite: protected repos keep their base selection unchanged but do pick up the two new preflights, since the plan applies them to every repo. The code was right; the prose was loose. Corrected above.

Not changed, deliberately. The reviewer noted that after a failed handoff write, begin leaves behind a branch it can prove it created — worktree add -b succeeding is itself proof no other agent owns it — while reporting it with the hedged "may have been left behind" wording the lost-race path needs. That is exactly what the plan mandates ("failure handling for step 8 or 9 ... The branch is not deleted"), so it stays; tightening it to delete on the step-9 path only is a fair follow-up against an amended plan, not a change to make silently here.

Also reported correct, for coverage: contract steps 1–10 in the plan's order; the -d-absent path verified byte-identical against origin/master on both protected and unprotected fixtures with zero new filesystem entries; pairwise list/cleanup agreement for every other state including the unlock→clean→cleanup flip; the rollback's post-condition check exercised in both directions; end-to-end behaviour under a $HOME containing spaces; and the workspace_repo_default_branch deviation independently reproduced, confirming the plan's record is what is wrong, not the code.

Review pass by a different model, as the dev plan's R2 review depth requires. It ran read-only against the branch, built its own fixtures in a scratch copy, and sabotage-checked the suite independently. No severe defect; one real contract violation, one weak assertion, and some prose corrections. Fixed in 90d68df. **The contract violation, confirmed and reproduced independently.** `cleanup` has six refusal sources, not the five the dev plan enumerates: `git worktree remove` also refuses a worktree containing a populated submodule. Such a worktree passes every check `list` was making — empty `status --porcelain`, nothing unpushed, not locked, detached, or prunable — so `list` printed `clean` and advised `allod change cleanup <path>`, which then died 128 with `fatal: working trees containing submodules cannot be moved or removed` and left the worktree in place. That is a direct counterexample to the binding contract, stated absolutely in the code comment, the README, and this PR body. Fixed by reporting rather than by weakening the claim. `list` gains a `submodule` state, ranked with the other structural blockers above `dirty`. The detection is deliberately narrow: a gitlink whose directory holds no `.git` is unpopulated and removes fine, and unpopulated is exactly what `begin` itself leaves behind, so firing on any gitlink would falsely block every worktree of any repo that merely has submodules. Both the fire and the no-fire case are asserted, and sabotaging the detection in either direction — removing it, or making it fire on unpopulated gitlinks — is caught by the intended assertion. Latent rather than live: no repo under `~/work` has a `.gitmodules`, and the failure is loud and non-destructive when hit. Worth fixing anyway, because the contract is the part of this work that outlives the bash implementation. **A weak assertion.** "list prints one row per linked worktree" counted lines with `printf '%s\n' "$CAPTURE_OUTPUT" | wc -l`, which returns 1 for empty output too, so it passed for a `list` that printed nothing. The reviewer caught it by stubbing `change_list_repo` to a no-op: the suite still bit, but one assertion later. Now counted with `grep -c .`, and a zero-row `list` fails the assertion that claims to check it. **Two prose corrections.** `prunable` does not mean the directory is gone — it means the link to it broke. A worktree whose directory survives but whose `.git` file was deleted is also reported `prunable`, and `git worktree prune` clears the admin entry without deleting anything, so the README's old "the directory is gone; only the admin entry is left" could have invited an `rm -rf` over uncommitted work. Reworded, with the caution stated. This PR body claimed protected-repo behaviour was "bit-identical". Not quite: protected repos keep their base selection unchanged but do pick up the two new preflights, since the plan applies them to every repo. The code was right; the prose was loose. Corrected above. **Not changed, deliberately.** The reviewer noted that after a failed handoff write, `begin` leaves behind a branch it *can* prove it created — `worktree add -b` succeeding is itself proof no other agent owns it — while reporting it with the hedged "may have been left behind" wording the lost-race path needs. That is exactly what the plan mandates ("failure handling for step 8 or 9 ... The branch is *not* deleted"), so it stays; tightening it to delete on the step-9 path only is a fair follow-up against an amended plan, not a change to make silently here. Also reported correct, for coverage: contract steps 1–10 in the plan's order; the `-d`-absent path verified byte-identical against `origin/master` on both protected and unprotected fixtures with zero new filesystem entries; pairwise `list`/`cleanup` agreement for every other state including the unlock→clean→cleanup flip; the rollback's post-condition check exercised in both directions; end-to-end behaviour under a `$HOME` containing spaces; and the `workspace_repo_default_branch` deviation independently reproduced, confirming the plan's record is what is wrong, not the code.