Skip to content

[dotnet] run format against slnx instead of looping csproj#17483

Open
titusfortner wants to merge 6 commits into
trunkfrom
format_slnx
Open

[dotnet] run format against slnx instead of looping csproj#17483
titusfortner wants to merge 6 commits into
trunkfrom
format_slnx

Conversation

@titusfortner
Copy link
Copy Markdown
Member

🔗 Related Issues

Implements #17457

💥 What does this PR do?

Uses slnx instead of looping over csproj. Format commit time is cut in half.

🔧 Implementation Notes

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Codex
    • What was generated: everything
    • I reviewed all AI output and can explain the change

@titusfortner titusfortner requested a review from nvborisenko May 16, 2026 14:48
@selenium-ci selenium-ci added C-dotnet .NET Bindings B-build Includes scripting, bazel and CI integrations labels May 16, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 16, 2026

Review Summary by Qodo

(Agentic_describe updated until commit 09a78e4)

Run dotnet format against slnx solution file instead of looping csproj files

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Replace per-project formatting loop with single slnx solution file
• Significantly improves format command performance by half
• Add solution file existence validation and error handling
• Fix Windows script runfiles directory path resolution
• Set AllowMissingPrunePackageData environment variable for Bazel SDK compatibility
Diagram
flowchart LR
  A["Loop over individual csproj files"] -->|Replace with| B["Single slnx solution file"]
  B -->|Performance gain| C["Format time cut in half"]
  D["Add validation"] -->|Ensure| B
  E["Set environment variable"] -->|Enable| B
Loading

Grey Divider

File Changes

1. dotnet/private/dotnet_format.bzl ✨ Enhancement +22/-14

Refactor dotnet format to use slnx solution file

• Replace find-and-loop pattern over csproj files with direct slnx solution file formatting
• Add solution file path variable and existence validation with error handling
• Set AllowMissingPrunePackageData environment variable for Bazel SDK compatibility
• Fix Windows script runfiles directory path from %~dp0%~n0 to %~dp0%~nx0
• Update both Unix shell and Windows batch script implementations

dotnet/private/dotnet_format.bzl


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 16, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1)

Grey Divider


Action required

1. Dotnet format args reordered ✓ Resolved 🐞 Bug ≡ Correctness
Description
The wrapper now calls dotnet format <solution> $@, but repo tooling invokes //dotnet:format with
positional commands like style/whitespace/analyzers as the first arguments; this changes the
resulting CLI shape from dotnet format style ... <workspace> to `dotnet format <workspace> style
.... This is likely to cause parsing failures and break formatting/lint workflows that call bazel
run //dotnet:format -- style ...`.
Code

dotnet/private/dotnet_format.bzl[R61-62]

+echo "Running dotnet format $@ on Selenium.slnx..."
+"$DOTNET" format "$SOLUTION" "$@" || exit 1
Evidence
Repo tooling passes positional dotnet format commands (style, whitespace, analyzers) as the
first arguments to //dotnet:format. The PR change moves the workspace (Selenium.slnx) ahead of
those positional tokens, altering the command structure compared to the prior implementation that
appended the workspace last.

dotnet/private/dotnet_format.bzl[30-65]
dotnet/private/dotnet_format.bzl[77-106]
rake_tasks/dotnet.rake[119-132]
scripts/format.sh[124-129]
scripts/format.ps1[118-123]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`dotnet/private/dotnet_format.bzl` changed the invocation from passing user-provided args before the workspace (`... format "$@" "$proj"`) to passing the workspace first (`... format "$SOLUTION" "$@"`). In this repo, `//dotnet:format` is invoked with positional commands like `style`, `whitespace`, and `analyzers` (without leading dashes), which are expected to be the first tokens after `format`.

## Issue Context
Existing callers (rake tasks and format scripts) run `bazel run //dotnet:format -- style ...` and `-- whitespace`, so the wrapper must preserve the previous ordering where `$@` comes before the workspace.

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[61-63]
- dotnet/private/dotnet_format.bzl[102-104]

## Suggested change
- Unix: change to `"$DOTNET" format "$@" "$SOLUTION"` (and adjust the echo if desired)
- Windows: change to `"%DOTNET%" format %* "%SOLUTION%"` (keeping quoting correct)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Windows uses %* unsafely 📘 Rule violation ⛨ Security ⭐ New
Description
The Windows formatter wrapper forwards all user-provided arguments via unquoted %*, which allows
CMD metacharacters (e.g., &, |) to be interpreted as additional commands and can break CI/script
safety guarantees. This violates the requirement to harden scripts with safe argument handling.
Code

