fix(reborn): persist chat timeline timestamps - #5764
Conversation
italic-jinxin
added
size: L
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThreadMessageRecord gains optional durable ChangesDurable message timestamps
Estimated code review effort: 4 (Complex) | ~60 minutes Async Test Stack Wrapper
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude review |
github-actions
Bot
added
size: M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces durable server-side timestamps (created_at and updated_at) to ThreadMessageRecord to track when messages are first persisted and when they are materially changed, such as when a draft is finalized. These fields are integrated into both the filesystem and in-memory session thread services, and the frontend chat UI is updated to prefer these durable timestamps when rendering message times. There are no review comments, so I have no feedback to provide.
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.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 045a60cdb010edafa6f2ee4045f35435cb2a8d34
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete, actionable issues found in the timestamp persistence and WebUI timeline rendering changes.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/stale/stalled state while reviewers run.
This comment was marked as resolved.
This comment was marked as resolved.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.16% — 282265 / 331443 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (4 entry/entries excluded from the accounting above)
|
github-actions
Bot
added
size: XL
|
🚅 Deployed to the ironclaw-pr-5764 environment in ironclaw-ci-preview
|
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 59003fbec4e32a84e850546f16b1e162e46fc21a
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete blocking issues found in the reviewed diff. The change adds durable message timestamps across in-memory/filesystem thread services and consumes them in WebUI history rendering with focused contract/unit coverage.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/stale/stalled state while reviewers run.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/filesystem_service.rs (1)
1144-1173: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
touch_thread_updated_atshould never write an older stamp.list_threads_for_scopeorders bythread.updated_at, so unconditionally storing the caller-capturednowcan overwrite a later concurrent activity and demote a freshly-active thread. Guard with>=/maxbefore the CAS write to keep the stamp advance-only and skip the no-op retry.🤖 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/filesystem_service.rs` around lines 1144 - 1173, The touch_thread_updated_at path is unconditionally persisting the caller’s updated_at value, which can overwrite a newer timestamp from concurrent activity. In touch_thread_updated_at, compare the current stored record.updated_at against the incoming DateTime<Utc> and only write when the new value is greater, otherwise keep the newer value or return early as a no-op. Preserve the existing CAS retry flow around read_thread_versioned and put_with_cas, but ensure thread_entry is built from the max/advance-only timestamp so list_threads_for_scope ordering never regresses.
🤖 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/src/contract.rs`:
- Around line 234-269: The timestamp validators in
validate_new_message_timestamps and validate_message_timestamps_not_cleared
currently return SessionThreadError::Backend(String), which hides a specific
failure mode behind a generic string. Add a typed SessionThreadError variant for
timestamp validation failures (for example, with fields like context,
message_id, and a reason enum) and update both validators to return that variant
instead of formatting a Backend string. Then adjust any call sites/tests that
currently match Backend(_) so they assert on the new structured error kind
directly.
In `@crates/ironclaw_webui_v2/static/js/pages/chat/lib/history-messages.test.mjs`:
- Around line 177-231: Add a test in messagesFromTimeline that exercises the
highest-priority record.received_at timestamp path, since the current cases only
cover created_at and updated_at fallbacks. Use the existing messagesFromTimeline
test suite in history-messages.test.mjs and assert that when a timeline record
includes received_at, the resulting message.timestamp uses it over the other
timestamp fields. Keep the test aligned with the existing
user/assistant/tool_activity coverage so it clearly validates the received_at
precedence branch.
---
Outside diff comments:
In `@crates/ironclaw_threads/src/filesystem_service.rs`:
- Around line 1144-1173: The touch_thread_updated_at path is unconditionally
persisting the caller’s updated_at value, which can overwrite a newer timestamp
from concurrent activity. In touch_thread_updated_at, compare the current stored
record.updated_at against the incoming DateTime<Utc> and only write when the new
value is greater, otherwise keep the newer value or return early as a no-op.
Preserve the existing CAS retry flow around read_thread_versioned and
put_with_cas, but ensure thread_entry is built from the max/advance-only
timestamp so list_threads_for_scope ordering never regresses.
🪄 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: bee24a9a-50ad-440c-84a2-58309367e42b
📒 Files selected for processing (12)
crates/ironclaw_loop_support/src/compaction_task.rscrates/ironclaw_product_workflow/src/inbound_turn.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/mod.rscrates/ironclaw_reborn_composition/src/observability/trace_capture.rscrates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_threads/tests/filesystem_session_thread_contract.rscrates/ironclaw_threads/tests/session_thread_contract.rscrates/ironclaw_webui_v2/static/js/pages/chat/lib/history-messages.jscrates/ironclaw_webui_v2/static/js/pages/chat/lib/history-messages.test.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/error.rs`:
- Around line 8-22: The TimestampViolation error type is using a manual Display
implementation instead of the standard error derive used elsewhere in error.rs.
Update TimestampViolation to follow the same thiserror pattern as the other
error types in this module, removing the hand-rolled fmt::Display and deriving
the error trait with appropriate messages on each variant. Use the existing
TimestampViolation enum in error.rs as the location to make the consistency
change.
🪄 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: 9eece944-2a56-40d2-bf80-b20291e4a22f
📒 Files selected for processing (3)
crates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/error.rscrates/ironclaw_webui_v2/static/js/pages/chat/lib/history-messages.test.mjs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/outbound_target.rs (1)
358-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree identical copies of
run_async_test_with_stack— hoist to shared test support.This helper is now byte-for-byte identical across
tests/reborn_qa_smoke_scenarios_e2e.rs,tests/integration/outbound_target.rs, andtests/integration/skill_activate.rs. The signature divergence flagged in the prior review was fixed, but the duplication itself remains. Per coding guidelines, extract helpers when logic is reused.♻️ Hoist into a shared test-support module
Move the helper into a shared crate (e.g.,
ironclaw_test_supportor atests/common/module) and import it from all three call sites:-fn run_async_test_with_stack<F, Fut>(name: &'static str, test: F) -where - F: FnOnce() -> Fut + Send + 'static, - Fut: std::future::Future<Output = ()> + 'static, -{ - let handle = std::thread::Builder::new() - .name(name.to_string()) - .stack_size(16 * 1024 * 1024) - .spawn(move || { - tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .expect("tokio test runtime") - .block_on(test()); - }) - .expect("spawn stack-sized test thread"); - if let Err(panic) = handle.join() { - std::panic::resume_unwind(panic); - } -} +// Import from shared test support instead. +use ironclaw_test_support::run_async_test_with_stack;🤖 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 `@tests/integration/outbound_target.rs` around lines 358 - 377, The run_async_test_with_stack helper is duplicated across multiple test files, so move the shared implementation into a common test-support module and reuse it from each call site. Keep the existing behavior in the helper, but reference the shared function from tests/reborn_qa_smoke_scenarios_e2e.rs, tests/integration/outbound_target.rs, and tests/integration/skill_activate.rs instead of maintaining separate copies; use the run_async_test_with_stack symbol as the single source of truth.
🤖 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 `@tests/integration/outbound_target.rs`:
- Around line 358-377: The run_async_test_with_stack helper is duplicated across
multiple test files, so move the shared implementation into a common
test-support module and reuse it from each call site. Keep the existing behavior
in the helper, but reference the shared function from
tests/reborn_qa_smoke_scenarios_e2e.rs, tests/integration/outbound_target.rs,
and tests/integration/skill_activate.rs instead of maintaining separate copies;
use the run_async_test_with_stack symbol as the single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ffe7f472-f713-4391-91a0-78cae19dee1b
📒 Files selected for processing (2)
tests/integration/outbound_target.rstests/integration/skill_activate.rs
# Conflicts: # crates/ironclaw_reborn_composition/tests/trigger_poller_e2e.rs
This branch was successfully deployed
3 participants
Summary
created_at/updated_attimestamps to Reborn thread timeline records.updated_at.Linked Issue
Closes #3535
Validation
cargo fmt --all -- --checkgit diff --checkcargo test -p ironclaw_threadsnode --test crates/ironclaw_webui_v2/static/js/pages/chat/lib/history-messages.test.mjscargo test -p ironclaw_webui_v2 chat_message_grouping_hoists_only_final_replies --features webui-v2-betaSecurity Impact
No auth, secret, network, or sandbox behavior changes.
Database Impact
No schema or migration changes. Filesystem transcript JSON gains optional timestamp fields; legacy rows deserialize with
None.Blast Radius
Limited to Reborn thread timeline records and WebChat timestamp presentation, plus test/helper construction sites for
ThreadMessageRecord.Rollback Plan
Revert this PR to return timeline messages to timestamp-less records and prior WebUI timestamp fallback behavior.