feat: gateway quota, Windows spinner + install, sql job_id, per-profile instance id - #89
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| } | ||
| if (loaded.length === 0) throw errors[0] | ||
| const name = current && profiles[current] ? current : Object.keys(profiles)[0] |
There was a problem hiding this comment.
HIGH (confidence: high on the code path) — this PR introduces the "another tenant's balance" fallback; it did not pre-exist.
const name = current && profiles[current] ? current : Object.keys(profiles)[0]The PR description lists this as follow-up #1 and says "The same fallback existed before this change." It didn't. Both places the old code touched billing were gated on exact equality with current:
includeBilling: name === current,
...
...(loaded.find((item) => item.name === current)?.billing ?? {}),So when Profile.current() returned a name absent from profiles.toml (stale CZ_PROFILE, or a default_profile naming a deleted profile), includeBilling was false for every iterated name and the loaded.find(...) lookup missed — the result carried no cash/owe at all. Safe: no balance row painted.
After this change the same input resolves name to Object.keys(profiles)[0], and fetchProfileSnapshot reads that profile's account_id and returns its cashAmount as cash. The sidebar then paints a real ¥ figure belonging to an account the session is not running as. Profile.current()'s own docstring says the undefined case "callers should treat as 'no profile configured' rather than substituting one of their own" — and readProfileInfo at tui-quota-data.ts:200 implements exactly that, with the reasoning spelled out:
A current that names a profile absent from the file (stale CZ_PROFILE, deleted profile) must render nothing rather than silently swap in a different tenant's identity
The two functions now disagree, which is why the Profile section renders nothing while the Quota section shows a balance — the symptom already noted in the PR description.
Smallest correct change is to mirror line 200's policy verbatim:
| const name = current && profiles[current] ? current : Object.keys(profiles)[0] | |
| const name = current === undefined ? Object.keys(profiles)[0] : profiles[current] ? current : undefined |
(with if (!name) return {} below already handling the new undefined.)
No test covers this path. The deleted "surfaces the balance error instead of masking it with another profile" case was the closest guard, and its replacement ("reads only the current Profile, whatever LLM entry is selected") only exercises a current that does resolve.
| export function recordGatewayQuota(input: { baseURL: string; apiKey: string; quotas: ClickzettaQuota[] }) { | ||
| const file = storePath() | ||
| try { | ||
| const entries = readStore(file) | ||
| entries[gatewayQuotaCacheKey(input.baseURL, input.apiKey)] = { | ||
| updated_at: Date.now(), | ||
| quotas: input.quotas, | ||
| } | ||
| mkdirSync(dirname(file), { recursive: true }) | ||
| writeFileSync(file, JSON.stringify({ entries }, null, 2), "utf-8") | ||
| } catch { | ||
| // An unwritable home, a full disk, a racing writer: the indicator goes stale, | ||
| // which is strictly better than failing the user's request over a cache write. | ||
| } | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — three synchronous fs syscalls per completion, on the opencode server's event loop, with an unbounded file behind them.
recordGatewayQuota runs once per doGenerate and once per stream finish part, i.e. once per LLM request per session. Each call does readFileSync + JSON.parse + mkdirSync + JSON.stringify(…, null, 2) + writeFileSync. In the streaming path this executes inside the TransformStream.transform callback (index.ts:200), so it blocks the loop while the response is still being delivered.
Two separable problems:
mkdirSync(dirname(file), { recursive: true })on line 69 runs on every request to create a directory that exists after the first one. It only needs to run when the write fails withENOENT, or once per process. (PR description follow-up [Feature] Add pre-deployment validation (dry-run) for tasks #4.)- The store never prunes. Its key is
baseURL#sha256(apiKey)[:16], so it accumulates one entry per (endpoint, key) pair ever seen — and rotating a virtual key mints a new key forever. Read-modify-write means every request pays parse+serialize for the whole accumulated file, pretty-printed at 2-space indent. There is no upper bound on that cost.
Given the file is explicitly "a cache, never a source of truth", the cheap fix for (2) is to drop entries older than the consumer's own staleness window on write — readHeaderQuota already refuses anything past HEADER_QUOTA_MAX_AGE_MS (6h), so entries older than that are dead weight the reader will never return.
Neither is a correctness bug; the whole body is inside a try/catch that correctly degrades to "indicator goes stale". Flagging because the cost lands on every request in the server hot path rather than on a background cadence.
| @@ -152,6 +197,11 @@ function wrapModel(model: LanguageModelV3, modelId: string): LanguageModelV3 { | |||
| controller.enqueue({ ...chunk, error: mapThrown(chunk.error) }) | |||
| return | |||
| } | |||
| if (chunk?.type === "finish") { | |||
| const providerMetadata = withQuota(chunk.providerMetadata, headers, target) | |||
| controller.enqueue(providerMetadata === chunk.providerMetadata ? chunk : { ...chunk, providerMetadata }) | |||
| return | |||
| } | |||
There was a problem hiding this comment.
MEDIUM (confidence: medium-high) — on the streaming path the cache write is coupled to the finish part, so an aborted turn records nothing even though the headers were already in hand.
The comment on line 187-189 correctly explains why the metadata is published on finish: consumers read per-step metadata there. But withQuota does double duty — it also calls recordGatewayQuota (line 155) — and the cache has no such constraint. Its only consumer is readHeaderQuota in another process reading a file.
So when a stream never reaches finish, the x-czgw-ratelimit-* values that were sitting in result.response.headers at line 190 are discarded. Abort is not an edge case in the TUI — Esc during a long turn is the ordinary way to interrupt, and the SDK can also surface a late in-stream error part (the branch immediately above) instead of a finish. In those cases the sidebar keeps painting the previous request's numbers until the next completed turn, which is precisely the staleness the 6h window and the per-step-finish refresh were added to avoid.
Splitting the two responsibilities fixes it without touching the metadata contract — record at stream open, where headers is already read:
const headers = result.response?.headers
const quotaAtOpen = parseClickzettaQuota(headers)
if (quotaAtOpen && target) recordGatewayQuota({ ...target, quotas: quotaAtOpen })…and have the finish branch only publish metadata (no write). doGenerate is unaffected either way.
Worth confirming this is a deliberate trade rather than an oversight — the comment explains the finish placement for metadata but is silent on the write riding along with it.
| // rows rather than picking one to show. Shortest period first, because that is the | ||
| // one a user is most likely to hit inside this session. | ||
| const quotas = [...(snapshot.quotas ?? [])] | ||
| .filter((quota) => quota.used !== undefined && quota.limit !== undefined && quota.limit > 0) |
There was a problem hiding this comment.
MEDIUM (confidence: medium) — this filter and formatClickzettaQuota disagree about what counts as a displayable reading, so the CLI and the sidebar can report different things from the same cache entry.
.filter((quota) => quota.used !== undefined && quota.limit !== undefined && quota.limit > 0)quota.ts is explicit that any of the four fields may be absent — it types limit/used/remaining as optional and numeric() drops a non-numeric or empty value rather than coercing it. Its own test pins that case: a response with limit: "unlimited" and used: "" parses to { period: "daily", periodCode: "PDO", remaining: 12, scope: "api-key" }.
Feed that entry to the two formatters and they diverge:
formatClickzettaQuota(used byai-gateway quotaandagent llm test) has branches for limit-only and used-only, and appends(N left)wheneverremainingis defined — it prints a line.quotaRowsrequires bothusedandlimit, so it renders nothing and the Quota section shows only the balance row.
remaining is the number this whole feature is named for, and it is the one field the filter never consults. When limit and remaining are present but used is not, used is recoverable as limit - remaining, which would let the existing two-row rendering work unchanged.
Practical exposure is limited — every gateway measured so far sends all four headers together — so this is about the contract quota.ts documents rather than an observed blank sidebar. But the divergence means "the CLI says I have quota, the sidebar says nothing" is reachable without either surface being wrong on its own terms.
| snapshot={() => { | ||
| const quotas = headerQuota() | ||
| const balance = snapshot() | ||
| if (!quotas) return balance | ||
| return { ...balance, quotas } | ||
| }} | ||
| profileInfo={activeProfileInfo} |
There was a problem hiding this comment.
MEDIUM — please confirm the intent (confidence: medium that it is reachable; the design is clearly deliberate, the question is whether the guard is sufficient)
These two pieces interact in a way that can produce token rows with no balance row under a non-ClickZetta model:
snapshot={() => {
const quotas = headerQuota()
const balance = snapshot()
if (!quotas) return balance
return { ...balance, quotas }
}}headerQuota is deliberately sticky — refreshHeaderQuota only ever calls setHeaderQuota when the read returns something (line 133), so a readHeaderQuota miss preserves the last figure. That is right for the case the comment names (a gateway that sends no headers must not blank a live number).
But when balance is undefined, { ...undefined, quotas } is { quotas } — truthy — so quotaRows renders. And fetchQuotaSnapshot returns undefined for exactly one reason: classifyClickzettaEntry(...).kind === "foreign". That is the case the existing test calls out as the one that should hide everything ("a ¥ figure next to a Claude model would name money that model is not spending"), and the inverse — token rows next to a Claude model — misattributes usage the same way.
The only thing standing between those two states is:
activeModel.onChange(({ sessionID }) => {
if (sessionID !== currentSessionID(api)) return
setHeaderQuota(undefined)so the clear depends on onChange firing and its sessionID matching currentSessionID(api) at that instant. If a provider switch is ever observed for a session id that doesn't match (or isn't observed at all), the early return skips the clear and the stale list survives into a foreign-provider render, since the busy→idle load() path can only ever set headerQuota, never clear it.
Two ways to close it without giving up the stickiness:
- make the combiner refuse to synthesise a snapshot the balance half rejected:
if (!balance) return undefinedbefore merging, or - move the "is this still a ClickZetta entry" check into
refreshHeaderQuota, clearing whenclassifyClickzettaEntry(...).kind === "foreign"rather than relying solely on theonChangeedge.
Is the onChange guard known to cover every provider-switch path, or is a defensive clear in the combiner worth it?
| error("MISSING_API_KEY", `Agent LLM '${entry.name}' has no api_key. Set one with \`cz-cli agent llm add ${entry.name} --api-key <API_KEY>\`.`, { format, exitCode: 2 }) | ||
| return | ||
| } | ||
| const probeModel = argv.model ?? (await firstClickzettaModel(entry.base_url, entry.api_key)) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — a model-not-served 404 is reported as GATEWAY_QUOTA_UNAVAILABLE, which misdiagnoses the failure.
const probeModel = argv.model ?? (await firstClickzettaModel(entry.base_url, entry.api_key))firstClickzettaModel is best-effort by design and returns undefined on any failure (its docstring: "any failure returns undefined and the caller keeps the old default"). buildLlmProbeRequest then falls back to DEFAULT_PROBE_MODELS.clickzetta = "deepseek/deepseek-v4-pro". On a tenant that does not serve that id, the gateway answers 404, and the !response.ok branch reports:
Could not read the quota for '': the gateway answered HTTP 404.
The key is fine, the quota is fine, and the endpoint is fine — only the guessed model is wrong. The user is told their quota is unreadable.
This is the same defect probe.ts:14-30 documents for agent llm test, where it produced "two surfaces, two verdicts, from the same credential." The !response.ok branch here reintroduces it for a new surface, because it collapses every non-2xx into one quota-specific error code.
rewriteClickzettaGatewayError is already being called on the body — it returns undefined for a non-billing error like a 404. That gives a clean discriminator: when there is no gateway billing/quota code and argv.model was not supplied, the honest message is that the probe model could not be resolved, naming probeModel and suggesting --model or cz-cli agent llm models <entry>. Worth at least including probeModel in the error extra so the cause is recoverable from the output.
Separately (LOW, same block): resolveClickzettaEntry at line 81 is a third set of "which key do I mean" rules, and it shares its name with the exported resolveClickzettaEntry in opencode-plugin/tui-quota-data.ts:333 while answering differently — the sidebar's version routes through classifyClickzettaEntry, which treats an explicit non-ClickZetta providerID as foreign and refuses to guess between multiple entries, whereas this one falls through to "first ClickZetta entry with an api_key". So cz-cli ai-gateway quota and the sidebar can report on different keys with no indication to the user. Noted as follow-up #2 in the description; flagging that the name collision makes the divergence easy to miss at the call site.
| }) | ||
| }) | ||
| // All four headers present but empty-valued yields nothing worth reporting. | ||
| const reported = quotas.filter((q) => q.periodCode !== "" || q.limit !== undefined || q.remaining !== undefined) |
There was a problem hiding this comment.
LOW (confidence: high) — the "is this a reading" filter omits used, so a used-only report is thrown away.
const reported = quotas.filter((q) => q.periodCode !== "" || q.limit !== undefined || q.remaining !== undefined)formatClickzettaQuota twelve lines below has a dedicated used-only branch (`${quota.used.toLocaleString("en-US")} tokens used`), so the two halves of this file disagree about whether a used value alone is worth reporting: the formatter says yes, the filter drops the entry before it can get there. With a single quota in play the whole parseClickzettaQuota call then returns undefined — "the gateway said nothing" — rather than "the gateway reported usage but no ceiling".
| const reported = quotas.filter((q) => q.periodCode !== "" || q.limit !== undefined || q.remaining !== undefined) | |
| const reported = quotas.filter( | |
| (q) => q.periodCode !== "" || q.limit !== undefined || q.used !== undefined || q.remaining !== undefined, | |
| ) |
Reachability is low: periodCode alone satisfies the filter, and every observed response carries the period header. Already listed as follow-up #5 in the description — repeating it here so it is anchored to the line.
| // cz-cli's single seam onto @clickzetta/ai-gateway: nothing else in this package | ||
| // imports it directly, so anything the gateway module owns is re-exported here. | ||
| // Named for the error classifier it started as; it now also carries the response- | ||
| // header quota reader, which shares that module's job of interpreting AIGW's wire | ||
| // format. |
There was a problem hiding this comment.
LOW (confidence: high) — this comment is contradicted by the same diff that adds it.
// cz-cli's single seam onto @clickzetta/ai-gateway: nothing else in this package
// imports it directly, so anything the gateway module owns is re-exported here.packages/cz-cli/src/llm/clickzetta-provider.ts:7 also imports from @clickzetta/ai-gateway, and this PR uses that second seam — tui-quota-data.ts:35 pulls normalizeClickzettaGatewayUrl from ../llm/clickzetta-provider.js while pulling readGatewayQuota from this file, so the same module now reaches the gateway package through both routes for one feature.
A comment stating an invariant that does not hold is worse than no comment: the next person adding a gateway export will trust it and re-export here, and the normalizeClickzettaGatewayUrl used by readHeaderQuota will keep coming from elsewhere. Either state the actual arrangement (two seams: this one for wire-format interpretation, clickzetta-provider.ts for provider construction and URL normalization) or make it true by routing the normalizer through here too — the latter would also remove the odd asymmetry in tui-quota-data.ts's import block.
Already listed as follow-up #6 in the description; anchoring it to the line since the comment is new in this PR.
| // good value", which for a profile that can never answer would pin a stale | ||
| // balance from a different account forever. | ||
| if (accountId === undefined) return {} | ||
| const credential = await profileTokenSource(config).get() |
There was a problem hiding this comment.
LOW (confidence: high) — dropping the readCurrentUserName call here leaves its docstring stale.
This rewrite removes fetchProfileSnapshot's use of readCurrentUserName (it existed only to supply userName to the retired listApiKeys read). But that helper's docstring at line 240 still says:
The one call site both
fetchProfileUserNameandfetchProfileSnapshotshare, so the envelope check (four-part: record / isPortalOk / record data / string name) is written once rather than kept in sync by hand in two places.
fetchProfileUserName (line 299) is now the only caller, so the stated justification no longer holds and a reader will go looking for a second call site that isn't there. Worth deleting the "both … share" clause — the helper itself is still worth keeping for the named envelope check.
CURRENT_USER_PATH (line 39) is likewise now reachable from just that one function; still live, just narrower than the comment above it implies.
| // One step-finish part per LLM request, which is exactly when the gateway has | ||
| // reported a new quota — and the only response-adjacent signal a TUI plugin can | ||
| // observe (the plugin API exposes no response hook, and opencode's processor | ||
| // drops the finish metadata). Reading the cache here is local and synchronous, | ||
| // so it costs nothing to do per request instead of per turn. | ||
| api.event.on("message.part.updated", (event) => { | ||
| if (event.properties.part?.type !== "step-finish") return | ||
| refreshHeaderQuota() | ||
| }), |
There was a problem hiding this comment.
LOW–MEDIUM (confidence: high on the gap, medium on whether it warrants a ledger edit) — this adds a new upstream-hook dependency that UPSTREAM-PATCHES.md does not record.
I verified the hook itself is real and the guard is right: message.part.updated is dispatched with event.properties.part, and "step-finish" is a live part type (opencode/src/session/processor.ts:693, sdk/js/src/v2/gen/types.gen.ts:559). No correctness concern here.
The issue is the ledger. HOOK entry 5 ("Quota/Profile sidebar sections") exists specifically so a re-baseline can re-verify the hooks this feature rides on — its own preamble says hook customizations "are listed so a re-baseline can confirm the hooks they depend on still exist in the new upstream." Its "Upstream hooks to re-verify" list covers sidebar_content, its append-vs-single_winner behavior, the 100/200/300/400/500 order values, and the parentID early return in sidebarVisible. This PR makes the token half of that section depend on two more things it doesn't mention:
- the
message.part.updatedevent carryingproperties.part, and that part'stypestill being the string"step-finish"— a property shape, not just an event name, so it can break without the event disappearing; - a new on-disk path,
~/.clickzetta/gateway-quota.json, which is the sole channel between the provider in the server process and this reader.
Entry 5's description also still says the Quota section shows "balance + token usage" sourced from the portal, which is no longer how the token half works.
Caveat on severity: the existing api.event.on("session.status", …) two lines up is already an unrecorded event-bus dependency, so this is a pre-existing omission being widened rather than a new class of gap. Still worth a line in entry 5 while the surrounding text is being brought up to date, since the "step-finish" literal is the linchpin of the new refresh cadence and would fail silently — the sidebar would just stop updating between turns, with no error.
| probe: "chat.completions", | ||
| sample_response: completion, | ||
| source: target.source, | ||
| ...(quotas ? { quotas } : {}), |
There was a problem hiding this comment.
LOW (confidence: high) — output-shape change on an existing command; flagging for the record, not as a defect.
agent llm test gains a quotas array in its structured payload (line 712) and up to N extra lines in its TTY output (line 698). Both are additive and conditional on the gateway actually reporting the headers, so a consumer reading sample_response/probe/source is unaffected, and a non-ClickZetta provider is byte-identical to before. --field extraction on existing keys still resolves.
Two small notes:
- The alignment works out:
"quota: "is 9 chars plus the separating space, matchingprovider:/url:/response:at 10. Continuation rows use 9 spaces plus the separator, so multi-period output lines up under the first. Verified by reading, not by running. - No test covers the new rows.
test:e2e:helponly exercises--helpsurfaces, and there's no fixture assertingllm testoutput with quota headers present.agent-llm-test-gateway-error.test.tsexists for the error path — a sibling case withx-czgw-ratelimit-*on a 200 would pin both the JSON key and the TTY alignment, which is the part most likely to drift.
Worth confirming the added quotas key is intended to be part of the command's stable contract, since agent llm test is the kind of thing CI scripts parse.
Review summaryTen inline findings. One is a genuine regression the PR description mis-classifies as pre-existing; the rest are LOW–MEDIUM. A. Upstream invasiveness — no issues foundNo file under More than that, the invariant was actively honoured where it would have been easy to break: the natural place to carry a response header from the server process to the TUI process is One ledger gap, flagged inline on the new event subscription: HOOK entry 5 gains an unrecorded dependency on B. Clean fix, or a hole drilled around the problem?Mostly the clean fix, and in one place a notably good one: retiring the Three places treat a symptom:
Plus a formatter divergence ( C. Regression riskOne real regression, inline at Everything else I could enumerate, with test coverage:
Two behavior changes I could not resolve from the code and raised as questions rather than asserting bugs: whether the sticky Verification notes: I read the full files and followed the calls, but could not run anything — |
c335903 to
6a9bfa8
Compare
suibianwanwank
changed the title
| import * as Profile from "../connection/profile-context.js" | ||
| import { deriveAuthType, explicitAuthType, loadProfiles } from "../connection/profile-store.js" | ||
| import { readLlmEntries } from "../llm/native-config.js" | ||
| import { readGatewayQuota, type ClickzettaQuota, type ClickzettaQuotaPeriod } from "../llm/gateway-error.js" |
There was a problem hiding this comment.
MEDIUM — confidence: medium-high. This import pulls the whole AI SDK provider into the pre-bundled TUI asset.
import { readGatewayQuota, type ClickzettaQuota, type ClickzettaQuotaPeriod } from "../llm/gateway-error.js"gateway-error.ts re-exports the quota helpers from the root @clickzetta/ai-gateway barrel, and that barrel's first line is:
import { createOpenAICompatible } from "@ai-sdk/openai-compatible"script/build.ts:320 pre-bundles tui-quota-runtime.ts through Bun.build({ target: "bun", format: "esm", minify: true }) with no external, so everything reachable from the entry is inlined. readGatewayQuota itself only needs node:fs + node:crypto; via the root barrel it drags in the openai-compatible provider, its wrapModel machinery and their transitive deps. Nothing is imported from them at runtime by this file, but a side-effectful module graph is not reliably shaken out of a minified bundle — that is exactly the failure mode INTRUSIVE #12 in UPSTREAM-PATCHES.md documents for opentui-spinner.
This PR already adds the narrow subpath export that avoids it ("./quota-store" in packages/clickzetta-ai-gateway/package.json), so:
import { readGatewayQuota } from "@clickzetta/ai-gateway/quota-store"
import type { ClickzettaQuota, ClickzettaQuotaPeriod } from "@clickzetta/ai-gateway/quota"Worth checking the built asset size before/after to confirm how much this actually costs — I could not run the build here.
| error("MISSING_BASE_URL", `Agent LLM '${entry.name}' needs a base_url before its quota can be read.`, { format, exitCode: 2 }) | ||
| return | ||
| } | ||
| if (_debug) process.stderr.write(`[debug] → ${probe.method} ${probe.url}\n`) |
There was a problem hiding this comment.
MEDIUM — confidence: high. This line is unreachable: the quota handler never arms _debug.
if (_debug) process.stderr.write(`[debug] → ${probe.method} ${probe.url}\n`)Every other handler in this file calls setGatewayDebug(!!argv.debug) as its first statement — lines 422, 510, 599, 650, 695, 741, 788, 845. The new quota handler (starting at line 322) omits it, so _debug keeps whatever the previous command in the process left it at, which for a one-shot CLI invocation is its initial false. Net effect: cz-cli ai-gateway quota --debug accepts the flag and prints nothing, and this line plus any _debug guard inside firstClickzettaModel/buildLlmProbeRequest reached from here is dead.
Fix is one line at the top of the handler body, next to const format = argv.format:
setGatewayDebug(!!argv.debug)
| // the cache is written now, while the reading cannot be lost to an aborted turn. | ||
| // Only the in-band publication waits for the "finish" part, because that is where | ||
| // consumers read per-step metadata. | ||
| const quota = recordQuota(result.response?.headers, target) |
There was a problem hiding this comment.
LOW — confidence: high. recordQuota sits outside the try/catch here, unlike in doGenerate.
const quota = recordQuota(result.response?.headers, target)recordQuota does header parsing plus a synchronous read-modify-write of ~/.clickzetta/gateway-quota.json. recordGatewayQuota swallows write failures, but the surrounding work is not fully failure-proof — readStore on a truncated/corrupt JSON file, a homedir() that resolves to an unwritable path, JSON.parse on a partially-written file from a concurrent process. Anything that escapes propagates straight out of doStream uncaught, failing an LLM request whose HTTP call already succeeded. Telemetry should never be able to do that.
The asymmetry is the part worth deciding on deliberately: in doGenerate the same call is inside the try, so a throw there gets rewritten by mapThrown into a ClickZetta gateway error — the request still fails, but now with a message blaming the gateway for a local cache problem. Neither placement is right; the fix is a dedicated guard so a cache failure costs the reading and nothing else:
let quota: ClickzettaQuota[] | undefined
try { quota = recordQuota(result.response?.headers, target) } catch { /* quota is best-effort */ }Low severity because every individual failure mode is unlikely, but the blast radius when it does happen is "the request fails" rather than "the sidebar shows a stale number".
| * quota resets inside this window's own scale, so a reading from a previous day | ||
| * would be actively misleading. | ||
| */ | ||
| const HEADER_QUOTA_MAX_AGE_MS = 6 * 60 * 60 * 1000 |
There was a problem hiding this comment.
LOW — confidence: medium. A 6-hour window straddles a daily reset, so this does not deliver the guarantee stated just above it.
const HEADER_QUOTA_MAX_AGE_MS = 6 * 60 * 60 * 1000The rationale is that yesterday's daily number must never paint as today's. But a reading cached at 23:00 is still inside the 6h window at 04:00, after the daily counter has reset — the sidebar then shows yesterday's near-exhausted used against today's fresh limit, which is the exact failure this constant is meant to prevent, and it is the worst direction to be wrong in (it reports "out of quota" to a user who has a full day's allowance).
The freshness bound that actually matches the invariant is the reset boundary, not a fixed duration: for a quota whose period is daily, drop a reading whose timestamp falls on an earlier local date (and correspondingly for weekly/monthly). total has no reset and can keep a duration-based bound.
Cheap and self-correcting in practice — the next LLM request overwrites the cache — so this only shows up in the first paint of a session started after a reset. Flagging it because the comment claims an invariant the code does not hold.
| // Both numbers, not just a percentage: seeing the ceiling is what tells you | ||
| // whether the percentage is worth acting on. | ||
| return [ | ||
| { text: `${abbreviate(quota.used!)} / ${abbreviate(quota.limit!)} tokens${periodSuffix(quota.period)}`, tone }, |
There was a problem hiding this comment.
LOW — confidence: high. abbreviate has no billions tier, so a lifetime (PTO) ceiling renders as a four-digit M.
{ text: `${abbreviate(quota.used!)} / ${abbreviate(quota.limit!)} tokens${periodSuffix(quota.period)}`, tone },abbreviate (line 18) tops out at M, which was fine when the only figure was a 10M complimentary grant. Header-reported PTO limits are total-lifetime caps and run larger: this PR's own test asserts "21.3M / 1000.0M tokens" for a 1,000,000,000 limit. 1000.0M is six characters of noise in a 42-column sidebar and reads worse than 1.0B.
One line:
if (abs >= 1_000_000_000) return `${(value / 1_000_000_000).toFixed(1)}B`Caveat worth weighing before taking it: the doc comment says this deliberately mirrors packages/tui/src/util/locale.ts number. If that helper also stops at M, adding B diverges from upstream's formatting — the divergence is probably still worth it here, since upstream's counter is per-session tokens where billions cannot occur, but that is your call.
| // no ratio to draw, and a lone figure in this column would read as one. | ||
| const quotas = [...(snapshot.quotas ?? [])] | ||
| .filter((quota) => quota.used !== undefined && quota.limit !== undefined && quota.limit > 0) | ||
| .sort((a, b) => PERIOD_ORDER.indexOf(a.period) - PERIOD_ORDER.indexOf(b.period)) |
There was a problem hiding this comment.
LOW — confidence: medium. Sorting on period alone means two quotas from different scopes collapse into indistinguishable rows.
.sort((a, b) => PERIOD_ORDER.indexOf(a.period) - PERIOD_ORDER.indexOf(b.period))parseClickzettaQuota deliberately keys on ^x-czgw-ratelimit-(.+)-token-…, i.e. it captures an arbitrary scope, not just api-key — the regex is written that way precisely so a tenant-scoped header set is carried through. When the gateway reports both scopes for the same period, quotaRows emits two 12.0M / 50.0M tokens today / 76% left today pairs with nothing distinguishing them, in an order determined by header arrival. The user sees a duplicated figure and no way to tell which ceiling is the binding one.
The CLI side does not have this problem: formatClickzettaQuota has the scope available and can name it. The sidebar drops it.
Options, cheapest first: include the scope in periodSuffix's row text when more than one distinct scope is present; or add scope as a secondary sort key so at least the grouping is stable; or filter to api-key here and treat other scopes as CLI-only. The first is the one that actually answers the user's question.
Only reachable if a deployment starts sending tenant-scoped headers, hence LOW — but it is silent when it happens rather than visibly broken.
| if (r.status === JobStatus.FAILED) { | ||
| const hint = await fetchSchemaHint(ctx, sql, r.errorMessage ?? "") | ||
| logOperation("sql", { sql, ok: false, errorCode: r.errorCode, timeMs: Date.now() - t0 }) | ||
| error(r.errorCode ?? "SQL_ERROR", await formatQueryError(r, ctx, argv.profile), { format, extra: hint ? { schema: hint } : undefined }) | ||
| await handleFailure(r, sql, ctx, format, t0, argv.profile) | ||
| return | ||
| } |
There was a problem hiding this comment.
LOW / please confirm intent — confidence: high on the behavior change, no opinion on whether it is wanted.
if (r.status === JobStatus.FAILED) {
await handleFailure(r, sql, ctx, format, t0, argv.profile)
return
}Folding this branch into handleFailure is the right consolidation — it is what makes job_id reach every failure path instead of being copy-pasted, and it fixes the real bug. But it also changes two things about this path beyond adding job_id, and the PR description only claims the latter:
ai_messageis now emitted.handleFailuresetsaiMessagewheneverfetchSchemaHintreturns a hint (line 502-504); the inlined code this replaced passedextrabut noaiMessage. For--format text/csv/table/jsonlthat means a new line on stderr (writeAiMessageToStderr,output/index.ts:403), and for every format a new top-levelai_messagekey in the JSON error envelope. Additive, so a script readingerror.codeis unaffected, but a script diffing stderr or asserting exact error-envelope keys would notice.logOperationnow fires for this path (line 501). Previously this branch reported the failure without recording the telemetry event. That is almost certainly the fix rather than a regression — the counts were undercounting single-statement SQL failures — but it does change what the telemetry stream contains, so downstream dashboards will show a step change insqlfailure volume that is an artifact of this PR, not of user behavior.
Both look like they were intended, in which case a line in the PR description is enough. Flagging rather than asserting, per "when a behavior change looks plausible but may be unintentional".
Test coverage: packages/cz-cli/test/sql-error-job-id.test.ts covers job_id on the failure envelope. I did not find a test asserting the ai_message/stderr shape for this specific path, so item 1 is uncovered.
| args: ["ai-gateway", "quota", "--help"], | ||
| expectHeader: "cz-cli ai-gateway quota", | ||
| expectOptions: ["--model"], | ||
| expectCommands: ["x-czgw-ratelimit-api-key-token-*"], |
There was a problem hiding this comment.
LOW — confidence: high. expectCommands is being used to assert an epilogue string, not a subcommand.
expectCommands: ["x-czgw-ratelimit-api-key-token-*"],x-czgw-ratelimit-api-key-token-* is text from the .epilogue(...) block at src/commands/ai-gateway.ts:315-321. It is not a subcommand of ai-gateway quota, which has none.
The assertion passes — e2e-help-runner.ts:48-52 does a plain combined.includes(cmd) over the whole help output, with no parsing of the Commands section — so this is not a broken test. It is a misfiled one: the failure message it would produce is missing subcommand "x-czgw-ratelimit-api-key-token-*" in help output, which would send whoever hits it looking for a subcommand that was never meant to exist. It also weakens the signal that every other expectCommands in this file carries (compare line 88 and line 99, which are genuine subcommand lists).
Two ways out: add an expectText?: string[] field to HelpCase for prose assertions and move this there, or drop the assertion — expectHeader plus expectOptions: ["--model"] already establishes the command is wired up, and the epilogue wording is the kind of copy that will be reworded without anyone intending to break a test.
Review summaryReviewed against the conventions in A — Upstream invasiveness: no issues found. The five
One thing I could not check: B — Clean fix vs. hole drilled around the problem: two findings, both minor; the core changes are the right shape. The three main changes are the correct fix rather than a workaround, and that is worth saying explicitly:
Inline in this category: the dead C — Regression risk: one behavior change worth confirming, otherwise covered.
I could not run the test suite or the build, so nothing here is a claim that anything passes. |
6a9bfa8 to
696a8b5
Compare
|
All seven inline findings acted on, one premise corrected and one factual correction. Fixes are folded into the commits they belong to rather than appended, so the branch is still three commits.
I built the asset both ways. Changing it anyway, because "the bundler shook it out this time" is a weaker guarantee than "it was never reachable" — which is the same reasoning as INTRUSIVE #12. Done at the seam rather than around it:
I should be straight about this one: the previous revision's description listed it as fixed. It wasn't — the comment claiming the invariant was still there and the constant was untouched. That was my miss, not a disagreement.
On the one thing you could not check: |
| import { registerSpinner } from "opentui-spinner/solid" | ||
|
|
||
| export function registerOpencodeSpinner() { | ||
| if (!getComponentCatalogue().spinner) registerSpinner() |
There was a problem hiding this comment.
LOW (confidence: low — I could not reach anomalyco/opencode#35292 from this environment)
INTRUSIVE #12's entire justification for shipping without a cz-cli change banner is that the edit is byte-identical to upstream's, so a banner would turn a clean fast-forward into a conflict. That makes byte-identity the load-bearing claim for the one patch in the ledger that rg -n "cz-cli change" cannot see.
The guard here is what makes me want it confirmed:
if (!getComponentCatalogue().spinner) registerSpinner()A defensive catalogue check reads like something added locally rather than something a "preserve spinner registration" upstream fix would need. If any of this file, the five call sites, or the ./component/register-spinner export-map entry differs from #35292 even slightly, the fast-forward premise is gone and there is no banner to find the divergence — which is exactly the failure mode entries 4, 7, 8 and 9 were written up to prevent.
Worth pasting the upstream diff into the ledger entry (or linking the commit 7a8e7c88f4 file list) so a future re-baseline can check identity rather than take it on trust.
For what it's worth, the substitute greps in the ledger do hold on this branch: rg -n 'registerOpencodeSpinner' packages/tui packages/opencode → 11 hits (1 definition + import/call at each of the 5 sites), rg -n 'import "opentui-spinner/solid"' → none.
| // reader alone would carry a ClickZetta reading into a session running a foreign | ||
| // provider, where the balance is undefined and the merge below would still paint the | ||
| // stale token rows. | ||
| const syncHeaderQuota = () => setHeaderQuota(readHeaderQuota(activeModel.providerID())) |
There was a problem hiding this comment.
LOW (confidence: high on the fact, low on the impact) — the documented split between the two readers is not the split the code has.
// Per CONTEXT (mount, session switch, provider switch — all of which route through
// onContext -> controller.refresh -> load) it means the figure on screen belongs to a
// credential that is no longer the active one, so it must go.
const syncHeaderQuota = () => setHeaderQuota(readHeaderQuota(activeModel.providerID()))syncHeaderQuota also runs on every busy→idle edge, i.e. once per agent turn, which the comment does not list. tui-quota-controller.ts:62-72 observeStatus calls refresh() on the busy→idle transition, refresh() calls load(), and load() is the closure at line 207 that invokes syncHeaderQuota().
So the clearing reader runs at the end of every turn, not only on the three context events, and the "sticky" property refreshHeaderQuota exists to provide is discarded one event later. In practice I don't think this misdisplays anything — recordQuota only ever writes a reading, never erases one, so a gateway that stops sending headers still finds the previous entry in the file and readHeaderQuota returns it either way. The two readers only actually diverge once the cached entry ages past HEADER_QUOTA_MAX_AGE_MS or crosses a daily reset, and blanking is the wanted behaviour there.
Which is the point: as written, the stickiness this comment justifies is not observable, so a future reader will reason from a rule the code does not follow. Either add busy→idle to the list, or say plainly that the two readers differ only in the aging/reset case.
| // good value", which for a profile that can never answer would pin a stale | ||
| // balance from a different account forever. | ||
| if (accountId === undefined) return {} | ||
| const credential = await profileTokenSource(config).get() |
There was a problem hiding this comment.
LOW (confidence: high) — dropping the key-listing read from fetchProfileSnapshot leaves readCurrentUserName's docstring describing a world with two callers.
That function's header (line ~240) now says:
The one call site both
fetchProfileUserNameandfetchProfileSnapshotshare, so the envelope check (four-part: record / isPortalOk / record data / string name) is written once rather than kept in sync by hand in two places.
After this change fetchProfileSnapshot reads billing only; fetchProfileUserName is the sole caller. The stated reason for extracting the helper no longer applies (keeping it extracted is still fine — it just isn't shared any more).
Same file, same shape as the neighbouring comment fixes this PR already made for the retired listApiKeys route — this one was missed because it sits outside the edited hunks.
| const file = storePath() | ||
| const now = Date.now() | ||
| try { | ||
| const entries = readStore(file) |
There was a problem hiding this comment.
LOW (confidence: high on the mechanism, cache-only on impact) — a concurrent read of a half-written file discards every entry, not "one".
The function's own docstring says:
Read-modify-write, so two servers finishing a request at the same instant can lose one entry.
But writeFileSync is not atomic, and readStore maps any parse failure to {} (correctly — line 118-122). So a writer that reads while another writer is mid-writeFileSync gets {}, then persists a file containing only its own entry. Every other (endpoint, key) pair on the machine is gone, not just the racing one.
const entries = readStore(file) // {} if the file is currently half-written
...
entries[gatewayQuotaCacheKey(...)] = { updated_at: now, quotas: input.quotas }
writeFileSync(file, body, "utf-8") // now the only entry in the fileConsequences are bounded — it is a cache, and each dropped key re-fills on its next request — so I would not block on it. Two options if you want the comment and the code to agree: either write to ${file}.tmp and renameSync (atomic on both POSIX and Windows-same-volume, removes the torn-read window entirely), or just widen the docstring to say a torn read costs the whole file rather than one row.
ab7a768 to
465ccb2
Compare
|
Heads-up: The cause is the fix for the inline on The narrower rule, pushing shortly: the definitive "this account has no instance by that name" is a hard error (your finding, unchanged); a lookup that was attempted and failed warns and proceeds on the credential's id; a profile with no Verifying the full suite before the next push rather than alongside it. |
465ccb2 to
4c4dc9f
Compare
| } else if (notListed) { | ||
| // A definitive answer, and fatal regardless of what the credential carries. If the | ||
| // account does not own an instance by this name, submitting under some OTHER instance | ||
| // — which is what falling back to the credential's id would do — is never what was | ||
| // asked for. Deciding this on whether a credential happens to carry an id is what let | ||
| // the two branches disagree about how serious the same answer was. | ||
| throw new Error( | ||
| `Instance '${config.instance}' is not listed for this account on ${config.service}. ` + | ||
| `Check the name (\`cz-cli profile list\`), or set instance_id in the profile.`, | ||
| ) | ||
| } else if (token.instanceId) { | ||
| // Reached only when the lookup could not answer — it failed, or there was no | ||
| // account_id to ask with. Never on a definitive "no such instance": that is the throw | ||
| // above. Only a PAT/password credential carries an id (from its own login response); | ||
| // an OAuth one does not, so it goes to the legacy read or the throw below. | ||
| config.instanceId = token.instanceId |
There was a problem hiding this comment.
HIGH — regression risk (section C). Confidence: medium-high.
The notListed throw is ordered before the token.instanceId fallback, so a definitive, credential-supplied id gets discarded:
} else if (notListed) {
throw new Error(
`Instance '${config.instance}' is not listed for this account on ${config.service}. ` + …
)
} else if (token.instanceId) {
config.instanceId = token.instanceIdThe comment argues notListed is "a definitive answer, and fatal regardless of what the credential carries". That holds when the credential's id is incidental (a PAT login response). It does not hold for cookie auth, where the id is also definitive. connection/cookie-token.ts:64-73:
const instanceId = numeric(payload.instanceId ?? payload.instance_id)
|| numeric(getHeader(config.customHeaders, "Instanceid"))
|| await resolveInstanceIdByName(…)
if (!instanceId) throw new Error(`Unable to resolve instance id for '${config.instance}' from cookie auth.`)getCookieToken is awaited at line 66, before this block, and it has already either read the id out of the JWT instanceId claim, taken it from an explicit Instanceid header, or resolved it — and thrown if it could not. So for a cookie profile that pins header.Instanceid (or whose token carries the claim) and whose instance name does not come back from serviceInstanceList with serviceId === 1, the command used to work and now hard-fails. The name-based lookup is a strictly weaker source than the header the user set explicitly, but it wins here.
Two smaller points on the same block:
- For cookie auth this is a second
serviceInstanceListround-trip per command (getCookieTokenmay already have made one), on top of the one this branch adds for every profile with no cachedinstance_id. test/exec-instance-id.test.tshas 9 cases, and none of them exercises the cookie path — so the interaction above is untested in either direction. (I can't run tests, so this is from reading the case list, not from a run.)
Suggestion: move the token.instanceId branch ahead of the notListed throw, or gate the throw on the credential not carrying an id (else if (notListed && !token.instanceId)), so an explicitly-supplied id is preferred over a name lookup that disagrees with it.
| } else if (legacyOAuthInstanceId(activeProfile)) { | ||
| // Last resort, and only to preserve what used to work: an OAuth profile with no | ||
| // cached id and no usable lookup. The value comes from the shared `[oauth.<id>]` | ||
| // section an older version wrote — per-profile data in a shared place, i.e. the very | ||
| // thing this change removes — so it is used but never cached, and never quietly. | ||
| const legacy = legacyOAuthInstanceId(activeProfile)! |
There was a problem hiding this comment.
LOW — section B (small, mechanical). Confidence: high.
legacyOAuthInstanceId(activeProfile) runs twice — once as the branch predicate, once to get the value:
} else if (legacyOAuthInstanceId(activeProfile)) {
…
const legacy = legacyOAuthInstanceId(activeProfile)!legacyOAuthInstanceId in connection/profile-store.ts reads and TOML-parses ~/.clickzetta/profiles.toml on each call, so this is two file reads plus two parses on a path that is already the slow fallback. Hoisting it above the if chain (const legacy = legacyOAuthInstanceId(activeProfile)) also removes the non-null !.
| const resolved = numeric(match?.id ?? match?.instanceId) | ||
| if (!resolved) opts?.onNotFound?.() |
There was a problem hiding this comment.
LOW — section D (correctness edge). Confidence: high on the behaviour, low on whether it matters today.
const resolved = numeric(match?.id ?? match?.instanceId)
if (!resolved) opts?.onNotFound?.()onNotFound is documented as "the lookup SUCCEEDED and no row matched the name" — a definitive statement about account ownership. But the predicate is !resolved, which also fires when a row did match and its id was absent, 0, or unparseable. Those are malformed-payload cases, not "this account does not own that instance".
That distinction matters because of how the one caller uses it: commands/exec.ts:111 turns onNotFound into a hard throw that deliberately overrides the credential's id. A portal response with a matching row but a junk id field would produce the "not listed for this account" error, which points the user at the wrong problem.
onNotFound firing only on match === undefined, with match && !resolved going down the onError path (or its own callback), would match the documented contract:
if (!match) opts?.onNotFound?.()
| if (src.instance) { | ||
| // The id travels WITH the name. A layer that carries its own id wins; a layer that | ||
| // names the SAME instance without one leaves the id alone; only a layer naming a | ||
| // DIFFERENT instance clears it, because a numeric id is meaningless for another | ||
| // instance and getExecContext must resolve the right one. | ||
| // | ||
| // The distinction is not academic: ConnectionEnv has no CZ_INSTANCE_ID, so an exported | ||
| // CZ_INSTANCE arrives with instanceId undefined. Clearing unconditionally meant anyone | ||
| // who exports CZ_INSTANCE — even naming the profile's own instance — paid a | ||
| // serviceInstanceList round trip on every single command, forever. | ||
| const changed = src.instance !== target.instance | ||
| target.instance = src.instance | ||
| if (src.instanceId !== undefined) target.instanceId = src.instanceId | ||
| else if (changed) target.instanceId = undefined |
There was a problem hiding this comment.
LOW — section C. Confidence: medium (behaviour is clear; blast radius is narrow).
The whole instanceId carry is nested inside if (src.instance):
if (src.instance) {
…
const changed = src.instance !== target.instance
target.instance = src.instance
if (src.instanceId !== undefined) target.instanceId = src.instanceId
else if (changed) target.instanceId = undefined
}So a source that carries instanceId but no instance never contributes its id. That combination is reachable: getProfileConfig writes instance: str(profileData.instance, ""), and "" is falsy, so a profile row with instance_id set and instance missing/empty silently drops the id at this layer.
For getExecContext this is moot — the pre-existing if (!config.instance) throw at commands/exec.ts:60 fires first. It is not moot for commands/studio-context.ts, which now does fallbackId: config.instanceId ?? credential.instanceId at two call sites; there the id would have been usable and gets discarded in favour of the credential's.
Hoisting the two instanceId lines out of the if (src.instance) block (keeping the changed-clears rule, which needs src.instance to be meaningful) would cover it. Low priority if a profile with instance_id and no instance is considered malformed by construction — worth saying so in the comment if that's the intent.
| @@ -42,11 +60,13 @@ export async function resolveInstanceIdByName( | |||
| && numeric(row.serviceId ?? 1) === 1, | |||
There was a problem hiding this comment.
Confirm intent, not a bug claim — section C. Confidence: medium that the asymmetry is real, no view on whether it's wrong.
&& numeric(row.serviceId ?? 1) === 1,This serviceId === 1 (Lakehouse) narrowing is what makes onNotFound — and therefore commands/exec.ts:111's new hard throw — fire. Login-time instance discovery does not apply an equivalent filter. commands/login-browser.ts:181-191:
const list = Array.isArray(body.instanceList) ? … : []
return list.map((inst) => {
const instanceId = typeof inst.id === "number" ? inst.id : 0
const instanceName = str(inst.name) ?? ""
if (!instanceId || !instanceName) return undefined
…Different endpoint and different payload shape, so the two aren't strictly comparable, but the consequence is: an instance the user can pick during auth login and have written into a profile may be one that serviceInstanceList later declines to return with serviceId === 1. Before this PR that degraded to fallbackId and the command carried on with the credential's id; now it is a fatal "Instance '…' is not listed for this account" — a message that would be actively misleading in that case, since the account does own it.
Intended (Lakehouse-only is the supported surface, and failing loudly is the point), or should the picker narrow to the same set so the two can't disagree? Either way nothing in test/exec-instance-id.test.ts pins the matched-row-but-wrong-serviceId case, so the current answer isn't locked in.
| // Same policy as readProfileInfo, and for the same reason: substitute the first | ||
| // TOML profile ONLY when nothing is pinned (genuinely unconfigured). A `current` | ||
| // that names a profile absent from the file — stale CZ_PROFILE, or a | ||
| // default_profile pointing at a deleted profile — must report no balance rather | ||
| // than silently bill a DIFFERENT tenant's account to the user. The pre-header | ||
| // version of this function got that right by a different route (it gated the | ||
| // billing read on `name === current` in two places); collapsing the profile walk | ||
| // dropped both gates, so it is spelled out here. | ||
| const name = current === undefined ? Object.keys(profiles)[0] : profiles[current] ? current : undefined |
There was a problem hiding this comment.
LOW/MEDIUM — section B (logic copied into a second place). Confidence: high.
const name = current === undefined ? Object.keys(profiles)[0] : profiles[current] ? current : undefinedThis line is byte-identical to readProfileInfo's at line 204, and the comment above it says so explicitly ("Same policy as readProfileInfo, and for the same reason") — including the history that collapsing the profile walk already dropped these gates once.
That history is the argument for extracting it rather than restating it. The rule being encoded is subtle and has a security-ish consequence spelled out in the comment ("must report no balance rather than silently bill a DIFFERENT tenant's account"), and it now lives in two places that must stay in sync with nothing enforcing that. A shared resolveActiveProfileName(profiles, current) would make the next person changing the policy change it once, and would give the comment one home instead of two.
Not a correctness finding as written — both copies agree today.
| ...(loaded.find((item) => item.name === current)?.billing ?? {}), | ||
| ...(loaded.map((item) => item.usage).find((usage) => Object.keys(usage).length > 0) ?? {}), | ||
| } | ||
| return fetchProfileSnapshot({ name, profile: profiles[name]!, signal: input.signal }) |
There was a problem hiding this comment.
LOW — section B (comment left stale by this refactor). Confidence: high.
return fetchProfileSnapshot({ name, profile: profiles[name]!, signal: input.signal })Collapsing the profile walk into this single call removed fetchProfileSnapshot's use of readCurrentUserName, but readCurrentUserName's docstring at line 244 still claims otherwise:
* The one call site bothfetchProfileUserNameandfetchProfileSnapshotshare,
Grep now gives exactly one caller — fetchProfileUserName at line 303. Worth updating in the same commit; a comment asserting a sharing constraint that no longer exists is the kind of thing that gets preserved by the next reader for no reason. (Line 244 isn't in the diff, so I couldn't anchor this to it directly.)
| const abs = Math.abs(value) | ||
| // A lifetime (PTO) ceiling is routinely 1e9+, and without this tier it renders as | ||
| // `1000.0M` — six characters of noise in a 42-column sidebar. | ||
| if (abs >= 1_000_000_000) return `${(value / 1_000_000_000).toFixed(1)}B` |
There was a problem hiding this comment.
LOW — section C (output shape change, looks intended). Confidence: high on the change, high that it's deliberate.
if (abs >= 1_000_000_000) return `${(value / 1_000_000_000).toFixed(1)}B`Recording it explicitly since section C asks for every behavioural change: abbreviate previously rendered a billion-scale value as 1000.0M, and now renders 1.0B. Anything reading the sidebar text — a screenshot-diff test, a doc example, a user's mental parse of the footer — sees a different string at that magnitude.
This one is covered: test/tui-quota-format.test.ts:23-26 pins both sides of the new boundary (1_000_000_000 → "1.0B", 978_693_583 → "978.7M", 21_306_417_000 → "21.3B"), and abbreviate is only reachable from the TUI display path, not from --format json, so no script output changes. No action needed — flagging for the changelog, not for a fix.
| // job_id travels on failures too, so a failed query can still be traced via | ||
| // `cz-cli job profile <id>` the same way a successful one can. | ||
| const extra = { ...(hint ? { schema: hint } : {}), ...(r.jobId ? { job_id: r.jobId } : {}) } | ||
| error(r.errorCode ?? "SQL_ERROR", await formatQueryError(r, ctx, profileName), { | ||
| format, | ||
| extra: hint ? { schema: hint } : undefined, | ||
| extra: Object.keys(extra).length > 0 ? extra : undefined, | ||
| ...(aiMessage && { aiMessage }), | ||
| }) |
There was a problem hiding this comment.
LOW/MEDIUM — section C (error output shape changes on paths that previously had none). Confidence: high on the changes, medium on whether any of them matters to a consumer.
const extra = { ...(hint ? { schema: hint } : {}), ...(r.jobId ? { job_id: r.jobId } : {}) }
error(r.errorCode ?? "SQL_ERROR", await formatQueryError(r, ctx, profileName), {
format,
extra: Object.keys(extra).length > 0 ? extra : undefined,Folding the two inline FAILED branches into this shared helper is the right direction — it's the "fix the one shared function" move, not a hole drilled around the problem, and it removes the drift the comment at line 683 describes. Recording the observable consequences since section C asks for them:
--format jsonfailures gain ajob_idkey (andschemawhere a hint resolves) on both the single-statement path and the multi-statement intermediate path. Additive, so a consumer reading known keys is fine; a consumer asserting on exact object shape, or snapshotting the error payload, sees a diff.- Row/table formats gain an extra stderr line for the same reason.
- The single-statement failure path gains
aiMessage. - The multi-statement intermediate-failure path gains a
fetchSchemaHintquery — one additional round-trip on failure where there previously was none, plusai_messageandtimeMsonlogOperation.
test/sql-error-job-id.test.ts covers the job_id-on-failure behaviour. I have not found a test pinning the multi-statement intermediate path's new schema-hint query, so the extra round-trip there looks unpinned. (I can't run tests, so this is from reading the files, not from a run.)
No fix suggested — this is a note for the changelog if --format json error output is treated as a contract.
| case "$PLATFORM" in | ||
| win32-*) DEFAULT_BINARY_NAME="cz-cli.exe" ;; | ||
| *) DEFAULT_BINARY_NAME="cz-cli" ;; | ||
| esac | ||
| # \${BINARY_NAME:-…}: setup.sh reads BINARY_NAME as an override, so an environment that | ||
| # already set it keeps winning. For every POSIX platform the default below is the same | ||
| # name setup.sh would have chosen on its own, which makes this line a no-op there. | ||
| BINARY_NAME="\${BINARY_NAME:-$DEFAULT_BINARY_NAME}" |
There was a problem hiding this comment.
LOW — section B (same mapping now stated twice, in two languages). Confidence: high.
case "$PLATFORM" in
win32-*) DEFAULT_BINARY_NAME="cz-cli.exe" ;;
*) DEFAULT_BINARY_NAME="cz-cli" ;;
esacThis restates in generated shell what platformBinary at line 69-71 already computes in JS:
function platformBinary(platform) {
return platform.startsWith("win32") ? "cz-cli.exe" : "cz-cli"
}And the generator already has a place to emit per-platform values: renderShellPlatformCase (line 350) writes ARCHIVE_URL / ARCHIVE_NAME / ARCHIVE_FORMAT / ARCHIVE_CHECKSUM into exactly this kind of case block. Adding BINARY_NAME=${shellQuote(platformBinary(platform))} there would reuse the existing function and keep one definition of "which platforms ship a .exe", instead of two that can drift — the drift being silent, since it only shows up on a Windows install.
Minor, and the current version is correct as written.
| if command -v powershell > /dev/null 2>&1 && command -v cygpath > /dev/null 2>&1; then | ||
| # Both paths come from mktemp, which honours \$TMPDIR — so they are environment-supplied, | ||
| # and a \`'\` in one would close the PowerShell single-quoted string early and let the | ||
| # rest parse as commands. PowerShell escapes a literal quote by doubling it. | ||
| PS_SRC=$(cygpath -w "$1" | sed "s/'/''/g") | ||
| PS_DST=$(cygpath -w "$2" | sed "s/'/''/g") | ||
| powershell -NoProfile -NonInteractive -Command "Expand-Archive -LiteralPath '$PS_SRC' -DestinationPath '$PS_DST' -Force" && return |
There was a problem hiding this comment.
LOW — section D. Confidence: high on the gap, low on how often it's hit.
if command -v powershell > /dev/null 2>&1 && command -v cygpath > /dev/null 2>&1; thenpowershell is the Windows PowerShell 5.1 stub. PowerShell 7 installs as pwsh and does not provide a powershell alias, so a host with only PowerShell 7 on PATH fails this probe, skips the branch, and lands on the print_error + exit 1 at line 415-416 — even though Expand-Archive is available to it. That's the third and last fallback, so it's the difference between the installer working and the installer refusing to run.
for ps in powershell pwsh; do command -v "$ps" && … (or just testing both in the condition and using the found one) closes it.
Separately, and worth noting as a positive: the sed "s/'/''/g" doubling on both mktemp-derived paths is the right treatment for the quote-injection hazard the comment describes, since these do come from $TMPDIR.
| try { | ||
| writeFileSync(file, body, { encoding: "utf-8", mode: 0o600 }) | ||
| } catch { | ||
| // Only the first write of a fresh install needs the directory; paying mkdirSync on | ||
| // every request to create something that already exists is a syscall per LLM call. | ||
| // | ||
| // 0700/0600 to match llm/native-config.ts. It matters which writer gets there first: | ||
| // mkdirSync does not tighten a directory that already exists, so if this one creates | ||
| // ~/.clickzetta at the default 0755, llm.json — which holds api_key in plaintext — is | ||
| // later created inside a world-readable directory. chmod after the write because an | ||
| // existing file keeps its old mode. | ||
| mkdirSync(dirname(file), { recursive: true, mode: 0o700 }) | ||
| writeFileSync(file, body, { encoding: "utf-8", mode: 0o600 }) | ||
| } | ||
| try { | ||
| chmodSync(file, 0o600) | ||
| } catch { | ||
| // An unwritable mode is not worth failing a cache write over. | ||
| } |
There was a problem hiding this comment.
LOW — section D (cost on the hot path; the stated rationale argues against what the code does). Confidence: high.
mkdirSync(dirname(file), { recursive: true, mode: 0o700 })
// Only the first write of a fresh install needs the directory; paying mkdirSync on
// every request to create something that already exists is a syscall per LLM call.
The reasoning is sound, but recordGatewayQuota runs on every LLM request and already pays, per request: readFileSync + JSON.parse (line 65), a full Object.entries retention sweep, JSON.stringify (line 73), writeFileSync (line 75), and chmodSync (line 89) — five-plus synchronous filesystem operations, in the opencode server's event loop, to avoid one. So the optimisation the comment justifies is dominated by what surrounds it.
Not urgent: the file is bounded small by RETENTION_MS, and the write is already off the response's critical path in doStream. But if the per-request cost is worth caring about, the lever is the read-modify-write itself — e.g. keep the parsed store in a module-level variable and re-read only on write failure, or debounce writes — rather than the mkdirSync. Alternatively drop the comment's cost claim, since the try/catch-first shape is defensible on its own (it's also the correct ordering for the 0700 concern the rest of the comment raises).
Two things I checked and found fine, noting them so they don't get re-raised: the catch branch does retry the write after mkdirSync, so a fresh install doesn't lose its first entry; and the non-atomic writeFileSync is safe for readers because readStore catches the truncated-parse case.
| api.event.on("message.part.updated", (event) => { | ||
| if (event.properties.part?.type !== "step-finish") return | ||
| refreshHeaderQuota() | ||
| }), |
There was a problem hiding this comment.
LOW — confirm intent, section C. Confidence: high that the asymmetry is real, medium that it's harmless.
api.event.on("message.part.updated", (event) => {
if (event.properties.part?.type !== "step-finish") return
refreshHeaderQuota()
}),This is the only one of the three subscriptions in this array that doesn't scope itself to the current session. session.status passes event.properties.sessionID through to controller.observeStatus, and activeModel.onChange guards with if (sessionID !== currentSessionID(api)) return. This handler fires on a step-finish part from any session — including a background/subagent session, or another session sharing the same server.
My read is that it's benign, because refreshHeaderQuota derives its key from activeModel.providerID() rather than from the event, so a foreign session can only cause an extra read of the active provider's own row — not a reading attributed to the wrong credential. Two things make it worth confirming rather than assuming:
- If a background session is running against a different llm.json entry, its
step-finishstill triggers a re-read keyed to the foreground provider, so the sidebar can refresh at a moment when the foreground key's row hasn't changed. Harmless with the sticky "only set when truthy" merge, but it means refresh frequency is coupled to unrelated activity. - It's the per-request path, so an unscoped subscription multiplies the cost below by the number of concurrently active sessions.
If the lack of a filter is deliberate (any LLM request anywhere is a good moment to re-read), a one-line comment saying so would keep the next reader from "fixing" it to match its siblings.
| // One step-finish part per LLM request, which is exactly when the gateway has | ||
| // reported a new quota — and the only response-adjacent signal a TUI plugin can | ||
| // observe (the plugin API exposes no response hook, and opencode's processor | ||
| // drops the finish metadata). Reading the cache here is local and synchronous, | ||
| // so it costs nothing to do per request instead of per turn. |
There was a problem hiding this comment.
LOW — section D (the comment's cost claim doesn't match what the call does). Confidence: high.
// Reading the cache here is local and synchronous,
// so it costs nothing to do per request instead of per turn.
"Local and synchronous" is right; "costs nothing" understates it. refreshHeaderQuota → readHeaderQuota (tui-quota-data.ts:331) does two synchronous filesystem reads plus two JSON.parses per call: classifyClickzettaEntry resolves the entry out of llm.json, then readGatewayQuota reads and parses ~/.clickzetta/gateway-quota.json. That happens on the TUI's thread, once per LLM request, and — per the sibling comment about the missing session filter — once per request across all active sessions.
Still small in absolute terms, and doing it per request rather than per turn is the right call for freshness. But "synchronous" is the reason to be careful here, not the reason not to be: the same request also triggers a synchronous read-modify-write of the same file in the server process (quota-store.ts:61), so the pair of processes are doing sync I/O on one small JSON file on every LLM call. Caching the resolved entry (it only changes when the provider changes, which already has its own subscription at line 237) would remove one of the two reads for free.
No behaviour change needed — I'd just soften the comment so it doesn't discourage measuring this later.
| // The instance id the wire needs belongs to the CONNECTION, not the credential — see | ||
| // ConnectionConfig.instanceId. A profile written before that carries no `instance_id`, so | ||
| // resolve it from the name the profile does carry and cache it back. One extra portal | ||
| // call, once per stale profile; a login records it up front. | ||
| if (!config.instanceId) { |
There was a problem hiding this comment.
MEDIUM — section C, enumerating what this block changes for already-working setups. Confidence: high on the enumeration.
if (!config.instanceId) {Every command routed through getExecContext (sql, job, table, and the studio/gateway contexts downstream) now enters this block whenever the profile has no cached instance_id — which is every profile written before this PR. What changes for those:
- One new
serviceInstanceListround-trip per command, until the id is cached back. It's only cached whennameIsProfiles(line 90), so an invocation that overrides the name via--instance,CZ_INSTANCEor--jdbc-urlpays the call every time, forever. That's correct — caching an overridden id would pin the profile wrongly — but it's a permanent latency change for anyone who drives cz-cli with--instance, and it's worth saying so in the release notes. - Two new fatal paths where the command previously proceeded: the
notListedthrow at line 117, and the catch-all at line 156 (noaccount_id, or a failed lookup with no credential id and no legacy OAuth id). Before this PR the id degraded tofallbackId/token.instanceIdand the query ran. - A new profile write (
patchProfileInstanceId) on the success path.
On coverage — test/exec-instance-id.test.ts has 9 cases and they pin most of this well, including the deliberate never-cache-an-override rule and "a name the account does not list is fatal even for a PAT credential". The gaps I can see:
- No case for cookie auth, where
getCookieTokenhas already produced a definitive id (see my other comment on lines 111-126). - No case for the legacy-OAuth fallback branch at line 142.
- No case for the "lookup failed transiently, credential has an id" branch at line 121, i.e. the warning path.
I can't run the suite, so this is from reading the case list, not from a run.
4c4dc9f to
9a721f6
Compare
| // No instance_id: this section is SHARED by every profile the login can reach, and an | ||
| // instance is per-profile — see ConnectionConfig.instanceId. Sections written by older | ||
| // versions still carry one; parseOAuthEntry ignores it. |
There was a problem hiding this comment.
MEDIUM (confidence: high) — "Sections written by older versions still carry one" stops being true after the first token refresh, which breaks the fallback this PR added to depend on it.
save() assigns the section wholesale from this function:
shared[id] = tokenToEntry(token) // line 783So when getToken rotates an expired OAuth token, [oauth.<id>].instance_id is dropped from disk permanently. oauth-section-hygiene.test.ts:221 pins exactly that behaviour.
That is the same number legacyOAuthInstanceId reads, and getExecContext reads it after the refresh has already happened:
const token = await getCookieToken(config) ?? await getToken(config) // may refresh → save() → erases instance_id
...
const legacyId = legacyOAuthInstanceId(activeProfile) // reads the value just erasedFailure scenario: OAuth profile with no instance_id on the profile yet, access token past its 60-minute TTL, and serviceInstanceList failing (the measured 8888 case on a tencentcloud region host, or any transient portal error). The refresh succeeds and wipes instance_id; the lookup fails; token.instanceId is undefined for OAuth; legacyId is gone → the final throw in getExecContext. The same invocation ran before this PR (on the shared token's id). It is also not recoverable by retrying, because the number is off disk for good.
exec-instance-id.test.ts:275 exercises the fallback only with a non-expired token (obtained_at = Date.now()), so the refresh interaction is untested.
Either of these closes it:
- preserve the key when rewriting — read the existing entry and carry
instance_idforward when the token has none, so this PR does not destroy data it still reads; or - migrate instead of reading forever: consult
legacyOAuthInstanceIdbeforegetTokeningetExecContextand write it onto the profile withpatchProfileInstanceIdon first use. That is what the ledger says this change is for — moving per-profile data out of the shared section — rather than leaving a read of a field nothing maintains.
| // There, "this account does not list it" contradicts something explicit and falling | ||
| // back to a credential's id would silently run against a DIFFERENT instance than the | ||
| // one named; that was the silent case worth closing. | ||
| // | ||
| // A name that came from the PROFILE is not fatal, because the credential's id can be | ||
| // just as definitive: cookie auth derives it from the cookie's own JWT payload | ||
| // (cookie-token.ts), and that cookie is scoped to an instance. Failing there would | ||
| // discard an authoritative answer over a lookup that can also miss for reasons other | ||
| // than ownership — a renamed instance, or one that is not serviceId 1. | ||
| throw new Error( | ||
| `Instance '${config.instance}' is not listed for this account on ${config.service}. ` + |
There was a problem hiding this comment.
MEDIUM — behaviour change worth confirming: an exported CZ_INSTANCE (or --instance / --jdbc-url) whose name serviceInstanceList does not return with serviceId === 1 now makes every command fail hard.
} else if (notListed && !nameIsProfiles) {
throw new Error(
`Instance '${config.instance}' is not listed for this account on ${config.service}. ` + …nameIsProfiles is entry.instance === config.instance, so the throw covers any name that came from outside the profile. Before this PR the same invocation ran using ctx.token.instanceId. Cases that reach it without anything actually being misconfigured:
- an instance the account genuinely owns but which
resolveInstanceIdByNamefilters out — theserviceId === 1narrowing, or a name that no longer matchesrow.name/row.instanceNameafter a rename; - a shell profile that exports
CZ_INSTANCEfor a value the current account does not list (switching accounts with the variable still set) — everycz-cli sql/job/table/fscall fails until it is unset, where it used to run.
Is failing hard on CZ_INSTANCE intended, or was the intent narrower — fatal only for a name the user typed on this invocation (--instance / --jdbc-url), with an inherited env value degrading the way resolveConnectionConfig's own docstring reasons about CZ_PROFILE ("CZ_PROFILE is the CLI's own channel … throwing on it here would also hit the callers that merely read a config")? exec-instance-id.test.ts:229 pins the PAT case but not the env-supplied one, so the distinction isn't currently locked either way.
| config.instanceId = resolved | ||
| if (nameIsProfiles) patchProfileInstanceId(activeProfile, resolved) | ||
| } else if (notListed && !nameIsProfiles) { | ||
| // Fatal only for a name the caller supplied — --instance, CZ_INSTANCE, --jdbc-url. |
There was a problem hiding this comment.
MEDIUM (confidence: high) — when the name did not come from the profile, the resolved id is never memoised anywhere, so serviceInstanceList is re-requested on every command for the rest of that configuration's life.
if (resolved) {
config.instanceId = resolved
if (nameIsProfiles) patchProfileInstanceId(activeProfile, resolved)
}Not caching onto the profile is right, and the comment above explains why. But there is no other cache either, and config is rebuilt per invocation — so a user who exports CZ_INSTANCE with a value different from the profile's instance pays one extra portal round-trip on every cz-cli sql, job, table, fs, and on sql --dry-run. applyNonAuth's new changed guard was added precisely to stop that cost for the same-name case ("anyone who exports CZ_INSTANCE … paid a serviceInstanceList round trip on every single command, forever"), and the different-name case still has it.
Failure scenario: export CZ_INSTANCE=other-inst in a shell, then a script running 200 cz-cli sql invocations issues 200 extra serviceInstanceList requests and adds a portal RTT to each command's latency — and each one becomes a new way for the command to fail if the portal is having a bad minute.
The smaller correct change is a name→id memo keyed on service + accountId + instance, either process-level (helps the single long-running process) or in a small on-disk cache next to the other ~/.clickzetta state (helps the script case, which is where the cost actually lands). Either way it stays out of profiles.toml, so the reason the profile write is gated is preserved.
| entries[gatewayQuotaCacheKey(input.baseURL, input.apiKey)] = { updated_at: now, quotas: input.quotas } | ||
| const body = JSON.stringify({ entries }, null, 2) | ||
| try { | ||
| writeFileSync(file, body, { encoding: "utf-8", mode: 0o600 }) |
There was a problem hiding this comment.
LOW (confidence: high) — non-atomic write, and a torn read here loses every cached entry, not one.
writeFileSync(file, body, { encoding: "utf-8", mode: 0o600 })writeFileSync truncates the live path, so a reader in the TUI process (readHeaderQuota runs on every step-finish) can observe a half-written body. readStore catches that and returns {} — correct for the reader, but recordGatewayQuota uses the same readStore, so a torn or corrupt read there makes the next write persist { entries: { <this one key> } }. Every other endpoint/key's reading is silently dropped.
The docstring says "two servers finishing a request at the same instant can lose one entry"; the actual loss is all entries but one.
Failure scenario: two opencode servers (two TUI windows, or a serve alongside a TUI) on two llm.json entries. Server A is mid-writeFileSync; server B reads the truncated body, gets {}, and writes back only its own key. A's reading is gone, and the sidebar for A's window falls back to blank until its next request.
Both profile-store.ts:20 and llm/native-config.ts:359 already use temp-file + renameSync for exactly this. Doing the same here removes the torn-read window entirely and costs nothing on the happy path — the mkdirSync-on-failure retry structure works unchanged, just against the temp path.
| const resolved = classifyClickzettaEntry(providerID) | ||
| return resolved.kind === "clickzetta" ? { name: resolved.name, apiKey: resolved.apiKey } : undefined | ||
| function periodResetSince(period: QuotaPeriod | undefined, readAt: number, now: number): boolean { | ||
| if (period !== "daily") return false |
There was a problem hiding this comment.
LOW (confidence: medium) — monthly is as calendar-determined as daily and could use the same rule; leaving it to the 6h bound reproduces, once a month, the failure this function exists to prevent.
if (period !== "daily") return falseThe docstring groups weekly and monthly together as "the gateway's own (and a week's start is a convention)". That holds for weekly — the start day genuinely is a convention. It does not hold for monthly: a month boundary is the 1st, in whichever zone, and the same conservative both-zones test already written for daily answers it exactly as well:
then.getMonth() !== current.getMonth() || then.getUTCMonth() !== current.getUTCMonth()Failure scenario: a key with a PMO limit near exhaustion, last request at 23:00 on the 31st, TUI left open, user returns at 02:00 on the 1st (3h later, inside HEADER_QUOTA_MAX_AGE_MS). The sidebar paints last month's near-exhausted spend against this month's fresh limit, in error tone — the exact "tells a user with a full allowance that they are out" direction the daily rule calls out as the worst one to be wrong in.
Small window (up to 6h, once a month), hence LOW, but it is the same defect with the same available fix.
| export function execInstanceId(ctx: ExecContext): number { | ||
| return ctx.config.instanceId ?? 0 | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — the ?? 0 is the silent zero this whole change exists to remove, kept as a default in the accessor named after it.
export function execInstanceId(ctx: ExecContext): number {
return ctx.config.instanceId ?? 0
}Today it is unreachable: getExecContext is the only constructor of ExecContext (grepped — sql.ts, job.ts, table.ts, fs.ts, profile.ts all go through it) and it either sets config.instanceId or throws. So this is dead, and that is the problem with it: the day someone builds a context another way, job profile/table summary/newJobId go back to submitting under instance_id = 0 with no diagnostic, which is precisely the class of bug the ledger entry and the new tests are about.
throw new Error("instance id unresolved — getExecContext should have resolved or failed") costs nothing and makes the invariant enforced rather than assumed. Same reasoning the SDK's newJobId guard in session.ts follows (if (this.config.instanceId) first, token only as a documented fallback).
| // The connection's own id first: it is the authoritative one now (see | ||
| // ConnectionConfig.instanceId), and for an OAuth profile the credential's is 0 — | ||
| // `[oauth.<id>]` no longer stores one, so it cannot be a fallback for anything. | ||
| fallbackId: config.instanceId ?? credential.instanceId, |
There was a problem hiding this comment.
LOW (confidence: high) — the fix landed on the exec path only; the studio/gateway path keeps the old shape, where an unresolvable id degrades to a silent 0.
fallbackId: config.instanceId ?? credential.instanceId,For an OAuth profile credential.instanceId is now hard-wired to 0 (toCredential's token.instanceId ?? 0), and the comment says as much. So when serviceInstanceList fails and the profile has no cached instance_id, resolveInstanceIdByName returns 0 and the studio request goes out with instanceid: "0" — the exact silent-wrong-instance shape getExecContext now refuses to produce.
It also does not participate in the backfill: patchProfileInstanceId is only called from getExecContext, so a user who only ever runs studio-side commands (job profile, task, analytics-agent, integration) pays a serviceInstanceList round-trip on every invocation and never caches.
Not a correctness regression versus before this PR — the fallback was a wrong id then and is 0 now, and the live lookup is still the primary source here — so LOW. But it means two call paths answer "which instance id" with two different policies. If the exec path's policy is right (resolve, cache, fail loudly), extracting it into one helper both paths call would leave one answer instead of two; if it is deliberately exec-only, a line saying why would keep the next reader from assuming the studio path is covered.
| const started = Date.now() | ||
| const r = await execSqlWithRetry(ctx, stmt, { hints: accumulatedHints, timeoutMs: argv.timeout * 1000, configStatements }) | ||
| if (isQueryResult(r) && r.status === JobStatus.FAILED) { | ||
| logOperation("sql", { sql: stmt, ok: false, errorCode: r.errorCode }) | ||
| error(r.errorCode ?? "SQL_ERROR", await formatQueryError(r, ctx, argv.profile), { format }) | ||
| // Through the shared helper, like every other failure path. Adding job_id | ||
| // inline here is what the single-statement branch used to do, and folding that | ||
| // one in is the reason this file has one failure reporter: an inline copy gets | ||
| // job_id but not the schema hint, the ai_message or the timing, so the two | ||
| // paths drift apart again the moment either gains something. | ||
| await handleFailure(r, stmt, ctx, format, started, argv.profile) |
There was a problem hiding this comment.
LOW (confidence: high) — folding this path into handleFailure is the right call (one failure reporter, and the duplicated inline copy is gone), but it adds a network round-trip to a path that is already reporting a failure.
handleFailure opens with await fetchSchemaHint(...), and formatQueryError → resolveAccountDisplayName adds a getCurrentUser call. Previously this branch issued neither: it logged and called error() directly. So a multi-statement run whose intermediate statement fails now issues an extra query plus a portal call before printing the error.
The single-statement branch already paid both, so this is consistency rather than a new class of cost — noting it because the failure path is exactly where the connection is most likely to be the thing that is broken, and fetchSchemaHint's own failure is swallowed (returns undefined) so the user sees a slower error rather than a different one. If fetchSchemaHint has no timeout of its own, a hung metadata query turns a fast SQL error into a hang.
Behavioural surface, for the record: job_id is a new key in the json envelope and a new stderr line in row formats on every SQL failure, and this branch additionally gains ai_message and timeMs. test/sql-error-job-id.test.ts covers both failure paths. I can't run it, so this is a read of the test, not a pass.
2 participants
Everything from this working session, as three commits. Stacked on #86 — the sidebar reads
its portal token through
profileTokenSource, which only exists on that branch, so thiscannot target
mainuntil #86 lands. GitHub retargets automatically once #86 merges.feat(gateway): read the key's token quota from the response headersfix(tui): preserve spinner registration (backport upstream #35292)fix(sql): report job_id on a failed query, not only a successful onefeat(install): install from Git Bash / MSYS2 / Cygwin on Windowsfix(auth): keep the instance id on the profile, not on the shared OAuth token1. Token quota from the gateway's response headers
"How much token quota is left on this key" now comes from the AI gateway's own response
headers instead of a portal poll. Every successful
/gateway/v1/chat/completionsresponsecarries the key's allowance for each configured period:
This is the only quota source a plain API key can reach.
ai-gateway key listreports thesame numbers but needs a portal token and ownership of the key, so it cannot answer "how
much is left on the key I am using right now".
New in
@clickzetta/ai-gatewayquota.ts— parses those headers. A key with several periods repeats all four headersonce per period, and both
Headersand the AI SDK'sSharedV3Headerscollapseduplicates into one comma-joined value, so the lists are zipped positionally.
undefinedmeans "not reported", never "zero" and never "no limit" — the headers areabsent on every error response (429 included) and on a gateway that predates the feature.
quota-store.ts—~/.clickzetta/gateway-quota.json, keyed bybaseURL#sha256(apiKey)[:16]. Only the provider sees the response headers and it runs inthe opencode server; the sidebar that displays them runs in the TUI process, and the TUI
plugin API exposes no response-side hook (
chat.headersis outbound only, the event busis a fixed set, and opencode's processor drops the
step-finishmetadata). A file waspreferred over an intrusive patch to that processor — the de-opencode invariant in
packages/cz-cli/UPSTREAM-PATCHES.mdis worth more. The store is a cache, never a sourceof truth, with a 7-day retention so it cannot grow without bound.
providerMetadata.clickzetta.quotaondoGenerateand on the stream'sfinishpart, and writes the cache as soon as theheaders exist — for a stream that is when it opens, so an aborted turn does not throw
away a reading already in hand. When the gateway reports nothing the result comes back
byte-identical.
New command —
cz-cli ai-gateway quota [llm]sends a 1-token chat request and reads theheaders off the response. No portal credentials, no ownership of the key. A spent key 429s
with no quota headers at all, so that case is reported through the gateway-error classifier
rather than swallowed as "quota unknown"; a 404 for a probe model the caller never named is
reported as its own condition, because it says nothing about the quota.
cz-cli agent llm testalso prints the quota — the probe already makes a completion, so itcosts nothing extra.
TUI sidebar — the token half switches to the local cache, refreshed on each
step-finishpart (one per LLM request); the balance half keeps its busy→idle portal read.That removed a whole mechanism from
fetchQuotaSnapshot: it used to walk every locallyconfigured Profile hunting the one whose portal knew the selected virtual key, because an
LLM entry and a Profile are independent configuration domains. The headers answer for the
key directly, so the balance needs exactly one Profile — net -144 lines. Every configured
period now gets its own rows, shortest window first, since the binding limit is whichever
runs out first.
UPSTREAM-PATCHES.mdHOOK entry 5 is updated for what this adds to that section'sdependencies: the
message.part.updated/step-finishevent shape, the new on-disk path,and the fact that the token half no longer touches Portal.
2. Windows
<spinner>crashOn Windows the released binary crashed the session as soon as anything rendered a spinner:
Reported on cz-cli 2.0.3. Five sites registered
<spinner>with a bare side-effect import,import "opentui-spinner/solid". That package declares a whitelistsideEffects, so themodule survives tree-shaking only while the bundler matches those POSIX patterns against
the resolved path — and our release build compiles the win32 target on a
windows-latestrunner (release-cos.yml:runner: windows-latest+OPENCODE_HOST_ONLY=1). There the match fails,minifydrops an import that bindsnothing,
extend({ spinner })never runs, and the first spinner render throws.Release-only and Windows-only: dev
bun rundoes not tree-shake, and upstream bundlesevery target — Windows included — on Linux (
build-cliruns onblacksmith-4vcpu-ubuntu-2404; itswindows-2025runner only signs), which is whyupstream never saw it.
Fixed by backporting anomalyco/opencode#35292 verbatim. Our
baseline v1.17.11 (2026-06-25) predates it by nine days; upstream v1.18.9 and later already
carry it. Recorded as INTRUSIVE #12 in
UPSTREAM-PATCHES.md, scoped to self-delete oncethe baseline reaches v1.18.9, with a substitute grep because the edit deliberately carries
no
cz-cli changebanner (a banner would turn a clean fast-forward into a conflict).Measured: forcing the
sideEffectsmatch to fail collapses a bare-import bundle from935,609 bytes to 27 — registration and all. Cross-compiling with
--target=bun-windows-x64from macOS keeps it, confirming the build host rather than thetarget decides.
4.
job_idon a failed SQL queryA FAILED query's error payload now carries
job_id, so it can be traced withcz-cli job profile <id>the same way a successful one can. One duplicated failure path inexecuteSinglealso folds into the existinghandleFailurehelper.Both failure paths now report through that helper, and both gain what it does. The
single-statement path gains
ai_messagewhen a schema hint is available (its inlinedpredecessor already called
logOperationwith the same fields, so its telemetry isunchanged). The multi-statement intermediate path — folded in after review — gains more:
job_id, the schema hint andai_message, plustimeMson itslogOperationrecord, andit now issues one
fetchSchemaHintquery on failure where it previously issued none. Alladditive in the envelope, but it is a new stderr line for row formats and a new key for
json, so scripts diffing either will see it. Both paths are covered by
test/sql-error-job-id.test.ts.3. Installing on Windows from a POSIX shell
curl -fsSL https://cz-cli.ai/install.sh | bashin Git Bash answered:That is the raw
uname -s, lowercased and joined to the arch, becauseplatform()in thegenerated installer had no mapping for the MSYS family. It reads as "there is no Windows
build" when the win32 archives are published like every other platform.
mingw*|msys*|cygwin*now resolves towin32, matchinginstall.ps1's ownwin32-$Arch. Baseline included: the PowerShell installer does notselect the
-baselinebuild either, so a pre-AVX2 Windows CPU stays an open gap inboth installers rather than becoming a new one here.
unzip— the win32 archives are zips and Git Bash ships nounzip, so the one tool the old precheck demanded is the one a Windows user is leastlikely to have.
extract_ziptriesunzip, thentar -xf(Windows 10 1803+ shipsbsdtar as
tar.exe, which reads zip; GNU tar does not, hence discard-on-failure ratherthan a version test), then PowerShell's
Expand-Archivewithcygpath-translated paths.BINARY_NAMEpassed through — the win32 archives carrycz-cli.exe, andsetup.shalready took the name as an env override, so that is the whole of what it needed. Written
as
${BINARY_NAME:-…}so an environment that already set it still wins, and the defaultfor every POSIX platform is the same name
setup.shwould have picked itself.cz-agent.cmd— the existing extensionless sh wrapper cannot be executed by cmd.exeor PowerShell, so a
.cmdshim goes next to it.cz-cliitself needs none: MSYS bashappends
.exewhen resolving a command, and cmd/PowerShell do the same via PATHEXT."command not found" can both be true at once. The hint now says so and gives the
setxform.
ensureRestartBinaryAtPath— its candidate was a hardcoded extensionless~/.local/bin/cz-cli. Now that install.sh really putscz-cli.exethere, that candidatecould never exist and every auto-update on such an install ended at the throw. Keyed off
process.platformnow.Blast radius: everything above is gated on the MSYS family or
process.platform === "win32",except
extract_zip, which macOS installs also traverse — their archives are zips too.For them the executed command is unchanged (
command -v unzipsucceeds, then the sameunzip -qo); what changes is that a mac withoutunzipnow falls through to bsdtar insteadof aborting at a precheck, and the failure message is more specific. Linux is untouched: the
tar.gz branch is unchanged.
One behaviour change worth naming
With nothing pinned — no
CZ_PROFILE, nodefault_profile— the balance is now readand shown for the first profile in
profiles.toml. The pre-header code showed none on thatpath: it gated the billing read on
name === current, which was false for every profilewhen
currentwas undefined. The new behaviour is deliberate, sincereadProfileInfoalready names that same profile as the session's identity, so a figure beside it belongs to
the account being shown — but it is a new portal request and a new visible ¥ figure, not a
refactor, and it is pinned by its own test.
5. The instance id belongs to the profile, not the shared OAuth token
AuthTokencarriedinstanceId, and it was persisted into[oauth.<id>]— a section thatis deliberately shared by every profile one login provisions ("one per instance ×
workspace, all sharing ONE
[oauth.<id>]"). One section can hold one id, so it is wrong forevery profile but one. And the request path read it from there:
newJobId(workspace, token.instanceId)insql/session.tsandcommands/exec.ts, so a profile submitted jobsunder whatever instance the shared token happened to name.
Measured on a real
profiles.toml, not inferred:Four instances across four regions, one id. And it is not only a multi-instance hazard —
querying the portal for the default profile on that same machine returned:
The token's value came from userinfo's "default" instance, which was not the Lakehouse one
(
serviceId1, the only kindresolveInstanceIdByNameaccepts). So a single-profile,single-login setup was already submitting under the wrong id.
The fix.
ConnectionConfiggainsinstanceId, read from the profile'sinstance_id;every request-time read goes through it (
execInstanceId(ctx)in cz-cli,config.instanceIdin the SDK session). Provisioning writes it per profile from the enumeration, which already
carried it — so a login needs no extra request, and because that write is unconditional it
also corrects a value an older version cached. A profile without one is resolved by name via
serviceInstanceListon first use and written back, in the shapepatchProfileUserIdalready established.
[oauth.<id>]stops storing it, andparseOAuthEntrystops requiringit — that half matters as much: the old reader demanded the field, so dropping the write
without dropping the requirement would have made every newly written section read as invalid
and signed users out.
AuthToken.instanceIdis now optional rather than gone. Full removal would takelogin.ts,token.ts,cookie-token.ts,studio-context.ts,login-browser.tsand thepublic
Credentialshape with it, and a standalone SDK password login has no profile to readfrom — the login response is its only source. It is documented as not authoritative for a
profile-backed connection, and cz-cli no longer reads it anywhere.
Verification
@clickzetta/ai-gateway: typecheck clean, 72/72 tests.cz-cli: typecheck clean, full isolated suite green,test:e2e:help114/114.packages/tui: typecheck clean; suite unchanged from baseline (203 pass / 8 pre-existingtui syncfailures, identical with the patch reverted).packages/opencode:test/cli/run/footer.view.test.tsx— the one test that asserts aspinner renders, over a modified path — 22 pass / 0 fail.
stale-profile balance guard, the stream-cache write, the 404 probe-model branch and the
daily-reset freshness rule.
uat-aimesh(single period).cn-shanghai-alicloud-aimeshsends noquota headers at all as of 2026-09-01 while still enforcing a quota — there the token
rows are simply absent and the balance row still paints.
installer-windows.test.tsruns the GENERATED installer end to end against a fixturearchive with
unameandcurlstubbed, asserting a Windows install landscz-cli.exe,both
cz-agentwrappers, the runtime assets andinstall.json— plus a case proving theextraction fallback works with
unzipgenuinely unreachable, and a POSIX regression case.Reverting the platform mapping turns 4 of its 5 cases red.
spinner fix removes the dependency on the path match regardless of why it fails; the
installer's shell logic is covered by the end-to-end test above, but that the shipped
cz-cli.exethen runs is untested by it.Fixed after the first review pass
CZ_PROFILEno longer paints another account's cash balance. This was a realregression in the first revision, and its description wrongly called the fallback
pre-existing — it wasn't. The pre-header code reached the right outcome by a different
route (it gated the billing read on
name === currentin two places); collapsing theprofile walk dropped both gates.
fetchQuotaSnapshotnow follows the policyreadProfileInfoalready documents. Covered by a test, mutation-checked.(don't blank a live figure when a gateway sends no headers) is now separate from a
context reader that runs on mount, session switch and provider switch and clears when the
now-active credential has nothing cached.
finishpart.ai-gateway quotadistinguishes a 404 on a guessed probe model from an unreadable quota.recordGatewayQuotano longer callsmkdirSyncper request, and prunes expired entries.parseClickzettaQuota's "is this a reading" filter now countsused, matching theformatter's used-only branch.
Verify:field plus thesubstitute grep, wired into the re-baseline procedure.
agent llm test'squotaskey now has behavioral tests pinning its shape, including theno-headers case.
Legacy surfaces dropped
The quota half is a clean break from the Portal-poll behaviour, not a compatibility layer
over it, and two leftovers that pretended otherwise are gone:
readGatewayQuotano longer takesmaxAgeMs. Whether a reading is still worthshowing is not one question but several — a daily allowance stops being true at its reset,
a lifetime one never does — so the caller answers it with
updated_atand the periods inhand, which is what
readHeaderQuotanow does in full. The parameter only ever duplicatedthe coarsest half of that decision, and after the reader took the whole of it, nothing
passed one. Its test now pins the contract that remains: the reading carries the moment it
was taken.
parseClickzettaQuotano longer accepts array-valued headers.HeaderSourceisHeaders | Record<string, string | undefined>— the two shapes response headers actuallyarrive in here (fetch's
Headersat the CLI sites, the AI SDK'sSharedV3Headersat theprovider). The array shape is node's raw-headers form, which nothing passes; the branch's
only test proved the branch existed.
Already gone with the feature itself: the
/clickzetta-portal/user/listApiKeysroute,maskApiKey,matchKeyUsage,QuotaSnapshot'sused/limit/period/aliasfields, andthe walk over every configured Profile hunting the one whose portal knew the selected key.
Fixed after the second review pass
readHeaderQuotadrops adailyreading taken before the counter reset instead ofrelying on a fixed duration, which no window under a day can express. Conservative:
a different calendar date in either local or UTC time counts as reset.
abbreviategained a billions tier, so a lifetime ceiling reads1.0B, not1000.0M.quotaRowsnames the limiter scope when more than one reports, and sorts by it, so atenant-wide cap cannot render as an unlabelled duplicate of the key's own.
recordQuotacannot fail a request that already succeeded — the guard is inside it, soneither call site can forget it.
ai-gateway quotaarmssetGatewayDebug, so--debugis no longer accepted-and-ignored.llm/gateway-error.tsre-exports from three narrow subpaths rather than the root@clickzetta/ai-gatewaybarrel, so the pre-bundled TUI asset never has the AI SDK inreach. Measured: the asset is 73,318 bytes either way and never contained the provider
machinery — bun shook it out — so this is defence in depth, not a size fix.
HelpCasegainedexpectTextso epilogue prose is asserted as prose, not as asubcommand.