terminal: drop sandbox output retry heuristics#312920
terminal: drop sandbox output retry heuristics#312920dileepyavan wants to merge 2 commits intomainfrom
Conversation
Remove sandbox-output heuristics from terminal unsandbox retry handling and only surface sandbox guidance for explicit command failures. Update the focused tests to cover the current analyzer and retry behavior instead of the removed helper. Refs #312718.
There was a problem hiding this comment.
Pull request overview
Removes remaining sandbox-output string heuristics from the terminal chat agent sandbox flow so retries and remediation guidance are driven by execution outcomes rather than parsing terminal text.
Changes:
- Update sandbox output analysis to only emit remediation guidance for explicit non-zero exit failures.
- Simplify automatic unsandbox retry gating by removing output-text inspection.
- Replace heuristic-focused unit tests with behavior-focused coverage for the analyzer and retry logic.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sandboxOutputAnalyzer.test.ts | Replaces heuristic tests with analyzer/remediation and retry behavior coverage. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sandboxOutputAnalyzer.ts | Drops “suspected failure” (output-text) path and only returns guidance on explicit failures. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts | Removes dependency on output-text heuristic for automatic unsandbox retry. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sandboxOutputAnalyzer.ts:45
outputLooksSandboxBlockedis still exported below, but it no longer has any call sites in the repo (it appears to be unused after removing the retry/analyzer heuristics). Consider removing the unused export (and its comment) to avoid keeping a deprecated heuristic around unintentionally and to better match the PR’s goal of dropping sandbox-output heuristics.
}
- Files reviewed: 3/3 changed files
- Comments generated: 2
| @@ -409,8 +409,7 @@ export function shouldAutomaticallyRetryUnsandboxed(options: IAutomaticUnsandbox | |||
| && !options.isPersistentSession | |||
| && !options.isBackgroundExecution | |||
| && !options.didTimeout | |||
There was a problem hiding this comment.
shouldAutomaticallyRetryUnsandboxed treats exitCode: undefined as a failure because undefined !== 0 is true, which can trigger an automatic unsandbox retry even when the exit code is simply unavailable. Consider only retrying on an explicit non-zero exit code (eg require exitCode !== undefined && exitCode !== 0) to avoid unnecessary confirmation prompts/retries when the strategy cannot determine an exit code.
| && !options.didTimeout | |
| && !options.didTimeout | |
| && options.exitCode !== undefined |
| const knownFailure = options.exitCode !== undefined && options.exitCode !== 0; | ||
| const suspectedFailure = !knownFailure && options.exitCode === undefined && this._outputLooksSandboxBlocked(options.exitResult); | ||
|
|
||
| if (!knownFailure && !suspectedFailure) { | ||
| if (!knownFailure) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
With the new early-return, knownFailure is effectively a guard ensuring the rest of analyze only runs for explicit non-zero exit codes. Since the remaining logic no longer needs to distinguish between “known” vs “suspected” failures, consider simplifying by inlining this guard (and removing any now-unreachable non-failure branches later in the method) to keep the control flow straightforward.
This issue also appears on line 45 of the same file.
Update the electron-browser automatic sandbox retry tests to reflect that retry decisions no longer depend on sandbox-output heuristics. Refs #312718.
Fixes #312718
Summary
This change removes the remaining sandbox-output text heuristics from the terminal chat agent sandbox flow.
shouldAutomaticallyRetryUnsandboxedonoutputLooksSandboxBlockedSandboxOutputAnalyzerfor explicit non-zero exit failuresTesting
npm run compile-check-ts-nativenode --experimental-strip-types build/hygiene.ts src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sandboxOutputAnalyzer.ts src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sandboxOutputAnalyzer.test.ts./scripts/test.sh --run src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sandboxOutputAnalyzer.test.ts