Feature/rate limiting filter

[gvaguirres]

Summary by CodeRabbit

• New Features

  • Full HTTP web server: static file serving with MIME detection, configurable port (CLI override), IP allowlist/blocklist, per‑IP rate limiting, compression, locale handling, request timeout handling, and detailed response construction.

• Documentation

  • Expanded README with usage, configuration, architecture, deployment and troubleshooting.
  • Added Port configuration guide.

• Chores

  • CI workflow and release automation; multi‑arch Docker image publishing and production-ready container.

[coderabbitai]

Warning

Rate limit exceeded

@gvaguirres has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 51 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 0583b14 and 6fed940.

📒 Files selected for processing (1)

• src/test/java/org/example/filter/RateLimitingFilterIpTest.java

📝 Walkthrough

Walkthrough

Adds a production-ready Java HTTP static-file server with configuration loading, TCP server using virtual threads, HTTP parsing/response utilities, a filter framework (rate limiting, IP, compression, locale, timeout, logging), extensive tests, Docker and GitHub Actions workflows, and updated docs (README and port guide).

Changes

Cohort / File(s)	Summary
CI/CD & Release ​.github/workflows/ci.yml, ​.github/workflows/release.yml	Add Maven CI workflow (checkout, determine Java version, setup-java, build, test) and a release workflow to build/push multi-arch Docker images to GHCR on release.
Containerization Dockerfile	Two-stage Dockerfile: Maven build stage and minimal Temurin JRE runtime, non-root user, exposes 8080, runs org.example.App.
Entry & Server src/main/java/org/example/App.java, src/main/java/org/example/TcpServer.java, src/main/java/org/example/ConnectionFactory.java	App loads config, resolves/validates port (CLI > config > default) and starts TcpServer; TcpServer accepts connections and delegates to ConnectionFactory/ConnectionHandler on virtual threads.
Connection Handling src/main/java/org/example/ConnectionHandler.java	New ConnectionHandler: parse requests, attach client IP, apply configurable filter chain, resolve target URI, and delegate to StaticFileHandler; implements AutoCloseable.
Static File Serving & Assets src/main/java/org/example/StaticFileHandler.java, www/index.html, www/pageNotFound.html	StaticFileHandler with URI sanitation, path-traversal protection, content-type detection, fallback 404 page; added index and 404 static pages.
HTTP Core & Parsers src/main/java/org/example/httpparser/..., src/main/java/org/example/httpparser/HttpRequest.java, src/main/java/org/example/http/HttpResponseBuilder.java, src/main/java/org/example/http/MimeTypeDetector.java	Request-line and header parsing, HttpRequest wrapper with attributes, HttpResponseBuilder for status/headers/body (binary-safe), and MimeTypeDetector utility.
Filter Framework & Pipeline src/main/java/org/example/filter/Filter.java, .../FilterChain.java, .../FilterChainImpl.java, src/main/java/org/example/server/ConfigurableFilterPipeline.java, src/main/java/org/example/server/FilterRegistration.java, src/main/java/org/example/server/RoutePattern.java	Introduce Filter/FilterChain interfaces and implementation, pipeline to arrange global/route filters with ordering and pattern matching.
Filters (new implementations) src/main/java/org/example/filter/RateLimitingFilter.java, IpFilter.java, CompressionFilter.java, LocaleFilter*.java, LoggingFilter.java, RequestTimeOutFilter.java	Add many filters: per-IP Bucket4j rate limiter with cleanup, IP allow/block modes, gzip compression, locale resolvers (with cookie variant), logging, and request timeout wrapper.
Configuration Model & Loader src/main/java/org/example/config/AppConfig.java, src/main/java/org/example/config/ConfigLoader.java, src/main/resources/application.yml, src/test/resources/application.yml	Record-based AppConfig with nested defaults/validation; ConfigLoader supports YAML/JSON, classpath fallback, caching (loadOnce) and test reset; sample application.yml added.
Build & Tooling pom.xml	Bump Java target to 25; add Bucket4j, Jackson YAML/JSON, Brotli, test libs (Mockito, Awaitility), PITEST and Spotless plugins.
Documentation README.md, PortConfigurationGuide.md	Large README rewrite to production server docs; new port configuration guide documenting CLI/config/default precedence and examples.
Tests src/test/... (many files) src/test/java/..., src/test/resources/...	Comprehensive unit/integration tests added: parsing, response builder, MIME detection, static file handling, config loader caching/fallback, IP and rate-limit behaviors, filter pipeline, TcpServer/ConnectionHandler, and port resolution tests. Numerous test helpers and test YAML added.

Sequence Diagram

sequenceDiagram
    actor Client
    participant TcpServer as "TcpServer\n(runs, accepts socket)"
    participant ConnectionHandler as "ConnectionHandler\n(parses & applies filters)"
    participant HttpParser as "HttpParser"
    participant FilterChain as "FilterChain / Filters"
    participant StaticFileHandler as "StaticFileHandler"
    participant HttpResponseBuilder as "HttpResponseBuilder"

    Client->>TcpServer: TCP connect & send request
    TcpServer->>ConnectionHandler: spawn handler (virtual thread) with Socket
    ConnectionHandler->>HttpParser: setReader / parseRequest()
    HttpParser-->>ConnectionHandler: HttpRequest (method, uri, headers)
    ConnectionHandler->>FilterChain: applyFilters(HttpRequest)
    FilterChain->>FilterChain: RateLimitingFilter / IpFilter / other filters
    alt early response (429/403/400)
        FilterChain->>HttpResponseBuilder: build error response
    else allowed
        ConnectionHandler->>StaticFileHandler: handleGetRequest(uri)
        StaticFileHandler->>HttpResponseBuilder: set status/body/headers
    end
    HttpResponseBuilder-->>ConnectionHandler: response bytes
    ConnectionHandler->>Client: write response and close

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐇 I hopped into code, found ports to keep,
Filters guarding visits, buckets counting sleep,
Static pages safe from sneaky trails,
Configs and Docker ships set sail,
A tiny server, steady and fleet.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.73% 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 'Feature/rate limiting filter' directly corresponds to the main objective of the PR, which is implementing rate limiting functionality. The changeset includes RateLimitingFilter, integration into the server architecture, comprehensive tests, and supporting infrastructure. The title accurately reflects this primary change.

