[rig-tasks] Add 10 rig samples — 2026-07-27 - #221
Conversation
- 220-git-rename-tracker-v2: git log diff-filter=R with repair addon - 221-yaml-workflow-linter: YAML validation with defineTool + node:fs - 222-two-phase-complexity-review-v2: two nano subagents (extractor+reviewer) - 223-barrel-file-generator-v2: async defineTool reading exports with node:fs/promises - 224-git-hook-inventory-v2: git hook classification with defineTool - 225-pr-review-checklist-v2: repair addon + git diff for PR review - 226-js-ast-node-counter: async defineTool regex-based AST counting - 227-git-tag-date-mapper: defineTool age classification + steering addon - 228-circular-import-detector: async defineTool graph builder + steering addon - 229-package-json-scorer: defineTool field scoring + repair addon 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 /codebase-design — commenting on design correctness issues in three recurring patterns.
📋 Key Themes & Highlights
Key Themes
- Computation delegated to the LLM instead of tools (228, 222, 229): cycle detection, line counting, and score aggregation are algorithmic tasks that belong in
defineToolhandlers, not in prompt instructions. Samples teach by example — these teach the wrong pattern. - Misleading naming / dead parameters (223, 224):
barrelFilesWrittennever writes, andnameis declared but unused. Readers copy what they see. - Regex correctness in tool handler (221): plain
includes()checks for YAML keys will false-positive on keys that contain the target string.
Positive Highlights
- ✅ Good variety of patterns across the batch:
repair(),steering(),s.record,s.optional, asyncdefineTool, subagent delegation - ✅ Typecheck fix for
readdirencoding noted and applied correctly in 228 - ✅ Dual-fallback
p.bashpattern in 225 is a nice defensive idiom - ✅ Discriminated union return type in 224's
classifyHookis well-structured
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 36.5 AIC · ⌖ 4.65 AIC · ⊞ 6.3K
Comment /matt to run again
| const isWorkflow = filePath.includes(".github/workflows"); | ||
| if (isWorkflow) { | ||
| if (!content.includes("name:")) issues.push("Missing 'name' key"); | ||
| if (!content.includes("on:") && !content.includes('"on":')) issues.push("Missing 'on' trigger"); |
There was a problem hiding this comment.
[/codebase-design] Substring matching with includes("name:") will produce false positives for YAML keys like container-name: or username:. This makes the linter unreliable and isn't a good pattern to demonstrate in a sample.
💡 Use anchored regex instead
if (!/^name:/m.test(content)) issues.push("Missing 'name' key");
if (!/^on:/m.test(content) && !content.includes('"on":')) issues.push("Missing 'on' trigger");
if (!/^jobs:/m.test(content)) issues.push("Missing 'jobs' key");Regex anchored to line start avoids false positives from partial key matches.
| parameters: s.object({ name: s.string, content: s.string }), | ||
| handler({ content }) { | ||
| if (!content || content.trim() === "missing") { | ||
| return { status: "missing" as const, isAsync: false, summary: "Hook file not present" }; |
There was a problem hiding this comment.
[/codebase-design] The name parameter is declared in parameters but ignored in the handler — only content is used. This is a misleading sample: readers expect declared parameters to be used, and silently ignoring name could cause confusion.
💡 Either use or remove the parameter
If name is not needed in the handler, remove it from parameters and pass it from the instruction context only. If it's useful for the summary, use it:
handler({ name, content }) {
// ...
const summary = isSample ? "Sample/placeholder hook" : `Active hook '${name}' (${lines} lines)`;
}| file: s.path, | ||
| })), | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/codebase-design] The extractor estimates line counts "from context" in the LLM prompt, but this is inherently unreliable — the model cannot accurately count lines from a grep summary. The sample teaches readers that line counts come from real tool calls, yet the extractor uses no tool for counting.
💡 Use a defineTool for actual line counting
Add a countLines tool that reads the file and returns an exact count:
const countLines = defineTool("countLines", {
parameters: s.object({ file: s.path }),
async handler({ file }) {
const { readFile } = await import("node:fs/promises");
const content = await readFile(file, "utf-8");
return { lineCount: content.split("
").length };
},
});This makes the sample more accurate and teaches a better pattern.
| handler({ fieldName, value, importance }) { | ||
| const present = value !== undefined && value !== null && value !== ""; | ||
| const nonEmpty = present && (typeof value !== "object" || Object.keys(value as object).length > 0); | ||
| const score = !present ? 0 : !nonEmpty ? 30 : importance === "required" ? 100 : importance === "recommended" ? 80 : 60; |
There was a problem hiding this comment.
[/codebase-design] The weighted average is described in the prompt comment but isn't actually weighted — the handler returns a fixed score per importance tier (100/80/60) and the instruction tells the LLM to "compute a weighted average", leaving the weighting to the model. This means results will be inconsistent and the sample doesn't demonstrate deterministic tool-based computation.
💡 Compute the final score in a tool or in the handler
Either add a computeScore tool that receives all field results and applies fixed weights, or compute it directly from the scoreField results in a deterministic way. Delegating arithmetic to the LLM is an anti-pattern in samples that are meant to showcase tool-driven logic.
|
|
||
| // Agent role: generate barrel index.ts content for TypeScript source directories. | ||
| const barrelFileGenerator = agent({ | ||
| model: "small", |
There was a problem hiding this comment.
[/codebase-design] The output field barrelFilesWritten counts directories with exports but the agent never actually writes any barrel files — it only detects exports. The name is misleading and doesn't match what the sample demonstrates. This could confuse readers about what the agent does.
💡 Rename to reflect what is actually computed
Rename to directoriesWithExports (or similar) to match the actual computation. If the intent is to simulate writing, add a p.write intent or note in the instruction that this is a dry-run count.
| addons: steering(), | ||
| instructions: p`Detect circular imports in TypeScript files. | ||
|
|
||
| Import overview: |
There was a problem hiding this comment.
[/codebase-design] The agent instruction asks the LLM to "analyse the graph to find cycles" from the returned graph record, but cycle detection is a graph algorithm that should be in a defineTool handler — not left to the model. Delegating DFS/BFS cycle detection to an LLM is unreliable and produces inconsistent results.
💡 Move cycle detection into the tool handler
Extend buildImportGraph (or add a detectCycles tool) to run a proper DFS and return cycles directly:
function findCycles(graph: Record<string, string[]>): string[][] {
const cycles: string[][] = [];
const visited = new Set<string>();
const stack: string[] = [];
// ... DFS implementation
return cycles;
}This makes the sample deterministic and teaches the right design: use tools for computation, LLM for orchestration.
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
One initial failure on task 9 (
228-circular-import-detector):node:fs/promises readdirwith{ recursive: true }returnsDirent[]when noencodingis specified, notstring[]. Fix: addedencoding: "utf-8"to the options object. All 10 tasks passed after this fix.Tasks run