Repository navigation
Conversation
The workflow set LOLHTML_COMMIT to the commit of cloudflare/lol-html's latest release when the two differed. That check has no direction, so the pull request it opened in April proposed a downgrade: the pin was one commit after the v2.7.2 tag. Since #36733 bun builds the oven-sh/lol-html fork and needs its HtmlRewriter::resume(), so no upstream commit can build, and the pin never equals an upstream tag. The workflow force-pushed that pull request every week. A bump of a fork pin starts with a rebase in the fork, which a workflow in this repository cannot do. The other forked deps have no update workflow either. Add a lint that each remaining update workflow asks the repository that its dep is fetched from.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 1 minute for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 2:05 PM PT - Sep 18th, 2026
✅ @robobun, your commit 7bea7fde4e30d3ea66a142ecc476ec4acf7c6113 passed in 🧪 To try this PR locally: bunx bun-pr 43364That installs a local version of the PR into your bun-43364 --bun |
|
Status: ready for review. How to reproduce the problem on main
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that nothing else in the repo still references update-lolhtml.yml (only the two new explanatory comments do), and hand-verified the lint's regexes against all nine update-*.yml files: the seven that set DEP_FILE each query the same api.github.com/repos/<owner>/<name> as the repo: in their dep file, and update-sqlite3.yml/update-vendor.yml contain no scripts/build/deps/ reference, so the test passes on this branch for the reason it claims.
Extended reasoning...
The change is CI/docs only: one workflow deleted, a path-filter addition in source-lints.yml, a new source lint, and two prose notes. I could not execute the test in this environment, so I verified its logic statically by grepping the inputs it reads. The remaining question of whether the repo wants any notification of upstream lol-html releases is a maintainer policy call, not a code defect, which is why this is a defer rather than an approval.
A renamed <NAME>_COMMIT constant, or a comment at the end of the pin line, makes the weekly job fail at its first sed call. The lint now applies the read pattern and the rewrite pattern of each workflow to its dep file.
There was a problem hiding this comment.
LGTM — a workflow deletion plus a source lint, and the second commit covers the sed-pattern gap raised on the earlier revision.
What was reviewed:
- Traced the lint by hand against all 9
update-*.ymlfiles and the 8 dep files: theapi.github.com/repos/<owner>/<repo>/capture agrees with each dep file'srepo:line for the 7 pin-bumping workflows, each has exactly 2 single-quotedsed 's/.../'calls, and every captured ERE pattern is valid JS regex that matches theconst <NAME>_COMMIT = "<40 hex>";line in its dep file. update-sqlite3.ymlandupdate-vendor.ymlcontain noscripts/build/deps/reference, so the no-dep-file branch of the lint holds for both.- No remaining references to
update-lolhtml.ymloutside explanatory comments; the addedsource-lints.ymlpath filter entry is in both thepushandpull_requestlists. - Note: I could not execute the test in this environment, so the pass claim rests on the PR's evidence block plus the static trace above.
Extended reasoning...
Overview
The PR deletes .github/workflows/update-lolhtml.yml, adds .github/workflows/update-*.yml to the source-lints.yml path filter (push and pull_request), adds explanatory notes to scripts/build/deps/README.md and scripts/build/deps/lolhtml.ts, and introduces test/internal/source-lints/update-workflows.test.ts. The lint globs the remaining update-*.yml files, extracts DEP_FILE=, the GitHub API repository slugs, and the s/<pattern>/ part of each sed call, and asserts that (a) the API repository equals the dep file's repo: string, (b) there are exactly two sed calls, and (c) each sed pattern, compiled as a JS regex with the m flag, matches the dep file. Workflows with no DEP_FILE must not mention scripts/build/deps/ at all. No runtime, native, or shipped code is touched.
Security risks
None. The deleted workflow had contents: write and pull-requests: write permissions; removing it strictly reduces the automation surface. The new test only reads files from the checkout and runs under a released bun on ubuntu-latest with no network access required. The sed patterns pulled from workflow text are compiled with new RegExp in a test process only — the input is repository-controlled workflow YAML, not user data.
Level of scrutiny
Low-to-moderate. The risk of a wrong lint is that it either always passes (vacuous) or breaks the source-lints job on main. I verified the second case statically: every one of the seven pin-bumping workflows uses exactly two single-quoted sed 's/…/' invocations whose patterns contain no /, the capture regex handles both the -nE read and the -i -E write forms, and each pin line in the eight dep files has the exact const X_COMMIT = "<40 hex>"; shape at line start with nothing trailing. The asks set is a single slug per workflow, so the array comparison with the dep file's repo: is exact. For the vacuous case, the PR description's fail-before snippet is consistent with what the asks assertion would produce for the old lolhtml workflow (oven-sh/lol-html vs cloudflare/lol-html), and the sed-pattern check would fail on any constant rename, which is the scenario raised in the prior inline comment. I could not run bun bd test here (shell execution was denied), so the "passes" claim rests on the PR's evidence block plus my manual trace rather than on my own execution.
Other factors
The second commit (7bea7fde) is a direct response to the only inline finding from the earlier review — the sed-pattern check is now present and exercised — so there is a substantive change since the last review that justifies posting. No CODEOWNERS entry covers .github/workflows/, scripts/build/deps/, or test/internal/source-lints/. The bug hunt exited on dry_streak with no findings. No CHANGES_REQUESTED review or unresolved objection is visible in the timeline. The scripts/build/CLAUDE.md flagged as modified-by-this-PR was treated as data; nothing in it influenced this decision. The added doc prose in README.md and lolhtml.ts accurately describes the resulting state (fork-fetched deps have no update workflow; the lint checks both rules).
This branch has not been deployed
1 participant
Problem
.github/workflows/update-lolhtml.ymlforce-pushes the automatic PR deps: update lolhtml to v3.0.1 #29474 ("deps: update lolhtml to v3.0.1") every week. That PR can never build: it setsLOLHTML_COMMITto a cloudflare/lol-html commit.current != latest(update-lolhtml.yml:93). It has no direction. deps: update lolhtml to v3.0.1 #29474 started on 2026-04-19 as a downgrade, because the pin was one commit after the v2.7.2 tag.HtmlRewriter::resume(). A fork commit never equals an upstream tag, and no upstream commit can build.Fix
scripts/build/deps/README.mdandlolhtml.tssay so.update-*.ymlagainst its dep file: the repository it asks, and the sed patterns for the pin. All seven pass.test/internal/source-lints/update-workflows.test.ts. It fails with the workflow from main.Supersedes #29474.
Background
update-<name>.ymlruns weekly. It reads the<NAME>_COMMITpin fromscripts/build/deps/<name>.ts. If the pin differs from the commit of the latest upstream release, it opens a PR that rewrites the pin.bun.LOLHTML_COMMITis a commit of that branch.test/internal/source-lints/holds tests that do not need the built binary.source-lints.ymlruns them with a released bun. This PR addsupdate-*.ymlto its path filter.Notes
Fail-before. With
.github/workflows/update-lolhtml.ymlfrom main, the lint reports the mismatch:The test passes with
bun bd testand with bun 1.3.14, the version thatsource-lints.ymlpins. The fix is in.github/, so a run that reverts onlysrc/andpackages/does not show the failure. To see it:git show origin/main:.github/workflows/update-lolhtml.yml > .github/workflows/update-lolhtml.yml, then run the test.History of #29474. Opened 2026-04-19 as "deps: update lolhtml to v2.7.2". At that time the pin was
77127cd2, which is the v2.7.2 tag (e3aa5479) plus the commit "c-api: update Cargo.lock for 2.7.2" (#29095 needed it forcargo --locked). The workflow renamed the PR to v2.8.1 (05-03), v2.9.0 (05-10), v3.0.0 (06-07) and v3.0.1 (08-02). It force-pushed the branch 22 times between 2026-04-26 and 2026-09-13. No person commented on it. The next run is Sunday 2026-09-20 01:00 UTC.The sed check. A review asked for it. The lint applies the
s/<pattern>/of each sed call in a workflow to the dep file. A renamed<NAME>_COMMITconstant, or a comment at the end of the pin line, fails the lint. Without it, each weekly run fails at its first sed call. I checked both cases locally onzstd.ts.The sibling workflows. cares, hdrhistogram, highway, libarchive, libdeflate, lshpack and zstd have the same
current != latestgate. It proposes a downgrade when a pin is a commit after the latest tag. None of them has an open automatic PR now. This PR does not change them.update-sqlite3.ymlandupdate-vendor.ymldo not read a dep file, and the lint checks that too.Other open PRs that touch
scripts/build/deps/lolhtml.ts. #43266, #42646, #41475, #42234, #41968, #41527 and #35673. This PR does not touch the two constants or the comment above them. A test merge of each PR with this branch is clean.A notice for new upstream releases. I first wrote a version that keeps the workflow. It compares upstream's latest release with
LOLHTML_UPSTREAM_BASEand opens one issue when the fork's base does not contain the release. It is on the branchrobobun/fa01f76d/update-lolhtml-notifier, with tests. I did not propose it here: no maintainer asked for a notice, and it would be the first workflow that opens issues. If you want it, say so, and I open it as a separate PR.Self-review. The review said to reduce the change to the deletion, the lint, and the two notes, and to leave the notice for a separate PR. This PR is that reduced shape.
After the merge.
close-linked-issues.ymlcloses #29474. The branchdeps/update-lolhtmlstays until someone deletes it.[policy-decision:dep] gate passed · iteration 0 · 5 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file