refactor: make exit command a regular, configurable command#1870
Conversation
…mented as all other commands without special treatment.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace hardcoded REPL exit checks with a pluggable ExitCommands group (configurable via ShellBuilder), centralize EOF handling in the dispatcher, and refactor tests to use shared terminal fixtures and a TestEchoCommand. ChangesPluggable Exit Command Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@shell/src/main/java/org/jline/shell/impl/ExitCommands.java`:
- Line 29: Rename the inner class currently named SetOptCommand to ExitCommand
to match its behavior; update any references/instantiations (e.g., the
constructor call that creates new SetOptCommand(...)) to use new
ExitCommand(...) and adjust the class declaration from "private static class
SetOptCommand" to "private static class ExitCommand" so the class name and all
usages reflect the exit/quit semantics.
In `@shell/src/test/java/org/jline/shell/impl/AbstractCommandDispatcherTest.java`:
- Around line 51-72: The tearDown() method closes terminalInput without checking
for null, which can throw NPE if setUp() failed; update tearDown() to guard
terminalInput with a null check before calling terminalInput.close(), similar to
how terminal and dispatcher are handled, and ensure this check occurs inside the
final try block that currently encloses terminalInput.close() and
terminalStream.close() so terminalStream remains closed only if non-null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b6e584e-47d4-43f8-bdbf-336b99b07e36
📒 Files selected for processing (7)
shell/src/main/java/org/jline/shell/Shell.javashell/src/main/java/org/jline/shell/ShellBuilder.javashell/src/main/java/org/jline/shell/impl/ExitCommands.javashell/src/test/java/org/jline/shell/impl/AbstractCommandDispatcherTest.javashell/src/test/java/org/jline/shell/impl/AbstractCommandsTest.javashell/src/test/java/org/jline/shell/impl/ExitCommandsTest.javashell/src/test/java/org/jline/shell/impl/VariableCommandsTest.java
💤 Files with no reviewable changes (1)
- shell/src/test/java/org/jline/shell/impl/AbstractCommandsTest.java
🤖 Augment PR SummarySummary: This PR makes the shell’s Changes:
Technical Notes: Exit now behaves like other commands (can be disabled, and can be overridden by earlier-registered command groups). 🤖 Was this summary useful? React with 👍 or 👎 |
| continue; | ||
| } | ||
|
|
||
| dispatcher.execute(line); |
There was a problem hiding this comment.
shell/src/main/java/org/jline/shell/Shell.java:131: Since dispatcher.execute(line) is now inside the same try, the generic catch (Exception e) will also catch exceptions from promptSupplier.get(), rightPromptSupplier.get(), or reader.readLine(...) and keep looping. That can hide configuration/terminal failures and potentially lead to an error-spam loop instead of failing fast.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
@gnodet The logic from this pull alligns with the old console module way of handling this loop. If you want this changed per this bot's comment I can do so.
…mand test cases and moved ShellBuilder exit tests to the correct class.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shell/src/test/java/org/jline/shell/impl/AbstractCommandDispatcherTest.java (1)
45-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard dispatcher cleanup when setup fails.
dispatcher.close()at Line 46 can throwNullPointerExceptionwhen@BeforeEachfails before initialization, masking the original failure.Proposed fix
`@Override` `@AfterEach` protected void tearDown() throws IOException { try { - dispatcher.close(); + if (dispatcher != null) { + dispatcher.close(); + } } finally { super.tearDown(); } }Based on learnings,
AfterEachruns even ifBeforeEachthrows, so cleanup must null-check partially initialized fields.🤖 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 `@shell/src/test/java/org/jline/shell/impl/AbstractCommandDispatcherTest.java` around lines 45 - 47, The test teardown calls dispatcher.close() unconditionally which can throw a NullPointerException if `@BeforeEach` failed; modify the AfterEach cleanup to guard the call by checking that dispatcher is not null before invoking dispatcher.close() (i.e., wrap the call with a null-check in the AfterEach method of AbstractCommandDispatcherTest) so partially-initialized state from a failed `@BeforeEach` won't mask the original failure.
🤖 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.
Inline comments:
In `@shell/src/test/java/org/jline/shell/AbstractTerminalTest.java`:
- Around line 34-41: Replace the current TerminalBuilder-based dumb terminal
construction in setUp() with a LineDisciplineTerminal-backed test fixture: stop
creating terminalInput/terminalStream and building via TerminalBuilder, instead
instantiate a LineDisciplineTerminal (use the constructor with parameters like
"test", "xterm", terminalOutput, StandardCharsets.UTF_8) and assign it to the
terminal field so the shared test fixture uses LineDisciplineTerminal for I/O in
setUp().
---
Duplicate comments:
In `@shell/src/test/java/org/jline/shell/impl/AbstractCommandDispatcherTest.java`:
- Around line 45-47: The test teardown calls dispatcher.close() unconditionally
which can throw a NullPointerException if `@BeforeEach` failed; modify the
AfterEach cleanup to guard the call by checking that dispatcher is not null
before invoking dispatcher.close() (i.e., wrap the call with a null-check in the
AfterEach method of AbstractCommandDispatcherTest) so partially-initialized
state from a failed `@BeforeEach` won't mask the original failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a9e1028-bf43-48af-aa8c-d0db4abf4010
📒 Files selected for processing (13)
shell/src/main/java/org/jline/shell/impl/DefaultCommandDispatcher.javashell/src/main/java/org/jline/shell/impl/ExitCommands.javashell/src/test/java/org/jline/shell/AbstractTerminalTest.javashell/src/test/java/org/jline/shell/ShellBuilderTest.javashell/src/test/java/org/jline/shell/impl/AbstractCommandDispatcherTest.javashell/src/test/java/org/jline/shell/impl/CommandHighlighterTest.javashell/src/test/java/org/jline/shell/impl/DefaultCommandDispatcherTest.javashell/src/test/java/org/jline/shell/impl/ExitCommandsTest.javashell/src/test/java/org/jline/shell/impl/HelpCommandsTest.javashell/src/test/java/org/jline/shell/impl/IORedirectionTest.javashell/src/test/java/org/jline/shell/impl/SignalHandlingTest.javashell/src/test/java/org/jline/shell/impl/SimpleCommandGroupTest.javashell/src/test/java/org/jline/shell/impl/TestEchoCommand.java
💤 Files with no reviewable changes (1)
- shell/src/test/java/org/jline/shell/impl/CommandHighlighterTest.java
✅ Files skipped from review due to trivial changes (1)
- shell/src/test/java/org/jline/shell/impl/SimpleCommandGroupTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- shell/src/main/java/org/jline/shell/impl/ExitCommands.java
gnodet
left a comment
There was a problem hiding this comment.
Shell.java:88 — The run() Javadoc still references the removed built-in exit/quit commands. Suggested fix:
* This method blocks until the user exits (e.g. via EOF or an exit command),
* or until {@link #stop()} is called.Claude Code on behalf of Guillaume Nodet
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@shell/src/main/java/org/jline/shell/impl/DefaultCommandDispatcher.java`:
- Around line 378-380: executePipeGroup currently treats EndOfFileException as a
generic Exception and sets session.setLastExitCode(1), causing inconsistent
exit-code behavior versus executeSingleStage; update executePipeGroup to catch
EndOfFileException separately (like the catch in executeSingleStage), call
session.setLastExitCode(0) and rethrow the EndOfFileException, so exit-code
handling is consistent across both execution contexts (refer to
executePipeGroup, executeSingleStage, EndOfFileException, and
session.setLastExitCode).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f29a5e1-5c9a-4e80-808c-00f087375534
📒 Files selected for processing (2)
shell/src/main/java/org/jline/shell/Shell.javashell/src/main/java/org/jline/shell/impl/DefaultCommandDispatcher.java
🚧 Files skipped from review as they are similar to previous changes (1)
- shell/src/main/java/org/jline/shell/Shell.java
…nd multi pipe command execution logic.
|
There are 2 things still of note for this pull:
@gnodet I would like your personal opinion on these 2 matters before a merge |
Aligning with the old console behaviour is a requirement. I'd prefer a cleaner approach, so I you can push a follow-up commit fixing both points, that would be really good imho. |
…hellException. Also fixed CommandDispatcher#trace not flushing the terminal.
|
Aligning with the old console behaviour is NOT a requirement. |
|
This is what I expected, as both points somewhat contradicted each other. I separated the try-blocks for cleaner command exception handling. |
gnodet
left a comment
There was a problem hiding this comment.
Good architectural improvement. Making the exit command a regular, configurable command group follows the framework's existing patterns and gives embedders control over shell lifecycle.
Key observations:
-
ExitShellException: Clean approach for control flow, as discussed with the maintainer. ExtendsRuntimeExceptionso it propagates naturally through the dispatcher. The optional message support is a nice touch. -
Pipe chain semantics: The
runCommandStage()refactoring correctly suppressesExitShellExceptionwhen it's the last command in a pipe chain (op == null && multi), matching bash behavior where pipe elements run in subshells. For&&,||, and;operators, the exception propagates and the shell exits — also correct. -
Test coverage: Thorough test matrix covering
exit | echo,exit && echo,exit || echo,exit ; echo, and the reverse directions. ThenoExitShellCommandtest properly validates the disable path. The sharedTestEchoCommandandAbstractTerminalTestreduce duplication across test classes. -
Builder API:
exitCommands(boolean)andexitMessage(String)are intuitive. The auto-enable behavior ofexitMessage()is documented in the javadoc.
All CI checks pass.
Fully automatic review from Claude Code on behalf of Guillaume Nodet
gnodet
left a comment
There was a problem hiding this comment.
Good architectural improvement — making the exit command a regular, configurable command group is the right direction. The refactoring is clean, the test coverage is thorough (pipe chains, &&/||/; semantics, enable/disable), and the runCommandStage() consolidation eliminates real duplication between single-stage and pipe-group execution.
A couple of minor observations for future consideration:
-
ExitShellExceptionstack trace overhead: SinceExitShellExceptionis purely a control-flow signal (never logged, never inspected), it's paying for stack trace generation on everyexit. OverridingfillInStackTrace()to returnthisis a common pattern for control-flow exceptions and would avoid that cost. -
Exit code arguments: The
ExitCommandcurrently ignoresargs, soexit 42silently discards the status code. Not blocking for this PR, but worth considering for future extensibility.
The pipe-chain semantics are correct: exit | echo recovered suppresses the exit (bash subshell behavior), while exit && echo, exit || echo, and exit ; echo all propagate. The test matrix covers both directions nicely.
Overall this is well done. No blocking issues.
Review by Claude Code on behalf of Guillaume Nodet
|
|
||
| public ExitShellException(String message) { | ||
| super(message); | ||
| } |
There was a problem hiding this comment.
Consider overriding fillInStackTrace() to avoid the cost of generating a stack trace. Since this exception is used purely for control flow (never logged or inspected), the stack trace is wasted work:
| } | |
| public ExitShellException(String message) { | |
| super(message); | |
| } | |
| @Override | |
| public synchronized Throwable fillInStackTrace() { | |
| return this; | |
| } |
This is a common pattern for control-flow exceptions. Not blocking, but a nice optimization.
Review by Claude Code on behalf of Guillaume Nodet
| } | ||
|
|
||
| @Override | ||
| public Object execute(CommandSession session, String[] args) { |
There was a problem hiding this comment.
Nit: the command ignores args, so exit 42 silently discards the exit code. Not blocking for this PR, but worth noting for future work — bash users may expect exit <code> to set the process exit status. The ExitShellException could carry an optional int exitCode field alongside the message.
Review by Claude Code on behalf of Guillaume Nodet
|
Make the exit command an optional command that can be disabled, implemented as all other commands without special treatment.
Summary by CodeRabbit
New Features
Behavior Changes
Tests