From d0d94ebe857bc6917bcf0cdbc10f594fa7a7d2e4 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Mon, 23 Feb 2026 13:20:22 +0100 Subject: [PATCH 1/7] Add contribution guidelines for pull requests to copilot review Added guidelines for reviewing PRs with copilot. --- .github/copilot-instructions.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .github/copilot-instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 000000000..a19d28f35 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,20 @@ +A change note is required for any pull request which modifies: + +- The structure or layout of the release artifacts. +- The evaluation performance (memory, execution time) of an existing query. +- The results of an existing query in any circumstance. + If only adding new rule queries, a change note is not required. + +Confirm that either a change note is not required or the change note is required and has been added. + +For PRs that add new queries or modify existing queries, also consider the following review checklist: + +- Confirm that the output format of shared queries is valid. +- Have all the relevant rule package description files been checked in? +- Have you verified that the metadata properties of each new query is set appropriately? +- Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases? +- Are all the alerts in the expected file annotated as NON_COMPLIANT in the test source file? +- Are the alert messages properly formatted and consistent with the style guide? +- Does the query have an appropriate level of in-query comments/documentation? +- Does the query not reinvent features in the standard library? +- Can the query be simplified further (not golfed!) From 38878e5d247ac52634511984823c568167310fc5 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Mon, 23 Feb 2026 13:26:02 +0100 Subject: [PATCH 2/7] Update copilot instructions for reporting checks --- .github/copilot-instructions.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index a19d28f35..194817ee2 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -18,3 +18,5 @@ For PRs that add new queries or modify existing queries, also consider the follo - Does the query have an appropriate level of in-query comments/documentation? - Does the query not reinvent features in the standard library? - Can the query be simplified further (not golfed!) + +Only report checks that failed From cd5940a5d9721d59d78761acaa67f4efed635dd2 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Mon, 23 Feb 2026 13:31:04 +0100 Subject: [PATCH 3/7] Update change note requirements and reporting language Clarified the requirement for change notes in pull requests and updated wording for reporting checks. --- .github/copilot-instructions.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 194817ee2..591079b3c 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,14 +1,12 @@ A change note is required for any pull request which modifies: - - The structure or layout of the release artifacts. - The evaluation performance (memory, execution time) of an existing query. - The results of an existing query in any circumstance. - If only adding new rule queries, a change note is not required. +If the pull request only adds new rule queries, a change note is not required. Confirm that either a change note is not required or the change note is required and has been added. For PRs that add new queries or modify existing queries, also consider the following review checklist: - - Confirm that the output format of shared queries is valid. - Have all the relevant rule package description files been checked in? - Have you verified that the metadata properties of each new query is set appropriately? @@ -19,4 +17,4 @@ For PRs that add new queries or modify existing queries, also consider the follo - Does the query not reinvent features in the standard library? - Can the query be simplified further (not golfed!) -Only report checks that failed +Only report checks that fail From 1b854ef15cb386a08ea5503d902337d3da6514f5 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Mon, 23 Feb 2026 13:48:30 +0100 Subject: [PATCH 4/7] Update .github/copilot-instructions.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .github/copilot-instructions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 591079b3c..9476bb6ef 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -17,4 +17,4 @@ For PRs that add new queries or modify existing queries, also consider the follo - Does the query not reinvent features in the standard library? - Can the query be simplified further (not golfed!) -Only report checks that fail +In your review output, list only those checklist items that are not satisfied or are uncertain, but also report any other problems you find outside this checklist; do not mention checklist items that clearly pass. From 7c0e8eb196786c20dad3af385ffa7dc115612d77 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Mon, 23 Feb 2026 13:48:40 +0100 Subject: [PATCH 5/7] Update .github/copilot-instructions.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .github/copilot-instructions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 9476bb6ef..d5e343f2d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -15,6 +15,6 @@ For PRs that add new queries or modify existing queries, also consider the follo - Are the alert messages properly formatted and consistent with the style guide? - Does the query have an appropriate level of in-query comments/documentation? - Does the query not reinvent features in the standard library? -- Can the query be simplified further (not golfed!) +- Can the query be simplified further (not golfed!). In your review output, list only those checklist items that are not satisfied or are uncertain, but also report any other problems you find outside this checklist; do not mention checklist items that clearly pass. From 83da8b3f4d27a8c1e520d07cfb8eac1c442ae51f Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Tue, 24 Feb 2026 10:47:11 +0100 Subject: [PATCH 6/7] Update .github/copilot-instructions.md Co-authored-by: Michael R Fairhurst --- .github/copilot-instructions.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d5e343f2d..222fb4acc 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,3 +1,11 @@ +--- +description: 'Code review guidelines for GitHub copilot in this project' +applyTo: '**' +excludeAgent: ["coding-agent"] +--- + +# Code Review Instructions + A change note is required for any pull request which modifies: - The structure or layout of the release artifacts. - The evaluation performance (memory, execution time) of an existing query. From c8ef2de3da6eba0ba57475d3e34dcb94d96a7905 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Tue, 24 Feb 2026 10:49:09 +0100 Subject: [PATCH 7/7] Update .github/copilot-instructions.md Co-authored-by: Michael R Fairhurst --- .github/copilot-instructions.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 222fb4acc..662931e2a 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -26,3 +26,34 @@ For PRs that add new queries or modify existing queries, also consider the follo - Can the query be simplified further (not golfed!). In your review output, list only those checklist items that are not satisfied or are uncertain, but also report any other problems you find outside this checklist; do not mention checklist items that clearly pass. + +## Validating tests and .expected files + +The test infrastructure for CodeQL that we use in this project involves the creation of a test directory with the following structure: +- Test root is `some/path/test/path/to/feature` (mirrors `some/path/src/path/to/query`) +- At least one test `c` or `c++` file, typically named `test.c`/`test.cpp`, with lines annotated `// COMPLIANT` or `// NON_COMPLIANT` +- A `.ql` file with test query logic, or a `.qlref` file referring to the production query logic +- A matching `FOO.expected` file to go with each `FOO.ql` or `FOO.qlref`, containing the test query results for the test `c` or `c++` files +- Note that some test directories simply have a `testref` file, to document that a certain query is tested in a different directory. + +As a code reviewer, it is critical to ensure that the results in the `.expected` file match the comments in the test file. + +The `.expected` file uses a columnar format: +- For example, a basic row may look like `| test.cpp:8:22:8:37 | element | message |`. +- For a query with `select x, "test"`, the columns are | x.getLocation() | x.toString() | "test" |` +- An alert with placeholders will use `$@` in the message, and have additional `element`/`string` columns for placeholder, e.g. `| test.cpp:8:22:8:37 | ... + ... | Invalid add of $@. | test.cpp:7:5:7:12 | my_var | deprecated variable my_var |`. +- Remember, there is one `.expected` file for each `.ql` or `.qlref` file. +- Each `.expected` file will contain the results for all test c/cpp files. +- The `toString()` format of QL objects is deliberately terse for performance reasons. +- For certain queries such as "path problems", the results may be grouped into categories via text lines with the category name, e.g. `nodes` and `edges` and `problems`. + +Reviewing tests in this style can be tedious and error prone, but fundamental to the effectiveness of our TDD requirements in this project. + +When reviewing tests, it is critical to: +- Check that each `NON_COMPLIANT` case in the test file has a row in the correct `.expected` file referring to the correct location. +- Check that each row in each `.expected` file has a `NON_COMPLIANT` case in the test file at the correct location. +- Check that there are no `.expected` rows that refer to test code cases marked as `COMPLIANT`, or with no comment +- Note that it is OK if the locations of the comment are not precisely aligned with the alert +- Check that the alert message and placeholders are accurate and understandable. +- Consider the "test coverage" of the query, are each of its logical statements effectively exercised individually, collectively? The test should neither be overly bloated nor under specified. +- Consider the edge cases of the language itself, will the analysis work in non-trivial cases, are all relevant language concepts tested here? This doesn't need to be exhaustive, but it should be thoughfully thorough.