feat(profiles): archive, unarchive and delete agents from the profile pane - #256
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at 2260c4f961d30cc62588f49d37490838fbd1a70f: one P2 concurrency defect, detailed inline. Serialize overlapping Archive/Unarchive and Delete, or explicitly report an incomplete Delete; add a held-request regression.
Existing CI run 36098204964 also fails the required gate: 114 functional browser failures across four shards plus a measurement failure. The new MentionPicker → useArchivedPredicate → archives.ensure() path requests the relay-author kind-13535 snapshot, which the strict browser fixture does not model. Repair the fixture/query-demand contract without suppressing unexpected-query assertions, then pass the affected browser gates. The ownership-signature helper does not issue this query.
Source-only review on the pinned Blox host covered relay authority/confirmation, discoverable-channel removals, cancellation, native-last deletion, credential custody and mention filtering. Existing hosted Vitest: 3741 passed; Node: 131 passed/2 skipped; Rust: 156 passed/4 ignored. No PR code or tests were executed for this review. Live relay/native process stop, OS-keychain deletion, restart recovery and packaged/Windows acceptance remain unverified; the documented undiscoverable-membership and best-effort key limits are not blockers.
| await removeAgentFromChannels(session, pubkey, signal); | ||
| signal.throwIfAborted(); | ||
| // A failed archive shows its own failure text and Archive retry. | ||
| if (!(await archive(signal)) || signal.aborted) return; |
There was a problem hiding this comment.
[P2] Do not silently abandon Delete behind an in-flight archive request
Delete remains enabled while Archive/Unarchive is pending because its disabled state only observes the native controller’s busy flag. Hold an Archive request before publication, then confirm Delete. Channel removals can finish, but the fresh archive read still says not archived, so the callback calls submit("archive", signal). ProfileAgentArchive.tsx:106 returns false immediately because the other request owns request.current. This line silently returns, bypassing the error handler and native deletion, and resets the Delete button. The user gets no incomplete-delete message although memberships may already be removed and the local agent is still present/running; the other request’s eventual “Archived on this relay” message does not explain that Delete was skipped.
Serialize these controls before admitting the destructive flow, or make the busy refusal an explicit Delete failure. Add a regression that holds the archive request while attempting Delete and verifies either no removals start, or the incomplete Delete is clearly reported and retryable. Cover the reverse overlap if the controls remain independently actionable.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one remaining P2 in Archive/Delete serialization across profile close/reopen (inline). The mounted-overlap repair and strict archive-query fixture repair address the previous findings.
Merge criteria: preserve admission fencing until native Delete settles, including after remount, and add the held-delete close/reopen regression described inline. No cross-session lock or broader lifecycle redesign is needed.
Validation: source-only re-review at 29c90fd173340f5eb7ac6b353a0e10460487465e against df7b7e7f45739f3e06e12d81623385701acdc51d. Existing CI run 36136064129, attempt 2, now passes JavaScript, Rust, all Chromium/WebKit shards, browser measurements and CI required. Attempt 1's warm-switch measurement failed; I did not initiate the rerun. Windows was skipped. Live relay/native deletion, OS keychain behavior and restart recovery remain unverified; no PR code or tests were executed for this review.
| ): Promise<boolean> { | ||
| if (request.current || owner?.aborted) return false; | ||
| if (owner ? operation.current !== "delete" : !!operation.current) | ||
| return false; |
There was a problem hiding this comment.
[P2] Keep the Delete admission fence across profile close/reopen
This guard only checks the current component's operation ref. After Delete confirms archive and enters control.delete(target.id), closing the panel aborts the presentation controller and unmounts this ref, but the native command continues. Reopening the same agent creates an empty slot and an enabled Unarchive agent button. The verified owner can then publish 9036 before native deletion settles, leaving the local record removed but the relay identity unarchived.
The app-owned AgentControl already retains busy=true while the native promise is held; only the nested Delete button currently observes it. Include that existing state in Archive/Unarchive's disabled rendering and synchronous submit admission (or an equivalent fence that survives remount). Clear it when the operation settles; no cross-session lock is needed.
Please extend the held-native-delete regression to unmount, reopen the same profile with the same session/control, attempt Unarchive and assert no 9036/signing before release, then verify the identity stays archived and actions recover after settlement. The current tests separately cover mounted overlap and late completion after close, but not their combination.
7fb1d92 to
133c141
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No remaining code blockers found in this re-review. The prior close/reopen finding is addressed: Archive/Unarchive observes the app-owned controller’s busy state and checks it synchronously before submission. The held-native-delete remount and already-open confirmation regressions cover the agreed admission fence without adding another lifecycle owner.
Reviewed 133c141ff1a83a7c8152cccdc5cf10ceda5f9edd against target base 92c2fb2a61200d2afbc60349a7e55ccfb233161b, focusing on changes since 29c90fd173340f5eb7ac6b353a0e10460487465e. Source-only tracing and an independent regression review; no local tests or PR code executed.
Remaining gates: the latest inspected CI run was still running JavaScript, Rust, Chromium/WebKit and measurements; Windows was skipped. Live relay/native deletion, OS keychain/process stop and restart recovery remain unverified. This is a comment, not approval; required CI and human approval remain outstanding.
133c141 to
0e48ab0
Compare
0e48ab0 to
efd28f8
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No remaining code blockers found in this re-review. Compared the complete previously reviewed series at 133c141ff1a83a7c8152cccdc5cf10ceda5f9edd with the rebased head efd28f8a4dd416a2454f566d644316744441acf5 against 01523e2c1781da1cd84620e61188df6c3d365938, including an independent integration challenge. The final resolutions preserve the remote-delete guard and runtime configuration fields, register both native commands, and retain the shared Archive/Delete admission fence across profile remount.
Validation: existing CI run 36141572922, attempt 1, passed JavaScript, Rust, all Chromium/WebKit shards, browser measurements and the required gate. Its synthetic merge has the pinned base/head as parents and exactly the head’s tree. Review was source-only; no PR code or tests were executed and no CI rerun was requested.
Remaining gaps: live relay/native deletion, real OS-keychain/process-stop behavior, restart recovery and Windows acceptance remain unverified. This is a comment, not approval; required human/code-owner approval still applies.
efd28f8 to
8e38eb4
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review (via Wes’s account)
No new actionable source-demonstrated defects found in this follow-up. This is a non-blocking source review, not approval or merge authorization.
- Head:
8e38eb4da82679ae9e0697ce49a6350c9218bd37 - Base:
fff36cc7e119d870f689f8ed9d550c08ff27dbd9 - Merge base:
4e0ec5cef55a83c7f3ebb7a96ba31aecb815356c
Checked
- The post-review integration now passes the selected native record’s revision through profile Delete → shared controller → native
agent_control_delete. Native deletion checks revision/remote deployment before stop/key removal, and remains last after the profile’s relay roster and archive confirmations (ProfileAgentDelete.tsx:67–94,control.ts:428–436,runtime.rs:460–482). - Earlier Archive/Delete admission and held-native-delete/remount repairs remain in place, including the synchronous controller-busy check before archive submission (
ProfileAgentArchive.tsx:124–172). The associated regression assertions still cover both already-open confirmations and closing/reopening during native deletion. - Traced native failure back to the retained snapshot and the existing Retry status control in
ProfileAgentActions.tsx:99–114; a successful refresh restores the shared controller’s write admission. Also checked the fresh archive authority/confirmation path, per-channel removal confirmation, and mention filtering’s unknown-state/self exceptions.
Scope: the profile feature and affected rebase integration, compared with the previously reviewed efd28f8a4dd416a2454f566d644316744441acf5; not an exhaustive audit of unrelated incoming main changes. The documented non-atomic relay/key/store sequence, undiscoverable private memberships, and failed-delete restart behavior remain stated limits, not newly raised findings.
Validation limits
Source only. Pinned Git-object extracts were byte/hash verified; no dirty checkout inputs were used, and merge-base→head git diff --check passed. I did not execute PR code, tests, builds, installs, or the app, and did not assess current CI. The PR’s reported test passes are not independently reproduced here. Live relay/native deletion, real OS-keychain/process-stop behavior, restart recovery, Windows behavior, and the mounted native-failure → Retry status → Delete journey remain unverified; source tracing is not runtime acceptance.
… pane Add base Buzz Archive agent / Unarchive agent and Delete agent actions to the profile pane, gated on verified NIP-OA owner or relay owner/admin authority. Delete confirms channel removals and the archive on the relay before the native delete, which is the shared revision-checked agent_control_delete. Native delete now refuses deployed remote records, and the profile pane offers no Delete for them. Co-authored-by: Kalvin Chau <kalvin@block.xyz> Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz>
8e38eb4 to
ff39c15
Compare
…-image * origin/main: (23 commits) fix(agents): recover status polling and scope failure diagnostics (#283) Share avatar editing across community profiles and managed agents (#271) feat(profiles): archive, unarchive and delete agents from the profile pane (#256) ci: run browser journeys on three shards per engine (#280) ci: publish scheduled macOS test prereleases (#262) feat: add private text feedback plugin (#242) 🤖 docs: add pre-PR checklist and review guidance to AGENTS.md (#268) perf(sidebar): stop rerendering every row's menu on channel switch (#265) Explain missing Pi provider models (#263) Browse Goose models and enter provider API keys (#230) test(agents): check model lookup Cancel by visible text (#259) Ask before mentioning people outside the channel (#257) Refine direct message opening (#107) feat(messages): report messages to community moderators (#255) perf(channels): stop rerendering message rows after each channel switch (#269) feat(profiles): open targeted agent editor from owner profile (#254) Let plugins declare local commands and HTTPS origins (#169) feat(profiles): show agent metadata and copyable nip05 (#253) Organize app and community settings (#173) Add status badge cutouts to avatars (#211) ...
…oser Main gained two mention changes while this branch was open: archived identities leave choices but never hide the viewer (#256), and prose after an unknown name closes the menu without extra directory reads (#303). - One archivedMention rule now owns the viewer exemption for candidates, disabled installed rows and the send-entry guard. The old per-surface predicate is removed. - The directory keeps #303's exhausted-prefix refutation. Only non-empty pages are cached, so an empty result is refuted by prefix evidence and a fresh search for the same query reads again. - Inline completion withdraws its menu for prose once local and directory evidence settle. - Tests from main follow this branch's rules: shown rows stay in place while disabled, and public keys match no choice. Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
* origin/main: feat: attach sanitized image and opt-in diagnostics to feedback (#245) test: repair three baseline Vitest failures (#276) fix(agents): recover status polling and scope failure diagnostics (#283) Share avatar editing across community profiles and managed agents (#271) feat(profiles): archive, unarchive and delete agents from the profile pane (#256) ci: run browser journeys on three shards per engine (#280) Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentEditor.tsx
Change
agent_control_delete(id, expectedRevision)(also used by Agents → My agents). It checks the revision, refuses deployed remote records, stops the local process, disables the record, deletes the app-owned key, then removes the record.agent_control_deleteis granted to the main webview only.docs/agents.mdanddocs/agent-control.md.Verification
pnpm check: pass.main, with the same three unrelated failures (PluginImport, ProfileAgentIdentity no-hint read count, MessageRow bylines).cargo test -p buzz-agent-controller: 66 passed, 1 ignored; 11 passed.agent-activity.spec.mjs(chromium): 16/16.