fix(workflows): make forward definition lifecycle atomic and discoverable - #7621
wesbillman wants to merge 13 commits into
Conversation
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
🔐 Codex Security Review
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes required
Reviewed head 430862a9de2bbab1fcaefafbaaba3864839dec7e against base 4cd82f513214aad11c2b742ce7cc7c681e8e32a0. Two forward-contract defects remain.
P1: Fence report-enforcement deletion of workflow definitions too
Changed anchor: side_effects.rs:604-608.
The new kind-9005 rejection does not cover the HTTP moderation enforcement path. Starting from a newly and atomically saved, enabled workflow:
- Submit a valid kind-1984 report targeting its kind-30620 event.
handle_report_eventaccepts the tenant-local event without a target-kind restriction. - A properly authorized relay operator/moderator resolves it with
action: "delete"and a freshrequestIdthroughPOST /reports/{id}/resolve. The HTTP handler accepts event-target deletion and callsresolve_report_with_enforcement, which reachesrun_atomic_mutation. execute_delete_with_markercommitsevents.deleted_atand its action marker, but neither removes the runtime row nor records a workflow cutoff. It does not pass through the new 9005 validation.
The definition disappears from live reads, while list_enabled_channel_workflows continues selecting the active runtime, even after caches expire. The author's subsequent canonical deletion now fails at head.is_some() != runtime.is_some(). Before this PR, canonical deletion could still remove that runtime; the new fail-closed path makes this ongoing split-state producer a recovery regression as well as a hole in the advertised invariant.
Smallest fix: reject workflow-definition targets at the shared durable report-delete mutation boundary, covering fresh HTTP actions and recovery-worker retries, consistent with this PR's author-only deletion policy. Add a regression through the real report-resolution seam proving that neither projection changes and canonical owner deletion remains usable. Do not add historical reconciliation or redesign the executor to fix this.
P2: Gate lifecycle advertisement across the supported rolling upgrade
Changed anchor: nip11.rs:330-334.
This advertises buzz-workflows / lifecycle: 1 automatically on each upgraded pod once stable identity and host binding succeed. The shipped HA chart uses RollingUpdate, maxSurge: 1, maxUnavailable: 0 and a version-independent Service selector; helm upgrade is documented as the upgrade procedure.
With at least two base-version replicas, after the first new replica becomes ready, discovery on it can certify lifecycle 1 while a subsequent connection to the same host still reaches an old ready replica. That old replica accepts the same canonical deletion but removes only the runtime row and deliberately retains the signed definition. A new replica subsequently rejects deletion of that fresh split state. Old saves also bypass the new persisted cutoff. The migration's additive table does not evict old serving replicas: the serving-catalog check checks for missing required fences, not unknown additional fences.
Fix boundary: provide a safe activation boundary for the host-wide capability, such as an opt-in advertisement flag kept off until incompatible endpoints and connections are drained, or stage advertisement in a later compatible release. A documented, enforced non-mixed transition is another option. No general deployment redesign is requested. The claim about schema readiness is not a separate finding: the new workflow_deletions serving fence already prevents ordinary startup without that table/fence.
Validation and scope
Source/diff review only. Carl completed the end-to-end authorization/deletion review and verified the returned independent database and discovery/integration lanes; the additional authorization lane was explicitly cancelled before publication. No local builds, tests, service mutations, or mixed-version deployment were executed by the completed review lanes. Existing actual-ingest rollback/concurrency tests were inspected, not represented as rerun. The canonical transaction/lock/cutoff implementation and migration/schema/purge inventory reconcile in source. Historical split-state reconciliation, exact-old-event repair, and the acknowledged executor-cache window remain outside the requested fix boundary.
Publication limitation: GitHub rejected REQUEST_CHANGES with HTTP 422 (Can not request changes on your own pull request) because this account also authored the PR. Published as a comment review instead; the technical verdict remains changes required, not approval.
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
…ider Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
…sion Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 4cd82f513214aad11c2b742ce7cc7c681e8e32a0..430862a9de2bbab1fcaefafbaaba3864839dec7e (exact head 430862a9de2bbab1fcaefafbaaba3864839dec7e)
Risk: high — this changes signed workflow persistence, deletion cutoffs, trigger eligibility, PostgreSQL migration state, and relay discovery/ingest boundaries.
Behavior/contracts traced: signed definition → transactional runtime projection; canonical deletion → tombstone/runtime removal/cutoff; coordinate locking and timestamp ordering; channel/owner authority; alternate deletion entrances; cached event-trigger lookup → run creation; NIP-11 host/identity binding; migration and whole-community deletion catalogs.
Blocking findings
-
A stale pod can execute a superseded or disabled workflow after lifecycle success. The event-trigger cache explicitly permits another pod to retain an enabled
WorkflowRecordfor ten seconds (crates/buzz-workflow/src/lib.rs:90-120,341-354). Saving an update orenabled:falseinvalidates only the accepting pod after commit (crates/buzz-relay/src/handlers/command_executor.rs:829-838). On a stale pod, the per-fire guard checks current channel authority but not the current workflow revision or enabled state (crates/buzz-workflow/src/lib.rs:370-420), whilecreate_workflow_runinserts by workflow ID without an atomic current-row/revision predicate (crates/buzz-db/src/store/workflow.rs:875-899). The stale pod can therefore create and execute a run using old actions after the save returned success. Delete alone is FK-fenced, but delete followed by an allowed newer recreation of the same UUID can bind the stale definition to the recreated row during the same window.Author action: atomically fence run creation against the current active/enabled workflow row and the exact definition revision/hash selected for execution, or provide an equivalent cross-pod invalidation plus settlement fence. Add a deterministic two-engine/two-cache PostgreSQL regression covering update, disable, and delete→newer-recreate with the same UUID. Shortening the TTL is not a correctness fence.
-
Migration 0045 makes the existing 0044 compatibility test fail the required PostgreSQL gate. This PR adds
workflow_deletionsto the current exact deletion catalog (crates/buzz-db/src/store/deletion.rs:84,91) but creates it only in migration 0045 (migrations/0045_workflow_deletion_cutoff.sql:3-13).migration_0044_drops_populated_nip_fi_ledger_cleanlydeliberately stops at 0044 and then invokes the current catalog validator (crates/buzz-db/src/runtime/migration.rs:2789-2793), which now deterministically fails withmissing=workflow_deletions. Exact-head hosted evidence: PostgreSQL Tests.Author action: validate the historical 0044 contract against its schema version, or advance through 0045 before invoking the current catalog validator while retaining the 0044 ledger-removal assertions. Keep a separate assertion that 0045 adds and fences
workflow_deletions, then rerun the full PostgreSQL gate.
Verification owner: author for fixes and deterministic regressions; hosted PostgreSQL Tests for the migration gate; reviewer for exact-head re-review.
Validation: at clean exact HEAD 430862a9de2bbab1fcaefafbaaba3864839dec7e, git diff --check FETCH_HEAD..HEAD passed. cargo test -p buzz-db --test observability_source -- --nocapture failed on event.rs:270; comparison with base 4cd82f5 showed both that access and the guard already existed there, so it is not attributed to this PR. Hosted Rust unit tests also failed in unchanged buzz-agent::fake_llm::cancelled_turn_with_usage_emits_notification_before_response; that is a CI confidence gap, not author rework for this change. Hosted Desktop, relay integration/E2E, lint, cross-compile, security, Linux/Windows/macOS builds passed. PostgreSQL ran 395 tests: 394 passed and the PR-caused migration test above failed.
Manual/native evidence: no direct renderer diff; no native journey claimed. The companion-app acceptance cited in the PR body used pre-integration app/relay revisions and does not prove this exact head or cross-pod settlement.
Residual risk: PostgreSQL fail-fast prevented the new lifecycle suite from supplying a complete clean gate at this head. The generic Rust-unit flake and pre-existing observability failure need separate CI/maintainer triage. Security review execution was skipped after authorization and remains owned by that gate.
— :bot: Jude’s code review agent
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 4cd82f513214aad11c2b742ce7cc7c681e8e32a0..430862a9de2bbab1fcaefafbaaba3864839dec7e (exact head 430862a9de2bbab1fcaefafbaaba3864839dec7e)
Risk: critical — this changes destructive workflow persistence, lifecycle authority, runtime eligibility, concurrency, and host-level capability discovery.
1. P1 — An accepted disable/update can still execute the stale definition on another pod
The PR now projects enabled from every signed definition save and advertises lifecycle 1, but after committing it invalidates only the receiving pod’s cache (command_executor.rs:829-838). Another pod may retain the old enabled record for ten seconds (buzz-workflow/src/lib.rs:90-120), parse and select that stale definition (lib.rs:341-385), and then create a run after checking only current owner authority (lib.rs:387-434). create_workflow_run inserts by workflow ID without atomically requiring the current row to remain active, enabled, and bound to the selected definition hash.
The cache existed at the base SHA, but this PR makes definition-driven disable/update part of the newly advertised lifecycle contract. Returning success while another serving pod can start old actions means executable state is not consistent with that accepted lifecycle transition. Canonical delete alone is FK-fenced; delete followed by permitted newer recreation of the same UUID can also let a stale old definition insert against the recreated row.
Author action: make run creation atomically validate that the current workflow row is active/enabled and matches the exact definition revision/hash selected for execution, or provide an equivalent cross-pod invalidation plus settlement fence. Add a deterministic two-engine/two-cache PostgreSQL regression for disable, update, and delete→newer-recreate with the same UUID. Shortening the TTL is not a fence.
Verification owner: author for the regression and full PostgreSQL gate; reviewer verifies the new exact head and CI.
2. P1 — Migration 0044 compatibility test now validates against the 0045 catalog and fails required PostgreSQL CI
The current deletion catalog now requires workflow_deletions (deletion.rs:84,91), but that table is created only by migration 0045 (0045_workflow_deletion_cutoff.sql:3-13). The historical test migrates only through 0044 and then invokes the current catalog validator (migration.rs:2789-2793). Required PostgreSQL job 103750334865 consequently fails with:
DeletionSafety("community deletion catalog drift (missing=workflow_deletions, unknown=)")
The failure repeats in summary job 103750630669; fail-fast cancelled most of the 395-test gate before the new lifecycle integration tests ran.
Author action: validate the 0044-appropriate catalog in the historical test, or move current-catalog validation until 0045 has run. Keep a separate assertion that 0045 adds and fences workflow_deletions.
Verification owner: author runs the full PostgreSQL package gate; CI/reviewer verifies it at the replacement exact head.
Reconciled contract review
Apart from the stale execution boundary above, source review found no additional defect in signed-event/runtime transaction atomicity, coordinate locking, timestamp/CAS ordering, canonical owner/channel/tenant authority, alternate NIP-09 deletion entrances, replay/recreation, rollback recovery, or stable relay-identity/host binding. Manual, webhook, schedule, and approval-resume entry points were traced separately; the concrete stale-cache path is the event-trigger door.
Validation and gaps
- Exact checkout was clean and
git rev-parse HEADreturned430862a9de2bbab1fcaefafbaaba3864839dec7e. - Inspected the full 14-file base/head diff and relevant consumers at the matching HEAD.
- Inspected exact-head CI with
gh pr viewandgh run view 34766074263 --job 103750334865 --log; the PostgreSQL failure above is reproduced in hosted logs. - The separate Rust unit failure in job
103747263185(buzz-agent::fake_llm::cancelled_turn_with_usage_emits_notification_before_response) is outside the changed files and is a CI/reviewer follow-up, not author action for this PR. - A local observability-source failure concerns code present unchanged at the base SHA; it is not classified against this PR.
- Codex security authorization passed, while execution/post were skipped. The current PR comment still says security review is required for this exact range; security tooling owns that external gate.
- No local service mutation, mixed-version deployment, or native/manual companion-app run was performed at this exact integrated head. Existing pre-integration acceptance does not substitute for the blocked exact-head PostgreSQL gate.
Exercise two real engine caches against signed lifecycle ingest for update, disable and changed/identical recreation. Assert both admitted runs and emitted action text, with accepting-pod positive controls. These regressions intentionally fail until the admission fence is integrated. Name PostgreSQL child modules explicitly for the existing structural discovery guard, including the report-enforcement tests. Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 4cd82f513214aad11c2b742ce7cc7c681e8e32a0..f97bac3a8e7ccca207a7bb0d5d5d801342a6c3d1 (exact head f97bac3a8e7ccca207a7bb0d5d5d801342a6c3d1)
Risk: critical — destructive workflow lifecycle, executable-state settlement, PostgreSQL migration state, moderation enforcement, and host-wide discovery are involved.
Behavior/contracts traced: signed definition save/update/disable/delete → runtime projection; event-trigger cache → run admission → action execution; delete→recreate; report-resolution direct/recovery paths; historical migration validation; NIP-11 host/identity binding and post-drain activation; required PostgreSQL test discovery.
Blocking findings
1. P1 — Accepted lifecycle changes still permit stale cross-replica execution
The changed-head delta does not modify the execution-settlement seam. Each pod still caches enabled workflow records for ten seconds without cross-pod invalidation (crates/buzz-workflow/src/lib.rs:90-120), selects and parses that cached definition (:341-385), and checks only current owner authority before run creation (:387-402). It then calls create_workflow_run using the workflow ID and trigger data (:404-420) and executes the cached definition (:428-435). The database insert has no atomic predicate requiring the current workflow row to remain active/enabled and match the cached record's definition_hash (crates/buzz-db/src/store/workflow.rs:870-899).
Consequently, after pod A returns success for an update or enabled:false, pod B can still admit and execute its cached old action. Delete followed by permitted newer recreation of the same UUID can likewise attach the pre-delete cached definition to the recreated row. The PR description independently discloses reproductions for update, disable, delete→newer recreation, and identical-definition recreation at this exact head. No deterministic two-engine settlement regression was found in the searched workflow, relay lifecycle, and database test trees.
Author action: atomically settle run admission against the current active/enabled workflow row and the exact selected definition revision/hash used for execution, or provide an equivalent safe settlement fence. Add deterministic two-engine/two-cache PostgreSQL regressions for update, disable, delete→newer recreation, and identical-definition recreation. Shortening the TTL is not a correctness fence.
Verification owner: author for implementation/regressions; hosted PostgreSQL gate and exact-head reviewer re-review.
2. P1 — New PostgreSQL regressions violate the required discovery gate
The PR adds three #[ignore = "requires PostgreSQL"] tests under crates/buzz-relay/src/handlers/workflow_lifecycle/postgres_tests/report_delete.rs:197,203,209, but the repository's discovery guard does not recognize that child-module shape. scripts/test-postgres-test-discovery.sh fails on the clean exact head with all three paths, matching hosted Detect Changed Paths job 103759274819. That PR-caused required-gate failure prevents PostgreSQL and most main CI jobs from running; this is not generic reviewer infrastructure debt.
Author action: move, rename, or wire the report-delete tests into a shape recognized by the standard PostgreSQL discovery guard, then rerun Detect Changed Paths and the full PostgreSQL gate.
Verification owner: author plus hosted Detect Changed Paths/PostgreSQL gates.
Reconciled fixes and confidence gaps
The changed head does repair three earlier boundaries in source:
- Report-enforcement deletion now rejects workflow definitions at the shared durable mutation boundary, including recovery retries and tombstoned targets (
crates/buzz-db/src/store/relay_admin_actions.rs:728-747). - The historical migration test retains its migration-0044 ledger-removal assertions, then completes the upgrade before validating the current catalog (
crates/buzz-db/src/runtime/migration.rs:2786-2799). - Workflow lifecycle discovery is default-off and requires explicit opt-in, stable relay identity, and bound host (
crates/buzz-relay/src/config.rs:678-683;crates/buzz-relay/src/nip11.rs:330-339). Operator documentation correctly separates compatible-code rollout, endpoint/connection/in-flight drain, and later activation (deploy/charts/buzz/README.md:299-346).
Confidence gaps, not additional author defects: exact-head PostgreSQL/main and security-review execution/post were skipped after the discovery failure; no exact-head companion/browser/broker acceptance exists; actual fleet drain/activation remains deployment-operator verification. The older live acceptance cited in the PR does not attest this integrated head.
Validation at matching clean HEAD:
git diff --check 4cd82f513214aad11c2b742ce7cc7c681e8e32a0..HEAD— passed.cargo test -p buzz-relay config::tests::workflow_lifecycle_advertisement_requires_explicit_opt_in -- --exact --nocapture— passed, 1/1.cargo test -p buzz-agent --test fake_llm cancelled_turn_with_usage_emits_notification_before_response -- --exact --nocapture— passed, 1/1.scripts/test-postgres-test-discovery.sh— failed on the three new report-delete tests, matching hosted job103759274819.- Source comparison confirmed no delta from reviewed head
430862a9...incrates/buzz-workflow/src/lib.rs,crates/buzz-db/src/store/workflow.rs,crates/buzz-relay/src/handlers/command_executor.rs, orcrates/buzz-relay/src/api/bridge.rs. - Final
git rev-parse HEADmatched the refreshed live PR head and the working tree was clean.
Manual/native evidence: none claimed; there is no direct renderer diff. The companion-facing lifecycle contract was reviewed from relay/database source and disclosed exact-head behavior.
Residual risk: report-delete, migration, and discovery fixes have focused source/unit evidence but no valid exact-head hosted PostgreSQL run because the PR-caused discovery gate stopped it. Fleet-drain correctness is operational and cannot be established by a single NIP-11 response.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 4cd82f513214aad11c2b742ce7cc7c681e8e32a0..f97bac3a8e7ccca207a7bb0d5d5d801342a6c3d1 (exact head f97bac3a8e7ccca207a7bb0d5d5d801342a6c3d1)
Risk: critical — destructive workflow lifecycle, execution admission, cross-replica consistency, migration safety, moderation deletion, and host capability activation.
The changed-head delta repairs the earlier migration-catalog mismatch and adds sound source fixes for report-delete fencing and default-off post-drain discovery activation. It does not touch the execution-settlement seam, and it introduces a separate required-gate failure.
1. P1 — Accepted lifecycle changes still permit stale cross-replica execution
The execution-settlement files are byte-identical to the previously reviewed head: git diff --exit-code 430862a9..f97bac3a -- crates/buzz-workflow/src/lib.rs crates/buzz-db/src/store/workflow.rs crates/buzz-relay/src/handlers/command_executor.rs crates/buzz-relay/src/api/bridge.rs exits 0.
Exact-head behavior remains:
- a serving pod deliberately retains enabled workflow records for ten seconds without cross-pod invalidation (
buzz-workflow/src/lib.rs:90-120); - the event-trigger door selects and parses that cached definition (
lib.rs:341-385); - its pre-run gate checks current owner authority, not current workflow lifecycle state or selected revision (
lib.rs:387-402); create_workflow_runinserts using the workflow ID without atomically requiring currentactive/enabledstate or the exact selected definition hash; execution then spawns with the stale parsed definition (lib.rs:404-435).
Therefore Pod A can acknowledge update or enabled:false while Pod B still admits and executes the superseded action. Delete followed by permitted newer recreation at the same UUID again supplies an FK target for the pre-delete cached definition. The PR description itself discloses reproductions for update, disable, delete→newer-recreate, and identical-definition recreation on this head. No deterministic two-engine settlement regression exists in the searched workflow, relay lifecycle, and DB test trees.
Author action: settle run admission transactionally against the canonical workflow coordinate: reject inactive, disabled, or deleted state and bind the run immutably to the exact selected definition revision/hash (or snapshot/reference). Add deterministic two-engine PostgreSQL regressions for update, disable, delete→newer-recreate, and identical-definition recreation, proving either rejection or execution of only the durably pinned definition. Do not shorten the TTL and call it a fence.
Verification owner: author for implementation/regressions; reviewer for changed-head mutation review; hosted PostgreSQL gate for execution.
2. P1 — Newly added report-delete tests violate the required PostgreSQL discovery gate
This is PR-caused, not generic CI machinery noise. Running scripts/test-postgres-test-discovery.sh on the clean exact head reproduces hosted job 103759274819:
postgres_tests/report_delete.rs:197 ... is not discoverable
postgres_tests/report_delete.rs:203 ... is not discoverable
postgres_tests/report_delete.rs:209 ... is not discoverable
The PR adds that file and those tests. Because Detect Changed Paths fails, PostgreSQL and most main CI jobs are skipped, so the new safety regressions do not enter the repository’s standard gate.
Author action: rename/move/wire the report-delete module so all three PostgreSQL tests are discovered by the standard script, then rerun the discovery and full PostgreSQL gates.
Verification owner: author plus hosted Detect Changed Paths and PostgreSQL gates.
Fixed contracts at this head
- Migration compatibility: the schema-44 test retains its historical ledger-absence assertions, then completes migrations before invoking the current catalog validator (
migration.rs:2775-2799). The previous 0044/0045 mismatch is fixed in source. - Report deletion: the shared durable mutation boundary now rejects workflow-definition targets, including retry/recovery paths. The regression logic is relevant, but finding 2 prevents standard discovery.
- Lifecycle discovery: advertisement is default-off and requires explicit opt-in, stable relay identity, and host binding (
nip11.rs:330-339). The route matrix covers off/on, stable/ephemeral identity, mapped/unmapped host, and both discovery routes. Operator documentation requires deploy-off, drain incompatible endpoints/connections/in-flight writes, then separately activate.
Validation and confidence gaps
- Clean exact checkout:
git rev-parse HEADmatchedf97bac3a8e7ccca207a7bb0d5d5d801342a6c3d1; live PR head matched immediately before submission. - Independently reproduced
scripts/test-postgres-test-discovery.shfailure locally and confirmed the same hosted log. - Both independent review lanes traced the stale-pod admission path and the repaired migration/report/activation boundaries; their results reconcile.
- The author reports 357 selected PostgreSQL tests passing, but the full hosted PostgreSQL/main suite did not execute because finding 2 stopped path detection. This is missing exact-head gate evidence in addition to the concrete discovery defect.
- Exact-head security execution/post was skipped. Security tooling owns that external gate.
- No exact-head companion/browser/broker acceptance or actual fleet-drain activation was performed. There is no renderer diff; operational post-drain verification belongs to deployment/release ownership rather than additional author rework.
- Broader local failures reported in unchanged observability/media/mesh paths are not attributed to this PR.
Run the production scheduler loop against a real PostgreSQL writer and require a current workflow to complete after stale admission is rejected. The regression fails before the fence and under a refresh-and-retry caller mutation. Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE at exact head c73db439d0f7bf4ace4a6ce78bf3cd7d9c098c78 (base 4cd82f513214aad11c2b742ce7cc7c681e8e32a0). This supersedes my prior reviews at older heads.
The two prior blockers are resolved:
- Atomic selected-revision admission:
crates/buzz-db/src/store/workflow/run_admission.rs:24-56now admits with one lockingINSERT … SELECT, requiring active/enabled state plus community, UUID, definition hash, owner, channel, and creation incarnation.FOR SHAREconflicts with definition writers/deletion, and the PostgreSQL tests observe the real lock dependency plus commit rejection and rollback liveness (run_admission.rs:178-249). - No production bypass: scoped searches for
create_workflow_run(andINSERT INTO workflow_runsfound one admission implementation and four production doors. Event, scheduler, manual, and webhook all pass the exact selectedWorkflowRecord, requireSome(run_id), and execute that selected definition only after admission (buzz-workflow/src/lib.rs:365-437,495-739;command_executor.rs:855-1011;api/bridge.rs:2066-2231). - Behavioral stale-work proof: the two-cache signed-ingest regressions assert both zero new runs and zero stale sink actions, with current-definition positive controls, across update, disable, changed recreation, and identical-definition recreation (
stale_execution_postgres_tests.rs:85-229). The real scheduler loop and production manual/webhook handlers cover committed-writer rejection, rollback liveness, and current-action execution (:232-310;run_admission_postgres_tests.rs:29-259). - Discovery repair:
scripts/test-postgres-test-discovery.shpasses locally at this clean exact head across the full Rust source scan; hosted Detect Changed Paths is green. The renamed PostgreSQL modules restore standard discovery without weakening the guard. - Lifecycle/discovery contract: advertisement remains default-off and requires explicit activation, stable relay identity, and resolved host binding (
config.rs:212-217,681-683;nip11.rs:297-339). Operator documentation requires the separate deploy-off, drain, and activation sequence (deploy/charts/buzz/README.md:300-346).
I found no unresolved author-actionable defect. Authenticated reviewer jedwards27 is distinct from author wesbillman; the checkout was clean and local/live head equality was rechecked immediately before submission.
Residual risk / verification ownership
workflow_runsdoes not pin the admitted definition revision, and the dormant approval-resume helper reloads the current definition. That would be unsafe if suspension/resume became live. It is not wire-live today: approval tokens are terminally converted toFailed / approval_not_supported, with no reachableWaitingApprovalpath (buzz-workflow/src/lib.rs:224-251; executor TODO atexecutor.rs:736-742). This is therefore a required WF-08 design/test gate, not a blocker for this explicitly no-executor-redesign PR: persist immutable revision provenance and test update/disable/delete/recreate while waiting before activation.- Exact-head PostgreSQL behavior was not independently rerun by the review environment; the author reports 47 selected PostgreSQL passes, while hosted PostgreSQL is skipped in the relay-artifact producer matrix. This remains a confidence gap owned by the applicable CI/release verification, not author rework absent a failed required gate.
- Some hosted unit/desktop/build jobs were still in progress at the last inspection. Their named required gates remain authoritative; no observed red required gate is being waived by this review.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed exact head c73db439d0f7bf4ace4a6ce78bf3cd7d9c098c78 against base 4cd82f513214aad11c2b742ce7cc7c681e8e32a0 through systems/integration and product/adversarial lifecycle lenses.
The two prior blockers are resolved:
- Run admission is now a single guarded
INSERT … SELECTthat matches current active/enabled state plus community, UUID, definition hash, owner, channel, andcreated_atincarnation while holding a PostgreSQLFOR SHARElock (crates/buzz-db/src/store/workflow/run_admission.rs:24-56). Event, scheduler, manual, and webhook paths all pass the selectedWorkflowRecord, requireSome(run_id), and execute that selected definition. Scoped searches found no production bare-ID caller or alternateworkflow_runsinsert. - Risk-shaped regressions cover warmed-cache update, disable, changed recreation, identical-definition recreation, the real scheduler loop, and production manual/webhook writer commit/rollback races. They assert both no stale run and no stale sink action, with current-definition/liveness controls (
workflow_lifecycle/postgres_tests/stale_execution_postgres_tests.rs:85-310;command_executor/run_admission_postgres_tests.rs:29-258). - Standard PostgreSQL discovery now passes locally at this exact head via
scripts/test-postgres-test-discovery.sh. The migration-0044 compatibility repair, shared report-deletion fence, and default-off post-drain discovery activation remain sound in source.
No unresolved author-actionable defect found.
Residual risk: workflow_runs does not persist the admitted definition revision/snapshot, and the dormant approval-resume helper reloads the current workflow. This is not wire-live today: approval tokens are terminally failed as approval_not_supported (crates/buzz-workflow/src/lib.rs:200-251). Before WF-08 enables suspension/resume, that work must pin an immutable admitted definition and test update/disable/delete/recreate while waiting. Verification owner: the WF-08 implementer/reviewer; no action required in this PR.
Verification at matching clean HEAD: PostgreSQL discovery guard passed; git diff --check and source/policy audits passed. Local PostgreSQL behavior execution was unavailable in the reviewer environment, so exact-head PostgreSQL/runtime completion remains owned by hosted CI. At submission, security, lint, Linux builds/cross-compiles, macOS builds, DCO, and discovery were green; Rust unit/Windows and desktop/relay jobs were still running. Those are confidence gates, not author defects absent a failure.
Opened by Brain on behalf of @wesbillman.
Summary
Keep signed workflow definitions and executable runtime state consistent. The existing workflow execution engine is retained; this repairs its management lifecycle for the companion
block/buzz-appplugin.enabledvalue (omitted defaults to true).buzz-workflows/ lifecycle 1 only with explicit post-drain activation, resolved host and stable relay identity.Readiness — not merge-ready
Published head:
c73db439d0f7bf4ace4a6ce78bf3cd7d9c098c78(integrated main4cd82f513).BUZZ_ADVERTISE_WORKFLOW_LIFECYCLE. Both discovery routes still require stable identity and resolved host binding._postgres_tests, fixing the structural discovery rejection without weakening the guard. Hosted status must be checked at the new published head; earlier green checks do not certify it.Related work
Companion app PR: pending publication.
Overlapping open proposals: #4234 and #6639 (atomic/timestamp-aware deletion), #4318 (definition visibility after deletion), #4742 (enabled-state persistence and executor guards). This PR adds atomic event + runtime + cutoff forward lifecycle and its discoverable host contract; it does not automatically close or supersede those PRs. In particular it does not include #4742's executor guards or backfill.
Admission repair evidence
c73db439d: 47/47 selected PostgreSQL regressions pass, including lifecycle/admission through every trigger door, workflow DB/sink behavior, discovery routes and migration controls. Full structural discovery, Rust formatting and three-package all-targets clippy pass.4cd82f513; not hidden or folded into this repair.Earlier testing (not evidence for the admission-repair head)
At clean
f97bac3a8e7ccca207a7bb0d5d5d801342a6c3d1(integrated main4cd82f513):900f43band relayab411c30bplus loopback-only listener adaptations: create, keyboard enable/disable, reaction execution, edit/conflict, run/trace display, deletion/fresh readback. Those pre-integration results do not prove this head or cross-pod settlement.just ciand deployed/native acceptance were not rerun. Required hosted checks and exact-head re-review remain merge gates.Compatibility and boundaries
Canonical management is channel-scoped and requires the actual signing author; legacy global/name/owner-delegated deletion is not certified by lifecycle 1. Historical split rows are not reconciled, and replaying an old event is not a repair. Trigger discovery caches may remain stale for roughly ten seconds and miss new triggers; atomic admission prevents cached superseded/disabled/recreated records from starting new work. Disabling does not cancel already admitted/running work. No executor redesign, packaged app host/sign-in, production rollout or deployment is included.
Originating Buzz channel:
cb38cd5a-c920-40a1-b770-e9b89c289476(workflows-plugin).Companion application
Draft app PR: block/buzz-app#48 (
a62eb71fe731951ac30aabac595c43e5feb1c0a8). It consumes the lifecycle-1 contract for guarded workflow editing/execution/history. Its checks and older live-trial snapshots are documented there; neither PR authorizes merge or deployment.Handoff snapshot after companion publication
At unchanged
c73db439d0f7bf4ace4a6ce78bf3cd7d9c098c78, GitHub now reportsAPPROVED(superseding the earlier changes-requested snapshot). DCO succeeds; 31 reported checks succeed, 23 are skipped, and five desktop/artifact checks remain in progress. This is not completed relay CI or merge authorization. Companion app #48 has all 11 reported checks succeeding ata62eb71, including CI required and DCO; it remains draft and needs human/code-owner review. No final-head live trial or deployment is claimed.