[rig-sampler] Add s.percent schema helper for percentage values (0–100) - #167
Conversation
Adds a constrained number schema s.percent that validates values in the range [0, 100] inclusive. This fills a gap revealed by running samples 41-45: sample 43 (snapshot-test-updater) and sample 41 (parse-coverage) both represent percentage fields as plain s.number, missing the semantic constraint that percentages must stay within 0-100. 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 /tdd, /codebase-design, and /grill-with-docs — commenting with non-blocking improvement suggestions.
📋 Key Themes & Highlights
Key Themes
- Motivating samples not updated: Samples 41 and 43 are cited in the PR as the reason for
s.percent, but still uses.numberfor percentage fields — the validation gap the PR aims to close remains open there. - SKILL.md not updated: The prompt-context documentation doesn't surface
s.percentin its helper table or prose, so AI agents generating Rig programs won't know to prefer it. - Test boundary precision: Rejection tests use integers only; adding fractional near-boundary values (e.g.
100.001) would give stronger coverage confidence.
Positive Highlights
- ✅ Clean additive change — follows the established
createConstrainedNumberSchemapattern exactly - ✅ Good JSDoc comment explaining motivation and usage
- ✅ Well-structured tests: serialization + acceptance range + rejection, matching the pattern of
s.positiveInt/s.nonNegativeInt - ✅ Non-breaking: no existing API is modified
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 33.7 AIC · ⌖ 4.38 AIC · ⊞ 6.3K
Comment /matt to run again
| expect(above.ok).toBe(false); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The rejection tests use integers only (-1, 101). Adding fractional near-boundary values would confirm the JSON Schema maximum/minimum exclusion works precisely.
💡 Suggested additions
expect(analyzeResponse(JSON.stringify(100.001), s.percent, "test", 1).ok).toBe(false);
expect(analyzeResponse(JSON.stringify(-0.001), s.percent, "test", 1).ok).toBe(false);JSON Schema minimum/maximum are inclusive, so these verify the validator does not drift at float precision.
| @@ -253,6 +253,8 @@ export const s = { | |||
| positiveInt: createConstrainedNumberSchema<IntegerSchema>({ type: "integer", minimum: 1 }), | |||
| /** Schema for a non-negative integer (minimum: 0). Call as `s.nonNegativeInt` or `s.nonNegativeInt("description")`. */ | |||
| nonNegativeInt: createConstrainedNumberSchema<IntegerSchema>({ type: "integer", minimum: 0 }), | |||
There was a problem hiding this comment.
[/codebase-design] The PR description explicitly identifies samples 41 and 43 as the motivation for s.percent, but neither sample was updated to use it. The helper provides value only when callers adopt it — leaving the motivating samples unchanged means the validation gap described in the PR body persists.
💡 What to update
In sample 41 (41-parse-coverage.md):
// Before
lines: s.number,
branches: s.number,
// After
lines: s.percent,
branches: s.percent,In sample 43 (43-snapshot-test-updater.md): the generated sample doesn't seem to include percentage fields, but the PR description says overallPct: s.number — confirm which sample file contains it and update accordingly.
Additionally, samples 123, 143, and 153 each have overallPct: s.number and coverageByCategory: s.record(s.number) — these are candidates for s.percent and s.record(s.percent) as well.
| @@ -30,7 +30,7 @@ | |||
| * T:JsonSchemaObject type {[key:string]:unknown} plain JSON Schema object | |||
There was a problem hiding this comment.
[/grill-with-docs] The header docstring and rig.ts JSDoc were updated, but SKILL.md's schema helper table (around line 62) still lists only s.number and doesn't mention s.percent, s.positiveInt, or s.nonNegativeInt. Agents using SKILL.md as prompt context won't know to prefer s.percent over s.number for percentage fields.
💡 Suggested SKILL.md addition
In the schema helpers table, add a row or note like:
| Constrained numbers | `s.positiveInt`, `s.nonNegativeInt`, `s.percent` (0–100) |
And in the prose guidance (around line 70), add: "Use s.percent instead of s.number for percentage fields to enforce the 0–100 range."
Samples run
41-parse-coverage.tss.record(s.object({lines: s.number, branches: s.number, ...})). Coverage percentages expressed as plains.number— no range constraint.42-json-repair.ts43-snapshot-test-updater.tss.enum("green","yellow","red")for rating andoverallPct: s.numberfor coverage percentage — the number field lacks a 0–100 constraint that would catch model hallucinations.44-flaky-test-analysis.ts45-code-owner-suggestion.tsAll five samples completed in 1 turn with no repair loops.
Improvement:
s.percentBoth sample 41 (
lines,branchescoverage fields) and sample 43 (overallPct) uses.numberfor percentage values. A percentage is semantically constrained to [0, 100], buts.numberaccepts any floating-point value — the harness will silently accept102.5or-5without a validation error or repair turn.This PR adds
s.percent: a constrainedNumberSchemawithminimum: 0andmaximum: 100.What changed
skills/rig/rig.ts: Addeds.percentviacreateConstrainedNumberSchema<NumberSchema>({ type: "number", minimum: 0, maximum: 100 })alongside the existings.positiveIntands.nonNegativeInthelpers. Updated the file-header docstring to include it.src/rig.test.ts: Added adescribe("s.percent")block with serialization, acceptance (0, 50.5, 100), and rejection (−1, 101) tests.The change is additive and non-breaking: no existing API is modified.