Skip to content

[codex-analytics] add protocol-native item timestamps#20290

Closed
rhan-oai wants to merge 1 commit intomainfrom
pr20290
Closed

[codex-analytics] add protocol-native item timestamps#20290
rhan-oai wants to merge 1 commit intomainfrom
pr20290

Conversation

@rhan-oai
Copy link
Copy Markdown
Collaborator

@rhan-oai rhan-oai commented Apr 30, 2026

No description provided.

@rhan-oai rhan-oai requested a review from a team as a code owner April 30, 2026 00:07
@rhan-oai
Copy link
Copy Markdown
Collaborator Author

Closing duplicate PR created during a relinked Sapling submit; continuing on #20239.

@rhan-oai rhan-oai closed this Apr 30, 2026
Copy link
Copy Markdown
Contributor

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d5bedde563

ℹ️ 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".

Comment on lines 489 to +493
parsed_cmd: exec_input.parsed_cmd.to_vec(),
source: exec_input.source,
interaction_input: exec_input.interaction_input.map(str::to_owned),
started_at_ms,
completed_at_ms: Some(completed_at_ms),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Badge Reuse begin timestamp when emitting ExecCommandEnd

ExecCommandBegin records started_at_ms at emission time, but emit_exec_end recomputes start as completed_at_ms - duration. On rejection paths duration is forced to zero, so end events report started_at_ms == completed_at_ms even after long approval waits. This produces inconsistent begin/end timestamps and under-reports latency in history/analytics.

Useful? React with 👍 / 👎.

Comment on lines 527 to +532
success,
changes,
status,
started_at_ms,
completed_at_ms: Some(completed_at_ms),
duration_ms,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Badge Preserve patch start time instead of deriving from duration

PatchApplyBegin emits a real started_at_ms, but emit_patch_end derives start from completed_at_ms - duration_ms. For declined/error paths duration is often zero, so completed events collapse start and end to the same instant, overwriting accurate start timing in projected thread items and skewing tool-duration analytics.

Useful? React with 👍 / 👎.

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.

1 participant