Skip to content

refactor(tray): move Permissions into shared-state MVVM ownership - #1075

Open
bkudiess wants to merge 3 commits into
mainfrom
bkudiess-permissions-mvvm-pilot
Open

refactor(tray): move Permissions into shared-state MVVM ownership#1075
bkudiess wants to merge 3 commits into
mainfrom
bkudiess-permissions-mvvm-pilot

Conversation

@bkudiess

@bkudiess bkudiess commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Moves the Permissions page's presentation, lifecycle, and shared settings behavior out of code-behind into a WinUI-free view model while preserving current security, command exposure, and visual behavior.

What changed

  • Add a transient PermissionsPageViewModel and no-cache runtime projection, activated through the existing navigation scope.
  • Make ISettingsStore field-scoped, serialized, versioned, and origin-aware so Settings, Permissions, tray, Hub, Connection, and local MCP writers converge without self-echo or stale updates.
  • Add a narrow observable presentation contract over the existing App-owned V2 ExecApprovalsStore; runtime and UI continue sharing one authoritative store.
  • Preserve V2 CAS metadata, invalid-path fail-closed behavior, dynamic inheritance, stable rule identity, monotonic events, and last-valid failure projection.
  • Keep NodeCapabilityGating as the sole command-exposure owner and preserve BrowserProxyActivation/shared-token behavior.
  • Rebuild the live capability list after permission changes in MCP-only mode, so tools/list updates immediately without requiring a gateway reconnect or app restart.
  • Reduce PermissionsPage.xaml.cs from 891 to 557 lines while leaving the visual tree intentionally unchanged.
  • Update the architecture ledger and behavioral guards for single-owner shared mutable state.

Ownership transfer

  • Old owner: PermissionsPage.xaml.cs directly owned settings mutation, runtime projection, approval-file presentation, and subscriptions.
  • New owners: PermissionsPageViewModel, PermissionsPageRuntimeSource, ISettingsStore, and the App-owned ExecApprovalsStore presentation contract.
  • Preserved invariant: field-scoped mutation persists before NotifySettingsSaved; active surfaces and MCP capability discovery converge to the newest version; command exposure remains owned by NodeCapabilityGating.

Scope decision

This lands as one atomic ownership transfer. Splitting the view model, shared-settings convergence, and V2 approval observation would temporarily leave parallel mutable owners or require throwaway compatibility paths. Visible Fluent/icon/card cleanup remains separate, so this PR stays behavior- and visual-identical.

Validation

  • ./build.ps1: all 5 projects passed
  • OpenClaw.Shared.Tests: 3,415 passed, 32 skipped, 0 failed
  • OpenClaw.Tray.Tests: 2,103 passed, 0 skipped, 0 failed
  • OpenClaw.WinNode.Cli.Tests: 126 passed, 0 skipped, 0 failed
  • Focused current-head settings/permissions/MCP-only reload tests: 69 passed
  • Observer ordering stress: 140/140 passed
  • Concurrency stress: 40/40 passed
  • ./scripts/validate-mxc-e2e.ps1: 2/2 passed, 0 skipped
  • Exact-head rebuild on August 4: 5/5 projects passed

Independent adversarial review

Exact head reviewed: 57ac65dc0dfaefc82e751786f337da0dba0064be.

  • Claude Opus 4.8: clean, no in-scope CRITICAL/HIGH/MEDIUM defects; 96% confidence that no in-scope CRITICAL/HIGH defect remains.
  • GPT-5.3 Codex: clean, no significant in-scope findings; 96% confidence that no in-scope CRITICAL/HIGH defect remains.

Both reviewers independently inspected the complete 8ac00c5f..57ac65dc diff. Their confidence accounts for the full suites, 20-cycle observer and concurrency stress, real gateway-to-Windows-node MXC E2E, live MCP-only camera gating, Settings persistence, active-page updates, and V2 approvals CAS/hash/UI restoration.

Real behavior proof

PR head 57ac65dc0dfaefc82e751786f337da0dba0064be was launched as an isolated side-by-side Dev app with local MCP.

  • Permissions opened with Node mode off, status Local MCP only, Camera on, and the camera tools present.
  • Clicking Camera off directly in the WinUI page changed the visible toggle to off and reduced served capabilities.
  • app.settings.get and isolated settings.json both returned NodeCameraEnabled=false.
  • The live MCP tool count changed 51 → 48, proving camera commands were removed immediately in MCP-only mode.
  • app.settings.set restored Camera to true; the already-open page returned to Camera on and the live tool count returned 48 → 51.
  • V2 approval CAS/UI/reopen/external-change round trips restored the exact initial state.
  • winnode --list-tools, local MCP settings commands, and the real gateway-to-Windows-node MXC proofs passed.

Inspectable current-head transcript

# Initial native accessibility state
AXRow (selected) Permissions
AXButton Node mode Value: off
AXStaticText Local MCP only
AXButton Camera Value: on
MCP tools/list count: 51

# Direct computer-use click on the native Camera ToggleSwitch
Camera Value: on -> Camera Value: off

$ winnode --command app.settings.get --params '{"name":"NodeCameraEnabled"}'
false
$ (Get-Content <isolated-data-dir>/settings.json | ConvertFrom-Json).NodeCameraEnabled
False
MCP tools/list count: 48

# Restore through local MCP
$ winnode --command app.settings.set \
    --params '{"name":"NodeCameraEnabled","value":"true"}'
{"name":"NodeCameraEnabled","value":true}

$ winnode --command app.settings.get --params '{"name":"NodeCameraEnabled"}'
true

# Same already-open native page after the MCP write
AXButton Camera Value: on
MCP tools/list count: 51
$ ./scripts/validate-mxc-e2e.ps1
Test Run Successful.
Total tests: 2
Passed: 2
Skipped: 0
Gateway MXC proof passed: RealGateway_SystemRun_ExecutesThroughWindowsNodeMxcSandbox
Gateway MXC proof passed: RealGateway_SystemRun_BlocksWritesToTrayDataDirectoryInMxcSandbox

Exact-final-head media proof

Exact source head 57ac65d was launched through the repository's normal non-isolated local path on an interactive Windows desktop. Native WinUI UI Automation read the Camera toggle, changed it from Off to On for this frame, then restored it to Off before the launched process exited.

Permissions Camera enabled exact-head proof

  • Sanitized proof: PNG
  • Capture manifest: manifest.json
  • Dedicated proof ref: �kudiess:proof/pr-1075-permissions-57ac65dc-proof at $proofCommit
  • State restoration: Camera Off -> captured On -> restored Off.
  • Capture method: native WinUI UI Automation plus PrintWindow(PW_RENDERFULLCONTENT), cropped to the Permissions content pane.
  • Privacy disclosure: Privacy crop/redaction only; UI state unchanged.
  • The published artifact is the sanitized derivative only. The uncropped original was not uploaded.
  • Both immutable image and manifest links returned HTTP 200; the PNG decoded at 874x746 and SHA-256 �fb99d60acce6cfd554192ed4bdf6aca8c4a0f03320eccdcfcccb1253e4a7234 matched the manifest.

Deferred visual work

This PR intentionally preserves the existing capability icons, colors, cards, spacing, and copy. Fluent icon/token/SettingsCard cleanup remains a separate visible-design change.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Aug 2, 2026
@clawsweeper

clawsweeper Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 6, 2026, 4:12 AM ET / 08:12 UTC.

ClawSweeper review

What this changes

Moves Permissions-page state into shared WinUI-free ownership and rebuilds local MCP capabilities after permission changes and MCP restart.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

Keep this PR open. Its collaborator author requires maintainer judgment, and the final MCP restart change lacks real behavior proof from the current head; the supplied screenshot and transcript cover the earlier 57ac65d head.

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

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The refactor has substantial tests and prior runtime evidence, but its final capability-restart behavior is not proven on the submitted head.
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 does not include this PR: The PR head is not an ancestor of current main; their merge base is the PR base commit, so the requested refactor remains unmerged.
Final behavior changed after supplied proof: The final head commit is “fix(tray): rebuild MCP-only capabilities on restart”; the PR proof names 57ac65d, so it does not demonstrate the final restart behavior.
MCP contract requires a live capability list: Current documentation defines MCP-only mode without a gateway and requires tools/list to read the live capability registry, making the post-restart tool-list transition the relevant proof.
Findings None None.
Security None None.

How this fits together

The tray app exposes Windows-node permissions through WinUI, tray, Hub, and local MCP settings commands. Those inputs update shared settings and execution approvals, then determine which tools local MCP and gateway-connected agents receive.

flowchart LR
  A[Permissions page] --> D[Shared settings store]
  B[Tray and Hub controls] --> D
  C[Local MCP settings commands] --> D
  D --> E[Permissions view model]
  D --> F[Capability gating]
  F --> G[Local MCP tool list]
  E --> H[WinUI presentation]
Loading

Decision needed

Question Recommendation
Is exact-head runtime proof of MCP capability rebuilding after restart sufficient before this permission-boundary refactor is merged? Require exact-head proof: Request a redacted current-head MCP-only run showing capability removal and restoration across an MCP restart.

Why: The final commit changes the behavior the earlier screenshot and transcript do not cover, and this boundary controls tools exposed to local agents.

Before merge

  • Resolve merge risk (P1) - The final restart rebuild can leave local MCP tools stale or incorrectly exposed if it is not validated against the exact current head.
  • Resolve merge risk (P1) - The local partial clone could not materialize PR-head source trees because its promisor remote was unreachable, limiting independent source-level review of the final delta.
  • Complete next step (P2) - A maintainer should require or explicitly waive exact-head proof for the final MCP restart behavior before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Review surface 42 files, +5,943/-1,174 lines The refactor spans persisted settings, approvals, WinUI presentation, capability registration, and regression tests.

Merge-risk options

Maintainer options:

  1. Prove the restart path on the final head (recommended)
    Before merge, show a redacted final-head MCP-only run where a permission change survives MCP stop/start and tools/list changes accordingly.
  2. Accept the proof gap
    Maintainers may merge based on the existing earlier-head transcript and tests, accepting that the final restart-only behavior was not observed live.

Technical review

Best possible solution:

Add a redacted exact-head MCP-only proof that disables a capability, stops and restarts local MCP, and shows tools/list plus the reopened Permissions page converging to the persisted setting.

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

Not applicable: this refactor must preserve established Permissions and MCP behavior; the missing evidence is an exact-head restart proof rather than a user bug reproduction.

Is this the best way to solve the issue?

Unclear: the ownership direction matches the architecture ledger, but the final MCP restart delta needs current-head runtime evidence before it can be judged the best safe implementation.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This is a substantial but non-emergency tray and MCP refactor.
  • merge-risk: 🚨 compatibility: Existing settings writers and persisted permission behavior are consolidated.
  • merge-risk: 🚨 security-boundary: Permission changes control which Windows-node capabilities local agents may invoke.
  • 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:

  • Current main does not include this PR: The PR head is not an ancestor of current main; their merge base is the PR base commit, so the requested refactor remains unmerged. (aaebf76640f4)
  • Final behavior changed after supplied proof: The final head commit is “fix(tray): rebuild MCP-only capabilities on restart”; the PR proof names 57ac65d, so it does not demonstrate the final restart behavior. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:963, aaebf76640f4)
  • MCP contract requires a live capability list: Current documentation defines MCP-only mode without a gateway and requires tools/list to read the live capability registry, making the post-restart tool-list transition the relevant proof. (docs/MCP_MODE.md:59, aac07abb1fbd)
  • Prepared visual proof inspected: The sanitized image is valid and shows the Permissions Camera toggle enabled, but its filename and PR body identify the earlier 57ac65d head. (57ac65dc0dfa)
  • Feature-history routing: Recent NodeService history includes Barbara Kudiess’s V2 approvals and connection recovery work; Scott Hanselman has the strongest aggregate history on the central paths. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:302, 4806eaac7c24)
  • Checkout limitation: The partial checkout could read current main and PR commit metadata, but materializing PR-head trees attempted a promisor fetch and failed because GitHub DNS was unavailable; final-head source review therefore relies on the supplied diff context. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:963, aaebf76640f4)

Likely related people:

  • bkudiess: Current-main history attributes V2 exec approvals and connection recovery to Barbara Kudiess, and this PR continues those same permission and NodeService boundaries. (role: recent area contributor; confidence: high; commits: 4806eaac7c24, bc210e048aa9, aaebf76640f4; files: src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs, src/OpenClaw.Tray.WinUI/Services/NodeService.cs)
  • Scott Hanselman: Feature-history shortlog shows the strongest aggregate ownership signal across NodeService, execution approvals, and Permissions-related paths. (role: longstanding area contributor; confidence: medium; commits: 261ba11aaad6; files: src/OpenClaw.Tray.WinUI/Services/NodeService.cs, src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs)

Rank-up moves

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

  • Run the exact aaebf76 MCP-only stop/start scenario and attach redacted tools/list and Permissions-state output.

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 (8 earlier review cycles)
  • reviewed 2026-08-02T18:56:47.230Z sha f665c48 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-03T03:15:34.065Z sha f665c48 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-03T23:13:57.032Z sha 57ac65d :: needs maintainer review before merge. :: none
  • reviewed 2026-08-03T23:23:40.661Z sha 57ac65d :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T06:27:51.333Z sha 57ac65d :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T07:14:12.801Z sha 57ac65d :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T08:06:37.838Z sha 57ac65d :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T20:04:06.259Z sha aaebf76 :: needs maintainer review before merge. :: none

@bkudiess

bkudiess commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 3, 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 proof: sufficient Contributor real behavior proof is sufficient. 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. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 3, 2026
@bkudiess

bkudiess commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 3, 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:

@bkudiess

bkudiess commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

1 similar comment
@bkudiess

bkudiess commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added the proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. label Aug 5, 2026
bkudiess and others added 3 commits August 5, 2026 11:01
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bkudiess
bkudiess force-pushed the bkudiess-permissions-mvvm-pilot branch from 57ac65d to aaebf76 Compare August 5, 2026 19:59
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed proof: sufficient Contributor real behavior proof is sufficient. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Aug 5, 2026
@bkudiess
bkudiess marked this pull request as ready for review August 6, 2026 08:08
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: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. 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