[rig-tasks] Add 10 rig samples — 2026-08-01 - #331
Conversation
Samples cover: release note enricher, docs refactor coordinator, dockerfile layer analyzer, git worktree status reporter, npm audit simplifier, js ast node counter, python requirements risk mapper, git merge complexity scorer, ts reexport chain tracer, source comment density reporter. 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, /diagnosing-bugs, and /grill-with-docs — commenting with a handful of correctness issues, no blocking concerns.
📋 Key Themes & Highlights
Key Themes
- Dead code in handler (344):
classifyWorktreecan never return"dirty"becausestatusOutputis always empty in the instructions — the branch is unreachable. - Import shadowing (349): Arrow-function parameter
sshadows thesschema import — rename tosymor similar. - Shell word-splitting (348): Unquoted
$hash/$msgin thewhile readpipeline will silently misparse merge messages that contain spaces. - Heuristic opacity (347):
knownLegacy+major < 2threshold is undocumented and gives surprising results for packages likenumpy(currently at v2) orpillow(at v10). - Inline block comment blind spot (350):
trimmed.startsWith("/*")missescode(); /* inline */patterns; worth noting as a known limitation.
Positive Highlights
- ✅ Consistent use of
s.*schema helpers andp.*prompt intents throughout all 10 samples - ✅
repair()andsteering()addons applied appropriately —repairon tool-heavy agents,steeringon multi-turn chains - ✅
node:prefix used correctly for Node.js built-ins in async tool handlers - ✅
s.optional,s.record, and nesteds.objectschemas all used correctly - ✅ All samples export a default agent — consistent with the project convention
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 41.1 AIC · ⌖ 4.79 AIC · ⊞ 6.3K
Comment /matt to run again
| try { source = await readFile(filePath, "utf8"); } catch { return { references: [] }; } | ||
| const starMatches = [...source.matchAll(/export\s+\*\s+from\s+['"]([^'"]+)['"]/g)]; | ||
| const namedMatches = [...source.matchAll(/export\s+\{([^}]+)\}\s+from\s+['"]([^'"]+)['"]/g)]; | ||
| const references = [ |
There was a problem hiding this comment.
[/codebase-design] The arrow-function parameter s in .map((s: string) => s.trim()) shadows the outer s schema import from rig, which is confusing and error-prone if this snippet is reused.
💡 Suggested fix
Rename to avoid the shadow:
...namedMatches.map((m) => ({ file: m[2], symbols: m[1].split(",").map((sym: string) => sym.trim()) })),The TypeScript compiler won't catch this because both values are string, but it violates the principle of least surprise for readers.
|
|
||
| Parse each worktree block (separated by blank lines). For each worktree, call classifyWorktree with its path, branch, and an empty statusOutput (use "dirty" heuristic based on porcelain output if available). Return the list of worktrees with their statuses, total count, and whether all are clean.`, | ||
| output: s.object({ | ||
| worktrees: s.array(s.object({ |
There was a problem hiding this comment.
[/codebase-design] The classifyWorktree tool is never able to return "dirty" — the instructions always pass an empty statusOutput. The "dirty" branch in the handler is dead code, making the tool's description misleading.
💡 Suggested fix
Either thread real git status --porcelain <path> output through the instructions, or drop the statusOutput parameter and rely solely on the porcelain block fields the model already has:
// in instructions, for each worktree path:
// statusOutput: ${p.bash("git -C <path> status --porcelain")}As written, readers who study the tool will expect dirty detection to work, but it never fires.
| model: "small", | ||
| instructions: p`Recent merge commits: ${p.bash("git log --merges --oneline -20 2>/dev/null || echo ''")} | ||
| Merge diff stats: ${p.bash("git log --merges --oneline -20 --format='%H %s' 2>/dev/null | head -20 | while read hash msg; do count=$(git show --stat $hash 2>/dev/null | grep -E 'files? changed' | grep -oE '[0-9]+ files? changed' | grep -oE '^[0-9]+' || echo 0); echo \"$hash|$count|$msg\"; done")} | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] Unquoted $hash and $msg variables in the shell pipeline risk word-splitting: a commit message with spaces will break the read hash msg assignment, and a hash that expands to multiple tokens will silently corrupt git show --stat.
💡 Safer alternative
Use --format with a delimiter that avoids word-splitting, or quote the variables:
git log --merges -20 --format='%H|||%s' | while IFS='|||' read hash msg; do
count=$(git show --stat "$hash" 2>/dev/null | grep -oE '^[0-9]+(?= files? changed)' || echo 0)
echo "$hash|$count|$msg"
doneAs a sample, this shell snippet is what readers will copy into real pipelines.
| const versionParts = version.split(".").map(Number); | ||
| const major = versionParts[0] ?? 0; | ||
| const isOutdated = major === 0 || (knownLegacy.includes(name.toLowerCase()) && major < 2); | ||
| const riskLevel: "low" | "medium" | "high" = |
There was a problem hiding this comment.
[/grill-with-docs] The knownLegacy heuristic flags e.g. requests@2.31 as "high" risk because major < 2 is false — wait, actually the condition is major < 2, so requests@2.x would be isOutdated=false and riskLevel="low". But django@3.x (well above the < 2 threshold) would also be low. The threshold of major < 2 conflates "version 0/1" with "old", which gives false negatives for packages where the current major is 4+. This means a well-known package at 1.x that is outdated gets "high", but django@1.x (end-of-life since 2017) correctly gets "high". The logic works for django specifically but the variable name knownLegacy implies all listed packages follow the same versioning, which is misleading.
💡 Suggestion
Document the assumption inline so sample readers understand the intent:
// heuristic: packages in this list are considered outdated if still on major version 0 or 1
const legacyMajorThreshold: Record<string, number> = {
django: 2, flask: 1, requests: 2, urllib3: 2, cryptography: 3, pillow: 9, numpy: 1,
};
const threshold = legacyMajorThreshold[name.toLowerCase()] ?? 1;
const isOutdated = major < threshold;This avoids the misleading knownLegacy name and makes per-package thresholds explicit.
| let inBlock = false; | ||
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
| if (inBlock) { commentLines++; if (trimmed.includes("*/")) inBlock = false; } |
There was a problem hiding this comment.
[/diagnosing-bugs] The block-comment parser has an off-by-one when a /* and */ appear on the same line: it increments commentLines, then since trimmed.includes("*/") is true it sets inBlock = false — correct — but it also skips the else if chain, so a one-liner like /* foo */ is counted correctly. However, a line like code(); /* inline comment */ that doesn't start with /* is never matched by trimmed.startsWith("/*"), so inline block comments are silently under-counted.
💡 Note
This is a sample and regex-based heuristics are expected to be imperfect, but a brief comment acknowledging the limitation would help readers who copy this:
// Note: inline block comments (e.g. code(); /* note */) are not countedOtherwise a reader may trust the density ratio more than it warrants.
1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
None — all 10 tasks passed typecheck.
Tasks run