Skip to content

fix: harden benchmark workflow against transient npm failures#99

Merged
carlos-alm merged 1 commit intomainfrom
fix/benchmark-resilience
Feb 26, 2026
Merged

fix: harden benchmark workflow against transient npm failures#99
carlos-alm merged 1 commit intomainfrom
fix/benchmark-resilience

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Add cache: "npm" to setup-node in all 4 benchmark jobs so packages are cached across runs
  • Switch npm installnpm ci --prefer-offline --no-audit --no-fund to prefer cached packages and skip unnecessary network requests
  • Fixes transient npm 403 errors (e.g. yargs-parser@21.1.1) that were failing the query-benchmark job

Note

The gh pr create permission error in build-benchmark is a repo settings issue — enable "Allow GitHub Actions to create and approve pull requests" under Settings > Actions > General.

Test plan

  • Re-run benchmark workflow via workflow_dispatch after merge
  • Confirm all 4 jobs pass the Install dependencies step

Add npm caching to setup-node and switch from bare `npm install` to
`npm ci --prefer-offline --no-audit --no-fund` in all 4 benchmark jobs.
This prevents transient 403s from the npm registry from failing the
workflow.
@claude
Copy link

claude bot commented Feb 25, 2026

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


PR Review in Progress

Review Tasks

  • Read and analyze the workflow changes
  • Phase 1: Root cause analysis
  • Phase 2: Configuration change detection
  • Phase 3: Test integrity assessment
  • Phase 4: Code quality review
  • Provide final rating and recommendation

Root Cause Analysis

Problem Being Solved: Transient npm registry 403 errors causing benchmark workflow failures, specifically affecting the query-benchmark job when trying to download packages like yargs-parser@21.1.1.

Why This Approach: The solution addresses the root cause by implementing two complementary strategies:

  1. npm caching: cache: "npm" in setup-node reduces registry requests by reusing cached packages
  2. Offline-first installation: npm ci --prefer-offline --no-audit --no-fund prioritizes cached packages and skips unnecessary network requests

Risk Assessment: ✅ LOW RISK - These are standard npm resilience best practices, not configuration workarounds.

Configuration Change Analysis

The changes are appropriate and well-targeted:

  • Added cache: "npm" to all 4 jobs (lines 31, 96, 173, 238) - Standard GitHub Actions caching
  • Changed npm installnpm ci (lines 34, 99, 176, 241) - Better for CI (deterministic, faster)
  • Added --prefer-offline --no-audit --no-fund flags:
    • --prefer-offline: Use cache first, fallback to registry
    • --no-audit: Skip security audit (appropriate for benchmark runs)
    • --no-fund: Skip funding messages (cleaner CI output)

No concerning relaxations detected - these changes strengthen reliability rather than weaken standards.

Test Integrity Assessment

No test integrity issues:

  • No test files modified
  • No assertions weakened
  • No coverage thresholds lowered
  • Changes are purely infrastructure/CI improvements

Code Quality Review

Strengths:

  • Consistent application across all 4 benchmark jobs
  • Uses npm ci instead of npm install (CI best practice)
  • Appropriate flags for CI environment
  • Addresses specific transient failure mentioned in issue

Minor Observations:

  • The PR description mentions a separate repo settings issue for "Allow GitHub Actions to create and approve pull requests" - this is correctly identified as unrelated to npm failures
  • Changes are minimal and focused solely on dependency installation resilience

Security: No security concerns - all changes are standard npm/GitHub Actions configurations.

Final Assessment

## Root Cause Analysis
**Problem Being Solved**: Transient npm registry 403 errors causing benchmark workflow failures
**Why This Approach**: Implements npm caching and offline-first installation - standard resilience practices
**Risk Assessment**: Low risk - strengthens CI reliability without compromising standards

## Critical Concerns
None identified. Changes follow CI best practices and address the specific transient failure issue.

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: APPROVE
- Reasoning: Well-targeted fix addressing genuine infrastructure issue with appropriate, low-risk solution. Uses CI best practices (npm ci, caching, offline-first). Consistent implementation across all jobs. No test integrity or security concerns.

The only reason this isn't 5 stars is that it's a straightforward infrastructure fix rather than exceptional code - but it's exactly the right solution for the problem.


@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 25, 2026

Greptile Summary

Hardens benchmark workflow against transient npm registry failures by adding cache: "npm" to all 4 setup-node steps and switching from npm install to npm ci --prefer-offline --no-audit --no-fund. This ensures npm preferentially uses cached packages and skips unnecessary audit/funding messages, reducing network dependency.

Key improvements:

  • Enables GitHub Actions npm caching on all 4 jobs (build-benchmark, embedding-benchmark, query-benchmark, incremental-benchmark)
  • Replaces npm install with npm ci --prefer-offline --no-audit --no-fund for faster, more deterministic installs
  • Adds descriptive step names for install steps (previously unnamed run commands)

The changes are consistent with best practices for CI/CD workflows and align with the existing pattern in shield-license-compliance.yml.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are straightforward CI/CD improvements that follow GitHub Actions best practices. Using npm ci with --prefer-offline is safer than npm install (ensures lockfile consistency), and adding npm caching reduces network calls. The pattern is already proven in shield-license-compliance.yml. No logic changes to code or benchmarks themselves.
  • No files require special attention

Important Files Changed

Filename Overview
.github/workflows/benchmark.yml Added npm caching and switched to npm ci with offline flags across all 4 benchmark jobs to reduce transient network failures

Last reviewed commit: 1b97fb9

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@carlos-alm carlos-alm merged commit 46848e9 into main Feb 26, 2026
23 of 26 checks passed
@carlos-alm carlos-alm deleted the fix/benchmark-resilience branch February 26, 2026 04:01
carlos-alm added a commit that referenced this pull request Mar 20, 2026
Add three Leiden-powered features: weighted community labels (#97),
hierarchical decomposition (#98), and community-aware impact scoring
(#99). Mark ID 96 (README dep count) as done after graphology removal.
Update ID 54 to reference Leiden instead of Louvain.
carlos-alm added a commit that referenced this pull request Mar 20, 2026
* docs: add community leverage features to backlog (IDs 97-99)

Add three Leiden-powered features: weighted community labels (#97),
hierarchical decomposition (#98), and community-aware impact scoring
(#99). Mark ID 96 (README dep count) as done after graphology removal.
Update ID 54 to reference Leiden instead of Louvain.

* fix(backlog): renumber community items to IDs 100-102, fix Leiden text

IDs 97-99 conflicted with existing entry #97 (unified multi-repo graph).
Renumbered to 100-102. Also corrected "Leiden/Louvain" to "Leiden" in
the section description for consistency with the rest of the PR.

* docs: rename Tier 1e′ to Tier 1e.1 for ASCII compatibility

* docs: fix premature Leiden references in backlog

The section description referenced src/graph/algorithms/leiden/ which
does not exist yet (pending PR #545). Item #96 was incorrectly marked
DONE when graphology is still a live dependency. Updated section to be
forward-looking and reverted #96 to BLOCKED on #545.

* docs: add #545 dependency to community leverage items 100-102

* docs: add #545 dependency to item #54 for Leiden consistency

* docs: mark #96 as SUPERSEDED instead of BLOCKED

Once #545 merges, the README's "3 runtime dependencies" claim becomes
correct automatically — no further action needed. BLOCKED implied
someone still needs to act on the item after #545 lands.
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.

1 participant