feat(tui): quota sidebar with Profile and Quota sections - #76
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const dispose = Effect.fn("InstanceHttpApi.dispose")(function* () { | ||
| // cz_change: drop the cached global config too, so the rebuilt instance | ||
| // re-reads it from disk. | ||
| //======================== cz-cli change ======================== |
There was a problem hiding this comment.
MEDIUM (confidence: medium — I could not diff against pristine v1.17.11 offline, so please confirm)
The banner wraps the cfg.invalidate() call, but the two things that call depends on are outside it:
import { Config } from "@/config/config" // line 3
...
const cfg = yield* Config.Service // line 19rg -n "cfg\." on this file returns exactly one hit — line 39, inside the banner. If cfg has no other consumer, then the binding on line 19 (and possibly the line 3 import) were added by this patch and are part of it, yet they sit outside the marked region and are not mentioned in ledger entry 8's Marker field.
Consequence at re-baseline: step 2's grep finds the banner, someone re-applies the yield* cfg.invalidate() block, and it fails to resolve cfg. That does get caught by step 4 (bun run typecheck), which is why this is MEDIUM rather than HIGH — but the checklist is supposed to be self-contained.
Smaller correct change: either extend the banner to cover line 19 (a second short banner around const cfg = yield* Config.Service), or add the binding + import to entry 8's Marker field so the re-apply is fully described. If line 19 is upstream's own, ignore this.
| `CZ_MCP_ARGS='{"prompt":"hi"}' bun test/mcp-call-repro.ts` must name the real cause, | ||
| not a bare ref. | ||
|
|
||
| ### 7. ClickZetta dynamic model discovery in the provider loader |
There was a problem hiding this comment.
MEDIUM (confidence: high)
The three new INTRUSIVE entries (7, 8, 9) each carry File / Marker / What-why / Why-intrusive / History, but none carries a Verify line. Every pre-existing entry (1–6) has one:
- feat: cz-cli distribution — a2a format, provider, profile gate #1 "run cz, confirm dirs resolve under
.../clickzetta" - re-fix splitSql function, and implement dry-run in sql subcommand #5 "
packages/cz-cli/test/flag-injection-visibility.test.ts… goes RED if this patch is dropped" - support use clause #6 "
packages/cz-cli/test/mcp-error-format.test.tspins the rendering of both shapes"
This file's stated job is to be the re-baseline checklist, and step 2 only tells you the banner is present — Verify is the only field that tells you the patch still works. Without it, entries 7–9 can be confirmed re-applied and still be silently broken.
All three have something concrete to point at:
- Feat/cz cli ts rewrite #7 →
packages/opencode/test/provider/clickzetta-discovery.test.tsandclickzetta-context-limit.test.tsalready exist and cover it. - feat: cz-cli task param system, JDBC datasource binding, execute improvements #8 → rewrite a global config file,
dispose, confirm the rebuilt instance sees the new providers. - USE SCHEMA / USE VCLUSTER should validate existence and return error if not found #9 →
packages/core/src/model-selection.ts's unit tests plus "cz-cli agent llm shownames a concrete model withconfig.modelunset".
Entries 7 and 9 also have no Upstream value line; for a pure addition that is arguably "none", but saying so explicitly would keep the schema uniform.
|
|
||
| --- | ||
|
|
||
| ## HOOK-based customizations (safe — live entirely in the cz layer) |
There was a problem hiding this comment.
MEDIUM (confidence: medium)
This PR moves the quota readout from home_prompt_right / session_prompt_right to sidebar_content, and relies on three upstream facts that nothing in this file records:
sidebar_contentstill exists inpackages/plugin/src/tui.ts'sTuiHostSlotMapand still passessession_id.- It is still an append slot (not
single_winnerlikesidebar_title/sidebar_footernext to it), otherwise the cz sections would displace upstream's Context section. - The
ordervalues it slots between are still 100 (context) / 200 (mcp) / 300 (lsp) / 400 (todo) / 500 (files) —order: 150intui-quota.tsxis meaningless if those change.
The HOOK section below says it exists precisely "so a re-baseline can confirm the hooks they depend on still exist in the new upstream", and the quota indicator has no entry there at all. That gap predates this PR, but this PR is the change that swaps which hook the feature rides on, so it is the natural moment to add HOOK entry #5 with those three re-verify points (plus the sidebar being session-only and auto-opening at ≥120 columns, which is the behavioral contract the feature now depends on).
Not an invariant violation — sidebar_content is public API and no upstream file was touched to use it, which is the right call. This is only about recording the dependency.
|
|
||
| - **File:** `packages/tui/src/context/local.tsx` | ||
| - **Marker:** two `//===== cz-cli change =====` banners — one around the | ||
| `resolveModelSelection` import, one around the `fallbackModel` memo body. |
There was a problem hiding this comment.
LOW (confidence: high on the fact, low on the impact)
- **cz files:** `packages/core/src/model-selection.ts` (new, cz-owned — the four-tierThis entry (and entry 4, for packages/tui/src/component/prompt/sql-command.ts) records a whole cz-owned source file living inside a package the invariant calls pristine. Both files carry only a cz_change: header:
packages/core/src/model-selection.ts:1:// cz_change: the TUI's startup model-selection chain, extracted so a CLI command
packages/tui/src/component/prompt/sql-command.ts:1:// cz_change: helpers for the `/sql` prompt command, which runs SQL through
So rg -n "cz-cli change" packages/core packages/opencode packages/tui — step 2 of the procedure — does not list either of them. The risk is lower than for an in-place edit (upstream has no file at those paths, so a fast-forward cannot overwrite them), but a reviewer running step 2 gets an incomplete picture of what cz owns inside those packages, and a git checkout upstream -- packages/core-style reset would not surface them either.
Cheapest fix: give each file the wrapped banner on its first line instead of cz_change:, so the one grep enumerates every cz-owned thing in those three packages. To be clear — this is not the "don't flag cz_change:" case: these two are in packages/core / packages/tui, not in packages/cz-cli or a test.
| if (isRecord(payload) && isPortalOk(payload.code)) return payload | ||
| // Neither host produced a usable answer: surface the original host's result so | ||
| // the failure reads as the profile's own, not the fallback's. | ||
| return firstPayload ?? payload |
There was a problem hiding this comment.
MEDIUM (confidence: high on the code path, medium on how often it fires)
// Neither host produced a usable answer: surface the original host's result so
// the failure reads as the profile's own, not the fallback's.
return firstPayload ?? payloadWhen the first host threw (network error, non-2xx from portalCall) and the central host answers HTTP 200 with a non-OK business code, firstPayload is undefined, so this returns the central host's unusable payload and firstError is discarded. portalRead then resolves successfully where portalCall used to reject.
Downstream that converts a transport failure into a blank section:
fetchProfileSnapshot—billing.status === "fulfilled"with a non-OKcode, sobillingDataisundefined,cash/oweareundefined, and neither of the twothrowguards on lines 590–591 fires.fetchQuotaSnapshot—loadedgets an entry, soif (loaded.length === 0) throw errors[0]is skipped and it returns{}.quotaRows({})→[]→Sectionrenders nothing.
That is precisely the outcome the surrounding code documents as forbidden: fetchQuotaSnapshot's own doc says "Throws on transport/auth failure so the caller can keep showing the previous value rather than replacing a good reading with an error", and the comment on line 492 warns about "an empty-but-successful snapshot, which … swallows the current profile's real error". The controller's .catch() keeps the last snapshot; a resolved {} overwrites it.
Smaller correct change: only treat the central-host attempt as authoritative when it produced a usable payload — if firstError is set and the fallback did not return an OK code, rethrow firstError:
if (firstPayload !== undefined) return firstPayload
if (firstError) throw firstError
return payloadNo test covers this combination — the two new fallback tests both have the first host answering HTTP 200 with code 8888, which populates firstPayload and so takes the safe branch.
| export function readProfileInfo(): ProfileInfo | undefined { | ||
| const current = Profile.current() | ||
| const profiles = loadProfiles() | ||
| const name = current && profiles[current] ? current : Object.keys(profiles)[0] |
There was a problem hiding this comment.
MEDIUM (confidence: medium-high)
const name = current && profiles[current] ? current : Object.keys(profiles)[0]When no profile is configured, this substitutes whatever profile happens to be first in profiles.toml. Profile.current() already resolves CZ_PROFILE → default_profile, and its doc is explicit about what undefined means:
Returns undefined only when neither is available, which callers should treat as "no profile configured" rather than substituting one of their own.
So in that state the Profile section names a profile that nothing else in the process is using, in a panel whose whole purpose (per the header comment) is answering "am I about to run this against the right lakehouse". The failure is quiet and it points the wrong way — the user reads a tenant/instance/workspace that their next /sql will not hit.
It also disagrees with the Quota half in the same panel: fetchProfileSnapshot is called with includeBilling: name === current, so with current undefined no profile is ever billed and the balance row is absent. Profile says "prod_0", Quota says nothing.
Smaller correct change: if (!current || !profiles[current]) return undefined and let the section not render, matching how every other field here drops its row when unknown. If a fallback really is wanted, getDefaultProfileName() is the meaningful one — but current() already consulted it, so reaching this line means there is genuinely no answer.
No test covers the fallback branch: all three new readProfileInfo tests pin CZ_PROFILE.
| authType: readAuthType(name), | ||
| accountName: str(profile.account_name), | ||
| userName: str(profile.username), | ||
| region: service ? detectEnv(service) : undefined, |
There was a problem hiding this comment.
MEDIUM (confidence: medium)
region: service ? detectEnv(service) : undefined,detectEnv never returns "unknown" — its last line is an unconditional fallback (packages/clickzetta-sdk/src/config/region.ts:11-12):
// For custom/enterprise domains, return "prod" as default (matching Python's fallback)
return "prod"So a private/on-prem deployment, a custom domain, or http://localhost:8080 all produce region: "prod", and profileRows renders the row prod region. In a panel meant to answer "am I pointed at the right lakehouse", an invented prod is worse than a missing row — and the rest of this function is careful to drop unknown fields rather than fill them.
There is also a naming mismatch: the SDK calls this value an env, and the two existing callers name it accordingly (env: detectEnv(config.service) in commands/studio-context.ts:42, centerRegion in commands/profile-bootstrap.ts:229). Relabeling it "region" in the UI means dev-api.* renders as dev region, which reads as a region named "dev".
Two options, either is smaller than the current behavior:
- Keep
detectEnvbut label the rowenvrather thanregion, and suppress the row when the host matched none ofdetectEnv's known patterns (i.e. re-derive the<label>.api.<clickzetta|singdata>.commatch here, the same shapecentralPortalHostabove already matches, and leaveregionundefined otherwise). - Or show the service host itself, which is never fabricated.
The two new tests (prod_0 → cn-shanghai-alicloud, dev_0 → dev) both use hosts detectEnv recognizes, so neither covers the fabricated-prod path.
| * Returns undefined rather than throwing: a missing user name should cost one | ||
| * line of the section, never the section itself. | ||
| */ | ||
| export async function fetchProfileUserName(input: { signal?: AbortSignal } = {}): Promise<string | undefined> { |
There was a problem hiding this comment.
LOW (confidence: high)
This is the second implementation of "POST getCurrentUser, validate the envelope, take data.name". The first is inline in fetchProfileSnapshot (lines 563–581):
const currentUser = await portalRead(baseUrl, CURRENT_USER_PATH, token.token, {
method: "POST", signal: input.signal,
})
if (isRecord(currentUser) && isPortalOk(currentUser.code) &&
isRecord(currentUser.data) && typeof currentUser.data.name === "string") {
userName = currentUser.data.name
}Same route, same method, same four-part envelope check, same field — differing only in which profile's config/token it uses and in that the new one memoizes via userNameCache while the old one does not. Two consequences:
- The
userNameCachethis PR adds is bypassed by the quota path, so an OAuth profile (nousernamein its TOML block) re-POSTsgetCurrentUseron every quota refresh — and the controller refreshes on every busy→idle edge, i.e. once per turn. - The envelope validation now has to be kept in sync in two places; the portal already disagrees with itself about success codes (
isPortalOkexists for that reason), so drift here is plausible.
Smaller correct change: extract one readCurrentUserName(baseUrl, token, signal) that both call, and key the cache on the profile name so fetchProfileSnapshot can reuse it. Worth noting the comment right above the inline copy says listApiKeys "ignores the userName value and scopes to the token identity regardless" — if that holds, the per-refresh POST in the quota path buys nothing at all and could just be dropped, which would remove the duplication outright.
| // render, so a `cz-cli profile use` elsewhere is reflected without extra plumbing. | ||
| // userName arrives separately because OAuth profiles need a portal call for it. | ||
| const profile = createMemo(() => { | ||
| const info = readProfileInfo() | ||
| if (!info) return undefined | ||
| return info.userName ? info : { ...info, userName: props.userName() } | ||
| }) |
There was a problem hiding this comment.
LOW (confidence: high on the mechanics, low on user impact)
// Read live rather than once: the profile is resolved from profiles.toml on every
// render, so a `cz-cli profile use` elsewhere is reflected without extra plumbing.
const profile = createMemo(() => {
const info = readProfileInfo()The comment describes behavior a memo does not have. createMemo recomputes only when a tracked signal it read changes; readProfileInfo() is a plain readFileSync + TOML parse with no reactive source. The only tracked dependency here is props.userName(), and even that is read solely in the !info.userName branch — so for a password/PAT profile (which carries username in its TOML block) the memo has zero dependencies and never recomputes for the life of the mount.
So cz-cli profile use in another shell is not reflected while the sidebar stays mounted; it only refreshes if the sidebar unmounts and remounts (the toggle). Impact today is small, since nothing in-process switches profiles (Profile.set has no caller under src/opencode-plugin), which is why this is LOW.
Either fix the comment to say what it does ("re-read when the section remounts"), or make it actually live — Profile.onChange already exists for exactly this and would drive a signal the memo can track.
| // Read live rather than once: the profile is resolved from profiles.toml on every | ||
| // render, so a `cz-cli profile use` elsewhere is reflected without extra plumbing. | ||
| // userName arrives separately because OAuth profiles need a portal call for it. | ||
| const profile = createMemo(() => { | ||
| const info = readProfileInfo() | ||
| if (!info) return undefined | ||
| return info.userName ? info : { ...info, userName: props.userName() } | ||
| }) |
There was a problem hiding this comment.
MEDIUM (confidence: high on the mechanism) — this comment describes behavior the code does not have.
// Read live rather than once: the profile is resolved from profiles.toml on every
// render, so a `cz-cli profile use` elsewhere is reflected without extra plumbing.
const profile = createMemo(() => {
const info = readProfileInfo()createMemo recomputes only when a signal it read changes. This body reads no signal except props.userName(), and even that only on the !info.userName branch — readProfileInfo() is a plain readFileSync + TOML parse with no reactive dependency. So for a profile whose TOML block carries username, the memo runs exactly once per slot mount and never again; for one that doesn't, it re-runs only when the userName signal lands.
Concretely, the two ways the active profile can change mid-session are both missed:
cz-cli profile use Brun in another shell (or by the agent via bash) rewritesdefault_profile, whichProfile.current()→getDefaultProfileName()reads from disk on each call — but nothing re-invokes the memo, so the panel keeps showing A.Profile.set()in-process firesProfile.onChangelisteners; nothing here subscribes, andfetchProfileUserNameis never called again either, so a staleuserNamefrom the old profile can be shown against the new one.
profile-context.ts makes the case for why this matters more than a cosmetic staleness: "an agent silently pointed at the wrong lakehouse can run writes there" — a Profile panel that reports the previous tenant is worse than no panel.
Two ways out, either is fine: subscribe to Profile.onChange (already exported from ../connection/profile-context.js) and drive a signal that both this memo and a fetchProfileUserName re-resolve depend on; or keep the once-per-mount read and correct the comment. Note the literal reading of the comment — a sync file read plus TOML parse on every frame — is not what you want, so the memo is the right structure; it's the claim that needs to change if you don't wire the subscription.
| // Resolved once, unawaited: identity is fixed for the session, and the rest of | ||
| // the Profile section is already on screen from profiles.toml without it. | ||
| const [userName, setUserName] = createSignal<string | undefined>(undefined) | ||
| void fetchProfileUserName({ signal: api.lifecycle.signal }) | ||
| .then((name) => { | ||
| if (name) setUserName(name) | ||
| }) | ||
| .catch(() => { | ||
| // A missing user name costs one row, never the section. | ||
| }) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — this puts an authenticated portal round-trip on TUI startup for users who are not on a ClickZetta provider at all.
void fetchProfileUserName({ signal: api.lifecycle.signal })installQuotaIndicator runs for every TUI start (the brand plugin always calls it). fetchProfileUserName → getCookieToken(config) ?? getToken(config) → for a PAT/password profile that's a loginSingle exchange, then a POST getCurrentUser, then possibly a second attempt against the central host via the new portalRead fallback. None of it is gated on the provider.
Before this PR the only network path was controller.refresh() → fetchQuotaSnapshot, which exits at classifyClickzettaEntry(...).kind === "foreign" before touching a token. So a user on anthropic/openai with a ClickZetta profile configured went from 0 portal requests at startup to 2–4.
Also, the docstring on fetchProfileUserName says "Only OAuth profiles need this — a password profile's TOML block already carries username". PAT profiles generally don't: the fixture this PR adds ([profiles.prod_0] with pat, no username) is exactly that shape, and readProfileInfo() returns userName: undefined for it, so the network path fires. The comment understates how often this runs.
If the Profile section is meant to paint for every provider (which the PR description implies), that's a defensible product call — but consider deferring the fetch until the sidebar slot actually mounts, so a session that never opens the sidebar spends nothing. getToken is coalesced (pendingFetches in clickzetta-sdk/src/auth/token.ts), so there's no double-login race with the concurrent controller.refresh() — this is purely about doing the work at all.
| // order 150 puts Quota immediately after upstream's Context section (order 100) | ||
| // and ahead of MCP/LSP/Todo/Files (200/300/400/500) — the two usage readouts read | ||
| // as one group, which is the point of moving here. | ||
| api.slots.register({ | ||
| order: 100, | ||
| order: 150, | ||
| slots: { | ||
| home_prompt_right() { | ||
| return <View api={api} activeModel={activeModel} snapshot={snapshot} onContext={onContext} /> | ||
| }, | ||
| session_prompt_right() { | ||
| return <View api={api} activeModel={activeModel} snapshot={snapshot} onContext={onContext} /> | ||
| sidebar_content(_ctx, props) { | ||
| return ( | ||
| <View | ||
| api={api} | ||
| activeModel={activeModel} | ||
| sessionID={props.session_id} | ||
| snapshot={snapshot} | ||
| userName={userName} | ||
| onContext={onContext} | ||
| /> | ||
| ) | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Behavior change — please confirm the intent (severity MEDIUM if unintended, confidence: high that it happens)
api.slots.register({
order: 150,
slots: {
sidebar_content(_ctx, props) {Dropping home_prompt_right / session_prompt_right for sidebar_content changes where — and whether — this readout is reachable:
- The home screen loses it entirely.
sidebar_contentis only rendered frompackages/tui/src/routes/session/sidebar.tsx:85. There is no home-route consumer, so a user who launches cz and looks at the home screen no longer sees a balance anywhere. - In a session it is conditional on sidebar visibility. The file comment already notes the ≥120-column auto-open; below that the balance/quota is behind a keybinding the user has to know about. The previous slot was unconditional.
- The refresh trigger moved with it. The
createEffectthat callsonContext→controller.refresh()now lives in a component that only mounts when the sidebar is open, so with the sidebar closed the only read is the single unawaitedcontroller.refresh()at install. That's consistent (nothing is displayed), but it does mean the snapshot can be arbitrarily old at the moment the sidebar is first opened, until the effect fires.
The PR description reads as though (1) and (2) are the intended trade for legibility at 80 columns, and the reasoning in the header comment is sound. Confirming explicitly because "balance disappeared from the home screen" is the kind of thing that reads as a regression to a user who never opens the sidebar. No test covers the slot wiring (name, order: 150, props.session_id); I verified it against packages/plugin/src/tui.ts:480 and the upstream section orders (context 100, mcp 200, lsp 300, todo 400, files 500) by reading, not by running anything.
| export function centralPortalHost(baseUrl: string): string | undefined { | ||
| const stripped = baseUrl.replace(/^(https?:\/\/)[a-z0-9-]+\.(api\.)/i, "$1$2") | ||
| return stripped === baseUrl ? undefined : stripped | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: medium-high) — the fallback sends the profile's portal token to a host that appears nowhere in the user's configuration.
export function centralPortalHost(baseUrl: string): string | undefined {
const stripped = baseUrl.replace(/^(https?:\/\/)[a-z0-9-]+\.(api\.)/i, "$1$2")The rewrite matches any <label>.api.<anything>, not just the two roots the doc comment measured. service in profiles.toml is user-supplied and may name a private or enterprise deployment, so a profile pointed at cn-east.api.acme-internal.example produces a fallback target of api.acme-internal.example — a hostname the tenant never configured and may not control. portalRead then sends x-clickzetta-token there on any unusable first answer, including a plain transport failure or an auth error, where retrying a different host cannot help anyway.
detectEnv in packages/clickzetta-sdk/src/config/region.ts:7-10 already encodes the list of roots where a leading label really is a region segment (clickzetta.com, singdata.com). Reusing that boundary would keep every case the comment measured working while making the fallback impossible to point at an unrelated host:
const stripped = baseUrl.replace(/^(https?:\/\/)[a-z0-9-]+\.(api\.(clickzetta|singdata)\.com)$/i, "$1$2")Narrowing the trigger would help too: the measured failure is the portal's business code 8888, so falling back only on a non-OK payload (not on a thrown transport/auth error) would keep the extra request off paths where it can't succeed.
Not a hypothetical based on the code alone: I could not verify what api.<private-root> resolves to in any real deployment, which is the point — neither can this function.
| authType: readAuthType(name), | ||
| accountName: str(profile.account_name), | ||
| userName: str(profile.username), | ||
| region: service ? detectEnv(service) : undefined, |
There was a problem hiding this comment.
MEDIUM (confidence: high) — this row can confidently print prod region for a deployment that is not prod.
region: service ? detectEnv(service) : undefined,detectEnv (packages/clickzetta-sdk/src/config/region.ts:11-12) ends with:
// For custom/enterprise domains, return "prod" as default (matching Python's fallback)
return "prod"So any host that isn't dev-api./sit-api./uat-api., isn't exactly api.clickzetta.com/api.singdata.com, and doesn't match <label>.api.(clickzetta|singdata).com yields "prod" — including a private deployment, a staging host under a customer domain, and http://localhost:8080. The sidebar then asserts prod region in text tone for a session that may be pointed at neither prod nor a region.
That default is defensible inside the SDK, where it only picks a service URL shape. It is not defensible as a displayed fact in a panel whose stated job (see the ProfileInfo docstring) is answering "am I pointed at the right lakehouse" — a wrong-but-confident prod is the one answer that costs the user something.
Suggestion: only show the row when the host actually matches a known pattern, and otherwise fall back to displaying the service host itself, which is always true. Either a local regionLabel(service) that returns undefined where detectEnv would guess, or region: knownRegion(service) ?? service.
The added test covers cn-shanghai-alicloud.api.clickzetta.com → the region segment and dev-api.clickzetta.com → dev; nothing covers the custom-domain fallback, which is where the misreport lives.
| const current = Profile.current() | ||
| const profiles = loadProfiles() | ||
| const name = current && profiles[current] ? current : Object.keys(profiles)[0] | ||
| if (!name) return undefined |
There was a problem hiding this comment.
MEDIUM (confidence: medium) — the fallback substitutes another tenant's identity instead of reporting that it doesn't know.
const current = Profile.current()
const profiles = loadProfiles()
const name = current && profiles[current] ? current : Object.keys(profiles)[0]
if (!name) return undefinedWhen Profile.current() names a profile that isn't in profiles.toml, this silently reports Object.keys(profiles)[0] — the first TOML block, which for the fixture in this PR would be a different account, region, instance and workspace. Reachable via a typo'd -p, a CZ_PROFILE inherited from a parent process, or a profile deleted while a session is open.
Profile.current()'s own docstring draws the opposite conclusion for exactly this case: "Returns undefined only when neither is available, which callers should treat as 'no profile configured' rather than substituting one of their own."
The existing precedent in fetchQuotaSnapshot uses the same shape (current && profiles[current] ? [current, ...] : Object.keys(profiles)), but there it's an ordering for a credential hunt where a wrong guess just fails to find a key. Here the value is rendered as a claim about the session.
Rendering nothing is the honest answer, and profileRows(undefined) already handles it: return undefined when current is set but absent from the file, keeping the Object.keys(profiles)[0] path only for the genuinely-unpinned case (current === undefined).
| //======================== cz-cli change ======================== | ||
| // Gate for the `detail` field below — see the banner in the handler. | ||
| import { Flag } from "@opencode-ai/core/flag/flag" | ||
| //====================== end cz-cli change ====================== |
There was a problem hiding this comment.
LOW (confidence: high) — the banner conversion here is right, but it makes the existing ledger entry for this file stale.
//======================== cz-cli change ========================
// Gate for the `detail` field below — see the banner in the handler.
import { Flag } from "@opencode-ai/core/flag/flag"
//====================== end cz-cli change ======================UPSTREAM-PATCHES.md entry 6 still says:
Marker: one
//===== cz-cli change =====banner around thedetailblock.
The file now carries two (this import, plus the one at lines 35-64). Since the ledger's step-2 grep is what a re-baseline reconciles against the entry text, a count that no longer matches is the same class of drift the entries added in this PR are fixing — someone re-applying entry 6 from the ledger alone restores the detail block and drops the import, leaving the file non-compiling.
Same nit applies to the entries this PR adds: they describe the marker as //===== cz-cli change =====, while every banner in the tree is the long //======================== cz-cli change ======================== form. Worth making entry 6 read "two banners — one around the Flag import, one around the detail block" while the file is open.
| @@ -1,351 +0,0 @@ | |||
| name: pr-standards | |||
There was a problem hiding this comment.
MEDIUM (confidence: high) — unrelated scope, and it leaves a paired workflow half-wired.
The third commit (ci: remove upstream community PR governance workflows) deletes 446 lines of CI that have nothing to do with the quota sidebar, and the PR description doesn't mention it. Two separate points:
1. Scope. A feat(tui) PR that also removes the repo's PR-governance automation is two reviews in one — a reviewer signing off on the sidebar is implicitly signing off on dropping title/template/issue-linking enforcement for outside contributors. This is the "unrelated drive-by edits mixed into the same PR" case; it wants its own PR so it can be reverted independently.
2. A concrete leftover. pr-standards.yml's check-compliance job was the only thing that applied needs:compliance to a pull request:
if (!hasComplianceLabel) {
await addLabel('needs:compliance');
}.github/workflows/compliance-close.yml is still present and still queries that label with labels: 'needs:compliance', closing both issues and PRs after two hours. The only remaining producer is duplicate-issues.yml, which labels issues only (gh issue edit ... --add-label needs:compliance). So the PR half of compliance-close.yml becomes unreachable — dead automation that will keep running daily. If the intent is to retire this whole mechanism for PRs, that workflow should be trimmed in the same change.
For what it's worth the deletion looks defensible on the merits: both files fetch .github/TEAM_MEMBERS at ref: 'dev', and this fork's default branch is main (claude-review-context.md calls out that the inherited AGENTS.md line about dev is wrong here), so getContent would throw on every PR — plus pr-management.yml installs upstream opencode from opencode.ai/install and comments as opencode-agent[bot]. That's a good rationale; it just belongs in its own PR with that rationale in the description.
|
Review summary A. Upstream invasiveness — no issues found The four files touched under
One ledger-accuracy nit introduced here: entry 6 now undercounts the banners in The feature itself stays in the cz layer, on the public B. Clean fix, or a hole drilled around the problem Mostly the clean fix. Two findings:
C. Regression risk
I cannot run tests, so nothing here is a claim that anything passes. |
- centralPortalHost/portalRead: pin the fallback root to clickzetta.com/ singdata.com (was matching any <label>.api.<anything>, which could point a profile's portal token at an unrelated custom domain), and only treat the fallback host's answer as authoritative when the profile's own host actually answered — a thrown transport/auth error now propagates instead of being swallowed into a resolved-but-empty snapshot that overwrites the last good one. - readProfileInfo: stop substituting a different tenant's profile when CZ_PROFILE names one absent from profiles.toml — render nothing instead, matching Profile.current()'s own contract. - Replace the region label (detectEnv, which ends in an unconditional "prod" for any unrecognized host) with a local knownEnv that renders nothing rather than a fabricated "prod" for a private/custom domain. - Extract readCurrentUserName so fetchProfileUserName and fetchProfileSnapshot share one getCurrentUser envelope check instead of two copies to keep in sync. - Correct the profile memo's comment: it is resolved once per mount, not live. - Ledger: entry 6 now names both banners in error.ts; entries 7-9 get Verify lines; new HOOK entry 5 records the sidebar_content dependency (append slot, order values, session-only/ ≥120-col visibility) this PR introduced. - compliance-close.yml: narrow to issues only now that pr-standards.yml (the only thing that ever labelled a PR needs:compliance) is gone, instead of leaving a dead PR-closing branch that can never fire.
| # Issues only: the PR half of this mechanism (pr-standards.yml's check-compliance | ||
| # job, the only thing that ever applied needs:compliance to a pull request) was | ||
| # removed as part of dropping the upstream community-PR-governance workflows this | ||
| # fork doesn't use (see .github/workflows/pr-standards.yml's removal). The only | ||
| # remaining producer of this label, duplicate-issues.yml, labels issues only. |
There was a problem hiding this comment.
MEDIUM (confidence: high) — unrelated drive-by: 446 lines of CI governance removed in a TUI feature PR.
# Issues only: the PR half of this mechanism (pr-standards.yml's check-compliance
# job, the only thing that ever applied needs:compliance to a pull request) was
# removed as part of dropping the upstream community-PR-governance workflows this
# fork doesn't use (see .github/workflows/pr-standards.yml's removal).The PR is titled feat(tui): quota sidebar with Profile and Quota sections and its body describes only the sidebar move. Alongside that it deletes .github/workflows/pr-management.yml (95 lines) and .github/workflows/pr-standards.yml (351 lines) and rewrites this workflow. Nothing in the quota work depends on any of it.
The rationale is plausible — pr-standards.yml reads .github/TEAM_MEMBERS with ref: 'dev', and per .github/claude-review-context.md this fork's default branch is main, not dev, so that getContent call would 404 and the job would error on every community PR. But that makes it a separate, independently reviewable fix, not a rider. Two concrete consequences a reviewer looking at a sidebar diff will not weigh:
pr-standards.ymlwas the only automated enforcement of the conventional-commit PR title format that.github/claude-review-context.mddocuments as a repo convention (titlePattern = /^(feat|fix|docs|chore|refactor|test)\s*(\([a-zA-Z0-9-]+\))?\s*:/). I grepped the remaining 27 workflows — nothing else checks PR titles. That enforcement is now gone for good.- It also carried the
needs:description/needs:compliancelabelling for community PRs, which is why the edit here is needed at all.
Suggestion: split the three .github/workflows/ changes into their own PR so the removal of contributor-governance automation gets reviewed on its own terms.
Note the change itself reads correctly to me: kind/isPR are removed everywhere they were used, and the surviving producer of needs:compliance (duplicate-issues.yml) does only label issues, as the comment claims.
| @@ -12,10 +12,15 @@ permissions: | |||
| pull-requests: write | |||
There was a problem hiding this comment.
LOW (confidence: high) — pull-requests: write is now unused.
permissions:
contents: read
issues: write
pull-requests: writeAfter this change the script filters to !item.pull_request and the only mutating calls left are issues.createComment, issues.removeLabel and issues.update — all covered by issues: write. The github.rest.pulls.update call that needed pull-requests: write was removed in this same diff. Worth dropping the grant so the workflow's token stays minimal.
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| run: | | ||
| COMMENT=$(bun script/duplicate-pr.ts -f pr_info.txt "Check the attached file for PR details and search for duplicates") |
There was a problem hiding this comment.
LOW (confidence: high) — deleting this job orphans script/duplicate-pr.ts.
COMMENT=$(bun script/duplicate-pr.ts -f pr_info.txt "Check the attached file for PR details and search for duplicates")This was the only invocation of that script. I grepped the whole repo for duplicate-pr and the only remaining hit is the file's own usage string, so script/duplicate-pr.ts becomes dead code the PR leaves behind. Remove it along with the workflow.
.github/TEAM_MEMBERS, the other file this job read, is still consumed by script/raw-changelog.ts — that one should stay.
| // The sidebar is session-only and auto-opens above 120 columns (sidebarVisible in | ||
| // packages/tui/src/routes/session/index.tsx); narrower terminals reach it with the | ||
| // toggle. That is upstream's layout policy and deliberately not fought here. |
There was a problem hiding this comment.
MEDIUM (confidence: high) — the visibility trade is bigger than this comment and the ledger describe. Please confirm all four losses are intended.
// The sidebar is session-only and auto-opens above 120 columns (sidebarVisible in
// packages/tui/src/routes/session/index.tsx); narrower terminals reach it with the
// toggle. That is upstream's layout policy and deliberately not fought here.sidebarVisible (packages/tui/src/routes/session/index.tsx:264-269) is:
const wide = createMemo(() => dimensions().width > 120)
const sidebarVisible = createMemo(() => {
if (session()?.parentID) return false
if (sidebarOpen()) return true
if (sidebar() === "auto" && wide()) return true
return false
})Four behavioral changes fall out of moving off home_prompt_right / session_prompt_right:
- No readout on the home screen at all.
sidebar_contentis rendered only frompackages/tui/src/routes/session/sidebar.tsx:85;home_prompt_right(packages/tui/src/routes/home.tsx:83) is no longer registered. A user who launches cz and reads their balance before starting a session no longer can. - Child sessions never show it.
if (session()?.parentID) return falseis unconditional — no width and no toggle reaches it. Sub-agent sessions lose the readout permanently. Neither this comment nor ledger HOOK re-fix splitSql function, and implement dry-run in sql subcommand #5 mentions this. sidebar: "hide"users lose it entirely until they re-toggle, at any width. Thesidebar() === "auto"guard means a persisted hide preference beats width.- The threshold is
width > 120, i.e. 121+ columns. Both this comment ("above 120") and the ledger ("auto-opens only at ≥120 columns") read as 120-inclusive.
The 80-column squeeze-out that motivated the move is real, and I'm not arguing the sidebar is the wrong home. But this trades "visible but cramped at 80 columns" for "invisible on the home route, invisible in child sessions, invisible at ≤120 columns unless toggled" — which is a strictly narrower set of situations than before for the ~80-120 column range. Worth stating explicitly in the ledger entry, since that's the document a future re-baseline reads.
No test covers the renderer or the slot registration — tui-quota-format.test.ts covers quotaRows/profileRows and tui-quota-data.test.ts covers the fetch layer, but nothing asserts which slot is registered or at what order, so a regression here is silent.
| props.onContext( | ||
| `${props.api.route.current.name}:${sessionID}:${props.activeModel.providerID(sessionID || undefined) ?? ""}`, | ||
| ) | ||
| props.onContext(`${props.sessionID}:${props.activeModel.providerID(props.sessionID) ?? ""}`) |
There was a problem hiding this comment.
LOW (confidence: medium) — dropping route.current.name from the key removes the refresh-on-return, and the comment below is now stale.
props.onContext(`${props.sessionID}:${props.activeModel.providerID(props.sessionID) ?? ""}`)Previously the key was ${route.name}:${sessionID}:${providerID}. context is a closure variable in installQuotaIndicator that survives every mount:
let context = ""
const onContext = (key: string) => {
if (key === context) return
context = key
controller.refresh()
}So: in session A the key becomes A:clickzetta_0. Navigate home (the sidebar section unmounts), come back to A — the effect re-runs, produces the identical key, and onContext returns early. No refresh. With the route name in the key, home↔session transitions previously produced distinct keys and did refresh.
The busy→idle edge in createQuotaController.observeStatus still refreshes after each turn, so a snapshot won't go stale during active use. It's the "left the session idle for an hour, came back" case that no longer re-reads. That may well be the behavior you want (fewer portal round-trips), but it isn't stated anywhere as a decision.
Separately, the comment above onContext still says:
// Provider sync and route changes both alter the provider the prompt displays.Neither half holds now — the route is out of the key, and this isn't in the prompt any more.
| void fetchProfileUserName({ signal: api.lifecycle.signal }) | ||
| .then((name) => { | ||
| if (name) setUserName(name) | ||
| }) | ||
| .catch(() => { | ||
| // A missing user name costs one row, never the section. | ||
| }) |
There was a problem hiding this comment.
MEDIUM (confidence: medium) — new unconditional token acquisition + portal POST at every TUI launch, regardless of provider. Please confirm this is intended.
void fetchProfileUserName({ signal: api.lifecycle.signal })
.then((name) => {
if (name) setUserName(name)
})
.catch(() => {
// A missing user name costs one row, never the section.
})installQuotaIndicator runs once at plugin load, so this fires on every TUI start. fetchProfileUserName short-circuits when profiles.toml already carries username — but for an OAuth or cookie profile it does not, and then it reaches:
const token = (await getCookieToken(config)) ?? (await getToken(config))
const name = await readCurrentUserName(toServiceUrl(config.service, config.protocol), token.token, input.signal)That's a credential acquisition plus a POST /clickzetta-portal/user/getCurrentUser on a code path that previously did neither for a large class of users: the existing controller.refresh() → fetchQuotaSnapshot route exits at classifyClickzettaEntry(...).kind === "foreign" before touching loadProfiles() or any token, so someone running cz against anthropic/openai with a ClickZetta profile configured used to make zero portal calls at startup. Now they make one, and getToken may refresh/persist an OAuth token as a side effect.
I think this is defensible — the Profile section is about connection identity, not the LLM, so it should render for any configured profile. But two things to check:
- Does
getTokenever block or prompt (browser launch) when the token cache is cold?installQuotaIndicatordoesn't await this, so it won't delay first paint, but an interactive login triggered from a background promise during TUI startup would be bad.fetchQuotaSnapshotcalls the same pair, so if this is safe there it's safe here — worth confirming rather than inferring. fetchProfileUserNamehas no test.tui-quota-data.test.tsadds coverage forreadProfileInfoandcentralPortalHostbut nothing exercises this function, including theuserNameCachehit path or thegetCookieToken ?? getTokenfallback. Given the review context's note that "profile-based auth (PAT, password, OAuth, cookie) is deliberately independent per method," this new fourth call site into that machinery is worth pinning down.
| // from the file (stale CZ_PROFILE, deleted profile) must render nothing rather | ||
| // than silently swap in a different tenant's identity — this panel's whole job | ||
| // is telling the user which lakehouse they're pointed at. | ||
| const name = current === undefined ? Object.keys(profiles)[0] : profiles[current] ? current : undefined |
There was a problem hiding this comment.
LOW (confidence: medium) — when nothing is pinned, the Profile section names a profile whose balance the Quota section can never show.
const name = current === undefined ? Object.keys(profiles)[0] : profiles[current] ? current : undefinedProfile.current() already falls back to getDefaultProfileName(), so current === undefined means neither CZ_PROFILE nor default_profile is set — reachable with a hand-edited profiles.toml. Substituting Object.keys(profiles)[0] here matches readProfileEntry's own final fallback (profile-store.ts:151), so the identity rows are right.
But fetchQuotaSnapshot does not make the same substitution:
const ordered =
current && profiles[current]
? [current, ...Object.keys(profiles).filter((name) => name !== current)]
: Object.keys(profiles)
...
includeBilling: name === current,
...
...(loaded.find((item) => item.name === current)?.billing ?? {}),With current === undefined, includeBilling is name === undefined → false for every profile, so no billing read is ever issued, and loaded.find((item) => item.name === current) can never match. Result: the Profile section confidently reads prod_0 · pat / acme account / … while the Quota section shows a token figure with no balance line, or no section at all — and there is no signal to the user that the balance was skipped rather than zero.
The narrower correct change is probably to have fetchQuotaSnapshot resolve its billing profile through the same expression (or a shared helper) rather than raw Profile.current(), so the two halves of the sidebar always agree on which profile they are describing. None of the new tests cover this state — every readProfileInfo test sets CZ_PROFILE explicitly.
| return { | ||
| profile: name, | ||
| authType: readAuthType(name), |
There was a problem hiding this comment.
LOW (confidence: high) — readProfileInfo reads and TOML-parses profiles.toml twice per call, in the render path.
return {
profile: name,
authType: readAuthType(name),loadProfiles() at the top of this function does readFileSync + parseTOML with no caching (profile-store.ts:77-89), and readAuthType(name) → readProfileEntry(name) → loadProfiles() again (profile-store.ts:145-152, 265-268). So two synchronous file reads and two TOML parses per invocation.
The call site is inside a Solid memo evaluated during render:
const profile = createMemo(() => {
const info = readProfileInfo()
...
})In practice this is once per sidebar mount (plus once more when props.userName() lands for OAuth profiles), so the cost is small and it's blocking a TUI frame rather than a hot loop. Still, profiles is already in hand at this point — explicitAuthType(profile) ?? deriveAuthType(profile) from profile-store.js takes the entry directly and would give the identical result with no second read. Cheap to fix, and it keeps synchronous I/O out of render.
| export function centralPortalHost(baseUrl: string): string | undefined { | ||
| const stripped = baseUrl.replace(/^(https?:\/\/)[a-z0-9-]+\.(api\.(?:clickzetta|singdata)\.com)(?=\/|$)/i, "$1$2") |
There was a problem hiding this comment.
LOW (confidence: high on the behaviour, which the docstring and the ledger both describe; the finding is about the missing escape hatch) — this sends the profile's portal bearer token to a host the user never named.
export function centralPortalHost(baseUrl: string): string | undefined {
const stripped = baseUrl.replace(/^(https?:\/\/)[a-z0-9-]+\.(api\.(?:clickzetta|singdata)\.com)(?=\/|$)/i, "$1$2")The narrowing is well done — pinned to the two measured roots so a private/enterprise service domain is never rewritten, anchored so uat-api./dev-api. are left alone, and I confirmed it cannot rewrite api.clickzetta.com to itself (no self-fallback loop). And UPSTREAM-PATCHES.md's new hook entry 5 records it as intentional.
What is still worth a second look is that the ledger entry itself notes "No config escape hatch exists to opt out of the fallback." A tenant with an internal policy about which endpoints may receive their credentials has no way to decline, and the trigger is a server-side condition (the region host answering an unusable business code) rather than anything the user did. An opt-out — a profiles.toml field or a CZ_* flag, defaulting to on so nothing changes for anyone else — would make this a documented choice rather than an unconditional one, and would cost very little here.
Two smaller notes on the same block, both LOW:
- The strike machinery (
unservedHostStrikes, per-route key, threshold 2, no expiry) is a fair amount of stateful heuristic for a status panel, and its root cause is server-side:ap-shanghai-tencentcloud.api.clickzetta.comanswering8888for routes it does not serve. If that is a portal bug, worth filing there too so this can eventually be deleted rather than carried. - Because the threshold is checked before the increment, the third call is the first to skip the direct host — i.e. two wasted round-trips per route, three routes, so up to six on such a profile. Matches the docstring; just noting it is 2 wasted attempts and not 1.
| @@ -0,0 +1,77 @@ | |||
| name: cz-test | |||
There was a problem hiding this comment.
MEDIUM (confidence: high; this is a scope observation, not a defect) — this PR carries four independent changes, and the CI-governance one is a drive-by relative to the stated subject.
The title and description are about moving the quota readout into the sidebar. Alongside that:
- the quota/Profile sidebar feature (the actual subject),
- deleting
pr-standards.yml(351 lines) andpr-management.yml(95 lines), narrowingcompliance-close.yml, and adding this workflow, - the
connection/env.tscredential-provenance refactor — a security-relevant rewrite of how credentials are selected, which is where the one HIGH finding on this PR lives, - converting four upstream
cz_change:comments to banners plus three new ledger entries.
Each is defensible on its own. (2) in particular checks out: I verified duplicate-issues.yml is the only remaining producer of needs:compliance and it labels issues only, so narrowing compliance-close.yml to issues is consistent, and the dev → main link fix in the close message is correct for this fork.
The cost is review attention. (3) is the part that most needs careful reading, and it is currently sharing a diff with 446 lines of deleted workflow YAML. Splitting (2) and (4) out as their own PRs — they touch nothing (1) or (3) touch — would let the credential refactor be reviewed on its own terms. Not a blocker if you would rather land it as-is; flagging it because the provenance refactor is the kind of change that benefits most from an isolated diff.
One thing genuinely worth keeping in this PR: the ledger-enforcement step at the bottom of this file. Pointing CI at packages/core/test/model-selection.test.ts and the two clickzetta-* provider tests is what turns UPSTREAM-PATCHES.md's "Verify" lines for entries 7 and 9 into an actual green check. I confirmed all three files exist at those paths.
| "build": "bun run script/build.ts", | ||
| "typecheck": "tsgo --noEmit", | ||
| "test": "bun test --timeout 30000", | ||
| "test": "bun test --isolate --timeout 30000", |
There was a problem hiding this comment.
LOW (confidence: medium) — --isolate is a global change to the local and CI test contract, added on top of a fix that already addresses the actual cause.
"test": "bun test --isolate --timeout 30000",The root cause was one suite: analytics-agent-session-commands.test.ts registering mock.module for three of its own src modules and never restoring them. This PR fixes that directly, with an afterAll restore. That is the clean fix, and it stands on its own.
--isolate then changes how all ~4148 tests run for everyone. cz-test.yml's comment argues for keeping both, and the belt-and-suspenders reasoning is legitimate — but it also concedes the two real costs: a fresh global per file is slower, and it "can also hide a REAL cross-file coupling that would still bite in the shipped binary, where every test file's module cache is shared." That second cost is the one worth weighing: the shipped binary has one shared module registry, so a suite that only passes under --isolate is a suite whose coupling you will now not find until runtime.
Worth confirming the tradeoff was chosen rather than inherited from debugging this one leak. If the goal is "no suite may leak a mock.module," a lint or a small meta-test that asserts every file registering mock.module also restores it would catch the class without changing the execution model. If the goal is genuinely "isolate everything," that is a reasonable call — just note it is the primary guarantee now, and the per-suite afterAll becomes the redundant one rather than the other way round.
Also worth noting for the summary: --isolate gives each file a fresh global object but process.env remains the single OS-process environment, so it does not protect against env-var leakage between files — see the separate CZ_ENV_DERIVED finding on connection/env.ts.
| - **Upstream value:** the four-tier chain is inlined directly in `local.tsx`'s | ||
| `fallbackModel` memo; there is no import of `@opencode-ai/core/model-selection` | ||
| and no shared implementation for a second caller to reuse. | ||
| - **cz files:** `packages/core/src/model-selection.ts` (new, cz-owned — the four-tier |
There was a problem hiding this comment.
LOW (confidence: high) — entry 9 is complete and correct; this is a gap in the grep, not in the entry.
- **cz files:** `packages/core/src/model-selection.ts` (new, cz-owned — the four-tier
packages/core/src/model-selection.ts is a cz-owned file living inside an upstream package, and it carries no cz-cli change banner. Step 2 of the re-baseline procedure (rg -n "cz-cli change" packages/core packages/opencode packages/tui) will surface the two banners in packages/tui/src/context/local.tsx — the import and the fallbackModel memo — but not the file they point at. Someone working the checklist sees "there is an import of @opencode-ai/core/model-selection" and has to read this ledger entry to learn that the target module is also ours.
A new file is not at risk of being silently overwritten by a fast-forward, which is why this is LOW rather than the HIGH that a missing banner on a modified upstream file would be. But a one-line //======================== cz-cli change ======================== at the top of model-selection.ts (and the same for packages/core/test/model-selection.test.ts, which currently carries only a cz_change: comment) would make the grep self-describing: every cz-owned thing under packages/core shows up in one command, with no cross-reference needed.
Same consideration applies to packages/tui/src/component/prompt/sql-command.ts named in entry 4 and, if it is also cz-owned, whatever entry 7's helpers depend on — worth doing as one sweep rather than per-entry.
For the record, everything I could check about the ledger additions holds: entries 7, 8 and 9 each carry File / Marker / Upstream value / What-why / Why-intrusive / History / Verify; all four upstream edits in this diff are banner-only conversions with no behaviour change; and hook entry 5's claims about the slot contract check out — sidebar_content exists in packages/plugin/src/tui.ts:480 and passes session_id, it is rendered only from packages/tui/src/routes/session/sidebar.tsx:85, and the order values are exactly 100 (context) / 200 (mcp) / 300 (lsp) / 400 (todo) / 500 (files), so order: 150 lands where the entry says it does.
| const profileName = argv.profile ?? Profile.current() | ||
| if (!profileName) { | ||
| error("PROFILE_NOT_FOUND", "No profile is active. Pass -p <profile> or set default_profile in profiles.toml.", { format }) | ||
| return | ||
| } |
There was a problem hiding this comment.
LOW — behavioural change worth an explicit confirmation, raised on its own so it is not buried in the summary.
const profileName = argv.profile ?? Profile.current()
if (!profileName) {
error("PROFILE_NOT_FOUND", "No profile is active. Pass -p <profile> or set default_profile in profiles.toml.", { format })
return
}workspace use <ws> --persist with profiles present but neither CZ_PROFILE nor default_profile set previously fell through to Object.keys(profiles)[0] and succeeded, writing into the first profile in the file. It now fails with PROFILE_NOT_FOUND.
The reasoning in the comment is sound — silently mutating a profile the user never selected is worse than an error, especially on a write path — and test/workspace-use-profile.test.ts covers all three cases (CZ_PROFILE wins, default_profile fallback, and this new error). So I am not calling it a bug.
What I want confirmed is that the user-visible break is intended: a script that relied on the implicit first-profile fallback now gets an error payload instead of a success one. Worth a line in the changelog / release notes if this repo keeps them, since the fix for an affected caller is to add -p <profile> or set default_profile.
Also worth confirming: argv.profile ?? Profile.current() means an explicit -p nonexistent now reports Profile 'nonexistent' not found rather than quietly persisting into the default — which reads like the right behaviour, just noting it changed at the same time.
|
Review summary Nine inline findings, one HIGH. Verdicts on the three requested axes below; everything substantive is anchored inline rather than restated here. A. Upstream invasiveness — no issues found All four edits under
Entries 7, 8 and 9 each carry File / Marker / Upstream value / What-why / Why-intrusive / History / Verify. This is the ledger doing exactly the job it was written for — three patches that were previously invisible to The feature itself is hook-based, not intrusive. I checked hook entry 5 against upstream rather than taking it on trust, and its claims hold: One LOW note on the ledger, about the grep rather than the entry — inline on entry 9. B. Clean fix, or a hole drilled around the problem? The connection-env refactor is the right shape. Making provenance an explicit published fact ( Three items:
No dead code, leftover debug logging, or swallowed-error fallbacks found. The C. Regression risk Behavioural changes I identified, and what covers them:
Two regressions with no covering test, both inline:
Also inline: I could not run the suite, so nothing above is a claim that any test passes or fails — only which paths have coverage and which do not. The documentation standard in this diff is unusually high; several findings came from reading a docstring that named its own edge case, which is what made them findable at all. All of these are suggestions — accept or reject as you see fit. |
…indings - run-cli.ts's splitConnectionEnv: credentialIsFlag checked field PRESENCE on overrides, not whether the flag's VALUE actually won. pickCredential's tier order is flag pat > env pat > profile pat > flag username/password, so `--username u --password p` against a profile that also stores a pat resolves to the profile's pat — resolved and overrides can name different kinds. The old check would then promote that profile pat into userFields, mislabelling it as the user's and making it permanently un-clearable by a later per-profile apply() for a different profile: the exact "--profile B authenticates as A" failure this whole refactor exists to fix, reached through a field the previous fix didn't cover. Now requires the VALUE to match what overrides supplied, not just the field being present. - connection/env.ts's applyUser: the clear-the-other-credential-kind branches deleted env vars unconditionally, including ones the user set in their own shell for reasons unrelated to this call. Gated on a new same-process writtenByApplyUser tracking set (distinct from CZ_ENV_DERIVED, which is published to children) so only a value this layer or a profile expansion actually holds gets cleared. - profile-env.test.ts, connection-config.test.ts, resolve-token-store.test.ts: CZ_ENV_DERIVED (and CZ_ACCOUNTS_URL where relevant) is a new process-global var applyClickZettaProfile now writes via ConnectionEnv.apply; add it to each file's cleanup so a leftover from one file can't misclassify another file's directly-set CZ_PAT/CZ_USERNAME as "inherited" instead of "user". - model-selection.ts and its test: convert the cz_change comment to the scannable wrapped banner, matching every other cz-owned file's convention. New regression tests for the credential value-matching fix. 4160 tests pass, typecheck clean.
| if (input.pinAllows("password") && input.profile?.username && input.profile.password) { | ||
| return { kind: "password", source: "profile", username: input.profile.username, password: input.profile.password } | ||
| } | ||
|
|
||
| if (input.ambient.inherited.pat) return { kind: "pat", source: "inherited", pat: input.ambient.inherited.pat } | ||
| if (input.ambient.inherited.username && input.ambient.inherited.password) { | ||
| return { | ||
| kind: "password", | ||
| source: "inherited", | ||
| username: input.ambient.inherited.username, | ||
| password: input.ambient.inherited.password, | ||
| } | ||
| } |
There was a problem hiding this comment.
MEDIUM — pinAllows gates the profile tiers but not the new inherited tiers, so auth_type can be bypassed by the same profile's own fields laundered through CZ_*. (confidence: high on the code path, medium on whether the current behavior is intended)
if (input.pinAllows("password") && input.profile?.username && input.profile.password) {
return { kind: "password", source: "profile", ... }
}
if (input.ambient.inherited.pat) return { kind: "pat", source: "inherited", pat: input.ambient.inherited.pat }
if (input.ambient.inherited.username && input.ambient.inherited.password) {The two inherited tiers are the only credential tiers in this function with no pinAllows check. That matters because bootstrap/profile-env.ts's FIELDS table expands every credential field a profile carries into CZ_* — pat, username and password — with no knowledge of auth_type, and ConnectionEnv.apply's own docstring makes that explicit:
A profile may legitimately carry both a pat and a username/password, so both are written when both exist; choosing between them is the resolver's job (
auth_type, then the tier order in connection/config.ts), not this layer's.
But the resolver only does that job for input.profile. Concrete path:
profiles.toml:[profiles.p] auth_type = "oauth", plus a leftoverpat = "P"on the same entry.- Agent-runtime middleware (
bootstrap/runtime.ts) →applyClickZettaProfile("p")→CZ_PAT=P, marked derived. resolveConnectionConfig():pinAllows("pat")is false so the profile pat is correctly skipped — theninput.ambient.inherited.patreturns"P"anyway andcfg.pat = "P".
When the profile has a live oauth = "<id>" pointer the store is still attached and getToken prefers the stored token, so the pin holds by accident. When it does not (never logged in, token cleared), the invocation silently authenticates by the PAT the pin was there to exclude, instead of failing with "not logged in". The same holds for auth_type = "cookie", where both pinAllows("pat") and pinAllows("password") are false and the inherited pat is the only tier left.
This is not a regression — the pre-refactor code reached the same value through envPat = process.env.CZ_PAT, which ranked even higher — but this PR is what introduces the provenance distinction and rewrites the tier table, and the config comment above (lines 63–68) states the intended contract as "when set on the profile it SELECTS one credential and the profile's other credential fields are ignored". The inherited layer is a profile's credential fields.
Smaller correct change than routing around it: either gate both inherited tiers with pinAllows the same way the profile tiers are, or make the expansion pin-aware so ConnectionEnv.apply only ever receives the credential the pin allows — the latter also stops the excluded credential from being handed to every child process in the environment.
No test covers auth_type against the inherited layer: resolve-token-store.test.ts and connection-provenance.test.ts both exercise the inherited tiers and the pin, but never together.
| const derivedFields: ConnectionEnv.Fields = {} | ||
| for (const key of NON_AUTH_CONNECTION_KEYS) { | ||
| if (!overrides[key] && resolved[key]) derivedFields[key] = resolved[key] | ||
| } |
There was a problem hiding this comment.
LOW — derivedFields structurally can never carry accountsUrl, and ConnectionEnv.apply deletes every derived name absent from fields, so CZ_ACCOUNTS_URL is cleared whenever this runs. (confidence: high on the mechanism, medium that a user-visible path is currently affected)
const derivedFields: ConnectionEnv.Fields = {}
for (const key of NON_AUTH_CONNECTION_KEYS) {
if (!overrides[key] && resolved[key]) derivedFields[key] = resolved[key]
}NON_AUTH_CONNECTION_KEYS is the six SDK fields, and resolved is resolveConnectionConfig's output — a ConnectionConfig, which has no accountsUrl member at all (packages/clickzetta-sdk/src/types/index.ts). So accountsUrl is the one ConnectionEnv.Field that can never appear in either output of this function.
ConnectionEnv.apply then treats absence as "no longer derived, delete it":
if (derived.has(name)) delete process.env[name]bootstrap/profile-env.ts's FIELDS does map accounts_url → accountsUrl, so a profile with accounts_url set gets CZ_ACCOUNTS_URL written and marked derived, and that marking is inherited by child processes through CZ_ENV_DERIVED. The pre-refactor applyAgentConnectionEnv never touched CZ_ACCOUNTS_URL; this version deletes it.
The reader is packages/clickzetta-ai-gateway/src/gateway-error.ts:199:
const accountsUrl = process.env.CZ_ACCOUNTS_URL?.trim()
if (!accountsUrl) return "Insufficient account balance."On the agent path the later applyClickZettaProfile in bootstrap/runtime.ts's middleware re-derives it, so I could not identify a currently-broken flow. The exposure is a nested cz-cli invocation that inherits CZ_ENV_DERIVED from an agent session, passes at least one connection flag (so the Object.keys(overrides).length === 0 early return does not fire), and never reaches the agent-runtime middleware — a non-agent subcommand run from a shell tool. There it loses the inherited value and the billing message drops its "add funds" URL.
Smallest fix: carry accountsUrl through, either by having splitConnectionEnv read it off the profile entry alongside the rest, or by exempting it from apply's delete sweep since nothing on this path can re-derive it. No test covers CZ_ACCOUNTS_URL across applyAgentConnectionEnv — connection-provenance.test.ts asserts only the six non-auth keys plus credentials.
| const OWNER = "src/connection/env.ts" | ||
| const VARS = [ | ||
| "CZ_PROFILE", | ||
| "CZ_ENV_DERIVED", | ||
| "CZ_PAT", | ||
| "CZ_USERNAME", | ||
| "CZ_PASSWORD", | ||
| "CZ_SERVICE", | ||
| "CZ_PROTOCOL", | ||
| "CZ_INSTANCE", | ||
| "CZ_WORKSPACE", | ||
| "CZ_SCHEMA", | ||
| "CZ_VCLUSTER", | ||
| "CZ_ACCOUNTS_URL", | ||
| ] | ||
|
|
||
| // Matches the ways a var is reachable through the environment — `process.env.X`, | ||
| // `process.env["X"]`, and the `env.X` shorthand after `const env = process.env`. | ||
| const ACCESS = new RegExp(`(?:process\\.)?env(?:\\.(?:${VARS.join("|")})\\b|\\[\\s*["'\`](?:${VARS.join("|")})["'\`]\\s*\\])`) | ||
|
|
||
| function sources(dir: string): string[] { | ||
| return readdirSync(dir).flatMap((entry) => { | ||
| const full = join(dir, entry) | ||
| if (statSync(full).isDirectory()) return sources(full) | ||
| return full.endsWith(".ts") || full.endsWith(".tsx") ? [full] : [] | ||
| }) | ||
| } | ||
|
|
||
| test("only connection/env.ts touches the CZ_* connection variables", () => { | ||
| const root = join(import.meta.dir, "..") | ||
| const offenders = sources(join(root, "src")) |
There was a problem hiding this comment.
LOW — the single-owner guard only scans packages/cz-cli/src, and there is already a second reader outside it. (confidence: high)
const OWNER = "src/connection/env.ts"
const VARS = [
"CZ_PROFILE",
"CZ_ENV_DERIVED",
...
"CZ_ACCOUNTS_URL",
] const offenders = sources(join(root, "src"))root is packages/cz-cli, so the sweep stops at this package. But packages/clickzetta-ai-gateway/src/gateway-error.ts:199 reads one of the listed variables directly:
const accountsUrl = process.env.CZ_ACCOUNTS_URL?.trim()The test's own premise is that provenance "cannot be reconstructed by a second writer" — reads are less dangerous than writes, but this reader is exactly the one that goes silently wrong if the value is deleted out from under it (see my note on splitConnectionEnv's derivedFields, which can never re-derive accountsUrl). As written the test would pass today and would keep passing if clickzetta-ai-gateway or clickzetta-sdk started writing one of these vars.
Two options: widen the sweep to the sibling cz packages and add gateway-error.ts as a second permitted reader, or narrow VARS/the docblock to say the invariant is scoped to packages/cz-cli/src so the gap is visible rather than implied.
Separately, the comment stripping is line.replace(/\/\/.*$/, ""), which also truncates at a // inside a string literal — so env["CZ_PAT"] sharing a line with any URL string escapes detection. Only weakens the guard; no current offender.
| if (!modelID) return undefined | ||
| return { providerID: provider.id, modelID, source: "first" } | ||
| } | ||
| //======================== cz-cli change ======================== |
There was a problem hiding this comment.
LOW — the closing banner uses the opening form, so rg -n "cz-cli change" shows two start markers and no terminator for this patch. (confidence: high)
//======================== cz-cli change ========================
The ledger's own spec (and every other patch converted in this PR) pairs the opening marker with a distinct terminator:
//======================== cz-cli change ========================
... the edit, with rationale ...
//====================== end cz-cli change ======================
Since step 2 of the re-baseline procedure is exactly that rg, the output for entry 9 will read as two unclosed patch starts rather than one whole-file wrap, and the extent of the patch is not recoverable from the grep alone. Should be //====================== end cz-cli change ======================.
packages/core/test/model-selection.test.ts:1 has the same problem in the other direction — an opening marker with no terminator anywhere in the file.
Rating this LOW rather than HIGH only because both markers are grep-visible and entry 9 exists in the ledger with a Verify line naming the test; the failure mode here is a reader mis-reading the extent of the patch, not the patch being invisible.
| "build": "bun run script/build.ts", | ||
| "typecheck": "tsgo --noEmit", | ||
| "test": "bun test --timeout 30000", | ||
| "test": "bun test --isolate --timeout 30000", |
There was a problem hiding this comment.
LOW — --isolate changes the test contract for all ~4148 tests to contain one suite's mock.module leak that this PR also fixes properly. (confidence: high on the tradeoff; I cannot run the suite to confirm either fix works)
"test": "bun test --isolate --timeout 30000",This is the shape category B warns about: a global knob added to route around a specific defect. The specific defect is fixed independently in the same PR — test/analytics-agent-session-commands.test.ts now restores its three mocks in afterAll, which is the correct, targeted fix. cz-test.yml's comment argues for keeping both as belt-and-suspenders, and concedes the cost itself:
it can also hide a REAL cross-file coupling that would still bite in the shipped binary, where every test file's module cache is shared
That concession is the argument against making it the default: with --isolate on, a suite that only passes because it gets a fresh module registry now looks green locally and in CI, while the shipped binary shares one registry. The afterAll restore does not have that downside.
Two smaller options: keep --isolate out of the default test script and add a separate test:isolate for diagnosing leaks, or land it but track which suites depend on it so the flag can be removed rather than becoming permanent. Either way, worth calling out explicitly in the PR description as a change to how everyone runs tests locally, since it is not implied by any of the four stated changes.
Also note this is the one line in the PR that makes local and CI test semantics differ from every other package in the monorepo, whose test scripts are untouched.
| const config = resolveConnectionConfig({ | ||
| profile: info.profile, | ||
| ...(typeof profile.service === "string" ? { service: profile.service } : {}), | ||
| ...(profile.protocol === "http" || profile.protocol === "https" ? { protocol: profile.protocol } : {}), | ||
| ...(typeof profile.instance === "string" ? { instance: profile.instance } : {}), | ||
| }) |
There was a problem hiding this comment.
LOW — the profile→resolveConnectionConfig override block is copy-pasted from fetchProfileSnapshot. (confidence: high)
const config = resolveConnectionConfig({
profile: info.profile,
...(typeof profile.service === "string" ? { service: profile.service } : {}),
...(profile.protocol === "http" || profile.protocol === "https" ? { protocol: profile.protocol } : {}),
...(typeof profile.instance === "string" ? { instance: profile.instance } : {}),
})fetchProfileSnapshot (line 677) builds the identical object from the identical three fields with the identical type guards. The two now have to be kept in step by hand: if a fourth field ever needs pinning, or the protocol guard needs to accept another value, only one of them will get it — and the visible symptom would be the Profile section and the Quota section resolving against different hosts for the same profile, which is precisely the class of confusion this panel exists to prevent.
One small profileConnectionConfig(name, profile) helper called from both sites covers it.
Worth noting the semantics too: passing the profile's own service/instance as cliArgs means they take the flag tier, so a user's CZ_SERVICE override is ignored for these two reads while it applies everywhere else in the CLI. That is pre-existing in fetchProfileSnapshot and consistent between the two, so not a new issue — but a shared helper is where a comment explaining it belongs.
| props.onContext( | ||
| `${props.api.route.current.name}:${sessionID}:${props.activeModel.providerID(sessionID || undefined) ?? ""}`, | ||
| ) | ||
| props.onContext(`${props.sessionID}:${props.activeModel.providerID(props.sessionID) ?? ""}`) |
There was a problem hiding this comment.
LOW — the || undefined guard on the providerID argument was dropped, and empty string is not equivalent to undefined here. (confidence: high on the semantics, low that it is reachable today)
props.onContext(`${props.sessionID}:${props.activeModel.providerID(props.sessionID) ?? ""}`)Previously:
const sessionID = currentSessionID(props.api) ?? ""
props.onContext(
`${props.api.route.current.name}:${sessionID}:${props.activeModel.providerID(sessionID || undefined) ?? ""}`,
)active-model.ts:96 resolves the argument with ??, not ||:
providerID(sessionID) {
const id = sessionID ?? currentSessionID(api)
return (id ? observed.get(id) : undefined) ?? predictedProviderID(api)
},So providerID("") keeps id = "", skips the observed map entirely, and falls straight to predictedProviderID(api) — whereas providerID(undefined) would have resolved the current session and consulted observed. The old sessionID || undefined existed to convert exactly that case.
sidebar_content receives session_id from packages/tui/src/routes/session/sidebar.tsx:85, which passes the route's props.sessionID, so it should always be a real id and this is unreachable in practice. Restoring props.sessionID || undefined costs nothing and removes the dependence on that upstream guarantee.
Dropping props.api.route.current.name from the context key is fine by contrast — sidebar_content only renders on the session route, so the segment was constant.
| const activeProfileInfo = () => { | ||
| // Latched on a DEFINED result, not on attempt: readProfileInfo() returning | ||
| // undefined (no profile resolved yet) must not stick — the same "latch on | ||
| // success" reasoning as startUserNameFetch's `started` below, otherwise a | ||
| // first mount that resolves nothing means the Profile section is gone for | ||
| // the rest of the session even once a profile becomes configured. | ||
| if (profileInfoSignal() === undefined) setProfileInfoSignal(readProfileInfo()) | ||
| return profileInfoSignal() | ||
| } |
There was a problem hiding this comment.
LOW — activeProfileInfo writes a signal it also reads, from inside View's createMemo. (confidence: medium)
const activeProfileInfo = () => {
if (profileInfoSignal() === undefined) setProfileInfoSignal(readProfileInfo())
return profileInfoSignal()
}This is only ever called from View's profile memo (props.profileInfo()), so the read subscribes the memo to profileInfoSignal and the write then invalidates the computation currently executing. It does terminate: the second pass finds the signal defined and skips the write, and when readProfileInfo() returns undefined the setter is a no-op under Solid's default === equality. But writing to a dependency of the running computation is the pattern Solid's dev build flags as a potential loop, and it makes the memo run twice on first paint.
There is also a case where the intended "I/O off the render path" property does not hold: while readProfileInfo() keeps returning undefined (no profile resolvable), the latch never engages, so every re-run of the profile memo — triggered by props.userName() or a load() refresh — repeats the two-to-three readFileSync + TOML parses that the comment at lines 76–86 says must not run per render. That is the unconfigured case only, so the cost is bounded.
load() already calls setProfileInfoSignal(readProfileInfo()) on every busy→idle edge, and installQuotaIndicator ends with an unconditional controller.refresh(). A single eager setProfileInfoSignal(readProfileInfo()) at install time (or letting that first refresh() be the only seed) would give the same first paint with a plain read here and no write-during-read.
| api.slots.register({ | ||
| order: 100, | ||
| order: 150, | ||
| slots: { | ||
| home_prompt_right() { | ||
| return <View api={api} activeModel={activeModel} snapshot={snapshot} onContext={onContext} /> | ||
| }, | ||
| session_prompt_right() { | ||
| return <View api={api} activeModel={activeModel} snapshot={snapshot} onContext={onContext} /> | ||
| sidebar_content(_ctx, props) { |
There was a problem hiding this comment.
MEDIUM (regression surface, documented as intentional) — moving from home_prompt_right/session_prompt_right to sidebar_content removes the readout from three places it renders today. (confidence: high)
sidebar_content(_ctx, props) {Enumerating what stops working, since the PR title frames this as a placement change:
- Home route —
sidebar_contentis consumed only bypackages/tui/src/routes/session/sidebar.tsx.home_prompt_rightwas registered before, so balance is no longer visible before entering a session. - Terminals under 120 columns —
sidebarVisibleinroutes/session/index.tsxauto-opens the sidebar only at ≥120 columns; below that it takes a keystroke. The stated motivation was that the prompt corner squeezed the balance away at 80 columns, and at 80 columns the new location is behind a toggle. - Child/subagent sessions —
sidebarVisible's first check isif (session()?.parentID) return false, evaluated before the toggle, so these sections cannot be reached at all on a child session at any width. The prompt-corner slot rendered identically on parent and child.
All three are written down in UPSTREAM-PATCHES.md's new HOOK entry 5, including the child-session case, so I am reading this as accepted rather than an oversight — flagging it here only so the tradeoff is anchored at the code and not just in the ledger. (1) in particular is not called out in the PR description.
No test covers the visibility change; tui-quota-format.test.ts covers row rendering and tui-quota-data.test.ts covers fetching, but slot registration and the sidebar's own gating are untested here (they are upstream behavior).
If losing (1) matters, keeping the home_prompt_right registration alongside sidebar_content would cover the home screen without touching upstream — the two slots are independent and Section already renders nothing when there are no rows.
| @@ -0,0 +1,77 @@ | |||
| name: cz-test | |||
There was a problem hiding this comment.
LOW (scope) — four independent changes in one PR, and the CI-governance one deletes two workflows unrelated to everything else here. (confidence: high)
The PR description is candid that this bundles the quota sidebar, a CI-governance change, the connection-env/provenance refactor, and the upstream-banner conversion. Three of those are genuinely independent of each other, and the credential-provenance refactor in particular is the highest-risk change in the PR (it rewrites how every cz-cli invocation picks an identity) while being the one least connected to the title. Splitting it out would make it reviewable and revertable on its own.
On this workflow specifically, two things worth confirming rather than assuming:
pr-standards.ymlhad two jobs,check-standardsandcheck-compliance. Thecompliance-close.ymlcomment accounts forcheck-compliance, but not for whatevercheck-standardsenforced. If any of its checks are configured as required status checks onmain, deleting the workflow leaves PRs waiting on a check that can never report. Worth verifying the branch-protection settings before merge..github/claude-review-context.mddocuments a conventional-commit convention for PR and commit titles. Ifcheck-standardswas what enforced that, it is now unenforced. That may be fine for a fork, but it is a policy change rather than a cleanup.
cz-test.yml itself looks right: pull_request (not pull_request_target), permissions: contents: read, pinned action SHAs, GitHub-hosted runners, and the extra step running the two cz-owned suites inside upstream packages so the ledger's Verify lines for entries 7 and 9 are actually backed by a check. I cannot run it, so I have not confirmed the steps pass.
Review summaryA. Upstream invasiveness — no blocking issuesAll four upstream-package edits in this PR are One formatting defect, LOW: the closing banner in B. Clean fix vs. hole drilled around the problemThe connection-env refactor is the right shape: Four findings:
No dead code, commented-out code, or leftover debug logging found. C. Regression risk
Behavior changes I checked and found deliberate and covered by a new test:
One change with no test that I could find: Public API changes to note: I could not run the build or the test suite in this environment, so nothing above is a claim that anything passes. |
- run-cli.ts: resolveConnectionConfig's output (ConnectionConfig) has no accountsUrl field at all, so splitConnectionEnv's derivedFields could never carry it — yet ConnectionEnv.apply deletes every derived name absent from what it's given. A nested cz-cli invocation that inherits CZ_ENV_DERIVED but passes a connection flag (so it reaches applyAgentConnectionEnv) and never hits the agent-runtime middleware that re-derives it would silently lose CZ_ACCOUNTS_URL, dropping the "add funds" link from the billing error message. New resolveAccountsUrl reads it directly off the profile entry and folds it into derivedFields so it survives. - model-selection.ts and its test: the whole-file wrap used the OPENING banner form as its own closing marker instead of the "end cz-cli change" terminator, so `rg -n "cz-cli change"` read as two unclosed patch starts. New tests for resolveAccountsUrl. 4164 tests pass, typecheck clean.
|
Review summary Read A. Upstream invasiveness — one finding Every edit this PR makes to
I confirmed the closing banners do not over-reach: provider.ts helper banner ends before the untouched The quota feature itself stays in the cz layer: it registers on the public The one finding: the conversion misses four cz-owned files that live inside upstream packages and still carry no banner — B. Clean fix, or a hole drilled around the problem? Mostly the clean fix, and in two places notably so:
Two things to weigh, both filed inline:
No dead code, no leftover debug logging, no copy-pasted logic. Removed exports ( C. Regression risk
Two smaller items I did not file inline:
Also verified: The remaining inline comments are on |
| //======================== cz-cli change ======================== | ||
| // cz_change: unit coverage for the extracted model-selection chain (UPSTREAM-PATCHES.md entry 9) |
There was a problem hiding this comment.
HIGH — the banner conversion stops short of four cz-owned files that live inside upstream packages (confidence: high)
//======================== cz-cli change ========================
// cz_change: unit coverage for the extracted model-selection chain (UPSTREAM-PATCHES.md entry 9)This is the right treatment, and it establishes the precedent: a wholly cz-owned file sitting inside an upstream package gets a whole-file banner so rg -n "cz-cli change" packages/core packages/opencode packages/tui (step 2 of the ledger's re-baseline procedure) finds it. But rg over the branch still shows these four cz-owned files with no banner at all:
packages/tui/src/component/prompt/sql-command.ts:1— production source,// cz_change:onlypackages/tui/test/component/prompt/sql-command.test.ts:1— no markerpackages/opencode/test/provider/clickzetta-discovery.test.ts:1—// cz_change:onlypackages/opencode/test/provider/clickzetta-context-limit.test.ts:1— no marker
Two of them are the ledger's own enforcement: entry 7's Verify names clickzetta-discovery.test.ts / clickzetta-context-limit.test.ts, and entry 4 names sql-command.ts under "cz files". Entry 4's own History note says the /sql patch "was invisible to the re-baseline checklist" for exactly this reason — that gap is still open for the file the patch depends on, and for the tests the ledger points at as proof the patches survived.
The PR description says it "converts remaining cz_change: comments in packages/opencode/packages/tui to the scannable banner format", so this looks like an oversight rather than a decision. Suggest wrapping all four the same way this file is wrapped (and, for sql-command.ts, that its banner is what makes entry 4's cz files line discoverable from the grep rather than only from reading the ledger).
| const explicitCredential = | ||
| (credential.source === "env" && credential.kind === "pat") || | ||
| (credential.source === "flag" && | ||
| (credential.kind === "pat" || Boolean(cliArgs.username && cliArgs.password))) |
There was a problem hiding this comment.
MEDIUM — please confirm intent: an explicit --username/--password pair that loses the tier now re-enables the stored OAuth token (confidence: high that the behavior changes; the intent is documented, so this is a confirm-and-move-on)
const explicitCredential =
(credential.source === "env" && credential.kind === "pat") ||
(credential.source === "flag" &&
(credential.kind === "pat" || Boolean(cliArgs.username && cliArgs.password)))Old formula: Boolean(cliPat) || Boolean(envPat) || Boolean(cliUsername && cliPassword) — presence of the flags was enough, whether or not they won.
Concrete case, and it's the one test/resolve-token-store.test.ts:191 pins: profile has pat + oauth = "sess", no auth_type, and the user runs --username alice --password secret.
- Before:
explicitCredentialtrue → notokenStore→ the request authenticates with the profile pat. - After:
credential.source === "profile"→ not explicit →tokenStoreattached →getToken()consults the store first (the comment at L118–123 is explicit about this ordering) → the session authenticates as the stored OAuth identity.
So the user passed a credential, got neither it nor the pat, and the identity actually used changed. The docstring's argument ("the flags LOST the tier, so they are not the ones authenticating") is internally consistent with the profile-level-pat case, but the net effect is that explicit credential flags are now silently discarded in favour of a persisted login — which is the failure mode L117–123 was written to prevent, just reached from the other direction.
Two cheaper options if this wasn't the intended trade:
- keep the source-based rule for
inherited(which is the actual provenance bug being fixed) and leave a presentcliArgs.pator a completecliArgs.username && cliArgs.passwordpair suppressing the store regardless of which tier won; or - treat "credential flags were passed but lost the tier" as an error rather than a silent discard.
If it is intended, no change needed — flagging it because it's an auth-identity change and the reasoning currently lives only in a comment and one test's prose.
| input.cliArgs.username || | ||
| input.jdbc?.username || | ||
| input.ambient.user.username || |
There was a problem hiding this comment.
LOW — behavior change worth naming in the changelog, not just the docstring (confidence: high)
input.cliArgs.username ||
input.jdbc?.username ||
input.ambient.user.username ||The docstring above (L229–241) is honest that dropping getEnvConfig()'s completeness gate changes precedence: a half-set CZ_USERNAME with no CZ_PASSWORD now participates in the flag-tier merge, so CZ_USERNAME=envuser cz-cli … --password p against a profile storing username = "profileuser" resolves to envuser/p where it used to resolve to profileuser/p. Covered by test/resolve-token-store.test.ts:209.
That's a change to which identity authenticates driven by an env var a user may have exported long ago for an unrelated reason, and it fails with a login error rather than anything self-explanatory. No code change requested — just noting it belongs in the release notes for this refactor, since a user hitting it has no way to connect the failure to a change in the completeness gate.
| } | ||
| return portalCall(baseUrl, `${API_KEYS_PATH}?userName=${encodeURIComponent(userName)}`, token.token, { | ||
| const userName = profileUserName || (await readCurrentUserName(baseUrl, token.token, input.signal)) || "" | ||
| return portalRead(baseUrl, `${API_KEYS_PATH}?userName=${encodeURIComponent(userName)}`, token.token, { |
There was a problem hiding this comment.
MEDIUM — the unserved-route key includes a query string that varies, so the double-request the strike counter exists to stop can persist (confidence: medium)
return portalRead(baseUrl, `${API_KEYS_PATH}?userName=${encodeURIComponent(userName)}`, token.token, {unservedHostKey is ${baseUrl}\n${path} (L528), and the path handed in here carries ?userName=<name>. userName is recomputed on every snapshot — profileUserName || (await readCurrentUserName(...)) || "" (L703) — and this path does not consult userNameCache (that's only in fetchProfileUserName), so the value can differ between refreshes.
On the exact deployment this mechanism was built for (a region host answering 8888), the sequence is:
- refresh 1: region host fails
getCurrentUser→readCurrentUserNamereturns undefined →userName = ""→ strike recorded under…listApiKeys?userName=. - once the central-host fallback starts answering
getCurrentUser,userNamebecomesalice→ route key becomes…listApiKeys?userName=alice, strike count 0 → the region host is probed directly again, and the accumulated strikes under the old key are stranded.
UNSERVED_HOST_THRESHOLD = 2 requires two consecutive strikes under the same key, so a userName that flips can keep the counter from ever reaching the threshold — meaning a session on such a profile keeps paying the extra failing round-trip once per agent turn, which is precisely what the docstring at L500–512 says the memory exists to avoid.
Suggest keying the strike map on the route shape rather than the resolved URL — e.g. pass a stable routeKey alongside path (API_KEYS_PATH without the query), or strip the query in unservedHostKey. The per-route granularity the docstring argues for is preserved either way, since the three routes still have distinct paths.
| export function centralPortalHost(baseUrl: string): string | undefined { | ||
| const stripped = baseUrl.replace(/^(https?:\/\/)[a-z0-9-]+\.(api\.(?:clickzetta|singdata)\.com)(?=\/|$)/i, "$1$2") |
There was a problem hiding this comment.
LOW — the profile's portal token goes to a host the profile never named, with no way to opt out (confidence: high; documented as intentional)
export function centralPortalHost(baseUrl: string): string | undefined {
const stripped = baseUrl.replace(/^(https?:\/\/)[a-z0-9-]+\.(api\.(?:clickzetta|singdata)\.com)(?=\/|$)/i, "$1$2")Verified the narrowing works as the docstring claims: uat-api.clickzetta.com / dev-api.clickzetta.com don't match (their label is part of uat-api, not a segment before .api.), a host on a private domain doesn't match, and the two roots are pinned. So the blast radius really is the two measured roots. UPSTREAM-PATCHES.md's HOOK entry 5 records this as confirmed intentional, including "No config escape hatch exists to opt out of the fallback."
Raising it once anyway because a reader of this file alone won't see that: the fallback transmits x-clickzetta-token for the active profile to a host the tenant did not configure in profiles.toml, and it fires automatically on any unusable business code — which per the docstring includes 8888, described elsewhere in the file as a generic server-side error a healthy host can return during a blip. A tenant with a policy about which endpoints their credentials may reach has no lever here.
Not asking for a redesign. Worth considering a single opt-out (a profile key or CZ_* flag checked before the rewrite), and — if the two roots are the only ones ever intended — noting that the i flag plus https? means an http:// profile has its token rewritten onto plaintext http://api.clickzetta.com too.
| if (profileInfoSignal() === undefined) setProfileInfoSignal(readProfileInfo()) | ||
| return profileInfoSignal() |
There was a problem hiding this comment.
LOW — the "latch on a defined result" rule means the file I/O this signal exists to avoid still runs repeatedly in the no-profile case (confidence: medium)
if (profileInfoSignal() === undefined) setProfileInfoSignal(readProfileInfo())
return profileInfoSignal()The comment above the signal declaration is explicit that readProfileInfo() "does two-to-three synchronous readFileSync+TOML-parse passes with no memoization of its own, so it must not run on every render." That holds once a profile resolves. It does not hold when it doesn't: readProfileInfo() returning undefined leaves the signal undefined, the === undefined guard stays open, and every subsequent evaluation of View's profile memo re-runs all three file reads. The memo also depends on props.userName(), so it is re-run by things unrelated to the profile.
Not a correctness bug — it converges the moment a profile resolves, and Solid's equality check means setProfileInfoSignal(undefined) doesn't loop. But an unconfigured or stale-CZ_PROFILE session is exactly the case where the section renders nothing, so the cost buys nothing.
A separate let attempted (or a three-state signal that can hold "read, found nothing") would keep the retry-on-refresh behaviour that load() already provides while closing the repeat-read path — load() calls setProfileInfoSignal(readProfileInfo()) unconditionally on every busy→idle edge, so a profile that becomes configured mid-session is still picked up without this guard staying open.
Separately: setProfileInfoSignal is called from inside the memo's own computation on a signal that computation reads. It converges here, but it's the kind of write-during-read Solid doesn't guarantee much about; doing the read in load() only (it already happens there) would avoid it entirely.
| "build": "bun run script/build.ts", | ||
| "typecheck": "tsgo --noEmit", | ||
| "test": "bun test --timeout 30000", | ||
| "test": "bun test --isolate --timeout 30000", |
There was a problem hiding this comment.
LOW — --isolate is a global mitigation for one file's leak, and it can hide coupling that still bites the shipped binary (confidence: high)
"test": "bun test --isolate --timeout 30000",The root cause is fixed properly in the same PR (test/analytics-agent-session-commands.test.ts now restores its three mock.module registrations in afterAll). --isolate on top of that is a second, much broader lever: it changes the test contract for all ~4148 tests locally and in CI, costs a fresh global per file, and — as cz-test.yml's own comment concedes — "can also hide a REAL cross-file coupling that would still bite in the shipped binary, where every test file's module cache is shared."
The reasoning for keeping both is written down in three places, so this is informational rather than a request. Two things worth weighing:
- The new module-level state in this PR (
writtenByApplyUserinconnection/env.ts,unservedHostStrikes/userNameCacheintui-quota-data.ts) each got a…ForTest()reset and is covered by--isolate. With--isolatein place, a suite that forgets a reset passes locally and in CI, and the omission only shows up as real-world state bleed in the bundled binary — which is the failure class--isolateis least able to warn about. - If the cost is later judged too high and
--isolateis dropped, the belt-and-suspenders story only holds for files that remembered the manual reset. A grep-able convention (or a sharedbeforeEachhelper that calls all three resets) would make that recoverable; today it's per-file discipline.
| # Issues only: the PR half of this mechanism (pr-standards.yml's check-compliance | ||
| # job, the only thing that ever applied needs:compliance to a pull request) was | ||
| # removed as part of dropping the upstream community-PR-governance workflows this | ||
| # fork doesn't use (see .github/workflows/pr-standards.yml's removal). The only | ||
| # remaining producer of this label, duplicate-issues.yml, labels issues only. |
There was a problem hiding this comment.
LOW — repo-governance changes bundled into a TUI feature PR (confidence: high)
# Issues only: the PR half of this mechanism (pr-standards.yml's check-compliance
# job, the only thing that ever applied needs:compliance to a pull request) was
# removed as part of dropping the upstream community-PR-governance workflows this
# fork doesn't use (see .github/workflows/pr-standards.yml's removal). The only
# remaining producer of this label, duplicate-issues.yml, labels issues only.I verified the factual claim: needs:compliance is now produced only by duplicate-issues.yml, which labels issues (gh issue edit, github.event.issue), so narrowing this job to !item.pull_request and dropping pull-requests: write is consistent. The remaining github.rest.issues.update path only needs issues: write. The dev → main fix in the CONTRIBUTING link is also correct for this fork.
The note is about packaging, not correctness: deleting pr-standards.yml (351 lines) and pr-management.yml (95 lines) removes automation that gates this repo's own PRs, in the same commit range as a sidebar feature and a credential-provenance refactor. That's the kind of change a reviewer would want to be able to revert independently, and the kind whose blast radius is only visible after the next few PRs land. The PR description does disclose it, so this is a suggestion to split rather than a claim anything is wrong.
One loose end while you're here: close-prs.yml, close-issues.yml, review.yml and triage.yml are still present. If the reason for removing the other two was "upstream community workflows this fork can't run", it'd be worth saying in the PR description whether those four were checked and deliberately kept, so the next person doesn't have to re-derive it.
2 participants
Moves the balance/quota readout into the session sidebar and adds a Profile section showing the active connection's identity.
This PR also bundles three follow-on changes surfaced while building/reviewing the above:
pr-standards.yml/pr-management.yml(upstream community workflows this fork can't run — they read.github/TEAM_MEMBERSoff adevbranch that doesn't exist here), narrowscompliance-close.ymlto issues-only, and addscz-test.ymlso cz-cli's own typecheck/tests actually run in CI (the existingtest.ymlonly covers upstream opencode packages and is stuck queued on unprovisioned Blacksmith runners).connection/env.tsas the single owner of theCZ_*env vars, distinguishing user-set values from ones this process derived from a profile so a stale derived value can't outrank a later profile switch or shadow a flag the user just passed.config.ts'sresolveConnectionConfig/workspace.ts'suse --persistnow both resolve the active profile throughProfile.current()instead of duplicating/skipping parts of that formula.cz_change:comments inpackages/opencode/packages/tuito the scannable banner format, with matchingUPSTREAM-PATCHES.mdledger entries.Test coverage:
bun run typecheckandbun run test(4148 tests) pass inpackages/cz-cli.