[larseggert]

• Set ilammy/msvc-dev-cmd arch to amd64_arm64 on ARM64 runners for cross-compilation targeting ARM64
• Install cargo-fuzz from source so it uses the same nightly toolchain as the fuzzing job
• Install cargo-careful from source on non-Linux until binary releases work on macOS/Windows

[Copilot]

Pull request overview

This PR updates the rust composite action to better support Windows ARM64 runners and to make Rust tool installation behave correctly for specific tools that need source builds.

Changes:

• Configure ilammy/msvc-dev-cmd to use amd64_arm64 on Windows ARM64 runners (otherwise amd64).
• Special-case tool installation so cargo-fuzz is installed via cargo install (to compile with the active toolchain) and cargo-careful is installed via cargo install on non-Linux runners.
• Expose runner.os to the install script via RUNNER_OS env var.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Clean, targeted PR. The three changes are well-scoped: ARM64 MSVC configuration, and two justified special-cases for tool installation.

The install loop is starting to accumulate tool-specific branches. If more tools need custom handling in the future, consider extracting this into a small helper script or a lookup table to keep the loop body from growing further — but for two cases this is fine.

See inline comments for minor suggestions on the MSVC arch choice and --locked consistency.

[github-actions]

The rust action changes are well-targeted — ARM64 MSVC setup and the two source-install special-cases are justified and clearly commented.

The main concern is that windows-11-arm is also added to the test-nss matrix, but nss/action.yml has no ARM64 Windows support (msys2 paths, MSVC arch, mingw64 packages). This will likely cause CI failures. See inline comment.

Previous inline feedback on rust/action.yml (redundant RUNNER_OS env, --locked for cargo-careful, native arm64 vs cross-compiler arch) still applies — those comments are on the prior review.

[github-actions]

The rust action changes are well-scoped — the MSVC arch conditional and the two source-install special cases are clearly motivated. Two items need addressing before merge:

1. test-nss matrix will break on ARM64: nss/action.yml has hardcoded x86_64 msys2 paths, x86_64-only pacman packages, and no arch parameter on its own MSVC setup. Adding windows-11-arm to test-nss will fail. Exclude it from that matrix and handle NSS ARM64 in a follow-up.

2. Missing --locked on cargo-careful: Unlike cargo-fuzz (which reasonably omits it due to nightly toolchain churn), cargo-careful has no such constraint. Add --locked for reproducibility, consistent with the generic fallback path.

Minor items (non-blocking):

• The RUNNER_OS env mapping is redundant — it's a default runner variable already available in shell steps.
• Consider using the native arm64 MSVC toolchain instead of the amd64_arm64 cross-compiler, if the runner image supports it.
• The cargo-fuzz and cargo-careful install paths have no CI coverage (test-rust only installs cargo-nextest). A test job exercising these would catch regressions.

[github-actions]

The rust/action.yml changes are in good shape after the fixup commits — native ARM64 MSVC selection (arm64), intentional --locked omission for cargo-fuzz (nightly compatibility), and the redundant RUNNER_OS env mapping removal all look correct.

The nsinstall replacement in nss/action.yml has two issues that need fixing before merge (see inline comment): an unpinned fetch from a mutable Git ref, and $HOME/bin not being on PATH/not being created.

I concur with the prior review feedback that test-nss should exclude windows-11-arm until nss/action.yml gains ARM64 awareness — the hardcoded C:/msys64/mingw64/bin path (x86_64-only), and the ilammy/msvc-dev-cmd step without an arch parameter, will both cause failures on ARM64 runners. The test-rust inclusion is fine since rust/action.yml handles the arch correctly.

Minor: the cargo-fuzz and cargo-careful special-case paths aren't exercised by any CI job (only cargo-nextest is installed in test-rust). Consider adding a matrix entry or dedicated job to smoke-test these installs on at least one platform.

[github-actions]

Summary

Well-scoped PR. The rust/action.yml ARM64 support and tool installation special-cases are clean. The Renovate custom datasource for Firefox tags is correctly wired. The fixup commits addressed most prior review feedback well:

• ✅ Native arm64 MSVC toolchain (not amd64_arm64 cross-compiler)
• ✅ --locked on cargo-careful, intentionally omitted on cargo-fuzz
• ✅ Redundant RUNNER_OS env mapping removed
• ✅ nsinstall pinned to FIREFOX_149_0_2_RELEASE with Renovate automation
• ✅ $HOME/bin created and added to $GITHUB_PATH

Remaining items

Blocking:

1. test-nss on windows-11-arm (see inline comment): nss/action.yml still has x86_64 msys2 assumptions (C:/msys64/mingw64/bin) and it's unclear whether windows-11-arm runners ship msys2. Exclude from test-nss and handle in a follow-up.

2. No integrity check on nsinstall.py fetch (see inline comment): Every other external fetch in this repo is checksummed or SHA-pinned. This downloads executable code without verification. Add a SHA256 check and extend the Renovate packageRules reminder to cover firefox.

Non-blocking (from prior reviews, still valid):

• The cargo-fuzz and cargo-careful special-case install paths have no CI coverage (test-rust only exercises cargo-nextest). Consider a smoke-test in a follow-up.

[github-actions]

Summary

Solid work adding Windows ARM64 support across the Rust and NSS actions, including a well-structured set of NSPR/NSS patches with clear documentation and removal conditions. The patch approach (individual, self-contained files applied at build time) is the right call for upstream-pending changes.

Key observations

Architecture: The 9 NSS/NSPR patches correctly address the full surface area needed for ARM64 MSVC: preprocessor guards (_M_ARM64), compiler intrinsics (Interlocked→_Interlocked), alignment attributes, GCM/AES/SHA crypto paths, and gyp configuration. Each patch is well-documented with upstream removal conditions.

Supply chain: The nsinstall.py fetch is pinned to FIREFOX_149_0_2_RELEASE with Renovate tracking — good. However, per prior review feedback, there's still no SHA256 integrity check on the downloaded executable code, unlike other external fetches in this repo. I concur this should be addressed.

Status of prior review threads

Thread	Status
rust/action.yml arm64 vs amd64_arm64	Addressed — now uses native 'arm64' toolchain
rust/action.yml redundant RUNNER_OS	Addressed — removed from env block
rust/action.yml cargo-careful --locked	Addressed — --locked added
test-nss windows-11-arm exclusion	Partially addressed — nss/action.yml now has full ARM64 support (MSVC arch, patches, nsinstall, gyp-next). The msys64/mingw64/bin concern remains valid (see inline comment)
nsinstall.py SHA256 check	Still open — concur this should be added

[github-actions]

Summary

Well-executed PR adding Windows ARM64 support across both the Rust and NSS composite actions, including a comprehensive set of 9 NSPR/NSS patches. The patch-per-issue structure with clear upstream-removal conditions is good engineering for carrying platform workarounds.

Prior review threads — status

All previously flagged blocking issues have been addressed:

Prior concern	Status
arm64 vs amd64_arm64 MSVC toolchain	Resolved — native arm64 used
Redundant RUNNER_OS env in rust/action.yml	Resolved — removed
cargo-careful --locked	Resolved — added
nsinstall.py unpinned fetch	Resolved — pinned to FIREFOX_149_0_2_RELEASE with Renovate tracking
nsinstall.py missing SHA256 check	Resolved — sha256sum -c added, firefox added to Renovate packageRules
$HOME/bin not on PATH	Resolved — mkdir -p + GITHUB_PATH append
VSPATH fallback :-1	Resolved — uses ${VSINSTALLDIR:?...}
mingw64/bin unconditional on ARM64	Resolved — conditional exclusion
Interlocked intrinsics incomplete	Resolved — all 5 functions redirected
test-nss on windows-11-arm	Resolved — nss/action.yml now has full ARM64 support

New observations (see inline comments)

• sed commands on nspr/configure and Makefile.in are fragile — if upstream changes the matched patterns, the sed silently no-ops and the build produces wrong-architecture output. Adding grep -q guards would make failures loud.
• IS_LITTLE_ENDIAN guard inconsistency in nss-aes-armv8-msvc.patch — the MSVC path omits the endianness check that the GCC/Clang path and the GHASH patch both require.
• /EHsc only on ARM64 in nss-config-gypi-arm64.patch — worth confirming this is intentional if the x64 block doesn't set it.

Still open from prior reviews (non-blocking)

• The cargo-fuzz and cargo-careful source-install paths have no CI coverage (test-rust only installs cargo-nextest). Consider a smoke-test in a follow-up.

No blocking issues identified. The PR is in good shape.

[github-actions]

Summary

Well-executed PR adding Windows ARM64 support across both composite actions and a comprehensive set of 9 NSPR/NSS patches. The patch-per-issue structure with clear upstream-removal conditions is excellent for carrying platform workarounds.

All previously flagged blocking issues from prior reviews have been addressed: native arm64 MSVC toolchain, --locked on cargo-careful, redundant RUNNER_OS removal, nsinstall.py SHA256 verification + Renovate tracking, $HOME/bin PATH setup, VSINSTALLDIR fail-fast via :?, and conditional mingw64/bin exclusion.

New observations

• Cross-patch dependency (see inline): nss-aes-armv8-msvc.patch depends on nss-config-gypi-arm64.patch for __ARM_FEATURE_CRYPTO. The "self-contained and can be removed individually" comment in nss/action.yml should note this exception.
• mozfile dependency (see inline): Missing a brief comment explaining why it's needed.

Still open from prior reviews (non-blocking, concur)

• The cargo-fuzz and cargo-careful source-install paths have no CI coverage (test-rust only exercises cargo-nextest). A smoke-test in a follow-up would catch regressions.
• IS_LITTLE_ENDIAN bypass in the AES patch is safe but inconsistent with the GHASH patch pattern.

No blocking issues. The PR is in good shape.

[github-actions]

Summary

Well-executed PR adding Windows ARM64 support across both composite actions, backed by a comprehensive set of 10 NSPR/NSS patches. The patch-per-issue structure with clear upstream-removal conditions and the use of patch -p1 (which fails loudly on context mismatch, unlike sed) is good engineering for carrying platform workarounds.

Crypto correctness

Verified all 10 patches touching NSPR/NSS crypto code. The changes are exclusively preprocessor guard extensions (_M_ARM64 alongside __aarch64__) and MSVC compatibility shims — no algorithmic changes:

• AES/GCM/GHASH: Guard extensions + __builtin_bswap64 → _byteswap_uint64, __builtin_assume_aligned → no-op (safe on ARM64 — unaligned NEON access is supported), poly64_t cast removal (correct for MSVC's vmull_p64 signature)
• SHA-1/SHA-256: _M_ARM64 exclusion from 64-bit SHA_HW_t (prevents stride mismatch with vld1q_u32), __attribute__((aligned)) → __declspec(align) portability macro
• Interlocked intrinsics: All 5 functions redirected (CompareExchange, Increment, Decrement, Exchange, ExchangeAdd), placed correctly after <windows.h> to shadow function declarations with intrinsic macros
• IS_LITTLE_ENDIAN: Checked consistently on both GCC/Clang and MSVC paths in both aes-armv8.c and ghash-aarch64.c

Prior review threads — resolution status

All previously flagged blocking issues have been addressed in fixup commits:

Prior concern	Status
arm64 vs amd64_arm64 MSVC toolchain	Resolved — native arm64 used
Redundant RUNNER_OS env in rust/action.yml	Resolved — removed
cargo-careful --locked	Resolved — added
nsinstall.py unpinned fetch	Resolved — pinned to FIREFOX_149_0_2_RELEASE with Renovate tracking
nsinstall.py missing SHA256 check	Resolved — sha256sum -c added, firefox added to Renovate packageRules
$HOME/bin not on PATH	Resolved — mkdir -p + GITHUB_PATH append
VSPATH fallback	Resolved — uses ${VSINSTALLDIR:?...} for fail-fast
mingw64/bin unconditional on ARM64	Resolved — conditional exclusion
Interlocked intrinsics incomplete	Resolved — all 5 functions redirected
test-nss on windows-11-arm	Resolved — nss/action.yml now has full ARM64 support
sed fragility	Resolved — replaced with patch -p1 which fails loudly on context mismatch

Non-blocking (concur with prior reviews)

• The cargo-fuzz and cargo-careful source-install paths have no CI coverage (test-rust only exercises cargo-nextest). A smoke-test in a follow-up would catch regressions.

No blocking issues identified. See inline comments for minor suggestions.

[github-actions]

Summary

Solid PR adding Windows ARM64 support across both composite actions, with a well-structured set of 10 NSPR/NSS patches. The patch-per-file approach with patch -p1 (which fails loudly on context mismatch) and clear upstream-removal conditions in each patch header is good engineering for carrying platform workarounds.

Crypto patch correctness

Verified all NSPR/NSS patches — the changes are exclusively preprocessor guard extensions (_M_ARM64 alongside __aarch64__) and MSVC compatibility shims with no algorithmic changes:

• Interlocked intrinsics: All 5 functions redirected, placed correctly after <windows.h> to shadow declarations with intrinsic macros. ARM64 kernel32.lib lacking these as callable symbols is a known platform constraint.
• __builtin_assume_aligned → no-op: Safe — ARM64 NEON supports unaligned access; the hint is a compiler optimization, not a correctness requirement.
• vmull_p64 cast removal (ghash patch): Correct — MSVC's vmull_p64 expects poly64x1_t vectors (returned by vget_low_p64), while GCC/Clang expect poly64_t scalars. Dropping the cast is compatible with both.
• SHA-1 SHA_HW_t exclusion: Correctly prevents 64-bit stride mismatch with vld1q_u32 in the ARM HW SHA-1 path.
• IS_LITTLE_ENDIAN: Checked on MSVC path in both aes-armv8.c and ghash-aarch64.c — consistent.

Prior review threads

All previously flagged blocking issues from the extensive prior review history have been addressed. Concur with the standing non-blocking note that the cargo-fuzz / cargo-careful source-install paths lack CI coverage (test-rust only exercises cargo-nextest).

New observations

See inline comments — a duplicate comment line from editing and a minor curl hardening suggestion. No blocking issues.

[github-actions]

Summary

Well-structured PR adding Windows ARM64 support across both composite actions, backed by 10 focused NSPR/NSS patches. The patch-per-file approach with patch -p1 (fails loudly on context mismatch) and clear upstream-removal conditions in each patch header is the right strategy for carrying platform workarounds.

Independent crypto patch verification

Reviewed all 10 NSPR/NSS patches for correctness. The changes are exclusively preprocessor guard extensions (_M_ARM64 alongside __aarch64__) and MSVC compatibility shims — no algorithmic changes:

• Interlocked intrinsics (nspr-winnt-interlocked-intrinsics.patch): All 5 functions (CompareExchange, Increment, Decrement, Exchange, ExchangeAdd) redirected to _Interlocked* compiler intrinsics via #define, placed correctly after <windows.h> to shadow function declarations.
• GHASH vmull_p64 cast removal (nss-ghash-aarch64-msvc.patch): Correct — vget_low_p64 returns poly64x1_t; GCC/Clang implicitly convert to poly64_t for vmull_p64, while MSVC expects poly64x1_t directly. Dropping the cast is compatible with both.
• SHA-1 SHA_HW_t exclusion (nss-sha-fast-arm64.patch): Correctly prevents 64-bit stride mismatch with vld1q_u32 in the ARM HW SHA-1 path.
• __builtin_assume_aligned → no-op (nss-arm64-gyp-and-aes.patch): Safe — this is a compiler optimization hint, not a correctness requirement. ARM64 NEON supports unaligned access.
• __ARM_FEATURE_CRYPTO via gyp PreprocessorDefinitions: Correct — Windows ARM64 ABI mandates crypto extensions, and blinit.c's CheckARMSupport (patched by nss-blinit-arm64.patch) provides runtime feature detection as an additional safety net.
• IS_LITTLE_ENDIAN: Checked consistently on the MSVC path in both aes-armv8.c and ghash-aarch64.c.
• Coupled patch dependency (nss-arm64-gyp-and-aes.patch): The __ARM_FEATURE_CRYPTO #error guard in aes-armv8.c requires the gyp PreprocessorDefinitions from the same patch — correctly documented as bundled.

Prior review threads

All previously flagged blocking issues have been addressed across fixup commits. Concur with the standing non-blocking observation that the cargo-fuzz and cargo-careful source-install paths in rust/action.yml lack CI coverage (test-rust only exercises cargo-nextest).

See inline comments for two minor suggestions.

[github-actions]

Summary

Well-structured PR adding Windows ARM64 support through 10 isolated NSPR/NSS patches, MSVC toolchain selection, and improved tool installation. The patch-based approach is significantly more maintainable than the earlier sed approach, and each patch file has excellent documentation (problem, fix, removal criteria).

Key improvements since earlier review rounds (addressing prior feedback):

• nsinstall.py pinned to a Firefox release tag with SHA256 verification (supply-chain risk mitigated)
• curl hardened with --proto '=https' --tlsv1.2
• VSPATH uses ${VSINSTALLDIR:?...} for fail-fast on missing env
• ilammy/msvc-dev-cmd arch uses native arm64 instead of amd64_arm64 cross-compiler
• || true added to prevent non-zero exit from the mingw64/bin conditional
• Renovate integration for tracking Firefox releases with manual SHA256 update notes

Remaining observations:

• The nsinstall replacement and gyp-next minimum bump affect all Windows builds, not just ARM64 (see inline comment). This is likely fine but broader than the PR description suggests.
• The cargo-fuzz and cargo-careful special-case install paths in rust/action.yml still lack CI coverage (as noted in a prior review).
• The PR description mentions amd64_arm64 for cross-compilation but the code now uses arm64 (native toolchain) — consider updating the description.

Cryptographic correctness: The NSPR/NSS patches correctly extend __aarch64__ guards to _M_ARM64, add MSVC-compatible intrinsic redirects, and handle the vmull_p64 type signature difference between MSVC and GCC/Clang. The runtime feature detection in blinit.c (IsProcessorFeaturePresent) provides a safety net for the unconditional __ARM_FEATURE_CRYPTO compile-time define.

[github-actions]

Well-executed PR — the 10 NSPR/NSS patches are individually well-documented with clear upstream-removal conditions, the crypto changes are exclusively guard extensions and MSVC compat shims (no algorithmic changes), and all prior review feedback has been addressed.

One blocking issue: the renovate.json change inadvertently removes the == match pattern, which breaks Renovate tracking for gyp-next (see inline comment).

The standing non-blocking observation from prior reviews still applies: the cargo-fuzz and cargo-careful source-install paths in rust/action.yml have no CI coverage (test-rust only installs cargo-nextest).

Minor: the PR description still references amd64_arm64 for cross-compilation, but the code now correctly uses native arm64 — consider updating the description to match.

[github-actions]

Well-structured PR. The 10 NSPR/NSS patches are individually documented with clear upstream-removal conditions, the crypto changes are strictly preprocessor guard extensions and MSVC compat shims (no algorithmic changes), and all prior blocking feedback has been addressed across fixup commits.

Two items remain (see inline comments):

1. renovate.json: The pip == match pattern was dropped when adding the Firefox datasource pattern, breaking Renovate tracking for gyp-next. Concur with the prior review — this needs to be restored before merge.

2. VSPATH workaround removal: The MSVC 18 workaround in the "Set up build environment (Windows)" step was silently removed. If windows-2025 runners carry MSVC 18, x64 NSS builds will regress. Worth confirming this is intentional.

Minor (non-blocking):

• The PR description still references amd64_arm64 for cross-compilation, but the code correctly uses native arm64 — consider updating to match.
• Concur with prior reviews that cargo-fuzz / cargo-careful source-install paths lack CI coverage (test-rust only exercises cargo-nextest).

[github-actions]

The ARM64 Windows patches for NSPR/NSS are well-structured — each patch is narrowly scoped, clearly documented with removal criteria, and the coupled gyp+AES pair is correctly bundled. The rust/action.yml tool installation logic and the nsinstall.py supply-chain hardening (SHA256 check, TLS pinning) are solid.

Blocking: nss-msvc-arm64.patch is referenced in the Build step but does not exist in nss/patches/. This will break all Windows NSS builds since the patch is applied unconditionally (not just ARM64). This appears to be either a missing commit or a rename that wasn't propagated.

Also still open from prior reviews:

• The == Renovate regex pattern was dropped, breaking gyp-next version tracking (see inline comment concurring with prior feedback).
• The VSPATH/GYP workaround for MSVC 18 (VS 2026) was removed — the missing nss-msvc-arm64.patch was presumably its replacement, so fixing the missing patch should resolve this too.

[github-actions]

Summary

Solid and well-structured approach to Windows ARM64 support. The NSS/NSPR patch set is well-documented — each patch file has clear header comments explaining the problem, the fix, and when it can be removed. The coupling between nss-arm64-gyp-and-aes.patch components is correctly called out. The ilammy/msvc-dev-cmd arch selection, nsinstall.py download with SHA256 verification, and the cargo-fuzz/cargo-careful special-casing all look correct.

Key issue

The renovate.json changes inadvertently remove two existing Renovate match patterns (the == pattern and the version+digest pattern) instead of adding the new Firefox pattern alongside them. This silently breaks automated version tracking for gyp-next and cargo-binstall. See inline comment for the fix.

Minor notes

• The nss-msvc-arm64.patch sed-based vc_crt derivation has an off-by-one for VS 2019/2022 (see inline). Not blocking for the current use case (VS 2026 is handled explicitly, and the static build doesn't use WIN32_REDIST_DIR).
• The DEBUG: echo statements in the ARM64 build path should be removed or commented out before merge — they'll appear in every downstream consumer's CI logs.

[Copilot]

Copilot review overview

🟡 Changes recommended

The MSVC architecture input and redistributable mappings can prevent the new Windows ARM64 builds from configuring successfully.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 · 1

Open (4)

• Configure ARM64 NSS with the supported MSVC toolchain target · New
• Avoid x86/x64 filtering when discovering ARM-only installations · New
• Use amd64_arm64 for the ARM64 MSVC target · New
• Correct the Visual Studio 2026 and VC145 rationale · New

[github-actions]

No whole-change observations.

[github-actions]

I found nothing worth an inline comment in this pull request. Every finding was either raised in an earlier review (the unpinned mozfile, cargo-fuzz without --locked, arm64 vs amd64_arm64, the msys2 layout on ARM64) or had no concrete failure scenario. I dropped one possible issue because I couldn't check it: Renovate's versioning=loose may not parse the FIREFOX_149_0_2_RELEASE tag. My request to fetch the Renovate docs was denied.

[Copilot]

🟡 Changes recommended

MSVC uses the wrong ARM64 architecture selector, and cargo-fuzz is not guaranteed to use the requested nightly toolchain.

1 open finding

• Pass the requested nightly version explicitly to cargo · New

4 resolved since last review

• Use amd64_arm64 for the ARM64 MSVC target
• Avoid x86/x64 filtering when discovering ARM-only installations
• Configure ARM64 NSS with the supported MSVC toolchain target
• Correct the Visual Studio 2026 and VC145 rationale

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[github-actions]

I found nothing worth an inline comment: every candidate finding either repeats an earlier review (cargo-fuzz without --locked, msys2/mingw64 on windows-11-arm, $HOME/bin on PATH) or has no concrete failure scenario I could verify.

[Copilot]

🟡 Changes recommended

The MSVC architecture value does not select the required amd64_arm64 cross-compilation environment, and new tool branches lack coverage.

1 open finding

• Add tests for cargo-fuzz and non-Linux cargo-careful · New

1 resolved since last review

• Pass the requested nightly version explicitly to cargo

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[Copilot]

🔵 Needs a closer look

Both MSVC setup steps use an unsupported ARM64 architecture value instead of amd64_arm64.

0 open findings

1 resolved since last review

• Add tests for cargo-fuzz and non-Linux cargo-careful

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[github-actions]

I found nothing worth an inline comment. The code-review pass came back with ten candidates, and none of them survived filtering. Some repeated points earlier reviews had already raised: $HOME/bin on PATH, dropping pacman python3-pip, and cargo-fuzz without --locked. The rest either depended on what the windows-11-arm image has preinstalled, which I couldn't check, or had no concrete failure scenario.