Conversation
- Security: add PathValidation, UrlValidation, SecurityError types - Null policy: replace nullable with Option across codebase - Factory methods: add missing factories for records - Composition: extract complex lambdas to named methods - Fork-Join: use Result.all() for parallel operations - Thread safety: document non-thread-safe classes, eager init - Immutability: defensive copies in record constructors - Shared utilities: GitHubContentFetcher reduces duplication
- Add AspectModel and KeyExtractorInfo model classes - Extend MethodModel to extract aspect annotations from methods - Update FactoryClassGenerator to generate wrapper records with caching - Add SliceRuntime/AspectFactory integration for cache creation - Add infra-aspect dependency (aether 0.8.0) - Add 5 tests for aspect annotation processing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- KeyExtractorInfo: Result-returning factories, validation - DependencyModel: empty string guard, acronym handling - MethodModel: extract lambdas to methods, stream-based processing - RouteConfigLoader: extract methods, use System.Logger - RouteSourceGenerator: escape paths and query params - DependencyVersionResolver: handle IllegalArgumentException - CstFullyQualifiedNameRule: flag java.lang.* FQCNs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Format all modules with jbct:format - Update CHANGELOG with @Aspect/@key processing, FQCN rule enhancement - Update release date to 2026-01-18 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- ManifestGenerator now writes slice artifact as groupId:artifactId-sliceName - FactoryClassGenerator adds .async() for Result->Promise conversion - Updated test stubs to match real SliceInvokerFacade interface - Added test for slice artifact naming convention Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…e libs - VerifySliceMojo now checks pragmatica-lite and aether dependencies - Errors if runtime libs are not marked as 'provided' scope - Updated slice template to use provided scope for all runtime deps Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- isAetherRuntime now filters all org.pragmatica-lite and org.pragmatica-lite.aether - Only include direct dependencies in dependencies file sections - Prevents Netty/Jackson transitives from http-routing-adapter in slice deps Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- toApiArtifactInfo reads api.artifact from META-INF/slice-api.properties - toSliceArtifactInfo reads slice.artifact from manifest - Fixes incorrect artifact names like inventory-api instead of inventory-inventory-service-api Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove ApiInterfaceGenerator and api package generation - All dependencies now go through SliceInvokerFacade proxy - Single JAR per slice (no separate API JAR) - Remove JBCT-SLICE-01 lint rule (no longer applicable) - Add slice dependency scope validation to VerifySliceMojo - Update all documentation and RFCs BREAKING CHANGE: Slices no longer generate separate API interfaces. Consumers should depend on the @slice interface directly.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughConsolidates slice packaging into a single Slice JAR, replaces API-JAR flows with generated factory+manifest, routes all dependencies via MethodHandle-based proxies through SliceInvokerFacade, adds aspect/key support and cache key extraction, and introduces security/fetch utilities (SecurityError, PathValidation, UrlValidation, GitHubContentFetcher). Changes
Sequence Diagram(s)sequenceDiagram
participant Processor as SliceProcessor
participant Model as SliceModel
participant Generator as FactoryClassGenerator
participant Facade as SliceInvokerFacade
participant Runtime as SliceRuntime
participant Repo as ArtifactRepo
Processor->>Model: extractMethods(element, env)
Processor->>Generator: generate(sliceModel)
Generator->>Facade: methodHandle(artifact, method, requestType, responseType)
Facade->>Runtime: request runtime MethodHandle/adapter
Runtime-->>Facade: Result<MethodHandle>
Facade-->>Generator: Result<MethodHandle>
Generator->>Generator: generate local proxy records + aspect wrappers
Generator->>Repo: emit factory class & slice manifest
Repo-->>Processor: artifacts written
sequenceDiagram
participant Maven as jbct-maven-plugin
participant Package as PackageSlicesMojo
participant Resolver as DependencyVersionResolver
participant Manifest as SliceManifest
participant Verifier as VerifySliceMojo
Maven->>Package: execute()
Package->>Resolver: read slice manifests from dependencies
Resolver-->>Package: artifact coordinates
Package->>Manifest: build slice.artifact and impl classes
Package->>Verifier: checkRuntimeDependencyScopes(deps)
Verifier->>Verifier: validate 'provided' scope for runtime & slice deps
Verifier-->>Package: ValidationResult
Package->>Maven: create impl JAR and POM (no API JAR)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstAlwaysSuccessResultRule.java (1)
49-51: Boolean expression continuation lacks proper indentation.The continuation lines of the
||expression are not indented, making the code difficult to read and inconsistent with formatting standards.🔧 Proposed fix
boolean hasFailure = methodText.contains("Result.failure(") || - methodText.contains(".result()") || // cause.result() - methodText.contains("failure"); + methodText.contains(".result()") || // cause.result() + methodText.contains("failure");jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstReturnKindRule.java (1)
59-74: Handle fully qualified forbidden types (e.g.,java.util.Optional).
typeTextcomparisons won’t matchjava.util.Optional<T>orjava.util.concurrent.CompletableFuture<T>, so forbidden types can slip through. Consider normalizing to a simple type name before the checks.🔧 Suggested normalization (minimal)
- var typeText = text(type, source).trim(); + var typeText = text(type, source).trim(); + var normalizedType = typeText.replaceFirst("^.*\\.", ""); var methodName = childByRule(method, RuleId.Identifier.class).map(id -> text(id, source)) .or("(unknown)"); // Check for void - if (typeText.equals("void")) { + if (normalizedType.equals("void")) { return Stream.of(createVoidDiagnostic(method, methodName, ctx)); } // Check for forbidden types for (var forbidden : FORBIDDEN_TYPES) { - if (typeText.startsWith(forbidden + "<") || typeText.equals(forbidden)) { + if (normalizedType.startsWith(forbidden + "<") || normalizedType.equals(forbidden)) { return Stream.of(createForbiddenTypeDiagnostic(method, methodName, typeText, ctx)); } }jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstZoneThreeVerbsRule.java (2)
147-155: Avoid suggesting a Zone‑2 verb as a Zone‑3 alternative.The suggestion
"check"is itself inZONE_2_VERBS, so the diagnostic points to a verb the rule flags. Let these fall through to the Zone‑3 default (or map to explicit Zone‑3 verbs).🛠️ Minimal fix (use the Zone‑3 default)
- case "validate", "verify", "check" -> "check";
103-115: Use CST modifiers instead of string search forprivate.The
text(classMember, source).contains("private ")pattern is fragile—it can misclassify when "private" appears in comments or strings. Use CST-based modifier detection instead, following the pattern inCstSealedErrorRule:- var memberText = text(classMember, source); - // Check if private - if (!memberText.contains("private ")) { + var isPrivate = childrenByRule(classMember, RuleId.Modifier.class).stream() + .map(m -> text(m, source).trim()) + .anyMatch("private"::equals); + if (!isPrivate) { return false; }Note:
childrenByRule()retrieves only direct modifier children (preferred overfindAll()for efficiency).README.md (1)
385-420: Docs still reference JBCT-SLICE-01 after its removal.
Please update/remove the JBCT-SLICE-01 rule andslicePackagesguidance to avoid misleading users.jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstAcronymNamingRule.java (1)
54-72: Suggested fix can duplicate a boundary letter for common acronyms.
With diagnostics now usingsuggestFix(...), cases likeHTTPClientcan render asHttpCClientbecause the last acronym letter is processed twice. Consider skipping reprocessing of that last uppercase character.🛠️ Proposed adjustment in
suggestFix- i = acronymEnd - 2; + i = acronymEnd - 1;docs/slice/architecture.md (1)
279-295: Add a language tag to the fenced code block.markdownlint reports MD040 here; please specify a language (e.g.,
text) for the Slice JAR contents block.✅ Suggested fix
-``` +```text org/example/order/OrderService.class org/example/order/OrderServiceImpl.class org/example/order/OrderServiceFactory.class org/example/order/OrderServiceFactory$orderServiceSlice.class org/example/order/OrderServiceFactory$inventoryService.class org/example/order/OrderServiceFactory$pricingEngine.class org/example/order/PlaceOrderRequest.class org/example/order/OrderResult.class META-INF/slice/OrderService.manifest META-INF/slice-api.properties META-INF/dependencies/org.example.order.OrderServiceFactory # Runtime deps META-INF/MANIFEST.MF Slice-Artifact: org.example:commerce-order-service:1.0.0 Slice-Class: org.example.order.OrderServiceFactory # Plus bundled external dependencies (jackson, etc.)</details> </blockquote></details> </blockquote></details>🤖 Fix all issues with AI agents
In `@CLAUDE.md`: - Line 6: The bare URL "https://github.com/siy/coding-technology/tree/master/proposals/rfc" in CLAUDE.md is triggering markdownlint MD034; change it by either wrapping it in angle brackets (<https://github.com/siy/coding-technology/tree/master/proposals/rfc>) or turning it into a labeled markdown link (e.g. [proposals/rfc](https://github.com/siy/coding-technology/tree/master/proposals/rfc)) so the linter warning is cleared. In `@jbct-core/src/main/java/org/pragmatica/jbct/config/JbctConfig.java`: - Around line 113-147: The lambda body in JbctConfig.merge(Option<JbctConfig> other) has inconsistent indentation; reformat the entire lambda so its body lines (including comments and statements that set mergedFormatter, mergedLint, mergedSourceDirs, mergedBusinessPackages, mergedSlicePackages and the return jbctConfig(...) call) are indented consistently according to the project's rule (lambda body aligned to the parameter + 4 spaces), ensuring comments align with the statements they describe. In `@jbct-core/src/main/java/org/pragmatica/jbct/format/cst/CstFormatter.java`: - Around line 15-20: The class-level javadoc wrongly claims thread-safety while CstFormatter holds a shared Java25Parser instance; to fix, stop reusing the single parser field and create a fresh Java25Parser per invocation of format(SourceFile): remove or stop referencing the shared parser field (the constructor initialization), and instantiate a new Java25Parser inside the format(...) method (or use a ThreadLocal supplier) so each call gets its own parser instance and no mutable parser state is shared across threads. In `@jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstConstructorBypassRule.java`: - Around line 76-81: The lambda body returned by findAncestor(root, node, RuleId.ClassMember.class) has inconsistent indentation; reformat the block around the lambda (the anonymous parameter "member", the var memberText = text(member, source); line, the inline comment, the return memberText.contains("static ") && memberText.contains("Result<"); line, and the closing "})") so all statements inside the lambda are consistently indented and aligned with the opening "{", preserving the comment position and keeping the expression returned from the lambda on its own properly indented; locate this in CstConstructorBypassRule.java by the findAncestor(...) call, the local variable memberText and the text(member, source) invocation. In `@jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstSealedErrorRule.java`: - Around line 44-46: The helper getInterfaceDecl currently falls back to returning the parent TypeDecl via .or(typeDecl), which can mask errors; change it to fail fast by using orElseThrow (or otherwise throw a clear IllegalStateException) so it only returns an InterfaceDecl when one exists: update getInterfaceDecl to call childByRule(typeDecl, RuleId.InterfaceDecl.class).orElseThrow(...) and include a message referencing typeDecl (or the lack of InterfaceDecl) to aid debugging; ensure any callers that assume hasInterfaceDecl remain valid or handle the exception. In `@jbct-core/src/main/java/org/pragmatica/jbct/shared/GitHubContentFetcher.java`: - Around line 46-59: Validate the repo, branch, and path inputs with the existing UrlValidation utility before building GitHub API URLs: in getLatestCommitSha, discoverFiles, fetchFileContent, and downloadFile call UrlValidation (or its appropriate method) to ensure each parameter is a safe, HTTPS/trusted-domain compliant value and reject or return a failure Result when validation fails; only construct the URI with validated values (and disallow characters like "../" or query fragments) so no untrusted parameter can be injected into the GitHub API URL. In `@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/CheckMojo.java`: - Around line 66-68: The method call chain on getLog() in CheckMojo should align the dotted method call with the end of the receiver; move the `.info(...)` so the dot is directly under the end of `getLog()` (i.e., break the line after `getLog()` and indent the next line so the `.` aligns to the last character of `getLog()`), ensuring the chained call `info(...)` remains unchanged otherwise. In `@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/CollectSliceDepsMojo.java`: - Around line 82-99: The code still reads and maps the now-unused manifest property "api.interface": remove the dead read props.getProperty("api.interface") and delete the conditional block that calls mappings.setProperty(apiInterface, value) and the accompanying getLog().debug("Found slice API...") call; update the CollectSliceDepsMojo class Javadoc to stop claiming it extracts api.interface and state that it only extracts impl.interface and slice.artifact instead so the documentation matches the current ManifestGenerator output. In `@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/DeploySlicesMojo.java`: - Line 101: In DeploySlicesMojo (class DeploySlicesMojo) there's a formatting issue: the try block is written as "try{"; update it to include a space before the brace ("try {") to match project style and other try blocks in the file. In `@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/FormatMojo.java`: - Around line 65-66: The chained call getLog().debug("Formatted: " + file) in class FormatMojo should have the chaining operator aligned with the receiver to meet coding guidelines; update the statement so the .debug(...) is placed on the same line as the receiver end or on the following line indented to align under getLog(), e.g., move the .debug call to align with the end of getLog() (or extract the nested callback into a separate method if preferred for readability) to correct the chain alignment. - Around line 42-43: The method chaining on getLog() violates the project's chain alignment rule; move the `.info(...)` call to its own line and align the leading `.` under the end of the `getLog()` receiver so the chain reads as getLog() followed by a newline and a dot-aligned `.info("Formatted: " + formatted.get() + ", Unchanged: " + unchanged.get() + ", Errors: " + errors.get())`; update the call in FormatMojo (referencing getLog() and .info(...) and the formatted/unchanged/errors getters) to use that alignment. In `@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/GenerateBlueprintMojo.java`: - Around line 133-155: The onPresent lambda calls resolveExternalDependencies(...) and catches MojoExecutionException only to wrap it in RuntimeException, which loses the checked exception and breaks Maven's error handling; change the flow so you don't throw RuntimeException from inside the lambda—e.g., when loadManifestFromDependency(...).onPresent(...) runs, store the depArtifact and depManifest (or the created SliceEntry) into a temporary collection, handle onEmpty the same way, and after the lambda processing finishes iterate that collection and call resolveExternalDependencies(depManifest, graph) from the outer method where you can declare/throw MojoExecutionException (or catch it and rethrow it directly) so the original MojoExecutionException propagates correctly; reference: loadManifestFromDependency, resolveExternalDependencies, SliceEntry, graph. In `@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/LintMojo.java`: - Around line 53-55: The chained method call alignment in LintMojo (getLog().info(...)) violates style: move the dot so the `.info(...)` call is vertically aligned under the end of `getLog()` (i.e., place the dot at the end of the `getLog()` line and indent the `.info(...)` line so its leading `.` lines up with the end of `getLog()`), ensuring the call remains `getLog()` followed on the next line by `.info("Lint results: " + errors.get() + " error(s), " + warnings.get() + " warning(s), " + infos.get() + " info(s)");` and preserves the original string expression. In `@pom.xml`: - Around line 55-58: The pom sets pragmatica-lite.version to 0.10.0 which conflicts with the project's guideline of using 0.9.10; either confirm the upgrade is intentional and document compatibility for modules that depend on "pragmatica-lite" (core, http-client, toml) or revert the pom property pragmatica-lite.version back to 0.9.10 so all modules use the guideline version; update the <pragmatica-lite.version> property and verify dependent modules (core, http-client, toml) build and tests pass (e.g., mvn test / dependency:tree) to ensure no breaking changes. In `@slice-processor/pom.xml`: - Around line 38-43: The pom uses an unavailable Aether version: update the aether.version Maven property from 0.8.0 to the published 0.7.4 so dependencies like org.pragmatica-lite.aether:infra-aspect (and other Aether artifacts) resolve; locate the <aether.version> property in the root POM and change its value to 0.7.4 (or revert to the published version) and then run a Maven build to verify resolution. In `@slice-processor/src/main/java/org/pragmatica/jbct/slice/routing/ErrorConflict.java`: - Around line 22-24: The compact constructor in ErrorConflict uses List.copyOf(matchingPatterns) which will NPE if matchingPatterns is null; modify the compact constructor (ErrorConflict) to guard against null by either: 1) validating with Objects.requireNonNull(matchingPatterns, "...") to fail fast with a clear message, or 2) defaulting null to an empty list (e.g., matchingPatterns == null ? List.of() : List.copyOf(matchingPatterns)) so matchingPatterns is never null after construction.🧹 Nitpick comments (49)
jbct-core/src/main/java/org/pragmatica/jbct/shared/SourceFile.java (1)
18-20: Chain formatting inconsistent with coding guidelines.The single-line chain exceeds typical line length (~115 chars) and doesn't follow the chain alignment convention (
.aligns to receiver end) used elsewhere in this file (e.g.,write()at lines 32-37,fileName()at lines 50-52).♻️ Suggested formatting to align with guidelines
public static Result<SourceFile> sourceFile(Path path) { - return lift(Causes::fromThrowable, () -> Files.readString(path)).map(content -> new SourceFile(path, content)); + return lift(Causes::fromThrowable, () -> Files.readString(path)) + .map(content -> new SourceFile(path, content)); }As per coding guidelines, chain alignment should have
.aligned to the receiver end.jbct-core/src/main/java/org/pragmatica/jbct/upgrade/GitHubReleaseChecker.java (1)
69-76: Chain alignment doesn't follow coding guidelines.Per the coding guidelines,
.should align to the receiver end. The.mapcall should align with the end ofcheckLatestRelease()for consistent chain alignment, similar to the pattern used in lines 57-59.♻️ Suggested fix for chain alignment
public Result<Option<ReleaseInfo>> checkForUpdate(String currentVersion) { return checkLatestRelease() - .map(release -> { - if (isNewerVersion(currentVersion, release.version())) { - return Option.option(release); - } - return Option.none(); - }); + .map(release -> { + if (isNewerVersion(currentVersion, release.version())) { + return Option.option(release); + } + return Option.none(); + }); }jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstAlwaysSuccessResultRule.java (2)
26-29: Chain alignment is inconsistent with coding guidelines.The
.mapand.orcalls should align to the receiver end. The current alignment places.mapat a different indentation level than expected.♻️ Suggested alignment
- var packageName = findFirst(root, RuleId.PackageDecl.class).flatMap(pd -> findFirst(pd, - RuleId.QualifiedName.class)) - .map(qn -> text(qn, source)) - .or(""); + var packageName = findFirst(root, RuleId.PackageDecl.class) + .flatMap(pd -> findFirst(pd, RuleId.QualifiedName.class)) + .map(qn -> text(qn, source)) + .or("");
40-42: Chain alignment inconsistent with coding guidelines.Per guidelines,
.should align to the receiver end.♻️ Suggested alignment
- return childByRule(method, RuleId.Type.class).map(type -> text(type, source)) - .filter(typeText -> typeText.startsWith("Result<")) - .isPresent(); + return childByRule(method, RuleId.Type.class) + .map(type -> text(type, source)) + .filter(typeText -> typeText.startsWith("Result<")) + .isPresent();jbct-core/src/main/java/org/pragmatica/jbct/init/ProjectInitializer.java (2)
91-91: Inconsistent constructor call formatting.This constructor call has 6 arguments on a single line, while similar calls elsewhere (lines 61-66, 74-79, 101-106) wrap parameters for readability and consistency.
♻️ Suggested formatting
- return new ProjectInitializer(projectDir, groupId, artifactId, basePackage, jbctVersion, pragmaticaVersion); + return new ProjectInitializer(projectDir, + groupId, + artifactId, + basePackage, + jbctVersion, + pragmaticaVersion);
114-119: Chain alignment does not follow coding guidelines.Per the coding guidelines,
.in method chains should align to the receiver end. The.flatMapshould align undercreateDirectories(), and the inner.mapshould align to its receiver.As per coding guidelines:
♻️ Suggested alignment
public Result<List<Path>> initialize() { return createDirectories() - .flatMap(_ -> Result.all(createTemplateFiles(), - createGitkeepFiles()) - .map(this::combineFileLists)); + .flatMap(_ -> Result.all(createTemplateFiles(), + createGitkeepFiles()) + .map(this::combineFileLists)); }slice-processor/src/test/java/org/pragmatica/jbct/slice/SliceProcessorTest.java (1)
1002-1005: Verify the identity extractor pattern generates valid lambda syntax.The assertion expects
request -> requestas the identity extractor. This is correct for lambda syntax, but consider also asserting the key type matches the request type to ensure the cache config is consistent.Optional: Add assertion for consistent key/value type in identity case
// Identity extractor (request type as key) assertThat(factoryContent).contains("new TypeToken<test.dto.GetUserRequest>() {}"); assertThat(factoryContent).contains("request -> request"); + + // Cache key type should match request type when using identity extractor + // This ensures CacheConfig<GetUserRequest, User> is generatedjbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/VerifySliceMojo.java (1)
140-143: Consider extracting ternary for cleaner formatting.The embedded ternary within string concatenation makes alignment awkward. Per coding guidelines, the
?and:should align to the opening(, but the deep nesting here reduces readability.♻️ Suggested refactor
+ var displayScope = scope == null ? "compile" : scope; if (runtimeGroupIds.contains(groupId) && !allowedNonProvided.contains(artifactId)) { if (!Artifact.SCOPE_PROVIDED.equals(scope) && !Artifact.SCOPE_TEST.equals(scope)) { - errors.add("Dependency " + groupId + ":" + artifactId + " must have 'provided' scope. " - + "Aether runtime libraries should not be bundled with slices. " + "Current scope: " + (scope == null - ? "compile" - : scope)); + errors.add("Dependency " + groupId + ":" + artifactId + " must have 'provided' scope. " + + "Aether runtime libraries should not be bundled with slices. " + + "Current scope: " + displayScope); } }jbct-core/src/main/java/org/pragmatica/jbct/init/SliceProjectValidator.java (1)
53-56: Chain alignment does not follow coding guidelines.Per the coding guidelines,
.should align to the receiver end. The.fold()should be indented to align withreadFile(pomFile).🔧 Suggested fix
return readFile(pomFile) - .fold(cause -> PartialResult.error("Failed to read pom.xml: " + cause.message()), - content -> checkPomContent(content)); + .fold(cause -> PartialResult.error("Failed to read pom.xml: " + cause.message()), + content -> checkPomContent(content));The same alignment issue applies to lines 78-80 and 107-109.
jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstConstructorBypassRule.java (1)
51-59: Minor formatting inconsistency with chain alignment guideline.The
.forEachon line 52 starts at column 8, which doesn't align to the receiver end (findAll(...)on line 51) as specified in the coding guidelines. Per the guidelines,.should align to the receiver end.However, this is a minor stylistic concern and doesn't affect correctness.
♻️ Suggested alignment (optional)
findAll(root, RuleId.RecordDecl.class) - .forEach(record -> { - var name = childByRule(record, RuleId.Identifier.class).map(id -> text(id, source)) - .or(""); - var recordText = text(record, source); - if (recordText.contains("Result<" + name + ">")) { - types.add(name); - } - }); + .forEach(record -> { + var name = childByRule(record, RuleId.Identifier.class).map(id -> text(id, source)) + .or(""); + var recordText = text(record, source); + if (recordText.contains("Result<" + name + ">")) { + types.add(name); + } + });jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstSealedErrorRule.java (2)
31-37: Consider method reference for cleaner filter predicate.Line 32's lambda
typeDecl -> hasInterfaceDecl(typeDecl)can be simplified to a method reference.♻️ Suggested simplification
return findAll(root, RuleId.TypeDecl.class).stream() - .filter(typeDecl -> hasInterfaceDecl(typeDecl)) + .filter(this::hasInterfaceDecl) .filter(typeDecl -> extendsCause(typeDecl, source)) .filter(typeDecl -> !isSealed(typeDecl, source))
48-53: String-based heuristic may have edge-case false positives.The
contains("extends Cause")check could match:
- Interfaces with "Cause" in their name (e.g.,
interface CauseMapper extends Something)- Comments within the interface text containing "Cause"
The first condition
contains("extends Cause")is also redundant—it's always true when the second conditioncontains("extends ") && contains("Cause")matches with adjacent text.This is acceptable for a lint heuristic, but worth noting as a known limitation.
♻️ Simplified condition (removes redundancy)
private boolean extendsCause(CstNode typeDecl, String source) { - // Check if interface extends Cause by examining the InterfaceDecl text - return childByRule(typeDecl, RuleId.InterfaceDecl.class).map(iface -> text(iface, source)) - .map(ifaceText -> ifaceText.contains("extends Cause") || ifaceText.contains("extends ") && ifaceText.contains("Cause")) - .or(false); + // Check if interface extends Cause by examining the InterfaceDecl text + return childByRule(typeDecl, RuleId.InterfaceDecl.class).map(iface -> text(iface, source)) + .map(ifaceText -> ifaceText.contains("extends ") && ifaceText.contains("Cause")) + .or(false); }jbct-cli/src/main/java/org/pragmatica/jbct/cli/UpgradeCommand.java (1)
95-99: Consider importingPathinstead of using FQCN.The fully qualified
java.nio.file.Pathis inconsistent with the rest of the file which uses imports. Adding the import would improve readability.Suggested change
Add to imports section:
import java.nio.file.Path;Then update method signature:
- private Result<String> installScriptsAndDownload(java.nio.file.Path installDir) { + private Result<String> installScriptsAndDownload(Path installDir) {jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/CollectSliceDepsMojo.java (1)
63-63: Add space before opening brace.Standard Java formatting requires a space before the opening brace of a
tryblock.🎨 Suggested formatting fix
- try{ + try {jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstFullyQualifiedNameRule.java (1)
32-35: Chain alignment may not follow coding guidelines.Per the coding guidelines, chain
.should align to the receiver end. The current alignment of.mapand.oron lines 34-35 appears inconsistent with the receiver.♻️ Suggested alignment fix
- var packageName = findFirst(root, RuleId.PackageDecl.class).flatMap(pd -> findFirst(pd, - RuleId.QualifiedName.class)) - .map(qn -> text(qn, source)) - .or(""); + var packageName = findFirst(root, RuleId.PackageDecl.class) + .flatMap(pd -> findFirst(pd, RuleId.QualifiedName.class)) + .map(qn -> text(qn, source)) + .or("");jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/GenerateBlueprintMojo.java (2)
186-191: Inconsistent chain alignment.Per coding guidelines,
.should align to the receiver end. The current formatting has mixed indentation levels for the chainedequalscalls.✏️ Suggested alignment fix
for (var artifact : project.getArtifacts()) { - if (artifact.getGroupId() - .equals(groupId) && - artifact.getArtifactId() - .equals(artifactId) && - artifact.getVersion() - .equals(version)) { + if (artifact.getGroupId().equals(groupId) && + artifact.getArtifactId().equals(artifactId) && + artifact.getVersion().equals(version)) { return loadManifestFromJar(artifact.getFile()); } }
321-324: Minor: Missing space before opening brace.✏️ Formatting fix
- try{ + try { Files.createDirectories(blueprintFile.toPath() .getParent());jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstValueObjectFactoryRule.java (1)
41-47: Consider CST-based factory detection to avoid string-match false negatives.
recordText.contains(...)can be fooled by comments/strings or formatting, and now fully gates diagnostics. Consider inspectingMethodDeclreturn types via CST nodes instead of substring checks. As per coding guidelines, use CST-based analysis with RuleId pattern matching.jbct-core/src/main/java/org/pragmatica/jbct/init/GitHubVersionResolver.java (1)
98-100: HandlesaveCache()failures to avoid silent cache persistence issues.
updateCache()currently ignores theResultfromsaveCache(), so IO failures are swallowed and you can end up repeatedly hitting GitHub without any signal. Consider at least logging or propagating that failure.Also applies to: 102-107
jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/PackageSlicesMojo.java (3)
117-120: Duplicate JAR reading for slice dependencies.
isSliceDependency()reads the slice manifest, and thentoSliceArtifactInfo()reads it again. For projects with many slice dependencies, this doubles I/O operations.Consider caching the manifest or refactoring to return the manifest data from
isSliceDependency.♻️ Suggested refactor to avoid duplicate reads
- } else if (isSliceDependency(artifact) && isDirectDependency) { - // Slice dependencies (direct only) - // Read actual artifact names from manifest (not Maven artifact ID) - sliceDeps.add(toSliceArtifactInfo(artifact)); + } else if (isDirectDependency) { + var sliceManifest = readSliceManifest(artifact); + if (sliceManifest != null) { + // Slice dependencies (direct only) + // Read actual artifact names from manifest (not Maven artifact ID) + sliceDeps.add(toSliceArtifactInfo(artifact, sliceManifest)); + }Then update
toSliceArtifactInfoto accept the pre-read manifest:private ArtifactInfo toSliceArtifactInfo(Artifact artifact, java.util.Properties props) { var sliceArtifact = props.getProperty("slice.artifact"); if (sliceArtifact != null && sliceArtifact.contains(":")) { var parts = sliceArtifact.split(":"); return new ArtifactInfo(parts[0], parts[1], toSemverRange(artifact.getVersion())); } return toArtifactInfo(artifact); }
132-138: Consider importingSetandHashSetfor consistency.The file imports
ArrayListandListbut uses fully-qualified names forSetandHashSet. Consider adding imports for consistency.
422-433: Consider making dependency versions configurable.The pragmatica-lite core (
0.9.10) and slice-api (0.1.0) versions are hardcoded. When these dependencies are upgraded, this code must be manually updated.Consider deriving versions from project dependencies or exposing them as plugin configuration parameters.
♻️ Example: Add plugin parameters for versions
`@Parameter`(property = "jbct.pragmaticaLiteVersion", defaultValue = "0.9.10") private String pragmaticaLiteVersion; `@Parameter`(property = "jbct.sliceApiVersion", defaultValue = "0.1.0") private String sliceApiVersion;Then use these in
generatePomContent:- sb.append(" <version>0.9.10</version>\n"); + sb.append(" <version>").append(pragmaticaLiteVersion).append("</version>\n");jbct-core/src/test/java/org/pragmatica/jbct/format/GoldenFormatterTest.java (2)
127-134: Test assertions verify presence but not alignment.The test is named
chainAlignment_alignsToDotPositionbut the assertions only check that chain elements are present (.contains()), not that they're properly aligned. Per the coding guidelines, chain alignment should have.aligned to the receiver end.Consider adding an assertion that verifies the actual alignment behavior, e.g., checking that
.map(appears at a specific column or that the formatted output matches an expected aligned structure.
158-167: Same observation: presence-only checks don't verify alignment.The test
argumentAlignment_alignsToOpeningParenonly verifies thatResult.all(,first(),second(), andthird()are present, but doesn't verify they're aligned to the opening parenthesis as the test name suggests.jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstLambdaTernaryRule.java (1)
26-29: Chain alignment doesn't follow coding guidelines.Per the coding guidelines, chain alignment should have
.aligned to the receiver end. The current formatting has inconsistent indentation:var packageName = findFirst(root, RuleId.PackageDecl.class).flatMap(pd -> findFirst(pd, RuleId.QualifiedName.class)) .map(qn -> text(qn, source)) .or("");The
.map()at line 28 appears to be indented to a different column than the receiver end.♻️ Suggested alignment
- var packageName = findFirst(root, RuleId.PackageDecl.class).flatMap(pd -> findFirst(pd, - RuleId.QualifiedName.class)) - .map(qn -> text(qn, source)) - .or(""); + var packageName = findFirst(root, RuleId.PackageDecl.class) + .flatMap(pd -> findFirst(pd, RuleId.QualifiedName.class)) + .map(qn -> text(qn, source)) + .or("");jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstConditionalLoggingRule.java (1)
45-50: Inconsistent alignment in boolean conditions.The continuation lines in this boolean expression don't align consistently. Based on coding guidelines, the operators and subsequent conditions should align to the opening parenthesis or maintain consistent indentation.
🔧 Suggested alignment fix
- return ( stmtText.contains("isDebugEnabled") || - stmtText.contains("isTraceEnabled") || - stmtText.contains("isInfoEnabled")) && - (stmtText.contains(".debug(") || - stmtText.contains(".trace(") || - stmtText.contains(".info(")); + return (stmtText.contains("isDebugEnabled") + || stmtText.contains("isTraceEnabled") + || stmtText.contains("isInfoEnabled")) + && (stmtText.contains(".debug(") + || stmtText.contains(".trace(") + || stmtText.contains(".info("));jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstChainLengthRule.java (1)
45-45: Minor: Unusual spacing in comparison.The comparison
!= - 1has an extra space before the negative sign. While this works, conventional style is!= -1.🔧 Suggested fix
- while ((idx = stmtText.indexOf(".", idx)) != - 1) { + while ((idx = stmtText.indexOf(".", idx)) != -1) {jbct-core/src/main/java/org/pragmatica/jbct/lint/LintConfig.java (1)
133-136: Minor redundancy in builder methods.The builder methods
withRuleSeverityandwithDisabledRulecallMap.copyOf/Set.copyOfbefore passing to the constructor, which will copy again. This is harmless but slightly wasteful.🔧 Suggested simplification
public LintConfig withRuleSeverity(String ruleId, DiagnosticSeverity severity) { var newSeverities = new HashMap<>(ruleSeverities); newSeverities.put(ruleId, severity); - return lintConfig(Map.copyOf(newSeverities), disabledRules, failOnWarning); + return lintConfig(newSeverities, disabledRules, failOnWarning); } public LintConfig withDisabledRule(String ruleId) { var newDisabled = new HashSet<>(disabledRules); newDisabled.add(ruleId); - return lintConfig(ruleSeverities, Set.copyOf(newDisabled), failOnWarning); + return lintConfig(ruleSeverities, newDisabled, failOnWarning); }Also applies to: 142-145
jbct-core/src/test/java/org/pragmatica/jbct/init/SliceProjectInitializerTest.java (2)
44-49: Test continues after potential assertion failure.If
result.isSuccess()returns false, the assertion at line 46 will fail but the test execution continues to line 49 whereFiles.readString()will throw an exception because the file won't exist. This masks the actual failure cause.Consider using the
onSuccesscallback pattern (like inshould_create_slice_project_structureat lines 22-38) or add a guard return.♻️ Suggested fix using guard pattern
var result = SliceProjectInitializer.sliceProjectInitializer(projectDir, "com.test", "test-slice") .flatMap(SliceProjectInitializer::initialize); assertThat(result.isSuccess()) .as("Project initialization should succeed") .isTrue(); + if (result.isFailure()) { + return; + } var pomContent = Files.readString(projectDir.resolve("pom.xml"));
67-73: Same issue: test continues after potential assertion failure.Same concern as the previous test - if initialization fails,
Files.readStringat line 73 will throw a misleadingNoSuchFileExceptioninstead of showing the actual initialization failure.♻️ Suggested fix
var result = SliceProjectInitializer.sliceProjectInitializer(projectDir, "org.example", "inventory-service") .flatMap(SliceProjectInitializer::initialize); assertThat(result.isSuccess()) .as("Project initialization should succeed") .isTrue(); + if (result.isFailure()) { + return; + } var sliceFile = projectDir.resolve("src/main/java/org/example/inventoryservice/InventoryService.java");jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstLambdaComplexityRule.java (3)
50-54: Control flow detection may trigger false positives on string literals.The text-based pattern matching (
lambdaText.contains("if ")) will match occurrences inside string literals, e.g.,s -> log("if condition is met")would incorrectly flag as containing control flow.Consider extracting string literals before checking, or accept this as a known limitation since complex lambdas with such strings are rare.
56-70: Statement counting includes nested blocks.
countStatementscounts all semicolons within the lambda body, including those inside nested blocks (e.g., inner lambdas, anonymous classes). A lambda likes -> { list.forEach(x -> { doA(); doB(); }); }would count 3 semicolons even though the outer block has only 1 statement.This may be acceptable since nested complexity is still complexity, but worth documenting in the class javadoc if intentional.
72-90: String escape handling doesn't account for escaped backslashes.Line 79 checks
blockBody.charAt(i - 1) != '\\'to detect escaped quotes, but this fails for"\\"(escaped backslash followed by closing quote). The sequence\\\"should close the string, but this logic would treat the quote as escaped.For a lint rule, this edge case is rare and the impact is minor (potential false negative).
jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstNullableParameterRule.java (1)
33-34: Consider extending pattern to detectObjects.isNull/Objects.nonNull.The regex detects
param == nullpatterns but missesObjects.isNull(param)andObjects.nonNull(param)which are equivalent null checks. These are less common but still indicate nullable parameters.jbct-core/src/main/java/org/pragmatica/jbct/shared/GitHubContentFetcher.java (2)
125-132: SHA extraction regex is reasonable but could be more specific.The current pattern
"sha"\s*:\s*"([^"]+)"will match the first"sha"field in the response. For the commits endpoint, this should work, but if the response structure changes or contains multipleshafields, it may extract the wrong value.
134-141: Regex-based JSON parsing lacks robustness for production use.The current regex approach works for GitHub's stable API format but is fragile against field reordering, escaped characters in paths, and other JSON edge cases. The project already has
pragmatica-liteJSON support available through transitive dependencies.Consider leveraging the
FileInforecord already defined in this file with a proper JSON parser:Suggested refactor
// Use pragmatica-lite JSON support (JsonMapper available via pragmatica-lite:core) // or parse structured JSON to populate FileInfo objects List<FileInfo> files = parseAsJson(body); // implement JSON parsing return files.stream() .filter(FileInfo::isBlob) .map(FileInfo::path) .filter(path -> path.startsWith(pathPrefix)) .toList();docs/slice/README.md (1)
60-66: Consider adding a language specifier to the fenced code block.The ASCII diagram code block lacks a language specifier. Adding
```textwould satisfy markdown linting rules and improve clarity.📝 Suggested fix
-``` +```text `@Slice` interface -> Annotation Processor -> Generated code + manifests | Maven Plugin -> Slice JAR | Blueprint Generator -> blueprint.toml</details> </blockquote></details> <details> <summary>slice-processor/src/main/java/org/pragmatica/jbct/slice/routing/RouteSourceGenerator.java (1)</summary><blockquote> `188-188`: **Empty catch blocks are intentional but could benefit from a comment.** The empty catches for `IOException` and `IllegalArgumentException` are valid since the service file may not exist on first generation. A brief inline comment would clarify this is intentional. <details> <summary>📝 Suggested improvement</summary> ```diff - } catch (IOException _) {} catch (IllegalArgumentException _) {} + } catch (IOException | IllegalArgumentException _) { + // File may not exist yet - expected during first generation + }jbct-core/src/main/java/org/pragmatica/jbct/update/AiToolsUpdater.java (2)
16-18: Import ordering doesn't follow guidelines.Per coding guidelines, imports should be grouped as: org.pragmatica, java/javax, static. The SLF4J imports should be placed before the java imports.
♻️ Suggested import reordering
import org.pragmatica.http.HttpOperations; import org.pragmatica.jbct.shared.GitHubContentFetcher; import org.pragmatica.jbct.shared.HttpClients; import org.pragmatica.jbct.shared.PathValidation; import org.pragmatica.lang.Option; import org.pragmatica.lang.Result; import org.pragmatica.lang.Unit; import org.pragmatica.lang.utils.Causes; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.List; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory;
57-62: Consider simplifying the update-needed logic.The current flow (filter for match → map to none → fallback to some) is correct but reads as a double-negative. A more direct approach would improve clarity.
♻️ Simpler alternative
private Option<String> checkIfUpdateNeeded(String latestSha) { var currentSha = getCurrentVersion(); - return currentSha.filter(sha -> sha.equals(latestSha)) - .map(_ -> Option.<String>none()) - .or(() -> Option.option(latestSha)); + var isUpToDate = currentSha.filter(sha -> sha.equals(latestSha)) + .isPresent(); + return isUpToDate + ? Option.none() + : Option.some(latestSha); }jbct-core/src/main/java/org/pragmatica/jbct/shared/UrlValidation.java (1)
50-54: UseLocale.ROOTfor case-insensitive domain comparison.
toLowerCase()without a locale argument uses the default system locale, which can produce unexpected results with certain locales (e.g., Turkish 'I' → 'ı'). For domain name comparison, useLocale.ROOT.♻️ Proposed fix
- var hostLower = host.toLowerCase(); + var hostLower = host.toLowerCase(java.util.Locale.ROOT);slice-processor/src/main/java/org/pragmatica/jbct/slice/routing/RouteConfigLoader.java (2)
119-123: Consider usingCollectors.toUnmodifiableMapinstead of manual collection.The helper can be simplified using built-in collectors.
♻️ Simplified alternative
- private static Map<String, RouteDsl> toImmutableMap(List<Map.Entry<String, RouteDsl>> entries) { - var map = new HashMap<String, RouteDsl>(); - entries.forEach(e -> map.put(e.getKey(), e.getValue())); - return Map.copyOf(map); - } + private static Map<String, RouteDsl> toImmutableMap(List<Map.Entry<String, RouteDsl>> entries) { + return entries.stream() + .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, + Map.Entry::getValue)); + }
98-101: Parse errors for slice config are silently discarded.Using
.option()converts parse failures toOption.none(), treating them the same as missing files. Consider logging a warning when the file exists but fails to parse, to help users diagnose configuration issues.♻️ Add warning for parse failures
var sliceConfig = Files.exists(slicePath) - ? load(slicePath).option() + ? load(slicePath) + .onFailure(cause -> LOGGER.log(WARNING, + "Failed to parse " + slicePath + ": " + cause.message())) + .option() : Option.<RouteConfig>none();slice-processor/src/main/java/org/pragmatica/jbct/slice/generator/ManifestGenerator.java (3)
188-189: Line exceeds reasonable length - consider breaking up.This single line is very long and hard to read. Consider extracting to a Set or breaking the conditions.
♻️ Improved readability
+ private static final Set<String> STANDARD_PREFIXES = Set.of("java.lang.", "java.util."); + private static final Set<String> PRIMITIVE_TYPES = Set.of("void", "int", "long", "boolean", "double", "float"); + 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 STANDARD_PREFIXES.stream().anyMatch(typeName::startsWith) + || PRIMITIVE_TYPES.contains(typeName); }
66-70: Minor duplication withgetArtifactFromEnv.Both methods retrieve
groupIdandartifactIdfrom options. Consider extracting the common base artifact computation.♻️ Extract common logic
private String getArtifactFromEnv() { - var groupId = options.getOrDefault("slice.groupId", "unknown"); - var artifactId = options.getOrDefault("slice.artifactId", "unknown"); - return groupId + ":" + artifactId; + return getBaseArtifact(); } private String getSliceArtifact(String sliceName) { + return getBaseArtifact() + "-" + toKebabCase(sliceName); + } + + private String getBaseArtifact() { var groupId = options.getOrDefault("slice.groupId", "unknown"); var artifactId = options.getOrDefault("slice.artifactId", "unknown"); - return groupId + ":" + artifactId + "-" + toKebabCase(sliceName); + return groupId + ":" + artifactId; }
112-117: Empty string for unresolved artifact may cause issues.Using
""as fallback for missingsliceArtifact(line 114) while using"UNRESOLVED"for version (line 117) is inconsistent. Consider using a consistent sentinel value.♻️ Consistent fallback values
props.setProperty(prefix + "artifact", resolved.sliceArtifact() - .or(() -> "")); + .or(() -> "UNRESOLVED"));jbct-core/src/main/java/org/pragmatica/jbct/shared/SecurityError.java (1)
5-6: Remove unused imports.
URIandPathare imported but never used in this file.🧹 Proposed fix
package org.pragmatica.jbct.shared; import org.pragmatica.lang.Cause; - -import java.net.URI; -import java.nio.file.Path;docs/slice/development-guide.md (1)
96-99: Add language specifier to fenced code block.The error message example should have a language specifier for consistent formatting.
📝 Proposed fix
-``` +```text Dependency org.pragmatica-lite:core must have 'provided' scope. Aether runtime libraries should not be bundled with slices.</details> </blockquote></details> <details> <summary>slice-processor/src/main/java/org/pragmatica/jbct/slice/generator/FactoryClassGenerator.java (1)</summary><blockquote> `398-406`: **Consider validating artifact resolution and failing early in code generation.** Using `"UNRESOLVED"` as a fallback for `fullArtifact()` generates compilable code that fails at runtime. Since artifacts are resolved during `generateFactoryClass()` before code generation begins, add validation to return `Result.failure()` from `generate()` when `fullArtifact()` is empty, preventing runtime failures with unresolved dependencies. </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ## Governing Documents | ||
|
|
||
| RFCs location: `../coding-technology/proposals/rfc/` | ||
| Remote: https://github.com/siy/coding-technology/tree/master/proposals/rfc |
There was a problem hiding this comment.
Wrap the bare URL to satisfy markdownlint (MD034).
Angle brackets or a labeled link will clear the lint warning.
✏️ Suggested fix
-Remote: https://github.com/siy/coding-technology/tree/master/proposals/rfc
+Remote: <https://github.com/siy/coding-technology/tree/master/proposals/rfc>📝 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.
| Remote: https://github.com/siy/coding-technology/tree/master/proposals/rfc | |
| Remote: <https://github.com/siy/coding-technology/tree/master/proposals/rfc> |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
6-6: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In `@CLAUDE.md` at line 6, The bare URL
"https://github.com/siy/coding-technology/tree/master/proposals/rfc" in
CLAUDE.md is triggering markdownlint MD034; change it by either wrapping it in
angle brackets
(<https://github.com/siy/coding-technology/tree/master/proposals/rfc>) or
turning it into a labeled markdown link (e.g.
[proposals/rfc](https://github.com/siy/coding-technology/tree/master/proposals/rfc))
so the linter warning is cleared.
| public JbctConfig merge(Option<JbctConfig> other) { | ||
| return other.map(o -> { | ||
| // Merge formatter config (use other if different from default) | ||
| var mergedFormatter = this.formatter; | ||
| if (!other.formatter.equals(FormatterConfig.DEFAULT)) { | ||
| mergedFormatter = other.formatter; | ||
| } | ||
| // Merge lint config (use other if different from default) | ||
| if (!o.formatter.equals(FormatterConfig.DEFAULT)) { | ||
| mergedFormatter = o.formatter; | ||
| } | ||
| // Merge lint config (use other if different from default) | ||
| var mergedLint = this.lint; | ||
| if (!other.lint.equals(LintConfig.DEFAULT)) { | ||
| mergedLint = other.lint; | ||
| } | ||
| // Merge source directories (use other if not default) | ||
| if (!o.lint.equals(LintConfig.DEFAULT)) { | ||
| mergedLint = o.lint; | ||
| } | ||
| // Merge source directories (use other if not default) | ||
| var mergedSourceDirs = this.sourceDirectories; | ||
| if (!other.sourceDirectories.equals(List.of("src/main/java"))) { | ||
| mergedSourceDirs = other.sourceDirectories; | ||
| } | ||
| // Merge business packages (use other if not default) | ||
| if (!o.sourceDirectories.equals(List.of("src/main/java"))) { | ||
| mergedSourceDirs = o.sourceDirectories; | ||
| } | ||
| // Merge business packages (use other if not default) | ||
| var mergedBusinessPackages = this.businessPackages; | ||
| if (!other.businessPackages.equals(List.of("**.usecase.**", "**.domain.**"))) { | ||
| mergedBusinessPackages = other.businessPackages; | ||
| } | ||
| // Merge slice packages (use other if not empty) | ||
| if (!o.businessPackages.equals(List.of("**.usecase.**", "**.domain.**"))) { | ||
| mergedBusinessPackages = o.businessPackages; | ||
| } | ||
| // Merge slice packages (use other if not empty) | ||
| var mergedSlicePackages = this.slicePackages; | ||
| if (!other.slicePackages.isEmpty()) { | ||
| mergedSlicePackages = other.slicePackages; | ||
| } | ||
| return jbctConfig(mergedFormatter, mergedLint, mergedSourceDirs, mergedBusinessPackages, mergedSlicePackages); | ||
| if (!o.slicePackages.isEmpty()) { | ||
| mergedSlicePackages = o.slicePackages; | ||
| } | ||
| return jbctConfig(mergedFormatter, | ||
| mergedLint, | ||
| mergedSourceDirs, | ||
| mergedBusinessPackages, | ||
| mergedSlicePackages); | ||
| }) | ||
| .or(this); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Severe indentation inconsistency in lambda body.
The code inside the lambda has mixed indentation - some lines (e.g., var mergedFormatter, var mergedLint) are flush left relative to the lambda body while the surrounding comments and conditionals are indented differently. Per the coding guidelines, "Lambda in broken args: Body aligns to parameter + 4 spaces."
🔧 Suggested fix
public JbctConfig merge(Option<JbctConfig> other) {
return other.map(o -> {
- // Merge formatter config (use other if different from default)
- var mergedFormatter = this.formatter;
- if (!o.formatter.equals(FormatterConfig.DEFAULT)) {
- mergedFormatter = o.formatter;
- }
- // Merge lint config (use other if different from default)
- var mergedLint = this.lint;
- if (!o.lint.equals(LintConfig.DEFAULT)) {
- mergedLint = o.lint;
- }
- // Merge source directories (use other if not default)
- var mergedSourceDirs = this.sourceDirectories;
- if (!o.sourceDirectories.equals(List.of("src/main/java"))) {
- mergedSourceDirs = o.sourceDirectories;
- }
- // Merge business packages (use other if not default)
- var mergedBusinessPackages = this.businessPackages;
- if (!o.businessPackages.equals(List.of("**.usecase.**", "**.domain.**"))) {
- mergedBusinessPackages = o.businessPackages;
- }
- // Merge slice packages (use other if not empty)
- var mergedSlicePackages = this.slicePackages;
- if (!o.slicePackages.isEmpty()) {
- mergedSlicePackages = o.slicePackages;
- }
- return jbctConfig(mergedFormatter,
- mergedLint,
- mergedSourceDirs,
- mergedBusinessPackages,
- mergedSlicePackages);
- })
- .or(this);
+ // Merge formatter config (use other if different from default)
+ var mergedFormatter = this.formatter;
+ if (!o.formatter.equals(FormatterConfig.DEFAULT)) {
+ mergedFormatter = o.formatter;
+ }
+ // Merge lint config (use other if different from default)
+ var mergedLint = this.lint;
+ if (!o.lint.equals(LintConfig.DEFAULT)) {
+ mergedLint = o.lint;
+ }
+ // Merge source directories (use other if not default)
+ var mergedSourceDirs = this.sourceDirectories;
+ if (!o.sourceDirectories.equals(List.of("src/main/java"))) {
+ mergedSourceDirs = o.sourceDirectories;
+ }
+ // Merge business packages (use other if not default)
+ var mergedBusinessPackages = this.businessPackages;
+ if (!o.businessPackages.equals(List.of("**.usecase.**", "**.domain.**"))) {
+ mergedBusinessPackages = o.businessPackages;
+ }
+ // Merge slice packages (use other if not empty)
+ var mergedSlicePackages = this.slicePackages;
+ if (!o.slicePackages.isEmpty()) {
+ mergedSlicePackages = o.slicePackages;
+ }
+ return jbctConfig(mergedFormatter,
+ mergedLint,
+ mergedSourceDirs,
+ mergedBusinessPackages,
+ mergedSlicePackages);
+ })
+ .or(this);
}📝 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 JbctConfig merge(Option<JbctConfig> other) { | |
| return other.map(o -> { | |
| // Merge formatter config (use other if different from default) | |
| var mergedFormatter = this.formatter; | |
| if (!other.formatter.equals(FormatterConfig.DEFAULT)) { | |
| mergedFormatter = other.formatter; | |
| } | |
| // Merge lint config (use other if different from default) | |
| if (!o.formatter.equals(FormatterConfig.DEFAULT)) { | |
| mergedFormatter = o.formatter; | |
| } | |
| // Merge lint config (use other if different from default) | |
| var mergedLint = this.lint; | |
| if (!other.lint.equals(LintConfig.DEFAULT)) { | |
| mergedLint = other.lint; | |
| } | |
| // Merge source directories (use other if not default) | |
| if (!o.lint.equals(LintConfig.DEFAULT)) { | |
| mergedLint = o.lint; | |
| } | |
| // Merge source directories (use other if not default) | |
| var mergedSourceDirs = this.sourceDirectories; | |
| if (!other.sourceDirectories.equals(List.of("src/main/java"))) { | |
| mergedSourceDirs = other.sourceDirectories; | |
| } | |
| // Merge business packages (use other if not default) | |
| if (!o.sourceDirectories.equals(List.of("src/main/java"))) { | |
| mergedSourceDirs = o.sourceDirectories; | |
| } | |
| // Merge business packages (use other if not default) | |
| var mergedBusinessPackages = this.businessPackages; | |
| if (!other.businessPackages.equals(List.of("**.usecase.**", "**.domain.**"))) { | |
| mergedBusinessPackages = other.businessPackages; | |
| } | |
| // Merge slice packages (use other if not empty) | |
| if (!o.businessPackages.equals(List.of("**.usecase.**", "**.domain.**"))) { | |
| mergedBusinessPackages = o.businessPackages; | |
| } | |
| // Merge slice packages (use other if not empty) | |
| var mergedSlicePackages = this.slicePackages; | |
| if (!other.slicePackages.isEmpty()) { | |
| mergedSlicePackages = other.slicePackages; | |
| } | |
| return jbctConfig(mergedFormatter, mergedLint, mergedSourceDirs, mergedBusinessPackages, mergedSlicePackages); | |
| if (!o.slicePackages.isEmpty()) { | |
| mergedSlicePackages = o.slicePackages; | |
| } | |
| return jbctConfig(mergedFormatter, | |
| mergedLint, | |
| mergedSourceDirs, | |
| mergedBusinessPackages, | |
| mergedSlicePackages); | |
| }) | |
| .or(this); | |
| } | |
| public JbctConfig merge(Option<JbctConfig> other) { | |
| return other.map(o -> { | |
| // Merge formatter config (use other if different from default) | |
| var mergedFormatter = this.formatter; | |
| if (!o.formatter.equals(FormatterConfig.DEFAULT)) { | |
| mergedFormatter = o.formatter; | |
| } | |
| // Merge lint config (use other if different from default) | |
| var mergedLint = this.lint; | |
| if (!o.lint.equals(LintConfig.DEFAULT)) { | |
| mergedLint = o.lint; | |
| } | |
| // Merge source directories (use other if not default) | |
| var mergedSourceDirs = this.sourceDirectories; | |
| if (!o.sourceDirectories.equals(List.of("src/main/java"))) { | |
| mergedSourceDirs = o.sourceDirectories; | |
| } | |
| // Merge business packages (use other if not default) | |
| var mergedBusinessPackages = this.businessPackages; | |
| if (!o.businessPackages.equals(List.of("**.usecase.**", "**.domain.**"))) { | |
| mergedBusinessPackages = o.businessPackages; | |
| } | |
| // Merge slice packages (use other if not empty) | |
| var mergedSlicePackages = this.slicePackages; | |
| if (!o.slicePackages.isEmpty()) { | |
| mergedSlicePackages = o.slicePackages; | |
| } | |
| return jbctConfig(mergedFormatter, | |
| mergedLint, | |
| mergedSourceDirs, | |
| mergedBusinessPackages, | |
| mergedSlicePackages); | |
| }) | |
| .or(this); | |
| } |
🤖 Prompt for AI Agents
In `@jbct-core/src/main/java/org/pragmatica/jbct/config/JbctConfig.java` around
lines 113 - 147, The lambda body in JbctConfig.merge(Option<JbctConfig> other)
has inconsistent indentation; reformat the entire lambda so its body lines
(including comments and statements that set mergedFormatter, mergedLint,
mergedSourceDirs, mergedBusinessPackages, mergedSlicePackages and the return
jbctConfig(...) call) are indented consistently according to the project's rule
(lambda body aligned to the parameter + 4 spaces), ensuring comments align with
the statements they describe.
| * <p>Uses the generated Java25Parser for parsing and preserves trivia (whitespace/comments). | ||
| * | ||
| * <p><b>Thread Safety:</b> Thread-safe for concurrent use. While the underlying parser | ||
| * and printer create per-operation state, instances of this class can be safely shared | ||
| * across threads. Each {@link #format(SourceFile)} call creates its own parser and | ||
| * printer instances internally. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and read CstFormatter.java to understand current implementation
fd -a "CstFormatter.java" . | head -1 | xargs cat -nRepository: siy/jbct-cli
Length of output: 3801
🏁 Script executed:
#!/bin/bash
# Inspect Java25Parser for mutable state and thread-safety concerns
file=$(fd -a "Java25Parser.java" .)
if [ -n "$file" ]; then
echo "=== Java25Parser class structure ==="
cat -n "$file" | head -100
else
echo "Java25Parser not found"
fiRepository: siy/jbct-cli
Length of output: 3341
Thread-safety javadoc is incorrect and creates data race risk.
Line 17–20 claim per-call parser creation and thread-safe concurrent use. However, CstFormatter maintains a shared Java25Parser instance (line 24) initialized once in the constructor (line 28) and reused across all format() calls (line 51). Java25Parser's own javadoc explicitly states: "This class is NOT thread-safe. Each thread must use its own instance." Concurrent format() calls on the same CstFormatter instance will race on the shared parser's mutable state, violating the documented guarantee.
Fix by instantiating a new parser per call:
Proposed fix
- private final Java25Parser parser;
-
private CstFormatter(FormatterConfig config) {
this.config = config;
- this.parser = new Java25Parser();
}
private Result<CstNode> parse(SourceFile source) {
- var result = parser.parseWithDiagnostics(source.content());
+ var result = new Java25Parser().parseWithDiagnostics(source.content());🤖 Prompt for AI Agents
In `@jbct-core/src/main/java/org/pragmatica/jbct/format/cst/CstFormatter.java`
around lines 15 - 20, The class-level javadoc wrongly claims thread-safety while
CstFormatter holds a shared Java25Parser instance; to fix, stop reusing the
single parser field and create a fresh Java25Parser per invocation of
format(SourceFile): remove or stop referencing the shared parser field (the
constructor initialization), and instantiate a new Java25Parser inside the
format(...) method (or use a ThreadLocal supplier) so each call gets its own
parser instance and no mutable parser state is shared across threads.
| return findAncestor(root, node, RuleId.ClassMember.class).map(member -> { | ||
| var memberText = text(member, source); | ||
| // Allow in factory methods (static methods returning Result) | ||
| return memberText.contains("static ") && memberText.contains("Result<"); | ||
| }) | ||
| }) | ||
| .or(false); |
There was a problem hiding this comment.
Inconsistent indentation within lambda body.
The reformatting has created visually broken indentation in this lambda:
- Lines 77-78: Content at column ~74 (aligned after
(member -> {) - Line 79:
returnstatement at column 8 (minimal indentation) - Line 80: Closing
})at column ~70
Line 79 appears to have been missed during the reformatting, resulting in jarring inconsistency within the same block. This significantly impacts readability.
🔧 Suggested fix to align the lambda body consistently
- return findAncestor(root, node, RuleId.ClassMember.class).map(member -> {
- var memberText = text(member, source);
- // Allow in factory methods (static methods returning Result)
- return memberText.contains("static ") && memberText.contains("Result<");
- })
- .or(false);
+ return findAncestor(root, node, RuleId.ClassMember.class)
+ .map(member -> {
+ var memberText = text(member, source);
+ // Allow in factory methods (static methods returning Result)
+ return memberText.contains("static ") && memberText.contains("Result<");
+ })
+ .or(false);📝 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.
| return findAncestor(root, node, RuleId.ClassMember.class).map(member -> { | |
| var memberText = text(member, source); | |
| // Allow in factory methods (static methods returning Result) | |
| return memberText.contains("static ") && memberText.contains("Result<"); | |
| }) | |
| }) | |
| .or(false); | |
| return findAncestor(root, node, RuleId.ClassMember.class) | |
| .map(member -> { | |
| var memberText = text(member, source); | |
| // Allow in factory methods (static methods returning Result) | |
| return memberText.contains("static ") && memberText.contains("Result<"); | |
| }) | |
| .or(false); |
🤖 Prompt for AI Agents
In
`@jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstConstructorBypassRule.java`
around lines 76 - 81, The lambda body returned by findAncestor(root, node,
RuleId.ClassMember.class) has inconsistent indentation; reformat the block
around the lambda (the anonymous parameter "member", the var memberText =
text(member, source); line, the inline comment, the return
memberText.contains("static ") && memberText.contains("Result<"); line, and the
closing "})") so all statements inside the lambda are consistently indented and
aligned with the opening "{", preserving the comment position and keeping the
expression returned from the lambda on its own properly indented; locate this in
CstConstructorBypassRule.java by the findAncestor(...) call, the local variable
memberText and the text(member, source) invocation.
| private CstNode getInterfaceDecl(CstNode typeDecl) { | ||
| return childByRule(typeDecl, RuleId.InterfaceDecl.class) | ||
| .or(typeDecl); | ||
| return childByRule(typeDecl, RuleId.InterfaceDecl.class).or(typeDecl); | ||
| } |
There was a problem hiding this comment.
Fallback returns wrong node type, masking potential errors.
The .or(typeDecl) fallback returns the parent TypeDecl instead of an InterfaceDecl. While the current call site (line 35) guarantees hasInterfaceDecl is true, this fallback could cause silent failures if the method is reused or filters change.
Consider throwing or using orElseThrow() to fail fast on unexpected state.
🛡️ Defensive alternative
private CstNode getInterfaceDecl(CstNode typeDecl) {
- return childByRule(typeDecl, RuleId.InterfaceDecl.class).or(typeDecl);
+ return childByRule(typeDecl, RuleId.InterfaceDecl.class)
+ .orElseThrow(() -> new IllegalStateException(
+ "Expected InterfaceDecl child in TypeDecl"));
}🤖 Prompt for AI Agents
In
`@jbct-core/src/main/java/org/pragmatica/jbct/lint/cst/rules/CstSealedErrorRule.java`
around lines 44 - 46, The helper getInterfaceDecl currently falls back to
returning the parent TypeDecl via .or(typeDecl), which can mask errors; change
it to fail fast by using orElseThrow (or otherwise throw a clear
IllegalStateException) so it only returns an InterfaceDecl when one exists:
update getInterfaceDecl to call childByRule(typeDecl,
RuleId.InterfaceDecl.class).orElseThrow(...) and include a message referencing
typeDecl (or the lack of InterfaceDecl) to aid debugging; ensure any callers
that assume hasInterfaceDecl remain valid or handle the exception.
| loadManifestFromDependency(dep.artifact(), | ||
| dep.version()).onPresent(depManifest -> { | ||
| // External dependencies use default config | ||
| var entry = new SliceEntry(depArtifact, | ||
| depManifest, | ||
| SliceConfig.defaults(), | ||
| true); | ||
| graph.put(depArtifact, entry); | ||
| try{ | ||
| resolveExternalDependencies(depManifest, graph); | ||
| } catch (MojoExecutionException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }) | ||
| .onEmpty(() -> { | ||
| graph.put(depArtifact, | ||
| new SliceEntry(depArtifact, | ||
| null, | ||
| SliceConfig.defaults(), | ||
| true)); | ||
| getLog().debug("No manifest found for dependency: " + depArtifact); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Checked exception wrapped in RuntimeException will bypass Maven's error handling.
The recursive call to resolveExternalDependencies inside the onPresent lambda catches MojoExecutionException and wraps it in RuntimeException. This causes Maven to display an ugly stack trace instead of a clean error message, and the exception type is lost.
Consider refactoring to collect dependencies first, then process them outside the lambda:
🔧 Suggested refactor to avoid exception wrapping
private void resolveExternalDependencies(SliceManifest manifest,
Map<String, SliceEntry> graph)
throws MojoExecutionException {
+ var pendingResolution = new ArrayList<SliceManifest>();
+
for (var dep : manifest.dependencies()) {
// Skip dependencies without artifact coordinates (local to this module)
if (dep.artifact() == null || dep.artifact().isEmpty()) {
continue;
}
var depArtifact = dep.artifact() + ":" + dep.version();
if (graph.containsKey(depArtifact)) {
continue;
}
- loadManifestFromDependency(dep.artifact(),
- dep.version()).onPresent(depManifest -> {
- // External dependencies use default config
- var entry = new SliceEntry(depArtifact,
- depManifest,
- SliceConfig.defaults(),
- true);
- graph.put(depArtifact, entry);
- try{
- resolveExternalDependencies(depManifest, graph);
- } catch (MojoExecutionException e) {
- throw new RuntimeException(e);
- }
- })
- .onEmpty(() -> {
- graph.put(depArtifact,
- new SliceEntry(depArtifact,
- null,
- SliceConfig.defaults(),
- true));
- getLog().debug("No manifest found for dependency: " + depArtifact);
- });
+ var maybeManifest = loadManifestFromDependency(dep.artifact(), dep.version());
+ if (maybeManifest.isPresent()) {
+ var depManifest = maybeManifest.unwrap();
+ var entry = new SliceEntry(depArtifact, depManifest, SliceConfig.defaults(), true);
+ graph.put(depArtifact, entry);
+ pendingResolution.add(depManifest);
+ } else {
+ graph.put(depArtifact, new SliceEntry(depArtifact, null, SliceConfig.defaults(), true));
+ getLog().debug("No manifest found for dependency: " + depArtifact);
+ }
+ }
+
+ // Recursively resolve after loop to avoid exception in lambda
+ for (var depManifest : pendingResolution) {
+ resolveExternalDependencies(depManifest, graph);
}
}🤖 Prompt for AI Agents
In
`@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/GenerateBlueprintMojo.java`
around lines 133 - 155, The onPresent lambda calls
resolveExternalDependencies(...) and catches MojoExecutionException only to wrap
it in RuntimeException, which loses the checked exception and breaks Maven's
error handling; change the flow so you don't throw RuntimeException from inside
the lambda—e.g., when loadManifestFromDependency(...).onPresent(...) runs, store
the depArtifact and depManifest (or the created SliceEntry) into a temporary
collection, handle onEmpty the same way, and after the lambda processing
finishes iterate that collection and call
resolveExternalDependencies(depManifest, graph) from the outer method where you
can declare/throw MojoExecutionException (or catch it and rethrow it directly)
so the original MojoExecutionException propagates correctly; reference:
loadManifestFromDependency, resolveExternalDependencies, SliceEntry, graph.
| getLog() | ||
| .info("Lint results: " + errors.get() + " error(s), " + warnings.get() + " warning(s), " + infos.get() | ||
| + " info(s)"); | ||
| .info("Lint results: " + errors.get() + " error(s), " + warnings.get() + " warning(s), " + infos.get() | ||
| + " info(s)"); |
There was a problem hiding this comment.
Chain alignment violates coding guidelines.
Per coding guidelines, . should align to the receiver end. The .info(...) call should align under getLog().
Suggested fix
- getLog()
- .info("Lint results: " + errors.get() + " error(s), " + warnings.get() + " warning(s), " + infos.get()
- + " info(s)");
+ getLog().info("Lint results: " + errors.get() + " error(s), " + warnings.get()
+ + " warning(s), " + infos.get() + " info(s)");🤖 Prompt for AI Agents
In `@jbct-maven-plugin/src/main/java/org/pragmatica/jbct/maven/LintMojo.java`
around lines 53 - 55, The chained method call alignment in LintMojo
(getLog().info(...)) violates style: move the dot so the `.info(...)` call is
vertically aligned under the end of `getLog()` (i.e., place the dot at the end
of the `getLog()` line and indent the `.info(...)` line so its leading `.` lines
up with the end of `getLog()`), ensuring the call remains `getLog()` followed on
the next line by `.info("Lint results: " + errors.get() + " error(s), " +
warnings.get() + " warning(s), " + infos.get() + " info(s)");` and preserves the
original string expression.
| <!-- Dependencies --> | ||
| <pragmatica-lite.version>0.9.11</pragmatica-lite.version> | ||
| <aether.version>0.7.5</aether.version> | ||
| <pragmatica-lite.version>0.10.0</pragmatica-lite.version> | ||
| <aether.version>0.8.0</aether.version> | ||
| <picocli.version>4.7.6</picocli.version> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the pom.xml file
fd -e pom.xml -type f | head -5Repository: siy/jbct-cli
Length of output: 226
🏁 Script executed:
# Get Maven Central info for pragmatica-lite versions
python3 - <<'PYTHON'
import json
import urllib.request
import sys
def get_maven_versions(group, artifact):
"""Query Maven Central for available versions"""
url = f"https://search.maven.org/solrsearch/select?q=g:%22{group}%22+AND+a:%22{artifact}%22&rows=100&wt=json"
try:
with urllib.request.urlopen(url, timeout=5) as response:
data = json.loads(response.read().decode('utf-8'))
docs = data.get("response", {}).get("docs", [])
if docs:
versions = sorted(set(doc.get("v") for doc in docs), reverse=True)
return versions[:10] # Top 10 versions
return []
except Exception as e:
print(f"Error: {e}", file=sys.stderr)
return []
# Check pragmatica-lite artifacts
for artifact in ["core", "http-client", "toml"]:
versions = get_maven_versions("org.pragmatica-lite", artifact)
print(f"pragmatica-lite:{artifact} - Available versions (latest 10):")
for v in versions:
print(f" {v}")
print()
PYTHONRepository: siy/jbct-cli
Length of output: 142
🏁 Script executed:
# Read the pom.xml file to see full dependency declarations
cat -n pom.xml | head -100Repository: siy/jbct-cli
Length of output: 4653
🏁 Script executed:
# Continue reading pom.xml to find infra-aspect and full dependency list
tail -n +100 pom.xml | head -100Repository: siy/jbct-cli
Length of output: 4027
🏁 Script executed:
# Search for infra-aspect in pom.xml
grep -n "infra-aspect" pom.xmlRepository: siy/jbct-cli
Length of output: 113
🏁 Script executed:
# Check if there's any Maven repository configuration or version constraints
grep -n "repository\|0.10.0" pom.xmlRepository: siy/jbct-cli
Length of output: 124
🏁 Script executed:
# Check git history or commit messages for context about this version change
git log --oneline pom.xml | head -10Repository: siy/jbct-cli
Length of output: 132
🏁 Script executed:
# Look for any CI configuration or error logs
find . -name "*.yml" -o -name "*.yaml" | grep -E "(github|gitlab|ci|workflow)" | head -10Repository: siy/jbct-cli
Length of output: 136
🏁 Script executed:
# Check if there are any comments or documentation about version choices
grep -B2 -A2 "0.10.0\|0.8.0" pom.xmlRepository: siy/jbct-cli
Length of output: 315
Pragmatica Lite versions conflict with coding guidelines.
Guidelines specify core/http-client/toml at 0.9.10; the pom.xml has them at 0.10.0. Confirm this upgrade is intentional and compatible, or revert to 0.9.10.
🤖 Prompt for AI Agents
In `@pom.xml` around lines 55 - 58, The pom sets pragmatica-lite.version to 0.10.0
which conflicts with the project's guideline of using 0.9.10; either confirm the
upgrade is intentional and document compatibility for modules that depend on
"pragmatica-lite" (core, http-client, toml) or revert the pom property
pragmatica-lite.version back to 0.9.10 so all modules use the guideline version;
update the <pragmatica-lite.version> property and verify dependent modules
(core, http-client, toml) build and tests pass (e.g., mvn test /
dependency:tree) to ensure no breaking changes.
| public ErrorConflict { | ||
| matchingPatterns = List.copyOf(matchingPatterns); | ||
| } |
There was a problem hiding this comment.
Guard against null inputs in the compact constructor.
List.copyOf throws an NPE if matchingPatterns is null, which is a behavior change vs. the previous record. If null can still surface here, consider an explicit guard (or default to List.of()).
✅ Suggested fix
public ErrorConflict {
- matchingPatterns = List.copyOf(matchingPatterns);
+ matchingPatterns = List.copyOf(java.util.Objects.requireNonNull(
+ matchingPatterns, "matchingPatterns"));
}📝 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 ErrorConflict { | |
| matchingPatterns = List.copyOf(matchingPatterns); | |
| } | |
| public ErrorConflict { | |
| matchingPatterns = List.copyOf(java.util.Objects.requireNonNull( | |
| matchingPatterns, "matchingPatterns")); | |
| } |
🤖 Prompt for AI Agents
In
`@slice-processor/src/main/java/org/pragmatica/jbct/slice/routing/ErrorConflict.java`
around lines 22 - 24, The compact constructor in ErrorConflict uses
List.copyOf(matchingPatterns) which will NPE if matchingPatterns is null; modify
the compact constructor (ErrorConflict) to guard against null by either: 1)
validating with Objects.requireNonNull(matchingPatterns, "...") to fail fast
with a clear message, or 2) defaulting null to an empty list (e.g.,
matchingPatterns == null ? List.of() : List.copyOf(matchingPatterns)) so
matchingPatterns is never null after construction.
- Bump pragmatica-lite to 0.11.1 (security fixes) - Set aether to 0.7.5 (latest published, 0.8.1 not yet available) - Skip slice-processor-tests module in CI (requires unreleased Aether) - TODO: Restore slice-processor-tests when Aether 0.8.1+ published
- Add tinylog 2.7.0 to parent pom dependencyManagement - Add tinylog-impl and slf4j-tinylog to jbct-core test scope - Add tinylog-impl and slf4j-tinylog to slice-processor test scope - Update jbct-cli to use version from parent (runtime scope) - Add tinylog-test.properties for test logging configuration - Eliminates 'No SLF4J providers found' warnings during tests
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@jbct-core/pom.xml`:
- Around line 47-56: The POM declares tinylog test-only providers but forgot to
mark them as test scope; update the two dependency entries with artifactId
tinylog-impl and slf4j-tinylog to include <scope>test</scope> so these SLF4J
bindings are only used during tests and not brought into the production
artifact.
In `@slice-processor/pom.xml`:
- Around line 82-91: The tinylog dependencies (artifactId tinylog-impl and
slf4j-tinylog) were intended as test-only SLF4J providers but lack a test scope,
causing them to be included in the compile/runtime classpath; update the two
dependency entries that reference tinylog-impl and slf4j-tinylog to add a
<scope>test</scope> element so Maven treats them as test-scoped dependencies and
they are not packaged or exposed on the compile classpath.
|
|
||
| <!-- Logging (SLF4J provider for tests) --> | ||
| <dependency> | ||
| <groupId>org.tinylog</groupId> | ||
| <artifactId>tinylog-impl</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.tinylog</groupId> | ||
| <artifactId>slf4j-tinylog</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Missing <scope>test</scope> on logging dependencies.
The comment indicates these are "SLF4J provider for tests," but neither dependency specifies test scope. Without explicit scope, Maven defaults to compile, which will:
- Include tinylog in the production artifact unnecessarily
- Risk SLF4J binding conflicts with downstream consumers
Proposed fix
<!-- Logging (SLF4J provider for tests) -->
<dependency>
<groupId>org.tinylog</groupId>
<artifactId>tinylog-impl</artifactId>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.tinylog</groupId>
<artifactId>slf4j-tinylog</artifactId>
+ <scope>test</scope>
</dependency>📝 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.
| <!-- Logging (SLF4J provider for tests) --> | |
| <dependency> | |
| <groupId>org.tinylog</groupId> | |
| <artifactId>tinylog-impl</artifactId> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.tinylog</groupId> | |
| <artifactId>slf4j-tinylog</artifactId> | |
| </dependency> | |
| <!-- Logging (SLF4J provider for tests) --> | |
| <dependency> | |
| <groupId>org.tinylog</groupId> | |
| <artifactId>tinylog-impl</artifactId> | |
| <scope>test</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.tinylog</groupId> | |
| <artifactId>slf4j-tinylog</artifactId> | |
| <scope>test</scope> | |
| </dependency> |
🤖 Prompt for AI Agents
In `@jbct-core/pom.xml` around lines 47 - 56, The POM declares tinylog test-only
providers but forgot to mark them as test scope; update the two dependency
entries with artifactId tinylog-impl and slf4j-tinylog to include
<scope>test</scope> so these SLF4J bindings are only used during tests and not
brought into the production artifact.
|
|
||
| <!-- Logging (SLF4J provider for tests) --> | ||
| <dependency> | ||
| <groupId>org.tinylog</groupId> | ||
| <artifactId>tinylog-impl</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.tinylog</groupId> | ||
| <artifactId>slf4j-tinylog</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Missing <scope>test</scope> on tinylog dependencies.
The comment states these are "SLF4J provider for tests", and the commit message confirms tinylog was added as a test dependency. However, no <scope>test</scope> is specified, so Maven will default to compile scope—packaging these JARs into the final artifact and exposing them on the compile classpath.
🔧 Proposed fix
<!-- Logging (SLF4J provider for tests) -->
<dependency>
<groupId>org.tinylog</groupId>
<artifactId>tinylog-impl</artifactId>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.tinylog</groupId>
<artifactId>slf4j-tinylog</artifactId>
+ <scope>test</scope>
</dependency>🤖 Prompt for AI Agents
In `@slice-processor/pom.xml` around lines 82 - 91, The tinylog dependencies
(artifactId tinylog-impl and slf4j-tinylog) were intended as test-only SLF4J
providers but lack a test scope, causing them to be included in the
compile/runtime classpath; update the two dependency entries that reference
tinylog-impl and slf4j-tinylog to add a <scope>test</scope> element so Maven
treats them as test-scoped dependencies and they are not packaged or exposed on
the compile classpath.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 participant
Summary
Major refactor of slice architecture plus JBCT compliance improvements and bug fixes.
Breaking Changes
Simplified Slice Architecture:
.apipackage)SliceInvokerFacadeproxy@Sliceinterface directlyMajor Features
Slice Architecture Simplification
ApiInterfaceGeneratorand API package generationSliceProcessor,SliceModel,ManifestGeneratorAnnotation Processing
@Aspectand@Keyannotation processing for cache-wrapped methods^[a-z][a-zA-Z0-9]+$)Security & Validation
jbct:verify-slicefails if runtime deps notprovidedscopeJBCT Compliance
Bug Fixes
MethodHandleAPI usageResult.async()usage for pragmatica-lite 0.10.0Documentation
Migration Guide
For slice projects:
@Sliceinterface, not.api.packageprovidedscopeFor slice consumers:
Test Results
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.