feat(server): strict response validation default in dev/test (#727)#757
Merged
Conversation
createAdcpServer defaults responses: 'strict' when NODE_ENV !== 'production' (was 'warn'), turning handler-returned schema drift into VALIDATION_ERROR with the offending field path. Production default stays 'off'. Opts out in three internal test files that use deliberately sparse handler fixtures for middleware coverage; schema-validation-server.test.js still exercises the validator itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
force-pushed
the
bokelley/issue-triage
branch
from
71a1fd1 to
7325457
Compare
code-reviewer: - clarify JSDoc that 'warn' still logs; 'strict' promotes to VALIDATION_ERROR - idempotency.release() catch now logs (parity with post-handler path) - test helper shallow-merges `validation` so a per-test override on one side doesn't silently clear the file-level opt-out on the other - add dispatcher-level tests asserting the NODE_ENV-driven default flip - changeset calls out NODE_ENV=undefined landing in the dev/test bucket security-reviewer: - gate `VALIDATION_ERROR` `details.issues[].schemaPath` behind `exposeErrorDetails` (same policy as SERVICE_UNAVAILABLE.details.reason); prod responses no longer fingerprint the handler's internal `oneOf` branch selection. Buyers keep pointer/message/keyword — enough to fix a drifted payload without revealing AJV branch state - replay-loop finding filed as #758 (pre-existing; affects warn mode too, deserves its own circuit-breaker-vs-cache design) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Rebased on main and addressed expert review feedback in 6dfcb53. code-reviewer (PR doc + correctness):
security-reviewer (details.issues[].schemaPath leak):
debugger passed the PR clean — no regressions, no timing-sensitive interactions with
|
Merged
3 tasks
bokelley
added a commit
that referenced
this pull request
Apr 22, 2026
…slim on creative-agent (#762) * docs: surface response contracts per tool in docs/llms.txt Each tool section now emits a "Response (success branch)" block with the required + optional fields, mirroring the existing request block. Pulled straight from the bundled JSON schemas; zero new source of truth. Closes the drift path matrix runs kept surfacing — agents dropped required response fields (missing format_id on creative_manifest, plural hallucinations like creative_deliveries for creatives, missing top-level currency) because the skill prose documented intent but the full per-field contract lived in the generated schemas and was never indexed in the llms.txt file Claude actually reads. Anchored sections let skills point at docs/llms.txt#<tool> instead of embedding the contract inline — keeps skills lean. * docs(skills): point at llms.txt for field contracts; slim creative-agent Every build-*-agent skill's "Tools and Required Response Shapes" section now opens with a canonical-contracts pointer directing Claude at the anchored llms.txt section for per-tool request + response fields — one source of truth, refreshed on every schema sync. build-creative-agent gets a surgical slim: the verbose response-shape blocks (which duplicate llms.txt verbatim now that #761 landed) are collapsed into a compact handler-binding table. Gotchas and anti- patterns stay; field enumerations move to llms.txt. Net -94 lines, same signal. The other 7 skills get the pointer line only — follow-up passes can do per-skill surgical slims once the pattern is validated. With strict response validation default in dev (#727/#757), drift between a skill example and the schema now fails immediately at the call site with the exact field path. The skill's job is to document patterns/gotchas; the schema's job is to enforce shape. This separation was muddied when every skill tried to re-state the shape inline. * docs(skills): imperative callout + anchor column per expert review Experts flagged the passive "contracts live at X" wording — Claude doesn't follow pointers without an imperative trigger. Rewrote to lead with "Before writing any handler's return statement, fetch docs/llms.txt and grep for #### \`<tool_name>\`" and reframed the validation safety-net from threat to permission. Applied to all 8 skills. Added a per-row Contract column to the creative-agent handler-binding table with direct anchor links — more reliably followed than a general pointer three lines up. Regenerated schemas/types (upstream context-object shift) and widened si-session-manager.ts generateInitialResponse signature from string to unknown so the overrides accept the new ContextObject shape. Unblocks CI's Validate generated files check.
Merged
This was referenced Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
createAdcpServer({ validation: { responses } })now defaults to'strict'whenNODE_ENV !== 'production'(was'warn'). Handler drift fails withVALIDATION_ERRORcarrying the field pointer instead of silently logging a warning.'off'. Explicit per-call config still wins.validation: { responses: 'off' }) because they use deliberately sparse fixtures to exercise envelope/state-store/idempotency middleware — schema conformance isn't their unit under test.test/lib/schema-validation-server.test.jscontinues to cover the validator itself.Why
The
compliance:skill-matrixharness keeps surfacingSERVICE_UNAVAILABLEfrom agents that returned spec-incomplete responses (see the test-agent-team review on #725). The dispatcher's response validator is the right layer — it catches drift once for every tool with a clear field pointer. Making it strict by default in dev/test catches drift while the handler is being written instead of in a downstream consumer.Closes #727(A). Part (B) (tightening generated discriminated unions in the types generator) remains open.
Test plan
npm test— 5135 pass / 0 fail (baseline before flip: same count)test/lib/schema-validation-server.test.jsstill asserts the three modes (strict/warn/off) behave as documentednpm run compliance:skill-matrixand confirm any residual drift surfaces as a single validator error per tool (the measurement the issue called for)🤖 Generated with Claude Code