Serve REST resolve path through octocrab - #194
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
Validation
WalkthroughModernize GitHub API access by documenting a three-part migration, adding octocrab and http dependencies, replacing the REST reply client, preserving endpoint and status behavior, and updating architecture and execution-plan documentation. ChangesGitHub API modernisation
Sequence Diagram(s)sequenceDiagram
participant RestClient
participant Octocrab
participant GitHubREST
RestClient->>Octocrab: POST reply route with JSON body
Octocrab->>GitHubREST: Send authenticated request with API headers
GitHubREST-->>Octocrab: Return HTTP status
Octocrab-->>RestClient: Return response or request error
RestClient-->>RestClient: Map status, timeout, or request error
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
Record gates, zero-finding CodeRabbit review, and draft PR #194 in the progress log.
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideRefactors the REST resolve reply path to use an File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/execplans/adopt-octocrab.md" line_range="321" />
<code_context>
+
+## Context and orientation
+
+The repository is a single Rust crate (`vk`, edition 2024, MSRV 1.89) with
+sources under `src/` and integration tests under `tests/`. The `Makefile` is
+the canonical command runner. Read `AGENTS.md` before contributing.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Expand MSRV the first time it is used so the acronym is defined in this document.
MSRV is not expanded on first use here. Please change this to something like "minimum supported Rust version (MSRV) 1.89" so the acronym is defined inline, in line with the documentation instructions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 2
<location path="docs/execplans/adopt-octocrab.md" line_range="278" />
<code_context>
+ consumed by the printer and summary modules; the generated types are an
+ implementation detail of the wire format. Date/Author: 2026-07-09, planning
+ session.
+- Decision: design the refitted GraphQL client (PRs 2 and 3) for cheap
+ future extraction into a shared crate. Rationale: the sibling project frankie
+ (a code-review TUI, currently REST-only octocrab) will need the GraphQL-only
</code_context>
<issue_to_address>
**suggestion (review_instructions):** ADR is introduced here without expanding the acronym when it first appears in this ExecPlan.
ADR is mentioned here for the first time in this document, but the acronym is not expanded. Please update this line (or an earlier one) to introduce it as "architecture decision record (ADR)" before using the short form on its own.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 3
<location path="docs/execplans/adopt-octocrab.md" line_range="278" />
<code_context>
+ consumed by the printer and summary modules; the generated types are an
+ implementation detail of the wire format. Date/Author: 2026-07-09, planning
+ session.
+- Decision: design the refitted GraphQL client (PRs 2 and 3) for cheap
+ future extraction into a shared crate. Rationale: the sibling project frankie
+ (a code-review TUI, currently REST-only octocrab) will need the GraphQL-only
</code_context>
<issue_to_address>
**suggestion (review_instructions):** TUI is used later in this bullet without being defined when first mentioned.
Within this decision text, "TUI" is used without expansion. Please adjust the wording to introduce it as "terminal user interface (TUI)" the first time it appears so readers who are unfamiliar with the acronym can follow.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| ## Context and orientation | ||
|
|
||
| The repository is a single Rust crate (`vk`, edition 2024, MSRV 1.89) with |
There was a problem hiding this comment.
suggestion (review_instructions): Expand MSRV the first time it is used so the acronym is defined in this document.
MSRV is not expanded on first use here. Please change this to something like "minimum supported Rust version (MSRV) 1.89" so the acronym is defined inline, in line with the documentation instructions.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
| consumed by the printer and summary modules; the generated types are an | ||
| implementation detail of the wire format. Date/Author: 2026-07-09, planning | ||
| session. | ||
| - Decision: design the refitted GraphQL client (PRs 2 and 3) for cheap |
There was a problem hiding this comment.
suggestion (review_instructions): ADR is introduced here without expanding the acronym when it first appears in this ExecPlan.
ADR is mentioned here for the first time in this document, but the acronym is not expanded. Please update this line (or an earlier one) to introduce it as "architecture decision record (ADR)" before using the short form on its own.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
| consumed by the printer and summary modules; the generated types are an | ||
| implementation detail of the wire format. Date/Author: 2026-07-09, planning | ||
| session. | ||
| - Decision: design the refitted GraphQL client (PRs 2 and 3) for cheap |
There was a problem hiding this comment.
suggestion (review_instructions): TUI is used later in this bullet without being defined when first mentioned.
Within this decision text, "TUI" is used without expansion. Please adjust the wording to introduce it as "terminal user interface (TUI)" the first time it appears so readers who are unfamiliar with the acronym can follow.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dea8c9e433
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .set_connect_timeout(Some(connect_timeout)) | ||
| .set_read_timeout(Some(timeout)) | ||
| .set_write_timeout(Some(timeout)) |
There was a problem hiding this comment.
When a REST endpoint spends less than timeout in each individual connect, write, and read operation but more than timeout overall, these per-operation octocrab timeouts allow the reply request to exceed --http-timeout; the previous reqwest ClientBuilder::timeout bounded the whole request. Store the configured duration and wrap _post in an overall timeout, with slow-response coverage, so the CLI option retains its documented semantics.
AGENTS.md reference: AGENTS.md:L64-L74
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 29-33: Update the octocrab dependency declaration to use the
specifically reviewed exact patch version instead of the ~0.54 range, preserving
its existing feature configuration; alternatively, revise the adjacent comment
to accurately describe the intended version range, but ensure the dependency
constraint enforces the stated patch-review gate.
In `@docs/adr-001-github-api-client-modernisation.md`:
- Line 1: Standardize the Oxford spelling in the specified documentation: update
docs/adr-001-github-api-client-modernisation.md at lines 1, 43, and 59-60, and
docs/execplans/adopt-octocrab.md at lines 1, 18-22, 179-182, and 674-678,
replacing each -ise/-isation form with its -ize/-ization equivalent, including
modernisation, modernise, Minimise, Modernise, and modernised.
In `@docs/contents.md`:
- Around line 49-52: The phrase “a hyper transport for the bespoke GraphQL
client” uses an unnecessary article before the uncountable noun “transport.” In
docs/contents.md lines 49-52 and docs/vk-design.md lines 85-89, remove “a” so
both references read “hyper transport for the bespoke GraphQL client.”
In `@docs/execplans/adopt-octocrab.md`:
- Around line 431-433: Convert every indented documentation code block in
adopt-octocrab.md, including the referenced ranges, to standard fenced Markdown
blocks with appropriate language identifiers such as toml, shell, text, or rust;
preserve each block’s contents and surrounding prose.
- Around line 431-433: Update the octocrab dependency snippet in the PR 1
section to match the final Cargo.toml feature set by adding default-client and
jwt-rust-crypto to the features list, unless this block is explicitly labeled as
the pre-spike state.
- Around line 440-448: Rewrite the PR 1 steps to specify that the implementation
directly uses octocrab’s raw _post route with the hand-built review-comment
endpoint. Remove the typed-route-first, fixture-enrichment, and fallback
guidance, while retaining the required request path and existing status
semantics documented in tests/resolve.rs. Align the wording with the final
decision recorded in the Decision Log and Progress sections.
- Around line 674-682: Update the 2026-07-09 second-revision note to reflect
that PR 1 is complete, replacing the stale statement that implementation has not
begun and approval is pending. Preserve the recorded scope changes and describe
the remaining post-PR1 work accurately, using the PR status documented near the
existing PR 1 entry.
In `@docs/vk-design.md`:
- Around line 52-68: Update the specified spelling instances to use
en-GB-oxendict -ize forms: change “initialises” to “initializes” in
docs/vk-design.md lines 52-68, “modernisation” to “modernization” in
docs/contents.md lines 30-36 and docs/vk-design.md lines 85-89, and “modernise”
to “modernize” in docs/contents.md lines 49-52.
In `@src/resolve/rest.rs`:
- Around line 112-143: Add substantive stub-server coverage for the raw POST
flow used by the reply operation, ensuring tests execute the migrated transport
path rather than exiting early. Cover successful responses, 404 continuation,
and another non-2xx response; assert the exact route, JSON body, authentication,
and required GitHub headers for each request.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e448143-0630-42bb-ae2c-c7ef8b63127c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomldocs/adr-001-github-api-client-modernisation.mddocs/contents.mddocs/execplans/adopt-octocrab.mddocs/vk-design.mdsrc/resolve/rest.rs
| # Tilde pin: octocrab has shipped breaking changes in patch releases | ||
| # (upstream issue 899); widen only after 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 = [ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '24,38p' Cargo.toml
python3 - <<'PY'
from packaging.version import Version
# Cargo semantics for "~0.54" are >=0.54.0, <0.55.0
base = Version("0.54.0")
candidates = ["0.54.0", "0.54.9", "0.55.0"]
for v in candidates:
vv = Version(v)
print(v, base <= vv < Version("0.55.0"))
PYRepository: leynos/vk
Length of output: 788
Pin octocrab exactly if patch releases need review
Cargo.toml:29-33 — ~0.54 still allows every 0.54.x release, so it does not enforce the patch-review gate the note describes. Pin the reviewed version exactly, or rewrite the comment to match the range you actually want.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` around lines 29 - 33, Update the octocrab dependency declaration
to use the specifically reviewed exact patch version instead of the ~0.54 range,
preserving its existing feature configuration; alternatively, revise the
adjacent comment to accurately describe the intended version range, but ensure
the dependency constraint enforces the stated patch-review gate.
| @@ -0,0 +1,181 @@ | |||
| # Architectural decision record (ADR) 001: GitHub API client modernisation | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Standardize the Oxford spelling across both documents.
Replace the recurring -ise/-isation forms with the repository-required -ize/-ization forms.
docs/adr-001-github-api-client-modernisation.md#L1-L1: changemodernisationin the title.docs/adr-001-github-api-client-modernisation.md#L43-L43: changemodernise.docs/adr-001-github-api-client-modernisation.md#L59-L60: changeMinimise.docs/execplans/adopt-octocrab.md#L1-L1: changeModernise.docs/execplans/adopt-octocrab.md#L18-L22: changemodernise.docs/execplans/adopt-octocrab.md#L179-L182: changemodernised.docs/execplans/adopt-octocrab.md#L674-L678: changemodernisedandmodernisation.
Triage: [type:spelling] [type:docstyle]
🧰 Tools
🪛 LanguageTool
[style] ~1-~1: Would you like to use the Oxford spelling “modernization”? The spelling ‘modernisation’ is also correct.
Context: ...ion record (ADR) 001: GitHub API client modernisation ## Status Accepted (2026-07-09). The ...
(OXFORD_SPELLING_Z_NOT_S)
📍 Affects 2 files
docs/adr-001-github-api-client-modernisation.md#L1-L1(this comment)docs/adr-001-github-api-client-modernisation.md#L43-L43docs/adr-001-github-api-client-modernisation.md#L59-L60docs/execplans/adopt-octocrab.md#L1-L1docs/execplans/adopt-octocrab.md#L18-L22docs/execplans/adopt-octocrab.md#L179-L182docs/execplans/adopt-octocrab.md#L674-L678
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr-001-github-api-client-modernisation.md` at line 1, Standardize the
Oxford spelling in the specified documentation: update
docs/adr-001-github-api-client-modernisation.md at lines 1, 43, and 59-60, and
docs/execplans/adopt-octocrab.md at lines 1, 18-22, 179-182, and 674-678,
replacing each -ise/-isation form with its -ize/-ization equivalent, including
modernisation, modernise, Minimise, Modernise, and modernised.
Sources: Coding guidelines, Path instructions
| - [Adopt octocrab](execplans/adopt-octocrab.md): Use this for the living | ||
| plan to modernise GitHub API access — octocrab for REST, a hyper | ||
| transport for the bespoke GraphQL client, and `graphql_client` codegen | ||
| for typed queries. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop the article before the uncountable noun "hyper transport". "Transport" here is uncountable; "a hyper transport" reads awkwardly — write "hyper transport" instead.
Triage: [type:grammar]
docs/contents.md#L49-L52: change "a hyper transport for the bespoke GraphQL client" to "hyper transport for the bespoke GraphQL client".docs/vk-design.md#L85-L89: change "a hyper transport for the bespoke GraphQL client" to "hyper transport for the bespoke GraphQL client".
🧰 Tools
🪛 LanguageTool
[style] ~50-~50: Would you like to use the Oxford spelling “modernize”? The spelling ‘modernise’ is also correct.
Context: ...d): Use this for the living plan to modernise GitHub API access — octocrab for REST, ...
(OXFORD_SPELLING_Z_NOT_S)
[grammar] ~50-~50: Uncountable nouns are usually not used with an indefinite article. Use simply “hyper transport”.
Context: ... GitHub API access — octocrab for REST, a hyper transport for the bespoke GraphQL client, and `gr...
(A_UNCOUNTABLE)
📍 Affects 2 files
docs/contents.md#L49-L52(this comment)docs/vk-design.md#L85-L89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/contents.md` around lines 49 - 52, The phrase “a hyper transport for the
bespoke GraphQL client” uses an unnecessary article before the uncountable noun
“transport.” In docs/contents.md lines 49-52 and docs/vk-design.md lines 85-89,
remove “a” so both references read “hyper transport for the bespoke GraphQL
client.”
Source: Linters/SAST tools
| # Tilde pin: octocrab has shipped breaking changes in patch releases | ||
| # (upstream issue 899); widen only after review. | ||
| octocrab = { version = "~0.54", default-features = false, features = ["rustls", "rustls-ring", "timeout"] } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fence all documentation code blocks with language identifiers.
Convert the indented code blocks to fenced blocks using identifiers such as toml, shell, text, and rust. The documentation rules require standard Markdown code blocks with language identifiers.
Triage: [type:syntax/md] [type:docstyle]
Also applies to: 517-521, 535-542, 546-547, 607-642, 648-651, 655-658
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/execplans/adopt-octocrab.md` around lines 431 - 433, Convert every
indented documentation code block in adopt-octocrab.md, including the referenced
ranges, to standard fenced Markdown blocks with appropriate language identifiers
such as toml, shell, text, or rust; preserve each block’s contents and
surrounding prose.
Sources: Coding guidelines, Path instructions
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== adopt-octocrab snippets ==\n'
rg -n -A4 -B3 'octocrab\s*=' docs/execplans/adopt-octocrab.md Cargo.toml || true
printf '\n== around lines 420-440 in docs/execplans/adopt-octocrab.md ==\n'
sed -n '420,440p' docs/execplans/adopt-octocrab.md
printf '\n== around lines 600-620 in docs/execplans/adopt-octocrab.md ==\n'
sed -n '600,620p' docs/execplans/adopt-octocrab.md
printf '\n== Cargo.toml octocrab declaration ==\n'
rg -n -A6 -B2 '^octocrab\s*=' Cargo.toml || trueRepository: leynos/vk
Length of output: 4662
🌐 Web query:
octocrab 0.54 default-client jwt-rust-crypto feature requirements Cargo.toml
💡 Result:
In octocrab version 0.54.0, both default-client and jwt-rust-crypto are feature flags that are included by default [1][2]. Their configuration requirements in Cargo.toml are as follows: - default-client: This feature enables the hyper-util/client-legacy dependency [3][1]. It is defined in the [features] table as default-client = ["hyper-util/client-legacy"] [1]. - jwt-rust-crypto: This feature enables the jsonwebtoken/rust_crypto dependency [3][1]. It is defined as jwt-rust-crypto = ["jsonwebtoken/rust_crypto"] [1]. It is important to note that jwt-rust-crypto and jwt-aws-lc-rs are mutually exclusive features [3][1]. Octocrab includes a compile-time check that will trigger an error if both are enabled simultaneously [4]. Additionally, the crate requires that at least one of these two JWT features be enabled [4]. Both features are included in the default feature set [1][2].
Citations:
- 1: https://github.com/XAMPPRocky/octocrab/blob/master/Cargo.toml
- 2: https://crates.io/crates/octocrab
- 3: https://github.com/XAMPPRocky/octocrab/blob/main/Cargo.toml
- 4: https://docs.rs/crate/octocrab/latest/source/src/lib.rs
🔍 Additional research query:
Context7 octocrab 0.54 feature list default-client jwt-rust-crypto
💡 Result:
octocrab 0.54.0 has 19 feature flags, with 12 enabled by default. default-client enables hyper-util/client-legacy, and jwt-rust-crypto enables jsonwebtoken/rust_crypto; both are in the default feature set. The other default features are follow-redirect, retry, rustls, rustls-ring, timeout, and tracing. (docs.rs)
Align the PR 1 octocrab snippet with the final dependency set. Add default-client and jwt-rust-crypto here, or label this block as the pre-spike state; the current snippet looks final but omits the features used in Cargo.toml.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/execplans/adopt-octocrab.md` around lines 431 - 433, Update the octocrab
dependency snippet in the PR 1 section to match the final Cargo.toml feature set
by adding default-client and jwt-rust-crypto to the features list, unless this
block is explicitly labeled as the pre-spike state.
Source: MCP tools
| first attempts the typed route | ||
| (`octocrab.pulls(owner, name).reply_to_comment(...)` or the closest current | ||
| equivalent — verify the exact method against octocrab 0.54 during the spike); | ||
| it must preserve the semantics `tests/resolve.rs` asserts: exact request path, | ||
| 404 mapped to a warning and success, other non-2xx mapped to a fatal `VkError`. | ||
| If the typed model rejects the stub response bodies, enrich the stubs toward | ||
| realistic fixtures (`tests/fixtures/review_comment.json` exists for this); if | ||
| semantics still cannot be matched, fall back to `_post` with the hand-built | ||
| route and record the outcome in the Decision Log. Delete `github_client` and |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rewrite the PR 1 steps to match the adopted route.
Replace the typed-route-first/fallback instructions with the implemented decision to call octocrab’s raw _post route directly. The Decision Log at Lines 288-297 and Progress at Lines 188-194 record raw _post as the final choice because it preserves the existing path and status error contract.
🧰 Tools
🪛 LanguageTool
[style] ~441-~441: Consider using the typographical ellipsis character here instead.
Context: ..._reply first attempts the typed route (octocrab.pulls(owner, name).reply_to_comment(...)` or the closest current equivalent — v...
(ELLIPSIS)
[uncategorized] ~448-~448: The official name of this software platform is spelled with a capital “H”.
Context: ...the outcome in the Decision Log. Delete github_client and the hand-rolled header cons...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/execplans/adopt-octocrab.md` around lines 440 - 448, Rewrite the PR 1
steps to specify that the implementation directly uses octocrab’s raw _post
route with the hand-built review-comment endpoint. Remove the typed-route-first,
fixture-enrichment, and fallback guidance, while retaining the required request
path and existing status semantics documented in tests/resolve.rs. Align the
wording with the final decision recorded in the Decision Log and Progress
sections.
| 2026-07-09 (second revision): scope changed on user direction after review of | ||
| the first draft. octocrab is now confined to the REST resolve path; the bespoke | ||
| GraphQL client is retained for its observability machinery and moves from | ||
| reqwest to a direct hyper transport; `graphql_client` codegen adds compile-time | ||
| query checking. Delivery is three pull requests. The planned ADR was renamed | ||
| from `adr-001-adopt-octocrab.md` to | ||
| `adr-001-github-api-client-modernisation.md`. Constraints, tolerances, risks, | ||
| decisions, and interfaces were rewritten to match; no implementation has begun | ||
| and the plan awaits approval. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale revision note.
Lines 674-682 say that implementation has not begun and the plan awaits approval, but Lines 188-194 record PR 1 as complete. Record the actual post-PR1 state so this living plan remains reliable handoff documentation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~681-~681: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...en to match; no implementation has begun and the plan awaits approval.
(COMMA_COMPOUND_SENTENCE_2)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/execplans/adopt-octocrab.md` around lines 674 - 682, Update the
2026-07-09 second-revision note to reflect that PR 1 is complete, replacing the
stale statement that implementation has not begun and approval is pending.
Preserve the recorded scope changes and describe the remaining post-PR1 work
accurately, using the PR status documented near the existing PR 1 entry.
| posts a reply via the REST API before resolving. The REST reply is served | ||
| through [octocrab](https://docs.rs/octocrab) (see | ||
| [ADR 001](adr-001-github-api-client-modernisation.md)), pinning | ||
| `x-github-api-version: 2022-11-28`; status handling stays in `vk` via | ||
| octocrab's raw request route. If the REST reply fails the command aborts | ||
| without calling `resolveReviewThread`, does not retry, and does not apply | ||
| backoff; missing comments return a warning and continue. The resolver pages | ||
| through the pull request's `reviewComments` connection using typed `serde` | ||
| structures (see `src/resolve/graphql.rs`), matching the requested | ||
| `databaseId` and extracting the owning thread identifier. Pagination detects | ||
| repeated or non-advancing cursors and aborts with an error rather than | ||
| looping indefinitely. This subcommand requires `GITHUB_TOKEN` with sufficient | ||
| scopes (resolving threads and posting replies require `repo`); if absent, it | ||
| aborts rather than performing anonymous calls. Resolution steps emit debug | ||
| spans via `tracing` to aid diagnostics; the binary initialises | ||
| `tracing_subscriber::fmt()` with an environment filter, so running with | ||
| `RUST_LOG=vk=debug` (or a more specific filter) surfaces the spans on stderr. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Fix the -ise/-ize spelling drift across both docs. Path instructions mandate en-GB-oxendict spelling, which uses the -ize suffix, not -ise. Swap every instance flagged below.
Triage: [type:spelling]
docs/vk-design.md#L52-L68: change "initialises" to "initializes".docs/contents.md#L30-L36: change "modernisation" to "modernization".docs/contents.md#L49-L52: change "modernise" to "modernize".docs/vk-design.md#L85-L89: change "modernisation" to "modernization".
🧰 Tools
🪛 LanguageTool
[uncategorized] ~56-~56: Possible missing comma found.
Context: ...'s raw request route. If the REST reply fails the command aborts without calling `r...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~62-~62: Possible missing comma found.
Context: ...on-advancing cursors and aborts with an error rather than looping indefinitely. Thi...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~66-~66: Would you like to use the Oxford spelling “initializes”? The spelling ‘initialises’ is also correct.
Context: ...tracingto aid diagnostics; the binary initialises tracing_subscriber::fmt()` with an e...
(OXFORD_SPELLING_Z_NOT_S)
📍 Affects 2 files
docs/vk-design.md#L52-L68(this comment)docs/contents.md#L30-L36docs/contents.md#L49-L52docs/vk-design.md#L85-L89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/vk-design.md` around lines 52 - 68, Update the specified spelling
instances to use en-GB-oxendict -ize forms: change “initialises” to
“initializes” in docs/vk-design.md lines 52-68, “modernisation” to
“modernization” in docs/contents.md lines 30-36 and docs/vk-design.md lines
85-89, and “modernise” to “modernize” in docs/contents.md lines 49-52.
Sources: Path instructions, Linters/SAST tools
| let route = format!( | ||
| "/repos/{}/{}/pulls/{}/comments/{}/replies", | ||
| reference.repo.owner, reference.repo.name, reference.pull_number, reference.comment_id | ||
| ); | ||
| let url = rest.api.join(&path).map_err(|e| VkError::RequestContext { | ||
| context: format!( | ||
| "build reply URL for comment {} in repo {}/{}", | ||
| reference.comment_id, reference.repo.owner, reference.repo.name | ||
| ) | ||
| .boxed(), | ||
| source: Box::new(e), | ||
| })?; | ||
| #[cfg(test)] | ||
| rest.request_count.fetch_add(1, Ordering::SeqCst); | ||
| let res = rest | ||
| let response = rest | ||
| .client | ||
| .post(url) | ||
| .json(&json!({ "body": body })) | ||
| .send() | ||
| ._post(route.as_str(), Some(&json!({ "body": body }))) | ||
| .await | ||
| .map_err(|e| VkError::RequestContext { | ||
| context: "post reply".into(), | ||
| context: "post reply".boxed(), | ||
| source: Box::new(e), | ||
| })?; | ||
| if res.status() == StatusCode::NOT_FOUND { | ||
| let status = response.status(); | ||
| if status.as_u16() == 404 { | ||
| warn!( | ||
| "reply target not found (url={}): {}/{} comment {} in PR #{}", | ||
| res.url(), | ||
| reference.repo.owner, | ||
| reference.repo.name, | ||
| reference.comment_id, | ||
| reference.pull_number | ||
| "reply target not found (route={route}): {}/{} comment {} in PR #{}", | ||
| reference.repo.owner, reference.repo.name, reference.comment_id, reference.pull_number | ||
| ); | ||
| // Treat missing original comment as non-fatal: continue to resolve. | ||
| return Ok(()); | ||
| } | ||
| res.error_for_status() | ||
| .map(|_| ()) | ||
| .map_err(|e| VkError::Request(Box::new(e))) | ||
| if status.is_success() { | ||
| return Ok(()); | ||
| } | ||
| Err(VkError::RequestContext { | ||
| context: format!("post reply to {route}").boxed(), | ||
| source: Box::new(std::io::Error::other(format!( | ||
| "unexpected HTTP status {status}" | ||
| ))), | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add behavioural coverage for the raw POST contract.
Exercise success, 404 continuation, and another non-2xx response against a stub
server. Assert the route, JSON body, authentication, and required GitHub
headers. The current test exits before the migrated transport path runs.
As per coding guidelines, new functionality and behavioural changes must have
substantive tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/resolve/rest.rs` around lines 112 - 143, Add substantive stub-server
coverage for the raw POST flow used by the reply operation, ensuring tests
execute the migrated transport path rather than exiting early. Cover successful
responses, 404 continuation, and another non-2xx response; assert the exact
route, JSON body, authentication, and required GitHub headers for each request.
Source: Coding guidelines
Record the migration plan for replacing the two bespoke reqwest-based GitHub clients (the GraphQL `GraphQLClient` and the feature-gated REST `RestClient`) with `octocrab` 0.54 as the transport layer. The plan keeps the existing `GraphQLClient` facade, `VkError` taxonomy, `backon` retry, transcript recording, and the `GITHUB_GRAPHQL_URL` / `GITHUB_API_URL` endpoint overrides intact, swapping only the transport internals via octocrab's raw `_post` path. Work is staged as four milestones gated by the existing test suite, beginning with a go/no-go spike, and includes authoring `docs/adr-001-adopt-octocrab.md`. Link the plan from `docs/contents.md`. No implementation has begun; the plan awaits approval.
Revise the octocrab adoption plan following review: octocrab's `graphql()` helper hides the raw responses that `vk`'s transcript, error-snippet, and retry machinery depend on, so routing GraphQL through it traded away the bespoke client's observability for little gain, while its typed REST surface remains a genuine win. The programme is now three pull requests: 1. Adopt octocrab for the REST resolve path only. 2. Replace `reqwest` inside the bespoke GraphQL client with a direct hyper transport, converging the binary on one HTTP stack. 3. Adopt `graphql_client` codegen (schema vendored from GitHub's published SDL) so queries and variables are checked at compile time, behind a conversion layer that keeps the exported domain types stable. Rename the planned ADR to `adr-001-github-api-client-modernisation.md` and update the plan's constraints, tolerances, risks, decision log, and interfaces to match. No implementation has begun.
Record a decision to keep vk-specific coupling (`VkError`, `vk::environment`) at the edges of the refitted GraphQL client so the executor (transport, retry, transcript, typed operations, pagination) can later be extracted into a shared crate for reuse by frankie, which will need the GraphQL-only review-thread surface. Extraction remains out of scope for this plan.
Author `docs/adr-001-github-api-client-modernisation.md`, the first ADR in the repository, recording the accepted three-part programme: 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 queries. Link the ADR from `docs/vk-design.md` and a new "Architecture decision records" section in `docs/contents.md`. Add the `octocrab ~0.54` dependency (tilde-pinned per the ADR's known-risks section; octocrab has shipped breaking changes in patch releases). Default features are disabled in favour of `default-client`, `rustls`, `rustls-ring`, and `timeout`; the `retry` feature is excluded so the REST reply path stays retry-free. `jwt-rust-crypto` is included because octocrab fails to compile without one of its JWT crypto features even when app auth is unused — recorded as a discovery in the ExecPlan. Mark the ExecPlan as IN PROGRESS with Stage A complete.
Rework `src/resolve/rest.rs` so `RestClient` wraps an
`octocrab::Octocrab` instance instead of a bespoke `reqwest::Client`,
deleting `github_client` and the hand-rolled header constants. The
`pub(crate)` surface (`RestClient::new`, `post_reply`) and observable
semantics are unchanged: the reply POSTs to the same
`repos/{owner}/{name}/pulls/{n}/comments/{id}/replies` route, 404
remains a warn-and-continue, and other non-2xx statuses remain fatal
with the route and status code in the error text.
Use octocrab's raw `_post` rather than the typed reply route: the typed
path reports GitHub errors without the request path or status code that
`tests/resolve.rs` asserts, while `_post` returns the raw response and
keeps status handling in `vk`.
Restore the `x-github-api-version: 2022-11-28` pin and
`Accept: application/vnd.github+json` via the builder's `add_header`,
adding a direct `http` dependency (already in the tree through hyper).
The REST path's `User-Agent` becomes `octocrab` (not asserted by any
test; recorded in the ExecPlan decision log). reqwest's total-request
timeout maps to octocrab's read and write timeouts, the closest
available analogue.
Update the ExecPlan progress and decision log accordingly.
Note in the resolve-threads section that the REST reply is served through octocrab with the `x-github-api-version: 2022-11-28` pin and raw-route status handling, referencing ADR 001.
Record gates, zero-finding CodeRabbit review, and draft PR #194 in the progress log.
dea8c9e to
3b22846
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Enforce `--http-timeout` across the complete octocrab reply request while retaining its connect, read, and write limits. Strengthen the resolve harness to verify the raw request route, payload, authentication, required GitHub headers, status handling, and slow-response timeout. Reconcile the ADR, design notes, and ExecPlan with the implemented transport and documentation conventions.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr-001-github-api-client-modernisation.md`:
- Around line 163-166: Update the documentation sentence describing grouped
operations and schema parsing to use the required spelling “amortize” instead of
“amortise”; do not alter the surrounding explanation.
In `@docs/contents.md`:
- Around line 32-35: Update the ADR 001 entry in the documentation index:
shorten its link label or restructure the bullet so all wrapped lines stay
within 80 columns, and rewrite both descriptions as declarative statements
without “Use this” or second-person wording. Apply the same style correction to
the additional referenced entry.
In `@docs/execplans/adopt-octocrab.md`:
- Around line 183-187: Update the completed Stage A ADR entry in the adoption
plan to include jwt-rust-crypto in the recorded octocrab feature set, preserving
the existing default-features-disabled and other feature details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a83009a7-4099-405a-a397-78058433a21d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomldocs/adr-001-github-api-client-modernisation.mddocs/contents.mddocs/execplans/adopt-octocrab.mddocs/vk-design.mdsrc/resolve/rest.rstests/resolve.rs
| - 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 amortise the parse. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required Oxford spelling.
Replace amortise with amortize.
Triage: [type:spelling] [type:docstyle]
As per coding guidelines and path instructions, documentation must use
en-GB-oxendict spelling.
🧰 Tools
🪛 LanguageTool
[misspelling] ~164-~164: This word is normally spelled as one.
Context: ...B, and each #[derive(GraphQLQuery)] re-parses it at compile time. The build-time impa...
(EN_COMPOUNDS_RE_PARSES)
[style] ~166-~166: Would you like to use the Oxford spelling “amortize”? The spelling ‘amortise’ is also correct.
Context: ...perations are grouped per document to amortise the parse. - GitHub's custom scalars (`...
(OXFORD_SPELLING_Z_NOT_S)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr-001-github-api-client-modernisation.md` around lines 163 - 166,
Update the documentation sentence describing grouped operations and schema
parsing to use the required spelling “amortize” instead of “amortise”; do not
alter the surrounding explanation.
Sources: Coding guidelines, Path instructions, Linters/SAST tools
| - [ADR 001: GitHub API client modernization](adr-001-github-api-client-modernisation.md): | ||
| Use this for the accepted decision to serve REST through octocrab, move the | ||
| bespoke GraphQL client onto hyper transport, and add `graphql_client` codegen | ||
| for typed queries. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Conform the index entries to the documentation style.
Rewrite both descriptions declaratively rather than using “Use this”. Shorten the
ADR link label or restructure the entry so the bullet wraps at 80 columns.
Triage: [type:docstyle]
As per coding guidelines, Markdown bullets must wrap at 80 columns. As per path
instructions, Markdown must avoid second-person pronouns.
Also applies to: 49-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/contents.md` around lines 32 - 35, Update the ADR 001 entry in the
documentation index: shorten its link label or restructure the bullet so all
wrapped lines stay within 80 columns, and rewrite both descriptions as
declarative statements without “Use this” or second-person wording. Apply the
same style correction to the additional referenced entry.
Sources: Coding guidelines, Path instructions
| - [x] (2026-07-09 15:30Z) Stage A: ADR | ||
| `docs/adr-001-github-api-client-modernisation.md` authored and linked from | ||
| `docs/vk-design.md` and `docs/contents.md`; octocrab `~0.54` added to | ||
| `Cargo.toml` (default features off; `default-client`, `rustls`, `rustls-ring`, | ||
| `timeout`; `retry` excluded deliberately). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record the complete final feature set.
Add jwt-rust-crypto here. Cargo.toml and the later discovery record identify it
as mandatory when default-features = false.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/execplans/adopt-octocrab.md` around lines 183 - 187, Update the
completed Stage A ADR entry in the adoption plan to include jwt-rust-crypto in
the recorded octocrab feature set, preserving the existing
default-features-disabled and other feature details.
2 participants
Summary
This branch delivers the first of three planned pull requests modernising
vk's GitHub API access: the REST resolve path now ridesoctocrab instead of a bespoke
reqwestclient.It also carries the programme's foundations: the repository's first ADR,
docs/adr-001-github-api-client-modernisation.md,
recording the accepted three-part decision (octocrab for REST, a hyper
transport for the bespoke GraphQL client, and
graphql_clientcodegen fortyped queries), and the living ExecPlan.
Execplan: docs/execplans/adopt-octocrab.md
— PR 1 of its three phases lands here; the hyper transport (PR 2) and typed
GraphQL (PR 3) phases remain as authorized future work.
Observable behaviour is unchanged: the reply still POSTs to
repos/{owner}/{name}/pulls/{n}/comments/{id}/replies, a 404 remains awarn-and-continue, other non-2xx statuses remain fatal with the route and
status code in the error text, and
tests/resolve.rspasses without anyassertion change.
Review walkthrough
docs/adr-001-github-api-client-modernisation.md
for the decision context, options considered, and known risks.
src/resolve/rest.rs
—
RestClientnow wrapsoctocrab::Octocrabbuilt withbase_urifromthe existing
GITHUB_API_URLresolution,personal_tokenonly when atoken is present, restored
x-github-api-version: 2022-11-28andAcceptheaders, and the raw_postroute so status handling stays invk(the typed reply route reports errors without the path and statusfragments the tests assert).
Cargo.toml
for the tilde-pinned
octocrab ~0.54(patch releases have brokenbuilds upstream; see the ADR) with
retrydeliberately excluded so thereply path stays retry-free, plus the new direct
httpdependency(already in the tree via hyper).
docs/execplans/adopt-octocrab.md
for the decision log (raw-
_postrationale, timeout mapping,User-Agentnote) and the remaining phases.Validation
make check-fmt: passmake lint(cargo clippy --all-targets --all-features -- -D warnings): passmake test(RUSTFLAGS="-D warnings" cargo test --all-targets --all-features): 297 tests pass, 1 ignored (requires a recorded network transcript)cargo test --features unstable-rest-resolve --test resolve: 8 passedmake markdownlint/make nixie: passcoderabbit review --agent: completed, zero findingsNotes
User-Agentbecomesoctocrab(octocrab hard-codes it;no test asserts the REST user agent). GraphQL requests keep
User-Agent: vk.octocrab reply request; octocrab also receives read and write timeouts,
while
connect_timeoutmaps directly.reqwestitself remains in the dependency graph until PR 2, which movesthe GraphQL client onto a direct hyper transport and removes it.
Summary by Sourcery
Replace the bespoke reqwest-based REST resolve client with an octocrab-based implementation while keeping existing resolve semantics, and introduce documentation and planning artifacts for the broader GitHub API client modernisation.
Enhancements:
Build:
Documentation:
References