[Alexandre Zollinger Chohfi (azchohfi)]

Problem

GitHub.Copilot.SDK's build targets download a native copilot.exe via an MSBuild <DownloadFile>, hard-defaulting to registry.npmjs.org. On a network that blocks that host, the download fails the build:

GitHub.Copilot.SDK.targets(108,5): error MSB3923: Failed to download file
"https://registry.npmjs.org/@github/copilot-win32-x64/-/copilot-win32-x64-1.0.79.tgz".
The SSL connection could not be established ... TLS alert: 'HandshakeFailure'.

That takes out every entry point building src/Reactor.Cli or the two samples referencing the SDK — including the command the docs freshness gate in ci.yml tells contributors to run by hand:

dotnet run --project src/Reactor.Cli -- docs compile --no-screenshots

bootstrap.ps1 already solved this for its own restores via Resolve-ReactorNpmRegistry, but a plain dotnet build / dotnet run / dotnet test never passes through bootstrap, so the same developer stayed blocked.

Approach

tools/CopilotNpmRegistry.props (imported by Directory.Build.props) applies that existing policy at MSBuild evaluation time. Every documented command then works unmodified on both networks — no new script, no new flag, no CI change.

A wrapper script was the alternative. It was rejected because it would fix only docs compile, would require changing the CI failure message, and would leave dotnet build / dotnet test / the samples broken.

Scenario	CopilotNpmRegistryUrl
Mirror already in ~/.npmrc / NPM_CONFIG_REGISTRY	the mirror
Nothing configured (public contributor)	https://registry.npmjs.org — unchanged
CI (CI=true)	irrelevant; CopilotSkipCliDownload=true already skips the download

Contributors on an unrestricted network configure none of this, match nothing, and keep the SDK's default.

Policy

Mirrors Resolve-ReactorNpmRegistry: NPM_CONFIG_REGISTRY short-circuits the user .npmrc; within the .npmrc the last registry= assignment wins and surrounding quotes are stripped; the value must be https, on the allow-listed host, with no userinfo, query, or fragment. Scheme and host compare case-insensitively and an explicit port is allowed, because the resolver compares already-normalized System.Uri.Scheme/.Host.

Two properties are load-bearing and easy to regress:

• The host must be followed by / or the end of the value. An unconstrained tail lets packagefeedproxy.microsoft.io.evil.example match its own prefix, handing a native-binary download to an attacker-controlled host. Once a port is permitted, that same anchor is also what stops host:443@evil.example smuggling in a foreign host via userinfo.
• Query and fragment are rejected. A registry URL can legitimately carry a token in its query string, and a user .npmrc is mostly _password= feed tokens. The .npmrc read is nested inside the Regex.Match argument so its contents never become a named property — which would publish them into every shared binlog.

Testing

tests/build_props/ci/CopilotNpmRegistry.Tests.ps1 — 59 assertions:

• evaluates the props directly against synthetic npm config;
• probes through the real Directory.Build.props, so deleting the import fails the suite;
• asserts no credential from .npmrc reaches a -v:diag log, with a positive control;
• a differential suite feeds one .npmrc corpus to both this file and Resolve-ReactorNpmRegistry and requires them to agree.

CI never exercises any of this — CI=true makes the props inert — so without the suite the file could rot unnoticed. Hence the dedicated path-filtered workflow.

Five mutations confirmed the oracles are live: relaxing the host anchor, dropping last-assignment-wins, re-allowing query/fragment, removing case/port handling, and hoisting the .npmrc read into a named property each reddened exactly the assertions that claim to cover them.

Verified end-to-end on a clean tree: the failing command above now completes, fetching the tarball through the mirror.

Also

Corrects the neighbouring CopilotSkipCliDownload comment, which claimed the native binary was only needed by the demo-script-tool sample. mur loc translate starts it too (CopilotTranslationProvider → new CopilotClient()), so a local blanket skip breaks the shipped CLI.

Note for reviewers

While tracing that comment I noticed a possible pre-existing tension, not touched by this PR: Directory.Build.props skips the download whenever CI=true, yet docs/security/threat-model.md states the binary is present in the released mur publish artifact. Those can't both hold unless the release path overrides the skip. Might be worth a separate look.

[github-actions]

📦 Build metrics

Artifact sizes for d28c5c5 vs the base branch (c730e4f).

Packages (compressed .nupkg)

Artifact	base	PR	Δ
Microsoft.UI.Reactor.nupkg	1.70 MB	1.70 MB	+3 B (0.00%)	≈
Microsoft.UI.Reactor.Advanced.nupkg	487.6 KB	487.6 KB	-8 B (0.00%)	≈
Microsoft.UI.Reactor.Devtools.nupkg	283.3 KB	283.3 KB	-1 B (0.00%)	≈

Assemblies in Microsoft.UI.Reactor

Artifact	base	PR	Δ
Reactor.Analyzers.dll	373.0 KB	373.0 KB	+0 B (0.00%)	≈
Reactor.dll	2.43 MB	2.43 MB	+0 B (0.00%)	≈
Reactor.Localization.Generator.dll	16.0 KB	16.0 KB	+0 B (0.00%)	≈
Reactor.Wrappers.Abstractions.dll	10.5 KB	10.5 KB	+0 B (0.00%)	≈
Reactor.Wrappers.Generator.dll	99.5 KB	99.5 KB	+0 B (0.00%)	≈

Assemblies in Microsoft.UI.Reactor.Advanced

Artifact	base	PR	Δ
Reactor.Advanced.dll	1021.5 KB	1021.5 KB	+0 B (0.00%)	≈

Assemblies in Microsoft.UI.Reactor.Devtools

Artifact	base	PR	Δ
Microsoft.UI.Reactor.Devtools.dll	784.5 KB	784.5 KB	+0 B (0.00%)	≈

No size change beyond the noise floor. ✅

_{✅ smaller / ⚠️ larger / ≈ within noise. Sizes come from a Release dotnet pack on the CI runner: packages are the compressed .nupkg download size, assemblies the uncompressed DLL inside it.
workflow run.}

[Copilot]

🟡 Changes recommended

The resolver has a critical regex failure plus parity, path, port-validation, CI-filter, and changelog issues.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds MSBuild-time npm mirror resolution for Copilot SDK downloads, enabling standard .NET commands on restricted networks.

Changes:

• Resolves and validates npm mirrors during MSBuild evaluation.
• Adds parity, security, integration, and CI test coverage.
• Documents mirror behavior and updates Copilot download guidance.

File summaries

File	Summary	Findings
tools/CopilotNpmRegistry.props	Resolves allowed npm mirrors during MSBuild evaluation.	Critical (3 votes): invalid variable-length regex lookbehind causes evaluation failure with existing .npmrc files. Moderate: whitespace environment parity (1), apostrophe path escaping (1), and invalid-port validation (2).
tests/build_props/ci/CopilotNpmRegistry.Tests.ps1	Tests parsing, security, integration, and resolver parity.	No findings.
README.md	Documents automatic mirror resolution and CLI download behavior.	Nit (3 votes): add a corresponding [Unreleased] changelog entry with an issue/spec reference.
Directory.Build.props	Imports the new props and updates Copilot download guidance.	No findings.
.github/workflows/build-props-lib-tests.yml	Runs dedicated Windows build-props tests.	Moderate (3 votes): include the resolver path in both push and pull-request filters.

Review details

Suppressed comments (2)

tools/CopilotNpmRegistry.props:104

