[rig-tasks] Add 10 rig samples — 2026-07-28 - #242
Conversation
10 new sample files covering subagent delegation, defineTool patterns, s.record output, s.enum classification, repair/steering addons, and new agentic patterns (binary detection, interface inheritance, license matrix, workflow timing). 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 /diagnosing-bugs and /grill-with-docs — no blocking issues, but 6 actionable findings across the new samples worth addressing.
📋 Key Themes & Highlights
Key Themes
- Shell injection (252):
urlinterpolated directly into acurlshell command — any URL from an untrusted source can execute arbitrary code. - Memory over-allocation (257): full file read before 8 KB slice; harmless for text repos but could OOM on large binary assets.
- Fragile data pipeline (259):
paste - -approach for pairingname/licenselines silently misaligns when apackage.jsonhas multiple matches or a missing field. - False schema fields (253):
reportWritten: s.booleancannot be reliably populated by the LLM sincep.writeOutputhas no return path back into the output shape. - Regex over-counting (256): job-count regex matches all 2-space-indented YAML keys, not just those under
jobs:, inflating the count and triggering incorrect suggestions. - SPDX naming mismatch (259):
spdxIdis the raw input string, not a normalised SPDX identifier — potentially misleading for callers doing SPDX lookups.
Positive Highlights
- ✅ Consistent use of
s.enumfor all classification fields — strong pattern for downstream consumers. - ✅
repair()addon correctly applied on agents that produce complex nested output. - ✅
steering()onbinaryFileDetectoris the right addon choice for large unbounded tool-call loops. - ✅ All 10 samples passed typecheck on first attempt — clean generation quality.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 61.3 AIC · ⌖ 4.88 AIC · ⊞ 6.3K
Comment /matt to run again
| const { execSync } = await import("node:child_process"); | ||
| try { | ||
| const code = execSync( | ||
| `curl --head --silent --max-time 5 -o /dev/null -w "%{http_code}" "${url}" 2>/dev/null`, |
There was a problem hiding this comment.
[/diagnosing-bugs] Shell injection risk: url is interpolated directly into a shell string. A URL with embedded $(...) or backticks would execute arbitrary code when curl is invoked via execSync.
💡 Suggested fix
Shell-escape the URL or pass it as an argument after --:
const safeUrl = url.replace(/"/g, '');
const code = execSync(
`curl --head --silent --max-time 5 -o /dev/null -w "%{http_code}" -- "${safeUrl}"`,
{ encoding: "utf8", timeout: 8000 }
).trim();Or avoid the shell entirely with node:https / fetch.
| const stats = await stat(filePath); | ||
| const sizeBytes = stats.size; | ||
| const buf = await readFile(filePath); | ||
| const sample = buf.slice(0, 8192); |
There was a problem hiding this comment.
[/diagnosing-bugs] buf.slice() returns a Buffer view sharing the same memory as the parent, not a copy — this is fine for reading, but for very large files the full file is still read into memory before slicing. Consider readFile with a byte range or use open + read to limit I/O to the 8KB sample.
💡 Suggested alternative
const fh = await import('node:fs/promises').then(m => m.open(filePath, 'r'));
try {
const buf = Buffer.alloc(8192);
const { bytesRead } = await fh.read(buf, 0, 8192, 0);
const sample = buf.subarray(0, bytesRead);
// ... null byte check
} finally {
await fh.close();
}This avoids loading a 500 MB binary into memory just to sample its first 8 KB.
| } else { | ||
| compatibility = "unknown"; | ||
| } | ||
| return JSON.stringify({ packageName, spdxId: licenseField, compatibility }); |
There was a problem hiding this comment.
[/grill-with-docs] The tool returns spdxId: licenseField (the raw input string), but the output schema also declares spdxId: s.string — the naming implies a normalised SPDX identifier, not the raw field. If licenseField is e.g. "Apache 2.0" (non-standard casing) the caller gets back a non-SPDX string labelled spdxId, which is misleading and could break downstream comparisons.
💡 Suggestion
Normalise to a canonical SPDX id before returning, or rename the field to licenseRaw to signal it's unprocessed. If normalisation isn't in scope, at least document the caveat in the tool description.
|
|
||
| package.json: ${p.read("package.json")} | ||
|
|
||
| Installed package licenses: ${p.bash("cat node_modules/*/package.json 2>/dev/null | grep -E '\"name\"|\"license\"' | paste - - | head -60 || echo 'Run npm install first'")} |
There was a problem hiding this comment.
[/diagnosing-bugs] cat node_modules/*/package.json | grep ... | paste - - is fragile: paste - - pairs consecutive lines assuming each package.json contributes exactly one "name" and one "license" line — which breaks for packages with multiple matches or missing fields. This can silently produce misaligned name/license pairs.
💡 More robust alternative
Use node -e to dump the data as newline-delimited JSON instead:
node -e "const fs=require('fs'),g=require('path').join; require('fs').readdirSync('node_modules').forEach(p=>{try{const m=JSON.parse(fs.readFileSync(g('node_modules',p,'package.json')));console.log(JSON.stringify({name:m.name,license:m.license||''}));}catch{}})"This is deterministic and safe even when license is an object.
| description: "Parse a GitHub Actions workflow YAML to extract job names, step counts, and detect optimization patterns", | ||
| parameters: s.object({ content: s.string, filename: s.string }), | ||
| handler: ({ content, filename }) => { | ||
| const jobMatches = content.match(/^\s{2}[\w-]+:/gm) ?? []; |
There was a problem hiding this comment.
[/diagnosing-bugs] The job-count regex /^\s{2}[\w-]+:/gm will match any top-level YAML key indented exactly two spaces — including on:, env:, defaults:, concurrency:, and permissions:, not only jobs.<id>. This overstates jobs and triggers the wrong suggestions.
💡 Suggested fix
Anchor the match to the jobs: block. A simple heuristic is to count keys two levels deep under a jobs: line:
const jobsSection = content.match(/^jobs:\n([\s\S]*?)(?=^\S|$)/m)?.[1] ?? '';
const jobMatches = jobsSection.match(/^ [\w-]+:/gm) ?? [];Or just document the known over-counting so sample readers aren't misled.
| }) | ||
| ), | ||
| report: s.string, | ||
| reportWritten: s.boolean, |
There was a problem hiding this comment.
[/grill-with-docs] reportWritten: s.boolean is declared in the output schema but there is no mechanism for the agent to know whether p.writeOutput succeeded — it's a prompt instruction, not a tool call with a return value. The field will be whatever the LLM guesses (likely true always), making it a false signal.
💡 Suggestion
Remove reportWritten from the output schema, or if confirming file writes matters, use a defineTool that does the write in-process and returns a real success/error value.
| parameters: s.object({ content: s.string }), | ||
| handler: ({ content }) => { | ||
| const results: Array<{ name: string; extends: string[] }> = []; | ||
| const re = /interface\s+(\w+)(?:<[^>]+>)?\s*(?:extends\s+([\w<>, ]+))?\s*\{/g; |
There was a problem hiding this comment.
[/diagnosing-bugs] The regex /interface\s+(\w+)(?:<[^>]+>)?\s*(?:extends\s+([\w<>, ]+))?\s*\{/g can false-positive on string literals, comments, or @interface JSDoc annotations. Additionally, the [\w<>, ]+ extends capture is greedy and will include leading/trailing spaces and multi-line extends that cross a newline — m[2] will be undefined in that case, silently dropping parents.
💡 Note
For a sample this is acceptable, but the description should note the regex-based limitation so readers don't copy it into production tooling. A comment like // NOTE: heuristic regex; does not handle multiline extends or string literals would set correct expectations.
1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.p.writeOutputdefineToolpre/post hook detection;s.recordoutput withrepair()defineToolcurl URL checker;s.optional(s.int)for HTTP codep.bash npm outdated+p.writeOutput; drift levels.enump.bashintents;s.record(s.object(...))author mapdefineToolwithnode:fsexistsSync;s.enum(valid/broken/unused)p.glob+defineToolYAML parsing; suggestionss.array(s.string)defineToolbuffer sampling;steering()addon for large setsdefineToolfor extends chains; depths.enum(root/derived/leaf)defineTool;hasConflicts s.booleansummaryTypecheck failures
No typecheck failures. All 10 programs passed on the first attempt.
Tasks run