Repository navigation
fix(identity): keep federated sessions and agent grants in one keychain item - #2225
Conversation
…in item store the owner session and every agent grant (and retirement record) in one federated-session bundle per profile and configuration, so the keychain prompts once instead of once per agent. each session or grant update compares the whole item under the account lock, now serialized in-process so unrelated agents no longer race for it. session removal keeps grants; the item is deleted once empty. a legacy session item is read as a bundle and rewritten on the next write; a legacy grant item moves in on read only from a loaded bundle that lacks it, and is deleted after the bundle is written and read back. deleting a grant removes its legacy item first. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Kalvin Chau <[email protected]>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Two correctness issues need fixing before this merges. Everything else looks sound: the session slot compare-and-swap, the order of the legacy grant migration, writing the refusal marker before removal, and the full sign-out wipe through the index. The native macOS Keychain run noted in the description is still needed to confirm a single prompt and the migration of existing items.
| Some(Zeroizing::new(next)) | ||
| }; | ||
| let next = next.as_deref().map(Vec::as_slice); | ||
| if next.is_some_and(|next| next.len() > MAX_BUNDLE) { |
There was a problem hiding this comment.
🤖 Windows can't hold the bundle. This bundles every grant into one item on every OS and allows up to 1 MiB, but on Windows compare_session writes through keyring's set_password (crates/credential-store/src/lib.rs session_write). keyring 3.6.3's Windows backend rejects anything over 2,560 bytes of UTF-16 (CRED_MAX_CREDENTIAL_BLOB_SIZE). A session record plus three small grants with 64-character tokens, each of which fits as its own item today, comes to about 3,030 bytes. Once a bundle hits that size on Windows:
- migrating the next existing grant fails on read, so agents that work today stop working after the upgrade
- new agent grants and pending-retirement writes fail with
Secure storage is unavailable
The one-prompt-per-item problem is macOS-only, and the native agent key bundle (agent-bundle-v1) this follows is gated to target_os = "macos" for the same reason. Suggest bundling only on macOS, keeping separate items on Windows (and Linux, unless you want the bundle there too), and adding a test that covers the Windows size limit.
There was a problem hiding this comment.
🤖 Fixed in 60a2f2d. Only macOS bundles now (store::BUNDLED = cfg!(target_os = "macos"), matching agent-bundle-v1). On Windows and Linux the session item stays the bare record and each grant or retirement record keeps its own item and index, as before this PR. There's nothing to migrate there.
Unbundled save refuses an item that holds grants rather than silently dropping them. Added without_bundles_each_record_keeps_an_item_within_the_windows_limit, which uses a session plus 8 grants and asserts:
- there are 9 items
- every item fits in 2,560 bytes of UTF-16
- the session item is the exact record
- delete, remove and the wipe clear the items and indexes
I've dispatched the Windows native validation job (cargo test/clippy on windows-2025) on this head.
| return Err("storage-unavailable"); | ||
| } | ||
| items::compare(&self.root, &self.account, current, next)?; | ||
| if items::read(&self.root, &self.account)? |
There was a problem hiding this comment.
🤖 This read-back can report failure after the replacement is already written. Before this change, Store::compare only errored when nothing had been written. finish_login (mod.rs around line 858) relies on that: on any compare error it calls Prepared::abandon, which clears the pending retirement this login just recorded (agent/mod.rs around line 72). When the account changes from A to B, a read-back here that is denied, fails, or doesn't match leaves B's session stored, A's grants stored, and no retirement marker. If B's session then expires or is refused before the next launch, restore removes it without calling preparing_session (mod.rs around line 570), and A's grants are admitted again.
Keep the read-back for the grant migration, since grant() needs it to confirm the bundle holds the grant before it deletes the legacy item. The session path just needs to tell "nothing was written" apart from "written but unconfirmed". For example, save could return a distinct outcome when the read-back fails after items::compare succeeds, and finish_login would keep the retirement in that case instead of calling abandon. Please add a regression test that drives finish_login through a failure after the replacement is written, then restores with B's session expired or refused.
There was a problem hiding this comment.
🤖 Fixed in 60a2f2d by removing the failure mode instead of adding a new outcome. save no longer reads back after compare_session. Session and grant writes go back to the pre-PR contract: an error means nothing was written. That keeps finish_login's abandon on compare errors correct without changes to mod.rs. The read-back stays only in the legacy grant migration, before the old item is deleted.
I didn't add the finish_login regression test. With the read-back gone, no store path returns an error after the replacement is written, so there's no "written but unconfirmed" state to drive. Any such error from the credential-store compare_session itself is unchanged by this PR.
There was a problem hiding this comment.
[P2] Preserve successful grant commits when legacy cleanup fails
The session read-back issue is fixed, but grant writes can still return an error after persisting: save succeeds before forget_item runs. If cleanup fails during enrolled's initial reservation commit, its stopped = Some(revision) remains stored and enrollment never starts; later attempts return STOPPED until another sign-in. Could cleanup be tracked separately from the successful commit, retaining its index for retry?
Posted by Codex (gpt-6.1-sol) on behalf of kylez.
There was a problem hiding this comment.
🤖 Agreed, fixed in 28d655a. Once the bundle commit succeeds, cleanup of the old item is best effort (retire_item), both for a grant write and for a migration on read. If the cleanup fails, the old item and its index stay. The bundle copy overrides them on reads. The next write or delete of that key retries the cleanup, and the boot erase owner still walks the index. An explicit delete still removes the old item first and reports a failure, so a failed delete can't bring the grant back.
Added a_committed_grant_stands_when_cleaning_up_its_old_item_fails. It makes the old item's index removal fail, then asserts:
- the reservation write returns
Okand is stored - the migration returns the grant
- the index is kept for a retry
- an explicit delete still reports the cleanup failure
…te contract windows credentials hold at most 2,560 bytes of utf-16, so a bundle of a session and a few grants fails there. only macos prompts once per item, so only macos bundles; other platforms keep one item per record, as before. drop the post-write read-back from the session and grant write paths: a login abandons its pending retirement on any compare error, so an error must still mean nothing was written. the legacy grant migration keeps its read-back before deleting the old item. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Kalvin Chau <[email protected]>
fs2 try_lock_exclusive surfaces windows lock violations as an uncategorized error, so a held account lock was reported as storage-unavailable. use std file try_lock, whose wouldblock covers every os, as the credential-store and sign-out locks already do. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Kalvin Chau <[email protected]>
kazemek21
left a comment
There was a problem hiding this comment.
Kyle reviewed this PR and the Nexus review-tracker notes. Approved with the non-blocking grant-cleanup comment.
Posted by Codex (gpt-6.1-sol) on behalf of kylez.
a grant write or migration returned an error after the bundle commit when deleting the pre-bundle item failed, so enrollment treated a persisted reservation as failed. cleanup after a commit is now best effort: the old item and its index stay for the next write or delete of that key, or the boot erase owner, and the bundle copy overrides it meanwhile. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Kalvin Chau <[email protected]>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
Reviewed head 28d655a339290bf543e4da49c0e0d6dd67857b55 against base 1a48ed12665ce6864a6d795c2129826a535cfedd (merge base e8b7165b1739321869d8fecd53d791b8de497bd4).
One P2 finding: the forward migration can lose or ignore a completed enrollment from a still-running pre-bundle development instance (inline). The current source addresses the earlier Windows item-size, post-write session read-back, and committed-grant cleanup findings. No additional actionable findings in the other reviewed session, refusal, retirement and full sign-out paths.
Independent coverage: Rocket—storage/migration/lifecycle; Mantis—correctness/security/concurrency; Groot—session and recovery flows; Star Lord—integration and public PR material.
Source-only: no tests, builds or app execution performed. The single CI snapshot showed CI required successful; Windows native validation was skipped in that snapshot. Real macOS Keychain prompt count and migration acceptance remain unverified, as disclosed in the PR.
| let written = self.save(raw.as_deref().map(Vec::as_slice), &mut bundle)?; | ||
| if items::read(&self.root, &self.account)?.as_deref() != written.as_deref() { | ||
| return Err("storage-corrupt"); | ||
| } | ||
| self.retire_item(key); |
There was a problem hiding this comment.
[P2] Make closing older macOS dev instances an upgrade prerequisite
An already-running pre-bundle macOS development instance writes each grant under its separate grant-key lock, while this migration holds only the owner-session lock. If the old instance has persisted an enrollment reservation and is awaiting the adapter, restoring the same profile/configuration in the new instance can copy that reservation here before the old enrollment commits its token. retire_item can then delete the newer token; a legacy write after deletion is also ignored because the bundled reservation wins. After the old instance exits, the upgraded instance still reports the agent stopped for the same session revision (agent/mod.rs:819–828) despite successful enrollment.
Parallel dev worktrees are explicitly supported (docs/contributing.md:101–103), and their shared boot locks do not exclude this transition. Please make closing all older debug instances before the first bundled launch an explicit upgrade prerequisite, or enforce that exclusion through existing instance ownership. The forward-only/downgrade warning currently does not mention this in-flight data-loss case. The impact is a recoverable stopped agent (new sign-in or explicit agent deletion resets it), not a credential disclosure. This is source-inferred; native reproduction remains unperformed.
There was a problem hiding this comment.
🤖 Made it an explicit prerequisite in 3c58c5b rather than enforcing it. A pre-bundle build takes no lock the new build can see, so instance ownership can only cover copies that already know about it. This matches the agents folder migration in docs/agents.md ("close older Buzz copies before the first upgraded launch").
docs/identity.mdnow describes the macOS bundle and says to close older Buzz copies, including other debug worktrees, before the first bundled launch. It also names the failure: an older copy still running can write a grant the bundle no longer reads, which leaves that agent stopped.- The forward-only section of the PR description lists the same prerequisite and in-flight case.
- Release builds already run under the single-instance lock, and an upgrade replaces the app, so only parallel dev builds are exposed.
describe the macos bundle layout and require closing pre-bundle debug copies before the first bundled launch. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Kalvin Chau <[email protected]>
…xecution-support-for-native-buzz-33ea2bea * origin/main: (74 commits) feat(host): harden byte lifecycle and add protected image upload (#2199) Updated onboarding (#2183) perf(relay): read each signed roster once in large channels (#2244) AIDA-2346: Keep spoilers hidden in reminder previews (#2261) fix: AIDA-2371 Keep sidebar selection pill aligned during resize (#2182) fix(relay): skip background rereads of profiles a read found absent (#2147) fix: prevent startup logo selection (#1823) fix(agents): send sign-in evidence with agent Git credentials (#2240) perf(mentions): stop scanning every member on each mention keystroke (#2246) fix(attachments): send image dimensions so previews don't shift (#2262) feat(shortcuts): add Slack-style hotkeys (#2170) perf(plugins): share host request clients across calls (#2243) fix(identity): keep federated sessions and agent grants in one keychain item (#2225) fix(profiles): evict the least recently requested profile (#2255) fix(unread): repair unread from the recovery tick only every 15 minutes (#2252) AIDA-2374: Use readable labels in chat titles (#2185) perf(profiles): keep verified names checked through a planned renewal (#2234) feat(plugins): let a plugin ask Buzz to review an install (#2059) fix(identity): sign FI builds in at first run through the native session (FOUNDATION: App.tsx, services.ts) (#2249) Place profile verification beside the verified name (#2224) ... Signed-off-by: Daniel Rouleau <[email protected]>
* commit 'c8e882165': (50 commits) Scope execution targets by harness session Read durable execution state before admission feat(host): harden byte lifecycle and add protected image upload (#2199) Updated onboarding (#2183) perf(relay): read each signed roster once in large channels (#2244) AIDA-2346: Keep spoilers hidden in reminder previews (#2261) fix: AIDA-2371 Keep sidebar selection pill aligned during resize (#2182) fix(relay): skip background rereads of profiles a read found absent (#2147) fix: prevent startup logo selection (#1823) fix(agents): send sign-in evidence with agent Git credentials (#2240) perf(mentions): stop scanning every member on each mention keystroke (#2246) fix(attachments): send image dimensions so previews don't shift (#2262) feat(shortcuts): add Slack-style hotkeys (#2170) perf(plugins): share host request clients across calls (#2243) fix(identity): keep federated sessions and agent grants in one keychain item (#2225) fix(profiles): evict the least recently requested profile (#2255) fix(unread): repair unread from the recovery tick only every 15 minutes (#2252) AIDA-2374: Use readable labels in chat titles (#2185) perf(profiles): keep verified names checked through a planned renewal (#2234) feat(plugins): let a plugin ask Buzz to review an install (#2059) ... Signed-off-by: Daniel Rouleau <[email protected]> Co-authored-by: Codex <[email protected]>
…ection * origin/main: (39 commits) Authorize local apps to use Buzz MCP tools (block#2211) feat(bestie): choose the private channels Bestie can read (block#2036) Polish message actions, spacing, and membership notices (block#2258) test(unread): prevent startup catch-up in receipt reserve journey (block#2274) test(search): wait for rejected join clearance to settle (block#2278) test(reactions): provide stable emoji media for icon rendering (block#2279) feat(profiles): reserve profile room for an open channel's roster (block#2269) feat(host): harden byte lifecycle and add protected image upload (block#2199) Updated onboarding (block#2183) perf(relay): read each signed roster once in large channels (block#2244) AIDA-2346: Keep spoilers hidden in reminder previews (block#2261) fix: AIDA-2371 Keep sidebar selection pill aligned during resize (block#2182) fix(relay): skip background rereads of profiles a read found absent (block#2147) fix: prevent startup logo selection (block#1823) fix(agents): send sign-in evidence with agent Git credentials (block#2240) perf(mentions): stop scanning every member on each mention keystroke (block#2246) fix(attachments): send image dimensions so previews don't shift (block#2262) feat(shortcuts): add Slack-style hotkeys (block#2170) perf(plugins): share host request clients across calls (block#2243) fix(identity): keep federated sessions and agent grants in one keychain item (block#2225) ... # Conflicts: # src/features/messages/ThreadPanel.tsx
…in item (block#2225) ## Problem Federated identity (block#1424) stores the owner session and each agent grant as separate `xyz.block.buzz.desktop.federated-session[.debug]` keychain items, so macOS prompts roughly once per agent. ## Existing pattern This follows the pattern native agent keys already use: `agent-bundle-v1` in `crates/agent-controller/src/credentials/bundle.rs`. Like that bundle, this one: - keeps one item per service on macOS - applies to macOS only (the `target_os = "macos"` gating in `credentials/platform.rs` and `bundle_lock.rs`; here, `store::BUNDLED`), since only macOS prompts once per item and a Windows credential holds at most 2,560 bytes of UTF-16 - migrates an individual item on read, only when the bundle loaded successfully and lacks it - deletes the individual item before the bundle on explicit deletion Unlike `agent-bundle-v1`, this one deletes the old item once the bundle holding it is written and read back, because we're only migrating forward. Windows and Linux keep one item per record, unchanged. ## Change - On macOS, one item per profile and configuration (the existing session account) holds `{"bundle":1,"session":"<record bytes>","grants":{"<grant key>":"<grant bytes or retiring>"}}`. Each slot keeps its exact bytes. - Every session or grant update reads the whole item, checks its own slot (the session must equal `expected`, as before), then runs `compare_session` from the old item to the new one, all under the account lock. As before, an error means nothing was written, which `finish_login` relies on when it abandons a pending retirement. Only the legacy grant migration reads back, before it deletes the old item. A process-wide mutex now sits in front of that lock, so concurrent agents queue instead of getting `storage-busy`. Another Buzz instance holding the lock is still reported as busy. - Removing the session (sign-out or refusal) clears only the session slot. Grants already outlive the session, and the item is deleted once it's empty. Refusal markers, the `{account}.record` index and `remove_all` are unchanged. The wipe still walks the index entries of legacy grants that haven't migrated. - Forward migration only: - A legacy session item, a bare `Record`, is read as a bundle holding only that session and rewritten on the next write. - A legacy grant item is migrated on read only when the bundle loaded successfully and lacks that key. The old item and its index are deleted after the bundle is written and read back. - A grant write saves the bundle first. - A grant deletion removes the legacy item first, so a failed save can't bring it back on a later read. - Anything else is `storage-corrupt` and is never overwritten. - Older builds can't read a bundle once it's written. - Upgrade prerequisite, as for the agents folder migration in `docs/agents.md`: close older Buzz copies, including other debug worktrees, before the first bundled launch. A pre-bundle copy still running writes grants to the old per-key items under their own locks, which the bundle doesn't see once it holds that key, so the agent stays stopped. Release builds already hold the single-instance lock, so this affects only parallel dev builds. Documented in `docs/identity.md`. - `Store`, `GrantStore` and the agent retirement logic are unchanged. `SecureGrantStore` now carries the bundle account. ## Tests In test builds, the secure items live in memory for temp roots that opt in. Every other root stays unavailable, as before, so no test touches OS credentials. New store tests cover: - one item holding the session and multiple grants - a stale session compare being refused - session removal keeping grants - deleting the item once it's empty, and the wipe - in-place read and replacement of a legacy session item, with corrupt items left untouched - grant migration only from a loaded bundle, cleanup of the old item and index, the bundle copy winning over a reappearing old item, and write/delete retiring the old item - concurrent in-process grant writes, plus busy reported for another instance - the unbundled layout used on Windows and Linux: one item per record, each within the Windows 2,560-byte UTF-16 limit, and the session item stays the bare record The two existing lock-contention tests now hold the lock the way another instance would. `cargo test --lib` (macOS): 803 passed. Clippy is clean. Linux runs in PR CI, and Windows runs as a manual dispatch of the Windows native validation job. Not yet done: a native run against the real macOS Keychain to confirm a single prompt and migration of existing items. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Kalvin Chau <[email protected]> Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-authored-by: Claude Opus 5.5 <[email protected]>
Problem
Federated identity (#1424) stores the owner session and each agent grant as separate
xyz.block.buzz.desktop.federated-session[.debug]keychain items, so macOS prompts roughly once per agent.Existing pattern
This follows the pattern native agent keys already use:
agent-bundle-v1incrates/agent-controller/src/credentials/bundle.rs. Like that bundle, this one:target_os = "macos"gating incredentials/platform.rsandbundle_lock.rs; here,store::BUNDLED), since only macOS prompts once per item and a Windows credential holds at most 2,560 bytes of UTF-16Unlike
agent-bundle-v1, this one deletes the old item once the bundle holding it is written and read back, because we're only migrating forward. Windows and Linux keep one item per record, unchanged.Change
{"bundle":1,"session":"<record bytes>","grants":{"<grant key>":"<grant bytes or retiring>"}}. Each slot keeps its exact bytes.expected, as before), then runscompare_sessionfrom the old item to the new one, all under the account lock. As before, an error means nothing was written, whichfinish_loginrelies on when it abandons a pending retirement. Only the legacy grant migration reads back, before it deletes the old item. A process-wide mutex now sits in front of that lock, so concurrent agents queue instead of gettingstorage-busy. Another Buzz instance holding the lock is still reported as busy.{account}.recordindex andremove_allare unchanged. The wipe still walks the index entries of legacy grants that haven't migrated.Record, is read as a bundle holding only that session and rewritten on the next write.storage-corruptand is never overwritten.docs/agents.md: close older Buzz copies, including other debug worktrees, before the first bundled launch. A pre-bundle copy still running writes grants to the old per-key items under their own locks, which the bundle doesn't see once it holds that key, so the agent stays stopped. Release builds already hold the single-instance lock, so this affects only parallel dev builds. Documented indocs/identity.md.Store,GrantStoreand the agent retirement logic are unchanged.SecureGrantStorenow carries the bundle account.Tests
In test builds, the secure items live in memory for temp roots that opt in. Every other root stays unavailable, as before, so no test touches OS credentials. New store tests cover:
The two existing lock-contention tests now hold the lock the way another instance would.
cargo test --lib(macOS): 803 passed. Clippy is clean. Linux runs in PR CI, and Windows runs as a manual dispatch of the Windows native validation job.Not yet done: a native run against the real macOS Keychain to confirm a single prompt and migration of existing items.
🤖 Generated with Claude Code