fix(#285): consolidate MemoryEntry models and strict schemas#300
Conversation
prakashUXtech
left a comment
There was a problem hiding this comment.
The hard part here is right. You consolidated to one canonical MemoryEntry in spec/ with the full field set, made runtime/types.py re-export it (which fixes the rerank.py annotation for free), regenerated the schemas with social in the enum and the RBAC fields present, and the new test_schema_drift is a real guard that regenerates and diffs rather than just checking the files parse. I also traced that real .soul files still deserialize, so existing souls are safe. Genuinely good work on a tricky change.
A few things to close before merge:
-
The "strict" behavior has no direct test. Nothing asserts that
MemoryEntry(content="x")(no type, no layer) now raises, that a layer-only or type-only entry is accepted, or that the schema'sanyOfvalidates a good doc and rejects a bad one. Right now the existing tests were patched to keep passing, so the new constraint itself is unverified. For a PR titled "strict schemas," that is the gap to close, plus one legacy-shaped.soulround-trip so back-compat is proven rather than reasoned. -
There is a breaking change worth documenting. Content-only construction now raises, and the runtime
iddefault changed from""to a random uuid, so anyif not entry.idsentinel logic behaves differently now. Both are defensible, but call them out in the PR body and CHANGELOG. -
spec/SOUL-FORMAT-SPEC.mdsection 6.4 is not updated and now contradicts the model: it still markstyperequired, is missingsocial, and lists only the old ~15 fields. Since #285 is fundamentally about the cross-language contract, the human spec needs to match the schemas you regenerated. -
Small: the
spec/container.py:28docstring example still showsMemoryEntry(content="Hello!"), which now raises; and the file headers onspec/memory.pyandruntime/types.pywant an(#285)Updated line.
One heads-up unrelated to the code: a few personal files (a resume, a .codex/config.toml) showed up in the branch history and were removed in a later commit. The net diff is clean and a squash-merge collapses to it safely, so no harm done, but it is worth being careful about what gets staged, and this one should be squash-merged (not rebased) when it lands.
|
Zooming out from the individual reviews for a second, one small pattern across your recent PRs (#294, #295, #299, #300, #301), meant as a shortcut rather than a knock. The code has been consistently solid, and the review notes keep landing in the same two places:
Fold those two habits in as you go and most of my "before merge" asks just disappear. The engineering itself has been genuinely good and you have turned feedback around fast, so this is about shaving the last lap off each PR. Thanks for all of these. (Cross-PR note, posted here on #300 since it is the most active thread.) |
1. Added tests asserting type/layer strictness and uuid defaults 2. Synced SOUL-FORMAT-SPEC.md 6.4 with v0.4/0.5 model changes 3. Documented breaking behavior in CHANGELOG 4. Fixed container docstring example and bumped Updated headers
prakashUXtech
left a comment
There was a problem hiding this comment.
All four asks from last round are resolved, and resolved well. The strict behavior now has a real test (test_content_only_raises uses pytest.raises(ValueError, match="type.*layer"), which genuinely fails without the validator), the type-only and layer-only accept paths are covered, the CHANGELOG has a Breaking section for both changes (content-only now raising, and the id default going from "" to a random hex), SOUL-FORMAT-SPEC section 6.4 is synced to the model, and the container.py example plus the file headers are fixed. Legacy back-compat is well covered by the existing (unmodified) layer tests, so real souls still awaken.
One new thing this round that CI doesn't catch, which is why I'd not merge on green alone: spike/memory_journal.py:417 still constructs MemoryEntry(..., timestamp=..., ...). The consolidation turned timestamp into a read-only property, and Pydantic v2 silently ignores a kwarg that names a property, so created_at falls back to now() and every journal-reconstructed entry loses its stored creation time. It stays green because recall orders by SQL seq, not timestamp, and nothing asserts timestamp fidelity. One-line fix: timestamp= becomes created_at=. It's experimental spike/ code and data-fidelity only, so if you consider spike/ out of scope this flips to approve, but the migration should really be complete.
Two smaller follow-ups, non-blocking: rfc/RFC-004-SOUL-FILE-FORMAT.md:170 still shows MemoryEntry(content="Hello!"), the same example you fixed in container.py, so it now raises if copied; and a jsonschema-based test that the published anyOf accepts a good doc and rejects a content-only one would close the last testing gap (the drift test proves the schema is published, not that it validates correctly).
The consolidation itself is solid and merge-ready once the spike line is fixed.
prakashUXtech
left a comment
There was a problem hiding this comment.
Scoping spike/ out for this PR per maintainer call, so this is good to merge. The consolidation is solid: one canonical model, regenerated schemas with a real drift guard, strict-behavior tests that fail without the validator, and back-compat proven by the existing legacy tests. The one-line spike/memory_journal.py:417 fix (timestamp= to created_at=) and the RFC-004:170 example are tracked as a follow-up so they aren't lost. Thanks for the thorough iteration.
Resolves #285
Description
This PR consolidates the dual
MemoryEntrymodels into a single canonical source of truth in the spec layer and enforces a stricter cross-language JSON schema contract for memory entries.Key Changes
MemoryEntryand its dependent types (MemoryType,MemoryCategory,MemoryProvenance,SomaticMarker) fromruntime/types.pytospec/memory.py.MemoryEntry.typeoptional to support modern layer-first initialization. Added a strict Pydanticmodel_validatorand a root-leveljson_schema_extraanyOfconstraint to guarantee that cross-language clients must provide either atypeor a non-emptylayer.MemoryVisibilitydefinition fromruntime/types.py.runtime/types.pyby securely re-exporting all migrated models so that runtime dependencies remain unaffected.spec/__init__.py.schemas/directory to match the new source of truth and introducedtest_schema_drift()intest_schemas.py, a rigorous byte-for-byte CI check that guarantees generated schemas stay synced with on-disk schemas.Verification
test_spec_memory_model.pyandtest_layer_strings.pypass cleanly.anyOfblock enforcing thetypeorlayerconstraint.