From 1f2ef8cf0e91070570529d69e3a2777ccd75c0e8 Mon Sep 17 00:00:00 2001 From: "linh.doan" Date: Sat, 12 Sep 2026 13:09:41 +0700 Subject: [PATCH] devagent(TASK-mtxyflr1-fq6y): auto-cleanup snapshot --- docs/PRD.html | 82 +++++++++-- docs/PRD.md | 15 ++ internal/cli/actions_gates.go | 2 +- internal/orchestrator/autopr-merge.go | 12 +- internal/orchestrator/autopr.go | 54 ++++++- internal/orchestrator/autopr_test.go | 51 +++++++ internal/orchestrator/pr-hygiene.go | 119 ++++++++++++--- internal/orchestrator/pr-hygiene_test.go | 178 ++++++++++++++++++++++- 8 files changed, 458 insertions(+), 55 deletions(-) diff --git a/docs/PRD.html b/docs/PRD.html index 11491ba7..86f40ecb 100644 --- a/docs/PRD.html +++ b/docs/PRD.html @@ -2085,7 +2085,23 @@

Phase 4 — Expansion (post-v1)

(#N) landing commit on main, or superseded with an explicit not-shipped comment when no landing commit exists; an open issue keeps the PR open with a once-per-PR evidence comment -(internal/orchestrator/pr-hygiene.go). Legacy +(internal/orchestrator/pr-hygiene.go). Both sweep probes are +three-valued (2026-09-12): a base branch counts as gone +only on gh's own not-found answer, and the landing listing reports "no +landing commit" only when it actually answered — transport noise +(network failure, 5xx, rate limit) is unknown, so the sweep +acts on nothing (untouched/base-unknown, +untouched/landing-unknown) and the merge-queue +gate reads it as alive instead of parking a mergeable PR +(internal/orchestrator/autopr.go). No auto-close runs before an age +floor (2026-09-12): a definitive verdict alone no longer closes a PR — +closeAgeFloorHours (24h, capped by +prHygiene.graceHours so a long configured window never +delays cleanup by months) must also have elapsed, and a PR with an +unknown age is never closed. Fresh PRs are reported +untouched (base-superseded-age-floor, +landing-age-floor) carrying the verdict that will close +them once they age past the floor. Legacy mergeProjectBranches gated so a board that published per-task PRs no longer double-merges (PR #107, src/cli.ts:850). Adapter-declared @@ -4844,20 +4860,56 @@

23.1 Requirements

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

-

Last updated: 2026-09-12 (fallback-route landing evidence: the -ship verdict off the tracker path) — pick.mergePR is only -ever set by a tracker pick, so a queue- or PO-delivered goal -(Goal: Land PR #N) had no evidence subject at all: loops -270/271/274/280 each logged falling back to LLM selection, -each dispatched a goal naming the pull request to land, each of those -pull requests merged inside its own iteration window, and each recorded -the non-productive no-pr row — four false negatives in -eleven passes against a starvation limit of five, with the Q27 -already-shipped guard equally blind. runIteration now -derives the merge target from the goal text (goalMergePR, -the mergePickRe researchPick already parses) before the -task dispatch, under the pick-time prState == OPEN guard, -so the derived pull request rides the existing prMerged → +

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/<base> 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 (fallback-route +landing evidence: the ship verdict off the tracker path) — +pick.mergePR is only ever set by a tracker pick, so a +queue- or PO-delivered goal (Goal: Land PR #N) had no +evidence subject at all: loops 270/271/274/280 each logged +falling back to LLM selection, each dispatched a goal +naming the pull request to land, each of those pull requests merged +inside its own iteration window, and each recorded the non-productive +no-pr row — four false negatives in eleven passes against a +starvation limit of five, with the Q27 already-shipped guard equally +blind. runIteration now derives the merge target from the +goal text (goalMergePR, the mergePickRe +researchPick already parses) before the task dispatch, under the +pick-time prState == OPEN guard, so the derived pull +request rides the existing prMerged → evidencePR → landedArtifactsVerified wiring and records ok with its artifacts verified — never no-pr, while a stale mention self-disqualifies and a diff --git a/docs/PRD.md b/docs/PRD.md index fad8218b..1ccc74b3 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -841,6 +841,20 @@ Webhook-triggered runs with HMAC verification and dedup, run dashboard/status co > `superseded` with an explicit not-shipped comment when no landing commit > exists; an open issue keeps the PR open with a once-per-PR evidence > comment (internal/orchestrator/pr-hygiene.go). +> Both sweep probes are three-valued (2026-09-12): a base branch counts as +> `gone` only on gh's own not-found answer, and the landing listing reports +> "no landing commit" only when it actually answered — transport noise +> (network failure, 5xx, rate limit) is `unknown`, so the sweep acts on +> nothing (`untouched`/`base-unknown`, `untouched`/`landing-unknown`) and the +> merge-queue gate reads it as alive instead of parking a mergeable PR +> (internal/orchestrator/autopr.go). +> No auto-close runs before an age floor (2026-09-12): a definitive verdict +> alone no longer closes a PR — `closeAgeFloorHours` (24h, capped by +> `prHygiene.graceHours` so a long configured window never delays cleanup by +> months) must also have elapsed, and a PR with an unknown age is never +> closed. Fresh PRs are reported `untouched` (`base-superseded-age-floor`, +> `landing-age-floor`) carrying the verdict that will close them once they +> age past the floor. > Legacy `mergeProjectBranches` gated so a > board that published per-task PRs no longer double-merges (PR #107, > `src/cli.ts:850`). Adapter-declared `WorkerAdapter.isProgress` replaces the @@ -1623,6 +1637,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 (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 (fallback-route landing evidence: the ship verdict off the tracker path) — `pick.mergePR` is only ever set by a tracker pick, so a queue- or PO-delivered goal (`Goal: Land PR #N`) had no evidence subject at all: loops 270/271/274/280 each logged `falling back to LLM selection`, each dispatched a goal naming the pull request to land, each of those pull requests merged inside its own iteration window, and each recorded the non-productive `no-pr` row — four false negatives in eleven passes against a starvation limit of five, with the Q27 already-shipped guard equally blind. `runIteration` now derives the merge target from the goal text (`goalMergePR`, the `mergePickRe` researchPick already parses) before the task dispatch, under the pick-time `prState == OPEN` guard, so the derived pull request rides the existing `prMerged` → `evidencePR` → `landedArtifactsVerified` wiring and records `ok` with its artifacts verified — never `no-pr`, while a stale mention self-disqualifies and a derived PR still open at record time is the driver-side verify-and-merge rescue's subject. The `no-pr` early return now also retires its queue claim `failed` with the `no-pr` detail (the shape gate's lease-recycling precedent) instead of leaving it to re-claim and re-burn the row every lease; tracker iterations are untouched (`queued == nil`). Pinned by `TestRunLoopGoalNamedOpenPRMergedRecordsShip`, `TestRunLoopGoalNamedMergedPRRecordsNoPR` and `TestGoalMergePR`; `internal/loopdriver/DECISION.md` records the decision.* *Last updated: 2026-09-12 (landing evidence: a ship needs artifacts on main) — `merged = shipped` trusted gh's status stamp alone, and three consecutive iterations (276–278) proved the stamp lies: loop 276 recorded `ok` for PR #342 (merged only as an auto-cleanup snapshot — the test file it was meant to add never reached main) and loop 277 burned a full iteration re-running that already-"landed" goal. The driver's verify-and-merge path now verifies the artifact half: `landedArtifactsVerified` (internal/loopdriver/artifacts.go) lists the merged PR's touched files and main's tree over the `gh api` files API and requires every touched implementation file to exist on main (docs surfaces exempt — a PRD regen ships alone; a file the PR `deleted` is no claim, and a truncated tree list reads as cannot-say); a mismatch records the non-productive `landed-without-artifact` row and returns before the `ok` write, so the queue claim and tracker issue stay live, and a pick-time preflight greps the ledger for the goal's fingerprint (the Q27 guard's 60-char key) on those rows and skips the dispatch instead of re-reading the same MERGED stamp — retiring that claim as `failed` with the preflight detail (the shape gate's lease-recycling precedent) while the issue stays open. A gh that cannot answer logs and passes — evidence gate, not a second availability gate — and both decoders were checked against the real repository before the fixtures were trusted. Covered by `TestRunLoopMergedWithoutArtifactRecordsEvidenceRow`, `TestRunLoopMergePickRescuedByDriverSideMerge` (artifact-backed ship), `TestRunLoopPreflightSkipsGoalLandedWithoutArtifact`, `TestLandedArtifactsVerifiedIgnoresRemovedAndTruncated` and `TestLedgerLandingFingerprint`; the goal-shape fix PR #343 was recovered in the same iteration (re-opened, head 50891d5 verified with `go test ./...` in a throwaway worktree, merged as 38e6513).* *Last updated: 2026-09-12 (goal-shape follow-up: queue-claim retirement + producer scope) — the shape gate's word cap binds every producer, not just PO output: a human goal from `POST /dispatch` or a scout-enqueued queue row past the 120-word statement budget is refused at the boundary too (the PO prompt is the one statement contract all dispatched goals trace back to; DECISION.md divergence #5 records the reasoning). The same pass closed the leak the cap opened: a queued claim whose goal is off-contract is now retired as `failed` with the rejection detail as its LastError at BOTH invalid sites (the goal-file gate and the taskDispatch boundary) — pre-fix the claim sat until its 2h lease lapsed, re-claimed, and re-burned an invalid row every cycle, walking the loop into the starvation halt with the row still live. Covered by `TestRunLoopInvalidQueuedGoalRetiresTheClaim` (run_test.go).* diff --git a/internal/cli/actions_gates.go b/internal/cli/actions_gates.go index 8eba3767..18c05631 100644 --- a/internal/cli/actions_gates.go +++ b/internal/cli/actions_gates.go @@ -659,7 +659,7 @@ func newPrHygieneCmd() *cobra.Command { }, } cmd.Flags().String("repo", "", "target repository (used for gh context)") - cmd.Flags().Float64("grace-hours", -1, "hours a PR may stay red before being flagged (default from config prHygiene.graceHours)") + cmd.Flags().Float64("grace-hours", -1, "hours a PR may stay red before being flagged; also the auto-close age floor, capped at 24h (default from config prHygiene.graceHours)") cmd.Flags().Bool("auto-merge", false, "report skipAutoMerge when a red-across-grace PR would have merged (default from config autoMerge)") cmd.Flags().Bool("apply", false, "comment and close for real (default: config prHygiene.dryRun, itself defaulting to dry-run)") return cmd diff --git a/internal/orchestrator/autopr-merge.go b/internal/orchestrator/autopr-merge.go index 276af311..ee16a7c9 100644 --- a/internal/orchestrator/autopr-merge.go +++ b/internal/orchestrator/autopr-merge.go @@ -185,13 +185,11 @@ func AutoReviewAndMergeOne(repoPath string, pr int, opts AutoReviewAndMergeOneOp // Merge-queue gate 1 (base-superseded): a PR whose head base branch was // merged or deleted can never integrate — waiting cannot fix a dead // base, so skip it with a reason instead of parking the pipeline. - // Auto-close of such PRs stays owned by the pr-hygiene sweep. A probe - // hiccup cannot be distinguished from a dead base through the seam's - // boolean — the TS try/catch swallows probe errors into "fall through"; - // the scripted seam mirrors gh by throwing only on real 404s, so this - // port probes through an error-carrying call and treats transport noise - // as alive (matching the tolerant TS behavior for non-404 failures). - if BaseBranchGone(repoPath, status.BaseRefName, run) { + // Auto-close of such PRs stays owned by the pr-hygiene sweep. Only a + // definitive gh 404 skips the PR: the probe is three-valued, and + // transport noise (BaseUnknown) reads as alive so a probe hiccup never + // parks a mergeable PR. + if ProbeBaseBranch(repoPath, status.BaseRefName, run) == BaseGone { return AutoMergeOutcome{ PR: pr, Title: status.Title, diff --git a/internal/orchestrator/autopr.go b/internal/orchestrator/autopr.go index e4633ddd..e0556ae3 100644 --- a/internal/orchestrator/autopr.go +++ b/internal/orchestrator/autopr.go @@ -18,6 +18,7 @@ package orchestrator import ( "encoding/json" + "errors" "fmt" "math" "regexp" @@ -566,13 +567,54 @@ func GetPrStatus(repoPath string, pr int, run RunGh) (PrStatus, error) { return parsePr(raw), nil } -// BaseBranchGone mirrors the TS baseBranchGone: a base branch is gone when -// gh cannot resolve the ref (merged+deleted or deleted directly). Shared by -// the merge-queue gate and the TASK-PR hygiene sweep: a PR whose head base -// branch died can never integrate. -func BaseBranchGone(repoPath string, base string, run RunGh) bool { +// BaseProbe is the three-valued verdict of a gh reachability probe. The TS +// booleans collapsed "gh answered not-found" and "gh never answered" into one +// value, so transport noise (network failure, 5xx, rate limit) read as a dead +// ref. The Go port keeps the unknown state explicit: callers act on a +// definitive verdict only, never on Unknown. +type BaseProbe string + +const ( + BaseAlive BaseProbe = "alive" // the ref resolved: gh answered + BaseGone BaseProbe = "gone" // gh answered not-found (merged+deleted or deleted) + BaseUnknown BaseProbe = "unknown" // gh never answered: transport noise +) + +// ghNotFoundRe matches gh's not-found signal: `gh api` reports an HTTP 404 as +// exit 1 with "Branch not found (HTTP 404)" / "Not Found (HTTP 404)" on +// stderr. The exit code cannot separate that from a network hiccup, so the +// probe classifies on the diagnostic text. +var ghNotFoundRe = regexp.MustCompile(`(?i)not found|http 404`) + +// ghNotFound reports whether a probe failure is a definitive gh 404 rather +// than transport noise. A *GhError is judged on its stderr alone — its message +// embeds the argv, whose ref names may themselves contain "404". +func ghNotFound(err error) bool { + if err == nil { + return false + } + var ge *GhError + if errors.As(err, &ge) { + return ghNotFoundRe.MatchString(ge.Stderr) + } + return ghNotFoundRe.MatchString(err.Error()) +} + +// ProbeBaseBranch is the three-valued successor of the TS boolean +// baseBranchGone: a base branch is gone when gh answers not-found +// (merged+deleted or deleted directly), alive when the ref resolves, and +// unknown when the lookup itself failed. Shared by the merge-queue gate and +// the TASK-PR hygiene sweep; a probe hiccup must never read as a dead base. +func ProbeBaseBranch(repoPath string, base string, run RunGh) BaseProbe { _, err := run([]string{"api", fmt.Sprintf("repos/{owner}/{repo}/branches/%s", base)}, repoPath) - return err != nil + switch { + case err == nil: + return BaseAlive + case ghNotFound(err): + return BaseGone + default: + return BaseUnknown + } } // ListOpenPrs mirrors the TS listOpenPrs. diff --git a/internal/orchestrator/autopr_test.go b/internal/orchestrator/autopr_test.go index fbb7a6ca..a72a7884 100644 --- a/internal/orchestrator/autopr_test.go +++ b/internal/orchestrator/autopr_test.go @@ -985,6 +985,57 @@ func TestAutoMergeSkipsBaseSuperseded(t *testing.T) { } } +func TestAutoMergeTreatsBaseProbeNoiseAsAlive(t *testing.T) { + // transport noise is not a dead base: a probe hiccup must not park a + // mergeable PR as base-superseded + run, calls := autoScriptedGh(map[string]ghResp{ + "view": {out: basePrView}, "diff": {}, "review": {}, "merge": {}, + "api": {err: &GhError{Stderr: "gh: Internal Server Error (HTTP 500)\n"}}, + }) + o := AutoReviewAndMergeOne("/repo", 9, AutoReviewAndMergeOneOpts{}, run) + if o.Action != ActionMerged { + t.Fatalf("expected merged, got %+v", o) + } + if autoCallCount(calls, "merge") == 0 { + t.Fatal("a probe hiccup must not park the merge") + } +} + +func TestGhNotFoundOnlyForRealGone(t *testing.T) { + // the three-valued probe's discriminator: only gh's own not-found answer + // is a dead ref — every other failure is transport noise + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"plain not-found error", fmt.Errorf("gh: Branch not found (HTTP 404)"), true}, + {"gh spawn timeout (goal's timeout case), no stderr", &GhError{Message: "gh api repos/o/r/branches/devagent/TASK-x exited -1: ", Code: -1}, false}, + {"gh branch not-found stderr", &GhError{Stderr: "gh: Branch not found (HTTP 404)\n"}, true}, + {"gh generic 404 stderr", &GhError{Stderr: "gh: Not Found (HTTP 404)\n"}, true}, + {"gh 5xx", &GhError{Stderr: "gh: Internal Server Error (HTTP 500)\n"}, false}, + {"gh network failure", &GhError{Stderr: "dial tcp: lookup api.github.com: no such host\n"}, false}, + {"gh rate limit", &GhError{Stderr: "gh: API rate limit exceeded for user\n"}, false}, + // The message embeds the argv, and a ref name may itself read like a + // 404 marker ("feature-404-fix"). These pin the INVARIANT that only + // gh's own wording in stderr can read as gone: the marker set stays + // narrow, so argv text never matches either alternative. They are not + // a reachable false-positive — git ref names cannot contain the space + // both alternatives require — so they fail only if the marker set is + // widened to a bare "404". + {"argv mentioning 404", &GhError{Message: "gh api repos/o/r/branches/feature-404-fix exited 1: connection refused", Stderr: "connection refused"}, false}, + {"argv mentioning not found", &GhError{Message: "gh api repos/o/r/branches/not-found-cleanup exited 128: timed out", Stderr: "timed out"}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := ghNotFound(tc.err); got != tc.want { + t.Fatalf("ghNotFound(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + func TestAutoMergeSkipsSupersededSibling(t *testing.T) { // skips a non-candidate head superseded by a same-base candidate conflicting := strings.Replace(basePrView, `"number":9`, `"number":7`, 1) diff --git a/internal/orchestrator/pr-hygiene.go b/internal/orchestrator/pr-hygiene.go index 957f275d..3177760d 100644 --- a/internal/orchestrator/pr-hygiene.go +++ b/internal/orchestrator/pr-hygiene.go @@ -23,6 +23,8 @@ import ( // - superseded/abandoned: issue closed, no landing commit → close as not shipped; // - genuinely-still-wanted: issue open → leave the PR open, comment evidence. // Only an actual landing commit may claim a merge; the close comments state it. +// Transport noise on the landing lookup is unknown evidence, not "no commit": +// the PR is left untouched for the next sweep, never closed on a failed listing. var issueRefRe = regexp.MustCompile(`#[0-9]+`) @@ -97,12 +99,15 @@ func prHygieneFlaggedInApply(repoPath string, pr int, reason string) bool { } // landingCommitOnMain returns the sha of the first commit on main whose -// message cites the issue as `(#N)` (the squash-merge convention), "" when -// none or the lookup fails. -func landingCommitOnMain(repoPath string, n int, run RunGh) string { +// message cites the issue as `(#N)` (the squash-merge convention). The second +// return says whether the main-branch listing actually answered: (sha, true) +// is a definitive verdict — sha "" then means no landing commit exists — +// while ("", false) is transport noise (the lookup or its parse failed), which +// is never evidence of "not shipped". +func landingCommitOnMain(repoPath string, n int, run RunGh) (string, bool) { r, err := run([]string{"api", "repos/{owner}/{repo}/commits?sha=main&per_page=100"}, repoPath) if err != nil { - return "" + return "", false } var commits []struct { SHA string `json:"sha"` @@ -111,14 +116,14 @@ func landingCommitOnMain(repoPath string, n int, run RunGh) string { } `json:"commit"` } if json.Unmarshal([]byte(r.Stdout), &commits) != nil { - return "" + return "", false } for _, c := range commits { if strings.Contains(c.Commit.Message, fmt.Sprintf("(#%d)", n)) { - return c.SHA + return c.SHA, true } } - return "" + return "", true } // Zombie-PR hygiene (PRD §17, surviving half of post-PR lifecycle automation): @@ -129,11 +134,14 @@ func landingCommitOnMain(repoPath string, n int, run RunGh) string { // - red-across-grace: CI stays red (completed failures, nothing pending) for // the full grace window — flag the PR and skip autoMerge until a green // check arrives. +// - age floor: no auto-close happens before the floor (closeAgeFloorHours, +// capped by the grace window) — a fresh PR is reported untouched instead, +// whatever the evidence says. // Every action writes one ledger row (pr, reason, grace-window age) so the // run ledger stays the replayable record of what automation did to PRs. // // The gh primitives (RunGh, DefaultRunGh, PrStatus, ListOpenPrs, -// BaseBranchGone, PostPrComment, AgeHours) are the autopr.ts port owned by +// ProbeBaseBranch, PostPrComment, AgeHours) are the autopr.ts port owned by // autopr.go — pr-hygiene.ts imports them the same way. var taskPrRe = regexp.MustCompile(`^devagent/TASK-`) @@ -154,13 +162,46 @@ func formatGraceHours(f float64) string { // to the autopr.go port of ageHours. var GraceAgeHours = AgeHours +// closeAgeFloorHours caps the age floor before ANY apply-mode auto-close: a PR +// younger than the floor is never closed by the sweep, whatever the evidence +// says (PR #346 was auto-closed 76 seconds after creation on a probe misread). +// The floor defends fresh work from a residual false-gone; the cap keeps a long +// operator-configured grace window from pushing zombie cleanup out by months. +const closeAgeFloorHours = 24.0 + +// closeUnderAgeFloor reports whether the PR is too young to auto-close for the +// configured grace window, whose own close arm stays authoritative when it is +// shorter. An unknown age counts as floored: no timestamp, no close. +func closeUnderAgeFloor(age *float64, graceHours float64) bool { + return age == nil || *age < min(graceHours, closeAgeFloorHours) +} + +// ageFloorDetail builds the untouched row for a floored close attempt, carrying +// the verdict that would have acted once the PR ages past the floor. +func ageFloorDetail(age *float64, graceHours float64, verdict string) string { + floor := formatGraceHours(min(graceHours, closeAgeFloorHours)) + if age == nil { + return fmt.Sprintf("%s, but the PR age is unknown: within the %sh auto-close floor, not closed this sweep", verdict, floor) + } + return fmt.Sprintf("%s, but the PR is %.0fh old: within the %sh auto-close floor, not closed this sweep", verdict, *age, floor) +} + +// ageLabel renders the grace-window age the sweep's details read it as. +func ageLabel(age *float64) string { + if age == nil { + return "unknown age" + } + return fmt.Sprintf("%dh since last update", int(*age)) +} + // PrHygieneOutcome mirrors the TS PrHygieneOutcome interface. type PrHygieneOutcome struct { PR int `json:"pr"` Title string `json:"title"` Action string `json:"action"` // Why the action fired: base-superseded | red-across-grace | - // not-a-task-pr | pending | green | red-within-grace. + // not-a-task-pr | pending | green | red-within-grace | base-unknown | + // landing-unknown | base-superseded-age-floor | landing-age-floor. Reason string `json:"reason"` // Hours since the PR's last update (grace-window age); null when unknown // or not applicable. TS JSON.stringify emits the key with null, so no @@ -171,8 +212,9 @@ type PrHygieneOutcome struct { // PrHygieneOptions mirrors the TS PrHygieneOptions interface. type PrHygieneOptions struct { - // Hours a PR may stay red before it is flagged for skip-autoMerge - // (config prHygiene.graceHours, default 24). + // Hours a PR may stay red before it is flagged for skip-autoMerge; also + // the auto-close age floor, capped by closeAgeFloorHours (config + // prHygiene.graceHours, default 24). GraceHours *float64 // Report without commenting or closing (config prHygiene.dryRun, // default true). @@ -260,8 +302,28 @@ func SweepTaskPrHygiene(repoPath string, opts PrHygieneOptions, run RunGh) PrHyg // Base-superseded first (condition 1): a PR whose head base branch // was merged or deleted can never integrate, whatever its CI says — - // waiting cannot fix a dead base. - if BaseBranchGone(repoPath, status.BaseRefName, run) { + // waiting cannot fix a dead base. Only a definitive gh 404 acts: the + // three-valued probe keeps transport noise from closing a live PR. + base := ProbeBaseBranch(repoPath, status.BaseRefName, run) + if base == BaseUnknown { + outcomes = append(outcomes, PrHygieneOutcome{ + PR: status.Number, Title: status.Title, Action: "untouched", + Reason: "base-unknown", + GraceAgeHours: age, + Detail: fmt.Sprintf("base %s lookup failed (transport); not acted on this sweep", status.BaseRefName), + }) + continue + } + if base == BaseGone { + if closeUnderAgeFloor(age, graceHours) { + outcomes = append(outcomes, PrHygieneOutcome{ + PR: status.Number, Title: status.Title, Action: "untouched", + Reason: "base-superseded-age-floor", + GraceAgeHours: age, + Detail: ageFloorDetail(age, graceHours, fmt.Sprintf("base %s was merged or deleted", status.BaseRefName)), + }) + continue + } detail := fmt.Sprintf("base %s was merged or deleted; branch can never integrate", status.BaseRefName) if dryRun { ledger.AppendPrHygieneRecord(repoPath, ledger.PrHygieneRecord{ @@ -311,7 +373,21 @@ func SweepTaskPrHygiene(repoPath string, opts PrHygieneOptions, run RunGh) PrHyg continue } var detail, comment, reason string - if sha := landingCommitOnMain(repoPath, n, run); sha != "" { + sha, known := landingCommitOnMain(repoPath, n, run) + if !known { + // Transport noise: the landing evidence is unknown, not + // absent — closing here would claim "not shipped" for work + // that may have landed. Leave the PR for the next sweep. + outcomes = append(outcomes, PrHygieneOutcome{ + PR: status.Number, Title: status.Title, Action: "untouched", + Reason: "landing-unknown", + GraceAgeHours: age, + Detail: fmt.Sprintf("issue #%d is closed but the main-branch commit listing failed (transport); not acted on this sweep", n), + }) + handled = true + break + } + if sha != "" { reason = "shipped-elsewhere" detail = fmt.Sprintf("issue #%d is closed and shipped elsewhere: landing commit %s on main", n, sha) comment = strings.Join([]string{ @@ -327,6 +403,16 @@ func SweepTaskPrHygiene(repoPath string, opts PrHygieneOptions, run RunGh) PrHyg "Reopen the issue and this PR if the work is still wanted.", }, "\n") } + if closeUnderAgeFloor(age, graceHours) { + outcomes = append(outcomes, PrHygieneOutcome{ + PR: status.Number, Title: status.Title, Action: "untouched", + Reason: "landing-age-floor", + GraceAgeHours: age, + Detail: ageFloorDetail(age, graceHours, detail), + }) + handled = true + break + } if dryRun { ledger.AppendPrHygieneRecord(repoPath, ledger.PrHygieneRecord{ TS: ledger.NowISO(), Kind: "event", Event: "pr-hygiene", @@ -371,10 +457,7 @@ func SweepTaskPrHygiene(repoPath string, opts PrHygieneOptions, run RunGh) PrHyg overdue := age == nil || *age >= graceHours if overdue { skipAutoMerge = true - ageStr := "unknown age" - if age != nil { - ageStr = fmt.Sprintf("%dh since last update", int(*age)) - } + ageStr := ageLabel(age) // Genuinely-still-wanted: the cited issues are open, so // the PR stays open — record the evidence on the PR. if !dryRun && len(cited) > 0 && !prHygieneFlaggedInApply(repoPath, status.Number, "red-across-grace") { diff --git a/internal/orchestrator/pr-hygiene_test.go b/internal/orchestrator/pr-hygiene_test.go index 647eab83..10fe339b 100644 --- a/internal/orchestrator/pr-hygiene_test.go +++ b/internal/orchestrator/pr-hygiene_test.go @@ -181,10 +181,14 @@ func TestGraceAgeHours(t *testing.T) { } func TestSweepTaskPrHygiene(t *testing.T) { + // Past the auto-close age floor (closeAgeFloorHours) so these fixtures + // still exercise the close arms; the floor itself is pinned by the + // fresh-PR subtests below. + aged := time.Now().Add(-30 * time.Hour).UTC().Format("2006-01-02T15:04:05.000Z07:00") t.Run("closes a base-superseded TASK PR (base branch deleted) and writes a ledger row", func(t *testing.T) { repo := hygieneTempRepo(t) run, calls := scriptedGhWithDeadBases([]string{"devagent/TASK-mtioq4ik-T0-a0"}, map[string]any{ - "list": "[" + hygieneTaskPr(t, nil) + "]", + "list": "[" + hygieneTaskPr(t, map[string]any{"updatedAt": aged}) + "]", "comment": "", "close": "", }) @@ -239,10 +243,34 @@ func TestSweepTaskPrHygiene(t *testing.T) { } }) + t.Run("a spawn timeout on the base probe leaves the PR untouched (never closes on a hiccup)", func(t *testing.T) { + repo := hygieneTempRepo(t) + run, calls := scriptedGh(map[string]any{ + "list": "[" + hygieneTaskPr(t, nil) + "]", + "api": &GhError{Message: "gh api repos/{owner}/{repo}/branches/devagent/TASK-mtioq4ik-T0-a0 exited -1: ", Code: -1}, // 30s spawn timeout: no stderr + }) + dry := false + res := SweepTaskPrHygiene(repo, PrHygieneOptions{DryRun: &dry}, run) + if res.SkipAutoMerge { + t.Fatal("skipAutoMerge must be false") + } + if len(res.Outcomes) != 1 || res.Outcomes[0].Action != "untouched" || res.Outcomes[0].Reason != "base-unknown" { + t.Fatalf("outcomes = %+v", res.Outcomes) + } + for _, c := range *calls { + if len(c) > 1 && (c[1] == "close" || c[1] == "comment") { + t.Fatalf("transport noise must not act: %v", c) + } + } + if rows := readLedgerRows(t, repo); len(rows) != 0 { + t.Fatalf("expected no ledger rows, got %d", len(rows)) + } + }) + t.Run("flags base-superseded in dry-run: no comment, no close, still one ledger row", func(t *testing.T) { repo := hygieneTempRepo(t) run, calls := scriptedGhWithDeadBases([]string{"devagent/TASK-mtioq4ik-T0-a0"}, map[string]any{ - "list": "[" + hygieneTaskPr(t, nil) + "]", + "list": "[" + hygieneTaskPr(t, map[string]any{"updatedAt": aged}) + "]", }) res := SweepTaskPrHygiene(repo, PrHygieneOptions{}, run) if res.SkipAutoMerge { @@ -477,7 +505,7 @@ func TestSweepTaskPrHygiene(t *testing.T) { t.Run("closes a red-within-grace PR whose base is gone (dead base cannot be fixed by waiting)", func(t *testing.T) { repo := hygieneTempRepo(t) run, calls := scriptedGhWithDeadBases([]string{"devagent/TASK-mtioq4ik-T0-a0"}, map[string]any{ - "list": "[" + hygieneTaskPr(t, map[string]any{"statusCheckRollup": hygieneRed}) + "]", + "list": "[" + hygieneTaskPr(t, map[string]any{"statusCheckRollup": hygieneRed, "updatedAt": aged}) + "]", "comment": "", "close": "", }) @@ -512,7 +540,7 @@ func TestSweepTaskPrHygiene(t *testing.T) { repo := hygieneTempRepo(t) stale := time.Now().Add(-48 * time.Hour).UTC().Format("2006-01-02T15:04:05.000Z07:00") listJSON := "[" + - hygieneTaskPr(t, map[string]any{"number": 11}) + "," + // base-superseded -> flagged (dry-run) + hygieneTaskPr(t, map[string]any{"number": 11, "updatedAt": aged}) + "," + // base-superseded -> flagged (dry-run), past the age floor hygieneTaskPr(t, map[string]any{"number": 12, "baseRefName": "main", "statusCheckRollup": hygieneRed, "updatedAt": stale}) + "," + // red across grace -> flagged hygieneTaskPr(t, map[string]any{"number": 13, "baseRefName": "main"}) + // green -> untouched "]" @@ -558,6 +586,58 @@ func TestSweepTaskPrHygiene(t *testing.T) { t.Fatalf("rows = %v", rows) } }) + + t.Run("a fresh PR is never auto-closed before the age floor (base branch deleted)", func(t *testing.T) { + repo := hygieneTempRepo(t) + run, calls := scriptedGhWithDeadBases([]string{"devagent/TASK-mtioq4ik-T0-a0"}, map[string]any{ + "list": "[" + hygieneTaskPr(t, nil) + "]", // updatedAt: now + "comment": "", + "close": "", + }) + dry := false + res := SweepTaskPrHygiene(repo, PrHygieneOptions{DryRun: &dry}, run) + if len(res.Outcomes) != 1 { + t.Fatalf("outcomes = %+v", res.Outcomes) + } + o := res.Outcomes[0] + if o.Action != "untouched" || o.Reason != "base-superseded-age-floor" { + t.Fatalf("outcome = %s/%s, want untouched/base-superseded-age-floor", o.Action, o.Reason) + } + if !strings.Contains(o.Detail, "auto-close floor") || !strings.Contains(o.Detail, "merged or deleted") { + t.Fatalf("detail must keep the verdict and name the floor: %q", o.Detail) + } + for _, c := range *calls { + if len(c) > 1 && (c[1] == "close" || c[1] == "comment") { + t.Fatalf("fresh PR must not be acted on: %v", c) + } + } + if rows := readLedgerRows(t, repo); len(rows) != 0 { + t.Fatalf("expected no ledger rows, got %d", len(rows)) + } + }) + + t.Run("an unknown PR age is floored: no close without a timestamp", func(t *testing.T) { + repo := hygieneTempRepo(t) + run, calls := scriptedGhWithDeadBases([]string{"devagent/TASK-mtioq4ik-T0-a0"}, map[string]any{ + "list": "[" + hygieneTaskPr(t, map[string]any{"updatedAt": "not-a-date"}) + "]", + "comment": "", + "close": "", + }) + dry := false + res := SweepTaskPrHygiene(repo, PrHygieneOptions{DryRun: &dry}, run) + o := res.Outcomes[0] + if o.Action != "untouched" || o.Reason != "base-superseded-age-floor" || o.GraceAgeHours != nil { + t.Fatalf("outcome = %+v", o) + } + if !strings.Contains(o.Detail, "age is unknown") { + t.Fatalf("detail = %q", o.Detail) + } + for _, c := range *calls { + if len(c) > 1 && (c[1] == "close" || c[1] == "comment") { + t.Fatalf("unknown age must not be acted on: %v", c) + } + } + }) } // landingScriptGh drives the landing-evidence triage: a PR list, issue @@ -626,7 +706,7 @@ func TestSweepTaskPrHygieneLandingEvidence(t *testing.T) { t.Run("shipped-elsewhere: closed issue with a landing commit on main closes the PR citing the sha", func(t *testing.T) { repo := hygieneTempRepo(t) - run, calls := landingScriptGh(t, prJSON("Closes #7", hygieneGreen, now), + run, calls := landingScriptGh(t, prJSON("Closes #7", hygieneGreen, old), map[int]string{7: "CLOSED"}, mainWithLanding) res := SweepTaskPrHygiene(repo, PrHygieneOptions{DryRun: &off, GraceHours: &grace}, run) if len(res.Outcomes) != 1 { @@ -659,7 +739,7 @@ func TestSweepTaskPrHygieneLandingEvidence(t *testing.T) { t.Run("superseded: closed issue without a landing commit closes the PR as not shipped", func(t *testing.T) { repo := hygieneTempRepo(t) - run, calls := landingScriptGh(t, prJSON("Closes #7", hygieneGreen, now), + run, calls := landingScriptGh(t, prJSON("Closes #7", hygieneGreen, old), map[int]string{7: "CLOSED"}, mainEmpty) res := SweepTaskPrHygiene(repo, PrHygieneOptions{DryRun: &off, GraceHours: &grace}, run) if len(res.Outcomes) != 1 { @@ -687,6 +767,63 @@ func TestSweepTaskPrHygieneLandingEvidence(t *testing.T) { } }) + t.Run("transport noise on the landing listing never closes the PR as not shipped", func(t *testing.T) { + repo := hygieneTempRepo(t) + calls := &[][]string{} + run := RunGh(func(args []string, cwd string) (*GhResult, error) { + *calls = append(*calls, args) + switch { + case args[0] == "pr" && len(args) > 1 && args[1] == "list": + return &GhResult{Stdout: prJSON("Closes #7", hygieneGreen, now)}, nil + case args[0] == "issue": + return &GhResult{Stdout: `{"state":"CLOSED"}`}, nil + case strings.Contains(strings.Join(args, " "), "commits?sha=main"): + return nil, fmt.Errorf("gh api repos/{owner}/{repo}/commits exited 1: gh: Internal Server Error (HTTP 500)") + case args[0] == "api": + return &GhResult{}, nil // the branch lookup answered: base intact + } + return &GhResult{}, nil + }) + res := SweepTaskPrHygiene(repo, PrHygieneOptions{DryRun: &off, GraceHours: &grace}, run) + if len(res.Outcomes) != 1 { + t.Fatalf("outcomes = %+v", res.Outcomes) + } + o := res.Outcomes[0] + if o.Action != "untouched" || o.Reason != "landing-unknown" { + t.Fatalf("outcome = %s/%s, want untouched/landing-unknown", o.Action, o.Reason) + } + for _, c := range *calls { + if c[0] == "pr" && (c[1] == "comment" || c[1] == "close") { + t.Fatalf("transport noise must not act: %v", c) + } + } + if rows := readLedgerRows(t, repo); len(rows) != 0 { + t.Fatalf("expected no ledger rows, got %d", len(rows)) + } + }) + + t.Run("an unparseable landing listing is unknown evidence, not an empty main", func(t *testing.T) { + repo := hygieneTempRepo(t) + run, calls := scriptedGh(map[string]any{ + "list": prJSON("Closes #7", hygieneGreen, now), + "api": "{not json", + "issue": `{"state":"CLOSED"}`, + }) + res := SweepTaskPrHygiene(repo, PrHygieneOptions{DryRun: &off, GraceHours: &grace}, run) + if len(res.Outcomes) != 1 { + t.Fatalf("outcomes = %+v", res.Outcomes) + } + o := res.Outcomes[0] + if o.Action != "untouched" || o.Reason != "landing-unknown" { + t.Fatalf("outcome = %s/%s, want untouched/landing-unknown", o.Action, o.Reason) + } + for _, c := range *calls { + if c[0] == "pr" && (c[1] == "comment" || c[1] == "close") { + t.Fatalf("unparseable evidence must not act: %v", c) + } + } + }) + t.Run("still-wanted: open issue keeps a red-across-grace PR open with an evidence comment", func(t *testing.T) { repo := hygieneTempRepo(t) run, calls := landingScriptGh(t, prJSON("Closes #7", hygieneRed, old), @@ -723,7 +860,7 @@ func TestSweepTaskPrHygieneLandingEvidence(t *testing.T) { t.Run("dry-run: triage flags without commenting or closing", func(t *testing.T) { repo := hygieneTempRepo(t) - run, calls := landingScriptGh(t, prJSON("Closes #7", hygieneGreen, now), + run, calls := landingScriptGh(t, prJSON("Closes #7", hygieneGreen, old), map[int]string{7: "CLOSED"}, mainWithLanding) res := SweepTaskPrHygiene(repo, PrHygieneOptions{GraceHours: &grace}, run) if len(res.Outcomes) != 1 || res.Outcomes[0].Action != "flagged" { @@ -759,7 +896,7 @@ func TestSweepTaskPrHygieneLandingEvidence(t *testing.T) { t.Run("closing-keyword refs win over incidental #N mentions", func(t *testing.T) { repo := hygieneTempRepo(t) - run, calls := landingScriptGh(t, prJSON("Related to #12, see also #13. Closes #7", hygieneGreen, now), + run, calls := landingScriptGh(t, prJSON("Related to #12, see also #13. Closes #7", hygieneGreen, old), map[int]string{7: "CLOSED"}, mainWithLanding) res := SweepTaskPrHygiene(repo, PrHygieneOptions{DryRun: &off, GraceHours: &grace}, run) if len(res.Outcomes) != 1 || res.Outcomes[0].Action != "closed" || res.Outcomes[0].Reason != "shipped-elsewhere" { @@ -771,4 +908,29 @@ func TestSweepTaskPrHygieneLandingEvidence(t *testing.T) { } } }) + + t.Run("a fresh PR is never auto-closed before the age floor (issue shipped elsewhere)", func(t *testing.T) { + repo := hygieneTempRepo(t) + run, calls := landingScriptGh(t, prJSON("Closes #7", hygieneGreen, now), // fresh + map[int]string{7: "CLOSED"}, mainWithLanding) + res := SweepTaskPrHygiene(repo, PrHygieneOptions{DryRun: &off, GraceHours: &grace}, run) + if len(res.Outcomes) != 1 { + t.Fatalf("outcomes = %+v", res.Outcomes) + } + o := res.Outcomes[0] + if o.Action != "untouched" || o.Reason != "landing-age-floor" { + t.Fatalf("outcome = %s/%s, want untouched/landing-age-floor", o.Action, o.Reason) + } + if !strings.Contains(o.Detail, "f00dc0de") || !strings.Contains(o.Detail, "auto-close floor") { + t.Fatalf("detail must keep the landing sha and name the floor: %q", o.Detail) + } + for _, c := range *calls { + if c[0] == "pr" && (c[1] == "comment" || c[1] == "close") { + t.Fatalf("fresh PR must not be acted on: %v", c) + } + } + if rows := readLedgerRows(t, repo); len(rows) != 0 { + t.Fatalf("expected no ledger rows, got %d", len(rows)) + } + }) }