fix(exec): respect OPENCLAW_STATE_DIR for exec approvals#65736
Conversation
Greptile SummaryThis PR fixes a bug where Confidence Score: 5/5Safe to merge — the fix is a focused path-resolution correction with no behavioral change for default installs and good regression coverage. All findings are P2 (style suggestion about a parallel env-var read in the effective-policy helper). The core path-resolution change in exec-approvals.ts is correct and consistent with how other paths already use resolveStateDir(). The test cleanly captures the regression scenario. src/infra/exec-approvals-effective.ts — the resolveDefaultExecApprovalsHostPath helper could be simplified to avoid a parallel env-var read, but this is non-blocking. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/exec-approvals-effective.ts
Line: 149-151
Comment:
**Parallel env-var read may drift from `resolveStateDir`**
`resolveDefaultExecApprovalsHostPath` reads `process.env.OPENCLAW_STATE_DIR` directly to decide which branch to take, while `resolveExecApprovalsPath()` delegates to `resolveStateDir()` which already owns all the normalization logic (trim, `~` expansion, legacy fallback). The two branches are consistent today because both trim, but they can diverge if `resolveStateDir` gains new semantics (e.g., supporting `OPENCLAW_STATE_DIR_BASE`, alias handling, or whitelist checks).
A simpler, more future-proof form would simply always call `resolveExecApprovalsPath()` and drop the fallback to `DEFAULT_HOST_PATH`. The resolved path is already equivalent when `OPENCLAW_STATE_DIR` is unset (e.g., `/home/user/.openclaw/exec-approvals.json`), and users reading policy output typically prefer the expanded path over the literal `~` string. If the tilde representation is required for backward compatibility in snapshot output, that concern belongs in a formatting helper rather than a branching resolver.
```suggestion
function resolveDefaultExecApprovalsHostPath(): string {
return resolveExecApprovalsPath();
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(exec): respect OPENCLAW_STATE_DIR fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8cdcba2a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
6780c89 to
09ef697
Compare
|
Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Keep this PR open. Current main still routes exec approval JSON and socket defaults through the home-relative Best possible solution: Keep this PR open for maintainer review and landing, or explicitly supersede it with one canonical implementation. The final fix should make exec approval JSON and socket defaults honor an explicit What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a820a307dfeb. |
Use the configured state directory for exec approvals file and socket resolution so Docker and hosted deployments with relocated state roots stop falling back to ~/.openclaw. Add regression coverage for OPENCLAW_STATE_DIR path resolution.
|
ProjectClownfish pushed a narrow repair to this branch so the original contributor path can stay canonical. Source PR: #65736 |
32aad32 to
b826cfa
Compare
openclaw-barnacle
Bot
added
docs
vincentkoc
added
clownfish:human-review
clawsweeper
2 participants
Use the configured state directory for exec approvals file and socket resolution so Docker and hosted deployments with relocated state roots stop falling back to ~/.openclaw.
Add regression coverage for OPENCLAW_STATE_DIR path resolution.
Summary
Describe the problem and fix in 2–5 bullets:
If this PR fixes a plugin beta-release blocker, title it
fix(<plugin-id>): beta blocker - <summary>and link the matchingBeta blocker: <plugin-name> - <summary>issue labeledbeta-blocker. Contributors cannot label PRs, so the title is the PR-side signal for maintainers and automation.OPENCLAW_STATE_DIRand continued to use~/.openclaw/exec-approvals.*./dataor/data/openclaw, so exec-backed actions could fail even when the configured state dir was valid and writable.OPENCLAW_STATE_DIRis set.~/.openclawbehavior for stock installs, exec approval semantics, approval policy evaluation, or Docker docs/runtime wiring outside this path-resolution bug.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write
N/A. If the cause is unclear, writeUnknown.src/infra/exec-approvals.tshardcoded the default approvals file/socket under~/.openclaw, while the rest of OpenClaw already supports relocating mutable state viaOPENCLAW_STATE_DIR.OPENCLAW_STATE_DIR, so this inconsistency was not caught./home/node/.openclaw, which likely masked the mismatch.Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should catch this. Otherwise write
N/A.src/infra/exec-approvals-store.test.tsOPENCLAW_STATE_DIRis set,resolveExecApprovalsPath()andresolveExecApprovalsSocketPath()should resolve inside that directory instead of expanding~/.openclaw.User-visible / Behavior Changes
Exec-backed actions now respect
OPENCLAW_STATE_DIRfor approvals file/socket storage. Users with relocated state roots should no longer see host exec approvals fall back to~/.openclawor fail because that home-relative path is unavailable.Diagram (if applicable)
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation: N/ARepro + Verification
Environment
openai-codex/gpt-5.4tools.execpath through the gatewayOPENCLAW_STATE_DIR=/data/openclaw,OPENCLAW_WORKSPACE_DIR=/data/workspace,OPENCLAW_CONFIG_PATH=/tmp/openclaw.jsonSteps
OPENCLAW_STATE_DIR=/tmp/repro-state.HOME=/tmp/repro-home.resolveExecApprovalsPath()orresolveExecApprovalsSocketPath()and inspect the resolved path.Expected
/tmp/repro-state/exec-approvals.json.Actual
~/.openclaw/exec-approvals.*underHOMEinstead of the configured state dir. In relocated-state Docker setups, that mismatch could then surface as runtime failures such asENOENTwhen the home-relative path was unavailable.resolveExecApprovalsPath()resolved to<HOME>/.openclaw/exec-approvals.jsonresolveExecApprovalsSocketPath()resolved to<HOME>/.openclaw/exec-approvals.socksaveExecApprovals(...)failed withENOENT: no such file or directory, mkdir '<HOME>/.openclaw'Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm exec vitest run src/infra/exec-approvals-store.test.ts src/infra/exec-approvals-policy.test.tsOPENCLAW_STATE_DIRset andHOMEunusable,saveExecApprovals(...)failed trying to create~/.openclawresolveExecApprovalsPath()resolved to<OPENCLAW_STATE_DIR>/exec-approvals.jsonresolveExecApprovalsSocketPath()resolved to<OPENCLAW_STATE_DIR>/exec-approvals.socktools.execran from/data/workspacewithout the old/home/node/.openclawENOENT failure after rebuild/restartOPENCLAW_STATE_DIRis documented as the mutable state root overrideOPENCLAW_STATE_DIR=/data~/.openclaw/...whenOPENCLAW_STATE_DIRis not explicitly setpnpm build && pnpm check && pnpm testReview Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.~/.openclaw/...while the runtime used a relocated state dir.exec-approvals-effective.tsnow reports the resolved approvals path whenOPENCLAW_STATE_DIRis explicitly set.