iframe-proxy

Viraj Mishra (VirajMishra1) · 2026-09-03T14:18:50Z

Fixes two small bugs in base utilities.

jsonEdit.ts: removing the last array element left a stray character

When removing the last element of an array, the deletion length was computed as parentEndOffset - 2 - offset. parentEndOffset is one past the closing ], so the correct length is parentEndOffset - 1 - offset. The extra - 1 stopped the edit one character short. In a compact array like [1,2] that is easy to hit, because there is no whitespace between the last element and the ] to hide it.

Added a regression test that removes index 1 from [1,2].

uri.ts: userinfo with a colon in the password

_asFormatted split userinfo on the last colon (lastIndexOf), so a password containing : was cut in the wrong place. It now splits on the first colon, since the user name can't contain one. The password is also now encoded with isAuthority = false, so a : in it is percent-encoded and can't be mistaken for the separator.

I haven't added a test for this one yet.

- extpath.ts: isRootOrDriveLetter checked path.length instead of pathNormalized.length, causing C:\\ (len 4) to be rejected even though it normalizes to the valid root C:\ (len 3) - jsonEdit.ts: last-array-element removal used parentEndOffset - 2 instead of - 1, leaving a stale character before the closing ] - terminalEnvironment.ts: escapeNonWindowsPath singleQuotes branch for bash/sh/zsh/git-bash used plain '...' quoting, which cannot contain escaped single quotes; fixed to $'...' (ANSI-C quoting) matching the bothQuotes branch - terminalEnvironment.ts: collapseTildePath used String.includes() instead of startsWith(), allowing a path like /home/virajfoo/x to incorrectly collapse when userHome is /home/viraj - uri.ts: password component in userinfo was encoded with isAuthority=true, leaving ':' unencoded in passwords; fixed to false

vs-code-engineering · 2026-09-03T14:21:13Z

Copilot

🟡 Changes recommended

The changes regress bare drive detection, mishandle POSIX sh, and do not fully encode passwords containing multiple colons.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes correctness issues in shared path, URI, JSON editing, and terminal utilities.

Changes:

Corrects normalized path and tilde-boundary checks.
Adjusts URI password encoding and JSON array removal spans.
Revises shell quoting for embedded single quotes.

File summaries

File	Description
`src/vs/base/common/extpath.ts`	Uses normalized lengths for root detection.
`src/vs/base/common/jsonEdit.ts`	Corrects last-array-element deletion range.
`src/vs/base/common/uri.ts`	Changes password-component encoding.
`src/vs/platform/terminal/common/terminalEnvironment.ts`	Updates shell quoting and home-path matching.

Review details

Files reviewed: 4/4 changed files
Comments generated: 5
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…, POSIX sh quoting

Viraj Mishra (VirajMishra1) · 2026-09-03T16:02:08Z

@microsoft-github-policy-service agree

…dePath collisions

Martin Aeschlimann (aeschli)

Please create a separate PR for the terminal issue.

Viraj Mishra (VirajMishra1) · 2026-09-07T22:59:34Z

Done, moved the terminal changes to #334988

Martin Aeschlimann (aeschli) · 2026-09-09T09:54:03Z

Copilot please review

The early-return guard also needs path.length > 3, not pathNormalized.length > 3. normalize('D:') can return 'D:.' (3 chars), making pathNormalized.length === 3 while the original path is only 2 chars — the guard must use the original path length.

Copilot

🟡 Changes recommended

Add URI password-colon regression coverage and include or remove the unimplemented path and terminal fixes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/vs/base/common/uri.ts:685

Please add a regression case for a password containing a colon, for example an authority like user:pa:ss@host expecting user:pa%3Ass@host. The existing user-info test only has one colon, so it does not verify that indexOf selects the user/password delimiter or that the password-specific encoding change preserves the remaining colon correctly.

			} else {
				// <user>:<pass>@<auth>
				res += encoder(userinfo.substr(0, idx), false, false);
				res += ':';
				res += encoder(userinfo.substr(idx + 1), false, false);

src/vs/base/common/uri.ts:678

The PR description claims fixes in extpath.ts and terminalEnvironment.ts, but neither file is changed in this patch, so those reported bugs remain: isRootOrDriveLetter still uses the unnormalized length (extpath.ts:323,328), collapseTildePath still uses an unbounded includes match (terminalEnvironment.ts:99), and the single-quote branches still emit invalid POSIX quoting (terminalEnvironment.ts:38,62). Please include the corresponding implementation and regression tests, or remove those fixes from the PR description.

			idx = userinfo.indexOf(':');

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

 				const offset = previous.offset + previous.length;
 				const parentEndOffset = parent.offset + parent.length;
-				edit = { offset, length: parentEndOffset - 2 - offset, content: '' };
+				edit = { offset, length: parentEndOffset - 1 - offset, content: '' };


The formatter expands the remaining array, so removing index 1 from [1,2] gives '[\n 1\n]', not '[1]'. This fixes the failing JSON - edits test.

Copilot AI balanced review requested due to automatic review settings September 3, 2026 14:18

Copilot started reviewing on behalf of Viraj Mishra (VirajMishra1) September 3, 2026 14:19

View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

fix: address Copilot review - drive letter length, URI userinfo split…

29cf0c1

…, POSIX sh quoting

vs-code-engineering Bot assigned Martin Aeschlimann (aeschli) Sep 3, 2026

test: add regression tests for jsonEdit compact array and collapseTil…

bbb6a67

…dePath collisions

Martin Aeschlimann (aeschli) requested changes Sep 7, 2026

View reviewed changes

Viraj Mishra (VirajMishra1) added 2 commits September 7, 2026 18:48

fix: use template literals to satisfy no-unexternalized-strings lint

32cf9d4

revert: move terminal changes to separate PR per maintainer request

b99b431

Viraj Mishra (VirajMishra1) requested a review from Martin Aeschlimann (aeschli) September 7, 2026 23:37

Martin Aeschlimann (aeschli) requested a balanced review from Copilot September 10, 2026 08:01

Copilot started reviewing on behalf of Martin Aeschlimann (aeschli) September 10, 2026 08:01

View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread src/vs/base/common/jsonEdit.ts

const offset = previous.offset + previous.length;

const parentEndOffset = parent.offset + parent.length;

edit = { offset, length: parentEndOffset - 2 - offset, content: '' };

edit = { offset, length: parentEndOffset - 1 - offset, content: '' };

Viraj Mishra (VirajMishra1) added 2 commits October 4, 2026 16:42

test: fix expected output for compact array last-item removal

2d0a877

The formatter expands the remaining array, so removing index 1 from [1,2] gives '[\n 1\n]', not '[1]'. This fixes the failing JSON - edits test.

test: cover colon in URI password

f7be48b

Viraj Mishra (VirajMishra1) force-pushed the fix-path-uri-util-bugs branch from 54920a6 to f7be48b Compare October 4, 2026 20:50

Martin Aeschlimann (aeschli) approved these changes Oct 5, 2026

View reviewed changes

roblourens approved these changes Oct 5, 2026

View reviewed changes

Dmitriy Vasyura (dmitrivMS) approved these changes Oct 5, 2026

View reviewed changes

Ben Villalobos (benvillalobos) approved these changes Oct 5, 2026

View reviewed changes

Martin Aeschlimann (aeschli) merged commit 922ce6a into microsoft:main Oct 15, 2026
35 checks passed

vs-code-engineering Bot added this to the 1.143.0 milestone Oct 15, 2026

This was referenced Oct 15, 2026

Removing the last keybinding (or array item) via jsonEdit leaves a character behind #341686

Closed

Fix jsonEdit leaving a character behind when removing the last array element #341687

Closed

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

fix: path, URI, JSON edit utility bugs - #334259

fix: path, URI, JSON edit utility bugs#334259
Martin Aeschlimann (aeschli) merged 8 commits into
microsoft:mainfrom
VirajMishra1:fix-path-uri-util-bugs

Viraj Mishra (VirajMishra1) commented Sep 3, 2026 •

edited

Loading

Uh oh!

vs-code-engineering Bot commented Sep 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Viraj Mishra (VirajMishra1) commented Sep 3, 2026

Uh oh!

Martin Aeschlimann (aeschli) left a comment

Uh oh!

Viraj Mishra (VirajMishra1) commented Sep 7, 2026

Uh oh!

Martin Aeschlimann (aeschli) commented Sep 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

Viraj Mishra (VirajMishra1) commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jsonEdit.ts: removing the last array element left a stray character

uri.ts: userinfo with a colon in the password

Uh oh!

vs-code-engineering Bot commented Sep 3, 2026

📬 CODENOTIFY

Anthony Kim (@anthonykim1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Viraj Mishra (VirajMishra1) commented Sep 3, 2026

Uh oh!

Martin Aeschlimann (aeschli) left a comment

Choose a reason for hiding this comment

Uh oh!

Viraj Mishra (VirajMishra1) commented Sep 7, 2026

Uh oh!

Martin Aeschlimann (aeschli) commented Sep 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Viraj Mishra (VirajMishra1) commented Sep 3, 2026 •

edited

Loading