fix(github): restore the recovery path for revoked user OAuth tokens

[kimnamwook1]

Fixes #10994.

Three small fixes in services/github/pod-github that together restore the recovery path for a GitHub user whose OAuth token has been revoked or has expired past refresh.

Why these three belong in one PR

The three defects sit on a single chain, and each one disables a different recovery mechanism that is already written in the codebase:

1. checkRefreshToken ignores force — the validation is skipped, so a dead token is never detected.
2. getOctokit keeps going after a failed check — a client is built from the dead token anyway, so the installation-token fallback the callers already wrote can never fire.
3. secretToUserRecord drops accounts — even when revocation is detected, the notification loop has nothing to iterate, so the user is never told to re-authorise.

Fixing only one of them leaves the chain broken. Each fix is independent and reviewable on its own (one commit each), but the user-visible symptom only clears with all three.

---

1. platform.ts — force was never read

checkRefreshToken declares force but the body never references it:

// services/github/pod-github/src/platform.ts:740-741 (develop @ c36dd74)
async checkRefreshToken (ctx: MeasureContext, auth: GithubUserRecord, force: boolean = false): Promise<boolean> {
  if (auth.refreshToken != null && auth.expiresIn != null && auth.expiresIn < Date.now() / 1000) {

The only caller that passes true is the user re-sync path:

// services/github/pod-github/src/worker.ts:591
await this.platform.checkRefreshToken(ctx, record, true)

so that call is indistinguishable from the unforced one and returns true without contacting GitHub.

Change:

const expired = auth.expiresIn != null && auth.expiresIn < Date.now() / 1000
if (auth.refreshToken != null && (force || expired)) {

This also narrows a second gap: a record with a refreshToken but expiresIn == null currently returns true unvalidated. platform.ts:481 can write exactly that (expiresIn: resultJson.expires_in != null ? … : null). After the change, the forced call validates it.

Behaviour for the unforced call is unchanged: force is false, so the condition reduces to refreshToken != null && expired, which is the original predicate.

2. worker.ts — a failed check did not stop client creation

// services/github/pod-github/src/worker.ts:644-646 (develop @ c36dd74)
if (!(await this.platform.checkRefreshToken(ctx, record))) {
  record.octokit = undefined
}
if (record.octokit !== undefined) {
  return record.octokit
}
record.octokit = ctx.withSync('create-octokit', {}, () => new Octokit({ auth: record.token, … }))
return record.octokit

Clearing record.octokit and then falling through means the next statement rebuilds a client from record.token — the token that was just rejected. getOctokit consequently never returns undefined for a revoked user.

That matters because the callers already handle undefined, at 13 sites:

// e.g. services/github/pod-github/src/sync/issues.ts:753-756
const okit = ensureGraphQLOctokit(
  (await this.provider.getOctokit(ctx, existingIssue.modifiedBy)) ?? container.container.octokit,
  container
)

(also issues.ts:803, issueBase.ts:343, comments.ts:156,365,441, reviews.ts:154,439, reviewComments.ts:159,442,520, reviewThreads.ts:383,461)

The ?? container.container.octokit branch is dead code today. Change:

if (!(await this.platform.checkRefreshToken(ctx, record))) {
  record.octokit = undefined
  return undefined
}

On whether enabling this fallback is intended — this is the one change here that widens existing behaviour, so it deserves the scrutiny. I read the ?? operators at those 13 sites as intended design rather than defensive noise, for three reasons: the right-hand side is the installation token, which is the natural degraded mode when a user token is gone; the expression is meaningless if the left side can never be undefined; and the helper those calls feed into declares the undefined case in its own signature and documents the fallback:

// services/github/pod-github/src/sync/utils.ts:45-50
export function ensureGraphQLOctokit (okit: Octokit | undefined, container: ContainerFocus): Octokit {
  if (okit !== undefined && typeof (okit as any).graphql === 'function') {
    return okit
  }
  return container.container.octokit
}

(same shape in ensureRESTOctokit, utils.ts:57-62). Both take Octokit | undefined and both fall back to the installation client — parameters that, before this change, could only ever receive a defined value.

If the intent was instead to fail the sync outright when a user token dies, the fix should be a thrown error and those 13 ?? clauses plus the | undefined parameters should go — happy to redo it that way.

3. users.ts — accounts was discarded on read

// services/github/pod-github/src/users.ts:38-45 (develop @ c36dd74)
return {
  ...(JSON.parse(secret.secret) ?? {}), // TODO: Add security
  account: secret.socialId,
  _id: login,
  accounts: {}
}

The literal accounts: {} sits after the spread, so it overwrites whatever was parsed.

accounts is genuinely persisted — updateUser stringifies the entire record:

// services/github/pod-github/src/users.ts:135, :143
secret: JSON.stringify(dta),

and it is populated on the authorisation path at platform.ts:488 and platform.ts:495:

accounts: { [payload.workspace]: payload.accountId }
…
dta.accounts = { ...existingUser.accounts, [payload.workspace]: payload.accountId }

So the write path stores it and the read path throws it away. The consequence is in revokeUserAuth:

// services/github/pod-github/src/platform.ts:1411-1421
public async revokeUserAuth (ctx: MeasureContext, record: GithubUserRecord): Promise<void> {
  for (const [ws, acc] of Object.entries(record.accounts)) {
    await this.updateAccountAuthRecord(ctx, { workspace: ws as WorkspaceUuid, accountId: acc }, { login: record._id }, undefined, true)
  }
}

For any record loaded via getAccount, record.accounts is {}, the loop body never executes, and no workspace ever learns that the user must re-authorise.

Change:

const parsed = JSON.parse(secret.secret) ?? {} // TODO: Add security
return {
  ...parsed,
  account: secret.socialId,
  _id: login,
  accounts: parsed.accounts ?? {}
}

?? {} keeps the old value for secrets written before the field existed. account and _id still deliberately override the parsed payload, as before.

---

Verification — what I actually ran

Please read this section literally; I have not claimed anything I did not execute.

Ran, passed:

• Format gate reproduction. I rebuilt the _phase:format pipeline (prettier 3.6.2 → eslint 8.54.0 with standard-with-typescript 40 and the three rig rule overrides, per platform-rig/profiles/default/eslint.config.json) in a standalone directory over the three files.
  • Harness validated first against the unmodified files at c36dd74: prettier --write + eslint --fix reproduced them byte for byte (diff exit 0 on all three), so the harness matches what CI expects.
  • With my changes applied, prettier --write + eslint --fix again left all three files byte for byte unchanged (diff exit 0). The formatting step should be a no-op on this branch.
• Differential lint. Same harness, no --fix: the unmodified files report 28 errors (all in worker.ts, all pre-existing strict-boolean-expressions / return-await; users.ts and platform.ts report zero). The modified files report the same 28 errors — same rules, same order, same counts. The three errors before the insertion point (lines 202, 242, 278) are unshifted; every error from line 735 onward shifted by exactly +1, accounted for by the single line added in getOctokit. No new lint findings, none removed.

Did not run, and why:

• No rush install, no rush build, no rushx test. @hcengineering/pod-github declares ~100 workspace: dependencies; compiling it means building a large portion of the monorepo, which I could not justify setting up for a 6-line change. The repository's own AGENTS.md also asks not to run build commands for verification.
• Consequently the lint run above had no real type information — the workspace imports were unresolved, so types degraded to any. That is exactly why the 28 strict-boolean-expressions errors appear on the unmodified files too. The differential is still meaningful (identical degradation on both sides), but it is not a substitute for tsc. Please let CI type-check this.
• No runtime or integration test. I did not exercise a revoked-token flow against GitHub. The behavioural claims above are read off the call sites cited, not observed at runtime.

Type-level reasoning I did check by hand (not machine-verified):

• getOctokit is declared Promise<Octokit | undefined> (worker.ts:621), so return undefined is within the signature.
• force and expired are both boolean, so force || expired is boolean.
• GithubUserRecord.accounts is Record<WorkspaceUuid, PersonId> and non-optional (types.ts:226); parsed.accounts ?? {} satisfies it.

Scope

Three files, +6 −3, all under services/github/pod-github/src/. No other files touched, no dependency or config changes.