Skip to content

Commit 3f7e6ee

Browse files
steipetesteipete
committed
refactor: unify command analysis for exec approvals
1 parent 0ee52e9 commit 3f7e6ee

20 files changed

Lines changed: 843 additions & 146 deletions

‎src/agents/bash-tools.exec-host-gateway.ts‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { AgentToolResult } from "@mariozechner/pi-agent-core";
2+
import { detectPolicyInlineEval } from "../infra/command-analysis/policy.js";
23
import {
34
addDurableCommandApproval,
45
type ExecAsk,
@@ -12,10 +13,7 @@ import {
1213
resolveApprovalAuditCandidatePath,
1314
requiresExecApproval,
1415
} from "../infra/exec-approvals.js";
15-
import {
16-
describeInterpreterInlineEval,
17-
detectInterpreterInlineEvalArgv,
18-
} from "../infra/exec-inline-eval.js";
16+
import { describeInterpreterInlineEval } from "../infra/exec-inline-eval.js";
1917
import type { SafeBinProfile } from "../infra/exec-safe-bin-policy.js";
2018
import { markBackgrounded, tail } from "./bash-process-registry.js";
2119
import {
@@ -292,13 +290,7 @@ export async function processGatewayAllowlist(
292290
commandText: params.command,
293291
});
294292
const inlineEvalHit =
295-
params.strictInlineEval === true
296-
? (allowlistEval.segments
297-
.map((segment) =>
298-
detectInterpreterInlineEvalArgv(segment.resolution?.effectiveArgv ?? segment.argv),
299-
)
300-
.find((entry) => entry !== null) ?? null)
301-
: null;
293+
params.strictInlineEval === true ? detectPolicyInlineEval(allowlistEval.segments) : null;
302294
if (inlineEvalHit) {
303295
params.warnings.push(
304296
`Warning: strict inline-eval mode requires explicit approval for ${describeInterpreterInlineEval(

‎src/agents/bash-tools.exec-host-node-phases.ts‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import crypto from "node:crypto";
22
import type { AgentToolResult } from "@mariozechner/pi-agent-core";
3+
import { detectPolicyInlineEval } from "../infra/command-analysis/policy.js";
34
import {
45
type ExecApprovalsFile,
56
type ExecAsk,
@@ -11,7 +12,7 @@ import {
1112
} from "../infra/exec-approvals.js";
1213
import {
1314
describeInterpreterInlineEval,
14-
detectInterpreterInlineEvalArgv,
15+
type InterpreterInlineEvalHit,
1516
} from "../infra/exec-inline-eval.js";
1617
import { buildNodeShellCommand } from "../infra/node-shell.js";
1718
import { parsePreparedSystemRunPayload } from "../infra/system-run-approval-context.js";
@@ -46,7 +47,7 @@ type NodeApprovalAnalysis = {
4647
analysisOk: boolean;
4748
allowlistSatisfied: boolean;
4849
durableApprovalSatisfied: boolean;
49-
inlineEvalHit: ReturnType<typeof detectInterpreterInlineEvalArgv>;
50+
inlineEvalHit: InterpreterInlineEvalHit | null;
5051
};
5152

5253
export function shouldSkipNodeApprovalPrepare(params: {
@@ -293,11 +294,7 @@ export async function analyzeNodeApprovalRequirement(params: {
293294
let durableApprovalSatisfied = false;
294295
const inlineEvalHit =
295296
params.request.strictInlineEval === true
296-
? (baseAllowlistEval.segments
297-
.map((segment) =>
298-
detectInterpreterInlineEvalArgv(segment.resolution?.effectiveArgv ?? segment.argv),
299-
)
300-
.find((entry) => entry !== null) ?? null)
297+
? detectPolicyInlineEval(baseAllowlistEval.segments)
301298
: null;
302299
if (inlineEvalHit) {
303300
params.request.warnings.push(

‎src/agents/bash-tools.exec.ts‎

Lines changed: 8 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
getShellPathFromLoginShell,
1717
resolveShellEnvFallbackTimeoutMs,
1818
} from "../infra/shell-env.js";
19+
import { extractShellWrapperInlineCommand } from "../infra/shell-wrapper-resolution.js";
1920
import { logInfo } from "../logger.js";
2021
import { parseAgentSessionKey, resolveAgentIdFromSessionKey } from "../routing/session-key.js";
2122
import { createLazyImportLoader } from "../shared/lazy-promise.js";
@@ -1141,7 +1142,6 @@ function parseOpenClawChannelsLoginShellCommand(raw: string): boolean {
11411142
function rejectUnsafeControlShellCommand(command: string): void {
11421143
const isEnvAssignmentToken = (token: string): boolean =>
11431144
/^[A-Za-z_][A-Za-z0-9_]*=.*$/u.test(token);
1144-
const shellWrappers = new Set(["bash", "dash", "fish", "ksh", "sh", "zsh"]);
11451145
const commandStandaloneOptions = new Set(["-p", "-v", "-V"]);
11461146
const envOptionsWithValues = new Set([
11471147
"-C",
@@ -1310,35 +1310,19 @@ function rejectUnsafeControlShellCommand(command: string): void {
13101310
}
13111311
return remaining;
13121312
};
1313-
const extractShellWrapperPayload = (argv: string[]): string[] => {
1314-
const [commandName, ...rest] = argv;
1315-
if (!commandName || !shellWrappers.has(path.basename(commandName))) {
1316-
return [];
1317-
}
1318-
for (let i = 0; i < rest.length; i += 1) {
1319-
const token = rest[i];
1320-
if (!token) {
1321-
continue;
1322-
}
1323-
if (token === "-c" || token === "-lc" || token === "-ic" || token === "-xc") {
1324-
return rest[i + 1] ? [rest[i + 1]] : [];
1325-
}
1326-
if (/^-[^-]*c[^-]*$/u.test(token)) {
1327-
return rest[i + 1] ? [rest[i + 1]] : [];
1328-
}
1329-
}
1330-
return [];
1331-
};
13321313
const buildCandidates = (argv: string[]): string[] => {
13331314
const envSplitCandidates = extractEnvSplitStringPayload(argv).flatMap((payload) => {
13341315
const innerArgv = splitShellArgs(payload);
13351316
return innerArgv ? buildCandidates(innerArgv) : [payload];
13361317
});
13371318
const stripped = stripApprovalCommandPrefixes(argv);
1338-
const shellWrapperCandidates = extractShellWrapperPayload(stripped).flatMap((payload) => {
1339-
const innerArgv = splitShellArgs(payload);
1340-
return innerArgv ? buildCandidates(innerArgv) : [payload];
1341-
});
1319+
const shellWrapperPayload = extractShellWrapperInlineCommand(stripped);
1320+
const shellWrapperCandidates = shellWrapperPayload
1321+
? (() => {
1322+
const innerArgv = splitShellArgs(shellWrapperPayload);
1323+
return innerArgv ? buildCandidates(innerArgv) : [shellWrapperPayload];
1324+
})()
1325+
: [];
13421326
return [
13431327
...(stripped.length > 0 ? [stripped.join(" ")] : []),
13441328
...envSplitCandidates,

‎src/gateway/server-methods/exec-approval.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
import {
2+
summarizeCommandSegmentsForDisplay,
3+
type CommandExplanationSummary,
4+
} from "../../infra/command-analysis/explain.js";
5+
import { analyzeCommandForPolicy } from "../../infra/command-analysis/policy.js";
16
import {
27
resolveExecApprovalCommandDisplay,
38
sanitizeExecApprovalDisplayText,
@@ -42,6 +47,43 @@ const APPROVAL_ALLOW_ALWAYS_UNAVAILABLE_DETAILS = {
4247
} as const;
4348
const RESERVED_PLUGIN_APPROVAL_ID_PREFIX = "plugin:";
4449

50+
function sanitizeCommandAnalysisSummary(
51+
summary: CommandExplanationSummary,
52+
): CommandExplanationSummary {
53+
return {
54+
commandCount: summary.commandCount,
55+
nestedCommandCount: summary.nestedCommandCount,
56+
riskKinds: summary.riskKinds.map((kind) => sanitizeExecApprovalWarningText(kind)),
57+
warningLines: summary.warningLines.map((line) => sanitizeExecApprovalWarningText(line)),
58+
};
59+
}
60+
61+
function resolveExecApprovalCommandAnalysis(params: {
62+
host: string;
63+
commandText: string;
64+
commandArgv?: string[];
65+
cwd?: string | null;
66+
}): CommandExplanationSummary | null {
67+
const analysis =
68+
Array.isArray(params.commandArgv) && params.commandArgv.length > 0
69+
? analyzeCommandForPolicy({
70+
source: "argv",
71+
argv: params.commandArgv,
72+
cwd: params.cwd ?? undefined,
73+
})
74+
: params.host === "node"
75+
? null
76+
: analyzeCommandForPolicy({
77+
source: "shell",
78+
command: params.commandText,
79+
cwd: params.cwd ?? undefined,
80+
});
81+
if (!analysis?.ok) {
82+
return null;
83+
}
84+
return sanitizeCommandAnalysisSummary(summarizeCommandSegmentsForDisplay(analysis.segments));
85+
}
86+
4587
type ExecApprovalIosPushDelivery = {
4688
handleRequested?: (request: ExecApprovalRequest) => Promise<boolean>;
4789
handleResolved?: (resolved: ExecApprovalResolved) => Promise<void>;
@@ -207,6 +249,12 @@ export function createExecApprovalHandlers(
207249
}
208250
const envBinding = buildSystemRunApprovalEnvBinding(p.env);
209251
const warningText = normalizeOptionalString(p.warningText);
252+
const commandAnalysis = resolveExecApprovalCommandAnalysis({
253+
host,
254+
commandText: effectiveCommandText,
255+
commandArgv: effectiveCommandArgv,
256+
cwd: effectiveCwd,
257+
});
210258
const systemRunBinding =
211259
host === "node"
212260
? buildSystemRunApprovalBinding({
@@ -241,6 +289,7 @@ export function createExecApprovalHandlers(
241289
security: p.security ?? null,
242290
ask: p.ask ?? null,
243291
warningText: warningText ? sanitizeExecApprovalWarningText(warningText) : null,
292+
commandAnalysis,
244293
allowedDecisions: resolveExecApprovalAllowedDecisions({ ask: p.ask ?? null }),
245294
agentId: effectiveAgentId ?? null,
246295
resolvedPath: p.resolvedPath ?? null,

‎src/gateway/server-methods/server-methods.test.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -897,6 +897,43 @@ describe("exec approval handlers", () => {
897897
await requestPromise;
898898
});
899899

900+
it("attaches shared command analysis to gateway exec approval requests", async () => {
901+
const { handlers, broadcasts, respond, context } = createExecApprovalFixture();
902+
903+
const requestPromise = requestExecApproval({
904+
handlers,
905+
respond,
906+
context,
907+
params: {
908+
twoPhase: true,
909+
host: "gateway",
910+
command: "python3 -c 'print(1)'",
911+
commandArgv: ["python3", "-c", "print(1)"],
912+
systemRunPlan: undefined,
913+
nodeId: undefined,
914+
},
915+
});
916+
917+
const requested = broadcasts.find((entry) => entry.event === "exec.approval.requested");
918+
const request = requested?.payload as { id?: string; request?: { commandAnalysis?: unknown } };
919+
expect(request.request?.commandAnalysis).toEqual(
920+
expect.objectContaining({
921+
commandCount: 1,
922+
riskKinds: expect.arrayContaining(["inline-eval"]),
923+
warningLines: expect.arrayContaining(["Contains inline-eval: python3 -c"]),
924+
}),
925+
);
926+
927+
const resolveRespond = vi.fn();
928+
await resolveExecApproval({
929+
handlers,
930+
id: request.id ?? "",
931+
respond: resolveRespond,
932+
context,
933+
});
934+
await requestPromise;
935+
});
936+
900937
it("lists pending exec approvals", async () => {
901938
const { handlers, respond, context } = createExecApprovalFixture();
902939
const requestPromise = requestExecApproval({

‎src/infra/approval-view-model.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ function buildExecViewBase<TPhase extends ApprovalPhase>(
6969
ask: request.request.ask ?? null,
7070
agentId: request.request.agentId ?? null,
7171
warningText: request.request.warningText ?? null,
72+
commandAnalysis: request.request.commandAnalysis ?? null,
7273
commandText,
7374
commandPreview,
7475
cwd: request.request.cwd ?? null,

‎src/infra/approval-view-model.types.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { InteractiveReplyButton } from "../interactive/payload.js";
22
import type { ChannelApprovalKind } from "./approval-types.js";
3+
import type { CommandExplanationSummary } from "./command-analysis/explain.js";
34
import type {
45
ExecApprovalDecision,
56
ExecApprovalRequest,
@@ -35,6 +36,7 @@ export type ExecApprovalViewBase = ApprovalViewBase & {
3536
ask?: string | null;
3637
agentId?: string | null;
3738
warningText?: string | null;
39+
commandAnalysis?: CommandExplanationSummary | null;
3840
commandText: string;
3941
commandPreview?: string | null;
4042
cwd?: string | null;

‎src/infra/command-analysis/explain.test.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import { describe, expect, it } from "vitest";
2+
import { explainShellCommand } from "../command-explainer/index.js";
3+
import { summarizeCommandExplanation, summarizeCommandSegmentsForDisplay } from "./explain.js";
4+
5+
describe("command-analysis explanation summary", () => {
6+
it("summarizes commands and risk kinds", async () => {
7+
const explanation = await explainShellCommand(`bash -lc 'python3 -c "print(1)"'`);
8+
const summary = summarizeCommandExplanation(explanation);
9+
10+
expect(summary.commandCount).toBe(1);
11+
expect(summary.riskKinds).toContain("shell-wrapper");
12+
expect(summary.riskKinds).toContain("inline-eval");
13+
expect(summary.warningLines.some((line) => line.includes("inline-eval"))).toBe(true);
14+
});
15+
16+
it("summarizes policy command segments without async parsing", () => {
17+
const summary = summarizeCommandSegmentsForDisplay([
18+
{
19+
raw: "sudo python3 -c 'print(1)'",
20+
argv: ["sudo", "python3", "-c", "print(1)"],
21+
resolution: null,
22+
},
23+
]);
24+
25+
expect(summary.commandCount).toBe(1);
26+
expect(summary.riskKinds).toEqual(["inline-eval"]);
27+
expect(summary.warningLines).toEqual(["Contains inline-eval: python3 -c"]);
28+
});
29+
});

‎src/infra/command-analysis/explain.ts‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
import { explainShellCommand } from "../command-explainer/extract.js";
2+
import type { CommandExplanation, CommandRisk } from "../command-explainer/types.js";
3+
import type { ExecCommandSegment } from "../exec-approvals-analysis.js";
4+
import { detectCommandCarrierArgv, detectInlineEvalInSegments } from "./risks.js";
5+
6+
export type CommandExplanationSummary = {
7+
commandCount: number;
8+
nestedCommandCount: number;
9+
riskKinds: string[];
10+
warningLines: string[];
11+
};
12+
13+
function riskLabel(risk: CommandRisk): string {
14+
switch (risk.kind) {
15+
case "inline-eval":
16+
return `${risk.command} ${risk.flag}`;
17+
case "shell-wrapper":
18+
return `${risk.executable} ${risk.flag}`;
19+
case "command-carrier":
20+
return risk.flag ? `${risk.command} ${risk.flag}` : risk.command;
21+
case "dynamic-argument":
22+
return `${risk.command} dynamic argument`;
23+
case "source":
24+
return risk.command;
25+
case "function-definition":
26+
return risk.name;
27+
default:
28+
return risk.kind;
29+
}
30+
}
31+
32+
export function summarizeCommandExplanation(
33+
explanation: CommandExplanation,
34+
): CommandExplanationSummary {
35+
const riskKinds = [...new Set(explanation.risks.map((risk) => risk.kind))];
36+
const warningLines = explanation.risks.map((risk) => {
37+
const label = riskLabel(risk);
38+
return label === risk.kind ? `Contains ${risk.kind}` : `Contains ${risk.kind}: ${label}`;
39+
});
40+
return {
41+
commandCount: explanation.topLevelCommands.length,
42+
nestedCommandCount: explanation.nestedCommands.length,
43+
riskKinds,
44+
warningLines: [...new Set(warningLines)],
45+
};
46+
}
47+
48+
function uniqueStrings(values: string[]): string[] {
49+
return [...new Set(values)];
50+
}
51+
52+
export function summarizeCommandSegmentsForDisplay(
53+
segments: readonly ExecCommandSegment[],
54+
): CommandExplanationSummary {
55+
const riskKinds: string[] = [];
56+
const warningLines: string[] = [];
57+
const inlineEval = detectInlineEvalInSegments(segments);
58+
if (inlineEval) {
59+
riskKinds.push("inline-eval");
60+
warningLines.push(
61+
`Contains inline-eval: ${inlineEval.normalizedExecutable} ${inlineEval.flag}`,
62+
);
63+
}
64+
for (const segment of segments) {
65+
const effectiveArgv = segment.resolution?.effectiveArgv ?? segment.argv;
66+
for (const hit of detectCommandCarrierArgv(effectiveArgv)) {
67+
riskKinds.push("command-carrier");
68+
warningLines.push(
69+
hit.flag
70+
? `Contains command-carrier: ${hit.command} ${hit.flag}`
71+
: `Contains command-carrier: ${hit.command}`,
72+
);
73+
}
74+
}
75+
return {
76+
commandCount: segments.length,
77+
nestedCommandCount: 0,
78+
riskKinds: uniqueStrings(riskKinds),
79+
warningLines: uniqueStrings(warningLines),
80+
};
81+
}
82+
83+
export async function explainCommandForDisplay(
84+
command: string,
85+
): Promise<{ explanation: CommandExplanation; summary: CommandExplanationSummary } | null> {
86+
try {
87+
const explanation = await explainShellCommand(command);
88+
return { explanation, summary: summarizeCommandExplanation(explanation) };
89+
} catch {
90+
return null;
91+
}
92+
}

‎src/infra/command-analysis/index.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
export * from "./explain.js";
2+
export * from "./policy.js";
3+
export * from "./risks.js";

0 commit comments

Comments
 (0)