Skip to content

fix(github): evict a rejected installation token on PR-mutating calls - #2406

Merged
JSONbored merged 4 commits into
mainfrom
claude/installation-token-retry-wiring
Jul 2, 2026
Merged

fix(github): evict a rejected installation token on PR-mutating calls#2406
JSONbored merged 4 commits into
mainfrom
claude/installation-token-retry-wiring

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

What

mergePullRequest, closePullRequest, createPullRequestReview, updatePullRequestBranch, createIssueComment, and getLastCloserLogin in src/github/pr-actions.ts all called createInstallationToken directly and never routed through withInstallationTokenRetry (src/github/app.ts) — the only caller of expireCachedInstallationToken. Only the check-run creator and the comment poster in src/github/comments.ts got evict-and-retry.

An installation suspended, or the App private key rotated, while a cached token (kept up to ~1h) is still in the cache meant every merge/close/review/update-branch call for that installation kept reusing the same now-invalid token and re-failing with 401 each time — no self-heal, unlike the paths that already route through the retry helper. It would just keep failing until the cache's own TTL lapsed on its own.

Fix

Routed all six through withInstallationTokenRetry, mirroring the existing comments.ts pattern exactly: return withInstallationTokenRetry(env, installationId, async (token) => { ...the existing octokit call, unchanged... });. On a 401 (isGitHubBadCredentialsError), the helper evicts the cached token and retries once with a freshly-minted one — a suspended installation or rotated App key now self-heals on the very next attempt instead of failing repeatedly against a known-bad cached token for up to an hour.

getLastCloserLogin keeps its existing outer try/catch (fail-closed to { login: null, coveredAllPages: false } on any unrecoverable error) — the retry now happens inside that try, so a persistent 401 after the retry still degrades the same way it always has.

Deliberately out of scope: createPullRequestReviewComments (not in the issue's list — it takes a mode: AgentActionMode parameter and is used for lower-stakes inline review notes) and any other caller of createInstallationToken elsewhere in the codebase. createInstallationToken itself is still imported/used for that one function, unchanged.

Tests

  • New test: closePullRequest receives a 401 on the first attempt, succeeds on the retry with a freshly-minted token — asserts exactly 2 close attempts and 2 token mints (proving the stale token was evicted, not reused).
  • 2 new tests closing coverage gaps the refactor's re-indentation surfaced in pre-existing, unrelated logic (updatePullRequestBranch's expectedHeadSha ternary TRUE branch; getLastCloserLogin's lastPage <= 1 branch, both directions of its ?? null fallback).
  • npx tsc --noEmit clean.
  • Scoped: github-pr-actions.test.ts — 22 passed.
  • Regression sweep: agent-action-executor.test.ts, agent-approval-queue.test.ts, routes-agent-approval.test.ts (direct importers), full queue.test.ts + inline-comments.test.ts — 317 passed combined.
  • Diff-range coverage-gap check on src/github/pr-actions.ts: fully covered.
  • Full unsharded npm run test:coverage: 5634 passed, 4 skipped (pre-existing/unrelated), 0 failed.
  • npm audit --audit-level=moderate: 0 vulnerabilities.

Advances #1936. Closes #2263.

mergePullRequest, closePullRequest, createPullRequestReview,
updatePullRequestBranch, createIssueComment, and getLastCloserLogin
all called createInstallationToken directly instead of routing
through withInstallationTokenRetry — the only caller of
expireCachedInstallationToken. Only the check-run creator and the
comment poster in comments.ts got evict-and-retry. An installation
suspended (or an App private key rotated) while a cached token was
still within its ~1h TTL meant every merge/close/review/update-branch
call kept reusing the same now-invalid token and re-failing with 401
until the cache entry expired on its own — no self-heal, unlike the
paths that already had the retry wiring.

Route all six through withInstallationTokenRetry, mirroring the
existing comments.ts pattern: on a 401, evict the cached token and
retry once with a freshly-minted one. createPullRequestReviewComments
and createInstallationToken's other caller are unaffected — not in
scope here.
@dosubot dosubot Bot added the size:L label Jul 1, 2026
@loopover-orb

loopover-orb Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 00:45:13 UTC

2 files · 1 AI reviewer · no blockers · readiness 93/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change consistently routes the PR action wrappers through `withInstallationTokenRetry`, so a 401 from a cached installation token now evicts and retries on the affected paths while preserving the existing request payloads and fail-closed catches. The wrapping is mechanically correct in the provided file, including the multi-request lookup helpers where retrying the whole read-only scan is safe. The notable issue is process/documentation drift: the PR description says `createPullRequestReviewComments` is deliberately out of scope, but the diff changes and tests that path.

Nits — 7 non-blocking
  • nit: The PR description is stale because `src/github/pr-actions.ts:createPullRequestReviewComments` is now routed through `withInstallationTokenRetry` despite the description saying it is deliberately out of scope.
  • nit: The new retry tests in `test/unit/github-pr-actions.test.ts` start from an empty cache, so their comments describe a stale cached token even though the first rejected token is minted inside the same call rather than pre-seeded as stale.
  • Update the PR description to list `createPullRequestReviewComments` as covered, or remove that code/test change if it is truly out of scope.
  • For one retry regression in `test/unit/github-pr-actions.test.ts`, pre-seed the cache with an initial successful call or adjust the test wording so it matches the real exercised path.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • AI maintainer-assist flagged possible low-effort patterns (elevated) — Advisory only — review the noted patterns; this AI assist never blocks the gate.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2263
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (size label size:L; 1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 572 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 572 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 65 PR(s), 572 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Triage stale or unlinked PRs.
  • No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 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.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 1, 2026
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.90%. Comparing base (bef2d67) to head (aa68022).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2406   +/-   ##
=======================================
  Coverage   95.90%   95.90%           
=======================================
  Files         224      224           
  Lines       25217    25217           
  Branches     9169     9169           
=======================================
  Hits        24185    24185           
  Misses        419      419           
  Partials      613      613           
Files with missing lines Coverage Δ
src/github/pr-actions.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…oken-retry-wiring

# Conflicts:
#	src/github/pr-actions.ts
#	test/unit/github-pr-actions.test.ts
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 2, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 54c2275 Commit Preview URL

Branch Preview URL
Jul 02 2026, 12:37 AM

JSONbored added 2 commits July 1, 2026 17:44
createPullRequestReviewComments and dismissLatestBotApproval still
called createInstallationToken directly, so a rejected cached token
would keep dropping inline review comments and skip stale-bot-approval
cleanup until the cache's own TTL lapsed -- the same failure mode the
other six functions in this file were just fixed for.

Route both through withInstallationTokenRetry, mirroring the existing
pattern exactly.
@JSONbored
JSONbored merged commit 1e456b7 into main Jul 2, 2026
9 checks passed
@JSONbored
JSONbored deleted the claude/installation-token-retry-wiring branch July 2, 2026 00:52
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 2, 2026
JSONbored added a commit that referenced this pull request Jul 2, 2026
…oken-minting poisoning

createAppJwt caches the App-level JWT for ~8 minutes with no eviction
path on rejection, unlike installation tokens (evicted + retried once
by withInstallationTokenRetry since #2406). A single GitHub-side
rejection of the currently-cached App JWT — a transient validation
hiccup, a clock-skew edge case, or a brief App suspend/reinstate —
kept getting reused by every subsequent installation-token mint
attempt across EVERY installation on the instance, since
mintInstallationToken threw straight through on the first non-ok
response with no retry at all.

Add expireCachedAppJwt (mirroring expireCachedInstallationToken) and
have mintInstallationToken evict + re-sign + retry once on a 401 from
the access-tokens endpoint, bounded the same way
withInstallationTokenRetry already bounds installation-token retries.

Fixes #2453
JSONbored added a commit that referenced this pull request Jul 2, 2026
…oken-minting poisoning

createAppJwt caches the App-level JWT for ~8 minutes with no eviction
path on rejection, unlike installation tokens (evicted + retried once
by withInstallationTokenRetry since #2406). A single GitHub-side
rejection of the currently-cached App JWT — a transient validation
hiccup, a clock-skew edge case, or a brief App suspend/reinstate —
kept getting reused by every subsequent installation-token mint
attempt across EVERY installation on the instance, since
mintInstallationToken threw straight through on the first non-ok
response with no retry at all.

Add expireCachedAppJwt (mirroring expireCachedInstallationToken) and
have mintInstallationToken evict + re-sign + retry once on a 401 from
the access-tokens endpoint, bounded the same way
withInstallationTokenRetry already bounds installation-token retries.

Fixes #2453
JSONbored added a commit that referenced this pull request Jul 2, 2026
…oken-minting poisoning (#2480)

* fix(github): evict a cached App JWT on rejection to stop fleet-wide token-minting poisoning

createAppJwt caches the App-level JWT for ~8 minutes with no eviction
path on rejection, unlike installation tokens (evicted + retried once
by withInstallationTokenRetry since #2406). A single GitHub-side
rejection of the currently-cached App JWT — a transient validation
hiccup, a clock-skew edge case, or a brief App suspend/reinstate —
kept getting reused by every subsequent installation-token mint
attempt across EVERY installation on the instance, since
mintInstallationToken threw straight through on the first non-ok
response with no retry at all.

Add expireCachedAppJwt (mirroring expireCachedInstallationToken) and
have mintInstallationToken evict + re-sign + retry once on a 401 from
the access-tokens endpoint, bounded the same way
withInstallationTokenRetry already bounds installation-token retries.

Fixes #2453

* fix(github): evict the App JWT again when the retried mint is also rejected

createAppJwt caches optimistically before GitHub validates the JWT. The
first-401 retry path evicted and re-signed once, but if that retried
JWT was ALSO rejected, the just-rejected JWT stayed cached and kept
poisoning every installation-token mint fleet-wide for up to
APP_JWT_REUSE_MS -- the exact failure mode the retry was meant to fix
(flagged by the gate's own review).

Evict again on a second 401 before throwing, so the next mint attempt
signs fresh instead of replaying the poisoned JWT.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

fix(github): a stale installation token is never evicted after a 401 on PR-mutating calls

1 participant