Skip to content

Fix: fail the license check when it inspected no files - #701

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/check-license-fail-on-empty-file-list
Open

Fix: fail the license check when it inspected no files#701
AmaadMartin wants to merge 2 commits into
mainfrom
fix/check-license-fail-on-empty-file-list

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 6, 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):
    No existing issue; description below.
  2. Or, if no issue exists, describe the change:

Problem: scripts/check_license.sh can report success without inspecting a
single file. It discards find's exit status, so a broken search prints zero
paths and the script still prints ✅ All files have the correct license header.
and exits 0. It also iterates the file list unquoted, so a path containing a
space splits into fragments, and perl -0777 -ne exits 0 on a file it cannot
open — an unlicensed file at such a path is accepted silently.

Solution: The script now writes NUL-terminated paths to a temp file, checks
find's exit status, rejects an empty result, and iterates a quoted bash array.
The original report suggested set -o pipefail, but the final design contains no
pipeline, so an explicit status check replaces it and gives the stronger
guarantee. The two new errors go to stderr and are worded differently, so a
failing CI log says which guard fired. Output and exit status are unchanged on
the clean-pass and the missing-header paths.

The script stays bash-3.2 and BSD-find compatible for the macos-latest
runner: while IFS= read -r -d '' instead of mapfile, and mktemp with an
explicit template. A trap 'rm -f "$FILE_LIST"' EXIT removes the temp file on
every exit path.

The temp file is what makes find's exit status readable. A pipe or a process
substitution hides it behind the read loop's own status, and that status is the
only way to catch a find that aborts partway: it prints the paths it reached,
so a partial scan otherwise reads as a full one. The empty-result guard does not
cover that case, because the list is not empty.

MISSING is a flag rather than a list of paths, because the loop already prints
each offending path and nothing reads the collection back.

Collision check: I listed all 593 open PRs on the fork and searched the whole
fork history for commits touching this file. No PR implements this fix. #594
edits the same find line for a separate cleanup, is not an ancestor of main,
and has no upstream PR, so this change is based on main and leaves that line's
prune clauses exactly as they are.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:

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

New file tests/integration/scripts/check_license_test.ts runs the real script
against five scratch directories. Every fixture is written at runtime into an
fs.mkdtemp directory, because a committed unlicensed fixture would turn the
repository's own License Header Check red. The suite is skipped on Windows.

npx vitest run --project integration tests/integration/scripts/check_license_test.ts
Test Files  1 passed (1)      Tests  5 passed (5)

A .sh file is not instrumented by the v8 coverage provider and
coverage.include in vitest.config.ts lists only core/src, dev/src and
integrations/src, so this change does not move the coverage numbers. Each new
branch of the script gets a dedicated test case instead.

Proof that the tests can fail. I ran the suite against the unfixed script and
against three single-line mutations.

Mutation Tests that fail Reported failure
Whole script reverted to main 3 of 5 (empty result, space in path, find failure) expected '' to contain 'no .js or .ts files were found'
Drop the if ! find ... status check fails when find exits non-zero after printing a path expected 'find: ./denied: Permission denied\n/u…' to contain 'failed to list source files'
Drop the [ "${#FILES[@]}" -eq 0 ] guard fails when the search matches no .js or .ts file expected '' to contain 'no .js or .ts files were found'
Change for FILE in "${FILES[@]}" to for FILE in ${FILES[@]} checks a file whose directory name contains a space expected '🔍 Checking for license headers...\n✅…' to contain '❌ Missing or invalid license header: …'
Drop the MISSING=1 assignment 2 of 5 (missing header, space in path) expected '🔍 Checking…❌ Missing or invalid…' to contain 'Error: Some files are missing the required license header.'

The two pre-existing behaviours (clean pass, missing header) pass under every
mutation, which is what they are there to pin.

Manual End-to-End (E2E) Tests:

  1. Happy path unchanged. bash scripts/check_license.sh from the repository
    root: exit 0 in 1.5s, empty stderr, and stdout byte-identical to the same
    command run from main (diff of the two captures is empty).
  2. Broken search now fails. Delete the space before the escaped closing paren on
    the find line and run the copy: find's own diagnostic on stderr, then
    ❌ Error: failed to list source files; the license check did not run., exit
    1. The same mutation on main prints and exits 0.
  3. Violation path unchanged. A scratch directory holding one unlicensed .ts
    file gives the ❌ Missing or invalid license header: line, the separator, the
    summary line, and exit 1.
  4. No temp file leaks. ls "${TMPDIR:-/tmp}"/adk_license_files.* after all runs
    finds nothing.

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.

check_license.sh discarded find's exit status and iterated an unquoted
string, so two conditions produced a silent pass: a find that printed no
paths and exited non-zero still reported "All files have the correct
license header", and a path containing a space was split into fragments
that perl could not open, which also exits 0.

The script now writes NUL-terminated paths to a temp file, checks find's
exit status, rejects an empty result, and iterates a quoted bash array.
Output and exit status on the clean-pass and missing-header paths are
unchanged.
@AmaadMartin
AmaadMartin force-pushed the fix/check-license-fail-on-empty-file-list branch from a590314 to 1318a9e Compare August 6, 2026 07:06
Address the complexity review: replace the write-only MISSING_FILES array
with a flag, since the loop already prints each offending path, and drive
the test's script runs with spawnSync instead of a hand-rolled spawn and
Promise wrapper.

The comment above the find guard described the empty-result case, which a
different guard handles. It now describes the case the guard is for: find
prints the paths it reached before it aborts, so a partial scan reads as a
full one unless its exit status is checked.
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