fix(storyboard): add pattern validation checks#2122
Conversation
There was a problem hiding this comment.
Two blockers. The library/CLI change ships without a changeset, and the drift detector is half-wired for the new payload check kind. Both are mechanical fixes — the actual implementation is sound.
Blockers
-
Missing changeset.
src/lib/testing/storyboard/validations.tsandsrc/lib/testing/storyboard/types.tsare published via the./testingexport.Check for changesetis already failing CI.npm run changeset→minor(additiveStoryboardValidationCheckunion members + newpattern?: stringfield onStoryboardValidation). -
Drift detection collects payload
field_patternand drops it.test/lib/storyboard-drift.test.js:235extendscollectFieldValidationsto gather payloadfield_patternentries, but the three reachabilitydescribeblocks at L325 (field_present), L398 (field_value), and L419 (field_value_or_absent) each filter strictly on their own check kind. The envelope-variant block at L371-377 correctly addedenvelope_field_pattern. Payloadfield_patternis collected and silently discarded — the moment a storyboard authorsfield_pattern { path: 'creative.bogus_url' }, a typo'd path passes drift detection. Add a mirror block:```js
describe('field_pattern paths are reachable in response schemas', () => {
const patternValidations = fieldValidations.filter(v => v.check === 'field_pattern');
// …same body as the field_value block at L398-417
});
```
Things I checked
- `validateFieldPattern` shape matches sibling validators — `resolvePath` + `toJsonPointer`, `actual ?? null` for missing fields, echoes `check` so reporters distinguish payload vs envelope variants. Same envelope as `validateFieldValue` (validations.ts:837-903).
- `scripts/conformance-replay.ts:336-362` inline implementation is in lockstep with the SDK helper — same empty-pattern reject, same invalid-regex reject, same non-string typeof reporting. The inline pattern is the existing convention in this script (`field_value`, `array_length` are inlined the same way). Acceptable as long as the two sides stay in sync; flag if a future PR drifts one without the other.
- `ProtocolEnvelopeSchema.merge(AdCPVersionEnvelopeSchema)` is valid. Both are `z.object(...).passthrough()`, the field sets are disjoint (envelope: `status`/`task_id`/`message`/`replayed`/etc. vs version envelope: `adcp_version`/`adcp_major_version`), no silent override.
- The version-negotiation integration test at L171-178 actually proves what the PR claims — asserting both `validation.not_applicable === undefined` and `result.validations_not_applicable === undefined` confirms `envelope_field_pattern` hits the explicit switch case and not the forward-compat fallback.
- ReDoS surface acceptable. Storyboard YAML is authored content, runner is a test harness, no adopter-traffic path.
Follow-ups (non-blocking)
- Asymmetric `?? null` in misconfiguration branches. validations.ts:1047 uses `actual: validation.pattern ?? null`; the invalid-regex branch at 1065 uses `actual: validation.pattern` (no `?? null`). The latter is guarded to be a non-empty string, so the `?? null` would be a no-op — but the asymmetry reads as a thinko. Match the two for grep symmetry.
- Stale assertion message at storyboard-drift.test.js:270. Says `'Expected at least one field_present, field_value, or field_value_or_absent validation'` — doesn't mention the pattern variants. Cosmetic.
Re-request review once the changeset lands and the payload-pattern reachability block is added.
There was a problem hiding this comment.
LGTM. Closes a real gap in the storyboard validation surface — runners were grading author-supplied field_pattern checks not_applicable under the forward-compat default, so storyboards asserting "looks like a release-precision version string" were silently passing. Fix-shaped, wired through all four enforcement seams (dispatcher, drift detection, conformance replay, version-negotiation runner), with the failure-shape matrix tested end-to-end.
Things I checked
validateFieldPattern(src/lib/testing/storyboard/validations.ts:1020-1106) — five failure modes branch correctly: missing path, missing/empty pattern, invalid regex source, missing/non-string value, mismatch. JSON pointer emitted on every failure path.expected: { pattern }shape consistent with the rest of the runner-output contract.- Dispatcher wiring at
validations.ts:256-273— bothfield_patternandenvelope_field_patternroute tovalidateFieldPattern. Envelope case-block fall-through still resolves correctly today. - Drift detection (
test/lib/storyboard-drift.test.js:22-25) —ProtocolEnvelopeSchema.merge(AdCPVersionEnvelopeSchema)widens the reachability set to includeadcp_version. The prior comment claimingadcp_versionis "NOT an envelope field" is now contradicted; the adcp#5195 reference is the right justification. - Conformance replay (
scripts/conformance-replay.ts:333-362) — same branching asvalidateFieldPattern, distinct shape because replay emitsfailures.push(string)notValidationResult. - Test coverage — 10 new cases on the validations file (pass / mismatch / missing / non-string / no-pattern / invalid-regex ×
field_patternandenvelope_field_pattern) plus the version-negotiation integration test provingnot_applicableshort-circuit no longer fires. - ReDoS surface — patterns are storyboard-author-supplied and run only in CI, not on hot adopter paths. Acceptable.
Follow-ups (non-blocking — file as issues)
- Changeset type.
patchis debatable. Two members added to an exportedStoryboardValidationCheckunion — additive, not breaking, but storyboard authors using exhaustiveswitchon the union will notice.minorwould match howarray_lengthand the envelope-prefixed checks landed. Not blocking; consumer-side narrowing stays safe. - Replay-script duplication.
conformance-replay.ts:336-362reimplements the same misconfig / invalid-regex / wrong-type branching asvalidateFieldPattern. Different output shape justifies separate sites, but a sharedcompilePattern(pattern): { re?: RegExp; error?: string }helper would keep error wording in lockstep across the two seams. - No regex flag support. A storyboard wanting case-insensitive match can't get it. Out of scope here; flag for a later minor that introduces
pattern_flags?: stringwith an allowlist (i/m/sonly — nog).
Minor nits (non-blocking)
- Positional fall-through at
validations.ts:267.return validateFieldValueOrAbsent(...)is correct today becauseenvelope_field_value_or_absentis the only case left in the block. Anyone adding a future envelope kind to the union without a matchingifarm above will silently route tovalidateFieldValueOrAbsent. Replace with an explicitif (validation.check === 'envelope_field_value_or_absent') return validateFieldValueOrAbsent(...)and let the switch'sdefault(already an exhaustiveness-style forward-compat) carry the unknown-kind case. actual: actual ?? nullatvalidations.ts:1082. Collapses explicit-null and undefined to the sameField not foundbranch (the=== undefined || === nulltest at L1077 already does this). Consistent with the existingfield_present/field_absentconvention — leave it, but a one-line comment noting "explicit null is reported as missing per convention" would save the next reader a trip.
Notable that the PR title/changeset both use fix(storyboard): while strictly adding new API surface — fits how this fills a gap the runner already half-supported, but the changeset-versioning purists will note it.
LGTM. Follow-ups noted.
There was a problem hiding this comment.
LGTM. Closes a real gap — the runner's forward-compat fallback was silently grading field_pattern assertions not_applicable, so "seller echoes release-precision adcp_version like 3.1-rc.3" was passing on every implementation regardless of what the wire said. Fix is downstream of upstream spec adcp#5195 (runner-output-contract v2.5.0), which authoritatively adds the two new check kinds and reclassifies adcp_version as an envelope field. SDK is implementing the spec, not ahead of it.
Things I checked
validateFieldPatternatsrc/lib/testing/storyboard/validations.ts:1020-1106— five failure modes branch cleanly (missing path / missing-or-empty pattern / invalid regex / non-string / mismatch), JSON pointer emitted on every failure path,expected: { pattern }shape consistent with runner-output-contract v2.5.0.- Dispatcher refactor at
validations.ts:252-273— positional fall-through replaced with explicit case arms (addressed a prior reviewer's note). Read line-by-line, no missingbreak, no typos. ProtocolEnvelopeSchema.merge(AdCPVersionEnvelopeSchema)attest/lib/storyboard-drift.test.js:24-25— envelope schemas atsrc/lib/types/schemas.generated.ts:2325-2342have disjoint keys (status/task_id/etc. vsadcp_version/adcp_major_version),.merge()is safe. Theerrorslives-in-payload invariant comment is preserved.scripts/conformance-replay.ts:333-362mirrors the SDK helper on all five failure modes — same empty-pattern reject, same invalid-regex reject, same non-string reporting. Output shape diverges as expected (replay emitsfailures.push(string)notValidationResult).- Drift detector now has a payload-
field_patternreachabilitydescribeblock attest/lib/storyboard-drift.test.js:419-440— verbatim mirror of thefield_valueblock. Earlier iteration collectedfield_patternand dropped it; that's now closed. - Integration test at
test/lib/storyboard-version-negotiation.test.js:107-180asserts bothvalidation.not_applicable === undefinedANDresult.validations_not_applicable === undefined— proves the new kind hits the explicit switch case, not the forward-compat fallback. That's the actual behavior change this PR claims. - Changeset is
minor. Correct: two additive members on the exportedStoryboardValidationCheckunion plus one new optionalpattern?: stringfield. Aligns with howarray_lengthand the envelope-prefixed kinds landed. - Witness-not-translator invariant intact — no wire fabrication, no normalization, no envelope synthesis. Test-runner-only addition.
Follow-ups (non-blocking — file as issues)
- Replay-script duplication.
conformance-replay.ts:333-362reimplements the same misconfig / invalid-regex / wrong-type branching asvalidateFieldPattern. Different output shape justifies separate sites today, but a sharedcompilePattern(pattern): { re?: RegExp; error?: string }helper would keep the two seams in lockstep. Flag the next PR that touches one without the other. - No regex flags. Storyboards can't author case-insensitive or multiline matches. Out of scope here — the spec yaml describes
patternas bare regex source, so this matches the upstream contract. If a future spec rev adds flags, bothvalidations.tsandconformance-replay.tsneed updating in lockstep.
Minor nits (non-blocking)
- Array-type reporting drift.
validateFieldPatternatvalidations.ts:1079reports arrays as'array'via an explicitArray.isArraycheck;conformance-replay.ts:356falls through to baretypeofand reports them as'object'. Cosmetic — pass/fail decision is identical — but the next person to grep failure messages will notice.
Interesting choice to ship purely additive surface under a fix(...) title and commit message — the changeset type and the prior bot's "fits the gap" framing both land on the right shape, but the convention purists will note it.
LGTM. Follow-ups noted.
Adds
field_patternandenvelope_field_patternstoryboard validations with regex compilation, missing-field handling, and non-string failure reporting.Extends conformance replay and storyboard drift detection so the new checks are enforced consistently, including version-envelope fields such as
adcp_version.Adds regression coverage for payload fields, envelope fields, invalid patterns, replay drift, and version negotiation.
Validated with
npm run build:lib, focused storyboard tests,git diff --check, and the pre-push typecheck/build hook.