Conversation
b48ac9f to
e02ab01
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head e02ab01af442d7cd0e8589928f5dbe5cf72f0f00 (14 files, +416/−147). The latest-usage selector walks the transcript backwards for a matching request anchor or context_compacted note, and accepts live diagnostics only when completedAt is strictly later than that note (apps/desktop/src/renderer/application/contracts/session-inspector/latest-request-usage.ts:41-110). Desktop carries the Host settlement time through (apps/desktop/src/renderer/application/contracts/session-inspector/live-context-usage.ts:63-90); Composer and WorkHub use the unknown/stale projection. I checked the zero/unknown, routing, compaction, and late-diagnostics regression cases (apps/desktop/src/main/__tests__/latest-request-usage.test.ts:75-259). I found no substantiated P0–P3 code issue in this review.
The current test check passes, but this branch conflicts with current main in apps/desktop/src/renderer/app-shell.tsx; it is not ready to merge. Please resolve the conflict and revalidate the resulting head. I did not run local tests or Electron E2E (Node 18/no installed dependencies), and have not verified real Host/transcript timestamp causality. This is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
778663b to
b7bd38a
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
I re-reviewed the current 14-file PR at 9fd2536fd0c7d2181e59630bcd95e7fa5977004d, including the resolved AppShell integration and the latest completedAt assertion. The selector treats a context_compacted transcript note as a boundary, and Composer/WorkHub now share the measured/stale/unavailable projection. I found one P3 ordering issue, detailed inline: an older live diagnostics snapshot can override a newer post-compaction request anchor while the debounced diagnostics refresh is pending or fails. The current tests cover the boundary against an older snapshot, but not a newer anchor against an older snapshot.
The current-head test check is in progress. GitHub reports the PR mergeable with base c0787020 (the current main at review time); the PR diff passes git diff --check. I did not run tests locally (Node 18/no installed dependencies), Electron E2E, or verify real Host/transcript timestamp causality. There is no storage schema migration in this change. This is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Thanks for the fix — the latest review on this head found no blocking issues, and it now merges cleanly. One thing we need before merging: the AI-use section says OpenAI Codex handled the later refinements, but also that "earlier implementation tooling provenance was not independently verified", and none of the four commits carries a Could you confirm which generative tool(s), if any, wrote the initial implementation ( This comment was drafted with automated assistance and posted by a maintainer-side reviewer. |
9fd2536 to
9a3066a
Compare
|
@hqhq1025 @Astro-Han Thanks for the review, I’ve addressed all comments, and all commits follow the CONTRIBUTING.md. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head 9a3066aece6e4c1a258ff492973218cac28dc884 (14 changed files, +460/−148). The follow-up preserves the post-compaction anchor timestamp, so an anchor can replace a retained pre-compaction diagnostics snapshot; the added regression covers that case. I found one P2 in the new ordering rule: for an ordinary completed request, the transcript anchor is written after the diagnostics completedAt, so the resolver also replaces the current per-request snapshot with a different input + output measurement and drops its metered window. This makes the composer gauge inaccurate in the common path. Please fix the causal comparison and add a same-request regression before merging.
The current-head test check passed; the PR is mergeable, though its merge state is blocked. I did not run local suites or an Electron UI session. The PR discloses Codex for this follow-up; the earlier implementation commit has no tool trailer, and its tooling provenance remains unverified.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
15462f9 to
7739e11
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
I re-reviewed head 7739e117eb43178d262f28068e8e72a32b876fa4, focusing on the nine-file follow-up and the earlier context-usage paths. The previous same-request timestamp finding is addressed: the anchor now carries the provider request's settlement time, not the later transcript row's write time. The runtime obtains it from the completed main attempt, the core validator accepts the optional field, and the renderer uses it to keep an equal-time diagnostics snapshot and its metered window. The added same-request test distinguishes this fix from the prior head; the post-compaction/new-anchor case remains covered. The optional field preserves legacy record decoding, and the Host compatibility epoch advances from 195 to 196 for the changed closed transcript shape. I found no substantiated new P0–P3 finding in this scope.
At publication, the current-head test check is still in progress. The PR is open/mergeable; the merge-tree against current main and git diff --check are clean. I did not run local suites (this checkout has Node 18 and no dependencies), a real Electron UI, or a cross-version Client–Host session. This is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Independent second review of head 7739e117 (different model lineage from the earlier review on this head).
The main paths look right. The pre-turn fold, manual /compact, the failed-open path, and the equal-timestamp case for a same-request anchor and snapshot all behave as intended. The previous P2, the ordering between a same-request anchor and its snapshot, is fixed. Compat epoch 196 is the correct next value (origin/main is at 195), and it is needed because the anchor schema check is strict. Old anchors without completedAt still decode. WorkHub and the conversation composer share the same resolver.
P2: a stale pre-compaction reading can still show as measured, which is the #5547 symptom. At settlement, ai-sdk-turn.ts:2384-2391 appends the context_compacted note before the token_usage row. The row's lastRequestAnchor (tokens and completedAt) only advances on a successful request. So in a multi-step turn like this one:
- step 1 succeeds (T1);
- step 2 hits context overflow;
- recovery compaction succeeds;
- the retry overflows again and the turn ends with an error.
The transcript then gets note(T3) followed by token_usage(anchor.completedAt=T1). selectLatestRequestUsage scans backwards, hits the token row first and returns {kind:'tokens', at:T1}, and never sees the newer note. Replaying this sequence through the compiled selector and resolver yields {kind:'measured', tokens:90000} instead of stale. No test covers the order "note before the row, but the anchor is older than the note". Possible fixes:
- in the selector, keep scanning for a same-
turnIdcontext_compactednote whosetsis later thananchor.completedAt; or - at runtime, omit the anchor when a fold happened after the last successful request.
P3 (non-blocking):
- CHANGELOG line 49 says the gauge renders
?. It actually renders the localized "Usage"/"用量" label, so stale and unavailable look identical except for the tooltip. - A mid-turn fold's note is only written at settlement, so the gauge never shows the compacted state while that turn is still running. The CHANGELOG wording about mid-turn recovery reads broader than that. This is a pre-existing runtime limitation.
- Between the note append and the
token_usageappend, the gauge briefly flips to compacted. If the token row write fails, that state persists until the next turn. - The composer tests don't assert the stale tooltip copy, so stale and unavailable aren't distinguished in tests.
Verified: npm run build:test and the 6 touched test files pass locally. Not verified: that the note ts and the telemetry now() share a clock (both are Host-side), whether a user Stop skips settlement, a runtime end-to-end replay of the P2 sequence, and manual UI behaviour.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; findings were checked against the code but please verify before acting.
| return { | ||
| kind: 'tokens', | ||
| tokens: anchor.inputTokens + output, | ||
| // The row is persisted after request settlement (and sometimes after a |
There was a problem hiding this comment.
P2: this returns on the first anchored token_usage row. However, settlement writes the context_compacted note before this row, and the anchor's completedAt can predate that note when the fold happened after the last successful request, for example when a post-fold retry fails. In that case the newer compaction is skipped and the pre-fold count shows as measured. Consider continuing the scan for a same-turn note with ts > anchor.completedAt, or not writing the anchor at runtime in that case.
| count is stale rather than current — showing it read as a live measurement of | ||
| what the session was about to send. The gauge walks the transcript backwards and | ||
| whichever fact comes first decides: a `context_compacted` note newer than every | ||
| measurement renders `?` with a tooltip saying why, and a measurement newer than |
There was a problem hiding this comment.
P3: the gauge now renders the localized Usage / 用量 label rather than ?.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
After compaction, the context gauge stops showing the pre-fold percentage and renders an explicit unavailable/stale reading until a post-fold measurement lands, with typed unavailable/measured/stale readings carried through both composers and settlement timestamps used to order live snapshots against the compaction boundary. The issue is real (base kept the last anchor indefinitely). The direction is sound: ledger order decides transcript state, timestamps only arbitrate the live snapshot, failed-open folds correctly don't count as boundaries, and the epoch bump 195→196 for the new anchor field is correct protocol hygiene. ContextDiagnosticsResult.completedAt exists (packages/runtime-host/src/protocol/context.ts:83), so the live path is well-typed.
Findings
- [P3]
CHANGELOG.mdclaims the post-compaction gauge renders?, but the shipped implementation renders the gauge icon with the localizedUsage/用量label and a tooltip (packages/ui/src/composer.tsxContextUsageAction; the PR body itself says so). User-facing doc contradicts the code. - [P3]
resolveContextUsagecompares the compaction note's write ts against the live snapshot'scompletedAt(apps/desktop/src/renderer/application/contracts/session-inspector/latest-request-usage.ts:83-90); the code comment itself concedes "the note may be recorded later than the actual fold". A late-written note suppresses a genuinely post-fold live measurement as stale until the next anchored usage row lands. Ledger order inselectLatestRequestUsagemakes this window narrow, but the timestamp is the weaker of the two signals. - [P3] PR is mergeable:CONFLICTING, CI was pending at snapshot time, and the body states tests were "not rerun against the unmodified base" with the "tests fail without it" box unchecked — the mutation evidence is asserted, not shown, for the final revision.
Verdict
needs-changes — conflicts must be resolved and the CHANGELOG/code mismatch fixed; core logic is otherwise sound.
7739e11 to
3c45c23
Compare
Treat measurements taken before a successful compaction as stale until a newer request settles. Update the composer display, localized tooltip, tests, and changelog. Generated-by: Codex
Preserve the selected request anchor timestamp so the context usage resolver can order it against a retained diagnostics snapshot. Cover the post-compaction sequence in resolver tests. Generated-by: Codex
Persist the provider request completion time on usage anchors so a later transcript write cannot displace the same request snapshot and its metered window. Preserve legacy anchors and bump the runtime-host compatibility epoch for the new field. Generated-by: Codex
The stale reading keeps the gauge chip on its localized Usage label with the compacted-context tooltip; it never rendered a question mark. Align the entry with the shipped ContextUsageAction. Generated-by: Codex
Describe what resolveContextUsage consumes (live Turn snapshot and durable transcript anchors/notes) and the ordering rule it applies after a compaction. Generated-by: Codex
3c45c23 to
0208b53
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 7739e117 (our last reviewed head) to 0208b53f (5 commits, 23 files, +545/-159). The PR was rebased from merge-base a8b8d503 onto dcef6427, which is current main. Comparing (old head vs its merge-base) with (new head vs its merge-base), the only substantive changes are:
0d0ada18rewrites the CHANGELOG entry so it describes the localizedUsagelabel rather than?. This fixes the earlier P3.0208b53frewrites the doc comment onresolveContextUsage. There is no logic change.- The rebase adapts the renderer to the Composer reads that moved below the shell on main.
conversation-workspace.tsnow memoizes the object-valuedselectLatestRequestUsageresult permessagesidentity, souseSyncExternalStorekeeps a stable snapshot.conversation-readers.tsxandconversation-owner.test.tsnow carrylatestRequestUsage. These look correct. - The compat epoch moves to 205. That is correct, because
mainis at 204.
The selector, resolver, runtime anchor stamping and telemetry are byte-identical to 7739e117 apart from that doc comment.
Prior findings
- hqhq1025 P2 (same-request anchor vs snapshot ordering) and P3 (a pre-fold live snapshot overriding a post-fold anchor): fixed, as already confirmed at
7739e117. - Astro-Han P2 (a stale reading shows as measured when the fold postdates the anchor): not fixed. The code is unchanged; see the inline comment.
- P3 CHANGELOG
?: fixed. - P3 stale tooltip not asserted: still open.
composer-context-usage.test.tsx:145only checks theUsagelabel, so a stale reading and an unavailable reading are indistinguishable in tests. - P3 note/row write gap and the CHANGELOG wording about mid-turn recovery: unchanged, and still non-blocking.
One P2 (carried over). No new findings in the delta.
CI test passes on this head. GitHub reports MERGEABLE (merge state BLOCKED). git merge-tree against main and git diff --check are both clean. Not run locally: the test suites (CI covers them on this head), a real Electron UI, and an end-to-end runtime replay of the P2 sequence. I traced that sequence by hand through the unchanged selector and resolver. This is not merge approval.
| if (!Number.isFinite(anchor.inputTokens) || anchor.inputTokens <= 0) return undefined; | ||
| const output = Number.isFinite(anchor.outputTokens ?? 0) ? Math.max(0, anchor.outputTokens ?? 0) : 0; | ||
| return anchor.inputTokens + output; | ||
| return { |
There was a problem hiding this comment.
P2 (carried over from the 7739e117 review, still unfixed): a fold that postdates the anchor still renders as measured, which is the #5547 symptom. Settlement appends the context_compacted note before the token_usage row (packages/runtime/src/ai-sdk-turn.ts:2416-2418). The row's anchor tokens come from lastStepInputTokens, which only advances when a step finishes (ai-sdk-turn.ts:1922), and its completedAt comes from the last completed main request. Settlement also persists usage before it rethrows terminalProviderError (:2432). Take this sequence:
- step 1 succeeds at T1;
- step 2 overflows;
- recovery compaction succeeds;
- the retry fails.
The ledger then ends with note(ts=T3) followed by token_usage(anchor.completedAt=T1). This loop scans backwards, hits the token row first and returns {kind:'tokens', at:T1} without ever seeing the newer note. resolveContextUsage then yields measured, either from the anchor or from the T1 live snapshot. Neither is stale.
Suggested fix: when the anchor carries completedAt, keep scanning for a context_compacted note with ts > anchor.completedAt (same turnId), and return compacted in that case. A fix in the selector is preferable to dropping the anchor at runtime, because the runtime also uses that anchor for the next turn's proactive fold decision. Please add a resolver test for the order "note before row, anchor older than the note".
Summary
After successful compaction, the context gauge displays its existing gauge icon with the localized
Usage/用量label instead of retaining the pre-compaction percentage. The tooltip explains that usage will update when the next request completes (简体中文:上下文已压缩,用量将在下一次请求完成后更新。), with matching Traditional Chinese and English copy. A newer provider measurement restores the reading, including during an ongoing turn; failed-open compaction preserves valid usage.Carry explicit unavailable/measured/stale readings through the conversation and WorkHub composers, and use request completion timestamps to reject stale live snapshots. Keep measured tokens paired with their metered context window.
Fixes #5547
Before:

After:

Verification
Earlier validation recorded for the initial implementation (before the label and tooltip refinement):
npm run lintnpm run format:checknpm run buildnpm run typechecknpx knip --workspace apps/desktopnpx knip --workspace packages/uigit diff --check upstream/main...HEADnode --test apps/desktop/dist/main/__tests__/latest-request-usage.test.js apps/desktop/dist/main/__tests__/live-context-usage.test.js packages/ui/dist/__tests__/composer-context-usage.test.js— 35 tests passed, 0 failed.Validation for the label and tooltip refinement:
git diff --checkpassed.Component assertions verify that both stale and unavailable usage render
Usage, a subsequent measured reading restores10%, and trace opening and window precedence still work. No manual UI screenshot/recording or full workspace test-suite run was performed in this submission. Tests were not rerun against the unmodified base.AI use
Tool(s) and scope: OpenAI Codex inspected the existing implementation, updated the post-compaction label and localized tooltips, adjusted the component assertion, ran the validation described above, and updated this PR and the linked issue. Earlier implementation tooling provenance was not independently verified.
Checklist
Does this PR entail a change in behavior?