fix(projects): allow owners to delete agent projects - #6533
Conversation
Co-authored-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes for one blocking user-visible authorization defect:
P1: Do not expose managed-agent deletion unless the production signer can exercise that authority. canDeleteProject treats every locally managed agent record as sufficient capability via managedAgentPubkeys.has(owner), including legacy/imported records that legitimately have no NIP-OA attestation. But deleteProject always uses signRelayEvent, which signs with the current human identity. The relay accepts that human-signed tombstone only when its database already contains the NIP-OA owner mapping. For a locally managed legacy/imported agent without that mapping, the UI enables Delete and confirmation deterministically fails with must be event author.
Either sign this narrowly constrained tombstone with the managed agent key (the existing project_owner_identity path demonstrates that capability), or stop treating local management alone as deletion authority and expose the action only for relay-verifiable ownership. Please also add coverage through the production signer/relay authorization boundary; the current injected signer/publisher test stubs away the rejection.
The relay authorization, tombstone race handling, and project read-model suppression otherwise look sound at a36f7a4cfca6c66c54a80e4586c130f490031738.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: f7942167372501576c9f0f589cf2c166882668bb..a36f7a4cfca6c66c54a80e4586c130f490031738
Risk: critical — destructive, identity-bound relay authorization and failure recovery.
Two material issues remain:
-
P1 — bind the Delete affordance to authority the production signer can exercise.
desktop/src/features/projects/projectDeletion.ts:34-39treatsmanagedAgentPubkeys.has(owner)as deletion authority, althoughuseProjectDeletionAccess.ts:17-25loads that machine-local list without binding it to the active identity. The operation then signs with the current human identity (projectDeletion.ts:79-84,100-107), while the relay accepts that signer only when its community database says the human owns the agent (crates/buzz-relay/src/handlers/side_effects.rs:249-258). The existing ownership helper explicitly documents that local managed-agent records can diverge from relay ownership (desktop/src/features/profile/lib/identity.ts:134-148). Thus an imported/legacy agent without NIP-OA ownership—or another human identity on the same installation—gets an enabled Delete action in Projects and the sidebar, then deterministically receivesmust be event author. Remove local-list presence as capability evidence, or perform this constrained deletion with authority that actually owns the author agent. Add a regression spanning the production signer/relay authorization seam; the injected signer/publisher test cannot catch this mismatch. -
P2 — reconcile the project cache when publish acknowledgement is uncertain.
RelayClient.publishEventmay time out after the relay has durably accepted the tombstone (desktop/src/shared/api/relayClientSession.ts:710-755).deleteProjectthen exits before its verification fetch (desktop/src/features/projects/projectDeletion.ts:103-113), anduseDeleteProjectMutationremoves/refetches only inonSuccess(desktop/src/features/projects/hooks.ts:964-975). Both confirmation flows close on failure (desktop/src/features/projects/ui/ProjectCards.tsx:424-428,desktop/src/features/sidebar/ui/SidebarProjectsSection.tsx:238-260). The result is a failure toast plus a still-actionable project from the five-minute cache; retry reports that no live head exists, still without reconciling the durable outcome. Invalidate/refetch the projects query on uncertain failure/settlement and cover accepted publish + lost ACK → deleted project absent. The timeout copy should not assert failure when the outcome is unknown.
The relay's community-scoped coordinate authorization, timestamp-dominating tombstone, project-only target, concurrent replacement detection, and the AlertDialog destructive confirmation looked sound in the reviewed paths. No unrelated schema, identity-storage, or release scope was introduced.
Validation at exact clean head: just desktop-typecheck passed; full just desktop-test passed (5,358/5,358); cargo check -p buzz-test-client --tests passed; git diff --check passed. CI relay/integration, smoke E2E, macOS build, Rust lint, security, cross-compile, and Docker jobs passed when checked. The Unit Tests job failed in sherpa-onnx-sys native static-library discovery after buzz-core passed 307/307; Desktop Core and Windows Rust were still running.
Residual risk: the ignored live-relay E2E and a native destructive UI journey were not run. Existing E2E does not cover the production Desktop signer or lost-ACK reconciliation.
Co-authored-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…project-deletion Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Co-authored-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Addressed the requested changes in
The failed Unit Tests job was unrelated infrastructure ( |
Summary
Testing
a36f7a4cfcargo check -p buzz-test-client --tests