fix(agents): fallback retry image safety and partial execution detection#57959
fix(agents): fallback retry image safety and partial execution detection#57959mitchmcalister wants to merge 6 commits intoopenclaw:mainfrom
Conversation
…n through model fallback
Greptile SummaryThis PR introduces failure-mode-aware image handling on fallback retries, cross-provider image privacy enforcement, prompt-image-detection suppression for cross-provider retries, and partial-execution context threading — all at the Key changes:
Issues found:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/failover-error.ts
Line: 50-55
Comment:
**`sanitizeToolNames` is never called in production code**
`sanitizeToolNames` is documented as "Sanitize tool names from model output before embedding in system prompts," but `buildPartialExecutionSystemContext` embeds `partialExecution.toolNames` directly into the system prompt string without ever calling this method. As-is, the sanitization layer added by this PR provides no actual protection.
If `partialExecution.toolNames` is ever populated from model-generated output (the docstring explicitly calls this out), unsanitized content would flow straight into the system prompt. The fix is to call `sanitizeToolNames` inside `buildPartialExecutionSystemContext` before building `toolList`:
```ts
// In buildPartialExecutionSystemContext:
const safeNames = FailoverError.sanitizeToolNames(partialExecution.toolNames);
if (safeNames.length === 0) return undefined;
const toolList = safeNames.join(", ");
```
Alternatively, apply it at the point where the `FailoverError` is constructed with `partialExecution`, but enforcing it at the point of use is harder to miss.
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/model-fallback.ts
Line: 819-829
Comment:
**Duplicate tool names when the same tool appears in multiple failed attempts**
The merge concatenates `toolNames` arrays without deduplication. If the same tool (e.g. `bash`) was executed in both the first and second failed attempts, the accumulated list will contain `"bash, bash, …"` and the resulting system prompt will say "already executed these tools: bash, bash, web_search." This is misleading and inflates the list toward the 20-entry cap that `sanitizeToolNames` enforces.
```suggestion
mergedPartialExecution = mergedPartialExecution
? {
toolNames: [
...new Set([
...mergedPartialExecution.toolNames,
...described.partialExecution.toolNames,
]),
],
didSendViaMessagingTool:
mergedPartialExecution.didSendViaMessagingTool ||
described.partialExecution.didSendViaMessagingTool,
}
: described.partialExecution;
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(agents): add suppressPromptImageDete..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static sanitizeToolNames(names: string[]): string[] { | ||
| return names | ||
| .map((n) => n.replace(/[^a-zA-Z0-9_-]/g, "").slice(0, 100)) | ||
| .filter(Boolean) | ||
| .slice(0, 20); | ||
| } |
There was a problem hiding this comment.
sanitizeToolNames is never called in production code
sanitizeToolNames is documented as "Sanitize tool names from model output before embedding in system prompts," but buildPartialExecutionSystemContext embeds partialExecution.toolNames directly into the system prompt string without ever calling this method. As-is, the sanitization layer added by this PR provides no actual protection.
If partialExecution.toolNames is ever populated from model-generated output (the docstring explicitly calls this out), unsanitized content would flow straight into the system prompt. The fix is to call sanitizeToolNames inside buildPartialExecutionSystemContext before building toolList:
// In buildPartialExecutionSystemContext:
const safeNames = FailoverError.sanitizeToolNames(partialExecution.toolNames);
if (safeNames.length === 0) return undefined;
const toolList = safeNames.join(", ");Alternatively, apply it at the point where the FailoverError is constructed with partialExecution, but enforcing it at the point of use is harder to miss.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/failover-error.ts
Line: 50-55
Comment:
**`sanitizeToolNames` is never called in production code**
`sanitizeToolNames` is documented as "Sanitize tool names from model output before embedding in system prompts," but `buildPartialExecutionSystemContext` embeds `partialExecution.toolNames` directly into the system prompt string without ever calling this method. As-is, the sanitization layer added by this PR provides no actual protection.
If `partialExecution.toolNames` is ever populated from model-generated output (the docstring explicitly calls this out), unsanitized content would flow straight into the system prompt. The fix is to call `sanitizeToolNames` inside `buildPartialExecutionSystemContext` before building `toolList`:
```ts
// In buildPartialExecutionSystemContext:
const safeNames = FailoverError.sanitizeToolNames(partialExecution.toolNames);
if (safeNames.length === 0) return undefined;
const toolList = safeNames.join(", ");
```
Alternatively, apply it at the point where the `FailoverError` is constructed with `partialExecution`, but enforcing it at the point of use is harder to miss.
How can I resolve this? If you propose a fix, please make it concise.| mergedPartialExecution = mergedPartialExecution | ||
| ? { | ||
| toolNames: [ | ||
| ...mergedPartialExecution.toolNames, | ||
| ...described.partialExecution.toolNames, | ||
| ], | ||
| didSendViaMessagingTool: | ||
| mergedPartialExecution.didSendViaMessagingTool || | ||
| described.partialExecution.didSendViaMessagingTool, | ||
| } | ||
| : described.partialExecution; |
There was a problem hiding this comment.
The merge concatenates toolNames arrays without deduplication. If the same tool (e.g. bash) was executed in both the first and second failed attempts, the accumulated list will contain "bash, bash, …" and the resulting system prompt will say "already executed these tools: bash, bash, web_search." This is misleading and inflates the list toward the 20-entry cap that sanitizeToolNames enforces.
| mergedPartialExecution = mergedPartialExecution | |
| ? { | |
| toolNames: [ | |
| ...mergedPartialExecution.toolNames, | |
| ...described.partialExecution.toolNames, | |
| ], | |
| didSendViaMessagingTool: | |
| mergedPartialExecution.didSendViaMessagingTool || | |
| described.partialExecution.didSendViaMessagingTool, | |
| } | |
| : described.partialExecution; | |
| mergedPartialExecution = mergedPartialExecution | |
| ? { | |
| toolNames: [ | |
| ...new Set([ | |
| ...mergedPartialExecution.toolNames, | |
| ...described.partialExecution.toolNames, | |
| ]), | |
| ], | |
| didSendViaMessagingTool: | |
| mergedPartialExecution.didSendViaMessagingTool || | |
| described.partialExecution.didSendViaMessagingTool, | |
| } | |
| : described.partialExecution; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-fallback.ts
Line: 819-829
Comment:
**Duplicate tool names when the same tool appears in multiple failed attempts**
The merge concatenates `toolNames` arrays without deduplication. If the same tool (e.g. `bash`) was executed in both the first and second failed attempts, the accumulated list will contain `"bash, bash, …"` and the resulting system prompt will say "already executed these tools: bash, bash, web_search." This is misleading and inflates the list toward the 20-entry cap that `sanitizeToolNames` enforces.
```suggestion
mergedPartialExecution = mergedPartialExecution
? {
toolNames: [
...new Set([
...mergedPartialExecution.toolNames,
...described.partialExecution.toolNames,
]),
],
didSendViaMessagingTool:
mergedPartialExecution.didSendViaMessagingTool ||
described.partialExecution.didSendViaMessagingTool,
}
: described.partialExecution;
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29cf1c59ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| // Accumulate failure context for the next candidate's run callback. | ||
| lastFailureReason = described.reason ?? "unknown"; | ||
| if (described.partialExecution) { |
There was a problem hiding this comment.
runWithModelFallback now merges described.partialExecution for later retries, but in the current non-test codebase no new FailoverError(...) call sets partialExecution (the throw sites in the embedded/CLI runners still omit it), so this branch never runs in production. That leaves previousPartialExecution unset on retries, meaning the new replay-avoidance system prompt is never emitted and non-idempotent tools can still be replayed after a failed attempt.
Useful? React with 👍 / 👎.
|
Addressed in c39c5cc: Greptile P1 (sanitizeToolNames not called): Fixed — Greptile P2 (duplicate tool names): Fixed — merge now uses Codex P1 (partialExecution never populated in production): Acknowledged — this is by design. The plumbing is ready for when the embedded/CLI runners start populating |
1 participant
Summary
resolveRetryImages— only strip onformaterrors (model can't handle images) or cross-provider retries (privacy boundary)suppressPromptImageDetectionto the embedded runner sodetectAndLoadPromptImagesis skipped on cross-provider retries — closes the gap where prompt-referenced local images bypassed the image strippreviousFailureReasonandpreviousPartialExecutionthroughModelFallbackRunOptionsso the retry callback knows what the prior attempt did; warn fallback models about already-executed tools to prevent replaying non-idempotent operationsBuilds on #55632 (prompt preservation for new sessions). Supersedes #44188. Closes #43481, #43492.
Design boundary
All changes stay at the
runAgentAttempt/runWithModelFallbackboundary. The pi-runner internals are not modified beyond accepting thesuppressPromptImageDetectionflag. This is intentional — the runner's internal loops (profile rotation, cooldown skips) are treated as opaque. See #44188 for the full design discussion.Changes
failover-error.tsPartialExecutiontype withtoolNamesanddidSendViaMessagingToolFailoverErrorcarries optionalpartialExecutionfieldsanitizeToolNamesstatic method (caps at 20 entries, strips non-alphanumeric, truncates to 100 chars)describeFailoverErrorreturnspartialExecutionwhen presentmodel-fallback.tsModelFallbackRunOptionsextended withpreviousFailureReasonandpreviousPartialExecutionrunOptionsfor next candidateattempt-execution.tsresolveRetryImages: failure-mode-aware image handling (format → strip, cross-provider → strip, same-provider non-format → preserve)buildPartialExecutionSystemContext: system prompt warning about already-executed tools (same-provider only, CWE-200)runAgentAttemptextended withprimaryProvider,previousFailureReason,previousPartialExecutionresolveRetryImagesandeffectiveExtraSystemPromptEmbedded runner (
params.ts,run.ts,attempt.ts)suppressPromptImageDetectionparam gatesdetectAndLoadPromptImagesagent-command.tsprimaryProvider,previousFailureReason,previousPartialExecutionfrom fallbackrunOptionsintorunAgentAttemptTest plan
resolveRetryImages— 6 tests: first attempt, format error, rate_limit, cross-provider, undefined provider, undefined imagesbuildPartialExecutionSystemContext— 3 tests: tool listing, messaging warning, empty toolsFailoverError.sanitizeToolNames— 3 tests: cap at 20, truncate to 100, filter emptyFailoverError.partialExecution— 2 tests: carries when provided, undefined when absentdescribeFailoverError— 2 tests: includes/omits partialExecutionrunWithModelFallback— 4 tests: threads previousFailureReason, threads previousPartialExecution, merges across attempts, omits when absentpnpm check— clean (0 warnings, 0 errors)pnpm build— clean (no type errors, no INEFFECTIVE_DYNAMIC_IMPORT)🤖 Generated with Claude Code