Skip to content

fix(session-db): persist post-flush /steer mutation to SQLite - #52604

Open
youngting520 wants to merge 1 commit into
NousResearch:mainfrom
youngting520:fix/steer-sessiondb-persistence
Open

youngting520 wants to merge 1 commit into
NousResearch:mainfrom
youngting520:fix/steer-sessiondb-persistence

Conversation

@youngting520

@youngting520 youngting520 commented Jun 25, 2026 •

Copy link
Copy Markdown

What does this PR do?

Fixes a durable-persistence bug where a mid-turn /steer is delivered to the live model but is not written back to SQLite SessionDB (state.db). A later resume, session API read, or search could therefore replay a transcript that differs from the content that actually produced the answer.

It also fixes the sibling post-flush mutation in aggregate tool-output budget enforcement: enforce_turn_budget can rewrite an already-flushed tool result in place, leaving the durable row with the pre-budget content while the model sees the replacement.

Root cause

AIAgent._flush_messages_to_session_db() marks message dicts with _db_persisted after writing them. Later flushes skip those dicts, and the bounded identity-prefix scan can skip the entire already-seen prefix. /steer and aggregate budget enforcement intentionally mutate tool-result content after that first write, so the durable row remained stale.

Approach on current main

This revision is rebased directly onto 9712b8f0c and follows the current batched persistence design:

  1. Steer injection and aggregate budget rewrites stamp _db_content_update_pending on the intentionally mutated tool message.
  2. The bounded identity-prefix scan stops before a pending mutation.
  3. The flush performs a scoped UPDATE of the active tool row, using its exact _row_id and falling back only to a unique tool_call_id.
  4. UPDATEs reuse the current transcript-write guards and patience budget, so compression leases and compression-closed sessions remain protected.
  5. New turn rows still land through one append_messages_batch() transaction. The batch API now returns row IDs only when return_row_ids=True; the live flush uses that opt-in to stamp freshly inserted tool messages without changing the default inserted-count contract.
  6. TUI branch seeding keeps current main's bounded chunked batch writes, preserves assistant/tool pairing metadata, and re-stamps copied messages with child-session row IDs.

The original revision also re-added per-tool steer drains. Current main deliberately delivers /steer at the batch-end drain after aggregate budget enforcement, so this revision preserves that ordering and does not restore the old drain scheme.

Changes made

  • agent/persistence_markers.py centralizes _db_persisted and _db_content_update_pending.
  • run_agent.py updates pending durable rows, integrates row-ID capture with batched inserts, stops the prefix fast path at pending mutations, and strips private persistence metadata from JSON snapshots.
  • hermes_state.py adds fail-closed update_tool_message_content(...) and opt-in row-ID returns for batched inserts.
  • agent/agent_runtime_helpers.py, agent/conversation_loop.py, and tools/tool_result_storage.py explicitly mark the three intentional mutation sites.
  • CLI, gateway, and TUI live-resume paths opt into row IDs; inspection/export consumers retain the historical transcript shape.
  • tui_gateway/methods_session.py preserves tool pairing and child-row identity without reverting current main's chunked batch-copy optimization.
  • agent/context_compressor.py removes both persistence markers when re-baselining compacted messages.
  • Regression coverage exercises real SQLite, FTS updates, compression leases, rewritten and duplicate row identities, TUI branching, concurrent/sequential steer drains, aggregate-budget rewrites, marker stripping, and the batched write contract.

Validation

6 focused files: 914 passed, 0 failed
ruff check: passed
git diff --check: passed

The focused run includes:

  • tests/run_agent/test_steer_sessiondb_persistence.py
  • tests/hermes_state/test_append_messages_batch.py
  • tests/tools/test_tool_result_storage.py
  • tests/agent/test_context_compressor.py
  • tests/run_agent/test_run_agent.py
  • tests/test_tui_gateway_server.py

Scope / impact

Affected: incremental SQLite persistence for normal CLI, gateway, and TUI turns, plus TUI branch copies.

No model-tool schema, config key, system-prompt content, or user-facing command surface changes. Full-transcript rewrite paths remain unchanged, and current main's batch-end-only steer delivery remains intact.

Checklist

  • Bug fix with real-path regression coverage
  • Single commit rebased directly onto latest main
  • Conventional commit title
  • Reuses the opt-in _row_id contract; no duplicate row-id metadata
  • Preserves batched turn flushes and bounded branch-copy transactions
  • No unrelated config, documentation, or tool-schema changes
  • Tested on Ubuntu / Python 3.11

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jun 25, 2026
@youngting520 youngting520 added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jun 25, 2026
@youngting520

Copy link
Copy Markdown
Author

Re-ran the regression module on Ubuntu (Linux 6.17, x86_64) via the canonical
runner — all green, including the FTS reindex assertion (sqlite built with FTS5):

scripts/run_tests.sh tests/run_agent/test_steer_sessiondb_persistence.py
→ 7 passed, 0 failed (13.1s)

CI hasn't been able to run on this PR yet — as a cross-repo first-time
contributor it needs a maintainer to approve workflow runs. Happy to address
any review feedback.

@youngting520
youngting520 force-pushed the fix/steer-sessiondb-persistence branch 2 times, most recently from 4aa74f9 to 0699bf6 Compare July 2, 2026 15:35

@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.

Thanks for tracing this to the incremental-flush marker path. The persistence gap remains on current main: agent/tool_executor.py:996-1005 flushes a tool row before /steer mutates it, while run_agent.py:1850-1851 skips that marker-stamped message on the later flush.

Problems

  • tests/run_agent/test_steer_sessiondb_persistence.py:56 uses tempfile.mkdtemp() for the patched Hermes home without cleanup. The test helper should use tmp_path or a TemporaryDirectory owner so test runs do not leave directories behind.

Suggested changes

  • Preserve the current-main persistence behavior while salvaging the row-update logic; GitHub currently reports this branch as non-mergeable against main.
  • Keep the SQLite/FTS regression coverage, including the direct post-flush mutation case.

This is an automated hermes-sweeper review.

Comment thread tests/run_agent/test_steer_sessiondb_persistence.py Outdated
@teknium1 teknium1 added the sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit label Jul 15, 2026
@youngting520
youngting520 force-pushed the fix/steer-sessiondb-persistence branch 2 times, most recently from c480f84 to a89d4f2 Compare July 16, 2026 14:14
@youngting520
youngting520 force-pushed the fix/steer-sessiondb-persistence branch from a89d4f2 to fef2ef0 Compare July 19, 2026 06:12
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
@youngting520
youngting520 force-pushed the fix/steer-sessiondb-persistence branch 9 times, most recently from 497bb1f to afb3ede Compare July 30, 2026 11:33
@youngting520
youngting520 force-pushed the fix/steer-sessiondb-persistence branch from 7e5ebc7 to c034f31 Compare August 2, 2026 14:08
@youngting520
youngting520 force-pushed the fix/steer-sessiondb-persistence branch from c034f31 to 993bc47 Compare August 4, 2026 18:04
Mid-turn /steer appends its marker to a tool result that the
incremental flush may already have written to state.db, leaving the
durable row stale (resume/session API/search would replay a transcript
that differs from what produced the answer). The aggregate tool-output
budget rewrite has the same post-flush mutation shape.

Both mutation sites now stamp _db_content_update_pending, and the
flush performs a scoped in-place UPDATE (via the opt-in _row_id
contract, falling back to a unique tool_call_id) instead of skipping
the row; the identity-prefix fast path stops before any pending
mutation.

Rebased onto main after 0fd0db1 ("preserve /steer through
turn-budget enforcement"): upstream now keeps /steer pending until
after aggregate budget enforcement, so this revision drops the
per-tool drains and the _steer_budget_protected_suffix machinery the
original patch added to defend them.

This branch has not been deployed

No deployments
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/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit 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.

3 participants