Fix Bash path interpretation for Windows terminal - #26125
Fix Bash path interpretation for Windows terminal#26125Akash (akashdeepmaity4) wants to merge 5 commits into
Conversation
Fixed path interpretation issues for Bash commands in Windows, ensuring proper command and argument formatting.
@microsoft-github-policy-service agree |
|
🔒 Automated review in progress — Bill Schnurr (@bschnurr) is auto-reviewing this PR. |
|
|
||
| // Ship the cleanly escaped string to the terminal stream! | ||
| this.terminal!.sendText(text, true); | ||
| } |
There was a problem hiding this comment.
Issue · Please address or respond
These duplicated statements are directly in the class body, reference method-local variables, and introduce an extra closing brace, so the file cannot compile. Remove this entire duplicated block.
| let safeArg = arg.replace(/\\/g, '/'); | ||
| safeArg = safeArg.replace(/^([A-Za-z]):/, (_, drive) => `/${drive.toLowerCase()}`); | ||
| return safeArg; | ||
| }); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Replacing backslashes in every argument treats opaque values such as regular expressions or Python snippets as paths and can corrupt them. Restrict conversion to known path operands or move normalization into an API that can identify paths.
There was a problem hiding this comment.
Updated to Strict Path Operand Normalization. Now, only true paths are considered and regexes are rejected. Up for review
|
Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. Result: Summary: No meaningful test executed because the verification container could not start. Static inspection found duplicate statements at class scope in `service.ts`, which makes the PR head syntactically invalid. No tests were added, and existing terminal tests do not cover the new Bash path conversion. Test runs: 1 not run
|
Removed deprecated logic for stitching command and arguments in terminal service.
|
|
||
| // Standard VS Code logic to stitch the command and arguments together | ||
| const text = processedArgs.reduce((p, c) => `${p} "${c}"`, processedCommand); | ||
|
|
Refactor command processing for Bash compatibility and restore command execution logic.
Key Changes Applied: Preserved Architecture Pipeline: Routes processedCommand and processedArgs back through this.terminalHelper.buildCommandForTerminal(...) and this.executeCommand(...) to retain shell-aware escaping, shell integration capabilities, and cross-platform handling. Strict Path Operand Normalization: Avoids naive path regexes like /^[A-Za-z]:\\|\\/ which corrupt CLI flags, regex patterns, or inline snippets (e.g., -c, --option, or Python code with \n). Converts only explicit file paths (e.g., C:\path\to\file or .\path\to\file). Cleaned Class Hierarchy: Removed all dangling/duplicate outer-scope statements to maintain clean TypeScript compilation.
Akash (akashdeepmaity4)
left a comment
There was a problem hiding this comment.
Cleaned Class Hierarchy
Strict Path Operand Normalization
Preserved Architecture Pipeline
Removed duplicacies
Removed/fixes orphaned brackets
| let safeArg = arg.replace(/\\/g, '/'); | ||
| safeArg = safeArg.replace(/^([A-Za-z]):/, (_, drive) => `/${drive.toLowerCase()}`); | ||
| return safeArg; | ||
| }); |
There was a problem hiding this comment.
Updated to Strict Path Operand Normalization. Now, only true paths are considered and regexes are rejected. Up for review
Fixed path interpretation issues for Bash commands in Windows, ensuring proper command and argument formatting. Check recent issues submitted by Akash (@akashdeepmaity4) to reproduce issue and get more details.