From 6ccbccab1ed783bf77dbe9ba9312eb597199b055 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sat, 7 Nov 2026 15:06:43 +0800 Subject: [PATCH] fix(workflow): keep a Triage step that already did the work when its state line is missing (#2583) The issue fast line's Triage node claimed the issue and opened the pull request, then ended without `OUTPUT state:`. #1773 failed the step there, so the run died at node 1 and claim/gate/enqueue never ran; the two runs in the issue each left a valid pull request behind. A missing `state` now stands when the step's own result names the pull request it opened or the claim it posted: the step completes, the run takes the node's first declared `state` branch, and the stage log and run record (outputMissing, producedPull) keep the format slip visible. Every other gate-read verdict still fails, and a step with no evidence still fails. --- changelog.d/2583.fixed.md | 1 + docs/subsystems/workflow.md | 2 +- plugins/workflow/client.js | 3 + plugins/workflow/src/graph.js | 42 +++++ plugins/workflow/src/service.js | 62 +++++++- .../test/missing-output-state.test.js | 149 ++++++++++++++++++ 6 files changed, 254 insertions(+), 5 deletions(-) create mode 100644 changelog.d/2583.fixed.md create mode 100644 plugins/workflow/test/missing-output-state.test.js diff --git a/changelog.d/2583.fixed.md b/changelog.d/2583.fixed.md new file mode 100644 index 000000000..e48ef7d90 --- /dev/null +++ b/changelog.d/2583.fixed.md @@ -0,0 +1 @@ +A workflow step that ends without the `OUTPUT state:` line its gate reads no longer kills the run when it has already done the work: a Triage node that opened a pull request or claimed the issue keeps its `done` status, the run continues, and `run.outputMissing` / `run.producedPull` record the slip and the pull request ([#2583](https://github.com/ThinkFlowLab/FlyBot/issues/2583)). diff --git a/docs/subsystems/workflow.md b/docs/subsystems/workflow.md index d070b9dc1..5b47d5020 100644 --- a/docs/subsystems/workflow.md +++ b/docs/subsystems/workflow.md @@ -59,7 +59,7 @@ Everything in `plugins/workflow/`: the graph rules, the run store, the matcher, While another dsh holds the Bot team the host is read-only (`roster.readOnly`): the page still reads through `workflow/state`, keeps its controls disabled, and offers no **Edit**, **Add link** or wire port, so it cannot take a change that core would refuse with `bot/read-only` and then drop (#2187). The same window draws the sidebar's workflow list without **New**, the row's **Rename**/**Delete** or **Hand a task to a Workflow**: the list is read-only too, and an `roster.readOnly` that is not a string, or holds only whitespace, counts as writable — trimmed the way `core/src/client/workflows.js:570` trims it (#2200). The Stage a row opens carries the same lock: it prints the host's sentence under `role="status"` and draws no **Hand a task to a Workflow**, no **Re-match**, no **Pause**/**Resume**/**Stop**, no **Edit**/drag palette and no **Reset**, while the row click, **Progress**, **Flow** and viewing a run still work; a writable roster draws every control exactly as before (#2213). Mounting the list in a read-only window still reads `projects` so the Project filter fills, but it sends no `workflow/ingest` for the open cards: ingest is a claim/match write the host refuses, so a reader opening the list writes nothing; a writable window still ingests every open card (#2222). The read-only sentence can also land after the list has mounted — a cold window starts on the empty roster, `ready: false` and no `readOnly` — so the ingest step asks the value the latest render saw rather than the one the mount saw, and it skips the board read and stops between two cards once the window has turned read-only. The Settings page draws no read-only sentence of its own: the host's Settings header already prints one for every page it shows (`core/src/client/settings.js` `bt-read-only`, `role="status"`, `hostText(roster.readOnly)`), and a second copy inside the page said the same thing twice (#2202). 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 — in a declared OUTPUT the value counts as a whole word, so a brief that names `data-open-project` is not `open-pr` (#1866) — 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, through `define_workflow`, 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 and `run.failure` say the step's deadline was reached (`step deadline reached (45 min)`, the number its lane set), and `warn` names it once; the line never reads as a reply that did not arrive, because a step still working is killed by the same timer (#1868). The deadline is cleared as soon as the step moves, and by Pause, Reset, delete, `reap`, `dispose` and a Stop that released every Bot; a run the owner paused 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. It also keeps the deadline of the node it was on — the one paused state that holds one — and a graph save re-syncs timers without taking it away (a save that cleared it left the run holding its slot with nothing left to free it); the deadline releases again there, ends the run as `paused-expired` and frees the slot, re-arming itself while a Bot still will not go (#2453, P-560). 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. 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. +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). One exception keeps the work a step already did (#2583): when `state` is the only gate-read output missing and the step's own reply shows its result — a pull request link, `pull/`, a line saying it opened PR #n, or a `Claimed-by:` / `Claimed #n` line — the step stands as `done`, the run takes the node's first declared `state` branch (on the fast line `state: ready → claim`, so the pull request reaches gate/enqueue instead of the `else` route the step never chose), and the run record carries `outputMissing: true` and `producedPull: ` when a pull request was named; every other missing verdict still fails, and a step whose reply shows no such result still fails the same way. `stepArtifacts` reads that evidence and `firstBranchValue` names the branch, both in `src/graph.js`; the engine has no GitHub client, so the reply is the only evidence it can read. 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 `when` or `until` that is present but cannot be honoured — a blank output name, or one outside `/^[a-z][a-z0-9-]{0,40}$/` such as `result_ok` — refuses the save with `A condition link needs an output name like "ready"` (or the loop's own line) instead of being dropped: before #2155 the refused `when` was indistinguishable from an absent one and the edge fired unconditionally, which is the opposite of what the author asked. An absent `when`/`until` is still a plain link, no early stop. Settings → Workflow graphs keeps **Add link** disabled for a Condition with a blank output name, and the refusal shows in the page's `.wf-err` alert. 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). A claim that yields leaves one line on the source workflow's log — `Task re-matched · → ` — in the same store write that attaches the new target, so the graph that lost the card says where it went (#1668). diff --git a/plugins/workflow/client.js b/plugins/workflow/client.js index 8f825b446..0a83ccd0d 100644 --- a/plugins/workflow/client.js +++ b/plugins/workflow/client.js @@ -164,6 +164,9 @@ window.__ModuleLoader__.load({ '✕ Failed · {task}': '✕ 失败 · {task}', '✕ Failed · {task} · {detail}': '✕ 失败 · {task} · {detail}', '{node} cannot start: missing input {missing}.': '{node} 不能开始：缺少输入 {missing}。', + '{node} did not print OUTPUT {outputs}; the step stands because it opened PR #{pull}.': '{node} 没有打印 OUTPUT {outputs}；它已开了 PR #{pull}，这一步按完成继续。', + '{node} did not print OUTPUT {outputs}; the step stands because it claimed the issue.': '{node} 没有打印 OUTPUT {outputs}；它已认领 issue，这一步按完成继续。', + '{node} did not print OUTPUT {outputs}; the step stands because it claimed the issue and opened PR #{pull}.': '{node} 没有打印 OUTPUT {outputs}；它已认领 issue 并开了 PR #{pull}，这一步按完成继续。', '{node} did not take this step ({code}).': '{node} 没有接到这一步（{code}）。', '{node} did not take this step ({code}); retrying in {seconds}s ({tries}/{total}).': '{node} 没有接到这一步（{code}），{seconds} 秒后重试（{tries}/{total}）。', '{node} did not take this step ({code}); {total} retries did not land, the run failed.': '{node} 没有接到这一步（{code}），重试 {total} 次仍未落地，run 已标记失败。', diff --git a/plugins/workflow/src/graph.js b/plugins/workflow/src/graph.js index fc48875c4..b0eb8cb1e 100644 --- a/plugins/workflow/src/graph.js +++ b/plugins/workflow/src/graph.js @@ -101,6 +101,34 @@ export function readStepResult(text, node) { return { text: body, outputs } } +// #2583: an issue fast line's Triage node claims the issue and opens the pull request +// before it prints its last line. The step's own result is the only evidence the engine +// has of that work — the plugin has no GitHub client — so the reply is read for a pull +// request it names or the claim line it posted. A bare reference ("see PR #1") is not +// evidence: a pull request counts by its URL, by `pull/`, or by a line that says it +// was opened. Anything else would let a step that only quoted an issue pass. +const PULL_URL = /https?:\/\/github\.com\/[^/\s]+\/[^/\s]+\/pull\/(\d+)/i +const PULL_PATH = /\bpull\/(\d+)\b/i +const PULL_SPOKEN = /\b(?:PR|pull request)\s*#?(\d+)/i +const OPENED = /\b(?:opened|created|raised|submitted|pushed)\b/i +const CLAIM_LINE = /^\s*claimed(?:-by)?\s*:/i +const CLAIM_ISSUE = /^\s*claimed\s+#\d+/i + +export function stepArtifacts(text, outputs) { + const haystack = [String(text ?? ''), ...Object.values(outputs ?? {}).map(value => String(value ?? ''))].join('\n') + const link = PULL_URL.exec(haystack) ?? PULL_PATH.exec(haystack) + let pull = link ? Number(link[1]) : null + if (pull == null) { + for (const line of haystack.split('\n')) { + if (!OPENED.test(line)) continue + const spoken = PULL_SPOKEN.exec(line) + if (spoken) { pull = Number(spoken[1]); break } + } + } + const claimed = haystack.split('\n').some(line => CLAIM_LINE.test(line) || CLAIM_ISSUE.test(line)) + return { pull: Number.isInteger(pull) ? pull : null, claimed } +} + function coord(value, fallback) { const number = Number(value) if (!Number.isFinite(number)) return fallback @@ -586,6 +614,20 @@ function takenFrom(graph, nodeId, outputs) { return fired } +// #2583: the branch a step that stands on its own work would have taken. The issue fast +// line declares `state: ready → claim` as its first branch, so a missing `state` line +// continues to the work nodes instead of falling into the `else` route the step never +// chose. Only used when the step stands on evidence it produced; a step with no evidence +// still fails its missing verdict (#1773). +export function firstBranchValue(graph, nodeId, name) { + ensure(graph) + for (const edge of graph.edges) { + if (edge.from !== nodeId || edge.loop === true) continue + if (edge.when && edge.when.else !== true && edge.when.output === name) return edge.when.eq ?? null + } + return null +} + // #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 diff --git a/plugins/workflow/src/service.js b/plugins/workflow/src/service.js index a3353df16..4c5fa3ad8 100644 --- a/plugins/workflow/src/service.js +++ b/plugins/workflow/src/service.js @@ -8,8 +8,8 @@ import { join } from 'node:path' import { releaseAll, spawnAll } from './ephemeral.js' import { chainLabel, cleanLine, cleanPorts, cleanPrompt, cleanTools, completeNode, cutCodePoints, doneCount, failNode, freshStatus, layerMarks, - missingProviders, missingVerdicts, nodeById, normalizeGraph, orderedIds, readStepResult, resolveInputs, retryNode, - rowPreview, startFrontier, summarizeTask, unmetRequired, validateGraph, + firstBranchValue, missingProviders, missingVerdicts, nodeById, normalizeGraph, orderedIds, readStepResult, resolveInputs, retryNode, + rowPreview, startFrontier, stepArtifacts, summarizeTask, unmetRequired, validateGraph, } from './graph.js' import { matchTask, namedTarget } from './match.js' import { linesFor } from './narrate.js' @@ -172,6 +172,10 @@ function publicRun(workflow, run) { ...(run.paused === true ? { paused: true } : {}), ...(run.failure ? { failure: { ...run.failure } } : {}), ...(run.earlyEnd ? { earlyEnd: run.earlyEnd } : {}), + // #2583: a step that ended without a gate-read OUTPUT says so, and the pull request + // it produced is named here so nobody has to search the chat for it. + ...(run.missingOutput && Object.keys(run.missingOutput).length > 0 ? { outputMissing: true } : {}), + ...(run.producedPull ? { producedPull: run.producedPull } : {}), ...(run.ephemeral && Object.keys(run.ephemeral).length > 0 ? { ephemeral: { ...run.ephemeral } } : {}), // #1862: the shadow pass's record, exposed so the run can be compared with the human path. ...(run.shadow ? { shadow: { ...run.shadow } } : {}), @@ -616,6 +620,9 @@ export function createWorkflowService({ const handed = { ...working.handed } if (op.type === 'fail' || op.type === 'retry') delete handed[op.nodeId] if (Array.isArray(result.reset)) for (const id of result.reset) delete handed[id] + const missingOutput = { ...working.missingOutput } + if (op.outputMissing) missingOutput[op.nodeId] = [...op.outputMissing] + else if (op.type === 'complete') delete missingOutput[op.nodeId] const next = { ...working, status: finished ? 'done' : stopped ? 'failed' : 'running', @@ -628,7 +635,10 @@ export function createWorkflowService({ ...(result.failAttempts ? { failAttempts: result.failAttempts } : {}), ...(Object.keys(handed).length > 0 ? { handed } : {}), ...(earlyEnded ? { earlyEnd: result.earlyEnd } : {}), + ...(op.producedPull ? { producedPull: op.producedPull } : {}), } + if (Object.keys(missingOutput).length > 0) next.missingOutput = missingOutput + else delete next.missingOutput if (Object.keys(handed).length === 0) delete next.handed if (next.status === 'failed') { const hole = blocked ? gated.blocked[0] : null @@ -666,6 +676,29 @@ export function createWorkflowService({ text: `${node?.name ?? hole.id} 不能开始：缺少输入 ${missing}。`, }) } + // #2583: the step stands on the work it did, not on the line it forgot. Say so in + // the stage log and name the pull request, so a format slip is visible as one. + if (op.type === 'complete' && op.outputMissing) { + const name = nodeById(item, op.nodeId)?.name ?? op.nodeId + const outputs = op.outputMissing.join(', ') + const pull = op.producedPull + const copy = pull != null && op.producedClaim + ? { + key: '{node} did not print OUTPUT {outputs}; the step stands because it claimed the issue and opened PR #{pull}.', + text: `${name} 没有打印 OUTPUT ${outputs}；它已认领 issue 并开了 PR #${pull}，这一步按完成继续。`, + } + : pull != null + ? { + key: '{node} did not print OUTPUT {outputs}; the step stands because it opened PR #{pull}.', + text: `${name} 没有打印 OUTPUT ${outputs}；它已开了 PR #${pull}，这一步按完成继续。`, + } + : { + key: '{node} did not print OUTPUT {outputs}; the step stands because it claimed the issue.', + text: `${name} 没有打印 OUTPUT ${outputs}；它已认领 issue，这一步按完成继续。`, + } + lines.push({ kind: 'system', key: copy.key, params: { node: name, outputs, ...(pull != null ? { pull } : {}) }, text: copy.text }) + warnOnce(`missing-output:${working.id}:${op.nodeId}`, `flybot-workflow: step ${op.nodeId} did not print OUTPUT ${outputs}; the step stands because it produced ${pull != null ? `PR #${pull}` : 'the claim'}${op.assumedState ? ` (gate reads state: ${op.assumedState})` : ''}`) + } return { run: next, lines } } @@ -1161,8 +1194,29 @@ export function createWorkflowService({ // 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(', ')}` } + const outputs = op.outputs ?? run.produced?.[op.nodeId] ?? {} + const missing = missingVerdicts(item, op.nodeId, outputs) + if (missing.length > 0) { + // #2583: a Triage step has already claimed the issue and opened the pull request + // when it ends without its `state` line. Failing the run there kills gate/enqueue + // after those writes, so the step stands when its own result shows the work, and + // the run record keeps the pull request number. Every other gate-read OUTPUT keeps + // #1773's rule: a verdict is not prose. + const artifact = missing.length === 1 && missing[0] === 'state' ? stepArtifacts(op.text, outputs) : { pull: null, claimed: false } + const stands = artifact.pull != null || artifact.claimed + // The step did the work; the gate still needs the branch it would have named. + // The graph's first `state` branch is that choice — on the issue fast line it is + // `ready → claim`, which keeps the pull request moving to gate/enqueue. + const value = stands ? firstBranchValue(item, op.nodeId, 'state') : null + op = { + ...op, + ...(stands ? {} : { type: 'fail', text: `Missing OUTPUT ${missing.join(', ')}` }), + ...(value != null && value !== '' ? { outputs: { ...outputs, state: value }, assumedState: value } : {}), + outputMissing: missing, + ...(artifact.pull != null ? { producedPull: artifact.pull } : {}), + ...(stands && artifact.claimed ? { producedClaim: true } : {}), + } + } } try { let stepped = applyOp(item, run, op) diff --git a/plugins/workflow/test/missing-output-state.test.js b/plugins/workflow/test/missing-output-state.test.js new file mode 100644 index 000000000..b4469e7cb --- /dev/null +++ b/plugins/workflow/test/missing-output-state.test.js @@ -0,0 +1,149 @@ +// #2583: an issue fast line's Triage node claims the issue and opens the pull request, +// then ends without its `state` line. The work is real and irreversible, so a format +// slip must not kill the run before gate/enqueue: the step stands, and the run record +// names the pull request. A step that produced nothing still fails (#1773). +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { firstBranchValue, stepArtifacts } from '../src/graph.js' +import { createWorkflowService } from '../src/service.js' + +const NOW = 1_700_000_000_000 +const PR_URL = 'https://github.com/ThinkFlowLab/FlyBot/pull/999' + +// The failing fast line's shape: `state: ready → claim` is the first branch and the +// `else` route goes to escalate, so a reply without `state` owes a verdict AND would +// otherwise fall into escalate rather than the work nodes. +function fastLineGraph() { + return { + nodes: [ + { id: 'triage', name: 'Triage', prompt: 'Triage the issue.' }, + { id: 'claim', name: 'Claim', prompt: 'Claim the issue.' }, + { id: 'escalate', name: 'Escalate', prompt: 'Report where it stopped.' }, + ], + edges: [ + { from: 'triage', to: 'claim', when: { output: 'state', eq: 'ready' } }, + { from: 'triage', to: 'escalate', when: { else: true } }, + ], + } +} + +function liveHarness(graph = fastLineGraph()) { + const dispatched = [] + const warnings = [] + 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 } + }, + warn: message => warnings.push(message), + }) + const made = svc.define({ name: 'Fast line', ...graph }).workflows.find(item => item.name === 'Fast line') + return { svc, id: made.id, dispatched, warnings } +} + +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 ?? '') + +async function runTriageTo(text, graph = fastLineGraph()) { + const harness = liveHarness(graph) + let views = await harness.svc.runNamed({ workflow: 'Fast line', brief: 'i2583-impl' }) + views = await harness.svc.stepResult({ botId: harness.dispatched[0].botId, text }) + return { ...harness, views } +} + +test('#2583: a Triage step that opened a PR stands and continues down its first branch', async () => { + const { views, id } = await runTriageTo(`Claimed #2583 — first step: read the issue.\nOpened ${PR_URL} with the fix.`) + const run = laneOf(views, id).run + assert.equal(run.nodeStatus.triage, 'done', 'the work stands even though the OUTPUT line is missing') + assert.equal(run.nodeStatus.claim, 'running', 'the run continues to the work node instead of dying on Triage') + assert.equal(run.nodeStatus.escalate, 'skipped', 'the else route the step never chose is not taken') + assert.equal(run.status, 'running') + assert.equal(run.outputMissing, true, 'the run record says a gate-read OUTPUT was missing') + assert.equal(run.producedPull, 999, 'the run record names the pull request the node produced') + assert.ok(messagesOf(views, id).some(text => text.includes('PR #999')), 'the stage log names the pull request') +}) + +test('#2583: the claim comment alone is enough evidence, and no PR number is invented', async () => { + const { views, id } = await runTriageTo('Claimed #2583 — first step: read the issue.') + const run = laneOf(views, id).run + assert.equal(run.nodeStatus.triage, 'done') + assert.equal(run.nodeStatus.claim, 'running') + assert.equal(run.outputMissing, true) + assert.equal(run.producedPull, undefined, 'no pull request was named') +}) + +test('#2583: a step that produced nothing still fails the run at its missing OUTPUT', async () => { + const { views, id } = await runTriageTo('Looked at the issue. Nothing to report yet.') + const run = laneOf(views, id).run + assert.equal(run.status, 'failed') + assert.equal(run.nodeStatus.triage, 'failed') + assert.equal(run.nodeStatus.claim, 'queued', 'the run does not advance past a silent step') + assert.equal(run.nodeStatus.escalate, 'queued') + assert.equal(run.outputMissing, true) + assert.match(run.failure?.text ?? '', /Missing OUTPUT state/) +}) + +test('#2583: a quoted PR reference is not evidence that this step produced one', async () => { + const { views, id } = await runTriageTo('The issue follows up on PR #999, which is already closed.') + const run = laneOf(views, id).run + assert.equal(run.status, 'failed', 'a line without a creation verb is not this step\'s work') + assert.equal(run.nodeStatus.triage, 'failed') +}) + +test('#2583: a verdict gate keeps #1773 — naming a PR does not stand in for the verdict', async () => { + const graph = { + nodes: [ + { 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: 'check', to: 'merge', when: { output: 'verdict', eq: 'PASS' } }], + } + const { views, id } = await runTriageTo(`Checked ${PR_URL} — 3 checks green.`, graph) + const run = laneOf(views, id).run + assert.equal(run.status, 'failed', 'only the state line is degraded, not every missing verdict') + assert.equal(run.nodeStatus.check, 'failed') + assert.match(run.failure?.text ?? '', /Missing OUTPUT verdict/) +}) + +test('#2583: stepArtifacts reads a pull request or a claim line, and nothing else', () => { + assert.deepEqual(stepArtifacts(`Opened ${PR_URL}`, {}), { pull: 999, claimed: false }) + assert.deepEqual(stepArtifacts('Branch pushed; see pull/999 for details.', {}), { pull: 999, claimed: false }) + assert.deepEqual(stepArtifacts('Opened PR #999 with the fix.', {}), { pull: 999, claimed: false }) + assert.deepEqual(stepArtifacts('Claimed-by: Triage', {}), { pull: null, claimed: true }) + assert.deepEqual(stepArtifacts('Claimed #2583 — first step: read the issue.', {}), { pull: null, claimed: true }) + assert.deepEqual(stepArtifacts('Follows PR #999, which is closed.', {}), { pull: null, claimed: false }) + assert.deepEqual(stepArtifacts('No work yet.', {}), { pull: null, claimed: false }) + assert.deepEqual(stepArtifacts('', { summary: `Opened ${PR_URL}` }), { pull: 999, claimed: false }, 'outputs are read too') +}) + +test('#2583: a failOnce retry that later prints its OUTPUT clears the missing-output record', async () => { + const graph = fastLineGraph() + graph.nodes[0].failOnce = true + const { svc, id, dispatched } = liveHarness(graph) + let views = await svc.runNamed({ workflow: 'Fast line', brief: 'i2583-impl' }) + views = await svc.stepResult({ botId: dispatched[0].botId, text: 'Nothing to report yet.' }) + assert.equal(laneOf(views, id).run.outputMissing, true, 'the failed attempt was a format failure') + views = await svc.stepResult({ botId: dispatched[0].botId, text: 'OUTPUT state: ready' }) + const run = laneOf(views, id).run + assert.equal(run.nodeStatus.triage, 'done', 'the retry delivered the verdict') + assert.equal(run.outputMissing, undefined, 'a delivered OUTPUT is not still recorded as missing') +}) + +test('#2583: firstBranchValue names the branch a standing step continues on', () => { + const graph = { + nodes: [{ id: 'triage', name: 'Triage' }, { id: 'claim', name: 'Claim' }, { id: 'escalate', name: 'Escalate' }], + edges: [ + { from: 'triage', to: 'claim', when: { output: 'state', eq: 'ready' } }, + { from: 'triage', to: 'escalate', when: { else: true } }, + ], + } + assert.equal(firstBranchValue(graph, 'triage', 'state'), 'ready') + assert.equal(firstBranchValue(graph, 'triage', 'status'), null) + assert.equal(firstBranchValue(graph, 'claim', 'state'), null) +})