Repository navigation
BUZZ-270: Prepare community access verification runbook - #8235
bradseiler wants to merge 3 commits into
Conversation
🔐 Codex Security Review
|
|
This runbook depends on unmerged siblings. Its local-rehearsal step runs |
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Two blocking issues, both in what the runbook tells an operator to run or expect. Following the local rehearsal as written skips the route behavior tests, and one of its commands fails on a fresh database. The timeout section also expects every write to fail, which isn't what the relay enforces. The rest checks out against source.
Blocking
1. The local rehearsal never runs the route behavior tests, and the cargo test step fails as written. community-access-verification.md L57–65
- The HTTP and root-WebSocket ban matrix that #8239 adds lives in
buzz-test-client, as#[ignore]d tests (community_ban_routes.rsL475–477, L609–611, L650–652 at #8239 headdda7ef88). It needs a running relay, and CI starts that relay withBUZZ_V1_ENABLED=trueand then runscargo test -p buzz-test-client --test community_ban_routes -- --ignored. Neither printed test command selects it.cargo test -p buzz-db -p buzz-relaynames other packages and skips ignored tests.scripts/postgres-test-run.shonly selects ignoredpostgres_tests::/postgres_targets (L79–82). The Python guard checks that inventory rows and test registrations line up. It doesn't exercise a single denial. So an operator can finish every step without testing one ingress gate. cargo test --no-fail-fast -p buzz-db -p buzz-relayisn't one of the repo's test lanes, and theJustfiletest-unitcomments explain why. Many non-ignoredbuzz-relaytests need a database with the schema loaded. For example,api::media'stest_stateseeds a community throughconfig.database_url(media.rsL1674–1686). Pointed at an empty disposable database as the runbook says, it failed in a live run: exit 101, 1,670 passed / 8 failed / 910 ignored, and all 8 failures were missingcommunities. Left unset, those tests fall back topostgres://buzz:buzz_dev@localhost:5432/buzz(test_support.rsL1), which is the shared dev database the runbook says never to use.
Fix: replace the cargo test line with just test-unit (with #8239 it runs the route guard and the infra-free relay lanes) or the guard alone. Keep scripts/postgres-test-run.sh. Then add a separate route-E2E step: start a task-owned relay on a non-default port with BUZZ_V1_ENABLED=true against the isolated PostgreSQL, Redis and media backing, set RELAY_URL / RELAY_HTTP_URL / DATABASE_URL explicitly, and run the two buzz-test-client commands from #8239's TESTING.md. Don't point to the shared-service startup script. Record source-test results and relay behavior results separately.
2. The timeout section expects writes that the relay allows to fail. community-access-verification.md L139–140
"Writes fail until expiry or untimeout" doesn't hold for the routes listed just above it:
- Reports and the unban/untimeout moderation commands are allowed by design during a timeout (
ingest.rsL2205–2235). - Git admission checks bans, not timeouts (
transport.rsL249–265).receive_packthrough the policy hook and publish path has no timeout gate (transport.rsL1214–1312,policy.rsL303–435). - Personal read-state updates stay allowed (
auth.rsL138–161).
An operator following the runbook would log these as failures, or come away believing a timeout contains more than it does. Fix: name the content and ephemeral writes that should fail, list the allowed exceptions as positive controls, and keep the read and existing root/audio session controls. Ephemeral restrictions on other pods are only bounded by a 30-second cache (state.rs L1709–1732), not enforced immediately, so the expected result should say that.
Checked and fine
- The four
buzz moderationcommands and their flags matchlib.rsL2053–2099 andmoderation.rsL24–85, andtimeoutcorrectly requires an expiry. BUZZ_POSTGRES_ADMIN_URL,PGHOST/PGPORT/PGUSER/PGPASSWORD,REDIS_URL,PG_BIN_DIR, the privilege text, and both doc anchors matchCONTRIBUTING.md,crates/buzz-db/TESTING.md, andpostgres-test-run.sh.- The 30-day v1 drain matches
MAX_INVITE_TTL_SECS, and measuring it from the last serving or rollback issuance rather than from the merge is right. The cutoff behavior matches #8237, and the webhook owner-ban behavior (point-in-time check, generic denial, deny on lookup failure, timeout allowed, existing runs untouched) matches #8242. The runbook correctly treats both as prerequisites, not current main behavior. - A timeout doesn't block root or audio admission or disconnect existing sessions, and reads stay available.
- Isolation, secret handling, staging fault authorization, the rule against disabling shared Redis, cleanup, and keeping NOT RUN separate from deployed evidence are all sound.
Nonblocking
- L111–117, L122: spell out the two boundaries separately. V1 expiry includes the expiry second (
invite_token.rsL181 rejects onlyexpiry < now), while #8237's cutoff rejectsnow >= cutoff. Deterministic boundary cases need both. - The rehearsal candidate is described as including only #8239, but the invitation, live-session and webhook cases also need #8237, #8241 and #8242 where they apply.
- BUZZ-271 and BUZZ-272 are referenced in the body without links. Readers who can't resolve those IDs need the PR links.
- Nothing links to the new
docs/testing/directory yet. - The description doesn't have
buzz-review-completedyet (AGENTS.md step 4).
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot coverage.
Reviewed head eb3b03afc3238de7a46916ca8d0d40a9fd6353da against base 326e2301cb4b1edcb8a72d01ac4b19a83545365f (merge-base 19aa619fdd70e916269b343232276d9ee68276df).
Two P2 documentation corrections remain, both already described in the existing review. We independently confirmed them and are linking that feedback instead of duplicating inline comments:
- Local rehearsal omits the route behavior matrix and uses an unprepared database lane (
docs/testing/community-access-verification.md:57–64). The printed Cargo command selects only buzz-db/buzz-relay, while #8239’s HTTP/root-WebSocket matrix is an ignored buzz-test-client target; the PostgreSQL wrapper’s filter does not select it either. Non-ignored media tests also require a populated database before that wrapper runs. Use the established unit/PostgreSQL lanes and add an explicit route-E2E step against task-owned, isolated services with the accessory API enabled. Record these results separately. - Timeout expectations are broader than the enforced policy (same file, lines 139–140; evidence template line 32). Reports and unban/untimeout commands are explicitly allowed by
allowed_while_timed_out; personal read-state writes are also allowed. Specify the content/ephemeral operations that must be denied and retain permitted operations as positive controls, including the documented cross-pod cache bound.
No additional actionable finding from this review. The runbook’s NOT RUN status, deployment prerequisites, isolated fixtures, authorized fault-injection boundary and cleanup requirements should remain.
Validation: read-only source/documentation inspection, including the public PR description (no attached images) and #8239’s pinned route-test source. No commands from the runbook, PR tests, applications, staging operations or fault injection were executed. CI snapshot: 11 successful checks, 32 skipped; it does not establish runtime, cross-pod, deployment or human-verification results. This is a non-blocking COMMENT review, not approval or deployment authorization.
## Summary A workflow whose saved owner is banned from the community can no longer create runs through any trigger: webhook, channel-event, cron, or signed command trigger. Every one of those doors already calls `WorkflowEngine::check_owner_authority`, which re-checks the owner's channel role. That gate now also reads the owner's authoritative restriction state. A ban denies the run, and a failed lookup fails closed. Unbanning restores admission. On the webhook door the denial is the same generic 404 as before, and no run is queued. On the event and cron paths, a ban denial is logged at WARN at most once per workflow every ten minutes, and at DEBUG in between. A failed lookup and every other denial still warn every time. The handler-local ban lookup in `api/bridge.rs` is removed because the shared gate covers it. This is a point-in-time check: a ban committed after the lookup can race with run creation. It does not cancel existing runs. Community timeouts do not block runs in this change. This PR also carries the shared mesh receive fix needed for independent validation (see Shared carry). ### Related BUZZ-272. - Invitations: [#8237](#8237). - Route coverage: [#8239](#8239). - Live-session revalidation: [#8241](#8241). - Staging runbook: [#8235](#8235). ### Testing Postgres lane: - `on_event_denies_run_while_owner_banned`: a banned owner's channel-event trigger creates no run, and after unban it creates one. - `cron_tick_skips_banned_owners_without_consuming_the_claim`: one tick skips a banned owner and an agent whose human owner is banned, while an unbanned workflow on the same tick still fires. After unban, a tick for the same scheduled instant fires both, so the denial did not consume their scheduled-fire claim. - `on_event_denies_run_when_restriction_lookup_fails`: the event path fails closed when the restriction read errors, and fires once the read succeeds. - `webhook_admission_enforces_owner_ban_tenant_scope_and_lookup_failure`: the HTTP door returns the same generic 404 for a wrong secret, a banned owner, a forbidden role and a failed restriction lookup, with no run created. It also covers another tenant's allowed control and unban. Unit lane: - `ban_denial_warns_once_per_interval_while_other_denials_always_warn`. Results: - At `81c367f48`, CI passed: Unit Tests 572/572, PostgreSQL Tests 1169/1169 and Relay E2E. A local-relay E2E also passed: - unbanned webhook: 202 and one run; - banned webhook: generic 404 and no run; - banned event trigger: no run; - after unban: one run. - At `72c8d8bbc` (merged with main), CI passed: Unit Tests (including all 171 buzz-workflow unit tests), PostgreSQL Tests 1227/1227 (all four ban tests listed by name), and both `Build (linux/amd64|arm64)` image jobs, which build with `--locked`. A Blox run at the same head also passed `cargo metadata --locked`, fmt and clippy, and the buzz-workflow unit tests passed there with no database reachable. - Not covered: the signed trigger has no dedicated test. It calls the same gate, which was confirmed by reading the code. ### Shared carry #8237, #8239, #8241 and #8242 each carry the same mesh-receive fix, boot and scraper regressions, and push-lease fixture correction. Whichever lands first owns them, and the others drop them on their next main merge. ### What got simpler The ban check now lives in the one gate every run-creation door already shares, so there is one fewer handler-local branch. Cron, event and signed triggers get the same protection without per-door code. The cron loop body is now `cron_tick(now)`, so a test can drive one tick without the 60-second sleep. buzz-workflow's infra-free unit tests ran in no CI lane. `just test-unit` and its cargo fallback in `scripts/run-tests.sh` now run them. buzz-review-completed 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: seiler <seiler@squareup.com> Signed-off-by: Brad Seiler <seiler@squareup.com> Signed-off-by: Luke Tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com> Co-authored-by: Codex <codex@openai.com> Co-authored-by: Luke Tornquist <tornquist@squareup.com> Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Add a repeatable staging runbook and versioned evidence template. Execution remains gated on deployed prerequisites and a disposable community owner. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Brad Seiler <seiler@squareup.com>
Signed-off-by: Local Builder <1c476cb1127ea270cb900bdc3891060140bdd1173f91498030ee9b4db01c2f20@buzz.block.builderlab.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Sarnow <1894dcbafaa4df1ac607a9bf879ac0b7d77fbccb0e0f4619ecc252c8adbad8a2@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
eb3b03a to
3909943
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Both blocking issues from the last review are mostly fixed. The timeout section now matches what the relay enforces. The local rehearsal now runs just test-unit, the PostgreSQL wrapper, and the route matrix against a private relay. One relay test the rehearsal needs is still missing, so it doesn't cover everything CI does.
Blocking
1. The local rehearsal still skips the relay-admin ban/timeout regression test. community-access-verification.md L71–88
CI's Relay E2E lane runs two community-ban targets against the running relay: community_ban_routes and regression_relay_admin_ban_gate (_ci-relay.yml L633–634). The runbook prints only the first. The second is the inventoried timeout_admin_denial case (community-ban-regressions.tsv L19): banned and timed-out administrators must be refused on relay-admin kinds 9030–9033, and an unrestricted administrator must still succeed. The test is #[ignore]d (L128–130), and neither just test-unit nor scripts/postgres-test-run.sh selects it. The large-roster script linked below the matrix checks discovery and reconciliation, not this. So an operator can finish the rehearsal without ever checking that a banned or timed-out admin is refused.
Fix: after the community_ban_routes command, add
REPRO_RELAY_HTTP=<private relay HTTP URL> REPRO_HOST=<private relay host:port> \
cargo test -p buzz-test-client --test regression_relay_admin_ban_gate -- --ignored --nocapturewith DATABASE_URL pointing at the same disposable database. This target doesn't read RELAY_URL or RELAY_HTTP_URL. Without the two REPRO_* variables it falls back to localhost:3999 (L21–28), so the runbook has to set them.
Checked and fine
- The timeout exceptions at L161–167 match
allowed_while_timed_out(ingest.rsL2336–2343): reports (1984), unban (9041) and untimeout (9043). They also match the ban-only Git gate (deny_banned_git_principal) and the 30-second cached check on the ephemeral path (enforce_cached_write_restriction). - The workflow-owner section matches
check_owner_authority(buzz-workflow/src/lib.rsL164–197) and all four places that call it: signed triggers (command_executor.rs), webhooks with a generic 404 (bridge.rs), channel events, and cron, which checks before it claims the fire slot. Bans are denied, timeouts aren't, and a failed lookup denies. The signed trigger separately goes through the ingest timeout gate, as the runbook says. just test-unitis the right replacement for the barecargo test. The private-relay setup (BUZZ_V1_ENABLED=true,BUZZ_KLIPY_API_KEYunset, explicitRELAY_URL/RELAY_HTTP_URL/DATABASE_URL, a warning offstart-relay-for-tests.sh) matches thecommunity_ban_routesheader andTESTING.md.- The prerequisites now ask for the whole integrated stack and #8529's repair order. Every merged fix the PR description names is an ancestor of this head. The
psqlrequirement matchesupgrade_postgres_tests.rs. All relative links and anchors resolve.
Nonblocking
- L66:
just test-unitalready runscheck-community-ban-route-inventory.pyand its fixture test (TESTING.mdL57), so the separate guard line runs it twice. Drop it. - L133–139: say outright that a v1 token is still accepted at
now == expiry(payload.e < nowrejects,invite_token.rsL202), and that the cutoff rejects atnow >= cutoff. Deterministic boundary tests need both edges. - L180–184: "If BUZZ-271 is deployed" and "30 seconds in the proposed implementation" conflict with prerequisite 2, which already requires #8241 (BUZZ-271) in every serving revision. Make the step unconditional, link #8241, and drop "proposed".
- L205–207: the cron case "after unban, the same due cron instant must remain available" holds only if the unban lands inside
cron_fire_instant's 60-second window (lib.rsL600). The ignored PostgreSQL test at L2302 already pins it. File it with the deterministic local checks, like the cutoff boundaries, rather than as a staging expectation. - L12–17 and L180: link #8529, #8241, #8239, #8237 and #8242 instead of leaving bare numbers and BUZZ IDs. Nothing in the repo links to
docs/testing/yet either. A pointer fromTESTING.md's community-ban section would make the runbook findable. - L90 and L166 aren't wrapped like the rest of the file.
Summary
Add a community-access verification runbook and an evidence template for an exact deployed revision. Cover invitation repair/revocation, selective timeouts, bans, existing root/audio sessions, tenant isolation, cross-pod limits and all workflow trigger sources.
The local rehearsal uses the integrated stack, isolated services, the PostgreSQL wrapper and a running private relay for the signed route matrix. Timeout expectations name the permitted operations and distinguish signed/manual workflow writes from webhook, event and cron owner controls.
Related issue
BUZZ-270. Top of #8529 → #8241 → #8239 → #8235. Source remediation #8005, #7706, #7828, #8128, #8130, #8237 and #8242 is merged. The repair and lower stack layers still need their own approval and rollout.
Testing
Independent source review found no remaining blockers. Relative document links and anchors resolve. Commands and policy statements were checked against the integrated source.
Staging: NOT RUN. The template remains NOT RUN. Deployment-owner authorization, disposable fixtures, exact deployed revisions and cleanup evidence are required before a passing verdict. This documentation does not authorize deployment or security-finding closure.
Generated with Codex