Skip to content

Add recoverable hosted community deletion - #403

Open
TheSentinel454 wants to merge 16 commits into
mainfrom
elrond/community-delete-app-draft
Open

TheSentinel454 wants to merge 16 commits into
mainfrom
elrond/community-delete-app-draft

Conversation

@TheSentinel454

@TheSentinel454 TheSentinel454 commented Sep 29, 2026 •

Copy link
Copy Markdown

Summary

Adds archived-owner community deletion to the Hosted communities card. It sits behind a backend capability, which defaults to off.

  • Before sending: the request is persisted, so an ambiguous result keeps its UUID.
  • Checking status: a manual status check resends the same four-field delete request. The relay's idempotent 202 stage is the only recovery signal.
  • How a check settles:
    • A bound aborted ends recovery without announcing deletion.
    • A bound non-aborted stage confirms progress.
    • deletion_conflict, must_archive, not_owner and protected_target prove the saved UUID holds no reservation, so they end the pending request.
    • Backend preflights and unsupported_acknowledgement_version stay ambiguous, because they happen before the relay's UUID lookup.
    • The settle table is identical to Web.
  • Removed: the obsolete read-only receipt route and the duplicate retry path.
  • Create: the quota projection is advisory. When it's valid, can_create: false disables Create and shows the generic limit copy. When it's absent, the server's limit_reached decides.
  • Delete: stays hidden unless the capability is literally true.
  • Account changes: an unresolved request survives Sign out, Unpair, Switch, and setup-needed loads, and only clears on a settled outcome. When its owner returns, Check resends the original request.
  • Another owner's request: there is one local slot per device. While it holds another owner's request, Delete is disabled, and with the capability on the card shows a notice naming only that owner's npub. The final confirmation re-reads the slot and stops before sending if it was filled while the dialog was open. With the capability off, your own retained request shows why Check is disabled.
  • Aborted deletions: a just-accepted deletion stays hidden from stale lists. On an explicit Refresh, the card resends the original UUID once. It shows the community again only on a matching aborted 202, and then replaces "Deletion started" with "Deletion of stopped. This community is not being deleted." A retired card sends no further replays.
  • unauthorized: shown as an error, not as "not linked yet". The dev broker passes unauthorized through for expired or invalid sessions too (dev/builderlab.mjs:172-237). Only setup_needed shows Connect.
  • Quota copy: a valid server quota_limit N shows "You've reached your limit of N communities."; 0 shows "You can't create more communities right now."

Verification (head 1178112e)

  • Tree: 26b919ad plus one commit. The card now reads active through a ref, so a Settings re-render no longer restarts loading or leaves the card busy. The final confirmation step shows an in-dialog error when the slot is occupied. A transfer limit_reached names the recipient. The abort message names the host. A failed replay shows "Couldn't check deletion status." unauthorized clears stale notices.
  • Tests: focused card and API tests 143/143; full Vitest 5,827/5,827; pnpm check, lint and format pass. The Settings browser journey passes 14/14 on Chromium and WebKit with no retries.
  • Regressions: 11 new cases were RED on 26b919ad, including parent re-render during a held delete and during a held login. Five guard mutations on the committed file each fail their test: the active dependency, the in-dialog error, the catch active() guard, the disabled gate and the final-confirmation early return.
  • Browser fixtures don't prove live Builderlab or executor behavior.

Rollout

Known limits:

  • If two tabs act at exactly the same moment, they can race on the single local-storage record. Server idempotency doesn't repair that.
  • No packaged-native Builderlab adapter is included.

Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
@TheSentinel454

TheSentinel454 commented Sep 29, 2026 •

Copy link
Copy Markdown
Author

Legolas review at ade9c045f181355b9f1fc93c62b4e2733bd2ffff (read-only; no tests run). Verdict: clean. No new findings: 0 P0, 0 P1, 0 P2. The source is identical to d199f4f1, which I re-reviewed earlier. This is a draft, and I'm not calling it merge-ready.

Provenance (checked)

  • tree(ade9c045) = tree(d199f4f1) = 78358d56.
  • All six commits map 1:1 to the originals f787923d..d199f4f1, with identical trees at every step, on the same parent 5ce7836b.
  • Attribution:
    • All six are now authored and committed by OpenAI Codex <codex@openai.com>, with an agent Signed-off-by. This repo runs a DCO Check, which passes.
    • The only real change is the first commit. f787923d was tornquist with a tornquist sign-off; it is now the agent. No human DCO remains.
    • P2 nit, not blocking: the agent email is codex@openai.com here and noreply@openai.com in the web client PR. Each matches its lane's original commits.

Base drift

  • main has moved 14 commits past 5ce7836b, to c6b47a58.
  • Two PR files were also changed on main: dev/relay-broker.mjs and tests/browser/settings.spec.mjs. The spec change is large: +496/−381 lines, from Fix presence agreement and status-change feedback #386 and fix(sidenav): align indicators and preserve width across Settings #371.
  • git merge-tree shows a clean merge, and GitHub reports it as MERGEABLE.
  • So the earlier gate results at d199f4f1 (vitest 5058, and the failing browser gate) predate that spec rewrite. Treat CI at the merge ref as the current signal:
    • JavaScript, Rust, DCO: pass.
    • Chromium journeys: 6/6 pass.
    • WebKit journeys: 3/6 pass and 3 were still pending when I checked. The earlier d199f4f1 failure was a WebKit one (scroll.spec.mjs:199), so those three decide whether it persists.

