Open
Conversation
📝 WalkthroughWalkthroughRefactors CLI construction to a factory, removes resource-based version fallback, extracts loopback and terminal-backend utilities, adds a headless request planner, updates native-image/resource build settings, and adapts tests to new CLI and option-recording flows. Changes
Sequence DiagramssequenceDiagram
participant User
participant CliFactory as CliCommandLineFactory
participant CmdLine as CommandLine
participant Parse as ParseResult
participant ReqOpts as RequestOptions
participant Root as QuarkusForgeCli
participant Gen as GenerateCommand
User->>CliFactory: create(cliInstance)
CliFactory->>CmdLine: construct/configure (subcommands, version, exec strategy)
User->>CmdLine: execute(args)
CmdLine->>Parse: parse args
CmdLine->>ReqOpts: recordMatchedOptions(Parse)
ReqOpts->>ReqOpts: update matchedOptions
CmdLine->>Root: dispatch command
alt Generate matched
Root->>Gen: invoke (root injected)
Gen->>ReqOpts: access values via requestOptions()
end
CmdLine-->>User: return exit code
sequenceDiagram
participant Service as HeadlessGenerationService
participant Planner as HeadlessRequestPlanner
participant Resolver as InputResolutionService
participant ExtRes as HeadlessExtensionResolutionService
participant Persist as HeadlessForgefilePersistenceService
participant Plan as HeadlessGenerationPlan
Service->>Planner: plan(inputs, catalog, timeout)
Planner->>Resolver: resolveValidatedState(inputs, catalog)
Resolver-->>Planner: validated state
Planner->>ExtRes: resolveExtensionIds(validatedState, timeout)
ExtRes-->>Planner: extensionIds
alt lock checks enabled
Planner->>Persist: validateDrift(validatedState)
end
Planner->>Plan: create(request + extensionIds)
Planner-->>Service: return plan
Service->>Service: execute generation using plan.request() and plan.extensionIds()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Replace picocli's generated native metadata with a minimal handwritten configuration, drop unused JLine metadata on Unix builds, and remove the SharedArenaSupport native-image flag.\n\nAlso simplify CLI version wiring and extract smaller runtime/headless helpers without changing behavior. The validated native size moved from 51,450,640 bytes to 50,008,848 bytes total (headless 23,267,720 -> 23,202,184; interactive 28,182,920 -> 26,806,664).
ayagmar
force-pushed
the
autoresearch/native-image-size-20260313
branch
from
b615110 to
e5ae32c
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCli.java (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Spotless formatting violations.
The CI pipeline reports Spotless format check failures for this file. Run the formatter before merging:
./mvnw spotless:apply🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCli.java` at line 1, The file package declaration in QuarkusForgeCli (package dev.ayagmar.quarkusforge.cli and associated class QuarkusForgeCli) violates Spotless formatting; run the project's formatter (./mvnw spotless:apply) or reformat the file to comply with Spotless rules and commit the modified file so CI passes, ensuring the package declaration and the class file are consistent with the project's Spotless configuration.src/main/java/dev/ayagmar/quarkusforge/cli/GenerateCommand.java (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Spotless formatting violations.
The CI pipeline reports Spotless format check failures for this file. Run the formatter before merging:
./mvnw spotless:apply🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ayagmar/quarkusforge/cli/GenerateCommand.java` at line 1, This file has Spotless formatting violations; run the formatter and commit the changes: execute ./mvnw spotless:apply (or run your IDE/formatter) to reformat the file containing package dev.ayagmar.quarkusforge.cli and the GenerateCommand class, review the modified GenerateCommand.java to ensure formatting is correct, then stage and push the updated file and re-run the build (mvn verify) to confirm Spotless passes.
🧹 Nitpick comments (4)
src/main/java/dev/ayagmar/quarkusforge/runtime/TuiBootstrapService.java (1)
31-31: Consider consolidating the property name constant.
BACKEND_PROPERTY_NAMEis duplicated in bothTuiBootstrapServiceandTerminalBackendPreference. You could expose a package-private getter inTerminalBackendPreferenceto eliminate the duplication:// In TerminalBackendPreference static String backendPropertyName() { return BACKEND_PROPERTY_NAME; }Then use
TerminalBackendPreference.backendPropertyName()at lines 81 and 138.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ayagmar/quarkusforge/runtime/TuiBootstrapService.java` at line 31, TuiBootstrapService defines a duplicate BACKEND_PROPERTY_NAME constant already present in TerminalBackendPreference; remove the duplicate constant from TuiBootstrapService and add a package-private static accessor in TerminalBackendPreference (e.g., backendPropertyName()) that returns TerminalBackendPreference's BACKEND_PROPERTY_NAME, then update TuiBootstrapService to call TerminalBackendPreference.backendPropertyName() wherever it used its local BACKEND_PROPERTY_NAME (references around usages formerly at lines 81 and 138).src/test/java/dev/ayagmar/quarkusforge/runtime/RuntimeConfigTest.java (1)
205-208: Consider callingLoopbackHostshelper methods directly instead of via reflection.Lines 205–208 use reflection to invoke
isLoopbackHost,isIpv4LoopbackHost, andisIpv4Octet. Since both the test andLoopbackHostsare in the same package and these methods are package-private static, they can be called directly withoutgetDeclaredMethodandsetAccessible, gaining type safety and clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/ayagmar/quarkusforge/runtime/RuntimeConfigTest.java` around lines 205 - 208, The test currently uses reflection in invokeBooleanHelper to call LoopbackHosts package-private helpers; replace that reflective call by invoking the methods directly (e.g., call LoopbackHosts.isLoopbackHost(value), LoopbackHosts.isIpv4LoopbackHost(value), LoopbackHosts.isIpv4Octet(value)) from the test so you get compile-time type safety and clarity; update or remove the invokeBooleanHelper helper to call the appropriate LoopbackHosts static method directly (or overload it to accept a Function<String,Boolean> and pass method references), and remove unnecessary setAccessible / reflection exception handling.src/main/java/dev/ayagmar/quarkusforge/headless/HeadlessRequestPlanner.java (1)
23-25: Narrow theplan(...)exception contract.
throws Exception(Line 25) is unnecessarily broad. The method only propagates checked exceptions fromextensionResolutionService.resolveExtensionIds(), which declaresthrows ExecutionException, InterruptedException, TimeoutException.♻️ Proposed narrowing
+import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; ... HeadlessGenerationPlan plan( HeadlessGenerationInputs inputs, CatalogData catalogData, Duration catalogTimeout) - throws Exception { + throws ExecutionException, InterruptedException, TimeoutException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ayagmar/quarkusforge/headless/HeadlessRequestPlanner.java` around lines 23 - 25, The method HeadlessGenerationPlan plan(HeadlessGenerationInputs inputs, CatalogData catalogData, Duration catalogTimeout) currently declares throws Exception; narrow this to only the checked exceptions propagated from extensionResolutionService.resolveExtensionIds(): replace throws Exception with throws ExecutionException, InterruptedException, TimeoutException on the plan(...) signature and update any callers as needed to handle or rethrow these specific exceptions; reference extensionResolutionService.resolveExtensionIds() to locate the source of the checked exceptions.pom.xml (1)
446-448: Pinnative-maven-pluginversion in the Unix trim profile.The plugin in the
unix-native-jline-trimprofile (line 447) is declared without<version>, while the same plugin is versioned elsewhere in the pom (line 407). Since this profile auto-activates on Unix systems via<os><family>unix</family></os>, the missing version can trigger Maven effective-model warnings and introduce build inconsistency.💡 Suggested change
<plugin> <groupId>org.graalvm.buildtools</groupId> <artifactId>native-maven-plugin</artifactId> + <version>${graalvm.native.plugin.version}</version> <configuration>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 446 - 448, The unix-native-jline-trim profile declares the org.graalvm.buildtools:native-maven-plugin without a <version>, which can cause effective-model warnings and inconsistent builds; update the plugin declaration inside the unix-native-jline-trim profile to include the same <version> value used elsewhere in the POM (the native-maven-plugin version declared near the other plugin entry) so the profile is fully version-pinned and consistent across environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/ayagmar/quarkusforge/cli/CliCommandLineFactory.java`:
- Around line 8-42: Run the code formatter/Spotless on the new file containing
CliCommandLineFactory (class and its methods create, addRequestOptionsMixin,
recordMatchedRequestOptions) to fix whitespace and style issues; ensure the file
adheres to the project's Spotless rules (e.g., import/order, spacing, braces)
and re-commit the formatted file so CI passes.
In `@src/main/java/dev/ayagmar/quarkusforge/cli/HeadlessCli.java`:
- Around line 41-42: The file HeadlessCli.java has Spotless formatting
violations; run the formatter and fix the style issues by applying mvn
spotless:apply and then reformat the class so it conforms to project
rules—specifically update the formatting around the usage call and return in the
method that calls CliCommandLineFactory.create(this).usage(System.out); and also
fix the formatting around the other violation near the code at the location
referenced (around the other statement at line ~58) to ensure Spotless passes.
In
`@src/main/java/dev/ayagmar/quarkusforge/headless/HeadlessGenerationService.java`:
- Around line 65-67: Spotless formatting violations were detected in
HeadlessGenerationService.java; run your project's Spotless formatter (e.g.,
./mvnw spotless:apply or gradle spotlessApply) and apply the fixes, then commit
the updated file. Specifically reformat the constructor block that instantiates
HeadlessRequestPlanner (the lines assigning this.requestPlanner with new
HeadlessRequestPlanner(Objects.requireNonNull(extensionResolutionService),
forgefilePersistenceService)) and any other offending sections around lines
where HeadlessGenerationService sets up requestPlanner and related fields so the
file matches the repository's Spotless rules before pushing.
In `@src/main/java/dev/ayagmar/quarkusforge/runtime/RuntimeConfig.java`:
- Around line 59-61: This file has Spotless formatting violations; run the
Spotless formatter and commit the corrected file so CI passes. Open the
RuntimeConfig class and reformat (or run the project-level Spotless/Maven/Gradle
task) to fix spacing/line-wrapping around the conditional that checks
normalizedScheme and LoopbackHosts.isLoopbackHost (the block containing
normalizedScheme.equals("http") and LoopbackHosts.isLoopbackHost(host)), then
stage and commit the formatted RuntimeConfig.java.
In `@src/test/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCliPrefillTest.java`:
- Around line 26-27: The file QuarkusForgeCliPrefillTest has a Spotless
formatting violation causing CI to fail; run the project's formatter/Spotless
autocorrect on this file (or reformat the contents of class
QuarkusForgeCliPrefillTest and the block covering lines ~31-141) so the spacing,
indentation and import ordering follow the repository style, then save and
commit the reformatted file so the CI pipeline no longer reports the violation.
In `@src/test/java/dev/ayagmar/quarkusforge/TuiBootstrapServiceTest.java`:
- Around line 206-207: The test file has Spotless formatting violations around
the reflective method lookup
backendPreferenceClass().getDeclaredMethod("configure", String.class,
String.class, String.class); — run the formatter (e.g., ./mvnw spotless:apply)
or your IDE's Spotless plugin to reformat the file and commit the changes so the
line breaks/spacing around that call (and the rest of TuiBootstrapServiceTest)
comply with the project's Spotless rules.
---
Outside diff comments:
In `@src/main/java/dev/ayagmar/quarkusforge/cli/GenerateCommand.java`:
- Line 1: This file has Spotless formatting violations; run the formatter and
commit the changes: execute ./mvnw spotless:apply (or run your IDE/formatter) to
reformat the file containing package dev.ayagmar.quarkusforge.cli and the
GenerateCommand class, review the modified GenerateCommand.java to ensure
formatting is correct, then stage and push the updated file and re-run the build
(mvn verify) to confirm Spotless passes.
In `@src/main/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCli.java`:
- Line 1: The file package declaration in QuarkusForgeCli (package
dev.ayagmar.quarkusforge.cli and associated class QuarkusForgeCli) violates
Spotless formatting; run the project's formatter (./mvnw spotless:apply) or
reformat the file to comply with Spotless rules and commit the modified file so
CI passes, ensuring the package declaration and the class file are consistent
with the project's Spotless configuration.
---
Nitpick comments:
In `@pom.xml`:
- Around line 446-448: The unix-native-jline-trim profile declares the
org.graalvm.buildtools:native-maven-plugin without a <version>, which can cause
effective-model warnings and inconsistent builds; update the plugin declaration
inside the unix-native-jline-trim profile to include the same <version> value
used elsewhere in the POM (the native-maven-plugin version declared near the
other plugin entry) so the profile is fully version-pinned and consistent across
environments.
In `@src/main/java/dev/ayagmar/quarkusforge/headless/HeadlessRequestPlanner.java`:
- Around line 23-25: The method HeadlessGenerationPlan
plan(HeadlessGenerationInputs inputs, CatalogData catalogData, Duration
catalogTimeout) currently declares throws Exception; narrow this to only the
checked exceptions propagated from
extensionResolutionService.resolveExtensionIds(): replace throws Exception with
throws ExecutionException, InterruptedException, TimeoutException on the
plan(...) signature and update any callers as needed to handle or rethrow these
specific exceptions; reference extensionResolutionService.resolveExtensionIds()
to locate the source of the checked exceptions.
In `@src/main/java/dev/ayagmar/quarkusforge/runtime/TuiBootstrapService.java`:
- Line 31: TuiBootstrapService defines a duplicate BACKEND_PROPERTY_NAME
constant already present in TerminalBackendPreference; remove the duplicate
constant from TuiBootstrapService and add a package-private static accessor in
TerminalBackendPreference (e.g., backendPropertyName()) that returns
TerminalBackendPreference's BACKEND_PROPERTY_NAME, then update
TuiBootstrapService to call TerminalBackendPreference.backendPropertyName()
wherever it used its local BACKEND_PROPERTY_NAME (references around usages
formerly at lines 81 and 138).
In `@src/test/java/dev/ayagmar/quarkusforge/runtime/RuntimeConfigTest.java`:
- Around line 205-208: The test currently uses reflection in invokeBooleanHelper
to call LoopbackHosts package-private helpers; replace that reflective call by
invoking the methods directly (e.g., call LoopbackHosts.isLoopbackHost(value),
LoopbackHosts.isIpv4LoopbackHost(value), LoopbackHosts.isIpv4Octet(value)) from
the test so you get compile-time type safety and clarity; update or remove the
invokeBooleanHelper helper to call the appropriate LoopbackHosts static method
directly (or overload it to accept a Function<String,Boolean> and pass method
references), and remove unnecessary setAccessible / reflection exception
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 041fcf7f-3f1b-494e-99bd-d81917927eb6
📒 Files selected for processing (23)
pom.xmlsrc/main/java/dev/ayagmar/quarkusforge/cli/CliCommandLineFactory.javasrc/main/java/dev/ayagmar/quarkusforge/cli/CliVersionProvider.javasrc/main/java/dev/ayagmar/quarkusforge/cli/GenerateCommand.javasrc/main/java/dev/ayagmar/quarkusforge/cli/HeadlessCli.javasrc/main/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCli.javasrc/main/java/dev/ayagmar/quarkusforge/cli/RequestOptions.javasrc/main/java/dev/ayagmar/quarkusforge/headless/HeadlessGenerationService.javasrc/main/java/dev/ayagmar/quarkusforge/headless/HeadlessRequestPlanner.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/LoopbackHosts.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/RuntimeConfig.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/TerminalBackendPreference.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/TuiBootstrapService.javasrc/main/resources/META-INF/native-image/dev.ayagmar/quarkusforge/reflect-config.jsonsrc/main/resources/META-INF/native-image/dev.ayagmar/quarkusforge/resource-config.jsonsrc/main/resources/version.propertiessrc/test/java/dev/ayagmar/quarkusforge/TuiBootstrapServiceTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/CliVersionProviderTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/HeadlessCliTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCliPrefillTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCliStartupMetadataTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCliTest.javasrc/test/java/dev/ayagmar/quarkusforge/runtime/RuntimeConfigTest.java
💤 Files with no reviewable changes (2)
- src/main/resources/META-INF/native-image/dev.ayagmar/quarkusforge/resource-config.json
- src/main/resources/version.properties
src/main/java/dev/ayagmar/quarkusforge/headless/HeadlessGenerationService.java
Show resolved
Hide resolved
src/test/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCliPrefillTest.java
Show resolved
Hide resolved
Comment on lines
+206
to
+207
| backendPreferenceClass().getDeclaredMethod( | ||
| "configure", String.class, String.class, String.class); |
There was a problem hiding this comment.
Fix Spotless formatting violations.
The pipeline failure indicates formatting issues in this file. Please run ./mvnw spotless:apply (or equivalent) to auto-fix the formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/dev/ayagmar/quarkusforge/TuiBootstrapServiceTest.java` around
lines 206 - 207, The test file has Spotless formatting violations around the
reflective method lookup backendPreferenceClass().getDeclaredMethod("configure",
String.class, String.class, String.class); — run the formatter (e.g., ./mvnw
spotless:apply) or your IDE's Spotless plugin to reformat the file and commit
the changes so the line breaks/spacing around that call (and the rest of
TuiBootstrapServiceTest) comply with the project's Spotless rules.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/dev/ayagmar/quarkusforge/runtime/TuiBootstrapService.java (1)
31-31: Consider removing duplicated constant.
BACKEND_PROPERTY_NAMEis defined both here and inTerminalBackendPreference. While it's used here to capturepreviousBackendPreferencebefore callingconfigure(), consider exposing this constant fromTerminalBackendPreferenceor adding a getter method to avoid potential drift if the property name ever changes.♻️ Possible approach
Add a package-private getter in
TerminalBackendPreference:static String backendPropertyName() { return BACKEND_PROPERTY_NAME; }Then use it here:
- private static final String BACKEND_PROPERTY_NAME = "tamboui.backend"; // ... - String previousBackendPreference = System.getProperty(BACKEND_PROPERTY_NAME); + String previousBackendPreference = System.getProperty(TerminalBackendPreference.backendPropertyName());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ayagmar/quarkusforge/runtime/TuiBootstrapService.java` at line 31, Duplicate constant BACKEND_PROPERTY_NAME exists; change TuiBootstrapService to stop defining its own BACKEND_PROPERTY_NAME and instead call a new package-private accessor on TerminalBackendPreference (e.g., add static String backendPropertyName() { return BACKEND_PROPERTY_NAME; }) and replace usages that set previousBackendPreference before calling configure() to use TerminalBackendPreference.backendPropertyName(); ensure no other references rely on the removed constant and update imports if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/dev/ayagmar/quarkusforge/cli/CliVersionProviderTest.java`:
- Around line 16-20: The test name
resolveVersionFallsBackToUnknownWhenImplementationVersionIsUnavailable
over-claims coverage; rename the JUnit test method in CliVersionProviderTest to
a truthful name (e.g., resolveVersionReturnsSemverOrUnknown or
resolveVersionMatchesSemverOrUnknown) so it reflects that it only asserts
CliVersionProvider.resolveVersion() returns a semver-ish string or "unknown";
update the method name everywhere it’s referenced (test runner/annotations) and
leave the assertions (String version = CliVersionProvider.resolveVersion();
assertThat(...).isNotNull(); assertThat(...).matches(...);) unchanged.
---
Nitpick comments:
In `@src/main/java/dev/ayagmar/quarkusforge/runtime/TuiBootstrapService.java`:
- Line 31: Duplicate constant BACKEND_PROPERTY_NAME exists; change
TuiBootstrapService to stop defining its own BACKEND_PROPERTY_NAME and instead
call a new package-private accessor on TerminalBackendPreference (e.g., add
static String backendPropertyName() { return BACKEND_PROPERTY_NAME; }) and
replace usages that set previousBackendPreference before calling configure() to
use TerminalBackendPreference.backendPropertyName(); ensure no other references
rely on the removed constant and update imports if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9be34f4a-06b4-4adf-a7bc-406f2986e052
📒 Files selected for processing (23)
pom.xmlsrc/main/java/dev/ayagmar/quarkusforge/cli/CliCommandLineFactory.javasrc/main/java/dev/ayagmar/quarkusforge/cli/CliVersionProvider.javasrc/main/java/dev/ayagmar/quarkusforge/cli/GenerateCommand.javasrc/main/java/dev/ayagmar/quarkusforge/cli/HeadlessCli.javasrc/main/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCli.javasrc/main/java/dev/ayagmar/quarkusforge/cli/RequestOptions.javasrc/main/java/dev/ayagmar/quarkusforge/headless/HeadlessGenerationService.javasrc/main/java/dev/ayagmar/quarkusforge/headless/HeadlessRequestPlanner.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/LoopbackHosts.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/RuntimeConfig.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/TerminalBackendPreference.javasrc/main/java/dev/ayagmar/quarkusforge/runtime/TuiBootstrapService.javasrc/main/resources/META-INF/native-image/dev.ayagmar/quarkusforge/reflect-config.jsonsrc/main/resources/META-INF/native-image/dev.ayagmar/quarkusforge/resource-config.jsonsrc/main/resources/version.propertiessrc/test/java/dev/ayagmar/quarkusforge/TuiBootstrapServiceTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/CliVersionProviderTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/HeadlessCliTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCliPrefillTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCliStartupMetadataTest.javasrc/test/java/dev/ayagmar/quarkusforge/cli/QuarkusForgeCliTest.javasrc/test/java/dev/ayagmar/quarkusforge/runtime/RuntimeConfigTest.java
💤 Files with no reviewable changes (2)
- src/main/resources/META-INF/native-image/dev.ayagmar/quarkusforge/resource-config.json
- src/main/resources/version.properties
🚧 Files skipped from review as they are similar to previous changes (6)
- pom.xml
- src/main/java/dev/ayagmar/quarkusforge/cli/HeadlessCli.java
- src/main/java/dev/ayagmar/quarkusforge/runtime/LoopbackHosts.java
- src/main/resources/META-INF/native-image/dev.ayagmar/quarkusforge/reflect-config.json
- src/main/java/dev/ayagmar/quarkusforge/cli/GenerateCommand.java
- src/main/java/dev/ayagmar/quarkusforge/cli/CliVersionProvider.java
Comment on lines
+16
to
20
| void resolveVersionFallsBackToUnknownWhenImplementationVersionIsUnavailable() { | ||
| String version = CliVersionProvider.resolveVersion(); | ||
| assertThat(version).isNotNull(); | ||
| // Must be a version string or "unknown" | ||
| assertThat(version).matches("(\\d+\\.\\d+\\.\\d+.*|unknown)"); | ||
| } |
There was a problem hiding this comment.
Test name over-claims fallback coverage.
The test on Line 16 says it validates the “implementation version unavailable” path, but it never forces that condition; it currently validates only “semver-ish or unknown”. Please rename to reflect actual coverage.
✅ Suggested fix
- void resolveVersionFallsBackToUnknownWhenImplementationVersionIsUnavailable() {
+ void resolveVersionReturnsSemverOrUnknown() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/dev/ayagmar/quarkusforge/cli/CliVersionProviderTest.java`
around lines 16 - 20, The test name
resolveVersionFallsBackToUnknownWhenImplementationVersionIsUnavailable
over-claims coverage; rename the JUnit test method in CliVersionProviderTest to
a truthful name (e.g., resolveVersionReturnsSemverOrUnknown or
resolveVersionMatchesSemverOrUnknown) so it reflects that it only asserts
CliVersionProvider.resolveVersion() returns a semver-ish string or "unknown";
update the method name everywhere it’s referenced (test runner/annotations) and
leave the assertions (String version = CliVersionProvider.resolveVersion();
assertThat(...).isNotNull(); assertThat(...).matches(...);) unchanged.
…hat removes the JdkLDAP provider from the native provider list while preserving live HTTPS, native dry-runs, release smoke, verify, and format checks\n\nResult: {"status":"keep","native_total_bytes":48829200}
…curity replacement that keeps only the non-LDAP provider list plus required keystore, SSL, and SecureRandom properties; native size improved and live HTTPS, native smoke, verify, and format checks still pass\n\nResult: {"status":"keep","native_total_bytes":48763664}
…cement; native size improved again while live HTTPS, native smoke, verify, and format checks still pass\n\nResult: {"status":"keep","native_total_bytes":48239376}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
config/native/no-ldap.java.security (1)
15-16: Consider using/dev/urandomto avoid potential blocking.
/dev/randomblocks when the entropy pool is depleted, which can cause startup delays or hangs in low-entropy environments such as containers, VMs, or freshly booted systems. For most cryptographic purposes,/dev/urandomis equally secure and non-blocking.If blocking behavior is intentional for your security requirements, consider documenting this choice.
💡 Suggested change to use non-blocking entropy source
-securerandom.source=file:/dev/random +securerandom.source=file:/dev/urandom🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/native/no-ldap.java.security` around lines 15 - 16, The securerandom.source property is set to file:/dev/random which can block; change the value of securerandom.source to use file:/dev/urandom instead (i.e., set securerandom.source to file:/dev/urandom) to avoid blocking in low-entropy environments, and if the blocking behavior was intentional, add a comment or documentation explaining why /dev/random is required; refer to the securerandom.source and securerandom.strongAlgorithms settings when applying the change.pom.xml (2)
437-459: Ensure consistent plugin version pinning and conditional activation across profiles.The
unix-native-jline-trimprofile activates automatically on all Unix systems but lacks an explicit version fornative-maven-plugin. This contrasts with thenativeprofile, which pins the plugin version to${graalvm.native.plugin.version}. Consider adding an explicit version reference here for build consistency and to avoid relying on Maven's default plugin resolution.Additionally, to prevent this profile from affecting non-native Unix builds, consider gating the activation to native builds using a property condition (as shown in the proposed diff below).
Proposed pom.xml adjustment
<profile> <id>unix-native-jline-trim</id> <activation> <os> <family>unix</family> </os> + <property> + <name>native.profile.active</name> + <value>true</value> + </property> </activation> <build> <plugins> <plugin> <groupId>org.graalvm.buildtools</groupId> <artifactId>native-maven-plugin</artifactId> + <version>${graalvm.native.plugin.version}</version> <configuration> <buildArgs combine.children="append"> <buildArg>--exclude-config</buildArg> <buildArg>jline-.*\.jar</buildArg> <buildArg>META-INF/native-image/org\.jline/(jansi|jline-native|jline-terminal-jansi|jline-terminal-jna|jline-terminal-jni)/.*</buildArg> </buildArgs> </configuration> </plugin> </plugins> </build> </profile>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 437 - 459, The unix-native-jline-trim profile is auto-activated on Unix but does not pin the native-maven-plugin version and can unintentionally affect non-native builds; update the profile (id "unix-native-jline-trim") to add an explicit <version> element for org.graalvm.buildtools:native-maven-plugin using the existing ${graalvm.native.plugin.version} property, and gate activation by adding a property condition (e.g., require a native build flag like native.build=true) alongside the current <os><family>unix</family></os> activation so the profile only runs for native builds on Unix.
427-427: Fulljava.securityreplacement needs a drift guard.Line 427 uses
==, which fully replaces JDK defaults. That’s fine for size, but it can silently miss new security properties when Java updates. Add a CI verification step to detect missing non-provider keys.#!/bin/bash set -euo pipefail CUSTOM_FILE="$(fd -a 'no-ldap\.java\.security$' config | head -n1 || true)" if [[ -z "${CUSTOM_FILE}" ]]; then echo "ERROR: custom java.security file not found under config/" exit 1 fi if [[ -z "${JAVA_HOME:-}" || ! -f "${JAVA_HOME}/conf/security/java.security" ]]; then echo "SKIP: JAVA_HOME/conf/security/java.security not available in this environment." exit 0 fi python - "$CUSTOM_FILE" "${JAVA_HOME}/conf/security/java.security" <<'PY' import re, sys custom_path, jdk_path = sys.argv[1], sys.argv[2] def read_keys(path): keys = set() with open(path, encoding="utf-8") as f: for raw in f: line = raw.strip() if not line or line.startswith("#") or "=" not in line: continue key = line.split("=", 1)[0].strip() if re.fullmatch(r"security\.provider\.\d+", key): continue keys.add(key) return keys custom = read_keys(custom_path) jdk = read_keys(jdk_path) missing = sorted(jdk - custom) print(f"Custom file: {custom_path}") print(f"JDK file : {jdk_path}") print(f"Missing non-provider keys: {len(missing)}") for k in missing: print(k) # Non-zero exit if drift is detected sys.exit(1 if missing else 0) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` at line 427, The pom.xml buildArg line that sets "-Djava.security.properties==${project.basedir}/config/native/no-ldap.java.security" fully replaces JDK defaults and needs a CI drift guard: add a CI job (e.g., GitHub Actions workflow or similar) that locates config/native/no-ldap.java.security and compares its keys against ${JAVA_HOME}/conf/security/java.security using the provided shell+Python check (skip if JAVA_HOME not present, fail the job if any non-provider keys are missing), and wire this job to run on PRs; ensure the job references the exact filename "no-ldap.java.security" and exits non-zero when drift is detected so authors must update the custom file when JDK security keys change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/native/no-ldap.java.security`:
- Around line 1-16: Add two properties to this security properties set:
jdk.tls.disabledAlgorithms and jdk.certpath.disabledAlgorithms. Set
jdk.tls.disabledAlgorithms to disable legacy protocols and weak
ciphers/signatures (e.g. TLSv1, TLSv1.1, SSLv3, MD5, SHA1, anon, NULL) and set
jdk.certpath.disabledAlgorithms to disable weak signature/hash algorithms and
short keys (e.g. MD2, MD5, SHA1, DSA, RSA keySize < 2048) so the custom file
preserves the JDK’s default algorithm restrictions; include these property names
exactly (jdk.tls.disabledAlgorithms and jdk.certpath.disabledAlgorithms) in the
file alongside the existing entries such as security.provider.* and
keystore.type.
---
Nitpick comments:
In `@config/native/no-ldap.java.security`:
- Around line 15-16: The securerandom.source property is set to file:/dev/random
which can block; change the value of securerandom.source to use
file:/dev/urandom instead (i.e., set securerandom.source to file:/dev/urandom)
to avoid blocking in low-entropy environments, and if the blocking behavior was
intentional, add a comment or documentation explaining why /dev/random is
required; refer to the securerandom.source and securerandom.strongAlgorithms
settings when applying the change.
In `@pom.xml`:
- Around line 437-459: The unix-native-jline-trim profile is auto-activated on
Unix but does not pin the native-maven-plugin version and can unintentionally
affect non-native builds; update the profile (id "unix-native-jline-trim") to
add an explicit <version> element for org.graalvm.buildtools:native-maven-plugin
using the existing ${graalvm.native.plugin.version} property, and gate
activation by adding a property condition (e.g., require a native build flag
like native.build=true) alongside the current <os><family>unix</family></os>
activation so the profile only runs for native builds on Unix.
- Line 427: The pom.xml buildArg line that sets
"-Djava.security.properties==${project.basedir}/config/native/no-ldap.java.security"
fully replaces JDK defaults and needs a CI drift guard: add a CI job (e.g.,
GitHub Actions workflow or similar) that locates
config/native/no-ldap.java.security and compares its keys against
${JAVA_HOME}/conf/security/java.security using the provided shell+Python check
(skip if JAVA_HOME not present, fail the job if any non-provider keys are
missing), and wire this job to run on PRs; ensure the job references the exact
filename "no-ldap.java.security" and exits non-zero when drift is detected so
authors must update the custom file when JDK security keys change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62384dd4-62dc-45b9-9cdf-3fa644ae6977
📒 Files selected for processing (2)
config/native/no-ldap.java.securitypom.xml
Comment on lines
+1
to
+16
| security.provider.1=SUN | ||
| security.provider.2=SunRsaSign | ||
| security.provider.3=SunEC | ||
| security.provider.4=SunJSSE | ||
| security.provider.5=SunJCE | ||
| security.provider.6=SunJGSS | ||
| security.provider.7=SunSASL | ||
| security.provider.8=XMLDSig | ||
| security.provider.9=SunPCSC | ||
| security.provider.10=JdkSASL | ||
| security.provider.11=SunPKCS11 | ||
| keystore.type=pkcs12 | ||
| ssl.KeyManagerFactory.algorithm=SunX509 | ||
| ssl.TrustManagerFactory.algorithm=PKIX | ||
| securerandom.source=file:/dev/random | ||
| securerandom.strongAlgorithms=NativePRNGBlocking:SUN,DRBG:SUN |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if there are other security configuration files that might provide algorithm restrictions,
# and verify if native-image build configuration references additional security properties.
echo "=== Searching for other java.security or security-related config files ==="
fd -e security -e properties . config/
echo ""
echo "=== Checking native-image configuration for security property overrides ==="
rg -n "disabledAlgorithms|security\.provider|java\.security" --type-add 'native:*.json' --type native config/
rg -n "disabledAlgorithms|java\.security" pom.xmlRepository: ayagmar/quarkus-forge
Length of output: 376
🏁 Script executed:
#!/bin/bash
# Check for native-image configuration files
echo "=== Searching for native-image configuration files ==="
fd -e properties -e json . | rg -i "native.image|native-image"
echo ""
echo "=== Checking pom.xml context around the security properties setting ==="
rg -B 5 -A 5 "java.security.properties" pom.xml
echo ""
echo "=== Looking for comments or documentation about security config ==="
cat config/native/no-ldap.java.securityRepository: ayagmar/quarkus-forge
Length of output: 1510
Add jdk.tls.disabledAlgorithms and jdk.certpath.disabledAlgorithms to the security configuration.
The custom security properties file replaces JDK defaults entirely when set via -Djava.security.properties. Without explicitly disabling weak algorithms (MD5, SHA1withRSA, TLSv1, TLSv1.1), the native image will accept deprecated protocols and signatures. These restrictions must be added to this configuration file to prevent use of insecure algorithms.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/native/no-ldap.java.security` around lines 1 - 16, Add two properties
to this security properties set: jdk.tls.disabledAlgorithms and
jdk.certpath.disabledAlgorithms. Set jdk.tls.disabledAlgorithms to disable
legacy protocols and weak ciphers/signatures (e.g. TLSv1, TLSv1.1, SSLv3, MD5,
SHA1, anon, NULL) and set jdk.certpath.disabledAlgorithms to disable weak
signature/hash algorithms and short keys (e.g. MD2, MD5, SHA1, DSA, RSA keySize
< 2048) so the custom file preserves the JDK’s default algorithm restrictions;
include these property names exactly (jdk.tls.disabledAlgorithms and
jdk.certpath.disabledAlgorithms) in the file alongside the existing entries such
as security.provider.* and keystore.type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SharedArenaSupportnative-image flag, and simplify CLI version wiring to reduce reachabilityNative size impact
51,450,640->50,008,848bytes (-1,441,792,-2.80%)23,267,720->23,202,184bytes (-65,536,-0.28%)28,182,920->26,806,664bytes (-1,376,256,-4.88%)Validation
./mvnw -q verify -Djacoco.skip=trueSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores