From 71d9c8db7fe1bc6d8945648ba8003bbb4f07819d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Huang=20ChiaCheng=20=28=E9=BB=83=E5=AE=B6=E6=94=BF=29?= <1557604+suzuke@users.noreply.github.com> Date: Wed, 8 Jul 2026 11:07:44 +0800 Subject: [PATCH 1/2] feat(push): require a lease for bare force-push to feature branches (embedder P0, #2677) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bare `git push --force`/`-f`/`+refspec` to a NON-protected branch can silently overwrite commits already on the remote (another agent's or session's work). The push arm's `push_protected_violation` deliberately ignores force ("HOW not WHAT") and only guards protected refs, so nothing caught this. Add a `push_force_without_lease_violation` guard, wired into the push arm AFTER the protected-ref guard: a bare force to a non-protected branch is denied with an actionable `--force-with-lease` retry sequence; lease forms (`--force-with-lease`/`--force-if-includes`) are allowed (footgun-removal, not capability-removal); protected refs stay hard-denied upstream. Adapted onto agentic-git's existing `parse_push_argv`/`PushArgv` (NOT a copy of agend-terminal's `force_push.rs`): `PushArgv` gains a `force` flag set by a bare `--force`/`-f` flag or a `+`-prefixed positional. Force detection is option-aware, so a `-o +val` push-option VALUE is never mistaken for a force refspec. Pure deletions are exempt via `is_pure_delete_push`, which is ALL-not-ANY (#2677 F1, a CONFIRMED bypass class): a mixed `git push --force origin :del real` deletes `:del` AND force-overwrites `real`, so the deletion must NOT exempt the whole push. Exempt only when `--delete`/`-d` is present or EVERY refspec is a `:` colon-deletion. RED-first: the mixed-refspec cases were verified failing against the buggy `.any()` variant before landing `.all()`. Unit tests cover bare-force deny, lease/normal allow, trailing-force-overrides-lease, pure-deletion exempt, the three mixed forms (incl. `+refspec` and `--` end-of-options variants), and the flag classifier. A smoke e2e proves the gate is reached in the push arm (denies the bound-branch bare force with the lease message; a lease force passes through), with a real origin set up so the fail-closed trust-root guard passes first. Ports agend-terminal #2677 (force-lease gate) + #2677 F1 (ALL-not-ANY is_delete). Co-Authored-By: Claude Opus 4.8 Signed-off-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com> --- crates/agentic-git/src/main.rs | 120 ++++++++++++++++++++++++++- crates/agentic-git/src/tests.rs | 131 ++++++++++++++++++++++++++++++ crates/agentic-git/tests/smoke.rs | 46 +++++++++++ 3 files changed, 296 insertions(+), 1 deletion(-) diff --git a/crates/agentic-git/src/main.rs b/crates/agentic-git/src/main.rs index 3c8ebeb..20a9301 100644 --- a/crates/agentic-git/src/main.rs +++ b/crates/agentic-git/src/main.rs @@ -375,6 +375,24 @@ fn shim_main() { ); std::process::exit(1); } + // #2677 (embedder P0): a BARE force-push (`--force`/`-f`/`+refspec`) to a + // NON-protected branch can silently overwrite another agent's/session's + // commits — `push_protected_violation` above ignores force ("HOW not WHAT"), + // so it never catches this. Require a lease (footgun-removal, not + // capability-removal); pure deletions are exempt (ALL-not-ANY, #2677 F1). + // Runs AFTER the protected guard so a protected-ref force is denied there. + if let Some(reason) = push_force_without_lease_violation(&args) { + emit_deny_error(subcommand, &reason, &agent, Some(&binding)); + write_git_event_typed( + &home, + &agent, + subcommand, + "deny_force_no_lease", + None, + Some(&reason), + ); + std::process::exit(1); + } // #4 Δa v5: the snapshot namespace must be shim-unpushable — a // "private" ref namespace is not private to `git push`. Two // cheap layers (text substring + resolved commit-tip match); @@ -2338,6 +2356,7 @@ fn current_branch_of(worktree: &str) -> String { struct PushArgv { bulk: bool, delete: bool, + force: bool, positionals: Vec, } @@ -2356,7 +2375,18 @@ fn is_delete_push_flag(arg: &str) -> bool { || matches!(arg.strip_prefix("--"), Some(n) if n.len() >= 3 && "delete".starts_with(n)) } -/// Option-aware parse of a `push` argv into (bulk?, delete?, positionals) — skips +/// A BARE (unconditional) force FLAG: `--force` (exact) or a single-dash short +/// cluster containing `f` (`-f`, `-uf`, `-fu`). `--force-with-lease[=…]` and +/// `--force-if-includes` are the SAFE lease forms — they refuse the push if the +/// remote moved — and are deliberately NOT matched. `--force` is an exact match: +/// git rejects the ambiguous abbreviation `--forc` (shared with +/// `--force-with-lease`), so no long-form abbreviation handling is needed. The +/// `+refspec` positional force form is detected in `parse_push_argv`, not here. +fn is_bare_force_flag(arg: &str) -> bool { + arg == "--force" || (arg.starts_with('-') && !arg.starts_with("--") && arg.contains('f')) +} + +/// Option-aware parse of a `push` argv into (bulk?, delete?, force?, positionals) — skips /// flags and the values of value-taking options, so a positional is genuinely a /// remote/ref (not e.g. a `-o` push-option value). fugu design review: a bare /// positional-count heuristic mis-reads `--delete ` and option values. @@ -2364,6 +2394,7 @@ fn parse_push_argv(args: &[String]) -> PushArgv { let mut p = PushArgv { bulk: false, delete: false, + force: false, positionals: Vec::new(), }; let mut i = 1; // skip "push" @@ -2380,9 +2411,19 @@ fn parse_push_argv(args: &[String]) -> PushArgv { if is_delete_push_flag(a) { p.delete = true; } + if is_bare_force_flag(a) { + p.force = true; + } i += 1; continue; } + // A `+`-prefixed positional IS a force refspec (`+src:dst`) — bare force via + // the refspec form rather than a `--force`/`-f` flag. Detected here (not in + // `is_bare_force_flag`) so the option-aware skip above never mistakes a + // `-o +val` push-option VALUE for a force refspec. + if a.starts_with('+') { + p.force = true; + } p.positionals.push(a.clone()); i += 1; } @@ -2510,6 +2551,83 @@ fn push_cross_branch_violation( None } +/// #t-…93550-2 (embedder P0, ports agend-terminal #2677): a BARE force-push +/// (`--force`/`-f`/`+refspec`) to a NON-protected branch can SILENTLY OVERWRITE +/// commits already on the remote branch — another agent's or session's work, or a +/// wrong-based branch. `push_protected_violation` deliberately ignores force ("HOW +/// not WHAT") and only guards protected refs, so it never catches this. Require a +/// LEASE (`--force-with-lease`/`--force-if-includes`) instead: it refuses the push +/// if the remote moved since the pusher's last fetch, REMOVING the footgun while +/// KEEPING the legitimate rebase-then-force workflow (footgun-removal, not +/// capability-removal). Returns an actionable deny reason (with an executable retry +/// sequence), or `None` to allow. +/// +/// Runs AFTER `push_protected_violation` in the push arm, so a force-push to a +/// protected ref is already denied there. Deletions don't overwrite history → +/// exempt (`is_pure_delete_push`). NB: git makes a trailing `--force` override a +/// `--force-with-lease`, so ANY bare force present = unconditional and is denied +/// even if a lease flag co-occurs (`p.force` is set by any bare `--force`/`-f`/`+`). +fn push_force_without_lease_violation(args: &[String]) -> Option { + let p = parse_push_argv(args); + if !p.force || is_pure_delete_push(&p) { + return None; + } + let (remote, branch) = force_push_target(&p); + let seq = match (remote.as_deref(), branch.as_deref()) { + (Some(r), Some(b)) => format!("git fetch {r} {b} && git push --force-with-lease {r} {b}"), + _ => "git fetch && git push --force-with-lease " + .to_string(), + }; + Some(format!( + "bare force-push denied: `--force` / `-f` / a `+refspec` can SILENTLY OVERWRITE \ + commits already on the remote branch (another agent's or session's work). Re-run \ + with a lease — it refuses the push if the remote moved since your last fetch, so you \ + cannot clobber commits you have not seen:\n {seq}\nProtected refs stay hard-denied \ + regardless; this guards feature branches. If you genuinely intend to discard remote \ + commits, fetch first so the lease baseline is current." + )) +} + +/// A PURE deletion push removes refs rather than overwriting history → exempt from +/// the force gate. `--delete`/`-d` (`p.delete`) makes EVERY named ref a deletion, so +/// it is unconditionally exempt. Otherwise a push is a pure deletion only when it +/// names a remote + ≥1 refspec and EVERY refspec is a `:` colon-deletion (after +/// an optional `+`). +/// +/// #2677 F1 (agend-terminal, CONFIRMED bypass): this MUST be ALL-not-ANY. A mixed +/// `git push --force origin :del real` deletes `:del` AND force-overwrites `real`; an +/// any-refspec exemption would let `real`'s force through. Exempting only when ALL +/// refspecs are deletions keeps a lone `:del` (or `--delete`) exempt while a mixed +/// push stays gated. +fn is_pure_delete_push(p: &PushArgv) -> bool { + if p.delete { + return true; + } + p.positionals.len() > 1 + && p.positionals[1..] + .iter() + .all(|a| a.strip_prefix('+').unwrap_or(a).starts_with(':')) +} + +/// Best-effort `(remote, branch)` for the deny message's retry sequence — only when +/// BOTH are explicitly on the command line (≥2 positionals: a remote followed by a +/// refspec). With fewer we cannot tell a remote from a refspec, so we return +/// `(None, None)` and the caller falls back to the generic ` ` +/// template. Reuses the option-aware `PushArgv.positionals`, so a `-o ` value is +/// never mistaken for a remote/refspec. +fn force_push_target(p: &PushArgv) -> (Option, Option) { + if p.positionals.len() < 2 { + return (None, None); + } + let remote = Some(p.positionals[0].trim_start_matches('+').to_string()); + let branch = p.positionals.get(1).map(|a| { + let a = a.strip_prefix('+').unwrap_or(a); + let dest = a.rsplit(':').next().unwrap_or(a); + dest.strip_prefix("refs/heads/").unwrap_or(dest).to_string() + }); + (remote, branch) +} + // ── Exec ──────────────────────────────────────────────────────────────── fn exec_with_conflict_guidance( diff --git a/crates/agentic-git/src/tests.rs b/crates/agentic-git/src/tests.rs index 527462e..71552cc 100644 --- a/crates/agentic-git/src/tests.rs +++ b/crates/agentic-git/src/tests.rs @@ -3195,3 +3195,134 @@ fn cross_branch_push_allowed_forms() { // an unbound caller (empty assigned) is never restricted here assert!(push_cross_branch_violation(&vargs(&["push", "origin", "feat/b"]), "", "feat/a", false).is_none()); } + +// ── #2677 embedder P0: bare force-push requires a lease (feature branches). ── + +#[test] +fn push_force_without_lease_denies_bare_force_to_feature_branch_2677() { + // THE core #2677 gate: a bare `--force`/`-f`/`+refspec` to a non-protected + // branch is denied — it could silently clobber remote commits. RM: drop the + // `!p.force` guard (or stop setting `p.force`) → these go RED. + let deny = |a: &[&str]| push_force_without_lease_violation(&vargs(a)); + for a in [ + &["push", "--force", "origin", "feat/x"][..], + &["push", "-f", "origin", "feat/x"][..], + &["push", "-uf", "origin", "feat/x"][..], // bundled short cluster + &["push", "-fu", "origin", "feat/x"][..], + &["push", "origin", "+feat/x"][..], // +refspec force form + &["push", "origin", "+HEAD:feat/x"][..], // +src:dst force form + &["push", "--force", "feat/x"][..], // no explicit remote positional + &["push", "origin", "--", "+feat/x"][..], // + refspec after end-of-options + ] { + assert!(deny(a).is_some(), "bare force must be DENIED: {a:?}"); + } + // the deny message carries the executable lease retry sequence. + let msg = deny(&["push", "--force", "origin", "feat/x"]).unwrap(); + assert!( + msg.contains("git fetch origin feat/x") + && msg.contains("git push --force-with-lease origin feat/x"), + "actionable retry seq: {msg}" + ); + // with too few positionals to name (remote, branch), fall back to the generic + // template rather than mis-labelling a lone refspec as the remote. + assert!(deny(&["push", "--force", "feat/x"]) + .unwrap() + .contains("git push --force-with-lease ")); +} + +#[test] +fn push_force_without_lease_allows_lease_and_normal_forms_2677() { + // Lease forms are the SAFE way to force → allowed. Normal (non-force) pushes are + // untouched (zero regression to legitimate pushes). + let allow = |a: &[&str]| push_force_without_lease_violation(&vargs(a)); + for a in [ + &["push", "--force-with-lease", "origin", "feat/x"][..], + &["push", "--force-with-lease=origin/feat/x", "origin", "feat/x"][..], + &["push", "--force-if-includes", "--force-with-lease", "origin", "feat/x"][..], + &["push", "origin", "feat/x"][..], + &["push", "-u", "origin", "feat/x"][..], + &["push"][..], + &["push", "origin"][..], + &["push", "-o", "ci.skip", "origin", "feat/x"][..], // push-option value not mis-read + &["push", "-o", "+val", "origin", "feat/x"][..], // + inside an option value ≠ force + ] { + assert!(allow(a).is_none(), "must be ALLOWED: {a:?} -> {:?}", allow(a)); + } +} + +#[test] +fn push_force_trailing_bare_force_overrides_lease_2677() { + // git makes a trailing `--force` override a `--force-with-lease`, so a bare + // `--force` present alongside a lease flag is still unconditional → denied. + assert!(push_force_without_lease_violation(&vargs(&[ + "push", "--force-with-lease", "--force", "origin", "feat/x" + ])) + .is_some()); +} + +#[test] +fn push_force_pure_deletion_is_exempt_2677() { + // Deletions don't overwrite history → exempt from the force gate even with force. + let allow = |a: &[&str]| push_force_without_lease_violation(&vargs(a)); + for a in [ + &["push", "--force", "origin", ":del"][..], // colon-delete + &["push", "--force", "origin", ":a", ":b"][..], // ALL refspecs are deletes + &["push", "--force", "origin", "+:del"][..], // force-delete refspec + &["push", "--delete", "origin", "foo"][..], // --delete flag (no force needed) + &["push", "--force", "--delete", "origin", "foo"][..], // force + --delete + ] { + assert!(allow(a).is_none(), "pure deletion must be EXEMPT: {a:?} -> {:?}", allow(a)); + } +} + +#[test] +fn push_force_mixed_delete_and_overwrite_stays_gated_2677_f1() { + // #2677 F1 (CONFIRMED bypass in agend-terminal): a mixed push that deletes one + // ref AND force-overwrites another must STAY GATED — the deletion must NOT exempt + // the whole push (that was the any-arg bug). RED-first: an any-refspec + // `is_pure_delete_push` returns true here → these wrongly return None. + let deny = |a: &[&str]| push_force_without_lease_violation(&vargs(a)); + for a in [ + &["push", "--force", "origin", ":del", "real"][..], // delete + overwrite + &["push", "--force", "origin", "real", ":del"][..], // order-independent + &["push", "--force", "origin", "+:del", "real"][..], // +delete + overwrite + &["push", "origin", "+:del", "real"][..], // force via + on the delete refspec + ] { + assert!(deny(a).is_some(), "mixed delete+overwrite must stay GATED: {a:?}"); + } +} + +#[test] +fn is_pure_delete_push_is_all_not_any_2677_f1() { + // Direct unit pin of the ALL-not-ANY discriminator (the exact #2677 F1 fix). + let pure = |a: &[&str]| is_pure_delete_push(&parse_push_argv(&vargs(a))); + // ALL refspecs are deletions → pure. + assert!(pure(&["push", "origin", ":a"])); + assert!(pure(&["push", "origin", ":a", ":b"])); + assert!(pure(&["push", "origin", "+:a"])); + assert!(pure(&["push", "--delete", "origin", "foo"])); // --delete → every ref a deletion + // a single non-delete refspec means NOT pure (force on `real` must stay gated). + assert!(!pure(&["push", "origin", ":a", "real"])); + assert!(!pure(&["push", "origin", "real", ":a"])); + assert!(!pure(&["push", "origin", "feat/x"])); + assert!(!pure(&["push", "origin"])); // remote only, no refspec → not a deletion +} + +#[test] +fn is_bare_force_flag_matches_force_forms_not_lease_2677() { + for f in ["--force", "-f", "-uf", "-fu"] { + assert!(is_bare_force_flag(f), "{f} must be a bare force flag"); + } + for f in [ + "--force-with-lease", + "--force-with-lease=origin/main", + "--force-if-includes", + "--forc", // ambiguous long abbrev (git rejects) — not matched + "-u", + "--", + "--tags", + "--delete", + ] { + assert!(!is_bare_force_flag(f), "{f} must NOT be a bare force flag"); + } +} diff --git a/crates/agentic-git/tests/smoke.rs b/crates/agentic-git/tests/smoke.rs index d274dfb..5576802 100644 --- a/crates/agentic-git/tests/smoke.rs +++ b/crates/agentic-git/tests/smoke.rs @@ -199,3 +199,49 @@ echo "UNTRACKED=$(cat untracked.txt 2>/dev/null || echo MISSING)" assert!(s.contains("UNTRACKED=NEW"), "untracked file recovered:\n{s}"); let _ = std::fs::remove_dir_all(&root); } + +/// #2677 embedder P0 — WIRING: a bare `git push --force` of the agent's own +/// (non-protected) bound branch is DENIED by the shim's push arm (not merely by +/// the unit-tested guard), carrying the actionable `--force-with-lease` retry; a +/// LEASE force of the same branch is NOT caught by this gate. A real `origin` + +/// `origin/main` is set up so the earlier trust-root guard (which fails CLOSED on +/// an unresolved `origin/main..HEAD`) passes and control actually reaches the +/// force-lease gate — otherwise trust-root would deny first and mask the wiring. +#[test] +fn run_session_bare_force_push_denied_wiring_2677() { + let root = tmp("force"); + let repo = root.join("repo"); + let remote = root.join("remote.git"); + std::fs::create_dir_all(&repo).unwrap(); + let rg = real_git(); + init_repo(&rg, &repo); + // bare remote + origin/main so the trust-root range (`origin/main..HEAD`) resolves. + git(&rg, &["init", "-q", "--bare", remote.to_str().unwrap()], &root); + git(&rg, &["remote", "add", "origin", remote.to_str().unwrap()], &repo); + git(&rg, &["push", "-q", "origin", "main"], &repo); + git(&rg, &["fetch", "-q", "origin"], &repo); + let home = root.join("home"); + + let script = r#" +set -u +git push --force origin feat/force 2>push.err; echo "PUSH_RC=$?" +grep -q "bare force-push denied" push.err && echo BARE_DENIED=yes || echo BARE_DENIED=no +grep -q "force-with-lease" push.err && echo LEASE_MSG=yes || echo LEASE_MSG=no +# a LEASE force of the same branch must NOT be caught by the bare-force gate +# (it may still fail/succeed later at real git — we only assert the gate misses it). +git push --force-with-lease origin feat/force >lease.out 2>lease.err; echo "LEASE_RC=$?" +grep -q "bare force-push denied" lease.err && echo LEASE_HIT_GATE=yes || echo LEASE_HIT_GATE=no +"#; + let out = run_session(&repo, &home, &rg, "force", "feat/force", script); + let s = String::from_utf8_lossy(&out.stdout); + assert!( + out.status.success(), + "session script failed:\n{}\n{s}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(s.contains("PUSH_RC=1"), "bare force must be denied (shim exit 1):\n{s}"); + assert!(s.contains("BARE_DENIED=yes"), "the deny must be the force-lease gate:\n{s}"); + assert!(s.contains("LEASE_MSG=yes"), "deny must carry the --force-with-lease retry:\n{s}"); + assert!(s.contains("LEASE_HIT_GATE=no"), "a lease force must NOT hit the bare-force gate:\n{s}"); + let _ = std::fs::remove_dir_all(&root); +} From 69b07f0dd7473d9f740ded513bbf668ee0d4a43e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Huang=20ChiaCheng=20=28=E9=BB=83=E5=AE=B6=E6=94=BF=29?= <1557604+suzuke@users.noreply.github.com> Date: Wed, 8 Jul 2026 11:29:30 +0800 Subject: [PATCH 2/2] fix(push): don't misclassify an attached push-option value as bare force (reviewer4 F1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git accepts the ATTACHED short push-option form `-o` (`git push -o+force origin main` = push-option `+force`, NOT a force push). `parse_push_argv` skipped only the SEPARATE `-o ` form, so an attached `-o` whose value contains `f` (`-o+force`, `-oforce`, `-oci.f`) fell through to `is_bare_force_flag`'s `contains('f')` and was wrongly flagged as force → a legitimate non-force push was DENIED. This was a fail-CLOSED false-deny, not a bypass (dev3 VERIFIED no bare force slips through). Fix: skip the attached glued-value token `-o` (`is_attached_short_value_opt`) before force/delete classification — one token consumed whole, so its value is never read as force/delete. `-o` is the only short value-option `git push` accepts; long options use `--opt[=val]` (already `--`-excluded). Genuine force is unaffected: `-f`/`-uf`/`-fu`/`--force`/`+refspec` don't start with `-o`, so they still deny — no reverse fail-open (verified by RED controls, the risk the lead flagged). F2 withdrawn by reviewer4 (verified non-bug): in `git push --repo=origin --force :dead` the positional `:dead` OVERRIDES `--repo=` and is consumed as the repository, so git never performs a dead-on-origin delete; the shim not exempting it is harmless. No parser change for F2. Also document the deliberate fail-closed over-deny for a clustered `-df` (force+delete of a non-colon ref) — denied not exempted, never fail-open. RED-first: the option-value cases were verified failing (wrongly denied) against the unfixed parser before landing the skip; the genuine-force controls stay denied throughout. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com> --- crates/agentic-git/src/main.rs | 32 ++++++++++++++++++++++++++++++++ crates/agentic-git/src/tests.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/crates/agentic-git/src/main.rs b/crates/agentic-git/src/main.rs index 20a9301..fe1b757 100644 --- a/crates/agentic-git/src/main.rs +++ b/crates/agentic-git/src/main.rs @@ -2367,6 +2367,19 @@ fn push_opt_takes_value(opt: &str) -> bool { matches!(opt, "-o" | "--push-option" | "--receive-pack" | "--exec") } +/// A value-taking SHORT option in ATTACHED (glued-value) form, e.g. `-o+force` +/// = `-o` with push-option value `+force`. Git consumes the REST of the `-o…` +/// token as the option's value, so it is NEVER a `-f` force cluster (reviewer4 F1: +/// `git push -o+force` reaches remote push-option handling, not force). `-o` is the +/// only short value-option `git push` accepts; the separate form `-o ` is +/// handled by `push_opt_takes_value`, and long options use `--opt[=val]` (a `--` +/// token, already excluded from force/positional classification). This token must +/// be consumed WHOLE so its value (which may contain `f` or a leading `+`) is never +/// read as force/delete. +fn is_attached_short_value_opt(arg: &str) -> bool { + arg.len() > 2 && arg.starts_with("-o") +} + /// `--delete` / `-d` (or an unambiguous long-prefix of `--delete`; `--d`/`--de` /// collide with `--dry-run`, so require ≥3 chars). This flag turns the push /// positionals into refs to DELETE rather than refspecs. @@ -2382,6 +2395,12 @@ fn is_delete_push_flag(arg: &str) -> bool { /// git rejects the ambiguous abbreviation `--forc` (shared with /// `--force-with-lease`), so no long-form abbreviation handling is needed. The /// `+refspec` positional force form is detected in `parse_push_argv`, not here. +/// +/// The `contains('f')` short-cluster test is safe ONLY because the caller +/// (`parse_push_argv`) first skips the attached push-option token `-o` +/// (`is_attached_short_value_opt`) — otherwise `-o+force`'s value would misfire it +/// (reviewer4 F1). `-f` is the sole `f`-bearing short flag `git push` has, so any +/// remaining single-dash-with-`f` token is a genuine force cluster. fn is_bare_force_flag(arg: &str) -> bool { arg == "--force" || (arg.starts_with('-') && !arg.starts_with("--") && arg.contains('f')) } @@ -2404,6 +2423,12 @@ fn parse_push_argv(args: &[String]) -> PushArgv { i += 2; // the option AND its value continue; } + // Attached glued-value short option (`-o`, e.g. `-o+force`): ONE token — + // skip it (never a force cluster; its value may contain `f`/`+`). reviewer4 F1. + if is_attached_short_value_opt(a) { + i += 1; + continue; + } if a.starts_with('-') && a != "-" { if is_bulk_push_flag(a) { p.bulk = true; @@ -2599,6 +2624,13 @@ fn push_force_without_lease_violation(args: &[String]) -> Option { /// any-refspec exemption would let `real`'s force through. Exempting only when ALL /// refspecs are deletions keeps a lone `:del` (or `--delete`) exempt while a mixed /// push stays gated. +/// +/// Deliberate fail-CLOSED over-deny (safe — a footgun guard may over-deny when a +/// clean alternative exists): a CLUSTERED `-df` (force+delete of a non-colon ref) +/// is denied rather than exempted, because `is_delete_push_flag` matches only the +/// standalone `-d`/`--delete`, not a `-d` glued into a short cluster. The user can +/// re-run as `git push --delete …` (exempt) or `--force-with-lease`. Never a +/// fail-open: the clustered form is denied, not allowed. fn is_pure_delete_push(p: &PushArgv) -> bool { if p.delete { return true; diff --git a/crates/agentic-git/src/tests.rs b/crates/agentic-git/src/tests.rs index 71552cc..c40c3ca 100644 --- a/crates/agentic-git/src/tests.rs +++ b/crates/agentic-git/src/tests.rs @@ -3326,3 +3326,34 @@ fn is_bare_force_flag_matches_force_forms_not_lease_2677() { assert!(!is_bare_force_flag(f), "{f} must NOT be a bare force flag"); } } + +#[test] +fn push_force_option_value_not_misclassified_as_force_2677_f1() { + // reviewer4 F1: git accepts the ATTACHED push-option form `-o`; a value + // containing `f` (or a leading `+`) is a push-option VALUE, not a force flag. + // RED before the fix: `-o+force` fell through to is_bare_force_flag's + // `contains('f')` → force=true → the legitimate non-force push was wrongly DENIED. + let allow = |a: &[&str]| push_force_without_lease_violation(&vargs(a)); + for a in [ + &["push", "-o+force", "origin", "feat/x"][..], // attached, value has a `+` and `f` + &["push", "-oforce", "origin", "feat/x"][..], // attached, value has `f` + &["push", "-oci.f", "origin", "feat/x"][..], + &["push", "--push-option=+x", "origin", "feat/x"][..], // long attached (already `--`-safe) + &["push", "-o", "ci.f", "origin", "feat/x"][..], // separate form, control + ] { + assert!(allow(a).is_none(), "push-option value ≠ force: {a:?} -> {:?}", allow(a)); + } + // The fix must NOT open a reverse fail-open — genuine force STAYS denied even + // when clustered with other short flags or sitting next to a push-option. + let deny = |a: &[&str]| push_force_without_lease_violation(&vargs(a)); + for a in [ + &["push", "-f", "origin", "feat/x"][..], + &["push", "-uf", "origin", "feat/x"][..], + &["push", "-fu", "origin", "feat/x"][..], + &["push", "-fo", "pushopt", "origin", "feat/x"][..], // `-f -o pushopt` — force IS present + &["push", "-o", "x", "--force", "origin", "feat/x"][..], + &["push", "--force", "-oval", "origin", "feat/x"][..], + ] { + assert!(deny(a).is_some(), "genuine force must STAY denied: {a:?}"); + } +}