Dedup log-entry logic between HTTP and STDIO transports - #94
Conversation
endpoints/logging.py and services/stdio_entry_tools.py each carried their own near-identical copy of marm_log_entry/marm_log_show/marm_delete's session-switch detection, SQL, and dual-write-to-semantic-memory logic. Extracted into a shared services/log_entry.py; both transports now call the same functions through thin, transport-specific wrappers with injectable log_info/log_warning callbacks so each keeps its existing logging destination (HTTP: print, STDIO: _stdio_log to stderr + file). Two behavioral divergences found during the merge, fixed rather than carried forward: - STDIO's error responses leaked raw exception text to the client (same CWE-209 class as the earlier marm_concept_recall fix); now returns the same fixed generic messages HTTP already used. - HTTP's whole-session marm_delete deleted from session_summary_cache without a try/except guard, unlike every other cache-invalidation touch in both files; unified on the guarded behavior for both. Independent Opus review found no correctness bugs in the extraction; closed one flagged test-coverage gap (invalid marm_delete type on both transports). Full suite: 623 passed, 20 skipped, zero regressions. Both fixes mutation-tested (reverted, confirmed exact expected failure, restored). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughHTTP and STDIO log/notebook operations now share a service module for creation, listing, and deletion. Transport wrappers retain local validation and logging, while tests cover generic errors, invalid types, isolated state, commit failures, and missing cache-table handling. ChangesCross-Transport Log Entry Deduplication
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@marm-mcp-server/marm_mcp_server/services/log_entry.py`:
- Line 86: Update the session/log creation flow around
events.emit("session_created", ...) and the additionally affected event
publication sites so event-delivery failures cannot propagate as handler
failures after durable writes commit. Catch and handle publication errors as
best-effort while preserving the successful response for the already-created
session and log data; do not alter the write semantics or create duplicate
durable records on client retry.
- Around line 48-84: Refactor the session-prefix and ordinary marm_log_entry
mutation paths to submit all SQLite writes through the serialized asynchronous
write queue instead of calling memory.get_connection directly. Ensure each
marm_log_entry operation queues both the durable log_entries insert and semantic
memories write together, including session-start entries, while preserving the
behavior that semantic-store failures do not fail the durable log write. Route
session updates, cache invalidation, and commits through the same queued
operation.
- Around line 338-340: Move the active_log_session assignment in the deletion
flow so it occurs only after conn.commit() completes successfully. Keep the
existing target check and "main" fallback, ensuring a commit failure leaves the
runtime session unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 150a75b9-0690-41a5-a2ac-146e6eca43e8
📒 Files selected for processing (7)
CHANGELOG.mdmarm-mcp-server/marm_mcp_server/endpoints/logging.pymarm-mcp-server/marm_mcp_server/services/log_entry.pymarm-mcp-server/marm_mcp_server/services/stdio_entry_tools.pymarm-mcp-server/tests/test_http_tools.pymarm-mcp-server/tests/test_stdio_transport.pymarm-mcp-server/tests/test_summary_cache.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Keep changes surgical: modify only what the task requires, match existing style, and preserve behavior during refactors.
Files:
marm-mcp-server/tests/test_http_tools.pymarm-mcp-server/tests/test_summary_cache.pymarm-mcp-server/tests/test_stdio_transport.pymarm-mcp-server/marm_mcp_server/services/stdio_entry_tools.pyCHANGELOG.mdmarm-mcp-server/marm_mcp_server/endpoints/logging.pymarm-mcp-server/marm_mcp_server/services/log_entry.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Prefer the smallest change that solves the problem; do not add speculative abstractions or unrequested configuration flags.
Keep comments minimal, explain only non-obvious reasons, and never narrate what the next line does.
Files:
marm-mcp-server/tests/test_http_tools.pymarm-mcp-server/tests/test_summary_cache.pymarm-mcp-server/tests/test_stdio_transport.pymarm-mcp-server/marm_mcp_server/services/stdio_entry_tools.pymarm-mcp-server/marm_mcp_server/endpoints/logging.pymarm-mcp-server/marm_mcp_server/services/log_entry.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
marm-mcp-server/tests/test_http_tools.pymarm-mcp-server/tests/test_summary_cache.pymarm-mcp-server/tests/test_stdio_transport.pymarm-mcp-server/marm_mcp_server/services/stdio_entry_tools.pymarm-mcp-server/marm_mcp_server/endpoints/logging.pymarm-mcp-server/marm_mcp_server/services/log_entry.py
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Run tests withpytestfrommarm-mcp-server/; tests should hit real FastAPI endpoints and real SQLite, mocking only when it meaningfully speeds tests and matches real behavior with at least 95% fidelity.
Every new MARM Console API route needs at least one happy-path FastAPI response-contract test with the MCP adapter stubbed.
Do not write existence-check or coded-to-pass tests; prefer deep tests exercising real paths over broad shallow coverage.
Usepytest.mark.skiponly for genuinely unavailable dependencies, such as the embedding model or spaCy[concepts]extra, never for lack of effort.
Files:
marm-mcp-server/tests/test_http_tools.pymarm-mcp-server/tests/test_summary_cache.pymarm-mcp-server/tests/test_stdio_transport.py
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-mcp-server/tests/test_http_tools.pymarm-mcp-server/tests/test_summary_cache.pymarm-mcp-server/tests/test_stdio_transport.py
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: All memory writes must use the serialized asynchronous write queue and must not bypass it.
marm_log_entrymust dual-write alog_entriesrow and a semanticmemoriesentry through the queue; semantic-store failure must never fail the log write.
Keep the memory database and concept-graph database isolated; never share connections between them.
Use one lazy-loadedall-MiniLM-L6-v2fastembed encoder serialized behind a lock; writes must succeed when the encoder is unavailable.
Keep orchestration in its current owner file; extract modules only at real boundaries consistent with the existing endpoint split.
Files:
marm-mcp-server/marm_mcp_server/services/stdio_entry_tools.pymarm-mcp-server/marm_mcp_server/endpoints/logging.pymarm-mcp-server/marm_mcp_server/services/log_entry.py
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Only flag documentation issues that are materially wrong, misleading for installation/release behavior, or inconsistent with live MCP behavior. Skip style, phrasing, formatting, and wording preferences.
Files:
CHANGELOG.md
marm-mcp-server/marm_mcp_server/endpoints/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/endpoints/**/*.py: Endpoint logic belongs inendpoints/, split by surface; shared helpers belong incore/.
When adding a tool, implement it in the appropriateendpoints/surface before registering its transports and documentation.
Files:
marm-mcp-server/marm_mcp_server/endpoints/logging.py
🪛 ast-grep (0.44.1)
marm-mcp-server/marm_mcp_server/services/log_entry.py
[warning] 134-134: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.match(entry_pattern, formatted_entry)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🔇 Additional comments (7)
marm-mcp-server/marm_mcp_server/services/log_entry.py (1)
1-32: LGTM!Also applies to: 116-143, 199-283, 341-367
marm-mcp-server/marm_mcp_server/endpoints/logging.py (1)
7-11: LGTM!Also applies to: 26-26, 40-40, 52-59
marm-mcp-server/marm_mcp_server/services/stdio_entry_tools.py (1)
1-35: LGTM!marm-mcp-server/tests/test_stdio_transport.py (1)
17-30: LGTM!Also applies to: 278-293, 397-428
marm-mcp-server/tests/test_http_tools.py (1)
700-715: LGTM!marm-mcp-server/tests/test_summary_cache.py (1)
166-193: LGTM!CHANGELOG.md (1)
5-15: LGTM!
- active_log_session was flipped to "main" before conn.commit() in marm_delete's whole-session branch. If commit ever failed, the delete would roll back but the runtime pointer would already say "main" while the target session's rows were still in the DB. Moved the flip to after commit succeeds. Pre-existing in both original files, carried forward verbatim by the dedup refactor. - Wrapped both events.emit() calls in try/except so an event-publish failure can't turn an already-committed write into a client-facing error (a retry on a false error response would create a duplicate entry). MARMEvents.emit() already isolates callback failures internally, so this is defense-in-depth for consistency with the file's existing semantic-store try/except, not a fix for a live bug. Deferred the 3rd finding (route all SQLite writes through the serialized write queue): checked core/write_queue.py -- it exists to serialize semantic-memory/embedding writes specifically, not as a general SQLite-write wrapper. The direct-connection pattern this finding flags is used identically across endpoints/compaction.py, concepts.py, memory.py, session.py, system.py, and services/documentation.py, notebook.py, recall.py, summary.py -- pre-existing codebase-wide, not introduced by this PR, and CodeRabbit itself tags it a heavy lift. Out of scope for a log-entry dedup PR. Both fixes mutation-tested (reverted, confirmed the exact expected failure, restored). Full suite: 624 passed, 20 skipped, zero regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
2 participants
Summary by CodeRabbit