Skip to content

fix(exec): migrate approvals to custom state dir - #744

Merged
vincentkoc merged 1 commit into
mainfrom
fix/exec-approvals-state-dir-migration
Jun 11, 2026
Merged

fix(exec): migrate approvals to custom state dir#744
vincentkoc merged 1 commit into
mainfrom
fix/exec-approvals-state-dir-migration

Conversation

@vincentkoc

Copy link
Copy Markdown
Member

Summary

  • honor OPENCLAW_STATE_DIR for the Windows exec-approvals V2 store
  • migrate the existing tray approvals file atomically during node capability startup, archive the source, and fail closed while migration is blocked
  • align askFallback with OpenClaw's security-valued contract while preserving legacy off / on-miss / always files
  • preserve raw approval JSON during migration so unknown restrictions such as argPattern are not dropped
  • clamp fallback permissions to the active security policy and evaluate allowlist matches when fallback uses allowlist mode

Context

This mirrors the state-dir approvals migration landed in OpenClaw at openclaw/openclaw@adad27d (fix(exec): honor state dir approvals).

The Windows app keeps %LOCALAPPDATA%\OpenClawTray as its default approvals root. When OPENCLAW_STATE_DIR is explicitly set, it migrates the existing file to that target instead of silently starting with a new policy.

Verification

  • Windows 11 ARM64 Parallels VM: dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --filter FullyQualifiedName~ExecApprovals — 131 passed
  • git diff --check
  • autoreview: clean, no accepted/actionable findings

Broader repo gates were attempted but are environment-blocked on the clean VM:

  • ./build.ps1 requires Git, Node.js, and the Windows 10 SDK in addition to the installed repo-pinned .NET 10 SDK
  • full shared tests: 2014 passed, 203 failed, 29 skipped; failures are the existing ARM64 libsodium native dependency load error
  • tray tests: 1025 passed, 15 failed; failures are the same ARM64 libsodium native dependency load error

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 10, 2026, 9:52 PM ET / 01:52 UTC.

Summary
The PR makes the Windows V2 exec-approvals store honor OPENCLAW_STATE_DIR, migrates and archives the legacy policy file, and changes askFallback to security-valued evaluation while accepting legacy values.

Reproducibility: yes. at the source level: current main always uses the tray data directory and never reads OPENCLAW_STATE_DIR, so changing the state directory leaves the V2 approvals policy behind. This read-only review did not independently run a Windows reproduction.

Review metrics: 3 noteworthy metrics.

  • Changed surface: 7 files, +383/-39. The PR combines persisted-policy migration, fallback-contract changes, startup wiring, and focused tests.
  • Focused verification: 131 exec-approval tests passed. The contributor reports strong targeted Windows coverage for the changed behavior.
  • Full-gate status: 3 required gates not fully clean. The build was dependency-blocked and both full test projects reported existing ARM64 native-library failures.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Run ./build.ps1 and both complete test projects on a supported Windows runner for the exact PR head.

Risk before merge

  • [P1] Existing installations using a custom state directory will intentionally fail closed while the legacy policy cannot be migrated, so command execution may stop until the filesystem or policy-file problem is corrected.
  • [P1] The patch changes persisted askFallback interpretation and serialization; focused tests cover legacy values, but maintainers should explicitly accept the cross-version contract before release.
  • [P1] The reported full build and repository test gates did not complete cleanly on the contributor's ARM64 environment, so the exact head still needs repository-required validation on a supported Windows runner.

Maintainer options:

  1. Validate the exact head on supported Windows (recommended)
    Run the required build and both complete test projects, then review the focused migration coverage before merging.
  2. Accept the fail-closed upgrade contract
    Maintainers may intentionally accept that an unreadable or unwritable legacy approvals file disables command approval until the operator repairs the state directory or policy file.
  3. Pause for upgrade-flow evidence
    Keep the draft open if maintainers want a redacted real Windows migration trace before accepting the persisted-policy transition.

Next step before merge

  • [P1] Because this is a member-authored, security-sensitive draft, the next action is explicit maintainer review of the fail-closed upgrade contract and supported Windows validation of the exact head.

Security
Cleared: The patch preserves raw policy data, uses exclusive temporary-file migration, fails closed on invalid or blocked migration, and introduces no concrete security or supply-chain regression.

Review details

Best possible solution:

Land the state-directory-aware raw migration and security-valued fallback contract after maintainers confirm the fail-closed upgrade policy and the exact head passes the required Windows build, shared-test, and tray-test gates.

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

Yes at the source level: current main always uses the tray data directory and never reads OPENCLAW_STATE_DIR, so changing the state directory leaves the V2 approvals policy behind. This read-only review did not independently run a Windows reproduction.

Is this the best way to solve the issue?

Yes, subject to upgrade validation: resolving the state directory in the store and atomically copying the raw file is the narrowest maintainable fix, while legacy conversion and security clamping preserve compatibility and fail-safe behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: reasoning high; reviewed against 3fdfbfad837f.

Label changes

Label changes:

  • add P2: This is a bounded but security-sensitive upgrade fix affecting users who configure a custom state directory.
  • add merge-risk: 🚨 compatibility: The PR migrates existing persisted approval files and changes the canonical representation of legacy fallback values.
  • add merge-risk: 🚨 security-boundary: The patch changes the policy controlling command execution when approval presentation is unavailable.
  • add merge-risk: 🚨 availability: The intentional fail-closed migration path can make system command execution unavailable when migration is blocked.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The contributor is a repository member, so the external-contributor proof gate does not apply; the PR nevertheless reports focused execution on a Windows 11 ARM64 VM.

Label justifications:

  • P2: This is a bounded but security-sensitive upgrade fix affecting users who configure a custom state directory.
  • merge-risk: 🚨 compatibility: The PR migrates existing persisted approval files and changes the canonical representation of legacy fallback values.
  • merge-risk: 🚨 security-boundary: The patch changes the policy controlling command execution when approval presentation is unavailable.
  • merge-risk: 🚨 availability: The intentional fail-closed migration path can make system command execution unavailable when migration is blocked.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The contributor is a repository member, so the external-contributor proof gate does not apply; the PR nevertheless reports focused execution on a Windows 11 ARM64 VM.
Evidence reviewed

What I checked:

Likely related people:

  • AlexAlves87: Introduced the V2 approvals store, evaluator, prompt adapter, and coordinator across the merged feature series. (role: feature owner; confidence: high; commits: a4f106c7d09c, 6d58171c5847, d8c2ca5184ad; files: src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs, src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs, src/OpenClaw.Shared/ExecApprovals/ExecApprovalEvaluation.cs)
  • Scott Hanselman: Recently hardened and annotated the exec-approvals area in commit d23f8ca. (role: recent security-hardening contributor; confidence: medium; commits: d23f8ca50013; files: src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs, src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@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. 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. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 11, 2026
@vincentkoc
vincentkoc marked this pull request as ready for review June 11, 2026 08:35
@vincentkoc

Copy link
Copy Markdown
Member Author

Land-ready verification for 1042457885a655e14e386c3c353d4f3f7879e8dd:

  • CI: test, e2etests, build (win-x64), build (win-arm64), CodeQL, and Socket Security passed.
  • Focused Windows proof: dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --filter FullyQualifiedName~ExecApprovals — 131 passed.
  • Autoreview: clean, no accepted/actionable findings.
  • Review threads: none unresolved.

The fail-closed migration behavior is intentional and mirrors the OpenClaw state-dir approvals fix at openclaw/openclaw@adad27d.

@vincentkoc
vincentkoc merged commit 913ba4e into main Jun 11, 2026
26 checks passed
@vincentkoc
vincentkoc deleted the fix/exec-approvals-state-dir-migration branch June 11, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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: 🐚 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant