Skip to content

fix(claude): verify Desktop backup and optional metadata reads - #7084

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:codex/fix-desktop-backup-metadata-20261013
Closed

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:codex/fix-desktop-backup-metadata-20261013

Conversation

@luvs01

@luvs01 luvs01 commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Desktop gateway apply now verifies regular source files for backup creation and metadata-failure rollback, preserving the original backup bytes through the existing atomic writer.
  • Optional metadata/profile reads distinguish missing entries from dangling symlinks and propagate read failures before publishing a profile. A missing profile no longer adopts a stale backup.
  • Addresses the two post-merge review findings on fix(claude): reject non-regular Desktop configuration reads #7036: backup/rollback reads and dangling metadata. This branch contains only the follow-up fix, regression coverage, test registration, and current behavior documentation.

Verification

Head cbd07c11c0b56830bc9ef178150cf02c78e69c25, based on dev 9b31677c4704215e8f3db1c321a38b36b40062a9. One commit, seven files (+218/-12).

Runtime Bun 1.4.2; repository test runner Bun 1.4.3. Tests use the normal preload/home protections and real fixture SQLite lifecycle locks.

  • bun run typecheck, bun run structure:check, bun run privacy:scan, and git diff --check: passed. Privacy scope is the materialized sparse checkout, including every changed file.
  • An explicit strict typecheck of the new test file is not clean: it reports one error in unchanged scripts/test-temp-lock.ts:78 (SyncSubprocess.signal is absent from the Bun type). The repository typecheck above covers src/; no new-test-file diagnostic was reported. Typechecking the unchanged tests/helpers/remove-tree.ts alone produces the same single error.
  • bun run build:gui: passed. Docs frozen-lockfile install and ASTRO_TELEMETRY_DISABLED=1 bun run build: passed, 577 pages and 81,200 internal links checked.
  • bun scripts/test.ts --parallel=1 --timeout 60000 tests/clients/desktop-3p-write-files.test.ts: 9 new cases passed, including real writer backup and rollback FIFO swaps in bounded child processes, repeated dangling-metadata refusal, exact backup bytes, stale backup handling, read errors, and normal publication/rollback controls. With only the two runtime files restored to base 9b31677c4704215e8f3db1c321a38b36b40062a9, these tests produced 3 passes / 6 failures; both FIFO cases reached the child timeout.
  • Related Desktop, layout, and file-size checks: 120 passed / 4 failed across 9 files after restoring a test inventory document omitted by sparse checkout and rerunning that one failed layout check. All new regressions and the existing 3P writer/removal/guard, remote-store, layout, and file-size cases passed; the four picker failures are described below.
  • bun scripts/test.ts --changed=9b31677c4704215e8f3db1c321a38b36b40062a9 --parallel=2 --bail=1 --timeout 60000: selected 1,321 of 2,319 test files; stopped after 106 files with 2,190 passes / 13 failures in the existing GitHub Copilot bearer/origin snapshot suite (67.2 seconds). This broader run is not green.
Related regression command
bun scripts/test.ts --parallel=1 --timeout 60000 \
  tests/clients/desktop-3p-write-files.test.ts \
  tests/clients/desktop-3p.test.ts \
  tests/clients/desktop-3p-removal.test.ts \
  tests/clients/desktop-3p-guard.test.ts \
  tests/clients/desktop-remote-store.test.ts \
  tests/claude-integration/claude-desktop-picker-profile.test.ts \
  tests/test-layout.test.ts tests/test-layout-tooling.test.ts \
  tests/ci-workflows/file-size-ratchet.test.ts

The initial invocation also included a nonexistent refresh-only filename, which selected no tests and is not claimed as coverage. The expanded test typecheck used bun x tsc --ignoreConfig --noEmit --target ESNext --module ESNext --moduleResolution bundler --resolveJsonModule --types bun-types --strict --esModuleInterop --skipLibCheck, targeting either the new regression file or tests/helpers/remove-tree.ts.

The missing-inventory check was rerun with --test-name-pattern='fixture histogram' tests/test-layout-tooling.test.ts: 1 pass, 0 fail, 24 filtered out. The 120/4 count combines these results without recounting passed cases.

The full suite and native macOS/Windows validation were not run in this shared Linux sandbox. Full-suite execution is disproportionate to this scoped Draft with existing environment failures; the focused regressions are the local behavior evidence. Four existing picker-profile failures also reproduce on the unchanged base (4 passes / 4 failures); they remain failures, not passing coverage. No guard was disabled to reach the writer. Independent security review remains outstanding; author validation is not independent approval. The PR remains Draft pending independent review.

Exact-head hosted verification (snapshot at 2026-10-13 22:33:20 UTC, before verification-only description edits): head cbd07c11c0b56830bc9ef178150cf02c78e69c25 has 21 successful check runs, 8 conditionally skipped, and no failed or pending checks. Cross-platform CI and its aggregate ci job completed successfully on attempt 1; all four Linux test shards passed. Catalog async contracts, React Doctor, and the PR policy workflows also succeeded. Skipped native jobs are not validation evidence. No rerun or manual dispatch was requested. These results do not erase the local failures or establish independent security approval; this PR remains Draft.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes
    • Claude Desktop configuration updates now stop before publishing a profile when configuration or metadata files are unreadable or unsafe, rather than proceeding with incomplete data.
    • Backups and rollbacks now use verified configuration files and preserve their exact contents. If a rollback cannot be completed, the update reports failure without claiming the previous profile was restored.

@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: c2e86c67-d0ac-4fc9-a3e5-c238a62f638f
📥 Commits

Reviewing files that changed from the base of the PR and between 9b31677 and cbd07c1.

📒 Files selected for processing: 7
  • docs-site/src/content/docs/guides/claude-code.md
  • scripts/test-layout/layout-clients.json
  • src/claude/desktop-3p-library.ts
  • src/claude/desktop-3p.ts
  • structure/clients/claude-desktop-config-reads.md
  • tests/clients/desktop-3p-write-files.test.ts
  • tests/fixtures/test-layout-expected.json

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

📝 Walkthrough

Claude Desktop configuration reads now distinguish missing files from other read failures. Config replacement backs up verified bytes and restores them through atomic writes. Tests and documentation cover unsafe paths and failed publication or rollback.

Changes: Claude Desktop configuration safety

Layer / File(s) Summary
Optional configuration reads
src/claude/desktop-3p-library.ts:87-99, 134-136, tests/clients/desktop-3p-write-files.test.ts:42-89
Metadata and profile foreign-key reads return empty defaults only when the file is missing. Other read errors propagate. Tests cover dangling symlinks and read errors that prevent profile publication.
Verified backup and rollback
src/claude/desktop-3p.ts:856, 873-881, tests/clients/desktop-3p-write-files.test.ts:91-176, docs-site/src/content/docs/guides/claude-code.md:274-277, structure/clients/claude-desktop-config-reads.md:4, 21-27, scripts/test-layout/layout-clients.json:44, tests/fixtures/test-layout-expected.json:581
Replacement backs up existing config bytes through the atomic writer. Rollback restores backup bytes through that writer. Tests cover exact-byte backups, absent profiles, failed metadata publication, and FIFO swaps. The guide, configuration-read documentation, and test-layout mappings describe or include this coverage.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to cbd07

No actionable merge-blocking issue was established; the change is mergeable after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (4 skipped: 4… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title accurately summarizes the main changes: verified Desktop backup handling and optional metadata reads. It is concise, specific, and directly related to the changes in src/claude/desktop-3p.ts…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (4 skipped: 4 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · 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.

❤️ Share

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

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 13, 2026
@luvs01
luvs01 marked this pull request as ready for review October 14, 2026 00:59
@lidge-jun

Copy link
Copy Markdown
Owner

Thanks @luvs01 — this branch doesn't allow maintainer edits, so I carried it as #7129 with your commit cherry-picked unchanged and a Co-authored-by trailer.

One follow-up there: the writer still reread the profile for the backup and reread the .bak for rollback. With the .bak swapped to a FIFO, rollback refused and left the new profile beside the old metadata. #7129 reads the profile once and reuses those verified bytes for foreign keys, backup, and rollback (rollback now always restores), and removes a profile the failed apply had just created. I'll close this one once #7129 lands.

@lidge-jun

Copy link
Copy Markdown
Owner

Superseded by #7129 (your commit cherry-picked unchanged, Co-authored-by). Thank you!

@lidge-jun lidge-jun closed this Oct 15, 2026
lidge-jun added a commit that referenced this pull request Oct 15, 2026
…7084) (#7129)

* fix(claude): verify Desktop backup and optional metadata reads

* fix(claude): back up and roll back Desktop profiles from one verified read

The gateway writer read the profile for foreign-key preservation, reread it
for the backup, and reread the backup for rollback. A .bak swapped between
replacement and rollback made rollback refuse, leaving the new profile beside
the old metadata. One verified snapshot now feeds all three, rollback restores
those bytes, and a profile created by the failed apply is removed so retries
cannot accumulate unreferenced profiles.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(claude): refuse stale Desktop snapshots and retain rollback errors

Revalidate verified profile bytes before backup and in the no-follow publication hook, refusing drift without retries. Keep metadata publication errors primary when profile restoration or cleanup also fails, and report both failures. Cover concurrent edits and rollback failures with regression tests.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* fix(claude): roll back a Desktop profile only while it still holds our bytes

When metadata publication fails, restore or cleanup now re-reads the profile through the verified reader and acts only if it still equals the bytes this apply published, using the same compare-and-swap helper that guards publication (also enforced before the restore rename). A profile another writer changed is preserved and the metadata error is reported with the rollback skipped.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

* docs(claude): describe ownership-aware Desktop profile rollback

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>

---------

Co-authored-by: Epinephrine <27862058+luvs01@users.noreply.github.com>
Co-authored-by: GOAT <goat@localhost>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants