feat(core): add an overridable seam for completed schema validation - #1491
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9da0c64cb
ℹ️ 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".
groksrc
changed the title
Validation is the only place that knows whether a note actually satisfies its schema. `validate_note` is called from exactly one place -- the schema router -- the write path never runs it, and no validation state is stored on the entity. A deployment that needs to react to a validation result therefore has no seam at all today. Its only alternative is to re-run validation itself, which means owning a copy of this router's schema resolution and having that copy drift from the answer the API actually returned. That is the same trap `on_accepted_mutation` was added to close for accepted writes. `SchemaValidationObserver` is a no-op in core, provided through `SchemaValidationObserverDep` so a deployment can override it the way it overrides any other dependency. What it receives is deliberately narrower than the report. A `ValidatedNoteOutcome` carries the note's external id, the schema it was checked against, and whether it passed -- no field names, values, warnings or error text, and no title. An observer can act on which schema was satisfied without coming to depend on note content. The endpoint returns from three branches -- one note, one type, every schema-covered type -- so all three now route through `_observed`, and the observer is told once per request whatever the scope. A seam covering only some branches would fire on one scope and silently miss the others. Notes whose frontmatter resolves to no schema are skipped here exactly as they are skipped in the report, rather than arriving as unvalidated passes. Unlike the write hook there is no transaction and nothing to make atomic, so the docstring says plainly that raising fails the caller's validation request, and that an implementation owns its own durability and failures. Each guard verified load-bearing: dropping the batch outcomes, hardcoding `passed=True`, and removing the observer call each fail exactly the test written for them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015SkS3AAxWzHdgrVy7VWBUc Signed-off-by: Drew Cain <groksrc@gmail.com>
`just typecheck` runs `ty`, which requires @OverRide on a method that overrides a base-class method. Caught by CI's Static Checks; I had run pyright locally, which does not enforce it. Signed-off-by: Drew Cain <groksrc@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015SkS3AAxWzHdgrVy7VWBUc
ba267c8 to
e6c3259
Compare
|
Reviewed commit Low — the missing-note exit skips the observer. The new Suggested fix: route that return through Otherwise, the change looks sound: outcomes match the report, dependency overrides are straightforward, and the default observer preserves existing behaviour. No significant correctness or maintainability problems found beyond this small contract gap. Verification: 323 API, dependency, and architecture tests passed, plus the targeted missing-note reproduction. No repository code changed. I would be comfortable merging after the small contract fix. |
Two review findings on the validation seam. **The once-per-request contract had a fourth exit.** When `identifier` resolves to no note the endpoint returned an empty report directly, without notifying. That is the same empty report the note-type branch produces for an empty type, so the contract silently meant "once per request, unless you asked by identifier". The existing empty-result test covered the note-type case and sat next to the gap without catching it. **`schema_entity` does not identify a schema.** It is copied from `SchemaDefinition.entity`, which comes from the schema note's own `entity:` frontmatter and names the note type the schema covers. Two schema notes may both declare `entity: person` and both report `person`, so an observer could not tell which authoritative schema produced the result -- while the docstring claimed exactly that capability. `ValidatedNoteOutcome` now carries both fields, because they answer different questions. `schema_entity` stays the covered type; `schema_reference` is what the validated note pointed at, the string in its own `schema:` frontmatter, and None when the schema was inline and there was nothing to point at. The docstring says which is which, and says plainly that the reference is the reference as written and matched rather than a stable id -- the schema note's external id is not available here without reworking the resolver, and implying otherwise would be worse than the limitation. Verified load-bearing: bypassing `_observed` on the new exit fails its regression test, and falling the reference back to the covered entity fails the two tests that distinguish them. Signed-off-by: Drew Cain <groksrc@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015SkS3AAxWzHdgrVy7VWBUc
1 participant
Why
Validation is the only place that knows whether a note actually satisfies its schema:
validate_noteis called from exactly one place in this repo,api/v2/routers/schema_router.pyRuntimeAcceptedNoteResponsecarriesnote_type,entity_metadata, observations and relations, but no validation resultSo a deployment that needs to react to a validation result has no seam. Its only alternative is to re-run validation itself, which means owning a copy of this router's schema resolution (
_resolve_schema_for_api,_schema_frontmatter_from_file) and letting that copy drift from the answer the API actually returned. That is the same trapon_accepted_mutation(#1483) was added to close for accepted writes.This is needed by Basic Memory Cloud for SPEC-87 Mission 5 ("structure knowledge with schemas"), whose contract is a validation result from the authoritative schema service. The Cloud side follows separately and is blocked on this.
What
SchemaValidationObserver— a no-op in core, provided throughSchemaValidationObserverDepso a deployment overrides it like any other dependency.What it receives is deliberately narrower than the report.
ValidatedNoteOutcomecarries the note's external id, the schema it was checked against, and whether it passed. No field names, values, warnings or error text, and no title. An observer can act on which schema was satisfied without coming to depend on note content — and the note is named by external id rather than by a title that is note content.All three exits report. The endpoint returns from three branches — one note, one type, every schema-covered type — so each now routes through
_observedand the observer is told once per request whatever the scope. A seam covering only some branches would fire on one scope and silently miss the others. Notes whose frontmatter resolves to no schema are skipped here exactly as they are skipped in the report, rather than arriving as unvalidated passes.No transaction semantics, and the docstring says so. Unlike the write hook there is nothing to make atomic — validation reads. Raising fails the caller's validation request, which is virtually never the right trade for bookkeeping the user did not ask for, so an implementation owns its own durability and its own failures.
Testing
tests/api/v2/test_schema_router.py: 30 passed (5 new).The new tests cover a passing note reported by external id, a strict-mode failure reported as failing (asserted against the report's own
passed, not just againstFalse), an empty validation still reporting once with no outcomes, all three scopes reporting exactly once, and core's default behaviour being unchanged without an override.Each guard verified load-bearing by breaking it and confirming the intended test fails: dropping the batch
outcomesargument, hardcodingpassed=True, and removing the observer call.Wider suite:
tests/api,tests/test_deps.py,tests/test_architecture_boundaries.py— 323 passed.pyrightreports the same 4 pre-existing errors on these files with and without this change (unrelatedResolvedRelationTargetvariance in the router).🤖 Generated with Claude Code