[codex] Harden MCP multimodal media intake

[Atharva-Kanherkar]

Summary

• harden MCP multimodal intake with explicit media-size validation before persistence
• return clearer host-side errors when callers pass local paths the MCP server cannot read
• add MCP regression coverage for local screenshot, audio, video, and PDF paths plus inline materialization and oversized payload rejection

Why

Issue #62 requires MCP multimodal inputs that work without FastAPI uploads, fail clearly on remote hosts when local paths are invalid, and return durable stored paths instead of temporary blobs. The shared MCP intake layer already handled most of the data flow, but it was missing early size enforcement and clearer host-visibility errors for local paths.

Validation

• PYTHONPATH=. .venv/bin/pytest -q tests/test_mcp_server.py tests/test_mcp_schemas.py
• PYTHONPATH=. .venv/bin/pytest -q tests/test_mcp_server.py tests/test_mcp_schemas.py tests/test_api.py

Closes #62.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agentic-memory	Ready	Preview, Comment	Mar 30, 2026 11:18am

[Atharva-Kanherkar]

@claude Please review this PR throughly., If any manual tests or anything else is needed please ask. Please do not leave any decisison tree or dry runs. go to eeach line of code, do a dry run, make a dependency tree, dry run those codes, find bugs and tell me. Even if you think this might be not an issue, but dry run is telling yo,u, tell me,. USe a real productin case where you will make a test suite, dry run it, see the procvess, report if it breaks, and tell me again,

[claude]

Claude finished @Atharva-Kanherkar's task in 5m 29s —— View job

---

PR Review: [codex] Harden MCP multimodal media intake

Tasks:

• Read changed files (mcp_server/server.py, tests/test_mcp_server.py)
• Read dependency files (config.py, stores/episodic_store.py, stores/media_store.py, tests/helpers.py, mcp_server/schemas.py)
• Build full dependency tree
• Dry-run all code paths line-by-line
• Report all bugs and issues
• Run test suite (requires shell execution approval — static analysis complete)

---

Dependency Tree

MemoryMCPServer.__init__
├── app_config.MEDIA_EMBED_MAX_BYTES  → config.py (MEDIA_EMBED_MAX_BYTES = int(env, 20MB))
│
├── _prepare_media(media, requested_media_type)
│   ├── [file_path branch]
│   │   ├── path.exists() / path.is_file() / os.access()
│   │   ├── _validate_media_size(path=path, size_bytes=path.stat().st_size)  ← NEW
│   │   └── MediaStore.resolve_media_type(path, requested_media_type)
│   │       └── _EXTENSION_MEDIA_TYPES lookup → fallback mimetypes.guess_type
│   │
│   └── [inline_content branch]
│       ├── base64.b64decode(inline.inline_base64, validate=True)
│       ├── _validate_media_size(size_bytes=len(payload))  ← NEW
│       ├── suffix = mimetypes.guess_extension(mime) or _MEDIA_TYPE_SUFFIXES[type] or ""  ← NEW
│       ├── tempfile.NamedTemporaryFile(prefix="agentic_memory_mcp_", suffix=suffix)
│       └── MediaStore.resolve_media_type(path, requested_media_type)
│
├── _validate_media_size(size_bytes, path)  ← NEW METHOD
│   └── size_bytes <= self._max_media_bytes → OK, else raise MediaTooLargeError
│
└── remember_episode / remember_fact / remember_procedure
    └── runtime.episodic_store.store(record)
        ├── _ensure_owned_media(record)  → media_store.ensure_owned()  → shutil.copy2()
        └── _embed_record(record)
            ├── size_bytes > self._max_media_bytes → raise MediaTooLargeError  ← INDEPENDENT CHECK
            └── embedder.embed_image/audio/video/multimodal()

---

Bug #1 — HIGH: max_media_bytes not propagated to EpisodicStore

Location: server.py:206-208, server.py:199-208

# server.py
self._max_media_bytes = (
    app_config.MEDIA_EMBED_MAX_BYTES if max_media_bytes is None else max_media_bytes
)

The max_media_bytes param controls the MCP-layer check only. The EpisodicStore created inside build_runtime() always uses config.MEDIA_EMBED_MAX_BYTES independently (see episodic_store.py:56-58). These two limits are completely decoupled.

Dry-run production case: Deploy with MEDIA_EMBED_MAX_BYTES=5MB in environment, but instantiate server with max_media_bytes=2MB to enforce a tighter MCP cap. A 3MB file passes the MCP check (3MB > 2MB → WRONG, 3MB > 2MB → rejected). Actually this direction is fine. But now try it the other way: deploy server with no custom max_media_bytes but set MEDIA_EMBED_MAX_BYTES=20MB. A 25MB file — MCP layer uses app_config.MEDIA_EMBED_MAX_BYTES=20MB, raises MediaTooLargeError with code="media_too_large". Episodic store would also reject it but never gets called. OK.

The real issue surfaces in tests: make_server(max_media_bytes=8) sets the MCP limit to 8 bytes. But build_runtime() gives the episodic store a 20MB limit. The test verifies MCP-layer rejection, but there's zero guarantee the episodic store would also reject at 8 bytes. If someone bypasses _prepare_media and calls runtime.episodic_store.store() directly with an oversized record, the 8-byte MCP limit is completely invisible to the store.

This is a design gap: the constructor parameter max_media_bytes is falsely implied to be a global server limit, but it only affects the MCP validation layer. The docstring/parameter name should clarify this, or max_media_bytes should be passed through to the runtime.

Fix this →

---

Bug #2 — HIGH: TOCTOU race between os.access() and path.stat().st_size

Location: server.py:441-445

if not os.access(path, os.R_OK):
    raise ValueError(
        f"Media file is not readable from the MCP server host: {path}"
    )
self._validate_media_size(path=path, size_bytes=path.stat().st_size)  # ← NEW

Four separate syscalls to validate the same path: exists(), is_file(), os.access(), then stat(). Between os.access() and stat(), the file can be:

• Deleted → stat() raises raw OSError: [Errno 2] No such file or directory: '/path' — NOT the "MCP server host" error message. _map_exception maps this as code="invalid_request" with the raw OS message, which is confusing.
• Replaced with a symlink to /dev/random → stat() returns a very large size → MediaTooLargeError (correct-ish, but unexpected path).

Dry-run: File /tmp/media.png exists at os.access() time, deleted 1μs later. path.stat() raises FileNotFoundError("No such file or directory: '/tmp/media.png'"). The client sees {"code": "invalid_request", "message": "No such file or directory: '/tmp/media.png'"} — no mention of "MCP server host". In a stdio transport this is unlikely but in a long-running streamable-http server it's a realistic race.

Minimal fix: Wrap path.stat() in a try/except to catch OSError and raise the cleaner message.

Fix this →

---

Bug #3 — MEDIUM: mimetypes.guess_extension platform inconsistency — _MEDIA_TYPE_SUFFIXES fallback fires too late

Location: server.py:463-467

suffix = (
    mimetypes.guess_extension(inline.mime_type)
    or _MEDIA_TYPE_SUFFIXES.get(requested_media_type or "")
    or ""
)

mimetypes.guess_extension is known to return platform-dependent, non-canonical extensions on Linux:

• "audio/mpeg" → .mpga (not .mp3) on many Debian/Ubuntu systems
• "image/jpeg" → .jpe (not .jpg) on some systems

The _MEDIA_TYPE_SUFFIXES fallback only fires when guess_extension returns None. On systems where it returns .mpga, the fallback is bypassed and the file gets stored as .mpga. This means:

• _EXTENSION_DIRECTORIES does not have .mpga, so _media_directory falls back to mimetypes.guess_type("x.mpga") → may or may not return "audio/mpeg" depending on system state.
• If mimetypes module has been monkey-patched or the system MIME DB is non-standard, _media_directory raises ValueError("Unsupported media file type for x.mpga").

Dry-run on Ubuntu 22.04:

import mimetypes
mimetypes.guess_extension("audio/mpeg")  # → '.mpga'

Inline audio with mime_type="audio/mpeg" → tempfile agentic_memory_mcp_XXX.mpga → MediaStore._media_directory("agentic_memory_mcp_XXX.mpga") → .mpga not in _EXTENSION_DIRECTORIES → mimetypes.guess_type("agentic_memory_mcp_XXX.mpga")[0] → None on most systems → ValueError("Unsupported media file type for agentic_memory_mcp_XXX.mpga") → tempfile is cleaned up, error surfaces to caller.

The _MEDIA_TYPE_SUFFIXES fallback intended to solve this exact problem but the OR chain means it can't fire if guess_extension returns any truthy value, even a non-canonical one.

Fix this →

---

Bug #4 — MEDIUM: Redundant size check creates inconsistent error messages

Location: server.py:445 (new) vs episodic_store.py:203-209 (pre-existing)

The MCP server's _validate_media_size raises:

"Cannot store MCP media input: size_bytes=X limit_bytes=Y path=Z"

The EpisodicStore's _embed_record also raises MediaTooLargeError (different message):

"Cannot store episodic media record via direct embedding: modality=X size_bytes=Y limit_bytes=Z path=W"

In the default configuration both use app_config.MEDIA_EMBED_MAX_BYTES=20MB. A file that passes the MCP check will ALSO pass the EpisodicStore check (same limit). The EpisodicStore check is dead code in the default deployment. But if MEDIA_EMBED_MAX_BYTES changes between when the server starts and when the request fires (e.g., env var changed, config reload), the two checks could have different limits at runtime.

More practically: since max_media_bytes on the MCP server is independent of the EpisodicStore (Bug #1), a file that's between those two limits could either pass or fail at the store layer with a different error code than the caller expects.

---

Bug #5 — LOW: New test assertion has weak recall validation

Location: tests/test_mcp_server.py:136-147

recall = server.recall_memories(
    RecallMemoriesRequest(query=query, top_k=1, memory_types=["episodic"])
)
assert recall.results[0].record.id == response.record.id

This assertion only holds trivially — there is exactly one episodic record in the store, so top_k=1 always returns it regardless of query similarity. The test doesn't validate that the record is actually retrievable by semantic similarity. On an empty store, recall.results would be [] and results[0] would raise IndexError — but that can't happen here since we just stored one record. The test is structurally correct but provides weak coverage of the recall path.

---

Bug #6 — LOW: _MEDIA_TYPE_SUFFIXES fallback is never tested for inline content

The new _MEDIA_TYPE_SUFFIXES constant is only exercised when:

1. inline.filename has no suffix, AND
2. mimetypes.guess_extension(mime_type) returns None/falsy

All parametrized tests in test_server_stores_local_media_paths_and_text_recall_finds_them use file_path, not inline content. The test_server_materializes_inline_media_for_episode_storage test uses filename="diagram.png" which provides an explicit suffix (bypasses the whole fallback chain). There is no test covering the case where requested_media_type="audio", no filename is given, and mimetypes.guess_extension returns None.

---

Positive Observations

• Error message clarity is improved: "Media file is not readable from the MCP server host: {path}" is significantly more useful for remote clients than the prior "Media file not found: {path}".
• _validate_media_size before tempfile write is the right design — prevents disk I/O for clearly-oversized payloads.
• PreparedMedia.cleanup=True pattern correctly prevents double-deletion of caller-provided files while cleaning up server-generated tempfiles.
• _map_exception correctly distinguishes MediaTooLargeError from generic errors with a dedicated "media_too_large" error code, allowing callers to handle this case programmatically.
• Test test_prepare_media_rejects_oversized_inline_payloads_before_tempfile_write correctly verifies no tempfiles leak using before/after glob snapshot — the finally placement is correct.

---

Priority Summary

#	Severity	Location	Issue
1	HIGH	server.py:206-208	max_media_bytes not propagated to EpisodicStore — limits are decoupled
2	HIGH	server.py:441-445	TOCTOU: stat() after os.access() emits raw OS error on file deletion
3	MEDIUM	server.py:463-467	mimetypes.guess_extension returns .mpga on Linux, bypassing _MEDIA_TYPE_SUFFIXES → potential ValueError from MediaStore
4	MEDIUM	server.py:445 + episodic_store.py:204	Redundant size checks with different error messages; EpisodicStore check is dead code in default config
5	LOW	test_mcp_server.py:136-147	Recall assertion passes trivially (single record), doesn't test similarity
6	LOW	test_mcp_server.py:95-151	_MEDIA_TYPE_SUFFIXES fallback is never exercised by any test

[Copilot]

Pull request overview

This PR hardens the MCP server’s multimodal media intake by enforcing media size limits earlier, improving host-side error messages for unreadable local paths, and adding regression tests covering local-path and inline media scenarios.

Changes:

• Add configurable max media size handling in MemoryMCPServer, with explicit size validation for both local paths and inline base64 payloads.
• Improve error messages when local file_path inputs are not readable from the MCP server host.
• Extend MCP regression tests for local screenshot/audio/video/PDF paths, inline materialization, and oversized payload rejection.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
mcp_server/server.py	Adds media suffix defaults, host-visible path errors, and size validation via _validate_media_size() for both file-path and inline media flows.
tests/test_mcp_server.py	Adds coverage for local media-path persistence + recall, and validates oversized inline payload rejection and improved local-path error messaging.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

max_media_bytes is only enforced in the MCP intake layer (_prepare_media), but it is not propagated into the underlying stores created by build_runtime() (which still use config.MEDIA_EMBED_MAX_BYTES). This can lead to inconsistent behavior (e.g., server accepts a payload that the store later rejects, or vice-versa). Consider plumbing max_media_bytes through build_runtime() into SemanticStore/EpisodicStore/ProceduralStore, or removing this parameter from MemoryMCPServer if it’s intended to be intake-only.

[Copilot]

Inline payload size is validated only after base64.b64decode(...) has already materialized the full bytes in memory. A very large inline_base64 can still cause high memory usage/DoS before the size check runs. Consider rejecting earlier by estimating decoded size from the base64 string length (or enforcing transport-level request limits) before decoding.

[Copilot]

This test uses a hard-coded /tmp/agentic_memory_missing_screenshot.png path; if that file exists on a developer/CI machine, the assertion expecting FileNotFoundError will fail. Prefer generating a guaranteed-missing path (e.g., tmp_path / "missing.png" or a TemporaryDirectory + non-existent filename).

Suggested change

	def test_prepare_media_reports_local_path_errors_from_server_host_view():
	server = make_server()
	missing_path = "/tmp/agentic_memory_missing_screenshot.png"
	try:
	with pytest.raises(FileNotFoundError) as exc_info:
	server._prepare_media( # type: ignore[attr-defined]
	RememberEpisodeRequest(
	session_id="session-1",
	text="Missing screenshot",
	media={"file_path": missing_path},
	).media,
	"image",
	)
	assert "MCP server host" in str(exc_info.value)
	assert missing_path in str(exc_info.value)
	def test_prepare_media_reports_local_path_errors_from_server_host_view(tmp_path: Path):
	server = make_server()
	missing_path = tmp_path / "agentic_memory_missing_screenshot.png"
	try:
	with pytest.raises(FileNotFoundError) as exc_info:
	server._prepare_media( # type: ignore[attr-defined]
	RememberEpisodeRequest(
	session_id="session-1",
	text="Missing screenshot",
	media={"file_path": str(missing_path)},
	).media,
	"image",
	)
	assert "MCP server host" in str(exc_info.value)
	assert str(missing_path) in str(exc_info.value)

[Atharva-Kanherkar]

@claude re review. patched important parts

[claude]

Claude finished @Atharva-Kanherkar's task in 2m 12s —— View job

---

Re-review: [codex] Harden MCP multimodal media intake

Tasks:

• Read trigger comment
• Read current mcp_server/server.py, runtime.py, tests/test_mcp_server.py
• Map each prior bug to patch status
• Identify any remaining or new issues
• Post findings

---

Status of Previous Findings

#	Severity	Prior Issue	Status
1	HIGH	max_media_bytes not propagated to EpisodicStore	✅ Fixed
2	HIGH	TOCTOU: stat() after os.access() emits raw OS error	✅ Fixed
3	MEDIUM	mimetypes.guess_extension returns .mpga on Linux, bypasses _MEDIA_TYPE_SUFFIXES	✅ Fixed
4	MEDIUM	Redundant size checks with decoupled limits	✅ Resolved (limits now in sync)
5	LOW	Recall assertion passes trivially (single record)	✅ Fixed
6	LOW	_MEDIA_TYPE_SUFFIXES fallback never tested for inline content	✅ Fixed

All HIGH and MEDIUM bugs from the prior review are addressed. Below are the remaining issues — all LOW severity.

---

Remaining Issue #1 — LOW: Hard-coded /tmp path in missing-file test

Location: tests/test_mcp_server.py:226

def test_prepare_media_reports_local_path_errors_from_server_host_view():
    server = make_server()
    missing_path = "/tmp/agentic_memory_missing_screenshot.png"  # ← still hard-coded

If /tmp/agentic_memory_missing_screenshot.png exists on the CI runner (e.g., left over from a previous test run or a media temp file with that name), _prepare_media will not raise FileNotFoundError and the test will silently fail. Copilot's suggestion to use tmp_path / "missing.png" is the correct fix.

Fix this →

---

Remaining Issue #2 — LOW: Full base64 decode before size check

Location: server.py:461-466

payload = base64.b64decode(inline.inline_base64, validate=True)   # full allocation
if not payload:
    raise ValueError("Cannot store empty media payload")
self._validate_media_size(size_bytes=len(payload))                 # checked too late

A caller can send a 200MB base64 string. The server allocates the full decoded buffer in memory, then rejects it. The Copilot comment flagged this and it remains. A cheap pre-check using the encoded length is sufficient: decoded size is approximately len(inline.inline_base64) * 3 // 4. You can reject obviously oversized payloads before spending memory on decoding:

# Quick estimate: base64 decodes to ~75% of encoded length
if len(inline.inline_base64) * 3 // 4 > self._max_media_bytes:
    raise MediaTooLargeError(
        f"Cannot store MCP media input: estimated size exceeds limit_bytes={self._max_media_bytes}"
    )
payload = base64.b64decode(inline.inline_base64, validate=True)

This doesn't need to be byte-perfect — it's an early exit, not the authoritative check (the post-decode check still catches borderline cases).

Fix this →

---

Positive Observations on the Patch

• Bug Add semantic memory retrieval with query/document embedding split #1 fix is clean and complete: max_media_bytes flows MemoryMCPServer → build_runtime() → EpisodicStore, and test_server_threads_custom_media_limit_into_runtime_store verifies the propagation end-to-end with a direct attribute assertion. Exactly right.
• TOCTOU fix is correct: wrapping path.stat() in try/except OSError and re-raising as FileNotFoundError with the host-visibility message is the minimal correct fix. The dedicated monkeypatch test (test_prepare_media_wraps_stat_race_with_server_host_error) exercises the race path cleanly.
• Suffix priority fix is correct: _MEDIA_TYPE_SUFFIXES.get(requested_media_type) now fires first, so .mp3 is always preferred over .mpga on Linux for audio/mpeg. The inline audio test locks this in.
• Recall test strengthened: adding a distractor record before the target means top_k=1 returning the target's ID is a meaningful assertion, not a trivial one.
• assert "agentic_memory_mcp_" not in response.record.media_ref in test_server_materializes_inline_media_for_episode_storage is a good addition — it confirms the tempfile was copied to a durable owned path, not returned as-is.

---

Summary

The two HIGH bugs and both MEDIUM bugs are resolved. Two LOW items remain: the hard-coded /tmp path in the missing-file test (easy to hit on a reused runner), and the pre-decode size check (memory concern for large base64 payloads). The patch is mergeable as-is if those two don't block you, but both fixes are straightforward.