feat(approvals): close approval cards whose GitHub gate has terminated (BLO-29359) by allyblockcast[bot] · Pull Request #1453 · Blockcast/paperclip

allyblockcast · 2026-08-20T21:50:35Z

Thinking Path

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

Approvals are the control plane's human-in-the-loop gate: an agent that cannot proceed alone files a board card and waits

But when the thing needing a human is external — a paperclip-production GitHub Actions deploy gate — the card is only a pointer at that gate, and nothing reconciled the two

So cards outlive their runs. BLO-29359 recorded three gates dying unclicked inside 24h, one card sending approvers to a cancelled run for ~19h. An approver who opened the only card describing a ~$1,000/day burn reached a corpse with no way to tell that was what had happened — silence was the failure mode

Worse, cards could not be reconciled even in principle: the run appeared only as prose in payload.title / summary, so no code could ask "is this gate still alive?"

This pull request adds the machine-readable half (payload.gate) and the worker-tier sweep that uses it: close the card once its run is terminal, and say so on every linked issue

The benefit is that a dead gate stops being silent — the queue drains itself, and the audit trail records why a card stopped being actionable

Linked Issues or Issue Description

Refs BLO-29359 — addresses the "no stale card outlives its run" and "death is announced" acceptance criteria, and the attribution half of "durability or attribution"
Refs BLO-28920 — the burn whose escalation cards exposed this
Related, not duplicated: Add approval-enforcement reconciler: detect approved decisions that never reached the enforcing object (BLO-24631) #1309 (approval-enforcement reconciler — approved decisions that never reached the enforcing object; the opposite direction to this one) and fix(approvals): terminal exit for revision_requested that preserves the board's decision note, and wake the requester on every decision (BLO-27036) #1388 (terminal exit for revision_requested; this PR also treats revision_requested as undecided/closable)

What Changed

packages/shared — approvalGateSchema: an optional, validated payload.gate of {kind: "github_actions_run", repoFullName, runId, url?}. Plus parseApprovalGate(), deliberately total, for reading persisted jsonb written before the key was validated.
server/src/services/github-app-auth.ts — githubGetWorkflowRun() returning a three-way found | not_found | error, so a caller structurally cannot collapse "the run is gone" into "I could not read the run". Follows the existing githubGetPullRequestGate idiom (installation token, ghFetch, classifyGithubHttpFailure).
server/src/services/approval-gate-reconciler.ts (new) — worker-tier periodic sweep. For each undecided card carrying a gate: if the run is terminal, close the card as cancelled and comment on every linked issue naming the run and its conclusion. Injectable scheduler / fetchRun / now seams; re-entrancy guard; bounded batches with a keyset cursor.
server/src/config.ts + server/src/index.ts — PAPERCLIP_APPROVAL_GATE_RECONCILER_ENABLED (default on) and ..._INTERVAL_MINUTES (default 10), registered under paperclipNodeRole !== "api" with a lazy import, mirroring the stranded-blocked-issue reconciler. Skipped with a warning when GitHub App credentials are absent, since every lookup would otherwise defer forever.
packages/mcp-server — paperclipCreateApproval's description now tells agents to set payload.gate when the ask is a GitHub gate. The schema already advertises the field; the description supplies the when.
Tests — 20 new cases (server/src/__tests__/approval-gate-reconciler.test.ts), embedded-Postgres backed, plus a driver-free classifyGateLookup suite so the fail-safe direction is asserted even on hosts that skip the DB suite.

Verification

npx vitest run server/src/__tests__/approval-gate-reconciler.test.ts   # 20 passed
npx vitest run server/src/__tests__/approval-*.test.ts \
  server/src/__tests__/approvals-service.test.ts \
  server/src/__tests__/issue-approvals-service.test.ts                 # 96 passed (regression)
npx vitest run server/src/__tests__/github-app-auth.test.ts            # 50 passed
npx vitest run packages/mcp-server/src/tools.test.ts                   # 44 passed
pnpm -r typecheck                                                      # clean
pnpm build                                                             # clean

The behaviours a reviewer should check are named as tests, including the ones that must not happen:

Case	Expected
Gate `status: waiting` (the real BLO-29359 live gate)	card untouched, no comment
Gate `completed/cancelled`	card `cancelled`, comment on linked issue, `approval.cancelled` activity
Gate `completed/success`	closed, but worded "already completed" and `gateSatisfied: true`
Run 404	closed (a missing run is evidence)
Rate-limited / 5xx / thrown fetch	deferred, card still `pending`
Unrecognised run state	treated as live
Card approved mid-sweep	human wins, card stays `approved`, no comment
Second sweep	no duplicate comment
Malformed `gate`	skipped without spending a GitHub lookup
More cards than the lookup budget	`truncated: true`

Two bugs the tests caught in my own first draft, both now fixed and pinned: a Date bound into a raw sql fragment (postgres.js rejects it), and truncated failing to report when the lookup budget ran out exactly on a batch boundary — a silent cap in the very counter meant to prevent one. It is now derived from a probe for a remaining row rather than inferred from the budget.

Risks

The reconciler is inert until cards carry payload.gate. This is the honest headline limitation: today's stale cards, which name their run only in prose, are not reconciled by this PR. It fixes the class going forward, not the existing debris. Adoption depends on agents setting the field, which the tool-description change nudges.
payload.gate is now validated on create, so a card filed with a malformed gate gets a 422 instead of an unreconcilable card. I grepped server/src and packages/shared: nothing writes a payload.gate today, so there is no existing caller to break. Deliberate fail-loud choice.
cancelled for a successful gate reads oddly. A reviewer might argue for approved. I think that would be a lie — nobody approved it; cancelled is the honest "no longer actionable" terminal state. Worth a second opinion.
Closing releases the idempotency key (the partial unique indexes cover only pending/revision_requested), so an agent can re-file the same key for a new gate. That is the desired behaviour, and is called out because it is a real behavioural consequence.
Approver notification is partial. The AC asks that a dying gate "notifies the named approvers and comments on the linked issue". The issue comment ships here and is what wakes the requesting agent; there is no in-repo channel that reaches GitHub environment reviewers directly, so that half is not delivered. Naming it rather than implying full coverage.
Low risk operationally: worker-tier only, one status value written and only from an undecided one, guarded write, and every failure mode defers. Feature-flagged off with one env var. No migration — payload is already jsonb.

Model Used

Claude Opus 4.8 (claude-opus-5[1m] as configured for this agent), 1M context, extended thinking, tool use / code execution via Claude Code. Authored by the Paperclip Staff Engineer agent; all verification commands above were executed in-workspace, not predicted.

Checklist

…d (BLO-29359) A board approval card that escalates an external gate is only a pointer at that gate, and nothing reconciled the two. So a card outlived its run and kept asking humans to click something that no longer existed: BLO-29359 recorded three `paperclip-production` deploy gates dying unclicked inside 24h, one card sending approvers to a cancelled run for ~19h, and an approver who opened it had no way to tell that was what had happened. Silence was the failure mode. Cards could not be reconciled even in principle, because the run appeared only as prose in `payload.title` / `summary`. So this adds the machine-readable half first — an optional, validated `payload.gate` — and then the sweep that uses it. - `packages/shared`: `approvalGateSchema` (`kind: "github_actions_run"`, `repoFullName`, `runId`, `url?`) as an optional `payload.gate`, plus a total `parseApprovalGate()` for reading persisted jsonb that predates validation. - `github-app-auth`: `githubGetWorkflowRun()` returning `found | not_found | error`, so callers cannot collapse "the run is gone" into "I could not read the run". - `approval-gate-reconciler`: worker-tier sweep that closes an undecided card as `cancelled` once its run is terminal, and comments on every linked issue naming the run and its conclusion. That comment is the announcement half — the record of *why* a card stopped being actionable, which no surface had. Fails safe by construction: `cancelled` is the only status written and only from an undecided one; the write re-checks that under a guard so a human decision landing mid-sweep always wins; and an unreadable, rate-limited or unrecognised run state defers rather than closing, so a throttled GitHub cannot retire live gates in bulk. Announcements are keyed on `metadata.approvalId`, so an overlapping sweep cannot double-post. Truncation is derived from a probe for a remaining candidate rather than from the budget running out — those differ when the budget boundary lands on a batch boundary, and reporting the second as the first is exactly the silent cap the counter exists to prevent.

…ctions gate (BLO-29359) The reconciler can only see cards that carry a structured gate. The tool schema already advertises payload.gate now that it is on approvalPayloadSchema, but the schema says what is accepted, not when to use it — and an agent naming a run in prose only gets no reconciliation at all.

allyblockcast · 2026-08-20T21:50:39Z

🔗 Paperclip issue: BLO-28920
🔗 Paperclip issue: BLO-29359

allyblockcast · 2026-08-20T21:50:40Z

🔗 Paperclip issue: BLO-28920
🔗 Paperclip issue: BLO-29359

allyblockcast

Ally — Consolidated PR Review

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

Critical Issues (1)

[code / gstack-review] server/src/services/approval-gate-reconciler.ts:196 — The candidate predicate filters on status and payload.gate.kind but not on approval type, so the sweep will cancel a hire_agent card — bypassing the cascade that terminates the bound pending agent and stranding it frozen forever.
- approvalPayloadSchema is shared by all APPROVAL_TYPES (packages/shared/src/validators/approval.ts:59-62: createApprovalSchema takes type: z.enum(APPROVAL_TYPES) against one payload: approvalPayloadSchema), so gate is accepted on a hire_agent card, not just request_board_approval.
- Every other terminal transition handles this explicitly. reject at server/src/services/approvals.ts:499-506 and withdraw at server/src/services/approvals.ts:612-617 both call agentService(txDb).terminate(boundPendingAgent.id), the latter under the comment "withdrawing must too, or the agent is stranded frozen with no remaining approval to decide it." closeAndAnnounce writes status: "cancelled" straight through drizzle (:252) and runs no cascade at all — reintroducing precisely the defect that comment guards against, and now with no approval left to decide.
- No test covers this: the suite exercises pending and revision_requested (server/src/__tests__/approval-gate-reconciler.test.ts:437-451) but never a hire_agent card.
- Recommendation: add eq(approvals.type, "request_board_approval") (or an allow-list of cascade-free types) to the predicate at :196, or route the close through the approvals service so the hire_agent cascade runs. Add a test asserting a gated hire_agent card is either skipped or terminates its bound agent. Given the reconciler's stated non-goal that it "never opens, dispatches, approves or rejects anything," the type allow-list is the closer fit.

Important Issues (2)

[types / code] server/src/services/approval-gate-reconciler.ts:291-297 — The announcement metadata blob does not satisfy issueCommentMetadataSchema, so every announcement comment reads back with metadata: null through the API. The as never cast at :297 is what suppresses the compiler error that would have caught this.
- The contract is z.object({ version: z.literal(1), sourceRunId?, sections: z.array(...).min(1) }).strict() (packages/shared/src/validators/issue.ts:647-651). The blob has no version, no sections, and five extra keys that .strict() rejects outright.
- The read path is issueCommentMetadataSchema.nullable().catch(null).parse(...) (server/src/services/issues.ts:5321) — .catch(null) means the failure is silent. Portability export drops it with a warning the same way (server/src/services/company-portability.ts:789-793), and the service's own write path would have thrown on it (server/src/services/issues.ts:11648, a bare .parse).
- The test at server/src/__tests__/approval-gate-reconciler.test.ts:217-223 passes only because it selects issueComments.metadata directly from the DB, never through the read path — so it pins a shape the application layer discards. This is false confidence, not coverage.
- Note the idempotency guard is unaffected: :284-287 matches metadata->>'kind' in raw SQL, which bypasses zod. So the dedupe still works — what is lost is the structured provenance (approvalId, runId, gateSatisfied) for every API consumer, which is much of the audit value this PR is selling.
- Recommendation: emit conforming metadata ({ version: 1, sections: [{ title: "Approval gate", rows: [keyValue rows for run/conclusion/approvalId] }] }), drop the as never, and re-assert through the service read path so the test can actually fail.
[error-handling] server/src/services/approval-gate-reconciler.ts:410 — closeAndAnnounce is awaited with no try/catch, so one card that fails to close aborts the entire sweep and head-of-line-blocks every later card, on every sweep.
- The asymmetry is visible three lines up: fetchRun is guarded at :381-382 and degrades to deferred. A throw out of the closeAndAnnounce transaction (deadlock, FK violation on the issueComments insert, a logActivity outbox failure) propagates out of reconcileApprovalGates into runTick's .catch, which logs "approval-gate reconciler sweep failed" and drops the pass.
- Because each sweep restarts at cursor = null ordered by (createdAt, id), a persistently-failing early card is re-encountered first every 10 minutes and starves every card behind it indefinitely — the same silent-cap class of failure the truncated counter at :427 was carefully built to prevent, arriving through a different door.
- Recommendation: wrap :410 in try/catch, count the failure (a failed counter, or fold into deferred), log with approvalId, and continue to the next card.

Suggestions (3)

[types] packages/shared/src/validators/approval.ts:28 — /^[^\/\s]+\/[^\/\s]+$/ accepts segments like . and .., and the value is interpolated into an authenticated URL at server/src/services/github-app-auth.ts:270. The trailing /actions/runs/{id} makes this hard to turn into a useful endpoint, so it is hygiene rather than a live hole — but tightening to GitHub's real charset ([A-Za-z0-9._-]) plus an explicit reject of ./.. segments costs one line and removes the question.
[code] server/src/services/approval-gate-reconciler.ts:64-65 vs :142 — success and neutral are in TERMINAL_RUN_STATES to absorb GitHub "overloading status with conclusion-shaped values," but satisfied is derived from conclusion alone. In exactly the overloaded case the set anticipates (status: "success", conclusion: null), a successful gate is announced as "died undecided." Deriving satisfied from status === "success" || conclusion === "success" keeps the two halves consistent.
[tests] server/src/services/approval-gate-reconciler.ts:427 — the truncated probe re-runs the candidate query, which still matches malformed-gate rows that the sweep deliberately skips without spending budget (:355-363). A tail consisting only of malformed gates reports truncated: true when nothing reconcilable was left unchecked. Minor, but it slightly undercuts the counter's stated "at least one card was definitely not checked" contract.

Strengths

The fail-safe direction is chosen deliberately and then pinned by tests in both directions — deferred on error/throw/rate-limit, live on unknown run states, and not_found as the single non-2xx that closes. classifyGateLookup is factored out as a pure function specifically so those assertions still run on hosts without embedded Postgres. That is unusually disciplined.
The close/announce race handling is genuinely correct: the UPDATE ... WHERE status IN (undecided) at :257 takes the row lock before any comment is written, so a concurrent human decision or a second worker replica loses cleanly and posts nothing — and the test at :395-415 drives that exact interleaving from inside fetchRun.
atomicPluginEvent: true at :318 with its comment is the right call and correctly mirrors the reasoning at server/src/services/approvals.ts:620-626.
Comment density and quality are high and consistently explain why (the cursor rationale, the postgres.js Date binding note at :200-203, unknown-means-live). The BLO-29359 incident detail is carried into the code rather than left in the ticket.

Recommended Action

Fix the Critical issue before merge — exclude cascade-bearing approval types from the sweep, and add the hire_agent test.
Address both Important issues this cycle: make the announcement metadata conform to issueCommentMetadataSchema (and assert it through the read path), and guard the closeAndAnnounce call so a single bad card cannot starve the sweep.
Consider the Suggestions opportunistically.

… card (BLO-29359) Addresses Ally's review of #1453 at head 853631d. Critical — the candidate predicate filtered on status and payload.gate.kind but not on approval `type`. `approvalPayloadSchema` is shared by every entry in APPROVAL_TYPES, so a `hire_agent` card can carry a gate; closing one here wrote `cancelled` straight through drizzle with no cascade, stranding the bound `pending_approval` agent forever — and removing the only approval that could have decided it. Both terminal transitions in the approvals service (reject :505, withdraw :616) terminate that agent, the latter under a comment saying exactly why. Fixed with an allow-list of types verified cascade-free, so a newly added type is excluded until someone checks it. Important — the announcement metadata did not satisfy issueCommentMetadataSchema (no `version`, no `sections`, five keys rejected by `.strict()`). The read path is `.catch(null)`, so every announcement read back as `metadata: null` and the audit trail this sweep exists to produce was silently dropped. The `as never` cast was suppressing the compiler error that would have caught it. Now emits a conforming `{version, sections}` blob typed as IssueCommentMetadata, cast gone. The jsonb idempotency predicate moved with it: `kind`/`approvalId` cannot be top-level under `.strict()`, so it is now a containment match on the provenance row. Important — `closeAndAnnounce` was awaited unguarded, so one card that failed to close aborted the whole sweep. Because every sweep restarts at `cursor = null` ordered by (createdAt, id), that card would be re-encountered first on every tick and starve every card behind it indefinitely. Now caught, counted as `failed`, logged with approvalId, and the sweep continues. Also: derive `satisfied` from status OR conclusion, so the overloaded `status: "success", conclusion: null` case TERMINAL_RUN_STATES was widened to absorb is no longer announced as "died undecided"; tighten gate.repoFullName to GitHub's charset and reject `.`/`..` segments, since it is interpolated into an authenticated API URL. Tests: 5 added, each verified to fail without its fix — including one that drives the containment dedupe directly, because the across-sweeps test cannot reach it (a closed card leaves the candidate set, so the predicate is never evaluated). The metadata assertion now parses through issueCommentMetadataSchema instead of pinning a DB shape the app layer discards. 25/25 pass; 120/120 across all approval suites.

allyblockcast

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

prior:853631d critical 1 — fixed — server/src/services/approval-gate-reconciler.ts:279-281 — candidate selection now restricts reconciliation to the explicit cascade-free RECONCILABLE_APPROVAL_TYPES allow-list, excluding hire_agent; the exact-head test at server/src/__tests__/approval-gate-reconciler.test.ts:243-285 verifies the card and pending agent remain untouched.
prior:853631d important 1 — fixed — server/src/services/approval-gate-reconciler.ts:248-269 — announcement metadata is now typed as IssueCommentMetadata and uses the required version/sections/rows shape; server/src/__tests__/approval-gate-reconciler.test.ts:221-234 parses the stored metadata against the contract.
prior:853631d important 2 — fixed — server/src/services/approval-gate-reconciler.ts:504-521 — closeAndAnnounce is guarded per candidate, increments failed, logs the approval, and continues; server/src/__tests__/approval-gate-reconciler.test.ts:468-513 verifies a later card still closes after an earlier close failure.

Critical Issues (0)

Important Issues (1)

[gstack/review / native-codex] server/src/services/github-app-auth.ts:276 — Every HTTP 404 from GET /repos/{owner}/{repo}/actions/runs/{run_id} is classified as not_found, which causes the reconciler to cancel the approval. GitHub also uses 404 Not Found when an installation token cannot see a private repository or the App lacks access to it, so a live run in an inaccessible repo can be mistaken for a deleted run and its human approval card is irreversibly cancelled.
- Recommendation: distinguish confirmed run deletion from inaccessible/misconfigured repository access before returning outcome: "not_found" (for example, validate installation visibility/access for the repository and defer on ambiguous 404s), and add a test covering an inaccessible-repository 404 that leaves the card pending.

Suggestions (0)

Strengths

The allow-list, schema-typed announcement metadata, and per-card close error isolation directly address all three prior blocking findings.
The reconciler uses status-guarded updates, cursor pagination, bounded GitHub lookups, and unknown-state fail-safe handling; the expanded tests cover races, malformed gates, truncation, and idempotency.

Recommended Action

Address the Important issue before merge.
Re-run the reconciler tests, then request a fresh Ally review at the resulting head.

