feat(submit): friendlier stale push error msg

[lararosekelley]

feat(submit): friendlier stale push error msg

Closes #256

• 🟣 feat(submit): friendlier stale push error msg (#258) (merged) 👈
• 🟣 feat(cli): handle merge queues (#257) (merged)
• main

---

Stack managed by git-stk

[github-actions]

Claude finished @lararosekelley's task in 2m 16s —— View job

---

Tasks

• Gather context and read changed files
• Check CI status
• Review code for correctness, safety, and coverage

---

Code Review - Confidence: 93/100

Adds a friendly stale-lease error with actionable git stk sync guidance instead of raw git rejection output.

• Bug — src/git.rs:213-227: stale_rejection collects all rejected refs regardless of reason; in a mixed failure the message names non-stale branches. (inline comment)

stale_rejection fires whenever any stale marker appears in stderr, then calls rejected_refs(stderr) which collects every [rejected] line — it doesn't filter by reason. If one branch fails with (stale info) and another with (permission denied), the bail message names both and tells the user to run git stk sync for a permissions problem. merge_queue_rejection defends against this class of mixed failure by returning None when stale markers are present, but the symmetric guard is missing here: stale_rejection needs to scope rejected_refs to only lines containing the stale markers. Fix this →

CI is still running; no failures to report yet. The integration test in tests/submit.rs is well-constructed — it manipulates the remote ref directly to produce a genuine stale-lease rejection rather than mocking the output.

[lararosekelley]

@bot review

[github-actions]

Claude finished @lararosekelley's task in 1m 57s —— View job

---

Tasks

• Gather context and read changed files
• Check CI status
• Review code for correctness, safety, and coverage

---

Code Review — Confidence: 95/100

Adds a friendly stale-lease error with actionable git stk sync guidance instead of raw git rejection output. The previous review's bug is already fixed in the current implementation.

Previous Bug — Resolved ✓

The prior inline comment flagged that stale_rejection called rejected_refs(stderr) to collect all rejected refs regardless of reason, which would misname non-stale branches in a mixed failure. The current implementation avoids this differently than the suggestion — and more conservatively:

// src/git.rs:222-233
let rejected: Vec<&str> = stderr.lines()
    .filter(|line| line.contains("[remote rejected]") || line.contains("[rejected]"))
    .collect();
if rejected.is_empty() || !rejected.iter().all(|line| line_is_stale(line)) {
    return None;  // ← all-or-nothing: any non-stale ref → fall through to raw git
}

The all(line_is_stale) gate means a mixed failure (one stale branch, one permission-denied branch) returns None and falls through to git's raw output — the user gets the real error, not wrong advice to run sync. This is strictly better than the proposed filter-to-stale-lines approach.

New: push_set_upstream_force_with_lease now also benefits — Good catch

push_set_upstream_force_with_lease (src/git.rs:265-269) previously called status() directly, bypassing all error classification. It now delegates to run_lease_push, so the first submit --push on a new branch (which uses --set-upstream) also shows the friendly stale message. This is a quiet but important fix.

Design: Ordering of merge_queue_rejection → stale_rejection

The priority is correct. merge_queue_rejection returns None whenever a stale marker is present in stderr, so a genuinely mixed queue+stale failure falls through to stale_rejection. If it's queue-only, run_lease_push returns Ok(()) early and stale is never checked. One edge case worth knowing:

If a push fails with a queued branch and a permission-denied branch (no stale markers), merge_queue_rejection calls rejected_refs(stderr) which collects both. The warning would name the permission-denied branch as "in a merge queue." This is a pre-existing issue, not introduced here.

Test Coverage — Strong

Unit tests (src/git.rs):

• a_stale_lease_push_names_the_rejected_branch — real-shaped stderr, single stale ref ✓
• a_non_fast_forward_push_is_treated_as_stale — non-fast-forward variant ✓
• an_unrelated_push_failure_is_not_classified_as_stale — permission denied, fatal ✓
• a_mixed_stale_and_non_stale_rejection_is_not_classified_as_stale — the key invariant ✓

Integration test (tests/submit.rs:846-875): uses git update-ref to rewind the bare remote out-of-band — a genuine lease mismatch, not mocked output. Verifies the user-facing message appears and that git's raw stale info output is suppressed. Well-constructed.

Minor observations

1. fetch first as a stale marker (line_is_stale): this marker appears on a non-forced push when the remote is ahead. Since these pushes are --force-with-lease, fetch first won't appear in practice, but including it is harmless and future-proof.

2. rejected_refs is now only used by merge_queue_rejection: consider making it a private module-level helper or inlining it eventually — but no urgency; it's correctly scoped and well-documented.

3. Integration test covers a single-branch stale case: the unit tests cover the multi-branch mixed case, which is sufficient. The integration test doesn't need to duplicate that.

CI

Main CI run is still in progress — no failures to report. Release workflow passed.

---

LGTM with no blocking issues. The core logic is sound, the docstrings are accurate, and the test construction is thoughtful.