Repository navigation
fix(accounts): remove a rejected GLM key when restore fails - #24964
Conversation
A failed plaintext replacement can publish the new key before file restriction is confirmed. If restoring the previous key fails before it replaces that file, delete only the rejected envelope so a completed restore is left intact.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Delete a rejected plaintext envelope on failed restore —
saveZcodePlanApiKeynow re-reads the credential file from a newremoveRejectedPlaintextEnvelopehelper and removes it only when the bytes still match the rejected replacement, so a restore that already published the previous key is preserved. - Two tests pin both restore-failure edges — one asserts deletion when the restore throws before publishing, the other asserts the file is kept when the restore throws after publishing the previous envelope.
The discriminator is sound: writeSecureFile (src/shared/secure-file.ts:146) renames the temp file into place before returning, so a false return always means the attempted bytes were published, a pre-rename throw leaves those bytes, and a post-rename throw leaves the previous bytes. The byte comparison distinguishes exactly those two cases, and readFileSync failures fall through to a no-op. Test 1 fails without the fix; test 2 guards against an unconditional delete.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The reviewed changes have no established actionable merge risk. The possible competing-publication races lack a demonstrated production trigger, and the current rollback guard preserves an identical previous key. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review details
Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 10 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Issue #24963requires removal of the rejected plaintext envelope only while it still owns the file.rollbackPlaintextEnvelopecompares the current bytes before recovery and removes the file only whe…Out of Scope Changes check The store and secure-file changes, regression tests, native Windows tests, and credential contract workflow support the credential-safety objective. The renderer dependency and fixture changes are ide… Title check The title clearly and concisely describes the main change: removing a rejected GLM key when rollback restoration fails. Description check The description follows the required template and explains the behavior change, rationale, linked issue, testing, platform coverage, security review, and checklist status. It also documents known veri…
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
-
🛠️ Fix failing CI checks 💡
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving 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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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:
4a1b637c-bf39-4488-b769-633b0b60c6c6
📒 Files selected for processing (2)
src/main/zcode/zcode-plan-api-key-store.test.tssrc/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; 9 remain after this review.
A restored plaintext envelope can match the rejected replacement byte for byte. Do not delete that file, since it is the previous key rather than a new secret.
|
Could you review this when you have a moment, @nwparker? It is a small follow-up to the GLM Coding Plan credential save path from #24618: a failed plaintext replacement no longer leaves a different rejected key on disk, and an identical previous key is preserved. I cannot add reviewers directly from my fork. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Guard the rejected-key cleanup against an identical previous key — the restore
catchnow skipsremoveRejectedPlaintextEnvelopewhen the captured previous envelope is byte-identical to the attempted replacement, so re-saving the same plaintext key while encryption is unavailable no longer deletes the only key. The same guard covers the post-publish-throw variant of that case, which is indistinguishable by bytes and would otherwise delete a just-restored key. - Regression test for the identical-key edge — the new test fails on the pre-guard code (the previous envelope equals the attempted bytes, so cleanup deleted the file) and passes now.
This closes the edge CodeRabbit raised without changing the shared writer: the helper still re-reads and deletes only when the published bytes match the rejected attempt, and the new guard handles the one case where that comparison would misidentify the previous key as the rejected one.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
Final reviewed head: Recommend merge first, before #24674. All hosted PR Checks now pass, including five test shards, relay integration, and both packaging jobs. Local store/writer verification passes 84 tests, plus typecheck and changed-code quality. The original authored commits and failure assertions are retained. The maintainer follow-up adds ownership checks around rollback publication and real disposable-file race regressions. The remaining non-atomic comparison/rename gap and native validation limits are documented in the body. |
2 participants
ELI5
A failed replacement of a GLM Coding Plan key now removes the rejected key when it is still on disk. It preserves a previous key that was restored and a newer key published by another writer.
What Changed
Keep @parkavenue9639’s original fix and authored history, merge current main, and strengthen the rollback ownership check. Compare the current file with the rejected envelope before restoration or removal. The existing secure-file writer accepts an optional check immediately after staging permissions, so restoring an old key does not overwrite a competing publication observed during staging.
Retain the author’s failure-edge assertions and add real disposable-file tests for failed restoration before and after publication, identical keys, missing files, and competing writes during permission staging. Other secure-file callers retain their existing behavior.
Why
The original report and fix are valid. Deleting unconditionally loses restored or newer credentials; restoring unconditionally can overwrite a newer credential. Reusing the existing writer permits a second ownership check without another credential store. The check and rename remain separate operations, so this does not claim an atomic filesystem transaction.
Linked Issue
Includes the separately reviewable lint prerequisite #27326. Merge that first; its identical cleanup is composed here so the final head can pass required checks.
Fixes #24963
Credential discovery (#24674) and sign-in setup (#27321) are now independent PRs without this storage change. Merge this focused safety repair first; the original author’s commits remain here.
Visual Proof
N/A — failure cleanup changes credential-file bytes, with no visible UI change.
Testing
84 tests pass across six store/secure-file suites, with one Windows-only fsync test skipped on macOS arm64.
pnpm tc, the changed-code quality gate against current main, formatting, diff checks, and normal commit hooks pass.Added an isolated macOS/Linux/Windows CI matrix and three native Windows cases. The Windows cases use the real production permission implementation and actual files; injected failures cover restoration before publication, restoration after publication, and a competing key arriving during permission staging. They assert real file and parent-directory hardening succeeded and no staged files remain. Local macOS runs pass 79 targeted tests; the three Windows-native cases correctly skip here. Typecheck passes. The native contract matrix passes on final head
77a7b5ec7552across macOS, Linux, and Windows. Windows runs all three native ACL cases, with 80 tests passed and two POSIX-only skips. Native verification. The standard Windows CI selection list and Windows boundary-test command now also register this test, as required by the repository-wide registration contract. All 31 registration checks pass locally. Hosted checks are rerunning after this configuration fix; older green checks are not claimed as proof of the new head. Real OS denial and competing external processes remain unverified.Review
The execution host still owns its credential file. No provider request, personal credential read, IPC secret, new setting, required remote field, or workspace assumption is added. Plaintext saves continue reporting failure when the file could not be restricted. Unreadable bytes do not establish ownership and are left untouched.
Agent skill upstream boundary
Notes
Risk reduction: storage is isolated from discovery and UI, real-filesystem failure cases cover ownership and cleanup, and native Windows permission verification now runs on the exact PR head. Low merge risk is conditional on the new matrix and broader hosted checks passing. The comparison and rename remain separate operations; no atomic transaction is claimed.
Credit: Lu Chong / @parkavenue9639 remains the primary author of the original contribution. The maintainer follow-up reuses the stronger rollback implementation previously prepared in #24674. The original author’s AI disclosure: Claude implemented the initial change under their direction and they reviewed it.
Checklist