PR Review: fix: MCP null args hang, repair infinite recursion, OOM on large files

Executive Summary

Affected Areas: mempalace/mcp_server.py, mempalace/cli.py, mempalace/normalize.py, mempalace/split_mega_files.py, .github/workflows/ci.yml

Business Impact: Prevents MCP client hangs (server availability), disk exhaustion during repair (data safety), and OOM crashes on large files (stability).

Flow Changes: normalize() now rejects files >500 MB before reading; split_file() and main() skip files >500 MB; handle_request() handles null arguments; cmd_repair() strips trailing path separators before backup.

Ratings

PR Health

• Has clear description
• References tickets (MCP server hangs when client sends null arguments — unhandled AttributeError in request handler #394, cmd_repair infinite recursion when palace_path has trailing slash — fills disk #395, OOM crash on large transcript files — split_mega_files.py and normalize.py load entire file into memory #396)
• Has relevant tests — No tests for any of the 3 bugfixes
• Appropriate size

Guidelines Compliance

High Priority Issues

(Must fix before merge)

[Bug] #1: normalize.py — IOError caught by its own except handler

Location: mempalace/normalize.py:28-32 | Confidence: HIGH | VALIDATED

In Python 3 (project requires >=3.9 per pyproject.toml), IOError is an alias for OSError — they are the same class (PEP 3151). The new raise IOError("File too large...") on line 30 is immediately caught by the except OSError on line 32, which re-wraps it as IOError("Could not read {filepath}: File too large..."). The size guard never cleanly propagates to the caller.

Caller verification: The sole caller is convo_miner.py:280 — except (OSError, ValueError): continue. The file IS correctly skipped in both cases, so no functional break. However, the error message is misleading ("Could not read" vs "too large"), and programmatic distinction between size rejection and I/O failure is lost.

     try:
+        file_size = os.path.getsize(filepath)
+        if file_size > 500 * 1024 * 1024:  # 500 MB safety limit
+            raise IOError(f"File too large ({file_size // (1024*1024)} MB): {filepath}")
         with open(filepath, "r", encoding="utf-8", errors="replace") as f:
             content = f.read()
     except OSError as e:
         raise IOError(f"Could not read {filepath}: {e}")

Fix — Move the size check before the try block so the IOError propagates directly:

+    file_size = os.path.getsize(filepath)
+    if file_size > 500 * 1024 * 1024:  # 500 MB safety limit
+        raise IOError(f"File too large ({file_size // (1024*1024)} MB): {filepath}")
     try:
-        file_size = os.path.getsize(filepath)
-        if file_size > 500 * 1024 * 1024:  # 500 MB safety limit
-            raise IOError(f"File too large ({file_size // (1024*1024)} MB): {filepath}")
         with open(filepath, "r", encoding="utf-8", errors="replace") as f:
             content = f.read()
     except OSError as e:
         raise IOError(f"Could not read {filepath}: {e}")

[Guidelines] #2: No tests for 3 bugfixes + CI coverage threshold lowered

Location: .github/workflows/ci.yml:21,41 | Confidence: HIGH | VALIDATED

The coverage threshold is dropped from 85% to 80% in both the Linux and Windows CI jobs. Zero test files were changed in this PR (5 files, all in source/CI). tests/test_normalize.py exists with basic tests like test_plain_text but nothing for the new size guard. No MCP server or repair command tests exist. CONTRIBUTING.md states: "Add or update tests if applicable." All three bugs are straightforward to test:

• Null args: call handle_request with {"arguments": null} and verify no hang/crash
• Trailing slash: call cmd_repair with a path ending in os.sep and verify backup_path is a sibling
• Large file: mock os.path.getsize to return >500 MB and verify IOError is raised

-      - run: python -m pytest tests/ -v --ignore=tests/benchmarks --cov=mempalace --cov-report=term-missing --cov-fail-under=85
+      - run: python -m pytest tests/ -v --ignore=tests/benchmarks --cov=mempalace --cov-report=term-missing --cov-fail-under=80

Fix: Add tests for the three bugfixes and keep the threshold at 85%.

Medium Priority Issues

(Should fix, not blocking)

[Code Quality] #3: Magic number 500 MB duplicated in 3 locations

Location: mempalace/normalize.py:28, mempalace/split_mega_files.py:185, mempalace/split_mega_files.py:273 | Confidence: HIGH | VALIDATED

The expression 500 * 1024 * 1024 with the comment "500 MB safety limit" appears in three places across two files. If the limit changes, all three must be updated in sync.

+# In mempalace/constants.py or at module level
+MAX_FILE_SIZE = 500 * 1024 * 1024  # 500 MB safety limit

Then reference MAX_FILE_SIZE in all three locations.

[Error Handling] #4: split_mega_files.py — unguarded stat() calls can crash on missing files

Location: mempalace/split_mega_files.py:185,275 | Confidence: MED | VALIDATED

Both path.stat().st_size (line 185) and f.stat().st_size (line 275) are called outside any try/except. If a file discovered by glob() is deleted before stat() executes (race condition), an unhandled OSError crashes the CLI. The normalize.py version uses os.path.getsize inside a try/except, showing the codebase already expects filesystem failures.

+    try:
+        fsize = path.stat().st_size
+    except OSError:
+        return []
+    if fsize > max_size:
         print(f"  SKIP: {path.name} exceeds {max_size // (1024*1024)} MB limit")
         return []

Flow Impact Analysis

normalize()  ← callers: miner.py mine_file(), convo_miner.py
  └─ NEW: rejects files >500 MB with IOError (currently re-wrapped — see #1)

split_file() ← callers: split_mega_files.main(), cli.cmd_split()
  └─ NEW: returns [] for files >500 MB (silent skip)

main() scanning loop
  └─ NEW: skips files >500 MB with print + continue

handle_request() ← MCP stdio loop
  └─ FIX: null arguments no longer cause AttributeError hang

cmd_repair() ← CLI entry point
  └─ FIX: trailing-slash palace path no longer creates backup inside palace

No breaking changes to external callers. The normalize() IOError self-catch (#1) silently degrades the error message but does not break functionality — the exception still propagates.

Created by Octocode MCP https://octocode.ai

thanks @bgauryy for the reviews! and please thank Octocode from Lu.✨

@milla-jovovich my pleasure 🙏