Skip to content

Commit 84784fc

Browse files
os-zhuangclaude
andauthored
feat(cli): lint ADR-0044 approval revise-loop footguns at compile time (#2279)
`objectstack compile` (lint-flow-patterns) now warns on two send-back-for- revision shapes an AI authoring an approval flow gets wrong: - Dead-end revise: an approval with a `revise` out-edge but no path looping back to it — a valid DAG that `registerFlow` ACCEPTS, yet the submitter reworks the record with nowhere to resubmit. The linter is the only place that catches this. - 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. Also flags `maxRevisions: 0` alongside a `revise` edge (send-back disabled). Advisory only; never fails the build. Mirrors the client-side rule shipped in objectui (#1958). Refs #2274, #1770 (ADR-0044). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent cd51229 commit 84784fc

3 files changed

Lines changed: 188 additions & 0 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/cli': patch
3+
---
4+
5+
feat(cli): lint ADR-0044 approval revise-loop footguns at compile time
6+
7+
`objectstack compile` now warns on two send-back-for-revision shapes an AI (or human) authoring an approval flow commonly gets wrong:
8+
9+
- **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.
10+
- **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'`).
11+
12+
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.

packages/cli/src/utils/lint-flow-patterns.test.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ import {
88
FLOW_PHANTOM_AGGREGATION,
99
FLOW_DOUBLE_BRACE_INTERP,
1010
FLOW_BARE_DOLLAR_REF,
11+
FLOW_APPROVAL_REVISE_DEAD_END,
12+
FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE,
13+
FLOW_APPROVAL_REVISE_DISABLED,
1114
} from './lint-flow-patterns.js';
1215

1316
const CEL = (source: string) => ({ dialect: 'cel', source });
@@ -181,3 +184,68 @@ describe('lintFlowPatterns — wrong interpolation syntax (#1315)', () => {
181184
});
182185
});
183186
});
187+
188+
189+
describe('lintFlowPatterns — approval revise loop (ADR-0044)', () => {
190+
const approvalFlow = (edges, approvalConfig = {}) => ({
191+
flows: [{
192+
name: 'budget_approval',
193+
nodes: [
194+
{ id: 'start', type: 'start', config: { triggerType: 'manual' } },
195+
{ id: 'mgr', type: 'approval', config: approvalConfig },
196+
{ id: 'wait', type: 'wait', config: { eventType: 'signal' } },
197+
{ id: 'ok', type: 'end' },
198+
{ id: 'no', type: 'end' },
199+
],
200+
edges,
201+
}],
202+
});
203+
const declaredLoop = [
204+
{ source: 'start', target: 'mgr' },
205+
{ source: 'mgr', target: 'ok', label: 'approve' },
206+
{ source: 'mgr', target: 'no', label: 'reject' },
207+
{ source: 'mgr', target: 'wait', label: 'revise' },
208+
{ source: 'wait', target: 'mgr', label: 'resubmit', type: 'back' },
209+
];
210+
211+
it('accepts a properly declared revise loop (closing edge type:back)', () => {
212+
expect(lintFlowPatterns(approvalFlow(declaredLoop))).toEqual([]);
213+
});
214+
215+
it('flags a revise loop whose closing edge is NOT type:back', () => {
216+
const edges = declaredLoop.map((e) =>
217+
e.source === 'wait' && e.target === 'mgr' ? { source: 'wait', target: 'mgr', label: 'resubmit' } : e,
218+
);
219+
const fnds = lintFlowPatterns(approvalFlow(edges));
220+
expect(fnds).toHaveLength(1);
221+
expect(fnds[0].rule).toBe(FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE);
222+
expect(fnds[0].where).toContain('mgr');
223+
expect(fnds[0].hint).toMatch(/back/i);
224+
});
225+
226+
it('flags a revise branch that never loops back (dead-end; registerFlow would accept it)', () => {
227+
const edges = [
228+
{ source: 'start', target: 'mgr' },
229+
{ source: 'mgr', target: 'ok', label: 'approve' },
230+
{ source: 'mgr', target: 'wait', label: 'revise' },
231+
];
232+
const fnds = lintFlowPatterns(approvalFlow(edges));
233+
expect(fnds).toHaveLength(1);
234+
expect(fnds[0].rule).toBe(FLOW_APPROVAL_REVISE_DEAD_END);
235+
});
236+
237+
it('flags maxRevisions:0 alongside a revise edge', () => {
238+
const fnds = lintFlowPatterns(approvalFlow(declaredLoop, { maxRevisions: 0 }));
239+
expect(fnds).toHaveLength(1);
240+
expect(fnds[0].rule).toBe(FLOW_APPROVAL_REVISE_DISABLED);
241+
});
242+
243+
it('does NOT flag a normal approval with no revise edge', () => {
244+
const edges = [
245+
{ source: 'start', target: 'mgr' },
246+
{ source: 'mgr', target: 'ok', label: 'approve' },
247+
{ source: 'mgr', target: 'no', label: 'reject' },
248+
];
249+
expect(lintFlowPatterns(approvalFlow(edges))).toEqual([]);
250+
});
251+
});

packages/cli/src/utils/lint-flow-patterns.ts

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ export const FLOW_DATE_EQUALITY_FILTER = 'flow-date-equality-filter';
5050
export const FLOW_PHANTOM_AGGREGATION = 'flow-phantom-aggregation';
5151
export const FLOW_DOUBLE_BRACE_INTERP = 'flow-double-brace-interpolation';
5252
export const FLOW_BARE_DOLLAR_REF = 'flow-bare-dollar-reference';
53+
export const FLOW_APPROVAL_REVISE_DEAD_END = 'flow-approval-revise-dead-end';
54+
export const FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE = 'flow-approval-revise-unmarked-backedge';
55+
export const FLOW_APPROVAL_REVISE_DISABLED = 'flow-approval-revise-disabled';
5356

5457
/**
5558
* 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
147150
}
148151
}
149152

153+
/** Edge `label`, normalized (trimmed, lowercased) for branch matching. */
154+
function edgeLabelOf(e: AnyRec): string {
155+
return typeof e.label === 'string' ? e.label.trim().toLowerCase() : '';
156+
}
157+
158+
/**
159+
* ADR-0044 send-back-for-revision footguns on an approval node that declares a
160+
* `revise` out-edge — the two shapes an AI authoring an approval flow gets wrong:
161+
* - the revise branch never loops back to the approval (the submitter reworks
162+
* the record with nowhere to resubmit). This is a VALID DAG, so `registerFlow`
163+
* ACCEPTS it — the linter is the only place that catches the dead end.
164+
* - the loop DOES return to the approval, but the closing edge isn't declared
165+
* `type: 'back'`, so `registerFlow` rejects it as an un-declared cycle. The
166+
* lint fires at compile time with the specific fix (mark the resubmit edge).
167+
*/
168+
function scanApprovalReviseLoops(
169+
flowName: string,
170+
nodes: AnyRec[],
171+
edges: AnyRec[],
172+
findings: FlowLintFinding[],
173+
): void {
174+
const approvals = nodes.filter((n) => n.type === 'approval');
175+
if (approvals.length === 0) return;
176+
const nodeIds = new Set(nodes.map((n) => (typeof n.id === 'string' ? n.id : '')).filter(Boolean));
177+
const outEdges = new Map<string, AnyRec[]>();
178+
for (const e of edges) {
179+
const src = typeof e.source === 'string' ? e.source : '';
180+
if (!src) continue;
181+
if (!outEdges.has(src)) outEdges.set(src, []);
182+
outEdges.get(src)!.push(e);
183+
}
184+
185+
for (const a of approvals) {
186+
const aid = typeof a.id === 'string' ? a.id : '';
187+
if (!aid) continue;
188+
const reviseTargets = edges
189+
.filter((e) => e.source === aid && edgeLabelOf(e) === 'revise')
190+
.map((e) => (typeof e.target === 'string' ? e.target : ''))
191+
.filter((t) => t && nodeIds.has(t));
192+
if (reviseTargets.length === 0) continue; // only approvals that declare a revise branch
193+
const where = `flow '${flowName}' \u00b7 approval '${aid}'`;
194+
195+
// maxRevisions:0 alongside a revise edge is self-contradictory — send-back is
196+
// disabled, so the branch always auto-rejects and never actually runs.
197+
const cfg = (a.config ?? {}) as AnyRec;
198+
if (cfg.maxRevisions === 0) {
199+
findings.push({
200+
where,
201+
message:
202+
`declares a 'revise' out-edge but \`maxRevisions: 0\` disables send-back — every revise ` +
203+
`auto-rejects, so the revise branch never runs.`,
204+
hint:
205+
`Set \`maxRevisions\` >= 1 to allow N send-backs before auto-reject, or drop the 'revise' edge ` +
206+
`if send-back isn't intended (ADR-0044).`,
207+
rule: FLOW_APPROVAL_REVISE_DISABLED,
208+
});
209+
}
210+
211+
// BFS from the revise target(s) over ALL edges; collect edges returning to
212+
// the approval (target === aid). A declared loop has >=1 such edge typed `back`.
213+
const seen = new Set<string>(reviseTargets);
214+
const queue = [...reviseTargets];
215+
const returnEdges: AnyRec[] = [];
216+
while (queue.length) {
217+
const cur = queue.shift()!;
218+
for (const e of outEdges.get(cur) ?? []) {
219+
if (e.target === aid) returnEdges.push(e);
220+
const t = typeof e.target === 'string' ? e.target : '';
221+
if (t && nodeIds.has(t) && !seen.has(t)) {
222+
seen.add(t);
223+
queue.push(t);
224+
}
225+
}
226+
}
227+
228+
if (returnEdges.length === 0) {
229+
findings.push({
230+
where,
231+
message:
232+
`has a 'revise' out-edge but no path loops back to it — the submitter reworks the record with ` +
233+
`nowhere to resubmit, so the revise branch dead-ends. (registerFlow accepts this — it's a valid DAG.)`,
234+
hint:
235+
`Close the loop: the 'revise' edge should reach a wait node whose resubmit edge returns to ` +
236+
`'${aid}' marked \`type: 'back'\` (ADR-0044). See examples/app-showcase showcase_budget_approval.`,
237+
rule: FLOW_APPROVAL_REVISE_DEAD_END,
238+
});
239+
} else if (!returnEdges.some((e) => e.type === 'back')) {
240+
findings.push({
241+
where,
242+
message:
243+
`has a 'revise' loop that returns to it, but the closing edge isn't declared \`type: 'back'\` — ` +
244+
`registerFlow rejects this as an un-declared cycle.`,
245+
hint:
246+
`Mark the resubmit edge (whose target is '${aid}') \`type: 'back'\` so cycle validation skips it ` +
247+
`while it still traverses at runtime; \`maxRevisions\` guards the loop (ADR-0044).`,
248+
rule: FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE,
249+
});
250+
}
251+
}
252+
}
253+
150254
/**
151255
* Lint every flow's start node for known authoring anti-patterns. Returns a
152256
* (possibly empty) list of advisory findings — never throws, never fails a build.
@@ -156,6 +260,7 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] {
156260
for (const flow of asArray(stack.flows)) {
157261
const flowName = typeof flow.name === 'string' ? flow.name : '(unnamed flow)';
158262
const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : [];
263+
const edges = Array.isArray(flow.edges) ? (flow.edges as AnyRec[]) : [];
159264

160265
// (a) #1874 — date-equality time condition on a record-change start node.
161266
const start = nodes.find((n) => n.type === 'start');
@@ -228,6 +333,9 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] {
228333
}
229334
}
230335
}
336+
337+
// (c) ADR-0044 — approval send-back-for-revision loop footguns.
338+
scanApprovalReviseLoops(flowName, nodes, edges, findings);
231339
}
232340
return findings;
233341
}

0 commit comments

Comments
 (0)