Skip to content

Apply configured pull request base#80

Merged
cosentinode merged 10 commits into
developfrom
issue-69-default-pr-base
Jul 17, 2026
Merged

Apply configured pull request base#80
cosentinode merged 10 commits into
developfrom
issue-69-default-pr-base

Conversation

@cosentinode

Copy link
Copy Markdown
Owner

Summary

  • resolve pull request bases in explicit, configured, then provider-default order
  • validate configured bases through existing GitHub planning with actionable fail-closed guidance
  • cover GitHub.com, Enterprise, and secret-safe TUI diagnostics
  • add practical configuration examples

Verification

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

Fixes #69

@cosentinode

Copy link
Copy Markdown
Owner Author

Review finding

  • [P2] Re-preview configured bases before executing deferred PR steps. The new resolution at plan_open_pull_request applies the configured base only when the concrete planner runs. For a sequence such as commit ... and push and open pr, the confirmation preview still claims base: repository default (lines 1815-1828); after confirmation, the executor reloads configuration, plans, and immediately executes the PR step (lines 4159-4233). Thus the configured target is never shown before the external action. Worse, if default-base-branch changes during an earlier step, policy confirmation revalidation does not compare that setting, so the PR can be redirected to the newly configured branch without a new preview. Please carry the effective base into the deferred preview or require a new preview/confirmation when the resolved target differs, and cover both an initially configured base and a mid-sequence config change.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fixed the deferred PR target binding concern in df0a64f.

  • Deferred sequence previews now resolve and show the canonical GitHub repository and effective base using explicit, configured, then provider-default precedence.
  • The queued request remains unchanged, so confirmation and each deferred execution re-resolve current precedence/state and require a new preview/confirmation if the repository or base differs.
  • Missing/auth-failed gh now blocks before any sequence side effect because a provider-default target cannot be safely previewed without it.
  • Added regressions for provider-default sequence previews, configured-base execution, mid-sequence config changes, explicit-base preservation, and GitHub Enterprise parity.

Verification:

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

