[rig-eslint] Add no-heterogeneous-parallel ESLint rule - #431
Conversation
Detect and autofix parallel() calls with thunks that call different agents (heterogeneous output types). Replace parallel() with Promise.all() which TypeScript can handle correctly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on one correctness bug: the autofix produces semantically broken code.
📋 Key Themes & Highlights
Blocking Issue
- Broken autofix:
parallelcalls thunks internally (thunks.map(t => t())), but the generated fix replaces it withPromise.all([...thunks])which does not call the thunks. At runtime the result would be[Function, Function]rather than the awaited values. The PR description's own "fixed" example has the same mistake. The fix should either unwrap the arrow functions or be removed in favour of a suggestion-only diagnostic.
Positive Highlights
- ✅ Well-motivated by evidence from 6 prior PRs — the detection logic is conservative and avoids false positives
- ✅ Dual-path implementation (token scanner + AST visitor) is consistent with the existing rule pattern
- ✅ Strong test coverage for accept/reject/idempotence cases and ESLint-rule alignment
- ✅ Clean integration into
index.jsandlint.js
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 47.1 AIC · ⌖ 4.35 AIC · ⊞ 6.3K
Comment /matt to run again
| 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" }], |
There was a problem hiding this comment.
[/tdd] The autofix is semantically broken: parallel calls each thunk internally via thunks.map(t => t()), but Promise.all does not — so after the fix the call silently returns [Function, Function] instead of awaited results.
💡 Why and how to fix
parallel's implementation is:
export async function parallel<Result>(thunks) {
return Promise.all(thunks.map(async (thunk) => thunk()));
}So parallel([() => call(agentA, x), () => call(agentB, x)]) works correctly, but the autofix produces Promise.all([() => call(agentA, x), () => call(agentB, x)]) which resolves to [Function, Function].
Two safe options:
- Remove the autofix (set
fixable: undefined, drop thefixcallback andeditsfield) and let the error message guide the developer to unwrap thunks manually. - Generate a correct fix that unwraps arrow function bodies: replace
() => 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.
| "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"); |
There was a problem hiding this comment.
[/tdd] The test for fixes only checks that the source string transforms correctly — it never asserts that the fixed code actually awaits the results. A runtime smoke test (or at minimum a comment) would catch the thunk-unwrapping bug.
💡 Add a runtime assertion
A 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.
Recurring mistake
parallel([thunkA, thunkB])is repeatedly used with thunks that call different agents with distinct output schemas. Becauseparallel<Result>requires a single homogeneous type parameter, TypeScript cannot unify differing return types and reports a type error. The correct fix isPromise.all([thunkA, thunkB]), which infers a tuple type and handles heterogeneous results correctly.Evidence — at least 5 distinct runs
`parallel()` helper doesn't support heterogeneous output types; switched to `Promise.all`.parallel()` requires homogeneous result type across thunks — solved by giving both subagents the same unified output schema.parallel()in workflow body was replaced withPromise.all()due to a TypeScript inference limitation when the two subagent output shapes differ.workflow({ agents: {...} })pattern ... Fixed by ... sequentialcall()invocations ... (heterogeneousparallel)parallel([() => call(agentA, ...), () => call(agentB, ...)])with heterogeneous agent output types ... Fixed by using sequentialcall()statements.parallel()toPromise.allto avoid heterogeneous type unification errorInvalid → fixed examples
The autofix replaces only the callee identifier
parallel→Promise.all. It preserves the array argument, all thunks, and all surrounding comments unchanged. The fix is idempotent becausePromise.allis not flagged.Four changed integration points
skills/rig/eslint/rules/no-heterogeneous-parallel.jsskills/rig/eslint/index.js— exportsno-heterogeneous-parallelskills/rig/eslint/lint.js— addsscanNoHeterogeneousParalleltotokenRulessrc/eslint-rules.test.js— addsdescribe("no-heterogeneous-parallel", ...)with invalid, fixed, idempotent, and ESLint-rule-alignment coverageDetection logic
The token-based scanner looks for
parallel ( [(not preceded by.), finds two or more top-level thunks inside the array, and checks whether theircall(agent, ...)first-argument identifiers are all distinct. If ≥2 distinct agent names are found, it flags and offers theparallel→Promise.allcallee replacement. Thunks without an identifiablecall(identifier, ...)pattern are not flagged (conservative, no false positives).Validation results