Skip to content

Fix jj large-file warnings leaking into UI output#71

Open
ArthurHeymans wants to merge 2 commits into
0WD0:mainfrom
ArthurHeymans:fix-jj-warning-output
Open

Fix jj large-file warnings leaking into UI output#71
ArthurHeymans wants to merge 2 commits into
0WD0:mainfrom
ArthurHeymans:fix-jj-warning-output

Conversation

@ArthurHeymans

@ArthurHeymans ArthurHeymans commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix jj stderr warnings leaking into Majutsu UI output when jj refuses to snapshot large untracked files, such as binary/reference files above snapshot.max-new-file-size.

The leak showed up in two places:

  • rendered buffers such as diff/status, because majutsu-jj-wash inserted jj output with stderr mixed into stdout;
  • machine-readable/completion paths, because the responsive process-file replacement did not honor explicit nil stderr destinations like (list t nil), allowing warnings to be parsed as candidates.

Changes

  • Route explicit nil stderr destinations to a throwaway stderr process in the responsive process runner, matching process-file semantics.
  • Capture stderr separately in majutsu-jj-wash.
    • Successful commands now render stdout only.
    • Failing commands still include stderr when the caller requests error output.
  • Add regression tests for discarded stderr, successful warning stderr, and preserved failure stderr.

Validation

  • Ran the majutsu-process-test.el and majutsu-jj-test.el test files together: 66/66 passed.
  • Manually verified against a jj worktree with large untracked pdf files: the snapshot warning no longer appears in washed status/diff output.

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of command errors so error output is preserved when requested and discarded when not needed.
    • Fixed temporary error-output cleanup to avoid leaving behind leftover files.
    • Prevented error text from being mixed into normal output when it should be suppressed.
  • Tests

    • Added regression tests covering successful runs, failed runs with preserved error output, and cases where error output is explicitly disabled.

Majutsu's responsive `process-file` replacement handled `(STDOUT STDERR)`
destinations differently from Emacs' `process-file`: when callers passed
`(list t nil)` to capture stdout and discard stderr, stderr was still
inherited by the main process buffer and mixed into stdout.

That made benign jj stderr warnings visible to callers that expected plain
machine-readable output, including completion readers.  In a repository with
large untracked files, jj can emit "Refused to snapshot some files" while
still exiting successfully; those warning lines were then parsed as bookmark
or revision candidates.

Route explicit nil stderr destinations to a throwaway stderr process instead
of allowing them to merge into stdout.  Keep the existing behavior for a
single stdout destination, where stderr intentionally remains mixed with
stdout.

Add a regression test for `(list t nil)` so future changes preserve the
`process-file` destination contract.
`majutsu-jj-wash` rendered successful jj commands by running jj with stdout
inserted directly into the destination buffer.  Because stderr was not kept
separate, successful commands could still prepend warning text to UI output.

This is especially visible in repositories with large untracked files.  jj
refuses to snapshot those files, exits successfully for commands such as
`status`, `diff`, and completion/listing queries, and writes a warning like:

  Warning: Refused to snapshot some files:
    file.bin: 7.1MiB; the maximum size allowed is 1.0MiB

Majutsu then treated that warning as part of the command's stdout.  The text
could appear at the top of diff/status buffers and, for machine-readable
queries, leak into completion menus such as bookmark movement.

Capture stderr separately in `majutsu-jj-wash`.  On successful commands,
render only stdout.  On failures, continue to include stderr when the caller
asked to keep error output so genuine jj errors remain visible.

Add regression coverage for both successful warning stderr and failure stderr
preservation.
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The process runner (majutsu--process-file-responsive) is updated to discard stderr without mixing it into stdout when the caller passes nil as the stderr destination. The jj wash layer (majutsu-jj-wash) gains a guaranteed err-file temp-file lifecycle via unwind-protect, conditional insertion of stderr content when keep-error is set, and explicit temp-file cleanup on return.

Changes

stderr discard and keep-error handling

Layer / File(s) Summary
Process runner stderr discard fix
majutsu-process.el, test/majutsu-process-test.el
Adds a stderr-discard flag; creates a temporary stderr buffer for both string-path and nil destinations, but restricts write-region flush to string-path destinations only. New regression test confirms stdout-only output when stderr destination is nil.
majutsu-jj-wash err-file lifecycle and keep-error insertion
majutsu-jj.el, test/majutsu-jj-test.el
Wraps majutsu-process-file call in unwind-protect and writes an empty err-file when the command does not produce one. Inserts err-file contents into the buffer on the no-output and non-zero-exit branches when keep-error is non-nil. Cleans up err-file via prog1/ignore-errors on return. Two new ERT tests cover stderr discard on success and stderr preservation on failure.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A rabbit digs tunnels for stderr to hide,
No errors shall leak to the stdout inside.
keep-error speaks up when the exit is bad,
And prog1 cleans temp files — no litter to add!
With unwind-protect guarding each burrow at night,
The wash basin gleams and the output is right. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main fix: jj large-file warnings leaking into UI output.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
majutsu-jj.el (1)

872-884: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Colorize kept stderr before returning.

These new insert-file-contents paths run after the existing ANSI pass, so KEEP-ERROR now inserts raw stderr escape sequences into the buffer. Since Majutsu runs jj with --color=always by default, failing commands can render unreadable control bytes here.

Suggested fix
       (when keep-error
-        (insert-file-contents err-file))
+        (let ((err-beg (point)))
+          (insert-file-contents err-file)
+          (when (and majutsu-process-apply-ansi-colors
+                     (> (point) err-beg))
+            (let ((ansi-color-apply-face-function
+                   #'ansi-color-apply-text-property-face))
+              (ansi-color-apply-on-region err-beg (point))))))
@@
       (when keep-error
-        (insert-file-contents err-file))
+        (let ((err-beg (point)))
+          (insert-file-contents err-file)
+          (when (and majutsu-process-apply-ansi-colors
+                     (> (point) err-beg))
+            (let ((ansi-color-apply-face-function
+                   #'ansi-color-apply-text-property-face))
+              (ansi-color-apply-on-region err-beg (point))))))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@majutsu-jj.el` around lines 872 - 884, Kept stderr is being inserted without
ANSI cleanup in the `keep-error` and failure-path branches of `majutsu-jj.el`,
so raw escape sequences can reach the buffer after the existing color-processing
step. Update the `insert-file-contents` handling in the `wash-jj-output` flow to
run the same ANSI stripping/colorization logic on `err-file` before inserting
it, especially in the `keep-error` path and the nonzero-exit failure branch. Use
the surrounding `keep-error`, `wash-anyway`, and `exit` handling in this
function to ensure all retained stderr is rendered consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@majutsu-jj.el`:
- Around line 872-884: Kept stderr is being inserted without ANSI cleanup in the
`keep-error` and failure-path branches of `majutsu-jj.el`, so raw escape
sequences can reach the buffer after the existing color-processing step. Update
the `insert-file-contents` handling in the `wash-jj-output` flow to run the same
ANSI stripping/colorization logic on `err-file` before inserting it, especially
in the `keep-error` path and the nonzero-exit failure branch. Use the
surrounding `keep-error`, `wash-anyway`, and `exit` handling in this function to
ensure all retained stderr is rendered consistently.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 27e46d86-7cc3-4601-9245-49b542445e67

📥 Commits

Reviewing files that changed from the base of the PR and between 9c93891 and 616828f.

📒 Files selected for processing (4)
  • majutsu-jj.el
  • majutsu-process.el
  • test/majutsu-jj-test.el
  • test/majutsu-process-test.el

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant