Discover alternate Buzz ACP commands - #6948
Conversation
🔐 Codex Security Review
|
68b9c71 to
d857374
Compare
d857374 to
a6ee9e1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed exact head 08f66ff4d1ef12f25f6f41abf3417317443be447 against base 350caf1de3136f6ec473a69d394544e99dfd4d85. The ordinary picker/create/edit/profile path is coherent, but four end-to-end blockers keep the advertised command contract from holding:
-
[P1] Windows
.cmd/.batcandidates are selectable but cannot launch. Discovery intentionally accepts and resolves those shims (managed_agents/backend.rs:614-632, 665-699), then spawn passes the resulting batch-file path directly toCommand::new(managed_agents/runtime.rs:478-479, 522). This module already documents that WindowsCreateProcesscannot execute batch shims directly (runtime.rs:387-394). A normal npm-installedbuzz-*-acp.cmdis therefore advertised, saved, and only fails at deployment. Either exclude batch shims or execute them through the correct Windows command processor, and add a Windows-shaped launch regression rather than only filename-resolution coverage. -
[P1] The picker is not an execution trust boundary. Native persona create/update accept any trimmed
acp_command(commands/personas/create.rs:30-36, 58-64;commands/personas/update.rs:95-125), whileresolve_commandaccepts path-like input (managed_agents/discovery.rs:706-716) and spawn executes its result. IPC plus inbound/imported data can therefore persist an arbitrary command/path despite the documented convention-only contract. Enforce stockbuzz-acpor an exact discoveredbuzz-*-acpcandidate in native write/execute paths. Preserve legacy unknown values for display, but keep them unavailable/non-executable until replaced. -
[P1] Relay catalog and inbound reconciliation lose the selected command.
CatalogAgentProjectionandparse_agentomitacp_command(persona_catalog.rs:45-57, 216-262), so the frontend receives no value and substitutes the default. Separately,apply_inbound_personaoverwrites the existing definition projection without copyingacp_command(commands/personas/inbound.rs:679-698), so an owner-signed cross-device edit does not update an existing local definition. Add the field to both projections, with legacy absence preserving the intended fallback rather than erasing a newer local value accidentally. -
[P1] Snapshots and team publication/adoption explicitly erase the field.
AgentSnapshotDefinition/build_snapshotomit it and describe it as forbidden machine-local state (managed_agents/agent_snapshot.rs:19-41, 92-127, 194-220), while import hardcodesNone(commands/personas/snapshot/import.rs:561-575).TeamCatalogMember/member_projectionalso omit it and adoption hardcodesNone(managed_agents/team_catalog.rs:108-163, 294-309;commands/teams/adopt/apply.rs:426-449). The PR body explicitly promises relay, snapshot, import, and team round trips, anddesktop/src/features/agents/AGENTS.mdrule 16 requires shared events and restart snapshots to carry the persona-owned command alias. Persist the portable alias, not a machine path, and preserve unavailable aliases as compatibility state.
CI is broadly green, including Windows Rust, and git diff --check is clean. Those checks do not exercise launching a .cmd wrapper or reject arbitrary native command input; the current snapshot tests affirmatively pin the contradictory exclusion.
wpfleger96
left a comment
There was a problem hiding this comment.
IMPORTANT — Correctness: persona_from_event carries the projected acp_command, but apply_inbound_persona() does not assign it when reconciling an existing local persona. As a result, an ACP command edit published from one device is parsed on another device but leaves that device's personas.json unchanged, so linked agents continue spawning through the stale command.
Please copy inbound.acp_command in the existing-record merge and add an inbound-reconciliation regression test proving that the command updates while local-only fields remain preserved.
08f66ff to
5da45b7
Compare
|
🤖 I’m Larry. I repaired and independently reviewed the complete PR at 5da45b7, not only the inbound merge. Review responses
My whole-PR pass also found and fixed two lifecycle defects: ACP-only dropdown changes did not mark the persona editor dirty, and normalized stock definitions could leave a stale wrapper on linked instances at restart. Tests now cover the actual save→publish→catalog workflow and stock reset at the shared snapshot-apply boundary. EvidenceAt this exact clean head on Blox
Evidence archive: New-head CI is running. The earlier change-request reviews remain active and need their reviewers’ re-evaluation; I have not dismissed them or enabled auto-merge. Originating Buzz discussion: buzz://message?channel=92a13e97-704d-43a4-8f5f-d0addead218d&id=6afaba82be0fa3a678212ef5cf6e560c12e25bd1b1dbe98731c5d0f3ab836d43 |
|
🤖 I’m Larry. Final remote verification at 5da45b7: the main CI matrix and separate relay-driven mesh lifecycle smoke passed. There are no running or failed checks. The Codex security review itself was skipped and still requires the exact authorization requested by its bot; a green authorization job is not a completed security review. Re-review requests remain with wesbillman and wpfleger96. I have not dismissed their reviews or enabled auto-merge. Local/Blox evidence and independent review results are in the response above. |
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@nostr> Signed-off-by: Logan Johnson <loganj@squareup.com> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Signed-off-by: Logan Johnson <loganj@squareup.com> Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Signed-off-by: Logan Johnson <loganj@squareup.com> Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Signed-off-by: Logan Johnson <loganj@squareup.com> Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Signed-off-by: Logan Johnson <loganj@squareup.com> Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Move ACP transport selection to the persona definition so create and edit flows preserve it through relay events, snapshots, and deployment. Keep legacy and unavailable commands compatible. Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Apply the transport projected by a kind:30175 update while retaining device-local persona identity, lineage, timestamps, and secrets. Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Carry conventional transport aliases through catalog and agent/team snapshot projections and adoption; reject nonportable foreign inputs without restricting owner-native custom commands. Mark ACP-only editor changes dirty and reset stale instance wrappers when the definition returns to stock. Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com> Signed-off-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Make shared stock resets explicit and preserve redacted owner-local overrides on replay. Document the wire contract and exercise shared publication, catalog consumption, and owner reconciliation. Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Supply the provider credential fixture, mirror command persistence and catalog projection, and assert the published alias as well as the saved value. Clarify sharing and local drift hashing in NIP-AP. Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com> Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
5da45b7 to
0928887
Compare
|
🤖 Larry: Rebased onto Only two mechanical conflict resolutions: retain both Tauri formatting and |
jedwards27
left a comment
There was a problem hiding this comment.
Review verdict: request changes
Reviewed base c3132c3ee982d194cd0198ad07b57ec8bd726e4e through exact live head 0928887ae34bc865f96fd2d04eeb762123630a92, integrating both assigned lanes. The implementation itself cleared both lanes, but the live PR is currently CONFLICTING / DIRTY against its target branch, so the required integration gate cannot pass at this head.
Author action: rebase or merge the live base and resolve the conflict without changing the reviewed discovery→persistence→launch contract. The resulting new head must be re-reviewed and required CI rerun; this exact-head clearance does not transfer.
Reviewed behavior
No code/product defect was found in alternate ACP discovery, custom-value preservation, persona/instance persistence, or launch resolution. Discovery and process spawn use the same resolver, and ownership/projection boundaries remained intact. Exact-head Desktop tests passed 5,803/5,803 and typecheck passed.
Local native execution was unavailable because the disposable reviewer checkout lacked the packaged Tauri sidecar; native packaged/visual/AX coverage remains a reviewer/release confidence gap, not additional author work.
— :bot: Jude’s code review agent
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: c3132c3ee982d194cd0198ad07b57ec8bd726e4e..0928887ae34bc865f96fd2d04eeb762123630a92 (exact head 0928887ae34bc865f96fd2d04eeb762123630a92)
Risk: high — this changes local process discovery/execution selection plus the persona, snapshot, team-catalog, and renderer↔Tauri persistence contracts.
Behavior/contracts traced: executable buzz-*-acp discovery through the same resolver used by spawn; Windows shim handling; create/edit state and IPC payloads; persona-owned vs definition-less instance behavior; signed owner reconciliation; catalog/team/snapshot portability and redaction; legacy absence/custom-value compatibility; managed-agent minting; and final Command::new launch resolution.
Findings: no blocking code, product, security, accessibility, persistence, or integration defect found. Persisted unavailable commands remain visible without being silently replaced; portable aliases cross intended sharing/import boundaries while machine-local command values remain local; shared stock reset is explicit; and discovery itself does not execute candidates.
Author action: none for the reviewed code. GitHub currently reports the PR as conflicting (mergeStateStatus: DIRTY), so the branch must be rebased/resolved before merge. Any resulting head change expires this approval and requires delta review and fresh required CI.
Verification owner: integrator/reviewer for the conflict-resolution delta; CI/release gate for required exact-head builds; reviewer/tooling for the remaining native journey.
Validation:
- PASS
git diff --check c3132c3ee982d194cd0198ad07b57ec8bd726e4e HEADat clean exact head. - PASS
just desktop-test: 5,803/5,803, independently reproduced at the exact head. - PASS
just desktop-typecheck, independently reproduced at the exact head. just desktop-ciattempted locally at the clean exact head: the 600 s reviewer limit terminated the Tauri test lane while still running; no test failure had been reported before termination. This is a reviewer time-budget gap, not an author defect.- Exact-head required GitHub checks pass, including Desktop, Desktop Build (macOS), Desktop E2E Integration, Desktop E2E Relay, Rust Lint, Windows Rust, DCO, and Desktop Release Candidate.
- Live PR head rechecked immediately before review: still
0928887ae34bc865f96fd2d04eeb762123630a92; authenticated reviewerjedwards27, authorloganj.
Manual/native evidence: the PR includes create/edit walkthrough screenshots. This review did not independently execute a real packaged wrapper-discovery → selection → launch journey or native focus/AX/zoom pass.
Residual risk: real filesystem discovery and packaged-process launch are joined in source by the shared resolver but remain unwitnessed end-to-end here; native focus return, narrow/zoom layout, and discovery failure/loading appearance are also unobserved. These are reviewer/tooling confidence gaps, not author-actionable defects. The current merge conflict is an external integration gate and must be resolved before merge.
— :bot: Jude’s code review agent
🤖 I’m Larry.
Summary
Managed-agent definitions can now choose which installed ACP transport will launch the agent. The choice sits beside Agent harness while creating or editing an agent, so it is made before first deployment and remains visible afterward.
Buzz discovers executable commands named
buzz-*-acp, keeps Buzz ACP (default) as the safe default, and preserves existing custom command values. Discovery and launch share the same resolver, so a command offered in the picker is the command Buzz will execute.UX walkthrough
1. The ACP command picker appears while creating the agent
The default is explicit, and the helper text explains that this selection controls deployment.
2. Installed wrapper commands are offered by name
Here Buzz has discovered
buzz-janet-acpalongside the stock transport.3. The wrapper is selected before deployment
4. The deployed profile reports the effective command
5. Editing the deployed agent reopens its definition with the choice preserved
Details
buzz-acp..exe,.cmd, and.batshims, deduplicates aliases, and returns a stable sorted list.