diff --git a/changelog.d/2654.fixed.md b/changelog.d/2654.fixed.md new file mode 100644 index 000000000..4cfadcb6c --- /dev/null +++ b/changelog.d/2654.fixed.md @@ -0,0 +1 @@ +A `define_workflow` call that replaces a saved graph now stores its `stepTimeoutMinutes` instead of returning success while keeping the old value, and refuses a value outside 1..120 the way the create branch does; the `workflow/graph` save applies the same rule, while a save that omits the field still keeps what is stored ([#2654](https://github.com/ThinkFlowLab/FlyBot/issues/2654)). diff --git a/docs/subsystems/workflow.md b/docs/subsystems/workflow.md index 47e12a33f..7e3095385 100644 --- a/docs/subsystems/workflow.md +++ b/docs/subsystems/workflow.md @@ -50,8 +50,8 @@ Everything in `plugins/workflow/`: the graph rules, the run store, the matcher, ## Entry points - Endpoint `workflow/state` and `workflow/templates` are reads. -- `workflow/create`, `workflow/rename`, `workflow/delete`, `workflow/assign`, `workflow/concurrency`, `workflow/graph`, `workflow/advance`, `workflow/reset`, `workflow/ingest` write. `workflow/concurrency` sets that workflow's `maxConcurrent` (an integer from 1 to 8); the same control is in Settings → Workflow graphs. `workflow/step-timeout` sets that workflow's `stepTimeoutMinutes` (an integer from 1 to 120, or `null` to go back to the default), and `workflow/create` and `define_workflow` accept the same field, refused outside 1..120 rather than dropped (#1868). Every write lands through `workflows.json..tmp`, and that temp name is asked as a file first (temp-name sweep): a symbolic or hard link left there refuses the write, with the temp name in the answer, instead of putting the workflows outside their folder and renaming the link into place — `lstat` reports a hard link as a plain file although another name holds the same bytes, so the guard also requires `nlink === 1`, while a crashed host's own plain leftover stays replaceable. `workflow/rename` updates a workflow's name and optional description; read-only hosts are refused. `workflow/delete` removes a workflow from state, releases its ephemeral Bots if a run exists, and cleans logs/runs/routes and every task claim that named it, so the next `ingest` of that task matches again instead of asking `assign` for a workflow that no longer exists; a load still drops a workflow claim whose workflow is gone, so a store written before this fix heals (#1632). A delete whose write is refused puts the workflow and its lane, run, log, route, claim and brief records back before the refusal reaches the caller, so the page and `workflows.json` cannot disagree and the delete can be retried (#2623). `workflow/ingest` also takes `rematch: true`: that one task's claim and route are dropped before matching, the panel's per-task action (#1647). `workflow/assign` reads `taskId` (or `id`), `title`, `brief` (stored on the card, and the card's own brief wins on a later call that omits it), `callerBotId`, `projectId` and `botId`; any other key is named in the response's `ignored` array instead of being dropped in silence (#1869). -- Tool `run_workflow` starts a named run with a brief. Tool `define_workflow` writes the same graph the canvas edits, and takes the same `stepTimeoutMinutes` (1..120) so a lane created that way does not silently inherit the 45-minute default (#1868). +- `workflow/create`, `workflow/rename`, `workflow/delete`, `workflow/assign`, `workflow/concurrency`, `workflow/graph`, `workflow/advance`, `workflow/reset`, `workflow/ingest` write. `workflow/concurrency` sets that workflow's `maxConcurrent` (an integer from 1 to 8); the same control is in Settings → Workflow graphs. `workflow/step-timeout` sets that workflow's `stepTimeoutMinutes` (an integer from 1 to 120, or `null` to go back to the default), and `workflow/create`, `workflow/graph` and either branch of `define_workflow` accept the same field, refused outside 1..120 rather than dropped and kept as it is when the call omits it (#1868, #2654). Every write lands through `workflows.json..tmp`, and that temp name is asked as a file first (temp-name sweep): a symbolic or hard link left there refuses the write, with the temp name in the answer, instead of putting the workflows outside their folder and renaming the link into place — `lstat` reports a hard link as a plain file although another name holds the same bytes, so the guard also requires `nlink === 1`, while a crashed host's own plain leftover stays replaceable. `workflow/rename` updates a workflow's name and optional description; read-only hosts are refused. `workflow/delete` removes a workflow from state, releases its ephemeral Bots if a run exists, and cleans logs/runs/routes and every task claim that named it, so the next `ingest` of that task matches again instead of asking `assign` for a workflow that no longer exists; a load still drops a workflow claim whose workflow is gone, so a store written before this fix heals (#1632). A delete whose write is refused puts the workflow and its lane, run, log, route, claim and brief records back before the refusal reaches the caller, so the page and `workflows.json` cannot disagree and the delete can be retried (#2623). `workflow/ingest` also takes `rematch: true`: that one task's claim and route are dropped before matching, the panel's per-task action (#1647). `workflow/assign` reads `taskId` (or `id`), `title`, `brief` (stored on the card, and the card's own brief wins on a later call that omits it), `callerBotId`, `projectId` and `botId`; any other key is named in the response's `ignored` array instead of being dropped in silence (#1869). +- Tool `run_workflow` starts a named run with a brief. Tool `define_workflow` writes the same graph the canvas edits, and takes the same `stepTimeoutMinutes` (1..120) whether it creates the lane or replaces a saved graph, so a lane created that way does not silently inherit the 45-minute default and a replaced graph does not keep an old value in silence (#1868, #2654). - `flybot.tasks.onWrite` calls `ingest`. A new Bot match returns `{ ownerBotId }`. A Workflow match starts the run inside the plugin and leaves the card's owner to the host. A host that offers no `tasks.onWrite` warns once and reports `tasksWired: false` in `workflow/state`, and the Workflows panel says that a board card cannot start a run there (#1462). ## Invariants diff --git a/plugins/workflow/src/service.js b/plugins/workflow/src/service.js index 1fa99c5d8..7d5156665 100644 --- a/plugins/workflow/src/service.js +++ b/plugins/workflow/src/service.js @@ -1814,6 +1814,13 @@ export function createWorkflowService({ saveGraph(body = {}) { const endedRuns = [] const saved = mutate(String(body.workflowId ?? ''), (item) => { + // #2654: a replaced graph has no door but this save — `define_workflow`'s replace branch + // and the `workflow/graph` endpoint both come through it — so the timeout is settled here + // for both. A bad value throws before anything is written; omitted, `null` and `''` keep + // what is stored. + const stepTimeoutMinutes = body.stepTimeoutMinutes === undefined || body.stepTimeoutMinutes === null || body.stepTimeoutMinutes === '' + ? undefined + : stepTimeoutValue(body.stepTimeoutMinutes) // #1544: a save from a Bot records it as the graph's author; a save with no Bot behind it (the // canvas, the API, a template copy) is the owner's own edit and lifts the ceiling again. const authoredBy = cleanLine(body.callerBotId ?? body.authoredBy, 80) || undefined @@ -1867,6 +1874,7 @@ export function createWorkflowService({ item.nodes = next.nodes item.edges = next.edges item.layers = next.layers + if (stepTimeoutMinutes !== undefined) item.stepTimeoutMinutes = stepTimeoutMinutes // Applied only once the graph validated and the runs settled, so a save that throws leaves // neither the record nor the lane half-changed (#1856). if (projectId === null) delete item.projectId @@ -1909,6 +1917,12 @@ export function createWorkflowService({ // #1544: the graph remembers the Bot that wrote it, so a run nobody's tool call started is still // capped by the switches of the Bot that chose the node's `tools`. const authoredBy = cleanLine(body.callerBotId, 80) || undefined + // #1868/#2654: both branches take the field, and both refuse a bad one — `create()` and + // `saveGraph()` validate it through `stepTimeoutValue()`. Omitting it, or passing `null` + // or `''`, keeps the value that is already stored. + const stepTimeoutMinutes = body.stepTimeoutMinutes === undefined || body.stepTimeoutMinutes === null || body.stepTimeoutMinutes === '' + ? undefined + : body.stepTimeoutMinutes const existing = body.workflow ? findWorkflow(body.workflow) : undefined if (existing) { const finish = () => { @@ -1919,18 +1933,12 @@ export function createWorkflowService({ } return present() } - const saved = this.saveGraph({ workflowId: existing.id, nodes: body.nodes, edges: body.edges ?? [], ...(authoredBy ? { authoredBy } : {}) }) + const saved = this.saveGraph({ workflowId: existing.id, nodes: body.nodes, edges: body.edges ?? [], ...(authoredBy ? { authoredBy } : {}), ...(stepTimeoutMinutes === undefined ? {} : { stepTimeoutMinutes }) }) // saveGraph returns a promise when the edit had to end a live run. if (typeof saved?.then === 'function') return saved.then(value => (value?.ok === false ? value : finish())) if (saved.ok === false) return saved return finish() } - // #1868: a lane created through `define_workflow` used to inherit the 45-minute - // default with no way to say otherwise. The field goes to `create()`, which - // validates it through `stepTimeoutValue()`: a bad value is refused, not dropped. - const stepTimeoutMinutes = body.stepTimeoutMinutes === undefined || body.stepTimeoutMinutes === null || body.stepTimeoutMinutes === '' - ? undefined - : body.stepTimeoutMinutes return this.create({ name: body.name, nodes: body.nodes, edges: body.edges ?? [], description: body.description, ...(stepTimeoutMinutes === undefined ? {} : { stepTimeoutMinutes }), diff --git a/plugins/workflow/test/save-graph-step-timeout.test.js b/plugins/workflow/test/save-graph-step-timeout.test.js new file mode 100644 index 000000000..2e5d6fde6 --- /dev/null +++ b/plugins/workflow/test/save-graph-step-timeout.test.js @@ -0,0 +1,58 @@ +// #2654: `define_workflow` promises `stepTimeoutMinutes` on every call, but only its create +// branch stored it. The replace branch and the `workflow/graph` endpoint share `saveGraph()`, +// so the field is decided there: a valid value is stored, an out-of-range one refuses the whole +// save without touching the record, and an omitted one keeps what is stored. +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { createWorkflowService } from '../src/service.js' + +const NODES = [{ id: 'only', name: 'Only', prompt: 'Do the thing' }] + +function harness() { + return createWorkflowService({ persist: false, now: () => 1_700_000_000_000, bots: () => [], tasksWired: false }) +} + +const stored = (svc, workflowId) => svc.state().workflows.find(workflow => workflow.id === workflowId) + +function createProbe(svc) { + const state = svc.define({ name: 'TimeoutProbe', nodes: NODES, edges: [], stepTimeoutMinutes: 10 }) + const item = state.workflows.find(workflow => workflow.name === 'TimeoutProbe') + assert.equal(item.stepTimeoutMinutes, 10, 'the create branch still stores the field') + return item.id +} + +test('#2654: define() replace stores a valid stepTimeoutMinutes and refuses an invalid one', () => { + const svc = harness() + const workflowId = createProbe(svc) + + const replaced = svc.define({ workflow: workflowId, name: 'TimeoutProbe', nodes: NODES, edges: [], stepTimeoutMinutes: 5 }) + assert.equal(replaced.ok, true, JSON.stringify(replaced)) + assert.equal(stored(svc, workflowId).stepTimeoutMinutes, 5, 'the replace branch stored 5') + + const bad = svc.define({ workflow: workflowId, name: 'TimeoutProbe', nodes: NODES, edges: [], stepTimeoutMinutes: 999 }) + assert.equal(bad.ok, false, JSON.stringify(bad)) + assert.match(String(bad.error), /stepTimeoutMinutes is an integer from 1 to 120/) + assert.equal(stored(svc, workflowId).stepTimeoutMinutes, 5, 'a refused replace wrote nothing') + + const omitted = svc.define({ workflow: workflowId, name: 'TimeoutProbe', nodes: NODES, edges: [] }) + assert.equal(omitted.ok, true, JSON.stringify(omitted)) + assert.equal(stored(svc, workflowId).stepTimeoutMinutes, 5, 'an omitted field keeps what is stored') +}) + +test('#2654: workflow/graph (saveGraph) decides stepTimeoutMinutes the same way', () => { + const svc = harness() + const workflowId = createProbe(svc) + + const replaced = svc.saveGraph({ workflowId, nodes: NODES, edges: [], stepTimeoutMinutes: 5 }) + assert.equal(replaced.ok, true, JSON.stringify(replaced)) + assert.equal(stored(svc, workflowId).stepTimeoutMinutes, 5, 'the graph endpoint stored 5') + + const bad = svc.saveGraph({ workflowId, nodes: NODES, edges: [], stepTimeoutMinutes: 999 }) + assert.equal(bad.ok, false, JSON.stringify(bad)) + assert.match(String(bad.error), /stepTimeoutMinutes is an integer from 1 to 120/) + assert.equal(stored(svc, workflowId).stepTimeoutMinutes, 5, 'a refused save wrote nothing') + + const omitted = svc.saveGraph({ workflowId, nodes: NODES, edges: [] }) + assert.equal(omitted.ok, true, JSON.stringify(omitted)) + assert.equal(stored(svc, workflowId).stepTimeoutMinutes, 5, 'an omitted field keeps what is stored') +})