Skip to content

Implement /health Endpoint #75#19

Closed
dinbah18 wants to merge 13 commits intomainfrom
feature/75
Closed

Implement /health Endpoint #75#19
dinbah18 wants to merge 13 commits intomainfrom
feature/75

Conversation

@dinbah18
Copy link

@dinbah18 dinbah18 commented Feb 22, 2026

Closes #75

Summary by CodeRabbit

Release Notes

  • New Features

    • Application now runs as an HTTP web server on port 8080
    • Serves static files from the www directory with automatic routing (/ → index.html)
    • Added /health endpoint for health checks
  • DevOps & Infrastructure

    • Docker multi-stage containerization support added
    • GitHub Actions CI/CD pipeline configured for automated testing and Docker image publishing
  • Tests

    • Comprehensive test coverage added for HTTP parsing and response handling
  • Dependencies

    • Updated Java compiler to version 25; added bucket4j, Jackson, and testing utilities

Kathify and others added 13 commits February 10, 2026 14:44
Add ci workflow for github actions.
* build: configure pom.xml with needed plugin/tools.
Setup Java 25 environment with JUnit 5, Mockito, JaCoCo, Pitest, and Spotless

* fix: add missing test scope for awaitility
* release.yml that builds and publishes Docker image to GitHub packages on release.

* Fixed unverified commit stopping pull request from being merged
* Dockerfile that builds an image in a docker container then runs in another docker container

Current implementation uses Temporary App.class reference before relevant file is created to start server.

* Fixed unverified commit

* Set up non-root user and updated Dockerfile to use user. Fixed file path to use /app/ instead of /app/org/example to prevent unnessary nesting of packages.
* added parser class for headers

* refactoring

* refactoring

* refactoring, added getHeadersMap

* refactoring

* refactoring code, adding debug boolean

* added unit tests for HttpParser.java class

* refactoring and all green tests for class

* refactoring the code

* refactoring the bufferedReader

* refactoring with coderabbit feedback for header put to merge

* refactoring with coderabbit feedback for header put to merge, changed headersmap

* refactoring from reviewer input

* refactoring for code rabbit comments
* Add HttpResponseBuilder and its tests

Signed-off-by: JohanHiths <johan.hietala@iths.se>

* Implement minimal HTTP response builder

Signed-off-by: JohanHiths <johan.hietala@iths.se>

* update coderabbit

* more coderabbit fixes

Signed-off-by: JohanHiths <johan.hietala@iths.se>

* Bytte från httptest till http efter rekommendationer från Xeutos

* code rabbit fix

* fixed more code rabbit problems

---------

Signed-off-by: JohanHiths <johan.hietala@iths.se>
* added parser class for headers

* refactoring

* refactoring

* refactoring, added getHeadersMap

* refactoring

* adding class for parsing requestLine (method, path, version). Has getters/setters for the three.

* refactoring code, private setters

* added tests for throwing error

* refactoring the bufferedReader

* refactoring, adding check for requestLineArray.length() is less or equals to two with added unit test

* refactoring, adding check for requestLineArray.length() is less or equals to two with added unit test

* refactoring, adding test for null value of input stream

* refactoring from coderabbit input

* refactoring test class.

* refactoring, adding debug boolean flag for logger lines

* refactoring and testing buffered reader stream

* refactoring, changed class HttpParser.java to extend HttpParseRequestLine.java and making them share the same buffered reader. Added method to set the reader and then methods to call the HttpParser

* refactoring, changed class HttpParser.java to extend HttpParseRequestLine.java and making them share the same buffered reader. Added method to set the reader and then methods to call the HttpParser
* Add Bucket4j dependency in pom file

* Extract Bucket4j version into a property and update version in dependency for consistency
* Add support for serving static files

- Introduced `StaticFileHandler` to serve static files from the `www` directory.
- Enhanced `HttpResponseBuilder` to support byte array bodies.
- Made `setReader` method public in `HttpParser` to improve flexibility.
- Code tested via Insomnia by coding a temporary method for multithreads in TcpServer.java, this is now removed before future implementations of ConnectionHandler

Co-authored-by: Daniel Fahlén xsz300@gmail.com

* Fix typo in `HttpResponseBuilder` (contentLength) and refine body handling logic
Co-authored-by: Daniel Fahlén xsz300@gmail.com

* Add basic HTML structure to `index.html` in `www` directory

* Make `handleGetRequest` method private in `StaticFileHandler` to encapsulate functionality
* Updates pom.xml, with jackson-dependencies, for config file

* Updates pom.xml, removes jackson-annotations:2.20, because it was apparently unnecessary.
* * Move HTTP handling logic from TcpServer to a dedicated ConnectionTask class
* Implement virtual thread execution for better scalability
* Add regex-based URI routing to support clean URLs and default to index.html
* Ensure sockets are properly closed

Co-authored-by: Caroline Nordbrandt <caroline_nordbradt@hotmail.com>

* change thrown exception to runtime instead with appropiet message in tcpServer.

---------

Co-authored-by: Caroline Nordbrandt <caroline_nordbradt@hotmail.com>
@coderabbitai
Copy link

coderabbitai bot commented Feb 22, 2026

📝 Walkthrough

Walkthrough

This PR introduces a complete TCP web server implementation in Java with HTTP request parsing, static file serving, and containerization support. Changes include new server classes (TcpServer, ConnectionHandler, StaticFileHandler), HTTP handling components (HttpParser, HttpResponseBuilder, HttpParseRequestLine), GitHub Actions CI/CD workflows, Docker multi-stage build, Maven dependencies, unit tests, and static assets.

Changes

Cohort / File(s) Summary
CI/CD Workflows
.github/workflows/ci.yml, .github/workflows/release.yml
Added GitHub Actions workflows: Java CI with Maven on push/PR to main branch (compile and test), and Docker image publishing to GHCR on release with multi-arch support (amd64, arm64).
Build & Containerization
Dockerfile, pom.xml
Introduced multi-stage Docker build (Maven build stage → JRE runtime stage with non-root user). Updated Maven: Java 25 compiler, added bucket4j, Jackson, mockito, awaitility dependencies, and Pitest + Spotless plugins.
Core Server Implementation
src/main/java/org/example/App.java, src/main/java/org/example/TcpServer.java, src/main/java/org/example/ConnectionHandler.java, src/main/java/org/example/StaticFileHandler.java
Replaced console output with TCP server startup on port 8080. TcpServer accepts connections and delegates to virtual-thread-based handlers. ConnectionHandler parses HTTP requests, routes to /health endpoint or file handler. StaticFileHandler serves static files with automatic index.html resolution and .html extension handling.
HTTP Parsing & Response
src/main/java/org/example/httpparser/HttpParseRequestLine.java, src/main/java/org/example/httpparser/HttpParser.java, src/main/java/org/example/http/HttpResponseBuilder.java
New classes for HTTP request line parsing (method, URI, version extraction), header parsing with duplicate key merging, and HTTP response construction with dynamic Content-Length calculation and configurable status/headers.
Unit Tests
src/test/java/org/example/http/HttpResponseBuilderTest.java, src/test/java/org/example/httpparser/HttpParseRequestLineTest.java, src/test/java/org/example/httpparser/HttpParserTest.java
Test coverage for HTTP response building (status lines, UTF-8 content length), request line parsing (valid requests, error cases), and header parsing (multi-header extraction, malformed line handling).
Static Assets
www/index.html
Added basic HTML landing page served by StaticFileHandler.

Sequence Diagram

sequenceDiagram
    participant Client
    participant TcpServer
    participant ConnectionHandler
    participant HttpParser
    participant StaticFileHandler
    participant HttpResponseBuilder

    Client->>TcpServer: TCP Connect (port 8080)
    TcpServer->>ConnectionHandler: new ConnectionHandler(socket)
    ConnectionHandler->>HttpParser: setReader(inputStream)
    ConnectionHandler->>HttpParser: parseRequest()
    HttpParser->>HttpParser: parse request line
    ConnectionHandler->>HttpParser: parseHttp()
    HttpParser->>HttpParser: parse headers
    
    alt URI == "/health"
        ConnectionHandler->>HttpResponseBuilder: new HttpResponseBuilder()
        HttpResponseBuilder->>HttpResponseBuilder: setStatusCode(200)<br/>setBody("OK")
        HttpResponseBuilder->>Client: write HTTP 200 response
    else URI for static file
        ConnectionHandler->>StaticFileHandler: sendGetRequest(outputStream, uri)
        StaticFileHandler->>StaticFileHandler: handleGetRequest(uri)<br/>read file bytes
        StaticFileHandler->>HttpResponseBuilder: new HttpResponseBuilder()
        HttpResponseBuilder->>HttpResponseBuilder: setBody(fileBytes)<br/>build response
        HttpResponseBuilder->>Client: write HTTP response
    end

    Client->>TcpServer: TCP Close
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A server hops to life, with ports so bright,
Parsing requests in Java's might,
From sockets to responses, all in flight,
Threading through the night!
Virtual threads and static files abound,
Web magic where we're bound!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.13% 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 'Implement /health Endpoint' accurately reflects the main addition in the pull request—a health check endpoint that responds with a 200 OK JSON payload when accessed at '/health', as evidenced in the ConnectionHandler.java summary.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 14

🧹 Nitpick comments (13)
src/test/java/org/example/http/HttpResponseBuilderTest.java (1)

7-44: Missing coverage for the byte[] body path and build() idempotency.

The two existing tests only exercise setBody(String). The setBody(byte[]) branch in build() is untested. Additionally, there is no test confirming that calling build() twice on the same instance produces identical output (which would catch the mutation bug noted in HttpResponseBuilder).

Consider adding:

  • A test calling setBody(byte[]) and verifying the response body and Content-Length.
  • A test calling build() twice and asserting both results are equal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/http/HttpResponseBuilderTest.java` around lines 7 -
44, Add two unit tests in HttpResponseBuilderTest: one that calls
HttpResponseBuilder.setBody(byte[]) with a known byte array (including a
multi-byte UTF-8 character if desired) and asserts the built response (via
build()) contains the expected Content-Length and body bytes, referencing
HttpResponseBuilder and setBody(byte[]); and another test that calls build()
twice on the same HttpResponseBuilder instance and asserts the two returned
Strings are identical to verify build() is idempotent (reference build() and
HttpResponseBuilder). Ensure the byte[] test verifies Content-Length equals the
actual byte length and that the response contains the raw body content.
src/main/java/org/example/ConnectionHandler.java (2)

10-11: Fields should be private.

client and uri are package-private by default. uri is only used internally via resolveTargetFile; client should not be accessible outside ConnectionHandler.

✏️ Proposed fix
-    Socket client;
-    String uri;
+    private final Socket client;
+    private String 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 10 - 11, Make
the fields in ConnectionHandler private: change the package-private Socket
client and String uri to private fields; update any internal uses (e.g.,
resolveTargetFile method) to access these private fields directly within the
class and ensure no external code relies on package access to client or uri
(refactor callers if needed).

17-42: No unit test covers the /health endpoint — the primary feature of this PR.

ConnectionHandler.runConnectionHandler() is the integration point for /health, but no test exercises it. Consider adding a test that feeds a "GET /health HTTP/1.1\r\n\r\n" stream via a mock or real socket pair and asserts the JSON response body and 200 OK status.

Would you like me to draft a ConnectionHandlerTest covering the /health endpoint and the static file fallback path?

🤖 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 17 - 42, Add
unit tests exercising ConnectionHandler.runConnectionHandler to cover the new
/health branch: create a test (e.g., ConnectionHandlerTest) that constructs a
ConnectionHandler with a mock or socket pair input stream containing "GET
/health HTTP/1.1\r\n\r\n", invokes runConnectionHandler, and asserts the output
stream contains a "HTTP/1.1 200 OK" status line and the JSON body
{"status":"ok"}; also add a second test that sends a GET for a non-/health path
and verifies resolveTargetFile is used and StaticFileHandler.sendGetRequest is
invoked with the expected uri (use mocks or stubs for StaticFileHandler and
override resolveTargetFile if needed) so both the /health shortcut and the
static-file fallback are covered.
src/test/java/org/example/httpparser/HttpParserTest.java (2)

32-32: Specify charset explicitly on empty-input test.

"".getBytes() is inconsistent with the explicit StandardCharsets.UTF_8 used on lines 18 and 43.

✏️ Proposed fix
-        InputStream in = new ByteArrayInputStream("".getBytes());
+        InputStream in = new ByteArrayInputStream("".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/httpparser/HttpParserTest.java` at line 32, The
empty-input test uses new ByteArrayInputStream("".getBytes()) without a charset
while other tests use StandardCharsets.UTF_8; update the creation in
HttpParserTest so the InputStream is built with an explicit charset (use
StandardCharsets.UTF_8) when calling getBytes on the empty string (modify the
line creating the ByteArrayInputStream/InputStream variable) to ensure
consistent encoding across tests.

16-16: Test method name violates camelCase convention.

TestHttpParserForHeaders uses PascalCase while all other test methods in this class use test... camelCase. Rename to testHttpParserForHeaders.

✏️ Proposed fix
-    void TestHttpParserForHeaders() throws IOException {
+    void testHttpParserForHeaders() throws IOException {
🤖 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 16, Rename
the test method TestHttpParserForHeaders to follow camelCase by changing its
name to testHttpParserForHeaders in the HttpParserTest class; update the method
declaration and any internal or external references (calls, listeners, or
annotations) that refer to TestHttpParserForHeaders so they point to
testHttpParserForHeaders to keep naming consistent with other tests.
src/test/java/org/example/httpparser/HttpParseRequestLineTest.java (1)

13-13: Misleading variable name — should reflect its actual type.

httpParseRequestLine is typed as HttpParser; the name implies it's a HttpParseRequestLine. Rename to httpParser or parser for clarity.

✏️ Proposed fix
-    private HttpParser httpParseRequestLine;
+    private HttpParser httpParser;

     `@BeforeEach`
     void setUp() {
-        httpParseRequestLine = new HttpParser();
+        httpParser = new HttpParser();
     }

(and update all references in the test methods)

🤖 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` at line
13, Rename the misleading test field httpParseRequestLine (declared as
HttpParser) to a clearer name such as httpParser (or parser) in the
HttpParseRequestLineTest class and update every reference to that field inside
the test methods; ensure the declaration "private HttpParser
httpParseRequestLine;" is changed to "private HttpParser httpParser;" and all
usages in setup and test methods are updated accordingly to avoid name/type
confusion.
src/main/java/org/example/httpparser/HttpParseRequestLine.java (2)

15-15: Redundant local variable; use the parameter directly.

✏️ Proposed fix
-        BufferedReader reader = br;
-        String requestLine = reader.readLine();
+        String requestLine = br.readLine();
🤖 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` at line 15,
In HttpParseRequestLine replace the redundant local variable declaration
"BufferedReader reader = br" by using the method/constructor parameter "br"
directly: remove the "reader" field/variable and update all references to use
"br" (e.g., inside parse logic or methods in HttpParseRequestLine) so there is
no unnecessary aliasing of the BufferedReader parameter.

11-12: debug flag is dead code — no external way to enable it.

Neither HttpParseRequestLine nor its subclass HttpParser exposes a setter for debug; the logger block is permanently unreachable. Either add a setDebug(boolean) or remove the flag and rely on the existing java.util.logging.Logger.

🤖 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
11 - 12, The private boolean field debug in HttpParseRequestLine is unused and
cannot be toggled externally; either remove the dead field and rely solely on
the existing java.util.logging.Logger (logger) or add a public setter to allow
enabling it from outside (for example add setDebug(boolean) in
HttpParseRequestLine and ensure HttpParser subclasses or callers call it as
needed); update any conditional branches that check the debug field to use
logger.isLoggable(Level.FINE) or the new setter-controlled flag consistently so
behavior is reachable.
src/main/java/org/example/httpparser/HttpParser.java (3)

16-20: setReader silently ignores subsequent calls — surprising contract.

Once a BufferedReader is set, calling setReader again (e.g., when reusing the object in tests) is silently a no-op. Either document this restriction with a comment or throw IllegalStateException to fail fast.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 16 - 20,
The setReader method in HttpParser currently ignores subsequent calls which is
surprising; change its behavior to fail fast by checking the reader field and
throwing an IllegalStateException if it's already set (e.g., in
HttpParser.setReader(InputStream) throw new IllegalStateException("reader
already set") when this.reader != null) so callers/tests that attempt to reset
the reader get an explicit error; update any tests that relied on resetting to
either create a new HttpParser or adjust to the new contract.

40-45: Debug block uses System.out.println instead of the class logger.

HttpParseRequestLine already has a Logger instance. Using System.out.println for debug output in the subclass is inconsistent and not suppressible via standard logging configuration. Use the inherited logger (or declare one here) with Level.FINE.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 40 - 45,
Replace the System.out.println debug block in HttpParser with the class logger
at Level.FINE: use the existing Logger from HttpParseRequestLine (or declare a
private static final Logger if needed) and call logger.isLoggable(Level.FINE)
before logging; then log the Host and each header via logger.log(Level.FINE,
...) (or logger.fine(...) consistently) using headersMap and preserving the
current debug conditional or remove the debug flag in favor of
logger.isLoggable(Level.FINE).

53-55: getHeadersMap() exposes the mutable internal map.

Callers can modify headersMap directly through the returned reference. Return an unmodifiable view.

✏️ Proposed fix
+import java.util.Collections;
 ...
     public Map<String, String> getHeadersMap() {
-        return headersMap;
+        return Collections.unmodifiableMap(headersMap);
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 53 - 55,
getHeadersMap() currently returns the internal mutable headersMap allowing
callers to modify class state; change the method to return an unmodifiable view
(e.g., wrap headersMap with Collections.unmodifiableMap(...) or use
Map.copyOf(...)) so callers cannot mutate the internal headersMap while
preserving read access; update the getHeadersMap method implementation and
import Collections if needed.
src/main/java/org/example/TcpServer.java (2)

18-18: Plain ServerSocket transmits data in cleartext.

The static analysis flag is valid for production deployments: a plain ServerSocket exposes all traffic to network interception. For a learning project this may be acceptable, but consider documenting the limitation and, before production use, replace with SSLServerSocketFactory.

🤖 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` at line 18, The TcpServer currently
constructs a plain ServerSocket (try (ServerSocket serverSocket = new
ServerSocket(port))) which sends cleartext; replace or make optional use of an
SSLServerSocket from SSLServerSocketFactory (or inject a ServerSocketFactory) to
enable TLS for production—update TcpServer to create an SSLServerSocket via
SSLServerSocketFactory.getDefault() (or accept an SSLContext/ServerSocketFactory
parameter), load server certs/truststore where appropriate, and document the
current cleartext behavior as a limitation until TLS is enabled.

16-21: Replace System.out.println with a proper logger; be mindful of PII.

The class uses System.out.println for server startup and per-connection logging. Line 21 logs the remote socket address (IP + port) which is PII. Switch to java.util.logging.Logger (already used in HttpParseRequestLine) and guard or omit the IP-logging line.

🤖 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 16 - 21, Replace the
System.out.println calls in TcpServer with a java.util.logging.Logger instance
(same style as used in HttpParseRequestLine); create a private static final
Logger LOGGER = Logger.getLogger(TcpServer.class.getName()) and use LOGGER.info
for the startup message instead of System.out.println, and for the
per-connection log avoid logging PII by removing or guarding the remote address:
either log only a non-PII event like "Client connected" with an anonymized
identifier or conditionally log clientSocket.getRemoteSocketAddress() behind a
configuration flag (e.g., logRemoteAddresses) so the IP:port is never logged by
default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 31-32: The CI step named "Test with Maven" currently runs "mvn -B
test --file pom.xml", which stops before the verify phase so plugins bound to
verify (e.g., spotless:check and maven-failsafe-plugin) never run; update the
command used in that step to run the verify phase (e.g., use "mvn -B verify
--file pom.xml") or at minimum append the spotless check ("mvn -B test
spotless:check --file pom.xml") so that spotless:check and integration tests
executed by maven-failsafe-plugin are enforced in CI.

In `@Dockerfile`:
- Around line 10-11: Move the USER instruction so it appears before the
ENTRYPOINT for clarity and convention: place USER appuser above ENTRYPOINT
["java", "-classpath", "/app", "org.example.App"] (ensure any file permissions
or ownership for /app and runtime artifacts are already set for appuser before
switching users). Update the Dockerfile so USER precedes ENTRYPOINT and verify
org.example.App runs correctly as appuser.
- Around line 3-5: The Dockerfile fails to include static assets and
dependencies: ensure the build stage packages the app with dependencies and
copies the www static directory into the final image; replace mvn compile with a
full package (e.g., mvn -DskipTests package) so target/*.jar contains all
compiled classes and dependency jars (or use the maven-assembly/shade plugin),
COPY the produced jar into the runtime image, and add COPY www/ www/ (or COPY
--from=build /build/target/www/ www/) so StaticFileHandler can find "www"; also
adjust the runtime classpath to run the packaged jar (java -jar
/app/app-with-deps.jar) instead of relying on /app classpath.

In `@pom.xml`:
- Around line 175-184: The Spotless execution is bound to the verify phase
(spotless:check goal inside the <execution> with <phase>verify</phase>) but CI
invokes mvn -B test so formatting isn't enforced; either change the <phase> in
the pom.xml execution from verify to test so spotless:check runs during mvn
test, or leave the pom as-is and update the CI invocation to run mvn -B verify
instead; locate the <execution> block containing the check goal and modify the
<phase> value or update the CI command accordingly.

In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 24-38: Remove the developer scaffolding and instructional Swedish
comments around the health-check handling in ConnectionHandler; keep the
health-check logic intact (the block that sets requestedUri = parser.getUri(),
checks requestedUri.equals("/health"), builds responseBody/header, writes to
client.getOutputStream(), flushes, and returns) but delete the comment lines
starting with "// --- DIN ÄNDRING..." and the inline "// Avslutar här..." so
only production-relevant comments (if any) remain.
- Around line 44-51: ConnectionHandler.resolveTargetFile currently appends
“.html” but does not strip or validate ../ sequences, and
StaticFileHandler.sendGetRequest constructs new File(WEB_ROOT, uri) without
normalization, allowing path traversal; fix by normalizing and validating the
resolved path inside StaticFileHandler.sendGetRequest (use
Paths.get(WEB_ROOT).resolve(uri).normalize()), verify the normalized path
startsWith(Paths.get(WEB_ROOT)) and reject with an IOException or access-denied
response if it does not, and also ensure ConnectionHandler.resolveTargetFile
normalizes or rejects suspicious URIs (e.g., remove leading slash and disallow
any .. segments) before returning the final uri so both resolveTargetFile and
sendGetRequest cannot be tricked by traversal sequences.

In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Line 49: The code in HttpResponseBuilder uses
REASON_PHRASES.getOrDefault(statusCode, "OK") which can produce incorrect reason
phrases (e.g., "503 OK"); change the default fallback to a neutral value like
"Unknown" and update the REASON_PHRASES map to include missing common codes
(301, 302, 403, 429, 503) or replace the map lookup with a more complete lookup
routine; locate the REASON_PHRASES map and the line that sets the local variable
reason in HttpResponseBuilder and adjust the getOrDefault fallback and/or enrich
the map entries accordingly.
- Around line 38-60: The build() method currently mutates object state by
calling setBody(...) when bytebody is present; instead, compute local variables
(e.g., byte[] effectiveBytes and String effectiveBody) without altering
this.body or this.bytebody: if bytebody != null use effectiveBytes = bytebody
and effectiveBody = new String(bytebody, StandardCharsets.UTF_8), otherwise use
effectiveBody = this.body and effectiveBytes =
effectiveBody.getBytes(StandardCharsets.UTF_8); compute contentLength from
effectiveBytes.length, write headers and effectiveBody into the StringBuilder,
and never call setBody or modify fields so build() is idempotent (refer to
build(), setBody, body, bytebody, and contentLength in the diff).

In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 28: The response currently hardcodes Content-Type to "text/html;
charset=utf-8" in StaticFileHandler (where response.setHeaders(Map.of(...)) is
called); change this to compute the MIME type from the requested URI/file before
calling response.setHeaders: use java.nio.file.Files.probeContentType(path) (or
a small extension->mime lookup map) to determine the contentType, append a
charset (e.g., "; charset=utf-8") only for text/* types, and fall back to
"application/octet-stream" (or "text/plain; charset=utf-8") if probeContentType
returns null; then set response.setHeaders(Map.of("Content-Type",
computedContentType)).
- Around line 18-22: The handleGetRequest method currently constructs new
File(WEB_ROOT, uri) directly, allowing path traversal; fix by URL-decoding the
incoming uri, resolving the candidate File's canonical path and comparing it
against the canonical path of WEB_ROOT to ensure the file is inside the web root
(reject request if it doesn't startWith the WEB_ROOT canonical path), and only
then read bytes into fileBytes after verifying file.isFile() and file.canRead();
update handleGetRequest to return an error/404/403 instead of reading files
outside the web root.
- Around line 30-31: The code uses PrintWriter (new PrintWriter(outputStream,
true)) and println(response.build()), which uses platform default encoding and
appends a newline; replace this by encoding the full response String to UTF-8
bytes (e.g., response.build().getBytes(StandardCharsets.UTF_8)), write those
bytes directly to outputStream and flush so no extra newline is added, and
ensure the Content-Length calculation (wherever set) uses the UTF-8 byte length
to match what you write; update references in StaticFileHandler where
PrintWriter and println are used.
- Line 14: The instance field fileBytes in StaticFileHandler causes a race when
handleGetRequest/sendGetRequest are called concurrently; remove the fileBytes
field and instead allocate a local byte[] inside sendGetRequest (and return or
use it directly within that call path), update handleGetRequest/sendGetRequest
signatures/usage as needed so each request works with its own local byte[]
(referencing fileBytes, StaticFileHandler, handleGetRequest, and sendGetRequest
to locate the change), and ensure no other methods rely on the removed instance
field.

In `@src/main/java/org/example/TcpServer.java`:
- Around line 22-34: The virtual-thread lambda started with
Thread.ofVirtual().start(() -> handleClient(clientSocket)) currently lets
exceptions go to the default uncaught-exception handler; change thread creation
to set an explicit uncaught-exception handler (via
Thread.ofVirtual().unstarted()/builder or
Thread.currentThread().setUncaughtExceptionHandler on the created thread) that
logs the error and any context so failures are observable, and update the catch
in handleClient(Socket) to throw new RuntimeException("Error handling client
connection", e) (pass the caught exception as the cause rather than
concatenating it) so the original stack trace from
ConnectionHandler.runConnectionHandler() is preserved.

In `@www/index.html`:
- Around line 3-6: The closing head tag has extra indentation compared to the
opening <head> tag; locate the <head> and </head> tags in the HTML and remove
the four leading spaces before </head> so the closing tag is aligned with the
opening <head> tag (ensure both tags start at the same column).

---

Nitpick comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 10-11: Make the fields in ConnectionHandler private: change the
package-private Socket client and String uri to private fields; update any
internal uses (e.g., resolveTargetFile method) to access these private fields
directly within the class and ensure no external code relies on package access
to client or uri (refactor callers if needed).
- Around line 17-42: Add unit tests exercising
ConnectionHandler.runConnectionHandler to cover the new /health branch: create a
test (e.g., ConnectionHandlerTest) that constructs a ConnectionHandler with a
mock or socket pair input stream containing "GET /health HTTP/1.1\r\n\r\n",
invokes runConnectionHandler, and asserts the output stream contains a "HTTP/1.1
200 OK" status line and the JSON body {"status":"ok"}; also add a second test
that sends a GET for a non-/health path and verifies resolveTargetFile is used
and StaticFileHandler.sendGetRequest is invoked with the expected uri (use mocks
or stubs for StaticFileHandler and override resolveTargetFile if needed) so both
the /health shortcut and the static-file fallback are covered.

In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 16-20: The setReader method in HttpParser currently ignores
subsequent calls which is surprising; change its behavior to fail fast by
checking the reader field and throwing an IllegalStateException if it's already
set (e.g., in HttpParser.setReader(InputStream) throw new
IllegalStateException("reader already set") when this.reader != null) so
callers/tests that attempt to reset the reader get an explicit error; update any
tests that relied on resetting to either create a new HttpParser or adjust to
the new contract.
- Around line 40-45: Replace the System.out.println debug block in HttpParser
with the class logger at Level.FINE: use the existing Logger from
HttpParseRequestLine (or declare a private static final Logger if needed) and
call logger.isLoggable(Level.FINE) before logging; then log the Host and each
header via logger.log(Level.FINE, ...) (or logger.fine(...) consistently) using
headersMap and preserving the current debug conditional or remove the debug flag
in favor of logger.isLoggable(Level.FINE).
- Around line 53-55: getHeadersMap() currently returns the internal mutable
headersMap allowing callers to modify class state; change the method to return
an unmodifiable view (e.g., wrap headersMap with
Collections.unmodifiableMap(...) or use Map.copyOf(...)) so callers cannot
mutate the internal headersMap while preserving read access; update the
getHeadersMap method implementation and import Collections if needed.

In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java`:
- Line 15: In HttpParseRequestLine replace the redundant local variable
declaration "BufferedReader reader = br" by using the method/constructor
parameter "br" directly: remove the "reader" field/variable and update all
references to use "br" (e.g., inside parse logic or methods in
HttpParseRequestLine) so there is no unnecessary aliasing of the BufferedReader
parameter.
- Around line 11-12: The private boolean field debug in HttpParseRequestLine is
unused and cannot be toggled externally; either remove the dead field and rely
solely on the existing java.util.logging.Logger (logger) or add a public setter
to allow enabling it from outside (for example add setDebug(boolean) in
HttpParseRequestLine and ensure HttpParser subclasses or callers call it as
needed); update any conditional branches that check the debug field to use
logger.isLoggable(Level.FINE) or the new setter-controlled flag consistently so
behavior is reachable.

In `@src/main/java/org/example/TcpServer.java`:
- Line 18: The TcpServer currently constructs a plain ServerSocket (try
(ServerSocket serverSocket = new ServerSocket(port))) which sends cleartext;
replace or make optional use of an SSLServerSocket from SSLServerSocketFactory
(or inject a ServerSocketFactory) to enable TLS for production—update TcpServer
to create an SSLServerSocket via SSLServerSocketFactory.getDefault() (or accept
an SSLContext/ServerSocketFactory parameter), load server certs/truststore where
appropriate, and document the current cleartext behavior as a limitation until
TLS is enabled.
- Around line 16-21: Replace the System.out.println calls in TcpServer with a
java.util.logging.Logger instance (same style as used in HttpParseRequestLine);
create a private static final Logger LOGGER =
Logger.getLogger(TcpServer.class.getName()) and use LOGGER.info for the startup
message instead of System.out.println, and for the per-connection log avoid
logging PII by removing or guarding the remote address: either log only a
non-PII event like "Client connected" with an anonymized identifier or
conditionally log clientSocket.getRemoteSocketAddress() behind a configuration
flag (e.g., logRemoteAddresses) so the IP:port is never logged by default.

In `@src/test/java/org/example/http/HttpResponseBuilderTest.java`:
- Around line 7-44: Add two unit tests in HttpResponseBuilderTest: one that
calls HttpResponseBuilder.setBody(byte[]) with a known byte array (including a
multi-byte UTF-8 character if desired) and asserts the built response (via
build()) contains the expected Content-Length and body bytes, referencing
HttpResponseBuilder and setBody(byte[]); and another test that calls build()
twice on the same HttpResponseBuilder instance and asserts the two returned
Strings are identical to verify build() is idempotent (reference build() and
HttpResponseBuilder). Ensure the byte[] test verifies Content-Length equals the
actual byte length and that the response contains the raw body content.

In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java`:
- Line 13: Rename the misleading test field httpParseRequestLine (declared as
HttpParser) to a clearer name such as httpParser (or parser) in the
HttpParseRequestLineTest class and update every reference to that field inside
the test methods; ensure the declaration "private HttpParser
httpParseRequestLine;" is changed to "private HttpParser httpParser;" and all
usages in setup and test methods are updated accordingly to avoid name/type
confusion.

In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Line 32: The empty-input test uses new ByteArrayInputStream("".getBytes())
without a charset while other tests use StandardCharsets.UTF_8; update the
creation in HttpParserTest so the InputStream is built with an explicit charset
(use StandardCharsets.UTF_8) when calling getBytes on the empty string (modify
the line creating the ByteArrayInputStream/InputStream variable) to ensure
consistent encoding across tests.
- Line 16: Rename the test method TestHttpParserForHeaders to follow camelCase
by changing its name to testHttpParserForHeaders in the HttpParserTest class;
update the method declaration and any internal or external references (calls,
listeners, or annotations) that refer to TestHttpParserForHeaders so they point
to testHttpParserForHeaders to keep naming consistent with other tests.

Comment on lines +31 to +32
- name: Test with Maven
run: mvn -B test --file pom.xml No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

mvn -B test stops before the verify phase — Spotless and Failsafe never execute in CI.

The spotless:check goal is bound to verify in pom.xml (line 177) and integration tests use maven-failsafe-plugin which also runs in verify. Neither will fire with mvn test. Consider updating to mvn -B verify (or at a minimum mvn -B test spotless:check) to enforce formatting and run integration tests in CI.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 31 - 32, The CI step named "Test with
Maven" currently runs "mvn -B test --file pom.xml", which stops before the
verify phase so plugins bound to verify (e.g., spotless:check and
maven-failsafe-plugin) never run; update the command used in that step to run
the verify phase (e.g., use "mvn -B verify --file pom.xml") or at minimum append
the spotless check ("mvn -B test spotless:check --file pom.xml") so that
spotless:check and integration tests executed by maven-failsafe-plugin are
enforced in CI.

Comment on lines +3 to +5
COPY src/ src/
COPY pom.xml pom.xml
RUN mvn compile
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

www/ static assets are never copied into the image — StaticFileHandler will always fail.

The build stage only copies src/ and pom.xml, and the runtime stage only copies compiled classes. StaticFileHandler resolves files under the relative path "www" at runtime, but that directory is not present in the image, so every static-file request will throw an IOException.

mvn compile also only compiles sources; dependency JARs remain in ~/.m2 and are not staged under /build/target/. At runtime -classpath /app contains only your compiled classes — any reference to bucket4j or Jackson classes will cause a NoClassDefFoundError at startup.

🔧 Proposed fix
 FROM maven:3-eclipse-temurin-25-alpine AS build
 WORKDIR /build
 COPY src/ src/
+COPY www/ www/
 COPY pom.xml pom.xml
-RUN mvn compile
+RUN mvn package -DskipTests
+RUN mvn dependency:copy-dependencies -DoutputDirectory=target/dependency

 FROM eclipse-temurin:25-jre-alpine
+WORKDIR /app
 RUN addgroup -S appgroup && adduser -S appuser -G appgroup
 COPY --from=build /build/target/classes/ /app/
+COPY --from=build /build/target/dependency/ /app/libs/
+COPY --from=build /build/www/ /app/www/
-ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
+ENTRYPOINT ["java", "-classpath", "/app:/app/libs/*", "org.example.App"]
 USER appuser
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 3 - 5, The Dockerfile fails to include static assets
and dependencies: ensure the build stage packages the app with dependencies and
copies the www static directory into the final image; replace mvn compile with a
full package (e.g., mvn -DskipTests package) so target/*.jar contains all
compiled classes and dependency jars (or use the maven-assembly/shade plugin),
COPY the produced jar into the runtime image, and add COPY www/ www/ (or COPY
--from=build /build/target/www/ www/) so StaticFileHandler can find "www"; also
adjust the runtime classpath to run the packaged jar (java -jar
/app/app-with-deps.jar) instead of relying on /app classpath.

Comment on lines +10 to +11
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
USER appuser
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

USER placed after ENTRYPOINT — non-idiomatic order.

While Docker applies the final USER instruction as the container's default user regardless of where it appears relative to ENTRYPOINT/CMD, convention is to place USER before ENTRYPOINT for clarity.

🔧 Proposed fix
 COPY --from=build /build/target/classes/ /app/
+USER appuser
 ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
-USER appuser
📝 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
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
USER appuser
USER appuser
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 10 - 11, Move the USER instruction so it appears
before the ENTRYPOINT for clarity and convention: place USER appuser above
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"] (ensure any file
permissions or ownership for /app and runtime artifacts are already set for
appuser before switching users). Update the Dockerfile so USER precedes
ENTRYPOINT and verify org.example.App runs correctly as appuser.

Comment on lines +175 to +184
<executions>
<execution>
<phase>verify</phase>
<goals>
<goal>
check
</goal>
</goals>
</execution>
</executions>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

spotless:check is bound to verify but CI only runs mvn test — style is never enforced.

The Spotless execution is bound to the verify phase (Line 177), but .github/workflows/ci.yml runs mvn -B test which stops before verify. This means Spotless will silently pass on every CI run regardless of formatting violations.

Either bind Spotless to the test phase or update the CI command to mvn -B verify.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pom.xml` around lines 175 - 184, The Spotless execution is bound to the
verify phase (spotless:check goal inside the <execution> with
<phase>verify</phase>) but CI invokes mvn -B test so formatting isn't enforced;
either change the <phase> in the pom.xml execution from verify to test so
spotless:check runs during mvn test, or leave the pom as-is and update the CI
invocation to run mvn -B verify instead; locate the <execution> block containing
the check goal and modify the <phase> value or update the CI command
accordingly.

Comment on lines +24 to +38
// --- DIN ÄNDRING FÖR ISSUE #75 BÖRJAR HÄR ---
String requestedUri = parser.getUri();
if (requestedUri.equals("/health")) {
String responseBody = "{\"status\": \"ok\"}";
String header = "HTTP/1.1 200 OK\r\n" +
"Content-Type: application/json\r\n" +
"Content-Length: " + responseBody.length() + "\r\n" +
"\r\n";

client.getOutputStream().write(header.getBytes());
client.getOutputStream().write(responseBody.getBytes());
client.getOutputStream().flush();
return; // Avslutar här så vi inte letar efter filer i onödan
}
// --- DIN ÄNDRING SLUTAR HÄR ---
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove developer scaffolding comments before merge.

The Swedish // --- DIN ÄNDRING ... --- and // Avslutar här ... comments are instructional notes, not production documentation, and should be removed.

🤖 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 24 - 38,
Remove the developer scaffolding and instructional Swedish comments around the
health-check handling in ConnectionHandler; keep the health-check logic intact
(the block that sets requestedUri = parser.getUri(), checks
requestedUri.equals("/health"), builds responseBody/header, writes to
client.getOutputStream(), flushes, and returns) but delete the comment lines
starting with "// --- DIN ÄNDRING..." and the inline "// Avslutar här..." so
only production-relevant comments (if any) remain.

Comment on lines +18 to +22
private void handleGetRequest(String uri) throws IOException {

File file = new File(WEB_ROOT, uri);
fileBytes = Files.readAllBytes(file.toPath());

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Path traversal vulnerability — user-controlled URI is not sanitised before file access.

new File(WEB_ROOT, uri) concatenates the user-supplied URI directly onto the web root. A URI like ../../etc/passwd or %2F..%2F..%2Fetc%2Fpasswd resolves to a path outside www/, giving the caller arbitrary read access to the filesystem.

Always compare the canonical path of the resolved file against the canonical path of WEB_ROOT:

🔒 Proposed fix
 private void handleGetRequest(String uri) throws IOException {
-    File file = new File(WEB_ROOT, uri);
+    File root = new File(WEB_ROOT).getCanonicalFile();
+    File file = new File(root, uri).getCanonicalFile();
+    if (!file.getPath().startsWith(root.getPath() + File.separator)
+            && !file.getPath().equals(root.getPath())) {
+        throw new IOException("Forbidden path: " + uri);
+    }
     fileBytes = Files.readAllBytes(file.toPath());
 }
📝 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 handleGetRequest(String uri) throws IOException {
File file = new File(WEB_ROOT, uri);
fileBytes = Files.readAllBytes(file.toPath());
private void handleGetRequest(String uri) throws IOException {
File root = new File(WEB_ROOT).getCanonicalFile();
File file = new File(root, uri).getCanonicalFile();
if (!file.getPath().startsWith(root.getPath() + File.separator)
&& !file.getPath().equals(root.getPath())) {
throw new IOException("Forbidden path: " + uri);
}
fileBytes = Files.readAllBytes(file.toPath());
🤖 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 18 - 22, The
handleGetRequest method currently constructs new File(WEB_ROOT, uri) directly,
allowing path traversal; fix by URL-decoding the incoming uri, resolving the
candidate File's canonical path and comparing it against the canonical path of
WEB_ROOT to ensure the file is inside the web root (reject request if it doesn't
startWith the WEB_ROOT canonical path), and only then read bytes into fileBytes
after verifying file.isFile() and file.canRead(); update handleGetRequest to
return an error/404/403 instead of reading files outside the web root.

public void sendGetRequest(OutputStream outputStream, String uri) throws IOException{
handleGetRequest(uri);
HttpResponseBuilder response = new HttpResponseBuilder();
response.setHeaders(Map.of("Content-Type", "text/html; charset=utf-8"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Content-Type is hardcoded to text/html regardless of file extension.

Every file — CSS, JavaScript, JSON, images — will be served as text/html; charset=utf-8. Browsers will misinterpret non-HTML resources, breaking stylesheets, scripts, and images. Derive the MIME type from the URI extension (e.g., via Files.probeContentType() or a manual lookup map) before setting the Content-Type header.

🤖 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` at line 28, The response
currently hardcodes Content-Type to "text/html; charset=utf-8" in
StaticFileHandler (where response.setHeaders(Map.of(...)) is called); change
this to compute the MIME type from the requested URI/file before calling
response.setHeaders: use java.nio.file.Files.probeContentType(path) (or a small
extension->mime lookup map) to determine the contentType, append a charset
(e.g., "; charset=utf-8") only for text/* types, and fall back to
"application/octet-stream" (or "text/plain; charset=utf-8") if probeContentType
returns null; then set response.setHeaders(Map.of("Content-Type",
computedContentType)).

Comment on lines +30 to +31
PrintWriter writer = new PrintWriter(outputStream, true);
writer.println(response.build());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

PrintWriter uses the platform default encoding and println() appends a spurious newline after the response.

Two distinct bugs here:

  1. new PrintWriter(outputStream, true) encodes characters using the JVM's platform default charset (not guaranteed to be UTF-8). If the body or headers contain non-ASCII characters, the bytes written to the socket will not match the Content-Length computed in UTF-8.
  2. writer.println(response.build()) appends a platform line-separator (\n on Linux) after the complete HTTP response, including after the already-written body. This adds stray bytes beyond the declared Content-Length, which can confuse strict HTTP clients.

Write the response directly as UTF-8 bytes instead:

🐛 Proposed fix
-        PrintWriter writer = new PrintWriter(outputStream, true);
-        writer.println(response.build());
+        outputStream.write(response.build().getBytes(java.nio.charset.StandardCharsets.UTF_8));
+        outputStream.flush();
📝 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
PrintWriter writer = new PrintWriter(outputStream, true);
writer.println(response.build());
outputStream.write(response.build().getBytes(java.nio.charset.StandardCharsets.UTF_8));
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 30 - 31, The
code uses PrintWriter (new PrintWriter(outputStream, true)) and
println(response.build()), which uses platform default encoding and appends a
newline; replace this by encoding the full response String to UTF-8 bytes (e.g.,
response.build().getBytes(StandardCharsets.UTF_8)), write those bytes directly
to outputStream and flush so no extra newline is added, and ensure the
Content-Length calculation (wherever set) uses the UTF-8 byte length to match
what you write; update references in StaticFileHandler where PrintWriter and
println are used.

Comment on lines +22 to +34
Thread.ofVirtual().start(() -> handleClient(clientSocket));
}
} catch (IOException e) {
throw new RuntimeException("Failed to start TCP server", 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);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Two related exception-handling problems.

  1. Swallowed virtual-thread exceptions (Line 22): A RuntimeException thrown inside the lambda propagates to the virtual thread's uncaught-exception handler, which by default merely prints to stderr. The server continues accepting connections with no observable failure signal. At minimum, set an explicit uncaught-exception handler on the virtual thread.

  2. Lost cause (Line 33): "Error handling client connection " + e converts the exception to its toString() representation and discards the original stack trace. Pass it as the cause argument instead.

🔧 Proposed fix
-                Thread.ofVirtual().start(() -> handleClient(clientSocket));
+                Thread.ofVirtual()
+                        .uncaughtExceptionHandler((t, ex) ->
+                                logger.log(Level.SEVERE, "Unhandled error in client thread", ex))
+                        .start(() -> handleClient(clientSocket));
-            throw new RuntimeException("Error handling client connection " + 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 22 - 34, The
virtual-thread lambda started with Thread.ofVirtual().start(() ->
handleClient(clientSocket)) currently lets exceptions go to the default
uncaught-exception handler; change thread creation to set an explicit
uncaught-exception handler (via Thread.ofVirtual().unstarted()/builder or
Thread.currentThread().setUncaughtExceptionHandler on the created thread) that
logs the error and any context so failures are observable, and update the catch
in handleClient(Socket) to throw new RuntimeException("Error handling client
connection", e) (pass the caught exception as the cause rather than
concatenating it) so the original stack trace from
ConnectionHandler.runConnectionHandler() is preserved.

Comment on lines +3 to +6
<head>
<meta charset="UTF-8">
<title>Welcome</title>
</head>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Mismatched <head> indentation.

</head> (Line 6) has four extra spaces of indentation compared to the opening <head> (Line 3).

🔧 Proposed fix
 <head>
     <meta charset="UTF-8">
     <title>Welcome</title>
-    </head>
+</head>
📝 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
<head>
<meta charset="UTF-8">
<title>Welcome</title>
</head>
<head>
<meta charset="UTF-8">
<title>Welcome</title>
</head>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@www/index.html` around lines 3 - 6, The closing head tag has extra
indentation compared to the opening <head> tag; locate the <head> and </head>
tags in the HTML and remove the four leading spaces before </head> so the
closing tag is aligned with the opening <head> tag (ensure both tags start at
the same column).

@kappsegla kappsegla closed this Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.