From 200021a1e4687f06beaf962bf7b7e6d2de79e4e8 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Mon, 26 Oct 2026 20:08:50 +0800 Subject: [PATCH] feat(workflow): the repo-dev preset and the fact providers (#1272 step 2) Closes the four debts #1291 named: a guard whose `is` nobody implements refuses instead of being read as `present`, one word for one thing (`role`, never `bot`), facts keyed by the stem. --- .github/scripts/fixtures/workflow-cases.json | 70 ++++++ .github/scripts/merge-gate.test.mjs | 48 ++++ core/src/host/workflow-facts.js | 116 +++++++++ core/src/shared/workflow-blank.js | 10 +- core/src/shared/workflow-repo-dev.js | 152 ++++++++++++ core/src/shared/workflow.js | 148 +++++++++--- core/test/workflow-repo-dev.test.js | 225 ++++++++++++++++++ core/test/workflow.test.js | 92 +++---- docs/subsystems/core-host.md | 36 ++- .../ds-bot/files/src/host/workflow-facts.js | 116 +++++++++ .../ds-bot/files/src/shared/workflow-blank.js | 10 +- .../files/src/shared/workflow-repo-dev.js | 152 ++++++++++++ patches/ds-bot/files/src/shared/workflow.js | 148 +++++++++--- .../files/test/workflow-repo-dev.test.js | 225 ++++++++++++++++++ patches/ds-bot/files/test/workflow.test.js | 92 +++---- 15 files changed, 1465 insertions(+), 175 deletions(-) create mode 100644 .github/scripts/fixtures/workflow-cases.json create mode 100644 core/src/host/workflow-facts.js create mode 100644 core/src/shared/workflow-repo-dev.js create mode 100644 core/test/workflow-repo-dev.test.js create mode 100644 patches/ds-bot/files/src/host/workflow-facts.js create mode 100644 patches/ds-bot/files/src/shared/workflow-repo-dev.js create mode 100644 patches/ds-bot/files/test/workflow-repo-dev.test.js diff --git a/.github/scripts/fixtures/workflow-cases.json b/.github/scripts/fixtures/workflow-cases.json new file mode 100644 index 000000000..6e574bacd --- /dev/null +++ b/.github/scripts/fixtures/workflow-cases.json @@ -0,0 +1,70 @@ +{ + "why": "The sentences the merge gate refuses with, written once. `.github/scripts/merge-gate.mjs`'s `judge` and the `repo-dev` preset's gate guards are judged against the same entries, and neither file imports the other: the gate test reads `gate` and the preset test reads `preset`, both comparing against `sentence` byte for byte (#1272 acceptance 9). Change a sentence in one place and the other side's test goes red.", + "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "oldHead": "175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90", + "cases": [ + { + "id": "hold", + "where": "merge-gate.mjs `judge`: the `hold` label refuses whatever the review says", + "sentence": "the pull request carries the `hold` label", + "gate": { + "labels": ["hold"], + "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "expectedHead": null, + "reports": [{ "verdict": "PASS", "tip": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "writtenBy": "Reviewer" }] + }, + "preset": { + "node": "gate", + "guard": "no-hold", + "present": [ + { "spec": "pr", "revision": "1274@abaf938b0c1d", "value": "hold" }, + { "spec": "verdict", "revision": "abaf938b0c1d", "value": "PASS", "by": "Reviewer" }, + { "spec": "check:tests-ok", "revision": "abaf938b0c1d" } + ], + "current": { "pr": "1274@abaf938b0c1d", "verdict": "abaf938b0c1d", "check:tests-ok": "abaf938b0c1d" } + } + }, + { + "id": "head-moved", + "where": "merge-gate.mjs `judge`: the head is not the one the caller asked about", + "sentence": "the pull request head is abaf938b0c1d, not the expected 175328d0c1b2", + "gate": { + "labels": [], + "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "expectedHead": "175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90", + "reports": [{ "verdict": "PASS", "tip": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "writtenBy": "Reviewer" }] + }, + "preset": { + "node": "gate", + "guard": "verdict-at-tip", + "present": [ + { "spec": "pr", "revision": "1274@abaf938b0c1d", "value": "none" }, + { "spec": "verdict", "revision": "175328d0c1b2", "value": "PASS", "by": "Reviewer" }, + { "spec": "check:tests-ok", "revision": "abaf938b0c1d" } + ], + "current": { "pr": "1274@abaf938b0c1d", "verdict": "abaf938b0c1d", "check:tests-ok": "abaf938b0c1d" } + } + }, + { + "id": "verdict-not-pass", + "where": "merge-gate.mjs `judge`: the newest conclusion at the tip is not a PASS", + "sentence": "reviewer-agent says INC at this tip", + "gate": { + "labels": [], + "head": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "expectedHead": null, + "reports": [{ "verdict": "INC", "tip": "abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "writtenBy": "Reviewer" }] + }, + "preset": { + "node": "gate", + "guard": "verdict-says-pass", + "present": [ + { "spec": "pr", "revision": "1274@abaf938b0c1d", "value": "none" }, + { "spec": "verdict", "revision": "abaf938b0c1d", "value": "INC", "by": "reviewer-agent" }, + { "spec": "check:tests-ok", "revision": "abaf938b0c1d" } + ], + "current": { "pr": "1274@abaf938b0c1d", "verdict": "abaf938b0c1d", "check:tests-ok": "abaf938b0c1d" } + } + } + ] +} diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index a4e46fe86..23e4cb7fd 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -3,6 +3,7 @@ import { test } from 'node:test' import assert from 'node:assert/strict' import { readFileSync } from 'node:fs' import { judge, prFromHeadRef } from './merge-gate.mjs' +import { guard } from './enqueue-rules.mjs' import { fileURLToPath } from 'node:url' const HEAD = 'abaf938b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f' @@ -594,3 +595,50 @@ test('#960: the pull_request report is unchanged and still never fails on the bo assert.equal(result.ok, true) assert.equal(result.mode, 'report') }) + +// #1272 step 2, acceptance 9. The fixture is the one place three of these sentences are written down: +// `core/test/workflow-repo-dev.test.js` asserts the `repo-dev` preset's gate guards refuse with the +// same words, and neither side imports the other. The readers pinned here are `judge` — the function +// merge-gate's required check runs — and `guard`, whose joined reasons are exactly what +// `enqueue-guard` prints after "do not enqueue #N:". Change a sentence on either side and this is red. +const workflowCases = JSON.parse(readFileSync(fileURLToPath(new URL('./fixtures/workflow-cases.json', import.meta.url)), 'utf8')) + +test('#1272 step 2: the workflow fixture sentences are the gate s own, byte for byte', () => { + assert.ok(workflowCases.cases.length >= 3) + for (const entry of workflowCases.cases) { + const reports = entry.gate.reports.map((r) => say([ + `Tip SHA: ${r.tip}`, + `Written-by: ${r.writtenBy}`, + 'Ran:', + ' - npm run verify → pass', + `Verdict: ${r.verdict}`, + ].join('\n'))) + const result = judge({ + labels: entry.gate.labels, + head: entry.gate.head, + author: 'opener', + reports, + policy, + expectedHead: entry.gate.expectedHead ?? undefined, + }) + assert.ok(result.reasons.includes(entry.sentence), `${entry.id}: judge said ${JSON.stringify(result.reasons)}`) + // enqueue-guard's REFUSE line is `do not enqueue #N: `, so the same sentence has to + // appear there too, for the same input. + const refused = guard({ + pr: 1272, + head: entry.gate.head, + labels: [...entry.gate.labels, 'agent-pr'], + body: 'Fixes #1272\nOwner: Dev3\nReviewer: Reviewer', + files: ['core/src/shared/workflow.js'], + mechanicalRelease: false, + fork: false, + draft: false, + merged: false, + result, + reports, + policy, + }) + assert.equal(refused.ok, false) + assert.ok(refused.message.includes(`${entry.sentence}`), `${entry.id}: enqueue-guard said ${refused.message}`) + } +}) diff --git a/core/src/host/workflow-facts.js b/core/src/host/workflow-facts.js new file mode 100644 index 000000000..3f3370366 --- /dev/null +++ b/core/src/host/workflow-facts.js @@ -0,0 +1,116 @@ +// What reads a fact (#1272 step 2, acceptance 10): one function per fact a guard asks about. A guard +// is a predicate over facts somebody else read, so every function here takes the answer a reader +// already got and turns it into one artifact — the fact's spec, the revision it was read at, and the +// value the node asked for. Nothing here fetches: the reader is the caller's, which is what keeps this +// module pure and what lets a test drive it with a recorded answer. +// +// The rule that matters: **no token, no answer, or an answer missing the field means `unreadable`**, +// never a guess. A fact nobody could read must refuse the node it feeds (`artifact.js` `freshness`, +// `workflow.js` `enterNode`), because "I did not manage to look" and "it is current" are not the same +// answer and only one of them may move work forward. + +import { artifact } from '../shared/artifact.js' + +// Every fact this module can answer, as `grep`-able names: a provider that is not here is one nobody +// wrote yet, and `workflowFact` says so rather than returning an empty artifact. +export const FACT_NAMES = Object.freeze([ + 'github.labels', + 'github.checkAtSha', + 'github.verdictAtTip', + 'github.mergeable', + 'github.prBodyTip', + 'task.progress', + 'manual.tick', +]) + +// The answer a caller gets when the fact could not be read: `ok: false` and the reason, with no +// `input`, so a guard cannot mistake it for a fact that happened to be empty. +export function unreadable(fact, why) { + return { fact, ok: false, reason: 'unreadable', why, input: null } +} + +// A fact that was read: one artifact and the same `ok` the caller checks. +const read = (fact, input) => (input ? { fact, ok: true, reason: 'read', why: null, input } : unreadable(fact, 'the answer carried no revision')) + +// GitHub reads need the token the host holds. Without it nothing was read, and a fact filled in from +// "the pull request probably has no hold label" is the one thing that would let a held card pass. +const hasToken = (token) => typeof token === 'string' && token.trim() !== '' + +const labelName = (label) => (typeof label === 'string' ? label : label?.name ?? null) + +// `pr` at the head the labels were read at, valued `hold` when the pull request carries that label +// and `none` otherwise. The gate's first guard is `equals: 'none'`. +export function githubLabels({ token, head, labels } = {}) { + if (!hasToken(token)) return unreadable('github.labels', 'no GITHUB_TOKEN') + if (!head || !Array.isArray(labels)) return unreadable('github.labels', 'the pull request labels were not read') + const names = labels.map(labelName).filter(Boolean).map((name) => String(name).toLowerCase()) + return read('github.labels', artifact('pr', head, { value: names.includes('hold') ? 'hold' : 'none' })) +} + +// `check:` 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. +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 })) +} + +// `verdict` at the tip the conclusion names, valued PASS / INC / FAIL / REQUEST_CHANGES and carrying +// the Bot that wrote it, which is what says who holds the gate. +export function githubVerdictAtTip({ token, tip, verdict, by } = {}) { + if (!hasToken(token)) return unreadable('github.verdictAtTip', 'no GITHUB_TOKEN') + if (!tip || !verdict) return unreadable('github.verdictAtTip', 'the newest conclusion at this tip was not read') + return read('github.verdictAtTip', artifact('verdict', tip, { value: verdict, by: by ?? null })) +} + +// `mergeable` against the tip it was computed on: `mergeable` / `conflict` / `unknown`. GitHub's own +// answer for a tip it has not computed yet is `unknown`, and `unknown` is not `mergeable`. +export function githubMergeable({ token, head, mergeable } = {}) { + if (!hasToken(token)) return unreadable('github.mergeable', 'no GITHUB_TOKEN') + if (!head || !mergeable) return unreadable('github.mergeable', 'the mergeable answer was not read') + return read('github.mergeable', artifact('mergeable', head, { value: mergeable })) +} + +// The `Tip SHA:` line of the pull request body, as the tip it names: this is the fact a checkpoint is +// read through, and a body with no such line is unreadable, not an empty tip. +export function githubPrBodyTip({ token, body } = {}) { + if (!hasToken(token)) return unreadable('github.prBodyTip', 'no GITHUB_TOKEN') + if (typeof body !== 'string') return unreadable('github.prBodyTip', 'the pull request body was not read') + const found = /^\s*Tip SHA:\s*([0-9a-f]{7,40})\s*$/im.exec(body) + if (!found) return unreadable('github.prBodyTip', 'the body carries no Tip SHA line') + return read('github.prBodyTip', artifact('text:pr-body-tip', found[1], { value: found[1].toLowerCase() })) +} + +// A card's own progress, 0–100, as a text fact: the revision is the number itself, so a card that has +// not moved is the same revision and a card that moved is a different one. +export function taskProgress({ card } = {}) { + if (!card || typeof card !== 'object') return unreadable('task.progress', 'the card was not read') + const progress = Number.isInteger(card.progress) ? card.progress : null + if (progress === null || progress < 0 || progress > 100) return unreadable('task.progress', 'the card carries no progress between 0 and 100') + return read('task.progress', artifact('text:card-progress', String(progress), { value: progress })) +} + +// A tick a node recorded itself — the one fact nothing outside read. It still needs a revision: the +// instant the node recorded, so a tick from before the work is not the tick that says it is done. +export function manualTick({ at, by } = {}) { + if (!at) return unreadable('manual.tick', 'the node recorded no instant') + return read('manual.tick', artifact('text:tick', at, { value: by ?? null })) +} + +export const FACT_PROVIDERS = Object.freeze({ + 'github.labels': githubLabels, + 'github.checkAtSha': githubCheckAtSha, + 'github.verdictAtTip': githubVerdictAtTip, + 'github.mergeable': githubMergeable, + 'github.prBodyTip': githubPrBodyTip, + 'task.progress': taskProgress, + 'manual.tick': manualTick, +}) + +// One spelling for a caller that has a fact's name as data (a guard's `spec`, a provider list): a name +// nobody wrote a function for is unreadable, with the name in the reason. +export function workflowFact(name, args = {}) { + const provider = FACT_PROVIDERS[name] + if (!provider) return unreadable(String(name ?? ''), 'no provider answers this fact') + return provider(args) +} diff --git a/core/src/shared/workflow-blank.js b/core/src/shared/workflow-blank.js index b6f4f31f3..2f7fec35d 100644 --- a/core/src/shared/workflow-blank.js +++ b/core/src/shared/workflow-blank.js @@ -1,4 +1,4 @@ -// The `blank` preset (#1272): three nodes, one Bot each, no guards, no repository. It is here as +// The `blank` preset (#1272): three nodes, one role each, no guards, no repository. It is here as // proof that a workflow is data — nothing in src/shared/workflow.js names these ids, and the same // JSON dropped in the workspace as `workflows/blank.json` is the whole definition. @@ -12,7 +12,7 @@ export const blankWorkflow = Object.freeze({ { id: 'draft', name: 'Draft', - bot: 'Writer', + role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'], guards: [], @@ -21,7 +21,7 @@ export const blankWorkflow = Object.freeze({ { id: 'review', name: 'Review', - bot: 'Editor', + role: 'Editor', inputs: ['text:draft'], outputs: ['text:edited'], guards: [], @@ -30,7 +30,7 @@ export const blankWorkflow = Object.freeze({ { id: 'publish', name: 'Publish', - bot: 'Publisher', + role: 'Publisher', inputs: ['text:edited'], outputs: ['link:published'], guards: [], @@ -44,5 +44,5 @@ export const blankWorkflow = Object.freeze({ loops: [], }) -// The Bots the preset names, so a caller can pass them as the team without inventing a roster. +// The roles the preset names, so a caller can pass them as the team without inventing a roster. export const BLANK_TEAM = Object.freeze(['Writer', 'Editor', 'Publisher']) diff --git a/core/src/shared/workflow-repo-dev.js b/core/src/shared/workflow-repo-dev.js new file mode 100644 index 000000000..492c7de65 --- /dev/null +++ b/core/src/shared/workflow-repo-dev.js @@ -0,0 +1,152 @@ +// The `repo-dev` preset (#1272 step 2): one repository change, from a ready issue to a release, as +// data. Every node names the role that does it, the typed facts it takes in and hands out, the guards +// that must hold before it starts, and the sentence it refuses with. No node id and no role is +// special-cased in `workflow.js`; the same calls run this and the `blank` preset. +// +// Two of the roles are not one Bot on a roster, and they say so in the form the engine can read +// rather than with a placeholder: +// - `verify` is `SelfTest` **and** CI. The Bot runs the suite; the CI run is the `check:tests-ok` +// fact this node hands out, and the node refuses until both the self-test and that fact exist. +// - `gate` is held by *the Reviewer that judged the pull request*, which is the `by` of the verdict +// this node takes in — not a fixed assignee. The role is `Reviewer` and the guards read the +// verdict's own `Written-by`, so the card cannot be held by someone who did not write it. +// +// `column` is which board column a card in this node sits in (#1265). `review` carries `false`: a +// review is a pull request's state, not a third lane of its own, so the node stays in the definition +// and draws no column on the board. + +export const repoDevWorkflow = Object.freeze({ + id: 'repo-dev', + name: 'Repository change', + version: 1, + inputs: ['issue'], + deliverables: ['release'], + nodes: [ + { + id: 'triage', + name: 'Triage', + role: 'Triage', + column: 'not-started', + inputs: ['issue'], + outputs: ['issue.ready'], + guards: [], + refusal: '{node} cannot start: it needs {spec} and has not read it yet.', + }, + { + id: 'build', + name: 'Build', + role: 'Dev', + column: 'in-progress', + inputs: ['issue.ready'], + outputs: ['pr'], + guards: [], + refusal: '{node} cannot start: its input {input} is not ready.', + }, + { + id: 'verify', + name: 'Verify — SelfTest and CI', + role: 'SelfTest', + column: 'in-progress', + inputs: ['pr'], + outputs: ['check:tests-ok'], + guards: [], + refusal: '{node} cannot start: it needs {spec} and has not run it yet.', + }, + { + id: 'review', + name: 'Review', + role: 'Reviewer', + // A review is the pull request's own state, not a column of the repository board (#1265). + column: false, + inputs: ['pr', 'check:tests-ok'], + outputs: ['verdict'], + guards: [ + { + id: 'tests-ran-at-tip', + spec: 'check:tests-ok', + is: 'fresh', + refusal: '{node} refuses: {input} did not run at the tip {now}, so there is nothing to review.', + }, + ], + refusal: '{node} cannot start: its input {input} is not current.', + }, + { + id: 'gate', + name: 'Gate', + role: 'Reviewer', + column: 'in-progress', + // The verdict is not an input the node waits for: it is the fact the guards check, which is what + // says whether the pull request may go on at all. Its own revision is what the head guard reads. + inputs: ['pr', 'check:tests-ok'], + outputs: ['risk:tier'], + guards: [ + { + id: 'no-hold', + spec: 'pr', + is: 'equals', + value: 'none', + // The merge gate's own sentence for a held pull request (`merge-gate.mjs` `judge`), pinned + // in `.github/scripts/fixtures/workflow-cases.json` and asserted byte for byte. + refusal: 'the pull request carries the `hold` label', + }, + { + id: 'verdict-at-tip', + spec: 'verdict', + is: 'fresh', + // …and its sentence for a head that is not the one the caller asked about. + refusal: 'the pull request head is {now}, not the expected {have}', + }, + { + id: 'verdict-says-pass', + spec: 'verdict', + is: 'equals', + value: 'PASS', + // …and its sentence for a conclusion that says something else, naming who said it. + refusal: '{by} says {value} at this tip', + }, + { + id: 'tests-ran-at-tip', + spec: 'check:tests-ok', + is: 'fresh', + refusal: '{node} refuses: {input} did not run at the tip {now}.', + }, + ], + refusal: '{node} cannot start: its input {input} is not current.', + }, + { + id: 'merge', + name: 'Merge', + // The merge queue writes the commit, so no Bot holds this node. + role: 'system', + column: 'done', + inputs: ['verdict', 'risk:tier'], + outputs: ['merge-commit'], + guards: [], + refusal: '{node} cannot start: its input {input} is not current.', + }, + { + id: 'release', + name: 'Release', + role: 'Release', + column: 'done', + inputs: ['merge-commit'], + outputs: ['release'], + guards: [], + refusal: '{node} cannot start: it needs {spec} and has not published it yet.', + }, + ], + edges: [ + { from: 'triage.issue.ready', to: 'build.issue.ready' }, + { from: 'build.pr', to: 'verify.pr' }, + { from: 'verify.check:tests-ok', to: 'review.check:tests-ok' }, + { from: 'review.verdict', to: 'merge.verdict' }, + { from: 'gate.risk:tier', to: 'merge.risk:tier' }, + { from: 'merge.merge-commit', to: 'release.merge-commit' }, + ], + // No declared loop yet: an INC sends the card back to `build` on the board, and a `loops[]` row is + // a dependency the graph already has (declaring one nothing implies is `loop/not-an-edge`). + loops: [], +}) + +// The roles the preset names, so a caller can pass them as the team without inventing a roster. +export const REPO_DEV_TEAM = Object.freeze(['Triage', 'Dev', 'SelfTest', 'Reviewer', 'Release']) diff --git a/core/src/shared/workflow.js b/core/src/shared/workflow.js index 1d839e456..3535afabc 100644 --- a/core/src/shared/workflow.js +++ b/core/src/shared/workflow.js @@ -10,28 +10,45 @@ export const WORKFLOW_VERSION = 1 const text = (value) => (typeof value === 'string' ? value.trim() : '') const textList = (value) => (Array.isArray(value) ? value.map(text).filter(Boolean) : []) -const GUARD_CHECKS = ['present', 'fresh', 'equals'] +export const GUARD_CHECKS = ['present', 'fresh', 'equals'] +export const NODE_COLUMNS = ['not-started', 'in-progress', 'done'] function normaliseGuard(raw) { const spec = text(raw?.spec) - const is = text(raw?.is) return { id: text(raw?.id) || spec, spec, - is: GUARD_CHECKS.includes(is) ? is : 'present', + // The check is kept exactly as written. An unknown one used to be read as `present`, which is a + // guard that can never refuse: `is: 'fres'` left the node unguarded and said nothing. Validation + // refuses it, and `guardRefusals` refuses it too, so a file that never went through validation + // still cannot pass on a check nobody implemented (#1291 debt). + is: text(raw?.is), value: raw?.value === undefined ? null : raw.value, + // One node can refuse several facts for different reasons, so a guard carries its own sentence + // when it needs one; `renderRefusal` prefers it over the node's. + refusal: text(raw?.refusal), } } +// `column: false` draws no column; a name is the board column this node's cards sit in; absent is +// "not stated", and then the node id is the column — what step 1 did (#1265, #1272 step 2). +function normaliseColumn(value) { + if (value === false) return false + return text(value) === '' ? null : text(value) +} + function normaliseNode(raw) { return { id: text(raw?.id), name: text(raw?.name) || text(raw?.id), - bot: text(raw?.bot), + // One word for one thing: a node names the `role` that does it. The older spelling `bot` meant the + // same thing and is not read any more, so a definition cannot state both and disagree (#1291 debt). + role: text(raw?.role), inputs: textList(raw?.inputs), outputs: textList(raw?.outputs), guards: Array.isArray(raw?.guards) ? raw.guards.map(normaliseGuard) : [], refusal: text(raw?.refusal), + column: normaliseColumn(raw?.column), } } @@ -74,13 +91,21 @@ export function splitEndpoint(endpoint, nodeIds) { return null } +// The spelling a fact is keyed by: `text: draft` and `text:draft` are one fact, not two (#1291 +// debt). Anything outside the vocabulary is not a spec and has no key. +export function canonicalSpec(spec) { + const parsed = parseArtifactSpec(spec) + return parsed ? formatArtifactSpec(parsed) : null +} + export function producersOf(def) { const producers = {} for (const node of def.nodes) { for (const spec of node.outputs) { - if (!parseArtifactSpec(spec)) continue - producers[spec] = producers[spec] ?? [] - if (!producers[spec].includes(node.id)) producers[spec].push(node.id) + const key = canonicalSpec(spec) + if (!key) continue + producers[key] = producers[key] ?? [] + if (!producers[key].includes(node.id)) producers[key].push(node.id) } } return producers @@ -90,9 +115,10 @@ export function consumersOf(def) { const consumers = {} for (const node of def.nodes) { for (const spec of node.inputs) { - if (!parseArtifactSpec(spec)) continue - consumers[spec] = consumers[spec] ?? [] - if (!consumers[spec].includes(node.id)) consumers[spec].push(node.id) + const key = canonicalSpec(spec) + if (!key) continue + consumers[key] = consumers[key] ?? [] + if (!consumers[key].includes(node.id)) consumers[key].push(node.id) } } return consumers @@ -111,8 +137,9 @@ export function resolvedEdges(raw) { to: edge.to, fromNode: from?.node ?? null, toNode: to?.node ?? null, + // One field for one value (#1291 debt): the artifact the edge carries. `carries` said the same + // thing twice, and a reader could never tell which one a change was supposed to move. artifact: from?.artifact ?? null, - carries: from?.artifact ?? null, } }) } @@ -137,7 +164,7 @@ export function dependencyEdges(raw) { } for (const node of def.nodes) { for (const spec of node.inputs) { - for (const from of producers[spec] ?? []) { + for (const from of producers[canonicalSpec(spec)] ?? []) { if (!isLoopEdge(def, from, node.id)) add(from, node.id) } } @@ -181,24 +208,30 @@ export function topologicalOrder(raw) { // state, and there is no built-in list of them anywhere in this file. export function workflowColumns(raw) { const def = normaliseWorkflow(raw) - return topologicalOrder(def).map((id) => { - const node = nodeById(def, id) - return { + return topologicalOrder(def) + .map((id) => nodeById(def, id)) + // `column: false` draws no column (#1265): a review is a pull request's state, not a lane of its + // own, so the node stays in the definition and out of the board. + .filter((node) => node.column !== false) + .map((node) => ({ id: node.id, + // The board column this node's cards sit in. A definition that states none keeps step 1's + // answer — the node id itself — so nothing that already read these columns moves. + column: node.column ?? node.id, name: node.name, - bot: node.bot, + role: node.role, inputs: node.inputs.slice(), outputs: node.outputs.slice(), guards: node.guards.map((guard) => ({ ...guard })), refusal: node.refusal, - } - }) + })) } // Who does this node: the definition's Bot, which is also the card's assignee while the card sits // there, so the two cannot disagree. export function holderOf(raw, nodeId) { - return nodeById(normaliseWorkflow(raw), nodeId)?.bot ?? null + const role = nodeById(normaliseWorkflow(raw), nodeId)?.role ?? '' + return role === '' ? null : role } function ranksOf(def, edges) { @@ -240,7 +273,7 @@ export function layoutWorkflow(raw, options = {}) { boxes.push({ id: node.id, name: node.name, - bot: node.bot, + role: node.role, rank: r, row, inputs: node.inputs.slice(), @@ -295,13 +328,25 @@ export function renderRefusal(node, details = {}) { // A revision that could not be read is never written as an empty slot, and never as `null`. now: unreadable ? 'an unreadable revision' : (details.now ?? 'no revision'), want: String(details.want ?? ''), + // What the fact said, and who wrote it: the sentences the merge gate emits name both, so a preset + // that has to say the same thing can be written in one template (#1272 step 2, acceptance 9). + value: details.input?.value ?? '', + by: details.input?.by ?? '', + } + // A check the engine does not implement is a defect in the definition, not a fact: it refuses with + // its own sentence instead of being read as `present` (#1291 debt). + if (details.unchecked !== undefined) { + return `${name} refuses: its check on ${spec ?? 'a fact'} asks "${details.unchecked}", which is not one of ${GUARD_CHECKS.join(', ')}.` } if (unreadable) { // The node's own wording knows "has not been produced" and "is stale"; neither is this answer, // so the engine names what it read and that the source could not be read at all. return `${name} refuses: its input ${input ?? spec} could not be checked against the source — it reads ${details.have ?? 'no revision'} and the source revision is unreadable, so the card must be read before it starts.` } - if (node?.refusal) return fill(node.refusal, values) + // The guard's own sentence wins over the node's: one node refuses several facts, and the sentence + // that fits a held pull request does not fit a verdict that says INC. + const template = details.guard?.refusal || node?.refusal + if (template) return fill(template, values) if (details.want !== undefined && details.want !== null) { return `${name} refuses: its input ${input ?? spec} does not say ${details.want}.` } @@ -339,23 +384,31 @@ function guardRefusals(node, facts) { const current = facts.current ?? {} const refusals = [] for (const guard of node.guards) { - const spec = parseArtifactSpec(guard.spec) ? formatArtifactSpec(parseArtifactSpec(guard.spec)) : null + const spec = canonicalSpec(guard.spec) if (!spec) { refusals.push({ guard, refusal: renderRefusal(node, { spec: guard.spec }) }) continue } + // Read before the fact: an unknown check is a defect in the file, and a reader has to be told + // that rather than shown a refusal about a fact that was never looked at. + if (!GUARD_CHECKS.includes(guard.is)) { + refusals.push({ guard, refusal: renderRefusal(node, { spec, guard, unchecked: guard.is }) }) + continue + } const have = artifactAt(present, spec) const state = freshness(have, 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. refusals.push({ guard, refusal: renderRefusal(node, { spec }) }) continue } if (guard.is === 'fresh' && !state.fresh) { - refusals.push({ guard, refusal: renderRefusal(node, { spec, input: have, stale: true, have: state.have, now: state.now }) }) + refusals.push({ guard, refusal: renderRefusal(node, { spec, input: have, guard, stale: true, have: state.have, now: state.now }) }) continue } if (guard.is === 'equals' && String(have.value ?? '') !== String(guard.value ?? '')) { - refusals.push({ guard, refusal: renderRefusal(node, { spec, input: have, want: guard.value }) }) + refusals.push({ guard, refusal: renderRefusal(node, { spec, input: have, guard, have: have.revision, want: guard.value }) }) } } return refusals @@ -434,13 +487,35 @@ export function validateWorkflow(raw, options = {}) { problems.push(message('node/duplicate-id', `two nodes share the id "${node.id}"`, { node: node.id })) } ids.add(node.id) - if (node.bot === '') { - problems.push(message('node/without-bot', `node "${node.id}" names no Bot; every node is done by one`, { node: node.id })) - } else if (team && !team.includes(node.bot) && node.bot !== 'system' && !node.bot.startsWith('human:')) { + if (node.role === '') { + problems.push(message('node/without-role', `node "${node.id}" names no role; every node is done by one`, { node: node.id })) + } else if (team && !team.includes(node.role) && node.role !== 'system' && !node.role.startsWith('human:')) { problems.push( - message('node/unknown-bot', `node "${node.id}" names "${node.bot}", who is not on this team`, { node: node.id }), + message('node/unknown-role', `node "${node.id}" names "${node.role}", who is not on this team`, { node: node.id }), ) } + if (node.column !== null && node.column !== false && !NODE_COLUMNS.includes(node.column)) { + problems.push( + message( + 'node/unknown-column', + `node "${node.id}" names the column "${node.column}", which is not one of ${NODE_COLUMNS.join(', ')}`, + { node: node.id }, + ), + ) + } + for (const guard of node.guards) { + // #1291 debt: an unknown check is refused, not read as `present`. The two spellings are named + // so the fix reads as "pick one of these" rather than "something about the guard". + if (!GUARD_CHECKS.includes(guard.is)) { + problems.push( + message( + 'guard/unknown-check', + `node "${node.id}" guard "${guard.id}" asks is: "${guard.is}", which is not one of ${GUARD_CHECKS.join(', ')}`, + { node: node.id, guard: guard.id }, + ), + ) + } + } for (const spec of [...node.inputs, ...node.outputs]) { if (!parseArtifactSpec(spec)) { problems.push( @@ -454,14 +529,18 @@ export function validateWorkflow(raw, options = {}) { const producers = producersOf(def) const consumers = consumersOf(def) - const declaredInputs = new Set(def.inputs) - const deliverables = new Set(def.deliverables) + // `text: draft` and `text:draft` are one fact: the declared lists are read the same way the + // producers are keyed, or a workflow that spells one spelling would call its own input a defect. + const canonicalList = (list) => list.map(canonicalSpec).filter(Boolean) + const declaredInputs = new Set(canonicalList(def.inputs)) + const deliverables = new Set(canonicalList(def.deliverables)) for (const node of def.nodes) { if (!ids.has(node.id)) continue for (const spec of node.inputs) { - if (!parseArtifactSpec(spec)) continue - if ((producers[spec] ?? []).length === 0 && !declaredInputs.has(spec)) { + const key = canonicalSpec(spec) + if (!key) continue + if ((producers[key] ?? []).length === 0 && !declaredInputs.has(key)) { problems.push( message('input/without-producer', `node "${node.id}" takes in "${spec}", but no node hands it out`, { node: node.id, @@ -471,8 +550,9 @@ export function validateWorkflow(raw, options = {}) { } } for (const spec of node.outputs) { - if (!parseArtifactSpec(spec)) continue - if ((consumers[spec] ?? []).length === 0 && !deliverables.has(spec)) { + const key = canonicalSpec(spec) + if (!key) continue + if ((consumers[key] ?? []).length === 0 && !deliverables.has(key)) { problems.push( message( 'output/without-consumer', diff --git a/core/test/workflow-repo-dev.test.js b/core/test/workflow-repo-dev.test.js new file mode 100644 index 000000000..51b5fd7cd --- /dev/null +++ b/core/test/workflow-repo-dev.test.js @@ -0,0 +1,225 @@ +// The `repo-dev` preset and the fact providers (#1272 step 2). What this file pins: +// - the preset is data: seven nodes, the roles the team really has, and the columns it draws; +// - the four debts #1291 named each have a test of their own; +// - the gate's refusal sentences are the merge gate's own sentences, byte for byte, through +// `.github/scripts/fixtures/workflow-cases.json` — the file both sides read and neither imports; +// - a fact nobody could read is `unreadable`, never a guess. +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { test } from 'node:test' +import { fileURLToPath } from 'node:url' +import { artifact, artifactsBySpec } from '../src/shared/artifact.js' +import { + canonicalSpec, + consumersOf, + dependencyEdges, + enterNode, + GUARD_CHECKS, + holderOf, + inputStates, + layoutWorkflow, + nodeById, + producersOf, + resolvedEdges, + validateWorkflow, + workflowColumns, +} from '../src/shared/workflow.js' +import { blankWorkflow } from '../src/shared/workflow-blank.js' +import { REPO_DEV_TEAM, repoDevWorkflow } from '../src/shared/workflow-repo-dev.js' +import { FACT_NAMES, FACT_PROVIDERS, githubCheckAtSha, githubLabels, githubPrBodyTip, githubVerdictAtTip, manualTick, taskProgress, workflowFact } from '../src/host/workflow-facts.js' + +const cases = JSON.parse(readFileSync(fileURLToPath(new URL('../../.github/scripts/fixtures/workflow-cases.json', import.meta.url)), 'utf8')) + +const factsFor = (entry) => ({ + present: artifactsBySpec(entry.present.map((item) => artifact(item.spec, item.revision, { value: item.value, by: item.by }))), + current: entry.current, +}) + +test('the repo-dev preset is data, and it validates with the roles this team has', () => { + const result = validateWorkflow(repoDevWorkflow, { team: REPO_DEV_TEAM }) + assert.equal(result.ok, true, JSON.stringify(result.problems)) + assert.deepEqual(repoDevWorkflow.nodes.map((node) => node.id), ['triage', 'build', 'verify', 'review', 'gate', 'merge', 'release']) + assert.deepEqual(repoDevWorkflow.nodes.map((node) => node.role), ['Triage', 'Dev', 'SelfTest', 'Reviewer', 'Reviewer', 'system', 'Release']) + // The two roles that are not one Bot on a roster say what they are, not who: the self-test Bot with + // CI as the `check:tests-ok` fact it hands out, and the Reviewer that judged the verdict it reads. + assert.equal(nodeById(repoDevWorkflow, 'verify').name, 'Verify — SelfTest and CI') + assert.deepEqual(nodeById(repoDevWorkflow, 'verify').outputs, ['check:tests-ok']) + assert.equal(holderOf(repoDevWorkflow, 'gate'), 'Reviewer') + // The verdict is the fact the gate guards, not an input it waits for: its revision is what the head + // guard compares against the live tip. + assert.deepEqual(nodeById(repoDevWorkflow, 'gate').guards.map((guard) => guard.spec), ['pr', 'verdict', 'verdict', 'check:tests-ok']) + assert.deepEqual(nodeById(repoDevWorkflow, 'gate').inputs, ['pr', 'check:tests-ok']) + // The merge is the queue's, so no Bot is named for it. + assert.equal(holderOf(repoDevWorkflow, 'merge'), 'system') +}) + +test('the columns are the definition s, and a node that says column: false draws none (#1265)', () => { + const columns = workflowColumns(repoDevWorkflow) + // `review` is the pull request's state, not a third lane: it stays in the definition, out of the board. + assert.deepEqual(columns.map((column) => column.id), ['triage', 'build', 'verify', 'gate', 'merge', 'release']) + assert.deepEqual(columns.map((column) => column.column), ['not-started', 'in-progress', 'in-progress', 'in-progress', 'done', 'done']) + assert.deepEqual(columns.map((column) => column.role), ['Triage', 'Dev', 'SelfTest', 'Reviewer', 'system', 'Release']) + // The node is still there — it just is not a column — so `enterNode` can still refuse it. + assert.ok(nodeById(repoDevWorkflow, 'review')) + assert.equal(nodeById(repoDevWorkflow, 'review').column, false) + // The flowchart keeps every node: the board's columns and the picture are two questions. + assert.equal(layoutWorkflow(repoDevWorkflow).boxes.length, repoDevWorkflow.nodes.length) +}) + +test('a definition that states no column keeps step 1 s answer, the node id', () => { + const columns = workflowColumns(blankWorkflow) + assert.deepEqual(columns.map((column) => column.id), ['draft', 'review', 'publish']) + assert.deepEqual(columns.map((column) => column.column), ['draft', 'review', 'publish']) +}) + +test('a column the board does not have, and a guard check nobody implements, are each refused by name', () => { + const base = { + inputs: ['text:topic'], + nodes: [ + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + ], + deliverables: ['link:published'], + } + const wrongColumn = validateWorkflow({ ...base, nodes: [{ ...base.nodes[0], column: 'doing' }, base.nodes[1]] }, { team: ['Writer', 'Publisher'] }) + assert.deepEqual(wrongColumn.problems.map((problem) => problem.code), ['node/unknown-column']) + assert.match(wrongColumn.problems[0].message, /names the column "doing", which is not one of not-started, in-progress, done/) + + // #1291 debt: `is: 'fres'` used to be read as `present`, which is a guard that can never refuse. + const wrongCheck = validateWorkflow({ ...base, nodes: [{ ...base.nodes[0], guards: [{ id: 'pass', spec: 'verdict', is: 'fres', value: 'PASS' }] }, base.nodes[1]] }, { team: ['Writer', 'Publisher'] }) + assert.ok(wrongCheck.problems.some((problem) => problem.code === 'guard/unknown-check')) + assert.match(wrongCheck.problems[0].message, /guard "pass" asks is: "fres", which is not one of present, fresh, equals/) +}) + +test('a guard whose check nobody implements refuses instead of passing on a guess (#1291 debt)', () => { + const def = { + inputs: ['pr'], + nodes: [ + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'], guards: [{ id: 'pass', spec: 'verdict', is: 'fres', value: 'PASS' }] }, + ], + deliverables: ['queue-entry'], + } + const present = { verdict: artifact('verdict', 'abc1234', { value: 'INC' }) } + const refused = enterNode(def, 'gate', { present, current: { verdict: 'abc1234' } }) + // Before the fix this guard was dropped and the node started on an INC. + assert.equal(refused.ok, false) + assert.match(refused.refusal, /its check on verdict asks "fres", which is not one of present, fresh, equals/) + assert.deepEqual(GUARD_CHECKS, ['present', 'fresh', 'equals']) +}) + +test('a fact is keyed by its parsed spec, so "text: draft" and "text:draft" are one fact (#1291 debt)', () => { + const def = { + inputs: ['text:topic'], + nodes: [ + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text: draft'] }, + { id: 'edit', name: 'Edit', role: 'Editor', inputs: ['text:draft'], outputs: ['link:published'] }, + ], + deliverables: ['link:published'], + } + assert.equal(canonicalSpec('text: draft'), 'text:draft') + assert.deepEqual(Object.keys(producersOf(def)), ['text:draft', 'link:published']) + assert.deepEqual(Object.keys(consumersOf(def)), ['text:topic', 'text:draft']) + // The one producer is found through the canonical key, so the workflow is valid and the dependency + // exists: with the raw spelling it read "no producer" and "no consumer", and lost the edge. + assert.equal(validateWorkflow(def, { team: ['Writer', 'Editor'] }).ok, true, JSON.stringify(validateWorkflow(def, { team: ['Writer', 'Editor'] }).problems)) + assert.deepEqual(dependencyEdges(def), [{ from: 'draft', to: 'edit' }]) +}) + +test('one word for one thing: a node names a role, and the older spelling is not read (#1291 debt)', () => { + const def = { + inputs: ['text:topic'], + nodes: [ + { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['link:published'] }, + ], + deliverables: ['link:published'], + } + const result = validateWorkflow(def, { team: ['Writer'] }) + assert.equal(result.ok, false) + assert.deepEqual(result.problems.map((problem) => problem.code), ['node/without-role']) + assert.equal(holderOf(def, 'draft'), null) + // The two spellings cannot both be written and disagree, because only one of them is read at all. + assert.equal(validateWorkflow({ ...def, nodes: [{ ...def.nodes[0], role: 'Writer' }] }, { team: ['Writer'] }).ok, true) +}) + +test('an edge carries its artifact once, and says so (#1291 debt)', () => { + const edge = resolvedEdges(repoDevWorkflow)[0] + assert.deepEqual(Object.keys(edge).sort(), ['artifact', 'from', 'fromNode', 'to', 'toNode'].sort()) + assert.equal(edge.artifact, 'issue.ready') + assert.equal('carries' in edge, false) +}) + +test('the gate s refusal is the merge gate s own sentence, byte for byte (acceptance 9)', () => { + assert.ok(cases.cases.length >= 3) + for (const entry of cases.cases) { + const { present, current } = factsFor(entry.preset) + const refused = enterNode(repoDevWorkflow, entry.preset.node, { present, current }) + assert.equal(refused.ok, false, `${entry.id} must refuse`) + assert.equal(refused.refusal, entry.sentence, `${entry.id}: the preset and ${entry.where} must say the same thing`) + // And the guard that answered is the one the fixture names, so a sentence cannot pass by accident + // on some other guard that happens to read the same way. + assert.equal(refused.guardRefusals[0].guard.id, entry.preset.guard, `${entry.id} must be refused by ${entry.preset.guard}`) + } +}) + +test('the gate starts only when every fact it reads is present, current and says the right thing', () => { + const entry = cases.cases.find((item) => item.id === 'verdict-not-pass') + const { present, current } = factsFor(entry.preset) + const pass = { ...current, verdict: 'abaf938b0c1d' } + const presentPass = { ...present, verdict: artifact('verdict', 'abaf938b0c1d', { value: 'PASS', by: 'Reviewer' }) } + assert.equal(enterNode(repoDevWorkflow, 'gate', { present: presentPass, current: pass }).ok, true) + // A fact nobody could read refuses rather than passing (acceptance 10): the same node, same facts, + // and the verdict's revision is not readable. + const unread = enterNode(repoDevWorkflow, 'gate', { present: presentPass, current: { ...pass, verdict: null } }) + assert.equal(unread.ok, false) + assert.match(unread.refusal, /could not be checked against the source/) + // A guarded fact nobody could read is refused by the guard, and the card says the same thing: the + // input that is present reads unreadable instead of fine. + const unreadStates = inputStates(repoDevWorkflow, 'gate', { present: presentPass, current: { ...pass, 'check:tests-ok': null } }) + assert.equal(unreadStates.find((state) => state.spec === 'check:tests-ok').unreadable, true) + assert.equal(enterNode(repoDevWorkflow, 'gate', { present: presentPass, current: { ...pass, 'check:tests-ok': null } }).ok, false) +}) + +test('a fact nobody could read is unreadable, and no fact is guessed (acceptance 10)', () => { + assert.deepEqual([...FACT_NAMES].sort(), [...Object.keys(FACT_PROVIDERS)].sort()) + // Every provider answers `unreadable` when it has nothing to answer from: no token, no field. + const nothing = [ + githubLabels({ head: 'abc1234', labels: [] }), + githubCheckAtSha({ name: 'tests-ok', sha: 'abc1234' }), + githubVerdictAtTip({ tip: 'abc1234', verdict: 'PASS' }), + githubPrBodyTip({ body: 'Tip SHA: abc1234' }), + workflowFact('github.nobody'), + ] + for (const answer of [githubLabels({}), githubCheckAtSha({}), githubVerdictAtTip({}), githubMergeableNothing(), githubPrBodyTip({}), taskProgress({}), manualTick({}), ...nothing]) { + assert.equal(answer.ok, false, `${answer.fact} must not answer without a reading`) + assert.equal(answer.reason, 'unreadable') + assert.equal(answer.input, null) + assert.ok(answer.why, `${answer.fact} must say why it could not read`) + } + // With the token and the answer, each one is an artifact at the revision it was read at. + const labels = githubLabels({ token: 'x', head: 'abaf938b0c1d', labels: [{ name: 'hold' }] }) + assert.equal(labels.ok, true) + assert.equal(labels.input.spec, 'pr') + assert.equal(labels.input.value, 'hold') + assert.equal(githubLabels({ token: 'x', head: 'abaf938b0c1d', labels: [] }).input.value, 'none') + const check = githubCheckAtSha({ token: 'x', name: 'tests-ok', sha: 'abaf938b0c1d', conclusion: 'success' }) + assert.equal(check.input.spec, 'check:tests-ok') + assert.equal(check.input.revision, 'abaf938b0c1d') + const verdict = githubVerdictAtTip({ token: 'x', tip: 'abaf938b0c1d', verdict: 'INC', by: 'Reviewer' }) + assert.equal(verdict.input.spec, 'verdict') + assert.equal(verdict.input.value, 'INC') + assert.equal(verdict.input.by, 'Reviewer') + const body = githubPrBodyTip({ token: 'x', body: 'Written-by: Dev3\nTip SHA: abaf938b0c1d\n' }) + assert.equal(body.input.revision, 'abaf938b0c1d') + // A body with no Tip SHA line is unreadable, not an empty tip. + assert.equal(githubPrBodyTip({ token: 'x', body: 'no tip here' }).reason, 'unreadable') + const progress = taskProgress({ card: { progress: 85 } }) + assert.equal(progress.input.revision, '85') + assert.equal(taskProgress({ card: { progress: 140 } }).reason, 'unreadable') + assert.equal(manualTick({ at: '2026-10-26T12:00:00Z' }).input.spec, 'text:tick') +}) + +// `githubMergeable` with nothing to read: kept out of the list above so each call reads as itself. +function githubMergeableNothing() { + return workflowFact('github.mergeable', {}) +} diff --git a/core/test/workflow.test.js b/core/test/workflow.test.js index 9e20c2a8f..51f7f3ad7 100644 --- a/core/test/workflow.test.js +++ b/core/test/workflow.test.js @@ -33,11 +33,11 @@ const content = { inputs: ['text:topic'], deliverables: ['text:archived'], nodes: [ - { id: 'idea', name: 'Idea', bot: 'Planner', inputs: ['text:topic'], outputs: ['text:brief'] }, - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:brief'], outputs: ['text:draft'] }, - { id: 'edit', name: 'Edit', bot: 'Editor', inputs: ['text:draft'], outputs: ['text:edited'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:edited'], outputs: ['link:published'] }, - { id: 'archive', name: 'Archive', bot: 'Archivist', inputs: ['link:published'], outputs: ['text:archived'] }, + { id: 'idea', name: 'Idea', role: 'Planner', inputs: ['text:topic'], outputs: ['text:brief'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:brief'], outputs: ['text:draft'] }, + { id: 'edit', name: 'Edit', role: 'Editor', inputs: ['text:draft'], outputs: ['text:edited'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:edited'], outputs: ['link:published'] }, + { id: 'archive', name: 'Archive', role: 'Archivist', inputs: ['link:published'], outputs: ['text:archived'] }, ], edges: [ { from: 'idea.text:brief', to: 'draft.text:brief' }, @@ -98,16 +98,16 @@ test('the blank preset validates with its own team, and names a Bot on every nod const result = validateWorkflow(blankWorkflow, { team: BLANK_TEAM }) assert.equal(result.ok, true, JSON.stringify(result.problems)) assert.deepEqual(workflowColumns(blankWorkflow).map((column) => column.id), ['draft', 'review', 'publish']) - assert.deepEqual(workflowColumns(blankWorkflow).map((column) => column.bot), ['Writer', 'Editor', 'Publisher']) + assert.deepEqual(workflowColumns(blankWorkflow).map((column) => column.role), ['Writer', 'Editor', 'Publisher']) assert.equal(holderOf(blankWorkflow, 'review'), 'Editor') assert.equal(holderOf(blankWorkflow, 'nowhere'), null) const other = validateWorkflow(blankWorkflow, { team: ['Someone'] }) assert.equal(other.ok, false) assert.deepEqual(other.problems.map((problem) => problem.code), [ - 'node/unknown-bot', - 'node/unknown-bot', - 'node/unknown-bot', + 'node/unknown-role', + 'node/unknown-role', + 'node/unknown-role', ]) }) @@ -115,7 +115,7 @@ test('the columns are the definition s, not a built-in list', () => { const blank = workflowColumns(blankWorkflow) const editorial = workflowColumns(content) assert.deepEqual(editorial.map((column) => column.id), ['idea', 'draft', 'edit', 'publish', 'archive']) - assert.deepEqual(editorial.map((column) => column.bot), contentTeam) + assert.deepEqual(editorial.map((column) => column.role), contentTeam) assert.notDeepEqual(blank.map((column) => column.id), editorial.map((column) => column.id)) // Same calls, two vocabularies: nothing above this line knows either set of ids. assert.equal(validateWorkflow(content, { team: contentTeam }).ok, true, JSON.stringify(validateWorkflow(content, { team: contentTeam }).problems)) @@ -129,7 +129,7 @@ test('the flowchart is generated: one box per node, one edge per edges[] row', ( for (const node of content.nodes) { const box = graph.boxes.find((candidate) => candidate.id === node.id) assert.ok(box, `no box for ${node.id}`) - assert.equal(box.bot, node.bot) + assert.equal(box.role, node.role) assert.deepEqual(box.inputs, node.inputs) assert.deepEqual(box.outputs, node.outputs) } @@ -152,21 +152,21 @@ test('the flowchart is generated: one box per node, one edge per edges[] row', ( const broken = layoutWorkflow({ nodes: content.nodes, edges: [{ from: 'idea.text:brief', to: 'gone.text:brief' }] }) assert.equal(broken.edges.length, 1) assert.equal(broken.edges[0].toBox, null) + // One field for one value: the edge carries `artifact`, and says it once (#1291 debt). assert.deepEqual(resolvedEdges(content)[0], { from: 'idea.text:brief', to: 'draft.text:brief', fromNode: 'idea', toNode: 'draft', artifact: 'text:brief', - carries: 'text:brief', }) }) test('an input nothing produces is refused, unless the workflow declares it as a starting point', () => { const def = { nodes: [ - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, ], edges: [], deliverables: ['link:published'], @@ -182,8 +182,8 @@ test('an output nothing consumes is refused, unless it is a declared deliverable const def = { inputs: ['text:topic'], nodes: [ - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft', 'text:extra'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft', 'text:extra'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, ], deliverables: ['link:published'], } @@ -197,9 +197,9 @@ test('one fact has one producer, and two producers are refused by name', () => { const def = { inputs: ['text:topic'], nodes: [ - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'redraft', name: 'Redraft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'redraft', name: 'Redraft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, ], deliverables: ['link:published'], } @@ -208,31 +208,31 @@ test('one fact has one producer, and two producers are refused by name', () => { assert.match(result.problems[0].message, /"text:draft" is handed out by "draft" and "redraft"/) }) -test('a node without a Bot is refused, and so is a Bot that names nobody on the team', () => { +test('a node without a role is refused, and so is a role that names nobody on the team', () => { const def = { inputs: ['text:topic'], nodes: [ - { id: 'draft', name: 'Draft', bot: '', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'publish', name: 'Publish', bot: 'Ghost', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: '', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Ghost', inputs: ['text:draft'], outputs: ['link:published'] }, ], deliverables: ['link:published'], } const loose = validateWorkflow(def) - assert.deepEqual(loose.problems.map((problem) => problem.code), ['node/without-bot']) + assert.deepEqual(loose.problems.map((problem) => problem.code), ['node/without-role']) const strict = validateWorkflow(def, { team: ['Writer'] }) - assert.deepEqual(strict.problems.map((problem) => problem.code), ['node/without-bot', 'node/unknown-bot']) + assert.deepEqual(strict.problems.map((problem) => problem.code), ['node/without-role', 'node/unknown-role']) assert.match(strict.problems[1].message, /node "publish" names "Ghost", who is not on this team/) - // `system` and a human role are part of the model, not a missing Bot. - assert.equal(validateWorkflow({ inputs: ['text:topic'], nodes: [{ id: 'merge', name: 'Merge', bot: 'system', inputs: ['text:topic'], outputs: ['merge-commit'] }], deliverables: ['merge-commit'] }, { team: [] }).ok, true) + // `system` and a human role are part of the model, not a missing role. + assert.equal(validateWorkflow({ inputs: ['text:topic'], nodes: [{ id: 'merge', name: 'Merge', role: 'system', inputs: ['text:topic'], outputs: ['merge-commit'] }], deliverables: ['merge-commit'] }, { team: [] }).ok, true) }) test('a cycle that is not declared is refused; a declared loop is how work goes back', () => { const twoStep = { inputs: ['text:topic'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr'] }, - { id: 'review', name: 'Review', bot: 'Reviewer', inputs: ['pr'], outputs: ['verdict'] }, - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['verdict'], outputs: [] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr'] }, + { id: 'review', name: 'Review', role: 'Reviewer', inputs: ['pr'], outputs: ['verdict'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict'], outputs: [] }, ], edges: [], deliverables: ['verdict'], @@ -243,8 +243,8 @@ test('a cycle that is not declared is refused; a declared loop is how work goes const loop = { inputs: ['text:topic'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic', 'verdict'], outputs: ['pr'] }, - { id: 'review', name: 'Review', bot: 'Reviewer', inputs: ['pr'], outputs: ['verdict'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic', 'verdict'], outputs: ['pr'] }, + { id: 'review', name: 'Review', role: 'Reviewer', inputs: ['pr'], outputs: ['verdict'] }, ], edges: [{ from: 'review.verdict', to: 'build.verdict' }], deliverables: ['pr'], @@ -261,8 +261,8 @@ test('a spelling the engine does not know, or an edge that does not line up, is const def = { inputs: ['text:topic'], nodes: [ - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, ], edges: [{ from: 'draft.text:draft', to: 'publish.link:published' }], deliverables: ['link:published'], @@ -277,8 +277,8 @@ test('a missing input refuses in the node s own words, and moves nothing', () => const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr'] }, - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['pr', 'check:tests-ok'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {spec} has not been produced yet.' }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['pr', 'check:tests-ok'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {spec} has not been produced yet.' }, ], edges: [], deliverables: ['queue-entry'], @@ -293,11 +293,11 @@ test('a stale input refuses with both revisions, so a card reads without asking const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, { id: 'gate', name: 'Gate', - bot: 'Reviewer', + role: 'Reviewer', inputs: ['verdict', 'check:tests-ok'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {input} is stale, the tip is now {now}.', @@ -346,11 +346,11 @@ test('a guard is a predicate over a fact, and its refusal says what the fact sai const def = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, { id: 'gate', name: 'Gate', - bot: 'Reviewer', + role: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'], guards: [{ id: 'pass', spec: 'verdict', is: 'equals', value: 'PASS' }], @@ -383,15 +383,15 @@ test('the ids are data: renaming them changes nothing but the strings', () => { assert.equal(graph.edges.length, 2) const blankGraph = layoutWorkflow(blankWorkflow) assert.deepEqual(graph.boxes.map((box) => box.rank), blankGraph.boxes.map((box) => box.rank)) - assert.equal(nodeById(renamed, 'step-1').bot, 'Editor') + assert.equal(nodeById(renamed, 'step-1').role, 'Editor') }) test('a fact whose source revision is unreadable refuses, whether the key is missing or null (#1291 INC)', () => { const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'] }, ], edges: [], deliverables: ['queue-entry'], @@ -413,9 +413,9 @@ test('the unreadable sentence names the fact it read and never fills the source const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, // The node's own wording speaks of "stale"; the engine must not use it for an unread source. - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {input} is stale, the tip is now {now}.' }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {input} is stale, the tip is now {now}.' }, ], edges: [], deliverables: ['queue-entry'], @@ -432,8 +432,8 @@ test('the card and the gate say the same thing about an unreadable input (#1291 const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['verdict', 'check:tests-ok'], outputs: ['queue-entry'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict', 'check:tests-ok'], outputs: ['queue-entry'] }, ], edges: [], deliverables: ['queue-entry'], diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 811ffff3b..c8ec6a104 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -53,8 +53,10 @@ 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:`, 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::` 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 `bot` and typed `inputs` / `outputs`), `edges` (`. -> .`) 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 Bot, a Bot not on the team, an unknown artifact kind, a misaligned edge); `workflowColumns` is the board's columns in dependency order, so the node ids are the state and no built-in list exists; `layoutWorkflow` is the flowchart (one box per node carrying its Bot 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. `holderOf` is the node's Bot, which is the card's assignee. Tests: `core/test/workflow.test.js`. | -| `core/src/shared/workflow-blank.js` | The `blank` preset (#1272): three nodes (`draft` / `review` / `publish`, Bots `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.js` | The workflow engine's pure half (#1272): a workflow is data — `nodes` (each with a required `role` and typed `inputs` / `outputs`), `edges` (`. -> .`) 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-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`. | | `core/src/shared/remote-label.js` | Owner-badge labels for a friend's Bot and “Shared room from …” titles. `friendBotTitle(bot)` / `friendBotOwner(bot)` name a friend's chat from the roster (`Chief (SelfB1233)`) and its owner (`SelfB1233`, the person an approval card names) with one formatter for every client surface (#498); `friendRecord` reads a published roster record (`roster.remoteBots`, name/owner/home at the top level) as well as a chat's `remote`, so the name does not change when the chat appears (#967). `remoteMentionLabel(record, others)` is the one name a mention wears — that title, and on a title two different handles share the home too (`Chief (Bob) · bob2.home`), so the menu, the bubble, the preview and the host's match cannot drift apart (#967). `remoteBotName` / `isAutoOwnerName` / `seatOwnerName` keep group and face-room labels in that format too (#526): Bot Social's ` · ` member label is not a Bot name, and an owner name made up from an install id (`FlyBot 682b8a43`) gives way to the username in the home handle (`Reviewer (flybot682b8a43)`). `isListedRemoteChat` / `listedChatBots` keep gone, hidden, and home-level (`@something.home`) remotes out of the **发给** picker, mentions, and `list_bots` friends lines: a share that ended is not a place to send to. The sidebar asks a different question through `isShownRemoteChat` (#956): a chat whose Bot left the published list keeps its row, marked *no longer shared*, while only a hide the user set (`bot.hidden`) takes a row out. History stays on disk either way. | ## Invariants @@ -63,17 +65,41 @@ Add a new invariant under the heading it belongs to (or a new heading), not at t ### A workflow is data (#1272) -A workflow definition — node ids, the Bot on each node, the typed artifacts each one takes in and +A workflow definition — node ids, the role on each node, the typed artifacts each one takes in and hands out, the edges and any declared loop — is read from the file and from nothing else: `core/src/shared/workflow.js` special-cases no node id, and `core/src/shared/artifact.js` names no -producer. A five-node content workflow and the `blank` preset pass through the same validation, -the same column order, the same layout and the same input check. +producer. A five-node content workflow, the `blank` preset and the `repo-dev` preset pass through +the same validation, the same column order, the same layout and the same input check. A node refuses to start on an input that is missing **or stale**, and the sentence names the revision the node read and the one the source holds now; the engine never decides a fact, and a fact it could not read refuses instead of passing. Validation and refusal live in the pure half, so a flowchart can be asserted against a definition with no browser, no network and no clock. +Two rules keep that from drifting: + +- **One word for one thing.** A node names a `role`; there is no second spelling for it, and no + definition can state both and disagree. A guard's `is` is likewise kept as written — an unknown + 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. +- **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 + board, which is how `review` is a pull request's state rather than a third lane (#1265), and a + definition that states no column keeps the node id, so nothing that already read these columns + moves. + +Refusal sentences are not re-typed per surface. The three the merge gate emits for a held pull +request, a moved head and a conclusion that is not a PASS are written once in +[`.github/scripts/fixtures/workflow-cases.json`](../../.github/scripts/fixtures/workflow-cases.json); +`merge-gate.mjs`'s `judge` (which `enqueue-guard.mjs` calls, and whose joined reasons it prints) and +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). + ### Default harness and usage ledger (#903) Step 0 of #874 Option B is behavior-neutral: one declarative harness and five usage fields that every `calls` row must carry. diff --git a/patches/ds-bot/files/src/host/workflow-facts.js b/patches/ds-bot/files/src/host/workflow-facts.js new file mode 100644 index 000000000..3f3370366 --- /dev/null +++ b/patches/ds-bot/files/src/host/workflow-facts.js @@ -0,0 +1,116 @@ +// What reads a fact (#1272 step 2, acceptance 10): one function per fact a guard asks about. A guard +// is a predicate over facts somebody else read, so every function here takes the answer a reader +// already got and turns it into one artifact — the fact's spec, the revision it was read at, and the +// value the node asked for. Nothing here fetches: the reader is the caller's, which is what keeps this +// module pure and what lets a test drive it with a recorded answer. +// +// The rule that matters: **no token, no answer, or an answer missing the field means `unreadable`**, +// never a guess. A fact nobody could read must refuse the node it feeds (`artifact.js` `freshness`, +// `workflow.js` `enterNode`), because "I did not manage to look" and "it is current" are not the same +// answer and only one of them may move work forward. + +import { artifact } from '../shared/artifact.js' + +// Every fact this module can answer, as `grep`-able names: a provider that is not here is one nobody +// wrote yet, and `workflowFact` says so rather than returning an empty artifact. +export const FACT_NAMES = Object.freeze([ + 'github.labels', + 'github.checkAtSha', + 'github.verdictAtTip', + 'github.mergeable', + 'github.prBodyTip', + 'task.progress', + 'manual.tick', +]) + +// The answer a caller gets when the fact could not be read: `ok: false` and the reason, with no +// `input`, so a guard cannot mistake it for a fact that happened to be empty. +export function unreadable(fact, why) { + return { fact, ok: false, reason: 'unreadable', why, input: null } +} + +// A fact that was read: one artifact and the same `ok` the caller checks. +const read = (fact, input) => (input ? { fact, ok: true, reason: 'read', why: null, input } : unreadable(fact, 'the answer carried no revision')) + +// GitHub reads need the token the host holds. Without it nothing was read, and a fact filled in from +// "the pull request probably has no hold label" is the one thing that would let a held card pass. +const hasToken = (token) => typeof token === 'string' && token.trim() !== '' + +const labelName = (label) => (typeof label === 'string' ? label : label?.name ?? null) + +// `pr` at the head the labels were read at, valued `hold` when the pull request carries that label +// and `none` otherwise. The gate's first guard is `equals: 'none'`. +export function githubLabels({ token, head, labels } = {}) { + if (!hasToken(token)) return unreadable('github.labels', 'no GITHUB_TOKEN') + if (!head || !Array.isArray(labels)) return unreadable('github.labels', 'the pull request labels were not read') + const names = labels.map(labelName).filter(Boolean).map((name) => String(name).toLowerCase()) + return read('github.labels', artifact('pr', head, { value: names.includes('hold') ? 'hold' : 'none' })) +} + +// `check:` 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. +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 })) +} + +// `verdict` at the tip the conclusion names, valued PASS / INC / FAIL / REQUEST_CHANGES and carrying +// the Bot that wrote it, which is what says who holds the gate. +export function githubVerdictAtTip({ token, tip, verdict, by } = {}) { + if (!hasToken(token)) return unreadable('github.verdictAtTip', 'no GITHUB_TOKEN') + if (!tip || !verdict) return unreadable('github.verdictAtTip', 'the newest conclusion at this tip was not read') + return read('github.verdictAtTip', artifact('verdict', tip, { value: verdict, by: by ?? null })) +} + +// `mergeable` against the tip it was computed on: `mergeable` / `conflict` / `unknown`. GitHub's own +// answer for a tip it has not computed yet is `unknown`, and `unknown` is not `mergeable`. +export function githubMergeable({ token, head, mergeable } = {}) { + if (!hasToken(token)) return unreadable('github.mergeable', 'no GITHUB_TOKEN') + if (!head || !mergeable) return unreadable('github.mergeable', 'the mergeable answer was not read') + return read('github.mergeable', artifact('mergeable', head, { value: mergeable })) +} + +// The `Tip SHA:` line of the pull request body, as the tip it names: this is the fact a checkpoint is +// read through, and a body with no such line is unreadable, not an empty tip. +export function githubPrBodyTip({ token, body } = {}) { + if (!hasToken(token)) return unreadable('github.prBodyTip', 'no GITHUB_TOKEN') + if (typeof body !== 'string') return unreadable('github.prBodyTip', 'the pull request body was not read') + const found = /^\s*Tip SHA:\s*([0-9a-f]{7,40})\s*$/im.exec(body) + if (!found) return unreadable('github.prBodyTip', 'the body carries no Tip SHA line') + return read('github.prBodyTip', artifact('text:pr-body-tip', found[1], { value: found[1].toLowerCase() })) +} + +// A card's own progress, 0–100, as a text fact: the revision is the number itself, so a card that has +// not moved is the same revision and a card that moved is a different one. +export function taskProgress({ card } = {}) { + if (!card || typeof card !== 'object') return unreadable('task.progress', 'the card was not read') + const progress = Number.isInteger(card.progress) ? card.progress : null + if (progress === null || progress < 0 || progress > 100) return unreadable('task.progress', 'the card carries no progress between 0 and 100') + return read('task.progress', artifact('text:card-progress', String(progress), { value: progress })) +} + +// A tick a node recorded itself — the one fact nothing outside read. It still needs a revision: the +// instant the node recorded, so a tick from before the work is not the tick that says it is done. +export function manualTick({ at, by } = {}) { + if (!at) return unreadable('manual.tick', 'the node recorded no instant') + return read('manual.tick', artifact('text:tick', at, { value: by ?? null })) +} + +export const FACT_PROVIDERS = Object.freeze({ + 'github.labels': githubLabels, + 'github.checkAtSha': githubCheckAtSha, + 'github.verdictAtTip': githubVerdictAtTip, + 'github.mergeable': githubMergeable, + 'github.prBodyTip': githubPrBodyTip, + 'task.progress': taskProgress, + 'manual.tick': manualTick, +}) + +// One spelling for a caller that has a fact's name as data (a guard's `spec`, a provider list): a name +// nobody wrote a function for is unreadable, with the name in the reason. +export function workflowFact(name, args = {}) { + const provider = FACT_PROVIDERS[name] + if (!provider) return unreadable(String(name ?? ''), 'no provider answers this fact') + return provider(args) +} diff --git a/patches/ds-bot/files/src/shared/workflow-blank.js b/patches/ds-bot/files/src/shared/workflow-blank.js index b6f4f31f3..2f7fec35d 100644 --- a/patches/ds-bot/files/src/shared/workflow-blank.js +++ b/patches/ds-bot/files/src/shared/workflow-blank.js @@ -1,4 +1,4 @@ -// The `blank` preset (#1272): three nodes, one Bot each, no guards, no repository. It is here as +// The `blank` preset (#1272): three nodes, one role each, no guards, no repository. It is here as // proof that a workflow is data — nothing in src/shared/workflow.js names these ids, and the same // JSON dropped in the workspace as `workflows/blank.json` is the whole definition. @@ -12,7 +12,7 @@ export const blankWorkflow = Object.freeze({ { id: 'draft', name: 'Draft', - bot: 'Writer', + role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'], guards: [], @@ -21,7 +21,7 @@ export const blankWorkflow = Object.freeze({ { id: 'review', name: 'Review', - bot: 'Editor', + role: 'Editor', inputs: ['text:draft'], outputs: ['text:edited'], guards: [], @@ -30,7 +30,7 @@ export const blankWorkflow = Object.freeze({ { id: 'publish', name: 'Publish', - bot: 'Publisher', + role: 'Publisher', inputs: ['text:edited'], outputs: ['link:published'], guards: [], @@ -44,5 +44,5 @@ export const blankWorkflow = Object.freeze({ loops: [], }) -// The Bots the preset names, so a caller can pass them as the team without inventing a roster. +// The roles the preset names, so a caller can pass them as the team without inventing a roster. export const BLANK_TEAM = Object.freeze(['Writer', 'Editor', 'Publisher']) diff --git a/patches/ds-bot/files/src/shared/workflow-repo-dev.js b/patches/ds-bot/files/src/shared/workflow-repo-dev.js new file mode 100644 index 000000000..492c7de65 --- /dev/null +++ b/patches/ds-bot/files/src/shared/workflow-repo-dev.js @@ -0,0 +1,152 @@ +// The `repo-dev` preset (#1272 step 2): one repository change, from a ready issue to a release, as +// data. Every node names the role that does it, the typed facts it takes in and hands out, the guards +// that must hold before it starts, and the sentence it refuses with. No node id and no role is +// special-cased in `workflow.js`; the same calls run this and the `blank` preset. +// +// Two of the roles are not one Bot on a roster, and they say so in the form the engine can read +// rather than with a placeholder: +// - `verify` is `SelfTest` **and** CI. The Bot runs the suite; the CI run is the `check:tests-ok` +// fact this node hands out, and the node refuses until both the self-test and that fact exist. +// - `gate` is held by *the Reviewer that judged the pull request*, which is the `by` of the verdict +// this node takes in — not a fixed assignee. The role is `Reviewer` and the guards read the +// verdict's own `Written-by`, so the card cannot be held by someone who did not write it. +// +// `column` is which board column a card in this node sits in (#1265). `review` carries `false`: a +// review is a pull request's state, not a third lane of its own, so the node stays in the definition +// and draws no column on the board. + +export const repoDevWorkflow = Object.freeze({ + id: 'repo-dev', + name: 'Repository change', + version: 1, + inputs: ['issue'], + deliverables: ['release'], + nodes: [ + { + id: 'triage', + name: 'Triage', + role: 'Triage', + column: 'not-started', + inputs: ['issue'], + outputs: ['issue.ready'], + guards: [], + refusal: '{node} cannot start: it needs {spec} and has not read it yet.', + }, + { + id: 'build', + name: 'Build', + role: 'Dev', + column: 'in-progress', + inputs: ['issue.ready'], + outputs: ['pr'], + guards: [], + refusal: '{node} cannot start: its input {input} is not ready.', + }, + { + id: 'verify', + name: 'Verify — SelfTest and CI', + role: 'SelfTest', + column: 'in-progress', + inputs: ['pr'], + outputs: ['check:tests-ok'], + guards: [], + refusal: '{node} cannot start: it needs {spec} and has not run it yet.', + }, + { + id: 'review', + name: 'Review', + role: 'Reviewer', + // A review is the pull request's own state, not a column of the repository board (#1265). + column: false, + inputs: ['pr', 'check:tests-ok'], + outputs: ['verdict'], + guards: [ + { + id: 'tests-ran-at-tip', + spec: 'check:tests-ok', + is: 'fresh', + refusal: '{node} refuses: {input} did not run at the tip {now}, so there is nothing to review.', + }, + ], + refusal: '{node} cannot start: its input {input} is not current.', + }, + { + id: 'gate', + name: 'Gate', + role: 'Reviewer', + column: 'in-progress', + // The verdict is not an input the node waits for: it is the fact the guards check, which is what + // says whether the pull request may go on at all. Its own revision is what the head guard reads. + inputs: ['pr', 'check:tests-ok'], + outputs: ['risk:tier'], + guards: [ + { + id: 'no-hold', + spec: 'pr', + is: 'equals', + value: 'none', + // The merge gate's own sentence for a held pull request (`merge-gate.mjs` `judge`), pinned + // in `.github/scripts/fixtures/workflow-cases.json` and asserted byte for byte. + refusal: 'the pull request carries the `hold` label', + }, + { + id: 'verdict-at-tip', + spec: 'verdict', + is: 'fresh', + // …and its sentence for a head that is not the one the caller asked about. + refusal: 'the pull request head is {now}, not the expected {have}', + }, + { + id: 'verdict-says-pass', + spec: 'verdict', + is: 'equals', + value: 'PASS', + // …and its sentence for a conclusion that says something else, naming who said it. + refusal: '{by} says {value} at this tip', + }, + { + id: 'tests-ran-at-tip', + spec: 'check:tests-ok', + is: 'fresh', + refusal: '{node} refuses: {input} did not run at the tip {now}.', + }, + ], + refusal: '{node} cannot start: its input {input} is not current.', + }, + { + id: 'merge', + name: 'Merge', + // The merge queue writes the commit, so no Bot holds this node. + role: 'system', + column: 'done', + inputs: ['verdict', 'risk:tier'], + outputs: ['merge-commit'], + guards: [], + refusal: '{node} cannot start: its input {input} is not current.', + }, + { + id: 'release', + name: 'Release', + role: 'Release', + column: 'done', + inputs: ['merge-commit'], + outputs: ['release'], + guards: [], + refusal: '{node} cannot start: it needs {spec} and has not published it yet.', + }, + ], + edges: [ + { from: 'triage.issue.ready', to: 'build.issue.ready' }, + { from: 'build.pr', to: 'verify.pr' }, + { from: 'verify.check:tests-ok', to: 'review.check:tests-ok' }, + { from: 'review.verdict', to: 'merge.verdict' }, + { from: 'gate.risk:tier', to: 'merge.risk:tier' }, + { from: 'merge.merge-commit', to: 'release.merge-commit' }, + ], + // No declared loop yet: an INC sends the card back to `build` on the board, and a `loops[]` row is + // a dependency the graph already has (declaring one nothing implies is `loop/not-an-edge`). + loops: [], +}) + +// The roles the preset names, so a caller can pass them as the team without inventing a roster. +export const REPO_DEV_TEAM = Object.freeze(['Triage', 'Dev', 'SelfTest', 'Reviewer', 'Release']) diff --git a/patches/ds-bot/files/src/shared/workflow.js b/patches/ds-bot/files/src/shared/workflow.js index 1d839e456..3535afabc 100644 --- a/patches/ds-bot/files/src/shared/workflow.js +++ b/patches/ds-bot/files/src/shared/workflow.js @@ -10,28 +10,45 @@ export const WORKFLOW_VERSION = 1 const text = (value) => (typeof value === 'string' ? value.trim() : '') const textList = (value) => (Array.isArray(value) ? value.map(text).filter(Boolean) : []) -const GUARD_CHECKS = ['present', 'fresh', 'equals'] +export const GUARD_CHECKS = ['present', 'fresh', 'equals'] +export const NODE_COLUMNS = ['not-started', 'in-progress', 'done'] function normaliseGuard(raw) { const spec = text(raw?.spec) - const is = text(raw?.is) return { id: text(raw?.id) || spec, spec, - is: GUARD_CHECKS.includes(is) ? is : 'present', + // The check is kept exactly as written. An unknown one used to be read as `present`, which is a + // guard that can never refuse: `is: 'fres'` left the node unguarded and said nothing. Validation + // refuses it, and `guardRefusals` refuses it too, so a file that never went through validation + // still cannot pass on a check nobody implemented (#1291 debt). + is: text(raw?.is), value: raw?.value === undefined ? null : raw.value, + // One node can refuse several facts for different reasons, so a guard carries its own sentence + // when it needs one; `renderRefusal` prefers it over the node's. + refusal: text(raw?.refusal), } } +// `column: false` draws no column; a name is the board column this node's cards sit in; absent is +// "not stated", and then the node id is the column — what step 1 did (#1265, #1272 step 2). +function normaliseColumn(value) { + if (value === false) return false + return text(value) === '' ? null : text(value) +} + function normaliseNode(raw) { return { id: text(raw?.id), name: text(raw?.name) || text(raw?.id), - bot: text(raw?.bot), + // One word for one thing: a node names the `role` that does it. The older spelling `bot` meant the + // same thing and is not read any more, so a definition cannot state both and disagree (#1291 debt). + role: text(raw?.role), inputs: textList(raw?.inputs), outputs: textList(raw?.outputs), guards: Array.isArray(raw?.guards) ? raw.guards.map(normaliseGuard) : [], refusal: text(raw?.refusal), + column: normaliseColumn(raw?.column), } } @@ -74,13 +91,21 @@ export function splitEndpoint(endpoint, nodeIds) { return null } +// The spelling a fact is keyed by: `text: draft` and `text:draft` are one fact, not two (#1291 +// debt). Anything outside the vocabulary is not a spec and has no key. +export function canonicalSpec(spec) { + const parsed = parseArtifactSpec(spec) + return parsed ? formatArtifactSpec(parsed) : null +} + export function producersOf(def) { const producers = {} for (const node of def.nodes) { for (const spec of node.outputs) { - if (!parseArtifactSpec(spec)) continue - producers[spec] = producers[spec] ?? [] - if (!producers[spec].includes(node.id)) producers[spec].push(node.id) + const key = canonicalSpec(spec) + if (!key) continue + producers[key] = producers[key] ?? [] + if (!producers[key].includes(node.id)) producers[key].push(node.id) } } return producers @@ -90,9 +115,10 @@ export function consumersOf(def) { const consumers = {} for (const node of def.nodes) { for (const spec of node.inputs) { - if (!parseArtifactSpec(spec)) continue - consumers[spec] = consumers[spec] ?? [] - if (!consumers[spec].includes(node.id)) consumers[spec].push(node.id) + const key = canonicalSpec(spec) + if (!key) continue + consumers[key] = consumers[key] ?? [] + if (!consumers[key].includes(node.id)) consumers[key].push(node.id) } } return consumers @@ -111,8 +137,9 @@ export function resolvedEdges(raw) { to: edge.to, fromNode: from?.node ?? null, toNode: to?.node ?? null, + // One field for one value (#1291 debt): the artifact the edge carries. `carries` said the same + // thing twice, and a reader could never tell which one a change was supposed to move. artifact: from?.artifact ?? null, - carries: from?.artifact ?? null, } }) } @@ -137,7 +164,7 @@ export function dependencyEdges(raw) { } for (const node of def.nodes) { for (const spec of node.inputs) { - for (const from of producers[spec] ?? []) { + for (const from of producers[canonicalSpec(spec)] ?? []) { if (!isLoopEdge(def, from, node.id)) add(from, node.id) } } @@ -181,24 +208,30 @@ export function topologicalOrder(raw) { // state, and there is no built-in list of them anywhere in this file. export function workflowColumns(raw) { const def = normaliseWorkflow(raw) - return topologicalOrder(def).map((id) => { - const node = nodeById(def, id) - return { + return topologicalOrder(def) + .map((id) => nodeById(def, id)) + // `column: false` draws no column (#1265): a review is a pull request's state, not a lane of its + // own, so the node stays in the definition and out of the board. + .filter((node) => node.column !== false) + .map((node) => ({ id: node.id, + // The board column this node's cards sit in. A definition that states none keeps step 1's + // answer — the node id itself — so nothing that already read these columns moves. + column: node.column ?? node.id, name: node.name, - bot: node.bot, + role: node.role, inputs: node.inputs.slice(), outputs: node.outputs.slice(), guards: node.guards.map((guard) => ({ ...guard })), refusal: node.refusal, - } - }) + })) } // Who does this node: the definition's Bot, which is also the card's assignee while the card sits // there, so the two cannot disagree. export function holderOf(raw, nodeId) { - return nodeById(normaliseWorkflow(raw), nodeId)?.bot ?? null + const role = nodeById(normaliseWorkflow(raw), nodeId)?.role ?? '' + return role === '' ? null : role } function ranksOf(def, edges) { @@ -240,7 +273,7 @@ export function layoutWorkflow(raw, options = {}) { boxes.push({ id: node.id, name: node.name, - bot: node.bot, + role: node.role, rank: r, row, inputs: node.inputs.slice(), @@ -295,13 +328,25 @@ export function renderRefusal(node, details = {}) { // A revision that could not be read is never written as an empty slot, and never as `null`. now: unreadable ? 'an unreadable revision' : (details.now ?? 'no revision'), want: String(details.want ?? ''), + // What the fact said, and who wrote it: the sentences the merge gate emits name both, so a preset + // that has to say the same thing can be written in one template (#1272 step 2, acceptance 9). + value: details.input?.value ?? '', + by: details.input?.by ?? '', + } + // A check the engine does not implement is a defect in the definition, not a fact: it refuses with + // its own sentence instead of being read as `present` (#1291 debt). + if (details.unchecked !== undefined) { + return `${name} refuses: its check on ${spec ?? 'a fact'} asks "${details.unchecked}", which is not one of ${GUARD_CHECKS.join(', ')}.` } if (unreadable) { // The node's own wording knows "has not been produced" and "is stale"; neither is this answer, // so the engine names what it read and that the source could not be read at all. return `${name} refuses: its input ${input ?? spec} could not be checked against the source — it reads ${details.have ?? 'no revision'} and the source revision is unreadable, so the card must be read before it starts.` } - if (node?.refusal) return fill(node.refusal, values) + // The guard's own sentence wins over the node's: one node refuses several facts, and the sentence + // that fits a held pull request does not fit a verdict that says INC. + const template = details.guard?.refusal || node?.refusal + if (template) return fill(template, values) if (details.want !== undefined && details.want !== null) { return `${name} refuses: its input ${input ?? spec} does not say ${details.want}.` } @@ -339,23 +384,31 @@ function guardRefusals(node, facts) { const current = facts.current ?? {} const refusals = [] for (const guard of node.guards) { - const spec = parseArtifactSpec(guard.spec) ? formatArtifactSpec(parseArtifactSpec(guard.spec)) : null + const spec = canonicalSpec(guard.spec) if (!spec) { refusals.push({ guard, refusal: renderRefusal(node, { spec: guard.spec }) }) continue } + // Read before the fact: an unknown check is a defect in the file, and a reader has to be told + // that rather than shown a refusal about a fact that was never looked at. + if (!GUARD_CHECKS.includes(guard.is)) { + refusals.push({ guard, refusal: renderRefusal(node, { spec, guard, unchecked: guard.is }) }) + continue + } const have = artifactAt(present, spec) const state = freshness(have, 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. refusals.push({ guard, refusal: renderRefusal(node, { spec }) }) continue } if (guard.is === 'fresh' && !state.fresh) { - refusals.push({ guard, refusal: renderRefusal(node, { spec, input: have, stale: true, have: state.have, now: state.now }) }) + refusals.push({ guard, refusal: renderRefusal(node, { spec, input: have, guard, stale: true, have: state.have, now: state.now }) }) continue } if (guard.is === 'equals' && String(have.value ?? '') !== String(guard.value ?? '')) { - refusals.push({ guard, refusal: renderRefusal(node, { spec, input: have, want: guard.value }) }) + refusals.push({ guard, refusal: renderRefusal(node, { spec, input: have, guard, have: have.revision, want: guard.value }) }) } } return refusals @@ -434,13 +487,35 @@ export function validateWorkflow(raw, options = {}) { problems.push(message('node/duplicate-id', `two nodes share the id "${node.id}"`, { node: node.id })) } ids.add(node.id) - if (node.bot === '') { - problems.push(message('node/without-bot', `node "${node.id}" names no Bot; every node is done by one`, { node: node.id })) - } else if (team && !team.includes(node.bot) && node.bot !== 'system' && !node.bot.startsWith('human:')) { + if (node.role === '') { + problems.push(message('node/without-role', `node "${node.id}" names no role; every node is done by one`, { node: node.id })) + } else if (team && !team.includes(node.role) && node.role !== 'system' && !node.role.startsWith('human:')) { problems.push( - message('node/unknown-bot', `node "${node.id}" names "${node.bot}", who is not on this team`, { node: node.id }), + message('node/unknown-role', `node "${node.id}" names "${node.role}", who is not on this team`, { node: node.id }), ) } + if (node.column !== null && node.column !== false && !NODE_COLUMNS.includes(node.column)) { + problems.push( + message( + 'node/unknown-column', + `node "${node.id}" names the column "${node.column}", which is not one of ${NODE_COLUMNS.join(', ')}`, + { node: node.id }, + ), + ) + } + for (const guard of node.guards) { + // #1291 debt: an unknown check is refused, not read as `present`. The two spellings are named + // so the fix reads as "pick one of these" rather than "something about the guard". + if (!GUARD_CHECKS.includes(guard.is)) { + problems.push( + message( + 'guard/unknown-check', + `node "${node.id}" guard "${guard.id}" asks is: "${guard.is}", which is not one of ${GUARD_CHECKS.join(', ')}`, + { node: node.id, guard: guard.id }, + ), + ) + } + } for (const spec of [...node.inputs, ...node.outputs]) { if (!parseArtifactSpec(spec)) { problems.push( @@ -454,14 +529,18 @@ export function validateWorkflow(raw, options = {}) { const producers = producersOf(def) const consumers = consumersOf(def) - const declaredInputs = new Set(def.inputs) - const deliverables = new Set(def.deliverables) + // `text: draft` and `text:draft` are one fact: the declared lists are read the same way the + // producers are keyed, or a workflow that spells one spelling would call its own input a defect. + const canonicalList = (list) => list.map(canonicalSpec).filter(Boolean) + const declaredInputs = new Set(canonicalList(def.inputs)) + const deliverables = new Set(canonicalList(def.deliverables)) for (const node of def.nodes) { if (!ids.has(node.id)) continue for (const spec of node.inputs) { - if (!parseArtifactSpec(spec)) continue - if ((producers[spec] ?? []).length === 0 && !declaredInputs.has(spec)) { + const key = canonicalSpec(spec) + if (!key) continue + if ((producers[key] ?? []).length === 0 && !declaredInputs.has(key)) { problems.push( message('input/without-producer', `node "${node.id}" takes in "${spec}", but no node hands it out`, { node: node.id, @@ -471,8 +550,9 @@ export function validateWorkflow(raw, options = {}) { } } for (const spec of node.outputs) { - if (!parseArtifactSpec(spec)) continue - if ((consumers[spec] ?? []).length === 0 && !deliverables.has(spec)) { + const key = canonicalSpec(spec) + if (!key) continue + if ((consumers[key] ?? []).length === 0 && !deliverables.has(key)) { problems.push( message( 'output/without-consumer', diff --git a/patches/ds-bot/files/test/workflow-repo-dev.test.js b/patches/ds-bot/files/test/workflow-repo-dev.test.js new file mode 100644 index 000000000..51b5fd7cd --- /dev/null +++ b/patches/ds-bot/files/test/workflow-repo-dev.test.js @@ -0,0 +1,225 @@ +// The `repo-dev` preset and the fact providers (#1272 step 2). What this file pins: +// - the preset is data: seven nodes, the roles the team really has, and the columns it draws; +// - the four debts #1291 named each have a test of their own; +// - the gate's refusal sentences are the merge gate's own sentences, byte for byte, through +// `.github/scripts/fixtures/workflow-cases.json` — the file both sides read and neither imports; +// - a fact nobody could read is `unreadable`, never a guess. +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { test } from 'node:test' +import { fileURLToPath } from 'node:url' +import { artifact, artifactsBySpec } from '../src/shared/artifact.js' +import { + canonicalSpec, + consumersOf, + dependencyEdges, + enterNode, + GUARD_CHECKS, + holderOf, + inputStates, + layoutWorkflow, + nodeById, + producersOf, + resolvedEdges, + validateWorkflow, + workflowColumns, +} from '../src/shared/workflow.js' +import { blankWorkflow } from '../src/shared/workflow-blank.js' +import { REPO_DEV_TEAM, repoDevWorkflow } from '../src/shared/workflow-repo-dev.js' +import { FACT_NAMES, FACT_PROVIDERS, githubCheckAtSha, githubLabels, githubPrBodyTip, githubVerdictAtTip, manualTick, taskProgress, workflowFact } from '../src/host/workflow-facts.js' + +const cases = JSON.parse(readFileSync(fileURLToPath(new URL('../../.github/scripts/fixtures/workflow-cases.json', import.meta.url)), 'utf8')) + +const factsFor = (entry) => ({ + present: artifactsBySpec(entry.present.map((item) => artifact(item.spec, item.revision, { value: item.value, by: item.by }))), + current: entry.current, +}) + +test('the repo-dev preset is data, and it validates with the roles this team has', () => { + const result = validateWorkflow(repoDevWorkflow, { team: REPO_DEV_TEAM }) + assert.equal(result.ok, true, JSON.stringify(result.problems)) + assert.deepEqual(repoDevWorkflow.nodes.map((node) => node.id), ['triage', 'build', 'verify', 'review', 'gate', 'merge', 'release']) + assert.deepEqual(repoDevWorkflow.nodes.map((node) => node.role), ['Triage', 'Dev', 'SelfTest', 'Reviewer', 'Reviewer', 'system', 'Release']) + // The two roles that are not one Bot on a roster say what they are, not who: the self-test Bot with + // CI as the `check:tests-ok` fact it hands out, and the Reviewer that judged the verdict it reads. + assert.equal(nodeById(repoDevWorkflow, 'verify').name, 'Verify — SelfTest and CI') + assert.deepEqual(nodeById(repoDevWorkflow, 'verify').outputs, ['check:tests-ok']) + assert.equal(holderOf(repoDevWorkflow, 'gate'), 'Reviewer') + // The verdict is the fact the gate guards, not an input it waits for: its revision is what the head + // guard compares against the live tip. + assert.deepEqual(nodeById(repoDevWorkflow, 'gate').guards.map((guard) => guard.spec), ['pr', 'verdict', 'verdict', 'check:tests-ok']) + assert.deepEqual(nodeById(repoDevWorkflow, 'gate').inputs, ['pr', 'check:tests-ok']) + // The merge is the queue's, so no Bot is named for it. + assert.equal(holderOf(repoDevWorkflow, 'merge'), 'system') +}) + +test('the columns are the definition s, and a node that says column: false draws none (#1265)', () => { + const columns = workflowColumns(repoDevWorkflow) + // `review` is the pull request's state, not a third lane: it stays in the definition, out of the board. + assert.deepEqual(columns.map((column) => column.id), ['triage', 'build', 'verify', 'gate', 'merge', 'release']) + assert.deepEqual(columns.map((column) => column.column), ['not-started', 'in-progress', 'in-progress', 'in-progress', 'done', 'done']) + assert.deepEqual(columns.map((column) => column.role), ['Triage', 'Dev', 'SelfTest', 'Reviewer', 'system', 'Release']) + // The node is still there — it just is not a column — so `enterNode` can still refuse it. + assert.ok(nodeById(repoDevWorkflow, 'review')) + assert.equal(nodeById(repoDevWorkflow, 'review').column, false) + // The flowchart keeps every node: the board's columns and the picture are two questions. + assert.equal(layoutWorkflow(repoDevWorkflow).boxes.length, repoDevWorkflow.nodes.length) +}) + +test('a definition that states no column keeps step 1 s answer, the node id', () => { + const columns = workflowColumns(blankWorkflow) + assert.deepEqual(columns.map((column) => column.id), ['draft', 'review', 'publish']) + assert.deepEqual(columns.map((column) => column.column), ['draft', 'review', 'publish']) +}) + +test('a column the board does not have, and a guard check nobody implements, are each refused by name', () => { + const base = { + inputs: ['text:topic'], + nodes: [ + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + ], + deliverables: ['link:published'], + } + const wrongColumn = validateWorkflow({ ...base, nodes: [{ ...base.nodes[0], column: 'doing' }, base.nodes[1]] }, { team: ['Writer', 'Publisher'] }) + assert.deepEqual(wrongColumn.problems.map((problem) => problem.code), ['node/unknown-column']) + assert.match(wrongColumn.problems[0].message, /names the column "doing", which is not one of not-started, in-progress, done/) + + // #1291 debt: `is: 'fres'` used to be read as `present`, which is a guard that can never refuse. + const wrongCheck = validateWorkflow({ ...base, nodes: [{ ...base.nodes[0], guards: [{ id: 'pass', spec: 'verdict', is: 'fres', value: 'PASS' }] }, base.nodes[1]] }, { team: ['Writer', 'Publisher'] }) + assert.ok(wrongCheck.problems.some((problem) => problem.code === 'guard/unknown-check')) + assert.match(wrongCheck.problems[0].message, /guard "pass" asks is: "fres", which is not one of present, fresh, equals/) +}) + +test('a guard whose check nobody implements refuses instead of passing on a guess (#1291 debt)', () => { + const def = { + inputs: ['pr'], + nodes: [ + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'], guards: [{ id: 'pass', spec: 'verdict', is: 'fres', value: 'PASS' }] }, + ], + deliverables: ['queue-entry'], + } + const present = { verdict: artifact('verdict', 'abc1234', { value: 'INC' }) } + const refused = enterNode(def, 'gate', { present, current: { verdict: 'abc1234' } }) + // Before the fix this guard was dropped and the node started on an INC. + assert.equal(refused.ok, false) + assert.match(refused.refusal, /its check on verdict asks "fres", which is not one of present, fresh, equals/) + assert.deepEqual(GUARD_CHECKS, ['present', 'fresh', 'equals']) +}) + +test('a fact is keyed by its parsed spec, so "text: draft" and "text:draft" are one fact (#1291 debt)', () => { + const def = { + inputs: ['text:topic'], + nodes: [ + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text: draft'] }, + { id: 'edit', name: 'Edit', role: 'Editor', inputs: ['text:draft'], outputs: ['link:published'] }, + ], + deliverables: ['link:published'], + } + assert.equal(canonicalSpec('text: draft'), 'text:draft') + assert.deepEqual(Object.keys(producersOf(def)), ['text:draft', 'link:published']) + assert.deepEqual(Object.keys(consumersOf(def)), ['text:topic', 'text:draft']) + // The one producer is found through the canonical key, so the workflow is valid and the dependency + // exists: with the raw spelling it read "no producer" and "no consumer", and lost the edge. + assert.equal(validateWorkflow(def, { team: ['Writer', 'Editor'] }).ok, true, JSON.stringify(validateWorkflow(def, { team: ['Writer', 'Editor'] }).problems)) + assert.deepEqual(dependencyEdges(def), [{ from: 'draft', to: 'edit' }]) +}) + +test('one word for one thing: a node names a role, and the older spelling is not read (#1291 debt)', () => { + const def = { + inputs: ['text:topic'], + nodes: [ + { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['link:published'] }, + ], + deliverables: ['link:published'], + } + const result = validateWorkflow(def, { team: ['Writer'] }) + assert.equal(result.ok, false) + assert.deepEqual(result.problems.map((problem) => problem.code), ['node/without-role']) + assert.equal(holderOf(def, 'draft'), null) + // The two spellings cannot both be written and disagree, because only one of them is read at all. + assert.equal(validateWorkflow({ ...def, nodes: [{ ...def.nodes[0], role: 'Writer' }] }, { team: ['Writer'] }).ok, true) +}) + +test('an edge carries its artifact once, and says so (#1291 debt)', () => { + const edge = resolvedEdges(repoDevWorkflow)[0] + assert.deepEqual(Object.keys(edge).sort(), ['artifact', 'from', 'fromNode', 'to', 'toNode'].sort()) + assert.equal(edge.artifact, 'issue.ready') + assert.equal('carries' in edge, false) +}) + +test('the gate s refusal is the merge gate s own sentence, byte for byte (acceptance 9)', () => { + assert.ok(cases.cases.length >= 3) + for (const entry of cases.cases) { + const { present, current } = factsFor(entry.preset) + const refused = enterNode(repoDevWorkflow, entry.preset.node, { present, current }) + assert.equal(refused.ok, false, `${entry.id} must refuse`) + assert.equal(refused.refusal, entry.sentence, `${entry.id}: the preset and ${entry.where} must say the same thing`) + // And the guard that answered is the one the fixture names, so a sentence cannot pass by accident + // on some other guard that happens to read the same way. + assert.equal(refused.guardRefusals[0].guard.id, entry.preset.guard, `${entry.id} must be refused by ${entry.preset.guard}`) + } +}) + +test('the gate starts only when every fact it reads is present, current and says the right thing', () => { + const entry = cases.cases.find((item) => item.id === 'verdict-not-pass') + const { present, current } = factsFor(entry.preset) + const pass = { ...current, verdict: 'abaf938b0c1d' } + const presentPass = { ...present, verdict: artifact('verdict', 'abaf938b0c1d', { value: 'PASS', by: 'Reviewer' }) } + assert.equal(enterNode(repoDevWorkflow, 'gate', { present: presentPass, current: pass }).ok, true) + // A fact nobody could read refuses rather than passing (acceptance 10): the same node, same facts, + // and the verdict's revision is not readable. + const unread = enterNode(repoDevWorkflow, 'gate', { present: presentPass, current: { ...pass, verdict: null } }) + assert.equal(unread.ok, false) + assert.match(unread.refusal, /could not be checked against the source/) + // A guarded fact nobody could read is refused by the guard, and the card says the same thing: the + // input that is present reads unreadable instead of fine. + const unreadStates = inputStates(repoDevWorkflow, 'gate', { present: presentPass, current: { ...pass, 'check:tests-ok': null } }) + assert.equal(unreadStates.find((state) => state.spec === 'check:tests-ok').unreadable, true) + assert.equal(enterNode(repoDevWorkflow, 'gate', { present: presentPass, current: { ...pass, 'check:tests-ok': null } }).ok, false) +}) + +test('a fact nobody could read is unreadable, and no fact is guessed (acceptance 10)', () => { + assert.deepEqual([...FACT_NAMES].sort(), [...Object.keys(FACT_PROVIDERS)].sort()) + // Every provider answers `unreadable` when it has nothing to answer from: no token, no field. + const nothing = [ + githubLabels({ head: 'abc1234', labels: [] }), + githubCheckAtSha({ name: 'tests-ok', sha: 'abc1234' }), + githubVerdictAtTip({ tip: 'abc1234', verdict: 'PASS' }), + githubPrBodyTip({ body: 'Tip SHA: abc1234' }), + workflowFact('github.nobody'), + ] + for (const answer of [githubLabels({}), githubCheckAtSha({}), githubVerdictAtTip({}), githubMergeableNothing(), githubPrBodyTip({}), taskProgress({}), manualTick({}), ...nothing]) { + assert.equal(answer.ok, false, `${answer.fact} must not answer without a reading`) + assert.equal(answer.reason, 'unreadable') + assert.equal(answer.input, null) + assert.ok(answer.why, `${answer.fact} must say why it could not read`) + } + // With the token and the answer, each one is an artifact at the revision it was read at. + const labels = githubLabels({ token: 'x', head: 'abaf938b0c1d', labels: [{ name: 'hold' }] }) + assert.equal(labels.ok, true) + assert.equal(labels.input.spec, 'pr') + assert.equal(labels.input.value, 'hold') + assert.equal(githubLabels({ token: 'x', head: 'abaf938b0c1d', labels: [] }).input.value, 'none') + const check = githubCheckAtSha({ token: 'x', name: 'tests-ok', sha: 'abaf938b0c1d', conclusion: 'success' }) + assert.equal(check.input.spec, 'check:tests-ok') + assert.equal(check.input.revision, 'abaf938b0c1d') + const verdict = githubVerdictAtTip({ token: 'x', tip: 'abaf938b0c1d', verdict: 'INC', by: 'Reviewer' }) + assert.equal(verdict.input.spec, 'verdict') + assert.equal(verdict.input.value, 'INC') + assert.equal(verdict.input.by, 'Reviewer') + const body = githubPrBodyTip({ token: 'x', body: 'Written-by: Dev3\nTip SHA: abaf938b0c1d\n' }) + assert.equal(body.input.revision, 'abaf938b0c1d') + // A body with no Tip SHA line is unreadable, not an empty tip. + assert.equal(githubPrBodyTip({ token: 'x', body: 'no tip here' }).reason, 'unreadable') + const progress = taskProgress({ card: { progress: 85 } }) + assert.equal(progress.input.revision, '85') + assert.equal(taskProgress({ card: { progress: 140 } }).reason, 'unreadable') + assert.equal(manualTick({ at: '2026-10-26T12:00:00Z' }).input.spec, 'text:tick') +}) + +// `githubMergeable` with nothing to read: kept out of the list above so each call reads as itself. +function githubMergeableNothing() { + return workflowFact('github.mergeable', {}) +} diff --git a/patches/ds-bot/files/test/workflow.test.js b/patches/ds-bot/files/test/workflow.test.js index 9e20c2a8f..51f7f3ad7 100644 --- a/patches/ds-bot/files/test/workflow.test.js +++ b/patches/ds-bot/files/test/workflow.test.js @@ -33,11 +33,11 @@ const content = { inputs: ['text:topic'], deliverables: ['text:archived'], nodes: [ - { id: 'idea', name: 'Idea', bot: 'Planner', inputs: ['text:topic'], outputs: ['text:brief'] }, - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:brief'], outputs: ['text:draft'] }, - { id: 'edit', name: 'Edit', bot: 'Editor', inputs: ['text:draft'], outputs: ['text:edited'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:edited'], outputs: ['link:published'] }, - { id: 'archive', name: 'Archive', bot: 'Archivist', inputs: ['link:published'], outputs: ['text:archived'] }, + { id: 'idea', name: 'Idea', role: 'Planner', inputs: ['text:topic'], outputs: ['text:brief'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:brief'], outputs: ['text:draft'] }, + { id: 'edit', name: 'Edit', role: 'Editor', inputs: ['text:draft'], outputs: ['text:edited'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:edited'], outputs: ['link:published'] }, + { id: 'archive', name: 'Archive', role: 'Archivist', inputs: ['link:published'], outputs: ['text:archived'] }, ], edges: [ { from: 'idea.text:brief', to: 'draft.text:brief' }, @@ -98,16 +98,16 @@ test('the blank preset validates with its own team, and names a Bot on every nod const result = validateWorkflow(blankWorkflow, { team: BLANK_TEAM }) assert.equal(result.ok, true, JSON.stringify(result.problems)) assert.deepEqual(workflowColumns(blankWorkflow).map((column) => column.id), ['draft', 'review', 'publish']) - assert.deepEqual(workflowColumns(blankWorkflow).map((column) => column.bot), ['Writer', 'Editor', 'Publisher']) + assert.deepEqual(workflowColumns(blankWorkflow).map((column) => column.role), ['Writer', 'Editor', 'Publisher']) assert.equal(holderOf(blankWorkflow, 'review'), 'Editor') assert.equal(holderOf(blankWorkflow, 'nowhere'), null) const other = validateWorkflow(blankWorkflow, { team: ['Someone'] }) assert.equal(other.ok, false) assert.deepEqual(other.problems.map((problem) => problem.code), [ - 'node/unknown-bot', - 'node/unknown-bot', - 'node/unknown-bot', + 'node/unknown-role', + 'node/unknown-role', + 'node/unknown-role', ]) }) @@ -115,7 +115,7 @@ test('the columns are the definition s, not a built-in list', () => { const blank = workflowColumns(blankWorkflow) const editorial = workflowColumns(content) assert.deepEqual(editorial.map((column) => column.id), ['idea', 'draft', 'edit', 'publish', 'archive']) - assert.deepEqual(editorial.map((column) => column.bot), contentTeam) + assert.deepEqual(editorial.map((column) => column.role), contentTeam) assert.notDeepEqual(blank.map((column) => column.id), editorial.map((column) => column.id)) // Same calls, two vocabularies: nothing above this line knows either set of ids. assert.equal(validateWorkflow(content, { team: contentTeam }).ok, true, JSON.stringify(validateWorkflow(content, { team: contentTeam }).problems)) @@ -129,7 +129,7 @@ test('the flowchart is generated: one box per node, one edge per edges[] row', ( for (const node of content.nodes) { const box = graph.boxes.find((candidate) => candidate.id === node.id) assert.ok(box, `no box for ${node.id}`) - assert.equal(box.bot, node.bot) + assert.equal(box.role, node.role) assert.deepEqual(box.inputs, node.inputs) assert.deepEqual(box.outputs, node.outputs) } @@ -152,21 +152,21 @@ test('the flowchart is generated: one box per node, one edge per edges[] row', ( const broken = layoutWorkflow({ nodes: content.nodes, edges: [{ from: 'idea.text:brief', to: 'gone.text:brief' }] }) assert.equal(broken.edges.length, 1) assert.equal(broken.edges[0].toBox, null) + // One field for one value: the edge carries `artifact`, and says it once (#1291 debt). assert.deepEqual(resolvedEdges(content)[0], { from: 'idea.text:brief', to: 'draft.text:brief', fromNode: 'idea', toNode: 'draft', artifact: 'text:brief', - carries: 'text:brief', }) }) test('an input nothing produces is refused, unless the workflow declares it as a starting point', () => { const def = { nodes: [ - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, ], edges: [], deliverables: ['link:published'], @@ -182,8 +182,8 @@ test('an output nothing consumes is refused, unless it is a declared deliverable const def = { inputs: ['text:topic'], nodes: [ - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft', 'text:extra'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft', 'text:extra'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, ], deliverables: ['link:published'], } @@ -197,9 +197,9 @@ test('one fact has one producer, and two producers are refused by name', () => { const def = { inputs: ['text:topic'], nodes: [ - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'redraft', name: 'Redraft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'redraft', name: 'Redraft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, ], deliverables: ['link:published'], } @@ -208,31 +208,31 @@ test('one fact has one producer, and two producers are refused by name', () => { assert.match(result.problems[0].message, /"text:draft" is handed out by "draft" and "redraft"/) }) -test('a node without a Bot is refused, and so is a Bot that names nobody on the team', () => { +test('a node without a role is refused, and so is a role that names nobody on the team', () => { const def = { inputs: ['text:topic'], nodes: [ - { id: 'draft', name: 'Draft', bot: '', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'publish', name: 'Publish', bot: 'Ghost', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: '', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Ghost', inputs: ['text:draft'], outputs: ['link:published'] }, ], deliverables: ['link:published'], } const loose = validateWorkflow(def) - assert.deepEqual(loose.problems.map((problem) => problem.code), ['node/without-bot']) + assert.deepEqual(loose.problems.map((problem) => problem.code), ['node/without-role']) const strict = validateWorkflow(def, { team: ['Writer'] }) - assert.deepEqual(strict.problems.map((problem) => problem.code), ['node/without-bot', 'node/unknown-bot']) + assert.deepEqual(strict.problems.map((problem) => problem.code), ['node/without-role', 'node/unknown-role']) assert.match(strict.problems[1].message, /node "publish" names "Ghost", who is not on this team/) - // `system` and a human role are part of the model, not a missing Bot. - assert.equal(validateWorkflow({ inputs: ['text:topic'], nodes: [{ id: 'merge', name: 'Merge', bot: 'system', inputs: ['text:topic'], outputs: ['merge-commit'] }], deliverables: ['merge-commit'] }, { team: [] }).ok, true) + // `system` and a human role are part of the model, not a missing role. + assert.equal(validateWorkflow({ inputs: ['text:topic'], nodes: [{ id: 'merge', name: 'Merge', role: 'system', inputs: ['text:topic'], outputs: ['merge-commit'] }], deliverables: ['merge-commit'] }, { team: [] }).ok, true) }) test('a cycle that is not declared is refused; a declared loop is how work goes back', () => { const twoStep = { inputs: ['text:topic'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr'] }, - { id: 'review', name: 'Review', bot: 'Reviewer', inputs: ['pr'], outputs: ['verdict'] }, - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['verdict'], outputs: [] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr'] }, + { id: 'review', name: 'Review', role: 'Reviewer', inputs: ['pr'], outputs: ['verdict'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict'], outputs: [] }, ], edges: [], deliverables: ['verdict'], @@ -243,8 +243,8 @@ test('a cycle that is not declared is refused; a declared loop is how work goes const loop = { inputs: ['text:topic'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic', 'verdict'], outputs: ['pr'] }, - { id: 'review', name: 'Review', bot: 'Reviewer', inputs: ['pr'], outputs: ['verdict'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic', 'verdict'], outputs: ['pr'] }, + { id: 'review', name: 'Review', role: 'Reviewer', inputs: ['pr'], outputs: ['verdict'] }, ], edges: [{ from: 'review.verdict', to: 'build.verdict' }], deliverables: ['pr'], @@ -261,8 +261,8 @@ test('a spelling the engine does not know, or an edge that does not line up, is const def = { inputs: ['text:topic'], nodes: [ - { id: 'draft', name: 'Draft', bot: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, - { id: 'publish', name: 'Publish', bot: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + { id: 'draft', name: 'Draft', role: 'Writer', inputs: ['text:topic'], outputs: ['text:draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, ], edges: [{ from: 'draft.text:draft', to: 'publish.link:published' }], deliverables: ['link:published'], @@ -277,8 +277,8 @@ test('a missing input refuses in the node s own words, and moves nothing', () => const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr'] }, - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['pr', 'check:tests-ok'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {spec} has not been produced yet.' }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['pr', 'check:tests-ok'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {spec} has not been produced yet.' }, ], edges: [], deliverables: ['queue-entry'], @@ -293,11 +293,11 @@ test('a stale input refuses with both revisions, so a card reads without asking const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, { id: 'gate', name: 'Gate', - bot: 'Reviewer', + role: 'Reviewer', inputs: ['verdict', 'check:tests-ok'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {input} is stale, the tip is now {now}.', @@ -346,11 +346,11 @@ test('a guard is a predicate over a fact, and its refusal says what the fact sai const def = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, { id: 'gate', name: 'Gate', - bot: 'Reviewer', + role: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'], guards: [{ id: 'pass', spec: 'verdict', is: 'equals', value: 'PASS' }], @@ -383,15 +383,15 @@ test('the ids are data: renaming them changes nothing but the strings', () => { assert.equal(graph.edges.length, 2) const blankGraph = layoutWorkflow(blankWorkflow) assert.deepEqual(graph.boxes.map((box) => box.rank), blankGraph.boxes.map((box) => box.rank)) - assert.equal(nodeById(renamed, 'step-1').bot, 'Editor') + assert.equal(nodeById(renamed, 'step-1').role, 'Editor') }) test('a fact whose source revision is unreadable refuses, whether the key is missing or null (#1291 INC)', () => { const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'] }, ], edges: [], deliverables: ['queue-entry'], @@ -413,9 +413,9 @@ test('the unreadable sentence names the fact it read and never fills the source const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, // The node's own wording speaks of "stale"; the engine must not use it for an unread source. - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {input} is stale, the tip is now {now}.' }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict'], outputs: ['queue-entry'], refusal: '{node} refuses: its input {input} is stale, the tip is now {now}.' }, ], edges: [], deliverables: ['queue-entry'], @@ -432,8 +432,8 @@ test('the card and the gate say the same thing about an unreadable input (#1291 const gate = { inputs: ['pr'], nodes: [ - { id: 'build', name: 'Build', bot: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, - { id: 'gate', name: 'Gate', bot: 'Reviewer', inputs: ['verdict', 'check:tests-ok'], outputs: ['queue-entry'] }, + { id: 'build', name: 'Build', role: 'Dev', inputs: ['text:topic'], outputs: ['pr', 'verdict'] }, + { id: 'gate', name: 'Gate', role: 'Reviewer', inputs: ['verdict', 'check:tests-ok'], outputs: ['queue-entry'] }, ], edges: [], deliverables: ['queue-entry'],