Skip to content

feat(desktop): add recoverable hosted community deletion - #7959

Open
TheSentinel454 wants to merge 87 commits into
mainfrom
elrond/community-delete-desktop-draft
Open

TheSentinel454 wants to merge 87 commits into
mainfrom
elrond/community-delete-desktop-draft

Conversation

@TheSentinel454

@TheSentinel454 TheSentinel454 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds recoverable hosted-community deletion with one durable request slot. A pending request survives Buzz identity switches; another identity sees a capability-gated npub notice and cannot send or overwrite it. If that identity cannot return, deletion on this device stays blocked and the user is directed to support.

After an accepted deletion, the row stays hidden against stale owner lists. An explicit Refresh replays the same request UUID only for the current owner with deletion enabled; only a tuple-matching 202 aborted or tuple-bound 409 deletion_aborted restores the archived row. An error-only 409 does not settle it. Zero-quota messaging no longer promises a slot from deletion.

Verification

Desktop unit tests, typecheck, lint/format, file-size policy, and the hosted-community Chromium mock-bridge spec cover owner recovery, the blocked slot, accepted-to-aborted Refresh, unsettled replay outcomes, and capability gating. The browser spec does not exercise a live backend.

Rollout

Deploy relay (merged), then the hosted backend, then clients. The deletion capability stays off by default until the backend path is ready.

TheSentinel454 and others added 30 commits September 25, 2026 15:19
Signed-off-by: tornquist <tornquist@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Owner admission records durable intent with no owner-facing cancellation,
so an operator needs a recovery path when preparation cannot continue.
Abort already reversed approved and fenced requests; extend it to the
submitted and inventoried stages, which have destroyed nothing.

Aborting releases the durable request fence over owner listing,
unarchive, and owner rotation. It reverses deletion intent only: the
community stays archived and the owner restores it explicitly. Stages
from drained onward stay closed.

Lock ordering and the active-serving-write-lease guard are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
The owner branch of community_deletion_owner_provenance tested only the
shape of each provenance column. A bare `col ~ '...'` on a NULL column
evaluates to NULL, and a CHECK constraint is satisfied by NULL, so
`FALSE OR NULL` admitted owner-origin rows with no owner key, no
mediating operator, or no acknowledgement version.

Reject a NULL in each column before testing its shape. Spell it
`NOT (col IS NULL)`: pgschema drops a named CHECK whose body contains
`IS NOT NULL` and still exits 0, which would leave the desired-state
bootstrap silently unguarded while the migration path stayed correct.

Assert one shared case table from both schema sources -- the migration
upgrade path and the pgschema desired-state bootstrap -- so the two
cannot drift into different owner-provenance guarantees.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
The malformed-body coverage sent a single request carrying several
invalid fields at once, including an unparseable `request_id`. Request
deserialization runs before the host and pubkey guards, so that request
was refused by serde and the guards it claimed to cover were never
executed — dropping the `owner_pubkey` validation entirely left the test
passing.

Give each guard its own request whose remaining fields are valid, so the
case reaches the guard under test, and assert no deletion intent
persists for any refusal.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com>
`/operator/communities/delete` admits an irreversible request, but the
endpoint's own tests only exercised the happy path and relied on generic
operator-auth coverage elsewhere. Nothing pinned that this route refuses
an unsigned caller, the X-Pubkey dev fallback, a non-operator signer, or
a signature bound to a different method, URL, or body.

Add direct coverage for each binding failure, and for idempotent replay:
an identical resubmission converges on the stored request, while a
resubmission that changes the owner, the host, or the acknowledgement
version is refused and leaves the stored intent untouched. Every
rejection asserts no deletion request was persisted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com>
The admission path reads as if the relay had verified the owner's consent.
It has not: the mediating operator authenticates the owner and collects
the acknowledgement out of band, and the request reaching the relay
carries only that operator's NIP-98 signature. The relay checks operator
authority and current ownership, and stores the owner pubkey, operator
pubkey, and acknowledgement version as provenance for the upstream
ceremony — never an owner-signed attestation.

Also record how manual handoff converges. Owner provenance pins
`requested_by` to `owner_pubkey`, so `buzz-admin deletions submit` must
repeat the owner pubkey to adopt an admitted request; the operator's own
pubkey conflicts with the one-active-request invariant instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
The runbook told operators to target `cronjob/<release>-buzz-deletion-drain`.
`buzz.fullname` collapses to the release name when it already contains the
chart name, so the documented name is wrong for the documented install:
`helm install buzz ...` renders `buzz-deletion-drain`. Discover the CronJob by
its component label instead, and describe the name rule rather than a single
guessed spelling.

Three more corrections:

`activeDeadlineSeconds` was presented as if a timed-out run were just another
retry. It is not recorded as one — shutdown releases the claim without
recording a retry, and only the object-store drain resumes mid-stage — so a
deadline landing repeatedly inside a non-resumable stage loops forever with a
rising `attempts`, a flat `retry_count`, and no block. Document how to spot
that from both the request and Kubernetes, how to size the deadline, and how
to recover.

`terminationGracePeriodSeconds` was presented as a clean handoff. Document
that a pod still working at the end of the window is SIGKILLed holding its
lease, and that recovery is lease expiry plus reclaim under a new generation.

An empty `serviceAccountName` was described as a neutral default. It inherits
the relay's service account; `automountServiceAccountToken: false` hides the
projected token but does not detach cloud IAM bindings resolved through the
node metadata path. Recommend a dedicated pre-created account when the
executor's IAM blast radius should be smaller than the relay's.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Exercise both supported credential shapes through the CI render matrix and bind the typed job guards, schema boundary, pod identity, and default sidecar annotation.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Share a suffix-preserving 52-character name helper between the deletion drain and storage accounting CronJobs while retaining their existing short names.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Distinguish the disabled Kubernetes API token mount from provider workload-identity credentials while retaining the dedicated service-account recommendation.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Bind each context-menu interaction to the exact mock message returned by the emitter so same-second ordering cannot select the wrong video.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com>
The runbook told operators to target `cronjob/<release>-buzz-deletion-drain`.
`buzz.fullname` collapses to the release name when it already contains the
chart name, so the documented name is wrong for the documented install:
`helm install buzz ...` renders `buzz-deletion-drain`. Discover the CronJob by
its component label instead, and describe the name rule rather than a single
guessed spelling.

Three more corrections:

`activeDeadlineSeconds` was presented as if a timed-out run were just another
retry. It is not recorded as one — shutdown releases the claim without
recording a retry, and only the object-store drain resumes mid-stage — so a
deadline landing repeatedly inside a non-resumable stage loops forever with a
rising `attempts`, a flat `retry_count`, and no block. Document how to spot
that from both the request and Kubernetes, how to size the deadline, and how
to recover.

`terminationGracePeriodSeconds` was presented as a clean handoff. Document
that a pod still working at the end of the window is SIGKILLed holding its
lease, and that recovery is lease expiry plus reclaim under a new generation.

An empty `serviceAccountName` was described as a neutral default. It inherits
the relay's service account; `automountServiceAccountToken: false` hides the
projected token but does not detach cloud IAM bindings resolved through the
node metadata path. Recommend a dedicated pre-created account when the
executor's IAM blast radius should be smaller than the relay's.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
@TheSentinel454

TheSentinel454 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

SUPERSEDED — NOT A GATE PASS (2026-09-29). P0-D1 reported by Legolas and confirmed by Gandalf (7f4682c1) shows this fixture did not match relay owner-list behavior after admitted delete/transfer (row absent) or unarchive (archived_at: null). The earlier screenshots and green checks below are accurate for the mock fixture only; they do not establish the real deletion recovery path, and should not be used to gate #7959. Re-test at Elrond’s forthcoming fix head with those three owner-list states is pending.


E2E verdict: EXERCISED AND WORKS — bounded frontend/mock-bridge scope. Tested exact PR head 2224c126723be6bca683241ca582b961676ec001 (tree 75bb1559b5b7bf3c46e1872e3dbe39b1a51d6d90) on a disposable Linux test host, 2026-09-29 UTC.

Built the Desktop E2E frontend (pnpm build:e2e), served locally on 127.0.0.1:4273, and drove Hosted communities in Chromium via Playwright with fresh mock Tauri state. Six scenarios covered all seven requested checks: reload preserves pending with no automatic resend; Check reuses the saved UUID/tuple once; matching 202 submitted settles; overridden 202 aborted settles without a false “Deletion started”; 409 must_archive settles and Archive becomes available after the mock list refresh; fresh-only 400 unsupported_acknowledgement_version retains pending; explicit can_create:false hides Create while an absent quota projection does not. 6/6 passed twice, then 6/6 individually logged reruns. Separate full pnpm test: 6,762 Node + 93 jsdom passed (not counted as E2E).

Visual evidence: pending after reload, 202 submitted, 202 aborted, 409 must_archive, fresh-only pending, explicit quota false / absent projection. Full JSON command ledgers, logs, trace/video, and the documented early harness failures retained under (private test workspace) (164-file MANIFEST.sha256: 2bd9aa49d26150325e32232e10b62cb5b6a53b6c9b7d66619fba75171fb08d7c).

Scope limitation: This did not exercise packaged Tauri/native credentials, authenticated backend, a live relay, or the executor. 202 aborted and omitted quota required test-only native-response overrides; attempted mock localhost:3000 traffic was blocked. This is not a relay/native integration sign-off or merge-readiness claim. Retargeting/combined-state CI and relay dependency are separate gates. No product source changed; test-only worktree was cleaned after capture.

@TheSentinel454

TheSentinel454 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

E2E verdict: EXERCISED AND WORKS — bounded Desktop frontend/mock-Tauri at head c3b4891ce24f65e8e64295b0c87187b56d66429b (tree 02a38b62c6b996b3fc684e1a8395f13334bf7834).

On a disposable Linux test host, built desktop with pnpm build:e2e and drove Hosted communities in Chromium via the repo's Playwright/mock-Tauri bridge, local preview 127.0.0.1:4274. Two independent three-case runs passed 6/6:

  1. Owner row omitted after simulated admission: reload restored the pending tuple without sending; Check deletion status resent the saved UUID/community/host/version exactly once; mock-native 202 submitted cleared pending and showed “Deletion started.”
  2. Row still present but archived_at:null after simulated unarchive: Check got mock-native 409 must_archive; pending cleared and Archive was offered again.
  3. Owner row omitted after simulated transfer: Check got mock-native 404 not_owner; pending cleared.

Three optional controls passed 3/3: fresh-only mock 400 unsupported_acknowledgement_version kept the pending tuple; explicit can_create:false hid Create; test-only omitted quota projection left Create available.

Evidence: before Check with missing row, submitted result, must_archive + Archive, not_owner. Full logs, request ledgers, screenshots, traces, repro and README remain at (private test workspace); the 65-file manifest digest is 27b31fac25be40293916a6e53bd57f02ccc18b9322158ac28557b5b8d2040d56 (all 65 files verified there). Worktree clean and preview stopped.

Limit: Browser frontend + sequenced mock-native replies, not packaged Tauri, real native credentials, authenticated backend, signed relay or executor. No live relay was contacted. The seven full local smoke failures reported by Elrond are not yet established as inherited; hosted CI and Gandalf's gate remain separate. This is not a merge-ready declaration.

@TheSentinel454
TheSentinel454 marked this pull request as ready for review September 30, 2026 01:44
@TheSentinel454
TheSentinel454 requested a review from a team as a code owner September 30, 2026 01:44

@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 c3b4891c against elrond/community-delete-relay-draft @ e21151f4. Blocking: one issue, details inline. Switching owners still deletes the unresolved deletion envelope, and it's the same defect that's open on buzz-app#403.

Sign-out and a lapsed or unauthorized identity now hide the envelope and keep it (lines 144-148 and 223-235), so that half of #403's finding is fixed here. The known-owner mismatch path still clears it, though. loadAccount (149-161) and the recovery effect (651-660) both call clearPendingCommunityDeletion as soon as the bound owner differs from bound_owner_pubkey. So A → B → A leaves A with no UUID to check, even though no server result settled the request. B doesn't need to be a different person: switchToDeviceIdentity gets there, and so does opening Settings while the Builderlab account is bound to a key other than this device's. The spec pins the discard as intended behavior. hosted-communities-settings-screenshots.spec.ts:354 is titled "a valid different bound owner discards the prior owner's envelope", and the A-B-A case at :392 ends with the stored request ID null. Carl's P2 on #403 names A → B → A as a case that has to stay recoverable, and this PR describes itself as matching #403.

That spec change also affects the fence test. The A-B-A case clears X before its response is released, so the persisted-match guard rejects X no matter what, and removing the unmount generation bump alone wouldn't turn it red. Once X survives the transition, the same case can actually exercise the fence. It would help to run it in one mounted instance and attach the red run with the fence removed.

The rest of the recovery logic checks out against the relay at e21151f4:

  • The ten accepted stages match DeletionStage. The UUID lookup runs before the owner, archive and lifecycle checks, so the definitive and fresh-only sets hold. They match #403 exactly.
  • Clearing is compare-and-delete on all six fields, and unmount bumps the generation, so a late response can't clear a different request.
  • The persist-and-read-back happens before the POST, and replay sends the identical four-field tuple.
  • http_status comes from reqwest, and a body claim can't override it (authenticated_json_response_from_parts).
  • The capability gate is literally true, and only an explicit can_create: false hides Create.
  • Nothing in desktop/ still calls the removed receipt command.

A couple of small things, none blocking:

  • The 64 KiB cap in authenticated_json_with_status now applies to every Builderlab call, not just delete. That's probably fine for today's payloads, but it's a quiet behavior change for list and identity.
  • The limit_reached message still interpolates the hard-coded HOSTED_COMMUNITY_LIMIT = 5 (hostedCommunityApi.ts:98), while the create flows now show the server's quota_limit.

CI at this head is green (30 passed, 23 skipped). I didn't run the account-switch flow live. The finding rests on the source and on the PR's own spec assertions.

Comment thread desktop/src/features/settings/ui/HostedCommunitiesSettingsCard.tsx Outdated
Base automatically changed from elrond/community-delete-relay-draft to main September 30, 2026 14:49
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 53a12100b34577286c89102f1875ca2b6eea61e5...c8bf27332e7c4925ec20532c0ffd00be48952739.
A new review must complete for this exact range. When manual authorization
is required, a user with write access must comment exactly
@buzz-security-review c8bf27332e7c4925ec20532c0ffd00be48952739 to authorize a new review.
Any previous review applies only to its recorded range.

Signed-off-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026

@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 75c76120 against main 53a12100. The owner-switch issue from my last review is fixed. Blocking: one new issue, details inline. After an accepted delete that an operator later aborts, the restored community never comes back on Refresh. It's the same defect Carl raised as a P2 on buzz-app#403, which App fixed in f5799087, and this PR says it works the same way as #403.

We reproduced it in the running UI against the mock bridge. After a matching 202 submitted, the list comes back with the community archived again (the operator-abort case), and Refresh leaves "Deletion started" on screen with the row hidden. The header reads "2 of 5 used" with only one row listed, so there's nothing on screen that explains the missing row, and Refresh, the one recovery control on the card, doesn't recover it. Reopening Settings does.

A → B → A is fixed:

  • The stored record now has exactly one writer, startCommunityDeletion. It re-reads storage before persisting (527-532), and persistPendingCommunityDeletion refuses to replace a valid record (communityDeletionPending.ts:113-114), so a confirmation dialog that was already open can't get around the occupied slot.
  • The only clears come after a server result (421, 431, 450). They're gated on owner, generation and the persisted record, and compare all six fields. loadAccount, the recovery effect, sign-out, unpair and switch now only change what's shown.
  • B sees only the blocking owner's npub. It can't Check, resend or overwrite A's request. Check as A resends the saved four-field request and doesn't need an owner-list row.
  • A pending Check that settles as aborted (a tuple-matching 202 aborted, or a tuple-bound 409 deletion_aborted) clears the record, replaces the status with "Deletion stopped" and leaves the archived row visible with Delete enabled. So #403's stale-notice problem doesn't show up on that path here.

Test runs at 75c76120: 24/24 Node tests (communityDeletionPending, hostedCommunityApi) and 32/32 hosted-communities Playwright cases against the mock bridge, one worker, no retries. We stopped the mutation runs once the blocker settled the verdict, so the red runs of the blocked-Delete and early-return mutations are still only the ones in the PR description.

A few small things, none blocking:

  • The description says a 409 deletion_aborted settles a Check. The code settles it only when all four tuple fields match (communityDeletionPending.ts:250-255). An error-only 409 stays uncertain, and that's the shape the mock bridge returns (e2eBridge.ts:12680-12701). The coupled KGoose PR returns a tuple-bound 202 for an aborted replay, so this isn't a production problem today. It's worth either narrowing the description to the tuple-bound 409, or agreeing on the error-only contract and testing it.
  • The card's at-limit copy (HostedCommunitiesSettingsCard.tsx:1046) still says "limit of 0 hosted communities" when quota_limit is 0. The new error copy (hostedCommunityApi.ts:103-106) shows "You can't create more communities right now." for that case.
  • Sign-out and the no-auth branches of Check clear pendingDeletion but leave blockingOwnerPubkey set. It's harmless today because the notice only renders while signed in and bound, but resetting both keeps the state honest.

CI at this head: Rust lint, the Windows desktop build, desktop E2E relay and integration shard 2/2 passed. Desktop core, smoke E2E, macOS, Windows Rust and integration shard 1/2 were still running when I posted.

Comment thread desktop/src/features/settings/ui/HostedCommunitiesSettingsCard.tsx
@TheSentinel454

TheSentinel454 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Gimli E2E verdict: EXERCISED AND WORKS — bounded Desktop Chromium UI/mock-Tauri bridge at head 75c761205a366c029423f645c28baa648b4297ad (tree 16f3b8cb). Not a native credentials → backend → live-relay/executor verdict.

On a disposable Linux test host: Hermit, pnpm build:e2e, built desktop dist, Playwright Chromium at 1440×1240, preview 127.0.0.1:4285, one fresh browser/bridge process per probe, zero retries. Six independent browser probes passed:

  1. A→B→A with B device key matching B bound key: B saw disabled Delete and A's npub; no B delete/no overwrite of A's exact saved pending bytes. Returning A restored pending, and Check resent its same UUID/community/host/ack tuple, received a mocked matching 202, and cleared pending. B blocked · A checked
  2. Sign-out/in retained same-owner pending and Check used the saved tuple. Restored
  3. Capability off disabled the owner's Check, hid B's blocking notice, and made Delete unavailable. Capability off
  4. Test-only native limit_reached overrides showed limit-7 copy and neutral limit-0 copy after clicking Create. This tests error copy, not at-limit banners or server enforcement.
  5. Confirmation-time race: B's dialog was enabled before A's pending was inserted; final click blocked B, preserved bytes, sent no delete, and showed A's npub. Blocked

Requested committed-source mutation rerun: Verified HostedCommunitiesSettingsCard.tsx in the pinned clean worktree SHA256 89816560f2230d145774dd8cfb844adbd8e34d51184a80cf501b57aada1585c5 (not the earlier draft b0fe2f53…). The two committed browser specs passed 2/2 at baseline. Removing || blockingOwnerPubkey !== null changed the fresh built JS hash ab337ec0… → 0819a10a… and failed the disabled-Delete assertion (expected disabled, received enabled). Failure screenshot. Restored and rebuilt: git diff --quiet true; JS hash back to ab337ec0…; 2/2 passed. Separately removed deletionView.blockingOwnerPubkey from startCommunityDeletion's early return: JS hash 9e78fba8…; confirmation-race spec failed at the absent owner-specific notice, instead showing generic save failure. Failure screenshot. Restored again: git diff --quiet true; JS hash ab337ec0…; 2/2 passed. This second mutant's assertion stopped before its later no-send/no-overwrite checks, so it proves the notice guard rather than independently proving a send occurred.

Audited reports, ledgers, builds, tests, and screenshot manifest are in (private test workspace): GIMLI_DESK7959_75C76120_REPORT.md and GIMLI_DESK7959_75C76120_MUTATIONS_REPORT.md. Evidence tarballs SHA256: UI c28212190f80fac97a699f098f0711673020970db5d3ef9014b0868c5fd3a34e; mutations 29ae20ad4944fb2720dbd0f6d5f4beb03f0693508d46b0fa02816385b77b4c3b. The Buzz Blossom media uploader rejected gzip/zip, so screenshots above are linked and tarballs remain in the team's workspace. Raw trace ZIPs containing synthetic fixture private-key fields were excluded from portable evidence. Final PR head/pull-ref and clean worktree matched the SHA. No live relay, native Tauri credential path, backend, backend quota enforcement, or privileged executor exercised; CI and source review remain separate gate inputs.

Buzz thread verdict: buzz://message?channel=a5175acf-3560-4ebc-91e6-558e24a57980&id=1b036620ef6d818ed9b2b0d30238bc79476c8c69e5ac9bafcfdb84ce23b44964&thread=8d0e5f87ce5e55d2aee8f5eb99b16df977bc7905ff9e279556bd4ef53db58b28.

Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026

@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 e5f56aa6 against main 53a12100. The Refresh blocker from my last review is fixed for a single accepted deletion, and the three small notes are all addressed. Blocking: one new issue in the Refresh loop when more than one deletion is accepted, details inline.

What's fixed:

  • Refresh replays the saved four-field request for each accepted deletion whose community shows up in the owner list again. Only a tuple-matching 202 aborted or a tuple-bound 409 deletion_aborted brings the row back, and "Deletion started" changes to "Deletion stopped". A replay that still reports an in-progress stage keeps the row hidden. A stale list that leaves the row out skips the replay and keeps the request for a later Refresh.
  • An owner change clears the accepted requests and bumps the generation, so nothing replays under the new owner. A mismatched device identity or deletion capability off also stops the replay.
  • The description now says an error-only 409 stays uncertain, and communityDeletionPending.test.mjs:283-290 pins that. Zero quota reads "You can't create more communities right now." in the settings card, the create flow, and onboarding. clearAccountView resets blockingOwnerPubkey on sign-out and on both no-auth branches of Check.

Test runs at e5f56aa6: 24/24 Node tests (communityDeletionPending, hostedCommunityApi) and 39/39 hosted-communities Playwright cases against the mock bridge, one worker, no retries. We stopped the mutation run once the blocker settled the verdict.

One small thing, not blocking, is inline on line 50.

CI at this head: no failures. Desktop Core was still running when I posted.

Comment on lines +73 to +77
context.accepted.delete(communityId);
restored = true;
}
}
if (restored && context.isCurrent()) context.restoreListed(listed);

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.

Blocking: line 73 takes an aborted request out of accepted right away, but the rows and the notice only update here, after the whole loop. Any exit before this line throws away a recovery that was already confirmed.

Say X and Y are both accepted, and Refresh lists both. X replays and comes back a matching aborted, so it's removed from the map. Then Y's get_builderlab_auth or delete_builderlab_community call rejects, or the capability comes back false at line 59. We return before restoreListed, so X stays hidden and "Deletion started" stays on screen, even though X is no longer being deleted. In the rejection case the user also gets an error that's about Y, not X. It takes another Refresh to show X, which is the same thing the last round was fixing.

Suggested fix: apply the restore in a generation-fenced finally whenever at least one abort was confirmed (or after each confirmed abort), and let Y's error still come through. A Playwright case with X aborted and Y rejecting, and one with Y hitting capability off, would pin it. The existing two-request case (X aborted, Y still in progress) doesn't cover either exit.

context.ownerPubkey() === envelope.bound_owner_pubkey &&
envelope.backend_origin === BUILDERLAB_BACKEND_ORIGIN &&
context.accepted.get(communityId) === envelope &&
!context.identityMismatch;

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.

Small one, not blocking: identityMismatch is the value from when Refresh was clicked, so re-checking it inside stillEligible() after every await reads like a live check when it's really a snapshot. checkDeletionStatus checks it once at entry. Doing the same here, and dropping the stillEligible() call at line 61 (no await since the one at line 53), would make the guards say what they actually check.

@TheSentinel454

Copy link
Copy Markdown
Contributor Author

E2E evidence at e5f56aa67b3bf40d84b04763a446995dcc0d2c86: Chromium against the repo's mock Tauri bridge, with fixture data only. This isn't a native, live-relay or live-backend result.

  • Accepted → tuple-bound abort → Refresh: only the aborted row comes back, with the stopped notice, and the header count matches the rows.
    Aborted row restored with the stopped notice
  • Two accepted, one aborted: only that row comes back, and "Deletion started" stays.
    Only the aborted row restored
  • An approved, 503 or error-only 409 replay keeps the row hidden. With the capability off, Refresh doesn't replay.
  • Zero quota on Settings, Create and Onboarding:
    Zero quota in Settings
    Zero quota in the create flow
    Zero quota in onboarding
  • Three mutations to the committed source each fail a targeted assertion.

Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026

@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 c8bf2733 against main 53a12100. No blockers. The Refresh issue from my last review is fixed, and so is the identityMismatch nit.

What's fixed:

  • The replay loop in acceptedDeletionRefresh.ts now runs inside try/finally, so a confirmed abort gets its row and notice back no matter how the loop exits afterward: a later auth or delete call rejecting, the capability coming back false, an owner or entry change, or an unexpected reply. The one exit that skips the restore is an unmount or an account generation bump, and a generation bump comes from adoptAccountOwner, which clears the accepted map anyway. A null auth goes through clearAccountView, so nothing gets written back into a signed-out view.
  • identityMismatch is read once when Refresh starts, and the account still loads under a mismatch. Only the replay is skipped, which also fixes e2d23ca8, where a mismatch skipped the load too.

Test runs at c8bf2733: 41/41 hosted-communities Playwright cases against the mock bridge (one worker, no retries) and 24/24 Node tests (communityDeletionPending, hostedCommunityApi). Mutations: restoring only after the loop finishes fails 2 cases, and dropping the restore on the capability-false exit fails 1. Removing the identity-mismatch replay guard still passes 41/41, details inline.

Two small things, both nonblocking, inline on lines 40 and 81.

CI at this head: everything passed except Run Codex Security Review, which was cancelled at its 45-minute limit.

return;
}
const listed = await context.loadAccount();
if (!context.isCurrent() || skipReplay) return;

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.

nonblocking: nothing tests this guard. with it and the skipReplay declaration removed, the full hosted-communities spec still passes 41/41. the A→B→A test loads the account from a mismatched view but never has an accepted deletion to replay. a case where an accepted deletion is refreshed while the identity mismatches, asserting no delete_builderlab_community call goes out, would pin it

context.accepted.delete(communityId);
restored = true;
} else if (disposition !== "accept") {
throw new Error("Couldn't check deletion status.");

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.

nonblocking: this replaces the server's error with a fixed "Couldn't check deletion status." for every retain or clear reply, so the error code and correlation ID are gone. applyDeletionResponse passes them through errorMessage(response.error, response.correlation_id, "Could not confirm deletion status. The existing request remains pending."), and support needs the correlation ID. could this use the same errorMessage call and wording?

This branch was successfully deployed

No deployments
codex-review — c8bf2733 Deployed Sep 30, 2026 by TheSentinel454 via Run Codex Security Review #6313
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.

3 participants