Greptile Summary

This PR fixes a real bug where invalid enum strings like "none" set in ~/.openclaw/exec-approvals.json were silently propagated through normalizeExecApprovals, causing minSecurity/maxAsk to receive unrecognized values and producing unpredictable host-routing behavior. The fix adds explicit validation of security, ask, and askFallback fields using the existing normalizeExecSecurity/normalizeExecAsk helpers for both defaults and per-agent entries — stripping invalid values so the downstream resolveExecApprovalsFromFile layer falls through to the correct built-in defaults. The test suite is thorough and covers the full round-trip through resolveExecApprovalsFromFile.

• Core fix (src/infra/exec-approvals.ts): normalizeExecApprovals now validates enum fields before constructing the normalized file, using the same helpers already relied upon in the resolution layer.
• Minor issue: The agentChanged comparison uses validatedSecurity !== agent.security; when agent.security is undefined, normalizeExecSecurity returns null, and null !== undefined is true. This causes every agent with absent security/ask/askFallback fields to be unnecessarily rebuilt with those keys explicitly set to undefined — functionally harmless but a false-positive in change detection. See inline comment.
• Tests (src/infra/exec-approvals-config.test.ts): Good coverage including per-field invalid value stripping, valid value preservation, agent-level and wildcard-agent entries, and a full resolution round-trip test.

Confidence Score: 5/5

Safe to merge; the bug fix is correct and well-tested, with only a minor false-positive in change detection that has no functional impact.

The core fix is correct: invalid enum values are now stripped at normalization time, and the downstream resolution layer correctly falls through to built-in defaults. The one remaining finding (null !== undefined false-positive causing spurious agent object rebuilds) is P2 — it does not affect JSON serialization, property access semantics, or runtime behavior in any observable way. All P2 findings default to 5/5.

No files require special attention for merge, though the agentChanged comparison in src/infra/exec-approvals.ts could be tightened to avoid spurious rebuilds of unchanged agent entries.

This is a comment left during a code review.
Path: src/infra/exec-approvals.ts
Line: 308-312

Comment:
**False-positive change detection for agents with absent fields**

When an agent doesn't have `security`, `ask`, or `askFallback` set at all (i.e. `undefined`), `normalizeExecSecurity(undefined)` returns `null`, making the comparison `null !== undefined` evaluate to `true`. This marks every agent without explicit security/ask/askFallback as changed, causing a new object to be created for it with those keys explicitly present as `undefined`.

Example: an agent like `{ allowlist: [...] }` (no security fields) will have `agentChanged = true` solely because `null !== undefined`, and the rebuilt object will have `security: undefined, ask: undefined, askFallback: undefined` as explicit keys — different from the original (no keys at all).

While functionally harmless today (`JSON.stringify` drops `undefined` and property access returns `undefined` either way), this defeats the purpose of the change-detection guard and makes the returned `agents` object differ in shape from the input even for unmodified entries.

A straightforward fix is to convert `null` → `undefined` before comparing:

```suggestion
    const agentChanged =
      allowlist !== agent.allowlist ||
      (validatedSecurity ?? undefined) !== agent.security ||
      (validatedAsk ?? undefined) !== agent.ask ||
      (validatedAskFallback ?? undefined) !== agent.askFallback;
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(exec): strip invalid security/ask en..." | Re-trigger Greptile}

🔒 Aisle Security Analysis

We found 2 potential security issue(s) in this PR:

function sanitizeExecApprovalPolicy(...): ExecApprovalsDefaults {
  return {
    ...,
    autoAllowSkills: policy?.autoAllowSkills,
  };
}

autoAllowSkills: Boolean(defaults.autoAllowSkills ?? fallbackAutoAllowSkills),

function toBooleanOrUndefined(value: unknown): boolean | undefined {
  return typeof value === "boolean" ? value : undefined;
}

function sanitizeExecApprovalPolicy(...): ExecApprovalsDefaults {
  return {
    security: normalizeExecSecurity(toStringOrUndefined(policy?.security)) ?? undefined,
    ask: normalizeExecAsk(toStringOrUndefined(policy?.ask)) ?? undefined,
    askFallback: normalizeExecSecurity(toStringOrUndefined(policy?.askFallback)) ?? undefined,
    autoAllowSkills: toBooleanOrUndefined((policy as any)?.autoAllowSkills),
  };
}

​// When resolving, treat only explicit true as enabled
autoAllowSkills: (defaults.autoAllowSkills ?? fallbackAutoAllowSkills) === true,

for (const [key, agent] of Object.entries(agents)) {
  ​// ...
  if (agentChanged) {
    agents[key] = {
      ...agent,
      allowlist,
      security: sanitizedPolicy.security,
      ask: sanitizedPolicy.ask,
      askFallback: sanitizedPolicy.askFallback,
    };
  }
}

const agents: Record<string, ExecApprovalsAgent> = Object.create(null);
for (const [key, agent] of Object.entries(file.agents ?? {})) {
  if (key === "__proto__" || key === "constructor" || key === "prototype") continue;
  agents[key] = agent;
}

​// ... then iterate over Object.entries(agents) safely

Analyzed PR: #59112 at commit 6f2c432

_{Last updated on: 2026-04-02T08:32:34Z}

All bot feedback has been addressed in the latest commits:

1. Prototype Pollution (CWE-1321): The agents dictionary is now built using Object.create(null), and dangerous keys (__proto__, constructor, prototype) are explicitly filtered out during normalization.
2. Type Confusion (CWE-704): Added a toStringOrUndefined helper to ensure non-string values (like numbers or booleans) in the config don't cause .trim() to throw a TypeError.
3. Object Reconstruction: Fixed the null !== undefined comparison issue raised by Greptile, so agents without explicit security fields no longer trigger unnecessary object reconstruction.

(Note: The "DoS via null agent entry" finding from the security bot refers to a scenario where an agent value in the JSON is null rather than an object—this is a pre-existing input validation gap that predates this PR's changes. Addressing it would require a broader type guard across all agent consumers, which falls outside the scope of this config enum normalization fix.)

The CI failure on checks-windows-node-test-5 is a transient infrastructure issue (The self-hosted runner lost communication with the server), unrelated to these changes.

Ready for review.