🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🔵 Tool execution metadata disclosure to fallback LLM providers via injected system prompt

Description

The fallback retry prompt now includes tool execution metadata (tool names + whether a messaging tool already sent messages) and injects it into the next model attempt’s prompt.

Data flow introduced by this diff:

• src/agents/pi-embedded-runner/run.ts captures executed tool names (attempt.toolMetas[].toolName) and sets FailoverError.partialExecution.
• src/agents/model-fallback.ts persists this as lastPartialExecution and forwards it to subsequent fallback attempts as runOptions.previousPartialExecution.
• src/commands/agent.ts interpolates previousPartialExecution.toolNames directly into a system prompt that is then sent to the fallback provider/model.

Why this is a problem:

• This sends internal execution details across trust boundaries to a potentially different third-party LLM provider during fallback.
• Even though pi-embedded-runner currently sanitizes names, FailoverError accepts arbitrary partialExecution and resolveFallbackRetryPrompt does not re-sanitize/escape. If any future caller constructs FailoverError with unsanitized toolNames, this becomes a prompt-injection primitive (newlines/markup) in addition to a disclosure.

Vulnerable code (prompt construction):

const toolList = params.previousPartialExecution.toolNames.join(", ");
...
`It completed these tool calls: ${toolList}.${messagingWarning} ` +

Recommendation

Minimize and harden what crosses the provider boundary.

• Default to redaction: avoid sending specific tool names to the fallback model; send only a boolean / count (and the didSendViaMessagingTool safety flag).
• Re-sanitize at the sink (resolveFallbackRetryPrompt) even if sanitized earlier, because FailoverError.partialExecution can be constructed elsewhere.

Example safer implementation:

import { FailoverError } from "../agents/failover-error.js";

function resolveFallbackRetryPrompt(params: {
  body: string;
  isFallbackRetry: boolean;
  previousPartialExecution?: { toolNames: string[]; didSendViaMessagingTool: boolean };
}): string {
  if (!params.isFallbackRetry) return params.body;
  if (!params.previousPartialExecution) {
    return "Continue where you left off. The previous model attempt failed or timed out.";
  }

  const safeToolNames = FailoverError.sanitizeToolNames(params.previousPartialExecution.toolNames);
  const toolCount = safeToolNames.length;
  const messagingWarning = params.previousPartialExecution.didSendViaMessagingTool
    ? " Messages were already sent to the user — do NOT re-send them."
    : "";

  return [
    `[System: The previous model attempt partially executed before failing. ` +
      `It executed ${toolCount} tool call(s).${messagingWarning} ` +
      `Do not repeat actions that have already been performed. ` +
      `Review the conversation history and continue from where the previous attempt left off.]`,
    params.body,
  ].join("\n\n");
}

Optionally gate inclusion of tool names behind an explicit debug flag/config that is off by default.

Analyzed PR: #43507 at commit f36dc8e

_{Last updated on: 2026-03-11T23:32:54Z}

Greptile Summary

This PR adds detection of partial tool execution before model fallback retries, threading sanitized tool names from FailoverError through model-fallback.ts into agent.ts where they are prepended as a [System: ...] context block — a meaningful safety improvement for non-idempotent operations like messaging and shell commands.

Two logic issues were found:

• Partial execution context lost on multi-hop fallback (model-fallback.ts:733): lastPartialExecution is unconditionally reassigned to described.partialExecution after each failed attempt. If a second fallback candidate fails without executing any tools, this overwrites lastPartialExecution with undefined, causing subsequent candidates to never learn about tools that the primary model already executed. Fix: use ?? lastPartialExecution to preserve earlier context.

• Empty toolNames produces a malformed system prompt (pi-embedded-runner/run.ts:1421): The guard attempt.toolMetas.length > 0 checks raw tool count but does not account for sanitizeToolNames reducing the list to []. This propagates to agent.ts where [].join(", ") produces an empty string, yielding the confusing prompt "It completed these tool calls: .". The sanitized names should be computed first and checked for length before constructing the partialExecution object.

Confidence Score: 3/5

• The core idea is sound, but two logic bugs can cause the fallback model to either lose partial execution context mid-chain or receive a malformed empty tool list in the system prompt.
• The sanitization approach and prompt injection prevention are well-designed. However, lastPartialExecution being unconditionally overwritten is a correctness bug in the scenario this PR is explicitly trying to fix (multi-model fallback chains with prior tool execution), and the empty-toolNames edge case produces a misleading prompt. Both should be addressed before merge.
• src/agents/model-fallback.ts (line 733) and src/agents/pi-embedded-runner/run.ts (lines 1421–1430) require fixes before merging.

This is a comment left during a code review.
Path: src/agents/model-fallback.ts
Line: 733

Comment:
**Partial execution context lost on multi-hop fallback failure**

`lastPartialExecution` is unconditionally overwritten here. If a secondary fallback attempt fails *without* executing any tools (e.g. it hits a rate limit immediately), `described.partialExecution` is `undefined`, which clobbers the context from the original attempt. Any further fallback candidates will then receive no `previousPartialExecution` — meaning they won't know that the primary model already sent a message or ran a shell command.

Example scenario: Attempt 1 runs `send_message` then times out → `lastPartialExecution = { toolNames: ["send_message"], ... }`. Attempt 2 hits a rate limit before executing anything → `lastPartialExecution = undefined`. Attempt 3 never learns about the message sent in Attempt 1 and may re-send it.

```suggestion
      lastPartialExecution = described.partialExecution ?? lastPartialExecution;
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run.ts
Line: 1421-1430

Comment:
**Empty `toolNames` after sanitization produces a malformed system prompt**

`attempt.toolMetas.length > 0` only guarantees that there were tool calls before sanitization. If every tool name consists solely of characters stripped by `sanitizeToolNames` (e.g. only emoji, CJK, or punctuation), the resulting `toolNames` array will be `[]`. This gets threaded all the way to `resolveFallbackRetryPrompt` in `agent.ts`, where `[].join(", ")` produces an empty string, resulting in the nonsensical fallback prompt:

> *"It completed these tool calls: . Do not repeat actions that have already been performed."*

This could confuse the fallback model. The `partialExecution` object should only be created when the sanitized names are non-empty:

```
const sanitizedToolNames = FailoverError.sanitizeToolNames(
  attempt.toolMetas.map((t) => t.toolName),
);
partialExecution:
  sanitizedToolNames.length > 0
    ? {
        hadToolExecution: true,
        toolNames: sanitizedToolNames,
        didSendViaMessagingTool: attempt.didSendViaMessagingTool,
      }
    : undefined,
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 399026c}

Converting to draft — the partial execution warning injection will be redesigned as part of a unified fallback retry approach that uses proper system context rather than user-turn injection.

See #43331 (comment) for context on the rethink.

Superseded by #44188 (unified fallback retry safety PR).