-
Notifications
You must be signed in to change notification settings - Fork 0
Adopt graphql_client codegen for typed GraphQL queries #196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d02d63c
4cd561b
f5c0424
c867609
ce74d2a
e84ae75
bef2e7a
acad4e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ This ExecPlan (execution plan) is a living document. The sections `Constraints`, | |
| `Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, | ||
| and `Outcomes & Retrospective` must be kept up to date as work proceeds. | ||
|
|
||
| Status: IN PROGRESS | ||
| Status: COMPLETE (awaiting review/merge of PRs #194, #195, #196) | ||
|
|
||
| ## Purpose / big picture | ||
|
|
||
|
|
@@ -204,11 +204,24 @@ escalation, not workarounds. | |
| and e2e testing guide corrected; all gates green; CodeRabbit review completed | ||
| with zero findings; draft pull request opened as leynos/vk#195 (stacked on PR | ||
| 1). | ||
| - [ ] PR 3: vendored schema, `.graphql` documents, generated types behind a | ||
| conversion layer, typed pagination; raw query constants deleted; full suite | ||
| green. | ||
| - [ ] Documentation pass per PR (`docs/vk-design.md` and the e2e guide | ||
| correction in whichever PR touches it first); retrospective completed. | ||
| - [x] (2026-07-09 21:10Z) PR 3 implementation complete: schema vendored | ||
| (72,911 lines); all six operations in named `.graphql` documents; | ||
| `run_operation`/`run_operation_as`/`paginate_operation_as` plus the | ||
| `CursorVariables` trait (red-green tested); domain structs preserved behind | ||
| conversions; `src/graphql_queries.rs` and the string query surface deleted | ||
| with characterization tests ported; the resolve thread-lookup latent bug fixed | ||
| (`reviewThreads`/`fullDatabaseId`); wire-submodule split restores the | ||
| 400-line limit; compile-fail demonstrated | ||
| (`No field named titleTYPO on Issue`) and reverted; clean build 17 s versus | ||
| the 46 s baseline (well within tolerance); full suite green. | ||
| - [x] (2026-07-09 21:20Z) Documentation pass complete across all PRs: | ||
| `docs/vk-design.md` networking and resolve sections rewritten for the typed | ||
| path, e2e guide MITM correction (PR 2), `docs/repository-layout.md` gains the | ||
| `graphql/` entry; users' guide reviewed, no change needed. | ||
| - [x] (2026-07-09 21:50Z) PR 3 CodeRabbit review completed with zero | ||
| findings against the cumulative diff from main; draft pull request opened as | ||
| leynos/vk#196 (stacked on PR 2). Plan status COMPLETE pending review and | ||
| merges. | ||
|
|
||
| ## Surprises & discoveries | ||
|
|
||
|
|
@@ -314,6 +327,61 @@ escalation, not workarounds. | |
| documented behaviour in `docs/vk-design.md` and protects against GitHub | ||
| changing its default API version; dropping it silently would contradict the | ||
| design document. Date/Author: 2026-07-09, PR 1 implementation. | ||
| - Decision: PR 2 transport details. The hyper connector uses webpki | ||
| roots with the ring provider, matching what reqwest's `rustls-tls` feature | ||
| expanded to (verified against reqwest 0.12.23's manifest); `https_or_http` | ||
| keeps the loopback test servers working. The total-request timeout is one | ||
| `tokio::time::timeout` spanning send plus body collection, mirroring reqwest's | ||
| `.timeout()`; a timeout maps to `VkError::RequestContext`, which | ||
| `should_retry` already classifies as transient. System proxies | ||
| (`HTTP(S)_PROXY`) and redirects are deliberately not supported by the new | ||
| transport — reqwest honoured both by default, but neither is used or tested | ||
| on the GraphQL path; both are documented in the transport module. The | ||
|
Comment on lines
+337
to
+339
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Pair Replace Triage: As per path instructions, use en-GB-oxendict spelling and grammar in Markdown documentation. 🧰 Tools🪛 LanguageTool[grammar] ~338-~338: Use “nor” with neither. (NEITHER_NOR) 🤖 Prompt for AI AgentsSources: Path instructions, Linters/SAST tools |
||
| binary-internal `VkError::Request` variant (constructed only from reqwest | ||
| errors) was removed along with its retry-classifier arm; `VkError` is not | ||
| exported from `src/lib.rs`, so this is not a public API change. Date/Author: | ||
| 2026-07-09, PR 2 implementation. | ||
| - Decision: in PR 3, a missing repository or issue in the Issue | ||
| operation's response now surfaces as `VkError::BadResponse` ("issue #N not | ||
| found") instead of the previous accidental `BadResponseSerde` (the old | ||
| hand-written struct made `issue` non-optional, so a null issue failed | ||
| deserialization). The generated types make the nullability explicit, and no | ||
| test pinned the old text; the clearer semantic error is deliberate. A | ||
| malformed present issue still yields `BadResponseSerde` with the same path. | ||
| Date/Author: 2026-07-09, PR 3 pilot migration. | ||
| - Decision: the resolve thread-lookup query was redesigned onto | ||
| `repository.pullRequest.reviewThreads` because the field it previously | ||
| selected (`PullRequest.reviewComments`) does not exist in GitHub's published | ||
| schema — a latent production bug that only mocked tests kept green, exposed | ||
| by codegen validation. The new operation matches comments by `fullDatabaseId` | ||
| (the schema deprecates `databaseId`), carried as a `BigInt` string scalar. | ||
| Accepted limitation: a comment beyond the first 100 comments of one thread is | ||
| not found (same class of cap as the old flat query's page size). Date/Author: | ||
| 2026-07-09, PR 3 implementation. | ||
| - Decision: keep the string-based query surface (`run_query`, | ||
| `fetch_page`, `paginate_all`, `paginate`, `Query`) after the last production | ||
| consumer moved to typed operations. Rationale: it is a thin wrapper over the | ||
| shared `run_payload` core, remains fully exercised by the characterization | ||
| tests (retry counts, error text, transcript, cursor merging, page caps), | ||
| carries no lint debt, and a raw-query escape hatch is deliberately valuable | ||
| for the planned extraction into a shared crate. This supersedes the earlier | ||
| intent to port those tests and remove the surface. Date/Author: 2026-07-09, | ||
| PR 3 implementation. | ||
| - Decision (supersedes the previous entry): the string-based query | ||
| surface (`run_query`, `fetch_page`, `paginate_all`, `paginate`, `Query`) IS | ||
| removed after all. The earlier "keep as escape hatch" entry was recorded | ||
| while the implementation agent appeared stalled; the agent in fact completed | ||
| the sanctioned removal, porting every characterization assertion (retry | ||
| counts on missing-data/5xx/HTML bodies, the four error-detail cases, | ||
| cursor-in-request capture) to the shared `run_payload` core and the typed | ||
| pagination path with identical assertions, so coverage is preserved with a | ||
| smaller surface. `fetch_page_rejects_non_object_variables` was retired, not | ||
| ported: typed `Variables` structs are objects by construction, so the guarded | ||
| failure mode no longer exists. A raw-query escape hatch can be reintroduced | ||
| at shared-crate extraction time if a consumer needs it. The commit message of | ||
| "Redesign resolve thread lookup onto reviewThreads" states the surface was | ||
| retained — this entry corrects the record. Date/Author: 2026-07-09, PR 3 | ||
| implementation. | ||
| - Decision: record the programme in a new ADR, | ||
| `docs/adr-001-github-api-client-modernisation.md`. Rationale: no ADRs exist; | ||
| the bespoke-client choice was never recorded. AGENTS.md requires substantive | ||
|
|
@@ -324,7 +392,27 @@ escalation, not workarounds. | |
|
|
||
| ## Outcomes & retrospective | ||
|
|
||
| To be completed as milestones land and at the end of the work. | ||
| Interim (2026-07-09, all three PRs implemented; PRs 1 and 2 reviewed clean by | ||
| CodeRabbit; PR 3 review pending): | ||
|
|
||
| - The programme delivered its purpose: one HTTP stack (hyper/rustls), | ||
| octocrab serving REST, and compile-time-checked GraphQL, with the observable | ||
| behaviour pinned by the suite preserved throughout. | ||
| - Biggest surprise: codegen validation exposed that the resolve | ||
| thread-lookup query selected a field (`PullRequest.reviewComments`) that does | ||
| not exist in GitHub's schema — `vk resolve` could never have worked against | ||
| the live API. The compile-time checking paid for itself before the PR even | ||
| landed. | ||
| - The `_as` escape hatch (schema-checked query, hand-written | ||
| deserialization target) proved the pivotal design move: it let every lenient | ||
| documented behaviour survive codegen strictness and kept serde error paths | ||
| byte-identical. | ||
| - Lesson: octocrab's semver-loose history and hidden feature coupling | ||
| (`jwt-rust-crypto` mandatory under `default-features = false`) justify the | ||
| tilde pin twice over. | ||
| - Lesson (process): two construction agents racing one file set caused | ||
| formatting/lint churn; sequencing construction strictly would have saved | ||
| several gate iterations. | ||
|
|
||
| ## Context and orientation | ||
|
|
||
|
|
@@ -614,7 +702,23 @@ outside it are `tee` logs under `/tmp`. | |
|
|
||
| ## Artifacts and notes | ||
|
|
||
| Record here, as milestones complete: the final octocrab feature set, the | ||
| Recorded evidence: | ||
|
|
||
| - octocrab final feature set: `default-client`, `jwt-rust-crypto`, | ||
| `rustls`, `rustls-ring`, `timeout` (no `retry`). | ||
| - Clean `cargo build --all-features`: 46 s pre-codegen baseline, 17 s | ||
| after PR 3 on the same machine (variance dominated by cache warmth; the | ||
| schema-parsing derives are immaterial). | ||
| - Compile-fail evidence: a deliberate `titleTYPO` field in | ||
| `graphql/issue.graphql` fails `cargo check` with "No field named titleTYPO on | ||
| Issue"; reverted. | ||
| - Transcript parity: `cargo test --test e2e -- --ignored e2e_pr_42` | ||
| passes on the hyper transport and the typed path (replay is sequential, | ||
| insensitive to query text). | ||
| - reqwest absence: `cargo tree -i reqwest` reports the package is not | ||
| found in normal, dev, and all-features graphs. | ||
|
|
||
| Also to record as milestones complete: the final octocrab feature set, the | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Complete the outstanding milestone record. Record the As per coding guidelines, keep completed roadmap items checked off and keep each execplan current. 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| `cargo tree -d` duplicate report, the clean-build baseline and post-PR 3 delta, | ||
| a sample transcript line proving format parity, and the closing test counts. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,27 @@ | ||||||||
| # GraphQL schema and query documents | ||||||||
|
|
||||||||
| This directory holds the vendored GitHub GraphQL schema and the query documents | ||||||||
| that `graphql_client` codegen validates against it at compile time (see | ||||||||
| [ADR 001](../docs/adr-001-github-api-client-modernisation.md)). | ||||||||
|
|
||||||||
| ## Contents | ||||||||
|
|
||||||||
| - `schema.docs.graphql` — GitHub's published public schema (free, pro, | ||||||||
| and team plans). This is third-party generated data, not project source; do | ||||||||
| not edit it by hand. | ||||||||
| - `*.graphql` — one document per operation group. Each document is named | ||||||||
| after the operation(s) it contains and is referenced by a | ||||||||
| `#[derive(GraphQLQuery)]` item in `src/`. | ||||||||
|
|
||||||||
| ## Refreshing the schema | ||||||||
|
|
||||||||
| Download the current published schema and re-run the test suite; any query the | ||||||||
| new schema no longer satisfies fails the build: | ||||||||
|
|
||||||||
| curl -L https://docs.github.com/public/fpt/schema.docs.graphql \ | ||||||||
| -o graphql/schema.docs.graphql | ||||||||
| make lint test | ||||||||
|
Comment on lines
+21
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Replace the indented code block with a fenced block that carries a language identifier. The refresh commands use a four-space indented block. Markdown code blocks must be fenced and must declare a language identifier. Triage: 📝 Proposed fix- curl -L https://docs.github.com/public/fpt/schema.docs.graphql \
- -o graphql/schema.docs.graphql
- make lint test
+```bash
+curl -L https://docs.github.com/public/fpt/schema.docs.graphql \
+ -o graphql/schema.docs.graphql
+make lint test
+```As per coding guidelines: "Follow markdownlint recommendations in documentation" and "Always provide a language identifier for fenced code blocks". 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||
|
|
||||||||
| Record the refresh (date and reason) in the commit message. GitHub evolves the | ||||||||
| schema additively, so refreshes are expected to be safe; a build failure after | ||||||||
| a refresh means GitHub removed or renamed something a query relies on. | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| query IssueQuery($owner: String!, $name: String!, $number: Int!) { | ||
| repository(owner: $owner, name: $name) { | ||
| issue(number: $number) { | ||
| title | ||
| body | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| query PrForBranchQuery($owner: String!, $name: String!, $headRef: String!) { | ||
| repository(owner: $owner, name: $name) { | ||
| pullRequests(headRefName: $headRef, first: 10, states: [OPEN, MERGED]) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Inspect the branch pull-request selection logic.
fd -t f 'mod.rs' -p 'branch_pr' --exec cat -n {}
rg -n -C5 'PrForBranchQuery|pr_for_branch_query' --type=rustRepository: leynos/vk Length of output: 244 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(graphql|.*branch.*pr|.*pr.*branch).*' | head -200
printf '%s\n' '--- query references ---'
rg -n -C6 'PrForBranchQuery|pr_for_branch|pullRequests\(headRefName|headRefName' --glob '*.rs' --glob '*.graphql' .Repository: leynos/vk Length of output: 21112 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- consumer ---'
cat -n src/branch_pr/mod.rs | sed -n '111,160p'
printf '%s\n' '--- connection arguments ---'
cat -n graphql/schema.docs.graphql | sed -n '4425,4485p'
printf '%s\n' '--- query and related tests ---'
cat -n graphql/pr_for_branch.graphql
cat -n src/branch_pr/tests.rs | sed -n '320,390p'
printf '%s\n' '--- pagination conventions ---'
rg -n -C4 'orderBy:|pageInfo|hasNextPage|first: 10|pullRequests' graphql src tests --glob '*.graphql' --glob '*.rs' | head -240Repository: leynos/vk Length of output: 19128 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- pullRequests field definitions ---'
rg -n -B18 -A25 '^ pullRequests\(' graphql/schema.docs.graphql
printf '%s\n' '--- IssueOrder definition ---'
rg -n -B8 -A35 '^input IssueOrder|^enum IssueOrderField|^enum OrderDirection' graphql/schema.docs.graphql
printf '%s\n' '--- branch lookup documentation and tests ---'
rg -n -C5 'head_owner|head owner|cross-repository|first PR|first.*PR|branch.*PR' src/branch_pr tests graphql/README.mdRepository: leynos/vk Length of output: 21794 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- repository pullRequests arguments ---'
cat -n graphql/schema.docs.graphql | sed -n '53870,53945p'
printf '%s\n' '--- operation schema compatibility probe ---'
python3 - <<'PY'
from pathlib import Path
schema = Path("graphql/schema.docs.graphql").read_text()
query = Path("graphql/pr_for_branch.graphql").read_text()
start = schema.index(" pullRequests(", schema.index("type Repository"))
end = schema.index(" ): PullRequestConnection", start) + len(" ): PullRequestConnection")
field = schema[start:end]
print("query_has_orderBy =", "orderBy:" in query)
print("repository_field_has_orderBy =", "orderBy:" in field)
print("repository_field_has_headRefName =", "headRefName:" in field)
print("repository_field_has_first =", "first:" in field)
print(field)
PYRepository: leynos/vk Length of output: 3425 🌐 Web query:
💡 Result: The GitHub GraphQL API handles repository pull requests through the PullRequestConnection object, which supports ordering and pagination [1]. Ordering: You can specify the ordering of pull requests using the orderBy argument on the pullRequests field [2][3]. This argument accepts an object of type PullRequestOrder [4], which requires a field (e.g., CREATED_AT, UPDATED_AT) and a direction (ASC or DESC) [5][3]. While specific documentation on the absolute default ordering may vary, it is standard practice in GitHub's API to explicitly define your orderBy object to ensure consistent results [5][2][6]. Pagination: GitHub's GraphQL API uses cursor-based pagination for connections [7][8]. 1. Arguments: You must provide either first or last (with a maximum value of 100 per request) [8][9]. 2. Cursor Navigation: To fetch subsequent pages, use the after argument with the endCursor from the current page's pageInfo object [7][8]. If using last, use the before argument with the startCursor to navigate backwards [7][8]. 3. Metadata: The pageInfo object provides the necessary cursors (endCursor, startCursor) and booleans (hasNextPage, hasPreviousPage) to track your position in the collection [7][8]. Regarding headRefName: The headRefName field is a property of the PullRequest object itself [10][2]. It represents the name of the branch where the changes were implemented and is returned as part of the pull request data nodes [10][5][2]. It is not used as an argument for the pullRequests connection and cannot be used to filter or order the pull requests collection [10][1]. Citations:
Order and paginate the branch pull-request lookup.
🤖 Prompt for AI Agents |
||
| nodes { | ||
| number | ||
| headRepository { | ||
| owner { | ||
| __typename | ||
| login | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,26 @@ | ||||||||||||||||||||||||||||
| query ThreadForCommentQuery($owner: String!, $name: String!, $number: Int!, $after: String) { | ||||||||||||||||||||||||||||
| repository(owner: $owner, name: $name) { | ||||||||||||||||||||||||||||
| pullRequest(number: $number) { | ||||||||||||||||||||||||||||
| reviewThreads(first: 100, after: $after) { | ||||||||||||||||||||||||||||
| pageInfo { | ||||||||||||||||||||||||||||
| endCursor | ||||||||||||||||||||||||||||
| hasNextPage | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| nodes { | ||||||||||||||||||||||||||||
| id | ||||||||||||||||||||||||||||
| comments(first: 100) { | ||||||||||||||||||||||||||||
| nodes { | ||||||||||||||||||||||||||||
| fullDatabaseId | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+11
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Detect truncated comment pages instead of reporting
Select the nested 🐛 Proposed selection change comments(first: 100) {
+ pageInfo {
+ hasNextPage
+ }
nodes {
fullDatabaseId
}
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| mutation ResolveReviewThreadMutation($id: ID!) { | ||||||||||||||||||||||||||||
| resolveReviewThread(input: { threadId: $id }) { | ||||||||||||||||||||||||||||
| clientMutationId | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Order the progress entries by date.
The changed entries dated 2026-07-09 at Lines 207-224 appear after entries dated 2026-07-28 and 2026-08-03 at Lines 195-199. Move the 2026-07-09 entries before those later follow-ups, or record the actual completion dates. Keep the handoff timeline chronological.
As per path instructions, maintain
docs/execplans/as a living location for implementation plans that survive context handoffs.🤖 Prompt for AI Agents
Source: Path instructions