Feature: Refactor CLI #68
Conversation
* print mount URI * terminate on EOT or ETX character
This reverts commit aecf18c.
WalkthroughThe changes in this pull request encompass significant updates to the Cryptomator CLI application, affecting various components including documentation, build scripts, and core functionality. The New Java classes have been introduced, such as The Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (16)
src/main/java/module-info.java (1)
6-13: LGTM: Dependencies are appropriate for the CLI refactoring.The required dependencies align well with the PR objectives:
org.cryptomator.cryptofsandorg.cryptomator.frontend.fusefor core functionality and FUSE integrationinfo.picoclifor modern CLI argument parsingorg.slf4j,ch.qos.logback.core, andch.qos.logback.classicfor loggingorg.fusesource.jansifor enhanced terminal supportorg.cryptomator.integrations.apifor various integration pointsConsider grouping related dependencies together (e.g., all logging-related dependencies) for better readability.
.github/workflows/build.yml (3)
33-39: LGTM: Improved release processThe new release step using
softprops/action-gh-release@v2is a good improvement. It simplifies the release process and includes useful features like automatic release note generation.A few observations:
- The conditional execution for tagged commits is correct.
- Using
CRYPTOBOT_RELEASE_TOKENis a good security practice.- Generating release notes automatically is efficient.
- Setting releases as drafts allows for review before publishing.
Consider adding a
nameto this step for better readability in the GitHub Actions UI:- name: Create GitHub Release if: startsWith(github.ref, 'refs/tags/') uses: softprops/action-gh-release@v2 with: token: ${{ secrets.CRYPTOBOT_RELEASE_TOKEN }} generate_release_notes: true draft: true🧰 Tools
🪛 yamllint
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
19-19: Consider addressing potential word splitting issueWhile not directly related to the current changes, the static analysis tool flagged a potential issue with word splitting in the shell command on this line. Consider updating it to use double quotes:
run: mvn versions:set --file ./pom.xml -DnewVersion="${GITHUB_REF##*/}" # use shell parameter expansion to strip of 'refs/tags'This change can prevent potential issues with special characters in the version string.
🧰 Tools
🪛 actionlint
19-19: shellcheck reported issue in this script: SC2086:info:1:48: Double quote to prevent globbing and word splitting
(shellcheck)
39-39: Add newline at end of fileThe static analysis tool flagged that there's no newline character at the end of the file. While this doesn't affect functionality, it's a good practice to end files with a newline. Please add an empty line at the end of the file.
🧰 Tools
🪛 yamllint
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
README.md (2)
12-12: Approved: Updated Java version requirementThe change from JDK 17 to JDK/JRE 22 is correctly reflected in the README. This aligns with the PR objectives.
Consider highlighting this significant version change in the PR description or release notes to ensure users are aware of the new requirement.
14-18: LGTM: Updated usage instructionsThe new usage instructions accurately reflect the changes described in the PR objectives, including:
- The use of
--enable-native-access="ALL-UNNAMED".- Specifying password input from stdin.
- Using the FUSE mounter.
- Specifying a mount point.
Consider adding a brief comment explaining the purpose of the
--enable-native-access="ALL-UNNAMED"flag for users who might not be familiar with it.src/main/java/org/cryptomator/cli/LogbackConfigurator.java (3)
54-55: Avoid Hardcoding Logger Configuration in CodeDisabling logging for
org.cryptomator.frontend.fuse.locksdirectly in the code reduces flexibility. Consider externalizing this configuration to a Logback configuration file or allowing users to adjust logger levels via command-line options or environment variables.
35-35: Consider Externalizing the Log PatternThe log pattern
[%thread] %highlight(%-5level) %cyan(%logger{15}) - %msg %nis hardcoded. For greater flexibility and maintainability, consider externalizing this pattern to a configuration file or allowing it to be set via a command-line argument or environment variable.
62-66: Enhance Documentation forsetLogLevelsMethodThe
setLogLevelsmethod could benefit from more detailed documentation. Consider specifying how the method should be used, any potential side effects, and examples of thelogLevelsparameter.src/main/java/org/cryptomator/cli/PasswordSource.java (3)
18-28: Maintain consistent terminology between option names and field namesThe command-line options use "password" in their names (e.g.,
--password:stdin), while the corresponding field names use "passphrase" (e.g.,passphraseStdin). For consistency and to avoid confusion, consider using the same term in both the option names and the field names.
46-47: UseSystem.errfor password promptsUsing
System.out.println("Enter the password:");may interfere with output redirection or scripts that rely on standard output. It's a best practice to useSystem.errfor prompts to ensure they are displayed to the user regardless of output redirection.Apply this diff to use
System.err:- System.out.println("Enter the password:"); + System.err.println("Enter the password:");
55-63: Simplify passphrase retrieval from environment variableIn
readPassphraseFromEnvironment(), manually copying characters from the string to a char array is unnecessary. You can simplify the code by using thetoCharArray()method.Apply this diff to simplify the code:
var tmp = System.getenv(passphraseEnvironmentVariable); if (tmp == null) { throw new ReadingEnvironmentVariableFailedException("Environment variable " + passphraseEnvironmentVariable + " is not defined."); } - char[] result = new char[tmp.length()]; - tmp.getChars(0, tmp.length(), result, 0); - return new Passphrase(result); + return new Passphrase(tmp.toCharArray());src/main/java/org/cryptomator/cli/CryptomatorCli.java (1)
69-69: Clarify Log Message: Possible TypoThe log message on line 69 reads "User verbose logging", which might be a typo. It seems intended to indicate that verbose logging is enabled. Consider changing it to "Using verbose logging" or "Verbose logging enabled" for clarity.
Apply this diff to correct the log message:
- LOG.debug("User verbose logging"); + LOG.debug("Verbose logging enabled");src/main/java/org/cryptomator/cli/MountOptions.java (1)
27-28: Ensure consistency in exception message formattingIn the
setMountServicemethod, you useString.format()for formatting the error message, whereas in themountmethod, you use.formatted(). For consistency and compatibility with older Java versions, consider usingString.format()in both places.Also applies to: 108-108
mvnw.cmd (1)
127-127: Clarify error message for unsupported checksum validation with MVNDIn line 127, the error message can be rephrased for improved clarity to help users understand the issue and the required action.
Apply this diff to enhance the error message:
- Write-Error "Checksum validation is not supported for maven-mvnd. `nPlease disable validation by removing 'distributionSha256Sum' from your maven-wrapper.properties." + Write-Error "Checksum validation is not supported for maven-mvnd. Please remove 'distributionSha256Sum' from your maven-wrapper.properties to disable checksum validation."mvnw (1)
74-78: Provide Guidance When Java Commands Are MissingIf
javaorjavacare not found in thePATHandJAVA_HOMEis not set, the script exits with an error. To assist users in resolving this issue, consider providing guidance on installing Java or settingJAVA_HOMEin the error message.Update the error message as follows:
-echo "The java/javac command does not exist in PATH nor is JAVA_HOME set, so mvnw cannot run." >&2 +echo "Java is required to run mvnw. Please install Java or set the JAVA_HOME environment variable to the location of your Java installation." >&2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
- .github/workflows/build.yml (1 hunks)
- .gitignore (1 hunks)
- .mvn/wrapper/maven-wrapper.properties (1 hunks)
- Dockerfile (0 hunks)
- README.md (1 hunks)
- mvnw (1 hunks)
- mvnw.cmd (1 hunks)
- pom.xml (4 hunks)
- src/main/java/module-info.java (1 hunks)
- src/main/java/org/cryptomator/cli/Args.java (0 hunks)
- src/main/java/org/cryptomator/cli/CryptomatorCli.java (1 hunks)
- src/main/java/org/cryptomator/cli/LogbackConfigurator.java (1 hunks)
- src/main/java/org/cryptomator/cli/MountOptions.java (1 hunks)
- src/main/java/org/cryptomator/cli/PasswordSource.java (1 hunks)
- src/main/java/org/cryptomator/cli/Version.java (0 hunks)
- src/main/java/org/cryptomator/cli/frontend/FuseMount.java (0 hunks)
- src/main/java/org/cryptomator/cli/frontend/WebDav.java (0 hunks)
- src/main/java/org/cryptomator/cli/pwd/PasswordFromFileStrategy.java (0 hunks)
- src/main/java/org/cryptomator/cli/pwd/PasswordFromPropertyStrategy.java (0 hunks)
- src/main/java/org/cryptomator/cli/pwd/PasswordFromStdInputStrategy.java (0 hunks)
- src/main/java/org/cryptomator/cli/pwd/PasswordStrategy.java (0 hunks)
- src/main/resources/META-INF/services/ch.qos.logback.classic.spi.Configurator (1 hunks)
- src/main/resources/logback.xml (0 hunks)
💤 Files with no reviewable changes (10)
- Dockerfile
- src/main/java/org/cryptomator/cli/Args.java
- src/main/java/org/cryptomator/cli/Version.java
- src/main/java/org/cryptomator/cli/frontend/FuseMount.java
- src/main/java/org/cryptomator/cli/frontend/WebDav.java
- src/main/java/org/cryptomator/cli/pwd/PasswordFromFileStrategy.java
- src/main/java/org/cryptomator/cli/pwd/PasswordFromPropertyStrategy.java
- src/main/java/org/cryptomator/cli/pwd/PasswordFromStdInputStrategy.java
- src/main/java/org/cryptomator/cli/pwd/PasswordStrategy.java
- src/main/resources/logback.xml
✅ Files skipped from review due to trivial changes (1)
- .mvn/wrapper/maven-wrapper.properties
🧰 Additional context used
🪛 actionlint
.github/workflows/build.yml
19-19: shellcheck reported issue in this script: SC2086:info:1:48: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/build.yml
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
🪛 LanguageTool
README.md
[uncategorized] ~29-~29: Possible missing comma found.
Context: ...tion Depending on the chosen mounter, you the vault is automatically integrated i...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (31)
.gitignore (1)
21-24: LGTM! These additions enhance Git ignore patterns.The changes to the
.gitignorefile are appropriate and beneficial:
- Adding
*.imlensures that IntelliJ IDEA module files are ignored, which is consistent with the existing patterns for IDE-specific files.- Including
dependency-reduced-pom.xmlis a good practice as it's a build artifact generated by the Maven Shade plugin and should not be version controlled.These additions will help keep the repository clean and prevent unnecessary files from being tracked.
src/main/java/module-info.java (4)
1-4: LGTM: Imports and module declaration look good.The import statements are appropriate for the
Configuratorinterface and its implementation. The module is declared as open, which allows for deep reflection. This is likely necessary for the CLI functionality, especially considering the use of libraries like PicoCLI.
5-5: LGTM: Appropriate use of theConfiguratorinterface.The
usesstatement forConfiguratoris consistent with the import statement and the laterprovidesclause. This setup allows the module to utilize implementations ofConfigurator, whether provided by this module or others.
1-16: Overall, the module-info.java file is well-structured and aligns with the PR objectives.The file correctly sets up the
org.cryptomator.climodule with appropriate dependencies, uses, and provides clauses. It supports the transition to PicoCLI for argument parsing, includes necessary cryptomator and FUSE dependencies, and sets up a custom logging configuration. This structure should facilitate the creation of a compact executable for vault operations as described in the PR objectives.
15-15: LGTM: Appropriate provision ofLogbackConfigurator.The
providesstatement is consistent with the import statements and theusesclause. It indicates that this module is providing a customLogbackConfiguratorimplementation for the Logback logging framework, allowing for a tailored logging setup specific to the CLI application.To ensure the
LogbackConfiguratoris properly implemented, please run the following script:.github/workflows/build.yml (4)
13-14: LGTM: Action versions updatedThe updates to the latest versions of
actions/checkoutandactions/setup-javaare good improvements. This ensures you're using the most recent features and security updates for these actions.
25-25: LGTM: Updated output methodThe change to use
$GITHUB_OUTPUTfor setting the project version is a good update. This follows the current best practices for GitHub Actions and replaces the deprecated::set-outputcommand.
29-29: LGTM: Artifact upload action updatedUpdating the
actions/upload-artifactaction to v4 is a good improvement. This keeps the workflow using the latest features and security updates for this action.
16-17: Verify compatibility with Java 22The update to Java 22 and the use of the 'temurin' distribution are good choices. This aligns with the PR objective of moving away from long-term support versions of OpenJDK.
Please ensure that all project dependencies and code are compatible with Java 22. Run the following script to check for any potential compatibility issues:
README.md (3)
6-6: LGTM: Updated project descriptionThe new description accurately reflects the updated functionality of the CLI, aligning with the PR objectives of creating a compact executable that unlocks and mounts a single vault.
22-25: LGTM: Added help command instructionsThe addition of instructions for viewing all available options is helpful for users. The command is consistent with the usage example, including the
--enable-native-access="ALL-UNNAMED"flag.
Line range hint
1-31: Overall: README updates accurately reflect PR objectivesThe changes made to the README file effectively communicate the updated functionality and usage of the Cryptomator CLI, aligning well with the PR objectives. Key points:
- The project description now emphasizes the "one mount, one process" principle.
- Java version requirements have been updated.
- Usage instructions have been revised to include new command-line options.
- A new section on filesystem integration has been added, explaining mounter options.
These updates provide users with clear and comprehensive information about the CLI's new features and requirements.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: Possible missing comma found.
Context: ...tion Depending on the chosen mounter, you the vault is automatically integrated i...(AI_HYDRA_LEO_MISSING_COMMA)
pom.xml (8)
5-5: Significant version update to 1.0.0-SNAPSHOTThe project version has been updated from 0.6.0-SNAPSHOT to 1.0.0-SNAPSHOT, indicating a major release. This aligns with the substantial refactoring described in the PR objectives.
Could you provide more information about the versioning strategy and what this 1.0.0 release signifies in terms of project maturity or feature completeness?
53-59: New developer added to the projectA new developer, Armin Schrenk, has been added to the developers section. This is good for proper attribution and project management.
Line range hint
1-199: Overall assessment of pom.xml changesThe changes made to the pom.xml file represent a significant upgrade to the project's build configuration and technology stack. Key improvements include:
- Updating to JDK 22 and introducing Java Platform Module System (JPMS) support.
- Switching to picocli for modern argument parsing.
- Improving the build process with maven-shade-plugin for creating an uber JAR.
- Preparing for potential native compilation.
- Enhancing FUSE integration with native access configurations.
These changes align well with the PR objectives of refactoring the CLI project and should result in a more modular, maintainable, and feature-rich application. The transition to a 1.0.0 version also suggests that the project is reaching a significant milestone in terms of stability and feature completeness.
To ensure that all these changes work together seamlessly, it's recommended to perform a full build and test cycle, including running the application with various command-line arguments to verify the new functionality.
111-125: Maven compiler plugin configuration updateThe maven-compiler-plugin configuration has been updated to:
- Use the new jdk.version property for the release setting.
- Add annotation processor paths for picocli-codegen.
These changes are necessary to support the JDK version update and the switch to picocli for argument parsing. The addition of picocli's annotation processor will enable compile-time processing for command-line parsing.
To ensure that the picocli annotation processing is working correctly, please run a clean compile and check for any warnings or errors:
mvn clean compileReview the output to confirm that picocli's annotation processing is successful and that no issues are reported.
129-169: New maven-jar-plugin and maven-shade-plugin configurations
The maven-jar-plugin configuration has been added to set up the main class in the manifest. This is good for creating executable JARs.
The maven-shade-plugin configuration has been added to create a shaded (uber) JAR. This is useful for distribution as it packages all dependencies into a single JAR file.
The shade plugin configuration includes several transformers to properly merge service files. This is crucial for maintaining the project's functionality when creating the uber JAR.
These changes improve the build process and make it easier to distribute and run the application.
To ensure that the shaded JAR is created correctly and is executable, please run the following commands after building:
mvn clean package java -jar target/cli-1.0.0-SNAPSHOT-shaded.jar --helpVerify that the JAR file is created successfully and that it runs without any class loading issues when executing the --help command.
170-194: Updated exec-maven-plugin configurationThe exec-maven-plugin configuration has been significantly updated:
- The execution goal has changed from 'java' to 'exec', allowing for more flexible execution options.
- New arguments have been added for module path and native access, indicating a move towards using the Java Platform Module System (JPMS) and native libraries.
- The configuration now includes specific arguments for the CLI application, including the use of FUSE mounting.
These changes align with the project's move towards modularity and the integration of native libraries for FUSE support, as mentioned in the PR objectives.
To ensure that the exec-maven-plugin configuration works as expected, please run:
mvn exec:execVerify that the application starts correctly with the specified arguments and that the FUSE mounting functionality works as intended. If there are any issues with native access or module resolution, they should become apparent during this test.
13-34: JDK version update and new build properties
The JDK version has been updated to 22, which is a significant change. Ensure that all dependencies and code are compatible with this version.
New properties have been added for native image options and the main class. This suggests preparation for native compilation. Could you provide more details about the plans for native compilation and how it fits into the project roadmap?
The addition of new properties for dependency versions and Maven plugin versions improves version management and build configuration. This is a good practice for maintaining the project.
To ensure all dependencies are compatible with JDK 22, please run the following command and review the output:
This will help identify any potential compatibility issues with the new JDK version.
80-82: Dependency updates and additions
The commons-cli dependency has been replaced with picocli. This aligns with the PR objectives of transitioning to a more modern and flexible argument parsing solution.
A new dependency on slf4j-api has been added. This suggests a move towards a more flexible logging setup, which is a good practice for maintainability.
The jansi dependency has been added, which likely improves console output handling, potentially enhancing the CLI user experience.
These changes appear to improve the project's capabilities and maintainability.
To ensure that all new dependencies are properly utilized and that there are no conflicts, please run the following command:
Review the output to confirm that all dependencies are resolved correctly and there are no version conflicts.
Also applies to: 86-90, 101-105
src/main/java/org/cryptomator/cli/LogbackConfigurator.java (3)
31-60: Ensure Thread Safety When Modifying Logger ContextThe
configuremethod modifies theLoggerContextand its loggers. If this configurator can be invoked in a multi-threaded environment, ensure that these modifications are thread-safe to prevent concurrent modification issues.
47-51: Double-Check Logger Additivity SettingSetting
logger.setAdditive(false);means that log events will not be passed to parent loggers. Ensure that this is the intended behavior and that it doesn't interfere with the overall logging strategy of the application.
58-58:⚠️ Potential issueVerify the Necessity of AtomicReference in
INSTANCE.compareAndSetUsing
INSTANCE.compareAndSet(null, this);suggests that thread safety is a concern. Verify whether multiple threads could initialize this configurator. If not, consider assigning the instance directly:- INSTANCE.compareAndSet(null, this); + INSTANCE = this;src/main/java/org/cryptomator/cli/CryptomatorCli.java (5)
24-28: Good Use of picocli Annotations andCallableInterfaceThe class is correctly annotated with
@Command, and implementingCallable<Integer>provides a clean way to handle command execution and return exit codes. This enhances the modularity and readability of the CLI application.
49-57: Validate and SetmaxCleartextNameLengthCorrectlyThe
setMaxCleartextNameLengthmethod properly validates the input to ensure it's a positive integer. Throwing aParameterExceptionwith a detailed message provides clear feedback to the user in case of invalid input.
102-110: Proper Handling of Masterkey LoadingThe
loadMasterkeymethod correctly reads the passphrase and loads the master key using the providedMasterkeyFileAccess. The use of try-with-resources ensures that sensitive data inpassphraseContaineris properly managed.
117-122: Efficient Reading of Vault ConfigurationThe
readConfigFromStoragemethod efficiently reads the vault configuration without verifying its integrity, as intended. UsingFiles.readStringwithStandardCharsets.US_ASCIIis appropriate for reading the configuration file.
124-129: StreamlinedmainMethod with picocli ExecutionThe
mainmethod leverages picocli'sCommandLineto parse arguments and execute the command, simplifying the entry point of the application. SettingsetPosixClusteredShortOptionsAllowed(false)enhances compatibility across different systems.src/main/java/org/cryptomator/cli/MountOptions.java (1)
67-67: Address the TODO forREAD_ONLYcapabilityThere is a TODO comment indicating that handling for the
READ_ONLYcapability is pending. Implementing this feature would enhance the functionality by allowing read-only mounts when supported by the mount service.Would you like assistance in implementing the
READ_ONLYcapability? I can help provide a code snippet or open a GitHub issue to track this task.mvnw.cmd (1)
76-78: Verify the correctness of$MVNW_REPO_PATTERNassignmentsIn lines 76-78, please verify that the assignment of
$MVNW_REPO_PATTERNis accurate. When$USE_MVNDis$true,$MVNW_REPO_PATTERNis set to"/org/apache/maven/", but it might be expected to be"/maven/mvnd/". This could affect the construction of the$distributionUrland lead to incorrect repository paths.Consider reviewing the repository paths to ensure that the correct pattern is used for MVND and Maven distributions.
mvnw (1)
202-214: Sanitize Variables in Generated Java Source to Prevent InjectionWhen generating
Downloader.javaat lines 202-214, thedistributionUrlis included directly in the source code. To prevent potential code injection, ensure thatdistributionUrlis sanitized or encoded appropriately.[security]
Verify that
distributionUrldoes not contain characters that could break the Java code or introduce malicious code. If necessary, sanitize the URL before including it.
src/main/resources/META-INF/services/ch.qos.logback.classic.spi.Configurator
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (22)
src/main/java/module-info.java (1)
1-15: Consider documenting module dependencies.To improve maintainability, consider adding documentation comments (using
/**style) before the module declaration to explain:
- The purpose of each required module
- Why the module needs to be 'open'
- The role of the LogbackConfigurator service provider
This will help future maintainers understand the module structure and dependencies.
src/main/java/org/cryptomator/cli/LoggingMixin.java (1)
14-17: Move implementation details to class-level documentation.The comment about multiple instances explains the class's design and would be more appropriate in the class-level JavaDoc.
Consider moving and expanding the implementation note:
+/** + * Mixin for handling verbose logging configuration across commands. + * <p> + * Note: Each subcommand that mixes in the LoggingMixin has its own instance + * of this class. To maintain consistent state, the verbosity setting is stored + * centrally in the top-level command's LoggingMixin instance. + */ public class LoggingMixin { // ... existing code ... /** * Sets a verbose logging leven on the LoggingMixin of the top-level command. * @param isVerbose boolean flag to activate verbose mode */ @Option(names = {"-v", "--verbose"}, description = { "Activate verbose mode"}) public void setVerbose(boolean isVerbose) { - // Each subcommand that mixes in the LoggingMixin has its own instance - // of this class, so there may be many LoggingMixin instances. - // We want to store the verbosity value in a single, central place, - // so we find the top-level command, - // and store the verbosity level on our top-level command's LoggingMixin. // ... rest of the method ... } }Also applies to: 21-25
src/main/java/org/cryptomator/cli/ListMounters.java (2)
9-17: Fix typo and enhance command description.There's a typo in the description text and the explanation could be more precise.
Apply this diff to improve the documentation:
header = "Lists available mounters", - description = "Prints a list of available mounters to STDIN. A mounter is is the object to mount/integrate the unlocked vault into the local filesystem. In the GUI app, mounter is named \"volume type\".", + description = "Prints a list of available mounters to STDIN. A mounter is an implementation that handles the integration of an unlocked vault into the local filesystem using different mounting strategies (e.g., FUSE, WebDAV, etc.). In the GUI app, mounter is referred to as \"volume type\".",
19-20: Consider adding a short option name and aligning terminology.The option could be more user-friendly with a short form and consistent terminology.
Apply this diff to enhance the option:
- @CommandLine.Option(names = {"--withDisplayName"}, description = "Prints also the display name of each mounter, as used in the GUI app.") + @CommandLine.Option(names = {"-d", "--display-name"}, description = "Include the volume type name as shown in the GUI")src/main/java/org/cryptomator/cli/CryptomatorCli.java (2)
11-15: Enhance command description to clarify the "one mount, one process" principle.While the current description is clear, consider adding more detail to align with the PR objectives:
- description = "Unlocks a cryptomator vault and mounts it into the system.", + description = "Unlocks a single cryptomator vault and mounts it into the system. Following the 'one mount, one process' principle, each vault requires a separate CLI instance.",
34-41: LGTM with a minor suggestion for the error message.The verbose mode activation is well-implemented with proper error handling and confirmation logging.
Consider making the error message more actionable:
- throw new IllegalStateException("Logging is not configured."); + throw new IllegalStateException("Logging system is not properly initialized. Ensure LogbackConfigurator is set up.");build_mac.sh (3)
22-37: Document the rationale for included Java modules.The script includes specific Java modules but lacks documentation explaining why these modules are necessary. This makes future maintenance challenging.
Add comments explaining module requirements:
echo "Creating JRE with jlink..." +"# Required modules: +# - java.base: Core Java functionality +# - java.compiler: Required for runtime compilation +# - java.naming: Required for JNDI operations +# - java.xml: Required for XML processing" "$JAVA_HOME/bin/jlink" \ --verbose \ --output target/runtime \
53-57: Document the purpose of Java runtime options.The script sets several Java options without explaining their purpose, making it difficult to maintain or adjust these settings.
Add comments explaining each option:
+ # Enable preview features for JDK 22 --java-options "--enable-preview" \ + # Enable native access for FUSE integration --java-options "--enable-native-access=org.cryptomator.jfuse.mac" \ + # Set thread stack size to 5MB --java-options "-Xss5m" \ + # Limit heap size to 256MB --java-options "-Xmx256m" \ + # Ensure UTF-8 encoding for file operations --java-options "-Dfile.encoding=utf-8" \
1-2: Add FUSE library verification.The PR objectives mention that FUSE-related mounters require additional libraries. The script should verify these dependencies.
Add this check at the beginning of the script:
#!/bin/bash +# Check for macFUSE installation +if ! pkgutil --pkg-info com.github.osxfuse.pkg.MacFUSE &> /dev/null; then + echo "Warning: macFUSE is not installed. FUSE-based mounting will not be available." + echo "Please install macFUSE from https://osxfuse.github.io/" +fi + echo "Building cryptomator cli..."build_win.ps1 (2)
3-6: Simplify and strengthen the Maven verification.The current Maven verification can be improved:
- The foreach loop is unnecessary for a single command
- The check doesn't verify a minimum Maven version
Consider this alternative implementation:
-$commands = 'mvn' -foreach ($cmd in $commands) { - Invoke-Expression -Command "${cmd} --version" -ErrorAction Stop -} +# Extract and verify Maven version +$mvnOutput = mvn --version 2>&1 +if ($LASTEXITCODE -ne 0) { + throw "Maven is not installed or not in PATH" +} +if ($mvnOutput -notmatch 'Apache Maven (\d+\.\d+\.\d+)') { + throw "Unable to determine Maven version" +} +$mvnVersion = $Matches[1] +if ([version]$mvnVersion -lt [version]"3.8.0") { + throw "Maven version 3.8.0 or higher is required. Found: $mvnVersion" +}
56-58: Enhance error reporting for binary creation.The error handling could provide more detailed diagnostics.
Consider this enhancement:
-if ( ($LASTEXITCODE -ne 0) -or (-not (Test-Path ./target/cryptomator-cli))) { - throw "Binary creation with jpackage failed with exit code $LASTEXITCODE." +if ($LASTEXITCODE -ne 0) { + throw "Binary creation with jpackage failed with exit code $LASTEXITCODE" +} elseif (-not (Test-Path ./target/cryptomator-cli)) { + throw "Binary creation completed but output directory ./target/cryptomator-cli not found. Check jpackage output for details." }build_linux.sh (1)
22-38: Consider making module list configurable.The hard-coded module list might need maintenance as requirements change. Consider extracting it to a configuration variable.
+# Required Java modules +JAVA_MODULES="java.base,java.compiler,java.naming,java.xml" + echo "Creating JRE with jlink..." "$JAVA_HOME/bin/jlink" \ --verbose \ --output target/runtime \ --module-path "${JAVA_HOME}/jmods" \ - --add-modules java.base,java.compiler,java.naming,java.xml \ + --add-modules "${JAVA_MODULES}" \ --strip-native-commands \ --no-header-files \ --no-man-pages \ --strip-debug \ --compress zip-0README.md (4)
28-31: Fix code block formatting.There's a formatting issue in the help command example.
-cryptomator-cli --help` +cryptomator-cli --help
45-49: Add example output for list-mounters.While the command is documented, showing example output would help users understand what to expect.
Consider adding:
Example output: ```shell Available mounters: - org.cryptomator.frontend.fuse.mount.LinuxFuseMountProvider - org.cryptomator.frontend.webdav.mount.FallbackMounter--- `98-99`: **Enhance cleanup documentation.** The cleanup section could be more detailed about the implications of failed unmounts. Consider adding: 1. Common scenarios that might cause unmount failures (e.g., open files, running processes) 2. How to identify processes holding open handles 3. Steps to ensure clean unmount <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [formatting] ~98-~98: Consider inserting a comma after an introductory phrase for better readability. Context: ...thout executing possible cleanup tasks. In that case the message "GRACEFUL UNMOUNT FAILED" i... (IN_THAT_CASE_COMMA) </details> </details> --- `102-103`: **Add language specifier to code block.** The code block is missing a language specifier. ```diff -``` +```shell fusermount -u /path/to/former/mountpoint<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 102-102: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>.github/workflows/build-binary.yml (3)</summary><blockquote> `1-19`: **Enhance workflow configuration maintainability** Consider these improvements for better maintainability and reliability: 1. Move Java version to repository variables for easier updates 2. Add concurrency controls to prevent parallel runs from interfering ```diff name: Build java app image +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + on: release: types: [published] workflow_dispatch: inputs: sem-version: description: 'Version' required: false env: JAVA_DIST: 'zulu' - JAVA_VERSION: '22.0.2+9' + JAVA_VERSION: ${{ vars.JAVA_VERSION }}
45-47: Improve job naming for clarityThe job name could be more descriptive to reflect its multi-platform nature.
build-binary: - name: Build java app image + name: Build multi-platform java app images
113-119: Consider externalizing JVM configurationThe Java options configuration could be enhanced by:
- Moving memory settings to environment variables
- Grouping related options together
+env: + JVM_HEAP_SIZE: "256m" + JVM_STACK_SIZE: "5m" + --java-options "-Dorg.cryptomator.cli.version=${{ needs.prepare.outputs.semVerStr }}" + # Preview and native access options --java-options "--enable-preview" --java-options "--enable-native-access=${{ matrix.native-access-lib }}" - --java-options "-Xss5m" - --java-options "-Xmx256m" + # Memory settings + --java-options "-Xss${JVM_STACK_SIZE}" + --java-options "-Xmx${JVM_HEAP_SIZE}" + # Encoding settings --java-options '-Dfile.encoding="utf-8"'src/main/java/org/cryptomator/cli/Unlock.java (1)
74-74: Consider caching SecureRandom instance.Creating a new SecureRandom instance for each call can be expensive. Consider initializing it once at class level for better performance.
-private SecureRandom csprng = null; +private static final SecureRandom CSPRNG = SecureRandom.getInstanceStrong(); -public Integer call() throws Exception { - csprng = SecureRandom.getInstanceStrong(); +public Integer call() throws Exception {pom.xml (2)
22-23: Document native optimization levelsThe native optimization property lacks documentation about available options and their implications. Consider adding comments explaining the available optimization levels and their trade-offs.
206-225: Document why license plugin is commented outThe license-maven-plugin configuration is commented out without explanation. If it's temporary, add a TODO comment explaining why it's disabled and when it should be re-enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
.github/workflows/build-binary.yml(1 hunks)README.md(2 hunks)build_linux.sh(1 hunks)build_mac.sh(1 hunks)build_win.ps1(1 hunks)pom.xml(4 hunks)src/main/java/module-info.java(1 hunks)src/main/java/org/cryptomator/cli/CryptomatorCli.java(1 hunks)src/main/java/org/cryptomator/cli/ListMounters.java(1 hunks)src/main/java/org/cryptomator/cli/LoggingMixin.java(1 hunks)src/main/java/org/cryptomator/cli/MountSetup.java(1 hunks)src/main/java/org/cryptomator/cli/Unlock.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/cli/MountSetup.java
🧰 Additional context used
📓 Learnings (1)
.github/workflows/build-binary.yml (2)
Learnt from: infeo
PR: cryptomator/cli#68
File: .github/workflows/build-binary.yml:46-65
Timestamp: 2024-10-24T14:37:51.863Z
Learning: In `.github/workflows/build-binary.yml`, the project prefers to specify exact job configurations using the `include` list in the matrix strategy and avoids combining arrays to prevent unintended combinations.
Learnt from: infeo
PR: cryptomator/cli#68
File: .github/workflows/build-binary.yml:83-96
Timestamp: 2024-10-24T14:35:18.017Z
Learning: The `--no-fallback` option does not exist for `jlink`; it is an option for `jpackage` instead. Avoid suggesting `--no-fallback` with `jlink` commands in future reviews.
🪛 actionlint
.github/workflows/build-binary.yml
41-41: shellcheck reported issue in this script: SC2006:style:1:13: Use $(...) notation instead of legacy backticks ...
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:2:33: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 LanguageTool
README.md
[locale-violation] ~16-~16: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...-cli.zip unzip cryptomator-cli.zip ``` Afterwards, you can directly run Cryptomator-CLI: ...
(AFTERWARDS_US)
[uncategorized] ~35-~35: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e filesystem, cryptomator-cli relies on third party libraries which must be installed separ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~35-~35: Possible missing comma found.
Context: ..., cryptomator-cli relies on third party libraries which must be installed separately. The...
(AI_HYDRA_LEO_MISSING_COMMA)
[formatting] ~58-~58: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...bDAV option. > WebDAV is not the default, because it has a low performance and might have...
(COMMA_BEFORE_BECAUSE)
[formatting] ~98-~98: Consider inserting a comma after an introductory phrase for better readability.
Context: ...thout executing possible cleanup tasks. In that case the message "GRACEFUL UNMOUNT FAILED" i...
(IN_THAT_CASE_COMMA)
🪛 Markdownlint
README.md
102-102: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (17)
src/main/java/module-info.java (3)
1-3: LGTM! Imports are clean and necessary.
The imports are correctly specified for the service provider configuration.
14-14: LGTM! Service provider declaration is correct.
The LogbackConfigurator service implementation is properly declared.
4-12: Verify the necessity of all required modules and consider security implications.
The module is declared as 'open' which allows unrestricted reflective access at runtime. While this is necessary for picocli's annotation processing, it has security implications.
Also, let's verify that all required modules are actually used in the codebase.
✅ Verification successful
Let me gather more information to verify the actual usage of these modules in the code.
All required modules are necessary and properly used
The verification shows that all modules are actively used in the codebase:
org.cryptomator.frontend.fuse: Used for FUSE mounting support and logging configurationorg.cryptomator.integrations.api: Used extensively for mount services and integration loadingorg.fusesource.jansi: While direct usage is not visible in the code, it's required by picocli for terminal coloring support
The 'open' module declaration is justified as it's required for picocli's annotation processing to work, which is evident from the extensive use of picocli annotations throughout the codebase (e.g., @CommandLine.Option, @ArgGroup).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of required modules in the codebase
echo "Checking usage of org.cryptomator.frontend.fuse..."
rg -l "org\.cryptomator\.frontend\.fuse" --type java
echo "Checking usage of org.fusesource.jansi..."
rg -l "org\.fusesource\.jansi" --type java
echo "Checking usage of org.cryptomator.integrations.api..."
rg -l "org\.cryptomator\.integrations\.api" --type java
# Note: Not checking core dependencies like picocli, slf4j, and logback
# as they are fundamental to the application's functionality
Length of output: 630
Script:
#!/bin/bash
# Check for actual usage patterns of these modules
echo "Checking frontend.fuse usage context..."
rg "org\.cryptomator\.frontend\.fuse" --type java -C 3
echo -e "\nChecking jansi usage context..."
rg "org\.fusesource\.jansi" --type java -C 3
echo -e "\nChecking integrations.api usage context..."
rg "org\.cryptomator\.integrations\.api" --type java -C 3
# Also check for any FUSE, Jansi, or integration-related class usage
echo -e "\nChecking for FUSE-related usage..."
rg "Fuse|Mount" --type java -C 3
echo -e "\nChecking for Jansi-related usage..."
rg "Ansi|Terminal" --type java -C 3
echo -e "\nChecking for Integration-related usage..."
rg "Integration" --type java -C 3
Length of output: 17635
src/main/java/org/cryptomator/cli/LoggingMixin.java (1)
12-12: Previous comment about encapsulating isVerbose is still valid.
src/main/java/org/cryptomator/cli/ListMounters.java (1)
1-8: LGTM! Clean and minimal imports.
The imports are well-organized and contain only the necessary dependencies.
src/main/java/org/cryptomator/cli/CryptomatorCli.java (5)
5-10: LGTM: Clean import organization for picocli integration.
The imports are well-organized and properly reflect the transition to the picocli framework as mentioned in the PR objectives.
18-22: LGTM: Proper logger initialization and version handling.
The static initialization ensures version information is always available, and the logger follows best practices using SLF4J.
24-25: Skipping comment as it's covered by past review.
27-32: LGTM: Clean execution strategy implementation.
The execution strategy properly handles verbose mode activation and follows picocli best practices with good separation of concerns.
44-51: LGTM: Well-structured main method with appropriate signature.
The varargs signature (String... args) is actually preferable here as it:
- Provides more flexibility in argument handling
- Is fully compatible with the standard
String[] argssignature - Aligns well with picocli's command-line parsing
The method properly configures the CommandLine instance and handles exit codes as expected for a CLI application.
build_win.ps1 (1)
17-31: Verify JRE module completeness and optimize settings.
The JRE creation might need adjustments:
- The module list seems minimal and might miss required modules for CLI functionality
- Using compression level 0 trades size for potentially faster startup
Consider these improvements:
+# Clean existing runtime
+if (Test-Path ./target/runtime) {
+ Remove-Item -Recurse -Force ./target/runtime
+}
+
& $env:JAVA_HOME/bin/jlink `
--verbose `
--output target/runtime `
--module-path "${env:JAVA_HOME}/jmods" `
- --add-modules java.base,java.compiler,java.naming,java.xml `
+ --add-modules java.base,java.compiler,java.naming,java.xml,java.logging,java.security.jgss `
--strip-native-commands `
--no-header-files `
--no-man-pages `
--strip-debug `
- --compress zip-0
+ --compress zip-6README.md (2)
6-6: LGTM: Clear purpose statement.
The introduction clearly states the single-vault focus, aligning with the PR objective of "one mount, one process" principle.
35-39: LGTM: Clear third-party requirements.
The filesystem integration requirements are well documented with specific links for each OS.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~35-~35: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e filesystem, cryptomator-cli relies on third party libraries which must be installed separ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~35-~35: Possible missing comma found.
Context: ..., cryptomator-cli relies on third party libraries which must be installed separately. The...
(AI_HYDRA_LEO_MISSING_COMMA)
src/main/java/org/cryptomator/cli/Unlock.java (2)
1-36: LGTM! Well-structured command configuration.
The command configuration is clear, well-documented, and follows picocli best practices with proper help text formatting.
39-39:
Review security implications of empty PEPPER.
An empty PEPPER array (new byte[0]) might reduce the security of the key derivation function. Consider using a non-empty pepper value to enhance security against rainbow table attacks.
pom.xml (2)
13-13: Consider using an LTS JDK version
Setting JDK version to 22 might unnecessarily limit the user base. Consider using JDK 17 (LTS) unless there are specific JDK 22 features required.
161-172: 🛠️ Refactor suggestion
Document security implications of native access and avoid hardcoded paths
- Native access flags are enabled without security documentation
- Test run configuration contains hardcoded paths (T:\vaultFormat8, X:\)
Consider:
- Adding security documentation for native access requirements
- Making the paths configurable via properties:
- <argument>--mountPoint=X:\\</argument>
- <argument>T:\\vaultFormat8</argument>
+ <argument>--mountPoint=${test.mount.point}</argument>
+ <argument>${test.vault.path}</argument>Add corresponding properties:
<properties>
<test.mount.point>X:\\</test.mount.point>
<test.vault.path>T:\\vaultFormat8</test.vault.path>
</properties>| static VaultConfig.UnverifiedVaultConfig readConfigFromStorage(Path vaultPath) throws IOException { | ||
| Path configPath = vaultPath.resolve(CONFIG_FILE_NAME); | ||
| LOG.debug("Reading vault config from file {}.", configPath); | ||
| String token = Files.readString(configPath, StandardCharsets.US_ASCII); | ||
| return VaultConfig.decode(token); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation in readConfigFromStorage.
The method should validate that:
- The vault path exists and is a directory
- The config file exists and is readable
- The config file is not empty
static VaultConfig.UnverifiedVaultConfig readConfigFromStorage(Path vaultPath) throws IOException {
+ if (!Files.isDirectory(vaultPath)) {
+ throw new IOException("Vault path does not exist or is not a directory: " + vaultPath);
+ }
Path configPath = vaultPath.resolve(CONFIG_FILE_NAME);
+ if (!Files.isReadable(configPath)) {
+ throw new IOException("Config file does not exist or is not readable: " + configPath);
+ }
LOG.debug("Reading vault config from file {}.", configPath);
String token = Files.readString(configPath, StandardCharsets.US_ASCII);
+ if (token.isEmpty()) {
+ throw new IOException("Config file is empty: " + configPath);
+ }
return VaultConfig.decode(token);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static VaultConfig.UnverifiedVaultConfig readConfigFromStorage(Path vaultPath) throws IOException { | |
| Path configPath = vaultPath.resolve(CONFIG_FILE_NAME); | |
| LOG.debug("Reading vault config from file {}.", configPath); | |
| String token = Files.readString(configPath, StandardCharsets.US_ASCII); | |
| return VaultConfig.decode(token); | |
| } | |
| static VaultConfig.UnverifiedVaultConfig readConfigFromStorage(Path vaultPath) throws IOException { | |
| if (!Files.isDirectory(vaultPath)) { | |
| throw new IOException("Vault path does not exist or is not a directory: " + vaultPath); | |
| } | |
| Path configPath = vaultPath.resolve(CONFIG_FILE_NAME); | |
| if (!Files.isReadable(configPath)) { | |
| throw new IOException("Config file does not exist or is not readable: " + configPath); | |
| } | |
| LOG.debug("Reading vault config from file {}.", configPath); | |
| String token = Files.readString(configPath, StandardCharsets.US_ASCII); | |
| if (token.isEmpty()) { | |
| throw new IOException("Config file is empty: " + configPath); | |
| } | |
| return VaultConfig.decode(token); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
build_linux.sh (1)
62-85: LGTM! Consider documenting memory settings.The jpackage configuration is well-structured. Consider adding a comment explaining the chosen memory settings (-Xss5m and -Xmx256m) to help with future maintenance.
.github/workflows/build-binary.yml (2)
56-59: Enhance self-hosted runner requirementsConsider adding explicit JDK version requirement to self-hosted runner labels to ensure proper execution environment.
- - os: [self-hosted, Linux, ARM64] + - os: [self-hosted, Linux, ARM64, jdk-22]
119-124: Consider externalizing JVM configurationMove JVM memory settings to environment variables for better maintainability.
+env: + JVM_MAX_HEAP: "256m" + JVM_STACK_SIZE: "5m" + --java-options "--enable-preview" --java-options "--enable-native-access=${{ matrix.native-access-lib }}" - --java-options "-Xss5m" - --java-options "-Xmx256m" + --java-options "-Xss${JVM_STACK_SIZE}" + --java-options "-Xmx${JVM_MAX_HEAP}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/build-binary.yml(1 hunks)build_linux.sh(1 hunks)build_mac.sh(1 hunks)build_win.ps1(1 hunks)src/main/java/org/cryptomator/cli/Unlock.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- build_win.ps1
- src/main/java/org/cryptomator/cli/Unlock.java
🧰 Additional context used
📓 Learnings (3)
.github/workflows/build-binary.yml (1)
Learnt from: infeo
PR: cryptomator/cli#68
File: .github/workflows/build-binary.yml:83-96
Timestamp: 2024-11-12T05:57:38.010Z
Learning: The `--no-fallback` option does not exist for `jlink`; it is an option for `jpackage` instead. Avoid suggesting `--no-fallback` with `jlink` commands in future reviews.
build_linux.sh (2)
Learnt from: infeo
PR: cryptomator/cli#68
File: build_linux.sh:70-73
Timestamp: 2024-11-12T05:57:38.010Z
Learning: The build scripts (`build_linux.sh`, `build_mac.sh`, `build_win.ps1`) are for local testing purposes and should remain simple without additional validation checks.
Learnt from: infeo
PR: cryptomator/cli#68
File: build_linux.sh:17-21
Timestamp: 2024-11-12T05:57:38.010Z
Learning: The build scripts (`build_linux.sh`, `build_mac.sh`, and `build_win.ps1`) are intended for local testing purposes and do not require additional error handling or directory checks to avoid bloating the script.
build_mac.sh (1)
Learnt from: infeo
PR: cryptomator/cli#68
File: build_linux.sh:70-73
Timestamp: 2024-11-12T05:57:38.010Z
Learning: The build scripts (`build_linux.sh`, `build_mac.sh`, `build_win.ps1`) are for local testing purposes and should remain simple without additional validation checks.
🪛 actionlint
.github/workflows/build-binary.yml
134-134: shellcheck reported issue in this script: SC1001:info:2:129: This \c will be a regular 'c' in this context
(shellcheck)
🔇 Additional comments (8)
build_mac.sh (4)
5-23: LGTM! Well-implemented version checks.
The initial checks are comprehensive and properly validate the build environment using Maven properties for version requirements.
25-28: LGTM! Build steps are clear and concise.
The Maven build and file operations are straightforward, which aligns with the local testing purpose of this script.
61-62: Note about preview features and native access.
The script enables:
- Preview features (
--enable-preview) - Native access for macOS FUSE (
--enable-native-access=org.cryptomator.jfuse.mac)
This aligns with the PR's mention of FUSE-related mounters requiring additional libraries.
47-70: Verify memory settings for typical vault operations.
The script sets conservative memory limits:
- Stack size: 5MB per thread
- Max heap: 256MB
- These might be restrictive for operations on large vaults or multiple concurrent operations.
#!/bin/bash
# Description: Search for memory-intensive operations in the codebase
# Look for potential memory-intensive operations
echo "Searching for operations that might require significant memory..."
rg "new (byte|char|int|long)\[\]" -g "*.java"
# Look for concurrent operations that might create multiple threads
echo "Searching for concurrent operations..."
ast-grep --pattern 'new Thread($$$)' --lang java
ast-grep --pattern 'ExecutorService' --lang javabuild_linux.sh (3)
1-16: LGTM! Good error handling and dependency checks.
The script starts with appropriate error handling and essential dependency checks for Maven and JAVA_HOME.
26-30: LGTM! Straightforward build process.
The Maven build and artifact handling is appropriately simple for a local build script.
48-60: LGTM! Good architecture-specific FUSE support handling.
The script properly handles different Linux architectures for FUSE support with clear warning messages, aligning with the PR objectives regarding FUSE-related mounters.
.github/workflows/build-binary.yml (1)
20-44: LGTM! Well-structured version determination logic
The version determination logic is well-implemented with proper error handling and clear output formatting.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
src/main/java/org/cryptomator/cli/PasswordSource.java (4)
18-18: Consider documenting the rationale for MAX_PASSPHRASE_FILE_SIZEWhile 5KB is a reasonable limit for a password file, it would be helpful to document why this specific limit was chosen.
- private static final int MAX_PASSPHRASE_FILE_SIZE = 5_000; //5KB + /** + * Maximum allowed size for password files. Limited to 5KB to prevent + * memory exhaustion attacks while being large enough for any reasonable + * password/key file. + */ + private static final int MAX_PASSPHRASE_FILE_SIZE = 5_000; // 5KB
31-31: Remove commented code referenceThe comment
//readPassphraseFromStdin()appears to be leftover from development and should be removed.- return new Passphrase(passphraseStdin); //readPassphraseFromStdin(); + return new Passphrase(passphraseStdin);
37-37: Use consistent exception typeFor consistency with other methods in this class, consider throwing
PasswordSourceExceptioninstead ofIllegalStateException.- throw new IllegalStateException("Passphrase source not specified, but required."); + throw new PasswordSourceException("Passphrase source not specified, but required.");
40-49: Clear sensitive data from temporary variableThe temporary string
tmpcontaining the password should be cleared if possible, although this is challenging with Java strings due to their immutable nature.Consider marking the variable as final to prevent accidental reuse:
- var tmp = System.getenv(passphraseEnvironmentVariable); + final var tmp = System.getenv(passphraseEnvironmentVariable);pom.xml (3)
23-24: Document native optimization settingsThe native optimization flag
-Obis set for development builds, but lacks documentation about:
- Available optimization levels
- Impact on build time and runtime performance
- Recommended settings for production builds
180-199: Re-enable license plugin for third-party attributionThe license plugin is commented out. Consider re-enabling it to:
- Generate proper attribution for third-party dependencies
- Ensure compliance with license requirements
- Maintain transparency about used libraries
153-176: Document modularity requirementsThe dependency plugin configuration separates JARs into modular and non-modular paths. Consider adding documentation about:
- Why certain groups need to stay on classpath
- Future plans for modularization
- Impact on native image building
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pom.xml(5 hunks)src/main/java/org/cryptomator/cli/PasswordSource.java(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/main/java/org/cryptomator/cli/PasswordSource.java (2)
Learnt from: infeo
PR: cryptomator/cli#68
File: src/main/java/org/cryptomator/cli/PasswordSource.java:70-72
Timestamp: 2024-11-18T13:11:56.138Z
Learning: In `src/main/java/org/cryptomator/cli/PasswordSource.java`, if the password array cannot be zeroed, the application should crash rather than silently continue, ensuring that sensitive data is securely handled.
Learnt from: infeo
PR: cryptomator/cli#68
File: src/main/java/org/cryptomator/cli/PasswordSource.java:49-50
Timestamp: 2024-11-12T05:57:38.010Z
Learning: In the Cryptomator CLI application, if `System.console()` returns `null`, it's acceptable to throw an `IllegalStateException` because the application cannot function without a console.
🔇 Additional comments (5)
src/main/java/org/cryptomator/cli/PasswordSource.java (2)
81-106: LGTM! Well-structured exception hierarchy
The exception classes are well-organized with clear inheritance and purpose.
108-114: LGTM! Secure implementation of password handling
The Passphrase record correctly implements AutoCloseable and securely clears sensitive data. As per the project's security requirements, if the array cannot be cleared, the application will fail fast rather than continue with sensitive data potentially exposed.
pom.xml (3)
Line range hint 87-112: LGTM: Well-structured dependency management
The dependencies are well-organized with:
- Proper version management through properties
- Complete logging stack (slf4j + logback)
- Modern CLI parsing with picocli
124-133: LGTM: Proper annotation processor configuration
The maven-compiler-plugin configuration correctly sets up picocli's annotation processor for generating CLI command metadata.
14-14:
Consider using JDK 17 (LTS) instead of JDK 22
Using JDK 22 (non-LTS) may limit adoption and long-term maintainability. Consider downgrading to JDK 17 (LTS) unless specific JDK 22 features are required.
This PR refactors the CLI project completely.
The new goal of the project is to provide a compact (at best native) executable, which unlocks and mounts a single vault, following a "one mount, one process" principle. The vault can be locked & unmounted by pressing
Ctrl+Cin the terminal window. If a graceful unmount cannot be performed, a message is printed the the console and possibly the user has to perform a manual cleanup.The password can be read from 3 sources:
The OS integration/mount is determined by the mounter. To get a list of all mounters, use the
list-mounterssub command. As written in the README, the FUSE related mounters need additional libraries not shipped with the cli.The application will be shipped as jpackaged zip-file. Unzip it and run the binary.
On the technical side, instead of a self programmed argument parsing solution, the project relies now on picoCLI, a modern, flexible and easy-to-use CLI framework.
Remarks: