fix(memory): datetime-safe to_prompt_json shim — fixes second-ingest 502 (#169)

[jalaliamirreza]

Summary

Fixes #169 — POST /memory/episodes 502s on the second ingest touching any already-extracted entity that carries a LAW datetime attribute. graphiti-core 0.29.0's to_prompt_json calls json.dumps() with no default= handler; entity-summary regeneration (extract_attributes_from_nodes → _extract_entity_summaries_batch → to_prompt_json) serializes the entity's attributes dict and raises TypeError: Object of type datetime is not JSON serializable. Our LAW ontology has 16 datetime | None fields (Person.date_of_birth, Matter.engagement_letter_date, Signed.signed_at, …). First ingest = single write, no summary regen → JSON dump never runs → 200. Second ingest → regen runs → 502. Confirmed live on memory-v0.0.22 (staging), 2026-06-02.

Ships option B from the issue (short-term shim). Option C (upstream PR) remains tracked; this shim is retired when it lands + we bump the pin.

What lands (file-by-file)

• apps/memory/src/mayia_memory/graphiti_patches.py (new) — install_datetime_safe_to_prompt_json() rebinds every loaded graphiti_core to_prompt_json reference onto a json.dumps(..., default=str) wrapper. A sys.modules sweep is required because from .prompt_helpers import to_prompt_json copies the reference into ~7 consumer modules (incl. the summarize_nodes crash site), so patching only the canonical module misses them. Identity-idempotent (value is _STOCK_TO_PROMPT_JSON) and re-runnable.
• graphiti_patches.journey.md (new) — surface, locked root cause, sweep rationale, J1–J5.
• graphiti_setup.py — installs the shim in _build_graphiti() (after telemetry opt-out, before any add_episode); WARNs if the sweep rebinds 0 references (a moved bind site on a future bump would otherwise silently reintroduce the 502).
• pyproject.toml — graphiti-core pin now documents the shim upgrade gate.
• docs/infra/cloudbuild-deploy.md — (unrelated, pre-existing) tag-triggered (server-v*/memory-v*) deploy docs, bundled at the author's request.

Tests

• test_graphiti_patches.py (8): bug characterization (stock fn raises), canonical + full import-site sweep, summarize_nodes crash site, default=str output, unicode/indent preservation, idempotency.
• test_graphiti_setup.py: J5 wiring test (_build_graphiti installs the shim once).
• Coverage: graphiti_patches.py 100%; memory suite 225 passed, 15 skipped, 92.9% total (gate 90%). Ruff clean.
• TDD lifecycle: journey → tests → RED (ImportError) → GREEN → refactor → coverage → python-reviewer.

Test plan

• uv run pytest — 225 pass, 92.9% ≥ 90%
• uv run ruff check && ruff format --check
• Pre-PR parallel review: security (APPROVE, 0 findings) + architect (APPROVE, 2 MEDIUM applied)
• Staging smoke: re-run the real second-ingest that 502'd on memory-v0.0.22

Depends-on / Blocks

None. Self-contained.

Out of scope

• Option A (LAW datetime → str | None) — rejected; loses Pydantic datetime parsing across 16 fields.
• Option C (upstream graphiti-core PR) — tracked separately.
• graphiti-core version bump; broader ontology audit.

[cloudflare-workers-and-pages]

Deploying mayia-staging with    Cloudflare Pages

Latest commit:	e8a6860
Status:	✅  Deploy successful!
Preview URL:	https://103ecb91.mayia-staging.pages.dev
Branch Preview URL:	https://fix-issue-169-to-prompt-json.mayia-staging.pages.dev

View logs

[claude]

Code Review

Overview

Well-scoped fix for the confirmed second-ingest 502 (issue #169). The sys.modules sweep approach is correctly reasoned — a single rebind of prompt_helpers.to_prompt_json would leave all consumer modules pointing at the buggy original. Root cause documentation, TDD lifecycle, and coverage (100% on the new patch file, 92.9% suite total) are all solid.

Findings

1. Test mock triggers the warning log path (minor)

In test_build_graphiti_installs_datetime_patch, the installer is mocked with return_value=0, which causes _build_graphiti to fire the _LOG.warning("...sweep rebound 0 references...") branch. The assertion only checks call_count == 1 so the test passes, but every run emits a WARNING. Use return_value=7 (or any positive int) to exercise the happy path and keep the test output clean.

2. Generator fixture annotated as -> None (minor)

Pytest accepts generator fixtures fine but the return annotation should be Generator[None, None, None] (or Iterator[None] from collections.abc). Ruff/mypy will flag this under strict mode.

3. Journey.md says "guard flag" but implementation uses identity comparison

The journey.md describes idempotency as "a guard flag means a second call is a no-op" — but there is no explicit flag. Idempotency is achieved by _is_original (identity comparison against _STOCK_TO_PROMPT_JSON). This is a correct and cleaner mechanism; the doc just names it wrong. Change to "identity comparison against the stock function" to avoid confusing the next reader.

4. default=str blesses all future non-serializable types (noted, not blocking)

default=str is the right short-term choice for datetimes, but it will also silently coerce bytes, Decimal, custom objects, etc. if any land in a future ontology attribute. This is documented in the PR and acceptable for a shim — just ensure the upgrade-gate test (test_stock_to_prompt_json_chokes_on_datetime) is the go/no-go for the bump, not a broader type-safety audit.

What's Good

• Vacuous-pass guard in test_install_rebinds_every_loaded_import_site (assert len(bound_modules) >= 2)
• The characterization test doubles as a retirement flag for when upstream fixes the bug
• list(sys.modules.values()) snapshot prevents RuntimeError from dict mutation during sweep
• WARN on 0 rebinds converts a silent prod regression into an alertable log line
• _reset_for_testing is correct: resets before and after each test for full isolation

Verdict

Approve with the return_value=0 → non-zero and fixture annotation fixes applied before merge. Finding 3 is a doc-only change. Finding 4 is noted for the upgrade gate, not a blocker.

[claude]

Code review — PR #172 (datetime-safe to_prompt_json shim)

Overall: clean, targeted bug fix. The sys.modules sweep rationale is sound, the identity-idempotent design is correct, and the test suite covers all five journeys including the characterization guard and the WARN-on-zero-rebinds path. One test correctness bug to fix before merge; one cleanup suggestion.

---

Bug — record.message is not reliably populated by pytest's caplog handler

File: apps/memory/tests/test_graphiti_setup.py, line 290

assert any("rebound 0 references" in record.message for record in caplog.records)

logging.LogRecord.message is only set as an instance attribute after Formatter.format(record) is called (it runs record.message = record.getMessage()). pytest's LogCaptureHandler.emit() appends the record to self.records without calling format(), so record.message may not exist and the expression raises AttributeError depending on the pytest version's internal emit path.

The project already uses the correct idiom in test_audit_callback.py — record.getMessage() — and pytest's own docs point to caplog.messages for this use case.

Fix (pick one):

# option A — consistent with test_audit_callback.py
assert any("rebound 0 references" in r.getMessage() for r in caplog.records)

# option B — idiomatic pytest
assert "rebound 0 references" in caplog.text

---

Cleanup — duplicate sweep loop between install_ and _reset_for_testing

File: apps/memory/src/mayia_memory/graphiti_patches.py

Both functions share the same list(sys.modules.values()) + startswith("graphiti_core") iteration skeleton; the only difference is the per-module predicate. A shared _iter_graphiti_to_prompt_json_refs() helper (three lines) would make the prefix string a single change point — relevant because that string is what the sweep gate depends on. Not blocking.

---

Ordering invariant (informational — no change needed)

The sweep relies on graphiti_core.prompts.__init__ having been eagerly loaded before _build_graphiti() is called, which is guaranteed by the module-level from graphiti_core import Graphiti in graphiti_setup.py. This is documented in graphiti_patches.journey.md but not at the call site. A one-line comment in _build_graphiti() noting the dependency would protect against a future lazy-import refactor silently narrowing the sweep.

---

No security or multi-tenant findings — the shim touches only apps/memory internals and has no bearing on org_id/workspace_id filtering.

[claude]

Code Review — PR #172

Overall: LGTM with minor notes. The monkey-patch approach is the right call — sweeping sys.modules is justified because from .prompt_helpers import to_prompt_json copies the reference into ~7 consumer modules independently. The idempotency design, rebound==0 WARN, characterization test as a retirement sentinel, and <0.30.0 pin together form a solid lifecycle. Three findings below; none are blockers.

---

Medium — default=str emits space-separated datetimes, not RFC 3339

File: graphiti_patches.py, line 68

str(datetime(1980, 3, 15, tzinfo=UTC)) → "1980-03-15 00:00:00+00:00" (space separator).
datetime.isoformat() → "1980-03-15T00:00:00+00:00" (T separator, RFC 3339).

The docstring correctly says "ISO-8601-ish" and today the output only goes to LLM prompts, so it is not a live bug. But every other datetime serialisation in apps/memory (search.py:122, episodes.py:195) uses .isoformat(). If graphiti-core ever re-parses the serialised attributes dict (dedup, entity comparison), the space form could behave differently on older Python. The inconsistency is worth closing:

# Instead of:
return json.dumps(data, ensure_ascii=ensure_ascii, indent=indent, default=str)

# Prefer (matches search.py pattern; handles non-datetime non-serialisables too):
return json.dumps(data, ensure_ascii=ensure_ascii, indent=indent,
                  default=lambda v: v.isoformat() if hasattr(v, 'isoformat') else str(v))

---

Low — Generator fixtures annotated -> None

Files: test_graphiti_patches.py line 23, test_graphiti_setup.py lines 22 and 36

All three yield-bearing fixtures are typed -> None. The correct annotation is -> Generator[None, None, None] (from collections.abc). Pytest handles it fine at runtime; no type-checker currently enforces it in this package. Worth fixing if mypy/pyright is ever added.

---

Low — _is_original helper is asymmetric with _reset_for_testing

File: graphiti_patches.py lines 71–75

install_datetime_safe_to_prompt_json calls _is_original() but _reset_for_testing does the inverse check inline (getattr(module, "to_prompt_json", None) is _datetime_safe_to_prompt_json). The helper abstracts one direction but not the other, forcing the reader to cross-reference. Small clarity win: inline the comparison in install_* (matching _reset_for_testing's style) or add a symmetric _is_patched helper.

---

Confirmed non-issues (investigated and ruled out):

• rebound-0 WARN firing falsely in tests: REFUTED — every _build_graphiti test path mocks install_datetime_safe_to_prompt_json
• Wrapper signature mismatch: REFUTED — graphiti-core 0.29.x to_prompt_json signature is byte-identical to the wrapper
• _STOCK_TO_PROMPT_JSON import-order fragility: REFUTED — graphiti_patches.py's own module-level from graphiti_core.prompts import prompt_helpers triggers the eager sub-module load before the capture

[claude]

Code review round 4 (HEAD: 26c9d09)

Overall: Solid fix. sys.modules sweep is the right approach, all prior round findings addressed (isoformat, getMessage, return_value=7, _is_original symmetry). Coverage 92.9% >= 90% gate. One new finding:

BUG: unused monkeypatch fixture in test_build_graphiti_installs_datetime_patch (apps/memory/tests/test_graphiti_setup.py)

monkeypatch: pytest.MonkeyPatch is declared but never used — all patching is via patch.object() context managers. Remove it:

async def test_build_graphiti_installs_datetime_patch(
_reset_graphiti_singleton: None,
) -> None:

Dead dependency — pytest instantiates it for nothing and misleads readers.

PROCESS: staging smoke test still unchecked in the test plan. Confirm before merge — it is the only path that exercises _extract_entity_summaries_batch -> to_prompt_json against production graphiti-core.

[jalaliamirreza]

Round 3 response (HEAD a29da25)

• Unused monkeypatch fixture — fixed in a29da25; removed the dead dependency from test_build_graphiti_installs_datetime_patch.

• Staging smoke (process note) — agreed it's the only path that exercises _extract_entity_summaries_batch → to_prompt_json against real graphiti-core, but it can't be a pre-merge gate here: apps/memory staging is INGRESS_TRAFFIC_INTERNAL_ONLY and this code isn't live until a memory-v* tag deploys. Plan: after merge + tag, re-run the exact double-ingest that 502'd on memory-v0.0.22 (real user import 2026-06-02) and confirm 200/200. Leaving the test-plan box unchecked until that post-deploy smoke passes — I won't mark it done from unit coverage alone.

Unit layer covers the failing function directly (test_install_rebinds_summarize_nodes_crash_site rebinds the exact summarize_nodes reference in the crash traceback; _json_default matches the upstream fix #1527). The root cause is also fixed upstream: getzep/graphiti#1527.

[claude]

Code Review — PR #172

Overall: The shim design is sound — sys.modules sweep for identity-idempotent monkey-patching is the right approach for this class of bug. Tests are thorough (J1–J5, vacuous-pass guard on J2, idempotency proof). Two findings worth addressing before merge.

---

Finding 1 — MEDIUM: False-positive WARNING on every retry after a failed cold-start

File: apps/memory/src/mayia_memory/graphiti_setup.py

install_datetime_safe_to_prompt_json() runs before the env-var checks in _build_graphiti(). When a required env var is missing (e.g. NEO4J_PASSWORD not yet injected at Cloud Run startup):

1. First call: patch installs successfully (returns 7) → logs INFO → then raises RuntimeError → _INSTANCE stays None
2. Every subsequent retry: _build_graphiti() runs again; sweep finds all references already rebound → returns 0 → logs WARNING "rebound 0 references — graphiti-core may have moved the bind site; issue #169 502 risk"

The 502 risk is absent (patch succeeded on call 1), but the WARNING fires on every request during a misconfigured cold-start, burying the real RuntimeError in noise and potentially triggering a false alert.

Fix: differentiate "already installed (idempotent)" from "nothing to patch at all" — e.g. return (rebound, already_installed) from install_datetime_safe_to_prompt_json(), or have _build_graphiti check for already-installed references before deciding whether to WARN:

rebound = install_datetime_safe_to_prompt_json()
if rebound:
    _LOG.info("memory: installed datetime-safe to_prompt_json (%d references)", rebound)
elif _patch_is_already_active():   # e.g. check any module's attribute
    _LOG.debug("memory: datetime-safe to_prompt_json already installed (idempotent)")
else:
    _LOG.warning(...)   # only genuine "nothing found" case

---

Finding 2 — LOW: J5 wiring test asserts call-count but not ordering

File: apps/memory/tests/test_graphiti_setup.py — test_build_graphiti_installs_datetime_patch

assert mock_install.call_count == 1 proves the function was called, but not that it was called before Graphiti.__init__. If a future refactor moves the install call to after Graphiti(...), this test still passes — defeating the invariant the comment describes ("BEFORE any add_episode runs").

A minimal improvement: use a side_effect on the Graphiti mock that asserts mock_install.call_count == 1 at the moment of construction.

---

Minor (no action required)

• graphiti_patches.py — duplicate loop: install_datetime_safe_to_prompt_json and _reset_for_testing share identical sys.modules iteration + prefix filter. A private _sweep_graphiti_modules() helper would reduce the duplication surface.
• isinstance(obj, (datetime, date)): datetime is a subclass of date; listing datetime separately is redundant since both branches call .isoformat(). isinstance(obj, date) alone is sufficient and clearer.
• Fixture annotation: _restore_to_prompt_json is typed -> None but uses yield — mypy strict mode will flag this; should be Generator[None, None, None] or Iterator[None].

[claude]

Code Review — PR #172

Verdict: Approve with minor fixes. The shim is well-designed and correctly addresses the root cause. The sys.modules sweep rationale is sound, the idempotency story is airtight, and the test coverage is thorough. Four small items below.

Findings

#	Severity	File	Description
1	Low	test_graphiti_patches.py:53	Docstring says "renders via str()" but implementation + assertion use .isoformat() — stale doc
2	Low	graphiti_patches.py:70	isinstance(obj, (datetime, date)) — datetime is a subclass of date, the datetime arm is unreachable independently
3	Low	test_graphiti_patches.py	No test exercises a bare date (non-datetime) value — silent regression risk if isinstance is narrowed
4	Trivial	test_graphiti_patches.py:47	match='datetime' in pytest.raises adds fragility to a characterization test without adding useful signal

No issues found in

• Multi-tenant correctness: no ACL surface touched
• Thread-safety: list(sys.modules.values()) snapshot + GIL-atomic attribute writes + asyncio.Lock in get_graphiti() serializes concurrent callers
• Idempotency: is _STOCK_TO_PROMPT_JSON identity check correctly prevents double-wrapping
• WARN gate: correct three-branch logic in _build_graphiti for moved-bind-site detection