⏱️ feat: Show Run-Step Durations On Tool Cards - #14892
Conversation
|
@codex review Head is
Worth a reviewer's attention specifically:
Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6045225f8e
ℹ️ 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".
| * construction; the callers only forward the number. | ||
| */ | ||
| const duration = | ||
| progress >= 1 && !error && isReportableRunStepDuration(durationMs) |
There was a problem hiding this comment.
When a completed run step has status failed (or a background task fails), callers pass errorSuffix but keep error false because that prop represents cancellation. Consequently, progress >= 1 && !error succeeds and the duration is rendered beside “Tool failed,” contrary to the stated failure suppression. Include the failure/error-suffix state in this gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d5344ef — the gate now checks !errorSuffix alongside !error, and the failed-via-suffix path is pinned by its own test (visible and announced halves both).
You read the prop semantics correctly: at every call site error carries cancellation and failure travels as errorSuffix alone, which my gate — and my test, which only exercised error: true — missed. Same defect class as AI-1810; one derivation meant a one-line fix rather than seven.
Generated by Claude Code
|
@codex review Round 1 addressed in
Verification: 876/876 across Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Self-audit round pushed; head is
670/670 across the touched client suites (4 new tests, including a Generated by Claude Code |
Surfaces how long each tool call took, derived from the `closed_at` / `created_at` pair already carried by `on_run_step_closed` — the same event #14871 and #14873 use for the terminal status. No new event, no new SDK surface. The duration is stamped onto the content part at the same three sites as `runStepStatus`, so it survives a reload and a resumable reconnect rather than living only on the live React message: - `callbacks.js`, on the aggregated part before the event is forwarded - `RedisJobStore`, in the host-authored replay reconstruction branch - `useStepHandler`, on the live message Rendering lands in the shared `ProgressText`, which nine tool cards already use, rather than in each card: one place decides whether a duration is shown and how it reads, and the cards only forward the number. That keeps this from adding a tenth independent state derivation to a component family whose label/announcement/progress split is already the subject of AI-1810. The value is deliberately absent rather than zero whenever it would be a guess — no `created_at`, non-finite input, or a negative elapsed time from two clocks that disagree, which is now reachable because a step can be opened in one process and closed in another after a checkpoint resume. Sub-second durations are suppressed as noise, and it renders only on a settled, non-error card, where the slot is not already carrying the cancelled icon or the error suffix. For assistive technology the compact form (`3.5s`) is hidden and paired with a spoken equivalent ("took 3.5 seconds"), both inside the button, so the accessible name carries the duration without an `aria-live` region re-announcing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
The import-sort gate runs against the files a PR changes, so pre-existing drift in `ProgressText.tsx` and `RedisJobStore.ts` surfaced on this branch. Both were already unsorted on `dev`; this is the sorter's output, with no semantic change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
`getReportableRunStepDurationMs` declared its parameter as `Pick<RunStepClosedEvent, 'created_at' | 'closed_at'>`, where `closed_at` is required. That contradicted the function's own purpose: every guard inside it exists precisely to handle stamps that may be missing. The Redis replay branch reconstructs closures from persisted JSON and holds nothing stronger than "might be a number", so it failed to typecheck against the narrower signature. Widened to an exported `RunStepTimestamps` shape with both stamps optional, rather than asserting at the call site — an assertion would move the decision about what is trustworthy somewhere it cannot be enforced, which is the thing the helper exists to centralize. Callers holding a fully-typed event still pass, since a required field satisfies an optional one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
At every call site `error` carries cancellation while failure travels through `errorSuffix` with `error` false, so gating the duration on `!error` alone rendered "· 3.5s" beside "· failed" — and announced it. The gate now checks both terminal-failure channels. The original test pinned only the `error: true` path, which is why this survived; the failed-via-suffix path is now pinned separately, both the visible and the announced half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
The three stamp sites filtered through the 1-second reportability threshold before persisting, baking a presentation rule into stored data: a 900ms step stored nothing, making "fast" indistinguishable from "not derivable" and unrecoverable if the display rule ever changes. Stamp sites now persist the raw `getRunStepDurationMs` value — absent only when genuinely not derivable — and the renderer alone decides what is worth showing, which `ProgressText` already did. Rendering is unchanged. `getReportableRunStepDurationMs` is removed; it existed only to serve the write-time filter, and a test now pins that sub-threshold durations survive to storage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
A backgrounded call's run step closes when dispatch returns the handle, so the stamped duration is the dispatch time. Rendering it beside "Running/Finished in background" misstated a detached task's runtime as seconds — and violated the "settled card only" rule, since the card is still tracking the detached run. Scope is exactly the two cards that parse background handles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
The fractional seconds value was interpolated as a raw JS number, which hardcodes the en-US decimal point into every language — "1.4s" where the locale writes "1,4 s" — and translators cannot fix a number formatted in code. The value is now formatted via Intl.NumberFormat with i18n.language, following MessageTimestamp's pattern of threading the language into the util; plural-key selection stays on the numeric value. A malformed language tag falls back to the plain number. Also documents the two accepted limits of the derivation, so they read as decisions rather than oversights: positive clock skew is undetectable from a single stamp pair, and the value is wall-clock elapsed, so a step held open across a suspension (checkpoint resume, HITL approval wait) includes that time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
2dfbc28 to
62b97e8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dfbc28728
ℹ️ 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".
| * handle, so its duration is the dispatch time — showing it beside | ||
| * "Running/Finished in background" would misstate a detached | ||
| * task's runtime as seconds. */ | ||
| durationMs={backgroundHandle == null ? runStepDurationMs : undefined} |
There was a problem hiding this comment.
When a background code task settles, createBackgroundCodeResultHandler passes its final stdout to updateToolCallResult, which replaces the original handle JSON in tool_call.output (packages/api/src/agents/harvest.ts:180-190 and packages/data-schemas/src/methods/message.ts:313-345). After that patch—or after reloading the persisted message—parseBackgroundHandle(output) returns null, so this condition restores the run-step duration even though it still measures only dispatch time; the same condition exists in ExecuteCode.tsx. Preserve a separate background marker or clear the dispatch duration when harvesting instead of relying on the transient output shape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 7f960c1, taking your first option — a durable marker rather than clearing the duration. The harvest patch (both the initial write and the reapply heal, which can overwrite a full-row save that reverted the part) now stamps backgrounded: true onto the tool call in the same atomic $mergeObjects that replaces the handle, and the two cards gate on handle-or-marker.
Clearing at harvest would have contradicted e4bd15d in this same PR — destroying stored data to enforce a display rule. The dispatch duration stays persisted as the fact it is; the marker is what tells renderers not to present it as the task's runtime.
Caveat, flagged in the commit too: data-schemas cannot be installed in this environment (same npm ci 403 as packages/api), so message.ts/harvest.ts are syntax-checked with resolution off and otherwise verified by review — CI runs their real typecheck and suites.
Generated by Claude Code
| key: 'com_ui_duration_minutes', | ||
| values: { 0: minutes, 1: seconds }, |
There was a problem hiding this comment.
For durations of a minute or more, the visible minutes/seconds and announced minute count are passed to i18next as raw numbers, and the app's interpolation configuration only disables escaping (client/src/locales/i18n.ts:305) rather than formatting numbers. Consequently Arabic, Persian, and other locales that use localized digits switch back to ASCII for 1m 5s even though the under-a-minute path formats its value with Intl.NumberFormat. Format all three minute-branch values with the supplied language before interpolation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 7f960c1 — all three minute-branch values (visible minutes, visible seconds, announced minute count) now flow through the same Intl.NumberFormat helper as the seconds branch, renamed formatDurationValue since it no longer formats only seconds. Plural selection stays on the numeric value; only the interpolated text is formatted. An ar-EG test pins the localized digits (١m ٥s), so the branch can't silently revert to ASCII again.
Generated by Claude Code
…ize Minute Digits Codex round 3, both findings confirmed. **Background origin survived only as transient state.** The dispatch handle in `tool_call.output` and the live status-marker attachment are both gone once the harvester patches the settled task's stdout over the handle — so the round-2 suppression (`backgroundHandle == null`) came back on after harvest or reload, showing dispatch time as the task's runtime. Following the same rule as e4bd15d (persist facts, decide at render): the harvest patch now stamps `backgrounded: true` onto the tool call in the same atomic write that erases the handle — on the heal path too, which re-applies over full-row saves that reverted the part. The cards gate on handle-or-marker; the dispatch duration itself stays stored. **Minute-branch digits bypassed locale formatting.** The seconds branch went through Intl.NumberFormat while minutes interpolated raw numbers, so Arabic/Persian locales flipped to ASCII digits above one minute. All interpolated values now flow through the (renamed) formatDurationValue; an ar-EG test pins the localized digits. data-schemas cannot be installed in this environment (same npm ci 403 as packages/api), so message.ts/harvest.ts are syntax-checked with resolution off and otherwise verified by review; CI runs their real typecheck and suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
|
@codex review Round 3 addressed in
671/671 across the touched client suites on the rebased base; client typecheck at baseline; lint and import-sort clean. Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f960c1dd0
ℹ️ 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".
| /** This patch replaces the dispatch-handle output — the client's only | ||
| * transient signal that the call ran detached — so it persists the | ||
| * durable `backgrounded` marker in the same atomic write. */ | ||
| markBackgrounded: true, |
There was a problem hiding this comment.
In the inspected api/server/controllers/agents/callbacks.background.spec.js suite, the successful-harvest test uses toHaveBeenCalledWith with an exact argument object that does not include markBackgrounded. This newly forwarded property therefore makes that test fail on every run of the API Jest suite, blocking CI; update the expectation to include and assert markBackgrounded: true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in dc89c1c (formatting follow-up fc1c04b). The exact-object expectation now includes markBackgrounded: true, and I added the assertion to the two objectContaining expectations as well — the file-failure best-effort path and the reapply heal — since the durable stamp is now part of the behavior those tests exist to pin, not incidental noise to be tolerated.
Good catch on a spec I can't execute here (api workspace, same install blockage) — this is the second finding of this session to land exactly on that seam.
Generated by Claude Code
The successful-harvest test's exact `toHaveBeenCalledWith` object did not include the newly forwarded `markBackgrounded`, so the API suite would fail on it. All three harvest-call expectations now assert `markBackgrounded: true` — the exact-object one of necessity, the two `objectContaining` ones deliberately, since the durable stamp (on the best-effort file-failure path and the reapply heal alike) is now part of the behavior under test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5
|
@codex review Round 4 addressed in Head is Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
* 🧩 refactor: Resolve Tool-Card State Once (AI-1810) Each tool card derived its state several times over — the visible label from one expression, the `aria-live` announcement from another, the icon and shimmer from a third, and since #14906 the follow-scroll from a fourth. Nothing tied them together; they agreed only because each was written to agree. Thirteen of the seventeen review findings on #14873 were instances of one derivation being updated and another left behind, and #14892 added more. `resolveToolCallPhase` is now the single source: one function encoding the precedence rules, each of which a specific review finding established, returning `running | completed | cancelled | failed`. Everything the card shows reads that value. `ProgressText` takes `phase` in place of the `error` + `errorSuffix` pair, which encoded three terminal states in two booleans — `error` meant cancelled, a present `errorSuffix` meant failed — and made every consumer reconstruct the distinction. That shape is precisely what let a duration render beside "failed" (Codex round 1 on #14892). Two things fell out once the state had one home, both dead code rather than deletions of behaviour: - `progress` left `ProgressText` entirely; the phase already carries everything it was used to decide. - The `useProgress` mask went with it. Passing 1 in still matters — it stops the 200ms interval — but masking the output no longer does, because the phase treats an explicit close as terminal outright. The "both halves are load-bearing" subtlety is now one half. Scope: the nine cards that render the shared `ProgressText`. The three with bespoke layouts (`WebSearch`, `SubagentCall`, `OpenAIImageGen`) still resolve their own state and are the natural follow-up — they can adopt the resolver without adopting the component. Refactor-only. 4891/4891 client tests pass unchanged, including the suites that encode the cancelled/failed precedence in both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Infer Cancellation From Reported Progress, Not The Animation `useProgress` holds below 1 for ~200ms after a call reports completion: it emits the previous value, then `0.99`, then `1` on a timeout. The resolver read that animated value for its cancellation inference, so a successful call whose submission ended inside that window rendered — and announced — as "Cancelled". The input is now split. `reportedProgress` is what the stream said and drives the inference; `displayProgress` is the animated value and drives `running` vs `completed`, so the label and shimmer still follow the animation rather than snapping. This restores `ToolCall` and `RetrievalCall`, whose previous predicates used `initialProgress` and were immune, and additionally fixes `useToolCallState`, which inferred from `rawProgress` and therefore carried the bug already — every card the hook backs was exposed to it before this PR. Three tests cover the window: a reported-complete call mid-settle is `running`, a genuinely unfinished one is still `cancelled`, and the card settles to `completed` without a cancelled frame in between. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🧹 chore: Drop Unused Phase Predicates; Correct A Stale Comment `isFailedPhase` and `isRunningPhase` had no callers — every consumer compares the phase directly, which reads better than a wrapper. An unused abstraction is the thing this PR argues against, so it should not ship one. The comment above the hook's resolver call still described "the raw progress the legacy heuristic was written against", which stopped being true when the input split into reported and display progress. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 --------- Co-authored-by: Claude <noreply@anthropic.com>
* ⏱️ feat: Show Run-Step Durations On Tool Cards Surfaces how long each tool call took, derived from the `closed_at` / `created_at` pair already carried by `on_run_step_closed` — the same event danny-avila#14871 and danny-avila#14873 use for the terminal status. No new event, no new SDK surface. The duration is stamped onto the content part at the same three sites as `runStepStatus`, so it survives a reload and a resumable reconnect rather than living only on the live React message: - `callbacks.js`, on the aggregated part before the event is forwarded - `RedisJobStore`, in the host-authored replay reconstruction branch - `useStepHandler`, on the live message Rendering lands in the shared `ProgressText`, which nine tool cards already use, rather than in each card: one place decides whether a duration is shown and how it reads, and the cards only forward the number. That keeps this from adding a tenth independent state derivation to a component family whose label/announcement/progress split is already the subject of AI-1810. The value is deliberately absent rather than zero whenever it would be a guess — no `created_at`, non-finite input, or a negative elapsed time from two clocks that disagree, which is now reachable because a step can be opened in one process and closed in another after a checkpoint resume. Sub-second durations are suppressed as noise, and it renders only on a settled, non-error card, where the slot is not already carrying the cancelled icon or the error suffix. For assistive technology the compact form (`3.5s`) is hidden and paired with a spoken equivalent ("took 3.5 seconds"), both inside the button, so the accessible name carries the duration without an `aria-live` region re-announcing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🎨 style: Sort Imports In Touched Files The import-sort gate runs against the files a PR changes, so pre-existing drift in `ProgressText.tsx` and `RedisJobStore.ts` surfaced on this branch. Both were already unsorted on `dev`; this is the sorter's output, with no semantic change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Accept Partial Timestamps In Run-Step Duration Helper `getReportableRunStepDurationMs` declared its parameter as `Pick<RunStepClosedEvent, 'created_at' | 'closed_at'>`, where `closed_at` is required. That contradicted the function's own purpose: every guard inside it exists precisely to handle stamps that may be missing. The Redis replay branch reconstructs closures from persisted JSON and holds nothing stronger than "might be a number", so it failed to typecheck against the narrower signature. Widened to an exported `RunStepTimestamps` shape with both stamps optional, rather than asserting at the call site — an assertion would move the decision about what is trustworthy somewhere it cannot be enforced, which is the thing the helper exists to centralize. Callers holding a fully-typed event still pass, since a required field satisfies an optional one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Suppress Duration When Failure Arrives As errorSuffix Alone At every call site `error` carries cancellation while failure travels through `errorSuffix` with `error` false, so gating the duration on `!error` alone rendered "· 3.5s" beside "· failed" — and announced it. The gate now checks both terminal-failure channels. The original test pinned only the `error: true` path, which is why this survived; the failed-via-suffix path is now pinned separately, both the visible and the announced half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🧩 refactor: Persist Raw Run-Step Durations, Threshold At Render Only The three stamp sites filtered through the 1-second reportability threshold before persisting, baking a presentation rule into stored data: a 900ms step stored nothing, making "fast" indistinguishable from "not derivable" and unrecoverable if the display rule ever changes. Stamp sites now persist the raw `getRunStepDurationMs` value — absent only when genuinely not derivable — and the renderer alone decides what is worth showing, which `ProgressText` already did. Rendering is unchanged. `getReportableRunStepDurationMs` is removed; it existed only to serve the write-time filter, and a test now pins that sub-threshold durations survive to storage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Suppress Duration On Backgrounded Bash And Code Cards A backgrounded call's run step closes when dispatch returns the handle, so the stamped duration is the dispatch time. Rendering it beside "Running/Finished in background" misstated a detached task's runtime as seconds — and violated the "settled card only" rule, since the card is still tracking the detached run. Scope is exactly the two cards that parse background handles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🌍 fix: Format The Sub-10s Decimal For The Active Locale The fractional seconds value was interpolated as a raw JS number, which hardcodes the en-US decimal point into every language — "1.4s" where the locale writes "1,4 s" — and translators cannot fix a number formatted in code. The value is now formatted via Intl.NumberFormat with i18n.language, following MessageTimestamp's pattern of threading the language into the util; plural-key selection stays on the numeric value. A malformed language tag falls back to the plain number. Also documents the two accepted limits of the derivation, so they read as decisions rather than oversights: positive clock skew is undetectable from a single stamp pair, and the value is wall-clock elapsed, so a step held open across a suspension (checkpoint resume, HITL approval wait) includes that time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Persist A Durable `backgrounded` Marker Through Harvest; Localize Minute Digits Codex round 3, both findings confirmed. **Background origin survived only as transient state.** The dispatch handle in `tool_call.output` and the live status-marker attachment are both gone once the harvester patches the settled task's stdout over the handle — so the round-2 suppression (`backgroundHandle == null`) came back on after harvest or reload, showing dispatch time as the task's runtime. Following the same rule as e4bd15d (persist facts, decide at render): the harvest patch now stamps `backgrounded: true` onto the tool call in the same atomic write that erases the handle — on the heal path too, which re-applies over full-row saves that reverted the part. The cards gate on handle-or-marker; the dispatch duration itself stays stored. **Minute-branch digits bypassed locale formatting.** The seconds branch went through Intl.NumberFormat while minutes interpolated raw numbers, so Arabic/Persian locales flipped to ASCII digits above one minute. All interpolated values now flow through the (renamed) formatDurationValue; an ar-EG test pins the localized digits. data-schemas cannot be installed in this environment (same npm ci 403 as packages/api), so message.ts/harvest.ts are syntax-checked with resolution off and otherwise verified by review; CI runs their real typecheck and suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🧪 test: Assert The `markBackgrounded` Stamp In Harvest Expectations The successful-harvest test's exact `toHaveBeenCalledWith` object did not include the newly forwarded `markBackgrounded`, so the API suite would fail on it. All three harvest-call expectations now assert `markBackgrounded: true` — the exact-object one of necessity, the two `objectContaining` ones deliberately, since the durable stamp (on the best-effort file-failure path and the reapply heal alike) is now part of the behavior under test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🎨 style: Wrap Harvest Spec Expectation Per Prettier Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 --------- Co-authored-by: Claude <noreply@anthropic.com>
* 🧩 refactor: Resolve Tool-Card State Once (AI-1810) Each tool card derived its state several times over — the visible label from one expression, the `aria-live` announcement from another, the icon and shimmer from a third, and since danny-avila#14906 the follow-scroll from a fourth. Nothing tied them together; they agreed only because each was written to agree. Thirteen of the seventeen review findings on danny-avila#14873 were instances of one derivation being updated and another left behind, and danny-avila#14892 added more. `resolveToolCallPhase` is now the single source: one function encoding the precedence rules, each of which a specific review finding established, returning `running | completed | cancelled | failed`. Everything the card shows reads that value. `ProgressText` takes `phase` in place of the `error` + `errorSuffix` pair, which encoded three terminal states in two booleans — `error` meant cancelled, a present `errorSuffix` meant failed — and made every consumer reconstruct the distinction. That shape is precisely what let a duration render beside "failed" (Codex round 1 on danny-avila#14892). Two things fell out once the state had one home, both dead code rather than deletions of behaviour: - `progress` left `ProgressText` entirely; the phase already carries everything it was used to decide. - The `useProgress` mask went with it. Passing 1 in still matters — it stops the 200ms interval — but masking the output no longer does, because the phase treats an explicit close as terminal outright. The "both halves are load-bearing" subtlety is now one half. Scope: the nine cards that render the shared `ProgressText`. The three with bespoke layouts (`WebSearch`, `SubagentCall`, `OpenAIImageGen`) still resolve their own state and are the natural follow-up — they can adopt the resolver without adopting the component. Refactor-only. 4891/4891 client tests pass unchanged, including the suites that encode the cancelled/failed precedence in both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Infer Cancellation From Reported Progress, Not The Animation `useProgress` holds below 1 for ~200ms after a call reports completion: it emits the previous value, then `0.99`, then `1` on a timeout. The resolver read that animated value for its cancellation inference, so a successful call whose submission ended inside that window rendered — and announced — as "Cancelled". The input is now split. `reportedProgress` is what the stream said and drives the inference; `displayProgress` is the animated value and drives `running` vs `completed`, so the label and shimmer still follow the animation rather than snapping. This restores `ToolCall` and `RetrievalCall`, whose previous predicates used `initialProgress` and were immune, and additionally fixes `useToolCallState`, which inferred from `rawProgress` and therefore carried the bug already — every card the hook backs was exposed to it before this PR. Three tests cover the window: a reported-complete call mid-settle is `running`, a genuinely unfinished one is still `cancelled`, and the card settles to `completed` without a cancelled frame in between. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🧹 chore: Drop Unused Phase Predicates; Correct A Stale Comment `isFailedPhase` and `isRunningPhase` had no callers — every consumer compares the phase directly, which reads better than a wrapper. An unused abstraction is the thing this PR argues against, so it should not ship one. The comment above the hook's resolver call still described "the raw progress the legacy heuristic was written against", which stopped being true when the input split into reported and display progress. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Summary
Surfaces how long each tool call took, on the tool card itself:
The duration is derived from the
closed_at/created_atpair already carried byon_run_step_closed— the same event #14871 and #14873 use for the terminal status. No new event, no new SDK surface, no new round trip. The information was already arriving and being discarded.This is the second half of AI-1794; the first half (terminal status) shipped in #14871 / #14873.
Where it is stamped
Alongside
runStepStatus, at the same three sites, so it survives a reload and a resumable reconnect rather than living only on the live React message:api/server/controllers/agents/callbacks.jspackages/api/src/stream/implementations/RedisJobStore.tsclient/src/hooks/SSE/useStepHandler.tsThe raw derivable value is what persists — absent only when genuinely not derivable (no
created_at, non-finite input, or a negative elapsed time from clocks that disagree, which is reachable since@librechat/agentsv3.6.0 lets a step open in one process and close in another). Whether a stored duration is worth showing is decided at render time only, so the display threshold stays adjustable without data loss and "fast" is never conflated with "unknown".Where it is rendered
In the shared
ProgressText, which nine tool cards already use — not in each card. One place decides whether a duration is shown and how it reads; the cards only forward the number. That is a deliberate structural choice given AI-1810 (13 of 17 Codex findings on #14873 were the label/announcement/progress split), and it paid for itself in this PR's own review: Codex round 1 found a gating bug, and the fix was one line in one file rather than seven.A duration renders only on a settled, successful card:
errorcarries cancellation at every call site, failure travels aserrorSuffixaloneCards that do not get it:
WebSearchandSubagentCall(bespoke layouts with noProgressText),OpenAIImageGen(its own localProgressText), andCodeAnalyze(does not receive run-step metadata at all).Accepted limits, documented in the code
Only the negative direction of clock skew is detectable from a single stamp pair — positive skew inflates the result and cannot be distinguished from a genuinely long step. And the value is wall-clock elapsed between open and close, so a step held open across a suspension (checkpoint resume, HITL approval wait) includes that time. Both are properties of the only data available; the docs say so rather than guessing at caps.
Accessibility & i18n
3.5sreads well on screen and badly aloud. The compact form isaria-hiddenand paired with a spoken equivalent ("took 3.5 seconds"), both inside the button — the accessible name carries the duration, and thearia-liveregion is untouched and does not re-announce. Above a minute the spoken form rounds to whole minutes; the precise value stays on the button.The sub-10s decimal is formatted per-locale via
Intl.NumberFormatwithi18n.language(theMessageTimestamppattern) — "1,4 s" locales get their comma instead of a hardcoded en-US point. Plural selection follows the existingcom_ui_tools_count/_oneconvention.Change Type
Testing
32 new tests, all green, plus the full existing suites:
packages/data-provider/src/runSteps.spec.ts(11) — the derivation: elapsed time, same-tick close, each absent-stamp case, the clock-skew negative, non-finite and non-numeric input, the render-time threshold in both directions, and a pin that sub-threshold durations survive to storage rather than being pre-filtered at a stamp site.client/src/utils/__tests__/runStepDuration.spec.ts(10, 100% coverage of the formatter) — decimal below ten seconds, whole seconds above it, the1.0strailing-zero case, minute splitting, the59.6sboundary that would otherwise render60s, ade-locale pin for the decimal comma, and malformed-language-tag fallback.client/src/components/Chat/Messages/Content/__tests__/ProgressText.test.tsx(11) — the gating (running / cancelled / failed-via-suffix / absent / sub-threshold all render nothing, with cancelled and failed pinned separately since they arrive through different props) and the accessibility contract (aria-hiddencompact form,sr-onlyspoken form, singular vs plural, both inside the button).Existing suites:
client380 suites green (last full run 4472 tests; 670/670 across the touched suites after the final round),packages/data-provider1519 passed / 1 skipped,npx tsc --noEmitonclientat its pre-existing baseline of 1 unrelated error (useRum.tsfetch.preconnect), eslint and import-sort clean on every touched file.Runtime check of the built CJS surface the server requires (
callbacks.jsis plain JS, so a bad import would surface only at request time):Test Configuration:
The
api/packages/apiworkspaces cannot be installed in this environment (npm ci403s on thecdn.sheetjs.comtarballxlsxpins), so their suites were not run locally. TheRedisJobStorestamp site is covered instead by a fixture compiling both call-site shapes — the loose replayed-JSON object and a fully-typedRunStepClosedEvent— under the data-provider tsconfig; the branch itself remains reviewed rather than executed. CI runs the real suites.Review history
Codex round 1 (duration rendered beside "failed" — the
error/errorSuffixsplit) fixed ind5344ef; round 2 clean. Self-audit found the backgrounded-card and decimal-locale bugs (7241cc6,2dfbc28) plus the documented skew/suspension limits;6045225fixed a CI-caught type error where the helper's parameter type contradicted its own guards;7e8a20fis pre-existing import-sort drift surfaced by the changed-files gate.Checklist