[rig-tasks] Add 10 rig samples — 2026-07-28 - #250
Conversation
- 260-git-submodule-health: p.bash + p.readOptional + defineTool - 261-ts-decorator-scanner: p.bash + defineTool classifyDecorator - 262-dotfile-inventory: p.bash + defineTool + repair addon - 263-regex-pattern-tester: input schema + defineTool + repair addon - 264-commit-churn-classifier: p.bash + steering addon + s.record - 265-npm-package-size: p.bash + p.readOptional + defineTool - 266-tsconfig-option-analyzer: p.read + p.glob + defineTool + repair - 267-git-patch-summarizer: p.bash + defineTool + steering addon - 268-npm-lifecycle-analyzer: p.read + defineTool classifyScript - 269-ts-jsdoc-coverage: p.bash + async defineTool + repair addon 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 and /codebase-design — requesting changes on correctness issues in sample tool handlers.
📋 Key Themes & Highlights
Key Issues
- Incorrect API usage (263):
p.json("input")serializes the literal string"input", not the agent's runtime input — the correct approach is to rely on the framework's built-in input injection via the declaredinput:schema. - Unused parameter (268):
classifyScriptdeclares acommandparameter that is never read, inflating tool call tokens and misleading readers. - Broken regex (261): The property-decorator pattern ends with
manytobut TypeORM decorators areManyToOne/ManyToMany— themany...variants will never match. - Duplicated classification logic (264): The
steering()message and theinstructionstext both define the churn thresholds, which is the opposite of the addon's intended use (behavioural nudges, not rule repetition). - Brittle heuristic (269): JSDoc detection looks back only 2 lines, missing blank lines between the doc comment and the export declaration.
Positive Highlights
- ✅ Good variety of patterns:
p.bash,p.read,p.glob,p.readOptional,repair(),steering(), asyncdefineTool - ✅ All 10 samples passed typecheck
- ✅ 264 correctly uses
s.recordfor a dynamic-key output schema - ✅ 269 is the most advanced sample in the batch — async
defineToolwithnode:fs/promisesis a useful new pattern to document
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 59.3 AIC · ⌖ 7.54 AIC · ⊞ 6.3K
Comment /matt to run again
| instructions: p`Run each regex pattern against its test cases and report results. | ||
|
|
||
| Input patterns and test cases: | ||
| ${p.json("input")} |
There was a problem hiding this comment.
[/grill-with-docs] p.json("input") is incorrect here — p.json takes a JS value and serializes it, so passing the string "input" embeds the literal JSON string "\"input\"" in the prompt, not the agent's runtime input.
💡 How to reference structured input
The input: schema declared on the agent makes the input available to the model at runtime through the framework's own injection. You do not need to explicitly embed it via p.json. Instead, just describe how to use it in the instructions prose:
instructions: p`Run each regex pattern against its test cases.
Patterns and test cases are provided in the input above.
Use the runRegexTest tool for each pattern+testCase combination.`,If you do need to embed a static value, p.json(someVariable) works — but a string literal "input" is not the right argument.
| handler({ scriptName }) { | ||
| const name = scriptName.toLowerCase(); | ||
| const isHook = /^(pre|post)/.test(name); | ||
| if (/build|compile|bundle|webpack|rollup|esbuild/.test(name)) return { category: "build", isHook }; |
There was a problem hiding this comment.
[/grill-with-docs] The command parameter is declared in the tool schema but never used in the handler — classifyScript only reads scriptName. This makes the declaration misleading and the tool signature wider than it needs to be.
💡 Fix: remove the unused parameter
const classifyScript = defineTool("classifyScript", {
parameters: s.object({ scriptName: s.string }), // remove command
handler({ scriptName }) { ... },
});And update the instructions to match: "call classifyScript with just the script name". The command value is never consulted, so including it inflates token cost on every tool call without benefit.
| } | ||
| } | ||
| const total = documentedCount + undocumentedCount; | ||
| return { |
There was a problem hiding this comment.
[/codebase-design] The heuristic for detecting a JSDoc comment checks only whether the immediately preceding or pre-preceding line contains */. This fails for single-line /** ... */ JSDoc on the same preceding line as the function, and also for functions prefixed by blank lines or decorators.
💡 More robust lookback
A slightly wider scan (e.g., look back up to 3 lines for */) would handle the common case of a blank line between the doc comment close and the export declaration:
const prevLines = lines.slice(Math.max(0, i - 3), i);
if (prevLines.some(l => /\*\//.test(l))) {
documentedCount++;
} else {
undocumentedCount++;
}As a sample, this is fine for demonstration, but a note in the code (or in the markdown prose) clarifying the limitation would help readers understand why coverage numbers might be lower than expected.
| return { type: "method" }; | ||
| if (/^(column|primarycolumn|primarygeneratedcolumn|onetomany|manytoone|manyto)/.test(lower)) | ||
| return { type: "property" }; | ||
| if (/^(param|body|query|request|response|headers|ip|session|uploadedfile)$/.test(lower)) |
There was a problem hiding this comment.
[/grill-with-docs] The regex for property decorators (manyto suffix) has a typo — it tests for manyto but the actual TypeORM decorators are ManyToMany and ManyToOne. The pattern /^(column|...|manyto)/ will never match ManyToMany or ManyToOne because they end in many/one, not manyto.
💡 Fix the pattern
if (/^(column|primarycolumn|primarygeneratedcolumn|onetomany|manytoone|manytomany)/.test(lower))
return { type: "property" };As a sample demonstrating defineTool, the classification table is the core of the example — having a subtle bug here undermines its value as reference material.
| // Agent role: classify repository files by commit churn frequency and assign a risk level. | ||
| const commitChurnClassifier = agent({ | ||
| model: "small", | ||
| addons: steering({ message: "Assign riskLevel based on churnCount: >20=critical, >10=volatile, >5=active, else stable. Only include files from the churn output." }), |
There was a problem hiding this comment.
[/grill-with-docs] The steering() addon duplicates the risk-level classification logic that is already spelled out in the instructions text. Having two conflicting sources of truth (the prose in instructions vs the steering message) can confuse the model and cause inconsistent output.
💡 Pick one canonical source
Either put the thresholds in instructions (and drop the steering addon), or put them in steering alone and simplify the instructions. The steering() addon is most useful for behavioural nudges not already described in the instructions, not for repeating the same rule twice:
// Option A: instructions only (drop steering addon entirely)
instructions: p`...Assign riskLevel: critical >20, volatile >10, active >5, stable <=5.`,
// Option B: steering only (keep instructions brief)
addons: steering({ message: "riskLevel thresholds: >20=critical, >10=volatile, >5=active, <=5=stable." }),
instructions: p`Classify repository files by commit churn. Assign a riskLevel per file.`,1 participant
Summary
Added 10 new rig sample files to
skills/rig/samples/.p.bash+p.readOptional+defineToolp.bash+defineTool classifyDecoratorp.bash+defineTool+repair()addondefineTool+repair()p.bash+steering()addon +s.recordoutputp.bash+p.readOptional+defineToolp.read+p.glob+defineTool+repair()p.bash+defineTool+steering()p.read+defineTool classifyScriptdefineTool+node:fs/promises+repair()Typecheck failures
None — all 10 tasks passed typecheck on the first attempt.
Tasks run