[rig-tasks] Add 10 rig samples — 2026-08-28 - #504
Conversation
- 481 git-blame-line-age-analyzer: defineTool+p.glob, s.record output - 482 ts-mapped-type-extractor: defineTool+p.glob, steering addon - 483 dotenv-drift-detector: p.readOptional+p.bash, s.enum classification - 484 vitest-snapshot-reporter: workflow with Promise.all parallel subagents - 485 git-stale-branch-reporter: defineTool with node:child_process, repair addon - 486 ts-abstract-class-finder: defineTool+p.glob, steering addon - 487 git-tag-message-extractor: defineTool+p.bash, s.enum classification - 488 parallel-dep-audit-workflow: workflow with two concurrent auditor agents - 489 ts-union-type-writer: p.writeInput, input schema, validateVariant tool - 490 json-schema-field-classifier: defineTool+p.glob, s.record nested output 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 /diagnosing-bugs, /codebase-design, and /grill-with-docs — requesting changes on two correctness issues and one instructions/schema mismatch.
📋 Key Themes & Highlights
Key Themes
- Shell injection (481, 485, 487): User-supplied
filePath,branch, andtagvalues are interpolated directly intoexecSynctemplate literals. Paths with spaces or metacharacters will silently break or be exploitable. Switch tospawnSyncwith an argument array. - Incorrect per-class metrics (486):
methodCountandabstractMethodCountare computed against the whole file instead of the matched class body, so any multi-class file produces wrong numbers for every class. - Schema/instructions mismatch (489):
typeSourceis declared in the output schema but omitted from theReturnclause in the instructions; this teaches readers that partial instructions are fine, when in practice it causes avoidable repair turns.
Positive Highlights
- ✅ Workflow samples (484, 488) correctly handle
call()returningT | nullwith type-predicate filters beforereduce()— good pattern to demonstrate - ✅ Bracket notation for
Record<string, unknown>in 490 is a nice explicit fix worth keeping - ✅ Consistent use of
s.optionalfor fields that may be absent (oldestFile, newestFile, etc.) is clean
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 41.2 AIC · ⌖ 4.42 AIC · ⊞ 6.3K
Comment /matt to run again
| const { execSync } = await import("node:child_process"); | ||
| let output = ""; | ||
| try { | ||
| output = execSync(`git blame --line-porcelain -- ${filePath}`, { encoding: "utf-8" }); |
There was a problem hiding this comment.
[/diagnosing-bugs] Shell injection risk: filePath is interpolated directly into the execSync command — a path with spaces or shell metacharacters will break or be exploited at runtime.
💡 Suggested fix
Use spawnSync with an argument array to avoid shell interpretation:
const { spawnSync } = await import("node:child_process");
const result = spawnSync("git", ["blame", "--line-porcelain", "--", filePath], { encoding: "utf-8" });
output = result.stdout;The same pattern applies to samples 485 (branch in git log) and 487 (tag in git cat-file).
| while ((m = classPattern.exec(content)) !== null) { | ||
| const name = m[1]; | ||
| const methodMatches = content.match(/\b(?:public|protected|private|async)?\s+\w+\s*[(<]/g) ?? []; | ||
| const abstractMatches = content.match(abstractMethodPattern) ?? []; |
There was a problem hiding this comment.
[/codebase-design] Incorrect per-class metrics: methodCount and abstractMethodCount are counted from the entire file content, not from within each class body. If a file has multiple classes, every class gets the same (wrong) counts.
💡 Suggested fix
Slice the content between the class opening brace and its matching close brace before counting methods:
// find the index of '{' after the class declaration, then track brace depth to find the end
const start = m.index + m[0].length - 1; // index of '{'
let depth = 1, i = start + 1;
while (i < content.length && depth > 0) {
if (content[i] === '{') depth++;
else if (content[i] === '}') depth--;
i++;
}
const body = content.slice(start, i);
const methodMatches = body.match(/(?:public|protected|private|async)?\s+\w+\s*[(<]/g) ?? [];
const abstractMatches = body.match(abstractMethodPattern) ?? [];As a sample, this inaccuracy teaches readers that the pattern is correct when it isn't.
| instructions: p`You are given a typeName, variants array, and outputFile path via input. For each variant in input.variants, call validateVariant to check it is a valid TypeScript identifier. Build a TypeScript union type declaration: "export type <typeName> = <variant1> | <variant2> | ...;" using only valid variants. Write the declaration to input.outputFile using ${p.writeInput("outputFile", "typeSource")}. Return outputFile, variantsWritten (valid variants), isValid (true if all variants passed), linesEmitted (line count of the declaration).`, | ||
| output: s.object({ | ||
| typeSource: s.string, | ||
| outputFile: s.path, |
There was a problem hiding this comment.
[/grill-with-docs] typeSource is in the output schema but not mentioned in the Return clause of the instructions — the LLM may omit it or produce it inconsistently, causing validation failures that trigger unnecessary repair turns.
💡 Suggested fix
Add it explicitly to the return list:
Return outputFile, typeSource (the full TypeScript declaration string), variantsWritten (valid variants only), isValid (true if all variants passed), and linesEmitted (line count).
repair() will catch it eventually, but as a teaching sample this sets a misleading precedent that incomplete instructions are fine.
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
Tasks 4, 8, and 10 initially failed typecheck and were fixed before committing:
WorkflowMetarequiresdescription(not justname);workflow()does not accept anoutputfield (type is inferred from body);call()returnsT | nullsoPromise.allresults need a type-predicate filter beforereduce().Record<string, unknown>requires bracket notationschema["properties"]— dot access triggers TS4111noPropertyAccessFromIndexSignature.Tasks run