Greptile Summary

This PR consolidates three draft PRs into a focused change that hardens the model fallback retry path across three dimensions: cross-provider image privacy, partial execution detection, and system context injection.

Key changes:

• Image handling: Replaces the blunt "strip images on any fallback" policy with resolveRetryImages, which strips only on cross-provider retries (privacy) or same-provider format errors (modality). Adds suppressPromptImageDetection to also block prompt-referenced local images from being loaded and forwarded cross-provider.
• Partial execution tracking: FailoverError now carries a sanitized partialExecution field populated from attempt.toolMetas in the embedded runner. The fallback loop in runWithModelFallback preserves the earliest partial-execution record across multi-hop chains (lastPartialExecution ?? previous).
• System context injection: Partial-execution warnings are delivered via extraSystemPrompt rather than prepended to the user message, keeping the signal in the correct prompt layer and avoiding models treating [System: ...] markers as user text.
• Prompt forwarding fix: Drops the "Continue where you left off" prompt substitution — the original user message is now always re-sent on fallback, with the orphaned-user-message repair in attempt.ts handling any duplicate-turn cleanup.

Two minor style observations were flagged: (1) sanitizeToolNames applies the 20-name cap before filtering empty names, so the output can be fewer than 20 valid entries even when more valid names exist beyond index 19 — reversing the slice/filter order would fix this if the intent is "up to 20 valid names". (2) The isCrossProviderRetry guard (primaryProvider !== providerOverride) uses strict !== while resolveRetryImages guards with primaryProvider && currentProvider, creating a subtle asymmetry when primaryProvider is undefined that could leave explicit images unstripped while still suppressing prompt image detection.

Confidence Score: 4/5

• Safe to merge — the changes are well-tested and the logic is correct; two minor style/consistency notes do not affect production behavior.
• The core logic is sound: image stripping, partial-execution capture, and system-prompt injection all work as described. Test coverage is comprehensive (unit + integration across all three feature areas). The two flagged items are edge cases (undefined primaryProvider is not reachable in production; the sanitizeToolNames ordering only matters when tool names contain only special characters) that do not affect the happy path.
• src/agents/failover-error.ts (sanitizeToolNames slice/filter order) and src/commands/agent.ts (isCrossProviderRetry guard alignment with resolveRetryImages).

This is a comment left during a code review.
Path: src/agents/failover-error.ts
Line: 98-101

Comment:
**`slice` applied before `filter` may yield fewer than 20 valid names**

The 20-item limit is applied to the *input* array before invalid names are stripped. If any of the first 20 names sanitize to an empty string (e.g. `"!!!"` → `""`), the result will have fewer than 20 valid entries even when more valid names exist at indices ≥ 20.

The current test passes 25 all-valid names, so it doesn't expose this gap. If the intent is "return up to 20 sanitized names", the order should be reversed:

```suggestion
  static sanitizeToolNames(names: string[]): string[] {
    return names
      .map((n) => n.replace(/[^a-zA-Z0-9_-]/g, "").slice(0, 100))
      .filter(Boolean)
      .slice(0, 20);
  }
```

If the intent is instead "process only the first 20 tool executions regardless of validity", the current order is intentional — but a short comment would make that explicit.

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/commands/agent.ts
Line: 499-500

Comment:
**`isCrossProviderRetry` guard differs from `resolveRetryImages` guard**

`isCrossProviderRetry` uses strict `!==`, so it evaluates to `true` whenever `primaryProvider` is `undefined` and `providerOverride` is set (`undefined !== "openai"` → `true`). In that scenario `suppressPromptImageDetection` would be `true`, yet `resolveRetryImages` would *not* strip explicit images because its own guard requires both providers to be truthy:

```ts
// resolveRetryImages — strips only when both providers are known
if (primaryProvider && currentProvider && primaryProvider !== currentProvider) {
```

The outcome would be: explicit images forwarded (not stripped) but prompt-based image detection suppressed — an inconsistent mix. `primaryProvider` should always be defined in practice (it comes from the resolved model config), but aligning the guard to mirror `resolveRetryImages` would make the invariant explicit and avoid silent mismatches if the code path ever changes:

```suggestion
  const isCrossProviderRetry =
    params.isFallbackRetry &&
    !!params.primaryProvider &&
    params.primaryProvider !== params.providerOverride;
```

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

