[rig-tasks] Add 10 rig samples — 2026-08-14 - #421
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 correctness issues that would mislead readers who copy these patterns.
📋 Key Themes & Highlights
Key Themes
- Shell injection (417):
execSyncwith an interpolated branch name should usespawnSync+ an argument array — samples are teaching material and this pattern is unsafe to copy. - File-scoped vs. class-scoped counters (418):
methodCountreports all methods in the file, not per-class. Multi-class files will produce inflated, misleading values. - Double-parsing in Dockerfile ENV (419): Two overlapping regexes both match
ENV KEY VALUEform; the second pass writes the key twice (to the same value, so no corruption, but the logic is fragile and confusing). - JSON round-trip overhead (415): Asking the LLM to serialize a lock entry to a JSON string so the tool can immediately
JSON.parseit adds friction, increases token usage, and introduces a failure mode thatrepair()then has to catch. - Undocumented regex scope (416): The GraphQL regex silently truncates types with nested braces; a brief comment would prevent readers from applying it to real-world schemas.
Positive Highlights
- ✅
as conston literal return values in 412 and 420 — great TypeScript practice. - ✅ Bracket notation for
Record<string, unknown>access in 415 — correctly addressednoPropertyAccessFromIndexSignature. - ✅ Homogeneous output schema across both
parallel()thunks in 414 — demonstrates the correct constraint clearly. - ✅ Consistent use of
repair()on output-sensitive agents andsteering()where correction via prompting is preferable.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 53.6 AIC · ⌖ 4.73 AIC · ⊞ 6.3K
Comment /matt to run again
| let lastCommit: string; | ||
| try { | ||
| lastCommit = execSync(`git log -1 --format=%ci "${branchName}" 2>/dev/null`, { encoding: "utf8" }).trim(); | ||
| } catch { |
There was a problem hiding this comment.
[/codebase-design] Shell injection risk: branchName is interpolated directly into the shell command without sanitisation. A branch name containing shell metacharacters (e.g. "; rm -rf .) could execute arbitrary commands — a bad pattern for teaching material.
💡 Suggested fix: use spawnSync with an argument array
import { spawnSync } from "node:child_process";
const result = spawnSync("git", ["log", "-1", "--format=%ci", branchName], { encoding: "utf8" });
lastCommit = result.stdout.trim();This avoids the shell entirely — no injection surface, clearer intent, and a better pattern for readers to learn from.
| // Count all method-like patterns in the file as approximation | ||
| const allMethods = (content.match(/^\s+(?:(?:public|private|protected|async|static|readonly)\s+)*\w+\s*\(/gm) ?? []).length; | ||
| const abstractMethods = (content.match(abstractMethodRe) ?? []).length; | ||
| classes.push({ name, methodCount: allMethods, abstractMethodCount: abstractMethods, sourceFile: filePath }); |
There was a problem hiding this comment.
[/codebase-design] The method-count logic is file-scoped, not class-scoped, so every abstract class in the file gets the same (inflated) methodCount. If the file has two abstract classes, each reports all methods in the entire file.
💡 What to do instead
To count per-class methods accurately you need to track the class body's brace depth after each abstract class match. For a sample, it's fine to simplify the claim — just narrow the comment or instructions to "approximate total methods in the file" so readers aren't misled:
// Count all method-like patterns in the file as approximation (not per-class)
const allMethods = (content.match(/.../) ?? []).length;Samples that teach a wrong mental model propagate the mistake to every reader who copies the pattern.
| const key = pair.slice(0, eqIdx); | ||
| const value = pair.slice(eqIdx + 1); | ||
| const envType = /^(BUILD_|CI_)/.test(key) ? "build-time" as const : "runtime" as const; | ||
| results[key] = { value, envType }; |
There was a problem hiding this comment.
[/codebase-design] Double-parsing overlap: the singleRe regex (ENV KEY VALUE) and assignRe (ENV ...) will both match lines in KEY VALUE form, causing each such variable to be written to results twice. assignRe matches any ENV ... line — including single-form lines — and will find no = sign, silently discarding the pair, but if the line happens to match, a later sample derived from this could exhibit subtle duplication bugs.
💡 Cleaner approach
Run only one pass and branch on whether the segment contains =:
const envRe = /^ENV\s+(.+)$/gm;
while ((m = envRe.exec(content)) !== null) {
const segment = m[1];
if (segment.includes("=")) {
// KEY=VALUE [KEY2=VALUE2 ...] form
for (const pair of segment.split(/\s+/)) {
const eq = pair.indexOf("=");
if (eq > 0) results[pair.slice(0, eq)] = { value: pair.slice(eq + 1), envType: classify(pair.slice(0, eq)) };
}
} else {
// KEY VALUE form (exactly two tokens)
const [key, ...rest] = segment.split(/\s+/);
if (key && rest.length) results[key] = { value: rest.join(" "), envType: classify(key) };
}
}| const results: Array<{ name: string; kind: string; fieldCount: number; fields: string[]; sourceFile: string }> = []; | ||
| const re = /^(type|input|enum|interface|union)\s+(\w+)[^{]*\{([^}]*)\}/gm; | ||
| let match: RegExpExecArray | null; | ||
| while ((match = re.exec(content)) !== null) { |
There was a problem hiding this comment.
[/codebase-design] The regex /^(type|input|enum|interface|union)\s+(\w+)[^{]*\{([^}]*)\}/gm captures only single-line-to-first-} bodies. A GraphQL type whose first field opens a nested brace (e.g. a directive with arguments) will be captured incorrectly, and multi-line types spanning multiple } characters are truncated at the first closing brace.
💡 Why this matters in samples
For simple flat schemas this works fine. But the sample presents itself as a general-purpose extractor; readers who reuse it on a real schema with directives or union-extends will get silently wrong field lists. Adding a brief comment like // works for flat schemas without nested braces would set the correct expectation without changing the logic.
| body: async ({ call, parallel }) => { | ||
| const [branchHealth, commitFrequency] = await parallel([ | ||
| () => call(branchHealthAgent, "analyze"), | ||
| () => call(commitFrequencyAgent, "analyze"), |
There was a problem hiding this comment.
[/grill-with-docs] call.json is used here to classify the overall health, but the inline comment says "Workflow role: ... classify overall repository health" without explaining to readers when to prefer call.json over spinning up a named agent(). Since this is teaching material, a one-line comment spelling out the trade-off (e.g. // call.json for lightweight one-shot LLM inference without a full agent definition) would make the sample significantly more instructive.
| }); | ||
|
|
||
| // Agent role: Check package-lock.json entries for completeness of resolved and integrity fields. | ||
| const lockfileIntegrityChecker = agent({ |
There was a problem hiding this comment.
[/grill-with-docs] The instruction tells the agent to serialize each package entry as a JSON string (entryJson), then immediately parse it inside the tool handler. This round-trips through JSON serialization only to deserialize it — the agent could just pass a typed object, or the tool could accept s.unknown and cast it. The current approach also puts the serialization burden on the LLM, which may produce invalid JSON for complex entries and trigger unnecessary repair() cycles.
1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
No final typecheck failures. Two tasks required fixes before passing:
WorkflowMetarequiresdescription(not justname); alsoparallel()requires homogeneous result type across thunks — solved by giving both subagents the same unified output schema.Record<string, unknown>requires bracket notation for property access due tonoPropertyAccessFromIndexSignature— fixedentry.integrity→entry["integrity"].Tasks run
p.glob+ asyncdefineTool+repair()p.bash+defineTool+steering()+as constliteralsp.read+defineTool+repair()workflow()+parallel()+call.json()p.read+defineTool+repair()p.bash+ asyncdefineTool+s.recordoutput +steering()p.bash+defineToolwithnode:child_process+repair()p.glob+ asyncdefineTool+steering()p.bash find+ asyncdefineTool+ nesteds.record+repair()p.read+defineTool+steering()+as constliterals