fix(workflow): look a fact up by its parsed spec, and refuse a check with no conclusion (#1298)

core/src/host/workflow-facts.js

@@ -48,11 +48,16 @@ export function githubLabels({ token, head, labels } = {}) {
 }
 
 // `check:<name>` at the sha the check ran on, valued with its conclusion. A check that never reported
-// has no sha to be at, so it is unreadable rather than a check at the tip that failed.
+// has no sha to be at, and one that ran but has not concluded has no answer either — `conclusion: null`
+// is GitHub's word for "still running", so both are unreadable rather than a check at the tip that
+// failed (#1298).
 export function githubCheckAtSha({ token, name, sha, conclusion } = {}) {
   if (!hasToken(token)) return unreadable('github.checkAtSha', 'no GITHUB_TOKEN')
   if (!name || !sha) return unreadable('github.checkAtSha', 'the check run was not read')
-  return read('github.checkAtSha', artifact(`check:${name}`, sha, { value: conclusion ?? null }))
+  if (typeof conclusion !== 'string' || conclusion.trim() === '') {
+    return unreadable('github.checkAtSha', 'the check run reported no conclusion')
+  }
+  return read('github.checkAtSha', artifact(`check:${name}`, sha, { value: conclusion }))
 }
 
 // `verdict` at the tip the conclusion names, valued PASS / INC / FAIL / REQUEST_CHANGES and carrying

core/src/shared/workflow.js

@@ -368,15 +368,31 @@ export function inputStates(raw, nodeId, facts = {}) {
     if (!have) return { spec, present: false, revision: null, stale: false, now: null, reason: 'missing', unreadable: false }
     // A card draws `stale` as a refusal, so an input `enterNode` refuses must never read
     // `stale: false` here: a source nobody could read is stale until it is read.
-    const state = freshness(have, current[spec])
+    const state = freshness(have, currentAt(current, spec))
     return { spec, present: true, revision: have.revision, stale: !state.fresh, now: state.now, reason: state.reason, unreadable: state.reason === 'unreadable' }
   })
 }
 
+// A fact is stored under the spelling `artifact()` gives it (`formatArtifactSpec`), and a definition
+// may spell the same fact with a space (#1298). Every lookup goes through the parsed spec first and
+// keeps the spelling as written as a second try, for a caller that keyed a map by hand.
+function factKey(spec) {
+  return canonicalSpec(spec) ?? spec
+}
+
 function artifactAt(present, spec) {
   if (!spec) return null
-  if (Array.isArray(present)) return present.find((item) => item?.spec === spec) ?? null
-  return present?.[spec] ?? null
+  const key = factKey(spec)
+  if (Array.isArray(present)) {
+    return present.find((item) => item?.spec === key) ?? present.find((item) => item?.spec === spec) ?? null
+  }
+  return present?.[key] ?? present?.[spec] ?? null
+}
+
+function currentAt(current, spec) {
+  if (!spec) return null
+  const key = factKey(spec)
+  return current?.[key] ?? current?.[spec] ?? null
 }
 
 function guardRefusals(node, facts) {
@@ -396,7 +412,7 @@ function guardRefusals(node, facts) {
       continue
     }
     const have = artifactAt(present, spec)
-    const state = freshness(have, current[spec])
+    const state = freshness(have, currentAt(current, spec))
     if (!have) {
       // A guard's own sentence says what the fact said, and a fact that is not there said nothing:
       // the node's wording answers this one, or the sentence would name two revisions that do not exist.
@@ -435,7 +451,7 @@ export function enterNode(raw, nodeId, facts = {}) {
     }
     // A revision nobody could read is asked as `unreadable`, never skipped: a key missing from
     // `current` is not evidence that the fact is current, and `null` is the same answer.
-    const state = freshness(have, current[spec])
+    const state = freshness(have, currentAt(current, spec))
     if (!state.fresh) stale.push({ spec, have: state.have, now: state.now, reason: state.reason })
   }
   if (missing.length > 0) {

core/test/workflow-repo-dev.test.js

@@ -223,3 +223,21 @@ test('a fact nobody could read is unreadable, and no fact is guessed (acceptance
 function githubMergeableNothing() {
   return workflowFact('github.mergeable', {})
 }
+
+test('a check run with no conclusion is unreadable, not a fact whose value is null (#1298)', () => {
+  const quiet = githubCheckAtSha({ token: 'x', name: 'tests-ok', sha: 'abaf938b0c1d' })
+  assert.equal(quiet.ok, false)
+  assert.equal(quiet.reason, 'unreadable')
+  assert.equal(quiet.input, null)
+  assert.match(quiet.why, /conclusion/)
+  // The same answer as a run with no sha and one with no name: none of them was read.
+  assert.equal(githubCheckAtSha({ token: 'x', name: 'tests-ok' }).reason, 'unreadable')
+  assert.equal(githubCheckAtSha({ token: 'x', sha: 'abaf938b0c1d' }).reason, 'unreadable')
+  // `conclusion: null` is GitHub saying the run has not concluded yet: still no reading.
+  assert.equal(githubCheckAtSha({ token: 'x', name: 'tests-ok', sha: 'abaf938b0c1d', conclusion: null }).ok, false)
+  // A run that did conclude is the fact, whatever it concluded.
+  const failed = githubCheckAtSha({ token: 'x', name: 'tests-ok', sha: 'abaf938b0c1d', conclusion: 'failure' })
+  assert.equal(failed.ok, true)
+  assert.equal(failed.input.value, 'failure')
+  assert.equal(failed.input.revision, 'abaf938b0c1d')
+})

core/test/workflow.test.js

@@ -14,6 +14,7 @@ import {
   sameRevision,
 } from '../src/shared/artifact.js'
 import {
+  dependencyEdges,
   enterNode,
   holderOf,
   inputStates,
@@ -342,6 +343,34 @@ test('a stale input refuses with both revisions, so a card reads without asking
   ])
 })
 
+test('a fact is looked up by its parsed spec, not by the spelling the definition used (#1298)', () => {
+  const def = {
+    inputs: ['text:topic'],
+    deliverables: ['issue'],
+    nodes: [
+      { id: 'draft', name: 'Draft', role: 'Dev', inputs: ['text:topic'], outputs: ['text:draft'] },
+      { id: 'edit', name: 'Edit', role: 'Editor', inputs: ['text: draft'], outputs: ['issue'] },
+    ],
+    edges: [],
+  }
+  // The definition is valid and the dependency is there: everything that decides whether it can run
+  // keys on the parsed spec.
+  const checked = validateWorkflow(def)
+  assert.equal(checked.ok, true, JSON.stringify(checked.problems))
+  assert.deepEqual(dependencyEdges(def), [{ from: 'draft', to: 'edit' }])
+  const present = { 'text:draft': artifact('text:draft', 'abc1234') }
+  const current = { 'text:draft': 'abc1234' }
+  const ran = enterNode(def, 'edit', { present, current })
+  assert.equal(ran.ok, true, JSON.stringify(ran))
+  assert.equal(ran.refusal, null)
+  // The card still shows the spec as the node wrote it, and reads it current.
+  const states = inputStates(def, 'edit', { present, current })
+  assert.deepEqual(states.map((state) => [state.spec, state.present, state.reason]), [['text: draft', true, 'current']])
+  // A map keyed by the spelling as written is still found, and staleness reads through the same lookup.
+  assert.equal(enterNode(def, 'edit', { present: { 'text: draft': present['text:draft'] }, current: { 'text: draft': 'abc1234' } }).ok, true)
+  assert.equal(enterNode(def, 'edit', { present, current: { 'text:draft': 'def5678' } }).stale.length, 1)
+})
+
 test('a guard is a predicate over a fact, and its refusal says what the fact said', () => {
   const def = {
     inputs: ['pr'],

docs/subsystems/core-host.md

@@ -53,7 +53,7 @@ The server side of the patched `ds-bot` core (`core/`): the part of [ds-bot-core
 | `core/src/host/tool-schema.js` | The tool definitions the model sees: its `defineTool` wraps DSH's and completes the compiled schema with `explicitObjectShape`, which fills `properties: {}` and `required: []` on every object node (recursing through `properties`, `items`, `oneOf`/`anyOf`/`allOf`, `$defs`, …). A strict OpenAI-compatible gateway reads an absent member as null and rejects the whole request (`null is not of type "array"`), so one argument-less tool would break every turn; argument validation keeps using DSH's schema unchanged. Every host module registers tools through this wrapper. Tests: `core/test/tool-schema.test.js`. |
 | `core/src/shared/artifact.js` | The typed facts a workflow node takes in and hands out (#1272): `parseArtifactSpec` reads `check:tests-ok` / `issue.ready` / `text:topic` into `{ kind, name }` and refuses a spelling the vocabulary does not have; `artifact(spec, revision, { value })` is one instance, and `REVISION_RULES` says what the revision of each kind is (a verdict its SHA, a check the SHA it ran on, a PR its number and `headRefOid`). `freshness(input, currentRevision)` answers present-and-current separately from present, and a fact nobody could read is `unreadable`, never a match. Tests: `core/test/workflow.test.js`. |
 | `core/src/shared/board-card.js` | Card identity for the one board (#1265 step 1): `githubCardKey(repo, number)` is `owner/name#number` and `taskCardKey(id)` is `task:<id>`, the two spellings the client's `BoardCard` puts on every card. The GitHub key is not keyed by kind — an issue and the pull request that closes it are one card — which is the identity step 3 hangs a FlyBot task record under; a row with no repository falls back to `github:<kind>:<number>` rather than imply a cross-repository identity. `githubCard(row, repo)` / `taskCard(task)` build the model, `sameCard(a, b)` is the one comparison. Tests: `core/test/board-card.test.js`. |
-| `core/src/shared/workflow.js` | The workflow engine's pure half (#1272): a workflow is data — `nodes` (each with a required `role` and typed `inputs` / `outputs`), `edges` (`<node>.<artifact> -> <node>.<artifact>`) and declared `loops` — and every function below is a function of that file. `validateWorkflow` refuses each way a definition cannot run, with its own sentence (an input nothing produces, an output nothing consumes and that is not a `deliverable`, two producers of one fact, an undeclared cycle, a node with no `role`, a role not on the team, an unknown artifact kind, a misaligned edge, a guard whose `is` is not one of `present` / `fresh` / `equals`, a `column` the board does not have); `workflowColumns` is the board's columns in dependency order, so the node ids are the state and no built-in list exists, and a node carrying `column: false` draws no column at all while a definition that states none keeps the node id as its column (#1265); `layoutWorkflow` is the flowchart (one box per node carrying its role and rails, one edge per `edges[]` row, ranks left to right); `enterNode` / `inputStates` / `renderRefusal` answer whether a card may start and, when it may not, say which input is missing or stale with both revisions, in the node's own `refusal` wording when it has one and in the guard's own wording when that guard carries one; `holderOf` is the node's role, which is the card's assignee. #1291's four debts are closed here: one word for one thing (`role`, never `bot`), an unknown guard `is` refuses instead of being read as `present`, `producersOf` / `consumersOf` key on the parsed spec so `text: draft` and `text:draft` are one fact, and `resolvedEdges` reports the artifact it carries once. Tests: `core/test/workflow.test.js`. |
+| `core/src/shared/workflow.js` | The workflow engine's pure half (#1272): a workflow is data — `nodes` (each with a required `role` and typed `inputs` / `outputs`), `edges` (`<node>.<artifact> -> <node>.<artifact>`) and declared `loops` — and every function below is a function of that file. `validateWorkflow` refuses each way a definition cannot run, with its own sentence (an input nothing produces, an output nothing consumes and that is not a `deliverable`, two producers of one fact, an undeclared cycle, a node with no `role`, a role not on the team, an unknown artifact kind, a misaligned edge, a guard whose `is` is not one of `present` / `fresh` / `equals`, a `column` the board does not have); `workflowColumns` is the board's columns in dependency order, so the node ids are the state and no built-in list exists, and a node carrying `column: false` draws no column at all while a definition that states none keeps the node id as its column (#1265); `layoutWorkflow` is the flowchart (one box per node carrying its role and rails, one edge per `edges[]` row, ranks left to right); `enterNode` / `inputStates` / `renderRefusal` answer whether a card may start and, when it may not, say which input is missing or stale with both revisions, in the node's own `refusal` wording when it has one and in the guard's own wording when that guard carries one; both read a fact by the same parsed key the rest of the engine uses, so the spelling on the card and the spelling on the producer cannot disagree (#1298); `holderOf` is the node's role, which is the card's assignee. #1291's four debts are closed here: one word for one thing (`role`, never `bot`), an unknown guard `is` refuses instead of being read as `present`, `producersOf` / `consumersOf` key on the parsed spec so `text: draft` and `text:draft` are one fact, and `resolvedEdges` reports the artifact it carries once. Tests: `core/test/workflow.test.js`. |
 | `core/src/shared/workflow-blank.js` | The `blank` preset (#1272): three nodes (`draft` / `review` / `publish`, roles `Writer` / `Editor` / `Publisher`), no guards, artifacts `text:topic -> text:draft -> text:edited -> link:published`. Data, not logic — the same JSON is what a workspace `workflows/blank.json` holds — and the proof that the engine special-cases no node id. Tests: `core/test/workflow.test.js`. |
 | `core/src/shared/workflow-repo-dev.js` | The `repo-dev` preset (#1272 step 2): one repository change as data — `triage` → `build` → `verify` → `review` → `gate` → `merge` → `release`, roles `Triage` / `Dev` / `SelfTest` / `Reviewer` / `system` / `Release`, and the artifacts `issue` → `issue.ready` → `pr` → `check:tests-ok` → `verdict` → `risk:tier` → `merge-commit` → `release`. Two roles say what they are rather than who: `verify` is `SelfTest` **and** CI, the run being the `check:tests-ok` fact it hands out, and `gate` is held by the Reviewer that judged the pull request, read from the verdict's own `Written-by` through a guard. The gate's guards refuse in the merge gate's own words against `.github/scripts/fixtures/workflow-cases.json`, which no file imports and both sides read. `column` per node: `not-started` / `in-progress` / `done`, with `review` on `false` — a review is the pull request's state, not a third lane (#1265). Tests: `core/test/workflow-repo-dev.test.js`. |
 | `core/src/host/workflow-facts.js` | The fact providers (#1272 step 2, acceptance 10): one function per fact a guard asks about — `github.labels`, `github.checkAtSha`, `github.verdictAtTip`, `github.mergeable`, `github.prBodyTip`, `task.progress`, `manual.tick` — each taking the answer a reader already got and returning one artifact (spec, revision, value), never fetching. No `GITHUB_TOKEN`, no answer, or an answer missing the field is `{ ok: false, reason: 'unreadable', why }`, never a guess: a fact nobody could read refuses the node it feeds instead of passing it. `workflowFact(name, args)` is the one spelling for a caller that has the name as data, and an unknown name is unreadable with the name in the reason. Tests: `core/test/workflow-repo-dev.test.js`. |
@@ -83,7 +83,9 @@ Two rules keep that from drifting:
   check refuses with its own sentence instead of being read as `present`, because a guard that can
   never refuse is worse than a guard that is missing: it looks like a check. A fact is keyed by its
   parsed spec, so `text: draft` and `text:draft` are one fact (`canonicalSpec`), in the producers,
-  in the consumers and in the declared input and deliverable lists alike.
+  in the consumers and in the declared input and deliverable lists alike — and in `enterNode` /
+  `inputStates` too, so a definition that spells an input `text: draft` reads the `text:draft`
+  artifact it has instead of refusing a fact that is there (#1298).
 - **The board's columns are the definition's, and a node may draw none.** `workflowColumns` returns
   the nodes in dependency order with the `column` each card sits in (`not-started`,
   `in-progress`, `done`); a node carrying `column: false` stays in the definition and out of the
@@ -98,7 +100,8 @@ request, a moved head and a conclusion that is not a PASS are written once in
 the `repo-dev` preset's gate guards are both asserted against it, byte for byte, and neither file
 imports the other (#1272 acceptance 9). The facts those guards read come from
 `core/src/host/workflow-facts.js`, one function per fact, where a fact nobody could read is
-`unreadable` and never a guess (acceptance 10).
+`unreadable` and never a guess (acceptance 10) — a check run that ran but reported no conclusion
+included, since "still running" is not a reading (#1298).
 
 ### Default harness and usage ledger (#903)
 

patches/ds-bot/files/src/host/workflow-facts.js

@@ -48,11 +48,16 @@ export function githubLabels({ token, head, labels } = {}) {
 }
 
 // `check:<name>` at the sha the check ran on, valued with its conclusion. A check that never reported
-// has no sha to be at, so it is unreadable rather than a check at the tip that failed.
+// has no sha to be at, and one that ran but has not concluded has no answer either — `conclusion: null`
+// is GitHub's word for "still running", so both are unreadable rather than a check at the tip that
+// failed (#1298).
 export function githubCheckAtSha({ token, name, sha, conclusion } = {}) {
   if (!hasToken(token)) return unreadable('github.checkAtSha', 'no GITHUB_TOKEN')
   if (!name || !sha) return unreadable('github.checkAtSha', 'the check run was not read')
-  return read('github.checkAtSha', artifact(`check:${name}`, sha, { value: conclusion ?? null }))
+  if (typeof conclusion !== 'string' || conclusion.trim() === '') {
+    return unreadable('github.checkAtSha', 'the check run reported no conclusion')
+  }
+  return read('github.checkAtSha', artifact(`check:${name}`, sha, { value: conclusion }))
 }
 
 // `verdict` at the tip the conclusion names, valued PASS / INC / FAIL / REQUEST_CHANGES and carrying