Skip to content

fix(#285): consolidate MemoryEntry models and strict schemas#300

Merged
prakashUXtech merged 6 commits into
qbtrix:devfrom
sshekhar563:fix/issue-285-memory-entry-schema
Jul 24, 2026
Merged

fix(#285): consolidate MemoryEntry models and strict schemas#300
prakashUXtech merged 6 commits into
qbtrix:devfrom
sshekhar563:fix/issue-285-memory-entry-schema

Conversation

@sshekhar563

Copy link
Copy Markdown
Collaborator

Resolves #285

Description

This PR consolidates the dual MemoryEntry models 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

  • Model Consolidation: Moved the comprehensive MemoryEntry and its dependent types (MemoryType, MemoryCategory, MemoryProvenance, SomaticMarker) from runtime/types.py to spec/memory.py.
  • Strict Schema Enforcement: Made MemoryEntry.type optional to support modern layer-first initialization. Added a strict Pydantic model_validator and a root-level json_schema_extra anyOf constraint to guarantee that cross-language clients must provide either a type or a non-empty layer.
  • Duplicate Removal: Removed the duplicated MemoryVisibility definition from runtime/types.py.
  • Backward Compatibility: Preserved imports in runtime/types.py by securely re-exporting all migrated models so that runtime dependencies remain unaffected.
  • Spec Visibility: Explicitly re-exported the newly migrated spec models in spec/__init__.py.
  • CI Drift Prevention: Regenerated the schemas/ directory to match the new source of truth and introduced test_schema_drift() in test_schemas.py, a rigorous byte-for-byte CI check that guarantees generated schemas stay synced with on-disk schemas.

Verification

  • All 24 dynamic schema generation and drift tests pass.
  • Targeted memory model tests in test_spec_memory_model.py and test_layer_strings.py pass cleanly.
  • Verified the generated JSON schema correctly outputs the anyOf block enforcing the type or layer constraint.

@prakashUXtech prakashUXtech left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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's anyOf validates 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 .soul round-trip so back-compat is proven rather than reasoned.

  2. There is a breaking change worth documenting. Content-only construction now raises, and the runtime id default changed from "" to a random uuid, so any if not entry.id sentinel logic behaves differently now. Both are defensible, but call them out in the PR body and CHANGELOG.

  3. spec/SOUL-FORMAT-SPEC.md section 6.4 is not updated and now contradicts the model: it still marks type required, is missing social, 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.

  4. Small: the spec/container.py:28 docstring example still shows MemoryEntry(content="Hello!"), which now raises; and the file headers on spec/memory.py and runtime/types.py want 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.

@prakashUXtech

Copy link
Copy Markdown
Contributor

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:

  1. Testing the new behavior. A few times the existing tests got patched to keep passing rather than a new test added that would fail without your change. The fastest way to make a fix bulletproof, and to breeze through review, is to write the test that reproduces the bug first, watch it fail on the old code, then make it pass. Fix/graph recall rebind 281 #295's recall test is a great example of exactly that: it only passes because the fix is there.

  2. The paper trail. Two conventions we hold to: bump the Updated: <date> (#NN) line in the top comment of any file you change, and update the human-facing doc in the same PR as the behavior (cli-reference, the spec doc, api-reference, encryption.md, whichever applies).

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 prakashUXtech left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 prakashUXtech left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@prakashUXtech
prakashUXtech merged commit 31e10ea into qbtrix:dev Jul 24, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants