feat(agents): Harnesses Goose install (slice 3/5) - #279
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested. Reviewed head 9d3591e2b6dea539b614b53d061b9922a711fa01 against stacked base/merge-base 5b4b02e94666f5cfaa803367c8f219a1f0401a78, with complementary UI, installer-security, and restart/concurrency review. Findings and required fixes are inline.
Merge criteria: preserve recovery Stop throughout download/restarts and preserve install progress/result/logs across Settings navigation. Keep ownership in the existing app control/native setup owners; no new installation framework is needed. Include deferred-operation regressions for these transitions, plus a genuinely eligible missing-CLI agent before exercising restart/Stop fencing. The new native Stop test currently starts with an empty waiting list and therefore does not prove waiting → stopped recovery.
Validation: complete 13-file diff and relevant stack integration paths reviewed; pinned-range git diff --check passed. Hosted CI run 36173979013, merge f89e647 (this head into its stated base), has 3,921 Vitest passes and the three baseline failures repaired by #276. Rust/tool integration and Chromium/WebKit journeys passed; Windows validation was skipped. No local tests, builds, installers, native launches, or live workflows were run; author-reported isolated installation is not our native acceptance evidence. The tests do not exercise the real installer command/process teardown; normal-Quit cleanup is also unverified, and the existing app exit handlers do not explicitly stop HarnessSetup. Do not infer Quit cleanup from kill-on-drop alone. No accidental screenshot or generated review files appear in the diff.
The approved upstream stable curl|bash trust model is not a new finding. Native eligibility re-checks, start-ticket cancellation, and revision checks are coherent in source. The inherited #277 profile-editor finding stays on #277. The PR description is stale about the original URL, stopped-agent eligibility, and checks at its earlier commit; correct it without treating metadata as a separate merge blocker.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review via Wes’s GitHub account, with an independent native-lifecycle pass by Mantis.
Reviewed head b708ab48e4a0b86211498c35243f10434bc109d7 against stacked base/merge-base 5b4b02e94666f5cfaa803367c8f219a1f0401a78, focusing on the substantive follow-up from previously reviewed 9d3591e2b6dea539b614b53d061b9922a711fa01 and its integration with the full feature.
The two earlier P2 findings are addressed in source. Installation now has an independent control lane, so it no longer monopolizes recovery Stop during download/automatic restart waits. Progress, report, error and log stay with the existing app-owned control projection across Settings unmount/remount. Deferred-operation tests cover Stop, older reads, retry and navigation; the native Stop regression now establishes a genuinely eligible missing-Goose failure before stopping it. These tests were inspected, not executed.
One remaining P3, non-blocking correction is inline: the new normal-Quit hook handles an already tracked group, but spawning and recording that group are not atomic with shutdown. This is the remaining edge of the earlier Quit-cleanup finding, not a new installation framework or a request to revisit the approved upstream stable curl|bash trust model. No additional actionable defects were established in the reviewed follow-up. Ancestor #277/#272 issues remain with their owning PRs.
Validation limits: immutable pinned source and lifecycle callers inspected; 29 source extracts verified against Git blobs and SHA-256; pinned-range git diff --check passed. No tests, builds, installers, app/agent launches, credential operations or live workflows were run. The current-head hosted check snapshot shows JavaScript and CI required failed; Rust/tool integration, browser measurements, all four Chromium/WebKit journey shards, DCO and security checks passed; Windows validation was skipped. See CI run 36183982972. I did not inspect failure logs or establish their cause; the PR’s reported local passes/failures are author evidence, not checks rerun here. Current native scrubbed-environment installation/restarts, proxy behavior and actual Quit cleanup still need acceptance evidence.
This is a COMMENT review only—not approval, a blocking review, CI-green readiness or merge authorization. No prior review was dismissed.
6a02b2b to
1b7e84f
Compare
b708ab4 to
a568bf9
Compare
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…ailures Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…nstall restarts Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…its report app-wide Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
e1ba49f to
d4b1193
Compare
…ad-on-send * origin/main: (58 commits) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) fix(status): reopen a Today status as Today near 16:00 (#275) test: use current navigation for GIF send roundtrip (#309) Fix composer focus when selecting channels and DMs (#307) fix: retire mention searches after chips and refuted prose (#303) ... # Conflicts: # src/features/messages/MessageComposer.test.tsx # src/features/messages/MessageComposer.tsx
* origin/main: (45 commits) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentCard.tsx # src/bundled/agents/AgentsPage.tsx
* origin/main: (36 commits) Delay message timestamp tooltips by 500 ms (#321) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentEditor.tsx # src/bundled/profiles/ProfileAgentIdentity.test.tsx
Slice 3/5 — one-click Goose install
#277 (Harnesses card) and #272 (docs) have merged. This PR targets
main. Settings → Agents → Harnesses offers Install for GooseCLI neededon macOS/Linux only; Windows keeps the status/hint but not the bash installer.goose_installIPC guards against concurrent installs; runsbash -o pipefail -c 'curl -fsSL https://github.com/aaif-goose/goose/releases/download/stable/download_cli.sh | CONFIGURE=false bash'with scrubbed environment, a five-minute timeout and process-group teardown. It captures combined output in app-dataagent-controller/goose-install.log(0600), returning a bounded tail and path on failure.HarnessSetuptracks the active process group and kills it on normal Quit, fencing late claims/spawns. The main-webview capability grants this IPC, while browser guests cannot use it.installed("goose"). Only enabled Goose agents whose prior start failed withRequired runtime executable is missingare eligible for restart. The supervisor rechecks under the controller lock immediately before each restart; Stop/Edit/Start during download cannot re-enable an agent no longer waiting. Stopped/disabled/running agents and failures for other causes are not restarted.AgentControllane, separate from agent writes: Stop remains usable during download and automatic restarts. Progress and the last result/log survive leaving and returning to Settings (but are not persisted across app restarts). On completion,control.refresh()re-detects via native snapshot after any older read or in-flight agent operation settles; the card displays progress, Ready/success/restart counts, or failure and the expandable log. A new attempt clears the previous report.Command evidence: old Buzz
desktop/src-tauri/src/managed_agents/discovery/catalog.rs:22specifies the exactaaif-goose/gooseURL above. The formerblock/gooseURL redirects to the same release asset. Upstream's script installs to$HOME/.local/binby default and skipsgoose configurewithCONFIGURE=false. No docs were edited; the documented script and noninteractive behavior are unchanged. The approved upstreamstablecurl|bash trust model is deliberate, not a pin/checksum change in this PR.Verification before restack (head
b708ab48e4a0b86211498c35243f10434bc109d7)pnpm check,cargo fmt --all --check,cargo clippy -p buzz-foundation --all-targets -- -D warnings: passed.host_command/terminalprocess tests fail under machine load; those passed in the serial run. New native tests use a local child, never the upstream installer.AgentSettings,control,control-native,AgentsPage): 138 passed. Browser agent-control, editor-grid and model journeys: 34 passed across Chromium and WebKit (--no-deps).PluginImport,MessageRow,ProfileAgentIdentityfailures fixed separately in test: repair three baseline Vitest failures #276 (not edited here);dev/vite-config.test.mjstimed out under load andNewMessage.test.tsxsaw one extra request under load, then both affected files passed focused (25/25). The manually run repository pre-push group failed only inheritedProfileAgentIdentity(447 passed / 1 failed); its design group passed. The push used this worktree's existing global hook path after recording the failures. CI remains the source for the latest merged-tree status.block/gooseredirect's upstream script installs executable Goose 1.52.0 to$HOME/.local/binwithout modifying the real user's installation; the disposable HOME was removed. The current native scrubbed-environment desktop install/restart, proxy, and Quit have not been hand-verified. Windows native runtime was not tested.Integration update (head
d4b1193597e11f7ab88cdd502a9389912a8a2592)mainafter feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) #277 squash merged. The reviewer’s Quit serialization commit and all authors’ DCO trailers are preserved.bin/pnpm typecheck, focused Vitest (94 tests), the agent-control browser journey in Chromium and WebKit (28 tests),cargo fmt --all --check, and the repository pre-push group (552 related tests and design checks) passed.host_command::tests::passes_exact_args_without_shell_interpretation; it passed in the serial run.Hand-test before acceptance
HTTPS_PROXYcan install.Next slice
Slice 4 can reuse
AgentHost/agents::start(..., Action::Restart, ...)andcontrol.refresh()/snapshot wiring for native effective-settings restarts, but should compare changed effective settings before restarting running agents. Do not reuse the Goose-specific installer guard/log. The profile-editor fix belongs to #277, not this PR.