fix(workflow-run): a blank hold is not a hold (#1337)

changelog.d/1337-workflow-blank-hold.fixed.md

@@ -0,0 +1 @@
+The workflow planner no longer reads a blank hold as a hold. A hold now names its holder or it is no hold at all: `''` — the shape a pull-request query that answered nothing arrives in — and `'   '` plan exactly like no hold, and a hold no name can be read from is treated as no hold instead of refusing every node of a job with either no sentence or nobody's name. A real holder is unchanged: every card-less node is still refused, with that name and the definition's own hold sentence. ([#1337](https://github.com/ThinkFlowLab/FlyBot/issues/1337))

core/src/shared/workflow-run.js

@@ -21,6 +21,10 @@
 //     `renderRefusal` with the fact it expects (`pr: hold`) and every refused node gets that one
 //     sentence. A definition with no such guard gets `refusal: null` and the holder's name rather than
 //     a sentence this layer made up: one sentence too few beats a new one.
+//   - **a hold names its holder, or it is no hold** (#1337). This is `heldBy`'s contract, stated on the
+//     function below, and it is what the host writer relies on: "the pull request query answered
+//     nothing" arrives as `''`, and a blank string must plan exactly like `hold: null` rather than
+//     refuse a whole job with no sentence to show.
 
 import { canonicalSpec, consumersOf, enterNode, normaliseWorkflow, nodeById, renderRefusal, topologicalOrder } from './workflow.js'
 import { freshness } from './artifact.js'
@@ -48,10 +52,15 @@ function heldSentence(def) {
   return null
 }
 
+// `heldBy` is the contract slice (b) reads: a hold **names** its holder or it is no hold at all. A
+// blank string is the shape "the pull request query answered nothing" arrives in, and a hold this
+// module cannot name (a non-string with no `by`, or a blank one) is normalised to "not held" on
+// purpose — refusing every node with `heldBy: null` would read exactly like a fact refusal (#1337).
 const heldBy = (hold) => {
   if (hold === null || hold === undefined || hold === false) return null
-  if (typeof hold === 'string') return hold.trim() === '' ? '' : hold.trim()
-  return typeof hold.by === 'string' ? hold.by.trim() : ''
+  if (typeof hold === 'string') return hold.trim() === '' ? null : hold.trim()
+  if (typeof hold !== 'object') return null
+  return typeof hold.by === 'string' && hold.by.trim() !== '' ? hold.by.trim() : null
 }
 
 // `(job, node)` is the identity of a node card (#1316 pin 4), so a card parented to another job is not

core/test/workflow-run.test.js

@@ -166,8 +166,40 @@ test("a held job opens nothing on any node, in the definition's own sentence", (
   assert.deepEqual(plain.open, [])
   assert.ok(plain.refused.every((entry) => entry.refusal === null && entry.heldBy === 'Birfy'))
   // A maintainer Bot's attempt is not a different caller here: the planner has no "who asks" input, so
-  // nothing about the request can loosen the hold.
-  assert.deepEqual(planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: { by: '' } }).open, [])
+  // nothing about the request can loosen a hold that names a holder.
+  assert.deepEqual(planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: 'Birfy' }).open, [])
+})
+
+// 7b. #1337: "the query answered nothing" reaches this layer as `''` from the host writer (slice (b)
+// reads the hold out of the pull request's labels/state), and `'   '` is the same absence written by a
+// form. Neither names a holder, so a blank string plans exactly like `hold: null` — node by node — and
+// nothing carries `heldBy: ''`; a job refused whole with no sentence would be a silent refusal.
+// Knife: restore the blank branch in `heldBy` (`hold.trim() === '' ? '' : hold.trim()`) → both blank
+// cases refuse every node and the deepEqual against the unheld plan goes red.
+test('a blank hold is not a hold: it plans exactly like no hold at all (#1337)', () => {
+  const unheld = planJob({ definition: repoDevWorkflow, job, facts: facts() })
+  assert.ok(unheld.open.length > 0)
+  for (const blank of ['', '   ', '\t\n']) {
+    const plan = planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: blank })
+    assert.deepEqual(plan, unheld, `hold: ${JSON.stringify(blank)} must plan like no hold`)
+    assert.ok(plan.refused.every((entry) => entry.heldBy !== ''))
+  }
+})
+
+// 7c. #1337: a hold this module cannot name — a non-string with no `by`, or a `by` that is blank or not
+// a string — is normalised to "not held" on purpose, and the module header states that contract. The
+// alternative, refusing every node with `heldBy: null`, reads exactly like a fact refusal: neither the
+// card nor the host writer could tell "held by an unnamed party" from "the query answered nothing".
+// Knife: return `''` for those shapes again → every case here refuses the whole job.
+test('a hold with no name is read as not held, deliberately (#1337)', () => {
+  const unheld = planJob({ definition: repoDevWorkflow, job, facts: facts() })
+  for (const shape of [{}, { by: '' }, { by: '   ' }, { by: 7 }, 42, true]) {
+    assert.deepEqual(planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: shape }), unheld, `hold: ${JSON.stringify(shape)}`)
+  }
+  // A name still holds, padded or not, and the holder is the trimmed name.
+  const padded = planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: '  Birfy  ' })
+  assert.deepEqual(padded.open, [])
+  assert.ok(padded.refused.every((entry) => entry.heldBy === 'Birfy'))
 })
 
 // 8. A fact nobody could read refuses, and says so: it is not a missing fact and not a current one

docs/subsystems/core-host.md

@@ -1,3 +1,3 @@
 ---
 subsystem: core-host
 owned_paths:
@@ -128,7 +128,7 @@
 stale one refuses with both revisions. A held job opens nothing on any node and is refused in the
 definition's own sentence — the `no-hold` guard, rendered with the fact it expects (`pr: hold`), which
 is the merge gate's wording and not a second copy of it; a definition that guards no held pull request
-gets the holder's name and no sentence, because this layer writes none. A job is done when its terminal
+gets the holder's name and no sentence, because this layer writes none. A hold names its holder or it is no hold at all (#1337): a blank string — the shape “the pull-request query answered nothing” arrives in — and a hold no name can be read from plan exactly like no hold, so a job is never refused whole with no sentence to show, while a named holder still refuses every card-less node in the definition's own sentence. A job is done when its terminal
 node's outputs are current, never because a card was moved or a merge was claimed. The host writer
 belongs to #1265 step 3, so that the parent is the real issue card; until it lands the planner is
 driven by fixture cards.

patches/ds-bot/files/src/shared/workflow-run.js

@@ -21,6 +21,10 @@
 //     `renderRefusal` with the fact it expects (`pr: hold`) and every refused node gets that one
 //     sentence. A definition with no such guard gets `refusal: null` and the holder's name rather than
 //     a sentence this layer made up: one sentence too few beats a new one.
+//   - **a hold names its holder, or it is no hold** (#1337). This is `heldBy`'s contract, stated on the
+//     function below, and it is what the host writer relies on: "the pull request query answered
+//     nothing" arrives as `''`, and a blank string must plan exactly like `hold: null` rather than
+//     refuse a whole job with no sentence to show.
 
 import { canonicalSpec, consumersOf, enterNode, normaliseWorkflow, nodeById, renderRefusal, topologicalOrder } from './workflow.js'
 import { freshness } from './artifact.js'
@@ -48,10 +52,15 @@ function heldSentence(def) {
   return null
 }
 
+// `heldBy` is the contract slice (b) reads: a hold **names** its holder or it is no hold at all. A
+// blank string is the shape "the pull request query answered nothing" arrives in, and a hold this
+// module cannot name (a non-string with no `by`, or a blank one) is normalised to "not held" on
+// purpose — refusing every node with `heldBy: null` would read exactly like a fact refusal (#1337).
 const heldBy = (hold) => {
   if (hold === null || hold === undefined || hold === false) return null
-  if (typeof hold === 'string') return hold.trim() === '' ? '' : hold.trim()
-  return typeof hold.by === 'string' ? hold.by.trim() : ''
+  if (typeof hold === 'string') return hold.trim() === '' ? null : hold.trim()
+  if (typeof hold !== 'object') return null
+  return typeof hold.by === 'string' && hold.by.trim() !== '' ? hold.by.trim() : null
 }
 
 // `(job, node)` is the identity of a node card (#1316 pin 4), so a card parented to another job is not

patches/ds-bot/files/test/workflow-run.test.js

@@ -166,8 +166,40 @@ test("a held job opens nothing on any node, in the definition's own sentence", (
   assert.deepEqual(plain.open, [])
   assert.ok(plain.refused.every((entry) => entry.refusal === null && entry.heldBy === 'Birfy'))
   // A maintainer Bot's attempt is not a different caller here: the planner has no "who asks" input, so
-  // nothing about the request can loosen the hold.
-  assert.deepEqual(planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: { by: '' } }).open, [])
+  // nothing about the request can loosen a hold that names a holder.
+  assert.deepEqual(planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: 'Birfy' }).open, [])
+})
+
+// 7b. #1337: "the query answered nothing" reaches this layer as `''` from the host writer (slice (b)
+// reads the hold out of the pull request's labels/state), and `'   '` is the same absence written by a
+// form. Neither names a holder, so a blank string plans exactly like `hold: null` — node by node — and
+// nothing carries `heldBy: ''`; a job refused whole with no sentence would be a silent refusal.
+// Knife: restore the blank branch in `heldBy` (`hold.trim() === '' ? '' : hold.trim()`) → both blank
+// cases refuse every node and the deepEqual against the unheld plan goes red.
+test('a blank hold is not a hold: it plans exactly like no hold at all (#1337)', () => {
+  const unheld = planJob({ definition: repoDevWorkflow, job, facts: facts() })
+  assert.ok(unheld.open.length > 0)
+  for (const blank of ['', '   ', '\t\n']) {
+    const plan = planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: blank })
+    assert.deepEqual(plan, unheld, `hold: ${JSON.stringify(blank)} must plan like no hold`)
+    assert.ok(plan.refused.every((entry) => entry.heldBy !== ''))
+  }
+})
+
+// 7c. #1337: a hold this module cannot name — a non-string with no `by`, or a `by` that is blank or not
+// a string — is normalised to "not held" on purpose, and the module header states that contract. The
+// alternative, refusing every node with `heldBy: null`, reads exactly like a fact refusal: neither the
+// card nor the host writer could tell "held by an unnamed party" from "the query answered nothing".
+// Knife: return `''` for those shapes again → every case here refuses the whole job.
+test('a hold with no name is read as not held, deliberately (#1337)', () => {
+  const unheld = planJob({ definition: repoDevWorkflow, job, facts: facts() })
+  for (const shape of [{}, { by: '' }, { by: '   ' }, { by: 7 }, 42, true]) {
+    assert.deepEqual(planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: shape }), unheld, `hold: ${JSON.stringify(shape)}`)
+  }
+  // A name still holds, padded or not, and the holder is the trimmed name.
+  const padded = planJob({ definition: repoDevWorkflow, job, facts: facts(), hold: '  Birfy  ' })
+  assert.deepEqual(padded.open, [])
+  assert.ok(padded.refused.every((entry) => entry.heldBy === 'Birfy'))
 })
 
 // 8. A fact nobody could read refuses, and says so: it is not a missing fact and not a current one