diff --git a/core/src/shared/workflow.js b/core/src/shared/workflow.js index 6bc1d6a8d..2d081debc 100644 --- a/core/src/shared/workflow.js +++ b/core/src/shared/workflow.js @@ -617,6 +617,13 @@ export function validateWorkflow(raw, options = {}) { } const nodeIds = def.nodes.map((node) => node.id) + // An edge names one fact at each end, and a node may spell that fact with a space (#1298): the + // alignment check reads both sides the way every other rule in this file does, or a definition + // that validation otherwise accepts is refused as misaligned over its spelling. + const specsOf = new Map(def.nodes.map((node) => [node.id, { + outputs: new Set(node.outputs.map(canonicalSpec).filter(Boolean)), + inputs: new Set(node.inputs.map(canonicalSpec).filter(Boolean)), + }])) for (const edge of def.edges) { if (!edge.from || !edge.to) { problems.push(message('edge/without-both-ends', 'an edge names only one end', { edge })) @@ -632,9 +639,9 @@ export function validateWorkflow(raw, options = {}) { ) continue } - const fromNode = nodeById(def, from.node) - const toNode = nodeById(def, to.node) - if (!fromNode.outputs.includes(from.artifact) || !toNode.inputs.includes(to.artifact)) { + const fromSpecs = specsOf.get(from.node) + const toSpecs = specsOf.get(to.node) + if (!fromSpecs?.outputs.has(from.artifact) || !toSpecs?.inputs.has(to.artifact)) { problems.push( message( 'edge/misaligned', diff --git a/core/test/workflow-edge-spec.test.js b/core/test/workflow-edge-spec.test.js new file mode 100644 index 000000000..cc6fca52c --- /dev/null +++ b/core/test/workflow-edge-spec.test.js @@ -0,0 +1,41 @@ +// An edge names one fact at each end, and a definition may spell that fact with a space (#1272, +// #1298). Everything that decides whether a definition can run keys on the parsed spec +// (`canonicalSpec`), so `text: draft` and `text:draft` are one fact on an edge too: a definition +// that mixes the two spellings between a node and its edge is runnable and must not be refused as +// misaligned. A genuinely crossed edge is still refused. +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { validateWorkflow } from '../src/shared/workflow.js' + +const definition = (edge) => ({ + id: 'draft-publish', + name: 'Draft and publish', + version: 1, + inputs: [], + deliverables: ['link:published'], + nodes: [ + // The producer spells the fact with a space; the consumer spells it without one. + { id: 'draft', name: 'Draft', role: 'Writer', outputs: ['text: draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + ], + edges: [edge], + loops: [], +}) + +test('an edge lines up when its two ends spell one fact differently', () => { + // The edge uses the consumer's spelling against the producer's spaced one. + const spacedNode = validateWorkflow(definition({ from: 'draft.text:draft', to: 'publish.text:draft' })) + assert.equal(spacedNode.ok, true, JSON.stringify(spacedNode.problems)) + // The edge uses the producer's spaced spelling against the consumer's canonical one. + const spacedEdge = validateWorkflow(definition({ from: 'draft.text: draft', to: 'publish.text:draft' })) + assert.equal(spacedEdge.ok, true, JSON.stringify(spacedEdge.problems)) +}) + +test('an edge that really does not line up is still refused', () => { + const crossed = validateWorkflow(definition({ from: 'draft.text:draft', to: 'publish.link:published' })) + assert.equal(crossed.ok, false) + assert.ok( + crossed.problems.some((problem) => problem.code === 'edge/misaligned' || problem.code === 'edge/unknown-end'), + JSON.stringify(crossed.problems), + ) +}) diff --git a/patches/ds-bot/files/src/shared/workflow.js b/patches/ds-bot/files/src/shared/workflow.js index 6bc1d6a8d..2d081debc 100644 --- a/patches/ds-bot/files/src/shared/workflow.js +++ b/patches/ds-bot/files/src/shared/workflow.js @@ -617,6 +617,13 @@ export function validateWorkflow(raw, options = {}) { } const nodeIds = def.nodes.map((node) => node.id) + // An edge names one fact at each end, and a node may spell that fact with a space (#1298): the + // alignment check reads both sides the way every other rule in this file does, or a definition + // that validation otherwise accepts is refused as misaligned over its spelling. + const specsOf = new Map(def.nodes.map((node) => [node.id, { + outputs: new Set(node.outputs.map(canonicalSpec).filter(Boolean)), + inputs: new Set(node.inputs.map(canonicalSpec).filter(Boolean)), + }])) for (const edge of def.edges) { if (!edge.from || !edge.to) { problems.push(message('edge/without-both-ends', 'an edge names only one end', { edge })) @@ -632,9 +639,9 @@ export function validateWorkflow(raw, options = {}) { ) continue } - const fromNode = nodeById(def, from.node) - const toNode = nodeById(def, to.node) - if (!fromNode.outputs.includes(from.artifact) || !toNode.inputs.includes(to.artifact)) { + const fromSpecs = specsOf.get(from.node) + const toSpecs = specsOf.get(to.node) + if (!fromSpecs?.outputs.has(from.artifact) || !toSpecs?.inputs.has(to.artifact)) { problems.push( message( 'edge/misaligned', diff --git a/patches/ds-bot/files/test/workflow-edge-spec.test.js b/patches/ds-bot/files/test/workflow-edge-spec.test.js new file mode 100644 index 000000000..cc6fca52c --- /dev/null +++ b/patches/ds-bot/files/test/workflow-edge-spec.test.js @@ -0,0 +1,41 @@ +// An edge names one fact at each end, and a definition may spell that fact with a space (#1272, +// #1298). Everything that decides whether a definition can run keys on the parsed spec +// (`canonicalSpec`), so `text: draft` and `text:draft` are one fact on an edge too: a definition +// that mixes the two spellings between a node and its edge is runnable and must not be refused as +// misaligned. A genuinely crossed edge is still refused. +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { validateWorkflow } from '../src/shared/workflow.js' + +const definition = (edge) => ({ + id: 'draft-publish', + name: 'Draft and publish', + version: 1, + inputs: [], + deliverables: ['link:published'], + nodes: [ + // The producer spells the fact with a space; the consumer spells it without one. + { id: 'draft', name: 'Draft', role: 'Writer', outputs: ['text: draft'] }, + { id: 'publish', name: 'Publish', role: 'Publisher', inputs: ['text:draft'], outputs: ['link:published'] }, + ], + edges: [edge], + loops: [], +}) + +test('an edge lines up when its two ends spell one fact differently', () => { + // The edge uses the consumer's spelling against the producer's spaced one. + const spacedNode = validateWorkflow(definition({ from: 'draft.text:draft', to: 'publish.text:draft' })) + assert.equal(spacedNode.ok, true, JSON.stringify(spacedNode.problems)) + // The edge uses the producer's spaced spelling against the consumer's canonical one. + const spacedEdge = validateWorkflow(definition({ from: 'draft.text: draft', to: 'publish.text:draft' })) + assert.equal(spacedEdge.ok, true, JSON.stringify(spacedEdge.problems)) +}) + +test('an edge that really does not line up is still refused', () => { + const crossed = validateWorkflow(definition({ from: 'draft.text:draft', to: 'publish.link:published' })) + assert.equal(crossed.ok, false) + assert.ok( + crossed.problems.some((problem) => problem.code === 'edge/misaligned' || problem.code === 'edge/unknown-end'), + JSON.stringify(crossed.problems), + ) +})