Improve GitHub Actions workflows: add permissions, consolidate build steps, and configure artifact retention #110
Conversation
|
Warning Rate limit exceeded@SauravBhowmick has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds Spotless formatting to Maven, updates GitHub Actions (permissions, make mvnw executable, artifact and test-result uploads with retention and SHA naming), applies wide code formatting, extends ShellContext with a richer public API, and refactors many commands to use ProcessBuilder, reader threads, timeouts, helper extraction, and standardized description()/usage() metadata methods. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Engine as ShellEngine
participant Cmd as Command
participant OS as SystemProcess
Note over User,Engine: ProcessBuilder-based command execution (general)
User->>Engine: enter command
Engine->>Cmd: execute(args, context)
Cmd->>Cmd: build OS-aware command list
Cmd->>OS: start ProcessBuilder (attach stdout/stderr)
par stream stdout
OS-->>Cmd: stdout lines
Cmd->>Engine: forward stdout
and stream stderr
OS-->>Cmd: stderr lines
Cmd->>Engine: forward stderr
end
Cmd->>OS: waitFor(timeout? 30s)
alt timeout
Cmd->>OS: destroy process
Cmd->>Engine: print timeout message
else completed
Cmd->>Engine: print exit/results
end
sequenceDiagram
autonumber
participant User
participant Engine as ShellEngine
participant Ping as PingCommand
participant OS as SystemProcess
Note over User,Ping: PingCommand — validate, build, stream, collect stats
User->>Engine: execute("ping ...")
Engine->>Ping: execute(args, context)
Ping->>Ping: validateHost()/resolveHost()
Ping->>Ping: buildPingCommand()
Ping->>OS: start ProcessBuilder (stream stdout)
OS-->>Ping: stdout lines
Ping->>Ping: extractTimeFromLine()/update stats
Ping->>Engine: print lines and final statistics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
github-actions
bot
requested a review
from anshumanjadiya1102
as a code owner
SauravBhowmick
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
src/main/java/com/mycmd/commands/SearchHistoryCommand.java (1)
74-96: Consider caching the lowercase conversion for minor performance improvement.The method repeatedly calls
text.toLowerCase()in the loop. For typical command history sizes this is fine, but you could optimize by converting once upfront.Optional refactor:
private String highlightTerm(String text, String term) { - // Case-insensitive highlight - int index = text.toLowerCase().indexOf(term.toLowerCase()); + // Case-insensitive highlight + String lowerText = text.toLowerCase(); + String lowerTerm = term.toLowerCase(); + int index = lowerText.indexOf(lowerTerm); if (index == -1) { return text; } StringBuilder result = new StringBuilder(); int lastIndex = 0; while (index >= 0) { result.append(text, lastIndex, index); result.append("["); result.append(text, index, index + term.length()); result.append("]"); lastIndex = index + term.length(); - index = text.toLowerCase().indexOf(term.toLowerCase(), lastIndex); + index = lowerText.indexOf(lowerTerm, lastIndex); } result.append(text.substring(lastIndex)); return result.toString(); }src/main/java/com/mycmd/commands/TelnetCommand.java (1)
38-58: Join the reader thread to prevent race conditions.The reader thread is started as a daemon but never joined. This can cause the "Disconnected." message (line 82) to print before the reader finishes outputting buffered data, resulting in lost or interleaved output.
Apply this diff to ensure clean shutdown:
reader.setDaemon(true); reader.start(); + Thread readerThread = reader; // capture reference for join // Writer loop: read stdin and send to remote try (OutputStream out = socket.getOutputStream(); OutputStreamWriter osw = new OutputStreamWriter(out); BufferedWriter bw = new BufferedWriter(osw); BufferedReader stdin = new BufferedReader(new InputStreamReader(System.in))) { String line; while ((line = stdin.readLine()) != null) { if ("exit".equalsIgnoreCase(line.trim())) break; bw.write(line); bw.write("\r\n"); // typical telnet line ending bw.flush(); } } catch (IOException ignored) { // stdin/socket error, will disconnect } - // ensure socket closes to stop reader - try { - socket.close(); - } catch (IOException ignored) { - } + // wait for reader to finish + try { + readerThread.join(1000); // wait up to 1 second + } catch (InterruptedException ignored) { + } + System.out.println("\nDisconnected.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (86)
pom.xml(3 hunks)src/main/java/com/mycmd/App.java(1 hunks)src/main/java/com/mycmd/Command.java(1 hunks)src/main/java/com/mycmd/CommandRegistry.java(1 hunks)src/main/java/com/mycmd/ConsoleShell.java(1 hunks)src/main/java/com/mycmd/ShellContext.java(1 hunks)src/main/java/com/mycmd/ShellEngine.java(1 hunks)src/main/java/com/mycmd/StringUtils.java(1 hunks)src/main/java/com/mycmd/commands/AliasCommand.java(1 hunks)src/main/java/com/mycmd/commands/ArpCommand.java(1 hunks)src/main/java/com/mycmd/commands/AssocCommand.java(1 hunks)src/main/java/com/mycmd/commands/AttribCommand.java(1 hunks)src/main/java/com/mycmd/commands/CdCommand.java(1 hunks)src/main/java/com/mycmd/commands/ChkdskCommand.java(1 hunks)src/main/java/com/mycmd/commands/ChoiceCommand.java(1 hunks)src/main/java/com/mycmd/commands/ClearHistoryCommand.java(1 hunks)src/main/java/com/mycmd/commands/ClipCommand.java(1 hunks)src/main/java/com/mycmd/commands/ClsCommand.java(1 hunks)src/main/java/com/mycmd/commands/ColorCommand.java(1 hunks)src/main/java/com/mycmd/commands/CompactCommand.java(1 hunks)src/main/java/com/mycmd/commands/CopyCommand.java(1 hunks)src/main/java/com/mycmd/commands/DateCommand.java(1 hunks)src/main/java/com/mycmd/commands/DelCommand.java(1 hunks)src/main/java/com/mycmd/commands/DirCommand.java(1 hunks)src/main/java/com/mycmd/commands/DriverqueryCommand.java(1 hunks)src/main/java/com/mycmd/commands/EchoCommand.java(1 hunks)src/main/java/com/mycmd/commands/ExitCommand.java(1 hunks)src/main/java/com/mycmd/commands/FcCommand.java(1 hunks)src/main/java/com/mycmd/commands/FindCommand.java(1 hunks)src/main/java/com/mycmd/commands/FindstrCommand.java(1 hunks)src/main/java/com/mycmd/commands/ForfilesCommand.java(1 hunks)src/main/java/com/mycmd/commands/FsutilCommand.java(1 hunks)src/main/java/com/mycmd/commands/FtypeCommand.java(1 hunks)src/main/java/com/mycmd/commands/GetmacCommand.java(1 hunks)src/main/java/com/mycmd/commands/HelpCommand.java(1 hunks)src/main/java/com/mycmd/commands/HistoryCommand.java(1 hunks)src/main/java/com/mycmd/commands/HostnameCommand.java(1 hunks)src/main/java/com/mycmd/commands/InteractiveSearchCommand.java(1 hunks)src/main/java/com/mycmd/commands/IpConfig.java(1 hunks)src/main/java/com/mycmd/commands/LabelCommand.java(1 hunks)src/main/java/com/mycmd/commands/LsCommand.java(1 hunks)src/main/java/com/mycmd/commands/MkdirCommand.java(1 hunks)src/main/java/com/mycmd/commands/MoreCommand.java(1 hunks)src/main/java/com/mycmd/commands/MoveCommand.java(1 hunks)src/main/java/com/mycmd/commands/MsgCommand.java(1 hunks)src/main/java/com/mycmd/commands/NetCommand.java(1 hunks)src/main/java/com/mycmd/commands/NetshCommand.java(1 hunks)src/main/java/com/mycmd/commands/NetstatCommand.java(1 hunks)src/main/java/com/mycmd/commands/NslookupCommand.java(1 hunks)src/main/java/com/mycmd/commands/PathCommand.java(1 hunks)src/main/java/com/mycmd/commands/PauseCommand.java(1 hunks)src/main/java/com/mycmd/commands/PingCommand.java(1 hunks)src/main/java/com/mycmd/commands/PwdCommand.java(1 hunks)src/main/java/com/mycmd/commands/RemCommand.java(1 hunks)src/main/java/com/mycmd/commands/RenameCommand.java(1 hunks)src/main/java/com/mycmd/commands/ReplaceCommand.java(1 hunks)src/main/java/com/mycmd/commands/RmdirCommand.java(1 hunks)src/main/java/com/mycmd/commands/RobocopyCommand.java(1 hunks)src/main/java/com/mycmd/commands/RouteCommand.java(1 hunks)src/main/java/com/mycmd/commands/SearchHistoryCommand.java(1 hunks)src/main/java/com/mycmd/commands/SetCommand.java(1 hunks)src/main/java/com/mycmd/commands/SfcCommand.java(1 hunks)src/main/java/com/mycmd/commands/ShutdownCommand.java(1 hunks)src/main/java/com/mycmd/commands/SortCommand.java(1 hunks)src/main/java/com/mycmd/commands/StartCommand.java(1 hunks)src/main/java/com/mycmd/commands/SysteminfoCommand.java(1 hunks)src/main/java/com/mycmd/commands/TaskkillCommand.java(1 hunks)src/main/java/com/mycmd/commands/TasklistCommand.java(1 hunks)src/main/java/com/mycmd/commands/TelnetCommand.java(1 hunks)src/main/java/com/mycmd/commands/TimeCommand.java(1 hunks)src/main/java/com/mycmd/commands/TimeoutCommand.java(1 hunks)src/main/java/com/mycmd/commands/TitleCommand.java(1 hunks)src/main/java/com/mycmd/commands/TouchCommand.java(1 hunks)src/main/java/com/mycmd/commands/TracertCommand.java(1 hunks)src/main/java/com/mycmd/commands/TreeCommand.java(1 hunks)src/main/java/com/mycmd/commands/TypeCommand.java(1 hunks)src/main/java/com/mycmd/commands/UnaliasCommand.java(1 hunks)src/main/java/com/mycmd/commands/UptimeCommand.java(1 hunks)src/main/java/com/mycmd/commands/VerifyCommand.java(1 hunks)src/main/java/com/mycmd/commands/VersionCommand.java(1 hunks)src/main/java/com/mycmd/commands/VolCommand.java(1 hunks)src/main/java/com/mycmd/commands/WhoamiCommand.java(1 hunks)src/main/java/com/mycmd/commands/WmicCommand.java(1 hunks)src/main/java/com/mycmd/commands/XcopyCommand.java(1 hunks)src/main/java/com/mycmd/gui/MainApp.java(1 hunks)src/main/java/com/mycmd/gui/TerminalController.java(1 hunks)
✅ Files skipped from review due to trivial changes (49)
- src/main/java/com/mycmd/commands/SysteminfoCommand.java
- src/main/java/com/mycmd/commands/CompactCommand.java
- src/main/java/com/mycmd/commands/NslookupCommand.java
- src/main/java/com/mycmd/commands/NetshCommand.java
- src/main/java/com/mycmd/commands/TimeCommand.java
- src/main/java/com/mycmd/commands/WhoamiCommand.java
- src/main/java/com/mycmd/ShellEngine.java
- src/main/java/com/mycmd/StringUtils.java
- src/main/java/com/mycmd/commands/ClearHistoryCommand.java
- src/main/java/com/mycmd/commands/InteractiveSearchCommand.java
- src/main/java/com/mycmd/commands/DateCommand.java
- src/main/java/com/mycmd/commands/PwdCommand.java
- src/main/java/com/mycmd/commands/EchoCommand.java
- src/main/java/com/mycmd/commands/IpConfig.java
- src/main/java/com/mycmd/commands/SfcCommand.java
- src/main/java/com/mycmd/commands/ArpCommand.java
- src/main/java/com/mycmd/commands/WmicCommand.java
- src/main/java/com/mycmd/commands/TracertCommand.java
- src/main/java/com/mycmd/commands/ChkdskCommand.java
- src/main/java/com/mycmd/commands/PauseCommand.java
- src/main/java/com/mycmd/commands/MoveCommand.java
- src/main/java/com/mycmd/commands/RmdirCommand.java
- src/main/java/com/mycmd/commands/VersionCommand.java
- src/main/java/com/mycmd/commands/TaskkillCommand.java
- src/main/java/com/mycmd/commands/ExitCommand.java
- src/main/java/com/mycmd/commands/DirCommand.java
- src/main/java/com/mycmd/commands/DelCommand.java
- src/main/java/com/mycmd/commands/HostnameCommand.java
- src/main/java/com/mycmd/commands/TitleCommand.java
- src/main/java/com/mycmd/commands/HistoryCommand.java
- src/main/java/com/mycmd/commands/GetmacCommand.java
- src/main/java/com/mycmd/App.java
- src/main/java/com/mycmd/commands/MsgCommand.java
- src/main/java/com/mycmd/commands/StartCommand.java
- src/main/java/com/mycmd/Command.java
- src/main/java/com/mycmd/commands/LsCommand.java
- src/main/java/com/mycmd/commands/ClsCommand.java
- src/main/java/com/mycmd/commands/VerifyCommand.java
- src/main/java/com/mycmd/commands/NetCommand.java
- src/main/java/com/mycmd/commands/ForfilesCommand.java
- src/main/java/com/mycmd/commands/ChoiceCommand.java
- src/main/java/com/mycmd/commands/XcopyCommand.java
- src/main/java/com/mycmd/commands/UptimeCommand.java
- src/main/java/com/mycmd/commands/TypeCommand.java
- src/main/java/com/mycmd/commands/CdCommand.java
- src/main/java/com/mycmd/commands/MoreCommand.java
- src/main/java/com/mycmd/commands/TreeCommand.java
- src/main/java/com/mycmd/commands/ReplaceCommand.java
- src/main/java/com/mycmd/commands/ColorCommand.java
🧰 Additional context used
🪛 ast-grep (0.39.9)
src/main/java/com/mycmd/commands/TelnetCommand.java
[info] 33-33: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(host, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (24)
src/main/java/com/mycmd/commands/MkdirCommand.java (2)
19-33: LGTM! Clean formatting applied.The execute method logic is correct and the formatting improvements enhance readability. The error handling appropriately covers all scenarios (missing args, existing directory, creation failure).
35-43: LGTM! Metadata methods are well-defined.The description and usage methods provide clear information about the command's purpose and syntax. The repositioning aligns with the broader standardization effort across command classes.
src/main/java/com/mycmd/commands/SearchHistoryCommand.java (3)
15-49: LGTM! Clean refactoring with improved flow.The execute method is well-structured with clear separation of concerns:
- Defensive empty-history check with early return
- Explicit handling of no-args vs. search cases
- Multi-word search support via
String.join- Helpful user feedback for both success and no-match scenarios
54-68: LGTM! Well-implemented display logic.The dynamic line-number width calculation ensures proper alignment regardless of history size, and the conditional highlighting is cleanly integrated.
98-110: LGTM! Clear documentation methods.The
description()andusage()methods provide helpful user-facing documentation with practical examples. This aligns well with the broader standardization pattern across the codebase mentioned in the AI summary.src/main/java/com/mycmd/commands/TelnetCommand.java (3)
16-32: LGTM! Clean argument validation and parsing.The early validation, default port handling, and error messages are clear and correct.
34-36: Good use of try-with-resources, but note security consideration.The socket lifecycle management with try-with-resources is correct. However, be aware that telnet transmits data in cleartext, making it vulnerable to interception. Consider documenting this security limitation in the class javadoc or usage guidance for users.
Based on static analysis hints.
88-96: LGTM! Clean implementation of Command interface methods.The
description()andusage()methods provide clear, concise information for users.src/main/java/com/mycmd/commands/LabelCommand.java (1)
16-31: Read-only messaging stays consistent.Line 17 keeps stressing the simulation-only nature of the command so users can’t mistake it for a destructive action. Looks good.
src/main/java/com/mycmd/commands/RobocopyCommand.java (1)
18-65: Argument quoting keeps Windows invocations safe.Line 37 wraps each argument before handing it to
cmd.exe, preventing spacing from breaking the robocopy call. Nicely done.src/main/java/com/mycmd/commands/ClipCommand.java (1)
19-34: Early OS guard prevents unsupported flows.Line 19 exits immediately on non-Windows hosts so the simulated guidance only shows where it’s relevant. 👍
src/main/java/com/mycmd/commands/VolCommand.java (1)
42-49: Helper extraction keeps serial logic tidy.Lines 42-49 isolate the pseudo-serial computation, making
execute()easier to read without altering behavior.src/main/java/com/mycmd/commands/TouchCommand.java (1)
23-35: Usage fallback keeps UX clear.Lines 23-26 short-circuit missing arguments through
usage(), keeping the experience aligned with other commands.src/main/java/com/mycmd/commands/SortCommand.java (1)
19-87: LGTM! Clean implementation with proper resource management.The execute method correctly uses try-with-resources for file reading, and the newly added description() and usage() methods align with the API consistency improvements across the codebase.
src/main/java/com/mycmd/commands/AssocCommand.java (1)
46-54: LGTM! Metadata methods added.The description() and usage() methods correctly provide command metadata, consistent with the API standardization effort.
src/main/java/com/mycmd/commands/AliasCommand.java (1)
9-67: LGTM! Clean alias management implementation.The refactored execute method properly handles all alias scenarios (list, create with =, create with multiple args), and the newly added description() and usage() methods provide clear command metadata.
src/main/java/com/mycmd/commands/CopyCommand.java (1)
20-53: LGTM! Excellent use of try-with-resources.The refactored execute method uses try-with-resources for proper stream management, and the newly added description() and usage() methods provide clear command metadata. The implementation is clean and follows best practices.
src/main/java/com/mycmd/commands/DriverqueryCommand.java (1)
49-57: LGTM! Metadata methods added.The description() and usage() methods correctly provide command metadata, consistent with the API standardization effort.
src/main/java/com/mycmd/commands/ShutdownCommand.java (1)
85-93: LGTM! Metadata methods added.The description() and usage() methods correctly provide command metadata.
src/main/java/com/mycmd/commands/RenameCommand.java (1)
13-44: LGTM! Clean rename implementation.The refactored execute method properly validates arguments and uses
Files.move()for the rename operation. The newly added description() and usage() methods provide clear command metadata.src/main/java/com/mycmd/commands/FcCommand.java (4)
31-79: LGTM! Clean refactoring with proper validation.The flag parsing, file-path resolution, and dispatch logic are well-structured. Extracting the comparison methods improves readability, and the control flow correctly handles both relative and absolute paths.
81-108: LGTM! Excellent resource management and efficient comparison.The binary comparison uses try-with-resources correctly, optimizes with a size check, and reports the first byte-level difference. The logic is sound and resource-safe.
110-152: LGTM! Comprehensive text comparison with proper options support.The text comparison correctly handles case sensitivity and line numbering, uses try-with-resources for proper cleanup, and reports all differences line-by-line. The logic for detecting length mismatches and comparing line content is sound.
154-162: LGTM! Good API metadata additions.The
description()andusage()methods provide clear command metadata and align with the broader pattern of standardizing command metadata exposure across the codebase.
|
@SauravBhowmick, Please once go through the problems given by Coderabbitai |
So many errors man... |
|
Yaa, Sure, I will also review and resolve the comments and issues. Happy |
Refactor command execution to use ProcessBuilder for better error handling and timeout management.
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/mycmd/commands/AssocCommand.java (2)
68-87: Consider checking the process exit code.While the timeout and cleanup logic are well-implemented, the code doesn't verify whether the
assoccommand succeeded. Consider checkingprocess.exitValue()after a successfulwaitFor()to detect command failures.You could add after line 81:
if (!finished) { process.destroyForcibly(); System.out.println("Command timed out."); + } else { + int exitCode = process.exitValue(); + if (exitCode != 0) { + System.err.println("Command failed with exit code: " + exitCode); + } }
89-91: Consider narrowing the exception catch.Catching
Exceptionis very broad and can mask programming errors likeNullPointerExceptionorIllegalArgumentException. Sinceexecute()declaresIOException, consider catching onlyIOExceptionor specific exception types.Apply this diff:
- } catch (Exception e) { + } catch (IOException e) { System.out.println("Error executing assoc: " + e.getMessage()); }Note: The
InterruptedExceptionis already handled specifically in the timeout block, so this change is safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/mycmd/commands/AssocCommand.java(2 hunks)
🔇 Additional comments (2)
src/main/java/com/mycmd/commands/AssocCommand.java (2)
29-40: Excellent fix for the command injection vulnerability!The switch from
Runtime.exec()with string concatenation toProcessBuilderwith aList<String>properly prevents command injection. Arguments are now passed safely without shell interpretation.
42-58: Good use of error gobbler pattern.The asynchronous error stream reading prevents potential deadlocks when both stdout and stderr buffers fill up. Using a daemon thread and try-with-resources ensures proper resource management.
Refactor DriverqueryCommand to use ProcessBuilder for command execution and add error handling with a dedicated thread for error stream.
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
Removed unnecessary command execution for cmd.exe.
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/mycmd/ShellContext.java(1 hunks)src/main/java/com/mycmd/commands/MoveCommand.java(1 hunks)src/main/java/com/mycmd/commands/ShutdownCommand.java(2 hunks)src/main/java/com/mycmd/commands/TelnetCommand.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.39.9)
src/main/java/com/mycmd/commands/TelnetCommand.java
[info] 34-34: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(host, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (11)
src/main/java/com/mycmd/commands/MoveCommand.java (2)
23-44: LGTM!The execute method logic is correct, with proper path resolution, existence checking, and error handling. The formatting improvements enhance readability.
46-54: LGTM!The new
description()andusage()methods provide clear metadata for the command, consistent with the API surface expansion pattern across other commands in this PR.src/main/java/com/mycmd/commands/ShutdownCommand.java (4)
38-46: LGTM - Scanner handling is correct given the System.in constraint.The confirmation prompt correctly avoids closing
System.inby not using try-with-resources, as identified in previous reviews. While the Scanner is not explicitly closed, this is an acceptable tradeoff to maintain interactive shell functionality.
48-64: LGTM!Process setup correctly handles multiple operating systems with appropriate command construction. The explicit handling of unsupported OS cases (lines 62-63) prevents cryptic errors.
67-106: LGTM - Excellent process and thread management.The concurrent reader threads (lines 67-93) correctly prevent deadlock by consuming stdout and stderr simultaneously. The timeout logic (lines 98-103) properly:
- Waits for process completion with a 30-second limit
- Forcibly destroys the process if timeout occurs
- Waits again to ensure cleanup
- Joins reader threads after process handling
All resource leaks and timing issues from previous reviews have been successfully resolved.
114-122: LGTM!The
description()andusage()methods provide clear command metadata, consistent with the API standardization across commands in this PR.src/main/java/com/mycmd/commands/TelnetCommand.java (5)
34-59: LGTM - Reader thread correctly implemented.The reader thread scope issue from previous reviews has been resolved by declaring
readerat line 34. The thread correctly:
- Uses try-with-resources for socket streams (lines 43-45)
- Is marked as daemon to prevent blocking JVM shutdown (line 58)
- Handles socket closure gracefully with IOException catch (line 53)
62-75: LGTM - stdin handling correctly preserves System.in.The stdin
BufferedReader(line 62) is intentionally created outside try-with-resources to avoid closingSystem.in, as identified in previous reviews. The socket output streams (lines 63-65) are properly managed with try-with-resources, and the writer loop correctly handles user input and exit conditions.
80-86: LGTM - Proper thread cleanup.The reader thread join (lines 80-86) correctly waits up to 1 second for the reader to finish and properly handles
InterruptedExceptionby restoring the interrupt status. The null check protects against initialization failures.
35-35: Note: Unencrypted connection is expected for telnet protocol.The static analysis warning about cleartext transmission (CWE-319) at line 35 is expected behavior for the telnet protocol, which does not support encryption. Users requiring secure connections should use SSH instead. Consider documenting this limitation in the class javadoc.
89-97: LGTM!The
description()andusage()methods provide clear command metadata, consistent with the API standardization pattern across this PR.
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/formatter.yml (1)
30-31: Add error handling for Spotless apply failure.If
mvn spotless:applyfails (e.g., due to malformed code or a plugin issue), the workflow continues silently to the commit step, potentially masking problems.Consider failing fast:
- name: Apply Spotless formatting - run: mvn spotless:apply + run: | + mvn spotless:apply || { echo "❌ Spotless formatting failed."; exit 1; }Or use the
-fflag:- name: Apply Spotless formatting - run: mvn spotless:apply + run: mvn -f pom.xml spotless:applyThis ensures formatter errors don't go unnoticed.
src/main/java/com/mycmd/ShellContext.java (2)
4-4: Consider explicit imports over wildcard.The wildcard import
import java.util.*;can obscure which classes are used and may cause naming conflicts. Explicit imports (Scanner, HashMap, ArrayList, Map, List) improve clarity.Apply this diff to use explicit imports:
-import java.util.*; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Scanner;
116-153: Code duplication in alias methods.The methods
getAliases()(line 116) andgetAllAliases()(line 151) have identical implementations. Consider consolidating or making one delegate to the other for maintainability.Apply this diff to eliminate duplication:
/** Get all aliases (for compatibility with existing code). */ public Map<String, String> getAliases() { - return new HashMap<>(aliases); + return getAllAliases(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/formatter.yml(2 hunks)src/main/java/com/mycmd/ShellContext.java(1 hunks)
🔇 Additional comments (9)
.github/workflows/formatter.yml (2)
7-8: Permissions addition is correct.Adding
contents: writeis necessary for the workflow to commit and push changes. Approved.
3-5: Confirm that post-merge-only formatting is intentional.The
pull_requesttrigger was intentionally removed. The workflow now runs only onpushtomain, meaning formatting checks and auto-fixes happen after code merges, not during PRs. This is a deliberate architectural choice (visible in the dedicated refactor commit), but requires confirmation it aligns with your CI strategy.Current behavior: PRs skip formatting validation entirely; formatter.yml runs exclusively post-merge on main. If your project should validate or auto-fix formatting during PR review, restore the
pull_requesttrigger:on: push: branches: [main] + pull_request: + branches: [main]If post-merge-only formatting is intentional, no change needed—but document this decision in a workflow comment or CONTRIBUTING guide for future maintainers.
src/main/java/com/mycmd/ShellContext.java (7)
44-49: Good: Defensive null check prevents uninitialized access.The
IllegalStateExceptioningetScanner()provides clear failure feedback if the scanner hasn't been initialized, which is better than allowing NPE.
67-73: Good: Path resolution logic is correct.The
resolvePathmethod properly handles both absolute and relative paths.
85-92: Good: Defensive copying prevents external mutation.Both
getAllEnvVars()and the compatibility methodgetEnvVars()return defensive copies, protecting internal state.
100-107: Good: Defensive copying and compatibility support.The
getHistory()method returns a defensive copy, and the legacygetCommandHistory()method maintains backward compatibility.
157-163: LGTM: Simple accessors for uptime tracking.The start time getter and setter are straightforward and support the uptime command functionality.
81-83: No null key/value issues found—current callers guarantee valid inputs.Verification confirms
setEnvVarcannot receive null keys or values from existing callers:
- SetCommand: The
arg.contains("=")check guaranteessplit("=", 2)produces exactly 2 elements; bothparts[0].trim()andparts[1].trim()return non-null strings.- PathCommand:
String.join(" ", args)always returns a String (never null); the key is always the literal"PATH".No fixes or defensive validation are required—the method is used safely across the codebase.
11-25: Thread-safety concern is valid but mitigated in current usage.The shared
ShellContextinstance reused across commands could theoretically cause race conditions. However, verification shows the threaded commands (TelnetCommand, TimeoutCommand, ShutdownCommand) create daemon threads that are properly joined before the command returns—none of them outlive the command execution or directly access the mutable fields (currentDir,scanner,startTime, or collection contents) from their threads. The current implementation avoids actual data races but relies on this sequencing, making it fragile.This design should be reviewed if:
- Commands are modified to access context fields from long-lived threads
- Background job execution is added in the future
- New threaded commands are created that access context state
No immediate fixes required, but recommend documenting this limitation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
@SauravBhowmick, Now I think that We can merge clearly |
Now, it's a good news... 😮💨 .... after so much efforts 🤓 |
2 participants
Vibe coding done.
Summary by CodeRabbit