Skip to content

ci: update the process.versions test hash in dep update workflows - #31954

Closed
robobun wants to merge 1 commit into
mainfrom
farm/2e23afd4/dep-update-workflows-test-hash
Closed

ci: update the process.versions test hash in dep update workflows#31954
robobun wants to merge 1 commit into
mainfrom
farm/2e23afd4/dep-update-workflows-test-hash

Conversation

@robobun

@robobun robobun commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

The dep auto-update workflows (c-ares, libarchive, libdeflate, lolhtml, lshpack, zstd) bump the commit in scripts/build/deps/<name>.ts but not the matching pinned hash in test/js/node/process/process.test.js, so every PR they open fails the process.versions test on all test lanes. The test pins these hashes on purpose (its comment says bumping a dep requires updating it too).

This bit #31315: the lshpack test lanes were red, and because the workflow regenerates its branch with add-paths limited to the dep file, a manually pushed test fix was wiped by the next force-push. It had to be re-applied three times.

Fix, in each of the six workflows:

  • the update step also rewrites the matching key in process.test.js (key ares for c-ares)
  • a grep right after verifies the new hash landed in the test file, so the job fails loudly if the test file's shape ever drifts instead of regressing to broken auto-PRs
  • add-paths includes the test file so the change is committed

Workflows for deps not pinned in the test (hdrhistogram, highway, sqlite3, root-certs, vendor) are unchanged.

How did you verify your code works?

Simulated the new step locally for each of the six keys against the real test file: the sed rewrites exactly one line and the grep passes. Also exercised the failure path: with the key renamed so the sed cannot match, the grep exits 1, which fails the step under set -euo pipefail. All six YAML files validated with a YAML parser.

@github-actions github-actions Bot added the claude label Jun 7, 2026
@robobun

robobun commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:00 AM PT - Jun 7th, 2026

@robobun, your commit 0b7edfb has 2 failures in Build #61253 (All Failures):

  • Linking two modules of different data layouts: '/var/lib/buildkite-agent/build/build/release/bun_rust.lto.o.tmp/regular-lto-flag-stub.bc' is '' whereas 'llvm-link' is 'e-m:e-p270:32:32-p271:32:32-p272:64:64-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128-Fn32' on 🐧 aarch64-musl - build-bun
  • Linking two modules of different data layouts: '/var/lib/buildkite-agent/build/build/release/bun_rust.lto.o.tmp/regular-lto-flag-stub.bc' is '' whereas 'llvm-link' is 'e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128' on 🐧 x64-musl - build-bun
  • Linking two modules of different data layouts: '/var/lib/buildkite-agent/build/build/release/bun_rust.lto.o.tmp/regular-lto-flag-stub.bc' is '' whereas 'llvm-link' is 'e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128' on 🐧 x64-musl-baseline - build-bun
  • test/cli/install/bunx.test.ts - code 1 on 🐧 13 x64-asan

🧪   To try this PR locally:

bunx bun-pr 31954

That installs a local version of the PR into your bun-31954 executable, so you can run:

bun-31954 --bun

@claude claude Bot 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.

LGTM — mechanical CI-only change that mirrors the existing sed pattern and matches the test file's current shape.

Extended reasoning...

Overview

Adds two sed/grep lines plus an add-paths entry to six scheduled dep-update workflows (update-{cares,libarchive,libdeflate,lolhtml,lshpack,zstd}.yml) so that when they bump the commit in scripts/build/deps/<name>.ts, they also bump the matching pinned hash in test/js/node/process/process.test.js. I verified the test file at lines 275–287 contains exactly the keys (ares, libarchive, libdeflate, lolhtml, lshpack, zstd) in the format the new sed expressions target: <indent>key: "<40-hex>",.

Security risks

None. $LATEST is already validated against ^[0-9a-f]{40}$ upstream in each workflow before it reaches the new sed line, so there's no injection vector into the replacement string. The workflows themselves only open PRs (which still require review/CI); they don't merge anything. No new permissions, secrets, or actions are introduced.

Level of scrutiny

Low. This is CI tooling that produces PRs for humans to review — worst case is the workflow step fails or the generated PR is wrong, both of which are caught before merge. The change is a copy-paste of the existing sed idiom already used one line above in each file, with a defensive grep -qF guard so drift in the test file fails the job loudly rather than silently producing broken PRs.

Other factors

No CODEOWNERS cover .github/workflows/. No prior reviewer comments to address. The bug-hunting system found nothing. The six edits are identical modulo the key name, and the one non-obvious mapping (c-ares → ares) is correct per the test file.

@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. test: derive process.versions expectations from scripts/build/deps #29295 - Also fixes process.versions test breakage on dep updates, but by deriving expected hashes from scripts/build/deps/*.ts at test time instead of patching the workflows

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 205d1ac1-f6ab-4212-89cf-118c1be7c172

📥 Commits

Reviewing files that changed from the base of the PR and between a988615 and 0b7edfb.

📒 Files selected for processing (6)
  • .github/workflows/update-cares.yml
  • .github/workflows/update-libarchive.yml
  • .github/workflows/update-libdeflate.yml
  • .github/workflows/update-lolhtml.yml
  • .github/workflows/update-lshpack.yml
  • .github/workflows/update-zstd.yml

Walkthrough

Six GitHub Actions workflows that update pinned dependencies now synchronize the dependency commits in test/js/node/process/process.test.js alongside their primary dependency files. Each workflow adds sed-based replacement and grep verification for the corresponding dependency, and includes the test file in the generated pull request.

Changes

Dependency version synchronization with test file

Layer / File(s) Summary
Sync test file updates across dependency workflows
.github/workflows/update-cares.yml, .github/workflows/update-libarchive.yml, .github/workflows/update-libdeflate.yml, .github/workflows/update-lolhtml.yml, .github/workflows/update-lshpack.yml, .github/workflows/update-zstd.yml
Each of the six dependency-update workflows now adds sed-based replacement logic in the "Update version if needed" step to synchronize the pinned commit in test/js/node/process/process.test.js with the latest detected SHA, followed by grep verification. The pull request creation step is updated to include test/js/node/process/process.test.js in the add-paths list so the test file is committed alongside the dependency update.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: updating process.versions test hashes across dependency update workflows.
Description check ✅ Passed The description follows the template with complete sections explaining what the PR does and how it was verified.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favor of #29295, which the duplicate check correctly flagged. It solves the same problem at the test layer by deriving the expected hashes from scripts/build/deps/*.ts, so dep bumps never need to touch the test and the update workflows need no changes at all. It is also the older PR and the approach its author chose.

The two fixes cannot both land: this PR's grep guard assumes the pinned key: "<hash>", lines exist in process.test.js, which #29295 removes, so merging both would fail every update workflow run.

#29295 was stale (merge conflict, a syntax error, and a regex that missed zlib.ts) and is now rebased and repaired, verified locally. If maintainers prefer the workflow-side approach after all, this can be reopened.

@robobun robobun closed this Jun 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant