Test/architecture test - #121
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ArchUnit tests and a test dependency, and updates the README architecture diagram to insert a Router node between FilterChain and Plugin; introduces a new test class enforcing directional access rules across the HTTP request lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as ServerSocket
participant Conn as ConnectionHandler
participant Pipeline as Pipeline
participant Filter as FilterChain
participant Router as Router
participant Plugin as Plugin
participant Writer as HttpResponseWriter
Client->>Server: TCP request
Server->>Conn: accept -> hand off (virtual thread)
Conn->>Pipeline: forward request
Pipeline->>Filter: apply filters
Filter->>Router: route to handler
Router->>Plugin: invoke plugin(s)
Plugin->>Writer: produce response
Writer->>Client: send response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/test/java/org/juv25d/ArchitectureTest.java (2)
81-81: Unresolved TODO should be tracked and resolved before treating this rule as stable.The comment raises a valid architectural question: if
Serveris only in the allow-list because it currently createsPipeline(but shouldn't), then the rule is temporarily permissive and won't enforce the intended constraint once the code is refactored.Would you like me to open a separate issue to track moving
Pipelinecreation out ofServerand intoConnectionHandlerFactory/ConnectionHandler?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` at line 81, The TODO indicates Server is allowed to create Pipeline in the ArchitectureTest (the .or(simpleName("Server")) exception) which masks an architectural violation; update the test and codebase by either removing Server from the allow-list and fixing code to move Pipeline creation into ConnectionHandlerFactory/ConnectionHandler, or create a tracked issue to refactor Pipeline creation out of Server and add a TODO comment referencing that issue ID; locate the allow-list change in ArchitectureTest (the .or(simpleName("Server")) clause), and the Pipeline creation site in Server, and then either remove the exception and refactor creation into ConnectionHandlerFactory/ConnectionHandler, or open a ticket and replace the inline TODO with the issue reference so the relaxed rule is not treated as stable.
57-63: Consider scoping subject and accessor predicates by fully-qualified package, not just simple name.Rules 1–4 rely on
haveSimpleName/simpleName(...)without any package anchoring. If a class with an identical simple name exists in another sub-package (e.g., a test double, a legacy artifact, or a future refactor), the rule will silently widen its scope.pluginRulealready demonstrates the more robust pattern usingresideInAPackage.♻️ Refactor example for connectionHandlerAccessRule
- ArchRuleDefinition.classes() - .that().haveSimpleName("ConnectionHandler") - .should().onlyBeAccessed().byClassesThat( - simpleName("Server") - .or(simpleName("ConnectionHandler")) - .or(simpleName("DefaultConnectionHandlerFactory")) - .or(simpleName("ConnectionHandlerFactory"))) + ArchRuleDefinition.classes() + .that().resideInAPackage("..connection..") + .and().haveSimpleName("ConnectionHandler") + .should().onlyBeAccessed().byClassesThat( + resideInAPackage("..server..") + .or(resideInAPackage("..connection..")))Apply the same pattern to
pipelineAccessRule,filterChainRule, androuterRule.Also applies to: 75-81, 93-99, 111-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 57 - 63, The rule uses haveSimpleName("ConnectionHandler") and simpleName(...) predicates which match by simple name only; change these to package-scoped predicates (e.g., replace haveSimpleName("ConnectionHandler") with resideInAPackage("..<your.base>.connection..").and().haveSimpleName("ConnectionHandler") and replace simpleName("Server") / simpleName("ConnectionHandler") / simpleName("DefaultConnectionHandlerFactory") / simpleName("ConnectionHandlerFactory") with resideInAPackage("..<your.base>..").and(simpleName("...")) or directly use resideInAPackage("..<fully.qualified.path>") variants so the rule only targets the intended package; apply the same transformation to the other rules referenced (pipelineAccessRule, filterChainRule, routerRule) to avoid accidental matches across packages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 38-43: The pom currently depends on
com.tngtech.archunit:archunit-junit5:1.4.1 which is incompatible with JUnit
Platform 6.x; replace that dependency by either (A) downgrading test framework
to JUnit 5.x (Platform 1.x) across the project, or (B) remove the
archunit-junit5 artifact and add com.tngtech.archunit:archunit:1.4.1, then
convert any `@ArchTest` field-based rules to explicit tests that call
rule.check(importedClasses) from regular `@Test` methods (update test classes that
reference `@ArchTest` and archunit-junit5 integration to use the core API and
rule.check calls).
In `@README.md`:
- Around line 50-51: Add a "Router" entry to the Core Components section
describing its role (e.g., "Router — Determines which plugin or handler should
process an incoming request based on routing rules and context") and update the
existing "Pipeline" description to reflect routing (replace "Executes the active
plugin" with something like "Coordinates execution: receives routed requests
from the Router and executes the active plugin(s) and post-processing steps").
Update the Core Components list so entries include Router and ensure the
Pipeline description mentions interaction with Router and plugin execution.
In `@src/test/java/org/juv25d/ArchitectureTest.java`:
- Around line 56-65: Update the because() messages for the four ArchRule
constants so they list the full set of allowed accessors instead of a single
class: change connectionHandlerAccessRule.because(...) to mention "Server,
ConnectionHandler, DefaultConnectionHandlerFactory, ConnectionHandlerFactory";
update pipelineAccessRule.because(...) to mention "ConnectionHandler,
DefaultConnectionHandlerFactory, ConnectionHandlerFactory, App, Server"; update
filterChainRule.because(...) to mention "Pipeline, FilterChain, FilterChainImpl,
org.juv25d.filter (filter package), ConnectionHandler"; and update
routerRule.because(...) to mention "FilterChain, FilterChainImpl, Router,
Pipeline, App" so violation messages accurately reflect the authorized classes.
---
Nitpick comments:
In `@src/test/java/org/juv25d/ArchitectureTest.java`:
- Line 81: The TODO indicates Server is allowed to create Pipeline in the
ArchitectureTest (the .or(simpleName("Server")) exception) which masks an
architectural violation; update the test and codebase by either removing Server
from the allow-list and fixing code to move Pipeline creation into
ConnectionHandlerFactory/ConnectionHandler, or create a tracked issue to
refactor Pipeline creation out of Server and add a TODO comment referencing that
issue ID; locate the allow-list change in ArchitectureTest (the
.or(simpleName("Server")) clause), and the Pipeline creation site in Server, and
then either remove the exception and refactor creation into
ConnectionHandlerFactory/ConnectionHandler, or open a ticket and replace the
inline TODO with the issue reference so the relaxed rule is not treated as
stable.
- Around line 57-63: The rule uses haveSimpleName("ConnectionHandler") and
simpleName(...) predicates which match by simple name only; change these to
package-scoped predicates (e.g., replace haveSimpleName("ConnectionHandler")
with
resideInAPackage("..<your.base>.connection..").and().haveSimpleName("ConnectionHandler")
and replace simpleName("Server") / simpleName("ConnectionHandler") /
simpleName("DefaultConnectionHandlerFactory") /
simpleName("ConnectionHandlerFactory") with
resideInAPackage("..<your.base>..").and(simpleName("...")) or directly use
resideInAPackage("..<fully.qualified.path>") variants so the rule only targets
the intended package; apply the same transformation to the other rules
referenced (pipelineAccessRule, filterChainRule, routerRule) to avoid accidental
matches across packages.
eafalkens
previously approved these changes
Feb 23, 2026
eafalkens
left a comment
eafalkens
left a comment
There was a problem hiding this comment.
Looks good to me! Just implement the suggested changes from coderabbit and it's approved.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/test/java/org/juv25d/ArchitectureTest.java (1)
61-73:because()messages are still misleading across all rules — partially addressed but incomplete.The
because()text has been improved from the prior review but several rules still don't match their actual allowed set. For example,pipelineAccessRulesays "only accessed by ConnectionHandler" (line 90) but permits five classes;filterChainRulesays "only accessed by Pipeline" (line 110) but permits the filter package andConnectionHandler. These messages appear verbatim in ArchUnit violation output, so an inaccurate message sends developers on the wrong trail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 61 - 73, The test messages in the ArchUnit rules are inaccurate and misleading: update the .because(...) texts to precisely reflect the allowed accessors for each rule (e.g., change the message for connectionHandlerAccessRule, pipelineAccessRule, and filterChainRule) so they match the actual predicate sets used; locate the rules named connectionHandlerAccessRule, pipelineAccessRule, and filterChainRule and edit their because(...) strings to list the exact allowed classes/packages (e.g., "only accessed by Server, ConnectionHandler, DefaultConnectionHandlerFactory, ConnectionHandlerFactory" for connectionHandlerAccessRule, and similarly enumerate the five allowed classes for pipelineAccessRule and the allowed package/classes for filterChainRule) so ArchUnit violation output is truthful and actionable.
🧹 Nitpick comments (1)
src/test/java/org/juv25d/ArchitectureTest.java (1)
62-62: Nit: extra space before()in method signatures.All five test methods have an inconsistent space before the parentheses (e.g.,
connectionHandlerAccessRule ()). Standard Java convention omits it.♻️ Example fix
- void connectionHandlerAccessRule () { + void connectionHandlerAccessRule() {(Same for
pipelineAccessRule,filterChainRule,routerRule,pluginRule.)Also applies to: 80-80, 100-100, 120-120, 140-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` at line 62, Remove the extra space before the parentheses in the five test method signatures in ArchitectureTest: change "connectionHandlerAccessRule ()", "pipelineAccessRule ()", "filterChainRule ()", "routerRule ()", and "pluginRule ()" to follow Java convention without the space (e.g., "connectionHandlerAccessRule()"); update each method declaration accordingly so signatures are consistent.
🤖 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/test/java/org/juv25d/ArchitectureTest.java`:
- Around line 79-92: The pipelineAccessRule test is missing the concrete factory
class: add simpleName("DefaultConnectionHandlerFactory") into the allowed access
chain so the rule permits that factory to access Pipeline; update the
.that().haveSimpleName("Pipeline").should().onlyBeAccessed().byClassesThat(...)
chain (the method pipelineAccessRule) to include
.or(simpleName("DefaultConnectionHandlerFactory")) alongside ConnectionHandler,
ConnectionHandlerFactory, Pipeline, App, and Server.
- Around line 46-152: Add a new ArchUnit test method named
httpResponseWriterRule that asserts classes with simpleName "HttpResponseWriter"
should onlyBeAccessed().byClassesThat include simpleName("HttpResponseWriter")
and simpleName("ConnectionHandler"), then .as(...) .because(...) and
.check(importedClasses); place it alongside the other test methods in
ArchitectureTest so the lifecycle rule for HttpResponseWriter is enforced;
adjust allowed classes only if plugins legitimately need direct access.
---
Duplicate comments:
In `@src/test/java/org/juv25d/ArchitectureTest.java`:
- Around line 61-73: The test messages in the ArchUnit rules are inaccurate and
misleading: update the .because(...) texts to precisely reflect the allowed
accessors for each rule (e.g., change the message for
connectionHandlerAccessRule, pipelineAccessRule, and filterChainRule) so they
match the actual predicate sets used; locate the rules named
connectionHandlerAccessRule, pipelineAccessRule, and filterChainRule and edit
their because(...) strings to list the exact allowed classes/packages (e.g.,
"only accessed by Server, ConnectionHandler, DefaultConnectionHandlerFactory,
ConnectionHandlerFactory" for connectionHandlerAccessRule, and similarly
enumerate the five allowed classes for pipelineAccessRule and the allowed
package/classes for filterChainRule) so ArchUnit violation output is truthful
and actionable.
---
Nitpick comments:
In `@src/test/java/org/juv25d/ArchitectureTest.java`:
- Line 62: Remove the extra space before the parentheses in the five test method
signatures in ArchitectureTest: change "connectionHandlerAccessRule ()",
"pipelineAccessRule ()", "filterChainRule ()", "routerRule ()", and "pluginRule
()" to follow Java convention without the space (e.g.,
"connectionHandlerAccessRule()"); update each method declaration accordingly so
signatures are consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pom.xmlsrc/test/java/org/juv25d/ArchitectureTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/test/java/org/juv25d/ArchitectureTest.java (3)
120-133:because()message on line 131 still only mentions "FilterChain" — same mismatch pattern as other rules.Previously flagged. Update to list all authorized accessors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 120 - 133, In routerRule update the .because(...) message to list all allowed accessors (FilterChain, FilterChainImpl, Router, Pipeline, App) so it matches the rule: inside the routerRule test (the ArchRuleDefinition for classes().that().haveSimpleName("Router")), change the because text from "FilterChain" to something like "only FilterChain, FilterChainImpl, Router, Pipeline or App may access Router" so the message aligns with the checked accessors.
100-113:because()message on line 111 still only mentions "Pipeline" — doesn't reflect the full allowed set.This was flagged in a previous review. The
because()text should list all authorized accessors (Pipeline, FilterChain, FilterChainImpl, filter package, ConnectionHandler) so that violation output is actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 100 - 113, Update the explanatory message passed to because() for the FilterChain access rule so it reflects the full set of allowed accessors: Pipeline, FilterChain, FilterChainImpl, any class in the ..filter.. package, and ConnectionHandler; locate the rule built by ArchRuleDefinition.classes() that targets haveSimpleName("FilterChain") and modify the because(...) call (currently "FilterChain should only be accessed by Pipeline") to enumerate all authorized accessors for actionable violation output.
46-153: Still noHttpResponseWriteraccess rule despite it being documented in the lifecycle Javadoc.Previously flagged — the class Javadoc documents
Plugin → HttpResponseWriter → Clientbut no ArchUnit rule enforces this constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 46 - 153, Add a new ArchUnit test method (e.g., httpResponseWriterRule) that enforces the documented lifecycle by restricting access to the HttpResponseWriter class: locate the class symbol "HttpResponseWriter" and use ArchRuleDefinition.classes().that().haveSimpleName("HttpResponseWriter").should().onlyBeAccessed().byClassesThat(...) allowing access from plugins (use resideInAPackage("..plugin..")), the HttpResponseWriter itself, and the "Client" consumer (simpleName("Client")), then attach an .as(...) and .because(...) message and call .check(importedClasses) — mirror the style of existing tests like pluginRule and routerRule.
🤖 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/test/java/org/juv25d/ArchitectureTest.java`:
- Line 89: Fix the typo in the inline TODO comment in ArchitectureTest: change
"Shold" to "Should" in the comment following the .or(simpleName("Server"))
expression so the TODO reads "Should this be handled by connectionHandler
instead to keep the strict flow?". Locate the comment adjacent to
.or(simpleName("Server")) in the ArchitectureTest class and update only the
comment text.
- Around line 79-93: Fix the typo in the comment (change "Shold" to "Should")
and update the ArchUnit rule's because() message in pipelineAccessRule() so it
accurately lists all allowed accessors: ConnectionHandler,
ConnectionHandlerFactory, DefaultConnectionHandlerFactory, Pipeline, App, and
Server; locate the rule building code that uses
simpleName("ConnectionHandler").or(...).or(simpleName("Server")) and replace the
current because("Pipeline should only be accessed by ConnectionHandler") with a
concise message enumerating those six classes to provide correct context in
violations.
- Around line 3-9: ArchitectureTest.java currently imports ArchUnit JUnit5
helpers but ArchUnit 1.4.1 is incompatible with JUnit 6.x; fix by either (A)
downgrading your JUnit platform to 5.x in build config so existing ArchUnit
JUnit5 integration works, or (B) remove the archunit-junit5-engine dependency
and switch the test to use plain ArchUnit core APIs (retain ClassFileImporter,
JavaClasses, ImportOption, ArchRuleDefinition) and standard `@Test` methods for
assertions; update build dependencies accordingly and adjust imports in
ArchitectureTest (remove any archunit-junit5-engine-specific annotations/usages
if chosen option B).
---
Duplicate comments:
In `@src/test/java/org/juv25d/ArchitectureTest.java`:
- Around line 120-133: In routerRule update the .because(...) message to list
all allowed accessors (FilterChain, FilterChainImpl, Router, Pipeline, App) so
it matches the rule: inside the routerRule test (the ArchRuleDefinition for
classes().that().haveSimpleName("Router")), change the because text from
"FilterChain" to something like "only FilterChain, FilterChainImpl, Router,
Pipeline or App may access Router" so the message aligns with the checked
accessors.
- Around line 100-113: Update the explanatory message passed to because() for
the FilterChain access rule so it reflects the full set of allowed accessors:
Pipeline, FilterChain, FilterChainImpl, any class in the ..filter.. package, and
ConnectionHandler; locate the rule built by ArchRuleDefinition.classes() that
targets haveSimpleName("FilterChain") and modify the because(...) call
(currently "FilterChain should only be accessed by Pipeline") to enumerate all
authorized accessors for actionable violation output.
- Around line 46-153: Add a new ArchUnit test method (e.g.,
httpResponseWriterRule) that enforces the documented lifecycle by restricting
access to the HttpResponseWriter class: locate the class symbol
"HttpResponseWriter" and use
ArchRuleDefinition.classes().that().haveSimpleName("HttpResponseWriter").should().onlyBeAccessed().byClassesThat(...)
allowing access from plugins (use resideInAPackage("..plugin..")), the
HttpResponseWriter itself, and the "Client" consumer (simpleName("Client")),
then attach an .as(...) and .because(...) message and call
.check(importedClasses) — mirror the style of existing tests like pluginRule and
routerRule.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/test/java/org/juv25d/ArchitectureTest.java (4)
89-89: Typo in TODO comment: "Shold" → "Should".✏️ Fix
- .or(simpleName("Server"))) //TODO right now server creates pipeline. Shold this be handled by connectionHandler instead to keep the strict flow? + .or(simpleName("Server"))) //TODO right now server creates pipeline. Should this be handled by connectionHandler instead to keep the strict flow?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` at line 89, Fix the typo in the inline TODO comment near the simpleName("Server") clause inside ArchitectureTest: change "Shold this be handled by connectionHandler" to "Should this be handled by connectionHandler" so the TODO reads correctly and remains clear for future maintainers.
3-9: Previous JUnit 6.x compatibility concern is mitigated by using plain ArchUnit core.The past critical finding was about
archunit-junit5-engine-apidepending on JUnit Platform 1.x being incompatible with JUnit 6.x. ArchUnit works with any test framework that executes Java code; by usingcom.tngtech.archunit:archunit(plain core) with standard@BeforeAll/@TestJUnit Jupiter annotations — rather thanarchunit-junit5engine annotations — the Platform-level incompatibility does not apply. No action required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 3 - 9, The review notes that the previous JUnit 6 compatibility concern is resolved by using plain ArchUnit core, so no code changes are required; leave the imports and test class (ArchitectureTest) as-is using com.tngtech.archunit:archunit with JUnit Jupiter annotations (`@BeforeAll/`@Test) and do not add or switch to archunit-junit5-engine artifacts or platform-specific annotations.
111-113:filterChainRuleandrouterRulebecause()messages still don't reflect full allowed accessor sets.
filterChainRule(line 112): message says"Pipeline"only, but rule allowsPipeline,FilterChain,FilterChainImpl,..filter..package, andConnectionHandler.routerRule(line 132): message says"FilterChain"only, but rule allowsFilterChain,FilterChainImpl,Router,Pipeline, andApp.Incomplete messages produce misleading violation output, making failures harder to diagnose.
✏️ Proposed fixes
- .because("FilterChain should only be accessed by Pipeline") + .because("FilterChain should only be accessed by Pipeline, FilterChainImpl, filter package classes, or ConnectionHandler")- .because("Router should only be accessed by FilterChain") + .because("Router should only be accessed by FilterChain, FilterChainImpl, Pipeline, or App during bootstrapping")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 111 - 113, Update the explanatory text passed to the because() calls for filterChainRule and routerRule to list the full set of allowed accessors so violation messages are accurate: for filterChainRule mention Pipeline, FilterChain, FilterChainImpl, the ..filter.. package and ConnectionHandler; for routerRule mention FilterChain, FilterChainImpl, Router, Pipeline and App (use the exact class names used in the rules: Pipeline, FilterChain, FilterChainImpl, Router, App and the package wildcard ..filter..). Locate the two because(...) invocations tied to filterChainRule and routerRule and replace their current short messages with these expanded, comma-separated allowed-accessor lists to reflect the rule logic.
141-154:HttpResponseWriterlifecycle step still has no enforcement rule.The class Javadoc includes
Plugin → HttpResponseWriter → Clientin the lifecycle diagram, but no ArchUnit rule restricts which classes may accessHttpResponseWriter. The lifecycle coverage gap remains unaddressed since the previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 141 - 154, Add a new ArchUnit test method (e.g., httpResponseWriterRule) that targets the HttpResponseWriter class and enforces who may access it: build an ArchRule using ArchRuleDefinition.classes().that().haveSimpleName("HttpResponseWriter") (or simpleName("HttpResponseWriter")) and apply .should().onlyBeAccessed().byClassesThat(resideInAPackage("..plugin..").or(resideInAPackage("..client..")).or(simpleName("App")).or(simpleName("FilterChainImpl"))), give it a descriptive .as(...)/.because(...) message, and call .check(importedClasses) to verify the lifecycle step is now enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/test/java/org/juv25d/ArchitectureTest.java`:
- Line 89: Fix the typo in the inline TODO comment near the simpleName("Server")
clause inside ArchitectureTest: change "Shold this be handled by
connectionHandler" to "Should this be handled by connectionHandler" so the TODO
reads correctly and remains clear for future maintainers.
- Around line 3-9: The review notes that the previous JUnit 6 compatibility
concern is resolved by using plain ArchUnit core, so no code changes are
required; leave the imports and test class (ArchitectureTest) as-is using
com.tngtech.archunit:archunit with JUnit Jupiter annotations (`@BeforeAll/`@Test)
and do not add or switch to archunit-junit5-engine artifacts or
platform-specific annotations.
- Around line 111-113: Update the explanatory text passed to the because() calls
for filterChainRule and routerRule to list the full set of allowed accessors so
violation messages are accurate: for filterChainRule mention Pipeline,
FilterChain, FilterChainImpl, the ..filter.. package and ConnectionHandler; for
routerRule mention FilterChain, FilterChainImpl, Router, Pipeline and App (use
the exact class names used in the rules: Pipeline, FilterChain, FilterChainImpl,
Router, App and the package wildcard ..filter..). Locate the two because(...)
invocations tied to filterChainRule and routerRule and replace their current
short messages with these expanded, comma-separated allowed-accessor lists to
reflect the rule logic.
- Around line 141-154: Add a new ArchUnit test method (e.g.,
httpResponseWriterRule) that targets the HttpResponseWriter class and enforces
who may access it: build an ArchRule using
ArchRuleDefinition.classes().that().haveSimpleName("HttpResponseWriter") (or
simpleName("HttpResponseWriter")) and apply
.should().onlyBeAccessed().byClassesThat(resideInAPackage("..plugin..").or(resideInAPackage("..client..")).or(simpleName("App")).or(simpleName("FilterChainImpl"))),
give it a descriptive .as(...)/.because(...) message, and call
.check(importedClasses) to verify the lifecycle step is now enforced.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/test/java/org/juv25d/ArchitectureTest.java (6)
75-93:⚠️ Potential issue | 🟡 MinorMake the
because()message reflect the full allowed set.The rule currently allows factories and Pipeline itself, but the message omits them.
✏️ Suggested message update
- .because("Pipeline should only be accessed by ConnectionHandler, App, Bootstrap during setup") + .because("Pipeline should only be accessed by ConnectionHandler, ConnectionHandlerFactory, DefaultConnectionHandlerFactory, Pipeline, App, or Bootstrap during setup")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 75 - 93, Update the ArchUnit rule's because() message in the pipelineAccessRule test to list all allowed accessors: ConnectionHandler, ConnectionHandlerFactory, DefaultConnectionHandlerFactory, Pipeline (self), App, and Bootstrap; locate the rule built with classes().that().haveSimpleName("Pipeline").should().onlyBeAccessed().byClassesThat(...) (the pipelineAccessRule) and replace the current because(...) string so it accurately reflects that full set.
116-132:⚠️ Potential issue | 🟡 MinorClarify the
because()message to list all authorized accessors.The rule allows FilterChainImpl, Router, Pipeline, and App in addition to FilterChain.
✏️ Suggested message update
- .because("Router should only be accessed by FilterChain") + .because("Router should only be accessed by FilterChain, FilterChainImpl, Router, Pipeline, or App")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 116 - 132, Update the explanatory message in the routerRule test so the because(...) text lists all allowed accessors (FilterChain, FilterChainImpl, Router, Pipeline, App) instead of only "FilterChain"; locate the routerRule() test and change the .because(...) string on the ArchRule built for classes().that().haveSimpleName("Router") to enumerate those five authorized classes.
96-112:⚠️ Potential issue | 🟡 MinorUpdate the
because()message to match the actual allowed accessors.The rule permits FilterChain, FilterChainImpl, filter package, and ConnectionHandler, but the message only cites Pipeline.
✏️ Suggested message update
- .because("FilterChain should only be accessed by Pipeline") + .because("FilterChain should only be accessed by Pipeline, FilterChain, FilterChainImpl, ConnectionHandler, or classes in ..filter..")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 96 - 112, The test's because() message is inaccurate: update the message in the filterChainRule (the ArchRuleDefinition.classes() rule that targets FilterChain) to reflect all allowed accessors (Pipeline, FilterChain, FilterChainImpl, classes in the ..filter.. package, and ConnectionHandler) instead of only mentioning Pipeline so the description matches the .onlyBeAccessed().byClassesThat(...) configuration.
61-72:⚠️ Potential issue | 🟡 MinorAlign
because()text with the actual allowed accessors.The rule allows multiple classes beyond “server” and factories; the message should enumerate them for actionable violation output.
✏️ Suggested message update
- .because("ConnectionHandler should only be accessed by server, connectionhandler or its factories") + .because("ConnectionHandler should only be accessed by Server, ConnectionHandler, ConnectionHandlerFactory, or DefaultConnectionHandlerFactory")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 61 - 72, Update the because() text for the connectionHandlerAccessRule to precisely list all allowed accessors (Server, ConnectionHandler, DefaultConnectionHandlerFactory, ConnectionHandlerFactory) so violation messages are actionable; locate the rule built with ArchRuleDefinition.classes() that .that().haveSimpleName("ConnectionHandler").should().onlyBeAccessed().byClassesThat(...) (the connectionHandlerAccessRule) and replace the current .because(...) string with one that enumerates those four class names.
3-9:⚠️ Potential issue | 🔴 CriticalResolve ArchUnit 1.4.1 vs JUnit Jupiter 6.0.2 compatibility.
ArchUnit 1.4.1’s JUnit integration targets the JUnit 5 line (Platform 1.x), while JUnit Jupiter 6.0.2 is on Platform 6.x. If the build is truly on JUnit 6, the ArchUnit JUnit integration may not run. Consider aligning to JUnit 5 or switching to ArchUnit core-only tests.
ArchUnit 1.4.1 JUnit Jupiter 6 compatibility🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 3 - 9, The test uses ArchUnit 1.4.1 with JUnit Jupiter 6.x compatibility mismatch; update the test to either run against JUnit 5 or avoid ArchUnit's JUnit integration: either change the project test framework to JUnit 5 (restore junit-jupiter 5.x) or remove any ArchUnit JUnit-specific helpers and keep a core-only test using ClassFileImporter and ArchRuleDefinition inside your ArchitectureTest class and regular `@Test` methods (e.g., keep imports for ClassFileImporter, JavaClasses and ArchRuleDefinition and invoke rule.check(classes) manually in the ArchitectureTest methods) so the tests run under JUnit Jupiter 6 without relying on ArchUnit's JUnit integration.
15-44:⚠️ Potential issue | 🟠 MajorAdd an ArchUnit rule for HttpResponseWriter to fully enforce the documented lifecycle.
The lifecycle Javadoc includes HttpResponseWriter, but there’s no rule constraining its access. This leaves a gap in the architectural enforcement.
➕ Suggested ArchUnit rule
+ /** + * HttpResponseWriter should only be touched by the component responsible for writing responses. + */ + `@Test` + void httpResponseWriterRule () { + ArchRuleDefinition.classes() + .that().haveSimpleName("HttpResponseWriter") + .should().onlyBeAccessed().byClassesThat( + simpleName("HttpResponseWriter") + .or(simpleName("ConnectionHandler"))) + .as("HttpResponseWriter access rule") + .because("HttpResponseWriter should only be accessed by ConnectionHandler") + .check(importedClasses); + }Adjust the allowed set if plugins or other components legitimately need direct access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/ArchitectureTest.java` around lines 15 - 44, Add an ArchUnit rule in ArchitectureTest that explicitly constrains who may access HttpResponseWriter: define a rule (similar to existing lifecycle rules) that forbids classes outside the allowed set from accessing the HttpResponseWriter class (reference the HttpResponseWriter type by name) and register that rule with ArchRule. Update the allowed set (e.g., plugins, router, filter chain) used in the rule to match legitimate direct callers in your design, and place this new rule alongside the other lifecycle rules in ArchitectureTest so the build fails if unauthorized layers access HttpResponseWriter.
🤖 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/test/java/org/juv25d/ArchitectureTest.java`:
- Around line 136-151: Update the assertion message passed to because() in the
pluginRule so it accurately lists all allowed accessors: Router,
FilterChainImpl, other plugins, and App; locate the rule built with
ArchRuleDefinition.classes() in pluginRule (uses resideInAPackage("..plugin.."),
.onlyBeAccessed().byClassesThat(resideInAPackage("..router..").or(resideInAPackage("..plugin..")).or(simpleName("App")).or(simpleName("FilterChainImpl"))))
and replace the current "Plugins should only be managed by the Router or during
startup" text with a concise message reflecting the full allowed set (e.g.,
"Plugins may only be accessed by Router, FilterChainImpl, other plugins, or
App").
---
Duplicate comments:
In `@src/test/java/org/juv25d/ArchitectureTest.java`:
- Around line 75-93: Update the ArchUnit rule's because() message in the
pipelineAccessRule test to list all allowed accessors: ConnectionHandler,
ConnectionHandlerFactory, DefaultConnectionHandlerFactory, Pipeline (self), App,
and Bootstrap; locate the rule built with
classes().that().haveSimpleName("Pipeline").should().onlyBeAccessed().byClassesThat(...)
(the pipelineAccessRule) and replace the current because(...) string so it
accurately reflects that full set.
- Around line 116-132: Update the explanatory message in the routerRule test so
the because(...) text lists all allowed accessors (FilterChain, FilterChainImpl,
Router, Pipeline, App) instead of only "FilterChain"; locate the routerRule()
test and change the .because(...) string on the ArchRule built for
classes().that().haveSimpleName("Router") to enumerate those five authorized
classes.
- Around line 96-112: The test's because() message is inaccurate: update the
message in the filterChainRule (the ArchRuleDefinition.classes() rule that
targets FilterChain) to reflect all allowed accessors (Pipeline, FilterChain,
FilterChainImpl, classes in the ..filter.. package, and ConnectionHandler)
instead of only mentioning Pipeline so the description matches the
.onlyBeAccessed().byClassesThat(...) configuration.
- Around line 61-72: Update the because() text for the
connectionHandlerAccessRule to precisely list all allowed accessors (Server,
ConnectionHandler, DefaultConnectionHandlerFactory, ConnectionHandlerFactory) so
violation messages are actionable; locate the rule built with
ArchRuleDefinition.classes() that
.that().haveSimpleName("ConnectionHandler").should().onlyBeAccessed().byClassesThat(...)
(the connectionHandlerAccessRule) and replace the current .because(...) string
with one that enumerates those four class names.
- Around line 3-9: The test uses ArchUnit 1.4.1 with JUnit Jupiter 6.x
compatibility mismatch; update the test to either run against JUnit 5 or avoid
ArchUnit's JUnit integration: either change the project test framework to JUnit
5 (restore junit-jupiter 5.x) or remove any ArchUnit JUnit-specific helpers and
keep a core-only test using ClassFileImporter and ArchRuleDefinition inside your
ArchitectureTest class and regular `@Test` methods (e.g., keep imports for
ClassFileImporter, JavaClasses and ArchRuleDefinition and invoke
rule.check(classes) manually in the ArchitectureTest methods) so the tests run
under JUnit Jupiter 6 without relying on ArchUnit's JUnit integration.
- Around line 15-44: Add an ArchUnit rule in ArchitectureTest that explicitly
constrains who may access HttpResponseWriter: define a rule (similar to existing
lifecycle rules) that forbids classes outside the allowed set from accessing the
HttpResponseWriter class (reference the HttpResponseWriter type by name) and
register that rule with ArchRule. Update the allowed set (e.g., plugins, router,
filter chain) used in the rule to match legitimate direct callers in your
design, and place this new rule alongside the other lifecycle rules in
ArchitectureTest so the build fails if unauthorized layers access
HttpResponseWriter.
VonAdamo
previously approved these changes
Feb 25, 2026
VonAdamo
left a comment
VonAdamo
left a comment
There was a problem hiding this comment.
Nice work! When the rabbit is happy, so are we :) 🚀
FionaSprinkles
dismissed stale reviews from Tyreviel and VonAdamo
via
February 25, 2026 10:27
2a7620a
kristinaxm
approved these changes
Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces architecture enforcement tests using ArchUnit to protect the intended lifecycle and dependency direction.
Closes #77
Summary by CodeRabbit
Documentation
Tests