Add command authorization planner without shell wrapper reuse#84118
Closed
jesse-merhi wants to merge 1 commit into
Closed
Add command authorization planner without shell wrapper reuse#84118jesse-merhi wants to merge 1 commit into
jesse-merhi wants to merge 1 commit into
Conversation
openclaw-barnacle
Bot
added
gateway
Contributor
|
Codex review: found issues before merge. Reviewed June 15, 2026, 2:41 AM ET / 06:41 UTC. Summary PR surface: Source +165, Tests +154. Total +319 across 23 files. Reproducibility: yes. for the blocking PR defect by source inspection: the PR builds and broadcasts the approval event with commandAnalysis set to null, then mutates only the in-memory record after delivery paths have already consumed the event. The broader planner behavior was not executed in this read-only review. Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Use the newer planner PR after its own blocker is fixed, or refresh this branch only if maintainers explicitly want the narrower no-wrapper-reuse shape with delivered command analysis preserved and focused exec approval validation. Do we have a high-confidence way to reproduce the issue? Yes for the blocking PR defect by source inspection: the PR builds and broadcasts the approval event with commandAnalysis set to null, then mutates only the in-memory record after delivery paths have already consumed the event. The broader planner behavior was not executed in this read-only review. Is this the best way to solve the issue? No as written. The planner direction may be the right architecture, but this branch must preserve commandAnalysis at delivery time and prove compatibility, or defer to the newer planner PR once that branch is fixed. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against bcb016a528b1. Label changesLabel justifications:
Evidence reviewedPR surface: Source +165, Tests +154. Total +319 across 23 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces the legacy POSIX shell parser used by exec approval analysis with a new command authorization planner. The planner produces a typed plan (analyzable / prompt-only / unanalyzable) that drives allowlist evaluation, allow-always persistence, and safe-bin / enforced shell-command rendering for non-Windows hosts, while Windows remains on the existing analyzer. The path that derived reusable trust from shell-wrapper inner payloads is intentionally removed; exact-command durable approval remains.
Changes:
- Introduces
src/infra/command-authorization(types.ts,plan.ts,index.ts) and corpus tests; wiresevaluateShellAllowlist,persistAllowAlwaysPatterns,analyzeCommandForPolicy, andresolveCommandAnalysisSummaryForDisplayto be async planner-backed. - Deletes legacy POSIX heredoc/pipeline/chain parsing and
buildSafeBinsShellCommand/splitCommandChain*fromexec-approvals-analysis.ts;analyzeShellCommandandbuildEnforcedShellCommandnow only handle Windows. - Threads
authorizationPlanthroughSystemRunAllowlistAnalysis, system-run policy phase, and gateway allowlist enforcement; gateway approval request now resolves command analysis asynchronously.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/infra/command-authorization/types.ts | New plan/unit/relationship types for the authorization planner. |
| src/infra/command-authorization/plan.ts | Planner implementation using explainShellCommand; builds tree/units and renders enforced/safe-bins shell commands. |
| src/infra/command-authorization/index.ts | Barrel exports for planner API and types. |
| src/infra/command-authorization/corpus.test.ts | Corpus tests covering argv/POSIX shape, render modes, prompt-only and unanalyzable plans. |
| src/infra/exec-approvals-analysis.ts | Removes POSIX shell parser/rebuilders; restricts analyzeShellCommand/buildEnforcedShellCommand to Windows. |
| src/infra/exec-approvals-analysis.test.ts | Drops POSIX shell parsing/heredoc tests; renames Windows-only describe. |
| src/infra/exec-approvals-allowlist.ts | evaluateShellAllowlist becomes async planner-backed; adds planner-based allow-always entry resolvers; drops inline-chain recursion. |
| src/infra/exec-approvals-allow-always.test.ts | Migrates tests to planner-backed helper; updates chain expectations to first-missed binary. |
| src/infra/exec-approvals.ts | persistAllowAlwaysPatterns becomes async; routes through planner when command text is available on non-Windows. |
| src/infra/exec-approvals-store.test.ts | New planner-based persistence tests covering unit scoping. |
| src/infra/exec-approvals-safe-bins.test.ts | Adjusts safe-bin tests to await async evaluateShellAllowlist. |
| src/infra/exec-approvals-parity.test.ts | Reroutes shell-parser parity fixture through the planner. |
| src/infra/command-analysis/policy.ts | analyzeCommandForPolicy becomes async; POSIX path uses planner via lazy import. |
| src/infra/command-analysis/explain.ts | resolveCommandAnalysisSummaryForDisplay becomes async. |
| src/infra/command-analysis/explain.test.ts / explain.lazy.test.ts | Awaits async API; lazy test now uses argv-host path. |
| src/node-host/invoke-system-run-allowlist.ts | Threads planner through allowlist evaluation; safe-bins shell rebuild uses planner renderer. |
| src/node-host/invoke-system-run.ts | Carries authorizationPlan through policy/execute phases; awaits async persistence with new analysisOk/commandText. |
| src/gateway/server-methods/exec-approval.ts | Resolves command analysis asynchronously; broadcasts request with commandAnalysis: null then mutates record on resolve. |
| src/gateway/server-methods/server-methods.test.ts | Adds vi.waitFor for the async analysis update. |
| src/agents/bash-tools.exec.ts | rejectUnsafeControlShellCommand becomes async/planner-backed. |
| src/agents/bash-tools.exec-host-node-phases.ts | Awaits async evaluateShellAllowlist. |
| src/agents/bash-tools.exec-host-gateway.ts | Uses planner-backed enforced shell rendering and async persistAllowAlwaysPatterns. |
Comment on lines
278
to
+365
| @@ -320,7 +323,7 @@ export function createExecApprovalHandlers( | |||
| security: p.security ?? null, | |||
| ask: p.ask ?? null, | |||
| warningText: warningText ? sanitizeExecApprovalWarningText(warningText) : null, | |||
| commandAnalysis, | |||
| commandAnalysis: null, | |||
| commandSpans, | |||
| allowedDecisions: resolveExecApprovalAllowedDecisions({ ask: p.ask ?? null }), | |||
| agentId: effectiveAgentId ?? null, | |||
| @@ -357,6 +360,9 @@ export function createExecApprovalHandlers( | |||
| createdAtMs: record.createdAtMs, | |||
| expiresAtMs: record.expiresAtMs, | |||
| }; | |||
| void commandAnalysisPromise.then((commandAnalysis) => { | |||
| record.request.commandAnalysis = commandAnalysis; | |||
| }); | |||
Comment on lines
+1173
to
+1215
| async function collectAllowAlwaysPatternsAsync(params: { | ||
| segment: ExecCommandSegment; | ||
| cwd?: string; | ||
| env?: NodeJS.ProcessEnv; | ||
| platform?: string | null; | ||
| strictInlineEval?: boolean; | ||
| depth: number; | ||
| out: AllowAlwaysPattern[]; | ||
| }): Promise<void> { | ||
| if (params.depth >= 3) { | ||
| return; | ||
| } | ||
|
|
||
| const trustPlan = resolveExecWrapperTrustPlan(params.segment.argv); | ||
| if (trustPlan.policyBlocked) { | ||
| return; | ||
| } | ||
| const segment = | ||
| trustPlan.argv === params.segment.argv | ||
| ? params.segment | ||
| : { | ||
| raw: trustPlan.argv.join(" "), | ||
| argv: trustPlan.argv, | ||
| resolution: resolveCommandResolutionFromArgv(trustPlan.argv, params.cwd, params.env), | ||
| }; | ||
|
|
||
| const candidatePath = resolveExecutionTargetCandidatePath(segment.resolution, params.cwd); | ||
| if (!candidatePath) { | ||
| return; | ||
| } | ||
| if (isInterpreterLikeAllowlistPattern(candidatePath)) { | ||
| const effectiveArgv = segment.resolution?.effectiveArgv ?? segment.argv; | ||
| if (params.strictInlineEval !== true || detectInlineEvalArgv(effectiveArgv) !== null) { | ||
| return; | ||
| } | ||
| } | ||
| if (!trustPlan.shellWrapperExecutable) { | ||
| const argPattern = buildArgPatternFromArgv(segment.argv, params.platform); | ||
| addAllowAlwaysPattern(params.out, candidatePath, argPattern); | ||
| return; | ||
| } | ||
| return; | ||
| } |
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
P2
Member
Author
|
Closing this as superseded by #84172. What changed between the two branches:
Given that #84172 is the active fuller version and this branch is stale/conflicting, keeping #84118 open would just split review attention. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Out of scope
sh -c 'echo ok'does not persist/bin/echoand does not auto-run from an/bin/echoallowlist entry.Verification
Real Behavior Proof
Behavior addressed: planner-backed exec approval analysis without shell-wrapper reusable trust.
Real environment tested: not tested after this split.
Exact steps or command run after this patch: not run.
Evidence after fix: not collected.
Observed result after fix: not verified.
What was not tested: unit tests, typecheck, changed checks, and live exec approval behavior.