Skip to content

chore(token-refresh): decompose services/tokenRefresh.ts into tokenRefresh/* leaves (999 → 724) - #8547

Merged
diegosouzapw merged 4 commits into
diegosouzapw:release/v3.8.49from
MumuTW:chore/decomp-token-refresh
Jul 26, 2026
Merged

diegosouzapw merged 4 commits into
diegosouzapw:release/v3.8.49from
MumuTW:chore/decomp-token-refresh

Conversation

@MumuTW

@MumuTW MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Phase 1 god-file decomposition, 3 of 3. Independent of the sibling PRs — disjoint file sets, mergeable in any order.

This is the highest-risk of the three because it touches the OAuth refresh path, so it is deliberately the smallest extraction of the set: tokenRefresh.ts 999 → 724 lines, only the pieces with a clean seam.

What

Leaf Lines Contents
tokenRefresh/rotationMap.ts 85 rotating-refresh-provider map
tokenRefresh/casGuard.ts 84 compare-and-swap guard
tokenRefresh/circuitBreaker.ts 168 refresh circuit-breaker
tokenRefresh/shared.ts +16 receives isUnrecoverableRefreshError (file already existed)

tokenRefresh.ts still re-exports the moved symbols, so the ~8 call sites across open-sse/executors, handlers/chatCore, tokenHealthCheck, codexAuthFile, claudeAuthFile and the OAuth import route are untouched. Verified by importing the module and asserting the symbol resolves.

One real regression, caught and fixed

Moving isUnrecoverableRefreshError out of tokenRefresh.ts broke tests/unit/oauth-providers-error-handling.test.ts. That suite makes text-based assertions on source files — it reads tokenRefresh.ts and regex-matches the function body to prove the unrecoverable sentinel is returned:

const src = await read("open-sse/services/tokenRefresh.ts");
const fnMatch = src.match(/export\s+function\s+isUnrecoverableRefreshError\([\s\S]+?\n\}/);

A re-export does not satisfy that, so the definition went missing and the test failed — the only red test across the 23 tokenRefresh-related suites. The second commit repoints the read() at tokenRefresh/shared.ts, which is where the body now lives. The assertion itself is unchanged, and the public surface is unchanged.

Worth flagging for reviewers of future decomposition work: this repo has source-text structural tests that a pure move can break without any behavioral change.

Faithfulness

  • 18 of 18 moved functions byte-identical after normalizing comments/whitespace
  • no function lost, none duplicated in the host
  • isUnrecoverableRefreshError is a genuine move, not new logic — it lived at line 489 of the pre-move tokenRefresh.ts

Verification

Check Result
typecheck:core clean
check:cycles no cycles
check-file-size 999 → 724, under cap 800; new leaves ≤ 168
Pre-existing + new suites touching tokenRefresh (23 files) 267/267 pass

Inherited base-red (not from this PR)

release/v3.8.49 is red on two gates this branch does not touch: lint (stale suppression, fixed by #8544) and file-size (providers/page.tsx 1990 > 1927, tokenHealthCheck.ts 843 > 841 — fixed by #8532 / #8524). Both verified present on the base at branch point.

@MumuTW
MumuTW requested a review from diegosouzapw as a code owner July 25, 2026 08:26
@diegosouzapw

Copy link
Copy Markdown
Owner

Reviewed #8547 in an isolated worktree against the PR head (167af6e).

Verification:

Diffed all 18 relocated functions against the pre-move file — they're byte-identical moves, and the re-export surface in tokenRefresh.ts keeps every downstream import site (chatCore, executors/base, tokenHealthCheck, codex/claude auth files, the OAuth import route) working unchanged. The oauth-providers-error-handling.test.ts source-text-assertion fix (following the regex to shared.ts) is exactly the kind of decomposition footgun worth calling out for future extractions — good catch.

One small nit, non-blocking: the new header comment in tokenRefresh.ts says credit for #7338 was "preserved via co-authorship on the extraction commits," but none of the 3 commits actually carries a Co-authored-by trailer. Since #7338 is closed and this is an independent rewrite rather than a reuse of that diff, it's not a real attribution problem — just worth tightening the wording (or adding the trailer) so the comment matches the commit history.

No blocking issues found from this review.

MumuTW added 4 commits July 25, 2026 22:33
…d.ts

cad2c72 moved isUnrecoverableRefreshError out of tokenRefresh.ts into
tokenRefresh/shared.ts. This suite asserts on source *text* (it regex-matches
the function body to prove the unrecoverable sentinel is returned), so the
move made it fail to find the definition — the only red test across the 23
tokenRefresh-related suites.

Repoint the read() at the file that now defines the body. The public surface
is unchanged: tokenRefresh.ts still re-exports the symbol, verified by import.
…tokenRefresh header

The header claimed credit for KooshaPari's diegosouzapw#7338 was "preserved via co-authorship on the
extraction commits", but none of the commits carries a Co-authored-by trailer -- and adding
one would be inaccurate, since this is an independent implementation against the current
tip rather than a reuse of that diff. The by-name credit for proposing the split stays;
only the false claim about the mechanism is removed.
@MumuTW
MumuTW force-pushed the chore/decomp-token-refresh branch from c9bd671 to 4c3f5d0 Compare July 25, 2026 14:36
@MumuTW

MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

The #7338 attribution nit is fixed — commit 4c3f5d00a (was c9bd671ea before the rebase) rewrites the header to state this is an independent implementation against the current tip, not a reuse of that diff, and drops the co-authorship claim entirely. The old wording was left over from an earlier plan to cherry-pick #7338 rather than reimplement it, so it was stale rather than aspirational — good catch.

Also rebased onto 4053e2314, which fixes the base-red gates you confirmed (#8544, #8534, #8539, #8561).

Re-verified on the rebased head: token-refresh-cas-guard, token-refresh-circuit-breaker, token-refresh-rotation-map and oauth-providers-error-handling pass 48/48; check:file-size OK.

@MumuTW

MumuTW commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Correction to my comment above: Fast Quality Gates will still be red after the rebase, and it isn't this PR. #8561 fixed check:file-size and thereby unmasked a fifth base-red gate behind it in the same job — check:complexity-ratchets (complexity 2169 > baseline 2130, cognitiveComplexity 956 > 951).

I measured it on pristine detached checkouts with an empty working tree: 4053e2314 (current tip) and 30709255c (the base you reviewed against) both report 2169 / 956 — identical to this PR's head, so it predates #8561 and is not caused by anything here. It was hidden because check:file-size ran earlier in the same bash -e job and short-circuited it. Full measurement table in my comment on #8546.

The gate is in quality.yml's fast-gates job, so it's red for every PR against release/v3.8.49 regardless of content. Everything else on this PR is green and verified locally.

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks @MumuTW — merged into release/v3.8.49 via the local merge-train (validated as one combined tree: full test:unit + test:vitest 274/274 on the 32-core box, tip d4b9ce6016). Your commit keeps its authorship. 🚀

@diegosouzapw diegosouzapw mentioned this pull request Jul 28, 2026
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
…fresh/* leaves (999 → 724) (diegosouzapw#8547)

* chore(token-refresh): extract rotation/cas/circuit-breaker refresh logic into tokenRefresh/* leaves

* test(oauth): follow isUnrecoverableRefreshError to tokenRefresh/shared.ts

cad2c72 moved isUnrecoverableRefreshError out of tokenRefresh.ts into
tokenRefresh/shared.ts. This suite asserts on source *text* (it regex-matches
the function body to prove the unrecoverable sentinel is returned), so the
move made it fail to find the definition — the only red test across the 23
tokenRefresh-related suites.

Repoint the read() at the file that now defines the body. The public surface
is unchanged: tokenRefresh.ts still re-exports the symbol, verified by import.

* docs(changelog): add fragment for this PR

* docs(auth): correct the diegosouzapw#7338 attribution wording in the tokenRefresh header

The header claimed credit for KooshaPari's diegosouzapw#7338 was "preserved via co-authorship on the
extraction commits", but none of the commits carries a Co-authored-by trailer -- and adding
one would be inaccurate, since this is an independent implementation against the current
tip rather than a reuse of that diff. The by-name credit for proposing the split stays;
only the false claim about the mechanism is removed.
@MumuTW
MumuTW deleted the chore/decomp-token-refresh branch September 5, 2026 10:18
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…fresh/* leaves (999 → 724) (diegosouzapw#8547)

* chore(token-refresh): extract rotation/cas/circuit-breaker refresh logic into tokenRefresh/* leaves

* test(oauth): follow isUnrecoverableRefreshError to tokenRefresh/shared.ts

cad2c72 moved isUnrecoverableRefreshError out of tokenRefresh.ts into
tokenRefresh/shared.ts. This suite asserts on source *text* (it regex-matches
the function body to prove the unrecoverable sentinel is returned), so the
move made it fail to find the definition — the only red test across the 23
tokenRefresh-related suites.

Repoint the read() at the file that now defines the body. The public surface
is unchanged: tokenRefresh.ts still re-exports the symbol, verified by import.

* docs(changelog): add fragment for this PR

* docs(auth): correct the diegosouzapw#7338 attribution wording in the tokenRefresh header

The header claimed credit for KooshaPari's diegosouzapw#7338 was "preserved via co-authorship on the
extraction commits", but none of the commits carries a Co-authored-by trailer -- and adding
one would be inaccurate, since this is an independent implementation against the current
tip rather than a reuse of that diff. The by-name credit for proposing the split stays;
only the false claim about the mechanism is removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants