From fbb05b06e0f540f0e57c747ff89befbde2d752b4 Mon Sep 17 00:00:00 2001 From: Dev4 Date: Fri, 30 Oct 2026 06:06:35 +0000 Subject: [PATCH 1/2] test(workflow): a silent gate verdict still finishes the run (#1773) A live step's reply advances the run without checking the OUTPUT the next edge gates on, so a step that never prints its verdict closes the run as done and the gate is skipped. Pin the run status, the node status, the recorded failure and the stage line before fixing the engine. --- plugins/workflow/test/missing-verdict.test.js | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 plugins/workflow/test/missing-verdict.test.js diff --git a/plugins/workflow/test/missing-verdict.test.js b/plugins/workflow/test/missing-verdict.test.js new file mode 100644 index 000000000..ad4d4bf06 --- /dev/null +++ b/plugins/workflow/test/missing-verdict.test.js @@ -0,0 +1,95 @@ +// #1773: a step that declares an OUTPUT its gate reads, and then ends without +// printing it, is not a finished step. Silence is not a verdict: the gate must +// never be read from it, and the run must not close as done. +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { createWorkflowService } from '../src/service.js' + +const NOW = 1_700_000_000_000 + +// Check declares two outputs on purpose: readStepResult only fills a single +// declared output from the reply body, so the gate's verdict can only be answered +// by a real `OUTPUT verdict:` line. +function gateGraph() { + return { + nodes: [ + { id: 'claim', name: 'Claim', prompt: 'Claim the issue.', outputs: [{ name: 'issue', type: 'text' }] }, + { id: 'check', name: 'Check', prompt: 'Check the pull request.', outputs: [{ name: 'summary', type: 'text' }, { name: 'verdict', type: 'text' }] }, + { id: 'merge', name: 'Merge', inputs: [{ name: 'verdict', type: 'text', required: true }] }, + ], + edges: [ + { from: 'claim', to: 'check' }, + { from: 'check', to: 'merge', when: { output: 'verdict', eq: 'PASS' } }, + ], + } +} + +function liveHarness(graph = gateGraph()) { + const dispatched = [] + let seq = 0 + const svc = createWorkflowService({ + persist: false, + now: () => NOW, + bots: () => [], + spawn: async () => `eph-${++seq}`, + release: async () => {}, + dispatch: async (step) => { + dispatched.push(step) + return { ok: true } + }, + }) + const made = svc.define({ name: 'Gate', ...graph }).workflows.find(item => item.name === 'Gate') + return { svc, id: made.id, dispatched } +} + +const laneOf = (views, id) => views.workflows.find(item => item.id === id) +const messagesOf = (views, id) => laneOf(views, id).messages.map(line => line.text ?? line.key ?? '') + +// The run the issue reported: claim passes, then Check answers with prose only. +async function runCheckToSilence() { + const { svc, id, dispatched } = liveHarness() + let views = await svc.runNamed({ workflow: 'Gate', brief: 'bs136-impl' }) + views = await svc.stepResult({ botId: dispatched[0].botId, text: 'OUTPUT issue: bs136-impl' }) + assert.deepEqual(laneOf(views, id).run.running, ['check'], 'Check is the step under test') + const checkBot = dispatched[1].botId + views = await svc.stepResult({ botId: checkBot, text: 'Retrying after a brief pause.' }) + return { svc, id, views } +} + +test('#1773: a step that never prints its verdict fails the run instead of finishing it', async () => { + const { id, views } = await runCheckToSilence() + const run = laneOf(views, id).run + assert.equal(run.status, 'failed', 'the run must not close as done') + assert.equal(run.nodeStatus.check, 'failed', 'a silent verdict is not a completed step') + assert.equal(run.nodeStatus.merge, 'queued', 'the gate is held, not read from silence') + assert.ok(!messagesOf(views, id).some(text => text.includes('✓ 完成')), 'the stage must not print the done line') +}) + +test('#1773: the run record names the step and the OUTPUT contract it missed', async () => { + const { id, views } = await runCheckToSilence() + const failure = laneOf(views, id).run.failure + assert.equal(failure?.nodeName, 'Check') + assert.match(failure?.text ?? '', /Missing OUTPUT verdict/) +}) + +test('#1773: a delivered verdict still completes the step and opens the gate', async () => { + const { svc, id, dispatched } = liveHarness() + let views = await svc.runNamed({ workflow: 'Gate', brief: 'bs136-impl' }) + views = await svc.stepResult({ botId: dispatched[0].botId, text: 'OUTPUT issue: bs136-impl' }) + views = await svc.stepResult({ botId: dispatched[1].botId, text: 'OUTPUT summary: 2 checks green\nOUTPUT verdict: PASS' }) + const run = laneOf(views, id).run + assert.equal(run.nodeStatus.check, 'done') + assert.equal(run.nodeStatus.merge, 'running', 'PASS takes the gate') + assert.equal(run.status, 'running') +}) + +test('#1773: a non-matching verdict is a skip, not a missing one', async () => { + const { svc, id, dispatched } = liveHarness() + let views = await svc.runNamed({ workflow: 'Gate', brief: 'bs136-impl' }) + views = await svc.stepResult({ botId: dispatched[0].botId, text: 'OUTPUT issue: bs136-impl' }) + views = await svc.stepResult({ botId: dispatched[1].botId, text: 'OUTPUT summary: red\nOUTPUT verdict: FAIL' }) + const run = laneOf(views, id).run + assert.equal(run.nodeStatus.check, 'done', 'a real verdict completes the step') + assert.equal(run.nodeStatus.merge, 'skipped', 'the gate answered FAIL') + assert.equal(run.status, 'done') +}) From 0df93d7d4e13a4b456ca0369dc8c482b54982f62 Mon Sep 17 00:00:00 2001 From: Dev4 Date: Fri, 30 Oct 2026 06:08:05 +0000 Subject: [PATCH 2/2] fix(workflow): fail a step that leaves its gate's verdict silent (#1773) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live run advanced on the reply alone: a step that declared the OUTPUT its next edge gates on, but never printed it, was recorded as done and the run closed as "✓ 完成" while the gate was skipped. Silence was then read the same way as a non-PASS verdict. The engine now asks which outputs the node's own `when`/`else` and `until` edges read, and a completion that leaves one of them blank fails the step instead of settling it. The gate therefore never sees a missing verdict, onFail recovery still applies, and the run record's `failure` names the node and the OUTPUT it never printed. --- .../1773-workflow-missing-verdict.fixed.md | 1 + docs/subsystems/workflow.md | 2 +- plugins/workflow/src/graph.js | 16 ++++++++++++++ plugins/workflow/src/service.js | 10 ++++++++- plugins/workflow/test/missing-verdict.test.js | 22 +++++++++++++++++++ 5 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 changelog.d/1773-workflow-missing-verdict.fixed.md diff --git a/changelog.d/1773-workflow-missing-verdict.fixed.md b/changelog.d/1773-workflow-missing-verdict.fixed.md new file mode 100644 index 000000000..5c74824e6 --- /dev/null +++ b/changelog.d/1773-workflow-missing-verdict.fixed.md @@ -0,0 +1 @@ +A Workflow step that ends without printing a declared `OUTPUT` its next gate reads now fails the step and the run instead of closing as `✓ 完成`: the downstream branch is held rather than skipped, and the run record names the step and the missing contract (`Missing OUTPUT verdict`), so "upstream emitted nothing" is no longer indistinguishable from "upstream emitted a non-PASS verdict" ([#1773](https://github.com/ThinkFlowLab/FlyBot/issues/1773)). diff --git a/docs/subsystems/workflow.md b/docs/subsystems/workflow.md index b84869c4a..775cf4293 100644 --- a/docs/subsystems/workflow.md +++ b/docs/subsystems/workflow.md @@ -50,7 +50,7 @@ Everything in `plugins/workflow/`: the graph rules, the run store, the matcher, 2. The canvas and the run share one graph: nodes, edges and positions. A drag commits `workflow/graph`, but only while **Edit** is on — a factory preset is copied first (#1378). Status highlighting reads that same graph. Replacing edges does not recompute who is already running. The Settings canvas paints that graph on the same 960px design plane, scaled to the width the settings page gives it, so the canvas is not a second scroll container inside the page's own scroll; its pointer deltas are divided by the scale, and its wires keep a stronger stroke than a plain divider so an edge can be followed at that size (#1483). 3. **A live run advances only when its ephemeral Bot replies, following the graph's edges in order.** That reply advances the run and may appear on the workflow stage log; it is not delivered into a persistent sidebar Bot conversation (for example Chief). The `workflow-step` send middleware denies those replies after `stepResult` runs. Nodes start only when their forward predecessors finish and edges reach them. **Early termination (#1537, #1538, #1546, #1570, #1571):** When a node completes **or fails**, the run ends immediately with status `done`, sets `run.earlyEnd` to the condition name, and does not advance to successors, take a success-path `loop`, or burn `onFail` recovery attempts. The two markers that report the task itself end it only on a structured signal (#1546): `needs-human` and `already-claimed` must be a declared OUTPUT whose value **equals** the marker, or a whole reply line that is exactly `EARLY-END ` in any case. A step that quotes an issue title, negates the label or reports it absent keeps running. `tip-unchanged` (#1570) and `open-pr` (#1571) accept that same structured signal and also honor their prose fallbacks (`tip unchanged` / `unchanged tip`, `open PR already` / `already has an open PR` / `has an open PR` / `found an open PR`), so a Bot that forgets the structured marker cannot soft-lock Claim↔Verify on the same tip. See `checkEarlyEnd` in Key files. Factory prompt guidance lives in `EARLY_END_GUIDANCE` (`plugins/workflow/src/template.js`) for Settings graphs that wire Triage / Claim / Verify: Verify must escalate once on an unchanged tip, not re-run the full suite. There is no scripted or seeded run path: `workflow/state` exposes only `live === true` runs — the running ones, plus the last run when it is still `live` and ended `failed` — a failed sibling stays on the stage beside whatever is running now, with `run.failure` (`{ code, at, nodeName?, text? }`) naming what ended it, so the room does not have to open Runs to learn that a run failed (#1493, #1494). The failed sibling is left out when a running run carries the same task id: that is a retry, and the live run keeps the row (#1507). History entries carry the run `id` and the same `failure`, so a Runs row shows when the run ended and why, and a row for a run already on the stage is not listed twice. A run does not create a schedule. `scheduleLabel` is display text. When the host can create Bots, starting a live run (`assign`, `run_workflow`, `ingest`) spawns one ephemeral Bot per node from that node's prompt, role, model and tools, sends it the step from the Main Bot, and removes every instance when the run finishes, fails for good, or is reset. A `failOnce` retry keeps them. They are hidden and left out of the sidebar. Loading the plugin does not spawn Bots for the default workflows. Plugin unload (`dispose`) and the load-time `reap` both end a live run still marked `running`: in-flight nodes are failed, the run is archived as `failed`, and any ephemeral Bot ids are released. A run already written as `running` with no ephemeral map (an older unload) is healed the same way on the next load (#1414). Loading is also the one moment a run the host left behind is noticed when its ephemeral map is still present: `reap` releases those instances and records the run as `failed`. Nothing resumes it. A run that already ended keeps its own status (#1388). `workflow/reset` stopping one live run ends it the same way before its Bots are released: nodes still `running` become `failed`, so the strip cannot keep painting a run that is over (#1424). A graph edit that drops a node that is still running ends that run the way Stop does: the node is failed, the run is archived `failed`, its node Bots are released, and the queue drains (#1549); an id whose release failed stays named on that archived run, and the next load's `reap` retries it, instead of the edit forgetting it (the #1616 contract). Every wait is bounded (#1530). A dispatch that does not land is not marked handed: the chat names the code and the attempt, and it is retried after a short backoff at most twice, never twice inside one hand-off, so a 429 is not hammered. When it still does not land, the step and the run fail with the reason written into the chat. Each running live step also holds a deadline — its workflow's `stepTimeoutMinutes`, `DEFAULT_STEP_TIMEOUT_MINUTES` (45 minutes) unless the record sets one, an integer from 1 to 120 given at create or through `workflow/step-timeout` (armed even when the host cannot spawn ephemeral Bots; #1530, #1580); when it expires the node and the run fail as `failed`, the run's ephemeral Bots are released, the chat says the step timed out, and `warn` names it once. The deadline is cleared as soon as the step moves, and by Pause, Stop, Reset, delete, `reap` and `dispose`; a paused run holds none, and Resume arms it again from that moment. A timer never outlives the run that started it. A reply that matches no running step — its run ended, or the node already moved — is reported through `warn` once per Bot and run instead of being dropped silently. A reply that says `could not answer your message` or `model call failed` fails that node and does not start its successor. A 429 or rate-limit mention in any other reply does not. Stop and Reset (`workflow/reset`) follow the same order as `dispose`: they release the run's Bots before the record forgets their ids, and an id whose release failed stays named on the still-live run, so the next Stop, Reset, dispose or load-time `reap` retries it (#1476). A run left that way is marked `paused` and `releasePending` and shows as Paused: it hands off no further node, `resume` is refused with an error, and Stop again is the retry. A whole-workflow Reset that could not release every Bot freezes every run it did not reset, because each of them has lost the ids whose release did go. The markers go away when the run ends: Stop again, `dispose`, `reap`, or its own completion. 4. Each workflow keeps a FIFO queue and up to `maxConcurrent` live runs (default 2, an integer from 1 to 8 on the record, via `workflow/concurrency`, or in Settings → Workflow graphs). A task that does not fit is enqueued. When a run finishes, every workflow with a free slot is drained, oldest enqueue first. A new assign on a workflow that already has a queue waits behind it. A task does not replace a running run or release that run's Bots. Migrating an old store keeps only runs that were still `running` with ephemeral Bot ids; other legacy scripted rows are dropped from active state (finished tasks may remain in lane history). History records a finished or failed live run. Lowering the cap does not stop a run that has already started. Assigning `{ botId }` claims the card for that local Bot and does not enqueue. A remote Bot is refused. `activeCount` is `runningOf(lane)` — live runs with `live === true` and `status === 'running'`. The stage aggregate chip and the Runs tab (运行记录) must agree: every such run appears as its own running row and progress card with its own `status` (never a hardcoded Running), the chip count does not exceed that list, and a failed sibling appears with its time and reason. On the Runs list, the clock stays in a non-shrinking column with a gap before the status chip, and a Failed row paints the Failed chip once — a generic Failed with no host detail does not also draw `wf-hist-reason` (#1618). -5. A node declares up to eight inputs and eight outputs. A required input is an output of the same name on an ancestor, or `task`, `title`, or `brief` on a root. A start with no provider is refused, and Bots spawned for that attempt are released. On a Bot-backed run a successor stays queued until its required inputs are present. If every forward predecessor is done, failed or skipped and an input is still missing, that node fails and the run stops. The dispatch text carries `Input : `. The Bot reports `OUTPUT : `; a single declared output may be the reply body. Nodes start in edge order. +5. A node declares up to eight inputs and eight outputs. A required input is an output of the same name on an ancestor, or `task`, `title`, or `brief` on a root. A start with no provider is refused, and Bots spawned for that attempt are released. On a Bot-backed run a successor stays queued until its required inputs are present. If every forward predecessor is done, failed or skipped and an input is still missing, that node fails and the run stops. The dispatch text carries `Input : `. The Bot reports `OUTPUT : `; a single declared output may be the reply body. A declared output a gate reads is required (§3): when a `when`/`else` or `until` edge reads it and the step's reply does not produce it, the step fails with `failure.text` `Missing OUTPUT ` instead of settling, so the gate never reads silence as "not this branch" and the run cannot close `done` on it; a delivered value that does not equal `eq` still completes the step and skips the untaken branch (#1773). Nodes start in edge order. 6. Each node is a real ephemeral Bot for that run, then it is removed. A permanent sidebar Bot (`botId`) is optional and is not required for the step to run. **File and shell rights are the same as a normal sidebar Bot's:** every node Bot gets the `danger-full-access` preset, and `EPHEMERAL_DENIED_TOOLS` still refuses the team-admin tools at the guard and approval layers (#1466, 29 names). **`node.tools` selects connectors only:** `spawn` passes the node's cleaned list to `fb.bot.create`, the host records it as `stepTools`, and `connector-access.js` allows `mcp__github__*` for `github` and `mcp__composio__*` for `composio` — a connector the node did not name is refused, and a node that names none reaches none. The list is also stored in `fields.tools` for the UI. A hidden run Bot cannot show an approval card, so the approval seam answers the same way the guard does. The host stores this on the run Bot as `stepTools` and `permission`. 7. A loop edge (`loop: true`, `max` from 1 to 8, default 3, optional `until: { output, eq }`) is the only cycle for success paths. The body runs again until `until` matches or `max` runs have finished, and the forward edges wait until then. Restarting the body drops the branch choice made inside it and queues a sibling that the previous pass skipped, so the next pass can take the other side. A `when: { output, eq }` edge is taken only when that output equals `eq`. `when: { else: true }` is taken only when no other `when` matched. A node whose forward edges are all closed is `skipped` and does not block a join. A run cannot spin: every loop has a max. An `onFail` edge (`onFail: true`, `max` recoveries from 1 to 8, default 3) provides failure recovery: when a node fails, if an `onFail` edge points from that node back to an upstream target and recoveries remain (tracked in `failAttempts` per edge), the workflow resets every node from the recovery target through the failing node (the target to `running`, the rest to `queued`) and re-runs them in order. After `max` recoveries are used, the node fails permanently and the run stops. An `onFail` edge does not interfere with `failOnce` retry (which restarts the same node once) or success-path `loop` edges. Settings → Workflow graphs can add a link, a condition, or a loop; a failure recovery edge is added through `define_workflow` (`onFail: true`) and has no Settings control yet. 8. Auto-assign considers an `open` card only. Every saved board write reaches the hook, including a card a Bot records for itself through `track_tasks`, so a `workflow:` on the card a Bot wrote starts that run (#1462). A saved Bot claim beats a workflow trigger. Otherwise first hit wins: a `workflow:` token, then `workflowId` / `workflow`, then an assignee that names a workflow, then a `bot:` token, then an assignee that names a local Bot, then the longest trigger in the title, note, criteria or tags — an ASCII trigger must match a whole word (`notice` does not match `noticeable`, `pr` does not match `Improve`), while a non-ASCII trigger matches inside a longer text because CJK has no word boundary. A miss returns no route. A second ingest of the same task and target is a no-op, and a live run for that task id is not restarted. A stored claim yields only to the card's own named target — a `workflow:`/`bot:` token, `workflowId`/`workflow`, or an assignee naming a workflow or local Bot — and only when that target is live and different: the claim and its route go, the card is matched again, and exactly one of each remains. A trigger, or a card that names nothing, never invalidates a claim; a claim naming a workflow or Bot that no longer exists keeps today's behaviour, because this invalidates on named intent and does not guess. `heldTasks`/`heldTaskCount` on `workflow/state` are that claim map read back per workflow, and `rematch: true` is the person's explicit drop for one task (#1647). diff --git a/plugins/workflow/src/graph.js b/plugins/workflow/src/graph.js index f6109da70..4561afdb4 100644 --- a/plugins/workflow/src/graph.js +++ b/plugins/workflow/src/graph.js @@ -560,6 +560,22 @@ function takenFrom(graph, nodeId, outputs) { return fired } +// #1773: the outputs a gate reads are verdicts, not prose. A `when`/`else` or `until` +// edge decides from them, so a reply that leaves one blank is silence, and the gate +// would take it for "not this branch". A step must not settle on that: name what it +// owes and let the caller fail the step. +export function missingVerdicts(graph, nodeId, outputs) { + ensure(graph) + if (!nodeById(graph, nodeId)) return [] + const read = new Set() + for (const edge of graph.edges) { + if (edge.from !== nodeId) continue + if (edge.when && edge.when.else !== true && edge.when.output) read.add(edge.when.output) + if (edge.until?.output) read.add(edge.until.output) + } + return [...read].filter(name => String(outputs?.[name] ?? '').trim() === '') +} + function loopDecision(graph, nodeId, outputs, loops, text) { // Do not take a success-path loop when the completion already signals stop. if (checkEarlyEnd(outputs, text).shouldEnd) return null diff --git a/plugins/workflow/src/service.js b/plugins/workflow/src/service.js index 546fc9e63..e44db96a9 100644 --- a/plugins/workflow/src/service.js +++ b/plugins/workflow/src/service.js @@ -8,7 +8,7 @@ import { join } from 'node:path' import { releaseAll, spawnAll } from './ephemeral.js' import { chainLabel, cleanLine, cleanPorts, cleanPrompt, cleanTools, completeNode, doneCount, failNode, freshStatus, layerMarks, - missingProviders, nodeById, normalizeGraph, orderedIds, readStepResult, resolveInputs, retryNode, + missingProviders, missingVerdicts, nodeById, normalizeGraph, orderedIds, readStepResult, resolveInputs, retryNode, rowPreview, startFrontier, summarizeTask, unmetRequired, validateGraph, } from './graph.js' import { matchTask, namedTarget } from './match.js' @@ -932,6 +932,14 @@ export function createWorkflowService({ const run = runFor(workflowId, op.runId, op.nodeId) if (!run || run.status === 'idle') return { ok: false, error: 'This workflow has no running task' } if (run.status === 'done' || run.status === 'failed') return { ok: false, error: 'This run is already finished' } + // #1773: a step whose gate reads a declared OUTPUT has to deliver it. Completing on + // silence skips every branch the gate guards and the run still closed as done, so the + // missing verdict fails the step here — the gate never runs, and the run's `failure` + // names the node and the OUTPUT it did not print. + if (op.type === 'complete') { + const missing = missingVerdicts(item, op.nodeId, op.outputs ?? run.produced?.[op.nodeId] ?? {}) + if (missing.length > 0) op = { ...op, type: 'fail', text: `Missing OUTPUT ${missing.join(', ')}` } + } try { let stepped = applyOp(item, run, op) if (op.type === 'fail' && nodeById(item, op.nodeId)?.failOnce === true && stepped.run.status === 'running') { diff --git a/plugins/workflow/test/missing-verdict.test.js b/plugins/workflow/test/missing-verdict.test.js index ad4d4bf06..e75eba7c5 100644 --- a/plugins/workflow/test/missing-verdict.test.js +++ b/plugins/workflow/test/missing-verdict.test.js @@ -3,6 +3,7 @@ // never be read from it, and the run must not close as done. import assert from 'node:assert/strict' import { test } from 'node:test' +import { missingVerdicts, normalizeGraph } from '../src/graph.js' import { createWorkflowService } from '../src/service.js' const NOW = 1_700_000_000_000 @@ -93,3 +94,24 @@ test('#1773: a non-matching verdict is a skip, not a missing one', async () => { assert.equal(run.nodeStatus.merge, 'skipped', 'the gate answered FAIL') assert.equal(run.status, 'done') }) + +test('#1773: only the outputs a gate reads are required, and silence names each one', () => { + const graph = normalizeGraph({ + nodes: [ + { id: 'check', name: 'Check', outputs: [{ name: 'summary' }, { name: 'verdict' }, { name: 'detail' }] }, + { id: 'merge', name: 'Merge' }, + ], + edges: [{ from: 'check', to: 'merge', when: { output: 'verdict', eq: 'PASS' } }], + }) + assert.deepEqual(missingVerdicts(graph, 'check', {}), ['verdict']) + assert.deepEqual(missingVerdicts(graph, 'check', { summary: 'green' }), ['verdict']) + assert.deepEqual(missingVerdicts(graph, 'check', { verdict: 'FAIL' }), []) + const plain = normalizeGraph({ + nodes: [ + { id: 'plan', name: 'Plan', outputs: [{ name: 'plan' }] }, + { id: 'code', name: 'Code' }, + ], + edges: [{ from: 'plan', to: 'code' }], + }) + assert.deepEqual(missingVerdicts(plain, 'plan', {}), [], 'an output no gate reads stays optional') +})