[rig-tasks] Add 10 rig samples — 2026-08-01 - #327
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 — requesting changes on correctness and API misuse issues found in 3 of the 10 samples.
📋 Key Themes
Correctness issues
- 346:
classifyWorktreehandler can never return"dirty"— the enum value is declared in the output schema and the agent description promises it, but the tool always falls through to"clean". This makes the sample demonstrably wrong. - 342:
p.writeOutput("outputPatch", "outputPatch")treats an input field name as a literal file path, so the patch is always written to a file calledoutputPatchrather than the caller-supplied path. - 344: Same
p.writeOutput/ fixed-path mismatch, and the output schema redundantly echoes back the hard-coded path as if it were a variable.
API confusion
- 345: The
.some((p) => ...)arrow parameter shadows the importedptemplate tag — confusing for readers learning from this sample.
Design observation
- 341:
p.bash("npm test ...")reruns the test suite on every invocation. A triage agent reading an existing log file would be a better-fit pattern.
Positive highlights
- ✅ Consistent use of
s.enumwithas constin handler return values - ✅ Good use of
repair()addon on agents whose output needs to be well-formed - ✅
349asyncdefineToolwithnode:fs/promisesis a clean pattern worth preserving - ✅
347two-p.bashpattern (ignored + tracked) nicely demonstrates multiple prompt intents
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 44.4 AIC · ⌖ 4.69 AIC · ⊞ 6.3K
Comment /matt to run again
| handler: (args) => { | ||
| const status = args.isBare ? "bare" as const | ||
| : args.isDetached ? "detached" as const | ||
| : "clean" as const; |
There was a problem hiding this comment.
[/codebase-design] The classifyWorktree handler never returns "dirty", yet the output schema declares s.enum("clean", "dirty", "bare", "detached") and the file title promises "clean, dirty, bare, or detached". When a worktree is neither bare nor detached the handler always returns "clean", silently misclassifying dirty worktrees.
💡 Suggested fix
Add isDirty: s.boolean to the tool parameters so callers can pass git-status info:
parameters: s.object({
path: s.string,
branch: s.optional(s.string),
isDetached: s.boolean,
isBare: s.boolean,
isDirty: s.boolean, // ← add
}),
handler: (args) => {
const status = args.isBare ? "bare" as const
: args.isDetached ? "detached" as const
: args.isDirty ? "dirty" as const
: "clean" as const;
return { status };
},| }), | ||
| }), | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/codebase-design] p.writeOutput("outputPatch", "outputPatch") passes the literal string "outputPatch" as the file path, but the intent is to write to a caller-supplied path. This means the file will always be written to a file literally named outputPatch in the working directory, ignoring the outputPatch input field.
💡 Suggested fix
Use p.writeInput("outputPatch", ...) to write to the path from the input field, or use a concrete path like "config.patch" if the output path is always fixed:
// option A: caller-supplied output path
${p.writeInput("outputPatch", "patchContent")}
// option B: fixed path
${p.write("config.patch", "...")}See p.writeInput(pathField, contentField) for the pattern where both path and content come from input fields.
|
|
||
| Return the list of extracted APIs, count of changes applied, and the output path.`, | ||
| agents: { apiExtractor, proseCleanup }, | ||
| output: s.object({ |
There was a problem hiding this comment.
[/codebase-design] p.writeOutput("outputPath", "refactored.md") writes to the hard-coded path "refactored.md", but then the output schema returns outputPath: s.string as if it is a variable value. The output field will always be the literal string "refactored.md" — the field in output is redundant and misleading.
💡 Note
Either remove outputPath from the output schema (since the path is fixed), or make it a real input field and use p.writeInput("outputPath", ...) so callers can control the destination. Returning a constant value in a structured output field is noise that could confuse consumers of this sample.
| (p) => args.args.includes(p) | ||
| ); | ||
| const cacheable = args.instruction !== "RUN" || !isHeavy; | ||
| const tip = isHeavy |
There was a problem hiding this comment.
[/codebase-design] The local variable p in the isHeavy predicate shadows the imported p (prompt template tag) from "rig". While it works at runtime, it is confusing and fragile — readers scanning the sample will see p used as both a string and a prompt helper.
💡 Suggested fix
Rename the predicate parameter to avoid the collision:
const isHeavy = ["npm install", "apt-get", "pip install", "COPY . ."].some(
(pattern) => args.args.includes(pattern)
);| const ciFlakeTriager = agent({ | ||
| model: "small", | ||
| instructions: p`You are a CI flake triager. Analyze the following test output and workflow config to classify the failure. | ||
|
|
There was a problem hiding this comment.
[/codebase-design] p.bash("npm test 2>&1 | tail -100") will actively run the test suite when this agent is invoked, not just read a log. For a triage-only agent this is likely undesirable — it triggers side effects and could take a long time. The sample would be more accurate if it reads an existing log file (e.g. p.readOptional("test-output.log") or a path from input).
💡 Alternative
input: s.object({ logFile: s.string }),
instructions: p`...
Test output:
${p.readInput("logFile")}
...`This makes the agent a pure analyzer rather than a test runner, which is the correct pattern for a triage tool.
1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.Typecheck failures
None — all 10 samples passed typecheck on first attempt.
Tasks run