Conversation
- Add 53 new E2E tests across 7 new test classes (80 total) - ManagementApiE2ETest: status, metrics, thresholds, alerts, controller - SliceInvocationE2ETest: route handling, error cases, distribution - MetricsE2ETest: collection, Prometheus format, distribution - ControllerE2ETest: configuration, status, leader behavior - BootstrapE2ETest: node restart, state recovery, rolling restart - GracefulShutdownE2ETest: peer detection, leader failover - NetworkPartitionE2ETest: quorum behavior, partition healing - Enhance AetherNodeContainer with 20+ new API methods - Add documentation map (docs/README.md) - Update development priorities with test coverage summary - Update E2E testing guide with new test suite overview 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughProject version bumped to v0.7.1; two new modules added ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AetherUp as AetherUp (CLI)
participant Loader as ConfigLoader
participant Builder as AetherConfig.Builder
participant Selector as GeneratorSelector
participant Generator as Generator (Local/Docker/K8s)
participant FS as Filesystem
User->>AetherUp: run CLI (args, --config, overrides)
AetherUp->>Loader: load(Path) or fromEnvironment
Loader->>Builder: assemble AetherConfig (defaults + overrides)
Builder-->>AetherUp: AetherConfig
AetherUp->>Selector: choose generator for config.environment
Selector-->>Generator: supports(config)?
Generator-->>AetherUp: yes
AetherUp->>Generator: generate(config, outputDir)
Generator->>FS: write manifests/scripts/.env/docker-compose (IO)
FS-->>Generator: files written / IO error
Generator-->>AetherUp: GeneratorOutput or GeneratorError
AetherUp->>User: print instructions / exit code
Note right of Generator: generation includes templates, permissions,\nand environment-specific artifacts (success/error paths)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
10-51: Complete the E2E test class listing in CHANGELOG.md.The changelog accurately claims "80 E2E tests across 12 test classes," but the itemized list includes only 7 test classes (totaling 53 tests). The changelog is missing 5 test classes with their test counts: ChaosE2ETest (5 tests), ClusterFormationE2ETest (4 tests), NodeFailureE2ETest (6 tests), RollingUpdateE2ETest (6 tests), and SliceDeploymentE2ETest (6 tests). Add these to the changelog bullet list for completeness.
🧹 Nitpick comments (6)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/GracefulShutdownE2ETest.java (1)
107-114: Consider tightening the success condition.The condition
slices.contains("example-slice") || !slices.contains("\"error\"")is overly permissive. It succeeds if either the slice is deployed OR there's no error, but doesn't require both. This could pass even if the deployment failed gracefully without the slice ever appearing.🔎 Suggested improvement
Consider whether the test should verify that the slice eventually deploys successfully, or if it's acceptable for deployment to fail during shutdown:
- try { - var slices = cluster.anyNode().getSlices(); - return slices.contains("example-slice") || !slices.contains("\"error\""); - } catch (Exception e) { - return false; - } + try { + var slices = cluster.anyNode().getSlices(); + // Deployment should eventually complete or gracefully fail + return slices.contains("example-slice") && slices.contains("ACTIVE"); + } catch (Exception e) { + return false; + }Or, if the intent is to verify graceful handling regardless of outcome:
try { var slices = cluster.anyNode().getSlices(); - return slices.contains("example-slice") || !slices.contains("\"error\""); + // Either succeeds or fails gracefully (no error state) + return !slices.contains("\"error\""); } catch (Exception e) { return false; }e2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.java (1)
71-76: Redundant conditional check inside loopThe loop checks
if (node.isRunning())but at this point (line 65) the code has already waited for all 3 nodes to be running. This conditional is redundant and suggests defensive coding that may mask issues.🔎 Suggested simplification
- // Verify all nodes are healthy - for (var node : cluster.nodes()) { - if (node.isRunning()) { - var health = node.getHealth(); - assertThat(health).doesNotContain("\"error\""); - } - } + // Verify all nodes are healthy + for (var node : cluster.nodes()) { + var health = node.getHealth(); + assertThat(health).doesNotContain("\"error\""); + }e2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.java (1)
60-71: Test name doesn't match polling intervalThe test method is named
metricsCollected_everySecond()but usesMETRICS_INTERVAL(2 seconds) as the poll interval. This creates a mismatch between the test name and its actual behavior.Consider renaming to
metricsCollected_reliably()or updating the comment to clarify that "everySecond" refers to the system behavior, not the test polling interval.e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java (1)
54-59: Assertion may be too permissiveThe assertion
containsAnyOf("error", "404", "not found", "Not Found")is quite broad and could match various unintended error conditions. Consider whether a more specific assertion would better validate the expected 404 behavior.If the API contract specifies a specific error format or status code, consider asserting on that specific format rather than multiple variations.
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.java (1)
116-126: Redundant conditional checkLine 122 checks
if (!prometheus.contains("error"))but line 120 already asserts that the response doesn't contain "error". This conditional is unreachable or suggests uncertainty about the assertion.🔎 Suggested simplification
void prometheusMetrics_validFormat_scrapable() { var prometheus = cluster.anyNode().getPrometheusMetrics(); - // Prometheus format uses # for comments and metric_name{labels} value assertThat(prometheus).doesNotContain("\"error\""); - // Should have at least some metrics exposed - if (!prometheus.contains("error")) { - // Valid Prometheus output has either metrics or is empty - assertThat(prometheus).isNotNull(); - } + // Prometheus format uses # for comments and metric_name{labels} value + // Valid Prometheus output has either metrics or is empty + assertThat(prometheus).isNotNull(); }e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherNodeContainer.java (1)
597-604: Consider supporting PUT and PATCH methods.The
invokeSlice()method currently supports GET, POST, and DELETE. If E2E tests need to invoke slice endpoints using PUT or PATCH (common for update operations), consider adding support for these methods.🔎 Proposed enhancement
public String invokeSlice(String httpMethod, String path, String body) { return switch (httpMethod.toUpperCase()) { case "GET" -> get(path); case "POST" -> post(path, body); + case "PUT" -> put(path, body); + case "PATCH" -> patch(path, body); case "DELETE" -> delete(path); default -> "{\"error\":\"Unsupported HTTP method: " + httpMethod + "\"}"; }; }You would need to add corresponding
put()andpatch()HTTP helper methods following the existing pattern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
CHANGELOG.mdCLAUDE.mdcli/pom.xmlcluster/pom.xmldocs/README.mddocs/archive/aether-high-level-overview.mddocs/development-priorities.mddocs/guide/cli-reference.mddocs/guide/e2e-testing.mddocs/guide/getting-started.mddocs/guide/migration-guide.mddocs/jbct-cli-slice-support-task.mddocs/typed-slice-api-design.mde2e-tests/pom.xmle2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/GracefulShutdownE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/NetworkPartitionE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherNodeContainer.javaexample-slice/README.mdexample-slice/pom.xmlexamples/order-demo/pom.xmlforge/pom.xmlinfra-services/artifact-repo/pom.xmlinfra-services/pom.xmlnode/pom.xmlpom.xmlslice-annotations/pom.xmlslice-api/pom.xmlslice/pom.xml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.java: Return exactly one of: T (synchronous infallible), Option (synchronous optional), Result (synchronous fallible), or Promise (asynchronous fallible). Never use Promise<Result>
Use factory naming convention: TypeName.typeName(...) with lowercase-first method names for creating instances
Never throw business exceptions. All business logic failures must flow through Result or Promise as typed Cause objects
Use sealed interfaces for domain-specific error hierarchies implementing Cause interface
Prefer record-based implementations for use cases, adapters, and data carriers
Use method references over lambdas when the lambda only calls a single method
Ensure all domain objects are immutable records. Use List.of(), Map.of(), Set.of() for collections
Apply timeouts as close to actual I/O operations as possible, not at the end of promise chains
Use Promise.lift() and Result.lift() for wrapping exception-prone operations in error handling
Use adapter leaves for all I/O operations (database, HTTP, file system) to ensure framework independence
Follow single pattern per function rule: Leaf (single operation), Sequencer (linear chain), Fork-Join (parallel), Condition (branching), Iteration (collection processing)
Use .map() and .flatMap() for Result and Promise transformations instead of extracting and re-wrapping values
Package structure must be organized by feature/use case, not by layer (e.g., /userregistration, not /services or /dao)
All Promise-based operations must include timeout configuration relative to their specific operation's expected duration
Keep lambdas simple. Extract complex logic to separate methods to maintain code clarity
Files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/GracefulShutdownE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/NetworkPartitionE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherNodeContainer.java
**/*Test.java
📄 CodeRabbit inference engine (CLAUDE.md)
**/*Test.java: Group test classes by feature/use case, not by layer. Use descriptive test method names with underscores: method_scenario_expectation()
Use .await() to block on Promise in tests, then test the Result like synchronous code
For Promise success cases in tests, use .onFailure(Assertions::fail).onSuccess(...) pattern
For Promise failure cases in tests, use .onSuccessRun(Assertions::fail).onFailure(...) pattern
Use Result.all() for complex object construction and validation in tests with multiple components
Include roundtrip tests for all parseable types, validating parse(string).map(asString()) == string
Test both success and failure paths for Result operations: valid input cases and failure cases
Import org.junit.jupiter.api.Assertions and org.assertj.core.api.Assertions in test files
Make test classes package-private with no visibility modifier
Use type declarations for stub dependencies in tests instead of inline mock creation
Files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/GracefulShutdownE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/NetworkPartitionE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.java
🧠 Learnings (25)
📓 Common learnings
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNode.java : AetherNode assembly must wire all components (SliceStore, DeploymentManagers, MetricsCollector, ControlLoop, ManagementServer) together
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/resolver/*Resolver.java : Use DependencyResolver with cycle detection to validate slice dependencies before loading
Applied to files:
slice/pom.xmlexample-slice/README.mddocs/guide/getting-started.mddocs/guide/migration-guide.mde2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNode.java : AetherNode assembly must wire all components (SliceStore, DeploymentManagers, MetricsCollector, ControlLoop, ManagementServer) together
Applied to files:
example-slice/README.mddocs/archive/aether-high-level-overview.mde2e-tests/src/test/java/org/pragmatica/aether/e2e/GracefulShutdownE2ETest.javanode/pom.xmle2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherNodeContainer.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/cli/*.java : AetherCli must support both interactive REPL mode and batch command mode for cluster management
Applied to files:
cli/pom.xmldocs/guide/cli-reference.md
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNodeConfig.java : AetherNodeConfig must provide all runtime configuration for node (timeouts, cluster addresses, metrics intervals, etc.)
Applied to files:
docs/archive/aether-high-level-overview.mdnode/pom.xmlpom.xmle2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherNodeContainer.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/invoke/*Invoker.java : SliceInvoker must implement inter-slice invocation with automatic retry logic and atomic guarantees
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/*EndpointRegistry.java : Endpoint registry must map SliceRoute paths to physical endpoint locations (nodeId + port)
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/invoke/*Handler.java : InvocationHandler must implement server-side method dispatch and route calls to SliceMethod implementations
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/*Slice.java : Service Slices must have multiple SliceMethod entries for CRUD operations or API gateway patterns
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/*Slice.java : Slice interface must implement three methods: Promise<Unit> start(), Promise<Unit> stop(), List<SliceMethod<?, ?>> methods()
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/*Slice.java : Lean Slices must have single SliceMethod entry implementing one DDD use case or event handler using JBCT patterns
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/*Route.java : SliceRoute must specify HTTP method (GET, POST, etc.) and path pattern for entry points
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/*DeploymentManager.java : Store slice lifecycle state in consensus KV-Store using structured keys following AetherKey pattern
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/node/*DeploymentManager.java : NodeDeploymentManager must watch KV-Store for slice state changes and coordinate with SliceStore for lifecycle operations
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/blueprint/*.java : Blueprint DSL must specify desired cluster configuration with slice artifacts and instance counts in JSON format
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/cluster/*DeploymentManager.java : ClusterDeploymentManager must watch KV-Store for blueprint changes, make allocation decisions, and write directly to slice-node-keys with LOAD state
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.java
📚 Learning: 2025-12-28T23:49:16.532Z
Learnt from: siy
Repo: siy/aether PR: 19
File: node/src/main/java/org/pragmatica/aether/node/AetherNode.java:343-355
Timestamp: 2025-12-28T23:49:16.532Z
Learning: In AetherNode.java, the compositeRepository method intentionally uses only the first repository (not multi-repository fallback like SliceStoreImpl) because BlueprintService parses text blueprints where fallback to another repository wouldn't help (same artifact would have the same result), unlike SliceStoreImpl which loads JARs where trying multiple sources makes sense. This simplification is documented in the Javadoc and will be extended when there's a real use case.
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/controller/*Controller.java : DecisionTreeController must run only on leader node and be evaluated every 1 second with deterministic rule engine
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/controller/*ControlLoop.java : ControlLoop must evaluate DecisionTreeController decisions only on leader node and apply resulting blueprint changes
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/cluster/*.java : Leader node is deterministically selected as first node in cluster topology for special operations
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/Artifact.java : Artifact coordinates must follow Maven format: groupId:artifactId:version[-qualifier]
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/api/*Server.java : ManagementServer must provide HTTP API for cluster management operations (deploy, scale, list, etc.)
Applied to files:
docs/guide/e2e-testing.mde2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/metrics/*.java : Collect core metrics every 1 second: Node CPU Usage, Calls Per Entry Point, Total Call Duration
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/metrics/*Scheduler.java : MetricsScheduler must implement leader-driven ping-pong metric distribution every 1 second with automatic failover
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/metrics/*.java : Metrics must not flow through KV-Store. Use MessageRouter for direct metric updates and ClusterMetricsSnapshot broadcast
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.java
🧬 Code graph analysis (5)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.java (1)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherCluster.java (1)
AetherCluster(39-373)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/GracefulShutdownE2ETest.java (2)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherCluster.java (1)
AetherCluster(39-373)docs/api/examples/python-client.py (3)
health(54-56)deploy(67-69)slices(63-65)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java (2)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.java (3)
Nested(50-86)Nested(88-110)Nested(112-142)docs/api/examples/python-client.py (4)
deploy(67-69)slices(63-65)undeploy(75-77)health(54-56)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.java (1)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherCluster.java (1)
AetherCluster(39-373)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.java (5)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.java (3)
Nested(50-86)Nested(88-110)Nested(112-142)e2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.java (3)
Nested(47-87)Nested(89-109)Nested(111-140)e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java (3)
Nested(50-91)Nested(93-132)Nested(134-167)e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherCluster.java (1)
AetherCluster(39-373)docs/api/examples/python-client.py (5)
health(54-56)slices(63-65)deploy(67-69)metrics(80-82)thresholds(119-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker-build
🔇 Additional comments (40)
docs/guide/getting-started.md (1)
101-101: LGTM! Documentation version correctly updated.The Maven dependency version has been properly updated to 0.7.1, ensuring users following this guide will use the correct release version.
cluster/pom.xml (1)
9-9: LGTM! Parent POM version correctly aligned.The parent version has been properly updated to 0.7.1, maintaining consistency across the module hierarchy.
docs/guide/migration-guide.md (1)
156-156: LGTM! Migration guide version correctly updated.The slice-api dependency version has been properly updated to 0.7.1 in the migration guide examples.
cli/pom.xml (1)
10-10: LGTM! CLI module version correctly aligned.The parent version has been properly updated to 0.7.1, maintaining consistency with the project-wide release.
infra-services/pom.xml (1)
9-9: LGTM! Infra-services module version correctly aligned.The parent version has been properly updated to 0.7.1, completing the consistent version alignment across all reviewed modules.
node/pom.xml (1)
9-9: LGTM! Version bump aligns with release 0.7.1.The parent POM version update is consistent with the release objectives.
slice-api/pom.xml (1)
9-9: LGTM! Version bump aligns with release 0.7.1.The parent POM version update is consistent with the release objectives.
forge/pom.xml (1)
10-10: LGTM! Version bump aligns with release 0.7.1.The parent POM version update is consistent with the release objectives.
docs/guide/cli-reference.md (1)
276-276: LGTM! Documentation updated to reflect release 0.7.1.The version string in the REPL example correctly reflects the new release version.
example-slice/pom.xml (1)
9-9: LGTM! Version bump aligns with release 0.7.1.The parent POM version update is consistent with the release objectives.
e2e-tests/pom.xml (1)
9-9: LGTM! Version bump aligns with release 0.7.1.The parent POM version update is consistent with the project-wide version bump for the production readiness release.
CLAUDE.md (1)
5-5: LGTM! Documentation version references updated correctly.The version updates in the project overview and artifact example are consistent with the 0.7.1 release.
Also applies to: 98-98
pom.xml (1)
9-9: LGTM! Root POM version updated for release 0.7.1.The version bump in the root POM is correct and serves as the authoritative version for all child modules in this multi-module project.
examples/order-demo/pom.xml (1)
10-10: LGTM! Parent and dependency versions updated consistently.Both the parent POM version and the aether.version property are correctly updated to 0.7.1, ensuring the order demo aligns with the new release.
Also applies to: 30-30
docs/archive/aether-high-level-overview.md (1)
5-5: LGTM! Archived documentation version reference updated.The version update in the archived high-level overview is consistent with the 0.7.1 release, maintaining documentation accuracy.
slice/pom.xml (1)
9-9: LGTM! Version bump aligns with release 0.7.1.The parent version update is consistent with the PR objectives for production readiness.
slice-annotations/pom.xml (1)
10-10: LGTM! Version bump aligns with release 0.7.1.The parent version update is consistent with the production readiness release.
docs/typed-slice-api-design.md (1)
908-908: LGTM! Documentation version updated correctly.The processor version in the manifest documentation aligns with the 0.7.1 release.
infra-services/artifact-repo/pom.xml (1)
9-9: LGTM! Version bump aligns with release 0.7.1.The parent version update for the artifact repository module is consistent with the production readiness release.
docs/jbct-cli-slice-support-task.md (1)
1267-1267: Version bump is correct and consistent.The processor.version update to 0.7.1 aligns with the release version and is appropriately documented in the metadata section.
CHANGELOG.md (2)
52-64: Breaking API changes are clearly documented.The AlertManager changes (lines 53-56) introducing Promise-based consensus operations and KV-Store persistence are significant and appropriately marked under Changed. ThresholdStrategy additions (lines 57-59) are backward compatible and well-documented.
65-195: Older changelog entries are well-maintained.Versions 0.6.5 and earlier follow consistent formatting and provide good historical context. The semantic versioning progression is clear.
docs/README.md (1)
1-76: Documentation hub structure is well-organized with all links valid.The docs/README.md provides excellent role-based navigation for Developers, Operators, and Architects with comprehensive Feature Guides and References sections. All 23 referenced documentation files exist and are properly linked.
docs/development-priorities.md (1)
117-133: LGTM! Excellent test coverage documentation.The test coverage summary provides clear metrics showing comprehensive E2E test coverage across 12 test classes and 80 tests. This aligns perfectly with the production readiness focus of v0.7.1.
e2e-tests/src/test/java/org/pragmatica/aether/e2e/GracefulShutdownE2ETest.java (1)
1-141: Well-structured E2E tests for graceful shutdown scenarios.The test suite comprehensively covers graceful shutdown behavior including peer detection, cluster functionality, deployment handling, and leader re-election. The tests follow proper E2E testing patterns with appropriate use of Awaitility, proper timeouts, and clear assertions.
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ControllerE2ETest.java (1)
1-143: LGTM! Excellent controller E2E test coverage.This test suite provides comprehensive coverage of controller behavior with clean organization using
@Nestedclasses. The tests properly verify:
- Configuration management (get, set, persistence)
- Status reporting and evaluation triggering
- Leader-only execution (line 131: deterministic leader node-1)
- Resilience to leader failover
The implementation aligns perfectly with the learnings about the DecisionTreeController running only on the leader node.
Based on learnings, DecisionTreeController must run only on leader node and be evaluated deterministically.
docs/guide/e2e-testing.md (1)
192-210: No action required. The "Test Suite Overview" section appears only once in the document at line 192. The search found a single occurrence with no duplicates present.e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceDeploymentE2ETest.java (1)
28-28: Artifact version is correct; ensuremvn packageis run before E2E tests.The test artifact coordinate
org.pragmatica-lite.aether:example-slice:0.7.1is correct and matches the project version. Theexample-slicemodule builds beforee2e-testsin the module order, ensuring the JAR is available in the local Maven repository. The Aether node (running in the container) resolves the artifact at deployment time from the Maven repository. The test setup already validates thataether-node.jarhas been built, so the full build must complete before integration tests run.e2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.java (2)
28-28: LGTM: Proper visibility modifierThe test class correctly uses package-private visibility as per coding guidelines.
34-46: LGTM: Proper test lifecycle managementSetup and teardown methods correctly initialize the cluster and ensure cleanup with a null check.
e2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.java (1)
114-139: LGTM: Proper async verification of metrics distributionThe test correctly waits for metrics to be distributed to all nodes with appropriate polling delay and interval, then verifies final state.
e2e-tests/src/test/java/org/pragmatica/aether/e2e/SliceInvocationE2ETest.java (2)
30-30: LGTM: Proper visibility modifierThe test class correctly uses package-private visibility.
113-131: LGTM: Thorough deploy/undeploy cycle testThe test properly verifies slice deployment, waits for presence, undeploys, waits for removal, and validates that routes are no longer available.
e2e-tests/src/test/java/org/pragmatica/aether/e2e/NetworkPartitionE2ETest.java (3)
25-28: Good documentation of test limitationsThe comment clearly documents that these tests simulate partitions by stopping nodes rather than true network isolation. This is an appropriate trade-off for testcontainer-based E2E tests.
48-69: LGTM: Proper majority partition testThe test correctly simulates a minority partition, verifies the majority maintains quorum, and validates the cluster continues operating with appropriate node count.
119-146: LGTM: Comprehensive consistency verification across quorum transitionsThe test deploys a slice, reduces to 2 nodes (maintaining quorum), verifies state persistence, restores full cluster, and confirms consistency throughout. This is a strong test for production readiness.
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.java (3)
27-27: LGTM: Proper visibility modifierThe test class correctly uses package-private visibility.
46-101: LGTM: Comprehensive status endpoint coverageThe StatusEndpoints tests properly cover health, status, nodes listing, and slices listing with appropriate waits for async operations and clear assertions.
167-228: LGTM: Thorough threshold and alert endpoint testingThe test group covers set/get/delete operations for thresholds and all alert operations (active, history, clear) with proper async verification.
e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherNodeContainer.java (1)
323-341: LGTM! DELETE helper follows established patterns.The
delete()method correctly mirrors the existingget()andpost()helper structure with appropriate timeout and error handling.
| public String getInvocationMetrics(String artifact, String method) { | ||
| var params = new StringBuilder(); | ||
| if (artifact != null) { | ||
| params.append("artifact=").append(artifact); | ||
| } | ||
| if (method != null) { | ||
| if (!params.isEmpty()) params.append("&"); | ||
| params.append("method=").append(method); | ||
| } | ||
| var path = params.isEmpty() ? "/invocation-metrics" : "/invocation-metrics?" + params; | ||
| return get(path); | ||
| } |
There was a problem hiding this comment.
Add URL encoding for query parameters.
The query string builder concatenates artifact and method parameters without URL encoding. This will cause request failures if parameters contain special characters like &, =, spaces, or Unicode.
🔎 Proposed fix using URLEncoder
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+
public String getInvocationMetrics(String artifact, String method) {
var params = new StringBuilder();
if (artifact != null) {
- params.append("artifact=").append(artifact);
+ params.append("artifact=").append(URLEncoder.encode(artifact, StandardCharsets.UTF_8));
}
if (method != null) {
if (!params.isEmpty()) params.append("&");
- params.append("method=").append(method);
+ params.append("method=").append(URLEncoder.encode(method, StandardCharsets.UTF_8));
}
var path = params.isEmpty() ? "/invocation-metrics" : "/invocation-metrics?" + params;
return get(path);
}🤖 Prompt for AI Agents
In
e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherNodeContainer.java
around lines 370-381, the query parameters are appended raw which breaks when
values contain spaces or special characters; change the builder to URL-encode
artifact and method values before appending (use java.net.URLEncoder with UTF-8
or URLEncoder.encode(value, StandardCharsets.UTF_8.name()) / the Charset
overload), keep the same null checks and ampersand logic, and handle the checked
encoding exception either by propagating as a runtime exception or surrounding
with a try/catch that wraps it in an unchecked exception so the method returns
the same path string but with properly encoded parameter values.
| public String setThreshold(String metric, double warning, double critical) { | ||
| var body = "{\"metric\":\"" + metric + "\",\"warning\":" + warning + ",\"critical\":" + critical + "}"; | ||
| return post("/thresholds", body); | ||
| } | ||
|
|
||
| /** | ||
| * Deletes a threshold for a metric. | ||
| * | ||
| * @param metric metric name | ||
| * @return response JSON | ||
| */ | ||
| public String deleteThreshold(String metric) { | ||
| return delete("/thresholds/" + metric); | ||
| } |
There was a problem hiding this comment.
Add proper JSON escaping and URL encoding.
Two issues in the threshold methods:
-
Line 421: Manual JSON construction without escaping the
metricparameter. Ifmetriccontains quotes or backslashes (e.g.,cpu"memory), this produces invalid JSON. -
Line 432: Path concatenation without URL encoding. If
metriccontains slashes or special characters (e.g.,cpu/usage), this produces an incorrect path.
🔎 Proposed fix with proper escaping
For JSON construction, either use a JSON library or escape manually:
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+
public String setThreshold(String metric, double warning, double critical) {
- var body = "{\"metric\":\"" + metric + "\",\"warning\":" + warning + ",\"critical\":" + critical + "}";
+ var escapedMetric = metric.replace("\\", "\\\\").replace("\"", "\\\"");
+ var body = "{\"metric\":\"" + escapedMetric + "\",\"warning\":" + warning + ",\"critical\":" + critical + "}";
return post("/thresholds", body);
}
public String deleteThreshold(String metric) {
- return delete("/thresholds/" + metric);
+ return delete("/thresholds/" + URLEncoder.encode(metric, StandardCharsets.UTF_8));
}Alternatively, consider using a JSON library (e.g., Jackson or Gson) for robust serialization.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherNodeContainer.java
around lines 420 to 433, the setThreshold method builds JSON by concatenating
the metric string (risking invalid JSON if metric contains quotes/backslashes)
and deleteThreshold concatenates metric into the path (risking invalid URLs if
metric contains slashes/special chars); fix by creating a small POJO or Map and
serializing it with your JSON library (e.g., Jackson/Gson) to produce a safe
body for post, and encode the metric in deleteThreshold using
URLEncoder.encode(metric, StandardCharsets.UTF_8.name()) (or URI/URL builder)
when constructing the DELETE path.
| public String startRollingUpdate(String oldVersion, String newVersion) { | ||
| var body = "{\"oldVersion\":\"" + oldVersion + "\",\"newVersion\":\"" + newVersion + "\"}"; | ||
| return post("/rolling-update/start", body); | ||
| } | ||
|
|
||
| /** | ||
| * Gets all active rolling updates. | ||
| * | ||
| * @return active updates JSON | ||
| */ | ||
| public String getRollingUpdates() { | ||
| return get("/rolling-updates"); | ||
| } | ||
|
|
||
| /** | ||
| * Gets status of a specific rolling update. | ||
| * | ||
| * @param updateId update identifier | ||
| * @return update status JSON | ||
| */ | ||
| public String getRollingUpdateStatus(String updateId) { | ||
| return get("/rolling-update/" + updateId); | ||
| } | ||
|
|
||
| /** | ||
| * Adjusts traffic routing during rolling update. | ||
| * | ||
| * @param updateId update identifier | ||
| * @param oldWeight weight for old version | ||
| * @param newWeight weight for new version | ||
| * @return response JSON | ||
| */ | ||
| public String setRollingUpdateRouting(String updateId, int oldWeight, int newWeight) { | ||
| var body = "{\"oldWeight\":" + oldWeight + ",\"newWeight\":" + newWeight + "}"; | ||
| return post("/rolling-update/" + updateId + "/routing", body); | ||
| } | ||
|
|
||
| /** | ||
| * Completes a rolling update. | ||
| * | ||
| * @param updateId update identifier | ||
| * @return response JSON | ||
| */ | ||
| public String completeRollingUpdate(String updateId) { | ||
| return post("/rolling-update/" + updateId + "/complete", "{}"); | ||
| } | ||
|
|
||
| /** | ||
| * Rolls back a rolling update. | ||
| * | ||
| * @param updateId update identifier | ||
| * @return response JSON | ||
| */ | ||
| public String rollbackRollingUpdate(String updateId) { | ||
| return post("/rolling-update/" + updateId + "/rollback", "{}"); | ||
| } |
There was a problem hiding this comment.
Add JSON escaping and URL encoding for rolling update methods.
Multiple issues in the rolling update API:
-
Line 520: Manual JSON construction without escaping
oldVersionandnewVersion. Version strings containing quotes will produce invalid JSON. -
Lines 540, 553, 562, 572: Path concatenation without URL encoding
updateId. If the ID contains slashes or special characters, requests will target incorrect paths.
🔎 Proposed fix with proper escaping
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+
public String startRollingUpdate(String oldVersion, String newVersion) {
- var body = "{\"oldVersion\":\"" + oldVersion + "\",\"newVersion\":\"" + newVersion + "\"}";
+ var escapedOld = oldVersion.replace("\\", "\\\\").replace("\"", "\\\"");
+ var escapedNew = newVersion.replace("\\", "\\\\").replace("\"", "\\\"");
+ var body = "{\"oldVersion\":\"" + escapedOld + "\",\"newVersion\":\"" + escapedNew + "\"}";
return post("/rolling-update/start", body);
}
public String getRollingUpdateStatus(String updateId) {
- return get("/rolling-update/" + updateId);
+ return get("/rolling-update/" + URLEncoder.encode(updateId, StandardCharsets.UTF_8));
}
public String setRollingUpdateRouting(String updateId, int oldWeight, int newWeight) {
var body = "{\"oldWeight\":" + oldWeight + ",\"newWeight\":" + newWeight + "}";
- return post("/rolling-update/" + updateId + "/routing", body);
+ return post("/rolling-update/" + URLEncoder.encode(updateId, StandardCharsets.UTF_8) + "/routing", body);
}
public String completeRollingUpdate(String updateId) {
- return post("/rolling-update/" + updateId + "/complete", "{}");
+ return post("/rolling-update/" + URLEncoder.encode(updateId, StandardCharsets.UTF_8) + "/complete", "{}");
}
public String rollbackRollingUpdate(String updateId) {
- return post("/rolling-update/" + updateId + "/rollback", "{}");
+ return post("/rolling-update/" + URLEncoder.encode(updateId, StandardCharsets.UTF_8) + "/rollback", "{}");
}🤖 Prompt for AI Agents
In
e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherNodeContainer.java
around lines 519-574, the rolling update methods build JSON and paths by string
concatenation which breaks for values containing quotes, slashes or other
specials; replace manual JSON construction with a proper JSON serializer (e.g.
use a JSONObject or Jackson ObjectMapper to build the body for
startRollingUpdate and setRollingUpdateRouting) so oldVersion/newVersion and
weights are correctly escaped, and URL-encode any path segments (use
URLEncoder.encode(updateId, StandardCharsets.UTF_8) or similar) when composing
URLs for getRollingUpdateStatus, setRollingUpdateRouting, completeRollingUpdate
and rollbackRollingUpdate so updateId cannot break the path.
| class MetricsE2ETest { | ||
| private static final Path PROJECT_ROOT = Path.of(System.getProperty("project.basedir", "..")); | ||
| private static final Duration WAIT_TIMEOUT = Duration.ofSeconds(30); | ||
| private static final Duration METRICS_INTERVAL = Duration.ofSeconds(2); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for metrics collection interval configuration in production code
rg -n "metrics.*interval|collection.*interval|METRICS_INTERVAL" --type java -g '!*Test.java' -i -C 3Repository: siy/aether
Length of output: 5571
🏁 Script executed:
# Find MetricsScheduler mentioned in learnings
fd -e java | xargs grep -l "MetricsScheduler" | head -20Repository: siy/aether
Length of output: 374
🏁 Script executed:
# Examine the test file to understand METRICS_INTERVAL usage
cat -n e2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.java | head -150Repository: siy/aether
Length of output: 5592
🏁 Script executed:
# Check MetricsScheduler for DEFAULT_INTERVAL_MS
cat -n node/src/main/java/org/pragmatica/aether/metrics/MetricsScheduler.java | head -60Repository: siy/aether
Length of output: 2699
🏁 Script executed:
# Check DeploymentMetricsScheduler for DEFAULT_INTERVAL_MS
cat -n node/src/main/java/org/pragmatica/aether/metrics/deployment/DeploymentMetricsScheduler.java | head -60Repository: siy/aether
Length of output: 2879
🏁 Script executed:
# Search for where METRICS_INTERVAL is used in tests
rg -n "METRICS_INTERVAL|pollInterval|pollDelay" e2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.javaRepository: siy/aether
Length of output: 298
Test polling interval is 2 seconds, not 1 second; rename test method to reflect actual polling behavior
The production metrics collection runs at 1-second intervals (confirmed in MetricsScheduler.metricsScheduler() which defaults to 1000ms). The METRICS_INTERVAL constant of 2 seconds is the test polling interval, which is intentional and acceptable for E2E tests. However, the test method name metricsCollected_everySecond() on line 60 is misleading since the test actually polls at 2-second intervals. Rename this method to accurately describe the polling behavior (e.g., metricsCollected_withCustomPolling() or metricsCollected_everyTwoSeconds()).
🤖 Prompt for AI Agents
In e2e-tests/src/test/java/org/pragmatica/aether/e2e/MetricsE2ETest.java around
line 30 (and update the test method at line ~60), the test method name
metricsCollected_everySecond() is misleading because METRICS_INTERVAL is 2
seconds; rename the test method to a name that reflects the actual polling
interval (e.g., metricsCollected_everyTwoSeconds() or
metricsCollected_withCustomPolling()) and update any method references, display
names or annotations that mention the old name so the test name accurately
describes the 2-second polling behavior.
- Update CLI version to 0.7.1 - Complete CHANGELOG test class listing (all 12 classes) - Remove redundant conditionals in BootstrapE2ETest - Remove redundant conditional in ManagementApiE2ETest
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.java (3)
91-91: Extract hard-coded artifact coordinate to a constant.The artifact coordinate
"org.pragmatica-lite.aether:example-slice:0.7.1"appears multiple times in this file (lines 91, 128, 265). Extract it to a private static final constant at the class level to improve maintainability and ensure consistency across tests.🔎 Proposed refactor
Add a constant at the class level:
class ManagementApiE2ETest { private static final Path PROJECT_ROOT = Path.of(System.getProperty("project.basedir", "..")); private static final Duration WAIT_TIMEOUT = Duration.ofSeconds(30); + private static final String EXAMPLE_SLICE_ARTIFACT = "org.pragmatica-lite.aether:example-slice:0.7.1"; private AetherCluster cluster;Then use it throughout the file:
- cluster.anyNode().deploy("org.pragmatica-lite.aether:example-slice:0.7.1", 1); + cluster.anyNode().deploy(EXAMPLE_SLICE_ARTIFACT, 1);
164-225: Consider adding tests for actual alert triggering behavior.The current tests verify threshold and alert management API operations (CRUD), but don't test whether alerts are actually triggered when thresholds are breached. Consider adding a test that:
- Sets a threshold for a metric
- Generates load to breach the threshold
- Verifies an alert appears in active alerts
This would provide more comprehensive coverage of the monitoring/alerting subsystem.
237-237: Consider extracting JSON config to a constant.The hard-coded JSON configuration string could be extracted to a private static final constant for better maintainability and reusability if needed in future tests.
🔎 Proposed refactor
class ManagementApiE2ETest { private static final Path PROJECT_ROOT = Path.of(System.getProperty("project.basedir", "..")); private static final Duration WAIT_TIMEOUT = Duration.ofSeconds(30); + private static final String CONTROLLER_CONFIG_JSON = "{\"scaleUpThreshold\":0.85,\"scaleDownThreshold\":0.15}"; private AetherCluster cluster;- var newConfig = "{\"scaleUpThreshold\":0.85,\"scaleDownThreshold\":0.15}"; + var newConfig = CONTROLLER_CONFIG_JSON; var updateResponse = cluster.anyNode().setControllerConfig(newConfig);e2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.java (3)
77-100: Consider clarifying test scope vs. name.The test name suggests verification of local state recovery (from disk), but the test only confirms that the slice remains visible after node restart. This could simply mean the restarted node synced state from peer nodes rather than recovering its own persisted state. Consider either:
- Renaming to
nodeRestart_maintainsClusterStateto reflect cluster-level consistency, or- Adding verification that the specific node recovered data from its own storage
102-116: Consider adjusting test name to match verification scope.The test name suggests availability is verified during the rolling restart, but the test only checks cluster health after the rolling restart completes. Consider renaming to
rollingRestart_completesSuccessfullyorrollingRestart_recoversToHealthyStateto better reflect what's actually verified.
118-137: Consider verifying leader identity before assuming node-1.The comment on line 122 states "Restart each non-leader node one at a time," assuming node-1 is the leader. While this may be true in practice due to deterministic startup order, consider either:
- Explicitly verifying which node is the leader and excluding it from restarts, or
- Updating the comment to reflect that nodes 2 and 3 are restarted regardless of leadership
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdcli/src/main/java/org/pragmatica/aether/cli/AetherCli.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.java: Return exactly one of: T (synchronous infallible), Option (synchronous optional), Result (synchronous fallible), or Promise (asynchronous fallible). Never use Promise<Result>
Use factory naming convention: TypeName.typeName(...) with lowercase-first method names for creating instances
Never throw business exceptions. All business logic failures must flow through Result or Promise as typed Cause objects
Use sealed interfaces for domain-specific error hierarchies implementing Cause interface
Prefer record-based implementations for use cases, adapters, and data carriers
Use method references over lambdas when the lambda only calls a single method
Ensure all domain objects are immutable records. Use List.of(), Map.of(), Set.of() for collections
Apply timeouts as close to actual I/O operations as possible, not at the end of promise chains
Use Promise.lift() and Result.lift() for wrapping exception-prone operations in error handling
Use adapter leaves for all I/O operations (database, HTTP, file system) to ensure framework independence
Follow single pattern per function rule: Leaf (single operation), Sequencer (linear chain), Fork-Join (parallel), Condition (branching), Iteration (collection processing)
Use .map() and .flatMap() for Result and Promise transformations instead of extracting and re-wrapping values
Package structure must be organized by feature/use case, not by layer (e.g., /userregistration, not /services or /dao)
All Promise-based operations must include timeout configuration relative to their specific operation's expected duration
Keep lambdas simple. Extract complex logic to separate methods to maintain code clarity
Files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.javacli/src/main/java/org/pragmatica/aether/cli/AetherCli.java
**/*Test.java
📄 CodeRabbit inference engine (CLAUDE.md)
**/*Test.java: Group test classes by feature/use case, not by layer. Use descriptive test method names with underscores: method_scenario_expectation()
Use .await() to block on Promise in tests, then test the Result like synchronous code
For Promise success cases in tests, use .onFailure(Assertions::fail).onSuccess(...) pattern
For Promise failure cases in tests, use .onSuccessRun(Assertions::fail).onFailure(...) pattern
Use Result.all() for complex object construction and validation in tests with multiple components
Include roundtrip tests for all parseable types, validating parse(string).map(asString()) == string
Test both success and failure paths for Result operations: valid input cases and failure cases
Import org.junit.jupiter.api.Assertions and org.assertj.core.api.Assertions in test files
Make test classes package-private with no visibility modifier
Use type declarations for stub dependencies in tests instead of inline mock creation
Files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.javae2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.java
**/cli/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
AetherCli must support both interactive REPL mode and batch command mode for cluster management
Files:
cli/src/main/java/org/pragmatica/aether/cli/AetherCli.java
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNode.java : AetherNode assembly must wire all components (SliceStore, DeploymentManagers, MetricsCollector, ControlLoop, ManagementServer) together
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/api/*Server.java : ManagementServer must provide HTTP API for cluster management operations (deploy, scale, list, etc.)
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNode.java : AetherNode assembly must wire all components (SliceStore, DeploymentManagers, MetricsCollector, ControlLoop, ManagementServer) together
Applied to files:
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/cli/*.java : AetherCli must support both interactive REPL mode and batch command mode for cluster management
Applied to files:
cli/src/main/java/org/pragmatica/aether/cli/AetherCli.java
🧬 Code graph analysis (1)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.java (1)
e2e-tests/src/test/java/org/pragmatica/aether/e2e/containers/AetherCluster.java (1)
AetherCluster(39-373)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker-build
🔇 Additional comments (6)
cli/src/main/java/org/pragmatica/aether/cli/AetherCli.java (1)
44-44: LGTM! Version strings updated consistently.The version annotations and REPL startup message are correctly updated to 0.7.1, aligning with the release objectives.
Also applies to: 92-92
e2e-tests/src/test/java/org/pragmatica/aether/e2e/ManagementApiE2ETest.java (3)
1-44: LGTM: Test structure follows guidelines.The test class structure is correct: package-private visibility, proper imports (AssertJ, JUnit 5, Awaitility), and standard BeforeEach/AfterEach lifecycle management for the cluster.
259-275: Basic smoke test coverage is acceptable.The test verifies the slice status endpoint responds without error after deployment. While the assertions are minimal (only checking for absence of error), this is acceptable for initial E2E coverage of the management API.
140-145: No action needed — null parameter handling is already correctly implemented and documented.The
getInvocationMetrics(String artifact, String method)method inAetherNodeContainerexplicitly handles null parameters with if-checks (lines 372, 375), skipping parameters when they are null. The contract is clearly documented in the JavaDoc ("null to skip") and in the API reference documentation (docs/api/management-api.md), which lists bothartifactandmethodas optional query parameters. The test correctly validates this behavior.e2e-tests/src/test/java/org/pragmatica/aether/e2e/BootstrapE2ETest.java (2)
34-46: LGTM!The setup and teardown lifecycle is appropriate for E2E testing. The defensive null check in tearDown() is reasonable defensive programming.
48-75: LGTM!The test correctly verifies that a restarted node rejoins the cluster and that quorum is maintained throughout the process. The additional manual health verification provides good coverage beyond the quorum check.
aether-config module provides shared configuration infrastructure: - TOML-based configuration with environment-aware defaults - Environment profiles: LOCAL (3 nodes), DOCKER (5 nodes), KUBERNETES (5 nodes) - ConfigLoader with override resolution (CLI → file → environment) - ConfigValidator with validation rules (odd nodes 3-7, port conflicts, TLS) aether-setup module provides production cluster setup tool (aether-up): - LocalGenerator: shell scripts for single-machine deployment - DockerGenerator: docker-compose.yml with health checks - KubernetesGenerator: StatefulSet, headless service, PDB manifests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (24)
aether-setup/pom.xml (1)
52-71: Optional: Consider shade plugin enhancements for production robustness.The current configuration is functional. However, for additional robustness in production, consider:
- Signature file exclusion — Add filters to exclude
META-INF/*.SF,*.DSA,*.RSAfrom bundled JARs to prevent potential conflicts.- ServicesResourceTransformer — Add only if transitive dependencies use Java's
ServiceLoaderpattern (not currently evident in aether-setup's direct code, but pragmatica-lite external dependencies may use it).These are defensive best practices, not critical requirements.
aether-config/src/main/java/org/pragmatica/aether/config/ResourcesConfig.java (1)
10-17: Consider adding resource format validation.The record uses String fields for Kubernetes resource specifications, which is appropriate. However, invalid formats (e.g., "invalid" instead of "500m") can be constructed and will only fail at Kubernetes deployment time rather than at configuration validation time.
Consider adding validation in
ConfigValidatorto verify that resource strings match Kubernetes patterns (e.g., CPU:^\d+m?$, memory:^\d+(Mi|Gi)$).aether-config/src/main/java/org/pragmatica/aether/config/DockerConfig.java (1)
8-16: Consider documenting image version pinning for production.The record correctly implements an immutable configuration with appropriate defaults. However, using the
"latest"tag inDEFAULT_IMAGEcan lead to non-reproducible deployments in production environments.Consider documenting (either in JavaDoc or project documentation) that users should override this default with pinned versions (e.g.,
ghcr.io/siy/aether-node:0.7.1) for production deployments.aether-config/src/main/java/org/pragmatica/aether/config/TlsConfig.java (1)
36-38: Consider including CA path in validation.The
hasProvidedCertificates()method checkscertPathandkeyPathbut notcaPath. If CA certificates are required for TLS validation, this should be included in the check. If CA is optional, consider clarifying this with a comment.🔎 Proposed fix if CA is required
public boolean hasProvidedCertificates() { - return !autoGenerate && !certPath.isBlank() && !keyPath.isBlank(); + return !autoGenerate && !certPath.isBlank() && !keyPath.isBlank() && !caPath.isBlank(); }aether-setup/src/main/java/org/pragmatica/aether/setup/generators/LocalGenerator.java (3)
33-33: Add space before opening brace.Minor formatting:
try{should betry {for consistency.- try{ + try {
111-118: Prefer String.join() or Collectors.joining() for string concatenation.Using
reduce()for string building is less clear than usingString.join()orCollectors.joining().🔎 Proposed refactor
-var peerList = IntStream.range(0, nodes) - .mapToObj(i -> "localhost:" + (clusterPort + i)) - .reduce((a, b) -> a + "," + b) - .orElse(""); +var peerList = IntStream.range(0, nodes) + .mapToObj(i -> "localhost:" + (clusterPort + i)) + .collect(Collectors.joining(",")); + -var nodeStarts = IntStream.range(0, nodes) - .mapToObj(i -> generateNodeStart(i, config, peerList)) - .reduce((a, b) -> a + "\n" + b) - .orElse(""); +var nodeStarts = IntStream.range(0, nodes) + .mapToObj(i -> generateNodeStart(i, config, peerList)) + .collect(Collectors.joining("\n"));
246-250: Improve formatting and add comment for empty catch block.Two minor issues:
- Line 247:
try{should betry {- Line 249: Empty catch block should have a comment explaining it's intentional for non-POSIX systems.
🔎 Proposed fix
private void makeExecutable(Path path) throws IOException { - try{ + try { Files.setPosixFilePermissions(path, PosixFilePermissions.fromString("rwxr-xr-x")); - } catch (UnsupportedOperationException e) {} + } catch (UnsupportedOperationException e) { + // Ignore: POSIX permissions not supported on this file system (e.g., Windows) + } }aether-config/src/main/java/org/pragmatica/aether/config/NodeConfig.java (2)
62-69: Minor formatting: missing spaces aftercasekeywords.The switch expression has formatting issues with missing spaces between
caseand the string literals.🔎 Proposed fix
public String javaOpts() { var gcOpt = switch (gc.toLowerCase()) { - case"zgc" -> "-XX:+UseZGC -XX:+ZGenerational"; - case"g1" -> "-XX:+UseG1GC"; + case "zgc" -> "-XX:+UseZGC -XX:+ZGenerational"; + case "g1" -> "-XX:+UseG1GC"; default -> "-XX:+UseZGC -XX:+ZGenerational"; }; return "-Xmx" + heap + " " + gcOpt; }
41-57: Consider addingwithMetricsInterval()andwithReconciliation()for API consistency.The record provides
withHeap(),withGc(), andwithResources()but lacks fluent methods formetricsIntervalandreconciliation. This creates an incomplete mutation API.aether-config/src/test/java/org/pragmatica/aether/config/ConfigLoaderTest.java (1)
177-183: Incomplete success verification pattern.The test only verifies validation doesn't fail but doesn't assert anything about the returned config. Per coding guidelines, success cases should use
.onFailure(Assertions::fail).onSuccess(...)to verify the result.🔎 Proposed fix
@Test void fromEnvironment_returnsValidConfig() { var config = ConfigLoader.fromEnvironment(Environment.DOCKER); ConfigValidator.validate(config) - .onFailureRun(Assertions::fail); + .onFailure(cause -> Assertions.fail(cause.message())) + .onSuccess(validatedConfig -> { + assertThat(validatedConfig.environment()).isEqualTo(Environment.DOCKER); + }); }aether-config/src/main/java/org/pragmatica/aether/config/ConfigValidator.java (3)
26-26: Unused constantVALID_NODE_COUNTS.The constant
VALID_NODE_COUNTSis defined but never used in validation logic. The actual validation on lines 52-59 performs manual checks instead of using this set. Either use the constant or remove it.🔎 Proposed fix - use the constant
private static void validateCluster(ClusterConfig cluster, List<String> errors) { // Node count validation int nodes = cluster.nodes(); - if (nodes < 3) { - errors.add("Minimum 3 nodes required for fault tolerance. Got: " + nodes); - }else if (nodes % 2 == 0) { - errors.add("Node count must be odd (3, 5, 7) for quorum. Got: " + nodes); - }else if (nodes > 7) { - errors.add("Maximum recommended node count is 7. Got: " + nodes - + ". More nodes add overhead without proportional benefit."); + if (!VALID_NODE_COUNTS.contains(nodes)) { + if (nodes < 3) { + errors.add("Minimum 3 nodes required for fault tolerance. Got: " + nodes); + } else if (nodes % 2 == 0) { + errors.add("Node count must be odd (3, 5, 7) for quorum. Got: " + nodes); + } else { + errors.add("Maximum recommended node count is 7. Got: " + nodes + + ". More nodes add overhead without proportional benefit."); + } }
49-79: Minor formatting: missing spaces afterelsekeywords.Lines 54 and 56 have
}else ifand}else ifwithout spaces.🔎 Proposed fix
if (nodes < 3) { errors.add("Minimum 3 nodes required for fault tolerance. Got: " + nodes); - }else if (nodes % 2 == 0) { + } else if (nodes % 2 == 0) { errors.add("Node count must be odd (3, 5, 7) for quorum. Got: " + nodes); - }else if (nodes > 7) { + } else if (nodes > 7) { errors.add("Maximum recommended node count is 7. Got: " + nodes
107-137: Inconsistent lambda indentation in TLS validation.The lambda bodies inside
ifPresenthave inconsistent indentation that reduces readability.🔎 Proposed fix
private static void validateTls(TlsConfig tls, List<String> errors) { if (!tls.autoGenerate()) { // User-provided certificates - check paths exist tls.certFile() - .ifPresent(path -> { - if (!Files.exists(path)) { - errors.add("Certificate file not found: " + path); - } - }); + .ifPresent(path -> { + if (!Files.exists(path)) { + errors.add("Certificate file not found: " + path); + } + }); tls.keyFile() - .ifPresent(path -> { - if (!Files.exists(path)) { - errors.add("Private key file not found: " + path); - } - }); + .ifPresent(path -> { + if (!Files.exists(path)) { + errors.add("Private key file not found: " + path); + } + }); tls.caFile() - .ifPresent(path -> { - if (!Files.exists(path)) { - errors.add("CA certificate file not found: " + path); - } - }); + .ifPresent(path -> { + if (!Files.exists(path)) { + errors.add("CA certificate file not found: " + path); + } + });aether-setup/src/main/java/org/pragmatica/aether/setup/AetherUp.java (2)
54-65: Simplify Result handling afterisFailure()check.The code checks
result.isFailure()and then usesfold(cause -> null, c -> c)to extract the config. This is redundant and thenullfallback is never reached. Consider using a cleaner pattern.🔎 Proposed fix
if (configPath.toFile() .exists()) { var overrides = extractOverrides(options); var result = ConfigLoader.loadWithOverrides(configPath, overrides); - if (result.isFailure()) { - System.err.println("Error loading configuration:"); - result.onFailure(cause -> System.err.println(" " + cause.message())); - System.exit(1); - return; - } - config = result.fold(cause -> null, c -> c); - }else { + config = result.fold( + cause -> { + System.err.println("Error loading configuration:"); + System.err.println(" " + cause.message()); + System.exit(1); + return null; // unreachable + }, + c -> c); + } else {
114-142: Unknown CLI arguments are silently ignored.Unrecognized arguments are silently skipped. This could hide typos (e.g.,
--nodeinstead of--nodes). Consider warning on unknown arguments.Also, minor formatting: missing spaces after
casekeywords throughout the switch.aether-setup/src/main/java/org/pragmatica/aether/setup/generators/GeneratorOutput.java (1)
15-31: Ensure immutability by copying the file list.The
List<Path>parameter is stored directly without defensive copying. If the caller modifies the original list, it affects the record's state. Per coding guidelines, useList.copyOf()for collections in domain objects.🔎 Proposed fix
public record GeneratorOutput( Path outputDir, List<Path> generatedFiles, Path startScript, Path stopScript, String instructions) { + public GeneratorOutput { + generatedFiles = List.copyOf(generatedFiles); + } + public static GeneratorOutput of(Path outputDir, List<Path> files, String instructions) { - return new GeneratorOutput(outputDir, files, null, null, instructions); + return new GeneratorOutput(outputDir, List.copyOf(files), null, null, instructions); } public static GeneratorOutput withScripts(Path outputDir, List<Path> files, Path start, Path stop, String instructions) { - return new GeneratorOutput(outputDir, files, start, stop, instructions); + return new GeneratorOutput(outputDir, List.copyOf(files), start, stop, instructions); } }aether-config/src/main/java/org/pragmatica/aether/config/Environment.java (1)
41-53: Consider returningResult<Environment>instead of throwing.Per coding guidelines, business logic failures should flow through
Result, not exceptions.fromString()throwsIllegalArgumentExceptionfor unknown values, which callers must catch. ReturningResult<Environment>would be more consistent with the codebase patterns.🔎 Proposed fix
+ import org.pragmatica.lang.Result; + /** * Parse environment from string, case-insensitive. */ - public static Environment fromString(String value) { + public static Result<Environment> fromString(String value) { if (value == null || value.isBlank()) { - return DOCKER; + return Result.success(DOCKER); } return switch (value.toLowerCase() .trim()) { - case"local" -> LOCAL; - case"docker" -> DOCKER; - case"kubernetes", "k8s" -> KUBERNETES; - default -> throw new IllegalArgumentException( - "Unknown environment: " + value + ". Valid: local, docker, kubernetes"); + case "local" -> Result.success(LOCAL); + case "docker" -> Result.success(DOCKER); + case "kubernetes", "k8s" -> Result.success(KUBERNETES); + default -> Result.failure(/* appropriate Cause */); }; }Also, minor formatting: missing spaces after
casekeywords on lines 47-49.This change would require updates to callers like
AetherUp.javaline 68 that currently callEnvironment.fromString()directly.aether-setup/src/main/java/org/pragmatica/aether/setup/generators/KubernetesGenerator.java (3)
129-135: Edge case:reduce()returns empty string when nodes=0.The
reduce()call returns""whennodes=0, which would generate an emptyCLUSTER_PEERSvalue. WhileConfigValidatorlikely catches this, the silent empty string could be confusing during debugging.🔎 Consider using Collectors.joining for clarity
- var peerList = IntStream.range(0, nodes) - .mapToObj(i -> String.format("aether-node-%d.aether-headless.%s.svc.cluster.local:%d", - i, - namespace, - clusterPort)) - .reduce((a, b) -> a + "," + b) - .orElse(""); + var peerList = IntStream.range(0, nodes) + .mapToObj(i -> String.format("aether-node-%d.aether-headless.%s.svc.cluster.local:%d", + i, + namespace, + clusterPort)) + .collect(Collectors.joining(","));
154-161: Simplify GC selection logic.The current pattern
toUpperCase().equals("ZGC") ? "ZGC" : "G1GC"is verbose. Consider extracting to a helper or usingequalsIgnoreCase.🔎 Simplified GC selection
- config.node() - .gc() - .toUpperCase() - .equals("ZGC") - ? "ZGC" - : "G1GC"); + "zgc".equalsIgnoreCase(config.node().gc()) ? "ZGC" : "G1GC");
190-194: Parameterize the container image tag viaKubernetesConfig.The image is hardcoded to
ghcr.io/siy/aether-node:latest. Usinglatestin production deployments can lead to unpredictable behavior when images are updated. Add animageTagfield toKubernetesConfigand pass it as a parameter to theString.format()call ingenerateStatefulSet(), with a sensible default value.aether-setup/src/main/java/org/pragmatica/aether/setup/generators/DockerGenerator.java (1)
188-195: Duplicate GC selection logic with KubernetesGenerator.Both generators have identical GC selection logic. Consider extracting to a shared utility method.
aether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.java (2)
53-58: Inconsistent return type:fromEnvironmentreturnsAetherConfigdirectly.Other methods return
Result<AetherConfig>, but this one returns the value directly. While semantically correct (environment defaults are infallible), consider wrapping inResult.success()for API consistency, or document why this is intentional.
78-80: Duplicate TLS parsing logic.TLS boolean parsing appears twice with identical logic. Extract to a local variable or helper.
🔎 Consolidate TLS parsing
+ // Parse TLS setting once + var tlsEnabled = doc.getString("cluster", "tls") + .map(s -> "true".equalsIgnoreCase(s)) + .or(environment.defaultTls()); + builder.tls(tlsEnabled); + // Ports var mgmtPort = doc.getInt("cluster.ports", "management") .or(PortsConfig.DEFAULT_MANAGEMENT_PORT); ... - // TLS config - var tlsEnabled = doc.getString("cluster", "tls") - .map(s -> "true".equalsIgnoreCase(s)) - .or(environment.defaultTls()); + // TLS config details if (tlsEnabled) {Also applies to: 93-95
aether-config/src/main/java/org/pragmatica/aether/config/AetherConfig.java (1)
163-167: Potential inconsistency: TLS config fallback ignores explicittls=falseoverride.If
tlsConfigis null andclusterConfig.tls()is true (from defaults),TlsConfig.autoGenerated()is used. However, if the user explicitly callsbuilder.tls(false), line 151 updatesclusterConfig, but line 165 still checksclusterConfig.tls(). This works correctly, but the logic flow is non-obvious.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
aether-config/pom.xmlaether-config/src/main/java/org/pragmatica/aether/config/AetherConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/ClusterConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.javaaether-config/src/main/java/org/pragmatica/aether/config/ConfigValidator.javaaether-config/src/main/java/org/pragmatica/aether/config/DockerConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/Environment.javaaether-config/src/main/java/org/pragmatica/aether/config/KubernetesConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/NodeConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/PortsConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/ResourcesConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/TlsConfig.javaaether-config/src/test/java/org/pragmatica/aether/config/AetherConfigTest.javaaether-config/src/test/java/org/pragmatica/aether/config/ConfigLoaderTest.javaaether-config/src/test/java/org/pragmatica/aether/config/ConfigValidatorTest.javaaether-config/src/test/java/org/pragmatica/aether/config/EnvironmentTest.javaaether-setup/pom.xmlaether-setup/src/main/java/org/pragmatica/aether/setup/AetherUp.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/DockerGenerator.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/Generator.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/GeneratorError.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/GeneratorOutput.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/KubernetesGenerator.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/LocalGenerator.javapom.xml
✅ Files skipped from review due to trivial changes (1)
- aether-config/pom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.java: Return exactly one of: T (synchronous infallible), Option (synchronous optional), Result (synchronous fallible), or Promise (asynchronous fallible). Never use Promise<Result>
Use factory naming convention: TypeName.typeName(...) with lowercase-first method names for creating instances
Never throw business exceptions. All business logic failures must flow through Result or Promise as typed Cause objects
Use sealed interfaces for domain-specific error hierarchies implementing Cause interface
Prefer record-based implementations for use cases, adapters, and data carriers
Use method references over lambdas when the lambda only calls a single method
Ensure all domain objects are immutable records. Use List.of(), Map.of(), Set.of() for collections
Apply timeouts as close to actual I/O operations as possible, not at the end of promise chains
Use Promise.lift() and Result.lift() for wrapping exception-prone operations in error handling
Use adapter leaves for all I/O operations (database, HTTP, file system) to ensure framework independence
Follow single pattern per function rule: Leaf (single operation), Sequencer (linear chain), Fork-Join (parallel), Condition (branching), Iteration (collection processing)
Use .map() and .flatMap() for Result and Promise transformations instead of extracting and re-wrapping values
Package structure must be organized by feature/use case, not by layer (e.g., /userregistration, not /services or /dao)
All Promise-based operations must include timeout configuration relative to their specific operation's expected duration
Keep lambdas simple. Extract complex logic to separate methods to maintain code clarity
Files:
aether-config/src/main/java/org/pragmatica/aether/config/DockerConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/PortsConfig.javaaether-config/src/test/java/org/pragmatica/aether/config/ConfigValidatorTest.javaaether-config/src/main/java/org/pragmatica/aether/config/Environment.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/GeneratorError.javaaether-config/src/test/java/org/pragmatica/aether/config/ConfigLoaderTest.javaaether-config/src/main/java/org/pragmatica/aether/config/KubernetesConfig.javaaether-config/src/test/java/org/pragmatica/aether/config/AetherConfigTest.javaaether-config/src/main/java/org/pragmatica/aether/config/NodeConfig.javaaether-setup/src/main/java/org/pragmatica/aether/setup/AetherUp.javaaether-config/src/main/java/org/pragmatica/aether/config/ConfigValidator.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/KubernetesGenerator.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/GeneratorOutput.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/Generator.javaaether-config/src/main/java/org/pragmatica/aether/config/TlsConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.javaaether-config/src/main/java/org/pragmatica/aether/config/ClusterConfig.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/DockerGenerator.javaaether-config/src/main/java/org/pragmatica/aether/config/ResourcesConfig.javaaether-config/src/test/java/org/pragmatica/aether/config/EnvironmentTest.javaaether-setup/src/main/java/org/pragmatica/aether/setup/generators/LocalGenerator.javaaether-config/src/main/java/org/pragmatica/aether/config/AetherConfig.java
**/*Test.java
📄 CodeRabbit inference engine (CLAUDE.md)
**/*Test.java: Group test classes by feature/use case, not by layer. Use descriptive test method names with underscores: method_scenario_expectation()
Use .await() to block on Promise in tests, then test the Result like synchronous code
For Promise success cases in tests, use .onFailure(Assertions::fail).onSuccess(...) pattern
For Promise failure cases in tests, use .onSuccessRun(Assertions::fail).onFailure(...) pattern
Use Result.all() for complex object construction and validation in tests with multiple components
Include roundtrip tests for all parseable types, validating parse(string).map(asString()) == string
Test both success and failure paths for Result operations: valid input cases and failure cases
Import org.junit.jupiter.api.Assertions and org.assertj.core.api.Assertions in test files
Make test classes package-private with no visibility modifier
Use type declarations for stub dependencies in tests instead of inline mock creation
Files:
aether-config/src/test/java/org/pragmatica/aether/config/ConfigValidatorTest.javaaether-config/src/test/java/org/pragmatica/aether/config/ConfigLoaderTest.javaaether-config/src/test/java/org/pragmatica/aether/config/AetherConfigTest.javaaether-config/src/test/java/org/pragmatica/aether/config/EnvironmentTest.java
**/*Error.java
📄 CodeRabbit inference engine (CLAUDE.md)
Group related error types in sealed interface hierarchies and locate in same package as domain logic
Files:
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/GeneratorError.java
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNode.java : AetherNode assembly must wire all components (SliceStore, DeploymentManagers, MetricsCollector, ControlLoop, ManagementServer) together
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNodeConfig.java : AetherNodeConfig must provide all runtime configuration for node (timeouts, cluster addresses, metrics intervals, etc.)
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNodeConfig.java : AetherNodeConfig must provide all runtime configuration for node (timeouts, cluster addresses, metrics intervals, etc.)
Applied to files:
aether-config/src/main/java/org/pragmatica/aether/config/DockerConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/PortsConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/KubernetesConfig.javaaether-config/src/test/java/org/pragmatica/aether/config/AetherConfigTest.javaaether-config/src/main/java/org/pragmatica/aether/config/NodeConfig.javaaether-setup/src/main/java/org/pragmatica/aether/setup/AetherUp.javaaether-config/src/main/java/org/pragmatica/aether/config/ConfigValidator.javaaether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.javaaether-config/src/main/java/org/pragmatica/aether/config/ClusterConfig.javaaether-config/src/main/java/org/pragmatica/aether/config/AetherConfig.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/*.java : Use sealed interfaces for domain-specific error hierarchies implementing Cause interface
Applied to files:
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/GeneratorError.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/*Error.java : Group related error types in sealed interface hierarchies and locate in same package as domain logic
Applied to files:
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/GeneratorError.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/cli/*.java : AetherCli must support both interactive REPL mode and batch command mode for cluster management
Applied to files:
aether-setup/src/main/java/org/pragmatica/aether/setup/AetherUp.java
🧬 Code graph analysis (3)
aether-config/src/test/java/org/pragmatica/aether/config/ConfigValidatorTest.java (1)
aether-config/src/main/java/org/pragmatica/aether/config/ConfigValidator.java (1)
ConfigValidator(25-155)
aether-config/src/test/java/org/pragmatica/aether/config/ConfigLoaderTest.java (2)
aether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.java (1)
ConfigLoader(22-180)aether-config/src/main/java/org/pragmatica/aether/config/ConfigValidator.java (1)
ConfigValidator(25-155)
aether-setup/src/main/java/org/pragmatica/aether/setup/AetherUp.java (1)
aether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.java (1)
ConfigLoader(22-180)
🔇 Additional comments (38)
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/GeneratorError.java (2)
8-21: Excellent error hierarchy design.The sealed interface pattern with record-based implementations perfectly follows the coding guidelines. The error types clearly represent distinct failure modes (I/O failures vs. environment mismatches), and the message overrides provide actionable descriptions.
Based on coding guidelines and learnings.
23-29: LGTM - Factory methods follow conventions.The static factory methods correctly follow the
TypeName.typeName(...)naming convention and provide a clean API for creating error instances.Based on coding guidelines.
aether-setup/pom.xml (2)
17-29: Dependency structure looks correct.The use of
${project.version}foraether-configensures version consistency across sibling modules. The unversionedcoredependency correctly relies on parent POM management.
7-11: Parent POM correctly manages all dependencies.The parent POM (
org.pragmatica-lite.aether:aether:0.7.1) properly declares all unversioned dependencies in its<dependencyManagement>section:
org.pragmatica-lite:core(version 0.9.5)org.junit.jupiter:junit-jupiter-api(version 5.13.4)org.junit.jupiter:junit-jupiter-engine(version 5.13.4)org.assertj:assertj-core(version 3.26.3)The aether-setup module correctly omits versions to inherit them from the parent.
aether-config/src/test/java/org/pragmatica/aether/config/ConfigValidatorTest.java (8)
10-26: LGTM!The test methods correctly validate success scenarios using appropriate Result handling patterns. The use of
.onFailureRun(Assertions::fail)for simple success assertions and.onFailure(cause -> Assertions.fail(...))with contextual error messages is idiomatic.
28-65: LGTM!The node count validation tests properly exercise boundary conditions (too low, even numbers, too high) using the correct Result failure handling pattern.
67-78: LGTM!The parameterized test correctly validates all accepted node counts (3, 5, 7) with appropriate error context.
80-104: LGTM!The heap format validation tests correctly verify both invalid formats and a comprehensive set of valid formats (with case variations) that match the validation pattern.
106-130: LGTM!The GC validation tests correctly verify both invalid options and valid options with case variations, matching the case-insensitive validation logic.
132-171: LGTM!The port validation tests correctly verify conflict detection, range validation, and overlap detection. The inline comment explaining the overlap scenario (line 160) is particularly helpful.
173-190: LGTM!Excellent test verifying that the validator accumulates all errors rather than failing fast. This ensures comprehensive validation feedback for users.
192-219: LGTM!The TLS validation tests correctly verify both scenarios: requiring explicit certificate paths when auto-generation is disabled, and accepting configurations with auto-generation enabled.
aether-config/src/main/java/org/pragmatica/aether/config/ResourcesConfig.java (1)
19-24: LGTM!The
minimal()factory provides appropriately scaled-down resource specifications for local/test environments, which is useful for development scenarios.aether-config/src/test/java/org/pragmatica/aether/config/EnvironmentTest.java (3)
9-34: LGTM!The default configuration tests correctly verify environment-specific defaults that scale appropriately from local development (3 nodes, minimal resources, no TLS) to production Kubernetes (5 nodes, larger heap, TLS enabled).
36-71: LGTM!The parsing tests comprehensively cover case-insensitive matching, the "k8s" alias for Kubernetes, default behavior for blank/null inputs, and error handling for unknown values.
73-78: LGTM!The display name test verifies consistent lowercase formatting across all environment types.
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/Generator.java (1)
1-32: LGTM!The interface correctly uses
Result<GeneratorOutput>for the falliblegenerate()operation and provides clear documentation explaining the purpose and expected implementations. The method signatures follow the coding guidelines for return type conventions.aether-config/src/main/java/org/pragmatica/aether/config/DockerConfig.java (1)
18-24: LGTM!The fluent mutator methods correctly implement the immutable update pattern, returning new
DockerConfiginstances while preserving unchanged fields.aether-config/src/test/java/org/pragmatica/aether/config/AetherConfigTest.java (1)
7-155: LGTM! Well-structured test coverage.The test class follows all coding guidelines: package-private visibility, descriptive test naming with underscores, appropriate AssertJ assertions, and focused test methods covering environment creation, defaults, and builder overrides.
aether-config/src/main/java/org/pragmatica/aether/config/TlsConfig.java (1)
22-31: LGTM! Factory methods follow conventions.The static factory methods follow the lowercase factory naming convention and clearly express intent for the two TLS configuration modes.
aether-config/src/main/java/org/pragmatica/aether/config/PortsConfig.java (1)
1-31: LGTM! Clean port configuration implementation.The record follows factory naming conventions, and the port calculation methods are straightforward. Node index validation is appropriately handled elsewhere (ConfigValidator per PR objectives).
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/LocalGenerator.java (1)
26-29: LGTM! Environment check is correct.aether-config/src/main/java/org/pragmatica/aether/config/ClusterConfig.java (1)
1-43: LGTM! Clean immutable configuration record.The implementation follows the factory naming convention and immutable record pattern with "with" methods. The comment mentions that nodes must be odd (3, 5, 7), and per the PR objectives, this validation is appropriately handled by ConfigValidator rather than in the record itself.
aether-config/src/main/java/org/pragmatica/aether/config/KubernetesConfig.java (1)
9-35: LGTM!Clean immutable record implementation with proper fluent mutation methods and null-safe
hasStorageClass()check. Follows coding guidelines for domain objects.aether-setup/src/main/java/org/pragmatica/aether/setup/generators/KubernetesGenerator.java (4)
29-32: LGTM!Clean implementation of the
supports()method using theEnvironmentenum.
34-104: LGTM on the overall generate() structure.The method correctly uses
Resultfor error handling and generates all required Kubernetes manifests. The try-catch wrapping IOException and returning aGeneratorErroraligns with the coding guidelines for fallible operations.
345-358:generateDeleteScriptignores theconfigparameter.The method accepts
AetherConfigbut doesn't use it. While not a bug (the script is generic), this could be intentional or a forgotten namespace reference.
360-364: Empty catch block is acceptable here.Silently ignoring
UnsupportedOperationExceptionfor non-POSIX systems is appropriate since executable permissions are not critical on Windows.aether-setup/src/main/java/org/pragmatica/aether/setup/generators/DockerGenerator.java (3)
29-32: LGTM!Correct environment check matching Docker deployments.
34-100: LGTM on generate() structure.The method follows the same robust pattern as
KubernetesGenerator, usingResultfor error handling and properly generating all deployment artifacts.
278-282: Empty catch block is acceptable here.Same rationale as KubernetesGenerator - graceful degradation on non-POSIX systems.
aether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.java (3)
28-32: LGTM!Clean composition using
flatMapfor chaining parsing, document conversion, and validation.
127-135: CLI override parsing can throwNumberFormatException.
Integer.parseInt(overrides.get("nodes"))at line 128 will throw if the user provides a non-numeric value. This is already inside a try-catch forIllegalArgumentException, butNumberFormatExceptionis a subclass of that, so it's handled. However, the error message may not be user-friendly.
168-179: LGTM on sealed ConfigError interface.The sealed interface with
InvalidConfigrecord properly implementsCauseand provides a factory method, aligning with the coding guidelines for domain-specific error hierarchies.aether-config/src/main/java/org/pragmatica/aether/config/AetherConfig.java (4)
34-39: LGTM on record design.The record properly aggregates all configuration aspects. Using
nullfor environment-specific configs (docker/kubernetes/tls) is a reasonable approach to avoid empty placeholder objects.
43-56: LGTM onforEnvironmentfactory.Clean implementation that properly initializes environment-specific configs based on the
Environmentenum and itsdefaultTls()predicate.
86-140: Builder setter methods look good.Fluent API with nullable wrapper types (
Integer,Boolean) allows distinguishing between "not set" and explicit values. This is appropriate for the override semantics.
142-179:build()method has correct override semantics but could benefit from clarity.The logic correctly applies overrides on top of environment defaults. The ternary chains for
finalTls,finalDocker, andfinalK8sare readable but dense.
| public static Duration parseDuration(String value) { | ||
| if (value == null || value.isBlank()) { | ||
| return Duration.ofSeconds(1); | ||
| } | ||
| value = value.trim() | ||
| .toLowerCase(); | ||
| if (value.endsWith("ms")) { | ||
| return Duration.ofMillis(Long.parseLong(value.substring(0, value.length() - 2))); | ||
| } | ||
| if (value.endsWith("s")) { | ||
| return Duration.ofSeconds(Long.parseLong(value.substring(0, value.length() - 1))); | ||
| } | ||
| if (value.endsWith("m")) { | ||
| return Duration.ofMinutes(Long.parseLong(value.substring(0, value.length() - 1))); | ||
| } | ||
| // Default: assume seconds | ||
| return Duration.ofSeconds(Long.parseLong(value)); | ||
| } |
There was a problem hiding this comment.
parseDuration throws NumberFormatException on invalid input.
Per coding guidelines, fallible operations should return Result<T> instead of throwing exceptions. Long.parseLong() can throw NumberFormatException for malformed input like "abc" or "10x".
🔎 Return Result<Duration> for safe parsing
- public static Duration parseDuration(String value) {
+ public static Result<Duration> parseDuration(String value) {
if (value == null || value.isBlank()) {
- return Duration.ofSeconds(1);
+ return Result.success(Duration.ofSeconds(1));
}
- value = value.trim()
- .toLowerCase();
- if (value.endsWith("ms")) {
- return Duration.ofMillis(Long.parseLong(value.substring(0, value.length() - 2)));
- }
- if (value.endsWith("s")) {
- return Duration.ofSeconds(Long.parseLong(value.substring(0, value.length() - 1)));
+ try {
+ var trimmed = value.trim().toLowerCase();
+ if (trimmed.endsWith("ms")) {
+ return Result.success(Duration.ofMillis(Long.parseLong(trimmed.substring(0, trimmed.length() - 2))));
+ }
+ if (trimmed.endsWith("s")) {
+ return Result.success(Duration.ofSeconds(Long.parseLong(trimmed.substring(0, trimmed.length() - 1))));
+ }
+ if (trimmed.endsWith("m")) {
+ return Result.success(Duration.ofMinutes(Long.parseLong(trimmed.substring(0, trimmed.length() - 1))));
+ }
+ return Result.success(Duration.ofSeconds(Long.parseLong(trimmed)));
+ } catch (NumberFormatException e) {
+ return ConfigError.invalidConfig("Invalid duration format: " + value).result();
}
- if (value.endsWith("m")) {
- return Duration.ofMinutes(Long.parseLong(value.substring(0, value.length() - 1)));
- }
- // Default: assume seconds
- return Duration.ofSeconds(Long.parseLong(value));
}🤖 Prompt for AI Agents
In aether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.java
around lines 146 to 163, parseDuration currently throws NumberFormatException on
malformed input; change its signature to return Result<Duration> and implement
safe parsing: handle null/blank by returning Result.ok(Duration.ofSeconds(1)),
normalize input, detect suffixes ("ms","s","m"), attempt Long.parseLong inside a
try/catch and on success wrap the Duration in Result.ok, and on
NumberFormatException return Result.error with a clear message (include the
original value) so callers receive a failure value instead of an exception.
| public Optional<Path> certFile() { | ||
| return certPath.isBlank() | ||
| ? Optional.empty() | ||
| : Optional.of(Path.of(certPath)); | ||
| } | ||
|
|
||
| public Optional<Path> keyFile() { | ||
| return keyPath.isBlank() | ||
| ? Optional.empty() | ||
| : Optional.of(Path.of(keyPath)); | ||
| } | ||
|
|
||
| public Optional<Path> caFile() { | ||
| return caPath.isBlank() | ||
| ? Optional.empty() | ||
| : Optional.of(Path.of(caPath)); | ||
| } |
There was a problem hiding this comment.
Path.of() can throw InvalidPathException.
The Path.of() calls can throw InvalidPathException for malformed path strings, violating the guideline to never throw business exceptions. Wrap these operations with Result.lift() to handle errors as typed failures.
🔎 Proposed fix using Result.lift()
Update the return types and implementation:
+import org.pragmatica.lang.Result;
+
-public Optional<Path> certFile() {
- return certPath.isBlank()
- ? Optional.empty()
- : Optional.of(Path.of(certPath));
+public Result<Path> certFile() {
+ return certPath.isBlank()
+ ? Result.failure(new InvalidConfigCause("Certificate path is blank"))
+ : Result.lift(() -> Path.of(certPath));
}
-public Optional<Path> keyFile() {
- return keyPath.isBlank()
- ? Optional.empty()
- : Optional.of(Path.of(keyPath));
+public Result<Path> keyFile() {
+ return keyPath.isBlank()
+ ? Result.failure(new InvalidConfigCause("Key path is blank"))
+ : Result.lift(() -> Path.of(keyPath));
}
-public Optional<Path> caFile() {
- return caPath.isBlank()
- ? Optional.empty()
- : Optional.of(Path.of(caPath));
+public Result<Path> caFile() {
+ return caPath.isBlank()
+ ? Result.failure(new InvalidConfigCause("CA path is blank"))
+ : Result.lift(() -> Path.of(caPath));
}You'll need to define an InvalidConfigCause implementing the Cause interface.
Committable suggestion skipped: line range outside the PR's diff.
| @Test | ||
| void loadFromString_fails_withInvalidToml() { | ||
| var toml = "invalid [ toml"; | ||
|
|
||
| ConfigLoader.loadFromString(toml) | ||
| .onSuccessRun(Assertions::fail); | ||
| } |
There was a problem hiding this comment.
Missing failure assertion in invalid TOML test.
The test calls onSuccessRun(Assertions::fail) but doesn't verify that a failure actually occurred. If loadFromString returns success unexpectedly, the test passes silently without any assertion.
🔎 Proposed fix
@Test
void loadFromString_fails_withInvalidToml() {
var toml = "invalid [ toml";
ConfigLoader.loadFromString(toml)
- .onSuccessRun(Assertions::fail);
+ .onSuccessRun(Assertions::fail)
+ .onFailure(cause -> assertThat(cause.message()).isNotBlank());
}📝 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.
| @Test | |
| void loadFromString_fails_withInvalidToml() { | |
| var toml = "invalid [ toml"; | |
| ConfigLoader.loadFromString(toml) | |
| .onSuccessRun(Assertions::fail); | |
| } | |
| @Test | |
| void loadFromString_fails_withInvalidToml() { | |
| var toml = "invalid [ toml"; | |
| ConfigLoader.loadFromString(toml) | |
| .onSuccessRun(Assertions::fail) | |
| .onFailure(cause -> assertThat(cause.message()).isNotBlank()); | |
| } |
🤖 Prompt for AI Agents
In
aether-config/src/test/java/org/pragmatica/aether/config/ConfigLoaderTest.java
around lines 155-161, the test for invalid TOML only calls
onSuccessRun(Assertions::fail) but never asserts that the loader actually
failed; update the test to explicitly assert the result is a failure (e.g.,
assertTrue(result.isFailure()) or use an onFailure callback to capture the
exception) and optionally assert the exception type or message to ensure invalid
TOML produces a failure; keep the onSuccessRun(Assertions::fail) safeguard so a
success still fails the test.
| @Test | ||
| void loadFromString_appliesCliOverrides() { | ||
| var toml = """ | ||
| [cluster] | ||
| environment = "docker" | ||
| nodes = 3 | ||
|
|
||
| [node] | ||
| heap = "256m" | ||
| """; | ||
|
|
||
| // This tests the override mechanism through loadWithOverrides | ||
| // For now we test the direct loading | ||
| ConfigLoader.loadFromString(toml) | ||
| .onFailure(cause -> Assertions.fail(cause.message())) | ||
| .onSuccess(config -> { | ||
| assertThat(config.cluster().nodes()).isEqualTo(3); | ||
| assertThat(config.node().heap()).isEqualTo("256m"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Test name is misleading - doesn't test CLI overrides.
The test loadFromString_appliesCliOverrides doesn't actually test CLI overrides via loadWithOverrides(). The comment acknowledges this, but the test name creates confusion. Either rename to match what it tests or implement actual override testing.
🔎 Proposed fix - rename test
@Test
- void loadFromString_appliesCliOverrides() {
+ void loadFromString_parsesNodeAndHeapConfig() {
var toml = """
[cluster]
environment = "docker"
nodes = 3
[node]
heap = "256m"
""";
- // This tests the override mechanism through loadWithOverrides
- // For now we test the direct loading
ConfigLoader.loadFromString(toml)
.onFailure(cause -> Assertions.fail(cause.message()))
.onSuccess(config -> {
assertThat(config.cluster().nodes()).isEqualTo(3);
assertThat(config.node().heap()).isEqualTo("256m");
});
}🤖 Prompt for AI Agents
In
aether-config/src/test/java/org/pragmatica/aether/config/ConfigLoaderTest.java
around lines 211 to 230, the test method is named
loadFromString_appliesCliOverrides but does not exercise CLI overrides (it calls
ConfigLoader.loadFromString); rename the test to reflect actual behavior (e.g.,
loadFromString_parsesConfigWithoutOverrides or loadFromString_parsesConfig) and
update any inline comment to remove the misleading “override” phrasing;
alternatively, if you prefer to test overrides, replace the call with the proper
loadWithOverrides invocation and assert that overrides take effect—choose one
approach and make the method name, comment, and implementation consistent.
| if (options.containsKey("tls")) { | ||
| builder.tls(true); | ||
| } | ||
| if (options.containsKey("no-tls")) { | ||
| builder.tls(false); | ||
| } |
There was a problem hiding this comment.
Conflicting --tls and --no-tls flags handled silently.
If both flags are provided, --no-tls wins due to order in extractOverrides(). Consider detecting the conflict and reporting an error to avoid user confusion.
🤖 Prompt for AI Agents
In aether-setup/src/main/java/org/pragmatica/aether/setup/AetherUp.java around
lines 78 to 83, the code silently allows both --tls and --no-tls to be set and
ends up with --no-tls winning; update the logic to detect the conflict and fail
fast: check if both options.containsKey("tls") and options.containsKey("no-tls")
are true and, if so, log/throw a clear error (or print to stderr and exit with
non-zero status) indicating the conflicting flags and instructing the user to
provide only one of them; otherwise keep the existing single-flag handling that
sets builder.tls(true) or builder.tls(false).
| /** | ||
| * Generates Docker Compose deployment for Aether cluster. | ||
| * | ||
| * <p>Generates: | ||
| * <ul> | ||
| * <li>docker-compose.yml - Service definitions for all nodes</li> | ||
| * <li>.env - Environment variables</li> | ||
| * <li>aether.toml - Resolved configuration for reference</li> | ||
| * <li>start.sh - Cluster start script</li> | ||
| * <li>stop.sh - Cluster stop script</li> | ||
| * <li>status.sh - Cluster status script</li> | ||
| * </ul> | ||
| */ |
There was a problem hiding this comment.
Documentation mentions aether.toml but it's not generated.
The Javadoc lists aether.toml - Resolved configuration for reference but the generate() method doesn't create this file.
🔎 Remove from documentation
/**
* Generates Docker Compose deployment for Aether cluster.
*
* <p>Generates:
* <ul>
* <li>docker-compose.yml - Service definitions for all nodes</li>
* <li>.env - Environment variables</li>
- * <li>aether.toml - Resolved configuration for reference</li>
* <li>start.sh - Cluster start script</li>
* <li>stop.sh - Cluster stop script</li>
* <li>status.sh - Cluster status script</li>
* </ul>
*/📝 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.
| /** | |
| * Generates Docker Compose deployment for Aether cluster. | |
| * | |
| * <p>Generates: | |
| * <ul> | |
| * <li>docker-compose.yml - Service definitions for all nodes</li> | |
| * <li>.env - Environment variables</li> | |
| * <li>aether.toml - Resolved configuration for reference</li> | |
| * <li>start.sh - Cluster start script</li> | |
| * <li>stop.sh - Cluster stop script</li> | |
| * <li>status.sh - Cluster status script</li> | |
| * </ul> | |
| */ | |
| /** | |
| * Generates Docker Compose deployment for Aether cluster. | |
| * | |
| * <p>Generates: | |
| * <ul> | |
| * <li>docker-compose.yml - Service definitions for all nodes</li> | |
| * <li>.env - Environment variables</li> | |
| * <li>start.sh - Cluster start script</li> | |
| * <li>stop.sh - Cluster stop script</li> | |
| * <li>status.sh - Cluster status script</li> | |
| * </ul> | |
| */ |
| private String generateStatusScript() { | ||
| return """ | ||
| #!/bin/bash | ||
|
|
||
| echo "=== Aether Cluster Status ===" | ||
| echo "" | ||
| docker compose ps | ||
| echo "" | ||
| echo "=== Node Health ===" | ||
| for port in $(seq $MANAGEMENT_PORT $((MANAGEMENT_PORT + NODES - 1))); do | ||
| echo -n "Node on port $port: " | ||
| curl -s http://localhost:$port/health 2>/dev/null || echo "unreachable" | ||
| done | ||
| """; |
There was a problem hiding this comment.
Status script uses undefined shell variables.
The script references $MANAGEMENT_PORT and $NODES but doesn't source the .env file. These variables won't be available when the script runs, causing the health check loop to fail silently.
🔎 Source the .env file
private String generateStatusScript() {
return """
#!/bin/bash
+ SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+ source "$SCRIPT_DIR/.env"
+
echo "=== Aether Cluster Status ==="
echo ""
docker compose ps
echo ""
echo "=== Node Health ==="
for port in $(seq $MANAGEMENT_PORT $((MANAGEMENT_PORT + NODES - 1))); do
echo -n "Node on port $port: "
curl -s http://localhost:$port/health 2>/dev/null || echo "unreachable"
done
""";
}🤖 Prompt for AI Agents
In
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/DockerGenerator.java
around lines 262 to 275, the generated status script references shell variables
$MANAGEMENT_PORT and $NODES but never sources the .env file; update the script
to source the project .env (e.g., load .env from the compose project directory
if present) before using those variables and add safe fallbacks (or an early
error message) if the variables remain unset so the health-check loop doesn't
fail silently.
| private String generatePdb(AetherConfig config) { | ||
| // Allow at most one node to be unavailable (maintain quorum) | ||
| var maxUnavailable = 1; | ||
| return String.format(""" | ||
| apiVersion: policy/v1 | ||
| kind: PodDisruptionBudget | ||
| metadata: | ||
| name: aether-pdb | ||
| namespace: %s | ||
| spec: | ||
| maxUnavailable: %d | ||
| selector: | ||
| matchLabels: | ||
| app: aether-node | ||
| """, | ||
| config.kubernetes() | ||
| .namespace(), | ||
| maxUnavailable); | ||
| } |
There was a problem hiding this comment.
PDB maxUnavailable=1 may not preserve quorum for all cluster sizes.
For a 3-node cluster requiring majority quorum (2 nodes), allowing 1 unavailable is correct. However, for larger clusters, this hardcoded value may be overly restrictive or insufficient. Consider computing based on nodes/2 or making it configurable.
🤖 Prompt for AI Agents
In
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/KubernetesGenerator.java
around lines 296 to 314, replace the hardcoded maxUnavailable = 1 with a
calculation (or config value) derived from cluster size: compute an appropriate
maxUnavailable from the configured node count (e.g. int maxUnavailable =
Math.max(1, config.nodeCount() / 2); or better yet allow an override via
config.kubernetes().podDisruptionBudgetMaxUnavailable() and fall back to the
computed value), then use that variable in the String.format for the PDB; ensure
the computed value is clamped to a valid range (>=1 and < nodeCount) and update
any config interfaces if necessary.
| config.node() | ||
| .gc() | ||
| .toUpperCase() | ||
| .equals("ZGC") | ||
| ? "ZGC" | ||
| : "G1GC", |
There was a problem hiding this comment.
GC type logic is overly simplistic.
The GC selection only checks for "ZGC" (case-insensitive) and defaults everything else to "G1GC". This doesn't handle other valid GC types (e.g., "G1", "Serial", "Shenandoah") or invalid values. Consider validating the GC value or supporting more GC types explicitly.
🔎 Proposed improvement
-config.node()
- .gc()
- .toUpperCase()
- .equals("ZGC")
-? "ZGC"
-: "G1GC",
+switch (config.node().gc().toUpperCase()) {
+ case "ZGC" -> "ZGC";
+ case "G1", "G1GC" -> "G1GC";
+ case "SERIAL" -> "SerialGC";
+ case "PARALLEL" -> "ParallelGC";
+ default -> "G1GC"; // safe default
+}📝 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.
| config.node() | |
| .gc() | |
| .toUpperCase() | |
| .equals("ZGC") | |
| ? "ZGC" | |
| : "G1GC", | |
| switch (config.node().gc().toUpperCase()) { | |
| case "ZGC" -> "ZGC"; | |
| case "G1", "G1GC" -> "G1GC"; | |
| case "SERIAL" -> "SerialGC"; | |
| case "PARALLEL" -> "ParallelGC"; | |
| default -> "G1GC"; | |
| } |
🤖 Prompt for AI Agents
In
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/LocalGenerator.java
around lines 169 to 174, the GC selection only checks for "ZGC" and otherwise
forces "G1GC"; update this to validate and map supported GC names (handle common
variants like "ZGC", "G1", "G1GC", "Serial", "Shenandoah", etc.), normalize the
input (case-insensitive, trim), return the canonical JVM flag name for each
supported type, and for unknown/invalid values either fall back to a safe
default (e.g., "G1GC") while emitting a warning or throw a clear configuration
error—implement the mapping/validation logic instead of the current simplistic
ternary operator so unsupported values are handled explicitly.
| echo "" | ||
|
|
||
| for i in $(seq 0 %d); do | ||
| PORT=$((${%d} + i)) |
There was a problem hiding this comment.
Fix malformed shell variable reference.
Line 224 contains ${%d} which will be formatted as ${8080} (or similar number), creating invalid shell syntax. The dollar sign and braces should be removed.
🔎 Proposed fix
- PORT=$((${%d} + i))
+ PORT=$((%d + i))This generates valid shell arithmetic: PORT=$((8080 + i)).
📝 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.
| PORT=$((${%d} + i)) | |
| PORT=$((%d + i)) |
🤖 Prompt for AI Agents
In
aether-setup/src/main/java/org/pragmatica/aether/setup/generators/LocalGenerator.java
around line 224, the shell arithmetic uses `${%d}` which produces invalid syntax
like `${8080}`; change the template so the formatted integer is inserted without
`$` and braces (e.g. `PORT=$((8080 + i))`) — i.e., remove the dollar sign and
braces from the formatted placeholder so the generated script contains valid
shell arithmetic with the raw number.
- Add --config option to Main.java for aether.toml loading - Add --config option to AetherCli for connection info from config - Make defaultSliceConfig() public for external access - Add aether-config dependency to node and cli modules - Maintain backward compatibility with existing CLI options 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/main/java/org/pragmatica/aether/cli/AetherCli.java (1)
77-90: Pre-parsing args before picocli can cause inconsistencies.Manually parsing arguments before picocli's standard parsing flow can lead to:
- Inconsistencies between manual parsing and picocli's parsing
- Maintenance burden (two parsing implementations)
- Potential issues if picocli validation fails after manual parsing succeeds
Consider using picocli's
@Mixinor initialization callbacks to access connection options, or leverage picocli'sparseArgs()with exception handling.
🧹 Nitpick comments (2)
node/src/main/java/org/pragmatica/aether/node/AetherNodeConfig.java (1)
41-43: Visibility change approved; consider adding Javadoc.The visibility change aligns with the PR objectives to enable external configuration usage. However, as this is now part of the public API, it would benefit from Javadoc explaining when and why external code should use this method versus creating custom SliceActionConfig instances.
📝 Suggested Javadoc
+ /** + * Returns the default slice action configuration for Aether nodes. + * Uses Fury serialization provider for efficient serialization. + * External modules can use this to obtain the standard default configuration. + * + * @return Default SliceActionConfig with Fury serialization + */ public static SliceActionConfig defaultSliceConfig() { return SliceActionConfig.defaultConfiguration(furySerializerFactoryProvider()); }node/src/main/java/org/pragmatica/aether/Main.java (1)
117-146: Consider consistent formatting for config fallback blocks.Both methods correctly implement the precedence chain (CLI > env > config > default), but the indentation in the config fallback blocks (lines 124-130, 139-145) is inconsistent with typical Java formatting. Consider aligning the brace placement for better readability.
🔎 Formatting suggestion
.orElseGet(() -> { - if (aetherConfig != null) { - return aetherConfig.cluster() - .ports() - .cluster(); - } - return DEFAULT_CLUSTER_PORT; - }); + if (aetherConfig != null) { + return aetherConfig.cluster() + .ports() + .cluster(); + } + return DEFAULT_CLUSTER_PORT; +});
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cli/pom.xmlcli/src/main/java/org/pragmatica/aether/cli/AetherCli.javanode/pom.xmlnode/src/main/java/org/pragmatica/aether/Main.javanode/src/main/java/org/pragmatica/aether/node/AetherNodeConfig.java
🚧 Files skipped from review as they are similar to previous changes (2)
- node/pom.xml
- cli/pom.xml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.java: Return exactly one of: T (synchronous infallible), Option (synchronous optional), Result (synchronous fallible), or Promise (asynchronous fallible). Never use Promise<Result>
Use factory naming convention: TypeName.typeName(...) with lowercase-first method names for creating instances
Never throw business exceptions. All business logic failures must flow through Result or Promise as typed Cause objects
Use sealed interfaces for domain-specific error hierarchies implementing Cause interface
Prefer record-based implementations for use cases, adapters, and data carriers
Use method references over lambdas when the lambda only calls a single method
Ensure all domain objects are immutable records. Use List.of(), Map.of(), Set.of() for collections
Apply timeouts as close to actual I/O operations as possible, not at the end of promise chains
Use Promise.lift() and Result.lift() for wrapping exception-prone operations in error handling
Use adapter leaves for all I/O operations (database, HTTP, file system) to ensure framework independence
Follow single pattern per function rule: Leaf (single operation), Sequencer (linear chain), Fork-Join (parallel), Condition (branching), Iteration (collection processing)
Use .map() and .flatMap() for Result and Promise transformations instead of extracting and re-wrapping values
Package structure must be organized by feature/use case, not by layer (e.g., /userregistration, not /services or /dao)
All Promise-based operations must include timeout configuration relative to their specific operation's expected duration
Keep lambdas simple. Extract complex logic to separate methods to maintain code clarity
Files:
node/src/main/java/org/pragmatica/aether/node/AetherNodeConfig.javacli/src/main/java/org/pragmatica/aether/cli/AetherCli.javanode/src/main/java/org/pragmatica/aether/Main.java
**/AetherNodeConfig.java
📄 CodeRabbit inference engine (CLAUDE.md)
AetherNodeConfig must provide all runtime configuration for node (timeouts, cluster addresses, metrics intervals, etc.)
Files:
node/src/main/java/org/pragmatica/aether/node/AetherNodeConfig.java
**/cli/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
AetherCli must support both interactive REPL mode and batch command mode for cluster management
Files:
cli/src/main/java/org/pragmatica/aether/cli/AetherCli.java
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNodeConfig.java : AetherNodeConfig must provide all runtime configuration for node (timeouts, cluster addresses, metrics intervals, etc.)
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNode.java : AetherNode assembly must wire all components (SliceStore, DeploymentManagers, MetricsCollector, ControlLoop, ManagementServer) together
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNodeConfig.java : AetherNodeConfig must provide all runtime configuration for node (timeouts, cluster addresses, metrics intervals, etc.)
Applied to files:
node/src/main/java/org/pragmatica/aether/node/AetherNodeConfig.javacli/src/main/java/org/pragmatica/aether/cli/AetherCli.javanode/src/main/java/org/pragmatica/aether/Main.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNode.java : AetherNode assembly must wire all components (SliceStore, DeploymentManagers, MetricsCollector, ControlLoop, ManagementServer) together
Applied to files:
node/src/main/java/org/pragmatica/aether/node/AetherNodeConfig.javanode/src/main/java/org/pragmatica/aether/Main.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/cli/*.java : AetherCli must support both interactive REPL mode and batch command mode for cluster management
Applied to files:
cli/src/main/java/org/pragmatica/aether/cli/AetherCli.javanode/src/main/java/org/pragmatica/aether/Main.java
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/forge/*Server.java : ForgeServer (Aether Forge) must provide standalone simulator CLI with visual dashboard for load/chaos testing
Applied to files:
cli/src/main/java/org/pragmatica/aether/cli/AetherCli.java
🧬 Code graph analysis (2)
cli/src/main/java/org/pragmatica/aether/cli/AetherCli.java (1)
aether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.java (1)
ConfigLoader(22-180)
node/src/main/java/org/pragmatica/aether/Main.java (1)
aether-config/src/main/java/org/pragmatica/aether/config/ConfigLoader.java (1)
ConfigLoader(22-180)
🔇 Additional comments (10)
cli/src/main/java/org/pragmatica/aether/cli/AetherCli.java (5)
3-4: LGTM!The config-related imports and default address constant properly support the new config-driven connection resolution feature.
Also applies to: 65-65
71-73: LGTM!The
--configoption properly follows picocli conventions and enables config file specification.
157-157: LGTM!Version string updated consistently with the CLI version annotation.
47-47: LGTM!Version bump aligns with the 0.7.1 release objectives.
68-68: LGTM!The description text is clear and accurate.
node/src/main/java/org/pragmatica/aether/Main.java (5)
3-4: LGTM! Documentation and imports are clear.The new imports support config-driven startup, and the javadoc accurately documents the config file option and precedence order (CLI overrides config file defaults).
Also applies to: 10-10, 13-13, 28-36
59-79: LGTM! Parameter parsing and node initialization.The code correctly parses all parameters with proper precedence (CLI > env > config > default), logs the configuration details for visibility, and passes them to the node configuration. The use of
AetherNodeConfig.defaultSliceConfig()aligns with the PR objective of making it public for external use.
187-203: LGTM! Peer string parsing logic is correct.The method correctly parses peer addresses from a string, uses
flatMapto filter invalid entries, and ensures the self node is included in the final list. The ArrayList approach for adding self is clear and correct.
231-234: LGTM! Clean environment variable helper.The
findEnvhelper correctly wrapsSystem.getenv()in an Optional and filters blank values, providing a consistent pattern withfindArg().
46-58: The current code follows the established pattern in the codebase (seeAetherUp.javaline 64, which uses the identicalresult.fold(cause -> null, c -> c)pattern). Additionally, the suggested refactoring references.toOptional(), which does not exist in the framework API and would cause a compilation error. The error handling approach—logging and gracefully continuing with defaults—is intentional and consistent with other configuration loading patterns in the project.Likely an incorrect or invalid review comment.
| private static boolean isReplMode(String[] args) { | ||
| // REPL if no args, or only connection-related options | ||
| if (args.length == 0) { | ||
| return true; | ||
| } | ||
| for (String arg : args) { | ||
| // Skip known non-subcommand options | ||
| if (arg.startsWith("-c") || arg.startsWith("--connect") || | ||
| arg.startsWith("--config") || arg.equals("-h") || arg.equals("--help") || | ||
| arg.equals("-V") || arg.equals("--version")) { | ||
| continue; | ||
| } | ||
| // Skip option values (next arg after option) | ||
| if (!arg.startsWith("-")) { | ||
| // Could be a subcommand or option value | ||
| // Check if previous arg was an option expecting value | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix broken REPL mode detection logic.
The method incorrectly identifies aether-cli --connect localhost:8080 as batch mode instead of REPL mode. When it encounters "localhost:8080" (which doesn't start with "-"), it immediately returns false without verifying whether the previous argument was an option expecting a value.
Example failure:
aether-cli --connect localhost:8080
- Processes "--connect" → continues
- Processes "localhost:8080" → returns false (incorrect)
- Result: Batch mode instead of REPL mode
🔎 Proposed fix
private static boolean isReplMode(String[] args) {
- // REPL if no args, or only connection-related options
if (args.length == 0) {
return true;
}
+ // Track which options expect values
+ boolean expectingValue = false;
for (String arg : args) {
- // Skip known non-subcommand options
- if (arg.startsWith("-c") || arg.startsWith("--connect") ||
- arg.startsWith("--config") || arg.equals("-h") || arg.equals("--help") ||
- arg.equals("-V") || arg.equals("--version")) {
+ if (expectingValue) {
+ // This arg is a value for the previous option
+ expectingValue = false;
continue;
}
- // Skip option values (next arg after option)
- if (!arg.startsWith("-")) {
- // Could be a subcommand or option value
- // Check if previous arg was an option expecting value
+ if (arg.equals("-c") || arg.equals("--connect") ||
+ arg.equals("--config") || arg.equals("-h") ||
+ arg.equals("--help") || arg.equals("-V") ||
+ arg.equals("--version")) {
+ // Options that expect values
+ if (arg.equals("-c") || arg.equals("--connect") || arg.equals("--config")) {
+ expectingValue = true;
+ }
+ continue;
+ }
+ if (arg.startsWith("--connect=") || arg.startsWith("--config=")) {
+ // Inline option values
+ continue;
+ }
+ if (!arg.startsWith("-")) {
+ // Found a non-option arg that's not an option value - must be a subcommand
return false;
}
}
return true;
}| private void resolveConnection(String[] args) { | ||
| // Parse args manually to get --connect and --config | ||
| String connectArg = null; | ||
| Path configArg = null; | ||
| for (int i = 0; i < args.length; i++ ) { | ||
| if ((args[i].equals("-c") || args[i].equals("--connect")) && i + 1 < args.length) { | ||
| connectArg = args[i + 1]; | ||
| }else if (args[i].startsWith("--connect=")) { | ||
| connectArg = args[i].substring("--connect=".length()); | ||
| }else if (args[i].equals("--config") && i + 1 < args.length) { | ||
| configArg = Path.of(args[i + 1]); | ||
| }else if (args[i].startsWith("--config=")) { | ||
| configArg = Path.of(args[i].substring("--config=".length())); | ||
| } | ||
| } | ||
| // Priority: --connect > config file > default | ||
| if (connectArg != null) { | ||
| nodeAddress = connectArg; | ||
| }else if (configArg != null && Files.exists(configArg)) { | ||
| ConfigLoader.load(configArg) | ||
| .onSuccess(config -> { | ||
| var port = config.cluster() | ||
| .ports() | ||
| .management(); | ||
| nodeAddress = "localhost:" + port; | ||
| }) | ||
| .onFailure(cause -> { | ||
| System.err.println("Warning: Failed to load config: " + cause.message()); | ||
| nodeAddress = DEFAULT_ADDRESS; | ||
| }); | ||
| configPath = configArg; | ||
| }else { | ||
| nodeAddress = DEFAULT_ADDRESS; | ||
| } | ||
| } |
There was a problem hiding this comment.
Manual argument parsing is brittle and duplicates picocli.
This method manually parses --connect and --config options, duplicating picocli's parsing logic. This approach:
- Doesn't handle edge cases (quoted values, escaping, etc.)
- Requires maintenance of two parsing implementations
- Is more error-prone than using picocli's robust parser
Consider using picocli's parsing capabilities (CommandLine.parseArgs() or early initialization hooks) instead of manual string manipulation.
🤖 Prompt for AI Agents
In cli/src/main/java/org/pragmatica/aether/cli/AetherCli.java around lines 114
to 148, the resolveConnection method manually parses args which duplicates
picocli and is brittle; replace the manual loop by using picocli to get the
parsed values (for example use CommandLine.parseArgs(...) or inject @Option
fields and ensure CommandLine has parsed them before calling resolveConnection),
then apply the intended precedence (CLI --connect if present, else --config file
if provided and exists, else DEFAULT_ADDRESS). When using the config file ensure
ConfigLoader is invoked synchronously or await its completion so nodeAddress is
set deterministically, set configPath only when a config file is used, and
handle load failures by logging the error and falling back to DEFAULT_ADDRESS.
| private NodeId parseNodeId(AetherConfig aetherConfig) { | ||
| // Command-line takes precedence | ||
| return findArg("--node-id=") | ||
| .map(NodeId::nodeId) | ||
| .or(() -> findEnv("NODE_ID") | ||
| .map(NodeId::nodeId)) | ||
| .orElseGet(NodeId::randomNodeId); | ||
| } |
There was a problem hiding this comment.
Unused parameter in method signature.
The aetherConfig parameter is declared but never used in the method body. If the config is intended to provide a node ID in the future, consider adding a TODO comment. Otherwise, the parameter can be removed to simplify the signature.
🔎 Suggested fix
If config should provide node ID (most likely intent based on other parse methods):
private NodeId parseNodeId(AetherConfig aetherConfig) {
// Command-line takes precedence
return findArg("--node-id=")
.map(NodeId::nodeId)
.or(() -> findEnv("NODE_ID")
.map(NodeId::nodeId))
+ .or(() -> aetherConfig != null
+ ? java.util.Optional.of(NodeId.nodeId(aetherConfig.node().id()))
+ : java.util.Optional.empty())
.orElseGet(NodeId::randomNodeId);
}Or if node ID should always be explicit/random, remove the parameter:
-private NodeId parseNodeId(AetherConfig aetherConfig) {
+private NodeId parseNodeId() {📝 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 NodeId parseNodeId(AetherConfig aetherConfig) { | |
| // Command-line takes precedence | |
| return findArg("--node-id=") | |
| .map(NodeId::nodeId) | |
| .or(() -> findEnv("NODE_ID") | |
| .map(NodeId::nodeId)) | |
| .orElseGet(NodeId::randomNodeId); | |
| } | |
| private NodeId parseNodeId() { | |
| // Command-line takes precedence | |
| return findArg("--node-id=") | |
| .map(NodeId::nodeId) | |
| .or(() -> findEnv("NODE_ID") | |
| .map(NodeId::nodeId)) | |
| .orElseGet(NodeId::randomNodeId); | |
| } |
🤖 Prompt for AI Agents
In node/src/main/java/org/pragmatica/aether/Main.java around lines 108-115 the
method parameter aetherConfig is declared but never used; remove the unused
parameter from the parseNodeId method signature and update all call sites to
call parseNodeId() with no arguments (or, if the original intent was to read a
node id from the config, instead read the node id from aetherConfig inside the
method or add a TODO and use aetherConfig.getNodeId() accordingly); ensure
compiler errors are fixed by updating imports/usages and run a quick build to
verify.
| if (aetherConfig != null) { | ||
| var nodes = aetherConfig.cluster() | ||
| .nodes(); | ||
| var clusterPort = aetherConfig.cluster() | ||
| .ports() | ||
| .cluster(); | ||
| // Generate peer addresses for all nodes | ||
| return IntStream.range(0, nodes) | ||
| .mapToObj(i -> { | ||
| var host = aetherConfig.environment() == org.pragmatica.aether.config.Environment.DOCKER | ||
| ? "aether-node-" + i | ||
| : "localhost"; | ||
| // Local deployment | ||
| var port = clusterPort + (aetherConfig.environment() == org.pragmatica.aether.config.Environment.LOCAL | ||
| ? i | ||
| : 0); | ||
| var nodeIdStr = "node-" + i; | ||
| return nodeInfo(NodeId.nodeId(nodeIdStr), | ||
| nodeAddress(host, port)); | ||
| }) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract peer generation logic to config layer.
The peer generation logic (lines 168-181) is complex, environment-specific, and belongs in the config layer rather than the Main entry point. This logic duplicates concerns that should be handled by AetherConfig or a dedicated peer resolution component, making it harder to test and maintain.
Consider moving this logic to a method in ConfigLoader or AetherConfig:
// In ConfigLoader or AetherConfig
public List<NodeInfo> generatePeerList(int clusterPort) {
return IntStream.range(0, nodes)
.mapToObj(i -> {
var host = environment == Environment.DOCKER
? "aether-node-" + i
: "localhost";
var port = clusterPort + (environment == Environment.LOCAL ? i : 0);
var nodeIdStr = "node-" + i;
return nodeInfo(NodeId.nodeId(nodeIdStr), nodeAddress(host, port));
})
.toList();
}Then simplify Main:
if (aetherConfig != null) {
return aetherConfig.generatePeerList(aetherConfig.cluster().ports().cluster());
}As per coding guidelines: "Package structure must be organized by feature/use case" - peer resolution from config is a config-layer concern, not a Main concern.
🤖 Prompt for AI Agents
In node/src/main/java/org/pragmatica/aether/Main.java around lines 161 to 182,
extract the peer generation logic into the configuration layer: add a method
(e.g. generatePeerList(int clusterPort) or similar) on AetherConfig or
ConfigLoader that captures nodes, environment and ports and returns the
List<NodeInfo> using the same host/port/nodeId calculation; then replace the
block in Main with a single call to that method passing
aetherConfig.cluster().ports().cluster(); ensure the new method is covered by
unit tests and uses the config's fields (nodes, environment, ports) rather than
Main-local logic so environment-specific rules are centralized.
- Fix scale command syntax (requires -n flag) - Add --config option documentation for CLI and node - Document invocation-metrics, controller, alerts, thresholds commands - Add environment variables for aether-node - Update demos.md JAR references to use shaded JAR name 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/guide/cli-reference.md (1)
394-410: Missing environment variable for config file path.The new Environment Variables section (lines 402–410) documents NODE_ID, CLUSTER_PORT, MANAGEMENT_PORT, and CLUSTER_PEERS, but does not include an environment variable for the config file path itself (e.g.,
AETHER_CONFIG). Given the PR's emphasis on config-driven workflows, adding this would improve consistency with the--configCLI option. Consider whether anAETHER_CONFIGenvironment variable should be documented and supported.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mddocs/demos.mddocs/guide/cli-reference.md
✅ Files skipped from review due to trivial changes (1)
- docs/demos.md
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNodeConfig.java : AetherNodeConfig must provide all runtime configuration for node (timeouts, cluster addresses, metrics intervals, etc.)
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/cli/*.java : AetherCli must support both interactive REPL mode and batch command mode for cluster management
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNode.java : AetherNode assembly must wire all components (SliceStore, DeploymentManagers, MetricsCollector, ControlLoop, ManagementServer) together
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/cli/*.java : AetherCli must support both interactive REPL mode and batch command mode for cluster management
Applied to files:
docs/guide/cli-reference.md
📚 Learning: 2026-01-02T22:43:33.374Z
Learnt from: CR
Repo: siy/aether PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T22:43:33.374Z
Learning: Applies to **/AetherNodeConfig.java : AetherNodeConfig must provide all runtime configuration for node (timeouts, cluster addresses, metrics intervals, etc.)
Applied to files:
docs/guide/cli-reference.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker-build
🔇 Additional comments (8)
README.md (1)
113-113: Command syntax update aligns with reference documentation.The change from positional to flag-based syntax (
-n 3) matches the CLI reference documentation and is consistent across the codebase.docs/guide/cli-reference.md (7)
153-156: Scale command syntax update is clear and consistent.The documented change to
-n <instances>flag format aligns with the README.md update and provides a clearer, more modern CLI interface.
272-287: New invocation-metrics command documentation is comprehensive.The section covers listing metrics, viewing slow invocations, and configuring threshold strategies with clear examples. The fixed and adaptive threshold strategy examples are particularly helpful.
289-305: New controller command documentation is well-structured.The section covers configuration, status, and evaluation with practical examples. The threshold configuration example (
--cpu-up 0.8 --cpu-down 0.3) clearly shows the option format.
307-323: New alerts command documentation is clear and complete.The section covers all expected operations (list, active, history, clear) with straightforward command syntax.
325-338: New thresholds command documentation is intuitive.The section clearly documents threshold management with good examples. The warning/critical level syntax (
-w 0.7 -c 0.9) is well-demonstrated.
347-347: REPL version bump is correct.Version bumped from v0.7.0 to v0.7.1, aligning with the PR release target.
54-58: Both--config <path>and--config=<path>formats are supported by the CLI. The picocli library (used for command parsing) natively accepts both space-separated and equals-separated option parameters. The documentation correctly reflects this by showing both formats in different sections—no standardization is needed.Likely an incorrect or invalid review comment.
1 participant
Summary
Release 0.7.1 focuses on production readiness with comprehensive E2E testing and documentation consolidation.
E2E Test Suite Expansion
Added 53 new E2E tests across 7 new test classes (80 total):
Infrastructure Enhancements
AetherNodeContainer enhanced with 20+ new API methods:
getPrometheusMetrics(),getInvocationMetrics(),getSlowInvocations()getThresholds(),setThreshold(),deleteThreshold()getActiveAlerts(),getAlertHistory(),clearAlerts()getControllerConfig(),setControllerConfig(),triggerControllerEvaluation()startRollingUpdate(),getRollingUpdates(),setRollingUpdateRouting()invokeSlice(),invokeGet(),invokePost(),getRoutes()Documentation Updates
Test plan
mvn test)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.