[rig-eslint] feat(eslint): add agents-must-be-object rule - #196
Conversation
Detects and autofixes agents declared as an array literal instead of
a named object shorthand. The pattern agents: [extractor] fails at
runtime; agents: { extractor } is required.
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 and /grill-with-docs — approving with minor suggestions.
📋 Key Themes & Highlights
Findings
- Redundant regex in
scanTokens— the tokenizer contract already guarantees only identifier or single-char punctuation tokens reach that branch; a clarifying comment would be cleaner than re-testing the shape. - String-context test is correct but expressed as a comment in the
it.eacharray rather than a named test case, which makes it harder to trace intent. - Brace spacing in fix output (
{ a, b }) — worth verifying it matches the project's formatter expectations.
Positive Highlights
- ✅ Dual implementation (token scanner + ESLint visitor) is clean and consistent with sibling rules
- ✅ Comprehensive test suite: valid cases, fix cases, idempotency, empty-array edge case, and cross-implementation alignment
- ✅ Safety constraint is clearly scoped: only fires when all elements are plain identifiers, no computed expressions or spreads
- ✅
closingBrackethelper handles nesting correctly and returnsundefinedgracefully - ✅ PR description is thorough: real evidence of the recurring mistake, clear code examples, validation results
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 51.3 AIC · ⌖ 7.25 AIC · ⊞ 6.3K
Comment /matt to run again
| for (const token of inner) { | ||
| if (token.value === ",") continue; | ||
| if (/^[A-Za-z_$][\w$]*$/.test(token.value)) { | ||
| identifiers.push(token.value); |
There was a problem hiding this comment.
[/tdd] The identifier regex /^[A-Za-z_$][\w$]*$/ is redundant — the tokenizer in lint.js already guarantees that identifier-shaped tokens are the only word tokens emitted (lines 43–46). Only single-char punctuation tokens can ever reach this branch, and only , passes the skip check. Consider replacing the regex with a comment that names this invariant, making the guard's intent clearer.
💡 Clarified guard
// The tokenizer emits only identifier tokens (matched by /[A-Za-z_$][\w$]*/) or
// single-char punctuation tokens. Commas are separators; anything else is a
// non-identifier element (e.g. '[', '.') that can't be safely renamed.
if (token.value === ",") continue;
if (token.value.length === 1 && !/[A-Za-z_$]/.test(token.value[0])) {
valid = false;
break;
}
identifiers.push(token.value);This avoids re-testing the regex on every identifier token.
| describe("agents-must-be-object", () => { | ||
| it.each([ | ||
| "agents: { extractor }", | ||
| "agents: { diagnose, fix }", |
There was a problem hiding this comment.
[/tdd] The lintSource-level valid cases include "const text = 'agents: [extractor]';" in a comment but the test string doesn't actually contain a string literal — the outer double-quotes make it just a comment note in the it.each array. The test is correct (the regex 'agents: [extractor]' inside single-quoted string content is skipped by the tokenizer), but the comment-as-code makes it harder to verify intent. Consider a dedicated it that names what is being guarded.
💡 Suggested clarifying test
it("does not flag agents array inside a string literal", () => {
const source = `const text = 'agents: [extractor]';`;
expect(lintSource(source).filter((p) => p.kind === "agents-must-be-object")).toEqual([]);
});This makes the string-context protection explicit and independently verifiable.
| } | ||
|
|
||
| export default { | ||
| meta: { |
There was a problem hiding this comment.
[/grill-with-docs] The fixedText uses { a, b } (spaces inside braces), but the existing no-object-literal-record.js fix and the codebase style don't have a settled convention here. If the user's code uses {a, b} (no spaces), the autofix introduces a style difference. This is a minor inconsistency, but worth aligning with whatever convention prettier/eslint enforces in the repo.
💡 Check
Run grep -r 'agents: {' skills/rig/samples/ to see whether existing samples use spaces. If the project has Prettier configured, the fix output will be re-formatted anyway, but it's good to be consistent with the fix output of sibling rules.
Recurring mistake
Generated Rig programs repeatedly used an array literal for the
agentsfield instead of the required named-object shorthand. The runtime requiresagents: { name }(so sub-agents can be referenced by name in prompt instructions), but models naturally reach foragents: [name]because arrays are the idiomatic multi-item JavaScript collection.Evidence — at least 3 distinct runs
agentsmust be a named object:agents: { summarizer }vsagents: [summarizer]is an easy mistake. The constraint deserves a dedicated lint rule."agents-must-be-object— Invalid:agents: [extractor]; Valid:agents: { extractor }; Why model-confusing: arrays are natural for multi-item collections. Autofix: convert array literal to shorthand object."agentsmust be a named object:agents: { summarizer }vsagents: [summarizer]is an easy mistake. The constraint deserves a dedicated lint rule."Why the autofix is safe
The rule only fires when every element of the array is a plain identifier (no computed expressions, spread operators, or function calls). In that case,
[a, b]→{ a, b }is a pure syntactic transformation that preserves all referenced bindings as shorthand property names. The fix is idempotent: applying it twice yields the same result.Code examples
Invalid:
Fixed:
Changed integration points
skills/rig/eslint/rules/agents-must-be-object.js— new rule module withscanTokensfor the dependency-free CLI and a full ESLintcreate()visitor.skills/rig/eslint/index.js— exports the new rule as"agents-must-be-object".skills/rig/eslint/lint.js— importsscanAgentsMustBeObjectand adds it totokenRules.src/eslint-rules.test.js— adds 12 new tests: valid cases, fix cases, idempotency, empty-array edge case, and ESLint-rule alignment checks.Validation results