Add unsigned Windows reproducibility proofs#563
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds pinned VC++/WiX helpers, a PowerShell DLL-staging script, a Windows CI orchestration script, workflow updates to invoke it and upload .exe/.dll artifacts, sccache environment adjustments, and extends artifact verification to validate Windows reproducibility proofs. ChangesWindows Desktop PR Build and Artifact Verification
Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Actions (.github/workflows/desktop-pr-build.yml)
participant Script as scripts/ci/desktop-windows-pr.sh
participant OrtSetup as scripts/provide-windows-onnxruntime.sh
participant DllStaging as frontend/src-tauri/scripts/stage-windows-runtime-dlls.ps1
participant Tauri as bun tauri build
participant VerifyArtifacts as scripts/ci/verify-release-artifacts.sh
Workflow->>Script: invoke Windows PR build step
Script->>OrtSetup: prepare ONNX runtime (export ORT_* values)
Script->>DllStaging: stage VC redist and copy onnxruntime.dll into resources/windows
Script->>Tauri: build frontend dist and unsigned Windows bundle
Script->>Script: collect .exe and .dll artifacts, generate SHA-256 manifests
Workflow->>VerifyArtifacts: download artifacts and run verifier (windows)
VerifyArtifacts->>Workflow: confirm final and runtime DLL proof verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Deploying maple with
|
| Latest commit: |
73f4453
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c1bb27c6.maple-ca8.pages.dev |
| Branch Preview URL: | https://fix-windows-reproducibility.maple-ca8.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
AnthonyRonning
force-pushed
the
fix/windows-reproducibility
branch
from
6272646 to
c6661e9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/ci/desktop-windows-pr.sh (1)
79-98: 💤 Low valueOptional: guard against empty artifact/DLL sets before writing the proof manifests.
findruns inside a process substitution, so a missingbundle/nsistree (or zero matches) is silently swallowed and yields an empty array. The resulting*.sha256manifests would then be empty proofs rather than failing loudly. Adding a non-empty check makes the reproducibility proof self-validating.♻️ Proposed guard
+if [ "${`#windows_artifacts`[@]}" -eq 0 ]; then + echo "No Windows .exe artifacts found under target/release/bundle/nsis" >&2 + exit 1 +fi +if [ "${`#windows_runtime_dlls`[@]}" -eq 0 ]; then + echo "No staged runtime DLLs found under resources/windows" >&2 + exit 1 +fi write_sha256_manifest "${repro_dir}/desktop-pr-windows-final.sha256" "${windows_artifacts[@]}" write_sha256_manifest "${repro_dir}/desktop-pr-windows-runtime-dlls.sha256" "${windows_runtime_dlls[@]}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/desktop-windows-pr.sh` around lines 79 - 98, The script currently calls write_sha256_manifest and print_file_hashes unconditionally, which can produce empty proof files when windows_artifacts or windows_runtime_dlls are empty; add guards that check the arrays (windows_artifacts and windows_runtime_dlls) are non-empty before calling write_sha256_manifest and print_file_hashes and, if empty, emit a clear error message (including the variable name and TAURI_DIR context) and exit non‑zero to fail the job; implement these checks right before the write_sha256_manifest calls so write_sha256_manifest and print_file_hashes are only invoked for non-empty sets while leaving verify_frontend_dist_unchanged unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/ci/desktop-windows-pr.sh`:
- Around line 79-98: The script currently calls write_sha256_manifest and
print_file_hashes unconditionally, which can produce empty proof files when
windows_artifacts or windows_runtime_dlls are empty; add guards that check the
arrays (windows_artifacts and windows_runtime_dlls) are non-empty before calling
write_sha256_manifest and print_file_hashes and, if empty, emit a clear error
message (including the variable name and TAURI_DIR context) and exit non‑zero to
fail the job; implement these checks right before the write_sha256_manifest
calls so write_sha256_manifest and print_file_hashes are only invoked for
non-empty sets while leaving verify_frontend_dist_unchanged unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 701b5da5-a2d8-4a8f-8330-686c00da4b1d
📒 Files selected for processing (7)
.github/workflows/desktop-pr-build.ymlfrontend/src-tauri/resources/windows/README.mdfrontend/src-tauri/scripts/onnxruntime-pins.shfrontend/src-tauri/scripts/stage-windows-runtime-dlls.ps1scripts/ci/_common.shscripts/ci/desktop-windows-pr.shscripts/ci/verify-release-artifacts.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src-tauri/resources/windows/README.md
- .github/workflows/desktop-pr-build.yml
- scripts/ci/_common.sh
- scripts/ci/verify-release-artifacts.sh
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
AnthonyRonning
force-pushed
the
fix/windows-reproducibility
branch
from
c6661e9 to
20f01a2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
AnthonyRonning
force-pushed
the
fix/windows-reproducibility
branch
2 times, most recently
from
de911a1 to
3f160b8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
frontend/src-tauri/scripts/stage-windows-runtime-dlls.ps1 (2)
307-323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear previously staged DLLs before copying the new set.
desktop-windows-pr.shhashes everyresources/windows/*.dll, so any leftover DLL from an earlier run becomes part of the uploaded and verified proof set. Remove existing*.dllfiles in$Destinationbefore staging the new runtime set.Suggested fix
New-Item -ItemType Directory -Force -Path $Destination, $redistDir | Out-Null +Get-ChildItem -LiteralPath $Destination -Filter "*.dll" -File -ErrorAction SilentlyContinue | + Remove-Item -Force if (!(Test-Path -LiteralPath $redistExe)) { Invoke-Download -Url $vcRedistUrl -OutFile $redistExe }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src-tauri/scripts/stage-windows-runtime-dlls.ps1` around lines 307 - 323, Before copying the new runtime DLLs, delete any existing DLLs in the target folder to avoid stale files being included in the uploaded hash set: add a step in stage-windows-runtime-dlls.ps1 that removes existing *.dll entries from $Destination (use the $Destination variable and wildcard *.dll) prior to the Copy-Item of $OrtDllPath and the foreach loop over $dllNames so the directory contains only the freshly staged runtime DLLs.
219-246:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstrain DLL resolution to x64 payloads.
This still returns the first name/metadata match across every extracted root, so
vc_redist.x64.execan stage an ARM64 runtime DLL when both architectures are present. Filter candidates by PE machine type, or by a known x64 subtree, before both return paths.Suggested fix
+function Test-PortableExecutableMachineAmd64 { + param([Parameter(Mandatory = $true)][string] $Path) + + $stream = [IO.File]::OpenRead($Path) + try { + $reader = New-Object IO.BinaryReader($stream) + if ($reader.ReadUInt16() -ne 0x5A4D) { + return $false + } + + $stream.Position = 0x3C + $peOffset = $reader.ReadInt32() + $stream.Position = $peOffset + 4 + return $reader.ReadUInt16() -eq 0x8664 + } finally { + $stream.Dispose() + } +} + $match = $files | - Where-Object { $_.Name -ieq $Name } | + Where-Object { ($_.Name -ieq $Name) -and (Test-PortableExecutableMachineAmd64 -Path $_.FullName) } | Sort-Object FullName | Select-Object -First 1 ... if ( - ($versionInfo.OriginalFilename -ieq $Name) -or - ($versionInfo.InternalName -ieq $Name) -or - ($versionInfo.FileDescription -ieq $Name) + ( + ($versionInfo.OriginalFilename -ieq $Name) -or + ($versionInfo.InternalName -ieq $Name) -or + ($versionInfo.FileDescription -ieq $Name) + ) -and + (Test-PortableExecutableMachineAmd64 -Path $file.FullName) ) { Write-Host ("resolved-windows-runtime-dll {0} {1}" -f $Name, $file.FullName) return $file.FullName🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src-tauri/scripts/stage-windows-runtime-dlls.ps1` around lines 219 - 246, The current resolver picks the first filename/metadata match from $files (via $match and the subsequent foreach over $file/ $versionInfo) without ensuring the binary is x64; update the logic to pre-filter candidates to x64-only before any early-return. Constrain the search by filtering $files to an x64 subtree (e.g. paths containing an x64 folder) or inspect each candidate's PE machine type (when iterating $file, validate the PE header indicates IMAGE_FILE_MACHINE_AMD64) and only consider those for the initial $match check and the metadata-based return paths that call return $file.FullName.scripts/ci/desktop-windows-pr.sh (1)
55-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConvert
ORT_DYLIB_PATHbefore passing it to PowerShell.If
provide-windows-onnxruntime.shemits an MSYS path like/c/...,Copy-Item -LiteralPathinstage-windows-runtime-dlls.ps1can fail before any runtime manifest is produced. Pass the DLL path throughto_windows_pathhere too.Suggested fix
pwsh -NoLogo -NoProfile -ExecutionPolicy Bypass \ -File "$(to_windows_path "${TAURI_DIR}/scripts/stage-windows-runtime-dlls.ps1")" \ - -OrtDllPath "${ORT_DYLIB_PATH:?ORT_DYLIB_PATH is required}" \ + -OrtDllPath "$(to_windows_path "${ORT_DYLIB_PATH:?ORT_DYLIB_PATH is required}")" \ -Destination "$(to_windows_path "${TAURI_DIR}/resources/windows")"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/desktop-windows-pr.sh` around lines 55 - 58, The pwsh call is passing ORT_DYLIB_PATH as a POSIX/MSYS path which can break Copy-Item in stage-windows-runtime-dlls.ps1; wrap the variable with to_windows_path before passing it to PowerShell so the DLL path is converted to a Windows-style path. Update the pwsh invocation argument -OrtDllPath "${ORT_DYLIB_PATH:?ORT_DYLIB_PATH is required}" to use to_windows_path (i.e. call to_windows_path on ORT_DYLIB_PATH) so the script stage-windows-runtime-dlls.ps1 receives a proper Windows path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@frontend/src-tauri/scripts/stage-windows-runtime-dlls.ps1`:
- Around line 307-323: Before copying the new runtime DLLs, delete any existing
DLLs in the target folder to avoid stale files being included in the uploaded
hash set: add a step in stage-windows-runtime-dlls.ps1 that removes existing
*.dll entries from $Destination (use the $Destination variable and wildcard
*.dll) prior to the Copy-Item of $OrtDllPath and the foreach loop over $dllNames
so the directory contains only the freshly staged runtime DLLs.
- Around line 219-246: The current resolver picks the first filename/metadata
match from $files (via $match and the subsequent foreach over $file/
$versionInfo) without ensuring the binary is x64; update the logic to pre-filter
candidates to x64-only before any early-return. Constrain the search by
filtering $files to an x64 subtree (e.g. paths containing an x64 folder) or
inspect each candidate's PE machine type (when iterating $file, validate the PE
header indicates IMAGE_FILE_MACHINE_AMD64) and only consider those for the
initial $match check and the metadata-based return paths that call return
$file.FullName.
In `@scripts/ci/desktop-windows-pr.sh`:
- Around line 55-58: The pwsh call is passing ORT_DYLIB_PATH as a POSIX/MSYS
path which can break Copy-Item in stage-windows-runtime-dlls.ps1; wrap the
variable with to_windows_path before passing it to PowerShell so the DLL path is
converted to a Windows-style path. Update the pwsh invocation argument
-OrtDllPath "${ORT_DYLIB_PATH:?ORT_DYLIB_PATH is required}" to use
to_windows_path (i.e. call to_windows_path on ORT_DYLIB_PATH) so the script
stage-windows-runtime-dlls.ps1 receives a proper Windows path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a260229-9b46-4b45-be1a-59a0e99583fa
📒 Files selected for processing (7)
.github/workflows/desktop-pr-build.ymlfrontend/src-tauri/resources/windows/README.mdfrontend/src-tauri/scripts/onnxruntime-pins.shfrontend/src-tauri/scripts/stage-windows-runtime-dlls.ps1scripts/ci/_common.shscripts/ci/desktop-windows-pr.shscripts/ci/verify-release-artifacts.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/ci/verify-release-artifacts.sh
- frontend/src-tauri/scripts/onnxruntime-pins.sh
- frontend/src-tauri/resources/windows/README.md
- .github/workflows/desktop-pr-build.yml
- scripts/ci/_common.sh
AnthonyRonning
force-pushed
the
fix/windows-reproducibility
branch
from
3f160b8 to
d84d200
Compare
AnthonyRonning
force-pushed
the
fix/windows-reproducibility
branch
from
d84d200 to
73f4453
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Scope
This handles unsigned Windows PR builds only. Windows master/release signing is intentionally left for a later phase.
Verification
Summary by CodeRabbit
Documentation
Chores