fix(core): validate schema validation modes - #1223
Conversation
Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4efea0a40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2a823a9fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: phernandez <paul@basicmachines.co>
Why
Schema validation accepted arbitrary
settings.validationvalues and treated every value exceptstrictas warning-only. That made the naturalerrorspelling fail open: notes missing required fields still reportedpassed: true.The public schema contract documents two modes,
warnandstrict. The parser also has to enforce that contract after Markdown frontmatter loading, where YAML 1.1 tokens such as bareoffmay already have become non-string values. Invalid schema configuration must also remain actionable at the API boundary instead of surfacing as a generic server error.Closes #1222.
What Changed
warn | strictvocabulary.erroras a compatibility alias normalized to canonicalstrict.off, its YAML-loadedFalserepresentation, and every other unknown value.warnandstrictexhaustively when recording validation findings.Implementation Details
parse_validation_mode()narrows the untrusted YAML value once, soSchemaDefinition.validation_modeis always a typedValidationMode. The validator consumes that closed type with an exhaustive match andassert_never, preventing a future mode from silently degrading to warnings.The compatibility alias preserves the spelling already used by the cloud acceptance flow while keeping
strictas the documented canonical enforcing mode. The integration fixture deliberately uses barevalidation: off: PyYAML loads it asFalse, and schema parsing now rejects it with the accepted vocabulary instead of bypassing the boundary behavior in hand-built test dictionaries.The schema router catches parser
ValueErroronly around schema resolution and returns HTTP 400 with the actionable parser message. Other failures retain their existing error behavior.Testing
Automated
uv run pytest tests/picoschema/test_parser.py tests/picoschema/test_resolver.py tests/picoschema/test_validator.py test-int/test_picoschema/test_parser_integration.py test-int/test_picoschema/test_validator_integration.py -q: 138 passeduv run pytest tests/api/v2/test_schema_router.py tests/mcp/test_client_schema.py tests/mcp/test_tool_schema.py -q: 56 passedjust fast-check: passedjust package-check: passedjust doctor: passedgit diff --check: passedManual
91e16b77409e52aefff138f2dcbf6ce3c283dfb7.offguidance; the only bareoffis the intentional invalid integration fixture.Risks / Follow-ups
off, now fail at schema parse time instead of degrading to warnings. This is intentional alignment with the publicwarn | strictcontract but may expose previously unnoticed invalid schemas.