refactor(tray): extract final App orchestration owners - #1088
Conversation
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed August 6, 2026, 4:12 AM ET / 08:12 UTC. ClawSweeper reviewWhat this changesThe PR extracts deep-link and toast activation planning, post-save settings effects, and exactly-once shutdown sequencing from the native tray App into focused services while retaining App as the composition root. Merge readinessKeep open. This collaborator-authored, stacked tray refactor has no discrete correctness finding in the reviewed patch; it needs normal maintainer integration after its three parent layers land or it is rebased onto current main. Priority: P2 Review scores
Verification
How this fits togetherThe Windows tray App receives launch, toast, and settings events, then drives tray/window surfaces, gateway and local MCP runtime state, and orderly shutdown. The new services plan those lifecycle operations while App remains the platform-specific composition and effect-adapter boundary. flowchart LR
A[Launch, toast, and settings events] --> B[Tray App composition root]
B --> C[Activation planning]
B --> D[Settings effect coordination]
B --> E[Shutdown sequencing]
C --> F[Tray and window surfaces]
D --> G[Gateway and local MCP runtime]
E --> H[Ordered process exit]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the parent ownership layers in order, then rebase this final lifecycle layer onto main and preserve its activation, settings, and shutdown proof on the rebased head. Do we have a high-confidence way to reproduce the issue? Not applicable as a refactor. The supplied exact-head runtime proof and focused tests exercise the behavior-equivalence paths rather than reproduce a pre-existing defect. Is this the best way to solve the issue? Yes. The extracted services retain App as composition root, use narrow effect ports, update the architecture ledger, and add focused guards; the collaborator's scope decision also resolves the earlier split concern. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against aac07abb1fbd. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
53a4d0b to
7a2b6b8
Compare
|
Maintainer scope decision: keep A3 as one final App-orchestration layer. The product owner explicitly requested fewer consolidated PRs. Activation routing, post-save settings coordination, and ordered shutdown are the three remaining App-owned orchestration seams and share the same composition adapters, lifecycle fencing, disposal order, ledger closeout, current-head runtime proof, and rollback boundary. Each owner still has its own typed contract, authoritative/closed ledger rows, deterministic behavioral guards, focused tests, and independently reviewed invariants. Splitting this already validated final layer would add stack/cascade risk without changing runtime ownership or proof. I accept the combined A3 scope for normal maintainer review. The branch remains mergeable; local exact-current build, Shared, Tray, Connection, Setup, FunctionalUI, Integration, Native UI, Accessibility, focused lifecycle, and SetupAndConnect E2E are green. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
1 participant
Extracts the final safe App orchestration seams while keeping startup and service construction in
App. This is layer 4 of native App stack #1083 and depends on #1085, #1082, and #1075.Summary
ActivationRouterfor launch, toast, deep-link, and forwarded single-instance activation planning, confirmation, redaction, current-user IPC, and lifecycle quiescence.DeepLinkHandlerandToastActivationRouteras the only route tables; App applies one closed semantic route union through the existing A2 surface owners/services.SettingsChangeCoordinatorfor detached-snapshot classification and exact post-save effect ordering through narrow App effect ports.AppShutdownCoordinatorfor first-wins shared-task shutdown, ordered per-step catch/log/continue, and final Exit exactly once.AppBootstrapper: current startup does not yet have construction seams that can move without creating a callback shell or replacement god coordinator.Ownership transfer
IActivationRouter/ActivationRouterApp.OnSettingsSavedclassification and effect orderISettingsChangeCoordinator/SettingsChangeCoordinatorIAppShutdownCoordinator/AppShutdownCoordinatorApp remains the sole composition root and startup owner. A0 shared state, A1 presentation, and A2 tray/window ownership remain unchanged.
Validation
./build.ps1: all 5 projects passed, 0 errors/warningsOpenClaw.Shared.Tests: 3,415 passed, 32 skippedOpenClaw.Tray.Tests: 2,258 passedOpenClaw.Connection.Tests: 529 passedOpenClaw.SetupEngine.Tests: 703 passedReal behavior proof
app.settings.setcalls producedUiOnly,NoOp, andNodeReconnectRequiredimpact paths while MCP remained running.openclaw://chat, exited 0, and the primary remained responsive.Connect to gateway to start chatting; no pairing-incomplete silent no-op.The non-interactive desktop returned a blank screenshot and blocked physical pointer injection, so visible evidence used source-blind Windows UI Automation names and exact runtime logs. Natural process exit and ordered shutdown evidence were obtained.
Adversarial review and fixes
finally, publishes the shared shutdown task before reentrant callbacks, continues after failures, and exits once.Final independent Claude Opus 4.8 and GPT-5.6 Sol staged-diff reviews found no remaining actionable findings. Gemini and Sonnet review findings were also reconciled; the intentional early ActivationRouter drain is a tested correctness improvement.
Architecture
Adds authoritative and closed ledger rows for activation, settings coordination, and shutdown, and narrows A2 residue. Startup remains in
App.OnLaunchedAsync;AppBootstrapperremains planned/deferred.Stack
Directly based on A2 PR #1085. This is the fourth/final App architecture layer after #1075, #1082, and #1085.