Add resolve subcommand to mark review comments resolved - #114
Conversation
Reviewer's GuideIntroduce a new "vk resolve" subcommand to post optional replies and mark pull request review threads resolved via GitHub APIs, including CLI dispatch, implementation module, documentation updates, and complete test coverage. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughAdd a resolve subcommand that posts an optional REST reply (when enabled) and then resolves a Pull Request review thread via GraphQL; implement resolver logic, add CLI args and dispatch, add docs, and add unit and e2e tests plus an httpmock dev-dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as vk CLI
participant Resolve as resolve::resolve_comment
participant REST as GitHub REST API
participant GraphQL as GitHub GraphQL API
rect rgba(240,248,255,0.6)
Note over CLI,Resolve: Read env (GITHUB_API_URL default https://api.github.com, GITHUB_GRAPHQL_URL override for tests)
end
User->>CLI: vk resolve <comment-ref> [-m MESSAGE]
CLI->>Resolve: resolve_comment(token, repo, comment_id, message)
alt message provided (feature enabled)
Resolve->>REST: POST /repos/{owner}/{repo}/pulls/{pull_number}/comments/{comment_id}/replies { body: MESSAGE }
REST-->>Resolve: 201/200
else no message (or feature disabled)
Note over Resolve: Skip REST reply
end
Resolve->>GraphQL: POST /graphql { mutation resolveReviewThread(threadId: base64("PullRequestReviewThread:{id}")) }
GraphQL-->>Resolve: 200 OK / data
Resolve-->>CLI: Ok(())
CLI-->>User: exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/vk-design.md (1)
200-203: Document both base-URL overrides together for discoverability.Call out the GraphQL override alongside the REST override.
- in the required parsers as transitive dependencies. The REST API base URL - defaults to `https://api.github.com` but can be overridden with - `GITHUB_API_URL` for testing. + in the required parsers as transitive dependencies. The GraphQL base URL + honours `GITHUB_GRAPHQL_URL` (default: `https://api.github.com/graphql`). + The REST API base URL defaults to `https://api.github.com` and can be + overridden with `GITHUB_API_URL` for testing.tests/utils.rs (1)
104-111: Replace forbidden #[allow] with narrowly scoped #[expect] lints.Project policy forbids
#[allow]. Use#[expect(..., reason = "...")]and keep scope tight.-#[allow( - clippy::missing_panics_doc, - clippy::must_use_candidate, - reason = "helper for integration tests" -)] -#[allow(dead_code, reason = "invoked by other test modules")] +#[expect(clippy::missing_panics_doc, reason = "helper for integration tests")] +#[expect(clippy::must_use_candidate, reason = "helper for integration tests")] +#[expect(dead_code, reason = "invoked by other test modules")]
♻️ Duplicate comments (1)
src/cli_args.rs (1)
91-102: Remove Default for a required field.Avoid creating an invalid “empty reference” state. Drop the manual Default impl for ResolveArgs; clap already enforces presence at parse-time, and OrthoConfig can still layer env/config over CLI.
Apply this diff:
-#[expect( - clippy::derivable_impls, - reason = "manual impl clarifies default empty reference" -)] -impl Default for ResolveArgs { - fn default() -> Self { - Self { - reference: String::new(), - message: None, - } - } -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)docs/vk-design.md(2 hunks)src/cli_args.rs(1 hunks)src/main.rs(8 hunks)src/resolve.rs(1 hunks)tests/resolve.rs(1 hunks)tests/utils.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Mandate caret requirements for all dependencies: All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3").
Prohibit unstable version specifiers: The use of wildcard (*), or open-ended inequality (>=) version requirements are strictly forbidden inCargo.toml. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Files:
tests/resolve.rstests/utils.rssrc/cli_args.rssrc/resolve.rssrc/main.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/resolve.rstests/utils.rssrc/cli_args.rssrc/resolve.rssrc/main.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/vk-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/vk-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/vk-design.md
🧬 Code graph analysis (3)
tests/resolve.rs (1)
tests/utils.rs (3)
start_mitm(49-98)vk_cmd(110-118)shutdown(30-33)
src/resolve.rs (2)
src/review_threads/tests.rs (1)
repo(16-21)src/test_utils.rs (2)
set_var(103-108)remove_var(113-118)
src/main.rs (2)
src/ref_parser.rs (1)
parse_pr_thread_reference(148-163)src/resolve.rs (1)
resolve_comment(71-99)
🔍 Remote MCP
• HTTP mocking dependency: The PR pins httpmock = "0.7" under [dev-dependencies]. Crate httpmock 0.7.0 was released on January 5, 2024; it is the latest stable 0.7.x series and provides a fully asynchronous core with both sync and async APIs, rich request matchers, parallel and standalone server modes, network delay simulation, and YAML-based mock specs.
• OrthoConfig derive on ResolveArgs: The ResolveArgs struct is annotated with #[derive(OrthoConfig)] and #[ortho_config(prefix = "VK")]. The OrthoConfig derive macro (from ortho_config_macros) generates a load-and-merge configuration method that layers CLI args, environment variables, and config files. The prefix attribute causes environment variables to be named VK_REFERENCE and VK_MESSAGE and maps CLI flags to the struct fields automatically.
• GitHub “resolve” endpoint availability: According to GitHub API references and community discussions, there is no official REST API endpoint for marking a pull-request review comment thread as resolved. That capability is only exposed via the GraphQL mutation resolveReviewThread; REST clients must otherwise fall back to GraphQL or add a reply comment. The PR’s use of PUT /repos/{owner}/{repo}/pulls/comments/{comment_id}/resolve is not documented in the REST API and may be unsupported or subject to change.
🔇 Additional comments (3)
tests/utils.rs (2)
102-103: LGTM: explain dual routing via MITM.The doc string clearly states GraphQL and REST interception.
113-114: LGTM: route REST traffic through the MITM via GITHUB_API_URL.This makes end-to-end assertions deterministic.
src/main.rs (1)
32-33: Ensuresrc/resolve.rshas a module-level doc comment.Project policy requires every module to start with a
//!comment.#!/bin/bash # Check for a leading module-level doc comment in src/resolve.rs head -n 5 src/resolve.rs rg -n '^\s*//!' src/resolve.rs || { echo "Missing module doc in src/resolve.rs"; exit 1; }
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
Cargo.toml (1)
49-49: Retain httpmock; it is used by unit tests.The dev-dependency is exercised in src/resolve.rs tests; the earlier request to remove it is now obsolete.
src/resolve.rs (4)
84-85: Normalise API base URL to avoid double slashes.Trim any trailing slash when reading
GITHUB_API_URL.- let api = env::var("GITHUB_API_URL").unwrap_or_else(|_| "https://api.github.com".into()); + let api = env::var("GITHUB_API_URL") + .unwrap_or_else(|_| "https://api.github.com".into()) + .trim_end_matches('/') + .to_owned();
1-6: Fix module docs: resolving is via GraphQL; REST is used only for replies.Align the docs with the implementation.
-//! Resolve pull request review comments via the GitHub REST API. +//! Resolve pull request review comments via the GitHub API +//! (GraphQL for resolving threads; REST for optional replies).
86-96: Skip sending empty messages.Avoid posting a reply when the message is blank after trimming.
- if let Some(body) = message { + if let Some(body) = message.filter(|s| !s.trim().is_empty()) {
25-45: Harden the HTTP client: add API version header and a timeout.Pin the REST API version and avoid hanging requests.
use reqwest::header::{ACCEPT, AUTHORIZATION, HeaderMap, USER_AGENT}; +use reqwest::header::HeaderName; +use std::time::Duration; @@ headers.insert( ACCEPT, "application/vnd.github+json" .parse() .expect("accept header"), ); + headers.insert( + HeaderName::from_static("x-github-api-version"), + "2022-11-28".parse().expect("version header"), + ); - reqwest::Client::builder() + reqwest::Client::builder() .default_headers(headers) + .timeout(Duration::from_secs(20)) .build()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.toml(2 hunks)docs/vk-design.md(2 hunks)src/resolve.rs(1 hunks)tests/resolve.rs(1 hunks)tests/utils.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/vk-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/vk-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/vk-design.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Files:
src/resolve.rstests/resolve.rstests/utils.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/resolve.rstests/resolve.rstests/utils.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Mandate caret requirements for all dependencies: All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3").
Prohibit unstable version specifiers: The use of wildcard (*), or open-ended inequality (>=) version requirements are strictly forbidden inCargo.toml. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🧬 Code graph analysis (2)
src/resolve.rs (2)
src/review_threads/tests.rs (1)
repo(16-21)src/test_utils.rs (2)
set_var(103-108)remove_var(113-118)
tests/resolve.rs (1)
tests/utils.rs (3)
start_mitm(49-98)vk_cmd(110-118)shutdown(30-33)
🪛 GitHub Check: build-test
tests/resolve.rs
[warning] 41-41:
Diff in /home/runner/work/vk/vk/tests/resolve.rs
[warning] 17-17:
Diff in /home/runner/work/vk/vk/tests/resolve.rs
🪛 GitHub Actions: CI
tests/resolve.rs
[warning] 17-17: Cargo fmt check detected formatting differences at line 17. Run 'cargo fmt' to apply changes.
[warning] 41-41: Cargo fmt check detected formatting differences at line 41. Run 'cargo fmt' to apply changes.
🔍 Remote MCP Ref
Relevant additional facts for review (concise)
- GitHub resolution is only supported via GraphQL mutation resolveReviewThread; there is no documented REST endpoint to mark a PR review thread resolved, so using GraphQL is correct and more stable than relying on undocumented REST routes.
- The PR adds optional REST reply behavior: when a message is supplied the code posts to /repos/{owner}/{repo}/pulls/comments/{comment_id}/replies before invoking the GraphQL mutation — this matches typical workaround patterns (post reply + GraphQL resolve). (from PR diff)
- Test tooling: httpmock = "0.7" was added to dev-dependencies; version 0.7.x provides async and sync servers and is appropriate for the MITM-style tests added.
- Config layering: ResolveArgs derives OrthoConfig with prefix VK, so environment variables VK_REFERENCE / VK_MESSAGE (and the existing GITHUB_API_URL / GITHUB_GRAPHQL_URL overrides used by tests) are expected and used by tests to redirect REST and GraphQL endpoints to the MITM server. (from PR diffs + OrthoConfig behavior)
- Tests added cover both cases (with and without message): they assert the exact request sequence (POST replies then POST /graphql, or only POST /graphql), using the MITM server and environment overrides — this provides good end-to-end coverage for the new behavior. (from tests/resolve.rs and tests/utils.rs)
Tool note: an attempted documentation search returned an HTTP 402 during research; nevertheless the above points are supported by the PR diffs, tests, and known GitHub API behavior.
🔇 Additional comments (6)
Cargo.toml (1)
27-27: Add base64 dependency — LGTM.Version specifier complies with caret semantics and matches usage in src/resolve.rs.
docs/vk-design.md (1)
201-203: Document REST base override — LGTM.The
GITHUB_API_URLoverride is clear and aligns with tests.tests/utils.rs (2)
102-104: MITM wiring for GraphQL and REST — LGTM.The doc and env wiring make assertions deterministic.
112-114: Env overrides — LGTM.Point both GraphQL and REST to the MITM server without trailing slashes.
tests/resolve.rs (2)
11-16: Exercise both paths — LGTM.Parameterise with/without message; good coverage of request sequencing.
12-13: Ignore incorrect expectation of two GraphQL calls.
resolve_comment encodes the thread ID locally and issues a single GraphQL mutation; no separate lookup request occurs. Leave the test’s expected sequence unchanged.Likely an incorrect or invalid review comment.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/vk-design.md (1)
204-207: Document the GraphQL endpoint override alongside REST.Expose
GITHUB_GRAPHQL_URLin the configuration section.- in the required parsers as transitive dependencies. The REST API base URL - defaults to `https://api.github.com` but can be overridden with - `GITHUB_API_URL` for testing. + in the required parsers as transitive dependencies. The REST API base URL + defaults to `https://api.github.com` and can be overridden with + `GITHUB_API_URL` for testing. The GraphQL endpoint defaults to + `https://api.github.com/graphql` and can be overridden with + `GITHUB_GRAPHQL_URL`.README.md (1)
61-63: Clarify token requirements for resolve.State that
resolverequires a token and aborts if missing.-`vk` uses the GitHub GraphQL API. Set `GITHUB_TOKEN` to authenticate. If it's -not set, you'll get a warning and anonymous requests may be rate limited. +`vk` uses the GitHub GraphQL API. Set `GITHUB_TOKEN` to authenticate. The +`resolve` subcommand requires a token and aborts if it is not set. For +read‑only operations, anonymous requests may be rate‑limited.src/cli_args.rs (1)
5-6: Replace file-wide allows with expects to comply with lint policy.Do not use
#[allow]. Use narrowly scoped#[expect(..., reason = "...")]instead.-#![allow(non_snake_case, reason = "clap generates non-snake-case modules")] -#![allow(unused_imports, reason = "clap derives import the struct internally")] +#![expect(non_snake_case, reason = "clap generates non-snake-case modules")] +#![expect(unused_imports, reason = "clap derives import the struct internally")]
♻️ Duplicate comments (10)
docs/vk-design.md (1)
39-45: Correct the thread resolution description and remove incorrect base64 guidance.Resolving requires the GraphQL thread ID. Fetch it from the comment node and use
resolveReviewThread. Do not instruct users to base64-encode"PullRequestReviewThread:<id>".-- **Resolve threads**: `vk resolve <comment-ref>` resolves the thread via the - `resolveReviewThread` GraphQL mutation. When compiled with the - `unstable-rest-resolve` feature, it posts a reply via the REST API before - resolving. The thread ID is derived by base64-encoding - `PullRequestReviewThread:<id>`. This subcommand requires `GITHUB_TOKEN`; if - absent, it aborts rather than performing anonymous calls. +- **Resolve threads**: `vk resolve <comment-ref> [-m <text>]` posts an optional + reply (when built with `unstable-rest-resolve`), then resolves the thread via + GitHub’s GraphQL `resolveReviewThread` mutation. The tool obtains the thread + identifier by querying the review comment’s node and reading + `pullRequestReviewThread.id`; no manual base64 construction is required. This + subcommand requires `GITHUB_TOKEN` and aborts if it is not set.src/cli_args.rs (1)
96-107: Drop the manual Default for ResolveArgs to avoid empty-string sentinels.Avoid exporting a misleading default for a type with a required field.
-#[expect( - clippy::derivable_impls, - reason = "manual impl clarifies default empty reference" -)] -impl Default for ResolveArgs { - fn default() -> Self { - Self { - reference: String::new(), - message: None, - } - } -} +// No Default: `reference` is required at the CLI boundary.src/resolve.rs (2)
104-121: Update the test to satisfy the lookup + resolve flow and assert both calls.Mock two GraphQL invocations and keep REST reply as-is.
- let resolve = server.mock(|when, then| { - when.method(POST).path("/graphql"); - then.status(200) - .json_body(json!({"data": {"resolveReviewThread": {"clientMutationId": null}}})); - }); + let resolve = server + .mock(|when, then| { + when.method(POST).path("/graphql"); + then.status(200).json_body(json!({ + "data": { + "node": {"pullRequestReviewThread": {"id": "PRRT_on3"}}, + "resolveReviewThread": {"clientMutationId": null} + } + })); + }) + .expect(2);
91-97: Resolve the correct thread: fetch the thread ID via GraphQL instead of base64-ing the comment ID.Base64‑encoding
"PullRequestReviewThread:{comment_id}"is incorrect and can resolve the wrong entity. Look up the comment node, readpullRequestReviewThread.id, then callresolveReviewThread.+const GET_THREAD_ID_QUERY: &str = r" + query($id: ID!) { + node(id: $id) { + ... on PullRequestReviewComment { + pullRequestReviewThread { id } + } + } + } +"; @@ - let gql = GraphQLClient::new(token, None)?; - let thread_id = STANDARD.encode(format!("PullRequestReviewThread:{comment_id}")); - let vars = json!({ "id": thread_id }); - gql.run_query::<_, Value>(RESOLVE_THREAD_MUTATION, vars) - .await?; + let gql = GraphQLClient::new(token, None)?; + // 1) Look up the thread ID from the comment node. + let comment_node_id = STANDARD.encode(format!("PullRequestReviewComment:{comment_id}")); + let lookup_vars = json!({ "id": comment_node_id }); + let lookup = gql + .run_query::<_, Value>(GET_THREAD_ID_QUERY, lookup_vars) + .await?; + let thread_id = lookup["data"]["node"]["pullRequestReviewThread"]["id"] + .as_str() + .ok_or_else(|| VkError::BadResponse("missing thread id".into()))?; + // 2) Resolve the thread. + let vars = json!({ "id": thread_id }); + gql.run_query::<_, Value>(RESOLVE_THREAD_MUTATION, vars).await?;Cargo.toml (1)
50-50: Drop the unused dev-dependency or use it in tests.
httpmockis not used; keep the surface tight.Run this to verify no usages:
#!/bin/bash set -euo pipefail rg -n -C2 '\bhttpmock\b|use\s+httpmock' || echo "No usages found"Apply:
- httpmock = "0.7"tests/resolve.rs (2)
47-90: Gate the REST reply flow and assert ordering — LGTM.Feature-guard the reply+resolve path and verify call sequence deterministically.
23-27: Future-proof the mock GraphQL response.Return both the lookup and resolve shapes so refactors that add a lookup won’t break the test.
- let body = if req.uri().path() == "/graphql" { - r#"{"data":{"resolveReviewThread":{"clientMutationId":null}}}"# - } else { - "{}" - }; + let body = if req.uri().path() == "/graphql" { + r#"{"data":{ + "node":{"pullRequestReviewThread":{"id":"PRRT_dummy"}}, + "resolveReviewThread":{"clientMutationId":null} + }}"# + } else { + "{}" + };src/main.rs (3)
83-86: Document the fragment requirement forresolve— LGTM.Help text clearly states
#discussion_r<ID>is mandatory.
125-126: IntroduceMissingAutherror — LGTM.Use a semantic error for missing
GITHUB_TOKEN.
372-381: Fail fast on missing token and enforce fragment — LGTM.Validate input, reject anonymous mutation, then delegate to resolver.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
Cargo.toml(2 hunks)README.md(2 hunks)docs/vk-design.md(2 hunks)src/cli_args.rs(1 hunks)src/main.rs(9 hunks)src/resolve.rs(1 hunks)tests/resolve.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Mandate caret requirements for all dependencies: All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3").
Prohibit unstable version specifiers: The use of wildcard (*), or open-ended inequality (>=) version requirements are strictly forbidden inCargo.toml. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/vk-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/vk-design.mdREADME.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/vk-design.mdREADME.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Files:
src/cli_args.rssrc/resolve.rssrc/main.rstests/resolve.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/cli_args.rssrc/resolve.rssrc/main.rstests/resolve.rs
🧬 Code graph analysis (3)
src/resolve.rs (2)
src/review_threads/tests.rs (1)
repo(16-21)src/test_utils.rs (2)
set_var(103-108)remove_var(113-118)
src/main.rs (2)
src/ref_parser.rs (1)
parse_pr_thread_reference(148-163)src/resolve.rs (1)
resolve_comment(58-97)
tests/resolve.rs (1)
tests/utils.rs (3)
start_mitm(49-98)vk_cmd(110-118)shutdown(30-33)
🪛 GitHub Check: build-test
src/resolve.rs
[failure] 69-69:
unused variable: client
[failure] 65-65:
unused variable: api
[failure] 63-63:
unused variable: message
[failure] 61-61:
unused variable: pull_number
[failure] 60-60:
unused variable: repo
[failure] 10-10:
unused import: reqwest::StatusCode
🪛 GitHub Actions: CI
src/resolve.rs
[error] 10-10: cargo llvm-cov failed due to compilation error: unused import: reqwest::StatusCode. Failing step: 'cargo llvm-cov --workspace --summary-only --lcov --output-path lcov.info'.
🔍 Remote MCP Deepwiki, Ref
Relevant facts for review (concise)
- GraphQL is correctly used to mark threads resolved via resolveReviewThread; no REST endpoint exists for resolving threads — the code posts an optional REST reply then runs the GraphQL mutation (matches PR tests and docs).
- Thread ID is formed by base64-encoding "PullRequestReviewThread:{comment_id}" (code and docs).
- New CLI: ResolveArgs added (Derives: Parser, OrthoConfig prefix VK, Default impl) and Commands::Resolve dispatch wired in main; missing GITHUB_TOKEN returns VkError::MissingAuth. Tests cover CLI parsing.
- resolve_comment builds a reqwest client with headers: User-Agent "vk", Authorization Bearer , Accept application/vnd.github+json, x-github-api-version 2022-11-28, 10s timeout; uses GITHUB_API_URL and GITHUB_GRAPHQL_URL overrides for tests.
- Feature gate unstable-rest-resolve controls whether the REST reply is sent; tests for the REST path are gated behind this feature.
- Dev/test changes: added base64 and httpmock = "0.7" to Cargo.toml and tests/utils updated to route both REST and GraphQL through the MITM proxy; end-to-end tests assert request ordering (reply then GraphQL when message supplied, otherwise only GraphQL).
Sources used: PR diff files and test code referenced in the provided context.
🔇 Additional comments (7)
src/cli_args.rs (1)
79-94: LGTM: ResolveArgs surface and help text are clear.Required
referenceasStringand -m help text read well.src/resolve.rs (2)
1-6: LGTM: Module doc accurately distinguishes GraphQL (resolve) vs REST (reply).Keep this phrasing; it avoids API confusion.
21-51: LGTM: HTTP client hardening is adequate.Timeout and API version pin are in place; headers are correct.
Cargo.toml (2)
27-27: Add base64 dependency — LGTM.Dependency aligns with thread ID encoding in resolve logic.
34-34: Gate unstable REST reply path — LGTM.Feature flag cleanly isolates the non-public REST behaviour for tests.
tests/resolve.rs (1)
11-14: Cover the no-message path — LGTM.This asserts the GraphQL-only flow as intended.
src/main.rs (1)
399-401: Wire upResolvedispatch — LGTM.Subcommand integration follows existing pattern.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
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.
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.
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.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/resolve.rs:120` </location>
<code_context>
+ }
+
+ let gql = GraphQLClient::new(token, None)?;
+ let comment_node = STANDARD.encode(format!("PullRequestReviewComment:{comment_id}"));
+ let lookup = gql
+ .run_query::<_, Value>(THREAD_ID_QUERY, json!({ "id": comment_node }))
</code_context>
<issue_to_address>
Base64 encoding of comment node may be fragile if GitHub changes ID format.
This approach relies on GitHub's current ID format, which may change. Please document this dependency or add a fallback to handle future changes.
</issue_to_address>
### Comment 2
<location> `tests/resolve.rs:53` </location>
<code_context>
+ if token.is_empty() {
+ return Err(VkError::MissingAuth);
+ }
+ #[cfg(feature = "unstable-rest-resolve")]
+ {
+ resolve::resolve_comment(
</code_context>
<issue_to_address>
Consider adding negative tests for REST API failures in resolve_flows_reply.
Please add tests for error responses from the REST API (such as 404 and 500) to verify correct CLI error handling.
</issue_to_address>
### Comment 3
<location> `src/resolve.rs:83` </location>
<code_context>
+/// # Errors
+///
+/// Returns [`VkError::RequestContext`] if an HTTP request fails.
+pub async fn resolve_comment(
+ token: &str,
+ reference: CommentRef<'_>,
</code_context>
<issue_to_address>
Consider refactoring resolve_comment by extracting helper functions for each major step to improve clarity and maintainability.
Here’s one way to collapse the `resolve_comment` logic into a handful of small helpers that each do one thing (REST‐reply, env‐URL, gql lookup, gql mutation). Nothing is removed or reverted—each helper just pulls a chunk of logic out of the big function:
```rust
// in resolve.rs
#[cfg(feature = "unstable-rest-resolve")]
async fn post_reply(
token: &str,
repo: &RepoInfo,
pull: u64,
comment_id: u64,
body: &str,
) -> Result<(), VkError> {
let api = std::env::var("GITHUB_API_URL")
.unwrap_or_else(|_| "https://api.github.com".into())
.trim_end_matches('/')
.to_owned();
let client = github_client(token)?;
let url = format!(
"{api}/repos/{owner}/{repo}/pulls/{pull}/comments/{cid}/replies",
owner = repo.owner,
repo = repo.name,
pull = pull,
cid = comment_id,
);
let resp = client
.post(url)
.json(&json!({ "body": body }))
.send()
.await
.map_err(|e| VkError::RequestContext {
context: "post reply".into(),
source: Box::new(e),
})?;
if resp.status() != StatusCode::NOT_FOUND {
resp.error_for_status()
.map_err(|e| VkError::Request(Box::new(e)))?;
}
Ok(())
}
async fn get_thread_id(
gql: &GraphQLClient,
comment_id: u64,
) -> Result<String, VkError> {
let node = STANDARD.encode(format!("PullRequestReviewComment:{comment_id}"));
let data: Value = gql
.run_query(THREAD_ID_QUERY, json!({ "id": node }))
.await?;
data.get("node")
.and_then(|n| n.get("pullRequestReviewThread"))
.and_then(|t| t.get("id"))
.and_then(Value::as_str)
.map(|s| s.to_owned())
.ok_or_else(|| VkError::BadResponse("missing thread id".into()))
}
async fn resolve_thread(
gql: &GraphQLClient,
thread_id: &str,
) -> Result<(), VkError> {
gql.run_query::<_, Value>(
RESOLVE_THREAD_MUTATION,
json!({ "id": thread_id }),
)
.await?;
Ok(())
}
pub async fn resolve_comment(
token: &str,
reference: CommentRef<'_>,
#[cfg(feature = "unstable-rest-resolve")] message: Option<String>,
) -> Result<(), VkError> {
#[cfg(feature = "unstable-rest-resolve")]
if let Some(body) = message {
post_reply(token, reference.repo, reference.pull_number, reference.comment_id, &body).await?;
}
let gql = GraphQLClient::new(token, None)?;
let thread_id = get_thread_id(&gql, reference.comment_id).await?;
resolve_thread(&gql, &thread_id).await?;
Ok(())
}
```
Steps taken:
1. Extract `post_reply`, `get_thread_id` and `resolve_thread` so each helper has one clear responsibility.
2. `resolve_comment` now simply sequences: optional reply → lookup thread → resolve thread.
3. Any shared bits (env lookup, error mapping) stay inside the small helpers.
This flattens the control flow, removes nested `#[cfg]` blocks from the core function, and groups all header/env logic into dedicated places.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Summary
resolvesub-command to mark review comments as resolvedTesting
make fmtmake lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_68bda8a64e248322b62d2a717e179914
Summary by Sourcery
Introduce a new
resolvesubcommand to mark pull request review comment threads as resolved, optionally posting a reply when built with theunstable-rest-resolvefeature.New Features:
resolveCLI subcommand accepting a comment reference and optional message to mark a review thread as resolvedEnhancements:
unstable-rest-resolvefeature is enabledBuild:
unstable-rest-resolvefeature and addbase64dependency for thread ID encodingDocumentation:
resolvesubcommand usage and behavior indocs/vk-design.mdandREADME.mdTests:
resolvesubcommand