Conversation
Removes package-based loading from SliceClassLoader and introduces explicit dependency declaration via [shared] and [slices] sections. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add demo/ module with DemoCluster, LoadGenerator, DemoMetrics - Implement REST API for cluster operations (kill, crash, add node, rolling restart) - Create dark-themed dashboard with D3.js topology and Chart.js metrics - Support dynamic load scaling (1K-10K req/sec) - Include Docker/docker-compose for containerized deployment - Dashboard shows real-time success rate, latency, and event timeline 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds an HTTP server and router with Netty integration, shared-library-aware slice classloading and dependency resolution, a demo-order multi-module (services and use cases), a resilience demo app and dashboard, many docs, tests, and moves routing types to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NettyServer
participant RouteMatcher
participant BindingResolver
participant SliceDispatcher
participant SliceInvoker
participant Slice
participant ResponseWriter
Client->>NettyServer: HTTP request (method, path, headers, body)
NettyServer->>RouteMatcher: match(method, path)
RouteMatcher-->>NettyServer: MatchResult(route, pathVars)
par Resolve bindings
NettyServer->>BindingResolver: resolve(route.bindings, RequestContext)
BindingResolver-->>NettyServer: resolvedParams
end
par Dispatch
NettyServer->>SliceDispatcher: dispatch(route, resolvedParams)
SliceDispatcher->>SliceInvoker: invokeLocal/remote(artifact, method, request)
SliceInvoker->>Slice: execute(method, request)
Slice-->>SliceInvoker: Promise(response)
SliceInvoker-->>SliceDispatcher: Promise(response)
end
NettyServer->>ResponseWriter: writeSuccess(ctx, response)
ResponseWriter->>Client: HTTP 200 + JSON
sequenceDiagram
participant Browser
participant DemoApiHandler
participant DemoCluster
participant DemoMetrics
participant LoadGenerator
Browser->>DemoApiHandler: GET /api/status
DemoApiHandler->>DemoCluster: status()
DemoCluster-->>DemoApiHandler: ClusterStatus
DemoApiHandler->>DemoMetrics: currentMetrics()
DemoMetrics-->>DemoApiHandler: MetricsSnapshot
DemoApiHandler-->>Browser: JSON status
Browser->>DemoApiHandler: POST /api/kill-node
DemoApiHandler->>DemoCluster: killNode(nodeId)
DemoCluster-->>DemoApiHandler: Promise<Unit>
DemoApiHandler-->>Browser: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/aether-high-level-overview.md (1)
158-159: Stale last-updated date.The date shows
2025-08-09but this PR is from December 2025. Consider updating to reflect the current release date.🔎 Proposed fix
-*Last Updated: 2025-08-09* +*Last Updated: 2025-12-24*
🟡 Minor comments (16)
slice/src/main/java/org/pragmatica/aether/slice/dependency/DependencyFile.java-151-161 (1)
151-161: Resource stream not closed on parse failure.If
parse(resource)fails, theInputStreamfromgetResourceAsStreamis never closed. Consider wrapping with try-with-resources or ensuring the stream is closed in all paths.🔎 Proposed fix
public static Result<DependencyFile> load(String sliceClassName, ClassLoader classLoader) { var resourcePath = "META-INF/dependencies/" + sliceClassName; - var resource = classLoader.getResourceAsStream(resourcePath); - - if (resource == null) { - // No dependencies file means no dependencies - this is valid - return Result.success(new DependencyFile(List.of(), List.of())); + try (var resource = classLoader.getResourceAsStream(resourcePath)) { + if (resource == null) { + // No dependencies file means no dependencies - this is valid + return Result.success(new DependencyFile(List.of(), List.of())); + } + return parse(resource); + } catch (IOException e) { + return Causes.fromThrowable(e).result(); } - - return parse(resource); }slice/src/main/java/org/pragmatica/aether/slice/dependency/DependencyResolver.java-134-134 (1)
134-134: Temporary SliceClassLoader is never closed.
createTempLoaderat line 134 creates aSliceClassLoaderfor reading the dependency file, but it's never closed. This can lead to resource leaks (JAR file handles).The loader should be closed after
DependencyFile.loadcompletes:🔎 Proposed fix
- // Load dependency file to get shared and slice dependencies - return DependencyFile.load(manifest.sliceClassName(), createTempLoader(location.url(), sharedLibraryLoader)) - .async() + // Load dependency file to get shared and slice dependencies + var tempLoader = createTempLoader(location.url(), sharedLibraryLoader); + return DependencyFile.load(manifest.sliceClassName(), tempLoader) + .onSuccessRun(() -> closeSilently(tempLoader)) + .onFailureRun(() -> closeSilently(tempLoader)) + .async()Add a helper method:
private static void closeSilently(SliceClassLoader loader) { try { loader.close(); } catch (IOException e) { log.warn("Failed to close temp classloader: {}", e.getMessage()); } }Committable suggestion skipped: line range outside the PR's diff.
node/src/main/java/org/pragmatica/aether/http/BindingResolver.java-92-104 (1)
92-104: Release the ByteBuf after deserialization.The
Unpooled.wrappedBuffer(body)creates a ByteBuf that should be explicitly released after use. TheDeserializer.decode()method establishes the expected pattern: wrap bytes in a buffer, callread(), and release in a finally block. Wrap thedeserializer.read(buf)call in a try-finally to ensure the buffer is released, even if an exception occurs during deserialization.demo-order/inventory-service/src/main/java/org/pragmatica/aether/demo/order/inventory/CheckStockRequest.java-3-3 (1)
3-3: Add validation to prevent invalid quantity values.The
quantityparameter accepts negative or zero values, which are invalid for stock check requests. Consider adding a compact constructor to validate thatproductIdis non-null/non-empty andquantityis positive.🔎 Proposed validation enhancement
-public record CheckStockRequest(String productId, int quantity) {} +public record CheckStockRequest(String productId, int quantity) { + public CheckStockRequest { + if (productId == null || productId.isBlank()) { + throw new IllegalArgumentException("productId must not be null or blank"); + } + if (quantity <= 0) { + throw new IllegalArgumentException("quantity must be positive"); + } + } +}node/src/main/java/org/pragmatica/aether/http/HttpMethod.java-15-26 (1)
15-26: Fix locale-dependent string comparison.The
toUpperCase()call without a Locale parameter is locale-dependent and can cause unexpected behavior in certain locales (e.g., Turkish locale where "i".toUpperCase() != "I"). For consistent, locale-independent parsing of HTTP methods, usetoUpperCase(Locale.ROOT)orLocale.ENGLISH.Details
🔎 Proposed fix+import java.util.Locale; + public static HttpMethod fromString(String method) { - return switch (method.toUpperCase()) { + return switch (method.toUpperCase(Locale.ROOT)) { case "GET" -> GET; case "POST" -> POST; case "PUT" -> PUT; case "DELETE" -> DELETE; case "PATCH" -> PATCH; case "HEAD" -> HEAD; case "OPTIONS" -> OPTIONS; default -> throw new IllegalArgumentException("Unknown HTTP method: " + method); }; }Committable suggestion skipped: line range outside the PR's diff.
http-server/src/test/java/org/pragmatica/http/server/HttpServerIntegrationTest.java-15-38 (1)
15-38: Incomplete test: no assertions after server starts.This test starts the server with
port 0but the comments indicate awareness that getting the actual port is unresolved. The test has no HTTP request or assertion, making it effectively a no-op.Either complete the test by retrieving the actual bound port from the server, or remove it to avoid confusion.
demo/src/main/java/org/pragmatica/aether/demo/StaticFileHandler.java-59-63 (1)
59-63: Directory traversal check is vulnerable to percent-encoded path bypass.The check for
".."at line 60 occurs on the raw URI before any URL decoding. Netty'sHttpRequest.uri()returns percent-encoded paths unchanged. A request like%2e%2e/etc/passwdwill pass the check, and depending on the ClassLoader implementation,getResourceAsStream()at line 89 may decode the percent-encoded bytes during resource lookup, enabling directory traversal.Decode the path before the security check:
+import java.net.URLDecoder; + // Security: prevent directory traversal +path = URLDecoder.decode(path, StandardCharsets.UTF_8); if (path.contains("..")) { sendError(ctx, FORBIDDEN, "Invalid path"); return; }Committable suggestion skipped: line range outside the PR's diff.
demo-order/place-order/src/main/java/org/pragmatica/aether/demo/order/usecase/placeorder/PlaceOrderSlice.java-101-132 (1)
101-132: Missing compensation for partial reservation failures.If some reservations succeed but others fail, the successful reservations are not released. This leaves stock in an inconsistent state.
For a demo this may be acceptable, but worth noting for production readiness.
🔎 Suggested approach
// On failure, release successful reservations before returning error if (successful.size() != results.size()) { // Release already-made reservations successful.forEach(res -> invoker.invokeAndWait(INVENTORY, MethodName.methodName("releaseStock").unwrap(), new ReleaseStockRequest(res.reservationId()), StockReleased.class)); return new PlaceOrderError.ReservationFailed( Causes.cause("Failed to reserve all items") ).promise(); }demo/src/main/java/org/pragmatica/aether/demo/DemoApiHandler.java-271-278 (1)
271-278: JSON escaping is incomplete.Missing escape sequences for backspace (
\b) and form feed (\f). Also, Unicode control characters should be escaped as\uXXXX.For a demo with controlled inputs, this is acceptable but could cause malformed JSON with edge-case strings.
demo-order/place-order/src/main/java/org/pragmatica/aether/demo/order/usecase/placeorder/PlaceOrderSlice.java-134-141 (1)
134-141: Created order is not persisted for CancelOrderSlice.The order is returned but not stored in
CancelOrderSlice.ORDERS. This means newly placed orders cannot be cancelled. Either persist here or document that cancellation only works for pre-seeded orders.🔎 Suggested fix
private PlaceOrderResponse createOrder(ValidWithReservations context) { + // Store order for cancellation support + CancelOrderSlice.storeOrderWithReservations( + context.orderId(), + OrderStatus.CONFIRMED, + context.reservations().stream().map(StockReservation::reservationId).toList() + ); + return new PlaceOrderResponse( context.orderId(), OrderStatus.CONFIRMED,Committable suggestion skipped: line range outside the PR's diff.
demo-order/cancel-order/src/main/java/org/pragmatica/aether/demo/order/usecase/cancelorder/CancelOrderSlice.java-106-129 (1)
106-129: Stock release calls may not be parallel and error wrapping is semantically incorrect.
The
stream().map().toList()pattern collects all Promises but whether they execute in parallel depends oninvokeAndWaitimplementation. Consider using explicit parallel execution if parallelism is intended.Line 122-124:
StockReleaseFailedwrapsInvalidRequest, which is semantically misleading. The inner cause should describe the actual release failure.🔎 Suggested fix for error wrapping
if (!allSuccess) { - // In production, we'd handle partial releases - return new CancelOrderError.StockReleaseFailed( - new CancelOrderError.InvalidRequest("Some reservations could not be released") - ).promise(); + return new CancelOrderError.StockReleaseFailed( + Causes.cause("Some reservations could not be released") + ).promise(); }Committable suggestion skipped: line range outside the PR's diff.
demo/src/main/java/org/pragmatica/aether/demo/DemoApiHandler.java-206-216 (1)
206-216: Race condition in event list trimming.
CopyOnWriteArrayList.removeFirst()is not atomic with thesize()check. Under concurrent access, multiple threads could attempt removal simultaneously. Additionally,removeFirst()onCopyOnWriteArrayListis O(n) as it copies the entire array.For a demo with low event frequency this is likely fine, but worth noting.
🔎 Suggested alternative using bounded queue
- private final List<DemoEvent> events = new CopyOnWriteArrayList<>(); + private final Deque<DemoEvent> events = new ConcurrentLinkedDeque<>(); ... public void addEvent(String type, String message) { var event = new DemoEvent(Instant.now().toString(), type, message); events.add(event); - - // Keep only last 100 events - while (events.size() > 100) { - events.removeFirst(); + // Trim excess - approximate, but avoids expensive operations + while (events.size() > 100) { + events.pollFirst(); }Committable suggestion skipped: line range outside the PR's diff.
demo/src/main/resources/static/dashboard.js-128-138 (1)
128-138: Simulation tick handler is re-registered on every update.Each call to
updateTopologyadds a newon('tick', ...)handler without removing the previous one. This could cause performance degradation over time with multiple handlers executing.🔎 Suggested fix
// Update simulation simulation.nodes(nodeData); simulation.force('link').links(links); simulation.alpha(0.3).restart(); + // Remove previous tick handler before adding new one + simulation.on('tick', null); simulation.on('tick', () => { svg.selectAll('.link')demo-order/inventory-service/src/main/java/org/pragmatica/aether/demo/order/inventory/InventoryServiceSlice.java-70-90 (1)
70-90: Race condition inreserveStock(demo-acceptable).There's a TOCTOU gap between reading
available(line 72) and thecomputecall (line 84). Concurrent reservations could drive stock negative since the compute lambda doesn't re-verify sufficiency:STOCK.compute(productId, (k, v) -> v - requested); // v could now be < requestedFor production, wrap the entire check-and-reserve in a single atomic operation:
STOCK.compute(productId, (k, v) -> { if (v == null || v < requested) return v; // signal failure differently return v - requested; });Since this is demo code, this is acceptable, but worth noting for future reference.
demo/src/main/java/org/pragmatica/aether/demo/DemoCluster.java-252-267 (1)
252-267: Potential NPE instatus()if maps are inconsistent.At line 257,
nodeInfos.get(entry.getKey())could returnnullif a node exists innodesbut not innodeInfos, causing a NPE when calling.address().port(). This could happen during concurrent add/remove operations.🔎 Proposed defensive fix
public ClusterStatus status() { var nodeStatuses = nodes.entrySet() .stream() + .filter(entry -> nodeInfos.containsKey(entry.getKey())) .map(entry -> new NodeStatus( entry.getKey(), nodeInfos.get(entry.getKey()).address().port(),node/src/main/java/org/pragmatica/aether/http/HttpRouter.java-126-142 (1)
126-142: Graceful shutdown doesn't wait for completion.The
stop()method callsshutdownGracefully()but doesn't wait for the event loop groups to actually shut down before resolving the promise. This could cause resource leaks or race conditions if the node is restarted quickly.🔎 Proposed fix to await graceful shutdown
@Override public Promise<Unit> stop() { return Promise.promise(promise -> { if (serverChannel != null) { serverChannel.close().addListener(f -> { - bossGroup.shutdownGracefully(); - workerGroup.shutdownGracefully(); - log.info("HTTP router stopped"); - promise.succeed(unit()); + var bossFuture = bossGroup.shutdownGracefully(); + var workerFuture = workerGroup.shutdownGracefully(); + bossFuture.addListener(bf -> + workerFuture.addListener(wf -> { + log.info("HTTP router stopped"); + promise.succeed(unit()); + }) + ); }); } else { - bossGroup.shutdownGracefully(); - workerGroup.shutdownGracefully(); - promise.succeed(unit()); + var bossFuture = bossGroup.shutdownGracefully(); + var workerFuture = workerGroup.shutdownGracefully(); + bossFuture.addListener(bf -> + workerFuture.addListener(wf -> promise.succeed(unit())) + ); } }); }
| function updateTimeline(newEvents) { | ||
| const timeline = document.getElementById('timeline'); | ||
|
|
||
| // Check for new events | ||
| if (newEvents.length > events.length) { | ||
| const added = newEvents.slice(events.length); | ||
| added.forEach(event => { | ||
| const div = document.createElement('div'); | ||
| div.className = 'timeline-event'; | ||
| div.innerHTML = ` | ||
| <span class="event-time">${formatEventTime(event.timestamp)}</span> | ||
| <span class="event-type ${event.type}">${event.type}</span> | ||
| <span class="event-message">${event.message}</span> | ||
| `; | ||
| timeline.insertBefore(div, timeline.firstChild); | ||
| }); | ||
|
|
||
| // Keep only last 20 events in DOM | ||
| while (timeline.children.length > 20) { | ||
| timeline.removeChild(timeline.lastChild); | ||
| } | ||
| } | ||
|
|
||
| events = newEvents; | ||
| } |
There was a problem hiding this comment.
Potential XSS vulnerability and timeline reset handling.
-
innerHTMLis used withevent.messagewhich comes from the server. If an attacker controls event messages, they could inject malicious HTML/JS. -
The timeline logic assumes
newEvents.length > events.lengthto detect new events. After a server-side reset (via/api/reset-metrics),newEventswill be shorter, causing the condition to fail and stale DOM elements to remain.
🔎 Suggested fix for XSS
+function escapeHtml(text) {
+ const div = document.createElement('div');
+ div.textContent = text;
+ return div.innerHTML;
+}
function updateTimeline(newEvents) {
const timeline = document.getElementById('timeline');
- // Check for new events
- if (newEvents.length > events.length) {
+ // Handle reset case
+ if (newEvents.length < events.length) {
+ timeline.innerHTML = '';
+ events = [];
+ }
+
+ if (newEvents.length > events.length) {
const added = newEvents.slice(events.length);
added.forEach(event => {
const div = document.createElement('div');
div.className = 'timeline-event';
- div.innerHTML = `
- <span class="event-time">${formatEventTime(event.timestamp)}</span>
- <span class="event-type ${event.type}">${event.type}</span>
- <span class="event-message">${event.message}</span>
- `;
+ div.innerHTML = `
+ <span class="event-time">${escapeHtml(formatEventTime(event.timestamp))}</span>
+ <span class="event-type ${escapeHtml(event.type)}">${escapeHtml(event.type)}</span>
+ <span class="event-message">${escapeHtml(event.message)}</span>
+ `;
timeline.insertBefore(div, timeline.firstChild);
});🤖 Prompt for AI Agents
In demo/src/main/resources/static/dashboard.js around lines 490 to 514, the code
uses innerHTML to inject server-provided event.message (XSS risk) and only
appends new events when newEvents.length > events.length (so DOM isn’t
reconciled when server resets or sends fewer events). Fix by escaping or
avoiding innerHTML: create elements and set their text via textContent or
createTextNode for event.message, event.type and time instead of templated
innerHTML; and change the update logic to reconcile the DOM whenever newEvents
differs from events (if newEvents is shorter or not strictly longer, rebuild the
timeline from newEvents or remove stale nodes before inserting added events),
then enforce trimming to the last 20 items and finally assign events =
newEvents.
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java
Show resolved
Hide resolved
| private static Version extractVersion(VersionPattern pattern) { | ||
| return switch (pattern) { | ||
| case VersionPattern.Exact(Version version) -> version; | ||
| case VersionPattern.Range(Version from, _, _, _) -> from; | ||
| case VersionPattern.Comparison(_, Version version) -> version; | ||
| case VersionPattern.Tilde(Version version) -> version; | ||
| case VersionPattern.Caret(Version version) -> version; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the Repository interface definition
fd -t f "Repository.java"Repository: siy/aether
Length of output: 214
🏁 Script executed:
# Search for the Repository.locate method definition
rg "interface Repository" -A 20Repository: siy/aether
Length of output: 3473
🏁 Script executed:
# Search for Artifact.artifact method
rg "static.*artifact\(" -A 5 -B 2Repository: siy/aether
Length of output: 5100
🏁 Script executed:
# Check how extractVersion is used in the codebase
rg "extractVersion" -B 3 -A 3Repository: siy/aether
Length of output: 5715
🏁 Script executed:
# Check LocalRepository implementation
cat slice/src/main/java/org/pragmatica/aether/slice/repository/maven/LocalRepository.javaRepository: siy/aether
Length of output: 3002
🏁 Script executed:
# Search for other Repository implementations
fd -t f -path "*/repository/*" -name "*.java" | head -20Repository: siy/aether
Length of output: 222
🏁 Script executed:
# Check how the dependency resolution actually works - look at the caller context
rg "toArtifact.*dependency" -B 5 -A 10Repository: siy/aether
Length of output: 8792
extractVersion returns an arbitrary version from the pattern rather than resolving to an available artifact.
For Range and Comparison patterns, extracting a lower bound or base version does not guarantee that version exists in the repository. Since Repository.locate() performs a direct file lookup for the exact version (e.g., {repo}/{groupId}/{artifactId}/1.0.0/artifact-1.0.0.jar), specifying a pattern like [1.0.0,2.0.0) will fail if only 1.5.0 is available—the method will return "artifact not found" rather than finding a compatible version within the range.
🤖 Prompt for AI Agents
In
slice/src/main/java/org/pragmatica/aether/slice/dependency/SharedDependencyLoader.java
around lines 161 to 169, extractVersion currently returns an arbitrary bound
(e.g., lower bound for Range) which may not exist in the repository; change the
logic so Range and Comparison patterns are resolved against the repository’s
available versions and return an actual existing version: update the method to
accept (or make a new helper that accepts) the Repository (or a list of
available versions) and, for Range/Comparison, query the repo for candidate
versions, filter them by the pattern predicate, choose the best match (e.g.,
highest compatible version), and return that Version (or Optional<Version>) so
Repository.locate() receives an existing version; if no compatible version is
found surface a clear "artifact not found" result instead of returning the
pattern bound.
There was a problem hiding this comment.
Acknowledged. This is a known limitation of the current implementation. Version range resolution against available artifacts is planned for when the artifact repository infrastructure is implemented. The current code works for exact versions; range patterns will fail gracefully with 'artifact not found' until proper resolution is added.
There was a problem hiding this comment.
@siy, thank you for acknowledging this limitation. I understand that version range resolution will be implemented later when the artifact repository infrastructure is in place, and that the current graceful failure with 'artifact not found' is an acceptable interim behavior. I've noted this for future reviews.
✏️ Learnings added
Learnt from: siy
Repo: siy/aether PR: 17
File: slice/src/main/java/org/pragmatica/aether/slice/dependency/SharedDependencyLoader.java:161-169
Timestamp: 2025-12-24T21:31:46.925Z
Learning: In SharedDependencyLoader.java, the extractVersion method returns an arbitrary version from patterns (e.g., lower bound for Range/Comparison) rather than resolving against available repository versions. This is a known limitation that will be addressed when artifact repository infrastructure is implemented. Current behavior: works correctly for exact versions, range patterns fail gracefully with 'artifact not found'.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| private Promise<LoadedSlice> loadWithSharedLoader(Artifact artifact, SharedLibraryClassLoader shared) { | ||
| return DependencyResolver.resolve(artifact, compositeRepository(), registry, shared) | ||
| .map(slice -> { | ||
| // Extract the classloader from the slice's class | ||
| var sliceClassLoader = slice.getClass().getClassLoader(); | ||
| if (sliceClassLoader instanceof SliceClassLoader scl) { | ||
| return createEntry(artifact, slice, scl); | ||
| } | ||
| // Fallback - create a minimal classloader entry | ||
| return createEntry(artifact, slice, new SliceClassLoader(new URL[0], shared)); | ||
| }) | ||
| .onFailure(cause -> log.error("Failed to load slice {}: {}", artifact, cause.message())); | ||
| } |
There was a problem hiding this comment.
Fallback creates SliceClassLoader without slice JAR URL.
At line 113, if the slice's class loader is not a SliceClassLoader, the fallback creates one with new URL[0]. This means the slice won't have access to its own JAR resources, which could cause ClassNotFoundException or missing resources.
This appears to be a "should never happen" defensive case. Consider logging a warning or throwing an error instead.
🔎 Proposed fix - log warning
// Fallback - create a minimal classloader entry
+ log.warn("Slice {} loaded with unexpected classloader type: {}. Resource access may be limited.",
+ artifact, sliceClassLoader.getClass().getName());
return createEntry(artifact, slice, new SliceClassLoader(new URL[0], shared));Or consider failing explicitly if this state is unexpected:
- // Fallback - create a minimal classloader entry
- return createEntry(artifact, slice, new SliceClassLoader(new URL[0], shared));
+ throw new IllegalStateException(
+ "Slice " + artifact + " loaded with unexpected classloader: " + sliceClassLoader.getClass());📝 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 Promise<LoadedSlice> loadWithSharedLoader(Artifact artifact, SharedLibraryClassLoader shared) { | |
| return DependencyResolver.resolve(artifact, compositeRepository(), registry, shared) | |
| .map(slice -> { | |
| // Extract the classloader from the slice's class | |
| var sliceClassLoader = slice.getClass().getClassLoader(); | |
| if (sliceClassLoader instanceof SliceClassLoader scl) { | |
| return createEntry(artifact, slice, scl); | |
| } | |
| // Fallback - create a minimal classloader entry | |
| return createEntry(artifact, slice, new SliceClassLoader(new URL[0], shared)); | |
| }) | |
| .onFailure(cause -> log.error("Failed to load slice {}: {}", artifact, cause.message())); | |
| } | |
| private Promise<LoadedSlice> loadWithSharedLoader(Artifact artifact, SharedLibraryClassLoader shared) { | |
| return DependencyResolver.resolve(artifact, compositeRepository(), registry, shared) | |
| .map(slice -> { | |
| // Extract the classloader from the slice's class | |
| var sliceClassLoader = slice.getClass().getClassLoader(); | |
| if (sliceClassLoader instanceof SliceClassLoader scl) { | |
| return createEntry(artifact, slice, scl); | |
| } | |
| // Fallback - create a minimal classloader entry | |
| log.warn("Slice {} loaded with unexpected classloader type: {}. Resource access may be limited.", | |
| artifact, sliceClassLoader.getClass().getName()); | |
| return createEntry(artifact, slice, new SliceClassLoader(new URL[0], shared)); | |
| }) | |
| .onFailure(cause -> log.error("Failed to load slice {}: {}", artifact, cause.message())); | |
| } |
| private Promise<LoadedSlice> loadWithSharedLoader(Artifact artifact, SharedLibraryClassLoader shared) { | |
| return DependencyResolver.resolve(artifact, compositeRepository(), registry, shared) | |
| .map(slice -> { | |
| // Extract the classloader from the slice's class | |
| var sliceClassLoader = slice.getClass().getClassLoader(); | |
| if (sliceClassLoader instanceof SliceClassLoader scl) { | |
| return createEntry(artifact, slice, scl); | |
| } | |
| // Fallback - create a minimal classloader entry | |
| return createEntry(artifact, slice, new SliceClassLoader(new URL[0], shared)); | |
| }) | |
| .onFailure(cause -> log.error("Failed to load slice {}: {}", artifact, cause.message())); | |
| } | |
| private Promise<LoadedSlice> loadWithSharedLoader(Artifact artifact, SharedLibraryClassLoader shared) { | |
| return DependencyResolver.resolve(artifact, compositeRepository(), registry, shared) | |
| .map(slice -> { | |
| // Extract the classloader from the slice's class | |
| var sliceClassLoader = slice.getClass().getClassLoader(); | |
| if (sliceClassLoader instanceof SliceClassLoader scl) { | |
| return createEntry(artifact, slice, scl); | |
| } | |
| throw new IllegalStateException( | |
| "Slice " + artifact + " loaded with unexpected classloader: " + sliceClassLoader.getClass()); | |
| }) | |
| .onFailure(cause -> log.error("Failed to load slice {}: {}", artifact, cause.message())); | |
| } |
🤖 Prompt for AI Agents
In slice/src/main/java/org/pragmatica/aether/slice/SliceStoreImpl.java around
lines 104 to 116, the fallback creates a SliceClassLoader with an empty URL
array which prevents the slice from accessing its own JAR; change the fallback
to extract the slice JAR URL from
slice.getClass().getProtectionDomain().getCodeSource().getLocation() (guarding
for null), construct a URL[] containing that location and pass it into the new
SliceClassLoader so the slice has its resources; if the
ProtectionDomain/CodeSource or URL is null, log a warning with artifact details
and return a failed Promise (or throw an IllegalStateException) rather than
silently creating an empty classloader.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
demo/src/main/resources/static/dashboard.js (1)
492-539: XSS mitigation and timeline reset handling properly addressed.The previous review concerns have been resolved:
escapeHtmlfunction added and used for safe output- Reset case handled when
newEvents.length < events.length- DOM elements created with
textContentinstead ofinnerHTMLtemplate literals
🧹 Nitpick comments (5)
docs/aether-high-level-overview.md (1)
141-147: Clarify Phase 3 status hierarchy.Phase 3 is labeled "Planned," but Line 144 indicates "Layer 1: Decision tree controller (done)," creating ambiguity about which items are actually completed versus planned. Consider restructuring this section to make the per-item status more explicit.
🔎 Suggested revision for clarity
~### Phase 3: AI Integration (Planned) +### Phase 3: AI Integration (In Progress) ~ ~- Layered autonomy architecture (see [ai-integration.md](ai-integration.md)) ~- Layer 1: Decision tree controller (done) ~- Layer 2: SLM integration (planned) ~- Layer 3: LLM integration (planned) ~- Direct agent API (no MCP) +- Layered autonomy architecture (see [ai-integration.md](ai-integration.md)) + - Layer 1: Decision tree controller ✅ + - Layer 2: SLM integration (planned) + - Layer 3: LLM integration (planned) + - Direct agent API (no MCP)demo/src/main/resources/static/dashboard.js (1)
541-545: Consider handling fractional seconds.If
status.uptimeSecondscontains a fractional value (e.g.,65.5), the modulo operation will produce5.5, and the display may show"1:5.5"instead of"1:05".🔎 Suggested fix
function formatUptime(seconds) { - const mins = Math.floor(seconds / 60); - const secs = seconds % 60; + const totalSecs = Math.floor(seconds); + const mins = Math.floor(totalSecs / 60); + const secs = totalSecs % 60; return `${mins}:${secs.toString().padStart(2, '0')}`; }demo/src/main/java/org/pragmatica/aether/demo/StaticFileHandler.java (3)
60-67: URL decoding before security check is correct.The implementation correctly decodes the URL (line 61) before checking for directory traversal (line 64), which prevents bypass attacks using percent-encoded paths like
%2e%2e. This aligns with the security fix mentioned in the PR commit messages.
Consider more robust path normalization.
While the current
contains("..")check is effective for this use case, consider usingjava.nio.file.Pathnormalization for additional robustness against edge cases like./, multiple slashes, or other path manipulation attempts.🔎 Example approach using Path normalization
import java.nio.file.Path; import java.nio.file.Paths; // After URL decoding path = URLDecoder.decode(path, StandardCharsets.UTF_8); // Normalize and verify the path stays within bounds try { var normalizedPath = Paths.get(path).normalize().toString(); if (normalizedPath.contains("..") || !normalizedPath.startsWith("/")) { sendError(ctx, FORBIDDEN, "Invalid path"); return; } path = normalizedPath; } catch (Exception e) { sendError(ctx, FORBIDDEN, "Invalid path"); return; }This would handle edge cases more comprehensively while maintaining the same security guarantee.
92-102: Resource loading implementation is correct for demo scope.The implementation properly uses try-with-resources and loads the entire resource into memory using
readAllBytes(). This is acceptable for a demo serving typical web assets (HTML, CSS, JavaScript, images).
For production, consider streaming large files.
Loading entire files into memory works well for small static assets but could be memory-intensive for larger files. For a production static file server, consider using Netty's
ChunkedWriteHandlerandDefaultFileRegionorChunkedFilefor efficient streaming.
89-89: Consider HTTP keep-alive for production use.The handler closes the connection after every response (line 89, also line 118 in
sendError). This is simple and acceptable for a demo, but in a production environment, supporting HTTP keep-alive would reduce connection overhead and improve performance.To support keep-alive, check if the request includes
Connection: keep-aliveand conditionally useChannelFutureListener.CLOSEonly when needed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
demo/src/main/java/org/pragmatica/aether/demo/StaticFileHandler.javademo/src/main/resources/static/dashboard.jsdocs/aether-high-level-overview.mdhttp-server/src/main/java/org/pragmatica/http/server/RequestContext.javahttp-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.javanode/src/main/java/org/pragmatica/aether/http/HttpMethod.java
🚧 Files skipped from review as they are similar to previous changes (2)
- node/src/main/java/org/pragmatica/aether/http/HttpMethod.java
- http-server/src/main/java/org/pragmatica/http/server/RequestContext.java
🧰 Additional context used
🧬 Code graph analysis (1)
demo/src/main/java/org/pragmatica/aether/demo/StaticFileHandler.java (1)
demo/src/main/java/org/pragmatica/aether/demo/DemoServer.java (1)
ChannelHandler(210-252)
🔇 Additional comments (15)
docs/aether-high-level-overview.md (2)
5-5: Documentation updates align well with release objectives.The v0.4.0 updates accurately reflect the PR objectives: version bump, module restructuring (demo-order replaces example-slice, cli added), Phase 2 completion with HTTP Router, Management API, Health monitoring, and CLI implementation, and the timestamp refresh. The document is well-structured and comprehensive.
Also applies to: 40-41, 135-139, 158-158
72-72: No action needed. The fileai-integration.mdexists in the docs/ directory and the markdown links at lines 72 and 143 are correctly formatted with valid relative paths.demo/src/main/resources/static/dashboard.js (7)
1-17: LGTM!State initialization and constants are well-organized. The
MAX_HISTORYcalculation correctly corresponds to 30 seconds at 500ms intervals.
19-25: LGTM!Clean initialization sequence using
DOMContentLoaded.
128-139: Good fix for tick handler accumulation.Clearing the previous tick handler with
simulation.on('tick', null)before setting a new one correctly prevents handler accumulation on topology updates.
60-127: LGTM!The D3 data-join pattern (enter/update/exit) is correctly implemented. The full-mesh link generation is appropriate for visualizing small cluster topologies.
163-289: LGTM!Chart configuration is well-structured with:
- Bounded history arrays preventing memory growth
- Disabled animations for high-frequency updates
- Color-coded thresholds for visual feedback
295-383: LGTM!Control handlers are well-implemented:
- Safe DOM manipulation using
textContent- Reset button properly clears local history state
- Modal pattern cleanly manages node selection
547-550: LGTM!Standard date parsing and locale-aware formatting.
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java (5)
22-29: LGTM: Clean handler structure.The class properly extends
SimpleChannelInboundHandlerand follows standard Netty patterns. The constructor injection of the handler is appropriate.
52-56: LGTM: Proper exception handling.The exception handler correctly logs the error and closes the channel, following standard Netty patterns.
87-90: LGTM: Appropriate bad request handling.The method correctly creates a response writer with
keepAlive=falseand sends a BAD_REQUEST response. Closing the connection on malformed requests is appropriate.
47-49: Finally block properly addresses the buffer leak.The
context.release()call in the finally block correctly ensures thatRequestContext.release()is invoked regardless of handler success or exception. TheRequestContext.release()method properly handles buffer cleanup by checking the reference count and callingbody.release()to decrement it.
58-77: Path is passed to downstream handlers without decoding or validation.The path extracted from the URI is passed directly to
RequestContextwithout any decoding, normalization, or traversal prevention checks. Unlike query parameters (which are properly decoded withURLDecoder), the path remains raw. This means:
- Percent-encoded paths (e.g.,
%2e%2e%2f) are not decoded- Directory traversal sequences (
../) are not filtered- Paths are not normalized for consistent routing
The responsibility for these security checks is delegated entirely to downstream handlers. While this separation of concerns is reasonable, the asymmetry with query parameter handling (which ARE decoded) may mask security issues if handlers assume normalized paths.
demo/src/main/java/org/pragmatica/aether/demo/StaticFileHandler.java (1)
24-43: Well-structured handler with appropriate patterns.The class correctly uses the
@Sharableannotation with a stateless design, implements the factory method pattern, and properly extendsSimpleChannelInboundHandler. The implementation is clean and well-organized for its demo scope.
| @Override | ||
| protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request) { | ||
| var path = request.uri(); | ||
|
|
||
| // Handle root path | ||
| if (path.equals("/") || path.equals("/index.html")) { | ||
| path = "/index.html"; | ||
| } | ||
|
|
||
| // Remove query string | ||
| var queryIdx = path.indexOf('?'); | ||
| if (queryIdx >= 0) { | ||
| path = path.substring(0, queryIdx); | ||
| } | ||
|
|
||
| // Decode URL before security check to prevent bypass via percent-encoding | ||
| path = URLDecoder.decode(path, StandardCharsets.UTF_8); | ||
|
|
||
| // Security: prevent directory traversal | ||
| if (path.contains("..")) { | ||
| sendError(ctx, FORBIDDEN, "Invalid path"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Add HTTP method validation for proper REST semantics.
Static file handlers should typically only respond to GET and HEAD requests. Currently, the handler will serve files for any HTTP method (POST, PUT, DELETE, etc.), which violates HTTP semantics for static resources.
🔎 Proposed fix
@Override
protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request) {
+ // Only handle GET and HEAD requests
+ if (request.method() != HttpMethod.GET && request.method() != HttpMethod.HEAD) {
+ sendError(ctx, METHOD_NOT_ALLOWED, "Method not allowed");
+ return;
+ }
+
var path = request.uri();
// Handle root pathFor HEAD requests, you may also want to avoid sending the response body while keeping the headers.
Simplify redundant root path condition.
Line 50 checks for both "/" and "/index.html", but if the path is already "/index.html", setting it to "/index.html" is redundant.
🔎 Proposed fix
- // Handle root path
- if (path.equals("/") || path.equals("/index.html")) {
- path = "/index.html";
- }
+ // Handle root path
+ if (path.equals("/")) {
+ path = "/index.html";
+ }🤖 Prompt for AI Agents
In demo/src/main/java/org/pragmatica/aether/demo/StaticFileHandler.java around
lines 45 to 67, add HTTP method validation so the handler only serves GET and
HEAD: read the request.method(), if it's neither GET nor HEAD respond with 405
Method Not Allowed (and close or set Allow: GET, HEAD header) and return; for
HEAD requests perform the same header generation as GET but do not write the
response body. Also simplify the root-path logic by replacing the redundant
check (path.equals("/index.html") || path.equals("/")) with a single condition
that maps only "/" to "/index.html" (leave "/index.html" unchanged). Ensure
these checks occur before any file lookup or body generation and keep the
existing query-string removal, URL decoding, and directory traversal check in
place.
| async function apiPost(endpoint, body = {}) { | ||
| try { | ||
| const response = await fetch(`${API_BASE}${endpoint}`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body) | ||
| }); | ||
| return await response.json(); | ||
| } catch (e) { | ||
| console.error('API error:', e); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing response.ok check before parsing JSON.
Unlike fetchStatus and fetchEvents, apiPost doesn't verify response.ok before calling response.json(). If the server returns an error status with a non-JSON body, this will throw an exception that gets caught but may mask the actual HTTP error.
🔎 Suggested fix
async function apiPost(endpoint, body = {}) {
try {
const response = await fetch(`${API_BASE}${endpoint}`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(body)
});
+ if (!response.ok) {
+ console.error('API error:', response.status, response.statusText);
+ return null;
+ }
return await response.json();
} catch (e) {
console.error('API error:', e);
return null;
}
}📝 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.
| async function apiPost(endpoint, body = {}) { | |
| try { | |
| const response = await fetch(`${API_BASE}${endpoint}`, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify(body) | |
| }); | |
| return await response.json(); | |
| } catch (e) { | |
| console.error('API error:', e); | |
| return null; | |
| } | |
| } | |
| async function apiPost(endpoint, body = {}) { | |
| try { | |
| const response = await fetch(`${API_BASE}${endpoint}`, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify(body) | |
| }); | |
| if (!response.ok) { | |
| console.error('API error:', response.status, response.statusText); | |
| return null; | |
| } | |
| return await response.json(); | |
| } catch (e) { | |
| console.error('API error:', e); | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In demo/src/main/resources/static/dashboard.js around lines 411 to 423, apiPost
currently calls response.json() unconditionally which can throw or mask HTTP
errors; update it to check response.ok first: if response.ok parse and return
JSON, otherwise attempt to parse JSON (catching parse errors) or fallback to
response.text(), log the status and body/error, and return null (or a structured
error) so HTTP error details aren't lost.
| query = null; | ||
| } | ||
|
|
||
| var method = HttpMethod.from(request.method().name()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find HttpMethod class file
fd -n 'HttpMethod' --type f | head -20
# If found, examine the file
fd -n 'HttpMethod.java' --type f -exec cat -n {} \;Repository: siy/aether
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Find HttpMethod class file
find . -type f -name '*HttpMethod*'
# Search for HttpMethod.from() implementation
rg -A 10 'public static.*from\(' --type javaRepository: siy/aether
Length of output: 1433
🏁 Script executed:
cat http-server/src/main/java/org/pragmatica/http/server/HttpMethod.javaRepository: siy/aether
Length of output: 824
Fix locale-independent conversion: use Locale.ROOT in HttpMethod.from().
The from() method currently uses method.toUpperCase() without specifying Locale.ROOT, which contradicts the commit message. Replace with method.toUpperCase(Locale.ROOT) to ensure locale-independent case conversion and avoid issues in locales like Turkish where character case conversions differ from the default English behavior.
🤖 Prompt for AI Agents
In
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java
around line 72, the call that converts the incoming method to an HttpMethod uses
a locale-dependent toUpperCase; update the conversion to be locale-independent
by passing Locale.ROOT (i.e., call toUpperCase(Locale.ROOT) when
building/normalizing the method string before passing it to HttpMethod.from) so
case conversion behaves consistently across locales.
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
slice/src/main/java/org/pragmatica/aether/slice/SliceStoreImpl.java (1)
104-118: Warning added per previous review, but fallback could be more robust.The warning at lines 113-114 addresses the observability concern from the previous review. However, the fallback
SliceClassLoaderis still created with an empty URL array, which prevents the slice from accessing its own JAR resources. If this case occurs in practice, it could causeClassNotFoundExceptionor missing resource errors.Consider extracting the slice JAR URL from
slice.getClass().getProtectionDomain().getCodeSource().getLocation()to make the fallback more robust.🔎 Optional enhancement: extract JAR URL for fallback
// Fallback - create a minimal classloader entry log.warn("Slice {} loaded with unexpected classloader type: {}. Resource access may be limited.", artifact, sliceClassLoader.getClass().getName()); - return createEntry(artifact, slice, new SliceClassLoader(new URL[0], shared)); + + // Attempt to extract the slice's JAR URL from its ProtectionDomain + var urls = new URL[0]; + try { + var codeSource = slice.getClass().getProtectionDomain().getCodeSource(); + if (codeSource != null && codeSource.getLocation() != null) { + urls = new URL[]{codeSource.getLocation()}; + } + } catch (SecurityException e) { + log.debug("Unable to access ProtectionDomain for slice {}: {}", artifact, e.getMessage()); + } + + return createEntry(artifact, slice, new SliceClassLoader(urls, shared));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
node/src/main/java/org/pragmatica/aether/http/SliceDispatcher.javanode/src/main/java/org/pragmatica/aether/invoke/SliceInvoker.javaslice/src/main/java/org/pragmatica/aether/slice/SliceStoreImpl.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-24T21:31:46.925Z
Learnt from: siy
Repo: siy/aether PR: 17
File: slice/src/main/java/org/pragmatica/aether/slice/dependency/SharedDependencyLoader.java:161-169
Timestamp: 2025-12-24T21:31:46.925Z
Learning: In SharedDependencyLoader.java, the extractVersion method returns an arbitrary version from patterns (e.g., lower bound for Range/Comparison) rather than resolving against available repository versions. This is a known limitation that will be addressed when artifact repository infrastructure is implemented. Current behavior: works correctly for exact versions, range patterns fail gracefully with 'artifact not found'.
Applied to files:
slice/src/main/java/org/pragmatica/aether/slice/SliceStoreImpl.java
📚 Learning: 2025-08-24T04:55:51.502Z
Learnt from: siy
Repo: siy/pragmatica-lite PR: 32
File: core/src/main/java/org/pragmatica/lang/Option.java:393-399
Timestamp: 2025-08-24T04:55:51.502Z
Learning: In the pragmatica-lite library, Option.lift() and Result.lift() methods are designed as boundary functions for integrating external APIs into the functional monad ecosystem. They intentionally catch all Throwables (including Errors) to prevent external API failures from breaking into functional code, maintaining a clear separation between external imperative APIs and internal functional code.
Applied to files:
slice/src/main/java/org/pragmatica/aether/slice/SliceStoreImpl.java
🔇 Additional comments (12)
node/src/main/java/org/pragmatica/aether/http/SliceDispatcher.java (3)
46-49: LGTM!The constructor correctly initializes the invoker and artifact resolver fields.
62-71: Fix confirmed: Empty params now use the map instead of null.The previous concern about NPE from null requests has been properly addressed. The code now assigns
resolvedParams(which may be empty) instead of null, preventing downstream null-pointer issues in slice methods.
73-83: LGTM!The method resolution and invocation logic is correct. Error handling properly wraps invocation failures with
HttpRouterError.InvocationFailed.node/src/main/java/org/pragmatica/aether/invoke/SliceInvoker.java (4)
83-94: LGTM!The public API addition for local invocation is well-documented and consistent with the existing invoke patterns. The method signature is clear and appropriate for HTTP router integration.
120-142: LGTM!Factory method signatures have been consistently updated to accept the
InvocationHandlerparameter, enabling local invocation support. The changes are applied to both overloads.
149-149: LGTM!The
SLICE_NOT_FOUNDconstant provides clear error semantics for local invocation failures.
154-154: LGTM!Internal state changes correctly integrate the
InvocationHandlerinto the class.Also applies to: 166-166, 173-173
slice/src/main/java/org/pragmatica/aether/slice/SliceStoreImpl.java (5)
10-10: LGTM!Import is necessary for the new Option field.
62-68: LGTM!Proper deprecation of the legacy factory method while maintaining backward compatibility.
70-72: LGTM!Record properly updated to include the optional shared library classloader field.
88-93: LGTM!Clean delegation logic using
Option.fold()to select between legacy and shared loader paths.
95-102: LGTM!Legacy loading path properly extracted with appropriate deprecation warning suppression. Error handling includes proper classloader cleanup.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java (1)
59-78: Consider URL-decoding the path.The path is not URL-decoded, so encoded characters (e.g.,
%20for space) remain encoded. This could cause issues with route matching if the router expects decoded paths.If this is intentional (to let the handler or router decode as needed), it's fine. Otherwise, consider decoding:
🔎 Suggested enhancement
+import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; + private RequestContext createRequestContext(FullHttpRequest request) { var uri = request.uri(); var queryIndex = uri.indexOf('?'); String path; String query; if (queryIndex >= 0) { - path = uri.substring(0, queryIndex); + path = URLDecoder.decode(uri.substring(0, queryIndex), StandardCharsets.UTF_8); query = uri.substring(queryIndex + 1); } else { - path = uri; + path = URLDecoder.decode(uri, StandardCharsets.UTF_8); query = null; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.javanode/src/main/java/org/pragmatica/aether/invoke/SliceInvoker.java
🚧 Files skipped from review as they are similar to previous changes (1)
- node/src/main/java/org/pragmatica/aether/invoke/SliceInvoker.java
🔇 Additional comments (2)
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java (2)
53-57: LGTM: Standard Netty error handling.The exception handler correctly logs the error and closes the channel.
88-91: LGTM: Bad requests close the connection.Setting
keepAlive=falsefor malformed requests is appropriate, as the connection state may be unpredictable after a decoder failure.
| @Override | ||
| protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request) { | ||
| if (!request.decoderResult().isSuccess()) { | ||
| sendBadRequest(ctx, "Invalid HTTP request"); | ||
| return; | ||
| } | ||
|
|
||
| var keepAlive = HttpUtil.isKeepAlive(request); | ||
| var context = createRequestContext(request); | ||
| var writer = new NettyResponseWriter(ctx, keepAlive); | ||
|
|
||
| try { | ||
| handler.accept(context, writer); | ||
| } catch (Exception e) { | ||
| LOG.error("Handler error", e); | ||
| writer.internalError(Causes.fromThrowable(e)); | ||
| } finally { | ||
| context.release(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Buffer leak edge case: exception before try block.
The finally block at line 49 correctly addresses the buffer leak flagged in the previous review. However, if createRequestContext (line 40) throws an exception after calling retain() at line 75, or if the NettyResponseWriter constructor (line 41) throws, the finally block is never entered and the retained buffer leaks.
Consider wrapping lines 40-41 in the try block or adding a separate try-catch around createRequestContext to ensure cleanup:
🔎 Refactored approach
- var keepAlive = HttpUtil.isKeepAlive(request);
- var context = createRequestContext(request);
- var writer = new NettyResponseWriter(ctx, keepAlive);
-
- try {
+ var keepAlive = HttpUtil.isKeepAlive(request);
+ RequestContext context = null;
+ try {
+ context = createRequestContext(request);
+ var writer = new NettyResponseWriter(ctx, keepAlive);
handler.accept(context, writer);
} catch (Exception e) {
LOG.error("Handler error", e);
- writer.internalError(Causes.fromThrowable(e));
+ new NettyResponseWriter(ctx, keepAlive).internalError(Causes.fromThrowable(e));
} finally {
- context.release();
+ if (context != null) {
+ context.release();
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| private Map<String, String> extractHeaders(FullHttpRequest request) { | ||
| var headers = new HashMap<String, String>(); | ||
| for (var entry : request.headers()) { | ||
| headers.put(entry.getKey().toLowerCase(Locale.ROOT), entry.getValue()); | ||
| } | ||
| return headers; | ||
| } |
There was a problem hiding this comment.
Multiple header values are lost.
HTTP allows multiple headers with the same name (e.g., Set-Cookie, Accept, Accept-Encoding). The current implementation overwrites earlier values when a duplicate name (case-insensitive) is encountered, keeping only the last one. This breaks functionality for multi-valued headers.
Consider using a Map<String, List<String>> or concatenating values with a delimiter (e.g., comma) to preserve all values:
🔎 Proposed fix with List
- private Map<String, String> extractHeaders(FullHttpRequest request) {
- var headers = new HashMap<String, String>();
+ private Map<String, java.util.List<String>> extractHeaders(FullHttpRequest request) {
+ var headers = new HashMap<String, java.util.List<String>>();
for (var entry : request.headers()) {
- headers.put(entry.getKey().toLowerCase(Locale.ROOT), entry.getValue());
+ headers.computeIfAbsent(entry.getKey().toLowerCase(Locale.ROOT), k -> new java.util.ArrayList<>())
+ .add(entry.getValue());
}
return headers;
}Note: This requires updating RequestContext to accept Map<String, List<String>> for headers.
🤖 Prompt for AI Agents
In
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java
around lines 80 to 86, the current extractHeaders implementation overwrites
duplicate header names and loses multiple values; change it to collect all
values per header name (case-insensitive) by returning Map<String, List<String>>
(or alternatively a Map<String, String> where values are joined by commas) and
append each header occurrence to the list instead of replacing; update the
RequestContext and any callers to accept Map<String, List<String>> (or the
chosen joined-string format) and adjust usages (e.g., parsing cookies, Accept
headers) to handle multiple values accordingly.
There was a problem hiding this comment.
Acknowledged as a design simplification. For this use case, single-value headers are sufficient. Multi-value header support can be added later if needed for specific scenarios like Set-Cookie handling.
There was a problem hiding this comment.
@siy, understood. I've noted that single-value headers are the intended design for this HTTP server implementation. The simplified API is appropriate when multi-value header scenarios aren't needed for the current use cases.
✏️ Learnings added
Learnt from: siy
Repo: siy/aether PR: 17
File: http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java:80-86
Timestamp: 2025-12-24T21:48:43.366Z
Learning: In the http-server module (http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java), extractHeaders uses Map<String, String> and keeps only the last value when duplicate header names appear. This is a deliberate design simplification—single-value headers are sufficient for the current use case. Multi-value header support (e.g., for Set-Cookie) can be added later if needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java (1)
61-80: Consider URL decoding for path and query parameters.The current URI parsing extracts path and query components but doesn't decode percent-encoded characters (e.g.,
%20for space,%2Ffor/). This may cause issues if handlers expect decoded values.🔎 Add URL decoding
+import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; private RequestContext createRequestContext(FullHttpRequest request) { var uri = request.uri(); var queryIndex = uri.indexOf('?'); String path; String query; if (queryIndex >= 0) { - path = uri.substring(0, queryIndex); - query = uri.substring(queryIndex + 1); + path = URLDecoder.decode(uri.substring(0, queryIndex), StandardCharsets.UTF_8); + query = uri.substring(queryIndex + 1); // Keep query encoded for later parameter parsing } else { - path = uri; + path = URLDecoder.decode(uri, StandardCharsets.UTF_8); query = null; }Note: Query string should typically remain encoded until parameter parsing to preserve
=and&delimiters.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-24T21:48:43.366Z
Learnt from: siy
Repo: siy/aether PR: 17
File: http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java:80-86
Timestamp: 2025-12-24T21:48:43.366Z
Learning: In the http-server module (http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java), extractHeaders uses Map<String, String> and keeps only the last value when duplicate header names appear. This is a deliberate design simplification—single-value headers are sufficient for the current use case. Multi-value header support (e.g., for Set-Cookie) can be added later if needed.
Applied to files:
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java
🔇 Additional comments (5)
http-server/src/main/java/org/pragmatica/http/server/impl/HttpRequestHandler.java (5)
20-30: LGTM!Clean handler setup following standard Netty patterns. The BiConsumer delegation provides good flexibility for routing.
55-59: LGTM!Appropriate channel-level exception handling with logging and cleanup.
82-88: LGTM!Locale-independent header name conversion is correctly implemented with
Locale.ROOT. Based on learnings, the single-value header design is an acceptable simplification for the current use case.
90-93: LGTM!Appropriate error response handling with connection closure for malformed requests.
32-53: The code is safe as structured. The edge case buffer leak concern is based on an incorrect premise.
HttpMethod.from()is the only operation increateRequestContextthat can throw an exception, and it occurs beforerequest.content().retain()is called. If it throws, no buffer is retained. TheRequestContextrecord has no constructor validation logic—it's auto-generated field assignment—so it won't throw. Therefore, the scenario of "throws AFTER retain() but BEFORE constructor completes" cannot occur.The try-finally pattern in
channelRead0correctly handles the actual risk: if handler processing throws, the context is properly released.
1 participant
Summary
Test plan
./mvnw clean install- all tests passSummary by CodeRabbit
New Features
Documentation
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.