[rig-tasks] Add 10 rig samples — 2026-08-08 - #370
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 /codebase-design — requesting changes for two correctness issues (371, 378) and two logic bugs (375, 377) before merging.
📋 Key Themes & Highlights
Issues to Fix
- 371 – Self-reference bug:
combinedContentincludes the exporting file itself, so symbols used only within their own file are never flagged as dead. Fix: scan only other files' content when checking each file's exports. - 378 –
require()in handler: Uses CommonJSrequire("node:child_process")inside a tool handler, violating the project's ES-import-only style andnode:prefix rule. Fix: top-levelimport { execSync } from "node:child_process". - 377 – Fragile YAML key traversal: The dot-path check narrows remaining content but ignores indentation, so
server.portcan false-positive ifportappears in any section afterserver:. Needs either a comment documenting the heuristic limit or a proper YAML parse. - 375 –
isAbstractoff-by-offset: The-10char slice before the match index is unsafe. The regex already captures theabstractkeyword — usem[0].startsWith("abstract")instead.
Positive Highlights
- ✅ All 10 samples pass typecheck on first attempt
- ✅ Consistent use of
repair()andsteering()addons appropriate to each agent's needs - ✅ Good coverage of new patterns:
p.readInput,s.objectinput schema,s.optional,s.path - ✅ 379 and 380 are clean, well-structured examples
🧠 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
| }); | ||
| if (unused.length > 0) unusedExports[file] = unused; | ||
| } | ||
| return JSON.stringify(unusedExports); |
There was a problem hiding this comment.
[/codebase-design] Self-reference bug: all file contents are joined into one combinedContent string, so if a symbol appears in an import statement within the same file, it passes the check and is never flagged as unused.
💡 Suggested fix
When testing whether a symbol is imported, scan only the other files' content, not the exporting file itself:
for (const [file, symbols] of Object.entries(exportedSymbols)) {
const otherContent = files
.filter((f) => f !== file)
.map((f) => allContent[files.indexOf(f)])
.join('\n');
const unused = symbols.filter((sym) => {
const importPattern = new RegExp(`import[^;]+\\b${sym}\\b`);
return !importPattern.test(otherContent);
});
if (unused.length > 0) unusedExports[file] = unused;
}A symbol that is only referenced in its own file is still dead from the outside.
| const { execSync } = require("node:child_process"); | ||
| try { | ||
| const log = execSync( | ||
| `git log ${fromTag}..${toTag} --oneline --format="%H %ai" 2>/dev/null | head -100`, |
There was a problem hiding this comment.
[/codebase-design] require("node:child_process") inside a tool handler breaks the project style rule: Node built-ins must use top-level ES import with the node: prefix. Using require inside a handler also means the module is re-resolved on every call.
💡 Suggested fix
Move the import to the top of the file:
import { execSync } from "node:child_process";Then remove the const { execSync } = require(...) line inside the handler. Every other sample in this PR already follows this pattern.
| let present = false; | ||
| // Check if all parts of the key path appear in order as indented keys | ||
| let remaining = content; | ||
| for (const part of parts) { |
There was a problem hiding this comment.
[/codebase-design] The key-path traversal is structurally unsound: it searches for each dot-notation segment as a regex anywhere in the remaining content, not within the correct indentation scope. For example, server.port would match if port appears in a completely different section that happens to follow the server: line.
💡 Suggested approach
Either use a YAML parser (e.g., js-yaml) for correctness, or document the limitation clearly so sample users know the tool is a heuristic approximation. The current code is a valid teaching example of progressive narrowing, but the comment (line 17) should warn:
// Heuristic: searches for each key segment in remaining content.
// Does not track indentation scope — may produce false positives
// for identical key names in sibling sections.If the sample is meant to demonstrate a real-world pattern, prefer a proper YAML parse.
| current: s.string("current installed version"), | ||
| latest: s.string("latest available version"), | ||
| }), | ||
| handler({ current, latest }) { |
There was a problem hiding this comment.
[/codebase-design] Minor: parse(current) is called twice via two separate destructuring assignments ([curMajor] and [, curMinor]). This doubles the work and is easy to misread. Combine into one call.
💡 Suggested fix
handler({ current, latest }) {
const parse = (v: string) => v.replace(/^[^0-9]*/, "").split(".").map(Number);
const [curMajor, curMinor] = parse(current);
const [latMajor, latMinor] = parse(latest);
if (latMajor > curMajor) return "major" as const;
if (latMinor > curMinor) return "minor" as const;
return "patch" as const;
},Also note: if current or latest is a pre-release string like 1.0.0-beta.1, map(Number) produces NaN for the pre-release segment. Adding a || 0 guard covers this edge case.
| parent: m[2] ?? null, | ||
| interfaces: m[3] ? m[3].split(",").map((s: string) => s.trim()).filter(Boolean) : [], | ||
| isAbstract: src.slice(Math.max(0, m.index! - 10), m.index!).includes("abstract"), | ||
| }); |
There was a problem hiding this comment.
[/codebase-design] The isAbstract detection is redundant and fragile. The regex already optionally captures abstract\s+ as part of the class pattern, so m[0] (the full match) reliably includes it. The current approach slices raw source backwards by a fixed 10-character offset, which can misfire when there's whitespace or a comment between abstract and class.
💡 Suggested fix
Use the match itself:
results.push({
name: m[1],
parent: m[2] ?? null,
interfaces: m[3] ? m[3].split(",").map((s: string) => s.trim()).filter(Boolean) : [],
isAbstract: m[0].startsWith("abstract"),
});This is unambiguous and does not require the unsafe index arithmetic.
1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
No failures — all 10 tasks passed typecheck on first attempt.
Tasks run
p.glob+p.bash+ asyncdefineTool+steering()+repair()p.read+defineToolclassification +p.write+repair()p.bash git diff --numstat+defineTools.enum+repair()p.readOptional+p.glob+ asyncdefineTool+p.write+repair()p.bash find+ asyncdefineTool+steering()p.bash git log+defineTool selectMidpoint+steering()maxTurns:8s.object+p.readInput+defineToolregex +repair()p.bash git tag+defineTool fetchTagCommits+steering()p.bash npm outdated --json+defineTools.enumseverity +repair()p.glob+ asyncdefineToolregex +steering()