[bradseiler]

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.
• Route coverage: #8239.
• Live-session revalidation: #8241.
• Staging runbook: #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

[github-actions]

🔐 Codex Security Review

Status: review required for the current range.

The current range is 5ea5ae1727fc6701a6b06595f0f920ec4f079b52...72c8d8bbc09bf7ed5525dabbea0e948c25380cf7.
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 72c8d8bbc09bf7ed5525dabbea0e948c25380cf7 to authorize a new review.
Any previous review applies only to its recorded range.

[wpfleger96]

🤖 Thanks for this. No blocking findings at d1c9dd34.

The new check sits right before create_workflow_run and uses the same host-bound community_id the run is inserted into, so changing the host picks another tenant's workflow rather than skipping the banned tenant's check. moderation_restriction_state reads from the writer, honors ban expiry, and covers an agent owner's human owner. A ban and a failed lookup both return the same 404 / {"error":"workflow not found"} as the earlier lifecycle and role denials, with no run row. The other HTTP path that creates runs, a signed workflow-trigger event, is owner-only, and ingest already rejects a banned signer, so it doesn't bypass the new check.

What we checked:

• The new PostgreSQL test calls the production handler. Deleting the ban check fails it at line 281 (202 instead of 404). Making a failed lookup admit fails it at line 338. Because bans leave channel membership in place, the existing role check can't mask either case. The lookup-failure fixture breaks only community_bans, so the error branch is really exercised.
• A real HTTP run against a locally built relay: ban gives 404 with no new run through both X-Webhook-Secret and ?secret=. Unban admits again. The same owner in another community is still admitted. An expired ban and a timeout-only owner are admitted. An unknown host and an injected restriction-read failure give 404.
• CI runs the new test by name in the PostgreSQL job (938/938) and the mesh receive regression in Unit Tests. The mesh, scraper, and push-lease fixture files are byte-identical to what #8239 carries.
• The Desktop Smoke failure (project-conversation-load-failure.spec.ts) passed on rerun. The desktop tree is unchanged from the base, and all checks are green now.

Minimalism 9, elegance 9, correctness 9: one existing authoritative query at the insertion boundary, no new abstraction, and both failure branches are proven by mutation.

Optional

• Cron and event-triggered workflows of a banned owner still create runs. That's out of scope per the description, and Luke covered it inline. Moving the restriction read into WorkflowEngine::check_owner_authority as a follow-up would close all four doors and let this handler-local branch go away.
• The test only sends the secret in the header. A query-only banned-owner request, plus a conflicting header/query request showing the header wins, would pin the converged credential path against a later split.
• The description is stale. It still lists the three failed Desktop checks, says "Draft only" on a ready PR, and has no buzz-review-completed.

[wesbillman]

🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency) and Groot (product/error outcomes/public material) review lanes.

No actionable findings in the reviewed diff. The webhook check uses the host-bound community and an uncached writer-side effective restriction lookup immediately before run insertion. Active bans and lookup failures deny without creating a run; the existing secret and channel-role gates remain intact. The source tests bind the production handler and assert run counts across denial, other-tenant admission and unban. The shared mesh change retains the same receive future across housekeeping ticks and ends its borrow before shutdown handling; the regression is selected by both unit-test entry points.

The documented point-in-time ban race, existing runs, timeout behavior and cron/event-triggered workflows remain outside this change’s scope. The latter already has discussion at #8242 (comment); no duplicate finding is added here. The public description and discussion have no attached images and yielded no actionable disclosure finding.

Reviewed head: d1c9dd341c3f8468ca976a99393ad1a7a0abefc6.
Base/merge-base: 326e2301cb4b1edcb8a72d01ac4b19a83545365f.

