From 5555062be49a88dfcf9f5e62c527a8191a0bb027 Mon Sep 17 00:00:00 2001 From: FlyBot Dev-5 Date: Thu, 5 Nov 2026 01:18:21 +0000 Subject: [PATCH] workflow: let an explicit brief replace or clear the cached facts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit assign() resolved the brief as `callBrief || prior.brief`, so `brief: ''` could never clear the facts a lane had cached, and delete() left the entry behind — a later dispatch carried old instructions and recorded a handoff from a lane that no longer existed (#2475). A call that passes `brief` is now the authority, `''` included; a call that omits it still carries the card's facts (#2468). delete() drops the entries whose workflowId is the lane it removes, and a handoff is recorded only from a lane that still exists. --- changelog.d/2475-brief-cache.fixed.md | 1 + docs/subsystems/workflow.md | 2 +- plugins/workflow/src/service.js | 23 ++++-- plugins/workflow/test/brief-cache.test.js | 98 +++++++++++++++++++++++ 4 files changed, 115 insertions(+), 9 deletions(-) create mode 100644 changelog.d/2475-brief-cache.fixed.md create mode 100644 plugins/workflow/test/brief-cache.test.js diff --git a/changelog.d/2475-brief-cache.fixed.md b/changelog.d/2475-brief-cache.fixed.md new file mode 100644 index 000000000..bd83a743b --- /dev/null +++ b/changelog.d/2475-brief-cache.fixed.md @@ -0,0 +1 @@ +A workflow assignment's brief is now the caller's to clear. An explicit `brief` replaces the facts cached on the lane, and `brief: ''` clears them instead of falling back to the old text; deleting a workflow drops the facts it cached, so a later assignment carries nothing from it and records no handoff naming a lane that no longer exists ([#2475](https://github.com/ThinkFlowLab/FlyBot/issues/2475)). diff --git a/docs/subsystems/workflow.md b/docs/subsystems/workflow.md index 9d0592ade..cf0d775ff 100644 --- a/docs/subsystems/workflow.md +++ b/docs/subsystems/workflow.md @@ -38,7 +38,7 @@ Everything in `plugins/workflow/`: the graph rules, the run store, the matcher, | --- | --- | | `plugins/workflow/index.js` | Plugin entry (`flybot-workflow`). Endpoints, `run_workflow`, `define_workflow`, the board listener, and the reply middleware that ingests a step result and denies delivery to persistent Bot chats. | | `plugins/workflow/src/graph.js` | Nodes, edges, positions, and named inputs and outputs. A node starts when its forward predecessors are settled and a taken edge reaches it. Same-rank nodes run together. A forward cycle is refused. A loop edge is the only cycle for success paths. A `when` edge chooses a branch; the other is skipped. A required input with no upstream provider is refused before a run starts. An `onFail` edge provides failure recovery: when a node fails with recoveries remaining, the workflow resets the body from the upstream target through the failing node and re-runs it in order (tracked by `failAttempts` per edge, with `max` counting recoveries, 1–8, default 3). After the recoveries are used, the node fails permanently. | -| `plugins/workflow/src/service.js` | The store, lanes (active runs, FIFO queue, terminal history), `maxConcurrent`, `rename`, `delete`, `saveGraph`, `define`, `runNamed`, `ingest`, `stepResult`. `rename` updates a workflow's name and optional description; `delete` removes the workflow, releases ephemeral Bots, and cleans logs/runs/routes and the task claims that named it — a claim is judged on its own record (`kind === 'workflow'` and that id), not by the route map, which its own cleanup has already emptied (#1632); a run Bot whose release failed is kept in the store's own `orphans` list — not in a variable — and the load-time `reap` retries it, once per id, capped, so deleting a workflow whose Bot will not go does not strand that Bot under a name nothing can reach (the #1616 contract). Factory template ids (e.g. `delivery`) are normal saved rows and may be deleted. A spawn hook makes one run Bot per node. Disposing the plugin ends every live run still marked `running` (nodes in flight are failed, the run is archived) and then releases every run Bot and drops those ids from the saved record only once the release has finished, handing the release back as a promise the host awaits, so a shutdown cannot persist a run as `running` with no ephemeral map and cannot leave hidden Bots that no state names, for every live run in a lane rather than only the one archived last (#1387, #1414, #1472). A released run Bot also gives its runner name back to the allocator instead of keeping it for the lifetime of the process. A dispatch that does not land is not marked handed: it is retried twice with a short backoff, and a step still unanswered after its workflow's `stepTimeoutMinutes` (45 minutes by default) fails the run and releases its Bots (#1530, #1580). A stored claim keeps a task only while the card names nothing new: when the card's own fields name another live workflow or Bot, the claim and its route are dropped and the card is matched again, and `rematch: true` on `ingest` drops that one claim first (#1647). `workflow/state` carries, per workflow, the ids it holds — `heldTasks`, a page of 20 sorted ids beside the whole `heldTaskCount` — and a Bot claim is not listed there (#1647); the read takes a `heldLimit` — 20 by default and 200 at most, read through `Number()` and kept only when it is a positive integer, so `'10'` reads 10 and `true` reads 1 while `0`, `-5`, `3.5`, `'many'` and `null` fall back to 20 — so a reader can ask for the rest of the page without turning the poll into a full read (#2256, #2260). Each public run also carries `finishedAt` (`null` while the run is live, else the moment it ended) and `currentStep` (`{ id, name }` of the node it walks now, else `null`) — pure additions for the Inbox, which reads them off `workflow/state` (#1706); a paused run keeps `status: 'running'` with `paused: true`, so it is not finished and still names its node. An assignment carries its instruction: `assign` reads `brief` — multi-line, up to 8000 characters, kept whole rather than flattened into the 300-character title — onto `task.brief`, and resolves it from the card first: once the card has a brief that brief wins, a call that still passes `brief` works and stores it on a card that has none, and re-dispatching the same card to another workflow keeps the same facts, records `task.handoff` `{from, to, at, reason}` naming the lane it moved from and to, and hands the next node the stored facts (a brief input in `resolveInputs` falls back to the title only when the card has none; #2468). It, a parked card keeps it in `workflow/state` and in `workflows.json`, and `stepBody` hands the node `task.brief`, falling back to the title only when there is none; before #1869 the node received the title twice and the brief never. `runNamed` gives a brief the same meaning and derives the title from its first line, and a field `assign` does not read comes back in `ignored` instead of disappearing (#1869). | +| `plugins/workflow/src/service.js` | The store, lanes (active runs, FIFO queue, terminal history), `maxConcurrent`, `rename`, `delete`, `saveGraph`, `define`, `runNamed`, `ingest`, `stepResult`. `rename` updates a workflow's name and optional description; `delete` removes the workflow, releases ephemeral Bots, and cleans logs/runs/routes and the task claims that named it — a claim is judged on its own record (`kind === 'workflow'` and that id), not by the route map, which its own cleanup has already emptied (#1632); a run Bot whose release failed is kept in the store's own `orphans` list — not in a variable — and the load-time `reap` retries it, once per id, capped, so deleting a workflow whose Bot will not go does not strand that Bot under a name nothing can reach (the #1616 contract). Factory template ids (e.g. `delivery`) are normal saved rows and may be deleted. A spawn hook makes one run Bot per node. Disposing the plugin ends every live run still marked `running` (nodes in flight are failed, the run is archived) and then releases every run Bot and drops those ids from the saved record only once the release has finished, handing the release back as a promise the host awaits, so a shutdown cannot persist a run as `running` with no ephemeral map and cannot leave hidden Bots that no state names, for every live run in a lane rather than only the one archived last (#1387, #1414, #1472). A released run Bot also gives its runner name back to the allocator instead of keeping it for the lifetime of the process. A dispatch that does not land is not marked handed: it is retried twice with a short backoff, and a step still unanswered after its workflow's `stepTimeoutMinutes` (45 minutes by default) fails the run and releases its Bots (#1530, #1580). A stored claim keeps a task only while the card names nothing new: when the card's own fields name another live workflow or Bot, the claim and its route are dropped and the card is matched again, and `rematch: true` on `ingest` drops that one claim first (#1647). `workflow/state` carries, per workflow, the ids it holds — `heldTasks`, a page of 20 sorted ids beside the whole `heldTaskCount` — and a Bot claim is not listed there (#1647); the read takes a `heldLimit` — 20 by default and 200 at most, read through `Number()` and kept only when it is a positive integer, so `'10'` reads 10 and `true` reads 1 while `0`, `-5`, `3.5`, `'many'` and `null` fall back to 20 — so a reader can ask for the rest of the page without turning the poll into a full read (#2256, #2260). Each public run also carries `finishedAt` (`null` while the run is live, else the moment it ended) and `currentStep` (`{ id, name }` of the node it walks now, else `null`) — pure additions for the Inbox, which reads them off `workflow/state` (#1706); a paused run keeps `status: 'running'` with `paused: true`, so it is not finished and still names its node. An assignment carries its instruction: `assign` reads `brief` — multi-line, up to 8000 characters, kept whole rather than flattened into the 300-character title — onto `task.brief`, and resolves it from the card first: a call that omits `brief` keeps the card's stored facts, a call that passes one stores it on a card that has none, and re-dispatching the same card to another workflow keeps the same facts, records `task.handoff` `{from, to, at, reason}` naming the lane it moved from and to, and hands the next node the stored facts (a brief input in `resolveInputs` falls back to the title only when the card has none; #2468). A call that passes `brief` is the latest word — `brief: ''` clears the card's facts instead of falling back to the cached ones — and deleting a lane drops the facts it cached, so a later dispatch records no handoff from a lane that no longer exists (#2475). It, a parked card keeps it in `workflow/state` and in `workflows.json`, and `stepBody` hands the node `task.brief`, falling back to the title only when there is none; before #1869 the node received the title twice and the brief never. `runNamed` gives a brief the same meaning and derives the title from its first line, and a field `assign` does not read comes back in `ignored` instead of disappearing (#1869). | | `plugins/workflow/src/ephemeral.js` | Builds each node's run Bot from its prompt, role, model and tools, and releases the ids the host returns. `node.tools` names the connectors that node may call: `spawn` passes the cleaned list to `fb.bot.create`, the host records it as `stepTools`, and `connector-access.js` allows only the connectors it names (`github` covers `mcp__github__*`, `composio` covers `mcp__composio__*`) — a node that names none reaches none. File and shell rights do not come from that list: every node Bot gets the `danger-full-access` preset a normal sidebar Bot has, and `EPHEMERAL_DENIED_TOOLS` still refuses the team tools at the guard and approval layers. The list is also stored in `fields.tools` for the UI. | | `plugins/workflow/src/match.js` | Which open card matches a Workflow or a local Bot. | | `plugins/workflow/src/template.js` | Factory delivery/weekly graphs plus `EARLY_END_GUIDANCE` for Claim/Verify/Triage early-end markers; 发布交付流水线 (a parallel branch) and the weekly report, each node with the inputs and outputs that edge carries. | diff --git a/plugins/workflow/src/service.js b/plugins/workflow/src/service.js index 943081fb5..6559083bd 100644 --- a/plugins/workflow/src/service.js +++ b/plugins/workflow/src/service.js @@ -1216,6 +1216,11 @@ export function createWorkflowService({ const claim = state.claims[taskId] if (claim?.kind === 'workflow' && claim.id === workflowId) delete state.claims[taskId] }) + // #2475: the lane held the card's facts. Once it is gone they are nobody's, + // and a later assignment must not inherit them from the deleted id. + Object.keys(state.briefs ?? {}).forEach(taskId => { + if (state.briefs[taskId]?.workflowId === workflowId) delete state.briefs[taskId] + }) write() return present() } @@ -1309,18 +1314,20 @@ export function createWorkflowService({ // #1869: a 20–40 line brief is the normal case, so it keeps its line breaks // (cleanPrompt) rather than being flattened into one 300-character line. const callBrief = cleanPrompt(body.brief, MAX_BRIEF) - // #2468: the card holds the facts; a lane is only an executor. A call that still - // passes `brief` keeps working, but once the card has a brief that stored brief - // wins, so a re-dispatch to another lane carries the same facts. A call that - // brings a brief for a card without one stores it on the card. + // #2468: the card holds the facts; a lane is only an executor. A call that + // omits `brief` keeps the card's stored facts, so a re-dispatch carries them. + // #2475: an explicit `brief` is the authority and replaces the cached one, and + // `brief: ''` clears it — otherwise a card could never drop old instructions. state.briefs ??= {} const prior = state.briefs[task.id] ?? {} - const brief = callBrief || prior.brief || '' - const fromLane = prior.workflowId && prior.workflowId !== workflowId ? prior.workflowId : '' + const brief = body.brief === undefined ? prior.brief || '' : callBrief + // #2475: a lane that no longer exists records no handoff; delete() drops its + // cache entries, and this guard covers an entry written before that fix. + const kept = prior.workflowId && prior.workflowId !== workflowId ? workflow(prior.workflowId) : undefined state.briefs[task.id] = { ...(brief ? { brief } : {}), workflowId } if (brief) task.brief = brief - if (fromLane) { - task.handoff = { from: workflow(fromLane)?.name ?? fromLane, to: item.name, at: now(), reason: 're-dispatched to another lane' } + if (kept) { + task.handoff = { from: kept.name, to: item.name, at: now(), reason: 're-dispatched to another lane' } } write() // #1544: the Bot whose tool call started this run, when a Bot started it. The board, the canvas diff --git a/plugins/workflow/test/brief-cache.test.js b/plugins/workflow/test/brief-cache.test.js new file mode 100644 index 000000000..5e2a1e5a5 --- /dev/null +++ b/plugins/workflow/test/brief-cache.test.js @@ -0,0 +1,98 @@ +// #2475: the brief an assignment stores on the lane is a cache, and the cache +// only ever grew. An empty brief could not clear it, and deleting the lane left +// its entry behind, so the next assignment carried facts nobody holds and a +// handoff named a lane that no longer exists. +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { createWorkflowService } from '../src/service.js' +import { DELIVERY_ID, WEEKLY_ID } from '../src/template.js' + +const CLOCK = 1_700_000_000_000 + +function service() { + return createWorkflowService({ persist: false, now: () => CLOCK, bots: () => [] }) +} + +function named(svc, name) { + const created = svc.create({ + name, + nodes: [{ id: 'only', name: 'Only', prompt: 'Do it.' }], + edges: [], + }) + assert.equal(created.ok, true, created.error) + return created.workflows.find(item => item.name === name) +} + +const lane = (svc, id) => svc.state().workflows.find(item => item.id === id) + +function cardOf(svc, id, taskId) { + const item = lane(svc, id) + return item.runs.find(run => run.task?.id === taskId) ?? item.queue.find(run => run.id === taskId) +} + +test('#2475: an empty brief clears the facts the card already carries', () => { + const svc = service() + svc.assign({ workflowId: DELIVERY_ID, taskId: 'task-a2', title: 'B card', brief: 'OLD facts.' }) + svc.assign({ workflowId: WEEKLY_ID, taskId: 'task-a2', title: 'B card', brief: '' }) + assert.equal( + cardOf(svc, WEEKLY_ID, 'task-a2').task.brief, + undefined, + "brief: '' must clear the card's facts", + ) +}) + +test('#2475: an explicit brief replaces the facts the card already carries', () => { + const svc = service() + svc.assign({ workflowId: DELIVERY_ID, taskId: 'task-a4', title: 'D card', brief: 'OLD facts.' }) + svc.assign({ workflowId: WEEKLY_ID, taskId: 'task-a4', title: 'D card', brief: 'NEW facts.' }) + assert.equal( + cardOf(svc, WEEKLY_ID, 'task-a4').task.brief, + 'NEW facts.', + "the call's brief, not the cached one, is what the node receives", + ) +}) + +test('#2475: deleting a lane drops the facts it cached, and the next handoff names no deleted lane', () => { + const svc = service() + svc.assign({ workflowId: DELIVERY_ID, taskId: 'task-a3', title: 'C card', brief: 'OLD facts.' }) + svc.delete({ workflowId: DELIVERY_ID }) + svc.assign({ workflowId: WEEKLY_ID, taskId: 'task-a3', title: 'C card' }) + + const moved = cardOf(svc, WEEKLY_ID, 'task-a3') + assert.equal(moved.task?.brief, undefined, 'the deleted lane kept no facts for the card') + assert.equal( + moved.task?.handoff, + undefined, + 'the lane it left is gone, so there is no handoff to record', + ) +}) + +// #2468: a call that omits the brief means "keep the card's facts", so the +// cache is what carries a re-dispatch. The clear has to be explicit (above). +test('#2475: a re-dispatch that passes no brief still carries the card facts', () => { + const svc = service() + const facts = 'Fact one.\nFact two.' + svc.assign({ workflowId: DELIVERY_ID, taskId: 'task-a5', title: 'E card', brief: facts }) + svc.assign({ workflowId: WEEKLY_ID, taskId: 'task-a5', title: 'E card' }) + + const moved = cardOf(svc, WEEKLY_ID, 'task-a5') + assert.equal(moved.task.brief, facts) + assert.equal(moved.task.handoff.from, lane(svc, DELIVERY_ID).name) +}) + +test('#2475: the re-match line on the source graph still lands after the card carries facts', async () => { + const svc = service() + const alpha = named(svc, 'Alpha') + const beta = named(svc, 'Beta') + + const first = await svc.ingest({ id: 't-1', status: 'open', title: 'Ship it', workflowId: alpha.id }) + assert.deepEqual(first.decision, { kind: 'workflow', id: alpha.id }) + svc.assign({ workflowId: alpha.id, taskId: 't-1', title: 'Ship it', brief: 'Facts.' }) + + const moved = await svc.ingest({ id: 't-1', status: 'open', title: 'Ship it', workflowId: beta.id }) + assert.equal(moved.already, false) + + const reroutes = lane(svc, alpha.id).messages.filter(entry => entry.key === 'Task re-matched · {task} → {to}') + assert.equal(reroutes.length, 1, 'the source graph keeps exactly one line saying where the card went') + assert.equal(reroutes[0].params.to, 'Beta') +})