[rig-tasks] Add 10 rig samples — 2026-07-30 - #294
Conversation
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 /grill-with-docs — requesting changes on correctness issues in three of the ten samples.
📋 Key Themes & Highlights
Blocking Issues
- 310 — Drift detection logic is inverted; the handler will never report actual version mismatches, only missing packages.
- 317 —
git log | uniq -coutput has count prefixes; passing lines verbatim toextractDomaincan confuse the LLM about what constitutes theemailargument. - 318 —
sourceDirinput is declared but ignored;p.bashalways scans the whole repo.
Non-blocking Issues
- 315 —
JSON.parsein the merge handler has no try/catch (inconsistent with 310's pattern). - 319 —
readWorkflowFiletool is registered but not mentioned in instructions; LLM would need to guess it should call it.
Positive Highlights
- ✅ Consistent use of
s.record,s.enum,s.path,s.intacross all samples — good schema discipline. - ✅ Proper
node:import prefix throughout. - ✅ 310's try/catch pattern for JSON parsing is a good model — extend it to 315.
- ✅ 311's toolless approach (two
p.bashcalls +steering()) is clean and idiomatic for simple scanners. - ✅ All samples typecheck cleanly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 52.9 AIC · ⌖ 4.66 AIC · ⊞ 6.3K
Comment /matt to run again
| const lockedEntry = lockPackages[lockKey]; | ||
| const locked = lockedEntry?.version ?? "missing"; | ||
| const declaredClean = (declared as string).replace(/^[\^~>=<]/, ""); | ||
| if (locked !== declaredClean && locked !== "missing") continue; |
There was a problem hiding this comment.
[/grill-with-docs] The drift detection logic is inverted — version mismatches are silently skipped, so the tool only ever reports missing packages, never actual version differences.
💡 Bug explanation
Line 48: if (locked !== declaredClean && locked !== "missing") continue;
This skips any entry where the locked version differs from the declared version and it is not "missing" — i.e., it skips every real drift case. Only the subsequent if (locked === "missing") block ever appends to drifted.
Fix the condition:
if (locked === "missing" || locked !== declaredClean) {
drifted.push({ name, declared, locked });
}| instructions: p`You are a TypeScript barrel re-exporter. | ||
|
|
||
| Find TypeScript source files to barrel (excluding index files): | ||
| ${p.bash("find . -name '*.ts' -not -name 'index.ts' -not -name '*.test.ts' -not -name '*.d.ts' -not -path '*/node_modules/*' | head -50")} |
There was a problem hiding this comment.
[/grill-with-docs] sourceDir is declared as an input field but p.bash hard-codes find . (whole repo), silently ignoring it — the agent will barrel files from everywhere, not from the supplied directory.
💡 Fix
Use p.readInput / the input value in the bash command:
${p.bash(`find ${p.readInput("sourceDir")} -name '*.ts' -not -name 'index.ts' ...`)}or via a shell-safe reference so the runtime substitutes the actual path before execution.
Alternatively, drop sourceDir from the input schema if the intent is always to scan the whole repo.
|
|
||
| Collect all author emails from the git log: | ||
| ${p.bash("git log --format=%ae 2>/dev/null | sort | uniq -c | sort -rn || echo 'no commits'")} | ||
|
|
There was a problem hiding this comment.
[/grill-with-docs] The git log pipeline uses uniq -c | sort -rn so each line fed to the model (and then to extractDomain) has the form 42 user@example.com, not a bare email — indexOf("@") will find the correct index, but email.slice(atIdx + 1) extracts example.com correctly only by coincidence. The real problem is the model is asked to call extractDomain once per unique email but receives count-prefixed lines; it may pass the whole line (including the count) as the email argument.
💡 Fix
Strip the count prefix in the bash command:
git log --format=%ae 2>/dev/null | sort | uniq -c | sort -rn | awk '{print $2}'or have extractDomain accept the raw uniq -c line and parse both the count and the email:
handler({ line }: { line: string }) {
const m = line.trim().match(/^(\d+)\s+(\S+)$/);
if (!m) return { domain: "unknown", count: 0 };
const [, count, email] = m;
const atIdx = email.indexOf("@");
return { domain: atIdx === -1 ? "unknown" : email.slice(atIdx + 1).toLowerCase(), count: Number(count) };
}| overrideJson: s.string, | ||
| }), | ||
| handler({ baseJson, overrideJson }) { | ||
| const base = JSON.parse(baseJson) as Record<string, unknown>; |
There was a problem hiding this comment.
[/grill-with-docs] JSON.parse in the handler has no error handling — a malformed config file will throw an uncaught exception and crash the tool call. Sample 310 handles this correctly with try/catch; this sample should do the same.
💡 Fix
handler({ baseJson, overrideJson }) {
let base: Record<string, unknown>;
let override: Record<string, unknown>;
try { base = JSON.parse(baseJson); } catch { return { error: "Failed to parse base config" }; }
try { override = JSON.parse(overrideJson); } catch { return { error: "Failed to parse override config" }; }
// ... rest of merge logic
}| Discover workflow files: | ||
| ${p.glob(".github/workflows/*.yml")} | ||
|
|
||
| Read each workflow file and use estimateStepDuration to classify each step's expected duration. |
There was a problem hiding this comment.
[/grill-with-docs] The instructions tell the LLM to "read each workflow file" but never direct it to call readWorkflowFile — it would need to infer tool usage from context alone. Either mention the tool explicitly in the instructions, or inline the file content via p.glob + a p.read per file so the content is available without a separate tool call.
💡 Suggested instruction text
Discover workflow files:
${p.glob(".github/workflows/*.yml")}
For each file, call readWorkflowFile to get its content, then call estimateStepDuration for each step.
Return the declared output with per-workflow estimates.
Explicit tool direction reduces hallucination risk and makes the example clearer for readers.
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
No failures — all 10 tasks passed typecheck. Task 1 required one fix: bracket notation (
pkg["dependencies"]) instead of dot notation onRecord<string, unknown>types due tonoPropertyAccessFromIndexSignature.Tasks run
p.bash,defineToolwith JSON.parse,repair()p.bashgrep pipeline,steering()defineTool,node:fs/promises,s.record(s.array(...))p.bash, syncdefineTool,s.enum,repair()defineTool, regex,s.record(s.object(...))inputschema withs.path,p.readInput,p.writeInput,repair()p.glob, asyncdefineTool,p.write, file discovery patterndefineTool,s.record(s.int),steering()p.bash find, async tool,p.writeInputp.glob, two tools (read + classify),s.enumin tool,steering()