Enforce v3 envelope integrity storyboard#4967
Conversation
There was a problem hiding this comment.
Approving. Closes the v3 envelope absence loop cleanly — task_status/response_status are now asserted absent at runtime, and the lint tightening keeps envelope_field_* honest about which schema it walks.
Things I checked
static/schemas/source/core/protocol-envelope.json:64-69carries thenot: { anyOf: [{ required: [task_status] }, { required: [response_status] }] }constraint. Storyboard now matches the schema; the TODO is retired correctly, not papered over.adcp_versionlives instatic/schemas/source/core/version-envelope.json, notprotocol-envelope.json. The flip tofield_presentinversion-negotiation.yaml:117is the right routing — onceenvelope_field_*was pinned toprotocol-envelope.json, the old check would have started failing. Pairing is coherent, not accidental.pathIsForbiddenByRequiredNotatscripts/lint-storyboard-validations-paths.cjs:310-340handles the exact top-levelnot.anyOf[].requiredpattern in the envelope and breaks$refcycles via theseenset.- Tests cover the three branching outcomes — envelope-absent acceptance, wrong-kind rejection, payload-path rejection — plus the contradictions classifier addition at
tests/lint-storyboard-contradictions.test.cjs:177. runner-output-contract.yamlbumped 2.2.0 → 2.3.0. Additive (new enum + expected/actual docs); the right semver call.
Follow-ups (non-blocking — file as issues)
- No changeset for the contract bump. Prior 2.1.0 → 2.2.0 shipped
.changeset/4418-runner-output-notices-field.md, andstatic/compliance/source/has been treated as changeset-eligible elsewhere (4547, 4399, 4339). Either add aminorchangeset, or document the carve-out so the next compliance-source PR knows it's intentional. storyboard-schema.yamlcheck-enum docs still omit plainfield_absenteven thoughrunner-output-contract.yamlnow lists it as an authored kind. Pre-existing drift, but this PR is the natural place to harmonize since the enum block is already being edited.
Minor nits (non-blocking)
- Dead-code arm at
scripts/lint-storyboard-validations-paths.cjs:404. The fall-throughif (v.check === 'field_absent' || v.check === 'envelope_field_absent')includes the envelope kind, but the routing block at L385-L402 unconditionallycontinues for both envelope kinds — that half of the||is unreachable. Either drop it or leave a one-line comment, otherwise the next reader will assume envelope-absent has a payload-schema fallback. pathIsForbiddenByRequiredNotis envelope-root only. Thesegments.length !== 1bail atscripts/lint-storyboard-validations-paths.cjs:310means nested paths silently won't get the forbidden-by-required-not exemption. Fine for today's usage; worth a one-line comment so a future caller doesn't assume deeper paths work.- Stray tab at
static/compliance/source/universal/runner-output-contract.yaml:266. Leading\ton theenvelope_field_absentactual-continuation comment; every other line in that block is space-indented. Doesn't break YAML parse, butgit diff --checkwill flag it.
A new check kind landing without a changeset, after the file's last bump shipped one, is notable. Worth picking a convention before the next bump.
LGTM. Follow-ups noted above.
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Clean activation of a previously stubbed invariant — the lint walker, storyboard, and runner contract all line up, and the envelope_field_present → field_present flip on adcp_version is the right shape because that field lives in core/version-envelope.json, not core/protocol-envelope.json.
Things I checked
scripts/lint-storyboard-validations-paths.cjs:268-310—pathIsForbiddenByRequiredNotwalksnot,not.anyOf,not.oneOfand recurses throughoneOf/anyOf/allOfand$refwith the sameseencycle guard pattern aspathResolves.segments.length !== 1is the right guard for the v3 invariant:protocol-envelope.json's top-levelnot: { anyOf: [{ required: [task_status] }, { required: [response_status] }] }only forbids top-level keys.scripts/lint-storyboard-validations-paths.cjs:404-407—forbiddenByEnvelopeis load-bearing, not dead. It's what stopscheck: field_absent, path: task_statusagainstget-adcp-capabilities-response.jsonfrom accidentally passing via the envelope'snotconstraint reachable throughallOf. The new test attests/lint-storyboard-validations-paths.test.cjs:234pins that behavior.static/compliance/source/universal/v3-envelope-integrity.yaml:17-23, 99-110— prose no longer mentions the prior TODO / "not yet shipped" status. Storyboard now asserts the full v3 invariant end-to-end againstprotocol-envelope.json:64-69.static/compliance/source/universal/version-negotiation.yaml:117—adcp_versionis defined only incore/version-envelope.json(self-described there as "part of the payload itself"), sofield_present(which walks the response body throughallOf) is correct andenvelope_field_presentwould have emitted a false negative.static/compliance/source/universal/runner-output-contract.yaml— 2.2.0 → 2.3.0 bump,envelope_field_absentadded toauthored_check_kinds,expected/actualsemantics documented for bothfield_absentandenvelope_field_absent. Forward-compat preserved — older runners grade unknown kindsnot_applicableper the contract's own rule..changeset/envelope-integrity-storyboard.mdpresent and describes the change accurately.- Three new lint tests cover the contract correctly: envelope-forbidden absence allowed, wrong-kind rejected, payload-path rejected.
Follow-ups (non-blocking — file as issues)
- Changeset level may be off. PR #4418 bumped the same contract 2.1.0 → 2.2.0 to add the
noticesblock and usedminor. This PR makes the directly analogous additive runner-surface change and is markedpatch. The change is forward-compatible (unknown kinds gradenot_applicable) so it doesn't break adopters, but the precedent points atminor. Either bump it or document the rationale in the changeset body. storyboard-schema.yaml:1045-1058enum prose is partial. The doc-comment now namesenvelope_field_present/envelope_field_absentbut still doesn't namefield_absent, even thoughfield_absentis in the canonicalauthored_check_kindslist. The file's own header says "Add new kinds to that enum AND document semantics here in the same PR" — pre-existing gap that was natural to close here.
Minor nits (non-blocking)
- Dead disjunct in the lint walker.
scripts/lint-storyboard-validations-paths.cjs:404—|| v.check === 'envelope_field_absent'is unreachable because the dedicated branch at L385-L401 alwayscontinues for that check kind. The inner narrow at L406 already matches only onfield_absent. Either drop the||or simplify the outer condition. - Stray TAB in an otherwise space-aligned comment block.
static/compliance/source/universal/runner-output-contract.yaml:266begins with a literal\tinside the newenvelope_field_absentdocumentation block this PR is editing; every other comment line in that file uses spaces. YAML still parses (the line is inside a comment), but it visibly skews in any editor with non-default tab width.
LGTM. Follow-ups noted below.
Summary
envelope_field_absentassertions inv3_envelope_integrityfor legacytask_status/response_statusfields.envelope_field_absentto the authored check contract and documents failureexpected/actualsemantics.core/protocol-envelope.json, while preserving response-schema forbidden-field support for plainfield_absent.version_negotiationto usefield_presentforadcp_version, which comes fromcore/version-envelope.json, notcore/protocol-envelope.json.Fixes #4930.
Tests
npm run test:storyboard-validations-pathsnpm run test:storyboard-check-enumnpm run test:storyboard-contradictionsTENANT_PATH=sales npx tsx server/tests/manual/run-storyboards.ts --filter v3_envelope_integritynpm run test:storyboardsnpm run test:storyboards:3.0-compatnpm run test:unit,npm run test:test-dynamic-imports,npm run test:callapi-state-change,npm run typecheck