Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive release channel support system across CI/CD, build infrastructure, and the desktop client. It refactors the Azure Storage sync workflow from Bash/Azure CLI to a Nuke-based C# solution, adds channel detection and propagation throughout the pipeline, implements Azure Blob adapters for index generation and artifact uploads, extends the desktop client with channel preference management via IPC, and includes extensive documentation covering proposals, specifications, and implementation guides for both server and client components. Changes
Sequence Diagram(s)sequenceDiagram
actor GHA as GitHub Actions
participant Build as GitHub Actions<br/>(build.yml)
participant Summary as Build Summary Job
participant Sync as Sync Job<br/>(sync-azure-storage.yml)
participant NukeBuild as Nuke Build Process<br/>(C#)
participant GitHub as GitHub API<br/>(gh CLI)
participant Azure as Azure Blob Storage
GHA->>Build: Trigger build with tag/branch
Build->>Build: Detect release channel<br/>(from tag or branch)
Build->>Build: Set RELEASE_CHANNEL env var
Build->>Summary: Pass channel via output
Summary->>Summary: Compile final channel<br/>from all job outputs
Summary->>Sync: Invoke with release_channel input
Sync->>Sync: Checkout code
Sync->>Sync: Setup .NET SDK
Sync->>NukeBuild: Execute build.sh<br/>with env vars
NukeBuild->>NukeBuild: GenerateAzureIndex target
NukeBuild->>GitHub: GetLatestReleaseTag (gh CLI)
GitHub-->>NukeBuild: Return tag/version
NukeBuild->>GitHub: GetReleaseAssets<br/>(if needed)
GitHub-->>NukeBuild: Return assets list
NukeBuild->>NukeBuild: Download assets
NukeBuild->>NukeBuild: PublishToAzureBlob target
NukeBuild->>Azure: Upload artifacts
NukeBuild->>NukeBuild: Generate index.json<br/>with channels metadata
NukeBuild->>Azure: Upload index.json
NukeBuild-->>Sync: Exit success
Sync->>Sync: Post-failure reporting<br/>(if needed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/build.yml (1)
617-625:⚠️ Potential issue | 🔴 CriticalMissing
outputsdeclaration inbuild-summaryjob causesrelease_channelto be empty.Line 624 references
needs.build-summary.outputs.channel, but thebuild-summaryjob (lines 488-612) does not declare anoutputs:section. This will result inrelease_channelbeing empty/undefined when passed to the sync-azure workflow.🐛 Proposed fix to add outputs declaration
Add the
outputssection to thebuild-summaryjob:build-summary: name: Build Summary needs: - build-windows - build-linux - build-macos if: always() runs-on: ubuntu-latest + outputs: + channel: ${{ steps.channel.outputs.channel }} steps:.github/workflows/sync-azure-storage.yml (1)
16-19:⚠️ Potential issue | 🟠 MajorDeclare
AZURE_BLOB_SAS_URLinworkflow_callsecrets.In reusable workflows, undeclared secrets resolve to empty;
AZURE_BLOB_SAS_URLis used on line 78 but not defined in theworkflow_callsecrets. Add it to the secrets section or ensure the caller usessecrets: inherit.🛠️ Add the missing secret
secrets: FEISHU_WEBHOOK_URL: required: true + AZURE_BLOB_SAS_URL: + required: true
🤖 Fix all issues with AI agents
In `@nukeBuild/Adapters/GitHubAdapter.cs`:
- Around line 7-12: The field _rootDirectory is assigned but never used; update
the process start configuration in GitHubAdapter so UseShellExecute = false, set
WorkingDirectory = _rootDirectory (so gh runs in the repo context when --repo
isn't provided), and enable RedirectStandardError = true along with any existing
RedirectStandardOutput/RedirectStandardInput settings; locate these changes
where ProcessStartInfo (or process start configuration) is created in the
GitHubAdapter class (constructor GitHubAdapter and the method that launches the
gh process) and apply the three changes together.
- Around line 20-27: The ProcessStartInfo blocks that build the "gh" invocations
(the variable psi in GitHubAdapter.cs) are misconfigured: when you set
RedirectStandardOutput = true you must set UseShellExecute = false, and if you
read StandardError you must set RedirectStandardError = true; update both
ProcessStartInfo instances to UseShellExecute = false and add
RedirectStandardError = true. Also fix the gh arguments that use -q by supplying
a valid jq filter (for example replace "-q .tagName" with "-q '.tagName'" or "-q
'.assets'") so the command is complete; update the Arguments strings used in
those ProcessStartInfo objects accordingly.
In `@nukeBuild/BuildConfig.cs`:
- Around line 1-11: The file references NukeBuild.RootDirectory in BuildConfig
but lacks the required using directive; add the appropriate using so the symbol
resolves (for example add "using Nuke.Common;" or "using static
Nuke.Common.NukeBuild;") so that BuildConfig.AbsolutePath
ReleasePackagedDirectory compiles against NukeBuild.RootDirectory/AbsolutePath.
🟠 Major comments (21)
build.cmd-6-7 (1)
6-7:⚠️ Potential issue | 🟠 MajorReplace ExecutionPolicy ByPass with RemoteSigned to reduce unnecessary permissions.
The
dotnet-install.ps1script downloaded fromdot.net/v1/...is Authenticode-signed by Microsoft, soRemoteSignedwill allow its execution while preventing unsigned remote scripts. Adding-Scope Processlimits the policy to the current session only, following the principle of least privilege.Suggested change
-powershell -ExecutionPolicy ByPass -NoProfile -File "%~dp0build.ps1" %* +powershell -ExecutionPolicy RemoteSigned -Scope Process -NoProfile -File "%~dp0build.ps1" %*Apply the same change to:
nukeBuild/scripts/windows/install.batnukeBuild/scripts/windows/check.batnukeBuild/scripts/linux/check.sh-46-55 (1)
46-55:⚠️ Potential issue | 🟠 MajorString-based version comparison is unreliable for semantic versions.
The
version_gefunction uses shell string comparison (\>) which performs lexicographic ordering. This fails for versions like"9.0.0"vs"10.0.0"since"9">"1"lexicographically.Additionally, as flagged by shellcheck (SC2155), declare and assign separately to avoid masking return values.
🐛 Proposed fix with proper version comparison
version_ge() { if [ -z "$1" ] || [ -z "$2" ]; then return 1 fi # Remove 'v' prefix if present - local v1=$(echo "$1" | sed 's/^v//') - local v2=$(echo "$2" | sed 's/^v//') - # Simple string comparison for now (can be improved with proper version comparison) - [ "$v1" != "$v2" ] && [ "$v1" \> "$v2" ] || [ "$v1" = "$v2" ] + local v1 + local v2 + v1=$(echo "$1" | sed 's/^v//') + v2=$(echo "$2" | sed 's/^v//') + # Use sort -V for proper version comparison + [ "$(printf '%s\n%s' "$v2" "$v1" | sort -V | head -n1)" = "$v2" ] }build.ps1-10-10 (1)
10-10:⚠️ Potential issue | 🟠 MajorAvoid reassigning automatic variable
$PSScriptRoot.As flagged by PSScriptAnalyzer (PSAvoidAssignmentToAutomaticVariable),
$PSScriptRootis an automatic variable that PowerShell sets automatically. Reassigning it can cause unexpected behavior, especially if this script is dot-sourced.🐛 Proposed fix using a different variable name
-$PSScriptRoot = Split-Path $MyInvocation.MyCommand.Path -Parent +$ScriptRoot = Split-Path $MyInvocation.MyCommand.Path -ParentThen update all references from
$PSScriptRootto$ScriptRooton lines 16, 17, and 19.nukeBuild/scripts/linux/check.sh-114-122 (1)
114-122:⚠️ Potential issue | 🟠 MajorOpenSpec version range check has the same string comparison issue.
The version range check at line 116 uses string comparison operators which will fail for versions crossing major version boundaries (e.g.,
0.9.0vs0.23.0where"9">"2"lexicographically).🐛 Proposed fix using sort -V
# Extract version number (remove 'v' prefix if present, format like "0.23.0") VERSION_NUM=$(echo "$OPENSPEC_VERSION" | sed 's/^v//' | sed 's/[^0-9.]//g') - # Simple version comparison: check if version is within range [min, max) - # This uses string comparison which works for semver-like versions - if [[ "$VERSION_NUM" > "$OPENSPEC_MIN_VERSION" || "$VERSION_NUM" == "$OPENSPEC_MIN_VERSION" ]] && [[ "$VERSION_NUM" < "$OPENSPEC_MAX_VERSION" ]]; then + # Version comparison using sort -V for proper semver handling + MIN_OK=$(printf '%s\n%s' "$OPENSPEC_MIN_VERSION" "$VERSION_NUM" | sort -V | head -n1) + MAX_OK=$(printf '%s\n%s' "$VERSION_NUM" "$OPENSPEC_MAX_VERSION" | sort -V | head -n1) + if [[ "$MIN_OK" == "$OPENSPEC_MIN_VERSION" ]] && [[ "$MAX_OK" == "$VERSION_NUM" ]]; then echo -e "${GREEN}✓ OpenSpec found: $OPENSPEC_VERSION${NC}"nukeBuild/scripts/windows/check.bat-5-5 (1)
5-5:⚠️ Potential issue | 🟠 MajorAvoid
-ExecutionPolicy Bypassunless strictly necessary.Bypassing execution policy weakens protections and can allow unintended scripts to run. If possible, use
RemoteSigned(or scope toCurrentUser) and document why Bypass is required.nukeBuild/scripts/macos/check.sh-45-78 (1)
45-78:⚠️ Potential issue | 🟠 MajorRequired dependency minimum versions aren’t validated.
.NET,Node, andNPMare treated as “ready” based solely on presence. If a tool exists but is below the minimum, the check still marks the environment ready. Consider validating versions against the minimums and using those results forREADYandrequiredInstalled.🧭 Suggested pattern (apply to .NET/Node/NPM)
if command -v dotnet &> /dev/null; then DOTNET_VERSION=$(dotnet --version 2>/dev/null || echo "unknown") DOTNET_PATH=$(which dotnet 2>/dev/null || echo "") - echo -e "${GREEN}✓ .NET runtime found: $DOTNET_VERSION${NC}" - DOTNET_STATUS="\"status\": \"installed\", \"version\": \"$DOTNET_VERSION\", \"path\": \"$DOTNET_PATH\"" + if version_ge "$DOTNET_VERSION" "$DOTNET_MIN_VERSION"; then + echo -e "${GREEN}✓ .NET runtime found: $DOTNET_VERSION${NC}" + DOTNET_OK=true + DOTNET_STATUS="\"status\": \"installed\", \"version\": \"$DOTNET_VERSION\", \"path\": \"$DOTNET_PATH\"" + else + echo -e "${RED}✗ .NET runtime too old: $DOTNET_VERSION (min: $DOTNET_MIN_VERSION)${NC}" + DOTNET_OK=false + DOTNET_STATUS="\"status\": \"version_mismatch\", \"version\": \"$DOTNET_VERSION\", \"path\": \"$DOTNET_PATH\"" + fi else ... fiThen compute
READYusingDOTNET_OK && NODE_OK && NPM_OKand incrementrequiredInstalledonly when the version meets min requirements.Also applies to: 121-128
nukeBuild/scripts/macos/install.sh-84-95 (1)
84-95:⚠️ Potential issue | 🟠 Major
--arch=x64is documented but rejected.The normalization only accepts
x86_64|amd64, so--arch=x64fails even though it’s listed in help. Includex64in the accepted patterns.✅ Proposed fix
-case $ARCH in - x86_64|amd64) +case $ARCH in + x86_64|amd64|x64) ARCH="x64" ;;nukeBuild/scripts/macos/check.sh-99-110 (1)
99-110:⚠️ Potential issue | 🟠 MajorFix OpenSpec semver range check (string compare is incorrect).
String comparison breaks for common semver cases (e.g.,
1.10.0vs1.2.0) and can misclassify versions. Use numeric segment comparison instead.🧮 Proposed fix (numeric semver compare helper)
+# Simple numeric semver compare for x.y.z (pads missing segments with 0) +version_ge() { + IFS='.' read -r a b c <<< "$1"; IFS='.' read -r x y z <<< "$2" + printf -v lhs "%03d%03d%03d" "${a:-0}" "${b:-0}" "${c:-0}" + printf -v rhs "%03d%03d%03d" "${x:-0}" "${y:-0}" "${z:-0}" + [[ "$lhs" -ge "$rhs" ]] +} +version_lt() { + IFS='.' read -r a b c <<< "$1"; IFS='.' read -r x y z <<< "$2" + printf -v lhs "%03d%03d%03d" "${a:-0}" "${b:-0}" "${c:-0}" + printf -v rhs "%03d%03d%03d" "${x:-0}" "${y:-0}" "${z:-0}" + [[ "$lhs" -lt "$rhs" ]] +} ... - if [[ "$VERSION_NUM" > "$OPENSPEC_MIN_VERSION" || "$VERSION_NUM" == "$OPENSPEC_MIN_VERSION" ]] && [[ "$VERSION_NUM" < "$OPENSPEC_MAX_VERSION" ]]; then + if version_ge "$VERSION_NUM" "$OPENSPEC_MIN_VERSION" && version_lt "$VERSION_NUM" "$OPENSPEC_MAX_VERSION"; thennukeBuild/Adapters/GitHubAdapter.cs-74-125 (1)
74-125:⚠️ Potential issue | 🟠 Major
-qflag requires a jq expression; command will fail and parsing logic won't work.
-qis shorthand for--jqand expects a jq expression to filter JSON output. The current commandgh release view {tag} --json assets -qis invalid and will error. Even if it succeeded, the line-by-line parsing logic expects a JSON object containingassets, which won't match the output. Simplest fix: drop-qand parse the full JSON output once.✅ Proposed fix (parse full JSON)
-Arguments = $"release view {tag} --json assets -q", +Arguments = $"release view {tag} --json assets", ... -process.Start(); - -var output = new List<GitHubReleaseAsset>(); -var outputReader = new System.IO.StreamReader(process.StandardOutput.BaseStream); -string? line; - -while ((line = await outputReader.ReadLineAsync()) != null) -{ - if (line.TrimStart().StartsWith("{")) - { - try - { - var json = System.Text.Json.JsonDocument.Parse(line); - var root = json.RootElement; - - if (root.TryGetProperty("assets", out var assetsElement)) - { - foreach (var asset in assetsElement.EnumerateArray()) - { - ... - } - } - } - catch (System.Text.Json.JsonException) - { - // Skip non-JSON lines - } - } -} +process.Start(); + +var outputJson = await process.StandardOutput.ReadToEndAsync(); +using var json = System.Text.Json.JsonDocument.Parse(outputJson); +var root = json.RootElement; +var output = new List<GitHubReleaseAsset>(); +if (root.TryGetProperty("assets", out var assetsElement)) +{ + foreach (var asset in assetsElement.EnumerateArray()) + { + ... + } +}nukeBuild/scripts/windows/install.bat-1-5 (1)
1-5:⚠️ Potential issue | 🟠 MajorConvert to Windows line endings (CRLF) for batch file compatibility.
The static analysis tool detected Unix line endings (LF) in this batch file. Windows batch files require CRLF line endings to function correctly - LF-only sequences can cause GOTO/CALL label parsing failures and script malfunction.
To fix, convert the file to Windows line endings using:
- Git: Configure
.gitattributeswith*.bat text eol=crlf- VS Code: Click "LF" in the status bar and select "CRLF"
- Command line:
unix2dos nukeBuild/scripts/windows/install.batsrc/main/main.ts-787-791 (1)
787-791:⚠️ Potential issue | 🟠 MajorAvoid accessing private members via
anycast.Casting
versionManagertoanyto access the privatepackageSourceConfigManagerbreaks encapsulation and is fragile to internal refactoring. IfVersionManagerrenames or removes this field, this code will fail silently at runtime.Consider adding a public method to
VersionManagerfor this operation:🛠️ Suggested approach
Add a method to
VersionManager:// In version-manager.ts async setChannelPreference(channel: string): Promise<boolean> { const currentConfig = this.getCurrentSourceConfig(); if (!currentConfig) { return false; } return this.packageSourceConfigManager.updateSource(currentConfig.id, { defaultChannel: channel }); }Then update the IPC handler:
- // Update defaultChannel in the source config - const packageSourceConfigManager = (versionManager as any).packageSourceConfigManager; - packageSourceConfigManager.updateSource(currentConfig.id, { - defaultChannel: channel - }); + // Update defaultChannel via VersionManager's public API + const updated = await versionManager.setChannelPreference(channel); + if (!updated) { + return { + success: false, + error: 'Failed to update channel preference' + }; + }nukeBuild/Build.Partial.cs-7-11 (1)
7-11:⚠️ Potential issue | 🟠 MajorDocumentation doesn't match implementation—fallback to
GitHubTokenparameter is missing.The XML documentation states the priority is "GitHubActions.Instance.Token (CI) > GitHubToken (parameter)", but the implementation only returns
GitHubActions?.Tokenwithout any fallback to theGitHubTokenparameter defined inBuild.cs.This means when running locally (where
GitHubActions.Instanceis null),EffectiveGitHubTokenreturnsnullinstead of falling back to the manually provided token parameter, causing the error handling inBuild.AzureStorage.cs(line 104) to fail.🔧 Proposed fix to implement the documented fallback
/// <summary> /// Gets the GitHub token from CI or parameter /// Priority: GitHubActions.Instance.Token (CI) > GitHubToken (parameter) /// </summary> - string EffectiveGitHubToken => GitHubActions?.Token; + string EffectiveGitHubToken => GitHubActions?.Token ?? GitHubToken;src/renderer/store/thunks/packageSourceThunks.ts-19-28 (1)
19-28:⚠️ Potential issue | 🟠 MajorAdd complete type declarations for
packageSourcemethods used in this file.The
declare globalblock declares onlyelectronAPI.version.setChannel, but the file uses eight methods fromwindow.electronAPI.packageSource(lines 40, 64, 89, 139, 189, 221, 271, 321):getConfig(),getAllConfigs(),setConfig(),switchSource(),validateConfig(),scanFolder(),fetchGithub(), andfetchHttpIndex(). While TypeScript may merge declarations across modules, explicitly declaring all used API surface in this file prevents hidden dependencies on other files' type declarations and improves code clarity.Suggested fix
// Types for window electronAPI declare global { interface Window { electronAPI: { version: { setChannel: (channel: string) => Promise<{ success: boolean; error?: string }>; }; packageSource: { getConfig: () => Promise<StoredPackageSourceConfig | null>; getAllConfigs: () => Promise<StoredPackageSourceConfig[]>; setConfig: (config: any) => Promise<{ success: boolean; error?: string }>; switchSource: (sourceId: string) => Promise<{ success: boolean; error?: string }>; validateConfig: (config: any) => Promise<{ valid: boolean; error?: string }>; scanFolder: (folderPath: string) => Promise<any>; fetchGithub: (params: any) => Promise<any>; fetchHttpIndex: (params: any) => Promise<any>; }; }; } }nukeBuild/Build.AzureStorage.cs-122-137 (1)
122-137:⚠️ Potential issue | 🟠 MajorFail fast when
gh release downloadfails.
exitCodeis computed but never checked, so auth or network failures become “no assets” and the workflow can silently skip uploads. Throw on non‑zero exit.✅ Guard on failure
- var exitCode = await GhDownloadReleaseAssetsAsync(versionTag, downloadDirectory); + var exitCode = await GhDownloadReleaseAssetsAsync(versionTag, downloadDirectory); + if (exitCode != 0) + { + throw new Exception($"gh release download failed (exit code {exitCode})"); + } downloadedFiles = Directory.GetFiles(downloadDirectory) .Where(f => !File.GetAttributes(f).HasFlag(FileAttributes.Directory)) .ToList();nukeBuild/Build.AzureStorage.cs-234-258 (1)
234-258:⚠️ Potential issue | 🟠 MajorRedirect StandardError before reading it.
Line 257 reads
StandardErrorbut theProcessStartInfodoes not setRedirectStandardError = true, which causes anInvalidOperationExceptionat runtime. Add the redirect flag to enable reading the error stream.🛠️ Fix stderr redirection
var psi = new ProcessStartInfo { FileName = "gh", Arguments = "release view --json tagName -q .tagName", RedirectStandardOutput = true, + RedirectStandardError = true, UseShellExecute = false, CreateNoWindow = true,nukeBuild/scripts/windows/install.ps1-50-146 (1)
50-146:⚠️ Potential issue | 🟠 MajorEnforce minimum versions instead of only checking presence.
The script loads min/recommended version variables but never uses them; a stale .NET/Node/NPM install will be treated as “installed” and skipped. Please compare installed versions against the minimums (and upgrade to recommended when below).
💡 Example fix (apply similarly to .NET/NPM/Claude/OpenSpec)
+ function Test-VersionAtLeast { + param([string]$Current, [string]$Minimum) + try { return ([version]($Current.TrimStart('v'))) -ge ([version]$Minimum) } + catch { return $false } + } ... - if ($NodeInfo.status -eq "installed") { + if ($NodeInfo.status -eq "installed" -and (Test-VersionAtLeast $NodeInfo.version $NodeMinVersion)) { Write-ColorOutput "Node.js already installed: $($NodeInfo.version)" "ok" } else { Write-Host "Node.js not found, installing..."nukeBuild/Adapters/AzureBlobAdapter.cs-180-184 (1)
180-184:⚠️ Potential issue | 🟠 MajorLogic error in validation condition.
The condition uses
&&(AND), which only triggers the warning if both properties are missing. It should use||(OR) to warn if either property is missing.🐛 Proposed fix
- if (!root.TryGetProperty("version", out _) && + if (!root.TryGetProperty("version", out _) || !root.TryGetProperty("files", out _)) { Log.Warning("Index.json may be missing expected properties"); }nukeBuild/scripts/windows/lib/shared.psm1-271-283 (1)
271-283:⚠️ Potential issue | 🟠 MajorAvoid assigning to
$argsautomatic variable.
$argsis a PowerShell automatic variable containing unbound arguments. Assigning to it can cause unexpected behavior. Rename to$buildArgsor$scriptArgs.🐛 Proposed fix for Invoke-AzureIndexGeneration
Write-ColorOutput "=== 步骤 1: 生成 Azure Index ===" "info" - $args = @("--target", "GenerateAzureIndex") + $buildArgs = @("--target", "GenerateAzureIndex") if (-not [string]::IsNullOrEmpty($AzureBlobSasUrl)) { - $args += "--azure-blob-sas-url" - $args += $AzureBlobSasUrl + $buildArgs += "--azure-blob-sas-url" + $buildArgs += $AzureBlobSasUrl } if (-not [string]::IsNullOrEmpty($IndexOutputPath)) { - $args += "--azure-index-output-path" - $args += $IndexOutputPath + $buildArgs += "--azure-index-output-path" + $buildArgs += $IndexOutputPath } - $args += $AdditionalArgs + $buildArgs += $AdditionalArgs try { - & $BuildScriptPath $args + & $BuildScriptPath $buildArgsnukeBuild/Adapters/AzureBlobAdapter.cs-21-39 (1)
21-39:⚠️ Potential issue | 🟠 Major
ValidateSasUrlAsyncdoesn't actually validate the SAS URL.This method only checks if the string is non-empty, but doesn't validate:
- URL format correctness
- SAS token presence/structure
- Actual connectivity to Azure Storage
The method name and callers (e.g.,
Build.AzureStorage.cslines 16-19) suggest real validation. Consider either:
- Implementing actual validation (parse URI, check for
sv=,sig=parameters, optionally test connectivity)- Renaming to
HasSasUrl()to reflect actual behavior🔧 Proposed fix - add actual SAS URL validation
public async Task<bool> ValidateSasUrlAsync(string sasUrl) { try { if (string.IsNullOrWhiteSpace(sasUrl)) { Log.Warning("SAS URL is empty"); return false; } - Log.Information("Validating SAS URL"); - return true; + // Validate URL format + if (!Uri.TryCreate(sasUrl, UriKind.Absolute, out var uri)) + { + Log.Warning("SAS URL is not a valid URI"); + return false; + } + + // Check for required SAS parameters + var query = uri.Query; + if (string.IsNullOrEmpty(query) || !query.Contains("sig=")) + { + Log.Warning("SAS URL appears to be missing signature parameter"); + return false; + } + + // Optionally test connectivity + var containerClient = new BlobContainerClient(uri); + await containerClient.GetPropertiesAsync(); + + Log.Information("SAS URL validated successfully"); + return true; } catch (Exception ex) { Log.Error(ex, "SAS URL validation failed"); return false; } }nukeBuild/scripts/windows/lib/shared.psm1-330-342 (1)
330-342:⚠️ Potential issue | 🟠 MajorSame
$argsvariable issue inInvoke-AzureBlobPublish.Apply the same rename from
$argsto$buildArgshere as well.🐛 Proposed fix
- $args = @("--target", "PublishToAzureBlob") + $buildArgs = @("--target", "PublishToAzureBlob") if (-not [string]::IsNullOrEmpty($AzureBlobSasUrl)) { - $args += "--azure-blob-sas-url" - $args += $AzureBlobSasUrl + $buildArgs += "--azure-blob-sas-url" + $buildArgs += $AzureBlobSasUrl } if (-not [string]::IsNullOrEmpty($LocalIndexPath)) { - $args += "--azure-index-output-path" - $args += $LocalIndexPath + $buildArgs += "--azure-index-output-path" + $buildArgs += $LocalIndexPath } - $args += $AdditionalArgs + $buildArgs += $AdditionalArgs try { - & $BuildScriptPath $args + & $BuildScriptPath $buildArgsnukeBuild/Adapters/AzureBlobAdapter.cs-108-143 (1)
108-143:⚠️ Potential issue | 🟠 Major
GenerateIndexOnlyAsyncgenerates placeholder data instead of actual index.This method creates hardcoded index data with
version = "1.0.0",channel = "beta", and an empty files list, which provides no actual indexing value. The method doesn't utilize theoptionsparameter (particularlySasUrl) to retrieve real blob data, unlikeGenerateIndexFromBlobsAsync(line 278) which properly fetches and indexes actual blobs from Azure Storage.The caller in
Build.AzureStorage.cs(line 41) expects valid JSON output, which the method produces—but the content is meaningless for an index file (empty files list). Either implement this method to generate real index data from blobs, or clarify if it's intentionally a stub for testing purposes.
🟡 Minor comments (25)
nukeBuild/scripts/windows/start.bat-1-32 (1)
1-32:⚠️ Potential issue | 🟡 MinorUse CRLF line endings for Windows batch scripts.
This file currently uses LF-only line endings, which can trigger parsing quirks in
cmd.exe. Convert to CRLF for proper Windows batch compatibility.nukeBuild/scripts/macos/start.sh-6-7 (1)
6-7:⚠️ Potential issue | 🟡 MinorAdd error handling to the
cdcommand to prevent silent failures.The
libdirectory may not exist in the current context, and ifcdfails, the script will continue executing in the wrong directory (the script directory instead of lib), causingdotnet PCode.Web.dllto fail with a confusing error message.Suggested change
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -cd "$SCRIPT_DIR/lib" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +cd "$SCRIPT_DIR/lib" || { echo "Error: lib directory not found"; exit 1; }This same pattern exists in
nukeBuild/scripts/linux/start.shand should be fixed there as well.nukeBuild/scripts/linux/start.sh-6-7 (1)
6-7:⚠️ Potential issue | 🟡 MinorAdd error handling to the
cdcommand to prevent silent failures.The script continues in the wrong directory if
cd "$SCRIPT_DIR/lib"fails, which can cause subsequent commands to fail or execute in unexpected locations. This is especially critical since this is a template that gets copied to the package root.Suggested change
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -cd "$SCRIPT_DIR/lib" +cd "$SCRIPT_DIR/lib" || { echo "Error: lib directory not found"; exit 1; }Note: The same issue exists in
nukeBuild/scripts/macos/start.shat line 7.build.cmd-1-7 (1)
1-7:⚠️ Potential issue | 🟡 MinorUse CRLF line endings for .cmd files.
The
build.cmdfile currently uses LF-only line endings, which is not the standard for Windows batch files. While modern Windows handles LF in batch scripts, CRLF is the conventional line ending for.cmdfiles and ensures compatibility across Windows environments.nukeBuild/_build.csproj.DotSettings-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorTypo in XML namespace declaration.
The namespace
urn:shemas-jetbrains-com:settings-storage-xamlhas a typo - it should beurn:schemas-jetbrains-com:settings-storage-xaml(missing 'c' in "schemas"). This may prevent ReSharper/Rider from correctly parsing the settings.✏️ Proposed fix
-<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation"> +<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:schemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">build.sh-13-15 (1)
13-15:⚠️ Potential issue | 🟡 MinorExtraneous double slashes in paths.
The paths have double slashes (
//) which, while functionally harmless on Unix, appear to be typos.✏️ Proposed fix
-TEMP_DIRECTORY="$SCRIPT_DIR//.nuke/temp" +TEMP_DIRECTORY="$SCRIPT_DIR/.nuke/temp" -DOTNET_GLOBAL_FILE="$SCRIPT_DIR//global.json" +DOTNET_GLOBAL_FILE="$SCRIPT_DIR/global.json"nukeBuild/scripts/linux/install.sh-136-141 (1)
136-141:⚠️ Potential issue | 🟡 MinorTilde does not expand inside quotes - use
$HOMEinstead.The grep pattern
'~/.dotnet'and the echo command won't work as expected because tilde expansion doesn't occur within quotes. As flagged by shellcheck (SC2088), use$HOMEinstead.✏️ Proposed fix
# Add to PATH if not already there - if ! grep -q '~/.dotnet' ~/.bashrc 2>/dev/null; then - run_cmd "echo 'export PATH=\$PATH:~/.dotnet' >> ~/.bashrc" - run_cmd "export PATH=\$PATH:~/.dotnet" + if ! grep -q "$HOME/.dotnet" ~/.bashrc 2>/dev/null; then + run_cmd "echo 'export PATH=\$PATH:$HOME/.dotnet' >> ~/.bashrc" + run_cmd "export PATH=\$PATH:$HOME/.dotnet" finukeBuild/scripts/linux/install.sh-176-181 (1)
176-181:⚠️ Potential issue | 🟡 MinorSame tilde expansion issue for Node.js PATH update.
✏️ Proposed fix
# Add to PATH - if ! grep -q '~/.local' ~/.bashrc 2>/dev/null; then - run_cmd "echo 'export PATH=\$PATH:~/.local/bin' >> ~/.bashrc" - run_cmd "export PATH=\$PATH:~/.local/bin" + if ! grep -q "$HOME/.local" ~/.bashrc 2>/dev/null; then + run_cmd "echo 'export PATH=\$PATH:$HOME/.local/bin' >> ~/.bashrc" + run_cmd "export PATH=\$PATH:$HOME/.local/bin" finukeBuild/scripts/linux/install.sh-206-217 (1)
206-217:⚠️ Potential issue | 🟡 MinorRedundant command check in NPM installation block.
Line 207 checks
! command -v npm &> /dev/null && ! command -v node &> /dev/null, but the enclosingelseblock at line 195 already guaranteesnpmis not available. The condition should check only fornodeavailability since NPM requires Node.js.✏️ Proposed fix
# Ensure Node.js is available - if ! command -v npm &> /dev/null && ! command -v node &> /dev/null; then + if ! command -v node &> /dev/null; then echo -e "${RED}Error: Node.js not available. Please install Node.js first.${NC}" ((FAILED_COUNT++)) elsenukeBuild/scripts/linux/check.sh-128-140 (1)
128-140:⚠️ Potential issue | 🟡 MinorInstalled count logic may produce incorrect results.
The logic uses
-n "$DOTNET_VERSION"to check if a dependency is installed, butDOTNET_VERSIONis set even when the version is"unknown". This could count a failed version detection as installed.🛡️ Proposed fix to check status instead of version string
Consider tracking installation status separately or checking the actual status from the JSON fragments:
# Calculate installed counts before generating JSON TOTAL_INSTALLED=0 REQUIRED_INSTALLED=0 READY=false -[ -n "$DOTNET_VERSION" ] && TOTAL_INSTALLED=$((TOTAL_INSTALLED + 1)) && REQUIRED_INSTALLED=$((REQUIRED_INSTALLED + 1)) -[ -n "$NODE_VERSION" ] && TOTAL_INSTALLED=$((TOTAL_INSTALLED + 1)) && REQUIRED_INSTALLED=$((REQUIRED_INSTALLED + 1)) -[ -n "$NPM_VERSION" ] && TOTAL_INSTALLED=$((TOTAL_INSTALLED + 1)) && REQUIRED_INSTALLED=$((REQUIRED_INSTALLED + 1)) +# Use command checks instead of version strings +command -v dotnet &>/dev/null && TOTAL_INSTALLED=$((TOTAL_INSTALLED + 1)) && REQUIRED_INSTALLED=$((REQUIRED_INSTALLED + 1)) +command -v node &>/dev/null && TOTAL_INSTALLED=$((TOTAL_INSTALLED + 1)) && REQUIRED_INSTALLED=$((REQUIRED_INSTALLED + 1)) +command -v npm &>/dev/null && TOTAL_INSTALLED=$((TOTAL_INSTALLED + 1)) && REQUIRED_INSTALLED=$((REQUIRED_INSTALLED + 1))build.ps1-17-19 (1)
17-19:⚠️ Potential issue | 🟡 MinorExtraneous double backslashes in paths.
Similar to
build.sh, the paths have double backslashes which appear to be typos.✏️ Proposed fix
-$TempDirectory = "$PSScriptRoot\\.nuke\temp" +$TempDirectory = "$PSScriptRoot\.nuke\temp" -$DotNetGlobalFile = "$PSScriptRoot\\global.json" +$DotNetGlobalFile = "$PSScriptRoot\global.json"nukeBuild/Utils/SemverExtensions.cs-47-83 (1)
47-83:⚠️ Potential issue | 🟡 MinorGuard against null
versionin extension methods.
IsNewerThan,IsNewerOrEqualThan, andGetChannelwill throwNullReferenceExceptionif the receiver is null. Consider explicitArgumentNullExceptionfor consistency with other validation.🧯 Proposed fix
public static bool IsNewerThan(this Semver.SemVersion version, string other) { + if (version is null) + { + throw new ArgumentNullException(nameof(version)); + } if (string.IsNullOrWhiteSpace(other)) { throw new ArgumentNullException(nameof(other), "Version string cannot be null or empty"); } ... } ... public static VersionChannel GetChannel(this Semver.SemVersion version) { + if (version is null) + { + throw new ArgumentNullException(nameof(version)); + } if (!version.PrereleaseIdentifiers.Any()) { return VersionChannel.Stable; } ... }Also applies to: 93-120
nukeBuild/scripts/windows/check.bat-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorConvert file to CRLF to avoid batch parsing issues.
Static analysis flags LF-only line endings, which can cause parsing failures on Windows batch files.
openspec/changes/archive/2026-02-15-refactor-azure-storage-sync-with-nuke/proposal.md-66-75 (1)
66-75:⚠️ Potential issue | 🟡 MinorAdd languages to fenced code blocks (MD040).
Several fenced blocks lack a language identifier; adding one fixes the lint warning and improves readability.
📝 Proposed fix
-``` +```text desktop/ ├── nukeBuild/ │ ├── Build.cs # 主构建脚本 ... -``` +``` -```csharp +```csharp `#region` Azure Blob Storage Parameters ... -``` +``` -```yaml +```yaml # .github/workflows/sync-azure-storage.yml (重构后) ... -``` +```Also applies to: 80-105, 119-177
docs/azure-storage-sync.md-128-140 (1)
128-140:⚠️ Potential issue | 🟡 MinorFix channel naming inconsistency: docs show "dev" but implementation serializes Preview channel as "preview".
The documentation shows a "dev" channel in the JSON examples (line 137) and lists "dev" channel detection rules (line 160), but the C# implementation only has three channels: Stable, Beta, and Preview. The
GetChannelName()method serializesVersionChannel.Previewas"preview", not"dev". WhileParseChannelName()accepts "dev" for backward compatibility (mapping it to Preview), the actual output serialization produces "preview".Additionally, the detection rules in the docs don't match the implementation:
- Docs claim:
-rcmaps to "beta", but implementation:-rcmaps to Preview (serialized as "preview")- Docs claim:
-alphaand-devmap to "dev" channel, but implementation: these map to Preview (serialized as "preview")Update the docs to use "preview" instead of "dev" in the JSON examples and align the detection rules: beta receives
-beta.only; preview receives-rc,-alpha,-dev,-pre,-preview, and other prerelease identifiers.openspec/changes/archive/2026-02-15-fix-github-token-for-azure-blob-publish/proposal.md-8-10 (1)
8-10:⚠️ Potential issue | 🟡 MinorAdd language specifier to code block.
The code block showing the error message should have a language specifier for proper syntax highlighting.
-``` +```text System.Exception: 必须配置 GitHub Token</blockquote></details> <details> <summary>openspec/changes/archive/2026-02-15-refactor-azure-storage-sync-with-nuke/tasks.md-29-29 (1)</summary><blockquote> `29-29`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix broken checkbox markdown syntax.** Line 29 has a malformed checkbox: `- ]` should be `- [ ]`. ```diff - - ] `Azure.Identity` + - [ ] `Azure.Identity`openspec/changes/archive/2026-02-15-desktop-client-channel-support/tasks.md-136-145 (1)
136-145:⚠️ Potential issue | 🟡 MinorAvoid local absolute paths in docs.
/home/newbe36524/...is user-specific and won’t exist for others. Please use a repo-relative path or link to a checked-in example artifact.nukeBuild/scripts/windows/install.ps1-18-19 (1)
18-19:⚠️ Potential issue | 🟡 Minor
-Regionhelp sayscnbut code checks forchina.Passing
-Region cnwon’t use the China registry mirror. Normalize both values (or update the help text).💡 Suggested normalization
- Write-Host " -Region cn|global Region selection (default: global)" + Write-Host " -Region cn|china|global Region selection (default: global)" ... - $NpmRegistryArgs = if ($Region -eq "china") { "--registry=https://registry.npmmirror.com" } else { "" } + $RegionNormalized = $Region.ToLower() + $UseChinaRegistry = $RegionNormalized -in @('cn', 'china') + $NpmRegistryArgs = if ($UseChinaRegistry) { "--registry=https://registry.npmmirror.com" } else { "" }Also applies to: 166-166
openspec/changes/archive/2026-02-15-desktop-client-channel-support/tasks.md-315-318 (1)
315-318:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block (MD040).
📝 Markdown fix
-``` +```textopenspec/changes/archive/2026-02-15-desktop-channel-support/desktop-channel-support/proposal.md-7-22 (1)
7-22:⚠️ Potential issue | 🟡 MinorAdd an explicit “UI Design Changes” section (even if N/A).
The proposal template calls for a UI Design Changes section; if there are no UI impacts, mark it as “N/A” to keep the structure consistent. Based on learnings: Structure proposal.md with Why, What Changes, UI Design Changes (if applicable), Code Flow Changes (if applicable), and Impact sections.
src/main/package-sources/http-index-source.ts-169-184 (1)
169-184:⚠️ Potential issue | 🟡 MinorChannel mapping only updates the first matching version; also fix the forEach lint.
If multiple entries share the same version string (e.g.,
-nortvariants), only the first gets a channel. Building a version→channel map and then assigning in one pass fixes this and removes the callback-return lint.✅ Safer mapping
- if (indexData.channels) { - log.info('[HttpIndexSource] Mapping versions to channels'); - for (const [channelName, channelInfo] of Object.entries(indexData.channels)) { - for (const versionStr of channelInfo.versions) { - const version = versions.find(v => v.version === versionStr); - if (version) { - version.channel = channelName; - } - } - } - } else { - // Backward compatibility: default all versions to 'beta' when no channels specified - log.info('[HttpIndexSource] No channels object found, defaulting all versions to beta'); - versions.forEach(v => v.channel = 'beta'); - } + if (indexData.channels) { + log.info('[HttpIndexSource] Mapping versions to channels'); + const channelByVersion = new Map<string, string>(); + for (const [channelName, channelInfo] of Object.entries(indexData.channels)) { + for (const versionStr of channelInfo.versions) { + channelByVersion.set(versionStr, channelName); + } + } + for (const v of versions) { + const channel = channelByVersion.get(v.version); + if (channel) v.channel = channel; + } + } else { + // Backward compatibility: default all versions to 'beta' when no channels specified + log.info('[HttpIndexSource] No channels object found, defaulting all versions to beta'); + for (const v of versions) { + v.channel = 'beta'; + } + }.github/workflows/sync-azure-storage.yml-56-64 (1)
56-64:⚠️ Potential issue | 🟡 MinorQuote
$GITHUB_OUTPUTto satisfy Shellcheck and avoid potential word-splitting issues in redirections.✅ Safe quoting
- echo "tag=${RELEASE_TAG}" >> $GITHUB_OUTPUT + echo "tag=${RELEASE_TAG}" >> "$GITHUB_OUTPUT".nuke/build.schema.json-87-93 (1)
87-93:⚠️ Potential issue | 🟡 MinorPlaceholder text in Target description.
The description contains
'{default_target}'which appears to be a placeholder that should be replaced with the actual default target name (DefaultorPublishToAzureBlob).📝 Proposed fix
"Target": { "type": "array", - "description": "List of targets to be invoked. Default is '{default_target}'", + "description": "List of targets to be invoked. Default is 'Default'", "items": {openspec/changes/archive/2026-02-15-desktop-client-channel-support/proposal.md-275-288 (1)
275-288:⚠️ Potential issue | 🟡 MinorInvalid JSON syntax in test data example.
The JSON example contains a comment (
// 无 channels 对象) which is not valid JSON syntax. While this is documentation, using invalid JSON examples could cause confusion or copy-paste errors.📝 Proposed fix - use JSONC or remove comment
Either mark it as JSONC:
-```json +```jsonc { "updatedAt": "2026-02-15T05:45:05.2931068Z", "versions": [Or move the comment outside:
} - // 无 channels 对象 }
+注意：此示例不包含
channels对象。</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (21)</summary><blockquote> <details> <summary>nukeBuild/.editorconfig (2)</summary><blockquote> `6-6`: **Consider allowing explicit accessibility modifiers.** Setting `dotnet_style_require_accessibility_modifiers = never:warning` discourages explicit accessibility modifiers, which goes against common C# conventions. Most style guides recommend being explicit about accessibility (e.g., `public`, `private`, `internal`) to improve code clarity and maintainability, especially in collaborative projects. Consider changing to `always:warning` or `omit_if_default:warning` for better alignment with C# best practices. <details> <summary>♻️ Suggested alternative configurations</summary> ```diff -dotnet_style_require_accessibility_modifiers = never:warning +dotnet_style_require_accessibility_modifiers = always:warningOr, if you prefer to allow implicit modifiers for defaults:
-dotnet_style_require_accessibility_modifiers = never:warning +dotnet_style_require_accessibility_modifiers = omit_if_default:warning
8-11: Inconsistent severity for expression-bodied members.Expression-bodied methods are set to
silentwhile properties, indexers, and accessors usewarning. This inconsistency means the IDE will suggest expression-bodied syntax for properties/indexers/accessors but not for methods.If this is intentional (e.g., because methods tend to be more complex), consider adding a comment explaining the rationale. Otherwise, consider standardizing the severity levels.
build.sh (1)
31-33: Declare and assign separately to avoid masking return values.As flagged by shellcheck (SC2155), if
command -v dotnetfails, the failure is masked by the assignment.♻️ Proposed fix
# If dotnet CLI is installed globally and it matches requested version, use for execution if [ -x "$(command -v dotnet)" ] && dotnet --version &>/dev/null; then - export DOTNET_EXE="$(command -v dotnet)" + DOTNET_EXE="$(command -v dotnet)" + export DOTNET_EXE elsebuild.ps1 (1)
68-71: NuGet source removal may fail silently without proper error handling.The
> $nullredirects stdout but not stderr. If the remove command fails for reasons other than "source not found," the error won't be visible.♻️ Proposed improvement
if (Test-Path env:NUKE_ENTERPRISE_TOKEN) { - & $env:DOTNET_EXE nuget remove source "nuke-enterprise" > $null + & $env:DOTNET_EXE nuget remove source "nuke-enterprise" 2>&1 | Out-Null & $env:DOTNET_EXE nuget add source "https://f.feedz.io/nuke/enterprise/nuget" --name "nuke-enterprise" --username "PAT" --password $env:NUKE_ENTERPRISE_TOKEN > $null }nukeBuild/scripts/macos/install.sh (1)
103-112: Avoidevalinrun_cmdto preserve arguments and reduce injection risk.
evalexpands input and can alter argument boundaries. Use"$@"and pass commands as arrays instead.♻️ Suggested refactor (apply to all call sites)
run_cmd() { if [ "$DRY_RUN" = true ]; then echo -e "${YELLOW}[DRY RUN]${NC} $*" else if [ "$VERBOSE" = true ]; then echo -e "${GREEN}Executing:${NC} $*" fi - eval "$@" + "$@" fi }Example call-site change:
-run_cmd "brew install dotnet" +run_cmd brew install dotnetopenspec/changes/archive/2026-02-15-fix-github-token-for-azure-blob-publish/tasks.md (1)
3-8: Consider removing developer-specific local paths.The reference paths like
/home/newbe36524/repos/newbe36524/hagicode-release/are specific to one developer's machine and won't be useful for others. Consider replacing these with:
- Relative paths within the repository
- Links to the actual repository (if it's public)
- Generic descriptions of where to find the reference implementation
nukeBuild/scripts/windows/install.bat (1)
5-5: Document the ExecutionPolicy Bypass usage.Using
-ExecutionPolicy Bypassis acceptable for installer scripts that need to run without user intervention, but consider adding a comment explaining why this is necessary (e.g., "Required to run unsigned scripts without prompting the user").📝 Suggested documentation
`@echo` off REM PCode Dependency Installer - Windows Batch Launcher REM Launches install.ps1 with proper PowerShell arguments +REM Note: ExecutionPolicy Bypass is required to run the installer script without +REM requiring the user to manually change their system execution policy. PowerShell.exe -NoProfile -ExecutionPolicy Bypass -WindowStyle Hidden -File "%~dp0install.ps1" %*src/renderer/store/thunks/packageSourceThunks.ts (1)
376-401: Consider dispatching error state for consistency.Other thunks in this file dispatch
setError(errorMessage)when catching errors (e.g., lines 46, 69-71, 114-115). ThesetChannelPreferencethunk only shows a toast but doesn't update the Redux error state. Consider addingdispatch(setError(errorMessage))for consistency if this error should be visible in the UI state.♻️ Proposed fix for consistency
} catch (error) { const errorMessage = error instanceof Error ? error.message : 'Failed to set channel'; + dispatch(setError(errorMessage)); toast.error('设置失败', { description: errorMessage, }); return false; }.github/workflows/build.yml (1)
43-81: Consider extracting duplicated channel detection logic.The channel detection bash script is duplicated verbatim across 4 jobs (Windows lines 43-81, Linux lines 263-300, macOS lines 388-425, build-summary lines 504-541). This increases maintenance burden and risk of inconsistencies.
Consider extracting this to a composite action or a reusable workflow step.
openspec/specs/ci-cd/spec.md (1)
299-311: Inconsistent casing in scenario keywords.The new scenarios use uppercase keywords (
**WHEN**,**THEN**,**AND**) while existing scenarios in this file use title case (**When**,**Then**,**And**). Consider aligning with the existing convention for consistency.♻️ Example fix for consistent casing
-**WHEN** 触发构建和发布工作流 -**THEN** 自动设置渠道为 `beta` -**AND** 生成的 index.json 中对应版本标记为 `beta` 渠道 +**When** 触发构建和发布工作流 +**Then** 自动设置渠道为 `beta` +**And** 生成的 index.json 中对应版本标记为 `beta` 渠道nukeBuild/scripts/windows/check.ps1 (2)
49-53: Remove or use the unused recommended version variables.The variables
DotNetRecommendedVersion,NodeRecommendedVersion, andNpmRecommendedVersionare assigned but never used in the script. Either remove them or include them in the JSON output if they should be tracked.♻️ Option 1: Remove unused variables
$DotNetMinVersion = if ($EnvVars.DOTNET_MIN_VERSION) { $EnvVars.DOTNET_MIN_VERSION } else { "10.0" } - $DotNetRecommendedVersion = if ($EnvVars.DOTNET_RECOMMENDED_VERSION) { $EnvVars.DOTNET_RECOMMENDED_VERSION } else { "10.0" } $NodeMinVersion = if ($EnvVars.NODE_MIN_VERSION) { $EnvVars.NODE_MIN_VERSION } else { "18.0.0" } - $NodeRecommendedVersion = if ($EnvVars.NODE_RECOMMENDED_VERSION) { $EnvVars.NODE_RECOMMENDED_VERSION } else { "24.12.0" } $NpmMinVersion = if ($EnvVars.NPM_MIN_VERSION) { $EnvVars.NPM_MIN_VERSION } else { "9.0.0" } - $NpmRecommendedVersion = if ($EnvVars.NPM_RECOMMENDED_VERSION) { $EnvVars.NPM_RECOMMENDED_VERSION } else { "10.9.0" }
242-260: Simplify redundant summary calculation.Since all 5 dependencies have
required = $true,$installedCountand$requiredInstalledCountwill always be identical. The separate counting logic (lines 246-256) is redundant.♻️ Simplified calculation
# Calculate summary $installedCount = 0 - $requiredInstalledCount = 0 if ($DotNetStatus.status -eq "installed") { $installedCount++ } if ($NodeStatus.status -eq "installed") { $installedCount++ } if ($NpmStatus.status -eq "installed") { $installedCount++ } if ($ClaudeStatus.status -eq "installed") { $installedCount++ } if ($OpenspecStatus.status -eq "installed") { $installedCount++ } - if ($DotNetStatus.status -eq "installed") { $requiredInstalledCount++ } - if ($NodeStatus.status -eq "installed") { $requiredInstalledCount++ } - if ($NpmStatus.status -eq "installed") { $requiredInstalledCount++ } - if ($ClaudeStatus.status -eq "installed") { $requiredInstalledCount++ } - if ($OpenspecStatus.status -eq "installed") { $requiredInstalledCount++ } $Result.summary.installed = $installedCount - $Result.summary.requiredInstalled = $requiredInstalledCount - $Result.summary.ready = ($requiredInstalledCount -eq 5) + $Result.summary.requiredInstalled = $installedCount # All deps are required + $Result.summary.ready = ($installedCount -eq 5)nukeBuild/BuildPaths.cs (1)
19-75: Consolidate script filename logic to reduce duplication.Several methods repeat the same Windows vs non‑Windows branching and return filenames (not paths). A small helper keeps it DRY and clearer.
♻️ Suggested refactor
internal static class BuildPaths { + private static string GetScriptFileName(string platform, string baseName) + => platform.Equals("windows", StringComparison.OrdinalIgnoreCase) + ? $"{baseName}.bat" + : $"{baseName}.sh"; + internal static string GetInstallScriptPath(string platform) { - return platform.Equals("windows", StringComparison.OrdinalIgnoreCase) ? "install.bat" : "install.sh"; + return GetScriptFileName(platform, "install"); } ... internal static string GetCheckScriptPath(string platform) { - return platform.Equals("windows", StringComparison.OrdinalIgnoreCase) ? "check.bat" : "check.sh"; + return GetScriptFileName(platform, "check"); } ... internal static string GetStartScriptSourcePath(string platform) { - return platform.Equals("windows", StringComparison.OrdinalIgnoreCase) ? "start.bat" : "start.sh"; + return GetScriptFileName(platform, "start"); } ... internal static string GetStartScriptDestFileName(string platform) { - return platform.Equals("windows", StringComparison.OrdinalIgnoreCase) ? "start.bat" : "start.sh"; + return GetScriptFileName(platform, "start"); } ... internal static string GetInstallScriptDestFileName(string platform) { - return platform.Equals("windows", StringComparison.OrdinalIgnoreCase) ? "install.bat" : "install.sh"; + return GetScriptFileName(platform, "install"); } ... internal static string GetCheckScriptDestFileName(string platform) { - return platform.Equals("windows", StringComparison.OrdinalIgnoreCase) ? "check.bat" : "check.sh"; + return GetScriptFileName(platform, "check"); } }nukeBuild/Build.cs (1)
93-102: Minor documentation discrepancy in XML comment.The XML comment at line 96 states "Dependencies: GenerateAzureIndex" but the target being documented is
PublishToAzureBlobwhich depends onGenerateAzureIndex(line 101). The comment reads as ifPublishToAzureBlobis a dependency, rather than having one. Consider rewording for clarity:/// <summary> /// Publish to Azure Blob target /// Uploads pre-generated index.json to Azure Blob Storage -/// Dependencies: GenerateAzureIndex +/// Depends on: GenerateAzureIndex /// Usage: For CI/CD scenarios where index.json is managed independently /// </summary>nukeBuild/AzureStorageConfiguration.cs (2)
2-2: Unused import.
System.Text.Jsonis imported but not used in this file. TheChannelClassificationand other classes don't useJsonSerializer.🧹 Proposed fix
using System.Collections.Generic; -using System.Text.Json; namespace AzureStorage;
103-143: Code duplication withSemverExtensions.GetChannel— consider consolidation or mark as unused.The
ClassifyVersionmethod duplicates channel classification logic that already exists innukeBuild/Utils/SemverExtensions.cs(line 93). Notably,ClassifyVersionis never called in the codebase and appears to be dead code.Key differences between implementations:
Aspect ClassifyVersionSemverExtensions.GetChannelInput type String (manual parsing) SemVersionobject"beta" detection "beta."(dot required)"beta"(case-insensitive, no dot)"dev" patterns Included ( "dev.","dev")Not included The "beta." vs "beta" difference is behavioral: "1.0.0-beta" would return
PreviewinClassifyVersionbutBetainGetChannel.Options:
- If ClassifyVersion is a public API: Keep it but consolidate by delegating to
SemverExtensions.TryParseVersion()andGetChannel(), noting the behavioral change for "beta" without a dot.- If it's internal-only: Remove it as unused code.
Example consolidation:
public static VersionChannel ClassifyVersion(string version) { if (SemverExtensions.TryParseVersion(version, out var semver)) return semver.GetChannel(); return VersionChannel.Stable; }openspec/changes/archive/2026-02-15-desktop-client-channel-support/proposal.md (1)
180-187: Add blank line before table for markdown compliance.The table at line 181 should be surrounded by blank lines per markdown best practices (MD058).
📝 Proposed fix
**文件变更**： + | 文件 | 变更类型 | 描述 | |------|----------|------|.nuke/build.schema.json (1)
33-41: Empty description for Verbosity enum.The
descriptionfield is an empty string. Consider adding a meaningful description.📝 Proposed fix
"Verbosity": { "type": "string", - "description": "", + "description": "Logging verbosity level during build execution", "enum": [nukeBuild/Adapters/AzureBlobAdapter.cs (2)
41-97: Missing retry logic for uploads.
AzureBlobPublishOptions.UploadRetriesis passed but never used inUploadArtifactsAsync. Consider implementing retry logic for transient failures.♻️ Proposed approach
for (int attempt = 1; attempt <= options.UploadRetries; attempt++) { try { await blobClient.UploadAsync(stream, overwrite: true); break; } catch (Azure.RequestFailedException ex) when (attempt < options.UploadRetries && IsTransient(ex)) { Log.Warning("Upload attempt {Attempt} failed, retrying...", attempt); await Task.Delay(TimeSpan.FromSeconds(Math.Pow(2, attempt))); // Exponential backoff stream.Position = 0; // Reset stream for retry } }
254-261: DateTime loses timezone information.Using
.DateTimeloses the offset information. For blob metadata, use.UtcDateTimeto preserve UTC semantics.📝 Proposed fix
blobs.Add(new AzureBlobInfo { Name = blobItem.Name, Size = blobItem.Properties.ContentLength ?? 0, LastModified = blobItem.Properties.LastModified.HasValue - ? blobItem.Properties.LastModified.Value.DateTime + ? blobItem.Properties.LastModified.Value.UtcDateTime : DateTime.MinValue });nukeBuild/scripts/windows/lib/shared.psm1 (1)
40-42: Empty catch block silently swallows errors.The catch block is empty, which can hide issues during troubleshooting. Consider adding a debug/verbose log.
📝 Proposed fix
catch { - # Command not found, return default status + Write-Debug "Command '$CommandName' not found or failed: $($_.Exception.Message)" }
| private readonly AbsolutePath _rootDirectory; | ||
|
|
||
| public GitHubAdapter(AbsolutePath rootDirectory) | ||
| { | ||
| _rootDirectory = rootDirectory; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the GitHubAdapter.cs file
fd -t f "GitHubAdapter.cs"Repository: HagiCode-org/desktop
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Read the GitHubAdapter.cs file to understand the full context
cat -n nukeBuild/Adapters/GitHubAdapter.csRepository: HagiCode-org/desktop
Length of output: 6182
_rootDirectory is unused and UseShellExecute configuration is invalid.
_rootDirectory is assigned but never used. Additionally, the code sets UseShellExecute = true while also attempting to redirect standard streams, which causes an InvalidOperationException at runtime. Both issues must be fixed:
- Set
UseShellExecute = falseto enable stream redirection - Set
WorkingDirectory = _rootDirectoryto ensureghexecutes in the repo context (required when--repoisn't provided) - Add
RedirectStandardError = truefor error logging
📌 Required fix (set UseShellExecute to false and use working directory)
var psi = new ProcessStartInfo
{
FileName = "gh",
Arguments = "release view --json tagName -q .tagName",
RedirectStandardOutput = true,
+ RedirectStandardError = true,
UseShellExecute = false,
CreateNoWindow = true,
+ WorkingDirectory = _rootDirectory
};Applies to lines 20-27, 74-81.
🤖 Prompt for AI Agents
In `@nukeBuild/Adapters/GitHubAdapter.cs` around lines 7 - 12, The field
_rootDirectory is assigned but never used; update the process start
configuration in GitHubAdapter so UseShellExecute = false, set WorkingDirectory
= _rootDirectory (so gh runs in the repo context when --repo isn't provided),
and enable RedirectStandardError = true along with any existing
RedirectStandardOutput/RedirectStandardInput settings; locate these changes
where ProcessStartInfo (or process start configuration) is created in the
GitHubAdapter class (constructor GitHubAdapter and the method that launches the
gh process) and apply the three changes together.
| var psi = new ProcessStartInfo | ||
| { | ||
| FileName = "gh", | ||
| Arguments = "release view --json tagName -q .tagName", | ||
| RedirectStandardOutput = true, | ||
| UseShellExecute = true, | ||
| CreateNoWindow = true | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "GitHubAdapter.cs" --type fRepository: HagiCode-org/desktop
Length of output: 99
🏁 Script executed:
cat -n nukeBuild/Adapters/GitHubAdapter.csRepository: HagiCode-org/desktop
Length of output: 6182
🌐 Web query:
.NET ProcessStartInfo UseShellExecute true RedirectStandardOutput incompatible InvalidOperationException
💡 Result:
ProcessStartInfo.UseShellExecute = true means Windows Shell (ShellExecute) is used to start the process. In that mode, .NET cannot wire up redirected stdin/stdout/stderr pipes, so setting RedirectStandardOutput / RedirectStandardError (or RedirectStandardInput) to true is incompatible and typically results in an InvalidOperationException when you call Process.Start(...). Microsoft’s docs summarize the rule as: to redirect streams, UseShellExecute must be false. [1] (also reiterated in older Microsoft documentation) [2].
Fix options
- If you need redirected output: set
UseShellExecute = false, then read fromprocess.StandardOutput/process.StandardError. [1] - If you must use the shell (e.g.,
Verb="runas"/ file association / “open” a document): you generally cannot capture stdout/stderr directly; use a workaround such as redirecting to a file at the command level or changing the architecture (e.g., run your parent process elevated too). [3]
Example (redirecting works):
var psi = new ProcessStartInfo
{
FileName = "cmd.exe",
Arguments = "/c dir",
UseShellExecute = false,
RedirectStandardOutput = true,
RedirectStandardError = true,
CreateNoWindow = true,
};
using var p = Process.Start(psi)!;
string stdout = p.StandardOutput.ReadToEnd();
string stderr = p.StandardError.ReadToEnd();
p.WaitForExit();Sources:
[1] Microsoft Learn: ProcessStartInfo.UseShellExecute (redirect requires false) [1]
[2] Microsoft (archived) doc noting UseShellExecute must be false to redirect streams [2]
[3] Stack Overflow discussion explaining UseShellExecute=true (often required for elevation via Verb="runas") prevents stdout/stderr redirection [3]
🌐 Web query:
gh CLI release view --json assets -q jq filter expression syntax
💡 Result:
gh release view --json assets -q <expr> uses jq query syntax (because -q is --jq). You must include --json ... for -q/--jq to work. [1][2]
Common jq expressions for assets (an array of objects) [1]:
# Print each asset name (one per line)
gh release view --json assets -q '.assets[].name'
# Print name + download URL (TSV)
gh release view --json assets -q '.assets[] | [.name, .url] | `@tsv`'
# Select one asset by exact name
gh release view --json assets -q '.assets[] | select(.name=="my.zip")'
# Select assets by suffix (e.g., .tar.gz)
gh release view --json assets -q '.assets[] | select(.name|endswith(".tar.gz"))'
# Get the first asset’s download URL
gh release view --json assets -q '.assets[0].url'
# Count assets
gh release view --json assets -q '.assets | length'Notes:
- Quote the jq expression to prevent your shell from interpreting characters like
|,[],*,()etc. [2] - The jq language reference is the standard jq syntax that
--jqexpects. [2][3]
Sources: GitHub CLI gh release view manual [1]; GitHub CLI JSON formatting / --jq docs [2]; jq project/docs [3].
ProcessStartInfo configuration is incompatible and will throw InvalidOperationException at runtime.
UseShellExecute=true cannot be combined with RedirectStandardOutput=true. Additionally, line 39 reads StandardError without setting RedirectStandardError=true. Both methods have the same misconfiguration.
🛠️ Proposed fix (apply to both methods)
var psi = new ProcessStartInfo
{
FileName = "gh",
- Arguments = "release view --json tagName -q .tagName",
+ Arguments = "release view --json tagName -q .tagName", // or adjust for second method
RedirectStandardOutput = true,
+ RedirectStandardError = true,
- UseShellExecute = true,
+ UseShellExecute = false,
CreateNoWindow = true
};Additionally, line 77's gh command is incomplete: -q requires a jq filter expression (e.g., -q '.assets'), not just the flag alone.
Also applies to: 38–39, 74–81
🤖 Prompt for AI Agents
In `@nukeBuild/Adapters/GitHubAdapter.cs` around lines 20 - 27, The
ProcessStartInfo blocks that build the "gh" invocations (the variable psi in
GitHubAdapter.cs) are misconfigured: when you set RedirectStandardOutput = true
you must set UseShellExecute = false, and if you read StandardError you must set
RedirectStandardError = true; update both ProcessStartInfo instances to
UseShellExecute = false and add RedirectStandardError = true. Also fix the gh
arguments that use -q by supplying a valid jq filter (for example replace "-q
.tagName" with "-q '.tagName'" or "-q '.assets'") so the command is complete;
update the Arguments strings used in those ProcessStartInfo objects accordingly.
| using Nuke.Common.IO; | ||
|
|
||
| /// <summary> | ||
| /// Build configuration values | ||
| /// </summary> | ||
| internal static class BuildConfig | ||
| { | ||
| /// <summary> | ||
| /// The release packaged directory path | ||
| /// </summary> | ||
| internal static AbsolutePath ReleasePackagedDirectory => NukeBuild.RootDirectory / "artifacts" / "packages"; |
There was a problem hiding this comment.
Missing using directive for NukeBuild.
The code references NukeBuild.RootDirectory on line 11, but there's no using Nuke.Common; or using static Nuke.Common.NukeBuild; directive. This will cause a compilation error.
🐛 Proposed fix to add missing using directive
using Nuke.Common.IO;
+using Nuke.Common;
/// <summary>
/// Build configuration values📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using Nuke.Common.IO; | |
| /// <summary> | |
| /// Build configuration values | |
| /// </summary> | |
| internal static class BuildConfig | |
| { | |
| /// <summary> | |
| /// The release packaged directory path | |
| /// </summary> | |
| internal static AbsolutePath ReleasePackagedDirectory => NukeBuild.RootDirectory / "artifacts" / "packages"; | |
| using Nuke.Common.IO; | |
| using Nuke.Common; | |
| /// <summary> | |
| /// Build configuration values | |
| /// </summary> | |
| internal static class BuildConfig | |
| { | |
| /// <summary> | |
| /// The release packaged directory path | |
| /// </summary> | |
| internal static AbsolutePath ReleasePackagedDirectory => NukeBuild.RootDirectory / "artifacts" / "packages"; |
🤖 Prompt for AI Agents
In `@nukeBuild/BuildConfig.cs` around lines 1 - 11, The file references
NukeBuild.RootDirectory in BuildConfig but lacks the required using directive;
add the appropriate using so the symbol resolves (for example add "using
Nuke.Common;" or "using static Nuke.Common.NukeBuild;") so that
BuildConfig.AbsolutePath ReleasePackagedDirectory compiles against
NukeBuild.RootDirectory/AbsolutePath.
Summary by CodeRabbit
New Features
Documentation