Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 11, 2026, 12:43 PM ET / 16:43 UTC (Revision 18). ClawSweeper reviewWhat this changesAdds locally signed development and unsigned Microsoft Store MSIX packages, Windows-managed packaged startup and updates, packaging checks, and a gateway-preserving default for interactive uninstall. Merge readiness⛔ Blocked before merge - 5 items remain This remains a useful, distinct packaging contribution that current main does not implement. The earlier code concerns are addressed, but the live serialization proof and explicit maintainer proof hold remain unresolved. Priority: P2 Review scores
Verification
How this fits togetherThe Windows companion packaging pipeline turns the WinUI application into installable artifacts. Package identity determines Windows startup registration, update ownership, and app-specific permission controls. flowchart TD
A[Companion source] --> B[Build mode]
B --> C[Unpackaged installer]
B --> D[Development MSIX]
B --> E[Store submission MSIX]
D --> F[Local signing and installation]
E --> G[Partner Center signing]
F --> H[Windows startup and permissions]
G --> H
Decision needed
Why: The reviewer explicitly required these checks before reconsidering acceptance, and the contributor acknowledges that the publishing pause does not authorize deferral. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep packaging opt-in and distribution paused, with live startup-ordering evidence and explicit maintainer acceptance of the upgrade-proof boundary before landing. Do we have a high-confidence way to reproduce the issue? Not applicable to the central feature request; this review found no remaining concrete patch defect requiring a failing reproduction. Is this the best way to solve the issue? Yes, the single-project packaging modes and focused startup service are a maintainable approach; their merge readiness still depends on the requested runtime and upgrade evidence. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against c71121cdcdc9. 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 (17 earlier review cycles; latest 8 shown)
|
Adds an MSIX packaging channel alongside the existing Inno Setup + Updatum unpackaged installer, which is unchanged. Two flavors are produced from the same source, selected by the new `build.ps1 -Msix Dev|Store` parameter: - Dev: side-by-side identity (OpenClaw.Companion.Dev), signed with a local self-signed certificate, for sideload testing. Its version revision is derived from the installed package revision + 1 because Add-AppxPackage requires a strictly increasing version. - Store: release identity, unsigned (the Store re-signs), built for both win-x64 and win-arm64. Its version revision is pinned to 0 because Partner Center reserves the fourth version field. Those two revision rules are mutually exclusive, so a single artifact cannot serve both channels. Package.appxmanifest is the single source of truth for release identity; Build-Msix.ps1 reads it and verifies identity, publisher, architecture, and version on every packaged output, so swapping in the Partner Center values needs no script change. It also asserts required payload content (wxc-exec.exe, VC++ runtime DLLs) and forbids dev-only artifacts from leaking into a Store package. The manifest also declares the toast COM activator, which resolves the "Failed to register notification activator" warning in packaged builds, and a StartupTask extension used by the following commit. build.ps1 gains [CmdletBinding()]. Without it a PowerShell script routes unknown named parameters into $args and silently ignores them, so a stale switch produced a normal unpackaged build with no error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The existing autostart path writes a scheduled task or a Run registry value. Neither is appropriate inside an MSIX package: the package is the owner of its own startup registration, and Windows exposes that to the user in Settings > Apps > Startup, where a user toggle can override the app's request. AutoStartManager now detects package identity and, when packaged, drives the StartupTask declared in Package.appxmanifest instead of the Win32 mechanisms. The Win32 path is untouched for unpackaged installs. Because the user's Settings toggle wins over the app's request, the settings view model reverts its checkbox to the effective state rather than reporting success when Windows declines to enable the task, so the UI cannot claim autostart is on while it is disabled by policy or by the user. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MsixDevelopmentSigningTests asserts the packaging contracts as source text, so a future edit to build.ps1, the csproj, the manifest, or either packaging script cannot silently break the channel split without a red test. Notably it pins the -Msix ValidateSet, the presence of [CmdletBinding()], and that no certificate password or PFX export is introduced. DEVELOPMENT.md documents both channels, when to use each, and why the Store revision must be 0 while the dev revision must increase. It also states plainly that the dev revision tracks the installed package rather than incrementing per build, since rebuilding without installing reuses the same revision. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces the placeholder Store identity with the values reserved in Partner Center > Product management > Product identity: Name OpenClawFoundation.OpenClaw Publisher CN=4BA40A7A-B719-4C40-BF91-84AF4F1136FC PublisherDisplayName OpenClaw Foundation All three validate against ST_PackageName, ST_Publisher_2010_v2, and ST_DisplayName in the Windows SDK AppxManifestTypes.xsd. The dev sideload identity follows as OpenClawFoundation.OpenClaw.Dev. Identity/@publisher is the Store publisher and is deliberately not the Authenticode subject used to sign the unpackaged installer, which is unchanged in docs/RELEASING.md and Test-ReleaseExecutableSignatures.ps1. Decouples AppIdentity.AppUserModelId from the MSIX package identity. The two were previously aliased through a PackageIdentityName constant, but SetCurrentProcessExplicitAppUserModelID is skipped once the process has package identity, so that constant only ever supplied the Win32 AUMID for unpackaged installs. Renaming it alongside the package would have orphaned the AUMID that installer.iss has already written into existing users' Start menu shortcuts and broken their notifications, while doing nothing for packaged builds. AppUserModelId now holds its own literal and a regression test asserts it stays different from Identity/@name. Because the package family name is derived from Name and Publisher, an already-installed dev package will not upgrade in place; remove it before installing the next dev build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The packaged app ran the Updatum check on startup with no package-identity guard, so a Store-installed Companion would have self-updated from GitHub releases. That bypasses Store servicing, and it lets the packaged app claim update ownership from a legacy Inno install before any migration has been agreed to (docs/MSIX_LEGACY_INSTALL_MIGRATION.md). UpdateCoordinator now short-circuits when the process has package identity, mirroring the existing development-build skip and placed ahead of it so no packaged path reaches the network check. Manual "Check for updates" still reports a result rather than silently doing nothing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uninstalling the Inno package asks whether to also unregister the OpenClaw WSL distro. The prompt used MB_YESNO, which makes Yes the default button, so pressing Enter destroys the local gateway. Users who uninstall in order to reinstall - for example when moving to the Microsoft Store package - should keep their gateway unless they deliberately choose otherwise, so make No the default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
app.manifest declares PerMonitorV2 DPI awareness, but ApplicationManifest sat in the PackageMsix != true property group, so it was applied only to the unpackaged Inno build. MSIX builds shipped a DPI-unaware executable that renders blurry on mixed-DPI displays. Packaging does not supply DPI awareness, so move the declaration to the shared property group and cover it with a regression test. Verified with the Windows App Certification Kit against the release-identity x64 package: DPIAwarenessValidation went from WARNING to PASS, taking the overall verdict from WARNING to PASS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Store package and the Inno installer produce the same application and are not supported side by side: both register the openclaw protocol, both can claim autostart, and production builds share the OpenClawTray single-instance mutex, which MSIX does not namespace-isolate. With both installed, opening the Store app usually surfaces the Inno one. Document uninstall-first as the supported path and record why it is safe: per-user state under %APPDATA%\OpenClawTray survives, the interactive gateway prompt now defaults to No, and silent uninstall always removes the gateway so it must not be used to migrate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The packaged manifest installs the StartupTask disabled and Windows owns its state from then on. Nothing reconciled that against the persisted AutoStart preference: SettingsChangeCoordinator.Apply only runs on a settings change, so an AutoStart=true value preserved from an unpackaged install was reported as enabled in Settings while nothing actually launched at logon. The same stale reading occurred after a user toggled the entry in Settings > Apps > Startup. Treat Windows as the source of truth. The stored intent is applied when it can be, and whatever Windows reports afterwards is persisted, so a DisabledByUser or DisabledByPolicy refusal surfaces as off instead of being retried silently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b95cf49-7892-48fe-8eda-b0b1541b52ed
…me it Build-Msix.ps1 produced only Microsoft Store packages: its sole caller was the Store branch of build.ps1, it hardcoded AppxPackageSigningEnabled=false, and it verified the produced package against the release identity. The Dev flavor never used it. The generic name invited the opposite reading, so rename it to Build-StoreMsix.ps1. It also accepted -Configuration Debug and then published and verified that package. build.ps1 -Msix Store forces Release, but the script is a documented entry point on its own, so a caller could produce a locally verified, provenance-stamped artifact that Partner Center rejects. Accept Release only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b95cf49-7892-48fe-8eda-b0b1541b52ed
The Dev manifest rewrite changed the package and application display names but left desktop:StartupTask/@DisplayName as OpenClaw Companion. Because the Dev and production packages install side by side, Windows Startup Apps and Task Manager showed two identical startup entries and the user could disable the wrong one. Rewrite that attribute alongside the other Dev fields, and fail the build when it is missing, matching how every other required element in the rewrite behaves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b95cf49-7892-48fe-8eda-b0b1541b52ed
11d1106 to
7352de4
Compare
The previous section told users to uninstall the Inno build before installing the Store package. That is not the supported path: both installs can coexist and no uninstall is required. Replace it with what actually happens. The two installs share the openclaw protocol registration, the OpenClawTray mutex, per-user data, the gateway port, and the WSL distro, so the first one launched wins and the second forwards its activation and exits. Both can register autostart, so the logon race persists. A running Store app also blocks the Inno uninstaller, because installer.iss sets AppMutex to the shared mutex name. Migration behavior remains unimplemented and is tracked separately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b95cf49-7892-48fe-8eda-b0b1541b52ed
…o/msix-development-clean
Adversarial review verdictHOLD_FOR_AUTHOR at Independent Opus and Codex reviews found no dual-model consensus blocker. Direct cross-check confirmed two actionable single-model findings:
The AutoStart unknown-state fix itself is sound, the prior thread is resolved, and the branch was refreshed onto current Local validation after the merge:
Still required before reconsidering TAKE: exact-head CI Gate plus signed-artifact/SAC, native ARM64 runtime, clean install/upgrade/uninstall, and packaged WinUI consent proof. |
The paused build-msix job rewrote Identity/@name to OpenClaw.Companion before packing. This branch moved packaging to OpenClawFoundation.OpenClaw, so the step no longer matched the tracked manifest and would have produced a package whose identity disagreed with the source it was built from. The step is also redundant. GitVersion.MsBuild populates $(Version) for every project in the repo, and PrepareOpenClawAppxManifest generates identity and version into obj/ without touching the tracked manifest. Verified locally by invoking that target with the job's exact arguments and no /p:Version, which produced Name="OpenClawFoundation.OpenClaw" and Version=2026.7.2.0. Alpha package identity is deliberately not replaced here. Restoring a prerelease variant belongs with re-enabling the job, tracked in openclaw#1375. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b95cf49-7892-48fe-8eda-b0b1541b52ed
Startup reconciliation read the stored preference, awaited a StartupTask query, then persisted the result. The Settings toggle is fire and forget, so a user flipping it while that query was in flight could have their choice overwritten by a decision derived from the value they had just replaced. ReconcileAsync also writes to Windows on the configured-on branch, so the stale decision could be pushed to Windows as well, not just to settings. Both mutation paths now take a shared gate, so their read-decide-write sequences cannot interleave and a toggle raised during reconciliation is applied afterwards and wins. The reconciled value is additionally re-checked against the current preference through the new AutoStartReconciliation.ShouldPersistReconciledValue policy, which covers writes that reach settings without passing through the gate. The saved event is raised after the gate is released. Subscribers apply auto-start themselves, and SemaphoreSlim is not reentrant, so raising it while holding the gate would deadlock the moment a subscriber routed through ApplyAutoStartCore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b95cf49-7892-48fe-8eda-b0b1541b52ed
OpenClaw is the name reserved in Partner Center, so the Store listing, the Start menu tile, and Startup Apps should all read OpenClaw rather than OpenClaw Companion. Only display names change. Package identity is Identity/@name plus @publisher, neither of which is touched, so upgrade behavior, pairing, and the Store submission path are unaffected. The Inno product name, the "OpenClaw Companion" scheduled task name, and the assembly metadata are deliberately left alone: the task name is identity bearing and renaming it would orphan scheduled tasks on existing installs, which a packaged build is forbidden from migrating. Docs continue to say OpenClaw Companion in prose. That still describes the Inno product accurately, and this rename is scoped to MSIX packaging. Adds a test pinning all three strings. Display names are labels only, so nothing else in the suite would catch a silent revert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b95cf49-7892-48fe-8eda-b0b1541b52ed
Route post-save auto-start effects through the shared toggle and startup reconciliation gate. Read the live preference after acquiring the gate and hold it until the Windows write completes, preserving fault observation. Extract the narrow AutoStartSettingsApplier seam, cover queued reads, asynchronous write ordering, and failure cleanup, and record the ownership boundary in the architecture ledger. Validation: full build passed; Shared 3989 passed, 34 skipped; Tray 2942 passed; focused auto-start/settings coverage 153 passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b95cf49-7892-48fe-8eda-b0b1541b52ed
Thanks @karkarl! Addressed these 2 in latest iteration and updated the description with packaged consent screenshots and install/upgrade/uninstall results |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
What Problem This Solves
Adds Microsoft Store MSIX packaging for the Windows companion while preserving the existing Inno Setup and Updatum channel. Package identity enables Windows to apply app-specific camera, microphone, and location permission controls.
This does not enable Store distribution. The
build-msixCI job remainsif: false, and the release gate indocs/RELEASING.mdremains in place. Store rollout is tracked in #1375; coexistence and migration work is tracked in #1374.Supersedes #732. Closes #1373.
Why This Change Was Made
One WinUI project produces unpackaged, locally signed Dev MSIX, or unsigned Store submission artifacts. Partner Center signs the Store submission at ingestion. Generated manifests stay under
obj/, so Dev identity and version changes never rewrite the tracked Store manifest.The packaging path preserves the embedded DPI manifest, registers packaged startup through
StartupTask, and leaves updates to Windows instead of Updatum.Follow-up changes now pushed
Current PR head:
baa62869099a3da32cddba1a6b7b904fb310981b..github/workflows/ci.yml, but does not enable the paused MSIX job.AutoStartSettingsApplieracquires the same gate, reads the live preference after acquisition, and holds the gate until the Windows write completes. Existing background fault reporting is preserved.The settings-save change transfers the gated read/write sequence from
App.SettingsChangeCoordinator.csto the narrowAutoStartSettingsApplierservice.SettingsChangeCoordinatorretains effect ordering, andAppsupplies the shared gate and delegates. Behavioral tests cover queued reads, asynchronous write ordering, and failure cleanup; the architecture ledger closes the old ungated snapshot-write path.The removed CI step also selected an
.Alphaidentity. This PR does not invent a replacement alpha distribution policy. Store flights versus a separately sideloaded alpha identity remains a proposed #1375 follow-up requiring maintainer agreement.User Impact
Developers can build a side-by-side Dev package with
-Msix Dev, or x64/ARM64 Store submission artifacts with-Msix Store. This PR does not publish either to the Store or replace the existing.exechannel.The Inno uninstall confirmation now defaults to No when asking whether to remove the WSL gateway. Button order and silent-uninstall behavior are unchanged. Silent-uninstall safety and coexistence arbitration remain in #1374.
Evidence
The screenshots below show the Dev package name, a live Packaged (MSIX) app, OpenClaw-owned consent dialogs, and Windows permission-control comparisons through the real local MCP capability implementations.
Dev-signed install, upgrade, uninstall, and reinstall were also exercised on the existing x64 host. These results are not Store-signing, SAC, ARM64, clean-VM, or gateway-path proof.
Change Type
Scope
No changes to
src/Directory.Build.targets. No new node command, automatic migration, legacy-install detection, or automatic Inno removal.Required proof pools
windows-winui-interactivewindows-clean-installer-upgradewindows-11-arm64windows-11-sac-onDeclaring these pools does not claim that they ran. The Store publishing pause does not waive the requested proof.
Validation
Latest completed local validation used the source snapshot now committed as
baa62869. All six files in the final commit still match their recorded capture-time SHA-256 hashes and the committed Git blobs. Committing did not introduce another code change..\build.ps1dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore.\scripts\run-proof-tests.ps1 -Project tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj -Filter FullyQualifiedName~InstallerIssAssertionTests -ResultName installer-sourceThe local build used the existing NuGet proxy configuration and
TargetLatestRuntimePatch=false; tests usedOPENCLAW_REPO_ROOTfor this checkout. The restore configuration is not published.Exact-head CI is not yet green. The new Build and Test run and CodeQL run both report
action_required, with no jobs started. Maintainer workflow approval is needed. The green CI Gate on the previous8935a8cdhead does not validate this head.Manual/rubber-duck review was performed during implementation and closeout. Structured autoreview was attempted with
python .agents\skills\autoreview\scripts\autoreview --mode localbefore committing, but could not run becausecodexis not installed. No clean structured-review result is claimed.Real Behavior Proof
Environment and source provenance
Captured September 10, 2026 on Windows x64, OS build 26683, using the real
OpenClawFoundation.OpenClaw.Devidentity and an isolated synthetic tray profile. Local MCP was enabled with node mode off. No real gateway credentials or user settings were copied into that profile.The current-package proof used Dev MSIX 2026.7.2.8, built from
ed3b5656plus the six then-uncommitted files that are now commitbaa62869. Their hashes were checked before publication. The package was not rebuilt after committing, and these are not screenshots of a CI-produced or Store-signed artifact.Both unmodified MSIX packages had valid signatures from
CN=OpenClaw Local Development:2026.7.2.7760B801BA1D1D715898259A3DCF52DC278A51FB77BF7419326B912C3207F51DC2026.7.2.84F98B5A58C0D873BB4B64FEDA0C465495787C2E88611257AE02CF3D664EDA3ADPackage labels and packaged runtime
The name captures were taken with Dev
.7; the labels are unchanged in.8. These are focused crops of the Dev entries, not a claim that no other OpenClaw-related application exists on the host.Start menu: OpenClaw (Dev)
Windows Startup Apps: OpenClaw (Dev)
Dev
.8: App info confirms Packaged (MSIX). The in-app product name remains OpenClaw Windows Companion; the reserved package display-name change does not rename that separate branding.Packaged consent and Windows permission controls
Actual local MCP
tools/listdiscovery andtools/callinvocation exercisedcamera.snap,camera.clip, andlocation.get. This proves the same-machine capability implementations, not a gateway-mediated invocation.OpenClaw-owned first-use dialogs. These are not Windows-owned permission popups.
Windows Settings comparisons. Each image is a focused crop of the OpenClaw (Dev) row on the corresponding native privacy page.
camera.snapUnauthorizedAccessException.camera.clip,durationMs=250,includeAudio=truedurationMs=250andhasAudio=true. This is MediaCapture audio/video, not STT.location.getLOCATION_PERMISSION_REQUIRED.positionReturned=truewas retained.No photos, recordings, transcripts, or coordinates are published. Camera and microphone app permissions were restored to Off before uninstall; device-wide location was restored to Off and its machine consent value confirmed as
Deny.Automatic Windows-owned first-use prompts were not captured. The app dialogs and native privacy switches above do not satisfy that separate requirement by themselves.
Dev-signed install, upgrade, uninstall, and reinstall
.7package. A fresh synthetic profile was used..7launched and responded to localapp.statusandapp.navigate..7to.8Add-AppxPackage -ForceApplicationShutdownregistered.8. A seeded packageLocalStatemarker and the isolated preferences survived..8launched, local MCP responded, and App info showed Packaged (MSIX) as pictured above..8Remove-AppxPackageremoved package registration, its Start entry, and the packageLocalStatemarker. The deliberately external synthetic profile remained..8LocalStatemarker was absent. Temporary test profiles were cleaned up.This was package-level clean installation on an existing machine, not a clean Windows VM or production migration. Repair and removal of the staged WindowsApps payload directory were not checked.
Historical proof retained from earlier revisions
Earlier WACK, coexistence, updater, and auto-start regression evidence (not current-head captures)
Earlier packaging proof at
7352de49produced x64 and ARM64 Store artifacts. The x64 WACK report recordedOVERALL_RESULT: PASSandDPIAwarenessValidation: PASS, with two optional findings. Those results are historical and are not claimed for the newly pushed head.At
20880a0a, an Inno build and production-identity MSIX could both be installed, but a running packaged app blocked the Inno installer and uninstaller through the shared mutex. In both launch orders the second process exited, and both auto-start registrations remained. This supports the separate coexistence work in #1374, not completion of the official clean-installer pool.The duplicate labels in these earlier captures predate the OpenClaw / OpenClaw (Dev) display-name fix:
Earlier packaged toggle proof observed
StartupTaskstates2 -> 0 -> 2while leaving the Inno Startup shortcut untouched and creating no legacy scheduled task or Run value.The packaged updater reported that Microsoft Store manages updates:
At
fd17b614, a temporary manifest with a mismatched startupTaskIddeliberately forced the Windows query to fail. The fixed path kept the configured value; a comparison with the old behavior changed it from True to False. The injected manifest and comparison-code changes were reverted after that test.fd17b614These screenshots do not exercise the newly added settings-save serialization. That final gate invariant is covered by the new deterministic behavioral tests and source-wiring guard.
Not verified / blocked
Security Impact
internetClient,runFullTrust,webcam,microphone, andlocationfor existing functionality.OpenClaw's remembered consent remains separate from Windows privacy controls. Partner Center privacy policy and
runFullTrustjustification are portal work.Compatibility and Migration
The Inno/Updatum channel remains available. There is no automatic uninstall or migration in this PR. Coexistence, startup arbitration, legacy-install detection, and survivor-safe uninstall remain scoped to #1374.
The Dev identity is side-by-side with the production identity. Display-name changes do not alter package identity, publisher, or the legacy scheduled-task identity.
Maintainer Decisions Requested
The CI patch and auto-start feedback have been addressed in the pushed changes, with the local Dev proof added above. This is not a claim that Karen's full
HOLD_FOR_AUTHORchecklist is satisfied.Maintainer agreement is still needed on the remaining proof and any deferral to #1375, including the official-signing/SAC path and alpha distribution policy. A paused Store rollout does not implicitly approve those deferrals.
Review Conversations
This description update does not resolve review threads. The author will reply separately; the overall maintainer review remains open.