diff --git a/changelog.d/2425-workflow-edge-canonical-spec.fixed.md b/changelog.d/2425-workflow-edge-canonical-spec.fixed.md new file mode 100644 index 000000000..c353f82b9 --- /dev/null +++ b/changelog.d/2425-workflow-edge-canonical-spec.fixed.md @@ -0,0 +1 @@ +A workflow whose node spells a fact with a space while the edge carries the canonical spelling no longer fails to save: the edge alignment check keys the node lists the same way the rest of the engine does, so `text: draft` on a node and `text:draft` on its edge are one fact. diff --git a/core/src/shared/workflow.js b/core/src/shared/workflow.js index 6bc1d6a8d..9457ceda5 100644 --- a/core/src/shared/workflow.js +++ b/core/src/shared/workflow.js @@ -634,7 +634,10 @@ export function validateWorkflow(raw, options = {}) { } const fromNode = nodeById(def, from.node) const toNode = nodeById(def, to.node) - if (!fromNode.outputs.includes(from.artifact) || !toNode.inputs.includes(to.artifact)) { + // The edge's artifact is canonical and the node lists are not (#2425): key them the way + // `producersOf` and `consumersOf` do, or a spelling difference refuses a definition the rest + // of the engine can run. + if (!canonicalList(fromNode.outputs).includes(from.artifact) || !canonicalList(toNode.inputs).includes(to.artifact)) { problems.push( message( 'edge/misaligned', diff --git a/core/test/workflow-edge-canonical-spec.test.js b/core/test/workflow-edge-canonical-spec.test.js new file mode 100644 index 000000000..4b4f17366 --- /dev/null +++ b/core/test/workflow-edge-canonical-spec.test.js @@ -0,0 +1,59 @@ +// One fact has one key (#1291 debt): `text: draft` and `text:draft` are the same fact, and every +// consumer/producer rule in the engine reads the node lists through `canonicalSpec`. The edge +// alignment check was the one place that compared the edge's canonical artifact against the raw +// lists, so a definition the engine can run was still refused (#2425). +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { artifact } from '../src/shared/artifact.js' +import { dependencyEdges, enterNode, validateWorkflow } from '../src/shared/workflow.js' + +const definition = (producerSpec, edgeFrom, edgeTo, consumerSpec) => ({ + id: 'draft-publish', + name: 'Draft and publish', + version: 1, + inputs: [], + deliverables: ['link:published'], + nodes: [ + { id: 'draft', name: 'Draft', role: 'Writer', outputs: [producerSpec] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: [consumerSpec], outputs: ['link:published'] }, + ], + edges: [{ from: edgeFrom, to: edgeTo }], + loops: [], +}) + +test('a space on the node and none on its edge is still one fact', () => { + const def = definition('text: draft', 'draft.text:draft', 'publish.text:draft', 'text:draft') + const result = validateWorkflow(def) + assert.deepEqual(result.problems, []) + assert.equal(result.ok, true) + assert.deepEqual(dependencyEdges(def), [{ from: 'draft', to: 'publish' }]) + const taken = enterNode(def, 'publish', { + present: { 'text:draft': artifact('text:draft', 'abc1234') }, + current: { 'text:draft': 'abc1234' }, + }) + assert.equal(taken.ok, true, taken.refusal) +}) + +test('a space on the consumer and none on the edge is still one fact', () => { + const def = definition('text:draft', 'draft.text:draft', 'publish.text:draft', 'text: draft') + const result = validateWorkflow(def) + assert.deepEqual(result.problems, []) + assert.equal(result.ok, true) +}) + +test('the same fact with the space moved to the edge is one fact too', () => { + const def = definition('text:draft', 'draft.text: draft', 'publish.text:draft', 'text:draft') + const result = validateWorkflow(def) + assert.deepEqual(result.problems, []) + assert.equal(result.ok, true) +}) + +test('an edge that carries a fact the consumer does not take in is still refused (control)', () => { + const def = definition('text:draft', 'draft.text:draft', 'publish.text:other', 'text:draft') + const result = validateWorkflow(def) + assert.equal(result.ok, false) + assert.ok( + result.problems.some((problem) => problem.code === 'edge/misaligned' || problem.code === 'edge/unknown-end'), + JSON.stringify(result.problems), + ) +}) diff --git a/patches/ds-bot/files/src/shared/workflow.js b/patches/ds-bot/files/src/shared/workflow.js index 6bc1d6a8d..9457ceda5 100644 --- a/patches/ds-bot/files/src/shared/workflow.js +++ b/patches/ds-bot/files/src/shared/workflow.js @@ -634,7 +634,10 @@ export function validateWorkflow(raw, options = {}) { } const fromNode = nodeById(def, from.node) const toNode = nodeById(def, to.node) - if (!fromNode.outputs.includes(from.artifact) || !toNode.inputs.includes(to.artifact)) { + // The edge's artifact is canonical and the node lists are not (#2425): key them the way + // `producersOf` and `consumersOf` do, or a spelling difference refuses a definition the rest + // of the engine can run. + if (!canonicalList(fromNode.outputs).includes(from.artifact) || !canonicalList(toNode.inputs).includes(to.artifact)) { problems.push( message( 'edge/misaligned', diff --git a/patches/ds-bot/files/test/workflow-edge-canonical-spec.test.js b/patches/ds-bot/files/test/workflow-edge-canonical-spec.test.js new file mode 100644 index 000000000..4b4f17366 --- /dev/null +++ b/patches/ds-bot/files/test/workflow-edge-canonical-spec.test.js @@ -0,0 +1,59 @@ +// One fact has one key (#1291 debt): `text: draft` and `text:draft` are the same fact, and every +// consumer/producer rule in the engine reads the node lists through `canonicalSpec`. The edge +// alignment check was the one place that compared the edge's canonical artifact against the raw +// lists, so a definition the engine can run was still refused (#2425). +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { artifact } from '../src/shared/artifact.js' +import { dependencyEdges, enterNode, validateWorkflow } from '../src/shared/workflow.js' + +const definition = (producerSpec, edgeFrom, edgeTo, consumerSpec) => ({ + id: 'draft-publish', + name: 'Draft and publish', + version: 1, + inputs: [], + deliverables: ['link:published'], + nodes: [ + { id: 'draft', name: 'Draft', role: 'Writer', outputs: [producerSpec] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: [consumerSpec], outputs: ['link:published'] }, + ], + edges: [{ from: edgeFrom, to: edgeTo }], + loops: [], +}) + +test('a space on the node and none on its edge is still one fact', () => { + const def = definition('text: draft', 'draft.text:draft', 'publish.text:draft', 'text:draft') + const result = validateWorkflow(def) + assert.deepEqual(result.problems, []) + assert.equal(result.ok, true) + assert.deepEqual(dependencyEdges(def), [{ from: 'draft', to: 'publish' }]) + const taken = enterNode(def, 'publish', { + present: { 'text:draft': artifact('text:draft', 'abc1234') }, + current: { 'text:draft': 'abc1234' }, + }) + assert.equal(taken.ok, true, taken.refusal) +}) + +test('a space on the consumer and none on the edge is still one fact', () => { + const def = definition('text:draft', 'draft.text:draft', 'publish.text:draft', 'text: draft') + const result = validateWorkflow(def) + assert.deepEqual(result.problems, []) + assert.equal(result.ok, true) +}) + +test('the same fact with the space moved to the edge is one fact too', () => { + const def = definition('text:draft', 'draft.text: draft', 'publish.text:draft', 'text:draft') + const result = validateWorkflow(def) + assert.deepEqual(result.problems, []) + assert.equal(result.ok, true) +}) + +test('an edge that carries a fact the consumer does not take in is still refused (control)', () => { + const def = definition('text:draft', 'draft.text:draft', 'publish.text:other', 'text:draft') + const result = validateWorkflow(def) + assert.equal(result.ok, false) + assert.ok( + result.problems.some((problem) => problem.code === 'edge/misaligned' || problem.code === 'edge/unknown-end'), + JSON.stringify(result.problems), + ) +})