Skip to content

feature: unified-shell-resolution (3/4) - #37

Closed
myk1yt wants to merge 31 commits into
pr/b05-shell-resolution-v2from
pr/b06-terminal-lifecycle-v2
Closed

feature: unified-shell-resolution (3/4)#37
myk1yt wants to merge 31 commits into
pr/b05-shell-resolution-v2from
pr/b06-terminal-lifecycle-v2

Conversation

@myk1yt

@myk1yt myk1yt commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Stack Position

Description

Full Feature Description

  • Feature Branch: feature/unified-shell-resolution
  • Feature Name: Unified Shell Resolution
  • Purpose: Resolves the problem where shell selection, profile interpretation, argument assembly, and terminal reuse differ across command execution paths. Unifies the priority among user-configured shell, VS Code default profile, OS default, and safe fallback into a single typed resolution pipeline. This ensures that the same user settings produce a predictable execution environment across Windows Command Prompt, PowerShell, WSL, and macOS/Linux POSIX shells, reducing cases where the entire task fails in unclear ways due to misconfiguration.
  • Full Change Description: B04 defines the shared shell settings types and the UI using local cached state before saving. B05 resolves settings and platform information into an executable, shell family, source, and argument array, preserving argument boundaries instead of string concatenation. B06 manages command queue, terminal lifecycle, registry, reuse, trace, cancellation, and disposal. B07 connects the resolver and lifecycle to the task, command tool, extension API, and webview message paths.
  • Impact Scope: Affects the shared contracts terminal.ts, global-settings.ts, vscode-extension-host.ts, the settings UI TerminalSettings.tsx and SettingsView.tsx, the backend terminal layer src/integrations/terminal, and the task/tool/API wiring Task.ts, ExecuteCommandTool.ts, api.ts.
  • Errors and Edge Cases: If an explicit user override is invalid, returns a typed rejectable error. If an automatic candidate is invalid, proceeds to the next candidate. Timeout, user cancellation, non-zero exit, and terminal disposal are kept as distinct outcomes. Shell path and command arguments are never combined into a single unescaped string. Inputs in SettingsView.tsx bind to cachedState, not live extension state.
  • Testing Method: Run B04's contract and settings component tests, B05's Windows/POSIX/WSL resolution and invocation tests, B06's queue/reuse/cancellation/disposal tests, B07's task/tool/message tests and 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 command queue, lifecycle state, registry/reuse, execution trace, cancellation, timeout, and disposal. Moves this implementation currently mixed into B05 to this stage and removes CI-only churn.

Included Files

  • src/integrations/terminal/CommandScheduler.ts
  • src/integrations/terminal/TerminalLifecycle.ts
  • src/integrations/terminal/TerminalRegistry.ts
  • src/integrations/terminal/CommandTrace.ts
  • Direct tests for process/reuse/cancellation/disposal

Exclusion Scope

  • Resolver and invocation primitives
  • B07 task/provider/extension wiring
  • Unrelated CI scaffolding and dependency churn
  • All items in the common removal rules

Zoo (VP) added 8 commits August 2, 2026 07:43
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).
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from 5ecf76b to b89fa25 Compare August 2, 2026 09:23
…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.
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from c085865 to 13fdbae Compare August 2, 2026 10:04
Zoo (VP) added 2 commits August 2, 2026 22:37
… 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.
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from 13fdbae to b5e9f20 Compare August 2, 2026 14:14
…rminalProfile

The mock sidebarProvider in unit tests may not have getCommandEnvironmentService.
Use ?.() optional call syntax to tolerate missing method.
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from b5e9f20 to 7c726b2 Compare August 2, 2026 14:24
… 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.
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from 7c726b2 to 151cb30 Compare August 2, 2026 18:25
…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.
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from 151cb30 to a3c4f5b Compare August 2, 2026 18:38
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.
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from a3c4f5b to 4534518 Compare August 2, 2026 18:50
…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
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from 4534518 to fe0ed18 Compare August 2, 2026 19:43
--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.
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from fe0ed18 to 7bb69de Compare August 2, 2026 20:13
@myk1yt myk1yt changed the title B06: Terminal Lifecycle v2 feature: unified-shell-resolution (3/4) Aug 3, 2026
Zoo (VP) added 2 commits August 4, 2026 04:38
The shell dropdown's onShellSelectionChange only updated the pending
selection state; the Save button stayed disabled unless the unrelated
onTerminalProfilePickerOpened hook happened to fire. Wrap the handler so
a shell selection change explicitly calls setChangeDetected(true),
enabling Save on shell-only changes. Behavior is otherwise identical.
@myk1yt
myk1yt force-pushed the pr/b05-shell-resolution-v2 branch from 02f48bc to d180b31 Compare August 4, 2026 11:38
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from 1e0f087 to 4fe1300 Compare August 4, 2026 11:40
@myk1yt
myk1yt force-pushed the pr/b05-shell-resolution-v2 branch from d180b31 to 889f969 Compare August 4, 2026 20:28
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from 4fe1300 to 81d56ef Compare August 4, 2026 20:28
@myk1yt
myk1yt force-pushed the pr/b05-shell-resolution-v2 branch 5 times, most recently from 4a54ceb to 4f7a590 Compare August 6, 2026 04:37
@myk1yt
myk1yt force-pushed the pr/b06-terminal-lifecycle-v2 branch from 81d56ef to bf2d780 Compare August 6, 2026 04:42
@myk1yt
myk1yt deleted the branch pr/b05-shell-resolution-v2 August 7, 2026 13:05
@myk1yt myk1yt closed this Aug 7, 2026
@myk1yt
myk1yt deleted the pr/b06-terminal-lifecycle-v2 branch August 7, 2026 13:05
@myk1yt
myk1yt restored the pr/b06-terminal-lifecycle-v2 branch August 7, 2026 13:27
@myk1yt myk1yt reopened this Aug 8, 2026
Zoo (VP) added 2 commits August 8, 2026 14:52
…oo-Code-Org#1135)

CI failure: E2E Tests (Mocked) run 31226547742 timed out on both terminal
lifecycle tests. The fixture matching 'call_terminal_lifecycle_001' kept
re-issuing 'echo lifecycle-second' because the conversation history retains
the first tool result even after the second command completes, so predicate 1
still matched. Guard it by requiring that no tool result for
call_terminal_lifecycle_002 exists yet.
Run: https://github.com/Zoo-Code-Org/Zoo-Code/actions/runs/31226547742
@myk1yt

myk1yt commented Aug 10, 2026

Copy link
Copy Markdown
Owner Author

Closing to recreate with main as target base branch. This PR had stale base branch references after fork sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant