Skip to content

Conversation

@mario-campos
Copy link
Contributor

@mario-campos mario-campos commented Oct 21, 2025

This pull request introduces two new instruction files that provide validation guides for change notes in library-packs and query-packs. These guides follow the format, required frontmatter, and content of Markdown change-note files as defined by the docs/change-notes.md guide. The only difference is that Copilot can act on these guides to provide actionable suggestions if it detects a violation in a change note.

Added a validation guide for query-pack change notes, including file naming conventions, frontmatter requirements, and examples of valid and invalid entries.
Added a validation guide for library-pack change notes, including file naming conventions, frontmatter requirements, and examples of valid and invalid entries.
@mario-campos mario-campos marked this pull request as ready for review October 21, 2025 21:22
@mario-campos mario-campos requested a review from a team as a code owner October 21, 2025 21:22
@Copilot Copilot AI review requested due to automatic review settings October 21, 2025 21:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds validation instructions for change note files in both query-packs and library-packs to enable Copilot to provide automated feedback during code reviews.

  • Introduces standardized validation guidelines for change note file names, frontmatter structure, and content format
  • Defines category-specific rules for query-pack and library-pack change notes
  • Provides clear examples of valid and invalid change note patterns

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
.github/instructions/query-pack-change-notes.instructions.md Defines validation rules for query-pack change notes including file naming, required frontmatter categories (breaking, deprecated, newQuery, queryMetadata, majorAnalysis, minorAnalysis, fix), and content formatting requirements
.github/instructions/library-pack-change-notes.instructions.md Defines validation rules for library-pack change notes with library-specific categories (breaking, deprecated, feature, majorAnalysis, minorAnalysis, fix) and identical file naming and formatting requirements

Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

As @esbena suggested, I think it's better to not repeat information already contained in change-notes.md, and just direct Copilot to reading that file, which lowers the probability we need to touch up these files. What I think is very valuable in these files though are the invalid examples, and what exactly is a library vs a query pack change note (as indicated by the applyTo field). For the query pack kind, I'd either specify it here or make it clearer in the change-notes.md file and let Copilot pick that up from there.

Another suggestion: we should probably specify a few more style guidelines in the change-notes.md file. Where it says

For consistency, change notes should use American English.

we can also add

They should be fully formed paragraphs, starting with a capital letter, correctly punctuated and ending with a period.

@owen-mc
Copy link
Contributor

owen-mc commented Oct 22, 2025

I strongly agree that if we can keep the specifications in one place then we should, to reduce the burden of updating it in two places in future.

A bit off topic so feel free to ignore this: another thing that we could specify more clearly (though this should probably go in the markdown guide) is how to refer to queries: I see both go/sql-injection and "Database query built from user-controlled sources" (go/sql-injection).

@mario-campos
Copy link
Contributor Author

I think it's better to not repeat information already contained in change-notes.md, and just direct Copilot to reading that file, which lowers the probability we need to touch up these files.

@redsun82 @owen-mc, I considered referring Copilot to docs/change-notes.md but I found its suggestions were less clear and more likely to refer you to the document rather than provide suggestions inline, which may make it less likely to offer "code" suggestions.

Consider the following screenshots of a file-naming violation:

Screenshot 2025-10-22 at 8 32 54 AM Screenshot 2025-10-22 at 8 33 11 AM

Another reason for this approach is that docs/change-notes.md contains categories for query packs and library packs, but they are not necessarily the same categories. I wonder if Copilot might be confused by these two sets of categories, and offer a "fix" when it's not needed (FP) or vice versa (FN).

@mario-campos
Copy link
Contributor Author

mario-campos commented Oct 22, 2025

Given the feedback above about duplicating docs/change-notes.md, I did some more testing. And the results were... not ideal. You will see that:

  • the instructions have been greatly simplified, referring to docs/change-notes.md as the guide for change-notes; it also tells Copilot to be clear about its review and to offer suggestions.
  • docs/change-notes.md has been revamped to be clearer and more structured, including more positive and negative examples.

And yet...

  • In one PR, Copilot didn't find anything wrong on the first attempt, and only when asked to re-review did it offer a suggestion—an erroneous suggestion that doesn't resolve the actual problem (invalid Markdown).
  • In another PR, Copilot didn't flag the invalid file name, not the first time nor the second time that I asked it to review.
  • The third PR actually worked as intended: Copilot found the violation and offered a valid suggestion 🎉
  • In the last PR, Copilot reviewed the change note, found a problem, and offered a suggestion; except the suggestion doesn't address the invalid category that I planted.

Perhaps the instructions or docs/change-notes.md guide could use more tweaking? Or maybe Copilot does not do well when deferred elsewhere.

@mario-campos
Copy link
Contributor Author

mario-campos commented Oct 23, 2025

For the sake of argument, I tested the reverse: I inlined the docs/change-notes.md into copilot-instructions.md, and tested against the same four PRs.

  • In the first PR, Copilot didn't find any problems on the first review. When prompted to review again, it offered a suggestion, but failed to address the original problem (incorrect Markdown).
  • In the second PR, I had Copilot review the PR three times and still it found nothing.
  • In the third PR, again Copilot reviewed the PR, found the intended problem, and offered a valid suggestion.
  • In the fourth and final PR, Copilot offered a suggestion only after requesting a second review. And its suggestion did not address the erroneous category.

All in all, it doesn't seem to make much of a difference whether the change-note guidelines are embedded directly into the Copilot instructions or elsewhere. The results seem subpar and inconsistent.

@owen-mc
Copy link
Contributor

owen-mc commented Oct 23, 2025

Thanks for doing that testing. That is disappointing. Do you think it is worth merging the PR anyway, in the hope that copilot will improve in future?

We could also give this as feedback so that people working on copilot could look into why it isn't performing well in this situation.

@mario-campos
Copy link
Contributor Author

Do you think it is worth merging the PR anyway, in the hope that copilot will improve in future?

I do, since it could still be helpful at least some of the time. And, as you said, it will likely improve.

Additionally, since it doesn't make much difference where the instructions live, it makes sense to not repeat ourselves. I'll adjust the PR to keep them in docs/change-notes.md and refer Copilot to that.

Move and consolidate change-note validation guidance into docs/change-notes.md, removing the separate query- and library-pack instruction files. Add a generic .github/instructions/change-notes.instructions.md (applyTo: "**/change-notes/*.md") that tells reviewers to ensure change notes follow the docs and to leave clear review comments and code suggestions when they do not.
@mario-campos
Copy link
Contributor Author

Additionally, since it doesn't make much difference where the instructions live, it makes sense to not repeat ourselves. I'll adjust the PR to keep them in docs/change-notes.md and refer Copilot to that.

@owen-mc, I've learned that Copilot Code Review does not support this ☝️ . To quote:

This is not supported. All of the instructions must exist in either the .github/copilot-instructions.md file or a *.instructions.md file inside of .github/instructions.

Given that, since we don't wish to duplicate information, perhaps it would be best to keep the information in .github/instructions/change-notes.instructions.md, and then have docs/change-notes.md refer to that file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants