From a56aa9ac47b0fd52e50a40b1812728ee16bee873 Mon Sep 17 00:00:00 2001 From: rrevenanttt <290873280+rrevenanttt@users.noreply.github.com> Date: Mon, 8 Jun 2026 05:06:48 +0300 Subject: [PATCH] fix(tui_gateway): reject negative truncate_before_user_ordinal to prevent silent history loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `prompt.submit` handler in the TUI gateway lets a client trim the conversation back to a chosen user turn via `truncate_before_user_ordinal`. It validated only the upper bound (`ordinal >= len(user_indices)`) and never the lower one. A negative ordinal therefore sailed straight past the guard and fell into Python's negative indexing: `user_indices[-1]` resolves to the *last* user turn, so the history was silently sliced to everything before it and that truncated list was immediately committed to disk with `db.replace_messages`, which deletes and reinserts the whole row in one transaction. The impact is severe and unrecoverable: a single out-of-range value — from a client bug, a hidden/real user-message desync, or any present or future frontend that emits a relative ordinal — permanently destroys the user's conversation on disk instead of returning the intended `4018` error. Because the gateway is deliberately frontend-agnostic, it cannot assume the value is well-formed; it must validate it. The fix is minimal and safe: extend the existing guard to reject negatives on the very same error path the upper bound already uses. No in-memory history is mutated and no DB write happens for an invalid ordinal, so a bad value now fails closed with no data loss. The valid-ordinal path is untouched. N/A - [x] 🐛 Bug fix (non-breaking change that fixes an issue) - `tui_gateway/server.py`: in the `prompt.submit` handler, change the ordinal guard from `if ordinal >= len(user_indices)` to `if ordinal < 0 or ordinal >= len(user_indices)` so a negative ordinal is rejected with error `4018` before any history slice or `replace_messages` write occurs. Added a comment explaining the negative-indexing hazard. - `tests/test_tui_gateway_server.py`: add `test_prompt_submit_rejects_negative_truncate_ordinal`, which submits a `truncate_before_user_ordinal` of `-1` and asserts the handler returns `4018`, leaves the in-memory history intact, never marks the session running, and never calls `replace_messages`. Added the `pytest` import used by the new test's fail-fast guards. 1. Check out this branch and run `scripts/run_tests.sh tests/test_tui_gateway_server.py -- -k negative_truncate` — the new test passes. 2. Reproduce the bug: temporarily revert the guard to the old `if ordinal >= len(user_indices)` and rerun — the test fails because the handler truncates the history and starts a turn instead of returning `4018`. 3. Full file run: `scripts/run_tests.sh tests/test_tui_gateway_server.py` (the only failure is the pre-existing, environment-dependent `test_browser_manage_connect_default_local_reports_launch_hint`, which also fails on clean `main` when a Chromium browser is installed locally). - [x] I've read the [Contributing Guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md) - [x] My commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) (`fix(scope):`, `feat(scope):`, etc.) - [x] I searched for [existing PRs](https://github.com/NousResearch/hermes-agent/pulls) to make sure this isn't a duplicate - [x] My PR contains **only** changes related to this fix/feature (no unrelated commits) - [x] I've run `pytest tests/ -q` and all tests pass - [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features) - [x] I've tested on my platform: macOS 15 (Darwin 25.5.0) - [x] I've updated relevant documentation (README, `docs/`, docstrings) — or N/A - [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A - [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A - [x] I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A - [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A --- tests/test_tui_gateway_server.py | 56 ++++++++++++++++++++++++++++++++ tui_gateway/server.py | 7 +++- 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 78e5639b449..6d39a252cfe 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -9,6 +9,8 @@ from datetime import datetime from pathlib import Path from unittest.mock import patch +import pytest + from hermes_constants import reset_hermes_home_override, set_hermes_home_override from hermes_cli.active_sessions import active_session_registry_snapshot from tui_gateway import server @@ -2002,6 +2004,60 @@ def test_notification_event_routing_by_session_key(monkeypatch): assert server._notification_event_belongs_elsewhere(mine, {"session_key": "ghost"}) is False +def test_prompt_submit_rejects_negative_truncate_ordinal(monkeypatch): + """A negative truncate_before_user_ordinal must be rejected, not honoured. + + The handler validates the upper bound (`ordinal >= len(user_indices)`) but a + negative ordinal would otherwise slip through and hit Python negative + indexing: `user_indices[-1]` selects the LAST user turn, truncating history + to everything before it and persisting that loss via replace_messages — an + unrecoverable overwrite of the session DB. Reject it on the safe 4018 path + and leave the in-memory history and the DB untouched. + """ + replaced = [] + + class _FakeDB: + def replace_messages(self, key, messages): + replaced.append((key, list(messages))) + + history = [ + {"role": "user", "content": "first"}, + {"role": "assistant", "content": "ok"}, + {"role": "user", "content": "second"}, + {"role": "assistant", "content": "done"}, + ] + server._sessions["trunc-sid"] = _session(history=list(history)) + monkeypatch.setattr(server, "_get_db", lambda: _FakeDB()) + # If the guard ever lets a negative ordinal through, these would run and the + # session would be marked busy; failing here makes that regression loud. + monkeypatch.setattr( + server, "_start_agent_build", lambda *a, **k: pytest.fail("must not start a turn") + ) + monkeypatch.setattr( + server, "_start_inflight_turn", lambda *a, **k: pytest.fail("must not start a turn") + ) + + try: + resp = server.handle_request( + { + "id": "1", + "method": "prompt.submit", + "params": { + "session_id": "trunc-sid", + "text": "next", + "truncate_before_user_ordinal": -1, + }, + } + ) + assert resp["error"]["code"] == 4018 + # History and the DB are left exactly as they were — no silent loss. + assert server._sessions["trunc-sid"]["history"] == history + assert server._sessions["trunc-sid"]["running"] is False + assert replaced == [] + finally: + server._sessions.pop("trunc-sid", None) + + def test_session_create_does_not_persist_empty_row(monkeypatch): """session.create must NOT eagerly write a DB row. diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 9dd54c9b6e3..c78d2895514 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -8140,7 +8140,12 @@ def _(rid, params: dict) -> dict: return _err(rid, 4004, "truncate_before_user_ordinal must be an integer") history = session.get("history", []) user_indices = [i for i, m in enumerate(history) if m.get("role") == "user"] - if ordinal >= len(user_indices): + # Reject out-of-range ordinals on BOTH ends. A negative value would + # otherwise sail past the upper-bound check and hit Python's negative + # indexing below (user_indices[-1] -> the LAST user turn), silently + # truncating history to everything before it and persisting that loss + # via replace_messages — an unrecoverable overwrite of the session DB. + if ordinal < 0 or ordinal >= len(user_indices): return _err(rid, 4018, "target user message is no longer in session history") truncated = history[: user_indices[ordinal]] session["history"] = truncated