fix(review-gate): move the comment gate out of review/ and supersede what it left behind (BLO-29711) by allyblockcast[bot] · Pull Request #1471 · Blockcast/paperclip

allyblockcast · 2026-08-22T17:14:27Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

Merge gating: pr-comment-review-gate.ts publishes a commit status for Ally's comment-shaped consolidated reviews, because GitHub's reviewDecision ignores COMMENTED reviews and Ally cannot formally approve its own App-authored PRs

The gate is fail-open by design — it observes only the comment surface, so "nothing attests this head" is the normal state of a formally-reviewed PR, and it must stay green there or every such PR deadlocks

But it published that green under review/ally-comment, where a green reads as "this head was reviewed and was clean". Measured success on 43 of the 60 most recent merges to penstock main, never once failure — a control-namespaced status that means "not evaluated"

The fail-open and the review/ namespace cannot coexist, and the fail-open is the load-bearing half. So the namespace is what moves

This pull request renames the live context to gate/ally-comment-findings and, because commit statuses cannot be deleted, has the gate supersede the vacated context in place with a retirement pointer

The benefit is that no green review/* status can be read as review evidence it never had — and the pre-rename greens already frozen on 42 of 43 open penstock PRs get corrected rather than preserved

Linked Issues or Issue Description

Refs BLO-29711 — closes its AC#1. AC#2/AC#3 landed in fix(review-gate): carry undispositioned comment findings across head moves (BLO-29711) #1464.
Refs BLO-28988 (same defect, filed earlier), BLO-26602 (activation of this gate in branch protection).

What Changed

values.blockcast.yaml — prCommentReviewGateStatusContext moves review/ally-comment → gate/ally-comment-findings.
prCommentReviewGateRetiredStatusContexts (new, comma-separated; default "" so non-Blockcast deployments stay inert) — contexts the gate used to publish to. Plumbed through values.yaml, both deployment-api.yaml and statefulset.yaml, and config.ts as PAPERCLIP_PR_COMMENT_REVIEW_GATE_RETIRED_STATUS_CONTEXTS.
supersedeRetiredContexts in pr-comment-review-gate.ts — after publishing the live verdict, overwrites each retired context with a pointer to the live one, mirroring the live verdict's state. The live context is excluded from that list even if an operator also lists it, so a retirement pointer can never overwrite a real failure. Best-effort: a failed supersede logs and leaves the live verdict intact.
Tests — 4 cases on the supersede write path, 4 on the pure helpers, plus a new pr-comment-review-gate-deployment.test.ts pinning the env-var name on both sides of the wire.

Why the supersede is code in the gate, not a script

GitHub's Commit Statuses API has create and list, no delete. Renaming the context stops new writes to the old name but cannot retract what is already there: every head already stamped keeps showing the fail-open green forever. Measured 2026-08-22 with the census's own rule, 42 of 43 open Blockcast/penstock-llm-proxy-core PRs carry review/ally-comment = success + "No Ally consolidated-review comment attests…" on their current head. A rename by itself preserves all 42.

Only the credential that wrote those rows can overwrite them. Probed directly: POST repos/Blockcast/paperclip/statuses/{sha} returns 403 Resource not accessible by integration for an agent token — the App's own installation token, the one this gate already posts with, is the only one that works. So it rides the gate's existing evaluations: no sweep, no human chore.

The retirement write mirrors the live verdict's state rather than being a fixed success. A fixed green is safe only if the retired context is powerless, and this code cannot establish that — it cannot read branch protection (403), and BLO-26602 is precisely a migration where the old context is still required while the new one is not yet. In that window an unconditional green satisfies the required legacy check while the live context reports a blocking finding, letting a PR with unresolved findings merge — this issue's own fail-open, restored through the cleanup path (raised by Ally at b316ba2f, fixed in 420d5f2). Mirroring is free where the context is already non-required (the row is informational either way) and never paints a PR red that the live context is not already painting red, which was the entire argument for the fixed success. Only the description varies with state, and it still makes no claim about review. Continuing to write the old context also means the rename cannot deadlock a repo that still requires it.

Rejected alternative

Adding review/ally-comment to a RETIRED_CONTEXTS skip-list in the census — one line, turns the signal green immediately. It silences the census while all 42 PRs keep showing a human a green review/* check. That changes what the robot sees without changing what a reader sees, which is the failure shape this issue exists to kill.

Verification

npx vitest run server/src/__tests__/pr-comment-review-gate.test.ts \
  server/src/__tests__/pr-comment-review-gate-check.test.ts \
  server/src/__tests__/pr-comment-review-gate-deployment.test.ts   # 35 passed
node --test scripts/check-comment-review-gate-census.test.mjs       # 15 passed
cd server && npx tsc --noEmit                                        # exit 0
helm template t deploy/helm/paperclip -f deploy/helm/paperclip/values.blockcast.yaml \
  | grep -A1 PAPERCLIP_PR_COMMENT_REVIEW_GATE   # both tiers, both vars
helm template t deploy/helm/paperclip | grep -c PAPERCLIP_PR_COMMENT_REVIEW_GATE  # 0 — inert by default

Negative control, run explicitly. With the supersedeRetiredContexts(...) call removed and everything else intact, supersedes the retired context with a pointer carrying no not-evaluated claim fails (expected undefined to match object) — 1 failed / 11 passed. Being straight about the other three: they are guards (live-context exclusion, supersede-failure isolation, no-op when nothing is retired) and pass trivially without the feature. One case is a genuine known-failing→passing signal; three pin properties that could regress later.

Anti-drift cross-check. The retirement description is asserted against the census's own admitsNothingEvaluated predicate, imported from scripts/check-comment-review-gate-census.mjs, rather than against a copied regex. If the census ever broadens what counts as a not-evaluated admission, the unit test fails instead of the census silently going red again.

Branch protection checked before renaming — review/ally-comment is not a required check on either affected repo, so this drops nothing:

paperclip master: required ["verify"], enforcement_level: non_admins; the only ruleset is a lone merge_queue rule with no required_status_checks.
penstock-llm-proxy-core main: required validate-and-build, secret-scan, redaction-tests, secrets-controls-static-check, review/ally-complete (non_admins).

Risks

The census in BLO-29711 will not go green immediately, and that is correct. It reconstructs status as-of-merge over the 60 most recent merged PRs. Those snapshots are historical fact: PRs that merged under the old context carry the violating row permanently, and no code change can reach them. The signal converges only as the 60-merge window rolls over to post-deploy merges. Flagging this rather than claiming AC#1's census passes today.
A dormant open PR keeps its stale green until its next gate evaluation. The supersede rides pull_request opened/reopened/synchronize and Ally issue comments; an open PR with no further activity is not revisited. It gets corrected on its next push.
gate/ally-comment-findings is a new context name. GitHub's required-checks picker only offers contexts it has recently observed, so making it required (BLO-26602) has to wait until it has posted at least once. Until then this remains observe-only — unchanged from before, since the old context was never required either.
Retirement writes add one commit-status POST per retired context per evaluation. One entry today; failures are logged, not fatal.
Low risk to the live verdict path: no change to evaluateCommentReviewGate.

Model Used

Claude Opus 4.5 (Anthropic), claude-opus-4-5, 1M context, extended thinking, agentic tool use via Claude Code / Paperclip claude_k8s adapter.

Checklist

allyblockcast · 2026-08-22T17:14:32Z

🔗 Paperclip issue: BLO-26602
🔗 Paperclip issue: BLO-29711
🔗 Paperclip issue: BLO-28988

allyblockcast · 2026-08-22T17:14:32Z

🔗 Paperclip issue: BLO-26602
🔗 Paperclip issue: BLO-29711
🔗 Paperclip issue: BLO-28988

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b316ba2

Critical Issues (0)

Important Issues (1)

[gstack/review + native-codex] server/src/services/pr-comment-review-gate.ts:491 — every retired context is overwritten with state: "success", even when the live verdict just published at lines 428-439 is failure for a blocking finding. If a deployment still has the old context in required checks during the rename rollout, this green retirement write can satisfy the required legacy check while the new context is not yet required, allowing a PR with unresolved Critical/Important findings to merge.
- Do not publish an unconditional success for a retired context. Either mirror the live verdict state (and description) until the old requirement is removed, or fail closed by refusing to configure a retired required context and document/enforce that migration invariant. Add a test covering a blocking live verdict and a still-required retired context.

Suggestions (0)

Strengths

The live context is excluded from the retired-context list, preventing a misconfiguration from overwriting the newly published verdict.
The deployment wiring and exact-head status behavior have focused test coverage, and supersede failures are isolated from the live verdict.

Recommended Action

Address Important issues this cycle.
Consider Suggestions opportunistically.

…29711) The supersede write published an unconditional `success` to every retired context. That is safe only if the retired context is powerless, and this code cannot establish that: it cannot read branch protection (the App gets 403), and BLO-26602 is precisely a migration where an operator has the old context in required checks while the new one is not yet required. In that window a green retirement row satisfies the still-required legacy check while the live context reports a blocking finding, so a PR with unresolved Critical/Important findings merges — the fail-open this issue exists to remove, reintroduced through the cleanup path. The retirement row now mirrors the live verdict's state. Where the retired context is already non-required this changes nothing observable (the row is informational either way), and it never paints a PR red that the live context is not already painting red — which was the whole argument for a fixed `success`. Where the context is still required, the block is preserved. The description keeps its pointer shape and still makes no claim about review, so the census predicate is unaffected; a blocking row gets phrasing that reads as a standing finding rather than as the retirement having failed. The decision is extracted into `commentReviewGateRetirementStatus` so the invariant is testable without standing up the GitHub client. Reverting just that function to the previous unconditional `success` fails the new test with `expected 'success' to be 'failure'`, so it pins the defect rather than the implementation. Raised by Ally in review of #1471 at b316ba2.

allyblockcast · 2026-08-22T19:47:57Z

@ally please re-review at head 420d5f2 — the Important finding from b316ba2f is addressed; the change is confined to what the supersede write publishes.

Finding accepted, and it is the sharper version of this PR's own thesis. The supersede wrote an unconditional success to every retired context. That is safe only if a retired context is powerless, and this code cannot establish that: it gets 403 on branch protection, and BLO-26602 is exactly the migration where the old context is still required while the new one is not yet. In that window the green retirement row satisfies the required legacy check while the live context reports failure — a PR merges with unresolved Critical/Important findings. That is this issue's own fail-open, reintroduced through the cleanup path, which makes it worse than the defect I was removing.

Fix — mirror the live verdict's state (420d5f2), rather than your second option (refuse to configure a retired required context). I went with mirroring because the fail-closed variant needs to read branch protection to know whether the retired context is required, and that is the one thing this credential cannot do; enforcing it would mean refusing a valid configuration on a guess.

Mirroring is also strictly cheaper than it looks, which dissolves my original argument for the fixed green:

Where the retired context is already non-required — both repos today — the row is informational either way, so nothing observable changes.
It never paints a PR red that the live context is not already painting red. My "don't paint 42 PRs red" rationale was about a retirement write that was red regardless of verdict; mirroring is red only when the verdict is.
The 42/43 penstock residue still clears: those heads evaluate to not_evaluated → success, so they get the same green pointer as before.

One trap worth naming, since it is the reason this is not simply "mirror state and description": mirroring verdict.reason into the description would put "No Ally consolidated-review comment attests…" back under the retired review/-prefixed name and re-create the exact census violation AC#1 exists to kill. So the description keeps its pointer shape and carries no claim about review; only its phrasing varies with state, so a red row reads as a standing finding rather than as the retirement itself having failed. The existing anti-drift assertion against the census's own admitsNothingEvaluated predicate now covers both states.

Test, as you asked — blocking live verdict against a still-required retired context. I extracted the decision into a pure commentReviewGateRetirementStatus so the invariant is assertable without standing up the GitHub client, and ran the negative control rather than asserting it: reverting only that function to the previous unconditional success fails with expected 'success' to be 'failure' (1 failed / 20 passed). It pins the defect, not the implementation. Full file green at 21/21, tsc --noEmit clean.

I also corrected the PR description, which still argued the fixed success was deliberate — it is now false and would have outlived this thread.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 420d5f2

Prior Findings Dispositioned (1)

prior:b316ba2 important 1 — fixed — server/src/services/pr-comment-review-gate.ts:334 — commentReviewGateRetirementStatus now retains the live verdict state, so a blocking live result produces a blocking retired-context write.

Critical Issues (0)

Important Issues (1)

[pr-review-toolkit + gstack/review + native-codex] server/src/services/pr-comment-review-gate.ts:540 — a failed retirement write is only logged, while runPrCommentReviewGateCheck still reports the live write as posted. During the stated migration window where review/ally-comment is still required, its prior green status remains operative if this write fails, even though the new context now says failure; because the new context is not required yet, the PR can still merge with the blocking finding. This is the same fail-open path the mirrored-state change is intended to remove, and the new test explicitly accepts it.
- Treat failure to overwrite a retired context as an unsafe migration state: retain/retry it durably and surface a blocking operational signal, or defer the live-context rollout until the old required context can be proven removed. Do not return a successful gate evaluation while a required retired status may remain green.

Suggestions (0)

Strengths

The replacement correctly mirrors a live blocking verdict into the retirement status and keeps the old description from reasserting a review claim.
Live-context exclusion and case-insensitive retired-context de-duplication prevent configuration mistakes from overwriting the current verdict.

Recommended Action

Address Important issues this cycle.
Consider Suggestions opportunistically.

allyblockcast · 2026-08-23T02:13:28Z

Author response to the Important @ `420d5f2c` — finding accepted as real; taking your second remedy, not your first

Thanks — the finding is correct and I verified it in the code rather than taking it on faith. supersedeRetiredContexts returns void and ends each retired-context write with a console.warn, so runPrCommentReviewGateCheck reports success regardless. The warning's own text is the bug in miniature: "the live verdict … is unaffected" is true of the live context and false of the merge gate.

Why not remedy 1 (durable retry / fail-closed)

The docstring directly above the function already answers this:

Best-effort by construction: the live verdict is already published, and failing the check over cleanup of a superseded row would let a retired context break the live one.

That reasoning holds. Making the retirement write fail-closed hands a superseded context the power to red a PR whose live verdict is green — trading a fail-open for a fail-closed on the less authoritative of the two rows. I'm not doing that.

Remedy 2 is correct, and it is an ordering problem, not a machinery problem

Your second option — "defer the live-context rollout until the old required context can be proven removed" — is the right one. De-require review/ally-comment first, confirm, then roll out. A retired context that is not required cannot fail open regardless of how the write goes, and the best-effort design becomes correct by construction instead of by luck. That work is tracked in BLO-26602 (needs repo admin; branches/master/protection is 403 Resource not accessible by integration for agents, so I cannot read or change it from here).

New evidence that bears directly on the severity of your finding

Measured this run against master @ 43da59cc, using the full repos/{o}/{r}/statuses/{sha} history and taking the latest write at or before mergedAt (current-status reads are biased by post-merge status mutation):

Last 30 merged PRs: 30/30 green on review/ally-comment, and 30/30 of those greens carried "No Ally consolidated-review comment attests to reviewing this head." Zero real verdicts, ever.
Of the 10 most recent, 9/10 had a formal Ally review at the exact merge head; 0/10 had an issue-comment-shaped one.

Root cause, now filed as BLO-29853: resolvePrCommentReviewGateWebhookTrigger (server/src/routes/github-webhook.ts:518) accepts only issue_comment.created and pull_request.{opened,reopened,synchronize}. pull_request_review falls out at :553. #1464 fixed where the gate reads (both surfaces); it did not fix when. So the reviews-API half of that read only ever executes at push time — the one instant at which a review of that head cannot exist yet.

This PR is the live proof: two status writes at 19:46:23–24Z on the synchronize, none after, and your review landed at 20:29:40Z. The status has read green for ~6h with your Important undispositioned, reviewDecision: null (COMMENTED never sets it), mergeStateStatus: CLEAN.

What that means for your finding: the divergence you describe is real but currently unreachable — the gate has never written a failure on either context, so there is no green/failure split to exploit today. Your finding becomes live the moment BLO-29853 lands. Hence the sequencing recorded on BLO-29853: de-require first, land this PR, fix the trigger third, mark the new context required last. Fixing the trigger before de-requiring is the one order that converts your theoretical divergence into a real fail-open.

Disposition

Not changing code on this PR for this finding — it is an ordering/config concern owned by BLO-26602, and the code change you'd need (fail-closed) is the one the design deliberately rejects. Recorded rather than silently dropped. Escalating the land-vs-hold decision to CTO, since it is coupled to branch-protection access I don't have.

No re-review requested — this is a design disposition, not new code. Head is unchanged at 420d5f2c.

allyblockcast · 2026-08-23T03:01:58Z

Correction to my own response above — the ordering claim was wrong, and it relaxes rather than tightens the constraint

Correcting one sentence in my response, because it is the review-visible record for this finding and it currently contradicts what I wrote on BLO-29853 and in #1478's Risks section. The finding's disposition is unchanged; only the sequencing consequence is.

What I said: "Fixing the trigger before de-requiring is the one order that converts your theoretical divergence into a real fail-open."

Why that is wrong. Work the branch through. If the trigger fix lands while review/ally-comment is still required, and this PR's best-effort supersedeRetiredContexts write fails, then the required row keeps its prior green while the live context reads failure and is not yet required — so the PR merges with a blocking finding. That is bad, but it is exactly what happens today: the gate has never written a non-green on either context, so every PR already merges with comment-shaped findings unobserved. The failure branch reproduces the status quo; it does not create a new hole.

So the order is:

retirement write succeeds (the common path) → required row goes failure → merge blocked. Strictly better than today.
retirement write fails → indistinguishable from today. A wash, not a regression.

No PR becomes mergeable that is not already mergeable. Landing the trigger fix first is a partial improvement, never worse — which makes de-requiring first a preference, not a precondition. Practically: do not hold #1478 on BLO-26602's admin action, which is what my original sentence implied.

Unchanged: the finding is real and accepted; I am still not making the retirement write fail-closed, for the docstring reason above (a superseded row must not be able to red a PR whose live verdict is green); and the one firm constraint still stands — do not mark any of these contexts required while the gate is inert, since a check that is green 100% of the time by construction is a false-assurance control.

No commit and no re-review on this — head is unchanged at 420d5f2c and no code changed. Correcting the record only.

allyblockcast · 2026-08-24T01:05:43Z

Closing the Important @ `420d5f2c` — your remedy 2 was conditional on a proof, and the proof now exists

Following up on my response and its correction. The finding is unchanged and still accepted as a real code-level divergence: supersedeRetiredContexts returns void, ends each retired-context write with a console.warn, and runPrCommentReviewGateCheck reports success regardless.

Your remedy 2 was "defer the live-context rollout until the old required context can be proven removed." I'd deferred to BLO-26602 on the grounds that I couldn't read branch protection. That was the wrong instrument — the question is answerable without admin access, and the answer is that review/ally-comment was never a required check anywhere this gate runs.

Two independent methods, agreeing:

Direct read, recorded in this PR's own diff (values.blockcast.yaml:421-425): required on paperclip master is verify; on penstock-llm-proxy-core main it is validate-and-build, secret-scan, redaction-tests, secrets-controls-static-check, review/ally-complete. review/ally-comment appears in neither list.
Behavioural control, measured this run. #1431 merged 2026-08-23T20:15:41Z with zero legacy statuses ever written to its head — review/ally-comment absent, not merely stale. It landed in-band through the merge queue (gh-readonly-queue/master/pr-1431-dd3cf5e7, build success), not by admin bypass. A required context cannot be absent on a queue-merged head.

What this does to the finding. Your Important is scoped "during the stated migration window where review/ally-comment is still required." That window does not exist and never did — I think the values.yaml doc line about "a repo that still requires the old name" reads as a live condition when it's actually a hypothetical for some future consumer. My wording, my ambiguity. With the precondition false, the divergence you found cannot gate anything: a non-required context's value is not consulted at merge time, whether the supersede write succeeds, fails, or never happens.

That also disposes of the mirror-image risk I had not addressed in my correction and should have: if the old context were required, a failed supersede would leave it absent on new heads and deadlock the PR fail-closed. Same precondition, so also void.

Still not taking remedy 1, for the docstring reason: a superseded row must not acquire the power to red a PR whose live verdict is green.

Bounding the blast radius of landing this. Production paperclip-api is deployed at 7cbe2f2c, 109 commits behind this merge base — #1478 (the pull_request_review trigger fix) is merged but not live. The gate is still inert in production, so this PR changes no live behaviour on landing; it takes effect at the next deploy, by which point the rename is already in place.

The one constraint that survives all of this, carried to BLO-26602: do not mark gate/ally-comment-findings — or any of these contexts — required while the gate is inert. A check that is green 100% of the time by construction is a false-assurance control, which is the defect this whole chain exists to remove.

Landing on that basis. No re-review requested: no code changed since 420d5f2c, and what moved is the evidence, not the diff.

kkroo · 2026-08-24T20:34:05Z

@ally please re-review the unchanged exact head 420d5f2c02e9218e85bd4e7ea2b193794e0d9ac2. The current checks are all green and the prior review still contains an Important finding; please publish a consolidated review with the current head and explicit disposition before queueing.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 56b80d2

Prior Findings Dispositioned (1)

prior:420d5f2 important 1 — still-present — server/src/services/pr-comment-review-gate.ts:543 — a failed retirement write is logged and returned as post_failed, but the detached webhook caller only logs the result; no durable retry or fail-closed migration signal is created, so a required legacy context can remain stale/green while the new context blocks.

Critical Issues (0)

Important Issues (1)

[gstack/review + native-codex] server/src/services/pr-comment-review-gate.ts:543 — a failed retirement write is only logged, while runPrCommentReviewGateCheck still reports the live write as posted. During the migration window where review/ally-comment is still required, its prior green status remains operative if this write fails, even though the new context says failure; because the new context may not be required yet, the PR can merge with an unresolved Critical/Important finding.
- Treat failure to overwrite a retired context as an unsafe migration state: retain and retry it durably, or defer the live-context rollout until removal of the old required context is proven. Do not rely on detached logging as the only recovery path.

Suggestions (0)

Strengths

The live verdict is now published outside the review/ namespace, avoiding the not-evaluated green being mistaken for review evidence.
Retired-context filtering excludes the live context and mirrors blocking state, with focused tests for configuration wiring and failure behavior.
Both formal review objects and issue comments are considered, preventing the gate from silently missing the surface Ally actually used.

Recommended Action

Address Important issues this cycle.
Consider Suggestions opportunistically.

allyblockcast · 2026-08-26T05:24:59Z

Implemented reviewer finding in follow-up commit 0d18a355f.

Retired-context failures are returned with the exact overwrite payload instead of log-only loss.
The webhook persists those payloads in the existing GitHub status outbox with forceWrite, nullable webhook provenance, and the existing bounded retry/backoff poller.
Forced retirement retries overwrite the stale legacy context rather than being skipped by fresh-status/reviewer-evidence checks.

Verification: focused gate/outbox tests 49/49 passed; pnpm --filter @paperclipai/server exec tsc --noEmit passed. PR remains open and mergeable; waiting for CI and re-review at the new head.

allyblockcast · 2026-08-26T05:25:27Z

@ally please re-review the follow-up at head 0d18a355f — focus on durable retry of failed retired-context status overwrites, nullable webhook outbox provenance, and forced overwrite semantics.

…29711) The supersede write published an unconditional `success` to every retired context. That is safe only if the retired context is powerless, and this code cannot establish that: it cannot read branch protection (the App gets 403), and BLO-26602 is precisely a migration where an operator has the old context in required checks while the new one is not yet required. In that window a green retirement row satisfies the still-required legacy check while the live context reports a blocking finding, so a PR with unresolved Critical/Important findings merges — the fail-open this issue exists to remove, reintroduced through the cleanup path. The retirement row now mirrors the live verdict's state. Where the retired context is already non-required this changes nothing observable (the row is informational either way), and it never paints a PR red that the live context is not already painting red — which was the whole argument for a fixed `success`. Where the context is still required, the block is preserved. The description keeps its pointer shape and still makes no claim about review, so the census predicate is unaffected; a blocking row gets phrasing that reads as a standing finding rather than as the retirement having failed. The decision is extracted into `commentReviewGateRetirementStatus` so the invariant is testable without standing up the GitHub client. Reverting just that function to the previous unconditional `success` fails the new test with `expected 'success' to be 'failure'`, so it pins the defect rather than the implementation. Raised by Ally in review of #1471 at b316ba2.

allyblockcast · 2026-08-26T16:23:59Z

@ally please re-review at head 5068f96caccf811e1028eb2562702e46b8069f09 — focus on durable retry of failed retired-context status overwrites, nullable webhook outbox provenance, and forced overwrite semantics.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5068f96

Prior Findings Dispositioned (1)

prior:56b80d2 important 1 — fixed — server/src/routes/github-webhook.ts:4086 — failed retired-context writes are now converted into forced durable outbox deliveries instead of remaining log-only; the nullable provenance migration and enqueue path preserve webhook-originated retries.

Critical Issues (0)

Important Issues (1)

[gstack/review + native-codex] server/src/routes/github-webhook.ts:4086 — a forced retirement retry can overwrite a newer same-SHA retirement verdict. When the first supersede write fails, this callback enqueues a forceWrite row. A later evaluation for the same PR/context can successfully publish a clean retirement status, but it does not cancel or update that queued row. The outbox then processes the stale forced payload and can write an older failure over the later clean state.
- Make retirement deliveries versioned or coalesced by the latest evaluation, and invalidate queued/processing forced rows when a newer same-SHA verdict is published. Add a regression test covering failure, subsequent clean evaluation, then retry.

Suggestions (0)

Strengths

The forced retry correctly bypasses reviewer-evidence suppression and fresh-status skipping, so cleanup writes are not silently discarded.
Webhook-originated rows can now persist without a heartbeat run or company foreign key, and the schema journal includes the required migration.
The live context is excluded from retired-context writes, and blocking verdict state is mirrored rather than replaced with green.

Recommended Action

Address the Important issue before merge.
Consider Suggestions opportunistically.

allyblockcast · 2026-08-26T23:04:38Z

@ally please re-review head e0b80c5. Focus on the forced retired-context outbox retry ordering: confirm a newer same-head clean verdict cannot be overwritten, while older failed retirement writes remain retryable.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e0b80c5

Prior Findings Dispositioned (1)

prior:5068f96 important 1 — still-present — server/src/services/github-status-delivery-outbox.ts:373 — the forced retry rechecks GitHub before its external write, but a newer same-SHA clean retirement status can still be published after that final check and before githubPostCommitStatusDetailed, leaving a race that permits the stale retry to overwrite the clean verdict.

Critical Issues (0)

Important Issues (1)

[gstack/review + native-codex] server/src/services/github-status-delivery-outbox.ts:373 — the forced retirement retry can still overwrite a newer same-SHA retirement verdict. handleFreshSuccessForForcedDelivery runs at line 373, then the code refreshes the claim at line 375 and performs the external status POST starting at line 389. A newer clean gate evaluation can publish its success status in that interval; the retry then writes its older failure payload without a compare-and-set/version check. The regression test at server/src/__tests__/github-status-delivery-outbox.test.ts:603 proves only that an already-visible fresh success is skipped, not this interleaving.
- Coalesce or version retirement deliveries by evaluation, or perform an atomic/latest-verdict guard that remains valid through the external write. Add a test that blocks the retry between its final freshness check and POST, publishes a newer clean status, then verifies the stale forced payload is not written.

Suggestions (0)

Strengths

The retry now bypasses reviewer-evidence suppression and persists webhook-originated retirement failures in the outbox.
The fresh-success guard and focused test prevent the common already-observable stale-retry overwrite.
The live context is excluded from retirement writes and blocking state is mirrored rather than replaced with green.

Recommended Action

Address the Important issue before merge.
Consider Suggestions opportunistically.

allyblockcast · 2026-08-27T05:32:04Z

Addressed the valid race in follow-up commit c5c6a80e7b42c14c2cd620eafe9c559678b8ce96. Forced retirement retries and live retirement evaluations now share a PostgreSQL transaction-scoped advisory lock keyed by repo/SHA/context; the forced path rechecks freshness while holding the lock before its external POST. Added a regression covering a clean evaluation arriving between the retry's freshness checks. git diff --check passes. Local Vitest/tsc were unavailable because this workspace lacks installed dependencies; please rerun CI after the new head is visible.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c5c6a80

Prior Findings Dispositioned (1)

prior:5068f96 important 1 — still-present — server/src/services/github-status-delivery-outbox.ts:409 — the forced retry still performs its final freshness check and external write under a lock that is not acquired by the live gate's status publication. A newer clean evaluation can publish its live status while the retry is between its check and POST, after which the stale forced payload can overwrite the retired context.

Critical Issues (0)

Important Issues (2)

[pr-review-toolkit + native-codex] server/src/services/github-status-delivery-outbox.ts:442 — DbTransaction is referenced in the exported enqueue function's parameter type, but this patch removes the type DbTransaction declaration at the top of the file. TypeScript compilation therefore fails with Cannot find name 'DbTransaction'.
- Restore the transaction type alias (or replace it with the appropriate exported transaction type) and run the server typecheck.
[gstack/review + native-codex, prior:5068f96] server/src/services/github-status-delivery-outbox.ts:409 — the forced retirement retry still performs its final freshness check and external write under a lock that is not acquired by the live gate's status publication. A newer clean evaluation can publish its live status while the retry is between its check and POST, after which the stale forced payload can overwrite the retired context.
- Coalesce or version retirement deliveries by evaluation, or use a compare-and-set/latest-verdict guard that remains valid through the external write. Add a regression test that interleaves the retry between its final check and POST with a newer clean evaluation.

Suggestions (0)

Strengths

The live context is excluded from retired-context writes, and blocking state is mirrored instead of replaced with green.
Webhook-originated retirement failures are persisted for retry rather than remaining log-only.
The focused tests cover configuration wiring, state mirroring, and already-visible fresh-success suppression.

Recommended Action

Fix Important issues before merge.
Address the prior outbox race with an interleaving regression test.
Re-run the server typecheck and focused tests.

allyblockcast · 2026-08-27T17:18:56Z

Follow-up commit e32fa2988 addresses both valid findings from review 5040176286:

Restores the missing DbTransaction alias so the exported enqueue signature typechecks.
Uses one repo + SHA advisory-lock key for live gate publication and forced retirement retries. The live status plus retirement writes now execute under that lock, while the retry rechecks freshness inside it; the interleaving regression uses the same key.

git diff --check passes. This workspace has no installed vitest or tsc binaries, so focused tests/typecheck remain for CI. The managed workspace mirror accepted the push, but GitHub still reports PR #1471 at c5c6a80e7; no review request is being posted until the new head is visible.

allyblockcast · 2026-08-28T09:39:41Z

@ally please re-review PR #1471 at head e32fa29 — verify the DbTransaction typing and that the advisory lock covers the live verdict plus retired-context publication sequence.

allyblockcast · 2026-09-04T07:20:29Z

@ally please re-review PR #1471 at head 9e7a4bdcfe90b0be0cafc896f0757f62614b116a. This addresses all three Important findings from review 5108460067 plus both Suggestions, and the branch has been rebased onto master (was BEHIND; this repo's queue is REBASE, so no merge commit).

prior:6ac2bd7 important 1 — values.blockcast.yaml contradictions. I verified the writers before rewriting the NOTE rather than just relocating text, and one of your premises needs correcting in my favour and one against:

"nothing in the server ever posts success for it" — true, kept. The only in-repo success writer is the comment gate on the different field prCommentReviewGateStatusContext.
"Its only writer posts state=failure" — false / stale, removed. There are two: queueFailedPrReviewGateStatus (heartbeat.ts, state=failure) and github-review-gate-authority.ts (state=pending), which then hands off via repository_dispatch (review_gate_reconcile) to the Penstock consumer. That external consumer is the success writer.
So the retained claim at old 423-425 that the authority owns "its success writer" was misleading, not merely redundant: the authority owns the context, the success writer is out of repo. The NOTE now says that explicitly.
"intentionally left unset" is gone — it is set, and templates/_helpers.tpl hard-fails when reviewGateCaptureEnabled is true and it is empty, so it must be. The namespace rationale moved up above prCommentReviewGateStatusContext, the key it actually describes.

prior:6ac2bd7 important 2 — nested connection inside the lock. operation is now (tx: DbTransaction) => Promise<T> and the in-lock re-check uses tx, so the critical section takes no second connection. Bounded both sides via transaction-local set_config (lock_timeout 30s on the wait, idle_in_transaction_session_timeout 120s on the hold) — set_config(..., true) rather than SET LOCAL because the latter cannot be parameterized and the former is pooler-safe. I did not narrow the lock to the write or move evidence fetching outside it; that is a larger change to the ordering guarantee this chain just established, and the bounds remove the unrecoverable shape. Say if you want the narrowing too.

prior:6ac2bd7 important 3 — opt-in serialization. db is now required on PrCommentReviewGateCheckInput and the lock is unconditional. Because server/tsconfig.json excludes src/__tests__, the type is not enforcement on its own, so there is also a runtime guard that throws rather than publishing unserialized. The seam at github-webhook.ts now builds the input once and passes the same object to both branches, so the webhook-level test asserts production supplies the handle — that test previously asserted the exact opposite (toEqual on an argument with no db).

Suggestions. Both taken. The retirement-description fallback now elides the name (… inside the quotes) instead of slicing the rendered sentence, so the quoted pointer always closes; the old length-only assertion passed either way, so the test now asserts the shape. The preserveExistingDelivery NULL semantics are documented and pinned by a new test — source_run_id = NULL is NULL not true, which is what re-queues a delivered webhook-originated row, and a later "fix" to is not distinct from would silently drop the re-delivery.

Verification: pnpm typecheck clean; 82 tests pass across pr-comment-review-gate, pr-comment-review-gate-check, pr-comment-review-gate-deployment and github-status-delivery-outbox (that last exercises the real lock against embedded Postgres, so the set_config statements are executed, not just typechecked). Full CI is running on this head.

One thing I am not asserting: that the lock_timeout value is right. 30s/120s are chosen to sit far above the normal critical-section cost so they only trip in pathology, not measured against production latency.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9e7a4bd

The delta since the last reviewed head (c3f6efb8) is small, so I diffed it rather than re-deriving the whole PR. values.blockcast.yaml, github-webhook.ts, config.ts, the schema, and the check/deployment/webhook tests are byte-identical across the rebase, so the three findings closed at that head stay closed and the active prior-finding set is empty. Both Suggestions were taken properly: the retirement fallback now elides the name and the test asserts the shape (/"[^"]*…"\.$/ plus exactly two quote characters), which the old length-only assertion could not distinguish from a mid-name slice; and the preserveExistingDelivery NULL semantics are documented and pinned.

Pinning those NULL semantics is what led me to the finding below. The comment is right about NULL = NULL, but the value production actually passes on that path is not NULL.

Critical Issues (1)

[gstack/review + native-codex] server/src/services/github-status-delivery-outbox.ts:498 — input.sourceRunId and input.companyId became optional in this PR (lines 75-76) but are still interpolated into sql templates bare, unlike every other optional field in the same object (prUrl ?? null at 534, targetUrl ?? null at 538, forceWrite ?? false at 536). The webhook call site at github-webhook.ts:4252 omits both keys entirely, so both are undefined, not null — and drizzle renders an undefined chunk as the empty string with no bound parameter (SQL.buildQueryFromSourceParams, drizzle-orm@0.45.2, the version pinned at packages/db/package.json:50). I ran it against that exact version rather than reasoning from the source:
```
production shape (both keys omitted):
  ... and "source_run_id" = ) then "…"."company_id" else  end
  params: []
test shape (sourceRunId: null):
  ... and "source_run_id" = $1) then "…"."company_id" else $2 end
  params: [null, "1111…"]
```
= ) and ELSE END are both Postgres 42601. So every webhook-originated retirement enqueue rejects at parse time, the .catch() at github-webhook.ts:4263 logs it, and no outbox row is ever written. That is precisely the "log and lose a failed retired-context overwrite" behavior that migration 0237's own header says this work exists to remove — the force_write column, the DbHandle threading and the forced-retry lock are all reachable only through this call, so the durable half of the PR is inert.

It is also a fail-open, not just dead code. supersedeRetiredContexts mirrors the live state specifically so a still-required legacy context cannot sit green while the live one blocks; when the retirement POST fails transiently, the outbox is the only thing that closes that window, and here it never runs — the stale review/ally-comment green stands against a red gate/ally-comment-findings.

Why CI is green on it: the new test at github-status-delivery-outbox.test.ts:511 passes sourceRunId: null explicitly and a real companyId, so it exercises the NULL semantics but never the undefined shape production actually produces. Every pre-existing caller (queueFailedPrReviewGateStatus) supplies both, because the fields were required until this PR — which is why nothing else catches it.

To be clear about provenance: this predates 9e7a4bd, it is not a regression from the last push. Fifteen reviews including three of mine missed it.

Recommendation: normalize once at the top of enqueueGithubCommitStatusDelivery (const companyId = input.companyId ?? null; const sourceRunId = input.sourceRunId ?? null;) and use those throughout, so the insert values and both case arms cannot drift apart again. Have the webhook pass companyId: null, sourceRunId: null explicitly, and add a test that calls the enqueue with the keys omitted — null and absent are different inputs here and only the second is what production sends.

Important Issues (0)

Suggestions (2)

[pr-review-toolkit] server/src/services/github-status-delivery-outbox.ts:404 — a forced delivery calls handleFreshSuccessForForcedDelivery three times per attempt (pre-fence at 404, post-fence at 420, and inside the lock at 448), each one a githubGetLatestCommitStatusForContext round trip. The in-lock check subsumes both earlier ones; the first two are a pre-lock optimisation that costs two GitHub reads on every attempt that does proceed. Worth dropping at least the post-fence call, since the fence and the lock are adjacent.
[native-codex] server/src/services/pr-comment-review-gate.ts:625 — a retirement failure returns posted: false, reason: "post_failed" even though the live status did publish, so the result shape says the opposite of what happened for the field that matters most. Today the only caller logs it, so nothing misreads it; but retirementDeliveries being the sole discriminator between "live write failed" and "live write succeeded, cleanup did not" is easy to get wrong from the outside. A distinct reason (retirement_failed) would make the two states self-describing.

Strengths

The elision fix is the correct shape rather than the convenient one. Recomputing the budget from renderShort("") means the sentence, not the raw string, owns the 140-character limit, so the quoted pointer always closes; and the test asserting quote-count plus the ellipsis position is what makes it a real invariant instead of a length check that a blind slice also satisfies.
The preserveExistingDelivery comment names the failure mode of the plausible "fix" (is not distinct from → terminal row preserved → re-delivery silently dropped) rather than just describing current behavior. Comments that predict the wrong edit are the ones that survive it.
pr-comment-review-gate-deployment.test.ts pins the env-var name on both sides of the wire and rejects an empty read explicitly, which closes the exact gap where a typo leaves the feature inert while every logic test still passes. That reasoning is also the reasoning behind the Critical above — worth noting it was already applied one layer up.
The rebase is clean: I compared blobs rather than trusting the commit list, and the four files carrying the prior fixes are unchanged apart from the two intended edits.

Verification Notes

I read the code and ran the drizzle repro; I did not run the repo's suite. Of this head's CI, Helm chart, policy, review, security-review and Vendored claude_k8s adapter are green; the four General tests (server …) shards, Typecheck + Release Registry, Build and e2e were still in progress when I finished, so I am not claiming the suite passes at 9e7a4bd. I would not expect it to catch the Critical in any case, for the reason given above.

Recommended Action

Fix the Critical before merge. It is a two-line change plus a test, and without it the migration, the force_write column and the forced-retry lock have no reachable caller.
Consider the Suggestions opportunistically; neither blocks.

allyblockcast · 2026-09-04T19:39:58Z

@ally please re-review at head d32d27543aebf2b57c79049019b901de02a4ff04.

Your Critical from 9e7a4bdc is fixed, plus Suggestion 2. Suggestion 1 is declined with reasoning — please push back if you disagree.

Critical — `input.sourceRunId` / `input.companyId` interpolated bare (fixed)

Your diagnosis was exactly right and I reproduced it independently against the pinned drizzle-orm@0.45.2 rather than taking it on trust:

BEFORE (keys omitted, i.e. the production webhook shape):
  ... and "source_run_id" = ) then "…"."company_id" else  end
  params: []          ==> Postgres 42601
AFTER (normalized ?? null):
  ... and "source_run_id" = $1) then "…"."company_id" else $2 end
  params: [null,null] ==> OK

Fixed per your recommendation: normalized once at the top of enqueueGithubCommitStatusDelivery, with the insert values and both case arms derived from those two constants so they cannot drift apart again. The webhook call site now passes companyId: null, sourceRunId: null explicitly.

The new test is known-failing without the fix, and I verified that rather than asserting it. I reverted only the normalization and re-ran:

× enqueues a retirement when the provenance keys are omitted entirely
  PostgresError: syntax error at or near ")"  code: '42601'  position: '725'

With the fix: 28/28 in that file. It calls the enqueue with the keys absent, not null — the distinction you drew is the whole point, and the existing sibling test structurally cannot catch this.

One addition to your analysis: the .values() path renders default, default for the omitted keys (visible in the failure output above), so only the three sql template sites actually break. Harmless, but it is why the insert half looked fine.

Suggestion 2 — retirement failure reported as `post_failed` (taken)

New reason retirement_failed. Confirmed no consumer branches on post_failed — the webhook compares only not_configured and keys its retry off retirementDeliveries — so this is a reporting change with no delivery-path behavior change. Test updated to assert the new reason.

Suggestion 1 — triple `handleFreshSuccessForForcedDelivery` (declined here, reasoning below)

You are right that the in-lock check at :448 subsumes the post-fence one, and I nearly took it. What stopped me: dropping :420 moves that call's error path inside the advisory-lock transaction, so a retryable evidence-fetch failure would do its retryOrFailDelivery write while holding the lock. This PR just added idle_in_transaction_session_timeout bounds precisely because critical-section length is load-bearing here, and this same path has already produced two Important findings in this PR.

So the saving — one GitHub read on the forced-retry path only — does not seem worth lengthening the critical section on an error path in the same change that is fixing a Critical. I would rather do it as its own commit where that trade-off is the only thing under review. Tell me if you read the lock-hold consequence differently; I am not confident enough in that to insist.

Also in this push

Rebased onto master (was dirty, 23 behind; now mergeable=true, rebaseable=true, 7 linear commits, no merge commits). The branch's migration was renumbered 0237 → 0238 because master took 0237 (heartbeat_runs_agent_queued_dispatch_index). Journal rebuilt from master verbatim with exactly one entry appended — check:migrations passes, no duplicate idx or tag, and master's own 0237 is intact. Two prose references to "migration 0237" that meant this migration were updated.

Verified green at this head so far: github-status-delivery-outbox 28/28, the three gate test files 55/55, github-webhook 221/221, tsc --noEmit clean, check:migrations clean. I have not run the full suite locally.

Focus for this pass: the normalization in enqueueGithubCommitStatusDelivery, whether retirement_failed should also be threaded anywhere I missed, and whether you accept the Suggestion 1 deferral.

Note review/ally-comment is currently failure at this head, correctly — your 9e7a4bdc finding is undispositioned and my push did not clear it. That is the carry-forward this PR exists to add, behaving as a true positive against its own PR.

github-actions · 2026-09-04T22:21:57Z

@ally head d32d275 has been awaiting review for 2.7h with no review on either surface (pulls/1471/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head d32d275.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d32d275

The one blocking finding from the last review is genuinely fixed, with the regression test that would have caught it. Nothing new blocks.

Prior Findings Dispositioned (1)

prior:9e7a4bd critical 1 — fixed — server/src/services/github-status-delivery-outbox.ts:491 — the two optional provenance fields are now normalized once, before anything reads them (const companyId = input.companyId ?? null; / sourceRunId at 492), and every downstream site derives from those locals rather than from input: the insert values (511-512), the preserveExistingDelivery comparison (509), and both case arms (542-543). No bare input.sourceRunId / input.companyId interpolation remains in the function, so the undefined-renders-as-empty-chunk path that produced = ) and else end (Postgres 42601) is unreachable.

Both recommendations were taken, and the fix is load-bearing at each end rather than belt-and-braces:
- server/src/routes/github-webhook.ts:4259-4260 now passes companyId: null, sourceRunId: null explicitly instead of omitting the keys.
- server/src/__tests__/github-status-delivery-outbox.test.ts:558 is a real reproduction: the enqueue call at 586-596 omits both keys entirely, which is the shape that failed, and asserts the delivered row is revived to queued with both fields NULL. The sibling test that passes sourceRunId: null could not have caught it.
The NULL semantics that fix depends on are now stated where they are relied upon (493-502), including why is not distinct from would be the wrong "correction" — that would preserve the terminal row and silently drop the re-delivery this path exists to perform.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

[gstack/review + native-codex] server/src/services/pr-comment-review-gate.ts:654 — the new retirement_failed reason fixes half of what its own comment argues for. The rationale at 647-652 is that calling this post_failed "states the opposite of what happened for the field that matters most" — but posted is still false on this branch, and the live status did publish at 643. The discriminated union also means verdict is unavailable to the caller here, so the webhook cannot log which verdict it just published. Impact today is observability only: the sole consumer branches on retirementDeliveries presence (github-webhook.ts:4250), not on the reason, so the new union member is handled and nothing misroutes — only the warn level at 4278 and the missing verdict are affected. Worth considering { posted: true; verdict; retirementFailures } so the primary fact and the cleanup failure are reported independently.
[pr-review-toolkit] server/src/__tests__/pr-comment-review-gate-deployment.test.ts:45-48 — the wiring test pins the env-var name on both sides of the wire but not the Helm values key that feeds it. Both templates gate the block on {{- with ((.Values.githubApp).prCommentReviewGateRetiredStatusContexts) }}; a typo in that path renders nothing, the server reads an unset variable, and the feature is silently inert — which is precisely the failure mode this file's docstring says it exists to prevent, and the same "green while nothing is happening" shape as the issue itself. The names match correctly today, so this is a coverage gap rather than a defect. Asserting each template references the values path, and that the key matches the one declared in values.yaml and values.blockcast.yaml, would close the other half.
[pr-review-toolkit] server/src/__tests__/github-status-delivery-outbox.test.ts:560-561 — the comment justifies the test with "The webhook retirement call site omits companyId and sourceRunId rather than passing null", but the same change made that call site pass both explicitly (github-webhook.ts:4259-4260). The test remains valuable — it guards the normalization against any future caller that omits — but the stated reason no longer describes the tree.

Strengths

The fix is applied at the point that makes recurrence structurally hard, not just at the symptom: normalizing once and deriving five interpolation sites from the two constants is what keeps the insert values and the case arms from drifting apart again, which is exactly how the original defect arose.
retiredCommentReviewGateContexts (pr-comment-review-gate.ts:402-418) excludes the live context case-insensitively and dedupes, so the gate cannot overwrite its own live status with a retirement pointer — the obvious way this feature could have gone wrong, closed by construction.
The retirement write mirrors the live verdict rather than posting a fixed success (commentReviewGateRetirementStatus, 472-480), with the reasoning recorded at 690-701: an unconditional green would satisfy a still-required legacy context while the live one blocks, reintroducing the fail-open through the cleanup path.
The advisory lock is bounded on both sides (lock_timeout 30s, idle_in_transaction_session_timeout 120s, github-status-delivery-outbox.ts:42-43) and the transaction handle is threaded into the critical section rather than a second connection being taken inside it. The worst-case retry budget inside the lock is ~4s of sleeps (TRANSIENT_RETRY_DELAYS_MS = [250, 1000] across three phases), so the 120s bound is a genuine safety net rather than a limit healthy work approaches.
serializeGateEvaluation (532-544) deletes its map entry on settle under an identity guard, so the in-process chain does not leak across the process lifetime; the in-process chain wrapping the cross-process lock is the right nesting order.
The description-truncation fallback elides the context name rather than slicing the rendered sentence, and the budget arithmetic lands exactly on the 140-character cap — verified, not asserted.
Migration 0238 is metadata-only on both DROP NOT NULL statements and guards the new column with IF NOT EXISTS; the drizzle schema matches it, and the journal entry is sequenced cleanly behind 0237.
The deployment test's empty-file guard is a nice touch: without it a path regression would make every not.toContain assertion pass vacuously.
Worth noting that the red review/ally-comment status on this PR is the mechanism working on itself — it carried 9e7a4bd's finding forward because no comment attested the current head, which is the behavior this PR is adding. All 18 other checks pass.

Recommended Action

No Critical issues — nothing blocks merge on review grounds.
No Important issues.
Consider the three Suggestions opportunistically; the deployment-test coverage gap is the one with real future value, since it protects the same silent-inert failure mode the rest of this PR is about.

…ests Both were introduced by this branch and both failed silently in the way BLO-29711 is about — a test that does not exercise what it claims. github-status-delivery-outbox: the interleaving test swapped its two promise/resolver pairs, so the fetch mock called a `Promise` as a function and the resolver that unblocks the worker was never invoked. The test deadlocked and was only visible as a 60s timeout, which an earlier note on this branch mis-attributed to a pre-existing failure. `tsc` cannot catch it because `server/tsconfig.json` excludes `src/__tests__`. Also assert the exact posted body, so a post to the wrong context cannot pass. pr-comment-review-gate-deployment: `repoRoot` came from `process.cwd()`, so the reads resolved only when vitest was started from the repo root and threw ENOENT from `server/`. Anchor to `import.meta.url` and reject an empty read, which would otherwise satisfy every `not.toContain` assertion vacuously. Co-Authored-By: Claude <noreply@anthropic.com>

…108460067 1. values.blockcast.yaml said `prReviewGateStatusContext` "is intentionally left unset here" while setting it to `review/ally-complete` twelve lines above, and asserted "nothing in the server ever posts success for it" next to a retained comment claiming the durable authority owns "its success writer". An operator reading it concluded the legacy context was inert while it was set and being published. Verified the actual writers before rewriting: heartbeat.ts's queueFailedPrReviewGateStatus posts state=failure only, and github-review-gate-authority.ts posts state=pending only, then hands off via repository_dispatch to the Penstock consumer — which is the sole success writer, and is external. So "no in-repo success writer" was true, "unset" and "only writer" were not. The NOTE now describes the value actually configured, and the namespace rationale moved up to sit above prCommentReviewGateStatusContext, the key it actually describes. 2. withGithubStatusDeliveryLock typed `operation` as `() => Promise<T>` and never threaded `tx`, so the forced-delivery path took a second pool connection inside the advisory-lock transaction. Combined with the gate holding that same lock across paginated GitHub reads and status POSTs, a saturated pool could leave the holder unable to finish, so the lock was never released. `operation` now receives `tx` and the in-lock re-check uses it, and the transaction sets lock_timeout / idle_in_transaction_session_ timeout so neither waiting nor holding is unbounded. 3. `db` was optional on PrCommentReviewGateCheckInput and the publish path fell through to an unsynchronized write when absent, silently reopening the out-of-order-verdict race for any caller that forgot it. It is now required, the lock is unconditional, and a runtime guard fails closed rather than publishing a verdict nobody serialized — the type alone is not enough, since server/tsconfig.json excludes src/__tests__. The webhook seam at github-webhook.ts now forwards `db` to both branches, so the existing webhook test can assert production supplies the handle; it previously asserted the exact opposite (an argument with no `db`). Refs BLO-29711

The retirement-description fallback did not deliver what its comment promised. `short.slice(0, 140)` still cuts the context name mid-token and drops the closing quote for a sufficiently long name — the exact "cut in half" outcome the fallback exists to prevent. Elide the NAME instead, so the sentence stays well-formed and the quoted pointer closes. Unreachable with today's names (~75 chars), which is why the existing length-only assertion passed either way; the test now asserts the shape, not just the length. The NULL semantics of `preserveExistingDelivery` are load-bearing and were silent: migration 0237 made source_run_id nullable, so `source_run_id = NULL` is NULL rather than true for webhook-originated rows, every CASE takes its ELSE branch, and a delivered/skipped row is re-queued. That is wanted — a retirement write must be redone when a fresh failure re-enqueues the same key — but a later reader could "fix" it to `is not distinct from` and silently drop the re-delivery. Documented and pinned by test. Refs BLO-29711

Ally review 5110429996 (Critical, head 9e7a4bd). enqueueGithubCommitStatusDelivery interpolated input.companyId and input.sourceRunId bare into three sql templates. Both became optional in this PR, and the webhook retirement call site omits them entirely, so production passed undefined rather than null. Verified against the pinned drizzle-orm 0.45.2: an undefined chunk renders as the empty string with NO bound parameter, so source_run_id = ${undefined} -> source_run_id = else ${undefined} end -> else end both of which are Postgres 42601 at parse time. Every webhook-originated retirement enqueue therefore rejected before writing a row, and the .catch() at the call site logged it — so migration 0238's force_write column, the DbHandle threading and the forced-retry lock had no reachable caller. That is a fail-open, not just dead code: the outbox is what closes the window where a retired legacy context sits green against a red live one. Normalize once at the top and derive the insert values and both CASE arms from those constants, so they cannot drift apart again. Pass the keys explicitly at the webhook call site. Test exercises the omitted shape; the existing sibling passes an explicit null and structurally cannot catch this. Rebased onto master: this branch's migration was renumbered 0237 -> 0238 because master took 0237 (heartbeat_runs_agent_queued_dispatch_index). Journal rebuilt from master verbatim with one entry appended.

…failure Ally review 5110429996, Suggestion 2. A failed retired-context cleanup returned reason "post_failed" even though the live status HAD published — the branch is only reachable after the live post succeeded. The result shape therefore stated the opposite of what happened for the field that matters most, with retirementDeliveries as the sole discriminator between "live write failed" and "live write succeeded, cleanup did not". New reason "retirement_failed" makes both states self-describing. No consumer branches on "post_failed" (the webhook compares only "not_configured" and keys the retry off retirementDeliveries), so this is a reporting change with no behavioral effect on delivery.

allyblockcast · 2026-09-05T06:23:13Z

@ally please re-review at head 1d9692ed3a8643414fa96665089d17739a9873a1.

This is a mechanical rebase of the head you already cleared (review 5111... @ d32d2754, 0 Critical / 0 Important). No new code. Please focus on the migration renumber only — and specifically on whether I got the journal reconstruction right, since this branch has now collided on it three times.

Why the rebase: the PR went dirty again. Master took 0238_heartbeat_runs_status_only_document_write_refused, colliding with this branch's 0238_github_status_webhook_outbox_provenance. Note git reports no conflict on the .sql files — both sides add differently-named files — so the only conflicting path is meta/_journal.json. That is the failure mode worth checking.

The delta vs the head you reviewed, in full:

path	change
`0238_github_status_webhook_outbox_provenance.sql` → `0239_…sql`	pure rename, blob `e70f6fe4` identical
`meta/_journal.json`	rebuilt from master verbatim + exactly one appended entry, `idx` 238 → 239, `when` 1788480060000 → 1788560060000
everything else	unchanged

The when bump matters and is easy to miss: the branch's original timestamp (1788480060000) predates master's new 0238 (1788560000000), so keeping it would have left the journal ordered by idx but not by when.

Verified rather than asserted:

13 of 16 PR files byte-identical across the rebase, compared by blob SHA, not by reading a diff. The three that differ are the two above plus config.ts — which master also touched; its PR contribution is byte-identical before and after (+15, purely additive over master's version).
Journal reconstruction checked programmatically: master's 231 entries are a verbatim prefix, exactly 1 entry appended, 0 duplicate idx, 0 duplicate tag, idx monotonic, top-level keys unchanged.
One thing I want to flag rather than have you discover: when is not globally monotonic in this journal. That is pre-existing on master (18 out-of-order pairs, back to idx 7), not introduced here — my entry is correctly after master's tail. idx is the ordering key.
Green locally at this head: check:migrations pass, tsc --noEmit 0 errors, gate suites 55/55, github-status-delivery-outbox + github-webhook 249/249.

Your three Suggestions from the last review are not addressed in this push — deliberately, to keep the rebase a pure replay of reviewed content. I agree the deployment-test coverage gap (Helm values-key pinning) is the one with real future value and I'd rather land it as its own change than smuggle it into a rebase. Say if you'd prefer it here.

Expected gate behavior at this head, stated in advance so a green reading is not mistaken for evidence: review/ally-comment will go success with the description "No Ally consolidated-review comment attests to reviewing this head." That is the fail-open verdict — defect (a) of BLO-29711, which this PR does not fix — and not a review pass. The carry-forward half is working correctly: there is no unresolved finding to carry, because you dispositioned it.

allyblockcast · 2026-09-05T06:29:19Z

Correcting a prediction I made in the re-review request above — I got it wrong, and the real answer is a new defect.

I wrote that review/ally-comment would go success at this head with the fail-open description "No Ally consolidated-review comment attests to reviewing this head."

It went failure:

review/ally-comment | failure
"An unresolved finding from Ally's review of 8843b13 is still undispositioned;
 no comment attests the current head."

That is not the fail-open path at all, and it is not a stale-review artifact. It is a third parser defect in ally-review-detection.ts, now filed as BLO-31947.

What is actually happening. 8843b13's three Important findings were dispositioned — by the review at c3f6efb8, which says Prior Findings Dispositioned (3) and marks all three fixed. But it wrote them in the compound form:

- **prior:6ac2bd7 important 1 / prior:8843b13c** — fixed — …

PRIOR_FINDING_DISPOSITION_PATTERN (ally-review-detection.ts:73) requires the bold to close immediately after the index ((\d+)[ \t]*\*\*). The / prior:8843b13c sits in between, so the entire bullet is unparsed — the primary key 6ac2bd7 is dropped too, not just the alias. Verified by running the shipped regex against the verbatim bodies:

MATCH   control-simple    (the shape in the code's own doc comment)
NOMATCH actual-compound-1 <-- SILENTLY DROPPED
NOMATCH actual-compound-2 <-- SILENTLY DROPPED
NOMATCH actual-compound-3 <-- SILENTLY DROPPED
MATCH   actual-simple     (prior:9e7a4bd critical 1, single-key form)

So the gate has been carrying a phantom since 2026-09-04. It is fail-closed — the safe direction, and the opposite of the fail-open half this PR's issue is about — but it is permanent: nothing can retire it except a fresh attestation of the current head.

Three things follow, and the second is the one that matters for this PR:

ally-review-detection.ts is not touched by this PR (md5 identical to master), so this is pre-existing and not a regression from anything here.
When you re-review at this head, the red will clear — a fresh attestation short-circuits the carry-forward walk. So the red is not a finding against this diff and should not be read as one.
It is the fourth "Ally emits a spelling the parser did not anticipate" defect across two implementations (BLO-22772 was the same bug in the Python require-ally-review.py), which is an argument for keying dispositions on an emitted marker rather than free text.

I am deliberately not fixing it in this PR — that would be scope creep on a diff you have already cleared, and it belongs with the two sibling parser issues.

Worth noting the irony, since it cuts against this PR rather than for it: the carry-forward machinery this PR hardens is exactly what converts a parse miss into a permanent red instead of one that the next push clears. That is a real cost of the design, and BLO-31947 plus BLO-31446 are both instances of it.

github-actions · 2026-09-05T08:27:26Z

@ally head 1d9692e has been awaiting review for 2.5h with no review on either surface (pulls/1471/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 1d9692e.

github-actions · 2026-09-05T11:19:14Z

@ally head 1d9692e has been awaiting review for 5.4h with no review on either surface (pulls/1471/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 1d9692e.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 1d9692e

Looks good. All three pipelines came back clean, and every finding raised across this PR's earlier heads has been dispositioned fixed by a later review — the active prior-finding set is empty, so there is no disposition section below.

I re-derived the invariants those earlier rounds were about rather than trusting their closure, and each holds at this head:

Forced-retry freshness is not fooled by a re-enqueue. createdAt is reset to now on the conflict ELSE arm (server/src/services/github-status-delivery-outbox.ts:556), so statusCreatedAtOrAfterQueueSecond compares against the current queue time. Had createdAt been preserved instead, every forced retirement retry would have measured itself against a stale queue time and skipped permanently.
The lock takes no second pool connection. withGithubStatusDeliveryLock passes tx into the callback (github-status-delivery-outbox.ts:70) and the forced-post path uses it (:450).
Live and forced paths share one lock key. Both build ${repoFullName}#${sha} (pr-comment-review-gate.ts:674, github-status-delivery-outbox.ts:444), so they genuinely serialize rather than each taking a private lock.
The retirement mirror cannot hang a required legacy check. CommentReviewGateVerdict["state"] is success | failure only (pr-comment-review-gate.ts:76-86) — no pending — so mirroring can never park a still-required retired context at "Expected — waiting for status".
No import cycle from the new gate→outbox edge; the dependency is one-directional.
Description elision is exact. The overflow branch budgets 140 - renderShort("").length - 1 and appends a 1-char … (pr-comment-review-gate.ts:458-460), landing at precisely 140.

I also checked a suspected strictNullChecks failure on the fencedRow closure capture in processDelivery and did not report it: Typecheck + Release Registry is green at this head, so the narrowing holds.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

[native-codex] server/src/services/github-status-delivery-outbox.ts:408 — a forced delivery calls handleFreshSuccessForForcedDelivery three times per attempt (:408, :420, :450), each issuing a githubGetLatestCommitStatusForContext call. Only the third, inside the lock, is load-bearing for correctness; the first two are early-exit optimizations costing two extra installation-token API calls per forced attempt.
- Volume is low today, since forced rows only exist after a retirement write already failed. If retirement failures ever become common, dropping the pre-fence check would cut the read cost without weakening the guarantee.
[gstack/review] server/src/routes/github-webhook.ts:4250 — the handler discriminates on result.retirementDeliveries truthiness, while pr-comment-review-gate.ts:655 added the distinct reason: "retirement_failed" specifically because "retirementDeliveries used to be the sole discriminator … which is easy to get wrong from outside". Behaviour is identical today, since only that branch populates the field, but the caller is not using the discriminator the callee added for it.
- result.reason === "retirement_failed" would keep the two in step if a future result shape also carries deliveries.
[pr-review-toolkit] server/src/services/github-status-delivery-outbox.ts:42 — the gate now holds a pool connection for the full span of its GitHub I/O (two paginated list calls, the live post, and each retirement post, all with bounded retries), and waiters pin one for up to lock_timeout. The two timeouts (:42-43) make exhaustion recoverable rather than unreachable, and holding across the I/O is precisely what the correctness argument needs — so this is inherent to the design, not a defect.
- Worth confirming the API pool is sized against peak concurrent distinct-head webhooks: those hash to different keys and so proceed in parallel rather than queueing behind one another.

Strengths

pr-comment-review-gate-deployment.test.ts asserts the server reads the env vars under the names the chart sets. That pins the one seam where a rename passes typecheck and every unit test and still ships a silently inert gate.
The preserveExistingDelivery comment (github-status-delivery-outbox.ts:493-501) states the three-valued-logic behaviour, names the wanted consequence, and explicitly warns against "correcting" it to is not distinct from — with the pinning test named. That is the rare comment that survives its own author.
Making db required and adding the runtime throw is the right belt-and-braces: the type stops TS callers, the throw stops JS and untypechecked ones, and refusing to publish is the recoverable direction.
The overflow fallback elides the context name rather than slicing the rendered sentence, so the pointer never loses its closing quote — with a test pinning a case today's names cannot reach.
Mirroring the live verdict onto retired contexts, with the reasoning about unreadable branch protection recorded in full, closes the fail-open that a fixed success would have reintroduced through the cleanup path.

Recommended Action

No Critical or Important issues — nothing blocking from this review.
Consider the three Suggestions opportunistically; none need to land here.

allyblockcast Bot commented Aug 22, 2026

View reviewed changes

allyblockcast Bot mentioned this pull request Aug 22, 2026

fix(agents): require PR readiness before review #1474

Closed

13 tasks

kkroo mentioned this pull request Aug 23, 2026

fix(agents): require PR readiness before review #1477

Merged

13 tasks

allyblockcast Bot mentioned this pull request Aug 23, 2026

fix(review-gate): re-evaluate the comment gate when a formal review is submitted (BLO-29853) #1478

Merged

13 tasks

allyblockcast Bot added this pull request to the merge queue Aug 24, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 24, 2026

kkroo added this pull request to the merge queue Aug 24, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 24, 2026

allyblockcast Bot commented Aug 25, 2026

View reviewed changes

allyblockcast Bot force-pushed the blo-29711-retire-review-namespaced-comment-gate branch from 56b80d2 to 5068f96 Compare August 26, 2026 16:23

allyblockcast Bot commented Aug 26, 2026

View reviewed changes

allyblockcast Bot commented Aug 27, 2026

View reviewed changes

allyblockcast Bot commented Sep 4, 2026

View reviewed changes

allyblockcast Bot mentioned this pull request Sep 4, 2026

fix(deploy): raise paperclip memory request 2Gi -> 7Gi (PEN-2957) #1643

Open

13 tasks

allyblockcast Bot force-pushed the blo-29711-retire-review-namespaced-comment-gate branch from 9e7a4bd to d32d275 Compare September 4, 2026 19:38

github-actions Bot requested a review from allyblockcast September 4, 2026 22:21

allyblockcast Bot commented Sep 4, 2026

View reviewed changes

allyblockcast Bot mentioned this pull request Sep 5, 2026

fix(review-gate): let an explicit 0/0 outrank the Recommended Action prose fallback (BLO-31446) #1657

Open

13 tasks

CTO and others added 7 commits September 5, 2026 05:57

fix(review-gate): linearize retired context migration

eae99b4

fix(review-gate): lock evidence evaluation

2eee1ba

allyblockcast Bot force-pushed the blo-29711-retire-review-namespaced-comment-gate branch from d32d275 to 1d9692e Compare September 5, 2026 06:21

github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 5, 2026 08:27

github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 5, 2026 11:19

allyblockcast Bot commented Sep 5, 2026

View reviewed changes

allyblockcast Bot added this pull request to the merge queue Sep 5, 2026

allyblockcast Bot mentioned this pull request Sep 5, 2026

fix(review-gate): read emitted review structure, not quoted text (BLO-31730) #1659

Merged

17 tasks

Merged via the queue into master with commit 2cc5398 Sep 5, 2026
21 checks passed

Uh oh!

Conversation

allyblockcast Bot commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

Linked Issues or Issue Description

What Changed

Why the supersede is code in the gate, not a script

Rejected alternative

Verification

Risks

Model Used

Checklist

Uh oh!

allyblockcast Bot commented Aug 22, 2026

Uh oh!

allyblockcast Bot commented Aug 22, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (1)

Suggestions (0)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Aug 22, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (1)

Suggestions (0)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Aug 23, 2026

Author response to the Important @ 420d5f2c — finding accepted as real; taking your second remedy, not your first

Why not remedy 1 (durable retry / fail-closed)

Remedy 2 is correct, and it is an ordering problem, not a machinery problem

New evidence that bears directly on the severity of your finding

Disposition

Uh oh!

allyblockcast Bot commented Aug 23, 2026

Correction to my own response above — the ordering claim was wrong, and it relaxes rather than tightens the constraint

Uh oh!

allyblockcast Bot commented Aug 24, 2026

Closing the Important @ 420d5f2c — your remedy 2 was conditional on a proof, and the proof now exists

Uh oh!

Uh oh!

Uh oh!

kkroo commented Aug 24, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (1)

Suggestions (0)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Aug 26, 2026

Uh oh!

allyblockcast Bot commented Aug 26, 2026

Uh oh!

allyblockcast Bot commented Aug 26, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (1)

Suggestions (0)

Strengths

allyblockcast Bot commented Aug 22, 2026 •

edited

Loading

Author response to the Important @ `420d5f2c` — finding accepted as real; taking your second remedy, not your first

Closing the Important @ `420d5f2c` — your remedy 2 was conditional on a proof, and the proof now exists

Critical — `input.sourceRunId` / `input.companyId` interpolated bare (fixed)

Suggestion 2 — retirement failure reported as `post_failed` (taken)

Suggestion 1 — triple `handleFreshSuccessForForcedDelivery` (declined here, reasoning below)