Skip to content

C#: Use ProcessStartInfo built-in for correct escaping/quoting in the Dependency manager. - #22377

Merged
michaelnebel merged 1 commit into
github:mainfrom
michaelnebel:csharp/dotnetquoting
Aug 27, 2026
Merged

C#: Use ProcessStartInfo built-in for correct escaping/quoting in the Dependency manager.#22377
michaelnebel merged 1 commit into
github:mainfrom
michaelnebel:csharp/dotnetquoting

Conversation

@michaelnebel

@michaelnebelmichaelnebel commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Instead of escaping/quoting of arguments when we invoke dotnet in the dependency manager, we should rely on the built-in support in ProcessStartInfo. That is, we should supply the arguments to the call via ArgumentList instead of Arguments (documentation can be seen here).

DCA looks good.

Also ran QA (note - this is only internally visible for GitHub), which showed significant extraction regressions for some repos (among others the dotnet/project-system). However, Iattempted to do the extraction locally and didn't observe a similar regression. Will run QA again just to make sure.

  • Issue with link to dashboard that shows regressions can be seen here.
  • Issue with link to dashboard that doesn't show any regressions can be seen here. It is worth noting that baseline variant failed to execute in several thousand repositories. However, from the "Run list" we can see that vsvim/vsvim, dotnet/project-system, microsoft/nodejstools and microsoft/ptvs succeeded in both variants and the execution times are similar (these were the outliers in the first QA run).

Comment threadcsharp/extractor/Semmle.Extraction.Tests/DotNet.cs Fixed

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Migrates dependency-manager process invocation from manually quoted strings to structured argument lists for reliable escaping.

Changes:

  • Refactors .NET and NuGet invocations to use argument lists.
  • Updates feed-source and restore APIs accordingly.
  • Adapts tests and source-generator execution.
Show a summary per file
FileDescription
Runtime.csUpdates the IDotNet test stub.
DotNet.cs (tests)Adapts command-construction tests.
DotnetSourceGeneratorWrapper.csPasses compiler arguments separately.
PackagesConfigRestorer.csUses structured NuGet arguments.
NugetPackageRestorer.csPropagates source argument lists.
IDotNetCliInvoker.csChanges invoker methods to list arguments.
IDotNet.csUpdates public command and restore contracts.
FeedManager.csProduces structured source arguments.
DotNetCliInvoker.csStarts processes with argument lists.
DotNet.cs (dependency fetching)Constructs all CLI commands as argument lists.

Review details

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

  • Files reviewed: 10/10 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment threadcsharp/extractor/Semmle.Extraction.Tests/DotNet.cs Outdated
@michaelnebel
michaelnebelforce-pushed the csharp/dotnetquoting branch 2 times, most recently from edeaaa0 to 9fd3396CompareAugust 24, 2026 12:19
@michaelnebel
michaelnebel requested a balanced review from CopilotAugust 24, 2026 12:41

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review details

  • Files reviewed: 10/10 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@michaelnebel
michaelnebel marked this pull request as ready for review August 25, 2026 07:35
@michaelnebel
michaelnebel requested a review from a team as a code ownerAugust 25, 2026 07:35
@michaelnebelmichaelnebel added the no-change-note-required This PR does not need a change note label Aug 25, 2026

@hvitvedhvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@michaelnebel
michaelnebel merged commit 68f4039 into github:mainAug 27, 2026
16 checks passed
@michaelnebel
michaelnebel deleted the csharp/dotnetquoting branch August 27, 2026 10:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C#no-change-note-requiredThis PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@michaelnebel@hvitved@github-advanced-security