_{✏️ 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: 8

Note

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

♻️ Duplicate comments (1)

src/test/java/org/example/httpparser/HttpParseRequestLineTest.java (1)

8-9: Same note as HttpParserTest.java: prefer Assertions over AssertionsForClassTypes for consistency.

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java` around
lines 8 - 9, Replace the AssertJ imports in HttpParseRequestLineTest so they use
the standard Assertions class instead of AssertionsForClassTypes: update the
static imports that reference assertThat and assertThatThrownBy to come from
org.assertj.core.api.Assertions (keeping the same method names) so they match
the rest of the test suite's usage (look for the import lines referencing
AssertionsForClassTypes and the tests in HttpParseRequestLineTest that call
assertThat / assertThatThrownBy).

🟡 Minor comments (21)

README.md-234-239 (1)

234-239: ⚠️ Potential issue | 🟡 Minor

Add language identifier to fenced block.

The allowed/blocked path examples should specify a language (e.g., text).

Suggested fix

-```
+```text

🤖 Prompt for AI Agents

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

In `@README.md` around lines 234 - 239, The fenced code block showing
allowed/blocked path examples in README.md is missing a language identifier;
update the triple-backtick fence to include a language such as "text" (i.e.,
change the opening ``` to ```text) so syntax-highlighting and tooling correctly
treat the content (the block containing "✅ Allowed:  /index.html" etc.).

README.md-80-82 (1)

80-82: ⚠️ Potential issue | 🟡 Minor

Add language identifier to fenced block.

The URL block should specify a language (e.g., text).

Suggested fix

-```
+```text

🤖 Prompt for AI Agents

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

In `@README.md` around lines 80 - 82, The fenced code block that currently
contains the URL "http://localhost:8080" should include a language identifier
(e.g., change the opening ``` to ```text) so the block is marked as plain text;
update the fenced block in README.md containing that URL to use ```text as the
opening fence.

README.md-37-37 (1)

37-37: ⚠️ Potential issue | 🟡 Minor

Remove extra space in heading marker.

## Quick Start has two spaces after the hashes; markdownlint flags this.

Suggested fix

-##  Quick Start
+## Quick Start

🤖 Prompt for AI Agents

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

In `@README.md` at line 37, The "##  Quick Start" markdown heading has two spaces
after the hashes which trips markdownlint; change the heading marker to a single
space by replacing "##  Quick Start" with "## Quick Start" so the heading
conforms to markdownlint rules and renders correctly.

README.md-87-89 (1)

87-89: ⚠️ Potential issue | 🟡 Minor

Add language identifier to fenced block.

The path block should specify a language (e.g., text).

Suggested fix

-```
+```text

🤖 Prompt for AI Agents

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

In `@README.md` around lines 87 - 89, Update the fenced code block that currently
contains "src/main/resources/application.yml" to include a language identifier
(e.g., use ```text) so the block reads as a text fenced code block; locate the
README.md fenced block with the content "src/main/resources/application.yml" and
change the opening backticks to include the language token.

README.md-340-351 (1)

340-351: ⚠️ Potential issue | 🟡 Minor

Add language identifier to fenced block.

The error output block should specify a language (e.g., text).

Suggested fix

-```
+```text

🤖 Prompt for AI Agents

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

In `@README.md` around lines 340 - 351, The fenced error output block containing
"Error: Address already in use" should include a language identifier; update the
opening fence from ``` to ```text so the block reads as a plain text code fence
(leave the surrounding explanatory text and the subsequent bash fenced blocks
unchanged).

README.md-2-2 (1)

2-2: ⚠️ Potential issue | 🟡 Minor

Fix heading level jump (H1 → H3).

Line 2 jumps from # to ###. Use ## for the subheading.

Suggested fix

-### **Class JUV25G** | Lightweight • Configurable • Secure
+## **Class JUV25G** | Lightweight • Configurable • Secure

🤖 Prompt for AI Agents

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

In `@README.md` at line 2, The README has a heading level jump: change the "###
**Class JUV25G** | Lightweight • Configurable • Secure" heading to use level 2
(replace the leading "###" with "##") so the document flows correctly under the
top-level H1; update the line containing "Class JUV25G" accordingly.

README.md-39-44 (1)

39-44: ⚠️ Potential issue | 🟡 Minor

Add language identifier to fenced block.

The ASCII art block should specify a language (e.g., text).

Suggested fix

-```
+```text

🤖 Prompt for AI Agents

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

In `@README.md` around lines 39 - 44, The README contains a fenced code block
showing ASCII art without a language identifier; update that fence around the
ASCII art (the triple-backtick block containing "Ready to launch your web
server? Follow these simple steps!") to include a language tag such as text
(e.g., change ``` to ```text) so the block is explicitly marked as plain text.

README.md-283-290 (1)

283-290: ⚠️ Potential issue | 🟡 Minor

Add language identifier to fenced block.

The directory example should specify a language (e.g., text).

Suggested fix

-```
+```text

🤖 Prompt for AI Agents

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

In `@README.md` around lines 283 - 290, The fenced code block showing the
directory tree (the triple-backtick code fence surrounding the www/ tree) lacks
a language identifier; update that opening fence from ``` to ```text so the
directory example is marked as plain text (e.g., change the fence that wraps the
"www/ ├── index.html ..." block to include the language identifier `text`).

README.md-48-49 (1)

48-49: ⚠️ Potential issue | 🟡 Minor

Fix duplicate command and repo directory name.

git clone git clone is duplicated, and the cd target likely won’t match the cloned folder name.

Suggested fix

-git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git
-cd project-webserver
+git clone https://github.com/ithsjava25/project-webserver-juv25g.git
+cd project-webserver-juv25g

🤖 Prompt for AI Agents

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

In `@README.md` around lines 48 - 49, Remove the duplicated "git clone" token and
update the directory name in the cd command to match the cloned repo; replace
the line "git clone git clone
https://github.com/ithsjava25/project-webserver-juv25g.git" with a single "git
clone https://github.com/ithsjava25/project-webserver-juv25g.git" and change the
"cd project-webserver" command to "cd project-webserver-juv25g" so the commands
successfully clone the intended repository and enter the correct folder.

README.md-134-157 (1)

134-157: ⚠️ Potential issue | 🟡 Minor

Add language identifier to fenced block.

The directory tree block should specify a language (e.g., text).

Suggested fix

-```
+```text

🤖 Prompt for AI Agents

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

In `@README.md` around lines 134 - 157, Update the fenced directory-tree block in
README.md to include a language identifier (e.g., change the opening ``` to
```text) so the tree is rendered as plain text; locate the block that starts
with the triple backticks followed by the project-webserver/ tree and add the
language token to the opening fence.

README.md-12-12 (1)

12-12: ⚠️ Potential issue | 🟡 Minor

Anchor links may not match emoji headings.

The anchors in the TOC may not resolve reliably when headings include emojis; markdownlint already flags invalid fragments. Consider adding explicit HTML anchors or using plain-text headings for stable IDs.

Suggested fix (explicit anchors)

-[Features](`#features`) • [Quick Start](`#quick-start`) • [Configuration](`#configuration`)
+<a id="features"></a> <a id="quick-start"></a> <a id="configuration"></a>
+[Features](`#features`) • [Quick Start](`#quick-start`) • [Configuration](`#configuration`)

🤖 Prompt for AI Agents

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

In `@README.md` at line 12, The TOC links like "[Features](`#features`)" can break
when headings contain emojis; update the README by adding explicit HTML anchor
elements immediately before the corresponding emoji headings (e.g., insert <a
id="features"></a> above the Features heading) or replace the emoji headings
with plain-text headings so the slug matches the link; ensure each TOC link
target (features, quick-start, configuration) exactly matches the id attribute
you add and keep anchor names lowercase and hyphenated to match existing links.

src/main/java/org/example/http/HttpResponseBuilder.java-79-82 (1)

79-82: ⚠️ Potential issue | 🟡 Minor

Stale Content-Length if setBody is called after setHeader("Content-Length", ...).

build() skips auto-computation whenever Content-Length is already present in the headers map (line 125). If a caller does setHeader("Content-Length", "X") and then setBody(newBody), the emitted Content-Length will be stale. This is a subtle API hazard worth documenting, or alternatively, setBody should clear any manually-set Content-Length.

Also applies to: 125-127

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/http/HttpResponseBuilder.java` around lines 79 -
82, The setBody method currently assigns bytebody and clears body but does not
update headers, which allows a previously set Content-Length (via setHeader) to
become stale; update setBody(byte[] body) to remove any existing
"Content-Length" header from the headers map (or normalize header name and
remove variants like "content-length") so that build() can recompute the correct
length, and likewise ensure any other body setters (e.g., setBody(String)) also
clear "Content-Length" to keep headers and body consistent; reference
methods/fields: setBody(byte[]), setHeader(...), headers (map) and build().

src/main/java/org/example/http/HttpResponseBuilder.java-84-87 (1)

84-87: ⚠️ Potential issue | 🟡 Minor

setHeaders(null) causes an unguarded NullPointerException.

putAll(null) throws NPE. A null guard or early return would prevent this.

🛡️ Proposed fix

 public void setHeaders(Map<String, String> headers) {
+    if (headers == null) return;
     this.headers = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
     this.headers.putAll(headers);
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/http/HttpResponseBuilder.java` around lines 84 -
87, The setHeaders method currently calls this.headers.putAll(headers) which
will throw a NullPointerException if the caller passes null; update
HttpResponseBuilder.setHeaders to guard against null by initializing
this.headers to a new TreeMap<>(String.CASE_INSENSITIVE_ORDER) and, if the
incoming headers parameter is non-null, call putAll(headers); otherwise leave
the map empty (or clear it) so setHeaders(null) is safe and does not NPE.

src/main/java/org/example/http/MimeTypeDetector.java-21-21 (1)

21-21: ⚠️ Potential issue | 🟡 Minor

application/javascript is deprecated — use text/javascript per RFC 9239.

RFC 9239 states that the application/javascript MIME type has been deprecated and text/javascript should be used. MDN confirms: JavaScript content should always be served with the MIME type text/javascript; browsers may support legacy JavaScript types, but you should not assume scripts served with any MIME type other than text/javascript will always load or run.

🛡️ Proposed fix

-            Map.entry(".js", "application/javascript; charset=UTF-8"),
+            Map.entry(".js", "text/javascript; charset=UTF-8"),

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/http/MimeTypeDetector.java` at line 21, The .js
MIME mapping in MimeTypeDetector uses the deprecated value
"application/javascript; charset=UTF-8"; update the Map entry for the ".js" key
in the MimeTypeDetector class to use "text/javascript; charset=UTF-8" instead
(preserve the charset and formatting), then run any MIME-related tests and/or
linter to ensure no other references expect the old string; locate the mapping
in the static map initializer where Map.entry(".js", ...) is defined and replace
the value accordingly.

src/main/java/org/example/filter/RateLimitingFilter.java-20-20 (1)

20-20: ⚠️ Potential issue | 🟡 Minor

Javadoc truncated: "maximum capaci"

The word is cut off mid-word. Should be "maximum capacity".

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` at line 20, Update
the truncated Javadoc comment in the RateLimitingFilter class so the sentence
reads "...up to the maximum capacity" instead of ending with "maximum capaci";
locate the comment above the rate-limiting logic or class/method Javadoc (search
for the phrase "Tokens are replenished at a fixed rate over time (Refill Rate),
up to the maximum capaci") and correct the typo to "maximum capacity".

PortConfigurationGuide.md-26-29 (1)

26-29: ⚠️ Potential issue | 🟡 Minor

YAML example has wrong indentation — port must be nested under server

As written, the example implies port is a top-level key, but AppConfig reads it as server.port. Anyone copying this snippet verbatim will get a misconfigured file and fall through to the default port silently.

📝 Proposed fix

 ```yaml
 server:
-port: 9090
+  port: 9090

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

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

In @PortConfigurationGuide.md around lines 26 - 29, The YAML example currently
places port as a top-level key; update the snippet so port is nested under
server (so AppConfig can read server.port correctly) by indenting the port line
under the server key (e.g., two spaces before "port: 9090"), ensuring the
resulting keys are "server" -> "port" rather than separate top-level entries.

</details>

</blockquote></details>
<details>
<summary>www/pageNotFound.html-47-53 (1)</summary><blockquote>

`47-53`: _⚠️ Potential issue_ | _🟡 Minor_

**Add a "Go Home" link so users can recover from the 404**

The page has no navigation option, leaving the user stuck. A simple anchor resolves this.

<details>
<summary>💡 Proposed addition</summary>

```diff
     <p>We cannot find the page you were looking for. ...</p>
+    <a href="/">← Back to Home</a>
 </div>

🤖 Prompt for AI Agents

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

In `@www/pageNotFound.html` around lines 47 - 53, Add a "Go Home" anchor so users
can recover from the 404 by inserting a link element (e.g., an <a> with href="/"
and text "Go Home") inside the existing <div class="container"> after the <p>
paragraph; give it a descriptive class like "home-link" or "btn" to match
styling, ensure it is keyboard accessible (role/button or proper focus styles)
and that it appears visually under the existing <h2> and <p> content so users
can easily navigate back to the site root.

src/test/java/org/example/filter/RateLimitingFilterTest.java-11-11 (1)

11-11: ⚠️ Potential issue | 🟡 Minor

Remove unused import java.net.http.HttpResponse

This is the JDK HTTP client's HttpResponse, which is never referenced in the test. It likely crept in from autocomplete.

🧹 Proposed fix

-import java.net.http.HttpResponse;

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/filter/RateLimitingFilterTest.java` at line 11,
Remove the unused import of java.net.http.HttpResponse from the test file; open
the RateLimitingFilterTest class and delete the import line "import
java.net.http.HttpResponse;" so only actually used imports remain, then
recompile/run tests to confirm no unused-import warnings.

src/main/java/org/example/filter/RateLimitingFilter.java-50-53 (1)

50-53: ⚠️ Potential issue | 🟡 Minor

Add Retry-After header to the 429 response

RFC 6585 §4 recommends including a Retry-After header on 429 responses to indicate when the client may retry. Given the refill rate is known (1 token per 10 seconds), the value is straightforward.

💡 Proposed addition

     response.setStatusCode(HttpResponseBuilder.SC_TOO_MANY_REQUESTS);
+    response.setHeader("Retry-After", String.valueOf(refillPeriod.getSeconds()));
     response.setBody("<h1>429 Too Many Requests</h1><p> Limit of requests exceeded.</p>\n");

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 50 -
53, When responding with 429 in RateLimitingFilter, include a Retry-After header
indicating when the client may retry; add response.setHeader("Retry-After",
"<seconds>") alongside the existing response.setStatusCode and setBody. If you
have a TokenBucket/RateLimiter helper, compute seconds until the next token
(e.g., refill rate is 1 token per 10s so use 10 or calculate timeToNextToken via
tokenBucket.getTimeToNextToken()) and use that value as the header; otherwise
hard-code "10" seconds to conform to RFC 6585 §4.

src/test/java/org/example/StaticFileHandlerTest.java-49-55 (1)

49-55: ⚠️ Potential issue | 🟡 Minor

Use explicit UTF‑8 decoding for response assertions.

ByteArrayOutputStream.toString() uses the platform default charset, which can make these tests flaky when responses are UTF‑8. Use the UTF‑8 overload and apply it consistently across tests.

🔧 Suggested fix

+import java.nio.charset.StandardCharsets;
 ...
-        String response = fakeOutput.toString();//Converts the captured byte stream into a String for verification
+        String response = fakeOutput.toString(StandardCharsets.UTF_8);//Converts the captured byte stream into a String for verification

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/StaticFileHandlerTest.java` around lines 49 - 55,
Update the test to decode the captured output explicitly as UTF-8 instead of
using platform-default ByteArrayOutputStream.toString(): in
StaticFileHandlerTest replace the use of fakeOutput.toString() (the local
variable response) with an explicit UTF-8 decode (e.g., use the UTF-8 overload
or new String(fakeOutput.toByteArray(), StandardCharsets.UTF_8)), and ensure
subsequent assertions that reference "HTTP/1.1 " + SC_OK + " OK", "Hello Test",
and the Content-Type header check use that UTF-8-decoded response; apply the
same change consistently across similar tests to avoid platform-dependent
failures.

src/main/java/org/example/config/AppConfig.java-23-24 (1)

23-24: ⚠️ Potential issue | 🟡 Minor

Remove leftover development comments.

Lines 23–24 contain Swedish-language reminder comments (← LÄGG TILL, ← UPPDATERA DENNA RAD) that appear to be development artifacts and should be cleaned up before merging.

-        IpFilterConfig ipFilterConfig = (ipFilter == null ? IpFilterConfig.defaults() : ipFilter.withDefaultsApplied());  // ← LÄGG TILL
-        return new AppConfig(serverConfig, loggingConfig, ipFilterConfig);  // ← UPPDATERA DENNA RAD
+        IpFilterConfig ipFilterConfig = (ipFilter == null ? IpFilterConfig.defaults() : ipFilter.withDefaultsApplied());
+        return new AppConfig(serverConfig, loggingConfig, ipFilterConfig);

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/config/AppConfig.java` around lines 23 - 24, Remove
the Swedish development note comments on the IpFilterConfig and AppConfig lines
so the code reads cleanly: keep the logic that assigns ipFilterConfig using
(ipFilter == null ? IpFilterConfig.defaults() : ipFilter.withDefaultsApplied())
and return new AppConfig(serverConfig, loggingConfig, ipFilterConfig) but delete
the trailing "← LÄGG TILL" and "← UPPDATERA DENNA RAD" comments; ensure symbols
IpFilterConfig, ipFilter, AppConfig, serverConfig, and loggingConfig remain
unchanged and compile.

🧹 Nitpick comments (16)

src/main/java/org/example/http/HttpResponseBuilder.java (1)

40-40: bytebody is non-standard naming — should be byteBody.

♻️ Proposed rename

-    private byte[] bytebody;
+    private byte[] byteBody;

Apply the rename consistently across setBody(byte[]) and build().

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/http/HttpResponseBuilder.java` at line 40, Rename
the non-standard field bytebody to byteBody and update all references
accordingly: change the private field declaration bytebody to byteBody, update
the setter setBody(byte[]) to assign to byteBody, and update build() to read
from byteBody; ensure getters, constructors, and any other usages in
HttpResponseBuilder that reference bytebody are renamed consistently.

src/main/java/org/example/filter/Filter.java (1)

8-12: init() and destroy() have no default implementations — add no-op defaults to reduce boilerplate.

Every concrete Filter implementation must provide empty init() and destroy() bodies even when the lifecycle hooks are unused. Providing default no-ops makes the interface more ergonomic.

♻️ Proposed default no-ops

 public interface Filter {
-    void init();
+    default void init() {}

     void doFilter(HttpRequest request, HttpResponseBuilder response, FilterChain chain);

-    void destroy();
+    default void destroy() {}
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/Filter.java` around lines 8 - 12, Add
default no-op implementations for init() and destroy() directly in the Filter
interface so concrete classes don’t need empty overrides: change the signatures
of init() and destroy() in the Filter interface to default methods with empty
bodies while keeping doFilter(HttpRequest, HttpResponseBuilder, FilterChain)
abstract; reference the Filter interface and the init() and destroy() method
names when making the change.

src/main/java/org/example/filter/FilterChainImpl.java (2)

20-22: Defensively copy the filters list and null-check the constructor parameter.

this.filters = filters stores the caller's reference directly. If the caller mutates the list after construction, the chain's behavior changes silently. Additionally, if null is passed, doFilter will throw NullPointerException on filters.size().

♻️ Proposed fix

+import java.util.Objects;

 public FilterChainImpl(List<Filter> filters) {
-    this.filters = filters;
+    this.filters = List.copyOf(Objects.requireNonNull(filters, "filters 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/example/filter/FilterChainImpl.java` around lines 20 - 22,
Constructor FilterChainImpl currently assigns the caller's list reference
directly and doesn't guard against null, so mutate-after-construction or a null
argument can break doFilter; update the FilterChainImpl(List<Filter> filters)
constructor to validate filters is not null (throw IllegalArgumentException or
NPE with a clear message) and create a defensive copy (e.g., new
ArrayList<>(filters)) and optionally wrap with Collections.unmodifiableList
before assigning to this.filters so the chain's internal list is immutable and
safe for use by doFilter and other methods.

---

29-31: Incomplete chain: no terminal action when all filters are exhausted.

The TODO on line 30 leaves a real gap — when the chain is fully traversed (all filters passed), nothing happens. The actual request handler (e.g., StaticFileHandler) is never called through the chain. Verify that ConnectionHandler handles this externally (calling the handler after chain.doFilter() returns), or add the terminal dispatch here.

Do you want me to generate a completion of this with a Runnable finalHandler terminal callback passed via the constructor?

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/FilterChainImpl.java` around lines 29 - 31,
The FilterChainImpl currently stops when no more filters exist (TODO) so the
terminal request handler never runs; fix by adding a terminal dispatch: add a
finalHandler field (e.g., Runnable finalHandler) to FilterChainImpl set via
constructor, update the constructor signature where FilterChainImpl is created
(e.g., in ConnectionHandler) to pass the actual request processor
(StaticFileHandler or equivalent), and in doFilter() when current index >=
filters.size() call finalHandler.run(); alternatively, if you prefer external
dispatch, verify and document that ConnectionHandler invokes the final handler
after chain.doFilter() returns and remove the TODO accordingly.

src/main/java/org/example/httpparser/HttpRequest.java (1)

12-49: Class appears to retain inner-class indentation — all members are indented 8 spaces instead of 4.

The entire body of HttpRequest is indented with 8 spaces, suggesting this was originally a nested class that was promoted to a top-level class without reformatting. This also causes the getters to appear on the same line as their closing braces (return method; }), which hurts readability.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/httpparser/HttpRequest.java` around lines 12 - 49,
The HttpRequest class body and its members are over-indented and several getter
methods have closing braces on the same line as the return statement, harming
readability; reformat the class so its declarations and methods use standard
4-space indentation (class HttpRequest, fields
method/path/version/headers/body/attributes, constructor HttpRequest(...), and
all methods like getMethod, getPath, getVersion, getHeaders, getBody,
setAttribute, getAttribute) and place method return statements and closing
braces on separate lines so each method follows conventional Java formatting.

.github/workflows/ci.yml (1)

16-19: mvn is invoked before Java is configured — fragile dependency on the runner's pre-installed JDK.

The Get Java Version step runs mvn help:evaluate before actions/setup-java configures the JDK. This works today because ubuntu-latest ships with a default JDK, but that JDK may differ from the project's required version (Java 25 per the AI summary) and could break if the runner image changes.

Consider resolving the bootstrapping problem by hard-coding the Java version in the workflow and reading it from the POM only to keep them in sync as a validation step — or accept the fragility and document it.

🔧 Suggested alternative: pin a bootstrap JDK for the evaluate step

+      - name: Set up bootstrap JDK (for Maven evaluate)
+        uses: actions/setup-java@v4
+        with:
+          java-version: '21'       # Minimum required to run Maven
+          distribution: 'temurin'
+
       - name: Get Java Version
         run: |
           JAVA_VERSION=$(mvn help:evaluate "-Dexpression=maven.compiler.release" -q -DforceStdout)
           echo "JAVA_VERSION=$JAVA_VERSION" >> $GITHUB_ENV
 
       - name: Set up JDK ${{ env.JAVA_VERSION }}
         uses: actions/setup-java@v4
         with:
           java-version: ${{ env.JAVA_VERSION }}
           distribution: 'temurin'
           cache: maven

🤖 Prompt for AI Agents

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

In @.github/workflows/ci.yml around lines 16 - 19, The workflow runs the "Get
Java Version" step (which executes mvn help:evaluate) before the runner JDK is
configured, making the mvn call fragile; move or add an actions/setup-java step
to install a known bootstrap JDK before the "Get Java Version" step (or
alternatively hard-code the project's Java version into an env var and use mvn
only as a validation step), update the "Get Java Version" step to rely on the
configured JDK, and ensure the workflow uses the same Java version in
actions/setup-java and any JAVA_VERSION env assignments to keep them in sync.

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

28-30: Inconsistent field modifiers — refillPeriod should also be static final

capacity and refillTokens are static final, but refillPeriod is an instance field. All three are immutable configuration constants and should have the same modifier.

♻️ Proposed fix

-    private final Duration refillPeriod = Duration.ofSeconds(10);
+    private static final Duration refillPeriod = Duration.ofSeconds(10);

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 28 -
30, The constant field modifiers are inconsistent: in RateLimitingFilter the
fields capacity and refillTokens are declared static final but refillPeriod is
an instance field; change refillPeriod to a class-level constant by making it
static final (i.e. declare refillPeriod as static final Duration refillPeriod =
Duration.ofSeconds(10)) so all three immutable configuration constants
(capacity, refillTokens, refillPeriod) share the same modifiers.

---

57-76: destroy() and clearBuckets() are identical — eliminate the duplicate

Both methods call buckets.clear(). clearBuckets() exists purely to let tests reset state, but this workaround is only needed because buckets is static (see above comment). Once buckets is made an instance field, tests can simply create a new RateLimitingFilter in @BeforeEach and clearBuckets() can be removed. If the static field must stay, at minimum have one delegate to the other.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 57 -
76, The class RateLimitingFilter currently has duplicate methods destroy() and
clearBuckets() both calling buckets.clear(); make buckets an instance field
(remove static) so tests can instantiate a fresh RateLimitingFilter in
`@BeforeEach` and then delete the clearBuckets() helper, or if you must keep
buckets static, collapse the duplication by having clearBuckets() delegate to
destroy() (or vice versa) and keep only one implementation; update any tests to
construct a new RateLimitingFilter when buckets becomes non-static or to call
the single remaining clearing method.

src/test/java/org/example/AppPortResolutionTest.java (1)

8-20: Consider adding tests for the error branches in App.resolvePort / App.validatePort

App.java has three IllegalArgumentException paths that aren't exercised:

• Port out of range (validatePort)
• Non-numeric value after --port
• Missing value after --port flag

💡 Example additional tests

`@Test`
void out_of_range_port_throws() {
    assertThatThrownBy(() -> App.resolvePort(new String[]{"--port", "99999"}, 8080))
        .isInstanceOf(IllegalArgumentException.class);
}

`@Test`
void non_numeric_port_throws() {
    assertThatThrownBy(() -> App.resolvePort(new String[]{"--port", "abc"}, 8080))
        .isInstanceOf(IllegalArgumentException.class);
}

`@Test`
void missing_value_after_flag_throws() {
    assertThatThrownBy(() -> App.resolvePort(new String[]{"--port"}, 8080))
        .isInstanceOf(IllegalArgumentException.class);
}

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/AppPortResolutionTest.java` around lines 8 - 20,
Add unit tests in AppPortResolutionTest to cover the three
IllegalArgumentException branches in App.resolvePort / App.validatePort: (1) a
test that passing an out-of-range port (e.g. "99999") via "--port" throws, (2) a
test that passing a non-numeric value (e.g. "abc") after "--port" throws, and
(3) a test that passing the "--port" flag with no following value throws; use
assertThatThrownBy (or your test framework's equivalent) to assert
IllegalArgumentException and reference App.resolvePort and App.validatePort in
the test names to make the intent clear.

.github/workflows/release.yml (1)

1-35: Consider running tests before publishing the Docker image

The workflow goes straight from checkout to build-and-push with no mvn test step. A release could publish a broken image if tests were never run. Adding a test step (or a required CI workflow that must pass first) would prevent this.

💡 Suggested addition before the build step

       - uses: docker/setup-buildx-action@v3.12.0
+      - name: Run tests
+        run: docker run --rm -v "$PWD":/build -w /build maven:3-eclipse-temurin-25-alpine mvn test
       - name: Log in to GHCR

🤖 Prompt for AI Agents

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

In @.github/workflows/release.yml around lines 1 - 35, The release workflow's
publish job currently builds and pushes the Docker image without running tests;
add a step before the "Build and push" step in the publish job to run the
project's test suite (e.g., a step named "Run tests" that runs mvn test or the
appropriate test command), fail the job on test failures, and ensure it runs
after checkout but before docker/metadata-action and docker/build-push-action so
only passing commits produce images.

src/test/java/org/example/filter/RateLimitingFilterTest.java (1)

29-36: clearBuckets() in @BeforeEach is a symptom of the static map

Once buckets is converted to an instance field (as suggested in the production code review), calling clearBuckets() becomes unnecessary because each new RateLimitingFilter() in @BeforeEach starts with a fresh map. clearBuckets() and the static escape-hatch can then be removed from the production API.

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/filter/RateLimitingFilterTest.java` around lines 29
- 36, The test is calling filter.clearBuckets() because buckets was a static
field; now that you converted RateLimitingFilter.buckets to an instance field,
remove the unnecessary clearBuckets() call from the `@BeforeEach` in
RateLimitingFilterTest and delete the clearBuckets() method and any "static"
escape-hatch from the RateLimitingFilter class (ensure buckets is an instance
member and normal constructor initialization leaves a fresh map on new
RateLimitingFilter()). Also update any other tests that referenced
clearBuckets() to instead rely on creating a new RateLimitingFilter instance.

src/test/java/org/example/httpparser/HttpParserTest.java (1)

9-9: Consider using Assertions instead of AssertionsForClassTypes.

AssertionsForClassTypes limits available assertions to only class types (no collection/iterable assertions). The idiomatic AssertJ entry point is org.assertj.core.api.Assertions, which is already used in the other test files in this PR (e.g., IpFilterTest.java).

♻️ Suggested fix

-import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.assertj.core.api.Assertions.assertThat;

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/httpparser/HttpParserTest.java` at line 9, Replace
the AssertJ entry import in HttpParserTest: the test currently imports
org.assertj.core.api.AssertionsForClassTypes; change it to use
org.assertj.core.api.Assertions so all assertion types (including
collections/iterables) are available and consistent with other tests (e.g.,
IpFilterTest). Update the import statement in the HttpParserTest source and run
the tests to ensure no other references to AssertionsForClassTypes remain.

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

43-50: Redundant trim() on Line 46 — normalizeIp already trims.

normalizeIp on Line 44 returns either null or ip.trim(). The subsequent clientIp.trim().isEmpty() on Line 46 applies trim() again on an already-trimmed string.

♻️ Suggested fix

-        if (clientIp == null || clientIp.trim().isEmpty()) {
+        if (clientIp == null || clientIp.isEmpty()) {

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/IpFilter.java` around lines 43 - 50, The
check in doFilter redundantly calls trim() on clientIp even though normalizeIp
already returns a trimmed string; update the null/empty check to first test
clientIp == null and then use clientIp.isEmpty() (or clientIp.length() == 0)
instead of clientIp.trim().isEmpty(), keeping the same response behavior
(setStatusCode(HttpResponseBuilder.SC_BAD_REQUEST) and setBody("Bad Request:
Missing client IP address")) so you only remove the redundant trim while
preserving the early return logic.

src/main/java/org/example/TcpServer.java (1)

15-27: Unencrypted ServerSocket and no graceful shutdown.

Two observations:

1. Cleartext traffic (CWE-319): ServerSocket transmits data unencrypted. For production, consider SSLServerSocket via SSLServerSocketFactory. Acceptable if this is a dev/learning project.
2. No shutdown hook: The while (true) loop has no exit condition. Consider tracking the ServerSocket in a field and adding a stop() method or a Runtime.getRuntime().addShutdownHook(...) for graceful cleanup.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/TcpServer.java` around lines 15 - 27, The start()
method opens a plain ServerSocket and spins forever; switch to a configurable,
closable server socket and add graceful shutdown and optional TLS: replace local
ServerSocket with a field (e.g., ServerSocket serverSocket), provide a public
stop() that closes serverSocket, update start() to loop while
(!serverSocket.isClosed()) and catch SocketException for shutdown, and add an
initialization option to create an SSLServerSocket via SSLServerSocketFactory
when TLS is enabled (preserve handleClient(Socket) usage). Also register a
Runtime.getRuntime().addShutdownHook(...) that calls stop() to ensure cleanup on
JVM exit.

src/main/java/org/example/config/AppConfig.java (2)

60-78: Use a proper import for java.util.List and defensively copy the IP lists.

Two things:

1. java.util.List is used as a fully-qualified name in multiple places — add a top-level import for readability.

2. When blockedIps/allowedIps are non-null (e.g., deserialized by Jackson as mutable ArrayLists), they are stored as-is, but when null they default to List.of() (immutable). This creates an inconsistency in the record's immutability guarantees. Wrap with List.copyOf() to ensure immutable, defensively-copied lists in all cases.

♻️ Proposed fix

Add import at the top of the file:

import java.util.List;

Then in IpFilterConfig:

     public IpFilterConfig withDefaultsApplied() {
         Boolean e = (enabled == null) ? false : enabled;
         String m = (mode == null || mode.isBlank()) ? "BLOCKLIST" : mode;
-        java.util.List<String> blocked = (blockedIps == null) ? java.util.List.of() : blockedIps;
-        java.util.List<String> allowed = (allowedIps == null) ? java.util.List.of() : allowedIps;
+        List<String> blocked = (blockedIps == null) ? List.of() : List.copyOf(blockedIps);
+        List<String> allowed = (allowedIps == null) ? List.of() : List.copyOf(allowedIps);
         return new IpFilterConfig(e, m, blocked, allowed);
     }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/config/AppConfig.java` around lines 60 - 78, Add an
import for java.util.List and update IpFilterConfig to use List<String> instead
of fully-qualified names; ensure both defaults() and withDefaultsApplied()
always store immutable defensive copies by wrapping any provided or default
collections with List.copyOf(...) (i.e., use List.copyOf(blockedIps == null ?
List.of() : blockedIps) and similarly for allowedIps) so the record fields are
consistently immutable.

---

73-73: Consider validating mode against known values.

mode accepts any non-blank string. If consumers only handle "BLOCKLIST" and "ALLOWLIST", a typo (e.g., "BLACKLIST") would silently pass through and potentially lead to unexpected behavior at runtime. An early validation or an enum would catch misconfiguration sooner.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/config/AppConfig.java` at line 73, The current
assignment String m = (mode == null || mode.isBlank()) ? "BLOCKLIST" : mode in
AppConfig allows any non-blank string; replace this with explicit validation
against allowed values (e.g., "BLOCKLIST" and "ALLOWLIST") or use an enum (e.g.,
enum Mode {BLOCKLIST, ALLOWLIST}) and parse mode in the AppConfig
initialization; treat null/blank as the default "BLOCKLIST", accept
case-insensitively, and throw an IllegalArgumentException with a clear message
if the provided mode is not one of the allowed values so misconfigurations fail
fast.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b78399 and 4140c95.

📒 Files selected for processing (34)

• .github/workflows/ci.yml
• .github/workflows/release.yml
• Dockerfile
• PortConfigurationGuide.md
• README.md
• pom.xml
• src/main/java/org/example/App.java
• src/main/java/org/example/ConnectionHandler.java
• src/main/java/org/example/StaticFileHandler.java
• src/main/java/org/example/TcpServer.java
• src/main/java/org/example/config/AppConfig.java
• src/main/java/org/example/config/ConfigLoader.java
• src/main/java/org/example/filter/Filter.java
• src/main/java/org/example/filter/FilterChain.java
• src/main/java/org/example/filter/FilterChainImpl.java
• src/main/java/org/example/filter/IpFilter.java
• src/main/java/org/example/filter/RateLimitingFilter.java
• src/main/java/org/example/http/HttpResponseBuilder.java
• src/main/java/org/example/http/MimeTypeDetector.java
• src/main/java/org/example/httpparser/HttpParseRequestLine.java
• src/main/java/org/example/httpparser/HttpParser.java
• src/main/java/org/example/httpparser/HttpRequest.java
• src/main/resources/application.yml
• src/test/java/org/example/AppPortResolutionTest.java
• src/test/java/org/example/StaticFileHandlerTest.java
• src/test/java/org/example/config/ConfigLoaderTest.java
• src/test/java/org/example/filter/IpFilterTest.java
• src/test/java/org/example/filter/RateLimitingFilterTest.java
• src/test/java/org/example/http/HttpResponseBuilderTest.java
• src/test/java/org/example/http/MimeTypeDetectorTest.java
• src/test/java/org/example/httpparser/HttpParseRequestLineTest.java
• src/test/java/org/example/httpparser/HttpParserTest.java
• www/index.html
• www/pageNotFound.html

[coderabbitai]

⚠️ Potential issue | 🟠 Major

application.yml is never loaded at runtime in Docker — config path mismatch

App.java loads the config from the hardcoded relative path "src/main/resources/application.yml". After mvn compile, Maven copies application.yml to target/classes/, and the Dockerfile then places it at /app/application.yml. At runtime the working directory is /app/, so the JVM looks for /app/src/main/resources/application.yml, which does not exist. ConfigLoader silently falls back to defaults, meaning server.port, rootDir, and ipFilter settings in application.yml are never applied in production.

Fix options:

• Option A (recommended): Change App.java to load from the classpath (ClassLoader.getSystemResourceAsStream("application.yml")) instead of a filesystem path.
• Option B: COPY the file to src/main/resources/application.yml relative to WORKDIR, or add an ADD/COPY into the expected path in the runtime stage.

💡 Option A — classpath loading in App.java (outside this file's diff)

// In App.java
try (InputStream is = App.class.getClassLoader().getResourceAsStream("application.yml")) {
    AppConfig appConfig = ConfigLoader.loadFromStream(is); // adjust ConfigLoader accordingly
    ...
}

🤖 Prompt for AI Agents

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

In `@Dockerfile` around lines 1 - 16, App.java currently loads configuration from
the filesystem path "src/main/resources/application.yml", which is missing at
runtime inside the Docker image; update App.java to load application.yml from
the classpath using
App.class.getClassLoader().getResourceAsStream("application.yml") and pass that
InputStream into ConfigLoader (e.g., ConfigLoader.loadFromStream) so the file
packaged in target/classes is used, or alternatively modify the Dockerfile
runtime stage to place application.yml at the expected filesystem path (copy
into /app/src/main/resources/application.yml) if you prefer the filesystem
approach; ensure references to App, ConfigLoader, and application.yml are
updated accordingly.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

buckets map is static — state is shared across all instances

All RateLimitingFilter instances share the same bucket map. In tests, this forces clearBuckets() to be called in @BeforeEach to compensate. In production, this works only because a single filter instance is expected, but any future instantiation (e.g., in testing a second chain, or replacing the filter) will silently reuse stale buckets. The map should be an instance field.

♻️ Proposed fix

-    private static final Map<String, Bucket> buckets = new ConcurrentHashMap<>();
+    private final Map<String, Bucket> buckets = new ConcurrentHashMap<>();

📝 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 static final Map<String, Bucket> buckets = new ConcurrentHashMap<>();
	private final Map<String, Bucket> buckets = new ConcurrentHashMap<>();

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` at line 27, The
buckets map is declared static causing shared state across all
RateLimitingFilter instances; change the declaration of "private static final
Map<String, Bucket> buckets" to an instance field (remove static) so each
RateLimitingFilter has its own map, update any references in the class
(including clearBuckets(), get-or-create logic, and tests) to use the instance
field, ensure constructors initialize the map if needed, and adjust tests that
relied on static clearing to reset or recreate the filter instance instead.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Parse request line with whitespace, not a single space.

split(" ", 3) mis-parses valid request lines containing multiple spaces or tabs (e.g., "GET / HTTP/1.1"). Use a whitespace regex.

✅ Suggested fix

-        String[] requestLineArray = requestLine.trim().split(" ", 3);
+        String[] requestLineArray = requestLine.trim().split("\\s+", 3);

📝 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

	String[] requestLineArray = requestLine.trim().split(" ", 3);
	if (requestLineArray.length <= 2) {
	throw new IOException("HTTP Request Line is not long enough");
	} else {
	setMethod(requestLineArray[0]);
	if (!getMethod().matches("^[A-Z]+$")){
	throw new IOException("Invalid HTTP method");
	}
	setUri(requestLineArray[1]);
	setVersion(requestLineArray[2]);
	String[] requestLineArray = requestLine.trim().split("\\s+", 3);
	if (requestLineArray.length <= 2) {
	throw new IOException("HTTP Request Line is not long enough");
	} else {
	setMethod(requestLineArray[0]);
	if (!getMethod().matches("^[A-Z]+$")){
	throw new IOException("Invalid HTTP method");
	}
	setUri(requestLineArray[1]);
	setVersion(requestLineArray[2]);

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java` around lines
21 - 31, The request-line splitting uses requestLine.trim().split(" ", 3) which
fails on multiple spaces or tabs; update the split to use a whitespace regex
(e.g., requestLine.trim().split("\\s+", 3)) so requestLineArray, populated for
subsequent setMethod, setUri, and setVersion calls, correctly handles runs of
whitespace; keep the existing length check and method validation logic intact.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Content‑Type should reflect the actual file served (sanitized URI or error page), not the raw request URI.

Right now the header is built from the original URI, so index.html?x=y and 404 fallbacks for non‑HTML assets produce incorrect MIME types. Track the served filename and use that for setContentTypeFromFilename(...).

✅ Suggested fix

 public class StaticFileHandler {
     private final String WEB_ROOT;
     private byte[] fileBytes;
     private int statusCode;
+    private String contentTypeName;
 ...
     private void handleGetRequest(String uri) throws IOException {
         // Sanitize URI
         int q = uri.indexOf('?');
         if (q >= 0) uri = uri.substring(0, q);
         int h = uri.indexOf('#');
         if (h >= 0) uri = uri.substring(0, h);
         uri = uri.replace("\0", "");
         if (uri.startsWith("/")) uri = uri.substring(1);
 ...
         if (!file.toPath().startsWith(root.toPath())) {
             fileBytes = "403 Forbidden".getBytes(java.nio.charset.StandardCharsets.UTF_8);
             statusCode = SC_FORBIDDEN;
+            contentTypeName = "forbidden.txt";
             return;
         }

         // Read file
         if (file.isFile()) {
             fileBytes = Files.readAllBytes(file.toPath());
             statusCode = SC_OK;
+            contentTypeName = file.getName();
         } else {
             File errorFile = new File(WEB_ROOT, "pageNotFound.html");
             if (errorFile.isFile()) {
                 fileBytes = Files.readAllBytes(errorFile.toPath());
+                contentTypeName = errorFile.getName();
             } else {
                 fileBytes = "404 Not Found".getBytes(java.nio.charset.StandardCharsets.UTF_8);
+                contentTypeName = "notfound.txt";
             }
             statusCode = SC_NOT_FOUND;
         }
     }

     public void sendGetRequest(OutputStream outputStream, String uri) throws IOException {
         handleGetRequest(uri);
         HttpResponseBuilder response = new HttpResponseBuilder();
         response.setStatusCode(statusCode);
         // Use MimeTypeDetector instead of hardcoded text/html
-        response.setContentTypeFromFilename(uri);
+        response.setContentTypeFromFilename(contentTypeName);
         response.setBody(fileBytes);
         outputStream.write(response.build());
         outputStream.flush();
     }
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/StaticFileHandler.java` around lines 26 - 65,
handleGetRequest currently determines which file is actually served but
sendGetRequest still calls response.setContentTypeFromFilename(uri); update the
flow to track the served filename (e.g., set a new or existing field
servedFilename to file.getName() when a real file is read, or to
errorFile.getName() / "404" fallback for not-found/forbidden cases) inside
handleGetRequest, and then in sendGetRequest call
response.setContentTypeFromFilename(servedFilename) instead of the raw request
uri so the Content-Type matches the actual file served; ensure servedFilename is
sanitized/non-null before use and preserve existing behavior for fileBytes and
statusCode.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Exception cause is lost — pass e as the cause to RuntimeException.

Line 33 uses string concatenation (+ e) which discards the original stack trace. This makes debugging connection failures significantly harder.

🐛 Proposed fix

         } catch (Exception e) {
-            throw new RuntimeException("Error handling client connection " + e);
+            throw new RuntimeException("Error handling client connection", e);
         }

📝 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 void handleClient(Socket client) {
	try (ConnectionHandler connectionHandler = new ConnectionHandler(client)) {
	connectionHandler.runConnectionHandler();
	} catch (Exception e) {
	throw new RuntimeException("Error handling client connection " + e);
	}
	}
	private void handleClient(Socket client) {
	try (ConnectionHandler connectionHandler = new ConnectionHandler(client)) {
	connectionHandler.runConnectionHandler();
	} catch (Exception e) {
	throw new RuntimeException("Error handling client connection", e);
	}
	}

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/TcpServer.java` around lines 29 - 35, The
RuntimeException thrown in handleClient currently concatenates the exception
into the message and loses the original stack trace; modify the catch block in
handleClient (which constructs ConnectionHandler and calls runConnectionHandler)
to rethrow a RuntimeException that includes the original exception as its cause
(use the constructor RuntimeException(String message, Throwable cause) instead
of string concatenation) so the original stack trace from e is preserved.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/main/java/org/example/filter/RateLimitingFilter.java (1)

27-27: ⚠️ Potential issue | 🟠 Major

Avoid static bucket state across filter instances.

buckets being static makes all RateLimitingFilter instances share rate-limit state, and one instance’s destroy() can clear another instance’s buckets.

♻️ Proposed fix

-    private static final Map<String, BucketWrapper> buckets = new ConcurrentHashMap<>();
+    private final Map<String, BucketWrapper> buckets = new ConcurrentHashMap<>();

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` at line 27, The Map
buckets is declared static causing shared state across RateLimitingFilter
instances; change the declaration of buckets to an instance field (remove
static) so each RateLimitingFilter has its own ConcurrentHashMap, and update any
references (e.g., usages in constructor, doFilter, and destroy()) to use the
instance field; ensure destroy() clears only the instance's buckets (not a
static shared map) and update any tests or initializers that assumed a static
map.

🧹 Nitpick comments (1)

src/main/java/org/example/filter/RateLimitingFilter.java (1)

126-135: Narrow test-only helpers from the public API.

getBucketsCount() and ageBucketsForTesting(...) look test-oriented; keeping them public unnecessarily expands production surface.

♻️ Proposed refactor

-    public int getBucketsCount() {
+    int getBucketsCount() {
         return buckets.size();
     }

-    public void ageBucketsForTesting(long millisToSubtract) {
+    void ageBucketsForTesting(long millisToSubtract) {
         for (BucketWrapper wrapper : buckets.values()) {
             long oldTime = wrapper.lastAccessTime;
             wrapper.lastAccessTime = oldTime - millisToSubtract;
         }
     }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 126 -
135, getBucketsCount() and ageBucketsForTesting(...) are test-only helpers
exposed as public; narrow their visibility by removing the public modifier and
making them package-private (no modifier) or, if you use Guava, mark them with
`@VisibleForTesting` while reducing visibility, so they no longer expand the
production API; update references in tests (use same package or call via the
test utilities) and keep the internal types (BucketWrapper, buckets) unchanged.

🤖 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/example/filter/RateLimitingFilter.java`:
- Around line 68-70: The destroy() method currently only clears buckets and does
not stop the background cleanup workers started by startCleanupThread(), which
can spawn multiple long-lived loops; modify the class to track the cleanup
worker (e.g., a volatile Thread cleanupThread or a ScheduledExecutorService
cleanupExecutor created by startCleanupThread()) and ensure startCleanupThread()
only creates one worker if none exists, and destroy() stops it gracefully
(interrupt or shutdown, await termination) before clearing buckets; reference
startCleanupThread(), destroy(), and the buckets collection so the cleanup
lifecycle is properly managed and no duplicate/lingering threads remain.

---

Duplicate comments:
In `@src/main/java/org/example/filter/RateLimitingFilter.java`:
- Line 27: The Map buckets is declared static causing shared state across
RateLimitingFilter instances; change the declaration of buckets to an instance
field (remove static) so each RateLimitingFilter has its own ConcurrentHashMap,
and update any references (e.g., usages in constructor, doFilter, and destroy())
to use the instance field; ensure destroy() clears only the instance's buckets
(not a static shared map) and update any tests or initializers that assumed a
static map.

---

Nitpick comments:
In `@src/main/java/org/example/filter/RateLimitingFilter.java`:
- Around line 126-135: getBucketsCount() and ageBucketsForTesting(...) are
test-only helpers exposed as public; narrow their visibility by removing the
public modifier and making them package-private (no modifier) or, if you use
Guava, mark them with `@VisibleForTesting` while reducing visibility, so they no
longer expand the production API; update references in tests (use same package
or call via the test utilities) and keep the internal types (BucketWrapper,
buckets) unchanged.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4140c95 and e18b048.

📒 Files selected for processing (2)

• src/main/java/org/example/filter/RateLimitingFilter.java
• src/test/java/org/example/filter/RateLimitingFilterTest.java

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

• src/test/java/org/example/filter/RateLimitingFilterTest.java

[coderabbitai]

Actionable comments posted: 9

🧹 Nitpick comments (12)

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

21-25: Defensively validate/copy constructor inputs.

Directly storing filters and terminalHandler can cause NPEs or external mutation side effects.

Suggested fix

     public FilterChainImpl(List<Filter> filters,
                            BiConsumer<HttpRequest, HttpResponseBuilder> terminalHandler) {
-        this.filters = filters;
-        this.terminalHandler = terminalHandler;
+        this.filters = List.copyOf(java.util.Objects.requireNonNull(filters, "filters must not be null"));
+        this.terminalHandler = java.util.Objects.requireNonNull(terminalHandler, "terminalHandler 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/example/filter/FilterChainImpl.java` around lines 21 - 25,
The FilterChainImpl constructor currently stores the incoming List<Filter>
filters and BiConsumer<HttpRequest, HttpResponseBuilder> terminalHandler
directly; add defensive validation and copying: check that filters and
terminalHandler are non-null (and optionally that no element in filters is
null), throw a clear NPE/IAE if violated, and assign a defensive copy of the
list (e.g., new ArrayList<>(filters)) or an unmodifiable wrapper to the
this.filters field to prevent external mutation; similarly assign the validated
terminalHandler to this.terminalHandler. Ensure references to the constructor,
filters field, and terminalHandler are updated accordingly.

src/test/java/org/example/filter/RequestTimeOutFilterTest.java (1)

21-26: Missing @AfterEach cleanup for executor resources.

Each test creates a new RequestTimeOutFilter with its own ExecutorService. Without calling filter.destroy() after each test, thread pools may leak across test runs. Consider adding cleanup:

♻️ Proposed fix

+import org.junit.jupiter.api.AfterEach;
+
 class RequestTimeOutFilterTest {
 
     private RequestTimeOutFilter filter;
     private HttpResponseBuilder response;
     private HttpRequest request;
 
     `@BeforeEach`
     void setUp() {
         filter = new RequestTimeOutFilter(100);
         response = new HttpResponseBuilder();
         request = new HttpRequest("GET", "/", "HTTP/1.1",null,"");
     }
+
+    `@AfterEach`
+    void tearDown() {
+        filter.destroy();
+    }

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/filter/RequestTimeOutFilterTest.java` around lines
21 - 26, Add an `@AfterEach` teardown that calls filter.destroy() to shut down the
ExecutorService created by RequestTimeOutFilter and prevent thread-pool leaks
across tests; locate the test class RequestTimeOutFilterTest (setUp creates
filter = new RequestTimeOutFilter(100)) and implement an `@AfterEach` method
(e.g., tearDown or cleanup) that checks filter != null and invokes
filter.destroy() (or the appropriate shutdown method on RequestTimeOutFilter) to
release resources.

src/test/java/org/example/filter/LocaleFilterWithCookieTest.java (1)

47-62: Consider adding test for cookie priority over Accept-Language header.

The current tests verify cookie and header separately, but there's no test confirming that when both are present, the cookie takes precedence over the header. This is important to verify the documented priority behavior.

💡 Suggested test case

`@Test`
void testCookieTakesPriorityOverHeader() {
    HttpRequest request = new HttpRequest(
            "GET", "/", "HTTP/1.1",
            Map.of(
                    "Cookie", "user-lang=de-DE",
                    "Accept-Language", "fr-FR"
            ),
            null
    );

    LocaleFilterWithCookie filter = new LocaleFilterWithCookie();
    filter.doFilter(request, new HttpResponseBuilder(), (req, res) -> {
        assertEquals("de-DE", LocaleFilterWithCookie.getCurrentLocale());
    });
}

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/filter/LocaleFilterWithCookieTest.java` around
lines 47 - 62, Add a new unit test that verifies cookie precedence over the
Accept-Language header: create an HttpRequest (using the HttpRequest(...)
constructor) containing both "Cookie" with "user-lang=de-DE" and
"Accept-Language" with "fr-FR", instantiate LocaleFilterWithCookie and call
LocaleFilterWithCookie.doFilter(request, new HttpResponseBuilder(), (req, res)
-> { ... }), and inside the callback assert that
LocaleFilterWithCookie.getCurrentLocale() equals "de-DE" to confirm the cookie
wins.

src/main/java/org/example/filter/RateLimitingFilter.java (1)

107-124: resolveClientIp has side effects on response object.

This public static helper modifies the response (sets status code and body) when clientIp is missing. This couples IP resolution with HTTP response handling, making it harder to reuse or test in isolation. Consider returning an Optional<String> or a result object instead, and letting the caller handle the error response.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 107 -
124, The resolveClientIp helper currently mutates HttpResponseBuilder when the
client IP is missing; change its signature to return Optional<String> (or a
small result object) instead of taking HttpResponseBuilder, and remove any calls
that set status/body from inside resolveClientIp; keep logic that reads
request.getAttribute("clientIp") and X-Forwarded-For parsing, but return
Optional.empty() when no valid IP is found so callers (e.g., code that invoked
resolveClientIp) can set HttpResponseBuilder.SC_BAD_REQUEST and the response
body themselves.

src/test/java/org/example/filter/CompressionFilterTest.java (2)

18-51: Consider verifying response headers after compression.

The test verifies the body is compressed and decompresses correctly, but doesn't assert that Content-Encoding: gzip and Vary: Accept-Encoding headers are set. These are important for HTTP compliance.

💡 Suggested assertions

assertEquals("gzip", response.getHeader("Content-Encoding"), 
    "Content-Encoding should be set to gzip");
assertTrue(response.getHeader("Vary").contains("Accept-Encoding"),
    "Vary header should include Accept-Encoding");

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/filter/CompressionFilterTest.java` around lines 18
- 51, Add assertions in testGzipCompressionWhenClientSupportsIt to verify that
the CompressionFilter set the proper response headers: assert that
response.getHeader("Content-Encoding") equals "gzip" and that
response.getHeader("Vary") contains "Accept-Encoding"; update the test after
calling filter.doFilter(...) and before decompressing to check these headers on
the HttpResponseBuilder (or its getHeader method) to ensure HTTP compliance for
CompressionFilter behavior.

---

113-131: Avoid reflection; use public API instead.

HttpResponseBuilder exposes getByteBody() and getBody() publicly. Using reflection to access private fields is fragile and will break if implementation details change.

♻️ Proposed fix

     private byte[] getBodyFromResponse(HttpResponseBuilder response) {
-        try {
-            var field = response.getClass().getDeclaredField("bytebody");
-            field.setAccessible(true);
-            byte[] bytebody = (byte[]) field.get(response);
-
-            if (bytebody != null) {
-                return bytebody;
-            }
-
-            var bodyField = response.getClass().getDeclaredField("body");
-            bodyField.setAccessible(true);
-            String body = (String) bodyField.get(response);
-            return body.getBytes(StandardCharsets.UTF_8);
-
-        } catch (Exception e) {
-            throw new RuntimeException("Failed to get body", e);
-        }
+        byte[] byteBody = response.getByteBody();
+        if (byteBody != null) {
+            return byteBody;
+        }
+        return response.getBody().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/example/filter/CompressionFilterTest.java` around lines 113
- 131, The test's getBodyFromResponse uses reflection to read private fields on
HttpResponseBuilder; replace it to call the public accessors instead: call
response.getByteBody() and if non-null return that, otherwise call
response.getBody() and return its UTF-8 bytes (use StandardCharsets.UTF_8).
Update the getBodyFromResponse method to reference
HttpResponseBuilder#getByteBody and HttpResponseBuilder#getBody rather than
using getDeclaredField or setAccessible.

src/main/java/org/example/filter/RequestTimeOutFilter.java (2)

22-28: Executor configuration: keepAliveTime has no effect with current settings.

Since corePoolSize == maximumPoolSize, threads are never considered idle and won't be terminated by the keep-alive timer. The 60-second keepAliveTime is effectively unused. This is fine if the intent is to maintain a fixed-size pool, but worth noting for clarity.

Also, the queue capacity of 50 may be limiting under high load. Consider making this configurable.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RequestTimeOutFilter.java` around lines 22 -
28, The ThreadPoolExecutor instantiation for the field executor currently sets
corePoolSize equal to maximumPoolSize so the 60s keepAliveTime is never used;
either make the pool truly dynamic by increasing maximumPoolSize (e.g., use
different values for core vs max) or call allowCoreThreadTimeOut(true) on the
ThreadPoolExecutor instance so the keepAliveTime can apply to core threads; also
replace the hard-coded new ArrayBlockingQueue<>(50) with a configurable queue
size (inject or read a property) so queue capacity can be tuned under load;
locate the constructor call creating the executor (the executor field /
ThreadPoolExecutor creation) and apply these changes there.

---

70-78: Timeout cancellation may not interrupt blocking I/O.

future.cancel(true) sets the interrupt flag, but if the downstream filter chain is blocked on I/O (e.g., socket read), it won't be interrupted. The response is correctly returned as 504, but the background task may continue running until it naturally completes or hits an interruptible point.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RequestTimeOutFilter.java` around lines 70 -
78, The timeout handling in RequestTimeOutFilter currently calls
future.cancel(true) in the TimeoutException handler, but that won't necessarily
stop a downstream filter chain blocked on I/O; modify the implementation so the
executing Thread is recorded when you submit the task (e.g., inside the
Callable/Runnable capture Thread.currentThread() and store it keyed by request
id) and in the catch (TimeoutException e) not only call future.cancel(true) but
also call thread.interrupt() on that recorded Thread and attempt to close the
request/response I/O (e.g., close input/output streams or the underlying socket)
or use the container async timeout APIs (AsyncContext) to force completion;
ensure cleanup removes the stored Thread reference and that the recording
happens in the same task submitted to the ExecutorService so the correct Thread
is interrupted.

src/main/java/org/example/filter/LocaleFilterWithCookie.java (1)

77-106: Consider validating locale format.

The cookie value is used directly without validation. A malformed or malicious cookie value (e.g., very long string, special characters) could propagate through the system. Consider basic validation or length limiting.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/LocaleFilterWithCookie.java` around lines 77
- 106, The extractLocaleFromCookie method currently returns the cookie value for
LOCALE_COOKIE_NAME without validation; add validation in extractLocaleFromCookie
to reject or sanitize values that don't match an expected locale pattern (e.g.,
two-letter language optionally hyphen/underscore region like en or en-US), and
enforce a reasonable max length (e.g., <= 10 chars) to prevent very long or
malicious input; upon invalid value return null (or fall back to default) so
downstream code never receives malformed locale strings.

src/test/java/org/example/ConnectionHandlerTest.java (1)

57-57: Use explicit charset when decoding test responses.

ByteArrayOutputStream#toString() uses platform default charset, which can make assertions flaky across environments.

✅ Suggested fix

-        String response = outputStream.toString();
+        String response = outputStream.toString(StandardCharsets.UTF_8);

Also applies to: 82-82, 104-104

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/ConnectionHandlerTest.java` at line 57, In
ConnectionHandlerTest replace platform‑default decoding of the
ByteArrayOutputStream (the lines where you do String response =
outputStream.toString()) with an explicit charset-based decode using
StandardCharsets.UTF_8 (or StandardCharsets.UTF_8.name()) so the test output is
deterministic across environments; update the same pattern for the other
occurrences around the response variables at the other two locations (the other
outputStream -> response assignments referenced in the file). Ensure you modify
the instances that reference outputStream and response in ConnectionHandlerTest
so assertions use the UTF-8 decoded string.

src/main/java/org/example/TcpServer.java (1)

26-31: Clarify TLS strategy for production deployments.

This server currently accepts cleartext TCP/HTTP. If exposed beyond trusted internal networks, terminate TLS (reverse proxy) or use SSL sockets.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/TcpServer.java` around lines 26 - 31, The server
currently creates a plain ServerSocket in TcpServer and accepts cleartext
connections (see ServerSocket creation and handleClient usage); for production
either change the socket creation to use an SSLServerSocket (configure an
SSLContext/KeyManager with keystore and replace new ServerSocket(port) with the
SSLServerSocket from SSLServerSocketFactory) or document and enforce TLS
termination upstream (e.g., require deployment behind a reverse proxy/SSL
terminator) and add startup checks in TcpServer to fail fast if not configured
for protected deployment; update logging and any docs to clearly state which
strategy (in-process SSL via SSLServerSocket or external TLS termination) is
required.

src/main/java/org/example/server/ConfigurableFilterPipeline.java (1)

43-55: Consider documenting the filter ordering semantics.

Global filters always execute before route-scoped filters regardless of their order values. For instance, a global filter with order=100 will still run before a route filter with order=1. If this is intentional, consider adding a class-level Javadoc clarifying this behavior, as users might expect a unified ordering across all filters.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/server/ConfigurableFilterPipeline.java` around
lines 43 - 55, Add class-level Javadoc to ConfigurableFilterPipeline explaining
that global filters (globalRegs) are always executed before route-scoped filters
(routeRegs) regardless of their order values; document that within each scope
filters are ordered by the Comparator comparing FilterRegistration::order but
there is no unified ordering across both scopes, and give a brief example (e.g.,
global order=100 runs before route order=1) so callers understand the semantics
and avoid surprises.

🤖 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/example/ConnectionHandler.java`:
- Around line 100-105: In resolveTargetFile, the incoming uri is assigned
directly (e.g., "index.html?v=1") which breaks static-file lookup; normalize by
stripping any query string and fragment before assigning this.uri: take the
input uri (in resolveTargetFile), trim leading "/", then split on '?' and '#'
and use the first segment (or "index.html" for null or "/") so this.uri contains
only the path component used for file resolution.

In `@src/main/java/org/example/filter/FilterChainImpl.java`:
- Around line 13-34: FilterChainImpl currently stores traversal state in the
mutable field "index", causing reuse and concurrency bugs; change doFilter to
avoid instance-level index by removing the "index" field and implementing
traversal with a local index or an overloaded private doFilter(HttpRequest,
HttpResponseBuilder, int) that uses a local int position to iterate through
"filters" and invoke each Filter.doFilter, falling back to "terminalHandler"
when the local index reaches filters.size(); ensure constructors and the public
doFilter(HttpRequest, HttpResponseBuilder) call the new local-index traversal so
each invocation (and concurrent invocations) use their own traversal state.

In `@src/main/java/org/example/filter/LoggingFilter.java`:
- Around line 21-26: The LoggingFilter currently swallows exceptions thrown by
chain.doFilter, losing failure context; update the try/catch around
chain.doFilter(request, response) to, upon catching Exception e, set the
response status to HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR if it was SC_OK
(using response.getStatusCode() and
response.setStatusCode(HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR)), and then
rethrow the original exception (preserve and throw e) so runtime/checked
exceptions propagate instead of being suppressed.

In `@src/main/java/org/example/filter/RateLimitingFilter.java`:
- Around line 117-121: The current RateLimitingFilter code unconditionally
trusts the X-Forwarded-For header to determine client IP, which can be spoofed.
To fix this, update the code to check if the request originates from a trusted
proxy IP before using the X-Forwarded-For header for clientIp extraction.
Implement a configuration mechanism to define and verify trusted proxy IPs,
falling back to the remote address if the proxy is untrusted. Additionally, add
documentation comments explaining this security consideration.
- Line 135: The catch uses the unnamed pattern variable catch
(InterruptedException _) which requires Java 21+; update the catch in
RateLimitingFilter (the InterruptedException handler) to use a named variable
(e.g., catch (InterruptedException e)) and handle it appropriately — at minimum
call Thread.currentThread().interrupt() and log or rethrow via the existing
logger/exception path used in the surrounding method to preserve interrupt
status and error visibility.

In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 98-100: The getBodyBytes method in HttpResponseBuilder returns the
internal mutable field bytebody directly, which can be mutated by callers;
change getBodyBytes to return a defensive copy of bytebody (e.g.,
Arrays.copyOf(bytebody, bytebody.length)) when bytebody is non-null, and
otherwise return body.getBytes(StandardCharsets.UTF_8) as before; ensure any
place that sets bytebody stores a copy rather than a caller-owned array so
internal state is never exposed.

In `@src/main/java/org/example/TcpServer.java`:
- Around line 50-52: The catch block in TcpServer that currently calls
handleInternalServerError(client) swallows the exception; modify that catch in
the request handling code to log the caught Exception e (including message and
stacktrace) before invoking handleInternalServerError(client), using the class
logger (or System.err if no logger exists), and consider passing the exception
into handleInternalServerError (e.g., handleInternalServerError(client, e)) so
the handler can include root-cause details in the response/logs; update the
handleInternalServerError signature and its callers accordingly if you choose to
pass the exception through.

In `@src/test/java/org/example/filter/RateLimitingFilterIpTest.java`:
- Around line 37-46: The test fails because resolveClientIp calls
request.getHeaders().get("X-Forwarded-For") but getHeaders() is not stubbed,
causing an NPE; update the test (RateLimitingFilterIpTest) to stub
request.getHeaders() to return a Map/MultivaluedMap (e.g., an empty map or one
containing "X-Forwarded-For") before calling resolveClientIp, keeping the
existing getAttribute stubs intact so resolveClientIp can safely call
request.getHeaders().get("X-Forwarded-For") and fall back to
request.getAttribute("clientIp") as intended.
- Around line 40-41: In RateLimitingFilterIpTest remove the incorrect redundant
stub that sets when(request.getAttribute("clientIp")).thenReturn(Map.of()) and
keep only the correct stub
when(request.getAttribute("clientIp")).thenReturn("10.0.0.5"); — locate the
duplicate stubbing of request.getAttribute("clientIp") in the test method and
delete the Map.of() return so the mocked request returns the expected String IP.

---

Nitpick comments:
In `@src/main/java/org/example/filter/FilterChainImpl.java`:
- Around line 21-25: The FilterChainImpl constructor currently stores the
incoming List<Filter> filters and BiConsumer<HttpRequest, HttpResponseBuilder>
terminalHandler directly; add defensive validation and copying: check that
filters and terminalHandler are non-null (and optionally that no element in
filters is null), throw a clear NPE/IAE if violated, and assign a defensive copy
of the list (e.g., new ArrayList<>(filters)) or an unmodifiable wrapper to the
this.filters field to prevent external mutation; similarly assign the validated
terminalHandler to this.terminalHandler. Ensure references to the constructor,
filters field, and terminalHandler are updated accordingly.

In `@src/main/java/org/example/filter/LocaleFilterWithCookie.java`:
- Around line 77-106: The extractLocaleFromCookie method currently returns the
cookie value for LOCALE_COOKIE_NAME without validation; add validation in
extractLocaleFromCookie to reject or sanitize values that don't match an
expected locale pattern (e.g., two-letter language optionally hyphen/underscore
region like en or en-US), and enforce a reasonable max length (e.g., <= 10
chars) to prevent very long or malicious input; upon invalid value return null
(or fall back to default) so downstream code never receives malformed locale
strings.

In `@src/main/java/org/example/filter/RateLimitingFilter.java`:
- Around line 107-124: The resolveClientIp helper currently mutates
HttpResponseBuilder when the client IP is missing; change its signature to
return Optional<String> (or a small result object) instead of taking
HttpResponseBuilder, and remove any calls that set status/body from inside
resolveClientIp; keep logic that reads request.getAttribute("clientIp") and
X-Forwarded-For parsing, but return Optional.empty() when no valid IP is found
so callers (e.g., code that invoked resolveClientIp) can set
HttpResponseBuilder.SC_BAD_REQUEST and the response body themselves.

In `@src/main/java/org/example/filter/RequestTimeOutFilter.java`:
- Around line 22-28: The ThreadPoolExecutor instantiation for the field executor
currently sets corePoolSize equal to maximumPoolSize so the 60s keepAliveTime is
never used; either make the pool truly dynamic by increasing maximumPoolSize
(e.g., use different values for core vs max) or call
allowCoreThreadTimeOut(true) on the ThreadPoolExecutor instance so the
keepAliveTime can apply to core threads; also replace the hard-coded new
ArrayBlockingQueue<>(50) with a configurable queue size (inject or read a
property) so queue capacity can be tuned under load; locate the constructor call
creating the executor (the executor field / ThreadPoolExecutor creation) and
apply these changes there.
- Around line 70-78: The timeout handling in RequestTimeOutFilter currently
calls future.cancel(true) in the TimeoutException handler, but that won't
necessarily stop a downstream filter chain blocked on I/O; modify the
implementation so the executing Thread is recorded when you submit the task
(e.g., inside the Callable/Runnable capture Thread.currentThread() and store it
keyed by request id) and in the catch (TimeoutException e) not only call
future.cancel(true) but also call thread.interrupt() on that recorded Thread and
attempt to close the request/response I/O (e.g., close input/output streams or
the underlying socket) or use the container async timeout APIs (AsyncContext) to
force completion; ensure cleanup removes the stored Thread reference and that
the recording happens in the same task submitted to the ExecutorService so the
correct Thread is interrupted.

In `@src/main/java/org/example/server/ConfigurableFilterPipeline.java`:
- Around line 43-55: Add class-level Javadoc to ConfigurableFilterPipeline
explaining that global filters (globalRegs) are always executed before
route-scoped filters (routeRegs) regardless of their order values; document that
within each scope filters are ordered by the Comparator comparing
FilterRegistration::order but there is no unified ordering across both scopes,
and give a brief example (e.g., global order=100 runs before route order=1) so
callers understand the semantics and avoid surprises.

In `@src/main/java/org/example/TcpServer.java`:
- Around line 26-31: The server currently creates a plain ServerSocket in
TcpServer and accepts cleartext connections (see ServerSocket creation and
handleClient usage); for production either change the socket creation to use an
SSLServerSocket (configure an SSLContext/KeyManager with keystore and replace
new ServerSocket(port) with the SSLServerSocket from SSLServerSocketFactory) or
document and enforce TLS termination upstream (e.g., require deployment behind a
reverse proxy/SSL terminator) and add startup checks in TcpServer to fail fast
if not configured for protected deployment; update logging and any docs to
clearly state which strategy (in-process SSL via SSLServerSocket or external TLS
termination) is required.

In `@src/test/java/org/example/ConnectionHandlerTest.java`:
- Line 57: In ConnectionHandlerTest replace platform‑default decoding of the
ByteArrayOutputStream (the lines where you do String response =
outputStream.toString()) with an explicit charset-based decode using
StandardCharsets.UTF_8 (or StandardCharsets.UTF_8.name()) so the test output is
deterministic across environments; update the same pattern for the other
occurrences around the response variables at the other two locations (the other
outputStream -> response assignments referenced in the file). Ensure you modify
the instances that reference outputStream and response in ConnectionHandlerTest
so assertions use the UTF-8 decoded string.

In `@src/test/java/org/example/filter/CompressionFilterTest.java`:
- Around line 18-51: Add assertions in testGzipCompressionWhenClientSupportsIt
to verify that the CompressionFilter set the proper response headers: assert
that response.getHeader("Content-Encoding") equals "gzip" and that
response.getHeader("Vary") contains "Accept-Encoding"; update the test after
calling filter.doFilter(...) and before decompressing to check these headers on
the HttpResponseBuilder (or its getHeader method) to ensure HTTP compliance for
CompressionFilter behavior.
- Around line 113-131: The test's getBodyFromResponse uses reflection to read
private fields on HttpResponseBuilder; replace it to call the public accessors
instead: call response.getByteBody() and if non-null return that, otherwise call
response.getBody() and return its UTF-8 bytes (use StandardCharsets.UTF_8).
Update the getBodyFromResponse method to reference
HttpResponseBuilder#getByteBody and HttpResponseBuilder#getBody rather than
using getDeclaredField or setAccessible.

In `@src/test/java/org/example/filter/LocaleFilterWithCookieTest.java`:
- Around line 47-62: Add a new unit test that verifies cookie precedence over
the Accept-Language header: create an HttpRequest (using the HttpRequest(...)
constructor) containing both "Cookie" with "user-lang=de-DE" and
"Accept-Language" with "fr-FR", instantiate LocaleFilterWithCookie and call
LocaleFilterWithCookie.doFilter(request, new HttpResponseBuilder(), (req, res)
-> { ... }), and inside the callback assert that
LocaleFilterWithCookie.getCurrentLocale() equals "de-DE" to confirm the cookie
wins.

In `@src/test/java/org/example/filter/RequestTimeOutFilterTest.java`:
- Around line 21-26: Add an `@AfterEach` teardown that calls filter.destroy() to
shut down the ExecutorService created by RequestTimeOutFilter and prevent
thread-pool leaks across tests; locate the test class RequestTimeOutFilterTest
(setUp creates filter = new RequestTimeOutFilter(100)) and implement an
`@AfterEach` method (e.g., tearDown or cleanup) that checks filter != null and
invokes filter.destroy() (or the appropriate shutdown method on
RequestTimeOutFilter) to release resources.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e18b048 and 0583b14.

⛔ Files ignored due to path filters (2)

• src/main/resources/test.jpg is excluded by !**/*.jpg
• www/test.jpg is excluded by !**/*.jpg

📒 Files selected for processing (28)

• pom.xml
• src/main/java/org/example/App.java
• src/main/java/org/example/ConnectionFactory.java
• src/main/java/org/example/ConnectionHandler.java
• src/main/java/org/example/TcpServer.java
• src/main/java/org/example/config/ConfigLoader.java
• src/main/java/org/example/filter/CompressionFilter.java
• src/main/java/org/example/filter/FilterChainImpl.java
• src/main/java/org/example/filter/LocaleFilter.java
• src/main/java/org/example/filter/LocaleFilterWithCookie.java
• src/main/java/org/example/filter/LoggingFilter.java
• src/main/java/org/example/filter/RateLimitingFilter.java
• src/main/java/org/example/filter/RequestTimeOutFilter.java
• src/main/java/org/example/http/HttpResponseBuilder.java
• src/main/java/org/example/server/ConfigurableFilterPipeline.java
• src/main/java/org/example/server/FilterRegistration.java
• src/main/java/org/example/server/RoutePattern.java
• src/test/java/org/example/ConnectionHandlerTest.java
• src/test/java/org/example/TcpServerTest.java
• src/test/java/org/example/config/ConfigLoaderTest.java
• src/test/java/org/example/filter/CompressionFilterTest.java
• src/test/java/org/example/filter/LocaleFilterTest.java
• src/test/java/org/example/filter/LocaleFilterWithCookieTest.java
• src/test/java/org/example/filter/LoggingFilterTest.java
• src/test/java/org/example/filter/RateLimitingFilterIpTest.java
• src/test/java/org/example/filter/RequestTimeOutFilterTest.java
• src/test/java/org/example/server/ConfigurableFilterPipelineTest.java
• src/test/resources/application.yml

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

• src/test/resources/application.yml

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

• src/test/java/org/example/config/ConfigLoaderTest.java

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Normalize URI before file resolution (query/fragment currently break static-file lookup).

At Line 104, paths like /index.html?v=1 become index.html?v=1, which will miss the file and return 404 incorrectly.

✅ Suggested fix

 private void resolveTargetFile(String uri) {
-    if (uri == null || "/".equals(uri)) {
+    if (uri == null) {
+        this.uri = "index.html";
+        return;
+    }
+
+    int q = uri.indexOf('?');
+    if (q >= 0) uri = uri.substring(0, q);
+    int h = uri.indexOf('#');
+    if (h >= 0) uri = uri.substring(0, h);
+
+    if (uri.isEmpty() || "/".equals(uri)) {
         this.uri = "index.html";
     } else {
         this.uri = uri.startsWith("/") ? uri.substring(1) : uri;
     }
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/ConnectionHandler.java` around lines 100 - 105, In
resolveTargetFile, the incoming uri is assigned directly (e.g.,
"index.html?v=1") which breaks static-file lookup; normalize by stripping any
query string and fragment before assigning this.uri: take the input uri (in
resolveTargetFile), trim leading "/", then split on '?' and '#' and use the
first segment (or "index.html" for null or "/") so this.uri contains only the
path component used for file resolution.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

FilterChainImpl holds mutable traversal state, making it unsafe to reuse.

index is shared instance state; after one full pass, later calls can skip filters. Concurrent use can also race on index.

Suggested direction

-    private int index = 0;
+    // Avoid shared mutable traversal state.

     `@Override`
     public void doFilter(HttpRequest request, HttpResponseBuilder response) {
-        if (index < filters.size()) {
-            Filter next = filters.get(index++);
-            next.doFilter(request, response, this);
-        } else {
-            terminalHandler.accept(request, response);
-        }
+        doFilterAt(request, response, 0);
+    }
+
+    private void doFilterAt(HttpRequest request, HttpResponseBuilder response, int currentIndex) {
+        if (currentIndex < filters.size()) {
+            Filter next = filters.get(currentIndex);
+            next.doFilter(request, response, (req, resp) -> doFilterAt(req, resp, currentIndex + 1));
+            return;
+        }
+        terminalHandler.accept(request, response);
     }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/FilterChainImpl.java` around lines 13 - 34,
FilterChainImpl currently stores traversal state in the mutable field "index",
causing reuse and concurrency bugs; change doFilter to avoid instance-level
index by removing the "index" field and implementing traversal with a local
index or an overloaded private doFilter(HttpRequest, HttpResponseBuilder, int)
that uses a local int position to iterate through "filters" and invoke each
Filter.doFilter, falling back to "terminalHandler" when the local index reaches
filters.size(); ensure constructors and the public doFilter(HttpRequest,
HttpResponseBuilder) call the new local-index traversal so each invocation (and
concurrent invocations) use their own traversal state.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don’t swallow downstream exceptions in the logging filter.

On Line 23, exceptions are caught and suppressed. This can hide failures and let execution continue with partial/error state. Set status, then rethrow runtime exceptions.

Suggested fix

-        } catch (Exception e) {
-            if(response.getStatusCode() == HttpResponseBuilder.SC_OK)
-                response.setStatusCode(HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR);
+        } catch (RuntimeException e) {
+            if (response.getStatusCode() == HttpResponseBuilder.SC_OK) {
+                response.setStatusCode(HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR);
+            }
+            throw 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

	try {
	chain.doFilter(request, response);
	} catch (Exception e) {
	if(response.getStatusCode() == HttpResponseBuilder.SC_OK)
	response.setStatusCode(HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR);
	} finally {
	try {
	chain.doFilter(request, response);
	} catch (RuntimeException e) {
	if (response.getStatusCode() == HttpResponseBuilder.SC_OK) {
	response.setStatusCode(HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR);
	}
	throw e;
	} finally {

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/LoggingFilter.java` around lines 21 - 26,
The LoggingFilter currently swallows exceptions thrown by chain.doFilter, losing
failure context; update the try/catch around chain.doFilter(request, response)
to, upon catching Exception e, set the response status to
HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR if it was SC_OK (using
response.getStatusCode() and
response.setStatusCode(HttpResponseBuilder.SC_INTERNAL_SERVER_ERROR)), and then
rethrow the original exception (preserve and throw e) so runtime/checked
exceptions propagate instead of being suppressed.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

X-Forwarded-For can be spoofed by clients.

The filter trusts the X-Forwarded-For header unconditionally. Malicious clients can spoof this header to bypass rate limiting. Consider:

1. Only trusting X-Forwarded-For from known proxies
2. Configuring trusted proxy IPs
3. Documenting the security implications

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 117 -
121, The current RateLimitingFilter code unconditionally trusts the
X-Forwarded-For header to determine client IP, which can be spoofed. To fix
this, update the code to check if the request originates from a trusted proxy IP
before using the X-Forwarded-For header for clientIp extraction. Implement a
configuration mechanism to define and verify trusted proxy IPs, falling back to
the remote address if the proxy is untrusted. Additionally, add documentation
comments explaining this security consideration.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

Java unnamed variables catch clause which version introduced

💡 Result:

Unnamed variables in a catch clause (e.g., catch (IOException _) { ... }) were introduced as a preview feature in Java 21 (JEP 443). (docs.oracle.com)

They were finalized in Java 22 (JEP 456) (no longer preview). (openjdk.org)

Citations:

• 1: https://docs.oracle.com/en/java/javase/21/language/java-language-changes-release.html?utm_source=openai
• 2: https://openjdk.org/jeps/456?utm_source=openai

---

Unnamed variable _ for caught exception requires Java 21+.

The unnamed pattern variable (catch (InterruptedException _)) is available as a preview feature in Java 21 (JEP 443) and was finalized in Java 22+ (JEP 456). If targeting earlier Java versions, use a named variable.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/filter/RateLimitingFilter.java` at line 135, The
catch uses the unnamed pattern variable catch (InterruptedException _) which
requires Java 21+; update the catch in RateLimitingFilter (the
InterruptedException handler) to use a named variable (e.g., catch
(InterruptedException e)) and handle it appropriately — at minimum call
Thread.currentThread().interrupt() and log or rethrow via the existing
logger/exception path used in the surrounding method to preserve interrupt
status and error visibility.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Avoid leaking internal mutable byte array.

On Line 99, returning bytebody directly exposes internal state and allows unintended mutation.

Suggested fix

     public byte[] getBodyBytes() {
-        if (bytebody != null) return bytebody;
+        if (bytebody != null) return Arrays.copyOf(bytebody, bytebody.length);
         return body.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/example/http/HttpResponseBuilder.java` around lines 98 -
100, The getBodyBytes method in HttpResponseBuilder returns the internal mutable
field bytebody directly, which can be mutated by callers; change getBodyBytes to
return a defensive copy of bytebody (e.g., Arrays.copyOf(bytebody,
bytebody.length)) when bytebody is non-null, and otherwise return
body.getBytes(StandardCharsets.UTF_8) as before; ensure any place that sets
bytebody stores a copy rather than a caller-owned array so internal state is
never exposed.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Request-processing exceptions are swallowed without root-cause logging.

At Line 50, failures are converted to 500 responses, but the exception detail is dropped, making production debugging much harder.

✅ Suggested fix

-        } catch (Exception e) {
-            handleInternalServerError(client);
+        } catch (Exception e) {
+            handleInternalServerError(client, e);
         } finally {
             if(handler != null)
                 handler.close();
         }
     }

-
-    private void handleInternalServerError(Socket client){
+    private void handleInternalServerError(Socket client, Exception cause){
+        System.err.println("Request processing failed: " + cause.getMessage());
         HttpResponseBuilder response = new HttpResponseBuilder();

📝 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 (Exception e) {
	handleInternalServerError(client);
	} finally {
	} catch (Exception e) {
	handleInternalServerError(client, e);
	} finally {
	if(handler != null)
	handler.close();
	}
	}
	private void handleInternalServerError(Socket client, Exception cause){
	System.err.println("Request processing failed: " + cause.getMessage());
	HttpResponseBuilder response = new HttpResponseBuilder();

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/TcpServer.java` around lines 50 - 52, The catch
block in TcpServer that currently calls handleInternalServerError(client)
swallows the exception; modify that catch in the request handling code to log
the caught Exception e (including message and stacktrace) before invoking
handleInternalServerError(client), using the class logger (or System.err if no
logger exists), and consider passing the exception into
handleInternalServerError (e.g., handleInternalServerError(client, e)) so the
handler can include root-cause details in the response/logs; update the
handleInternalServerError signature and its callers accordingly if you choose to
pass the exception through.