gate: close the inbox by supersession and mootness

[itsHabib]

gate next listed 164 rows against 3 open pull requests. Every claim below was measured against the live ledger (~/dev/gate/state/log.jsonl, 5,082 artifacts), read-only.

The diagnosis differs from the brief, and it moves the fix

The 164 is 14 parked + 150 ready-to-merge, not 164 parks.

The brief's root cause — "an escalation is never discharged except by a judgment against its own run, so earlier runs' escalations stay parked forever" — is not what was happening. parkedRuns already folded runs by subject and kept the newest terminal, and it was doing real work: of the log's 366 escalations, 245 were discharged by judgment and 107 by supersession, leaving 14. Supersession was never the leak.

The leak is a PR ending. Every action gate writes is dry_run / would_merge — gate authorizes, an executor acts — so once the operator ran the emitted merge command and the PR landed, nothing in the log ever said so. The row stood forever. That is 150 of the 164, and it lives on the surface the brief did not name.

Two facts that shaped the design:

• gate next -live already fixes the display (164 → 3, in 2.0s) and has since it shipped. It throws away what it learns on every invocation. The operator was reading the default, offline projection.
• The default cannot simply become live. escalate serve shells gate next -json for its grant lookup inside a hard budget on the Slack ack path; one gh subprocess per distinct repo there trades a stale queue for a stranded interaction — the exact failure gate: bind decisions to a decider, record what landed, and measure the bypass #249 is fixing one commit over.

So: derive closure from state, and give the live read somewhere to persist what it already knows.

1. One subject-scoped reduction (observe/closure.go)

foldSubjectTerminals + a closure index, built once, consumed by the parked projection, the ready projection, sweep's work list, and the audit metric.

This is the extraction FOLLOWUPS' "Still open (1)" named as the durable fix for gate's three independently-derived "is this park still open" notions. TestParkDischargeAgreesWithTheInbox pins that two of them cannot report different live counts. It does not close that entry — cmdResolve's pre-check and the locked requireOpenEscalation check are write-path, still run-scoped, and pointing them at the shared reduction is a decision-path change with a wider blast radius than an observe-only PR should carry. FOLLOWUPS records exactly that split.

2. Rows are classified, never dropped

class	meaning
superseded	a newer terminal for the same repo#PR displaced it
moot	the pull request itself is no longer open
stale	still open, but the head moved past the authorized SHA — needs re-gating

stale is deliberately not folded into moot. The live reconcile already dropped both, silently and identically; they are not the same fact, and reporting owed work as finished is the failure this PR exists to fix, inverted. Both were previously invisible: reconcileReadyLive dropped 161 rows and said nothing.

Counts always project (discharged in JSON, printed by the text renderer); -all shows the rows with their reasons. A discharged park carries no judge/resolve command — the judgment it would spend is one-shot, and spending it on a settled question is the concrete harm.

3. gate sweep records mootness

The same batched open-PR read next -live and preflight already share — gate grows no second GitHub client — persisted as a subject_closed artifact parented to the terminal its row stands on, so the store's absent-parent guard makes "one closure per terminal" structural and a re-run a genuine no-op.

It records only what that read proves: not_open. Which commit landed, when, and by whom is receipt/reconcile's claim, read back from the platform with its own clock and actor — a sweep asserting it would fabricate exactly what a receipt exists to check. TestSweepRecordsOnlyWhatItObserved fails if the body ever grows merge_commit, actor, or merged_at.

An unread repo is UNKNOWN, never closed. Assuming closure on a failed read would delete the operator's queue on a network blip.

It is a separate verb, not a flag on next: it writes, and Observability views are read-only and storeless. A next that sometimes wrote would put a store mutation behind a display flag on the Slack path.

4. gate audit reports the ratio

After the chain check, never touching the exit code — integrity and health are different claims, and a metric that could fail an audit trains the reader to ignore audit failures.

chain intact
park discharge (366): 245 by judgment, 107 by supersession, 0 moot, 14 awaiting
  supersession share 29% — parks a later run overtook before anyone answered them

Judgment takes precedence over supersession: a park that was answered is answered regardless of what happened to the PR afterwards.

Coordination with #249

#249 is still open, so this consumes its kinds by string literal (kindReceipt, kindCoverage) and compiles without it. TestClosureReadsReceiptAndCoverage constructs #249's exact body shapes — receipt outcomes merged/superseded/abandoned close, failed does not (the PR is still open); coverage's landed classes close, authorized_never_landed does not (it lists authorizations, not merges). If either body drifts before #249 lands, that test fails loudly instead of silently emptying the moot class.

Rebase task on merge: swap the two constants for state.Kind*. Recorded in FOLLOWUPS, along with the note that coverage's basis is merged-pull-requests and says nothing about a PR closed without merging — a real part of the ghost population, and why sweep's not_open predicate is not redundant with reconcile.

Verification

Read-only against the live ledger; no writes to ~/dev/gate/state.

sweep -dry-run: swept 164 live row(s); would record 161 closure(s)
next -live:     discharged (287) — parked 119 (11 moot, 108 superseded)
                                   ready  168 (150 moot, 18 superseded)
                → 3 live rows, matching -live exactly

The end-to-end write path is pinned in Go against a temp store instead: sweep closes the queue, is idempotent, leaves an unread repo alone, -dry-run writes nothing, and an open PR is untouched.

gofmt, go vet, golangci-lint (0 issues), go test ./..., and go test -race ./cmd/gate/... all clean. Guide pair byte-identical.

Notes for review

• JSON is additive. discharged is new and always present; discharge/discharge_why are omitempty on rows. The console forwards gate next -json -live verbatim and parses no fields, and the -live path's row membership is unchanged.
• NextText/NextJSON took a NextRequest. The offline/live and default/all axes multiply into four entry points and then eight; this follows preflight's existing request-struct shape and collapses 4 → 2. Fetch == nil is what makes offline the default rather than a bool nobody can read at a call site.
• subject_closed is provenance, not an outcome. Like grant_needed and resolution it sits outside the action/escalation families: countingSubject guards on kind before anything else, so it burns no review cycle, never re-parks a run, and authorizes nothing. Its only effect is on what the inbox shows.
• Not done deliberately: the write-path checks are untouched (above), and sweep is not wired into any driver or schedule — when it runs is the operator's call.

🤖 Generated with Claude Code

[itsHabib]

@claude please review.

Focus, given this is T3 (gate machinery — the reduction decides which merge-authorization parks the operator is shown, and sweep is a new writer into the hash-chained ledger):

1. Can a closing fact hide a LIVE park? observe/closure.go marks a subject moot from four sources. The dangerous direction is a false positive — suppressing a park that still needs judgment. Check the receipt failed case and coverage's authorized_never_landed, both of which I deliberately excluded, and whether any other body shape can be misread as closed.
2. Subject-key collisions. The closure index is keyed repo#number with last-writer-wins. Can an artifact with a partially-decoded body (empty repo, zero number, or a repo from one run and a number from another via mergeRunFacts) pollute another subject's key?
3. sweep idempotency and the absent-parent guard. The closure is parented to the terminal artifact. Is one-closure-per-terminal actually structural, and does a PR that is re-gated after being closed behave correctly?
4. The supersession/moot precedence in ParkDischargeReport. Judgment is checked first. Is any park double-counted or dropped from the total?
5. NextRequest refactor. Fetch == nil now means offline. Any call path that previously got a live read and now silently gets an offline one? (console shells next -json -live explicitly; escalate serve shells next -json.)

[itsHabib]

@codex review

[claude]

Claude finished @itsHabib's task in 6m 38s —— View job

---

Review — gate: close the inbox by supersession and mootness

• Load tools and gather context
• Read observe/closure.go — false-positive suppression logic
• Read observe/discharge.go + ParkDischargeReport — supersession/moot precedence
• Read observe/inbox.go — NextRequest refactor, offline vs live paths
• Read sweep.go — idempotency and absent-parent guard
• Read state/state.go — new artifact kinds
• Read test files for coverage of the 5 focus areas
• Analyze focus area 1: Can a closing fact hide a LIVE park?
• Analyze focus area 2: Subject-key collisions
• Analyze focus area 3: sweep idempotency and absent-parent guard
• Analyze focus area 4: Supersession/moot precedence in ParkDischargeReport
• Analyze focus area 5: NextRequest refactor — offline vs live read paths

---

Focus Area 1 — Can a closing fact hide a LIVE park?

The explicitly excluded cases are correct. failed receipts are excluded from receiptClosings with a comment explaining the inversion. authorized_never_landed is excluded from absorbCoverage — only the three landed lists are read. TestClosureReadsReceiptAndCoverage pins both shapes exactly, and if #249 drifts either body, the test fails loud.

absorbAlreadyMerged is conservative. It reads the subject from facts[a.Run] (the folded run facts, not the action body) and guards f.Repo == "" || f.Number == 0 before writing to the index. An already-merged refusal with no attributable subject teaches the index nothing — it can't close what it can't name.

One genuine false-positive path: re-opened PR after sweep. This is the medium finding in this review.

After gate sweep writes a subject_closed artifact for, say, o/widget#7 (parented to terminal esc_1), the closure index permanently records o/widget#7 as finished:

// absorbSubjectClosed — no guard on which terminal the artifact is parented to
func (idx closureIndex) absorbSubjectClosed(a state.Artifact) {
    ...
    idx[subjectKey(b.Repo, b.Number)] = closingFact{...}  // keyed on subject, not on terminal
}

If the PR is then re-opened, a new gate gate run produces a new terminal (esc_2). In the offline projection, parkedSubject does:

fact, finished := closed.lookup(row.Repo, row.Number)  // finds the stale sbc_1 entry
if finished {
    return live, append(discharged, dischargeParked(row, DischargeMoot, fact.why()))
}

The new live park (esc_2) is classified MOOT in the offline projection — hidden from the operator — because buildClosureIndex treats subject_closed as an append-only, terminal-agnostic closure. The live path (gate next -live) correctly overrides this via reconcileLive. But escalate serve uses gate next -json (no -live), so the re-gated park is invisible to the Slack resolution path.

The comment in state.go on KindSubjectClosed says "a PR that is gated again later gets a fresh terminal that a later sweep can close on its own merits" — this addresses sweep idempotency (the sweep won't write a second sbc for a PR that's open), but it doesn't address the offline index false-positive between re-open and the next sweep. No entry in FOLLOWUPS.md names this gap.

A terminal-scoped fix: in absorbSubjectClosed, check that the artifact's parent is still the newest terminal for that subject — if it isn't, the PR was re-gated after the closure was written and the offline index should not suppress the newer park. buildClosureIndex already receives facts and could receive terms subjectTerminals too (the call site in buildInbox has terms in scope).

Fix this →

absorbSubjectClosed has no guard on b.State. Any KindSubjectClosed artifact with valid repo/number closes the subject — the actual state value is used only for rendering, not for classification. Since sweep is the only writer and always writes ClosedNotOpen, this is low risk today, but future writers should know the kind semantically implies closure regardless of what the state field says.

