Apply configured pull request base#80
Conversation
Review finding
|
|
Fixed the deferred PR target binding concern in
Verification:
|
| OperationRequest::Checkout { branch: target } | ||
| if branch.as_deref() != Some(target) => | ||
| { | ||
| branch = Some(sequence_checkout_resulting_branch(&git, target)?); |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Fixed the whole-sequence PR preflight concern in
Verification:
|
|
Follow-up hardening is in This extends the remote-checkout binding to account for known triangular push configuration ( The remote-checkout regression now changes Full verification remains green:
|
Review findings
|
|
Fixed both review concerns in
Verification:
|
Review finding
|
Review finding
Verification: |
|
Fixed the downstream PR target revalidation concern in
Verification:
|
| .ok() | ||
| .and_then(|target| head_target_branch(&target).map(ToOwned::to_owned)) | ||
| }); | ||
| let deferred_pull_request_targets = |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
Review finding
Verification: |
|
Fixed the deferred push replanning TOCTOU in
Verification:
|
Review finding
Verification: |
|
Fixed the config reload/base-binding race from #80 (comment) in
Verification:
|
|
Final hardening is in Instead of issuing a second provider-backed target resolution after detecting a reload, the executor now fails closed when The full verification set was rerun on the final two-commit state and remains green:
|
|
ZERO CONCERNS Fresh review of the complete diff against Verified locally:
All reported PR checks pass, including Linux, macOS, Windows, and GitGuardian checks. |
Summary
Verification
cargo fmt --all --checkcargo clippy --locked --workspace --all-targets -- -D warningscargo test --locked --workspaceFixes #69