Comment thread crates/bitbygit-tui/src/lib.rs Outdated
OperationRequest::Checkout { branch: target }
if branch.as_deref() != Some(target) =>
{
branch = Some(sequence_checkout_resulting_branch(&git, target)?);

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 tracking changes before allowing predecessor side effects

This simulation records only the resulting branch name. For a valid sequence such as checkout fork/topic and push and open pr, where topic does not yet exist locally and fork is not the default remote, the preview resolves push_target("topic") from the pre-checkout repository and falls back to origin. The checkout then creates topic with fork/topic as its upstream, so the deferred push is replanned and executed against fork before the PR-step comparison at lines 4377-4391 can report that the target changed. The start-of-execution check has the same pre-checkout state and therefore repeats the stale prediction rather than preventing the push. Model the upstream/push target established by remote checkout (or revalidate the bound PR destination before every preceding side-effecting step), and add a remote-checkout -> push -> PR regression test that asserts no push occurs when re-preview is required.

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 9a64da9. Whole-sequence preflight now models the upstream/push target created by remote checkout and binds the canonical PR repository, head repository/ref, base, and compare target before step 1. The binding is recomputed before execution and compared again against the prepared PR step. The new remote_checkout_push_pr_revalidates_target_before_any_step regression changes the fork destination after preview and asserts no checkout/push side effects (empty audit and no remote ref). Full fmt, workspace clippy, and workspace tests pass.

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.

Additional hardening pushed in d9e8fd3: prospective remote-checkout PR bindings now honor branch.*.pushRemote, remote.pushDefault, and push.default, and sequence simulation tracks upstream established by an intervening push. The regression changes remote.pushDefault after preview, then asserts rejection before checkout/push, an empty audit, and no remote ref. Full workspace fmt, clippy, and tests pass.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fixed the whole-sequence PR preflight concern in 9a64da9.

  • Sequence simulation now carries the push destination established by a remote checkout, so checkout fork/topic -> push -> open PR binds the fork rather than the pre-checkout default remote.
  • Deferred PR preview binds and displays the canonical provider repository, canonical head repository/head ref, base, and compare target before execution. Unknown checkout transitions fail preflight and require a new preview.
  • Execution-start preflight recomputes the full binding before step 1, and the just-before-PR check compares the fully planned repository/head/base binding again.
  • Added a regression that changes the fork destination after preview and verifies rejection before checkout, an empty audit log, and no pushed remote ref.

Verification:

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

@cosentinode

Copy link
Copy Markdown
Owner Author

Follow-up hardening is in d9e8fd3.

This extends the remote-checkout binding to account for known triangular push configuration (branch.*.pushRemote, remote.pushDefault, and push.default) and tracks when an intervening sequence push establishes upstream for a new/untracked branch. An unresolvable prospective push target now rejects the sequence during preflight and requires a new preview.

The remote-checkout regression now changes remote.pushDefault after preview and verifies the changed PR head destination is detected before checkout/push. A Git-layer regression checks prospective targets against Git's actual configured push targets.

Full verification remains green:

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

@cosentinode

Copy link
Copy Markdown
Owner Author

Review findings

  • [P1] Bind deferred PR heads to the destination BitByGit will actually push. prospective_push_target applies branch.*.pushRemote, remote.pushDefault, and push.default, and the deferred resolver uses that result at lib.rs#L2309-L2320. The actual typed push planner does not use those settings: tracked branches are always pushed to branch.*.remote/merge, while untracked branches use default_remote_name (lib.rs#L2462-L2516). Thus a remote-checkout sequence with remote.pushDefault=origin, or a create-branch sequence with a preconfigured branch.<new>.pushRemote, previews/revalidates one PR head repository but pushes to another; the later PR plan can reject only after that external push. The added test compares against native push_target, not the destination selected by plan_push, so it misses the mismatch. Please derive the binding from the same typed push semantics that will execute and add remote-checkout/create-branch regressions asserting both the pushed ref and PR head.

  • [P2] Reject deferred base=head targets during whole-sequence preflight. The concrete planner blocks a same-repository PR when base == upstream_branch (lib.rs#L2792-L2798), but the deferred resolver proceeds directly from base selection to branch_exists (lib.rs#L2352-L2388). A sequence such as checkout origin/topic and push and open pr to topic therefore passes preview and execution-start revalidation, performs checkout/push, and is rejected only when the final concrete PR step is planned. Mirror the base/head validation in deferred preflight and cover rejection before step 1 with no audit or remote-ref side effects.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fixed both review concerns in b6cc7e2.

  • Replaced the separate prospective approximation with one typed push-target resolver shared by actual push planning, execution revalidation, deferred sequence binding, and concrete PR planning/revalidation. It handles branch.*.pushRemote, remote.pushDefault, tracked upstreams, new branches, and remote checkouts consistently.
  • Deferred PR preflight now applies the same same-repository base/head validation as concrete planning before checkout or push can run.
  • Added end-to-end regressions proving a remote checkout with a differing configured push remote and a new branch with branch.*.pushRemote push the expected remote ref and use that repository/ref as the PR head.
  • Added a base == head sequence regression that verifies rejection during whole-sequence preflight with no checkout, audit entry, or remote-ref side effect.

Verification:

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

@cosentinode

Copy link
Copy Markdown
Owner Author

Review finding

  • [P1] Revalidate the bound downstream PR target before an intermediate push. The whole-sequence check runs only once before step 1 (lib.rs#L4467-L4482), while deferred_pull_request_targets records a target only on the OpenPullRequest entry, leaving the preceding Push entry as None (lib.rs#L2201-L2214). Consequently, in fetch -> push -> open pr, if the fetch transport or a concurrent process changes remote.pushDefault, branch.*.pushRemote, or the selected remote URL after execution-start preflight, the loop replans and executes the push with that changed destination without comparing it to the confirmed PR binding (lib.rs#L4568-L4626, lib.rs#L4669-L4678). The mismatch is detected only when planning the PR, after the external push has already landed in an unconfirmed repository. Revalidate all remaining bound PR targets before each side-effecting step (at minimum before Push) and add a regression where an earlier step changes the push destination, asserting that no remote ref is created.

@cosentinode

Copy link
Copy Markdown
Owner Author

Review finding

  • [P1] Revalidate the bound downstream PR target before an intermediate push. The full deferred binding is compared only once before step 1 (lib.rs#L4467-L4482), while deferred_pull_request_targets records None for the preceding push and a target only for the later OpenPullRequest entry (lib.rs#L2201-L2214). In a sequence such as fetch -> push -> open pr, the fetch transport or a concurrent process can change remote.pushDefault, branch.*.pushRemote, or a selected remote URL after execution-start validation. The loop then replans and executes the push against that changed destination because the push entry has no PR binding to compare (lib.rs#L4568-L4678); the mismatch is detected only while planning the PR, after the commit has already been pushed to an unconfirmed repository. Recompute and compare all remaining bound PR targets before each side-effecting step, at minimum before Push, and add a regression where an earlier step changes the push destination and no remote ref is created.

Verification: cargo test --locked --workspace passes, confirming the current suite does not cover this transition.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fixed the downstream PR target revalidation concern in 2353645.

  • Before every deferred side-effecting sequence step, the executor now recomputes and compares all still-bound downstream PR targets against the confirmed preview. A changed target is rejected before the intermediate operation is planned or executed.
  • Added a fetch -> push -> open PR regression whose fetch transport changes remote.pushDefault; execution stops before push, no push audit entry is recorded, and no ref is created at the changed remote.

Verification:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • cargo test --locked -p bitbygit-tui
  • cargo test --locked -p bitbygit-tui push_revalidates_downstream_pr_target_after_fetch -- --nocapture
  • git diff --check

Comment thread crates/bitbygit-tui/src/lib.rs Outdated
.ok()
.and_then(|target| head_target_branch(&target).map(ToOwned::to_owned))
});
let deferred_pull_request_targets =

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.

[P2] Skip provider binding when policy already blocks the sequence. This call runs before prompt_sequence_policy_evaluations determines that a step is blocked. As a result, a sequence such as branches -> open pr under policy.confirmation.medium = "blocked" still invokes gh auth/repository/branch APIs and can fail with installation, authentication, or network guidance instead of producing the policy-blocked plan. That also performs provider access for an operation policy says cannot run, contrary to the existing blocked-pull preflight behavior. Evaluate the sequence policy first and avoid deferred provider resolution when it is blocked (while preserving the vector shape), and add a regression using a missing or marker-producing fake gh.

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 c92e89d. plan_prompt_sequence now computes policy evaluations before deferred PR target binding. If any request or promoted sequence evaluation is blocked, it supplies None entries for every deferred position instead of invoking the provider, preserving vector shape and the policy-blocked preview. The new blocked_prompt_sequence_does_not_bind_pull_request_target regression plans branches -> open pr with medium confirmation blocked and asserts both [None] target context and that the fake gh invocation marker was never created. Targeted TUI test, format, clippy, and full workspace tests pass.

let push_default = self
.config_value(["config", "--get", "push.default"])?
.unwrap_or_else(|| "simple".to_owned());
let target = match push_default.as_str() {

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.

[P2] Preserve support for push.default=tracking. tracking is a valid deprecated Git synonym for upstream, but it falls into the unsupported-value arm here. Because this resolver now backs ordinary push planning as well as PR planning/revalidation, repositories using that valid setting can no longer push or open a PR; on develop, push planning ignored this setting and PR planning used push_target, which resolves it successfully. Normalize tracking to upstream and include it in the resolver parity 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 c92e89d. typed_push_target now handles push.default=tracking in the same match arm as upstream. The existing resolver parity test now includes tracking and compares the typed result directly with Git-derived push_target; the targeted Git test passes, as do format, clippy, and full workspace tests.

@cosentinode

Copy link
Copy Markdown
Owner Author

Review finding

  • [P1] Revalidate the downstream PR binding after replanning the side-effecting step. Before a deferred step, the executor computes current_targets and compares it with the confirmed binding (lib.rs#L4609-L4639), but only afterward calls planner.plan_request (lib.rs#L4641). Those are independent resolutions. In fetch -> push -> open pr, if remote.pushDefault, branch.*.pushRemote, or the selected remote URL changes after deferred_pull_request_targets reads it but before plan_push reads it, the comparison succeeds for the old confirmed target, the new push plan binds the changed destination, and its execution revalidation accepts that new destination. The commit is then pushed to an unconfirmed repository, with the mismatch detected only at the PR step. This can be reproduced deterministically by having a provider command run during deferred_pull_request_targets change the Git config after the target fields have been resolved. Compare the prepared push payload to the confirmed downstream binding, or recompute/compare the binding again after plan_request and before execution; add a regression that mutates the push destination in that interval and asserts no remote ref is created.

Verification: cargo test --locked --workspace passes at c92e89d; the current suite does not cover this interval.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fixed the deferred push replanning TOCTOU in 918fe01.

  • Deferred side-effecting steps now recompute and compare every downstream PR binding again immediately after plan_request, so a push payload cannot be prepared for a repository that differs from the confirmed binding. The existing pre-plan check remains in place.
  • Execution-time typed push validation still closes changes after the post-plan check by comparing the current destination and URLs with the prepared payload.
  • Added push_revalidates_downstream_pr_target_after_replanning, which changes remote.pushDefault from origin to fork during the provider call after the pre-plan resolver captured the old fields. The sequence stops before push; no push audit entry exists and no remote ref is created at the fork.

Verification:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • cargo test --locked -p bitbygit-tui (130 passed)
  • cargo test --locked -p bitbygit-git (64 passed)
  • cargo test --locked -p bitbygit-tui push_revalidates_downstream_pr_target_after_replanning -- --nocapture
  • git diff --check
  • git diff --check HEAD^ HEAD

@cosentinode

Copy link
Copy Markdown
Owner Author

Review finding

  • [P1] Recompute downstream PR targets after the final config reload. The executor constructs planner from the policy loaded at the start of the deferred step and uses that same snapshot for both downstream-target comparisons (lib.rs#L4601-L4693). It then reloads configuration at lib.rs#L4724, but only re-evaluates confirmation policy; default_pull_request_base is not part of that evaluation and the downstream PR binding is not recomputed. Thus, in fetch -> push -> open pr, if the config changes from default-base-branch = "release" to "main" during the provider calls made by the post-plan target resolver, both comparisons accept the stale release snapshot, the reload observes main, and the push still executes. The PR step then detects the changed base only after the external push, leaving a partially applied sequence despite the target-change guard. The execution-start check has the same stale-snapshot interval at lib.rs#L4473-L4501. Reload config before the final target comparison (or recompute with the freshly loaded policy) and add a regression that changes the configured base during revalidation and asserts no push audit/ref side effect.

Verification: cargo test --locked -p bitbygit-tui passes (130 tests), confirming this interval is not covered.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fixed the config reload/base-binding race from #80 (comment) in 617574f.

  • Execution-start and per-step downstream PR target checks now reload configuration after provider-backed resolution and recompute with a fresh planner when the effective policy changed.
  • The post-plan guard now performs that reload/rebinding before its final comparison, so the same reloaded policy is used for confirmation evaluation and an intermediate push cannot run against a stale configured PR base.
  • Added push_revalidates_downstream_pr_base_after_config_reload, which changes default-base-branch from release to main during post-plan revalidation and asserts rejection before push, no push audit entry, and no remote ref.

Verification:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • cargo test --locked -p bitbygit-tui (131 passed)
  • cargo test --locked -p bitbygit-tui push_revalidates_downstream_pr -- --nocapture
  • cargo test --locked -p bitbygit-tui deferred_pull_request_stops_when_config_changes_effective_base -- --nocapture
  • cargo test --locked -p bitbygit-git typed_push_target_resolves_tracked_and_new_branches -- --nocapture
  • git diff --check
  • git diff --check HEAD^ HEAD

@cosentinode

Copy link
Copy Markdown
Owner Author

Final hardening is in 3a7c4a2 on top of 617574f.

Instead of issuing a second provider-backed target resolution after detecting a reload, the executor now fails closed when default-base-branch changed for any remaining implicit-base PR. This makes the config reload the final read before comparison and removes the secondary provider-call interval entirely. Explicit-base PRs remain unaffected by configured-default changes. The same guard applies at execution start, before side-effect planning, and after planning.

The full verification set was rerun on the final two-commit state and remains green:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • cargo test --locked -p bitbygit-tui (131 passed)
  • focused downstream-target/configured-base TUI tests
  • focused typed push-target Git test
  • git diff --check
  • git diff --check 918fe01 HEAD

@cosentinode

Copy link
Copy Markdown
Owner Author

ZERO CONCERNS

Fresh review of the complete diff against develop, issue #69, CI, tests, and surrounding PR/push/config-reload code found no blocking concerns at head 3a7c4a28a7aae1a325bb997db14f9267df34621c.

Verified locally:

  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • git diff --check origin/develop...pr-80

All reported PR checks pass, including Linux, macOS, Windows, and GitGuardian checks.

@cosentinode
cosentinode merged commit 62ae924 into develop Jul 17, 2026
8 checks passed
@cosentinode
cosentinode deleted the issue-69-default-pr-base branch July 17, 2026 22:34
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 13] Apply default PR base and surface config diagnostics

1 participant