coilyco-flight-deck/ward

Fork 0

Salvage should post back to the carried issue that spawned the run, and must not salvage runs whose work already landed #518

New issue

Closed

opened 2026-07-02 07:33:03 +00:00 by coilysiren · 2 comments

coilysiren commented

2026-07-02 07:33:03 +00:00

Owner

What Kai asked for

Two related complaints about the reaper's salvage path, from the ward#492 burndown session:

Salvages are collapsed into one mega-issue (ward#504 now holds 6 different runs' salvage notices as comments), instead of each landing somewhere addressable.
The better design: the salvage notice should post right back to the issue that spawned the engineer, not into a separate salvage issue at all. The reaper already knows the carried issue (env.Issue), so a run that fails to land should comment on its own issue - that is the natural tracking home, the thing the director and the next engineer already watch.

Root cause of the mega-issue (complaint 1)

fileSalvageIssue (cmd/ward/container_reap.go) deliberately dedups by title prefix:

if n, found, err := fc.findOpenIssueByTitlePrefix(ctx, env.Owner, env.Name, salvageIssueTitlePrefix); err == nil && found {
    // appends a comment to the existing open [ward-salvage] issue
    return fc.commentIssue(ctx, env.Owner, env.Name, n, body)
}
// else creates one

So every salvage after the first appends to whichever [ward-salvage] issue is still open. It never looks at env.Issue, even though the reaper carries it and uses it elsewhere (issueClosingReferencePresent, runProvenanceLanded).

Wanted behaviour

Ordered, because deliverable 1 is a hard prerequisite for 2:

Do not salvage a run whose work already landed and closed its issue. See the sibling false-salvage bug below - today a fully-successful run can still be salvaged, and if that salvage then comments on the carried issue (deliverable 2) it would reopen/scare a correctly-closed issue. This must be fixed first.
When a genuine salvage is needed and the run carried an issue (env.Issue != 0), post the salvage notice as a comment on that issue, not a [ward-salvage] issue. Reopen the carried issue if the aborted push had tripped its closes #N. The comment still names the salvage branch, the reason, and the recovery steps.
Only when there is no carried issue (freeform run, env.Issue == 0) fall back to a standalone salvage issue - and even then, one issue per salvage instance, never append-to-open. Drop the findOpenIssueByTitlePrefix dedup.

The false-salvage bug (deliverable 1, with evidence from tonight)

Regression correlated with ward#513 (commit ee76581, landed 06:07Z today) adding runProvenanceLanded. In reapTargetTree, the salvage gates fire before the nothing-to-reap / already-landed check:

captureAndCommitResidual
fetch origin
readRunProvenance            -> err? salvage(reasonConflict)
issueClosingReferencePresent -> no?  salvage(reasonCloseRef)
runProvenanceLanded          -> no?  salvage(reasonConflict)   <-- fires here
if residual==0 && cleanTree  -> nothing to reap, return nil     <-- never reached

So a run whose HEAD is already the landed main commit (residual 0, clean tree) gets salvaged if any of those gates returns a false negative, instead of correctly reading as done.

Confirmed tonight: four salvage branches whose tips are current main commits with zero unique work, each for a run that closed its issue cleanly:

ward-salvage/ward-caf324d4 -> tip 96c0e1b (closed #476)
ward-salvage/ward-e0723a81 -> tip 06db4aa (closed #465)
ward-salvage/ward-5305de94 -> tip ecf6d74 (closed #478)
ward-salvage/ward-59758860 -> tip 181b570 (closed #441)

All four are ancestors of main (git merge-base --is-ancestor <tip> origin/main = yes, 0 unique commits). The work landed, the issues are closed, yet each spawned a bogus salvage branch and a "merge conflict integrating onto main" comment on ward#504. (The two earlier salvages, ward-4b5af5f3 for #450 and ward-b4d43076 for #489, are genuine - 1 unique commit each - and predate ee76581.)

Harm today is noise, not lost work: the real commits are on main and the issues are closed. But once deliverable 2 lands (salvage comments post to the carried issue), a false salvage would reopen and alarm a correctly-closed issue, so this is load-bearing.

Acceptance

A run whose carried work landed on main and closed its issue produces no salvage branch and no salvage notice.
A genuine salvage on a carried run posts its notice as a comment on the spawning issue (reopening it if the failed push had closed it), with no standalone [ward-salvage] issue.
A genuine salvage on a run with no carried issue files exactly one standalone issue, never appended to a prior one.
Regression tests: (a) landed-and-closed run -> no salvage, (b) real conflict on a carried run -> comment on that issue + reopen, (c) no-issue run -> one fresh issue.
docs/container-reap.md updated for the new notify-the-carried-issue behaviour.

ward#513 (ee76581) the precursor that introduced the ordering the false-salvage rides on.
ward#515 the mirror-image bug on the same reap decision path (residual landed without closing the issue). One coordinated fix to reapTargetTree should serve both - do them on the same branch or in sequence to avoid a same-file collision.
ward#514 the broader landing-contract redesign this fits under.
ward#504 the mega salvage issue this replaces, and the branches above that can be pruned once this lands.

Filed by Claude (she/her) during the ward#492 burndown, capturing Kai's two salvage-flow directives.

## What Kai asked for Two related complaints about the reaper's salvage path, from the ward#492 burndown session: 1. Salvages are collapsed into one mega-issue (ward#504 now holds 6 different runs' salvage notices as comments), instead of each landing somewhere addressable. 2. The better design: **the salvage notice should post right back to the issue that spawned the engineer**, not into a separate salvage issue at all. The reaper already knows the carried issue (`env.Issue`), so a run that fails to land should comment on its own issue - that is the natural tracking home, the thing the director and the next engineer already watch. ## Root cause of the mega-issue (complaint 1) `fileSalvageIssue` (cmd/ward/container_reap.go) deliberately dedups by title prefix: ```go if n, found, err := fc.findOpenIssueByTitlePrefix(ctx, env.Owner, env.Name, salvageIssueTitlePrefix); err == nil && found { // appends a comment to the existing open [ward-salvage] issue return fc.commentIssue(ctx, env.Owner, env.Name, n, body) } // else creates one ``` So every salvage after the first appends to whichever `[ward-salvage]` issue is still open. It never looks at `env.Issue`, even though the reaper carries it and uses it elsewhere (`issueClosingReferencePresent`, `runProvenanceLanded`). ## Wanted behaviour Ordered, because deliverable 1 is a hard prerequisite for 2: 1. **Do not salvage a run whose work already landed and closed its issue.** See the sibling false-salvage bug below - today a fully-successful run can still be salvaged, and if that salvage then comments on the carried issue (deliverable 2) it would reopen/scare a correctly-closed issue. This must be fixed first. 2. **When a genuine salvage is needed and the run carried an issue (`env.Issue != 0`), post the salvage notice as a comment on that issue**, not a `[ward-salvage]` issue. Reopen the carried issue if the aborted push had tripped its `closes #N`. The comment still names the salvage branch, the reason, and the recovery steps. 3. **Only when there is no carried issue** (freeform run, `env.Issue == 0`) fall back to a standalone salvage issue - and even then, **one issue per salvage instance**, never append-to-open. Drop the `findOpenIssueByTitlePrefix` dedup. ## The false-salvage bug (deliverable 1, with evidence from tonight) Regression correlated with ward#513 (commit `ee76581`, landed 06:07Z today) adding `runProvenanceLanded`. In `reapTargetTree`, the salvage gates fire **before** the nothing-to-reap / already-landed check: ``` captureAndCommitResidual fetch origin readRunProvenance -> err? salvage(reasonConflict) issueClosingReferencePresent -> no? salvage(reasonCloseRef) runProvenanceLanded -> no? salvage(reasonConflict) <-- fires here if residual==0 && cleanTree -> nothing to reap, return nil <-- never reached ``` So a run whose HEAD is already the landed `main` commit (residual 0, clean tree) gets salvaged if any of those gates returns a false negative, instead of correctly reading as done. **Confirmed tonight:** four salvage branches whose tips are *current main commits* with zero unique work, each for a run that closed its issue cleanly: - `ward-salvage/ward-caf324d4` -> tip `96c0e1b` (closed #476) - `ward-salvage/ward-e0723a81` -> tip `06db4aa` (closed #465) - `ward-salvage/ward-5305de94` -> tip `ecf6d74` (closed #478) - `ward-salvage/ward-59758860` -> tip `181b570` (closed #441) All four are ancestors of `main` (`git merge-base --is-ancestor <tip> origin/main` = yes, 0 unique commits). The work landed, the issues are closed, yet each spawned a bogus salvage branch and a "merge conflict integrating onto main" comment on ward#504. (The two *earlier* salvages, `ward-4b5af5f3` for #450 and `ward-b4d43076` for #489, are genuine - 1 unique commit each - and predate `ee76581`.) Harm today is noise, not lost work: the real commits are on `main` and the issues are closed. But once deliverable 2 lands (salvage comments post to the carried issue), a false salvage would reopen and alarm a correctly-closed issue, so this is load-bearing. ## Acceptance - A run whose carried work landed on `main` and closed its issue produces **no** salvage branch and **no** salvage notice. - A genuine salvage on a carried run posts its notice as a comment on the spawning issue (reopening it if the failed push had closed it), with no standalone `[ward-salvage]` issue. - A genuine salvage on a run with no carried issue files exactly one standalone issue, never appended to a prior one. - Regression tests: (a) landed-and-closed run -> no salvage, (b) real conflict on a carried run -> comment on that issue + reopen, (c) no-issue run -> one fresh issue. - docs/container-reap.md updated for the new notify-the-carried-issue behaviour. ## Related - ward#513 (`ee76581`) the precursor that introduced the ordering the false-salvage rides on. - ward#515 the mirror-image bug on the same reap decision path (residual landed *without* closing the issue). One coordinated fix to `reapTargetTree` should serve both - do them on the same branch or in sequence to avoid a same-file collision. - ward#514 the broader landing-contract redesign this fits under. - ward#504 the mega salvage issue this replaces, and the branches above that can be pruned once this lands. Filed by Claude (she/her) during the ward#492 burndown, capturing Kai's two salvage-flow directives.

coilysiren added the

headless

labels

2026-07-02 07:33:03 +00:00

coilyco-ops commented

2026-07-02 07:34:19 +00:00

Member

🔒 Reserved by ward agent --driver claude — container engineer-claude-ward-518 on host kais-macbook-pro-2.local is carrying this issue (reserved 2026-07-02T07:34:17Z). Concurrent ward agent runs are blocked until it finishes or the reservation goes stale (2h0m0s TTL); --force overrides.

— Claude (she/her), via ward agent

🔒 Reserved by `ward agent --driver claude` — container `engineer-claude-ward-518` on host `kais-macbook-pro-2.local` is carrying this issue (reserved 2026-07-02T07:34:17Z). Concurrent `ward agent` runs are blocked until it finishes or the reservation goes stale (2h0m0s TTL); `--force` overrides.  — Claude (she/her), via `ward agent`

coilysiren referenced this issue from a commit

2026-07-02 07:47:26 +00:00

fix(reap): notify the carried issue on salvage, never false-salvage a landed run

coilysiren closed this issue

2026-07-02 07:47:26 +00:00

coilysiren commented

2026-07-02 07:48:01 +00:00

Author

Owner

WARD-OUTCOME: done - reaper no longer false-salvages landed runs and posts genuine salvage notices back on the carried issue

Landed on main as 9013c9d. This one was satisfying because the diagnosis in the issue was already precise: the fix for deliverable 1 was literally moving the nothing-to-reap check above the three salvage gates, since all of them read the origin/main..HEAD range that goes empty exactly when a run has already landed. Nice when the root cause reduces to a reorder.

A pleasant surprise: putting the check first also fixes pre-launch deaths (no provenance file). They used to trip the provenance-error salvage gate and spawn a bogus branch; now they read as nothing-to-reap and correctly release the reservation.

Deliverables 2 and 3 pushed me to extract a salvageNotifier interface so notifySalvage is unit-testable with a fake - the forgejo client shells back to the ward binary, so there was no clean seam to assert reopen-vs-comment-vs-create otherwise. That felt like the right call and the three regression tests came out clean.

What fought back was purely the doc size caps: both container-reap.md and troubleshooting.md were already within a hair of the 4000-char limit, so documenting new behaviour meant trimming prose elsewhere to stay under. Cosmetic, but it ate a few iterations.

Confidence is high on the code path. The one thing I could not exercise end-to-end is the live forgejo reopen+comment against a real closed issue (tested via the fake only). Worth noting: this pairs with ward#515 (residual landed WITHOUT closing the issue) on the same reapTargetTree decision path - my reorder makes residual==0 always read as nothing-to-reap, which is the right base for #515 to build its "but should this issue be closed?" check on top of. Also ward#504's four bogus salvage branches can now be pruned.

WARD-OUTCOME: done - reaper no longer false-salvages landed runs and posts genuine salvage notices back on the carried issue Landed on main as 9013c9d. This one was satisfying because the diagnosis in the issue was already precise: the fix for deliverable 1 was literally moving the nothing-to-reap check above the three salvage gates, since all of them read the origin/main..HEAD range that goes empty exactly when a run has already landed. Nice when the root cause reduces to a reorder. A pleasant surprise: putting the check first also fixes pre-launch deaths (no provenance file). They used to trip the provenance-error salvage gate and spawn a bogus branch; now they read as nothing-to-reap and correctly release the reservation. Deliverables 2 and 3 pushed me to extract a salvageNotifier interface so notifySalvage is unit-testable with a fake - the forgejo client shells back to the ward binary, so there was no clean seam to assert reopen-vs-comment-vs-create otherwise. That felt like the right call and the three regression tests came out clean. What fought back was purely the doc size caps: both container-reap.md and troubleshooting.md were already within a hair of the 4000-char limit, so documenting new behaviour meant trimming prose elsewhere to stay under. Cosmetic, but it ate a few iterations. Confidence is high on the code path. The one thing I could not exercise end-to-end is the live forgejo reopen+comment against a real closed issue (tested via the fake only). Worth noting: this pairs with ward#515 (residual landed WITHOUT closing the issue) on the same reapTargetTree decision path - my reorder makes residual==0 always read as nothing-to-reap, which is the right base for #515 to build its "but should this issue be closed?" check on top of. Also ward#504's four bogus salvage branches can now be pruned.

coilysiren referenced this issue from a commit

2026-07-02 07:48:52 +00:00

feat(agent): recover the gh-backed GitHub issue-forge client (ward#489 completion)

coilyco-ops referenced this issue

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

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