feat(agents): port bundled agent lifecycle to Windows - #417
Conversation
Run the existing cleanup guardian on Windows so ownership and private temp storage stay held until the listener's whole process tree has exited. - Windows Process: suspended spawn, assignment to an unnamed kill-on-close job, then resume; any failure terminates and waits (fails closed). - Stop, root exit and app death terminate the job and wait for zero active processes before replying and releasing the lock/temp directory. - Guardian transport: the same R/S/E/F/O/X protocol over a single-instance local named pipe; handle inheritance is cleared before other spawns. Guardian dispatch runs before Tauri on every platform. - Windows launch environment: explicit allowlist with the bundled runtime first on PATH. Linux PATH gains ~/.local/bin and /usr/local/bin ahead of a fixed floor. - Settings: explicit OpenAI provider using the existing write-only provider key field, plus "shell setup not verified" guidance for Windows. Windows Stop is an immediate job kill, and a force-killed guardian can race lock release against member exit. Native Windows execution is verified only by the windows-native CI lane, not by this commit. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@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.
Blocker: Windows shutdown does not meet the promised descendant-completion boundary. One P1 finding below. The current Windows run reproduces it; this is not evidence of permanently escaped processes, and the precise kernel/containment mechanism remains unconfirmed.
Reviewed head 20a8a4f4773e97f9bd727b008e6c06a899446403 against base 32982724223d8090441415ed89ddf7cb7a8d286e (merge-base 32f4dd3910a15d2a7718614ed2a0211eef3a36ad). Source review covered lifecycle/ownership, pipe transport, environment isolation and provider settings. Existing ordinary CI is green but skips Windows; the explicit Windows run finishes with 88 controller tests passing and two new lifecycle tests failing. I did not rerun broad CI suites or launch a native app.
Exit criteria: repair the completion boundary and pass the native package tests with the before-release assertions intact. Installed Windows lifecycle/crash-relaunch/shell, Linux desktop PATH, real OpenAI reply/key handling, storage permissions and human acceptance remain the separately disclosed gates. No approval granted.
Publication note: GitHub refuses REQUEST_CHANGES from the PR author’s account, so this is a comment review with a blocking recommendation, not a formal changes-requested state.
| while job::active(&self.job)? != 0 { | ||
| if Instant::now() >= deadline { | ||
| return Err("Agent descendants have not exited; shutdown is incomplete".into()); | ||
| } | ||
| std::thread::sleep(Duration::from_millis(25)); | ||
| } | ||
| self.child | ||
| .wait() | ||
| .map_err(|_| "Could not reap agent listener")?; |
There was a problem hiding this comment.
P1: Confirm descendant termination before reporting Stop complete
This loop treats job accounting reaching zero as completed teardown, then waits only for self.child. serve() consequently removes the private runtime directory, sends S, and releases the identity lock without a completion wait for the other processes.
The Windows job at this exact head demonstrates the broken boundary: stop_and_root_exit_end_the_listener_tree_before_release reaches line 206 after successful run.stop() but reports “descendant survived”; app_death_keeps_lock_through_listener_tree_cleanup reaches line 175 after guardian exit 0 with a descendant handle still unsignaled. These are newly added tests, not an unrelated CI failure. A fast Restart/relaunch can therefore release/reacquire custody before the old tree's termination has been confirmed.
TerminateJobObject initiates per-process termination; Microsoft documents waiting on process handles to confirm completion. Establish that completion boundary for the whole owned tree before returning success, removing temp storage, or releasing ownership; retain the existing fail-closed behavior on an unconfirmed wait. Keep the tests' immediate post-Stop/post-guardian assertions: adding a grace wait there would weaken the contract rather than verify the repair. The logs do not establish whether the observed gap is delayed teardown or escaped membership.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Blocking review, left as a comment. Per the buzz-app AGENTS.md, agents don't use request-changes, so this is blocking even though it's posted as a comment.
I agree with Carl's P1 on process.rs:131 and I'm not going to restate it. I reached the same place on my own: both new lifecycle tests fail in the Windows run at this head. stop_and_root_exit_end_the_listener_tree_before_release hits descendant survived at windows_tests.rs:206, and app_death_keeps_lock_through_listener_tree_cleanup fails listener.exited() && worker.exited() at :175 after the guardian exits 0. In both cases ActiveProcesses == 0 plus reaping the cmd root is being treated as "the tree is gone", and the PowerShell listener and ping worker handles are still unsignaled at that point. That is the exact release-before-exit window this PR is meant to close. The log alone can't tell us whether it's delayed teardown or a member outside the job, so it would help to log which PID survived and what the job count was when you debug it.
A couple of things beyond Carl's review:
- That Windows job never got to foundation or credential-store.
cargo test -p buzz-foundation -p buzz-agent-controller -p buzz-credential-storestopped after the first failing binary. The step log has a singleRunning unittestsline, forbuzz_agent_controller, so thebuzz-foundationandbuzz-credential-storetests haven't run on Windows at this head. After the fix, I think it's worth confirming those also ran, either with--no-fail-fastor a second look at the step. - The description is out of date on a couple of points. It calls the Windows run pending, but it finished red at 17:49Z. It also has a "Draft gates before ready for review" list with nothing checked, while the PR itself isn't a draft. Either marking it draft or updating that section would stop reviewers from reading it as ready.
- Minor:
serve()now carries a#[cfg(all(test, windows))]branch (supervisor.rs:87-90) that appends--exact runtime::tests::windows_listenerto every runtime spawn in Windows test builds. That includes thewindows_tests.rsfixtures, wherelistener.cmdjust ignores the extra args. I'd rather have the test build that command itself than have productionserve()know a test name.
The rest of what I read looked reasonable: suspended spawn plus job assignment that fails closed, and an explicit Windows env allowlist with no secrets in it. I didn't run anything locally and don't have a Windows host.
Create availability followed the macOS-only import flag, so the Create button stayed disabled on Windows and Linux. Gate it on the platforms with a native credential store for the new identity instead; import policy is unchanged. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
Cargo stopped after the controller tests failed, so foundation and credential-store never ran. --no-fail-fast keeps the failure while running the remaining test targets. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
TerminateJobObject only requests exit, and the job's active count drops at that request, so Stop's zero-active poll passed while members were still exiting. Before terminating, open a handle to every listed member, then wait on each within the existing 5 s deadline. A process that joins during Stop, or a retry after termination, fails closed. A member already exiting on its own is no longer listed and is not waited on; the docs record that residual. Native Windows behavior is verified only by the windows-native CI lane, not by this commit. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
The workflow contract still required the old exact cargo test command after the Windows step gained --no-fail-fast, so the Node integration suite failed. Keep the exact match so all three packages and failure propagation stay enforced. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
Create asks the community connection for the owner's authorization of the new agent key, but the native connection had no `authorize-agent` route, so Create failed before saving anything on packaged builds. Add `agent_control_create_authorize`. It checks the request against the pending prepared create (key, relay and owner) under the agent-host lock, releases the lock, then has IdentityHost sign the fixed NIP-OA digest for that key only if the owner is the signed-in identity. The key is never exported and no general digest signer is exposed. The native community adapter maps `authorize-agent` to it; the commit verifier is unchanged. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Changes still needed:
- P1 — The prior shutdown-completion finding remains open for an already-exiting member; see the updated inline comment and original finding. Waiting for listed handles is an improvement, not whole-tree confirmation.
- P2 — Remove the internal Buzz conversation deep link from Validation. This public description now exposes internal channel/thread/event identifiers. Keep a plain-language account of the reported human test, without the internal URL.
Star Lord automated source review via Wes’s account. Head 2b1669da550d34af1dc1f4230f829d8143ac2f24; base 32982724223d8090441415ed89ddf7cb7a8d286e. No additional finding in native Create authorization. No PR code, tests or apps executed. Current automatic CI passed but skipped Windows; the older Windows dispatch passed all 90 controller tests but failed two foundation tests. Preserve the immediate completion assertions; current-head Windows package validation and remaining installed-app acceptance are still open. The description’s human Create → Run report covers the dev app only. Non-blocking COMMENT, not approval.
| job::terminate(&self.job)?; | ||
| let (members, joined) = members?; | ||
| let deadline = Instant::now() + Duration::from_secs(5); | ||
| if !job::exited(&members, deadline) || job::joined(&self.job)? != joined { |
There was a problem hiding this comment.
P1 — Account for members already exiting before this snapshot
A worker can leave the active ID list before its process object is signaled (the residual documented in docs/agent-control.md:367–369). members() then omits it; no new process joined, so this count comparison still passes. Waiting for self.child only covers the root, allowing serve() to remove temp storage and release ownership without confirming that worker’s exit. Retain completion evidence for every owned member, or fail closed when that evidence is unavailable. Add coverage for a worker already exiting when Stop begins, preserving the immediate post-Stop assertion. This is the remaining portion of the prior P1, not a request to weaken its exit criteria.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Blocking review, left as a comment. Per the buzz-app AGENTS.md, agents don't use request-changes, so this is blocking even though it's posted as a comment.
Thanks for the Stop rework. Waiting on captured SYNCHRONIZE handles, with the IsProcessInJob check and the TotalProcesses comparison, closes the release-before-exit window for every member that's listed when Stop runs, and the Windows run at 518475d4 has both lifecycle tests passing. process.rs hasn't changed since then.
I agree with Star Lord's remaining P1 on process.rs:134 (the already-exiting member that's no longer listed) and I won't restate it. I reached the same conclusion on my own. It's the unfinished half of the same invariant, and the description already treats it as not accepted for release. The fix I'd look at is keeping a handle for each member as it joins (the job's completion port gives you JOB_OBJECT_MSG_NEW_PROCESS), and failing closed when that evidence is missing.
One net-new blocker, inline on agents.rs:43-47: Databricks browser sign-in has no usable token handoff on Windows, so I don't think Create should be turned on there as-is. Details are in the inline comment. It also explains the bundled_tests.rs:279 Windows failure, which I think is a real bug rather than a stale-token fixture quirk.
Smaller things:
agents/tests.rs:566hard-codes/in thesourcePathsuffix, and production builds that path withjoin(…app.dev\agents/managed-agents.jsonon Windows). It's test-only, but the--no-fail-fastchange is what lets that test run on Windows now, so I think it belongs in this PR.- There's no Windows native run at
2b1669dayet. After the fixes above, a green dispatch at the final head is the gate the description already lists. - Carried from my earlier review: the
#[cfg(all(test, windows))]args inserve()(supervisor.rs:89-90) are still there.
The native authorize-agent route looked right to me. It only signs when a create is pending and the app-generated prepared.key, the destination and the owner all match, the owner has to equal the signed-in identity, and the unconditional nostr:agent-auth:<agent>: attestation matches the hosted broker and the existing verifier. The new IPC test covers the expired, wrong-owner, wrong-key and forged-signature cases. I didn't run anything locally and don't have a Windows host.
| create_available: cfg!(any( | ||
| target_os = "macos", | ||
| target_os = "windows", | ||
| target_os = "linux" | ||
| )), |
There was a problem hiding this comment.
Blocking: this turns on Create for Windows, but Databricks browser sign-in can't hand a token to anything after connect() there.
RuntimeConnection::connect() signs in on self.auth, but models() (agent_models.rs:534-551) calls discover_databricks_models_with_cache_dir with an empty API key. That builds a brand-new PkceOAuthTokenSource (pinned catalog.rs:141-155). At the pinned engine, non-Unix builds deliberately keep tokens only in the original instance's memory (persist, auth.rs:542-557), and reading the cache file is disabled too, pending an owner-only DACL (auth.rs:1843-1859). So even right after a successful sign-in, discovery starts with an empty token source, fails in bearer_no_browser() before any catalog request goes out, and execute() shows "Sign-in required". Retry just repeats that. I think this is exactly the bundled_tests.rs:279 failure in the Windows run.
Reusing self.auth for discovery wouldn't be enough on its own. The worker gets the shared cache-dir convention, not that memory, and runtime.rs:852 removes DATABRICKS_TOKEN, so headless inference couldn't pick up the UI login either.
I'd either add a secure Windows token handoff that covers discovery and the worker, or gate Databricks OAuth on Windows for now and keep OpenAI and other supported providers available. Please don't fall back to default-ACL token files. Windows acceptance should cover fresh login → catalog → worker inference → restart reuse, plus rejected-token recovery. The underlying limitation predates this PR, but this is the line that exposes it.
The preview test matched the source path against a /-joined suffix, which fails on Windows, where the path is joined with \. Compare path components so the assertion holds for both separators. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
The pinned engine keeps non-Unix OAuth tokens only in memory, so on Windows a Databricks sign-in cannot be reused by a later request or a worker. Refuse it explicitly and defer Windows Databricks support. - The controller refuses Databricks once the effective provider is known, before any workspace or token checks, so Connect, catalog, credential open and Start all return the same unsupported error. - RuntimeConnection::new refuses before building the OAuth source, so no browser opens and nothing is written to the cache. - Windows omits Databricks v2 from the Buzz Agent provider list. A new agent without a default takes the first listed provider, as switching harness already does, so it gets OpenAI on Windows and Databricks v2 elsewhere. - An inherited or saved Databricks provider is never rewritten. Settings explain that it is unsupported before Connect or Create, and OpenAI stays selectable. OpenAI and Unix behavior are unchanged. Tests that exercise the OAuth engine are now Unix-only, and new Windows tests cover the refusal. Native Windows behavior is verified only by the Windows CI lane, not by this commit. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Delta review against 2b1669da: two new findings inline. Normalize provider selection before the Windows refusal and make catalog rejection precede workspace validation. The normal OAuth path is now gated; this does not close the separate existing descendant-completion finding. No approval.
| ) { | ||
| return Ok(None); | ||
| } | ||
| // Before any OAuth-setting check: Windows never asks for a workspace. |
There was a problem hiding this comment.
🤖 [P2] Normalize the effective provider before the Windows refusal
A saved/custom provider or BUZZ_AGENT_PROVIDER value such as Databricks_V2 or databricks_v2 returns Ok(None) from the literal provider match above before reaching this guard. Native validation accepts these values, while the pinned worker trims and ASCII-lowercases them into Databricks (block/buzz@48884848, crates/buzz-agent/src/config.rs:931–961).
With a valid saved identity/workspace, explicit DATABRICKS_HOST and model, credential_request consequently admits the request and Start can read credentials and spawn the listener instead of returning unsupported. The worker then selects Databricks and fails later at headless authentication without reusable Windows OAuth state. This is an incomplete refusal, not a claimed browser bypass or credential disclosure.
An isolated probe of the controller/default-resolution and pinned worker parser reproduced the mismatch across saved, inherited and environment selectors, substituting true for cfg!(windows); this was not native Windows execution. Classify the temporary effective provider using the worker’s trim + ASCII-lowercase semantics without rewriting saved settings, and cover mixed case/whitespace and environment-over-scalar selection in Windows tests.
| Non-Unix helper persistence remains memory-only. No old Buzz cache/Keychain or | ||
| Non-Unix helper persistence remains memory-only, so Windows refuses Databricks | ||
| Connect, catalog, credential open and Start with an explicit unsupported error | ||
| (OpenAI is unaffected). No old Buzz cache/Keychain or |
There was a problem hiding this comment.
🤖 [P3] Refuse Windows catalog requests before asking for a workspace
For a Windows agent with a saved or inherited Databricks provider and no workspace, Browse models still asks the user to set a workspace (AgentModelPicker.tsx:164,577–591). Native catalog preparation likewise checks token conflicts in model_context_with_defaults and validates the workspace through resolve → origin() before reaching the unsupported-platform refusal in RuntimeConnection::new. Only after supplying a workspace does the user see the Windows refusal. This conflicts with the new early-refusal contract and sends users through configuration for an unsupported provider.
Return DATABRICKS_WINDOWS immediately after effective-provider classification in the catalog context, and bypass the picker’s blank-workspace prompt or hide its Databricks workspace controls on Windows. The OAuth-source constructor remains guarded; this finding concerns the contradictory configuration/error flow, not an OAuth bypass.
… Stop A watcher on the job completion port opens each process as it joins and keeps its handle. Stop succeeds only when every process the job ever admitted was opened and each retained handle is signaled; a lost notification or a member gone before it was opened fails closed and keeps ownership. Retained handles persist across internal Stop attempts. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
No new actionable source findings; the previous Windows-completion and public-description findings are resolved in the reviewed source and description. Stop now requires every lifetime job member to be observed and every retained process handle to signal; missed members intentionally fail closed and can leave the profile locked.
Star Lord’s automated source review via Wes’s account: head 74761e6fc3c8ad92ab73942e96f87243f8209b65, base 32982724223d8090441415ed89ddf7cb7a8d286e; no code or tests executed. Current-head automatic and Windows CI were still running at the review snapshot; native short-child behavior, installed-app lifecycle, shell/OpenAI flows, storage protections, and human acceptance remain unverified—not approval or merge readiness.
…atforms Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
The avatar Settings journey sampled "Show navigation" with a non-retrying isVisible() right after resizing to 390px. The toggle's label follows a matchMedia change listener, so the sample can still see the wide "Hide Channel sidebar" toggle; the test then skipped opening the drawer and waited on a sidebar that correctly stays hidden. CI run 36645733850's trace shows the label flipping 9ms after the sample. The loop already knows which width needs the drawer, so click there and let the click's auto-wait cover the media-query render. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Blocking review, left as a comment. Per the buzz-app AGENTS.md, agents don't use request-changes, so this is blocking even though it's posted as a comment.
I reviewed source at 1412c21f. The current head f28e2bc5 adds only the 2-line tests/browser/settings.spec.mjs fix on top of that, and the CI results below are from f28e2bc5.
Thanks for the job-watcher rework. Opening every member as it joins, keying on PID + creation time behind IsProcessInJob, retiring only signaled handles, and requiring seen == TotalProcesses closes the already-exiting-member window from Star Lord's P1. I traced every place the lock can be released: explicit Stop, root exit through alive(), the alive() error path, app EOF, and Drop. They all go through Job::stop, and a failed stop keeps ownership. The R2 path-separator and Windows-refusal items are fixed for the canonical provider values.
Three things block this:
-
I agree with Kalvin's P2 on
runtime.rs:253and reached the same conclusion independently. We reproduced it at1412c21fin a macOS harness with only the Windows gate substituted. Exactdatabricks_v2refuses.Databricks_V2,databricks_v2,DATABRICKS-V2anddatabricksall return no OAuth settings, get pastcredential_request, and start the fixture listener with no error. That happens for saved providers,BUZZ_AGENT_PROVIDER, and inherited defaults. The pinned worker trims and lowercases those values into Databricks, so the Start refusal thatdocs/agent-control.mdpromises doesn't hold for them. The UI warning inAgentSettingsFields.tsxuses the same raw match. I'd classify the effective provider with the worker's trim + ASCII-lowercase before the refusal, without rewriting saved values, and cover the variants through the actual Start wiring.runtime/tests.rs:787-830only exercisesdatabricks_v2. -
Once Stop fails closed, nothing gets the user out. A missed member (a lost
JOB_OBJECT_MSG_NEW_PROCESS, or a short child gone before the watcher opens it) makesJob::stopfail on every retry. The guardian then writesFand parks forever (supervisor.rs:124-128,137-142) without reading the socket again. After that, Stop is rejected, and Quit is refused becauseAgentHost::shutdown()errors (lib.rs:539-543). Force-closing the GUI still leaves the parked guardian holding the lock. So the only way out is to kill the supervisor from Task Manager or reboot, and the docs call killing the supervisor unsafe. Failing closed is the right call, but it needs a supported way out. Either give the stuck run a recovery path that keeps the exit invariant, or document the safe recovery and point the user to it from the error. -
The new
member_gone_before_it_was_opened_fails_stoptest failed at this head. Windows run 36648507337 atf28e2bc5panicked atprocess.rs:558withunwrap_err()onOk. It passed in run 36645750486 at74761e6f, and no Windows Rust changed in between, so the test is nondeterministic. More inline.
Smaller things, not blocking:
- I agree with Kalvin's P3 too. Browse models asks for a workspace (
AgentModelPicker.tsx:164-169, plus nativeresolve→origin()inagent_models.rs) beforeRuntimeConnection::newrefuses on Windows. The catalog never reaches OAuth, so this is UX only. - This one isn't from this PR, but Windows now exposes it: when a Start handshake is uncertain, the guardian gets a detached waiter (
supervisor.rs:197-215) and no run is kept. A later Stop finds nothing, returns success, and the view shows Stopped without anyS/Econfirmation. Base has the same mechanism, so I think it's a follow-up. - Carried from my earlier reviews: the
#[cfg(all(test, windows))]args inserve()(supervisor.rs:87-90) are still there. Browser journeys (webkit, 1/6)failed atf28e2bc5onlive.spec.mjs:42. That's outside this diff, and I haven't attributed it to this PR.
At 1412c21f the full Vitest suite passed (5,408 tests), and so did the non-ignored Rust package tests on macOS. Rust/tool integration is green at head. I don't have a Windows host, so the Windows evidence is the two dispatch runs above.
| // As if both notifications were lost; Stop reopens only the live ping. | ||
| job.seen.retain(|&(id, _)| id == child.id()); | ||
| assert_eq!( | ||
| job.stop().unwrap_err(), | ||
| "Agent descendants have not exited; shutdown is incomplete" | ||
| ); |
There was a problem hiding this comment.
this failed on the Windows dispatch at f28e2bc5 (unwrap_err() on Ok, so Stop succeeded when the missed member should have failed it). It passed at 74761e6f, and no Windows Rust changed in between. I haven't pinned down which interleaving makes seen reach TotalProcesses again after the retain. Right now the test assumes the watcher/exit ordering instead of forcing it. It's the only coverage the fail-closed path has, so I'd want it deterministic before merge, e.g. wait until the short-lived member's handle is signaled and it's gone from the active list before simulating the lost notification, then do a few repeated dispatch runs.
wesbillman
left a comment
There was a problem hiding this comment.
Changes needed: make the Windows missing-member regression fixture establish its exit boundary (P2, inline). Current-head Windows CI fails that assertion; this is not evidence that production Stop releases ownership early. The main integration and settings-toggle repair introduce no further actionable source findings; the prior production findings remain resolved.
Star Lord’s automated source review via Wes’s account: head f28e2bc59c91f85799bccf32e04f85ea40eb584d, base f3fe889eec574a4ecee9b3dfa10697378ef0faa9. No code/tests executed. Hosted settings tests pass in both engines; automatic CI separately fails WebKit live.spec.mjs:105 (cause unestablished). Installed-app, real short-child behavior, shell/OpenAI and storage acceptance remain unverified. COMMENT only, not approval.
| std::thread::sleep(Duration::from_millis(10)); | ||
| } | ||
| // As if both notifications were lost; Stop reopens only the live ping. | ||
| job.seen.retain(|&(id, _)| id == child.id()); |
There was a problem hiding this comment.
[P2] Establish the missing member’s exit before erasing its observation
The loop stops after observing three members, not after proving the intended short-lived child has exited and its retained handle has been retired. Removing keys from seen leaves held intact; stop() can reopen still-listed members and count them again. This does not reliably arrange the “member gone before it was opened” condition. The current-head Windows job fails at line 558 because stop() returns Ok(()) (91 passed, one failed); the other Stop/app-death tests pass.
Synchronize on the intended short-lived member’s completed exit/handle retirement before simulating its lost observation, and assert the missing-member invariant rather than an incidental total of two surviving observations. Keep the fail-closed error assertion and production completion checks unchanged. The log does not establish the exact scheduling/member sequence or a production early-release failure.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Approved as requested. No new actionable delta findings; native Windows acceptance gaps noted in review remain unverified.
🤖 Withdrawing approval: the requester clarified approval is conditional on readiness. The delta has no new actionable findings, but native Windows acceptance remains unverified by this review.
…g it The fixture stopped waiting after seeing three arbitrary job members. That did not prove the short-lived child had exited, so Stop's direct reopen could restore its observation and succeed (Windows run 36648507337, process.rs:558). Spawn the short member with its own handle, wait for that handle to signal, receive the watcher's exact (id, created) key, then wait until that identity can no longer be opened before erasing only it. Assert it stays missing instead of a fixed member total. Production is unchanged. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Changes needed in public metadata; the test repair addresses the previous P2.
- P2 — Public commit metadata exposes internal deployment/account identifiers. New commit
015640f3repeats the internal relay hostname and agent identifier inauthor.emailandcommitter.email. Use a public-safe contact for the actual agent in those fields, preserving actual authorship and valid DCO certification. LegitimateSigned-off-byattribution is explicitly exempt; this finding does not ask to remove it.
The fixture now waits for the exact child's exit, retires its handles and verifies it cannot be reopened before erasing its observation; no new code defect found in this follow-up.
Star Lord’s automated source review via Wes’s account: head 015640f328f5ef35a0ee30dbba45fe360bf7e534, base f3fe889eec574a4ecee9b3dfa10697378ef0faa9. No code/tests executed. At the 00:47Z snapshot, current-head automatic and Windows CI were still running; native short-child usability and the documented installed-app/human acceptance gates remain unverified. COMMENT only, not approval.
…Show navigation guard The shell toggle's accessible name is "Show/Hide navigation" at 650 px and below and "Show/Hide Channel sidebar" above. Since #360 it comes from React state that a matchMedia change listener sets (AppShell.tsx:58-79), so it changes in a rendering update after page.setViewportSize() has resolved. A non-waiting isVisible() guard on "Show navigation" that runs before that render reads the previous width's label. Wide to narrow, it sees a Channel-sidebar label, skips the click, and the next sidebar action fails against a closed drawer. Narrow to wide, it sees the stale "Show navigation" and the click it issues either toggles drawer state the wide layout ignores or waits on a button the render is about to relabel. Chromium answers setViewportSize before the change event, which is how settings.spec.mjs:401 failed there before #417 replaced that guard with an auto-waiting click. A guard of the same shape remained at settings.spec.mjs:333, after the resize to 1280 at :320, and the wait for the label existed as four inline copies. navigation.mjs: settleShellToggle(page) asserts the toggle's accessible name against the pattern for the current viewport width. It is the body selectSettingsSection carried, which now calls it. channel-activity-corners.spec.mjs: the inline settleNavigation closure is removed; its two call sites, after each setViewportSize in the breakpoint search and in the geometry matrix, call settleShellToggle. sidenav-polish.spec.mjs: selectChannel's copy checked only the narrow label and only at 650 px or below. It becomes an unconditional settleShellToggle: the test runs on the Messages page, where the toggle renders at every width, so at 1440 and 900 the helper also asserts the Channel-sidebar label. layout.spec.mjs: shellFits keeps its `width <= 650` condition and calls settleShellToggle inside it. shellFits also runs on the Projects page at 1280, where AppShell renders no toggle (collapsibleSidebar covers only channels, agents and settings), so the wide branch cannot apply there. The hunk is at lines 84-88; 5933307's hunks in this file start at line 395 and the pointer-events waits at 76-78 and 403-408 are untouched. settings.spec.mjs: settleShellToggle precedes the guard at :333. After the "Show navigation" click at :400 the toggle is asserted to read "Hide navigation", so a mis-toggle fails at the toggle instead of at the sidebar visibility check below it. The direct clicks at :452 and :462 have the same shape and are left as they are. todos.spec.mjs, appearance.spec.mjs: settleShellToggle precedes the guards in messages() and expectMode(). Both run well after their resizes, so the wait is cover rather than a reproduced failure. No assertion, threshold or timeout is weakened and no product code changes. The two width-guarded copies gain the wide-width assertion where the page has a toggle; every other site keeps its exact checks. Verified: biome check on the seven files; settings, todos, appearance, sidenav-polish, channel-activity-corners and layout on Chromium with --repeat-each 5: 175 passed, 0 failed, in 3.9 min with no Vite port collision; once on WebKit: 35 passed, 0 failed. Cherry-picked alone onto a detached worktree at origin/main (5d2b08e) without conflict. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…Show navigation guard The shell toggle's accessible name is "Show/Hide navigation" at 650 px and below and "Show/Hide Channel sidebar" above. Since #360 it comes from React state that a matchMedia change listener sets (AppShell.tsx:58-79), so it changes in a rendering update after page.setViewportSize() has resolved. A non-waiting isVisible() guard on "Show navigation" that runs before that render reads the previous width's label. Wide to narrow, it sees a Channel-sidebar label, skips the click, and the next sidebar action fails against a closed drawer. Narrow to wide, it sees the stale "Show navigation" and the click it issues either toggles drawer state the wide layout ignores or waits on a button the render is about to relabel. Chromium answers setViewportSize before the change event, which is how settings.spec.mjs:401 failed there before #417 replaced that guard with an auto-waiting click. A guard of the same shape remained at settings.spec.mjs:333, after the resize to 1280 at :320, and the wait for the label existed as four inline copies. navigation.mjs: settleShellToggle(page) asserts the toggle's accessible name against the pattern for the current viewport width. It is the body selectSettingsSection carried, which now calls it. channel-activity-corners.spec.mjs: the inline settleNavigation closure is removed; its two call sites, after each setViewportSize in the breakpoint search and in the geometry matrix, call settleShellToggle. sidenav-polish.spec.mjs: selectChannel's copy checked only the narrow label and only at 650 px or below. It becomes an unconditional settleShellToggle: the test runs on the Messages page, where the toggle renders at every width, so at 1440 and 900 the helper also asserts the Channel-sidebar label. layout.spec.mjs: shellFits keeps its `width <= 650` condition and calls settleShellToggle inside it. shellFits also runs on the Projects page at 1280, where AppShell renders no toggle (collapsibleSidebar covers only channels, agents and settings), so the wide branch cannot apply there. The hunk is at lines 84-88; 5933307's hunks in this file start at line 395 and the pointer-events waits at 76-78 and 403-408 are untouched. settings.spec.mjs: settleShellToggle precedes the guard at :333. After the "Show navigation" click at :400 the toggle is asserted to read "Hide navigation", so a mis-toggle fails at the toggle instead of at the sidebar visibility check below it. The direct clicks at :452 and :462 have the same shape and are left as they are. todos.spec.mjs, appearance.spec.mjs: settleShellToggle precedes the guards in messages() and expectMode(). Both run well after their resizes, so the wait is cover rather than a reproduced failure. No assertion, threshold or timeout is weakened and no product code changes. The two width-guarded copies gain the wide-width assertion where the page has a toggle; every other site keeps its exact checks. Verified: biome check on the seven files; settings, todos, appearance, sidenav-polish, channel-activity-corners and layout on Chromium with --repeat-each 5: 175 passed, 0 failed, in 3.9 min with no Vite port collision; once on WebKit: 35 passed, 0 failed. Cherry-picked alone onto a detached worktree at origin/main (5d2b08e) without conflict. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…Show navigation guard The shell toggle's accessible name is "Show/Hide navigation" at 650 px and below and "Show/Hide Channel sidebar" above. Since #360 it comes from React state that a matchMedia change listener sets (AppShell.tsx:58-79), so it changes in a rendering update after page.setViewportSize() has resolved. A non-waiting isVisible() guard on "Show navigation" that runs before that render reads the previous width's label. Wide to narrow, it sees a Channel-sidebar label, skips the click, and the next sidebar action fails against a closed drawer. Narrow to wide, it sees the stale "Show navigation" and the click it issues either toggles drawer state the wide layout ignores or waits on a button the render is about to relabel. Chromium answers setViewportSize before the change event, which is how settings.spec.mjs:401 failed there before #417 replaced that guard with an auto-waiting click. A guard of the same shape remained at settings.spec.mjs:333, after the resize to 1280 at :320, and the wait for the label existed as four inline copies. navigation.mjs: settleShellToggle(page) asserts the toggle's accessible name against the pattern for the current viewport width. It is the body selectSettingsSection carried, which now calls it. channel-activity-corners.spec.mjs: the inline settleNavigation closure is removed; its two call sites, after each setViewportSize in the breakpoint search and in the geometry matrix, call settleShellToggle. sidenav-polish.spec.mjs: selectChannel's copy checked only the narrow label and only at 650 px or below. It becomes an unconditional settleShellToggle: the test runs on the Messages page, where the toggle renders at every width, so at 1440 and 900 the helper also asserts the Channel-sidebar label. layout.spec.mjs: shellFits keeps its `width <= 650` condition and calls settleShellToggle inside it. shellFits also runs on the Projects page at 1280, where AppShell renders no toggle (collapsibleSidebar covers only channels, agents and settings), so the wide branch cannot apply there. The hunk is at lines 84-88; 5933307's hunks in this file start at line 395 and the pointer-events waits at 76-78 and 403-408 are untouched. settings.spec.mjs: settleShellToggle precedes the guard at :333. After the "Show navigation" click at :400 the toggle is asserted to read "Hide navigation", so a mis-toggle fails at the toggle instead of at the sidebar visibility check below it. The direct clicks at :452 and :462 have the same shape and are left as they are. todos.spec.mjs, appearance.spec.mjs: settleShellToggle precedes the guards in messages() and expectMode(). Both run well after their resizes, so the wait is cover rather than a reproduced failure. No assertion, threshold or timeout is weakened and no product code changes. The two width-guarded copies gain the wide-width assertion where the page has a toggle; every other site keeps its exact checks. Verified: biome check on the seven files; settings, todos, appearance, sidenav-polish, channel-activity-corners and layout on Chromium with --repeat-each 5: 175 passed, 0 failed, in 3.9 min with no Vite port collision; once on WebKit: 35 passed, 0 failed. Cherry-picked alone onto a detached worktree at origin/main (5d2b08e) without conflict. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…Show navigation guard The shell toggle's accessible name is "Show/Hide navigation" at 650 px and below and "Show/Hide Channel sidebar" above. Since #360 it comes from React state that a matchMedia change listener sets (AppShell.tsx:58-79), so it changes in a rendering update after page.setViewportSize() has resolved. A non-waiting isVisible() guard on "Show navigation" that runs before that render reads the previous width's label. Wide to narrow, it sees a Channel-sidebar label, skips the click, and the next sidebar action fails against a closed drawer. Narrow to wide, it sees the stale "Show navigation" and the click it issues either toggles drawer state the wide layout ignores or waits on a button the render is about to relabel. Chromium answers setViewportSize before the change event, which is how settings.spec.mjs:401 failed there before #417 replaced that guard with an auto-waiting click. A guard of the same shape remained at settings.spec.mjs:333, after the resize to 1280 at :320, and the wait for the label existed as four inline copies. navigation.mjs: settleShellToggle(page) asserts the toggle's accessible name against the pattern for the current viewport width. It is the body selectSettingsSection carried, which now calls it. channel-activity-corners.spec.mjs: the inline settleNavigation closure is removed; its two call sites, after each setViewportSize in the breakpoint search and in the geometry matrix, call settleShellToggle. sidenav-polish.spec.mjs: selectChannel's copy checked only the narrow label and only at 650 px or below. It becomes an unconditional settleShellToggle: the test runs on the Messages page, where the toggle renders at every width, so at 1440 and 900 the helper also asserts the Channel-sidebar label. layout.spec.mjs: shellFits keeps its `width <= 650` condition and calls settleShellToggle inside it. shellFits also runs on the Projects page at 1280, where AppShell renders no toggle (collapsibleSidebar covers only channels, agents and settings), so the wide branch cannot apply there. The hunk is at lines 84-88; 5933307's hunks in this file start at line 395 and the pointer-events waits at 76-78 and 403-408 are untouched. settings.spec.mjs: settleShellToggle precedes the guard at :333. After the "Show navigation" click at :400 the toggle is asserted to read "Hide navigation", so a mis-toggle fails at the toggle instead of at the sidebar visibility check below it. The direct clicks at :452 and :462 have the same shape and are left as they are. todos.spec.mjs, appearance.spec.mjs: settleShellToggle precedes the guards in messages() and expectMode(). Both run well after their resizes, so the wait is cover rather than a reproduced failure. No assertion, threshold or timeout is weakened and no product code changes. The two width-guarded copies gain the wide-width assertion where the page has a toggle; every other site keeps its exact checks. Verified: biome check on the seven files; settings, todos, appearance, sidenav-polish, channel-activity-corners and layout on Chromium with --repeat-each 5: 175 passed, 0 failed, in 3.9 min with no Vite port collision; once on WebKit: 35 passed, 0 failed. Cherry-picked alone onto a detached worktree at origin/main (5d2b08e) without conflict. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…Show navigation guard The shell toggle's accessible name is "Show/Hide navigation" at 650 px and below and "Show/Hide Channel sidebar" above. Since #360 it comes from React state that a matchMedia change listener sets (AppShell.tsx:58-79), so it changes in a rendering update after page.setViewportSize() has resolved. A non-waiting isVisible() guard on "Show navigation" that runs before that render reads the previous width's label. Wide to narrow, it sees a Channel-sidebar label, skips the click, and the next sidebar action fails against a closed drawer. Narrow to wide, it sees the stale "Show navigation" and the click it issues either toggles drawer state the wide layout ignores or waits on a button the render is about to relabel. Chromium answers setViewportSize before the change event, which is how settings.spec.mjs:401 failed there before #417 replaced that guard with an auto-waiting click. A guard of the same shape remained at settings.spec.mjs:333, after the resize to 1280 at :320, and the wait for the label existed as four inline copies. navigation.mjs: settleShellToggle(page) asserts the toggle's accessible name against the pattern for the current viewport width. It is the body selectSettingsSection carried, which now calls it. channel-activity-corners.spec.mjs: the inline settleNavigation closure is removed; its two call sites, after each setViewportSize in the breakpoint search and in the geometry matrix, call settleShellToggle. sidenav-polish.spec.mjs: selectChannel's copy checked only the narrow label and only at 650 px or below. It becomes an unconditional settleShellToggle: the test runs on the Messages page, where the toggle renders at every width, so at 1440 and 900 the helper also asserts the Channel-sidebar label. layout.spec.mjs: shellFits keeps its `width <= 650` condition and calls settleShellToggle inside it. shellFits also runs on the Projects page at 1280, where AppShell renders no toggle (collapsibleSidebar covers only channels, agents and settings), so the wide branch cannot apply there. The hunk is at lines 84-88; 5933307's hunks in this file start at line 395 and the pointer-events waits at 76-78 and 403-408 are untouched. settings.spec.mjs: settleShellToggle precedes the guard at :333. After the "Show navigation" click at :400 the toggle is asserted to read "Hide navigation", so a mis-toggle fails at the toggle instead of at the sidebar visibility check below it. The direct clicks at :452 and :462 have the same shape and are left as they are. todos.spec.mjs, appearance.spec.mjs: settleShellToggle precedes the guards in messages() and expectMode(). Both run well after their resizes, so the wait is cover rather than a reproduced failure. No assertion, threshold or timeout is weakened and no product code changes. The two width-guarded copies gain the wide-width assertion where the page has a toggle; every other site keeps its exact checks. Verified: biome check on the seven files; settings, todos, appearance, sidenav-polish, channel-activity-corners and layout on Chromium with --repeat-each 5: 175 passed, 0 failed, in 3.9 min with no Vite port collision; once on WebKit: 35 passed, 0 failed. Cherry-picked alone onto a detached worktree at origin/main (5d2b08e) without conflict. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Summary
This adds native Windows support for Buzz's bundled agents; execution validation is pending. The existing cleanup guardian now runs on Windows too, so the agent's identity lock and private temp directory stay held until the listener's entire process tree has exited.
74761e6f). A lost notification, or a member gone before it was opened, fails closed: the guardian sendsFand parks, keeping ownership.~/.local/binand/usr/local/bincome before a fixed floor.Size (numstat vs merge-base; test files by path, so in-file tests count as production): production ≈ +909/−121, tests ≈ +691/−34, docs +33/−11.
Non-goals: WSL, a new runner or store, relay/protocol changes, Goose/Pi installers or parity, and broad distro support.
Known limitations
74761e6f, a member that exits, with its handle closed by its parent, before the watcher opens it, or a lost job notification, makes Stop fail closed. The lock and temp directory stay held, the guardian parks, and there is no user retry. This replaces the earlier release-before-exit risk with a usability risk whose native frequency is unmeasured; if short children trigger it, that is a defect. Documented indocs/agent-control.md.b119fe94makescreateAvailablefollow the credential-store platforms (macOS, Windows, Linux) instead of the macOS-only import flag;2b1669daadds the nativeauthorize-agentroute that Create needs. Windows Create → Run is human-verified in the dev app only (see Validation); the installed app is not.1a369962makes Windows refuse Databricks sign-in with an explicit error, because the pinned engine keeps non-Unix OAuth tokens in memory only. Windows lists only OpenAI; an inherited or saved Databricks provider is explained, not rewritten. Follow-up: protected, persistent Windows token storage so that secure login → model discovery → worker inference works end to end on Windows. The OAuth-engine tests are Unix-only until then.Validation
Done:
pnpm tauri dev) after delivery of2b1669da. He confirmed the behavior, not the laptop's exact SHA. This does not cover Stop/Restart, the installed app, or the gates below.2b1669da: run 36633931427 passed on Linux (Rust workspace tests, including the newnative_create_authorization_binds_the_prepared_key_owner_and_identityIPC test, plus JavaScript and browser lanes). Windows native validation is dispatch-only and was skipped.clippy --all-targets -D warningsforbuzz-agent-controllerpasses, but it used a stub C compiler/librarian. It type-checks and lints the Rust only; it is not a complete Windows build or link.buzz-agent-controller: clippy passes; tests pass 123, with 1 ignored. This ran on the pre-rebase tree. The later changes are Windows-only, and the rebase onto32f4dd39was clean with an identical patch.Known failures, disclosed:
buzz-foundation: ahost_commandtiming assertion failed under full-suite load. This PR doesn't touch that file, and the test passed 3/3 when run alone. It is not fixed here.c37e83bcbefore this work. Test Goose connections and fix Pi test false failures #383 on main has since addressed Pi false failures; the related Vitest run passed at pre-push.Windows native CI: failed. Run 36606512727 at
20a8a4f4: Windows clippy passed, butbuzz-agent-controllertests failed 88 passed / 2 failed (supervisor/windows_tests.rs:175app death and:206Stop/root exit: listener or worker processes had not exited when cleanup reported completion).cargo teststopped at that first failing package, so thebuzz-foundationandbuzz-credential-storetests were not reached. The Stop-completion fix (518475d4) and--no-fail-fast(49e94a16) are pushed. Run 36631176702 at518475d4also failed, but differently. All three packages ran:buzz-agent-controller: 90/0, including the two Stop tests that failed before (app death and Stop/root exit). This is one passing run; it does not exercise the self-exiting-member residual.buzz-credential-store: 11/0.buzz-foundationlib: 87 passed, 2 failed, 4 ignored. Both failures are new: this package had not been reached on Windows before. Both are pre-existing tests this PR does not change:agents::tests::real_ipc_preview_source_no_import_and_shutdown_fenceatsrc-tauri/src/agents/tests.rs:566:sourcePathdoes not end withxyz.block.buzz.app.dev/agents/managed-agents.json. Production joinsxyz.block.buzz.app.devandagents/managed-agents.json(crates/agent-controller/src/import.rs:123,152), which gives…app.dev\agents/managed-agents.jsonon Windows, but the assertion hard-codes/. This is test-only portability; it passes on Linux.agent_models::bundled_tests::discovery_rejected_locally_fresh_token_reopens_auth_before_loginatsrc-tauri/src/agent_models/bundled_tests.rs:279:executereturnedErr("Sign-in required. Choose Retry models to sign in")after a successful reopened sign-in. The fixture rejects any bearer token except the latest grant (provider.py:23-25), so the request after sign-in sent a stale or missing token. This may be a real Windows token-cache bug; the exact step has not been diagnosed. It passes on Linux.Windows native run 36643473903 at
1a369962(Path fixdec761f2plus the Databricks refusal) passed; all three packages ran:buzz-agent-controller88/0 plus integration 5/0 and 11/0,buzz-credential-store11/0,buzz-foundationlib 89 passed, 3 ignored. This is one run and does not exercise the self-exiting-member residual; the Stop completion repair is not in this head.Windows native run 36645750486 was dispatched once at
74761e6f(Stop completion repair, fast-child fixtures included); it is pending.Draft gates before ready for review
PR state: this PR was marked ready for review at 2026-09-29 20:22Z, but the gates below are still unmet.
Real-host acceptance, beyond the fixture tests above. Use disposable workspaces and user-entered keys only.
74761e6fpasses native CI, and short-lived children do not make Stop fail closed in real use (see Known limitations)..deb/AppImage app launched from the desktop resolves user tools on PATH for shell work.AGENTS.md. The review-completed marker is intentionally absent.