Adopt graphql_client codegen for typed GraphQL queries

Cargo.toml

@@ -10,21 +10,55 @@ repository = "https://github.com/leynos/vk"
 
 [dependencies]
 clap = { version = "4.5.47", features = ["derive"] }
-reqwest = { version = "0.12.23", features = ["json", "rustls-tls"] }
 serde = { version = "1.0.196", features = ["derive"] }
 serde_json = "1.0.113"
 serde_path_to_error = "0.1.17"
-tokio = { version = "1.36.0", features = ["rt-multi-thread", "macros"] }
+tokio = { version = "1.36.0", features = ["rt-multi-thread", "macros", "time"] }
 termimad = "0.35.1"
 thiserror = "2.0.16"
 url = "2.5.0"
 regex = "1.10.3"
 diffy = "0.5.1"
 chrono = { version = "0.4.42", features = ["serde", "clock"] }
+graphql_client = "0.16"
 anyhow = "1.0"
 backon = "1.5.2"
 html5ever = "0.35.0"
+http = "1"
+# GraphQL transport stack (replaces reqwest). `hyper-util`'s legacy pooled
+# client over a `hyper-rustls` connector reproduces the connection pooling and
+# rustls TLS that reqwest supplied. The `server` feature on `hyper-util` and
+# the plain `hyper`/`http-body-util` crates are also exercised by the
+# integration tests, so a single runtime entry covers both uses.
+hyper = { version = "1", features = ["client", "http1"] }
+hyper-util = { version = "0.1", features = [
+    "client-legacy",
+    "http1",
+    "tokio",
+    "server",
+] }
+# webpki-roots + ring matches reqwest's `rustls-tls` feature, which expands to
+# `rustls-tls-webpki-roots` (Mozilla's bundled roots) with the ring provider;
+# see docs/adr-001-github-api-client-modernisation.md. Defaults are disabled to
+# keep aws-lc-rs out of the graph so ring remains the sole crypto provider.
+hyper-rustls = { version = "0.27", default-features = false, features = [
+    "ring",
+    "webpki-roots",
+    "http1",
+    "tls12",
+] }
+http-body-util = "0.1"
 markup5ever_rcdom = "0.35.0"
+# Patch updates within octocrab 0.54 are accepted; widening to 0.55 requires
+# review. The `retry` feature is deliberately excluded so the REST reply path
+# stays retry-free (see docs/adr-001-github-api-client-modernisation.md).
+octocrab = { version = "~0.54", default-features = false, features = [
+    "default-client",
+    "jwt-rust-crypto",
+    "rustls",
+    "rustls-ring",
+    "timeout",
+] }
 ortho_config = "0.8.0"
 base64 = "0.23.0"
 tracing = "0.1"
@@ -47,9 +81,6 @@ tokio = { version = "1.36.0", features = ["full"] }
 serde_json = "1.0"
 predicates = "3.1"
 mockall = "0.15.0"
-hyper = "1.6.0"
-hyper-util = { version = "0.1.17", features = ["server", "http1", "tokio"] }
-http-body-util = "0.1"
 bytes = "1"
 futures = "0.3"
 insta = { version = "1.43", features = ["redactions"] }

docs/adr-001-github-api-client-modernisation.md

@@ -0,0 +1,181 @@
+# Architectural decision record (ADR) 001: GitHub API client modernization
+
+## Status
+
+Accepted (2026-07-09). The project owner accepted the three-part programme
+after reviewing the ExecPlan draft: octocrab for the REST resolve path, a
+direct hyper transport inside the bespoke GraphQL client (removing reqwest), and
+`graphql_client` codegen for compile-time-checked GraphQL queries.
+
+## Date
+
+2026-07-09.
+
+## Context and problem statement
+
+`vk` is a command-line tool that shows unresolved GitHub pull request review
+comments. It talks to GitHub through two bespoke, hand-rolled clients built
+directly on the `reqwest` crate:
+
+- A GraphQL client (`src/api/client/`) used by every subcommand. It carries a
+  substantial observability investment: transcript recording of each request,
+  redacted error snippets in failure context, `backon` jittered-exponential
+  retry with transient-error classification (HTTP 5xx, HTTP 429, and
+  HTML-looking bodies), `serde_path_to_error` deserialization diagnostics, and
+  an environment-variable endpoint override (`GITHUB_GRAPHQL_URL`).
+- A feature-gated REST client (`src/resolve/rest.rs`, compiled only under the
+  `unstable-rest-resolve` feature) that posts review-comment replies with no
+  retry and its own environment-variable base-URL override (`GITHUB_API_URL`).
+
+The original decision to hand-roll these clients was never recorded, so the
+constraints that justified it are no longer legible to maintainers. The current
+arrangement carries four problems:
+
+- Two duplicated header-building and client-construction code paths that drift
+  independently.
+- No compile-time checking of GraphQL query strings: queries are raw `&str`
+  constants and operation names are recovered by string-sniffing the query text.
+- A dependency on `reqwest` for work that a single hyper stack could serve,
+  when the rest of the intended stack (hyper, rustls) is already present.
+- Bespoke REST plumbing (authentication, base-URI handling, header
+  construction) that duplicates what a maintained library already provides.
+
+The question this record settles is how to modernize GitHub API access without
+regressing the observable behaviour that the test suite pins: command output,
+error-message fragments, retry semantics, transcript format, authentication
+precedence, and the environment-variable endpoint overrides.
+
+## Decision drivers
+
+- Preserve the observability machinery. Transcript recording, error snippets,
+  and retry classification all depend on access to the raw HTTP response
+  (status and body), including partial-success GraphQL payloads that carry both
+  `data` and `errors`.
+- Preserve the test infrastructure. The environment-variable endpoint overrides
+  redirect the binary to loopback servers, and roughly twenty tests assert
+  exact error text; neither may change.
+- Converge on a single HTTP stack rather than maintaining two.
+- Gain compile-time validation of GraphQL queries and their variables.
+- Minimize bespoke plumbing by delegating maintained concerns to a library
+  where doing so does not compromise observability.
+- Keep TLS rustls-only; introduce no native-tls or OpenSSL dependency.
+- Remain compatible with the minimum supported Rust version (MSRV) of 1.89.
+
+## Options considered
+
+### Option 1: keep both bespoke clients (status quo)
+
+Retain the two `reqwest`-based clients unchanged. This preserves all behaviour
+at zero migration cost but resolves none of the problems: there is no
+compile-time query checking, the duplicated plumbing persists, and two HTTP
+paths remain.
+
+### Option 2: route all traffic through octocrab
+
+Adopt [octocrab](https://docs.rs/octocrab) as the single transport for both
+REST and GraphQL. Rejected: octocrab's `graphql()` helper hides the raw
+response, discarding the partial-success `data` and the response body that the
+transcript, error-snippet, and retry-classification machinery depend on. Its
+default builder accepts no custom middleware layers through which that
+machinery could be reattached, GraphQL cursor pagination remains hand-rolled
+regardless, and the escape-hatch `_post` method would become the primary
+interface. The compile-time-checking gain is marginal because octocrab does not
+type GraphQL queries itself.
+
+### Option 3: graphql_client codegen with reqwest retained
+
+Adopt `graphql_client` codegen for typed queries while keeping `reqwest` as the
+transport. This gains typed queries but keeps two HTTP stacks once octocrab
+arrives for the REST path, and retains the bespoke REST plumbing that a
+maintained library would otherwise absorb.
+
+### Option 4 (adopted): three-part split
+
+Separate the concerns by transport:
+
+- octocrab (tilde-pinned `~0.54`) serves the REST resolve path only, replacing
+  the bespoke `reqwest` REST client and inheriting maintained authentication
+  and base-URI plumbing.
+- A direct hyper transport (`hyper-util` legacy client plus `hyper-rustls`,
+  promoted from dev-dependencies to runtime dependencies) replaces `reqwest`
+  inside the bespoke GraphQL client, which retains its observability machinery.
+  `reqwest` then leaves the dependency graph.
+- `graphql_client` 0.16 codegen validates every query against GitHub's vendored
+  public schema
+  ([`schema.docs.graphql`](https://docs.github.com/public/fpt/schema.docs.graphql))
+  at compile time. Generated types stay private behind conversions to the
+  existing exported domain structs.
+
+| Dimension                     | Option 1 | Option 2 | Option 3 | Option 4 |
+| ----------------------------- | -------- | -------- | -------- | -------- |
+| Compile-time query checking   | No       | Marginal | Yes      | Yes      |
+| HTTP stacks after change      | Two      | One      | Two      | One      |
+| Observability preserved       | Yes      | No       | Yes      | Yes      |
+| Bespoke REST plumbing removed | No       | Yes      | No       | Yes      |
+| Maintained REST library       | No       | Yes      | Yes      | Yes      |
+
+_Table 1: Comparison of the four GitHub API client options against the decision
+drivers._
+
+## Decision outcome
+
+Adopt option 4, the three-part split. octocrab's value concentrates in its
+typed REST surface and maintained plumbing, whereas the bespoke GraphQL
+client's value is its observability, which octocrab's `graphql()` helper would
+discard. octocrab's own GraphQL example delegates query typing to
+`graphql_client`, confirming that the intended division of labour matches the
+tools' strengths.
+
+The programme is delivered as three pull requests, each behaviour-preserving
+and gated by the existing test suite:
+
+1. Adopt octocrab for the REST resolve path.
+2. Replace `reqwest` inside the GraphQL client with a hyper transport and
+   remove `reqwest` from the dependency graph.
+3. Adopt `graphql_client` codegen so malformed queries fail the build rather
+   than a runtime request.
+
+The REST-first ordering proves octocrab in-tree with the smallest blast radius;
+the transport change then removes `reqwest` before the largest change; the
+codegen change is type-level only and benefits from a settled transport beneath
+it.
+
+## Migration plan
+
+The migration is tracked as a living document in the ExecPlan
+[`docs/execplans/adopt-octocrab.md`](execplans/adopt-octocrab.md). Its numbered
+phases correspond to the three pull requests above, and it records the
+constraints, tolerances, risks, decision log, and per-phase acceptance criteria
+in detail. This record does not duplicate that content; consult the ExecPlan
+for the authoritative migration sequence and progress.
+
+## Known risks and limitations
+
+- octocrab has shipped breaking changes in patch releases (upstream issue 899).
+  The dependency is therefore tilde-pinned (`~0.54`) rather than caret-ranged,
+  with the reason recorded both here and in a `Cargo.toml` comment; widen only
+  after review.
+- The hyper transport must reproduce the pooling, TLS root-store, and
+  total-request timeout semantics that `reqwest` provided for free. A subtle
+  difference under failure could change behaviour; the retry and timeout unit
+  tests act as a characterization harness because the transport change alters
+  nothing else.
+- GitHub's vendored public schema is roughly 1.5 MB, and each
+  `#[derive(GraphQLQuery)]` re-parses it at compile time. The build-time impact
+  is bounded by the tolerance recorded in the ExecPlan; operations are grouped
+  per document to amortize the parse.
+- GitHub's custom scalars (`DateTime`, `URI`, `HTML`, and related types)
+  require Rust type aliases in scope of each derive. A shared `scalars` module
+  supplies them; a missing alias surfaces as an easily misread compile error.
+
+## Design for reuse
+
+The refitted GraphQL client is shaped for cheap future extraction into a shared
+crate. The sibling project frankie (a code-review terminal user interface,
+currently REST-only on octocrab) will need the same GraphQL review-thread
+surface, and the executor (transport, retry, transcript, typed operations, and
+cursor pagination) is the reusable part while query documents stay per-project.
+To keep extraction cheap, `vk`-specific coupling (`VkError` and
+`vk::environment`) is confined to the edges of the transport and typed modules
+rather than woven through them. Extraction itself is out of scope for this
+decision.

docs/contents.md

@@ -27,6 +27,13 @@
 - [Documentation style guide](documentation-style-guide.md): Use this for
   spelling, formatting, and document-structure rules.
 
+## Architecture decision records
+
+- [ADR 001: GitHub API client](adr-001-github-api-client-modernisation.md):
+  Records the accepted decision to serve REST through octocrab, move the
+  bespoke GraphQL client onto hyper transport, and add `graphql_client` code
+  generation for typed queries.
+
 ## Migration guides
 
 - [Ortho Config v0.6.0 migration guide](ortho-config-v0-6-0-migration-guide.md):
@@ -39,5 +46,9 @@
 
 - [Execution plans](execplans/): Use this directory for living implementation
   plans that need to survive context handoffs.
+  - [Adopt octocrab](execplans/adopt-octocrab.md):
+    Records the living plan to modernize GitHub API access: octocrab for REST,
+    hyper transport for the bespoke GraphQL client, and `graphql_client`
+    code generation for typed queries.
   - [Adopt Ortho Config v0.8.0](execplans/adopt-ortho-config-v0-8-0.md): Use
     this for the recorded plan behind the v0.8.0 configuration adoption work.

docs/developers-guide.md

@@ -83,6 +83,23 @@ Before extracting a helper, port, or abstraction, check whether one already
 exists and document the new ownership boundary in the relevant design or
 developer document.
 
+### REST resolve client
+
+REST review-comment replies use octocrab through its raw `_post` route. The
+`http` and `octocrab` entries in `Cargo.toml` are direct dependencies: `http`
+supplies the header types, while octocrab owns authentication, base-URI
+handling, and request execution. The octocrab `retry` feature remains excluded
+so the reply path stays retry-free.
+
+The connection timeout maps to octocrab's connect timeout. The request timeout
+configures octocrab's read and write timeouts and also wraps the complete
+`_post` operation, preserving a total deadline across connection, write, and
+response-read work.
+
+Status interpretation remains in `vk`: the resolve client warns and continues
+for HTTP 404, accepts other successful statuses, and maps every other non-2xx
+status to `VkError::RequestContext` with the route and status.
+
 ## Documentation maintenance
 
 Update documentation in the same branch as the behaviour it describes: