fix(core): normalize Windows PATH-like env keys for shell execution

[Sakuranda]

TLDR

This PR addresses a Windows command resolution edge case by normalizing PATH-like environment keys before shell spawn.

On Windows, both Path and PATH can coexist in process.env with different values. That ambiguity may cause spawned processes to resolve commands from an unintended key, which can lead to "not recognized" errors in extension-driven command execution (related to #1892).

This change:

• adds Windows-only PATH-key normalization
• keeps canonical Path and removes duplicate PATH-like keys
• applies to both PTY execution and child_process fallback
• adds focused tests for both execution paths

Dive Deeper

Problem

Issue #1892 reports that commands available in user environment (e.g., flutter) may fail when executed through the extension flow on Windows.

Root Cause

Windows treats env keys case-insensitively, but Node.js process.env can still contain multiple PATH-like keys (Path, PATH) as separate object keys.
If these keys diverge, child process resolution may use an unintended value, causing command lookup failure.

Fix

Implemented normalizePathEnvForWindows(env) in packages/core/src/services/shellExecutionService.ts:

• detect all PATH-like keys (key.toLowerCase() === 'path')
• keep canonical Path
• remove duplicate PATH-like keys
• preserve selected canonical value

Applied normalization before spawn in both:

• PTY execution path
• child_process fallback path

Validation Scope

• Added Windows-specific tests in packages/core/src/services/shellExecutionService.test.ts:
  • PTY path normalization assertion
  • child_process fallback normalization assertion
• Verified targeted test suite passes.

Risk / Compatibility

• Windows-only behavior change.
• No functional changes intended for Linux/macOS.
• Minimal blast radius: env normalization only, no command parsing/execution strategy changes.

Reviewer Test Plan

1. Run targeted tests:

npm run test --workspace packages/core -- src/services/shellExecutionService.test.ts --coverage.enabled false

Expected:

• test suite passes (e.g., 39 passed)
• includes new Windows assertions for both PTY and child_process fallback

Testing Matrix

	🍏	🪟	🐧
npm run	❓	✅	❓

Linked issues / bugs

Fixes #1892

Screenshots

1. Local reproduction output

repro_issue_1892_force_conflict.js (click to expand)

//repro_issue_1892_force_conflict.js
const { spawnSync } = require('child_process');
const fs = require('fs');
const os = require('os');
const path = require('path');

const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-1892-force-'));
const tool = path.join(tempDir, 'qwen1892check.bat');
fs.writeFileSync(tool, '@echo QWEN_1892_FORCE_OK\r\n');

function runCase(name, env) {
  const setOut = spawnSync('cmd.exe', ['/d', '/s', '/c', 'set path'], {
    env,
    encoding: 'utf8',
    windowsHide: true,
  });

  const runOut = spawnSync('cmd.exe', ['/d', '/s', '/c', 'qwen1892check'], {
    env,
    encoding: 'utf8',
    windowsHide: true,
  });

  console.log(`=== ${name} ===`);
  console.log('effective:', (setOut.stdout || '').split('\n')[0].trim());
  console.log('status:', runOut.status);
  console.log('stdout:', (runOut.stdout || '').trim() || '<empty>');
}

const base = { ...process.env };

console.log('has Path key:', Object.prototype.hasOwnProperty.call(base, 'Path'));
console.log('has PATH key:', Object.prototype.hasOwnProperty.call(base, 'PATH'));

const conflict = {
  ...base,
  PATH: 'C:\\Windows\\System32',
  Path: `${tempDir};C:\\Windows\\System32`,
};

const pathOnly = {
  ...base,
  PATH: `${tempDir};C:\\Windows\\System32`,
};

runCase('Conflict (PATH + Path)', conflict);
runCase('PATH only (control)', pathOnly);

2. Targeted test pass output:
   npm run test --workspace packages/core -- src/services/shellExecutionService.test.ts --coverage.enabled false

[wenshao]

Issues & Suggestions

1. Hardcoding Path as canonical may not always be correct

The function always normalizes to Path (title-case). However, Node.js on Windows typically uses PATH (all-caps) as the conventional key. The choice of Path seems arbitrary — Windows itself is case-insensitive, but downstream tools (e.g., cross-platform Node libraries) may expect PATH.

Consider using PATH as canonical instead, or at minimum document why Path was chosen.

2. Value merging is absent — silent data loss risk

When both Path and PATH exist with different values, the function silently discards one. A safer approach would be to merge/concatenate both values with
; as the delimiter:

const mergedValue = [...new Set(
pathKeys.flatMap(k => (normalized[k] ?? '').split(';'))
)].join(';');

This avoids silently dropping directories from the user's PATH. The current behavior could cause the exact same "command not found" problem it's trying to fix, just with the other key's directories missing.

3. Unnecessary as keyof NodeJS.ProcessEnv cast

normalized['Path'] ?? normalized[pathKeys[0] as keyof NodeJS.ProcessEnv];

pathKeys[0] is already a string, and NodeJS.ProcessEnv uses string index signatures. The cast is unnecessary.

4. Normalization runs on every execution

normalizePathEnvForWindows(process.env) is called on every shell spawn, even though process.env rarely changes at runtime. This is cheap but could be cached if performance matters. Not a blocker.

5. Test duplication

The two test cases (PTY and child_process) are nearly identical — 28 lines duplicated. Consider extracting a shared helper:

function setupConflictingPathEnv() { ... }
function restorePathEnv(original: { Path?: string; PATH?: string }) { ... }

6. Tests mutate process.env directly

While the try/finally cleanup is correct, mutating process.env in tests is fragile. If mockPlatform or other mocks throw before the try block, environment state leaks. Consider using vi.stubEnv() if available in the test framework, which handles cleanup automatically.

[Sakuranda]

Thanks for the detailed review. I pushed follow-up changes to address the points you raised.

• Switched the canonical Windows key to a single PATH.
• Merged/de-duplicated entries from duplicate PATH-like keys instead of dropping one value.
• Removed the unnecessary cast.
• Added a small cache for the merged Windows PATH snapshot so we don’t recompute it on every spawn.
• Extracted shared test helpers to reduce duplication.
• Moved test env setup/restore into shared cleanup instead of per-test try/finally.

The targeted shellExecutionService test file still passes locally (39/39).

[DennisYu07]

LGTM！