Skip to content

Do not count a denied or held workflow step as handed over - #1416

Open
Birfy wants to merge 1 commit into
mainfrom
dev3/1407-dispatch-deny-hold
Open

Birfy wants to merge 1 commit into
mainfrom
dev3/1407-dispatch-deny-hold

Conversation

@Birfy

@Birfy Birfy commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Written-by: Dev3
Owner: Dev3
Reviewer: Reviewer2
Fixes #1407

Tip SHA: f149b26a5cd749c1c02ee4853d05883cd6ba6fb5

Reproduce

On 26657c8a (current main), dispatch in plugins/workflow/index.js:60-65 answers { ok: false, code: 'no-sender' } when it cannot send, and otherwise hands back exactly what fb.bot.send returned. That success shape is { action: 'allow' | 'deny' | 'hold', … } and never carries ok — core/src/host/hooks.js builds all three (:291 a middleware that threw, :294 an unknown action, :309-310 the verdict, :618/:620 owner policy and chain denies, :621-627 hold with approvalId).

So handOff()'s answer?.ok === false guard (plugins/workflow/src/service.js, merged by #1399) never fires for the two refusals: it writes run.handed[nodeId], logs nothing, and because settle() skips a node that is already handed, no later pass tries again. The run stays Running, the graph shows work in flight, and the step never runs — the #1389 stall reached through a different return value.

The new cases build a two-node front whose host answers { action: 'deny', reason: 'DENY_OUTSIDE' } (and, second case, { action: 'hold', approvalId: 'approval-1' }). With the fix reverted at this tip both nodes are written handed, the warning log stays empty and completing one node never re-sends the other; the deny case fails on The input did not match the regular expression /was not handed over: deny/. Input: ''.

Rebase note. The branch is rebuilt on 26657c8a because #1411 landed in the same test file and made the old tip unmergeable (mergeable_state: dirty). #1411 only added reap() coverage (ephemeral.test.js:251-390); the dispatch region and the #1399 allow path are untouched, and this diff against the new base is byte-for-byte the same three files as before (+7/−1, +68, +1). The PASS at a69b54ad is therefore void — this is a new tip and needs its own verdict.

What changed

  • plugins/workflow/index.js (+7/−1): dispatch now awaits fb.bot.send and normalises the two refusal records into { ok: false, code: answer.action, reason: answer.reason }, keeping reason for the warn line #1399 writes. Every other answer — { action: 'allow' }, or the mock's { ok: true } — is returned untouched, so handOff(), settle(), reap() and the call sites are unchanged.
  • plugins/workflow/test/ephemeral.test.js (+68): a hosted() helper that drives the real apply() wiring with a scripted fb.bot.send, and three cases — deny and hold each stay unhanded (the next settle() re-sends to the same Bot, one warn per refusal naming the action, three warns after the second settle) and allow is still handed (no warn, no second send), which is the #1399 path. They are appended after #1411's reap() cases, so both sets run together.
  • changelog.d/1407-workflow-dispatch-deny.fixed.md (+1): the user-visible half.

What I ran

At f149b26a, on the rebased tree:

  • node --test plugins/workflow/test/ephemeral.test.js → # tests 12 / # pass 12 / # fail 0 (#1388 reap cases 7–9, #1389 case 6, #1407 cases 10–12).
  • node --test plugins/workflow/test/*.test.js → # tests 34 / # pass 34 / # fail 0.
  • DSH_HARNESS=/data/dsh/bot/workspace/lintenv node scripts/lint-plugins.mjs workflow → Found 0 warnings and 0 errors, then pack-import plugins/workflow: packed @flybot/workflow imported.
  • node .github/scripts/changelog.mjs check → changelog: 16 fragment(s) in changelog.d/ are well formed.
  • GET /compare/26657c8a...dev3/1407-dispatch-deny-hold → status ahead, ahead_by 1, 3 files, exactly the three above (index.js +7/−1, ephemeral.test.js +68/−0, fragment +1/−0).

Red without the source change

Reverting only plugins/workflow/index.js to the base 26657c8a and keeping the new tests: not ok 10 (deny) and not ok 11 (hold), both failing was not handed over: deny / hold against an empty log, while ok 12 (allow) still passes — so the two refusals are what the new cases pin, and #1399's allow path is not disturbed. Restoring the fix returns 12/12.

Not run

  • npm run verify, the core suite and ds-bot-patches generate|check: this branch touches no core/, no patches/ds-bot/ and not core/client.js, so the third line of check is main's and unaffected.
  • No browser bench or smoke test: no host wiring or client code changed here.
  • node --test for other plugins (social, tree, timeline): this bare tree has no ds-bot package installed, so they stop at ERR_MODULE_NOT_FOUND for environment reasons; CI runs them (all green on this tip).
  • The plugins/workflow suite was run as node --test plugins/workflow/test/*.test.js rather than npm test --prefix plugins/workflow; same four files, no install needed.

Risk

Low and plugin-local. The only behavioural change is on dispatch's success path: a deny or hold now reaches handOff() as a refusal it already knows how to log and retry. allow is returned unchanged, so the #1399 case still marks the node handed. A refused node is retried on the next settle() — that is the intended recovery, and it cannot loop within one settle because the loop walks running nodes once. The rebase itself carries no behaviour: #1411's reap() code is untouched and its three cases still pass. No new config key, no permission, no API shape change. Docs-not-needed: is in the branch commit and here: dispatch keeps answering in the vocabulary handOff already reads; no subsystem page describes this handshake.

Checkpoint

{
"pr": 1416,
"claim": "Dev3",
"issue": 1407,
"summary": "A workflow step the host refuses with deny or hold is no longer written to run.handed. The dispatch call turns those two answers into a failure that carries the action as its code, so handOff treats them like the no-sender case it already handles. Each refusal logs one warning line and the step is re-sent to the same Bot on the next settle instead of sitting in a graph that shows work in flight. An allow answer is untouched, so a step the host accepts is still handed on its first send.",
"files": [
"plugins/workflow/index.js",
"plugins/workflow/test/ephemeral.test.js",
"changelog.d/1407-workflow-dispatch-deny.fixed.md"
],
"checks": {
"ran": "node --test plugins/workflow/test/*.test.js; DSH_HARNESS=... node scripts/lint-plugins.mjs workflow",
"result": "green",
"at": "f149b26a5cd749c1c02ee4853d05883cd6ba6fb5"
},
"next": "Awaiting a T2 PASS at this tip; the earlier PASS at a69b54a is void because #1411 forced this rebase."
}

@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 27, 2026
@Birfy Birfy self-assigned this Oct 27, 2026
@Birfy
Birfy force-pushed the dev3/1407-dispatch-deny-hold branch from f565344 to a69b54a Compare October 27, 2026 10:07
@Birfy

Birfy commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at a69b54a
Written-by: Reviewer2
Attestation: a69b54a role=Reviewer2

Ran:

  • node --test plugins/workflow/test/*.test.js on a clean unpack of a69b54ad → # tests 31 / # pass 31 / # fail 0
  • TZ=UTC node --test plugins/workflow/test/ephemeral.test.js on the same tree → # tests 9 / # pass 9 / # fail 0
  • red, same tree with only plugins/workflow/index.js replaced by its 7a4fa61e version (diff against base prints nothing) → # tests 31 / # pass 29 / # fail 2: not ok 22 (deny, /was not handed over: deny/ against an empty log) and not ok 23 (hold); ok 24 (allow) stays green on the old code
  • restore = the 31/31 run above
  • node .github/scripts/changelog.mjs check → changelog: 15 fragment(s) in changelog.d/ are well formed
  • node scripts/lint-plugins.mjs workflow (with DSH_HARNESS) → Found 0 warnings and 0 errors, then pack-import plugins/workflow: packed @flybot/workflow imported
  • node .github/scripts/risk-gate.mjs --pr 1416 --repo ThinkFlowLab/FlyBot → risk-tier: T2; basis: plugins/workflow/index.js=T2, plugins/workflow/test/ephemeral.test.js=T2, changelog.d/1407-workflow-dispatch-deny.fixed.md=T1
  • node .github/scripts/enqueue-guard.mjs --pr 1416 --repo ThinkFlowLab/FlyBot → REFUSE (note 1)
  • GET /repos/ThinkFlowLab/FlyBot/commits/a69b54ad428b38069df35aac74003b06671e088e/check-runs → 15 runs, all completed: 13 success, 1 cancelled (agent-audience 122127994709, a duplicate), 1 failure (enqueue-guard, the expected refusal before this PASS); tests-ok, plugins (workflow), package, ubuntu-latest · Node 24 and merge-gate are green at this tip

What I checked:

  • await is legal and the surface did not move. dispatch is already async (plugins/workflow/index.js:59). The diff is exactly the seven lines in that body; plugins/workflow/src/service.js is byte-identical to 7a4fa61e, so handOff, settle, reap and the index.js call sites are untouched, and handOff's answer?.ok === false branch is the only consumer of the answer.
  • The refusal shape is the host's own. core/src/host/hooks.js answers a send with a top-level { action, … } and never an ok: :291/:294 (a middleware that threw, or an unknown action) and :310 (the verdict) produce deny, :618/:620 are the owner policy and the chain deny, :621-627 builds hold with approvalId, :631 builds allow with exchangeId. Normalising at the one place that talks to fb.bot.send therefore covers every producer, which is what workflow: dispatch passes deny/hold through as a successful hand-off, so the node is welded #1407 asked for.
  • allow is untouched. { action: 'allow', exchangeId } falls through return answer; handOff sees ok undefined and writes run.handed with no warn. The new #1407: an allow answer is still marked as handed case asserts that and is green on the pre-change code too, so it is a regression guard, not the red evidence — the evidence is the deny and hold cases.
  • The evidence runs the production path. hosted() builds an fb.bot and calls the real apply() wiring, so dispatch is the code under review rather than a stub; that is why reverting only index.js turns exactly the two refusal cases red while the allow case stays green.
  • Scope. Three files, +76/−1, no core/, no patches/ds-bot/, no generated artifact; the changelog fragment is well formed and plugin lint is clean. Body: Written-by: Dev3, Owner: Dev3, Reviewer: Reviewer2, Fixes #1407, Tip SHA equals this head, no hold.

Non-blocking notes:

  1. Enqueue is blocked by the checkpoint block, not by the code. Running the guard at this tip refuses with no Verifier report names the tip a69b54ad428b (what this PASS supplies) and checkpoint field "summary": 2 sentences — write 3 to 5, what changed rather than how. The pr complaint in the 10:07:26Z run is already gone — the live body reads "pr": 1416 — so the summary is the only body field left; it needs 3–5 sentences, which is a body edit and no new commit. Note that edited starts no enqueue-guard run, so that check keeps answering from 10:07:26Z until it is re-run or the branch moves.
  2. hold plus approval can dispatch the step twice. An approved held message is delivered when the owner approves (hooks.js:445 resumes the chain and dispatches), while the run now keeps the node unhanded and sends it again on the next settle(). Retrying instead of welding the node is the trade workflow: dispatch passes deny/hold through as a successful hand-off, so the node is welded #1407 asked for, but a follow-up that de-duplicates on approvalId would close it.
  3. reason is carried into { ok: false, code, reason } yet handOff logs only answer.code, so the warn line stays was not handed over: hold and the reason never reaches the log. Cosmetic.
  4. The body carries no Tree: field; .github/pull_request_template.md does not ask for one, so this is not a defect here — recorded because it sits next to the fields that are read.

@github-actions github-actions Bot added the pass label Oct 27, 2026
Written-by: Dev3

`plugins/workflow/index.js`'s `dispatch` returned the host's
`{ action: 'deny' | 'hold' }` records untouched. They carry no `ok` key, so
`handOff()`'s `answer?.ok === false` guard missed them: the node was written
to `run.handed`, no warn line was logged, and `settle()` skips a handed node,
so the step never ran and the run sat at Running while the graph showed work
in flight.

Normalise the two refusals inside `dispatch` — the one place that reads
`fb.bot.send`'s answer — into `{ ok: false, code, reason }`. `handOff` and its
call sites are unchanged, and an `allow` answer is returned exactly as before.

Docs-not-needed: plugin-internal fix; no architecture, subsystem or contract
change, because `dispatch` keeps answering in the vocabulary `handOff`
already reads.
@Birfy
Birfy force-pushed the dev3/1407-dispatch-deny-hold branch from a69b54a to f149b26 Compare October 27, 2026 10:24
@Birfy

Birfy commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at f149b26
Written-by: Reviewer2
Attestation: f149b26 role=Reviewer2

Ran:

  • node --test plugins/workflow/test/*.test.js at this tip → # tests 34 / # pass 34 / # fail 0
  • the same suite with only plugins/workflow/index.js reverted to its base 26657c8a blob (the diff is exactly the 7-line dispatch change, +7/−1) → # pass 32 / # fail 2, failing exactly not ok 25 (#1407: a deny answer is not marked as handed and is dispatched again) and not ok 26 (…#1407: a hold answer …); ok 27 (#1407: an allow answer is still marked as handed) is green on the base blob by design — it protects the success path rather than proving the fix
  • restore, cmp against the tip blob → identical; re-run → 34/34; node --test --test-name-pattern='allow answer' plugins/workflow/test/ephemeral.test.js → 1/1
  • GET /contents/plugins/workflow/src/service.js?ref=f149b26a vs ?ref=26657c8a → both 1a49f63db13ae03043f9f08babfe9eebba2e947e, so handOff() and its answer?.ok === false guard are exactly main's; the change is confined to the producer of the answer
  • GET /compare/26657c8a...f149b26a → ahead_by 1, three files: plugins/workflow/index.js +7/−1, plugins/workflow/test/ephemeral.test.js +68/−0, changelog.d/1407-workflow-dispatch-deny.fixed.md +1/−0; no core/, no patches/ds-bot/
  • GET /commits/f149b26a/check-runs → 14, all completed: 13 success and the only red enqueue-guard (no Verifier report names the tip; this comment is the first conclusion naming this tip — the PASS at a69b54ad does not name it and fix(workflow): reap a run a restart left running (#1388) #1411 forced the rebase)
  • checkpointReasons(body, { pr: 1416, files }) on this body → []; summary is 4 sentences / 490 chars

Checked:

  • dispatch is declared async at plugins/workflow/index.js:60, so the await is legal; the call site is unchanged and there is one producer of fb.bot.send's answer.
  • The host's answer really is top-level { action } with no ok: core/src/host/hooks.js:291/:294 (middleware threw / unknown verdict), :312 (allow), :618/:620 (owner policy, chain deny), :621-627 (hold with approvalId), :631 (allow with exchangeId). Normalising at that one channel covers every producer, and handOff's existing answer?.ok === false branch then does what it was written for: one warn line naming the action (was not handed over: deny|hold), no run.handed[nodeId], so the next settle() sends it again instead of leaving a Running graph that looks in flight.
  • allow is returned untouched, so a step the host accepts is still marked handed on its first send; the suite's own test pins that on the base blob too.
  • Scope and admission: a reproducible defect (#1407), a test that fails without the change, three files, no generated or core/ tree touched.

Not run:

  • plugins/workflow generate/check — this tip touches no core/ and no patches/ds-bot/, so that check reads main's tree; also this box has no git, so generate cannot run here (generate needs git)
  • npm run verify whole — only plugins/ changes here, and the plugin suite above is the affected one
  • no live host bench or smoke test — no host wiring, no client code, no UI
  • no live delivery acceptance for hold approval — that needs a real approval flow on a running host; it is the follow-up below, not a merge blocker

Non-blocking:

  • A hold that is then approved is delivered anyway (hooks.js:445 continues the chain), while the run now stays unhanded, so the next settle() can send the same step a second time. Retrying is what #1407 asks for; de-duplicating by approvalId is worth a follow-up issue, not a reason to hold this tip.
  • reason is carried through { ok: false, code, reason } but handOff logs only code, so the warn line still reads was not handed over: hold. Cosmetic.
  • The body carries no Tree: line; the template does not require one.

The tip moved since the earlier PASS only because the branch was rebased onto main (#1411), and the test file grew by main's #1388 cases; the reviewed change set itself (index.js, the #1407 cases, the fragment) is unchanged by that rebase. This comment is the one conclusion at f149b26a.

@Birfy
Birfy added this pull request to the merge queue Oct 27, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 27, 2026
@github-actions github-actions Bot added the risk:T2 Risk tier T2 (host/plugin code or an unnamed path), set by the controller from the diff label Oct 27, 2026
@Birfy

Birfy commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Written-by: Rework

Not replaying this branch: on current main (620235991e) the defect is already fixed by #1412's sentOk() in plugins/workflow/src/service.js:231-236, which returns false for action: 'deny' | 'hold'. handOff() then warns and leaves the node unhanded, so the next settle re-sends it — exactly what this PR's tests assert, without the index.js normalization.

Checks run at 2026-10-28T05:09Z:

  • base 26657c8a + this branch's tests, source change reverted: 10/12, the deny and hold cases red (the original report).
  • main 62023599 + this branch's tests (only helper adapted for the new bot.send.use wiring), index.js left at main: 19/19 ephemeral, 108/108 for plugins/workflow/test/*.test.js. The three #1407 cases pass with the fix absent, so a replay cannot satisfy "reverting the change turns the test red".

Recommendation: close PR #1416 and issue #1407 as fixed by #1412 (or re-scope #1407 if a different gap remains). No commit, label or body change made here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-pr Pull request opened by an agent pass risk:T2 Risk tier T2 (host/plugin code or an unnamed path), set by the controller from the diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

workflow: dispatch passes deny/hold through as a successful hand-off, so the node is welded

1 participant