From f149b26a5cd749c1c02ee4853d05883cd6ba6fb5 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Tue, 27 Oct 2026 18:24:17 +0800 Subject: [PATCH] Do not count a denied or held workflow step as handed over (#1407) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../1407-workflow-dispatch-deny.fixed.md | 1 + plugins/workflow/index.js | 8 ++- plugins/workflow/test/ephemeral.test.js | 68 +++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 changelog.d/1407-workflow-dispatch-deny.fixed.md diff --git a/changelog.d/1407-workflow-dispatch-deny.fixed.md b/changelog.d/1407-workflow-dispatch-deny.fixed.md new file mode 100644 index 00000000..f6ff0e8d --- /dev/null +++ b/changelog.d/1407-workflow-dispatch-deny.fixed.md @@ -0,0 +1 @@ +A workflow step the host refuses is no longer counted as delivered. When `fb.bot.send` answers `deny` (an edge outside the tree, a social limit, a middleware that threw) or `hold` (the message waits in the owner's approval queue), the step now stays unmarked and the next settle sends it again to the same Bot, with one line in the log naming which of the two refused it; an `allow` is unchanged. ([#1407](https://github.com/ThinkFlowLab/FlyBot/issues/1407)) diff --git a/plugins/workflow/index.js b/plugins/workflow/index.js index 9050bf6d..8ab37a65 100644 --- a/plugins/workflow/index.js +++ b/plugins/workflow/index.js @@ -61,7 +61,13 @@ export function apply(ctx, config = {}) { const from = (fb.bot.list?.() ?? []).find(bot => bot.isMain)?.id if (!from || typeof fb.bot.send !== 'function') return { ok: false, code: 'no-sender' } const text = `Task ${step.taskId}: ${step.brief}\nDo this step and reply with the result.` - return fb.bot.send({ from, to: step.botId, text, role: 'request' }) + const answer = await fb.bot.send({ from, to: step.botId, text, role: 'request' }) + // The host refuses with { action: 'deny' | 'hold' } and no ok key, which + // handOff would read as delivered. Answer in its own vocabulary instead. + if (answer?.action === 'deny' || answer?.action === 'hold') { + return { ok: false, code: answer.action, reason: answer.reason } + } + return answer }, } : {} const service = createWorkflowService({ diff --git a/plugins/workflow/test/ephemeral.test.js b/plugins/workflow/test/ephemeral.test.js index 6d869bc8..fd99c291 100644 --- a/plugins/workflow/test/ephemeral.test.js +++ b/plugins/workflow/test/ephemeral.test.js @@ -388,3 +388,71 @@ test('the plugin load itself reaps a run the restart left running (#1388)', asyn rmSync(dir, { recursive: true, force: true }) } }) + +// #1407: the host answers a refused hand-off with { action: 'deny' | 'hold' }, +// never with { ok: false }. `dispatch` has to normalise that shape, or the run +// welds the node shut exactly like #1389 did through { ok: false }. +function hosted(answerFor) { + const created = [] + const sent = [] + const warnings = [] + const fb = { + bot: { + list: () => roster, + async create() { created.push({}); return `bot-${created.length}` }, + async remove() { return true }, + async send(message) { sent.push(message); return answerFor(message) }, + }, + readOnly: () => false, + api: { handle() { return () => {} } }, + tools: { register() { return () => {} } }, + warn: (...args) => warnings.push(args.join(' ')), + } + const started = apply({ flybot: { scope: () => fb }, effect(fn) { fn() } }, { persist: false }) + return { service: started.service, sent, warnings } +} + +function pairNodes() { + return [ + { id: 'plan', name: 'Planner', role: 'plan', prompt: 'Write the plan.' }, + { id: 'code', name: 'Coder', role: 'code', prompt: 'Implement it.' }, + ] +} + +for (const refused of [ + { action: 'deny', reason: 'DENY_OUTSIDE', by: { bot: 'tree' } }, + { action: 'hold', approvalId: 'approval-1', reason: 'Owner approval is required', by: { bot: 'policy' } }, +]) { + test(`#1407: a ${refused.action} answer is not marked as handed and is dispatched again`, async () => { + const { service, sent, warnings } = hosted(() => refused) + const made = await service.define({ name: 'Pair', nodes: pairNodes(), edges: [] }) + const workflowId = made.workflows.find(item => item.name === 'Pair').id + const started = await service.runNamed({ workflow: 'Pair', brief: 'Ship the note' }) + assert.equal(started.ok, true) + const run = started.workflows.find(item => item.id === workflowId).run + assert.equal(run.status, 'running') + assert.equal(sent.length, 2) + assert.match(warnings.join('\n'), new RegExp(`was not handed over: ${refused.action}`)) + + // The refused step is not welded shut: the next settle sends it again, to the same Bot. + assert.deepEqual(sent.map(message => message.to).sort(), ['bot-1', 'bot-2']) + const codeTo = sent[1].to + const moved = await service.advance({ workflowId, nodeId: 'plan', outcome: 'done' }) + assert.equal(moved.ok, true) + const resent = sent.filter(message => message.to === codeTo) + assert.equal(resent.length, 2) + assert.equal(warnings.length, 3) + }) +} + +test('#1407: an allow answer is still marked as handed', async () => { + const { service, sent, warnings } = hosted(() => ({ action: 'allow' })) + const made = await service.define({ name: 'Pair', nodes: pairNodes(), edges: [] }) + const workflowId = made.workflows.find(item => item.name === 'Pair').id + const started = await service.runNamed({ workflow: 'Pair', brief: 'Ship the note' }) + assert.equal(started.ok, true) + assert.deepEqual(warnings, []) + const moved = await service.advance({ workflowId, nodeId: 'plan', outcome: 'done' }) + assert.equal(moved.ok, true) + assert.equal(sent.length, 2) +})