[rig-tasks] Add 10 rig samples — 2026-08-04 - #350
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 — 3 correctness/safety issues and 2 style issues found. Requesting changes.
📋 Key Themes & Highlights
Key Issues
- Shell injection (362):
packageNameinterpolated unsanitised intoexecSyncshell command — highest-severity finding - Fragile heuristic (366): JSDoc detection only checks 2 lines back, undercounts multi-line comments
- Missing error handling (370):
readJsonFilepropagatesJSON.parseexceptions, inconsistent with every other tool in the PR require()instead ofimport(369): only sample using CJS-style require, breaks the teaching pattern- Split imports (368): two separate
importstatements from"rig"in one file
Positive Highlights
- ✅ All 10 samples typecheck cleanly on first attempt
- ✅ Consistent use of
repair()/steering()addons - ✅ Uniform
s.*schema style throughout - ✅ Good use of
p.bashandp.readprompt intents - ✅ Structured output schemas are well-designed and meaningful
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 38.4 AIC · ⌖ 4.54 AIC · ⊞ 6.3K
Comment /matt to run again
| const license = execSync( | ||
| `node -e "const p=require('./node_modules/${packageName}/package.json');console.log(p.license||'')" 2>/dev/null`, | ||
| { encoding: "utf-8" } | ||
| ).trim(); |
There was a problem hiding this comment.
[/grill-with-docs] Shell injection risk: packageName is interpolated directly into an execSync shell string — a crafted package name (e.g. containing ; or $()) could execute arbitrary commands.
💡 Suggested fix
Read the package.json directly instead of using execSync:
const { readFile } = await import("node:fs/promises");
const pkg = JSON.parse(await readFile(`./node_modules/${packageName}/package.json`, "utf-8"));
const license = pkg.license ?? "";Or at minimum validate packageName against a safe identifier pattern before interpolating.
| handler: ({ revision, filePath }: { revision: string; filePath: string }) => { | ||
| const { execSync } = require("node:child_process"); | ||
| try { | ||
| const size = parseInt( |
There was a problem hiding this comment.
[/grill-with-docs] require() used instead of import: this sample uses const { execSync } = require("node:child_process") while all other samples consistently use ES module import syntax (e.g. await import(...) in sample 362, or top-level import elsewhere). Inconsistency makes this sample misleading as a pattern reference.
💡 Suggested fix
Replace with a dynamic import for consistency:
const { execSync } = await import("node:child_process");Or hoist to a top-level import since the tool is always synchronous:
import { execSync } from "node:child_process";| const content = await readFile(filePath, "utf-8"); | ||
| return JSON.parse(content); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
[/grill-with-docs] readJsonFile throws on parse error: unlike every other tool in this PR which wraps risky I/O in try/catch, readJsonFile propagates exceptions from both readFile and JSON.parse. A malformed JSON input will crash the agent rather than return a structured error.
💡 Suggested fix
handler: async ({ filePath }: { filePath: string }) => {
try {
const content = await readFile(filePath, "utf-8");
return JSON.parse(content);
} catch {
return null;
}
},Consistency matters here — it makes the pattern teachable and avoids surprising runtime failures when the sample is adapted.
| # 368 - TS Optional Chaining Counter | ||
|
|
||
| ```rig | ||
| import { agent, p, s, defineTool } from "rig"; |
There was a problem hiding this comment.
[/grill-with-docs] Double import from "rig": agent, p, s, defineTool and steering are imported in two separate statements from the same module. The project style consolidates all rig imports into one line.
💡 Suggested fix
import { agent, p, s, defineTool, steering } from "rig";| if (/^export\s+(async\s+)?function\s+\w+/.test(line) || /^export\s+const\s+\w+\s*=\s*(async\s*)?\(/.test(line)) { | ||
| const prevLine = lines[i - 1]?.trim() ?? ""; | ||
| const prevPrevLine = lines[i - 2]?.trim() ?? ""; | ||
| if (prevLine.endsWith("*/") || prevPrevLine.endsWith("*/") || prevLine.startsWith("/**") || prevLine.startsWith("*")) { |
There was a problem hiding this comment.
[/grill-with-docs] JSDoc detection heuristic is fragile: the tool checks the immediately previous 1–2 lines for */ or /**, but multiline JSDoc blocks have many intermediate lines. A function preceded by a 5-line JSDoc comment would be counted as undocumented because prevLine would be a * continuation line rather than */.
💡 Suggested fix
Scan backwards from i-1 until a non-* line or blank line is found:
let hasJsDoc = false;
for (let j = i - 1; j >= 0; j--) {
const t = lines[j].trim();
if (t.endsWith("*/") || t.startsWith("/**")) { hasJsDoc = true; break; }
if (t === "" || (!t.startsWith("*") && !t.startsWith("/"))) break;
}
if (hasJsDoc) documentedCount++; else undocumentedCount++;1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
None — all 10 tasks passed typecheck on first attempt.
Tasks run