diff --git a/apps/desktop/src/app/session/hooks/use-prompt-actions/rewind.ts b/apps/desktop/src/app/session/hooks/use-prompt-actions/rewind.ts index d4b02639dab30..40ac314bf9b8f 100644 --- a/apps/desktop/src/app/session/hooks/use-prompt-actions/rewind.ts +++ b/apps/desktop/src/app/session/hooks/use-prompt-actions/rewind.ts @@ -18,6 +18,7 @@ import { branchGroupForUser, type ChatMessage, chatMessageText, textPart } from import { appendText, isSessionBusyError, + isVisibleUserMessage, visibleUserIndexAtOrdinal, visibleUserOrdinal, withSessionBusyRetry, @@ -66,7 +67,7 @@ export function rebindSurvivorRowIds(messages: ChatMessage[], survivorRowIds: Su let ordinal = 0 return messages.map(message => { - if (message.role !== 'user' || message.hidden) { + if (!isVisibleUserMessage(message)) { return message } diff --git a/apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts b/apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts index 52c2555669d41..bf4b3bd77168e 100644 --- a/apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts +++ b/apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts @@ -495,8 +495,15 @@ export function appendText(message: AppendMessage): string { .trim() } +/** The one visible-user filter every user-ordinal computation must share — + * truncate ordinals, ordinal→index resolution, and survivor-rowId rebinding + * all rely on counting exactly the same turns. */ +export function isVisibleUserMessage(message: ChatMessage): boolean { + return message.role === 'user' && !message.hidden +} + export function visibleUserOrdinal(messages: readonly ChatMessage[], end: number): number { - return messages.slice(0, end).filter(m => m.role === 'user' && !m.hidden).length + return messages.slice(0, end).filter(isVisibleUserMessage).length } export function visibleUserIndexAtOrdinal(messages: readonly ChatMessage[], targetOrdinal: number): number { @@ -505,7 +512,7 @@ export function visibleUserIndexAtOrdinal(messages: readonly ChatMessage[], targ for (let index = 0; index < messages.length; index += 1) { const message = messages[index] - if (message.role !== 'user' || message.hidden) { + if (!isVisibleUserMessage(message)) { continue } diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 9f624eda25e76..788e1043bac2d 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -17922,3 +17922,80 @@ def test_prompt_submit_consecutive_rewinds_with_returned_survivor_row_ids( assert isinstance(survivors2, list) and len(survivors2) == 1 finally: server._sessions.pop(sid, None) + + +def test_prompt_submit_unconfirmed_truncation_refuses_before_target_resolution( + monkeypatch, +): + """Consent (4029) is checked BEFORE target resolution: an unconfirmed + submit carrying truncation params must not pay the durable-transcript + read or heal-stamp live history (simplify review on #83785), and an + out-of-range unconfirmed ordinal refuses 4029, not 4018 — the baseline + precedence before the row-id feature. + """ + db_reads = [] + + class _SpyDB: + def get_messages_as_conversation(self, *a, **k): + db_reads.append(1) + return [] + + def replace_messages(self, *a, **k): + pytest.fail("must not write") + + hist = [ + {"role": "user", "content": "first"}, + {"role": "assistant", "content": "r1"}, + {"role": "user", "content": "second"}, + {"role": "assistant", "content": "r2"}, + ] + sess = _session(history=list(hist), session_key="consent-precedence-key") + sid = "consent-precedence-sid" + server._sessions[sid] = sess + monkeypatch.setattr(server, "_get_db", lambda: _SpyDB()) + 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: + # Unconfirmed row_id: 4029, no DB read, no heal stamps on history. + resp = server.handle_request( + { + "id": "1", + "method": "prompt.submit", + "params": {"session_id": sid, "text": "x", "truncate_before_row_id": 3}, + } + ) + assert (resp.get("error") or {}).get("code") == 4029, resp + assert db_reads == [] + assert all("_row_id" not in m for m in sess["history"]) + + # Malformed param still beats consent: bool row_id is 4004. + resp = server.handle_request( + { + "id": "2", + "method": "prompt.submit", + "params": {"session_id": sid, "text": "x", "truncate_before_row_id": True}, + } + ) + assert (resp.get("error") or {}).get("code") == 4004, resp + + # Unconfirmed out-of-range ordinal: 4029 (consent), not 4018 (range). + resp = server.handle_request( + { + "id": "3", + "method": "prompt.submit", + "params": { + "session_id": sid, + "text": "x", + "truncate_before_user_ordinal": 99, + }, + } + ) + assert (resp.get("error") or {}).get("code") == 4029, resp + assert len(sess["history"]) == 4 + finally: + server._sessions.pop(sid, None) diff --git a/tui_gateway/methods_prompt.py b/tui_gateway/methods_prompt.py index d4fdb5c3dccec..0e13b4cb2451f 100644 --- a/tui_gateway/methods_prompt.py +++ b/tui_gateway/methods_prompt.py @@ -155,19 +155,19 @@ 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. +def _coerce_truncate_int(rid, value, param_name="truncate_before_user_ordinal"): + """Return ``(int_value, error_response)`` for a client-supplied integer param. 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. + 1 and aim a confirmed rewind at the wrong 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") + return None, _err(rid, 4004, f"{param_name} 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") + return None, _err(rid, 4004, f"{param_name} must be an integer") def _reconcile_client_ordinal(rid, sid, client_ordinal, msg_ordinal, param_name, target_repr): @@ -180,7 +180,7 @@ def _reconcile_client_ordinal(rid, sid, client_ordinal, msg_ordinal, param_name, """ if client_ordinal is None: return msg_ordinal, None - ordinal, err = _coerce_truncate_ordinal(rid, client_ordinal) + ordinal, err = _coerce_truncate_int(rid, client_ordinal) if err is not None: return None, err if ordinal != msg_ordinal: @@ -380,26 +380,60 @@ def _(rid, params: dict) -> dict: or truncate_row_id is not None ): history = session.get("history", []) + + # Malformed params refuse first (4004), regardless of consent — + # the historical ordinal-path precedence. + target_row_id = None + if truncate_row_id is not None: + target_row_id, err = _coerce_truncate_int( + rid, truncate_row_id, "truncate_before_row_id" + ) + if err is not None: + return err + client_ordinal = None + if truncate_user_ordinal is not None: + client_ordinal, err = _coerce_truncate_int(rid, truncate_user_ordinal) + if err is not None: + return err + + # An ordinal/id alone is not consent. A client that carries a leftover + # ordinal into an ORDINARY submit sends a request that is + # indistinguishable, field by field, from a real rewind — same + # method, same shape, an in-range target — and the cut it asks for + # is a destructive replace_messages() the user never requested + # (#80763: 296 -> 52 messages, 244 durable rows gone). Only the + # client knows whether this submit is a rewind/edit/regenerate, so + # it has to say so; refuse the cut when it doesn't. Consent is + # checked BEFORE target resolution: an unconfirmed (leaked-state) + # request must refuse with 4029 without paying the durable + # transcript read or heal-stamping live history dicts that + # row-id resolution performs. + if not is_truthy_value(params.get("confirm_truncate")): + logger.warning( + "prompt.submit: REFUSED unconfirmed truncation of session %s " + "(%d messages held; ordinal=%s, row_id=%s, message_id=%s). " + "The client attached truncation parameters without " + "confirm_truncate — likely stale truncation parameters on " + "an ordinary submit.", + sid, + len(history), + client_ordinal, + target_row_id, + truncate_message_id, + ) + return _err( + rid, + 4029, + "truncation parameters require confirm_truncate=true; " + "an ordinary prompt.submit must not drop session history " + "(update your Hermes client if a rewind was intended)", + ) + user_indices = _history_user_indices(history) ordinal = None - if truncate_row_id is not None: - if isinstance(truncate_row_id, bool): - return _err( - rid, - 4004, - "truncate_before_row_id must be an integer", - ) - try: - target_row_id = int(truncate_row_id) - except (TypeError, ValueError): - return _err( - rid, - 4004, - "truncate_before_row_id must be an integer", - ) - + if target_row_id is not None: # Durable address first — never degrade a missing row_id into a # client ordinal cut (#82959 / #82766 review). Unknown id refuses # without touching data; stale ordinal with a *resolved* row_id @@ -407,8 +441,6 @@ def _(rid, params: dict) -> dict: found_match = _resolve_truncate_row_id( session, history, target_row_id ) - # user_indices may have been healed with _row_id stamps - user_indices = _history_user_indices(history) if found_match is None: logger.warning( @@ -425,7 +457,7 @@ def _(rid, params: dict) -> dict: msg_ordinal, _ = found_match ordinal, err = _reconcile_client_ordinal( - rid, sid, truncate_user_ordinal, msg_ordinal, + rid, sid, client_ordinal, msg_ordinal, "truncate_before_row_id", target_row_id, ) if err is not None: @@ -457,49 +489,14 @@ def _(rid, params: dict) -> dict: msg_ordinal, _ = found_match ordinal, err = _reconcile_client_ordinal( - rid, sid, truncate_user_ordinal, msg_ordinal, + rid, sid, client_ordinal, msg_ordinal, "truncate_before_message_id", msg_id_str, ) if err is not None: return err else: - ordinal, err = _coerce_truncate_ordinal(rid, truncate_user_ordinal) - if err is not None: - return err + ordinal = client_ordinal - if ordinal < 0 or ordinal >= len(user_indices): - return _err( - rid, - 4018, - "target user message is no longer in session history", - ) - - # An ordinal/id alone is not consent. A client that carries a leftover - # ordinal into an ORDINARY submit sends a request that is - # indistinguishable, field by field, from a real rewind — same - # method, same shape, an in-range target — and the cut it asks for - # is a destructive replace_messages() the user never requested - # (#80763: 296 -> 52 messages, 244 durable rows gone). Only the - # client knows whether this submit is a rewind/edit/regenerate, so - # it has to say so; refuse the cut when it doesn't. - if not is_truthy_value(params.get("confirm_truncate")): - logger.warning( - "prompt.submit: REFUSED unconfirmed truncation of session %s " - "(%d messages held; ordinal=%d). The client attached " - "truncation parameter without confirm_truncate — " - "likely stale truncation parameters on an ordinary submit.", - sid, - len(history), - ordinal, - ) - return _err( - rid, - 4029, - "truncation parameters require confirm_truncate=true; " - "an ordinary prompt.submit must not drop session history " - "(update your Hermes client if a rewind was intended)", - ) - 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 @@ -1350,7 +1347,7 @@ def register(server) -> None: _mem_db_pair_agrees, _find_user_turn_by_row_id, _resolve_truncate_row_id, - _coerce_truncate_ordinal, + _coerce_truncate_int, _reconcile_client_ordinal, _pending_reaction_notes, ): diff --git a/website/docs/developer-guide/programmatic-integration.md b/website/docs/developer-guide/programmatic-integration.md index 621fb322b846a..b74dbff452f75 100644 --- a/website/docs/developer-guide/programmatic-integration.md +++ b/website/docs/developer-guide/programmatic-integration.md @@ -70,6 +70,8 @@ A rewind / edit / regenerate is a `prompt.submit` that drops part of the stored A truncation parameter without `confirm_truncate` is refused with code `4004` or `4029` and nothing is written. Hosts that implement rewind must set the flag at the moment the user asks for it, and must never keep truncation parameters in state across ordinary submits. Prefer `truncate_before_row_id` (from resume `row_id` / `_row_id`) over ordinals; keep the ordinal as a back-compat / optimistic-row path only when no durable id is available yet. +On a successful truncating submit against a durable session, the `prompt.submit` result additionally carries `survivor_user_row_ids` — the fresh post-rewrite row IDs of the surviving user turns, in visible-user-ordinal order. The rewrite re-inserts the kept prefix as new rows, so every row ID the host cached before the rewind is stale afterward; rebind cached IDs from this list (a `null` entry means that turn has no durable ID — drop the cached one) or the next rewind targeting an older surviving turn will be refused with `4018`. + ### Events streamed back `message.delta`, `message.complete`, `tool.start`, `tool.progress`, `tool.complete`, `approval.request`, `clarify.request`, `sudo.request`, `sudo.expire`, `secret.request`, `secret.expire`, `gateway.ready`, plus session lifecycle and error events. Expiry events carry the original `{ request_id }`; external hosts should clear only the matching pending prompt.