From 7dec0273f587ab53c9d1cc1d2866dff6a19a4a89 Mon Sep 17 00:00:00 2001 From: mintaka Date: Fri, 21 Aug 2026 01:01:29 -0400 Subject: [PATCH 1/2] fix: submit an explicitly-named bookmark regardless of author (RIG-2267) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `jj-vine submit ` silently no-ops — exit 0, "No bookmarks pushed" — when the bookmark's commit author differs from the configured jj `user.email`. The fleet's commit-author flip (`seal@sealedsecurity.com` → `mintaka@rigel.build`) left pre-flip commits authored under the old identity, so re-submitting one of those bookmarks quietly does nothing. Root cause is a two-pass double-filter. `submit()` resolves the bookmark set from the revset (pass 1, verbatim), announces it, then re-resolves what to actually submit via `find_changes_to_submit` (pass 2). Pass 2 intersected the explicitly-named target with `mine()`, so a foreign-authored named bookmark resolved to the empty set — announced, then dropped, with a success exit. Fix (both halves): - **`find_changes_to_submit` (`src/submit.rs`)** — include an explicitly-named target *raw*; only the ancestry-walked companions stay narrowed by `mine()` (so a stacked submit still doesn't sweep in other people's bookmarks). Revset becomes `(({explicit}) | (({ancestry}) & mine() & bookmarks()) | ({pending})) ~ (::trunk())`. - **`submit()` guard (`src/commands/submit.rs`)** — after pass 2, if pass 1 announced a non-empty bookmark set but pass 2 resolved nothing, fail loudly with an actionable message instead of exiting 0. The guard runs *before* `ForgeImpl::new`, so it fails before any token subprocess or HTTP-client construction. Two red-green kill-tests: `find_changes_to_submit_includes_foreign_authored_named_target` (Option A — a bookmark authored under a different identity is still found) and `named_bookmark_in_trunk_errors_instead_of_silent_noop` (Option B — the guard fires). Both fail on the pre-fix code and pass after. This fix originated in the orion `oss/forks/jj-vine/` subtree (RIG-2267) and moved here when the fork externalized to its own repo. The `Test` CI job may show one pre-existing failure — `config::tests::resolved_token_errors_on_non_utf8_output`, a `printf '\xff\xfe'` fixture that only emits non-UTF-8 bytes under bash, not the dash `/bin/sh` on GitHub's ubuntu runners. That is unrelated to this change and is already fixed by PR #1 (`fix-non-utf8-token-test-portability`); it clears once that merges. Spec-impact: none. Closes RIG-2267 Co-authored-by: Matt Wilkinson --- src/commands/submit.rs | 22 ++++++++++++++------ src/submit.rs | 38 +++++++++++++++++++++++----------- src/tests/edge_cases.rs | 28 +++++++++++++++++++++++++ src/tests/submit/validation.rs | 35 +++++++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 18 deletions(-) diff --git a/src/commands/submit.rs b/src/commands/submit.rs index 381aadc..247e156 100644 --- a/src/commands/submit.rs +++ b/src/commands/submit.rs @@ -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 `). For a stacked/ancestry-walked submit, also confirm `jj config get user.email` matches the change authors.", + bookmarks.iter().map(ToString::to_string).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 { diff --git a/src/submit.rs b/src/submit.rs index 7018183..67e3c28 100644 --- a/src/submit.rs +++ b/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 ` 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, change_ids_pending_bookmarks: &HashSet, ) -> Result> { + let target_atoms: Vec = 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, ) diff --git a/src/tests/edge_cases.rs b/src/tests/edge_cases.rs index 3ae1da4..5de2565 100644 --- a/src/tests/edge_cases.rs +++ b/src/tests/edge_cases.rs @@ -167,6 +167,34 @@ 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(()) +} + #[cfg(not(feature = "no-e2e-tests"))] mod e2e { use assertables::assert_contains; diff --git a/src/tests/submit/validation.rs b/src/tests/submit/validation.rs index 9fcda55..71ac11d 100644 --- a/src/tests/submit/validation.rs +++ b/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(()) +} From b806e2feacefb11cfe92b712cfeef589e8eb0f4f Mon Sep 17 00:00:00 2001 From: mintaka Date: Fri, 21 Aug 2026 01:50:41 -0400 Subject: [PATCH 2/2] fix(submit): cover ancestry mine()-narrowing and drop ANSI from guard error (RIG-2267) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review of the RIG-2267 fix. - Test (medium): the fix has a dual invariant — an explicitly-named target is taken raw, but ancestry-walked companions stay `& mine()` so a stacked submit does not sweep in other people's bookmarks. Only the first half was covered. Adds `find_changes_to_submit_excludes_foreign_authored_ancestry_companion`: a stack `a(mine) -> c(foreign) -> b(mine)` where submitting `b` must resolve to exactly `{a, b}` — `c` is excluded by `& mine()`. Proven red (resolves `["a","b","c"]`) if `& mine()` is dropped from the ancestry branch, green with the fix. The two-explicit-target assertion in the same test also exercises the multi-target `join(" | ")` on both the explicit and ancestry branches, previously only single-target. - Guard message (low): the announce-then-empty backstop rendered bookmark names through `BookmarkOrPending`'s `Display`, which colorizes unconditionally (`.magenta()`), so the propagated error string carried raw ANSI escapes into non-tty stderr / CI logs. Switch to `JJName::raw_name()` for the uncolored name. No production revset or control-flow change; `find_changes_to_submit` and the guard placement are unchanged. Spec-impact: none. Refs RIG-2267 Co-authored-by: Matt Wilkinson --- src/commands/submit.rs | 4 +-- src/tests/edge_cases.rs | 56 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/commands/submit.rs b/src/commands/submit.rs index 247e156..1a8927f 100644 --- a/src/commands/submit.rs +++ b/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}, @@ -234,7 +234,7 @@ pub async fn submit(config: &SubmitCommandConfig, cli_config: &CliConfig<'_>) -> 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 `). For a stacked/ancestry-walked submit, also confirm `jj config get user.email` matches the change authors.", - bookmarks.iter().map(ToString::to_string).join(", ") + bookmarks.iter().map(|b| b.raw_name()).join(", ") ); let forge = ForgeImpl::new(&repo_config)?; diff --git a/src/tests/edge_cases.rs b/src/tests/edge_cases.rs index 5de2565..28a97c4 100644 --- a/src/tests/edge_cases.rs +++ b/src/tests/edge_cases.rs @@ -195,6 +195,62 @@ fn find_changes_to_submit_includes_foreign_authored_named_target() -> Result<()> 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;