reaper: granted-repo landing check false-positives, spurious empty salvage + reopen when a granted main advanced after clone #587

New issue

Closed

opened 2026-07-03 20:34:55 +00:00 by coilysiren · 2 comments

coilysiren commented

2026-07-03 20:34:55 +00:00

Owner

Problem

The container reaper's granted-repo (--repo) landing verification produces false-positive salvage branches and issue reopens even when the granted repo's content is fully present on origin/main. Two live cases on 2026-07-03:

coilyco-flight-deck/ward#585 (grants: cli-guard, infrastructure) - reopened once. Both named salvage branches (ward-salvage/cli-guard-672418b2, ward-salvage/infrastructure-f7037e8a) have zero commits unique to origin/main. Their tips are already-landed, unrelated HEADs (fix(sandbox)... ward#546, Merge issue-460... CI_RELEASE_TOKEN), nothing to do with the eco work.
coilyco-gaming/eco-ops#29 (grants: eco-app, eco-mods) - reopened twice, once after each of two runs. All four salvage tips (eco-app-0b1b2143, eco-mods-4fc3f8a1, eco-app-7c41d1c3, eco-mods-c9e1efd8) are ancestors of current main with zero unique commits. The catalog: blocks the task added are live on all three mains.

eco-ops#29 is the clean reproduction. The task was pasting one identical catalog: block into three repos' .ward/ward.yaml. The second run changed nothing, verified the blocks already on every main, and closed via API with no commit. The reaper still generated fresh salvage hashes and reopened. So the reaper fabricates a salvage even when the run authored zero commits.

Root cause (hypothesis)

Per AGENTS.md the reaper "fetches each granted repo and checks your push actually landed (local HEAD on the freshly-fetched origin/main)". That check is HEAD-identity / ancestry based, not content based. It fails whenever any of these hold, all of them normal:

The granted repo's main advanced between clone and reap (busy main), so the clone's local main is behind.
The added change landed under a different commit hash (carried forward as an ancestor by a later unrelated push - eco-ops#29's "Expand climate pollution explanation" / "Tune canoe speed").
The reaper's own "commit anything left loose + integrate onto main" step produces a fresh local commit on the stale local main, whose HEAD is by construction not contained in the advanced remote main.

Because the reaper never actually pushes a granted repo (by design, ward#291), the check can never pass for a granted repo whose remote main moved on, regardless of whether the content landed. Result: a guaranteed false "1 local commit never reached origin/main" -> empty salvage branch -> reopen that undoes the primary closes #N.

Impact

Trivial cross-repo tasks (paste one block into N repos) reopen indefinitely. eco-ops#29 has already looped twice and would loop again.
The reopen text tells a human to git fetch + re-push a salvage branch that is empty, wasting the follow-up.
Real losses become indistinguishable from these false positives, so the whole backstop stops being trustworthy.

Suggested fix

Make the granted-repo verification content / patch based, not HEAD-ancestry based:

For each commit the run authored in the granted clone, confirm its change is present in origin/main by patch-id / git cherry / an empty git diff origin/main -- <touched paths>, before declaring loss.
Or attempt a real fast-forward-only push and salvage only on an actual rejection, instead of inferring loss from HEAD divergence.
And skip salvage entirely when the run authored zero commits in the granted repo (the eco-ops#29 second-run case).

Evidence

Verified from the director surface: all six salvage branches across both issues have rev-list --count origin/main..<tip> == 0 and merge-base --is-ancestor <tip> origin/main true.

ward#291 - reaper verifies granted-repo landing (the check at fault)
coilyco-flight-deck/ward#585 - false reopen, empty salvage
coilyco-gaming/eco-ops#29 - false reopen twice, empty salvage, clean repro

## Problem The container reaper's granted-repo (`--repo`) landing verification produces **false-positive salvage branches and issue reopens** even when the granted repo's content is fully present on `origin/main`. Two live cases on 2026-07-03: - **coilyco-flight-deck/ward#585** (grants: cli-guard, infrastructure) - reopened once. Both named salvage branches (`ward-salvage/cli-guard-672418b2`, `ward-salvage/infrastructure-f7037e8a`) have **zero commits unique to `origin/main`**. Their tips are already-landed, unrelated HEADs (`fix(sandbox)... ward#546`, `Merge issue-460... CI_RELEASE_TOKEN`), nothing to do with the eco work. - **coilyco-gaming/eco-ops#29** (grants: eco-app, eco-mods) - reopened **twice**, once after each of two runs. All four salvage tips (`eco-app-0b1b2143`, `eco-mods-4fc3f8a1`, `eco-app-7c41d1c3`, `eco-mods-c9e1efd8`) are ancestors of current `main` with zero unique commits. The `catalog:` blocks the task added are live on all three mains. eco-ops#29 is the clean reproduction. The task was pasting one identical `catalog:` block into three repos' `.ward/ward.yaml`. The **second** run changed nothing, verified the blocks already on every `main`, and closed via API with **no commit**. The reaper still generated fresh salvage hashes and reopened. So the reaper fabricates a salvage even when the run authored zero commits. ## Root cause (hypothesis) Per AGENTS.md the reaper "fetches each granted repo and checks your push actually landed (local `HEAD` on the freshly-fetched `origin/main`)". That check is **HEAD-identity / ancestry based, not content based**. It fails whenever any of these hold, all of them normal: 1. The granted repo's `main` advanced between clone and reap (busy main), so the clone's local main is behind. 2. The added change landed under a **different commit hash** (carried forward as an ancestor by a later unrelated push - eco-ops#29's "Expand climate pollution explanation" / "Tune canoe speed"). 3. The reaper's own "commit anything left loose + integrate onto main" step produces a fresh local commit on the stale local main, whose HEAD is by construction not contained in the advanced remote main. Because the reaper never actually pushes a granted repo (by design, ward#291), the check can **never pass** for a granted repo whose remote `main` moved on, regardless of whether the content landed. Result: a guaranteed false "1 local commit never reached origin/main" -> empty salvage branch -> reopen that undoes the primary `closes #N`. ## Impact - Trivial cross-repo tasks (paste one block into N repos) reopen indefinitely. eco-ops#29 has already looped twice and would loop again. - The reopen text tells a human to `git fetch` + re-push a salvage branch that is empty, wasting the follow-up. - Real losses become indistinguishable from these false positives, so the whole backstop stops being trustworthy. ## Suggested fix Make the granted-repo verification **content / patch based, not HEAD-ancestry based**: - For each commit the run authored in the granted clone, confirm its change is present in `origin/main` by patch-id / `git cherry` / an empty `git diff origin/main -- <touched paths>`, before declaring loss. - Or attempt a real fast-forward-only push and salvage only on an actual rejection, instead of inferring loss from HEAD divergence. - And skip salvage entirely when the run authored **zero** commits in the granted repo (the eco-ops#29 second-run case). ## Evidence Verified from the director surface: all six salvage branches across both issues have `rev-list --count origin/main..<tip> == 0` and `merge-base --is-ancestor <tip> origin/main` true. ## Related - ward#291 - reaper verifies granted-repo landing (the check at fault) - coilyco-flight-deck/ward#585 - false reopen, empty salvage - coilyco-gaming/eco-ops#29 - false reopen twice, empty salvage, clean repro

coilyco-ops commented

2026-07-03 20:53:50 +00:00

Member

🔒 Reserved by ward agent --driver claude — container engineer-claude-ward-587 on host KAI-DESKTOP-TOWER is carrying this issue (reserved 2026-07-03T20:53:45Z). Concurrent ward agent runs are blocked until it finishes or the reservation goes stale (2h0m0s TTL); --force overrides.

Do not comment on or edit this issue to steer the run while it is reserved. The engineer seeded the body once at launch and never re-reads it, so a comment or edit reaches only human readers, never the running engineer. A correction goes to a new issue, dispatched fresh — that is the only channel that reaches a run in flight. Where the forge supports it, ward locks this conversation to make that a road-block rather than a convention (ward#494).

— Claude (she/her), via ward agent

🔒 Reserved by `ward agent --driver claude` — container `engineer-claude-ward-587` on host `KAI-DESKTOP-TOWER` is carrying this issue (reserved 2026-07-03T20:53:45Z). Concurrent `ward agent` runs are blocked until it finishes or the reservation goes stale (2h0m0s TTL); `--force` overrides. **Do not comment on or edit this issue to steer the run while it is reserved.** The engineer seeded the body once at launch and never re-reads it, so a comment or edit reaches only human readers, never the running engineer. A correction goes to a **new issue, dispatched fresh** — that is the only channel that reaches a run in flight. Where the forge supports it, ward locks this conversation to make that a road-block rather than a convention (ward#494).  — Claude (she/her), via `ward agent`

coilysiren referenced this issue from a commit

2026-07-03 21:02:16 +00:00

fix(reap): verify granted-repo landing by patch-id, not HEAD-ancestry (closes #587)

coilysiren closed this issue

2026-07-03 21:02:16 +00:00

coilysiren commented

2026-07-03 21:02:43 +00:00

Author

Owner

WARD-OUTCOME: done - grantLanded now accepts a grant as landed by patch-id (git cherry), not just HEAD-ancestry, killing the empty-salvage + reopen loop.

Landed on main as b9b1885. The fix itself was small and the suggested-fix section basically wrote the design for me: git cherry origin/main HEAD marks a local commit - when its patch is already upstream, so "zero + lines" is exactly the content-landed signal we wanted, and it slots in as a fallback after the existing reachability fast path.

What fought back a bit: I couldn't fully reproduce the zero-commit second-run case in my head from the code alone - a clean-tree ancestor HEAD should already pass the ancestry check, so the exact prod git state stays a little mysterious. But the patch-id approach is a strict superset of ancestry and directly covers all three named root causes (busy-main advance, different-hash landing, reaper's own residual commit whose content already landed), so I'm confident the class of false positive is closed regardless of which sub-case bit eco-ops#29. The telling smell was the old if ahead == 0 { ahead = 1 // shouldn't happen } defensive hack - that "impossible" state was the bug fabricating a phantom commit, and it's gone now.

Confidence: high on the code and the three new regression tests (different-hash grantLanded, end-to-end no-salvage, and the patch-count accounting). Rough edge worth noting: a squash-landing still can't be detected by patch-id (different patch), so a granted repo whose change lands squashed would still false-salvage - narrower than before but not zero. Could file a follow-up if that ever shows up in practice.

WARD-OUTCOME: done - grantLanded now accepts a grant as landed by patch-id (git cherry), not just HEAD-ancestry, killing the empty-salvage + reopen loop. Landed on main as b9b1885. The fix itself was small and the suggested-fix section basically wrote the design for me: `git cherry origin/main HEAD` marks a local commit `-` when its patch is already upstream, so "zero `+` lines" is exactly the content-landed signal we wanted, and it slots in as a fallback after the existing reachability fast path. What fought back a bit: I couldn't fully reproduce the *zero-commit second-run* case in my head from the code alone - a clean-tree ancestor HEAD should already pass the ancestry check, so the exact prod git state stays a little mysterious. But the patch-id approach is a strict superset of ancestry and directly covers all three named root causes (busy-main advance, different-hash landing, reaper's own residual commit whose content already landed), so I'm confident the class of false positive is closed regardless of which sub-case bit eco-ops#29. The telling smell was the old `if ahead == 0 { ahead = 1 // shouldn't happen }` defensive hack - that "impossible" state was the bug fabricating a phantom commit, and it's gone now. Confidence: high on the code and the three new regression tests (different-hash grantLanded, end-to-end no-salvage, and the patch-count accounting). Rough edge worth noting: a *squash*-landing still can't be detected by patch-id (different patch), so a granted repo whose change lands squashed would still false-salvage - narrower than before but not zero. Could file a follow-up if that ever shows up in practice.

coilyco-ops referenced this issue

2026-07-08 22:19:00 +00:00

Make agent workflow defaults configurable and default engineers to PRs #707