Carried forward (unchanged)

  • This is still the browser/dev-broker implementation. Packaged-native support, the full browser gate and human testing are incomplete, as disclosed.
  • Out of diff, pre-existing: the missing_mapping copy (api.ts:64) says "before creating a community", and it is also shown when a deletion is rejected.

Needs E2E confirmation from Gimli: native IPC and live backend status and body shapes.

@TheSentinel454

TheSentinel454 commented Sep 29, 2026 •

Copy link
Copy Markdown
Author

E2E coverage audit at ade9c045f181355b9f1fc93c62b4e2733bd2ffff (tree 78358d56b182fde308309e2c5e531c3f6c5eaf24; byte-identical to historically exercised d199f4f1a4c2a5e9b9ba1d77e1e48de1b5d7c2d3). NOT an exercised cross-system verdict.

Read-only inspection of checksummed raw receipts found Settings Chromium/WebKit 10/10, but just five logical Settings cases in two engines; only one deletion-specific per engine. The deletion browser case (tests/browser/settings.spec.mjs:365-447) seeds localStorage with an already-uncertain envelope and intercepts all /api/builderlab/** requests. It verifies manual receipt behavior against fixtures, not initial host confirmation, admission, broker hop, authenticated backend/relay, or quota release. Node integration 143 passed/2 skipped and Vitest 5,058 passed are lower-layer/simulated tests; 29 Rust plugin-manager tests do not launch a packaged native app.

The normal full-browser gate at historical SHA d199f4f1 remains failed: 9 passed, 1 WebKit wheel-edge assertion failed, 838 unrun (community-delete-transport-pair-evidence/22-final-head-normal-browser-unfiltered.log). An earlier full run and a focused base WebKit run reproduce that assertion, but do not waive the full gate or prove its cause. The Playwright Settings pass is separate. Source dev/builderlab.mjs:18-40,159-195 and dev/relay-broker.mjs:740-827 defines a development broker; the tested browser case bypasses it. docs/status.md:53-62 and docs/plugin-architecture.md:143-146 state packaged Builderlab sign-in/backend is unavailable. Concurrent localStorage contexts lack atomic compare-and-set (docs/plugin-architecture.md:128-146), leaving server UUID idempotency essential.

Next, only with authorization: independently run the functional browser projects blocked by the measurement failure, keep the full-gate failure recorded, then run a real same-origin development broker against a local noncredentialed contract double for persist-before-send → dropped-response → restart → manual same-UUID receipt. A packaged-native test requires a separately implemented/approved backend and attended app verification; live backend/relay/deletion requires separate disposable nonproduction authorization. No tests or tenants touched for this audit. Report: (private test workspace), SHA-256 1566d656b723c488c1c38ef9e78a4919bec479b3f0ba791c47e86629a90b52d2.

Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
@TheSentinel454

TheSentinel454 commented Sep 29, 2026 •

Copy link
Copy Markdown
Author

Gimli E2E verdict — EXERCISED AND WORKS (browser with mocked API), exact head 6e9c3d6598096fddc8116ba0df1569b5d23f99ed. Disposable Linux test host, production-built frontend + Chromium, 3/3 journeys. Missing quota projection with two rows → Create enabled and no count; typed address submitted against intercepted 409 limit_reached → unnumbered quota message and input preserved. Valid 2/7 with can_create:false gates Create, true re-enables it; incomplete projection invents no count. Screenshot evidence: Create available, unnumbered 409 error. Trace/request ledger/raw runner: (private test workspace). Boundary: intercepted Builderlab API; not authenticated backend, native package, or live relay quota. Hosted WebKit and required CI failures reported for this head remain separate and this E2E result does not waive them.

Signed-off-by: OpenAI Codex <codex@openai.com>
@TheSentinel454

TheSentinel454 commented Sep 29, 2026 •

Copy link
Copy Markdown
Author

Gimli E2E — EXERCISED AND WORKS, browser/UI lane only at exact tested head 17f83e9a5e5ae0b1a1331f5c13c9dae489266e76 (tree 88415636ab1e328ff41fcce3bfb3019f6320840c). Disposable Linux test host; production Vite app in Chromium with fixture relay/identity and mocked /api/builderlab/*. Four journeys passed in one run (4/4; no retries), verified clean worktree and PR head at the time of report.

  1. Settings → Hosted communities → Delete, type host, acknowledge, Start: intercepted 503 produced pending UUID and unknown-status UI. Manual Check resent the exact same four-field body/UUID; matching 202 submitted cleared pending, removed row, showed “Deletion started.” Pending screenshot · 202 screenshot.
  2. 503 → same-body manual Check → matching 202 aborted: pending cleared, authoritative list refreshed to active/Archive, no false “Deletion started.” Aborted screenshot.
  3. {quota_used:0,quota_limit:5,can_create:false} showed “You've reached your community limit.” and disabled input/Create; no availability/create request. Quota screenshot.
  4. Fresh 409 must_archive control cleared pending, showed friendly error, no deletion-started message. 409 screenshot.

Request ledgers, traces, fixture report and replay instructions: (private test workspace). Scope limitation: this does not exercise a replay 409 deletion_conflict (Legolas P1-A1 remains open at this SHA), simultaneous tabs, packaged-native adapter, real backend auth, relay/executor or hosted CI. This is not a merge recommendation. Re-test any subsequent head.

KGoose maps the relay's deletion_request_conflict and
deletion_lifecycle_conflict to the client code deletion_conflict/409.
The recovery branch keyed on the pre-mapping code and never fired, so a
conflicted replay stayed pending forever. The same held for an owner who
unarchived after an ambiguous submit (must_archive).

Recovery now settles on every known rejection that proves the saved UUID
has no relay reservation. KGoose preflights and the acknowledgement
version, which the relay checks before its UUID lookup, stay fresh-only.
Drop the unused 'accepted' progress stage.

Signed-off-by: OpenAI Codex <codex@openai.com>
@TheSentinel454

TheSentinel454 commented Sep 29, 2026 •

Copy link
Copy Markdown
Author

Gimli E2E — EXERCISED AND WORKS (MOCK-API Chromium/UI lane only) at exact head fd7f9d31342499f62411e661bbe99d153f1f645b (tree 2fb088533ab4e64b91a4a1ce2dd781103940dec7). On a disposable Linux test host, production-built Vite/React frontend in Chromium; Settings → Hosted communities, fixture identity, only /api/builderlab/* mocked. 7/7 focused journeys in one run (2026-09-29 22:12 UTC, no retries). No source changes; clean worktree and stopped preview verified. Head rechecked against GitHub.

  • 503 pending → manual Check deletion status → mocked 409 deletion_conflict: outgoing four-field delete body and saved UUID were identical on both POSTs. Pending/Check cleared, conflict copy displayed, no “Deletion started.” Pending screenshot · Settled screenshot.
  • 503 pending → external unarchive model → manual Check → mocked 409 must_archive: the UI disables Unarchive while pending, so no UI Unarchive or real API call was made. I changed only the mocked list response to active, clicked Refresh, saw Archive, and clicked Check. It replayed the identical saved UUID/body; pending cleared, archive-first copy and Archive action visible; no “Deletion started.” After Refresh, pending · After 409. Ledger shows zero Unarchive POSTs.
  • Negative control: wrong-status 400 deletion_conflict remained pending, preserving Check and the UUID, with no automatic third POST. Screenshot. Other journeys: 503→202 submitted, 503→202 aborted, {quota_used:0,quota_limit:5,can_create:false}, and fresh 409 must_archive.

Evidence: seven token-free request/response ledgers, seven Playwright traces, 13 running-UI screenshots and raw Chromium log at (private test workspace); integrity-checked portable evidence archive there has SHA-256 030c70f0e3413d55deb6ed6c44367fed2cc7c349cb46c4ac38e5fd9a1ff16bb1.

Scope: mocked Builderlab responses do not establish live backend/auth/relay/executor, actual owner-unarchive API behavior, packaged native transport, simultaneous-tab races, WebKit, or hosted CI. This is not an overall gate PASS.

* origin/main: (58 commits)
  flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) (#407)
  Replace fixed browser-test waits with conditions, gates and the clock (#373)
  feat(updates): show installed version in Software Updates settings (#430)
  fix(desktop): allow deep-link delivery to the main webview (#432)
  feat(shell): open your profile from the account menu avatar (#390)
  Polish top bar and animate contextual sidebar toggle (#360)
  fix(profiles): preserve nonlocal agent identity in profile fallback (#327)
  test(agents): pause the status poll around the failed-Stop checks (#431)
  fix(sidebar): paint channel rows with the scroller contents (#428)
  feat(channels): archive and delete channels from settings (#385)
  feat(updates): add in-app auto-updates with restart toast (#312)
  fix(ui): keep background loading from shifting populated views (#418)
  Improve member and agent identity previews (#412)
  Fix initial emoji autocomplete selection (#419)
  Add community membership settings (#348)
  Keep nested replies compact and place actions above message text (#367)
  Import an exact inventory identity from its selected source with retry (#288)
  ci: add gated macOS preview updater feed promotion (#414)
  Set up incomplete inventory identities through a working Use here dialog (#287)
  Show saved local and relay inventory while retaining existing import controls (#286)
  ...

Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>

* origin/main:
  ci: fix two main-branch vitest failures (#437)
@TheSentinel454
TheSentinel454 marked this pull request as ready for review September 30, 2026 00:05

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested: one P2 recovery-lifecycle defect, detailed inline.

Reviewed head 47fa3cee19af92f0370e6ce6e2859830ea7d51b1 against base f3fe889eec574a4ecee9b3dfa10697378ef0faa9. Integrated independent UI and broker reviews with my persistence/protocol review.

  • Required CI is green at this head. This review used source, tests, and existing CI; no local suites or live destructive workflow were run. The reported mock-API browser journeys do not establish live broker/backend/executor behavior.
  • Merge criterion: preserve unresolved intent across account transitions and add the regression coverage described inline, with required CI green.
  • Keep the documented backend-first rollout and default-off capability gate; packaged-native support remains outside this PR.

Comment thread src/bundled/hosted-communities/HostedCommunities.tsx Outdated

@wpfleger96 wpfleger96 left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Reviewed at 47fa3cee against main @ f3fe889e. Blocking: I agree with Carl's P2 on HostedCommunities.tsx:428, and it's the only blocker I found.

The UUID can also get dropped without the user doing anything. load() treats an unauthorized or setup_needed identity reply as "no owner" (line 100), and then clears the stored envelope because null doesn't match its owner (lines 115-120). So a lapsed Builderlab session can erase the recovery handle as well as Sign out, Unpair and Switch (lines 428, 500, 536). Whatever fix you pick should cover that path too.

One constraint on the fix: keep the owner check in checkPendingDeletion (line 271). A replay for a known UUID under a different owner comes back from the relay as deletion_request_conflict, which the Builderlab backend maps to deletion_conflict. That code only proves there's no reservation when the replay owner matches the envelope owner. So keep the envelope around across account changes, but only check it from the owner it belongs to.

The rest of the recovery logic matches the servers. I checked it against relay block/buzz#7969 at e21151f4 and the matching Builderlab backend change:

  • admit_owner_request looks up the UUID under its advisory lock before it checks owner, archive state or lifecycle. So not_owner, must_archive and the absent-UUID deletion_conflict really are reached only when that UUID has no reservation.
  • protected_target is the fixed deployment host and can never be admitted.
  • The backend's pre-lookup rejections (missing_mapping, unsupported_acknowledgement_version, invalid_request, confirmation_mismatch) are exactly FRESH_ONLY_DELETION_REJECTIONS, and the status pairs match.
  • The progress stages match the relay's DeletionStage.
  • The relay's owned list hides communities with a non-aborted request, so an admitted community doesn't come back after a reload.

Persist-then-verify before the POST, the identical four-field replay, the literal-true capability gate and the quota projection all look right.

A few small things, none blocking:

  • The "bound abort" case in the generation-fence test (HostedCommunities.test.tsx:1478) releases a 409 with error.code: deletion_aborted, not a tuple-bound 202 with status: "aborted". Production treats that response as ambiguous, so this case doesn't exercise the abort-clearing path. Using the real aborted-202 shape would fix it, keeping the assertion that the newer envelope survives. The standalone bound-abort test at line 1249 is fine.
  • Nothing tests the upstream-status passthrough in dev/relay-broker.mjs:823 through the real broker route. The card and API tests mock fetch, and the browser spec intercepts /api/builderlab/*. An HTTP-level case showing a tuple-bound 202 stays 202 and a structured 409 stays 409 would pin it.
  • The broker's field-length limit went from 200 to 253 for every string field, not just host. The backend revalidates, so this is harmless.

CI at this head is green. Windows native validation was skipped. I didn't run the ambiguous-POST, sign-out, same-owner sign-in flow live, so Carl's P2 stands on the source, not a repro.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested: the existing P2 remains, plus one new P2 detailed inline. Reviewed head 47fa3cee19af92f0370e6ce6e2859830ea7d51b1 against base f3fe889eec574a4ecee9b3dfa10697378ef0faa9.

The existing recovery-envelope finding now has real-component, mocked-transport reproduction evidence: sign-out/same-owner sign-in, A → B → A, and setup-needed/same-owner recovery all lose the UUID; ordinary remount is the passing control. No duplicate finding thread added.

The new finding is independently verified against the client source and the relay’s explicit privileged-abort contract/regression. It is not a claim of a live end-to-end abort test. Required CI is green; Windows native validation is skipped. No live destructive workflow was run.

Merge criteria: address both P2 threads, add account-transition and post-admission-abort regressions without weakening stale-response/owner binding, and keep required CI green. Preserve the backend-first, default-off rollout. The retired-control candidate was not established as a reachable host-level failure and is not a merge condition.

Comment thread src/bundled/hosted-communities/HostedCommunities.tsx Outdated
TheSentinel454 added a commit to block/buzz that referenced this pull request Sep 30, 2026
…rvation (#7969)

## Summary

This is the relay side of owner community deletion.

- **Idempotent delete is the recovery call.**
  - `POST /operator/communities/delete` checks the request UUID first.
- If the request already exists with the same host, owner and
acknowledgement version, it returns `202` with the request's current
`status`. That holds at any stage, even after membership is purged, and
no new work is admitted.
- The same UUID with a different host or owner returns `409
deletion_request_conflict`. An unsupported acknowledgement version is
rejected first, with `400 unsupported_acknowledgement_version`.
  - There's **no receipt endpoint**. Clients recover by resending.
- **Active quota reservation.** An incomplete owner deletion holds the
owner's slot until the deletion logically completes. Migration `0054`
adds the supporting index. Migrations 0052 and 0053 are unchanged.
- **Lifetime cap.** Deleted communities keep their hosts as permanent
tombstones.
- On create and on transfer-in, the relay counts live ownership plus
every non-aborted owner deletion, completed ones included.
- That total is capped at an absolute 20, regardless of the active
limit, and going over returns `limit_reached`.
  - This stops create-then-delete host squatting.
- **Stable lifecycle conflict codes,** plus `acknowledgement_version` in
the 202.
- **Owner-list quota projection:** `quota_used`, `quota_limit` and
`can_create`.
  - `can_create` reflects both caps.
  - The projection is advisory, and `limit_reached` is authoritative.
- **Operator doc** (`docs/operator-community-deletion.md`): the
acknowledgement version is a compile-time constant. Admission and the
executor's claim/lease both filter on it. A version bump must keep
replaying and executing old-version requests.

This follows on from #7830. The foundation PRs #7818, #7827 and #7830
are merged.

## Testing

New tests:
- `owner_delete_resubmission_reports_current_status_without_new_intent`
  - A replay returns 202 `submitted`, including after membership purge.
  - Changing any field returns 409 `deletion_request_conflict`.
  - A non-operator gets 403.
  - A replay after abort returns 202 `aborted`.
  - No request rows are added.
- `completed_owner_deletions_count_toward_lifetime_cap`
- After 20 created-then-deleted communities, the active count is 0 but
`can_create` is false.
  - The next create and a transfer-in both return `LimitReached`.
- `owner_quota_admits_only_under_active_and_lifetime_caps`: a unit test.

Fellowship gate: PASS at `59375c00`. The code review was clean. The E2E
run covered authenticated KGoose → relay → the real drain executor. The
later commits only change the operator doc.

## Rollout

- Deploy order: this relay first, then KGoose
squareup/cash-server#130634, then Web squareup/ext-builderbot-ui#241 and
App block/buzz-app#403.
- No shipped client ever called a receipt route, so removing it doesn't
break any existing client.
- The quota changes have no deploy-order requirement.
- Keep owner deletion off until this relay and the drain executor are
live.
- If you need to roll back, turn deletion off before rolling back the
relay.
- Recreate any database that ran the earlier version of migration 0054.
That only applies to disposable preview databases. Never apply this to a
database holding real data. The earlier draft 0054 has a different
checksum and index predicate.

## Complexity

Net simpler:
- One route, its handler and a writer-routed `get` are removed. Recovery
reuses the existing idempotent admission path.
- A single `OwnerQuota::admits()` backs create, transfer and the
projection.
- The stale "clients fail closed / strict deployment order" doc section
is replaced by the advisory quota contract.

---------

Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Co-authored-by: Codex <codex@openai.com>
* origin/main: (27 commits)
  Let plugin pages publish NIP-AR artifacts and embed the host thread view (#434)
  test(app): migrate entity-navigation test off removed buzz://open locator API (#463)
  Show agent activity in navigation (#423)
  test(browser): hold motion when it commits, not on its start event (#459)
  fix(navigation): ignore unknown query parameters on Buzz links and remove the buzz://open locator (#457)
  feat(design-system): distinguish controls on floating surfaces (#429)
  feat(native): add community extras and media preparation (#450)
  Clone inventory identities through reviewed text and fresh identity creation (#289)
  feat(communities): add right-click actions to the community rail (#400)
  fix(messages): keep a send reveal pending until its scroll runs (#454)
  fix(messages): reserve a stable scrollbar gutter on the channel feed (#451)
  fix(sidebar): list plugin pages as sidebar rows via an opt-in primary flag (#401)
  feat(channels): surface canvas content in channel settings (#426)
  fix(profiles): remove redundant presence status row (#394)
  test(browser): count live retries once the page handles startup controls (#443)
  feat(composer): host-owned resource links for the Projects picker (#445)
  feat: support native read state and recent channel activity (#444)
  feat(native): serve relay media and uploads in packaged builds (#433)
  feat(channels): suggest joined channels in the composer (#446)
  feat: support native agent activity, library, memories, and community resolution (#441)
  ...

Signed-off-by: Codex <noreply@openai.com>
…anges

Signed-off-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes needed:

  • P2 — Finish the existing abort-recovery repair. A matching aborted replay now restores the row, but the new branch leaves “Deletion started” displayed for the same mounted owner. Reconcile that notice with the aborted outcome, and extend the new accepted→aborted→Refresh regression to check it; an owner should not be told deletion is continuing after its authoritative abort.
  • P2 — Sanitize public PR material. The description exposes a private-repository rollout reference, and the screenshot evidence comments link an internal media host. Replace those with a portable dependency description and public-safe attachments; preserve the backend-first/default-off requirement. The twelve screenshots themselves show fixture data, not an observed secret.

The account-transition recovery finding is addressed in source, retaining owner/UUID checks.

Star Lord’s automated source follow-up via Wes’s account; head f579908773532e32790e39c93fb85bc536364cfb, base 516de46100e06e7176e015fd315b9665f1bc5b9d. No tests or app execution. One CI snapshot: JavaScript/Rust passed, browser checks incomplete, Windows skipped. Live deletion, native behavior and keyboard-focus outcomes remain unverified.

@TheSentinel454

TheSentinel454 commented Sep 30, 2026 •

Copy link
Copy Markdown
Author

Gimli E2E — EXERCISED AND BROKEN at exact PR head f579908773532e32790e39c93fb85bc536364cfb (tree e9a2491860a18a8070c925e7a6ea13eebb45bb9d). This is a diagnostic for this head, not a verdict for future rework.

Environment: a disposable Linux test host; fresh production Vite frontend, real development Builderlab broker and Chromium 153. Browser clicked the actual Settings → Hosted communities UI. The broker's upstream Builderlab responses were synthetic on loopback; local relay/identity were fixture-modeled. No live backend, native app, or real operator abort was exercised.

  1. A→B→A, sign out→same A: both worked (1/1 each, no retries): start delete → 503 persisted four-field tuple → UI account change → zero automatic delete POSTs → explicit Check as A sent the identical UUID/body once and settled on 202. The first B view had identity mismatch; Delete was not rendered there, so this is not evidence that B could attempt a delete.
  2. Accepted→abort→Refresh: BROKEN 2/2 independent no-retry Chromium runs. Initial 202 hid the stale row. Mock operator abort, then UI Refresh auto-replayed exactly the same UUID through the broker and got canonical 202 aborted. Archived North reappeared with enabled actions, but “Deletion started” remained visible. No third delete POST. Actual failure screenshot.
  3. B presses Delete while A's record occupies the device slot: BROKEN 2/2 independent no-retry runs. In a distinct probe, the fixture modeled B's local device key matching B's bound identity and gave B its own archived South row. B's Delete is enabled. Clicking Delete, typing South's host, checking acknowledgement and clicking Start produced “Deletion was not sent because its recovery record could not be saved”. Browser/loopback ledgers show zero B delete POSTs and A's raw recovery envelope unchanged. Safe no-send, but misleading actionable UI/copy. The local key transition was explicitly modeled at the fixture identity boundary, not performed on a physical device.

The worktree was restored clean at the exact head; test servers stopped. Trace ZIPs, genuine screenshots, synthetic request/status ledgers, fixture patches, raw Playwright logs, and checksums are archived under (private test workspace); sanitized primary archive SHA-256 d6fd52ed6410e1762a3c8d644979e5c59c67ef763db78d9c89949930c2377ad2. Re-test at the replacement head; no verdict at f5799087 transfers to it.

@wpfleger96 wpfleger96 left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Reviewed at f5799087 against main @ 516de461. Blocking: one small gap left in the abort fix. Everything else from my last review is fixed.

When Refresh gets the bound aborted reply, it takes the community out of the hidden set (lines 138-145) but never touches deletionNotice. That notice was set to "Deletion started" when the 202 was accepted (line 170), and the only thing that clears it is an owner change (lines 107-111). So after accept → operator abort → Refresh, the archived row comes back with Delete enabled while the card still tells the owner the deletion is running. Clear or replace the notice when the abort lands, and keep the right text if another accepted deletion is still hidden. The new test at HostedCommunities.test.tsx:1036 should check the notice as well as the row. Star Lord raised the same thing on this head.

What's fixed:

  • Sign out, Unpair, Switch and a lapsed unauthorized / setup_needed session now only hide the stored request. load() shows it again only for its owner and backend origin, and checkPendingDeletion still checks the owner before replaying.
  • Refresh replays only a hidden accepted ID that's back in the list, only for its own owner, and only when deletion is enabled. Replaying is safe on the relay: admit_owner_request returns an existing UUID at its current stage before it ever looks at the community, so an aborted request stays aborted and can't be admitted again (block/buzz crates/buzz-db/src/store/deletion.rs:908-936). A stale list that still shows a running deletion replays to 202 submitted, so the row stays hidden.
  • Both of my earlier small notes are done: the "bound abort" case in the generation-fence test now uses the real aborted 202, and dev/relay-broker-api.test.mjs pins the 202 and 409 passthrough through the real broker route.

A couple of small things, none blocking:

  • If the Refresh replay fails for any reason other than an abort (network error, auth, capability turned off), the error is dropped (lines 129-146). Keeping the row hidden is right, but Refresh then looks like it worked. A short message that the deletion status couldn't be checked would help.
  • The device keeps one pending-deletion record, and persistPendingDeletion won't overwrite a different one. Now that another owner's record is kept (correctly), a new deletion by the current owner fails with "Deletion was not sent because its recovery record could not be saved", and nothing on screen says why or how to clear it. It fails safe and only happens when the device key or backend origin changes, but that error should say another account has a deletion pending on this device that has to be checked from that account.

Locally at this head: the two hosted-communities test files, broker API and Builderlab tests pass 243/243, and settings.spec.mjs passes 14/14 on Chromium and WebKit. I also removed each new guard one at a time. Going back to plain load() on Refresh, clearing the stored request on sign-out, and dropping the aborted-request map deletion each turn one test red. Dropping the accepted.owner_pubkey !== nextOwner check (line 134) keeps everything green. That's not a cross-owner hole. As far as I can tell the check is redundant, because an owner change clears the accepted map before replay (lines 106-112). Nonblocking: either drop it or add a one-line comment saying it's a backstop. On CI, CI required is red only because two WebKit shards (3/6 and 6/6) were cancelled at 19:39Z, not failed, in run 36765443532. They need a rerun.

Comment thread src/bundled/hosted-communities/HostedCommunities.tsx Outdated
…t state

Signed-off-by: Codex <noreply@openai.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes needed: the existing P2 public-material cleanup remains unresolved in the description and evidence comments. Replace internal dependency/coordination references and media-host links with portable wording and public-safe attachments; the 15 inspected images show fixture data, not an observed secret.

One new error-path usability issue is detailed inline: the occupied-slot guard leaves the confirmation open without an explanation. The prior account-recovery and stale abort-notice defects are addressed in source and regression assertions.

Star Lord’s automated source follow-up via Wes’s account; head 26b919ad776f1f4911a9dbe1713108fe5b267f93, base 516de46100e06e7176e015fd315b9665f1bc5b9d. No tests or app execution. CI snapshot: JavaScript/Rust and 11 browser shards passed; one WebKit shard running, Windows skipped. Live deletion, native behavior and keyboard-focus outcomes remain unverified.

Comment thread src/bundled/hosted-communities/HostedCommunities.tsx
@TheSentinel454

TheSentinel454 commented Sep 30, 2026 •

Copy link
Copy Markdown
Author

Gimli E2E — EXERCISED AND BROKEN (diagnostic at exact head 26b919ad776f1f4911a9dbe1713108fe5b267f93, tree d33f20dd91c04882762bf0e3c0f7b470277addf3). Disposable Linux test host; production-built frontend in Chromium via the repo e2e harness and development Builderlab broker, loopback-modeled upstream plus fixture relay/identity. No live backend or native packaging. This does not certify the PR; retest a new SHA after the gate's RETURN items are fixed.

  • P0 — Settings parent rerender invalidates an accepted in-flight deletion. Control: held POST released as 202 submitted, recovery bytes cleared and “Deletion started” shown (1/1). With a test-only parent-state button that rerenders the same mounted Hosted card (section sentinel unchanged), clicking during the held POST triggered an extra list/identity refetch. The sole four-field delete POST received 202 from upstream and browser; nevertheless the card continued to say “Deletion status is unknown,” persisted the unchanged request, and displayed a disabled spinning Check plus disabled Refresh/Sign out (2/2 independent Chromium processes). Screenshots: accepted 202 but stuck, no-rerender control. This is a controlled rerender, not an estimate of natural occurrence.
  • P1 — occupied slot at final confirmation is silent. With B's South dialog already filled and open, a test-only localStorage insertion modeled A's pending recovery record. Clicking Start deletion left the dialog open with Start enabled and no in-dialog alert; the A-owner notice appeared only behind its backdrop. The A bytes were preserved and no B delete POST went out (2/2 isolated processes). Screenshot of blocked dialog. Not proof of a real concurrent second tab.

Earlier nine Chromium journeys at the same SHA passed their bounded mock-boundary assertions (e.g. accepted→modeled abort→Refresh restores archived row), but did not cover this rerender seam and do not overturn the failures. The follow-up's Playwright traces, request/response ledgers, first-attempt harness corrections, and audit are in (private test workspace) (app403-26b919ad-p0-diagnostic.tar.gz, SHA-256 855daf41397764d4f4df786af02d1097252b77a977223b94b776870ca85b3e86). Baseline nine-run report is ../evidence/REPORT.md. The test-only build/fixture changes were reversed; exact HEAD/tree and clean worktree checked after the run against PR head. The transfer limit-copy P1 was code-reviewed by Legolas/Gandalf, not independently exercised in this browser probe.

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Thanks for the follow-ups. No blockers at 1178112e.

The stale "Deletion started" notice from my last review is fixed. When Refresh replays the request and gets a tuple-bound deletion_aborted, the card drops the accepted entry, restores the row, clears the notice once no accepted deletions remain, and names the stopped host (HostedCommunities.tsx:171-180).

The two smaller items from that review are fixed too. A non-abort replay failure now shows "Couldn't check deletion status." and keeps the row hidden. A slot held by another owner now gets its own notice with Delete disabled (:133-146, :666-673, :738-742), instead of the "could not be saved" message.

The occupied-slot P2 raised at 26b919ad also no longer holds. The final-confirmation reread sets error, and the dialog renders it through failure (:342-350, :856-878). We reproduced it in headless Chromium and WebKit with a second same-origin tab writing the envelope while the dialog was open. At 26b919ad the dialog stayed silent. At this head the alert shows inside the dialog, nothing is POSTed, and the saved envelope is unchanged.

unauthorized is no longer treated as the connect state, which matches the backend: NostrIdentityService.current returns 200 with identity: null for an unlinked account, and unauthorized only as a 403 when the session lacks the USER role.

Tests: 143/143 in the hosted-communities Vitest suite at head and 18/18 for Settings in headless Chromium and WebKit. Four mutations (dropping the abort notice clear, the foreign-owner slot detection, the stable active closure plus action-owner busy clear, and the occupied-slot early return) each failed 1-3 tests. The three new browser scenarios fail at f5799087 and pass here. CI is green; Windows native validation was skipped.

Minor, non-blocking:

  • The shared error at HostedCommunities.tsx:342-349 overflows the dialog when it holds a full npub. At 1440×950 the text runs past the dialog edge in both engines (scrollWidth 628 vs clientWidth 446). Adding min-w-0 to the span and a break rule for long keys (break-all / overflow-wrap: anywhere) fixes it.
  • api.ts:67 "Your Builderlab session ended. Sign out, then sign in again." assumes an expired session. From the identity endpoint, unauthorized means the account isn't authorized for Buzz identities, and signing in again won't fix that. Something like "This Builderlab account can't manage Buzz identities right now. Try signing in again." would cover both cases.
  • When another context clears the blocked slot while the dialog is open, :858-860 says "Refresh and try again.", but pressing Start deletion again sends immediately. "Try again." is accurate.
  • blockedOwner also triggers when the owner matches but backend_origin differs (:134-140). The notice then asks the user to switch to the identity they're already using. This is unlikely in the desktop app; a separate origin message, or ignoring origin in that copy, would fix it.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes needed: the existing P2 public-material cleanup remains in historical PR comments/reviews. The description’s dependency wording is fixed; sanitize the remaining internal links and workspace/private-dependency references, replacing evidence links with public-safe attachments.

The occupied-slot finding is fixed in source and regression assertions. The stable callback/busy-state repair also matches Settings’ caller lifecycle; no new material code defects found in this follow-up. All 19 linked screenshots were inspected and show fixture data, not an observed secret.

Star Lord’s automated source follow-up via Wes’s account; head 1178112e95df025499663576c4639a230f6cba22, base 516de46100e06e7176e015fd315b9665f1bc5b9d. No tests or app execution. CI snapshot green; Windows skipped. Live deletion, packaged-native behavior and current keyboard/focus outcomes remain unverified.

@TheSentinel454

Copy link
Copy Markdown
Author

E2E evidence at 1178112e95df025499663576c4639a230f6cba22: production-built frontend in Chromium, running against the development broker with a modeled upstream and fixture data. This isn't a live-backend, native or real two-tab result.

  • The Settings parent re-renders during a held /delete: one POST goes out and returns 202, "Deletion started" shows, and Refresh is enabled. A held login also completes after a re-render.
    Accepted after a parent re-render
  • Occupied slot at final confirmation: the error shows inside the dialog, nothing is POSTed, and the stored bytes are unchanged.
    Another owner's slot: in-dialog alert
    Same owner's slot: in-dialog alert
  • A transfer limit_reached shows recipient copy, not the sender's quota.
    Recipient limit alert
  • Accepted → abort → Refresh restores the row with the host-specific stopped copy.
    Abort recovery
  • A list or identity 401 shows the session-ended copy.
    401 copy

@wesbillman

Copy link
Copy Markdown
Collaborator
Screenshot 2026-10-01 at 7 41 17 AM

What's this "Probe parent renderer" thing?

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Code blockers addressed; the existing P2 public-material cleanup remains. Reviewed head 1178112e95df025499663576c4639a230f6cba22 against base 516de46100e06e7176e015fd315b9665f1bc5b9d, including independent UI and public-material follow-ups. No new material code findings.

The account-transition, accepted-abort/notice, occupied-dialog and parent-rerender repairs match their regression assertions. Owner/origin binding, same-UUID replay and stale-result protection remain intact.

Remaining scope of the existing cleanup finding:

  • The author of this historical review needs to replace its private backend repository/PR/head locator with portable contract wording.
  • Remove private coordination deep links from this evidence paragraph and this closing paragraph. The former’s claim that images require Buzz access is now obsolete. Preserve the public GitHub attachments.

The description, concrete workspace locators and internal media-host URLs are cleaned up. The media audit inspected all 26 linked images and found fixture UI, not an observed secret. No source/history rewrite is requested for this remaining cleanup.

Required CI is green; Windows native validation is skipped. This follow-up used source, regression assertions, existing CI and published browser evidence; no local suites or live destructive workflow were run. Keep the backend-first/default-off rollout: live backend/relay/executor and packaged-native behavior are not established by the mocked evidence.

Regarding “Probe parent rerender”: the diagnostic describes a temporary test button that forces a Settings rerender during a held request. It appears in the instrumented screenshots, not the tracked application at this head; the committed equivalent is the test-only Parent update helper. It is not a shipped control.

@TheSentinel454

Copy link
Copy Markdown
Author

What's this "Probe parent renderer" thing?

@wesbillman That's a temporary diagnostic button the E2E harness injected so it could force a Settings parent re-render while a request was held. It only appears in the instrumented screenshots and isn't in the app at 1178112e. The committed test equivalent is the test-only Parent update helper.

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 approving at 1178112e. the code hasn't changed since my last review, it still merges cleanly with current main, the relay's deletion code hasn't changed in the meantime, and required CI is green.

the four nonblocking items from my last review are still open. fine to fix here or in a follow-up:

  • the dialog error runs past the dialog edge when it holds a full npub (HostedCommunities.tsx:342-349)
  • the unauthorized copy at api.ts:67 assumes the session expired, but it can also mean the account isn't authorized for Buzz identities
  • "Refresh and try again." at :858-860 should just be "Try again.", since Start deletion sends right away
  • blockedOwner also fires when the owner matches but backend_origin differs (:134-140), so the notice asks you to switch to the identity you're already using

could you also remove the internal coordination links from this comment and this one, per Carl's cleanup note? I've removed the private backend reference from my first review.

all testing so far, ours included, ran against a mocked backend, so a real deletion still needs a check once the backend capability is turned on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants