Refactor/server stdio split - #88
Merged
Merged
Conversation
Task 1 of docs/current/server-stdio-module-split.md. Pure extraction, verbatim move verified byte-identical against git history. Removed now-dead logging/pathlib imports from server_stdio.py (only consumer was this block). Functional check confirms _stdio_log still configures correctly (stderr + file handlers) from its new location. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
…/stdio_tool_lifecycle.py Task 2 of docs/current/server-stdio-module-split.md. Pure extraction (_log_tool_call decorator + protocol/compaction delivery state), verbatim move verified byte-identical against git history. Removed now-dead imports from server_stdio.py (json, functools, _debug, claim_pending_compaction_prompt, ensure_marm_started, maybe_auto_refresh, read_protocol_file, read_protocol_lite_file -- only consumer was this block). Late-binding gotcha (flagged in the spec) confirmed and handled: the decorator's body resolved several names from server_stdio.py's later imports via Python's call-time name resolution. Now isolated in its own module, it imports all of those itself. Verified with a real end-to-end tool call (marm_log_show), not just an import-doesn't-crash check -- confirmed logging, session init, and first-call protocol injection all still fire correctly. Updated tests/test_stdio_transport.py and tests/test_protocol_lite.py: several tests monkeypatch or directly mutate protocol-delivery state and its dependencies via module-qualified names (stdio.ensure_marm_started, stdio._protocol_delivered, etc.) that lived directly on server_stdio before this move. Repointed those patches to core.stdio_tool_lifecycle, the code's new home -- no assertion or test logic changed, only which module owns the patched name. test_stdio_handles_mcp_initialize_and_exposes_tools (slow_stdio, real subprocess spawn) flakes intentionally independent of this change -- verified by running it against the unmodified base commit, where it also fails intermittently. Pre-existing environmental flakiness, not a regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
…_entry_tools.py Task 3 of docs/current/server-stdio-module-split.md. Body moved verbatim (no dedent -- the spec's original plan called for dedenting by 4 spaces, but the body was already at the correct indentation for a function body since it's still one function-body level deep in its new home, not becoming top-level code. Caught this via py_compile IndentationError before it ever reached a commit, reverted the first attempt cleanly, and corrected the spec's own reasoning for this task type). _SESSION_PREFIXES/_SESSION_INACTIVITY_NOTICE_SECONDS moved with it (private to this function, verified no other use). marm_log_entry itself is now a 2-line pass-through, matching marm_smart_recall's existing thin-wrapper shape. Removed now-dead imports from server_stdio.py (re, uuid, events, MARM_PROJECT, MARM_PLATFORM). Real bug caught and fixed during test verification, not just a test-wiring issue: tests/test_stdio_transport.py's isolated-stdio fixture patched the memory object on the stdio module to an isolated test instance, but create_log_entry_stdio (now in services.stdio_entry_tools) imports its own separate memory binding, so the fixture's patch never reached it. This silently misdirected writes to the real production memory singleton instead of the test's isolated database (one test's assertion failed: 0 entries found instead of 2, because the write landed elsewhere) and caused a second test to hang indefinitely (the production singleton's write queue was never started in-process, so the queued semantic-memory store call blocked forever waiting for a consumer that didn't exist). Fixed by also patching the memory object in services.stdio_entry_tools inside the fixture. Verified both failure modes are resolved: the count assertion now passes, and the previously-hanging test completes in under a second. Functional smoke test: real end-to-end marm_log_entry calls exercising both the plain-entry and session-switch code paths, confirmed correct persistence via marm_log_show. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
…entry_tools.py Task 4 of docs/current/server-stdio-module-split.md. Body moved verbatim to services.stdio_entry_tools.list_log_entries_stdio, no transform needed. marm_log_show is now a 1-line pass-through. No now-dead imports in server_stdio.py this time -- memory is still used directly by marm_delete, which hasn't moved yet. Integrity check passed byte-identical against git history. Functional smoke test covers both code paths (list-all-sessions and list-by-session). Targeted test suite passes clean, no test-wiring fixes needed this time since the isolated-stdio fixture's stdio_entry_tools.memory patch (added in Task 3) already covers this function's new home. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
…try_tools.py Task 5 (final task) of docs/current/server-stdio-module-split.md. Body moved verbatim to services.stdio_entry_tools.delete_entry_stdio, no transform needed. marm_delete is now a 1-line pass-through. Removed now-dead datetime/timezone imports from server_stdio.py (last consumer was this block). Integrity check passed byte-identical against git history. Functional smoke test exercised all four branches given the real data-deletion blast radius here (targeted log-entry delete by session+topic, whole-session delete cascading across sessions/log_entries/memories/ session_summary_cache, notebook entry delete, and the invalid-type error path) -- confirmed correct row counts and status on each. Targeted test suite passes clean. All 5 tasks of the server_stdio.py module split are now complete on this branch (logging setup, tool-call lifecycle decorator, and the three tools with real inline logic all extracted; the 11 already-thin tool wrappers were left in place per the spec's Option 1 scope). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
Contributor
|
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 (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (2)**/tests/**⚙️ CodeRabbit configuration file
Files:
**/*.py⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughSTDIO logging and tool-call lifecycle behavior are extracted into core modules. Entry CRUD operations move into shared services, while the server delegates to them. Tests update lifecycle state, memory isolation, routing assertions, and transport health checks. ChangesSTDIO server refactor
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/core/stdio_tool_lifecycle.py`:
- Around line 74-89: The one-time protocol delivery check in the STDIO lifecycle
can race across concurrent calls. Update the flow around _protocol_delivered and
read_protocol_file() to use a lock or equivalent per-session synchronization,
ensuring only one call reads and injects marm_protocol while preserving the
existing retry behavior when delivery fails.
- Around line 37-46: The stdio lifecycle flow should claim compaction prompts
against the session that actually received the log entry. In
create_log_entry_stdio(), after fn returns, re-resolve the session name from
memory.active_log_session and pass that resolved value to
claim_pending_compaction_prompt instead of relying on the originally supplied or
default session_name.
In `@marm-mcp-server/marm_mcp_server/services/stdio_entry_tools.py`:
- Around line 115-117: Replace the diagnostic print in the chunk-boundary
handling flow with the existing _stdio_log helper, preserving the session and
elapsed-gap details while ensuring the message is sent to stderr rather than the
STDIO JSON-RPC stdout stream.
In `@marm-mcp-server/tests/test_stdio_transport.py`:
- Around line 15-44: Reset the module-global _protocol_call_count within the
_isolated_stdio fixture setup so each test starts from a clean counter. Ensure
_log_tool_call cannot inherit a multiple-of-_STDIO_LITE_INTERVAL value and
unexpectedly inject marm_protocol_lite, while preserving the fixture’s existing
isolated state setup.
🪄 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: e2ffc17f-017f-4caf-b570-727babe2e8ea
📒 Files selected for processing (6)
marm-mcp-server/marm_mcp_server/core/stdio_logging.pymarm-mcp-server/marm_mcp_server/core/stdio_tool_lifecycle.pymarm-mcp-server/marm_mcp_server/server_stdio.pymarm-mcp-server/marm_mcp_server/services/stdio_entry_tools.pymarm-mcp-server/tests/test_protocol_lite.pymarm-mcp-server/tests/test_stdio_transport.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
⚠️ CI failures not shown inline (2)
GitHub Actions: Ruff CI / 0_ruff.txt: Refactor/server stdio split
Conclusion: failure
##[group]Run ruff format --check
�[36;1mruff format --check�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.11.15/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib
##[endgroup]
Would reformat: tests/test_docker_transports.py
1 file would be reformatted, 123 files already formatted
##[error]Process completed with exit code 1.
GitHub Actions: Ruff CI / ruff: Refactor/server stdio split
Conclusion: failure
##[group]Run ruff format --check
�[36;1mruff format --check�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.11.15/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib
##[endgroup]
Would reformat: tests/test_docker_transports.py
1 file would be reformatted, 123 files already formatted
##[error]Process completed with exit code 1.
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/marm_mcp_server/core/stdio_logging.pymarm-mcp-server/marm_mcp_server/core/stdio_tool_lifecycle.pymarm-mcp-server/tests/test_protocol_lite.pymarm-mcp-server/marm_mcp_server/services/stdio_entry_tools.pymarm-mcp-server/tests/test_stdio_transport.pymarm-mcp-server/marm_mcp_server/server_stdio.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_protocol_lite.pymarm-mcp-server/tests/test_stdio_transport.py
🪛 ast-grep (0.44.1)
marm-mcp-server/marm_mcp_server/core/stdio_tool_lifecycle.py
[info] 96-96: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
marm-mcp-server/marm_mcp_server/services/stdio_entry_tools.py
[warning] 121-121: 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/core/stdio_logging.py (1)
1-35: LGTM!marm-mcp-server/marm_mcp_server/core/stdio_tool_lifecycle.py (1)
96-107: 🗄️ Data Integrity & IntegrationVerify merged result shape stays MCP-conformant.
result = {**result, "content": [...]}mixes the tool's own top-level keys (status,entry_id, etc.) with an MCP-stylecontentblock list. Worth confirming the STDIO server/client doesn't expect tool results to be either a plain dict or a{content: [...]}shape exclusively — mixing both on the compaction path only (but not otherwise) could produce an inconsistent result schema depending on whether compaction fired.marm-mcp-server/marm_mcp_server/services/stdio_entry_tools.py (1)
81-165: 🗄️ Data Integrity & IntegrationConfirm multi-step session create + entry insert is safe if interrupted.
Session resolution/creation (lines 88-100), the chunk-boundary read (104-107), and the entry insert (133-165) each open/commit
memory.get_connection()independently rather than as one transaction, andmemory.active_log_sessionis mutated as plain shared state (line 101) in between. If two calls race (e.g., concurrent session switches) or the process is interrupted between commits, a session row can exist with no matching entry, oractive_log_sessioncan point at a different session than the one just written. Worth confirmingget_connection()'s isolation/rollback guarantees and whether concurrent STDIO tool invocations are actually possible in this transport.marm-mcp-server/marm_mcp_server/server_stdio.py (2)
30-42: LGTM!Also applies to: 134-135, 137-157, 162-178, 183-195
55-58: 🎯 Functional Correctness
server_stdio.pyno longer usesMARM_PROJECTorMARM_PLATFORM.> Likely an incorrect or invalid review comment.marm-mcp-server/tests/test_protocol_lite.py (1)
232-254: LGTM!Also applies to: 278-301
marm-mcp-server/tests/test_stdio_transport.py (1)
345-348: LGTM!Also applies to: 926-941, 956-971, 987-1005
… delivery race, drop stray print Four CodeRabbit findings on PR #88, all in core/stdio_tool_lifecycle.py and services/stdio_entry_tools.py: 1. Claim compaction against the resolved session (Major). _log_tool_call snapshotted session_name = kwargs.get('session_name', 'default') BEFORE calling fn. create_log_entry_stdio can resolve or create a completely different session on memory.active_log_session when the caller omits session_name -- the stale pre-call snapshot then claimed compaction against the literal string 'default', which almost never has any log entries, silently missing the session that actually received the write. Fixed by re-resolving from memory.active_log_session after fn returns, only when the caller didn't explicitly pass session_name (explicit caller intent is always respected as-is). Added a real regression test (spy on claim_pending_compaction_prompt), mutation-tested against the original bug. Fixing this surfaced a related test-isolation gap: tests/test_stdio_transport.py's _isolated_stdio fixture patches memory on server_stdio, notebook service, and stdio_entry_tools, but not on stdio_tool_lifecycle itself -- so this new re-resolution read was hitting the real production singleton in tests, not the isolated instance. Fixed the fixture too. 2. Guard _protocol_delivered with a lock (Major). The one-time protocol delivery check could interleave with the read_protocol_file() await across concurrent STDIO tool calls (MCP's V2 SDK dispatcher supports concurrent tool-call handling), so two concurrent calls could both inject the full protocol before the flag flips. Wrapped the check-read-mark block in an asyncio.Lock, mirroring the pattern already used on the HTTP side (middleware/protocol_injection.py). 3. Stray print() in stdio_entry_tools.py's chunk-boundary log (flagged Critical -- claimed it would corrupt the STDIO JSON-RPC stream). Verified this empirically before touching anything: server_stdio.py's global builtins.print monkey-patch (lines 1-35, redirects all print() calls process-wide to stderr) already covers this call regardless of which module it's in, confirmed via a real subprocess test capturing actual stdout/stderr. Not a live bug. Still swapped it for the module's own _stdio_log (already imported, already used for real errors here) since it removes a fragile implicit dependency on import order for no behavior change and no risk. 4. Reset _protocol_call_count in _isolated_stdio (Minor). Module-global counter leaked across tests; landing on a multiple of _STDIO_LITE_INTERVAL could inject marm_protocol_lite unexpectedly and break exact-dict assertions in unrelated tests. Added the missing reset alongside the existing _protocol_delivered reset. Also fixes the same pre-existing tests/test_docker_transports.py ruff format drift already fixed on refactor/server-py-split (unrelated file, zero changes on this branch's own history, independently failing this PR's ruff CI check). Full suite: 599 passed / 20 skipped / 0 failed, ruff clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
PR #88 review finding: no test in test_stdio_transport.py called marm_log_show through the actual in-process MCP client/server session, only marm_log_entry and marm_delete were exercised that way. Extended test_stdio_inprocess_client_wraps_notebook_delete_and_log_results to capture the resolved session name from the session-switch call, then call marm_log_show against it and assert status, session_name, entry count, and content. Entry count is 2, not 1: the "Session: envelope-session" switch call itself writes a session_start marker entry into the new session before the real log entry is added afterward. Asserted membership on the entries list rather than index 0 so the test isn't coupled to entry_date ordering between the marker and the real entry.
shaneholloman
pushed a commit
to shaneholloman/marm-systems
that referenced
this pull request
Jul 20, 2026
…o_graph_tools.py Option 2 of docs/current/server-stdio-module-split.md, deferred until Option 1 shipped and ran stable (merged via PR Lyellr88#88). Moves the 5 graph tools (marm_graph_index, marm_code_lookup, marm_graph_trace, marm_graph_architecture, marm_graph_impact) and the 2 concept tools (marm_concept_build, marm_concept_recall), plus their private _graph_available/_graph_unavailable helpers, out of server_stdio.py into a new services/stdio_graph_tools.py module. They register onto a FastMCP instance now shared via a new core/stdio_mcp_app.py module, avoiding a circular import between server_stdio.py and the new tool module. server_stdio.py drops from 632 to 339 lines, under the project's 400-line check-file-length.py threshold. The 7 moved functions are re-imported into server_stdio.py so existing `stdio.marm_graph_index` style access still resolves. Extraction was script-based index-slicing from git history, not retyped, and integrity-checked byte-for-byte against an independent re-slice of the original file (both the moved body and the untouched tail). Updated tests/test_stdio_transport.py: 5 monkeypatch.setattr(stdio, ...) calls patched names (_run_recall, _marm_concept_build_endpoint, graph_router) that now live in stdio_graph_tools's own module globals, not server_stdio's -- redirected each to target the new module. Confirmed the old attribute paths raise AttributeError on server_stdio now, proving the redirects are load-bearing rather than cosmetic. Full suite: 626 passed, 29 skipped. Live smoke test confirmed all 14 tools still register on the shared mcp instance and both a moved tool and an unmoved tool execute correctly end to end.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests