Skip to content

feat(workflow): persist and resume approval gates (WF-08) - #2377

Open
brocoppler wants to merge 1 commit into
block:mainfrom
brocoppler:feat/workflow-approval-gates
Open

feat(workflow): persist and resume approval gates (WF-08)#2377
brocoppler wants to merge 1 commit into
block:mainfrom
brocoppler:feat/workflow-approval-gates

Conversation

@brocoppler

Copy link
Copy Markdown

What this solves

Closes the WF-08 gap called out in VISION.md and ARCHITECTURE.md (known-gap #5): runs that hit request_approval were marked Failed with "approval gates not yet implemented". The resume half — grant/deny handlers, resume_workflow_after_approval, execute_from_step, workflow_approvals CRUD, migration, CLI, SDK — already existed but was unreachable because the suspend half never persisted anything. This PR wires that half. Design decisions are laid out in #2376 for sign-off.

How it works

  • Executor (buzz-workflow): StepResult::Suspended now carries a PendingApproval (token, step id, resolved approver spec, message, timeout), captured where the resolved action is in scope. The approver spec is resolved before suspension — "any", 64-char hex, or npub1…, each with optional leading @. Unresolvable specs (display names) fail the step with an actionable error instead of minting an approval check_approver_spec could never authorize. Invalid timeout: strings now error at the step as well.
  • Engine (finalize_run): the Failed stub is replaced with arm-then-announce: (1) create_approval row — raw token hashed by buzz-db; (2) run → WaitingApproval at the suspended step with the accumulated trace; (3) announce via the new ActionSink::emit_approval_requested. Ordering means a half-armed gate is never observable. Any persistence failure marks the run Failed; announce failures are logged at ERROR but don't fail the run (the gate is armed and grantable by token).
  • Relay sink: emits kind:46010 signed by the relay keypair — d tag = SHA-256 token hash (the grant handlers' get_approval_by_stored_hash key, derived from the now-pub buzz_db::workflow::hash_approval_token, keeping every hash derivation on one definition), p tag = resolved approver or the workflow owner for "any" (so the request lands in query_needs_action), h tag when channel-scoped, and content JSON carrying the raw token, message, workflow/run/step ids, and expiry. Persist + fan-out mirror the send_message path (insert_event_with_thread_metadata + dispatch_persistent_event).

With the row minted and the run in WaitingApproval, the existing grant path (handle_approval_grant → spawn resume_workflow_after_approval(step_index + 1)) and deny path (→ Cancelled) operate unchanged — no relay handler changes.

Testing

  • New unit tests for approver-spec resolution (any/hex/@-prefixed/npub round-trip via bech32, display-name and malformed inputs fail closed).
  • cargo test -p buzz-workflow: 153 passed, 0 failed. cargo clippy --all-targets --all-features clean across buzz-workflow/buzz-relay/buzz-db; fmt clean.
  • cargo test -p buzz-relay --lib: 719 passed; 1 failure in api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo which reproduces identically on clean main in my environment (passes standalone, fails under the parallel suite) — pre-existing flake, unrelated to this change; can file separately.
  • Not yet covered: an end-to-end integration test (trigger → suspend → grant → resume → complete). The two #[ignore]d conformance rows (approval_token_is_community_confined, workflow_trigger_is_community_confined) are now unblocked — I'd like to rewrite them as a follow-up PR unless you want them in this one.

Follow-ups (deliberately out of scope)

  • kind:46011/46012 lifecycle emission from the grant/deny handlers.
  • Display-name approver resolution via the member directory (needs the same ambiguity policy as resolve_mention_pubkeys).
  • Rewriting the two pending_lane conformance rows against the live gate.

🤖 Generated with Claude Code

Runs that hit a request_approval step were marked Failed with 'approval
gates not yet implemented' — the executor minted a token and returned
Suspended, but finalize_run discarded it, so the grant/deny handlers,
resume_workflow_after_approval, and the workflow_approvals CRUD were all
dead code. This wires the missing suspend half:

- executor: StepResult::Suspended now carries PendingApproval (token,
  step_id, resolved approver spec, message, timeout). The approver spec
  is resolved up front — "any", hex pubkey, or npub1…, with or without
  a leading @ — and unresolvable specs (display names) fail the step
  loudly instead of minting an approval that check_approver_spec could
  never authorize. Invalid timeout strings now error at the step too.
- finalize_run: replaces the Failed stub with arm-and-announce ordering:
  create the workflow_approvals row (raw token hashed by buzz-db), move
  the run to WaitingApproval at the suspended step, then announce via
  the new ActionSink::emit_approval_requested. Any persistence failure
  marks the run Failed so a run never waits on an ungrantable gate; the
  announcement is best-effort once the gate is armed, logged at ERROR
  on failure.
- relay sink: emits kind:46010 signed by the relay keypair — d tag =
  SHA-256 token hash (the grant handlers' lookup key, now derived from
  the shared buzz_db::workflow::hash_approval_token), p tag = resolved
  approver (or workflow owner for "any") so the request lands in the
  needs-action feed, h tag when the workflow is channel-scoped, raw
  token + message + run/step metadata in the JSON content.

With the row minted and the run in WaitingApproval, the existing grant
path (handle_approval_grant → resume_workflow_after_approval →
execute_from_step) and deny path (→ Cancelled) operate unchanged.

Not covered here, noted for follow-up: kind:46011/46012 lifecycle
emission on grant/deny, approval-token multi-tenant conformance rows
(currently #[ignore]d pending_lane), and display-name approver
resolution via the member directory.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Broc Oppler <brocoppler@gmail.com>
@nmcitra

nmcitra commented Aug 3, 2026

Copy link
Copy Markdown

Reviewing against #2376 since the sign-off questions live there. I came at this from the authorization-flow side rather than the Rust. I was tracing who can grant one of these gates, and my assistant walked the source with me to check the flow matched.

Decision 1, arm-then-announce: agree. Persist the row, move the run to WaitingApproval, then emit. Failing the run on a persistence error while tolerating an announce failure is the right asymmetry: once the row exists the gate is grantable through buzz workflows approve, so a dropped announcement costs discoverability, not correctness.

Decision 2, fail-closed approver resolution: agree, and resolving before suspension beats minting an approval check_approver_spec could never authorize. It does surface something separate, see #2878 below.

Decision 3, kind:46010 shape: agree on structure, changes requested. Deriving the d tag from the shared hash_approval_token so every hash comes from one definition is right. Two adjustments before this lands.

The announcement isn't read-confined

KIND_WORKFLOW_APPROVAL_REQUESTED (crates/buzz-core/src/kind.rs:565) is in none of the kind-gating sets: not AUTHOR_ONLY_KINDS (kind.rs:120), not RESULT_GATED_KINDS (kind.rs:129), not P_GATED_KINDS (kind.rs:146), and not the shared-tag set (kind.rs:202). reader_authorized_for_event (crates/buzz-core/src/filter.rs:23-34) restricts only the result-gated pair, so 46010 passes through. For this kind the p tag only drives feed routing (crates/buzz-db/src/feed.rs), not read access.

So the announcement, message and d-tag hash included, is readable by any member who can query the community's events. When the workflow has no channel_id there's no h tag narrowing it at all.

On its own that's disclosure. Combined with an "any" or empty approver_spec it becomes authorization: any authenticated member who can read the announcement has what they need to grant it, and check_approver_spec (crates/buzz-relay/src/handlers/command_executor.rs:1004-1027) lets them through.

The raw token isn't a bearer credential, and that changes what needs fixing. handle_approval_grant resolves the approval by the d-tag hash and authorizes on the signer's identity (command_executor.rs:1040, :1070), so under a hex approver_spec a reader holding the token still can't grant. The token in content is redundant exposure rather than an independent bypass, and it contradicts the token-hashing rationale in crates/buzz-db/src/workflow.rs (stored hashed so a database read doesn't expose the raw value). It isn't free to remove — buzz workflows approve --token (crates/buzz-cli/src/commands/workflows.rs:193-206) consumes the raw token today — but the grant handler already accepts an e tag referencing the announcement, so clients don't need the raw value in content. Happy to include the small CLI change in a follow-up.

Two requests:

  1. Drop token from the 46010 content payload (with the CLI adjustment above).
  2. Add KIND_WORKFLOW_APPROVAL_REQUESTED to P_GATED_KINDS (kind.rs:146), which confines reads to the p-tagged approver and suppresses the event from search in the same move. Per-kind read confinement has precedent in feat(relay): add author-only-unless-shared read gate for kind 30175 #2768's gate for kind 30175, though that one is shared-tag rather than p-tag.

Adjacent, happy to take both

#2878. check_approver_spec accepts "", "any", or 64-char hex and rejects everything else, including the "@release-manager" form the schema documents at crates/buzz-workflow/src/schema.rs:133-135. upsert_workflow (crates/buzz-db/src/workflow.rs:313-355) doesn't validate either, so a workflow written to the documented syntax saves clean and fails only when a human tries to approve.

#2830. resume_workflow_after_approval reloads the definition live (command_executor.rs:1304, parsed at :1312) and resumes at step_index + 1, while WorkflowRunRecord (crates/buzz-db/src/workflow.rs:192-223) carries no snapshot and upsert_workflow mutates in place. A definition edited while a run waits changes what the grant authorizes. The workflows table already stores definition_hash, so binding that into the approval row at arm time and comparing at resume is a small fail-closed fix. It'd hash the JSON string rather than a canonical form, so a semantically identical re-save would invalidate a pending approval. Safe direction to fail, but worth documenting rather than discovering.

Neither collides with this PR's scope and #2376 already defers the first. Happy to take both on top of this branch once it lands, same working split as here: I map the flow, my assistant writes the Rust.

@tlongwell-block, flagging given the recent authorization work on the ingest path.

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.

2 participants