feat: add open pull request workflow#49
Conversation
cosentinode
left a comment
There was a problem hiding this comment.
Found two correctness concerns:
-
P1:
existing_pull_requestsis queried only by head branch and the first result is returned without verifyingbase_ref_name == base. Foropen pr to releasewhen that branch already has an open PR tomain, the confirmation saysrelease, but execution reports the PR tomainas success. The same fallback after a create error has this issue. Filter/reject on a base mismatch (and cover it with a test) so confirmation and result agree. -
P2: The preview compare URL inserts
baseandheaddirectly into its path/query. Valid Git ref names may contain#or%, which makes the displayed URL point to a fragment or decode differently from the branch passed togh. Percent-encode the ref names when composing the preview URL and add coverage for such a ref name.
|
Addressed both review concerns in c2a71d0:
Verification:
|
| .map_err(open_pull_request_gh_error)?; | ||
| let existing_url = existing | ||
| .iter() | ||
| .find(|pull_request| pull_request.base_ref_name == base) |
There was a problem hiding this comment.
gh pr list --head filters only by branch name (and explicitly does not support <owner>:<branch>), but this accepts any result whose base matches. In a fork workflow, an open PR from another fork with the same branch name, such as bob:feature/open-pr -> main, is therefore returned here and presented as the PR for this checked-out/pushed branch; execution then exits without creating the user’s PR. Include and validate the head repository/owner against the tracked push remote (or otherwise avoid treating an unqualified branch-name match as the current branch), and add a fork collision test.
There was a problem hiding this comment.
Fixed in e471fe5. Existing PR detection now requests headRepository and accepts a result only when its base, head branch, and head repository all match the confirmed current repository. This applies during planning, execution, and the post-create-error fallback. Added a fork-collision regression test that confirms bob/repo:feature/open-pr is not surfaced for octo/repo:feature/open-pr and creation proceeds.\n\nVerification: cargo fmt --all --check; cargo clippy --locked --workspace --all-targets -- -D warnings; cargo test --locked --workspace.
cosentinode
left a comment
There was a problem hiding this comment.
P1: Bind gh to the verified remote before creating a PR
GitHub::new(&repo_root) invokes gh repo view, pr list, and pr create without --repo, so gh chooses its default repository independently of the remote whose pushed OID was validated above. In a standard fork setup (for example, origin is the upstream and the feature branch tracks fork/feature), this verifies the fork branch but then queries/creates against origin with the bare --head feature. The duplicate check will not find a fork-backed PR and creation can target the wrong repository or fail despite the previewed remote.
Derive and retain the GitHub repository for the selected push remote, pass it through the gh boundary with --repo, and use a qualified head when a fork is supported. Otherwise, fail closed when the selected remote is not the repository gh resolves. Add a fork/multi-remote regression test.
Reference: crates/bitbygit-tui/src/lib.rs:1943-1991 and :2792-2842.
|
Addressed P1 in aeeb8ed. The open-PR planner derives the GitHub repository from the selected, verified push URL and persists that binding into execution. The gh repository view, pull request list, and pull request create commands now all receive --repo; unsupported remote URL forms fail closed. Raw configured push URLs preserve repository identity while normal Git transport rewriting still verifies the pushed OID. Added pull_request_gh_commands_follow_the_tracked_fork_remote, covering a branch tracked from fork while an unrelated origin exists. It asserts that every repository or pull request command targets github.com/octo/repo and never upstream/repo. Verification:
|
cosentinode
left a comment
There was a problem hiding this comment.
P1: Resolve the effective push URL before binding the PR repository
remote_push_urls now reads remote.<name>.pushurl / remote.<name>.url directly instead of using git remote get-url --push. That bypasses Git URL rewrites, notably url.<fork>.pushInsteadOf=<upstream>. In that valid configuration, a tracked branch is pushed to the fork, but this planner retains the upstream URL: ls-remote validates the upstream rather than the actual push target and the derived --repo also names the upstream. The operation therefore blocks a normally pushed branch, or can create/query a PR in the wrong repository if the matching upstream branch happens to exist. Use Git’s effective push URL resolution (while keeping the test transport rewrite separate) and add a pushInsteadOf regression test.
Affected: crates/bitbygit-git/src/lib.rs:281-305.
|
Addressed the effective push URL concern in e9e48db. Added Verification:
|
cosentinode
left a comment
There was a problem hiding this comment.
Review finding\n\nP1: Create fork-backed pull requests against the intended base repository\n\ngithub_repository is derived from the tracked push URL and passed as gh pr create --repo, but --repo selects the repository that receives the PR (the base repository), not the source/head repository. In the standard fork workflow, the current branch tracks fork/feature while the desired PR is fork:feature -> upstream:main; this implementation instead runs gh pr create --repo github.com/<fork-owner>/<fork> --head feature --base main, creating (or finding) a PR entirely within the fork. The preview compare URL has the same incorrect target.\n\nThis makes open pr produce a mostly useless fork-to-fork PR even though an upstream remote is present. Model the base repository separately from the verified push/head repository, use the upstream repository as the --repo target when appropriate, and pass a qualified owner:branch head. Add a regression test covering fork/feature -> upstream/main.\n\nAffected: crates/bitbygit-tui/src/lib.rs:1944-1950, crates/bitbygit-tui/src/lib.rs:2827-2869.
|
Addressed the fork-backed PR target concern in 63ccb09.
Added Verification:
|
cosentinode
left a comment
There was a problem hiding this comment.
P1: Use valid gh command forms for repository lookup and PR discovery
with_repository appends --repo to gh repo view (crates/bitbygit-gh/src/lib.rs:65-72), but gh repo view takes the repository as its optional positional argument and does not support --repo. On the current CLI, gh repo view --json ... --repo github.com/octo/repo exits with unknown flag: --repo, so plan_open_pull_request always fails before displaying a confirmation.
There are two additional incompatible forms in the same boundary: gh pr list --limit 0 is rejected (invalid value for --limit: 0), and gh pr list --head owner:branch explicitly does not support the qualified fork syntax used here. The latter means fork-backed existing PRs cannot be found even after fixing the first command. Use gh repo view <repository> --json ..., a positive pagination strategy for listing, and a supported query/API path for matching a fork head. The fake gh tests should reject unsupported argv shapes so this is exercised against the real CLI contract.
|
Corrected the GitHub CLI boundary in 3e8b8e2.
Verification passed: |
cosentinode
left a comment
There was a problem hiding this comment.
P1: Deserialize the REST pull response shape before treating an existing PR as a query failure
existing_pull_requests now calls gh api ... repos/{owner}/{repo}/pulls, whose entries have base.ref, head.ref, and head.repo.full_name. PullRequest still requires the GraphQL fields baseRefName, headRefName, and headRepository, so any matching existing PR causes parse_json to return InvalidOutput and blocks planning/execution instead of surfacing its URL. I reproduced the endpoint output with gh 2.96.0; it contains the REST base/head objects. Model that response (or query GraphQL consistently), then add a regression test with the actual REST JSON shape.
|
Addressed the REST/GraphQL decoding mismatch in f6ffac4.
Verification passed:
|
cosentinode
left a comment
There was a problem hiding this comment.
Review findings
P1: Preserve the selected GitHub host for authentication and existing-PR lookup
github_repository_from_push_url deliberately accepts and retains arbitrary GitHub hosts, and gh repo view <host>/owner/repo correctly targets that host. However, every operation first calls setup_status, which always authenticates only against github.com (crates/bitbygit-gh/src/lib.rs:52-58). A user authenticated only to GitHub Enterprise is therefore blocked as unauthenticated. If they also happen to have a github.com session, existing_pull_requests calls gh api without --hostname (:88-101), so it queries github.com rather than the selected Enterprise host and can miss a duplicate or surface an unrelated public PR with the same owner/repo/head. The preview URL is likewise hard-coded to github.com (crates/bitbygit-tui/src/lib.rs:1217).
Carry the parsed host through the GitHub boundary, use it for auth status and api --hostname, construct the target URL from it, and add an Enterprise-host regression test.
P1: Do not execute repository-controlled core.sshCommand during PR planning/revalidation
This change replaces the fixed non-interactive SSH command with core.sshCommand read from the repository config and placed into GIT_SSH_COMMAND (crates/bitbygit-git/src/lib.rs:825-831, :869-881). GIT_SSH_COMMAND is shell-evaluated by Git when remote_url_head_oid runs git ls-remote, so simply selecting open pr in a cloned/untrusted repository can now execute an arbitrary command supplied in .git/config. The previous implementation intentionally forced ssh plus batch/no-prompt options.
Keep the fixed trusted SSH executable for application-initiated network calls. Adapt the test transport without honoring repository core.sshCommand, and add a regression test showing a configured command is not invoked.
|
GitHub Enterprise support is intentionally out of the current MVP scope and is deferred to #50. The PR remains responsible for the separate repository-config execution concern; a focused fix and fresh review will follow. |
|
Addressed the repository-controlled SSH command concern in d93cd2b.
Verification passed:
GitHub Enterprise host support remains out of scope here and is tracked in #50. |
cosentinode
left a comment
There was a problem hiding this comment.
Review findings
P1: Validate the effective push URL before invoking Git transport
plan_open_pull_request passes the effective URL returned from repository configuration to remote_url_head_oid / git ls-remote before github_repository_from_push_url verifies that it is a GitHub HTTPS/SSH URL (crates/bitbygit-tui/src/lib.rs:1935-1955, crates/bitbygit-git/src/lib.rs:275-286). The fixed GIT_SSH_COMMAND does not constrain other Git transports. I reproduced this with a repository configured with protocol.ext.allow=always and remote.origin.url=ext::touch <marker>: the marker command executes as soon as this verification path runs, even though the URL is rejected as non-GitHub afterward. Thus merely planning open pr in such a repository can execute a repository-configured remote helper. Parse/allowlist the effective GitHub URL before any ls-remote invocation (and constrain allowed protocols), with a regression test proving an ext:: helper is never launched.
P2: Validate and revalidate the target base branch
The requested/default base is only checked for string equality with the head and then forwarded to existing-PR lookup/create (crates/bitbygit-tui/src/lib.rs:1967-1975, :2931-2983). There is no query proving that this branch exists in the selected base repository, either while building the confirmation or immediately before creation. Consequently open pr to <typo-or-deleted-branch> displays a valid-looking target and only ends in a sanitized generic GitHub CLI failed after confirmation. This also misses issue #46's explicit base/head revalidation requirement. Resolve the base ref during planning and revalidate its existence before create; add missing/deleted-base coverage.
P2: Match GitHub repository identities case-insensitively
head_repository retains the spelling from the configured remote URL, while REST returns GitHub's canonical head.repo.full_name; the duplicate checks compare them with case-sensitive == (crates/bitbygit-tui/src/lib.rs:1960-1986, :2963-2999). GitHub repository paths are case-insensitive (for example, querying repos/COSENTINODE/BITBYGIT returns cosentinode/bitbygit), so a valid differently-cased remote misses an existing PR. gh pr create then fails because that PR already exists, and the fallback repeats the same mismatch instead of surfacing its URL. Normalize GitHub host/owner/repository identities or compare them case-insensitively, and cover a mixed-case remote.
|
Addressed all findings from review 4695972476 in commit
Verification:
The exact full workspace test command passed on the final run. |
cosentinode
left a comment
There was a problem hiding this comment.
Review findings
-
P1: Do not persist credential-bearing Git transport errors.
validate_open_pull_request_planconverts the revalidationGitErrorto aString(crates/bitbygit-tui/src/lib.rs:2347-2349) and then carries it asStepExecutionError::Blocked.GitError::Displayincludes the completels-remoteargument list and raw command detail (crates/bitbygit-git/src/lib.rs:1145-1163), while the audit path persists blocked strings after only marker-based word redaction. If an HTTPS push URL contains userinfo such ashttps://user:abc123@github.com/owner/repo.git, a successful planning probe followed by a failed execution-time probe can therefore write that credential-bearing URL into the audit log. This violates the issue requirement that tokens are not persisted. Keep this as a typed Git error with the existing generic audit representation, or explicitly redact URL userinfo/raw transport output, and add an execution-failure audit regression. -
P2: Accept valid GitHub remote URLs without a
.gitsuffix.github_repository_from_push_urlusesstrip_suffix(".git").unwrap_or_default()(crates/bitbygit-tui/src/lib.rs:2116-2122), so a normal valid remote such ashttps://github.com/owner/repoorgit@github.com:owner/repoproduces an empty repository name and is rejected before PR planning. Git accepts these URLs directly; the parser should treat.gitas optional and cover suffixless head and upstream remotes. -
P2: Allow equal branch names when the head and base repositories differ. The unconditional
if base == branchguard (crates/bitbygit-tui/src/lib.rs:1967-1972) also blocksfork-owner:main -> upstream:main. That is a valid and common fork PR because the refs belong to different repositories, and the code has already computed both repository identities and a qualified head. Restrict the equality guard to same-repository PRs and add a fork regression with equal ref names.
|
Addressed all findings from review 4696104535 in commit
Verification passed:
|
Fresh review findings
Verification performed on head |
|
Addressed both findings from #49 (comment) in
Verification passed:
The workspace test initially encountered a transient Linux |
cosentinode
left a comment
There was a problem hiding this comment.
Fresh review findings
-
P1: Do not execute repository-configured credential helpers during HTTPS head verification.
github_remote_url_head_oidlimits protocols, but it still runsgit ls-remotewith repository-local Git configuration intact (crates/bitbygit-git/src/lib.rs:279-302,:852-875), and the planner/executor invoke it atcrates/bitbygit-tui/src/lib.rs:1952-1954and:2354-2356. Git HTTPS transport executescredential.helper; neitherGIT_TERMINAL_PROMPT=0norGIT_ALLOW_PROTOCOLdisables helpers. I reproduced this under the exact environment set here by configuringcredential.helper=!<marker command>and using a credential-requiringhttps://...@github.com/...push URL: merely running the newls-remoteprobe executed the marker command. Thus the SSH-command andext::defenses still leave an application-initiated command-execution path while planningopen prin a configured/untrusted repository. Run this probe without repository-controlled credential helpers (or replace it with an authenticatedgh/API head query), and add an HTTPS credential-helper non-execution regression. -
P1: Pin the REST API calls to github.com instead of allowing
GH_HOSTto redirect them. Authentication is explicitly checked for github.com andrepo view/pr createreceive a github.com repository, but bothexisting_pull_requestsandbranch_existscallgh apiwithout--hostname(crates/bitbygit-gh/src/lib.rs:88-101,:114-125).GH_HOSToverrides that default; I verifiedGH_HOST=example.com gh api rate_limitcontacted example.com. A user withGH_HOSTset for Enterprise can therefore query a different server for base/existing-PR state while creation remains targeted at github.com. Besides blocking valid requests, a same-shaped response can make execution surface an unrelated existing URL and skip creation. Pass the selected host explicitly (github.com for this PR scope) and cover a conflictingGH_HOST. -
P2: Resolve the actual push destination instead of treating the fetch upstream as the PR head.
plan_open_pull_requestrejects based on status relative to@{upstream}and then callsupstream_push_target(crates/bitbygit-tui/src/lib.rs:1918-1958), but that helper only readsbranch.<name>.remoteand.merge(crates/bitbygit-git/src/lib.rs:376-387). It ignoresbranch.<name>.pushRemoteandremote.pushDefault. In a standard triangular fork setup wherefeaturetracksupstream/main, has been fully pushed tofork/feature, and usesbranch.feature.pushRemote=fork, this code either rejects the branch as ahead or probes/targets upstream instead of the pushed fork. That misses issue #46s current-pushed-branch criterion. Resolve and revalidate the Git push destination/ref (for example, the equivalent of@{push}) separately from the fetch/base upstream, with a triangular-workflow regression. -
P2: Canonicalize the head repository before matching existing PRs. The configured push URL slug is retained as
head_github_repository(crates/bitbygit-tui/src/lib.rs:1940-1966), while only the base is resolved throughgh repo view;matching_pull_requestlater compares the REST APIs canonicalhead.repo.full_nameagainst that configured slug (:2200-2217). GitHub keeps old repository URLs working as redirects after a rename, so a valid remote using the old slug probes successfully and the base query canonicalizes, but an already-open PR reports the new slug and is never matched. Creation then fails as a duplicate and the fallback repeats the mismatch instead of surfacing the URL. Resolve/canonicalize the head repository identity too, and add a renamed/redirected-remote duplicate regression.
Verification on fcc27afe2657c5136929ef3ea8948cca6ee2a4bf: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo test --locked --workspace all pass; current GitHub checks are green.
|
Addressed all concerns from review 4696363692 in
Verification passed:
|
cosentinode
left a comment
There was a problem hiding this comment.
Fresh review finding
- P1: Preserve the resolved push remote when the push ref atom is empty.
Git::push_targetfalls back wholesale toupstream_push_targetwhenever%(push:short)is empty (crates/bitbygit-git/src/lib.rs:391-408). In the common triangular setup where localfeaturetracksupstream/main,branch.feature.pushRemote=forkis set, andpush.defaultis left at its defaultsimple,git pushsendsfeaturetofork/feature, but the queried atoms arefork\0(remote resolved, target empty). The fallback therefore returnsupstream/main;plan_open_pull_requestprobes the wrong repository/ref and rejects a branch that is fully pushed, or can construct the wrong head if the OIDs happen to match (crates/bitbygit-tui/src/lib.rs:1921-1948). I reproduced this with local bare upstream/fork repositories: plaingit pushcreatedrefs/heads/featurein the fork while the exactfor-each-refinvocation used here returned an empty push ref. The added triangular tests avoid the bug by explicitly settingpush.default=current. Resolve the destination according to the effective push rules without replacing the already-resolved push remote with the fetch upstream, and add coverage using the defaultsimplemode with differently named local/upstream branches.
Verification on 69369d6: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo test --locked --workspace all pass. Current GitHub checks are green.
|
Addressed the default triangular push-target concern in
Verification passed:
|
cosentinode
left a comment
There was a problem hiding this comment.
Fresh review findings
-
P1: Preserve authenticated access when verifying private HTTPS heads.
github_remote_url_head_oidprepends-c credential.helper=to every constrainedls-remote(crates/bitbygit-git/src/lib.rs:293-304), and the planner/executor require that probe before any PR lookup or creation (crates/bitbygit-tui/src/lib.rs:1944-1948,:2333-2340). The empty helper resets the complete helper list, including trusted global/system helpers such as Git Credential Manager and the helper installed bygh auth setup-git. Consequently a private repository with a normalhttps://github.com/owner/repo.gitremote is rejected at “Unable to verify pushed branch” even whengh auth statusis ready and Git can ordinarily fetch/push it. The credential-helper non-execution test confirms this behavior but there is no private-HTTPS success case. Keep repository-controlled helpers out of the probe without discarding trusted authentication, or revalidate the head OID through the already-authenticated GitHub API, and add private HTTPS coverage. -
P2: Make the required workspace test command reliable under its default parallelism. On final head
9467630,cargo test --locked --workspacefailed twice consecutively withExecutableFileBusy (Text file busy), first increates_pull_request_with_literal_shell_metacharacters_in_titleand then indecodes_rest_existing_pull_request_responses.cargo test --locked -p bitbygit-gh -- --test-threads=1passes, so the shell-backedFakeGhtests still have a parallel execution race; a lucky green CI run does not make the issue verification repeatable. Remove the race rather than requiring a serial-only command that differs from issue #46’s verification.
Current GitHub checks are green. cargo fmt --all --check, clippy with -D warnings, the serial bitbygit-gh tests, and all 17 focused TUI pull-request tests pass.
|
Addressed both findings in
Verification:
All passed. |
Fresh review findings
Verification on |
|
Fixed both findings from #49 (comment) in
Verification:
All checks passed locally. |
cosentinode
left a comment
There was a problem hiding this comment.
Fresh review of current head 1787103: zero concerns. I independently inspected issue #46, the complete PR diff and surrounding code, existing tests and review state, exercised the real read-only gh API command forms, and ran cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo test --locked --workspace successfully. Current Rust and GitGuardian checks are green.
Fixes #46\n\n## Summary\n- add typed single-step open PR requests, planning, confirmation, and execution\n- surface existing open PRs without duplicate creation and revalidate pushed branch state before execution\n- keep GitHub CLI errors and audit records sanitized\n\n## Verification\n- cargo fmt --all --check\n- cargo clippy --locked --workspace --all-targets -- -D warnings\n- cargo test --locked --workspace