From c815445acc73856acd25cd5e6b17b42b63b93edf Mon Sep 17 00:00:00 2001 From: HugoXK Date: Thu, 5 Nov 2026 00:45:49 +0000 Subject: [PATCH] workflow: a re-matched card leaves one line in the source graph's log When ingest re-matched a card held by one workflow into another (#1647), only the target side was written: the source workflow's log did not change at all, so a person watching that graph saw a held card vanish with nothing to read. Push one system line onto the source graph naming the card and the destination, written in the same mutation as the new claim and never by an extra write(); the target side, the same-target path and the already:true path append nothing. The source line names the card by its id and title and the destination by its workflow name, with the Chinese fallback filled the way the other stage lines are. Fixes #1668 --- changelog.d/1668.fixed.md | 1 + docs/subsystems/workflow.md | 4 +- plugins/workflow/src/service.js | 16 ++ .../workflow/test/claim-reroute-log.test.js | 168 ++++++++++++++++++ 4 files changed, 187 insertions(+), 2 deletions(-) create mode 100644 changelog.d/1668.fixed.md create mode 100644 plugins/workflow/test/claim-reroute-log.test.js diff --git a/changelog.d/1668.fixed.md b/changelog.d/1668.fixed.md new file mode 100644 index 00000000..87c00799 --- /dev/null +++ b/changelog.d/1668.fixed.md @@ -0,0 +1 @@ +A card re-matched from one workflow into another now leaves one line in the source graph's log naming the card and its destination, so the hand-over is visible where the card disappeared ([#1668](https://github.com/ThinkFlowLab/FlyBot/issues/1668)). diff --git a/docs/subsystems/workflow.md b/docs/subsystems/workflow.md index 3d52bc7f..dfa64b15 100644 --- a/docs/subsystems/workflow.md +++ b/docs/subsystems/workflow.md @@ -62,7 +62,7 @@ Everything in `plugins/workflow/`: the graph rules, the run store, the matcher, 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 `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). +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. The source workflow's log gains exactly one system line naming the card and its destination (`Task re-matched · {task} → {to}`), written in the same mutation as the new claim and never by a write of its own, so a person watching the graph reads where the card went instead of seeing it vanish (#1668). 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). 9. The browser draws text only. Names and messages are React text children. The unit renderer asserts a hostile name is not markup. 10. `enabled: false` registers no endpoint, tool or listener and writes nothing. 11. A sidebar click opens the stage or a dialog and leaves the Workflows section in place. The stage and the dialogs portal onto `document.body`. When that container is missing they stay in the section tree; the click does not throw. @@ -121,7 +121,7 @@ conclusion about the pull request. ## Tests -`plugins/workflow/test/`: graph advance and join, graph edit, assign and persistence, the FIFO queue and concurrency cap, Bot-reply advance, a dispatch that fails without being marked handed, a bounded hand-off retry and a step deadline that fails the run and clears its timer (#1530), a per-workflow `stepTimeoutMinutes` that replaces the ten-minute constant and is refused outside 1..120 (#1580), disposing releases the Bots before the record drops their ids and a released runner name is reusable again, assign-to-bot versus assign-to-workflow, inputs and outputs along edges, a loop that breaks and an if/else branch, failure recovery (onFail edges with max attempts, exhaustion behavior, and compatibility with failOnce and loop edges), templates, `define` / `runNamed`, the matcher, ephemeral spawn and cleanup, a stopped run with its settled node map (#1424), the reply middleware, the client canvas, the settings page (including a condition and a loop), rename and delete (including factory-id rows such as `delivery`), the stage chat's scroll rule (a new visible line follows, a filtered node reply moves nothing), the assign dialog's preselected card and its failed and empty board states (#1457, #1458), a sidebar click that leaves the Workflows section in place, and the claim rules — a re-routed card moves with one claim and one route, the same target writes nothing, a trigger never invalidates, the held list is capped and omits a Bot claim, the held page answering `heldLimit` up to its 200 cap and the stage unfolding it behind its `wf-more` row (#2256), `rematch: true` drops one claim, and the panel's one re-match action goes through `ingest` (#1647), and the assigned brief reaching the step prompt while a task without one still gets its title, a parked card keeping its brief on disk, and an unknown `assign` field coming back in `ignored` (#1869). Run `npm test` in `plugins/workflow`. The host seam that applies `ownerBotId` is `core/test/task-route.test.js`, which also pins that a card written through `track_tasks` reaches the listener (#1462). The host omission of run Bots, and the grant of `stepTools` plus the hidden-approval answer, is `core/test/flybot.test.js`. Which connectors a run Bot may call is `core/test/connector-access.test.js`, and the chain from a node's `tools` through `spawn` to that guard — the built-in delivery nodes included — is `core/test/workflow-node-connectors.test.js`. +`plugins/workflow/test/`: graph advance and join, graph edit, assign and persistence, the FIFO queue and concurrency cap, Bot-reply advance, a dispatch that fails without being marked handed, a bounded hand-off retry and a step deadline that fails the run and clears its timer (#1530), a per-workflow `stepTimeoutMinutes` that replaces the ten-minute constant and is refused outside 1..120 (#1580), disposing releases the Bots before the record drops their ids and a released runner name is reusable again, assign-to-bot versus assign-to-workflow, inputs and outputs along edges, a loop that breaks and an if/else branch, failure recovery (onFail edges with max attempts, exhaustion behavior, and compatibility with failOnce and loop edges), templates, `define` / `runNamed`, the matcher, ephemeral spawn and cleanup, a stopped run with its settled node map (#1424), the reply middleware, the client canvas, the settings page (including a condition and a loop), rename and delete (including factory-id rows such as `delivery`), the stage chat's scroll rule (a new visible line follows, a filtered node reply moves nothing), the assign dialog's preselected card and its failed and empty board states (#1457, #1458), a sidebar click that leaves the Workflows section in place, and the claim rules — a re-routed card moves with one claim and one route, the same target writes nothing, a trigger never invalidates, the held list is capped and omits a Bot claim, the held page answering `heldLimit` up to its 200 cap and the stage unfolding it behind its `wf-more` row (#2256), `rematch: true` drops one claim, and the panel's one re-match action goes through `ingest` (#1647), and a re-match leaves one notice line in the source log while the target side and the untouched paths stay byte-for-byte the same (#1668), and the assigned brief reaching the step prompt while a task without one still gets its title, a parked card keeping its brief on disk, and an unknown `assign` field coming back in `ignored` (#1869). Run `npm test` in `plugins/workflow`. The host seam that applies `ownerBotId` is `core/test/task-route.test.js`, which also pins that a card written through `track_tasks` reaches the listener (#1462). The host omission of run Bots, and the grant of `stepTools` plus the hidden-approval answer, is `core/test/flybot.test.js`. Which connectors a run Bot may call is `core/test/connector-access.test.js`, and the chain from a node's `tools` through `spawn` to that guard — the built-in delivery nodes included — is `core/test/workflow-node-connectors.test.js`. ## Connector ceiling (#1544) diff --git a/plugins/workflow/src/service.js b/plugins/workflow/src/service.js index 37db29b6..fde440c1 100644 --- a/plugins/workflow/src/service.js +++ b/plugins/workflow/src/service.js @@ -1815,6 +1815,21 @@ export function createWorkflowService({ const key = `${decision.kind}:${decision.id}` const prev = state.routes[taskId] if (prev?.key === key) return { ok: true, matched: true, already: true, decision } + // #1668: a card leaving one workflow for another leaves a line in the source graph, so a + // person watching it reads where the card went instead of seeing it vanish. Written in the + // same mutation as the target's claim below, never by a write() of its own. + const reroutedFrom = held?.kind === 'workflow' && decision.kind === 'workflow' && held.id !== decision.id ? held.id : undefined + const noteReroute = () => { + if (reroutedFrom === undefined) return + const taskLabel = summarizeTask(task) + const to = workflow(decision.id)?.name ?? decision.id + push(reroutedFrom, [{ + kind: 'system', + key: 'Task re-matched · {task} → {to}', + params: { task: taskLabel, to }, + text: `任务已改投 · ${taskLabel} → ${to}`, + }], now()) + } if (decision.kind === 'bot') { rememberClaim(taskId, decision) write() @@ -1831,6 +1846,7 @@ export function createWorkflowService({ const finish = (value) => { if (value?.ok === false) return value rememberClaim(taskId, decision) + noteReroute() write() return { ...present(), matched: true, already: false, decision } } diff --git a/plugins/workflow/test/claim-reroute-log.test.js b/plugins/workflow/test/claim-reroute-log.test.js new file mode 100644 index 00000000..081e69f1 --- /dev/null +++ b/plugins/workflow/test/claim-reroute-log.test.js @@ -0,0 +1,168 @@ +// #1668: when a claim is re-matched into another workflow (#1647), only the target +// graph got a line: the source workflow's log did not change at all, so the card +// vanished from its held list with nothing to read. The source now gets exactly one +// system line naming the card and the destination, written in the same mutation as +// the target's claim (no extra write()), and the target side is untouched. +import assert from 'node:assert/strict' +import { mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' +import { createWorkflowService } from '../src/service.js' + +const CLOCK = 1_700_000_000_000 +const roster = [ + { id: 'main', name: 'Chief', role: 'lead', isMain: true, remote: false }, +] +const REROUTE_KEY = 'Task re-matched · {task} → {to}' + +function readStore(dir) { + return JSON.parse(readFileSync(join(dir, 'workflows.json'), 'utf8')) +} + +function serviceAt(dir, bots = () => roster) { + let seq = 0 + return createWorkflowService({ + dir, + persist: true, + now: () => CLOCK, + bots, + spawn: async () => `eph-${++seq}`, + release: async () => {}, + dispatch: async () => ({ ok: true }), + // Step deadlines are timers; they are not part of what these cases assert. + setTimer: () => 0, + clearTimer: () => {}, + }) +} + +function named(svc, name, extra = {}) { + const created = svc.create({ + name, + nodes: [{ id: 'only', name: 'Only', prompt: 'Do it.' }], + edges: [], + ...extra, + }) + assert.equal(created.ok, true, created.error) + return created.workflows.find(item => item.name === name) +} + +const messagesOf = (svc, name) => svc.state().workflows.find(item => item.name === name).messages + +// The store assigns ids from one global counter, so two runs that reach the same +// entries at different counter positions differ in `id` (and only there, with the +// clock frozen). Compare what a reader sees. +const readable = entries => entries.map(({ kind, key, params, text }) => ({ kind, key, params, text })) + +test('#1668: a re-matched card leaves exactly one line in the source graph', async () => { + const dir = mkdtempSync(join(tmpdir(), 'wf-reroute-log-')) + try { + const svc = serviceAt(dir) + const alpha = named(svc, 'Alpha') + const beta = named(svc, 'Beta') + await svc.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: alpha.id }) + + const beforeCount = messagesOf(svc, 'Alpha').length + const second = await svc.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: beta.id }) + assert.deepEqual({ matched: second.matched, already: second.already }, { matched: true, already: false }) + assert.deepEqual(second.decision, { kind: 'workflow', id: beta.id }) + + const after = messagesOf(svc, 'Alpha') + assert.equal(after.length, beforeCount + 1, 'the source graph gains exactly one line') + const line = after.at(-1) + assert.equal(line.kind, 'system') + assert.equal(line.key, REROUTE_KEY, 'the line is the re-match notice') + assert.match(String(line.params.task), /task-9/, 'the line names the card') + assert.equal(line.params.to, 'Beta', 'the line names the destination workflow') + assert.match(String(line.text), /task-9/, 'the Chinese fallback names the card too') + assert.match(String(line.text), /Beta/) + assert.deepEqual(readStore(dir).logs[alpha.id].at(-1), line, 'the line is on disk, not only in memory') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('#1668: the target side of a re-match is what a first assignment writes', async () => { + const controlDir = mkdtempSync(join(tmpdir(), 'wf-reroute-control-')) + const routedDir = mkdtempSync(join(tmpdir(), 'wf-reroute-target-')) + try { + const control = serviceAt(controlDir) + named(control, 'Alpha') + const controlBeta = named(control, 'Beta') + await control.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: controlBeta.id }) + + const routed = serviceAt(routedDir) + const routedAlpha = named(routed, 'Alpha') + const routedBeta = named(routed, 'Beta') + await routed.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: routedAlpha.id }) + await routed.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: routedBeta.id }) + + assert.deepEqual(readable(messagesOf(routed, 'Beta')), readable(messagesOf(control, 'Beta')), 'the destination log gains nothing of its own') + } finally { + rmSync(controlDir, { recursive: true, force: true }) + rmSync(routedDir, { recursive: true, force: true }) + } +}) + +test('#1668: a claim that simply matches appends nothing and writes nothing', async () => { + const dir = mkdtempSync(join(tmpdir(), 'wf-reroute-noop-')) + try { + const svc = serviceAt(dir) + const alpha = named(svc, 'Alpha') + await svc.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: alpha.id }) + const file = join(dir, 'workflows.json') + const before = readFileSync(file, 'utf8') + const mtime = statSync(file).mtimeMs + + const again = await svc.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: alpha.id }) + assert.equal(again.already, true) + assert.equal(messagesOf(svc, 'Alpha').filter(entry => entry.key === REROUTE_KEY).length, 0) + assert.equal(readFileSync(file, 'utf8'), before, 'the non-reroute path writes no bytes') + assert.equal(statSync(file).mtimeMs, mtime, 'and touches no mtime') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('#1668: an already:true re-match appends nothing to the source', async () => { + const dir = mkdtempSync(join(tmpdir(), 'wf-reroute-already-')) + try { + const svc = serviceAt(dir) + const alpha = named(svc, 'Alpha') + const beta = named(svc, 'Beta') + // Beta is already running the card, so re-matching it there is `already: true`. + await svc.assign({ workflowId: beta.id, taskId: 'task-9', title: 'Ship it' }) + await svc.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: alpha.id, rematch: true }) + const before = messagesOf(svc, 'Alpha').length + + const again = await svc.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: beta.id }) + assert.equal(again.already, true, 'the destination already runs the card') + assert.equal(messagesOf(svc, 'Alpha').length, before, 'already:true adds no source line') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('#1668: the MAX_MESSAGES cap still holds after the source line is added', async () => { + const dir = mkdtempSync(join(tmpdir(), 'wf-reroute-cap-')) + try { + const svc = serviceAt(dir) + const alpha = named(svc, 'Alpha') + const beta = named(svc, 'Beta') + const store = readStore(dir) + store.logs[alpha.id] = Array.from({ length: 200 }, (_, index) => ({ id: `old-${index + 1}`, at: CLOCK, kind: 'system', text: `old line ${index + 1}` })) + store.seq = 1000 + writeFileSync(join(dir, 'workflows.json'), JSON.stringify(store)) + + const reloaded = serviceAt(dir) + await reloaded.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: alpha.id }) + await reloaded.ingest({ id: 'task-9', status: 'open', title: 'Ship it', workflowId: beta.id }) + + const after = messagesOf(reloaded, 'Alpha') + assert.equal(after.length, 200, 'the source log stays at the cap') + assert.equal(after.at(-1).key, REROUTE_KEY, 'the newest line is the re-match notice') + assert.ok(after.every(entry => entry.id !== 'old-1'), 'the oldest line was dropped') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +})