Conversation
- Java version 21 → 25 - Default versions: Pragmatica 0.11.1, Aether 0.8.1, JBCT 0.6.0 - Implementation pattern: record-based nested in interface - Factory accepts Config dependency parameter - Removed separate *Impl.java, SampleRequest.java, SampleResponse.java - Request/Response/Error records nested in @slice interface Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Request/Response instead of SampleRequest/SampleResponse - Inner implementation record named after lowercased slice name (e.g., mySlice) not Impl Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughBumps project to 0.6.0 and Java 25, replaces single slice-api properties with per-slice manifests under META-INF/slice/*.manifest, introduces JBCT scoring (CLI + Maven), refactors slice generation/packaging (dependency split, route integration, bytecode transforms), tightens validations, and updates CI to install modules individually. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "CLI / ScoreCommand"
participant Processor as "SliceProcessor"
participant Generator as "Manifest/Factory Generator"
participant Infra as "InfraStore/Resolver"
participant Artifact as "Built JAR"
participant Plugin as "jbct-maven-plugin"
participant Validator as "Verify/Validator"
CLI->>Processor: request generate/init
Processor->>Generator: generateFactory (split infraDeps, sliceDeps)
Generator->>Infra: resolve infraDeps & sliceDeps (uses slice context)
Generator->>Artifact: emit classes + META-INF/slice/{Name}.manifest (include routesClass?)
Processor->>Processor: collect route entries
Processor->>Artifact: write META-INF/services if routes present
Plugin->>Artifact: scan JAR for META-INF/slice/*.manifest
Plugin->>Validator: validate manifests & routes.toml requirements
Validator-->>Plugin: validation result (errors/warnings)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
- Java: 21 → 25 - Pragmatica Lite: 0.10.0 → 0.11.1 - Aether: 0.8.0 → 0.8.1 - JBCT: 0.5.0 → 0.6.0 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…solution Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pom.xml (1)
48-66: Add required Aether URL properties for deploy scripts.The deploy scripts require
aether.forge.url,aether.test.url, andaether.prod.urlin POM properties; they’re missing here. Please add them with the values used by the deploy scripts.🧩 Suggested structure (fill with real URLs)
<properties> <java.version>25</java.version> <maven.compiler.source>${java.version}</maven.compiler.source> <maven.compiler.target>${java.version}</maven.compiler.target> <maven.compiler.release>${java.version}</maven.compiler.release> <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> + <aether.forge.url><!-- TODO: set forge repo URL --></aether.forge.url> + <aether.test.url><!-- TODO: set test repo URL --></aether.test.url> + <aether.prod.url><!-- TODO: set prod repo URL --></aether.prod.url> <!-- Dependencies --> <pragmatica-lite.version>0.11.1</pragmatica-lite.version> <aether.version>0.8.1</aether.version>As per coding guidelines, ...
slice-processor/pom.xml (1)
38-43: Fix infra-aspect version in parent pom.xml — 0.8.1 does not exist on Maven Central.The dependency inherits
aether.version=0.8.1from the parent pom, butorg.pragmatica-lite.aether:infra-aspectis only published as0.7.4on Maven Central. Either update<aether.version>in the parent pom to0.7.4, or use a separate version property for infra-aspect to unblock the build.
🤖 Fix all issues with AI agents
In
`@jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectInitializer.java`:
- Around line 515-571: The call Result.failure(ValidationError.emptyValue())
fails because pragmatica-lite 0.11.1 requires a Cause parameter; update the
sealed interface ValidationError to extend org.pragmatica.lang.Cause (or Cause)
so ValidationError.emptyValue() is a valid Cause, then keep the
Result.failure(...) call as-is; modify the ValidationError declaration (and
imports if needed) to implement Cause to restore compilation for Request.request
and Result.failure usage.
In `@pom.xml`:
- Around line 55-58: The pom currently sets aether.version to 0.8.1 which is not
in Maven Central; either change the property aether.version to 0.7.4 or add the
Aether repository configuration: define the properties aether.forge.url,
aether.test.url, and aether.prod.url and add a custom <repositories> section
that references those URLs so Maven can resolve
org.pragmatica-lite.aether:infra-aspect:0.8.1; update the
<pragmatica-lite.version> and <picocli.version> properties only if needed and
ensure the new repository properties are used by the repository entries.
jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectInitializer.java
Show resolved
Hide resolved
| <!-- Dependencies --> | ||
| <pragmatica-lite.version>0.11.1</pragmatica-lite.version> | ||
| <aether.version>0.7.5</aether.version> | ||
| <aether.version>0.8.1</aether.version> | ||
| <picocli.version>4.7.6</picocli.version> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the pom.xml file
find . -name "pom.xml" -type f | head -20Repository: siy/jbct-cli
Length of output: 190
🏁 Script executed:
# Check the size and examine the root pom.xml
wc -l ./pom.xmlRepository: siy/jbct-cli
Length of output: 69
🏁 Script executed:
# Read pom.xml around lines 55-58 and search for Aether URLs
cat -n ./pom.xml | sed -n '50,65p'Repository: siy/jbct-cli
Length of output: 982
🏁 Script executed:
# Search for Aether URL properties in pom.xml
grep -n "aether\." ./pom.xmlRepository: siy/jbct-cli
Length of output: 333
🏁 Script executed:
# Search for repositories configuration and deploy scripts
echo "=== Repositories section ===" && grep -n "repositories\|repository" ./pom.xml | head -20
echo -e "\n=== Deploy scripts ===" && find . -name "deploy-*.sh" -type fRepository: siy/jbct-cli
Length of output: 108
🏁 Script executed:
# Show the Aether dependencies in detail
cat -n ./pom.xml | sed -n '110,140p'Repository: siy/jbct-cli
Length of output: 1445
🏁 Script executed:
# Check for CI configuration files
echo "=== CI files ===" && find . -path "*/.git" -prune -o -name ".github" -o -name ".gitlab-ci.yml" -o -name ".circleci" -o -name "pom.xml" | grep -E "(github|gitlab|circleci)" | head -10
echo "=== GitHub workflow files ===" && find . -path "./.github/workflows" -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -10Repository: siy/jbct-cli
Length of output: 112
🏁 Script executed:
# List .github contents
ls -la ./.github/Repository: siy/jbct-cli
Length of output: 286
🏁 Script executed:
# List workflow files
ls -la ./.github/workflows/Repository: siy/jbct-cli
Length of output: 283
🏁 Script executed:
# Examine the CI workflow
cat ./.github/workflows/ci.ymlRepository: siy/jbct-cli
Length of output: 411
🌐 Web query:
org.pragmatica-lite.aether infra-aspect 0.8.1 Maven Central
💡 Result:
org.pragmatica-lite.aether:infra-aspect:0.8.1 is not present on Maven Central. The Maven Central repository path for this artifact shows only version 0.7.4 (as of January 27, 2026). [1]
To use what’s available on Central:
<dependency>
<groupId>org.pragmatica-lite.aether</groupId>
<artifactId>infra-aspect</artifactId>
<version>0.7.4</version>
</dependency>[2]
Sources: Maven Central directory listing [1], Sonatype Central artifact page/snippet [2].
Configure Aether repository URLs and adjust version — 0.8.1 not available in Maven Central.
The artifact org.pragmatica-lite.aether:infra-aspect:0.8.1 is not published in Maven Central (only 0.7.4 is available). More critically, pom.xml must configure Aether repository URLs per guidelines: define aether.forge.url, aether.test.url, and aether.prod.url properties and add a custom <repositories> section. Either downgrade to 0.7.4 or ensure the correct repository configuration is in place for accessing 0.8.1 from the appropriate Aether server.
🤖 Prompt for AI Agents
In `@pom.xml` around lines 55 - 58, The pom currently sets aether.version to 0.8.1
which is not in Maven Central; either change the property aether.version to
0.7.4 or add the Aether repository configuration: define the properties
aether.forge.url, aether.test.url, and aether.prod.url and add a custom
<repositories> section that references those URLs so Maven can resolve
org.pragmatica-lite.aether:infra-aspect:0.8.1; update the
<pragmatica-lite.version> and <picocli.version> properties only if needed and
ensure the new repository properties are used by the repository entries.
- ValidationError extends Cause (required for Result.failure) - Promise.success() instead of Promise.successful() Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…lice template - Add groupId validation for Java package name format - Add annotation processor configuration to maven-compiler-plugin - Remove Config dependency from slice template (slices should be self-contained) - Simplify template to parameterless factory method Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Update README slice template description (nested records, no Config) - Update SliceProjectInitializerTest expectations (parameterless factory) - Check for ValidationError instead of Config in generated code Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Update to show nested records in single interface file - Remove references to separate *Impl, *Request, *Response files - Show ValidationError extends Cause with message() implementation - Update Promise.successful() to Promise.success() - Update Java version requirement: 21+ → 25+ Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Show single MySlice.java instead of separate Impl/Request/Response files - Update JAR contents description to reflect nested structure Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectInitializer.java (1)
320-394: Add required Aether URL properties to the generated POM.The deploy scripts expect
aether.forge.url,aether.test.url, andaether.prod.url, but the template omits them, so generated projects will miss mandatory config. As per coding guidelines, add the properties (use placeholders or defaults as appropriate).
As per coding guidelines: Slice deploy scripts (deploy-forge.sh, deploy-test.sh, deploy-prod.sh) must configure Aether URLs in pom.xml properties: aether.forge.url, aether.test.url, aether.prod.url.🛠️ Suggested update to the POM template
<properties> <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> <maven.compiler.release>25</maven.compiler.release> + <aether.forge.url>REPLACE_ME</aether.forge.url> + <aether.test.url>REPLACE_ME</aether.test.url> + <aether.prod.url>REPLACE_ME</aether.prod.url> <pragmatica-lite.version>{{pragmaticaVersion}}</pragmatica-lite.version> <aether.version>{{aetherVersion}}</aether.version> <jbct.version>{{jbctVersion}}</jbct.version> </properties>
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 9-10: Update the two entries in the Changed section so their
initial words use Sentence/Title-style capitalization to match the rest of the
section: change the entry containing "bump Aether to 0.8.1" so it starts with a
capitalized token (e.g., "Bump Aether to 0.8.1") and change the entry containing
"re-enabled slice-processor-tests module" so it starts capitalized (e.g.,
"Re-enabled slice-processor-tests module").
In `@docs/slice/quickstart.md`:
- Around line 7-9: Update the quickstart prerequisites to match the README by
specifying the JBCT CLI minimum version: change the third bullet from a generic
"JBCT CLI installed (`jbct --version` should work)" to explicitly require "JBCT
CLI 0.6.0+ installed (`jbct --version` should work)"; ensure the phrasing
mirrors the README to avoid version mismatch.
🧹 Nitpick comments (1)
jbct-cli/src/main/java/org/pragmatica/jbct/cli/InitCommand.java (1)
264-275: Consider adding_(underscore) to the keyword list.Since Java 9,
_(underscore alone) is a reserved keyword and cannot be used as an identifier. With Java 25 as the target, a groupId segment of just_would pass this validation but fail compilation.♻️ Proposed fix
private static boolean isJavaKeyword(String word) { return switch (word) { - case "abstract", "assert", "boolean", "break", "byte", "case", "catch", "char", + case "_", "abstract", "assert", "boolean", "break", "byte", "case", "catch", "char", "class", "const", "continue", "default", "do", "double", "else", "enum",
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/VerifySliceMojo.java (1)
168-171: Stale comment references old slice-api.properties approach.The comment on line 168 mentions
slice-api.propertiesbut the code now checks for.manifestfiles.📝 Fix stale comment
- // Check if this is a slice dependency (has slice-api.properties) + // Check if this is a slice dependency (has META-INF/slice/*.manifest) if (!isSliceArtifact(file)) {
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 33-38: The changelog has a duplicated "### Fixed" heading (MD024);
update the second occurrence surrounding the slice-init bullets (the lines
starting with "- Slice init: `ValidationError` now extends `Cause`", etc.) by
either merging those bullets into the earlier "### Fixed" section or renaming
this heading to a unique label (e.g., "### Fixed (Slice init)"), then save so
markdownlint MD024 no longer reports a duplicate heading.
In
`@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/CollectSliceDepsMojo.java`:
- Around line 93-100: The current parsing of baseArtifact (variable
baseArtifact) only checks for the presence of ":" so inputs like ":artifact" or
"group:" yield empty groupId; update the validation in CollectSliceDepsMojo to
ensure both group and artifact parts are non-empty before assigning groupId:
split baseArtifact on ":" into parts, verify parts.length >= 2 and
parts[0].trim() is not empty (and optionally parts[1].trim() not empty),
otherwise call getLog().warn with the jarFile.getName() and entryName and
continue; ensure you use the trimmed non-empty parts when setting groupId.
- Around line 21-25: Update the Javadoc in CollectSliceDepsMojo to document the
required base.artifact manifest field in addition to slice.interface and
slice.artifactId: state that base.artifact is required and explain its purpose
(used to derive groupId for the slice artifact), mention expected format/value
and that the code depends on it for groupId derivation, and include this new
field alongside the existing bullet points so template authors are not misled.
🧹 Nitpick comments (5)
slice-processor/src/main/java/org/pragmatica/jbct/slice/generator/ManifestGenerator.java (1)
162-164: Consider breaking up the long conditional for readability.This line is quite long and could benefit from extracting the type check conditions into separate methods or breaking across lines.
♻️ Suggested refactor
private boolean isStandardType(String typeName) { - return typeName.startsWith("java.lang.") || typeName.startsWith("java.util.") || typeName.equals("void") || typeName.equals("int") || typeName.equals("long") || typeName.equals("boolean") || typeName.equals("double") || typeName.equals("float"); + return typeName.startsWith("java.lang.") + || typeName.startsWith("java.util.") + || isPrimitiveType(typeName); + } + + private boolean isPrimitiveType(String typeName) { + return typeName.equals("void") + || typeName.equals("int") + || typeName.equals("long") + || typeName.equals("boolean") + || typeName.equals("double") + || typeName.equals("float"); }jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectValidator.java (1)
86-96: Consider refactoring thefold()usage to avoid side effects.Using
fold()that mutates an external list and returnsnullis not idiomatic for functional programming. Consider usingmapwith error collection instead.♻️ Suggested refactor
var errors = new ArrayList<String>(); for (var manifestFile : manifestFiles) { - loadProperties(manifestFile) - .fold(cause -> { - errors.add("Failed to read " + manifestFile.getFileName() + ": " + cause.message()); - return null; - }, props -> { - checkRequired(props, "slice.interface", errors); - checkRequired(props, "slice.artifactId", errors); - return null; - }); + loadProperties(manifestFile) + .onFailure(cause -> errors.add("Failed to read " + manifestFile.getFileName() + ": " + cause.message())) + .onSuccess(props -> { + checkRequired(props, "slice.interface", errors); + checkRequired(props, "slice.artifactId", errors); + }); }jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (2)
147-168: Consider extracting common JAR entry scanning logic.
isSliceDependencyandreadFirstSliceManifestboth open the JAR, iterate entries, and check for.manifestfiles underMETA-INF/slice/. Consider extracting a helper to reduce duplication.♻️ Suggested helper method
private <T> T findFirstSliceManifestEntry(Artifact artifact, java.util.function.BiFunction<JarFile, JarEntry, T> handler, T defaultValue) { var file = artifact.getFile(); if (file == null || !file.exists() || !file.getName().endsWith(".jar")) { return defaultValue; } try (var jar = new JarFile(file)) { var entries = jar.entries(); while (entries.hasMoreElements()) { var entry = entries.nextElement(); if (entry.getName().startsWith(SLICE_MANIFEST_DIR) && entry.getName().endsWith(".manifest")) { return handler.apply(jar, entry); } } return defaultValue; } catch (IOException e) { getLog().debug("Could not read JAR: " + file + " - " + e.getMessage()); return defaultValue; } }Then simplify:
private boolean isSliceDependency(Artifact artifact) { return findFirstSliceManifestEntry(artifact, (jar, entry) -> true, false); }Also applies to: 170-195
205-210: Edge case:baseArtifactwithout colon would cause array access issue.While
baseArtifact.contains(":")is checked, usingsplit(":")[0]is safe here. However, ifbaseArtifactis exactly":",split(":")[0]returns an empty string. This might produce an invalid groupId.♻️ More defensive check
- if (sliceArtifactId != null && baseArtifact != null && baseArtifact.contains(":")) { - var groupId = baseArtifact.split(":")[0]; + if (sliceArtifactId != null && baseArtifact != null) { + var parts = baseArtifact.split(":"); + if (parts.length >= 2 && !parts[0].isEmpty()) { + var groupId = parts[0]; return new ArtifactInfo(groupId, sliceArtifactId, toSemverRange(artifact.getVersion())); } + }jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/VerifySliceMojo.java (1)
185-201: Duplicate JAR scanning logic exists across multiple Mojo classes.This
isSliceArtifactimplementation is nearly identical toPackageSlicesMojo.isSliceDependency. Consider extracting to a shared utility class.
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/CollectSliceDepsMojo.java
Show resolved
Hide resolved
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/CollectSliceDepsMojo.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (1)
448-458: Fix groupId for slice-api and externalize hardcoded dependency versions.Line 455 uses groupId
org.pragmatica.aether, but should beorg.pragmatica-lite.aetherto match the check at line 144 and consistency with SliceProjectInitializer and parent pom.xml. The mismatch could cause artifacts to be filtered incorrectly.Additionally, core (
0.9.10) and slice-api (0.1.0) versions are hardcoded but the parent pom.xml definespragmatica-lite.version=0.11.1andaether.version=0.8.1. Use these properties or make versions configurable plugin parameters instead of hardcoding stale values.Code snippet
sb.append(" <dependency>\n"); sb.append(" <groupId>org.pragmatica-lite</groupId>\n"); sb.append(" <artifactId>core</artifactId>\n"); sb.append(" <version>0.9.10</version>\n"); sb.append(" </dependency>\n"); // And slice-api for runtime sb.append(" <dependency>\n"); sb.append(" <groupId>org.pragmatica.aether</groupId>\n"); sb.append(" <artifactId>slice-api</artifactId>\n"); sb.append(" <version>0.1.0</version>\n"); sb.append(" </dependency>\n");
🧹 Nitpick comments (4)
jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectValidator.java (1)
78-101: Consider narrowing exception type and using a more idiomatic side-effect pattern.Two minor observations:
Line 99: The
catch (Exception e)could be narrowed tocatch (IOException e)sinceFiles.list()only throwsIOException.Lines 87-96: Using
fold()with callbacks that returnnullworks but is unconventional. Thefold()method is typically used for transformations. If pragmatica-lite providesifSuccess/ifErrororonSuccess/onErrormethods, those would be more idiomatic for side-effect-only operations.♻️ Suggested refinement
- } catch (Exception e) { + } catch (IOException e) { return PartialResult.error("Failed to scan META-INF/slice/: " + e.getMessage()); }jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (3)
170-195: Consider caching or combining JAR scans to avoid redundant I/O.Both
isSliceDependency(line 147) andreadFirstSliceManifestopen and iterate through the same JAR file entries. For slice dependencies, this results in scanning the JAR twice: once to check if it's a slice dependency, and again to read the manifest properties.♻️ Suggested approach
Consider combining the check and read into a single method that returns an
Optional<Properties>, or cache the result ofreadFirstSliceManifestand use its presence to determine if the artifact is a slice dependency:- private boolean isSliceDependency(Artifact artifact) { - var file = artifact.getFile(); - ... - return false; - } - - private java.util.Properties readFirstSliceManifest(Artifact artifact) { + // Single method that returns Optional<Properties> + private java.util.Optional<java.util.Properties> readSliceManifest(Artifact artifact) { var file = artifact.getFile(); if (file == null || !file.exists() || !file.getName().endsWith(".jar")) { - return null; + return java.util.Optional.empty(); } try (var jar = new JarFile(file)) { var entries = jar.entries(); while (entries.hasMoreElements()) { var entry = entries.nextElement(); if (entry.getName().startsWith(SLICE_MANIFEST_DIR) && entry.getName().endsWith(".manifest")) { var props = new java.util.Properties(); try (var stream = jar.getInputStream(entry)) { props.load(stream); } - return props; + return java.util.Optional.of(props); } } - return null; + return java.util.Optional.empty(); } catch (IOException e) { getLog().debug("Could not read JAR: " + file + " - " + e.getMessage()); - return null; + return java.util.Optional.empty(); } }Then use
readSliceManifest(artifact).isPresent()for the check andreadSliceManifest(artifact).orElse(null)when properties are needed.
201-214: Minor formatting: unexpected space in array access.Line 208 has an unusual space between the method call and array access:
baseArtifact.split(":") [0]. While syntactically valid, this is unconventional and may cause confusion.✨ Suggested fix
- var groupId = baseArtifact.split(":") [0]; + var groupId = baseArtifact.split(":")[0];
333-339: Temp files accumulate until JVM exit.Each JAR entry from external dependencies creates a temp file with
deleteOnExit(). For builds with many external libraries, this could consume significant temporary disk space until the Maven JVM terminates.Consider using a single temp directory that can be cleaned up after archiver finishes, or track temp files for explicit deletion after
createArchive()completes:♻️ Alternative approach
// In createImplJar, create a temp directory and clean up after archive creation var tempDir = Files.createTempDirectory("jbct-bundle-"); try { bundleExternalLibs(archiver, classification.externalDeps(), tempDir); // ... create archive ... } finally { // Clean up temp directory try (var walk = Files.walk(tempDir)) { walk.sorted(java.util.Comparator.reverseOrder()) .forEach(p -> { try { Files.delete(p); } catch (IOException ignored) {} }); } }
Infrastructure dependencies (CacheService, DatabaseService, etc.) are now correctly accessed via InfraStore instead of being proxied through SliceInvokerFacade. This ensures shared singleton instances across all slices as specified in RFC-0007. Changes: - Added DependencyModel.isInfrastructure() to detect infra deps by package - Split dependency generation into infra (InfraStore) and slice (proxy) paths - Generate InfraStore.instance().flatMap() chain for infra dependencies - Only create proxy records for slice dependencies - Reduced flatMap depth: UrlShortener 13→3, Analytics 11→1 - Added imports: InfraStore, VersionedInstance, Causes, Option - Fixed closing parentheses and toResult() parameter bugs Performance impact: - 77% reduction in async chain depth for mixed dependencies - Eliminates proxy overhead for infrastructure services - Correct singleton semantics (all slices share infra instances) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
SliceManifest was reading 'impl.artifactId' but manifests contain 'slice.artifactId' (per RFC-0004). This caused: - Empty JAR names: -1.0.0-SNAPSHOT.jar - JAR overwriting: all slices created same-named JAR Fixed by reading correct property name. Now generates: - url-shortener-analytics-1.0.0-SNAPSHOT.jar - url-shortener-url-shortener-1.0.0-SNAPSHOT.jar Each slice gets unique JAR with correct artifact ID. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Fix isAetherRuntime() filtering - was too broad, skipped all infra deps - Now only skips slice-annotations, slice-api, and slice-processor - Allow infra-* and core to be included in dependency file sections - Filter infrastructure deps from blueprint (resolved via InfraStore) - Fix local dependency resolution with package context - Analytics now resolves to url-shortener-analytics (not urlshortener) Fixes empty dependency files (0 bytes) and UNRESOLVED artifacts in blueprint Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (1)
459-463: Fix incorrect groupId for slice-api dependency in generated POM.The generated POM at line 460 uses
org.pragmatica.aetheras the groupId forslice-api, butisAetherRuntime()at line 145 filters artifacts with groupIdorg.pragmatica-lite.aether. This mismatch preventsslice-apifrom being correctly filtered, potentially causing it to be incorrectly bundled. All other references in the codebase (SliceProjectInitializer, documentation, parent POM) useorg.pragmatica-lite.aether.Suggested fix
// And slice-api for runtime sb.append(" <dependency>\n"); - sb.append(" <groupId>org.pragmatica.aether</groupId>\n"); + sb.append(" <groupId>org.pragmatica-lite.aether</groupId>\n"); sb.append(" <artifactId>slice-api</artifactId>\n"); sb.append(" <version>0.1.0</version>\n"); sb.append(" </dependency>\n");
🧹 Nitpick comments (6)
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (3)
175-200: Consider extracting shared JAR scanning logic.Both
isSliceDependency(lines 152-173) andreadFirstSliceManifestiterate over JAR entries searching for manifests underMETA-INF/slice/*.manifest. This duplicated traversal could be consolidated into a helper method.♻️ Suggested refactor
private JarEntry findFirstSliceManifestEntry(JarFile jar) { var entries = jar.entries(); while (entries.hasMoreElements()) { var entry = entries.nextElement(); if (entry.getName().startsWith(SLICE_MANIFEST_DIR) && entry.getName().endsWith(".manifest")) { return entry; } } return null; }Then both methods can use this helper to avoid duplicating the search logic.
453-463: Consider externalizing hardcoded dependency versions.The POM generation hardcodes
coreversion0.9.10andslice-apiversion0.1.0. Given that PR objectives mention updated versions (Pragmatica Lite 0.11.1, Aether 0.8.1), these hardcoded values may become stale.Consider reading versions from plugin configuration or project properties for easier maintenance.
338-344: Temp files accumulate until JVM shutdown.
deleteOnExit()schedules deletion only on JVM termination. For builds with many external dependencies, numerous temp files could accumulate during the build. Consider tracking and explicitly deleting these files aftermavenArchiver.createArchive()completes, or using a single temp directory that can be cleaned up at once.slice-processor/src/main/java/org/pragmatica/jbct/slice/generator/DependencyVersionResolver.java (1)
105-121:toKebabCasehandles consecutive uppercase letters as individual words.Input like
"URLShortener"becomes"u-r-l-shortener"rather than"url-shortener". If slice names could contain acronyms, this may produce unexpected artifact names.If slice naming conventions prohibit acronyms or this edge case is acceptable, no change needed.
slice-processor/src/main/java/org/pragmatica/jbct/slice/generator/ManifestGenerator.java (2)
180-197: DuplicatetoKebabCaseimplementation.This method duplicates the one in
DependencyVersionResolver(lines 105-121). Both produce the same output but have slightly different implementations. Consider extracting to a shared utility class to avoid inconsistencies and reduce maintenance burden.♻️ Suggested extraction
Create a shared utility:
// In a new file or existing utility class public final class SliceNaming { private SliceNaming() {} public static String toKebabCase(String pascalCase) { if (pascalCase == null || pascalCase.isEmpty()) { return pascalCase; } var result = new StringBuilder(); for (int i = 0; i < pascalCase.length(); i++) { char c = pascalCase.charAt(i); if (Character.isUpperCase(c)) { if (i > 0) { result.append('-'); } result.append(Character.toLowerCase(c)); } else { result.append(c); } } return result.toString(); } }Then use
SliceNaming.toKebabCase()in both classes.
168-170: Long line inisStandardTypecould be split for readability.The boolean expression spans a single long line. Consider breaking it across multiple lines for improved readability.
♻️ Suggested formatting
private boolean isStandardType(String typeName) { - return typeName.startsWith("java.lang.") || typeName.startsWith("java.util.") || typeName.equals("void") || typeName.equals("int") || typeName.equals("long") || typeName.equals("boolean") || typeName.equals("double") || typeName.equals("float"); + return typeName.startsWith("java.lang.") + || typeName.startsWith("java.util.") + || typeName.equals("void") + || typeName.equals("int") + || typeName.equals("long") + || typeName.equals("boolean") + || typeName.equals("double") + || typeName.equals("float"); }
Skip UNRESOLVED local dependencies when they're already in graph with correct version. Prevents duplicate entries like: - url-shortener-analytics:UNRESOLVED (from transitive dep) - url-shortener-analytics:1.0.0 (from local manifest) Blueprint now correctly shows each slice once with proper version. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/GenerateBlueprintMojo.java`:
- Around line 129-143: In GenerateBlueprintMojo, the DFS traversal must use the
same resolved graph key as insertion: when you encounter a dependency with
dep.version() == "UNRESOLVED" (constructed previously as dep.artifact() + ":" +
dep.version() when scanning), map/normalize that UNRESOLVED entry to the actual
resolved graph key already present in graph (i.e., find the existing graph key
that startsWith dep.artifact() + ":" and use that key) before checking
graph.containsKey() or recursing in the DFS; update the code paths that build
lookup keys during traversal so they resolve UNRESOLVED -> resolvedKey
consistently (refer to dep.artifact(), dep.version(), graph.containsKey(), and
the DFS traversal in GenerateBlueprintMojo).
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/GenerateBlueprintMojo.java
Show resolved
Hide resolved
When multiple @slice interfaces exist in same Maven module, their dependencies are now included in the [slices] section for: 1. Correct deployment ordering in blueprint 2. Runtime dependency resolution Example: UrlShortener depends on Analytics (same module) Dependency file now includes: [slices] org.pragmatica.aether.example:url-shortener-analytics:^1.0.0 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 29: Remove the duplicate "### Changed" heading by merging its entries
into the existing "### Changed" section: locate the second "### Changed" heading
(the one in the diff) and move its list items under the first "### Changed"
header (the existing section), then delete the duplicate header so there is a
single "### Changed" heading in CHANGELOG.md.
In
`@jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectInitializer.java`:
- Around line 627-628: In SliceProjectInitializer where you build the request
via {{sliceName}}.Request.request("test"), replace the non-existent
Result.unwrap() call with Result.getOrThrow(): update the expression that
currently ends with .unwrap() to use .getOrThrow() so the pragmatica-lite Result
type is accessed correctly (look for the Request.request("test") call site to
make the change).
🧹 Nitpick comments (3)
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (2)
141-160: Guard against duplicate local slice dependencies.If a local slice is also present as a direct dependency, this method can add duplicates, leading to repeated lines in the dependency file and ambiguous version map entries. Consider deduping by
groupId:artifactIdbefore appending.♻️ Possible dedupe tweak
private void addLocalSliceDependencies(SliceManifest manifest, List<ArtifactInfo> sliceDeps) { + var existing = new java.util.HashSet<String>(); + for (var depInfo : sliceDeps) { + existing.add(depInfo.groupId() + ":" + depInfo.artifactId()); + } // Check manifest dependencies for local slices (same module) for (var dep : manifest.dependencies()) { // Local slice dependencies have artifact coordinates but may be UNRESOLVED if (dep.artifact() == null || dep.artifact() .isEmpty()) { continue; } // Check if this is a local slice (same groupId and base artifactId) var depArtifact = dep.artifact(); if (depArtifact.startsWith(project.getGroupId() + ":" + project.getArtifactId() + "-")) { // Extract slice artifact ID and create version range var version = "^" + project.getVersion(); - sliceDeps.add(new ArtifactInfo(project.getGroupId(), - depArtifact.substring(project.getGroupId() - .length() + 1), - version)); + var artifactId = depArtifact.substring(project.getGroupId().length() + 1); + var key = project.getGroupId() + ":" + artifactId; + if (existing.add(key)) { + sliceDeps.add(new ArtifactInfo(project.getGroupId(), artifactId, version)); + } getLog().debug("Added local slice dependency: " + depArtifact + ":" + version); } } }
538-563: Consider cleaning up transformed temp class files.Repeated builds can accumulate temp files. Marking them for deletion on JVM exit is a cheap cleanup.
🧹 Temp file cleanup
var tempClass = Files.createTempFile("factory-", ".class"); Files.write(tempClass, transformedBytes); +tempClass.toFile().deleteOnExit(); archiver.addFile(tempClass.toFile(), relativePath);jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectInitializer.java (1)
391-402: Tinylog version 2.7.0 is current stable. Consider parameterizing the version like other dependency versions (e.g.,${tinylog.version}) for easier maintenance and updates, though the codebase shows mixed patterns with some dependencies hardcoded and others parameterized.
| var request = {{sliceName}}.Request.request("test") | ||
| .unwrap(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pragmatica-lite Result unwrap getOrThrow API documentation
💡 Result:
For Pragmatica Lite (org.pragmatica-lite:core), the Result.getOrThrow() API is documented in the core Javadoc artifact (and it was introduced in core 0.8.6). (pragmatica.dev)
Where to get the official API docs
- Maven Central directory for core 0.11.1 includes
core-0.11.1-javadoc.jar(download it and openindex.htmlinside): (repo.maven.apache.org) - If you need the version where it was introduced: the Pragmatica changelog notes
Result/Optiongot.getOrThrow()in the 0.8.6 line. (pragmatica.dev)
About unwrap
I couldn’t find an official Pragmatica Lite Result.unwrap API reference in the published docs I checked; Pragmatica Lite appears to standardize on getOrThrow() instead. (repo.maven.apache.org)
Citations:
- 1: https://pragmatica.dev/changelog?utm_source=openai
- 2: https://repo.maven.apache.org/maven2/org/pragmatica-lite/core/0.11.1/
- 3: https://pragmatica.dev/changelog?utm_source=openai
- 4: https://repo.maven.apache.org/maven2/org/pragmatica-lite/core/0.11.1/
Replace .unwrap() with .getOrThrow()
The unwrap() method does not exist on pragmatica-lite's Result type. The correct method is getOrThrow().
🤖 Prompt for AI Agents
In
`@jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectInitializer.java`
around lines 627 - 628, In SliceProjectInitializer where you build the request
via {{sliceName}}.Request.request("test"), replace the non-existent
Result.unwrap() call with Result.getOrThrow(): update the expression that
currently ends with .unwrap() to use .getOrThrow() so the pragmatica-lite Result
type is accessed correctly (look for the Request.request("test") call site to
make the change).
…solver - Extract nested lambdas in PackageSlicesMojo.transformFactoryBytecode() into separate methods - Replace fold() with or() in GitHubVersionResolver for cleaner fallback handling - Replace fold() with recover() for error handling with default values - Extract timeout magic numbers to named constants (API_TIMEOUT)
- Test buildVersionMap() with slices dependencies, empty content, null content, no slices section - Test stripSemverPrefix() for caret, tilde, and no prefix cases - All 7 tests passing
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@jbct-core/src/main/java/org/pragmatica/jbct/init/GitHubVersionResolver.java`:
- Around line 114-118: The recovery lambda in GitHubVersionResolver modifying
var cmp (inside comparison.recover in method that compares v1/v2 components)
logs "using v1" but returns 0 (equal), which is inconsistent; update the
recovery to either (A) return a positive value (e.g., 1) if the intended
behavior is to prefer v1 on parse failure, or (B) change the LOG.debug message
to accurately state that the components are being treated as equal on parse
failure; modify the comparison.recover lambda accordingly so the log text and
the returned integer are consistent with the chosen behavior.
🧹 Nitpick comments (5)
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (5)
147-167: Chain alignment issue on lines 151-152.The
.isEmpty()call should align to the receiver end per coding guidelines.♻️ Suggested fix for chain alignment
- if (dep.artifact() == null || dep.artifact() - .isEmpty()) { + if (dep.artifact() == null || dep.artifact().isEmpty()) {As per coding guidelines: "Chain alignment:
.aligns to the receiver end".
189-210: Chain alignment could be improved.Lines 191-192 and 199-201 have chain breaks that could follow the alignment convention more closely. However, this is a minor style preference.
♻️ Suggested alignment improvements
private boolean isSliceDependency(Artifact artifact) { var file = artifact.getFile(); - if (file == null || !file.exists() || !file.getName() - .endsWith(".jar")) { + if (file == null || !file.exists() || !file.getName().endsWith(".jar")) { return false; } try (var jar = new JarFile(file)) { var entries = jar.entries(); while (entries.hasMoreElements()) { var entry = entries.nextElement(); - if (entry.getName() - .startsWith(SLICE_MANIFEST_DIR) && entry.getName() - .endsWith(".manifest")) { + if (entry.getName().startsWith(SLICE_MANIFEST_DIR) + && entry.getName().endsWith(".manifest")) { return true; } }As per coding guidelines: "Chain alignment:
.aligns to the receiver end".
250-250: Minor formatting: extraneous space before array access.Line 250 has
split(":") [0]with a space before the bracket.♻️ Suggested fix
- var groupId = baseArtifact.split(":") [0]; + var groupId = baseArtifact.split(":")[0];
270-270: Minor formatting: missing space aftertry.Standard Java formatting uses
try {with a space.♻️ Suggested fix
- try{ + try {
552-578: Consider consistent temp file cleanup.Line 565-566 creates temp files without calling
deleteOnExit(), unlike line 380-381 inbundleExternalLibs. While Maven plugin execution is typically short-lived, consistent cleanup patterns improve robustness.♻️ Suggested fix for consistent cleanup
var tempClass = Files.createTempFile("factory-", ".class"); + tempClass.toFile().deleteOnExit(); Files.write(tempClass, transformedBytes);Also, same
try{formatting issue as line 270:- try{ + try {
jbct-core/src/main/java/org/pragmatica/jbct/init/GitHubVersionResolver.java
Outdated
Show resolved
Hide resolved
Replace flatMap chain + recover().unwrap() with cleaner Result.all() pattern: - Aggregates both parseInt operations - Maps to Integer.compare() - Uses .or(0) for fallback value More idiomatic JBCT pattern.
- ManifestGenerator: accept optional Routes class in impl.classes - SliceProcessor: sequential generation to pass Routes class to manifest - PackageSlicesMojo: filter service file per-slice for multi-slice modules Fixes ServiceLoader discovery of SliceRouterFactory implementations
- Removed unused properties: timeoutMs, memoryMb, loadBalancing, affinityKey - Aether runtime only consumes artifact + instances (per SliceSpec) - Changed default from 1 to 3 instances - Updated docs to reflect actual implementation
Generated slice wrapper record now implements both Slice (for Aether runtime) and the slice interface (e.g., UrlShortener) with delegate methods. This fixes router type checking where sliceType().isInstance() was returning false. Also bumps pragmatica-lite to 0.11.2 and documents dependency cycle workaround in build commands.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (1)
651-655:⚠️ Potential issue | 🔴 CriticalFix incorrect groupId for slice-api in generated POM: use
org.pragmatica-lite.aetherinstead oforg.pragmatica.aether(line 652).Line 182 of the same file correctly identifies Aether runtime artifacts by checking for
org.pragmatica-lite.aether, and all project templates across the codebase use this groupId for slice-api. The generated POM at line 652 must match this convention to ensure proper dependency resolution.Generated POM snippet (lines 651-655)
sb.append(" <dependency>\n"); sb.append(" <groupId>org.pragmatica.aether</groupId>\n"); sb.append(" <artifactId>slice-api</artifactId>\n"); sb.append(" <version>0.1.0</version>\n"); sb.append(" </dependency>\n");
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- Around line 109-116: The markdown table's separator row is not properly
aligned with the header and violates MD060; fix it by rewriting the separator
row so each column has a matching pipe and a sequence of dashes (e.g.,
|---|---|---|) that aligns with the header columns for "Category", "Weight", and
"Rules Included" and ensure the pipes in the header, separator, and data rows
line up vertically; update the separator row in the existing table block (the
header line starting with "| Category | Weight | Rules Included |" and the
subsequent separator line) to use consistent pipes and dash counts so
markdownlint MD060 passes.
In `@jbct-core/src/main/java/org/pragmatica/jbct/slice/SliceConfig.java`:
- Around line 43-45: Replace the incorrect Supplier usage in the toml default
for instances: change the call to toml.getInt("blueprint", "instances").or(() ->
3) to use a direct value default toml.getInt("blueprint", "instances").or(3);
update the variable named instances accordingly so
BlueprintConfig.blueprintConfig(instances) receives the resolved int; ensure
this matches other usages like RouteConfigLoader and compiles against the
toml.getInt API that accepts a value, not a Supplier.
In
`@slice-processor/src/main/java/org/pragmatica/jbct/slice/generator/FactoryClassGenerator.java`:
- Around line 261-269: The infra flatMap chain in generateAspectCreateChain
currently calls generateInfraStoreCall(infra) without binding each infra result
into a named variable, so the final .map() lambda cannot reference the factory
parameter names (factoryArgs); update the chain to mirror
generateMixedDependencyChain by flatMapping with a lambda that binds the infra
result to a distinct variable (e.g., infraX or a name derived from
infra.parameterName()) and passing that variable through the promise chain
(threading it alongside existing cacheVarNames), or alternatively ensure the
final .map() only uses the variables actually passed (cacheVarNames and slice
handles); adjust the flatMap lines that iterate infraDeps and the construction
of factoryArgs to use those bound variable names so they are in scope in the
final .map().
🧹 Nitpick comments (8)
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (4)
250-250: Minor: Unexpected space before array index.There's a space between
split(":")and[0]which is inconsistent with typical Java style.🔧 Suggested fix
- var groupId = baseArtifact.split(":") [0]; + var groupId = baseArtifact.split(":")[0];
270-270: Minor: Missing space aftertrykeyword.Standard Java formatting convention places a space between
tryand the opening brace.🔧 Suggested fix
- try{ + try {
460-462: Consider addingdeleteOnExit()for temp file consistency.Other temp files in this class (e.g., line 381-383, line 592) call
deleteOnExit()to ensure cleanup. This temp file should follow the same pattern for consistency.🔧 Suggested fix
if (!filteredLines.isEmpty()) { var tempService = Files.createTempFile("service-", ".txt"); + tempService.toFile().deleteOnExit(); Files.writeString(tempService, String.join("\n", filteredLines));
583-583: Minor: Missing space aftertrykeyword.Consistent with the earlier observation at line 270.
🔧 Suggested fix
- try{ + try {CLAUDE.md (1)
317-325: Consider noting repo-root context (or parent POM install) for per-module installs.This helps users avoid running the commands from a submodule without the parent installed.
✍️ Doc clarification suggestion
### Installing to Local Repository -Due to the dependency cycle, install modules individually: +Run these commands from the repo root (or install the parent POM with `mvn install -N -Djbct.skip=true`) to ensure the parent is available. +Due to the dependency cycle, install modules individually:docs/slice/deployment.md (1)
78-82: Table alignment issue flagged by linter.The markdownlint tool reports table column style issues (MD060) on line 80. The table pipes should align with the header for consistent formatting.
🔧 Suggested fix
| Property | Type | Default | Description | |----------|------|---------|-------------| -| `instances` | int | `3` | Number of slice instances | +| `instances` | int | `3` | Number of slice instances |slice-processor/src/main/java/org/pragmatica/jbct/slice/SliceProcessor.java (1)
221-237: Consider extractingtoKebabCaseto a shared utility.This helper method is duplicated in
ManifestGenerator. Consider extracting it to a shared utility class to adhere to DRY principles.slice-processor/src/main/java/org/pragmatica/jbct/slice/generator/ManifestGenerator.java (1)
179-181: Consider breaking up the long condition for readability.Line 180 contains multiple
||conditions on a single line. Splitting across multiple lines would improve readability.♻️ Suggested refactor
private boolean isStandardType(String typeName) { - return typeName.startsWith("java.lang.") || typeName.startsWith("java.util.") || typeName.equals("void") || typeName.equals("int") || typeName.equals("long") || typeName.equals("boolean") || typeName.equals("double") || typeName.equals("float"); + return typeName.startsWith("java.lang.") + || typeName.startsWith("java.util.") + || typeName.equals("void") + || typeName.equals("int") + || typeName.equals("long") + || typeName.equals("boolean") + || typeName.equals("double") + || typeName.equals("float"); }
slice-processor/src/main/java/org/pragmatica/jbct/slice/generator/FactoryClassGenerator.java
Outdated
Show resolved
Hide resolved
- RouteSourceGenerator: use Elements#getTypeElement instead of Class.forName - FactoryClassGenerator: use lowercaseFirst() for factory names, fix infra chain threading - CollectSliceDepsMojo: tighten base.artifact validation, document in Javadoc, scan Maven resources - GenerateBlueprintMojo: fix UNRESOLVED dependency edge resolution - SliceConfig: fix or(() -> 3) to or(3) - SliceProjectInitializer: rewrite test template without .unwrap() - Docs: fix CHANGELOG headings, add JBCT 0.6.0+ to quickstart, add text lang to code block
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectInitializer.java (1)
710-717:⚠️ Potential issue | 🟡 MinorUpdate quickstart documentation to show
instances = 3.The blueprint generator correctly reads
instancesfrom the slice configuration. However, the quickstart documentation (line 114) shows the generated blueprint withinstances = 1, which conflicts with the template default ofinstances = 3inSLICE_CONFIG_TEMPLATE. Update the example output in the quickstart to reflect the actual default from the template.
🤖 Fix all issues with AI agents
In `@slice-processor/src/main/java/org/pragmatica/jbct/slice/SliceProcessor.java`:
- Around line 186-190: The note() call inside the onSuccess handler is executed
even when qualifiedNameOpt is none, producing a misleading "Generated routes"
message; update the logic so the log is emitted only when a route was actually
produced—e.g., move the note() invocation into the
qualifiedNameOpt.onPresent(...) callback (or check qualifiedNameOpt.isPresent()
before calling note()), referencing qualifiedNameOpt, routeServiceEntries,
note(...), interfaceElement and sliceModel.simpleName() so the message is logged
only when a route entry is added.
- Around line 79-82: The code currently returns false from process() when
validateOneSlicePerPackage(interfaceElement) fails, which wrongly signals the
annotation isn’t claimed; instead, report the error via the Messager inside
validateOneSlicePerPackage (or immediately after it fails) and ensure process()
returns true to claim the annotations; update the branch around
validateOneSlicePerPackage in SliceProcessor.process() to emit a diagnostic
(using messager.printMessage or similar) with the validation failure details and
then return true so compilation fails due to the error but the annotations
remain claimed.
🧹 Nitpick comments (6)
CLAUDE.md (2)
93-93: Add language specifiers to formula code blocks.The formula code blocks at lines 93 and 103 are missing language specifiers, violating markdownlint rule MD040. Add
textlanguage tags to these blocks.📝 Proposed fix for formula blocks
Line 93:
-``` +```text weighted_violations = Σ(count[severity] × multiplier[severity])Line 103:
-``` +```text overall_score = Σ(category_score[i] × weight[i])Also applies to: 103-103
121-121: Add language specifier to terminal output code block.The terminal output ASCII art block is missing a language specifier (MD040). Add
textto satisfy the linter.📝 Proposed fix
-``` +```text ╔═══════════════════════════════════════════════════╗slice-processor/src/main/java/org/pragmatica/jbct/slice/routing/RouteSourceGenerator.java (1)
138-143: Theelementsfield is assigned but never used.The
Elementsfield was added (addressing prior review feedback aboutClass.forName()), but thevalidateHttpRoutingDependency()method that would have used it appears to have been removed from this class. The field is now stored but never referenced.Either remove the unused field and constructor parameter, or verify whether the dependency validation logic was intentionally moved elsewhere.
🔍 Verify if dependency validation moved elsewhere
#!/bin/bash # Search for validateHttpRoutingDependency or SliceRouterFactory validation in the codebase rg -n "validateHttpRoutingDependency|SliceRouterFactory" --type java -C 2slice-processor/src/main/java/org/pragmatica/jbct/slice/SliceProcessor.java (1)
47-48: Consider using imports instead of inline qualified types.The fields use fully qualified
java.util.Map,java.util.Set,java.util.HashMap, andjava.util.LinkedHashSetinstead of imports. While functional, this is inconsistent with the rest of the file's style.♻️ Suggested refactor
Add imports:
import java.util.HashMap; import java.util.LinkedHashSet; import java.util.Map;Then simplify fields:
- private final java.util.Map<String, TypeElement> packageToSlice = new java.util.HashMap<>(); - private final java.util.Set<String> routeServiceEntries = new java.util.LinkedHashSet<>(); + private final Map<String, TypeElement> packageToSlice = new HashMap<>(); + private final Set<String> routeServiceEntries = new LinkedHashSet<>();jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectInitializer.java (2)
627-634: Consider a flatter test structure for readability.The nested callback pattern works but has 4 levels of nesting which can be hard to follow. For test templates, a more imperative style might be clearer for users:
♻️ Optional: Flatter test structure
`@Test` void should_process_request() { - {{sliceName}}.Request.request("test") - .onFailure(Assertions::fail) - .onSuccess(request -> slice.process(request) - .await() - .onFailure(Assertions::fail) - .onSuccess(r -> assertThat(r.result()).isEqualTo("Processed: test"))); + var request = {{sliceName}}.Request.request("test") + .getOrThrow(); + var response = slice.process(request) + .await() + .getOrThrow(); + assertThat(response.result()).isEqualTo("Processed: test"); }
378-402: Consider parameterizing hardcoded dependency versions in the template.The template currently hardcodes specific versions for test dependencies (junit-jupiter 5.11.0, assertj-core 3.26.3, tinylog-api/impl 2.7.0). While these are reasonably current stable releases, a generated template with fixed versions will inevitably become stale as the ecosystem evolves. Consider making these parameterizable similar to
${pragmatica-lite.version}to allow template consumers to update baseline versions without modifying the template source.
| // Enforce one-slice-per-package convention | ||
| if (!validateOneSlicePerPackage(interfaceElement)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Returning false from process() has different semantics than error reporting.
Returning false from process() indicates the processor does not claim the annotation types, which may allow other processors to attempt processing them. If the intent is to halt processing due to an error, the processor should still return true (claiming the annotations) and rely on the error emitted via messager to fail compilation.
✏️ Suggested fix
// Enforce one-slice-per-package convention
- if (!validateOneSlicePerPackage(interfaceElement)) {
- return false;
- }
+ if (!validateOneSlicePerPackage(interfaceElement)) {
+ continue; // Skip this interface, error already reported
+ }
processSliceInterface(interfaceElement);📝 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.
| // Enforce one-slice-per-package convention | |
| if (!validateOneSlicePerPackage(interfaceElement)) { | |
| return false; | |
| } | |
| // Enforce one-slice-per-package convention | |
| if (!validateOneSlicePerPackage(interfaceElement)) { | |
| continue; // Skip this interface, error already reported | |
| } |
🤖 Prompt for AI Agents
In `@slice-processor/src/main/java/org/pragmatica/jbct/slice/SliceProcessor.java`
around lines 79 - 82, The code currently returns false from process() when
validateOneSlicePerPackage(interfaceElement) fails, which wrongly signals the
annotation isn’t claimed; instead, report the error via the Messager inside
validateOneSlicePerPackage (or immediately after it fails) and ensure process()
returns true to claim the annotations; update the branch around
validateOneSlicePerPackage in SliceProcessor.process() to emit a diagnostic
(using messager.printMessage or similar) with the validation failure details and
then return true so compilation fails due to the error but the annotations
remain claimed.
| .onSuccess(qualifiedNameOpt -> { | ||
| qualifiedNameOpt.onPresent(routeServiceEntries::add); | ||
| note(interfaceElement, | ||
| "Generated routes: " + sliceModel.simpleName() + "Routes"); | ||
| }); |
There was a problem hiding this comment.
Log message may be misleading when no routes are generated.
The note() call logs "Generated routes: ..." unconditionally in onSuccess, but onSuccess is triggered even when qualifiedNameOpt is Option.none() (no routes exist). This could log a misleading message when routes were not actually generated.
✏️ Suggested fix
.onSuccess(qualifiedNameOpt -> {
- qualifiedNameOpt.onPresent(routeServiceEntries::add);
- note(interfaceElement,
- "Generated routes: " + sliceModel.simpleName() + "Routes");
+ qualifiedNameOpt.onPresent(name -> {
+ routeServiceEntries.add(name);
+ note(interfaceElement,
+ "Generated routes: " + sliceModel.simpleName() + "Routes");
+ });
});📝 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.
| .onSuccess(qualifiedNameOpt -> { | |
| qualifiedNameOpt.onPresent(routeServiceEntries::add); | |
| note(interfaceElement, | |
| "Generated routes: " + sliceModel.simpleName() + "Routes"); | |
| }); | |
| .onSuccess(qualifiedNameOpt -> { | |
| qualifiedNameOpt.onPresent(name -> { | |
| routeServiceEntries.add(name); | |
| note(interfaceElement, | |
| "Generated routes: " + sliceModel.simpleName() + "Routes"); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In `@slice-processor/src/main/java/org/pragmatica/jbct/slice/SliceProcessor.java`
around lines 186 - 190, The note() call inside the onSuccess handler is executed
even when qualifiedNameOpt is none, producing a misleading "Generated routes"
message; update the logic so the log is emitted only when a route was actually
produced—e.g., move the note() invocation into the
qualifiedNameOpt.onPresent(...) callback (or check qualifiedNameOpt.isPresent()
before calling note()), referencing qualifiedNameOpt, routeServiceEntries,
note(...), interfaceElement and sliceModel.simpleName() so the message is logged
only when a route entry is added.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@jbct-cli/src/main/java/org/pragmatica/jbct/cli/JbctCommand.java`:
- Line 18: JbctCommand.java references a non-existent ScoreCommand class causing
compilation to fail; either add the missing ScoreCommand implementation or
remove/replace its reference in JbctCommand (the list/registration where
ScoreCommand.class appears) until the class is provided. Locate the reference to
ScoreCommand.class in JbctCommand and either implement a matching public class
ScoreCommand with the expected command API, or remove the ScoreCommand.class
entry and adjust any dependent wiring (command registration or imports) so the
code compiles.
🧹 Nitpick comments (1)
README.md (1)
109-141: Excellent documentation for the new Score feature!The Score section is comprehensive and well-structured:
- Clear usage examples with practical baseline scenarios
- Three output formats (terminal, json, badge) properly documented
- Scoring formula categories add up to 100% correctly
- Categories align well with JBCT principles
Optional clarification: The phrase "density + severity weighting" (line 134) might benefit from a brief explanation of what density and severity mean in this context, but the category breakdown is clear enough for users to understand the scoring system.
| subcommands = {FormatCommand.class, | ||
| LintCommand.class, | ||
| CheckCommand.class, | ||
| ScoreCommand.class, |
There was a problem hiding this comment.
Compilation failure: ScoreCommand class does not exist.
The CI pipeline fails with cannot find symbol: class ScoreCommand. The ScoreCommand.java file appears to be missing from this PR. Either add the implementation or remove this reference until the class is available.
🧰 Tools
🪛 GitHub Actions: CI
[error] 18-18: Compilation error: cannot find symbol. symbol: class ScoreCommand
🤖 Prompt for AI Agents
In `@jbct-cli/src/main/java/org/pragmatica/jbct/cli/JbctCommand.java` at line 18,
JbctCommand.java references a non-existent ScoreCommand class causing
compilation to fail; either add the missing ScoreCommand implementation or
remove/replace its reference in JbctCommand (the list/registration where
ScoreCommand.class appears) until the class is provided. Locate the reference to
ScoreCommand.class in JbctCommand and either implement a matching public class
ScoreCommand with the expected command API, or remove the ScoreCommand.class
entry and adjust any dependent wiring (command registration or imports) so the
code compiles.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@slice-processor/src/main/java/org/pragmatica/jbct/slice/routing/RouteSourceGenerator.java`:
- Line 12: Remove the dead Elements usage from RouteSourceGenerator: delete the
private Elements elements field, remove the javax.lang.model.util.Elements
import, and remove the corresponding Elements parameter from the
RouteSourceGenerator constructor along with any assignment to this.elements;
there is no reference to elements in this class (the
validateHttpRoutingDependency() logic lives in CollectSliceDepsMojo), so simply
eliminate the unused field and constructor parameter to clean up the class.
🧹 Nitpick comments (1)
slice-processor/src/main/java/org/pragmatica/jbct/slice/routing/RouteSourceGenerator.java (1)
290-298: Consider reusing the already-validated method to avoid redundant lookup.The method is looked up twice: once during validation (line 270) and again during generation (line 296). Since
validRoutesalready confirmed the method exists, the second lookup is redundant.♻️ Suggested refactor to avoid double lookup
Store the method alongside the entry during validation:
- var validRoutes = new ArrayList<Map.Entry<String, RouteDsl>>(); + record ValidRoute(String handlerName, RouteDsl routeDsl, MethodModel method) {} + var validRoutes = new ArrayList<ValidRoute>(); for (var entry : routeEntries) { var handlerName = entry.getKey(); var routeDsl = entry.getValue(); var methodOpt = Option.option(methodMap.get(handlerName)); if (methodOpt.isEmpty()) { // ... error handling continue; } // ... param count check - validRoutes.add(entry); + validRoutes.add(new ValidRoute(handlerName, routeDsl, methodOpt.get())); } for (int i = 0; i < validRoutes.size(); i++) { - var entry = validRoutes.get(i); - var handlerName = entry.getKey(); - var routeDsl = entry.getValue(); + var route = validRoutes.get(i); var hasMore = i < validRoutes.size() - 1; - Option.option(methodMap.get(handlerName)) - .onPresent(method -> generateRoute(out, routeConfig.prefix(), routeDsl, method, hasMore)); + generateRoute(out, routeConfig.prefix(), route.routeDsl(), route.method(), hasMore); }
|
|
||
| import javax.annotation.processing.Filer; | ||
| import javax.annotation.processing.Messager; | ||
| import javax.lang.model.util.Elements; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if elements field is used anywhere in this file or if validateHttpRoutingDependency exists elsewhere
echo "=== Searching for 'elements' usage in RouteSourceGenerator ==="
rg -n "elements\." slice-processor/src/main/java/org/pragmatica/jbct/slice/routing/RouteSourceGenerator.java
echo -e "\n=== Searching for validateHttpRoutingDependency in codebase ==="
rg -n "validateHttpRoutingDependency" --type java
echo -e "\n=== Searching for Class.forName in slice-processor ==="
rg -n "Class\.forName" slice-processor/src/main/javaRepository: siy/jbct-cli
Length of output: 536
Remove the unused elements field and constructor parameter.
The Elements field (line 12) and its corresponding constructor parameter (lines 140-143) are never used in this class. The validateHttpRoutingDependency() method that would require this field exists in a different class (CollectSliceDepsMojo), not in RouteSourceGenerator. This is dead code and should be removed.
Also applies to: 138-138, 140-143
🤖 Prompt for AI Agents
In
`@slice-processor/src/main/java/org/pragmatica/jbct/slice/routing/RouteSourceGenerator.java`
at line 12, Remove the dead Elements usage from RouteSourceGenerator: delete the
private Elements elements field, remove the javax.lang.model.util.Elements
import, and remove the corresponding Elements parameter from the
RouteSourceGenerator constructor along with any assignment to this.elements;
there is no reference to elements in this class (the
validateHttpRoutingDependency() logic lives in CollectSliceDepsMojo), so simply
eliminate the unused field and constructor parameter to clean up the class.
- SliceProcessor: use ConcurrentHashMap and synchronizedSet for thread safety - PackageSlicesMojo: return Optional<Properties> instead of null - JbctConfig/RouteConfig/ErrorPatternConfig: extract merge lambdas to methods - Version: log IOException at DEBUG level instead of swallowing - KeyExtractorInfo: remove redundant compact constructor validation - JarInstaller: wrap System.getProperty in Option with fallback - ConfigLoader: use recursive Option pattern instead of null loop - SliceManifest: use Option.option() for property access - SliceProjectValidator: use onSuccess/onFailure instead of fold - InitCommand: catch IOException specifically instead of Exception - Tests: replace .unwrap() with monadic assertions in 3 test files Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@jbct-cli/src/main/java/org/pragmatica/jbct/cli/InitCommand.java`:
- Around line 270-281: The isJavaKeyword method's keyword switch is missing
newer reserved/contextual keywords; update the switch in isJavaKeyword to
include "_" and the contextual/reserved tokens: "yield", "record", "sealed",
"permits", "var", "non-sealed", "module", "open", "exports", "opens",
"requires", "uses", "provides", "to", "transitive", "when", "with" (alongside
the existing entries) so the method returns true for these additional Java 22–25
keywords.
In `@jbct-cli/src/main/java/org/pragmatica/jbct/cli/ScoreCommand.java`:
- Around line 78-82: The LintContext built in the CLI commands omits slice
package wiring; update the context builders in ScoreCommand.createContext (and
likewise in LintCommand.createContext and CheckCommand.createContext) to call
.withSlicePackages(jbctConfig.slicePackages()) on the LintContext fluent builder
so that slicePackages from JbctConfig are applied when creating the context.
In `@jbct-core/src/main/java/org/pragmatica/jbct/upgrade/JarInstaller.java`:
- Around line 47-50: The fallback path in defaultInstallPath() uses
System.getProperty("java.io.tmpdir") directly which can be null and cause
Path.of(...) to throw; modify defaultInstallPath() to guard the tmpdir lookup by
checking System.getProperty("java.io.tmpdir") and using a safe fallback (e.g.,
System.getProperty("user.dir") or ".") if tmpdir is null or empty before
constructing the Path for DEFAULT_INSTALL_DIR, LIB_DIR and JAR_NAME so the or()
branch never calls Path.of with a null first argument.
In
`@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java`:
- Around line 582-607: The temp file created in addClassFiles (via
Files.createTempFile -> tempClass) for transformed bytecode isn't cleaned up;
modify addClassFiles so after writing transformedBytes and calling
archiver.addFile(tempClass.toFile(), relativePath) you delete the temp file
(e.g. Files.deleteIfExists(tempClass)) in a finally block or immediately after
archiving, ensuring deletion even on exceptions; keep transformFactoryBytecode
unchanged but ensure temp file lifecycle is tied to the archiving step to avoid
leaving junk in the temp directory.
In `@slice-processor/src/main/java/org/pragmatica/jbct/slice/SliceProcessor.java`:
- Around line 203-218: The iteration over the synchronized set
routeServiceEntries in writeRouteServiceFile is not protected by external
synchronization, risking concurrent-modification; wrap the loop that writes each
entry with a synchronized(routeServiceEntries) { ... } block (inside
writeRouteServiceFile, around the for (var entry : routeServiceEntries)
writer.println(entry);) so the creation of the service file iterates under the
set's monitor and prevents race conditions when process() may concurrently add
entries.
🧹 Nitpick comments (4)
jbct-cli/src/main/java/org/pragmatica/jbct/cli/Version.java (1)
22-24: Good improvement: logging instead of swallowing the exception.Logging at DEBUG level is appropriate for this non-critical failure scenario. Optionally, you could include the exception itself as the last argument to capture the stack trace in DEBUG logs if needed for troubleshooting:
♻️ Optional: Include exception for stack trace
} catch (IOException e) { - LOG.debug("Failed to load version properties: {}", e.getMessage()); + LOG.debug("Failed to load version properties: {}", e.getMessage(), e); }jbct-core/src/main/java/org/pragmatica/jbct/slice/SliceManifest.java (1)
80-82: Add import forOptioninstead of using fully qualified name.The code uses
org.pragmatica.lang.Option.option()with fully qualified class name in three places (lines 80, 107, 137). Adding an import would improve readability and maintain consistency with the existing imports forResultandCauses.♻️ Proposed fix
Add to imports section (after line 4):
import org.pragmatica.lang.Option;Then simplify usages:
- return org.pragmatica.lang.Option.option(props.getProperty("slice.name")) + return Option.option(props.getProperty("slice.name"))- return org.pragmatica.lang.Option.option(props.getProperty(key)) + return Option.option(props.getProperty(key))- return org.pragmatica.lang.Option.option(value) + return Option.option(value)jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectValidator.java (1)
69-103: Stabilize manifest scan order and make missing-property errors actionable.
Files.list()order is unspecified, which can lead to non‑deterministic error ordering. Also, missing-property messages don’t indicate which manifest failed when multiple slices exist. Sorting and including the filename will make diagnostics deterministic and easier to fix.♻️ Recommended refactor
- var manifestFiles = files.filter(p -> p.toString() - .endsWith(".manifest")) - .toList(); + var manifestFiles = files.filter(p -> p.toString() + .endsWith(".manifest")) + .sorted((a, b) -> a.getFileName() + .toString() + .compareTo(b.getFileName().toString())) + .toList(); @@ - .onSuccess(props -> { - checkRequired(props, "slice.interface", errors); - checkRequired(props, "slice.artifactId", errors); - }); + .onSuccess(props -> { + checkRequired(props, "slice.interface", manifestFile, errors); + checkRequired(props, "slice.artifactId", manifestFile, errors); + }); @@ - private void checkRequired(Properties props, String key, List<String> errors) { - if (props.getProperty(key) == null) { - errors.add("Missing required property '" + key + "' in slice manifest"); - } - } + private void checkRequired(Properties props, String key, Path manifestFile, List<String> errors) { + var value = props.getProperty(key); + if (value == null || value.isBlank()) { + errors.add("Missing required property '" + key + "' in " + manifestFile.getFileName()); + } + }jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (1)
212-258: Replacejava.util.Optionalwith pragmatica-liteOption.Lines 212-258 use
java.util.OptionalinreadFirstSliceManifest()andtoSliceArtifactInfo(), but per coding guidelines, pragmatica-lite functional types should be used for all Java files. Replacejava.util.OptionalwithOptionfrom pragmatica-lite:core.
| private static boolean isJavaKeyword(String word) { | ||
| return switch (word) { | ||
| case "abstract", "assert", "boolean", "break", "byte", "case", "catch", "char", | ||
| "class", "const", "continue", "default", "do", "double", "else", "enum", | ||
| "extends", "final", "finally", "float", "for", "goto", "if", "implements", | ||
| "import", "instanceof", "int", "interface", "long", "native", "new", "package", | ||
| "private", "protected", "public", "return", "short", "static", "strictfp", | ||
| "super", "switch", "synchronized", "this", "throw", "throws", "transient", | ||
| "try", "void", "volatile", "while", "true", "false", "null" -> true; | ||
| default -> false; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Java 22 Java 23 Java 24 Java 25 reserved keywords full list official specification
💡 Result:
For Java 22, 23, 24, and 25, the official JLS list of reserved keywords is the same. Java SE 22–25 are listed on Oracle’s Java SE Specifications page. [1]
Reserved keywords (cannot be used as identifiers) — 51
abstract continue for new switch
assert default if package synchronized
boolean do goto private this
break double implements protected throw
byte else import public throws
case enum instanceof return transient
catch extends int short try
char final interface static void
class finally long strictfp volatile
const float native super while
_
(From JLS §3.9 “Keywords”.) [2][3]
Contextual keywords (keyword meaning depends on context) — 17
exports opens requires uses yield
module permits sealed var
non-sealed provides to when
open record transitive with
(From JLS §3.9 “Keywords”.) [2][3]
Not keywords (but still not usable as identifiers because they are literals)
true,false(boolean literals)null(null literal) [3]
Sources: Oracle Java SE Specifications index (SE 22–25) [1]; JLS §3.9 for Java SE 25 [2] and Java SE 22 (same lists) [3].
🏁 Script executed:
# Locate and inspect the actual InitCommand.java file
find . -name "InitCommand.java" -type fRepository: siy/jbct-cli
Length of output: 121
🏁 Script executed:
# Inspect the actual isJavaKeyword method in InitCommand.java
sed -n '270,281p' ./jbct-cli/src/main/java/org/pragmatica/jbct/cli/InitCommand.javaRepository: siy/jbct-cli
Length of output: 800
Java keyword list is incomplete for Java 22-25 specification.
The keyword list is missing reserved and contextual keywords that should be blocked for package name segments:
Reserved keyword: _
Contextual keywords: yield, record, sealed, permits, var, non-sealed, module, open, exports, opens, requires, uses, provides, to, transitive, when, with
While unlikely in real package names, including the full official JLS keyword set prevents edge-case failures.
🔧 Proposed fix to add missing keywords
private static boolean isJavaKeyword(String word) {
return switch (word) {
case "abstract", "assert", "boolean", "break", "byte", "case", "catch", "char",
"class", "const", "continue", "default", "do", "double", "else", "enum",
"extends", "final", "finally", "float", "for", "goto", "if", "implements",
"import", "instanceof", "int", "interface", "long", "native", "new", "package",
"private", "protected", "public", "return", "short", "static", "strictfp",
"super", "switch", "synchronized", "this", "throw", "throws", "transient",
- "try", "void", "volatile", "while", "true", "false", "null" -> true;
+ "try", "void", "volatile", "while", "true", "false", "null",
+ "_", "yield", "record", "sealed", "permits", "var", "non-sealed",
+ "module", "open", "exports", "opens", "requires", "uses", "provides",
+ "to", "transitive", "when", "with" -> true;
default -> false;
};
}🤖 Prompt for AI Agents
In `@jbct-cli/src/main/java/org/pragmatica/jbct/cli/InitCommand.java` around lines
270 - 281, The isJavaKeyword method's keyword switch is missing newer
reserved/contextual keywords; update the switch in isJavaKeyword to include "_"
and the contextual/reserved tokens: "yield", "record", "sealed", "permits",
"var", "non-sealed", "module", "open", "exports", "opens", "requires", "uses",
"provides", "to", "transitive", "when", "with" (alongside the existing entries)
so the method returns true for these additional Java 22–25 keywords.
| private LintContext createContext(JbctConfig jbctConfig) { | ||
| return LintContext.defaultContext() | ||
| .withConfig(jbctConfig.lint()) | ||
| .withBusinessPackages(jbctConfig.businessPackages()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find LintContext class definition and API
rg -n --type=java 'class LintContext|record LintContext' -A 20Repository: siy/jbct-cli
Length of output: 2338
🏁 Script executed:
#!/bin/bash
# Search for slicePackages usage throughout the codebase
rg -n --type=java 'slicePackages|withSlicePackages'Repository: siy/jbct-cli
Length of output: 2279
🏁 Script executed:
#!/bin/bash
# Find other context creation methods to compare approach
rg -n --type=java 'createLintContext|createContext' -B 2 -A 5Repository: siy/jbct-cli
Length of output: 8992
🏁 Script executed:
#!/bin/bash
# Check JbctConfig structure to verify slicePackages field exists
rg -n --type=java 'class JbctConfig|record JbctConfig' -A 30Repository: siy/jbct-cli
Length of output: 3738
🏁 Script executed:
#!/bin/bash
# Get full createContext() implementation from LintCommand and CheckCommand
cat -n jbct-cli/src/main/java/org/pragmatica/jbct/cli/LintCommand.java | sed -n '99,110p'
cat -n jbct-cli/src/main/java/org/pragmatica/jbct/cli/CheckCommand.java | sed -n '112,125p'Repository: siy/jbct-cli
Length of output: 1258
🏁 Script executed:
#!/bin/bash
# Also check the LintContext.fromConfig() method to confirm it wires slicePackages
cat -n jbct-core/src/main/java/org/pragmatica/jbct/lint/LintContext.java | sed -n '125,135p'Repository: siy/jbct-cli
Length of output: 414
Add .withSlicePackages() to all CLI command context builders.
LintContext.fromConfig() in the Maven plugin correctly wires both businessPackages and slicePackages, but all three CLI commands—ScoreCommand, LintCommand, and CheckCommand—omit .withSlicePackages(jbctConfig.slicePackages()) from their context builders. Since JbctConfig carries slicePackages (required for JBCT-SLICE-01) and LintContext fully supports it, this creates inconsistency: the Maven plugin correctly configures slice rules while the CLI commands will silently ignore all slice package configuration. Add the missing call to all three commands:
.withSlicePackages(jbctConfig.slicePackages())🤖 Prompt for AI Agents
In `@jbct-cli/src/main/java/org/pragmatica/jbct/cli/ScoreCommand.java` around
lines 78 - 82, The LintContext built in the CLI commands omits slice package
wiring; update the context builders in ScoreCommand.createContext (and likewise
in LintCommand.createContext and CheckCommand.createContext) to call
.withSlicePackages(jbctConfig.slicePackages()) on the LintContext fluent builder
so that slicePackages from JbctConfig are applied when creating the context.
| public static Path defaultInstallPath() { | ||
| var userHome = System.getProperty("user.home"); | ||
| return Path.of(userHome, DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME); | ||
| return Option.option(System.getProperty("user.home")) | ||
| .map(userHome -> Path.of(userHome, DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME)) | ||
| .or(() -> Path.of(System.getProperty("java.io.tmpdir"), DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME)); |
There was a problem hiding this comment.
Guard the tmpdir fallback against a missing property.
If java.io.tmpdir is null or blocked, Path.of(null, ...) will throw, so the resilience improvement is incomplete. Add a default value (e.g., user.dir or ".") for the fallback.
🛠️ Suggested fix
- return Option.option(System.getProperty("user.home"))
- .map(userHome -> Path.of(userHome, DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME))
- .or(() -> Path.of(System.getProperty("java.io.tmpdir"), DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME));
+ return Option.option(System.getProperty("user.home"))
+ .map(userHome -> Path.of(userHome, DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME))
+ .or(() -> Path.of(System.getProperty("java.io.tmpdir",
+ System.getProperty("user.dir", ".")),
+ DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME));📝 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.
| public static Path defaultInstallPath() { | |
| var userHome = System.getProperty("user.home"); | |
| return Path.of(userHome, DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME); | |
| return Option.option(System.getProperty("user.home")) | |
| .map(userHome -> Path.of(userHome, DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME)) | |
| .or(() -> Path.of(System.getProperty("java.io.tmpdir"), DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME)); | |
| public static Path defaultInstallPath() { | |
| return Option.option(System.getProperty("user.home")) | |
| .map(userHome -> Path.of(userHome, DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME)) | |
| .or(() -> Path.of(System.getProperty("java.io.tmpdir", | |
| System.getProperty("user.dir", ".")), | |
| DEFAULT_INSTALL_DIR, LIB_DIR, JAR_NAME)); | |
| } |
🤖 Prompt for AI Agents
In `@jbct-core/src/main/java/org/pragmatica/jbct/upgrade/JarInstaller.java` around
lines 47 - 50, The fallback path in defaultInstallPath() uses
System.getProperty("java.io.tmpdir") directly which can be null and cause
Path.of(...) to throw; modify defaultInstallPath() to guard the tmpdir lookup by
checking System.getProperty("java.io.tmpdir") and using a safe fallback (e.g.,
System.getProperty("user.dir") or ".") if tmpdir is null or empty before
constructing the Path for DEFAULT_INSTALL_DIR, LIB_DIR and JAR_NAME so the or()
branch never calls Path.of with a null first argument.
| private void addClassFiles(JarArchiver archiver, String className, Map<String, String> versionMap) | ||
| throws MojoExecutionException { | ||
| var classesPath = classesDirectory.toPath(); | ||
| var paths = SliceManifest.classToPathsWithInner(className, classesPath); | ||
| for (var relativePath : paths) { | ||
| var classFile = new File(classesDirectory, relativePath); | ||
| if (classFile.exists()) { | ||
| archiver.addFile(classFile, relativePath); | ||
| try{ | ||
| for (var relativePath : paths) { | ||
| var classFile = new File(classesDirectory, relativePath); | ||
| if (classFile.exists()) { | ||
| // Transform factory classes with UNRESOLVED versions | ||
| if (className.endsWith("Factory") && !versionMap.isEmpty() && | ||
| relativePath.equals(className.replace('.', '/') + ".class")) { | ||
| var transformedBytes = transformFactoryBytecode(classFile, versionMap); | ||
| // Write transformed bytecode to temp file for archiving | ||
| var tempClass = Files.createTempFile("factory-", ".class"); | ||
| Files.write(tempClass, transformedBytes); | ||
| archiver.addFile(tempClass.toFile(), relativePath); | ||
| getLog().info("Transformed bytecode: " + className); | ||
| } else { | ||
| // Add non-factory classes and inner classes as-is | ||
| archiver.addFile(classFile, relativePath); | ||
| } | ||
| } | ||
| } | ||
| } catch (IOException e) { | ||
| throw new MojoExecutionException("Failed to transform factory bytecode", e); | ||
| } |
There was a problem hiding this comment.
Add temp-file cleanup for transformed class output.
The temp file created for transformed bytecode isn’t marked for cleanup, so repeated runs can leave junk in the temp directory.
Proposed fix
- var tempClass = Files.createTempFile("factory-", ".class");
+ var tempClass = Files.createTempFile("factory-", ".class");
+ tempClass.toFile().deleteOnExit();
Files.write(tempClass, transformedBytes);🤖 Prompt for AI Agents
In
`@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java`
around lines 582 - 607, The temp file created in addClassFiles (via
Files.createTempFile -> tempClass) for transformed bytecode isn't cleaned up;
modify addClassFiles so after writing transformedBytes and calling
archiver.addFile(tempClass.toFile(), relativePath) you delete the temp file
(e.g. Files.deleteIfExists(tempClass)) in a finally block or immediately after
archiving, ensuring deletion even on exceptions; keep transformFactoryBytecode
unchanged but ensure temp file lifecycle is tied to the archiving step to avoid
leaving junk in the temp directory.
| private void writeRouteServiceFile() { | ||
| try{ | ||
| var serviceFile = processingEnv.getFiler() | ||
| .createResource(StandardLocation.CLASS_OUTPUT, | ||
| "", | ||
| "META-INF/services/org.pragmatica.aether.http.adapter.SliceRouterFactory"); | ||
| try (var writer = new java.io.PrintWriter(serviceFile.openWriter())) { | ||
| for (var entry : routeServiceEntries) { | ||
| writer.println(entry); | ||
| } | ||
| } | ||
| } catch (IOException e) { | ||
| processingEnv.getMessager() | ||
| .printMessage(Diagnostic.Kind.ERROR, | ||
| "Failed to write SliceRouterFactory service file: " + e.getMessage()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "SliceProcessor.java" | head -1Repository: siy/jbct-cli
Length of output: 131
🏁 Script executed:
cat -n slice-processor/src/main/java/org/pragmatica/jbct/slice/SliceProcessor.java | head -100Repository: siy/jbct-cli
Length of output: 5636
🏁 Script executed:
rg "routeServiceEntries" -A 2 -B 2 slice-processor/src/main/java/org/pragmatica/jbct/slice/SliceProcessor.javaRepository: siy/jbct-cli
Length of output: 1367
Synchronize iteration over the synchronized set.
Collections.synchronizedSet requires external synchronization during iteration to avoid race hazards. The process() method can be called concurrently by the annotation processor framework, and routeServiceEntries is modified via add() during processing. Wrap the iteration with a synchronized block:
Suggested fix
- try (var writer = new java.io.PrintWriter(serviceFile.openWriter())) {
- for (var entry : routeServiceEntries) {
- writer.println(entry);
- }
- }
+ try (var writer = new java.io.PrintWriter(serviceFile.openWriter())) {
+ synchronized (routeServiceEntries) {
+ for (var entry : routeServiceEntries) {
+ writer.println(entry);
+ }
+ }
+ }📝 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.
| private void writeRouteServiceFile() { | |
| try{ | |
| var serviceFile = processingEnv.getFiler() | |
| .createResource(StandardLocation.CLASS_OUTPUT, | |
| "", | |
| "META-INF/services/org.pragmatica.aether.http.adapter.SliceRouterFactory"); | |
| try (var writer = new java.io.PrintWriter(serviceFile.openWriter())) { | |
| for (var entry : routeServiceEntries) { | |
| writer.println(entry); | |
| } | |
| } | |
| } catch (IOException e) { | |
| processingEnv.getMessager() | |
| .printMessage(Diagnostic.Kind.ERROR, | |
| "Failed to write SliceRouterFactory service file: " + e.getMessage()); | |
| } | |
| private void writeRouteServiceFile() { | |
| try{ | |
| var serviceFile = processingEnv.getFiler() | |
| .createResource(StandardLocation.CLASS_OUTPUT, | |
| "", | |
| "META-INF/services/org.pragmatica.aether.http.adapter.SliceRouterFactory"); | |
| try (var writer = new java.io.PrintWriter(serviceFile.openWriter())) { | |
| synchronized (routeServiceEntries) { | |
| for (var entry : routeServiceEntries) { | |
| writer.println(entry); | |
| } | |
| } | |
| } | |
| } catch (IOException e) { | |
| processingEnv.getMessager() | |
| .printMessage(Diagnostic.Kind.ERROR, | |
| "Failed to write SliceRouterFactory service file: " + e.getMessage()); | |
| } |
🤖 Prompt for AI Agents
In `@slice-processor/src/main/java/org/pragmatica/jbct/slice/SliceProcessor.java`
around lines 203 - 218, The iteration over the synchronized set
routeServiceEntries in writeRouteServiceFile is not protected by external
synchronization, risking concurrent-modification; wrap the loop that writes each
entry with a synchronized(routeServiceEntries) { ... } block (inside
writeRouteServiceFile, around the for (var entry : routeServiceEntries)
writer.println(entry);) so the creation of the service file iterates under the
set's monitor and prevents race conditions when process() may concurrently add
entries.
- PathTraversal → PathTraversalDetected - InvalidUrl → UrlRejected - UntrustedDomain → DomainRejected Also added versioning policy to CLAUDE.md: backward compatibility not guaranteed until 1.0. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
420-435:⚠️ Potential issue | 🟡 MinorUpdate slice project structure example to match the new template.
This section still showsMySliceImplandSampleRequest/Response, which conflicts with the new nested-record pattern and removed “Sample” prefixes. Please update the example to avoid onboarding confusion.
🧹 Nitpick comments (5)
jbct-core/src/main/java/org/pragmatica/jbct/shared/SecurityError.java (1)
5-6: Remove unused imports.
URIandPathare imported but not used in this file. They may be remnants from before the refactoring.🧹 Proposed fix
package org.pragmatica.jbct.shared; import org.pragmatica.lang.Cause; -import java.net.URI; -import java.nio.file.Path; - /**jbct-core/src/main/java/org/pragmatica/jbct/shared/UrlValidation.java (1)
31-31: Minor formatting: add space before brace.Missing space between
tryand{for consistency with Java conventions.🧹 Proposed fix
- try{ + try {CLAUDE.md (1)
76-185: Clarify score clamping/rounding to keep results within 0–100.
The formula can go below 0 if weighted violations exceed checkpoints; a one-liner note that scores are clamped to [0,100] and rounded would prevent ambiguity.CHANGELOG.md (2)
9-10: Ensure consistent capitalization after category prefixes.Lines 11-12 use uppercase verbs (Bump, Re-enabled) following the past review corrections. The remaining entries should match this pattern for consistency.
📝 Proposed capitalization fixes
-- Slice init: added tinylog dependencies (2.7.0) in test scope -- Slice init: added tinylog.properties configuration file in test resources +- Slice init: Added tinylog dependencies (2.7.0) in test scope +- Slice init: Added tinylog.properties configuration file in test resources - Build: Bump Aether to 0.8.1 - CI: Re-enabled slice-processor-tests module -- Slice init: Java version 21 to 25 -- Slice init: updated default versions (Pragmatica Lite 0.11.1, Aether 0.8.1, JBCT 0.6.0) -- Slice init: implementation pattern changed to record-based (nested record in interface) -- Slice init: removed Config dependency from template (factory now parameterless) -- Slice init: added annotation processor configuration to maven-compiler-plugin -- Slice init: added compilerArgs with `-Aslice.groupId` and `-Aslice.artifactId` -- Slice init: removed separate *Impl.java, SampleRequest.java, SampleResponse.java files +- Slice init: Updated Java version from 21 to 25 +- Slice init: Updated default versions (Pragmatica Lite 0.11.1, Aether 0.8.1, JBCT 0.6.0) +- Slice init: Changed implementation pattern to record-based (nested record in interface) +- Slice init: Removed Config dependency from template (factory now parameterless) +- Slice init: Added annotation processor configuration to maven-compiler-plugin +- Slice init: Added compilerArgs with `-Aslice.groupId` and `-Aslice.artifactId` +- Slice init: Removed separate *Impl.java, SampleRequest.java, SampleResponse.java files - Slice init: Request/Response/Error records now nested in `@Slice` interface -- Slice init: removed "Sample" prefix from Request/Response records -- Slice init: inner implementation record uses lowercased slice name, not "Impl" -- Init: version resolution uses running binary version as minimum (overrides GitHub API if newer) -- Init: version comparison uses Result-based `Number.parseInt()` from pragmatica-lite +- Slice init: Removed "Sample" prefix from Request/Response records +- Slice init: Inner implementation record uses lowercased slice name, not "Impl" +- Init: Version resolution uses running binary version as minimum (overrides GitHub API if newer) +- Init: Version comparison uses Result-based `Number.parseInt()` from pragmatica-liteAlso applies to: 14-25
13-13: Add explicit verb for clarity.The entry "Java version 21 to 25" could be more explicit about the action taken.
📝 Suggested improvement
-- Slice init: Java version 21 to 25 +- Slice init: Updated Java version from 21 to 25
Critical: - FormattingError: rename ParseError→ParseFailed, IoError→IoFailed - TestSlice: delete TestSliceImpl, use inline lambda factory - SliceProjectInitializer: extract lambdas to named methods - SliceManifest: extract buildManifest() method - SliceProcessorTest: replace orElseThrow() with get() Warnings: - InitCommand: use Option.option().or() for null checks - FormatCommand/LintCommand: replace AtomicInteger with int[] - FileCollector: return List.copyOf() for immutability - AiToolsUpdater: extract performUpdate() method - CstNodes: extract findAncestorInPath() method - JbctFormatterTest: use monadic assertions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jbct-cli/src/main/java/org/pragmatica/jbct/cli/InitCommand.java (1)
197-205:⚠️ Potential issue | 🟡 MinorSlice projects ignore version overrides and
effectiveAetherVersion()is dead code.Slice projects don't support version overrides from CLI options (
--jbct-version,--pragmatica-version,--aether-version), unlike regular projects (which support JBCT and Pragmatica overrides). TheinitSliceProject()method only passesprojectDir,groupId, andartifactIdtoSliceProjectInitializer.sliceProjectInitializer().Additionally,
effectiveAetherVersion()(lines 187-190) is never called—not even for regular projects, which only useeffectiveJbctVersion()andeffectivePragmaticaVersion()for overrides.jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectInitializer.java (1)
139-160:⚠️ Potential issue | 🔴 CriticalUse
Result.all()instead—Result.allOf()is for collections, not varargs.In pragmatica-lite 0.9.10,
Result.allOf(...)accepts a collection of Results (List<Result<T>>), not variable arguments. The call toResult.allOf(createProjectFiles(), createTestResources(), ...)will not compile.Use
Result.all(createProjectFiles(), createSourceFiles(), createTestResources(), createDeployScripts(), createSliceConfigFiles())for a fixed set of independent validations, or wrap the arguments in a collection:Result.allOf(List.of(createProjectFiles(), ...))if you need to build the list dynamically.
🤖 Fix all issues with AI agents
In `@jbct-cli/src/main/java/org/pragmatica/jbct/cli/LintCommand.java`:
- Around line 114-120: The switch inside LintCommand's lambda over diagnostics
has misaligned case labels; reformat the switch in the method containing the
loop over diagnostics so the case statements (case ERROR, case WARNING, case
INFO) are indented to match the switch body/lambda block rather than shifted
left, ensuring consistent alignment with the surrounding for (var d :
diagnostics) { ... } block and keeping the existing counters[0..2] increments
unchanged.
🧹 Nitpick comments (6)
slice-processor/src/test/java/org/pragmatica/jbct/slice/SliceProcessorTest.java (1)
533-536: Consider usingorElseThrow()with a descriptive message for better test failure diagnostics.While
get()works in test contexts where compilation success is already asserted, usingorElseThrow(() -> new AssertionError("Expected OrderServiceFactory to be generated"))provides clearer failure messages when debugging test failures. This applies to all 11 occurrences in this file.That said, the compilation success assertion on line 531 should guarantee the Optional is present, so this is a minor style preference.
CROSS_CUTTING_REVIEW.md (2)
272-272: Fix table formatting for consistency.The tables have inconsistent spacing around pipe characters. Add spaces around pipes for the "compact" style as flagged by markdownlint.
📝 Proposed formatting fixes
For line 272:
-| Pattern | Status | Usage | -|---------|--------|-------| +| Pattern | Status | Usage | +| ------- | ------ | ----- |For line 299:
-| Check | Status | Notes | -|-------|--------|-------| +| Check | Status | Notes | +| ----- | ------ | ----- |Also applies to: 299-299
327-327: Consider using a heading instead of emphasis.The "Overall Grade" line uses bold emphasis but could be semantically more correct as a heading.
📝 Proposed fix
-**Overall Grade: A (Excellent)** +### Overall Grade: A (Excellent)jbct-cli/src/main/java/org/pragmatica/jbct/cli/FormatCommand.java (1)
67-67: Consider named constants for counter indices.Using magic indices like
counters[0],counters[1],counters[2]throughout the code reduces readability. Since this is internal, it's acceptable, but named constants would improve maintainability.♻️ Optional: Use named constants for clarity
+ private static final int FORMATTED = 0; + private static final int UNCHANGED = 1; + private static final int ERRORS = 2; + `@Override` public Integer call() { // ... - var counters = new int[3]; // 0=formatted, 1=unchanged, 2=errors + var counters = new int[3];Then use
counters[FORMATTED],counters[UNCHANGED],counters[ERRORS]throughout.jbct-cli/src/main/java/org/pragmatica/jbct/cli/LintCommand.java (1)
77-77: Same readability note applies here.For consistency with the FormatCommand suggestion, consider named constants for the four counter indices (ERRORS=0, WARNINGS=1, INFOS=2, PARSE_ERRORS=3).
jbct-cli/src/main/java/org/pragmatica/jbct/cli/InitCommand.java (1)
85-90: Consider importingOptionto reduce verbosity.Fully qualified
org.pragmatica.lang.Optionappears multiple times throughout the file. Adding an import would improve readability.♻️ Suggested import addition
Add to imports section:
import org.pragmatica.lang.Option;Then simplify usages:
- projectDir = org.pragmatica.lang.Option.option(projectDir) + projectDir = Option.option(projectDir) .map(Path::toAbsolutePath) .or(() -> Path.of(System.getProperty("user.dir")));
| for (var d : diagnostics) { | ||
| switch (d.severity()) { | ||
| case ERROR -> errors.incrementAndGet(); | ||
| case WARNING -> warnings.incrementAndGet(); | ||
| case INFO -> infos.incrementAndGet(); | ||
| case ERROR -> counters[0]++; | ||
| case WARNING -> counters[1]++; | ||
| case INFO -> counters[2]++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Indentation issue in switch statement.
The case statements appear to be incorrectly indented - they should align with the surrounding lambda body context, not at column 12.
🔧 Proposed indentation fix
for (var d : diagnostics) {
switch (d.severity()) {
- case ERROR -> counters[0]++;
- case WARNING -> counters[1]++;
- case INFO -> counters[2]++;
- }
+ case ERROR -> counters[0]++;
+ case WARNING -> counters[1]++;
+ case INFO -> counters[2]++;
+ }
}📝 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.
| for (var d : diagnostics) { | |
| switch (d.severity()) { | |
| case ERROR -> errors.incrementAndGet(); | |
| case WARNING -> warnings.incrementAndGet(); | |
| case INFO -> infos.incrementAndGet(); | |
| case ERROR -> counters[0]++; | |
| case WARNING -> counters[1]++; | |
| case INFO -> counters[2]++; | |
| } | |
| } | |
| for (var d : diagnostics) { | |
| switch (d.severity()) { | |
| case ERROR -> counters[0]++; | |
| case WARNING -> counters[1]++; | |
| case INFO -> counters[2]++; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@jbct-cli/src/main/java/org/pragmatica/jbct/cli/LintCommand.java` around lines
114 - 120, The switch inside LintCommand's lambda over diagnostics has
misaligned case labels; reformat the switch in the method containing the loop
over diagnostics so the case statements (case ERROR, case WARNING, case INFO)
are indented to match the switch body/lambda block rather than shifted left,
ensuring consistent alignment with the surrounding for (var d : diagnostics) {
... } block and keeping the existing counters[0..2] increments unchanged.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- FactoryClassGenerator: fix infra flatMap nesting for variable scoping - GenerateBlueprintMojo: properly handle UNRESOLVED dependency edges - CollectSliceDepsMojo: tighten base.artifact validation, improve docs - HTTP-ROUTE-GENERATION.md: add text language to code blocks - CHANGELOG.md: remove empty section headings
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 16: Update the changelog entry to reflect the actual template factory
signature: confirm whether the slice template's static factory method still
accepts a Config parameter or is now parameterless, and then edit the line
"Slice init: removed Config dependency from template (factory now
parameterless)" to accurately state the change (e.g., "Slice init: factory now
accepts Config parameter" or "Slice init: removed Config dependency; factory is
now parameterless") so it matches the implementation of the factory static
method and the Config dependency.
- Around line 26-35: The changelog removal of slice-api.properties may be a
breaking change for consumers expecting META-INF/slice-api.properties; update
either the release notes or the code to preserve compatibility: either (A)
explicitly document the breaking change in CHANGELOG.md and state the new
canonical requirement (manifests at META-INF/slice/*.manifest and that
slice-api.properties is deprecated/removed), or (B) restore generation of
META-INF/slice-api.properties (ensuring CollectSliceDepsMojo, VerifySliceMojo,
PackageSlicesMojo, SliceProjectValidator and SliceManifest continue to
read/produce it) and mark manifests as the preferred format; choose one and make
matching changes to the changelog and the implementations (references:
slice-api.properties, META-INF/slice/*.manifest, CollectSliceDepsMojo,
VerifySliceMojo, PackageSlicesMojo, SliceProjectValidator, SliceManifest).
🧹 Nitpick comments (1)
slice-processor/src/main/java/org/pragmatica/jbct/slice/generator/FactoryClassGenerator.java (1)
649-721: LGTM, but note the different closing strategy fromgenerateAspectCreateChain.The method correctly keeps all flatMaps open and closes them via the loop at lines 699-702. This differs from
generateAspectCreateChainwhere slice handle flatMaps close inline (line 295-296 ends with")") while only infra flatMaps are tracked for deferred closing.Both approaches produce valid code, but the inconsistency could cause maintenance confusion. Consider unifying the closing strategy across both methods in a future refactor.
| - Slice init: Java version 21 to 25 | ||
| - Slice init: updated default versions (Pragmatica Lite 0.11.1, Aether 0.8.1, JBCT 0.6.0) | ||
| - Slice init: implementation pattern changed to record-based (nested record in interface) | ||
| - Slice init: removed Config dependency from template (factory now parameterless) |
There was a problem hiding this comment.
Changelog entry likely conflicts with the intended template signature.
Line 16 says the factory is now parameterless, but the PR objectives explicitly mention the factory static method accepts a Config dependency parameter. Please confirm the actual template change and adjust the entry accordingly to avoid misleading release notes.
🤖 Prompt for AI Agents
In `@CHANGELOG.md` at line 16, Update the changelog entry to reflect the actual
template factory signature: confirm whether the slice template's static factory
method still accepts a Config parameter or is now parameterless, and then edit
the line "Slice init: removed Config dependency from template (factory now
parameterless)" to accurately state the change (e.g., "Slice init: factory now
accepts Config parameter" or "Slice init: removed Config dependency; factory is
now parameterless") so it matches the implementation of the factory static
method and the Config dependency.
| ### Fixed | ||
| - RFC-0004 compliance: removed non-standard slice-api.properties generation | ||
| - RFC-0004 compliance: slice manifests now include `slice.interface` property | ||
| - RFC-0004 compliance: renamed `impl.artifactId` to `slice.artifactId` in manifests | ||
| - RFC-0007 compliance: infrastructure dependencies now accessed via InfraStore instead of being proxied | ||
| - CollectSliceDepsMojo: now scans META-INF/slice/*.manifest instead of slice-api.properties | ||
| - VerifySliceMojo: validates manifest files instead of slice-api.properties | ||
| - PackageSlicesMojo: reads slice metadata from .manifest files | ||
| - SliceProjectValidator: checks for .manifest files instead of slice-api.properties | ||
| - SliceManifest: reads `slice.artifactId` property (was incorrectly reading `impl.artifactId`) |
There was a problem hiding this comment.
Potential spec/backward-compat break: removal of slice-api.properties.
The changelog states slice-api.properties generation was removed in favor of manifests. If any tooling or RFC still requires META-INF/slice-api.properties, this is a breaking change and should be called out (or the removal reconsidered). Please confirm the current canonical requirement and update the changelog (or tooling) accordingly. Based on learnings: “Slice API manifest must be generated at META-INF/slice-api.properties mapping artifact to slice interface.”
🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 26 - 35, The changelog removal of
slice-api.properties may be a breaking change for consumers expecting
META-INF/slice-api.properties; update either the release notes or the code to
preserve compatibility: either (A) explicitly document the breaking change in
CHANGELOG.md and state the new canonical requirement (manifests at
META-INF/slice/*.manifest and that slice-api.properties is deprecated/removed),
or (B) restore generation of META-INF/slice-api.properties (ensuring
CollectSliceDepsMojo, VerifySliceMojo, PackageSlicesMojo, SliceProjectValidator
and SliceManifest continue to read/produce it) and mark manifests as the
preferred format; choose one and make matching changes to the changelog and the
implementations (references: slice-api.properties, META-INF/slice/*.manifest,
CollectSliceDepsMojo, VerifySliceMojo, PackageSlicesMojo, SliceProjectValidator,
SliceManifest).
1 participant
Summary
Changes
Slice Init Template Updates
Template Structure
Test Status
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changed
Chores
✏️ Tip: You can customize this high-level summary in your review settings.