Validation: source inspection only; no builds, tests, SQL, app/relay execution or live HTTP/mesh rehearsal. The GitHub check snapshot showed 68 successful and 32 skipped checks, superseding the description’s older failed-check snapshot. GitHub currently reports a merge conflict; this review does not validate conflict resolution, sibling integration, runtime performance or deployment behavior. Minimalism, elegance and source correctness: 9/10 each within this scope. This is a COMMENT review, not approval or merge authorization.

[wpfleger96]

🤖 No blocking findings at 81c367f4. Moving the ban check into WorkflowEngine::check_owner_authority closes the cron, event and signed-trigger gaps from the last review, and the webhook handler no longer needs its own branch.

All four production paths that create a run call the gate first: channel events (lib.rs:413), cron and interval schedules (lib.rs:626, before the schedule claim), webhooks (bridge.rs:2758) and signed triggers (command_executor.rs:886). The insert at workflow_sink.rs:883 that looks ungated is a helper inside #[cfg(test)] mod postgres_tests. Step execution and approval resume don't call the gate, so a ban stops new runs but doesn't kill one already in flight, which matches the description. The restriction read goes to the writer, ignores expired bans and covers an agent owner's human owner. The 44f46993 merge keeps both the test module declaration and main's authenticate_member_post.

What we checked:

• Removing the restrictions.banned rejection fails both regressions: the event test creates one run instead of zero, and the webhook test gets 202 instead of 404.
• Making a failed restriction lookup admit fails only the webhook regression (workflow_webhook_postgres_tests.rs:339). The event test still passes, so the webhook test is the only one covering fail-closed.
• Cron against a locally built relay: on the first tick, a banned owner's workflow and an agent workflow whose human owner was banned each had zero runs and zero schedule claims, while an unbanned control ran. After unbanning, both ran on the next tick and completed. A denial doesn't consume the scheduled claim, and one banned owner doesn't stop other workflows on the same tick. Bans were set with SQL here, not through the moderation commands.
• CI: the PostgreSQL job passed 1169/1169 and lists both regressions by name. Unit and Relay E2E passed. Desktop Tauri (mesh) failed because apt-get install of the Tauri Linux packages timed out three times on the runner. I reran it. It isn't related to this PR.

Minimalism 9, elegance 9, correctness 9: one authoritative read in the gate every path already shares, no new abstraction, and mutation testing and a live run both catch the ban branch.

Optional

• Each denied workflow logs one WARN per matching event, and a scheduled one logs one WARN per tick per pod for as long as it stays enabled, with no rate limit (lib.rs:416-419, :629-632). Once bans are routine that's steady noise. Rate-limiting expected ban denials per workflow while keeping lookup failures loud would fix it.
• Every matching workflow adds one uncached writer read, done in sequence, even when they share an owner. I'd keep the authoritative read over a cache. Combining reads per owner is only worth it if it shows up under load.
• No dedicated test for cron, signed triggers or agent-owned workflows, and nothing in the event test covers a failed lookup. The live cron run above passed, so this doesn't block. A cron or agent-owner case in the buzz-workflow Postgres lane would pin it.
• The title still says "webhook" even though every trigger is covered now. Something like "Deny workflow runs for banned owners" would match.
• The description has no buzz-review-completed yet.

[wpfleger96]

🤖 One blocker at 1941b36b: Cargo.lock wasn't updated for the new sqlx dev-dependency.

crates/buzz-workflow/Cargo.toml:30-31 adds sqlx under [dev-dependencies], but the buzz-workflow entry in Cargo.lock still has no "sqlx", line, and this commit doesn't change Cargo.lock. Cargo resolves dev-dependencies into the lockfile too, so every locked command fails at this head. In an untouched checkout, cargo metadata --locked --format-version 1 exits 101 with cannot update the lock file … because --locked was passed to prevent this, and it fails the same way with --offline. That breaks the locked builds in Dockerfile:79 and Dockerfile.sprig:19, and every CI job that passes --locked. An ordinary cargo command adds the single missing "sqlx", line, so the fix is to commit that lockfile change. Run cargo metadata --locked afterwards to confirm.

The PR also conflicts with main in Justfile and scripts/run-tests.sh, where main added its own unit-test lines next to the new buzz-workflow --lib step. Because of that, only the security and lint checks ran at this head; Unit Tests, PostgreSQL Tests and Relay E2E didn't. The main merge will probably touch Cargo.lock too, so check --locked after the merge as well.

The rest of the change looks right:

• cron_tick(now) is the old loop body moved out and dedented. Ignoring whitespace, the only changes are continue becoming return when the workflow list fails to load (same effect, since the old continue also skipped the prune at the end of the tick) and the new denial log call. The owner gate still runs before claim_scheduled_workflow_fire, so a ban doesn't use up the scheduled claim, and the loop still sleeps 60 s between ticks.
• OwnerBanned doesn't change any response. The webhook handler (bridge.rs:2758-2760) and the signed-trigger handler (command_executor.rs:886-891) map every gate error to their generic denial, and nothing else matches on Unauthorized. The display text is unchanged. Only code() is new.
• The WARN limiter (lib.rs:805-817) only treats the first insert per workflow as fresh, and the 600 s TTL is checked on lookup. Failed lookups and every other denial skip the cache, so they still warn every time. Past the 10,000-entry cap, eviction only causes extra WARNs.
• The failed-lookup test proves itself. If the shadowed community_bans view didn't error, the unbanned owner would get a run and the assertion would fail.
• With the lockfile repaired locally, all 171 buzz-workflow unit tests passed (5 ignored).

Also fix

• cron_tick_skips_banned_owners_without_consuming_the_claim calls cron_tick, which scans every enabled workflow in the shared test database. It will fire any other test's due schedule workflows, too. Nothing depends on that today, but it couples the test to whatever else is in the database. Scoping the tick to the test's community, or giving the test its own database, would remove that.

[TheSentinel454]

🤖 Thanks. All addressed at 72c8d8b:

• Cargo.lock: 90e660f adds the missing sqlx line. A --locked build/metadata run leaves the lockfile unchanged.
• Main conflict: merged in 563ecec. Main already carried the mesh-receive lines, so Justfile and scripts/run-tests.sh keep main's side plus the new buzz-workflow --lib step.
• Cron test coupling: the PostgreSQL lane gives each test process its own database (scripts/postgres-test-wrapper.sh), so other tests' schedules can't reach this one. 72c8d8b documents that on the test rather than scoping cron_tick.
• The WARN rate-limit test now builds its cache through the engine's constructor and checks expiry: WARN, suppressed, WARN again after the interval.

rereview

[wpfleger96]

🤖 No blocking findings at 72c8d8bb. The blocker from the last review is fixed: Cargo.lock now lists sqlx under buzz-workflow, and the locked builds pass.

Checked in this round:

• ban_denial_warn_cache keeps the production cache the same (10,000 entries, BAN_DENIAL_WARN_INTERVAL TTL), and the engine and the test now share it. The new re-WARN assertion would fail if the TTL were dropped.
• The merge with main keeps main's invitation-contract and mesh-receive steps in Justfile and scripts/run-tests.sh. The only additions on top are the new buzz-workflow --lib steps.
• The isolation comment on cron_tick_skips_banned_owners_without_consuming_the_claim is accurate for the PostgreSQL lane. postgres_tests:: matches the postgres-ci filter, and scripts/postgres-test-wrapper.sh clones a fresh database for each test. All four named Postgres tests and the 171 buzz-workflow unit tests passed in CI at this head.

One small, non-blocking fix:

• crates/buzz-workflow/src/lib.rs:2263-2266 and 2282-2288: each insert-then-suppress pair only passes if the test thread resumes within the 500 ms TTL. A scheduler pause that long on a loaded runner fails a correct implementation. A wider interval, say 2 s with a matching longer sleep, or a controllable clock would remove that dependency. This is a risk found by reading the code; it hasn't failed in CI, and the ten-repeat flake check passed.