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

New issue

Closed

opened 2026-07-28 03:28:53 +01:00 by allod-agent · 1 comment

allod-agent commented

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

Member

Make has_unpushed_commits distinguish "no unpushed commits" from "cannot tell", so allod change cleanup stops deleting a branch whose commits it was never able to check.

Primary goals:

A destructive command requires proof, not absence of evidence — cleanup deletes a worktree and its agent/* branch, so it should proceed only when it can show the commits exist on origin, never because a check it could not perform came back false.
Three answers, not two — the helper currently collapses "definitely nothing unpushed" and "no base to compare against" into the same return 1, which is the silent fallback architecture.md principle 11 forbids.
Keep record and list honest — the same collapse makes record say "no unpushed commits to push" when it cannot tell, and makes allod change list report clean for a worktree holding commits that exist nowhere else.

Current state

allod:203-228. Four paths answer "no unpushed commits" when the honest answer is "cannot determine":

branch=$(git -C "$dir" branch --show-current 2>/dev/null || true)
[[ -n "$branch" ]] || return 1                    # :208  detached HEAD
...
  else
    return 1                                     # :220  origin/<default> does not resolve
  fi
else
  return 1                                       # :223  no origin remote at all
fi

count=$(git -C "$dir" rev-list --count "${base}..HEAD" 2>/dev/null || printf '0')   # :226  rev-list failed
[[ "$count" =~ ^[0-9]+$ && "$count" -gt 0 ]]

Three consumers read it. change_cleanup:501-503 is the destructive one: past that check it runs worktree remove (:506) and branch -D (:508). change_record:354-357 reports die 4 "nothing to commit and no unpushed commits to push", which is misleading rather than harmful. allod change list, added in PR allod/tools#125, reports clean.

Reproduction and consequence

Verified on git 2.51.2. A repo with no origin remote — so every commit on the branch is by definition unique local work — reaches :223:

git init -b master repo && cd repo && git commit -qm initial --allow-empty
git worktree add ../wt -b agent/local-only
cd ../wt && git commit -qam "irreplaceable work"
allod change cleanup ../wt

cleanup exits 0, removes the worktree, and prints Deleted branch agent/local-only. The commit is then reachable from no ref: the branch reflog died with the branch, the worktree's HEAD reflog died with its private git dir, and the main checkout's reflog never saw the commit. Recovery is git fsck --lost-found until git gc prunes the object. allod change list reports the same worktree as clean beforehand, so both halves agree and both are wrong.

The :208 detached path is not the dangerous one and is already documented in allod/strategy dev-plans/allod-change-always-isolate.md: cleanup calls current_branch_or_die (allod:495) first and refuses detached worktrees before reaching this check, which makes that path a reporting hazard rather than data loss. :220 is narrow for the reason recorded in issue allod/tools#126 — git repairs refs/remotes/origin/HEAD on any fetch. :223 is the reachable one.

Scope

In: allod:203-228 and its three consumers at :354, :501, and change_list; tests/allod-change.sh.

The default: give the helper a third result — commits unpushed, none unpushed, or base undeterminable — and handle the third explicitly. cleanup refuses it, naming the missing origin or unresolvable base and pointing at git -C <repo> branch -D <branch> as the deliberate manual override, per principle 4 (rails are overridden with standard git, not with a bypass flag). record says it could not determine what is unpushed rather than asserting there is nothing. list gains a distinct state word for it.

list's binding contract from PR allod/tools#125 — it reports clean if and only if cleanup on that path would succeed — must keep holding, so whatever cleanup starts refusing, list stops calling clean in the same change. The per-state agreement tests in tests/allod-change.sh already assert that pairing and extend to the new state.

Considered and rejected: having cleanup require an upstream branch outright. It refuses the same cases but reports every one of them as "no upstream", losing the distinction between a repo with no remote, an unresolvable default branch, and a genuine failure to run rev-list, which is the information the operator needs to act.

Out: the origin/HEAD resolution defect behind :220, which is issue allod/tools#126 and is a different contract in a different file.

Found while tracing consumers for allod/tools#126.

Make `has_unpushed_commits` distinguish "no unpushed commits" from "cannot tell", so `allod change cleanup` stops deleting a branch whose commits it was never able to check. Primary goals: - **A destructive command requires proof, not absence of evidence** — `cleanup` deletes a worktree and its `agent/*` branch, so it should proceed only when it can show the commits exist on `origin`, never because a check it could not perform came back false. - **Three answers, not two** — the helper currently collapses "definitely nothing unpushed" and "no base to compare against" into the same `return 1`, which is the silent fallback `architecture.md` principle 11 forbids. - **Keep `record` and `list` honest** — the same collapse makes `record` say "no unpushed commits to push" when it cannot tell, and makes `allod change list` report `clean` for a worktree holding commits that exist nowhere else. ### Current state `allod:203-228`. Four paths answer "no unpushed commits" when the honest answer is "cannot determine": branch=$(git -C "$dir" branch --show-current 2>/dev/null || true) [[ -n "$branch" ]] || return 1 # :208 detached HEAD ... else return 1 # :220 origin/<default> does not resolve fi else return 1 # :223 no origin remote at all fi count=$(git -C "$dir" rev-list --count "${base}..HEAD" 2>/dev/null || printf '0') # :226 rev-list failed [[ "$count" =~ ^[0-9]+$ && "$count" -gt 0 ]] Three consumers read it. `change_cleanup:501-503` is the destructive one: past that check it runs `worktree remove` (`:506`) and `branch -D` (`:508`). `change_record:354-357` reports `die 4 "nothing to commit and no unpushed commits to push"`, which is misleading rather than harmful. `allod change list`, added in PR allod/tools#125, reports `clean`. ### Reproduction and consequence Verified on git 2.51.2. A repo with no `origin` remote — so every commit on the branch is by definition unique local work — reaches `:223`: git init -b master repo && cd repo && git commit -qm initial --allow-empty git worktree add ../wt -b agent/local-only cd ../wt && git commit -qam "irreplaceable work" allod change cleanup ../wt `cleanup` exits 0, removes the worktree, and prints `Deleted branch agent/local-only`. The commit is then reachable from no ref: the branch reflog died with the branch, the worktree's HEAD reflog died with its private git dir, and the main checkout's reflog never saw the commit. Recovery is `git fsck --lost-found` until `git gc` prunes the object. `allod change list` reports the same worktree as `clean` beforehand, so both halves agree and both are wrong. The `:208` detached path is not the dangerous one and is already documented in `allod/strategy` `dev-plans/allod-change-always-isolate.md`: `cleanup` calls `current_branch_or_die` (`allod:495`) first and refuses detached worktrees before reaching this check, which makes that path a reporting hazard rather than data loss. `:220` is narrow for the reason recorded in issue allod/tools#126 — git repairs `refs/remotes/origin/HEAD` on any fetch. `:223` is the reachable one. ### Scope In: `allod:203-228` and its three consumers at `:354`, `:501`, and `change_list`; `tests/allod-change.sh`. The default: give the helper a third result — commits unpushed, none unpushed, or base undeterminable — and handle the third explicitly. `cleanup` refuses it, naming the missing `origin` or unresolvable base and pointing at `git -C <repo> branch -D <branch>` as the deliberate manual override, per principle 4 (rails are overridden with standard git, not with a bypass flag). `record` says it could not determine what is unpushed rather than asserting there is nothing. `list` gains a distinct state word for it. `list`'s binding contract from PR allod/tools#125 — it reports `clean` if and only if `cleanup` on that path would succeed — must keep holding, so whatever `cleanup` starts refusing, `list` stops calling `clean` in the same change. The per-state agreement tests in `tests/allod-change.sh` already assert that pairing and extend to the new state. Considered and rejected: having `cleanup` require an upstream branch outright. It refuses the same cases but reports every one of them as "no upstream", losing the distinction between a repo with no remote, an unresolvable default branch, and a genuine failure to run `rev-list`, which is the information the operator needs to act. Out: the `origin/HEAD` resolution defect behind `:220`, which is issue allod/tools#126 and is a different contract in a different file. Found while tracing consumers for allod/tools#126.

allod-agent referenced this issue

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

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

allod-agent referenced this issue

2026-07-28 03:30:06 +01:00

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