Skip to content

refactor(chat): extract runtime state owners - #1096

Open
bkudiess wants to merge 3 commits into
mainfrom
bkudiess-chat-runtime-bundle
Open

refactor(chat): extract runtime state owners#1096
bkudiess wants to merge 3 commits into
mainfrom
bkudiess-chat-runtime-bundle

Conversation

@bkudiess

@bkudiess bkudiess commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Reduce OpenClawChatDataProvider from 7,249 to 1,728 lines and leave it as the same public IChatDataProvider facade for bridge IO, telemetry, events, and composition.
  • Move the sole atomic runtime lock into ChatConversationState.
  • Split coherent lock-internal mechanics into lock-free queue, reset, history, presentation, lifecycle, and approval substates.
  • Extract history fetch/retry ownership, queue decision policy, metadata persistence, last-state persistence, event mapping, content formatting, and snapshot projection.
  • Preserve Reactor UI/composer/timeline ownership and integrate current-main theme, bottom-anchoring, checkpoint-detail, and flattened session-classification behavior.

Ownership transfer

New owner Responsibility Locking boundary
ChatConversationState Timelines, entry metadata, and cross-domain atomic transactions Sole runtime lock
ChatQueueState + ChatSendQueuePolicy Queue/local-echo/drain/retry mechanics and decisions Lock-free substate, invoked under root lock
ChatResetState Reset generations, cutoffs, accepted/ignored runs, buffered starts, backfill gates Lock-free substate
ChatHistoryState Session identity, revisions, connection/replacement tokens, commit acceptance Lock-free substate; one authoritative generation
ChatHistoryLoader Bridge fetch lifetime, CTS, in-flight coalescing, retry scheduling, rebuild plans Independent IO lifetime; root atomically accepts plans
ChatPresentationState Sessions, models, catalog, remembered state, immutable projection inputs Lock-free substate
ChatLifecycleState Active runs, aborts, lifecycle sequence, terminal dedupe Lock-free substate
ChatApprovalState Bounded approval identity correlation/dedupe Lock-free substate
ChatMetadataStore Tool/attachment caches, atomic persistence, marker security, reset eviction Independent metadata lock
ChatStatePersistence Aborted IDs and last-chat-state persistence Independent persistence lifetime
Mapper/projector/formatting owners Pure gateway-event mapping, immutable snapshot projection, content formatting Stateless

No substate owns a second runtime lock or duplicate reset/history generation. Provider source guards prevent _gate or mutable runtime collections from returning.

Validation

  • ./build.ps1: all projects passed
  • Shared: 3,382 passed, 32 skipped
  • Tray: 2,088 passed
  • Focused runtime/provider/history/reset/queue/metadata/persistence/mapper/projector/classification suites: 524 passed
  • Integration: 19 passed
  • Functional UI: 19 passed
  • Native UI: 100 passed
  • Accessibility: 19 passed
  • UTF-8, final newline, trailing whitespace, conflict-marker, owner guards, and git diff --check: clean

Validation was run after rebasing onto current main e9d571a9 and integrating #1069 flattened session classification plus #1091 compacted-history/checkpoint behavior into the new owners.

Real behavior proof

Validated exact D1 head 67d8a8b5 through a disposable real-gateway profile:

  • Operator and node reached connected/paired Ready state.
  • Authenticated local MCP exposed app.chat.send and app.chat.snapshot; no credential values were printed or retained.
  • A benign marker send returned sent=true, queue count 0, and snapshot polling observed one user echo, an active turn, then terminal state.
  • The disposable gateway lacked an OpenAI API key, so the real terminal path produced one assistant failure/Agent error rather than model chunks. This provider blocker is explicit rather than reported as streamed-model success.
  • Navigated away/remounted Chat to force real chat.history; the marker and terminal state replayed exactly once without duplication.
  • Stopped and restarted the real gateway with canonical E2E commands. The tray left Ready, recovered to Ready with both roles Connected, and a second history load retained the same marker exactly once.
  • Cleanup stopped only the proof tray, removed its profile, and unregistered its disposable distro. The shared tray/profile marker count remained zero and the shared gateway remained unchanged.

Redacted evidence is retained in d1-live-gateway-proof-67d8a8b5-20260805-204618.

Review

  • Claude Opus 4.8 exact full diff: clean
  • GPT-5.6 Sol exact full diff: clean
  • Rubber-duck: clean, specifically finding no replacement god object, fake split, duplicate generation, or current-main ownership regression
  • Final review fixes close concurrent-dispose first-wins, stale history retry accumulation, and stale retry displacement of current-generation work.
  • Bundled autoreview failed closed before model invocation because the complete bundle exceeded its 180,000-byte limit. No truncated or partial result was accepted.

Architecture

Adds authoritative/closed ledger rows and behavioral/source guards for the transaction root, queue/reset/history/presentation/lifecycle/approval substates, history loader, metadata and persistence owners, mapper/projector/formatting owners, and provider residue.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Aug 5, 2026
@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 6, 2026, 5:59 AM ET / 09:59 UTC.

ClawSweeper review

What this changes

The PR decomposes the tray Chat provider into focused conversation-state, history-loading, queue, reset, lifecycle, persistence, mapping, formatting, and snapshot-projection owners while retaining the public provider facade.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep open for human merge review. The refactor is not on current main, has no discrete source defect identified, but its final head changes reset and deferred-abort behavior after the only reported real-gateway proof head; as a collaborator-authored PR, it also requires explicit maintainer judgment.

Priority: P2
Reviewed head: 0e348722fc17daaabf5249e1acc8f821c44fc075
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The refactor has focused ownership and behavioral coverage, but current-head real behavior proof is still required before merge.
Proof confidence 🦐 gold shrimp (3/6) Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Evidence reviewed 6 items Current-main integration: The PR head has current main as its merge base and remains a 26-file unmerged change, so its runtime-owner refactor is not already implemented on main.
Runtime ownership boundary: The final head establishes ChatConversationState as the sole lock root and delegates reset, queue, history, lifecycle, and presentation mechanics to substates.
Architecture guards: The PR adds source-shape and behavioral ownership contracts covering the sole lock root, lock-free substates, history loading, and provider residue.
Findings None None.
Security None None.

How this fits together

Gateway history and events enter the tray Chat provider, which coordinates local conversation state and produces immutable snapshots for the WinUI chat timeline. Metadata and remembered state are persisted separately from the runtime transaction lock.

flowchart LR
A[Gateway events and history] --> B[Chat provider facade]
B --> C[Conversation state]
B --> D[History loader]
C --> E[Snapshot projection]
D --> C
E --> F[WinUI chat timeline]
C --> G[Metadata and state persistence]
Loading

Decision needed

Question Recommendation
Should this collaborator-authored runtime refactor proceed after current-head proof is attached, or should the earlier-head proof be accepted as sufficient? Require current-head proof: Request a redacted final-head gateway trace covering the new post-reset terminal lifecycle path before approving.

Why: The final branch changes central session-state behavior after its cited live proof, and collaborator-authored PRs require explicit maintainer judgment.

Before merge

  • Resolve merge risk (P1) - The exact final head adds post-reset lifecycle and deferred-abort behavior without published, inspectable real-gateway proof from that head.
  • Resolve merge risk (P1) - The local structured-review helper could not build its bundle because required promisor data was unavailable offline; this is review infrastructure evidence, not a claimed patch defect.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Files affected 26 files The branch redistributes a central Chat runtime surface across production and test code.
Production versus tests production +8,552, -6,368; tests +2,346, -155 The large ownership transfer includes substantial focused coverage rather than only source movement.
Latest hardening 7 files changed The final commit alters post-reset lifecycle and deferred-abort behavior after the cited gateway proof.

Merge-risk options

Maintainer options:

  1. Refresh final-head gateway proof (recommended)
    Run the existing disposable-gateway scenario on final head and attach redacted evidence for the new reset and terminal-lifecycle path.
  2. Accept the older proof
    A maintainer may accept the 67d8a8b proof despite the later runtime-state changes.

Technical review

Best possible solution:

Publish redacted disposable-gateway output from final head 0e34872 that exercises reset, reconciled local echo, terminal lifecycle completion, and deferred-abort fencing, then obtain collaborator-path maintainer approval.

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

Not applicable to the structural refactor itself. The final reset and lifecycle behavior has focused source and test coverage, but no high-confidence real-gateway reproduction from final head is available.

Is this the best way to solve the issue?

Yes for the architecture direction: the provider remains the facade while a single transaction root coordinates lock-free substates, with ledger entries and guard tests preventing responsibility drift.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This changes central Chat state behavior but does not establish an urgent confirmed user regression.
  • merge-risk: 🚨 session-state: Conversation, queue, reset, history, and lifecycle state move into new owners.
  • merge-risk: 🚨 compatibility: The established provider behavior is internally redistributed across multiple new components.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.

Evidence

What I checked:

Likely related people:

  • bkudiess: The current PR head is authored by this collaborator, and prior merged Chat work on the provider path includes the native-tool identity change. (role: prior and current Chat-area contributor; confidence: high; commits: 0e348722fc17, ad472243bcb8; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs, src/OpenClaw.Tray.WinUI/Chat/ChatConversationState.cs)
  • calebeden: Recent current-main Chat work covers compact tool activity and checkpoint-history behavior that this refactor preserves. (role: recent adjacent contributor; confidence: high; commits: aac07abb1fbd, d2de6150753b; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs)
  • dallinromney: Recent session presentation and flattened-classification changes affect the snapshots this refactor now projects. (role: recent adjacent contributor; confidence: medium; commits: cdcb74f44a21, 23414509b9d0; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs, src/OpenClaw.Tray.WinUI/Chat/ChatSnapshotProjector.cs)

Rank-up moves

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

  • Attach redacted disposable-gateway output from final head 0e34872 that exercises the post-reset terminal lifecycle path.

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 (7 earlier review cycles)
  • reviewed 2026-08-05T10:09:06.177Z sha 4301c4f :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T22:55:23.578Z sha 4301c4f :: needs maintainer review before merge. :: none
  • reviewed 2026-08-06T00:44:07.179Z sha 4301c4f :: found issues before merge. :: [P1] Adopt the current flattened session resolver
  • reviewed 2026-08-06T03:01:47.300Z sha 4301c4f :: found issues before merge. :: [P1] Use the current session display resolver
  • reviewed 2026-08-06T03:59:40.692Z sha 67d8a8b :: found issues before merge. :: [P1] Rebase the provider onto current session-state changes
  • reviewed 2026-08-06T05:20:31.520Z sha 67d8a8b :: found issues before merge. :: [P1] Preserve canonical session liveness in projected activity
  • reviewed 2026-08-06T08:52:31.344Z sha 61ed054 :: needs maintainer review before merge. :: none

@bkudiess

bkudiess commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Added exact-head real gateway proof to the PR body: authenticated app.chat.send, one user echo and terminal lifecycle, real chat.history remount without duplication, canonical gateway stop/start reconnect, and post-reconnect history preservation. The disposable gateway had no OpenAI API key, so the terminal assistant error path is documented rather than misreported as model streaming. All artifacts are redacted and the isolated profile/distro were removed.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. proof: sufficient Contributor real behavior proof is sufficient. 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. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. proof: sufficient Contributor real behavior proof is sufficient. labels Aug 5, 2026
@bkudiess
bkudiess force-pushed the bkudiess-chat-runtime-bundle branch from 4301c4f to 67d8a8b Compare August 6, 2026 03:55
@bkudiess

bkudiess commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Updated exact-head proof and review closeout for 67d8a8b5: real gateway send, terminal lifecycle, history replay without duplication, stop/start reconnect, and post-reconnect history all pass on the new head. #1069 classification is integrated into the projection owner. Final fixes cover concurrent first-wins disposal, stale retry accumulation, and stale retry displacement.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added the proof: sufficient Contributor real behavior proof is sufficient. label Aug 6, 2026
@bkudiess
bkudiess force-pushed the bkudiess-chat-runtime-bundle branch from 67d8a8b to 61ed054 Compare August 6, 2026 08:48
@clawsweeper clawsweeper Bot added status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 6, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
bkudiess and others added 2 commits August 6, 2026 01:58
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use exact submitted echoes to open buffered post-reset lifecycles after queue reconciliation, and fence deferred abort work to the active runtime generation.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bkudiess
bkudiess force-pushed the bkudiess-chat-runtime-bundle branch from 61ed054 to 0e34872 Compare August 6, 2026 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant