Skip to content

Fix: align the esbuild license banner with the repo's canonical Apache-2.0 header - #670

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/build-license-banner-indentation
Open

Fix: align the esbuild license banner with the repo's canonical Apache-2.0 header#670
AmaadMartin wants to merge 2 commits into
mainfrom
fix/build-license-banner-indentation

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

N/A

  1. Or, if no issue exists, describe the change:

Problem: core/build.js, integrations/build.js and dev/build.js each
define a licenseHeaderText banner that indents its comment body with two
spaces. Every other .ts/.js file in the repo uses one space, and so does the
perl pattern in scripts/check_license.sh. esbuild writes that banner verbatim
as the first bytes of every published artifact, so the header at the top of
dist/ does not match the shape the repo enforces on its own sources.

Solution: Re-indent the four interior lines of the literal to one space in
all three scripts. The three scripts stay standalone and dependency-free, so I
fixed the literals in place instead of extracting a shared constant — a
cross-workspace import to deduplicate a five-line string is a worse trade. The
banner is a comment, so no runtime behaviour changes.

Scope note: scripts/check_license.sh is unanchored, so it passes today. The
accurate claim is that the pattern anchored to the start of the file fails on
every artifact before this change and passes after it. Missing banners on
non-bundled ESM and web output are a separate defect and are out of scope here.

Collision check: I searched the 569 open PRs on this fork for adjacent work.
PRs #553, #555, #552 and #430 touch core/build.js and integrations/build.js,
but all of them edit the banner assembly logic, not the licenseHeaderText
literal. #482 touches dev/build.js but only removes a @ts-ignore. No PR
lands this change, so I branched from main.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

New test: tests/integration/build_scripts/license_header_test.ts. It reads
each of the three build scripts, extracts the licenseHeaderText literal, and
asserts the body matches the anchored, whitespace-exact equivalent of the
pattern in scripts/check_license.sh.

$ npx vitest run --project integration tests/integration/build_scripts/license_header_test.ts
 ✓ |integration| tests/integration/build_scripts/license_header_test.ts (3 tests) 6ms
 Test Files  1 passed (1)
      Tests  3 passed (3)

Mutation proof. I restored the two-space indent in core/build.js only and
re-ran the test. It fails and names that file:

 × Build script license banner > core/build.js defines the canonical Apache-2.0 header
   AssertionError: expected '/**\n  * @license\n  * Copyright 2026…' to match
   /^\/\*\*\n \* @license\n \* Copyright \d{4} Google LLC\n \* SPDX-License-Identifier: Apache-2\.0\n \*\/\n$/
 ✓ dev/build.js …
 ✓ integrations/build.js …

I also proved the missing-literal path. Renaming the constant in dev/build.js
fails that case with dev/build.js does not define licenseHeaderText. Both
scripts were restored afterwards.

The first push failed on windows-latest only. Git checks the build scripts
out with CRLF there, so the LF-exact pattern did not match. The test now
normalizes the line terminator before matching. I reproduced the failure
locally by converting core/build.js to CRLF, confirmed the fix passes, and
re-ran the mutation proof to confirm the normalization does not mask a
two-space indent.

Coverage: vitest.config.ts restricts coverage.include to
core/src, dev/src and integrations/src, so the build scripts record no
coverage and the thresholds do not move. The test exercises all three literals.

Manual End-to-End (E2E) Tests:

$ npm run build:bundle -w @google/adk
$ head -5 core/dist/cjs/index.js
/**
 * @license
 * Copyright 2026 Google LLC
 * SPDX-License-Identifier: Apache-2.0
 */

Applying the check_license.sh pattern anchored with \A to the bundled
artifacts, before and after the fix:

before                          after
core/dist/cjs/index.js NO MATCH   MATCH
core/dist/esm/index.js NO MATCH   MATCH
core/dist/web/index.js NO MATCH   MATCH

bash scripts/check_license.sh still exits 0
(All files have the correct license header).

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 5, 2026 01:55
The `licenseHeaderText` banner in all three build scripts indented its
comment body with two spaces, so the header at the top of every published
artifact did not match the shape `scripts/check_license.sh` enforces on
sources. Re-indent the literals to one space and pin the shape with a test.
Git checks the build scripts out with CRLF on Windows, so the LF-exact
pattern failed on the windows-latest CI runner. Normalize the line
terminator before matching; the assertion is about the indentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant