Repository navigation
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.
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.
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.
|
@Astro-Han Thanks for the follow-up review. Here is the status of the outstanding findings:
|
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: 0208b53f (our last reviewed head) to bc17646d (2 commits: 2acb4249 "order usage anchors against the compaction apply time", bc17646d "assert context usage tooltips"; 13 files, +687/-37). The merge-base is unchanged (dcef6427, current main), so comparing (old head vs its merge-base) with (new head vs its merge-base) gives exactly 0208b53f..bc17646d.
What changed. The runtime now records a new hidden system note, context_compaction_applied, at the moment each compaction is applied. The hook is onCompactionApplied in compactActiveRequestHistory (ai-sdk-compaction.ts:1206-1213), and it fires from both the capacity hook and recoverFromOverflowError. The manual path (runtime-kernel.ts:1311) records it too. The settlement anchor's completedAt now falls back to the step-finish time when no tracker is wired (ai-sdk-turn.ts:1935, :2375).
Both readers stop trusting ledger position alone. They hold the newest anchored row as a candidate and let a boundary behind it supersede it when the boundary time is at or after anchor.completedAt. One reader is the renderer selectLatestRequestUsage. The other is the runtime persistedRequestAnchor, which seeds the next turn's step-0 baseline. A settlement context_compacted note borrows the time of the same turn's applied note. The applied note is filtered out of the visible transcript through isUserVisibleSessionSystemNote.
Prior findings
- P2 (#5547: a stale reading shows as measured after step success, overflow, compaction, then a failed retry): fixed. I traced the ledger
applied(T2), context_compacted(T3), token_usage(anchor.completedAt=T1). The scan holds the T1 candidate. The display note resolves to T2 throughlatestCompactionAppliedAt, and T2 >= T1 givescompacted. A T1 live snapshot then resolves to stale because it predates T2. The healthy case isapplied(T2), a retry that completes at T4, then the note and the row. There T2 < T4, so the reading stays measured. Supporting checks:ProviderRequestTrackeronly advanceslatestCompletedMainRequestAtforcompletedmain-call attempts (provider-request-telemetry.ts:624-627), so a failed retry cannot move the anchor past the boundary.- The tracker, the turn and
AgentRun.recordSystemNoteall stamp with the Host'sdeps.now. - Every
activeStep"replaced" decision, which is what gates the display note inshouldAppendContextCompactedNote, comes fromcompactActiveRequestHistory. So no display note is written without an applied note. mainnever stampsanchor.completedAt, so legacy anchors cannot be superseded and existing sessions keep their measured reading.
- P3 (stale/unavailable tooltip not asserted): fixed.
composer-context-usage.test.tsxnow asserts all three tooltip texts. - P3 (note/row write gap; CHANGELOG wording): unchanged. Still non-blocking.
Tests: 16 new selector/resolver cases (latest-request-usage.test.ts). They include the exact #5547 ledger, a double apply in one turn, a cross-turn apply row, a usage row between apply and note, and tied or missing times. There are 5 runtime cases in mid-turn-capacity-backend.test.ts, 2 in overflow-reactive-recovery.test.ts (failed and completed retry), and a materialize test that the applied note stays hidden. The runtime side now also drops a pre-compaction anchor from the step-0 baseline. I think that is right, because that size no longer describes what the next request sends.
New findings
- P3 (inline):
protocol/index.ts:108. The epoch-205 comment only mentionscompletedAt. The PR also adds a storedsystem_notekind. Stored-message decode is strict on kind (core/session.ts:1730-1736, "Invalid stored message schema"), so a pre-PR peer cannot read a transcript that contains it. The bump covers it, but the comment should say so.
Coordination (unchanged): epoch 205 is also claimed by #5709 and #5902, and #3700 now claims 207. Whichever merges later must renumber.
No P0-P2 open. CI: the test job on this head is queued/pending (run 37345116104), so there is no result yet. GitHub reports MERGEABLE (merge state BLOCKED). git merge-tree against main and git diff --check are both clean. Not run locally: the test suites, typecheck, the Electron UI, and an end-to-end runtime replay. I traced the ledger sequences by hand. This is not merge approval.
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: bc17646d (our last reviewed head) to 0e9d5331. Two commits: 96bad29b "cover the compaction note kind in the epoch-205 note" and 0e9d5331 "name the compaction apply-time boundary row". 3 files, +17/-14. The PR's merge-base is still dcef6427. main has since moved one commit to 3597abe8 (#5866), which takes epoch 205. Comparing (old head vs its merge-base) with (new head vs its merge-base) gives exactly bc17646d..0e9d5331. No runtime or renderer code changed in this delta.
Prior findings
- P3 (the epoch-205 comment did not mention the new note kind): fixed in wording.
protocol/index.ts:108-111now says the transcript may carrycontext_compaction_appliedsystem notes and that older peers reject them. But the commit also added a compatible-change declaration to get the comment edit past the pre-commit guard. That declaration is the subject of the new P2 below. - P3 (CHANGELOG wording): fixed. The entry now names the
context_compaction_appliedboundary row, separates it from thecontext_compacteddisplay row, and is shorter. It matches the implementation. - P3 (note/row write gap): unchanged. Still non-blocking.
New findings
- P2 (blocking): epoch collision with
main, and the new declaration disables the guard that would catch it. #5866 merged withRUNTIME_HOST_COMPATIBILITY_EPOCH = 205and its own 205 note. GitHub now reports this PR as CONFLICTING/DIRTY.git merge-tree --write-tree origin/main 0e9d5331conflicts inpackages/runtime-host/src/protocol/index.ts.- The worse part is how a naive rebase would resolve.
protocol-compatible-changes/epoch-note-compaction-kind.json(epoch: 205,files: [protocol/index.ts]) is added against base, andindex.tsis the PR's only file undersrc/protocol/. Keep 205 and append this PR's note under #5866's, andscripts/protocol-epoch-check.mjsreturns ok ("declared compatible extension at epoch 205"). I checked this by runningevaluateEpochCheckfrom this head with base=205, head=205, and the declared file. - The PR's real wire changes are in
packages/core/src/session.tsandusage-record-schema.ts:lastRequestAnchor.completedAtand the new strictsystem_notekind. Both are outside the guarded directory, so two incompatible protocols would ship as 205. That is the #3313 failure mode the guard exists to stop. - The declaration also contradicts the README's rule: "any new or removed field, key, error code, or accepted value — those move the epoch". It was needed only because the
--stagedpre-commit check judges the comment-only commit against HEAD. - Fix: rebase onto current
main, deleteepoch-note-compaction-kind.json, and bump to the next free epoch. 206 is claimed by #4138 and 207 by #3700, so 208 unless those are renumbered. Put this PR's note above #5866's 205 line. A real bump needs no declaration, and keeping one pinned to 205 would failparseDeclarationafter the bump anyway. If you squash, the comment edit and the bump land together and the staged hook is satisfied without a declaration.
- The worse part is how a naive rebase would resolve.
CI: test SUCCESS on this head (run 37357103476). That result predates the #5866 conflict, so it does not cover the merge result. git diff --check is clean on the delta. Not run locally: the test suites, typecheck, and the Electron UI. The delta contains only docs and a declaration. This is not merge approval.
0e9d533 to
c8c2ff8
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: 0e9d5331 (our last reviewed head) to c8c2ff8f. The branch was rebased: the merge-base moved from dcef6427 to 3597abe8, which is current main and includes #5866. To compare, I diffed the old head against its merge-base and the new head against its merge-base. Those two patches differ only in the protocol epoch area. All runtime, renderer, UI, test, and CHANGELOG hunks are identical: 29 files, +1205/-171 now, versus 30 files, +1210/-171 before.
Prior findings
- P2 (epoch 205 collided with #5866, and the compatible-change declaration bypassed the guard): fixed.
protocol-compatible-changes/epoch-note-compaction-kind.jsonis gone. It is not in the head tree and not in any commit in3597abe8..c8c2ff8f.packages/runtime-host/src/protocol/index.ts:107now bumps205 -> 208. This PR's history note (:108-111) sits above #5866's// 205:note, which is kept unchanged at:112.- Rebase conflict resolution is correct: nothing from main was dropped, and
git merge-treeagainst current main is clean. - I ran this head's own
scripts/protocol-epoch-check.mjs --base 3597abe8 --head c8c2ff8f. It reports "Protocol changed and the epoch moved: 205 -> 208" and exits 0. The script's tests pass (17/17).
- P3 (note/row write gap): unchanged. Still non-blocking.
New findings
- P3 (coordination, non-blocking): #5709 also claims 208. I swept all 222 open PRs. Above 205, the claims are #4138 at 206, #3700 at 207, and both #5709 and this PR at 208. Nothing claims 209 or higher. Our previous review suggested 208 here before #5709 moved to 208, so the duplicate is partly our doing. Whichever of #5548 and #5709 merges second must re-bump to the next free epoch (209 if #4138 and #3700 are still open) and keep the other PR's note. The epoch guard will not catch this by itself after the first merge, because the second PR would still show a move from base. Maintainers should watch for it.
- Nit: commit
febd4c13is still titled "...in the epoch-205 note". The repo is squash-merge-only, so this only matters if the commit list ends up in the squash body.
CI: test is still pending on this head (run 37397051682), so there is no green result yet for the rebased tree. GitHub reports MERGEABLE, but merge state is BLOCKED (review required). git diff --check is clean on the PR diff. I did not run the test suites, typecheck, or Electron UI locally. This review is not merge approval.
c8c2ff8 to
5659fb9
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: c8c2ff8f (our last reviewed head) to 5659fb9b. The branch history was rewritten on the same base. Both heads have merge-base 3597abe8, which is current main. I diffed each head against its merge-base. The two patches are identical except for the epoch constant and its history note in packages/runtime-host/src/protocol/index.ts: 208 is now 206. All runtime, renderer, UI, test, and CHANGELOG hunks are unchanged (29 files, +1205/-171).
Prior findings
- P3 (#5709 also claimed 208): resolved by moving off 208, but the move creates a new collision (see below).
- P3 (note/row write gap): unchanged. Still non-blocking.
- Nit (commit
febd4c13title said epoch-205): fixed. The rewritten commit8d2775e2is titled "...in the epoch-206 note". Its body no longer has the explanation orRefs #5547. The repo is squash-merge-only, so this does not matter.
New findings
-
P3 (coordination, non-blocking): epoch 206 duplicates #4138.
packages/runtime-host/src/protocol/index.ts:107now bumps205 -> 206. I swept the 24 open PRs that touch this file. Above main's 205, the claims are #4138 at 206 (CONFLICTING, last updated 2026-10-04), this PR at 206, #3700 at 207, and #5709 at 208. Nothing claims 209 or higher. The bump is valid againstmain, and CI's guard agrees ("Protocol changed and the epoch moved: 205 -> 206"; guard tests 17/17). #5866's// 205:note is kept just below this PR's note (:108-111). Options:- Move to
209, the first epoch no open PR claims. - Keep 206 if maintainers treat #4138 as stale.
Either way, whichever of #5548 and #4138 merges second must re-bump and keep the other PR's note. The guard cannot catch this after the first merge.
- Move to
CI
- Run 37399704288 failed in one step,
test/Desktop e2e: 1 failed, 33 passed. - The failing test is
e2e/workhub-layout.spec.ts:26("WorkHub uses its coordination model and shared attachment composer"). It failed at:150withjsHandle.evaluate: Target page, context or browser has been closed. That is the Electron main process dying during the native workbar-menuclosePopupon Linux, which is the known flake. - This PR does not cause it:
- The previous head
c8c2ff8fpassedtest(run 37397051682). Its code is byte-identical to this head apart from the epoch number. - The PR's only WorkHub change (
workhub-root.tsx) changes which value the renderer's context-usage gauge reads. It does not touch the native menu or the main process. - Every other step passed, including the epoch guard. Main's latest CI (
3597abe8) is green.
- The previous head
- A re-run of the failed job should be enough.
GitHub reports the PR as MERGEABLE, with merge state BLOCKED (review required). git merge-tree against main is clean, and git diff --check is clean. I did not run the test suites, typecheck, or Electron UI locally. This review is not merge approval.
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: 5659fb9b (our last reviewed head) to 11b1a818. The merge-base is unchanged (3597abe8, current main), and two commits were added on top:
23fd022e"fix(runtime): forward compaction boundaries while a turn is running"11b1a818"fix(protocol): advance compatibility epoch past open claims"
The delta is 27 files, +681/-126.
Prior findings
-
P3 (epoch 206 duplicated #4138): resolved.
packages/runtime-host/src/protocol/index.ts:107now bumps205 -> 209, and the note above #5866's// 205:line now also covers the new subscription frame. I re-swept all 221 open PRs' epoch constants. Above main's 205, the claims are:209 is unique today. As before, any later PR has to bump past whichever lands first.
-
P3 (mid-turn fold invisible until settlement; CHANGELOG overclaimed): resolved by
23fd022e. The boundary is now emitted live, and the CHANGELOG entry describes the new behaviour accurately.
Delta review (23fd022e)
- Runtime.
recordCompactionApplied(ai-sdk-turn.ts:1056-1066) now callsqueue.pushAndWaitUntilConsumedwith acontext_compaction_appliedSessionEvent instead of callingrecordSystemNotedirectly. The consumer maps that event to the same hiddensystem_note(session-event-runtime-mapper.ts, using the event's id and ts) and acks after persisting it.- The queue rejects when the consumer has detached, the queue is closed, or it has errored. The old helper was fail-open, but the only caller already wraps the hook in try/catch (
ai-sdk-compaction.ts:1209-1213), so a stop or detach still cannot undo a compaction. - The same pattern is already used for steering (
:3068). - The new
mid-turn-capacity-backendcase holds request 3 open and asserts, in both consumer modes, that the boundary has already been emitted and persisted exactly once with a matching id, ts andhiddenvisibility. That rules out a deadlock from awaiting the ack inside step preparation. The write-failed and double-fold cases also assert 0 and 2 boundaries respectively.
- The queue rejects when the consumer has detached, the queue is closed, or it has errored. The old helper was fail-open, but the only caller already wraps the hook in try/catch (
- Protocol.
SessionEventFrame.eventgainsSessionContextCompactionAppliedEvent, decoded with exact keys (session-continuity.ts:812). It is forwarded byisRuntimeSessionForwardedEventand projected identically on the Host and in the desktop adapter. Older peers reject it under the closed decoder, so the epoch bump is needed and the note says so. - Host continuity. The event takes the generic forward path. Like
steering_message, it clears any provider-retry live state first, which is acceptable. - Renderer.
- The live tracker records
appliedAtfrom the event and immediately reports{kind:'compacted'}. It keeps reporting that until a diagnostics read withat > appliedAtarrives.appliedAtresets on a target change. resolveContextUsagegives a live boundary priority unless a durable measurement is strictly newer, which is symmetric with the existing durable-boundary rule.- The new event type is ignored by the live-turn projection (
live-turn-projection.ts), the conversation reducer andprojectDesktopSessionEventthrough their default branches. - The hook subscribes per Session, so another Session's boundary cannot hold this gauge.
- The live tracker records
- Refactor.
LiveContextUsageand the localLatestRequestUsageshape are unified into oneContextUsageSnapshotunion.meteredWindowis renamed tocontextWindow, and no stale references remain.
New findings: none at P0-P3.
CI: test is still in progress on 11b1a818 (run 37483332180). The run on 23fd022e was cancelled when it was superseded, and 5659fb9b passed. So this head has no green result yet. GitHub reports MERGEABLE, with merge state BLOCKED (review required). git merge-tree against main and git diff --check are both clean. I did not run the test suites, typecheck, or the Electron UI locally. This review is not merge approval.
11b1a81 to
23e5262
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
Record context_compaction_applied when a compaction lands so the boundary carries the apply time rather than the turn's settlement time, and persist completedAt on the settlement usage anchor even without provider telemetry. A send that compacts mid-turn and never completes its retry settles the usage row after the compaction notes while the anchor still describes the pre-compaction request, so readers arbitrate the newest anchored row against a boundary found behind it by event time. The applied row is hidden from the user-facing notes; context_compacted stays the display row. Refs apache#5547 Generated-by: Codex
…dings The share-resolution test only checked the visible label, so a stale reading and an unavailable reading were indistinguishable. Resolve the button aria-describedby to its tooltip element and assert the copy for the stale, measured, and unavailable states so the tooltip follows the reading across transitions in the same mounted control. Generated-by: Codex
Generated-by: Codex
The gauge entry credited context_compacted alone, but the mid-turn boundary is the context_compaction_applied row recorded when a compaction lands; context_compacted stays the settlement-time display note. Refs apache#5547 Generated-by: Codex
A mid-turn compaction only reached the composer gauge once the turn settled and the transcript refreshed, so the pre-compaction figure stayed on screen for the rest of a long turn. The runtime now emits a context_compaction_applied event when a compaction lands, persists it as the hidden boundary note, and the Runtime Host forwards it over the session subscription. The live context usage tracker holds the gauge on the compacted reading until a measurement that settled after the boundary arrives. Live and transcript readings share one ContextUsageSnapshot union, and the measured reading's window is renamed to contextWindow. Refs apache#5547 Generated-by: Codex
Preserve the upstream measured and unavailable tooltip copy in the post-compaction component assertions. The branch compatibility bump was already reconciled while replaying the protocol changes: epoch 214 is ahead of main at 212 and the 213 claims in open PRs apache#5394 and apache#5969. Generated-by: Codex
23e5262 to
dc7a4fd
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.
I checked the increment from 11b1a818b8f3d8031f66ecd9bcac0ba4a828f00c to dc7a4fdd4b89b4845349b5543c56c2eb81a9b042, including the current PR diff, range-diff, compaction boundary persistence/forwarding, context-reading arbitration and the composer consumers. The merge base moved from 3597abe8 to current main 2463d86c; the large raw old/new diff includes main changes. The branch's current author diff is 44 files, +1,785/-196. Most patches are retained; meaningful reconciliation adapts main's consumers and shortened tooltip text, and advances the epoch. The final new commit changes only two tooltip expectations, without weakening the assertion.
Previous findings remain resolved. Successful compaction emits an apply-time boundary, waits for its consumer acknowledgment before the replacement request, persists a hidden note and forwards the boundary during a running turn. The live tracker retires an older reading immediately and restores one only when its completion time is newer. Failed-open compaction does not manufacture a successful boundary. The composer and WorkHub use the same reading resolver; stale and unavailable states retain Usage with different tooltips, and a newer measurement restores the percentage. The changelog accurately distinguishes apply-time and settlement-time notes.
Epoch: no current collision. packages/runtime-host/src/protocol/index.ts:107 declares 214, covering lastRequestAnchor.completedAt, the hidden note and the live subscription event. Main remains 212. I read the epoch constant at all 209 open PR heads in the current listing, not only files returned in a truncated change list. Above main, the claims are 213 for #5394 and #5969, and 214 for this PR alone. This PR needs no renumbering now. 215 is the next unclaimed value in that snapshot, but merge ordering must be rechecked before using it; these values are not permanent reservations.
New findings: none verified at P0–P3 in this increment. Six Core/Storage/MCP/Runtime/Host/UI builds, the computer-use dependency build and Desktop main build pass. Desktop preload/main/renderer/Storybook typechecks pass. Locally, 453 focused Core/Host/Runtime/UI tests and 67 Desktop reader/tracker/trace/owner tests pass, with no skips. These cover the held replacement request with a slow ledger consumer, failed-open behavior, timestamp ordering, closed protocol decoding, hidden-note projection and mounted tooltip transitions. The epoch guard passes for 212 → 214; a merge check against current main and git diff --check are clean. There is no SQLite schema change in this increment.
CI is not green yet. Run 38030110725 failed its first attempt on the unchanged UsageLongTail and UsageNarrow stories: 168.03125 pixels did not fit a 168-pixel cell. This PR changes neither that Usage column nor those stories. Main's successful run 38029499971 skipped Storybook build/smoke, so it is not a passing baseline for that surface. I have not established that this PR caused the failure and do not assign it a new P2. The same exact-head run is currently on attempt 2, in progress.
Not run locally: the full monorepo suite, Storybook rendering/pixel comparison, real Electron or a live provider. The runtime tests use deterministic model and consumer fixtures, not production performance measurements. This is not merge approval.
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?