[rig-tasks] Add 10 rig samples — 2026-07-27 - #208
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 /grill-with-docs, /codebase-design, and /diagnosing-bugs — requesting changes.
📋 Key Themes & Highlights
Key Themes
- Numbering collision (blocking): All 10 new samples (190–199) duplicate numbers already used by an existing batch in this repo. Must be renumbered to 200–209 before merging.
- Input not threaded through (197):
gitBlameOwnershipdeclares afilePathinput but thep.bashinstruction ignores it, always running blame on the entire working tree. - Shell injection risk (196):
checkEndpointinterpolates a caller-supplied URL directly into the shell string passed toexecSync. - Stash iteration bug (195): The second
p.bashintent has no stash ref, so onlystash@{0}is ever inspected regardless of how many stashes exist. - Regex gap (198):
extractGenericsmisses lowercase-starting type params (Array<string>,Promise<void>, etc.). - Duplicated schema (192): Outer agent and
diffAnalyzershare an identical output schema with no higher-level value added.
Positive Highlights
- ✅ Great variety of patterns: async
defineTool,p.readOptional,p.readInput,p.inputField,s.recordroot output,steering+repaircombos - ✅ Defensive bash commands throughout (2>/dev/null fallbacks)
- ✅ All 10 samples pass typecheck
- ✅
193-dockerfile-security-auditoris a particularly clean per-line tool pattern
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 52.7 AIC · ⌖ 4.56 AIC · ⊞ 6.3K
Comment /matt to run again
| @@ -0,0 +1,21 @@ | |||
| # 190 - Commit Msg Rewriter | |||
There was a problem hiding this comment.
[/grill-with-docs] Numbering collision: sample numbers 190–199 are already taken by an existing batch in this repo (e.g. 190-git-tag-timeline-reporter.md, 191-service-health-probe.md, etc.).
Two samples sharing the same numeric prefix make it impossible to reference samples by ID and can break sample-runner tooling. Renumber this batch to the next free range (200–209) before merging.
| const gitBlameOwnership = agent({ | ||
| model: "small", | ||
| input: s.object({ filePath: s.path }), | ||
| instructions: p`Run git blame on the file: ${p.bash("git blame --line-porcelain HEAD -- . 2>/dev/null | head -500 || echo ''")}. Use parseBlameOutput on the porcelain output. Compute per-author percentage from line counts. Identify the top author.`, |
There was a problem hiding this comment.
[/codebase-design] The p.bash instruction hardcodes HEAD -- . instead of using the declared filePath input, so the agent always blames the entire working tree rather than the file the caller specified.
The input field declares { filePath: s.path } but the prompt never threads it through. Use p.bash(git blame --line-porcelain HEAD -- ${p.inputField('filePath')}) (or construct the command string with the field reference) so the tool actually targets the requested file.
| const start = Date.now(); | ||
| try { | ||
| const code = execSync( | ||
| `curl -o /dev/null -s -w '%{http_code}' --max-time 5 "${url}"`, |
There was a problem hiding this comment.
[/diagnosing-bugs] The URL is interpolated directly into the shell string passed to execSync, which allows shell injection if a caller supplies a URL containing shell metacharacters (e.g. `(example.com/redacted)
💡 Safer approach
Pass the URL via an environment variable so the shell never parses it:
const code = execSync(
`curl -o /dev/null -s -w '%{http_code}' --max-time 5 "$TARGET_URL"`,
{ encoding: 'utf8', env: { ...process.env, TARGET_URL: url } },
).trim();Or use spawnSync with an args array to avoid a shell entirely.
| // Agent role: inventory all git stashes, listing changed files and classifying staleness. | ||
| const gitStashInventory = agent({ | ||
| model: "small", | ||
| instructions: p`List all stashes: ${p.bash("git stash list 2>/dev/null || echo 'no stashes'")}. For each stash ref, show changed files: ${p.bash("git stash show --name-only 2>/dev/null || true")}. Classify staleness: fresh (< 1 week), aging (1–4 weeks), stale (1–3 months), ancient (> 3 months).`, |
There was a problem hiding this comment.
[/codebase-design] The second p.bash call (git stash show --name-only) has no stash ref argument, so it always inspects stash@{0} regardless of how many stashes exist. The agent is given a list of stash refs from the first intent but has no way to expand each one programmatically via a prompt intent.
Consider using a defineTool (e.g. showStash(ref)) that runs git stash show --name-only \<ref\> for each ref returned by the first call, matching the pattern used in sample 197.
| async handler({ filePath }) { | ||
| const { readFile } = await import("node:fs/promises"); | ||
| const content = await readFile(filePath, "utf8").catch(() => ""); | ||
| const matches = content.match(/<[A-Z][A-Za-z]*(?:,\s*[A-Z][A-Za-z]*)*>/g) ?? []; |
There was a problem hiding this comment.
[/codebase-design] The regex /<[A-Z][A-Za-z]*(?:,\s*[A-Z][A-Za-z]*)*>/g only matches generics whose type parameters start with an uppercase letter, so it silently skips common real-world forms like Array<string>, Promise<void>, Map<string, number>, and Record<keyof T, unknown>.
If this is intentional (only user-defined type params), add a comment explaining the design choice. If not, widen to /<[A-Za-z][A-Za-z0-9]*(?:,\s*[A-Za-z][A-Za-z0-9]*)*>/g and acknowledge false-positive risk from HTML-like content.
| breakingChange: s.boolean, | ||
| })), | ||
| agents: { diffAnalyzer }, | ||
| }); |
There was a problem hiding this comment.
[/codebase-design] The outer agent's output schema is an exact copy of the diffAnalyzer subagent's output. If the subagent's output evolves, both schemas need to be updated in sync, which is fragile.
Since the outer agent is essentially a passthrough that delegates to diffAnalyzer, consider having it return a higher-level migrationPlan object (steps, summary, breakingChangeCount) rather than the raw diff array — giving the outer agent distinct value and a single source-of-truth schema.
1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.s.array(s.object)withs.enumcategoryp.readOptionalx2,defineToolregex extractiondiffAnalyzersubagent,p.readInputdefineTool auditPatternsper line,p.readInputp.bash npm audit --json,repair()p.bashintents,s.enumstalenessdefineToolwith curl,s.array(s.url)inputdefineTool parseBlameOutputwith regex,steeringdefineTool,s.record(...)root outputdefineTool,s.record(s.object(...))root outputTypecheck failures
No failures this run — all 10 samples passed typecheck.
Tasks run