dotnet/private/dotnet_format.bzl[109]

+"%DOTNET%" format %* "%SOLUTION%" || exit /b 1
Evidence
PR Compliance ID 14 requires scripts to use safe argument handling and avoid fragile/unsafe forms.
The new Windows command executes dotnet format with raw %*, which can be interpreted by
cmd.exe when it contains metacharacters, violating that requirement.

dotnet/private/dotnet_format.bzl[108-110]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Windows batch script forwards arguments using `%*` without any validation/escaping. In `cmd.exe`, metacharacters in `%*` can be interpreted by the shell (command injection / unexpected extra commands), making the script less safe and less deterministic.

## Issue Context
This is a build/CI helper script (`dotnet_format`) that is commonly run in automated environments. The compliance requirement asks for hardened scripts with safe argument handling.

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[108-110]

Potential approaches (pick one that fits project constraints):
- Validate allowed arguments (e.g., only known `dotnet format` subcommands/options) and fail fast on anything else.
- If arbitrary args must be supported, implement a safer forwarding strategy for batch (e.g., parse known flags into variables and reconstruct the command), avoiding raw `%*` expansion in the executed command.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 09a78e4

Results up to commit 38dc133


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)


Action required
1. Dotnet format args reordered ✓ Resolved 🐞 Bug ≡ Correctness
Description
The wrapper now calls dotnet format <solution> $@, but repo tooling invokes //dotnet:format with
positional commands like style/whitespace/analyzers as the first arguments; this changes the
resulting CLI shape from dotnet format style ... <workspace> to `dotnet format <workspace> style
.... This is likely to cause parsing failures and break formatting/lint workflows that call bazel
run //dotnet:format -- style ...`.
Code

dotnet/private/dotnet_format.bzl[R61-62]

+echo "Running dotnet format $@ on Selenium.slnx..."
+"$DOTNET" format "$SOLUTION" "$@" || exit 1
Evidence
Repo tooling passes positional dotnet format commands (style, whitespace, analyzers) as the
first arguments to //dotnet:format. The PR change moves the workspace (Selenium.slnx) ahead of
those positional tokens, altering the command structure compared to the prior implementation that
appended the workspace last.

dotnet/private/dotnet_format.bzl[30-65]
dotnet/private/dotnet_format.bzl[77-106]
rake_tasks/dotnet.rake[119-132]
scripts/format.sh[124-129]
scripts/format.ps1[118-123]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`dotnet/private/dotnet_format.bzl` changed the invocation from passing user-provided args before the workspace (`... format "$@" "$proj"`) to passing the workspace first (`... format "$SOLUTION" "$@"`). In this repo, `//dotnet:format` is invoked with positional commands like `style`, `whitespace`, and `analyzers` (without leading dashes), which are expected to be the first tokens after `format`.

## Issue Context
Existing callers (rake tasks and format scripts) run `bazel run //dotnet:format -- style ...` and `-- whitespace`, so the wrapper must preserve the previous ordering where `$@` comes before the workspace.

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[61-63]
- dotnet/private/dotnet_format.bzl[102-104]

## Suggested change
- Unix: change to `"$DOTNET" format "$@" "$SOLUTION"` (and adjust the echo if desired)
- Windows: change to `"%DOTNET%" format %* "%SOLUTION%"` (keeping quoting correct)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Comment thread dotnet/private/dotnet_format.bzl Outdated
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 16, 2026

Persistent review updated to latest commit 71273a5

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 16, 2026

Persistent review updated to latest commit 837f368

@titusfortner titusfortner marked this pull request as draft May 17, 2026 13:54
@titusfortner
Copy link
Copy Markdown
Member Author

Interesting, this works locally on my mac, even when I delete cache and things, adding diagnostics to debug issue on windows.

@titusfortner
Copy link
Copy Markdown
Member Author

There we go, it was erroring based on Pruned data on Windows, these settings fixed it. verified that time on CI is much less as well as local

@titusfortner titusfortner marked this pull request as ready for review May 17, 2026 14:47
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 17, 2026

Persistent review updated to latest commit 09a78e4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-dotnet .NET Bindings Compliance violation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants