fix: submit an explicitly-named bookmark regardless of author (RIG-2267)

src/commands/submit.rs

@@ -15,7 +15,7 @@ use tracing::warn;
 use unicode_segmentation::UnicodeSegmentation as _;
 
 use crate::{
-    bookmark::{BookmarkGraph, BookmarkOrPending},
+    bookmark::{BookmarkGraph, BookmarkOrPending, JJName as _},
     cli::CliConfig,
     commands::{GetBookmarksOptions, StrVisualWidth as _},
     config::{Config, ForgeType},
@@ -221,6 +221,22 @@ pub async fn submit(config: &SubmitCommandConfig, cli_config: &CliConfig<'_>) ->
 
     ensure_whatever!(!bookmarks.is_empty(), "No bookmarks in revset {}", revset);
 
+    let changes = find_changes_to_submit(
+        &jj,
+        bookmarks.iter().map(BookmarkOrPending::change_id),
+        &pending_bookmarks,
+    )?;
+
+    // Backstop (RIG-2267): pass 1 resolved a non-empty bookmark set from the
+    // revset, but pass 2 (find_changes_to_submit) resolved it to nothing to
+    // submit. Never announce bookmarks and then exit 0 with "No bookmarks
+    // pushed" — fail loudly with an actionable message.
+    ensure_whatever!(
+        !changes.is_empty(),
+        "Resolved bookmark(s) {} but found no changes to submit — the named bookmark(s) may already be merged into trunk (inspect with `jj log -r <bookmark>`). For a stacked/ancestry-walked submit, also confirm `jj config get user.email` matches the change authors.",
+        bookmarks.iter().map(|b| b.raw_name()).join(", ")
+    );
+
     let forge = ForgeImpl::new(&repo_config)?;
 
     output.log_message(&format!(
@@ -235,12 +251,6 @@ pub async fn submit(config: &SubmitCommandConfig, cli_config: &CliConfig<'_>) ->
         bookmarks.iter().map(|b| b.magenta().to_string()).join(", ")
     ));
 
-    let changes = find_changes_to_submit(
-        &jj,
-        bookmarks.iter().map(BookmarkOrPending::change_id),
-        &pending_bookmarks,
-    )?;
-
     let bookmark_graph = BookmarkGraph::from_changes(&jj, &changes, config.revset_options.tracked)?;
 
     let submission_plan = plan::plan(PlanContext {

src/submit.rs

@@ -20,25 +20,39 @@ pub mod plan;
 pub mod stack_link;
 
 /// Find the changes that matter for a submission starting from `targets`:
-/// bookmarked changes authored by the current user that are reachable from
-/// the targets and are not already in the trunk ancestry.
+/// bookmarked changes reachable from the targets that are not already in the
+/// trunk ancestry. An explicitly-named target is included regardless of its
+/// author; only the ancestry-walked companions are narrowed to `mine()` (so a
+/// stacked submit does not sweep in other people's bookmarks). This split is
+/// the RIG-2267 fix: filtering the explicit target by `mine()` too made
+/// `submit <bookmark>` silently no-op (exit 0, "No bookmarks pushed") whenever
+/// the bookmark's commit author differed from the configured `user.email`.
 pub fn find_changes_to_submit(
     jj: &Jujutsu,
     targets: impl IntoIterator<Item = impl JJName>,
     change_ids_pending_bookmarks: &HashSet<String, impl BuildHasher>,
 ) -> Result<Vec<Change>> {
+    let target_atoms: Vec<String> = targets.into_iter().map(|t| t.name_for_jj()).collect();
+
+    let explicit = if target_atoms.is_empty() {
+        "none()".to_owned()
+    } else {
+        target_atoms.iter().join(" | ")
+    };
+    let ancestry = if target_atoms.is_empty() {
+        "none()".to_owned()
+    } else {
+        target_atoms.iter().map(|t| format!("::{t}")).join(" | ")
+    };
+    let pending = if change_ids_pending_bookmarks.is_empty() {
+        "none()".to_owned()
+    } else {
+        change_ids_pending_bookmarks.iter().join(" | ")
+    };
+
     jj.log_with_pending_bookmarks(
         format!(
-            "((({}) & mine() & bookmarks()) | ({})) ~ (::trunk())",
-            targets
-                .into_iter()
-                .map(|t| format!("::{}", t.name_for_jj()))
-                .join(" | "),
-            if change_ids_pending_bookmarks.is_empty() {
-                "none()".to_owned()
-            } else {
-                change_ids_pending_bookmarks.iter().join(" | ")
-            }
+            "(({explicit}) | (({ancestry}) & mine() & bookmarks()) | ({pending})) ~ (::trunk())"
         ),
         change_ids_pending_bookmarks,
     )

src/tests/edge_cases.rs

@@ -167,6 +167,90 @@ fn find_changes_to_submit_with_advanced_main() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn find_changes_to_submit_includes_foreign_authored_named_target() -> Result<()> {
+    let repo = TestRepo::with_local_remote();
+
+    repo.jj.exec(["new", "main"])?;
+    repo.create_change("f1.txt", "f1", "Feature 1")
+        .create_bookmark("feature");
+
+    // Author the bookmarked commit under a *different* identity than the one
+    // configured now — the RIG-2267 trigger (the fleet commit-author flip left
+    // pre-flip commits authored under the old identity). `mine()` would drop it.
+    repo.set_config("user.email", "seal@sealedsecurity.com");
+    repo.set_config("user.name", "seal");
+    repo.jj.exec(["metaedit", "--update-author"])?;
+    repo.set_config("user.email", "mintaka@rigel.build");
+    repo.set_config("user.name", "mintaka");
+
+    // An explicitly-named target must be submitted regardless of its author.
+    let changes = find_changes_to_submit(&repo.jj, ["feature"], &HashSet::new())?;
+    let names: Vec<_> = Bookmark::from_changes(&changes)
+        .into_iter()
+        .map(|b| b.name().to_owned())
+        .collect();
+    assert_eq!(names, vec!["feature".to_owned()]);
+
+    Ok(())
+}
+
+#[test]
+fn find_changes_to_submit_excludes_foreign_authored_ancestry_companion() -> Result<()> {
+    let repo = TestRepo::with_local_remote();
+
+    // Build a stack off trunk: a (mine) -> c (foreign) -> b (mine), where the
+    // middle bookmark `c` is authored under a *different* identity (the
+    // RIG-2267 commit-author flip). Only the explicitly-named target is taken
+    // raw; ancestry-walked companions stay narrowed to `mine()`, so a foreign
+    // companion sitting in the ancestry of the target must be EXCLUDED — the
+    // other half of the fix (a stacked submit must not sweep in other people's
+    // bookmarks). Without `& mine()` on the ancestry branch, `c` would leak in.
+    repo.set_config("user.email", "mintaka@rigel.build");
+    repo.set_config("user.name", "mintaka");
+
+    repo.jj.exec(["new", "main"])?;
+    repo.create_change("a.txt", "a", "Change A")
+        .create_bookmark("a");
+
+    repo.jj.exec(["new"])?;
+    repo.create_change("c.txt", "c", "Change C")
+        .create_bookmark("c");
+    // Re-author `c` (=@) under the old identity, then restore `user.email` so
+    // `mine()` resolves to `mintaka` at query time.
+    repo.set_config("user.email", "seal@sealedsecurity.com");
+    repo.set_config("user.name", "seal");
+    repo.jj.exec(["metaedit", "--update-author"])?;
+    repo.set_config("user.email", "mintaka@rigel.build");
+    repo.set_config("user.name", "mintaka");
+
+    repo.jj.exec(["new"])?;
+    repo.create_change("b.txt", "b", "Change B")
+        .create_bookmark("b");
+
+    // Submitting `b` walks its ancestry: `a` (mine) is included, `c` (foreign)
+    // is dropped by `& mine()`.
+    let changes = find_changes_to_submit(&repo.jj, ["b"], &HashSet::new())?;
+    let mut names: Vec<_> = Bookmark::from_changes(&changes)
+        .into_iter()
+        .map(|b| b.name().to_owned())
+        .collect();
+    names.sort();
+    assert_eq!(names, vec!["a".to_owned(), "b".to_owned()]);
+
+    // Two explicit targets exercise the multi-target `join(" | ")` on both the
+    // explicit and ancestry branches; `c` stays excluded from the ancestry.
+    let changes = find_changes_to_submit(&repo.jj, ["a", "b"], &HashSet::new())?;
+    let mut names: Vec<_> = Bookmark::from_changes(&changes)
+        .into_iter()
+        .map(|b| b.name().to_owned())
+        .collect();
+    names.sort();
+    assert_eq!(names, vec!["a".to_owned(), "b".to_owned()]);
+
+    Ok(())
+}
+
 #[cfg(not(feature = "no-e2e-tests"))]
 mod e2e {
     use assertables::assert_contains;

src/tests/submit/validation.rs

@@ -1,3 +1,7 @@
+use assertables::assert_contains;
+
+use crate::{error::Result, tests::TestRepo};
+
 #[cfg(not(feature = "no-e2e-tests"))]
 mod e2e {
     use assertables::assert_contains;
@@ -44,3 +48,34 @@ mod e2e {
         Ok(())
     }
 }
+
+#[tokio::test]
+async fn named_bookmark_in_trunk_errors_instead_of_silent_noop() -> Result<()> {
+    let repo = TestRepo::with_local_remote();
+
+    // A bookmark pointing at a commit already in trunk resolves in pass 1
+    // (verbatim, no mine() filter) but has nothing to submit in pass 2. It must
+    // error, not exit 0 with "No bookmarks pushed" (RIG-2267 backstop). The
+    // guard runs before any forge network call, so a config that merely passes
+    // validation plus --dry-run keeps this off the network and lets it gate CI
+    // (which runs with --features no-e2e-tests). Seed the full minimum GitHub
+    // config (forge + project + token) at the repo layer — `with_local_remote`
+    // sets no jj-vine config, and relying on an ambient user-level `forge`/token
+    // would make this test pass only on a developer box (the exact config-leak
+    // non-hermeticity documented in config.rs), while CI's clean HOME fails the
+    // parse with "missing field `forge`" before ever reaching the guard.
+    repo.set_config("jj-vine.forge", "github");
+    repo.set_config("jj-vine.github.project", "owner/repo");
+    repo.set_config("jj-vine.github.token", "gh-test-token");
+    repo.jj
+        .exec(["bookmark", "create", "on-trunk", "-r", "main"])?;
+
+    let result = repo.try_run(["submit", "on-trunk", "--dry-run"]).await;
+
+    assert_contains!(
+        result.unwrap_err().to_string(),
+        "found no changes to submit"
+    );
+
+    Ok(())
+}