fix(gate): a run that decides nothing records why, and still burns no cycle

[itsHabib]

The report, and what the log actually says

a gate gate run that dies during evidence gathering still consumes a review
cycle against the grant's max_cycles ceiling … cycle counting derives from
distinct runs that own evidence artifacts

The failure was real. run_f7ea75f642e3064c (#247, 2026-08-23)
died with exit 4 on Post "https://api.github.com/graphql": read tcp …: connection reset by peer, after gh pr view had already landed its evidence
artifact.

The counting claim is not. cycleCount has joined outcome → parent reduced
verdict → subject since gate moved in at #59 — one distinct run holding a
counting action or escalation is one cycle. Evidence is never read. Recomputing
both counts over the live log for #247:

outcome-based cycles  1   [run_b18672cdb7b12634]              <- what gate enforces
evidence-based runs   2   [run_b18672…, run_f7ea75f642e3064c] <- the number in the report

The aborted run owns exactly one artifact — the pr view evidence — and no
outcome, so it contributed zero. The single cycle came from the completed run
that followed it and parked for judgment. Option (c) in the issue is the
shipped design, so the literal fix is a no-op.

That leaves three things worth doing, and this PR does all three.

Weighing the options

• (a) don't append evidence until the run is committed to a verdict —
  rejected. It would delete the honest record of what gate read, and the
  already-merged refusal is decided from the view artifact specifically so it
  lands before the rest of the sweep can fail. Withholding evidence to protect a
  count that doesn't read evidence trades a real property for an imaginary one.
• (b) mark aborted runs with an explicit terminal artifact — taken. Not to
  fix the count, but because the log cannot currently say a run died:
  gate explain -run run_f7ea75f642e3064c prints one evidence artifact, exit 0,
  and nothing else. An aborted run is indistinguishable from one still in flight.
• (c) count from verdicts/escalations — already how it works. Now pinned by
  tests instead of holding by construction.

What changed

The invariant is asserted, not incidental. TestOnlyRunsThatDecideBurnACycle
walks one subject through all three cases in order — a completed run consumes a
cycle, a run that dies gathering evidence consumes none, the next completed run
consumes the next — so a regression in either direction fails. The abort is
driven by a stub gh that reproduces the real shape: view succeeds, diff dies on
the reset. A second assertion checks gate next's number against the one gate
enforces; the two counting rules live in different packages
(main.cycleCount and observe.cyclesBySubject) and nothing previously stopped
them drifting apart — an operator reading "2/3" while gate refuses at 3.

An aborted run now says so. A run_aborted record naming the subject and the
cause, appended at the single error funnel in runGateWithSynthesis. It sits
outside the action/escalation families exactly like grant_needed, so the count,
the reducer, and next's subject reduction all ignore it — that exclusion is
pinned too. The evidence already written stays: an append-only log answers "this
run died" by appending the fact, never by un-writing what came before. Two exits
are deliberately not annotated — any code but codeError reached a terminal
and decided something, and a tampered log is corruption rather than an abort
worth describing.

The blip is now unlikely to end a run at all. Each gh read retries up to
three times with growing backoff. The classifier is an allowlist — transport
faults and GitHub's retryable statuses retry; a missing binary, a bad credential,
a 404, a malformed query fail on the first attempt rather than sleeping through
the bound. It fails closed: a failure that outlives the bound is returned
unchanged and the run still aborts. Retrying is safe only because every one of
these calls is a read, and it must never become a way to proceed without an
answer — TestGHFailsClosedAtTheBound is the guard on that.

Consistency with #242 and #249

Rebased onto main (8fc0300). #242 landed the pre-flight ceiling refusal and
cycles_used/cycles_max on every result — both are on main, and this PR
changes neither the counting rule nor the reporting fields, so #242's
grant_needed refusal record and this run_aborted record are siblings under
the same law: durable facts, never outcomes. #249 (decision provenance) is open
and touches the decision path, not counting; no overlap.

Checks

gofmt · go vet · golangci-lint run ./... (0 issues) · go test ./... ·
go test -race ./cmd/gate/... — all green. Guide pair re-synced; boundary law
verified locally (no new cross-tool imports).

🤖 Generated with Claude Code

[itsHabib]

@claude review

[itsHabib]

@codex review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

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

---

Code Review — PR #253

Todos

• Load tools
• Read changed files
• Review main.go changes (abort record + retry wiring)
• Review evidence/evidence.go (retry logic)
• Review state/state.go (run_aborted record type)
• Review observe/observe.go (cycle counting)
• Review tests (aborted_cycle_test.go, retry_test.go, panel_test.go)
• Review docs (DESIGN.md, CLAUDE.md, AGENTS.md)
• Post final review

---

Reviewed head: a7a14a3. Overall: clean, well-motivated, and correctly scoped. Three substantive observations below; the rest are minor.

---

recordAbortIfUndecided and the defer placement

The defer at main.go:536:

defer func() { err = recordAbortIfUndecided(e, run, grantID, subject, code, err) }()

subject is captured by reference (Go closures capture the variable, not the value), so if readinessVerdict successfully mutates it to carry HeadSHA before a later step fails, the abort record will carry that SHA — which is the right answer. If the run dies before readinessVerdict returns, it carries {Repo, Number} only. Both are correct. ✓

The guard in recordAbortIfUndecided (code != codeError || cause == nil || errors.Is(cause, errLogTampered)) is complete: parked/refused/clean-exit all pass through unchanged, tampered-log failures are excluded, and the nil-cause short-circuit prevents a vacuous record. TestAbortRecordIsScopedToUndecidedRuns pins all five branches. ✓

One nit: recordRunAborted uses time.Now().UTC() directly rather than e.now. This is consistent with recordGrantNeeded, so it's not a new inconsistency, but it means the at timestamp in the abort body is uncontrollable in tests (the test doesn't assert it, so no practical harm). If e.now is ever used as a test seam for this field elsewhere, something to track.

---

Retry logic in evidence.go

The allowlist + transientGH design is correct: anything unrecognized fails fast rather than sleeping through the bound.

func gh(args ...string) (json.RawMessage, error) {
    for attempt := 1; ; attempt++ {
        out, err := runGH(args...)
        if err == nil {
            return out, nil
        }
        if attempt == ghAttempts || !transientGH(err.Error()) {
            return nil, err
        }
        ghSleep(time.Duration(attempt) * ghBackoff)
    }
}

TestGHFailsClosedAtTheBound is the essential guard here — retrying must never silently become a path to proceed without an answer. ✓

Observation: "no such host" in ghTransient is a DNS-lookup failure. DNS failures are legitimately transient (SERVFAIL, network partition), so this is a reasonable call — but it means a completely wrong hostname also retries three times before surfacing. Three bounded attempts with growing backoff is a modest cost; the test TestGHDoesNotRetryAPermanentFailure confirms the 404 path still fails fast. The allowlist design itself is the right framing here, and the comment in transientGH is clear that additions require deliberate justification.

Minor: ghSleep is a package-level var mutated by tests via t.Cleanup. This is idiomatic Go for time injection in tests, and stubSleep restores the original on cleanup, so parallel test isolation is fine. Just worth noting it's the one mutable package global in this package.

---

assertNextAgreesOnBudget — the cross-package count pin

This is the most important new assertion:

func assertNextAgreesOnBudget(t *testing.T, e env, parkRun string, subject verify.Subject) {
    want := mustCycleCount(t, e, subject)
    // ... checks observe.ParkedRun.CyclesUsed == want
    // ... checks next text contains "cycles 2/3"
}

cycleCount (in main) and cyclesBySubject (in observe) are independent implementations. Before this PR, nothing stopped them drifting — the PR description calls that out explicitly. The test that pins both in one scenario to the same number is the right fix, and it's driven end-to-end through the real run/park/abort/count path. ✓

The hardcoded "cycles 2/3" string in the text assertion is tight to the test's scenario (second cycle, ceiling of 3). It would silently pass if the label format changes to something other than cycles N/M — but given the existing golden tests in the observe package, a format change would likely be caught there. Acceptable.

---

observe.go — the two-line addition

-case state.KindGrant, state.KindEscalation, state.KindAction, state.KindGrantNeeded:
+case state.KindGrant, state.KindEscalation, state.KindAction, state.KindGrantNeeded, state.KindRunAborted:

Correct in both projectNode and renderNode. The run_aborted body is a flat object (repo, number, grant, error, at), which projectFlat/renderFlat handle cleanly. The existing golden tests in the observe package should cover the rendering path. ✓

---

state.go — KindRunAborted

KindRunAborted = "run_aborted" with prefix "abt" added to the prefix map. The docstring is clear about why it sits outside the action/escalation families, and the invariant it exists to make explicit rather than incidental. ✓

---

Tests

All four test scenarios in TestOnlyRunsThatDecideBurnACycle are well-chosen: completed → aborted → completed, with the stub ghResetAfterView reproducing the real-world failure shape (view lands, diff resets). assertRunAborted additionally verifies that gate explain surfaces the cause — the operator-facing payoff of the whole change. ✓

TestAbortRecordIsScopedToUndecidedRuns is exhaustive over the branching conditions. The fakeGH + full PATH replacement pattern is correct here (shell built-in echo is safe with no PATH).

---

Summary

No blocking issues. The PR correctly:

• Pins the cycle-count invariant (was always true, now tested)
• Adds the abort record as a non-outcome fact (the log can now say why a run stopped)
• Makes the blip less likely to end a run at all (bounded, allowlist-gated retries)

The implementation is consistent with the repo's boundary law (KindRunAborted outside action/escalation, no cross-tool imports) and the append-only contract (abort appends, never un-writes).

[github-actions]

Reviewer: claude
Reviewed commit: a7a14a3fa7b2c6661f0d93b19982375e05978f75

[Copilot]

🔵 Needs a closer look

It changes core gate decision-path behavior (new durable artifact type plus evidence-retry semantics) that affects authorization invariants and should receive final human review.

Pull request overview

This PR hardens gate’s review-cycle accounting and run observability by (1) explicitly recording when a run aborts before any decision is produced, (2) pinning the “only decisions burn cycles” invariant with tests, and (3) making evidence collection more resilient via bounded, allowlisted retries for transient gh failures.

Changes:

• Add a run_aborted artifact recorded on undecided (exit-4) runs so the log can explain why a run stopped without producing an outcome, while still burning no review cycle.
• Add regression tests that prove aborted runs do not advance cycle count and that gate next’s budget display matches the enforced cycle counting rule.
• Retry gh evidence reads up to 3 times with growing backoff for allowlisted transient failures; fail closed at the bound and for permanent errors.

File summaries

File	Description
cmd/gate/main.go	Records run_aborted on undecided error exits; cycle counting remains outcome-based.
cmd/gate/internal/state/state.go	Adds KindRunAborted and its ID prefix.
cmd/gate/internal/observe/observe.go	Projects/renders run_aborted as a flat node in explain output.
cmd/gate/internal/evidence/evidence.go	Implements bounded retry around gh reads with transient-failure allowlist.
cmd/gate/internal/evidence/panel_test.go	Extends package TestMain to dispatch the new retry helper process.
cmd/gate/internal/evidence/retry_test.go	New tests pin retry/backoff behavior and fail-closed semantics.
cmd/gate/aborted_cycle_test.go	New tests pin cycle invariants across completed/aborted/completed runs and next agreement.
cmd/gate/docs/DESIGN.md	Documents the outcome-based cycle rule, abort annotation, and retry policy.
cmd/gate/CLAUDE.md	Updates tenant guidance to include the new invariants and retry behavior.
cmd/gate/AGENTS.md	Mirrors the same guidance updates as CLAUDE.md.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[itsHabib]

Round 1 dispositions against the panel on a7a14a3 (fixes in 5746e45) — this is the only fix-round this PR needs; anything residual goes to the judge.

Copilot — aborted_cycle_test.go: shell stub makes the test OS-dependent — fixed. Switched to the helper-process pattern the evidence package already uses: the test binary installs itself on PATH as gh and TestMain intercepts. Portable, and it drops the shell-quoting fragility the script carried (PATH holds only the stub, so even cat was unreachable — that bit once during development).

Claude — no blocking issues; three observations, all acknowledged rather than changed:

• "no such host" retries a permanently wrong hostname three times — deliberate. DNS SERVFAIL and partitions are genuinely transient, and three bounded attempts is the cost of covering them. The allowlist's whole point is that the default is permanent.
• ghSleep as the package's one mutable global — idiomatic time injection, restored via t.Cleanup, never reassigned in production. -race clean.
• Hardcoded "cycles 2/3" is tight to the scenario — kept. The observe package's golden tests own the label format; this assertion owns the number, which is the thing that could drift between main.cycleCount and observe.cyclesBySubject.