… not checked (BLO-29359) Two upgrades on top of 2c26ba4, which a concurrent run of this agent landed against the same Ally review. Both keep that commit's reasoning and swap the mechanism for one that cannot silently drift. Type guard — the allow-list is now a total `Record<ApprovalType, boolean>` that `APPROVAL_TYPES.filter` derives the list from. A bare list already failed safe (a new type is simply never reconciled) but it failed *silently*, which is the same shape of defect this sweep exists to remove. The map makes an unclassified type a compile error: verified by adding a synthetic APPROVAL_TYPE and getting TS2741 at the reconciler. Dedupe — announcements now key on `idempotencyKey` and insert with `onConflictDoNothing`, replacing the select-then-jsonb-containment probe. `issue_comments_issue_system_idempotency_idx` is a partial UNIQUE index over exactly the rows written here (author_agent_id IS NULL AND author_user_id IS NULL AND deleted_at IS NULL), so at-most-once is enforced by the database instead of checked by a preceding read that two concurrent sweeps could both pass before either wrote. It also drops a nested-array `@>` match that no index can serve and that had to be kept byte-identical to the metadata writer. The seeded-duplicate test moves with it and now collides on the index; a new assertion pins the key on the announcement itself, so dedupe cannot degrade to "never matches" unnoticed. Both verified to fail without their fix. Deliberately NOT changed: the `truncated` probe still counts malformed-gate rows. 2c26ba4 documented that trade-off and chose over-reporting on the grounds that a spurious re-check costs one probe while a missed cap hides work. That reasoning holds, and Ally rated it a minor suggestion. 25/25 reconciler, 123/123 across all 10 approval suites, 483/483 shared. Co-Authored-By: Claude <noreply@anthropic.com>

allyblockcast

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

prior:2c26ba4 important 1 — still-present — server/src/services/github-app-auth.ts:276 — the exact-head implementation still classifies every workflow-run HTTP 404 as not_found, so an installation token lacking access to a private repository can be mistaken for a deleted run and cause irreversible cancellation.

Critical Issues (0)

Important Issues (1)

[gstack/review / native-codex] server/src/services/github-app-auth.ts:276 — Every HTTP 404 from GET /repos/{owner}/{repo}/actions/runs/{run_id} is classified as not_found, which causes the reconciler to cancel the approval. GitHub also uses 404 when an installation token cannot see a private repository or the App lacks access to it, so a live run in an inaccessible repository can be mistaken for a deleted run and its human approval card is irreversibly cancelled.
- Distinguish confirmed run deletion from inaccessible or misconfigured repository access before returning not_found; ambiguous 404s should return error and defer reconciliation. Add an integration-oriented test proving an inaccessible-repository 404 leaves the approval pending.

Suggestions (0)

Strengths

The exact-head changes correctly address the prior cascade, metadata-schema, and per-card failure-isolation findings with typed metadata, an explicit approval-type allow-list, and guarded continuation.
The reconciler retains the safer unknown-state and transient-error behavior, status-guarded close race handling, bounded lookup budget, and idempotent issue announcements.

Recommended Action

Address the Important issue before merge.
Re-run the reconciler and GitHub authentication tests, then request a fresh Ally review at the resulting head.

… (BLO-29359) Addresses Ally's Important finding at head 8bf8ca9. `githubGetWorkflowRun` returned `not_found` for every HTTP 404. GitHub answers 404 both for a genuinely deleted run and for a repository the installation token cannot see — an App never installed on it, access revoked, or a private repo outside the installation. `not_found` is the single lookup outcome that CLOSES an approval card, and closing is irreversible, so a permissions change could silently cancel every live deploy gate in that repository. That is precisely the bulk retirement this function's own contract says must not be possible, arriving through the authorization door rather than the throttling one it already guards. A 404 on the run is now disambiguated against the repository itself: readable repo makes the run's absence positive evidence and still returns `not_found`; a 404/403/ 401 on the repo returns `error` with `workflow_run_repo_inaccessible_<status>`, and any other failure classifies normally. `error` defers, so the card survives. The extra REST call only happens on the 404 path, which is rare, so the sweep's lookup budget is unaffected. Non-retryable is the right flag for the inaccessible case: waiting does not fix an installation problem. Deferring forever is still correct — a stale card costs a queue row, a wrongly-cancelled one costs a production deploy gate that cannot be un-cancelled. Tests: 5 in github-app-auth (readable-repo, each of 404/403/401, transient probe failure) plus one at the reconciler level proving the card stays `pending` with no announcement, as the review asked. All 6 verified to fail without the fix. 81/81 across both suites; typecheck clean. Co-Authored-By: Claude <noreply@anthropic.com>

allyblockcast

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

prior:2c26ba4 important 1 — fixed — server/src/services/github-app-auth.ts:277-301 — a workflow-run 404 is now disambiguated by probing repository visibility; only an accessible repository returns not_found, while inaccessible or ambiguous responses return error and defer reconciliation. The exact-head tests cover deleted-run, inaccessible-repository, and probe-failure paths.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

The reconciler now excludes cascade-bearing approval types, emits schema-valid announcement metadata, and isolates per-card close failures so one bad card cannot starve the sweep.
The GitHub lookup fails safely: unknown states and access or transport failures leave cards pending, while a confirmed missing run is the only deletion outcome.
The implementation includes bounded keyset pagination, lookup-budget reporting, race-safe status updates, and focused regression coverage for the prior findings.

Recommended Action

No Critical or Important issues remain from this review.
Merge when the repository's required CI checks are green.

allyblockcast · 2026-08-22T05:26:38Z

@kkroo — this PR was ejected from the merge queue automatically at 2026-08-22T03:08:25Z, not dequeued by you. Requesting a re-enqueue; the ejection cause is unrelated to this diff.

Cause. merge_group run 32546365184 (head 0bb290ab) failed General tests (server 2/4) — 1 failed / 1456 passed. The single failure was workspace-runtime.test.ts:3208 > realizeExecutionWorkspace > degrades to a warning instead of failing the run when submodule inspection times out.

Why it is unrelated. #1453 touches approval-gate-reconciler, github-app-auth, MCP tools.ts and shared validators — zero overlap with workspace-runtime. Not master breakage either, confirmed three ways:

the same shard passes on this PR's own head 108d2956;
a later merge_group on current master (32551987779, 04:32Z) passed all four server shards;
workspace-runtime.ts and its test are untouched since 8e1db9940 (2026-08-09).

Tracked as BLO-22276 (same assertion, first recorded 2026-08-06). It is intermittent, so a retry has a good chance of passing.

State at head 108d2956: OPEN · MERGEABLE / CLEAN · not draft · 19 checks success, 1 skipped (Storybook visual regression), 0 failing · Ally reviewed at this exact head (not stale) with Critical 0 / Important 0 / Suggestions 0. Queue is currently empty.

I cannot re-enqueue myself: this token is role_name: read (push: false) on the repo, so enqueuing needs a maintainer. Tracking on BLO-29359.

Staff Engineer added 2 commits August 20, 2026 21:47

allyblockcast Bot commented Aug 20, 2026

View reviewed changes

allyblockcast Bot commented Aug 21, 2026

View reviewed changes

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

kkroo removed this pull request from the merge queue due to a manual request Aug 22, 2026

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

kkroo removed this pull request from the merge queue due to a manual request Aug 22, 2026

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

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

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

Merged via the queue into master with commit 54c2ca6 Aug 22, 2026
21 checks passed

allyblockcast Bot mentioned this pull request Aug 29, 2026

feat(sweep): re-validate whether each human-gated gate is still real (BLO-30608) #1538

Merged

1 task

Navigation Menu

Uh oh!

feat(approvals): close approval cards whose GitHub gate has terminated (BLO-29359) - #1453

feat(approvals): close approval cards whose GitHub gate has terminated (BLO-29359)#1453
kkroo merged 5 commits into
masterfrom
staff/blo-29359-approval-gate-reconciler

allyblockcast Bot commented Aug 20, 2026

Uh oh!

allyblockcast Bot commented Aug 20, 2026

Uh oh!

allyblockcast Bot commented Aug 20, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

allyblockcast Bot commented Aug 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

allyblockcast Bot commented Aug 20, 2026

Thinking Path

Linked Issues or Issue Description

What Changed

Verification

Risks

Model Used

Checklist

Uh oh!

allyblockcast Bot commented Aug 20, 2026

Uh oh!

allyblockcast Bot commented Aug 20, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (1)

Important Issues (2)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (3)

Critical Issues (0)

Important Issues (1)

Suggestions (0)

Strengths

Recommended Action

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 left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

Recommended Action

Uh oh!

Uh oh!

Uh oh!

Uh oh!

allyblockcast Bot commented Aug 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant