Add review-specs-update skill - #1978
Open
GromNaN wants to merge 5 commits into
Open
Conversation
Document the workflow for reviewing Dependabot PRs that bump the tests/specifications submodule: tracing DRIVERS-XXXX spec commits to PHPLIB/PHPC tickets, checking CI, skipping newly-broken tests with a ticket reference, and approving/merging the PR. Includes a read-only helper script to list submodule commits and their changed files.
Found while dry-running the skill on PR mongodb#1977: `jira issue comment add` takes the comment body as a positional argument, not a -b flag.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new agent-agnostic review skill for handling Dependabot PRs that bump the tests/specifications git submodule, along with a helper script to enumerate upstream spec commits and classify changed files to streamline review and triage.
Changes:
- Add
.agents/skills/review-specs-update/SKILL.mddocumenting a step-by-step review procedure (ticket tracing, CI verification, and when/how to skip tests). - Add
.agents/skills/review-specs-update/scripts/list-spec-commits.shto extract the submodule SHA range from a PR, list upstream commits, and categorize changed files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .agents/skills/review-specs-update/SKILL.md | Documents the process for reviewing tests/specifications submodule bump PRs. |
| .agents/skills/review-specs-update/scripts/list-spec-commits.sh | Provides a read-only CLI helper to list submodule bump commits and classify file changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+18
to
+20
| The `tests/specifications` git submodule tracks `mongodb/specifications`. A Dependabot PR that bumps it | ||
| (e.g. `Bump tests/specifications from `92b3c0b` to `1de749a` (#1977)`) usually bundles several upstream commits, each | ||
| referencing a `DRIVERS-XXXX` Jira ticket. Any spec change that requires driver-specific work is normally "split" into a |
Comment on lines
+22
to
+25
| DIFF=$(gh pr diff "$PR_NUMBER" --repo "$REPO") | ||
|
|
||
| OLD_SHA=$(echo "$DIFF" | grep -m1 -- "-Subproject commit" | awk '{print $3}') | ||
| NEW_SHA=$(echo "$DIFF" | grep -m1 -- "+Subproject commit" | awk '{print $3}') |
Comment on lines
+39
to
+41
| if ! git -C "$REPO_ROOT/$SUBMODULE_PATH" cat-file -e "$NEW_SHA" 2>/dev/null; then | ||
| git -C "$REPO_ROOT/$SUBMODULE_PATH" fetch origin --quiet | ||
| fi |
Found while running the skill on PR mongodb#1977: Jira does not render the owner/repo#N GitHub shorthand as a link, and the jira CLI has no comment-edit command. Also document posting a PR comment listing the PHPLIB/PHPC tickets, separate from the approval step.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
.agents/skills/review-specs-update/SKILL.md:20
- The inline-code example nests backticks (a code span containing
92b3c0b/1de749a), which breaks Markdown rendering. Use plain text for the full title and keep backticks only around the SHAs.
The `tests/specifications` git submodule tracks `mongodb/specifications`. A Dependabot PR that bumps it
(e.g. `Bump tests/specifications from `92b3c0b` to `1de749a` (#1977)`) usually bundles several upstream commits, each
referencing a `DRIVERS-XXXX` Jira ticket. Any spec change that requires driver-specific work is normally "split" into a
Comment on lines
+22
to
+26
| DIFF=$(gh pr diff "$PR_NUMBER" --repo "$REPO") | ||
|
|
||
| OLD_SHA=$(echo "$DIFF" | grep -m1 -- "-Subproject commit" | awk '{print $3}') | ||
| NEW_SHA=$(echo "$DIFF" | grep -m1 -- "+Subproject commit" | awk '{print $3}') | ||
|
|
Comment on lines
+39
to
+41
| if ! git -C "$REPO_ROOT/$SUBMODULE_PATH" cat-file -e "$NEW_SHA" 2>/dev/null; then | ||
| git -C "$REPO_ROOT/$SUBMODULE_PATH" fetch origin --quiet | ||
| fi |
- Scope the submodule diff parsing to tests/specifications only, so a PR touching multiple submodules doesn't pick up the wrong SHA range. - Fetch with --all instead of assuming a remote named "origin", and check both SHAs are present locally before fetching. - Don't let set -e abort on a grep with no match when extracting SHAs or DRIVERS tickets. - Fix a nested-backticks Markdown span in the SKILL.md example.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.agents/skills/review-specs-update/SKILL.md:20
- The example PR title code span is broken because it nests backticks inside an inline-code block, which will render incorrectly in Markdown.
The `tests/specifications` git submodule tracks `mongodb/specifications`. A Dependabot PR that bumps it
(e.g. ``Bump tests/specifications from `92b3c0b` to `1de749a` (#1977)``) usually bundles several upstream commits, each
referencing a `DRIVERS-XXXX` Jira ticket. Any spec change that requires driver-specific work is normally "split" into a
.agents/skills/review-specs-update/scripts/list-spec-commits.sh:26
- The script extracts the first "Subproject commit" lines from the whole PR diff. This repo has multiple submodules (e.g., tests/drivers-evergreen-tools, generator/mql-specifications), so this can pick the wrong SHA pair if another submodule is also updated in the PR.
DIFF=$(gh pr diff "$PR_NUMBER" --repo "$REPO")
# Scope to the diff hunk for this submodule only (a PR can touch several submodules,
# e.g. generator/mql-specifications, tests/drivers-evergreen-tools).
SUBMODULE_DIFF=$(echo "$DIFF" | awk -v path="$SUBMODULE_PATH" '
paulinevos
reviewed
Aug 31, 2026
| change-stream or connection test with no link to the changed specs) is more likely a pre-existing flake — flag it to | ||
| the operator and suggest re-running the job, rather than skipping it under Step 4a. | ||
|
|
||
| ## Step 4a — CI red: skip the newly-broken tests |
Contributor
There was a problem hiding this comment.
I feel like everything before this could be a script. Maybe something to ask it to generate, so we have a deterministic way of doing most of this stuff?
Comment on lines
+50
to
+51
| COMMITS=$(git -C "$REPO_ROOT/$SUBMODULE_PATH" log --oneline "$OLD_SHA..$NEW_SHA") | ||
|
|
Comment on lines
+63
to
+64
| - CI green: go to Step 5. | ||
| - CI red: find the failing tests, e.g. `gh run view --log-failed --repo mongodb/mongo-php-library <RUN_ID>`. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Pauline pointed out that the manual Jira lookup and CI check steps were just as mechanical as the commit listing, so fold them into the helper script (renamed list-spec-commits.sh -> review-spec-pr.sh) for a deterministic report. Judgment calls (unticketed commits, unrelated CI failures, skipping tests) stay documented in the skill.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
.agents/skills/review-specs-update/scripts/review-spec-pr.sh:84
- The commit-list loop will still run once even when
COMMITSis empty (e.g., if the submodule pointer moves backwards or the SHAs have noOLD..NEWrange), which makesgit show "$SHA"run with an empty SHA and abort the script underset -e.
Guard the loop so it only executes when COMMITS is non-empty, and print a helpful message otherwise.
echo "$COMMITS" | while IFS= read -r line; do
SHA=$(echo "$line" | cut -d' ' -f1)
echo "$line"
git -C "$REPO_ROOT/$SUBMODULE_PATH" show --name-status --pretty=format: "$SHA" | sed '/^$/d' | while IFS=$'\t' read -r status path new_path; do
Comment on lines
+27
to
+31
| Run the helper script: | ||
|
|
||
| ```bash | ||
| .agents/skills/review-specs-update/scripts/review-spec-pr.sh <PR_NUMBER> | ||
| ``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an agent-agnostic skill documenting how to review Dependabot PRs that bump the
tests/specificationsgit submodule: tracing DRIVERS-XXXX spec commits to PHPLIB/PHPC tickets, checking CI, skipping newly-broken tests with a ticket reference, and approving/merging the PR.The skill lives under
.agents/skills/review-specs-update/(excluded from release archives via the existing.* export-ignorerule in.gitattributes) and includes a read-only helper script,scripts/list-spec-commits.sh, that lists the commits in a submodule bump and classifies each changed file (spec, unified test, prose test, other) with its action (added/updated/deleted).Dry-run tested against PR #1977 with a fresh agent that had no prior context on the skill.