Skip to content

fix(gateway): preserve archived compaction history on /retry - #80218

Closed
Adolanium wants to merge 2 commits into
NousResearch:mainfrom
Adolanium:fix/gateway-retry-preserve-archived-history
Closed

fix(gateway): preserve archived compaction history on /retry#80218
Adolanium wants to merge 2 commits into
NousResearch:mainfrom
Adolanium:fix/gateway-retry-preserve-archived-history

Conversation

@Adolanium

Copy link
Copy Markdown
Contributor

What does this PR do?

On messaging platforms, /retry truncates the live transcript to before the last user message and persists it through SessionStore.rewrite_transcript, which calls replace_messages() with the default active_only=False. That DELETEs every row for the session, including the soft-archived active=0/compacted=1 rows that in-place compaction keeps on disk (#38763). Any /retry after a compaction permanently wiped the archived pre-compaction history.

#57803 named this call site as a residual gap after its global-default approach was rejected in favor of per-caller guards, and the TUI sibling (prompt.submit truncation) was fixed in #80195. This is the gateway half of that gap.

The handler now probes has_archived_messages() and passes active_only=True when archives exist, so only the live rows are replaced. The default stays False because yuanbao recall redaction deliberately purges. Same contract the ACP adapter's _persist and the gateway /compress comments already document (#61145, #44794, #39704).

Related Issue

Fixes #80216

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Security fix
  • Documentation update
  • Tests (adding or improving test coverage)
  • Refactor (no behavior change)
  • New skill (bundled or hub)

Changes Made

  • gateway/slash_commands.py: _handle_retry_command probes has_archived_messages() and passes active_only=has_archived to rewrite_transcript.
  • gateway/session.py: rewrite_transcript gains an active_only parameter (default False, forwarded to replace_messages), plus a SessionStore.has_archived_messages wrapper that AsyncSessionStore exposes automatically through its __getattr__ offload. Also corrects the rewrite_transcript docstring, which still listed /undo as a caller even though /undo soft-archives via rewind_session.
  • tests/gateway/test_retry_replacement.py: new regression test drives _handle_retry_command against a real SessionStore and SessionDB seeded with soft-archived compaction rows and asserts the archives survive (active=0/compacted=1) and the live set reflects the truncation plus the retried exchange.

Coordination note: #62031 (open, yuanbao recall) also adds an active_only parameter to rewrite_transcript for its own caller. The two are complementary. If it merges first this PR shrinks to the call site; if this merges first its rebase is trivial.

How to Test

  1. scripts/run_tests.sh tests/gateway/test_retry_replacement.py -q
    • passes, includes the new archived-rows regression test
  2. scripts/run_tests.sh tests/gateway/test_retry_replacement.py tests/gateway/test_retry_response.py tests/gateway/test_session.py tests/gateway/test_compress_command.py tests/gateway/test_session_hygiene.py -q
    • 86 passed, 0 failed
  3. Manual: compact a gateway session (archived rows appear as active=0), send /retry, then SELECT count(*) FROM messages WHERE session_id = ? AND active = 0. Before the fix: 0. After: unchanged.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the suites above and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) - the rewrite_transcript docstring is updated as part of the fix (stale /undo reference removed, active_only semantics documented)
  • I've updated cli-config.yaml.example if I added/changed config keys - N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows - N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide - SQLite query path only, no platform surface. Developed and tested on Windows
  • I've updated tool descriptions/schemas if I changed tool behavior - N/A

Screenshots / Logs

$ scripts/run_tests.sh tests/gateway/test_retry_replacement.py tests/gateway/test_retry_response.py tests/gateway/test_session.py tests/gateway/test_compress_command.py tests/gateway/test_session_hygiene.py -q
=== Summary: 5 files, 86 tests passed, 0 failed (100% complete) in 8.3s (32 workers) ===

/retry truncates the live transcript to before the last user message
and persists it via SessionStore.rewrite_transcript, which calls
replace_messages() with the default active_only=False. That DELETEs
every row for the session, including the soft-archived
active=0/compacted=1 rows that in-place compaction keeps on disk
(NousResearch#38763), so any /retry after a compaction permanently wiped the
archived history. NousResearch#57803 named this call site as a residual gap after
its global-default approach was rejected; the TUI sibling was fixed
in NousResearch#80195.

The handler now probes has_archived_messages() (new SessionStore
wrapper, auto-exposed through AsyncSessionStore) and passes
active_only=True when archives exist, so only the live rows are
replaced. rewrite_transcript gains an active_only parameter that
defaults to False, keeping the destructive semantics yuanbao recall
redaction depends on. Also corrects the rewrite_transcript docstring,
which still listed /undo as a caller even though /undo soft-archives
via rewind_session.

The regression test drives _handle_retry_command against a real
SessionStore and SessionDB seeded with archived compaction rows and
asserts the archives survive.
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter platform/discord Discord bot adapter platform/slack Slack app adapter platform/whatsapp WhatsApp Business adapter sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 6, 2026

poisdahl commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

For #80216, I reproduced the current CI failure on the exact PR head
(76e0dd36):
tests/gateway/test_discord_channel_prompts.py::test_retry_preserves_channel_prompt
now raises AttributeError because its existing SimpleNamespace store has
no has_archived_messages method.

There is also a data-safety issue in the probe design itself:

  • SessionStore.has_archived_messages() catches any lookup exception and
    returns False, which sends /retry back through the destructive
    active_only=False rewrite and can delete the archive this PR is meant to
    preserve.
  • Probe and rewrite are separate async offloads/DB lock scopes. A compaction
    that commits after a False probe but before the rewrite creates a TOCTOU
    window where the new archive is deleted.

I prepared a narrow follow-up commit directly on this PR head:

  • make /retry pass active_only=True unconditionally;
  • remove the new one-use SessionStore probe wrapper;
  • strengthen the real-DB regression by making the low-level archive probe
    raise and asserting that /retry neither calls it nor loses archived rows.

This is equivalent for ordinary sessions: /retry loads and rewrites only the
active transcript, and when no inactive rows exist active_only=True deletes
the same rows as False. When compaction/undo history exists, it preserves the
rows /retry never intends to purge. Other rewrite_transcript callers retain
the destructive default. messages.active is NOT NULL DEFAULT 1, and schema
initialization heals legacy NULL values on every open, so without inactive
rows the AND active = 1 predicate deletes exactly the same set.

Evidence:

  • PR head + strengthened regression: 1 passed, 1 failed; the archived row
    set became empty after the simulated lookup failure.
  • candidate focused gate: 6 passed, 0 failed, including the CI-failing
    Discord test;
  • broader retry/session/compression/hygiene gate: 257 passed, 0 failed;
  • targeted Ruff and git diff --check: passed;
  • an independent read-only review of the commit (code trace; tests not
    re-executed by that reviewer) found no blocking issues.

Public commit (based directly on 76e0dd36):
poisdahl@476e4ee

Branch:
https://github.com/poisdahl/hermes-agent/tree/codex/pr-80218-retry-archive-safety

The commit's parent is this PR's head (76e0dd36), so it applies with no
rebase:

git remote add poisdahl https://github.com/poisdahl/hermes-agent.git
git fetch poisdahl codex/pr-80218-retry-archive-safety
git cherry-pick 476e4ee33072a5c675099dbc34955d43c70c706c

This matches replace_messages' documented contract: callers sharing a
session id with in-place compaction must preserve the rows the agent archived.

@Adolanium

Copy link
Copy Markdown
Contributor Author

Verified all three points locally, including the CI failure on our head. Cherry-picked your commit as-is (3d03e56), full retry/session/compress/hygiene/discord gate passes 90/0. Thanks for the catch and the ready-made fix.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #80913 (salvage). Your commits cherry-picked with authorship preserved — all three contributors (@Adolanium, @poisdahl, @Kshitij) credited in git history.

Your PR's approach (explicit active_only parameter, per-caller guard, regression test) was chosen over the competing probe-based approach in #80695 because it avoids the TOCTOU race and fail-open exception path. Thanks for the thorough fix!

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

Labels

comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround platform/discord Discord bot adapter platform/slack Slack app adapter platform/telegram Telegram bot adapter platform/whatsapp WhatsApp Business adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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]: /retry permanently deletes archived compaction history on messaging platforms

4 participants