• The reference resolver treats a whitespace NPM_CONFIG_REGISTRY as unset (tools/BootstrapFeedResolver.ps1:31), but this branch checks only for the empty string. With NPM_CONFIG_REGISTRY=' ' and a mirror in the user config, the props skips the .npmrc read and leaves CopilotNpmRegistryUrl empty, so plain builds diverge from bootstrap and still use the blocked public registry. Use the same null/whitespace semantics and add an environment-parity case.

    <CopilotNpmRegistryUrl Condition="'$(CopilotNpmRegistryUrl)' == ''
                                      and '$(NPM_CONFIG_REGISTRY)' == ''
                                      and Exists('$(_ReactorNpmUserConfig)')">$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_ReactorNpmUserConfig)')), '(?m)(?&lt;=^[ \t]*registry[ \t]*=[ \t]*["\x27]?)(?i:https://packagefeedproxy\.microsoft\.io)(?::\d+)?(?:/[^\s?#"\x27]*)?(?=["\x27]?[ \t]*\r?$)(?![\s\S]*^[ \t]*registry[ \t]*=)'))</CopilotNpmRegistryUrl>

tools/CopilotNpmRegistry.props:104

• After the regex is fixed, this path interpolation still breaks valid user configurations whose NPM_CONFIG_USERCONFIG contains an apostrophe: the value is inserted into ReadAllText('...') without escaping, producing malformed MSBuild property-function syntax (for example C:\Users\O'Connor\.npmrc). Read the path through an escaping-safe argument mechanism and add a regression case for a quoted path, otherwise those users still cannot use automatic mirror detection.

                                      and Exists('$(_ReactorNpmUserConfig)')">$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_ReactorNpmUserConfig)')), '(?m)(?&lt;=^[ \t]*registry[ \t]*=[ \t]*["\x27]?)(?i:https://packagefeedproxy\.microsoft\.io)(?::\d+)?(?:/[^\s?#"\x27]*)?(?=["\x27]?[ \t]*\r?$)(?![\s\S]*^[ \t]*registry[ \t]*=)'))</CopilotNpmRegistryUrl>

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved resolver parity and path-handling issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

tools/CopilotNpmRegistry.props:103

• Bootstrap's fallback is $env:USERPROFILE\.npmrc (tools/BootstrapFeedResolver.ps1:17,36), but this uses SpecialFolder.UserProfile. When USERPROFILE is redirected, as the new test harness explicitly notes, bootstrap/npm and plain MSBuild inspect different files; a configured mirror is therefore silently missed by this path. Use the same USERPROFILE-based fallback or remove the parity claim and test the intentional divergence.
  tools/CopilotNpmRegistry.props:105
• Bootstrap trims the configured value before URI validation (tools/BootstrapFeedResolver.ps1:7) and returns the trimmed value (:55). This branch matches the raw $(NPM_CONFIG_REGISTRY), while the next branch is disabled whenever .Trim() is nonempty, so NPM_CONFIG_REGISTRY=' https://packagefeedproxy.microsoft.io/npm/ ' is accepted by bootstrap but leaves this property empty and still uses the public registry. Match against $(NPM_CONFIG_REGISTRY.Trim()) and add that parity case.

tools/CopilotNpmRegistry.props:108

• The reference resolver's -match is case-insensitive (tools/BootstrapFeedResolver.ps1:41-45), but this pattern only applies case-insensitivity to the scheme/host; both literal registry tokens remain lowercase. A Registry=https://packagefeedproxy.microsoft.io/npm/ assignment is therefore selected by bootstrap but ignored here, leaving plain builds on the public default and breaking the claimed differential parity. Make both key matches case-insensitive and add this form to the corpus.

                                      and Exists('$(_ReactorNpmUserConfig)')">$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_ReactorNpmUserConfig)')), '(?m)(?&lt;=^[ \t]*registry[ \t]*=[ \t]*["\x27]?)(?i:https://packagefeedproxy\.microsoft\.io)(?::(?:6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{4}|\d{1,4}))?(?:/[^\s?#"\x27]*)?(?=["\x27]?[ \t]*\r?$)(?![\s\S]*^[ \t]*registry[ \t]*=)'))</CopilotNpmRegistryUrl>

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[github-actions]

🧪 Merged coverage

Coverage for d28c5c5 vs the base branch (6c9fdfe) — unit + selftest merged.

Metric	base	PR	Δ
Line	85.77%	85.76%	-0.01 pp	≈
Branch	77.63% (958/1234)	77.55% (957/1234)	-0.08 pp	≈

No coverage change beyond the noise floor. ✅

_{✅ higher / ⚠️ lower / ≈ within noise. Δ is in percentage points; coverage is unit + selftest merged (Debug x64) on the CI runner. Cobertura reports attached to the workflow run as artifacts.}

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate correctness and parity issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

tools/CopilotNpmRegistry.props:112

• This port-range regex rejects valid zero-padded ports such as https://packagefeedproxy.microsoft.io:00080/npm/: Uri.TryCreate accepts that as port 80, so Resolve-ReactorNpmRegistry selects the mirror while this MSBuild path silently leaves the public default. The same pattern is duplicated in the .npmrc branch; validate with URI parsing or otherwise normalize leading zeros, and add a parity case.

Directory.Build.props:134

• This hunk corrects the sample-only explanation, but the same stale claim remains in build/pipelines/OneBranch.CI.Reactor.yml:39-44 and build/pipelines/OneBranch.Official.Reactor.yml:60-64, where the native binary is still documented as needed only by demo-script-tool. Those comments now contradict the three-project list here and can continue to mislead maintainers about the shipped CLI's runtime dependency; update the duplicated pipeline comments with this correction.

    "Runtime artifact" is NOT the same as "sample-only", so do not reach for
    CopilotSkipCliDownload=true as a local fix for a failing download. Three projects
    reference the SDK, and the first is the shipped CLI, not a sample:

tests/build_props/ci/CopilotNpmRegistry.Tests.ps1:344

• The same Environment.GetFolderPath('UserProfile') assumption makes this default-path assertion fail when USERPROFILE is intentionally redirected, even though the props is supposed to follow that environment variable. Compute $expectedDefault from $env:USERPROFILE so this test matches the behavior it documents.

    $expectedDefault = Join-Path ([Environment]::GetFolderPath('UserProfile')) '.npmrc'
    Assert-Equal $expectedDefault (Invoke-ProbeProperty -Property 'USERCONFIG') `
        'with NPM_CONFIG_USERCONFIG unset the user config path falls back to ~/.npmrc'

tools/CopilotNpmRegistry.props:115

• The last-assignment lookahead treats any later registry= line as an override, including a blank assignment. Resolve-ReactorNpmRegistry only updates on a non-whitespace value (tools/BootstrapFeedResolver.ps1:42-45), so an .npmrc containing registry=<mirror> followed by registry= resolves to the mirror in bootstrap but to an empty value here. Make the lookahead require a non-whitespace value and add that case to the differential corpus.

                                      and Exists('$(_ReactorNpmUserConfig)')">$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_ReactorNpmUserConfig)')), '(?m)(?&lt;=^[ \t]*(?i:registry)[ \t]*=[ \t]*["\x27]?)(?i:https://packagefeedproxy\.microsoft\.io)(?::(?:6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{4}|\d{1,4}))?(?:/[^\s?#"\x27]*)?(?=["\x27]?[ \t]*\r?$)(?![\s\S]*^[ \t]*(?i:registry)[ \t]*=)'))</CopilotNpmRegistryUrl>

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The mirror parser does not match the reference resolver for valid registry URLs containing apostrophes in the path.

Review details

Suppressed comments (1)

tools/CopilotNpmRegistry.props:129

• This .npmrc branch is not fully equivalent to Resolve-ReactorNpmRegistry for valid registry URLs containing an apostrophe in the path. The reference resolver accepts any URL that Uri.TryCreate accepts and only rejects userinfo/query/fragment (tools/BootstrapFeedResolver.ps1:7-11), so registry=https://packagefeedproxy.microsoft.io/npm/o'clock/ is selected there; the path character class on this line excludes apostrophes, so the end assertion fails and plain MSBuild stays on the public default. Either make the parser preserve apostrophes in unquoted/quoted URL paths or explicitly align the reference policy, and add this case to the differential corpus.

                                      and $([System.IO.File]::Exists('$(_ReactorNpmUserConfig)'))">$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_ReactorNpmUserConfig)')), '(?m)(?&lt;=^[ \t]*(?i:registry)[ \t]*=[ \t]*["\x27]?)(?i:https://packagefeedproxy\.microsoft\.io)(?::0*(?:6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{4}|\d{1,4}))?(?:/[^\s?#"\x27]*)?(?=["\x27]?[ \t]*\r?$)(?![\s\S]*^[ \t]*(?i:registry)[ \t]*=[ \t]*\S)'))</CopilotNpmRegistryUrl>

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate findings remain in locale-sensitive regex handling and test isolation.

Review details

Suppressed comments (4)

tests/build_props/ci/CopilotNpmRegistry.Tests.ps1:376

• These path-only probes leave USERPROFILE untouched, so a developer machine with ~/.npmrc causes the production ReadAllText branch to consume the real config despite the test header promising no real-config reads. That makes the suite dependent on arbitrary local registry contents and feeds user credentials into evaluation; use the synthetic $redirected profile for both default-path assertions instead.

    Assert-Equal (Join-Path $env:USERPROFILE '.npmrc') `
        (Invoke-ProbeProperty -Property 'USERCONFIG' `
        -Environment @{ NPM_CONFIG_USERCONFIG = '   ' }) `
        'an all-whitespace NPM_CONFIG_USERCONFIG falls back to the default path'

tests/build_props/ci/CopilotNpmRegistry.Tests.ps1:158

• The phrase “BOTH orderings must land on the proxy” contradicts the following proxyThenPublic case, which intentionally expects no mirror when the later public assignment wins. Please describe only the publicThenProxy ordering here so the test rationale does not mislead future changes.

    # npm lets a later line override an earlier one, so BOTH orderings must land
    # on the proxy. A "first registry= line wins, then validate the host" reading
    # would silently fail the first of these — the exact shape the bootstrap
    # resolver's own tests pin.

tools/CopilotNpmRegistry.props:134

• This Regex.Match uses the inline (?i:...) flag without RegexOptions.CultureInvariant. Ignore-case matching is current-culture-sensitive; under tr-TR, the existing uppercase HTTPS://PACKAGEFEEDPROXY.MICROSOFT.IO/... case can fail because ASCII I/i do not fold, so a configured mirror is ignored and the build falls back to the blocked public registry. Pass CultureInvariant to this overload and add a non-en-US regression case; apply the same fix to the .npmrc regex below.

    <CopilotNpmRegistryUrl>$([System.Text.RegularExpressions.Regex]::Match($(NPM_CONFIG_REGISTRY.Trim()), '^(?i:https://packagefeedproxy\.microsoft\.io)(?::0*(?:6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{4}|\d{1,4}))?(?:/[^\s?#]*)?$'))</CopilotNpmRegistryUrl>

tools/CopilotNpmRegistry.props:137

• The .npmrc parser has the same locale-sensitive (?i:...) matching as the environment branch. On a Turkish-locale build, an uppercase scheme/host that the PowerShell/URI resolver accepts can produce no match here, leaving CopilotNpmRegistryUrl empty and causing the native download to target npmjs.org. Add RegexOptions.CultureInvariant to this Regex.Match call (the existing inline (?m) can remain) and cover it with a non-en-US test.

                                      and $([System.IO.File]::Exists('$(_ReactorNpmUserConfig)'))">$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_ReactorNpmUserConfig)')), '(?m)(?&lt;=^[ \t]*(?i:registry)[ \t]*=[ \t]*["\x27]?)(?i:https://packagefeedproxy\.microsoft\.io)(?::0*(?:6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{4}|\d{1,4}))?(?:/(?:[^\s?#"]*[^\s?#"\x27])?)?(?=["\x27]?[ \t]*\r?$)(?![\s\S]*^[ \t]*(?i:registry)[ \t]*=[ \t]*\S)'))</CopilotNpmRegistryUrl>

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The resolver needs ASCII-only port validation and trailing-quote parity with the reference resolver.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

tools/CopilotNpmRegistry.props:149

• The port grammar uses \d, which in .NET matches every Unicode decimal digit, not just the ASCII digits accepted by URI port syntax. A value such as https://packagefeedproxy.microsoft.io:١٢٣/npm/ can therefore pass this regex and be exported as CopilotNpmRegistryUrl even though Resolve-ReactorNpmRegistry/Uri.TryCreate takes the no-mirror path, causing a malformed download URL instead of parity. Use [0-9] throughout the port alternatives and add a non-ASCII-digit parity case; the same correction is needed in the .npmrc expression below.

tools/CopilotNpmRegistry.props:152

• The .npmrc parser is not fully parity-compatible with Resolve-ReactorNpmRegistry for a registry value whose unquoted URL ends in a quote character. The resolver captures the value and then calls .Trim('"').Trim("'") (tools/BootstrapFeedResolver.ps1:42-44), so registry=https://packagefeedproxy.microsoft.io/npm/foo' resolves to the mirror, whereas this regex rejects a path ending in ' (and likewise ") and leaves the property empty. That makes the claimed differential contract false for a value the reference resolver accepts; either align the quote handling in both implementations or explicitly reject it in both, and add it to the parity corpus.

                                      and $([System.IO.File]::Exists('$(_ReactorNpmUserConfig)'))">$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_ReactorNpmUserConfig)')), '(?m)(?&lt;=^[ \t]*[Rr][Ee][Gg][Ii][Ss][Tt][Rr][Yy][ \t]*=[ \t]*["\x27]?)[Hh][Tt][Tt][Pp][Ss]://[Pp][Aa][Cc][Kk][Aa][Gg][Ee][Ff][Ee][Ee][Dd][Pp][Rr][Oo][Xx][Yy]\.[Mm][Ii][Cc][Rr][Oo][Ss][Oo][Ff][Tt]\.[Ii][Oo](?::0*(?:6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{4}|\d{1,4}))?(?:/(?:[^\s?#"]*[^\s?#"\x27])?)?(?=["\x27]?[ \t]*\r?$)(?![\s\S]*^[ \t]*[Rr][Ee][Gg][Ii][Ss][Tt][Rr][Yy][ \t]*=[ \t]*\S)'))</CopilotNpmRegistryUrl>

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Update workflow filters for Directory.Packages.props and strengthen SDK target integration coverage.

Review details

Suppressed comments (1)

.github/workflows/build-props-lib-tests.yml:26

• This job is the only check that evaluates the mirror property—the ordinary matrix sets CI=true, which makes the new props inert—but these filters omit Directory.Packages.props. A future GitHub.Copilot.SDK version change can alter or remove the CopilotNpmRegistryUrl contract while this job stays skipped and normal CI remains green. Add the package-version file to both push and pull_request filters, and preferably make the integration probe exercise the restored SDK target as well.

      - 'tools/CopilotNpmRegistry.props'
      # The suite's differential corpus dot-sources this resolver and requires
      # the props to agree with it, so a change there can invalidate the claimed
      # parity while this job stays skipped.
      - 'tools/BootstrapFeedResolver.ps1'

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Two moderate resolver parity issues remain unresolved.

Review details

Suppressed comments (2)

tools/CopilotNpmRegistry.props:157

• The .npmrc regex does not preserve the resolver's handling of a double-quoted URL whose path legitimately ends in an apostrophe, e.g. registry="https://packagefeedproxy.microsoft.io/npm/foo'". Resolve-ReactorNpmRegistry strips the surrounding double quote and then the trailing apostrophe before validating (tools/BootstrapFeedResolver.ps1:42-44), so it selects the mirror, but this pattern cannot consume the apostrophe as path data or as the double-quote delimiter and returns no match. That makes plain MSBuild silently fall back to npmjs for a configuration the bootstrap path accepts; add this case to the parity corpus and adjust the delimiter/path pattern (or align the resolver) so both paths agree.

                                      and $([System.IO.File]::Exists('$(_ReactorNpmUserConfig)'))">$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_ReactorNpmUserConfig)')), '(?m)(?&lt;=^[ \t]*[Rr][Ee][Gg][Ii][Ss][Tt][Rr][Yy][ \t]*=[ \t]*["\x27]?)[Hh][Tt][Tt][Pp][Ss]://[Pp][Aa][Cc][Kk][Aa][Gg][Ee][Ff][Ee][Ee][Dd][Pp][Rr][Oo][Xx][Yy]\.[Mm][Ii][Cc][Rr][Oo][Ss][Oo][Ff][Tt]\.[Ii][Oo](?::0*(?:6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}|6[0-4][0-9]{3}|[1-5][0-9]{4}|[0-9]{1,4}))?(?:/(?:[^\s?#"]*[^\s?#"\x27])?)?(?=["\x27]?[ \t]*\r?$)(?![\s\S]*^[ \t]*[Rr][Ee][Gg][Ii][Ss][Tt][Rr][Yy][ \t]*=[ \t]*\S)'))</CopilotNpmRegistryUrl>

tools/CopilotNpmRegistry.props:157

• These matches preserve a trailing /, but Resolve-ReactorNpmRegistry strips trailing slashes before passing -p:CopilotNpmRegistryUrl (tools/BootstrapFeedResolver.ps1:54-56). A normal .npmrc therefore produces a different SDK input; the parity loop currently hides that mismatch by calling .TrimEnd('/') on the actual value at tests/build_props/ci/CopilotNpmRegistry.Tests.ps1:568. Normalize the selected value with TrimEnd('/') in both branches and assert the exact normalized contract, avoiding a possible double slash when the SDK appends the package path.

    <CopilotNpmRegistryUrl>$([System.Text.RegularExpressions.Regex]::Match($(NPM_CONFIG_REGISTRY.Trim()), '^[Hh][Tt][Tt][Pp][Ss]://[Pp][Aa][Cc][Kk][Aa][Gg][Ee][Ff][Ee][Ee][Dd][Pp][Rr][Oo][Xx][Yy]\.[Mm][Ii][Cc][Rr][Oo][Ss][Oo][Ff][Tt]\.[Ii][Oo](?::0*(?:6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}|6[0-4][0-9]{3}|[1-5][0-9]{4}|[0-9]{1,4}))?(?:/[^\s?#]*)?$'))</CopilotNpmRegistryUrl>
    <CopilotNpmRegistryUrl Condition="'$(CopilotNpmRegistryUrl)' == ''
                                      and '$(NPM_CONFIG_REGISTRY.Trim())' == ''
                                      and $([System.IO.File]::Exists('$(_ReactorNpmUserConfig)'))">$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_ReactorNpmUserConfig)')), '(?m)(?&lt;=^[ \t]*[Rr][Ee][Gg][Ii][Ss][Tt][Rr][Yy][ \t]*=[ \t]*["\x27]?)[Hh][Tt][Tt][Pp][Ss]://[Pp][Aa][Cc][Kk][Aa][Gg][Ee][Ff][Ee][Ee][Dd][Pp][Rr][Oo][Xx][Yy]\.[Mm][Ii][Cc][Rr][Oo][Ss][Oo][Ff][Tt]\.[Ii][Oo](?::0*(?:6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}|6[0-4][0-9]{3}|[1-5][0-9]{4}|[0-9]{1,4}))?(?:/(?:[^\s?#"]*[^\s?#"\x27])?)?(?=["\x27]?[ \t]*\r?$)(?![\s\S]*^[ \t]*[Rr][Ee][Gg][Ii][Ss][Tt][Rr][Yy][ \t]*=[ \t]*\S)'))</CopilotNpmRegistryUrl>

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Address the invalid environment URL handling and clarify the documented skip behavior.

Review details

Suppressed comments (2)

README.md:104

• The new README escape hatch omits the runtime consequence that this PR documents elsewhere: setting CopilotSkipCliDownload=true also removes the binary used by mur loc translate and both Copilot SDK samples (Directory.Build.props:132-147). A contributor following this advice to get past a blocked build will produce a CLI/sample that fails later at runtime; qualify this as compile-only and point users to the mirror option for a usable local build.

The same npm mirror detection also applies to plain `dotnet build` / `dotnet run` / `dotnet test`, which never pass through bootstrap. `src/Reactor.Cli` and two samples reference `GitHub.Copilot.SDK`, whose build targets download a native `copilot` CLI from `registry.npmjs.org`; on a network that blocks it, that download fails the build itself with `MSB3923`. [`tools/CopilotNpmRegistry.props`](tools/CopilotNpmRegistry.props) resolves the mirror from the same user configuration at evaluation time, so every documented command — including the `mur docs compile` the docs freshness gate asks you to run — works unmodified on both networks. Set `CopilotNpmRegistryUrl` explicitly to use a different mirror, or `CopilotSkipCliDownload=true` to skip the native CLI entirely (CI does the latter).

tools/CopilotNpmRegistry.props:154

• The environment-variable branch does not apply the same URI validation as Resolve-ReactorNpmRegistry: [^ \s?#]* permits an unescaped " in the path, so NPM_CONFIG_REGISTRY=https://packagefeedproxy.microsoft.io/npm/foo" is exported even though Test-ReactorPackageFeedUrl rejects it via Uri.TryCreate (tools/BootstrapFeedResolver.ps1:4-11). That sends an invalid registry URL to the SDK instead of taking the no-mirror path; exclude " in this branch and add the corresponding environment case (the existing trailing-quote case only exercises .npmrc).

    <CopilotNpmRegistryUrl>$([System.Text.RegularExpressions.Regex]::Match($(NPM_CONFIG_REGISTRY.Trim()), '^[Hh][Tt][Tt][Pp][Ss]://[Pp][Aa][Cc][Kk][Aa][Gg][Ee][Ff][Ee][Ee][Dd][Pp][Rr][Oo][Xx][Yy]\.[Mm][Ii][Cc][Rr][Oo][Ss][Oo][Ff][Tt]\.[Ii][Oo](?::0*(?:6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}|6[0-4][0-9]{3}|[1-5][0-9]{4}|[0-9]{1,4}))?(?:/[^\s?#]*)?$'))</CopilotNpmRegistryUrl>

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite