Skip to content

ci: replace npm ci with npm install in benchmark and license workflows#308

Merged
carlos-alm merged 2 commits intomainfrom
fix/benchmark-duplicate-prs
Mar 4, 2026
Merged

ci: replace npm ci with npm install in benchmark and license workflows#308
carlos-alm merged 2 commits intomainfrom
fix/benchmark-duplicate-prs

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Replace npm ci with npm install in the license compliance scan and benchmark workflows
  • npm ci fails when package-lock.json is out of sync with package.json for the @optave/codegraph-* native binary optional deps (only published during release)
  • npm install tolerates missing optional deps gracefully, matching the main CI workflow

Fixes the "License Compliance Scan" failure on PRs #298, #300, #307.

Test plan

  • Verify license compliance scan passes on this PR
  • Verify benchmark workflow can still run successfully

The Benchmark workflow triggers on any Publish workflow completion.
During a stable release, both a dev-push Publish (from merging PRs)
and the release-event Publish complete successfully, each spawning
a Benchmark run that resolves to the same stable version via git tag
lookup — creating duplicate PRs.

Two fixes:
1. Filter workflow_run triggers to exclude push-initiated Publish
   completions (dev builds should not benchmark the latest stable tag)
2. Check for existing open PRs before creating new ones as a safety net
npm ci fails when package-lock.json is out of sync with package.json
for the @optave/codegraph-* native binary optional deps, which are only
published during the release process. npm install tolerates missing
optional deps gracefully, matching how the main CI workflow already
handles this.
@claude
Copy link

claude bot commented Mar 4, 2026

Claude finished @carlos-alm's task —— View job


PR Review Complete

Root Cause Analysis

Problem Being Solved: The license compliance scan and benchmark workflows are failing because npm ci requires an exact match between package.json and package-lock.json, but the optional native binary packages (@optave/codegraph-*) are only published during releases. This creates a mismatch where the lock file references packages that don't exist on npm during development/PR builds.

Why This Approach: npm install is more tolerant of missing optional dependencies and gracefully handles the case where platform-specific native binaries aren't available. This aligns with the main CI workflow (.github/workflows/ci.yml:28) which already uses npm install with retry logic.

Risk Assessment: LOW RISK - This is a legitimate fix for a real infrastructure issue, not a symptom-hiding change.

Configuration Changes Analysis

  • Type: CI workflow dependency installation method
  • Scope: 2 workflow files, 5 total replacements of npm cinpm install
  • Consistency: Aligns with existing main CI workflow pattern
  • Justification: Solid - the optional dependencies architecture requires this approach

Technical Assessment

✅ Strengths

  1. Legitimate Fix: Addresses a real problem with optional native dependencies
  2. Consistency: Matches the pattern already established in main CI workflow
  3. Maintains Functionality: Preserves all existing flags (--prefer-offline, --no-audit, --no-fund, --ignore-scripts)
  4. Targeted Changes: Only modifies the problematic dependency installation steps
  5. Builds on Previous Work: Complements PR ci: add retry logic for npm install and skip scripts in license scan #295's retry logic

⚠️ Minor Concerns

  1. Documentation: The PR description could better explain the optional dependency architecture
  2. Error Messages: Updated error messages in license workflow correctly reference npm install instead of npm ci

Technical Details Verified

  • package.json:72-77 shows the optional native binary dependencies
  • Main CI workflow uses npm install with retry logic (lines 28, 64)
  • License workflow uses --ignore-scripts which is appropriate for metadata-only scanning
  • Benchmark workflows need the packages for actual benchmarking, so npm install tolerance is crucial

Final Recommendation

  • Rating: ⭐⭐⭐⭐ (4/5)
  • Action: APPROVE
  • Reasoning: This is a well-justified infrastructure fix that addresses a real problem with the optional dependency architecture. The changes are minimal, targeted, and maintain consistency with existing CI patterns. The approach is technically sound and the implementation is clean.

Recommendation

APPROVE - This PR correctly fixes a legitimate infrastructure issue without relaxing any safety measures or hiding problems.


@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 4, 2026

Greptile Summary

This PR replaces npm ci with npm install in the license compliance and benchmark workflows to handle missing optional dependencies gracefully (specifically the @optave/codegraph-* native binaries that are only published during releases). The change aligns these workflows with the main CI workflow which already uses npm install.

PR Convention Issue: This PR bundles two separate commits with different concerns:

  • Commit b6f55fa: Adds event filtering and duplicate PR prevention logic for benchmark workflows
  • Commit f4c3141: Changes npm ci to npm install

Only the second change is mentioned in the PR title and description. According to CLAUDE.md: "One PR = one concern. Each pull request should address a single feature, fix, or refactor. Do not pile unrelated changes into an existing PR." The duplicate PR prevention logic (commit b6f55fa) should be in a separate PR.

Technical Changes:

  • License workflow: Correctly replaces npm ci with npm install and updates error messages
  • Benchmark workflow: Replaces npm ci with npm install in all 4 jobs (build, embedding, query, incremental)
  • Additional changes (undocumented): Event filtering to prevent duplicate runs on stable releases + duplicate PR detection logic

The npm cinpm install change itself is correct and will fix the failing license compliance scans mentioned in PRs #298, #300, #307.

Confidence Score: 4/5

  • Safe to merge - the technical changes are correct and address the stated problem
  • Score reflects that while the code changes are technically sound and will fix the reported issues, the PR violates the repository's "One PR = one concern" convention by bundling two separate commits with different purposes
  • No files require special attention - the changes are straightforward and correct

Important Files Changed

Filename Overview
.github/workflows/shield-license-compliance.yml Replaced npm ci with npm install and updated error messages - correctly addresses optional dependency sync issues
.github/workflows/benchmark.yml Replaced npm ci with npm install in 4 jobs, plus added event filtering and duplicate PR prevention (undocumented changes)

Last reviewed commit: f4c3141

@carlos-alm carlos-alm merged commit d240352 into main Mar 4, 2026
16 checks passed
@carlos-alm carlos-alm deleted the fix/benchmark-duplicate-prs branch March 4, 2026 02:27
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant