Skip to content

Use npm ci in eval workflow dependencies#2748

Merged
JasonYeMSFT merged 2 commits into
microsoft:mainfrom
fallintoplace:fix/eval-workflow-ci-install
Jul 16, 2026
Merged

Use npm ci in eval workflow dependencies#2748
JasonYeMSFT merged 2 commits into
microsoft:mainfrom
fallintoplace:fix/eval-workflow-ci-install

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • Replace npm install with deterministic lockfile installs in the eval workflow.
  • Use npm ci --ignore-scripts at repo root.
  • Install scripts dependencies explicitly via npm ci so npm run vally still works.

Safety rationale

  • npm ci uses package-lock.json exactly and avoids lockfile drift in CI.
  • Scripts are now installed in an explicit step, avoiding hidden postinstall behavior from root only.

Copilot AI review requested due to automatic review settings July 5, 2026 00:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 updates the eval GitHub Actions workflow to use lockfile-driven installs, making dependency installation deterministic and avoiding implicit postinstall behavior at the repo root (which previously installed scripts/ dependencies via npm install).

Changes:

  • Switch the root dependency install in the eval workflow from npm install to npm ci --ignore-scripts.
  • Add an explicit scripts/ dependency install step via npm ci (within the same workflow step) to keep npm run vally working.

@jongio jongio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The approach here is correct. The root package.json has a postinstall hook (cd scripts && npm install) that implicitly installed scripts/ dependencies. Since --ignore-scripts skips that hook, the explicit cd scripts; npm ci step is the right compensation.

Both root and scripts/ have a package-lock.json, so npm ci will work in both locations.

One minor note on the scripts install below.

Comment thread .github/workflows/eval.yml Outdated

@jongio jongio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Incremental review (new commits since my last pass):

The scripts/ install now includes --ignore-scripts, which was the one suggestion from my prior review. Both install steps are consistent: lockfile-pinned (npm ci) with script suppression for CI safety.

Final state looks correct. No remaining concerns.

Comment thread .github/workflows/eval.yml Outdated

@RickWinter RickWinter left a comment

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.

This switches the eval workflow to lockfile-pinned installs and explicitly restores the scripts dependencies skipped by the root install. The approach is the right shape. The current discussion proposes removing that required scripts install, which would break the metadata validation step. That must be resolved before merge.

Comment thread .github/workflows/eval.yml Outdated
@fallintoplace
fallintoplace force-pushed the fix/eval-workflow-ci-install branch from 3de0f37 to ea8877b Compare July 14, 2026 00:38
@JasonYeMSFT
JasonYeMSFT merged commit d7d3480 into microsoft:main Jul 16, 2026
10 checks passed
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.

5 participants