Drain DeferredBusy messages when the blocking run reaches terminal state

[henrypark133]

Completes the blocked-thread UX arc (#4799 routing, #4811 feedback): messages sent while a run is blocked on a gate are no longer swallowed forever — they drain automatically once the blocking run terminates.

Problem

A run blocked on an approval/auth gate holds the thread's active lock (keeps_active_lock). New inbound messages persist as MessageStatus::DeferredBusy with no run created — and nothing ever resubmits them. Even after the user approves the gate and the run completes, every message sent during the block stays silently dead.

Change

• ironclaw_threads: SessionThreadService::list_deferred_busy_messages — sequence-ordered (oldest first) DeferredBusy user messages for a thread scope; implemented on both backends (in-memory + filesystem) with contract tests on each.
• ironclaw_reborn_composition: DeferredBusyDrainObserver, subscribed to the turn lifecycle bus via additional_required_observers (same pattern as SubagentCompletionObserver, incl. bind_coordinator back-reference). On every terminal event (is_terminal() — the exact inverse of keeps_active_lock, so Completed/Failed/Cancelled/RecoveryRequired all drain) it resubmits the oldest deferred message; one-at-a-time cascade — the resubmitted run's own terminal event drains the next.

Safety properties:

• Identity preserved: resubmission carries the message record's original actor_id; a record without one is left deferred rather than misattributed to the thread owner.
• Idempotent: coordinator idempotency key drain:<message_id> — a duplicate terminal event replays the same run instead of double-submitting; mark_message_submitted flips the record so the product replay path returns AlreadySubmitted.
• Never poisons the terminal path: every failure (list error, invalid refs, busy thread, submit error) logs and leaves the message deferred; next terminal event retries.

Tests

Thread-service contract tests (both impls: ordering, filtering, empty), drain integration (blocked run cancelled → deferred message becomes a submitted run), idempotency (double terminal event → single submission), e2e harness test.

Reviewer note — submission path

The drain submits through TurnCoordinator directly with refs reconstructed from the thread record, rather than the product-workflow inbound replay path (which requires the original product envelope that isn't persisted). Same precedent as trigger_poller_trusted_submit. The product-side replay still converges via the message-status guard (AlreadySubmitted). If we'd rather persist enough of the envelope to route drains through InboundTurnService replay, that's a structural follow-up worth discussing on this PR.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Deferred-message drain observer to resubmit deferred-busy messages; session thread services expose listing of deferred-busy messages.
  • Runtime composition can accept additional required observers for wiring the drain observer.

• Bug Fixes

  • Timeline responses now scrub internal routing binding references before being returned to clients.

• Chores / Tests

  • Persisted binding-ref/idempotency fields for deferred messages; extensive contract and integration tests covering defer/retry/drain behaviors and timeline scrubbing.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds persisted canonical turn binding refs and a ListDeferredBusyMessagesRequest; extends SessionThreadService (defer signature + listing); implements backend listing and marker fast-paths; introduces DeferredBusyDrainObserver wired into runtime; updates defer/retry to persist refs and scrubs timeline output; broad test coverage.

Changes

Deferred-Busy Drain Flow

Layer / File(s)	Summary
Thread contracts and session API crates/ironclaw_threads/src/contract.rs, crates/ironclaw_threads/src/service.rs, crates/ironclaw_threads/src/lib.rs, crates/ironclaw_reborn/src/thread_scope.rs	ThreadMessageRecord gains turn_source_binding_ref and turn_reply_target_binding_ref; ListDeferredBusyMessagesRequest added; SessionThreadService extends mark_message_deferred_busy and adds list_deferred_busy_messages; Arc<> delegation updated; terminal-scope helpers exposed.
Filesystem & InMemory deferred behavior crates/ironclaw_threads/src/filesystem_service.rs, crates/ironclaw_threads/src/in_memory.rs	Backends persist canonical refs and idempotency keys on defer, maintain per-thread presence marker (file or HashSet), implement list_deferred_busy_messages with ascending-sequence ordering, after_sequence (strict >) cursor, limit, and marker tombstone rules; history projection includes new fields.
DeferredBusyDrainObserver core & tests crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs, .../tests.rs	Adds observer that pages deferred messages, validates actor and canonical refs, builds replay SubmitTurnRequest (idempotency fallback), submits via coordinator, marks submitted on Accepted, stops on ThreadBusy, and treats failures as non-fatal. Extensive scenario tests A–L.
Runtime registration & binding crates/ironclaw_reborn/src/runtime.rs, crates/ironclaw_reborn_composition/src/runtime.rs, crates/ironclaw_reborn_composition/src/lib.rs, tests/harness files	DefaultPlannedRuntimeParts gains additional_required_observers; runtime build subscribes them; composition registers DeferredBusyDrainObserver as required observer and binds coordinator after composition; harness/tests set field to empty vec where used.
Workflow defer/retry and facade scrubbing crates/ironclaw_product_workflow/src/inbound_turn.rs, .../reborn_services.rs, crates/ironclaw_product_workflow/src/lib.rs, crates/ironclaw_webui_v2/src/handlers.rs	Submit paths capture canonical source/reply binding refs, persist them on defer via updated mark_message_deferred_busy call, retry with cloned request preserving idempotency, and get_timeline scrubs internal routing fields (including idempotency key) at facade boundary; facade re-exports ThreadMessageRecord.
Thread service deferred-list contract tests crates/ironclaw_threads/tests/*, filesystem & in-memory	Comprehensive contract tests added for empty results, deferred-only filtering, oldest-first ordering, wrong-scope empty, limit behaviors (including 0), strict after_sequence cursor semantics, marker fast-path, immediate marker visibility, tombstone idempotency, and idempotency-key persistence.
Adapters, doubles, fixtures, and webui tests crates/ironclaw_loop_support/*, crates/ironclaw_product_workflow/tests/*, crates/ironclaw_reborn_composition/*, crates/ironclaw_webui_v2/tests/*	Test doubles updated to accept new defer params and add/implement list_deferred_busy_messages (delegate, stub, or panic where appropriate); fixtures updated to include new fields; webui contract verifies scrubbed wire response.

Repository Hygiene

Layer / File(s)	Summary
Ignore local codegraph artifact directory .gitignore	Adds .codegraph/ to ignore rules.

Sequence Diagram (high-level drain flow):

sequenceDiagram
  participant TurnLifecycleBus
  participant DeferredBusyDrainObserver
  participant SessionThreadService
  participant TurnCoordinator

  TurnLifecycleBus->>DeferredBusyDrainObserver: observe_committed_state/event(terminal)
  DeferredBusyDrainObserver->>SessionThreadService: list_deferred_busy_messages(request)
  SessionThreadService-->>DeferredBusyDrainObserver: deferred messages[]
  DeferredBusyDrainObserver->>TurnCoordinator: submit_turn(replayed deferred message)
  TurnCoordinator-->>DeferredBusyDrainObserver: Accepted | ThreadBusy | Error
  DeferredBusyDrainObserver->>SessionThreadService: mark_message_submitted(if accepted)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

• Filesystem backend: per-thread DeferredBusy index to avoid full transcript scans #4833 — overlaps filesystem deferred-list indexing vs marker scan approach.
• Route DeferredBusy drain resubmission through a product_workflow replay entry point #4831 — relates to drain resubmit routing vs product_workflow replay entrypoint.
• Batch drained DeferredBusy messages into a single run #4832 — relates to one-at-a-time drain semantics vs batching.

Suggested reviewers

• serrrfirat
• abbyshekit

No CLAUDE.md/AGENTS.md/.claude/rules invariant violations detected.

"Deferred messages wait, then quietly fly,
Terminal runs whisper: give the next one a try,
Canonical refs travel, then are scrubbed from view,
Tests press the corners; the drain wakes anew."

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	Title uses descriptive action verb and domain language ('Drain DeferredBusy') but lacks conventional commit prefix (type/scope).	Adopt conventional commits format: 'feat(ironclaw_threads): drain DeferredBusy messages when blocking run reaches terminal state' or similar type(scope): pattern per repository guidelines.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	Description covers problem, solution, safety properties, and tests comprehensively, but omits explicit validation checklist items (fmt/clippy/build/test passes) required by template.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to drain and resubmit user messages that were parked with a DeferredBusy status once a blocking run terminates. It adds the list_deferred_busy_messages query to the thread service and implements the DeferredBusyDrainObserver to handle the resubmission. However, the current implementation of the drain observer is vulnerable to head-of-line blocking and queue stalling if the oldest deferred message fails validation. To prevent the queue from permanently stalling, the observer should loop through deferred messages, discard or mark invalid ones as failed, and proceed to the next message instead of returning early.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Head-of-Line Blocking & Queue Stalling Vulnerability

Currently, if the oldest deferred message fails any validation (e.g., resolve_actor_user_id fails, or bindings are invalid), the observer logs a warning and returns Ok(()), leaving the message in the DeferredBusy state.

This causes two severe issues:

1. Head-of-Line Blocking: Since the invalid message remains the oldest message in the database, every subsequent terminal event will pick it up, fail validation, and return early. No other deferred messages on this thread will ever be drained.
2. Queue Stalling: Because no new run is started when validation fails, no future terminal events will fire to trigger another drain, leaving all subsequent valid messages permanently stuck.

Recommended Solution

Instead of only processing the first message and returning early on validation failure, we should loop through the deferred messages:

• If a message fails validation, mark it as submitted with a sentinel error ID (e.g., "failed_validation") to remove it from the DeferredBusy state, and continue the loop to try the next message.
• If a message is successfully submitted, mark it as submitted and break the loop (as the new run will eventually trigger the next drain).
• If submission fails with ThreadBusy, leave it deferred and break the loop.

References

1. In batch processing or polling workers, handle individual record processing errors (such as due-record processing) per record by reporting or logging the failure and continuing to subsequent records, rather than aborting the entire loop.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0f7975956c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Observe terminal state publications too

When a blocked run is resumed and then finishes normally, the terminal transition is published through TurnLifecycleEventBus::publish_state (see crates/ironclaw_turns/src/lifecycle.rs for complete_run/fail_run/runner cancellation), which only calls observers whose observes_state returns true. Because this observer returns false here and observe_committed_state is a no-op, DeferredBusy messages still never drain for the main approval/auth success path; the new drain only runs for coordinator-origin terminal events such as direct cancel_run. Please handle terminal TurnRunStates as well, or otherwise ensure runner-origin terminal events reach the drain.

Useful? React with 👍 / 👎.

[henrypark133]

Code Review (multi-agent)

Intent: Automatically drain DeferredBusy messages after a gate-blocking run reaches terminal state, preserving identity and idempotency.

Stats: 2 findings posted (from 11 raw reviewer findings; stale/off-diff findings excluded; overlapping scan findings deduped) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.

Bugs

1. High Drain rejects valid persisted binding IDs over the turn-ref limit (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:130-156, confidence 80) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:130
   Deferred messages persist raw product binding IDs, while normal product submission bounds/hashes those raw values before constructing turn refs. The drain uses raw constructors directly, so valid deferred messages with long binding IDs can remain stuck forever.

Performance / Concurrency

1. Medium Drain lists the whole thread history to use one message (crates/ironclaw_threads/src/filesystem_service.rs:904-910, confidence 82) — anchor: crates/ironclaw_threads/src/filesystem_service.rs:904
   The observer drains one message at a time, but the filesystem backend materializes and sorts every deferred user message on each terminal event, making backlog recovery repeat full-history scans.

[henrypark133]

High — Drain rejects valid persisted binding IDs over the turn-ref limit.

Deferred messages persist the raw product source_binding_id / reply_target_binding_id strings. The normal product submission path converts those raw values through bounded_source_binding_ref / bounded_reply_target_binding_ref, hashing oversized values before constructing turn refs, but this drain path calls SourceBindingRef::new(raw) and ReplyTargetBindingRef::new(raw) directly. Since those constructors reject values over 256 bytes while product binding material can be much longer, a valid DeferredBusy message can be logged as invalid and left deferred forever instead of draining after the blocking run terminates.

Fix: Reconstruct the coordinator refs with the same bounded binding-ref conversion used by the product submit/replay path, or persist and replay the already-bounded coordinator refs.

[henrypark133]

Medium — Drain lists the whole thread history to use one message.

The observer only submits the oldest deferred message, but this backend loads every message in the thread, filters all DeferredBusy user messages, and sorts the whole filtered list. Because draining is one-at-a-time, a large busy backlog repeats a full history scan for each drained turn, turning recovery into O(total_history * deferred_backlog) work on the terminal lifecycle path.

Fix: Add a bounded service method for the oldest DeferredBusy user message, or make the filesystem implementation walk the existing sequence order and stop once it finds the first eligible record instead of collecting all matches.

Also flagged by: security/Medium

[henrypark133]

Follow-ups from the submission-path reviewer note (trusted-resubmit seam, stale-intent policy, startup drain sweep) are tracked in #4817 — decided not to address in this PR.

[henrypark133]

Review comments addressed in 242cf33..37dbd35:

• Drain never fired on the normal success path (codex P1 — confirmed, critical): the observer only watched committed events, but runner-origin terminal transitions (resume → complete/fail) publish through publish_state. The observer now implements observes_state for terminal statuses and drains from observe_committed_state too; the drain:<message_id> idempotency key makes a duplicate event+state pair a replay, not a double-submit. Regression test drives the real path: claim → complete_run via LifecyclePublishingTurnStateStore → deferred message submitted (deferred_message_submitted_after_blocking_run_completes_via_publish_state).
• Oversized binding ids bricked the drain (henrypark133 — confirmed, High): the drain now reuses the product path's bounded_source_binding_ref / bounded_reply_target_binding_ref (exposed as pub, not duplicated), so refs converge with what the original submission produced. Test: 300-byte binding id round-trips defer → drain.
• Head-of-line blocking / queue stall (gemini — confirmed, High, modified fix): the drain iterates oldest-first and skips validation-failed messages instead of stopping. Deliberately NOT taking the suggested sentinel-status mutation — falsifying a message's status conflicts with the data-retention rule; failed records stay DeferredBusy, are re-examined and re-skipped each drain. Bounded window (limit 8) caps the rescan cost. Test: invalid oldest + valid second → second drains.
• Full history scan per drain (henrypark133): ListDeferredBusyMessagesRequest gains limit, applied after sequence sort in both backends; drain reads at most 8. Contract tests on both impls.

clippy zero warnings; 877 tests pass (pre-existing local_dev_runtime_* parallel flake unchanged, also fails on main).

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs (1)

451-456: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't drop mission_id when rebuilding ThreadScope.

Both helpers hard-code mission_id: None, so deferred messages parked on mission-scoped threads won't be found or marked submitted during drain. The filesystem backend keys thread storage by mission_id, and the thread services compare full scope, so this silently turns the new drain into a no-op for that class of threads. Please carry mission scope through the lifecycle surface, or recover the canonical ThreadScope before calling the thread service.

As per coding guidelines, "Preserve tenant/user/agent/project/mission/thread scope on authority, state, memory, process, network, outbound, resource, and event records."

Also applies to: 471-476

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs` around lines
451 - 456, The ThreadScope reconstruction is dropping mission context (setting
mission_id: None) causing mission-scoped deferred messages to be missed; update
the ThreadScope creation in the code paths around ThreadScope { tenant_id:
event.scope.tenant_id.clone(), agent_id, project_id:
event.scope.project_id.clone(), owner_user_id, mission_id: None, ... } to carry
through the original mission id (e.g. use event.scope.mission_id.clone() or
recover the canonical ThreadScope before calling the thread service) so the
filesystem backend and thread service comparisons see the full scope; apply the
same fix to the other occurrence noted (around the 471-476 region).

Source: Coding guidelines

🧹 Nitpick comments (1)

crates/ironclaw_threads/tests/session_thread_contract.rs (1)

2729-2745: ⚡ Quick win

Assert the exact oldest deferred rows here.

This only proves "two increasing sequences," not "the oldest two after sorting." A truncate-before-sort regression could still pass. Please assert the returned sequences/content are exactly alpha then beta.

Suggested assertion tighten-up

-    assert_eq!(result.len(), 2, "limit=2 must cap at 2");
-    // Must be oldest first.
-    let seqs: Vec<_> = result.iter().map(|m| m.sequence).collect();
-    assert!(
-        seqs.windows(2).all(|w| w[0] < w[1]),
-        "limited results not in ascending sequence order: {seqs:?}"
-    );
+    assert_eq!(
+        result.iter().map(|m| m.sequence).collect::<Vec<_>>(),
+        vec![1, 2],
+        "limit=2 must keep the oldest deferred rows",
+    );
+    assert_eq!(
+        result.iter().map(|m| m.content.as_deref()).collect::<Vec<_>>(),
+        vec![Some("alpha"), Some("beta")],
+    );

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_threads/tests/session_thread_contract.rs` around lines 2729 -
2745, The test currently only checks two ascending sequences from the
list_deferred_busy_messages call; instead assert that the returned rows exactly
match the expected oldest two (alpha then beta). After calling
service.list_deferred_busy_messages(ListDeferredBusyMessagesRequest { ... }) and
unwrapping into result, replace the loose seqs check with an exact equality
check that result[0] corresponds to alpha and result[1] corresponds to beta
(compare their sequence field or full message identity used elsewhere in the
test), e.g. assert that result.len()==2 and that result[0].sequence ==
alpha.sequence and result[1].sequence == beta.sequence (or compare message
IDs/content) so the test fails on any truncate-before-sort regression.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs`:
- Around line 451-456: The ThreadScope reconstruction is dropping mission
context (setting mission_id: None) causing mission-scoped deferred messages to
be missed; update the ThreadScope creation in the code paths around ThreadScope
{ tenant_id: event.scope.tenant_id.clone(), agent_id, project_id:
event.scope.project_id.clone(), owner_user_id, mission_id: None, ... } to carry
through the original mission id (e.g. use event.scope.mission_id.clone() or
recover the canonical ThreadScope before calling the thread service) so the
filesystem backend and thread service comparisons see the full scope; apply the
same fix to the other occurrence noted (around the 471-476 region).

---

Nitpick comments:
In `@crates/ironclaw_threads/tests/session_thread_contract.rs`:
- Around line 2729-2745: The test currently only checks two ascending sequences
from the list_deferred_busy_messages call; instead assert that the returned rows
exactly match the expected oldest two (alpha then beta). After calling
service.list_deferred_busy_messages(ListDeferredBusyMessagesRequest { ... }) and
unwrapping into result, replace the loose seqs check with an exact equality
check that result[0] corresponds to alpha and result[1] corresponds to beta
(compare their sequence field or full message identity used elsewhere in the
test), e.g. assert that result.len()==2 and that result[0].sequence ==
alpha.sequence and result[1].sequence == beta.sequence (or compare message
IDs/content) so the test fails on any truncate-before-sort regression.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6cb5e327-cc9d-498c-910b-d798bd4c4763

📥 Commits

Reviewing files that changed from the base of the PR and between 0f79759 and 37dbd35.

📒 Files selected for processing (8)

• crates/ironclaw_product_workflow/src/binding_ref.rs
• crates/ironclaw_product_workflow/src/lib.rs
• crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs
• crates/ironclaw_threads/src/contract.rs
• crates/ironclaw_threads/src/filesystem_service.rs
• crates/ironclaw_threads/src/in_memory.rs
• crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs
• crates/ironclaw_threads/tests/session_thread_contract.rs

✅ Files skipped from review due to trivial changes (1)

• crates/ironclaw_product_workflow/src/lib.rs

🚧 Files skipped from review as they are similar to previous changes (3)

• crates/ironclaw_threads/src/contract.rs
• crates/ironclaw_threads/src/in_memory.rs
• crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs

[henrypark133]

Code Review (multi-agent)

Intent: Automatically drain DeferredBusy messages after a blocking gate run reaches terminal state.
Stats: 9 findings (from 13 raw, 9 after filtering/dedup) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 2. Refreshed against head 72ad2412abe41ef9f015b29bc35b67afc61f015e.

bugs

1. High Drain rewrites non-product binding refs with product prefixes (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:150-185, confidence 85) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:150
   The drain always reconstructs deferred messages with src: and reply: prefixes. That only matches the product inbound path; WebUI and runtime/task submissions persist different raw binding ids that are rebuilt with source-specific prefixes such as webui-src/webui-reply or already-stable runtime refs. A DeferredBusy WebUI/runtime message can therefore resume with different binding refs than the original submission path would have used, risking misrouted follow-up interactions.
   Also flagged by: maintainability/Medium, approach/Medium.
2. Medium Invalid deferred messages can permanently hide valid later ones (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:97-100, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:100
   Each drain only loads the oldest 8 DeferredBusy messages, while validation failures leave those records unchanged. If the first 8 deferred records are invalid, the loop skips them all, submits nothing, and the next terminal event fetches the same 8 records again, so any valid message after them is never reached.

performance

1. Medium Drain limit is applied after a full transcript scan (crates/ironclaw_threads/src/filesystem_service.rs:904-912, confidence 88) — anchor: crates/ironclaw_threads/src/filesystem_service.rs:904
   list_deferred_busy_messages accepts limit, and the drain calls it with Some(8), but the filesystem backend first calls list_thread_messages, which lists, reads, deserializes, collects, and sorts every message JSON in the thread before truncating. Normal terminal events now add O(total thread messages) filesystem I/O even when there are zero deferred messages; draining N queued messages cascades into N full transcript scans.

tests

1. High Production drain wiring lacks a caller-level test (crates/ironclaw_reborn_composition/src/runtime.rs:2149-2226, confidence 100) — anchor: crates/ironclaw_reborn_composition/src/runtime.rs:2215
   The new DeferredBusyDrainObserver is wired into build_reborn_runtime, but the existing drain tests manually construct and subscribe the observer. The adjacent e2e file is only a no-op redirect, so a regression that drops additional_required_observers or fails bind_coordinator in the production runtime path would not be caught.
2. Medium Cascade semantics are not tested with multiple valid messages (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:296-298, confidence 100) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:296
   The drain stops after the first successful submit to enforce one-at-a-time cascade behavior, but the tests only cover a single valid deferred message or an invalid head followed by one valid message. There is no test with two valid DeferredBusy messages proving the first terminal event submits only the oldest and the next terminal event drains the next.
3. Medium List failure path is untested (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:95-112, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:105
   The observer catches list_deferred_busy_messages failures and returns Ok so terminal run publication is not poisoned, but no adjacent drain test exercises a SessionThreadService list error. The PR explicitly promises that list errors leave messages deferred and do not fail the terminal path.
4. Medium Re-busy drain contention branch is untested (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:300-310, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:300
   The drain has a ThreadBusy branch for the case where another run reacquires the thread lock before the deferred message can be resubmitted, but existing tests only exercise ThreadBusy while initially marking messages deferred. No test covers the drain attempt itself hitting ThreadBusy and leaving the message DeferredBusy for retry on the next terminal event.

local-patterns

1. Low Module docs use item docs instead of inner docs (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:1-17, confidence 75) (no diff position - body only) — anchor: crates/ironclaw_reborn_composition/src/runtime.rs:1
   The new file starts with /// comments before a use, so the overview documents the import item instead of the module. Nearby composition modules put file-level overviews in //! inner docs, which keeps rustdoc and source navigation attached to the module itself.
2. Nit Tracing dependency comment is stale against Cargo (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:497-500, confidence 50) (no diff position - body only) — anchor: crates/ironclaw_reborn_composition/Cargo.toml:163
   The comment says tracing is available transitively via other crates, but this crate already declares tracing = "0.1" directly. That makes the dependency explanation misleading during future cleanup and is unnecessary for a normal import.

[henrypark133]

High — Drain rewrites non-product binding refs with product prefixes.

The drain always reconstructs deferred messages with src: and reply: prefixes. That only matches the product inbound path; WebUI and runtime/task submissions persist different raw binding ids that are rebuilt with source-specific prefixes such as webui-src/webui-reply or already-stable runtime refs. A DeferredBusy WebUI/runtime message can therefore resume with different binding refs than the original submission path would have used, risking misrouted follow-up interactions.

Fix: Persist/replay the canonical refs or move the resubmission handoff behind the product-workflow owner so each source reconstructs refs with its original prefix/path.

Also flagged by: maintainability/Medium, approach/Medium

[henrypark133]

Medium — Invalid deferred messages can permanently hide valid later ones.

Each drain only loads the oldest 8 DeferredBusy messages, while validation failures leave those records unchanged. If the first 8 deferred records are invalid, the loop skips them all, submits nothing, and the next terminal event fetches the same 8 records again, so any valid message after them is never reached.

Fix: Either drain without this fixed prefix limit, paginate past skipped invalid records, or mark invalid records as non-drainable so later valid messages can progress.

[henrypark133]

Medium — Drain limit is applied after a full transcript scan.

list_deferred_busy_messages accepts limit, and the drain calls it with Some(8), but the filesystem backend first calls list_thread_messages, which lists, reads, deserializes, collects, and sorts every message JSON in the thread before truncating. Normal terminal events now add O(total thread messages) filesystem I/O even when there are zero deferred messages; draining N queued messages cascades into N full transcript scans.

Fix: Maintain a per-thread deferred-busy index/queue updated by mark_message_deferred_busy and mark_message_submitted, then read only the first limit entries instead of materializing the whole transcript.

[henrypark133]

High — Production drain wiring lacks a caller-level test.

The new DeferredBusyDrainObserver is wired into build_reborn_runtime, but the existing drain tests manually construct and subscribe the observer. The adjacent e2e file is only a no-op redirect, so a regression that drops additional_required_observers or fails bind_coordinator in the production runtime path would not be caught.

Fix: tests::deferred_busy_drain_e2e::runtime_drains_deferred_busy_after_gate_terminal covering product/runtime inbound DeferredBusy message, blocking run terminal completion, and resubmitted message status

[henrypark133]

Medium — Cascade semantics are not tested with multiple valid messages.

The drain stops after the first successful submit to enforce one-at-a-time cascade behavior, but the tests only cover a single valid deferred message or an invalid head followed by one valid message. There is no test with two valid DeferredBusy messages proving the first terminal event submits only the oldest and the next terminal event drains the next.

Fix: tests::deferred_busy_drain::drain_submits_one_valid_message_per_terminal_event_cascade covering two valid DeferredBusy messages, oldest-first submission, and second-message drain after the first drained run terminates

[henrypark133]

Medium — List failure path is untested.

The observer catches list_deferred_busy_messages failures and returns Ok so terminal run publication is not poisoned, but no adjacent drain test exercises a SessionThreadService list error. The PR explicitly promises that list errors leave messages deferred and do not fail the terminal path.

Fix: tests::deferred_busy_drain::drain_list_error_returns_ok_and_leaves_deferred covering a failing list_deferred_busy_messages service and observe_committed_event/observe_committed_state returning Ok

[henrypark133]

Medium — Re-busy drain contention branch is untested.

The drain has a ThreadBusy branch for the case where another run reacquires the thread lock before the deferred message can be resubmitted, but existing tests only exercise ThreadBusy while initially marking messages deferred. No test covers the drain attempt itself hitting ThreadBusy and leaving the message DeferredBusy for retry on the next terminal event.

Fix: tests::deferred_busy_drain::drain_leaves_deferred_when_resubmit_hits_thread_busy covering coordinator submit_turn returning ThreadBusy during drain

[henrypark133]

Second-round comments addressed in f45a562..6757cc4 (the 14:4x batch re-reported pre-push findings already fixed in 37dbd35; the 16:57 batch was fresh):

• Drain rewrote non-product binding refs with product prefixes (High): the drain no longer reconstructs refs at all. The canonical, already-bounded turn refs are now persisted on the message record at defer time (turn_source_binding_ref/turn_reply_target_binding_ref, #[serde(default)] — old records rehydrate as None) by every DeferredBusy-marking path, and replayed verbatim. Records without canonical refs (legacy shape) are skipped with a warning — acceptable pre-merge. This kills the per-source prefix divergence class (src:/webui-src:/runtime refs) rather than special-casing it.
• Invalid heads could hide valid messages behind the window (Medium): the drain now paginates — after_sequence on the list request; fully-skipped windows advance to the next, bounded by a documented total-examined cap. No status mutation of skipped records (retention rule), consistent with the earlier decision against sentinel statuses.
• Full transcript scan per terminal event (Medium): presence-marker fast path. The filesystem backend writes a per-thread marker before flipping a message to DeferredBusy (conservative ordering: stale marker = one wasted scan; missing marker with deferred messages can't happen); list_deferred_busy_messages returns empty without scanning when the marker is absent and clears it when a scan finds nothing. Zero-deferred terminal events — the common case — no longer touch the transcript.
• Production wiring untested (High): runtime_drains_deferred_busy_after_gate_terminal drives the real runtime build path — failing if additional_required_observers wiring or bind_coordinator regresses.
• Missing tests: drain_submits_one_valid_message_per_terminal_event_cascade, drain_list_error_returns_ok_and_leaves_deferred, drain_leaves_deferred_when_resubmit_hits_thread_busy added.

Note for #4811 coupling: with the drain real, #4811's deferred-busy generic copy can be restored to queued-wording once both merge.

threads + composition suites green, clippy zero warnings, workspace check clean.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/ironclaw_threads/src/in_memory.rs (1)

663-685: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove the deferred-busy marker when a thread is deleted.

mark_message_deferred_busy inserts (ThreadScope, ThreadId) into deferred_threads, but delete_thread only removes the thread record and idempotency entries. Any thread that ever hit DeferredBusy leaves a dead marker behind, so create/delete churn can grow this HashSet without bound.

As per coding guidelines, "User-controlled inputs must not grow unbounded. Apply hard size limits (entries + total bytes) with documented eviction policies on interners, caches, and accumulators."

Suggested cleanup

         state.threads.remove(thread_id);
+        state
+            .deferred_threads
+            .remove(&(scope.clone(), thread_id.clone()));
         state
             .inbound_idempotency
             .retain(|_, record| &record.thread_id != thread_id);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_threads/src/in_memory.rs` around lines 663 - 685,
delete_thread currently removes the thread record and inbound_idempotency
entries but leaves deferred_threads entries behind; update the delete_thread
implementation so after removing the thread from state.threads it also removes
any deferred-busy marker for that thread by removing (scope, thread_id) from
state.deferred_threads (the same HashSet that mark_message_deferred_busy writes
to); do this while holding the same state lock so no races occur and ensure you
reference state.deferred_threads, delete_thread, and mark_message_deferred_busy
when making the change.

Source: Coding guidelines

🧹 Nitpick comments (2)

crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs (1)

1114-1394: ⚡ Quick win

Add a contract case for after_sequence.

The new deferred-busy cursor is part of the service contract, but this suite only exercises after_sequence: None. A backend can ignore the pagination/skip semantics and still pass every test here. Seed multiple deferred messages and assert that after_sequence skips the older window before ordering and limit are applied.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs` around
lines 1114 - 1394, Add a new test (e.g.
filesystem_list_deferred_busy_messages_respects_after_sequence) that seeds
multiple deferred-busy messages via
FilesystemSessionThreadService::accept_inbound_message and
mark_message_deferred_busy, then call list_deferred_busy_messages with
ListDeferredBusyMessagesRequest specifying after_sequence: Some(x) (pick x to
skip the oldest N messages) and optional limit, and assert that every returned
message has message.sequence > x, that results are ordered ascending by
sequence, and that limit is applied; use the existing helpers
scoped_threads_fs_at, scope(...), and the same service methods
(accept_inbound_message, mark_message_deferred_busy,
list_deferred_busy_messages) and assert MessageStatus::DeferredBusy and
MessageKind where appropriate.

crates/ironclaw_threads/tests/session_thread_contract.rs (1)

2518-2902: ⚡ Quick win

Add explicit contract coverage for after_sequence pagination semantics.

The new section exercises limit/ordering well, but every request uses after_sequence: None (e.g., Line 2537, Line 2597, Line 2646). Add one test that sets after_sequence and verifies strict exclusion of older/equal sequences plus correct ordering/limit behavior in the returned window.

Suggested test shape

+#[tokio::test]
+async fn list_deferred_busy_messages_after_sequence_filters_and_orders() {
+    let service = InMemorySessionThreadService::default();
+    let thread = service
+        .ensure_thread(EnsureThreadRequest {
+            scope: scope("after-seq"),
+            thread_id: None,
+            created_by_actor_id: "actor-a".into(),
+            title: None,
+            metadata_json: None,
+        })
+        .await
+        .unwrap();
+
+    for label in ["m1", "m2", "m3", "m4"] {
+        let msg = service
+            .accept_inbound_message(AcceptInboundMessageRequest {
+                scope: scope("after-seq"),
+                thread_id: thread.thread_id.clone(),
+                actor_id: "actor-a".into(),
+                source_binding_id: None,
+                reply_target_binding_id: None,
+                external_event_id: None,
+                content: user_message(label),
+            })
+            .await
+            .unwrap();
+        service
+            .mark_message_deferred_busy(
+                &scope("after-seq"),
+                &thread.thread_id,
+                msg.message_id,
+                None,
+                None,
+            )
+            .await
+            .unwrap();
+    }
+
+    let page = service
+        .list_deferred_busy_messages(ListDeferredBusyMessagesRequest {
+            scope: scope("after-seq"),
+            thread_id: thread.thread_id,
+            limit: Some(2),
+            after_sequence: Some(2),
+        })
+        .await
+        .unwrap();
+
+    let seqs: Vec<_> = page.iter().map(|m| m.sequence).collect();
+    assert_eq!(seqs, vec![3, 4]);
+}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_threads/tests/session_thread_contract.rs` around lines 2518 -
2902, Add a new tokio test (e.g.,
list_deferred_busy_messages_after_sequence_excludes_and_limits) that creates an
InMemorySessionThreadService and thread, accepts and defers several messages via
accept_inbound_message and mark_message_deferred_busy, captures their sequence
numbers, then calls list_deferred_busy_messages with
ListDeferredBusyMessagesRequest using after_sequence set to one of the captured
sequences and asserts the returned window excludes messages with sequence <=
after_sequence, is ordered strictly ascending by message.sequence, and respects
limit (exercise with limit=None and limit=Some(1)); reference
accept_inbound_message, mark_message_deferred_busy, list_deferred_busy_messages,
ListDeferredBusyMessagesRequest and the message.sequence field to locate the
logic to test.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs`:
- Around line 107-147: The per-message loop in DeferredBusyDrainObserver can
exceed DRAIN_TOTAL_CAP because total_examined is only checked between page
fetches; inside the for message in window loop you must enforce the cap by
checking total_examined >= DRAIN_TOTAL_CAP before processing each message and
breaking out of both the per-message loop and outer loop (returning Ok(() ) or
jumping to the same exit path) when the cap is hit. Update the logic around
total_examined in the for message in window iteration to stop processing further
messages and avoid inspecting beyond the hard cap, ensuring any necessary
bookkeeping (e.g., window_last_sequence/after_sequence) matches the existing
exit behavior.

In `@crates/ironclaw_threads/src/contract.rs`:
- Around line 130-141: ThreadMessageRecord currently exposes internal
replay-only fields turn_source_binding_ref and turn_reply_target_binding_ref
which will leak routing metadata to public reads; remove or hide them from the
public DTO by moving these fields into a backend-only persisted shape (e.g., a
separate InternalThreadMessageRecord / StoredThreadMessageRecord) or marking
them to be skipped for public serialization and ensure public read paths return
the scrubbed ThreadMessageRecord; update any code that currently copies those
fields into ThreadMessageRecord (search for usages of turn_source_binding_ref
and turn_reply_target_binding_ref) to instead record them only in the
backend-only struct and/or populate them when the drain reads from the
backend-only representation.

---

Outside diff comments:
In `@crates/ironclaw_threads/src/in_memory.rs`:
- Around line 663-685: delete_thread currently removes the thread record and
inbound_idempotency entries but leaves deferred_threads entries behind; update
the delete_thread implementation so after removing the thread from state.threads
it also removes any deferred-busy marker for that thread by removing (scope,
thread_id) from state.deferred_threads (the same HashSet that
mark_message_deferred_busy writes to); do this while holding the same state lock
so no races occur and ensure you reference state.deferred_threads,
delete_thread, and mark_message_deferred_busy when making the change.

---

Nitpick comments:
In `@crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs`:
- Around line 1114-1394: Add a new test (e.g.
filesystem_list_deferred_busy_messages_respects_after_sequence) that seeds
multiple deferred-busy messages via
FilesystemSessionThreadService::accept_inbound_message and
mark_message_deferred_busy, then call list_deferred_busy_messages with
ListDeferredBusyMessagesRequest specifying after_sequence: Some(x) (pick x to
skip the oldest N messages) and optional limit, and assert that every returned
message has message.sequence > x, that results are ordered ascending by
sequence, and that limit is applied; use the existing helpers
scoped_threads_fs_at, scope(...), and the same service methods
(accept_inbound_message, mark_message_deferred_busy,
list_deferred_busy_messages) and assert MessageStatus::DeferredBusy and
MessageKind where appropriate.

In `@crates/ironclaw_threads/tests/session_thread_contract.rs`:
- Around line 2518-2902: Add a new tokio test (e.g.,
list_deferred_busy_messages_after_sequence_excludes_and_limits) that creates an
InMemorySessionThreadService and thread, accepts and defers several messages via
accept_inbound_message and mark_message_deferred_busy, captures their sequence
numbers, then calls list_deferred_busy_messages with
ListDeferredBusyMessagesRequest using after_sequence set to one of the captured
sequences and asserts the returned window excludes messages with sequence <=
after_sequence, is ordered strictly ascending by message.sequence, and respects
limit (exercise with limit=None and limit=Some(1)); reference
accept_inbound_message, mark_message_deferred_busy, list_deferred_busy_messages,
ListDeferredBusyMessagesRequest and the message.sequence field to locate the
logic to test.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2b6749f9-abbf-44ae-b953-9d9acbef5338

📥 Commits

Reviewing files that changed from the base of the PR and between 72ad241 and 6757cc4.

📒 Files selected for processing (17)

• crates/ironclaw_loop_support/src/compaction_task.rs
• crates/ironclaw_loop_support/src/subagent_spawn_port/tests.rs
• crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs
• crates/ironclaw_product_workflow/src/inbound_turn.rs
• crates/ironclaw_product_workflow/src/reborn_services.rs
• crates/ironclaw_product_workflow/tests/reborn_services_contract.rs
• crates/ironclaw_reborn/src/loop_exit_applier/tests/mod.rs
• crates/ironclaw_reborn/src/subagent/completion_observer.rs
• crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rs
• crates/ironclaw_threads/src/contract.rs
• crates/ironclaw_threads/src/filesystem_service.rs
• crates/ironclaw_threads/src/in_memory.rs
• crates/ironclaw_threads/src/service.rs
• crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs
• crates/ironclaw_threads/tests/session_thread_contract.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• crates/ironclaw_product_workflow/tests/reborn_services_contract.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rs
• crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs

[henrypark133]

Code Review (multi-agent)

Intent: Automatically drain DeferredBusy messages after a gate-blocked run reaches terminal state so blocked-thread messages are not swallowed.

Stats: 11 findings (from 13 raw, 11 after dedup/filter) across 6 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1.

Performance / Concurrency

1. High Marker can be deleted before the deferred write commits (crates/ironclaw_threads/src/filesystem_service.rs:891-899, confidence 88) - anchor: crates/ironclaw_threads/src/filesystem_service.rs:891
   The filesystem backend writes the DeferredBusy presence marker before the message status is flipped, then deletes that marker on an empty scan. A terminal event in that await gap can leave a DeferredBusy message markerless and permanently skipped by future drains.

Bugs

1. Medium Drained WebUI messages bypass skill activation recording (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:270-285, confidence 75) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:285
   WebUI records activation before coordinator submit, clears it on ThreadBusy, and the drain later calls TurnCoordinator directly. Deferred slash/mention skill messages can replay without the activation side effect they get on the immediate path.

Tests

1. Medium Product inbound defer path does not assert persisted refs (crates/ironclaw_product_workflow/src/inbound_turn.rs:650-658, confidence 75) - anchor: crates/ironclaw_product_workflow/src/inbound_turn.rs:651
   The product ThreadBusy path now persists canonical refs, but the busy-path contract can still pass if those refs are dropped or swapped.
2. Medium WebUI defer path does not assert persisted refs (crates/ironclaw_product_workflow/src/reborn_services.rs:1764-1774, confidence 75) - anchor: crates/ironclaw_product_workflow/src/reborn_services.rs:1766
   The WebUI ThreadBusy path computes and passes canonical refs, but tests do not read the stored DeferredBusy message fields.
3. Medium In-memory deferred listing lacks after_sequence coverage (crates/ironclaw_threads/src/in_memory.rs:284-292, confidence 100) - anchor: crates/ironclaw_threads/src/in_memory.rs:284
   The drain depends on after_sequence paging past invalid windows, but in-memory contract tests only exercise None.
4. Medium Filesystem deferred listing lacks after_sequence coverage (crates/ironclaw_threads/src/filesystem_service.rs:929-938, confidence 100) - anchor: crates/ironclaw_threads/src/filesystem_service.rs:929
   The filesystem backend implements the same after_sequence paging filter, but its contract tests only exercise None.
5. Medium Drain identity skip path is not covered (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:151-160, confidence 75) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:151
   The PR promises actor-less records remain deferred instead of being submitted as the owner, but there is no test covering that branch.

Conventions

1. Medium Remove unused public binding-ref re-exports (crates/ironclaw_product_workflow/src/lib.rs:84-86, confidence 100) - anchor: CLAUDE.md:20
   The drain reconstructs persisted canonical refs directly and does not consume these helpers, so this widens product_workflow's public API without a downstream consumer. Also flagged by maintainability/Low.

Local Patterns

1. Low Comment bakes branch-temporary context into runtime code (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:164-166, confidence 75) - anchor: crates/ironclaw_reborn/src/subagent/completion_observer.rs:83
   The comment references an unmerged branch state instead of a durable invariant.
2. Nit Tracing macro import is buried outside the import block (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:527-530, confidence 75) - anchor: crates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rs:1
   The tracing macro import is late in the file, unlike sibling composition modules.

Maintainability

1. Low Drain paging and request construction are fused into one branch-heavy loop (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:151-285, confidence 50) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:151 (no diff position - body only)
   The loop mixes paging, skip policy, request construction, submission, and reconciliation. Consider extracting a per-message projection helper if this area changes further.

[henrypark133]

High - Marker can be deleted before the deferred write commits.

The filesystem backend writes the DeferredBusy presence marker before the message status is flipped, then list_deferred_busy_messages deletes that marker whenever a scan finds no DeferredBusy rows. If the blocking run terminates between the marker write and apply_message_update, the drain can observe the marker, scan before the status change, delete the marker, and then the later status update leaves a DeferredBusy message that future terminal drains skip via the marker fast path.

Fix: Do not make marker deletion race the status write. Write the marker only after the DeferredBusy status/ref fields commit, make the marker update atomic with the message update, or avoid deleting the marker on an empty scan when a defer write may still be in flight.

[henrypark133]

Fixed in 02f1f18 — see crates/ironclaw_threads/src/filesystem_service.rs.

[henrypark133]

Medium - Drained WebUI messages bypass skill activation recording.

The WebUI submit path records skill activation before submitting to the coordinator, but clears that pending activation on ThreadBusy before marking the message DeferredBusy. The drain later submits the persisted message directly through TurnCoordinator, so a deferred slash/mention skill message is replayed without the skill-activation side effect that the same message gets on the immediate submit path.

Fix: Replay drained WebUI/product messages through a product-aware hook that records activation again, or persist enough activation metadata at defer time and restore it before submit_turn.

[henrypark133]

Medium - Product inbound defer path does not assert persisted refs.

The ThreadBusy branch now persists canonical source/reply binding refs for later drain replay, but the adjacent busy-thread contract only checks DeferredBusy status. If either argument is dropped or swapped, the drain skips the message and the test still passes.

Fix: Add caller-level coverage for the ThreadBusy path that reads the stored message and asserts turn_source_binding_ref and turn_reply_target_binding_ref are persisted with the expected canonical values.

[henrypark133]

Fixed in f2600cc — see crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs.

[henrypark133]

Medium - WebUI defer path does not assert persisted refs.

The WebUI ThreadBusy branch computes canonical webui refs and passes them into mark_message_deferred_busy, but the busy-path tests do not read the stored message fields. A regression that omits these refs would leave WebUI DeferredBusy messages undrainable without failing tests.

Fix: Add a reborn_services busy-path test that verifies DeferredBusy messages persist the canonical webui source and reply refs.

[henrypark133]

Fixed in f2600cc — see crates/ironclaw_product_workflow/tests/reborn_services_contract.rs.

[henrypark133]

Medium - In-memory deferred listing lacks after_sequence coverage.

list_deferred_busy_messages filters by sequence after an already-examined window, but the in-memory contract tests only pass after_sequence: None. If this filter regresses, the drain can keep seeing the same invalid window and never reach later valid deferred messages.

Fix: Add contract coverage where after_sequence is Some(seq) and only later DeferredBusy records are returned.

[henrypark133]

Fixed in f2600cc — see crates/ironclaw_threads/tests/session_thread_contract.rs.

[henrypark133]

Medium - Filesystem deferred listing lacks after_sequence coverage.

The filesystem backend implements the same after_sequence paging filter used by the drain, but its contract tests only exercise None. A backend-specific mistake here would break drain pagination for persisted threads.

Fix: Add filesystem contract coverage where after_sequence is Some(seq) and only later DeferredBusy records are returned.

[henrypark133]

Fixed in f2600cc — see crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs.

[henrypark133]

Medium - Drain identity skip path is not covered.

The PR explicitly promises that a deferred record without actor_id remains deferred rather than being resubmitted as the thread owner, but the drain tests only cover missing canonical refs and successful actor resolution. The actor-missing branch can regress without a failing test.

Fix: Add a drain test that creates a DeferredBusy message with actor_id None, fires the terminal observer, and asserts no submission occurs and the message remains DeferredBusy.

[henrypark133]

Fixed in f2600cc — see crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs.

[henrypark133]

Medium - Remove unused public binding-ref re-exports.

This adds pub re-exports for binding-ref helpers, but the drain observer reconstructs persisted canonical refs directly and does not consume these helpers. That broadens product_workflow's public API without a downstream consumer in this diff.

Fix: Keep the binding-ref helpers crate-private and remove the lib.rs re-export unless the drain actually routes through the bounded conversion helpers.

Also flagged by: maintainability/Low

[henrypark133]

Fixed in f2600cc — see crates/ironclaw_product_workflow/src/lib.rs.

[henrypark133]

Low - Comment bakes branch-temporary context into runtime code.

The comment explains missing binding refs as a legacy unmerged-branch condition. After this lands, that wording becomes stale review context rather than a durable invariant.

Fix: Reword the comment to a stable rule, such as records persisted before canonical binding refs existed have None and are skipped.

[henrypark133]

Fixed in f2600cc — see crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs.

[henrypark133]

Nit - Tracing macro import is buried outside the import block.

This new module imports tracing macros after helper functions with a dependency note, while sibling composition modules either keep imports at the top or call tracing::debug!/warn! directly.

Fix: Move the tracing import into the top use block, or use fully qualified tracing::debug! and tracing::warn! like sibling composition files.

[henrypark133]

Fixed in f2600cc — see crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs.

[henrypark133]

Follow-ups filed from review: #4831 (route drain resubmission through product_workflow replay owner — also resolves the skill-activation bypass), #4832 (batch drained messages into one run, depends on #4831), #4833 (per-thread deferred index for filesystem scan cost).

[henrypark133]

Code Review (multi-agent)

Intent: Automatically drain and resubmit DeferredBusy messages once the blocking run reaches a terminal state.
Stats: 11 findings (from 14 raw, 11 after dedup) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0

security

1. Medium Deferred drain limit still scans the entire transcript (crates/ironclaw_threads/src/filesystem_service.rs:938-950, confidence 75) - anchor: crates/ironclaw_threads/src/filesystem_service.rs:938
   The new drain path passes limit=8, but the filesystem backend first calls list_thread_messages(), which lists, reads, deserializes, and sorts every message in the thread before filtering DeferredBusy rows and truncating. A large thread can therefore turn each terminal event that triggers the drain into an O(thread size) filesystem scan despite the small drain cap.
   Also flagged by: performance/Medium

bugs

1. Medium Marker write failure can hide a deferred message forever (crates/ironclaw_threads/src/filesystem_service.rs:908-916, confidence 75) - anchor: crates/ironclaw_threads/src/filesystem_service.rs:908
   The filesystem backend first persists the message as DeferredBusy and only then writes deferred-busy.marker. If that marker write fails, the caller sees a transient error but the message is already durable as DeferredBusy without the marker. Future drain attempts take the marker-absent fast path and return an empty list, so the message is never retried unless the user happens to resubmit/replay it.
2. Medium Invalid deferred records can permanently starve later messages (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:109-115, confidence 75) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:109
   Each drain invocation starts at after_sequence = None and stops after examining 64 records. Because skipped invalid or legacy records are not mutated and no cursor is persisted across invocations, a thread with 64 invalid DeferredBusy rows before a valid one will re-scan the same 64 rows on every terminal event and never reach the valid message behind them.

tests

1. Medium Drain submit-error branch is untested (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:340-347, confidence 100) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:340
   The drain's failure contract says coordinator submit failures must log, return Ok, and leave the message deferred, but the adjacent tests only cover success, list errors, invalid records, and ThreadBusy. A non-ThreadBusy submit error could regress into poisoning the terminal path without a test catching it.
2. Medium Post-submit persistence failure is untested (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:307-323, confidence 100) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:307
   After the coordinator accepts a drained turn, mark_message_submitted can fail and the observer is supposed to warn without failing the terminal transition. No adjacent drain test injects that persistence error, so the no-poisoning guarantee is uncovered for this branch.

conventions

1. Medium Keep internal drain refs out of facade timeline DTOs (crates/ironclaw_threads/src/contract.rs:130-141, confidence 75) - anchor: crates/ironclaw_product_workflow/AGENTS.md:18
   The new serde-visible turn_source_binding_ref and turn_reply_target_binding_ref fields are internal drain-replay metadata, but RebornTimelineResponse still returns Vec from the product-workflow facade. The WebUI handler scrubs one HTTP route, but direct RebornServicesApi::get_timeline consumers and future facade routes can still receive the enriched substrate record.

local-patterns

1. Low Module overview is attached as an outer doc comment (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:1-16, confidence 75) - anchor: crates/ironclaw_reborn_composition/src/runtime.rs:1
   This file starts with /// comments, which document the following use item rather than the module. Nearby composition modules put file-level overviews in inner //! comments, so this new overview will not show up as module documentation.
2. Nit Comment describes an unreachable post-submit path (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:350-351, confidence 75) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:350
   The comment says this point is reached after validation skips, but validation skips use continue before the submit match and every submit-match arm returns. That leaves misleading guide text in the core drain loop.

maintainability

1. Medium Drain duplicates the canonical thread-scope rule (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:480-517, confidence 75) - anchor: crates/ironclaw_reborn/src/thread_scope.rs:9
   The new drain reconstructs ThreadScope from TurnLifecycleEvent and TurnRunState with local owner-fallback rules. ironclaw_reborn::thread_scope documents ThreadScopeResolver as the single owner-rewrite rule because hand-rolled copies can silently drift, and the new conversion now adds another copy in composition.
2. Medium DeferredBusy writes allow invalid new records (crates/ironclaw_threads/src/service.rs:46-53, confidence 75) - anchor: crates/ironclaw_threads/src/contract.rs:130
   The storage record needs optional turn binding refs for legacy rows, but the write boundary now accepts Option too. New DeferredBusy records can still be created in the legacy shape, and the drain will keep skipping those messages because the real contract lives only in callers.
3. Low New drain module lands as a 1,982-line file (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:540-541, confidence 100) - anchor: .claude/rules/architecture.md:186
   The new production observer and its large test harness are in one file, pushing a brand-new module past the repo's large-file threshold. Roughly three quarters of the file is inline test scaffolding and scenarios, which makes the production drain implementation harder to review and navigate.
   Also flagged by: conventions/Low

[henrypark133]

Medium - Post-submit persistence failure is untested.

After the coordinator accepts a drained turn, mark_message_submitted can fail and the observer is supposed to warn without failing the terminal transition. No adjacent drain test injects that persistence error, so the no-poisoning guarantee is uncovered for this branch.

Fix: Add a drain_mark_submitted_error_returns_ok_after_accept test covering mark_message_submitted failure after SubmitTurnResponse::Accepted.

[henrypark133]

Fixed in ba5f6ed — drain_mark_submitted_error_returns_ok_after_accept added.

[henrypark133]

Medium - Drain submit-error branch is untested.

The drain's failure contract says coordinator submit failures must log, return Ok, and leave the message deferred, but the adjacent tests only cover success, list errors, invalid records, and ThreadBusy. A non-ThreadBusy submit error could regress into poisoning the terminal path without a test catching it.

Fix: Add a drain_submit_error_returns_ok_and_leaves_deferred_busy test covering a non-ThreadBusy TurnCoordinator::submit_turn error.

[henrypark133]

Fixed in ba5f6ed — drain_submit_error_returns_ok_and_leaves_deferred_busy added.

[henrypark133]

Medium - Invalid deferred records can permanently starve later messages.

Each drain invocation starts at after_sequence = None and stops after examining 64 records. Because skipped invalid or legacy records are not mutated and no cursor is persisted across invocations, a thread with 64 invalid DeferredBusy rows before a valid one will re-scan the same 64 rows on every terminal event and never reach the valid message behind them.

Fix: Persist a skip or repair state for permanently invalid records, or change the cap logic so later valid records can eventually be reached across drain invocations.

[henrypark133]

Deferred to #4831 — persisted skip/non-drainable state for permanently invalid records belongs with the product_workflow replay owner.

[henrypark133]

Medium - Drain duplicates the canonical thread-scope rule.

The new drain reconstructs ThreadScope from TurnLifecycleEvent and TurnRunState with local owner-fallback rules. ironclaw_reborn::thread_scope documents ThreadScopeResolver as the single owner-rewrite rule because hand-rolled copies can silently drift, and the new conversion now adds another copy in composition.

Fix: Expose or reuse the canonical ThreadScopeResolver/thread-scope conversion from the owning Reborn runtime layer and delete the local thread_scope_from_event/thread_scope_from_state copies.

[henrypark133]

Fixed in ba5f6ed — drain now uses ThreadScopeResolver::derive_for_terminal_event/state; local owner-fallback copies deleted.

[henrypark133]

Medium - Keep internal drain refs out of facade timeline DTOs.

The new serde-visible turn_source_binding_ref and turn_reply_target_binding_ref fields are internal drain-replay metadata, but RebornTimelineResponse still returns Vec from the product-workflow facade. The WebUI handler scrubs one HTTP route, but direct RebornServicesApi::get_timeline consumers and future facade routes can still receive the enriched substrate record.

Fix: Return a product-safe timeline message type, or scrub these fields inside RebornServices::get_timeline before constructing RebornTimelineResponse.

[henrypark133]

Fixed in ba5f6ed — scrub moved into RebornServices::get_timeline so all facade consumers get scrubbed records; handler-level scrub removed; facade contract test added.

[henrypark133]

Medium - Deferred drain limit still scans the entire transcript.

The new drain path passes limit=8, but the filesystem backend first calls list_thread_messages(), which lists, reads, deserializes, and sorts every message in the thread before filtering DeferredBusy rows and truncating. A large thread can therefore turn each terminal event that triggers the drain into an O(thread size) filesystem scan despite the small drain cap.

Fix: Back DeferredBusy listing with a per-status sequence index or queue, or otherwise honor the limit before reading and deserializing the full transcript.

Also flagged by: performance/Medium

[henrypark133]

Tracked in #4833 (per-thread deferred index).

[henrypark133]

Medium - DeferredBusy writes allow invalid new records.

The storage record needs optional turn binding refs for legacy rows, but the write boundary now accepts Option too. New DeferredBusy records can still be created in the legacy shape, and the drain will keep skipping those messages because the real contract lives only in callers.

Fix: Keep ThreadMessageRecord fields optional for old persisted data, but make mark_message_deferred_busy require canonical source and reply refs for new writes. Tests that need legacy rows should build those records directly instead of going through the write API.

[henrypark133]

Deferred to #4831 — the replay-owner refactor touches the mark_message_deferred_busy contract; tightening new writes to require canonical refs belongs in that change.

[henrypark133]

Low - New drain module lands as a 1,982-line file.

The new production observer and its large test harness are in one file, pushing a brand-new module past the repo's large-file threshold. Roughly three quarters of the file is inline test scaffolding and scenarios, which makes the production drain implementation harder to review and navigate.

Fix: Keep the production observer in deferred_busy_drain.rs and move the test module/harness into deferred_busy_drain/tests.rs or a focused integration test file.

Also flagged by: conventions/Low

[henrypark133]

Deferred — mechanical test/module split; skipping pre-merge churn.

[henrypark133]

Low - Module overview is attached as an outer doc comment.

This file starts with /// comments, which document the following use item rather than the module. Nearby composition modules put file-level overviews in inner //! comments, so this new overview will not show up as module documentation.

Fix: Change the file header from /// to //! so it follows the local module-documentation pattern.

[henrypark133]

Fixed in ba5f6ed — file overview converted to inner //! docs.

[henrypark133]

Nit - Comment describes an unreachable post-submit path.

The comment says this point is reached after validation skips, but validation skips use continue before the submit match and every submit-match arm returns. That leaves misleading guide text in the core drain loop.

Fix: Delete the comment, or move the explanation next to the validation continue paths it describes.

[henrypark133]

Fixed in ba5f6ed — comment reworded to describe the skip-only reality.

[henrypark133]

Code Review (multi-agent)

Intent: Automatically drain DeferredBusy messages once the blocking run reaches terminal state, preserving identity and retry/idempotency semantics.
Stats: 11 findings (from 14 raw, 11 after dedup) across 6 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0

bugs

1. Medium Zero-limit deferred scan clears live deferred marker (crates/ironclaw_threads/src/in_memory.rs:304-315, confidence 75) — anchor: crates/ironclaw_threads/src/in_memory.rs:304
   list_deferred_busy_messages applies limit before deciding whether a no-cursor scan found zero deferred rows. A caller using limit: Some(0) gets an empty vector even when DeferredBusy messages exist, then removes the deferred_threads marker. Future drains take the fast path and return empty, leaving those messages permanently hidden until another defer happens.
2. Medium Zero-limit deferred scan deletes live marker (crates/ironclaw_threads/src/filesystem_service.rs:949-962, confidence 75) — anchor: crates/ironclaw_threads/src/filesystem_service.rs:949
   The filesystem backend has the same pre-limit/tombstone ordering bug: limit: Some(0) truncates a non-empty DeferredBusy result to empty, then the no-cursor empty check deletes deferred-busy.marker. After that, future drain calls skip the scan and the existing deferred records are no longer discoverable.
3. Medium Drain cap can permanently strand valid tail messages (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:106-116, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:106
   The drain's total_examined cap is local to a single invocation, while invalid deferred records are intentionally left unmodified. If a thread has 64 invalid legacy DeferredBusy records before a valid one, every terminal event restarts at after_sequence None, re-examines the same invalid prefix, hits the cap again, and never reaches the valid message behind it.
   Also flagged by: tests/Medium.

performance

1. High Deferred marker write can miss the only drain event (crates/ironclaw_threads/src/filesystem_service.rs:897-916, confidence 90) — anchor: crates/ironclaw_threads/src/filesystem_service.rs:897
   The filesystem backend commits the message as DeferredBusy, then writes the presence marker in a separate operation. If the blocking run reaches terminal state between those two awaits, the observer calls list_deferred_busy_messages, sees no marker, returns empty, and the marker is written only after the only drain trigger has already passed. With no later terminal event or defer, that message remains DeferredBusy indefinitely under a normal timing race.
2. Medium Deferred pagination still reads the whole thread (crates/ironclaw_threads/src/filesystem_service.rs:938-950, confidence 82) — anchor: crates/ironclaw_threads/src/filesystem_service.rs:267
   list_deferred_busy_messages accepts limit and after_sequence, but the filesystem implementation first calls list_thread_messages, which lists the message directory and reads every message JSON before filtering and truncating. Every drain on a marked long thread therefore does O(total thread messages) file reads, and a window of invalid deferred records can repeat that scan up to eight times despite DRAIN_TOTAL_CAP.
   Also flagged by: security/Medium.

tests

1. Medium Persisted binding refs are not asserted on submit (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:177-204, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:177
   The drain is supposed to replay the canonical source/reply refs persisted on the message, but the existing verbatim test only checks that the message becomes Submitted; it never captures the SubmitTurnRequest sent to the coordinator. A drain that re-derived different valid refs would still pass.

conventions

1. Medium Drain bypasses the product workflow submit boundary (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:280-295, confidence 75) — anchor: crates/ironclaw_product_workflow/CLAUDE.md:7
   The new drain reconstructs a SubmitTurnRequest from thread records and calls TurnCoordinator directly from ironclaw_reborn_composition, then marks the message submitted itself. That duplicates the product-workflow inbound submit path, which owns binding resolution, turn submission, idempotency, and busy/deferred handling, so future policy changes in the product workflow path can drift from drain replays.
   Also flagged by: maintainability/Medium.
2. Low New drain module exceeds the file-size budget (crates/ironclaw_reborn_composition/src/deferred_busy_drain.rs:1-1, confidence 75) — anchor: .claude/rules/architecture.md:126
   This PR adds deferred_busy_drain.rs as a 2440-line new file. The architecture rule says new Rust files should aim for under 800 lines and that oversized files need a tracking issue; this module has no arch-exempt/tracking note, so the drain implementation and its large test harness are starting above the repo's reviewability budget.

local-patterns

1. Low Deferred-busy docs describe the wrong replay path (crates/ironclaw_threads/src/contract.rs:438-440, confidence 75) — anchor: .claude/rules/review-discipline.md; crates/ironclaw_threads/src/contract.rs:438
   The new request docs say the drain resubmits messages through the normal inbound path, but the changed implementation builds a SubmitTurnRequest in DeferredBusyDrainObserver and calls TurnCoordinator directly. In this repo, doc strings are treated as contract, so this stale path description will send future readers looking for product inbound replay semantics that are not actually used.
2. Low E2E test target is only a redirect comment (crates/ironclaw_reborn_composition/tests/deferred_busy_drain_e2e.rs:1-27, confidence 75) — anchor: crates/ironclaw_reborn_composition/tests/subagent_runtime_wiring.rs:5
   This new tests/*_e2e.rs target is documented as integration tests but contains no executable tests; even grep for #[tokio::test] lands on the doc comment. Sibling test targets in this crate contain real test functions, so normal path-based navigation or cargo test --test deferred_busy_drain_e2e leads to an empty target while the actual tests are hidden inline in src/deferred_busy_drain.rs.

maintainability

1. Medium DeferredBusy writers can omit refs the drain requires (crates/ironclaw_threads/src/service.rs:46-53, confidence 75) — anchor: crates/ironclaw_threads/src/service.rs:46
   mark_message_deferred_busy now accepts optional canonical source/reply refs, but the new drain can only resubmit records when both refs are present. That leaves the real writer contract hidden: new callers can create fresh DeferredBusy rows that are structurally undrainable, and the failure is only discovered later as a warn-and-skip path.

[henrypark133]

High — Deferred marker write can miss the only drain event.

The filesystem backend commits the message as DeferredBusy, then writes the presence marker in a separate operation. If the blocking run reaches terminal state between those two awaits, the observer calls list_deferred_busy_messages, sees no marker, returns empty, and the marker is written only after the only drain trigger has already passed. With no later terminal event or defer, that message remains DeferredBusy indefinitely under a normal timing race.

Fix: Serialize the status flip, marker update, and marker-based listing with a shared per-thread lock, or otherwise make the post-defer path schedule or attempt a drain after the marker is durable.

[henrypark133]

Fixed in 8cff5cc — both defer paths retry submit_turn once after the defer (and marker) commit: Accepted = run terminated in the gap, message takes the normal accept path; ThreadBusy = marker already durable for the eventual terminal drain. Window closed from both sides; 6 caller-level tests added.

[henrypark133]

Medium — Deferred pagination still reads the whole thread.

list_deferred_busy_messages accepts limit and after_sequence, but the filesystem implementation first calls list_thread_messages, which lists the message directory and reads every message JSON before filtering and truncating. Every drain on a marked long thread therefore does O(total thread messages) file reads, and a window of invalid deferred records can repeat that scan up to eight times despite DRAIN_TOTAL_CAP.

Fix: Use the sequence index or add a deferred-message index so the backend reads forward from after_sequence and stops once limit matching DeferredBusy user messages are found.

Also flagged by: security/Medium

[henrypark133]

Tracked in #4833 — per-thread deferred index honoring limit before reading the transcript.

[henrypark133]

Medium — Zero-limit deferred scan clears live deferred marker.

list_deferred_busy_messages applies limit before deciding whether a no-cursor scan found zero deferred rows. A caller using limit: Some(0) gets an empty vector even when DeferredBusy messages exist, then removes the deferred_threads marker. Future drains take the fast path and return empty, leaving those messages permanently hidden until another defer happens.

Fix: Track whether any rows matched before truncating, and tombstone only when the pre-limit match set is empty.

[henrypark133]

Fixed in 18bb5b3 — tombstone now keyed on the untruncated match count; zero-limit contract test added.

[henrypark133]

Medium — Zero-limit deferred scan deletes live marker.

The filesystem backend has the same pre-limit/tombstone ordering bug: limit: Some(0) truncates a non-empty DeferredBusy result to empty, then the no-cursor empty check deletes deferred-busy.marker. After that, future drain calls skip the scan and the existing deferred records are no longer discoverable.

Fix: Base marker deletion on the untruncated match count, not on the returned page after applying limit.

[henrypark133]

Fixed in 18bb5b3 — same pre-limit count fix on the filesystem backend, with contract test.

[henrypark133]

Medium — Drain cap can permanently strand valid tail messages.

The drain's total_examined cap is local to a single invocation, while invalid deferred records are intentionally left unmodified. If a thread has 64 invalid legacy DeferredBusy records before a valid one, every terminal event restarts at after_sequence None, re-examines the same invalid prefix, hits the cap again, and never reaches the valid message behind it.

Fix: Persist progress past skipped invalid records, mark them with a non-blocking failure state, or otherwise ensure the next invocation starts after the capped window.

Also flagged by: tests/Medium

[henrypark133]

Tracked in #4831 — persisted skip/non-drainable state for permanently invalid records.

[henrypark133]

Medium — Drain bypasses the product workflow submit boundary.

The new drain reconstructs a SubmitTurnRequest from thread records and calls TurnCoordinator directly from ironclaw_reborn_composition, then marks the message submitted itself. That duplicates the product-workflow inbound submit path, which owns binding resolution, turn submission, idempotency, and busy/deferred handling, so future policy changes in the product workflow path can drift from drain replays.

Fix: Move the deferred-message replay boundary into ironclaw_product_workflow or expose a workflow-owned replay method that accepts persisted canonical refs, then keep the observer as wiring.

Also flagged by: maintainability/Medium

[henrypark133]

Tracked in #4831 — drain resubmission moves behind a product_workflow replay entry point; observer stays as wiring.

[henrypark133]

Medium — DeferredBusy writers can omit refs the drain requires.

mark_message_deferred_busy now accepts optional canonical source/reply refs, but the new drain can only resubmit records when both refs are present. That leaves the real writer contract hidden: new callers can create fresh DeferredBusy rows that are structurally undrainable, and the failure is only discovered later as a warn-and-skip path.

Fix: Replace the loose optional parameters with a request type that requires canonical routing refs for new deferrals, while keeping the ThreadMessageRecord fields optional only for legacy deserialization/backward compatibility.

[henrypark133]

Tracked in #4831 — request type requiring canonical refs for new deferrals lands with the replay-owner refactor.

[henrypark133]

Low — New drain module exceeds the file-size budget.

This PR adds deferred_busy_drain.rs as a 2440-line new file. The architecture rule says new Rust files should aim for under 800 lines and that oversized files need a tracking issue; this module has no arch-exempt/tracking note, so the drain implementation and its large test harness are starting above the repo's reviewability budget.

Fix: Split the observer tests/support into separate modules or add an arch-exempt large_file note with the decomposition issue.

[henrypark133]

Fixed in 18bb5b3 — split into deferred_busy_drain/mod.rs (494-line observer) + tests.rs.

[henrypark133]

Low — Deferred-busy docs describe the wrong replay path.

The new request docs say the drain resubmits messages through the normal inbound path, but the changed implementation builds a SubmitTurnRequest in DeferredBusyDrainObserver and calls TurnCoordinator directly. In this repo, doc strings are treated as contract, so this stale path description will send future readers looking for product inbound replay semantics that are not actually used.

Fix: Update the comment to say the drain uses the returned records to resubmit through the coordinator with persisted canonical binding refs, or remove the path detail from this contract type.

[henrypark133]

Fixed in 18bb5b3 — docs now describe coordinator replay with persisted canonical refs.

[henrypark133]

Low — E2E test target is only a redirect comment.

This new tests/*_e2e.rs target is documented as integration tests but contains no executable tests; even grep for #[tokio::test] lands on the doc comment. Sibling test targets in this crate contain real test functions, so normal path-based navigation or cargo test --test deferred_busy_drain_e2e leads to an empty target while the actual tests are hidden inline in src/deferred_busy_drain.rs.

Fix: Either move an executable integration-style test into this file via a small public/test-support seam, or delete/rename the no-op target and rely on the inline unit-test module without advertising an empty e2e file.

[henrypark133]

Fixed in 18bb5b3 — no-op e2e target deleted; coverage lives in the inline drain tests + runtime wiring test.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs`:
- Around line 14-16: Replace all internal diagnostic uses of the warn! macro in
the deferred busy-drain observer with debug! (or a typed source logger) so
background terminal-state handling never emits warn/info that can corrupt the
REPL/TUI; specifically, locate each branch where warn! is invoked (the
observer's drain-failure handlers and the other warn! occurrences referenced)
and swap warn! -> debug! while preserving the log message text and the
subsequent return Ok(()) behavior. Ensure no other behavior changes and apply
this replacement for the listed occurrences (including the many warn! calls in
the observer branches).
- Around line 106-116: The loop resets after_sequence to None each invocation
causing starvation when >DRAIN_TOTAL_CAP invalid DeferredBusy rows block valid
ones; update DeferredBusyDrainObserver so after_sequence is advanced past
fully-skipped windows (or mark/quarantine permanently-invalid rows) instead of
always restarting at None so subsequent iterations resume after the last
examined sequence; modify the logic around after_sequence and the loop that
checks DRAIN_TOTAL_CAP to persist progress across paged scans (or delete/mark
invalid rows) and add a regression test (#[tokio::test]) that inserts
DRAIN_TOTAL_CAP + 1 invalid DeferredBusy records followed by one valid record to
reproduce and verify the fix.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5c764b5e-13aa-4b49-914f-a9e5c5a5379a

📥 Commits

Reviewing files that changed from the base of the PR and between 7418309 and c9b956a.

📒 Files selected for processing (7)

• crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs
• crates/ironclaw_reborn_composition/src/deferred_busy_drain/tests.rs
• crates/ironclaw_threads/src/contract.rs
• crates/ironclaw_threads/src/filesystem_service.rs
• crates/ironclaw_threads/src/in_memory.rs
• crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs
• crates/ironclaw_threads/tests/session_thread_contract.rs

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

DRAIN_TOTAL_CAP can permanently starve valid deferred messages behind a long invalid head.

after_sequence is reset to None on every invocation, so once a thread has more than 64 invalid DeferredBusy records at the front, each terminal event re-examines the same first 64 and exits at the cap. Any later valid message is never reached, so the queue can stall indefinitely. Persist progress past fully-skipped windows or quarantine permanently invalid rows, and add a regression test with DRAIN_TOTAL_CAP + 1 invalid records followed by one valid record. As per coding guidelines, "Every bug fix must include a regression test (#[test] or #[tokio::test]) that reproduces the original failure."

Also applies to: 355-363

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs` around
lines 106 - 116, The loop resets after_sequence to None each invocation causing
starvation when >DRAIN_TOTAL_CAP invalid DeferredBusy rows block valid ones;
update DeferredBusyDrainObserver so after_sequence is advanced past
fully-skipped windows (or mark/quarantine permanently-invalid rows) instead of
always restarting at None so subsequent iterations resume after the last
examined sequence; modify the logic around after_sequence and the loop that
checks DRAIN_TOTAL_CAP to persist progress across paged scans (or delete/mark
invalid rows) and add a regression test (#[tokio::test]) that inserts
DRAIN_TOTAL_CAP + 1 invalid DeferredBusy records followed by one valid record to
reproduce and verify the fix.

Source: Coding guidelines

[henrypark133]

Tracked in #4831 — persisted skip/non-drainable state for permanently invalid records (regression test specified there).

[coderabbitai]

@henrypark133 Acknowledged — deferring to #4831 is reasonable given scope. The starvation invariant remains: a thread with > DRAIN_TOTAL_CAP permanently invalid records at the head will silently stop making progress until #4831's persisted skip/quarantine lands. Keeping this comment open until that ships.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/ironclaw_product_workflow/tests/reborn_services_contract.rs (1)

8202-8415: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add the required large-file growth justification for this new test block.

Lines 8202-8415 add a sizable section to a file already far beyond 3,000 lines, but this addition does not include an inline arch-exempt: large_file justification tied to this expansion (or equivalent decomposition move).

As per coding guidelines, "Maintain file size discipline: ... files > 3,000 lines should have a tracking issue for decomposition. PRs adding > 200 lines to files > 3,000 lines require an inline justification."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_product_workflow/tests/reborn_services_contract.rs` around
lines 8202 - 8415, The new test block
(webui_defer_retry_still_busy_message_stays_deferred,
webui_defer_retry_gap_hit_message_becomes_submitted,
webui_defer_retry_non_busy_error_message_stays_deferred_call_succeeds) added
~200+ lines to an already-large file needs the required large-file
justification: add an inline comment immediately above the new tests stating
arch-exempt: large_file and reference a tracking/decomposition issue (or explain
why decomposition isn’t feasible), or move these tests into a new test
module/file and update imports; ensure the marker/comment is adjacent to the
tests so reviewers can see the justification.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_product_workflow/src/inbound_turn.rs`:
- Around line 698-701: The DeferredBusy paths currently return the original
blocking info (accepted_message_ref / busy from earlier) instead of the latest
blocker from the retry; update the handlers in inbound_turn.rs (the branch that
constructs InboundTurnOutcome::DeferredBusy) and the analogous path in
reborn_services.rs so they use the ThreadBusy payload from the retry
Err(TurnError::ThreadBusy(new_busy)) rather than the stale `busy` captured
earlier; locate the place constructing InboundTurnOutcome::DeferredBusy in
submit_turn/submit_turn_retry flows and replace references to the original
blocker with the retry's blocker (use the new_busy.active_run_id /
new_busy.binding / new_busy.event_cursor as needed) so callers receive the
up-to-date blocker information.

In `@crates/ironclaw_threads/src/filesystem_service.rs`:
- Around line 893-899: The comments overstate a timing guarantee: update the
comment near submit_turn and mark_message_submitted in the filesystem_service.rs
post-defer retry block and the comment in DeferredBusyDrain
(deferred_busy_drain::mod) to state the actual invariant — the retry can close
the marker-visibility gap and the drain:<message_id> key deduplicates
observer-side replays, but there is no hard ordering that the retry always wins
before a drain starts; remove language asserting the retry "beats" the observer
or guarantees exclusive ordering and instead describe the weaker race-safe
outcome (marker visibility closure + observer-side deduplication) and how those
two pieces interact to avoid double-submission.

---

Outside diff comments:
In `@crates/ironclaw_product_workflow/tests/reborn_services_contract.rs`:
- Around line 8202-8415: The new test block
(webui_defer_retry_still_busy_message_stays_deferred,
webui_defer_retry_gap_hit_message_becomes_submitted,
webui_defer_retry_non_busy_error_message_stays_deferred_call_succeeds) added
~200+ lines to an already-large file needs the required large-file
justification: add an inline comment immediately above the new tests stating
arch-exempt: large_file and reference a tracking/decomposition issue (or explain
why decomposition isn’t feasible), or move these tests into a new test
module/file and update imports; ensure the marker/comment is adjacent to the
tests so reviewers can see the justification.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bba693ce-a43e-4f81-96bc-60e19fa19768

📥 Commits

Reviewing files that changed from the base of the PR and between c9b956a and 8cff5cc.

📒 Files selected for processing (7)

• crates/ironclaw_product_workflow/src/inbound_turn.rs
• crates/ironclaw_product_workflow/src/reborn_services.rs
• crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs
• crates/ironclaw_product_workflow/tests/product_workflow_contract.rs
• crates/ironclaw_product_workflow/tests/reborn_services_contract.rs
• crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs
• crates/ironclaw_threads/src/filesystem_service.rs

[henrypark133]

Code Review (multi-agent)

Intent: Automatically resubmit DeferredBusy messages after a gate-blocked run reaches terminal state.

Stats: 15 findings (from 15 raw, 15 after dedup) across 7 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0

security

1. Medium Drain can submit a second run after post-defer retry (crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:255-258, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:257
   The new defer paths retry submit_turn with the original idempotency key after marking the message DeferredBusy. If that retry is accepted but mark_message_submitted fails, the run is already queued under the original key while the transcript record remains DeferredBusy. The drain later resubmits the same accepted_message_ref with a different key, drain:<message_id>, so coordinator idempotency will not replay the accepted retry and can create a second run for the same user message, re-executing side effects and consuming quota after a transient thread-store failure.

bugs

1. High Deferred message can be hidden if marker write fails (crates/ironclaw_threads/src/filesystem_service.rs:900-919, confidence 82) — anchor: crates/ironclaw_threads/src/filesystem_service.rs:900
   The filesystem backend commits the message status to DeferredBusy before writing the deferred-busy presence marker. If the marker put fails after the message update succeeds, mark_message_deferred_busy returns an error and the post-defer retry is skipped, but the message remains DeferredBusy. Later list_deferred_busy_messages returns empty when the marker is absent, so the terminal-state drain will never see or resubmit that message.
2. Medium Retry-busy response returns stale active-run metadata (crates/ironclaw_product_workflow/src/reborn_services.rs:1827-1833, confidence 78) — anchor: crates/ironclaw_product_workflow/src/reborn_services.rs:1827
   After the post-defer retry, this arm ignores the ThreadBusy value returned by the retry and reports the active_run_id, status, and event_cursor from the original ThreadBusy. The turn store does not memoize ThreadBusy idempotency results, so the retry can legitimately be blocked by a different run with different status/cursor. WebUI clients can then watch the wrong run or resume events from a stale cursor.
3. Medium Product retry-busy ack can report the wrong active run (crates/ironclaw_product_workflow/src/inbound_turn.rs:698-702, confidence 74) — anchor: crates/ironclaw_product_workflow/src/inbound_turn.rs:698
   The product inbound post-defer retry also discards the retry's ThreadBusy payload and returns the original active_run_id. If the first blocking run terminated and another run acquired the thread before the retry, the ProductInboundAck points adapters at the old run even though the message is now deferred behind the newer active run.

performance

1. Medium Deferred drain limit still scans every message (crates/ironclaw_threads/src/filesystem_service.rs:941-943, confidence 92) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:137
   list_deferred_busy_messages accepts limit: Some(8) from the terminal-event drain, but the filesystem implementation first calls list_thread_messages, which lists the whole message directory and serially reads every message JSON before filtering/truncating. Any thread with a stale deferred marker and a long transcript now makes every terminal run publication perform O(total thread messages) filesystem I/O on the terminal path, even when only one deferred message can be drained.

tests

1. Medium Terminal-event scope derivation lacks direct tests (crates/ironclaw_reborn/src/thread_scope.rs:62-79, confidence 100) — anchor: crates/ironclaw_reborn/src/thread_scope.rs:62
   derive_for_terminal_event is new public API and encodes explicit-owner fallback plus the agentless error path. The adjacent tests only cover the older resolver methods, and scoped search found only production calls from the drain observer.
2. Medium Terminal-state scope derivation lacks direct tests (crates/ironclaw_reborn/src/thread_scope.rs:90-105, confidence 100) — anchor: crates/ironclaw_reborn/src/thread_scope.rs:90
   derive_for_terminal_state is new public API and its owner precedence differs from the event helper by falling back to state.actor. The adjacent tests do not call it or exercise the actor fallback and agentless error paths.
3. Medium Malformed persisted binding refs are untested (crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:190-227, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:190
   The drain explicitly skips records whose persisted turn_source_binding_ref or turn_reply_target_binding_ref fails parsing, but existing drain tests cover missing refs and valid refs, not malformed Some(...) refs. A regression could stop scanning or submit with re-derived refs for corrupted persisted metadata.
4. Low Drain total-cap boundary is untested (crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:122-167, confidence 100) — anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:123
   The drain has a DRAIN_TOTAL_CAP boundary to stop paging through pathological invalid deferred records, but adjacent tests cover one skipped record and normal cascades, not the cap boundary. This leaves the bounded-scan guarantee unprotected.

conventions

1. Low New test module exceeds the file-size budget (crates/ironclaw_reborn_composition/src/deferred_busy_drain/tests.rs:1-1, confidence 75) — anchor: .claude/rules/architecture.md:132
   The new deferred-busy drain test module is 2054 lines, which exceeds the architecture rule that new .rs files should aim for under 800 lines. This makes the new scenario suite hard to review and maintain as a single unit.
2. Low Large contract test file grows without justification (crates/ironclaw_product_workflow/tests/reborn_services_contract.rs:549-553, confidence 75) — anchor: .claude/rules/architecture.md:136
   This PR adds 466 lines to reborn_services_contract.rs, which is already over 3000 lines, starting with another coordinator test helper. The architecture rule says files over 3000 lines need a decomposition issue, and PRs adding more than 200 lines need inline justification.

local-patterns

1. Low Observer logs do not match sibling tracing style (crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:144-148, confidence 75) — anchor: crates/ironclaw_reborn/src/subagent/completion_observer.rs:246
   The new observer prefixes tracing messages with DeferredBusyDrainObserver: throughout the module. Sibling observers rely on the tracing target plus structured fields and use event-shaped messages, so this new prefix makes log search/filtering inconsistent and adds repeated noise without carrying new context.
2. Nit Test helper name uses an inconsistent Reborns plural (crates/ironclaw_product_workflow/tests/reborn_services_contract.rs:554-559, confidence 75) — anchor: crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs:114
   ScriptedRebornsCoordinator introduces a new pluralized name for a coordinator test double, while the crate consistently names the facade RebornServices and sibling scripted coordinator helpers use ScriptedTurnCoordinator. The typo-like plural makes the helper harder to find by the established names.

maintainability

1. Medium Internal drain refs leak into the UI transcript record (crates/ironclaw_threads/src/contract.rs:130-141, confidence 75) — anchor: crates/ironclaw_threads/src/contract.rs:109
   ThreadMessageRecord is documented as the UI/projection transcript snapshot, but the drain-only routing refs are now added directly to that shared record. That makes every product-facing history/timeline caller carry a hidden scrub requirement, as shown by the new scrub_internal_timeline_refs helper in the product facade, and future consumers can accidentally expose or couple to drain replay metadata.

approach

1. Medium Legacy DeferredBusy rows are outside the drain transition (crates/ironclaw_threads/src/filesystem_service.rs:927-938, confidence 75) — anchor: crates/ironclaw_threads/src/filesystem_service.rs:927
   The PR adds new persisted canonical binding refs plus a deferred-busy marker, but the filesystem list path returns empty when the new marker is absent, and the drain later skips records whose new refs are None. The base system already persisted DeferredBusy messages before either field/marker existed, so those messages remain silently dead after upgrade even though this PR's goal is to recover deferred messages. A transition/backfill path is net safer than treating pre-change rows as impossible because it makes the persisted-contract change explicit and prevents existing user messages from being stranded.

[henrypark133]

High — Deferred message can be hidden if marker write fails.

The filesystem backend commits the message status to DeferredBusy before writing the deferred-busy presence marker. If the marker put fails after the message update succeeds, mark_message_deferred_busy returns an error and the post-defer retry is skipped, but the message remains DeferredBusy. Later list_deferred_busy_messages returns empty when the marker is absent, so the terminal-state drain will never see or resubmit that message.

Fix: Make the status update and marker durable atomically, or on marker-write failure restore the message to an accepted/retryable state or make listing fall back to a full scan when marker creation may have failed.

[henrypark133]

Mitigation noted on #4817 (startup sweep is the systemic repair for marker-write failure); the post-defer retry in 8cff5cc already narrows in-session impact: a failed defer returns an error to the caller, which surfaces the submit failure instead of silently parking the message.

[henrypark133]

Medium — Terminal-event scope derivation lacks direct tests.

derive_for_terminal_event is new public API and encodes explicit-owner fallback plus the agentless error path. The adjacent tests only cover the older resolver methods, and scoped search found only production calls from the drain observer.

Fix: tests::thread_scope::derive_for_terminal_event_prefers_explicit_owner_falls_back_to_event_owner_and_rejects_agentless covering explicit owner, event owner fallback, and agentless error

[henrypark133]

Fixed in 0c3fd56 — derive_for_terminal_event branch tests added (explicit owner, event-owner fallback, agentless error).

[henrypark133]

Medium — Terminal-state scope derivation lacks direct tests.

derive_for_terminal_state is new public API and its owner precedence differs from the event helper by falling back to state.actor. The adjacent tests do not call it or exercise the actor fallback and agentless error paths.

Fix: tests::thread_scope::derive_for_terminal_state_prefers_explicit_owner_falls_back_to_actor_and_rejects_agentless covering explicit owner, actor fallback, and agentless error

[henrypark133]

Fixed in 0c3fd56 — derive_for_terminal_state branch tests added (explicit owner, actor fallback, agentless error).

[henrypark133]

Medium — Deferred drain limit still scans every message.

list_deferred_busy_messages accepts limit: Some(8) from the terminal-event drain, but the filesystem implementation first calls list_thread_messages, which lists the whole message directory and serially reads every message JSON before filtering/truncating. Any thread with a stale deferred marker and a long transcript now makes every terminal run publication perform O(total thread messages) filesystem I/O on the terminal path, even when only one deferred message can be drained.

Fix: Maintain a DeferredBusy sequence/status index, or page through the existing sequence index and stop once the requested limit is satisfied instead of materializing the full transcript.

[henrypark133]

Tracked in #4833 — per-thread deferred index honoring limit before transcript reads.

[henrypark133]

Medium — Retry-busy response returns stale active-run metadata.

After the post-defer retry, this arm ignores the ThreadBusy value returned by the retry and reports the active_run_id, status, and event_cursor from the original ThreadBusy. The turn store does not memoize ThreadBusy idempotency results, so the retry can legitimately be blocked by a different run with different status/cursor. WebUI clients can then watch the wrong run or resume events from a stale cursor.

Fix: Bind the retry busy value and return its active_run_id, status, and event_cursor.

[henrypark133]

Fixed in 0c3fd56 — WebUI response now returns the retry blocker's active_run_id, status, and event_cursor; test asserts run Y + cursor 99.

[henrypark133]

Low — Drain total-cap boundary is untested.

The drain has a DRAIN_TOTAL_CAP boundary to stop paging through pathological invalid deferred records, but adjacent tests cover one skipped record and normal cascades, not the cap boundary. This leaves the bounded-scan guarantee unprotected.

Fix: tests::deferred_busy_drain::drain_stops_after_total_cap_when_all_records_invalid covering more than DRAIN_TOTAL_CAP invalid deferred messages and no submit attempt after the cap

[henrypark133]

Fixed in 0c3fd56 — drain_stops_after_total_cap_when_all_records_invalid (Scenario M) covers >64 invalid records, bounded list calls, no submit.

[henrypark133]

Low — Large contract test file grows without justification.

This PR adds 466 lines to reborn_services_contract.rs, which is already over 3000 lines, starting with another coordinator test helper. The architecture rule says files over 3000 lines need a decomposition issue, and PRs adding more than 200 lines need inline justification.

Fix: Move the new post-defer retry coverage into a focused test file/module or add an inline large-file justification with a tracking plan.

[henrypark133]

Header note added in 0c3fd56; retry/defer coverage moves with #4831.

[henrypark133]

Low — New test module exceeds the file-size budget.

The new deferred-busy drain test module is 2054 lines, which exceeds the architecture rule that new .rs files should aim for under 800 lines. This makes the new scenario suite hard to review and maintain as a single unit.

Fix: Split the drain harness and scenario groups into smaller test modules, or add an explicit decomposition plan if this must stay monolithic.

[henrypark133]

Decomposition note added in 0c3fd56; the suite moves with the #4831 replay-owner refactor.

[henrypark133]

Low — Observer logs do not match sibling tracing style.

The new observer prefixes tracing messages with DeferredBusyDrainObserver: throughout the module. Sibling observers rely on the tracing target plus structured fields and use event-shaped messages, so this new prefix makes log search/filtering inconsistent and adds repeated noise without carrying new context.

Fix: Remove the DeferredBusyDrainObserver: prefix from the new debug!/warn! messages, or add structured context such as observer = "deferred_busy_drain" if an explicit discriminator is needed.

[henrypark133]

Fixed in 0c3fd56 — prefix dropped, observer structured field added.

[henrypark133]

Nit — Test helper name uses an inconsistent Reborns plural.

ScriptedRebornsCoordinator introduces a new pluralized name for a coordinator test double, while the crate consistently names the facade RebornServices and sibling scripted coordinator helpers use ScriptedTurnCoordinator. The typo-like plural makes the helper harder to find by the established names.

Fix: Rename the helper and its impl/use sites to ScriptedTurnCoordinator or ScriptedRebornServicesCoordinator.

[henrypark133]

Fixed in 0c3fd56 — renamed to ScriptedTurnCoordinator.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/ironclaw_reborn/src/thread_scope.rs (1)

62-66: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Give the new public derivation helpers a typed error.

These methods were just added to the public API, but they return &'static str. That makes downstream handling and future evolution contract-by-text. A small ThreadScopeDerivationError enum would keep the surface stable without changing the call pattern. As per coding guidelines, "Prefer strong types over strings (enums, newtypes)".

Also applies to: 90-93

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn/src/thread_scope.rs` around lines 62 - 66, Replace the
&'static str error returns from the new public derivation helpers (e.g.,
derive_for_terminal_event and the similar method around lines 90–93) with a
typed error enum (e.g., ThreadScopeDerivationError) and update their signatures
to return Result<ThreadScope, ThreadScopeDerivationError>; implement variants
for the current cases (for example AgentlessScope) and adjust all early returns
(currently returning Err("agentless turn scope — no ThreadScope")) to return the
appropriate enum variant; update any callers/tests to pattern-match the enum (or
use ? where appropriate) so external code can handle errors via type rather than
raw strings.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_product_workflow/tests/reborn_services_contract.rs`:
- Around line 8282-8330: The test
webui_defer_retry_still_busy_reports_retry_run_id_not_original currently only
asserts active_run_id and event_cursor; extend the pattern matcher on outcome
(RebornSubmitTurnResponse::DeferredBusy) to also check the reported status
equals the retry's status (the queued status pushed for run_y, e.g.
TurnStatus::Queued) so the DeferredBusy payload is validated for active_run_id,
status, and event_cursor together (use the existing id and EventCursor(99)
checks and add a status == TurnStatus::Queued condition referencing
run_y/coordinator setup).

---

Outside diff comments:
In `@crates/ironclaw_reborn/src/thread_scope.rs`:
- Around line 62-66: Replace the &'static str error returns from the new public
derivation helpers (e.g., derive_for_terminal_event and the similar method
around lines 90–93) with a typed error enum (e.g., ThreadScopeDerivationError)
and update their signatures to return Result<ThreadScope,
ThreadScopeDerivationError>; implement variants for the current cases (for
example AgentlessScope) and adjust all early returns (currently returning
Err("agentless turn scope — no ThreadScope")) to return the appropriate enum
variant; update any callers/tests to pattern-match the enum (or use ? where
appropriate) so external code can handle errors via type rather than raw
strings.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e59805a1-8d5c-425e-849f-6c2509a9d011

📥 Commits

Reviewing files that changed from the base of the PR and between 8cff5cc and 0c3fd56.

📒 Files selected for processing (8)

• crates/ironclaw_product_workflow/src/inbound_turn.rs
• crates/ironclaw_product_workflow/src/reborn_services.rs
• crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs
• crates/ironclaw_product_workflow/tests/reborn_services_contract.rs
• crates/ironclaw_reborn/src/thread_scope.rs
• crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs
• crates/ironclaw_reborn_composition/src/deferred_busy_drain/tests.rs
• crates/ironclaw_threads/src/filesystem_service.rs

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/ironclaw_product_workflow/src/reborn_services.rs (1)

2748-2769: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Fail loud on deferred-mark write failures.

Line 2766 only verifies MessageStatus::DeferredBusy. If mark_message_deferred_busy(...) returns after flipping status but before persisting turn_source_binding_ref, turn_reply_target_binding_ref, or turn_idempotency_key, this path still returns success and leaves an undrainable record that the new drain will skip as legacy. Either verify the full persisted message contract here, or propagate the error instead of reconciling on status alone.

As per coding guidelines, "Fail loud: flag silent-failure patterns — warn-and-continue that poisons state. Errors propagate with ? into thiserror types with context." and "Test through the caller: when a helper gates a side effect, require a test driving the real call site, not only the helper."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_product_workflow/src/reborn_services.rs` around lines 2748 -
2769, The current handling of mark_message_deferred_busy(...) treats any replay
with MessageStatus::DeferredBusy as success, which can hide partial-write
failures of turn_source_binding_ref, turn_reply_target_binding_ref, or
turn_idempotency_key; change this so we either (A) verify the full persisted
contract after mark_message_deferred_busy by loading the persisted
message/replay and asserting the binding fields match the sent
turn_source_binding_ref, turn_reply_target_binding_ref and
client_action_id/turn_idempotency_key (fail with Err if any mismatch/missing),
or (B) stop reconciling on status alone and propagate the original error instead
of returning Ok(()) — update the code around mark_message_deferred_busy,
reconcile_terminal_duplicate, and the predicate (currently |replay|
replay.status == MessageStatus::DeferredBusy) to include checks for those
binding fields so incomplete writes do not silently succeed.

Source: Coding guidelines

♻️ Duplicate comments (1)

crates/ironclaw_reborn_composition/src/runtime.rs (1)

6669-6758: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

This regression still bypasses the real deferred-busy caller.

The test reconstructs the deferred state with direct accept_inbound_message / submit_turn / mark_message_deferred_busy calls, so it can pass even if the shipped caller stops marking busy messages correctly. For this bug, the guard needs to drive the actual runtime/product entrypoint that produces DeferredBusy, then assert the drain resubmits it after the blocking run terminates. As per coding guidelines, "Test through the caller: when a helper gates a side effect, require a test driving the real call site (handler/factory/manager), not only the helper."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 6669 - 6758,
The test is rebuilding a deferred-busy state by calling
accept_inbound_message/submit_turn/mark_message_deferred_busy directly, which
bypasses the real caller that produces DeferredBusy; change the test to drive
the actual runtime entrypoint (the handler/factory/manager that normally handles
inbound turns and, on ThreadBusy, invokes mark_message_deferred_busy—see
inbound_turn.rs behavior) instead of calling mark_message_deferred_busy
directly, by submitting the second message through the same public entrypoint
that previously produced DeferredBusy via turn_coordinator/thread_service and
arranging a blocking/ongoing run (the first Accepted/Submit path using
turn_coordinator::submit_turn that returns ThreadBusy) so the drain observer
fires on A's terminal event and you can assert the drain resubmits B; ensure you
reference and use accept_inbound_message, turn_coordinator::submit_turn, and the
real inbound turn handler path rather than directly invoking
mark_message_deferred_busy in the test.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs`:
- Around line 1875-1907: The test writes both binding refs but only asserts the
source; add an assertion that result[0].turn_reply_target_binding_ref.as_deref()
equals Some("reply:binding-fs") (mirroring the existing turn_source_binding_ref
check) so the persistence contract verifies reply binding is stored; update the
same test that calls mark_message_deferred_busy(...) and
list_deferred_busy_messages(...) and use the existing result and expected_key
variables to locate where to insert this new assertion.

---

Outside diff comments:
In `@crates/ironclaw_product_workflow/src/reborn_services.rs`:
- Around line 2748-2769: The current handling of mark_message_deferred_busy(...)
treats any replay with MessageStatus::DeferredBusy as success, which can hide
partial-write failures of turn_source_binding_ref,
turn_reply_target_binding_ref, or turn_idempotency_key; change this so we either
(A) verify the full persisted contract after mark_message_deferred_busy by
loading the persisted message/replay and asserting the binding fields match the
sent turn_source_binding_ref, turn_reply_target_binding_ref and
client_action_id/turn_idempotency_key (fail with Err if any mismatch/missing),
or (B) stop reconciling on status alone and propagate the original error instead
of returning Ok(()) — update the code around mark_message_deferred_busy,
reconcile_terminal_duplicate, and the predicate (currently |replay|
replay.status == MessageStatus::DeferredBusy) to include checks for those
binding fields so incomplete writes do not silently succeed.

---

Duplicate comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 6669-6758: The test is rebuilding a deferred-busy state by calling
accept_inbound_message/submit_turn/mark_message_deferred_busy directly, which
bypasses the real caller that produces DeferredBusy; change the test to drive
the actual runtime entrypoint (the handler/factory/manager that normally handles
inbound turns and, on ThreadBusy, invokes mark_message_deferred_busy—see
inbound_turn.rs behavior) instead of calling mark_message_deferred_busy
directly, by submitting the second message through the same public entrypoint
that previously produced DeferredBusy via turn_coordinator/thread_service and
arranging a blocking/ongoing run (the first Accepted/Submit path using
turn_coordinator::submit_turn that returns ThreadBusy) so the drain observer
fires on A's terminal event and you can assert the drain resubmits B; ensure you
reference and use accept_inbound_message, turn_coordinator::submit_turn, and the
real inbound turn handler path rather than directly invoking
mark_message_deferred_busy in the test.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c8bca171-5e32-4d6e-b0d3-8f533b7475ab

📥 Commits

Reviewing files that changed from the base of the PR and between 0c3fd56 and d40728e.

📒 Files selected for processing (19)

• crates/ironclaw_loop_support/src/compaction_task.rs
• crates/ironclaw_loop_support/src/subagent_spawn_port/tests.rs
• crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs
• crates/ironclaw_product_workflow/src/inbound_turn.rs
• crates/ironclaw_product_workflow/src/reborn_services.rs
• crates/ironclaw_product_workflow/tests/reborn_services_contract.rs
• crates/ironclaw_reborn/src/loop_exit_applier/tests/mod.rs
• crates/ironclaw_reborn/src/subagent/completion_observer.rs
• crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs
• crates/ironclaw_reborn_composition/src/deferred_busy_drain/tests.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rs
• crates/ironclaw_threads/src/contract.rs
• crates/ironclaw_threads/src/filesystem_service.rs
• crates/ironclaw_threads/src/in_memory.rs
• crates/ironclaw_threads/src/service.rs
• crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs
• crates/ironclaw_threads/tests/session_thread_contract.rs
• crates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rs

[henrypark133]

Code Review (multi-agent)

Intent: Automatically drain DeferredBusy messages after a blocking run reaches terminal state so user messages are resubmitted instead of silently stranded.

Stats: 12 findings (from 14 raw, 12 after dedup/filter) across 7 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 4.

performance

1. High Marker tombstone can hide a concurrent deferred message (crates/ironclaw_threads/src/filesystem_service.rs:970-976, confidence 90) - anchor: crates/ironclaw_threads/src/filesystem_service.rs:970
   The filesystem backend deletes the thread-level deferred marker after an empty scan without proving the marker is still the same one it read before the scan. If a concurrent inbound path commits a new DeferredBusy message and refreshes the marker between the scan and this delete, the delete removes the fresh marker; future list_deferred_busy_messages calls then take the marker-absent fast path and never scan the now-deferred message.
2. Medium Drain limit still scans every thread message (crates/ironclaw_threads/src/filesystem_service.rs:945-955, confidence 82) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:146
   The drain requests limit=8, but the filesystem implementation first calls list_thread_messages, which lists and reads every message JSON for the thread, then filters, sorts, and truncates. On long threads with DeferredBusy present, every terminal event pays O(total thread messages) filesystem reads to drain at most one message; draining k queued messages becomes O(k*n) reads.

bugs

1. Medium Ownerless terminal runs drain the wrong thread scope (crates/ironclaw_reborn/src/thread_scope.rs:94-98, confidence 75) - anchor: crates/ironclaw_reborn/src/thread_scope.rs:94
   For TurnScope::new_with_owner(..., None), thread_owner is explicitly Ownerless, but derive_for_terminal_state treats it like ActorFallback and falls back to state.actor. The drain then looks under owner_user_id = actor instead of the ownerless thread scope where DeferredBusy messages were stored, so ownerless threads never drain after terminal state.

conventions

1. Medium Internal drain refs serialize on transcript records (crates/ironclaw_threads/src/contract.rs:135-150, confidence 100) - anchor: crates/ironclaw_threads/CLAUDE.md:7
   ThreadMessageRecord is documented as a UI/projection transcript snapshot, but the new internal drain replay refs use skip_serializing_if rather than the existing internal-metadata pattern at lines 124-127, so any direct transcript serialization outside RebornServices::get_timeline can expose routing refs and idempotency keys as ordinary transcript fields. This violates the threads guardrail to never expose raw runtime/private backend metadata as ordinary transcript content.
2. Medium Drain replay logic lives in composition (crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:144-147, confidence 75) (body only) - anchor: crates/ironclaw_product_workflow/CLAUDE.md:7
   The new composition module owns deferred-message selection, binding-ref replay, idempotency replay, and direct turn resubmission, but product_workflow is the documented owner for message staging, idempotency, and busy/deferred handling. The PR body explains why the original product envelope is not available, so treat this as design discussion unless paired with the concrete serialization/ownership follow-up.

tests

1. Medium Timeline scrub lacks idempotency-key coverage (crates/ironclaw_product_workflow/src/reborn_services.rs:3699-3705, confidence 100) - anchor: crates/ironclaw_product_workflow/src/reborn_services.rs:3704
   scrub_internal_timeline_refs clears turn_idempotency_key as internal drain metadata, but the existing facade and wire tests only cover source/reply refs and seed turn_idempotency_key as None. A regression that leaked persisted idempotency keys through RebornServices::get_timeline would not fail.
2. Medium Retry-accepted mark-submitted failure is untested (crates/ironclaw_product_workflow/src/inbound_turn.rs:680-693, confidence 75) - anchor: crates/ironclaw_product_workflow/src/inbound_turn.rs:689
   The post-defer retry Accepted branch propagates mark_message_submitted failures as ProductWorkflowError::Transient, but adjacent tests cover retry Accepted, retry ThreadBusy, and retry non-busy submit errors without exercising this mark-submitted error path.
3. Medium Malformed idempotency-key skip path is untested (crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:278-289, confidence 75) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:279
   The drain validates message.turn_idempotency_key and skips the record on parse failure, but tests only cover valid persisted keys and None fallback. A malformed stored key at the head of the deferred list could block later valid messages if this skip path regresses.
4. Medium Drain pagination lacks valid-second-window test (crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:409-418, confidence 75) - anchor: crates/ironclaw_reborn_composition/src/deferred_busy_drain/mod.rs:418
   The drain advances after a full window of invalid DeferredBusy records via after_sequence, but tests cover invalid+valid in one window and all-invalid-to-cap, not a valid record in the next window. A regression in after_sequence/window_last_sequence could strand messages after the first invalid page.

maintainability

1. Low Post-defer retry flow is duplicated across submit paths (crates/ironclaw_product_workflow/src/reborn_services.rs:1781-1848, confidence 75) (body only) - anchor: crates/ironclaw_product_workflow/src/reborn_services.rs:1789 duplicates crates/ironclaw_product_workflow/src/inbound_turn.rs:676
   The WebUI submit path duplicates the same protocol added in inbound_turn.rs: clone SubmitTurnRequest, mark DeferredBusy with canonical refs, retry once, mark submitted on accepted retry, return DeferredBusy on ThreadBusy, and warn on other retry errors. This retry is now part of the deferred-busy contract, so keeping two inline copies makes future changes to the race closure or idempotency behavior easy to drift.

local-patterns

1. Low Generic observer failures are reported as subagent completion (crates/ironclaw_reborn/src/runtime.rs:419-423, confidence 75) (body only) - anchor: crates/ironclaw_reborn/src/runtime.rs:419-423
   additional_required_observers can now contain non-subagent observers such as the deferred-busy drain, but subscription failures are still wrapped in DefaultPlannedRuntimeBuildError::SubagentCompletion, whose Display text says subagent completion wiring failed. Startup diagnostics point at the wrong subsystem when a generic lifecycle observer fails to subscribe.
2. Nit Field doc promises warn-level drain behavior (crates/ironclaw_threads/src/contract.rs:133-134, confidence 50) (body only) - anchor: crates/ironclaw_threads/src/contract.rs:133-134; .claude/rules/review-discipline.md
   The new turn_source_binding_ref doc says legacy None records are warned and skipped, but the drain module documents and implements non-poisoning failures at debug!. That stale wording can send readers looking for warning-level telemetry that will not exist.

[henrypark133]

High - Marker tombstone can hide a concurrent deferred message.

The filesystem backend deletes the thread-level deferred marker after an empty scan without proving the marker is still the same one it read before the scan. If a concurrent inbound path commits a new DeferredBusy message and refreshes the marker between the scan and this delete, the delete removes the fresh marker; future list_deferred_busy_messages calls then take the marker-absent fast path and never scan the now-deferred message.

Fix: Make marker deletion conditional on the marker version observed before the scan, or stop deleting this optimization marker and tolerate stale-marker extra scans.

Also flagged by: approach/Low

[henrypark133]

Approach withdrawn — after review we are dropping the defer-and-drain mechanism in favor of explicit gate-open feedback (no background resubmission). This PR is being closed; the branch stays for reference. Replacement CX lands in a follow-up PR.

[henrypark133]

Medium - Ownerless terminal runs drain the wrong thread scope.

For TurnScope::new_with_owner(..., None), thread_owner is explicitly Ownerless, but derive_for_terminal_state treats it like ActorFallback and falls back to state.actor. The drain then looks under owner_user_id = actor instead of the ownerless thread scope where DeferredBusy messages were stored, so ownerless threads never drain after terminal state.

Fix: Only fall back to the actor when the turn scope is not explicit; preserve None for explicit ownerless scopes.

[henrypark133]

Approach withdrawn — after review we are dropping the defer-and-drain mechanism in favor of explicit gate-open feedback (no background resubmission). This PR is being closed; the branch stays for reference. Replacement CX lands in a follow-up PR.

[henrypark133]

Medium - Drain limit still scans every thread message.

The drain requests limit=8, but the filesystem implementation first calls list_thread_messages, which lists and reads every message JSON for the thread, then filters, sorts, and truncates. On long threads with DeferredBusy present, every terminal event pays O(total thread messages) filesystem reads to drain at most one message; draining k queued messages becomes O(k*n) reads.

Fix: Maintain an indexed DeferredBusy queue or use the existing sequence index/ranged reads so list_deferred_busy_messages stops after the requested limit instead of materializing the whole thread.

[henrypark133]

Approach withdrawn — after review we are dropping the defer-and-drain mechanism in favor of explicit gate-open feedback (no background resubmission). This PR is being closed; the branch stays for reference. Replacement CX lands in a follow-up PR.

[henrypark133]

Medium - Internal drain refs serialize on transcript records.

ThreadMessageRecord is documented as a UI/projection transcript snapshot, but the new internal drain replay refs use skip_serializing_if rather than the existing internal-metadata pattern at lines 124-127, so any direct transcript serialization outside RebornServices::get_timeline can expose routing refs and idempotency keys as ordinary transcript fields. This violates the threads guardrail to never expose raw runtime/private backend metadata as ordinary transcript content.

Fix: Keep these fields off the public serialized ThreadMessageRecord shape and persist them through the private stored-record wrapper, like tool_result_provider_call.

[henrypark133]

Approach withdrawn — after review we are dropping the defer-and-drain mechanism in favor of explicit gate-open feedback (no background resubmission). This PR is being closed; the branch stays for reference. Replacement CX lands in a follow-up PR.

[henrypark133]

Medium - Timeline scrub lacks idempotency-key coverage.

scrub_internal_timeline_refs clears turn_idempotency_key as internal drain metadata, but the existing facade and wire tests only cover source/reply refs and seed turn_idempotency_key as None. A regression that leaked persisted idempotency keys through RebornServices::get_timeline would not fail.

Fix: tests::reborn_services_contract::get_timeline_scrubs_internal_idempotency_key_at_facade_boundary covering a DeferredBusy message marked with Some(turn_idempotency_key) and asserting the timeline record returns None

[henrypark133]

Approach withdrawn — after review we are dropping the defer-and-drain mechanism in favor of explicit gate-open feedback (no background resubmission). This PR is being closed; the branch stays for reference. Replacement CX lands in a follow-up PR.

[henrypark133]

Medium - Drain pagination lacks valid-second-window test.

The drain advances after a full window of invalid DeferredBusy records via after_sequence, but tests cover invalid+valid in one window and all-invalid-to-cap, not a valid record in the next window. A regression in after_sequence/window_last_sequence could strand messages after the first invalid page.

Fix: tests::deferred_busy_drain::drain_advances_past_full_invalid_window_and_submits_next covering first DRAIN_LIST_LIMIT records invalid and the next window containing a valid DeferredBusy message

[henrypark133]

Approach withdrawn — after review we are dropping the defer-and-drain mechanism in favor of explicit gate-open feedback (no background resubmission). This PR is being closed; the branch stays for reference. Replacement CX lands in a follow-up PR.

[henrypark133]

Medium - Malformed idempotency-key skip path is untested.

The drain validates message.turn_idempotency_key and skips the record on parse failure, but tests only cover valid persisted keys and None fallback. A malformed stored key at the head of the deferred list could block later valid messages if this skip path regresses.

Fix: tests::deferred_busy_drain::drain_skips_malformed_persisted_idempotency_key_and_submits_next covering malformed Some(turn_idempotency_key) followed by a valid deferred message

[henrypark133]

Approach withdrawn — after review we are dropping the defer-and-drain mechanism in favor of explicit gate-open feedback (no background resubmission). This PR is being closed; the branch stays for reference. Replacement CX lands in a follow-up PR.

[henrypark133]

Medium - Retry-accepted mark-submitted failure is untested.

The post-defer retry Accepted branch propagates mark_message_submitted failures as ProductWorkflowError::Transient, but adjacent tests cover retry Accepted, retry ThreadBusy, and retry non-busy submit errors without exercising this mark-submitted error path.

Fix: tests::inbound_turn_contract::defer_retry_accept_mark_submitted_failure_returns_transient covering retry Accepted followed by SessionThreadService::mark_message_submitted error

[henrypark133]

Approach withdrawn — after review we are dropping the defer-and-drain mechanism in favor of explicit gate-open feedback (no background resubmission). This PR is being closed; the branch stays for reference. Replacement CX lands in a follow-up PR.

[henrypark133]

Closing — design decision: we are dropping the defer-and-drain mechanism entirely. Messages arriving while a gate-blocking run holds the thread will get explicit gate-open feedback ("an approval gate is open; resolve it before continuing") instead of being parked and auto-resubmitted. Rationale: no background resubmission machinery in the agent loop — the user stays the retry actor; the entire replay-identity/idempotency/marker class of complexity disappears.

The branch (feat/deferred-busy-drain, through d40728e) stays pushed for reference: it contains the fully review-hardened drain (5 review passes, 64 comments resolved) should this ever be revisited.

Replacement CX: follow-up PR rendering the gate-open notice across channels, with no parking semantics.