[rig-tasks] Add 10 rig samples — 2026-08-09 - #382
Conversation
- 381: TS dead export finder (reused) - 382: pkg scripts documenter (reused) - 383: git diff stats summarizer (reused) - 384: dotenv template generator (reused) - 385: TS class hierarchy extractor (reused) - 386: git bisect helper (reused) - 387: shell shebang validator (new) - 388: git log graph summarizer (new) - 389: TS complexity scorer (new) - 390: parallel multi-tool workflow (new) All 10/10 typecheck passed. 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 correctness issues in the tool handler logic across several samples.
📋 Key Themes & Highlights
Key Issues
- Inverted classification logic (383):
classifyDiffEntryhasadded/deletedswapped — verified by running the function against realgit diff --numstatoutput. New files are never classified as"added". - Always-false
isAbstract(385): The regex capturesabstractas part of its own match, som.indexpoints beforeabstract, making the 10-char lookback window always miss it. Easy one-line fix. - O(N2) file reads with no caching (381): The
findUnusedExportstool re-reads every file for every exported name in a sequential nested loop. Minor perf concern for a sample but teaches a bad pattern. - False-positive ternary counting (389):
?.and??operators inflate the ternary score — common in modern TS. - Ungrounded bisect narrative (386):
stepsRunin the output isn't anchored to actualselectMidpointcalls; the model will fabricate the step count.
Positive Highlights
- ✅ Good variety across patterns:
p.glob,p.bash,p.read/p.readOptional/p.write,repair(),steering(), async tools - ✅ Sample 390 correctly uses
Promise.allfor parallel agent execution — the workflow pattern is clean - ✅ Consistent use of
s.pathfor file paths,s.intfor counts,s.enumfor categoricals - ✅ All samples passed typecheck — the pre-submission fixes were the right call
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 56.2 AIC · ⌖ 7.81 AIC · ⊞ 6.3K
Comment /matt to run again
|
|
||
| const classifyDiffEntry = defineTool("classifyDiffEntry", { | ||
| description: "Classify a git diff --numstat line as added, modified, deleted, or renamed", | ||
| parameters: s.object({ |
There was a problem hiding this comment.
[/diagnosing-bugs] The classifyDiffEntry logic has "added" and "deleted" inverted — and the first branch is never reachable for actual new files. git diff --numstat uses additions<TAB>deletions<TAB>path, so a new file shows 5 0 file.ts; startsWith("0\t") matches zero additions (i.e. deleted files).
💡 Corrected classification
Actual outputs from the current code:
classifyDiffEntry("5\t0\tsrc/new.ts")→"modified"❌ (should be"added")classifyDiffEntry("0\t3\tsrc/gone.ts")→"added"❌ (should be"deleted")
Fixed handler:
handler: ({ line }: { line: string }) => {
const [adds, dels] = line.split("\t");
if (line.includes("=>")) return "renamed" as const;
if (adds !== "0" && dels === "0") return "added" as const;
if (adds === "0" && dels !== "0") return "deleted" as const;
return "modified" as const;
},| for (const other of allFilePaths) { | ||
| if (other === filePath) continue; | ||
| const otherContent = await readFile(other, "utf-8").catch(() => ""); | ||
| if (new RegExp(`\\b${name}\\b`).test(otherContent)) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The cross-file search uses \bName\b (word boundary regex) against raw file text, which produces false positives for any identifier that contains the exported name as a substring (e.g. export Button matches ButtonGroup), and misses renamed imports (import { Button as Btn }). This could cause all exports to appear "used" on a large codebase.
💡 More reliable search
A simple improvement: check for the name appearing in an import statement rather than anywhere in the file:
if (new RegExp(`[{,]\\s*${name}\\s*[},]`).test(otherContent)) {
isImported = true;
break;
}For a production tool, consider using ts-morph or the TypeScript compiler API. For a sample this scope is acceptable, but the description should note the limitation so users know not to rely on this for critical dead-code decisions.
| handler: async ({ filePath, allFilePaths }: { filePath: string; allFilePaths: string[] }) => { | ||
| const content = await readFile(filePath, "utf-8"); | ||
| const exportMatches = [...content.matchAll(/^export\s+(?:function|const|class|type|interface|enum)\s+(\w+)/gm)]; | ||
| const exportedNames = exportMatches.map((m) => m[1]); |
There was a problem hiding this comment.
[/codebase-design] The findUnusedExports tool reads every other file for every exported name — O(files × exports) sequential readFile calls with no caching. On a 200-file codebase with 20 exports each, this is 4000 file reads. The pattern also defeats the async runtime by await-ing inside nested loops.
💡 Cache file contents up front
Read all files once into a Map before the inner loop:
handler: async ({ filePath, allFilePaths }) => {
const files = new Map(
await Promise.all(allFilePaths.map(async p => [p, await readFile(p, "utf-8").catch(() => "")] as const))
);
const content = files.get(filePath) ?? "";
const exportedNames = [...content.matchAll(/^export\s+(?:function|const|class|type|interface|enum)\s+(\w+)/gm)]
.map(m => m[1]);
return exportedNames.filter(name =>
!allFilePaths.some(p => p !== filePath && new RegExp(`\\b${name}\\b`).test(files.get(p) ?? ""))
);
},This reads each file once via parallel Promise.all instead of re-reading every file for every exported name.
| classes.push({ | ||
| name: m[1], | ||
| parent: m[2] ?? null, | ||
| interfaces: m[3] ? m[3].split(",").map((s: string) => s.trim()) : [], |
There was a problem hiding this comment.
[/diagnosing-bugs] The isAbstract detection looks 10 characters before the class keyword: content.slice(m.index - 10, m.index). But the regex already includes (?:abstract\s+)? as its own optional prefix — so when the regex does match an abstract class, m.index points to the start of abstract, not class, meaning the 10-char lookback window is looking at content before the keyword itself and the .includes("abstract") check can never be true for those matches.
💡 Fix: capture the abstract group directly from the regex match
The regex (?:abstract\s+)?class\s+(\w+)... already optionally matches abstract. Just check m[0].startsWith("abstract"):
classes.push({
name: m[1],
parent: m[2] ?? null,
interfaces: m[3] ? m[3].split(",").map((s: string) => s.trim()) : [],
isAbstract: m[0].startsWith("abstract"),
});This is simpler and correct — no fragile index arithmetic needed.
| filePath: s.path, | ||
| }), | ||
| handler: async ({ filePath }: { filePath: string }) => { | ||
| const content = await readFile(filePath, "utf-8").catch(() => ""); |
There was a problem hiding this comment.
[/diagnosing-bugs] The ternary counter regex /\?[^?:]/g matches any ? not followed by ? or :, including optional chaining (foo?.bar) and nullish coalescing (a ?? b). This inflates the ternary count significantly in modern TypeScript codebases, since ?. and ?? are very common.
💡 Narrow the ternary pattern
// Match only conditional ternary: expression ? branch : ...
// Exclude ?. (optional chain) and ?? (nullish coalescing)
const ternaries = (content.match(/(?<!\?)\?(?![.?])/g) ?? []).length;With the original regex, foo?.bar ?? baz scores 2 ternaries when it should score 0.
| Perform up to 8 binary search steps over the commit list. | ||
| After narrowing down, return your best suspect commit, how many steps you took, | ||
| the commit range you searched, and your confidence level.`, | ||
| output: s.object({ |
There was a problem hiding this comment.
[/grill-with-docs] The agent's prompt says "Perform up to 8 binary search steps" but the output schema has no way to communicate which commit was actually checked at each step, so the model can't justify how it narrowed from the full list to suspectCommit. The stepsRun field is returned but never grounded by tool calls — the model will fabricate a narrative. Consider tracking checkedCommits: s.array(s.string) in the output or using a loop structure that makes each selectMidpoint call observable.
💡 Add a checkedCommits field to the output
output: s.object({
suspectCommit: s.optional(s.string),
stepsRun: s.int,
checkedCommits: s.array(s.string), // add this
commitRange: s.object({ start: s.string, end: s.string }),
confidence: s.enum("high", "medium", "low"),
}),This forces the model to materialise each binary search step as a real selectMidpoint call result rather than inventing a story.
| instructions: p`You are a package.json scripts documenter. | ||
| Read the package.json: ${p.read("package.json")} | ||
|
|
||
| For each script in the "scripts" field, call inferScriptPurpose to classify it. |
There was a problem hiding this comment.
[/grill-with-docs] The p.write intent here passes literal placeholder content ("## Scripts\n\n<!-- generated -->"), but the agent is supposed to write the actual classified script documentation. The p.write intent is a declarative placeholder for the path — the model fills in the real content at runtime. As written it reads as though the agent will always write the same boilerplate. The intent placeholder content should be a representative example or the actual format expected, not a static stub that misleads the reader.
💡 Use a representative placeholder
${p.write("SCRIPTS.md", "## Scripts\n\n| Script | Category | Command |\n|--------|----------|---------|\n| build | build | tsc |\n")}This makes it clear to the model (and sample readers) what structure to produce, not just that a write will happen.
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
No failures — all 10/10 samples passed typecheck. Two samples required minor fixes before passing:
as constfrom array return in async handler (TS1355)nameparam from handler destructuring (TS6133)outputfield fromworkflow(), (2) switched fromparallel()toPromise.allto avoid heterogeneous type unification errorTasks run