From 040420bd1143b9d8cba8f4d52b484d84fe86fdff Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:15:42 +0530 Subject: [PATCH] refactor(gateway): dedupe truncation-target validation; drop dead state and redundant test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review cleanup on the #83202 salvage (findings from the 4-angle + 3-reviewer passes, all verified against the diff): - Extract _coerce_truncate_ordinal() and _reconcile_client_ordinal(): the bool-check/int-coercion block was duplicated verbatim 3x and the 4030 ordinal-mismatch block 2x across the row-id/message-id/ordinal branches (~90 lines of copy-paste with drift risk between the two durable branches). - Delete target_idx (4 assignments, 0 reads — the cut uses user_indices[ordinal]) and replace the stale inline user-indices comprehension with the _history_user_indices helper it duplicated. - Drop test_reproduce_row_id_truncation: a strict subset of test_prompt_submit_truncates_by_row_id + test_prompt_submit_refuses_ordinal_and_row_id_mismatch with weaker asserts. - Collapse PR-introduced blank-line runs in the test file. Behavior-preserving: error codes, messages, and log fields unchanged (4004/4018/4029/4030 wording identical); full test_tui_gateway_server.py suite green (549 passed). --- tests/test_tui_gateway_server.py | 56 ----------- tui_gateway/methods_prompt.py | 159 ++++++++++++++----------------- 2 files changed, 69 insertions(+), 146 deletions(-) diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index ed990a06a06be..561d0b67d278f 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -4622,9 +4622,6 @@ def test_prompt_submit_refuses_ordinal_and_message_id_mismatch(monkeypatch): server._sessions.pop("mismatch-trunc-sid", None) - - - def test_prompt_submit_truncates_by_row_id(monkeypatch): """#82959: prompt.submit with truncate_before_row_id must cut at the target row id.""" replaced = [] @@ -4708,57 +4705,6 @@ def test_prompt_submit_truncates_by_string_row_id(monkeypatch): server._sessions.pop("str-row-id-trunc-sid", None) -def test_reproduce_row_id_truncation(monkeypatch): - """#82959: Reproduction test for durable row_id truncation and 4030 ordinal mismatch validation.""" - replaced = [] - - class _FakeDB: - def replace_messages(self, key, messages, active_only=False, archive_dropped=False): - replaced.append((key, list(messages))) - - history = [ - {"_row_id": 101, "role": "user", "content": "first"}, - {"_row_id": 102, "role": "assistant", "content": "reply 1"}, - {"_row_id": 103, "role": "user", "content": "second"}, - {"_row_id": 104, "role": "assistant", "content": "reply 2"}, - ] - server._sessions["repro-sid"] = _session(history=list(history)) - monkeypatch.setattr(server, "_get_db", lambda: _FakeDB()) - monkeypatch.setattr(server, "_start_agent_build", lambda *a, **k: None) - - try: - # 1. Target turn 2 via row_id 103 - resp = server.handle_request({ - "id": "1", - "method": "prompt.submit", - "params": { - "session_id": "repro-sid", - "text": "edited second turn", - "truncate_before_row_id": 103, - "confirm_truncate": True, - } - }) - assert resp.get("error") is None - - # 2. Refuse ordinal & row_id mismatch with code 4030 - server._sessions["mismatch-sid"] = _session(history=list(history)) - resp_mismatch = server.handle_request({ - "id": "2", - "method": "prompt.submit", - "params": { - "session_id": "mismatch-sid", - "text": "stale rewind", - "truncate_before_row_id": 103, - "truncate_before_user_ordinal": 0, - "confirm_truncate": True, - } - }) - assert resp_mismatch["error"]["code"] == 4030 - finally: - server._sessions.pop("repro-sid", None) - server._sessions.pop("mismatch-sid", None) - - def test_prompt_submit_refuses_ordinal_and_row_id_mismatch(monkeypatch): """#82959: A mismatch between truncate_before_user_ordinal and truncate_before_row_id must return 4030.""" history = [ @@ -4870,8 +4816,6 @@ def test_prompt_submit_row_id_ignores_platform_id_fallback(monkeypatch): server._sessions.pop("string-id-sid", None) - - def test_prompt_submit_refuses_empty_truncation_without_confirm(monkeypatch): """A confirmed rewind still must not wipe a non-empty transcript by accident. diff --git a/tui_gateway/methods_prompt.py b/tui_gateway/methods_prompt.py index f35e58bfd0852..3547c5b150bd6 100644 --- a/tui_gateway/methods_prompt.py +++ b/tui_gateway/methods_prompt.py @@ -155,6 +155,55 @@ def _resolve_truncate_row_id(session: dict, history: list, target_row_id: int): return db_ord, mem_idx +def _coerce_truncate_ordinal(rid, value): + """Return ``(ordinal, error_response)`` for a client-supplied ordinal. + + bool is an int subclass: a JSON ``true`` would coerce via int() to + ordinal 1 and aim a confirmed rewind at the second user turn — refuse it + like any other non-integer. + """ + if isinstance(value, bool): + return None, _err(rid, 4004, "truncate_before_user_ordinal must be an integer") + try: + return int(value), None + except (TypeError, ValueError): + return None, _err(rid, 4004, "truncate_before_user_ordinal must be an integer") + + +def _reconcile_client_ordinal(rid, sid, client_ordinal, msg_ordinal, param_name, target_repr): + """Cross-check a client ordinal against a resolved durable target. + + Returns ``(ordinal, error_response)``: the target's ordinal when the + client sent none or agreed, else the 4004/4030 refusal. A stale ordinal + alongside a *resolved* durable id is the #82756 drift class — refuse + rather than guess which address the user meant. + """ + if client_ordinal is None: + return msg_ordinal, None + ordinal, err = _coerce_truncate_ordinal(rid, client_ordinal) + if err is not None: + return None, err + if ordinal != msg_ordinal: + logger.warning( + "prompt.submit: REFUSED truncation due to ordinal mismatch for session %s " + "(ordinal=%d, %s_ordinal=%d, %s=%s). " + "Stale truncate_before_user_ordinal detected.", + sid, + ordinal, + param_name, + msg_ordinal, + param_name, + target_repr, + ) + return None, _err( + rid, + 4030, + f"truncate_before_user_ordinal ({ordinal}) does not match " + f"{param_name} target turn ({msg_ordinal})", + ) + return ordinal, None + + def _pending_reaction_notes(session: dict) -> str: """Note block describing reactions the user added since the last turn, or "". @@ -329,7 +378,6 @@ def _(rid, params: dict) -> dict: history = session.get("history", []) user_indices = _history_user_indices(history) - target_idx = None ordinal = None if truncate_row_id is not None: @@ -371,41 +419,13 @@ def _(rid, params: dict) -> dict: "target user message is no longer in session history", ) - msg_ordinal, target_idx = found_match - - if truncate_user_ordinal is not None: - if isinstance(truncate_user_ordinal, bool): - return _err( - rid, - 4004, - "truncate_before_user_ordinal must be an integer", - ) - try: - ordinal = int(truncate_user_ordinal) - except (TypeError, ValueError): - return _err( - rid, - 4004, - "truncate_before_user_ordinal must be an integer", - ) - if ordinal != msg_ordinal: - logger.warning( - "prompt.submit: REFUSED truncation due to ordinal mismatch for session %s " - "(ordinal=%d, row_id_ordinal=%d, row_id=%d). " - "Stale truncate_before_user_ordinal detected.", - sid, - ordinal, - msg_ordinal, - target_row_id, - ) - return _err( - rid, - 4030, - f"truncate_before_user_ordinal ({ordinal}) does not match " - f"truncate_before_row_id target turn ({msg_ordinal})", - ) - else: - ordinal = msg_ordinal + msg_ordinal, _ = found_match + ordinal, err = _reconcile_client_ordinal( + rid, sid, truncate_user_ordinal, msg_ordinal, + "truncate_before_row_id", target_row_id, + ) + if err is not None: + return err elif truncate_message_id is not None: msg_id_str = str(truncate_message_id) found_match = None @@ -431,56 +451,17 @@ def _(rid, params: dict) -> dict: "target user message is no longer in session history", ) - msg_ordinal, target_idx = found_match - - if truncate_user_ordinal is not None: - if isinstance(truncate_user_ordinal, bool): - return _err( - rid, - 4004, - "truncate_before_user_ordinal must be an integer", - ) - try: - ordinal = int(truncate_user_ordinal) - except (TypeError, ValueError): - return _err( - rid, - 4004, - "truncate_before_user_ordinal must be an integer", - ) - if ordinal != msg_ordinal: - logger.warning( - "prompt.submit: REFUSED truncation due to ordinal mismatch for session %s " - "(ordinal=%d, message_id_ordinal=%d, message_id=%s). " - "Stale truncate_before_user_ordinal detected.", - sid, - ordinal, - msg_ordinal, - msg_id_str, - ) - return _err( - rid, - 4030, - f"truncate_before_user_ordinal ({ordinal}) does not match " - f"truncate_before_message_id target turn ({msg_ordinal})", - ) - else: - ordinal = msg_ordinal + msg_ordinal, _ = found_match + ordinal, err = _reconcile_client_ordinal( + rid, sid, truncate_user_ordinal, msg_ordinal, + "truncate_before_message_id", msg_id_str, + ) + if err is not None: + return err else: - if isinstance(truncate_user_ordinal, bool): - return _err( - rid, - 4004, - "truncate_before_user_ordinal must be an integer", - ) - try: - ordinal = int(truncate_user_ordinal) - except (TypeError, ValueError): - return _err( - rid, - 4004, - "truncate_before_user_ordinal must be an integer", - ) + ordinal, err = _coerce_truncate_ordinal(rid, truncate_user_ordinal) + if err is not None: + return err if ordinal < 0 or ordinal >= len(user_indices): return _err( @@ -488,7 +469,6 @@ def _(rid, params: dict) -> dict: 4018, "target user message is no longer in session history", ) - target_idx = user_indices[ordinal] # An ordinal/id alone is not consent. A client that carries a leftover # ordinal into an ORDINARY submit sends a request that is @@ -515,10 +495,7 @@ def _(rid, params: dict) -> dict: "an ordinary prompt.submit must not drop session history " "(update your Hermes client if a rewind was intended)", ) - user_indices = [ - i for i, m in enumerate(history) - if m.get("role") == "user" and not m.get("display_kind") - ] + user_indices = _history_user_indices(history) # 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 @@ -1336,6 +1313,8 @@ def register(server) -> None: _mem_db_pair_agrees, _find_user_turn_by_row_id, _resolve_truncate_row_id, + _coerce_truncate_ordinal, + _reconcile_client_ordinal, _pending_reaction_notes, ): setattr(