From 3591adffe7d97afe0b26746146ffaeba008f3565 Mon Sep 17 00:00:00 2001 From: "linh.doan" Date: Sat, 12 Sep 2026 14:06:09 +0700 Subject: [PATCH] devagent(TASK-mty0vgkg-oerc): auto-cleanup snapshot --- docs/PRD.html | 53 +++++++- docs/PRD.md | 22 ++++ docs/SELF-BUILD-LOOP.md | 10 ++ internal/loopdriver/DECISION.md | 81 ++++++++++++ internal/loopdriver/issues.go | 43 ++++--- internal/loopdriver/run.go | 114 ++++++++++++++-- internal/loopdriver/run_test.go | 221 +++++++++++++++++++++++++++++++- 7 files changed, 505 insertions(+), 39 deletions(-) diff --git a/docs/PRD.html b/docs/PRD.html index 3dba22ed..a790a674 100644 --- a/docs/PRD.html +++ b/docs/PRD.html @@ -2330,6 +2330,30 @@

Phase 4 — Expansion (post-v1)

quotes a pull request which happens to merge inside the window certifies on that merge; the window and the artifact gate are the bounds (internal/loopdriver/run.go, issues.go).

+

2026-09-12 (the rescue reaches CLOSED-but-unmerged pull requests): +the driver-side verify-and-merge rescue — and the goal-text derivation +feeding it — only ever accepted a subject that read OPEN +before the dispatch, so the false closes were unreachable: PR +#346/#347 (auto-closed by the sweep's BaseBranchGone(main) +transport-noise bug minutes after they opened, green and mergeable) and +loop 282's #345 (CLOSED at dispatch, the worker's job being to reopen +it) all recorded no-pr with a landable branch left closed. +prView now also decodes +baseRefName/headRefName; a goal-named pull +request that is CLOSED, unmerged, based on main, with its +head ref still on origin becomes the merge subject, and +verifyAndMergeRescue reopens it (gh pr reopen, +repo-scoped, 60s-bounded, drain-tolerant), re-reads the state — the +reopen is verified, never assumed — and only then merges through the +ordinary mergePRBounded path. Every other close lands +nothing: a superseded/other base, a head ref gone from origin, a merge +stamp, and a reopen gh refuses all return 0 rather than fabricating a +ship. Pinned by +TestRunLoopClosedUnmergedGoalPRReopenedAndMerged, +TestRunLoopClosedGoalPRLandsNothingWhenNotReopenable +(base-superseded, head-gone, reopen-refused) and +TestVerifyAndMergeRescueRefusesNonReopenablePR +(internal/loopdriver/run.go, issues.go).

- Cross-board retry memory beyond the SHA guard — commit 60638d3 stops re-issuing shipped goals, but re-queued failures @@ -4901,9 +4925,32 @@

23.1 Requirements

chaos schedule) so "the driver works perfectly" is a checkable claim, not a hope.

-

*Last updated: 2026-09-12 (both green heads landed straight onto main -— no pull request, no re-implementation) — the zombie sweep was still -armed when PR #347 and PR #346 were opened, and its +

*Last updated: 2026-09-12 (the rescue reaches CLOSED-but-unmerged +pull requests: a goal-named closed PR is reopened and merged, or lands +nothing) — the driver-side verify-and-merge rescue, and the goal-text +derivation feeding it, only accepted subjects that read +OPEN before the dispatch, so the false closes were +unreachable: PR #346/#347 (auto-closed by the zombie sweep's +BaseBranchGone(main) transport-noise bug minutes after they +opened, green and mergeable) and loop 282's #345 each recorded +no-pr with a landable branch left closed. +prView now decodes +baseRefName/headRefName, and a goal-named pull +request that is CLOSED, unmerged, based on main with its +head ref still on origin becomes the merge subject: +verifyAndMergeRescue reopens it (gh pr reopen, +repo-scoped, 60s wall, drain-tolerant), re-reads the state (gh's reopen +is verified, never believed) and only then merges through the existing +mergePRBounded pipeline. Everything else lands nothing — +another/superseded base, a head ref gone from origin, a merge stamp, a +reopen gh refuses — so no ship is ever fabricated. Pinned by +TestRunLoopClosedUnmergedGoalPRReopenedAndMerged, +TestRunLoopClosedGoalPRLandsNothingWhenNotReopenable and +TestVerifyAndMergeRescueRefusesNonReopenablePR +(internal/loopdriver/run.go, issues.go). *Last +updated: 2026-09-12 (both green heads landed straight onto main — no +pull request, no re-implementation) — the zombie sweep was still armed +when PR #347 and PR #346 were opened, and its BaseBranchGone(main) transport-noise bug auto-closed both within minutes, so the two verified heads were landed straight onto main instead: 1f2ef8c (devagent/TASK-mtxyflr1-fq6y diff --git a/docs/PRD.md b/docs/PRD.md index 5b9ba418..a1157c18 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -1048,6 +1048,27 @@ Webhook-triggered runs with HMAC verification and dedup, run dashboard/status co > request which happens to merge inside the window certifies on that merge; > the window and the artifact gate are the bounds > (`internal/loopdriver/run.go`, `issues.go`). +> +> 2026-09-12 (the rescue reaches CLOSED-but-unmerged pull requests): the +> driver-side verify-and-merge rescue — and the goal-text derivation feeding +> it — only ever accepted a subject that read `OPEN` *before* the dispatch, so +> the false closes were unreachable: PR #346/#347 (auto-closed by the sweep's +> `BaseBranchGone(main)` transport-noise bug minutes after they opened, green +> and mergeable) and loop 282's #345 (CLOSED at dispatch, the worker's job +> being to reopen it) all recorded `no-pr` with a landable branch left closed. +> `prView` now also decodes `baseRefName`/`headRefName`; a goal-named pull +> request that is CLOSED, unmerged, based on `main`, with its head ref still +> on origin becomes the merge subject, and `verifyAndMergeRescue` reopens it +> (`gh pr reopen`, repo-scoped, 60s-bounded, drain-tolerant), re-reads the +> state — the reopen is verified, never assumed — and only then merges through +> the ordinary `mergePRBounded` path. Every other close lands nothing: a +> superseded/other base, a head ref gone from origin, a merge stamp, and a +> reopen gh refuses all return 0 rather than fabricating a ship. Pinned by +> `TestRunLoopClosedUnmergedGoalPRReopenedAndMerged`, +> `TestRunLoopClosedGoalPRLandsNothingWhenNotReopenable` (base-superseded, +> head-gone, reopen-refused) and +> `TestVerifyAndMergeRescueRefusesNonReopenablePR` +> (`internal/loopdriver/run.go`, `issues.go`). ~~- **Cross-board retry memory beyond the SHA guard** — commit 60638d3 stops re-issuing shipped goals, but re-queued failures still get a fresh attempt budget; carry the prior board's failure class onto the re-bridged goal so the scout deprioritizes until the root-cause fix lands (Q27).~~ ~~- **Regression oracle before board merge** — gates judge single PRs and PR #108's committed STRIDE allowlist widens suppression paths; add a board-level "is the system at least as good?" check (full suite on the merged result) ahead of `autoMerge`, per the Kitchen Loop zero-regression rule.~~ @@ -1674,6 +1695,7 @@ existing driver with validation surfaces (test, command, telemetry, chaos schedule) so "the driver works perfectly" is a checkable claim, not a hope. --- +*Last updated: 2026-09-12 (the rescue reaches CLOSED-but-unmerged pull requests: a goal-named closed PR is reopened and merged, or lands nothing) — the driver-side verify-and-merge rescue, and the goal-text derivation feeding it, only accepted subjects that read `OPEN` before the dispatch, so the false closes were unreachable: PR #346/#347 (auto-closed by the zombie sweep's `BaseBranchGone(main)` transport-noise bug minutes after they opened, green and mergeable) and loop 282's #345 each recorded `no-pr` with a landable branch left closed. `prView` now decodes `baseRefName`/`headRefName`, and a goal-named pull request that is CLOSED, unmerged, based on `main` with its head ref still on origin becomes the merge subject: `verifyAndMergeRescue` reopens it (`gh pr reopen`, repo-scoped, 60s wall, drain-tolerant), re-reads the state (gh's reopen is verified, never believed) and only then merges through the existing `mergePRBounded` pipeline. Everything else lands nothing — another/superseded base, a head ref gone from origin, a merge stamp, a reopen gh refuses — so no ship is ever fabricated. Pinned by `TestRunLoopClosedUnmergedGoalPRReopenedAndMerged`, `TestRunLoopClosedGoalPRLandsNothingWhenNotReopenable` and `TestVerifyAndMergeRescueRefusesNonReopenablePR` (`internal/loopdriver/run.go`, `issues.go`). *Last updated: 2026-09-12 (both green heads landed straight onto main — no pull request, no re-implementation) — the zombie sweep was still armed when PR #347 and PR #346 were opened, and its `BaseBranchGone(main)` transport-noise bug auto-closed both within minutes, so the two verified heads were landed straight onto main instead: `1f2ef8c` (`devagent/TASK-mtxyflr1-fq6y` — three-valued hygiene probes plus the auto-close age floor) fast-forwarded main, then `635214d` (`devagent/TASK-mtxwr39q-nva6` — the landing-certification merge window) merged on top. The code paths are disjoint (`internal/orchestrator` vs `internal/loopdriver`) and only `docs/PRD.*` overlapped. Neither PR was reopened or closed by this landing; both stay CLOSED, unmerged as pull requests. Verified on the merged tree: `go test ./...` green, including `TestAutoMergeTreatsBaseProbeNoiseAsAlive` and `TestGhNotFoundOnlyForRealGone`, and `git merge-base --is-ancestor` true for both heads. *Last updated: 2026-09-12 (zombie-PR hygiene probes are three-valued, with an auto-close age floor) — both gh probes behind the `pr-hygiene` sweep collapsed "gh never answered" into a decisive answer: `BaseBranchGone` returned `err != nil`, so a network hiccup, 5xx, or rate-limit response on the `branches/` lookup read as a dead base and the sweep CLOSED a live PR as `base-superseded` (PR #346: created 05:27:21Z, closed 05:28:37Z, 76s later, never merged); `landingCommitOnMain` returned empty on a failed or unparseable commit listing, so transport noise read as "no landing commit" and the PR was closed as `superseded` with an explicit "not shipped" comment. `ProbeBaseBranch` (internal/orchestrator/autopr.go) now answers `BaseAlive` / `BaseGone` / `BaseUnknown`, judging "gone" only on gh's own not-found stderr (`ghNotFound` reads the diagnostic, never the argv-embedding message, whose ref names may themselves contain "404"; `GhError.Code` is the gh exit code and is never consulted), and `landingCommitOnMain` returns `(sha, ok)` with `ok` false whenever the listing failed. On `unknown` the sweep records `untouched`/`base-unknown` or `untouched`/`landing-unknown` and acts on nothing; the merge-queue gate treats `BaseUnknown` as alive, so a probe hiccup no longer parks a mergeable PR. Both auto-close arms now also wait out an age floor: `closeAgeFloorHours` (24h) capped by `prHygiene.graceHours`, with an unknown PR age treated as floored — a fresh PR is reported `untouched`/`base-superseded-age-floor` or `untouched`/`landing-age-floor` and the `--grace-hours` help states the floor. Regression-pinned by `TestSweepTaskPrHygiene` (base-probe noise, fresh and unknown-age floors), `TestSweepTaskPrHygieneLandingEvidence` (landing noise, unparseable listing, landing floor), `TestAutoMergeTreatsBaseProbeNoiseAsAlive`, and `TestGhNotFoundOnlyForRealGone` — each fails against the pre-fix behavior.* *Last updated: 2026-09-12 (landing certification: the window, not the pre-dispatch stamp, is what certifies a ship) — the ship gate could only certify a pull request the OPEN guard had admitted before dispatch, so loop 282's reopen-and-merge (#345 merged 04:39:14Z inside its window) recorded non-productive: `gh pr view --json state` never said *when* a merge happened. `prView` now reads `state,mergedAt` in that one call (all five OPEN gates unchanged, `prState` a thin wrapper), a goal-named pull request whose merge stamp falls inside `[iterStart, now]` certifies `ok` (artifact gate included) on both the failed-dispatch and the rc-0 no-pr paths, an out-of-window stamp certifies nothing (loop 281), and a goal that IS a `Land PR #N` directive whose target had already merged before the iteration is refused pre-dispatch — `skipped` row, claim retired, no worker burned (loop 280). The refusal binds to the directive's position (the statement must open with it), so a tracker implement goal carrying an issue title that reads like a merge directive still dispatches instead of halting the loop through re-pick/refuse cycles. Pinned by `TestRunLoopGoalNamedMergedInWindowRecordsShip`, `TestRunLoopGoalNamedMergedAfterNoPRRunRecordsShip`, `TestRunLoopStaleGoalHeadMergedPRSkipsDispatch`, `TestRunLoopGoalNamedOutOfWindowPRRecordsNoPR`, `TestRunLoopTrackerTitleReadingLikeMergeDirectiveStillDispatches` and `TestRunLoopGoalQuotingMergeDirectiveMidProseStillDispatches`; ceiling — the record-time scan is mention-level, so the window and the artifact gate are what qualify a mention.* diff --git a/docs/SELF-BUILD-LOOP.md b/docs/SELF-BUILD-LOOP.md index 1253c9b1..32843e26 100644 --- a/docs/SELF-BUILD-LOOP.md +++ b/docs/SELF-BUILD-LOOP.md @@ -84,6 +84,16 @@ the Node tree (FR-GO-16, #205). precedent) rather than leaving it to re-claim and re-burn the row every lease; tracker-path iterations are unaffected (`queued == nil`). + **CLOSED-but-unmerged rescue (2026-09-12).** The derivation's OPEN guard and + the rescue's own OPEN guard made a falsely closed pull request unreachable + for exactly the class the route exists for — PR #346/#347, auto-closed by the + sweep's transport-noise bug minutes after they opened, green and mergeable. A + goal-named pull request that is CLOSED, unmerged, based on `main`, with its + head ref still on origin is now taken as the subject, and the rescue reopens + it (`gh pr reopen`, repo-scoped, bounded), re-reads the state — verified, + never believed — and merges through the ordinary pipeline. Any other close + (superseded base, head ref gone, merge stamp, reopen refused) lands nothing. + **Landing certification (2026-09-12).** The one `gh pr view` now carries `state,mergedAt`, so a merge is *dated* rather than only observed: a pull request the goal names whose merge falls inside this iteration's window diff --git a/internal/loopdriver/DECISION.md b/internal/loopdriver/DECISION.md index 136be6e8..03bd88d9 100644 --- a/internal/loopdriver/DECISION.md +++ b/internal/loopdriver/DECISION.md @@ -344,3 +344,84 @@ title that would have halted the loop) and quote). (Recorded 2026-09-12; TASK-mtxwr39q-nva6.) + +## DECISION: the rescue reaches CLOSED-but-unmerged pull requests (2026-09-12, TASK-mty0vgkg-oerc) + +Every mechanism above reads a pull request's state *before* the dispatch, and +`verifyAndMergeRescue` carried the same OPEN guard — so a pull request closed +unmerged was unreachable for all of them. That is exactly the class the route +exists for: #346 and #347 were auto-closed by the zombie sweep's +`BaseBranchGone(main)` transport-noise bug minutes after they opened (green, +mergeable, `origin/main` already contained), and loop 282's #345 was CLOSED at +dispatch with the worker's job being to reopen it. Each time the goal asked for +the pull request to be landed, the branch was landable, and the iteration +recorded `no-pr`. Three pieces, all read-only until the reopen: + +1. **`prView` widens.** One `gh pr view` now decodes + `state,mergedAt,baseRefName,headRefName`. The state and merge-stamp halves + are unchanged for every existing reader (all five OPEN gates, the window + certification); the two ref names are what the reopen gate judges. +2. **The goal-text derivation takes a closed subject.** When `pick.mergePR == + 0` and the goal names a merge target (`goalMergePR`), a CLOSED reference is + taken as the evidence subject while `reopenSubject` holds: no merge stamp, + `baseRefName == main`, and the head ref still resolving on origin + (`gh api repos//branches/` through `ghAPIDecode`, the driver's own + repo-scoped read). The OPEN case is untouched and takes precedence, and a + merged reference still falls through to the pre-dispatch refusal. +3. **The rescue reopens before merging.** `verifyAndMergeRescue` no longer + bails on a non-OPEN state: it re-checks the gate — the derivation vetted the + subject before the dispatch, and an iteration is long enough for a close to + land mid-flight — runs `gh pr reopen --repo ` (repo-scoped, + 60s wall, `pipeDrainDelay` teardown, the `prView` contract), and re-reads the + state. The reopen is verified, never believed: `gh pr reopen` exiting 0 + without gh reporting OPEN is a refusal. Only then does the ordinary + `mergePRBounded` → `AutoReviewAndMergeOne` path run — that pipeline skips any + non-OPEN pull request itself, which is why the reopen has to happen here + rather than inside it. + +Ceiling (named, not fixed): "reopenable" is judged from the pull request's own +refs, not from the reason it was closed — gh does not return that in the same +read, and inferring it from the sweep's close comment would couple the rescue to +prose. A closed-unmerged pull request on main with a live head that a human +closed on purpose (superseded by newer work, abandoned) is therefore reopened +and merged when the goal names it as its merge target and the run ships nothing. +What bounds it: the goal must carry a present-tense merge directive naming that +pull request (`mergePickRe`, the verb-proximity parse this whole route rests +on), the reopen must actually take, `AutoReviewAndMergeOne` must still pass it +(checks, hazard scan, review, base probe), and the artifact gate must find its +touched implementation files on main. The residual exposure is +`mergePickRe`'s, not this change's: the scan is the whole goal text, so an +*incidental* directive-shaped mention — a goal quoting `Goal: Land PR #N`, or a +tracker issue whose TITLE reads like one ("reopen and merge PR #N") — becomes a +subject too, where the OPEN case could only certify a merge that happened anyway +and this one can reopen. Two upgrade paths, in cost order: bind the closed case +to the directive's position (`goalHeadMergePR`, the pre-dispatch refusal's +bound — rejected here because loop 282's goal carries its directive in the +second sentence), or make the parse structural (`PICK: action=merge-pr pr=#N` +from the research prompt) and key both routes on it. For a deliberate close: the +sweep records its verdict (reason plus evidence) in the ledger and +`reopenSubject` refuses a close the driver itself made. + +Pinned by `TestRunLoopClosedUnmergedGoalPRReopenedAndMerged` (derivation → +reopen → merge → `ok`, with `gh pr reopen 344` ordered before `gh pr merge +344`), `TestRunLoopClosedGoalPRLandsNothingWhenNotReopenable` (a base-superseded +close and a head ref gone from origin never become subjects, and a reopen gh +refuses ends the rescue — all three keep the `no-pr` row with no merge) and +`TestVerifyAndMergeRescueRefusesNonReopenablePR` (the rescue's own re-check +refuses an ineligible subject before any mutation). The fake gh harness gained +the reopen/merge markers these need: a `pr reopen` decides whether later +`pr view` reads say OPEN, a `pr merge` decides whether they say MERGED. + +Live decode note (the fixtures' counterpart, per this file's precedent): the +widened read and the ref probe were checked against the real repository before +the fakes were trusted. `gh pr view 347 --repo FreePeak/devagent --json +state,mergedAt,baseRefName,headRefName` answers `{"baseRefName":"main", +"headRefName":"devagent/TASK-mtxyflr1-fq6y","mergedAt":null,"state":"CLOSED"}` — +an unmerged close carries a JSON `null` stamp, which decodes to the empty string +`mergedAtTime` already treats as no evidence — and `gh api +repos/FreePeak/devagent/branches/devagent/TASK-mtxyflr1-fq6y` resolves the +slash-containing ref, while a missing one answers exit 1 with "Branch not found +(HTTP 404)": the shape `GH_BRANCHES_GONE` reproduces. #347 is itself that class — +closed unmerged on main, its head ref still on origin. + +(Recorded 2026-09-12; TASK-mty0vgkg-oerc.) diff --git a/internal/loopdriver/issues.go b/internal/loopdriver/issues.go index 778bc013..8bf16dba 100644 --- a/internal/loopdriver/issues.go +++ b/internal/loopdriver/issues.go @@ -103,34 +103,44 @@ func (d *driver) closeIssue(num int, comment string) { _ = cmd.Run() } +// prViewResult is one `gh pr view --json state,mergedAt,baseRefName,headRefName` +// read. The state half answers the OPEN gates, the merge stamp answers the +// landing-evidence window, and the two ref names answer the CLOSED-but-unmerged +// reopen gate (run.go): a closed pull request is only reopenable while it still +// targets main and its head ref is still on origin. +type prViewResult struct { + State string `json:"state"` + MergedAt string `json:"mergedAt"` + BaseRefName string `json:"baseRefName"` + HeadRefName string `json:"headRefName"` +} + // prView reads gh's view of pull request num: its state (OPEN | MERGED | -// CLOSED, "" when gh cannot say) and its merge stamp mergedAt (second- -// precision RFC3339, empty when it has not merged). One -// `gh pr view --json state,mergedAt` per call, so the landing-evidence -// readers share a read instead of one gh call per field. Decoded rather than -// substring-matched: the verdicts gate a merge, gh's JSON shape is not -// contractual, and decoding is this file's own convention (pickIssue). -func (d *driver) prView(num int) (state, mergedAt string) { +// CLOSED, "" when gh cannot say), its merge stamp mergedAt (second-precision +// RFC3339, empty when it has not merged), and the base/head ref names the +// reopen gate needs. One `gh pr view --json …` per call, so the +// landing-evidence readers share a read instead of one gh call per field. +// Decoded rather than substring-matched: the verdicts gate a merge, gh's JSON +// shape is not contractual, and decoding is this file's own convention +// (pickIssue). +func (d *driver) prView(num int) prViewResult { ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() cmd := exec.CommandContext(ctx, d.cfg.GhBin, "pr", "view", strconv.Itoa(num), - "--repo", d.cfg.GHRepo, "--json", "state,mergedAt") + "--repo", d.cfg.GHRepo, "--json", "state,mergedAt,baseRefName,headRefName") cmd.Dir = d.cfg.Repo cmd.WaitDelay = pipeDrainDelay out, err := cmd.Output() // A drain cutoff (ErrWaitDelay) must not read the view as failed when // gh itself exited 0 (#286). if err != nil && (cmd.ProcessState == nil || !cmd.ProcessState.Success()) { - return "", "" - } - var view struct { - State string `json:"state"` - MergedAt string `json:"mergedAt"` + return prViewResult{} } + var view prViewResult if err := json.Unmarshal(out, &view); err != nil { - return "", "" + return prViewResult{} } - return view.State, view.MergedAt + return view } // prState is the state-only half of prView — the reader the OPEN gates use. @@ -138,8 +148,7 @@ func (d *driver) prView(num int) (state, mergedAt string) { // inside it" reads prView instead (the landing-evidence certification in // run.go). func (d *driver) prState(num int) string { - state, _ := d.prView(num) - return state + return d.prView(num).State } // prMerged reports whether pull request num has merged — the publish evidence diff --git a/internal/loopdriver/run.go b/internal/loopdriver/run.go index 1137aab0..9186129f 100644 --- a/internal/loopdriver/run.go +++ b/internal/loopdriver/run.go @@ -9,9 +9,11 @@ package loopdriver import ( + "context" "fmt" "io" "os" + "os/exec" "path/filepath" "regexp" "strconv" @@ -393,8 +395,13 @@ func (d *driver) runIteration(n int, logF io.Writer, gradient, clusters string) // passes against a starvation limit of five. Derive the subject from the // goal text with the matcher researchPick parses, under the pick-time OPEN // guard above: a stale or incidental PR mention self-disqualifies, so - // nothing here can certify a do-nothing iteration. The derived PR then - // flows through the prMerged/evidencePR/landedArtifactsVerified wiring. + // nothing here can certify a do-nothing iteration. A closed-and-unmerged + // reference is the same subject one step earlier — the branch never + // landed, so the goal is still unfulfilled — and is taken only while the + // reopen gate (reopenSubject: main base, head ref on origin, unmerged) + // says the close is the only obstruction; the derivation then hands the + // rescue a subject it reopens before merging. The derived PR flows + // through the prMerged/evidencePR/landedArtifactsVerified wiring. // // The same read carries the pre-dispatch half of the certification: a // goal that IS a "Land PR #N" directive naming a pull request that had @@ -411,14 +418,23 @@ func (d *driver) runIteration(n int, logF io.Writer, gradient, clusters string) // loop into the starvation halt with nothing to retire. if pick.mergePR == 0 { if pr := goalMergePR(goal); pr != 0 { - state, mergedAt := d.prView(pr) - at, merged := mergedAtTime(mergedAt) + view := d.prView(pr) + at, merged := mergedAtTime(view.MergedAt) switch { - case state == "OPEN": + case view.State == "OPEN": pick.mergePR = pr _, _ = fmt.Fprintf(logF, "[verify] goal names open PR #%d — deriving the merge-evidence subject from the goal text\n", pr) + case d.reopenSubject(view): + // The CLOSED half of the same route: a pull request closed + // unmerged (the false zombie-sweep verdict of #346/#347, a + // superseded close, a stale close) still carries the branch + // the goal names, so it becomes the rescue's subject — which + // reopens it (reopenClosedPR) before merging. Only a + // reopenable close qualifies: main base, head ref on origin. + pick.mergePR = pr + _, _ = fmt.Fprintf(logF, "[verify] goal names closed-unmerged PR #%d based on %s with its head ref on origin — deriving the reopen-and-merge subject from the goal text\n", pr, mainBranch) case merged && at.Before(iterStart) && goalHeadMergePR(goal) == pr: - _, _ = fmt.Fprintf(logF, "[verify] goal-head PR #%d merged %s, before this iteration started — skipping dispatch\n", pr, mergedAt) + _, _ = fmt.Fprintf(logF, "[verify] goal-head PR #%d merged %s, before this iteration started — skipping dispatch\n", pr, view.MergedAt) d.record(logF, n, "skipped", goal) // The claim is retired for the same reason the shape gate // retires one: left live it re-claims after its lease and @@ -797,14 +813,20 @@ func (d *driver) mergePRBounded(n int, logF io.Writer, pr int) bool { // verifyAndMergeRescue closes the verify-and-merge loop for an iteration // whose run ended without shipping evidence: when the picked issue still has -// an OPEN pull request — research's pick (OPEN-guarded at pick time), a -// cross-referenced one, or one the run itself opened — the driver lands it -// through mergePRBounded and reports its number (0 = nothing landed). +// a pull request to land — research's pick (OPEN-guarded at pick time), a +// cross-referenced one, one the run itself opened, or the goal-named +// CLOSED-but-unmerged subject the derivation above took — the driver lands it +// through mergePRBounded and reports its number (0 = nothing landed). A +// closed subject is reopened first (reopenClosedPR) and only when the merge +// could not have happened without it; a refusal lands nothing. func (d *driver) verifyAndMergeRescue(n int, logF io.Writer, pr, issueNum int) int { if pr == 0 && issueNum != 0 { pr = d.openPROfIssue(issueNum) } - if pr == 0 || d.prState(pr) != "OPEN" { + if pr == 0 { + return 0 + } + if d.prState(pr) != "OPEN" && !d.reopenClosedPR(logF, pr) { return 0 } _, _ = fmt.Fprintf(logF, "[verify] run ended without shipping evidence — driver-side verify-and-merge of open PR #%d\n", pr) @@ -814,6 +836,69 @@ func (d *driver) verifyAndMergeRescue(n int, logF io.Writer, pr, issueNum int) i return 0 } +// mainBranch is the base branch a reopen-and-merge subject must target: the +// loop's own landing branch, hardcoded the way mainTreePaths' git/trees/main +// is. A pull request closed against anything else (a superseded base, a +// stacked branch) carries a branch the driver cannot land. +const mainBranch = "main" + +// reopenSubject reports whether a pull request the goal text names is a +// CLOSED-but-unmerged merge subject: closed (not merged), based on main, and +// its head ref still on origin. The class it serves is the false close — +// #346/#347 were auto-closed by the zombie sweep's BaseBranchGone +// transport-noise bug minutes after they opened, with green, mergeable +// branches — where the goal asks for the pull request to be landed and the +// only obstruction is the close itself. +// +// Read-only: the goal-text derivation asks it before deciding to dispatch, +// and reopenClosedPR asks it again before mutating anything. Every miss is +// the conservative direction (no subject, no reopen): a merged stamp, a base +// the driver does not land on, a deleted head ref, and a gh that cannot +// answer the ref probe all read as not reopenable, so a stale or incidental +// PR mention can never fabricate a ship. +func (d *driver) reopenSubject(view prViewResult) bool { + if view.State != "CLOSED" || view.HeadRefName == "" || view.BaseRefName != mainBranch { + return false + } + if _, merged := mergedAtTime(view.MergedAt); merged { + return false + } + return d.ghAPIDecode(fmt.Sprintf("repos/%s/branches/%s", d.cfg.GHRepo, view.HeadRefName), &struct{}{}) +} + +// reopenClosedPR reopens pull request pr and reports whether it is OPEN +// afterwards — the closed half of the rescue. gh's reopen is repo-scoped and +// bounded like every other driver gh call (60s wall, drain-tolerant teardown +// per #286), and its verdict is not believed: the state is re-read after the +// call, so "reopened" means gh says OPEN, never that the command exited 0. +// Anything else returns false and the rescue lands nothing — the +// base-superseded and reopen-refused classes. +func (d *driver) reopenClosedPR(logF io.Writer, pr int) bool { + if view := d.prView(pr); !d.reopenSubject(view) { + _, _ = fmt.Fprintf(logF, "[verify] PR #%d is %s (base %s, head %q) — not a reopen-and-merge subject\n", + pr, view.State, view.BaseRefName, view.HeadRefName) + return false + } + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, d.cfg.GhBin, "pr", "reopen", strconv.Itoa(pr), "--repo", d.cfg.GHRepo) + cmd.Dir = d.cfg.Repo + cmd.WaitDelay = pipeDrainDelay + out, err := cmd.CombinedOutput() + // Same drain tolerance as prView: a cutoff that crossed gh's own rc 0 is + // not a refusal. + if err != nil && (cmd.ProcessState == nil || !cmd.ProcessState.Success()) { + _, _ = fmt.Fprintf(logF, "[verify] gh pr reopen %d refused: %s\n", pr, strings.TrimSpace(string(out))) + return false + } + if state := d.prState(pr); state != "OPEN" { + _, _ = fmt.Fprintf(logF, "[verify] PR #%d did not reopen (state %s)\n", pr, state) + return false + } + _, _ = fmt.Fprintf(logF, "[verify] reopening closed-unmerged PR #%d to land it\n", pr) + return true +} + // applyPrHygieneTriage runs the shipped pr-hygiene sweep in apply mode after // a ship: zombie TASK PRs (base-superseded, red-across-grace) and the // landing-evidence triage of issues cited by open PRs (#336) act for real, @@ -914,7 +999,8 @@ var researchPickHeadingRe = regexp.MustCompile(`(?im)^\s*#+\s*pick\b`) // direction: the merge route prints no "PR opened:", so shipping rests on // prMerged, which is true for a PR that merged weeks ago, and the iteration // would record a productive row AND close an issue nobody worked. runIteration -// caps that: the route requires prState == OPEN at pick time, so every stale or +// caps that: the route requires a subject — OPEN at pick time, or (goal-text +// derivation) CLOSED-but-unmerged past the reopen gate — so every stale or // incidental PR reference self-disqualifies and a miss degrades to today's // implement dispatch. Upgrade path if phrasing ever defeats the parse: have the // research prompt emit `PICK: issue=#N action=merge-pr pr=#N @@ -989,11 +1075,11 @@ func (d *driver) goalMergedWithin(goal string, start, now time.Time) int { if err != nil || pr <= 0 { continue } - state, mergedAt := d.prView(pr) - if state != "MERGED" { + view := d.prView(pr) + if view.State != "MERGED" { continue } - at, ok := mergedAtTime(mergedAt) + at, ok := mergedAtTime(view.MergedAt) if !ok || at.Before(start) || at.After(now) { continue } diff --git a/internal/loopdriver/run_test.go b/internal/loopdriver/run_test.go index c03eb6bd..713326c3 100644 --- a/internal/loopdriver/run_test.go +++ b/internal/loopdriver/run_test.go @@ -76,7 +76,25 @@ case "$1 $2" in # gate's skip branch). Every other api call still answers the timeline. "api repos/"*"/pulls/"*"/files"*) printf '%s' "$GH_PR_FILES_JSON" ;; "api repos/"*"/git/trees/"*) printf '%s' "$GH_TREE_JSON" ;; + # The reopen gate's head-ref probe (repos//branches/): a + # decodable body means the ref resolves on origin, GH_BRANCHES_GONE answers + # the definitive 404 (the not-reopenable direction) instead. + "api repos/"*"/branches/"*) + if [ -n "${GH_BRANCHES_GONE:-}" ]; then echo "Branch not found (HTTP 404)" >&2; exit 1; fi + printf '{"name":"branch"}\n' ;; api\ *) printf '%s' "$GH_TIMELINE_JSON" ;; + # The rescue's reopen, modelled as the state change it is: a nonzero + # GH_PR_REOPEN_RC is the refusal; on success GH_PR_REOPEN_FILE appears and + # every later pr view reads OPEN. A pr merge marks GH_PR_MERGE_FILE the + # same way, so a merged state follows the action instead of a call count. + "pr reopen") + rc="${GH_PR_REOPEN_RC:-0}" + if [ "$rc" != "0" ]; then echo "could not reopen pull request" >&2; exit "$rc"; fi + [ -n "${GH_PR_REOPEN_FILE:-}" ] && : > "$GH_PR_REOPEN_FILE" + exit 0 ;; + "pr merge") + [ -n "${GH_PR_MERGE_FILE:-}" ] && : > "$GH_PR_MERGE_FILE" + exit 0 ;; # gh 2.98 pipes --json output (compact); prState decodes, so the shape is # the real one and the driver must not care. GH_PR_STATE answers every call; # GH_PR_STATE_FIRST + GH_PR_STATE_FILE sequence the first one differently @@ -95,14 +113,16 @@ case "$1 $2" in c=$((c + 1)); echo "$c" > "${GH_PR_STATE_OPEN_CALLS_FILE}" [ "$c" -le "$GH_PR_STATE_OPEN_CALLS" ] && s="OPEN" || s="${GH_PR_STATE:-OPEN}" fi + [ -n "${GH_PR_REOPEN_FILE:-}" ] && [ -f "$GH_PR_REOPEN_FILE" ] && s="OPEN" + [ -n "${GH_PR_MERGE_FILE:-}" ] && [ -f "$GH_PR_MERGE_FILE" ] && s="MERGED" # mergedAt rides the same --json read (gh omits the key when unset): # GH_PR_MERGED_AT answers the record-time landing certification, so a # MERGED stamp without it is gh-cannot-say, never a certified ship. - if [ -n "${GH_PR_MERGED_AT:-}" ]; then - printf '{"state":"%s","mergedAt":"%s"}\n' "$s" "$GH_PR_MERGED_AT" - else - printf '{"state":"%s"}\n' "$s" - fi ;; + # baseRefName/headRefName ride it too — the reopen gate reads them, and + # main + a TASK branch is the shape the live repo has. + printf '{"state":"%s","baseRefName":"%s","headRefName":"%s"' "$s" "${GH_PR_BASE_REF:-main}" "${GH_PR_HEAD_REF:-devagent/TASK-fixture}" + if [ -n "${GH_PR_MERGED_AT:-}" ]; then printf ',"mergedAt":"%s"' "$GH_PR_MERGED_AT"; fi + printf '}\n' ;; esac exit 0 `, @@ -1096,6 +1116,197 @@ func TestRunLoopGoalNamedOutOfWindowPRRecordsNoPR(t *testing.T) { } } +// The CLOSED-but-unmerged half of the goal-derived subject. #346/#347 were +// auto-closed by the zombie sweep's BaseBranchGone transport-noise bug minutes +// after they opened, with green, mergeable branches, so a goal that asks for +// the pull request to be landed named a subject the derivation's OPEN guard +// refused — and verifyAndMergeRescue's own OPEN guard refused it a second +// time. The rescue was unreachable for exactly the class it exists for. A +// closed-unmerged pull request is now a subject when the close is the only +// obstruction (main base, head ref on origin, no merge stamp), and the rescue +// reopens it before merging: gh's reopen is not believed, the state is +// re-read, and only then does the ordinary merge path run. +func TestRunLoopClosedUnmergedGoalPRReopenedAndMerged(t *testing.T) { + repo := initFixtureRepo(t) + installFakes(t, repo) + if _, err := queue.EnqueueTask(repo, queue.EnqueueInput{ + ID: "TASK-1", + Title: "Land PR 344", + Goal: "Goal: Land PR #344 (branch `devagent/TASK-mtxqd5xx-23cu`) — verify it and merge it; do not re-implement it.", + }); err != nil { + t.Fatal(err) + } + t.Setenv("DEVAGENT_FAKE_TASK_NO_PR", "1") + // Closed unmerged on main with its head ref still on origin — the false + // close the goal asks the loop to undo. The reopen/merge markers are the + // fake's state changes: OPEN after the reopen, MERGED after the merge. + t.Setenv("GH_PR_STATE", "CLOSED") + t.Setenv("GH_PR_HEAD_REF", "devagent/TASK-mtxqd5xx-23cu") + t.Setenv("GH_PR_REOPEN_FILE", filepath.Join(t.TempDir(), "reopened")) + t.Setenv("GH_PR_MERGE_FILE", filepath.Join(t.TempDir(), "merged")) + t.Setenv("GH_PR_FILES_JSON", `[{"filename":"internal/loopdriver/run.go"}]`) + t.Setenv("GH_TREE_JSON", `{"tree":[{"path":"internal/loopdriver/run.go","type":"blob"}],"truncated":false}`) + cfg := loopConfigFor(t, repo, func(c *LoopConfig) { + c.Now = func() time.Time { return certWindowNow() } + c.GHRepo = "FreePeak/devagent" + c.GhRun = execFakeGh + }) + cfg.DryRun = false + if rc := RunLoop(cfg); rc != 0 { + logData, _ := os.ReadFile(filepath.Join(repo, ".selfbuild", "logs", "loop-1.log")) + t.Fatalf("rc = %d, want 0\nlog:\n%s", rc, logData) + } + rows := readLedger(t, repo) + if len(rows) != 1 || rows[0]["status"] != "ok" { + t.Fatalf("rows: %v, want the ok row for a reopened-and-merged PR (never no-pr)", rows) + } + logData, err := os.ReadFile(filepath.Join(repo, ".selfbuild", "logs", "loop-1.log")) + if err != nil { + t.Fatal(err) + } + log := string(logData) + // Both ends of the route: the derivation took the closed subject, the + // rescue reopened it and only then merged. + if !strings.Contains(log, "[verify] goal names closed-unmerged PR #344 based on main with its head ref on origin — deriving the reopen-and-merge subject from the goal text") { + t.Fatalf("the closed subject was not derived:\n%s", log) + } + if !strings.Contains(log, "[verify] reopening closed-unmerged PR #344 to land it") { + t.Fatalf("the rescue did not reopen the closed PR:\n%s", log) + } + // gh's own actions, in order: reopen before merge. + calls, err := os.ReadFile(filepath.Join(repo, "devagent-calls.log")) + if err != nil { + t.Fatal(err) + } + reopenAt := strings.Index(string(calls), "gh pr reopen 344") + mergeAt := strings.Index(string(calls), "gh pr merge 344") + if reopenAt < 0 || mergeAt < 0 { + t.Fatalf("want both gh pr reopen 344 and gh pr merge 344, calls:\n%s", calls) + } + if reopenAt > mergeAt { + t.Fatalf("the merge ran before the reopen, calls:\n%s", calls) + } + // The ship is artifact-backed like every other merge verdict. + if !strings.Contains(log, "[verify] PR #344's implementation files verified on main") { + t.Fatalf("artifact verification not logged:\n%s", log) + } + assertFileContains(t, filepath.Join(repo, ".devagent", "queue", "TASK-1.json"), `"status": "done"`) +} + +// The reopen gate's misses land NOTHING: a close the driver may not undo (a +// base it does not land on, a head ref gone from origin) never becomes a +// subject, and a reopen gh refuses ends the rescue without a merge. All three +// keep the ordinary no-pr verdict with its claim retired — a fabricated ship +// is the failure mode this route exists to avoid. +func TestRunLoopClosedGoalPRLandsNothingWhenNotReopenable(t *testing.T) { + cases := []struct { + name string + env map[string]string + // wantReopenCall is false when the gate refuses the subject before any + // mutation, true when gh's reopen was attempted and refused. + wantReopenCall bool + wantLog string + }{ + { + name: "base-superseded: closed against another base", + env: map[string]string{"GH_PR_BASE_REF": "release/0.3"}, + }, + { + name: "head ref gone from origin", + env: map[string]string{"GH_BRANCHES_GONE": "1"}, + }, + { + name: "reopen refused by gh", + env: map[string]string{"GH_PR_REOPEN_RC": "1"}, + wantReopenCall: true, + wantLog: "[verify] gh pr reopen 344 refused: could not reopen pull request", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + repo := initFixtureRepo(t) + installFakes(t, repo) + if _, err := queue.EnqueueTask(repo, queue.EnqueueInput{ + ID: "TASK-1", + Title: "Land PR 344", + Goal: "Goal: Land PR #344 (branch `devagent/TASK-mtxqd5xx-23cu`) — verify it and merge it; do not re-implement it.", + }); err != nil { + t.Fatal(err) + } + t.Setenv("DEVAGENT_FAKE_TASK_NO_PR", "1") + t.Setenv("GH_PR_STATE", "CLOSED") + t.Setenv("GH_PR_HEAD_REF", "devagent/TASK-mtxqd5xx-23cu") + t.Setenv("GH_PR_REOPEN_FILE", filepath.Join(t.TempDir(), "reopened")) + t.Setenv("GH_PR_MERGE_FILE", filepath.Join(t.TempDir(), "merged")) + for k, v := range tc.env { + t.Setenv(k, v) + } + cfg := loopConfigFor(t, repo, func(c *LoopConfig) { + c.Now = func() time.Time { return certWindowNow() } + c.GHRepo = "FreePeak/devagent" + c.GhRun = execFakeGh + }) + cfg.DryRun = false + if rc := RunLoop(cfg); rc != 0 { + logData, _ := os.ReadFile(filepath.Join(repo, ".selfbuild", "logs", "loop-1.log")) + t.Fatalf("rc = %d, want 0\nlog:\n%s", rc, logData) + } + rows := readLedger(t, repo) + if len(rows) != 1 || rows[0]["status"] != "no-pr" { + t.Fatalf("rows: %v, want the no-pr row (nothing was landable)", rows) + } + calls, err := os.ReadFile(filepath.Join(repo, "devagent-calls.log")) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(calls), "gh pr merge 344") { + t.Fatalf("a merge ran for a PR that was never reopened, calls:\n%s", calls) + } + if got := strings.Contains(string(calls), "gh pr reopen 344"); got != tc.wantReopenCall { + t.Fatalf("gh pr reopen 344 attempted = %v, want %v, calls:\n%s", got, tc.wantReopenCall, calls) + } + logData, err := os.ReadFile(filepath.Join(repo, ".selfbuild", "logs", "loop-1.log")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(logData), "[publish] task succeeded without opening a PR") { + t.Fatalf("the no-pr verdict was not recorded:\n%s", logData) + } + if tc.wantLog != "" && !strings.Contains(string(logData), tc.wantLog) { + t.Fatalf("want log %q:\n%s", tc.wantLog, logData) + } + }) + } +} + +// The rescue's own guard, unit-level: the derivation vetted the subject before +// dispatch, but the close can land while the run is in flight (the zombie +// sweep, a human, a base change), so verifyAndMergeRescue re-checks before +// mutating anything. A subject that no longer passes the gate is refused and +// nothing merges — the reachability the ticket adds must not become "reopen +// anything the goal names". +func TestVerifyAndMergeRescueRefusesNonReopenablePR(t *testing.T) { + repo := initFixtureRepo(t) + installFakes(t, repo) + t.Setenv("GH_PR_STATE", "CLOSED") + t.Setenv("GH_PR_BASE_REF", "release/0.3") + d := &driver{cfg: loopConfigFor(t, repo, func(c *LoopConfig) { c.GHRepo = "FreePeak/devagent" })} + var log bytes.Buffer + if pr := d.verifyAndMergeRescue(1, &log, 344, 0); pr != 0 { + t.Fatalf("verifyAndMergeRescue = %d, want 0 for a base-superseded close", pr) + } + if !strings.Contains(log.String(), "[verify] PR #344 is CLOSED (base release/0.3, head \"devagent/TASK-fixture\") — not a reopen-and-merge subject") { + t.Fatalf("the refusal was not logged:\n%s", log.String()) + } + calls, err := os.ReadFile(filepath.Join(repo, "devagent-calls.log")) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(calls), "gh pr reopen") || strings.Contains(string(calls), "gh pr merge") { + t.Fatalf("an ineligible subject was mutated, calls:\n%s", calls) + } +} + // The position bound on the pre-dispatch refusal, loop 281's shape: the goal // QUOTES a merge directive ("PO goals like `Goal: Land PR #344`…") mid-prose // while its own work is something else, and that quoted pull request had