diff --git a/gcode/history.py b/gcode/history.py index 2420358..9b5e04a 100644 --- a/gcode/history.py +++ b/gcode/history.py @@ -4,8 +4,11 @@ ``messages_from_dict`` so tool-call <-> tool-message pairing survives a reload. """ +import contextlib import json import os +import sys +import tempfile from langchain_core.messages import messages_from_dict, messages_to_dict @@ -20,7 +23,13 @@ def _path(session: str) -> str: def load(session: str = DEFAULT_SESSION): - """Load persisted messages for a session, or None if none exist.""" + """Load persisted messages for a session, or None if none exist. + + A missing file returns None silently. A file that exists but cannot be + parsed is reported to stderr (naming the path) instead of being silently + treated as "no history", so a corrupt session file cannot masquerade as an + empty conversation. + """ path = _path(session) if not os.path.isfile(path): return None @@ -28,18 +37,41 @@ def load(session: str = DEFAULT_SESSION): with open(path, encoding="utf-8") as f: data = json.load(f) return messages_from_dict(data) - except Exception: + except Exception as exc: # noqa: BLE001 — surface corrupt/malformed history + print( + f"Warning: could not read session history from {path} ({exc}). " + "The file may be corrupt — inspect or remove it to start a fresh session.", + file=sys.stderr, + ) return None def save(session: str, messages) -> None: - """Persist messages to disk (includes the leading system message).""" + """Persist messages to disk atomically (includes the leading system message). + + Writes to a temporary file in the same directory and ``os.replace``s it + into place, so an interrupted or failed save can never leave a partial + session file behind. Failures are reported to stderr instead of being + silently swallowed — a broken save should not look like "no history". + """ path = _path(session) try: - with open(path, "w", encoding="utf-8") as f: - json.dump(messages_to_dict(messages), f, indent=2) - except Exception: # noqa: S110 # nosec B110 — persistence is best-effort; never crash the REPL on disk errors - pass + data = json.dumps(messages_to_dict(messages), indent=2) + fd, tmp_path = tempfile.mkstemp(dir=os.path.dirname(path), suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(data) + os.replace(tmp_path, path) + except Exception: + # Best-effort cleanup so a failed write leaves no stray temp file. + with contextlib.suppress(OSError): + os.unlink(tmp_path) + raise + except Exception as exc: # noqa: BLE001 — persistence must never crash the REPL + print( + f"Warning: could not save session history to {path} ({exc}).", + file=sys.stderr, + ) def clear(session: str = DEFAULT_SESSION) -> None: diff --git a/tests/test_history.py b/tests/test_history.py index e8e34b2..4dda5dd 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -85,6 +85,28 @@ def test_load_corrupt_json(tmp_path, monkeypatch): assert result is None +def test_load_corrupt_json_warns_with_path(tmp_path, monkeypatch, capsys): + """A corrupt session file must warn and name the path, not silently vanish.""" + monkeypatch.setattr(history, "BASE_DIR", str(tmp_path)) + filepath = history._path("corrupt_warn_session") + with open(filepath, "w", encoding="utf-8") as f: + f.write("{ invalid json content ...") + + result = history.load("corrupt_warn_session") + assert result is None + err = capsys.readouterr().err + assert filepath in err + assert "corrupt" in err + + +def test_load_missing_file_no_warning(tmp_path, monkeypatch, capsys): + """A missing session file stays silent (no history yet is not an error).""" + monkeypatch.setattr(history, "BASE_DIR", str(tmp_path)) + result = history.load("never_saved_session") + assert result is None + assert capsys.readouterr().err == "" + + def test_load_invalid_message_dict(tmp_path, monkeypatch): monkeypatch.setattr(history, "BASE_DIR", str(tmp_path)) filepath = history._path("invalid_msg_session") @@ -118,9 +140,18 @@ def test_clear_default_session(tmp_path, monkeypatch): assert not os.path.isfile(history._path(history.DEFAULT_SESSION)) -def test_save_write_failure_resilience(tmp_path, monkeypatch): +def test_save_write_failure_resilience(tmp_path, monkeypatch, capsys): + """A failed save must warn, never raise, and leave no partial session file.""" monkeypatch.setattr(history, "BASE_DIR", str(tmp_path)) messages = [HumanMessage(content="Test")] - with patch("builtins.open", side_effect=OSError("Disk full or permission denied")): + with patch("gcode.history.os.replace", side_effect=OSError("Disk full or permission denied")): # Should not raise exception history.save("fail_session", messages) + + path = history._path("fail_session") + assert not os.path.isfile(path) # no partial file at the real path + leftovers = [name for name in os.listdir(tmp_path) if name.endswith(".tmp")] + assert leftovers == [] # temp file cleaned up + err = capsys.readouterr().err + assert "could not save session history" in err + assert path in err