[rig-sampler] docs(rig): add JSDoc for p.var and p.region in PromptHelpers - #133
Conversation
Both helpers were publicly exported but undocumented in the PromptHelpers interface. Add @example annotations and prose descriptions consistent with the rest of the p.* family. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs — requesting one change for a correctness issue in the p.var JSDoc.
📋 Key Themes & Highlights
Issue Found
- Misleading storage claim in
p.varJSDoc: The description states the variable is "stored inPromptBuilder.vars", butp.var()(thePromptHelpershelper) only creates aPromptVariableobject and returns it — it never touches anyvarsMap. Thevarsregistration happens only viaPromptBuilder.var(). The@examplethen showsbuilder.get("content")which would returnundefinedwhen thep.var()path is used insidep\...`template interpolation, since the template engine routes throughrenderPromptPart, notbuilder.var()`.
Positive Highlights
- ✅
p.regionJSDoc is accurate — the description, note aboutPromptIntent/PromptBuilderpre-rendering, and@exampleexpansion all match the implementation. - ✅ Good initiative filling the documentation gap; the
p.regionaddition is immediately useful. - ✅ PR notes that all 228 tests pass and typecheck is clean.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 40.2 AIC · ⌖ 7.1 AIC · ⊞ 6.4K
Comment /matt to run again
| * const builder = p`Summarize: ${p.var("content", p.read("README.md"))}`; | ||
| * // builder.get("content") returns the PromptIntent for README.md | ||
| */ | ||
| var<T>(name: string, value: T): PromptVariable<T>; |
There was a problem hiding this comment.
[/grill-with-docs] The claim that p.var() stores in PromptBuilder.vars is inaccurate.
The JSDoc says "the name is stored in PromptBuilder.vars for later retrieval with .get(name)", but p.var() (the helper on line 1072–1074) only calls createPromptVariable and returns — it never writes to any vars map. That storage only happens in PromptBuilder.var() (line 1101). Crucially, when p.var(...) is used inside a p\...`template interpolation, the engine callsbuilder.write(value)→renderPromptPart, which handles PromptVariableby rendering its value directly —builder.var()is never called, so the variable is never registered in.vars. The @example's builder.get("content")call would returnundefined` in that pattern.
💡 Suggested correction
/**
* Creates a named prompt variable binding. When used inside a `p\`...\`` template
* interpolation the value is rendered inline. To also register the variable for
* later retrieval, call `builder.var(name, value)` directly on a `PromptBuilder`
* instance — the template tag's `write()` path does not invoke `builder.var()`.
*
* `@example`
* // Register and retrieve via PromptBuilder:
* const builder = p();
* builder.var("content", p.read("README.md"));
* builder.write(`Summarize: `, builder.get("content"));
* // builder.get("content") returns the PromptIntent for README.md
*
* // Or use inline (no retrieval):
* const prompt = p`Summarize: ${p.var("content", p.read("README.md"))}`;
*/
Samples run
Five consecutive samples were exercised through the stub runner (
npm run sample):26-design-review.tss.enum("approve","revise","reject")worked cleanly in 1 turn27-dependency-upgrade-plan.tsrisk: s.stringis loose but caused no repair in the stub run28-license-check.tsp.bash+s.boolean+ nested objects — all fine29-bug-report-draft.tsp.bashinput, clean schema30-github-action-review.tsWhat the runs revealed
All five samples completed in a single turn with no repair loops — the schemas are well-matched to their tasks. The runs confirmed that
s.enum(used in sample 26) prevents repair by constraining the decision field precisely.Reading
rig.tsafter the runs highlighted a small documentation gap: thePromptHelpersinterface fully documents everyp.*helper exceptp.varandp.region, which were exported with no JSDoc at all. IDE hover, generated API docs, and SKILL.md examples all surface these helpers, but users hovering over them in an editor get no description, parameters, or@example.Change
Added JSDoc to the
var<T>andregionsignatures in thePromptHelpersinterface inskills/rig/rig.ts:p.var(name, value)— explains the named binding concept, how the value is stored inPromptBuilder.vars, and when to use it vs plain interpolation.p.region(language, body)— explains fenced-block rendering, thatPromptIntent/PromptBuildervalues are resolved first, and shows the expansion in the@example.No behaviour changes. All 228 unit tests pass; typecheck clean.