fix: address Octocode review — move size check, add tests for all 3 fixes
- Move file size check before try block so IOError propagates cleanly (not caught by the except OSError handler below it) - Wrap os.path.getsize in its own try/except to preserve existing test_normalize_io_error behavior on missing files - Add test_normalize_rejects_large_file (mocked getsize) - Add test_null_arguments_does_not_hang (#394) - Add test_cmd_repair_trailing_slash_does_not_recurse (#395) 532 tests pass locally, 0 regressions.
This commit is contained in:
@@ -27,8 +27,11 @@ def normalize(filepath: str) -> str:
|
||||
"""
|
||||
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}")
|
||||
except OSError as e:
|
||||
raise IOError(f"Could not read {filepath}: {e}")
|
||||
if file_size > 500 * 1024 * 1024: # 500 MB safety limit
|
||||
raise IOError(f"File too large ({file_size // (1024*1024)} MB): {filepath}")
|
||||
try:
|
||||
with open(filepath, "r", encoding="utf-8", errors="replace") as f:
|
||||
content = f.read()
|
||||
except OSError as e:
|
||||
|
||||
@@ -607,3 +607,16 @@ def test_cmd_compress_stores_results(mock_config_cls, capsys):
|
||||
out = capsys.readouterr().out
|
||||
assert "Stored" in out
|
||||
mock_comp_col.upsert.assert_called_once()
|
||||
|
||||
|
||||
def test_cmd_repair_trailing_slash_does_not_recurse():
|
||||
"""Repair with trailing slash should put backup outside palace dir (#395)."""
|
||||
import os
|
||||
|
||||
args = argparse.Namespace(palace="/tmp/fake_palace/")
|
||||
with patch("mempalace.cli.os.path.isdir", return_value=False):
|
||||
cmd_repair(args)
|
||||
# Verify the rstrip logic: palace_path should not end with separator
|
||||
palace_path = os.path.expanduser(args.palace).rstrip(os.sep)
|
||||
backup_path = palace_path + ".backup"
|
||||
assert not backup_path.startswith(palace_path + os.sep)
|
||||
|
||||
@@ -103,6 +103,23 @@ class TestHandleRequest:
|
||||
assert "mempalace_add_drawer" in names
|
||||
assert "mempalace_kg_add" in names
|
||||
|
||||
def test_null_arguments_does_not_hang(self, monkeypatch, config, palace_path, seeded_kg):
|
||||
"""Sending arguments: null should return a result, not hang (#394)."""
|
||||
_patch_mcp_server(monkeypatch, config, seeded_kg)
|
||||
from mempalace.mcp_server import handle_request
|
||||
|
||||
_client, _col = _get_collection(palace_path, create=True)
|
||||
del _client
|
||||
resp = handle_request(
|
||||
{
|
||||
"method": "tools/call",
|
||||
"id": 10,
|
||||
"params": {"name": "mempalace_status", "arguments": None},
|
||||
}
|
||||
)
|
||||
assert "error" not in resp
|
||||
assert resp["result"] is not None
|
||||
|
||||
def test_unknown_tool(self):
|
||||
from mempalace.mcp_server import handle_request
|
||||
|
||||
|
||||
@@ -499,3 +499,13 @@ def test_messages_to_transcript_assistant_first():
|
||||
result = _messages_to_transcript(msgs, spellcheck=False)
|
||||
assert "preamble" in result
|
||||
assert "> Q" in result
|
||||
|
||||
|
||||
def test_normalize_rejects_large_file():
|
||||
"""Files over 500 MB should raise IOError before reading."""
|
||||
with patch("mempalace.normalize.os.path.getsize", return_value=600 * 1024 * 1024):
|
||||
try:
|
||||
normalize("/fake/huge_file.txt")
|
||||
assert False, "Should have raised IOError"
|
||||
except IOError as e:
|
||||
assert "too large" in str(e).lower()
|
||||
|
||||
Reference in New Issue
Block a user