Skip to content

feat: add open pull request workflow#49

Merged
cosentinode merged 16 commits into
developfrom
issue-46-open-pr-workflow
Jul 14, 2026
Merged

feat: add open pull request workflow#49
cosentinode merged 16 commits into
developfrom
issue-46-open-pr-workflow

Conversation

@cosentinode

Copy link
Copy Markdown
Owner

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

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found two correctness concerns:

  • P1: existing_pull_requests is queried only by head branch and the first result is returned without verifying base_ref_name == base. For open pr to release when that branch already has an open PR to main, the confirmation says release, but execution reports the PR to main as 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 base and head directly 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 to gh. Percent-encode the ref names when composing the preview URL and add coverage for such a ref name.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed both review concerns in c2a71d0:

  • Existing PR lookup now requires baseRefName to match the confirmed base during planning, execution, and the post-create error fallback.
  • Compare preview URL refs are percent-encoded, including # and %.

Verification:

  • cargo fmt --all --check
  • cargo test --locked -p bitbygit-tui pull_request
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

Comment thread crates/bitbygit-tui/src/lib.rs Outdated
.map_err(open_pull_request_gh_error)?;
let existing_url = existing
.iter()
.find(|pull_request| pull_request.base_ref_name == base)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cosentinode

cosentinode commented Jul 9, 2026

Copy link
Copy Markdown
Owner Author

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:

  • cargo fmt --all --check
  • cargo test --locked -p bitbygit-tui pull_request
  • cargo test --locked -p bitbygit-gh
  • cargo test --locked -p bitbygit-git
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cosentinode

cosentinode commented Jul 9, 2026

Copy link
Copy Markdown
Owner Author

Addressed the effective push URL concern in e9e48db. remote_push_urls now uses git remote get-url --push --all, so Git pushInsteadOf rewriting is applied before pushed-OID verification and GitHub repository binding. The PR fixtures now use an isolated SSH transport instead of URL rewriting, while retaining batch/no-prompt SSH options.

Added remote_push_urls_applies_push_instead_of_rewrites.

Verification:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cosentinode

cosentinode commented Jul 9, 2026

Copy link
Copy Markdown
Owner Author

Addressed the fork-backed PR target concern in 63ccb09.

  • The tracked remote effective push URL remains the verified head source.
  • When an upstream remote is configured, its GitHub repository is used as the PR base target and compare URL.
  • gh pr list and gh pr create receive the upstream --repo plus a qualified fork head (owner:branch); duplicate matching verifies the source head repository.
  • Execution revalidates that the selected base repository did not change. Invalid or cross-host upstream configuration fails closed without exposing command output.

Added pull_request_gh_commands_target_upstream_for_a_tracked_fork, covering fork/feature -> upstream/main.

Verification:

  • cargo fmt --all --check
  • cargo test --locked -p bitbygit-tui pull_request
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cosentinode

Copy link
Copy Markdown
Owner Author

Corrected the GitHub CLI boundary in 3e8b8e2.

  • gh repo view now receives the selected repository as its positional argument.
  • Existing PR lookup now uses gh api --method GET --paginate --slurp against the selected base repository, with typed state, head, and per_page fields. This supports qualified fork heads without gh pr list --head and avoids --limit 0.
  • Regression fakes reject the invalid shapes and assert positional repository lookup, the API request, and fork/upstream targets.

Verification passed: cargo fmt --all --check; cargo clippy --locked --workspace --all-targets -- -D warnings; cargo test --locked --workspace.

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the REST/GraphQL decoding mismatch in f6ffac4.

  • Existing PR discovery now decodes the REST /repos/{owner}/{repo}/pulls shape through a private typed response boundary: html_url, base.ref, head.ref, and head.repo.full_name.
  • The public PullRequest model remains the planner/executor boundary, preserving the decoded base ref and qualified head repository used by existing matching logic.
  • Added REST-shaped response regression coverage and updated TUI fake responses to the actual endpoint shape.

Verification passed:

  • cargo fmt --all --check
  • cargo test --locked -p bitbygit-gh -- --test-threads=1
  • cargo test --locked -p bitbygit-tui pull_request
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cosentinode

Copy link
Copy Markdown
Owner Author

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.

@cosentinode

Copy link
Copy Markdown
Owner Author

Product scope changed: GitHub Enterprise support is now tracked as implementation issue #50, dependent on #46. PR #49 remains scoped to #46; Enterprise host support will arrive through its own reviewed PR.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the repository-controlled SSH command concern in d93cd2b.

  • Git subprocesses now unconditionally use the fixed ssh executable with batch mode, zero password prompts, disabled keyboard-interactive authentication, and strict host-key checking.
  • Repository core.sshCommand and inherited GIT_SSH_COMMAND values are no longer used by application Git calls.
  • Pull-request fixtures inject their isolated SSH transport explicitly instead of storing it in repository config.
  • Added remote_url_head_oid_ignores_configured_ssh_command, which configures a marker-writing command and verifies git ls-remote does not invoke it.

Verification passed:

  • cargo test --locked -p bitbygit-git remote_url_head_oid_ignores_configured_ssh_command
  • cargo test --locked -p bitbygit-tui pull_request
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

GitHub Enterprise host support remains out of scope here and is tracked in #50.

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed all findings from review 4695972476 in commit bace2b9.

  • Validates the effective push URL as a GitHub HTTPS/SSH URL before any remote probe, and constrains the GitHub probe to https:ssh so repository-enabled ext:: helpers cannot run.
  • Resolves the requested/default base branch during planning and revalidates it immediately before PR creation, with explicit missing/deleted-base failures.
  • Matches GitHub repository identities case-insensitively for existing-PR detection and related identity revalidation.
  • Added regressions for enabled ext:: transport non-execution, encoded base refs, missing bases, deleted bases, and mixed-case remotes.

Verification:

  • cargo test --locked -p bitbygit-gh
  • cargo test --locked -p bitbygit-git github_remote_url_head_oid
  • cargo test --locked -p bitbygit-tui
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

The exact full workspace test command passed on the final run.

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review findings

  • P1: Do not persist credential-bearing Git transport errors. validate_open_pull_request_plan converts the revalidation GitError to a String (crates/bitbygit-tui/src/lib.rs:2347-2349) and then carries it as StepExecutionError::Blocked. GitError::Display includes the complete ls-remote argument 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 as https://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 .git suffix. github_repository_from_push_url uses strip_suffix(".git").unwrap_or_default() (crates/bitbygit-tui/src/lib.rs:2116-2122), so a normal valid remote such as https://github.com/owner/repo or git@github.com:owner/repo produces an empty repository name and is rejected before PR planning. Git accepts these URLs directly; the parser should treat .git as optional and cover suffixless head and upstream remotes.

  • P2: Allow equal branch names when the head and base repositories differ. The unconditional if base == branch guard (crates/bitbygit-tui/src/lib.rs:1967-1972) also blocks fork-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.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed all findings from review 4696104535 in commit c0bfed7.

  • Preserved execution-time Git transport failures as typed GitError values so audit records use the existing generic sanitized representation and cannot persist credential-bearing remote URLs or raw transport output.
  • Accepted GitHub HTTPS/SSH remote paths with or without the optional .git suffix, including suffixless fork head and upstream remotes.
  • Restricted the base=head rejection to same-repository pull requests, allowing valid fork:main -> upstream:main requests.
  • Added regressions for credential-bearing transport failure audits, suffixless head/upstream remotes, and equal branch names across fork/base repositories.

Verification passed:

  • cargo test --locked -p bitbygit-tui pull_request (14 passed)
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh review findings

  • P2: Use the tracked remote ref as the PR head instead of requiring it to match the local branch. crates/bitbygit-tui/src/lib.rs:1923-1933 rejects every branch whose valid upstream has a different ref name. For example, a local feature branch can be fully pushed while tracking origin/review-feature; the existing push planner already models branch and upstream_branch separately, but open pr reports that this current pushed branch is unsupported. This misses issue #46s acceptance criterion for the current pushed branch. Retain the local branch for HEAD/OID revalidation, use upstream_branch for ls-remote, the qualified GitHub head, and duplicate matching, and add a differently named upstream regression test.

  • P2: Restore the option delimiter when resolving a remote push URL. crates/bitbygit-git/src/lib.rs:311-318 removed the existing -- before the remote name. Git permits a remote such as -fork (git remote add -- -fork <url>), but the new invocation git remote get-url --push --all -fork parses that name as switches and fails with unknown switch f. This regresses both the existing push workflow and the new PR planner. git remote get-url --push --all -- -fork succeeds and still applies pushInsteadOf, so retain the delimiter and cover a leading-hyphen remote.

Verification performed on head c0bfed7: cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and isolated cargo test --locked --workspace all pass; current GitHub checks are green.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed both findings from #49 (comment) in fcc27af.

  • The open-PR workflow now keeps the local branch for HEAD revalidation while carrying the tracked remote branch through pushed-OID checks, GitHub head construction, base/head validation, existing-PR matching, and execution-time revalidation. Differently named tracking refs such as feature/open-pr -> origin/review-feature are accepted and existing PRs are surfaced without duplication.
  • git remote get-url --push --all once again uses -- before the remote name, preserving pushInsteadOf behavior while accepting leading-hyphen remotes such as -fork.
  • Added regressions that remove the same-named remote ref before planning and verify lookup/matching uses review-feature, plus direct leading-hyphen remote URL resolution coverage.

Verification passed:

  • cargo test --locked -p bitbygit-git remote_push_urls
  • cargo test --locked -p bitbygit-tui pull_request (15 passed)
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

The workspace test initially encountered a transient Linux ETXTBSY in an unchanged bitbygit-gh fake-executable test. cargo test --locked -p bitbygit-gh -- --test-threads=1 passed, and the exact full workspace command passed on rerun.

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh review findings

  • P1: Do not execute repository-configured credential helpers during HTTPS head verification. github_remote_url_head_oid limits protocols, but it still runs git ls-remote with repository-local Git configuration intact (crates/bitbygit-git/src/lib.rs:279-302, :852-875), and the planner/executor invoke it at crates/bitbygit-tui/src/lib.rs:1952-1954 and :2354-2356. Git HTTPS transport executes credential.helper; neither GIT_TERMINAL_PROMPT=0 nor GIT_ALLOW_PROTOCOL disables helpers. I reproduced this under the exact environment set here by configuring credential.helper=!<marker command> and using a credential-requiring https://...@github.com/... push URL: merely running the new ls-remote probe executed the marker command. Thus the SSH-command and ext:: defenses still leave an application-initiated command-execution path while planning open pr in a configured/untrusted repository. Run this probe without repository-controlled credential helpers (or replace it with an authenticated gh/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_HOST to redirect them. Authentication is explicitly checked for github.com and repo view/pr create receive a github.com repository, but both existing_pull_requests and branch_exists call gh api without --hostname (crates/bitbygit-gh/src/lib.rs:88-101, :114-125). GH_HOST overrides that default; I verified GH_HOST=example.com gh api rate_limit contacted example.com. A user with GH_HOST set 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 conflicting GH_HOST.

  • P2: Resolve the actual push destination instead of treating the fetch upstream as the PR head. plan_open_pull_request rejects based on status relative to @{upstream} and then calls upstream_push_target (crates/bitbygit-tui/src/lib.rs:1918-1958), but that helper only reads branch.<name>.remote and .merge (crates/bitbygit-git/src/lib.rs:376-387). It ignores branch.<name>.pushRemote and remote.pushDefault. In a standard triangular fork setup where feature tracks upstream/main, has been fully pushed to fork/feature, and uses branch.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 through gh repo view; matching_pull_request later compares the REST APIs canonical head.repo.full_name against 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.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed all concerns from review 4696363692 in 69369d6:

  • Disabled repository-configured credential helpers for the constrained GitHub HTTPS/SSH ls-remote probe, with a non-execution regression.
  • Pinned REST pull-request and branch queries to github.com even when GH_HOST conflicts.
  • Resolved and revalidated the actual Git push destination via push metadata, including triangular pushRemote workflows, while retaining the tracked-upstream fallback.
  • Canonicalized and revalidated the head repository through gh repo view, so renamed remote URLs match existing PRs.

Verification passed:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • Focused bitbygit-gh, credential-helper, push-target, and pull-request workflow regressions
  • Real CLI host override check: GH_HOST=example.com gh api rate_limit --hostname github.com --jq .resources.core.limit

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh review finding

  • P1: Preserve the resolved push remote when the push ref atom is empty. Git::push_target falls back wholesale to upstream_push_target whenever %(push:short) is empty (crates/bitbygit-git/src/lib.rs:391-408). In the common triangular setup where local feature tracks upstream/main, branch.feature.pushRemote=fork is set, and push.default is left at its default simple, git push sends feature to fork/feature, but the queried atoms are fork\0 (remote resolved, target empty). The fallback therefore returns upstream/main; plan_open_pull_request probes 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: plain git push created refs/heads/feature in the fork while the exact for-each-ref invocation used here returned an empty push ref. The added triangular tests avoid the bug by explicitly setting push.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 default simple mode 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.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the default triangular push-target concern in 9467630.

  • When Git resolves a push remote but leaves %(push:short) empty under effective push.default=simple, Git::push_target now preserves that remote. It uses the local branch name when the push remote differs from the fetch-upstream remote, matching the default triangular behavior, while retaining the configured upstream branch for same-remote tracked refs.
  • Updated the Git resolver and end-to-end open-PR regressions to leave push.default unset while local feature / feature/open-pr tracks upstream/main and pushes to fork.

Verification passed:

  • cargo test --locked -p bitbygit-git push_target_uses_default_simple_in_triangular_workflow
  • cargo test --locked -p bitbygit-tui pull_request_uses_default_simple_in_triangular_workflow
  • cargo test --locked -p bitbygit-tui pull_request_uses_differently_named_tracked_remote_branch
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh review findings

  • P1: Preserve authenticated access when verifying private HTTPS heads. github_remote_url_head_oid prepends -c credential.helper= to every constrained ls-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 by gh auth setup-git. Consequently a private repository with a normal https://github.com/owner/repo.git remote is rejected at “Unable to verify pushed branch” even when gh auth status is 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 --workspace failed twice consecutively with ExecutableFileBusy (Text file busy), first in creates_pull_request_with_literal_shell_metacharacters_in_title and then in decodes_rest_existing_pull_request_responses. cargo test --locked -p bitbygit-gh -- --test-threads=1 passes, so the shell-backed FakeGh tests 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.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed both findings in 3ff4451.

  • P1: PR head OIDs are now verified and revalidated through the authenticated GitHub matching-refs API instead of git ls-remote. This supports private HTTPS repositories through the existing gh authentication boundary and prevents repository-controlled credential helpers from running. Added coverage for a private HTTPS remote with a malicious repository helper, plus API OID decoding/revalidation coverage.
  • P2: FakeGh now runs generated scripts through stable /bin/sh rather than executing newly written files, removing the ETXTBSY race. Added a 32-thread parallel regression test.

Verification:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • cargo test --locked -p bitbygit-git
  • cargo test --locked -p bitbygit-gh
  • cargo test --locked -p bitbygit-tui
  • 20 consecutive default-parallel bitbygit-gh runs
  • 10 additional consecutive default-parallel full-workspace runs

All passed.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh review findings

  • P1: Derive the PR base from a split fetch/push remote instead of defaulting to the fork. plan_open_pull_request correctly resolves the selected remote's effective push URL as the head (crates/bitbygit-tui/src/lib.rs:1928-1944), but github_base_repository ignores that remote's fetch URL and only looks for a separate remote literally named upstream (:2139-2147). A valid one-remote fork setup such as origin (fetch) = https://github.com/upstream/repo.git and origin (push) = https://github.com/fork/repo.git (including the equivalent pushInsteadOf setup) therefore resolves the head as fork/repo, finds no upstream remote, and also chooses fork/repo as the base. The workflow queries/creates a fork-to-fork PR and misses an existing PR against the actual upstream repository. Use the selected push remote's differing fetch repository as the base before falling back to naming conventions, and add an end-to-end split-URL regression.

  • P2: Do not globally discard an inherited GIT_SSH_COMMAND for existing Git workflows. Git::run_args now unconditionally installs the fixed SSH command (crates/bitbygit-git/src/lib.rs:867-880), whereas the base implementation only supplied it when GIT_SSH_COMMAND was absent. Because run_args backs existing fetch, push, and pull operations too, users who intentionally launch bitbygit with an SSH wrapper, identity, proxy, or custom config now have that trusted transport silently replaced and their existing remote workflows can fail. The final open-PR implementation verifies heads through the authenticated GitHub API, so it no longer needs a Git network probe that justifies this global override. Preserve the inherited user transport for established operations (while continuing not to trust repository core.sshCommand) or scope the fixed command to the specific hardened call, and add regression coverage.

Verification on 3ff4451: current GitHub checks are green; cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo test --locked --workspace all pass. I also exercised the final real gh api --paginate --slurp branch and PR query forms successfully against PR #49.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fixed both findings from #49 (comment) in 1787103.

  • Split fetch/push remotes now derive the PR base from the selected push remote's differing GitHub fetch repository before falling back to a remote named upstream. Execution revalidates the same derivation. Added an end-to-end pushInsteadOf regression proving the PR query and creation target upstream while the head remains on the fork.
  • Existing Git operations now preserve an inherited user GIT_SSH_COMMAND. When none is inherited, bitbygit still installs its fixed non-interactive SSH command, and explicit test transport overrides remain supported. Added an isolated fetch regression proving the inherited wrapper runs while repository core.sshCommand does not; the existing repository-command suppression regression also passes.

Verification:

  • cargo test --locked -p bitbygit-git fetch_preserves_inherited_ssh_command -- --nocapture
  • cargo test --locked -p bitbygit-tui pull_request_targets_fetch_repository_for_a_split_remote -- --nocapture
  • cargo test --locked -p bitbygit-git remote_url_head_oid_ignores_configured_ssh_command -- --nocapture
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace

All checks passed locally.

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cosentinode cosentinode merged commit 12ab01f into develop Jul 14, 2026
2 checks passed
@cosentinode cosentinode deleted the issue-46-open-pr-workflow branch July 14, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Phase 11] Add single-step open pr planning and execution

1 participant