[rig-tasks] Add 10 rig samples — 2026-08-07 - #365
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 /diagnosing-bugs and /codebase-design — requesting changes on correctness issues found across several samples.
📋 Key Themes & Highlights
Key Themes
- Shell injection (371):
execSyncwith a template-stringfilePath— switch tospawnSyncwith an argument array. - Algorithmic false negatives (373):
String.includesfor symbol lookup produces false negatives; use word-boundary regex. - Schema/tool mismatch (374):
commandfield in output schema has no corresponding tool return — the LLM must hallucinate it. - Unreliable CWD heuristic (379):
process.cwd()to detect the main worktree is wrong outside the repo root; use the porcelain structure instead. - Regex coverage gaps (372, 380): YAML indent assumptions miss 2-space workflows;
typeofpattern misses!==. - Optional field handling (377, 378):
sshadowed in reduce; explicitundefinedreturned fors.optionalfields.
Positive Highlights
- ✅ All 10 samples typecheck cleanly, including the
exactOptionalPropertyTypesfix in 372. - ✅ Consistent use of
repair()addon on samples with structured output — good default. - ✅
s.enumused correctly for discriminated string fields (e.g.changeType,category). - ✅
p.readInput/p.readOptionalused appropriately in 376, 377, 378 — idiomatic rig patterns. - ✅
steering()chosen for open-ended analysis agents (371, 379);repair()for structured extraction — correct pairing.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 50.2 AIC · ⌖ 4.96 AIC · ⊞ 6.3K
Comment /matt to run again
| description: "Get git commit history for a file and return ownership info.", | ||
| parameters: { filePath: s.path }, | ||
| handler: ({ filePath }: { filePath: string }) => { | ||
| try { |
There was a problem hiding this comment.
[/diagnosing-bugs] Shell injection risk: filePath is interpolated directly into the shell command string without sanitization — paths containing shell metacharacters (spaces, $(), backticks) will be interpreted by the shell.
💡 Fix: use spawnSync with argument array
Switch from a template-string execSync call to spawnSync with a proper argument array, so the path is never parsed by the shell:
import { spawnSync } from "node:child_process";
const result = spawnSync("git", ["log", "--format=%ae", "--", filePath], { encoding: "utf-8" });
const output = result.stdout.trim();Agent-generated tool inputs are a realistic injection vector since the LLM controls the filePath argument.
| if (other === filePath) continue; | ||
| try { | ||
| const otherContent = await readFile(other, "utf-8"); | ||
| if (otherContent.includes(sym)) { found = true; break; } |
There was a problem hiding this comment.
[/diagnosing-bugs] False negative: otherContent.includes(sym) matches any substring, so a symbol like log will be considered "used" if any file contains logError, blogPost, etc. This produces incorrect dead-export results.
💡 Fix: use a word-boundary regex match
const usagePattern = new RegExp(`\\b${sym}\\b`);
if (usagePattern.test(otherContent)) { found = true; break; }Word-boundary matching is the minimum needed to avoid false negatives on short symbol names.
| command: s.string, | ||
| }) | ||
| ), | ||
| documentedCount: s.int, |
There was a problem hiding this comment.
[/codebase-design] Schema/tool mismatch: the output schema declares scripts: s.record(s.object({ purpose, category, command })), but inferScriptPurpose only returns { purpose, category } — it never returns command. The LLM must hallucinate command from context, which is unreliable.
💡 Fix: either return `command` from the tool or remove it from the output schema
Option A — return command from the tool:
return { purpose, category, command } as const;Option B — drop command from the output schema since the agent instructions can describe the scripts table without it.
The output schema should be the single source of truth; every field the LLM must populate should have a clear data source in the instructions or tool returns.
| parameters: { path: s.string, branch: s.string, commit: s.string, bare: s.boolean, detached: s.boolean }, | ||
| handler: ({ path, branch, commit, bare, detached }: { path: string; branch: string; commit: string; bare: boolean; detached: boolean }) => { | ||
| const type: "main" | "linked" | "bare" = bare ? "bare" : path === process.cwd() ? "main" : "linked"; | ||
| const status: "clean" | "dirty" | "detached" = detached ? "detached" : "clean"; |
There was a problem hiding this comment.
[/diagnosing-bugs] process.cwd() inside a tool handler always returns the CLI's working directory, not the git repository root. On any invocation where the repo root differs from CWD — or in future non-stdio transports — this heuristic will misclassify the main worktree as "linked".
💡 Fix: use git to identify the main worktree
The porcelain output already marks the main worktree (the first block has no worktree prefix before the path); use that structural signal instead of CWD comparison:
// In the agent instructions: track whether the current block is the first one
// and pass an `isFirst` boolean to classifyWorktree.
const type: "main" | "linked" | "bare" = bare ? "bare" : isFirst ? "main" : "linked";Alternatively, git worktree list marks the main worktree as the one with no gitdir field — use that from the parsed output.
| try { | ||
| const content = await readFile(filePath, "utf-8"); | ||
| const typeofCount = (content.match(/\btypeof\s+\w+\s*[=!]==/g) ?? []).length; | ||
| const instanceofCount = (content.match(/\binstanceof\b/g) ?? []).length; |
There was a problem hiding this comment.
[/diagnosing-bugs] The typeof regex /\btypeof\s+\w+\s*[=!]==/g only matches === and !==, so it misses the common patterns typeof x == 'string' (double equals) and typeof x === 'string' with a string literal on the right (the == char class [=!] is followed by = but typeof x === "number" ends in three = signs — confirmed match). More critically, it won't match typeof x !== 'string' because !== ends in = not = after !=.
💡 Fix: broaden the pattern to match both operand styles
// Match: typeof x === ..., typeof x !== ..., typeof x == ..., typeof x != ...
const typeofCount = (content.match(/\btypeof\s+\w+\s*[!=]==?/g) ?? []).length;Also consider counting all typeof occurrences rather than only narrowing guard forms, since typeof x in non-comparison contexts (e.g. logging) won't be captured.
| } | ||
| const totalSections = Object.keys(sections).filter((k) => k !== "__default__").length; | ||
| const totalKeys = Object.values(sections).reduce((sum, s) => sum + Object.keys(s).length, 0); | ||
| const result: Record<string, Record<string, string>> = {}; |
There was a problem hiding this comment.
[/diagnosing-bugs] Variable name s in the reduce callback shadows the imported s schema helper from "rig". TypeScript will not catch this because s inside the callback refers to the callback parameter, but it creates a confusing naming collision in a file that prominently uses s.* for schemas.
💡 Fix: rename the reduce accumulator
const totalKeys = Object.values(sections).reduce((sum, kvs) => sum + Object.keys(kvs).length, 0);Using sum for the accumulator avoids the shadow entirely.
| parameters: { filePath: s.path }, | ||
| handler: async ({ filePath }: { filePath: string }) => { | ||
| const content = await readFile(filePath, "utf-8"); | ||
| const inputsMatch = content.match(/workflow_dispatch:\s*\n(?:\s+.*\n)*?\s+inputs:([\s\S]*?)(?=\n\w|\n\s{0,2}\w|$)/); |
There was a problem hiding this comment.
[/diagnosing-bugs] The inputsMatch regex is fragile: it requires workflow_dispatch: to be followed immediately by inputs: within a narrow look-ahead, and uses a fixed indentation depth of 4–8 spaces for inputPattern. Real-world workflow files often use 2-space indentation (GitHub's default), which will be silently missed, returning { inputs: {}, inputCount: 0 } for valid workflow files.
💡 Suggestion: use a YAML parser or loosen the indentation constraint
The simplest fix is to widen the indent range in inputPattern:
const inputPattern = /^\s{2,}(\w+):\s*\n((?:\s{3,}.+\n?)*)/gm;A more robust fix would be to use a lightweight YAML parser (e.g. js-yaml) instead of regex, which is explicitly what the sample's docstring implies the agent does. Since this is a sample, a comment noting the limitation is also acceptable.
| return { columnName, type, uniqueCount, min, max, mean }; | ||
| } | ||
| return { columnName, type, uniqueCount, min: undefined, max: undefined, mean: undefined }; | ||
| }, |
There was a problem hiding this comment.
[/codebase-design] analyzeColumn returns { min: undefined, max: undefined, mean: undefined } for non-numeric columns, but the output schema declares these as s.optional(s.number). In s.object, s.optional means the key may be absent — not that it may be undefined. Returning explicit undefined values may cause schema validation to fail depending on how the serializer handles them.
💡 Fix: omit the keys entirely for non-numeric columns
// Instead of: return { columnName, type, uniqueCount, min: undefined, ... }
return { columnName, type, uniqueCount };Omitting optional keys is the correct pattern with s.optional; explicit undefined is only safe if the runtime strips it before validation.
1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
One failure was encountered and fixed before writing the sample file:
Task 2 (372-workflow-input-validator): The local type annotation
{ default?: string }conflicted withexactOptionalPropertyTypes: truewhen assigningstring | undefined. Fixed by widening to{ default?: string | undefined }.Tasks run