test(config): make non-UTF-8 token fixture portable (octal, not hex printf) - #1
Merged
Merged
Conversation
…rintf) `resolved_token_errors_on_non_utf8_output` generated its invalid-UTF-8 bytes with `sh -c "printf '\xff\xfe'"`. Hex `\xHH` escapes are a bash / GNU-coreutils extension, not POSIX; a dash `/bin/sh` (GitHub ubuntu-24.04 runners) passes `\xff` through literally as valid UTF-8, so `from_utf8` succeeds and the test's `expect_err` fails. POSIX-guaranteed octal `\377\376` emits the same raw bytes on every printf. Test-only; the production non-UTF-8 rejection is unchanged.
rigel-mintaka
added a commit
that referenced
this pull request
Aug 21, 2026
`jj-vine submit <bookmark>` 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 <matt@rigel.build>
mattwilkinsonn
added a commit
that referenced
this pull request
Aug 21, 2026
…67) (#2) * fix: submit an explicitly-named bookmark regardless of author (RIG-2267) `jj-vine submit <bookmark>` 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 <matt@rigel.build> * fix(submit): cover ancestry mine()-narrowing and drop ANSI from guard error (RIG-2267) 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 <matt@rigel.build> --------- Co-authored-by: Matt Wilkinson <matt@rigel.build>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a stack containing 2 PRs:
mainThe
resolved_token_errors_on_non_utf8_outputunit test generated itsinvalid-UTF-8 bytes with
sh -c "printf '\xff\xfe'". Hex\xHHescapesare a bash / GNU-coreutils
printfextension, not POSIX — a dash/bin/sh(as on GitHububuntu-24.04) passes\xffthrough literally asvalid UTF-8, so
String::from_utf8succeeds and the test'sexpect_errfails. POSIX-guaranteed octal
\377\376emits the same rawff febyteson every
printf.Test-only; the production non-UTF-8 rejection path is unchanged. This is
the fix that turns the fork's GitHub Actions CI green after the initial
seed.