fix(gateway): persist user messages at send time (#2409)

[henrypark133]

Summary

• User messages disappear when switching threads in the web gateway because the DB persist happens asynchronously in the agent loop — there's a timing gap where loadHistory() finds nothing
• chat_send_handler now writes the user message to DB immediately before returning 202, so history queries always find it
• Agent loop skips duplicate persist via a user_message_persisted metadata flag, scoped to the gateway channel only (non-gateway channels can't abuse it)
• Drain loop clears the flag on cloned messages so queued follow-up turns always get their own DB persist
• Incomplete turns now use time-based state: recent (<5 min) = "Processing", stale = "Failed"

Test plan

• cargo fmt — clean
• cargo clippy --all --all-features — zero warnings
• 19 build_turns unit tests pass (including new stale/recent split)
• 2 new libsql regression tests: test_early_persist_writes_user_message_to_db, test_db_allows_duplicate_user_messages_without_flag
• Manual: send message in thread A, switch to B, switch back to A — message visible with "Processing..." indicator
• Manual: wait for response, switch away and back — full turn visible
• Manual: refresh page — same behavior

Closes #2409

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request updates the project to version 0.25.0, bumping versions for internal crates and registry artifacts. It implements early user message persistence in the gateway to prevent message loss before the agent loop starts, with the agent loop now checking a metadata flag to avoid duplicates. Message state logic is also refined to mark recent unanswered messages as "Processing" instead of "Failed". Review feedback highlights that early persistence is currently skipped for new threads without an ID and recommends using safer JSON access methods to prevent potential panics when modifying message metadata.

[Copilot]

Pull request overview

Fixes a web gateway race where freshly-sent user messages could disappear on thread switch by persisting the user message to the DB at send time (before returning 202), and preventing duplicate persistence in the agent loop via a gateway-only metadata flag. Also adjusts how incomplete turns are displayed (recent = Processing, stale = Failed) and updates release/registry/versioning metadata for the 0.25.0 release.

Changes:

• Persist gateway user messages to the DB synchronously in chat_send_handler, and mark messages with user_message_persisted to dedup in the agent loop.
• Update turn building to classify incomplete turns as Processing if recent (<5 min) and Failed if stale.
• Bump versions / release config and update registry artifact URLs and checksums for 0.25.0.

Reviewed changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/channels/web/util.rs	Time-based state for incomplete turns + new unit tests.
src/channels/web/server.rs	Early DB persist in chat_send_handler; tests updated/added.
src/channels/web/handlers/chat.rs	Update tests to expect Processing for incomplete recent turns.
src/agent/thread_ops.rs	Skip duplicate persistence when gateway already wrote the user message.
release-plz.toml	Configure tagging/publishing behavior for workspace packages.
registry/tools/github.json	Bump tool version and artifact URL/checksum to 0.25.0 release.
registry/tools/composio.json	Add wasm artifact URL/checksum for Composio tool.
registry/channels/whatsapp.json	Update channel artifact URL/checksum to 0.25.0 release.
registry/channels/telegram.json	Update channel artifact URL/checksum to 0.25.0 release.
registry/channels/slack.json	Update channel artifact URL/checksum to 0.25.0 release.
registry/channels/feishu.json	Update channel artifact URL/checksum to 0.25.0 release.
registry/channels/discord.json	Update channel artifact URL/checksum to 0.25.0 release.
crates/ironclaw_tui/Cargo.toml	Mark crate as non-publishable.
crates/ironclaw_safety/Cargo.toml	Version bump to 0.2.1.
crates/ironclaw_safety/CHANGELOG.md	Add crate changelog entry for 0.2.1.
crates/ironclaw_gateway/Cargo.toml	Mark crate as non-publishable.
crates/ironclaw_engine/Cargo.toml	Update ironclaw_common dependency version reference.
crates/ironclaw_common/Cargo.toml	Version bump to 0.2.0.
crates/ironclaw_common/CHANGELOG.md	Add crate changelog entry for 0.2.0.
channels-src/telegram/src/lib.rs	Formatting-only refactor in message splitting/sending helpers.
Cargo.toml	Bump workspace version to 0.25.0; adjust path dependency declarations.
Cargo.lock	Lockfile updates for version bumps.
CHANGELOG.md	Add 0.25.0 release notes.
.github/workflows/release.yml	Restrict release workflow trigger to ironclaw-v* tags.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[henrypark133]

Review: Persist user messages at send time (Risk: Medium)

Well-designed fix for a real UX bug — early persist at gateway send time with metadata flag dedup is architecturally sound. The module placement follows existing patterns. A few items to address before merge.

Concerning: dispatch-exempt annotation imprecise

File: src/channels/web/server.rs (early-persist block)
The annotation reads // dispatch-exempt: pre-job write, no JobContext exists yet to dispatch through — the actual reason is a structural constraint (no JobContext at the gateway send boundary). Suggest: // dispatch-exempt: no JobContext available at gateway send boundary; dispatcher requires it for audit trail.

Concerning: Early persist bypasses TenantStore ownership layer

File: src/channels/web/server.rs
The early-persist calls store.add_conversation_message() on the raw Arc<dyn Database>, which is the only handler callsite that bypasses TenantStore. The ensure_conversation WHERE clause provides structural protection against cross-tenant writes, but this is a deviation from the established ownership pattern. Consider wrapping with a TenantStore-scoped call, or document why the structural guard is sufficient.

Concerning: 5-minute hardcoded threshold for Processing→Failed

File: src/channels/web/util.rs:99-105
The chrono::TimeDelta::minutes(5) threshold has no relationship to the agent's actual processing timeout. Long-running turns (>5 min) will show "Failed" in UI while still processing. Consider making this configurable or computing from agent turn timeout config.

Concerning: Missing caller-level integration test

The metadata flag dedup predicate in thread_ops.rs gates a DB write side effect, with the flag set in server.rs — this is exactly the wrapper-between-helper-and-side-effect shape that the project's "Test Through the Caller" rule targets. The two DB-tier tests verify the DB layer in isolation but don't exercise the gateway→agent_loop roundtrip. A test using GatewayWorkflowHarness that sends a message and asserts exactly one DB row would catch key-name mismatches between the two files.

Concerning: Ok(false) log message misleading

File: src/channels/web/server.rs
"Early persist skipped: conversation not writable" — Ok(false) from ensure_conversation means the thread belongs to a different user/channel, not a permissions/lock issue. Suggest: "Early persist skipped: thread_id belongs to a different user/channel".

Minor: Tests are libsql-only

Both integration tests are #[cfg(feature = "libsql")] with no PostgreSQL counterpart. Consider mirroring under #[cfg(feature = "integration")].

Minor: 5-minute boundary not tested

The time threshold tests cover 0ms ("Processing") and 10min ("Failed") but not the exact boundary. A test at TimeDelta::minutes(5) would confirm the strict < behavior.

Convention notes:

• Logging levels are correct (debug! throughout)
• thiserror patterns not changed
• No crate:: vs super:: violations

🤖 Generated with Claude Code

[henrypark133]

All review comments addressed in fa20833

From the self-review (#4102588621):

Comment	Status
dispatch-exempt annotation imprecise	Fixed — updated wording
Early persist bypasses TenantStore	Fixed — add_conversation_message now goes through TenantScope
5-minute hardcoded threshold	Extracted to INCOMPLETE_TURN_THRESHOLD constant
Missing caller-level integration test	Added test_gateway_persisted_flag_metadata_roundtrip + shared GATEWAY_PERSISTED_FLAG constant
Ok(false) log message misleading	Fixed — now says "thread belongs to a different user/channel"
Tests are libsql-only	Added PostgreSQL counterparts under #[cfg(all(feature = "integration", feature = "postgres"))]
5-minute boundary not tested	Added test_build_turns_incomplete_at_exact_threshold

From Copilot (2nd review):

Comment	Status
History ordering with concurrent messages	Fixed — early-persist now skips when thread is Processing
Drain-loop flag carry-over	Already fixed (flag removal) + now uses shared constant
PR description vs test name mismatch	Test was already renamed; PR description needs update

Changes:

• src/channels/web/util.rs — GATEWAY_PERSISTED_FLAG + INCOMPLETE_TURN_THRESHOLD constants, boundary test
• src/channels/web/server.rs — Processing check, TenantScope, log message, annotation, PG tests, flag roundtrip test
• src/agent/thread_ops.rs — uses shared constant
• src/agent/agent_loop.rs — uses shared constant

All unit tests pass, clippy clean, both backends compile.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This test assumes the agent hasn’t responded yet and asserts state is only Processing or Failed. In CI the agent loop can complete quickly enough that the first history fetch already returns Completed, which would make this test flaky even though early-persist is working. Consider allowing Completed here (or asserting only that the user turn is present immediately, regardless of state/response).

Suggested change

	# The agent hasn't responded yet, so the turn should be in-progress
	assert turns[0]["state"] in ("Processing", "Failed"), turns[0]["state"]
	# The user turn must already be persisted; depending on timing, the
	# agent may still be processing, may have failed, or may already have
	# completed before this first history fetch returns.
	assert turns[0]["state"] in ("Processing", "Failed", "Completed"), turns[0]["state"]

[serrrfirat]

This early persist stores the raw gateway request content, but the agent path later augments attachment context before persisting (thread_ops.rs builds effective_content with augment_with_attachments). Because this branch also sets GATEWAY_PERSISTED_FLAG, the agent skips the augmented DB write. That means image-only messages can persist as an empty string, and text+image messages lose the attachment context from durable history if the process crashes or when the DB transcript is reloaded.

Can we either early-persist the same augmented content after images_to_attachments(), or avoid setting the persisted flag when attachments/images are present so thread_ops.rs still persists effective_content? This needs caller-level coverage through /api/chat/send for image-only and text+image messages, not just the flag helper, because the bug is in the wrapper input to the DB write.

[serrrfirat]

Paranoid Security Review — APPROVE

PR: #2434 — Persist user messages at send time (#2409)
Reviewer context: Automated deep security review

Findings

Excellent tenant isolation and defense-in-depth.

• Authentication enforced: AuthenticatedUser extractor verifies bearer token before any new code. user_id derived from token, not request body.
• Cross-tenant write protection (dual layer): ensure_conversation() has SQL WHERE user_id = excluded.user_id clause. TenantScope::add_conversation_message() calls conversation_belongs_to_user() before INSERT.
• Channel-scoped flag trust: already_persisted gates on channel == "gateway" (set server-side) AND flag being true. Non-gateway channels cannot abuse the flag. Explicitly tested.
• SQL injection: none. All queries use parameterized placeholders.
• No new .unwrap()/.expect() in production code.
• Pre-safety persistence acceptable: Content is written to DB before safety pipeline runs, but per the project's "LLM data is never deleted" audit-log policy, blocked content in the DB is valuable for security review. Blocked content never reaches the LLM.

Minor notes (not blocking)

• No transaction wrapping ensure_conversation + add_conversation_message (the agent loop acts as a fallback — no data loss scenario).
• TOCTOU between thread_is_processing check and persist is benign (dedup flag catches double-persist, agent loop catches skip).

Test coverage is thorough: 3 unit, 2 flag round-trip, 4 DB integration, 5 E2E Playwright tests. LGTM.

[serrrfirat]

Medium Severity — Architecture

Inverted dependency: agent module imports from channel-specific module.

thread_ops.rs and agent_loop.rs both reference crate::channels::web::util::GATEWAY_PERSISTED_FLAG. This creates a dependency from the core agent module to a specific channel implementation (web gateway), inverting the expected dependency direction (channels depend on agent, not vice versa). If the web gateway is ever feature-gated or extracted, the agent module breaks.

Suggestion: Move GATEWAY_PERSISTED_FLAG to src/channels/mod.rs or add a method on IncomingMessage:

impl IncomingMessage {
    pub fn is_gateway_persisted(&self) -> bool {
        self.channel == "gateway"
            && self.metadata
                .get("user_message_persisted")
                .and_then(|v| v.as_bool())
                .unwrap_or(false)
    }
}

This keeps the constant in the channels layer without coupling agent→web, and also addresses Finding #2 (triplicated check logic) by centralizing the check in one place.

[serrrfirat]

Medium Severity — Maintainability

Triplicated already_persisted check logic.

The same 5-line pattern appears identically here, in thread_ops.rs:597-602, and is tested inline in two separate test functions. If the check logic ever needs to change (e.g., supporting additional trusted channels), all three sites must be updated in lockstep.

Suggestion: Extract into a single helper (e.g., IncomingMessage::is_gateway_persisted() as described in the companion comment on thread_ops.rs). All three call sites become one-liners, and tests can validate the helper directly.

[serrrfirat]

Review feedback addressed in 0fb8d90

Comment	Source	Status
Attachment data loss — early persist stores raw content, agent skips augmented persist when flag is set	@serrrfirat (server.rs:2287) + self-review	Fixed — GATEWAY_PERSISTED_FLAG is now skipped when req.images is non-empty, so the agent loop persists effective_content with attachment context
Inverted dependency — agent module imports GATEWAY_PERSISTED_FLAG from channels::web::util	@serrrfirat (thread_ops.rs:600)	Fixed — extracted IncomingMessage::is_gateway_persisted() method on channel.rs; agent/bridge modules call the method instead of importing the web-channel constant directly
Triplicated check logic — same 5-line pattern in thread_ops.rs, bridge/router.rs, and tests	@serrrfirat (bridge/router.rs:2421)	Fixed — all three call sites now use msg.is_gateway_persisted(), tests updated
E2E test flakiness — test asserts only Processing/Failed but agent can complete before first history fetch	Copilot (test_message_persistence.py:91)	Fixed — now allows Completed state
DB write failure log level — debug! should be warn! for persist failure	Self-review (server.rs:2262)	Fixed — promoted to warn!
Second drain-loop missing flag clear (ApprovalResponse path)	Copilot (agent_loop.rs:1749)	Already addressed — both drain-loops (lines 1747 and 1935) already clear the flag

Changes

• src/channels/channel.rs — new IncomingMessage::is_gateway_persisted() method
• src/channels/web/server.rs — skip persisted flag when images present, debug!→warn!, tests use new method
• src/agent/thread_ops.rs — use message.is_gateway_persisted() instead of inline check
• src/bridge/router.rs — use message.is_gateway_persisted() instead of inline check, tests updated
• tests/e2e/scenarios/test_message_persistence.py — allow Completed state

All unit tests pass, clippy clean, both backends compile.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[serrrfirat]

Remaining ilblackdragon review feedback addressed in 825bd6a

Comment	Status
Critical: attachment divergence (data loss)	Fixed in prior commit (0fb8d90) — skip GATEWAY_PERSISTED_FLAG when images present
v2 coverage	Verified — v2 engine (ConversationManager) persists independently to its own store, no double-persist. V1 dual-write dedup via is_gateway_persisted() works correctly. Added caller-level #[cfg(feature = "libsql")] integration test v2_dual_write_skips_when_gateway_persisted that exercises extracted v2_dual_write_user_message() helper with real DB
Integration tests	Added — test_chat_send_persists_and_flags_message (text-only: DB row + flag set) and test_chat_send_with_images_does_not_set_persisted_flag (images: DB row + flag NOT set) via axum ServiceExt::oneshot()
Playwright E2E	Added — test_user_message_survives_server_restart and test_no_duplicate_user_messages_after_restart using managed_gateway_server fixture
INCOMPLETE_TURN_THRESHOLD hardcoded	Fixed — now configurable via AgentConfig.incomplete_turn_threshold / INCOMPLETE_TURN_THRESHOLD_SECS env var (default 300s). Handlers use build_turns_with_threshold() with config value
dispatch-exempt comment placement	Fixed — moved inline annotations to actual state.session_manager and state.store lines
Stuck turns recovery	Tracked — created #2456
debug! → warn! for persist failure	Fixed in prior commit (0fb8d90)

New tests added

• bridge::router::tests::v2_dual_write_skips_when_gateway_persisted (libsql integration)
• channels::web::server::tests::test_chat_send_persists_and_flags_message (libsql integration)
• channels::web::server::tests::test_chat_send_with_images_does_not_set_persisted_flag (libsql integration)
• channels::web::util::tests::test_build_turns_custom_threshold (unit)
• tests/e2e/scenarios/test_message_persistence.py::test_user_message_survives_server_restart (Playwright)
• tests/e2e/scenarios/test_message_persistence.py::test_no_duplicate_user_messages_after_restart (Playwright)

All unit + integration tests pass, clippy clean, both backends compile.

[serrrfirat]

playwright e2e and good to go

[standardtoaster]

The early persist writes user messages to DB before the safety pipeline (scan_inbound_for_secrets, check_policy) runs. The comment acknowledges this, but the message is also immediately visible via the history API. If the safety pipeline subsequently blocks the message, it's already been served to the client. Is there a plan to mark blocked messages in history, or is the assumption that blocked messages are rare enough that this is acceptable?

[henrypark133]

Closing without merge

After review discussion, we're closing this PR. The core issue: the early-persist approach writes user messages to the DB before the safety pipeline (scan_inbound_for_secrets, check_policy) runs. This means secrets could be persisted to the DB before being caught.

The existing agent-loop persist at thread_ops.rs:659 already handles user message persistence correctly — it runs after safety checks and before the LLM call. Tool calls and assistant responses have their own separate persistence. The timing window between the gateway 202 Accepted and the agent-loop persist is milliseconds for an idle thread, which is acceptable.

Rather than adding complexity to fix the safety ordering (wiring SafetyLayer into GatewayState, or maintaining the GATEWAY_PERSISTED_FLAG dedup machinery across 5 files), the simpler answer is to rely on the existing persist that's already in the right place.

The Playwright happy-path test (test_message_persists_across_page_reload) will be submitted in a separate PR.

@standardtoaster Thank you so much for providing feedback on this!