fix(cloud): steward session follow-ups — one host map, retryable cookie sync, exp-less tokens clearable, 401-guard tests#11067
Conversation
…-less tokens clearable, test the 401 guards Addresses the /review findings on this branch: - ELIZA_CLOUD_DIRECT_API_BY_HOST is now exported from steward-url.ts and imported by StewardProviderShared + public-pages steward-session — one map instead of three hand-synced copies in packages/ui. Map drift between those copies was exactly the bug this PR fixes. - The session-sync keep-path resets the dedupe marker so the cookie POST retries on the next trigger once the endpoint heals, instead of never re-attempting for that token's lifetime. - tokenIsExpired now treats a missing exp claim as expired: the 401 handlers keep any non-expired token, so an exp-less token would otherwise be uncloseable (no 401 could clear it and it never ages out). - New StewardProviderRuntime.test.tsx covers the load-bearing 401 behavior: valid token survives session+refresh 401s (and the sync retries), expired and exp-less tokens still clear. Includes an in-memory Storage shim for Node >=22's broken bare localStorage global under vitest.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
lalalune
left a comment
lalalune
left a comment
There was a problem hiding this comment.
Review + independent verification:
- Host-map dedup verified: the surviving
ELIZA_CLOUD_DIRECT_API_BY_HOSTinsteward-url.tsis a complete superset of both deleted copies (all four hosts incl. the staging→api-staging mapping whose absence caused the sign-in loop); both former call sites now import it. The documentedcloud/sharedboundary copy staying is correct — ui can't depend on the server bundle. tokenIsExpiredflip audited across all callers: exp-less ⇒ expired is safe at every use — :85 skips cookie-sync (conservative), :128 keep-path (the fix target: exp-less no longer unkillable), :196 clear-path. Steward always mintsexp, so no legit token changes behavior.- Keep-path retry: resetting
lastSyncedTokenon the kept-401 path is the minimal correct way to let the next visibility/storage trigger re-attempt the HttpOnly cookie POST. - Ran the suites locally (Windows): 3 files, 11/11 pass (Runtime 401-handling incl. the retry-on-next-trigger assertion, Shared unit tests, email-callback regression).
Nice follow-through on the #11016 review findings. Merging.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
2 participants
What
Follow-up to #11016 (merged), addressing the review findings on it:
One host→API map instead of three.
ELIZA_CLOUD_DIRECT_API_BY_HOSTis now exported fromsteward-url.tsand imported byStewardProviderSharedand public-pagessteward-session(which had its ownELIZA_CLOUD_AUTH_BASEScopy). Drift between these hand-synced copies was exactly the staging sign-in-loop bug — staging existed in two of them but not the third. Adding a future env host is now a one-line, one-file change. Thepackages/cloud/sharedcopy stays:uican't depend on the server bundle (documented boundary port).The session-sync keep-path retries. When a 401 hits a still-valid token, the handler now resets the sync dedupe marker, so the next trigger (visibility/storage/re-render) re-attempts the HttpOnly cookie POST once the endpoint heals — previously that token would ride out its whole lifetime with no cookie ever established.
Exp-less tokens are clearable.
tokenIsExpirednow treats a missingexpclaim as expired. Since the 401 handlers keep any non-expired token, an exp-less token (foreign/malformed — Steward always mintsexp) would otherwise be uncloseable: no 401 could clear it and it never ages out.The load-bearing 401 behavior is tested. New
StewardProviderRuntime.test.tsxexercises the realAuthTokenSyncagainst a stubbed fetch (only the@stwdSDK boundary is mocked): a valid token survives session+refresh 401s AND the cookie sync retries on the next trigger; expired and exp-less tokens still clear. Plus direct unit tests for the newtokenIsExpiredsemantics. Includes an in-memoryStorageshim because Node ≥22's barelocalStorageglobal is non-functional under vitest and shadows jsdom's (the same environmental issue that failsCloudRouterShell.test.tsxlocally on develop).Test
bunx vitest run packages/ui/src/cloud/shell/ packages/ui/src/cloud/public-pages/pages/auth/email-callback-page.test.tsx→ my 4 suites pass (16 tests);CloudRouterShell.test.tsxfails identically on clean develop (pre-existing Node 25 localStorage env issue, untouched here)bun run --cwd packages/ui typecheck→ clean