Add unit tests for HttpResponse

[eafalkens]

This PR adds unit tests for the HttpResponse class.

Tests cover:

• Default and set values for statusCode and statusText.
• Null-check behavior when setting statusText.
• Default empty headers and setting/retrieving headers.
• Default empty body and setting/retrieving body content.

These tests ensure that HttpResponse correctly handles status, headers, and body, making the response system more stable and reliable.

Summary by CodeRabbit

• New Features

  • Full HTTP server with request parsing, response writing, static file serving, health/metrics, proxying, plugin-based routing, middleware, and a new CLI entrypoint.

• Documentation

  • Expanded README, ADRs, and docs with architecture, pipeline, and configuration examples; in-repo static UI updated.

• Tests

  • Extensive unit and integration test suites added/expanded.

• Chores

  • CI and Docker publish workflows, Maven wrapper, editor config, .dockerignore, and build metadata added.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a complete Java HTTP server: entrypoint, server socket loop with virtual threads, HTTP parsing/serialization, middleware pipeline and filters, routing/plugins (static, proxy, health, metrics), DI container, config loading, build/CI/Docker tooling, docs/ADRs, and extensive tests.

Changes

Cohort / File(s)	Summary
Build & CI pom.xml, .mvn/wrapper/maven-wrapper.properties, mvnw, mvnw.cmd, .github/workflows/ci.yml, .github/workflows/docker-release.yml, Dockerfile, .dockerignore, build.properties, src/main/resources-filtered/build.properties	Added Maven wrapper and properties, bumped to Java 25, new dependencies/plugins (spotless, shade, pitest, snakeyaml, bucket4j, testcontainers), CI and docker-release workflows, multi-stage Dockerfile, and .dockerignore.
Project config & tooling .editorconfig, .gitignore, src/main/resources/application-properties.yml	Editor formatting rules, .gitignore updated (META-INF), and YAML runtime configuration for server, logging, rate-limiting, and proxy routes.
Entry & server core src/main/java/org/juv25d/App.java, src/main/java/org/juv25d/Server.java, src/main/java/org/juv25d/ConnectionHandler*.java, src/main/java/org/juv25d/ConnectionHandlerFactory.java, src/main/java/org/juv25d/DefaultConnectionHandlerFactory.java	New application main, Server accept loop (virtual threads), connection handler that parses requests, builds pipeline chains, and writes responses; factory interface and default implementation added.
HTTP model & I/O src/main/java/org/juv25d/http/... (HttpParser.java, HttpRequest.java, HttpResponse.java, HttpResponseWriter.java, HttpStatus.java)	Added request parser, immutable HttpRequest record, mutable HttpResponse, response writer/serialization and HttpStatus enum.
Filter pipeline & scanning src/main/java/org/juv25d/filter/*, src/main/java/org/juv25d/filter/annotation/*	Complete middleware framework: Filter interface, FilterChain and implementation, registry, matcher, factory, scanner, registration records, and annotations @Global/@Route.
Built-in filters src/main/java/org/juv25d/filter/{Logging,SecurityHeaders,IpFilter,Timing,RateLimiting,Redirect}*.java	Added global/route filters for logging, security headers, IP allow/deny, timing, token-bucket rate limiting, and redirect rules.
Routing & plugins src/main/java/org/juv25d/router/*, src/main/java/org/juv25d/plugin/*, src/main/java/org/juv25d/router/RouterConfig.java	Router interface and SimpleRouter (exact/wildcard matching); plugins for static files, health, metrics, proxy, not-found, sample; RouterConfig wires route registrations including proxy routes from config.
Static file handling & assets src/main/java/org/juv25d/handler/StaticFileHandler.java, src/main/java/org/juv25d/handler/MimeTypeResolver.java, src/main/resources/static/*	Classpath static file serving with traversal protection, MIME detection, ETag/304 support; SPA assets, CSS/JS/HTML and static README added.
Proxying src/main/java/org/juv25d/proxy/{ProxyPlugin,ProxyRoute}.java	Proxy plugin and ProxyRoute to forward requests to upstreams with header filtering and error-to-status mapping.
DI & bootstrap src/main/java/org/juv25d/di/{Container,Inject}.java, src/main/java/org/juv25d/Bootstrap.java	Lightweight DI container with bindings, constructor injection, circular-dependency detection; Bootstrap scans and wires filters and returns Pipeline.
Configuration helpers src/main/java/org/juv25d/util/ConfigLoader.java, src/main/java/org/juv25d/config/*	Singleton YAML config loader and small config classes (RateLimitConfig, IpFilterConfig, RedirectConfig, FilterConfig).
Logging & context src/main/java/org/juv25d/logging/{ServerLogging,ServerLogFormatter,LogContext}.java	Central logger configuration, custom log formatter including connection ID, and thread-local LogContext utilities.
DI & filter internals src/main/java/org/juv25d/filter/{ClassScanner,FilterScanner,FilterRegistry,FilterMatcher,FilterFactory,FilterRegistration}.java	Class scanning, filter instantiation, registration, matching, and lifecycle management implemented.
Plugins & handlers src/main/java/org/juv25d/plugin/*, src/main/java/org/juv25d/handler/*, src/main/java/org/juv25d/router/*	Metric, HealthCheck, StaticFiles, NotFound, Hello sample plugin; StaticFileHandler and MimeTypeResolver; SimpleRouter and RouterConfig to register plugins.
Tests — unit & integration src/test/java/org/juv25d/**/* (removed src/test/java/org/example/*)	Large set of new unit and integration tests (parser, response writer, filters, registry, DI container, static handler, proxy, plugins); AppIT replaced to use Testcontainers.
Docs & ADRs & static site README.md, docs/adr/*, docs/notes/pipeline-usage.md, src/main/resources/static/*	README rewritten to architecture guide, ADR template and static-file ADR added, pipeline usage notes, and in-repo static documentation site assets.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant Server as Server
    participant Handler as ConnectionHandler
    participant Pipeline as Pipeline
    participant Chain as FilterChain
    participant Router as Router
    participant Plugin as Plugin
    participant Writer as HttpResponseWriter

    Client->>Server: TCP connect + send HTTP request
    Server->>Handler: accept socket -> spawn virtual thread
    Handler->>Handler: HttpParser.parse(InputStream)
    Handler->>Pipeline: createChain(HttpRequest)
    Pipeline->>Chain: new FilterChainImpl(filters, router)
    Handler->>Chain: doFilter(request, response)
    loop filters
      Chain->>Chain: filter.doFilter -> may call next
      alt filter short-circuits
        Chain-->>Handler: response mutated (status/body), stop chain
      end
    end
    Chain->>Router: resolve(request)
    Router->>Plugin: plugin.handle(request, response)
    Plugin-->>Chain: plugin completed
    Handler->>Writer: HttpResponseWriter.write(outputStream, response)
    Writer-->>Client: bytes flushed and connection closed

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

🐰 I hopped from template, code in paw,
Built filters, plugins, and a routing law.
Static files snug, proxies that stream,
Virtual threads hum like a dream—
Cheers! The server's up, ready to beam. 🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.87% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Add unit tests for HttpResponse' is clear, specific, and directly summarizes the main purpose of the changeset—introducing unit tests for the HttpResponse class.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 13

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (29)

docs/notes/pipeline-usage.md-25-25 (1)

25-25: ⚠️ Potential issue | 🟡 Minor

Clarify or remove the "(if implemented)" qualifier.

Documenting "/admin/**" as a supported pattern while hedging with (if implemented) is misleading — a reader who uses it won't know whether it will work. Either confirm and remove the qualifier, or replace the line with a clear note that recursive wildcards are not yet supported.

📝 Suggested alternatives

If implemented:

-        //   - "/admin/**" = recursive wildcard (if implemented)
+        //   - "/admin/**" = recursive wildcard (matches any nested path under /admin/)

If not yet implemented:

-        //   - "/admin/**" = recursive wildcard (if implemented)
+        //   NOTE: Recursive wildcards ("**") are not yet supported; use "/admin/*" for single-level matching.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/notes/pipeline-usage.md` at line 25, The documentation line mentioning
the pattern "/admin/**" currently hedges with "(if implemented)"; update that
line to be definitive: either remove the qualifier and state that recursive
wildcards are supported (confirming the behavior and optionally add a short
example), or replace the line with a clear note that recursive wildcards are not
yet supported and suggest the current supported alternative (e.g., "/admin/*" or
explicit paths). Locate the "/admin/**" entry in pipeline-usage.md and make the
text change so readers know definitively whether recursive wildcards work.

docs/notes/pipeline-usage.md-1-69 (1)

1-69: ⚠️ Potential issue | 🟡 Minor

Documentation file is missing all Markdown structure.

The entire file is raw commented-out Java code with no Markdown headings, no prose explanation, and no code fence. As a .md file, GitHub will render it as a plain wall of text with no syntax highlighting or navigational context.

At minimum, add a title, a one-line description, and wrap the snippet in a java code fence:

📝 Suggested structure

+# Pipeline Usage Examples
+
+Example configuration snippet for wiring global and route-specific filters into the application pipeline.
+Uncomment and adapt the relevant lines in your `App` bootstrap class.
+
+```java
         // Global filters: Applied to every request
         // ...
+```

Additionally, this file appears unrelated to the stated PR objective (unit tests for HttpResponse). Consider whether it belongs in a separate PR or commit.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/notes/pipeline-usage.md` around lines 1 - 69, The Markdown file
docs/notes/pipeline-usage.md lacks proper Markdown structure and code fencing;
add a top-level title (e.g., "# Pipeline Usage"), a one-line description, and
wrap the Java snippet (the commented lines starting with "// Global filters" and
calls like pipeline.addGlobalFilter(...) / pipeline.addRouteFilter(...)) in a
```java code fence for proper rendering and syntax highlighting; also evaluate
whether this docs change should be split into its own PR separate from the
HttpResponse unit-test work and move it to a dedicated commit/PR if unrelated.

.github/workflows/docker-release.yml-37-37 (1)

37-37: ⚠️ Potential issue | 🟡 Minor

org.opencontainers.image.source receives a non-URL value.

${{ github.repository }} resolves to owner/repo (e.g. ithsjava25/ithsjava25-java-project-webserver-maven-java-template), not a URI. The OCI image spec requires a full URL for this annotation. docker/metadata-action@v5 already auto-generates this label correctly as https://github.com/<repo> from the workflow context — this explicit override replaces the correct value with a malformed one.

Either remove the line entirely (let the action populate it automatically) or supply the full URL:

🐛 Proposed fix

-          labels: org.opencontainers.image.source=${{ github.repository }}
+          labels: org.opencontainers.image.source=https://github.com/${{ github.repository }}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/docker-release.yml at line 37, The annotation
org.opencontainers.image.source is being set to a non-URL value using ${{
github.repository }} which yields "owner/repo" and breaks the OCI spec; remove
this explicit labels: org.opencontainers.image.source=${{ github.repository }}
line so docker/metadata-action@v5 can auto-populate the correct
https://github.com/<repo> URL, or replace the value with a full URL (e.g., build
the URL from github.repository with the https://github.com/ prefix) wherever the
label is defined.

src/main/java/org/juv25d/handler/MimeTypeResolver.java-19-19 (1)

19-19: ⚠️ Potential issue | 🟡 Minor

application/javascript is OBSOLETE per IANA/RFC 9239 — use text/javascript instead.

The IANA registration for application/javascript lists its intended usage as OBSOLETE, obsoleted in favour of text/javascript. Per the IANA Media Types registry, RFC 9239, and the HTML specification, JavaScript content should always be served using the MIME type text/javascript.

The corresponding assertion in MimeTypeResolverTest (lines 22 and 65) will also need updating.

🐛 Proposed fix

-        MIME_TYPES.put("js", "application/javascript");
+        MIME_TYPES.put("js", "text/javascript");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/handler/MimeTypeResolver.java` at line 19, Update
the MIME mapping for JavaScript in MimeTypeResolver by replacing the obsolete
"application/javascript" value with "text/javascript" where MIME_TYPES.put("js",
...) is defined, and update the expected assertions in MimeTypeResolverTest (the
tests that assert the MIME for ".js" or "js") to expect "text/javascript"
instead of "application/javascript"; ensure any related comments or constants
referencing the obsolete type are adjusted accordingly.

src/test/java/org/juv25d/http/HttpParserTest.java-119-127 (1)

119-127: ⚠️ Potential issue | 🟡 Minor

Request string in parseValidQueryString has no terminating blank line — potentially fragile.

The request "GET /search?q=java HTTP/1.1\r\n" has no headers and no trailing \r\n blank line that normally signals end-of-headers. The test relies on the parser gracefully handling EOF as an end-of-headers signal. If HttpParser ever tightens validation to require the blank line (as the HTTP/1.1 spec mandates), this test will break. Consider adding a terminating \r\n:

🛡️ Suggested fix

-        String request = "GET /search?q=java HTTP/1.1\r\n";
+        String request = "GET /search?q=java HTTP/1.1\r\n" +
+            "\r\n";

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/http/HttpParserTest.java` around lines 119 - 127,
The test parseValidQueryString constructs a request string without the required
terminating blank line, making it fragile if HttpParser starts enforcing
end-of-headers; update the request string used in HttpParserTest (the local
variable request in parseValidQueryString) to append an extra "\r\n" so the
input passed to parser.parse(createInputStream(request)) includes the required
CRLF blank line after headers, ensuring the parser sees a proper end-of-headers
per HTTP/1.1.

src/main/java/org/juv25d/http/HttpResponse.java-22-27 (1)

22-27: ⚠️ Potential issue | 🟡 Minor

4-arg constructor allows null statusText, inconsistent with the setter's requireNonNull.

setStatusText enforces non-null via Objects.requireNonNull, but the parameterised constructor stores statusText without any check. A caller can construct an HttpResponse with null statusText and bypass the invariant entirely.

🐛 Proposed fix

 public HttpResponse(int statusCode, String statusText, Map<String, String> headers, byte[] body) {
     this.statusCode = statusCode;
-    this.statusText = statusText;
+    this.statusText = Objects.requireNonNull(statusText, "statusText must not be null");
     this.headers = new LinkedHashMap<>(headers);
     this.body = body != null ? body.clone() : new byte[0];
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/http/HttpResponse.java` around lines 22 - 27, The
4-arg constructor HttpResponse(int statusCode, String statusText, Map<String,
String> headers, byte[] body) accepts a null statusText and bypasses the
non-null invariant enforced by setStatusText; update the constructor to validate
statusText (e.g., call Objects.requireNonNull(statusText, "statusText") or
delegate to setStatusText(statusText)) before assigning to this.statusText so
the same null-check is applied consistently.

src/test/java/org/juv25d/http/HttpParserTest.java-153-153 (1)

153-153: ⚠️ Potential issue | 🟡 Minor

Use StandardCharsets.UTF_8 for charset consistency with createInputStream.

body.getBytes() uses the JVM's default charset. This is inconsistent with createInputStream, which explicitly uses StandardCharsets.UTF_8. It works today because "body" is ASCII, but it's a latent failure for any non-ASCII body content.

🐛 Proposed fix

-        assertThat(result.body()).isEqualTo(body.getBytes());
+        assertThat(result.body()).isEqualTo(body.getBytes(StandardCharsets.UTF_8));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/http/HttpParserTest.java` at line 153, The assertion
uses body.getBytes() which relies on the platform default charset and is
inconsistent with createInputStream (which uses StandardCharsets.UTF_8); update
the assertion to compare result.body() to body.getBytes(StandardCharsets.UTF_8)
and add/import java.nio.charset.StandardCharsets so the test explicitly uses
UTF-8 (refer to result.body(), body.getBytes(), and createInputStream).

src/test/java/org/juv25d/logging/ServerLoggingTest.java-123-141 (1)

123-141: ⚠️ Potential issue | 🟡 Minor

Fallback test does not control the LOG_LEVEL environment variable.

ServerLogging.configure checks System.getenv("LOG_LEVEL") before ConfigLoader. If the LOG_LEVEL env var happens to be set in the test environment (e.g., CI), the test won't actually exercise the config-file fallback path and could pass or fail for the wrong reason.

Consider documenting this assumption, or using a dedicated logger + a custom configure overload that accepts a fallback explicitly, to make the test deterministic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/logging/ServerLoggingTest.java` around lines 123 -
141, The test doesn’t control the LOG_LEVEL environment variable read by
ServerLogging.configure, so make the test deterministic by removing or
overriding that env var for the duration of the test: before calling
ServerLogging.configure(Logger), capture the current value of
System.getenv("LOG_LEVEL"), remove the "LOG_LEVEL" entry from the process
environment (via the standard reflection-based approach that mutates the
internal unmodifiable env map) or set it to null/empty, then call
ServerLogging.configure(testLogger) and finally restore the original env value
in the finally block; alternatively (preferred long-term) add an overload
ServerLogging.configure(Logger, String explicitEnvLogLevelFallback) and call
that from the test with null to force the ConfigLoader path — reference
ServerLogging.configure, ConfigLoader.getInstance().getLogLevel(), and
System.getenv("LOG_LEVEL").

pom.xml-49-60 (1)

49-60: ⚠️ Potential issue | 🟡 Minor

Testcontainers version inconsistency: testcontainers:2.0.3 vs junit-jupiter:1.21.4.

These artifacts from the same org.testcontainers group have divergent version numbers. While the junit-jupiter module is designed to work with any compatible testcontainers core version, the inconsistency creates maintenance confusion. Align both to the same version line, or use a BOM (testcontainers-bom) to manage versions consistently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pom.xml` around lines 49 - 60, The pom declares
org.testcontainers:testcontainers:2.0.3 and
org.testcontainers:junit-jupiter:1.21.4 causing a version mismatch; update the
pom to manage Testcontainers versions consistently by either aligning both
artifacts to the same version (e.g., set junit-jupiter to 2.0.3 or both to
1.21.4 depending on desired release) or, better, import
org.testcontainers:testcontainers-bom and remove explicit version tags so both
testcontainers and junit-jupiter inherit a single BOM-managed version; adjust
the dependency entries (artifactId testcontainers and junit-jupiter) to omit
versions when using the BOM and add the <dependencyManagement> BOM import.

src/main/resources/static/readme.html-18-18 (1)

18-18: ⚠️ Potential issue | 🟡 Minor

Numeric character references are missing the required trailing semicolons.

&#128269 and &#128270 are both malformed. The HTML5 spec requires the closing ;. Most browsers tolerate the omission via lenient parsing, but it is non-conformant markup that can silently fail in stricter parsers or XML-mode rendering.

🐛 Proposed fix

-        <h1>&#128269 ReadMe &#128270</h1>
+        <h1>&#128269; ReadMe &#128270;</h1>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/readme.html` at line 18, The numeric character
references in the H1 element are missing trailing semicolons (currently written
as &#128269 and &#128270); update the H1 (the <h1> element containing these
entities) to use properly-terminated references &#128269; and &#128270; so the
HTML is conformant (i.e., replace each malformed numeric reference with the same
code plus a semicolon).

src/main/resources/static/readme.html-37-37 (1)

37-37: ⚠️ Potential issue | 🟡 Minor

purify.min.js uses a relative path while all other assets use absolute paths.

js/purify.min.js (no leading /) will resolve correctly when the page is served at root level, but diverges from the established convention of /js/… paths used by every other resource in this file and in index.html. This would silently break if the page were ever served from a sub-path.

🐛 Proposed fix

-<script src="js/purify.min.js"></script>
+<script src="/js/purify.min.js"></script>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/readme.html` at line 37, The script tag for the
Purify asset is using a relative src ("js/purify.min.js") which will break when
the page is served from a sub-path; update the script element in readme.html to
use the absolute path by changing the src to "/js/purify.min.js" (matching the
convention used by other assets), and verify other references to purify.min.js
follow the same absolute "/js/…" pattern.

docs/adr/ADR-001-static-file-serving-architecture.md-238-238 (1)

238-238: ⚠️ Potential issue | 🟡 Minor

Placeholder repository URL should be replaced with the actual project URL.

https://github.com/your-repo/issues/18 is a template placeholder. Update it to the real repository URL.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/adr/ADR-001-static-file-serving-architecture.md` at line 238, Replace
the placeholder repository URL "https://github.com/your-repo/issues/18" with the
actual project repository URL pointing to Issue `#18`; locate the markdown link
text (the string "https://github.com/your-repo/issues/18" in
ADR-001-static-file-serving-architecture.md) and update it to the real repo path
(e.g., "https://github.com/your-org/your-repo/issues/18") so the link resolves
to the correct issue.

src/main/resources/static/css/styles.css-63-106 (1)

63-106: ⚠️ Potential issue | 🟡 Minor

Multiple declaration-empty-line-before [error] violations inside .nav-menu.

Stylelint flags 10+ occurrences of this rule across the file (lines 5, 65, 72, 88, 90–105, 183, 212, 226, 228, 230–231, 246). These are [error]-level and will fail CI. Remove the blank lines immediately preceding property declarations within rule blocks, or configure the rule to accommodate nested/CSS-in-CSS contexts.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/css/styles.css` around lines 63 - 106, The
.nav-menu rule contains extra blank lines immediately before several property
declarations and nested blocks (e.g., before width, margin, display,
anchor-name, the li:hover nested rule, and inside &::before/&::after before
content/position/top/left/right/bottom/border-radius/position-anchor/transition)
which triggers declaration-empty-line-before errors; remove those empty lines so
each property or nested block starts directly after the preceding declaration or
brace (or alternatively update the stylelint declaration-empty-line-before
config to allow your nested/CSS-in-CSS style), focusing on the .nav-menu
selector, the nested li:hover rule, and the &::before / &::after blocks and
their properties like anchor-name, position-anchor, and transition.

docs/adr/ADR-001-static-file-serving-architecture.md-216-218 (1)

216-218: ⚠️ Potential issue | 🟡 Minor

Example usage references a non-existent method handleRequest.

The actual method is StaticFileHandler.handle(request). Anyone copying this snippet will get a compile error.

📝 Suggested fix

-    HttpResponse response = StaticFileHandler.handleRequest(request);
+    HttpResponse response = StaticFileHandler.handle(request);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/adr/ADR-001-static-file-serving-architecture.md` around lines 216 - 218,
The example calls a non-existent method StaticFileHandler.handleRequest; update
the snippet to call the real method StaticFileHandler.handle(request) so the
example compiles—locate the call to StaticFileHandler.handleRequest in the
snippet and replace it with StaticFileHandler.handle, keeping the same request
argument and downstream use of HttpResponse and HttpResponseWriter.write intact.

docs/adr/ADR-001-static-file-serving-architecture.md-89-89 (1)

89-89: ⚠️ Potential issue | 🟡 Minor

ADR states 500 for I/O errors, but the implementation actually returns 404.

StaticFileHandler.handle() catches IOException from loadResource() and calls createNotFoundResponse(), which returns a 404, not a 500. The ADR should reflect the actual behavior.

📝 Suggested fix

-- 500 Internal Server Error: I/O errors
+- 404 Not Found: File doesn't exist or cannot be read (I/O errors)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/adr/ADR-001-static-file-serving-architecture.md` at line 89, The ADR
currently claims "500 Internal Server Error: I/O errors" but
StaticFileHandler.handle() treats IOExceptions from loadResource() by calling
createNotFoundResponse(), producing a 404; update the ADR text to reflect that
I/O errors are handled as 404 Not Found (or explicitly note the implementation
choice), referencing StaticFileHandler.handle(), loadResource(), and
createNotFoundResponse() so readers understand the mapping between the ADR and
existing behavior.

src/main/resources/static/index.html-47-47 (1)

47-47: ⚠️ Potential issue | 🟡 Minor

Relative path on purify.min.js is inconsistent with the other two script tags.

Lines 46 and 48 use absolute paths (/js/marked.min.js, /js/app.js). Line 47 uses a relative path that will silently break if this page is ever served from any path other than /.

🐛 Proposed fix

-<script src="js/purify.min.js"></script>
+<script src="/js/purify.min.js"></script>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/index.html` at line 47, Change the inconsistent
relative script path so it matches the other absolute script tags: update the
<script src="js/purify.min.js"></script> entry in index.html to use an absolute
path (/js/purify.min.js) to prevent the script from breaking when the page is
served from non-root paths; ensure the change is applied alongside the existing
/js/marked.min.js and /js/app.js tags for consistency.

src/main/resources/static/css/styles.css-111-111 (1)

111-111: ⚠️ Potential issue | 🟡 Minor

Stylelint comment-whitespace-inside errors on commented-out blocks.

Lines 111 and 156 have missing whitespace inside comment delimiters.

📝 Proposed fixes

-    /*backdrop-filter: blur(2px);*/
+    /* backdrop-filter: blur(2px); */

For the closing */ on line 156 of the multi-line commented block, ensure there is a space before */:

-    }*/
+    } */

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/css/styles.css` at line 111, The comments in
styles.css violate stylelint's comment-whitespace-inside rule; update the inline
comment "/*backdrop-filter: blur(2px);*/" and the multi-line comment that ends
near line 156 to include a space after the opening /* and a space before the
closing */ (e.g., "/* backdrop-filter: ... */"), ensuring both the single-line
and the multi-line commented blocks are adjusted to have whitespace inside the
comment delimiters.

docs/adr/ADR-001-static-file-serving-architecture.md-46-46 (1)

46-46: ⚠️ Potential issue | 🟡 Minor

Three fenced code blocks are missing a language identifier (markdownlint MD040).

Lines 46, 65, and 224 open ``` blocks without a language tag. For plain-text directory trees or component diagrams, use text or plaintext.

📝 Example fix (apply to all three blocks)

-```
+```text
 src/main/resources/
 └── static/

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/adr/ADR-001-static-file-serving-architecture.md` at line 46, Three
fenced code blocks in ADR-001-static-file-serving-architecture.md are missing
language identifiers (markdownlint MD040); update each opening ``` to include a
language tag such as text or plaintext (e.g., change ``` to ```text) for the
blocks that show directory trees and component diagrams so the markdown linter
passes and the content renders as plain text.

src/test/java/org/juv25d/filter/RateLimitingFilterTest.java-85-85 (1)

85-85: ⚠️ Potential issue | 🟡 Minor

Misleading comment — the loop exhausts the bucket, it doesn't empty it.

📝 Suggested fix

-        for (int i = 0; i < 6; i++) { // Empty first bucket
+        for (int i = 0; i < 6; i++) { // Exhaust first IP bucket (5 allowed + 1 blocked)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/filter/RateLimitingFilterTest.java` at line 85, The
inline comment on the for-loop in RateLimitingFilterTest (the loop: for (int i =
0; i < 6; i++) { ...) is misleading — it says "Empty first bucket" but the loop
actually exhausts the bucket; update the comment to accurately reflect the
action (e.g., "Exhaust first bucket" or "Consume all tokens in the first
bucket") so readers of the RateLimitingFilterTest class understand that the loop
depletes the bucket rather than empties it.

src/main/resources/static/css/styles.css-44-47 (1)

44-47: ⚠️ Potential issue | 🟡 Minor

Stylelint flags corner-shape as an unknown property at 7 locations; Stylelint config needs updating.

corner-shape: squircle is a CSS Borders Module Level 4 experimental property used correctly inside @supports (corner-shape: squircle) blocks, so the usage is intentional progressive enhancement. However, Stylelint reports it as property-no-unknown [error] at each occurrence (lines 45, 129, 167, 258, 271, 341, 364), which will fail CI.

Either add an inline disable for these blocks or extend the Stylelint config:

📝 Option A — inline suppression

     `@supports` (corner-shape: squircle) {
+        /* stylelint-disable property-no-unknown */
         corner-shape: squircle;
         border-radius: 24px;
+        /* stylelint-enable property-no-unknown */
     }

📝 Option B — Stylelint config (`.stylelintrc.*`)

+  "rules": {
+    "property-no-unknown": [true, { "ignoreProperties": ["corner-shape"] }]
+  }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/css/styles.css` around lines 44 - 47, Stylelint
flags the experimental CSS property "corner-shape" used inside the `@supports`
(corner-shape: squircle) blocks, causing CI failures; fix by either (A) adding
an inline suppression around each block (e.g., wrap the `@supports` block with
comments to disable/enable the property-no-unknown rule for that block) so
"corner-shape: squircle" is ignored by Stylelint, or (B) update the Stylelint
config (.stylelintrc) to allow that property globally by configuring the
property-no-unknown rule to ignore "corner-shape" (or adding it to
ignoreProperties), ensuring occurrences in the `@supports` (corner-shape:
squircle) blocks no longer trigger errors.

src/main/resources/static/README.md-178-178 (1)

178-178: ⚠️ Potential issue | 🟡 Minor

Heading level skips from H1 to H3 violate MD001.

Lines 178 (### Example Mapping) and 251 (### Supported Patterns) appear directly under H1 (#) sections without an intervening H2 (##). Each heading level should increment by at most one step.

Change the ### headings to ## (or restructure the surrounding section hierarchy):

-### Example Mapping
+## Example Mapping

Also applies to: 251-251

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/README.md` at line 178, The headings "### Example
Mapping" and "### Supported Patterns" are jumping from H1 to H3 (violating
MD001); update those headings to H2 by changing "### Example Mapping" and "###
Supported Patterns" to "## Example Mapping" and "## Supported Patterns" (or
adjust surrounding section hierarchy so each heading increases by at most one
level) so the document follows proper Markdown heading level order.

src/main/resources/static/README.md-39-39 (1)

39-39: ⚠️ Potential issue | 🟡 Minor

Multiple fenced code blocks are missing language specifiers (MD040).

Lines 39, 141, 156, 162, 174, 269, 322, 364, 370, and 386 all open ``` without a language hint. This suppresses syntax highlighting in GitHub-rendered Markdown and fails markdownlint.

Add the appropriate identifier to each fence, e.g.:

• ASCII diagram blocks → text
• Shell commands → bash
• Path/URL blocks → text
• YAML → yaml
• Directory trees → text

Also applies to: 141-141, 156-156, 162-162, 174-174, 269-269, 322-322, 364-364, 370-370, 386-386

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/README.md` at line 39, Update the README.md fenced
code blocks that open with ``` (specifically the blocks flagged at lines 39,
141, 156, 162, 174, 269, 322, 364, 370, 386) by adding appropriate language
identifiers to each opening fence (e.g., use bash for shell commands, yaml for
YAML snippets, text for ASCII diagrams or directory trees, and text for
paths/URLs) so the fences become ```bash, ```yaml, or ```text as appropriate;
ensure each corresponding closing ``` remains unchanged and re-run markdownlint
to confirm MD040 is resolved.

src/main/java/org/juv25d/Server.java-22-22 (1)

22-22: ⚠️ Potential issue | 🟡 Minor

Cleartext ServerSocket — no TLS (CWE-319).

All traffic between clients and the server is unencrypted. For an educational project this is an acknowledged tradeoff, but if this server ever handles credentials, tokens, or session cookies the absence of TLS becomes a real confidentiality risk. Production deployments should front the server with a TLS-terminating reverse proxy or replace ServerSocket / Socket with SSLServerSocket / SSLSocket via SSLServerSocketFactory.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/Server.java` at line 22, The Server currently
constructs a plain ServerSocket in the Server class (line with "new
ServerSocket(port, 64)") which yields unencrypted traffic; replace that creation
with an SSLServerSocket obtained from SSLServerSocketFactory (e.g.,
SSLServerSocketFactory.getDefault().createServerSocket(port, 64)) and update the
try-with-resources target to SSLServerSocket so accept() yields SSLSocket
instances, and ensure the JVM keystore/truststore is configured (or load an
SSLContext with your key material) before starting; alternatively, document and
enforce that deployments must run the Server behind a TLS-terminating reverse
proxy (so code changes to ServerSocket are not needed) and add a TODO/comment in
the Server main to indicate which approach is used.

src/main/java/org/juv25d/handler/StaticFileHandler.java-99-104 (1)

99-104: ⚠️ Potential issue | 🟡 Minor

Misleading comment — logic enforces paths that start with /, it does not "reject" them.

Line 100 says // Reject absolute paths (should start with /) but the guard at line 101 returns false when the path does not start with /. The comment contradicts the implementation.

✏️ Fix the comment

-        // Reject absolute paths (should start with /)
-        if (!path.startsWith("/")) {
-            return false;
-        }
+        // Only accept paths that start with /
+        if (!path.startsWith("/")) {
+            return false;
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 99 -
104, The comment in StaticFileHandler near the path validation is misleading: it
currently reads "// Reject absolute paths (should start with /)" while the guard
(if (!path.startsWith("/")) return false;) actually enforces that paths must
start with '/' and rejects non-absolute ones; change the comment to accurately
describe the behavior (e.g., "// Require absolute paths: must start with '/'" or
similar) so it matches the logic in the method performing the check.

src/main/java/org/juv25d/Server.java-25-29 (1)

25-29: ⚠️ Potential issue | 🟡 Minor

Potential socket leak if virtual thread startup throws.

socket is not in a try-finally guard. If Thread.ofVirtual().start(handler) throws (e.g., OutOfMemoryError), the socket is never closed. The ConnectionHandler only closes the socket inside its own run() — which never executes if the thread doesn't start.

🔧 Proposed fix — guard socket with try-finally

-                Socket socket = serverSocket.accept();
-                Runnable handler = handlerFactory.create(socket, pipeline);
-                Thread.ofVirtual().start(handler);
+                Socket socket = serverSocket.accept();
+                boolean started = false;
+                try {
+                    Runnable handler = handlerFactory.create(socket, pipeline);
+                    Thread.ofVirtual().start(handler);
+                    started = true;
+                } finally {
+                    if (!started) {
+                        try { socket.close(); } catch (IOException ignored) {}
+                    }
+                }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/Server.java` around lines 25 - 29, The loop
accepting connections currently creates a Socket via serverSocket.accept(),
builds a handler with handlerFactory.create(socket, pipeline), and then starts a
virtual thread with Thread.ofVirtual().start(handler) but does not close the
socket if thread startup fails; update the accept loop to ensure the socket is
closed when starting the thread fails: after creating the handler (from
handlerFactory.create), attempt to start the virtual thread inside a try block
and on any Throwable (or Exception) in the catch/finally close the socket to
avoid leaking it; do not close the socket on successful start (so
ConnectionHandler.run() can manage it), and reference the accept/create/start
sequence (serverSocket.accept(), handlerFactory.create(...),
Thread.ofVirtual().start(handler)) when making the change.

src/main/resources/static/js/app.js-33-36 (1)

33-36: ⚠️ Potential issue | 🟡 Minor

nav is queried at module scope — null dereference if the DOM isn't ready or the element is absent.

document.querySelector(".nav-menu") runs at script-evaluation time, before DOMContentLoaded. If the script is loaded without defer (or the .nav-menu element is ever removed/renamed), nav is null and every call to navigate() will throw a TypeError at nav.classList.add(...).

Move the query inside navigate() or capture it inside the DOMContentLoaded handler:

✏️ Proposed fix — defer the query

-const nav = document.querySelector(".nav-menu");
-
 function navigate(href) {
+    const nav = document.querySelector(".nav-menu");
+    if (!nav) return;
     nav.classList.add("disable-anchors");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/js/app.js` around lines 33 - 36, The module-scope
query const nav = document.querySelector(".nav-menu") can be null if the DOM
isn't ready or the element is missing, causing navigate() to throw; move the
query into the navigate(href) function (or initialize it inside a
DOMContentLoaded handler) and guard its use with a null check before calling
nav.classList.add/remove so navigate and any other callers won't dereference
null. Locate the nav symbol and the navigate(href) function in app.js and either
perform document.querySelector(".nav-menu") at the start of navigate() or set
nav after DOMContentLoaded, and ensure you bail out or handle the case when nav
is still null.

src/main/java/org/juv25d/filter/RedirectRule.java-30-35 (1)

30-35: ⚠️ Potential issue | 🟡 Minor

Missing null validation for constructor parameters.

sourcePath and targetUrl are not validated for null. If sourcePath is null, matches() will throw a NullPointerException at sourcePath.contains("*"). Similarly, a null targetUrl will propagate silently and cause issues when used as a Location header value.

🛡️ Proposed fix

 public RedirectRule(String sourcePath, String targetUrl, int statusCode) {
     validateStatusCode(statusCode);
+    this.sourcePath = java.util.Objects.requireNonNull(sourcePath, "sourcePath must not be null");
+    this.targetUrl = java.util.Objects.requireNonNull(targetUrl, "targetUrl must not be null");
-    this.sourcePath = sourcePath;
-    this.targetUrl = targetUrl;
     this.statusCode = statusCode;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RedirectRule.java` around lines 30 - 35, The
RedirectRule constructor does not validate sourcePath and targetUrl and can lead
to NPEs (e.g., matches() calls sourcePath.contains("*")); update the
RedirectRule(String sourcePath, String targetUrl, int statusCode) to validate
both parameters (e.g., throw IllegalArgumentException if sourcePath or targetUrl
is null or empty) before calling validateStatusCode and assigning fields so
matches() and subsequent use of targetUrl are safe; reference the RedirectRule
constructor and matches() method when making the change.

src/main/java/org/juv25d/filter/RateLimitingFilter.java-109-119 (1)

109-119: ⚠️ Potential issue | 🟡 Minor

Avoid reflecting client IP in the response body.

Including the client IP in the 429 response body (line 110) can leak information (e.g., internal proxy addresses) and reflects attacker-controlled input. The status code and headers are sufficient for the client to understand the rate-limit; consider removing the IP from the body.

Proposed fix

     private void sendTooManyRequests(HttpResponse res, String ip) {
-        byte[] body = ("429 Too Many Requests: Rate limit exceeded for IP " + ip + "\n")
+        byte[] body = "429 Too Many Requests: Rate limit exceeded\n"
             .getBytes(StandardCharsets.UTF_8);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` around lines 109 -
119, The sendTooManyRequests method currently includes the client IP (ip) in the
response body, which leaks and reflects attacker-controlled input; change
sendTooManyRequests(HttpResponse res, String ip) to build a generic body that
does not include ip (e.g., "429 Too Many Requests: rate limit exceeded\n"), keep
the same status code, headers (Content-Type, Content-Length, Retry-After) and
res.setBody(byte[]) logic, and stop referencing the ip parameter in the response
content (you can keep the parameter if used for logging elsewhere, otherwise
remove it).

src/main/java/org/juv25d/Pipeline.java-43-65 (1)

43-65: ⚠️ Potential issue | 🟡 Minor

Potential duplicate filters when an exact path matches a wildcard pattern key.

If a route filter is registered under a wildcard pattern like /api/* and the request path happens to be /api/* itself (or matches both an exact key and a wildcard prefix), the same FilterRegistration list will be added twice: once from the exact-match lookup (line 47) and again from the wildcard loop (line 57). Although this is an edge case, it silently doubles filter execution.

A simple guard in the wildcard loop to skip exact-match keys would prevent this:

Proposed fix

         for (Map.Entry<String, List<FilterRegistration>> entry : routeFilters.entrySet()) {
             String pattern = entry.getKey();
-            if (pattern.endsWith("*") && path.startsWith(pattern.substring(0, pattern.length() - 1))) {
+            if (pattern.endsWith("*")
+                && !pattern.equals(path)
+                && path.startsWith(pattern.substring(0, pattern.length() - 1))) {
                 entry.getValue().stream()
                     .sorted()
                     .map(FilterRegistration::filter)
                     .forEach(filters::add);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/Pipeline.java` around lines 43 - 65, createChain is
adding filters from routeFilters twice when a key both exactly matches the
request path and also qualifies as a wildcard pattern; prevent duplicate
additions by skipping wildcard processing for keys that equal the exact path
(i.e., in the loop over routeFilters check if pattern.equals(path) and continue)
or alternatively maintain a Set of added FilterRegistration (or their identity)
and only add a registration to filters if not already present; update the loop
that inspects pattern.endsWith("*") to exclude exact-match keys or consult the
Set to avoid duplicating FilterRegistration entries.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Run the container as a non-root user.

The runtime stage has no USER instruction, so the JVM process runs as root inside the container. The official eclipse-temurin docs confirm that containers can be run with a non-root UID, noting that restricted environments like Red Hat OpenShift do this by default. Add a dedicated system user before the ENTRYPOINT:

🔒 Proposed fix — add a non-root user

 FROM eclipse-temurin:25-jre-alpine

 WORKDIR /app

+RUN addgroup -S appgroup && adduser -S appuser -G appgroup
+USER appuser

 COPY --from=build /app/target/app.jar app.jar
 ENTRYPOINT ["java", "-jar", "app.jar"]

📝 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.

Suggested change

	FROM eclipse-temurin:25-jre-alpine
	WORKDIR /app
	# might need to update this later when we have our explicit class names
	COPY --from=build /app/target/app.jar app.jar
	ENTRYPOINT ["java", "-jar", "app.jar"]
	FROM eclipse-temurin:25-jre-alpine
	WORKDIR /app
	RUN addgroup -S appgroup && adduser -S appuser -G appgroup
	USER appuser
	# might need to update this later when we have our explicit class names
	COPY --from=build /app/target/app.jar app.jar
	ENTRYPOINT ["java", "-jar", "app.jar"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 11 - 17, The container currently runs the JVM as
root (ENTRYPOINT ["java", "-jar", "app.jar"])—create a non-root system user and
switch to it before ENTRYPOINT: add a user/group (e.g., appuser/appgroup) via an
Alpine-friendly RUN (addgroup/adduser), chown the WORKDIR and the copied
artifact (referencing WORKDIR and the COPY of app.jar) to that user, and add a
USER appuser instruction immediately before the ENTRYPOINT so the JVM runs
unprivileged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Search for all construction sites of DefaultConnectionHandlerFactory
rg -n --type=java "new DefaultConnectionHandlerFactory" -A 2 -B 1

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 428

---

🏁 Script executed:

# Verify that this.pipeline is never used in DefaultConnectionHandlerFactory.java
cat -n src/main/java/org/juv25d/DefaultConnectionHandlerFactory.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 949

---

🏁 Script executed:

# Check the ConnectionHandlerFactory interface to understand the contract
fd -e java -x grep -l "interface ConnectionHandlerFactory" {} \;

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 164

---

🏁 Script executed:

# Check the ConnectionHandlerFactory interface definition
cat -n src/main/java/org/juv25d/ConnectionHandlerFactory.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 303

---

🏁 Script executed:

# Search for all usages of DefaultConnectionHandlerFactory to verify callers
rg -n "DefaultConnectionHandlerFactory" --type=java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 630

---

🏁 Script executed:

# Verify that this.pipeline is truly never used (not referenced in the create method or elsewhere)
rg -n "this\.pipeline" src/main/java/org/juv25d/DefaultConnectionHandlerFactory.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 144

---

Dead pipeline field — the stored pipeline is never used.

this.pipeline is assigned in the constructor but never read anywhere in the class. The create(Socket, Pipeline) method parameter shadows it, and the ConnectionHandlerFactory interface already passes the pipeline per-invocation. The extra constructor parameter and stored field mislead maintainers into believing a different pipeline could be injected at factory construction time.

🔧 Proposed fix — remove the unused field and constructor parameter

 public class DefaultConnectionHandlerFactory implements ConnectionHandlerFactory{
     private final HttpParser httpParser;
     private final Logger logger;
-    private final Pipeline pipeline;
 
-    public DefaultConnectionHandlerFactory(HttpParser httpParser, Logger logger, Pipeline pipeline) {
+    public DefaultConnectionHandlerFactory(HttpParser httpParser, Logger logger) {
         this.httpParser = httpParser;
         this.logger = logger;
-        this.pipeline = pipeline;
     }
 
     `@Override`
     public Runnable create(Socket socket, Pipeline pipeline) {
         return new ConnectionHandler(socket, httpParser, logger, pipeline);
     }
 }

📝 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.

Suggested change

	private final HttpParser httpParser;
	private final Logger logger;
	private final Pipeline pipeline;
	public DefaultConnectionHandlerFactory(HttpParser httpParser, Logger logger, Pipeline pipeline) {
	this.httpParser = httpParser;
	this.logger = logger;
	this.pipeline = pipeline;
	}
	@Override
	public Runnable create(Socket socket, Pipeline pipeline) {
	return new ConnectionHandler(socket, httpParser, logger, pipeline);
	}
	private final HttpParser httpParser;
	private final Logger logger;
	public DefaultConnectionHandlerFactory(HttpParser httpParser, Logger logger) {
	this.httpParser = httpParser;
	this.logger = logger;
	}
	`@Override`
	public Runnable create(Socket socket, Pipeline pipeline) {
	return new ConnectionHandler(socket, httpParser, logger, pipeline);
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/DefaultConnectionHandlerFactory.java` around lines 9
- 22, DefaultConnectionHandlerFactory currently stores a dead field pipeline set
in the constructor but never used because create(Socket, Pipeline) receives and
forwards a pipeline parameter; remove the unused private final Pipeline pipeline
field and the Pipeline parameter from the constructor (and its assignment) so
the factory only keeps HttpParser and Logger, update the constructor signature
accordingly, and ensure create(Socket, Pipeline) continues to instantiate
ConnectionHandler(socket, httpParser, logger, pipeline) using the per-invocation
pipeline parameter.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Unbounded per-IP bucket map is a memory leak / DoS vector.

The ConcurrentHashMap<String, Bucket> grows without bound. A malicious actor (or simply a large user base) will cause unbounded memory growth since entries are never evicted — destroy() is only called on shutdown. Consider using a size-bounded or time-based eviction cache (e.g., Caffeine with expireAfterAccess) to cap the map size.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` at line 25, The
buckets Map in RateLimitingFilter is unbounded which can leak memory; replace
the ConcurrentHashMap<String, Bucket> buckets with a bounded/time-evicting cache
(e.g., Caffeine or Guava Cache) configured with maximumSize and
expireAfterAccess to evict idle or excess per-IP entries, update usages of
buckets.get/put/computeIfAbsent to use cache.get(key, k -> createBucket(...)) or
cache.getIfPresent and cache.put, and ensure any Bucket.destroy() or cleanup is
invoked on eviction if needed (use Caffeine removalListener or equivalent); keep
the RateLimitingFilter class and destroy() semantics but remove the unbounded
map to prevent DoS/memory growth.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

find . -name "pom.xml" -type f | head -5

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 117

---

🏁 Script executed:

if [ -f "pom.xml" ]; then grep -A 5 -B 2 "bucket4j" pom.xml; fi

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 417

---

🌐 Web query:

Bucket4j 8.x API changes Bandwidth.classic deprecated

💡 Result:

In Bucket4j 8.x, the convenience factory/mutator methods on Bandwidth were deprecated — including Bandwidth.classic(...), Bandwidth.simple(...), withInitialTokens(...), and withId(...). The intended replacement is the fluent Bandwidth.builder() API (or the lambda-based limit builder via Bucket.builder().addLimit(...)). [1][2]

Replace Bandwidth.classic(...)

Before (deprecated):

Bandwidth limit = Bandwidth.classic(100, Refill.greedy(10, Duration.ofSeconds(1)));

After (8.x):

Bandwidth limit = Bandwidth.builder()
    .capacity(100)
    .refillGreedy(10, Duration.ofSeconds(1))
    .build();

Refill variants map directly:

• Refill.greedy(...) → .refillGreedy(...)
• Refill.intervally(...) → .refillIntervally(...)
• Refill.intervallyAligned(..., firstRefillTime) → .refillIntervallyAligned(..., firstRefillTime) [2]

Replace Bandwidth.simple(capacity, period)

simple(capacity, period) was essentially “capacity tokens per period with greedy refill”; use:

Bandwidth limit = Bandwidth.builder()
    .capacity(capacity)
    .refillGreedy(capacity, period)
    .build();

[1][2]

Replace withInitialTokens / withId

Before (deprecated):

Bandwidth limit = Bandwidth.classic(100, Refill.greedy(10, Duration.ofSeconds(1)))
    .withInitialTokens(13)
    .withId("my-limit");

After:

Bandwidth limit = Bandwidth.builder()
    .capacity(100)
    .refillGreedy(10, Duration.ofSeconds(1))
    .initialTokens(13)
    .id("my-limit")
    .build();

[1][2]

Sources: Bucket4j 8.x Bandwidth Javadoc (deprecations + “use builder()”) [1], Bucket4j 8.11.1 reference docs (builder DSL: capacity, refillGreedy/refillIntervally/refillIntervallyAligned, initialTokens, id) [2].

---

Replace deprecated Bucket4j 8.x API — Bandwidth.classic() and Refill.intervally() no longer exist.

Project uses Bucket4j 8.16.1, which removed the convenience factory methods Bandwidth.classic(...) and replaced Refill.intervally(...) with a builder-based API. Replace with:

Bandwidth limit = Bandwidth.builder()
    .capacity(capacity)
    .refillIntervally(refillTokens, refillPeriod)
    .build();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` around lines 83 -
91, The createBucket method uses deprecated Bucket4j 7.x factory methods
(Bandwidth.classic and Refill.intervally); update it to the 8.x builder API by
replacing the Bandwidth.classic(...) and Refill.intervally(...) usage with
Bandwidth.builder(), set capacity via .capacity(capacity), configure the
periodic refill via .refillIntervally(refillTokens, refillPeriod), then call
.build() to produce the Bandwidth and use that in
Bucket.builder().addLimit(...).build(); keep the method name createBucket and
only change the Bandwidth/Refill construction.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Reflected XSS — user-supplied path is interpolated into HTML without escaping.

A request such as GET /<script>alert(1)</script> HTTP/1.1 passes isPathSafe (starts with /, no .., //, or \), fails the resource load, and then createNotFoundResponse(path) embeds the raw path directly into the <code> element. The browser parses and executes the injected <script> tag.

HTML-encode the path before inserting it into the template:

🛡️ Proposed fix — HTML-encode the reflected path

+    private static String htmlEncode(String input) {
+        return input
+            .replace("&", "&")
+            .replace("<", "&lt;")
+            .replace(">", "&gt;")
+            .replace("\"", "&quot;")
+            .replace("'", "&#x27;");
+    }
+
     private static HttpResponse createNotFoundResponse(String path) {
         String html = """
                 ...
-                    <p>The requested resource <code>%s</code> was not found on this server.</p>
+                    <p>The requested resource <code>%s</code> was not found on this server.</p>
                 ...
-                """.formatted(path);
+                """.formatted(htmlEncode(path));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 151 -
180, The createNotFoundResponse method reflects the raw user-supplied path into
the HTML template causing a reflected XSS; before calling formatted(path)
HTML-encode/escape the path (convert &, <, >, ", ' to their entities) or use a
safe utility such as Apache Commons StringEscapeUtils.escapeHtml4 to produce an
escapedPath and insert that into the template; keep the Content-Type header and
UTF-8 bytes as-is so the escaped value renders safely.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

router may be null when createChain is invoked.

The router field defaults to null and is only set via setRouter. If createChain is called before setRouter, a null router is passed to FilterChainImpl, which will NPE when the chain exhausts all filters and calls router.resolve(req). Add a null guard or document the required initialization order.

Proposed fix

+        if (router == null) {
+            throw new IllegalStateException("Router must be set before creating a filter chain");
+        }
         return new FilterChainImpl(filters, router);

📝 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.

Suggested change

	return new FilterChainImpl(filters, router); // Pass router instead of plugin
	if (router == null) {
	throw new IllegalStateException("Router must be set before creating a filter chain");
	}
	return new FilterChainImpl(filters, router); // Pass router instead of plugin

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/Pipeline.java` at line 64, createChain currently
passes the possibly-null router to new FilterChainImpl(filters, router) which
can cause an NPE later; update the createChain method to guard router: if router
is null throw an IllegalStateException (message like "router not initialized;
call setRouter(...) before createChain") or instantiate/pass a no-op Router
implementation, so FilterChainImpl never receives null; reference the
createChain method, the router field and FilterChainImpl to locate where to add
the null-check and exception or no-op fallback.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Empty handle() produces a malformed HTTP response (status 0, null statusText, empty body).

When the pipeline routes a request to HelloPlugin, the HttpResponse is left in its default-constructor state: statusCode = 0, statusText = null. Any downstream serialisation (e.g., HttpResponseWriter) will either throw a NullPointerException on the null statusText or emit a nonsensical response line such as HTTP/1.1 0 null. Per the AI summary, HelloPlugin is already registered in PipelineTest at "/hello", so this path is actively tested.

At minimum, set a valid status before returning:

 `@Override`
 public void handle(HttpRequest req, HttpResponse res) throws IOException {
-    // placeholder response logic
+    res.setStatusCode(200);
+    res.setStatusText("OK");
+    res.setHeader("Content-Type", "text/plain");
+    byte[] bodyBytes = "Hello, World!".getBytes(java.nio.charset.StandardCharsets.UTF_8);
+    res.setHeader("Content-Length", String.valueOf(bodyBytes.length));
+    res.setBody(bodyBytes);
 }

Would you like me to open an issue to track a proper implementation of HelloPlugin, or generate a more complete stub?

📝 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.

Suggested change

	@Override
	public void handle(HttpRequest req, HttpResponse res) throws IOException {
	// placeholder response logic
	}
	`@Override`
	public void handle(HttpRequest req, HttpResponse res) throws IOException {
	res.setStatusCode(200);
	res.setStatusText("OK");
	res.setHeader("Content-Type", "text/plain");
	byte[] bodyBytes = "Hello, World!".getBytes(java.nio.charset.StandardCharsets.UTF_8);
	res.setHeader("Content-Length", String.valueOf(bodyBytes.length));
	res.setBody(bodyBytes);
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/plugin/HelloPlugin.java` around lines 10 - 13,
HelloPlugin.handle currently leaves the HttpResponse in its default state
causing malformed responses; update HelloPlugin.handle to set a valid status and
minimal response fields before returning (e.g., set HttpResponse.statusCode to
200, statusText to "OK", and a non-empty body and headers as needed) so
downstream serializers like HttpResponseWriter don't encounter null statusText
or zero statusCode; locate the method HelloPlugin.handle in HelloPlugin and
assign the status and body on the provided HttpResponse instance before exiting
the method.

[coderabbitai]

Actionable comments posted: 10

🧹 Nitpick comments (15)

src/main/resources/static/css/styles.css (3)

1-8: Custom properties declared on * — consider moving to :root.

Defining CSS custom properties (--base-color, --base-color-white) on the universal selector * means they're set on every element. Convention is to declare them on :root for a single source of truth and slightly less work for the browser.

♻️ Move custom properties to :root

+:root {
+    --base-color: 23, 23, 46;
+    --base-color-white: `#f7f7f7fc`;
+}
+
 * {
-    --base-color: 23, 23, 46;
-    --base-color-white: `#f7f7f7fc`;
     margin: 0;
     padding: 0;
     box-sizing: border-box;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/css/styles.css` around lines 1 - 8, The CSS custom
properties (--base-color, --base-color-white) are declared on the universal
selector '*' which sets them on every element; move those declarations to the
':root' selector so they act as global variables and avoid per-element
redeclaration—update the stylesheet by removing the custom-property lines from
the '*' rule and adding them under a ':root' rule (keep other rules in the '*'
block unchanged) so references to --base-color and --base-color-white continue
to work site-wide.

---

273-292: Duplicate &::after rule blocks — merge into one.

The .features selector has two consecutive &::after blocks (lines 273–287 and 289–292). The second block silently overrides the first's background-color and z-index properties. While this works, it's confusing and fragile.

♻️ Merge the two blocks

     &::after {
         content: "";
         position: absolute;
         top: calc(anchor(top) + 20px);
         left: calc(anchor(left) + 4px);
         right: calc(anchor(left) - 12px);
         bottom: calc(anchor(bottom) + 20px);
 
         border-radius: 10px;
 
         position-anchor: --hovered-feature;
 
         transition: 700ms
         linear(0, 0.012 0.9%, 0.05 2%, 0.411 9.2%, 0.517 11.8%, 0.611 14.6%, 0.694 17.7%, 0.765 21.1%, 0.824 24.8%, 0.872 28.9%, 0.91 33.4%, 0.939 38.4%, 0.977 50.9%, 0.994 68.4%, 1);
+        background-color: rgba(0, 150, 255, 0.3);
+        z-index: -1;
     }
 
-    &::after {
-        background-color: rgba(0, 150, 255, 0.3);
-        z-index: -1;
-    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/css/styles.css` around lines 273 - 292, There are
two consecutive &::after rule blocks that conflict; merge them into a single
&::after block so properties from the first (top, left, right, bottom,
border-radius, position-anchor, transition) and the second (background-color,
z-index) are defined together. Update the single &::after rule to include
background-color: rgba(0,150,255,0.3) and z-index: -1, ensure the transition
declaration is syntactically valid (add missing semicolon and correct
timing-function syntax), and remove the duplicate block so all styles for the
pseudo-element live in one place.

---

189-211: Remove commented-out code blocks.

Two large blocks of commented-out CSS remain (lines 189–211 for the :active nav styles and lines 377–383 for the old .features li styles). These add noise and are better tracked in version control history if needed later.

Also applies to: 377-383

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/css/styles.css` around lines 189 - 211, Remove the
dead/commented CSS blocks to reduce noise: delete the entire commented block
containing the &:has(a:active)::before / &:has(a:active)::after rules and the
&:has(a:active) > ul > li * rule, and also remove the old commented .features li
block; these selectors ( &:has(a:active)::before, &:has(a:active) > ul > li *,
and .features li ) identify the commented sections to remove so the stylesheet
contains only active rules and the history remains in version control.

src/test/java/org/juv25d/http/HttpResponseTest.java (1)

1-7: Upgrade junit-jupiter to 6.0.3 (current GA).

JUnit 6.0.3 was released as current GA on February 15, 2026. Version 6.0.2 (currently in pom.xml) is one patch behind. The upgrade includes two bug fixes: reliability improvements to @EnabledOnJre / @DisabledOnJre when using JRE.OTHER with newer Java versions, and a deadlock fix in NamespacedHierarchicalStore.computeIfAbsent().

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/http/HttpResponseTest.java` around lines 1 - 7,
Update the project's junit-jupiter dependency version from 6.0.2 to 6.0.3 in the
Maven POM where the junit-jupiter artifact/version is declared (look for
dependency elements with artifactId "junit-jupiter" or related junit-jupiter
modules such as "junit-jupiter-engine" / "junit-jupiter-api"), change the
<version> value to 6.0.3, then run a quick mvn test to ensure the new release
integrates cleanly.

pom.xml (2)

21-21: JUnit 6.0.3 is available — consider upgrading.

JUnit 6.0.3 was released on February 15, 2026 (the current version is 6.0.2). 6.0.3 is a bug fix release that resolves a deadlock issue in NamespacedHierarchicalStore.computeIfAbsent.

-        <junit.jupiter.version>6.0.2</junit.jupiter.version>
+        <junit.jupiter.version>6.0.3</junit.jupiter.version>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pom.xml` at line 21, The pom's JUnit property is out of date: update the
Maven property junit.jupiter.version from 6.0.2 to 6.0.3 (the XML element
<junit.jupiter.version>) so the build uses JUnit 6.0.3 which contains the
deadlock bugfix; after changing the property, run a quick mvn -q test to verify
dependencies resolve and tests run.

---

229-245: buildnumber-maven-plugin is declared twice — remove the redundant pluginManagement entry.

The plugin is already fully configured in <build><plugins> (Lines 188–204) with all three <configuration> properties. The <pluginManagement> entry is incomplete (no <configuration> block) and redundant for a single-module project. It adds confusion about which configuration applies.

♻️ Proposed fix: remove the incomplete pluginManagement entry

         <pluginManagement>
             <plugins>
-                <plugin>
-                    <groupId>org.codehaus.mojo</groupId>
-                    <artifactId>buildnumber-maven-plugin</artifactId>
-                    <version>3.2.0</version>
-                    <executions>
-                        <execution>
-                            <phase>validate</phase>
-                            <goals>
-                                <goal>create</goal>
-                            </goals>
-                        </execution>
-                    </executions>
-                </plugin>
             </plugins>
         </pluginManagement>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pom.xml` around lines 229 - 245, Remove the redundant <pluginManagement>
entry that declares the org.codehaus.mojo:buildnumber-maven-plugin; the plugin
is already fully configured under <build><plugins> (the buildnumber-maven-plugin
configuration with all three properties), so delete the <pluginManagement> block
containing that plugin declaration to avoid duplicate/incomplete declarations
and confusion.

src/main/java/org/juv25d/plugin/HealthCheckPlugin.java (1)

19-31: Redundant setStatusCode(200) and inconsistent JSON indentation in the text block.

Two minor issues:

1. res.setStatusCode(200) (Line 28) is a no-op since HttpResponse's default constructor already initialises statusCode to 200.
2. The closing } in the JSON text block is indented inconsistently with its opening { — it sits at 2 spaces instead of 0:

♻️ Proposed fix

         String jsonBody = """
-            {
-              "status": "UP"
-              }
-            """;
+            {"status": "UP"}
+            """;

         byte[] bodyBytes = jsonBody.getBytes(StandardCharsets.UTF_8);

-        res.setStatusCode(200);
         res.setHeader("Content-Type", "application/json");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java` around lines 19 - 31,
Remove the redundant res.setStatusCode(200) call in HealthCheckPlugin
(HttpResponse defaults to 200) and fix the JSON text block assigned to jsonBody
so its closing brace aligns with the opening brace (make the block consistently
indented as {"status":"UP"} style), then recompute bodyBytes from the corrected
jsonBody; ensure you still set Content-Type and Content-Length on res after
updating jsonBody and bodyBytes.

src/test/java/org/juv25d/plugin/MetricPluginTest.java (1)

30-30: Fragile string assertion assumes specific JSON whitespace formatting.

body.contains("\"server\": \"juv25d-webserver\"") will silently fail (as false) if MetricPlugin serializes JSON without the space after the colon ("server":"juv25d-webserver"). The assertion message won't reveal whether the field is absent or just differently formatted. For consistency with the other checks (which only assert on key presence), consider either asserting on just the key or parsing the JSON:

♻️ Proposed fix: decouple key and value assertions

-        assertTrue(body.contains("\"server\": \"juv25d-webserver\""), "Body should contain server info");
+        assertTrue(body.contains("\"server\""), "Body should contain server key");
+        assertTrue(body.contains("juv25d-webserver"), "Body should contain server value");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/plugin/MetricPluginTest.java` at line 30, The test's
fragile assertion in MetricPluginTest that checks body.contains("\"server\":
\"juv25d-webserver\"") can fail due to JSON whitespace differences; update the
test to either parse the JSON string in the body (e.g., into a Map/JsonNode) and
assert that the "server" key exists and equals "juv25d-webserver", or change the
assertion to check for the key and value separately (e.g.,
assertTrue(body.contains("\"server\"") and
assertTrue(body.contains("juv25d-webserver"))). Locate the failing assertion
around the assertTrue call referencing body in MetricPluginTest and replace it
with a parsed-JSON equality check or decoupled key/value checks so formatting
differences no longer cause false negatives.

src/test/java/org/juv25d/filter/IpFilterTest.java (1)

10-44: LGTM — consider adding blacklist coverage.

The whitelist allow/block scenarios are correctly verified. The second IpFilter constructor parameter (blacklist) is always null in both tests; blacklist-based blocking is untested. Consider adding a test case like:

`@Test`
void blacklist_blocksMatchedIp() throws IOException {
    IpFilter filter = new IpFilter(null, Set.of("10.0.0.1"));
    HttpRequest req = mock(HttpRequest.class);
    when(req.remoteIp()).thenReturn("10.0.0.1");
    HttpResponse res = new HttpResponse();
    FilterChain chain = mock(FilterChain.class);

    filter.doFilter(req, res, chain);
    verify(chain, never()).doFilter(req, res);
    assertEquals(403, res.statusCode());
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/filter/IpFilterTest.java` around lines 10 - 44, Add
a new unit test that verifies blacklist behavior: create an IpFilter with the
whitelist parameter null and the blacklist set to Set.of("10.0.0.1"), mock an
HttpRequest to return remoteIp() = "10.0.0.1", instantiate HttpResponse and a
mocked FilterChain, call filter.doFilter(req, res, chain), verify that
chain.doFilter(req, res) is never called, and assert the response statusCode()
is 403 (and optionally statusText() equals "Forbidden"); place this alongside
the existing whitelist tests and name it e.g. blacklist_blocksMatchedIp to
mirror the style of whitelist_allowsIp and whitelist_blocksIpNotInList.

src/main/java/org/juv25d/filter/TimingFilter.java (1)

21-21: Prefer parameterized JUL logging over string concatenation.

The concatenation in logger.info(...) always allocates a new String regardless of whether INFO is enabled. JUL supports deferred construction:

♻️ Proposed fix

-        logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
+        logger.log(Level.INFO, "{0} {1} took {2} ms",
+            new Object[]{req.method(), req.path(), durationMs});

Add the required import:

import java.util.logging.Level;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/TimingFilter.java` at line 21, Replace the
string-concatenation log in TimingFilter with parameterized JUL logging: import
java.util.logging.Level and change the logger.info(req.method() + " " +
req.path() + " took " + durationMs + " ms") usage to logger.log(Level.INFO,
"...", new Object[]{req.method(), req.path(), durationMs}) so the message
parameters (req.method(), req.path(), durationMs) are only formatted when INFO
is enabled; update the TimingFilter code where logger is used accordingly.

src/main/java/org/juv25d/filter/IpFilter.java (1)

42-44: getClientIp is a trivial one-liner indirection.

The private method adds no logic beyond req.remoteIp(). Inline it at the call site.

♻️ Proposed simplification

-    String clientIp = getClientIp(req);
+    String clientIp = req.remoteIp();
 
     boolean allowed;
     ...
-
-    private String getClientIp(HttpRequest req){
-        return req.remoteIp();
-    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/IpFilter.java` around lines 42 - 44, The
getClientIp method is a trivial one-liner that just returns req.remoteIp();
remove the private method getClientIp(HttpRequest req) and replace all its call
sites with direct calls to req.remoteIp(), updating imports/usages if any;
ensure no references remain to getClientIp so the class compiles after deleting
the method.

src/main/java/org/juv25d/App.java (2)

34-40: Hardcoded redirect rules are a maintenance and ops concern.

The three redirect entries (/old-page, /temp, /docs/*) are baked into main(), so changing or disabling them requires a rebuild and redeployment. Consider loading them from application-properties.yml through ConfigLoader, consistent with how port, rate-limiting, and log level are already externalised.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/App.java` around lines 34 - 40, Replace the
hardcoded List.of(...) redirect rules in main() with a configuration-driven load
via ConfigLoader: add a redirectRules property in application-properties.yml,
update ConfigLoader to parse that property into List<RedirectRule> (or provide a
getRedirectRules() method), then call pipeline.addGlobalFilter(new
RedirectFilter(redirectRules), 4) using the loaded list; locate the current
construction of RedirectRule and the pipeline.addGlobalFilter(...) call in
main() to remove the inline entries and wire the ConfigLoader-provided list
instead.

---

46-47: Redundant StaticFilesPlugin registration at both "/" and "/*".

SimpleRouter.resolve checks exact matches first, then falls back to the longest-prefix wildcard. Any request to "/" would be caught by the "/*" wildcard (prefix "/", and every path starts with "/"), so the explicit "/" registration is dead code. This also creates two separate StaticFilesPlugin instances unnecessarily.

♻️ Proposed simplification

-router.registerPlugin("/", new StaticFilesPlugin()); // Register StaticFilesPlugin for the root path
 router.registerPlugin("/*", new StaticFilesPlugin()); // Register StaticFilesPlugin for all paths

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/App.java` around lines 46 - 47, The duplicate
StaticFilesPlugin registration is redundant: SimpleRouter.resolve will match the
"/*" longest-prefix wildcard for "/" too, and registering both creates two
plugin instances; remove the router.registerPlugin("/", new StaticFilesPlugin())
call, instantiate a single StaticFilesPlugin and register it only for "/*" via
router.registerPlugin("/*", staticFilesPlugin) (referencing
router.registerPlugin and StaticFilesPlugin and SimpleRouter.resolve to locate
the code).

src/main/java/org/juv25d/filter/SecurityHeadersFilter.java (1)

20-27: Inconsistent indentation inside the finally block.

The finally body (lines 22–25) and its closing brace (line 27) are indented deeper than the surrounding method body. The opening } finally { is at 8 spaces but the body is at 16 and the closing } is at 12. Align them consistently.

♻️ Proposed fix

         try {
             chain.doFilter(req, res);
         } finally {
-
-                res.setHeader("X-Content-Type-Options", "nosniff");
-                res.setHeader("X-Frame-Options", "DENY");
-                res.setHeader("X-XSS-Protection", "0");
-                res.setHeader("Referrer-Policy", "no-referrer");
-
-            }
-        }
-    }
+            res.setHeader("X-Content-Type-Options", "nosniff");
+            res.setHeader("X-Frame-Options", "DENY");
+            res.setHeader("X-XSS-Protection", "0");
+            res.setHeader("Referrer-Policy", "no-referrer");
+        }
+    }
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java` around lines 20 -
27, The finally block in SecurityHeadersFilter (within the
doFilter/doFilterInternal method) has inconsistent indentation; align the
opening "finally {", its body lines that call res.setHeader(...), and the
closing "}" to the same indentation level as the surrounding method body so the
finally block and its contents use consistent spacing/indentation with the rest
of the method.

src/main/java/org/juv25d/plugin/MetricPlugin.java (1)

45-50: escapeJson does not escape JSON control characters.

The method only handles \ and ", but valid JSON requires escaping \n, \r, \t, and other control characters (U+0000–U+001F). If build.properties values ever contain newlines (e.g., multi-line CI-generated metadata), the JSON response will be malformed and unparseable by clients.

Consider either completing the escaping or replacing manual JSON construction with a standard library (org.json, Jackson, Gson):

♻️ Minimal fix – complete control-char escaping

 private String escapeJson(String value) {
     if (value == null) {
         return "";
     }
-    return value.replace("\\", "\\\\").replace("\"", "\\\"");
+    return value
+        .replace("\\", "\\\\")
+        .replace("\"", "\\\"")
+        .replace("\n", "\\n")
+        .replace("\r", "\\r")
+        .replace("\t", "\\t");
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/plugin/MetricPlugin.java` around lines 45 - 50, The
escapeJson method in MetricPlugin currently only escapes backslashes and quotes
and therefore fails for control chars (newline, carriage return, tab and
U+0000–U+001F), which can break JSON; fix it by either (preferred) replacing the
manual JSON assembly that uses escapeJson with a standard JSON serializer (e.g.,
Jackson ObjectMapper or Gson) to serialize the data structure safely, or
(minimal) extend the escapeJson(String value) implementation to also escape
control characters (\b, \f, \n, \r, \t) and to emit \uXXXX for any character
with codepoint < 0x20; update usages in MetricPlugin to use the new serializer
or updated escape behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@build.properties`:
- Around line 1-3: The root build.properties file contains hardcoded build
metadata (build.commit, build.version, build.time) that will become stale;
remove this committed file and add build.properties to .gitignore, or if it must
remain as a deliberate local stub, replace its contents with Maven placeholders
and document its intent; reference the existing
src/main/resources-filtered/build.properties as the canonical template and
ensure only that templated file is committed while the root build.properties is
ignored.

In `@pom.xml`:
- Around line 57-68: The two Testcontainers dependencies use mismatched major
versions (artifactId testcontainers version 2.0.3 vs junit-jupiter version
1.21.4) causing runtime API incompatibilities; update the junit-jupiter
dependency to the same 2.0.3 version (or align both to a single chosen
Testcontainers version) or instead add the org.testcontainers:testcontainers-bom
with the chosen version (e.g., 2.0.3) in dependencyManagement and remove
explicit versions to keep all org.testcontainers modules in sync (adjust
artifactId entries testcontainers and junit-jupiter accordingly).

In `@src/main/java/org/juv25d/App.java`:
- Line 8: The import statement for NotFoundPlugin in App.java contains a stale
inline comment "// New import"; remove that trailing comment so the line reads
simply "import org.juv25d.plugin.NotFoundPlugin;" to clean up committed code and
avoid leftover developer markers.

In `@src/main/java/org/juv25d/filter/LoggingFilter.java`:
- Line 14: Replace the direct System.out.println call in LoggingFilter with the
project's logger: obtain the logger via ServerLogging.getLogger() inside the
LoggingFilter and log the request method and path (e.g., using info or fine
depending on existing filter log levels) instead of printing to stdout; update
the statement that currently uses req.method() + " " + req.path() to call
logger.<appropriateLevel>(...) so logging goes through java.util.logging
handlers, formatters and respects configured levels.

In `@src/main/java/org/juv25d/filter/TimingFilter.java`:
- Around line 14-23: The timing log in TimingFilter.doFilter is skipped if
chain.doFilter(req, res) throws an IOException; wrap the call and the timing
calculation/logging in a try-finally so the duration is computed and
logger.info(...) is invoked in the finally block; specifically, record start =
System.nanoTime() before calling chain.doFilter(req, res), then in a finally
compute durationMs and call logger.info(req.method() + " " + req.path() + " took
" + durationMs + " ms") to ensure the log always runs even when an exception is
thrown.

In `@src/main/java/org/juv25d/http/HttpRequest.java`:
- Around line 5-17: HttpRequest stores mutable references for headers and body;
add a compact canonical constructor to defensively copy them: in the HttpRequest
record add a canonical/compact constructor that replaces the incoming headers
with an unmodifiable copy (e.g., new LinkedHashMap<>(headers) wrapped or
Map.copyOf/Collections.unmodifiableMap) and replaces the incoming body with a
cloned byte[] (body == null ? null/empty : body.clone()), keeping
creationTimeNanos unchanged; reference the HttpRequest record, the headers and
body components, and the existing one-arg constructor when implementing this fix
and add the needed imports (java.util.Map, java.util.Collections or
java.util.LinkedHashMap).

In `@src/main/resources/static/css/styles.css`:
- Around line 163-167: The Stylelint complaint is due to missing whitespace
inside the inline CSS comment in the &::before rule; fix it by either removing
the commented line entirely if it's dead code or by adding the required spaces
so the comment reads with a space after /* and before */, e.g. update the
comment in the &::before selector (the commented backdrop-filter line) to use
proper spacing or delete the commented-out backdrop-filter line and any other
unused commented blocks.

In `@src/main/resources/static/js/metric.js`:
- Around line 27-29: The catch block only sets health-status to "DOWN" but
leaves other DOM fields stale; update the catch block to also clear or set safe
placeholders for the other elements (local-time, utc-time, health-version,
health-commit, health-response, health-memory) so they no longer show old data
when the request fails—locate the catch handling in metric.js and set each of
those element's textContent to a neutral value (e.g. "-" or "N/A") alongside
setting health-status to "DOWN".

In `@src/test/java/org/juv25d/http/HttpResponseTest.java`:
- Around line 17-40: Add positive “roundtrip” tests that exercise the mutable
API described in the PR: call response.setStatusCode(...) and assert
response.statusCode() returns the value; call response.setStatusText(...) with a
non-null string and assert response.statusText() returns it; mutate headers via
response.headers().put(...) (or response.setHeader if available) and assert
headers contain expected entries; and set body bytes via the
response.setBody(...) or response.body(byte[]) setter and assert response.body()
returns the same byte array. Ensure tests still include the existing null-guard
test for setStatusText(null) and use the same response test fixture (response)
in each new test.

In `@src/test/java/org/juv25d/util/ConfigLoaderTest.java`:
- Around line 52-68: The test usesDefaultsWhenServerKeysMissing() supplies a
YAML with an empty server block rather than omitting the server key, so rename
or split the test and add a new variant to cover the truly-missing-case: keep or
rename the current test (e.g., usesDefaultsWhenServerBlockEmpty) to assert
defaults when server: {} is present, and add a new test (e.g.,
usesDefaultsWhenServerSectionMissing) that constructs a YAML string that does
not include the server top-level key at all and then asserts
ConfigLoader.getPort(), getRootDirectory(), getLogLevel(), and
isRateLimitingEnabled() return the same defaults; reference the ConfigLoader
constructor used in the test to create the ByteArrayInputStream from the YAML
and reuse the same assertEquals/assertFalse checks.

---

Nitpick comments:
In `@pom.xml`:
- Line 21: The pom's JUnit property is out of date: update the Maven property
junit.jupiter.version from 6.0.2 to 6.0.3 (the XML element
<junit.jupiter.version>) so the build uses JUnit 6.0.3 which contains the
deadlock bugfix; after changing the property, run a quick mvn -q test to verify
dependencies resolve and tests run.
- Around line 229-245: Remove the redundant <pluginManagement> entry that
declares the org.codehaus.mojo:buildnumber-maven-plugin; the plugin is already
fully configured under <build><plugins> (the buildnumber-maven-plugin
configuration with all three properties), so delete the <pluginManagement> block
containing that plugin declaration to avoid duplicate/incomplete declarations
and confusion.

In `@src/main/java/org/juv25d/App.java`:
- Around line 34-40: Replace the hardcoded List.of(...) redirect rules in main()
with a configuration-driven load via ConfigLoader: add a redirectRules property
in application-properties.yml, update ConfigLoader to parse that property into
List<RedirectRule> (or provide a getRedirectRules() method), then call
pipeline.addGlobalFilter(new RedirectFilter(redirectRules), 4) using the loaded
list; locate the current construction of RedirectRule and the
pipeline.addGlobalFilter(...) call in main() to remove the inline entries and
wire the ConfigLoader-provided list instead.
- Around line 46-47: The duplicate StaticFilesPlugin registration is redundant:
SimpleRouter.resolve will match the "/*" longest-prefix wildcard for "/" too,
and registering both creates two plugin instances; remove the
router.registerPlugin("/", new StaticFilesPlugin()) call, instantiate a single
StaticFilesPlugin and register it only for "/*" via router.registerPlugin("/*",
staticFilesPlugin) (referencing router.registerPlugin and StaticFilesPlugin and
SimpleRouter.resolve to locate the code).

In `@src/main/java/org/juv25d/filter/IpFilter.java`:
- Around line 42-44: The getClientIp method is a trivial one-liner that just
returns req.remoteIp(); remove the private method getClientIp(HttpRequest req)
and replace all its call sites with direct calls to req.remoteIp(), updating
imports/usages if any; ensure no references remain to getClientIp so the class
compiles after deleting the method.

In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java`:
- Around line 20-27: The finally block in SecurityHeadersFilter (within the
doFilter/doFilterInternal method) has inconsistent indentation; align the
opening "finally {", its body lines that call res.setHeader(...), and the
closing "}" to the same indentation level as the surrounding method body so the
finally block and its contents use consistent spacing/indentation with the rest
of the method.

In `@src/main/java/org/juv25d/filter/TimingFilter.java`:
- Line 21: Replace the string-concatenation log in TimingFilter with
parameterized JUL logging: import java.util.logging.Level and change the
logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms")
usage to logger.log(Level.INFO, "...", new Object[]{req.method(), req.path(),
durationMs}) so the message parameters (req.method(), req.path(), durationMs)
are only formatted when INFO is enabled; update the TimingFilter code where
logger is used accordingly.

In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java`:
- Around line 19-31: Remove the redundant res.setStatusCode(200) call in
HealthCheckPlugin (HttpResponse defaults to 200) and fix the JSON text block
assigned to jsonBody so its closing brace aligns with the opening brace (make
the block consistently indented as {"status":"UP"} style), then recompute
bodyBytes from the corrected jsonBody; ensure you still set Content-Type and
Content-Length on res after updating jsonBody and bodyBytes.

In `@src/main/java/org/juv25d/plugin/MetricPlugin.java`:
- Around line 45-50: The escapeJson method in MetricPlugin currently only
escapes backslashes and quotes and therefore fails for control chars (newline,
carriage return, tab and U+0000–U+001F), which can break JSON; fix it by either
(preferred) replacing the manual JSON assembly that uses escapeJson with a
standard JSON serializer (e.g., Jackson ObjectMapper or Gson) to serialize the
data structure safely, or (minimal) extend the escapeJson(String value)
implementation to also escape control characters (\b, \f, \n, \r, \t) and to
emit \uXXXX for any character with codepoint < 0x20; update usages in
MetricPlugin to use the new serializer or updated escape behavior.

In `@src/main/resources/static/css/styles.css`:
- Around line 1-8: The CSS custom properties (--base-color, --base-color-white)
are declared on the universal selector '*' which sets them on every element;
move those declarations to the ':root' selector so they act as global variables
and avoid per-element redeclaration—update the stylesheet by removing the
custom-property lines from the '*' rule and adding them under a ':root' rule
(keep other rules in the '*' block unchanged) so references to --base-color and
--base-color-white continue to work site-wide.
- Around line 273-292: There are two consecutive &::after rule blocks that
conflict; merge them into a single &::after block so properties from the first
(top, left, right, bottom, border-radius, position-anchor, transition) and the
second (background-color, z-index) are defined together. Update the single
&::after rule to include background-color: rgba(0,150,255,0.3) and z-index: -1,
ensure the transition declaration is syntactically valid (add missing semicolon
and correct timing-function syntax), and remove the duplicate block so all
styles for the pseudo-element live in one place.
- Around line 189-211: Remove the dead/commented CSS blocks to reduce noise:
delete the entire commented block containing the &:has(a:active)::before /
&:has(a:active)::after rules and the &:has(a:active) > ul > li * rule, and also
remove the old commented .features li block; these selectors (
&:has(a:active)::before, &:has(a:active) > ul > li *, and .features li )
identify the commented sections to remove so the stylesheet contains only active
rules and the history remains in version control.

In `@src/test/java/org/juv25d/filter/IpFilterTest.java`:
- Around line 10-44: Add a new unit test that verifies blacklist behavior:
create an IpFilter with the whitelist parameter null and the blacklist set to
Set.of("10.0.0.1"), mock an HttpRequest to return remoteIp() = "10.0.0.1",
instantiate HttpResponse and a mocked FilterChain, call filter.doFilter(req,
res, chain), verify that chain.doFilter(req, res) is never called, and assert
the response statusCode() is 403 (and optionally statusText() equals
"Forbidden"); place this alongside the existing whitelist tests and name it e.g.
blacklist_blocksMatchedIp to mirror the style of whitelist_allowsIp and
whitelist_blocksIpNotInList.

In `@src/test/java/org/juv25d/http/HttpResponseTest.java`:
- Around line 1-7: Update the project's junit-jupiter dependency version from
6.0.2 to 6.0.3 in the Maven POM where the junit-jupiter artifact/version is
declared (look for dependency elements with artifactId "junit-jupiter" or
related junit-jupiter modules such as "junit-jupiter-engine" /
"junit-jupiter-api"), change the <version> value to 6.0.3, then run a quick mvn
test to ensure the new release integrates cleanly.

In `@src/test/java/org/juv25d/plugin/MetricPluginTest.java`:
- Line 30: The test's fragile assertion in MetricPluginTest that checks
body.contains("\"server\": \"juv25d-webserver\"") can fail due to JSON
whitespace differences; update the test to either parse the JSON string in the
body (e.g., into a Map/JsonNode) and assert that the "server" key exists and
equals "juv25d-webserver", or change the assertion to check for the key and
value separately (e.g., assertTrue(body.contains("\"server\"") and
assertTrue(body.contains("juv25d-webserver"))). Locate the failing assertion
around the assertTrue call referencing body in MetricPluginTest and replace it
with a parsed-JSON equality check or decoupled key/value checks so formatting
differences no longer cause false negatives.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69fafc6 and 04addf4.

📒 Files selected for processing (22)

• build.properties
• pom.xml
• src/main/java/org/juv25d/App.java
• src/main/java/org/juv25d/filter/IpFilter.java
• src/main/java/org/juv25d/filter/LoggingFilter.java
• src/main/java/org/juv25d/filter/RateLimitingFilter.java
• src/main/java/org/juv25d/filter/RedirectFilter.java
• src/main/java/org/juv25d/filter/SecurityHeadersFilter.java
• src/main/java/org/juv25d/filter/TimingFilter.java
• src/main/java/org/juv25d/http/HttpRequest.java
• src/main/java/org/juv25d/http/HttpResponse.java
• src/main/java/org/juv25d/plugin/HealthCheckPlugin.java
• src/main/java/org/juv25d/plugin/MetricPlugin.java
• src/main/java/org/juv25d/util/ConfigLoader.java
• src/main/resources-filtered/build.properties
• src/main/resources/static/css/styles.css
• src/main/resources/static/index.html
• src/main/resources/static/js/metric.js
• src/test/java/org/juv25d/filter/IpFilterTest.java
• src/test/java/org/juv25d/http/HttpResponseTest.java
• src/test/java/org/juv25d/plugin/MetricPluginTest.java
• src/test/java/org/juv25d/util/ConfigLoaderTest.java

✅ Files skipped from review due to trivial changes (1)

• src/main/resources-filtered/build.properties

🚧 Files skipped from review as they are similar to previous changes (5)

• src/main/java/org/juv25d/filter/RateLimitingFilter.java
• src/main/java/org/juv25d/util/ConfigLoader.java
• src/main/java/org/juv25d/filter/RedirectFilter.java
• src/main/java/org/juv25d/http/HttpResponse.java
• src/main/resources/static/index.html

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Hardcoded build metadata should not be committed to VCS.

This file captures a specific build snapshot (build.commit=1c8d31d, build.time=2026-02-20T22:14:43Z). It will be stale immediately after the next commit. The src/main/resources-filtered/build.properties with Maven property placeholders is the correct approach and already exists in the project. This root-level file should either be deleted and added to .gitignore, or confirmed as a deliberate development stub.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@build.properties` around lines 1 - 3, The root build.properties file contains
hardcoded build metadata (build.commit, build.version, build.time) that will
become stale; remove this committed file and add build.properties to .gitignore,
or if it must remain as a deliberate local stub, replace its contents with Maven
placeholders and document its intent; reference the existing
src/main/resources-filtered/build.properties as the canonical template and
ensure only that templated file is committed while the root build.properties is
ignored.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Version mismatch within the org.testcontainers group.

testcontainers:2.0.3 is from the Testcontainers 2.x major release line, while junit-jupiter:1.21.4 is from the 1.x line. These two modules must use the same version because the 2.x series introduced breaking API changes relative to 1.x. Using mismatched major versions will likely produce ClassNotFoundException or NoSuchMethodError at test runtime.

🐛 Proposed fix: align both testcontainers modules to the same version

         <dependency>
             <groupId>org.testcontainers</groupId>
             <artifactId>testcontainers</artifactId>
             <version>2.0.3</version>
             <scope>test</scope>
         </dependency>
         <dependency>
             <groupId>org.testcontainers</groupId>
             <artifactId>junit-jupiter</artifactId>
-            <version>1.21.4</version>
+            <version>2.0.3</version>
             <scope>test</scope>
         </dependency>

Alternatively, prefer the BOM to keep all testcontainers modules in sync automatically:

<dependencyManagement>
    <dependencies>
        <dependency>
            <groupId>org.testcontainers</groupId>
            <artifactId>testcontainers-bom</artifactId>
            <version>2.0.3</version>
            <type>pom</type>
            <scope>import</scope>
        </dependency>
    </dependencies>
</dependencyManagement>

📝 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.

Suggested change

	<dependency>
	<groupId>org.testcontainers</groupId>
	<artifactId>testcontainers</artifactId>
	<version>2.0.3</version>
	<scope>test</scope>
	</dependency>
	<dependency>
	<groupId>org.testcontainers</groupId>
	<artifactId>junit-jupiter</artifactId>
	<version>1.21.4</version>
	<scope>test</scope>
	</dependency>
	<dependency>
	<groupId>org.testcontainers</groupId>
	<artifactId>testcontainers</artifactId>
	<version>2.0.3</version>
	<scope>test</scope>
	</dependency>
	<dependency>
	<groupId>org.testcontainers</groupId>
	<artifactId>junit-jupiter</artifactId>
	<version>2.0.3</version>
	<scope>test</scope>
	</dependency>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pom.xml` around lines 57 - 68, The two Testcontainers dependencies use
mismatched major versions (artifactId testcontainers version 2.0.3 vs
junit-jupiter version 1.21.4) causing runtime API incompatibilities; update the
junit-jupiter dependency to the same 2.0.3 version (or align both to a single
chosen Testcontainers version) or instead add the
org.testcontainers:testcontainers-bom with the chosen version (e.g., 2.0.3) in
dependencyManagement and remove explicit versions to keep all org.testcontainers
modules in sync (adjust artifactId entries testcontainers and junit-jupiter
accordingly).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

IOException from chain.doFilter silently drops the timing log — use try-finally.

If the downstream filter throws, execution jumps past the logger.info call and the timing for that request is never recorded. Wrap with try-finally to guarantee the log is emitted regardless of outcome.

🛡️ Proposed fix

     `@Override`
     public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException {
         long start = System.nanoTime();
-
-        chain.doFilter(req, res);
-
-        long durationMs = (System.nanoTime() - start) / 1_000_000;
-        logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
+        try {
+            chain.doFilter(req, res);
+        } finally {
+            long durationMs = (System.nanoTime() - start) / 1_000_000;
+            logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
+        }
     }

📝 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.

Suggested change

	@Override
	public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException {
	long start = System.nanoTime();
	chain.doFilter(req, res);
	long durationMs = (System.nanoTime() - start) / 1_000_000;
	logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
	}
	`@Override`
	public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException {
	long start = System.nanoTime();
	try {
	chain.doFilter(req, res);
	} finally {
	long durationMs = (System.nanoTime() - start) / 1_000_000;
	logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
	}
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/TimingFilter.java` around lines 14 - 23, The
timing log in TimingFilter.doFilter is skipped if chain.doFilter(req, res)
throws an IOException; wrap the call and the timing calculation/logging in a
try-finally so the duration is computed and logger.info(...) is invoked in the
finally block; specifically, record start = System.nanoTime() before calling
chain.doFilter(req, res), then in a finally compute durationMs and call
logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms") to
ensure the log always runs even when an exception is thrown.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Mutable headers map and body array are stored by reference — defensive copies required.

Java records do not automatically clone mutable fields. The auto-generated canonical constructor assigns both Map<String, String> headers and byte[] body by reference, so:

• HttpParser creates a TreeMap (mutable) and hands it straight to the constructor; any code holding that reference (or any code calling req.headers().put(...)) mutates the request visible to all downstream filters/plugins.
• req.body() returns the raw array, allowing req.body()[0] = x to silently corrupt data.

HttpResponse (e.g., this.headers = new LinkedHashMap<>(...) and body.clone()) already applies this protection correctly. Add a compact canonical constructor to bring HttpRequest in line:

🐛 Proposed fix – compact canonical constructor

 public record HttpRequest(
         String method,
         String path,
         String queryString,
         String httpVersion,
         Map<String, String> headers,
         byte[] body,
         String remoteIp,
         long creationTimeNanos
 ) {
+    // Compact canonical constructor – enforce immutability
+    public HttpRequest {
+        headers = headers != null
+            ? Collections.unmodifiableMap(new java.util.LinkedHashMap<>(headers))
+            : Map.of();
+        body = body != null ? body.clone() : new byte[0];
+    }
+
     public HttpRequest(String method, String path, String queryString, String httpVersion, Map<String, String> headers, byte[] body, String remoteIp) {
         this(method, path, queryString, httpVersion, headers, body, remoteIp, System.nanoTime());
     }
 }

You will also need to import java.util.Collections and java.util.Map.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/http/HttpRequest.java` around lines 5 - 17,
HttpRequest stores mutable references for headers and body; add a compact
canonical constructor to defensively copy them: in the HttpRequest record add a
canonical/compact constructor that replaces the incoming headers with an
unmodifiable copy (e.g., new LinkedHashMap<>(headers) wrapped or
Map.copyOf/Collections.unmodifiableMap) and replaces the incoming body with a
cloned byte[] (body == null ? null/empty : body.clone()), keeping
creationTimeNanos unchanged; reference the HttpRequest record, the headers and
body components, and the existing one-arg constructor when implementing this fix
and add the needed imports (java.util.Map, java.util.Collections or
java.util.LinkedHashMap).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Stylelint: missing whitespace inside comment.

Stylelint flags the inline comment on line 166. The comment /*backdrop-filter: blur(2px);*/ is missing the required spaces after /* and before */.

Proposed fix

-        /*backdrop-filter: blur(2px);*/
+        /* backdrop-filter: blur(2px); */

But if this is dead code, consider removing it entirely along with the other commented-out blocks.

📝 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.

Suggested change

	&::before {
	z-index: -1;
	background: rgba(0, 0, 0, 0.2);
	/*backdrop-filter: blur(2px);*/
	}
	&::before {
	z-index: -1;
	background: rgba(0, 0, 0, 0.2);
	/* backdrop-filter: blur(2px); */
	}

🧰 Tools

🪛 Stylelint (17.3.0)

[error] 166-166: Expected whitespace after "/*" (comment-whitespace-inside)

(comment-whitespace-inside)

---

[error] 166-166: Expected whitespace before "*/" (comment-whitespace-inside)

(comment-whitespace-inside)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/css/styles.css` around lines 163 - 167, The
Stylelint complaint is due to missing whitespace inside the inline CSS comment
in the &::before rule; fix it by either removing the commented line entirely if
it's dead code or by adding the required spaces so the comment reads with a
space after /* and before */, e.g. update the comment in the &::before selector
(the commented backdrop-filter line) to use proper spacing or delete the
commented-out backdrop-filter line and any other unused commented blocks.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Stale DOM fields when the request fails.

The catch block only updates health-status to "DOWN" while all other elements (local-time, utc-time, health-version, health-commit, health-response, health-memory) continue to display values from the last successful poll. This can be misleading — the UI will show "DOWN" status alongside seemingly valid timestamps and memory metrics.

🛡️ Proposed fix: clear stale fields on error

     } catch (e) {
+        const staleDomIds = ["local-time", "utc-time", "health-version",
+            "health-commit", "health-response", "health-memory"];
+        staleDomIds.forEach(id => {
+            const el = document.getElementById(id);
+            if (el) el.textContent = "–";
+        });
         document.getElementById("health-status").textContent = "DOWN";
     }

📝 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.

Suggested change

	} catch (e) {
	document.getElementById("health-status").textContent = "DOWN";
	}
	} catch (e) {
	const staleDomIds = ["local-time", "utc-time", "health-version",
	"health-commit", "health-response", "health-memory"];
	staleDomIds.forEach(id => {
	const el = document.getElementById(id);
	if (el) el.textContent = "–";
	});
	document.getElementById("health-status").textContent = "DOWN";
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/static/js/metric.js` around lines 27 - 29, The catch block
only sets health-status to "DOWN" but leaves other DOM fields stale; update the
catch block to also clear or set safe placeholders for the other elements
(local-time, utc-time, health-version, health-commit, health-response,
health-memory) so they no longer show old data when the request fails—locate the
catch handling in metric.js and set each of those element's textContent to a
neutral value (e.g. "-" or "N/A") alongside setting health-status to "DOWN".

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Setter/roundtrip tests described in the PR are absent.

The PR description explicitly states the tests cover "Default and set values for statusCode and statusText", "setting/retrieving headers", and "setting/retrieving body content" — but the file only exercises default/null-guard paths. Tests for the mutable API (all four setters) are entirely missing, leaving the advertised positive setter behavior untested.

🧪 Proposed additional tests

+    `@Test`
+    void shouldReturnSetStatusCode() {
+        response.setStatusCode(200);
+        assertEquals(200, response.statusCode());
+    }
+
+    `@Test`
+    void shouldReturnSetStatusText() {
+        response.setStatusText("OK");
+        assertEquals("OK", response.statusText());
+    }
+
+    `@Test`
+    void shouldSetAndRetrieveHeader() {
+        response.setHeader("Content-Type", "text/plain");
+        assertEquals("text/plain", response.headers().get("Content-Type"));
+    }
+
+    `@Test`
+    void shouldSetAndRetrieveBody() {
+        byte[] body = "hello".getBytes();
+        response.setBody(body);
+        assertArrayEquals(body, response.body());
+    }

📝 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.

Suggested change

	@Test
	void shouldReturnDefaultStatusCode() {
	assertEquals(0, response.statusCode());
	}
	@Test
	void shouldReturnDefaultText() {
	assertEquals("", response.statusText());
	}
	@Test
	void shouldThrowExceptionWhenStatusTextIsNull() {
	assertThrows(NullPointerException.class, () -> response.setStatusText(null));
	}
	@Test
	void shouldHaveEmptyHeaderByDefault() {
	assertTrue(response.headers().isEmpty());
	}
	@Test
	void shouldHaveEmptyBodyByDefault() {
	assertArrayEquals(new byte[0], response.body());
	}
	`@Test`
	void shouldReturnDefaultStatusCode() {
	assertEquals(0, response.statusCode());
	}
	`@Test`
	void shouldReturnDefaultText() {
	assertEquals("", response.statusText());
	}
	`@Test`
	void shouldThrowExceptionWhenStatusTextIsNull() {
	assertThrows(NullPointerException.class, () -> response.setStatusText(null));
	}
	`@Test`
	void shouldHaveEmptyHeaderByDefault() {
	assertTrue(response.headers().isEmpty());
	}
	`@Test`
	void shouldHaveEmptyBodyByDefault() {
	assertArrayEquals(new byte[0], response.body());
	}
	`@Test`
	void shouldReturnSetStatusCode() {
	response.setStatusCode(200);
	assertEquals(200, response.statusCode());
	}
	`@Test`
	void shouldReturnSetStatusText() {
	response.setStatusText("OK");
	assertEquals("OK", response.statusText());
	}
	`@Test`
	void shouldSetAndRetrieveHeader() {
	response.setHeader("Content-Type", "text/plain");
	assertEquals("text/plain", response.headers().get("Content-Type"));
	}
	`@Test`
	void shouldSetAndRetrieveBody() {
	byte[] body = "hello".getBytes();
	response.setBody(body);
	assertArrayEquals(body, response.body());
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/http/HttpResponseTest.java` around lines 17 - 40,
Add positive “roundtrip” tests that exercise the mutable API described in the
PR: call response.setStatusCode(...) and assert response.statusCode() returns
the value; call response.setStatusText(...) with a non-null string and assert
response.statusText() returns it; mutate headers via response.headers().put(...)
(or response.setHeader if available) and assert headers contain expected
entries; and set body bytes via the response.setBody(...) or
response.body(byte[]) setter and assert response.body() returns the same byte
array. Ensure tests still include the existing null-guard test for
setStatusText(null) and use the same response test fixture (response) in each
new test.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test name/doc doesn't match actual input — consider adding a missing-key variant.

The method is named usesDefaultsWhenServerKeysMissing and the Javadoc says "YAML input omits server configuration keys", but the YAML provided contains all three top-level keys (server, logging, rate-limiting) as empty objects {}. The implementation may behave differently when a key is truly absent (e.g., a YAML document with no server key at all). Consider adding a companion test with a completely absent server block to confirm defaulting behavior in both cases.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/util/ConfigLoaderTest.java` around lines 52 - 68,
The test usesDefaultsWhenServerKeysMissing() supplies a YAML with an empty
server block rather than omitting the server key, so rename or split the test
and add a new variant to cover the truly-missing-case: keep or rename the
current test (e.g., usesDefaultsWhenServerBlockEmpty) to assert defaults when
server: {} is present, and add a new test (e.g.,
usesDefaultsWhenServerSectionMissing) that constructs a YAML string that does
not include the server top-level key at all and then asserts
ConfigLoader.getPort(), getRootDirectory(), getLogLevel(), and
isRateLimitingEnabled() return the same defaults; reference the ConfigLoader
constructor used in the test to create the ByteArrayInputStream from the YAML
and reuse the same assertEquals/assertFalse checks.

[coderabbitai]

Actionable comments posted: 15

♻️ Duplicate comments (7)

src/main/java/org/juv25d/handler/StaticFileHandler.java (1)

224-255: ⚠️ Potential issue | 🔴 Critical

Reflected XSS in 404 body — escape the path before rendering.

Line 247 interpolates the raw request path into HTML. A crafted path can inject markup/script. HTML‑encode the path before formatting the template.

🛡️ Suggested fix

+    private static String htmlEncode(String input) {
+        return input
+            .replace("&", "&")
+            .replace("<", "&lt;")
+            .replace(">", "&gt;")
+            .replace("\"", "&quot;")
+            .replace("'", "&#x27;");
+    }
+
     private static HttpResponse createNotFoundResponse(String path) {
         String html = """
                 <!DOCTYPE html>
                 <html>
                 <head>
@@
                     <h1>404 - Not Found</h1>
-                    <p>The requested resource <code>%s</code> was not found on this server.</p>
+                    <p>The requested resource <code>%s</code> was not found on this server.</p>
                 </body>
                 </html>
-                """.formatted(path);
+                """.formatted(htmlEncode(path));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 224 -
255, The createNotFoundResponse(String path) method interpolates the raw path
into the HTML template causing reflected XSS; escape/HTML-encode the path before
calling formatted(path) (or add and call a dedicated helper like escapeHtml or
HtmlUtils.encode) so characters like &, <, >, " and ' are converted to their
HTML entities, then use the encoded value in the template and return the
response as before; update createNotFoundResponse to perform this encoding and
reuse an existing HTML-escape utility if available.

src/test/java/org/juv25d/http/HttpResponseTest.java (1)

17-40: ⚠️ Potential issue | 🟡 Minor

Add positive round‑trip tests for the mutators.

The suite still only covers defaults and the null guard; it doesn’t verify that setStatusCode, setStatusText, setHeader, and setBody round‑trip correctly, which is part of the PR objective.

🧪 Suggested tests

     `@Test`
     void shouldHaveEmptyBodyByDefault() {
         assertArrayEquals(new byte[0], response.body());
     }
+
+    `@Test`
+    void shouldReturnSetStatusCode() {
+        response.setStatusCode(201);
+        assertEquals(201, response.statusCode());
+    }
+
+    `@Test`
+    void shouldReturnSetStatusText() {
+        response.setStatusText("Created");
+        assertEquals("Created", response.statusText());
+    }
+
+    `@Test`
+    void shouldSetAndRetrieveHeader() {
+        response.setHeader("Content-Type", "text/plain");
+        assertEquals("text/plain", response.headers().get("Content-Type"));
+    }
+
+    `@Test`
+    void shouldSetAndRetrieveBody() {
+        byte[] body = "hello".getBytes();
+        response.setBody(body);
+        assertArrayEquals(body, response.body());
+    }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/http/HttpResponseTest.java` around lines 17 - 40,
Tests only assert defaults and null guard; add positive round‑trip tests in
HttpResponseTest to verify mutators work: create or reuse the response instance
and assert that setStatusCode(int)/statusCode(),
setStatusText(String)/statusText(), setHeader(String,String)/headers().get(...),
and setBody(byte[])/body() return the values set (and that headers contain the
expected key/value and body equals the byte array). Use
assertEquals/assertArrayEquals/assertTrue as appropriate and include a simple
overwrite case for headers to ensure replacement/append behavior is correct.

Dockerfile (1)

12-19: ⚠️ Potential issue | 🟠 Major

Run the runtime container as a non‑root user.

The runtime stage has no USER, so the JVM runs as root. Add a dedicated user/group and switch to it before ENTRYPOINT.

🔒 Suggested change

 FROM eclipse-temurin:25-jre-alpine

 WORKDIR /app

+RUN addgroup -S appgroup && adduser -S appuser -G appgroup
 COPY --from=build /app/target/deps/     libs/
 COPY --from=build /app/target/classes/  classes/
+RUN chown -R appuser:appgroup /app
+USER appuser

 ENTRYPOINT ["java", "-cp", "classes:libs/*","org.juv25d.App"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 12 - 19, The Dockerfile runtime stage currently runs
the JVM as root (ENTRYPOINT ["java", ...]) which is a security risk; add
creation of a dedicated non-root user/group in the runtime image and switch to
it before ENTRYPOINT (create a user and group using Alpine package utilities or
system commands, chown the classes/ and libs/ directories to that user, and add
a USER <name> line so the JVM invoked by ENTRYPOINT runs as the non-root user).

src/main/java/org/juv25d/filter/TimingFilter.java (1)

16-23: ⚠️ Potential issue | 🟠 Major

Ensure timing log always emits (use try/finally).

If chain.doFilter throws, the duration log is skipped. Wrap with try/finally to guarantee logging.

🛠️ Suggested fix

     public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException {
         long start = System.nanoTime();

-        chain.doFilter(req, res);
-
-        long durationMs = (System.nanoTime() - start) / 1_000_000;
-        logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
+        try {
+            chain.doFilter(req, res);
+        } finally {
+            long durationMs = (System.nanoTime() - start) / 1_000_000;
+            logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
+        }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/TimingFilter.java` around lines 16 - 23, The
timing log in TimingFilter.doFilter is skipped if chain.doFilter throws; wrap
the invocation of chain.doFilter(req, res) with a try/finally so the duration
calculation and logger.info(req.method() + " " + req.path() + " took " +
durationMs + " ms") always runs; compute start before the try, compute duration
and call logger.info in the finally block (using the same req, res, start
variables) to guarantee emission even on exceptions.

src/main/java/org/juv25d/router/RouterConfig.java (1)

3-29: Route config should use the project logger (not stdout).

This should go through the configured logging pipeline to respect levels/formatters.

✅ Suggested fix

 import org.juv25d.plugin.StaticFilesPlugin;
 import org.juv25d.proxy.ProxyPlugin;
 import org.juv25d.proxy.ProxyRoute;
 import org.juv25d.util.ConfigLoader;
+import org.juv25d.logging.ServerLogging;
+
+import java.util.logging.Logger;
 
 public class RouterConfig {
+    private static final Logger logger = ServerLogging.getLogger();
 
     `@Inject`
     public RouterConfig(SimpleRouter router) {
@@
-        System.out.println("Router configured");
+        logger.info("Router configured");
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/router/RouterConfig.java` around lines 3 - 29,
Replace the System.out.println call in the RouterConfig constructor with the
project logger: create or obtain a Logger for RouterConfig (e.g., via your
logging framework's factory) and call logger.info(...) or appropriate level
instead of printing to stdout; remove the System.out.println("Router
configured") line and ensure the Logger is imported and used consistently in
RouterConfig.

src/main/java/org/juv25d/filter/RateLimitingFilter.java (2)

28-29: ⚠️ Potential issue | 🟠 Major

Unbounded per‑IP bucket map can grow without limit.

buckets never evicts entries, so a large or malicious client population can grow memory without bound. Consider a size/TTL eviction strategy for per‑IP buckets.

Also applies to: 98-99

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` around lines 28 -
29, The current ConcurrentHashMap field buckets in RateLimitingFilter is
unbounded and can grow indefinitely; replace it with a bounded/TTL cache (e.g.,
Caffeine or Guava Cache) to enforce maximumSize and
expireAfterAccess/expireAfterWrite eviction. Update all accesses that use
buckets (creation via computeIfAbsent or direct get/put) to use the cache API
(cache.get(key, Callable) or cache.getIfPresent + cache.put) so per‑IP Bucket
entries are created on demand and evicted automatically; configure sensible max
size and TTL and ensure thread-safe bucket retrieval in the same methods that
currently reference buckets (the buckets field and the code around lines noted
in the review).

---

112-116: ⚠️ Potential issue | 🔴 Critical

Update Bucket4j limit construction to the 8.x builder API.

Bandwidth.classic(...) and Refill.intervally(...) are deprecated/removed in Bucket4j 8.x. Use the builder DSL to avoid build/runtime breakage.

🔧 Suggested update

-        Bandwidth limit = Bandwidth.classic(
-            capacity,
-            Refill.intervally(refillTokens, refillPeriod));
+        Bandwidth limit = Bandwidth.builder()
+            .capacity(capacity)
+            .refillIntervally(refillTokens, refillPeriod)
+            .build();

#!/bin/bash
# Verify Bucket4j version and usage across the repo
rg -n "bucket4j" -g 'pom.xml'
rg -n --type=java -C2 'Bandwidth\.classic|Refill\.intervally'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` around lines 112 -
116, In createBucket(), replace the deprecated Bandwidth.classic(...) and
Refill.intervally(...) usage with the Bucket4j 8.x builder DSL: construct the
Bandwidth via Bandwidth.builder() (setting capacity and the refill policy) and
use the new Refill factory method from 8.x for interval refills; update the
createBucket() implementation to build the Bandwidth with Bandwidth.builder()
and then create the Bucket from that builder, replacing any references to
Bandwidth.classic and Refill.intervally accordingly (keep the same capacity,
refillTokens and refillPeriod variables).

🧹 Nitpick comments (9)

src/main/java/org/juv25d/http/HttpStatus.java (1)

30-37: Consider a static Map for O(1) reverse lookup.

getStatusFromCode calls HttpStatus.values() on every invocation, which allocates a new array each time and does a linear scan. The idiomatic Java pattern for enum reverse-lookup is a static Map initialised once at class-load time.

♻️ Proposed refactor

+    private static final java.util.Map<Integer, HttpStatus> CODE_MAP;
+    static {
+        CODE_MAP = new java.util.HashMap<>();
+        for (HttpStatus status : HttpStatus.values()) {
+            CODE_MAP.put(status.code, status);
+        }
+    }
+
     public static HttpStatus getStatusFromCode(int code) {
-        for (HttpStatus status : HttpStatus.values()) {
-            if (status.getCode() == code) {
-                return status;
-            }
-        }
-        return UNKNOWN;
+        return CODE_MAP.getOrDefault(code, UNKNOWN);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/http/HttpStatus.java` around lines 30 - 37, Replace
the current linear scan in getStatusFromCode with an O(1) reverse lookup: add a
static final Map<Integer, HttpStatus> (e.g., CODE_MAP) in the HttpStatus enum
and populate it once at class-load time (static initializer or stream collector)
mapping each status.getCode() to the enum, then change getStatusFromCode to
return CODE_MAP.getOrDefault(code, UNKNOWN); keep the UNKNOWN fallback and keep
the enum constants unchanged.

src/main/java/org/juv25d/proxy/ProxyRoute.java (1)

21-24: Normalize slashes and query prefix when building the URL.

Line 22 concatenates strings directly; this can produce // or missing slashes (and ?? if the query already includes ?). Normalizing makes proxy URLs more resilient to config/input variance.

♻️ Suggested change

     public String buildUrl(String targetPath, String query) {
-        String url = upstreamUrl + targetPath;
-        if (query != null && !query.isEmpty()) url += "?" + query;
+        String base = upstreamUrl.endsWith("/") ? upstreamUrl.substring(0, upstreamUrl.length() - 1) : upstreamUrl;
+        String path = (targetPath == null || targetPath.isEmpty())
+                ? ""
+                : (targetPath.startsWith("/") ? targetPath : "/" + targetPath);
+        String url = base + path;
+        if (query != null && !query.isEmpty()) {
+            String q = query.startsWith("?") ? query.substring(1) : query;
+            url += "?" + q;
+        }
 
         return url;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/proxy/ProxyRoute.java` around lines 21 - 24, The
buildUrl method currently concatenates upstreamUrl + targetPath and naively
appends "?" + query, which can produce duplicate or missing slashes and double
question marks; update buildUrl to normalize slashes and the query prefix: trim
any trailing '/' from upstreamUrl and any leading '/' from targetPath (or ensure
exactly one '/' between them) before joining, and when appending query ensure
you prepend a single '?' only if query is non-empty and does not already start
with '?'. Make these changes in the buildUrl method (referencing upstreamUrl,
targetPath, and query) so the resulting URL always has one slash separator and a
single leading '?' for the query.

src/test/java/org/juv25d/PipelineTest.java (1)

1-4: Empty test skeleton provides no coverage.

There are no test methods in this class, so it doesn’t validate any behavior. Consider adding Pipeline tests or removing the stub to avoid a false sense of coverage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/PipelineTest.java` around lines 1 - 4, The
PipelineTest class is an empty JUnit test skeleton and provides no coverage;
either remove the stub or add concrete JUnit 5 test methods that exercise the
Pipeline behavior. Locate the PipelineTest class and either delete it, or add
`@Test` methods that instantiate the Pipeline (the Pipeline constructor), call its
primary methods (e.g., process/execute/run or whatever public method names exist
on Pipeline), assert expected outputs and edge cases (normal input, null/empty
input, and error conditions) using Assertions.assertEquals/assertThrows, and
include setup/teardown or fixtures if Pipeline requires dependencies (mock
collaborators or use test doubles). Ensure each test has a descriptive name and
covers meaningful behavior so the test file no longer remains empty.

src/main/java/org/juv25d/config/FilterConfig.java (1)

7-14: Consider defensive-copy and null-guard in the constructor for future robustness.

Currently the constructor stores the caller's map directly by reference. While existing call sites in FilterScanner always pass fresh HashMap instances, defensive copying protects against future external callers accidentally passing null or mutating the map after construction.

♻️ Suggested improvement

 public FilterConfig(Map<String, String> params) {
-    this.params = params;
+    this.params = params != null ? Map.copyOf(params) : Map.of();
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/config/FilterConfig.java` around lines 7 - 14, The
constructor of FilterConfig currently stores the incoming Map by reference which
risks NPEs or external mutation; update the FilterConfig(Map<String,String>
params) constructor to null-guard and defensively copy the input (e.g., treat
null as an empty map and assign a new HashMap<>(params) or an unmodifiable copy)
so the private final params field is never null and cannot be mutated from
outside; keep the get(String key) behavior unchanged.

src/main/java/org/juv25d/Pipeline.java (1)

16-19: Add null guards for constructor parameters.

Neither matcher nor router is validated. A null sneaking in would produce a confusing NPE deep inside FilterChainImpl.doFilter rather than at construction time.

♻️ Proposed fix

     public Pipeline(FilterMatcher matcher, Router router) {
+        Objects.requireNonNull(matcher, "matcher must not be null");
+        Objects.requireNonNull(router, "router must not be null");
         this.matcher = matcher;
         this.router = router;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/Pipeline.java` around lines 16 - 19, Add null guards
to the Pipeline constructor so matcher and router are validated at construction
time: in the Pipeline(FilterMatcher matcher, Router router) constructor check
that matcher and router are non-null (e.g. via Objects.requireNonNull or
explicit null checks) and throw a clear NPE or IllegalArgumentException if
either is null; this ensures failures surface at Pipeline construction rather
than later in FilterChainImpl.doFilter.

src/main/java/org/juv25d/di/Inject.java (1)

3-7: Consider adding @Documented for API discoverability.

Without @Documented, the @Inject annotation will not appear in generated Javadoc for constructors that bear it, making the DI contract invisible to consumers of the public API.

♻️ Proposed fix

 import java.lang.annotation.*;
 
 `@Retention`(RetentionPolicy.RUNTIME)
 `@Target`(ElementType.CONSTRUCTOR)
+@Documented
 public `@interface` Inject {
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/di/Inject.java` around lines 3 - 7, The Inject
annotation lacks `@Documented` which prevents it appearing in generated Javadoc;
update the org.juv25d.di.Inject annotation to add the
java.lang.annotation.Documented meta-annotation (import Documented if needed) so
the `@Inject` marker is included in Javadoc for constructors while keeping the
existing `@Retention`(RetentionPolicy.RUNTIME) and
`@Target`(ElementType.CONSTRUCTOR).

src/test/java/org/juv25d/filter/FilterRegistryTest.java (1)

67-78: Add a test verifying shutdown() idempotency (calling it twice must not double-destroy).

shouldDestroyFiltersOnlyOnce covers the identity-dedup case within a single shutdown() call, but there is no test asserting that calling shutdown() a second time is a no-op.

✅ Suggested additional test

`@Test`
void shouldBeIdempotentOnDoubleShutdown() {
    FilterRegistry registry = new FilterRegistry();
    TestFilter filter = new TestFilter();
    registry.registerGlobal(filter, 1);

    registry.shutdown();
    registry.shutdown();

    assertEquals(1, filter.destroyCount);
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/filter/FilterRegistryTest.java` around lines 67 -
78, Add a new unit test method (e.g., shouldBeIdempotentOnDoubleShutdown) in
FilterRegistryTest that constructs a FilterRegistry and a TestFilter, registers
the filter via registerGlobal(filter, 1), calls registry.shutdown() twice, and
asserts that TestFilter.destroyCount equals 1; this verifies that
FilterRegistry.shutdown() is idempotent and does not double-destroy the same
filter.

src/main/java/org/juv25d/filter/FilterFactory.java (1)

13-15: Add generics to avoid unsafe casts in FilterFactory.create.

Class<?> permits non-Filter classes and forces a runtime cast. Since Container.get already provides generic type safety, refactor to <T extends Filter> T create(Class<T> clazz) to enforce compile-time type safety and eliminate the unsafe cast.

♻️ Suggested refactor

-    public Filter create(Class<?> clazz) {
-        return (Filter) container.get(clazz);
-    }
+    public <T extends Filter> T create(Class<T> clazz) {
+        return container.get(clazz);
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterFactory.java` around lines 13 - 15,
Change the create method in FilterFactory to use a bounded generic so callers
get compile-time safety: replace the signature public Filter create(Class<?>
clazz) with a generic form like public <T extends Filter> T create(Class<T>
clazz) and return container.get(clazz) without an explicit cast; update
references to FilterFactory.create if necessary. Ensure you use the
Container.get(Class<T>) generic overload so the cast is removed and the compiler
enforces that clazz extends Filter (symbols: FilterFactory.create and
Container.get).

src/main/java/org/juv25d/filter/FilterScanner.java (1)

87-90: Preserve exception details when skipping a filter.

The catch block only logs e.getMessage(), which drops stack traces and makes diagnosing misconfigured filters harder. Consider logging the exception itself.

💡 Minimal change to keep stack traces

-                System.err.println("Skipping filter: "
-                    + filterClass.getName() + " due to " + e.getMessage());
+                System.err.println("Skipping filter: "
+                    + filterClass.getName() + " due to " + e);
+                e.printStackTrace(System.err);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterScanner.java` around lines 87 - 90, In
FilterScanner's catch(Exception e) block where it currently prints "Skipping
filter: " + filterClass.getName() + " due to " + e.getMessage(), preserve full
exception details by including the exception/stack trace in the output instead
of only e.getMessage(); e.g., replace the current System.err print with either a
logging call that accepts the Throwable (logger.log(Level.SEVERE, "...", e)) or
call e.printStackTrace() after the message so the stack trace for filterClass is
available for debugging.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/org/juv25d/di/Container.java`:
- Around line 147-160: The resolve method currently replaces the interface type
with its binding before checking beans, so a bean registered under the interface
key (via register(IFoo.class, instance)) is never found; fix by checking
beans.get(type) first and returning it if present, then apply bindings (type =
bindings.get(type)) and proceed with the interface binding validation and later
existing-bean fast path. Update resolve to: 1) check beans.get(type) and return
if non-null, 2) then if bindings.containsKey(type) set type =
(Class<T>)bindings.get(type), 3) then if type.isInterface() &&
!bindings.containsKey(type) throw, and 4) continue with the rest of resolve;
reference methods/fields: resolve, bindings, beans, register.

In `@src/main/java/org/juv25d/filter/ClassScanner.java`:
- Around line 58-65: The Javadoc for ClassScanner.scanDir incorrectly declares
"@throws ClassNotFoundException" even though the method catches and swallows
ClassNotFoundException internally; remove the "@throws ClassNotFoundException"
tag from the scanDir Javadoc comment so the documentation matches the
implementation (locate the Javadoc above the scanDir method in ClassScanner and
delete the `@throws` line).
- Around line 30-31: The code in ClassScanner.findClasses calls
Thread.currentThread().getContextClassLoader() and immediately uses
cl.getResources(path), which can NPE if the context ClassLoader is null; update
findClasses to check if cl is null and if so fall back to a non-null loader (for
example ClassScanner.class.getClassLoader() or
ClassLoader.getSystemClassLoader()) before calling getResources(path), and use
the resulting 'cl' variable for the subsequent resources lookup to avoid the
NullPointerException.
- Around line 79-83: The ClassScanner currently calls Class.forName(className,
true, cl) which forces static initialization and can throw Error
(ExceptionInInitializerError) that will crash startup; change the call in
ClassScanner where classes.add(Class.forName(className, true, cl)) is invoked to
use Class.forName(className, false, cl) so classes are loaded without running
static initializers (preserving annotation-based scanning and avoiding Error
propagation from static initializers on class load).

In `@src/main/java/org/juv25d/filter/FilterMatcher.java`:
- Around line 36-43: The matches method in FilterMatcher incorrectly handles
patterns like "/api/*" because it strips only the '*' leaving a trailing slash
and thus doesn't match the exact base "/api"; update the
"pattern.endsWith(\"/*\")" branch to compute the basePrefix by removing "/*"
(e.g., pattern.substring(0, pattern.length() - 2)) and then return true if
path.equals(basePrefix) or path starts with basePrefix + "/" so both the exact
base ("/api") and its children ("/api/...", "/api/v1") match.

In `@src/main/java/org/juv25d/filter/FilterRegistration.java`:
- Around line 3-8: The compareTo in FilterRegistration currently only compares
order which returns 0 for ties and can collapse distinct registrations; update
FilterRegistration.compareTo to add deterministic tie-breakers after
Integer.compare(this.order, o.order): first compare the pattern strings
(this.pattern vs o.pattern) using String.compareTo (handling nulls), and if
still equal, compare the filter instances deterministically (e.g., compare
filter.getClass().getName() then fallback to System.identityHashCode(filter) or
Objects.hashCode(filter)); ensure the method never returns 0 for distinct
registrations unless they are truly equal.

In `@src/main/java/org/juv25d/filter/FilterRegistry.java`:
- Around line 30-45: The shutdown() method has a TOCTOU race: concurrent callers
can both see isShutdown false and call destroyFilter() twice; fix by making the
shutdown initiation atomic (replace the volatile boolean check with an
AtomicBoolean and use compareAndSet(false, true) or synchronize the entry) so
only the first caller proceeds; keep the rest of the logic (creating the
destroyed set and iterating global and routes) intact and retain calls to
destroyFilter(FilterRegistration.filter(), destroyed) so subsequent threads
return immediately without iterating.
- Around line 26-28: getRouteFilters currently returns an unmodifiable view of
the map but leaves the inner lists (e.g.,
CopyOnWriteArrayList<FilterRegistration> stored in routes) mutable; change
getRouteFilters to produce a new map that maps each route key to
Collections.unmodifiableList(theList) and then return
Collections.unmodifiableMap(thatNewMap) so callers cannot mutate inner lists;
reference the routes field and the getRouteFilters method and ensure the
returned value uses unmodifiableList wrappers around each
List<FilterRegistration> before wrapping the whole map.

In `@src/main/java/org/juv25d/http/HttpStatus.java`:
- Around line 3-20: The HttpStatus enum is missing several production-critical
codes (notably 204, 304, 405, 429, 503) which causes features like REST
DELETE/PUT, caching, routing, rate-limiting and proxying to fall back to
UNKNOWN; update the HttpStatus enum by adding entries NO_CONTENT(204, "No
Content"), NOT_MODIFIED(304, "Not Modified"), METHOD_NOT_ALLOWED(405, "Method
Not Allowed"), TOO_MANY_REQUESTS(429, "Too Many Requests"), and
SERVICE_UNAVAILABLE(503, "Service Unavailable") alongside the existing constants
(ensure commas/termination match the enum syntax and keep UNKNOWN(-1, "Unknown
Status") intact).

In `@src/main/java/org/juv25d/proxy/ProxyPlugin.java`:
- Around line 81-85: The upstream response loop forwards hop-by-hop headers to
the client; modify the block that iterates
upstreamResponse.headers().map().forEach(...) so it checks
isRestrictedHeader(name) before calling res.setHeader, skipping headers like
"transfer-encoding", "connection", "keep-alive" (i.e., reuse the same
isRestrictedHeader logic used for outgoing requests). Ensure you apply the
filter to the header name (and still handle non-empty values) when setting
headers via res.setHeader to avoid forwarding transport-level headers.
- Around line 38-40: The code in ProxyPlugin (using proxyRoute.getBaseRoute(),
req.path(), and proxyRoute.buildUrl) can throw StringIndexOutOfBoundsException
when req.path() is shorter than baseRoute; fix by checking lengths/startsWith
before calling substring: compute int start = Math.min(req.path().length(),
baseRoute.length()) or if (!req.path().startsWith(baseRoute)) treat targetPath
as "" (or "/" per existing semantics), then call proxyRoute.buildUrl(targetPath,
req.queryString()); ensure you handle null/empty queryString consistently so
buildUrl receives a safe targetPath instead of causing substring to fail.

In `@src/main/resources/application-properties.yml`:
- Around line 6-7: Replace the hardcoded third‑party test URL in the default
config by changing the upstream-url property to a non-functional placeholder or
an environment-variable reference (e.g., use an env var like UPSTREAM_URL with a
sensible empty/default placeholder) so the default config cannot accidentally
proxy traffic to jsonplaceholder; update the upstream-url entry in the YAML (the
upstream-url property near base-route) to reference the environment variable or
placeholder and add a brief comment indicating it must be set for real
deployments.

In `@src/test/java/org/juv25d/filter/LoggingFilterTest.java`:
- Around line 37-58: The test LoggingFilterTest is non-deterministic because the
global logger level (from ServerLogging.getLogger()) may be higher than INFO so
the LoggingFilter's INFO logs are not recorded; fix by saving the current logger
level, set logger.setLevel(java.util.logging.Level.INFO) before calling
filter.doFilter(req, res, chain), run the assertions, and then restore the
original level in the finally block (use the same logger variable obtained via
ServerLogging.getLogger()) so the test does not depend on external log
configuration.

In `@src/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java`:
- Around line 17-46: The test doesn't set an explicit logger level and never
removes the handler, causing message filtering and handler leakage; set the test
logger's level to Level.INFO (call logger.setLevel(Level.INFO)) before logging,
and in the finally block remove the custom Handler (call
logger.removeHandler(handler)) and clear the LogContext as already done;
reference the Logger instance named logger, the Handler variable handler, and
the LogContext.clear() call when applying these changes.

In `@src/test/java/org/juv25d/proxy/ProxyPluginTest.java`:
- Around line 3-12: The IntegrationTests nested class performs real network
calls and should be annotated with a JUnit tag to exclude it from default unit
test runs; add `@Tag`("integration") to the IntegrationTests class declaration and
add the corresponding import (org.junit.jupiter.api.Tag) at the top of
ProxyPluginTest.java so these tests run only when explicitly selected.

---

Duplicate comments:
In `@Dockerfile`:
- Around line 12-19: The Dockerfile runtime stage currently runs the JVM as root
(ENTRYPOINT ["java", ...]) which is a security risk; add creation of a dedicated
non-root user/group in the runtime image and switch to it before ENTRYPOINT
(create a user and group using Alpine package utilities or system commands,
chown the classes/ and libs/ directories to that user, and add a USER <name>
line so the JVM invoked by ENTRYPOINT runs as the non-root user).

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java`:
- Around line 28-29: The current ConcurrentHashMap field buckets in
RateLimitingFilter is unbounded and can grow indefinitely; replace it with a
bounded/TTL cache (e.g., Caffeine or Guava Cache) to enforce maximumSize and
expireAfterAccess/expireAfterWrite eviction. Update all accesses that use
buckets (creation via computeIfAbsent or direct get/put) to use the cache API
(cache.get(key, Callable) or cache.getIfPresent + cache.put) so per‑IP Bucket
entries are created on demand and evicted automatically; configure sensible max
size and TTL and ensure thread-safe bucket retrieval in the same methods that
currently reference buckets (the buckets field and the code around lines noted
in the review).
- Around line 112-116: In createBucket(), replace the deprecated
Bandwidth.classic(...) and Refill.intervally(...) usage with the Bucket4j 8.x
builder DSL: construct the Bandwidth via Bandwidth.builder() (setting capacity
and the refill policy) and use the new Refill factory method from 8.x for
interval refills; update the createBucket() implementation to build the
Bandwidth with Bandwidth.builder() and then create the Bucket from that builder,
replacing any references to Bandwidth.classic and Refill.intervally accordingly
(keep the same capacity, refillTokens and refillPeriod variables).

In `@src/main/java/org/juv25d/filter/TimingFilter.java`:
- Around line 16-23: The timing log in TimingFilter.doFilter is skipped if
chain.doFilter throws; wrap the invocation of chain.doFilter(req, res) with a
try/finally so the duration calculation and logger.info(req.method() + " " +
req.path() + " took " + durationMs + " ms") always runs; compute start before
the try, compute duration and call logger.info in the finally block (using the
same req, res, start variables) to guarantee emission even on exceptions.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java`:
- Around line 224-255: The createNotFoundResponse(String path) method
interpolates the raw path into the HTML template causing reflected XSS;
escape/HTML-encode the path before calling formatted(path) (or add and call a
dedicated helper like escapeHtml or HtmlUtils.encode) so characters like &, <,
>, " and ' are converted to their HTML entities, then use the encoded value in
the template and return the response as before; update createNotFoundResponse to
perform this encoding and reuse an existing HTML-escape utility if available.

In `@src/main/java/org/juv25d/router/RouterConfig.java`:
- Around line 3-29: Replace the System.out.println call in the RouterConfig
constructor with the project logger: create or obtain a Logger for RouterConfig
(e.g., via your logging framework's factory) and call logger.info(...) or
appropriate level instead of printing to stdout; remove the
System.out.println("Router configured") line and ensure the Logger is imported
and used consistently in RouterConfig.

In `@src/test/java/org/juv25d/http/HttpResponseTest.java`:
- Around line 17-40: Tests only assert defaults and null guard; add positive
round‑trip tests in HttpResponseTest to verify mutators work: create or reuse
the response instance and assert that setStatusCode(int)/statusCode(),
setStatusText(String)/statusText(), setHeader(String,String)/headers().get(...),
and setBody(byte[])/body() return the values set (and that headers contain the
expected key/value and body equals the byte array). Use
assertEquals/assertArrayEquals/assertTrue as appropriate and include a simple
overwrite case for headers to ensure replacement/append behavior is correct.

---

Nitpick comments:
In `@src/main/java/org/juv25d/config/FilterConfig.java`:
- Around line 7-14: The constructor of FilterConfig currently stores the
incoming Map by reference which risks NPEs or external mutation; update the
FilterConfig(Map<String,String> params) constructor to null-guard and
defensively copy the input (e.g., treat null as an empty map and assign a new
HashMap<>(params) or an unmodifiable copy) so the private final params field is
never null and cannot be mutated from outside; keep the get(String key) behavior
unchanged.

In `@src/main/java/org/juv25d/di/Inject.java`:
- Around line 3-7: The Inject annotation lacks `@Documented` which prevents it
appearing in generated Javadoc; update the org.juv25d.di.Inject annotation to
add the java.lang.annotation.Documented meta-annotation (import Documented if
needed) so the `@Inject` marker is included in Javadoc for constructors while
keeping the existing `@Retention`(RetentionPolicy.RUNTIME) and
`@Target`(ElementType.CONSTRUCTOR).

In `@src/main/java/org/juv25d/filter/FilterFactory.java`:
- Around line 13-15: Change the create method in FilterFactory to use a bounded
generic so callers get compile-time safety: replace the signature public Filter
create(Class<?> clazz) with a generic form like public <T extends Filter> T
create(Class<T> clazz) and return container.get(clazz) without an explicit cast;
update references to FilterFactory.create if necessary. Ensure you use the
Container.get(Class<T>) generic overload so the cast is removed and the compiler
enforces that clazz extends Filter (symbols: FilterFactory.create and
Container.get).

In `@src/main/java/org/juv25d/filter/FilterScanner.java`:
- Around line 87-90: In FilterScanner's catch(Exception e) block where it
currently prints "Skipping filter: " + filterClass.getName() + " due to " +
e.getMessage(), preserve full exception details by including the exception/stack
trace in the output instead of only e.getMessage(); e.g., replace the current
System.err print with either a logging call that accepts the Throwable
(logger.log(Level.SEVERE, "...", e)) or call e.printStackTrace() after the
message so the stack trace for filterClass is available for debugging.

In `@src/main/java/org/juv25d/http/HttpStatus.java`:
- Around line 30-37: Replace the current linear scan in getStatusFromCode with
an O(1) reverse lookup: add a static final Map<Integer, HttpStatus> (e.g.,
CODE_MAP) in the HttpStatus enum and populate it once at class-load time (static
initializer or stream collector) mapping each status.getCode() to the enum, then
change getStatusFromCode to return CODE_MAP.getOrDefault(code, UNKNOWN); keep
the UNKNOWN fallback and keep the enum constants unchanged.

In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 16-19: Add null guards to the Pipeline constructor so matcher and
router are validated at construction time: in the Pipeline(FilterMatcher
matcher, Router router) constructor check that matcher and router are non-null
(e.g. via Objects.requireNonNull or explicit null checks) and throw a clear NPE
or IllegalArgumentException if either is null; this ensures failures surface at
Pipeline construction rather than later in FilterChainImpl.doFilter.

In `@src/main/java/org/juv25d/proxy/ProxyRoute.java`:
- Around line 21-24: The buildUrl method currently concatenates upstreamUrl +
targetPath and naively appends "?" + query, which can produce duplicate or
missing slashes and double question marks; update buildUrl to normalize slashes
and the query prefix: trim any trailing '/' from upstreamUrl and any leading '/'
from targetPath (or ensure exactly one '/' between them) before joining, and
when appending query ensure you prepend a single '?' only if query is non-empty
and does not already start with '?'. Make these changes in the buildUrl method
(referencing upstreamUrl, targetPath, and query) so the resulting URL always has
one slash separator and a single leading '?' for the query.

In `@src/test/java/org/juv25d/filter/FilterRegistryTest.java`:
- Around line 67-78: Add a new unit test method (e.g.,
shouldBeIdempotentOnDoubleShutdown) in FilterRegistryTest that constructs a
FilterRegistry and a TestFilter, registers the filter via registerGlobal(filter,
1), calls registry.shutdown() twice, and asserts that TestFilter.destroyCount
equals 1; this verifies that FilterRegistry.shutdown() is idempotent and does
not double-destroy the same filter.

In `@src/test/java/org/juv25d/PipelineTest.java`:
- Around line 1-4: The PipelineTest class is an empty JUnit test skeleton and
provides no coverage; either remove the stub or add concrete JUnit 5 test
methods that exercise the Pipeline behavior. Locate the PipelineTest class and
either delete it, or add `@Test` methods that instantiate the Pipeline (the
Pipeline constructor), call its primary methods (e.g., process/execute/run or
whatever public method names exist on Pipeline), assert expected outputs and
edge cases (normal input, null/empty input, and error conditions) using
Assertions.assertEquals/assertThrows, and include setup/teardown or fixtures if
Pipeline requires dependencies (mock collaborators or use test doubles). Ensure
each test has a descriptive name and covers meaningful behavior so the test file
no longer remains empty.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04addf4 and 338b354.

📒 Files selected for processing (43)

• Dockerfile
• src/main/java/org/juv25d/App.java
• src/main/java/org/juv25d/Bootstrap.java
• src/main/java/org/juv25d/ConnectionHandler.java
• src/main/java/org/juv25d/Pipeline.java
• src/main/java/org/juv25d/config/FilterConfig.java
• src/main/java/org/juv25d/config/IpFilterConfig.java
• src/main/java/org/juv25d/config/RateLimitConfig.java
• src/main/java/org/juv25d/config/RedirectConfig.java
• src/main/java/org/juv25d/di/Container.java
• src/main/java/org/juv25d/di/Inject.java
• src/main/java/org/juv25d/filter/ClassScanner.java
• src/main/java/org/juv25d/filter/Filter.java
• src/main/java/org/juv25d/filter/FilterChainImpl.java
• src/main/java/org/juv25d/filter/FilterFactory.java
• src/main/java/org/juv25d/filter/FilterMatcher.java
• src/main/java/org/juv25d/filter/FilterRegistration.java
• src/main/java/org/juv25d/filter/FilterRegistry.java
• src/main/java/org/juv25d/filter/FilterScanner.java
• src/main/java/org/juv25d/filter/IpFilter.java
• src/main/java/org/juv25d/filter/LoggingFilter.java
• src/main/java/org/juv25d/filter/RateLimitingFilter.java
• src/main/java/org/juv25d/filter/RedirectFilter.java
• src/main/java/org/juv25d/filter/TimingFilter.java
• src/main/java/org/juv25d/handler/StaticFileHandler.java
• src/main/java/org/juv25d/http/HttpStatus.java
• src/main/java/org/juv25d/logging/LogContext.java
• src/main/java/org/juv25d/logging/ServerLogFormatter.java
• src/main/java/org/juv25d/logging/ServerLogging.java
• src/main/java/org/juv25d/proxy/ProxyPlugin.java
• src/main/java/org/juv25d/proxy/ProxyRoute.java
• src/main/java/org/juv25d/router/RouterConfig.java
• src/main/java/org/juv25d/util/ConfigLoader.java
• src/main/resources/application-properties.yml
• src/test/java/org/juv25d/PipelineTest.java
• src/test/java/org/juv25d/di/ContainerTest.java
• src/test/java/org/juv25d/filter/FilterRegistryTest.java
• src/test/java/org/juv25d/filter/IpFilterTest.java
• src/test/java/org/juv25d/filter/LoggingFilterTest.java
• src/test/java/org/juv25d/handler/StaticFileHandlerTest.java
• src/test/java/org/juv25d/http/HttpResponseTest.java
• src/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java
• src/test/java/org/juv25d/proxy/ProxyPluginTest.java

🚧 Files skipped from review as they are similar to previous changes (4)

• src/main/java/org/juv25d/filter/Filter.java
• src/main/java/org/juv25d/filter/FilterChainImpl.java
• src/test/java/org/juv25d/handler/StaticFileHandlerTest.java
• src/main/java/org/juv25d/filter/RedirectFilter.java

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Registered interface instances are unreachable without bindings.

resolve throws on unbound interfaces before checking beans, so register(IFoo.class, instance) can never be retrieved. Check for registered instances first, then fall back to binding validation.

🛠️ Suggested fix

-        // 0. Resolve interface bindings
-        if (bindings.containsKey(type)) {
-            type = (Class<T>) bindings.get(type);
-        }
-
-        // 1. Cannot resolve unbound interface
-        if (type.isInterface() && !bindings.containsKey(type)) {
-            throw new RuntimeException("No binding found for interface: " + type.getName());
-        }
-
-        // 2. Fast path: return existing bean
-        Object existing = beans.get(type);
-        if (existing != null) return (T) existing;
+        // 0. Fast path: return existing bean
+        Object existing = beans.get(type);
+        if (existing != null) return (T) existing;
+
+        // 1. Resolve interface bindings
+        Class<?> bound = bindings.get(type);
+        if (bound != null) {
+            type = (Class<T>) bound;
+            existing = beans.get(type);
+            if (existing != null) return (T) existing;
+        } else if (type.isInterface()) {
+            throw new RuntimeException("No binding found for interface: " + type.getName());
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/di/Container.java` around lines 147 - 160, The
resolve method currently replaces the interface type with its binding before
checking beans, so a bean registered under the interface key (via
register(IFoo.class, instance)) is never found; fix by checking beans.get(type)
first and returning it if present, then apply bindings (type =
bindings.get(type)) and proceed with the interface binding validation and later
existing-bean fast path. Update resolve to: 1) check beans.get(type) and return
if non-null, 2) then if bindings.containsKey(type) set type =
(Class<T>)bindings.get(type), 3) then if type.isInterface() &&
!bindings.containsKey(type) throw, and 4) continue with the rest of resolve;
reference methods/fields: resolve, bindings, beans, register.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

NPE if the context ClassLoader is null.

Thread.currentThread().getContextClassLoader() may return null in certain environments (e.g., the bootstrap class loader context). The immediate cl.getResources(path) on line 31 would then throw a NullPointerException that propagates out of findClasses as a RuntimeException.

🛡️ Proposed fix

         ClassLoader cl = Thread.currentThread().getContextClassLoader();
+        if (cl == null) {
+            cl = ClassScanner.class.getClassLoader();
+        }
         Enumeration<URL> resources = cl.getResources(path);

📝 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.

Suggested change

	ClassLoader cl = Thread.currentThread().getContextClassLoader();
	Enumeration<URL> resources = cl.getResources(path);
	ClassLoader cl = Thread.currentThread().getContextClassLoader();
	if (cl == null) {
	cl = ClassScanner.class.getClassLoader();
	}
	Enumeration<URL> resources = cl.getResources(path);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/ClassScanner.java` around lines 30 - 31, The
code in ClassScanner.findClasses calls
Thread.currentThread().getContextClassLoader() and immediately uses
cl.getResources(path), which can NPE if the context ClassLoader is null; update
findClasses to check if cl is null and if so fall back to a non-null loader (for
example ClassScanner.class.getClassLoader() or
ClassLoader.getSystemClassLoader()) before calling getResources(path), and use
the resulting 'cl' variable for the subsequent resources lookup to avoid the
NullPointerException.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Inaccurate @throws ClassNotFoundException on scanDir.

The declared @throws ClassNotFoundException is misleading — that exception is silently swallowed in the inner catch block at line 81. Remove the @throws tag from the Javadoc.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/ClassScanner.java` around lines 58 - 65, The
Javadoc for ClassScanner.scanDir incorrectly declares "@throws
ClassNotFoundException" even though the method catches and swallows
ClassNotFoundException internally; remove the "@throws ClassNotFoundException"
tag from the scanDir Javadoc comment so the documentation matches the
implementation (locate the Javadoc above the scanDir method in ClassScanner and
delete the `@throws` line).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/main/java/org/juv25d/filter/ClassScanner.java | head -90

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 4078

---

🌐 Web query:

Class.forName ExceptionInInitializerError Java documentation

💡 Result:

Class.forName(String) loads and initializes the named class; i.e., Class.forName("X") “causes the class named X to be initialized.” If that initialization (running static field initializers and static initializer blocks) fails, the Javadoc says Class.forName can throw ExceptionInInitializerError (“if the initialization provoked by this method fails”). [1]

ExceptionInInitializerError itself is documented as signaling that an unexpected exception occurred during a static initializer or static variable initialization; the underlying problem is available via exception chaining (preferred: getCause()), and the legacy accessor getException(). [2]

Docs:

• java.lang.Class#forName(String) throws ExceptionInInitializerError when initialization triggered by forName fails. [1]
• java.lang.ExceptionInInitializerError definition and how to retrieve the wrapped cause. [2]

Sources
[1] Oracle Javadoc: java.lang.Class (method forName(String)) (docs.oracle.com)
[2] Oracle Javadoc: java.lang.ExceptionInInitializerError (docs.oracle.com)

Citations:

• 1: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Class.html?utm_source=openai
• 2: https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/lang/ExceptionInInitializerError.html?utm_source=openai

---

🌐 Web query:

ExceptionInInitializerError extends Error or Exception Java class hierarchy

💡 Result:

ExceptionInInitializerError is an Error, not an Exception.

Class hierarchy (simplified):

Object → Throwable → Error → LinkageError → ExceptionInInitializerError [1][2]

It is declared as:

public class ExceptionInInitializerError extends LinkageError [2]

---

🏁 Script executed:

# Search for usage of ClassScanner to understand if it's used for annotation scanning
rg "ClassScanner|findClasses" --type java -B 2 -A 2 | head -50

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 2202

---

🏁 Script executed:

cat -n src/main/java/org/juv25d/filter/FilterScanner.java | head -60

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 2784

---

ExceptionInInitializerError from class initialization will crash server startup.

Class.forName(className, true, cl) triggers static initializers. If a static initializer throws an exception, Class.forName throws ExceptionInInitializerError, which extends Error (not Exception). Neither the inner catch at line 81–83 (catches ClassNotFoundException | NoClassDefFoundError) nor the outer catch at line 53 (catches Exception) will catch Error. Any class in the scanned package with a failing static initializer will crash the server at startup.

The fix is to pass false for the initialize parameter, which is appropriate for annotation-based scanning where static initialization side effects are unwanted.

🐛 Proposed fix

-                    classes.add(Class.forName(className, true, cl));
+                    classes.add(Class.forName(className, false, cl));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/ClassScanner.java` around lines 79 - 83, The
ClassScanner currently calls Class.forName(className, true, cl) which forces
static initialization and can throw Error (ExceptionInInitializerError) that
will crash startup; change the call in ClassScanner where
classes.add(Class.forName(className, true, cl)) is invoked to use
Class.forName(className, false, cl) so classes are loaded without running static
initializers (preserving annotation-based scanning and avoiding Error
propagation from static initializers on class load).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Wildcard pattern /api/* does not match the exact base path /api.

pattern.substring(0, pattern.length() - 1) strips only *, leaving the prefix as /api/. A request to /api (path.startsWith("/api/") → false) will miss the pattern. Depending on intent, the base path may need to be matched explicitly.

♻️ Proposed fix to also match the exact base

         if (pattern.endsWith("/*")) {
             String prefix = pattern.substring(0, pattern.length() - 1);
-            return path.startsWith(prefix);
+            String base = pattern.substring(0, pattern.length() - 2); // strip /*
+            return path.equals(base) || path.startsWith(prefix);
         }

📝 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.

Suggested change

	private boolean matches(String pattern, String path) {
	if (pattern.equals(path)) return true;
	if (pattern.endsWith("/*")) {
	String prefix = pattern.substring(0, pattern.length() - 1);
	return path.startsWith(prefix);
	}
	return false;
	private boolean matches(String pattern, String path) {
	if (pattern.equals(path)) return true;
	if (pattern.endsWith("/*")) {
	String prefix = pattern.substring(0, pattern.length() - 1);
	String base = pattern.substring(0, pattern.length() - 2); // strip /*
	return path.equals(base) || path.startsWith(prefix);
	}
	return false;
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterMatcher.java` around lines 36 - 43, The
matches method in FilterMatcher incorrectly handles patterns like "/api/*"
because it strips only the '*' leaving a trailing slash and thus doesn't match
the exact base "/api"; update the "pattern.endsWith(\"/*\")" branch to compute
the basePrefix by removing "/*" (e.g., pattern.substring(0, pattern.length() -
2)) and then return true if path.equals(basePrefix) or path starts with
basePrefix + "/" so both the exact base ("/api") and its children ("/api/...",
"/api/v1") match.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Hop-by-hop headers from the upstream response are forwarded to the client unfiltered.

isRestrictedHeader is applied only to outgoing request headers (line 62) but not here when relaying the upstream response. Headers such as transfer-encoding, connection, and keep-alive from the upstream should not be forwarded — they are transport-level and can confuse clients or cause protocol errors.

🛡️ Proposed fix

             upstreamResponse.headers().map().forEach((name, values) -> {
-                if (!values.isEmpty()) {
+                if (!values.isEmpty() && !isRestrictedHeader(name)) {
                     res.setHeader(name, values.get(0));
                 }
             });

📝 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.

Suggested change

	upstreamResponse.headers().map().forEach((name, values) -> {
	if (!values.isEmpty()) {
	res.setHeader(name, values.get(0));
	}
	});
	upstreamResponse.headers().map().forEach((name, values) -> {
	if (!values.isEmpty() && !isRestrictedHeader(name)) {
	res.setHeader(name, values.get(0));
	}
	});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/proxy/ProxyPlugin.java` around lines 81 - 85, The
upstream response loop forwards hop-by-hop headers to the client; modify the
block that iterates upstreamResponse.headers().map().forEach(...) so it checks
isRestrictedHeader(name) before calling res.setHeader, skipping headers like
"transfer-encoding", "connection", "keep-alive" (i.e., reuse the same
isRestrictedHeader logic used for outgoing requests). Ensure you apply the
filter to the header name (and still handle non-empty values) when setting
headers via res.setHeader to avoid forwarding transport-level headers.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Hardcoded third-party test URL in the committed default config.

https://jsonplaceholder.typicode.com is a public test service. Shipping this as the default upstream-url means any server started with the default config will proxy production traffic to a public API. Replace it with a placeholder or an environment-variable reference so it cannot be accidentally used.

♻️ Suggested change

       - base-route: /api/test
-        upstream-url: https://jsonplaceholder.typicode.com
+        upstream-url: ${PROXY_UPSTREAM_URL:https://localhost:8081}

📝 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.

Suggested change

	- base-route: /api/test
	upstream-url: https://jsonplaceholder.typicode.com
	- base-route: /api/test
	upstream-url: ${PROXY_UPSTREAM_URL:https://localhost:8081}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/application-properties.yml` around lines 6 - 7, Replace
the hardcoded third‑party test URL in the default config by changing the
upstream-url property to a non-functional placeholder or an environment-variable
reference (e.g., use an env var like UPSTREAM_URL with a sensible empty/default
placeholder) so the default config cannot accidentally proxy traffic to
jsonplaceholder; update the upstream-url entry in the YAML (the upstream-url
property near base-route) to reference the environment variable or placeholder
and add a brief comment indicating it must be set for real deployments.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "LoggingFilterTest.java" | head -5

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 164

---

🏁 Script executed:

cat -n ./src/test/java/org/juv25d/filter/LoggingFilterTest.java | sed -n '1,80p'

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 2485

---

🏁 Script executed:

find . -type f -name "ServerLogging.java" | head -5

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 161

---

🏁 Script executed:

cat -n ./src/main/java/org/juv25d/logging/ServerLogging.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 1668

---

🏁 Script executed:

find . -type f -name "LoggingFilter.java" | head -5

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 160

---

🏁 Script executed:

cat -n ./src/main/java/org/juv25d/filter/LoggingFilter.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 841

---

Make logging assertions deterministic by pinning logger level.

The global logger's level is configurable via log.level property, LOG_LEVEL environment variable, or ConfigLoader.getLogLevel(). If the environment sets the level to WARN or ERROR, the test will fail since LoggingFilter logs at INFO level. Set the level for this test and restore it in finally.

🛠️ Suggested fix

         java.util.logging.Logger logger = org.juv25d.logging.ServerLogging.getLogger();
+        java.util.logging.Level previousLevel = logger.getLevel();
+        logger.setLevel(java.util.logging.Level.INFO);
         java.util.List<java.util.logging.LogRecord> records = new java.util.ArrayList<>();
         java.util.logging.Handler handler = new java.util.logging.Handler() {
             `@Override`
             public void publish(java.util.logging.LogRecord record) {
                 records.add(record);
             }
@@
         };
+        handler.setLevel(java.util.logging.Level.ALL);
         logger.addHandler(handler);

         try {
             filter.doFilter(req, res, chain);
@@
             assertTrue(found, "Logger should have captured the method and path");
         } finally {
             logger.removeHandler(handler);
+            logger.setLevel(previousLevel);
         }

📝 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.

Suggested change

	java.util.logging.Logger logger = org.juv25d.logging.ServerLogging.getLogger();
	java.util.List<java.util.logging.LogRecord> records = new java.util.ArrayList<>();
	java.util.logging.Handler handler = new java.util.logging.Handler() {
	@Override
	public void publish(java.util.logging.LogRecord record) {
	records.add(record);
	}
	@Override
	public void flush() {}
	@Override
	public void close() throws SecurityException {}
	};
	logger.addHandler(handler);
	try {
	filter.doFilter(req, res, chain);
	boolean found = records.stream()
	.anyMatch(r -> r.getMessage().contains("GET /test"));
	assertTrue(found, "Logger should have captured the method and path");
	} finally {
	logger.removeHandler(handler);
	java.util.logging.Logger logger = org.juv25d.logging.ServerLogging.getLogger();
	java.util.logging.Level previousLevel = logger.getLevel();
	logger.setLevel(java.util.logging.Level.INFO);
	java.util.List<java.util.logging.LogRecord> records = new java.util.ArrayList<>();
	java.util.logging.Handler handler = new java.util.logging.Handler() {
	`@Override`
	public void publish(java.util.logging.LogRecord record) {
	records.add(record);
	}
	`@Override`
	public void flush() {}
	`@Override`
	public void close() throws SecurityException {}
	};
	handler.setLevel(java.util.logging.Level.ALL);
	logger.addHandler(handler);
	try {
	filter.doFilter(req, res, chain);
	boolean found = records.stream()
	.anyMatch(r -> r.getMessage().contains("GET /test"));
	assertTrue(found, "Logger should have captured the method and path");
	} finally {
	logger.removeHandler(handler);
	logger.setLevel(previousLevel);
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/filter/LoggingFilterTest.java` around lines 37 - 58,
The test LoggingFilterTest is non-deterministic because the global logger level
(from ServerLogging.getLogger()) may be higher than INFO so the LoggingFilter's
INFO logs are not recorded; fix by saving the current logger level, set
logger.setLevel(java.util.logging.Level.INFO) before calling
filter.doFilter(req, res, chain), run the assertions, and then restore the
original level in the finally block (use the same logger variable obtained via
ServerLogging.getLogger()) so the test does not depend on external log
configuration.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find the test file and explore repository structure
find . -name "ConnectionIdLoggingTest.java" -type f 2>/dev/null | head -5

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 171

---

🏁 Script executed:

# List the test directory structure to understand the project layout
fd --type f -e java | grep -E "(test|Test)" | head -20

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 1164

---

🏁 Script executed:

# Check if the file exists and read its content
cat -n src/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 1901

---

🏁 Script executed:

# Find and read LogContext.java
find . -name "LogContext.java" -type f | head -1

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 158

---

🏁 Script executed:

# Check ServerLoggingTest for pattern comparison
cat -n src/test/java/org/juv25d/logging/ServerLoggingTest.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 5237

---

🏁 Script executed:

# Search for other logger.removeHandler usages in test files
rg "removeHandler" src/test --type java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 502

---

🏁 Script executed:

# Check logging package structure
ls -la src/main/java/org/juv25d/logging/ 2>/dev/null || ls -la src/*/org/juv25d/logging/ 2>/dev/null | head -20

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 430

---

🏁 Script executed:

# Read LogContext.java to understand what LogContext.clear() does
cat -n src/main/java/org/juv25d/logging/LogContext.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 611

---

🏁 Script executed:

# Check LoggingFilterTest to see how it handles handler cleanup
cat -n src/test/java/org/juv25d/filter/LoggingFilterTest.java | head -100

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 2485

---

🏁 Script executed:

# Search for logger.setLevel usage in test files to see if this is a common pattern
rg "logger.setLevel|\.setLevel" src/test --type java -B 2 -A 2

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 90

---

🏁 Script executed:

# Verify Java logging level inheritance behavior by checking ServerLogging.java
cat -n src/main/java/org/juv25d/logging/ServerLogging.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 1668

---

Set explicit logger level and remove handler in finally block.

The logger will inherit its level from the parent logger even though setUseParentHandlers(false) is called—this only disables parent handlers, not level inheritance. Without an explicit level, INFO messages may be filtered depending on parent logger configuration. Additionally, the handler is never removed, causing it to leak into subsequent tests in the same JVM. See LoggingFilterTest for the correct cleanup pattern.

🛠️ Suggested fix

         Logger logger = Logger.getLogger("test.connectionid");
         logger.setUseParentHandlers(false);
+        logger.setLevel(java.util.logging.Level.INFO);

         List<String> formattedMessages = new ArrayList<>();
         ServerLogFormatter formatter = new ServerLogFormatter();

         Handler handler = new Handler() {
             `@Override`
             public void publish(LogRecord record) {
                 formattedMessages.add(formatter.format(record));
             }
             `@Override`
             public void flush() {}
             `@Override`
             public void close() throws SecurityException {}
         };
+        handler.setLevel(java.util.logging.Level.ALL);
         logger.addHandler(handler);

         try {
             String testId = "test-123";
             LogContext.setConnectionId(testId);

             // Act
             logger.info("This is a test message");

             // Assert
             assertTrue(formattedMessages.get(0).contains("[" + testId + "]"),
                 "Log message should contain the connection ID. Found: " + formattedMessages.get(0));
         } finally {
+            logger.removeHandler(handler);
             LogContext.clear();
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java` around lines
17 - 46, The test doesn't set an explicit logger level and never removes the
handler, causing message filtering and handler leakage; set the test logger's
level to Level.INFO (call logger.setLevel(Level.INFO)) before logging, and in
the finally block remove the custom Handler (call logger.removeHandler(handler))
and clear the LogContext as already done; reference the Logger instance named
logger, the Handler variable handler, and the LogContext.clear() call when
applying these changes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's check if the file exists and get its size
wc -l src/test/java/org/juv25d/proxy/ProxyPluginTest.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 163

---

🏁 Script executed:

# Read the entire file to see the test structure, especially lines 30-104
cat -n src/test/java/org/juv25d/proxy/ProxyPluginTest.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 11597

---

Isolate external-network integration tests from default test runs.

The IntegrationTests class (lines 30–104) makes real network calls to external services (jsonplaceholder.typicode.com and an invalid upstream domain), which introduces unreliable dependencies in your test suite. These tests can be slow, flaky, or blocked entirely in CI environments with restricted network egress. Separate them from your unit tests using JUnit 5's @Tag annotation so they run only when explicitly invoked.

Example: Tag integration tests

+import org.junit.jupiter.api.Tag;

     `@Nested`
+    `@Tag`("integration")
     class IntegrationTests {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/proxy/ProxyPluginTest.java` around lines 3 - 12, The
IntegrationTests nested class performs real network calls and should be
annotated with a JUnit tag to exclude it from default unit test runs; add
`@Tag`("integration") to the IntegrationTests class declaration and add the
corresponding import (org.junit.jupiter.api.Tag) at the top of
ProxyPluginTest.java so these tests run only when explicitly selected.

[coderabbitai]

Actionable comments posted: 6

♻️ Duplicate comments (8)

Dockerfile (1)

12-19: ⚠️ Potential issue | 🟠 Major

Run the runtime container as a non-root user.

Line 19 runs the JVM without a preceding USER, so the process runs as root in the runtime image. Please add an unprivileged user and switch to it before ENTRYPOINT.

🔒 Suggested fix

 FROM eclipse-temurin:25-jre-alpine
 
 WORKDIR /app
 
-COPY --from=build /app/target/deps/     libs/
-COPY --from=build /app/target/classes/  classes/
+RUN addgroup -S appgroup && adduser -S appuser -G appgroup
+COPY --from=build --chown=appuser:appgroup /app/target/deps/     libs/
+COPY --from=build --chown=appuser:appgroup /app/target/classes/  classes/
+USER appuser
 
 ENTRYPOINT ["java", "-cp", "classes:libs/*","org.juv25d.App"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 12 - 19, The runtime image currently runs the JVM as
root (ENTRYPOINT ["java", "-cp", "classes:libs/*","org.juv25d.App"]); create an
unprivileged user/group, chown the WORKDIR and the copied directories (classes
and libs) to that user, and add a USER instruction to switch to it before
ENTRYPOINT so the JVM runs unprivileged; locate the Dockerfile entries
referencing WORKDIR, the copied targets (libs and classes) and ENTRYPOINT to add
the user creation, chown, and USER change.

src/main/java/org/juv25d/Pipeline.java (1)

16-23: ⚠️ Potential issue | 🟠 Major

Fail fast on null matcher/router to avoid deferred runtime NPEs.

Current constructor allows null dependencies and shifts failure into request handling.

♻️ Suggested fix

     public Pipeline(FilterMatcher matcher, Router router) {
-        this.matcher = matcher;
-        this.router = router;
+        this.matcher = java.util.Objects.requireNonNull(matcher, "matcher must not be null");
+        this.router = java.util.Objects.requireNonNull(router, "router must not be null");
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/Pipeline.java` around lines 16 - 23, Constructor
allows null matcher/router which defers NPEs; validate and fail fast in the
Pipeline constructor by checking parameters (e.g., use Objects.requireNonNull or
explicit checks) for matcher and router and throw a clear exception
(IllegalArgumentException or NullPointerException) with descriptive message;
this ensures createChain (which calls matcher.match and constructs new
FilterChainImpl(filters, router)) never runs with null dependencies.

src/main/java/org/juv25d/filter/RateLimitingFilter.java (2)

28-28: ⚠️ Potential issue | 🟠 Major

Unbounded per-IP bucket storage can be abused for memory exhaustion.

buckets grows forever with new client IPs; this is a DoS/memory-pressure risk under high-cardinality traffic.

♻️ Suggested fix (bounded + expiring cache)

-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
@@
-    private final Map<String, Bucket> buckets = new ConcurrentHashMap<>();
+    private final Cache<String, Bucket> buckets = Caffeine.newBuilder()
+        .maximumSize(100_000)
+        .expireAfterAccess(Duration.ofMinutes(30))
+        .build();
@@
-        Bucket bucket = buckets.computeIfAbsent(clientIp, k -> createBucket());
+        Bucket bucket = buckets.get(clientIp, k -> createBucket());
@@
-        return buckets.size();
+        return (int) buckets.estimatedSize();
@@
-        buckets.clear();
+        buckets.invalidateAll();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` at line 28, The
buckets field (private final Map<String, Bucket> buckets) is unbounded and can
be abused to exhaust memory; replace it with a bounded, expiring cache (e.g.,
Caffeine/Guava LoadingCache or Cache with maxSize and expireAfterAccess) keyed
by client IP and returning/creating Bucket instances for RateLimitingFilter;
update any accesses that call buckets.get/put to use cache.get(key) or
cache.getIfPresent and ensure thread-safe bucket creation/eviction semantics are
preserved and that metrics/logging still work when entries are evicted.

---

113-115: ⚠️ Potential issue | 🟠 Major

Verify Bucket4j API compatibility for Bandwidth.classic / Refill.intervally.

If your dependency is on newer 8.x behavior, these calls may be deprecated/unsupported and should be migrated to Bandwidth.builder()...build().

In Bucket4j 8.16.x, are `Bandwidth.classic(...)` and `Refill.intervally(...)` still supported? If deprecated/removed, what is the exact `Bandwidth.builder()` replacement?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` around lines 113 -
115, Bandwidth.classic(...) and Refill.intervally(...) may be deprecated in
newer Bucket4j versions; replace the call to Bandwidth.classic(capacity,
Refill.intervally(refillTokens, refillPeriod)) with the builder-style API:
create a Bandwidth via Bandwidth.builder(), set the capacity and refill with the
equivalent Refill (e.g. Refill.intervally(refillTokens, refillPeriod) if
available), then call .build() — i.e. use
Bandwidth.builder().capacity(capacity).refill(Refill.intervally(refillTokens,
refillPeriod)).build() (or the exact .refill(...) variant the library exposes)
to restore compatibility for the code using Bandwidth.classic and
Refill.intervally.

src/main/java/org/juv25d/filter/FilterRegistry.java (2)

30-33: ⚠️ Potential issue | 🟠 Major

shutdown() has a check-then-set race.

Concurrent callers can both pass the boolean guard and run destruction; make shutdown entry atomic.

🔒 Suggested fix (CAS guard)

-    private volatile boolean isShutdown = false;
+    private final java.util.concurrent.atomic.AtomicBoolean isShutdown =
+        new java.util.concurrent.atomic.AtomicBoolean(false);
@@
     public void shutdown() {
-        if (isShutdown) return;
-        isShutdown = true;
+        if (!isShutdown.compareAndSet(false, true)) return;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterRegistry.java` around lines 30 - 33,
The shutdown() method has a race between the boolean check and set (isShutdown)
allowing concurrent callers to both proceed; fix by making the entry
atomic—replace the plain boolean guard with an atomic compare-and-set or
otherwise synchronize entry so only one caller performs destruction (e.g., use
an AtomicBoolean isShutdown and call compareAndSet(false, true) at the start of
shutdown(), or synchronize the method and check isShutdown before proceeding);
update any references to isShutdown accordingly to reflect the atomic guard.

---

26-28: ⚠️ Potential issue | 🟡 Minor

getRouteFilters() still exposes mutable inner lists.

Wrapping only the map leaves each route list modifiable by callers, which breaks registry encapsulation.

♻️ Suggested fix (deeply unmodifiable snapshot)

     public Map<String, List<FilterRegistration>> getRouteFilters() {
-        return Collections.unmodifiableMap(routes);
+        Map<String, List<FilterRegistration>> snapshot = new HashMap<>();
+        routes.forEach((k, v) -> snapshot.put(k, List.copyOf(v)));
+        return Collections.unmodifiableMap(snapshot);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterRegistry.java` around lines 26 - 28,
getRouteFilters() currently returns an unmodifiable map but still exposes
mutable inner lists; change it to return a deep unmodifiable snapshot by copying
each entry in the routes map and replacing each List<FilterRegistration> value
with an unmodifiable copy (e.g. new ArrayList<>(originalList) wrapped with
Collections.unmodifiableList) and then wrapping the resulting map with
Collections.unmodifiableMap before returning; update the method that references
the routes field (routes) and the getRouteFilters() implementation accordingly
so callers cannot mutate the inner lists.

src/main/java/org/juv25d/filter/FilterMatcher.java (1)

38-40: ⚠️ Potential issue | 🟡 Minor

Wildcard route matching misses exact base path (e.g. /api).

For patterns like /api/*, current logic matches /api/... but not /api.

♻️ Suggested fix

         if (pattern.endsWith("/*")) {
-            String prefix = pattern.substring(0, pattern.length() - 1);
-            return path.startsWith(prefix);
+            String base = pattern.substring(0, pattern.length() - 2); // strip /*
+            return path.equals(base) || path.startsWith(base + "/");
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterMatcher.java` around lines 38 - 40, In
FilterMatcher where you handle pattern.endsWith("/*"), the current logic builds
prefix = pattern.substring(0, pattern.length() - 1) which yields "/api/" for
"/api/*" and therefore misses an exact "/api" match; change the check to accept
either the exact base path or any subpath by deriving the base (pattern with
trailing "/*" removed) and returning path.equals(base) || path.startsWith(base +
"/") (or equivalently check path.equals(prefixWithoutTrailingSlash) ||
path.startsWith(prefix) depending on how you compute prefix) so "/api" and
"/api/..." both match.

src/main/java/org/juv25d/logging/ServerLogging.java (1)

26-37: ⚠️ Potential issue | 🔴 Critical

Guard levelName before toUpperCase() to prevent startup NPE.

At Line 32, levelName.toUpperCase() can throw when property/env/config all resolve to null. The catch (IllegalArgumentException) at Line 34 will not catch that NullPointerException.

Defensive fix

         String levelName = System.getProperty(
             "log.level",
             System.getenv().getOrDefault("LOG_LEVEL", ConfigLoader.getInstance().getLogLevel())
         );
 
         try {
-        Level level = Level.parse(levelName.toUpperCase());
-        logger.setLevel(level);
+            String normalized = (levelName == null || levelName.isBlank())
+                ? "INFO"
+                : levelName.trim().toUpperCase(java.util.Locale.ROOT);
+            Level level = Level.parse(normalized);
+            logger.setLevel(level);
         } catch (IllegalArgumentException e) {
             logger.setLevel(Level.INFO);
             logger.warning("Invalid log level: '" + levelName + "', defaulting to INFO");
         }

Run this to verify null-path risk from config + current call site:

#!/bin/bash
set -euo pipefail

echo "1) Inspect getLogLevel() implementation"
rg -n -C4 --type=java 'getLogLevel\s*\(' src/main/java/org/juv25d/util/ConfigLoader.java

echo
echo "2) Inspect configured defaults for log level"
fd 'application-properties.yml'
rg -n -C3 'log.?level|logging' src/main/resources/application-properties.yml

echo
echo "3) Inspect current usage in ServerLogging"
rg -n -C4 --type=java 'levelName|toUpperCase|Level\.parse' src/main/java/org/juv25d/logging/ServerLogging.java

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/logging/ServerLogging.java` around lines 26 - 37,
The code in ServerLogging calls levelName.toUpperCase() without null-check,
which can throw NPE before Level.parse is reached; fix by guarding levelName
(the variable computed from
System.getProperty/System.getenv/ConfigLoader.getLogLevel()) and defaulting it
to a safe value (e.g. "INFO") before calling toUpperCase and Level.parse, then
proceed to call Level.parse(...) and logger.setLevel(...), keeping the existing
catch for IllegalArgumentException for invalid values; update the logic around
Level.parse(levelName.toUpperCase()) and logger.setLevel(...) to use the
non-null default.

🧹 Nitpick comments (10)

src/main/java/org/juv25d/config/RedirectConfig.java (1)

9-15: Prefer a static cached rule list instead of rebuilding on each call.

rules() currently allocates a new immutable list and new RedirectRule objects every call. Since these rules are constant, use a private static final list and return it to reduce allocation churn and make intent explicit.

♻️ Proposed refactor

 public class RedirectConfig {
+    private static final List<RedirectRule> RULES = List.of(
+        new RedirectRule("/old-page", "/new-page", 301),
+        new RedirectRule("/temp", "https://example.com/temporary", 302),
+        new RedirectRule("/docs/*", "/documentation/", 301)
+    );
 
     public List<RedirectRule> rules() {
-        return List.of(
-            new RedirectRule("/old-page", "/new-page", 301),
-            new RedirectRule("/temp", "https://example.com/temporary", 302),
-            new RedirectRule("/docs/*", "/documentation/", 301)
-        );
+        return RULES;
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/config/RedirectConfig.java` around lines 9 - 15, The
rules() method in RedirectConfig currently creates a new immutable List and new
RedirectRule instances on every call; change this by declaring a private static
final List<RedirectRule> (e.g. RULES) initialized once with the
RedirectRule(...) entries and have rules() simply return that static list to
avoid repeated allocations and make intent explicit; locate the RedirectConfig
class and replace the per-call construction in rules() with a reference to the
static final RULES constant.

src/test/java/org/juv25d/handler/StaticFileHandlerTest.java (2)

175-182: Minor: Redundant assertion.

Lines 180-181 have redundant assertions—isNotEmpty() already implies length > 0. The second assertion can be removed.

♻️ Suggested cleanup

         assertThat(response.body()).isNotEmpty();
-        assertThat(response.body().length).isGreaterThan(0);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java` around lines 175
- 182, In the test method shouldReturnNonEmptyBodyForSuccessfulRequest in
StaticFileHandlerTest, remove the redundant assertion
assertThat(response.body().length).isGreaterThan(0) because
assertThat(response.body()).isNotEmpty() already covers that; leave the single
isNotEmpty() assertion to keep the test concise and clear while still validating
a non-empty response body from StaticFileHandler.handle(request).

---

108-139: Consider adding URL-encoded traversal test.

The security tests cover common traversal patterns but don't test URL-encoded variants (e.g., %2e%2e for .., %2f%2f for //). Depending on whether HttpRequest decodes paths, these could bypass the current checks.

➕ Suggested additional test

`@Test`
void shouldReturn403ForUrlEncodedPathTraversal() {
    // Test URL-encoded ".." (%2e%2e)
    HttpRequest request = createRequest("GET", "/%2e%2e/%2e%2e/etc/passwd");
    HttpResponse response = StaticFileHandler.handle(request);

    // Should be 403 if decoded, or 404 if not found (but NOT serve /etc/passwd)
    assertThat(response.statusCode()).isIn(403, 404);
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java` around lines 108
- 139, Add a test that covers URL-encoded traversal sequences to ensure the
handler blocks encoded attacks: create a new test (e.g.,
shouldReturn403ForUrlEncodedPathTraversal) that builds an HttpRequest via
createRequest with a path like "/%2e%2e/%2e%2e/etc/passwd" (and optionally
"/%2e%2e/%2f%2e%2e/etc/passwd") and calls StaticFileHandler.handle(request);
assert that the response.statusCode() is not 200 — prefer asserting it is either
403 or 404 (assertThat(response.statusCode()).isIn(403, 404)) to allow both
decoded-forbidden or not-found behavior, and include the test alongside the
existing traversal tests.

src/main/java/org/juv25d/handler/StaticFileHandler.java (1)

215-221: Minor: opaqueTag() is called multiple times on same value.

Line 217 calls opaqueTag(part) twice. Consider caching the result for clarity and minor efficiency.

♻️ Suggested improvement

         String[] parts = value.split(",");
         for (String part : parts) {
-            if (part != null && opaqueTag(part) != null && opaqueTag(part).equals(opaqueTag(currentEtag))) {
+            String partTag = opaqueTag(part);
+            if (partTag != null && partTag.equals(opaqueTag(currentEtag))) {
                 return true;
             }
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 215 -
221, In StaticFileHandler, avoid calling opaqueTag(...) multiple times inside
the loop: compute opaqueTag(currentEtag) once before the loop and for each part
cache opaqueTag(part) into a local variable (e.g., partTag) and compare
partTag.equals(cachedCurrentTag); return true on match and false after the loop
to preserve behavior while improving clarity and minor performance.

src/main/java/org/juv25d/util/ConfigLoader.java (1)

93-98: Long.parseLong can throw NumberFormatException for non-numeric config values.

If requests-per-minute or burst-capacity contains a non-numeric string, parsing fails with an uncaught exception. The outer catch will wrap it, but a more graceful fallback to defaults would be cleaner.

♻️ Example with safe parsing

-                this.requestsPerMinute =
-                    Long.parseLong(String.valueOf(rateLimitingConfig.getOrDefault("requests-per-minute", 60L)));
+                this.requestsPerMinute = parseLongOrDefault(
+                    rateLimitingConfig.get("requests-per-minute"), 60L);

-                this.burstCapacity =
-                    Long.parseLong(String.valueOf(rateLimitingConfig.getOrDefault("burst-capacity", 100L)));
+                this.burstCapacity = parseLongOrDefault(
+                    rateLimitingConfig.get("burst-capacity"), 100L);

Add helper method:

private static long parseLongOrDefault(Object value, long defaultValue) {
    if (value instanceof Number n) return n.longValue();
    if (value != null) {
        try { return Long.parseLong(String.valueOf(value)); }
        catch (NumberFormatException ignored) { }
    }
    return defaultValue;
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 93 - 98, Handle
non-numeric config values for requestsPerMinute and burstCapacity by adding a
safe parser (e.g., parseLongOrDefault) that accepts Object, returns the
Number.longValue() when value is a Number, attempts Long.parseLong in a
try/catch for strings, and falls back to the provided default on failure; then
replace the current Long.parseLong(String.valueOf(...)) calls that read from
rateLimitingConfig with calls to
parseLongOrDefault(rateLimitingConfig.getOrDefault("requests-per-minute", 60L),
60L) and parseLongOrDefault(rateLimitingConfig.getOrDefault("burst-capacity",
100L), 100L) respectively to avoid NumberFormatException propagating.

src/main/java/org/juv25d/filter/FilterChainImpl.java (1)

10-19: FilterChainImpl instance is not reusable — document or enforce single-use.

The mutable index field makes this class single-use only. If accidentally reused (or shared across threads), the chain state will be corrupted. Consider making the filter list immutable via defensive copy.

♻️ Proposed defensive copy

     public FilterChainImpl(List<Filter> filters, Router router) {
-        this.filters = filters;
+        this.filters = List.copyOf(filters);
         this.router = router;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterChainImpl.java` around lines 10 - 19,
The FilterChainImpl is single-use because the mutable field index tracks
progress; fix by making a defensive, immutable copy of the incoming filters in
the constructor (in FilterChainImpl(List<Filter> filters, Router router) assign
this.filters = Collections.unmodifiableList(new ArrayList<>(filters))) to
prevent external mutation, and add class-level Javadoc on FilterChainImpl
documenting that instances are not reusable/are single-use (and/or add a runtime
guard in the filter dispatch method to throw IllegalStateException if reused via
the index field). Ensure references to the fields filters and index remain
consistent after the change.

src/main/java/org/juv25d/proxy/ProxyRoute.java (1)

21-26: Consider URL encoding in buildUrl if paths with special characters may be passed.

The current string concatenation will work for typical proxy paths, but if targetPath or query contain characters requiring encoding (e.g., spaces, non-ASCII), the constructed URL may be malformed. Given that targetPath comes directly from req.path() without decoding, encoding here could cause double-encoding. Verify how the HTTP parser handles path encoding before adding URLEncoder.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/proxy/ProxyRoute.java` around lines 21 - 26,
buildUrl currently concatenates upstreamUrl, targetPath and query which can
produce malformed URLs for paths/queries with special chars; change buildUrl to
compose the URL with java.net.URI: parse upstreamUrl into a URI
(URI.create(upstreamUrl)), combine its scheme/authority with a properly joined
path (base.getPath() + targetPath) and the query, then construct a new URI via
new URI(scheme, authority, combinedPath, query, null) and return its toString();
to avoid double-encoding, do not apply URLEncoder to targetPath if it already
contains percent-escapes (check for '%' or use URL-decoding only if necessary)
and only encode query parameter values before passing them in so the query
component is safe.

src/main/java/org/juv25d/filter/FilterScanner.java (1)

54-55: Use centralized logger instead of System.err for scanner errors.

Structured logging keeps severity, formatting, and routing consistent with the rest of the server.

♻️ Suggested refactor

+import org.juv25d.logging.ServerLogging;
+import java.util.logging.Logger;
@@
 public class FilterScanner {
+    private static final Logger logger = ServerLogging.getLogger();
@@
-                    System.err.println("Filter " + filterClass.getName()
-                        + " has both `@Global` and `@Route` — skipping. Use only one.");
+                    logger.severe("Filter " + filterClass.getName()
+                        + " has both `@Global` and `@Route`; skipping.");
@@
-                System.err.println("Skipping filter: "
-                    + filterClass.getName() + " due to " + e.getMessage());
+                logger.severe("Skipping filter " + filterClass.getName() + ": " + e.getMessage());

Also applies to: 89-90

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterScanner.java` around lines 54 - 55,
Replace the System.err.println calls in FilterScanner with the centralized
logger: add/ensure a Logger instance in the FilterScanner class (e.g., private
static final Logger logger = LoggerFactory.getLogger(FilterScanner.class)) and
change the two occurrences that print the skip message (the one using
filterClass.getName() around the check for `@Global` and `@Route`, and the
similar one at lines ~89-90) to use logger.warn or logger.error with the same
message string; also add the appropriate import for the logging framework used
by the project.

src/main/java/org/juv25d/router/RouterConfig.java (1)

28-28: Prefer structured logger over System.out.println.

Use the shared server logger here for consistent formatting/levels and better production observability.

♻️ Suggested refactor

+import org.juv25d.logging.ServerLogging;
+import java.util.logging.Logger;
@@
 public class RouterConfig {
+    private static final Logger logger = ServerLogging.getLogger();
@@
-        System.out.println("Router configured");
+        logger.info("Router configured");
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/router/RouterConfig.java` at line 28, Replace the
System.out.println in RouterConfig with the shared server logger: locate the
System.out.println("Router configured") call in class RouterConfig and change it
to use the project's shared logger (e.g., call serverLogger.info("Router
configured") or LOGGER.info(...) depending on the existing server logger
symbol). If RouterConfig doesn't yet reference the shared logger, add an
import/reference to the existing shared server logger instance and use its
info-level method to emit the message so formatting/levels remain consistent
with other services.

src/main/java/org/juv25d/Bootstrap.java (1)

33-35: Make RouterConfig pre-resolution explicit.

Line 34 resolves org.juv25d.router.RouterConfig but discards it. If this is intentional fail-fast/warm-up behavior, make it explicit with a named variable or comment.

Suggested clarity tweak

-        Router router = container.get(Router.class);
-        container.get(org.juv25d.router.RouterConfig.class);
+        Router router = container.get(Router.class);
+        // Fail fast: ensure router config is resolvable during bootstrap.
+        org.juv25d.router.RouterConfig routerConfig =
+            container.get(org.juv25d.router.RouterConfig.class);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/Bootstrap.java` around lines 33 - 35, The code calls
container.get(org.juv25d.router.RouterConfig.class) and discards the result
which obscures the intended pre-resolution/warm-up/fail-fast behavior; make this
explicit by assigning it to a clearly named local variable (e.g., RouterConfig
routerConfig = container.get(org.juv25d.router.RouterConfig.class)) or, if you
truly only need the side-effect, add a short inline comment above the call (or
rename the call to a dummy variable like var ignoredRouterConfig) referencing
RouterConfig to document the intentional pre-resolution in Bootstrap (where
Router is also resolved).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/org/juv25d/config/IpFilterConfig.java`:
- Around line 7-19: IpFilterConfig currently returns hardcoded defaults; replace
those constant-return methods with backed instance fields so the class can hold
real config: add private fields Set<String> whitelist, Set<String> blacklist and
boolean allowByDefault (remove final default), provide getters whitelist(),
blacklist(), allowByDefault() that return those fields, and add a constructor or
setters to populate them from configuration (or annotate the class with
`@ConfigurationProperties`(prefix="ipfilter") / `@Component` so Spring can bind
properties). Update any callers to construct or inject IpFilterConfig rather
than relying on the stubbed defaults.

In `@src/main/java/org/juv25d/ConnectionHandler.java`:
- Around line 54-60: The current try/catch only handles IOException and misses
unchecked failures from pipeline.createChain and chain.doFilter; update
ConnectionHandler's request handling to also catch RuntimeException (or catch
Exception/Throwable) around the pipeline invocation
(pipeline.createChain(request) and chain.doFilter(request, response)), log the
full exception via logger.log(Level.SEVERE, "Unhandled exception while handling
request", e) and ensure an HTTP 500 error response is sent via
HttpResponseWriter.write so the client receives a proper error response.

In `@src/main/java/org/juv25d/filter/RedirectFilter.java`:
- Around line 42-50: The constructor currently assigns the provided
List<RedirectRule> directly to the rules field and accepts null, allowing
external mutation and NPEs later; update both constructors (the
RedirectFilter(List<RedirectRule> rules) and the no-arg constructor that calls
new RedirectConfig().rules()) to defensively validate and snapshot the list:
call Objects.requireNonNull(rules, "rules") (or throw an explicit exception)
then set this.rules = List.copyOf(rules) so the internal list is immutable and
never null; apply the same pattern to the default rules obtained from
RedirectConfig().rules().

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java`:
- Around line 54-57: The log call in StaticFileHandler that currently records
the raw attacker-controlled path should be changed to sanitize or truncate the
input before logging: in the branch where isPathSafe(path) is false, replace the
direct usage of path in logger.warning with a sanitized/truncated representation
(e.g., limit length, escape CR/LF, or remove non-printable chars) and then call
logger.warning with that safe string while still returning
createErrorResponse(403, "Forbidden"); keep references to isPathSafe,
logger.warning, and createErrorResponse so the check and response behavior
remain unchanged.

In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 68-72: The loop in ConfigLoader that constructs ProxyRoute entries
uses String.valueOf(route.get(...)) which yields the literal "null" when keys
are missing; update the code in the method that populates this.proxyRoutes to
validate that route.get("base-route") and route.get("upstream-url") are non-null
and non-empty before creating new ProxyRoute instances (e.g., explicitly check
for null/empty, log or throw a clear configuration error referencing the
problematic route, and skip or fail fast instead of adding a ProxyRoute with the
string "null"); ensure the checks reference the "base-route" and "upstream-url"
keys and update behavior consistently where ProxyRoute is constructed.

In `@src/test/java/org/juv25d/PipelineTest.java`:
- Around line 1-4: The PipelineTest class is missing its package declaration;
add the package line for the org.juv25d package at the top of the file so the
class PipelineTest is declared under package org.juv25d, ensuring test discovery
and compilation work correctly.

---

Duplicate comments:
In `@Dockerfile`:
- Around line 12-19: The runtime image currently runs the JVM as root
(ENTRYPOINT ["java", "-cp", "classes:libs/*","org.juv25d.App"]); create an
unprivileged user/group, chown the WORKDIR and the copied directories (classes
and libs) to that user, and add a USER instruction to switch to it before
ENTRYPOINT so the JVM runs unprivileged; locate the Dockerfile entries
referencing WORKDIR, the copied targets (libs and classes) and ENTRYPOINT to add
the user creation, chown, and USER change.

In `@src/main/java/org/juv25d/filter/FilterMatcher.java`:
- Around line 38-40: In FilterMatcher where you handle pattern.endsWith("/*"),
the current logic builds prefix = pattern.substring(0, pattern.length() - 1)
which yields "/api/" for "/api/*" and therefore misses an exact "/api" match;
change the check to accept either the exact base path or any subpath by deriving
the base (pattern with trailing "/*" removed) and returning path.equals(base) ||
path.startsWith(base + "/") (or equivalently check
path.equals(prefixWithoutTrailingSlash) || path.startsWith(prefix) depending on
how you compute prefix) so "/api" and "/api/..." both match.

In `@src/main/java/org/juv25d/filter/FilterRegistry.java`:
- Around line 30-33: The shutdown() method has a race between the boolean check
and set (isShutdown) allowing concurrent callers to both proceed; fix by making
the entry atomic—replace the plain boolean guard with an atomic compare-and-set
or otherwise synchronize entry so only one caller performs destruction (e.g.,
use an AtomicBoolean isShutdown and call compareAndSet(false, true) at the start
of shutdown(), or synchronize the method and check isShutdown before
proceeding); update any references to isShutdown accordingly to reflect the
atomic guard.
- Around line 26-28: getRouteFilters() currently returns an unmodifiable map but
still exposes mutable inner lists; change it to return a deep unmodifiable
snapshot by copying each entry in the routes map and replacing each
List<FilterRegistration> value with an unmodifiable copy (e.g. new
ArrayList<>(originalList) wrapped with Collections.unmodifiableList) and then
wrapping the resulting map with Collections.unmodifiableMap before returning;
update the method that references the routes field (routes) and the
getRouteFilters() implementation accordingly so callers cannot mutate the inner
lists.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java`:
- Line 28: The buckets field (private final Map<String, Bucket> buckets) is
unbounded and can be abused to exhaust memory; replace it with a bounded,
expiring cache (e.g., Caffeine/Guava LoadingCache or Cache with maxSize and
expireAfterAccess) keyed by client IP and returning/creating Bucket instances
for RateLimitingFilter; update any accesses that call buckets.get/put to use
cache.get(key) or cache.getIfPresent and ensure thread-safe bucket
creation/eviction semantics are preserved and that metrics/logging still work
when entries are evicted.
- Around line 113-115: Bandwidth.classic(...) and Refill.intervally(...) may be
deprecated in newer Bucket4j versions; replace the call to
Bandwidth.classic(capacity, Refill.intervally(refillTokens, refillPeriod)) with
the builder-style API: create a Bandwidth via Bandwidth.builder(), set the
capacity and refill with the equivalent Refill (e.g.
Refill.intervally(refillTokens, refillPeriod) if available), then call .build()
— i.e. use
Bandwidth.builder().capacity(capacity).refill(Refill.intervally(refillTokens,
refillPeriod)).build() (or the exact .refill(...) variant the library exposes)
to restore compatibility for the code using Bandwidth.classic and
Refill.intervally.

In `@src/main/java/org/juv25d/logging/ServerLogging.java`:
- Around line 26-37: The code in ServerLogging calls levelName.toUpperCase()
without null-check, which can throw NPE before Level.parse is reached; fix by
guarding levelName (the variable computed from
System.getProperty/System.getenv/ConfigLoader.getLogLevel()) and defaulting it
to a safe value (e.g. "INFO") before calling toUpperCase and Level.parse, then
proceed to call Level.parse(...) and logger.setLevel(...), keeping the existing
catch for IllegalArgumentException for invalid values; update the logic around
Level.parse(levelName.toUpperCase()) and logger.setLevel(...) to use the
non-null default.

In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 16-23: Constructor allows null matcher/router which defers NPEs;
validate and fail fast in the Pipeline constructor by checking parameters (e.g.,
use Objects.requireNonNull or explicit checks) for matcher and router and throw
a clear exception (IllegalArgumentException or NullPointerException) with
descriptive message; this ensures createChain (which calls matcher.match and
constructs new FilterChainImpl(filters, router)) never runs with null
dependencies.

---

Nitpick comments:
In `@src/main/java/org/juv25d/Bootstrap.java`:
- Around line 33-35: The code calls
container.get(org.juv25d.router.RouterConfig.class) and discards the result
which obscures the intended pre-resolution/warm-up/fail-fast behavior; make this
explicit by assigning it to a clearly named local variable (e.g., RouterConfig
routerConfig = container.get(org.juv25d.router.RouterConfig.class)) or, if you
truly only need the side-effect, add a short inline comment above the call (or
rename the call to a dummy variable like var ignoredRouterConfig) referencing
RouterConfig to document the intentional pre-resolution in Bootstrap (where
Router is also resolved).

In `@src/main/java/org/juv25d/config/RedirectConfig.java`:
- Around line 9-15: The rules() method in RedirectConfig currently creates a new
immutable List and new RedirectRule instances on every call; change this by
declaring a private static final List<RedirectRule> (e.g. RULES) initialized
once with the RedirectRule(...) entries and have rules() simply return that
static list to avoid repeated allocations and make intent explicit; locate the
RedirectConfig class and replace the per-call construction in rules() with a
reference to the static final RULES constant.

In `@src/main/java/org/juv25d/filter/FilterChainImpl.java`:
- Around line 10-19: The FilterChainImpl is single-use because the mutable field
index tracks progress; fix by making a defensive, immutable copy of the incoming
filters in the constructor (in FilterChainImpl(List<Filter> filters, Router
router) assign this.filters = Collections.unmodifiableList(new
ArrayList<>(filters))) to prevent external mutation, and add class-level Javadoc
on FilterChainImpl documenting that instances are not reusable/are single-use
(and/or add a runtime guard in the filter dispatch method to throw
IllegalStateException if reused via the index field). Ensure references to the
fields filters and index remain consistent after the change.

In `@src/main/java/org/juv25d/filter/FilterScanner.java`:
- Around line 54-55: Replace the System.err.println calls in FilterScanner with
the centralized logger: add/ensure a Logger instance in the FilterScanner class
(e.g., private static final Logger logger =
LoggerFactory.getLogger(FilterScanner.class)) and change the two occurrences
that print the skip message (the one using filterClass.getName() around the
check for `@Global` and `@Route`, and the similar one at lines ~89-90) to use
logger.warn or logger.error with the same message string; also add the
appropriate import for the logging framework used by the project.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java`:
- Around line 215-221: In StaticFileHandler, avoid calling opaqueTag(...)
multiple times inside the loop: compute opaqueTag(currentEtag) once before the
loop and for each part cache opaqueTag(part) into a local variable (e.g.,
partTag) and compare partTag.equals(cachedCurrentTag); return true on match and
false after the loop to preserve behavior while improving clarity and minor
performance.

In `@src/main/java/org/juv25d/proxy/ProxyRoute.java`:
- Around line 21-26: buildUrl currently concatenates upstreamUrl, targetPath and
query which can produce malformed URLs for paths/queries with special chars;
change buildUrl to compose the URL with java.net.URI: parse upstreamUrl into a
URI (URI.create(upstreamUrl)), combine its scheme/authority with a properly
joined path (base.getPath() + targetPath) and the query, then construct a new
URI via new URI(scheme, authority, combinedPath, query, null) and return its
toString(); to avoid double-encoding, do not apply URLEncoder to targetPath if
it already contains percent-escapes (check for '%' or use URL-decoding only if
necessary) and only encode query parameter values before passing them in so the
query component is safe.

In `@src/main/java/org/juv25d/router/RouterConfig.java`:
- Line 28: Replace the System.out.println in RouterConfig with the shared server
logger: locate the System.out.println("Router configured") call in class
RouterConfig and change it to use the project's shared logger (e.g., call
serverLogger.info("Router configured") or LOGGER.info(...) depending on the
existing server logger symbol). If RouterConfig doesn't yet reference the shared
logger, add an import/reference to the existing shared server logger instance
and use its info-level method to emit the message so formatting/levels remain
consistent with other services.

In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 93-98: Handle non-numeric config values for requestsPerMinute and
burstCapacity by adding a safe parser (e.g., parseLongOrDefault) that accepts
Object, returns the Number.longValue() when value is a Number, attempts
Long.parseLong in a try/catch for strings, and falls back to the provided
default on failure; then replace the current Long.parseLong(String.valueOf(...))
calls that read from rateLimitingConfig with calls to
parseLongOrDefault(rateLimitingConfig.getOrDefault("requests-per-minute", 60L),
60L) and parseLongOrDefault(rateLimitingConfig.getOrDefault("burst-capacity",
100L), 100L) respectively to avoid NumberFormatException propagating.

In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java`:
- Around line 175-182: In the test method
shouldReturnNonEmptyBodyForSuccessfulRequest in StaticFileHandlerTest, remove
the redundant assertion assertThat(response.body().length).isGreaterThan(0)
because assertThat(response.body()).isNotEmpty() already covers that; leave the
single isNotEmpty() assertion to keep the test concise and clear while still
validating a non-empty response body from StaticFileHandler.handle(request).
- Around line 108-139: Add a test that covers URL-encoded traversal sequences to
ensure the handler blocks encoded attacks: create a new test (e.g.,
shouldReturn403ForUrlEncodedPathTraversal) that builds an HttpRequest via
createRequest with a path like "/%2e%2e/%2e%2e/etc/passwd" (and optionally
"/%2e%2e/%2f%2e%2e/etc/passwd") and calls StaticFileHandler.handle(request);
assert that the response.statusCode() is not 200 — prefer asserting it is either
403 or 404 (assertThat(response.statusCode()).isIn(403, 404)) to allow both
decoded-forbidden or not-found behavior, and include the test alongside the
existing traversal tests.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 338b354 and b01bc0f.

📒 Files selected for processing (42)

• Dockerfile
• src/main/java/org/juv25d/App.java
• src/main/java/org/juv25d/Bootstrap.java
• src/main/java/org/juv25d/ConnectionHandler.java
• src/main/java/org/juv25d/Pipeline.java
• src/main/java/org/juv25d/config/FilterConfig.java
• src/main/java/org/juv25d/config/IpFilterConfig.java
• src/main/java/org/juv25d/config/RateLimitConfig.java
• src/main/java/org/juv25d/config/RedirectConfig.java
• src/main/java/org/juv25d/di/Container.java
• src/main/java/org/juv25d/di/Inject.java
• src/main/java/org/juv25d/filter/ClassScanner.java
• src/main/java/org/juv25d/filter/Filter.java
• src/main/java/org/juv25d/filter/FilterChainImpl.java
• src/main/java/org/juv25d/filter/FilterFactory.java
• src/main/java/org/juv25d/filter/FilterMatcher.java
• src/main/java/org/juv25d/filter/FilterRegistration.java
• src/main/java/org/juv25d/filter/FilterRegistry.java
• src/main/java/org/juv25d/filter/FilterScanner.java
• src/main/java/org/juv25d/filter/IpFilter.java
• src/main/java/org/juv25d/filter/LoggingFilter.java
• src/main/java/org/juv25d/filter/RateLimitingFilter.java
• src/main/java/org/juv25d/filter/RedirectFilter.java
• src/main/java/org/juv25d/filter/TimingFilter.java
• src/main/java/org/juv25d/handler/StaticFileHandler.java
• src/main/java/org/juv25d/http/HttpStatus.java
• src/main/java/org/juv25d/logging/LogContext.java
• src/main/java/org/juv25d/logging/ServerLogFormatter.java
• src/main/java/org/juv25d/logging/ServerLogging.java
• src/main/java/org/juv25d/proxy/ProxyPlugin.java
• src/main/java/org/juv25d/proxy/ProxyRoute.java
• src/main/java/org/juv25d/router/RouterConfig.java
• src/main/java/org/juv25d/util/ConfigLoader.java
• src/main/resources/application-properties.yml
• src/test/java/org/juv25d/PipelineTest.java
• src/test/java/org/juv25d/di/ContainerTest.java
• src/test/java/org/juv25d/filter/FilterRegistryTest.java
• src/test/java/org/juv25d/filter/IpFilterTest.java
• src/test/java/org/juv25d/filter/LoggingFilterTest.java
• src/test/java/org/juv25d/handler/StaticFileHandlerTest.java
• src/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java
• src/test/java/org/juv25d/proxy/ProxyPluginTest.java

🚧 Files skipped from review as they are similar to previous changes (16)

• src/test/java/org/juv25d/filter/IpFilterTest.java
• src/main/java/org/juv25d/di/Inject.java
• src/main/java/org/juv25d/App.java
• src/main/java/org/juv25d/di/Container.java
• src/main/java/org/juv25d/config/FilterConfig.java
• src/main/java/org/juv25d/filter/FilterFactory.java
• src/main/java/org/juv25d/filter/LoggingFilter.java
• src/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java
• src/test/java/org/juv25d/filter/LoggingFilterTest.java
• src/test/java/org/juv25d/proxy/ProxyPluginTest.java
• src/main/resources/application-properties.yml
• src/main/java/org/juv25d/filter/FilterRegistration.java
• src/main/java/org/juv25d/filter/IpFilter.java
• src/test/java/org/juv25d/di/ContainerTest.java
• src/test/java/org/juv25d/filter/FilterRegistryTest.java
• src/main/java/org/juv25d/filter/Filter.java

[coderabbitai]

⚠️ Potential issue | 🟠 Major

IpFilterConfig is hardcoded and cannot carry real configuration values.

whitelist(), blacklist(), and allowByDefault() always return defaults, so this class cannot represent configured policy (including deny-by-default). That can leave IP filtering effectively permissive even when stricter settings are expected.

Proposed fix

 public class IpFilterConfig {
+    private final Set<String> whitelist;
+    private final Set<String> blacklist;
+    private final boolean allowByDefault;
 
-    public Set<String> whitelist() {
-        return Set.of();
-    }
-
-    public Set<String> blacklist() {
-        return Set.of();
-    }
-
-    private final boolean allowByDefault = true;
+    public IpFilterConfig() {
+        this(Set.of(), Set.of(), true);
+    }
+
+    public IpFilterConfig(Set<String> whitelist, Set<String> blacklist, boolean allowByDefault) {
+        this.whitelist = whitelist == null ? Set.of() : Set.copyOf(whitelist);
+        this.blacklist = blacklist == null ? Set.of() : Set.copyOf(blacklist);
+        this.allowByDefault = allowByDefault;
+    }
+
+    public Set<String> whitelist() {
+        return whitelist;
+    }
+
+    public Set<String> blacklist() {
+        return blacklist;
+    }
 
     public boolean allowByDefault() {
         return allowByDefault;
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/config/IpFilterConfig.java` around lines 7 - 19,
IpFilterConfig currently returns hardcoded defaults; replace those
constant-return methods with backed instance fields so the class can hold real
config: add private fields Set<String> whitelist, Set<String> blacklist and
boolean allowByDefault (remove final default), provide getters whitelist(),
blacklist(), allowByDefault() that return those fields, and add a constructor or
setters to populate them from configuration (or annotate the class with
`@ConfigurationProperties`(prefix="ipfilter") / `@Component` so Spring can bind
properties). Update any callers to construct or inject IpFilterConfig rather
than relying on the stubbed defaults.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Unhandled runtime failures from pipeline/filter chain are not caught here.

Catching only IOException misses unchecked exceptions from createChain/doFilter, which can terminate request handling without local logging.

♻️ Suggested fix

-        } catch (IOException e) {
+        } catch (Exception e) {
             logger.log(Level.SEVERE, "Error while handling request", e);
         } finally {

📝 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.

Suggested change

	var chain = pipeline.createChain(request);
	chain.doFilter(request, response);
	HttpResponseWriter.write(out, response);
	} catch (IOException e) {
	logger.log(Level.SEVERE, "Error while handling request", e);
	var chain = pipeline.createChain(request);
	chain.doFilter(request, response);
	HttpResponseWriter.write(out, response);
	} catch (Exception e) {
	logger.log(Level.SEVERE, "Error while handling request", e);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/ConnectionHandler.java` around lines 54 - 60, The
current try/catch only handles IOException and misses unchecked failures from
pipeline.createChain and chain.doFilter; update ConnectionHandler's request
handling to also catch RuntimeException (or catch Exception/Throwable) around
the pipeline invocation (pipeline.createChain(request) and
chain.doFilter(request, response)), log the full exception via
logger.log(Level.SEVERE, "Unhandled exception while handling request", e) and
ensure an HTTP 500 error response is sent via HttpResponseWriter.write so the
client receives a proper error response.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Defensively validate/copy redirect rules at construction time.

Directly storing caller-provided rules allows external mutation and accepts null, which can fail later during request processing. Fail fast and snapshot with List.copyOf(...).

🛡️ Proposed fix

 import java.io.IOException;
 import java.util.List;
+import java.util.Objects;
 import java.util.logging.Logger;
@@
 public class RedirectFilter implements Filter {
     private final List<RedirectRule> rules;
-    private final Logger logger;
+    private static final Logger LOGGER = Logger.getLogger(RedirectFilter.class.getName());
@@
     public RedirectFilter(List<RedirectRule> rules) {
-        this.rules = rules;
-        this.logger = Logger.getLogger(RedirectFilter.class.getName());
+        this.rules = List.copyOf(Objects.requireNonNull(rules, "rules must not be null"));
     }
 
     public RedirectFilter() {
-        this.rules = new RedirectConfig().rules();
-        this.logger = Logger.getLogger(RedirectFilter.class.getName());
+        this(new RedirectConfig().rules());
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RedirectFilter.java` around lines 42 - 50,
The constructor currently assigns the provided List<RedirectRule> directly to
the rules field and accepts null, allowing external mutation and NPEs later;
update both constructors (the RedirectFilter(List<RedirectRule> rules) and the
no-arg constructor that calls new RedirectConfig().rules()) to defensively
validate and snapshot the list: call Objects.requireNonNull(rules, "rules") (or
throw an explicit exception) then set this.rules = List.copyOf(rules) so the
internal list is immutable and never null; apply the same pattern to the default
rules obtained from RedirectConfig().rules().

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Minor: Logging attacker-controlled input may pollute logs.

Line 55 logs the raw path value directly. While not exploitable in most modern logging frameworks, this can clutter logs with attack payloads and potentially aid log-based attacks (e.g., CRLF injection in older setups). Consider truncating or sanitizing the path before logging.

🛡️ Suggested mitigation

-            logger.warning("Security violation: Attempted path traversal with path: " + path);
+            logger.warning("Security violation: Attempted path traversal with path: " + 
+                (path.length() > 100 ? path.substring(0, 100) + "..." : path).replaceAll("[\\r\\n]", ""));

📝 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.

Suggested change

	if (!isPathSafe(path)) {
	logger.warning("Security violation: Attempted path traversal with path: " + path);
	return createErrorResponse(403, "Forbidden");
	}
	if (!isPathSafe(path)) {
	logger.warning("Security violation: Attempted path traversal with path: " +
	(path.length() > 100 ? path.substring(0, 100) + "..." : path).replaceAll("[\\r\\n]", ""));
	return createErrorResponse(403, "Forbidden");
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 54 -
57, The log call in StaticFileHandler that currently records the raw
attacker-controlled path should be changed to sanitize or truncate the input
before logging: in the branch where isPathSafe(path) is false, replace the
direct usage of path in logger.warning with a sanitized/truncated representation
(e.g., limit length, escape CR/LF, or remove non-printable chars) and then call
logger.warning with that safe string while still returning
createErrorResponse(403, "Forbidden"); keep references to isPathSafe,
logger.warning, and createErrorResponse so the check and response behavior
remain unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

String.valueOf(null) produces literal "null" string for missing route keys.

If base-route or upstream-url is absent from a route entry, String.valueOf(route.get(...)) returns the string "null", creating an invalid ProxyRoute. Consider adding null checks or validation.

🛡️ Proposed fix with validation

                     for (Map<String, Object> route : routes) {
-                        String baseRoute = String.valueOf(route.get("base-route"));
-                        String upstreamUrl = String.valueOf(route.get("upstream-url"));
-                        this.proxyRoutes.add(new ProxyRoute(baseRoute, upstreamUrl));
+                        Object baseObj = route.get("base-route");
+                        Object upstreamObj = route.get("upstream-url");
+                        if (baseObj != null && upstreamObj != null) {
+                            this.proxyRoutes.add(new ProxyRoute(
+                                String.valueOf(baseObj),
+                                String.valueOf(upstreamObj)
+                            ));
+                        }
                     }

📝 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.

Suggested change

	for (Map<String, Object> route : routes) {
	String baseRoute = String.valueOf(route.get("base-route"));
	String upstreamUrl = String.valueOf(route.get("upstream-url"));
	this.proxyRoutes.add(new ProxyRoute(baseRoute, upstreamUrl));
	}
	for (Map<String, Object> route : routes) {
	Object baseObj = route.get("base-route");
	Object upstreamObj = route.get("upstream-url");
	if (baseObj != null && upstreamObj != null) {
	this.proxyRoutes.add(new ProxyRoute(
	String.valueOf(baseObj),
	String.valueOf(upstreamObj)
	));
	}
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 68 - 72, The
loop in ConfigLoader that constructs ProxyRoute entries uses
String.valueOf(route.get(...)) which yields the literal "null" when keys are
missing; update the code in the method that populates this.proxyRoutes to
validate that route.get("base-route") and route.get("upstream-url") are non-null
and non-empty before creating new ProxyRoute instances (e.g., explicitly check
for null/empty, log or throw a clear configuration error referencing the
problematic route, and skip or fail fast instead of adding a ProxyRoute with the
string "null"); ensure the checks reference the "base-route" and "upstream-url"
keys and update behavior consistently where ProxyRoute is constructed.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Missing package declaration will cause compilation/test discovery issues.

The test class lacks a package declaration, which will prevent proper test discovery and may cause compilation errors depending on the build configuration. Based on the file path, this should be in org.juv25d.

🛠️ Proposed fix

+package org.juv25d;
+
 import static org.junit.jupiter.api.Assertions.*;
+
 class PipelineTest {

 }

📝 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.

Suggested change

	import static org.junit.jupiter.api.Assertions.*;
	class PipelineTest {
	}
	package org.juv25d;
	import static org.junit.jupiter.api.Assertions.*;
	class PipelineTest {
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/PipelineTest.java` around lines 1 - 4, The
PipelineTest class is missing its package declaration; add the package line for
the org.juv25d package at the top of the file so the class PipelineTest is
declared under package org.juv25d, ensuring test discovery and compilation work
correctly.

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (7)

src/main/java/org/juv25d/handler/StaticFileHandler.java (2)

54-56: ⚠️ Potential issue | 🟡 Minor

Sanitize attacker-controlled path before logging.

Line 55 logs raw request input; strip CR/LF and truncate before logging to avoid log pollution.

🛡️ Proposed fix

+    private static String sanitizeForLog(String path) {
+        if (path == null) return "null";
+        String cleaned = path.replaceAll("[\\r\\n]", "");
+        return cleaned.length() > 120 ? cleaned.substring(0, 120) + "..." : cleaned;
+    }
+
     // Validate path for security
     if (!isPathSafe(path)) {
-        logger.warning("Security violation: Attempted path traversal with path: " + path);
+        logger.warning("Security violation: Attempted path traversal with path: " + sanitizeForLog(path));
         return createErrorResponse(403, "Forbidden");
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 54 -
56, The logger.warning call in StaticFileHandler (inside the isPathSafe check)
logs the raw attacker-controlled path; sanitize the path before logging by
removing CR/LF characters and truncating it to a safe max length (e.g., 200
chars) and then log the sanitized value instead; update the warning invocation
that references path to use the sanitized variable and keep the return via
createErrorResponse(403, "Forbidden") unchanged.

---

227-250: ⚠️ Potential issue | 🔴 Critical

Reflected XSS in 404 page: escape the reflected path.

Line 247/250 injects raw path into HTML. A crafted URL can execute script in the browser.

🛡️ Proposed fix

+    private static String htmlEncode(String input) {
+        if (input == null) return "";
+        return input
+            .replace("&", "&")
+            .replace("<", "&lt;")
+            .replace(">", "&gt;")
+            .replace("\"", "&quot;")
+            .replace("'", "&#x27;");
+    }
+
     private static HttpResponse createNotFoundResponse(String path) {
@@
-                """.formatted(path);
+                """.formatted(htmlEncode(path));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 227 -
250, The 404 page reflects the raw path into HTML causing reflected XSS; in
createNotFoundResponse, HTML-escape the path before inserting it into the HTML
template (don't inject the raw path). Update createNotFoundResponse to sanitize
the variable path (e.g., via
org.apache.commons.text.StringEscapeUtils.escapeHtml4 or an equivalent
HTML-entity encoder) and use the escaped value in the formatted HTML so special
characters like <, >, & and quotes cannot execute as markup or script.

src/main/java/org/juv25d/Pipeline.java (1)

16-23: ⚠️ Potential issue | 🟠 Major

Fail fast on null pipeline dependencies.

matcher, router, or request being null will crash later in the request path. Validate at construction and chain creation time to surface errors immediately.

Proposed fix

 import java.util.List;
+import java.util.Objects;
@@
     private final FilterMatcher matcher;
     private final Router router;
 
     public Pipeline(FilterMatcher matcher, Router router) {
-        this.matcher = matcher;
-        this.router = router;
+        this.matcher = Objects.requireNonNull(matcher, "matcher must not be null");
+        this.router = Objects.requireNonNull(router, "router must not be null");
     }
 
     public FilterChainImpl createChain(HttpRequest request) {
-        List<Filter> filters = matcher.match(request);
+        HttpRequest nonNullRequest = Objects.requireNonNull(request, "request must not be null");
+        List<Filter> filters = matcher.match(nonNullRequest);
         return new FilterChainImpl(filters, router);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/Pipeline.java` around lines 16 - 23, Validate
non-null dependencies in Pipeline: in the Pipeline constructor check that
matcher and router are not null and throw a clear
NullPointerException/IllegalArgumentException with a descriptive message if they
are; likewise, in createChain(HttpRequest request) validate that request is not
null before calling matcher.match(request) and throw a descriptive exception if
it is, ensuring errors surface immediately rather than crashing later when
FilterChainImpl is constructed with matcher/router/request.

src/main/java/org/juv25d/filter/TimingFilter.java (1)

17-23: ⚠️ Potential issue | 🟠 Major

Ensure timing is logged even when downstream processing throws.

If chain.doFilter(req, res) throws, the duration log is skipped. Wrap the call in try/finally so timing is always emitted.

Proposed fix

     `@Override`
     public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException {
         long start = System.nanoTime();
-
-        chain.doFilter(req, res);
-
-        long durationMs = (System.nanoTime() - start) / 1_000_000;
-        logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
+        try {
+            chain.doFilter(req, res);
+        } finally {
+            long durationMs = (System.nanoTime() - start) / 1_000_000;
+            logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
+        }
 
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/TimingFilter.java` around lines 17 - 23, In
doFilter of TimingFilter, ensure the duration log is emitted even if
chain.doFilter(req, res) throws by wrapping chain.doFilter(req, res) in a
try/finally: record start with System.nanoTime(), call chain.doFilter(...)
inside the try, and compute/log duration in the finally using
logger.info(req.method() + " " + req.path() + " took " + durationMs + " ms");
keep exception propagation unchanged (do not catch/suppress exceptions).

src/main/java/org/juv25d/filter/RedirectFilter.java (1)

42-49: ⚠️ Potential issue | 🟠 Major

Defensively validate and snapshot redirect rules in constructors.

Direct assignment allows external mutation and accepts null, which can fail later in doFilter. Copy and null-check at construction time.

Proposed fix

 import java.io.IOException;
 import java.util.List;
+import java.util.Objects;
 import java.util.logging.Logger;
@@
     public RedirectFilter(List<RedirectRule> rules) {
-        this.rules = rules;
+        this.rules = List.copyOf(Objects.requireNonNull(rules, "rules must not be null"));
         this.logger = Logger.getLogger(RedirectFilter.class.getName());
     }
 
     public RedirectFilter() {
-        this.rules = new RedirectConfig().rules();
-        this.logger = Logger.getLogger(RedirectFilter.class.getName());
+        this(new RedirectConfig().rules());
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RedirectFilter.java` around lines 42 - 49,
The constructors of RedirectFilter should defensively validate and snapshot the
incoming rules to avoid nulls and external mutation: in both
RedirectFilter(List<RedirectRule> rules) and the no-arg constructor (which uses
new RedirectConfig().rules()), check for null and throw IllegalArgumentException
or substitute an empty list, then assign a defensive copy (e.g., new
ArrayList<>(rules)) to the instance field instead of assigning the passed list
directly so doFilter uses an immutable snapshot of rules; keep existing Logger
initialization unchanged.

src/main/java/org/juv25d/filter/FilterRegistration.java (1)

7-8: ⚠️ Potential issue | 🟠 Major

compareTo should not collapse distinct registrations on equal order.

Current implementation returns 0 for different records sharing the same order, which can cause incorrect behavior in natural-order contexts (e.g., TreeSet/TreeMap).

Proposed fix

 package org.juv25d.filter;
 
+import java.util.Comparator;
+
 public record FilterRegistration(Filter filter, int order, String pattern)
     implements Comparable<FilterRegistration> {
 
     `@Override`
     public int compareTo(FilterRegistration o) {
-        return Integer.compare(this.order, o.order);
+        int cmp = Integer.compare(this.order, o.order);
+        if (cmp != 0) return cmp;
+        cmp = Comparator.nullsFirst(String::compareTo).compare(this.pattern, o.pattern);
+        if (cmp != 0) return cmp;
+        return Integer.compare(System.identityHashCode(this.filter), System.identityHashCode(o.filter));
     }
 }

Verification script (read-only)

#!/bin/bash
set -euo pipefail

echo "== FilterRegistration natural-order usage =="
rg -nP --type=java '\bFilterRegistration\b'

echo
echo "== Potential sorted structures/calls that may rely on compareTo =="
rg -nP --type=java 'Tree(Set|Map)<\s*FilterRegistration|Sorted(Set|Map)<\s*FilterRegistration|new\s+Tree(Set|Map)\s*<\s*FilterRegistration|Collections\.sort\(|\.sorted\(\)'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterRegistration.java` around lines 7 - 8,
The compareTo implementation in FilterRegistration currently returns 0 when two
instances share the same order, collapsing distinct registrations; update
FilterRegistration.compareTo to preserve a strict weak ordering by first
comparing this.order to o.order and, if equal, using a deterministic tie-breaker
(e.g., compare System.identityHashCode(this) to System.identityHashCode(o) or
another unique field on FilterRegistration such as an id/name if available) so
that equal orders do not produce 0 for different instances.

src/main/java/org/juv25d/filter/RateLimitingFilter.java (1)

28-28: ⚠️ Potential issue | 🟠 Major

Per-IP bucket store is unbounded and can be abused for memory growth.

buckets never evicts entries, so high-cardinality client IPs can grow memory without limit. Use a bounded/expiring cache (or equivalent eviction policy) instead of a plain ConcurrentHashMap.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` at line 28, The
per-IP bucket map in RateLimitingFilter (variable buckets) is unbounded and can
cause memory growth; replace the ConcurrentHashMap<String, Bucket> with a
bounded/expiring cache (e.g., Caffeine/Guava Cache) configured with a sensible
maximumSize and expireAfterAccess/expireAfterWrite, and update usages of
buckets.get/put to use cache.get(key, k -> createBucket(...)) or
cache.getIfPresent/put accordingly so entries are evicted automatically; ensure
the Bucket-creation helper (the existing bucket factory logic) is reused when
populating the cache.

🧹 Nitpick comments (8)

src/main/java/org/juv25d/logging/LogContext.java (1)

3-4: Consider adding a private constructor to prevent instantiation.

This utility class contains only static methods. Adding a private constructor is a common Java idiom to signal that instantiation is not intended.

Suggested fix

 public class LogContext {
+    private LogContext() {
+        // Utility class
+    }
+
     private static final ThreadLocal<String> connectionId = new ThreadLocal<>();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/logging/LogContext.java` around lines 3 - 4,
LogContext is a static-only utility class (it declares private static
ThreadLocal<String> connectionId) and should not be instantiable; add a private
constructor to class LogContext (e.g., private LogContext() { throw new
AssertionError("No instances."); }) to prevent construction and signal intent,
leaving existing static fields/methods unchanged.

src/main/java/org/juv25d/util/ConfigLoader.java (2)

31-36: Consider adding a resetInstance() method for test isolation.

The singleton pattern prevents resetting the shared instance between tests. While the package-private constructor allows creating separate test instances, tests using getInstance() will share state. A package-private reset method would improve test isolation.

💡 Optional: Add test reset method

     public static synchronized ConfigLoader getInstance() {
         if (instance == null) {
             instance = new ConfigLoader();
         }
         return instance;
     }

+    // For testing only - allows resetting the singleton between tests
+    static synchronized void resetInstance() {
+        instance = null;
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 31 - 36, Add a
package-private synchronized reset method to ConfigLoader to clear the singleton
for tests: implement a method named resetInstance() (package-private) that sets
the static instance field to null in a synchronized block (matching the
synchronization used by getInstance()) so tests that call
ConfigLoader.getInstance() can reset shared state between runs; reference the
static instance field and the getInstance() method when adding this helper.

---

22-28: Minor: Unconventional brace placement.

The closing braces on the same line as the method call are unconventional and reduce readability. Consider moving them to separate lines for consistency with Java conventions.

     private ConfigLoader() {
         loadConfiguration(getClass().getClassLoader()
-            .getResourceAsStream("application-properties.yml")); }
+            .getResourceAsStream("application-properties.yml"));
+    }

     // new constructor for testing
     ConfigLoader(InputStream input) {
-        loadConfiguration(input); }
+        loadConfiguration(input);
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 22 - 28,
Reformat the constructor brace placement in the ConfigLoader class so closing
braces are on their own lines: update the private ConfigLoader() that calls
loadConfiguration(getClass().getClassLoader().getResourceAsStream("application-properties.yml"))
and the package-private ConfigLoader(InputStream input) that calls
loadConfiguration(input) to move their terminating braces to separate lines,
preserving the existing logic and references to loadConfiguration and the
resource stream call.

src/main/java/org/juv25d/di/Container.java (1)

162-172: Consider making the lock timeout configurable.

The 5-second timeout is hardcoded. For applications with complex dependency graphs or slow initialization, this may be insufficient. Consider allowing this to be configured via the constructor or a constant.

Suggested enhancement

+    private static final long LOCK_TIMEOUT_SECONDS = 5;
+
     // In resolve method:
-            acquired = lock.tryLock(5, java.util.concurrent.TimeUnit.SECONDS);
+            acquired = lock.tryLock(LOCK_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/di/Container.java` around lines 162 - 172, The
hardcoded 5-second timeout in the Container lock acquisition
(locks.computeIfAbsent, ReentrantLock lock and lock.tryLock(5,
java.util.concurrent.TimeUnit.SECONDS)) should be made configurable: add a
configurable field (e.g., lockTimeout and lockTimeoutUnit) on the Container
class settable via constructor or builder, replace the literal 5 and
TimeUnit.SECONDS in the tryLock call with these fields, and ensure any existing
constructors are updated to provide a default value so behavior is unchanged
unless explicitly overridden; keep the existing RuntimeException path that
references resolving.get() for diagnostics.

src/main/java/org/juv25d/proxy/ProxyPlugin.java (1)

128-138: Consider using a Set for cleaner restricted header lookup.

The current implementation is correct, but using a Set would be more maintainable and potentially faster.

♻️ Optional refactor using Set

+    private static final Set<String> RESTRICTED_HEADERS = Set.of(
+        "connection", "content-length", "host", "upgrade",
+        "http2-settings", "te", "trailer", "transfer-encoding"
+    );
+
     private boolean isRestrictedHeader(String headerName) {
-        String lower = headerName.toLowerCase();
-        return lower.equals("connection") ||
-            lower.equals("content-length") ||
-            lower.equals("host") ||
-            lower.equals("upgrade") ||
-            lower.equals("http2-settings") ||
-            lower.equals("te") ||
-            lower.equals("trailer") ||
-            lower.equals("transfer-encoding");
+        return RESTRICTED_HEADERS.contains(headerName.toLowerCase());
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/proxy/ProxyPlugin.java` around lines 128 - 138,
Replace the series of string equals checks in ProxyPlugin.isRestrictedHeader
with a single static final Set<String> of lowercased restricted header names
(e.g., RESTRICTED_HEADERS) and change the method to return
RESTRICTED_HEADERS.contains(headerName.toLowerCase()); ensure the Set is
unmodifiable and initialized once (static) so lookups are cleaner and faster
while preserving the same header list
("connection","content-length","host","upgrade","http2-settings","te","trailer","transfer-encoding").

src/test/java/org/juv25d/handler/StaticFileHandlerTest.java (1)

89-96: Add a regression test for HTML-escaped path in 404 body.

Given the reflected path behavior in the handler, this suite should assert that user path content is escaped.

🧪 Suggested test addition

+    `@Test`
+    void shouldEscapeUserSuppliedPathIn404Body() {
+        HttpRequest request = createRequest("GET", "/<script>alert(1)</script>.html");
+        HttpResponse response = StaticFileHandler.handle(request);
+
+        String body = new String(response.body(), java.nio.charset.StandardCharsets.UTF_8);
+        assertThat(response.statusCode()).isEqualTo(404);
+        assertThat(body).contains("&lt;script&gt;alert(1)&lt;/script&gt;");
+        assertThat(body).doesNotContain("<script>alert(1)</script>");
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java` around lines 89
- 96, Update the test in StaticFileHandlerTest (method
shouldReturn404ResponseWithHtmlContent) to assert that any reflected request
path is HTML-escaped in the 404 body: call createRequest with a path containing
characters that need escaping (e.g., "/<script>alert()</script>.html" or
"/a&b.html"), pass it to StaticFileHandler.handle(request), then assert
response.body() contains the escaped form (e.g.,
"&lt;script&gt;alert()&lt;/script&gt;" or "a&amp;b") and does not contain the
raw unescaped characters; keep using createRequest and StaticFileHandler.handle
to locate the change.

src/main/java/org/juv25d/filter/FilterScanner.java (2)

87-91: Consider using java.util.logging.Logger instead of System.err.

Other classes in this codebase (e.g., ClassScanner, RateLimitingFilter) use java.util.logging.Logger. Using System.err here is inconsistent and harder to control in production.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterScanner.java` around lines 87 - 91, The
catch block in FilterScanner that currently writes to System.err should use a
java.util.logging.Logger for consistency and controllable logging; add or reuse
a private static final Logger (e.g., Logger logger =
Logger.getLogger(FilterScanner.class.getName())) in the FilterScanner class and
replace the System.err.println call in the catch for Exception e with
logger.log(Level.WARNING, "Skipping filter: " + filterClass.getName(), e) (or an
equivalent message and level) so the filterClass and exception are logged
consistently with ClassScanner and RateLimitingFilter.

---

63-85: Consider using else if for mutually exclusive annotation handling.

Since classes with both @Global and @Route are already skipped (lines 53-57), the two if blocks are mutually exclusive. Using else if would make this explicit and prevent accidental double-initialization if the guard is ever modified.

                 // Handle global filters
                 if (isGlobal) {
                     Global g = filterClass.getAnnotation(Global.class);
                     params.put("scope", "global");
                     params.put("order", String.valueOf(g.order()));

                     filter.init(new FilterConfig(params));
                     registry.registerGlobal(filter, g.order());
                 }

                 // Handle route-specific filters
-                if (isRoute) {
+                else if (isRoute) {
                     Route r = filterClass.getAnnotation(Route.class);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/filter/FilterScanner.java` around lines 63 - 85, In
FilterScanner, the two mutually exclusive annotation handlers for Global and
Route can both run; change the second "if (isRoute)" to "else if (isRoute)" so a
filterClass annotated only one way is initialized and registered exactly once;
ensure you keep the same logic that reads annotations (Global g =
filterClass.getAnnotation(Global.class) and Route r =
filterClass.getAnnotation(Route.class)), call filter.init(new
FilterConfig(params)) only once per branch, and continue to use
registry.registerGlobal(filter, g.order()) and registry.registerRoute(filter,
r.order(), pattern) as before.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/org/juv25d/handler/StaticFileHandler.java`:
- Around line 96-99: In StaticFileHandler, don't map every IOException to a 404:
change the catch logic around the resource-read code (the block that currently
logs "File not found: " + resourcePath and returns createNotFoundResponse(path))
to only treat not-found errors as 404 (e.g., catch
NoSuchFileException/FileNotFoundException or check Files.exists/resource ==
null), and for other IOExceptions log the full exception and return an
internal-server-error response (e.g., a new or existing
createInternalServerErrorResponse or createServerErrorResponse). Apply the same
fix to the second occurrence referenced by the comment (the block around lines
155-160) so missing resources return createNotFoundResponse(path) while other IO
failures return a 500 and include exception details in the log.

In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 84-98: Set sane defaults for requestsPerMinute and burstCapacity
before reading the optional rate-limiting section so they aren’t left as 0 when
the section is absent: initialize requestsPerMinute and burstCapacity (alongside
rateLimitingEnabled) to the desired default values at the top of the
rate-limiting block, then keep the existing
asStringObjectMap(config.get("rate-limiting")) check and parsing logic that
updates requestsPerMinute and burstCapacity if the map is present; refer to the
fields requestsPerMinute, burstCapacity, rateLimitingEnabled and the helper
asStringObjectMap to make the change.

In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java`:
- Around line 94-96: The test in StaticFileHandlerTest uses platform-default
decoding for response.body(); change calls like new String(response.body()) to
new String(response.body(), StandardCharsets.UTF_8) to explicitly decode as
UTF-8, and add the import for java.nio.charset.StandardCharsets; update all
similar assertions in the class (e.g., the assertions that check for "404" and
"Not Found") to use the UTF-8-aware decoding.

In `@src/test/java/org/juv25d/proxy/ProxyPluginTest.java`:
- Around line 156-180: The test ProxyPluginTest::filtersRestrictedHeaders
currently only asserts status code; capture the outgoing
java.net.http.HttpRequest passed into httpClient.send (use an ArgumentCaptor or
verify with an argument matcher against httpClient.send) after calling
proxyPlugin.handle(req, res) and assert that the captured request's headers do
not contain "Connection" or "Host" while still containing "Content-Type" as
"application/json"; reference the test method name filtersRestrictedHeaders, the
proxyPlugin.handle(...) call, and the mocked httpClient.send(...) invocation
when locating where to add the assertions.

---

Duplicate comments:
In `@src/main/java/org/juv25d/filter/FilterRegistration.java`:
- Around line 7-8: The compareTo implementation in FilterRegistration currently
returns 0 when two instances share the same order, collapsing distinct
registrations; update FilterRegistration.compareTo to preserve a strict weak
ordering by first comparing this.order to o.order and, if equal, using a
deterministic tie-breaker (e.g., compare System.identityHashCode(this) to
System.identityHashCode(o) or another unique field on FilterRegistration such as
an id/name if available) so that equal orders do not produce 0 for different
instances.

In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java`:
- Line 28: The per-IP bucket map in RateLimitingFilter (variable buckets) is
unbounded and can cause memory growth; replace the ConcurrentHashMap<String,
Bucket> with a bounded/expiring cache (e.g., Caffeine/Guava Cache) configured
with a sensible maximumSize and expireAfterAccess/expireAfterWrite, and update
usages of buckets.get/put to use cache.get(key, k -> createBucket(...)) or
cache.getIfPresent/put accordingly so entries are evicted automatically; ensure
the Bucket-creation helper (the existing bucket factory logic) is reused when
populating the cache.

In `@src/main/java/org/juv25d/filter/RedirectFilter.java`:
- Around line 42-49: The constructors of RedirectFilter should defensively
validate and snapshot the incoming rules to avoid nulls and external mutation:
in both RedirectFilter(List<RedirectRule> rules) and the no-arg constructor
(which uses new RedirectConfig().rules()), check for null and throw
IllegalArgumentException or substitute an empty list, then assign a defensive
copy (e.g., new ArrayList<>(rules)) to the instance field instead of assigning
the passed list directly so doFilter uses an immutable snapshot of rules; keep
existing Logger initialization unchanged.

In `@src/main/java/org/juv25d/filter/TimingFilter.java`:
- Around line 17-23: In doFilter of TimingFilter, ensure the duration log is
emitted even if chain.doFilter(req, res) throws by wrapping chain.doFilter(req,
res) in a try/finally: record start with System.nanoTime(), call
chain.doFilter(...) inside the try, and compute/log duration in the finally
using logger.info(req.method() + " " + req.path() + " took " + durationMs + "
ms"); keep exception propagation unchanged (do not catch/suppress exceptions).

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java`:
- Around line 54-56: The logger.warning call in StaticFileHandler (inside the
isPathSafe check) logs the raw attacker-controlled path; sanitize the path
before logging by removing CR/LF characters and truncating it to a safe max
length (e.g., 200 chars) and then log the sanitized value instead; update the
warning invocation that references path to use the sanitized variable and keep
the return via createErrorResponse(403, "Forbidden") unchanged.
- Around line 227-250: The 404 page reflects the raw path into HTML causing
reflected XSS; in createNotFoundResponse, HTML-escape the path before inserting
it into the HTML template (don't inject the raw path). Update
createNotFoundResponse to sanitize the variable path (e.g., via
org.apache.commons.text.StringEscapeUtils.escapeHtml4 or an equivalent
HTML-entity encoder) and use the escaped value in the formatted HTML so special
characters like <, >, & and quotes cannot execute as markup or script.

In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 16-23: Validate non-null dependencies in Pipeline: in the Pipeline
constructor check that matcher and router are not null and throw a clear
NullPointerException/IllegalArgumentException with a descriptive message if they
are; likewise, in createChain(HttpRequest request) validate that request is not
null before calling matcher.match(request) and throw a descriptive exception if
it is, ensuring errors surface immediately rather than crashing later when
FilterChainImpl is constructed with matcher/router/request.

---

Nitpick comments:
In `@src/main/java/org/juv25d/di/Container.java`:
- Around line 162-172: The hardcoded 5-second timeout in the Container lock
acquisition (locks.computeIfAbsent, ReentrantLock lock and lock.tryLock(5,
java.util.concurrent.TimeUnit.SECONDS)) should be made configurable: add a
configurable field (e.g., lockTimeout and lockTimeoutUnit) on the Container
class settable via constructor or builder, replace the literal 5 and
TimeUnit.SECONDS in the tryLock call with these fields, and ensure any existing
constructors are updated to provide a default value so behavior is unchanged
unless explicitly overridden; keep the existing RuntimeException path that
references resolving.get() for diagnostics.

In `@src/main/java/org/juv25d/filter/FilterScanner.java`:
- Around line 87-91: The catch block in FilterScanner that currently writes to
System.err should use a java.util.logging.Logger for consistency and
controllable logging; add or reuse a private static final Logger (e.g., Logger
logger = Logger.getLogger(FilterScanner.class.getName())) in the FilterScanner
class and replace the System.err.println call in the catch for Exception e with
logger.log(Level.WARNING, "Skipping filter: " + filterClass.getName(), e) (or an
equivalent message and level) so the filterClass and exception are logged
consistently with ClassScanner and RateLimitingFilter.
- Around line 63-85: In FilterScanner, the two mutually exclusive annotation
handlers for Global and Route can both run; change the second "if (isRoute)" to
"else if (isRoute)" so a filterClass annotated only one way is initialized and
registered exactly once; ensure you keep the same logic that reads annotations
(Global g = filterClass.getAnnotation(Global.class) and Route r =
filterClass.getAnnotation(Route.class)), call filter.init(new
FilterConfig(params)) only once per branch, and continue to use
registry.registerGlobal(filter, g.order()) and registry.registerRoute(filter,
r.order(), pattern) as before.

In `@src/main/java/org/juv25d/logging/LogContext.java`:
- Around line 3-4: LogContext is a static-only utility class (it declares
private static ThreadLocal<String> connectionId) and should not be instantiable;
add a private constructor to class LogContext (e.g., private LogContext() {
throw new AssertionError("No instances."); }) to prevent construction and signal
intent, leaving existing static fields/methods unchanged.

In `@src/main/java/org/juv25d/proxy/ProxyPlugin.java`:
- Around line 128-138: Replace the series of string equals checks in
ProxyPlugin.isRestrictedHeader with a single static final Set<String> of
lowercased restricted header names (e.g., RESTRICTED_HEADERS) and change the
method to return RESTRICTED_HEADERS.contains(headerName.toLowerCase()); ensure
the Set is unmodifiable and initialized once (static) so lookups are cleaner and
faster while preserving the same header list
("connection","content-length","host","upgrade","http2-settings","te","trailer","transfer-encoding").

In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 31-36: Add a package-private synchronized reset method to
ConfigLoader to clear the singleton for tests: implement a method named
resetInstance() (package-private) that sets the static instance field to null in
a synchronized block (matching the synchronization used by getInstance()) so
tests that call ConfigLoader.getInstance() can reset shared state between runs;
reference the static instance field and the getInstance() method when adding
this helper.
- Around line 22-28: Reformat the constructor brace placement in the
ConfigLoader class so closing braces are on their own lines: update the private
ConfigLoader() that calls
loadConfiguration(getClass().getClassLoader().getResourceAsStream("application-properties.yml"))
and the package-private ConfigLoader(InputStream input) that calls
loadConfiguration(input) to move their terminating braces to separate lines,
preserving the existing logic and references to loadConfiguration and the
resource stream call.

In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java`:
- Around line 89-96: Update the test in StaticFileHandlerTest (method
shouldReturn404ResponseWithHtmlContent) to assert that any reflected request
path is HTML-escaped in the 404 body: call createRequest with a path containing
characters that need escaping (e.g., "/<script>alert()</script>.html" or
"/a&b.html"), pass it to StaticFileHandler.handle(request), then assert
response.body() contains the escaped form (e.g.,
"&lt;script&gt;alert()&lt;/script&gt;" or "a&amp;b") and does not contain the
raw unescaped characters; keep using createRequest and StaticFileHandler.handle
to locate the change.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b01bc0f and f5ae02a.

📒 Files selected for processing (42)

• Dockerfile
• src/main/java/org/juv25d/App.java
• src/main/java/org/juv25d/Bootstrap.java
• src/main/java/org/juv25d/ConnectionHandler.java
• src/main/java/org/juv25d/Pipeline.java
• src/main/java/org/juv25d/config/FilterConfig.java
• src/main/java/org/juv25d/config/IpFilterConfig.java
• src/main/java/org/juv25d/config/RateLimitConfig.java
• src/main/java/org/juv25d/config/RedirectConfig.java
• src/main/java/org/juv25d/di/Container.java
• src/main/java/org/juv25d/di/Inject.java
• src/main/java/org/juv25d/filter/ClassScanner.java
• src/main/java/org/juv25d/filter/Filter.java
• src/main/java/org/juv25d/filter/FilterChainImpl.java
• src/main/java/org/juv25d/filter/FilterFactory.java
• src/main/java/org/juv25d/filter/FilterMatcher.java
• src/main/java/org/juv25d/filter/FilterRegistration.java
• src/main/java/org/juv25d/filter/FilterRegistry.java
• src/main/java/org/juv25d/filter/FilterScanner.java
• src/main/java/org/juv25d/filter/IpFilter.java
• src/main/java/org/juv25d/filter/LoggingFilter.java
• src/main/java/org/juv25d/filter/RateLimitingFilter.java
• src/main/java/org/juv25d/filter/RedirectFilter.java
• src/main/java/org/juv25d/filter/TimingFilter.java
• src/main/java/org/juv25d/handler/StaticFileHandler.java
• src/main/java/org/juv25d/http/HttpStatus.java
• src/main/java/org/juv25d/logging/LogContext.java
• src/main/java/org/juv25d/logging/ServerLogFormatter.java
• src/main/java/org/juv25d/logging/ServerLogging.java
• src/main/java/org/juv25d/proxy/ProxyPlugin.java
• src/main/java/org/juv25d/proxy/ProxyRoute.java
• src/main/java/org/juv25d/router/RouterConfig.java
• src/main/java/org/juv25d/util/ConfigLoader.java
• src/main/resources/application-properties.yml
• src/test/java/org/juv25d/PipelineTest.java
• src/test/java/org/juv25d/di/ContainerTest.java
• src/test/java/org/juv25d/filter/FilterRegistryTest.java
• src/test/java/org/juv25d/filter/IpFilterTest.java
• src/test/java/org/juv25d/filter/LoggingFilterTest.java
• src/test/java/org/juv25d/handler/StaticFileHandlerTest.java
• src/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java
• src/test/java/org/juv25d/proxy/ProxyPluginTest.java

✅ Files skipped from review due to trivial changes (1)

• src/main/resources/application-properties.yml

🚧 Files skipped from review as they are similar to previous changes (19)

• src/test/java/org/juv25d/di/ContainerTest.java
• src/main/java/org/juv25d/logging/ServerLogging.java
• src/main/java/org/juv25d/config/RateLimitConfig.java
• src/main/java/org/juv25d/router/RouterConfig.java
• src/main/java/org/juv25d/http/HttpStatus.java
• src/main/java/org/juv25d/proxy/ProxyRoute.java
• src/main/java/org/juv25d/Bootstrap.java
• src/test/java/org/juv25d/PipelineTest.java
• src/main/java/org/juv25d/filter/FilterFactory.java
• src/main/java/org/juv25d/ConnectionHandler.java
• src/main/java/org/juv25d/filter/LoggingFilter.java
• src/main/java/org/juv25d/App.java
• src/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java
• src/main/java/org/juv25d/di/Inject.java
• src/main/java/org/juv25d/config/FilterConfig.java
• src/main/java/org/juv25d/filter/FilterMatcher.java
• src/test/java/org/juv25d/filter/IpFilterTest.java
• src/test/java/org/juv25d/filter/LoggingFilterTest.java
• src/main/java/org/juv25d/logging/ServerLogFormatter.java

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don’t return 404 for all IOException cases.

Line 96 currently maps any read failure to 404. Missing resource should be 404, but other IO failures should be 500.

🔧 Proposed fix

+import java.io.FileNotFoundException;
 import java.io.IOException;
@@
-        } catch (IOException e) {
+        } catch (FileNotFoundException e) {
             logger.log(Level.WARNING, "File not found: " + resourcePath);
             return createNotFoundResponse(path);
+        } catch (IOException e) {
+            logger.log(Level.SEVERE, "Failed to read static file: " + resourcePath, e);
+            return createErrorResponse(500, "Internal Server Error");
         }
@@
-        if (inputStream == null) {
-            throw new IOException("Resource not found: " + resourcePath);
+        if (inputStream == null) {
+            throw new FileNotFoundException("Resource not found: " + resourcePath);
         }

Also applies to: 155-160

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 96 -
99, In StaticFileHandler, don't map every IOException to a 404: change the catch
logic around the resource-read code (the block that currently logs "File not
found: " + resourcePath and returns createNotFoundResponse(path)) to only treat
not-found errors as 404 (e.g., catch NoSuchFileException/FileNotFoundException
or check Files.exists/resource == null), and for other IOExceptions log the full
exception and return an internal-server-error response (e.g., a new or existing
createInternalServerErrorResponse or createServerErrorResponse). Apply the same
fix to the second occurrence referenced by the comment (the block around lines
155-160) so missing resources return createNotFoundResponse(path) while other IO
failures return a 500 and include exception details in the log.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

requestsPerMinute and burstCapacity have no defaults when rate-limiting config section is absent.

When the YAML lacks a rate-limiting section entirely, requestsPerMinute and burstCapacity remain at their primitive default of 0L, while rateLimitingEnabled is correctly defaulted to false. For consistency with other defaults and to avoid unexpected zero values if rate limiting is later enabled, set defaults before the conditional block.

🛠️ Proposed fix

             // rate-limiting
             // defaults (consistent pattern)
             this.rateLimitingEnabled = false;
+            this.requestsPerMinute = 60L;
+            this.burstCapacity = 100L;

             Map<String, Object> rateLimitingConfig = asStringObjectMap(config.get("rate-limiting"));
             if (rateLimitingConfig != null) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 84 - 98, Set
sane defaults for requestsPerMinute and burstCapacity before reading the
optional rate-limiting section so they aren’t left as 0 when the section is
absent: initialize requestsPerMinute and burstCapacity (alongside
rateLimitingEnabled) to the desired default values at the top of the
rate-limiting block, then keep the existing
asStringObjectMap(config.get("rate-limiting")) check and parsing logic that
updates requestsPerMinute and burstCapacity if the map is present; refer to the
fields requestsPerMinute, burstCapacity, rateLimitingEnabled and the helper
asStringObjectMap to make the change.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cd src/test/java/org/juv25d/handler && wc -l StaticFileHandlerTest.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 138

---

🏁 Script executed:

cd src/test/java/org/juv25d/handler && sed -n '90,110p' StaticFileHandlerTest.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 995

---

🏁 Script executed:

cd src/test/java/org/juv25d/handler && head -20 StaticFileHandlerTest.java | cat -n

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 830

---

🏁 Script executed:

cd src/test/java/org/juv25d/handler && grep -n "StandardCharsets\|import java.nio.charset" StaticFileHandlerTest.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 90

---

🏁 Script executed:

cd src/test/java/org/juv25d/handler && rg "new String\(response\.body" StaticFileHandlerTest.java -n

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 330

---

🏁 Script executed:

cd src/test/java/org/juv25d/handler && sed -n '1,30p' StaticFileHandlerTest.java | cat -n

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 1208

---

🏁 Script executed:

cd src/test/java/org/juv25d/handler && rg "new String\(" StaticFileHandlerTest.java -n

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 330

---

🏁 Script executed:

cd src/test/java/org/juv25d/handler && sed -n '100,110p' StaticFileHandlerTest.java | cat -n

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 687

---

Use explicit UTF-8 when decoding response body in tests.

These assertions rely on platform default charset, which can make tests flaky across environments. The response is explicitly UTF-8 encoded, so decode it consistently using StandardCharsets.UTF_8.

Proposed fix

+import java.nio.charset.StandardCharsets;
@@
-        assertThat(new String(response.body())).contains("404");
-        assertThat(new String(response.body())).contains("Not Found");
+        assertThat(new String(response.body(), StandardCharsets.UTF_8)).contains("404");
+        assertThat(new String(response.body(), StandardCharsets.UTF_8)).contains("Not Found");
@@
-        assertThat(new String(response.body())).contains("<!DOCTYPE html>");
+        assertThat(new String(response.body(), StandardCharsets.UTF_8)).contains("<!DOCTYPE html>");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java` around lines 94
- 96, The test in StaticFileHandlerTest uses platform-default decoding for
response.body(); change calls like new String(response.body()) to new
String(response.body(), StandardCharsets.UTF_8) to explicitly decode as UTF-8,
and add the import for java.nio.charset.StandardCharsets; update all similar
assertions in the class (e.g., the assertions that check for "404" and "Not
Found") to use the UTF-8-aware decoding.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test does not verify that restricted headers were actually filtered.

The test name and description claim to verify restricted header filtering, but the only assertion is that the status code is 200. To properly test this behavior, capture the outgoing request and verify that Connection and Host headers are not included.

💡 Proposed fix to add proper assertions

+        java.net.http.HttpRequest[] capturedRequest = new java.net.http.HttpRequest[1];
-        doReturn(upstreamResponse).when(httpClient).send(
-            any(java.net.http.HttpRequest.class),
-            any(java.net.http.HttpResponse.BodyHandler.class)
-        );
+        when(httpClient.send(any(java.net.http.HttpRequest.class), any()))
+            .thenAnswer(invocation -> {
+                capturedRequest[0] = invocation.getArgument(0);
+                return upstreamResponse;
+            });

         proxyPlugin.handle(req, res);

         assertThat(res.statusCode()).isEqualTo(200);
+        // Verify restricted headers were filtered
+        var sentHeaders = capturedRequest[0].headers().map();
+        assertThat(sentHeaders).doesNotContainKey("Connection");
+        assertThat(sentHeaders).doesNotContainKey("Host");
+        // Verify non-restricted headers were forwarded
+        assertThat(sentHeaders).containsKey("Content-Type");

📝 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.

Suggested change

	@DisplayName("filters restricted headers from being proxied")
	@Test
	void filtersRestrictedHeaders() throws Exception {
	Map<String, String> headers = Map.of(
	"Content-Type", "application/json",
	"Connection", "keep-alive",
	"Host", "example.com"
	);
	HttpRequest req = new HttpRequest("GET", "/api/test/users", null, "HTTP/1.1",
	headers, new byte[0], "UNKNOWN");
	HttpResponse res = new HttpResponse();
	when(upstreamResponse.statusCode()).thenReturn(200);
	when(upstreamResponse.body()).thenReturn(new byte[0]);
	when(upstreamResponse.headers()).thenReturn(
	HttpHeaders.of(Map.of(), (name, value) -> true));
	doReturn(upstreamResponse).when(httpClient).send(
	any(java.net.http.HttpRequest.class),
	any(java.net.http.HttpResponse.BodyHandler.class)
	);
	proxyPlugin.handle(req, res);
	assertThat(res.statusCode()).isEqualTo(200);
	}
	`@DisplayName`("filters restricted headers from being proxied")
	`@Test`
	void filtersRestrictedHeaders() throws Exception {
	Map<String, String> headers = Map.of(
	"Content-Type", "application/json",
	"Connection", "keep-alive",
	"Host", "example.com"
	);
	HttpRequest req = new HttpRequest("GET", "/api/test/users", null, "HTTP/1.1",
	headers, new byte[0], "UNKNOWN");
	HttpResponse res = new HttpResponse();
	when(upstreamResponse.statusCode()).thenReturn(200);
	when(upstreamResponse.body()).thenReturn(new byte[0]);
	when(upstreamResponse.headers()).thenReturn(
	HttpHeaders.of(Map.of(), (name, value) -> true));
	java.net.http.HttpRequest[] capturedRequest = new java.net.http.HttpRequest[1];
	when(httpClient.send(any(java.net.http.HttpRequest.class), any()))
	.thenAnswer(invocation -> {
	capturedRequest[0] = invocation.getArgument(0);
	return upstreamResponse;
	});
	proxyPlugin.handle(req, res);
	assertThat(res.statusCode()).isEqualTo(200);
	// Verify restricted headers were filtered
	var sentHeaders = capturedRequest[0].headers().map();
	assertThat(sentHeaders).doesNotContainKey("Connection");
	assertThat(sentHeaders).doesNotContainKey("Host");
	// Verify non-restricted headers were forwarded
	assertThat(sentHeaders).containsKey("Content-Type");
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/juv25d/proxy/ProxyPluginTest.java` around lines 156 - 180,
The test ProxyPluginTest::filtersRestrictedHeaders currently only asserts status
code; capture the outgoing java.net.http.HttpRequest passed into httpClient.send
(use an ArgumentCaptor or verify with an argument matcher against
httpClient.send) after calling proxyPlugin.handle(req, res) and assert that the
captured request's headers do not contain "Connection" or "Host" while still
containing "Content-Type" as "application/json"; reference the test method name
filtersRestrictedHeaders, the proxyPlugin.handle(...) call, and the mocked
httpClient.send(...) invocation when locating where to add the assertions.