-
Notifications
You must be signed in to change notification settings - Fork 0
[rig-eslint] Add no-heterogeneous-parallel ESLint rule #431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| function closingBracket(tokens, openingIndex) { | ||
| let depth = 0; | ||
| for (let index = openingIndex; index < tokens.length; index += 1) { | ||
| if (tokens[index].value === "[") depth += 1; | ||
| if (tokens[index].value === "]") depth -= 1; | ||
| if (depth === 0) return index; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| function parseTopLevelEntries(tokens, source, startIndex, endIndex) { | ||
| const entries = []; | ||
| let currentStart = startIndex; | ||
| let parenDepth = 0; | ||
| let bracketDepth = 0; | ||
| let braceDepth = 0; | ||
|
|
||
| for (let index = startIndex; index < endIndex; index += 1) { | ||
| const token = tokens[index]; | ||
| if (token.value === "(") parenDepth += 1; | ||
| if (token.value === ")") parenDepth -= 1; | ||
| if (token.value === "[") bracketDepth += 1; | ||
| if (token.value === "]") bracketDepth -= 1; | ||
| if (token.value === "{") braceDepth += 1; | ||
| if (token.value === "}") braceDepth -= 1; | ||
|
|
||
| if (token.value === "," && parenDepth === 0 && bracketDepth === 0 && braceDepth === 0) { | ||
| entries.push({ startIndex: currentStart, endIndex: index }); | ||
| currentStart = index + 1; | ||
| } | ||
| } | ||
|
|
||
| if (currentStart < endIndex) { | ||
| entries.push({ startIndex: currentStart, endIndex }); | ||
| } | ||
|
|
||
| return entries.filter(({ startIndex: si, endIndex: ei }) => { | ||
| for (let i = si; i < ei; i += 1) { | ||
| if (tokens[i]) return true; | ||
| } | ||
| return false; | ||
| }); | ||
| } | ||
|
|
||
| function extractFirstCallAgent(tokens, startIndex, endIndex) { | ||
| for (let index = startIndex; index < endIndex - 2; index += 1) { | ||
| const t = tokens[index]; | ||
| const next = tokens[index + 1]; | ||
| const after = tokens[index + 2]; | ||
| if ( | ||
| t?.value === "call" | ||
| && next?.value === "(" | ||
| && after?.value | ||
| && /^[A-Za-z_$]/.test(after.value) | ||
| ) { | ||
| return after.value; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| export function scanTokens(tokens, source = "") { | ||
| const problems = []; | ||
|
|
||
| for (let index = 0; index <= tokens.length - 3; index += 1) { | ||
| const [parallel, openParen, openBracket] = tokens.slice(index, index + 3); | ||
| if ( | ||
| parallel.value !== "parallel" | ||
| || openParen.value !== "(" | ||
| || openBracket.value !== "[" | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| // Skip member access like foo.parallel( | ||
| if (tokens[index - 1]?.value === ".") continue; | ||
|
|
||
| const closeBracketIndex = closingBracket(tokens, index + 2); | ||
| if (closeBracketIndex === undefined) continue; | ||
|
|
||
| const closeParen = tokens[closeBracketIndex + 1]; | ||
| if (closeParen?.value !== ")") continue; | ||
|
|
||
| const entries = parseTopLevelEntries(tokens, source, index + 3, closeBracketIndex); | ||
| if (entries.length < 2) continue; | ||
|
|
||
| const agents = entries.map(({ startIndex: si, endIndex: ei }) => | ||
| extractFirstCallAgent(tokens, si, ei), | ||
| ); | ||
|
|
||
| const definedAgents = agents.filter(Boolean); | ||
| if (definedAgents.length < 2) continue; | ||
|
|
||
| const unique = new Set(definedAgents); | ||
| if (unique.size < 2) continue; | ||
|
|
||
| problems.push({ | ||
| start: parallel.start, | ||
| end: closeParen.end, | ||
| message: | ||
| "parallel() requires homogeneous output types. Use Promise.all([...]) when thunks call agents with different output schemas.", | ||
| kind: "no-heterogeneous-parallel", | ||
| edits: [{ start: parallel.start, end: parallel.end, text: "Promise.all" }], | ||
| }); | ||
| } | ||
|
|
||
| return problems; | ||
| } | ||
|
|
||
| export default { | ||
| meta: { | ||
| type: "problem", | ||
| docs: { | ||
| description: | ||
| "Disallow parallel() with thunks that call agents with different output schemas", | ||
| }, | ||
| fixable: "code", | ||
| schema: [], | ||
| messages: { | ||
| heterogeneous: | ||
| "parallel() requires homogeneous output types. Use Promise.all([...]) when thunks call agents with different output schemas.", | ||
| }, | ||
| }, | ||
| create(context) { | ||
| return { | ||
| CallExpression(node) { | ||
| if ( | ||
| node.callee.type !== "Identifier" | ||
| || node.callee.name !== "parallel" | ||
| || node.arguments.length !== 1 | ||
| || node.arguments[0].type !== "ArrayExpression" | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const elements = node.arguments[0].elements; | ||
| if (elements.length < 2) return; | ||
|
|
||
| const agentNames = elements.map((elem) => { | ||
| if (!elem) return null; | ||
| const body = | ||
| elem.type === "ArrowFunctionExpression" ? elem.body : | ||
| elem.type === "FunctionExpression" ? elem.body : | ||
| null; | ||
| if (!body) return null; | ||
| function findCall(node) { | ||
| if (!node || typeof node !== "object") return null; | ||
| if ( | ||
| node.type === "CallExpression" | ||
| && node.callee?.type === "Identifier" | ||
| && node.callee.name === "call" | ||
| && node.arguments?.length >= 1 | ||
| && node.arguments[0].type === "Identifier" | ||
| ) { | ||
| return node.arguments[0].name; | ||
| } | ||
| for (const val of Object.values(node)) { | ||
| if (Array.isArray(val)) { | ||
| for (const child of val) { | ||
| const found = findCall(child); | ||
| if (found) return found; | ||
| } | ||
| } else if (val && typeof val === "object" && val.type) { | ||
| const found = findCall(val); | ||
| if (found) return found; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
| return findCall(body); | ||
| }); | ||
|
|
||
| const defined = agentNames.filter(Boolean); | ||
| if (defined.length < 2) return; | ||
|
|
||
| const unique = new Set(defined); | ||
| if (unique.size < 2) return; | ||
|
|
||
| context.report({ | ||
| node: node.callee, | ||
| messageId: "heterogeneous", | ||
| fix(fixer) { | ||
| return fixer.replaceText(node.callee, "Promise.all"); | ||
| }, | ||
| }); | ||
| }, | ||
| }; | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import noImplicitAnyRule from "../skills/rig/eslint/rules/no-implicit-any-in-too | |
| import preferPGlobRule from "../skills/rig/eslint/rules/prefer-p-glob-over-bash-find.js"; | ||
| import noInvalidAgentFieldsRule from "../skills/rig/eslint/rules/no-invalid-agent-fields.js"; | ||
| import enumReturnNeedsAsConstRule from "../skills/rig/eslint/rules/enum-return-needs-as-const.js"; | ||
| import noHeterogeneousParallelRule from "../skills/rig/eslint/rules/no-heterogeneous-parallel.js"; | ||
|
|
||
| describe("define-tool-arg-count", () => { | ||
| it.each([ | ||
|
|
@@ -926,3 +927,120 @@ describe("enum-return-needs-as-const", () => { | |
| expect(reports).toHaveLength(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("no-heterogeneous-parallel", () => { | ||
| it.each([ | ||
| // Homogeneous: both thunks call the same agent — OK | ||
| "parallel([() => call(agentA, 'go'), () => call(agentA, 'go')])", | ||
| // Single thunk — OK | ||
| "parallel([() => call(agentA, 'go')])", | ||
| // Not a parallel call — OK | ||
| "Promise.all([() => call(agentA, 'go'), () => call(agentB, 'go')])", | ||
| // Member expression — OK | ||
| "foo.parallel([() => call(agentA, 'a'), () => call(agentB, 'b')])", | ||
| // Inside string literal — not tokenized | ||
| "const text = 'parallel([() => call(agentA), () => call(agentB)])';", | ||
| // Thunks without identifiable call(agent, ...) pattern — not flagged conservatively | ||
| "parallel([() => someWork(), () => otherWork()])", | ||
| ])("accepts %s", (source) => { | ||
| const problems = lintSource(source).filter((p) => p.kind === "no-heterogeneous-parallel"); | ||
| expect(problems).toEqual([]); | ||
| }); | ||
|
|
||
| it.each([ | ||
| [ | ||
| "parallel([() => call(agentA, 'analyze'), () => call(agentB, 'analyze')])", | ||
| "Promise.all([() => call(agentA, 'analyze'), () => call(agentB, 'analyze')])", | ||
| ], | ||
| [ | ||
| "const result = await parallel([() => call(branchAgent, input), () => call(commitAgent, input)])", | ||
| "const result = await Promise.all([() => call(branchAgent, input), () => call(commitAgent, input)])", | ||
| ], | ||
| [ | ||
| "parallel([\n () => call(agentX, msg),\n () => call(agentY, msg),\n])", | ||
| "Promise.all([\n () => call(agentX, msg),\n () => call(agentY, msg),\n])", | ||
| ], | ||
| ])("fixes %s", (source, expected) => { | ||
| const problems = lintSource(source).filter((p) => p.kind === "no-heterogeneous-parallel"); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test for 💡 Add a runtime assertionA minimal runtime check that would have caught the broken fix: it('fixed output actually resolves values, not functions', async () => {
const source = "parallel([() => call(agentA, 'x'), () => call(agentB, 'y')])";
const fixed = fixSource(source);
// If thunks are not unwrapped, eval(fixed) would resolve to [Function, Function]
// This forces the author to think about semantics, not just string output.
expect(fixed).toMatch(/Promise\.all\(\[call\(/);
});Even a pure string assertion that the thunk wrappers are removed would surface the issue. |
||
| expect(problems).toHaveLength(1); | ||
| expect(fixSource(source, problems)).toBe(expected); | ||
| }); | ||
|
|
||
| it("is idempotent", () => { | ||
| const source = "parallel([() => call(agentA, 'go'), () => call(agentB, 'go')])"; | ||
| const once = fixSource(source); | ||
| const twice = fixSource(once); | ||
| expect(twice).toBe(once); | ||
| expect(lintSource(once).filter((p) => p.kind === "no-heterogeneous-parallel")).toEqual([]); | ||
| }); | ||
|
|
||
| it("keeps the ESLint rule aligned", () => { | ||
| const reports = []; | ||
|
|
||
| function makeThunk(agentName) { | ||
| return { | ||
| type: "ArrowFunctionExpression", | ||
| params: [], | ||
| body: { | ||
| type: "CallExpression", | ||
| callee: { type: "Identifier", name: "call" }, | ||
| arguments: [{ type: "Identifier", name: agentName }], | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const visitor = noHeterogeneousParallelRule.create({ | ||
| sourceCode: {}, | ||
| report: (problem) => reports.push(problem), | ||
| }); | ||
|
|
||
| visitor.CallExpression({ | ||
| type: "CallExpression", | ||
| callee: { type: "Identifier", name: "parallel" }, | ||
| arguments: [ | ||
| { | ||
| type: "ArrayExpression", | ||
| elements: [makeThunk("agentA"), makeThunk("agentB")], | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| expect(reports).toHaveLength(1); | ||
| expect(reports[0].messageId).toBe("heterogeneous"); | ||
| expect(reports[0].fix({ replaceText: (_node, text) => text })).toBe("Promise.all"); | ||
| }); | ||
|
|
||
| it("does not flag homogeneous parallel via ESLint rule", () => { | ||
| const reports = []; | ||
|
|
||
| function makeThunk(agentName) { | ||
| return { | ||
| type: "ArrowFunctionExpression", | ||
| params: [], | ||
| body: { | ||
| type: "CallExpression", | ||
| callee: { type: "Identifier", name: "call" }, | ||
| arguments: [{ type: "Identifier", name: agentName }], | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const visitor = noHeterogeneousParallelRule.create({ | ||
| sourceCode: {}, | ||
| report: (problem) => reports.push(problem), | ||
| }); | ||
|
|
||
| visitor.CallExpression({ | ||
| type: "CallExpression", | ||
| callee: { type: "Identifier", name: "parallel" }, | ||
| arguments: [ | ||
| { | ||
| type: "ArrayExpression", | ||
| elements: [makeThunk("agentA"), makeThunk("agentA")], | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| expect(reports).toHaveLength(0); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The autofix is semantically broken:
parallelcalls each thunk internally viathunks.map(t => t()), butPromise.alldoes not — so after the fix the call silently returns[Function, Function]instead of awaited results.💡 Why and how to fix
parallel's implementation is:So
parallel([() => call(agentA, x), () => call(agentB, x)])works correctly, but the autofix producesPromise.all([() => call(agentA, x), () => call(agentB, x)])which resolves to[Function, Function].Two safe options:
fixable: undefined, drop thefixcallback andeditsfield) and let the error message guide the developer to unwrap thunks manually.() => call(agentA, x)→call(agentA, x)while renamingparallel→Promise.all. This is significantly harder in a token-based scanner.The test for idempotence passes because
Promise.all([thunks])is not re-flagged, but it never validates that the output actually works at runtime.