spec: per-message timestamps in Agent Mode (#9889)#10128
spec: per-message timestamps in Agent Mode (#9889)#10128lonexreb wants to merge 5 commits intowarpdotdev:masterfrom
Conversation
Adds product.md + tech.md for issue warpdotdev#9889: Agent Mode shows the conversation as a sequence of prompts and responses with no temporal information. Users cannot tell when a prompt was submitted, how long the agent took, or how recent any given exchange is. Investigation: the data already exists at the model layer. AIAgentExchange carries derived start_ts and completed_ts (app/src/ai/agent/conversation.rs:3218-3219), with helpers at lines 538-567 deriving them from message timestamps and from AIAgentContext::CurrentTime on the user input. Only the view layer omits this information. Spec proposes: - Pure derivation helper at the agent_view layer (no new state on the model) reusing the existing model-level helpers. - Three new widgets (TimestampLabel, DurationLabel, ProgressDurationLabel) added inline in the existing bubble metadata row, not in a new gutter — leverages existing density. - Single shared 1Hz timer (TimestampTickService) for all in-progress duration counters, gated to "panel visible AND in-progress exchange present" so an idle long conversation costs nothing per second. - Format auto-promotion ("just now" -> "Xm ago" -> "HH:MM" -> "Day HH:MM" -> "YYYY-MM-DD HH:MM") via a 30s tick. - Hover tooltip with ISO 8601 disambiguator for users who need exact timing. - New setting agents.show_message_timestamps (default true, SyncToCloud::Always); when false the widgets are not added to the layout (zero off-state cost). - Missing-timestamp fallback to em-dash with a once-per-exchange warn log so production data gaps are spotted. - Cancellation handling: "cancelled at HH:MM • Xs." Test plan covers five data-derivation unit tests, six format unit tests, and eight integration tests including a snapshot test of the off-state and a clock-advance test for format auto-promotion. Four open questions for maintainer review on the default, inline vs gutter rendering, tick service lifetime, and tooltip format.
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This spec defines product behavior and a technical plan for per-message timestamps in Agent Mode. The product goals are clear, but the tech spec currently relies on incorrect model references and targets the wrong UI integration point, which would lead implementation away from the actual conversation rendering path.
Concerns
- The data-source section names fields and locations that do not match the checked-out codebase, so the implementation plan needs to be corrected before coding.
- The UI integration section points at the Agent Message Bar even though conversation prompt/response rendering lives elsewhere.
- Product formatting promises locale-preferred time display, while the tech spec hard-codes English 12-hour
chrono::formatstrings.
Security
- The new opt-out telemetry should explicitly document that no timestamp values, conversation IDs, or message content are emitted and that existing telemetry privacy controls apply.
Verdict
Found: 0 critical, 3 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| ## What already exists | ||
|
|
||
| - `AIAgentExchange` carries a `start_ts: Option<DateTime<Local>>` |
There was a problem hiding this comment.
AIAgentExchange is defined in app/src/ai/agent/mod.rs with start_time and finish_time; the referenced conversation.rs:3218-3219 lines are SerializedBlock fields. Update the data-model references and derivation plan so implementers do not target nonexistent start_ts / completed_ts exchange fields.
| Per product.md §risk 1 recommendation: **inline in the existing | ||
| message-bubble metadata row**. | ||
|
|
||
| For the user prompt bubble (in `agent_view/agent_message_bar.rs`): |
There was a problem hiding this comment.
agent_view/agent_message_bar.rs renders the bottom status/input message bar, not the conversation prompt bubble. Point the spec at the actual prompt and response renderers (for example ai/blocklist/block/view_impl/query.rs and the corresponding response block renderer) before implementation.
| match delta { | ||
| d if d < ChronoDuration::minutes(1) => "just now".into(), | ||
| d if d < ChronoDuration::hours(1) => format!("{}m ago", d.num_minutes()), | ||
| d if d < ChronoDuration::days(1) => ts.format("%-I:%M %p").to_string(), |
There was a problem hiding this comment.
| with maintainers before implementation; this is the most | ||
| reversible decision. | ||
|
|
||
| 5. **Telemetry.** Add a one-time `setting_changed` event when the |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Define the setting_changed payload and privacy guardrails here: it should not include timestamp values, conversation IDs, message content, or bypass existing telemetry opt-out controls.
Bot review (warpdotdev#10128) identified: - The data-source section named fields and locations that do not exist in the codebase (start_ts/completed_ts at conversation.rs lines 3218-3219 — those are unrelated SerializedBlock fields). - The UI integration section pointed at agent_message_bar.rs which renders the bottom input bar, not the per-exchange prompt/response bubbles. - The hard-coded English 12-hour format string contradicted product B2's locale-preferred-format requirement. - Missing privacy guardrails for the new opt-out telemetry. Fixes: - Correct facts on AIAgentExchange (defined in app/src/ai/agent/mod.rs:2835 with start_time: DateTime<Local> and finish_time: Option<DateTime<Local>>). - Cite the right helpers: Conversation::start_time_from_exchange_messages at conversation.rs:556, finish_time_from_exchange_messages at conversation.rs:536, and recompute call sites at 1776/1894/1961. - Point UI integration at the correct render sites: app/src/ai/blocklist/block/view_impl/query.rs::render_query for the prompt bubble, view_impl/output.rs for the response bubble. Move all new files under view_impl/ alongside the consumers; explicitly call out that agent_message_bar.rs is NOT modified. - Replace English 12-hour format with a ClockFormat struct sourced from the OS at startup (NSLocale on mac, LOCALE_ITIME on Windows, LC_TIME/LANG on Linux). Use chrono::format_localized for weekday names. T8/T9 are duplicated as T8b/T9b for non-English/24-hour locales. - Drop submitted_at fallback (start_time is always present). Keep the finish_time inconsistency-state warn. - Add explicit telemetry privacy guardrails to product.md §Risk 5: payload shape, no IDs/timestamp values/message content, respect global telemetry opt-out, no new server-side channels. Each correction includes a "Correction (review warpdotdev#10128)" callout so reviewers can cross-reference the bot's concerns.
|
Pushed corrections addressing all 4 bot review concerns:
Each correction includes a "Correction (review #10128)" callout in the spec so future reviewers can cross-reference. Ready for re-review. /oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines user-facing behavior and a technical approach for per-message timestamps in Agent Mode. The product scope is generally aligned with the linked issue, and the security/privacy surface is limited by explicit telemetry guardrails.
Concerns
- The technical spec incorrectly states that both prompt and response render paths already receive an exchange or exchange id; the prompt path needs explicit data plumbing before it can bind a submitted-at timestamp.
- The proposed locale-aware formatter depends on
chrono::Locale/format_localized, but the workspace chrono dependency does not enable the required locale feature. - The missing-timestamp fallback is inconsistent with the current model paths, which synthesize
Local::now()for missing finish timestamps and would bypass the dash fallback required by the product spec. - No additional security findings.
Verdict
Found: 0 critical, 3 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| bound to `submitted_at` plus the shared 1Hz tick instead. | ||
|
|
||
| Both bubbles already receive an `AIAgentExchange` (or its id) from | ||
| the parent block, so neither integration requires plumbing new data |
There was a problem hiding this comment.
query::Props / render_query currently receive only query text and input_index; specify the required plumbing from AIBlock/AIBlockModel::exchange_id so the prompt timestamp can actually bind to an exchange.
| /// Locale tag (e.g. "en-US", "de-DE") used by chrono's locale-aware | ||
| /// formatters for weekday names. Sourced from the OS at startup; not | ||
| /// re-read on every tick. | ||
| locale: chrono::Locale, |
There was a problem hiding this comment.
chrono::Locale and format_localized require chrono's locale feature, but the workspace dependency only enables serde; specify the dependency feature change or choose a formatter that compiles with current features.
| only fires for `completed_at`, which is `Option<DateTime<Local>>`: | ||
|
|
||
| - If the exchange is not in progress (`output_status` reports | ||
| finished/errored/cancelled) AND `finish_time` is `None`, the model |
There was a problem hiding this comment.
finish_time to Local::now() when message timestamps are missing, so this finish_time is None fallback would not satisfy B7 and could display a fabricated time; specify changing that model fallback or deriving from raw messages before display.
| - Per-token / per-tool-call timestamps. | ||
| - Cumulative agent CPU time / credits per exchange (overlaps | ||
| #10000, #10052). | ||
| - Time-zone selection and 24h vs 12h preference. |
There was a problem hiding this comment.
💡 [SUGGESTION] This conflicts with the formatter section that explicitly implements and tests 24h-vs-12h preferences; keep time-zone selection out of scope but remove 24h-vs-12h preference from follow-ups.
Round-2 bot review identified 3 important concerns the previous
corrections introduced:
- The prompt path doesn't actually receive an AIAgentExchange — I
claimed it did. render_query in view_impl/query.rs takes
primitive parameters via Props (query: &str, user_display_name,
attachments, etc.) and the timestamp must be plumbed through.
- The proposed locale-aware formatter depended on chrono::Locale
and format_localized, but the workspace chrono dependency
(Cargo.toml: chrono = { version = "0.4.38", features = ["serde"]
}) does not enable the unstable-locales feature these APIs need.
- The missing-timestamp dash fallback can never fire — the model's
recompute call sites at conversation.rs:1777/1895/1962 do
finish_time_from_exchange_messages(...).unwrap_or_else(Local::now),
so finished exchanges always have finish_time: Some(...).
Fixes:
- UI integration: explicit plumbing plan for the prompt path —
add submitted_at: DateTime<Local> to view_impl/query.rs::Props
and the props builder. The parent block has AIAgentExchangeId
context and reads exchange.start_time from the model. One field
on each of two structs, no new layer.
- Format auto-promotion: drop chrono::Locale / format_localized.
V1 keeps English weekday names ("Mon", "Tue") which chrono's
default %a formatter provides without unstable-locales. The
12h-vs-24h preference is read from the OS — that's the more
impactful localization knob and works without the feature flag.
Localized weekday names called out as an explicit V2 follow-up
with two implementation options.
- Missing-timestamp fallback: the dash glyph is now scoped to two
defensive-guard cases (finished exchange with finish_time: None
due to a hypothetical state-machine bug; or a future schema
change that makes start_time optional). Both fire warn logs at
most once per exchange. The happy path uses
ProgressDurationLabel for in-progress and TimestampLabel for
finished — no dash in normal operation.
- Tests: T8b/T9b updated to test 24h format with English weekdays
(instead of German weekdays) since V1 doesn't localize day
names.
Each correction includes a "Correction (re-review warpdotdev#10128)" callout.
|
Round-2 corrections pushed (commit 523d7f2):
/oz-review |
There was a problem hiding this comment.
Overview
This spec defines product and technical behavior for showing per-message timestamps in Agent Mode, including formatting, setting gates, restoration behavior, timer strategy, and tests.
Concerns
- The product and tech specs conflict on missing timestamp behavior: product acceptance requires an em dash, while the tech design renders synthesized
Local::now()values for restored conversations with lost timestamps. - The proposed setting uses a non-existent
SyncToCloud::Alwaysmode, leaving implementation and cloud-sync privacy semantics unspecified. - The tech spec simultaneously implements OS-derived 12h/24h formatting and lists 12h vs 24h preference as out of scope.
Verdict
Found: 1 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| recompute paths above, it is: | ||
| - `None` exactly while the exchange is in progress. | ||
| - `Some(...)` once the exchange transitions out of in-progress — | ||
| including `Some(Local::now())` synthesized for restored |
There was a problem hiding this comment.
🚨 [CRITICAL] This contradicts product.md B7/A7 and IT7: rendering Some(Local::now()) for lost stored timestamps shows a fabricated load-time completion instead of —. Define a path that detects the synthesized fallback, or change the product acceptance/tests to match the tech behavior.
| show_message_timestamps: bool, | ||
| AISettings, | ||
| SupportedPlatforms::DESKTOP, | ||
| SyncToCloud::Always, |
There was a problem hiding this comment.
SyncToCloud::Always is not a valid settings sync mode in this repo; existing settings use Globally(RespectUserSyncSetting::Yes/No), PerPlatform(...), or Never. Pick the exact mode so the implementation and privacy semantics are unambiguous.
| - Per-token / per-tool-call timestamps. | ||
| - Cumulative agent CPU time / credits per exchange (overlaps | ||
| #10000, #10052). | ||
| - Time-zone selection and 24h vs 12h preference. |
There was a problem hiding this comment.
… (round 3) Round-3 bot review identified 1 critical + 2 important concerns: - B7/A7 promised an em-dash for missing-timestamp restored conversations, but the model already synthesizes Local::now() via finish_time_from_exchange_messages(...).unwrap_or_else(...) at conversation.rs:1777/1895/1962. Promise contradicts model. - Setting cited SyncToCloud::Always — that variant does NOT exist. Real variants in cloud_preferences.rs are Never, Globally(...), PerPlatform(...). Right value here is Globally(RespectUserSyncSetting::Yes), matching every UI preference in code.rs. - Non-goals listed "24h vs 12h preference" as out-of-scope while tech implements OS-derived 12h/24h formatting. Resolved as: *time-zone* selection out, but 12h/24h preference IN scope (read from OS, not a user-facing setting in V1). Fixes: - B7 rewritten as a defensive-guard-only invariant: dash never renders in normal operation. Reserved for two state-machine bug cases (output_status reports finished but finish_time is None; or a future schema change makes start_time optional). A7 updated to match. - B5 corrected to use SyncToCloud::Globally(RespectUserSyncSetting::Yes). tech.md and product.md SyncToCloud references both updated; matches the codebase (cloud_preferences.rs:25 plus code.rs:19/29/40/48). - Non-goal updated to clarify time-zone vs 24h preference: time-zone is out, 24h/12h reads from OS at startup (not a user setting). Each correction includes a "Correction (re-review warpdotdev#10128)" callout.
|
Round-3 corrections pushed (commit cdbd9c0):
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines per-exchange submitted/completed timestamps for Agent Mode, including formatting, settings, restoration behavior, timers, and tests. The model-level timestamp source is mostly aligned with the current code, and the security/privacy surface is addressed by limiting telemetry to the setting toggle with existing opt-outs.
Concerns
- The timer strategy depends on an
is_visibleview hook that is not identified in the current agent-view API, so the pause/resume invariant needs a concrete lifecycle source. - Cancellation rendering is internally inconsistent and can duplicate the completion timestamp.
- The integration test plan still expects stripped timestamps to render a dash, contradicting the spec's corrected
Local::now()synthesis behavior. - The out-of-scope list contradicts the product contract and formatter section on 12h/24h preference support.
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| labels. Use a single underlying timer firing at 1Hz and | ||
| internally rate-limit the 30s consumers. | ||
| - Stops ticking when the agent view loses visibility | ||
| (existing `is_visible` hook on the view; verify by grepping |
There was a problem hiding this comment.
is_visible hook, but the current agent-view code exposes active/fullscreen/inline state rather than this hook. Specify the actual event or source the service subscribes to; otherwise the hidden-panel no-tick invariant is not implementable as written.
|
|
||
| The `ExchangeTimes` struct exposes `cancelled_at: Option<DateTime<Local>>`. | ||
| `DurationLabel` checks `cancelled_at` before `completed_at`; if | ||
| present, renders "cancelled at HH:MM • Xs" instead of "HH:MM • Xs". |
There was a problem hiding this comment.
TimestampLabel for completed_at described above: a cancelled response would render both the timestamp label and cancelled at HH:MM. Define a single cancellation label path, or make DurationLabel render only the duration.
| build (snapshot test). | ||
| - IT6: Restore a yaml conversation with messages dated yesterday; | ||
| exchanges show "Mon 3:47 PM" not "just now." | ||
| - IT7: An exchange with stripped timestamps shows "—" and emits a |
There was a problem hiding this comment.
Local::now(), not render —. Change IT7 to construct the explicit invariant-violation state (Finished plus finish_time: None) or remove it.
| - Per-token / per-tool-call timestamps. | ||
| - Cumulative agent CPU time / credits per exchange (overlaps | ||
| #10000, #10052). | ||
| - Time-zone selection and 24h vs 12h preference. |
There was a problem hiding this comment.
24h vs 12h preference from out-of-scope, or change the behavior contract and formatter design to make it a follow-up.
- is_visible hook: identified concrete source as BlockListViewModel::is_panel_visible / terminal_view().is_active (same checks the agent view's existing 1Hz status refresh uses); no new lifecycle API needed. - Cancellation rendering deduplicated: three structurally distinct shapes (in-progress / cancelled / finished) rather than swapping strings in one slot. Cancelled shape is a single composite label that owns both the time and duration, so they cannot duplicate. - IT7 corrected: tests force-stripped finish_time on a Finished output_status (since the model normally synthesizes Local::now()). This is the defensive-guard path from B7. - Non-goals updated: time-zone selection out, 24h/12h preference IN scope per B2 (matches formatter section).
|
Round-4 corrections (commit 51fba7c):
|
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
Reviewed the product and technical specs for per-message timestamps in Agent Mode, including the supplemental security pass. The spec covers the core behavior, setting/privacy guardrails, and most test cases, but the timer lifecycle design has implementation-affecting gaps.
Concerns
- The timer strategy is gated to visible in-progress exchanges, which would stop 30s relative timestamp refreshes after an exchange completes even though B2/A8 require completed labels to auto-promote.
- The visibility pause/resume mechanism names lifecycle APIs/observers that do not appear in the checkout, so implementers do not have a concrete hook to use.
- Cancellation display has product/technical behavior but no integration coverage for the composite label.
Verdict
Found: 0 critical, 2 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ## Timer strategy | ||
|
|
||
| Per product.md §risk 2 recommendation: **single shared 1Hz timer, | ||
| gated to visible in-progress exchanges**. |
There was a problem hiding this comment.
| > concrete lifecycle source is the `ViewHandle`'s lifecycle | ||
| > callbacks: `BlockListViewModel::is_panel_visible(app)` / | ||
| > `terminal_view().is_active(app)` (the same checks the agent | ||
| > view's existing 1Hz status refresh uses). The | ||
| > `TimestampTickService` registers a `view_visibility_changed` |
There was a problem hiding this comment.
view_visibility_changed, BlockListViewModel::is_panel_visible(app), and terminal_view().is_active(app) as existing lifecycle sources, but those symbols do not appear in the checkout. Replace this with the actual view/lifecycle API or specify the new API to add; otherwise the pause/resume design is not implementable as written.
| timestamps). | ||
| - T11: Duration formatter cases (B3): "<1s", "Xs", "Xm Ys", "Xh Ym". | ||
|
|
||
| ### Integration tests (`app/src/integration_testing/agent_mode/timestamps_test.rs` — new) |
There was a problem hiding this comment.
💡 [SUGGESTION] Add an integration test for cancelling a running exchange and asserting the single composite cancelled at HH:MM • Xs label, with no separate completed timestamp; B8 defines this behavior but the integration plan only covers completion and defensive guards.
1 participant
Summary
Spec for issue #9889 — Agent Mode shows the conversation as a sequence of prompts and responses with no temporal information. Users cannot tell when a prompt was submitted, how long the agent took, or how recent any given exchange is — particularly relevant for long-running tasks, comparing two attempts, or reviewing what an autonomous agent did overnight.
Investigation
The data already exists at the model layer:
AIAgentExchangecarriesstart_ts: Option<DateTime<Local>>andcompleted_ts: Option<DateTime<Local>>(app/src/ai/agent/conversation.rs:3218-3219).Conversation::start_time_from_exchange_messagesderives the start time from the user input'sAIAgentContext::CurrentTime(conversation.rs:556).Only the view layer omits this information.
What's in the spec
product.md— 8 testable behavior invariants (B1–B8), 8 acceptance criteria (A1–A8), explicit non-goals (per-token timestamps, server/client time reconciliation, CPU/credit accounting), and 5 risks with concrete TECH decisions to make.tech.md— picks the data-derivation site (pure helper at the view layer, no new model state), the UI integration site (existing bubble metadata row, not a new gutter), the timer strategy (single shared 1Hz timer gated to visible in-progress exchanges), and lays out 5+6 unit tests + 8 integration tests.Architectural choices
TimestampTickServicefor all in-progress duration counters, gated to "panel visible AND in-progress exchange present." A long conversation with 50 visible exchanges costs one timer, not 50.agents.show_message_timestamps(defaulttrue,SyncToCloud::Always). Whenfalsethe widgets are not added to the layout — zero off-state cost (A5 invariant).log::warn!so production data gaps are visible without flooding the log.Test plan
exchange_times.rs)timestamp_widgets.rs)Open questions for maintainer review
true(opt-out, recommended) orfalse(opt-in)?needs-mocks.TimestampTickServicelifetime: per-conversation (if conversations can render in multiple places simultaneously) or singleton (if not)?Closes (spec-only) #9889 — implementation PR will follow once spec direction is confirmed.