coder/vscode-coderPublic

NotificationsYou must be signed in to change notification settings
Fork35
Star102

feat: handle windows specific paths in log-path parameter#512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

mtojek merged 1 commit intomainfrommike/430-vs-code-win-path

Jun 23, 2025

Merged

feat: handle windows specific paths in log-path parameter#512

mtojek merged 1 commit intomainfrommike/430-vs-code-win-path

Jun 23, 2025

Conversation

Copy link

Collaborator

ibetitsmike commentedMay 22, 2025•
edited
Loading

Fixes:#430

matifali reviewed

May 26, 2025

View reviewed changes

package-lock.json OutdatedShow resolvedHide resolved

ibetitsmike force-pushed themike/430-vs-code-win-path branch from44e3dfa toab2f929Compare

June 10, 2025 16:28

Copy link

Contributor

bcpeinhardt commentedJun 10, 2025•
edited
Loading

I suspect the issue was that claude nuked your lockfile (it likes to do that). Just reverting the lockfile fixed the build. I'll leave passing the test and lint checks up to you :)

ibetitsmike force-pushed themike/430-vs-code-win-path branch frombdc35ba to281fe1aCompare

June 17, 2025 17:28

ibetitsmike closed this

Jun 17, 2025

ibetitsmike force-pushed themike/430-vs-code-win-path branch from281fe1a tobd9d1caCompare

June 17, 2025 17:47

bug: added escaping the cmd arguments for log-path

a9a1cb2

ibetitsmike reopened this

Jun 17, 2025

matifali requested a review fromjaggederest

June 18, 2025 07:24

jaggederest requested a review fromcode-asher

June 18, 2025 17:28

jaggederest reviewed

Jun 18, 2025

View reviewed changes

Copy link

Contributor

jaggederest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This seems fine to me, but I am not yet competent to hand out the ✅ of approval so I've asked Asher to take a look 👍

Copy link

Member

code-asher commentedJun 20, 2025•
edited
Loading

It seems like a reasonable change to me since we already useescapeCommandArg() on other path arguments. It would be nice to see a test forupdateSSHConfig().

The issue says we are printing--log-dir C%3A%5CUsers%5Cmicha%5Clogs, so I assume now we will be printing--log-dir "C%3A%5CUsers%5Cmicha%5Clogs" which might work (or at least not error), but it seems like we should really be printing--log-dir "C:\Users\micha\logs".

Do we know where the escaped chars came from?

code-asher approved these changes

Jun 20, 2025

View reviewed changes

Copy link

Member

code-asher commentedJun 20, 2025

Oh duh it was usingescape before. Looks good to me. I do think we should test this stuff eventually.

Copy link

Member

code-asher commentedJun 20, 2025•
edited
Loading

I know what happened,escapeCommandArg used to be calledescape and was local toupdateSSHConfig, thenformatLogArg was broken out into its own function, with no access to the localescape, butescape happens to be a built-in function so no errors were thrown lol

Copy link

CollaboratorAuthor

ibetitsmike commentedJun 20, 2025

Oh duh it was usingescape before. Looks good to me. I do think we should test this stuff eventually.

@code-asher as in 'unit test'?

Copy link

Member

code-asher commentedJun 20, 2025

Unit test would be ideal yeah! But I thinkupdateSSHConfig() would need to be broken out ofRemote. Might not be worth the effort right now, feel free to merge.