Repository navigation
fix(components): subtract permissionWaitMs from Worked-for duration - #509
Conversation
The schema already records time spent waiting on permission and documents effective work as (endedAt - timestamp) - permissionWaitMs. The renderer only subtracted the two clocks, so leaving a plan card for a minute or ten showed that wait as "Worked for …".
github-actions
Bot
added
scope: components
status:needs-pr-attention
![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: a05c7c1e58
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const wait = message.permissionWaitMs; | ||
| if (typeof wait !== 'number' || !Number.isFinite(wait) || wait <= 0) return span; |
There was a problem hiding this comment.
In the production chat flow, ai-gui/index.tsx passes history through buildChatStreamItems(), but its message projection in build-chat-stream-items.ts:151-168 omits permissionWaitMs (and its cache predicate does not track the field). Consequently message.permissionWaitMs is always undefined here, this function returns the original wall-clock span, and the shipped Worked-for bug remains; copy the field into the parsed message and include it in cache invalidation. The new test misses this because it calls the helper directly.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Production chat goes ai-gui/index.tsx → buildChatStreamItems(). The projection at build-chat-stream-items.ts did not copy permissionWaitMs, and the cache predicate did not watch it, so the helper on this PR was a no-op on the real path. The original unit test called the helper directly, which is why it stayed green.
Pushed b62e3a07 on this branch: copy permissionWaitMs onto the parsed message; invalidate the stream cache when it changes. Tests in build-chat-stream-items.test.ts cover the copy and the cache miss, and run resolveSessionHistoryDurationMs on the projected message.
Live OSS after that commit, same 1-minute wait at Implement this plan?: 工作了 38秒, not 1分27秒.
buildChatStreamItems omitted the field, so Worked-for still used wall clock on the real render path. Copy it onto the parsed message and invalidate the stream cache when it changes.
github-actions
Bot
removed
the
status:needs-pr-attention
|
Thanks |
2 participants
Author type
An AI agent found the mismatch and implemented the subtraction. A human reviewed the live OSS runs, the schema/renderer comparison, and this patch before submit.
Related issue
Closes #512
Distinct from closed #260 (that restamped
endedAtafter quit/reopen). This change is the live Worked-for label on a turn that never quit.Problem / pressure
The Worked-for duration display includes permission wait time. Only turns that actually wait on a permission card are affected.
The schema already stores the wait and documents the formula (
packages/shared/src/schema.ts):CLI writes
permissionWaitMswhen a permission resolves.resolveSessionHistoryDurationMsonlyPicksendedAtandtimestamp, so the number on screen is wall clock, including time spent atImplement this plan?.Live OSS 0.76.0, Codex
5.6-Sol, Plan on, Auto review (so a second file-permission wait does not stack). After Yes, implementation was ~15s in each group.permissionWaitMsImmediate Yes (control):
Wait 1 minute, then Yes:
Wait 10 minutes, then Yes:
Repro: OSS desktop, Codex, Plan on. Send a prompt that pops
Implement this plan?. Do not quit. Wait, click Yes, let the file write finish. Worked-for tracks the wait, not the ~15s of work.Summary
resolveSessionHistoryDurationMsnow subtracts a finitepermissionWaitMsand clamps a negative remainder to 0. Turns with no wait field stayendedAt - timestamp.endedAtis not restamped.buildChatStreamItemscopiespermissionWaitMsonto the parsed message and invalidates the stream cache when the field changes, so the production chat path actually reaches the helper.Before / after
endedAt - timestamp(wall clock, wait included)permissionWaitMs, clamped to 0Test plan
On
fix/subtract-permission-wait-durationafter rebase ontoorigin/main084ddb86:Result: 3 passed (no wait unchanged; recorded wait subtracted; wait longer than the span → 0).
Result: 15 passed, including copying
permissionWaitMsonto the rendered message and not reusing a cached item when the wait changes.Live OSS after the projection fix, same 1-minute wait recipe: 工作了 38秒, not 1分27秒. Screenshot in the review-thread reply.
Context handoff
Context handoff is public. An invalid fork PR body receives seven days to be corrected before closure.
Instructions for reviewing agents
packages/components/src/lib/session-history-duration.tsandpackages/components/tests/session-history-duration.test.ts;view.tsxstill consumes a number or null from this helper.endedAt([Bug] Completed turn duration grows after the app is quit and reopened #260 / fix(cli): keep a finished turn's endedAt across teardown finalize #409). Clamp an oversize wait to 0 instead of returning null.permissionWaitMsmust stay wall-clock. Unit tests do not drive Electron; live evidence is the three OSS screenshots.Authoring context
endedAt. Do not change permission UX, title overwrite, or dead Yes buttons.permissionWaitMs. Missing or non-finite wait is treated as 0.