Skip to content

Pin Corepack explicitly for the VS Code extension build#17630

Open
adamint wants to merge 17 commits into
microsoft:mainfrom
adamint:dev/adamint/extension-corepack
Open

Pin Corepack explicitly for the VS Code extension build#17630
adamint wants to merge 17 commits into
microsoft:mainfrom
adamint:dev/adamint/extension-corepack

Conversation

@adamint
Copy link
Copy Markdown
Member

@adamint adamint commented May 28, 2026

Description

The VS Code extension build previously assumed that corepack (and therefore Yarn) was already present on the build agent's PATH. That worked on most agents because Corepack ships with Node.js, but it's brittle:

  • We didn't pin which Corepack we were getting, so a different agent image could silently change behavior (Corepack 0.31+ enforces signature verification with keys baked into the shim, so the Corepack version actually matters).
  • The Yarn version (1.22.22) was duplicated as an inline corepack yarn@1.22.22 … pin in four places (extension/build.sh, extension/build.ps1, two paths in extension/Extension.proj) plus three AzDO pipelines that bypassed Corepack entirely with npm install -g yarn@1.22.22. Bumping Yarn meant editing seven files in lockstep and hoping you didn't miss one.
  • Corepack downloads Yarn from registry.npmjs.org by default; it does not read the project's .npmrc. That means even though the rest of the extension build is wired to the dotnet-public-npm mirror, the Yarn tarball itself was coming from the public registry.

This PR makes the Corepack and Yarn versions explicit and centralized, and routes Corepack downloads through the approved internal feed.

What changed

  • extension/package.json: added "packageManager": "yarn@1.22.22". This is the canonical Corepack hook — corepack prepare --activate reads it automatically — so the Yarn version now has a single source of truth.
  • extension/build.sh and extension/build.ps1: now npm install -g corepack@0.34.7 first, then corepack enable, then corepack prepare --activate. Both scripts default COREPACK_NPM_REGISTRY to the dnceng dotnet-public-npm mirror and set COREPACK_ENABLE_DOWNLOAD_PROMPT=0 so unattended runs don't block on the "download Yarn?" prompt. Both vars are overridable from the environment.
  • extension/Extension.proj: the four corepack yarn@1.22.22 … invocations are now corepack yarn …. CheckYarnInstalled now runs from $(ExtensionSrcDir) so Corepack picks up the packageManager field, and depends on ValidateYarnLockRegistries so we fail fast if the lockfile drifted off the internal mirror.
  • AzDO pipelines (azure-pipelines.yml, azure-pipelines-unofficial.yml, azure-pipelines-codeql.yml): the Install yarn step (which previously did npm install -g yarn@1.22.22 and bypassed Corepack) is replaced with Install Corepack, which installs corepack@0.34.7 and runs corepack prepare --activate from the extension/ directory. The existing npmAuthenticate@0 step that runs immediately before this provides the credentials needed for Corepack's first-run pull-through into the dnceng feed.
  • AzDO pipeline variables: added COREPACK_NPM_REGISTRY (dnceng mirror) and COREPACK_ENABLE_DOWNLOAD_PROMPT=0 to both eng/pipelines/common-variables.yml (included by the internal/unofficial/codeql pipelines and the release publish pipeline) and eng/pipelines/templates/public-pipeline-template.yml (the PR pipeline, which has its own variables: block).
  • extension/CONTRIBUTING.md: prerequisites and dependency-override sections updated to describe the Corepack flow, plus a new "Updating the Yarn version" section pointing at the packageManager field.

Why pin Corepack via npm instead of letting Node ship it

Corepack ships with Node.js, but the version that ships is whatever was current when that Node release was cut. With Corepack 0.31+ enforcing signature verification using keys baked into the shim, the Corepack version is now part of our build's reproducibility surface. Pinning it via npm install -g corepack@<version> matches how we pin every other tool in CI.

There is no package.json field that controls which Corepack version is installed (only which yarn/pnpm/npm Corepack provisions), so the version necessarily lives in the build scripts.

Why a separate "follow up" comment about Yarn 4

Bumping to Yarn 4 is the natural next step now that Corepack is in place, but it's a real dependency upgrade (new lockfile format, --frozen-lockfile--immutable, .yarnrc.yarnrc.yml, vsce interaction with the chosen linker mode, possibly PnP vs node-modules). I'm keeping that out of this PR so the build-infra fix can land on its own risk profile, and will follow up with a dedicated upgrade PR.

Validation

  • Bash, PowerShell, JSON, YAML, and XML parsers all parse cleanly after the edits.
  • Final stale-pin sweep across extension/ and eng/pipelines/ is clean — the only remaining 1.22.22 references are the canonical packageManager field in extension/package.json and the entries in extension/yarn.lock.
  • Verified via authenticated HTTP that yarn-1.22.22.tgz is already mirrored in dnceng dotnet-public-npm (returns 200, 1.2 MB). corepack@0.34.7 is not yet mirrored, but the existing npmAuthenticate@0 step in the AzDO pipelines provides the credentials Corepack needs to trigger the first-run pull-through. After the first successful pipeline run the package is permanently cached.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No

Replaces the implicit 'corepack is somewhere on PATH' assumption with an
explicit 'npm install -g corepack@0.34.7' step in extension/build.sh,
extension/build.ps1, and the three AzDO pipelines that build the
extension. The Yarn version is now pinned in extension/package.json via
the standard 'packageManager' field (yarn@1.22.22), removing duplicate
@1.22.22 pins from build.sh, build.ps1, and Extension.proj.

The build scripts default COREPACK_NPM_REGISTRY to the dnceng
dotnet-public-npm mirror and disable the Corepack download prompt, and
the same defaults are set as AzDO pipeline variables in
common-variables.yml and public-pipeline-template.yml so Corepack
downloads Yarn from an approved internal feed rather than npmjs.org.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 28, 2026 21:23
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 28, 2026

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17630

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17630"

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes the VS Code extension build more reproducible and less agent-image-dependent by explicitly pinning Corepack (installed via npm) and centralizing the Yarn version pin via the packageManager field, while also routing Corepack’s downloads through the internal dotnet-public-npm mirror in CI.

Changes:

  • Add packageManager: yarn@1.22.22 to centralize the Yarn pin for Corepack.
  • Update extension build scripts and MSBuild packaging to use corepack prepare --activate + corepack yarn ... (no inline Yarn version pin).
  • Update AzDO pipelines/templates to install a pinned Corepack and set Corepack registry/prompt environment variables.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
extension/package.json Adds packageManager to make the Yarn version a single source of truth for Corepack.
extension/Extension.proj Removes inline yarn@... pins and makes Corepack resolve Yarn via packageManager from the extension directory.
extension/CONTRIBUTING.MD Updates contributor guidance to the Corepack-based flow and documents how to bump Yarn.
extension/build.sh Installs pinned Corepack via npm, enables Corepack shims, and activates Yarn from packageManager.
extension/build.ps1 PowerShell equivalent of the pinned Corepack + prepare --activate flow.
eng/pipelines/templates/public-pipeline-template.yml Adds Corepack env vars so CI uses the internal npm mirror and doesn’t hang on prompts.
eng/pipelines/common-variables.yml Adds shared Corepack env vars for internal/unofficial/codeql/release pipeline usage.
eng/pipelines/azure-pipelines.yml Replaces “install yarn” with “install Corepack + prepare --activate” from extension/.
eng/pipelines/azure-pipelines-unofficial.yml Same Corepack installation/activation changes as the official pipeline.
eng/pipelines/azure-pipelines-codeql.yml Same Corepack installation/activation changes for CodeQL pipeline.

Comment thread extension/build.sh Outdated
Comment thread extension/build.ps1 Outdated
adamint and others added 10 commits May 28, 2026 17:29
- Add npmAuthenticate@0 + NPM_CONFIG_USERCONFIG setup to the CodeQL
  pipeline before the Install Corepack step. The previous CodeQL pipeline
  worked anonymously against dnceng dotnet-public-npm only because
  yarn@1.22.22 was already cached there; corepack@0.34.7 is not, and the
  pipeline would have started failing on the first run.
- After 'npm install -g corepack@<pin>' in build.sh, build.ps1, and all
  three AzDO pipeline PowerShell steps, run 'corepack --version' and
  fail loudly if the version doesn't match the pin. On Windows the
  bundled corepack.cmd under %ProgramFiles%\nodejs can shadow the
  npm-global shim under %APPDATA%\npm, so a successful install does not
  guarantee the pinned Corepack is what 'corepack enable' actually runs.
