[rig-tasks] Add 10 rig samples — 2026-08-15 - #427
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 and /grill-with-docs — requesting changes on two correctness issues and a significant duplication concern.
📋 Key Themes & Highlights
Blocking Issues
- Duplicate sample (412): Near-identical to existing
360-parallel-branch-analysis-workflow.md— same domain, same output shape, same health enum. The only difference is usingPromise.allinstead ofparallel()as a workaround, which teaches a weaker pattern. Either differentiate the domain or remove this sample. - GraphQL regex (414):
[^}]*won't span newlines, so the extractor silently returns empty results for any real schema file. Needs[\s\S]*?to work at all. rootDirinput ignored (415):p.bash("find . ...")always operates on cwd regardless of therootDirthe caller provides. This makes theinputschema a lie.
Minor Issues
- Duplicate import (414): Two separate
import ... from "rig"statements — easy one-line fix. - Ambiguous counter names (419):
longLineCountexcludes lines >120, which is surprising without a comment.
Positive Highlights
- ✅ Consistent use of
repair()/steering()addons across all samples - ✅ Good use of
s.optionalfor nullable fields (416dominantError) - ✅ Clean tool handler patterns in 411, 413, 416, 417, 418
- ✅ 420 workflow correctly passes typed input to
cycleDetector({ scripts: graph.scripts }) matching the fix noted in the PR description
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 68.1 AIC · ⌖ 4.62 AIC · ⊞ 6.3K
Comment /matt to run again
|
|
||
| ```rig | ||
| import { agent, defineTool, p, s } from "rig"; | ||
| import { steering } from "rig"; |
There was a problem hiding this comment.
[/grill-with-docs] Duplicate import from "rig" — steering should be merged into the first import on line 4.
💡 Suggested fix
import { agent, defineTool, p, s, steering } from "rig";Two separate import statements from the same module is inconsistent with every other sample in this repo. Merge them.
| import { agent, p, s, workflow } from "rig"; | ||
|
|
||
| const branchMetric = s.object({ | ||
| totalBranches: s.number, |
There was a problem hiding this comment.
[/codebase-design] This sample is near-duplicate of the existing 360-parallel-branch-analysis-workflow.md, which covers the same domain (parallel branch + commit analysis, same health enum). The key difference — using Promise.all instead of parallel() — is noted in the PR description as a typecheck workaround, but that makes this sample teach a weaker pattern.
💡 Options
- Differentiate the domain: rename and repurpose this sample to something distinct (e.g. tag analysis, contributor frequency).
- Fix and replace 360: if the
parallel()helper now supports heterogeneous types, update 360 instead of adding a workaround sample. - Delete this sample: if 360 already covers the pattern, remove this one to keep the sample set non-redundant.
Samples are learning material — duplicates with subtly worse patterns are actively harmful.
| instructions: p`Analyze the directory structure of the given rootDir. | ||
|
|
||
| Directories found: | ||
| ${p.bash("find . -maxdepth 3 -type d -not -path '*/node_modules/*' 2>/dev/null || echo '(none)'")} |
There was a problem hiding this comment.
[/codebase-design] p.bash("find . ...") ignores the agent's rootDir input — the discovered directories will always be relative to the process cwd, not the caller-supplied root. This breaks the contract the input schema advertises.
💡 Suggested fix
Use p.readInput isn't applicable for shell args, so inject the value via a p.bash template:
instructions: p`Analyze the directory structure of the given rootDir.
Directories found:
${p.bash("find ${input.rootDir} -maxdepth 3 -type d -not -path '*/node_modules/*' 2>/dev/null || echo '(none)'")}`,Or use a defineTool that receives rootDir as a parameter and calls fs.readdir recursively so the path is validated.
| const body = match[3]; | ||
| const fields = body.split("\n").map((l: string) => l.trim()).filter((l: string) => l.length > 0 && !l.startsWith("#")); | ||
| results[name] = { kind, fieldCount: fields.length, fields, sourceFile: filePath }; | ||
| } |
There was a problem hiding this comment.
[/codebase-design] The regex /^(type|...)\s+\w+[^{]*\{([^}]*)\}/gm only matches type bodies that fit on one line — any multi-line GraphQL type body silently produces no match. Real schemas almost always span multiple lines, so this tool will return empty results for typical inputs.
💡 Suggested fix
Remove the m flag and use a dotall approach, or use a character-class that crosses newlines:
// Replace [^}]* with [\s\S]*? to span lines
const pattern = /^(type|input|enum|interface|union)\s+(\w+)[^{]*\{([\s\S]*?)\}/gm;This matches the closing } of multi-line type bodies. Note: nested braces (e.g. inline input types) will still confuse the regex — that's acceptable for a sample, but worth a comment.
| const len = line.length; | ||
| totalLength += len; | ||
| if (len > maxLength) maxLength = len; | ||
| if (len > 120) veryLongLineCount++; |
There was a problem hiding this comment.
[/grill-with-docs] longLineCount silently excludes lines >120 chars (those go to veryLongLineCount), so "long" means exactly 80–120. This is a reasonable choice but is invisible to callers — a line at 150 chars contributes 0 to longLineCount, which is surprising. Consider renaming the bucket or adding a comment.
💡 Suggested clarification
Either rename to make the range explicit:
if (len > 120) veryLongLineCount++; // >120
else if (len > 80) mediumLineCount++; // 81–120Or add a brief comment:
if (len > 120) veryLongLineCount++; // >120 chars
else if (len > 80) longLineCount++; // 81–120 chars (exclusive)1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
No final failures. Four tasks required fixes before passing:
parallel()helper doesn't support heterogeneous output types; switched toPromise.all.Record<string, unknown>property access.joinimport fromnode:pathcausedTS6133.{ scripts: graph.scripts }).Tasks run