[Bug]: transient 403 on Gate completion leaves check stuck in_progress

[galuis116]

[Bug]: A transient 403 on the Gate-completion call leaves the check stuck in_progress — defeating the #655 guarantee

Summary

PR #655 ("never leave the Gittensory Gate check stuck in_progress") wraps gate
evaluation in a try/catch so that any failure finalizes the already-posted
pending check to a neutral, non-blocking terminal state. That guarantee has a
hole: the finalize path only runs for failures that throw. A GitHub 403
on the gate-completion call does not throw — createOrUpdateNamedCheckRun
classifies every 403 as permission_missing and returns it. So gateFinalized
stays false, the catch block never runs, and the pending in_progress check is
orphaned forever — exactly the multi-hour stuck Gate #655 set out to eliminate.

Because GitHub returns 403 for transient conditions too — secondary/abuse
rate limits ("You have exceeded a secondary rate limit") and momentary "Resource
not accessible by integration" blips — this is reachable in normal operation, not
just on a genuine permission revocation.

Evidence

// src/queue/processors.ts — gate completion (inside the try)
if (gateEnabled) {
  const gateCheckResult = await createOrUpdateGateCheckRun(
    env, installationId, repoFullName, advisory,
    gateCheckPolicy(settings, readiness.total, confirmedContributor),
    { checkRunId: pendingGateCheckRunId },
  );
  if (gateCheckResult?.kind === "published") gateFinalized = true;
  if (gateCheckResult?.kind === "permission_missing") {
    await auditGateCheckPermissionMissing(...);   // <-- only audits; does NOT finalize, does NOT throw
  }
}
// ...no throw -> the catch (which calls createOrUpdateErroredGateCheckRun) is skipped.

// src/github/app.ts — every 403 becomes a non-throwing permission_missing
} catch (error) {
  if (isCheckRunPermissionError(error)) {
    return { kind: "permission_missing", warning: "GitHub App Checks: write permission is missing. ..." };
  }
  throw error;
}

function isCheckRunPermissionError(error: unknown): boolean {
  ...
  if (e.status === 403) return true;   // <-- ALL 403s, including transient secondary-rate-limit
  return typeof e.message === "string" && /resource not accessible by integration|not have permission/i.test(e.message);
}

The catch block that the #655 fix relies on only fires on a throw:

// src/queue/processors.ts — the finalize-on-error path
} catch (error) {
  if (gateEnabled && pendingGateCheckRunId !== undefined && !gateFinalized) {
    await createOrUpdateErroredGateCheckRun(env, installationId, repoFullName, advisory, { checkRunId: pendingGateCheckRunId }).catch(() => undefined);
    await recordAuditEvent(env, { eventType: "github_app.gate_finalized_on_error", ... });
  }
  throw error;
}

Why it's wrong

The comment at the top of the guarded block promises:

Everything from here until the gate is completed runs inside a try so that
ANY failure/timeout … still finalizes the check to a neutral, non-blocking
state instead of orphaning it in_progress forever (the cause of the multi-hour
stuck Gate).

A 403 on the completion call is such a failure, but it is funneled into the
silent permission_missing no-op rather than the finalize path, so the promise
is not kept.

Crucially, this 403 is almost always transient: the pending check was already
posted successfully (pendingGateCheckRunId is set), which proves the App had
Checks:write at pending time. A 403 a moment later on the completion PATCH is
therefore far more likely a secondary rate-limit than a real permission loss —
and the correct response is to finalize the check (it re-runs on the next push),
not to leave it hanging.

Reachability

A normal pull_request webhook with the gate enabled. The pending check posts
(common case — permission present), then the completion PATCH hits a transient
403 from GitHub. No exotic configuration required. When createOrUpdatePendingGateCheckRun
itself returns permission_missing (genuine missing permission), pendingGateCheckRunId
stays undefined and there is nothing to orphan — so the broken case is exactly
the transient one, where a check was posted and then can't be finalized.

Suggested fix

When the completion of an already-posted pending check returns
permission_missing (i.e. pendingGateCheckRunId !== undefined && !gateFinalized),
still finalize the pending check to neutral — mirroring the catch block:

if (gateCheckResult?.kind === "permission_missing") {
  await auditGateCheckPermissionMissing(...);
  if (pendingGateCheckRunId !== undefined && !gateFinalized) {
    await createOrUpdateErroredGateCheckRun(env, installationId, repoFullName, advisory, { checkRunId: pendingGateCheckRunId }).catch(() => undefined);
  }
}

If the 403 was a genuine permission revocation, this finalize PATCH also fails and
is swallowed by .catch(() => undefined) — no worse than today. If it was
transient (the common case), the check is correctly closed to neutral and re-runs
on the next push. A cleaner alternative is to not classify a 403 on a known
check_run_id completion PATCH as permission_missing at all and let it throw into
the existing catch — but the targeted finalize is the smaller, lower-risk change.

Test status

Not covered. test/unit/queue.test.ts ("finalizes the Gate to neutral instead of
leaving it in_progress when gate completion fails") injects a 500 on the
completion PATCH — a 500 throws, so the catch finalizes and the test passes. The
403 / permission_missing completion path is untested and silently leaves the
check in_progress. A regression test should stub the completion PATCH to return
403 and assert the pending check (same id) is finalized to neutral.

Confidence note

High. This is a concrete, reachable gap in the exact guarantee #655 codified,
with a contract stated in-code, a clear transient trigger (GitHub secondary rate
limits are 403), and a test that demonstrably only exercises the throw path. The
only judgement call is the fix shape (targeted finalize vs. reclassifying the 403);
both are defensible, and the targeted finalize is the conservative choice.

Distinct from prior reports

Builds directly on #655 but is a different code path: #655 handles failures
that throw (via the catch); this is the permission_missing return on the
completion of an already-posted pending check, which bypasses that catch entirely.
No existing report covers the transient-403 finalize gap.