Repository navigation
Conversation
Adapt the reviewed GLM accounts contribution to current main, retain Antigravity behavior, guard late credential results, expose storage protection, and redact quota errors. Co-authored-by: Luchong <lu740528977@gmail.com>
5 of 6 tasks
… tests Apply the independently reviewed Accounts correction from697284a without the v2 adapter commits. Preserve the saved-key store and serialized write behavior.
nwparker
marked this pull request as ready for review
October 2, 2026 11:49
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta from e22d5a5 to f5ead0e — the hosted-CI fixture repair. Both touched files are test-only; no production code changed.
- Added a
zcodePlanCredentials.getStatusstub to theAccountsPanelifetime fixture returning{ apiKeyConfigured: false, zcodeCliConfigured: false, apiKeyProtection: null }, which matchesZcodePlanCredentialsStatusinsrc/shared/zcode-plan-sites.ts:27. - Added
'zcodePlanCredentials'to the expected web preload API inventory, in the exact position production composes it (src/renderer/src/web/web-preload-api.ts:111).
The substantive adapter, renderer setup section, i18n and fixtures are unchanged since the prior reviewed head (e22d5a5a05).
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe changes add GLM Coding Plan credential discovery for Z.AI and BigModel, secure desktop key storage, and IPC operations to read, save, and clear credentials. Rate-limit refresh now uses plan or Zcode CLI credentials and discards results that no longer match the active credentials. Accounts settings display plan usage and provide credential and CLI setup controls. Web settings scope the plan site to the active environment. The changes also update macOS Electron test-environment isolation. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A failed key replacement can leave the new credential readable by others with disk access. Remove the failed plaintext replacement before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Selected-account validation and restricted request destinations limit exposure. However, a rejected plaintext key replacement can leave the attempted credential on disk if permission enforcement and restoration fail. The risk is conditional on the device configuration and filesystem access, rather than an established remote attack. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 7.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 50 files. (25 skipped: 12 unsupported, 13 over the file limit.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c56c86c8-8803-4e1a-b4fa-1341cda1ef69
📒 Files selected for processing (75)
src/main/ipc/register-core-handlers/register-core-handlers.test.tssrc/main/ipc/register-core-handlers/register-core-handlers.tssrc/main/ipc/zcode-plan-credentials.test.tssrc/main/ipc/zcode-plan-credentials.tssrc/main/rate-limits/service-zcode-usage.test.tssrc/main/rate-limits/service/service-account-refresh.tssrc/main/rate-limits/service/service-configuration.tssrc/main/rate-limits/service/service-fetch-policy.tssrc/main/rate-limits/service/service-fetch-targets.tssrc/main/rate-limits/service/service-full-cycle-application.tssrc/main/rate-limits/service/service-full-cycle-preparation.tssrc/main/rate-limits/service/service-state.tssrc/main/rate-limits/service/service-types.tssrc/main/rate-limits/zcode-usage-credentials.tssrc/main/rate-limits/zcode-usage-fetcher.test.tssrc/main/rate-limits/zcode-usage-fetcher.tssrc/main/runtime/runtime-client-settings.tssrc/main/runtime/runtime-store-contract.tssrc/main/startup/main-process-account-services.tssrc/main/zcode/fixtures/v2-upstream/bigmodel/credentials.jsonsrc/main/zcode/fixtures/v2-upstream/bigmodel/provider_config.jsonsrc/main/zcode/fixtures/v2-upstream/fallback-host/credentials.jsonsrc/main/zcode/fixtures/v2-upstream/fallback-host/provider_config.jsonsrc/main/zcode/fixtures/v2-upstream/provenance.jsonsrc/main/zcode/fixtures/v2-upstream/zai-other/credentials.jsonsrc/main/zcode/fixtures/v2-upstream/zai-other/provider_config.jsonsrc/main/zcode/fixtures/v2-upstream/zai/credentials.jsonsrc/main/zcode/fixtures/v2-upstream/zai/provider_config.jsonsrc/main/zcode/zcode-plan-api-key-store.test.tssrc/main/zcode/zcode-plan-api-key-store.tssrc/main/zcode/zcode-v2-personal-overlay-raw-json.test.tssrc/main/zcode/zcode-v2-personal-overlay.tssrc/main/zcode/zcode-v2-plan-credentials.test.tssrc/main/zcode/zcode-v2-plan-credentials.tssrc/preload/api-types.tssrc/preload/api/agent-account-api.tssrc/preload/api/zcode-plan-credentials-bridge.tssrc/preload/index.tssrc/renderer/src/components/settings/AccountsPane.section-lifetime.test.tsxsrc/renderer/src/components/settings/AccountsPane.tsxsrc/renderer/src/components/settings/ZcodeCliSetupSection.test.tsxsrc/renderer/src/components/settings/ZcodeCliSetupSection.tsxsrc/renderer/src/components/settings/ZcodePlanAccountsSection.test.tsxsrc/renderer/src/components/settings/ZcodePlanAccountsSection.tsxsrc/renderer/src/components/settings/accounts-search.test.tssrc/renderer/src/components/settings/accounts-search.tssrc/renderer/src/components/settings/use-zcode-plan-credentials.test.tsxsrc/renderer/src/components/settings/use-zcode-plan-credentials.tssrc/renderer/src/components/settings/zcode-plan-usage-windows.tsxsrc/renderer/src/components/status-bar/status-bar-provider-visibility.test.tssrc/renderer/src/components/status-bar/status-bar-provider-visibility.tssrc/renderer/src/components/status-bar/usage-provider-settings-target.test.tssrc/renderer/src/components/status-bar/usage-provider-settings-target.tssrc/renderer/src/components/status-bar/use-status-bar-controller.tssrc/renderer/src/i18n/en-runtime-required.jsonsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/web/preload-api/web-agent-accounts-api.tssrc/renderer/src/web/preload-api/web-preferences-store.tssrc/renderer/src/web/preload-api/web-runtime-session.tssrc/renderer/src/web/preload-api/web-zcode-plan-site.tssrc/renderer/src/web/web-preload-api-composition.test.tssrc/renderer/src/web/web-preload-api.tssrc/renderer/src/web/web-zcode-plan-settings.test.tssrc/shared/default-global-settings.tssrc/shared/global-settings-types.tssrc/shared/node-bounded-file-reader-sync.test.tssrc/shared/node-bounded-file-reader.tssrc/shared/orca-workspace-layout.tssrc/shared/rate-limit-state-factory.tssrc/shared/rate-limit-types.tssrc/shared/rpc-contract/client-settings-params.tssrc/shared/zcode-plan-sites.tstests/e2e/helpers/electron-home-isolation.tstests/e2e/helpers/electron-home-isolation.unit.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
Comment on lines
+122
to
+138
| // Why: an unrestricted plaintext credential must never be reported as saved. | ||
| if (!wroteRestricted) { | ||
| if (!previousEnvelope) { | ||
| rmSync(keyPath, { force: true }) | ||
| } else { | ||
| try { | ||
| writeSecureFile(keyPath, previousEnvelope.toString('utf8')) | ||
| } catch { | ||
| // Why: restriction is failing device-wide; the restored bytes keep the | ||
| // previous credential available instead of deleting it, and the thrown | ||
| // save error still tells the user the store is not secure. | ||
| } | ||
| } | ||
| throw new Error('GLM Coding Plan API key could not be stored securely on this device') | ||
| } | ||
| cachedZcodePlanApiKey = trimmed | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,190p' src/shared/secure-file.ts
sed -n '1,45p' src/main/zcode/zcode-plan-api-key-store.ts
rg -n 'applySecurePathRestriction|writeSecureFileSync|writeSecureFile' src/shared src/main/zcode/zcode-plan-api-key-store.tsRepository: stablyai/orca
Length of output: 11425
🏁 Script executed:
printf '%s\n' '--- secure-file restriction dispatch ---'
sed -n '220,310p' src/shared/secure-file.ts
printf '%s\n' '--- restriction helpers ---'
rg -n -C 8 'export function restrictWindowsPathSync|function restrictWindowsPathSync|bestEffortRestrictWindowsPath|restrict.*Path|chmodSync|process\.platform' src/shared/secure-path-windows-acl.ts
printf '%s\n' '--- store save flow ---'
sed -n '75,150p' src/main/zcode/zcode-plan-api-key-store.ts
printf '%s\n' '--- relevant writer tests ---'
sed -n '280,360p' src/shared/secure-file.test.ts
printf '%s\n' '--- relevant store tests ---'
sed -n '110,195p' src/main/zcode/zcode-plan-api-key-store.test.ts
printf '%s\n' '--- diff scope ---'
git diff --stat 3793c58abd43aa7272b47dc072241af3bc5e0774 bb34682361ee9a49dec2d15c16f0b3a326d94345 -- src/shared/secure-file.ts src/shared/secure-path-windows-acl.ts src/main/zcode/zcode-plan-api-key-store.ts src/main/zcode/zcode-plan-api-key-store.test.tsRepository: stablyai/orca
Length of output: 16428
🏁 Script executed:
printf '%s\n' '--- synchronous Windows restriction implementation ---'
sed -n '1,115p' src/shared/secure-path-windows-acl.ts
sed -n '198,290p' src/shared/secure-path-windows-acl.ts
printf '%s\n' '--- Windows ACL policy, plan and failure tests ---'
rg -n -C 5 'inherited|DACL|SID|failed|cannot|false|planFor|restrictWindowsPathSync|writeFileSync|renameSync|ENOSPC|EACCES|disk' src/shared/secure-path-windows-acl.test.ts src/shared/secure-file.test.ts
printf '%s\n' '--- exact writer catch and calls ---'
sed -n '123,164p' src/shared/secure-file.ts
printf '%s\n' '--- store path/restore lines ---'
nl -ba src/main/zcode/zcode-plan-api-key-store.ts | sed -n '100,141p'
printf '%s\n' '--- ACL source line references ---'
nl -ba src/shared/secure-path-windows-acl.ts | sed -n '35,85p;198,275p'Repository: stablyai/orca
Length of output: 41909
🏁 Script executed:
printf '%s\n' '--- save API key callers ---'
rg -n -C 3 'saveZcodePlanApiKey\\(' src
printf '%s\n' '--- ACL plan and SID resolution ---'
rg -n -C 8 'function planFor|function currentUser|whoami|parse.*SID|buildAclPlan' src/shared/secure-path-windows-acl.ts
printf '%s\n' '--- false-on-ACL-failure write test ---'
nl -ba src/shared/secure-file.test.ts | sed -n '655,700p'
printf '%s\n' '--- secure writer contract ---'
nl -ba src/shared/secure-file.ts | sed -n '115,160p'Repository: stablyai/orca
Length of output: 8267
🏁 Script executed:
rg -n -F -C 4 'saveZcodePlanApiKey' srcRepository: stablyai/orca
Length of output: 9510
Remove the failed plaintext replacement when rollback fails.
When encryption is unavailable and both Windows ACL checks fail, writeSecureFile still publishes the plaintext file and returns false. Windows ignores mode: 0o600, so that file keeps the parent directory’s inherited ACL. If the restore’s temporary-file write then fails before rename, the store’s catch leaves the new envelope at keyPath; on a path whose inherited ACL grants other principals read access, the rejected key remains exposed. Remove the path only if it still contains the attempted plaintext envelope, so an error after a successful restore does not delete the previous credential.
Suggested fix
- const wroteRestricted = writeSecureFile(
- keyPath,
- encodeApiKeyEnvelope('plaintext', Buffer.from(trimmed, 'utf8'))
- )
+ const plaintextEnvelope = encodeApiKeyEnvelope('plaintext', Buffer.from(trimmed, 'utf8'))
+ const plaintextEnvelopeBytes = Buffer.from(plaintextEnvelope, 'utf8')
+ const wroteRestricted = writeSecureFile(keyPath, plaintextEnvelope)
...
} catch {
- // Why: restriction is failing device-wide; the restored bytes keep the
- // previous credential available instead of deleting it, and the thrown
- // save error still tells the user the store is not secure.
+ try {
+ const currentEnvelope = readFileSync(keyPath)
+ if (
+ currentEnvelope.equals(plaintextEnvelopeBytes) &&
+ !previousEnvelope?.equals(currentEnvelope)
+ ) {
+ rmSync(keyPath, { force: true })
+ }
+ } catch {
+ // Keep the save failure; cleanup is best-effort.
+ }
}
Reconcile the seven squash-parent conflicts while retaining the selected v2 credential reader, GLM saved-key and host protections, and pinned main changes. Remove a rejected plaintext replacement only while its envelope is still owned by the failed save, preserving a restored previous key and other writers. Co-authored-by: Luchong <lu740528977@gmail.com> Co-authored-by: guanbear <guanbear@users.noreply.github.com>
Check failed-write ownership after the secure writer hardens staged rollback bytes and immediately before its rename. Cancel only this guarded publication when the target changed, keeping existing best-effort callers and rollback cleanup behavior. Cover the observed schedule using real writes and a competing sibling-file rename. Co-authored-by: Luchong <lu740528977@gmail.com> Co-authored-by: guanbear <guanbear@users.noreply.github.com>
Merge the pinned current main into the accepted ZCode changes. Keep the synchronous regular-file checks and the asynchronous nonblocking, bounded, cancellable reads, resolving their shared import conflict with both filesystem stat functions. Co-authored-by: Luchong <lu740528977@gmail.com> Co-authored-by: guanbear <guanbear@users.noreply.github.com>
Compose the reviewed ZCode credential compatibility with fixed main 752871b. Preserve both ZCode setup and notification-host locale sections. Retain Luchong's GLM contribution #24618 and GuanBear's ZCode quota contribution #23520. Co-authored-by: Luchong <lu740528977@gmail.com> Co-authored-by: guanbear <guanbear@users.noreply.github.com>
Use the existing ClaudePromptRegistry required by the current child-work drain API. Preserve the original retention and ownership assertions.
Check the canonical runtime host stamped by current snapshots, then seed the legacy missing-host tab through the existing fixture/store before the original collision and notification assertions.
Compose main ce07786 with the existing ZCode v2 reader and GLM account work, retaining both fixture assertions and original history. Keep credential rollback ownership checks and disposable E2E credential roots without changing intentional production overrides. Original GLM account contribution: Luchong (#24618). Original ZCode usage contribution: GuanBear (#23520). Co-authored-by: Luchong <lu740528977@gmail.com> Co-authored-by: guanbear <guanbear@users.noreply.github.com>
nwparker
marked this pull request as draft
October 4, 2026 06:57
nwparker
changed the title
This was referenced Oct 5, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI5
Orca recognizes the Coding Plan selected by migrated ZCode credentials and avoids showing another account’s quota. A rejected replacement key preserves the previous or newer credential that Orca can still identify as owned by that write.
What Changed
The read-only v2 adapter resolves the selected Z.AI or BigModel identity and key from the upstream credential store. A present unsupported or broken v2 selection prevents stale legacy fallback; an Orca-saved key retains priority. Quota results are checked against their credential identity after requests and sibling-provider waits.
Rollback checks the rejected envelope before restoring or deleting it, and the existing secure-file writer checks ownership again after staging permissions. The E2E launcher strips inherited ZCode credential-root overrides case-insensitively and rejects launch/extra-environment overrides before creating directories. Intentional production overrides still work. Accounts exposes the existing ZCode sign-in terminal separately from the quota key.
This head merges main
ce0778626618bb9a5197209f5189190f2d51545cthrough normal hooks. Original history and both fixture assertions are retained; only the overlapping fixture corrections needed conflict resolution.Why
Reuses the existing quota service, credential store, permission writer, launcher isolation policy, and setup terminal. The small compatibility reader avoids new private upstream dependencies and writes to the user's CLI credential store. Ownership checks preserve competing publications observed during staging without claiming an atomic filesystem compare-and-swap.
Linked Issue
Addresses the credential safety work tracked in #24963. Broader #21757 and #8476 remain open. Builds on Luchong’s GLM account contribution #24618 and GuanBear’s ZCode usage contribution #23520.
Visual Proof
These are retained historical author captures from earlier synthetic-profile runs, not new rendered validation of this head. Their availability was not reverified in this publication task.
Before migrated credential recognition:
After migrated credential recognition:
Historical saved-key coexistence:
Testing
pnpm tc,pnpm run check:code-quality:changed ce0778626618bb9a5197209f5189190f2d51545c, changed-file formatting, and diff checks passed. Normal installed Husky ran oxlint, React Doctor, and formatting.Review
Execution-host ownership remains with the existing credential/account services and terminal routing. Folder workspaces do not require Git metadata. No new status store, stream opcode, or required remote field is introduced; protocol 3 / minimum 2 remains unchanged. Existing SecretStore, data-host settings, encryption, durable-write options, and runtime-checked Windows ACL behavior are retained.
Agent skill upstream boundary
Notes
Final head:
feef7f7bd84f35d49c07b303df3215e0fb8bee92, treef63d4420d7410f593c07e563fe3b0176633006e5; ordered parents are the reviewede44878e612d04dee5c919afead9303c7dddeef65and fetched maince0778626618bb9a5197209f5189190f2d51545c.Earlier publication/setup failures and the historical damaged dependency graph (381 files / 16 links) remain recorded failures. This task repaired required host dependencies using the unchanged frozen-lockfile install and repository native scripts; tracked manifests and lockfile were unchanged. The first test attempt retained one fixture-environment failure; the final isolated run passed. The pre-merge quality attempt retained two incoming-main mobile diagnostics, while the final main-based gate passed with zero findings.
Native Windows file ACL evidence is recorded below. Native safeStorage, provider authentication and vault behavior, the store’s asynchronous parent-directory ACL and ACL-syscall-failure rollback, Linux, SSH/WSL, live paired execution, rendered UI, packaging, and paid-provider behavior remain unverified. The ownership predicate and rename are separate operations, so a competing external writer in that remaining gap is not protected by an atomic transaction. Fresh hosted CI and independent review of this exact head are required before any merge.
Credit: Luchong / parkavenue9639 (#24618), with verified
lu740528977@gmail.comcredit; GuanBear (#23520), retaining verifiedguanbear@users.noreply.github.comcoauthor credit and original author objects. No original author was rewritten.Checklist
Native Windows credential proof — 4 October 2026
On Windows 10.0.26200 / x64 / Node 24.18.0, this exact head and tree passed 29 tests across three files with no skips. The disposable checkout exercised the production ZCode Coding Plan key store, secure-file writer, SID lookup and real
icacls/Windows DACL operations. Electron safeStorage was deliberately unavailable and homedir pointed to a temporary directory; keys were fake and no provider request or app launch occurred. The four-case store proof helper was an untracked validation artifact, not a test shipped by this PR.The published plaintext key file had exactly three explicit full-control entries: current user, SYSTEM and Administrators; no inherited entries or Everyone. Input validation rejecting a newline retained the prior file and cached key. A separate production-writer
shouldPublish: () => falsecase retained the prior target and cleaned staging. These cases do not exercise the store’s ACL-syscall-failure rollback after publication. The generic native directory suite passed separately; it does not prove the ZCode store’s asynchronous parent hardening.Exact exported helper: 5,568 bytes, SHA-256
ed2b216fb3c0cf4b7e18f3e7cfd7a6112cafd9656efd5bb799577546dda2926a. Final raw log: 5,716 bytes, SHA-25645af8b5736f88bcbdf225c6ac7e93166e3c3a55f24e0b10355afa92021a6ed8e. Source/log association is documented by the disposable worktree and timestamps; the log does not embed a source hash. Final tracked source, manifest and lockfile bytes were verified at the stated head after the recorded lockfile restoration. Earlier setup, parser and lint diagnostics remain recorded.This is evidence for the ZCode store at
.orca/zcode-plan-api-key.enc, not for every GLM credential store or closure of #24963. Independent source review is clear for this pinned composition. Hosted checks observed at 09:27 UTC reported 15 successes and 18 skips; skipped lanes are not runtime proof. This remains a draft awaiting the broader validation above and a merge decision.Additional native Windows encryption and reader proof (2026-10-04)
This updates the earlier native safeStorage gap; the other Notes limitations remain as stated.
On Windows 11 Pro / Windows_NT 10.0.26200, win32 x64, the production modules at
feef7f7bd84f35d49c07b303df3215e0fb8bee92were exercised in fresh background Electron processes with fake credentials and disposable child-onlyUSERPROFILE/app-data roots. Global user settings were unchanged.safeStoragereported encryption available. Production save wrote an encrypted envelope with a 58-byte payload; the file contained no plaintext fake key. Actualicaclsoutput listed the test Windows user, SYSTEM and Administrators with full control.sealed. The production clear call then completed successfully; post-clear file absence was not explicitly checked.okfor runtime-generated Node AES-256-GCM synthetic identity/key fields. This proves the fixture reader path, without exercising a real ZCode vault, account login or provider request.The three process runs exited successfully. The earlier malformed-path ACL-inspection attempt remains a failed attempt; the later run used the SystemRoot-resolved
icaclsexecutable. The earlier 29-test native plaintext/ACL proof and its recorded history remain unchanged.Asynchronous production parent-directory ACL remains unasserted, and actual post-publication ACL-syscall failure rollback was not induced. This adds bounded Windows encryption/read evidence; authenticated generation, rendered Orca, other platforms/remote hosts and generic GLM account or credential-store coverage remain unverified. It does not establish closure of broader issue #24963.