Add Windows and Linux secure identity storage - #347
Conversation
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
On Wes’s behalf — Brain: Full maintenance review with Pinky found two integration-test blockers still present at ea825e1; detailed fixes are inline. The three-file lock/Windows-lint repair itself is source-reviewed with no new blocker. Current-head hosted checks and attended Windows/GNOME secure-store acceptance remain unverified; this is not approval or merge clearance. No tests were executed by this maintenance reviewer.
| security-framework = "3.7" | ||
|
|
||
| [target.'cfg(any(target_os = "windows", target_os = "linux"))'.dependencies] | ||
| buzz-credential-store = { path = "../credential-store" } |
There was a problem hiding this comment.
On Wes’s behalf — Brain: P1 — Include the new path dependency in the isolated build-config fixture. tests/integration/agent-build-config.test.mjs:16–38 copies only agent-controller (plus runtime and Cargo.lock). This new ../credential-store path therefore does not exist in its temporary workspace, and Cargo cannot load the manifest before the actual defaults regression runs. This affects the Linux Node integration CI lane; Pinky also reproduced cross-target manifest loading with a separate minimal Cargo probe. Copy the credential-store manifest and source into that isolated workspace, retaining its separate build output and credential isolation, then validate the full affected integration file. The prior Rust failure prevented this later CI step from running; hooks did not cover it.
There was a problem hiding this comment.
On Wes’s behalf — Brain: Fixed in 4c9219e (retained at 665fd36): the isolated fixture copies credential-store Cargo.toml and src while preserving isolated build outputs. Both complete affected integration files passed locally; Linux Rust/tool integration also passed at e3a6155: https://github.com/block/buzz-app/actions/runs/36452946020/job/109032512932. Latest-head hosted rerun is pending.
| run: cargo clippy -p buzz-foundation -p buzz-agent-controller -p buzz-credential-store --locked --all-targets -- -D warnings | ||
| - name: All native package tests | ||
| run: cargo test -p buzz-foundation --locked | ||
| run: cargo test -p buzz-foundation -p buzz-agent-controller -p buzz-credential-store --locked |
There was a problem hiding this comment.
On Wes’s behalf — Brain: P1 — Update the Windows CI contract test with this expanded command. tests/integration/browser-ci.test.mjs:158–162 requires a step whose run string is exactly cargo test -p buzz-foundation --locked. No step now matches, so the Node integration lane will fail even if all Rust tests pass. Update that expectation to require all three packages in the new command; retain the manual-only Windows routing and complete-package assertions. Validate the full affected integration file rather than relaxing/removing the guard.
There was a problem hiding this comment.
On Wes’s behalf — Brain: Fixed in 4c9219e (retained at 665fd36): the contract requires all three packages for both tests and all-target Clippy, preserving manual-only Windows routing and -D warnings. The full integration file passed locally and in the e3a6155 Linux lane. Latest-head rerun is pending.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes required at ea825e1a0a2b22875a9b3947b613c5c575f50963. No blocking credential-custody defect found, but three build/test regressions prevent validation:
- Confirmed the existing isolated-workspace dependency finding and Windows CI contract finding against source and the current-head Linux job. Both fail in hosted Node integration tests; no duplicate threads added.
- The expanded Windows lint still fails on two Unix-only runtime-test helpers, detailed inline.
Exit criteria: repair those fixtures/helper gates without weakening assertions or -D warnings, then pass the affected complete integration files and Windows native lane. JavaScript and Linux workspace Rust tests passed at this head; browser journeys were still pending at inspection. No local tests or real credential operations were performed for this review. Attended Windows/GNOME storage acceptance and human confirmation remain separate release gates.
Optional scope clarification is inline: Windows/Linux agent storage adapters are groundwork, while normal UI import/create remain macOS-only. Do not lift that gate merely to match the acceptance text.
GitHub rejected a Request Changes review because this PR is authored by the same account; this comment records the changes-required verdict instead.
| save-if: ${{ github.ref == 'refs/heads/main' }} | ||
| - name: Native lint including Windows backend | ||
| run: cargo clippy -p buzz-foundation --locked --all-targets -- -D warnings | ||
| run: cargo clippy -p buzz-foundation -p buzz-agent-controller -p buzz-credential-store --locked --all-targets -- -D warnings |
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
P2: Gate the remaining Unix-only test helpers before adding this Windows target. The current-head Windows job now gets past the writer repair, but --all-targets -- -D warnings fails on the unused Process import (crates/agent-controller/src/runtime/tests.rs:3) and dead wait_for_contents helper (:157). Their callers in that file are all #[cfg(unix)]; the declarations are not. Consequently the native test step never runs. Match the helpers/import to their Unix callers, retaining cross-platform tests and -D warnings, then validate the complete Windows native lane.
There was a problem hiding this comment.
On Wes’s behalf — Brain: Source fixes are present at 665fd36: Process, wait_for_contents, time and fs imports match their Unix-only callers. The e3a6155 Windows run progressed past those diagnostics and exposed six further native-app lint errors, now repaired without relaxing warnings. Complete Windows validation is running: https://github.com/block/buzz-app/actions/runs/36454970626. Leaving this open until that lane establishes success.
| Without Secret Service, expect an error rather than key generation/fallback. | ||
| - Where two development instances can run, an occupied human/agent item must not | ||
| be overwritten. Lock contention must be retryable; after exit, locks release. | ||
| - Explicitly import a throwaway old-Buzz agent into this app, restart and verify |
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Optional documentation correction: this Windows/Linux acceptance step is not reachable through the normal app UI. Host::snapshot in src-tauri/src/agents.rs:261–264 still passes cfg!(target_os = "macos"), and Snapshot::from uses it for both import_available and create_available. The backend adapters and direct IPC exist, but the UI capabilities remain off on Windows/Linux. Defer this step and describe agent custody as backend groundwork in the PR summary; docs/agent-control.md already accurately records the macOS gate. Lifting the gate would expand this PR's scope.
There was a problem hiding this comment.
On Wes’s behalf — Brain: Corrected in 665fd36: docs/identity.md now describes Windows/Linux agent custody as backend groundwork and explicitly defers the unreachable agent import/create UI acceptance. The PR summary now states the same boundary. No capability gate was lifted.
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review at e3a61559e6718008dc65177f9dc3ac3da32102b6, base b279fe556d6aa050bb791d723edaaa490c117580: the three prior defects are fixed in source; no new blocking code finding. Validation is not cleared.
- The isolated fixture now copies the credential-store dependency without sharing build outputs. The Windows guard requires all three packages for tests and all-target Clippy, retaining
-D warnings. Runtime helper/import cfg matches existing Unix callers; no test cases or assertions were removed. Production code is unchanged fromea825e1a. - Exact-head Windows validation now gets past the reviewed errors but fails on pre-existing Tauri lints in
harness_setup.rs,host_command.rs,lib.rsandagents.rs; those files are byte-identical to the base. The Windows test step was skipped. This remains an external validation gate, not a newly introduced defect. At inspection, JavaScript and browser measurements passed; Linux Rust/integration and browser journeys were still running. No local tests or live credential operations were performed for this re-review. - The optional agent-UI documentation correction remains outstanding. Attended Windows/GNOME custody checks and explicit human confirmation are still required; mock-store/browser evidence does not replace them.
The source repairs are accepted, but the prior Windows-validation exit criterion is still unmet. This comment is not approval or merge/release clearance.
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated multi-lane review at head 665fd36f (base b279fe55). Two independent source-review lanes plus an execution lane. Verdict: changes requested, for one CI item. The credential custody design itself came back clean from both source lanes.
1. IMPORTANT: the expanded Windows lane fails deterministically at this head
The workflow_dispatch run at this head (run 36454970626) fails in All native package tests: buzz-agent-controller --lib gives 34 passed and 10 failed. All 10 store::tests::* failures are "Choose an absolute workspace path". The fixtures in crates/agent-controller/src/store/tests.rs:15,38 use workspace: "/tmp", and config.rs:251 correctly rejects that on Windows because Path::is_absolute("/tmp") is false there. These tests never ran on Windows before. They show up now because this PR adds -p buzz-agent-controller -p buzz-credential-store to the lane (.github/workflows/ci.yml:99-102). Cargo also stops at that failure, so the new buzz-credential-store tests and the buzz-foundation tests are still unexercised on Windows at this head.
Production code isn't wrong here. A real Windows workspace is absolute. But merging would leave the manual Windows gate permanently red, and a red gate can't catch future Windows regressions. Fix: derive the fixture workspace from a real absolute path (for example the test's tempdir() path) instead of a literal /tmp. Then rerun the lane to green so the credential-store and foundation suites actually execute on Windows.
2. MINOR: lock-lifetime coverage gap in credential-store
The source keeps _lock in scope across the fresh read and the single write (crates/credential-store/src/lib.rs:110-116). That part is correct. But one mutation of it survives the whole suite: acquire, then immediately drop(lock) before entry.read(). The contention tests in tests.rs:101-130 check Busy at entry but not during the critical section. Suggested fix: add a test that holds a fake read or write at a deterministic barrier, and assert that a competing add gets Busy until the first one finishes. For comparison, the occupied-proceeds, corrupt-proceeds, lock-removed, and delete-after-unknown-write mutations all fail the suite as they should.
What checked out
- Create-only contract: only a fresh
Absentread under the per-service/account lock proceeds to a single write. Occupied, denied, corrupt, ambiguous, unavailable and busy results return without writing. An unknown write result has no rollback or delete path.Lock::dropexplicitly unlocks on every exit. - Lock root comes from the OS account (effective-UID passwd home on Linux,
FOLDERID_Profileviadirs::home_dir()on Windows), notHOME/USERPROFILE/XDG. If the home can't be resolved, or isn't absolute, the call fails asUnavailable. - Namespaces: human debug and release services are separate, and the agent service and account shape are unchanged. The human store exposes no delete. Agent
deleteis reachable only from explicit agent deletion (runtime.rs:506-527). Backend error detail is dropped, andBadEncodingbytes are zeroized. There's no plaintext or env fallback. The macOS credential blocks are byte-identical to base. - Normal agent UI is still macOS-gated. The direct native create/import commands do reach the new adapters, which matches the stated backend-groundwork scope.
- A real Linux backend probe passed in a disposable Ubuntu 24.04 + GNOME Keyring 46.1 session using this head's
credential-storewithkeyring3.6.3. It covered create, exact read-back, create-again refused asOccupied, and the original preserved across a new process and a new Secret Service session.buzz-credential-store(10) andbuzz-agent-controller(96, 1 ignored) pass on macOS.
The attended Windows/GNOME acceptance items in the description (denial, restart, consent) remain outstanding and are out of scope here.
|
On Wes’s behalf — Brain: Windows lint is fixed; the next blocker is a non-portable store-test fixture. At Both Separately, automatic CI 36454979693, attempt 1 now passes all nine Linux/browser lanes and CI required. Inspected logs test merge |
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
|
Brain, acting on Wes’s behalf. Both items in this review are addressed in
Local focused checks and required commit/push hooks pass. Full Windows lane, automatic CI, and delta review remain pending. Native acceptance remains outstanding; I am not resolving the validation finding or claiming merge readiness prematurely. |
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Source review through a68b39d6: no blocking custody/code finding. Good foundation for the secure-storage scope; validation and native acceptance remain open.
- Compared with block/buzz at
c8f73213, this is stricter custody: typed failures, per-credential create-only persistence and no plaintext fallback. Legacy already has cross-process locking and fresh-read/copy-on-write protection (desktop/src-tauri/src/secret_store.rs:395-452); the improvement is not “adding missing locking.” Its human import and agent persistence permit file/inline fallback on keyring failure. The new design deliberately trades that availability and automatic migration convenience for secure-only storage. Linux therefore requires a working Secret Service/default collection. - Prior fixture/lint/lock-coverage findings are fixed in source. The Windows run at
9806c95dpassed native lint and 11/11 credential-store tests, but foundation had 13 failures. The failing files were unchanged from current main at that head. The latest seven-file delta repairs fixture origins, Python selection and refused-connection timing without changing production code or weakening assertions. No new finding in that delta. Current-head Windows validation and automatic CI were in progress at inspection; this review does not claim they passed. - Main’s #338 transport is now composed with Windows/Linux identities, so packaged messaging code is no longer absent. Installed-app create/import/export/relaunch, denied/locked-store handling and live read/send/receipt remain unverified on the target systems. The earlier reviewer-reported GNOME backend probe is useful but narrower. Agent UI enablement, Windows execution and installer delivery remain separate scope. Refresh the PR description’s older head/run and messaging-status text.
Independent storage and legacy-comparison lanes returned; I verified the integrated state, full diff and PR text. No local tests, live credentials, app launch, approval or merge performed. Do not treat this source verdict as completed Windows/Linux acceptance.
|
On Wes’s behalf — Brain: Automatic CI passes, but current-head Windows validation still has two failing foundation tests. At
Disposition requires an explicit platform boundary, not a blind skip. Both bundled tests assume disk-cache sharing, which the pinned engine supports only on Unix; the existing Automatic run 36462023012, attempt 1 succeeds: 4,766 JS tests, 244 Rust tests, 136 tool tests, 808 functional browser cases and seven measurements. Its logs check out synthetic merge Implementation and PR-description updates remain with the originating owner. No source changes, local execution, CI rerun, approval or merge by this maintenance pass. |
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Approved at a68b39d6e3b764cf291dec0ec309a37bdc91ebdf. Current-head review found no actionable code defects; required automatic CI is green. Minimalness 9/10, elegance 9/10, correctness 9/10. Native Windows/Linux installed-app acceptance remains a separate validation gap, not a code-review blocker.
Brain, acting on Wes’s behalf.
Summary
Enable human identity setup/restore/import/export on Windows and Linux through Windows Credential Manager and Linux Secret Service (keyring 3.6.3). macOS custody remains unchanged.
Origin: Buzz channel
8461e5e0-89cb-4ca9-be35-1389f0bcb3df, threade52159b5e9b2de73e8bf5ad3f1eb53707a08b8fa0a1d8147be4db65d4f7015c0.Current status — conflicts resolved; validation pending
Head:
a68b39d6e3b764cf291dec0ec309a37bdc91ebdf.f551b81cvia9806c95d, preserving cross-platform identity composition/failure coverage and main's packaged relay restore/recovery contract. GitHub reports MERGEABLE; this only means no file conflicts, not approval or readiness. DCO passes.a68b39d6is a seven-file test/docs follow-up (43 insertions, 25 deletions; zero production changes). Native IPC fixtures use the mock webview's actual platform URL; the synthetic provider selects Windows Python with a cleared environment plus SystemRoot; the connect-failure test owns its listener through a TLS attempt rather than relying on OS refusal timing. No skipped tests, timeout increases or production ACL changes. Three platform descriptions now reflect the shared relay implementation without claiming native acceptance.9806c95dpassed native lint/compilation, all 44 controller unit tests, 16 controller integration tests, and all 11 credential-store tests including the new lock-lifetime regression. Foundation then failed 13 tests: 11 incorrect IPC-origin fixtures, the Unix Python path, and the loopback timeout. The follow-up addresses those fixture mechanisms; a Windows pass at the new head is not yet established.9806c95d..a68b39d6fixture/docs delta (matching SHA256) with no blockers. Review was read-only, not Windows execution. The merge review's three documentation corrections are included. Fresh CI and attended native acceptance remain required.Validation and evidence boundaries
007f93abb9d26c4545bf97dd57176891d9e46a9db2d86a7d31f21ac04ce9806c.a68b39d6:agent_models::(11 passed, 1 existing ignored),agents::tests::(19 passed, 1 existing ignored), andrelay::tests::(5 passed). Full native package suites are delegated to hosted CI, not claimed from these focused checks.e779db81retained production custody behavior. Isolated source/build mutation controls demonstrated that releasing the lock before read or before write fails the new contention assertion; restored control passed. The later Windows run executed this regression successfully. These tests use fake backends and establish lock lifetime, not secure-store persistence or consent.Native acceptance still required
Native acceptance instructions and custody limitations. Use a disposable OS account/VM and throwaway keys: changed HOME/profile/port does not isolate credentials. Verify human create/import/export → quit/relaunch → same key, refusal to overwrite, and Linux locked/denied/unavailable storage failing closed. Debug/release stores are separate. Agent UI import acceptance is deferred until that capability is enabled.
Linux needs a working Secret Service/default collection. Neither new backend isolates secrets from other code running as that OS user; Windows credentials may roam under OS policy. The shared native messaging implementation is inherited from main; installed-app messaging acceptance on Windows/Linux remains open.