From c36109b945c18e53f7533a5d1f16d47f2e85ed8b Mon Sep 17 00:00:00 2001 From: Bram Date: Thu, 12 Mar 2026 16:40:53 +0100 Subject: [PATCH 1/3] Fix issue in push when remote is ahead --- bug-report.md | 93 ++++++++++++++++++++++++++++++++++++++++++++ src/commands.rs | 22 +++++++++++ tests/harness/mod.rs | 14 +++++++ tests/push.rs | 25 ++++++++++++ 4 files changed, 154 insertions(+) create mode 100644 bug-report.md diff --git a/bug-report.md b/bug-report.md new file mode 100644 index 0000000..f7d76fb --- /dev/null +++ b/bug-report.md @@ -0,0 +1,93 @@ +Repository information request +Bram +· +17m +ago +smart +brdv/stck:main +stck (v0.1.3) is your Rust CLI for managing stacked GitHub PRs. It automates the tedious parts — creating branches, rebasing stacks, force-pushing with lease, and updating PR base targets — while staying close to native git/gh behavior. It has 5 commands (new, submit, status, sync, push), only 3 runtime dependencies (clap, serde, serde_json), and shells out to git/gh rather than using library bindings. Currently in early public release (v0.1.x), installable via Homebrew or from source. + +Here's the current feature set across 5 commands: + +stck new — Creates the next branch in a stack. Auto-pushes, sets upstream, bootstraps a PR for the parent branch if missing, and opens a stacked PR targeting the correct base. + +stck submit [--base ] — Opens a PR for the current branch. Auto-discovers the stack parent from open PR metadata (up to 100 PRs), falls back to the default branch, or accepts an explicit --base. + +stck status — Fetches origin, discovers the linear PR stack via gh, and prints stack order, PR state (open/merged), and flags like needs_sync, needs_push, and base_mismatch. + +stck sync [--continue|--reset] — Rebases the local stack in order based on GitHub PR relationships. Handles merged parents by restacking onto the default branch. Supports resumable workflows — state is persisted to .git/stck/ so you can resolve conflicts and --continue, or --reset to start over. + +stck push — Pushes rewritten branches with --force-with-lease and retargets PR bases on GitHub. Also resumable on partial failure. + +Cross-cutting: preflight checks (clean worktree, git/gh installed, gh authenticated), git stck subcommand alias via Homebrew, and grep-friendly CLI output. + +Good catch — let me trace the exact code path to confirm whether --force-with-lease is being neutralized by a prior fetch. + +You're right — this is a confirmed bug. Here's the problem: + +run_push (line 634) calls gitops::fetch_origin() before calling gitops::push_force_with_lease(). The fetch updates the remote tracking refs (refs/remotes/origin/), which is exactly what git push --force-with-lease checks against. So by the time the push runs, git thinks your local branch is "up to date" with the remote tracking ref and allows the force push — even though your local branch is actually missing commits that exist on the remote. + +The same issue exists in run_sync (line 488 area) — it also fetches before rebasing, so the rebase itself may silently drop remote-only commits. + +The fix should be: before pushing each branch, compare the local branch tip against the (freshly fetched) remote tracking ref to verify the local branch is a descendant of the remote. If it isn't (i.e., the remote has commits the local branch doesn't), stck push should abort for that branch with a warning instead of silently overwriting. Would you like me to implement that? + +The approach: in run_push, after the fetch but before each push_force_with_lease, add a safety check using the existing gitops::is_ancestor() helper to verify that origin/ is an ancestor of the local . If it's not — meaning the remote has commits the local branch doesn't — abort for that branch with a clear error telling the user to integrate the remote changes first (e.g., pull or rebase). + +The building blocks are already there (is_ancestor wraps git merge-base --is-ancestor), so it's essentially a small guard inserted into the push loop. No changes needed to sync — the problem is only destructive at push time. + +Now I understand the test harness. I need to look at how the git stub handles push --force-with-lease and the is_ancestor check to understand what env vars to use. + +Now I have the full picture. The test needs to simulate a scenario where the remote has commits the local branch doesn't (remote is NOT an ancestor of local). I need to: + +Set up a branch where rev-parse returns different SHAs for local vs remote (so needs_push is true) + +Make the is_ancestor check for that remote→local pair return false (remote is ahead/diverged) + +Expect that push currently succeeds (the bug), so the test should assert the push goes through — but written to assert the correct behavior (abort), meaning it fails now. + +Let me write the test expecting the correct (fixed) behavior so it fails against the current code: + +Now I need to add the stck_cmd_with_stubbed_tools_custom_git helper and the is_ancestor override to the harness. The key is making merge-base --is-ancestor origin/feature-branch feature-branch return exit code 1 (not an ancestor) for the diverged scenario: + +Now I need to add setup_stubbed_tools_custom_git. Let me look at how the existing git stub is structured to inject extra behavior. Actually, a simpler approach: I can use the existing STCK_TEST_ANCESTOR_PAIRS env var mechanism in the merge-base --is-ancestor handler. The current stub defaults to allowing main:feature-branch, feature-branch:feature-child, etc. I just need a way to make a specific pair return "not ancestor". Let me re-read that section: + +The current is_ancestor stub (lines 220-247) first checks STCK_TEST_ANCESTOR_PAIRS for explicit matches, then falls through to hardcoded cases. There's no way to make it return "not ancestor" for a hardcoded pair. Rather than adding a whole custom git approach, let me add a simpler env var STCK_TEST_NOT_ANCESTOR_PAIRS: + +Good, reverted. Now let me take a cleaner approach — add a STCK_TEST_NOT_ANCESTOR_PAIRS env var to the existing git stub: + +Now add the failing test to push.rs. The scenario: feature-branch has diverged from its remote (remote has commits local doesn't), so origin/feature-branch is NOT an ancestor of feature-branch. The fix should abort, but current code will happily push: + +Now let's verify the test fails (confirming the bug): + +The test fails as expected — confirming the bug. The current code happily pushes feature-branch with --force-with-lease even though the remote has commits the local branch doesn't. The test asserts the correct behavior (exit code 1, no force push), which will pass once the fix is implemented. + +The oracle confirms this is a real bug. Here's the precise chain of events: + +Diagram +The root cause: git push --force-with-lease (without an explicit expected SHA) checks against refs/remotes/origin/. The git fetch origin on line 634–637 of commands.rs updates that tracking ref to match the actual remote — so the lease check degrades to "remote still equals what I fetched moments ago" instead of "remote still equals what my rewrite was based on". + +The proposed fix (checking is_ancestor(origin/, ) after fetch, before push) catches exactly this: if the freshly-fetched remote tip is not contained in your local branch, you'd be overwriting commits. That's precisely what the test we wrote asserts. + +Two realistic options: + +Option A — Pre-push ancestor check (recommended) +After fetch, before each push_force_with_lease, call is_ancestor("origin/", ""). If false, abort with a clear error telling the user to integrate remote changes first. Simple, uses existing gitops::is_ancestor, no new deps, works with any Git version. + +Option B — Explicit SHA lease +Use git push --force-with-lease=: with a SHA captured before the rebase/fetch. More surgically correct, but requires plumbing the expected SHA through the sync state model and changing the push command — more invasive for the same practical protection. + +I'd go with Option A because: + +It's a ~10-line guard in the run_push loop + +Uses infrastructure that already exists (is_ancestor) + +Keeps the push command simple + +Matches the test we already wrote + +Gives a clear, actionable error message + +The fix would go right before the push on line 742 — check ancestry, abort if diverged. Want me to go ahead? + +Repository information request - Amp Docs diff --git a/src/commands.rs b/src/commands.rs index 37cabc2..7d7a219 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -733,6 +733,28 @@ pub(crate) fn run_push(preflight: &env::PreflightContext) -> ExitCode { for index in state.completed_pushes..state.push_branches.len() { let branch = &state.push_branches[index]; + + let remote_ref = format!("refs/remotes/origin/{branch}"); + let local_ref = format!("refs/heads/{branch}"); + match gitops::is_ancestor(&remote_ref, &local_ref) { + Ok(true) => {} + Ok(false) => { + if let Err(save_error) = sync_state::save_push(&state) { + eprintln!("error: {save_error}"); + return ExitCode::from(1); + } + eprintln!( + "error: remote branch `origin/{branch}` has commits not in local `{branch}`; \ + pull or rebase to integrate remote changes before pushing" + ); + return ExitCode::from(1); + } + Err(message) => { + eprintln!("error: {message}"); + return ExitCode::from(1); + } + } + println!( "Pushing branch {}/{}: {}", index + 1, diff --git a/tests/harness/mod.rs b/tests/harness/mod.rs index 618259a..d7d5467 100644 --- a/tests/harness/mod.rs +++ b/tests/harness/mod.rs @@ -228,6 +228,16 @@ if [[ "${1:-}" == "merge-base" && "${2:-}" == "--is-ancestor" ]]; then descendant_branch="${descendant#refs/heads/}" descendant_branch="${descendant_branch#refs/remotes/origin/}" + if [[ -n "${STCK_TEST_NOT_ANCESTOR_PAIRS:-}" ]]; then + IFS=',' read -ra pairs <<< "${STCK_TEST_NOT_ANCESTOR_PAIRS}" + for pair in "${pairs[@]}"; do + IFS=':' read -r pa pd <<< "${pair}" + if [[ "${ancestor_branch}" == "${pa}" && "${descendant_branch}" == "${pd}" ]]; then + exit 1 + fi + done + fi + if [[ -n "${STCK_TEST_ANCESTOR_PAIRS:-}" ]]; then IFS=',' read -ra pairs <<< "${STCK_TEST_ANCESTOR_PAIRS}" for pair in "${pairs[@]}"; do @@ -238,6 +248,10 @@ if [[ "${1:-}" == "merge-base" && "${2:-}" == "--is-ancestor" ]]; then done fi + if [[ "${ancestor_branch}" == "${descendant_branch}" ]]; then + exit 0 + fi + case "${ancestor_branch}:${descendant_branch}" in main:feature-base|main:feature-branch|main:feature-child) exit 0 ;; feature-base:feature-branch|feature-base:feature-child) exit 0 ;; diff --git a/tests/push.rs b/tests/push.rs index c736df5..1fe8e56 100644 --- a/tests/push.rs +++ b/tests/push.rs @@ -364,6 +364,31 @@ fn sync_then_push_after_squash_merge_produces_correct_retargets() { )); } +#[test] +fn push_aborts_when_remote_has_commits_not_in_local_branch() { + let (temp, mut cmd) = stck_cmd_with_stubbed_tools(); + let log_path = log_path(&temp, "stck-push-diverged.log"); + cmd.env("STCK_TEST_LOG", log_path.as_os_str()); + cmd.env("STCK_TEST_NEEDS_PUSH_BRANCHES", "feature-branch"); + // Simulate remote/feature-branch NOT being an ancestor of local feature-branch, + // meaning the remote has commits that would be lost on force push. + cmd.env("STCK_TEST_NOT_ANCESTOR_PAIRS", "feature-branch:feature-branch"); + cmd.arg("push"); + + cmd.assert() + .code(1) + .stderr(predicate::str::contains("feature-branch")); + + // The force push should NOT have been executed. + if log_path.exists() { + let log = fs::read_to_string(&log_path).expect("push log should be readable"); + assert!( + !log.contains("push --force-with-lease origin feature-branch"), + "push should not force-push a branch whose remote has diverged" + ); + } +} + #[test] fn push_blocked_while_sync_state_exists() { let (temp, mut sync) = stck_cmd_with_stubbed_tools(); From 89096c9095ac1c2b89348b1f0f1a34e72b8ddacb Mon Sep 17 00:00:00 2001 From: Bram Date: Thu, 12 Mar 2026 16:43:20 +0100 Subject: [PATCH 2/3] Remove accidentally added bug-report.md --- bug-report.md | 93 --------------------------------------------------- 1 file changed, 93 deletions(-) delete mode 100644 bug-report.md diff --git a/bug-report.md b/bug-report.md deleted file mode 100644 index f7d76fb..0000000 --- a/bug-report.md +++ /dev/null @@ -1,93 +0,0 @@ -Repository information request -Bram -· -17m -ago -smart -brdv/stck:main -stck (v0.1.3) is your Rust CLI for managing stacked GitHub PRs. It automates the tedious parts — creating branches, rebasing stacks, force-pushing with lease, and updating PR base targets — while staying close to native git/gh behavior. It has 5 commands (new, submit, status, sync, push), only 3 runtime dependencies (clap, serde, serde_json), and shells out to git/gh rather than using library bindings. Currently in early public release (v0.1.x), installable via Homebrew or from source. - -Here's the current feature set across 5 commands: - -stck new — Creates the next branch in a stack. Auto-pushes, sets upstream, bootstraps a PR for the parent branch if missing, and opens a stacked PR targeting the correct base. - -stck submit [--base ] — Opens a PR for the current branch. Auto-discovers the stack parent from open PR metadata (up to 100 PRs), falls back to the default branch, or accepts an explicit --base. - -stck status — Fetches origin, discovers the linear PR stack via gh, and prints stack order, PR state (open/merged), and flags like needs_sync, needs_push, and base_mismatch. - -stck sync [--continue|--reset] — Rebases the local stack in order based on GitHub PR relationships. Handles merged parents by restacking onto the default branch. Supports resumable workflows — state is persisted to .git/stck/ so you can resolve conflicts and --continue, or --reset to start over. - -stck push — Pushes rewritten branches with --force-with-lease and retargets PR bases on GitHub. Also resumable on partial failure. - -Cross-cutting: preflight checks (clean worktree, git/gh installed, gh authenticated), git stck subcommand alias via Homebrew, and grep-friendly CLI output. - -Good catch — let me trace the exact code path to confirm whether --force-with-lease is being neutralized by a prior fetch. - -You're right — this is a confirmed bug. Here's the problem: - -run_push (line 634) calls gitops::fetch_origin() before calling gitops::push_force_with_lease(). The fetch updates the remote tracking refs (refs/remotes/origin/), which is exactly what git push --force-with-lease checks against. So by the time the push runs, git thinks your local branch is "up to date" with the remote tracking ref and allows the force push — even though your local branch is actually missing commits that exist on the remote. - -The same issue exists in run_sync (line 488 area) — it also fetches before rebasing, so the rebase itself may silently drop remote-only commits. - -The fix should be: before pushing each branch, compare the local branch tip against the (freshly fetched) remote tracking ref to verify the local branch is a descendant of the remote. If it isn't (i.e., the remote has commits the local branch doesn't), stck push should abort for that branch with a warning instead of silently overwriting. Would you like me to implement that? - -The approach: in run_push, after the fetch but before each push_force_with_lease, add a safety check using the existing gitops::is_ancestor() helper to verify that origin/ is an ancestor of the local . If it's not — meaning the remote has commits the local branch doesn't — abort for that branch with a clear error telling the user to integrate the remote changes first (e.g., pull or rebase). - -The building blocks are already there (is_ancestor wraps git merge-base --is-ancestor), so it's essentially a small guard inserted into the push loop. No changes needed to sync — the problem is only destructive at push time. - -Now I understand the test harness. I need to look at how the git stub handles push --force-with-lease and the is_ancestor check to understand what env vars to use. - -Now I have the full picture. The test needs to simulate a scenario where the remote has commits the local branch doesn't (remote is NOT an ancestor of local). I need to: - -Set up a branch where rev-parse returns different SHAs for local vs remote (so needs_push is true) - -Make the is_ancestor check for that remote→local pair return false (remote is ahead/diverged) - -Expect that push currently succeeds (the bug), so the test should assert the push goes through — but written to assert the correct behavior (abort), meaning it fails now. - -Let me write the test expecting the correct (fixed) behavior so it fails against the current code: - -Now I need to add the stck_cmd_with_stubbed_tools_custom_git helper and the is_ancestor override to the harness. The key is making merge-base --is-ancestor origin/feature-branch feature-branch return exit code 1 (not an ancestor) for the diverged scenario: - -Now I need to add setup_stubbed_tools_custom_git. Let me look at how the existing git stub is structured to inject extra behavior. Actually, a simpler approach: I can use the existing STCK_TEST_ANCESTOR_PAIRS env var mechanism in the merge-base --is-ancestor handler. The current stub defaults to allowing main:feature-branch, feature-branch:feature-child, etc. I just need a way to make a specific pair return "not ancestor". Let me re-read that section: - -The current is_ancestor stub (lines 220-247) first checks STCK_TEST_ANCESTOR_PAIRS for explicit matches, then falls through to hardcoded cases. There's no way to make it return "not ancestor" for a hardcoded pair. Rather than adding a whole custom git approach, let me add a simpler env var STCK_TEST_NOT_ANCESTOR_PAIRS: - -Good, reverted. Now let me take a cleaner approach — add a STCK_TEST_NOT_ANCESTOR_PAIRS env var to the existing git stub: - -Now add the failing test to push.rs. The scenario: feature-branch has diverged from its remote (remote has commits local doesn't), so origin/feature-branch is NOT an ancestor of feature-branch. The fix should abort, but current code will happily push: - -Now let's verify the test fails (confirming the bug): - -The test fails as expected — confirming the bug. The current code happily pushes feature-branch with --force-with-lease even though the remote has commits the local branch doesn't. The test asserts the correct behavior (exit code 1, no force push), which will pass once the fix is implemented. - -The oracle confirms this is a real bug. Here's the precise chain of events: - -Diagram -The root cause: git push --force-with-lease (without an explicit expected SHA) checks against refs/remotes/origin/. The git fetch origin on line 634–637 of commands.rs updates that tracking ref to match the actual remote — so the lease check degrades to "remote still equals what I fetched moments ago" instead of "remote still equals what my rewrite was based on". - -The proposed fix (checking is_ancestor(origin/, ) after fetch, before push) catches exactly this: if the freshly-fetched remote tip is not contained in your local branch, you'd be overwriting commits. That's precisely what the test we wrote asserts. - -Two realistic options: - -Option A — Pre-push ancestor check (recommended) -After fetch, before each push_force_with_lease, call is_ancestor("origin/", ""). If false, abort with a clear error telling the user to integrate remote changes first. Simple, uses existing gitops::is_ancestor, no new deps, works with any Git version. - -Option B — Explicit SHA lease -Use git push --force-with-lease=: with a SHA captured before the rebase/fetch. More surgically correct, but requires plumbing the expected SHA through the sync state model and changing the push command — more invasive for the same practical protection. - -I'd go with Option A because: - -It's a ~10-line guard in the run_push loop - -Uses infrastructure that already exists (is_ancestor) - -Keeps the push command simple - -Matches the test we already wrote - -Gives a clear, actionable error message - -The fix would go right before the push on line 742 — check ancestry, abort if diverged. Want me to go ahead? - -Repository information request - Amp Docs From abc0780263c0506534616eb28ab3251c2f3bf5a1 Mon Sep 17 00:00:00 2001 From: Bram Date: Thu, 12 Mar 2026 16:52:56 +0100 Subject: [PATCH 3/3] format --- tests/push.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/push.rs b/tests/push.rs index 1fe8e56..504dceb 100644 --- a/tests/push.rs +++ b/tests/push.rs @@ -372,7 +372,10 @@ fn push_aborts_when_remote_has_commits_not_in_local_branch() { cmd.env("STCK_TEST_NEEDS_PUSH_BRANCHES", "feature-branch"); // Simulate remote/feature-branch NOT being an ancestor of local feature-branch, // meaning the remote has commits that would be lost on force push. - cmd.env("STCK_TEST_NOT_ANCESTOR_PAIRS", "feature-branch:feature-branch"); + cmd.env( + "STCK_TEST_NOT_ANCESTOR_PAIRS", + "feature-branch:feature-branch", + ); cmd.arg("push"); cmd.assert()