Skip to content

github(app): guard the NaN expiry in mintInstallationToken so an unparseable expires_at cannot disable the installation-token cache #10026

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

mintInstallationToken's local App-JWT path parses GitHub's expires_at straight into the cache entry with no
finiteness check (src/github/app.ts:364-367):

  const expiresAtMs = payload.expires_at
    ? Date.parse(payload.expires_at)
    : Date.now() + 50 * 60_000;
  await writeCachedToken(installationId, { token: payload.token, expiresAtMs });

Date.parse returns NaN for any present-but-unparseable string. A NaN expiry poisons every downstream
comparison, because all of them are >/- comparisons that are false for NaN:

  • createInstallationToken (src/github/app.ts:145-147) — cached.expiresAtMs - TOKEN_SAFETY_MARGIN_MS > Date.now()
    is false, so the cached token is never considered live and a fresh token is minted on every single call.
  • The brokered stale-token grace (src/github/app.ts:292) — cached.expiresAtMs > Date.now() is false, so the
    grace path can never serve the still-valid cached token.

Minting on every call is the exact regression the cache exists to prevent; the cache's own header comment
(src/github/app.ts:89-94) records that it caused an observed min_remaining=0 REST exhaustion — "reviews
errored → dead-lettered → missed syncs → stale head SHAs". A NaN expiry reintroduces that failure silently:
nothing logs, nothing errors, the only symptom is a mint storm.

The sibling broker path already guards this exact case, and documents why
(src/orb/broker-client.ts:64-68):

  // A present-but-unparseable expiresAt must fall back like an absent one: Date.parse → NaN would otherwise
  // propagate into the installation-token cache, where `cached.expiresAtMs - margin > Date.now()` is always
  // false for NaN — re-minting a brokered token on every GitHub call instead of caching it for ~an hour.
  const parsedExpiry = payload.expiresAt ? Date.parse(payload.expiresAt) : Number.NaN;
  const expiresAtMs = Number.isFinite(parsedExpiry) ? parsedExpiry : Date.now() + 50 * 60_000;

The two token sources write into the same cache (writeCachedToken) and are read by the same
comparison, so the local App-JWT path is the one remaining hole in a guard that is already the documented
convention for the other half of the same feature.

Requirements

  • mintInstallationToken's local App-JWT branch (src/github/app.ts:364-366) MUST treat a present-but-
    unparseable expires_at identically to an absent one: fall back to Date.now() + 50 * 60_000.
  • The fallback constant MUST stay 50 * 60_000 — do NOT change the fallback window, the safety margin
    (TOKEN_SAFETY_MARGIN_MS), or any other cache-lifetime number.
  • A well-formed expires_at MUST keep producing exactly Date.parse(payload.expires_at) — the happy path is
    correct today and must be byte-identical after this change.
  • An ABSENT expires_at MUST keep producing the Date.now() + 50 * 60_000 fallback it produces today.
  • Do NOT change src/orb/broker-client.ts — it is already correct and is the pattern being mirrored.
  • Do NOT add logging, metrics, or a thrown error for the unparseable case: the broker sibling degrades silently
    to the fallback, and this must behave identically.

⚠️ Required pattern: mirror fetchBrokeredInstallationToken's guard at src/orb/broker-client.ts:67-68
(const parsed = ...; Number.isFinite(parsed) ? parsed : Date.now() + 50 * 60_000) inline in
mintInstallationToken. What does NOT satisfy this issue: (a) a repo-wide "safe date parse" utility applied
across src/** — the blast radius is wrong and every other Date.parse call site in src/github/** already
has its own explicit guard; (b) making createInstallationToken's liveness comparison NaN-tolerant instead of
fixing the write — that leaves a NaN in the persisted store for the shared self-host
InstallationTokenStore to serve back; (c) a test-only PR that asserts the current behaviour.

Deliverables

  • mintInstallationToken in src/github/app.ts returns a cache entry whose expiresAtMs is finite for
    every response shape. Exact inputs/outputs: a token response of
    { token: "t", expires_at: "not-a-date" } MUST write { token: "t", expiresAtMs: <Date.now() + 3_000_000> }
    (finite), NOT NaN; a response of { token: "t", expires_at: "2030-01-01T00:00:00Z" } MUST write
    expiresAtMs === Date.parse("2030-01-01T00:00:00Z"); a response of { token: "t" } MUST write
    expiresAtMs === Date.now() + 3_000_000.
  • A test in test/unit/github-app.test.ts asserting that after a mint whose expires_at is unparseable,
    a SECOND createInstallationToken(env, installationId) call for the same installation issues no
    further POST /app/installations/{id}/access_tokens request (i.e. the cache is honored) — assert the
    fetch-call count, not just the returned token string.
  • A test in test/unit/github-app.test.ts asserting the well-formed expires_at path is unchanged: the
    cached token is reused on the second call, and the parsed value equals Date.parse(...).
  • A regression test at test/unit/github-app.test.ts named for this bug (e.g.
    "REGRESSION: an unparseable expires_at must not disable the installation-token cache").

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that adds the Number.isFinite guard but only tests the returned token string rather than asserting the mint
call count, so the "cache is actually honored" property is never proven — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts; src/github/app.ts is measured and gated.

The change introduces one new ternary/conditional with three distinct arms that each need a test:
absent expires_at (fallback), present-and-parseable (Date.parse result), present-and-unparseable
(fallback). All three arms must be covered — the unparseable arm is the new branch and is the one an
incomplete PR will leave untested.

This change is NOT in packages/loopover-engine/src/**, so the dual-upload engine-coverage rule does not apply.

Expected Outcome

A GitHub installation-token response carrying a malformed expires_at degrades to the same ~50-minute cache
window an absent expires_at already gets, instead of silently disabling the installation-token cache for that
installation and re-minting a token on every GitHub call until the process restarts. The local App-JWT path
and the Orb broker path now write provably-finite expiries into the shared token cache.

Links & Resources

  • src/github/app.ts:364-367 — the unguarded Date.parse(payload.expires_at) write
  • src/github/app.ts:145-147createInstallationToken's liveness check, always false for a NaN expiry
  • src/github/app.ts:89-94 — the cache's own header comment on the REST-exhaustion incident it prevents
  • src/github/app.ts:292 — the brokered stale-token grace, also always false for a NaN expiry
  • src/orb/broker-client.ts:64-69 — the documented sibling guard this must mirror

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions