Skip to content

fix(claude-desktop): keep applied state consistent across profile edits - #5590

Closed
luvs01 wants to merge 8 commits into
devfrom
stack/desktop-applied-state
Closed

luvs01 wants to merge 8 commits into
devfrom
stack/desktop-applied-state

Conversation

@luvs01

@luvs01 luvs01 commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Consolidate #5337 with the related CLI explanation correction so Claude Desktop's saved settings and applied-state markers describe the configuration actually written. Editing a desired profile clears stale applied markers; a sync that races with another edit or deletion reports that its marker was skipped instead of claiming the newer settings are applied.

  • Preserve server-owned appliedFingerprint/appliedAt only for unchanged profile content, reject client-supplied markers, and persist the fingerprint of the exact Desktop write through the config mutation lock.
  • Compare both desired-profile presence and content before committing that marker. A concurrent profile deletion, whole claudeCode deletion or deletion of an explicitly empty profile stays deleted. An initially absent profile and an unchanged profile still record successful writes normally.
  • Compare default routes by family rather than JSON key insertion order. Reordering the same defaults during sync keeps the applied marker; real selection changes still clear it.
  • Keep a user-owned gateway's managed-provider guard during native fallback; remove it only when the admission credential belongs to OpenCodex.
  • Explain the gateway state captured before apply, including the no-argument CLI path, while preserving the existing gateway pinning and deletion tombstones.
  • Bound unrelated work in the CLI fixtures through existing runtime/catalog test seams and direct fixture placement. The verified apply writers, receipt journal, transition persistence and ACL paths remain real.

Verification

  • Exact head: 9d0999f29ce0dde6f396e9670f720c0dbc779c79, based on dev a4bdc03054d4a449c7762eec9853f04557a06fb4. The clean dev merge at 955d93c preserved all 11 contribution files from tested d933915; 2f61b77 then fixed family-order comparison and added its regression/contract. The current dev refresh leaves all 11 final Desktop contribution files and all proxy sources under src/ unchanged from that tested 2f61b77 head.
  • Hosted test shard 3 failed only on the new-dev provider registry oracle (expected count 11 while its explicit roster had 13). Upstream test: repair Windows-only and roster regressions surfaced by full-platform dev CI #5592 already fixed it, so this refresh incorporates that correction without a duplicate patch. The exact repaired parity test, provider/generated/catalog data and dependency manifests match the separately verified Meta integration run (59 passed, 0 failed); that is shared evidence, not a second Desktop execution. Updated-head hosted CI remains pending.
  • New deletion cases failed against the preceding implementation (3 failures), then passed after the presence guard. bun run test tests/clients/sync-client-integrations.test.ts on pinned Bun 1.4.0 passed 37 tests, 0 failures, 238 assertions, 22.10 seconds, including edited/deleted/empty/initially-absent desired profiles and existing sync contracts.
  • Typecheck, structure SSOT and privacy scan passed after the race correction; the final file-size ratchet passed. Refreshed-head typecheck, structure, privacy and whitespace checks also passed after the dev merge.
  • The source review's defaults-order concern was reproduced in the real sync coordinator: a same-value reorder incorrectly reported a skipped marker. After the family-wise comparison, bun run test -- ./tests/clients/sync-client-integrations.test.ts ./tests/clients/desktop-profile.test.ts passed 55 tests, 0 failures, 653 assertions, 30.71 seconds on pinned Bun 1.4.0. Final typecheck, structure, privacy, file-size ratchet and whitespace checks passed. The earlier 37-test run is separate and not added to this total.
  • Earlier CLI fixture validation reported 40 passed / 0 failed / 258 assertions in 60.8 seconds and a separate explanation file passed 5 / 0. A preceding run of the same CLI content had 38 passed / 2 timeouts with icacls ETIMEDOUT; that failure is retained and the precise OS cause is unconfirmed. A separate diagnostic used a 30-second timeout and is not acceptance evidence. These samples are not combined into one passing run or a claim that all Windows timing failures are solved.
  • Full changed-suite coverage, exact-head hosted CI and independent security review remain incomplete. This PR stays draft; no live Desktop restart, account change or runtime deployment was performed.

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.

luvs01 and others added 6 commits September 23, 2026 03:20
…edits

- Native launch fallback: strip CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST only when the admission credential is ours, so a user-owned gateway keeps its guard instead of losing it while the credential stays.

- Desktop profile rebuilds: carry appliedFingerprint/appliedAt only when the desired profile is unchanged, so a saved edit no longer reports the old on-disk config as applied. The management PUT discards client-supplied markers and restores the server-owned ones only for an identical result.

- ocx sync: persist the fingerprint of the exact Desktop config just written through the config-mutation lock, so the applied marker advances with the bytes instead of going stale.

(cherry picked from commit 3254286)
(cherry picked from commit 9b81802d349220043e8956929bfd545b0927c86a)
…rifted mid-sync

The carried sync stamps the fingerprint of the Desktop bytes it just
wrote onto the persisted profile under the config-mutation lock. If
another writer saved a different desired profile between the Desktop
write and the marker commit, the new profile would claim the old
bytes as applied. The callback now compares the persisted profile's
desired content with the profile the write actually used and skips
the marker when they differ, reporting the skip instead of a clean
change. Unchanged profiles still store the fingerprint.

Co-authored-by: luvs01 <luvs01@users.noreply.github.com>
(cherry picked from commit 4992a7740322636483dc18063c3f7393719b3abc)
(cherry picked from commit 7597bce)
(cherry picked from commit 0abd90ea579c1d59ee7dcb666ef8037a986c21a5)
Four cases sat at the 5s timeout because fixture placement paid for work no assertion inspects: saveConfig's mutation-lock and ACL subprocess per write, and buildClaudeDesktopState's runtime probe plus bundled-catalog subprocess. Place fixture config bytes directly, seed the runtime/bundled caches through the existing test seams, and split the no-arg and --static cases into their own rows. Assertions, flags, ACL hardening on verified paths, and the 5s limit are unchanged.
…profile removal

The sync route compared the persisted desired profile against a normalized empty profile when the desired profile had been deleted between the write and the marker commit, so a concurrent delete or empty-profile write still passed the presence guard and resurrected the profile with a fresh fingerprint. Compare presence first, then content, and decline the marker write when the persisted profile no longer matches what was written.
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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 Sep 22, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 61 / 80

이 PR은 Claude Desktop의 “지금 디스크에 적용된 설정”을 가리키는 표시(appliedFingerprint / appliedAt)가 거짓말하지 않도록 고칩니다. 예전에는 프로필을 다시 만들 때 표시를 무조건 이어 붙여서, 저장한 내용이 바뀌었는데도 “적용됨”으로 남을 수 있었습니다. 이제는 원하는 프로필 내용이 그대로일 때만 표시를 남기고, 내용이 바뀌면 지웁니다. 관리 API는 클라이언트가 보낸 표시를 믿지 않고 서버가 가진 표시만 조건부로 되돌립니다. ocx sync는 Desktop 파일을 쓴 뒤, 잠금 안에서 “지금 저장된 원하는 프로필”이 방금 쓴 것과 같은지(먼저 있는지/없는지, 그다음 내용)를 보고 표시를 찍습니다. 중간에 다른 저장이 프로필을 지우거나 바꾸면 표시를 찍지 않고 “skipped”로 알립니다. 기본값(JSON 키 순서만 바뀐 경우)은 같은 내용으로 보고, 실제로 고른 모델이 바뀌면 다르게 봅니다. 같이 들어온 native 실행 환경 쪽은 OpenCodex가 준 자격증명일 때만 CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST를 지워서, 사용자가 쓰는 게이트웨이 가드가 잘못 빠지지 않게 합니다. CLI는 apply 전에 읽은 설정으로 설명을 남겨, apply 직후 상태가 섞여 보이지 않게 합니다. 테스트는 sync 경합·삭제·키 순서·PUT으로 표시가 지워지는 경우와 CLI fixture 부하를 줄인 경우가 많이 늘었습니다. base는 dev이고, 닫힌 #5337을 이어서 정리한 초안(draft)입니다.

라인 - src/claude/desktop-profile.ts sameProfileContent: family별 defaults 비교는 키 순서에 안 흔들리게 고쳤지만, assignments 값 객체(family/alias)의 키 삽입 순서는 그대로 JSON.stringify에 맡깁니다. 같은 내용인데 필드 순서만 다르면 “다르다”로 나와 표시를 스킵하거나 지울 수 있습니다.
라인 - src/server/management/config-routes.ts sync 결과: Desktop 파일 쓰기는 성공했는데 원하는 프로필이 중간에 바뀌어 표시만 스킵하면 ok: false입니다. 호출 쪽이 “쓰기 실패”로 보고 다시 sync하면, 이미 써 둔 Desktop 바이트와 원하는 프로필이 더 어긋날 여지가 있습니다.
라인 - 같은 파일: written: true여도 fingerprint가 없으면 이제 실패입니다. writer가 항상 fingerprint를 주는지 전제인데, 예전 경로/목(mock)이 fingerprint 없이 true만 주면 sync가 갑자기 깨집니다.
라인 - PR 본문: security checklist 미체크, draft 유지, Windows CLI fixture에서 icacls ETIMEDOUT 타임아웃 이력이 남아 있고 hosted CI·독립 보안 리뷰는 아직 완전하지 않다고 적혀 있습니다.

메인테이너의 판단이 필요한 지점

표시 스킵을 ok: false(실패)로 둘지, “쓰기는 됐지만 표시만 안 함”을 구분하는 상태(예: ok + reason)로 둘지. 그리고 draft를 ready로 올리기 전에 바꾼 테스트 스위트·보안(자격증명/호스트 managed 가드) 확인을 어느 선까지 필수 게이트로 둘지.

너의 추천

방향은 맞고, 표시를 서버 소유로 두고 경합 때 삭제된 프로필을 되살리지 않는 쪽이 맞습니다. nested assignment 키 순서도 family처럼 정규화하거나, 비교를 필드 단위로 바꾸면 defaults 순서 버그와 같은 종류의 재발을 줄입니다. sync 스킵은 호출 계약부터 정한 뒤 문서/테스트를 맞추세요. #5337은 이미 닫혀 있으니 이 PR로 이어서 보고, 중복 carry PR이 생기면 닫으세요. ready 전환 전에는 바꾼 스위트 CI와 managed-by-host/표시 위조 경로만 짧게라도 확인한 뒤 머지하는 편이 안전합니다.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun

Copy link
Copy Markdown
Owner

Carried into #5619 at 9d0999f (same net patch as the earlier 2f61b77) with your authorship kept. Follow-ups apply the same applied-marker compare to the provider-change writer, the client-sync writer and the profile PUT, so a concurrent edit or a newer marker is never overwritten. Closing in favor of #5619. Thank you!

@lidge-jun lidge-jun closed this Sep 23, 2026
lidge-jun added a commit that referenced this pull request Sep 23, 2026
… fixes (#5619)

* fix(cursor): bound capability reads and buffered tool budgets (#5533)

Carries #5533 (and the closed #5233 it consolidates) onto current dev.

Co-authored-by: Epinephrine <luvs01@hanmail.net>

* fix(moonshot): bound normalized tool-schema expansion (#5547)

Carries #5547, which consolidates #5464 and the request-wide inline budget, onto current dev.

Co-authored-by: yeongjunyoo <47925973+yeongjunyoo@users.noreply.github.com>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: Epinephrine <luvs01@hanmail.net>

* fix(moonshot): restore rejected inline budgets and charge nested growth once

A rejected sibling-reference expansion now restores the byte, node and expansion allowances it consumed, and outer growth no longer re-charges nested copies, so later independent expansions in the same request keep their allowance. Documents the provider-driven object type inference as a deliberate tradeoff and rewrites ADR-0355 in English.

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

* feat(reasoning): consolidate replay, opt-in tag parsing, and summary policy (#5566)

Carries #5566, which consolidates #5449, #5205 and #5491, onto current dev. The provider guide keeps the current bridge replay paragraph and adds the inline-tag and summary paragraphs.

Co-authored-by: Joonsuh Park <trckstr4422@gmail.com>
Co-authored-by: Daniel Sjöstrand <16033062+Danielsjostrand1979@users.noreply.github.com>
Co-authored-by: alexph-dev <alexph-dev@users.noreply.github.com>
Co-authored-by: Yum-wu <1172989563@qq.com>

* fix: bound Fernet slot runs, Kiro error-body read, and skill-path line slice (#5310)

Carries #5310 onto current dev. The follow-up commit makes the Fernet run cap fail closed and moves the Kiro regression out of the capped stream suite.

* docs(reasoning): reconcile inline-tag whitespace contract

Interleaved inline-tag parsing preserves answer whitespace; only Kiro single-block mode drops the whitespace after its leading block.

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

* fix(responses): fail closed on Fernet run overflow and keep the Kiro suite under its cap

A slot with more than 64 structurally valid Fernet runs is now treated as unreadable or omitted as a whole, so no unexamined tail reaches the provider as text. The bounded Kiro fallback error-body regression moves byte for byte into a registered sibling file, and the Kiro, Responses and inbound contracts document the new bounds.

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

* fix(reasoning): scan inline think tags with a moving cursor

The parser copied, rescanned and reserved the whole remaining response after every block, so one upstream chunk carrying many short blocks cost quadratic work. It now scans each chunk from an offset and charges the translator budget only for retained carry: undecided leading input or a trailing tag fragment.

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

* fix(reasoning): keep undecided leading whitespace incremental

Before the format was decided, every content delta rebuilt, trimmed and re-reserved the whole leading prefix, so a stream of one-character whitespace deltas cost quadratic work. Leading whitespace is now kept in segments whose bytes are reserved once and joined only when the format is decided or the stream flushes.

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

* fix(meta-muse): consolidate login admission and bounded response handling (#5591)

Carries #5591, which consolidates the closed #5234 and #5432, onto current dev. The provider contract keeps the inline-tag paragraph and adds the Meta Muse admission paragraph.

Co-authored-by: Epinephrine <luvs01@hanmail.net>

* fix(claude-desktop): keep applied state consistent across profile edits (#5590)

Carries #5590, which consolidates the closed #5337, onto current dev.

Co-authored-by: Epinephrine <luvs01@hanmail.net>
Co-authored-by: luvs01 <luvs01@users.noreply.github.com>

* fix(claude-desktop): commit applied markers only over the observed baseline

Both Desktop writers, provider-change auto-apply and client sync, now capture the desired profile and its applied marker before the Desktop write and commit the new marker only if profile presence, content, fingerprint and timestamp are unchanged. A concurrent edit, deletion or newer marker keeps its state and the write reports a skipped marker. The provider-change path no longer saves a whole stale config snapshot.

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

* fix(claude-desktop): commit profile edits against the persisted marker

The Desktop profile PUT built its response from an earlier snapshot and saved that whole snapshot, so a marker committed by another writer during the awaited state build could be replaced by an older one. The edit now commits in one persisted-config mutation that keeps the latest marker for unchanged content and answers 409 when the profile itself changed meanwhile. The Meta Muse overflow test now asserts that the bounded-body limit, not a generic failure, produced the error.

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

* fix(claude-desktop): report an unreadable config separately from an edit conflict

A missing or invalid config now answers 500 with its reason; only a concurrent profile change or exhausted rebase answers 409.

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

* feat(desktop): consolidate consent-based runtime takeover and ownership contracts (#5564)

Carries #5564, which consolidates #5459 and #5457, onto current dev. The review screenshot stays in the pull request description rather than the tree.

Co-authored-by: jun <bitkyc08@gmail.com>
Co-authored-by: sanggyulee <andy53295774@gmail.com>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* fix(desktop): bind takeover stop to the approved runtime and fail closed

Desktop takeover re-resolves ownership immediately before stopping and passes the approved PID, endpoint, config home, CLI version and compatibility token to an opt-in guarded stop. The guard is checked under the ownership mutation lease before any manager or signal stop; the approved PID and endpoint must settle and the service manager must then be proven inactive, otherwise the stop answers approval-changed or manager-still-active and the desktop neither waits for silence nor claims. Unreadable or unparseable stop output is terminal as well. A second unreadable service-state read now blocks takeover, Windows managing-CLI discovery follows PATHEXT with file-only candidates and refuses command-interpreter metacharacters, the claim refusal test uses real sandbox state, and the runtime and desktop contracts record that the claim token is a consistency check rather than consent proof. Plain ocx stop is unchanged.

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

* fix(desktop): keep plain stop entry points and format the takeover changes

Desktop exit keeps its plain runtime_stop::run entry while takeover uses run_approved, AttachPlan::Ask no longer carries an unread field, the Rust changes follow rustfmt, the plain CLI stop path keeps its literal outcome return, the stop source oracles follow the reader and outcome union that now include the two guarded refusals, and the runtime contract fits its 600-line budget.

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

* fix(desktop): run takeover seam tests without tokio macros and harden manager and shim checks

The two async takeover seam tests now run on the shell runtime already used by the crate instead of tokio test macros, which this crate does not enable. Windows command-shim probes refuse command-interpreter metacharacters in every recorded argument as well as the executable, and the guarded stop re-inspects the service manager identity immediately before the manager command, answering approval-changed without stopping if it moved.

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

* fix(responses): keep effort-based reasoning visible after routing

Final-route normalization recomputed hideThinkingSummary without the validated active-effort condition, so routed Chat and Kiro requests with an active effort and an omitted summary still hid raw reasoning. It now uses the same predicate as the parser; explicit "none" and requests without an active effort stay hidden.

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

* fix(service): match the running CLI case-insensitively only on Windows

On case-sensitive filesystems a PATH executable that differs only in case is a different file, so it must get its own version probe instead of reporting the running CLI version.

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

* fix(meta-muse): require the dashboard session for manual login codes

The manual-code continuation now applies the same dashboard-session admission as the login start, so a management token cannot advance a pending Meta Muse login.

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

* fix(reasoning): reserve the joined leading-whitespace copy

Joining retained leading whitespace allocated a second copy outside the translator budget; the join is now reserved first and released once the segments are cleared.

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

* fix(service): skip CLI probes for an absent runtime and treat failed systemd units as stopped

Resolve no longer spawns managing-CLI version probes when no runtime is live, since takeover is only offered for a live runtime. A systemd unit reported failed with no main PID is stopped, so a guarded stop that leaves it failed succeeds and a leftover failed unit does not block takeover.

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

* fix(service): keep failed systemd units fail-closed and assess takeover only for a live runtime in tests

systemd can report failed before an automatic restart, so failed with no main PID is again treated as unknown rather than stopped. The resolve contract tests that assert ownership and takeover fields now use a live runtime, matching the skip of managing-CLI probes when no runtime is live.

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

---------

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Co-authored-by: Epinephrine <luvs01@hanmail.net>
Co-authored-by: yeongjunyoo <47925973+yeongjunyoo@users.noreply.github.com>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: Joonsuh Park <trckstr4422@gmail.com>
Co-authored-by: Daniel Sjöstrand <16033062+Danielsjostrand1979@users.noreply.github.com>
Co-authored-by: alexph-dev <alexph-dev@users.noreply.github.com>
Co-authored-by: Yum-wu <1172989563@qq.com>
Co-authored-by: luvs01 <luvs01@users.noreply.github.com>
Co-authored-by: sanggyulee <andy53295774@gmail.com>
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