Skip to content

fix(chat): correct context footer usage snapshots - #1473

Draft
karkarl wants to merge 2 commits into
mainfrom
fix/chat-context-footer-usage-1433
Draft

karkarl wants to merge 2 commits into
mainfrom
fix/chat-context-footer-usage-1433

Conversation

@karkarl

@karkarl karkarl commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • stop adding each assistant response's overlapping request total to the previous footer snapshot
  • distinguish fresh sessions.list usage snapshots from activity-only cached session republishes
  • preserve authoritative equal, lower, and compacted corrections without allowing replayed terminal frames to undo them

Fixes #1433

Required proof pools

  • windows-winui-interactive: chat footer context usage display changed; maintainer-scheduled visual proof can verify the rendered footer in the isolated WinUI app.

Validation

Current head: cceafd9ae7269dde4b600f9f1a2a140fabfbe672

  • $env:OPENCLAW_REPO_ROOT=(Get-Location).Path; .\build.ps1 - passed. All builds succeeded and documentation validation checked 50 Markdown files.
  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore - passed. Failed: 0, Passed: 3068, Skipped: 0, Total: 3068.
  • Focused chat/context footer tests - passed. Failed: 0, Passed: 441, Skipped: 0, Total: 441.
  • Focused fresh-session provenance test - passed. Failed: 0, Passed: 1, Skipped: 0, Total: 1.
  • Full Shared tests ran twice. Each run passed 4093 and skipped 32, but the unrelated McpHttpServerTests.Dispose_DuringInFlightHandler_DoesNotSurfaceObjectDisposedException race failed under full-suite load. The same test passed when rerun alone. Exact-head CI remains the authoritative closeout for that order/load-dependent failure.
  • Rubber-duck review found no blocker. One observation was accepted by narrowing replay suppression so equal-usage frames can still refresh changed context metadata.
  • Project autoreview was attempted twice. The exact-commit bundle was valid, but the reviewer engine failed authentication with HTTP 401, so no structured autoreview result is claimed.

Real behavior proof

  • Fresh sessions.list parsing now emits SessionUsageSnapshotUpdated; tool/job activity continues to emit only generic SessionsUpdated, so cached activity cannot reconcile footer usage.
  • Regression coverage proves activity-only cached updates cannot replace newer per-message usage either before or after an authoritative correction.
  • Regression coverage proves a duplicate terminal frame cannot lower a newer authoritative total after reconciliation.
  • Existing coverage still proves genuine equal and lower authoritative snapshots correct totals and stale percentages.
  • Latest assistant context metadata can still refresh when usage is unchanged.

Not verified / blocked: visual windows-winui-interactive proof was not run locally for this draft; automated focused tests cover the footer accounting state that drives the UI text.

Stop accumulating overlapping assistant request totals for the chat context footer. Let authoritative session usage reconcile lower totals and clear stale percentages while cached final-message fallbacks preserve newer per-message usage until the session snapshot arrives.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@karkarl karkarl added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 23, 2026
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 23, 2026
@clawsweeper

clawsweeper Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 25, 2026, 7:41 PM ET / 23:41 UTC (Revision 2).

ClawSweeper review

What this changes

The branch changes the Windows chat footer to use the latest assistant request total and accept session-list usage corrections, with a separate event for fresh list responses and focused regression tests.

Merge readiness

⛔ Blocked before merge - 4 items remain

Keep open. Current main and the latest release retain the reported footer defect. The follow-up addresses the two earlier review findings, but the branch still has a source-proven ordering gap and incomplete required validation.

Priority: P2
Reviewed head: cceafd9ae7269dde4b600f9f1a2a140fabfbe672

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Focused regression coverage improves confidence, but the ordering finding and incomplete required Shared validation keep the patch below merge-ready quality.
Proof confidence 🦐 gold shrimp (3/6) Not applicable: The changed production path runs from Gateway usage events through conversation state to the WinUI footer. The collaborator-authored PR reports focused tests but no current-head visual run, and explicitly records the interactive proof-pool blocker; the external-contributor proof gate does not apply. No stored-data contract changes.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The changed production path runs from Gateway usage events through conversation state to the WinUI footer. The collaborator-authored PR reports focused tests but no current-head visual run, and explicitly records the interactive proof-pool blocker; the external-contributor proof gate does not apply. No stored-data contract changes.
Evidence reviewed 9 items Introduced reconciliation: The new authoritative path assigns a session-list total to the latest assistant entry without comparing the response's age with a later assistant frame.
Gateway event provenance: Parsing a sessions.list response now emits both the general session update and the new usage-snapshot event; session-change notices can start asynchronous list requests.
Provider path: The provider applies fresh-list usage to conversation state after the general session update and publishes the resulting footer snapshot.
Findings 1 actionable finding [P2] Reject stale session snapshots before replacing live usage
Security None None.

How this fits together

The Gateway supplies assistant usage frames, session-list responses, and activity updates to the Windows chat provider. The provider reconciles those values into conversation snapshots that the WinUI chat footer formats as context usage.

flowchart LR
A[Assistant usage frames] --> D[Usage reconciliation]
B[Session list responses] --> D
C[Cached activity updates] --> E[Conversation snapshot]
D --> E
E --> F[WinUI context footer]
Loading

Before merge

  • Reject stale session snapshots before replacing live usage (P2) - If a sessions.list request captures 1,000 tokens, a 1,500-token assistant final arrives, and the delayed list reply then arrives, this new authoritative assignment rewinds the footer to 1,000. The response's age is never compared with the assistant frame. Add an out-of-order regression and preserve legitimate lower corrections only when the list result is newer.
  • Resolve merge risk (P1) - A delayed session-list response can replace usage from a newer assistant frame because the new authoritative path has no request or event ordering check.
  • Resolve merge risk (P1) - The required full Shared test suite failed twice under load on this head; an isolated passing rerun does not complete the required full-suite closeout.
  • Complete next step (P2) - Guard delayed session-list responses against newer assistant usage, add an out-of-order regression, and complete a passing required Shared suite closeout before merge.

Findings

  • [P2] Reject stale session snapshots before replacing live usage — src/OpenClaw.Tray.WinUI/Chat/ChatConversationState.cs:531-533
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Code and test delta production +48 net lines, tests +227 net lines The production growth establishes usage-event provenance and is accompanied by focused regression coverage.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #1433
Summary: This PR explicitly proposes the fix for the linked, still-open chat footer bug.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Fence delayed list responses (recommended)
    Track enough request or event chronology to reject a list result older than the latest assistant frame, then cover that ordering alongside genuine lower corrections.

Technical review

Best possible solution:

Order session-list snapshots against live assistant usage, retain legitimate lower corrections such as compaction, and verify the corrected footer in an isolated WinUI run when the scheduled pool is available.

Do we have a high-confidence way to reproduce the issue?

Yes. Source supports a deterministic bridge sequence: emit a newer assistant final, then deliver an older delayed session-list response; this read-only review did not run it.

Is this the best way to solve the issue?

No. Separating cached activity from list responses addresses the earlier findings, but list provenance alone cannot establish that a response is newer than an assistant frame.

Full review comments:

  • [P2] Reject stale session snapshots before replacing live usage — src/OpenClaw.Tray.WinUI/Chat/ChatConversationState.cs:531-533
    If a sessions.list request captures 1,000 tokens, a 1,500-token assistant final arrives, and the delayed list reply then arrives, this new authoritative assignment rewinds the footer to 1,000. The response's age is never compared with the assistant frame. Add an out-of-order regression and preserve legitimate lower corrections only when the list result is newer.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.83

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5a59535216ee.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: Incorrect context occupancy is a limited user-facing chat defect without evidence of blocked messaging or runtime outage.
  • merge-risk: 🚨 other: Merging the unordered reconciliation path could rewind the visible context count after a newer assistant response.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The changed production path runs from Gateway usage events through conversation state to the WinUI footer. The collaborator-authored PR reports focused tests but no current-head visual run, and explicitly records the interactive proof-pool blocker; the external-contributor proof gate does not apply. No stored-data contract changes.

Evidence

What I checked:

Likely related people:

  • the99missedcalls: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Barbara Kudiess: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • shanselman: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add an out-of-order session-list regression and reject snapshots older than the latest assistant usage.
  • Complete a passing required full Shared suite closeout on the current head.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-23T01:21:55.630Z sha d83e864 :: needs changes before merge. :: [P2] Distinguish fresh usage snapshots from cached activity updates | [P2] Prevent duplicate final frames from undoing session reconciliation

@shanselman

Copy link
Copy Markdown
Collaborator

Maintainer follow-up on this draft at d83e864016ff: the direction is useful, but an activity-only cached SessionsUpdated can still replace a fresher per-message usage contribution after reconciliation. Please guard the newer contribution when that cached event carries no authoritative usage, and add a focused regression for activity-only update after the authoritative value; a duplicate terminal event after reconciliation is also worth covering. The identityless production reachability remains disputed, so I am not treating it as a second proven defect. I am leaving the draft with its author rather than taking it over; once the narrow behavior tests and current-head checks are green, we can reassess it for landing.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@karkarl

karkarl commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator Author

@shanselman Addressed the unresolved feedback in additive commit cceafd9. Fresh sessions.list usage now travels through a separate SessionUsageSnapshotUpdated event, while activity-only cached republishes remain presentation-only and cannot reconcile footer usage. The contribution marker is retained across authoritative reconciliation so a replayed terminal frame cannot lower the corrected total. Added focused regressions for activity-only updates before and after authoritative correction, duplicate finals after an intervening correction, and fresh-event provenance. Build, 3068 Tray tests, and 441 focused chat/context tests pass. The full Shared suite still exposes an unrelated order/load-dependent McpHttpServerTests.Dispose_DuringInFlightHandler_DoesNotSurfaceObjectDisposedException failure, while that test passes alone; exact-head CI is running. There are no inline review threads on this PR to resolve.

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. label Sep 25, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Chat context footer accumulates overlapping request totals and cannot correct inflated usage

2 participants