Fix collapsed message timestamps in old sessions

[franksong2702]

Summary

This normalizes collapsed message timestamps in old Web UI sessions so long histories no longer render as if every message happened at the same second.

What changed

• add timestamp normalization for sessions whose message timestamps are all missing or effectively collapsed
• spread uniformly collapsed timestamps back into order when sessions are loaded/saved
• keep Session.load() read-only by default; direct session access opts into repair
• keep the fix isolated to session timestamp handling

Validation

• tests/test_session_timestamps.py
• tests/test_regressions.py
• targeted test run: 40 passed

Fixes #580

[franksong2702]

Sorry I mixed up the baseline and will fix and resubmit

[nesquena-hermes]

Code Review — PR #581

Thanks for the fix and the note about resubmitting. Here's a full review of the current state.

What looks good ✅

• normalize_message_timestamps() is clean, well-scoped, and handles both the "all collapsed" case (spread uniformly backward from anchor) and the "gaps only" case (fill in missing _ts/timestamp fields without touching existing ones).
• persist=False default on Session.load() is the right call — index building (_write_session_index) and bulk listing (all_sessions) should not trigger writes as a side effect.
• get_session() passes persist=True — the single interactive load path is the right place to trigger repair-on-open, so old sessions are transparently fixed the first time they're viewed.
• The test coverage is solid: uniform-collapse case and a full round-trip persist-and-reload test.

Issues to address before merge 🔍

1. _ts vs timestamp dual write — normalize_message_timestamps writes both timestamp and _ts on every changed message. If the existing session only has one of these fields, the normalizer may be silently adding a key that other parts of the code don't expect. Check whether the rest of the codebase treats _ts as a private/internal alias or as an authoritative field — if it's never written elsewhere, the dual write may create unexpected data.

2. The anchor_ts in save() — normalize_message_timestamps(self.messages, anchor_ts=self.updated_at or time.time()) is called on every save(). This means even a one-message session, or a session with already-correct timestamps, runs through the normalizer each save. For sessions with len(seen_ts) > 1 the function is a no-op, so it's cheap — but it's worth confirming the early-exit path handles the common case without touching messages.

3. test_session_load_repairs_uniform_message_timestamps session payload — the fixture does not include source_tag, metadata, or any other fields the Session constructor may have received in newer versions. If the constructor has required fields added since the tests were written, this will fail. Run the test suite against the current public branch to confirm.

4. Author's note about baseline — you mentioned you "mixed up the baseline." Can you clarify what was incorrect and what the resubmission will change? If there's a known issue with the current diff, a clean rebase before re-review would be helpful.

Summary

The approach is correct and the code is well-structured. Address the dual-write question and confirm the test passes cleanly against the current public branch before the resubmit, and this should be merge-ready.

[nesquena]

PR Review — Fix collapsed message timestamps in old sessions

Thanks for the contribution @franksong2702, and noted on your planned resubmit. Since you're iterating, here's a thorough review to fold into the next version.

Security Audit ✅

Clean — no injection vectors, no external resources, no secrets. Session.load() retains its path-traversal guard. normalize_message_timestamps() is pure dict manipulation.

Tests

• 1153 passed, 2 failed, 58 skipped.
• The 2 failures are pre-existing PyYAML issues on the develop baseline (already fixed on master in PR test: skip onboarding config tests when PyYAML unavailable #564) — not regressions from this PR.
• Your new tests/test_session_timestamps.py (2 tests) passes cleanly ✅
• CI green on all Python versions ✅

_ts vs timestamp — not a concern

I verified: the frontend (static/ui.js:1150) reads m._ts || m.timestamp, and static/messages.js writes _ts for streamed messages. Server-side streaming writes timestamp. Both fields are real and coexist — so the dual write is correct.

Design concerns (worth rethinking before resubmit) 🔍

1. Silent data mutation on every read of an old session

get_session() now passes persist=True, so opening any session with uniform timestamps triggers a file rewrite. This overwrites the original {"timestamp": X} values with synthesized ones on first load — irreversibly. The original data is lost.

Two problems with this:

• Users can't opt out or roll back
• If the normalize logic is ever wrong (see Hermes Web UI — Sprints 11-14: multi-provider models, settings, sessi… #2 below), the damage is permanent

Consider: the bug is a display concern. A display fix (frontend) is non-destructive. A persistence fix requires stronger justification.

2. Detection criterion (len(seen_ts) <= 1) has a false-positive problem

This triggers for any session where all messages share a timestamp, including legitimate cases:

• A short session where 3 messages were genuinely sent within the same second (rapid back-and-forth)
• A 2-message session where user pressed Enter twice quickly

The PR would rewrite these with a synthesized [now-N, ..., now] sequence, making timestamps less accurate than the truth. For a 3-message session at ts=1776138157, you'd get [155, 156, 157] — a fictitious 2-second spread that never happened.

For the true "old session" case (many messages, all stamped with session.created_at because the timestamp field wasn't set per-message), the fix works. But the detection can't distinguish "bug" from "legitimate same-second messages."

3. Backward-spreading from anchor_ts synthesizes false chronology

start_ts = base_ts - len(dict_messages) + 1 pushes timestamps into the recent past, relative to updated_at or time.time(). This doesn't reflect when messages actually arrived — it just makes them look like they happened a few seconds before the most recent save. For a 500-message session synthesized this way, messages will claim to span ~8 minutes right before the last save, which is fiction.

If the goal is "show a progression," a frontend-only fix that numbers messages (Message 1, Message 2, ...) or shows conversation position would be more honest than synthesizing fake timestamps.

4. normalize_message_timestamps runs on every save()

Even when it's a no-op (most common case: len(seen_ts) > 1), it walks all messages building seen_ts. For sessions with hundreds of messages, this is a small but real cost on every save. Consider an early exit: if any two adjacent messages have distinct timestamps, skip the normalizer entirely.

5. Anchor semantics on save() are surprising

On save() with touch_updated_at=True, self.updated_at = time.time() is set before normalize, so the anchor becomes now. If a session with collapsed timestamps gets saved (e.g., after any activity), its historical messages get synthesized as "just before now" — not as "just before their original save time." This is probably not intended.

Test coverage gaps

Only the fully-collapsed case is tested. Missing coverage:

• Partial collapse (4 messages, 3 same ts + 1 different) — should be a no-op under current logic, but worth asserting
• Messages with no timestamp at all (empty/missing field)
• Session with len(messages) < 2 early exit
• The "gaps only" path (mixed present/missing timestamps)
• Anchor smaller than message count (would produce timestamps ≤ 0)

Suggested alternative approach

Since this is fundamentally a display concern:

Option A — Frontend-only fix (non-destructive): In static/ui.js:1150, detect when all messages in a session share a timestamp and either:

• Hide per-message timestamps, showing a single session-level "conversation from " header
• Show position-in-conversation ("Message 1 of N") instead of time

Option B — Opt-in repair: Add a "Normalize timestamps" button on the session detail view that runs the fix once, visibly, with a confirmation. Users who want it can opt in; existing data is never silently rewritten.

Option C — Narrow the detection: Require at least N messages (say, 10) AND all with the exact same ts before triggering — this avoids the false-positive on genuine same-second short sessions.

Summary

The approach (persist=False default, targeted function, test file) is thoughtful and the code is well-structured. The concerns are about the broader design: silent data mutation, false-positive detection on legitimate same-second messages, and fabricating chronology that never existed. A display-layer fix would address the root user complaint with less risk.

Happy to dig further on any of these on the resubmit. Looking forward to v2!

[franksong2702]

I want to sync with the team on timestamp behavior before we proceed.

Current behavior (as observed in code) still has a consistency gap:

• static/ui.js uses one time source for rendering: m._ts || m.timestamp.
• Timestamp UI is still user-centric in practice: footer time is rendered only for user messages via userTimeHtml; assistant messages do not get the same row-level timestamp treatment.
• CSS keeps user footer hidden by default (opacity: 0) and shows it on hover/focus (msg-row[data-role="user"]:hover), so time is not a stable visible affordance.
• Server-side done-path currently stamps missing timestamps on eligible messages (api/streaming.py), so visibility can look inconsistent across older history vs newer turns.

So from UX side this is not symmetric:

• user + assistant are not shown with the same timestamp policy,
• and hover-dependent visibility means "time exists" does not always mean "time is reliably visible".

@nesquena-hermes @aronprins I suggest we align this before adding more timestamp polish:

1. show message time for both user + assistant in transcript consistently;
2. use message-generated time as the display anchor;
3. apply a single, predictable visibility rule.

[franksong2702]

Related follow-up: #717

I linked this because #581 and #680 are clearly in the same timestamp/problem family, but they are not exactly the same rendering path.

The new PR follows the current owner direction from #680:

• assistant footer timestamps are now rendered
• older messages show a fuller date+time in the footer
• unchanged historical messages preserve their original timestamps instead of being re-stamped to the latest reply time

So #581 remains useful prior context, while #717 is the current implementation path aligned to #680.

[aronprins]

Replying to this comment.

From a UI/UX perspective, I think the right direction is:

• align timestamp behavior for both user and assistant messages
• preserve recorded timestamps for unchanged history
• avoid synthesizing or persisting invented per-message times for legacy sessions

#581 improves readability, but it does it by mutating saved history and creating false precision. If a legacy session recorded every message with the same timestamp, that is incomplete historical data. We should handle that in the UI, not rewrite it into a believable but synthetic chronology.

So my recommendation is:

1. Continue with the #717 / #680 direction: render assistant timestamps, use one consistent visibility rule, and preserve existing timestamps for unchanged messages.
2. Do not auto-normalize and persist synthetic timestamps on load/save.
3. For legacy sessions with collapsed timestamps, use a UI fallback instead. For example:
   • show a single session-level note/header like Legacy session: per-message times were not recorded accurately
   • or suppress per-message times for that session and show one conversation-level timestamp
   • or otherwise de-emphasize identical repeated times without inventing seconds

UX principle here: display what we know, preserve what was actually recorded, and do not invent precision just to make the transcript look cleaner.

One additional preference from my side: if timestamps are part of the transcript affordance, they should follow the same rule for both roles. I would lean toward always-visible but visually quiet, rather than hover-only for one side and absent for the other.

[nesquena-hermes]

Thanks for the follow-up questions — good instincts to raise them. Here is the direction:

Keep this PR narrow. The goal is to fix the specific regression: timestamps not showing correctly in collapsed/old sessions. That is the right scope for this PR.

The broader questions (should assistant messages always show timestamps, hover-only vs always-visible, CSS opacity behavior) are valid UX considerations but belong in a separate issue or PR. Mixing them here would make this harder to review and merge.

Concrete ask: make sure the collapsed-session timestamp display is correct and nothing is regressed in normal (non-collapsed) sessions. Once that is solid, this is ready to merge.

Let us know if you need any clarification on what the expected behavior should be and we can spec it out.

[franksong2702]

I prefer to close this PR and follow it up pr 717.