From ca6b2719a1f9b5f3f2c8a61c023ca79b76c846a6 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:24:42 -0400 Subject: [PATCH 1/2] ci: target compile/test to touched .ql files, fix silent test-step stderr pr-compile.sh: replace unconditional full-language compile with a three-way gate. A PR that only touches leaf .ql files now gets a fast, strict per-file recompile instead of compiling every query in the language. Any change to a shared library (.qll anywhere, not just lib/), qlpack.yml/lockfile/suite metadata, or a dependency/CLI version bump (.codeqlversion, .release.yml) still triggers a full, strict recompile - this also fixes a latent gap where a deleted dependency file was silently ignored. .github/** changes get a lenient full compile (matching the prior no-PR-context behavior) plus a strict per-file recompile of any touched queries. ci.yml Test Queries step: mirror the same classification to scope codeql test run to just the .qlref file(s) that test the touched .ql file(s), resolved via each .qlref's authoritative query: line (not folder-name convention, which is unreliable - see CWE-089/ CWE-208 test layouts). Falls back to the full 4-way sliced suite for any non-fast-path change. Also fixes test-step stderr always being discarded on success: the child's progress output now goes to a real per-slice log file that is always printed after the process exits, instead of being captured via communicate() and only shown on failure. Upload test results now also picks up these log files for post-hoc debugging. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 84006984-666d-4926-bbb8-795d91f3b5ab --- .github/scripts/pr-compile.sh | 102 ++++++++++++++++++++++-------- .github/workflows/ci.yml | 114 +++++++++++++++++++++++++++------- 2 files changed, 168 insertions(+), 48 deletions(-) diff --git a/.github/scripts/pr-compile.sh b/.github/scripts/pr-compile.sh index ca18dd48..d7345414 100755 --- a/.github/scripts/pr-compile.sh +++ b/.github/scripts/pr-compile.sh @@ -3,48 +3,96 @@ set -euo pipefail PR_NUMBER=${1:-} LANGUAGE=${2} -# to stop recompiling all queries if multiple files are modified -LIBRARY_SCANNED=false -echo "[+] Compiling all queries in $LANGUAGE" -codeql query compile --threads=0 --check-only "./$LANGUAGE/" +run_lenient_full_compile() { + # No --warnings=error: today's tree has pre-existing deprecated-API warnings in a + # handful of files that were never caught because no PR-mode diff has ever touched + # all of them at once. Failing on that backlog would block CI-infra validation + # (workflow_dispatch, or a PR that only touches .github/**) on unrelated debt. + echo "[+] Compiling all queries in $LANGUAGE" + codeql query compile --threads=0 --check-only "./$LANGUAGE/" +} + +run_strict_full_compile() { + echo "[+] Compiling all queries in $LANGUAGE (strict)" + codeql query compile --threads=0 --check-only --warnings=error "./$LANGUAGE/" +} if [[ -z "$PR_NUMBER" ]]; then # No PR context (e.g. workflow_dispatch run directly on a branch) - there is no PR - # file list to walk. The plain compile above already covers every query in the - # language directory, matching what publish.yml itself requires to ship a pack - # (`codeql pack install`/`publish` - neither treats warnings as fatal). We deliberately - # do NOT re-run with --warnings=error here: today's tree has pre-existing deprecated-API - # warnings in a handful of files that were never caught because no PR-mode diff has ever - # touched all of them at once. Failing full-mode runs on that backlog would block CI-infra - # validation on unrelated debt. See the tracking issue for the plan to clean up the - # backlog and then make PR-mode itself trigger a full strict compile whenever a PR - # touches .codeqlversion or a codeql-pack.lock.yml (a dependency/CLI bump can change - # behavior across every query, not just the files literally edited). + # file list to walk, so always do the full, lenient compile. This matches what + # publish.yml itself requires to ship a pack (`codeql pack install`/`publish` - + # neither treats warnings as fatal). + run_lenient_full_compile echo "[+] No PR number provided - full compile above already covered $LANGUAGE. Done." exit 0 fi -for file in $(gh pr view "$PR_NUMBER" --json files --jq '.files.[].path'); do - if [[ ! -f "$file" ]]; then - continue +mapfile -t CHANGED_FILES < <(gh pr view "$PR_NUMBER" --json files --jq '.files.[].path') + +# A full compile of every query in $LANGUAGE is required whenever a changed file could +# plausibly affect the compilation of more than just itself: a shared library (.qll, +# wherever it lives - not just under lib/), qlpack.yml/lockfile/suite metadata, or a +# dependency/CLI version bump (.codeqlversion, .release.yml). Only a PR that touches +# nothing but leaf .ql files in $LANGUAGE gets the fast, targeted path below. +DEPENDENCY_CHANGED=false +LANG_FILE_SEEN=false +WORKFLOW_CHANGED=false +declare -a TOUCHED_QUERIES=() + +for file in "${CHANGED_FILES[@]}"; do + if [[ "$file" == ".codeqlversion" || "$file" == ".release.yml" ]]; then + echo "[+] $file changed - a dependency/CLI version bump can affect every query" + DEPENDENCY_CHANGED=true + elif [[ "$file" == "$LANGUAGE"/* ]]; then + LANG_FILE_SEEN=true + if [[ "$file" == *.ql ]]; then + TOUCHED_QUERIES+=("$file") + else + echo "[+] $file changed - not a leaf .ql file, compiling everything in $LANGUAGE" + DEPENDENCY_CHANGED=true + fi + elif [[ "$file" == .github/* ]]; then + # The CI script/workflow that drives this very compile step changed - be safe + # and validate the whole language rather than trusting the new logic blindly. + WORKFLOW_CHANGED=true fi +done - # if the file is a query file .ql or .qll - if [[ "$file" == $LANGUAGE/**.ql ]]; then - echo "[+] Compiling $file (in $LANGUAGE)" +if [[ "$DEPENDENCY_CHANGED" == true ]]; then + run_strict_full_compile + echo "[+] Complete" + exit 0 +fi - # compile the query +if [[ "$WORKFLOW_CHANGED" == true ]]; then + run_lenient_full_compile + # Also strict-compile any touched .ql files on top, matching what would have run + # anyway if only those files (and not .github/**) had changed. + for file in "${TOUCHED_QUERIES[@]}"; do + if [[ ! -f "$file" ]]; then + continue + fi + echo "[+] Compiling $file (in $LANGUAGE)" codeql query compile --threads=0 --check-only --warnings=error "./$file" + done + echo "[+] Complete" + exit 0 +fi - # if lib folder is modified - elif [[ "$file" == $LANGUAGE/lib/* ]] && [[ $LIBRARY_SCANNED == false ]]; then - echo "[+] Libray changed, compiling all queries in $LANGUAGE" - codeql query compile --threads=0 --check-only --warnings=error "./$LANGUAGE/" - # set LIBRARY_SCANNED to true to prevent recompiling - LIBRARY_SCANNED=true +if [[ "$LANG_FILE_SEEN" == false ]]; then + echo "[+] No compile-relevant changes for $LANGUAGE. Nothing to do." + exit 0 +fi +# Fast path: every changed file under $LANGUAGE/ is a leaf .ql file, so only those need +# a strict recompile - no need to touch the other untouched queries. +for file in "${TOUCHED_QUERIES[@]}"; do + if [[ ! -f "$file" ]]; then + continue fi + echo "[+] Compiling $file (in $LANGUAGE)" + codeql query compile --threads=0 --check-only --warnings=error "./$file" done echo "[+] Complete" \ No newline at end of file diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b5eccd37..13f0e022 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,12 +62,13 @@ jobs: - name: Test Queries if: steps.changes.outputs.src == 'true' || github.event_name == 'workflow_dispatch' env: + GITHUB_TOKEN: ${{ github.token }} RUNNER_TEMP: ${{ runner.temp }} shell: python run: | import os - import sys import subprocess + import sys from pathlib import Path def print_error(fmt, *args): @@ -78,33 +79,103 @@ jobs: sys.exit(1) runner_temp = os.environ['RUNNER_TEMP'] + language = "${{ matrix.language }}" + pr_number = "${{ github.event.number }}" + test_root = Path('${{ github.workspace }}', language, 'test') + + def get_pr_changed_files(): + # No PR context (e.g. workflow_dispatch run directly on a branch) - there is + # no PR file list to walk, so the caller always runs the full test suite. + if not pr_number: + return None + result = subprocess.run( + ["gh", "pr", "view", pr_number, "--json", "files", "--jq", ".files.[].path"], + capture_output=True, text=True, check=True, + ) + return [line for line in result.stdout.splitlines() if line] + + def find_targeted_qlrefs(changed_files): + # If this PR only touches leaf .ql file(s) in `language` (no shared library, + # pack metadata/lockfile, or CLI/dependency version-bump changes), return the + # .qlref file(s) that test those exact quer(y/ies) - which may be empty if none + # of the touched queries have test coverage. Otherwise return None, meaning + # "run the full test suite" (some other kind of change could affect any test). + touched_queries = [] + for f in changed_files: + if f in (".codeqlversion", ".release.yml") or f.startswith(".github/"): + return None + if f.startswith(f"{language}/"): + if f.endswith(".ql"): + touched_queries.append(f) + else: + return None + if not touched_queries: + return None + + src_prefix = f"{language}/src/" + touched_rel = set() + for q in touched_queries: + if not q.startswith(src_prefix): + return None + touched_rel.add(q[len(src_prefix):]) + + # .qlref files declare the query they test via a `query: ` + # line - this is the authoritative src<->test mapping (test folder names are + # only a loose, unreliable convention, not a formal one). + matches = [] + for qlref in test_root.rglob("*.qlref"): + for line in qlref.read_text().splitlines(): + line = line.strip() + if line.startswith("query:"): + if line.split(":", 1)[1].strip() in touched_rel: + matches.append(qlref) + break + return matches + + changed_files = get_pr_changed_files() + targeted_qlrefs = find_targeted_qlrefs(changed_files) if changed_files is not None else None - test_root = Path('${{ github.workspace }}', '${{ matrix.language }}', 'test') - print(f"Executing tests found (recursively) in the directory '{test_root}'") files_to_close = [] try: - # Runners have 4 cores, so split the tests into 4 "slices", and run one per thread - num_slices = 4 - procs = [] + if targeted_qlrefs is not None: + if not targeted_qlrefs: + print(f"[+] No tests reference the changed .ql file(s) in {language} - nothing to run") + report_path = os.path.join(runner_temp, language, "test_report_slice_1_of_1.json") + os.makedirs(os.path.dirname(report_path), exist_ok=True) + Path(report_path).write_text("[]") + sys.exit(0) + + print(f"[+] PR only touches leaf .ql file(s) in {language} - running {len(targeted_qlrefs)} targeted test(s) instead of the full suite") + slices = [(1, 1, [str(p) for p in targeted_qlrefs])] + else: + print(f"Executing tests found (recursively) in the directory '{test_root}'") + # Runners have 4 cores, so split the tests into 4 "slices", and run one per thread + num_slices = 4 + slices = [(n, num_slices, [f"--slice={n}/{num_slices}", str(test_root)]) for n in range(1, num_slices+1)] - for slice in range(1, num_slices+1): - test_report_path = os.path.join(runner_temp, "${{ matrix.language }}", f"test_report_slice_{slice}_of_{num_slices}.json") + procs = [] + for slice_num, total_slices, extra_args in slices: + test_report_path = os.path.join(runner_temp, language, f"test_report_slice_{slice_num}_of_{total_slices}.json") + test_log_path = os.path.join(runner_temp, language, f"test_log_slice_{slice_num}_of_{total_slices}.txt") os.makedirs(os.path.dirname(test_report_path), exist_ok=True) test_report_file = open(test_report_path, 'w') + test_log_file = open(test_log_path, 'w') files_to_close.append(test_report_file) - procs.append(subprocess.Popen(["codeql", "test", "run", "--failing-exitcode=122", f"--slice={slice}/{num_slices}", "--ram=2048", "--format=json", test_root], stdout=test_report_file, stderr=subprocess.PIPE)) - - for p in procs: - _, err = p.communicate() - if p.returncode != 0: - if p.returncode == 122: - # Failed because a test case failed, so just print the regular output. - # This will allow us to proceed to validate-test-results, which will fail if - # any test cases failed - print(f"{err.decode()}") - else: - # Some more serious problem occurred, so print and fail fast - print_error_and_fail(f"Failed to run tests with return code {p.returncode}\n{err.decode()}") + files_to_close.append(test_log_file) + procs.append((subprocess.Popen(["codeql", "test", "run", "--failing-exitcode=122", "--verbosity=progress", "--ram=2048", "--format=json", *extra_args], stdout=test_report_file, stderr=test_log_file), test_log_path)) + + for p, test_log_path in procs: + p.wait() + # Progress output goes to stderr by default - previously this was only + # printed on failure, so a normal passing run looked completely silent. + # Always surface it now. + log_text = Path(test_log_path).read_text() + if log_text: + print(log_text) + if p.returncode != 0 and p.returncode != 122: + # 122 just means a test case failed - validate-test-results will catch + # that from the JSON report. Anything else is a real crash - fail fast. + print_error_and_fail(f"Failed to run tests with return code {p.returncode}") finally: for file in files_to_close: file.close() @@ -116,6 +187,7 @@ jobs: name: ${{ matrix.language }}-test-results path: | ${{ runner.temp }}/${{ matrix.language }}/test_report_slice_*.json + ${{ runner.temp }}/${{ matrix.language }}/test_log_slice_*.txt if-no-files-found: error - name: Compile / Check Suites & Packs From 30cff715df7bf12c8e8ccafcb692b369355063c2 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:46:07 -0400 Subject: [PATCH 2/2] fix: close review gaps - unreachable dependency gate, print_error tuple bug The compile-and-test job's dorny/paths-filter only watched `${{ matrix.language }}/**` and `.github/**`, so a PR that only bumps .codeqlversion/.release.yml never set steps.changes.outputs.src and the whole Install Packs/Compile Queries/Test Queries sequence was skipped for every language on a real pull_request-triggered run - the DEPENDENCY_CHANGED full-recompile branch this PR adds was unreachable outside workflow_dispatch. Add both paths to the filter so a dependency/CLI version bump actually triggers full validation across every language, as intended. Also fix print_error_and_fail passing args as an unpacked tuple instead of *args - it printed a stray '()' after fatal error messages. Pre-existing bug carried over unchanged from main, but it's inside the block this PR rewrites, so fixing it here. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 84006984-666d-4926-bbb8-795d91f3b5ab --- .github/workflows/ci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 13f0e022..a20d25f5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,6 +35,8 @@ jobs: src: - '${{ matrix.language }}/**' - '.github/**' + - '.codeqlversion' + - '.release.yml' - name: Setup CodeQL if: steps.changes.outputs.src == 'true' || github.event_name == 'workflow_dispatch' @@ -75,7 +77,7 @@ jobs: print(f"::error::{fmt}", *args) def print_error_and_fail(fmt, *args): - print_error(fmt, args) + print_error(fmt, *args) sys.exit(1) runner_temp = os.environ['RUNNER_TEMP']