Conversation
WalkthroughBumps multiple dependency and SDK versions, updates a dotnet global tool version, replaces two F# range constructions with a List.replicate-based construction in Args.fs, and injects DOTNET_ROLL_FORWARD and DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FX environment variables into process execution in Build/actions.fs and Build/targets.fs. No files added or removed; no public API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Build as Build/actions.fs / Build/targets.fs
participant Proc as CreateProcess
participant Tool as dotnet / External tool
Build->>Proc: Build Run(..., env)
Note right of Proc #F3F4F6: Inject env vars<br/>DOTNET_ROLL_FORWARD="Major"<br/>DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FX="2"
Proc->>Tool: Start process with environment
Tool-->>Proc: Exit code / output
Proc-->>Build: Return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.config/dotnet-tools.json(1 hunks)Build/targets.fs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: linux
- GitHub Check: windows
🔇 Additional comments (2)
Build/targets.fs (1)
1541-1566: Nice consolidation onto RunDotnet.Standardizing the Coveralls upload through the same RunDotnet path improves consistency and reuse of shared options (working dir, roll-forward).
.config/dotnet-tools.json (1)
6-6: Version bump looks good — CI must restore the ReportGenerator toolVerification failed because dotnet is not available in the verification environment ("dotnet: command not found"). Ensure your CI pipeline runs
dotnet tool restoreso ReportGenerator v5.4.14 is installed before ReportGenerator.generateReports runs.
| let options = | ||
| dotnetOptions | ||
| >> withWorkingDirectoryVN "_Reports" | ||
| >> dotnetOptionsWithRollForwards | ||
|
|
||
| let args = | ||
| "--opencover" | ||
| + " -i " | ||
| + coverage | ||
| + " --repoToken " | ||
| + (Environment.environVar "COVERALLS_REPO_TOKEN") | ||
| + " --commitId " | ||
| + commitHash | ||
| + " --commitBranch " | ||
| + Information.getBranchName (".") | ||
| + " --commitAuthor " | ||
| + (maybe "COMMIT_AUTHOR" "") | ||
| + " --commitEmail " | ||
| + (maybe "COMMIT_AUTHOR_EMAIL" "") | ||
| + " --commitMessage " | ||
| + commit | ||
| + " --jobId " | ||
| + DateTime.UtcNow.ToString("yyMMdd-HHmmss") | ||
|
|
||
| Actions.RunDotnet options "csmacnz.Coveralls" args "Coveralls upload failed" | ||
|
|
There was a problem hiding this comment.
❓ Verification inconclusive
Quote args and avoid passing the Coveralls token on the command line (breakage + secret leak).
- Unquoted values (coverage path, author, commit message) will break when they contain spaces; commit is currently derived with a leading space.
- Passing --repoToken logs the secret in process args (even if masked on GH Actions, it’s unnecessary risk). Prefer env var.
Apply this diff to safely quote values, trim the commit message, and source the token from env:
- let options =
- dotnetOptions
- >> withWorkingDirectoryVN "_Reports"
- >> dotnetOptionsWithRollForwards
-
- let args =
- "--opencover"
- + " -i "
- + coverage
- + " --repoToken "
- + (Environment.environVar "COVERALLS_REPO_TOKEN")
- + " --commitId "
- + commitHash
- + " --commitBranch "
- + Information.getBranchName (".")
- + " --commitAuthor "
- + (maybe "COMMIT_AUTHOR" "")
- + " --commitEmail "
- + (maybe "COMMIT_AUTHOR_EMAIL" "")
- + " --commitMessage "
- + commit
- + " --jobId "
- + DateTime.UtcNow.ToString("yyMMdd-HHmmss")
+ // Inject token via environment to avoid leaking it in logs/args
+ let withEnvFrom name (o: DotNet.Options) =
+ let v = Environment.environVar name
+ if String.IsNullOrWhiteSpace v then o else o.WithEnvironment (o.Environment.Add(name, v))
+
+ let options =
+ dotnetOptions
+ >> withWorkingDirectoryVN "_Reports"
+ >> dotnetOptionsWithRollForwards
+ >> withEnvFrom "COVERALLS_REPO_TOKEN"
+
+ // Quote values that may contain spaces; trim leading space from shortlog slice
+ let q (s: string) = "\"" + s.Replace("\"", "\\\"") + "\""
+ let commitMsg = commit.TrimStart()
+
+ let args =
+ String.concat " "
+ [ "--opencover"
+ "-i"; q coverage
+ "--commitId"; commitHash
+ "--commitBranch"; q (Information.getBranchName ".")
+ "--commitAuthor"; q (maybe "COMMIT_AUTHOR" "")
+ "--commitEmail"; q (maybe "COMMIT_AUTHOR_EMAIL" "")
+ "--commitMessage"; q commitMsg
+ "--jobId"; DateTime.UtcNow.ToString("yyMMdd-HHmmss") ]You may also want to switch commit extraction to log.Substring(gap + 1) at Line 1539 to avoid the leading space entirely.
Do not pass COVERALLS_REPO_TOKEN on the CLI — quote args and trim the commit message.
Unquoted values (coverage path, author, commit message) will break when they contain spaces; the current commit extraction introduces a leading space. Passing --repoToken exposes the secret in process args. Fix: inject COVERALLS_REPO_TOKEN into the dotnet Options environment (don’t append --repoToken), escape/quote every argument (use a small q helper), trim the commit (commit.TrimStart()), and build args with String.concat instead of raw string concatenation.
File: Build/targets.fs lines 1541–1566
🤖 Prompt for AI Agents
In Build/targets.fs around lines 1541 to 1566, the current args string
concatenation leaks the COVERALLS_REPO_TOKEN on the command line, fails to
quote/escape values that may contain spaces (coverage path, author, commit
message), and leaves a leading space in the commit; fix by: stop appending
--repoToken to args and instead add the token into dotnetOptions environment
(e.g. augment dotnetOptions to set "COVERALLS_REPO_TOKEN"), trim the commit with
commit.TrimStart() (or Trim()), create a small q helper that quotes/escapes
values, build the argument list as a string array or list of quoted parts and
join with String.concat " " (or String.Join " "), and ensure every interpolated
value (coverage, branch, author, email, commit, jobId) is passed through q so
spaces are safe and secrets are not exposed in process args.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
AltCover.Engine/Args.fs (2)
109-110: Replace range+map with replicate; preserves semantics, fewer allocations.Using a range solely to repeat a constant element builds an unnecessary sequence. Replicate the flag directly, clamping negatives.
- |> List.collect (fun (a, b) -> seq { 0..b } |> Seq.map (fun _ -> a) |> Seq.toList) + |> List.collect (fun (a, b) -> + let n = max 0 (b + 1) + List.replicate n a)
156-157: Do the same replicate refactor here for consistency and lower churn.- |> List.collect (fun (a, b) -> seq { 0..b } |> Seq.map (fun _ -> a) |> Seq.toList) + |> List.collect (fun (a, b) -> + let n = max 0 (b + 1) + List.replicate n a)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
AltCover.Engine/Args.fs(2 hunks)Build/targets.fs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: windows
- GitHub Check: linux
🔇 Additional comments (1)
Build/targets.fs (1)
1541-1565: Do not pass COVERALLS_REPO_TOKEN on the CLI; quote args and trim the commit message.Token is exposed via process args and logs; several values aren’t safely quoted; commit retains a leading space. Inject the token via environment, quote all dynamic values, and trim the commit.
Apply this diff:
- let options = - dotnetOptions - >> withWorkingDirectoryVN "_Reports" - >> dotnetOptionsWithRollForwards - - let args = - "--opencover" - + " -i " - + coverage - + " --repoToken \"" - + (Environment.environVar "COVERALLS_REPO_TOKEN") - + "\" --commitId " - + commitHash - + " --commitBranch " - + Information.getBranchName (".") - + " --commitAuthor \"" - + (maybe "COMMIT_AUTHOR" "") - + "\" --commitEmail \"" - + (maybe "COMMIT_AUTHOR_EMAIL" "") - + "\" --commitMessage \"" - + commit - + "\" --jobId " - + DateTime.UtcNow.ToString("yyMMdd-HHmmss") + // Inject token via environment (avoids leaking secret in argv) + let withEnvFrom name (o: DotNet.Options) = + let v = Environment.environVar name + if String.IsNullOrWhiteSpace v then o else o.WithEnvironment (o.Environment.Add(name, v)) + + let q (s: string) = "\"" + s.Replace("\"", "\\\"") + "\"" + let commitMsg = commit.TrimStart() + + let options = + dotnetOptions + >> withWorkingDirectoryVN "_Reports" + >> dotnetOptionsWithRollForwards + >> withEnvFrom "COVERALLS_REPO_TOKEN" + + let args = + String.concat " " + [ "--opencover" + "-i"; q coverage + "--commitId"; commitHash + "--commitBranch"; q (Information.getBranchName ".") + "--commitAuthor"; q (maybe "COMMIT_AUTHOR" "") + "--commitEmail"; q (maybe "COMMIT_AUTHOR_EMAIL" "") + "--commitMessage"; q commitMsg + "--jobId"; DateTime.UtcNow.ToString("yyMMdd-HHmmss") ]Optionally also fix the source of the leading space earlier:
// earlier around commit extraction let commit = log.Substring(gap + 1)#!/bin/bash # Expect: no remaining uses of --repoToken; args are quoted; commitMessage uses TrimStart rg -n --line-number -C1 -e '\-\-repoToken' -e 'commitMessage.*commit[^A-Za-z]' Build/targets.fs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Build/actions.fs (1)
393-393: Prefer modernDOTNET_ROLL_FORWARD(keep legacy var for compatibility).
DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FXis superseded; hosts since .NET Core 3.0 preferDOTNET_ROLL_FORWARD. To preserve intent (allow major/minor/patch roll-forward) on .NET 9 while keeping back-compat, set both, withDOTNET_ROLL_FORWARD=Major. (learn.microsoft.com)Apply:
- |> CreateProcess.withEnvironment [("DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FX", "2")] + |> CreateProcess.withEnvironment [ + ("DOTNET_ROLL_FORWARD", "Major"); + ("DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FX", "2") ]If you’d rather not override a user-provided setting, gate the additions:
- |> CreateProcess.withEnvironment [("DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FX", "2")] + let rollForwardEnv = + [ + if not (Environment.hasEnvironVar "DOTNET_ROLL_FORWARD") then + "DOTNET_ROLL_FORWARD", "Major" + if not (Environment.hasEnvironVar "DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FX") then + "DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FX", "2" + ] + |> fun e -> CreateProcess.withEnvironment eFor background on legacy vs. new behavior and values, see docs. (learn.microsoft.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Build/actions.fs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: windows
- GitHub Check: linux
🔇 Additional comments (1)
Build/actions.fs (1)
386-396: Confirm scope: DOTNET_RO_FORWARD only affects .NET-hosted processesRun (Build/actions.fs) currently injects DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FX for every CreateProcess call, but many callers use Run for native tools (e.g., Build/targets.fs:919 ILRepack, 957/977 mcs, 6826 altcover) so the env var is a no-op there. Dotnet invocations are handled mainly via Actions.RunDotnet or explicit CreateProcess.fromRawCommand "dotnet" (e.g., Build/targets.fs:989). If the goal is to influence dotnet behavior, add the variable to DotNet.Options in RunDotnet or only attach the env when launching 'dotnet' processes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Build/targets.fs (1)
990-996: Good: ensure lint runs with roll-forward.Nit: consider factoring a tiny helper to set both env vars to avoid future drift with dotnetOptionsWithRollForwards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
AltCover.Engine/Args.fs(2 hunks)Build/actions.fs(1 hunks)Build/targets.fs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- AltCover.Engine/Args.fs
- Build/actions.fs
🔇 Additional comments (2)
Build/targets.fs (2)
209-215: Roll-forward env vars added — looks good.Setting both DOTNET_ROLL_FORWARD_ON_NO_CANDIDATE_FX=2 and DOTNET_ROLL_FORWARD=Major at the DotNet.Options layer is reasonable.
Please confirm "Major" is the intended policy across CI agents (it can change selected frameworks vs LatestMajor).
1540-1566: Do not pass COVERALLS_REPO_TOKEN on the CLI; quote args and trim the commit message.Token on argv is a secret-leak risk; unquoted values (paths, branch, author, message) can break on spaces;
Substring gapleaves a leading space. Switch to Actions.RunDotnet with env injection, remove--repoToken, quote args, and trim commit.- let log = Information.shortlog "." - let gap = log.IndexOf ' ' - let commit = log.Substring gap - - Actions.Run - ("dotnet", - "_Reports", - [ "csmacnz.Coveralls" - "--opencover" - "-i" - coverage - "--repoToken" - Environment.environVar "COVERALLS_REPO_TOKEN" - "--commitId" - commitHash - "--commitBranch" - Information.getBranchName (".") - "--commitAuthor" - maybe "COMMIT_AUTHOR" "" - "--commitEmail" - maybe "COMMIT_AUTHOR_EMAIL" "" - "--commitMessage" - commit - "--jobId" - DateTime.UtcNow.ToString("yyMMdd-HHmmss") ]) - "Coveralls upload failed" + let log = Information.shortlog "." + let gap = log.IndexOf ' ' + let commit = if gap >= 0 then log.Substring(gap + 1) else log + + // Inject token via env; avoid leaking it on argv + let withEnvFrom name (o: DotNet.Options) = + let v = Environment.environVar name + if String.IsNullOrWhiteSpace v then o else o.WithEnvironment (o.Environment.Add(name, v)) + + // Quote/escape values that may contain spaces + let q (s: string) = "\"" + s.Replace("\"", "\\\"") + "\"" + + let args = + String.concat " " + [ "--opencover" + "-i"; q coverage + "--commitId"; commitHash + "--commitBranch"; q (Information.getBranchName ".") + "--commitAuthor"; q (maybe "COMMIT_AUTHOR" "") + "--commitEmail"; q (maybe "COMMIT_AUTHOR_EMAIL" "") + "--commitMessage"; q commit + "--jobId"; DateTime.UtcNow.ToString("yyMMdd-HHmmss") ] + + Actions.RunDotnet + (withWorkingDirectoryVN "_Reports" + >> dotnetOptionsWithRollForwards + >> withEnvFrom "COVERALLS_REPO_TOKEN") + "csmacnz.Coveralls" + args + "Coveralls upload failed"
1 participant
net 9.0.305 and other tooling
Summary by CodeRabbit
Chores
Refactor