mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(tui_gateway): reject negative truncate_before_user_ordinal to prevent silent history loss
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
This commit is contained in:
parent
01bf61c865
commit
a56aa9ac47
2 changed files with 62 additions and 1 deletions
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue