Skip to content

fix(gateway): persist internal prompt pins across restart - #125832

Closed
JoaoMarcos44 wants to merge 5 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/125793-persist-internal-prompt-pins
Closed

JoaoMarcos44 wants to merge 5 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/125793-persist-internal-prompt-pins

Conversation

@JoaoMarcos44

@JoaoMarcos44 JoaoMarcos44 commented Sep 27, 2026 •

Copy link
Copy Markdown

Fixes #125793.

Root cause

The steady-state pinning from #124179 is process-local:

  • ConversationState.ephemeral_pin holds the exact rendered session-context bytes;
  • ConversationState.channel_pin holds the effective channel_prompt + parent_chat_id.

After a gateway restart those fields are empty. If the first turn for an existing session is internal (startup resume, recovered process completion, restored delegation/wake), it cannot reconstruct the exact human-turn prompt identity: the internal event commonly has no channel_prompt, and its source may also lack human-only context. The API-time system prompt therefore flips A→B until the next human turn repins A.

The lifetime was wrong: the pins protect provider/local prompt caches that may outlive the gateway process, but the pins themselves did not.

Fix

Persist one versioned, coherent prompt-pin snapshot on the session routing entry:

  • exact session-context change key + rendered bytes;
  • effective channel_prompt;
  • pinned parent_chat_id.

The routing index/state.db already owns durable per-session routing state, so this needs no SQLite schema migration.

On a human turn:

  1. normal current-config pin resolution remains authoritative;
  2. after both context and channel pins are resolved, the coherent pair is persisted before agent execution;
  3. unchanged snapshots do not write again.

On an internal-first turn after restart:

  1. a missing in-memory pin lazily rehydrates from the routing entry;
  2. the exact previous bytes and parent-channel authority are restored;
  3. the internal turn reuses the same system-prompt inputs the session used before restart.

A human-first turn after restart deliberately does not rehydrate stale durable bytes; it recomputes from current configuration and replaces the durable snapshot.

Boundary / concurrency safety

I initially considered the generic SessionEntry.metadata primitive, but three lifecycle hazards make a dedicated contract safer:

  1. stale writers: a human turn can be in flight while /new or /resume moves the routing key. set_prompt_pin(..., expected_session_id=...) is a CAS: an old turn cannot write old-conversation system bytes onto the new session;
  2. stale readers: an internal turn can resolve session A, then a boundary can repoint the same routing key to session B before lazy rehydration. get_prompt_pin(..., expected_session_id=...) now fences the read too, so the old turn cannot consume the new conversation's prompt bytes;
  3. non-boundary repoints: switch_session() is also used by internal continuation/repoint flows. Prompt pins therefore follow those repoints by default, while true conversation boundaries explicitly opt out: /resume and CLI→gateway handoff both pass preserve_prompt_pin=False.

The store persists the candidate before publishing it in memory, rejects malformed snapshots rather than interpreting them as a clear, and appends the new dataclass field after existing positional fields for compatibility.

Performance / failure behavior

  • Snapshot equality is checked before the async store write, so normal unchanged human turns do not produce routing writes.
  • The existing single-entry routing UPSERT is used; this is not a full index rewrite in the normal state.db path.
  • Persistence failure is fail-soft: the current turn still runs with its correct in-memory pins.
  • Old routing rows without the versioned field load normally with prompt_pin=None.

Regression coverage

The committed regressions cover both storage and call-site wiring:

  • real SessionStore: prompt pin survives a fresh store instance from state.db even with sessions.json removed;
  • malformed durable pin cannot erase a valid snapshot;
  • malformed in-memory prompt-pin state is omitted from serialization instead of emitting "prompt_pin": null;
  • reset drops the pin;
  • stale pre-reset session_id cannot write across the boundary;
  • stale pre-reset session_id cannot read a post-reset prompt pin across the same routing key;
  • non-boundary repoint preserves the pin;
  • boundary repoint can explicitly clear it;
  • the real /resume handler passes preserve_prompt_pin=False;
  • the real CLI handoff path also clears any prompt pin from the route it replaces before dispatching its synthetic internal turn;
  • human → fresh runner → first internal → human preserves context bytes, channel prompt, parent override and the effective ephemeral prompt;
  • human-first after restart ignores stale durable inputs and refreshes them from current configuration;
  • identical human prompt inputs do not rewrite durable state.

I have not run the repository pytest lane locally in this connector-only workflow, so I am not claiming a local green suite. Current CI/Nix/Docker runs are action_required with jobs: [], so no hosted job has executed yet.

Related work / non-duplication

Immediately before publication I searched open PRs by #125793 and by prompt_pin, channel_pin, ephemeral_pin, persistence and restart terms. No overlapping implementation was found.

No cherry-pick or third-party production patch was reused. The root implementation and follow-up fixes were authored directly on this branch. During review, @teknium1 used maintainer edits to push the test-only compatibility commit 679eacc84a044696421b7d0de358eb65126b88da, widening existing hand-written switch_session stubs for the new keyword argument; that maintainer-authored test fix is retained and explicitly listed below.

10 blocker / improvement passes

  1. Reality: traced the post-restart internal-first path on current upstream and confirmed the lifetime mismatch.
  2. Duplication: compared fix(gateway): internal events keep the session-context and channel prompt pins #124179, [Bug]: Leftover steer / interrupt-text follow-ups run with channel_prompt=None → system prompt flips, agent rebuild, prompt-cache miss #125763, fix(gateway): preserve eventless follow-up channel prompt #125783/fix(gateway): preserve channel prompt for eventless follow-ups #125784 and symbol/issue searches; the persistence gap remains independent.
  3. Storage owner: rejected process-local state and parallel files; reused the existing durable routing entry/state.db boundary.
  4. Concurrency: found both directions of the route-boundary race: stale writers are fenced by set_prompt_pin(... expected_session_id=...), and the later adversarial pass found stale readers too; rehydration is now fenced by the resolved session_id before accepting durable bytes.
  5. Lifecycle: audited reset, resume, compression/repoint, continuation and CLI handoff. This post-PR pass found one real gap: CLI handoff is also a conversation boundary but initially inherited the prior route's durable pin. A dedicated second commit clears it and regression-tests the caller.
  6. Profile/multiplex: prompt state follows the routing entry and therefore the existing profile-scoped routing owner/rekey/purge behavior.
  7. Compatibility/corruption: append-only dataclass field, versioned sanitizer, defensive copies, malformed writes rejected.
  8. Performance/privacy: bounded existing prompt inputs only; unchanged snapshots skip writes; no credentials/secrets added.
  9. Restart polarity: internal-first rehydrates; human-first recomputes current inputs; unchanged human turns are write-idempotent.
  10. Exact-head gate: rebased after upstream drift and rechecked after the post-PR concurrency/lifecycle/review fixes; final branch is 5 commits ahead / 0 behind main@bac0c45d8593ed9d53a8e3fcacecdc920b71a2c4, remains mergeable, and duplicate searches still return only this PR. No hosted run is attached to the current head yet, so no fresh green-CI claim is made.

Commits

  • 0da37221523633bef2c3454ece033f8eac285893 — fix(gateway): persist internal prompt pins
  • f38beed0a2de48947aae2086d998148fc409a7b7 — fix(gateway): clear prompt pin on CLI handoff
  • d15d25312dfaf62add09fe36d2203be5288d28fe — fix(gateway): fence prompt pin rehydration
  • 679eacc84a044696421b7d0de358eb65126b88da — test(gateway): accept preserve_prompt_pin in handoff switch_session stubs (@teknium1 maintainer edit)
  • 8928355d002697108443ac7b0edfcd28b02a2738 — fix(gateway): omit malformed prompt pins from routing state

The first commit is parented directly to current upstream main@bac0c45d8593ed9d53a8e3fcacecdc920b71a2c4; the second and third are focused post-PR hardening commits, the fourth is the maintainer-authored test compatibility fix from review, and the fifth addresses the review nit that malformed in-memory state should not serialize as a null prompt pin.

AI assistance disclosure: AI assistance was used for investigation, implementation, adversarial validation, and drafting. I am submitting the change and take responsibility for it.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P0 Critical — data loss, security, crash loop area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) labels Sep 27, 2026
JoaoMarcos44 and others added 2 commits September 27, 2026 20:48
…tubs

The CLI-handoff commit passes preserve_prompt_pin=False to switch_session, so
the two hand-written stubs that fake the store on that path (secondary-profile
adapter, Telegram DM topic lane) raised TypeError on the new kwarg and the
exact-call assertion no longer matched. Widen the stubs and the assertion;
no production change.
teknium1
teknium1 previously approved these changes Sep 27, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: approve (code review only; the merge decision is Teknium's).

Context. #124179 made internal events (kanban wakes, delegation completions, watch notifications) reuse the last human turn's session-context bytes and channel inputs so the system prompt stays byte-stable across turns, but both pins live only in ConversationState. After a gateway restart the first internal turn renders from the internal source, and the next human turn re-keys it back (A→B→A): one prefix miss + one agent rebuild per affected session (#125793).

What it does. Persists a versioned {context_key, context_prompt, channel_prompt, parent_chat_id} snapshot on the routing SessionEntry (state.db gateway_routing row, single-entry UPSERT), written on human turns before _run_agent only when changed, rehydrated lazily on the internal path when the in-memory pin is missing. Human-first after restart recomputes from current config and overwrites. /new drops it via the fresh entry; /resume and CLI handoff pass preserve_prompt_pin=False; compression/continuation repoints keep it. Read and write are CAS-fenced on the turn's launch session_id.

Verified locally (worktree on origin/main tip, fake HOME):

  • PR test files: 7 new/changed tests red with main's gateway/ sources, green on the PR head (98 passed).
  • Real-store E2E (not committed): real SessionStore over state.db, human×2 → close store, delete sessions.json → fresh SessionStore + fresh GatewayRunner → internal wake → human. context_prompt, channel_prompt, parent_chat_id and the combined ephemeral prompt are byte-identical across all three turns; second human turn produced no durable rewrite. Same probe on main: AttributeError: get_prompt_pin / A→B→A.
  • 85 gateway test files touching switch_session / SessionEntry / pins / handoff / resume: 862 passed after the fix below (the one remaining red, test_session_store_prune::test_session_store_default_db_uses_runtime_hermes_home, is red on main in my environment too — unrelated).
  • ruff clean on all touched files.

Defect found and fixed on the branch (679eacc8, pushed via maintainer edits): the CLI-handoff commit's new preserve_prompt_pin=False kwarg broke three existing tests whose hand-written switch_session stubs have a fixed signature — tests/gateway/test_handoff_secondary_profile_adapter.py (2 tests, TypeError: unexpected keyword argument) and tests/gateway/test_telegram_topic_mode.py::test_handoff_to_telegram_dm_topic_uses_dm_lane_not_generic_thread (stub TypeError + assert_called_once_with mismatch). Widened the stubs and the assertion; no production change. CI on f38beed0 would have gone red on these.

Non-blocking notes (inline): to_dict can emit "prompt_pin": null; routing-index/sessions.json growth (~1.4 KB per session measured on the Discord thread fixture); the queued-follow-up path (run_turn.py ~L3875, outside the diff) re-pins channel_pin in memory for human follow-ups but never calls _persist_prompt_pins, so the durable snapshot can lag until the next top-level human turn — per-chat config is identical within a drain, so the only effect is one self-healing rehydration in a restart-during-drain window (harmless asymmetry). The getattr(store, "get_prompt_pin")/callable guards in _rehydrate_prompt_pins/_persist_prompt_pins only exist for MagicMock stores in tests; the real store always has both — fine to leave, but they are not protecting anything in production.

The read-side fence in d15d2531 is correct and cheap; no concerns.

Comment thread gateway/session.py Outdated
Comment thread gateway/session.py
@JoaoMarcos44
JoaoMarcos44 requested a review from a team September 27, 2026 23:58
@JoaoMarcos44
JoaoMarcos44 force-pushed the fix/125793-persist-internal-prompt-pins branch from 687275d to 679eacc Compare September 27, 2026 23:59

Copy link
Copy Markdown
Author

@teknium1 thanks for the review and for pushing the handoff-stub compatibility fix in 679eacc84a044696421b7d0de358eb65126b88da.

I addressed the actionable serialization nit in 8928355d002697108443ac7b0edfcd28b02a2738:

  • malformed in-memory prompt_pin state is now omitted from SessionEntry.to_dict() instead of becoming "prompt_pin": null;
  • added test_malformed_prompt_pin_is_omitted_from_serialized_entry as a regression.

I replied to and resolved both inline threads. For the ~1.4 KB/session routing-mirror trade-off, I kept the current routing-entry ownership deliberately: moving the snapshot to session metadata would broaden the lifecycle/storage design for a point you marked non-blocking, while the current CAS/boundary behavior is already covered.

I also rechecked the exact branch after the change: current head is 8928355d002697108443ac7b0edfcd28b02a2738, 5 commits ahead / 0 behind main@bac0c45d8593ed9d53a8e3fcacecdc920b71a2c4, with the expected 11 changed files, and GitHub reports the PR mergeable.

The head change automatically dismissed the prior approval. No hosted workflow/status is attached to this new head yet, so I am not claiming a fresh green CI run.

Copy link
Copy Markdown
Author

@teknium1 thanks for the review and for the maintainer-edit test fix.

I addressed the actionable nit from review 5332730737 in 8928355d002697108443ac7b0edfcd28b02a2738:

  • SessionEntry.to_dict() now emits prompt_pin only when sanitize_prompt_pin(...) returns a valid snapshot, so malformed in-memory state is omitted instead of serialized as "prompt_pin": null;
  • added test_malformed_prompt_pin_is_omitted_from_serialized_entry to lock that behavior.

I also kept your test-only compatibility commit 679eacc84a044696421b7d0de358eb65126b88da, which widens the existing hand-written switch_session stubs for preserve_prompt_pin.

On the storage-size note: I kept the routing-entry design rather than moving the snapshot to session metadata. Your point about sessions.json mirror growth is valid, but changing storage ownership at this stage would broaden the lifecycle surface materially; the current design already has explicit read/write session-id fencing, boundary clears, restart coverage, and unchanged-snapshot write elision. I documented the trade-off rather than hiding it.

I did not expand this PR to persist event-less queued-followup re-pins mid-drain. As you noted, per-chat config is identical within the drain and the only restart-window effect is a self-healing rehydration on the next top-level turn, so that remains a harmless asymmetry rather than a correctness blocker for #125793.

Current head: 8928355d002697108443ac7b0edfcd28b02a2738. GitHub reports the PR mergeable. No hosted workflow run is attached to this head yet, so I am not claiming fresh CI green.

If you have a chance, please re-review the new head; GitHub dismissed the prior approval when the follow-up commits changed it.

Copy link
Copy Markdown
Author

@teknium1 the actionable review feedback is addressed on the current head 8928355d002697108443ac7b0edfcd28b02a2738 and both inline threads are resolved. The push auto-dismissed the previous approval; when convenient, could you re-review this head? I’m not claiming fresh hosted CI yet because no status/run is attached to this commit.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still loses the prompt pins in a real restart-recovery path. The ordinary restart case passes, but a crash between compression publication and routing-index advancement restores the child session without the snapshot. I reproduced the loss through the real message handler and a real SQLite store at 8928355d002697108443ac7b0edfcd28b02a2738.

P1 — compression recovery discards the durable prompt snapshot

A human turn persists its pins, then SessionDB.publish_compression_child() atomically ends the parent and publishes its child. If the process stops before advance_compression_session() moves the routing entry, the restart loader finds the ended parent and recovers the child. _stale_entry_verdict() returns a newly constructed entry, and that constructor does not carry prompt_pin. The loader replaces the original entry and saves the replacement. The snapshot is lost before the new get_prompt_pin() can return it.

The resulting first internal turn reached the agent boundary with channel_prompt=None instead of "human channel hint", and its context differed from the last human turn. The otherwise identical control, which advances the route before restart, retained both values. That leaves the restart/cache-continuity defect from #125793 open for a committed compression whose route publication was interrupted. This is an incomplete repair of the existing defect class, not a claim that this PR introduced compression or transcript loss.

Carry the coherent snapshot through recovery of a verified compression descendant, then persist the recovered route with it. Do not copy pins onto an arbitrary newer session merely because it shares the routing key. Keep the explicit reset/resume/handoff clears. The regression must cover human turn → real compression publication → restart before route advance → internal turn, with exact prompt equality and the normal-advance control. The inline finding includes the runnable probe.

Verification and prior review

I ran the six changed test files through scripts/run_tests.sh on Linux / Python 3.14.7: 123 passed, exit 0. The separate compression/restart probe produced 1 passed, 1 failed, exit 1. It uses the existing handler harness with a real SessionStore, a real compression lease and publish_compression_child(), removes sessions.json, then constructs a fresh store and runner. Provider execution is captured at _run_agent; no live provider cache-hit or billing claim is being made.

The malformed-pin serialization regression passes, as do the handoff/resume tests after Teknium's retained test-stub commit 679eacc8. The previously noted routing-mirror size trade-off is not the blocker here. I also traced the queued-follow-up sibling: #125783 and #125784 address eventless prompt forwarding; neither closes this startup recovery path. This review does not claim coverage of every queued-drain/config-change interleaving.

Reviewed the five-commit branch against bac0c45d8593ed9d53a8e3fcacecdc920b71a2c4. Live main had advanced to 801a9022a742562a3c4578c0d8dd12cfd393fc47; its intervening file changes do not touch the gateway/session/compression files behind this finding.

CI remains a merge gate

The exact head has no check runs or commit statuses. Its CI, Docker, and Nix workflow runs are action_required. The successful required-check aggregate belongs to 679eacc8, not the reviewed head. Across the five commits, 0da37221 and d15d2531 also have no check runs/statuses, while f38beed0 has a failed required-check aggregate and cancelled jobs. The branch is not green at every commit. Local test results do not replace that gate.

Comment thread gateway/session.py
) -> 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — this read loses the snapshot while loading a compression-recovered route.

_entry_locked() can run startup recovery before this getter examines the pin. When compression has committed but the routing entry still names its parent, _stale_entry_verdict() returns the child entry built by _create_entry_from_recovered_row(). That entry has prompt_pin=None; the loader replaces the original and saves it. Preserving pins in switch_session() does not cover this path.

At this head I drove a human turn through the real handler, committed compression through publish_compression_child() with a real lease, deleted the JSON mirror and opened a fresh store/runner. With route advancement before restart, the internal turn retained its channel hint and context. Without that advancement, recovery still selected the correct child, but the internal turn received channel_prompt=None and different context. No provider was contacted.

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 failed

Save as tests/gateway/test_prompt_pin_review_probe.py and run:

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 crash-before-route-advance:

RECOVERED compressed-child PIN None
TURN_CHANNEL human channel hint -> None
CONTEXT_EQUAL False
AssertionError: assert None == 'human channel hint'

The normal-advance control passes, with the channel hint retained and CONTEXT_EQUAL True. Linux, Python 3.14.7, isolated test homes, real SQLite and compression transaction; the existing harness captures provider execution at _run_agent.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #126137 by 87f8ef5. When restart recovery repoints a stale route, _stale_entry_verdict now carries prompt_pin over only if the recovered session is the ended parent's compression tip (_compression_tip_for_session_id), so unrelated or new conversations never inherit it. The repointed child is saved with the pin.

I ran your probe verbatim on the final head (f8954fe): both normal-advance and crash-before-route-advance pass. The branch also has a permanent store-level regression test, test_prompt_pin_follows_compression_child_recovered_after_crash. Removing the carry-over makes that test fail.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supplemental P1 — durable prompt reuse must respect the current privacy policy

The routing-entry ownership, candidate-before-publication write, and session-ID read/write fences are useful parts of this fix. This is an additional finding for #125793, not a repost of my already-published compression-recovery review. It is a different boundary: preserving the conversation does not preserve permission to send its old unredacted system bytes.

Reviewed head: 8928355d002697108443ac7b0edfcd28b02a2738; comparison base: bac0c45d8593ed9d53a8e3fcacecdc920b71a2c4.

P1: enabling privacy.redact_pii across restart is ignored by the internal-first path

On a supported platform such as Telegram, run a human turn with privacy.redact_pii: false, then enable it, restart the gateway without changing the conversation, and let a startup-resume/delegation/process wake run before another human message. The new durable snapshot contains the old clear-text identifiers. The session-ID comparison succeeds because it is still the same conversation. _rehydrate_prompt_pins() restores the snapshot, and _pinned_session_context_prompt() immediately returns the old rendered bytes.

The code path is explicit:

  • run_turn.py:2072-2081 reads privacy.redact_pii for this message and passes it to the pin helper.
  • run_agent_cache.py:714-720 restores the durable pin and returns it before evaluating the current change key or renderer.
  • run_agent_cache.py:792-804 already includes bool(redact_pii) in that key, but the internal return bypasses it.
  • session.py:374-462 is the renderer that hashes model-visible user/chat/home-channel identifiers on supported platforms. The documented privacy setting promises this redaction before the system prompt reaches the LLM; routing retains the original IDs separately.

This is a regression relative to a cold runner on the comparison base: without durable rehydration, its first internal call has no pin and renders with the newly enabled setting. The pre-existing same-process internal-pin behavior is not being attributed to this PR. This addition carries that stale privacy state across a process restart as well. The observable effect is new model input containing identifiers that the current configuration says to hash, not merely an avoidable cache miss. It does not imply that redaction can retract identifiers sent before the setting was enabled.

Executed bounded probe, with controls

I ran a six-case source-excerpt probe against the retrieved base/head pin-helper branches: 5 passed, 1 failed; exit 1. The failing case is false → restart → true, internal-first; it returned **User ID:** "123456789" for a synthetic Telegram-shaped fixture. Cold-base internal-first, head human-first, unchanged-false, unchanged-true, and a rejected mismatched session-ID were the five passing controls.

Scope matters: this is a reduced local control-flow probe, with a JSON-file store double, a reduced renderer for the production User ID field, and a change-key double that retains the privacy dimension. It is not a full GatewayRunner/real-SessionStore integration run or a provider call. The source trace above establishes the production wiring. I have not rerun the repository pytest suite for this addendum and am not claiming the earlier review's suite counts as a new run.

Probe receipt: probe_privacy_restart.py, SHA-256 4ebd7b53a0778f1c72e31204cd76ebcbde684353cb10a454fa7496c76d36b2c7; command python probe_privacy_restart.py. The inline comment includes a repository-native regression suggestion using the existing handler harness; that suggested integration test has not been executed here.

Required invariant and repair

Durable cache continuity may preserve metadata identity, but must not override the current privacy policy. Bind the persisted snapshot to the effective privacy/render-policy identity and validate it before accepting restored bytes. On mismatch or an older snapshot with no demonstrable policy match, rebuild safe context under the current policy rather than replaying the old text. Preserve exact-byte reuse when policy is unchanged.

Do not simply compare the entire current context hash: internal sources intentionally omit human metadata, so that would undo #125793's purpose. Keep the policy discriminator separate from the human-metadata continuity key, and validate any new wire field in the sanitizer. Add the changed-policy/internal-first regression, the human-first control, unchanged-policy byte-equality controls, and an older-snapshot compatibility case. Preserve the current session-ID fences and explicit conversation-boundary clears.

The exact head still reports zero check runs and zero commit statuses in my fresh reads; this addendum is not an approval or a green-CI claim. The separate compression-recovery finding and CI gate remain as recorded in the earlier review. I am not reopening the accepted mirror-size trade-off or the resolved malformed-pin nit.

Credit remains with @aidiffuser for the restart reproducer, @JoaoMarcos44 for this implementation, and @teknium1 for the independent triage/review and retained handoff-test fix. #124179 supplies the steady-state pinning lineage; #125763 and #125783/#125784 are the event-less sibling, not this privacy-policy boundary.

Comment on lines +714 to +717
if internal and _eph_pin is None:
self._rehydrate_prompt_pins(session_key, expected_session_id)
_pin_state = self._peek_session_state(session_key) if session_key else None
_eph_pin = _pin_state.conversation.ephemeral_pin if _pin_state else None

Copy link
Copy Markdown

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=False can be restored after restart when the current setting is True. 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 with True. 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.py using its existing helpers/imports. This integration test is suggested, not executed here; the review body reports the separately executed bounded probe accurately.

@pytest.mark.asyncio
@pytest.mark.parametrize("first_internal", [True, False])
async def test_restart_pin_respects_new_redaction_setting(monkeypatch, first_internal):
    import gateway.run as gr

    config = {"privacy": {"redact_pii": False}}
    monkeypatch.setattr(gr, "_load_gateway_config", lambda: config)
    durable = {}
    raw_id = "123456789"  # synthetic fixture

    def source():
        return SessionSource(
            platform=Platform.TELEGRAM, chat_id=raw_id, user_id=raw_id,
            chat_type="dm", message_id="synthetic-message",
        )

    before = _make_runner(monkeypatch, durable_prompt_pin=durable)
    before.session_store.get_or_create_session.return_value.platform = Platform.TELEGRAM
    old_calls = []
    _capture(before, old_calls)
    await _drive(before, ((False, source()),))
    assert len(old_calls) == 1
    assert raw_id in old_calls[0]["context_prompt"]
    assert isinstance(durable.get("value"), dict)

    config["privacy"]["redact_pii"] = True
    after = _make_runner(monkeypatch, durable_prompt_pin=durable)
    after.session_store.get_or_create_session.return_value.platform = Platform.TELEGRAM
    new_calls = []
    _capture(after, new_calls)
    await _drive(after, ((first_internal, source()),))
    assert len(new_calls) == 1
    assert raw_id not in new_calls[0]["context_prompt"]

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.

Copy link
Copy Markdown

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_key extracted from the pinned source, rather than a reduced renderer. A human turn renders under redact_pii=False; its real _prompt_pin_snapshot() output is JSON-round-tripped into a fresh host; an internal-first call then runs with redact_pii=True and 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 identifier 5550123456:

base bac0c45d8593ed9d53a8e3fcacecdc920b71a2c4, internal-first, redact_pii=True:
**Source:** Telegram ("DM with user_54f87d384a95")
**User ID:** "user_54f87d384a95"
- `"origin"` → Back to this chat ("54f87d384a95")
renderer calls: 1; privacy assertion: PASS; process exit: 0

head 8928355d002697108443ac7b0edfcd28b02a2738, internal-first, redact_pii=True:
**Source:** Telegram ("DM with 5550123456")
**User ID:** "5550123456"
- `"origin"` → Back to this chat ("5550123456")
renderer calls: 0; privacy assertion: FAIL; process exit: 1
AssertionError: redact_pii=True returned raw pre-restart identifier bytes

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 False policy 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_turn reads the live flag and passes it into this helper, and _handle_message_with_agent passes the returned prepared.context_prompt to _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.

Copy link
Copy Markdown

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_pii it was rendered under, as a required field in both the in-memory pin (ephemeral_pin is (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_pin refuses a pin without redact_pii, so it falls back to a fresh render. v1 was never released, so there are no older on-disk pins to migrate.

@kshitijk4poor

Copy link
Copy Markdown

Thanks @JoaoMarcos44. I've carried this forward in #126137 with your commits cherry-picked, so authorship is kept, and @teknium1's stub fix is included too. On top of them it adds:

  • the two P1s raised here: bytes rendered under the old redact_pii setting are reused after a restart, and the pin is dropped in the compression crash window
  • queued human follow-ups now persist their pin
  • the rehydrate read moves off the event loop

Live A→B→A probes and results are in the PR body. I'll leave this open until #126137 lands.

@kshitijk4poor

Copy link
Copy Markdown

Closing in favour of #126137, which is armed for rebase-merge. It carries all five commits from this branch unchanged: @JoaoMarcos44's four, with authorship kept, and @teknium1's handoff-stub fix.

What #126137 adds on top:

  • Privacy (andrexibiza's P1): the pin records the redact_pii it was rendered under and is never reused under a different policy.
  • Compression crash window (andrexibiza's P1): restart recovery keeps the pin when the recovered session is the parent's compression tip.
  • Queued follow-ups now persist their re-pinned channel inputs. This was teknium's non-blocking note.
  • Rehydrate reads through async_session_store, off the event loop, and only runs when the process holds no pins for the key. The MagicMock-only getattr/callable guards teknium flagged are gone.
  • Placement: the store API moves from the gateway/session.py facade into gateway/session_prompt_pin.py.

Both reviewer probes pass on the final head, and the routing-entry design and the to_dict omission from this PR are unchanged. Thanks again, Joao.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P0 Critical — data loss, security, crash loop sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Internal-event pins are in-memory: the first internal event after a gateway restart still flips the system prompt

5 participants