[rig-sampler] feat(s): add s.nullable schema helper for T | null types - #32
Conversation
Adds NullableSchema type and s.nullable(inner, description?) helper that:
- Serializes to {anyOf: [inner, {type: "null"}]} in JSON Schema
- Validates that values are either null or match the inner schema
- Infers to InferSchema<Inner> | null in TypeScript
- Supports an optional description parameter
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 and /grill-with-docs — requesting changes on one correctness gap and three documentation/robustness items.
📋 Key Themes & Highlights
Key Themes
- Schema-symbol invariant broken (correctness):
s.nullable()returns a plain object that doesn't carrySCHEMA_SYMBOL, violating theINV:schema-symbolinvariant that every schema object must satisfy.isSchema()will returnfalsefor nullable schemas, which could cause silent failures in any code path that guards on the symbol. - Structural type discriminant is fragile:
InferSchemaandvalidateSchemause anullable: trueproperty check rather than a unique symbol, making the discriminant broader than necessary — unlikes.optionalwhich usesOPTIONAL_SYMBOL. - Missing test coverage: no test for
s.nullablecomposed withs.optional, and no test fors.nullablewrapping complex types likes.objectors.array. - SKILL.md not updated: the schema helper reference table lists all other
s.*helpers but omitss.nullable.
Positive Highlights
- ✅ Clean
anyOfserialization — the correct JSON Schema idiom that LLMs understand natively - ✅ Five well-structured tests covering the happy path and the rejection case
- ✅ Consistent API surface: optional
descriptionparameter mirrors every others.*helper - ✅ Good JSDoc with
@exampleblocks - ✅ The inline doc comment on line 31 was updated to mention
s.nullable
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 44.6 AIC · ⌖ 5.27 AIC · ⊞ 6.3K
Comment /matt to run again
| type UnwrapOptional<T> = Omit<T, typeof OPTIONAL_SYMBOL>; | ||
| export type OptionalSchema<Inner extends Schema = Schema> = Inner & OptionalMarker; | ||
| export type NullableSchema<Inner extends Schema = Schema> = { nullable: true; inner: Inner; description?: string }; | ||
|
|
There was a problem hiding this comment.
[/grill-with-docs] NullableSchema is a plain object shape, unlike every other Schema member which carries SCHEMA_SYMBOL via markAsSchema. This means isSchema(s.nullable(s.string)) returns false, and any guard that relies on the schema-symbol invariant (INV:schema-symbol, line 54) will silently miss nullable values.
Suggested fix
Call markAsSchema the same way s.optional does:
nullable<Inner extends Schema>(schema: Inner, description?: string): NullableSchema<Inner> {
const obj = description !== undefined
? { nullable: true as const, inner: schema, description }
: { nullable: true as const, inner: schema };
return markAsSchema(obj) as NullableSchema<Inner>;
}| const inner = serializeSchema((schema as NullableSchema).inner); | ||
| return withDescription({ anyOf: [inner, { type: "null" }] }); | ||
| } | ||
| if ("enum" in schema) { |
There was a problem hiding this comment.
[/tdd] The serializeSchema description extraction uses a cast (schema as { description?: string }) because NullableSchema stores description on itself, not via the shared description?: string that most schema types carry. This cast will silently break if the property is ever renamed or moved. Consider adding description?: string directly to the Schema base union (or an interface) so the cast isn't needed.
| T | PromptIntent | PromptBuilder; | ||
|
|
||
| export type InferSchema<T> = | ||
| T extends { nullable: true; inner: infer Inner extends Schema } ? InferSchema<Inner> | null : |
There was a problem hiding this comment.
[/tdd] The InferSchema branch for nullable checks structural duck-typing ({ nullable: true; inner: infer Inner }). Any object literal that coincidentally has nullable: true and inner will match — a potential false positive. s.optional avoids this via a unique symbol (OPTIONAL_SYMBOL). Consider adding a NULLABLE_SYMBOL guard the same way to make the type discriminant robust.
Why this matters
TypeScript structural typing means { nullable: true, inner: s.string, someExtraField: 1 } would match the InferSchema branch, which could produce confusing inferred types for unrelated objects that happen to have these keys.
| it("rejects invalid values that are neither null nor inner type", async () => { | ||
| mocks.setSendAndWaitImpl(async () => JSON.stringify({ score: "not-a-number" })); | ||
| const a = agent({ output: s.object({ score: s.nullable(s.number) }), maxTurns: 1 }); | ||
| await expect(a("")).rejects.toBeInstanceOf(AgentError); |
There was a problem hiding this comment.
[/tdd] Missing edge case: s.nullable nested inside s.optional (and vice versa). Both modifiers interact at the validation level — optional short-circuits on undefined, nullable short-circuits on null. A combined test like s.object({ x: s.optional(s.nullable(s.number)) }) would confirm the composability of the two helpers and prevent a silent regression if either dispatch order changes.
| } | ||
| if ("enum" in schema) { | ||
| const enumValues = schema.enum as readonly unknown[]; | ||
| const allStrings = enumValues.length > 0 && enumValues.every((v) => typeof v === "string"); |
There was a problem hiding this comment.
[/grill-with-docs] SKILL.md documents every s.* helper in the schema reference table (lines 130–146) but s.nullable is not listed there. Callers reading the docs won't discover the new helper until they inspect the source. A one-liner addition to the table (s.nullable(inner) / s.nullable(inner, "desc")) would bring docs in sync with the implementation.
Sample run
Sample:
src/samples/12-security-scan-review.tsThe sample ran in 1 turn with no repair loops, producing valid JSON output. It used
s.optional(s.string)for theversionfield in a structured output schema. The run succeeded cleanly.What the run revealed
While reviewing the existing schema helpers after this run, I noticed there was no clean way to express
T | nulltypes — a very common pattern in real API responses where a field might be absent asnullrather than omitted. Currently, sample authors must either uses.unknown(too loose) ors.enum(null, ...)(semantically wrong). This is a schema-fit gap that can cause unnecessary repair loops when models returnnullfor optional-like fields.The improvement
Added
s.nullable(inner, description?)— a new schema helper that:{anyOf: [{...inner}, {type: "null"}]}in JSON Schema, which models understand nativelynullor match the inner schema (rejects other types with a clear error)InferSchema<Inner> | nullin TypeScript, giving callers proper type safetydescriptionparameter, consistent with others.*helpersThe implementation is self-contained: a new
NullableSchema<Inner>type, additions to theSchemaunion,InferSchema,serializeSchema, andvalidateSchema, plus thes.nullableexport with JSDoc.Tests added
Five new tests in
src/rig.test.tscovering JSON Schema serialization, description passthrough, null acceptance, non-null acceptance, and rejection of invalid types.