Codex — out of quota (why #250 dropped it from require). Not gathered.

Also folded in one pin the review prompted rather than requested: an oversized diff (HTTP 406) must route to the local-diff fallback, never to a retry — the answer is deterministic, so retrying would only delay it. Now a row in the allowlist's permanent set.

@claude review

[claude]

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

---

Code Review — PR #253 (Round 2, head 5746e45)

Reviewed head: 5746e45. Two changes relative to Round 1: the shell-script stub replaced by the helper-process pattern, and HTTP 406 pinned as permanent. Both are clean. No blocking issues.

---

Todos

• Load tools
• Read changed files since Round 1 (5746e45 vs a7a14a3)
• Review aborted_cycle_test.go — helper-process gh stub
• Review retry_test.go — HTTP 406 permanent-failure pin
• Consistency check: duplicate TestMain? arg-slice guard? format string match?
• Post final review

---

Fix 1 — helper-process stub (aborted_cycle_test.go)

The Copilot finding was valid. The replacement is correct:

• TestMain gates on GO_WANT_GH_RESET_HELPER_PROCESS, distinct from the evidence package's GO_WANT_GH_HELPER_PROCESS and GO_WANT_GH_RETRY_HELPER_PROCESS — no cross-package env collision. ✓
• runResetAfterView checks len(os.Args) > 2 && os.Args[1] == "pr" && os.Args[2] == "view". For gh pr view 7, os.Args is ["<binary>", "pr", "view", "7"] — guard is correct, index 2 is always in bounds when the condition holds. For gh pr diff, it falls through to the reset error. ✓
• No other TestMain in package main (main_test.go defines none) — no duplicate. ✓
• install hard-links, falls back to copy, and handles ".exe" on Windows. This is a deliberate copy of installFakeGH in the evidence package; the comment explains the reasoning (twenty lines isn't worth a coupling). The design is correct by the repo's boundary law. ✓
• t.Setenv restores env vars after each test, so test isolation holds. ✓

One nit (no action needed): the comment on openPRView says "the already-merged refusal declines to fire and the run proceeds to the diff read that kills it" — accurate, and helpful for a future reader who wonders why a clean reviewDecision: APPROVED is still used when the test goal is just to reach the diff call.

Fix 2 — HTTP 406 pin (retry_test.go)

Added to the permanent slice in TestTransientGHIsAnAllowlist:

"evidence: gh [pr diff]: HTTP 406: the diff exceeded the maximum number of lines",

• 406 is already permanent by default (not in ghTransient), so this is an asserting pin rather than a behavioral change. ✓
• The format string matches what runGH produces: fmt.Errorf("evidence: gh %v: %s", args[:2], ee.Stderr) where args[:2] is [pr diff]. The test message evidence: gh [pr diff]: HTTP 406: ... is consistent. ✓
• The comment is clear on why: deterministic answer, retrying only delays routing to the local-diff fallback. An accidental future addition of a broad "http 4" pattern to the allowlist would cause this test to fail immediately. ✓

Summary

Both fixes are correct and complete. The helper-process migration is the right pattern for portability (same shape as the evidence package, no shared test-only coupling). The 406 pin is the right shape of assertion — it tests the default rather than adding a new mechanism. Nothing warranted follow-up from Round 1's observations; the residuals were correctly acknowledged and deferred.

---

[itsHabib]

Gate: parked — run_5ccef5cad4529352, exit 2, cycle 1 of 3 against grt_559f14e81ff58c72. Head 5746e45, all three CI jobs green, MERGEABLE.

readiness: no review decision reported by GitHub — cannot verify readiness; review-panel-completeness: review panel incomplete: completed=0 expected=1 pending=[] missing=[claude]

Not a finding against the diff. The panel requires a formal GitHub review from claude, and the Claude app posts issue comments here — so reviewDecision stays empty and completeness reads 0/1. Both Claude rounds substantively reviewed the exact head (round 2 names 5746e45 and reports no blocking issues); Copilot's only finding was fixed in 5746e45 and its inline comment is now outdated. Codex is out of quota, which is why #250 dropped it from require.

This is the same terminal state #200 and #242 reached — the judge's residual acceptance is the terminating condition, not a fourth panel round. Not re-running gate: a second run would re-evaluate from scratch and re-park on the same gap.

Awaiting the operator-gated judge.