Repository navigation
fix(gateway): persist internal prompt pins across restart #125832
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0da3722
f38beed
d15d253
679eacc
8928355
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -477,6 +477,32 @@ def sanitize_model_override(override: Optional[Dict[str, Any]]) -> Optional[Dict | |
| return cleaned or None | ||
|
|
||
|
|
||
| _PROMPT_PIN_VERSION = 1 | ||
|
|
||
|
|
||
| def sanitize_prompt_pin(pin: Any) -> Optional[Dict[str, Any]]: | ||
| """Validated durable snapshot of the exact ephemeral inputs reused by internal turns.""" | ||
| if not isinstance(pin, dict) or pin.get("version") != _PROMPT_PIN_VERSION: | ||
| return None | ||
| context_key = pin.get("context_key") | ||
| context_prompt = pin.get("context_prompt") | ||
| channel_prompt = pin.get("channel_prompt") | ||
| parent_chat_id = pin.get("parent_chat_id") | ||
| if not isinstance(context_key, str) or not context_key or not isinstance(context_prompt, str): | ||
| return None | ||
| if channel_prompt is not None and not isinstance(channel_prompt, str): | ||
| return None | ||
| if parent_chat_id is not None and not isinstance(parent_chat_id, str): | ||
| return None | ||
| return { | ||
| "version": _PROMPT_PIN_VERSION, | ||
| "context_key": context_key, | ||
| "context_prompt": context_prompt, | ||
| "channel_prompt": channel_prompt, | ||
| "parent_chat_id": parent_chat_id, | ||
| } | ||
|
|
||
|
|
||
| @dataclass | ||
| class SessionEntry: | ||
| """Routing-index entry: maps a session key to its current session ID and metadata.""" | ||
|
|
@@ -533,6 +559,9 @@ class SessionEntry: | |
| # "default" spelled out). The key namespace only says where the turn RUNS; after a restart this is | ||
| # what says which bot may deliver to it. None = unknown (row predates the field, or standalone). | ||
| transport_profile: Optional[str] = None | ||
| # Exact session-context/channel inputs from the last human turn. Append-only dataclass field so | ||
| # older positional construction of transport_profile keeps its meaning. | ||
| prompt_pin: Optional[Dict[str, Any]] = None | ||
|
|
||
| # Fields (de)serialized verbatim, in wire order (``from_dict`` reads them with | ||
| # ``data.get(name, <dataclass default>)``), split around the three ISO-datetime/token keys. | ||
|
|
@@ -562,6 +591,10 @@ def to_dict(self) -> Dict[str, Any]: | |
| if self.model_override: | ||
| # Defence-in-depth against an unsanitized dict stored directly. | ||
| result["model_override"] = sanitize_model_override(self.model_override) | ||
| if self.prompt_pin: | ||
| # Same defence-in-depth: routing JSON must never preserve malformed pin state. | ||
| if pin := sanitize_prompt_pin(self.prompt_pin): | ||
| result["prompt_pin"] = pin | ||
| if self.transport_profile: | ||
| result["transport_profile"] = self.transport_profile | ||
| if self.origin: | ||
|
|
@@ -604,6 +637,7 @@ def from_dict(cls, data: Dict[str, Any]) -> "SessionEntry": | |
| last_resume_marked_at=_parse_iso(data.get("last_resume_marked_at")), | ||
| active_turn_token=token, active_turn_started_at=started_at, | ||
| model_override=sanitize_model_override(data.get("model_override")), | ||
| prompt_pin=sanitize_prompt_pin(data.get("prompt_pin")), | ||
| transport_profile=transport_profile if isinstance(transport_profile, str) and transport_profile else None, | ||
| **plain, | ||
| ) | ||
|
|
@@ -1110,6 +1144,48 @@ def get_model_override(self, session_key: str) -> Optional[Dict[str, str]]: | |
| entry = self._entry_locked(session_key) | ||
| return dict(entry.model_override) if entry and entry.model_override else None | ||
|
|
||
| def set_prompt_pin( | ||
| self, session_key: str, pin: Optional[Dict[str, Any]], *, | ||
| expected_session_id: Optional[str] = None, | ||
| ) -> bool: | ||
| """Persist effective prompt inputs without letting a stale turn cross a boundary. | ||
|
|
||
| The candidate is durable before the in-memory entry is published. The expected session id | ||
| is the turn's launch identity: a concurrent reset or resume makes this a no-op instead of | ||
| writing the old conversation's system bytes onto the new one. | ||
| """ | ||
| cleaned = sanitize_prompt_pin(pin) | ||
| if pin is not None and cleaned is None: | ||
| return False | ||
| with self._lock: | ||
| entry = self._entry_locked(session_key) | ||
| if entry is None: | ||
| return False | ||
| if expected_session_id is not None and entry.session_id != expected_session_id: | ||
| return False | ||
| if entry.prompt_pin == cleaned: | ||
| return True | ||
| candidate = entry.to_dict() | ||
| if cleaned is None: | ||
| candidate.pop("prompt_pin", None) | ||
| else: | ||
| candidate["prompt_pin"] = cleaned | ||
| self._save_entry(session_key, entry_data=candidate, lock_held=True) | ||
| entry.prompt_pin = dict(cleaned) if cleaned is not None else None | ||
| return True | ||
|
|
||
| def get_prompt_pin( | ||
| self, session_key: str, *, expected_session_id: Optional[str] = None, | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """Return the prompt pin only while the route still owns the caller's session.""" | ||
| with self._lock: | ||
| entry = self._entry_locked(session_key) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1 — this read loses the snapshot while loading a compression-recovered route.
At this head I drove a human turn through the real handler, committed compression through Preserve the snapshot when recovery proves the replacement is a compression descendant of its owner, and save the child route with it. Keep unrelated/new conversations and explicit boundaries from inheriting it. This regression needs both sides of the crash window, not only a fresh store whose route already names the live child. Runnable regression — current head: 1 passed, 1 failedSave as HERMES_PYTHON=/path/to/test-environment/bin/python bash scripts/run_tests.sh \
tests/gateway/test_prompt_pin_review_probe.py -q -s --tb=short -j 1 --file-retries 0"""Independent review probes for durable prompt-pin lifecycle boundaries."""
import pytest
from gateway.config import GatewayConfig, Platform
from gateway.session import SessionSource, SessionStore
@pytest.mark.parametrize("publish_route", [True, False], ids=["normal-advance", "crash-before-route-advance"])
@pytest.mark.asyncio
async def test_compression_restart_preserves_prompt_pin(tmp_path, monkeypatch, publish_route):
import gateway.session as subject
from tests.gateway.test_internal_event_pin_wiring import _make_runner, _capture, _drive, _human_source, _wake_source
print("SUBJECT", subject.__file__)
config = GatewayConfig()
sessions = tmp_path / "sessions"
source = _human_source()
store = SessionStore(sessions_dir=sessions, config=config)
before = _make_runner(monkeypatch, config)
before.session_store = store
calls_before = []
_capture(before, calls_before)
await _drive(before, ((False, source),), channel_prompt="human channel hint")
entry = store.get_or_create_session(source)
key, parent = entry.session_key, entry.session_id
pin = store.get_prompt_pin(key, expected_session_id=parent)
assert pin and pin["channel_prompt"] == "human channel hint"
db = store._db
child = "compressed-child"
assert db.try_acquire_compression_lock(parent, "review-compressor")
db.publish_compression_child(
parent_session_id=parent, child_session_id=child, source="discord",
messages=[{"role": "user", "content": "compressed continuation"}],
compression_lock_holder="review-compressor",
)
if publish_route:
assert store.advance_compression_session(key, parent, child)
# Compression committed, but a process crash can precede routing publication.
(sessions / "sessions.json").unlink(missing_ok=True)
db.close()
restarted = SessionStore(sessions_dir=sessions, config=config)
try:
actual = restarted.get_prompt_pin(key, expected_session_id=child)
recovered = restarted._entries[key]
print("RECOVERED", recovered.session_id, "PIN", actual)
assert recovered.session_id == child
after = _make_runner(monkeypatch, config)
after.session_store = restarted
calls_after = []
_capture(after, calls_after)
await _drive(after, ((True, _wake_source()),))
assert len(calls_before) == len(calls_after) == 1
print("TURN_CHANNEL", calls_before[0]["channel_prompt"], "->", calls_after[0]["channel_prompt"])
print("CONTEXT_EQUAL", calls_before[0]["context_prompt"] == calls_after[0]["context_prompt"])
assert calls_after[0]["channel_prompt"] == calls_before[0]["channel_prompt"]
assert calls_after[0]["context_prompt"] == calls_before[0]["context_prompt"]
assert actual == pin
finally:
restarted._db.close()Observed for The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in #126137 by 87f8ef5. When restart recovery repoints a stale route, I ran your probe verbatim on the final head (f8954fe): both |
||
| if entry is None: | ||
| return None | ||
| if expected_session_id is not None and entry.session_id != expected_session_id: | ||
| return None | ||
| return dict(entry.prompt_pin) if entry.prompt_pin else None | ||
|
|
||
| def reset_session(self, session_key: str, display_name: Optional[str] = None) -> Optional[SessionEntry]: | ||
| """Force reset a session, creating a new session ID.""" | ||
| with self._lock: | ||
|
|
@@ -1224,13 +1300,16 @@ def remove_by_session_id(self, session_id: str) -> int: | |
| # restart-resume freshness gate (#85709). | ||
| def switch_session( | ||
| self, session_key: str, target_session_id: str, *, expected_session_id: Optional[str] = None, | ||
| preserve_prompt_pin: bool = True, | ||
| ) -> Optional[SessionEntry]: | ||
| """Point a session key at an existing session ID (``/resume``): ends the current row and | ||
| reopens the target so resume matches the CLI. | ||
|
|
||
| ``expected_session_id`` makes the repoint a compare-and-swap: ``None`` is returned when | ||
| the key no longer points at that session, so a caller that resolved against a snapshot | ||
| across an await (async-delegation re-pin) cannot overwrite a concurrent /new or /resume. | ||
| Prompt pins follow non-boundary repoints by default; /resume opts out explicitly because it | ||
| starts a different conversation on the same routing key. | ||
| """ | ||
| with self._lock: | ||
| old_entry = self._entry_locked(session_key) | ||
|
|
@@ -1247,6 +1326,10 @@ def switch_session( | |
| new_entry = self._replace_route_locked( | ||
| session_key, old_entry, target_session_id, _now(), | ||
| display_name=old_entry.display_name, model_override=old_entry.model_override, | ||
| prompt_pin=( | ||
| dict(old_entry.prompt_pin) | ||
| if preserve_prompt_pin and old_entry.prompt_pin is not None else None | ||
| ), | ||
| ) | ||
|
|
||
| if self._db_for_key(session_key) and old_entry.session_id: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1 — validate current privacy policy before accepting a durable pin. On Telegram/WhatsApp/Signal, a pin created with
privacy.redact_pii=Falsecan be restored after restart when the current setting isTrue. The read fence succeeds for the same session; lines 718–719 then return the old unredacted text before the privacy-aware key/renderer is consulted. A cold runner on the base instead renders withTrue. This is separate from the compression-recovery finding.Persist/validate a separate effective privacy-policy discriminator (including legacy-pin handling), not the whole current metadata key: internal sources intentionally lack human metadata. Unchanged-policy restarts should still reuse exact bytes.
Repository-native regression suggestion, to append to
tests/gateway/test_internal_event_pin_wiring.pyusing its existing helpers/imports. This integration test is suggested, not executed here; the review body reports the separately executed bounded probe accurately.Also preserve the existing unchanged-policy byte-equality tests and add an old-format snapshot case. Current policy must win before the bytes reach
_run_agent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional execution evidence for this existing P1, at the unchanged head
8928355d002697108443ac7b0edfcd28b02a2738; this is a follow-up to the finding above, not another finding or another formal review.I reran the cold-runner differential with the production
build_session_context_prompt, ID-hashing helpers, source-description methods, and privacy-aware_ephemeral_change_keyextracted from the pinned source, rather than a reduced renderer. A human turn renders underredact_pii=False; its real_prompt_pin_snapshot()output is JSON-round-tripped into a fresh host; an internal-first call then runs withredact_pii=Trueand the same expected session ID. The key itself changes when privacy changes, but the restored-pin return bypasses it.Linux / CPython 3.13.5, separate processes for base and head,
TZ=UTC LANG=C.UTF-8 PYTHONHASHSEED=0. Observed returned text for the synthetic identifier5550123456:At the head, all four discriminating controls behaved as expected: human-first after the policy change redacted; internal-first with no durable pin redacted; a different stored session ID rejected the snapshot and redacted; unchanged
Falsepolicy reused the old bytes without rendering. The last case is the intended cache behavior, not a failure. The source-description, user-ID and origin-delivery fields all expose the same bypass; these are not three different bugs.Evidence boundary: this remains source-isolated execution, not a complete gateway/SQLite/provider run. The cache methods and renderer execute; the session-state host and session-ID-fenced getter are explicit fixtures, and the routing record is JSON-round-tripped. The supported Telegram renderer branch is exercised; unrelated platform-note hooks and the full config loader are not. Source tracing verifies that
_hmwa_prepare_turnreads the live flag and passes it into this helper, and_handle_message_with_agentpasses the returnedprepared.context_promptto_run_agent(gateway/run_turn.py:2068–2081,2207–2211). I did not rerun the earlier review's 123-test suite or compression/SQLite probe in this environment, and I am not claiming live external disclosure or retroactive redaction of transcript history.This strengthens the existing policy finding without changing its scope or severity. Keep same-policy exact-byte reuse and the session-ID fence; separately validate effective privacy/render-policy identity before promoting durable prompt bytes into the current turn. An opaque metadata hash alone cannot establish that an old snapshot is safe under the current policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #126137 by 62eb2b3. The pin now records the
privacy.redact_piiit was rendered under, as a required field in both the in-memory pin (ephemeral_pinis(key, text, redact_pii)) and the durable snapshot. Pinned bytes are never reused under a different policy. That applies to internal turns after a restart and also within a running process, which had the same bypass until the next human turn. If the policy hasn't changed, restarts still reuse the exact bytes.Your suggested test passes on the final head (f8954fe), both internal-first and human-first. The branch has
test_internal_event_never_reuses_prompt_pin_from_another_privacy_policy, and removing the policy check makes it fail. On legacy snapshots:sanitize_prompt_pinrefuses a pin withoutredact_pii, so it falls back to a fresh render. v1 was never released, so there are no older on-disk pins to migrate.