Skip to content

feat(session): add per-message created_at timestamps#2788

Open
LiusCraft wants to merge 5 commits into
sipeed:mainfrom
LiusCraft:feat/session-message-timestamps
Open

feat(session): add per-message created_at timestamps#2788
LiusCraft wants to merge 5 commits into
sipeed:mainfrom
LiusCraft:feat/session-message-timestamps

Conversation

@LiusCraft
Copy link
Copy Markdown
Contributor

@LiusCraft LiusCraft commented May 6, 2026

Background

Session API (GET /api/sessions/{id}) returns messages without individual timestamps. The frontend has to use the session-level updated time for all messages, which is inaccurate when messages span different times.

Changes

image

Backend

  • pkg/providers/protocoltypes/types.go — Added CreatedAt time.Time field to Message struct with json:"created_at,omitempty" for backward compatibility
  • pkg/memory/jsonl.go — Set CreatedAt = time.Now() in addMsg when the field is zero (after transient message check), so every stored message gets a timestamp
  • web/backend/api/session.go — Expose created_at in sessionChatMessage API response; pass through from providers.Message to all transcript message types (user, assistant, thought, tool_calls, visible tool messages); fallback to session updated time for legacy messages with zero CreatedAt
  • pkg/memory/jsonl_test.go — Added TestAddMessage_SetsCreatedAt verifying timestamps are set on write

Frontend

  • src/api/sessions.ts — Added created_at?: string to SessionDetail.messages type
  • src/features/chat/history.ts — Use message.created_at when available (message.created_at ?? detail.updated)
  • src/components/chat/user-message.tsx — Added timestamp prop and displays formatted time below message bubble
  • src/components/chat/assistant-message.tsx — Show timestamp in collapsed block header (thought, tool_calls) — previously only shown for normal assistant messages
  • src/components/chat/chat-page.tsx — Pass timestamp to UserMessage

Behavior Impact

  • New messages written after this change will have accurate per-message created_at timestamps in API responses
  • Legacy messages (existing JSONL lines without created_at) will use the session updated time as fallback — no more 0001-01-01T00:00:00Z
  • All message types now display timestamps: user, assistant, thought, tool_calls
  • No change to LLM provider communication (Message is never directly serialized to provider APIs)

Risks and Rollback

  • Risk: Legacy JSONL lines lack the field — handled by omitempty, IsZero() guard, and backend fallback to session.updated
  • Rollback: Revert the commits, old behavior is fully preserved

Verification

  • go test ./pkg/memory/ — 44/44 PASS
  • go test ./web/backend/api/ — all PASS
  • go build ./pkg/providers/protocoltypes/ ./pkg/memory/ ./web/backend/api/ — all pass
  • Frontend build — pnpm build:backend passes

Notes

Fixes #2787

Comment thread pkg/memory/jsonl.go Outdated
return nil
}
if msg.CreatedAt.IsZero() {
msg.CreatedAt = time.Now()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not optimal to assigns time.Now() before acquiring the per-session lock. Two concurrent writers can therefore persist messages in an order that does not match their timestamps, which breaks chronology and any merge/sort logic that trusts created_at. Capture one now only after the lock is held, then use it for both msg.CreatedAt and meta.UpdatedAt.

Comment thread pkg/providers/protocoltypes/types.go Outdated
type Message struct {
Role string `json:"role"`
Content string `json:"content"`
CreatedAt time.Time `json:"created_at,omitempty"`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using omitempty on a time.Time field does not omit zero values with Go's encoding/json. Any uninitialized message written through SessionManager or rewriteJSONL will be serialized as 0001-01-01T00:00:00Z, which is garbage data masquerading as a real timestamp. Use *time.Time for an optional field, or guarantee population on every marshal path and add raw-JSON tests to prove zero timestamps are never written.

@LiusCraft
Copy link
Copy Markdown
Contributor Author

Thanks for the review! I've addressed all the feedback — @afjcjsbx could you take another look when you get a chance?

Comment thread pkg/memory/jsonl.go

now := time.Now()
if msg.CreatedAt == nil {
msg.CreatedAt = &now
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using the JSONL backend, this timestamp is added only to the local copy passed into addMsg; callers such as turnState.recordPersistedMessage still keep the same message with CreatedAt == nil. After a mid-turn restore-point refresh (for example context-window retry compression), refreshRestorePointFromSession reads the persisted messages back with non-nil CreatedAt and compares them to the recorded messages with reflect.DeepEqual, so the current turn tail no longer matches and gets included in the rollback snapshot; aborting that turn can then leave partially persisted current-turn messages in history.

Copy link
Copy Markdown
Contributor Author

@LiusCraft LiusCraft May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexhoshina The CreatedAt normalization (setting to nil on both sides before DeepEqual) in refreshRestorePointFromSession fixes the specific regression this PR introduced. However, after deeper analysis, there is a pre-existing issue with the same tail-matching logic: fields tagged with json:"-" — such as ToolCall.Name, ToolCall.Arguments, and Message.PromptLayer — are also lost during the JSONL roundtrip, so reflect.DeepEqual would still mismatch when comparing an in-memory persisted message (which has these fields populated) with a deserialized history message (which does not).

This means the tail-matching heuristic is fundamentally fragile — it breaks on any field that differs between the in-memory and serialized representations, not just CreatedAt.

I think the proper long-term fix is to introduce a unique MessageID so we can use set-subtraction instead of content-based DeepEqual matching. For now, the CreatedAt nil normalization is sufficient to fix this PR's regression. The json:"-" issue is pre-existing and unrelated to this PR — I'll file a separate PR to address it.

@LiusCraft
Copy link
Copy Markdown
Contributor Author

Thanks for the reviews! Both issues from the initial review have been addressed in commit 409206f0:

  1. Concurrency: time.Now() is now called after acquiring the per-session lock — two concurrent writers will get consistent ordering.
  2. omitempty: Changed CreatedAt from time.Time to *time.Time, so zero values are properly omitted during JSON marshaling.

Regarding the DeepEqual tail-matching regression raised by @alexhoshinaCreatedAt is now normalized to nil on both sides before reflect.DeepEqual, which resolves the mismatch. The pre-existing issue with `json:"-"" fields is unrelated to this PR and will be handled separately.

@afjcjsbx @alexhoshina could you take another look when you get a chance? The blocking CHANGES_REQUESTED is still pending. Thanks!

@afjcjsbx
Copy link
Copy Markdown
Collaborator

The main problem here is not the timestamp itself, but its ownership.

With this patch CreatedAt is not guaranteed by the persistence layer: it is populated manually at various runtime call sites (pipeline_*, prompt_turn, steering), while AddFullMessage continues to accept and persist messages with CreatedAt == nil.

This makes the invariant fragile: it only takes a future AddFullMessage without a timestamp to revert to writing history partially without created_at, at which point the backend and frontend "fix" it differently (handleGetSession uses sess.Updated, the frontend fallbacks to detail.updated). It is dangerous because history merge uses timestamp in signature/ordering, so we can introduce duplicate or inconsistent reorder after reload/hydration.

In my opinion the correct fix is to move the responsibility to the persistence boundary:

  • normalize CreatedAt in the store when it is missing
  • do it at least in JSONLStore.addMsg
  • ideally also in SetHistory
  • ideally also align the SessionManager fallback, so the behavior does not depend on the active backend

Call sites should set CreatedAt only when they want to preserve a real event time; they should not be the ones to guarantee the persistence invariant.

I would also ask for specific tests on AddFullMessage, SetHistory, and API transcripts, because currently new coverage only tests AddMessage, which is not the main path used by the runtime.

- Persistence layer (jsonl.go addMsg/SetHistory) normalizes CreatedAt
  when missing so the invariant is guaranteed at the storage boundary
- API layer (session.go) exposes created_at on all transcript message
  types with session.updated fallback for legacy messages
- Frontend uses per-message timestamps when available
- messagesContentEqual ignores CreatedAt for tail-matching after
  JSONL roundtrip

Fixes sipeed#2787
@LiusCraft LiusCraft force-pushed the feat/session-message-timestamps branch from 8d0614b to 80f6e2b Compare May 10, 2026 16:45
@LiusCraft LiusCraft force-pushed the feat/session-message-timestamps branch from d64a366 to 1674911 Compare May 10, 2026 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Session messages lack individual timestamps — all messages share session.updated time

3 participants