[rig-tasks] Add 10 rig samples — 2026-07-27 - #227
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 — requesting changes on correctness issues in defineTool handlers that readers will copy into production agents.
📋 Key Themes & Highlights
Issues found (5 inline comments)
- Dead code (234):
hookPath.endsWith(".sample")is alwaysfalse— the.sampleguard intent is silently lost - Glob scope (238):
p.glob("**/*.ts")pulls innode_modules; the instruction says to exclude them but the list is already resolved before the model sees it - Redundant parameter (236):
parseCoverageSummarytakes afilePathparameter but the agent always passes the hard-coded string"coverage/coverage-summary.json", making the parameter misleading - Date NaN (237):
totalCommitsincrements even when the date is unparseable, causing a mismatch withperDayOfWeektotals - Shell word-splitting (232):
wc -l $(find ...)splits on spaces in file paths; readers copying this pattern may hit errors in real repos
Positive Highlights
- ✅ Consistent use of
s.optional,s.enum, ands.recordthroughout - ✅
repair()addon applied on agents that output structured data where parse failures are likely - ✅ Good use of
p.readOptionalfor graceful file-absent handling in 236 and 239 - ✅ Two-phase subagent orchestration in 232 is a clean illustration of the pattern
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 61.3 AIC · ⌖ 4.83 AIC · ⊞ 6.3K
Comment /matt to run again
| try { | ||
| await access(hookPath, constants.F_OK); | ||
| const content = await readFile(hookPath, "utf8"); | ||
| const isSample = content.includes("sample") || hookPath.endsWith(".sample"); |
There was a problem hiding this comment.
[/grill-with-docs] Dead branch: hookPath.endsWith(".sample") is always false — hookPath is built as .git/hooks/${hookName} and hookName is never a .sample-suffixed string.
💡 Fix
Remove the dead branch or adjust the logic to also list .sample files:
// Option A: drop the dead condition
const isSample = content.includes("sample");
// Option B: if you want to catch .git/hooks/pre-commit.sample files,
// pass them as hookName values and the check works naturallyAs written the intent (skip .sample files) is silently lost.
| description: "Parse a vitest coverage-summary.json file to extract per-file coverage metrics.", | ||
| parameters: { filePath: s.path }, | ||
| handler: async ({ filePath }) => { | ||
| const raw = await readFile(filePath, "utf8"); |
There was a problem hiding this comment.
[/grill-with-docs] The tool reads the file but the agent passes the path via the prompt instruction (line 41) as the hard-coded string "coverage/coverage-summary.json" — the filePath parameter is effectively unused for path selection. If the coverage file is at a different location this pattern silently breaks.
💡 Fix
Have the agent pass the detected path to the tool, or remove the parameter and hard-code the path inside the tool:
// Option A: remove filePath parameter, embed the path
handler: async () => {
const raw = await readFile("coverage/coverage-summary.json", "utf8");
...
}
// Option B (better): let the agent derive the path from p.readOptional and pass it
parameters: { filePath: s.path },
// and instruction says: call parseCoverageSummary with the file path shown above| model: "small", | ||
| instructions: p`Analyze TypeScript import depth across all source files. | ||
|
|
||
| Source files to analyze: ${p.glob("**/*.ts")} |
There was a problem hiding this comment.
[/grill-with-docs] The p.glob("**/*.ts") prompt intent includes node_modules and generated files — the instruction comment says "excluding node_modules" but the glob has no exclusion, so the agent will be directed to analyze potentially thousands of files at runtime.
💡 Fix
Use a more targeted glob, consistent with how sample 232 tackles the same problem:
Source files to analyze: ${p.glob("src/**/*.ts")}Or document that the agent is expected to filter in the instruction text, which is fragile since the file list itself is already resolved before the model sees it.
| instructions: p`List TypeScript source files and their line counts. | ||
|
|
||
| Files found: ${p.glob("**/*.ts")} | ||
| Line counts: ${p.bash("wc -l $(find . -name '*.ts' -not -path '*/node_modules/*' -not -path '*/.git/*' | head -20) 2>/dev/null | tail -21")} |
There was a problem hiding this comment.
[/grill-with-docs] The wc -l command uses shell word-splitting on the $(find ...) subshell — file paths with spaces will be split into multiple arguments causing wc to error on them. This is a shell injection risk in samples (readers may copy this pattern into production tools).
💡 Fix
Use xargs or a while read loop to handle paths safely, or replace with find ... -exec wc -l:
find . -name "*.ts" -not -path "*/node_modules/*" -not -path "*/.git/*" | head -20 | xargs wc -l 2>/dev/null | tail -21| const author = parts[1].trim(); | ||
| const dateStr = parts[3].trim(); | ||
| perAuthor[author] = (perAuthor[author] ?? 0) + 1; | ||
| const day = days[new Date(dateStr).getDay()]; |
There was a problem hiding this comment.
[/grill-with-docs] new Date(dateStr).getDay() returns NaN when dateStr is empty or malformed, so days[NaN] is undefined — the if (day) guard silently drops those commits from perDayOfWeek but still increments totalCommits, causing a mismatch.
💡 Fix
Validate dateStr before parsing, or guard on the date being valid:
const date = new Date(dateStr);
if (!isNaN(date.getTime())) {
const day = days[date.getDay()];
if (day) perDayOfWeek[day] = (perDayOfWeek[day] ?? 0) + 1;
}
totalCommits++;The current guard on if (day) only prevents the undefined key insertion but does not prevent the totalCommits inflation.
| description: "Parse an env file string and return the list of key names.", | ||
| parameters: { content: s.string }, | ||
| handler: ({ content }) => { | ||
| return content |
There was a problem hiding this comment.
[/grill-with-docs] parseEnvKeys receives the file content as a string, but when p.readOptional returns nothing (file absent), the agent is instructed to pass that content to the tool — the content will be an empty string or the literal token (not available), which will result in ["(not", "available)"] being parsed as keys unless the agent handles it.
💡 Fix
Guard in the tool handler or document the expected contract:
handler: ({ content }) => {
if (!content || content.startsWith("(not")) return [];
return content.split("
")...
}Otherwise readers copying this pattern may be surprised by phantom keys appearing in exampleKeys or presentKeys when files are missing.
| parameters: { filePath: s.path }, | ||
| handler: async ({ filePath }) => { | ||
| const content = await readFile(filePath, "utf8"); | ||
| const exportMatches = content.match(/^export\s+(?:default\s+)?(?:const|function|class|type|interface|enum)\s+(\w+)/gm) ?? []; |
There was a problem hiding this comment.
[/grill-with-docs] The match(/^export\s+.../gm) regex captures the full export ... line, then a second .match(/(\w+)\s*$/) extracts the last word. For export default function foo this correctly returns foo, but for export const myVar = the trailing = is not in the pattern so the last word captured is correctly myVar — but export type { Foo } (re-export braces syntax) and export * from patterns are silently missed.
💡 Suggestion
This is a sample so perfect correctness is not required, but it would improve the sample quality (and reader trust) to note the limitation in a comment:
// Note: only detects direct declaration exports; re-exports (`export { Foo }`, `export * from`)
// are not captured by this regex.1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.p.bashgit log--diff-filter=R,s.array,repair()p.glob, asyncdefineTool,s.record,s.enump.glob, asyncdefineTool,p.writeOutputp.bash,defineTool classifyHook,s.recordp.bash git diff,s.enumcategories/prioritiesp.readOptional, asyncdefineTool,s.recordp.bashpipe-delimited log, synchronousdefineToolp.glob, asyncdefineTool,s.recordp.readOptional×2,defineTool parseEnvKeysTypecheck failures
No typecheck failures in final committed versions. Task 10 had an initial
noImplicitAnyfailure on.map(line =>callbacks in adefineToolhandler — fixed by adding explicit: stringtype annotations.Tasks run