SCAL-319970 [SAFETY REVERT] Revert multi-org feature (pre-approved rollback for #176) - #181
Open
rohitthughtspot wants to merge 22 commits into
Open
SCAL-319970 [SAFETY REVERT] Revert multi-org feature (pre-approved rollback for #176)#181rohitthughtspot wants to merge 22 commits into
rohitthughtspot wants to merge 22 commits into
Conversation
Applies the multi-org feature on top of merged main (thoughtspot#165 auth extraction), reconciled with intervening main work: - 3-way merged types.ts and tool-definitions.ts so thoughtspot#171's answer_data_source_id coexists with the org types/tools (a blind copy of the pre-thoughtspot#171 branch had reverted it, breaking streaming-utils.ts). - Auth-adjacent bits ride the @thoughtspot/mcp-auth hooks in index.ts: authMode (enrichMcpRequestProps -> oauth, extendProps -> bearer/token), extendGrantProps carrying refreshToken/expiry into the grant for keep-warm, UserTokenStore export. - All org logic (list_orgs/switch_org, OrgService, UserTokenStore DO, keep-warm, isolation/fan-out/F2a/F4/T3 + tests) unchanged app-side. Requires @thoughtspot/mcp-auth with the extendGrantProps hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…til re-auth) A grant minted before multi-org shipped has no refresh token in its props (extendGrantProps adds it at login). areOrgToolsAvailable() now also requires hasMultiOrgGrant() (refreshToken present), so such sessions: - do NOT see list_orgs/switch_org, and - get NO org overlay/minting — they keep the pre-multi-org behavior (login token, no forced re-mint) until the user re-authenticates. This fixes the observed break where an old grant hit "authentication expired" because the org overlay tried to mint against a stale global token. Tests cover both: old grant hides org tools and applies no overlay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ken presence) Move the pre/post-multi-org grant check to a single init-time decision: postInit sets grantHasRefreshToken = (props.refreshToken present), and hasMultiOrgGrant() reads that flag. init() awaits postInit before any tool call or listTools, so the flag is always set first (no race). Everything multi-org keys off this one decision: tool visibility, the connect overlay, and the per-request org load. A pre-multi-org grant (no refresh token) keeps the old no-org behavior until re-auth. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…session-aware) Locks the backward-compat contract: org selection is shared among re-authenticated sessions (per-user via hash(refreshToken)), but a still-legacy grant (no refresh token) must keep working as-is until re-auth. Even sharing the same store, an old grant resolves no active org and keeps using its login token after a new session's switch_org. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ests - Depend on @thoughtspot/mcp-auth ^2.0.0 (adds the extendGrantProps hook that carries the refresh token into the OAuth grant for keep-warm). - Tests: listOrgs field-mapping edges (id/name fallback, id-as-name, empty description -> undefined) and switch_org non-4xx (5xx) -> generic retry error, not "no access", and a no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…over props In loadOrSeedWarmToken, when there's no refresh token to re-seed with, prefer the stored access token only if it isn't expired; otherwise fall back to the (possibly fresher) props token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Load warm token before initializeService so getSessionInfo uses the keep-warm token instead of the short-lived props access token - Preserve lastSeenAt across re-seeds (only first seed + touch set it), so idle TTL tracks real tool-call activity not reconnects Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…clean up seedTokenStore - loadOrSeedWarmToken now reads the stored token FIRST; if it is a different (alarm-refreshed) token that is not expired, it wins over the incoming props token without any write at all. Removes the per-token-string comparison that was inside seedTokenStore, which the mock could not replicate. - seedTokenStore is now a plain write (no merge logic): the decision of which token to use has already been made by the caller. - Reverts TOKEN_REFRESH_INTERVAL_MS (11h) and SESSION_IDLE_TTL_MS (14d) from test values back to production. - Fixes broken fallback branch that referenced undefined `store` and `storedExpired` variables. - All 628 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The initializeService override that pre-loaded the warm token before getSessionInfo was not fixing the configInfo error (which is pre-existing and unrelated to this branch). Removes the override and restores postInit to call loadOrSeedWarmToken unconditionally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…g hint - is_active field only included in response when true (saves tokens at scale) - ListOrgsOutputSchema updated to optional() to match - get_session_updates tool description: mention wrong org or no access when data not found Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… fixes Naming changes (Rifdhan feedback): - warmGlobalToken → globalToken; TokenStore → GlobalTokenData - accessToken/refreshToken fields → globalToken/globalRefreshToken - token-store route/key → global-token-data/global-token-details - active-org route → active-org-id-and-token; active-org-id storage key - touch route → last-seen; TOKEN_STORE_KEY → GLOBAL_TOKEN_KEY - ensureOrgToken → getOrRecreateActiveOrgToken - forceRemintOrgToken → forceRecreateActiveOrgToken - refreshTokenStore → refreshGlobalToken - Remove isOAuthAuth() and getGlobalToken() wrappers, inline directly Review design fixes: - Make getActiveBearerToken/getActiveOrgId abstract in BaseMCPServer - Remove withOrgTokenRetry; callers use getThoughtSpotService() directly - Remove per-callTool upfront org token mint; minting stays in postInit - forceRecreateActiveOrgToken: drop orgId param, use this.activeOrgId - getOrRecreateActiveOrgToken: use setActiveOrg(orgId, token) not setActiveOrgToken - seedGlobalToken: skip write if stored token is unchanged (avoid spurious DO writes) - add get_session_updates wrong-org hint in tool description - list_orgs: is_active field only present when true Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dant org-id write - getActiveBearerToken → getActiveOrgToken in base class and MCPServer - POST /active-org-id-and-token: skip put+delete when org-id is unchanged (same org re-set on every cold connect no longer writes to storage) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d write Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eActiveOrgToken on connect getOrRecreateActiveOrgToken is no longer called anywhere — the only mint paths are postInit (forceRecreateActiveOrgToken when token missing) and callSwitchOrg (inline). Remove it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The keep-warm alarm now re-mints the active org token from the freshly refreshed global token, so the org token stays as warm as the global token and never expires mid-session. Org-token validity dropped 30d -> 24h to match. On a mint failure the existing org token is kept (the next 11h alarm retries); there is deliberately no reactive re-mint / retry-on-401 path — a genuinely expired token only surfaces after 14 idle days, as an expected 401. Adds tests for the re-mint, the no-active-org skip, and graceful handling of 200-but-no-token responses on both the global refresh and the org mint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename getActiveOrgToken -> getActiveToken: it returns the org-scoped token when an org is active, else the global/cluster token, so the old name overpromised org-scope. - Inline hasMultiOrgGrant() into its sole caller areOrgToolsAvailable(). - forceRecreateActiveOrgToken no longer reloads the active org; it mints for the currently-set this.activeOrgId (both callers already set it first). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…org-token flow Storage split: - Move the org/token surface out of StorageServiceClient into a dedicated OrgStorageServiceClient (src/storage-service/org-storage-service.ts); the Spotter conversation storage and the per-user org/token store are now separate clients with their own factories (getStorageService / getOrgStorageService). Drop the dead setActiveOrgToken method + its route. Shared token endpoints (de-dup): - New src/thoughtspot/token-endpoints.ts owns the org-token mint + global-token refresh (endpoint paths, headers, response shape, ORG_TOKEN_VALIDITY_SEC). Both the SDK client (fetchOrgBearerToken) and the keep-warm DO call it, so a change propagates to the interactive and background paths instead of drifting. Naming to the global* convention: - Props: refreshToken/tokenCreatedTime/tokenExpiryDuration -> globalRefreshToken/globalTokenCreatedAt/globalTokenExpiresAt. - getActiveOrgToken -> getActiveToken; loadOrSeedWarmToken -> initGlobalTokenAndReconcileWithStorage; refreshGlobalToken -> refreshGlobalAndOrgTokens. Active-org wire field orgToken -> activeOrgToken. Correctness / robustness: - callTool preamble re-mint now catches a failed mint (e.g. lost org access 403) and returns a graceful error instead of a raw JSON-RPC throw. - Ping now awaits validateConnection() (was reporting success unconditionally). - postInit mints before persisting the active org id, so a failed mint can't leave a tokenless active org that poisons later calls. - Simplify seedGlobalToken: arm the alarm only when storing a new token. OAuth manual-paste: require the full gettoken JSON (token + refreshToken + timestamps); drop the token-only paste paths that would disable keep-warm/orgs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cile; naming - getActiveToken / reseed: never seed or serve an expired grant access token (frozen at login, never refreshed). Fail closed with a 401 that callTool's central catch turns into a graceful reauthenticate response. Non-numeric expiry (bearer) is treated as not-expired, so bearer is unaffected. - Non-orgs OAuth: reconcile the global token from the DO per call, so a long-lived instance picks up the keep-warm alarm's refreshed token instead of a stale in-memory copy. Org sessions unchanged (they use the org token; list/ switch reconcile inline) — no duplicate DO reads. - Extract MCPServer.isExpired helper (dedup the expiry check). - Rename for clarity: seedGlobalToken -> storeGlobalTokenData; the client methods mirror their request paths (getActiveOrgIdAndToken / setActiveOrgIdAndToken / getGlobalTokenData / setGlobalTokenData / setLastSeen); ACTIVE_ORG_ID -> STORAGE_STUB_ID. - Revert storage-service.ts to its pre-orgs form (org/token surface lives in OrgStorageServiceClient; the diff was comment-only). - Tests for the expired-token fail-closed paths and the non-orgs per-call global reconcile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…op accessToken fallback initGlobalTokenAndReconcileWithStorage now returns the resolved global token and throws 401 when no valid token exists, instead of setting this.globalToken to undefined. This removes the `this.globalToken ?? this.ctx.props.accessToken` fallback at the three mint/list sites (forceRecreateActiveOrgToken, callListOrgs, callSwitchOrg) that would otherwise resurrect an expired grant token and fire a pointless upstream request with a dead bearer. The throw fails at the source. The non-orgs OAuth per-call reconcile is moved inside callTool's central try so its fail-closed 401 is caught and rendered as a graceful reauth response rather than escaping as a raw JSON-RPC error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pre-built revert of the multi-org feature (PR thoughtspot#176) so it can be reviewed and approved BEFORE merge — giving an instant, pre-approved rollback path if something goes wrong in prod after the feature ships. Removes the entire org footprint: list_orgs/switch_org tools, the per-user UserTokenStore keep-warm Durable Object, org/token storage service, shared token endpoints, and all org changes to mcp-server, tool-definitions, thoughtspot-client/service, index, utils, and the oauth-callback assets. All touched source/test files return to their main state. wrangler.jsonc is NOT restored to main: the v8 migration (which creates UserTokenStoreSQLite) is kept and a new v9 deleted_classes migration is added to tear the DO class down. Removing v8 outright would break deploys where it already ran — Cloudflare requires a deletion migration, not un-declaration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the @thoughtspot/mcp-auth dependency to version ^2.0.1 in package.json and package-lock.json, and adds SQLite migration tags (v8 and v9) for UserTokenStoreSQLite in wrangler.jsonc. However, the package-lock.json file incorrectly references a local relative path dependency (../ts-mcp-auth), which will cause build or installation failures in clean CI/CD and production environments. It is recommended to regenerate the lockfile using the registry-resolved dependency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+53
to
+71
| "../ts-mcp-auth": { | ||
| "name": "@thoughtspot/mcp-auth", | ||
| "version": "2.0.1", | ||
| "extraneous": true, | ||
| "license": "ThoughtSpot End user license agreement", | ||
| "devDependencies": { | ||
| "@biomejs/biome": "^1.9.4", | ||
| "@cloudflare/workers-oauth-provider": "^0.0.5", | ||
| "@cloudflare/workers-types": "^4.20250612.0", | ||
| "@types/node": "^22.15.3", | ||
| "hono": "^4.12.21", | ||
| "typescript": "^5.8.3", | ||
| "vitest": "^3.1.2" | ||
| }, | ||
| "peerDependencies": { | ||
| "@cloudflare/workers-oauth-provider": "^0.0.5", | ||
| "hono": "^4.12.21" | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
The lockfile contains a reference to a local relative path dependency ../ts-mcp-auth with "extraneous": true. This local path will not exist in clean CI/CD or production deployment environments, which will cause npm ci or npm install to fail. Please regenerate the lockfile after installing the package from the registry to ensure it points to the correct registry-resolved dependency.
Rifdhan
marked this pull request as ready for review
July 10, 2026 18:33
Rifdhan
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Pre-built, reviewable revert of the multi-org feature (#176), opened before #176 merges so it can be reviewed and approved now — giving an instant, pre-approved rollback path if something goes wrong in prod after the feature ships.
This is a separate PR and does not touch #176 or its branch.
What it does
Removes the entire org footprint and returns every touched source/test file to its
mainstate:list_orgs/switch_orgtoolsUserTokenStoreSQLitekeep-warm Durable Objectmcp-server,tool-definitions,thoughtspot-client/service,index,utils, and theoauth-callbackassetsMigration safety (
wrangler.jsonc) — read thiswrangler.jsoncis not restored tomain. Thev8migration that createsUserTokenStoreSQLiteis kept, and a newv9deleted_classes: ["UserTokenStoreSQLite"]migration is added to tear the DO down.Removing
v8outright would break deploys where it already ran — Cloudflare requires a deletion migration, not un-declaration of an applied one. So the revert adds v9 rather than dropping v8.Verification
mainexceptwrangler.jsonc(the intentional v9 migration).tsc: 27 errors (matchesmainbaseline; the feature's extra error is gone).🤖 Generated with Claude Code