Skip to content

feat(usage): link GLM Coding Plan accounts in AI Provider Accounts - #23804

Open
parkavenue9639 wants to merge 3 commits into
stablyai:mainfrom
parkavenue9639:parkavenue9639/zai
Open

parkavenue9639 wants to merge 3 commits into
stablyai:mainfrom
parkavenue9639:parkavenue9639/zai

Conversation

@parkavenue9639

Copy link
Copy Markdown

ELI5

Orca can already show a GLM Coding Plan quota meter, but only if you installed the ZCode CLI and configured it. This adds a GLM Coding Plan section to Settings → AI Provider Accounts: pick which site your plan belongs to (Z.AI international or Zhipu BigModel mainland), paste the plan API key, and the status-bar meter works — no ZCode CLI needed.

What Changed

  • Settings → AI Provider Accounts → GLM Coding Plan (new section, #accounts-zcode): site selector + API key input with Save/Replace/Forget, a "Get API key" link to the selected site's console, credential-state card, and the live 5-hour / weekly / MCP quota windows.
  • Credential store src/main/zcode/zcode-plan-api-key-store.ts: same safeStorage envelope pattern as the MiniMax API-key store; the key never crosses IPC back to the renderer.
  • Fetcher priority: fetchZcodeRateLimits accepts an Orca-saved planCredential (site → base URL via the shared src/shared/zcode-plan-sites.ts table, host allowlist reused) that takes priority over ~/.zcode/cli/config.json; the CLI config path is unchanged as fallback.
  • Service wiring: setZcodePlanConfigResolver, config-hash + zcodeFetchGeneration invalidation and generation-guarded result application, mirroring MiniMax; RateLimitState.zcodePlanApiKeyConfigured keeps the meter durable across reloads.
  • Status-bar gating: a saved plan key exempts the zcode meter from ZCode CLI PATH detection (same exemption MiniMax/Cursor have), so subscribers without the CLI still see their quota.
  • Wire parity: zcodePlanSite in GlobalSettings + RPC contract + runtime client settings projection; localization catalogs with Chinese translations; settings search entries; usage-provider-settings-target now points zcode at the new section.

Why

The merged #23520 meter reads only the ZCode CLI's config file, and the meter is PATH-gated on that CLI — a GLM Coding Plan subscriber running GLM through Claude Code, OpenCode, or Cline had no way to link their plan inside Orca. Both sites serve the same quota endpoint (/api/monitor/usage/quota/limit, raw Authorization key), so one credential flow with a site picker covers both. Extending the merged zcode provider (rather than adding a new provider id like earlier community PRs did) keeps a single status-bar slot and one snapshot pipeline.

Linked Issue

Fixes #23803

Visual Proof

Before: Settings → AI Provider Accounts ended at Cursor — no GLM section, and the status bar showed no zcode meter without the ZCode CLI on PATH.

After (live Coding Plan data on the mainland BigModel site, key linked through the real IPC path in a headless Electron run):

GLM Coding Plan section in AI Provider Accounts

Status bar zcode meter with live quota

The strip reads 66% used 15m · 33% used 3d — the 5-hour and weekly windows from the live quota endpoint, with reset countdowns.

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • Live smoke against https://open.bigmodel.cn/api/monitor/usage/quota/limit with a real Coding Plan key through the production fetcher: mapped 5-hour (credit-based percentage) and weekly windows, plan level, reset timestamps; a mangled key surfaces the server's auth error with no credential leakage.

  • The screenshots above come from an Electron run that linked the key through zcodePlanCredentials:saveApiKey (real safeStorage store → invalidate → refresh) and asserted the section's live rows before capturing.

  • New/extended suites: src/main/zcode/zcode-plan-api-key-store.test.ts, src/main/rate-limits/zcode-usage-fetcher.test.ts (plan-credential priority, host rejection, malformed fallback), src/main/rate-limits/service-zcode-usage.test.ts (resolver wiring, config-change discard, in-flight invalidation), src/renderer/src/components/settings/ZcodePlanAccountsSection.test.tsx, plus updated registration/visibility/search fixtures.

  • Local checks: pnpm tc, oxlint on all changed files, pnpm run check:code-quality:changed, localization catalog/extraction/coverage verifies, targeted vitest (rate-limits, status-bar, settings, startup, IPC: 1685 tests), electron-vite build --mode e2e. Full matrix left to CI.

AI Disclosure

Claude (Anthropic) implemented this change end-to-end under my direction, including the live smoke test and the headless screenshot run; I reviewed and validated the result.

Review

Self-reviewed for:

  • secret handling: key stored in the main process only (safeStorage envelope, hardened file), IPC returns booleans, provenance is an HMAC fingerprint, error strings asserted key-free;
  • no new provider id, no status-bar defaults change — one meter, one snapshot pipeline;
  • cross-platform: pure Node/net.fetch path, no shortcuts/platform assumptions, plaintext-envelope fallback matches the MiniMax store's landed behavior;
  • SSH/folder workspaces: usage monitoring stays provider/config scoped, no worktree dependency;
  • backwards compatibility: zcodePlanSite defaults to zai for existing profiles; all RateLimitState additions are additive.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Security: the quota request reuses the merged fetcher's hardened transport (HTTPS-only allowlisted hosts, default port, redirect: 'error', 15s timeout, unread-body cancellation).
  • Cross-platform: no -ExecutionPolicy, interpreter spawning, or shell paths added.
  • Remote SSH / mobile: credential is host-local by design (same as MiniMax); remote runtimes are out of scope for this PR.
  • Performance: credential presence is a cached in-process read; getState() only adds one sync existence check like the MiniMax flags.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

Add a GLM Coding Plan section to Settings > AI Provider Accounts with a
site selector (Z.AI / Zhipu BigModel) and an encrypted in-app API key store,
extending the merged zcode provider so the saved key takes priority over
~/.zcode/cli/config.json and usage refreshes hit the selected site.

A saved plan key also exempts the zcode status-bar meter from ZCode CLI PATH
detection, so subscribers without the CLI still see their quota.
@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 2/5

[Medium risk] Adds GLM Coding Plan credential management to the rate-limit system.

The PR is not yet safe to merge because failed credential replacement can leave a plaintext key insufficiently protected.

Findings

  1. P1 Security Failed restoration exposes API key ▶
  2. P1 Failed replacement deletes saved key ▶

Summary

The PR adds a GLM Coding Plan account section, local API-key storage, site-aware quota fetching, and status-bar visibility without requiring the ZCode CLI.

  • The latest change attempts to preserve an existing credential when a plaintext replacement cannot be secured, but its restoration path does not confirm that the credential remains protected.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Replace saved key] --> B[Write new plaintext envelope]
  B --> C{Permission restriction succeeded?}
  C -->|Yes| D[Use new key]
  C -->|No| E{Previous envelope available?}
  E -->|No| F[Remove published file]
  E -->|Yes| G[Attempt restoration]
  G --> H[Restoration result not verified]
Loading

Reviews (3) · Last reviewed commit: "fix(usage): restore the previous key whe..."

Comment thread src/main/rate-limits/zcode-usage-fetcher.ts
Comment thread src/main/zcode/zcode-plan-api-key-store.ts Outdated
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 68b77598-02d0-4e94-a811-f06426e9a6ff

📥 Commits

Reviewing files that changed from the base of the PR and between e3b970b and d2f13d2.

📒 Files selected for processing (2)
  • src/main/zcode/zcode-plan-api-key-store.test.ts
  • src/main/zcode/zcode-plan-api-key-store.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/zcode/zcode-plan-api-key-store.test.ts
  • src/main/zcode/zcode-plan-api-key-store.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The change adds Z.AI and BigModel site settings, API-key storage with encrypted and plaintext envelope handling, and IPC and preload methods for credential management. Zcode quota fetching can use the saved key ahead of CLI credentials. Fetch generations prevent results from older credential configurations from replacing current state. The settings UI adds site and key controls, usage windows, and status-bar visibility based on saved-key status or CLI detection.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to d2f13

Under an uncommon file-permission failure, an unsuccessful key replacement may take effect after restart. The change is otherwise mergeable with this known risk tracked for correction.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to e3b97

The new account-linking flow keeps keys in the main process and restricts quota requests to approved sites, but credential-file permission failures and failed credential changes can leave security-sensitive state inconsistent. Access to the new credential controls from less-trusted windows also needs confirmation.

Retained concerns

  • Medium · security · inferred: The new credential store can report an existing key as configured and read it after file hardening fails; encrypted saves can report success without checking whether the published file was restricted. Exposure depends on platform permissions and whether the stored envelope is plaintext.
  • Low · security · inferred: If a plaintext replacement fails its restriction check, the file is removed but the previous key can remain cached. Because the IPC handler then exits before invalidation, subsequent quota resolution can still use the old credential despite the failed replacement.
  • Medium · security · inferred: The new save and clear IPC channels do not check the sender locally. A renderer able to invoke them could replace or remove this host-local key; whether less-trusted renderers have that capability is unresolved. The channels do not return the stored secret.
Security review details

Security Blast Radius

  • inferred — The affected asset is a host-local GLM plan key and its quota display. Permission failure could expose a plaintext envelope to another principal with file access; no evidence establishes broader tenant or service access.

Security Findings and Attack Paths

  • inferred — A readable, insufficiently restricted plaintext envelope could be consumed as an active quota credential because read and status continue after attempted hardening. Actual cross-principal file access and the deferred candidate's exploitability are not established.

Trust Boundaries and Controls

  • observed — The fetcher rejects malformed saved keys and destinations outside its HTTPS host allowlist, prohibits redirects, and sends the authorization value only in the main-process request path shown.
  • inferred — Sender authorization for the new credential channels is unresolved: the handlers validate the key's type but do not inspect the IPC event, while registration sets a trusted UI renderer identifier for other handlers.

Resilience and Maintainability Implications

  • inferred — Generation invalidation protects completed credential changes from obsolete quota results, but failed save, replacement, or deletion exits before the handler's invalidation step. Deletion also clears the cache before filesystem removal, so failure can leave the file recoverable.

Hardening Proposals

  • proposed — Make permission outcomes explicit for credential reads and all save branches, and define the behavior when restriction is pending or fails before treating a key as available.
  • proposed — Confirm the IPC sender policy for credential channels and make failed replacement and forget transitions reconcile the file, cache, and visible quota state.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 39 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description includes all required sections and provides clear user-facing changes, rationale, linked issue, visual proof, testing details, AI disclosure, review notes, compatibility considerations…
Title check ✅ Passed The title is concise, specific, and accurately describes the main change: linking GLM Coding Plan accounts in AI Provider Accounts.
Linked Issues check ✅ Passed The PR satisfies the coding objectives in [#23803]. It adds the GLM Coding Plan settings section with Z.AI and BigModel selection, API-key controls, usage windows, localization, and search entries. Th…
Out of Scope Changes check ✅ Passed The changes stay within [#23803]. Credential storage, IPC, runtime settings, quota fetching, invalidation, status-bar visibility, settings UI, localization, search, and related tests directly support …
  • Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: ce99b535-f780-43d2-b135-3a469ec392f0

📥 Commits

Reviewing files that changed from the base of the PR and between 643f93f and dd3277d.

📒 Files selected for processing (41)
  • src/main/ipc/register-core-handlers/register-core-handlers.test.ts
  • src/main/ipc/register-core-handlers/register-core-handlers.ts
  • src/main/ipc/zcode-plan-credentials.ts
  • src/main/rate-limits/service-zcode-usage.test.ts
  • src/main/rate-limits/service/service-account-refresh.ts
  • src/main/rate-limits/service/service-configuration.ts
  • src/main/rate-limits/service/service-fetch-policy.ts
  • src/main/rate-limits/service/service-fetch-targets.ts
  • src/main/rate-limits/service/service-full-cycle-application.ts
  • src/main/rate-limits/service/service-full-cycle-preparation.ts
  • src/main/rate-limits/service/service-state.ts
  • src/main/rate-limits/service/service-types.ts
  • src/main/rate-limits/zcode-usage-fetcher.test.ts
  • src/main/rate-limits/zcode-usage-fetcher.ts
  • src/main/runtime/runtime-client-settings.ts
  • src/main/runtime/runtime-store-contract.ts
  • src/main/startup/main-process-account-services.ts
  • src/main/zcode/zcode-plan-api-key-store.test.ts
  • src/main/zcode/zcode-plan-api-key-store.ts
  • src/preload/api-types.ts
  • src/preload/api/agent-account-api.ts
  • src/preload/api/zcode-plan-credentials-bridge.ts
  • src/preload/index.ts
  • src/renderer/src/components/settings/AccountsPane.tsx
  • src/renderer/src/components/settings/ZcodePlanAccountsSection.test.tsx
  • src/renderer/src/components/settings/ZcodePlanAccountsSection.tsx
  • src/renderer/src/components/settings/accounts-search.ts
  • src/renderer/src/components/settings/zcode-plan-usage-windows.tsx
  • src/renderer/src/components/status-bar/status-bar-provider-visibility.test.ts
  • src/renderer/src/components/status-bar/status-bar-provider-visibility.ts
  • src/renderer/src/components/status-bar/usage-provider-settings-target.ts
  • src/renderer/src/components/status-bar/use-status-bar-controller.ts
  • src/renderer/src/i18n/locales/en.json
  • src/renderer/src/i18n/locales/zh.json
  • src/shared/default-global-settings.ts
  • src/shared/global-settings-types.ts
  • src/shared/rate-limit-state-factory.ts
  • src/shared/rate-limit-types.test.ts
  • src/shared/rate-limit-types.ts
  • src/shared/rpc-contract/client-settings-params.ts
  • src/shared/zcode-plan-sites.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Comment thread src/main/rate-limits/service/service-full-cycle-preparation.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one clear copy bug and a couple of minor suggestions.

Reviewed changes

Read the full 41-file diff for the new GLM Coding Plan linking flow in Settings → AI Provider Accounts.

  • Credential store zcode-plan-api-key-store.ts — safeStorage envelope mirror of the MiniMax store (~/.orca/zcode-plan-api-key.enc); main-only, IPC returns booleans, hardened file, plaintext fallback. Correctly matches the landed MiniMax behavior.
  • Fetcher priority zcode-usage-fetcher.ts — an Orca-saved planCredential takes priority over the ZCode CLI config, reusing the HTTPS/port/host allowlist; credentialSource becomes 'orca-plan'. Host-rejection and malformed-key fallback are tested.
  • Service wiring — setZcodePlanConfigResolver, config-hash + zcodeFetchGeneration invalidation, generation-guarded apply, resolver errors surfaced as zcode-only keychain-unavailable state, and RateLimitState.zcodePlanApiKeyConfigured as the durable meter signal. The generation/invalidation shape mirrors MiniMax and the in-flight-discard case is covered.
  • Settings section — site picker, save/replace/forget with success/error toasts, credential-state card, live quota windows; search entries, accounts-zcode target, and en/zh catalogs added.
  • Status-bar gating — a saved plan key exempts the zcode meter from ZCode CLI PATH detection, same exemption as MiniMax/Cursor.

ℹ️ The GLM section is not usable from the browser web client

PreloadApi.zcodePlanCredentials was added to the desktop preload, but no matching shim was added to src/renderer/src/web/preload-api/web-agent-accounts-api.ts / web-preload-api.ts, unlike minimaxCredentials. In the browser web client the section still renders, but window.api.zcodePlanCredentials resolves through the fallback proxy: getStatus() yields [] and saveApiKey() resolves undefined, so next.apiKeyConfigured in saveApiKey throws and the user gets an opaque "credential update failed" toast. MiniMax's shim returns an explicit "only available in the desktop app" message instead.

Technical details
# Web client shim for zcodePlanCredentials

## Affected sites
- `src/renderer/src/web/preload-api/web-agent-accounts-api.ts` — add a `createZcodePlanCredentialsApi()` alongside `createMiniMaxCredentialsApi`, returning `{ apiKeyConfigured: false, zcodeCliConfigured: false }` for `getStatus` and rejecting `saveApiKey` with the desktop-only explanation.
- `src/renderer/src/web/web-preload-api.ts` — register `zcodePlanCredentials: createZcodePlanCredentialsApi()`.

## Required outcome
- On the browser web client, the GLM Coding Plan section degrades like the MiniMax section: it reads as unlinked and any save attempt reports that key storage is desktop-only, rather than throwing.
- `web-preload-api-composition.test.ts` enumerates the concrete surface and will need `zcodePlanCredentials` added to its expected key list.

## Open questions for the human
- If the browser web client is intentionally out of scope for this feature (the PR notes remote/mobile), hiding the section there instead of shimming it is also fine — but the current partial wiring leaves a Save button that cannot work.

ℹ️ Nitpicks

  • ZCODE_PLAN_SITES and isZcodePlanSite in src/shared/zcode-plan-sites.ts are exported but unused; drop them or consume isZcodePlanSite in the site-select validation instead of the inline value !== 'zai' && value !== 'bigmodel' check.
  • src/main/ipc/zcode-plan-credentials.ts has no dedicated handler test, while the parallel minimax-credentials.ts has minimax-credentials.test.ts covering the same getStatus/save/clear and argument-validation paths.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/renderer/src/components/settings/zcode-plan-usage-windows.tsx Outdated
- Reject keys with interior newlines at save time and surface an unusable
  saved key as its own error instead of silently switching to the ZCode CLI
  config's account (greptile P1).
- Refuse to keep an unrestricted plaintext key when file hardening fails
  (greptile P1, security).
- Include the resolver error in the zcode config hash, mirroring MiniMax
  (CodeRabbit).
- Render the reset countdown from the bare duration so the copy reads
  'resets in 47m' once, not 'resets in Resets in 47m' (Pullfrog); covered
  by a new component test.
Comment on lines +99 to +102
if (!wroteRestricted) {
rmSync(getZcodePlanApiKeyPath(), { force: true })
throw new Error('GLM Coding Plan API key could not be stored securely on this device')
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Failed replacement deletes saved key

If a user replaces an existing key while OS encryption is unavailable and file hardening fails, the write has already replaced the old file before this cleanup deletes the new one. The replacement reports failure, but the old key remains in memory, so the current session can keep showing its usage while the saved key is gone after restart. Preserve the existing credential on a failed replacement, or clear the stale cache and make the loss explicit.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d2f13d2: the previous envelope bytes are captured before the plaintext write, and a failed replacement now restores them (writeSecureFile with the old bytes) instead of deleting the file — only a first-time key with nothing to preserve is removed. The in-memory cache was never updated on failure, so memory and disk stay consistent (old key both places). Same fix covers the CodeRabbit major; new test: restores the previous envelope when a plaintext replacement cannot be restricted.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟠 Major · Reject insecure plaintext credentials during reads. · zcode-plan-api-key-store.ts:106-130

src/main/zcode/zcode-plan-api-key-store.ts:106-130
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject insecure plaintext credentials during reads.

hardenExistingSecureFile() discards failed and pending hardening results. Therefore, hasZcodePlanApiKey() can report an unrestricted file as configured, and readZcodePlanApiKey() can return and cache its plaintext payload. The startup resolver then uses that credential.

The save path already deletes the file when restriction fails. Apply the same fail-closed policy to existing plaintext files. Remove the file and do not cache the key unless synchronous hardening succeeds.

Suggested fix
-import { hardenExistingSecureFile, writeSecureFile } from '../../shared/secure-file'
+import {
+  hardenExistingSecureFile,
+  hardenSecurePath,
+  writeSecureFile
+} from '../../shared/secure-file'
...
-export function hasZcodePlanApiKey(): boolean {
-  const keyPath = getZcodePlanApiKeyPath()
-  if (!existsSync(keyPath)) {
-    return false
-  }
-  try {
-    hardenExistingSecureFile(keyPath)
-  } catch (error) {
-    if (!warnedZcodePlanApiKeyStatusHardenFailure) {
-      warnedZcodePlanApiKeyStatusHardenFailure = true
-      console.warn(
-        '[zcode] Failed to harden GLM Coding Plan API key file while checking status',
-        error
-      )
-    }
-  }
-  return true
+export function hasZcodePlanApiKey(): boolean {
+  try {
+    return readZcodePlanApiKey() !== null
+  } catch {
+    return false
+  }
 }
...
     const raw = readFileSync(keyPath)
     const envelope = decodeApiKeyEnvelope(raw)
+    if (envelope.kind === 'plaintext') {
+      const restricted = hardenSecurePath(keyPath, {
+        isDirectory: false,
+        platform: process.platform,
+        sync: true
+      })
+      if (!restricted) {
+        rmSync(keyPath, { force: true })
+        throw new Error('GLM Coding Plan API key could not be stored securely')
+      }
+    }
     cachedZcodePlanApiKey = readEnvelope(envelope)
-): void {
-  applySecurePathRestriction(
+): boolean {
+  return applySecurePathRestriction(
     targetPath,
     options.isDirectory,
     options.platform,
     options.sync ?? false
-  )
+  ) === 'applied'
 }

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 1ee3837b-d6c9-4049-9d28-8b3ed9cf581c

📥 Commits

Reviewing files that changed from the base of the PR and between dd3277d and e3b970b.

📒 Files selected for processing (7)
  • src/main/rate-limits/service/service-full-cycle-preparation.ts
  • src/main/rate-limits/zcode-usage-fetcher.test.ts
  • src/main/rate-limits/zcode-usage-fetcher.ts
  • src/main/zcode/zcode-plan-api-key-store.test.ts
  • src/main/zcode/zcode-plan-api-key-store.ts
  • src/renderer/src/components/settings/ZcodePlanAccountsSection.test.tsx
  • src/renderer/src/components/settings/zcode-plan-usage-windows.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/rate-limits/service/service-full-cycle-preparation.ts
  • src/renderer/src/components/settings/zcode-plan-usage-windows.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Comment thread src/main/zcode/zcode-plan-api-key-store.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues from this delta itself — the follow-up commit resolves the prior Pullfrog copy bug, hardens the credential store, and adds real coverage. Holding approval only because an open thread from another reviewer on the store's failed-replacement path is still unresolved; please resolve or dismiss it before merge.

Reviewed changes

Read the fix(usage): address review on GLM Coding Plan linking commit (e3b970b) since the prior Pullfrog review, against the full PR diff for context.

  • Hardened the credential store — saveZcodePlanApiKey rejects keys with interior CR/LF, and the plaintext fallback now checks writeSecureFile's hardening result: on failure it removes the file and throws, so an unrestricted credential is neither reported as saved nor left on disk.
  • Stopped cross-account fallback — fetchZcodeRateLimits returns a dedicated The saved GLM Coding Plan API key is unusable error when a supplied plan credential can't be parsed, instead of silently reading a different account's quota from the ZCode CLI config.
  • Detected resolver errors as config changes — the zcode config hash now falls back to zcodePlanConfigResult.error ?? '', mirroring MiniMax, so an undecryptable saved key invalidates the snapshot and resets the visible meter.
  • Fixed the doubled reset copy — ZcodeUsageWindowView formats the reset from the bare formatResetDuration rather than formatResetCountdown, so it renders 5 hours — resets in 47m once.
  • Coverage — new store tests (interior newline, hardening-failure refusal), rewritten fetcher tests for the no-fallback semantics, and a reset-countdown component test; 40 tests pass locally across the four touched suites.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

writeSecureFile has already replaced the credential file when it reports
that restriction failed, so deleting the result also destroyed the user's
previous working key (greptile P1, CodeRabbit major). Capture the previous
envelope before the write and restore it on failure; only a first-time key
with nothing to preserve is removed.
rmSync(keyPath, { force: true })
} else {
try {
writeSecureFile(keyPath, previousEnvelope.toString('utf8'))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Failed restoration exposes API key

If OS encryption is unavailable and file-permission hardening fails during a key replacement, this restoration ignores whether the old key was written securely. An old plaintext key can remain readable under inherited file permissions. If the restoration write throws, the newly entered plaintext key can remain instead, even though the save reports failure. Remove the published file when secure restoration cannot be confirmed.

How this was verified: The credential writer publishes the file before reporting a hardening failure, and this path neither checks the restoration result nor removes the file when restoration throws.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Reviewed the fix(usage): restore the previous key when a plaintext replacement fails commit (d2f13d2) since the prior Pullfrog review, against the full PR diff for context.

  • Restored the previous credential on a failed plaintext replacement — saveZcodePlanApiKey now snapshots the existing envelope before the fallback write and re-publishes it when writeSecureFile reports the new file could not be restricted, instead of deleting the file and leaving cachedZcodePlanApiKey pointing at a credential that no longer exists on disk. rmSync is now reached only when there was no previous envelope.
  • Added a regression test — the store suite asserts the restore write targets the captured previous bytes and that the file is not removed, so it fails against the old delete-and-throw behavior.

This resolves the prior failed-replacement concern that was holding approval. The remaining third-party note on the same path (the restore write's hardening result is not re-checked) is the availability-over-restriction tradeoff the commit documents in-line, and the unrestricted-publish window originates in the first fallback write rather than in this delta — I don't consider it a merge blocker.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Link a GLM Coding Plan API key in AI Provider Accounts

1 participant