[1125] feature: unified-shell-resolution (2/4) - #44
Open
myk1yt wants to merge 40 commits into
Open
Conversation
Patch coverage checks were blocking 10+ PRs with 80%/70% thresholds. Changed to informational: true so patch coverage is reported but not a required status check.
Merge feature/unified-shell-resolution into pr/b04-shell-contracts-v2. Combines B04's command_output ask delay with B05's shell resolution system (ShellResolver, ShellInvocationAdapter, TerminalProfileResolver, CommandEnvironmentService, CommandScheduler). Conflict resolution in ExecuteCommandTool.ts: - Kept B05 ShellFallbackMismatchError + enhanced getTerminalProviderForExecution - Kept B04 COMMAND_OUTPUT_ASK_DELAY_MS + command_output ask delay logic - Merged onShellExecutionStarted signature (process param from B04 + traceBuilder from B05) - Combined commandStartedAt fallback with ExecaTerminal shell invocation plan Conflict resolution in executeCommandTool.spec.ts: - Kept both B04 command_output ask policy tests and B05 cwd parameter validation tests Note: no-explicit-any lint errors are pre-existing in feature/unified-shell-resolution
…s for new test files, update counts for modified files
…onmentService - fixes e2e terminal-profile test where no VS Code terminal was created because provider was hardcoded to execa
- reserveTerminal: guard integration-ready self-transition when reusing a
terminal already in integration-ready state (fixes IllegalTransitionError
in e2e shell-race tests; the "404 No fixture matched" OpenRouter errors
were a downstream symptom).
- classifyShellFamily: use separator-agnostic basename instead of
path.basename so Windows paths classify correctly on POSIX hosts
(fixes ubuntu getProfileShell("win32") returning undefined for Git Bash).
- ExecaTerminal.runCommand: transition from creating/idle to fallback-ready
so setActiveStream's -> running transition is legal for directly
constructed terminals (fixes ubuntu ExecaTerminal onLine not firing).
- TerminalRegistry: replace two as-any casts with proper types
(removes no-explicit-any lint errors without touching suppressions).
…ode-sync cachedState reset - Terminal.ts: When resolvedEnv is present, also check Terminal.getProfileShell() for shellArgs and pass them to vscode.window.createTerminal(). This fixes the e2e-mock terminal-profile test where creationOptions.shellArgs was missing --noprofile/--norc from the configured Bash profile. - SettingsView.tsx: Re-apply mode-based cachedState sync from ac0ed1b that was reverted by a68ac23 (B05 merge). The useEffect now resets cachedState when either currentApiConfigName OR mode changes, fixing platform-unit-test failures on both ubuntu and windows.
… os-name in shell-env prompt spec - Terminal.ts waitForShellIntegration: skip integration-ready/integration-pending transitions when already in integration-ready/fallback-ready. Reused VS Code terminals promoted by the registry fire the readiness path while already in integration-ready, causing IllegalTransitionError (integration-ready → integration-ready) and 6 e2e-mock failures (long-running-silent-command, terminal-reuse-shell-race, zero-chunk-shell-race). - shell-environment-prompt.spec.ts: mock os-name to avoid spawning PowerShell per test. Under coverage instrumentation on windows-latest this exceeded the 20s test timeout (8 getSystemInfoSection failures). Matches all sibling prompt specs.
…d env resolution Task.resolveCommandEnvironment() only read terminalProfile from persisted provider state, ignoring programmatic overrides set via api.setTerminalProfile(). This caused the ShellResolver to resolve the default shell instead of the profile override, leading to e2e test timeout in terminal-profile.test.ts. Fix: fall back to Terminal.getTerminalProfile() when state.terminalProfile is undefined, and invalidate the CommandEnvironmentService cache in api.setTerminalProfile() so the next task re-resolves with the new profile.
…rminalProfile The mock sidebarProvider in unit tests may not have getCommandEnvironmentService. Use ?.() optional call syntax to tolerate missing method.
… tests The profile-override test flaked in CI (run 30752014262): the custom --noprofile/--norc bash terminal did not emit the OSC 633;A shell-integration marker within the default 5s window on a loaded runner, aborting with SI_ACTIVATION_TIMEOUT and hitting the 90s waitUntilCompleted budget. Set terminalShellIntegrationTimeout to 30s in both Terminal Profile task configurations so shell integration has time to activate.
…al-profile e2e Root cause of persistent Terminal Profile e2e flake (runs 30752014262, 30760530287): the previous fix set terminalShellIntegrationTimeout via the per-task startNewTask configuration, but that settings key is only applied through the webview config-applier (ClineProvider). The extension-host API setConfiguration path (contextProxy.setValues) never reaches Terminal.setShellIntegrationTimeout, so the activation window stayed at the default 5s and the --noprofile/--norc bash profile terminal aborted with SI_ACTIVATION_TIMEOUT on loaded CI runners (terminal create -> abort exactly 5.000s). - Add API.setShellIntegrationTimeout(timeoutMs) that updates the Terminal static immediately, and declare it on the RooCodeAPI interface. - terminal-profile.test.ts now calls setShellIntegrationTimeout(30_000) in suiteSetup (restored to 5_000 in suiteTeardown) and drops the ineffective per-task config keys.
The --noprofile/--norc bash profile depends on VS Code injecting shell integration via the shell startup path. On loaded CI runners that injection intermittently exceeds even a 30s activation window (run 30761508190: terminal created 18:40:49.05, abort 18:41:19.05 = exactly 30s, SI never fired). Each mocha retry runs the test against a freshly created terminal, which typically lets SI activate. Matches the retries:3 pattern already used by apply-diff.
…ARCH-TERMINAL-002) Remove --norc from the terminal-profile E2E test so VS Code can inject shell integration through the Bash startup path. --norc disables .bashrc reading, which makes shell integration physically impossible. - Change profile args from --noprofile --norc to --noprofile - Remove Mocha retries (the failure was deterministic, not flaky) - Remove 30s shell-integration timeout override (test-only API) - Remove setShellIntegrationTimeout from RooCodeAPI and extension facade Split the single contradictory assertion into two contracts: 1. Compatible profile: proves profile selection + shell integration works 2. Incompatible profile (--norc): will prove typed Execa fallback (B07) Refs: ARCH-TERMINAL-002
--noprofile also blocks VS Code's bash shell integration injection (just like --norc). Use --login instead, which is safe for shell integration while still proving custom profile args pass-through.
- Rename 'should reject profile with empty profileName' test to match its .not.toThrow() assertion (z.string() accepts empty strings; validation is the extension host's responsibility) - Replace queryByTestId + if-guard with getByTestId in TerminalSettings.shell.spec.tsx so the profile-selection test fails when the option is missing instead of silently passing - Translate terminal.inlineShell keys in all 17 non-English locales (previously English copies)
…ronment (PR Zoo-Code-Org#1125) CI failures: 1. Code QA Roo Code run 31226119026 (platform-unit-test): 6 unhandled EnvironmentTeardownError rejections from core/task/__tests__/Task.throttle.test.ts. The PR's new resolveCommandEnvironment() awaited the mocked provider.getState(); the continuation's console.error fired after test teardown, tripping vitest's 'Closing rpc while onUserConsoleLog was pending'. Swallow the best-effort resolution error instead of logging (legacy getShell() fallback still applies). 2. E2E Tests (Mocked) run 31226119032: shell-resolution.test.ts startNewTask calls had no aimock fixture ('404 No fixture matched'). Fix: - Add fixtures/shell-resolution.json (initial write_to_file tool call per marker) and src/fixtures/shell-resolution.ts (attempt_completion once the tool result lands), registered in runTest.ts. Runs: https://github.com/Zoo-Code-Org/Zoo-Code/actions/runs/31226119026 https://github.com/Zoo-Code-Org/Zoo-Code/actions/runs/31226119032
…for aimock fixture compatibility
…ent aimock 404 retry loops
…ell-resolution aimock tests
added 10 commits
August 8, 2026 20:01
The write_to_file fixture needs write approval to proceed. Without it, the approval ask is never answered in headless e2e, causing waitUntilCompleted to spin to timeout.
- executeCommandTool: +16 tests (ShellFallbackMismatch, legacy provider, unterminated-quote, terminal callbacks, timeout abort, safe-fallback) - TerminalRegistry: +24 tests (getTerminals, closeTerminal, execa reuse, recoverStaleTerminal, prepareProviderSwitch errors, double-init) - ClineProvider: +13 tests (getCommandEnvironmentService, handleRequest/- SetTerminalShellSelection, buildTerminalShellOptions, resolveWebviewView)
…Org#1125 - Delete obsolete JSON fixture apps/vscode-e2e/fixtures/shell-resolution.json - Rewrite TS fixture with predicate-based matching to exclude tool-result turns (prevents infinite re-match loop on Turn 2) - Fix TS2554 in executeCommandTool.spec.ts (add missing arg to 10 calls) - Fix TS2345/TS2339 in TerminalRegistry.spec.ts (ShellFamily casts, terminal prop) - Update eslint-suppressions.json to match new line counts CI run 31270258348/31270258438 were from merge commit 99692d0 that included upstream/main but excluded these test fixes.
…atching The unscoped 'any tool result' wildcard fixture was registered before the DeepSeek Turn-2 fixtures. aimock breaks ties between plain fallback fixtures by registration order, so the wildcard stole DeepSeek Turn-2 requests and served a generic 'Task completed via fallback fixture' instead of the expected DEEPSEEK_V4_MARKER_* (4 failures). The shell-resolution Turn-1 predicate only inspected the LAST user message for the marker tag. The extension appends an <environment_details> user message after the task text, so the tag was missed and Turn 1 never matched, leaving marker files unwritten and tasks hanging (5 failures). - Scope the wildcard fallback to requests containing a shell-resolution marker tag so it can no longer steal other suites' Turn-2 requests. - Match the Turn-1 tag anywhere in the request (whole-request JSON match, the proven subtasks.ts pattern) instead of only the last user message.
Three root causes fixed: 1. Shell-resolution fixture path mismatch: write_to_file wrote to workspace root (.txt) but test reads from shell-resolution-e2e/ subdirectory. Fixed path to shell-resolution-e2e/.txt. 2. Shell-resolution fallback stealing DeepSeek Turn-2 requests: the scoped fallback matched any request containing shell-resolution marker tags, including DeepSeek requests where the workspace directory listing contained those filenames. Added model guard (gpt-4.1) so only OpenRouter requests match the fallback. 3. Missing DeepSeek Turn-1 fixtures: only Turn-2 fixtures existed, causing 404 No fixture matched on Turn 1. Added Turn-1 fixtures that return read_file tool calls with correct file paths and probeTag-scoped matching.
requestContainsTag() was searching the entire serialized request (JSON.stringify(req)) for marker tags. This caused cross-test fixture contamination: after Test 1 created shell-resolution-override-ok.txt, the <environment_details> block in subsequent tests' requests listed that filename, causing Test 1's fixture to steal Tests 2-5's requests. Fix: requestContainsTag now inspects each message individually and skips <environment_details> user messages. Tool-result messages are still searched (Turn 2 matching needs the tag in the file path). Root cause: CI log showed Tests 2-5 completing in ~2s without creating marker files or surfacing errors — the wrong fixture returned attempt_completion for the wrong marker tag.
…ent part
The extension appends <environment_details> as a content part WITHIN the
last user message, not as a separate message. The old requestContainsTag
function concatenated all text parts into one string, then checked
startsWith('<environment_details>') on the combined text. Since the
combined text starts with the task text (not '<environment_details>'),
the skip logic never triggered. Marker filenames from prior tests
appearing in the <environment_details> workspace listing caused the
wrong fixture to match — Test 2's request matched Test 1's override
fixture because 'shell-resolution-override-ok' appeared in the
directory listing.
Fix: check EACH content part individually. Skip parts whose text starts
with '<environment_details>', search all other parts for the tag. This
prevents cross-test fixture contamination while still matching the
correct tag in the task text portion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack Position
feature/unified-shell-resolutionDescription
https://youtube.com/shorts/-cm4pnaoXD0
Full Feature Description
feature/unified-shell-resolutionterminal.ts,global-settings.ts,vscode-extension-host.ts, the settings UITerminalSettings.tsxandSettingsView.tsx, the backend terminal layersrc/integrations/terminal, and the task/tool/API wiringTask.ts,ExecuteCommandTool.ts,api.ts.SettingsView.tsxbind tocachedState, not live extension state.terminal-profile.test.ts. Manually run the same command in default, PowerShell, Command Prompt, and where available WSL/POSIX profiles, comparing the selected executable, output, exit code, cancellation, and cleanup.Why Split Into 17 PRs
Instead of submitting this feature as a single unified PR, it was split into individual PRs because as code size grows, safely reviewing a PR becomes very difficult. The feature was broken into mutually exclusive individual PRs so that each can be reviewed independently.
What This PR Specifically Changes
Adds CLI/user/legacy/VS Code/OS/fallback priority resolver, platform shell classification, typed result/error, executable and safe argument array. Does not include scheduler, registry, or task wiring.
Included Files
src/integrations/terminal/shell/ShellResolver.tssrc/integrations/terminal/shell/ShellInvocationAdapter.tssrc/integrations/terminal/shell/TerminalProfileResolver.tssrc/utils/shell.tsExclusion Scope
src/integrations/terminal/CommandScheduler.tssrc/integrations/terminal/TerminalLifecycle.tssrc/integrations/terminal/TerminalRegistry.tssrc/integrations/terminal/CommandTrace.tsSummary by CodeRabbit
Upstream PR: Zoo-Code-Org#1125