Extend vk pr: implicit PR retrieval with branch/fragment handling#185
Merged
Extend vk pr: implicit PR retrieval with branch/fragment handling#185
Conversation
Allow `vk pr` to be invoked without a URL or PR number. When no reference is provided, the command detects the PR associated with the current Git branch using the GitHub API. This also supports passing only a comment fragment (`#discussion_r<ID>`) without the full URL or PR number, in which case the PR is auto-detected and the specified comment thread is displayed. Changes: - Add `NoPrForBranch` error variant to `VkError` for clearer error messages - Add `PR_FOR_BRANCH_QUERY` GraphQL query to look up PRs by head ref - Add `current_branch()` function to read branch from `.git/HEAD` - Add `is_fragment_only()` and `parse_fragment_only()` for fragment parsing - Make `parse_repo_str()` and `repo_from_fetch_head()` public - Create `branch_pr` module with `fetch_pr_for_branch()` function - Update `PrArgs.reference` to be optional (not required) - Update `setup_pr_output()` to async with `resolve_pr_reference()` logic - Add comprehensive unit tests for new parsing functions - Add e2e tests for auto-detection scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Reviewer's GuideImplements implicit PR resolution for the Sequence diagram for implicit vk pr resolution from current branchsequenceDiagram
actor User
participant Cli as vk_pr_CLI
participant Commands as run_pr
participant Setup as setup_pr_output
participant Resolver as resolve_pr_reference
participant RefParser as ref_parser
participant BranchPr as branch_pr
participant Client as GraphQLClient
participant GitHub as GitHub_API
participant Git as Git_repo
User->>Cli: invoke vk pr [optional reference]
Cli->>Commands: parse_args -> run_pr(args, global)
Commands->>Setup: setup_pr_output(args, global, token)
Setup->>Client: build_graphql_client(token)
Setup->>Resolver: resolve_pr_reference(args.reference, global.repo, client)
alt no_reference_provided
Resolver->>RefParser: current_branch()
RefParser->>Git: read .git/HEAD
Git-->>RefParser: HEAD_content
RefParser-->>Resolver: Some(branch) or None
alt detached_HEAD
Resolver-->>Setup: Err(VkError_InvalidRef)
Setup-->>Commands: Err(VkError_InvalidRef)
Commands-->>Cli: print_error and exit
else symbolic_HEAD
Resolver->>RefParser: parse_repo_str(global.repo)
alt repo_from_global
RefParser-->>Resolver: Some(RepoInfo)
else repo_from_fetch_head
Resolver->>RefParser: repo_from_fetch_head()
RefParser->>Git: read .git/FETCH_HEAD
Git-->>RefParser: FETCH_HEAD_content
RefParser-->>Resolver: Some(RepoInfo) or None
alt repo_not_found
Resolver-->>Setup: Err(VkError_RepoNotFound)
Setup-->>Commands: Err(VkError_RepoNotFound)
Commands-->>Cli: print_error and exit
end
end
Resolver->>BranchPr: fetch_pr_for_branch(client, RepoInfo, branch)
BranchPr->>Client: run_query(PR_FOR_BRANCH_QUERY, vars)
Client->>GitHub: POST GraphQL PR_FOR_BRANCH_QUERY
GitHub-->>Client: PR list (nodes)
Client-->>BranchPr: PrForBranchData
alt pr_found
BranchPr-->>Resolver: pr_number
Resolver-->>Setup: (RepoInfo, pr_number, None)
else no_pr_for_branch
BranchPr-->>Resolver: Err(VkError_NoPrForBranch)
Resolver-->>Setup: Err(VkError_NoPrForBranch)
Setup-->>Commands: Err(VkError_NoPrForBranch)
Commands-->>Cli: print_error and exit
end
end
else fragment_only_reference
Resolver->>RefParser: is_fragment_only(reference)
RefParser-->>Resolver: true
Resolver->>RefParser: parse_fragment_only(reference)
RefParser-->>Resolver: Ok(comment_id) or Err(VkError_InvalidRef)
alt invalid_fragment
Resolver-->>Setup: Err(VkError_InvalidRef)
Setup-->>Commands: Err(VkError_InvalidRef)
Commands-->>Cli: print_error and exit
else valid_fragment
Resolver->>RefParser: current_branch()
RefParser->>Git: read .git/HEAD
Git-->>RefParser: HEAD_content
RefParser-->>Resolver: Some(branch) or None
alt detached_HEAD
Resolver-->>Setup: Err(VkError_InvalidRef)
Setup-->>Commands: Err(VkError_InvalidRef)
Commands-->>Cli: print_error and exit
else symbolic_HEAD
Resolver->>RefParser: parse_repo_str(global.repo) or repo_from_fetch_head()
RefParser-->>Resolver: Some(RepoInfo) or None
alt repo_resolved
Resolver->>BranchPr: fetch_pr_for_branch(client, RepoInfo, branch)
BranchPr->>Client: run_query(PR_FOR_BRANCH_QUERY, vars)
Client->>GitHub: POST GraphQL PR_FOR_BRANCH_QUERY
GitHub-->>Client: PR list
Client-->>BranchPr: PrForBranchData
BranchPr-->>Resolver: pr_number or Err(VkError_NoPrForBranch)
alt pr_found
Resolver-->>Setup: (RepoInfo, pr_number, Some(comment_id))
else no_pr_for_branch
Resolver-->>Setup: Err(VkError_NoPrForBranch)
Setup-->>Commands: Err(VkError_NoPrForBranch)
Commands-->>Cli: print_error and exit
end
else repo_not_found
Resolver-->>Setup: Err(VkError_RepoNotFound)
Setup-->>Commands: Err(VkError_RepoNotFound)
Commands-->>Cli: print_error and exit
end
end
end
else full_reference
Resolver->>RefParser: parse_pr_thread_reference(reference, global.repo)
RefParser-->>Resolver: (RepoInfo, pr_number, comment_id) or Err(VkError_InvalidRef)
Resolver-->>Setup: result
end
Setup-->>Commands: Some(PrContext)
Commands->>Cli: render_PR_output_and_threads
Class diagram for new branch_pr and updated ref_parser and VkErrorclassDiagram
class RefParser {
+current_branch() Option_String
+parse_repo_str(repo String) Option_RepoInfo
+repo_from_fetch_head() Option_RepoInfo
+is_fragment_only(input String) bool
+parse_fragment_only(input String) Result_u64_VkError
+parse_pr_thread_reference(input String, default_repo Option_String) Result_PrRef_VkError
}
class RepoInfo {
+owner String
+name String
}
class BranchPr {
+fetch_pr_for_branch(client GraphQLClient, repo RepoInfo, branch String) Result_u64_VkError
}
class PrForBranchData {
+repository PrForBranchRepository
}
class PrForBranchRepository {
+pull_requests PrConnection
}
class PrConnection {
+nodes Vec_PrNode
}
class PrNode {
+number u64
}
class GraphQLQueries {
+PR_FOR_BRANCH_QUERY &str
}
class Commands {
+resolve_pr_reference(reference Option_str, default_repo Option_str, client GraphQLClient) Result_PrThreadRef_VkError
+setup_pr_output(args PrArgs, global GlobalArgs, cli_token Option_str) Result_Option_PrContext_VkError
}
class PrArgs {
+reference Option_String
}
class VkError {
+InvalidRef
+RepoNotFound
+NoPrForBranch branch Box_str
+CommentNotFound comment_id u64
+BadResponse message Box_str
}
class GraphQLClient {
+run_query(query &str, vars Map_String_Value) Result_T_VkError
}
class PrContext {
+repo RepoInfo
+number u64
+comment_id Option_u64
+client GraphQLClient
}
RefParser --> RepoInfo
BranchPr --> RepoInfo
BranchPr --> GraphQLClient
BranchPr --> PrForBranchData
PrForBranchData --> PrForBranchRepository
PrForBranchRepository --> PrConnection
PrConnection --> PrNode
GraphQLQueries <.. BranchPr
Commands --> RefParser
Commands --> BranchPr
Commands --> GraphQLClient
Commands --> PrArgs
Commands --> PrContext
VkError <.. RefParser
VkError <.. BranchPr
VkError <.. Commands
GraphQLClient ..> VkError
BranchPr ..> VkError
RefParser ..> VkError
PrArgs --> Commands
RepoInfo --> PrContext
GraphQLClient --> PrContext
Flow diagram for PR resolution from branch or referenceflowchart TD
A["Start vk pr"] --> B{Reference provided?}
B -->|No| C["Read current_branch from .git/HEAD"]
C --> D{Branch found?}
D -->|No| E["Error VkError::InvalidRef (detached HEAD)"]
D -->|Yes| F["Resolve RepoInfo from global.repo via parse_repo_str"]
F --> G{RepoInfo resolved?}
G -->|No| H["Resolve RepoInfo from .git/FETCH_HEAD via repo_from_fetch_head"]
H --> I{RepoInfo resolved?}
I -->|No| J["Error VkError::RepoNotFound"]
I -->|Yes| K["fetch_pr_for_branch via GraphQL PR_FOR_BRANCH_QUERY"]
G -->|Yes| K
K --> L{PR found for branch?}
L -->|No| M["Error VkError::NoPrForBranch"]
L -->|Yes| N["Use (RepoInfo, pr_number, None) as PR context"]
B -->|Yes| O{"is_fragment_only(reference)?"}
O -->|Yes| P["parse_fragment_only -> comment_id"]
P --> Q{Valid comment_id?}
Q -->|No| R["Error VkError::InvalidRef"]
Q -->|Yes| C
C --> S["After branch and repo resolution"]
S --> T["fetch_pr_for_branch and use (RepoInfo, pr_number, Some(comment_id))"]
O -->|No| U["parse_pr_thread_reference(reference, global.repo)"]
U --> V{Parsed successfully?}
V -->|No| W["Error VkError::InvalidRef"]
V -->|Yes| X["Use parsed (RepoInfo, pr_number, optional_comment_id)"]
N --> Y["Build PrContext and render PR"]
T --> Y
X --> Y
Y --> Z["End"]
E --> Z
J --> Z
M --> Z
R --> Z
W --> Z
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. WalkthroughEnable PR auto‑detection when the CLI reference is omitted by making the reference optional; relocate and expand reference parsing into a ref_parser module with git helpers; add branch→PR GraphQL lookup with fork disambiguation; convert PR setup flow to async; add DetachedHead and NoPrForBranch error variants; and extend unit and e2e tests and test helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Commands
participant RefParser
participant Git
participant BranchPR as BranchPR (GraphQL)
participant GitHub as GitHub API
rect rgba(0, 100, 200, 0.5)
note over CLI,GitHub: PR Auto-Detection Flow (no reference)
end
CLI->>Commands: setup_pr_output(args, global, token) [no reference]
Commands->>Git: current_branch()
Git-->>Commands: branch name or error (detached)
Commands->>RefParser: resolve_branch_and_repo(branch)
RefParser->>Git: repo_from_fetch_head() / repo_from_origin()
Git-->>RefParser: RepoInfo, maybe head owner
RefParser-->>Commands: BranchContext(repo, branch, head_owner?)
Commands->>BranchPR: fetch_pr_for_branch(repo, branch, head_owner)
BranchPR->>GitHub: PR_FOR_BRANCH_QUERY (headRefName, owner, name)
GitHub-->>BranchPR: PR list with head repository owner info
BranchPR->>BranchPR: filter by head_owner if provided
BranchPR-->>Commands: PR number
Commands-->>CLI: PrContext with PR number
rect rgba(100, 200, 0, 0.5)
note over CLI,Commands: Fragment-only input handling
end
CLI->>Commands: setup_pr_output(args, global, token) [fragment "#discussion_r123"]
Commands->>RefParser: is_fragment_only("#discussion_r123")
RefParser-->>Commands: true
Commands->>Commands: parse fragment ID and short-circuit branch lookup
Commands-->>CLI: PrContext with fragment-only selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +374 to +384 fn current_branch_parses_symbolic_ref() {
let dir = tempdir().expect("tempdir");
let git_dir = dir.path().join(".git");
fs::create_dir(&git_dir).expect("create git dir");
fs::write(git_dir.join("HEAD"), "ref: refs/heads/feature-branch\n").expect("write HEAD");
let cwd = std::env::current_dir().expect("cwd");
std::env::set_current_dir(dir.path()).expect("chdir temp");
let branch = current_branch().expect("branch from HEAD");
std::env::set_current_dir(cwd).expect("restore cwd");
assert_eq!(branch, "feature-branch");
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Extract common test setup for .git/HEAD file handling into a `with_git_head` helper function. Add `#[serial]` attribute to tests that change the current working directory to prevent race conditions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 10 issues, and left some high level feedback:
- Both
current_branch()andrepo_from_fetch_head()assume.gitis in the current working directory, which will fail whenvk pris run from a subdirectory of the repo; consider resolving paths relative to the repository root (e.g. viagit rev-parse --show-toplevelor similar) instead of assuming.. - The
resolve_pr_referencelogic repeats the samedefault_repo.and_then(parse_repo_str).or_else(repo_from_fetch_head).ok_or(VkError::RepoNotFound)?chain in multiple match arms; factoring this into a small helper (e.g.resolve_repo(default_repo)) would reduce duplication and make future changes less error-prone. - When in a detached HEAD state,
vk prnow returnsVkError::InvalidRefwith a generic "invalid reference" message; consider introducing a more specific error variant or message to indicate the detached HEAD condition explicitly, since this is a common scenario users may need guidance on.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `current_branch()` and `repo_from_fetch_head()` assume `.git` is in the current working directory, which will fail when `vk pr` is run from a subdirectory of the repo; consider resolving paths relative to the repository root (e.g. via `git rev-parse --show-toplevel` or similar) instead of assuming `.`.
- The `resolve_pr_reference` logic repeats the same `default_repo.and_then(parse_repo_str).or_else(repo_from_fetch_head).ok_or(VkError::RepoNotFound)?` chain in multiple match arms; factoring this into a small helper (e.g. `resolve_repo(default_repo)`) would reduce duplication and make future changes less error-prone.
- When in a detached HEAD state, `vk pr` now returns `VkError::InvalidRef` with a generic "invalid reference" message; consider introducing a more specific error variant or message to indicate the detached HEAD condition explicitly, since this is a common scenario users may need guidance on.
## Individual Comments
### Comment 1
<location> `src/ref_parser.rs:215-216` </location>
<code_context>
+/// assert!(!is_fragment_only("42#discussion_r123"));
+/// assert!(!is_fragment_only("https://github.com/o/r/pull/1#discussion_r123"));
+/// ```
+pub fn is_fragment_only(input: &str) -> bool {
+ input.starts_with("#discussion_r")
+}
+
</code_context>
<issue_to_address>
**suggestion:** Share the fragment prefix constant between `is_fragment_only` and `parse_fragment_only` to keep them in sync.
`is_fragment_only` hardcodes `"#discussion_r"` while `parse_fragment_only` uses a `FRAG` constant. Please reuse a single shared constant (e.g., a module-level `FRAG`) so changes to the fragment format are made in one place.
Suggested implementation:
```rust
pub fn is_fragment_only(input: &str) -> bool {
input.starts_with(FRAG)
}
```
To fully apply the suggestion, make sure:
1. The `FRAG` constant used by `parse_fragment_only` is defined at module scope (e.g. near the top of `src/ref_parser.rs`), like:
```rust
const FRAG: &str = "#discussion_r";
```
2. If `FRAG` is currently defined inside `parse_fragment_only`, move it out to module scope so both `parse_fragment_only` and `is_fragment_only` can reference the same constant.
</issue_to_address>
### Comment 2
<location> `tests/e2e.rs:237` </location>
<code_context>
+ })
+ .to_string();
+ let reviews_body = include_str!("fixtures/reviews_empty.json").to_string();
+ set_sequential_responder(&handler, vec![pr_lookup_body, threads_body, reviews_body]);
+
+ let dir = tempdir().expect("tempdir");
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting the GraphQL request parameters (especially `headRef`) in the MITM handler to prove branch-based PR lookup wiring end-to-end.
Right now these e2e tests only verify behavior against canned GraphQL responses, not that the client sends the correct request. If `fetch_pr_for_branch` or `resolve_pr_reference` used the wrong branch or a hard-coded value, the tests could still pass. If the MITM helper supports it, consider extending `set_sequential_responder` (or adding a variant) to inspect the incoming GraphQL request body and assert that:
- `headRef` matches the branch name from `.git/HEAD` (e.g., `my-feature-branch`), and
- The repository owner/name match what was parsed from `FETCH_HEAD` or `--repo`.
That would turn these into true end-to-end checks of the branch-based lookup wiring to the API.
Suggested implementation:
```rust
let threads_body = serde_json::json!({
"data": {"repository": {"pullRequest": {"reviewThreads": {
"nodes": [],
"pageInfo": {"hasNextPage": false, "endCursor": null}
}}}}
})
.to_string();
let reviews_body = include_str!("fixtures/reviews_empty.json").to_string();
// Assert GraphQL request variables to make this a true end-to-end wiring test.
//
// We expect:
// - headRef to match the branch name from .git/HEAD
// - owner/name to match what was parsed from FETCH_HEAD / --repo
set_sequential_responder_with_assert(
&handler,
vec![pr_lookup_body, threads_body, reviews_body],
|body: &serde_json::Value| {
// Shape is expected to be: { "query": "...", "variables": { ... } }
let vars = &body["variables"];
// Branch-based lookup wiring
assert_eq!(
vars["headRef"],
"my-feature-branch",
"GraphQL headRef variable should match the branch from .git/HEAD"
);
// Repository owner/name wiring
assert_eq!(
vars["owner"],
"owner",
"GraphQL owner variable should match the repo owner parsed from FETCH_HEAD / --repo"
);
assert_eq!(
vars["name"],
"repo",
"GraphQL name variable should match the repo name parsed from FETCH_HEAD / --repo"
);
},
);
```
To fully implement this change, you will need to:
1. **Add a new helper** in your MITM / test HTTP helper module (where `set_sequential_responder` is defined), for example:
```rust
pub fn set_sequential_responder_with_assert<F>(
handler: &HandlerType, // use the actual handler type
responses: Vec<String>,
assert_body: F,
)
where
F: Fn(&serde_json::Value) + Send + Sync + 'static,
{
// Pseudocode – adapt to your existing MITM implementation:
//
// 1. For each expected response, register a handler on `handler`
// that:
// - reads the incoming HTTP request body,
// - parses it as JSON (`serde_json::from_slice`),
// - calls `assert_body(&parsed_json)`,
// - returns the corresponding canned response.
//
// 2. Preserve any existing behavior from `set_sequential_responder`
// (status codes, headers, sequencing, etc.).
}
```
2. **Ensure JSON parsing of GraphQL requests**:
- The function must read the incoming request body and parse it as `serde_json::Value`.
- Call the provided `assert_body` closure before returning the canned response.
3. **Update call sites as needed**:
- Keep the existing `set_sequential_responder` for tests that don’t care about inspecting the request.
- Use `set_sequential_responder_with_assert` where you want end-to-end wiring assertions like in this test.
4. If your actual GraphQL variables use different keys (e.g. `headRefName`, `repositoryOwner`, `repositoryName`), adjust the `vars["..."]` lookups in the test and the assertions to match the real schema.
</issue_to_address>
### Comment 3
<location> `src/commands.rs:174` </location>
<code_context>
+/// 1. No reference: detect PR from current branch
+/// 2. Fragment only (`#discussion_r<ID>`): detect PR from branch, extract comment ID
+/// 3. Full reference: use existing parsing
+async fn resolve_pr_reference(
+ reference: Option<&str>,
+ default_repo: Option<&str>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the shared branch-and-repo resolution into a helper to simplify and de-duplicate `resolve_pr_reference`.
You can reduce the new complexity by factoring out the shared “branch + repo” resolution and keeping `resolve_pr_reference` focused on classifying the reference and wiring calls together.
Specifically, the `None` and `Some(input) if is_fragment_only(input)` arms duplicate:
- `current_branch().ok_or(VkError::InvalidRef)?`
- `default_repo.and_then(parse_repo_str).or_else(repo_from_fetch_head).ok_or(VkError::RepoNotFound)?`
- `fetch_pr_for_branch(client, &repo, &branch).await?`
Refactor that into a small helper and reuse it:
```rust
fn resolve_branch_and_repo(
default_repo: Option<&str>,
) -> Result<(RepoInfo, String), VkError> {
let branch = current_branch().ok_or(VkError::InvalidRef)?;
let repo = default_repo
.and_then(parse_repo_str)
.or_else(repo_from_fetch_head)
.ok_or(VkError::RepoNotFound)?;
Ok((repo, branch))
}
```
Then simplify `resolve_pr_reference` to:
```rust
async fn resolve_pr_reference(
reference: Option<&str>,
default_repo: Option<&str>,
client: &GraphQLClient,
) -> Result<(RepoInfo, u64, Option<u64>), VkError> {
match reference {
None => {
let (repo, branch) = resolve_branch_and_repo(default_repo)?;
let number = fetch_pr_for_branch(client, &repo, &branch).await?;
Ok((repo, number, None))
}
Some(input) if is_fragment_only(input) => {
let comment_id = parse_fragment_only(input)?;
let (repo, branch) = resolve_branch_and_repo(default_repo)?;
let number = fetch_pr_for_branch(client, &repo, &branch).await?;
Ok((repo, number, Some(comment_id)))
}
Some(input) => parse_pr_thread_reference(input, default_repo),
}
}
```
This:
- Removes duplicated logic and ensures the “detect PR from branch” path stays consistent.
- Keeps `resolve_pr_reference` focused on reference classification + orchestration, with repo/branch resolution in a single, testable helper.
- Preserves all current behavior, including the new “no reference” and fragment-only cases.
</issue_to_address>
### Comment 4
<location> `src/commands.rs:174` </location>
<code_context>
+/// 1. No reference: detect PR from current branch
+/// 2. Fragment only (`#discussion_r<ID>`): detect PR from branch, extract comment ID
+/// 3. Full reference: use existing parsing
+async fn resolve_pr_reference(
+ reference: Option<&str>,
+ default_repo: Option<&str>,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add both unit and behavioural tests for the new PR auto-detection and fragment-only resolution logic.
Exercise `resolve_pr_reference` via tests that cover all three match arms:
- `None` reference: PR auto-detected from the current branch (happy path and error cases such as missing `.git/HEAD`, no repo, or no PR for branch).
- Fragment-only reference: `#discussion_r<ID>` combined with branch auto-detection, ensuring the comment ID is correctly propagated.
- Full reference: regression tests verifying existing behaviour is preserved.
These should include at least one higher-level/behavioural test (e.g., integration or e2e) for the CLI flow, not just pure unit tests.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `src/commands.rs:180` </location>
<code_context>
+ client: &GraphQLClient,
+) -> Result<(RepoInfo, u64, Option<u64>), VkError> {
+ match reference {
+ None => {
+ let branch = current_branch().ok_or(VkError::InvalidRef)?;
+ let repo = default_repo
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Refactor the duplicated branch and repo resolution logic in `resolve_pr_reference` to keep the code DRY while remaining readable.
The `None` and `Some(input) if is_fragment_only(input)` arms both perform:
- `current_branch().ok_or(VkError::InvalidRef)?`
- the same `default_repo.and_then(parse_repo_str).or_else(repo_from_fetch_head).ok_or(VkError::RepoNotFound)?` pipeline.
Extract this into a small helper (e.g., `fn resolve_branch_and_repo(...)`) to avoid duplication while keeping the control flow clear.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
Keep code DRY, but readable. Use refactoring approaches best suited for the language in question. Context managers and generators for Python; RAII and macros for Rust.
</details>
</issue_to_address>
### Comment 6
<location> `src/branch_pr.rs:43` </location>
<code_context>
+///
+/// Returns [`VkError::NoPrForBranch`] if no PR exists for the branch, or
+/// propagates API errors from the underlying request.
+pub async fn fetch_pr_for_branch(
+ client: &GraphQLClient,
+ repo: &RepoInfo,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit tests for `fetch_pr_for_branch` covering both successful and `NoPrForBranch` cases.
Currently the tests only cover deserialization helpers, not the behaviour of `fetch_pr_for_branch` itself. Add unit tests using a stub or mock `GraphQLClient` (or a test double) to verify that:
- When the underlying query returns at least one node, the function returns the correct PR number.
- When `nodes` is empty, the function returns `VkError::NoPrForBranch` with the expected branch value.
This satisfies the requirement for unit tests on new functionality.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 7
<location> `src/main.rs:57` </location>
<code_context>
- /// thread starting from the referenced comment. When a fragment is
- /// provided, both resolved and unresolved threads are searched.
- /// Without a fragment, only unresolved threads are shown.
+ /// When invoked without arguments, detects the PR associated with the
+ /// current Git branch. Passing a `#discussion_r<ID>` fragment shows only
+ /// that discussion thread, auto-detecting the PR when no number or URL
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests to cover the new CLI behaviour where `pr` without arguments auto-detects the PR from the current branch and supports bare `#discussion_r<ID>` fragments.
You changed the `pr` command semantics so that:
- Invoking `pr` with no reference auto-detects the PR from the current branch.
- Passing a bare `#discussion_r<ID>` fragment auto-detects the PR and filters to that thread.
Add behavioural/e2e tests (e.g., in `tests/e2e.rs`) that invoke the binary or integration layer to validate these flows end-to-end, including error paths (no PR for branch, invalid fragment, etc.). Unit tests alone are not sufficient per the review instructions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 8
<location> `src/cli_args.rs:59` </location>
<code_context>
+ /// A bare `#discussion_r<ID>` fragment shows only that discussion thread
+ /// (auto-detecting the PR). File filters are ignored when a fragment is
+ /// provided; unresolved filtering still applies.
+ #[arg(required = false)]
+ // The argument is optional; when absent the PR is auto-detected from the
+ // current branch. The `Option` allows `PrArgs::default()` and config
</code_context>
<issue_to_address>
**issue (review_instructions):** Ensure there are tests exercising the new optional `reference` argument semantics for the `pr` subcommand.
Changing `reference` from required to optional fundamentally alters how argument parsing behaves. Add tests that:
- Verify `pr` with no positional reference parses successfully and flows into the new auto-detection logic.
- Verify `pr` with a number, URL, and bare `#discussion_r<ID>` still parse as expected.
These can be CLI-level tests (e.g., invoking `PrArgs::try_parse_from` or higher-level behavioural tests) to demonstrate the new behaviour is correct.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 9
<location> `src/ref_parser.rs:253` </location>
<code_context>
+ /// Creates a temporary directory with a `.git` subdirectory and writes the
+ /// provided `head_content` to `.git/HEAD`. Changes to that directory,
+ /// executes the closure, then restores the original working directory.
+ fn with_git_head<F>(head_content: &str, test_fn: F)
+ where
+ F: FnOnce(),
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Shared test setup in `with_git_head` is implemented as a helper function rather than an `rstest` fixture as requested.
The `with_git_head` helper encapsulates shared setup for multiple tests, but the review guidelines request using `rstest` fixtures for shared setup. Consider turning this into a fixture (e.g. a function or async fixture returning a temp repo with `.git/HEAD` configured) and injecting it into the tests via `#[rstest]` instead of calling a helper directly.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Use `rstest` fixtures for shared setup.
</details>
</issue_to_address>
### Comment 10
<location> `src/ref_parser.rs:99` </location>
<code_context>
+///
+/// ```
+/// # use vk::ref_parser::parse_repo_str;
+/// let repo = parse_repo_str("owner/repo").unwrap();
+/// assert_eq!(repo.owner, "owner");
+/// assert_eq!(repo.name, "repo");
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The doctest uses `.unwrap()` instead of `.expect()` which goes against the error-handling guideline.
The instructions ask to prefer `.expect()` over `.unwrap()`. In the doctest, `parse_repo_str("owner/repo").unwrap()` should be changed to something like `parse_repo_str("owner/repo").expect("valid owner/repo string")` so that failures carry an explicit message.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Prefer `.expect()` over `.unwrap()`
</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.
![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: dc3b318fbe
ℹ️ 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".
- Fix .git path resolution to work from subdirectories by using `git rev-parse --show-toplevel` instead of assuming `.git` is in cwd - Add `DetachedHead` error variant for clearer error messages when repository is in detached HEAD state - Extract `resolve_branch_and_repo` helper to reduce duplication in `resolve_pr_reference` function - Share `DISCUSSION_FRAGMENT` constant between `is_fragment_only` and `parse_fragment_only` functions - Add request assertion support to e2e tests via `set_sequential_responder_with_assert` helper - Add unit tests for `resolve_branch_and_repo` helper - Add CLI argument parsing tests for optional reference semantics - Convert `with_git_head` test helper to rstest `GitRepoFixture` struct - Replace `.unwrap()` with `.expect()` in doctests per style guidelines - Update e2e tests to initialize real git repositories for path resolution to work correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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: 7
🤖 Fix all issues with AI agents
In `@src/commands/tests.rs`:
- Around line 75-98: The test returns non-deterministic results because it
relies on the current working directory's git state; change the test
returns_repo_from_default_repo_when_provided to use an rstest temp-repo fixture
that creates a fresh git repo (git init), writes a reachable HEAD and FETCH_HEAD
(or creates an initial commit and sets HEAD to a known branch), then call
resolve_branch_and_repo(Some("owner/repo")) from that temp repo and assert the
single expected outcome (e.g., Ok((repo, branch)) with repo.owner == "owner" and
repo.name == "repo"); reference the test function
returns_repo_from_default_repo_when_provided and the resolve_branch_and_repo
function when adding the fixture and assertions so the test no longer varies by
CI/dev cwd.
In `@src/ref_parser.rs`:
- Around line 5-8: Reorder the imports in src/ref_parser.rs to satisfy rustfmt:
combine and order std imports canonically (e.g., use std::{fs, process::Command,
sync::LazyLock};) and keep external imports like use url::Url; after them, then
run cargo fmt (or cargo fmt --check) to ensure the formatting passes CI.
- Around line 60-82: current_branch() and repo_from_fetch_head() incorrectly
read .git/HEAD and .git/FETCH_HEAD directly, which breaks for worktrees/linked
gitdirs where .git is a file pointing to a gitdir; instead invoke git to resolve
the correct paths and branch: call `git rev-parse --abbrev-ref HEAD` to get the
branch name (return None when it yields "HEAD" or failure) in current_branch(),
and call `git rev-parse --git-path FETCH_HEAD` to obtain the actual FETCH_HEAD
file path before reading it in repo_from_fetch_head(); replace the direct
fs::read_to_string(root.join(".git/HEAD")) and root.join(".git/FETCH_HEAD")
usages with these git-resolved results and propagate errors as Option::None on
failure.
In `@tests/e2e.rs`:
- Around line 289-296: Extract the repeated tempdir + git setup into an rstest
fixture: create a #[fixture] (e.g., git_repo_with_fetch_head) that calls
init_git_repo(dir.path(), head_content) and writes FETCH_HEAD into
dir.path().join(".git/FETCH_HEAD") using the fetch_head parameter (provide
defaults for head_content and fetch_head), then update the tests that currently
call tempdir()/init_git_repo()/fs::write(FETCH_HEAD) (the blocks around
init_git_repo and writing FETCH_HEAD) to accept and use this fixture via
#[rstest] and pass custom head_content/fetch_head only when needed.
- Around line 30-45: The test helper init_git_repo currently passes the
--initial-branch=main flag
(StdCommand::new("git").args(["init","--initial-branch=main"])...) which
requires Git >=2.28; make it backward-compatible by removing the hard flag and
instead run git with a per-invocation config (use git -c init.defaultBranch=main
init) or attempt the current command and on failure fall back to
StdCommand::new("git").args(["init"]) and then set the branch/HEAD explicitly;
update the init_git_repo logic to prefer the -c init.defaultBranch=main style or
implement the try-with-fallback approach so older Git versions still succeed.
In `@tests/utils/mod.rs`:
- Around line 226-227: Replace the lint suppression on the helper function
set_sequential_responder_with_assert: change the attribute from
#[allow(dead_code, reason = "...")] to #[expect(dead_code, reason = "...")] so
the test-only helper uses the expected-lint annotation; keep the same reason
string and location on the function signature to preserve intent and scope.
- Around line 237-242: The request body parsing inside the handler closure
currently silently skips assertions on malformed JSON because it uses if let
Ok(...); change the parsing in the closure created at
handler.lock().expect("lock handler") (the closure taking req: &Request<Bytes>)
to fail fast: replace the conditional parse with a direct parse that panics on
error (e.g., let json =
serde_json::from_slice::<serde_json::Value>(body_bytes).expect("invalid JSON
request body");) and then call assert_fn(&json) so malformed payloads do not
bypass assertions.
♻️ Duplicate comments (1)
src/graphql_queries.rs (1)
64-71: Disambiguate branch PR lookup across forks.Filter by head repository owner (or use a search query with
head:owner:branch) and add explicit ordering;headRefNameplusfirst: 1can return the wrong PR when multiple forks share a branch name.GitHub GraphQL pullRequests headRefName headRepositoryOwner orderBy arguments
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Add a Mermaid flowchart documenting the three supported input modes for `vk pr` reference resolution: no reference (auto-detect from branch), fragment-only (#discussion_r...), and full reference (URL or owner/repo#N format). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add a Mermaid sequence diagram showing the module interactions during branch-based PR auto-detection: CLI → ref_parser → branch_pr → GitHub GraphQL API, illustrating the flow from user invocation through to PR data output. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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: 1
🤖 Fix all issues with AI agents
In `@docs/vk-design.md`:
- Around line 248-253: The FRAG_ONLY/AUTO2 path oversimplifies branch detection;
change the diagram so FRAG_ONLY routes into the existing branch-detection flow
(e.g., FRAG_ONLY --> GET_BRANCH) instead of directly to FETCH_WITH_FRAG, and
update the PR_FOUND node to conditionally split to FETCH_WITH_FRAG when a
fragment is present and to FETCH_THREADS when not (use PR_FOUND -->|Yes,
fragment present| FETCH_WITH_FRAG and PR_FOUND -->|Yes, no fragment|
FETCH_THREADS); alternatively you may implement this by referencing a shared
Mermaid subgraph for GET_BRANCH → BRANCH_OK → GET_REPO → REPO_OK → QUERY_PR →
PR_FOUND and reuse it from both AUTO and FRAG_ONLY paths so the fragment-only
resolution is explicit and unambiguous.
Owner
Author
|
@coderabbitai Have the following now been resolved? |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationsrc/main.rs: What lead to degradation?introduced similar code in: tests.pr_subcommand_parses_fragment_only,tests.pr_subcommand_parses_without_reference Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
- Route FRAG_ONLY through shared branch detection subgraph instead of oversimplified direct path to FETCH_WITH_FRAG - Add subgraph to encapsulate branch-based PR detection flow - Track fragment state with SET_FRAG/SET_NO_FRAG nodes - Split PR_FOUND into conditional paths based on fragment presence - Both AUTO and FRAG_ONLY now use identical branch detection logic 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Reorder imports in ref_parser.rs for rustfmt compliance - Use git symbolic-ref and git rev-parse --git-path for worktree/linked gitdir support in current_branch() and repo_from_fetch_head() - Make init_git_repo backward-compatible with Git < 2.28 using -c init.defaultBranch=main instead of --initial-branch - Create GitRepoWithFetchHead fixture in e2e.rs to reduce test setup duplication - Make resolve_branch_and_repo tests deterministic using rstest fixtures with proper git symbolic-ref setup - Make JSON parsing fail fast in set_sequential_responder_with_assert 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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: 1
🤖 Fix all issues with AI agents
In `@docs/vk-design.md`:
- Around line 245-246: The flowchart node GET_BRANCH currently says "Get current
branch from .git/HEAD" which is inaccurate; update the GET_BRANCH label to
describe the git-based branch resolution (worktree-safe lookup) instead of a
direct .git/HEAD read so it matches the implementation, and ensure the
downstream BRANCH_OK decision text remains consistent with that change.
♻️ Duplicate comments (2)
tests/utils/mod.rs (1)
226-227: Replace forbidden#[allow]with#[expect]on the new helper.Swap the lint suppression to
#[expect(dead_code, reason = "...")]so the policy is honoured. As per coding guidelines, avoid#[allow]and use narrowly scoped#[expect].♻️ Proposed fix
-#[allow(dead_code, reason = "helper used in some tests only")] +#[expect(dead_code, reason = "helper used in some tests only")]tests/e2e.rs (1)
95-126: Convert shared git-repo setup into an rstest fixture.Replace the shared setup with an
#[fixture]that yields the repo instance so duplicated wiring is removed and the tests align with the required fixture style. As per coding guidelines, use rstest fixtures for shared setup.♻️ Fixture sketch
+use rstest::fixture; + const DEFAULT_FETCH_HEAD: &str = "deadbeef\tnot-for-merge\tbranch 'main' of https://github.com/owner/repo.git"; + +#[fixture] +fn git_repo_with_fetch_head( + #[default("ref: refs/heads/main\n")] head: &str, + #[default(DEFAULT_FETCH_HEAD)] fetch_head: &str, +) -> GitRepoWithFetchHead { + GitRepoWithFetchHead::new(head, fetch_head) +}- let repo = GitRepoWithFetchHead::new( + let repo = git_repo_with_fetch_head( "ref: refs/heads/my-feature-branch\n", "deadbeef\tnot-for-merge\tbranch 'main' of https://github.com/owner/repo.git", );
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +381 to +412 async fn filters_by_head_owner_when_provided(upstream_repo: RepoInfo) {
let body = json!({
"data": {
"repository": {
"pullRequests": {
"nodes": [
{
"number": 100,
"headRepository": {
"owner": { "login": "other-fork" }
}
},
{
"number": 200,
"headRepository": {
"owner": { "login": "my-fork" }
}
}
]
}
}
}
})
.to_string();
let server = start_mock_server(body);
let result =
fetch_pr_for_branch(server.client(), &upstream_repo, "feature", Some("my-fork"))
.await;
assert_eq!(result.expect("success"), 200);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
- Deleted large src/branch_pr.rs and moved its contents to new src/branch_pr/mod.rs namespace - Encapsulated PR-related data structures and fetch_pr_for_branch function within branch_pr module - Added comprehensive tests for the branch_pr logic to src/branch_pr/tests.rs - Minor adjustments to ref_parser module for attribute style (allow to expect) This refactor improves module organization by isolating PR lookup logic in its own module, improving code clarity and maintainability without functional changes. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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: 4
🤖 Fix all issues with AI agents
In `@src/branch_pr/mod.rs`:
- Around line 96-99: The Map insertion uses unnecessary clones inside the json!
macro; change the inserts to pass references (e.g. json!(&repo.owner),
json!(&repo.name), and json!(branch) as-is) so you don't clone repo.owner/name
unnecessarily—update the three vars.insert calls (the Map named vars, and the
json! usage) to use references instead of .clone().
In `@src/branch_pr/tests.rs`:
- Around line 125-187: The mock server (MockServer / start_mock_server) should
capture and assert incoming request bodies so tests verify GraphQL variables;
inside the service_fn handler used in start_mock_server, read the request body
(e.g., hyper::body::to_bytes(req.into_body()).await), parse it to
serde_json::Value, and assert the expected fields like
json["variables"]["owner"], json["variables"]["name"], and
json["variables"]["headRef"] before returning the mocked response; keep the
existing RAII cleanup and client creation (GraphQLClient::with_endpoint_retry)
but add these request-inspection assertions to validate the client sends the
correct query/variables.
- Around line 189-203: The two test fixtures basic_repo and upstream_repo use
the fully-qualified attribute #[rstest::fixture]; change both to the imported
attribute #[fixture] to match the existing rstest import and project
style—update the attributes on the functions basic_repo() and upstream_repo() to
#[fixture] so the crate import is used consistently.
In `@src/ref_parser/mod.rs`:
- Around line 278-290: Add Rust doc comments (///) to the public functions
parse_issue_reference and parse_pr_reference describing their purpose (parsing a
string into (RepoInfo, u64) for issue and PR references respectively),
documenting the parameters (input: &str, default_repo: impl
Into<DefaultRepo<'a>>), the return value, and possible error VkError conditions;
place the comments immediately above each function signature and keep them
concise so cargo doc picks them up (mention ResourceType mapping to Issues and
PullRequest for clarity).
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +257 to +287 async fn filters_by_head_owner_when_provided(upstream_repo: RepoInfo) {
let body = json!({
"data": {
"repository": {
"pullRequests": {
"nodes": [
{
"number": 100,
"headRepository": {
"owner": { "login": "other-fork" }
}
},
{
"number": 200,
"headRepository": {
"owner": { "login": "my-fork" }
}
}
]
}
}
}
})
.to_string();
let server = start_mock_server(body);
let result =
fetch_pr_for_branch(server.client(), &upstream_repo, "feature", Some("my-fork")).await;
assert_eq!(result.expect("success"), 200);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
- Enhance fetch_pr_for_branch to filter PRs by head owner when provided. - Add comprehensive tests covering filtering logic and null headRepository cases. - Introduce helpers to build mock PR data for testing. - Minor code fixes: use references when inserting JSON values for owner and name. Also add parsing helpers in ref_parser for issue and pull request references. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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: 3
🤖 Fix all issues with AI agents
In `@src/ref_parser/mod.rs`:
- Around line 395-403: The module exceeds the 400-line cap; split out URL
parsing and git helper logic into one or more submodules (e.g., create new
submodules like url.rs and git.rs and declare them in mod.rs), move related
functions such as parse_fragment_only plus constants like DISCUSSION_FRAGMENT
and error type references (VkError) that belong to URL parsing into the new url
submodule, update mod.rs to pub use the moved symbols or adjust call sites to
reference ref_parser::url::parse_fragment_only, and ensure internal imports and
visibility (pub/pub(crate)) are corrected so existing callers compile.
- Around line 278-323: Add runnable doc examples for both parse_issue_reference
and parse_pr_reference by inserting a fenced code block under their /// #
Examples sections that imports the function (e.g. use
vk::ref_parser::parse_issue_reference;), calls it with a full GitHub URL (e.g.
"https://github.com/o/r/issues/42" and "https://github.com/o/r/pull/42"),
unwraps the Result, and asserts repo.owner == "o", repo.name == "r", and number
== 42; ensure the examples compile as doctests by using the same argument type
expected for default_repo (convert or pass the appropriate DefaultRepo/None
value used in project docs) and keeping the code minimal so it runs as-is in CI.
- Around line 76-78: The static GITHUB_RE currently uses .expect() and may panic
at startup; change its type to LazyLock<Result<Regex, regex::Error>> and
construct it without .expect() so compilation errors are stored, then update
parse_repo_str to retrieve and propagate that Result (e.g., map_err/and_then or
?-propagate) before using the regex — reference GITHUB_RE and parse_repo_str to
locate the change, return or propagate the regex::Error from parse_repo_str (or
convert to the function's error type) instead of letting the program panic.
♻️ Duplicate comments (1)
src/branch_pr/tests.rs (1)
153-187: Assert GraphQL variables in the mock server handler.Validate
owner,name, andheadRefin the incoming JSON so the tests verify the client’s query wiring, not just responses.♻️ Proposed refactor
+ #[derive(Clone, Copy)] + struct ExpectedVars { + owner: &'static str, + name: &'static str, + head_ref: &'static str, + } + - fn start_mock_server(body: String) -> MockServer { + fn start_mock_server(body: String, expected: ExpectedVars) -> MockServer { let body = Arc::new(body); + let expected = Arc::new(expected); let svc = third_wheel::hyper::service::make_service_fn(move |_conn| { let body = Arc::clone(&body); + let expected = Arc::clone(&expected); async move { - Ok::<_, Infallible>(service_fn(move |_req| { + Ok::<_, Infallible>(service_fn(move |req| { let body = Arc::clone(&body); + let expected = Arc::clone(&expected); async move { + let bytes = third_wheel::hyper::body::to_bytes(req.into_body()) + .await + .expect("request body"); + let json: serde_json::Value = + serde_json::from_slice(&bytes).expect("request json"); + assert_eq!(json["variables"]["owner"].as_str(), Some(expected.owner)); + assert_eq!(json["variables"]["name"].as_str(), Some(expected.name)); + assert_eq!(json["variables"]["headRef"].as_str(), Some(expected.head_ref)); Ok::<_, Infallible>( Response::builder() .status(StatusCode::OK) .header("Content-Type", "application/json") .body(Body::from(body.as_ref().clone())) .expect("response"), ) } })) } });Update call sites to pass
ExpectedVarsper test.
- Introduced new `git` module in `ref_parser` for extracting repository info and current branch - Support fetching origin remote URL, current branch, and parsing `FETCH_HEAD`, compatible with worktrees - Moved existing git detection code from `mod.rs` to `git.rs` and re-exported functions - Updated tests to reflect changes and improve PR lookup JSON construction This enhances repository context detection by interfacing directly with git commands, improving robustness for fork detection and branch identification. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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: 4
🤖 Fix all issues with AI agents
In `@src/branch_pr/tests.rs`:
- Around line 75-123: The test duplicates the predicate logic from
fetch_pr_for_branch; extract that predicate into a reusable helper in mod.rs
(e.g., pub(crate) fn head_owner_matches(pr: &PrNode, owner: &str) -> bool) that
encapsulates the head_repository/as_ref/is_some_and and eq_ignore_ascii_case
check, update fetch_pr_for_branch to call this helper, and change this test to
call the new helper (or remove it and rely on fetch_pr_for_branch_tests) so the
behavior is asserted via the single implementation rather than duplicated logic.
- Around line 228-229: Swap the attribute order so rstest runs before tokio's
test macro for each async test: place #[rstest] above #[tokio::test] for the
functions returns_pr_number_on_success, returns_no_pr_for_branch_when_empty,
head_owner_filtering, and returns_no_pr_when_head_owner_not_found; update the
attribute ordering on those function declarations so rstest's macro expansion is
applied first.
In `@src/ref_parser/mod.rs`:
- Around line 118-147: The short-ref branch in parse_repo_str currently accepts
any string containing '/' which lets inputs with extra path segments (e.g.,
"o/r/extra" or full URLs not matched by GITHUB_RE) pass; change that branch to
only accept exactly one '/' and non-empty owner and repo parts: replace the
repo.contains('/') check with a check that the slash count == 1 (or split into
parts and ensure parts.len() == 2), then validate both owner and name_part are
non-empty before returning RepoInfo and still call strip_git_suffix(name_part);
keep behavior of the GITHUB_RE branch and use the same RepoInfo construction
(refer to parse_repo_str, GITHUB_RE, strip_git_suffix, RepoInfo).
- Around line 243-280: Extract the three-branch match that parses
DISCUSSION_FRAGMENT out of parse_pr_thread_reference into a small helper (e.g.,
parse_discussion_fragment) that takes the input &str and returns Result<(&str,
Option<u64>), VkError>; inside the helper mirror the current logic:
split_once(DISCUSSION_FRAGMENT), return Err(VkError::InvalidRef) for empty
fragments or non-numeric ids and Ok((input, None)) when no fragment; then
simplify parse_pr_thread_reference to call this helper, pass the returned base
into parse_pr_reference along with default_repo, and return Ok((repo, number,
comment)). Ensure the helper uses the same symbols (DISCUSSION_FRAGMENT,
VkError::InvalidRef) and accepts default_repo only in parse_pr_reference as
before.
♻️ Duplicate comments (2)
src/branch_pr/tests.rs (1)
125-187: MockServer lacks request inspection.The
MockServercorrectly handles RAII cleanup and dynamic port allocation. However, per prior feedback, the mock server does not inspect incoming GraphQL request variables to verify the client sends the expectedheadRef,owner, andnamevalues. This remains unaddressed.src/ref_parser/mod.rs (1)
80-82: Remove panic on regex initialization.Line 80 calls
.expect()outside tests, which can panic at start-up and breaches the no-expectrule. Store the regex initialization result and short-circuit inparse_repo_str.As per coding guidelines.♻️ Proposed fix
-static GITHUB_RE: LazyLock<Regex> = LazyLock::new(|| { - Regex::new(r"github\.com[/:](?P<owner>[^/]+)/(?P<repo>[^/]+)").expect("valid regex") -}); +static GITHUB_RE: LazyLock<Result<Regex, regex::Error>> = LazyLock::new(|| { + Regex::new(r"github\.com[/:](?P<owner>[^/]+)/(?P<repo>[^/]+)") +}); @@ -pub fn parse_repo_str(repo: &str) -> Option<RepoInfo> { - if let Some(caps) = GITHUB_RE.captures(repo) { +pub fn parse_repo_str(repo: &str) -> Option<RepoInfo> { + let re = GITHUB_RE.as_ref().ok()?; + if let Some(caps) = re.captures(repo) {
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file //! Parse pull request and issue references into repository and number pairs, optionally including discussion comment IDs.
❌ New issue: String Heavy Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
… and enhance PR reference parsing with discussion fragments - Introduced `head_owner_matches` helper in branch_pr to simplify and unify case-insensitive owner comparison. - Updated `fetch_pr_for_branch` and tests to use the new helper. - Improved parse_repo_str to only accept owner/repo with exactly one slash and non-empty parts. - Added support for parsing PR references with optional discussion fragments (`#discussion_r<id>`) in ref_parser. - Refactored PR thread parsing to use a helper function for discussion fragment extraction. These changes improve code clarity, correctness, and extend PR reference parsing capabilities. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Extract parsing logic and helper types (ResourceType, parse_github_url, etc.) from mod.rs into a new internal parse.rs module - Remove redundant code and imports from mod.rs - Introduce LazyLock for regex compilation in parse.rs - Keep API surface in mod.rs minimal by re-exporting only needed items This improves code organization by separating core parsing implementation details from the public API. Additionally, enhance branch_pr tests: - Add capturing of GraphQL request variables in mock server - Verify sent request variables in tests for more robust validation. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file //! Parse pull request and issue references into repository and number pairs, optionally including discussion comment IDs.
❌ New issue: String Heavy Function Arguments |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +172 to +174 if let Some(bytes) = bytes
&& let Ok(json) = serde_json::from_slice::<Value>(&bytes)
&& let Some(vars) = json.get("variables").cloned()❌ New issue: Complex Conditional |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Refactor test code in src/branch_pr/tests.rs by extracting the logic for parsing GraphQL variables from the request body into a dedicated helper function `extract_graphql_variables`. This improves code readability and reusability in the test suite. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
fetch_pr_for_branchto resolve a PR number for a given branch via the GitHub GraphQL API.PR_FOR_BRANCH_QUERYand returnsVkError::NoPrForBranchwhen no PR is found.PR_FOR_BRANCH_QUERYto fetch a PR by repository owner/name and headRef.current_branch()to read the current branch from.git/HEAD.is_fragment_only()andparse_fragment_only()helpers to handle fragment-based references.PrArgs.referenceis now optional and supports a new description:#discussion_r<ID>fragment still auto-detects PR and limits to that discussion; file filters are ignored when a fragment is provided.resolve_pr_referenceto handle three cases:#discussion_r<ID>): detect PR from branch and extract comment ID.setup_pr_outputnow resolves PR reference asynchronously using the new logic.branch_prmodule.How it works
vk pris invoked with no arguments, the tool reads the current branch from.git/HEAD, derives the repository information (via either an explicit repo,FETCH_HEAD, or fetch/origin data), and queries GitHub for a PR whose head ref matches the current branch usingPR_FOR_BRANCH_QUERY.#discussion_r123, the tool auto-detects the PR for the current branch and returns the specified discussion context.VkError::NoPrForBranchis returned with the branch name. Detached HEAD states surface asVkError::DetachedHead.Test plan
Migration and usage
vk prwithout arguments will attempt to auto-detect the PR for the current branch.vk pr <PR-URL-or-number>orvk pr #discussion_r<ID>to target a specific thread.🌿 Generated by Terry
📎 Task: https://www.terragonlabs.com/task/48c367e5-f3d2-4172-840b-028b93e02129