Skip to content

Route allow-always through command authorization planner#80922

Open
jesse-merhi wants to merge 55 commits into
mainfrom
jesse/command-auth-planner-contract
Open

Route allow-always through command authorization planner#80922
jesse-merhi wants to merge 55 commits into
mainfrom
jesse/command-auth-planner-contract

Conversation

@jesse-merhi
Copy link
Copy Markdown
Member

@jesse-merhi jesse-merhi commented May 12, 2026

Summary

  • Add the command authorization planner contract and Tree-sitter-backed POSIX planner corpus.
  • Route POSIX allowlist evaluation, allow-always persistence, approval display summaries, control-command rejection, and enforced shell rendering through the planner.
  • Remove the legacy POSIX chain/pipeline/heredoc parser from exec approval analysis; the remaining shell analyzer path is Windows-only.
  • Preserve first-miss chain behavior, safe wrapper payload handling, positional carrier guards, pipe-to-shell prompting, and strict inline-eval no-persistence behavior.

Real Behavior Proof

  • Ran a live terminal proof against this branch, outside the unit harness.
  • Command: fnm exec --using 22.21.0 pnpm exec tsx -e '<planner allowlist proof>'
  • Result: planner-rendered enforced pipeline '/usr/bin/printf' 'hi' | '/usr/bin/wc' '-c' executed through /bin/sh -lc and returned 2.
  • Result: allow-always on sh -c 'echo ok' persisted /bin/echo; a later sh -c 'id > /tmp/openclaw-parser-proof-marker' still had allowlistSatisfied: false.

Verification

  • fnm exec --using 22.21.0 pnpm test src/infra/command-authorization/corpus.test.ts src/infra/exec-approvals-analysis.test.ts src/infra/exec-approvals-allow-always.test.ts src/infra/exec-approvals-store.test.ts src/infra/exec-approvals-safe-bins.test.ts src/infra/command-analysis/explain.test.ts src/infra/command-analysis/explain.lazy.test.ts src/infra/exec-approvals-parity.test.ts src/agents/bash-tools.exec-host-gateway.test.ts src/node-host/invoke-system-run.test.ts
  • fnm exec --using 22.21.0 pnpm tsgo:core
  • fnm exec --using 22.21.0 pnpm tsgo:core:test
  • fnm exec --using 22.21.0 pnpm check:changed

Copilot AI review requested due to automatic review settings May 12, 2026 06:25
@openclaw-barnacle openclaw-barnacle Bot added size: L maintainer Maintainer-authored PR labels May 12, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 12, 2026

Codex review: needs maintainer review before merge.

Summary
The PR adds a planner-backed command authorization path and routes POSIX allowlist evaluation, allow-always persistence, approval summaries, control-command rejection, and enforced shell rendering through it.

Reproducibility: not applicable. this is a feature/refactor PR rather than a current-main bug report. The PR body provides terminal proof and the branch adds focused regression coverage for the preserved approval-ID behavior.

Real behavior proof
Not applicable: The external contributor proof gate does not apply to this member-authored PR with the protected maintainer label; the PR body still includes terminal proof for planner execution and allow-always persistence.

Next step before merge
Protected maintainer-labeled member PR with no narrow repair finding; the next action is normal maintainer/security review and merge-gate validation.

Security
Cleared: No concrete security or supply-chain regression was found, but the authorization-boundary change remains security-sensitive and needs maintainer review.

Review details

Best possible solution:

Land the planner-backed authorization path only after maintainer/security review confirms the command trust boundary and the focused plus broad validation gates are green.

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

Not applicable: this is a feature/refactor PR rather than a current-main bug report. The PR body provides terminal proof and the branch adds focused regression coverage for the preserved approval-ID behavior.

Is this the best way to solve the issue?

Yes, with maintainer review: centralizing POSIX allowlist and allow-always behavior through one planner is a maintainable direction, and this review did not find a discrete blocker in the latest head.

What I checked:

  • Live PR metadata: The PR is open, non-draft, mergeable, authored by a MEMBER, and still carries the protected maintainer label at head fe36a33. (fe36a338c50a)
  • Planner-backed allowlist path: Latest head routes non-Windows shell allowlist evaluation through planCommandForAuthorization and returns planner metadata with the allowlist analysis. (src/infra/exec-approvals-allowlist.ts:1632, fe36a338c50a)
  • Approval-ID semantics preserved: Latest head registers approval records before awaiting display analysis, hides list/get while analysis is pending, and has a regression test that resolving the explicit approval ID still succeeds during that pending-analysis window. (src/gateway/server-methods/server-methods.test.ts:1266, fe36a338c50a)
  • Current-main approval contract: Current main registers the approval record synchronously before request delivery and documents that the approval ID is valid immediately after the accepted response; the PR preserves this contract with the new pending-analysis path. (src/gateway/server-methods/exec-approval.ts:339, be166b9ae48d)
  • Planner corpus coverage: The new corpus exercises reusable POSIX pipelines plus prompt-only handling for unsupported modifiers, wrapper payloads, inline eval, dynamic payloads, relative wrapper executables, and shell state mutation. (src/infra/command-authorization/corpus.test.ts:75, fe36a338c50a)
  • Review discussion: Live PR review comments are Copilot notes on planner source normalization, unreachable suffix checks, and older analyzer-shape concerns; I did not find a concrete blocking runtime defect from those comments in the latest head. (src/infra/command-authorization/plan.ts:214, fe36a338c50a)

Likely related people:

  • @jesse-merhi: Prior merged work added optional exec command highlighting and approval risk display in the same gateway approval surface; this PR also comes from that contributor. (role: recent area contributor; confidence: high; commits: 79c2ed9065d0, 297a16453661; files: src/gateway/server-methods/exec-approval.ts, src/agents/bash-tools.exec-host-gateway.ts)
  • @steipete: Recent command-analysis refactors unified exec approval analysis and routed inline eval through the central command-analysis path this PR extends. (role: feature-history owner; confidence: high; commits: 3f7e6eebc2f5, bd0e10a2f68f; files: src/infra/exec-approvals-allowlist.ts, src/gateway/server-methods/exec-approval.ts, src/infra/command-analysis/policy.ts)
  • @pgondhi987: Recent merged safe-bin hardening touched the adjacent exec authorization and allowlist trust boundary that this PR changes. (role: recent security-hardening contributor; confidence: medium; commits: 9ac4272b35e9; files: src/infra/exec-approvals-allowlist.ts, src/infra/exec-approvals-safe-bins.test.ts)

Remaining risk / open question:

  • Large security-sensitive command authorization refactor was source-reviewed only in this read-only sweep; merge should still depend on maintainer review plus the focused and broad gates listed in the PR context.
  • Replacing the legacy POSIX parser with the Tree-sitter-backed planner leaves subtle shell edge cases as the main residual review area.

Codex review notes: model gpt-5.5, reasoning high; reviewed against be166b9ae48d.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces a new internal “command authorization planner” contract and a first-pass implementation that classifies commands into analyzable, prompt-only, or unanalyzable, producing a tree of command units suitable for downstream allowlist/trust evaluation.

Changes:

  • Added planner contract types (CommandAuthorizationPlan, units, tree relationships, and reason enums).
  • Implemented planCommandForAuthorization() with dialect handling and conservative prompt-only detection (inline eval, command substitution, Windows wrappers).
  • Added a corpus-style Vitest suite covering representative argv/POSIX/Windows wrapper/malformed cases.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
src/infra/command-authorization/types.ts Defines the planner contract types, tree structure, and reason enums.
src/infra/command-authorization/plan.ts Implements the planner and unit/tree construction logic using existing exec-approvals analyzers.
src/infra/command-authorization/index.ts Exposes the planner entrypoint and exported types.
src/infra/command-authorization/corpus.test.ts Adds corpus tests to lock in planner behavior across common command shapes and risk cases.

Comment thread src/infra/command-authorization/plan.ts Outdated
Comment on lines +52 to +53
const source = command ?? argvInput.join(" ");
const argv = argvInput.map((entry) => entry.trim()).filter((entry) => entry.length > 0);
Comment on lines +107 to +110
const source = command.trim();
if (!source) {
return unanalyzablePlan(command, "posix-shell", ["empty-command"]);
}
relationship: "wrapper-inline",
promptOnlyReasons: [reason],
});
return promptOnlyPlan(command, dialect, { kind: "unit", unitId: unit.id }, [unit]);
Comment on lines +325 to +328
if (executable === "cmd" || executable === "cmd.exe") {
return { dialect: "windows-cmd", reason: "unsupported-cmd-wrapper" };
}
if (executable === "powershell" || executable === "powershell.exe" || executable === "pwsh") {
Comment thread src/infra/command-authorization/plan.ts Outdated
command: part.part,
cwd: context.cwd,
env: context.env,
platform: context.platform,
Comment thread src/infra/command-authorization/plan.ts Outdated
Comment on lines +279 to +286
function unanalyzableFromAnalysis(
source: string,
dialect: CommandDialect,
analysis: ExecCommandAnalysis,
): CommandAuthorizationPlan {
const reason: CommandUnanalyzableReason =
analysis.reason === "empty command" ? "empty-command" : "malformed-shell";
return unanalyzablePlan(source, dialect, [reason]);
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XL and removed size: L labels May 13, 2026
@jesse-merhi jesse-merhi changed the title Add command authorization planner contract Route allow-always through command authorization planner May 13, 2026
@openclaw-barnacle openclaw-barnacle Bot added the gateway Gateway runtime label May 13, 2026
@jesse-merhi jesse-merhi force-pushed the jesse/command-auth-planner-contract branch 6 times, most recently from eb63177 to f95d24b Compare May 14, 2026 16:14
@openclaw-barnacle openclaw-barnacle Bot added the commands Command implementations label May 15, 2026
@jesse-merhi jesse-merhi force-pushed the jesse/command-auth-planner-contract branch from 76666de to 68a9521 Compare May 15, 2026 04:48
@jesse-merhi jesse-merhi force-pushed the jesse/command-auth-planner-contract branch from ecd2f84 to fe36a33 Compare May 15, 2026 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling commands Command implementations gateway Gateway runtime maintainer Maintainer-authored PR size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants