fix(workflow): keep stepTimeoutMinutes on a replaced graph

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)).

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.<pid>.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.<pid>.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

plugins/workflow/src/service.js

@@ -260,13 +260,13 @@
   }
 
   function clearRunTimers(runId) {
     for (const [key, entry] of [...timers]) {
       if (entry.runId === runId) clearTimerAt(key)
     }
     for (const key of [...warned]) {
       if (key.endsWith(`:${runId}`)) warned.delete(key)
     }
     for (const key of [...handOffTries.keys()]) {
       if (key.startsWith(`${runId}:`)) handOffTries.delete(key)
     }
   }
@@ -303,7 +303,7 @@
     // keep its deadline: without it the run holds its lane slot with nothing left
     // to free it until the owner presses Stop again (P-560).
     const stopRetry = run.status === 'running' && run.paused === true && run.releasePending === true
     for (const [key, entry] of [...timers]) {
       if (entry.runId !== run.id) continue
       if (run.status !== 'running' || (run.paused === true && !stopRetry) || !running.has(entry.nodeId)) clearTimerAt(key)
     }
@@ -591,7 +591,7 @@
       const missing = unmetRequired(item, id, run.produced, run.task)
       if (missing.length === 0) { started.push(id); continue }
       const preds = item.edges.filter(edge => edge.to === id && edge.loop !== true).map(edge => edge.from)
       const stuck = preds.length === 0 || preds.every(pred => status[pred] === 'done' || status[pred] === 'failed' || status[pred] === 'skipped')
       if (stuck) {
         status[id] = 'failed'
         blocked.push({ id, missing })
@@ -604,7 +604,7 @@
   function applyOp(item, run, op) {
     let working = run
     if (op.type === 'complete' && op.outputs && typeof op.outputs === 'object') {
       working = { ...run, produced: { ...(run.produced ?? {}), [op.nodeId]: op.outputs } }
     }
     let result
     if (op.type === 'start') result = { ...startFrontier(item, working.nodeStatus, working.fired), joined: false, layerSize: 0 }
@@ -796,7 +796,7 @@
         break
       }
       handOffTries.delete(`${current.id}:${nodeId}`)
       current = { ...current, handed: { ...current.handed, [nodeId]: true }, inputs: { ...(current.inputs ?? {}), [nodeId]: body.inputs } }
       storeRun(item.id, current)
     }
     syncTimers(item, current)
@@ -1814,6 +1814,13 @@
     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 @@
         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 @@
       // #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 @@
           }
           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 }),

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')
+})