Add paginated GraphQL unresolved-thread query to code-review skill

[David Engel (David-Engel)]

Description

The code-review skill's "read what has already been said" step only listed REST endpoints, which don't expose review-thread resolution state. During the #617 review, a reviewThreads(first: 100) GraphQL query was used instead; the PR had 122 threads and the 3 unresolved ones were past the first page, so the review wrongly reported none.

This adds to .github/skills/code-review/SKILL.md step 3:

• A note that resolution state is only available via GraphQL, that GraphQL connections cap at 100 nodes per page with no warning, and that gh api graphql --paginate only pages when the query takes $endCursor and selects pageInfo{hasNextPage endCursor} (without them it silently stops at 100).
• A compact paginated gh api graphql snippet that lists unresolved review threads.

posting.md was left unchanged; its --paginate note covers verifying posted REST comments, and repeating the GraphQL guidance there would be redundant.

Verification (snippet extracted from SKILL.md and run in bash):

• PR 617: pages through all 122 threads, returns 0 unresolved (all now resolved).
• PR 685: returns its 6 unresolved threads. PR 639 also returned its 7 unresolved threads.
• Dropping pageInfo from the query on PR 617 reads only 100 threads, confirming the requirement.

Related Issues

Closes #688

Checklist

Documentation-only change to a skill file; no Rust code changed, so cargo validation was not run.

• cargo bfmt passes (N/A, no Rust changes)
• cargo bclippy passes (N/A, no Rust changes)
• cargo btest passes (N/A, no Rust changes)
• New/changed functionality has tests (N/A, docs only; snippet verified manually as above)
• Public API changes are documented (N/A)

[ttk (Theekshna)]

Unattended hourly review of PR #690 at 9eaa2d6e.

Covered all six checks. (1) msodbcsql parity: N/A — change touches only .github/skills/code-review/SKILL.md, no ODBC surface. (2) Test sufficiency: doc-only change with no automated test infra for prose; the PR's own verification steps are the "test" here — re-derived below. (3) Divergences documented: N/A — process/skill content, not driver behavior. (4) PR description currency: matches the diff exactly, closes #688 (verified the issue text and confirms the proposed change was implemented as asked), checklist N/A boxes are consistent with the diff (no Rust changed) and not contradicted by gh pr checks. (5) AI slop: none — both added paragraphs are information-dense, no filler, consistent with the file's existing terse style. (6) Evidence audit: re-ran the exact snippet the PR adds against all three PRs it cites.

Severity	Count
Blocking	0
Suggestion	0
Nit	1

Evidence re-derivation (check 6):

• PR #617, with pageInfo/$endCursor: 122 threads total across 2 pages, 0 unresolved — matches the PR's claim.
• PR #617, same query with pageInfo/endCursor stripped: exactly 100 nodes returned, silently truncated — matches the claimed failure mode this PR is documenting.
• PR #685: 6 unresolved threads returned — matches.
• PR #639: 7 unresolved threads returned — matches.

All four claims hold exactly as stated. One neighbor case the evidence doesn't cover: every verified PR needed at most 2 pages (122 threads = page 1 of 100 + page 2 of 22). A PR needing 3+ pages (>200 threads) to reach its unresolved threads was not exercised, so continued pagination past a single hasNextPage hop is asserted by --paginate's general behavior, not demonstrated here. Low risk since --paginate is gh's own generic mechanism, not bespoke code in this repo — noted as a Nit, not blocking.

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

[Copilot]

Copilot review overview

🟢 Approval recommended

The query correctly implements GraphQL cursor pagination and fulfills issue #688.

Review effort: Balanced
Findings: None

What changed in this PR

Adds reliable discovery of unresolved review threads to the code-review skill.

Changes:

• Documents REST/GraphQL resolution-state differences and pagination requirements.
• Adds a paginated GraphQL query for unresolved threads.

File	Description
.github/​skills/​code-review/​SKILL.md	Adds paginated unresolved-thread guidance and query.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Saurabh Singh (saurabh500)]

Unattended PR-review sweep using Copilot; posted after independent critique and live-state reconciliation.

Verdict: clean - no findings. This is not an approval. The paginated thread-state lookup complements the existing fully paginated REST discussion reads and addresses #688.

Blocking: None.

Suggestion: None.

Nit: None.

Evidence: Reviewed the complete one-file diff and prior discussion. Extracted the documented query and ran it through gh on #617: 122 unique threads across pages of 100 + 22; changing only the page size to 50 returned the same IDs across 50 + 50 + 22. Removing pageInfo truncated the result to 100 and failed the completeness assertion. The exact unresolved filter matched complete thread data in both empty (#690: 0) and nonempty (#679: 5) cases. Query/filter execution used argv, not the Bash snippet's shell parsing. No Rust/driver/database tests were run for this documentation-only change; the required ADO validation check is NEUTRAL/skipped.

Performance: No driver hot path is affected. Query pagination and field selection were inspected; no timing, allocation or throughput measurements were made, and no performance finding is claimed.

[Vahid Beiranvand (Vahid-b)]

Automated review — generated by GitHub Copilot on behalf of Vahid Beiranvand (@Vahid-b). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

No findings. This PR addresses #688 and appears ready for human review; it has not been approved.

The change is confined to review-tooling documentation, outside the driver runtime. Previously, step 3 documented complete REST discussion reads without a way to inspect thread resolution; the new GraphQL query supplements those reads with cursor-paginated resolution and outdated state. It keeps the root comment for context and filters unresolved threads across every page without removing any existing review rule.

Verification

• Reviewed head 9eaa2d6ed15f5285339ba78b1e4521106ca3a81c in the dedicated PR worktree against merge base eade56b76efce1b965c56edb77e675aabb1757e5. Fetched origin/main was 022d29bb186dc02a55912df202b7fee51535f9b6.
• Read the complete diff, linked issue, all three paginated discussion endpoints, and every review thread. Read .github/copilot-instructions.md, .github/instructions/pr-workflow.instructions.md, and .github/skills/code-review/SKILL.md plus posting.md from fetched origin/main, then checked the proposed skill change against them.
• Extracted the documented snippet and executed it in Bash with gh 2.101.0: PR 690 returned no unresolved threads; fixture PR 617 returned none; fixtures 685 and 639 returned 6 and 7 respectively. The nonempty filtered IDs matched complete unfiltered enumeration.
• Instrumenting the query with totalCount confirmed 122 unique fixture threads across pages of 100 + 22. Changing only first:100 to first:50 returned the same IDs across 50 + 50 + 22. Removing pageInfo returned only 100 and failed the completeness invariant. These were in-memory query variants; no repository files were modified.
• git diff --check eade56b76efce1b965c56edb77e675aabb1757e5..HEAD passed; the review worktree remained clean.
• Required CI is not failing: gh pr checks 690 --repo microsoft/mssql-rs --required exited 0. The exact-head required validation check is NEUTRAL/skipped, with the check-run explanation that no pipeline path filters matched. Other reported checks passed.

Round 1 examined the read-only query, shell argument handling, and data flow; no security findings. Round 2 examined pagination, filtering, surrounding workflow, rule preservation, and evidence sufficiency; no engineering findings. Driver parity is not applicable. Rust builds, driver tests, and runtime performance measurements were not run for this documentation-only change.

Coverage ledger

Area	Result
Primary logic	Examined: complete query and filter
Siblings	Examined: existing REST reads and posting guidance
Callers and implementers	Examined: step 3 workflow and gh api pagination contract
Diagnostics	Not applicable: no driver diagnostic changes
Tests	Examined: exact Bash snippet, empty/nonempty results, additional cursor hop, negative control
Build, packaging, pipelines	Examined: no build changes; CI and coverage path exclusions checked
Docs and comments	Examined: additive change preserves existing rules
Description and work items	Examined: #688 and all stated verification counts
CI evidence	Examined: required checks and exact-head check-run explanation

Blocking

None.

Suggestion

None.

Nit

None.

Severity	R1	R2	Total
Blocking	0	0	0
Suggestion	0	0	0
Nit	0	0	0

Deferred / to file

None.