diff --git a/.changeset/lint-approval-revise-loop.md b/.changeset/lint-approval-revise-loop.md new file mode 100644 index 0000000000..7dff6d0dc4 --- /dev/null +++ b/.changeset/lint-approval-revise-loop.md @@ -0,0 +1,12 @@ +--- +'@objectstack/cli': patch +--- + +feat(cli): lint ADR-0044 approval revise-loop footguns at compile time + +`objectstack compile` now warns on two send-back-for-revision shapes an AI (or human) authoring an approval flow commonly gets wrong: + +- **Dead-end revise** — an approval node with a `revise` out-edge but no path looping back to it. This is a valid DAG, so `registerFlow` accepts it, yet the submitter reworks the record with nowhere to resubmit. The linter is the only place that catches the dead end. +- **Un-declared revise loop** — the loop returns to the approval but the closing edge isn't `type: 'back'`, so `registerFlow` rejects it as an un-declared cycle. The lint fires at compile time with the specific fix (mark the resubmit edge `type: 'back'`). + +Also flags `maxRevisions: 0` alongside a `revise` edge (send-back disabled, so the branch always auto-rejects and never runs). Advisory only — never fails the build. Part of #2274 / ADR-0044. diff --git a/packages/cli/src/utils/lint-flow-patterns.test.ts b/packages/cli/src/utils/lint-flow-patterns.test.ts index 63cae54fb4..c1629fe2db 100644 --- a/packages/cli/src/utils/lint-flow-patterns.test.ts +++ b/packages/cli/src/utils/lint-flow-patterns.test.ts @@ -8,6 +8,9 @@ import { FLOW_PHANTOM_AGGREGATION, FLOW_DOUBLE_BRACE_INTERP, FLOW_BARE_DOLLAR_REF, + FLOW_APPROVAL_REVISE_DEAD_END, + FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE, + FLOW_APPROVAL_REVISE_DISABLED, } from './lint-flow-patterns.js'; const CEL = (source: string) => ({ dialect: 'cel', source }); @@ -181,3 +184,68 @@ describe('lintFlowPatterns — wrong interpolation syntax (#1315)', () => { }); }); }); + + +describe('lintFlowPatterns — approval revise loop (ADR-0044)', () => { + const approvalFlow = (edges, approvalConfig = {}) => ({ + flows: [{ + name: 'budget_approval', + nodes: [ + { id: 'start', type: 'start', config: { triggerType: 'manual' } }, + { id: 'mgr', type: 'approval', config: approvalConfig }, + { id: 'wait', type: 'wait', config: { eventType: 'signal' } }, + { id: 'ok', type: 'end' }, + { id: 'no', type: 'end' }, + ], + edges, + }], + }); + const declaredLoop = [ + { source: 'start', target: 'mgr' }, + { source: 'mgr', target: 'ok', label: 'approve' }, + { source: 'mgr', target: 'no', label: 'reject' }, + { source: 'mgr', target: 'wait', label: 'revise' }, + { source: 'wait', target: 'mgr', label: 'resubmit', type: 'back' }, + ]; + + it('accepts a properly declared revise loop (closing edge type:back)', () => { + expect(lintFlowPatterns(approvalFlow(declaredLoop))).toEqual([]); + }); + + it('flags a revise loop whose closing edge is NOT type:back', () => { + const edges = declaredLoop.map((e) => + e.source === 'wait' && e.target === 'mgr' ? { source: 'wait', target: 'mgr', label: 'resubmit' } : e, + ); + const fnds = lintFlowPatterns(approvalFlow(edges)); + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE); + expect(fnds[0].where).toContain('mgr'); + expect(fnds[0].hint).toMatch(/back/i); + }); + + it('flags a revise branch that never loops back (dead-end; registerFlow would accept it)', () => { + const edges = [ + { source: 'start', target: 'mgr' }, + { source: 'mgr', target: 'ok', label: 'approve' }, + { source: 'mgr', target: 'wait', label: 'revise' }, + ]; + const fnds = lintFlowPatterns(approvalFlow(edges)); + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_APPROVAL_REVISE_DEAD_END); + }); + + it('flags maxRevisions:0 alongside a revise edge', () => { + const fnds = lintFlowPatterns(approvalFlow(declaredLoop, { maxRevisions: 0 })); + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_APPROVAL_REVISE_DISABLED); + }); + + it('does NOT flag a normal approval with no revise edge', () => { + const edges = [ + { source: 'start', target: 'mgr' }, + { source: 'mgr', target: 'ok', label: 'approve' }, + { source: 'mgr', target: 'no', label: 'reject' }, + ]; + expect(lintFlowPatterns(approvalFlow(edges))).toEqual([]); + }); +}); diff --git a/packages/cli/src/utils/lint-flow-patterns.ts b/packages/cli/src/utils/lint-flow-patterns.ts index ea0256556b..3c535b59e5 100644 --- a/packages/cli/src/utils/lint-flow-patterns.ts +++ b/packages/cli/src/utils/lint-flow-patterns.ts @@ -50,6 +50,9 @@ export const FLOW_DATE_EQUALITY_FILTER = 'flow-date-equality-filter'; export const FLOW_PHANTOM_AGGREGATION = 'flow-phantom-aggregation'; export const FLOW_DOUBLE_BRACE_INTERP = 'flow-double-brace-interpolation'; export const FLOW_BARE_DOLLAR_REF = 'flow-bare-dollar-reference'; +export const FLOW_APPROVAL_REVISE_DEAD_END = 'flow-approval-revise-dead-end'; +export const FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE = 'flow-approval-revise-unmarked-backedge'; +export const FLOW_APPROVAL_REVISE_DISABLED = 'flow-approval-revise-disabled'; /** * Node-config keys that name a capability the automation engine does NOT have. @@ -147,6 +150,107 @@ function collectTemplateStrings(value: unknown, key: string | undefined, out: st } } +/** Edge `label`, normalized (trimmed, lowercased) for branch matching. */ +function edgeLabelOf(e: AnyRec): string { + return typeof e.label === 'string' ? e.label.trim().toLowerCase() : ''; +} + +/** + * ADR-0044 send-back-for-revision footguns on an approval node that declares a + * `revise` out-edge — the two shapes an AI authoring an approval flow gets wrong: + * - the revise branch never loops back to the approval (the submitter reworks + * the record with nowhere to resubmit). This is a VALID DAG, so `registerFlow` + * ACCEPTS it — the linter is the only place that catches the dead end. + * - the loop DOES return to the approval, but the closing edge isn't declared + * `type: 'back'`, so `registerFlow` rejects it as an un-declared cycle. The + * lint fires at compile time with the specific fix (mark the resubmit edge). + */ +function scanApprovalReviseLoops( + flowName: string, + nodes: AnyRec[], + edges: AnyRec[], + findings: FlowLintFinding[], +): void { + const approvals = nodes.filter((n) => n.type === 'approval'); + if (approvals.length === 0) return; + const nodeIds = new Set(nodes.map((n) => (typeof n.id === 'string' ? n.id : '')).filter(Boolean)); + const outEdges = new Map(); + for (const e of edges) { + const src = typeof e.source === 'string' ? e.source : ''; + if (!src) continue; + if (!outEdges.has(src)) outEdges.set(src, []); + outEdges.get(src)!.push(e); + } + + for (const a of approvals) { + const aid = typeof a.id === 'string' ? a.id : ''; + if (!aid) continue; + const reviseTargets = edges + .filter((e) => e.source === aid && edgeLabelOf(e) === 'revise') + .map((e) => (typeof e.target === 'string' ? e.target : '')) + .filter((t) => t && nodeIds.has(t)); + if (reviseTargets.length === 0) continue; // only approvals that declare a revise branch + const where = `flow '${flowName}' \u00b7 approval '${aid}'`; + + // maxRevisions:0 alongside a revise edge is self-contradictory — send-back is + // disabled, so the branch always auto-rejects and never actually runs. + const cfg = (a.config ?? {}) as AnyRec; + if (cfg.maxRevisions === 0) { + findings.push({ + where, + message: + `declares a 'revise' out-edge but \`maxRevisions: 0\` disables send-back — every revise ` + + `auto-rejects, so the revise branch never runs.`, + hint: + `Set \`maxRevisions\` >= 1 to allow N send-backs before auto-reject, or drop the 'revise' edge ` + + `if send-back isn't intended (ADR-0044).`, + rule: FLOW_APPROVAL_REVISE_DISABLED, + }); + } + + // BFS from the revise target(s) over ALL edges; collect edges returning to + // the approval (target === aid). A declared loop has >=1 such edge typed `back`. + const seen = new Set(reviseTargets); + const queue = [...reviseTargets]; + const returnEdges: AnyRec[] = []; + while (queue.length) { + const cur = queue.shift()!; + for (const e of outEdges.get(cur) ?? []) { + if (e.target === aid) returnEdges.push(e); + const t = typeof e.target === 'string' ? e.target : ''; + if (t && nodeIds.has(t) && !seen.has(t)) { + seen.add(t); + queue.push(t); + } + } + } + + if (returnEdges.length === 0) { + findings.push({ + where, + message: + `has a 'revise' out-edge but no path loops back to it — the submitter reworks the record with ` + + `nowhere to resubmit, so the revise branch dead-ends. (registerFlow accepts this — it's a valid DAG.)`, + hint: + `Close the loop: the 'revise' edge should reach a wait node whose resubmit edge returns to ` + + `'${aid}' marked \`type: 'back'\` (ADR-0044). See examples/app-showcase showcase_budget_approval.`, + rule: FLOW_APPROVAL_REVISE_DEAD_END, + }); + } else if (!returnEdges.some((e) => e.type === 'back')) { + findings.push({ + where, + message: + `has a 'revise' loop that returns to it, but the closing edge isn't declared \`type: 'back'\` — ` + + `registerFlow rejects this as an un-declared cycle.`, + hint: + `Mark the resubmit edge (whose target is '${aid}') \`type: 'back'\` so cycle validation skips it ` + + `while it still traverses at runtime; \`maxRevisions\` guards the loop (ADR-0044).`, + rule: FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE, + }); + } + } +} + /** * Lint every flow's start node for known authoring anti-patterns. Returns a * (possibly empty) list of advisory findings — never throws, never fails a build. @@ -156,6 +260,7 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { for (const flow of asArray(stack.flows)) { const flowName = typeof flow.name === 'string' ? flow.name : '(unnamed flow)'; const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + const edges = Array.isArray(flow.edges) ? (flow.edges as AnyRec[]) : []; // (a) #1874 — date-equality time condition on a record-change start node. const start = nodes.find((n) => n.type === 'start'); @@ -228,6 +333,9 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { } } } + + // (c) ADR-0044 — approval send-back-for-revision loop footguns. + scanApprovalReviseLoops(flowName, nodes, edges, findings); } return findings; }