diff --git a/src/agents/pi-embedded-subscribe.handlers.messages.ts b/src/agents/pi-embedded-subscribe.handlers.messages.ts index d1f0234b1dd..f813c8855f7 100644 --- a/src/agents/pi-embedded-subscribe.handlers.messages.ts +++ b/src/agents/pi-embedded-subscribe.handlers.messages.ts @@ -182,11 +182,6 @@ export function handleMessageStart( // may deliver late text_end updates after message_end, which would otherwise // re-trigger block replies. ctx.resetAssistantMessageState(ctx.state.assistantTexts.length); - // Resolve text-repetition-guard config once per message to avoid re-parsing on every tick. - ctx.state.resolvedTextRepetitionGuardConfig = resolveTextRepetitionGuardConfig({ - cfg: ctx.params.config, - agentId: ctx.params.agentId, - }); // Use assistant message_start as the earliest "writing" signal for typing. void ctx.params.onAssistantMessageStart?.(); } @@ -287,21 +282,6 @@ export function handleMessageUpdate( } } - // Text repetition guard: throttle checks to every N chars of new content. - const guardConfig = ctx.state.resolvedTextRepetitionGuardConfig; - if (guardConfig && guardConfig.enabled) { - const checkInterval = guardConfig.checkIntervalChars; - if (ctx.state.deltaBuffer.length - ctx.state.textRepetitionLastCheckedLen >= checkInterval) { - ctx.state.textRepetitionLastCheckedLen = ctx.state.deltaBuffer.length; - const result = detectTextRepetition(ctx.state.deltaBuffer, guardConfig); - if (result.looping) { - ctx.state.abortedByTextRepetitionGuard = true; - void ctx.params.session.abort(); - return; - } - } - } - if (ctx.state.streamReasoning) { // Handle partial tags: stream whatever reasoning is visible so far. ctx.emitReasoningStream(extractThinkingFromTaggedStream(ctx.state.deltaBuffer)); @@ -538,10 +518,10 @@ export function handleMessageEnd( text && onBlockReply ) { - if (ctx.blockChunker?.hasBuffered() && !ctx.state.abortedByTextRepetitionGuard) { + if (ctx.blockChunker?.hasBuffered()) { ctx.blockChunker.drain({ force: true, emit: ctx.emitBlockChunk }); ctx.blockChunker.reset(); - } else if (text !== ctx.state.lastBlockReplyText && !ctx.state.abortedByTextRepetitionGuard) { + } else if (text !== ctx.state.lastBlockReplyText) { // Check for duplicates before emitting (same logic as emitBlockChunk). const normalizedText = normalizeTextForComparison(text); if ( diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index fb5be71ec03..87085bb934f 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -367,73 +367,44 @@ function resolveTrustedSkillExecutionIds(params: { return skillIds; } -/** - * Maximum recursion depth for evaluating shell wrapper inline commands. - * Prevents infinite recursion from nested shell wrappers (e.g. `sh -c "sh -c '...'"`) - * while allowing reasonable nesting for skill-constructed compound commands. - */ const MAX_SHELL_WRAPPER_INLINE_EVAL_DEPTH = 3; -/** - * Recursively evaluates a shell wrapper's inline command (the `-c` payload) against the - * allowlist by parsing it into sub-commands and checking each one. - * - * Returns the evaluation result when the inline command is parseable and all sub-commands - * are satisfied, or `null` when evaluation fails or any sub-command is unsatisfied. - */ -function evaluateShellWrapperInlineCommand( - inlineCommand: string, - params: ExecAllowlistContext, - inlineDepth: number, - precomputedChainParts?: string[], -): { matches: ExecAllowlistEntry[]; segmentSatisfiedBy: ExecSegmentSatisfiedBy[] } | null { - if (inlineDepth >= MAX_SHELL_WRAPPER_INLINE_EVAL_DEPTH) { +function evaluateShellWrapperInlineChain(params: { + inlineCommand: string; + context: ExecAllowlistContext; + inlineDepth: number; + precomputedChainParts?: string[]; +}): { matches: ExecAllowlistEntry[] } | null { + if (params.inlineDepth >= MAX_SHELL_WRAPPER_INLINE_EVAL_DEPTH) { + return null; + } + if (isWindowsPlatform(params.context.platform)) { + return null; + } + const chainParts = params.precomputedChainParts ?? splitCommandChain(params.inlineCommand); + if (!chainParts || chainParts.length <= 1) { return null; } - const chainParts = - precomputedChainParts ?? (isWindowsPlatform(params.platform) ? null : splitCommandChain(inlineCommand)); - - if (!chainParts) { - // Single command or pipeline (no chain operators). - const analysis = analyzeShellCommand({ - command: inlineCommand, - cwd: params.cwd, - env: params.env, - platform: params.platform, - }); - if (!analysis.ok) { - return null; - } - const result = evaluateSegments(analysis.segments, params, inlineDepth); - return result.satisfied - ? { matches: result.matches, segmentSatisfiedBy: result.segmentSatisfiedBy } - : null; - } - - // Multiple commands joined by chain operators (&&, ||, ;). - const allMatches: ExecAllowlistEntry[] = []; - const allSegmentSatisfiedBy: ExecSegmentSatisfiedBy[] = []; + const matches: ExecAllowlistEntry[] = []; for (const part of chainParts) { const analysis = analyzeShellCommand({ command: part, - cwd: params.cwd, - env: params.env, - platform: params.platform, + cwd: params.context.cwd, + env: params.context.env, + platform: params.context.platform, }); if (!analysis.ok) { return null; } - const result = evaluateSegments(analysis.segments, params, inlineDepth); + const result = evaluateSegments(analysis.segments, params.context, params.inlineDepth); if (!result.satisfied) { return null; } - allMatches.push(...result.matches); - allSegmentSatisfiedBy.push(...result.segmentSatisfiedBy); + matches.push(...result.matches); } - return { matches: allMatches, segmentSatisfiedBy: allSegmentSatisfiedBy }; + return { matches }; } - function evaluateSegments( segments: ExecCommandSegment[], params: ExecAllowlistContext, @@ -550,34 +521,24 @@ function evaluateSegments( : skillAllow ? "skills" : null; - - // When no direct match and the segment is a shell wrapper with an inline *compound* - // command (e.g. `/bin/sh -c "cat SKILL.md && gog-wrapper calendar events"`), - // recursively parse and evaluate each sub-command inside the inline payload. - // This prevents shell-wrapped skill exec commands from being silently rejected - // because the allowlist check was comparing `/bin/sh` instead of the actual target - // binaries inside the compound command. - // - // Only recurse when the inline command contains chain operators (&&, ||, ;) — - // single-command shell wrappers (e.g. `bash -lc 'script.sh'`) keep the original - // behavior to preserve allow-always persisted-pattern security constraints. - const inlineChainParts = by === null && inlineCommand ? splitCommandChain(inlineCommand) : null; - if (inlineChainParts !== null && inlineChainParts.length > 1) { - const inlineResult = evaluateShellWrapperInlineCommand( - inlineCommand, - params, - inlineDepth + 1, - inlineChainParts, - ); - if (inlineResult !== null) { - matches.push(...inlineResult.matches); - // Preserve the per-sub-command satisfaction reasons from the inline evaluation - // rather than collapsing them into a single "allowlist" entry for the wrapper segment. - segmentSatisfiedBy.push(...inlineResult.segmentSatisfiedBy); - return true; + if (by === null && inlineCommand) { + const inlineChainParts = splitCommandChain(inlineCommand); + if (inlineChainParts && inlineChainParts.length > 1) { + const inlineResult = evaluateShellWrapperInlineChain({ + inlineCommand, + context: params, + inlineDepth: inlineDepth + 1, + precomputedChainParts: inlineChainParts, + }); + if (inlineResult) { + matches.push(...inlineResult.matches); + // Keep per-segment metadata aligned with segments: one satisfaction marker + // for this wrapper segment, even when the inline payload has multiple parts. + segmentSatisfiedBy.push("allowlist"); + return true; + } } } - segmentSatisfiedBy.push(by); return Boolean(by); }); diff --git a/src/infra/exec-approvals-analysis.test.ts b/src/infra/exec-approvals-analysis.test.ts index 70052675224..b164561b772 100644 --- a/src/infra/exec-approvals-analysis.test.ts +++ b/src/infra/exec-approvals-analysis.test.ts @@ -645,7 +645,7 @@ describe("exec approvals shell analysis", () => { env, }); expect(result.analysisOk).toBe(true); - expect(result.allowlistSatisfied).toBe(false); + expect(result.allowlistSatisfied).toBe(true); } finally { fs.rmSync(dir, { recursive: true, force: true }); } @@ -654,138 +654,101 @@ describe("exec approvals shell analysis", () => { it("normalizes safe bin names", () => { expect([...normalizeSafeBins([" jq ", "", "JQ", " sort "])]).toEqual(["jq", "sort"]); }); - }); - describe("shell wrapper inline command evaluation (#57377)", () => { - it("satisfies allowlist for shell-wrapped compound command when all inner binaries match", () => { - const dir = makeTempDir(); - const catPath = path.join(dir, "cat"); - const printfPath = path.join(dir, "printf"); - const gogPath = path.join(dir, "gog-wrapper"); - const shPath = path.join(dir, "sh"); - fs.writeFileSync(catPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(printfPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(gogPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(shPath, "#!/bin/sh\n", { mode: 0o755 }); - const env = makePathEnv(dir); - try { - const result = evaluateShellAllowlist({ - command: `${shPath} -c "cat SKILL.md && printf '---CMD---' && gog-wrapper calendar events"`, - allowlist: [{ pattern: catPath }, { pattern: printfPath }, { pattern: gogPath }], - safeBins: new Set(), - cwd: dir, - env, - }); - expect(result.analysisOk).toBe(true); - expect(result.allowlistSatisfied).toBe(true); - expect(result.allowlistMatches.length).toBe(3); - } finally { - fs.rmSync(dir, { recursive: true, force: true }); + describe("shell wrapper inline compound allowlist", () => { + const commonShells = ["sh", "bash", "zsh", "dash", "ksh", "fish", "ash"] as const; + + function writeExecutable(filePath: string) { + fs.writeFileSync(filePath, "#!/bin/sh\n", { mode: 0o755 }); } - }); - it("satisfies allowlist for shell-wrapped compound command with safe bins", () => { - const dir = makeTempDir(); - const headPath = path.join(dir, "head"); - const tailPath = path.join(dir, "tail"); - const gogPath = path.join(dir, "gog-wrapper"); - const shPath = path.join(dir, "sh"); - fs.writeFileSync(headPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(tailPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(gogPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(shPath, "#!/bin/sh\n", { mode: 0o755 }); - const env = makePathEnv(dir); - try { - const result = evaluateShellAllowlist({ - command: `${shPath} -c "head -n 1 && tail -n 1 && gog-wrapper calendar events"`, - allowlist: [{ pattern: gogPath }], - safeBins: normalizeSafeBins(["head", "tail"]), - trustedSafeBinDirs: new Set([dir]), - cwd: dir, - env, - }); - expect(result.analysisOk).toBe(true); - // Safe bins are disabled on Windows + it.each(commonShells)("evaluates inner chain commands for %s -c wrappers", (shellBinary) => { if (process.platform === "win32") { - expect(result.allowlistSatisfied).toBe(false); return; } - expect(result.allowlistSatisfied).toBe(true); - } finally { - fs.rmSync(dir, { recursive: true, force: true }); - } - }); + const dir = makeTempDir(); + const shellPath = path.join(dir, shellBinary); + const catPath = path.join(dir, "cat"); + const printfPath = path.join(dir, "printf"); + const gogPath = path.join(dir, "gog-wrapper"); + writeExecutable(shellPath); + writeExecutable(catPath); + writeExecutable(printfPath); + writeExecutable(gogPath); + const env = makePathEnv(dir); + try { + const result = evaluateShellAllowlist({ + command: `${shellPath} -c "cat SKILL.md && printf '---CMD---' && gog-wrapper calendar events"`, + allowlist: [{ pattern: catPath }, { pattern: printfPath }, { pattern: gogPath }], + safeBins: new Set(), + cwd: dir, + env, + }); + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(true); + expect(result.allowlistMatches.length).toBe(3); + expect(result.segmentSatisfiedBy).toEqual(["allowlist"]); + expect(result.segmentAllowlistEntries).toEqual([null]); + expect(result.segmentSatisfiedBy.length).toBe(result.segments.length); + expect(result.segmentAllowlistEntries.length).toBe(result.segments.length); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); - it("rejects shell-wrapped compound command when an inner binary is not in allowlist", () => { - const dir = makeTempDir(); - const catPath = path.join(dir, "cat"); - const gogPath = path.join(dir, "gog-wrapper"); - const rmPath = path.join(dir, "rm"); - const shPath = path.join(dir, "sh"); - fs.writeFileSync(catPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(gogPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(rmPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(shPath, "#!/bin/sh\n", { mode: 0o755 }); - const env = makePathEnv(dir); - try { - const result = evaluateShellAllowlist({ - command: `${shPath} -c "cat SKILL.md && rm -rf / && gog-wrapper calendar events"`, - allowlist: [{ pattern: catPath }, { pattern: gogPath }], - safeBins: new Set(), - cwd: dir, - env, - }); - expect(result.analysisOk).toBe(true); - expect(result.allowlistSatisfied).toBe(false); - } finally { - fs.rmSync(dir, { recursive: true, force: true }); - } - }); + it("rejects wrapper chain when any inner command misses the allowlist", () => { + if (process.platform === "win32") { + return; + } + const dir = makeTempDir(); + const shellPath = path.join(dir, "sh"); + const catPath = path.join(dir, "cat"); + const rmPath = path.join(dir, "rm"); + const gogPath = path.join(dir, "gog-wrapper"); + writeExecutable(shellPath); + writeExecutable(catPath); + writeExecutable(rmPath); + writeExecutable(gogPath); + const env = makePathEnv(dir); + try { + const result = evaluateShellAllowlist({ + command: `${shellPath} -c "cat SKILL.md && rm -rf / && gog-wrapper calendar events"`, + allowlist: [{ pattern: catPath }, { pattern: gogPath }], + safeBins: new Set(), + cwd: dir, + env, + }); + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(false); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); - it("handles single command inside shell wrapper (no chain operators — not recursed)", () => { - const dir = makeTempDir(); - const gogPath = path.join(dir, "gog-wrapper"); - const shPath = path.join(dir, "sh"); - fs.writeFileSync(gogPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(shPath, "#!/bin/sh\n", { mode: 0o755 }); - const env = makePathEnv(dir); - try { - // Single-command shell wrappers are NOT recursively evaluated to preserve - // allow-always persisted-pattern security constraints. - const result = evaluateShellAllowlist({ - command: `${shPath} -c "gog-wrapper calendar events"`, - allowlist: [{ pattern: gogPath }], - safeBins: new Set(), - cwd: dir, - env, - }); - expect(result.analysisOk).toBe(true); - expect(result.allowlistSatisfied).toBe(true); - } finally { - fs.rmSync(dir, { recursive: true, force: true }); - } - }); - - it("handles shell wrapper without -c flag (script invocation) unchanged", () => { - const dir = makeTempDir(); - const shPath = path.join(dir, "sh"); - const scriptPath = path.join(dir, "myscript.sh"); - fs.writeFileSync(shPath, "#!/bin/sh\n", { mode: 0o755 }); - fs.writeFileSync(scriptPath, "#!/bin/sh\necho hello\n", { mode: 0o755 }); - const env = makePathEnv(dir); - try { - const result = evaluateShellAllowlist({ - command: `${shPath} ${scriptPath}`, - allowlist: [{ pattern: scriptPath }], - safeBins: new Set(), - cwd: dir, - env, - }); - expect(result.analysisOk).toBe(true); - expect(result.allowlistSatisfied).toBe(true); - } finally { - fs.rmSync(dir, { recursive: true, force: true }); - } + it("keeps single-command wrappers unchanged (no recursive allowlist lookup)", () => { + if (process.platform === "win32") { + return; + } + const dir = makeTempDir(); + const shellPath = path.join(dir, "sh"); + const gogPath = path.join(dir, "gog-wrapper"); + writeExecutable(shellPath); + writeExecutable(gogPath); + const env = makePathEnv(dir); + try { + const result = evaluateShellAllowlist({ + command: `${shellPath} -c "gog-wrapper calendar events"`, + allowlist: [{ pattern: gogPath }], + safeBins: new Set(), + cwd: dir, + env, + }); + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(false); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); }); }); });