fix(gate): finalize the Gate on a transient 403 at completion - #732
Conversation
PR JSONbored#655 finalizes the pending in_progress Gate check on any failure that throws. But a 403 on the completion call is classified as permission_missing and returns without throwing, so the catch never runs and the pending check is orphaned in_progress forever. The pending check already posted (pendingGateCheckRunId is set), proving the App had Checks:write, so a 403 at completion is almost always a transient secondary-rate-limit rather than a real revocation. Finalize the pending check to neutral in the permission_missing branch too (mirroring the catch); a genuine revocation makes this PATCH 403 as well and it is swallowed. Closes JSONbored#730
|
Note Gittensory Gate skippedPR closed before full evaluation. No late first comment was created.
💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. |
|
gittensory · advisory review Reviewed 2 changed file(s) — two independent AI reviewers. Changed files: Suggested action: ✅ Safe to merge — both reviewers found no blocking issues. Reviewer A · Suggestions
Worth double-checking
Reviewer B · No blocking issues spotted. |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
JSONbored
left a comment
There was a problem hiding this comment.
PR #732 — fix(gate): finalize the Gate on a transient 403 at completion
Author: philluiz2323 | CI: ✅ pass | Status: Failing CI
ACTION: Changes required
Highlights:
- Real reliability fix: a 403 at the completion PATCH was classified permission_missing (non-throwing), so the pending check was left orphaned in_progress; this finalizes it to neutral.
- Does NOT change who is blocked — only resolves a stuck check to a non-blocking state; upholds the confirmed-contributor-only Gate invariant.
- Strong regression test (stubs 403 secondary-rate-limit, asserts check finalized to neutral). Closes #730.
Notes / before merge:
- mergeable_state: failing CI, please ensure proper test coverage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #732 +/- ##
=======================================
Coverage 96.69% 96.69%
=======================================
Files 94 94
Lines 13845 13848 +3
Branches 5050 5051 +1
=======================================
+ Hits 13388 13391 +3
Misses 88 88
Partials 369 369 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JSONbored
left a comment
There was a problem hiding this comment.
PR #732 — fix(gate): finalize the Gate on a transient 403 at completion
Verdict: MERGE
Resolves issue #730: FULLY
CI: Green where completed (test 2, workers); test 1/lint/ui in_progress at review time, no failures. gittensory neutral.
Highlights:
- Implements the issue's targeted finalize: in the completion permission_missing branch, when pendingGateCheckRunId is set and !gateFinalized, calls createOrUpdateErroredGateCheckRun (neutral terminal) with .catch(()=>undefined) and sets gateFinalized=true — closing the orphaned-in_progress hole that bypassed the #655 catch.
- Correctly scoped to the completion-of-already-posted case; a genuine pending-time permission_missing is untouched; a real revocation's finalize PATCH 403 is harmlessly swallowed; gateFinalized guards against double-finalize.
- Realistic regression test: first completion PATCH 403 ("secondary rate limit"), then asserts a second PATCH on the SAME check id finalizes to completed/neutral — the throw-vs-return gap the old 500-injection test missed.
Concerns / required changes:
- None blocking — let the mid-run jobs finish green (identical-content jobs + test(2) shard already passed).
Completeness vs issue: Applies the issue's smaller targeted-finalize option and the 403-path regression test; the #655 "never orphan in_progress" guarantee now holds for the transient-403 completion path.
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 state. That guarantee only covers failures that throw — and a GitHub 403 on the completion call does not throw.createOrUpdateNamedCheckRunclassifies every 403 as a non-throwingpermission_missingreturn:In
processors.ts, the completion'spermission_missingbranch only audited — it didn't finalize and didn't throw. SogateFinalizedstayedfalse, the catch never ran, and the pendingin_progresscheck was orphaned — exactly the multi-hour stuck Gate #655 set out to eliminate.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"), so this is reachable in normal operation. Crucially, the pending check already posted (
pendingGateCheckRunIdis set), proving the App hadChecks:writeat pending time — so a 403 a moment later at completion is almost always transient, not a real revocation, and the correct response is to finalize.Fix
Finalize the pending check to neutral in the completion's
permission_missingbranch as well, mirroring the catch:If the 403 was a genuine permission revocation, this finalize PATCH also 403s 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.Tests
The existing
queue.test.tstest injects a 500 on the completion PATCH — a 500 throws, so the catch finalized and the test passed, leaving the 403 path uncovered. Added a regression test that stubs the completion PATCH to return 403 ("secondary rate limit") and asserts the same pending check (id 971) is finalized toneutral("Gittensory Gate — could not finish evaluating") rather than leftin_progress.Full coverage suite green locally (only the known CRLF-local
gittensory-focus-manifesttest fails locally; passes in CI).Closes #730