[rig-tasks] Add 10 rig samples — 2026-08-13 - #415
Conversation
Samples cover: ts generic constraints, readme badge analysis, python risk mapper, git merge complexity, ts reexport chain, comment density, git blame age, ts mapped types, dotenv drift detection, vitest snapshot reporting. 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 /codebase-design and /grill-with-docs — requesting changes on two shell injection risks, one logic bug in comment detection, and one correctness gap in circular detection.
📋 Key Themes & Highlights
Key Issues
- Shell injection (414, 417):
hashandfilePathare interpolated directly intoexecSyncshell commands. UseexecFileSyncwith an args array instead. require()inside handlers (414, 417): Dynamicrequire()should be a top-levelimport— all other samples do this correctly.- False-positive comment counting (416):
t.startsWith("*")matches non-comment code; guard it withinBlock. - Circular detection unimplemented (415):
circularDetectedin the output schema is never computed by the tool — it's left to the LLM's memory, which is unreliable. - Vulnerability heuristic logic (413):
major < 2misses CVEs inmajor == 2versions of packages likerequests.
Positive Highlights
- ✅ Consistent use of
s.*schema helpers andp.*prompt intents across all 10 samples - ✅ All samples correctly use
node:prefix for built-in imports (except therequire()cases) - ✅
repair()andsteering()addons chosen appropriately per use case - ✅ Workflow sample (420) correctly uses
Promise.all()after theparallel()TypeScript inference fix - ✅ Good use of
s.optionaland fallback patterns throughout
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 51.5 AIC · ⌖ 4.76 AIC · ⊞ 6.3K
Comment /matt to run again
| description: "Score a merge commit complexity by running git show --stat", | ||
| parameters: s.object({ hash: s.string, message: s.string }), | ||
| handler({ hash, message }: { hash: string; message: string }) { | ||
| const { execSync } = require("node:child_process"); |
There was a problem hiding this comment.
[/codebase-design] Shell injection risk: hash is interpolated directly into the shell command without quoting. A crafted commit hash (or a hash supplied by the LLM) could execute arbitrary shell code.
💡 Fix: use execFileSync with an args array
Replace the dynamic import and execSync with a top-level import and execFileSync:
import { execFileSync } from "node:child_process";
// inside handler:
const stat = execFileSync("git", ["show", "--stat", hash], { encoding: "utf-8" });This completely avoids shell parsing. As a bonus, also move the require() call to a top-level import, consistent with all other samples.
| const { execSync } = require("node:child_process"); | ||
| try { | ||
| const out = execSync(`git blame --line-porcelain "${filePath}" 2>/dev/null`, { encoding: "utf-8" }); | ||
| const now = Date.now() / 1000; |
There was a problem hiding this comment.
[/codebase-design] Shell injection risk: filePath is inserted into the shell command inside double-quotes, but a path containing " or $(...) can still break out and run arbitrary commands.
💡 Fix: use execFileSync with an args array
import { execFileSync } from "node:child_process";
const out = execFileSync("git", ["blame", "--line-porcelain", filePath], { encoding: "utf-8" });Also move the require() call to a top-level import to match the style of the other samples.
| if (inBlock) { commentLines++; if (t.includes("*/")) inBlock = false; } | ||
| else if (t.startsWith("/*") || t.startsWith("*")) { commentLines++; if (!t.includes("*/")) inBlock = true; } | ||
| else if (t.startsWith("//")) commentLines++; | ||
| } |
There was a problem hiding this comment.
[/codebase-design] False-positive comment detection: t.startsWith("*") matches lines like * 2 or *args in code, not just JSDoc continuation lines. This will over-count comment lines in files that use multiplication or pointer-like patterns.
💡 Suggested fix
Restrict the continuation-line check to lines that start with optional whitespace followed by * as a JSDoc marker:
// Replace:
else if (t.startsWith("/*") || t.startsWith("*")) { ... }
// With:
else if (t.startsWith("/*") || (inBlock && t.startsWith("*"))) { ... }
// or simply:
else if (t.startsWith("/*")) { commentLines++; if (!t.includes("*/")) inBlock = true; }Counting t.startsWith("*") only when inBlock is true avoids matching stray * characters in real code.
| while ((m = regex.exec(content)) !== null) exports.push(m[1]); | ||
| return { filePath, reexports: exports }; | ||
| } catch { | ||
| return { filePath, reexports: [] }; |
There was a problem hiding this comment.
[/grill-with-docs] Circular detection is declared in the output schema (circularDetected: s.boolean) but the tool never tracks visited paths — the detection is left entirely to the LLM's memory. On deep or large trees the LLM will hallucinate this value.
💡 Implement tracking in the tool handler
Pass a visited set from the workflow level, or have the handler accept a chain parameter and check it:
parameters: s.object({ filePath: s.path, visitedPaths: s.array(s.string) }),
async handler({ filePath, visitedPaths }) {
if (visitedPaths.includes(filePath)) return { filePath, reexports: [], circular: true };
// ... read and extract
return { filePath, reexports: exports, circular: false };
}Alternatively, document in the sample that circularDetected is a best-effort LLM judgement, not a computed fact.
| const major = parseInt(version.split(".")[0] ?? "0", 10); | ||
| let risk: "low" | "medium" | "high" = "low"; | ||
| let reason = "stable package"; | ||
| if (!version || version === "unknown") { risk = "high"; reason = "no version pinned"; } |
There was a problem hiding this comment.
[/codebase-design] Vulnerability heuristic is inverted: the condition checks major < 2 for packages like requests and urllib3, but requests has had CVEs on major == 2 (e.g. 2.18 → CVE-2018-18074). major < 2 would wrongly mark requests==1.x as high-risk and pass requests==2.x as low. A version-range check (e.g. < 2.20) would be more accurate.
💡 Example improvement
const knownVulnerable = [
{ name: "requests", maxSafe: [2, 20] },
{ name: "pyyaml", maxSafe: [5, 4] },
{ name: "pillow", maxSafe: [9, 0] },
];
const entry = knownVulnerable.find(v => n === v.name);
if (entry) {
const [safeMaj, safeMin] = entry.maxSafe;
const minor = parseInt(version.split(".")[1] ?? "0", 10);
if (major < safeMaj || (major === safeMaj && minor < safeMin)) {
risk = "high";
reason = "known past vulnerability in old version";
}
}This is a sample, so a code comment acknowledging the simplification would also suffice.
1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
None — all 10 tasks passed typecheck. Task 10 required one fix:
parallel()in workflow body was replaced withPromise.all()due to a TypeScript inference limitation when the two subagent output shapes differ.Tasks run