fix(cli): honor profiles and refresh analytics tokens - #84
Conversation
e54f00d to
34db2ae
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| requestInfo(url, route, argv, studio.tenantId, response.status, responseRequestId(text)), | ||
| const context = ctx ?? await resolveAnalyticsContext(argv) | ||
| const endpoint = context.endpoint | ||
| const url = buildUrl(endpoint, route, argv, query, context.studio.tenantId) |
There was a problem hiding this comment.
HIGH (confidence: high) — the retried request carries a stale tenantId in its query string.
const url = buildUrl(endpoint, route, argv, query, context.studio.tenantId)
let didRefresh = false
while (true) {
const studio = context.studiourl is built once, before the loop, from the pre-refresh tenant. context.refresh() then replaces context.studio wholesale — including tenantId, which moves from the [profiles.<n>.agent].tenant_id value to getCurrentUser()'s accountId. The retry re-derives headers and requestBody from the new studio but reuses the old url, so the second request sends two different tenants: the old one in ?tenantId=, the new one in the tenantId/accountId headers (and in the openSessionAuth body).
This is not a narrow route: buildUrl appends tenantId unless route.tenantIdQuery === false, and no entry in ROUTES sets that, so it applies to every analytics-agent call.
The PR's own new test exercises exactly this divergence without asserting on it — [profiles.test.agent] tenant_id = 55 while stubStudioContext's DEFAULT_CONTEXT.tenantId is 10 (test/support/cz-fixtures.ts:97), so the retry issues ?tenantId=55 with tenantId: 10 headers and still passes because only x-clickzetta-token is checked.
requestInfo(url, route, argv, studio.tenantId, ...) on line 720 inherits the same split — the reported tenantId won't match the query it reports alongside it.
Moving the buildUrl call inside the loop (below const studio = context.studio) fixes both, and costs nothing on the non-retry path.
| function responseIndicatesExpiredToken(responseBody: string): boolean { | ||
| return /(?:token|authorization).*(?:expired|invalid)|(?:expired|invalid).*(?:token|authorization)/i.test(responseBody) | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: medium-high) — deciding "should I refresh?" by regex-matching English prose in the response body is a heuristic layered on top of a signal that is already structured.
function responseIndicatesExpiredToken(responseBody: string): boolean {
return /(?:token|authorization).*(?:expired|invalid)|(?:expired|invalid).*(?:token|authorization)/i.test(responseBody)
}Three concrete ways it misses:
- No
s/dotAllflag, so.does not cross newlines. A pretty-printed body with"code": "401"on one line and"message": "token expired"on another matches only if both keywords land on the same line. Nothing guarantees the gateway does not pretty-print. - The backend returns localized messages elsewhere in this file's surface area (
ai_message, business errors).令牌已过期/凭证无效does not match, and the fallback silently does not happen — the user gets the raw 401. - It matches anywhere in the body, including inside a nested payload that merely mentions a token, so it can also fire on a 401 that is a genuine authorization denial.
The structured signal is right there: this file already has extractBusinessError(payload), and the sibling test on line ~258 stubs { code: "401", message: "token expired" }. Keying on the response's business code (or simply on response.status === 401, which is what the other branch does) would be deterministic.
Related — the asymmetry between the two branches is undocumented and looks accidental:
context.refreshOn401 = responseIndicatesExpiredToken // profile-[agent] branch
...
context.refreshOn401 = () => true // getStudioContext branchIf the reason the [agent] branch needs a stricter gate is "refreshing here switches identity (agent token → OAuth session), so only do it when the token is genuinely dead", that is worth a comment — it is the load-bearing fact and nothing in the code says it. If there is no such reason, both branches should use the same predicate.
| function selectedProfile(argv: GlobalArgs, profiles: Record<string, Record<string, unknown>>, defaultProfile: string | undefined): string | undefined { | ||
| const requested = typeof argv.profile === "string" && argv.profile.length > 0 ? argv.profile : defaultProfile | ||
| return requested && profiles[requested] ? requested : undefined | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — this re-derives "which profile is active" instead of calling the function that already owns that formula, and drops one of its two tiers.
const requested = typeof argv.profile === "string" && argv.profile.length > 0 ? argv.profile : defaultProfileProfile.current() (src/connection/profile-context.ts:49) is ConnectionEnv.profileName() ?? getDefaultProfileName() — i.e. CZ_PROFILE, falling back to default_profile. Its docstring is explicit about why it exists:
Both steps live here because call sites that spelled the order themselves drifted — one read only the default and retargeted a
--profile Bsession at another tenant.
selectedProfile is that same drift with the CZ_PROFILE tier missing. And --profile does not need special handling here: runCli already pins it into CZ_PROFILE before any handler runs (src/run-cli.ts:771, ConnectionEnv.pin(profileOverride)), so replacing data.default_profile with Profile.current() fixes the reported bug for --profile and for CZ_PROFILE, with no new helper.
The gap is reachable, and it is the same class of bug this PR is fixing. Profile.set() (TUI /profile switch) writes CZ_PROFILE, which child processes inherit — so after switching profiles in the TUI, the agent shelling out to cz-cli auth status gets the default profile's session reported back, while cz-cli sql in the same shell connects as the switched one. That is exactly the "verification reported the wrong profile" failure the new system-prompt line on agent-system-prompt.ts:181 is trying to steer around.
Neither new test in test/auth-profile-override.test.ts covers CZ_PROFILE — both pass --profile explicitly.
Note also that argv.profile naming a non-existent profile cannot reach here: runCli reports PROFILE_NOT_FOUND at the boundary for any explicitly typed name (run-cli.ts:752-763). So the && profiles[requested] guard only ever changes behavior for a dangling default_profile — see the separate comment on runList.
| const profiles = (data.profiles ?? {}) as Record<string, Record<string, unknown>> | ||
| const activeProfile = typeof data.default_profile === "string" ? data.default_profile : undefined | ||
| const defaultProfile = typeof data.default_profile === "string" ? data.default_profile : undefined | ||
| const activeProfile = selectedProfile(argv, profiles, defaultProfile) |
There was a problem hiding this comment.
LOW (confidence: high on the mechanism, unsure on intent) — please confirm this output-shape change is deliberate.
const activeProfile = selectedProfile(argv, profiles, defaultProfile)selectedProfile returns undefined when the resolved name is not a key in [profiles]. When default_profile names a profile that no longer exists (deleted by auth logout, or hand-edited), auth list --format json now reports "active_profile": null where it previously reported the dangling name. active_session was already null in that case, so this is the one field that moves.
runStatus is unaffected — it already had the if (!activeProfile || !profiles[activeProfile]) guard below.
Reporting null is arguably the better answer, but it is a change to a documented JSON field that scripts may read, and no test covers the dangling-default_profile case in either subcommand. Worth either a line in the docstring or a test pinning it, so a future reader knows it was chosen rather than inherited from the profiles[requested] guard.
| if (response.status === 401 && !didRefresh && context.refresh && (context.refreshOn401?.(text) ?? false)) { | ||
| didRefresh = true | ||
| context.refreshPromise ??= context.refresh().finally(() => { | ||
| context.refreshPromise = undefined | ||
| }) | ||
| await context.refreshPromise | ||
| continue | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — a failure inside the refresh replaces the 401's diagnostics with the refresh's own error.
if (response.status === 401 && !didRefresh && context.refresh && (context.refreshOn401?.(text) ?? false)) {
didRefresh = true
context.refreshPromise ??= context.refresh().finally(() => {
context.refreshPromise = undefined
})
await context.refreshPromise
continue
}context.refresh() can throw for reasons unrelated to the original 401 — forceRefreshToken rethrows transient network/5xx failures verbatim (clickzetta-sdk/src/auth/token.ts:126-128), and the getStudioContext call that follows it makes three more network round trips (getCurrentUser, resolveInstanceIdByName, getWorkspaceByName). When any of those fails, the AnalyticsHttpError that would have been thrown is never constructed, so:
- the
extra: { request: err.request }block at the ~15 call sites (e.g. line 1000, 1125, 1308) has nothing to attach — the user loses path/query/tenantId/requestId, which is the only handle for correlating with the backend; requestStatusCommandWithFallbackloses its dispatch:if (!(err instanceof AnalyticsHttpError)) throw err(line 1094) rethrows instead of consulting(err.request.status ?? 0) < 500;applyStatusChangeBatch's per-itemcatch(line 1281) records the refresh error as that item's failure reason.
A network blip during the refresh turns a diagnosable "HTTP 401: token expired" into an opaque message. Wrapping the refresh so a refresh failure falls through to throwing the original AnalyticsHttpError (with the refresh error as cause) keeps both facts.
Separately, on the same block: the refreshPromise coalescing has no caller that can observe it. Every shared-ctx path in this file is a sequential await — the poll loop (line 963), the batch loop (line 1271-1288), the pagination loop (line 1328) — and there is no Promise.all/allSettled anywhere in the file. didRefresh is already the per-call guard, so context.refreshPromise is always undefined when read. Either drop it, or add a comment naming the concurrent caller it is defending against.
| const profileAgent = getProfileAgentContext(argv) | ||
| if (profileAgent) { | ||
| const config = resolveConnectionConfig(argv) | ||
| const context: ResolvedContext = { endpoint: endpoint!, studio: profileAgent } | ||
| if (config.tokenStore && !hasCookieToken(config)) { |
There was a problem hiding this comment.
LOW (confidence: medium) — two things to confirm in this branch.
const profileAgent = getProfileAgentContext(argv)
if (profileAgent) {
const config = resolveConnectionConfig(argv)
const context: ResolvedContext = { endpoint: endpoint!, studio: profileAgent }
if (config.tokenStore && !hasCookieToken(config)) {1. resolveConnectionConfig runs twice here. getProfileAgentContext already calls it internally (commands/studio-context.ts:135) to build the context it just returned. It is a pure re-read (TOML parse + env merge), so this is cost and a second chance to diverge, not a correctness bug — but the config is already available one frame up if you'd rather return it.
2. The config.tokenStore gate is asymmetric with the branch below. This branch requires a token store; the getStudioContext branch (line 656) requires only !hasCookieToken(config).
tokenStore is attached only when the profile has an OAuth pointer and no explicit credential and auth_type permits oauth (connection/config.ts:219-222). So a profile carrying an [agent] block plus a pat gets no refresh path at all — its expired agent token stays fatal — while the same profile with the [agent] block removed would refresh via a full PAT re-login. That is an odd shape for the feature to have: the profiles most likely to carry a hand-pasted legacy agent token are also the ones most likely to be PAT-based.
If the gate is deliberate ("only fall back to an OAuth session, never to a PAT login"), a one-line comment would carry it. If not, !hasCookieToken(config) alone matches the other branch and forceRefreshToken handles the PAT case fine (acquireToken(config, true) → no store → full login).
34db2ae to
80e7b3d
Compare
| // A cookie-pinned profile has no credential to re-exchange — its token IS the | ||
| // cookie — so leave `refresh` unset and let a 401 surface. Everything else | ||
| // (OAuth session, PAT, password) can mint a fresh token via forceRefreshToken. | ||
| if (!hasCookieToken(config)) { | ||
| context.refresh = async () => { | ||
| await forceRefreshToken(config) | ||
| // For a profile carrying an [agent] block this deliberately swaps identity: | ||
| // the hand-pasted legacy agent token is what the server just rejected, so | ||
| // the profile's own login takes over the session from here. | ||
| context.studio = await getStudioContext(argv, { allowMissingWorkspace: true }) | ||
| } | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — the guard tests the wrong property, so a credential-less profile pays a ~6 s dead-end login on every 401.
if (!hasCookieToken(config)) {
context.refresh = async () => {
await forceRefreshToken(config)"not a cookie" is not the same as "can mint a fresh token". Trace a profile that carries only a [profiles.<n>.agent] block — no oauth = "<id>" pointer, no pat, no username/password — which is exactly the shape getProfileAgentContext exists to serve:
resolveConnectionConfigattachescfg.tokenStoreonly when the profile has an OAuth pointer or a legacy inline subtable (connection/config.ts:219), so there is no store here.forceRefreshToken→acquireToken(config, true)→ no store, no candidate →fetchToken(config)(clickzetta-sdk/src/auth/token.ts:181-184).config.patis"", so that lands onloginWithPassword(baseUrl, "", "", instance)(token.ts:46-53;DEFAULT_CONNECTIONseedsusername/passwordas empty strings).loginWithRetryretries every non-InterfaceErrorfailure — 6 attempts with 200/400/800/1600/2000 ms of backoff (auth/login.ts:105-181).
So each 401 costs ~5.2 s of sleeps plus six round trips and then gets swallowed by the catch below, and the user is shown the same 401 they would have seen instantly. The SDK already names the condition you want: hasLoginCredentials (token.ts:40-42) documents this exact dead end ("wastes ~6s of retries and ends in a misleading 'Login failed'"). Gating on "config has a tokenStore, or a pat, or a username+password" rather than on the cookie check would express the real precondition.
| } catch { | ||
| // The refresh itself failed (network, revoked credential, workspace | ||
| // lookup). The 401 below is the diagnosis the caller asked for, so | ||
| // report that rather than the refresh's own error. | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — this discards the one error that tells the user how to recover.
} catch {
// The refresh itself failed (network, revoked credential, workspace
// lookup). The 401 below is the diagnosis the caller asked for, so
// report that rather than the refresh's own error.
}The premise holds for a network blip, but not for the dominant case. When the refresh token is dead the SDK raises SESSION_EXPIRED whose message exists precisely to be shown: OAuth session expired and could not be refreshed (…). Run `cz-cli auth login <name>` to sign in again. (clickzetta-sdk/src/auth/token.ts:65-71, thrown at :125). Swallowing it leaves the user with HTTP 401: token expired and no route forward — which is the state this PR is trying to get them out of. Attaching the refresh failure to the thrown AnalyticsHttpError (as extra, or as a second line in the message) keeps the 401 as the primary diagnosis without dropping the actionable half.
Secondary, lower confidence: the catch also swallows control flow that has already written output. handledError calls error() — which does process.stdout.write(...) (src/output/index.ts:132) — before throwing. Any handledError reached inside context.refresh() would therefore emit one error document to stdout, get swallowed here, and then the 401 emits a second one, giving two JSON documents on stdout for a single command. allowMissingWorkspace: true skips the three handledError branches in getStudioContext today, so I could not construct a live path — but the catch is what makes it silent if one is ever added.
| if (response.status === 401 && !didRefresh && context.refresh) { | ||
| didRefresh = true | ||
| try { | ||
| context.refreshPromise ??= context.refresh().finally(() => { | ||
| context.refreshPromise = undefined | ||
| }) | ||
| await context.refreshPromise |
There was a problem hiding this comment.
LOW (confidence: medium) — didRefresh is per-call, so "refresh at most once" holds per request but not per context.
if (response.status === 401 && !didRefresh && context.refresh) {
didRefresh = true
try {
context.refreshPromise ??= context.refresh().finally(() => {
context.refreshPromise = undefined
})refreshPromise lives on ctx but is cleared as soon as it settles, and every caller sharing that ctx arrives with a fresh didRefresh = false. Callers that issue many sequential requests against one context — the poll loop at analytics-agent.ts:957, the pagination loop at :1239, the batch loop below it — will therefore attempt a refresh on each request that 401s. Against a server that keeps rejecting the freshly minted token, a default 360 s session run does one extra request and one OAuth refresh-token rotation per 2 s poll.
No infinite loop (didRefresh bounds each individual request at two attempts), so this is a cost/rotation-churn issue rather than a hang. If the intended guarantee was "one refresh per resolved context", the flag belongs next to refreshPromise on ResolvedContext.
Separately: I checked every requestAnalytics/requestAnalyticsData call site that shares a ctx and they are all sequential — no Promise.all — so the ??= coalescing is currently unreachable defensive code rather than something load-bearing.
| // For a profile carrying an [agent] block this deliberately swaps identity: | ||
| // the hand-pasted legacy agent token is what the server just rejected, so | ||
| // the profile's own login takes over the session from here. | ||
| context.studio = await getStudioContext(argv, { allowMissingWorkspace: true }) |
There was a problem hiding this comment.
Question — please confirm the intent (MEDIUM, confidence: medium). Not asserting a bug; the deliberate part is documented, but I do not think the mid-flight case is.
// For a profile carrying an [agent] block this deliberately swaps identity:
// the hand-pasted legacy agent token is what the server just rejected, so
// the profile's own login takes over the session from here.
context.studio = await getStudioContext(argv, { allowMissingWorkspace: true })Swapping identity when the first request 401s is defensible. But ResolvedContext is shared across a whole multi-request flow, and the new test confirms the swap moves tenantId (55 → 10), which buildUrl puts in the query string. So a refresh that fires partway through a flow retargets the remainder at a different tenant than the part that already ran:
executeSessionRunCommand(:936-960) resolvesctx, gets aquestionIdfromsessionRununder tenant 55, then pollssessionResultwith the samectx. A 401 on a poll swaps to tenant 10 and keeps polling for aquestionIdthat was minted in 55. The retried poll returns 200, soisTerminalResponsedecides what happens next — if the other tenant answers with a non-terminal shape rather than a business error, thedo/whilekeeps polling to the 360 s deadline.runBatchStatusChange(:1228-1239) andlistAllDatasets(:1322) paginate under onectx; a mid-loop swap means pages 1..n came from one tenant and n+1.. from another, silently merged into one result set.uploadKnowledgeFile(:1656-1683) requests a presigned URL, PUTs the bytes, then callsknowledgeUploadCompletewith anodeIdfrom the first tenant.
Was the intent to allow the swap only on the first request of a context (i.e. during resolution), and to let a 401 on a later request surface instead? That would keep the fix for the reported symptom while removing the split-identity flows.
| * Hide the inherited global connection flags from `auth` help. `auth` manages | ||
| * sessions, not a connection you read: --profile selects which profile to READ | ||
| * (irrelevant — auth operates on sessions/tokens), and jdbc/service/instance/… | ||
| * describe a connection auth doesn't consume. Hidden (not removed) so parsing | ||
| * stays intact; `auth login` re-shows the few flags it genuinely uses. | ||
| * sessions rather than making a data request; --profile is still accepted so | ||
| * list/status can report the selected profile, while jdbc/service/instance/… | ||
| * describe connection details auth does not consume. Hidden (not removed) so | ||
| * parsing stays intact; `auth login` re-shows the few flags it genuinely uses. |
There was a problem hiding this comment.
LOW (confidence: high) — the docstring now says --profile is meaningful for list/status, but the code below still hides it, so cz-cli auth status --help does not mention it (auth.ts:46: .option("profile", { hidden: true })).
* sessions rather than making a data request; --profile is still accepted so
* list/status can report the selected profile, …
That matters more than usual here because agent-system-prompt.ts was changed in this same PR to instruct the model to pass --profile on auth status/auth list. A model that consults --help before composing the command sees no such flag. The other seven options in the list genuinely are unconsumed by auth; profile no longer is. Consider un-hiding it, or re-showing it on the list/status builders the way auth login re-shows its credential flags.
| function selectedProfile(argv: GlobalArgs): string | undefined { | ||
| if (typeof argv.profile === "string" && argv.profile.length > 0) return argv.profile | ||
| return Profile.current() | ||
| } |
There was a problem hiding this comment.
LOW / behavior change worth confirming (confidence: high on the mechanism, medium on whether it is intended).
function selectedProfile(argv: GlobalArgs): string | undefined {
if (typeof argv.profile === "string" && argv.profile.length > 0) return argv.profile
return Profile.current()
}Profile.current() is ConnectionEnv.profileName() ?? getDefaultProfileName() (connection/profile-context.ts:49-51), so this changes active_profile in two ways, not one. The --profile half is the fix and is covered by the new test. The CZ_PROFILE half is a second, untested change: any environment that exports CZ_PROFILE now gets a different active_profile / active_session / logged_in from cz-cli auth status --format json than it did before, with no flag on the command line. That includes the agent runtime, where run-cli.ts:758 (if (profileOverride) ConnectionEnv.pin(profileOverride)) pins it for the whole process. Probably the point of the PR — worth stating so it is a decision rather than a side effect.
Two smaller notes:
- Because
run-cli.tsalready expands--profileintoCZ_PROFILEbefore the command runs, the explicit-argv branch is redundant on the real CLI path; both branches return the same name. It is load-bearing only for library-style calls. Fine as written, just not doing the work the new test appears to attribute to it. - No test covers the
CZ_PROFILEpath, and the pre-existingtest/auth-list-format.test.tsspawns withenv: { ...process.env, … }, so aCZ_PROFILEpresent in the runner's environment would now flip itsexpect(lines[0].startsWith("uat\ttrue\t"))assertion. UnsettingCZ_PROFILEin both spawn helpers would make the suite independent of the ambient environment. I cannot run the tests, so this is from reading the fixture, not from an observed failure.
|
Review summary A. Upstream invasiveness — no issues found This PR touches nothing under New import edges are all intra-package ( B. Clean fix vs. hole drilled around the problem — mostly the right fix, two findings The core decision is right and worth saying so: the 401 retry goes into Two findings, both about guards/fallbacks rather than the structure:
Plus one lower-severity structural note: No dead code, no commented-out code, no leftover debug logging, no copy-pasted logic. The C. Regression risk Behavioral changes I found, and their test coverage:
One coverage gap not tied to a specific line: the retry is exercised only against I could not run the build or the test suite in this environment, so nothing above is a claim that any test passes. |
80e7b3d to
3a32a61
Compare
| function canRefreshCredential(config: ConnectionConfig): boolean { | ||
| if (hasCookieToken(config)) return false | ||
| return Boolean(config.tokenStore || config.pat || (config.username && config.password)) | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: medium-high) — config.tokenStore is not proof that a refresh has "anything to re-exchange", so this guard still admits the ~6 s dead end its docstring says it prevents.
function canRefreshCredential(config: ConnectionConfig): boolean {
if (hasCookieToken(config)) return false
return Boolean(config.tokenStore || config.pat || (config.username && config.password))
}The docstring says this "Mirrors the SDK's own hasLoginCredentials guard" — but that function is Boolean(config.pat || (config.username && config.password)) (clickzetta-sdk/src/auth/token.ts:40-42); it deliberately excludes the token store. tokenStore is attached whenever the profile has any oauth = "<id>" pointer (connection/config.ts:219-222), regardless of whether that pointer resolves.
When it does not resolve, makeProfileTokenStore(...).load() returns undefined (profile-store.ts:648, 657-658, 669-672 — missing [oauth.<id>] section, or an unparseable/corrupt file). acquireToken then finds no candidate, skips refreshOrLogin entirely, and goes straight to fetchToken(config) → loginWithPassword(baseUrl, undefined, undefined, instance) (token.ts:181-185, 44-53). That is the credential-less login token.ts:80-83 warns about: "We must NOT attempt a password login with empty credentials (it wastes ~6s of retries and ends in a misleading 'Login failed')". The SESSION_EXPIRED short-circuit that normally prevents it only exists on the refreshOrLogin path, which is never reached without a candidate.
Failure scenario: a profile whose [oauth.<id>] section was pruned by pruneOrphanOAuthSections while profiles.<n>.oauth still points at it. An Analytics Agent 401 now spends ~6 s of login retries and reports HTTP 401: … \nToken refresh also failed: Login failed …, where before it failed immediately with the server's own message.
Gating on a loadable token rather than a configured store closes it, e.g. config.tokenStore?.load() !== undefined || config.pat || (config.username && config.password). load() is synchronous and file-local, so it fits the "cheap, synchronous check" shape this guard is going for.
| const oauth = (data.oauth ?? {}) as Record<string, OAuthEntry> | ||
| const profiles = (data.profiles ?? {}) as Record<string, Record<string, unknown>> | ||
| const activeProfile = typeof data.default_profile === "string" ? data.default_profile : undefined | ||
| const activeProfile = selectedProfile(argv) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — with an inherited CZ_PROFILE, this change can make auth status report "not logged in" on a machine that is logged in, and it drops the one field that would explain why.
const activeProfile = selectedProfile(argv)
if (!activeProfile || !profiles[activeProfile]) {
return success({ logged_in: false, active_profile: null }, { format })
}selectedProfile's docstring (line 96–98) justifies returning a nonexistent name because "both callers look the name up and handle the miss, and reporting it keeps a dangling default_profile visible instead of rendering as 'no profile at all'". runStatus does the opposite — it collapses the name to null, which is "no profile at all".
That matters because auth is deliberately not in PROFILE_REQUIRED_COMMANDS (run-cli.ts:30-43). The comment at run-cli.ts:738-745 is explicit that the exemption exists so a stale exported CZ_PROFILE does not lock users out of "profile create, profile use, auth list". So an inherited CZ_PROFILE reaches selectedProfile unvalidated, and auth status/auth list now follow it:
export CZ_PROFILE=deleted_profile # removed by `auth logout`, still exported
cz-cli auth status # before: default_profile's live session
# after: {"logged_in": false, "active_profile": null}
cz-cli auth list # after: active_profile null, every session active:false
The user has a healthy default_profile and a valid token, and is told they are not logged in, with no mention of deleted_profile. An explicitly typed --profile bogus is fine — run-cli.ts:752-763 sets profileMustExist whenever the name was typed, so that exits PROFILE_NOT_FOUND. Only the inherited-env case lands here.
Smallest correct change: keep the name when it resolved but was not found, e.g. success({ logged_in: false, active_profile: activeProfile ?? null, profile_missing: activeProfile !== undefined }, ...). That matches the docstring and makes both the dangling-default_profile and stale-CZ_PROFILE cases self-diagnosing.
No test covers a CZ_PROFILE naming a profile absent from [profiles] — the new auth-profile-override.test.ts cases all point at profiles that exist, and envFor deletes CZ_PROFILE rather than exercising a stale one.
| requestInfo(url, route, argv, studio.tenantId, response.status, responseRequestId(text)), | ||
| ) | ||
| } | ||
| context.committedTenantId ??= studio.tenantId |
There was a problem hiding this comment.
HIGH (confidence: high) — committedTenantId is never updated after an identity swap, and combined with runBatchStatusChange's per-target catch that lets a batch finish under a tenant it did not start in — the exact outcome the guard above says it refuses.
context.committedTenantId ??= studio.tenantId??= means the value is pinned to the first successful response and never revised. That is fine as long as the tenant-mismatch throw at line 737 is terminal for the whole flow — but it is not, because refresh() has already replaced context.studio wholesale and set context.refreshed = true before the throw, and one caller catches and continues:
runBatchStatusChange (lines 1307-1323) shares one ResolvedContext across every target and records failures rather than aborting:
try {
await applyOne(target.id, ctx)
succeeded++
} catch (err) {
failed++
results.push({ id: target.id, name: target.name, result: "failed", error: message })
}Failure scenario — cz-cli analytics-agent simple-metric disable --all, profile with [agent] tenant_id = 55 and an OAuth session on tenant 10 (this PR's own fixture):
- the paginated list (line 1281) succeeds under tenant 55 →
committedTenantId = 55 - target feat: cz-cli distribution — a2a format, provider, profile gate #1's
simpleMetricDisable401s →refresh()swapscontext.studioto tenant 10 → mismatch → throws - the
catchrecords target feat: cz-cli distribution — a2a format, provider, profile gate #1 asfailedand continues - targets [Feature] Support mapping configuration injection for MULTI_DI sync tasks #2..#N are issued with
?tenantId=10and ids enumerated from tenant 55's catalog.refreshedis true so the guard never runs again, and??=leavescommittedTenantIdat 55, so nothing detects the split.
The command exits reporting succeeded for DISABLE writes against whatever objects hold those numeric ids in tenant 10.
The smaller correct change is to make the mismatch terminal for the context rather than for one request — on detecting it, clear context.refresh and set an abandoned flag that requestAnalytics checks before its first fetch, so every later call on that ctx fails fast regardless of what the caller's catch does. runBatchStatusChange is the caller that makes the current shape unsafe today, but any future per-item loop inherits the same hole.
| context.refreshPromise ??= context.refresh().finally(() => { | ||
| context.refreshPromise = undefined | ||
| }) | ||
| await context.refreshPromise |
There was a problem hiding this comment.
LOW (confidence: high) — refreshPromise is dead code: the coalescer can never observe a pending promise.
context.refreshPromise ??= context.refresh().finally(() => {
context.refreshPromise = undefined
})
await context.refreshPromiseThe enclosing guard is if (response.status === 401 && !context.refreshed && context.refresh), and context.refreshed = true is assigned synchronously on the line immediately before this block. Any second caller reaching the 401 check after that assignment fails !context.refreshed and never gets to the ??=; any caller that got there before it would have had to interleave between two adjacent synchronous statements, which cannot happen. So ??= always sees undefined, and the .finally that clears it is unreachable as a coalescing mechanism.
It is also unnecessary in practice: every flow that shares a ResolvedContext is sequential — the poll loop (do { await requestAnalytics(...) } while, line 998-1002), the pagination loop (line 1280), the batch apply loop (line 1307), and the upload handshake all await one request at a time. There is no Promise.all over a shared ctx.
If the intent was to make concurrent callers wait for one refresh rather than fail, refreshed is the wrong gate — it needs to be the promise itself (if (status === 401 && context.refresh && !context.refreshedOnce) → await (context.refreshPromise ??= ...), setting refreshedOnce after the await). If concurrency was never in scope, dropping refreshPromise from ResolvedContext and awaiting context.refresh() directly says the same thing in two fewer moving parts.
| signal: AbortSignal.timeout(300_000), | ||
| }) | ||
| const text = await response.text() | ||
| if (response.status === 401 && !context.refreshed && context.refresh) { |
There was a problem hiding this comment.
MEDIUM (confidence: low — please confirm the wire behaviour) — refresh keys on the HTTP status only, but this file documents that this backend does not use HTTP status for errors.
if (response.status === 401 && !context.refreshed && context.refresh) {Line 814-816 of this same file states:
Analytics Agent backend always returns HTTP 200, using
success: falseinside the envelope to signal business errors.
If an expired token is reported that way — HTTP 200 with a business code in the envelope, which extractBusinessError then turns into an AnalyticsBusinessError — none of this refresh machinery fires, and the user still sees the raw auth business error. All five new tests construct new Response(..., { status: 401 }) by hand, so they cannot distinguish "the backend really 401s on expiry" from "we assumed it does".
Two things worth stating in the PR:
- Was a real expired-credential response captured, and was it an HTTP 401? If it was a 200-with-envelope, the condition needs to also inspect
extractBusinessError(JSON.parse(text))for the auth code, and the fix as written does not address the reported symptom. - If the backend does both (401 from the gateway, 200-with-code from the app), the condition covers one of the two paths.
Not asserting this is a bug — the author may have the capture. But the file's own comment and the test fixtures point in opposite directions, and only one of them can be right.
| const response = await fetch(url, { | ||
| method: route.method, | ||
| headers, | ||
| ...(route.method === "GET" ? {} : { body: JSON.stringify(requestBody) }), | ||
| signal: AbortSignal.timeout(300_000), | ||
| }) |
There was a problem hiding this comment.
LOW (confidence: high) — moving fetch inside the retry loop doubles the effective per-call wall-clock budget, and session run's poll deadline cannot claw it back.
const response = await fetch(url, {
...
signal: AbortSignal.timeout(300_000),
})AbortSignal.timeout(300_000) is now constructed per attempt rather than once per call. A 401 followed by a retry gives a single requestAnalytics up to 300 s + refresh time + 300 s ≈ 10 minutes before it returns, where before the change 300 s was the ceiling.
In executeSessionRunCommand the poll loop checks its deadline only between requests:
payload = await requestAnalytics(argv, ROUTES.sessionResult, pollBody, {}, ctx)
if (isTerminalResponse(payload) || Date.now() >= deadline) breakso a --timeout-ms 360000 run can sit for ~10 minutes inside one poll before the deadline is even consulted. The user-visible symptom is --timeout-ms being silently exceeded by ~2.7x while the spinner keeps running.
Hoisting one AbortSignal.timeout(300_000) above the while (true) preserves the previous ceiling and makes the retry share the original budget rather than get a fresh one — which is also the more defensible semantic, since the point of the budget is "how long the caller waits", not "how long each attempt gets".
| endpoint: endpoint!, | ||
| studio: profileAgent ?? (await getStudioContext(argv, { allowMissingWorkspace: true })), | ||
| } | ||
| if (canRefreshCredential(config)) { |
There was a problem hiding this comment.
LOW (confidence: medium-high) — scope question, not a defect: the same expired-credential failure exists on the shared studio path, which this PR leaves unfixed.
if (canRefreshCredential(config)) {Every other Lakehouse command family — task, dqc, runs, attempts, datasource, integration, job, workspace-param — reaches the backend through studioRequest (clickzetta-sdk/src/studio/client.ts:10-55), which builds its ClientOptions from StudioConfig and never sets opts.config:
const opts: ClientOptions = {
baseUrl: config.baseUrl,
token: config.token,
customHeaders: { ... },
}client.ts:116-130 only force-refreshes when opts.config is present:
if (resp.status === AUTH_EXPIRED_STATUS && attempt < MAX_RETRIES) {
...
if (opts.config) { const fresh = await forceRefreshToken(opts.config); ... }
}so those commands retry a 401 with the same rejected token and then fail. Threading the ConnectionConfig into studioRequest's ClientOptions would fix all eight families in one place with machinery that already exists.
I don't think that makes this PR wrong — the Analytics Agent path genuinely cannot use studioRequest (different envelope, tenantId in the query string, openSessionAuth body merging), so a local retry here is defensible. But it's worth saying whether the shared path was considered and deferred, because the identity-swap and tenant-guard reasoning developed here is the interesting part and it will have to be re-derived if studioRequest is fixed later.
Also flagging a new dependency edge for confirmation: commands/analytics-agent.ts now imports forceRefreshToken and type ConnectionConfig from @clickzetta/sdk, plus connection/cookie-token.js and connection/config.js. @clickzetta/sdk and connection/* were already imported here (createTraceparent, mergeHeaders, readAgentEndpoint), so no package boundary is newly crossed — just confirming that's intended rather than incidental.
| return y | ||
| .option("profile", { hidden: true }) | ||
| .option("jdbc", { hidden: true }) |
There was a problem hiding this comment.
LOW (confidence: high) — un-hiding profile at the group level also un-hides it on auth logout, where it does nothing.
return y
.option("jdbc", { hidden: true })hideConnectionGlobals is applied to the group and to each subcommand builder, including logout (line 120). The new comment justifies visibility for list/status — "the agent prompt tells the model to pass --profile on exactly those two commands" — but logout operates on its <name> positional (the [oauth.<name>] id) and never reads argv.profile. So cz-cli auth logout --help now advertises a flag that is silently ignored, and the agent prompt change in agent-system-prompt.ts ("pass --profile <name> on every cz-cli command that should use it") makes it more likely a model tries auth logout --profile x and misreads the result.
.option("profile", { hidden: true }) in logout's own builder would keep list/status visible while leaving logout as it was. Style-adjacent, so entirely fine to leave — noting it because the group-level comment now covers three subcommands with one rationale that applies to two of them.
Review summaryA. Upstream invasiveness — no issues foundNo file under B. Clean fix vs. hole drilled around the problem — mostly clean, three itemsThe core design is the right shape: the 401 refresh lives at the one place that owns the Analytics Agent request ( Three things to look at:
C. Regression riskBehavioral changes I can see, with coverage:
Three carry real risk and have inline findings:
No tests were deleted, skipped, or loosened. The One item I could not verify and raised as a question: whether an expired credential actually arrives as HTTP 401, given this file's own comment that the backend "always returns HTTP 200, using |
| if (context.committedTenantId !== undefined && context.studio.tenantId !== context.committedTenantId) { | ||
| throw new AnalyticsHttpError( | ||
| `HTTP 401: the credential expired mid-request and the refreshed session belongs to tenant ${context.studio.tenantId}, not ${context.committedTenantId}. Re-run the command to start over under the new session.`, | ||
| requestInfo(url, route, argv, studio.tenantId, response.status, responseRequestId(text)), | ||
| ) | ||
| } |
There was a problem hiding this comment.
HIGH (confidence: medium-high) — the tenant guard is inert on the first request of a flow, which is exactly where a user-supplied resource id is at risk.
if (context.committedTenantId !== undefined && context.studio.tenantId !== context.committedTenantId) {committedTenantId is only assigned after a successful response (context.committedTenantId ??= studio.tenantId, line 758). So on the first request it is undefined and the condition short-circuits — the retry proceeds under whatever tenant the refresh landed on.
Failure scenario, using this PR's own test fixture ([profiles.test.agent] tenant_id = 55, OAuth session tenant 10):
cz-cli analytics-agent session run --session-id 7 --msg hello
--session-id 7 was minted earlier under tenant 55. The [agent] token is expired, so the very first sessionRun 401s, refresh() swaps identity to the OAuth login, and the retry re-issues the same request with ?tenantId=10 and session-id 7 — a session id that tenant 10 either does not own or, worse, owns as a different session. The guard the comment describes ("Refuse instead of splitting the flow in two") never runs, because nothing has committed yet.
The same applies to every numeric id taken from argv: --id, --domain-id, --dataset-id, and the enable/disable/update routes that write.
The root cause is that committedTenantId records "a tenant a request succeeded under" when what the guard actually needs is "the tenant this flow was resolved under". Seeding it at context construction — committedTenantId: context.studio.tenantId in resolveAnalyticsContext — makes the check fire on the first attempt too, and costs nothing on the pure-401-at-startup case where the tenant happens to be unchanged.
3a32a61 to
35a3534
Compare
| if (!response.ok) { | ||
| const detail = refreshError instanceof Error ? refreshError.message : refreshError ? String(refreshError) : undefined | ||
| throw new AnalyticsHttpError( | ||
| `HTTP ${response.status}: ${text.slice(0, 500)}${detail ? `\nToken refresh also failed: ${detail}` : ""}`, | ||
| requestInfo(url, route, argv, studio.tenantId, response.status, responseRequestId(text)), | ||
| ) |
There was a problem hiding this comment.
MEDIUM — the refresh error is dropped on the 200-envelope path, which is half the reason authRejected exists. Confidence: high.
if (!response.ok) {
const detail = refreshError instanceof Error ? refreshError.message : refreshError ? String(refreshError) : undefinedauthRejected deliberately matches two shapes: HTTP 401, and HTTP 200 carrying success: false + code: "401". But the combined message is only built under !response.ok, so on the 200 shape:
authRejected(response, text)→ true via the envelope branch.context.refresh()throws (revoked refresh token →SESSION_EXPIRED, "Runcz-cli auth login <name>"; or a network blip).refreshErroris set, so the tenant guard is skipped andif (refreshError === undefined) continueis false.if (!response.ok)is false — we fall straight through.context.committedTenantId ??= studio.tenantIdcommits a tenant from a response that was auth-rejected.- The 200 body is parsed and returned;
requestAnalyticsDataraises a bareAnalyticsBusinessError("401", "token expired").
So refreshError is silently discarded on exactly the path the comment at lines 761-764 says it is kept for ("the refresh error carries the recovery step (auth login), so report both"). The user gets 401: token expired with no indication that a refresh was attempted or why it failed.
Smallest fix: raise on authRejected + refreshError, not on !response.ok. Something like hoisting the throw into the refresh block:
if (refreshError !== undefined) {
const detail = refreshError instanceof Error ? refreshError.message : String(refreshError)
throw new AnalyticsHttpError(
`HTTP ${response.status}: ${text.slice(0, 500)}\nToken refresh also failed: ${detail}`,
requestInfo(url, route, argv, studio.tenantId, response.status, responseRequestId(text)),
)
}
continuewhich also stops step 5 from committing a tenant off a rejected response.
| // finds no candidate, skips the refresh path, and falls through to a login with | ||
| // empty credentials — the ~6 s dead end token.ts:80-83 warns about. Ask the store | ||
| // for an actual token (synchronous, file-local) rather than for its existence. | ||
| return Boolean(config.tokenStore?.load() || config.pat || (config.username && config.password)) |
There was a problem hiding this comment.
MEDIUM — this still admits the ~6 s dead end its own docstring is written to exclude. Confidence: high.
return Boolean(config.tokenStore?.load() || config.pat || (config.username && config.password))load() is truthy for a stored token that has no refresh_token. refresh_token is optional in parseOAuthEntry — packages/cz-cli/src/connection/profile-store.ts:590-595 only attaches refreshToken when the key is present, and parseOAuthEntry's hard requirements are access_token / expire_time_ms / obtained_at / instance_id / user_id. Legacy sections written before rotation existed are exactly this shape (the SDK calls them out: "legacy tokens without one always re-login", auth/token.ts:178-180).
For such a profile the flow is:
canRefreshCredential→trueforceRefreshToken→acquireToken(config, true)→candidate.refreshTokenisundefined→fetchToken(config)- no
pat, nousername/password→loginWithPassword(baseUrl, undefined, undefined, …)→ the "~6 s of login retries to arrive back at the same 401" this function's docstring names.
The comment two lines up says "Ask the store for an actual token … rather than for its existence", which closes the pruned-[oauth.<id>] hole but not this one. Asking for the thing that is actually re-exchangeable closes both:
return Boolean(config.tokenStore?.load()?.refreshToken || config.pat || (config.username && config.password))Not covered by a test — the new "a profile with no refreshable credential fails the 401 immediately" case has no [oauth.*] section at all, so tokenStore is never attached and this branch is not exercised.
| const rejectedTokenIsSdkOwned = profileAgent === undefined | ||
| context.refresh = async () => { | ||
| if (rejectedTokenIsSdkOwned) await forceRefreshToken(config) | ||
| context.studio = await getStudioContext(argv, { allowMissingWorkspace: true }) | ||
| } |
There was a problem hiding this comment.
Please confirm the intent: an expired [agent] token silently changes which tenant the command targets. Confidence: high that this is what the code does; no view on whether it is wanted.
const rejectedTokenIsSdkOwned = profileAgent === undefined
context.refresh = async () => {
if (rejectedTokenIsSdkOwned) await forceRefreshToken(config)
context.studio = await getStudioContext(argv, { allowMissingWorkspace: true })
}When the profile has a [profiles.<n>.agent] block, context.studio starts as getProfileAgentContext(...) — whose tenantId/userId come from the hand-pasted tenant_id/user_id in profiles.toml (studio-context.ts:131-151). The refresh replaces it with getStudioContext(...), whose tenantId is getCurrentUser(...).accountId — the OAuth login's tenant. Those are independent values, and this PR's own test asserts they differ (55 → 10 in "falls back from an expired legacy agent token to the profile OAuth session").
So a user who pinned tenant_id = 55 in their profile gets requests answered under tenant 10 with no message on either stream — success() reports the payload as if nothing happened. The [agent] block reads like a deliberate identity pin, and the alternative behaviour ("the [agent] token expired; refresh it or remove the block") would keep the pin honest.
Two things worth deciding explicitly:
- Is the silent identity swap the intended remedy, or should it at least warn (
aiMessage/ stderr) that the tenant moved? The model driving this CLI has no way to notice otherwise. - If it is intended, should it be restricted to reads? See my other comment on the
committedTenantIdguard — as written, the swap also applies todomain delete/metric delete/datasource delete, which is where a wrong tenant stops being recoverable.
| /** | ||
| * Whether a refresh has anything to re-exchange: an OAuth token store, a PAT, or | ||
| * username+password. Gating on "not a cookie" alone is not enough — a profile | ||
| * carrying only an [agent] block reaches `fetchToken` with empty credentials and | ||
| * spends ~6 s of login retries to arrive back at the same 401. Mirrors the SDK's | ||
| * own `hasLoginCredentials` guard, which is private to auth/token.ts. | ||
| */ | ||
| /** | ||
| * Whether the response says the credential was rejected. The gateway answers an | ||
| * expired token with HTTP 401, but this backend also reports failures as HTTP 200 | ||
| * with `success: false` and a code in the envelope (see extractBusinessError), so | ||
| * an envelope code of "401" counts too. Keyed on the structured code only — never | ||
| * on message prose, which is localized. | ||
| */ | ||
| function authRejected(response: Response, text: string): boolean { |
There was a problem hiding this comment.
LOW — orphaned docblock. Confidence: high.
Two /** … */ blocks stack up here, and the first one documents a different function:
/**
* Whether a refresh has anything to re-exchange: an OAuth token store, a PAT, or
* username+password. Gating on "not a cookie" alone is not enough — …
*/
/**
* Whether the response says the credential was rejected. …
*/
function authRejected(response: Response, text: string): boolean {The first block belongs to canRefreshCredential (line 648), which currently has no docblock — only the inline tokenStore note inside its body. As it stands authRejected carries a JSDoc describing PAT/username/password gating, which is nothing it does, and hover/IDE tooltips will show whichever one the tooling picks.
Move the first block down to line 648.
| /** CZ_PROFILE selects which profile `auth list`/`status` report on, so an ambient | ||
| * one in the runner's environment would steer these assertions. */ | ||
| function envFor(home: string, overrides: Record<string, string> = {}): Record<string, string> { | ||
| const env = { ...process.env, HOME: home, CLICKZETTA_TEST_HOME: home, NO_COLOR: "1" } as Record<string, string> | ||
| delete env.CZ_PROFILE | ||
| return { ...env, ...overrides } | ||
| } |
There was a problem hiding this comment.
LOW — the same hardening is missing at one other site that asserts the same value. Confidence: high.
function envFor(home: string, overrides: Record<string, string> = {}): Record<string, string> {
const env = { ...process.env, HOME: home, CLICKZETTA_TEST_HOME: home, NO_COLOR: "1" } as Record<string, string>
delete env.CZ_PROFILEThis guard is right, and it is needed precisely because selectedProfile now consults CZ_PROFILE. But packages/cz-cli/test/parameter-hardening.test.ts:665 asserts the identical thing —
expect(payload.data.active_profile).toBe("uat_0")— against a run() helper (that file, line 65-69) that spreads process.env without deleting CZ_PROFILE:
env: { ...process.env, HOME: home, CLICKZETTA_TEST_HOME: home, NO_COLOR: "1", ...env },Before this PR that assertion read default_profile straight off the file and was immune to the ambient environment; now it is not. Worth exporting envFor (or lifting it into test/support/cz-fixtures.ts) and using it there too, so the two active_profile assertions can't diverge on a developer machine that has CZ_PROFILE exported.
|
Review verdict — sections A, B, C below. All specific findings are inline comments on the lines they concern. A. Upstream invasiveness — no issues found All eight changed files are under New import edges are all intra-package ( B. Clean fix, or a hole drilled around the problem Mostly the clean fix — the 401 retry sits in the one shared
Separately, a question about intent on the silent tenant swap when a No dead code, no new env var or config key routing around a bug, no copy-pasted logic, no drive-by edits. The C. Regression risk Behavioral changes I can identify, with coverage:
No tests were deleted, skipped, or loosened. I could not run the suite, so nothing here is a claim that anything passes. 🤖 Generated with Claude Code |
| if ( | ||
| refreshError === undefined && | ||
| context.committedTenantId !== undefined && | ||
| context.studio.tenantId !== context.committedTenantId | ||
| ) { | ||
| context.abandoned = new AnalyticsHttpError( | ||
| `HTTP 401: the credential expired mid-flow and the refreshed session belongs to tenant ${context.studio.tenantId}, not ${context.committedTenantId} which this command started under. Re-run the command to continue under the new session.`, | ||
| requestInfo(url, route, argv, studio.tenantId, response.status, responseRequestId(text)), | ||
| ) | ||
| context.refresh = undefined | ||
| throw context.abandoned | ||
| } |
There was a problem hiding this comment.
HIGH — the cross-tenant guard never fires for a single-request command, so a delete/update can land in the wrong tenant. Confidence: high on the code path, medium on the backend consequence.
if (
refreshError === undefined &&
context.committedTenantId !== undefined &&
context.studio.tenantId !== context.committedTenantId
) {committedTenantId is only ever assigned on a successful response (line 794, context.committedTenantId ??= studio.tenantId). So the guard requires that some earlier request on this context already succeeded. When the very first request 401s, committedTenantId is undefined, the guard is skipped, and continue retries under whatever tenant the refresh landed on.
That is not a rare shape — it is the shape of most of this file. executeAnalyticsCommand (line 1468) calls requestAnalytics(argv, route, body, query) with no ctx, so every one of its ~55 call sites builds a fresh context and issues exactly one request. Among them:
ROUTES.domainDelete(line 2112),ROUTES.datasourceDelete(2010),ROUTES.simpleMetricDelete(2845),ROUTES.knowledgeEntryDelete(3155),ROUTES.knowledgeSpaceDelete(3211),ROUTES.knowledgeNodeDelete(3298, 3393),ROUTES.domainTableRemove(2226)ROUTES.domainUpdate(2091),ROUTES.simpleMetricUpdate(2712),ROUTES.datasourceUpdate(2002),ROUTES.knowledgeNodeMove(3330),ROUTES.knowledgeNodeRename(3314)
Concretely, with the [profiles.test.agent] fixture from this PR's own tests: cz-cli analytics-agent domain delete --domain-id 195 with an expired [agent] token → request 1 returns 401 under tenantId=55 → refresh swaps to the profile's OAuth identity, tenant 10 → continue → request 2 deletes domain 195 in tenant 10. The id came from a domain list the user or the model ran earlier under tenant 55.
The comment right above this block already states the hazard exactly — "a batch loop would apply writes to whatever holds those ids over there" — but keys the protection on "did a request already succeed" rather than on "which tenant was this command asked to target". Those coincide only for multi-request flows.
The smaller correct change is to seed the committed tenant at context construction rather than on first success, so the comparison is against the tenant the command was asked to use:
const context: ResolvedContext = {
endpoint: endpoint!,
studio: profileAgent ?? (await getStudioContext(argv, { allowMissingWorkspace: true })),
}
context.committedTenantId = context.studio.tenantId…and drop the ??= at line 794. Then both the batch case and the single-request delete refuse. If a cross-tenant fallback is genuinely wanted for reads (which is what the service enabled test at test #2 exercises), gate it on the route — e.g. an AnalyticsRoute flag — rather than on whether a prior request happened to succeed.
|
lgtm |
1 similar comment
|
lgtm |
2 participants
Honor explicit profiles and refresh expired Analytics Agent credentials.