---

Focus Area 2 — Subject-key collisions

All four absorb paths guard correctly:

Path	Guard
absorbAlreadyMerged	f.Repo == "" || f.Number == 0 (run facts, not action body)
absorbSubjectClosed	b.Repo == "" || b.Number == 0
absorbReceipt	!ok || b.Repo == "" || b.Number == 0
absorbCoverage / absorbCoverageRows	outer b.Repo == "" + inner r.Number == 0

No degenerate key "#0" or "" can be written to the index.

mergeRunFacts cross-contamination: absorbAlreadyMerged resolves the subject from facts[a.Run] — the accumulated facts for the run the action belongs to. Since each run ID is unique to one gate gate -repo R -pr N invocation, all artifacts in a run target the same PR. A run cannot span multiple subjects, so mergeRunFacts's last-non-zero-wins merge cannot produce a {Repo: "a", Number: 5} for a run that targeted b#7. No collision risk.

---

Focus Area 3 — sweep idempotency and the absent-parent guard

Structural idempotency is correct. recordClosed calls AppendIfAbsentParent(KindSubjectClosed, s.Run, s.Terminal, ...), which evaluates under the store lock:

// checks: a.Run == s.Run AND a.Kind == KindSubjectClosed AND s.Terminal ∈ a.Parents
for _, a := range audit.All {
    if a.Run != run || !contains(uniqueKinds, a.Kind) || !hasParent(a.Parents, uniqueParent) {
        continue
    }
    ...
}

Two concurrent sweeps cannot both write a closure for the same (run, terminal). A re-run produces ErrAlreadyExists, which recordClosed converts to Already: true. TestSweepIsIdempotent pins this — the second sweep writes zero new artifacts. ✅

Re-gate after closure: as noted in Focus Area 1, the sweep correctly avoids writing a second subject_closed for a re-opened PR (the PR is open, so it's not in the work list). But the existing subject_closed from before re-open continues to poison the offline closure index. The absent-parent guard is sound for its stated purpose (idempotency per terminal); the gap is in what the closure index does with that artifact once a newer terminal exists.

---

Focus Area 4 — Supersession/moot precedence in ParkDischargeReport

The precedence chain in classify is: Judgment → Unattributed → Supersession → Moot → Live. Each path returns immediately, so every escalation lands in exactly one bucket. Total is incremented before classify, and ByJudgment + Unattributed + BySupersession + Moot + Live == Total by construction. No double-counting, no dropped parks.

The precedence is correct:

• A park that was judged and then superseded → ByJudgment. This is intentional: the loop worked (someone answered), and crediting it to supersession would understate health. ✅
• A park that was judged and the PR is now moot → ByJudgment. Same reason. ✅
• A park that was never judged, superseded by a later terminal → BySupersession. The terms.newest[key].artifact.ID != a.ID check is the correct proxy for "a later run displaced this one." ✅
• A park that is the current terminal for its subject, and the PR is finished → Moot. ✅

TestParkDischargeClassifiesEveryPark constructs all five classes including the judged-then-superseded case and pins the counts. TestParkDischargeAgreesWithTheInbox pins that rep.Live == len(in.Parked) — the shared-reduction invariant that motivated the extraction. Both tests are load-bearing.

One thing to confirm with #249: receipt: "superseded" maps to ClosedMerged in receiptClosings. If "superseded" in receipt vocabulary means "the merge action was superseded by a later action" (not that the PR itself merged), the PR might still be open and this mapping would be a false positive. The test asserts it's treated as a closing fact, but whether that's semantically correct depends on #249's definition. Worth a comment confirming the intent once that PR lands.

---

Focus Area 5 — NextRequest refactor, offline vs live paths

The refactor is clean. Before: four entry points (NextText, NextJSON, NextTextLive, NextJSONLive). After: two (NextText, NextJSON) with req.Fetch == nil meaning offline.

The call paths in cmdNext are correct:

req := observe.NextRequest{StateArg: ..., IncludeDischarged: *all}
if *live {
    req.Fetch = lookupOpenPRs   // live only when -live is passed
}

• gate next -json (what escalate serve shells) → req.Fetch = nil → offline ✅
• gate next -json -live (what console shells) → req.Fetch = lookupOpenPRs → live ✅
• gate next -all → IncludeDischarged = true, offline ✅

Both console and escalate shell the gate binary; neither imports the observe package directly. No Go call site silently demoted from live to offline. The comment in NextRequest.Fetch explaining why the default is offline (the escalate serve Slack budget constraint) is exactly the right documentation — it names the specific failure mode that gate sweep exists to avoid.

---

Summary

#	Area	Verdict
1a	failed receipt excluded	✅ correct
1b	authorized_never_landed excluded	✅ correct
1c	Re-opened PR after sweep → offline false-MOOT	⚠️ medium — undocumented gap
1d	absorbSubjectClosed ignores b.State	ℹ️ low — sweep is the only writer
2	Subject-key collisions	✅ all paths guarded
3a	Sweep idempotency (absent-parent guard)	✅ structural
3b	Re-gate after closure in sweep	follows from 1c
4	ParkDischargeReport precedence	✅ no double-count, no drops
5	NextRequest refactor	✅ no silent offline demotion

The core logic is sound and the test suite is thorough — TestParkDischargeAgreesWithTheInbox, TestSweepRecordsOnlyWhatItObserved, and TestClosureReadsReceiptAndCoverage are exactly the right invariant tests to pin. The one functional gap is the offline projection's permanent false-MOOT for a re-opened PR, which the live path handles correctly but escalate serve (offline path) does not see. Whether this is worth fixing before merge depends on how often PRs are re-opened in practice; FOLLOWUPS would be the right place to record it if deferring.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0160ac5a3e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[itsHabib]

Codex's P1 was found independently in self-review and is already fixed — 7e3e128, pushed before the review posted. Same defect, same reasoning.

The fix. closingFact now carries its position in the log, and closureIndex.settles requires the fact to postdate the terminal it is asked to settle. A closure recorded at order 100 no longer settles a park appended at order 150, so a reopened PR's fresh park stays live. Subject-wide last-writer-wins was the wrong shape; the entry is still subject-keyed but is no longer unconditional.

Codex named a consequence worth pinning on its own — "future sweeps cannot repair it because LiveSubjects also excludes it" — which is right and is the part that made this unrecoverable rather than merely wrong. Added in b4dcbaa: the reopened subject must be back on the sweep work list, not just back on screen.

Pinned in both directions, since a fix that just disabled the moot class would also pass the first half:

• park after a closure stays live; re-authorization after a closure stays ready; the audit metric counts it live; the subject is sweepable again;
• TestClosureStillSettlesAnOlderTerminal — a closure that legitimately postdates its terminal still moots it.

Also in 7e3e128: sweep now shares observe's tested parallel fan-out (OpenSets) instead of a second sequential copy of the same read. 19 repos in 1.9s. Live ledger unchanged: 161 of 164 closed, leaving the 3 genuinely open.

gofmt, go vet, golangci-lint (0 issues), go test ./..., go test -race ./cmd/gate/... all clean.

[itsHabib]

Panel folded. All three reviewers — codex, claude, and my own adversarial pass — independently found the same P1 (the re-opened PR), which is a good sign about the finding and about the panel.

1c — re-opened PR false-MOOT: fixed (7e3e128, pushed before either review posted)

The fix is ordering-based, not parent-based, which is deliberately more general than the terminal-parent check the review suggested. closingFact carries its position in the log and settles requires it to postdate the terminal it is asked to settle.

Parent-scoping would only have covered subject_closed, the one closing kind that has a terminal parent. A coverage artifact from #249 is parented to nothing subject-shaped and lists many PRs at once, and a receipt is parented to the action, not to whatever terminal is newest now. Ordering covers all four sources with one rule.

The review's note that the live path masks this offline bug is right and is the reason it mattered: console shells next -json -live and would have looked fine, while escalate serve shells next -json and would not. Codex named the other half — the row is also unsweepable, since the sweep's work list is the live rows — which is what made it unrecoverable rather than merely wrong. Pinned separately in 388b7b3.

Pinned in both directions, since a fix that simply disabled the moot class would pass the first half: park-after-closure stays live, re-authorization-after-closure stays ready, the audit metric counts it live, the subject is sweepable again — and TestClosureStillSettlesAnOlderTerminal asserts a closure that legitimately postdates its terminal still moots it.

1d — absorbSubjectClosed ignored b.State: fixed (3c546bc)

Agreed, and it is the same failure direction. Now whitelisted against the three states that mean finished; an unrecognised one leaves the row visible. A stale row is recoverable by looking at the screen, a hidden park is not.

receipt: "superseded" — confirmed correct, now documented

Verified against origin/claude/gate-decision-provenance:

OutcomeSuperseded: the PR merged, but at a DIFFERENT head than the one authorized. The merge happened; this authorization did not cover it.

It is a fact about the PR, not about the authorization — which makes it a bad discharge and a perfectly good closing fact. Read as "the authorization was superseded" the mapping looks wrong, so that reasoning is now in the code rather than left to be re-derived.

---

gofmt, go vet, golangci-lint (0 issues), go test ./..., go test -race ./cmd/gate/... all clean. Live ledger unchanged: 161 of 164 discharged, leaving the 3 genuinely open.

That is fix-round 1 of 2. @claude @codex please re-review.

[claude]

Claude finished @itsHabib's task in 3m 27s —— View job

---

Re-review — gate: close the inbox by supersession and mootness (fix-round 1)

• Load tools and gather context
• Read closure.go — ordering-based settles fix (1c)
• Read closure_test.go — pin in both directions
• Read sweep.go — sweepability fix (1c/388b7b3)
• Read state.go — b.State whitelist fix (1d/3c546bc)
• Verify receipt: "superseded" documentation
• Analyze: is the ordering-based fix sound across all four closing sources?
• Analyze: are any new issues introduced?
• Post review

---

Fix 1c — Ordering-based settles (7e3e128)

The fix is sound, and more general than the parent-scoping approach the review suggested. closingFact carries the artifact's position in the arts slice as order, and settles requires f.order > terminalOrder strictly — a closing fact only settles a terminal it postdates.

The crucial invariant: both foldSubjectTerminals and buildClosureIndex range over the same arts slice with the same for order, a := range arts pattern. Every caller (buildInbox, ParkDischargeReport, LiveSubjects) passes the same slice to both functions, so the order values are directly comparable across the two indexes. ✅

The fix covers all four closing sources with one rule:

• subject_closed (parent-scoped to a terminal) — parent scoping would have worked here too, but ordering generalises away the dependency on that structural property
• receipt (parented to the action, not to whatever terminal is newest now) — parent scoping would NOT have covered this
• coverage (parented to nothing subject-shaped; lists many PRs at once) — parent scoping would NOT have covered this
• already_merged action (no closing artifact at all) — parent scoping is inapplicable

Three call sites, all correct:

• inbox.go:603 ready-row path: closed.settles(row.Repo, row.Number, terms.newest[key].order) ✅
• inbox.go:725 parked-run path: closed.settles(row.Repo, row.Number, newest.order) ✅
• discharge.go:77 audit metric: closed.settles(f.Repo, f.Number, newest.order) ✅

Boundary condition: f.order <= terminalOrder means a closing fact at the same position as the terminal (which cannot actually happen — each artifact gets a unique slice index) would not settle it. The safe direction. ✅

The last-writer-wins + ordering interaction is correct across all multi-closure scenarios:

Scenario	Result
One closure (order 5) before terminal (order 10)	Not settled — 5 <= 10 ✅
One closure (order 15) after terminal (order 10)	Settled — 15 > 10 ✅
Two closures (order 5, then order 12) before a later terminal (order 20)	Not settled — last-writer gives {order:12}, 12 <= 20 ✅
Two closures (order 5, then order 15) straddling a terminal (order 10)	Settled — last-writer gives {order:15}, 15 > 10 ✅

The last row covers the normal lifecycle: a PR is closed, gated once more (terminal at 10 in this example), and then stays closed. The latest closing fact (order 15, from a fresh sweep after re-close) correctly settles the new terminal. ✅

Fix 1c / 388b7b3 — Sweepability

TestReopenedPRIsNotMootedByItsOldClosure pins three sub-cases: park-after-closure stays live, authorization-after-closure stays ready, and — the subtlety Codex named — the reopened park is back on the sweep work list. The third case calls LiveSubjects directly and asserts the re-gated park appears. This is correct because LiveSubjects calls parkedRuns(terms, closed, "") which uses the fixed settles, so the ordering check removes the stale closure from the work-blocking path. ✅

Fix 1d — b.State whitelist (3c546bc)

var closingStates = map[string]bool{
    ClosedNotOpen:   true,
    ClosedMerged:    true,
    ClosedAbandoned: true,
}
// ...
if b.Repo == "" || b.Number == 0 || !closingStates[b.State] {
    return
}

Correct and in the right direction: an unrecognised state costs a stale row, never a hidden park. Tested by TestUnrecognisedClosingStateLeavesTheRowVisible with four invalid values ("", "OPEN", "draft", "not-open"). ✅

receipt: "superseded" — documented

The comment in receiptClosings now explains the vocabulary distinction: "superseded" in #249 is a fact about the PR (merged at a different head), not about the authorization. The PR is gone either way, making it a closing fact. This was the round-1 note-to-confirm; it is confirmed and recorded. ✅

---

Summary

Finding	Round 1 verdict	Round 2 verdict
1c — re-opened PR false-MOOT	⚠️ medium	✅ fixed — ordering-based settles, pinned in both directions
1c/sweep — re-opened park unsweepable	⚠️ (Codex named)	✅ fixed — LiveSubjects uses the same fixed settles, sub-test pins it
1d — absorbSubjectClosed ignores b.State	ℹ️ low	✅ fixed — whitelist, fail-safe direction, tested
receipt: "superseded" semantics	🔍 confirm before #249 lands	✅ confirmed and documented in-code
All round-1 greens (1a, 1b, 2, 3a, 4, 5)	✅	unchanged, still hold

No new issues found. The ordering-based fix is correct across all four closing sources, the slice-index ordering invariant is maintained at every call site, and the tests pin both directions of the fix. The PR is ready from a round-2 review perspective.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3c546bc13f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Revalidate the terminal before recording a closure

When a PR is reopened and re-gated while the sweep's GitHub fetch is in flight, the fetch can report it absent, a new terminal can then be appended, and this call finally appends subject_closed after that new terminal. AppendIfAbsentParent only deduplicates closures for the old terminal, while closureIndex.settles uses subject-wide log ordering, so the late closure incorrectly hides the fresh park/ready row and removes it from future sweep work. Fresh evidence beyond the earlier thread is this unlocked read-to-append race; revalidate that the parent is still the subject's newest terminal atomically, or scope subject_closed settlement to its parent.

AGENTS.md reference: cmd/gate/AGENTS.md:L153-L173

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Match the wrapped duplicate sentinel with errors.Is

When two sweep processes operate on the same live terminal concurrently, the first append succeeds and the second returns fmt.Errorf("%w: ...", ErrAlreadyExists) from the store. Direct equality is therefore false, so the second sweep exits with an error instead of reporting the successfully deduplicated closure as Already; use errors.Is(err, state.ErrAlreadyExists) as the other callers do.

Useful? React with 👍 / 👎.

[itsHabib]

Both findings from round 2 are real and are fixed in 7ecbc6b. The P2 also caught a weakness in one of my own tests, which is the more useful half of it.

P1 — read-to-append race: fixed

Correct, and it is the same defect as the earlier one arriving by a different route. The ordering rule fixed history (a closure written before a later terminal); this is the same thing produced by a race: the work list is read, the GitHub fetch takes seconds, and a PR reopened and re-gated inside that window gets a new terminal — so the closure lands after it in the log and moots the fresh park. The absent-parent guard waved it through because it is keyed on the old terminal.

recordClosed now passes a stillNewest check to AppendIfAbsentParentWhereAfterAudit, evaluated inside the store lock, so no terminal can land between the check and the append — the same primitive applyJudgment's requireOpenEscalation uses.

I took the revalidate option over scoping settlement to the parent, for the reason given on the earlier thread: coverage is parented to nothing subject-shaped and receipt to the action, so parent-scoping only ever covers subject_closed. The check reduces through observe's shared fold (NewestTerminal) rather than forming a second opinion about which terminal is current.

A re-gated subject is reported, not failed — the fresh terminal is the current truth, and a later sweep can close it on its own merits. TestSweepDeclinesAClosureForARegatedSubject asserts the closure is declined and that the fresh park is still live afterwards.

P2 — wrapped sentinel: fixed, and it exposed a vacuous test

Right on the mechanism — the store returns fmt.Errorf("%w: ...", ErrAlreadyExists), so err == never fires and a correctly deduplicated concurrent sweep reports as a hard failure. Now errors.Is, matching every other caller.

The part worth naming: TestSweepIsIdempotent could never have caught this. The work list is the live rows, so after the first sweep there is nothing left to check — second.Closed is empty and its Already assertion looped over an empty slice. It passed by being vacuous. That vacuity is now asserted directly (second.Checked == 0), and TestSweepReportsAWrappedDuplicate reaches the real path by racing two closures on one terminal.

Both new tests were confirmed to fail against their specific bug and pass against the fix, rather than assumed to cover it.

---

gofmt, go vet, golangci-lint (0 issues), go test ./..., go test -race ./cmd/gate/... all clean. Live ledger unchanged: 161 of 164, leaving the 3 genuinely open.

Unrelated: CI's check job failed once on TestReconcileRace_OneWinner in cmd/runway — no dependency path from this diff (go list -deps ./cmd/runway/internal/controller | grep cmd/gate is empty), main green at df45b2f, passes locally 8× under -race. Green on re-run. Filed separately, since corrupt run state: terminal journal without result may be a genuine write-ordering bug in runway rather than only a flaky assertion.

That is fix-round 2 of 2 — the cap. Per the repo's review-cycle discipline I stop fixing here; anything further goes to the judge as residual.

[itsHabib]

@claude one final pass on the current head 7ecbc6b — the panel's newest attestation is on 3c546bc, and gate evaluates the exact head.

No code changes since your round-2 re-review beyond 7ecbc6b itself, which addresses codex's round-2 P1/P2:

1. The read-to-append race. recordClosed now passes a stillNewest check to AppendIfAbsentParentWhereAfterAudit, evaluated inside the store lock, revalidating that the closure's parent is still the subject's newest terminal. Worth checking: does the check reduce through the same shared fold (observe.NewestTerminal) rather than forming a second opinion, and is errTerminalMoved correctly a reported outcome rather than a failure?
2. The wrapped sentinel. err == state.ErrAlreadyExists → errors.Is. Also worth checking that the new Regated field doesn't distort renderSweep's closure count (it subtracts res.Regated from len(res.Closed)).

This is a re-attestation of the final head, not a fix round — I'm at the 2-round cap and will take any further findings to the judge as residual.

[claude]

Claude finished @itsHabib's task in 2m 39s —— View job

---

Final pass — 7ecbc6b

• Load tools
• Read sweep.go — recordClosed, errTerminalMoved, renderSweep
• Read AppendIfAbsentParentWhereAfterAudit in state.go
• Read observe.NewestTerminal — confirm shared fold
• Check errors.Is fix and Regated field accounting
• Post final review

---

Check 1 — stillNewest reduces through the shared fold; errTerminalMoved is a reported outcome

Shared fold confirmed. observe.NewestTerminal calls foldSubjectTerminals(arts) with the same arts slice that every other projection reduces through. It is not a second opinion — it is the same computation as buildClosureIndex and buildInbox. ✅

Lock coverage confirmed. The execution path in AppendIfAbsentParentWhereAfterAudit is:

1. s.lock()
2. s.auditLocked() — produces audit.All
3. check(audit) — stillNewest runs here, inside the lock
4. uniqueness scan (→ ErrAlreadyExists)
5. s.appendLocked()

No terminal can land between the check and the append. Same primitive applyJudgment's requireOpenEscalation uses. ✅

errTerminalMoved propagation confirmed. The check(audit) return value is returned unwrapped (return Artifact{}, err — no %w), so errors.Is(err, errTerminalMoved) matches it directly in recordClosed. The match converts to out.Regated = true and returns (out, nil) — no error escapes to runSweep. The test names this property explicitly: "a re-gated subject is a normal outcome, not an error". ✅

Precedence between errTerminalMoved and ErrAlreadyExists is structural, not ordering-dependent. check runs before the uniqueness scan; if it returns an error, the function returns immediately and the ErrAlreadyExists path is never reached. The two errors are mutually exclusive by construction, so the order of the errors.Is checks in recordClosed is irrelevant. ✅

---

Check 2 — Regated field does not distort renderSweep's closure count

res.Closed is appended for every subject GitHub reports as not-open — including Already entries and Regated entries. res.Regated is incremented only after closed.Regated == true inside the same loop, so res.Regated <= len(res.Closed) by construction; the subtraction cannot go negative.

len(res.Closed) - res.Regated correctly computes "subjects with a confirmed closure artifact (newly recorded or already present)". A regated entry means nothing was recorded — the terminal moved, so we cannot even assert the old terminal is closed — and it should not count as a closure. The arithmetic is correct. ✅

closedNote prints (re-gated mid-sweep; not closed) on those rows, and the summary banner below them explains the policy. A reader of the text output gets the right count and can see exactly which entries were declined and why. ✅

---

The vacuous test fix is correct

TestSweepIsIdempotent previously asserted c.Already in a loop over second.Closed — but the work list is the live rows, so after the first sweep there are no live rows to check, second.Closed is empty, and the loop was vacuous. Now it asserts second.Checked == 0 directly. TestSweepReportsAWrappedDuplicate reaches the real deduplication path by calling recordClosed twice on the same terminal directly. ✅

---

Summary

Check	Verdict
stillNewest reduces through observe.NewestTerminal (same fold)	✅ confirmed
Lock covers check-to-append gap	✅ confirmed
errTerminalMoved is a reported outcome, not a failure	✅ confirmed
errors.Is for ErrAlreadyExists (wrapped sentinel)	✅ confirmed
len(res.Closed) - res.Regated arithmetic is sound	✅ confirmed
Prior vacuous test corrected	✅ confirmed

No new issues at 7ecbc6b. The PR is clean at this head.

[itsHabib]

@claude review

[claude]

Claude finished @itsHabib's task in 2m 28s —— View job

---

Final review — gate: close the inbox by supersession and mootness (7ecbc6b)

• Read sweep.go — recordClosed, errTerminalMoved, Regated field, renderSweep
• Read state/state.go — AppendIfAbsentParentWhereAfterAudit, lock semantics
• Read observe/closure.go — NewestTerminal, shared fold
• Read observe/discharge.go — ParkDischargeReport, precedence
• Read sweep_test.go — new tests for race and wrapped sentinel
• Analyze all focus areas
• Post final verdict

---

Check 1 — stillNewest reduces through the shared fold

observe.NewestTerminal at closure.go:526–536 calls foldSubjectTerminals(arts) directly — the same function buildInbox, ParkDischargeReport, and LiveSubjects all reduce through. No second opinion, no parallel derivation. Every caller passes the same arts slice, so the order indices in closingFact and the terminal IDs in subjectTerminals are drawn from the same positional basis.

The critical path in AppendIfAbsentParentWhereAfterAudit (state.go:202–237):

1. s.lock() — store lock acquired
2. s.auditLocked() — scans the log, producing audit.All
3. check(audit) — stillNewest runs here with audit.All, inside the lock
4. uniqueness scan (→ ErrAlreadyExists)
5. s.appendLocked()

No terminal can land between the check and the append. ✅

Check 2 — errTerminalMoved is a reported outcome, not a failure

check is called at state.go:220–223 and its error is returned unwrapped (return Artifact{}, err). Back in recordClosed, the check order is:

if errors.Is(err, state.ErrAlreadyExists) { ... }   // wrapped by store
if errors.Is(err, errTerminalMoved)       { out.Regated = true; return out, nil }
if err != nil                             { return SweepClosed{}, fmt.Errorf(...) }

errTerminalMoved is matched by errors.Is via pointer equality (it's returned unwrapped; errors.Is works on the sentinel itself). The match converts to out.Regated = true and return out, nil — no error escapes to runSweep. ✅

Precedence between the two sentinels is structural, not ordering-dependent. check(audit) runs before the uniqueness scan; if it returns an error the function returns immediately and the ErrAlreadyExists path is never reached. The two errors are mutually exclusive by construction. ✅

Check 3 — Regated field does not distort renderSweep's closure count

In runSweep, every non-error recordClosed result — including Regated and Already entries — is appended to res.Closed, while res.Regated is incremented only for those where closed.Regated == true. So len(res.Closed) - res.Regated is the number of subjects with an actual (or pre-existing) closure artifact on the log, which is exactly what the banner should count. ✅

closedNote prints "(re-gated mid-sweep; not closed)" on regated rows in the per-row table, so the output is transparent: the headline count excludes them, the row list names them and explains why. ✅

Dry-run note: recordClosed returns early before AppendIfAbsentParentWhereAfterAudit when dryRun=true, so the stillNewest check never runs on the dry-run path. A dry run therefore may overcount "would record" by the number of subjects that would be declined as regated. This is an acceptable approximation — it shows what the sweep would do if no re-gate occurs between the read and the write, which is what a dry run is for.

Check 4 — errors.Is for the wrapped ErrAlreadyExists

The store wraps on the ErrAlreadyExists return (fmt.Errorf("%w: run %s kind %s", ErrAlreadyExists, ...)), so a direct == comparison silently never fires. recordClosed now uses errors.Is(err, state.ErrAlreadyExists), which unwraps correctly. TestSweepReportsAWrappedDuplicate reaches this path by calling recordClosed twice on the same terminal — the vacuous-loop issue the previous test had is also corrected (second.Checked == 0 asserted directly). ✅

Check 5 — TestSweepDeclinesAClosureForARegatedSubject covers the race

The test appends a fresh escalation for o/widget#7 (simulating the re-gate that happened while the fetch was in flight), then calls recordClosed with the old terminal. It asserts:

• got.Regated == true, got.Artifact == "" — closure declined, not an error
• in.Parked[0].Run == "run_regate" — the fresh park survives intact

Both directions of the ordering rule are pinned: TestReopenedPRIsNotMootedByItsOldClosure (pre-existing closure does not moot a later park) and TestClosureStillSettlesAnOlderTerminal (a closure that legitimately postdates its terminal still moots it). ✅

---

Summary

Check	Verdict
stillNewest reduces through observe.NewestTerminal (shared fold)	✅ confirmed
Lock covers check-to-append gap	✅ confirmed
errTerminalMoved is reported, not a failure; return out, nil	✅ confirmed
Precedence of errTerminalMoved vs ErrAlreadyExists is structural	✅ confirmed
errors.Is for wrapped ErrAlreadyExists	✅ confirmed
len(res.Closed) - res.Regated arithmetic is sound	✅ confirmed
Dry-run approximation is acceptable	✅ noted, intentional
All prior round-2 greens unchanged	✅

No new issues at 7ecbc6b. The PR is clean at this head.

[github-actions]

Reviewer: claude
Reviewed commit: 7ecbc6bcc862f1e2a03540cb7306ff0b73ef30f3

[itsHabib]

Gate: parked for judgment — the residual, in writing

Two gate runs, and the queue narrowed to one thing.

Run 1 (run_52379bf5e4e04f1f) parked on three rungs. Two were my own procedural error, not the code's:

review-panel-completeness: completed=0 expected=1 missing=[claude]

Claude publishes as an issue comment, which carries no commit anchor, so gate takes its authority from the workflow attestation instead — and that step only fires when the triggering comment is exactly a review request. The regex requires the whole body to be @claude review (modulo please / this pr / trailing punctuation). All three of my triggers carried explanatory text, so none attested. Claude reviewed this PR four times and gate could not see any of it.

That is the workflow behaving as designed — claude.yml spells out the tradeoff, precision over recall, because "a missed attestation leaves the panel incomplete and gate parks for judgment, which is merely today's behaviour, while a false one credits a review that never happened." Correct call. A bare @claude review produced the attestation for 7ecbc6b, and run 2 (run_bf8085bff596fd8e) cleared both rungs.

What remains — the actual residual:

review-consolidation: 6 bot comments: 1 actionable
p1 cmd/gate/internal/observe/closure.go:233 [chatgpt-codex-connector] Scope closure facts to the terminal they close

That is codex's round-1 P1, and it is fixed — in 7e3e128, before its round-2 review even posted. closure.go:233 is now order: order on the closingFact literal, and settles refuses any fact that does not postdate the terminal it is asked to settle. It is unretracted because bots do not retract, and the comment is anchored to a line that still exists.

The evidence that it is addressed, not merely claimed:

• TestReopenedPRIsNotMootedByItsOldClosure pins four sub-cases — park stays live, re-authorization stays ready, the subject is back on the sweep work list, the audit metric counts it live.
• TestClosureStillSettlesAnOlderTerminal pins the other direction, so a fix that merely disabled the moot class would fail.
• Codex's round-2 P1 (the read-to-append race — the same defect via a race rather than history) is fixed in 7ecbc6b with a locked revalidation, and its round-2 P2 (wrapped sentinel) alongside it. Both new tests were confirmed to fail against their specific bug and pass against the fix.
• Claude re-reviewed 7ecbc6b directly: "No new issues at 7ecbc6b. The PR is clean at this head." It independently checked the invariant the whole fix rests on — that foldSubjectTerminals and buildClosureIndex range the same slice, so their orders are comparable — at all three call sites.

Stopping here by rule, not by preference. Two fix-rounds is the cap and both are spent; two gate runs is the stop threshold and both are spent. retry_helps: false — a third run re-evaluates from scratch and re-parks on the same unretracted comment. This is the verified-addressed-but-unretracted case the review-cycle discipline sends to the judge, so that is where it goes.

Escalation esc_269a87b006a26c70 · run run_bf8085bff596fd8e · head 7ecbc6b · CI green · merge state CLEAN.

[itsHabib]

@claude review

[claude]

Claude finished @itsHabib's task in 1m 56s —— View job

---

Re-attestation — gate: close the inbox by supersession and mootness (b91e997)

• Load tools and gather context
• Examine what changed since last reviewed commit (7ecbc6b)
• Read key PR files: closure.go, sweep.go, discharge.go, inbox.go
• Verify merge of main did not introduce regressions
• Post review

---

The previous attestation was on 7ecbc6b. The current HEAD is b91e997, a merge of origin/main into this branch. The gate PR files — closure.go, sweep.go, discharge.go, inbox.go, state.go — are unchanged since 7ecbc6b. The prior verdict on each carries forward. What the merge added is narrow:

What the merge commit (b91e997) changed

1. cmd/gate/AGENTS.md and cmd/gate/CLAUDE.md — docs-only conflict resolution

Two bullets from main's #253 were appended alongside this branch's inbox-closure documentation: "A run that decides nothing spends nothing" and "Evidence reads retry on an allowlist, and still fail closed." The merge kept both sides independently. The pair remains byte-identical as CI requires. ✅

2. cmd/gate/aborted_cycle_test.go — one call-site adaptation

Main's #253 added this file with observe.NextText(&text, e.st, time.Now, "") — the old bare-string signature. This branch changed NextText/NextJSON to take observe.NextRequest. The merge adapted the call:

// before (main's form, old signature):
observe.NextText(&text, e.st, time.Now, "")

// after (this branch's form, new signature):
observe.NextText(&text, e.st, time.Now, observe.NextRequest{})

NextRequest{} is semantically equivalent: StateArg: "" matches the old empty string, and Fetch: nil gives offline mode — which is what cycles_preflight_test.go already uses at its identical call site. The test's invariant (gate next agrees with cycleCount on the budget) is unchanged. ✅

3. FOLLOWUPS.md — a pre-existing runway bug (runway scope, not gate scope)

The runway writeResultAtomic fsync entry was added during the #259 review; it documents a pre-existing issue in runway's write path, not introduced by this PR.

---

Summary

Area	Verdict
Core PR logic (closure.go, sweep.go, discharge.go, inbox.go, state.go)	✅ unchanged from 7ecbc6b, prior attestation carries
AGENTS.md / CLAUDE.md byte-identical pair	✅ confirmed
aborted_cycle_test.go adaptation ("" → NextRequest{})	✅ semantically equivalent
Merge introduced no gate-semantic changes	✅ confirmed

No new issues at b91e997. The PR is clean at this head.

[github-actions]

Reviewer: claude
Reviewed commit: b91e9979c1862f1dd10a16521a1fdb8c404e4359