- In build.sh and build.ps1 only (not the pipelines), force the public
  npm registry for the Corepack install via
  --registry=https://registry.npmjs.org so first-time OSS contributors
  are not blocked on dnceng cache misses. Corepack is build tooling and
  never ships in the extension VSIX, so registry choice is local-dev
  ergonomics only.
- Document the 'EACCES from npm install --global' and 'corepack version
  mismatch' troubleshooting steps in extension/CONTRIBUTING.MD, plus a
  note that bumping Yarn requires the new tarball to be pulled through
  dotnet-public-npm at least once with credentials.
- Drop the misleading 'update Extension.proj inline pin' comment from
  the build scripts (no such inline pin remains).
- Drop the redundant DependsOnTargets='ValidateYarnLockRegistries' from
  CheckYarnInstalled; the parent BuildAndPackageExtension target already
  declares the same dependency.
- Normalize the workingDirectory path separator across the three AzDO
  pipelines to backslash, matching the convention used elsewhere in
  those Windows-only files.
- Add trailing newline to extension/CONTRIBUTING.MD.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
corepack@0.34.7 is already cached in the dnceng dotnet-public-npm feed
and serves anonymously, so the build scripts don't need to bypass the
internal mirror to install Corepack. Verified anonymously:

  GET .../dotnet-public-npm/.../corepack/-/corepack-0.34.7.tgz
  -> 200 OK, 229 KB

The earlier comment overstated the problem: only versions that have
never been requested from the feed return 401 (the feed's pull-through
behavior requires auth for the very first fetch, then anyone can read
the cached copy). The same caveat applies to bumping the pinned Yarn
or Corepack version, so the heads-up about pre-seeding the feed now
lives in extension/CONTRIBUTING.MD rather than the build-script comments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ensure Corepack and Yarn setup use the configured npm registry and authenticated Azure Artifacts credentials across local scripts and CI.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Azure Artifacts does not support the npm /package/version metadata endpoint Corepack uses when COREPACK_NPM_REGISTRY is set. Keep the internal feed for npm's Corepack install, but let Corepack prepare Yarn without that registry override.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Hosted Windows images can already have a Yarn shim in npm's global prefix, and the npm Corepack package owns that shim. Use --force only in CI tool setup so the pinned Corepack install can replace ephemeral runner shims without changing local developer scripts.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
After installing the pinned Corepack package, hosted Windows runners can still resolve the bundled Corepack first. Prepend npm's global prefix for the current CI step and subsequent steps so Corepack 0.34.7 is the shim that prepares and runs Yarn.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Match the duplicate ATS capability parser/test/baseline shape from PR microsoft#17631 to avoid merge conflicts between the branches.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@adamint adamint requested a review from sebastienros as a code owner May 29, 2026 17:35
adamint and others added 2 commits May 29, 2026 13:44
Keep the TypeScript API compatibility fix in the dedicated Foundry API PR instead of duplicating it in this Corepack CI fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@adamint
Copy link
Copy Markdown
Member Author

adamint commented May 29, 2026

Validation update after merging latest main (SHA 21d73040ef0369873b4f8af11cb0dc448cd0fe49):

  • ADO build 20260529.14 / 2987334 got past the original failure: 🟣Install Corepack completed successfully.
  • The remaining CI failures I see are not from the Corepack/Yarn install path: Hosting.OpenAI failed in OpenAIFunctionalTests.DependentResourceWaitsForOpenAIResourceWithHealthCheckToBeHealthy waiting for nginx to reach Running, and Prepare WinGet installer artifacts failed in Upload installer artifact after finding 4 files to upload.

ADO run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2987334&view=results

@adamint
Copy link
Copy Markdown
Member Author

adamint commented May 29, 2026

adamint added a commit to adamint/aspire that referenced this pull request May 29, 2026
Applies the dedicated Corepack pinning and Yarn cache seeding diff from microsoft#17630 instead of keeping a local source-build workaround on this branch.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ling

- extension/build.sh: export NPM_REGISTRY so the child
  scripts/prepareCorepackYarn.mjs process actually inherits it. Without
  the export, the script silently fell back to its DefaultNpmRegistry
  constant and any user override of NPM_REGISTRY would be ignored when
  seeding Corepack's Yarn cache. COREPACK_ENABLE_DOWNLOAD_PROMPT on the
  next line was already exported; this restores symmetry.

- extension/scripts/prepareCorepackYarn.mjs: add a comment in
  getCorepackHome() documenting the implicit coupling to corepack
  0.34.x's own cache-path resolution. If COREPACK_VERSION is later bumped
  to a release that switches schemes (e.g., env-paths, which would
  relocate the macOS cache to ~/Library/Caches/node/corepack), this
  fallback would silently seed the wrong directory. The AzDO pipelines
  already set COREPACK_HOME explicitly to avoid this; this comment flags
  the same hardening as the simplest fix when the pin is updated.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One HIGH-severity robustness issue from a multi-model review pass (Opus 4.8 + GPT 5.5, validated by Opus 4.7 high). The local extension/build.sh and extension/build.ps1 are missing --force on the npm install --global corepack@... call, which all CI paths already pass. This will break on developer machines that have any of yarn/pnpm/yarnpkg/pnpx already installed globally (which is most devs who have ever touched this repo — the project itself shipped npm install -g yarn@1.22.22 until this PR). Details inline.

Will follow up with additional MEDIUM/LOW findings once they're re-validated.

Comment thread extension/build.sh Outdated
Comment thread extension/build.ps1 Outdated
Copy link
Copy Markdown
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: 5 MEDIUM findings from the same multi-model review pass (Opus 4.8 + GPT 5.5, validated by Opus 4.7 high; two more independent validators are still running and I'll add any net-new findings they surface).

None of these are blockers — the PR's primary goal is achieved — but several weaken the reproducibility/maintainability story it's building:

  • CI inconsistency (#2): GH Actions Windows fetches Yarn from yarnpkg.com, bypassing dnceng, and the seed script has zero non-Windows CI coverage.
  • Documented dev flow gap (#3): root ./build.sh /p:BuildExtension=true (the Arcade flow) never runs the new bootstrap, so a clean machine following the documented build instructions will fail in a way the new error message doesn't actually explain.
  • Concurrency hazard (#4): local builds racing on the shared user-default COREPACK_HOME can corrupt each other; the script's own comment already calls out the fix.
  • Canonical-form mismatch (#5): the packageManager regex rejects the integrity-suffixed form that corepack use yarn@<v> itself writes, which is the only thing CONTRIBUTING.MD now points contributors at.
  • Drift surface (#6): five files own 0.34.7 and three pipelines own the same ~30-line install block.

Details inline.

Comment thread .github/workflows/tests.yml Outdated
Comment thread extension/Extension.proj Outdated
Comment thread extension/scripts/prepareCorepackYarn.mjs
Comment thread extension/scripts/prepareCorepackYarn.mjs Outdated
Comment thread extension/build.sh Outdated
adamint and others added 2 commits May 29, 2026 18:38
Six follow-ups from the review on microsoft#17630:

* Add --force to the local 'npm install --global corepack@<version>' in
  extension/build.sh and extension/build.ps1. Without --force, npm refuses
  to overwrite the yarn / yarnpkg / pnpm / pnpx bin entries owned by any
  pre-existing global yarn or pnpm install (the state this repo itself
  shipped before the bootstrap existed), aborting with EEXIST. The CI
  pipelines already pass --force for the same reason.

* Switch the GitHub Actions 'extension_tests_win' job to use
  prepareCorepackYarn.mjs instead of 'corepack prepare --activate'. The
  built-in Corepack prepare path downloads Yarn 1.x from
  registry.yarnpkg.com (hardcoded in Corepack 0.34's config.json and not
  redirectable via COREPACK_NPM_REGISTRY), bypassing the dnceng feed this
  workflow exists to validate. Also scope COREPACK_HOME to runner.temp.

* Add an 'extension_bootstrap_linux' GH Actions job so the non-Windows
  branches of prepareCorepackYarn.mjs (POSIX npm invocation, no node.exe
  wrapping) are exercised on a fresh CI image, not only on contributor
  machines.

* Update the 'CheckYarnInstalled' error in extension/Extension.proj to
  name the actual supported entry points (the root Arcade flow
  './build.sh -build-extension' and direct 'dotnet build
  extension/Extension.proj' both require running extension/build.sh or
  extension/build.ps1 first) instead of pointing developers at a path
  that isn't part of the documented root build flow.

* Scope COREPACK_HOME to '$SCRIPT_DIR/.corepack-cache' in the local
  build entrypoints. prepareCorepackYarn.mjs rewrites the cache in place
  via rmSync + renameSync, so concurrent builds (multiple worktrees,
  parallel invocations) sharing the user's default cache can corrupt
  each other. The CI pipelines already scope this per-job via
  Agent.TempDirectory / runner.temp; do the same locally. New cache
  directory is gitignored.

* Loosen PackageManagerPattern in prepareCorepackYarn.mjs to accept the
  optional integrity suffix that 'corepack use yarn@<v>' writes
  ('yarn@1.22.22+sha512.<hex>'). CONTRIBUTING.MD points contributors at
  'corepack use' for updating the pin, so rejecting the canonical
  spec-conformant value would have broken that flow.

* Centralize the pinned Corepack version in
  extension/scripts/corepack-version.txt. The bash and PowerShell build
  scripts, the GitHub Actions workflow, and all three AzDO pipelines
  (azure-pipelines.yml, azure-pipelines-unofficial.yml,
  azure-pipelines-codeql.yml) now read from this single file, removing
  the six-place version-drift hazard.

Validated by running extension/build.sh end-to-end from a clean state:
npm install (with --force), corepack enable, prepareCorepackYarn.mjs
against $SCRIPT_DIR/.corepack-cache, corepack yarn install
--frozen-lockfile, corepack yarn compile, dotnet build Aspire.Cli.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The runner context is not available in job-level env evaluation, so
'COREPACK_HOME: ${{ runner.temp }}/corepack' caused the workflow file
to be rejected before any job could run ('This run likely failed because
of a workflow file issue', latest_check_runs_count: 0).

Forward COREPACK_HOME from inside the Install Corepack step using
$RUNNER_TEMP (which is exposed as an env var on the runner) and
$GITHUB_ENV. The value reaches all subsequent steps the same way a
job-level env entry would have, so the corepack cache stays isolated
to the job.

Verified with actionlint: previously two 'context "runner" is not
allowed here' errors at lines 320 and 395; now clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@adamint
Copy link
Copy Markdown
Member Author

adamint commented May 29, 2026

/azp run microsoft-aspire

Copy link
Copy Markdown
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-model review pass (Opus 4.8 + GPT 5.5, validated by Opus 4.8). 7 inline findings on the code, plus 1 finding on the PR description below.


[MEDIUM] PR description is materially out of date

The "What changed" section still says:

Both scripts default COREPACK_NPM_REGISTRY to the dnceng dotnet-public-npm mirror …

then corepack prepare --activate

AzDO pipeline variables: added COREPACK_NPM_REGISTRY (dnceng mirror)

All three are false in the current code:

  • Scripts use NPM_REGISTRY, not COREPACK_NPM_REGISTRY. extension/CONTRIBUTING.MD:46 explicitly warns against pointing COREPACK_NPM_REGISTRY at the Azure Artifacts feed (Corepack's /<pkg>/<ver> metadata route 404s there).
  • The flow is node ./scripts/prepareCorepackYarn.mjs, not corepack prepare --activate (which would pull Yarn from registry.yarnpkg.com, the exact thing this PR is supposed to avoid).
  • The pipeline var added is NPM_REGISTRY (eng/pipelines/common-variables.yml:15, templates/public-pipeline-template.yml:37).

The "downloads route through the approved internal feed" claim is security-relevant. A reviewer trusting the body sees a mechanism (COREPACK_NPM_REGISTRY + corepack prepare --activate) that the implementation specifically avoids because it doesn't work against Azure Artifacts.

Fix: rewrite the bullets to describe NPM_REGISTRY, prepareCorepackYarn.mjs, per-job COREPACK_HOME, and scripts/corepack-version.txt. Drop the COREPACK_NPM_REGISTRY / corepack prepare --activate references.

// The suffix is optional but its presence must not break us. Match an optional
// `+<token>` suffix and ignore it; only the version is needed to seed the cache
// (Corepack itself verifies the package hash via its own metadata after we
// rename the staging dir into place).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Misleading comment: Corepack does NOT re-verify the seeded hash.

This comment implies a second integrity gate. There isn't one.

In Corepack v0.34.7 installVersion, when the cache directory already contains .corepack, the function returns the recorded hash/bin immediately without re-hashing or signature-checking:

const corepackContent = await fs.promises.readFile(corepackFile, `utf8`);
const corepackData = JSON.parse(corepackContent);
return { hash: corepackData.hash, location: installFolder, bin: corepackData.bin };

The Mismatch hashes check fires only on the download path, which a pre-seeded cache never reaches. runVersion never references hash at all. So the sha1.${packEntry.shasum} we write at line 87 is decorative — trust on the seeded Yarn rests entirely on the npm pack fetch from $NPM_REGISTRY.

Fix: update the comment to say Corepack does not re-verify a pre-seeded cache on reuse; integrity rests on the npm pack fetch from the dnceng feed. Optionally, validate packEntry.integrity (npm pack emits it) against the published sha512 to add a real second gate.

Comment thread extension/Extension.proj
</Exec>

<Error Condition="'$(YarnExitCode)' != '0'" Text="Corepack is not installed or cannot run Yarn Classic. To build the extension, install a Node.js version that includes Corepack." />
<Error Condition="'$(YarnExitCode)' != '0'" Text="Corepack is not installed or cannot run the Yarn version pinned in extension/package.json. The extension's Corepack and Yarn bootstrap lives in extension/build.sh and extension/build.ps1: those scripts install a pinned Corepack via npm and seed Corepack's Yarn cache from the internal npm feed. Run one of them once before invoking the extension build, including before `./build.sh -build-extension` (the root Arcade flow) or direct `dotnet build extension/Extension.proj` — neither installs Corepack itself. See extension/CONTRIBUTING.MD for details." />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] The documented ./build.sh -build-extension recovery path is self-defeating.

This error tells devs to run extension/build.sh or extension/build.ps1 first, then build via ./build.sh -build-extension or dotnet build extension/Extension.proj. But:

  1. extension/build.sh:48 exports COREPACK_HOME=$SCRIPT_DIR/.corepack-cache in a subprocess. The variable doesn't persist to the parent shell after build.sh exits.
  2. eng/build.sh -build-extension doesn't source extension/build.sh. It runs the Arcade build with BuildExtension=true, which builds Extension.proj in a fresh shell with COREPACK_HOME unset.
  3. None of the corepack Execs in this file (lines 34, 39, 40, 94) set COREPACK_HOME.
  4. getCorepackHome() in prepareCorepackYarn.mjs then falls back to ~/.cache/node/corepack, which extension/build.sh never wrote to.

Net: following the documented recovery path literally seeds extension/.corepack-cache, then CheckYarnInstalled looks in ~/.cache/node/corepack and still fails.

CI is unaffected — AzDO uses Write-Host "##vso[task.setvariable variable=COREPACK_HOME]…" (azure-pipelines.yml:315) and GHA uses $GITHUB_ENV (tests.yml:336,416), both of which propagate to later steps in the same job. This is a dev-machine-only gap.

Fix options:

  • Set COREPACK_HOME=$([MSBuild]::NormalizePath($(ExtensionSrcDir), '.corepack-cache')) as env on each corepack yarn Exec in this file; or
  • Have prepareCorepackYarn.mjs also seed Corepack's default user cache so any subsequent invocation finds it; or
  • Reword the error to tell devs to also export COREPACK_HOME=$(repoRoot)/extension/.corepack-cache in the shell they invoke the root build from.


corepack yarn --version
- name: Validate lockfile registries
run: node -e "const fs = require('fs'); const lock = fs.readFileSync('yarn.lock', 'utf8'); if (/registry\\.(?:npmjs\\.org|yarnpkg\\.com)/.test(lock)) { throw new Error('extension/yarn.lock contains public npm registry URLs. Regenerate it using the internal dotnet-public-npm feed before restoring.'); }"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Lockfile registry validation is a 2-entry denylist (duplicated in 2 places).

This check and extension/Extension.proj:82 only reject registry.npmjs.org and registry.yarnpkg.com. The stated intent in CONTRIBUTING.MD:40 is "ensure regenerated entries resolve through the internal dotnet-public-npm feed" — i.e., an allowlist. The denylist lets registry.npmmirror.com, npm.pkg.github.com, jsr.io, or any CDN tarball pass.

All 873 resolved lines in the current yarn.lock already point at pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm. An allowlist scoped to lines beginning with resolved wouldn't break anything (npm: aliases on lines 308-312 carry no URL).

Fix: switch both checks to an allowlist asserting the host contains pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm, applied to lines starting with resolved. Consolidating into one shared script would also remove the drift risk between the two implementations.

# CONTRIBUTING.MD would also be broken.
run: node ./scripts/prepareCorepackYarn.mjs
- name: Validate seeded cache via yarn install
run: corepack yarn install --frozen-lockfile --non-interactive
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] extension_bootstrap_linux skips the lockfile registry validation that extension_tests_win performs.

This Linux bootstrap job runs the seed script and corepack yarn install --frozen-lockfile, but does not run the equivalent of the Validate lockfile registries step at line 370-371. If yarn.lock drifts to a public-registry URL, only the Windows job catches it. Asymmetric coverage in what's meant to be the cross-platform bootstrap validator.

Fix: add the same node -e validation step here too, before Install dependencies runs.

renameSync(stagingDirectory, installDirectory);
cacheSeeded = true;
} catch (error) {
if (error?.code === 'EEXIST') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] EEXIST-only catch is incomplete, and the build.sh/build.ps1 isolation comment overclaims.

Two related issues:

  1. The catch handles only EEXIST. On Linux/macOS, rename(staging, installDirectory) against a non-empty existing dir fails with ENOTEMPTY (Linux) or ENOTEMPTY/EEXIST (varies by filesystem). The current code re-throws ENOTEMPTY, so the loser of a concurrent-build race fails the build with a confusing error instead of logging "already contains" and exiting cleanly.

  2. extension/build.sh:46-47 (and build.ps1:45-46) overclaim isolation:

    so local concurrent invocations and multi-worktree setups stay isolated

    Multi-worktree: ✅ (distinct $SCRIPT_DIR). Same-worktree concurrent builds: ❌ — both share $SCRIPT_DIR/.corepack-cache. Both processes pass existsSync at line 47, both rmSync(installDirectory) at line 52 (process B can delete process A's freshly-installed dir mid-yarn install), then race on renameSync at line 91. The loser hits the unhandled-ENOTEMPTY path above.

Fix: catch both EEXIST and ENOTEMPTY here, and tighten the build.sh/build.ps1 comment to "multi-worktree setups stay isolated" only. (A stronger fix would be a file lock + skipping the unconditional rmSync when the cache already exists at start.)

node ./scripts/prepareCorepackYarn.mjs
if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }

corepack yarn --version
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] AzDO "Install Corepack" PowerShell block is triplicated.

This ~40-line block is line-for-line near-identical in:

  • eng/pipelines/azure-pipelines.yml:302-345
  • eng/pipelines/azure-pipelines-unofficial.yml:213-256
  • eng/pipelines/azure-pipelines-codeql.yml:71-112

Same version-file read, same task.setvariable COREPACK_HOME, same npm install -g --force, same version-mismatch guard, same corepack enable, same prepareCorepackYarn.mjs call, same corepack yarn --version. Three places to keep in sync.

The repo already uses eng/pipelines/templates/ for this kind of extraction (e.g. BuildAndTest.yml, public-pipeline-template.yml). Worth pulling out to templates/install-corepack.yml.

// running `node ./scripts/prepareCorepackYarn.mjs` directly). It mirrors
// Corepack 0.34.x's own cache-path resolution so we seed the directory
// Corepack will later read from.
// Source: https://github.com/nodejs/corepack/blob/v0.34.0/sources/folderUtils.ts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] Stale Corepack source citation.

This link cites v0.34.0/sources/folderUtils.ts, but extension/scripts/corepack-version.txt pins 0.34.7. The cache-path resolution is identical between the two so behavior is fine, but the citation version should track the pin.

Fix: change v0.34.0 to v0.34.7 here (and at line 18-19 if the v0.34.0 reference is also there).

@radical
Copy link
Copy Markdown
Member

radical commented May 30, 2026

After your fixes could you also please do a fresh azdo build for validation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants