[rig-tasks] Add 10 rig samples — 2026-07-27 - #217
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs and /tdd — requesting changes on correctness issues in tool handlers and output schemas.
📋 Key Themes & Highlights
Key Themes
- Broken regex backreference (210):
.replace(/.../, '.test.$1')produces a literal$1— the most impactful bug since it corrupts allmissing-specsuggestions. - Enum/handler mismatch (210):
"wrong-prefix"is in the output schema but never returned by the tool, creating a misleading contract. - Unused parameter (211):
nameis declared inparametersbut never used, adding noise for the LLM. - Misleading tool signature (217):
classifyWorktreeacceptspathbut completely ignores it; the tool description should reflect the actual split of responsibilities. - Record loses duplicate keys (218):
interfaceskeyed by name drops all-but-one entry for conflicting interfaces — the core data the agent is meant to surface.
Positive Highlights
- ✅ All 10 samples pass typecheck
- ✅ Good variety of patterns:
defineTool, async handlers,p.readOptional,repair(),steering(), subagent delegation - ✅ Defensive fallbacks in bash commands (
|| echo '{}',2>/dev/null) - ✅ 213 (git-rename-tracker) and 219 (npm-audit-simplifier) are clean, idiomatic, copy-paste-ready examples
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 56.8 AIC · ⌖ 4.62 AIC · ⊞ 6.3K
Comment /matt to run again
| } | ||
| return { convention: "missing-spec" as const, suggestedName: base.replace(/\.(ts|js|tsx|jsx)$/, ".test.$1") }; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
[/grill-with-docs] Broken backreference: .test.$1 is a literal string — the regex has no capture group, so every missing-spec suggestion will contain the literal text $1 instead of the actual extension.
💡 Fix
Capture the extension explicitly:
// current (broken):
base.replace(/\.(ts|js|tsx|jsx)$/, '.test.$1')
// fixed:
const ext = base.split('.').pop();
return { convention: 'missing-spec' as const, suggestedName: base.replace(/\.(ts|js|tsx|jsx)$/, ``) + `.test.${ext}` };Or use a replacement function: .replace(/\.(ts|js|tsx|jsx)$/, (_, e) => .${e})
| model: "small", | ||
| instructions: p`Find all test files: ${p.bash("find . \\( -name '*.test.ts' -o -name '*.spec.ts' -o -name '*.test.js' -o -name '*.spec.js' -o -name '*Test.ts' -o -name '*Test.js' \\) -not -path '*/node_modules/*' | head -60")}. Use the classifyFile tool for each file path. Build a record keyed by file path with the convention classification and optional suggestedName. Set allConform to true only when every entry is classified as correct.`, | ||
| output: s.object({ | ||
| files: s.record(s.object({ |
There was a problem hiding this comment.
[/grill-with-docs] Output schema includes "wrong-prefix" as a convention enum value, but the tool handler never returns it — only "correct", "wrong-suffix", and "missing-spec" are possible. This creates a misleading contract and can confuse the LLM into hallucinating that case.
💡 Fix
Remove "wrong-prefix" from the output s.enum to match the tool's actual return values, or add a handler branch that detects wrong-prefix files (e.g. test.foo.ts).
| inPeerDependencies: s.boolean, | ||
| }), | ||
| handler({ inDependencies, inDevDependencies, inPeerDependencies }) { | ||
| if (inPeerDependencies) return "peer"; |
There was a problem hiding this comment.
[/grill-with-docs] The name parameter is declared in parameters but never destructured or used in the handler body. This is dead input that inflates the tool's parameter surface without contributing to its output, and may mislead the agent into passing extra data unnecessarily.
| const { execSync } = await import("node:child_process"); | ||
| try { | ||
| const escaped = symbolName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const out = execSync( |
There was a problem hiding this comment.
[/grill-with-docs] Shell-injecting symbolName directly into a grep command string is a command injection risk in a real tool. Even though the input is sanitised with a regex escape, the pattern is embedded in double-quoted shell interpolation, which means symbols containing \" or \ could still break the command. For a sample that readers will copy, consider using grep -F (fixed-string) to remove the need for regex escaping entirely.
💡 Safer alternative
const out = execSync(
`grep -rln -F "${symbolName}" --include="*.ts" . | grep -v node_modules`,
{ encoding: 'utf8' }
);```
`-F` treats the pattern as a literal string, eliminating the escaping surface.
</details>| isDetached: s.boolean, | ||
| }), | ||
| handler({ branch, isBare, isDetached }) { | ||
| if (isBare) return "bare"; |
There was a problem hiding this comment.
[/grill-with-docs] classifyWorktree ignores the path parameter entirely and always returns "clean" for non-bare, non-detached worktrees — even if the working tree has modifications. The actual dirty-detection is delegated to the agent via the git status intent, but the tool's description says nothing about this split. This makes the tool's contract misleading: callers see a path input but it has zero effect on the output.
💡 Fix
Either remove path from the parameters (since the handler doesn't use it) and rely on the agent to override status to dirty, or rename the tool to classifyWorktreeShape to make it clear it only classifies structural state (bare/detached/clean), not dirtiness.
| interfaces: s.record(s.object({ | ||
| fields: s.array(s.string), | ||
| file: s.path, | ||
| hasConflict: s.boolean, |
There was a problem hiding this comment.
[/grill-with-docs] The output schema models interfaces as a flat s.record keyed by interface name, but the agent is asked to detect duplicates across files — meaning the same key can appear in multiple files. A flat record by name loses all but one entry for duplicated names. The conflicts array captures them separately, but the interfaces record will silently drop earlier entries for conflicting names.
💡 Fix
Key interfaces by ${file}::${name} or change it to an array so all occurrences are preserved:
interfaces: s.array(s.object({
name: s.string,
file: s.path,
fields: s.array(s.string),
hasConflict: s.boolean,
})),1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
None — all 10 samples passed typecheck.
Tasks run