_{Last reviewed commit: 8491415}

Fixed in 350de99 — gated partialExecContext on same-provider retry using the existing isCrossProviderRetry check, matching the suggested fix exactly.

The isCrossProviderRetry computation was hoisted earlier in runAgentAttempt (from line 507 to line 393) so it's now shared by all three cross-provider safety gates: image stripping, prompt image detection suppression, and partial execution context suppression.

Cross-provider retries will now skip buildPartialExecutionSystemContext entirely, so no tool names or messaging status leak to a different LLM provider.

The remaining check CI failure is a pre-existing oxfmt formatting issue on main — 3 CSS files in ui/src/styles/ (grouped.css, layout.css, chat/layout.css). Confirmed the same check job fails on the latest main CI run (23017465466). Unrelated to this PR.

🔒 Aisle Security Analysis

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

lastPartialExecution = lastPartialExecution
  ? {
      ...lastPartialExecution,
      toolNames: [
        ...new Set([
          ...lastPartialExecution.toolNames,
          ...described.partialExecution.toolNames,
        ]),
      ],
      didSendViaMessagingTool:
        lastPartialExecution.didSendViaMessagingTool ||
        described.partialExecution.didSendViaMessagingTool,
    }
  : described.partialExecution;

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

const merged = [
  ...new Set([
    ...lastPartialExecution.toolNames,
    ...described.partialExecution.toolNames,
  ]),
];

lastPartialExecution = {
  ...lastPartialExecution,
  toolNames: FailoverError.sanitizeToolNames(merged),
  didSendViaMessagingTool:
    lastPartialExecution.didSendViaMessagingTool ||
    described.partialExecution.didSendViaMessagingTool,
};

const imageResult = params.suppressPromptImageDetection
  ? { images: params.images ?? [], detectedRefs: [], loadedCount: 0, skippedCount: 0 }
  : await detectAndLoadPromptImages({ ... maxBytes, maxDimensionPx ... });

if (imageResult.images.length > 0) {
  await abortable(activeSession.prompt(effectivePrompt, { images: imageResult.images }));
}

import { sanitizeImageBlocks } from "../../tool-images.js";

const imageSanitization = resolveImageSanitizationLimits(params.config);
const sanitizedExisting = params.images
  ? (await sanitizeImageBlocks(params.images, "prompt:images", {
      maxBytes: MAX_IMAGE_BYTES,
      maxDimensionPx: imageSanitization.maxDimensionPx,
    })).images
  : [];

const imageResult = params.suppressPromptImageDetection
  ? { images: sanitizedExisting, detectedRefs: [], loadedCount: 0, skippedCount: 0 }
  : await detectAndLoadPromptImages({
      ...,
      existingImages: params.images,
      maxBytes: MAX_IMAGE_BYTES,
      maxDimensionPx: imageSanitization.maxDimensionPx,
    });

Analyzed PR: #44188 at commit 4cf0a10

_{Last updated on: 2026-03-25T01:57:39Z}

Rebased onto current main — the upstream file move (src/commands/agent.ts → src/agents/agent-command.ts) is resolved and CI should be clean. No upstream changes have addressed the root causes (prompt/image loss on fallback retry, cross-provider image disclosure, or partial execution blindness).

Addressed in 4cf0a10:

#1 (High): Acknowledged — other runWithModelFallback call sites (e.g. followup-runner.ts) don't apply cross-provider guards. This is intentionally out of scope for this PR (see Non-goals), but worth a follow-up issue.

#2 (Medium): Fixed — suppressPromptImageDetection now also triggers on same-provider format retries, matching the resolveRetryImages strip behavior.

#3 (Low): Intentional trade-off. Same-provider = same trust boundary; tool names are already sanitized via FailoverError.sanitizeToolNames().

Re: Codex review on agent-command.ts:420 (CLI session duplicate turns) — pre-existing behavior. The old generic continuation message had the same duplicate-turn issue on CLI session retries. Orphan repair for the CLI runner path is a separate concern tracked in #54192.

Superseded by #57959 — narrower scope after #55632 landed prompt preservation for new sessions. The new PR focuses on the remaining image safety and partial execution detection layers. See #57959 for the full design discussion from #44188 carried forward.