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 ( |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: COMMENT — the two previously blocking code defects are resolved at this SHA, but merge clearance is withheld pending a mergeable branch and clean required CI.
Reviewed: f99532585a0715bac73b4a6361a9b4966bdb5095..6c40c2f0bc48d5153ffc03eaa25adbf5dbf3c353 (exact head 6c40c2f0bc48d5153ffc03eaa25adbf5dbf3c353)
Risk: critical — destructive, identity-bound relay authorization with uncertain network outcomes.
The integrated re-review clears both prior findings:
-
Signer authority is now truthful.
desktop/src/features/projects/projectDeletion.ts:26-37grants Delete only to the coordinate author or the human identified by the author’s cryptographically verified NIP-OA profile. Machine-local managed-agent presence is no longer capability evidence. Both Projects and sidebar use this decision (ProjectsOverviewItems.tsx:77-88,104-119;SidebarProjectsSection.tsx:124-160,310-335). The profile owner derives from verified NIP-OA evidence (desktop/src-tauri/src/nostr_convert.rs:64-79) and matches the relay’s community-scopedis_agent_ownerauthorization (crates/buzz-relay/src/handlers/side_effects.rs:237-259). The negative regression atprojectDeletion.test.mjs:28-34rejects an unverified viewer. -
Accepted deletion with a lost ACK now reconciles. Timeout copy reports an uncertain outcome (
projectDeletion.ts:101-105), whileprojectDeletionMutation.ts:9-23returns anonSettledinvalidation for success and failure. The active-observer regression atprojectDeletionMutation.test.mjs:19-47proves the rejected mutation triggers a second fetch and replaces stale[project]state with[]; independent mutation checks showed the regression fails whenonSettledis removed.
The remaining destructive path is bounded correctly: the tombstone targets only project.projectAddress, dominates the fetched live head, and detects a surviving concurrent replacement (projectDeletion.ts:40-51,83-111). Relay authorization remains tenant-scoped, and the read model applies relay-accepted tombstone timestamps (projectModels.ts:389-427). The confirmation UI remains an AlertDialog with explicit irreversible scope, Cancel, destructive action, and pending disablement (ProjectCards.tsx:373-438; SidebarProjectsSection.tsx:395-447). The 13-file diff introduces no schema, migration, identity-storage, or release changes and remains aligned with VISION.md and VISION_PROJECTS.md ownership/tenant boundaries.
Exact-head validation:
just desktop-test— PASS, 5,400/5,400.just desktop-typecheck— PASS.- Targeted authority and lost-ACK regressions — PASS, with both material fixes mutation-proven.
cargo check -p buzz-test-client --tests— PASS.git diff --check f99532585a0715bac73b4a6361a9b4966bdb5095..HEAD— PASS.- GitHub’s relay E2E, backend integration, Desktop integration, Desktop Core, macOS build, Rust lint, security, cross-compile, and release-candidate checks passed at this head.
Integration blockers: GitHub still has red Unit Tests, Desktop Smoke E2E (4), and aggregate Desktop checks. The unit failure is unchanged buzz-voice native discovery (sherpa-onnx-c-api missing); smoke shard 4 has one persistent workflow-controls failure plus seven workflow/virtualization flakes, outside this PR’s changed paths. GitHub earlier reported the branch dirty; final freshness queries returned mergeability unknown, not proof of a clean merge. Rebase/resolve as needed, rerun required checks, and refresh review on the resulting head. Naturally, GitHub chose necromancy instead of a stable answer.
Residual risk: the ignored live-relay owner-deletion E2E and a native destructive GUI journey were not run. Confirmation focus/rendering and a real transport-level lost-ACK are therefore not newly runtime-proven; deterministic state-machine coverage and passing relay/integration lanes are the available evidence.
Summary
Testing
a36f7a4cfcargo check -p buzz-test-client --tests