Add forbidden name detection, DI violations, and severity icons#7
Merged
ClankerGuru merged 13 commits intomainfrom Apr 13, 2026
Merged
Add forbidden name detection, DI violations, and severity icons#7ClankerGuru merged 13 commits intomainfrom
ClankerGuru merged 13 commits intomainfrom
Conversation
- Add FORBIDDEN severity level with 🚫 icon to AntiPattern.Severity and FindingSeverity - Detect forbidden package names (utils, helpers, managers, common, misc, base, shared) - Detect dependency inversion violations: flag concrete class usage when interface exists - Update all renderers (Dashboard, ProjectReport, IncludedBuild) to use emoji severity icons - Smell classes in forbidden-named packages elevated to FORBIDDEN severity - Tests for all new detection rules
|
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 a FORBIDDEN severity and icons; implements configurable forbidden-package and forbidden-class-name checks plus dependency-inversion detection; propagates new severity through models and renderers; introduces multiple split report renderers and files; wires new DSL settings and task inputs; updates and adds tests. Changes
Sequence DiagramsequenceDiagram
participant Source as SourceFiles
participant Detector as AntiPatternDetector
participant Resolver as TypeResolver
participant Analysis as ProjectAnalysis
participant Task as ContextTask
participant Renderer as ReportRenderers
Source->>Detector: provide classified components & metadata
Detector->>Detector: detectForbiddenNames(forbiddenPackages)
Detector->>Detector: detectForbiddenClassNames(forbiddenClassPatterns)
Detector->>Resolver: resolve imports -> concrete types & interfaces
Resolver->>Detector: return resolved types/interfaces
Detector->>Detector: detectDependencyInversionViolations()
Detector->>Analysis: emit findings (FORBIDDEN/WARNING/INFO) deduped by message
Task->>Analysis: call analyzeProject with forbidden settings (cross-build)
Analysis->>Renderer: supply summaries and cross-build analysis
Renderer->>Renderer: group by severity and render split markdown files
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (5)
src/test/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetectorTest.kt (1)
220-232: Minor: consider restructuring the icon tests.The
whenblock title mentions only FORBIDDEN, but the nestedthenblocks test all three severity icons. Consider renaming for clarity or restructuring.♻️ Suggested restructure
- `when`("severity FORBIDDEN has the correct icon") { - then("FORBIDDEN icon is the no-entry emoji") { + `when`("severity icons are defined correctly") { + then("FORBIDDEN icon is the no-entry emoji") { AntiPattern.Severity.FORBIDDEN.icon shouldBe "\uD83D\uDEAB" } then("WARNING icon is the warning emoji") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetectorTest.kt` around lines 220 - 232, The test block in AntiPatternDetectorTest mixes assertions for multiple severities while its `when` title only mentions FORBIDDEN; split or rename it so each severity is clearly described: either rename the `when` to something like "`severity icons`" or create separate `when`/`then` blocks for AntiPattern.Severity.FORBIDDEN, AntiPattern.Severity.WARNING, and AntiPattern.Severity.INFO, keeping each assertion (accessing the `.icon` property) under the matching `when`/`then` to make intent and failure messages clear.src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt (1)
54-56: Consider extracting the warning count predicate.The same filtering logic
it.severity == FindingSeverity.WARNING || it.severity == FindingSeverity.FORBIDDENis repeated three times. A helper would improve maintainability.♻️ Suggested helper
Add a helper method or extension:
private fun isActionableSeverity(severity: FindingSeverity): Boolean = severity == FindingSeverity.WARNING || severity == FindingSeverity.FORBIDDENThen replace the repeated predicate:
-s.analysis?.findings?.count { - it.severity == FindingSeverity.WARNING || it.severity == FindingSeverity.FORBIDDEN -} ?: 0 +s.analysis?.findings?.count { isActionableSeverity(it.severity) } ?: 0Also applies to: 92-95, 131-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt` around lines 54 - 56, Extract the repeated predicate used to count actionable findings into a single helper (e.g., add a private function isActionableSeverity(severity: FindingSeverity): Boolean) and replace the three inline predicates (the occurrences inside the s.analysis?.findings?.count { ... } expressions) with calls to that helper; update all instances referenced in DashboardRenderer (the counts at the original lines and the other two occurrences mentioned) to call isActionableSeverity(it.severity) so the logic is centralized and easier to maintain.src/main/kotlin/zone/clanker/gradle/srcx/report/IncludedBuildRenderer.kt (1)
160-160: Consider removing the suppression annotation.The
appendProblems()method appears streamlined after the refactoring—the control flow is now a simple if-check, a single loop, and conditional cycle rendering. The suppression may no longer be necessary.♻️ Suggested removal
- `@Suppress`("CyclomaticComplexMethod") private fun StringBuilder.appendProblems() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/IncludedBuildRenderer.kt` at line 160, The `@Suppress`("CyclomaticComplexMethod") annotation above appendProblems() is likely no longer needed; remove that suppression annotation (the one immediately preceding the appendProblems function declaration in IncludedBuildRenderer) and then run the static-analysis/lint and compile to confirm no new warnings or errors; if any remaining complexity warnings reference appendProblems, simplify branching or narrow the suppression to specific cases instead of the whole method.src/main/kotlin/zone/clanker/gradle/srcx/report/ProjectReportRenderer.kt (1)
89-107: Consider consolidating the three rendering loops.The three loops have identical rendering logic. A single sorted loop would reduce duplication and align with the approach in
IncludedBuildRenderer.appendProblems().♻️ Suggested consolidation
- val forbidden = analysis.findings.filter { it.severity == FindingSeverity.FORBIDDEN } - val warnings = analysis.findings.filter { it.severity == FindingSeverity.WARNING } - val infos = analysis.findings.filter { it.severity == FindingSeverity.INFO } - - if (forbidden.isNotEmpty() || warnings.isNotEmpty() || infos.isNotEmpty()) { + if (analysis.findings.isNotEmpty()) { appendLine("## Findings") appendLine() - for (f in forbidden) { - appendLine("- **${f.severity.icon}** ${f.message}") - appendLine(" - ${f.suggestion}") - } - for (f in warnings) { - appendLine("- **${f.severity.icon}** ${f.message}") - appendLine(" - ${f.suggestion}") - } - for (f in infos) { + for (f in analysis.findings.sortedBy { it.severity }) { appendLine("- **${f.severity.icon}** ${f.message}") appendLine(" - ${f.suggestion}") } appendLine() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/ProjectReportRenderer.kt` around lines 89 - 107, The three nearly identical loops that render findings (variables forbidden, warnings, infos derived from analysis.findings filtered by FindingSeverity) should be consolidated into a single iteration: build one list from analysis.findings (or group/sort by severity using FindingSeverity order), then iterate once and call the same appendLine logic for each item (reuse the same string formatting used currently: "- **${f.severity.icon}** ${f.message}" and " - ${f.suggestion}"); mirror the approach used in IncludedBuildRenderer.appendProblems() to sort/group and render in ProjectReportRenderer (preserve the initial check for empty lists before writing the "## Findings" header).src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt (1)
173-174: Consider adding a clarifying comment for the data class exclusion.Including
isDataClassin the abstraction check is a reasonable design choice (DTOs/value objects typically don't need interfaces), but it's non-obvious. A brief comment would help future readers understand the intent.// Data classes are excluded as they're typically value objects that don't require abstraction val isAbstraction = resolved.source.isInterface || resolved.source.isAbstract || resolved.source.isDataClass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt` around lines 173 - 174, Add a brief clarifying comment above the isAbstraction check in AntiPatternDetector (the mapNotNull block where resolved.source.isInterface / isAbstract / isDataClass are evaluated) explaining why data classes are treated as abstractions to be excluded (e.g., "Data classes are excluded as they're typically value/DTO objects that don't require an interface or further abstraction"), so future readers understand the intent behind including resolved.source.isDataClass in the condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt`:
- Around line 193-201: The message in AntiPatternDetector.kt is misleading
because the detection scans imports (symbol `c` / `c.source.simpleName`) not
constructor parameters; update the `message` (and optionally `suggestion`) where
`ifaceName` and `c.source.simpleName` are used to say something like "Code
imports concrete `<class>` instead of interface `<ifaceName>`" or "Imports
concrete `<class>` instead of interface `<ifaceName>`" so it reflects
import-based detection rather than "Constructor takes..."; keep the rest of the
diagnostic (severity, file, suggestion) unchanged except to rephrase the
suggestion text to "Depend on the interface `<ifaceName>` instead of the
concrete class..." if needed.
---
Nitpick comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt`:
- Around line 173-174: Add a brief clarifying comment above the isAbstraction
check in AntiPatternDetector (the mapNotNull block where
resolved.source.isInterface / isAbstract / isDataClass are evaluated) explaining
why data classes are treated as abstractions to be excluded (e.g., "Data classes
are excluded as they're typically value/DTO objects that don't require an
interface or further abstraction"), so future readers understand the intent
behind including resolved.source.isDataClass in the condition.
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt`:
- Around line 54-56: Extract the repeated predicate used to count actionable
findings into a single helper (e.g., add a private function
isActionableSeverity(severity: FindingSeverity): Boolean) and replace the three
inline predicates (the occurrences inside the s.analysis?.findings?.count { ...
} expressions) with calls to that helper; update all instances referenced in
DashboardRenderer (the counts at the original lines and the other two
occurrences mentioned) to call isActionableSeverity(it.severity) so the logic is
centralized and easier to maintain.
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/IncludedBuildRenderer.kt`:
- Line 160: The `@Suppress`("CyclomaticComplexMethod") annotation above
appendProblems() is likely no longer needed; remove that suppression annotation
(the one immediately preceding the appendProblems function declaration in
IncludedBuildRenderer) and then run the static-analysis/lint and compile to
confirm no new warnings or errors; if any remaining complexity warnings
reference appendProblems, simplify branching or narrow the suppression to
specific cases instead of the whole method.
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/ProjectReportRenderer.kt`:
- Around line 89-107: The three nearly identical loops that render findings
(variables forbidden, warnings, infos derived from analysis.findings filtered by
FindingSeverity) should be consolidated into a single iteration: build one list
from analysis.findings (or group/sort by severity using FindingSeverity order),
then iterate once and call the same appendLine logic for each item (reuse the
same string formatting used currently: "- **${f.severity.icon}** ${f.message}"
and " - ${f.suggestion}"); mirror the approach used in
IncludedBuildRenderer.appendProblems() to sort/group and render in
ProjectReportRenderer (preserve the initial check for empty lists before writing
the "## Findings" header).
In
`@src/test/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetectorTest.kt`:
- Around line 220-232: The test block in AntiPatternDetectorTest mixes
assertions for multiple severities while its `when` title only mentions
FORBIDDEN; split or rename it so each severity is clearly described: either
rename the `when` to something like "`severity icons`" or create separate
`when`/`then` blocks for AntiPattern.Severity.FORBIDDEN,
AntiPattern.Severity.WARNING, and AntiPattern.Severity.INFO, keeping each
assertion (accessing the `.icon` property) under the matching `when`/`then` to
make intent and failure messages clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04299182-d4cf-4c44-b421-1d4e6bc77c9c
📒 Files selected for processing (9)
src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.ktsrc/main/kotlin/zone/clanker/gradle/srcx/analysis/ProjectAnalysis.ktsrc/main/kotlin/zone/clanker/gradle/srcx/model/AnalysisSummary.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/IncludedBuildRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/ProjectReportRenderer.ktsrc/test/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetectorTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/IncludedBuildRendererTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/ProjectReportAnalysisTest.kt
ContextTask now collects source dirs from root project AND all included builds, runs the full analysis pipeline on the merged set. Hub classes referenced across build boundaries now appear in a Hot Classes (cross-build) section with a table and per-hub dependent lists. Uses model-layer AnalysisSummary to respect package boundary rules (report does not import from analysis).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt (1)
56-61: Consider: Summary label says "warnings" but includes FORBIDDEN findings.The count includes both
WARNINGandFORBIDDENseverities, but the label remains "warnings". Users might expect FORBIDDEN to be counted separately or the label to reflect both. This is consistent across the file but worth considering for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt` around lines 56 - 61, The calc for totalWarnings mixes FindingSeverity.WARNING and FindingSeverity.FORBIDDEN so the label "warnings" is misleading; update DashboardRenderer.kt to separate counts: compute totalWarnings by counting only FindingSeverity.WARNING and compute totalForbidden by counting FindingSeverity.FORBIDDEN (or rename totalWarnings to totalWarningsAndForbidden if you prefer a combined metric), then use the appropriate variable(s) when rendering labels so the UI shows distinct "warnings" and "forbidden" counts (or a combined label that clearly indicates both).src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt (1)
111-140: UnusedrootDirparameter.The
rootDirparameter is suppressed as unused but remains in the signature. If it's intentionally unused (since package-level findings useFile(".")), consider removing it to simplify the API. If other detection functions require a consistent signature, this is acceptable as-is.♻️ Option: Remove unused parameter
-@Suppress("UnusedParameter") -private fun detectForbiddenNames( - components: List<ClassifiedComponent>, - rootDir: File, -): List<AntiPattern> { +private fun detectForbiddenNames( + components: List<ClassifiedComponent>, +): List<AntiPattern> {And update the call site at line 52:
- patterns.addAll(detectForbiddenNames(components, rootDir)) + patterns.addAll(detectForbiddenNames(components))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt` around lines 111 - 140, The detectForbiddenNames function has an unused rootDir parameter; remove rootDir from the signature of detectForbiddenNames and update all call sites that pass a rootDir (so callers match the new signature), and keep the body using File(".") as before (or, if you prefer to surface a real file, switch the AntiPattern.file to use a representative file from ClassifiedComponent.source where available); ensure you update references to detectForbiddenNames everywhere to avoid compilation errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt`:
- Around line 39-40: FORBIDDEN_PACKAGE_NAMES in AntiPatternDetector.kt is
missing "common" and "shared"; update the set declared as
FORBIDDEN_PACKAGE_NAMES to include "common" and "shared" (so it matches the PR
objectives: utils, helpers, managers, common, misc, base, shared), ensuring the
change appears inside the same val/set declaration used by the
AntiPatternDetector logic.
---
Nitpick comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt`:
- Around line 111-140: The detectForbiddenNames function has an unused rootDir
parameter; remove rootDir from the signature of detectForbiddenNames and update
all call sites that pass a rootDir (so callers match the new signature), and
keep the body using File(".") as before (or, if you prefer to surface a real
file, switch the AntiPattern.file to use a representative file from
ClassifiedComponent.source where available); ensure you update references to
detectForbiddenNames everywhere to avoid compilation errors.
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt`:
- Around line 56-61: The calc for totalWarnings mixes FindingSeverity.WARNING
and FindingSeverity.FORBIDDEN so the label "warnings" is misleading; update
DashboardRenderer.kt to separate counts: compute totalWarnings by counting only
FindingSeverity.WARNING and compute totalForbidden by counting
FindingSeverity.FORBIDDEN (or rename totalWarnings to totalWarningsAndForbidden
if you prefer a combined metric), then use the appropriate variable(s) when
rendering labels so the UI shows distinct "warnings" and "forbidden" counts (or
a combined label that clearly indicates both).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd34ac4f-6366-4378-b873-5138e2743bcf
📒 Files selected for processing (3)
src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.kt
✅ Files skipped from review due to trivial changes (1)
- src/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.kt
Add forbiddenPackages and forbiddenClassSuffixes to the srcx
extension DSL. Both are additive on top of defaults. Also adds
detectForbiddenClassNames for suffix-based class name detection.
Usage:
srcx {
forbiddenPackages.add("legacy")
forbiddenClassSuffixes.add("BaseActivity")
}
- forbiddenPackages("legacy", "internal") vararg syntax
- forbiddenClassPatterns("Base", "Impl") vararg syntax
- Class name matching uses contains instead of endsWith
so BaseActivity, DataHelperImpl, AbstractManager all match
- Rename DEFAULT_FORBIDDEN_CLASS_SUFFIXES to DEFAULT_FORBIDDEN_CLASS_PATTERNS
- Remove class doc comments from constants
Extract long string literals into vals. Use triple-quoted templates where needed. Remove @Suppress("MaxLineLength") annotations. All messages stay as single strings, no + concatenation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt (1)
214-215:⚠️ Potential issue | 🟡 MinorMisleading message: detection is import-based, not constructor-based.
The message says "Constructor takes concrete" but this detection scans
imports(line 191), not constructor parameters. An import may be used anywhere—field declarations, method parameters, local variables, static calls—not necessarily in a constructor.This was flagged in a previous review but appears unaddressed.
🔧 Proposed fix
- "Constructor takes concrete `$concreteName` instead of interface `$ifaceName`" + "Depends on concrete `$concreteName` instead of interface `$ifaceName`"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt` around lines 214 - 215, The diagnostic message in AntiPatternDetector currently reads "Constructor takes concrete `$concreteName` instead of interface `$ifaceName`" but the detector scans imports (see import-checking logic around the imports scan) not constructor parameters; update the message string `msg` to accurately reflect import-based detection (e.g. "Import of concrete `$concreteName` instead of interface `$ifaceName`" or similar) so it matches the actual check performed by the code in AntiPatternDetector.
🧹 Nitpick comments (3)
src/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.kt (2)
91-97: Naming inconsistency:forbiddenClassSuffixesvsforbiddenClassPatterns.The property is named
forbiddenClassSuffixesbut the PR description and theanalyzeProjectfunction signature (seeProjectAnalysis.kt:64-75) useforbiddenClassPatterns. The matching logic usescontains()(pattern-based), notendsWith()(suffix-based). This mismatch may confuse future maintainers.Consider renaming to
forbiddenClassPatternsfor consistency with the actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.kt` around lines 91 - 97, Rename the task property forbiddenClassSuffixes to forbiddenClassPatterns to match the actual pattern-based matching behavior and the analyzeProject signature; update the abstract property name in ContextTask (keeping `@get`:Input and SetProperty<String>), and update all call sites/usages (e.g., where ContextTask.forbiddenClassSuffixes is passed to analyzeProject or referenced elsewhere) so they use forbiddenClassPatterns and preserve types and annotations; also update any documentation/comments to reflect "patterns" instead of "suffixes".
177-188: Duplicate work: sources are scanned and classified twice.
analyzeProject(line 178) internally callsscanSourcesandclassifyAll(seeProjectAnalysis.kt:70-72). Lines 184-187 then repeat these same operations on the sameallDirs. This doubles the work unnecessarily.Consider either extracting the diagram generation into
ProjectAnalysisor reusing its internal results.♻️ Possible refactor
return runCatching { val analysis = analyzeProject( allDirs, rootDir, forbiddenPackages.get(), forbiddenClassSuffixes.get(), ) - val sources = scanSources(allDirs) - val components = classifyAll(sources) - val depEdges = buildDependencyGraph(components) - val diagram = generateDependencyDiagram(components, depEdges) + val diagram = analysis.generateDependencyDiagram() // if exposed diagram to analysis }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.kt` around lines 177 - 188, analyzeProject currently performs scanSources and classifyAll internally, but the code then calls scanSources and classifyAll again to build the diagram, causing duplicated work; modify the implementation so you reuse the results from analyzeProject (e.g., extend analyzeProject to return ProjectAnalysis that includes sources/components or expose getter methods for sources/components) and then call generateDependencyDiagram with those returned components/depEdges instead of re-scanning; alternatively move generateDependencyDiagram into ProjectAnalysis (or add a toDiagram() method) and call that to produce the diagram, ensuring analyzeProject, scanSources, classifyAll and generateDependencyDiagram are not invoked twice for the same allDirs.src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt (1)
56-61: Label says "warnings" but count includes FORBIDDEN findings.
totalWarningsaggregates bothWARNINGandFORBIDDENseverities, but the label at line 71 only says "warnings". Users may not realize FORBIDDEN items are included in this count. Consider either renaming the variable/label to "issues" or separating the counts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt` around lines 56 - 61, The variable totalWarnings in DashboardRenderer aggregates both FindingSeverity.WARNING and FindingSeverity.FORBIDDEN but the UI label only says "warnings"; update the code to make this accurate by either (A) renaming totalWarnings and its usage/label to a neutral term like totalIssues/totalFindings and update the displayed label accordingly, or (B) split the calculation into two distinct counters (e.g., totalWarnings for FindingSeverity.WARNING and totalForbidden for FindingSeverity.FORBIDDEN) and update the renderer to display both counts separately; locate the aggregation where totalWarnings is computed and any UI label rendering (references: totalWarnings, FindingSeverity.WARNING, FindingSeverity.FORBIDDEN, DashboardRenderer) and make the corresponding variable/label changes so counts and text match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt`:
- Around line 150-152: The filter that excludes test files in
AntiPatternDetector.kt currently only checks for "/test/" (the anonymous .filter
block around the code using it); update this to handle Windows paths too by
matching both forward and backslash separators (e.g., check contains("/test/")
OR contains("\\test\\") or normalize the path before checking), mirroring the
other checks in this file (see similar logic around lines referenced for
consistency) so test file detection is platform-independent.
In `@src/main/kotlin/zone/clanker/gradle/srcx/Srcx.kt`:
- Around line 75-79: DEFAULT_FORBIDDEN_PACKAGES is missing two entries from the
spec; update the Set assigned to DEFAULT_FORBIDDEN_PACKAGES in Srcx.kt to
include "common" and "shared" so it matches the PR objectives (keep the existing
entries: "util","utils","helper","helpers","manager","managers","misc","base"
and add "common","shared"). Ensure only the DEFAULT_FORBIDDEN_PACKAGES value is
changed and DEFAULT_FORBIDDEN_CLASS_PATTERNS remains untouched.
---
Duplicate comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.kt`:
- Around line 214-215: The diagnostic message in AntiPatternDetector currently
reads "Constructor takes concrete `$concreteName` instead of interface
`$ifaceName`" but the detector scans imports (see import-checking logic around
the imports scan) not constructor parameters; update the message string `msg` to
accurately reflect import-based detection (e.g. "Import of concrete
`$concreteName` instead of interface `$ifaceName`" or similar) so it matches the
actual check performed by the code in AntiPatternDetector.
---
Nitpick comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt`:
- Around line 56-61: The variable totalWarnings in DashboardRenderer aggregates
both FindingSeverity.WARNING and FindingSeverity.FORBIDDEN but the UI label only
says "warnings"; update the code to make this accurate by either (A) renaming
totalWarnings and its usage/label to a neutral term like
totalIssues/totalFindings and update the displayed label accordingly, or (B)
split the calculation into two distinct counters (e.g., totalWarnings for
FindingSeverity.WARNING and totalForbidden for FindingSeverity.FORBIDDEN) and
update the renderer to display both counts separately; locate the aggregation
where totalWarnings is computed and any UI label rendering (references:
totalWarnings, FindingSeverity.WARNING, FindingSeverity.FORBIDDEN,
DashboardRenderer) and make the corresponding variable/label changes so counts
and text match.
In `@src/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.kt`:
- Around line 91-97: Rename the task property forbiddenClassSuffixes to
forbiddenClassPatterns to match the actual pattern-based matching behavior and
the analyzeProject signature; update the abstract property name in ContextTask
(keeping `@get`:Input and SetProperty<String>), and update all call sites/usages
(e.g., where ContextTask.forbiddenClassSuffixes is passed to analyzeProject or
referenced elsewhere) so they use forbiddenClassPatterns and preserve types and
annotations; also update any documentation/comments to reflect "patterns"
instead of "suffixes".
- Around line 177-188: analyzeProject currently performs scanSources and
classifyAll internally, but the code then calls scanSources and classifyAll
again to build the diagram, causing duplicated work; modify the implementation
so you reuse the results from analyzeProject (e.g., extend analyzeProject to
return ProjectAnalysis that includes sources/components or expose getter methods
for sources/components) and then call generateDependencyDiagram with those
returned components/depEdges instead of re-scanning; alternatively move
generateDependencyDiagram into ProjectAnalysis (or add a toDiagram() method) and
call that to produce the diagram, ensuring analyzeProject, scanSources,
classifyAll and generateDependencyDiagram are not invoked twice for the same
allDirs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 802e59ea-64dd-4904-a27f-6a0260f9d4c6
📒 Files selected for processing (5)
src/main/kotlin/zone/clanker/gradle/srcx/Srcx.ktsrc/main/kotlin/zone/clanker/gradle/srcx/analysis/AntiPatternDetector.ktsrc/main/kotlin/zone/clanker/gradle/srcx/analysis/ProjectAnalysis.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/kotlin/zone/clanker/gradle/srcx/analysis/ProjectAnalysis.kt
Comment on lines
+75
to
+79
| val DEFAULT_FORBIDDEN_PACKAGES: Set<String> = | ||
| setOf("util", "utils", "helper", "helpers", "manager", "managers", "misc", "base") | ||
|
|
||
| val DEFAULT_FORBIDDEN_CLASS_PATTERNS: Set<String> = | ||
| setOf("Helper", "Manager", "Utils", "Util") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if DEFAULT_FORBIDDEN_PACKAGES includes "common" and "shared" in the current code
rg -n "DEFAULT_FORBIDDEN_PACKAGES" --type=kotlin -A 3Repository: ClankerGuru/srcx
Length of output: 2034
Add common and shared to DEFAULT_FORBIDDEN_PACKAGES.
The PR objectives specify forbidden package names as: utils, helpers, managers, common, misc, base, shared. The current DEFAULT_FORBIDDEN_PACKAGES set at line 76 is missing common and shared.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/zone/clanker/gradle/srcx/Srcx.kt` around lines 75 - 79,
DEFAULT_FORBIDDEN_PACKAGES is missing two entries from the spec; update the Set
assigned to DEFAULT_FORBIDDEN_PACKAGES in Srcx.kt to include "common" and
"shared" so it matches the PR objectives (keep the existing entries:
"util","utils","helper","helpers","manager","managers","misc","base" and add
"common","shared"). Ensure only the DEFAULT_FORBIDDEN_PACKAGES value is changed
and DEFAULT_FORBIDDEN_CLASS_PATTERNS remains untouched.
Owner
Author
There was a problem hiding this comment.
╔══════════════════════════════════╗
║ ℹ️ Intentional ║
╚══════════════════════════════════╝
Same as the earlier comment -- "common" and "shared" were intentionally excluded from DEFAULT_FORBIDDEN_PACKAGES. These are valid, descriptive names used in many projects (e.g. Kotlin Multiplatform shared modules). The forbidden list targets genuinely vague names like "util", "helper", "manager", "misc", and "base". The plugin resolution analysis is also incorrect -- the plugin is registered in build.gradle.kts via the gradlePlugin block and resolved from the build-logic included build. settings.gradle.kts only applies "clkx-settings", not "zone.clanker.gradle.srcx".
There was a problem hiding this comment.
@ClankerGuru, understood and apologies for the noise! Both points are duly noted:
commonandsharedexclusion — confirmed intentional; these are valid, widely-used package names (e.g. Kotlin Multiplatformsharedmodules) and the forbidden list is scoped to genuinely vague names likeutil,helper,manager,misc, andbase.- Plugin resolution — the plugin
zone.clanker.gradle.srcxis registered via thegradlePluginblock inbuild.gradle.ktsand resolved from the build-logic included build.settings.gradle.ktsonly appliesclkx-settings, notzone.clanker.gradle.srcx.
I'll update my memory so I don't repeat these mistakes in future reviews.
(╯°□°）╯彡🐇 trips over wrong analysis, tumbles away
✏️ Learnings added
Learnt from: ClankerGuru
Repo: ClankerGuru/srcx PR: 7
File: src/main/kotlin/zone/clanker/gradle/srcx/Srcx.kt:75-79
Timestamp: 2026-04-13T14:29:46.817Z
Learning: In the srcx project (ClankerGuru/srcx), the `zone.clanker.gradle.srcx` plugin is registered via the `gradlePlugin` block in `build.gradle.kts` and resolved from the build-logic included build. `settings.gradle.kts` only applies the `clkx-settings` plugin, not `zone.clanker.gradle.srcx`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ClankerGuru
Repo: ClankerGuru/srcx PR: 0
File: :0-0
Timestamp: 2026-04-13T00:30:35.774Z
Learning: In the srcx project (ClankerGuru/srcx), the forbidden package names intentionally exclude `common` and `shared` because they are valid, widely-used package names in the codebase. Only `utils`, `helpers`, `managers`, `misc`, and `base` are treated as forbidden package names. The PR description listing `common` and `shared` as forbidden was inaccurate; the implementation is correct.
New split output files under .srcx/:
- hot-classes.md, entry-points.md, anti-patterns.md
- interfaces.md, cross-build.md, flows/{EntryPoint}.md
New analysis: ArchitecturalLayer enum, EntryPointKind classification.
context.md slimmed to overview + links to detail files.
6 new renderers with tests. Coverage at 91.8%.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/kotlin/zone/clanker/gradle/srcx/report/DashboardRendererTest.kt (1)
147-200:⚠️ Potential issue | 🟡 MinorTest intent and assertions are now misaligned
At Line 147, the scenario still says “hub classes and dependent names”, but Line 199-200 only verifies a generic project-row substring. This drops coverage for the hub/dependent behavior that the fixture is building and makes the test misleading.
Consider either renaming/simplifying the scenario setup, or restoring assertions that validate the hub/dependent-related output path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/gradle/srcx/report/DashboardRendererTest.kt` around lines 147 - 200, The test description says it verifies "hub classes and dependent names" but the assertion only checks for a generic project row; update the test to assert the hub-related output produced by DashboardRenderer("test", summaries, emptyList()).render() — specifically verify the rendered output contains the hub class "Core" and the dependent names "A", "B", and "C" (and optionally their paths or counts from HubClass/HubDependentRef) so the fixtures (ProjectSummary, HubClass, HubDependentRef) are actually validated; alternatively, if you intend a simpler test, rename the scenario to not mention hubs and simplify the fixture to match the assertion.
🧹 Nitpick comments (6)
src/test/kotlin/zone/clanker/gradle/srcx/report/EntryPointsRendererTest.kt (1)
71-114: Consider adding test case for "Stub" pattern.The test doubles test covers
MockRepositoryandFakeServicebut doesn't test theStubpattern that the implementation supports.✨ Optional: Add Stub pattern test
SymbolEntry( SymbolName("FakeService"), SymbolKind.CLASS, PackageName("com.example"), FilePath("FakeService.kt"), 1, ), + SymbolEntry( + SymbolName("StubClient"), + SymbolKind.CLASS, + PackageName("com.example"), + FilePath("StubClient.kt"), + 1, + ), ), listOf("src/test/kotlin"), ),And update the assertion:
then("it shows test doubles") { output shouldContain "## Test Doubles" output shouldContain "| `MockRepository` | com.example | Mock |" output shouldContain "| `FakeService` | com.example | Fake |" + output shouldContain "| `StubClient` | com.example | Stub |" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/gradle/srcx/report/EntryPointsRendererTest.kt` around lines 71 - 114, Add a test case entry to the existing "rendering with test doubles" scenario to exercise the Stub pattern: in the summaries passed to EntryPointsRenderer add a SymbolEntry with SymbolName("ExampleStub") (or similar), SymbolKind.CLASS, PackageName("com.example"), FilePath("ExampleStub.kt"), and ensure it's included in the same SourceSetSummary; then extend the assertions after renderer.render() to expect the Stub section/row—e.g., output shouldContain a table row containing "`ExampleStub`" and "Stub" (alongside the existing Mock and Fake checks) so the renderer's Stub handling is verified.src/test/kotlin/zone/clanker/gradle/srcx/report/AntiPatternsRendererTest.kt (1)
113-154: Consider asserting which source is preserved during deduplication.The deduplication test verifies the message appears once but doesn't assert which source (
:aor:b) is retained. This could help catch regressions if the deduplication order changes.✨ Optional: Verify preserved source
then("it shows the duplicate message only once") { val occurrences = output.split("duplicate message").size - 1 occurrences shouldBe 1 + // Verify first source is preserved + output shouldContain "**:a**" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/gradle/srcx/report/AntiPatternsRendererTest.kt` around lines 113 - 154, The test currently only asserts the duplicate message appears once; update the test for AntiPatternsRenderer.render to also assert which project source is preserved after deduplication — e.g., verify the output contains the project path for the first summary (ProjectPath(":a")) and does not contain the second (ProjectPath(":b")) to lock in the expected preservation order; locate this behavior in the test block around AntiPatternsRenderer(summaries).render() and add the corresponding positive and negative assertions.src/main/kotlin/zone/clanker/gradle/srcx/report/AntiPatternsRenderer.kt (1)
63-76: Deduplication may lose valuable source context.The
distinctBy { it.second.message }approach keeps only the first occurrence of each finding message, discarding subsequent sources. If the same anti-pattern appears in multiple projects (e.g.,:appand:lib), the report will only show one source.Consider whether you want to:
- Group findings by message and list all affected sources, or
- Keep the current behavior but document that only the first source is shown.
♻️ Optional: Group sources per finding message
- private fun collectAllFindings(): List<Pair<String, Finding>> { + private fun collectAllFindings(): List<Pair<List<String>, Finding>> { val rootFindings = summaries.flatMap { s -> val path = s.projectPath.value s.analysis?.findings?.map { f -> path to f } ?: emptyList() } val buildFindings = includedBuildSummaries.flatMap { (name, projects) -> projects.flatMap { s -> s.analysis?.findings?.map { f -> name to f } ?: emptyList() } } - return (rootFindings + buildFindings).distinctBy { it.second.message } + return (rootFindings + buildFindings) + .groupBy { it.second.message } + .map { (_, pairs) -> pairs.map { it.first } to pairs.first().second } }Then update the rendering to show multiple sources:
appendLine("- ${finding.severity.icon} **${sources.joinToString(", ")}** — ${finding.message}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/AntiPatternsRenderer.kt` around lines 63 - 76, collectAllFindings currently deduplicates by finding.message using distinctBy which drops additional source context; change it to group findings by it.second.message (use groupBy on message) and produce a structure that preserves all source paths per message (e.g., map to Pair<List<String>, Finding> or a small data class with sources and finding), then update the renderer code that consumes collectAllFindings (in AntiPatternsRenderer rendering block that builds lines) to iterate each grouped entry and print all sources (joinToString) next to the finding message/severity; alternatively, if you prefer minimal change, add a comment/docs near collectAllFindings explaining that only the first source is shown.src/main/kotlin/zone/clanker/gradle/srcx/report/InterfacesRenderer.kt (1)
124-135: Implementation counting heuristics look reasonable but may have edge cases.The
endsWith(baseName)check on line 132 could potentially over-count for short interface names. For example, if the interface isIService, the baseName becomesService, and any class ending inService(likeUserService,OrderService) would be counted as an implementation.This may be intentional to cast a wide net, but consider if you want tighter matching.
♻️ Optional: Tighter implementation matching
private fun countImplementations(interfaceName: String, classNames: Set<String>): Int { val baseName = interfaceName.removePrefix("I") return classNames.count { cn -> cn != interfaceName && ( cn == "${baseName}Impl" || cn == "Default$interfaceName" || cn == "Default$baseName" || - (cn.endsWith(baseName) && cn != baseName) + (cn.endsWith(baseName) && cn != baseName && cn.length > baseName.length) ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/InterfacesRenderer.kt` around lines 124 - 135, The heuristic in countImplementations can over-count because cn.endsWith(baseName) will match things like UserService for IService; update the logic in countImplementations to tighten suffix matches: keep the existing exact-name and Default/Impl checks, but replace the broad cn.endsWith(baseName) branch with a guard that only counts when the class actually has a CamelCase boundary before the baseName (e.g., the character immediately before the matched baseName is an uppercase letter) and optionally skip this suffix rule for very short baseName values; modify the check around cn.endsWith(baseName) to verify cn.length > baseName.length and that the char at cn.length - baseName.length - 1 isUpperCase() (and/or check baseName.length >= 3) so only true implementation-style class names are counted.src/main/kotlin/zone/clanker/gradle/srcx/report/CrossBuildRenderer.kt (1)
33-39: Prefer stable ordering for grouped dependenciesAt Line 33-39, output order depends on incoming edge order. Sorting
fromgroups andtoentries makes generated markdown deterministic and reduces noisy diffs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/CrossBuildRenderer.kt` around lines 33 - 39, The grouping iteration over buildEdges (val byFrom = buildEdges.groupBy { it.from }) is non-deterministic; change the loop to iterate over sorted group keys and sorted edge targets so output is stable: enumerate byFrom.keys.sorted() (or byFrom.entries.sortedBy { it.key }) and for each group's edges use edges.sortedBy { it.to } before appending lines for "from" and each edge.to to produce deterministic markdown in CrossBuildRenderer (variables: buildEdges, byFrom, from, edges, edge.to).src/main/kotlin/zone/clanker/gradle/srcx/report/HotClassesRenderer.kt (1)
27-32: Make output ordering deterministic in the rendererAt Line 27 and Line 31, rendering relies on input order. If callers pass unsorted hubs, report ordering can fluctuate. Sorting locally keeps output stable and consistent with the KDoc expectation.
Proposed refactor
internal class HotClassesRenderer( private val hubs: List<HubClass>, ) { fun render(): String = buildString { + val orderedHubs = hubs.sortedByDescending { it.dependentCount } appendLine("# Hot Classes") appendLine() - if (hubs.isEmpty()) { + if (orderedHubs.isEmpty()) { appendLine("No hub classes detected.") appendLine() return@buildString } appendLine("| Class | File | Dependents | Role |") appendLine("|-------|------|------------|------|") - for (hub in hubs) { + for (hub in orderedHubs) { appendLine("| `${hub.name}` | ${hub.filePath}:${hub.line} | ${hub.dependentCount} | ${hub.role} |") } appendLine() - for (hub in hubs.filter { it.dependentCount >= DETAIL_THRESHOLD }) { + for (hub in orderedHubs.filter { it.dependentCount >= DETAIL_THRESHOLD }) { appendLine("## ${hub.name}") appendLine() for (dep in hub.dependents) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/HotClassesRenderer.kt` around lines 27 - 32, The renderer iterates over the input list "hubs" in HotClassesRenderer which makes output order non-deterministic; sort the hubs locally before rendering (e.g., create a sorted list = hubs.sortedWith(compareByDescending<Hub> { it.dependentCount }.thenBy { it.name })) and use that sorted list in both table generation and the detailed section (including the filter for DETAIL_THRESHOLD) so output is stable and deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/CrossBuildRenderer.kt`:
- Around line 19-23: The renderer currently only shows the "No cross-build
references detected." message when buildEdges.isEmpty() and crossBuildAnalysis
== null, which skips the fallback when crossBuildAnalysis exists but has no hubs
or cycles; update the conditional in CrossBuildRenderer (the block referencing
buildEdges and crossBuildAnalysis) to treat an analysis with empty hubs and
cycles as equivalent to null — e.g., change the check to if
(buildEdges.isEmpty() && (crossBuildAnalysis == null ||
(crossBuildAnalysis.hubs.isEmpty() && crossBuildAnalysis.cycles.isEmpty()))) and
keep the existing appendLine/return behavior so the no-data message is emitted
for empty-but-non-null analyses.
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt`:
- Around line 50-55: totalWarnings currently sums only top-level summaries,
missing findings from included builds; update the calculation (the totalWarnings
variable) to include findings from includedBuildSummaries as well by aggregating
root summaries plus each included build's summaries' analysis.findings and
counting those with severity == FindingSeverity.WARNING or
FindingSeverity.FORBIDDEN (mirroring how totalSymbols incorporates
includedBuildSummaries). Locate totalWarnings and modify its summation to
iterate both summaries and includedBuildSummaries (or concatenate the lists) so
included-build findings are counted.
- Around line 139-160: The early return in appendSplitFileLinks() hides links to
split files that ContextTask.writeSplitFiles() always emits (entry-points.md and
interfaces.md) and may emit cross-build.md; remove the early return and always
render the "Details" section and the mandatory links for entry-points.md and
interfaces.md, while keeping the conditional additions for hot-classes (when
crossBuildAnalysis?.hubs?.isNotEmpty()), anti-patterns (when
summaries/includedBuildSummaries contain findings), and cross-build (when
buildEdges.isNotEmpty() || crossBuildAnalysis != null); update the function
appendSplitFileLinks() to rely on the existing booleans (hasHubs, hasFindings)
and variables (crossBuildAnalysis, summaries, includedBuildSummaries,
buildEdges) and reference ContextTask.writeSplitFiles behavior in a comment if
helpful.
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/EntryPointsRenderer.kt`:
- Around line 91-99: findTestClasses() only detects classes ending with "Test"
but should also include "Spec" like
ComponentClassifier.classifySingleEntryPoint; update the filter in
findTestClasses() (the lambda that selects symbols where it.kind ==
SymbolKind.CLASS && it.name.value.endsWith("Test")) to also accept names ending
with "Spec" (e.g., check endsWith("Test") || endsWith("Spec")) so both
Kotest/Spek-style test classes are included; keep the rest of the pipeline
(sourceSets filtering and distinctBy) unchanged.
In `@src/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.kt`:
- Around line 191-194: The anti-patterns file is generated from per-build
summaryList/includedBuildSummaries and so misses forbidden-package/class inputs
and cross-build DI findings produced by analyzeCrossBuild(); change the
anti-patterns generation to use the repo-wide analysis result returned by
analyzeCrossBuild() (or the combined crossBuildSummary variable it produces)
instead of summaryList/includedBuildSummaries when constructing
AntiPatternsRenderer so anti-patterns.md reflects cross-build findings and
forbidden inputs.
In `@src/test/kotlin/zone/clanker/gradle/srcx/report/InterfacesRendererTest.kt`:
- Around line 102-151: Test shows mocks (e.g., MockUserService) are being
counted both as implementations and as mocks; update
InterfacesRenderer.fromSummaries to exclude mock/fake naming patterns (e.g.,
names starting with "Mock", "Fake", or matching configured mock prefixes) from
the implementation detection logic so they only set hasMock and are not
incremented in implementationCount, keep detection of real implementations like
UserServiceImpl unchanged, and then update the test in InterfacesRendererTest
(the "summaries have interface-like naming (Service suffix)" case) to expect
implementationCount = 1 while hasMock remains true.
---
Outside diff comments:
In `@src/test/kotlin/zone/clanker/gradle/srcx/report/DashboardRendererTest.kt`:
- Around line 147-200: The test description says it verifies "hub classes and
dependent names" but the assertion only checks for a generic project row; update
the test to assert the hub-related output produced by DashboardRenderer("test",
summaries, emptyList()).render() — specifically verify the rendered output
contains the hub class "Core" and the dependent names "A", "B", and "C" (and
optionally their paths or counts from HubClass/HubDependentRef) so the fixtures
(ProjectSummary, HubClass, HubDependentRef) are actually validated;
alternatively, if you intend a simpler test, rename the scenario to not mention
hubs and simplify the fixture to match the assertion.
---
Nitpick comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/AntiPatternsRenderer.kt`:
- Around line 63-76: collectAllFindings currently deduplicates by
finding.message using distinctBy which drops additional source context; change
it to group findings by it.second.message (use groupBy on message) and produce a
structure that preserves all source paths per message (e.g., map to
Pair<List<String>, Finding> or a small data class with sources and finding),
then update the renderer code that consumes collectAllFindings (in
AntiPatternsRenderer rendering block that builds lines) to iterate each grouped
entry and print all sources (joinToString) next to the finding message/severity;
alternatively, if you prefer minimal change, add a comment/docs near
collectAllFindings explaining that only the first source is shown.
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/CrossBuildRenderer.kt`:
- Around line 33-39: The grouping iteration over buildEdges (val byFrom =
buildEdges.groupBy { it.from }) is non-deterministic; change the loop to iterate
over sorted group keys and sorted edge targets so output is stable: enumerate
byFrom.keys.sorted() (or byFrom.entries.sortedBy { it.key }) and for each
group's edges use edges.sortedBy { it.to } before appending lines for "from" and
each edge.to to produce deterministic markdown in CrossBuildRenderer (variables:
buildEdges, byFrom, from, edges, edge.to).
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/HotClassesRenderer.kt`:
- Around line 27-32: The renderer iterates over the input list "hubs" in
HotClassesRenderer which makes output order non-deterministic; sort the hubs
locally before rendering (e.g., create a sorted list =
hubs.sortedWith(compareByDescending<Hub> { it.dependentCount }.thenBy { it.name
})) and use that sorted list in both table generation and the detailed section
(including the filter for DETAIL_THRESHOLD) so output is stable and
deterministic.
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/InterfacesRenderer.kt`:
- Around line 124-135: The heuristic in countImplementations can over-count
because cn.endsWith(baseName) will match things like UserService for IService;
update the logic in countImplementations to tighten suffix matches: keep the
existing exact-name and Default/Impl checks, but replace the broad
cn.endsWith(baseName) branch with a guard that only counts when the class
actually has a CamelCase boundary before the baseName (e.g., the character
immediately before the matched baseName is an uppercase letter) and optionally
skip this suffix rule for very short baseName values; modify the check around
cn.endsWith(baseName) to verify cn.length > baseName.length and that the char at
cn.length - baseName.length - 1 isUpperCase() (and/or check baseName.length >=
3) so only true implementation-style class names are counted.
In `@src/test/kotlin/zone/clanker/gradle/srcx/report/AntiPatternsRendererTest.kt`:
- Around line 113-154: The test currently only asserts the duplicate message
appears once; update the test for AntiPatternsRenderer.render to also assert
which project source is preserved after deduplication — e.g., verify the output
contains the project path for the first summary (ProjectPath(":a")) and does not
contain the second (ProjectPath(":b")) to lock in the expected preservation
order; locate this behavior in the test block around
AntiPatternsRenderer(summaries).render() and add the corresponding positive and
negative assertions.
In `@src/test/kotlin/zone/clanker/gradle/srcx/report/EntryPointsRendererTest.kt`:
- Around line 71-114: Add a test case entry to the existing "rendering with test
doubles" scenario to exercise the Stub pattern: in the summaries passed to
EntryPointsRenderer add a SymbolEntry with SymbolName("ExampleStub") (or
similar), SymbolKind.CLASS, PackageName("com.example"),
FilePath("ExampleStub.kt"), and ensure it's included in the same
SourceSetSummary; then extend the assertions after renderer.render() to expect
the Stub section/row—e.g., output shouldContain a table row containing
"`ExampleStub`" and "Stub" (alongside the existing Mock and Fake checks) so the
renderer's Stub handling is verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b613fc2f-2325-40fe-b33a-0b7e30e90039
📒 Files selected for processing (18)
src/main/kotlin/zone/clanker/gradle/srcx/analysis/ComponentClassifier.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/AntiPatternsRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/CrossBuildRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/EntryPointsRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/FlowRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/HotClassesRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/InterfacesRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.ktsrc/test/kotlin/zone/clanker/gradle/srcx/BuildEdgesTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/analysis/ComponentClassifierTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/AntiPatternsRendererTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/CrossBuildRendererTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/DashboardRendererTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/EntryPointsRendererTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/FlowRendererTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/HotClassesRendererTest.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/InterfacesRendererTest.kt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt (2)
50-55:⚠️ Potential issue | 🟡 MinorInclude included-build findings in the overview warning total.
totalSymbolsalready aggregatesincludedBuildSummaries, buttotalWarningsonly counts root-project findings. If warnings exist only in an included build, the overview will still report0 warnings.Proposed fix
val totalWarnings = - summaries.sumOf { s -> - s.analysis?.findings?.count { - it.severity == FindingSeverity.WARNING || it.severity == FindingSeverity.FORBIDDEN - } ?: 0 - } + summaries.sumOf { s -> + s.analysis?.findings?.count { + it.severity == FindingSeverity.WARNING || it.severity == FindingSeverity.FORBIDDEN + } ?: 0 + } + + includedBuildSummaries.values.sumOf { projects -> + projects.sumOf { s -> + s.analysis?.findings?.count { + it.severity == FindingSeverity.WARNING || it.severity == FindingSeverity.FORBIDDEN + } ?: 0 + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt` around lines 50 - 55, totalWarnings currently sums only the root `summaries` and ignores `includedBuildSummaries`, causing included-build findings to be omitted; update the calculation to aggregate over both collections (for example by summing `(summaries + includedBuildSummaries)` or otherwise merging them) and then count findings with `it.severity == FindingSeverity.WARNING || it.severity == FindingSeverity.FORBIDDEN`, preserving the existing null-safe handling of `s.analysis?.findings`.
147-160:⚠️ Potential issue | 🟠 MajorDon’t return before rendering the split-report links.
Line 147 makes the
crossBuildAnalysis != nullcheck on Line 159 ineffective when there are no hubs/findings/build edges, soCross-Build Referencescan disappear even though cross-build analysis exists. It also hides the unconditionalEntry PointsandInterfaceslinks in the same case.Proposed fix
- if (!hasHubs && !hasFindings && buildEdges.isEmpty()) return - appendLine("## Details") appendLine() if (hasHubs) { appendLine("- [Hub Classes](hub-classes.md)") } appendLine("- [Entry Points](entry-points.md)") if (hasFindings) { appendLine("- [Anti-Patterns](anti-patterns.md)") } appendLine("- [Interfaces](interfaces.md)") if (buildEdges.isNotEmpty() || crossBuildAnalysis != null) { appendLine("- [Cross-Build References](cross-build.md)") } appendLine()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt` around lines 147 - 160, The early return "if (!hasHubs && !hasFindings && buildEdges.isEmpty()) return" prevents rendering split-report links (Entry Points, Interfaces, Cross-Build References) when crossBuildAnalysis exists or when you still want unconditional links; update the condition or move the return so that crossBuildAnalysis is considered — either include "crossBuildAnalysis == null" in the compound check or move the return to after the block that appends the split-report links; ensure checks reference hasHubs, hasFindings, buildEdges, and crossBuildAnalysis in DashboardRenderer.kt so Cross-Build References and the unconditional Entry Points and Interfaces links are emitted when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@settings.gradle.kts`:
- Line 12: The plugin declaration zone.clanker.gradle.srcx is being applied in
settings.gradle.kts but its versioned registration lives in the root
build.gradle.kts (evaluated later), causing resolution failure; move the plugin
registration into the included build-logic's gradlePlugin block so it is
available during settings evaluation: add a gradlePlugin.register("srcx") entry
in build-logic/build.gradle.kts matching id = "zone.clanker.gradle.srcx" and
implementationClass = "zone.clanker.gradle.srcx.Srcx$SettingsPlugin" (and
displayName/description similar to clkx-settings), then in settings.gradle.kts
remove the explicit version and apply id("zone.clanker.gradle.srcx") so it
resolves from build-logic.
---
Duplicate comments:
In `@src/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.kt`:
- Around line 50-55: totalWarnings currently sums only the root `summaries` and
ignores `includedBuildSummaries`, causing included-build findings to be omitted;
update the calculation to aggregate over both collections (for example by
summing `(summaries + includedBuildSummaries)` or otherwise merging them) and
then count findings with `it.severity == FindingSeverity.WARNING || it.severity
== FindingSeverity.FORBIDDEN`, preserving the existing null-safe handling of
`s.analysis?.findings`.
- Around line 147-160: The early return "if (!hasHubs && !hasFindings &&
buildEdges.isEmpty()) return" prevents rendering split-report links (Entry
Points, Interfaces, Cross-Build References) when crossBuildAnalysis exists or
when you still want unconditional links; update the condition or move the return
so that crossBuildAnalysis is considered — either include "crossBuildAnalysis ==
null" in the compound check or move the return to after the block that appends
the split-report links; ensure checks reference hasHubs, hasFindings,
buildEdges, and crossBuildAnalysis in DashboardRenderer.kt so Cross-Build
References and the unconditional Entry Points and Interfaces links are emitted
when appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa50c1bc-25de-4a0d-9216-d00616ce451f
📒 Files selected for processing (5)
settings.gradle.ktssrc/main/kotlin/zone/clanker/gradle/srcx/report/DashboardRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/report/HotClassesRenderer.ktsrc/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.ktsrc/test/kotlin/zone/clanker/gradle/srcx/report/HotClassesRendererTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/kotlin/zone/clanker/gradle/srcx/report/HotClassesRendererTest.kt
- src/main/kotlin/zone/clanker/gradle/srcx/report/HotClassesRenderer.kt
- src/main/kotlin/zone/clanker/gradle/srcx/task/ContextTask.kt
- Strip KDoc and comments before same-package reference matching to eliminate false circular deps from @see cross-references - Skip self-edges in cycle detection (Case -> Case) - Add isTest field to HubClass model - Hub classes renderer separates production and test classes with 🧪 icon prefix for test classes
- Hub classes: tree format with ├── └── connectors, test paths marked 🧪 - Entry points: removed useless "First Call" column, added description - Cross-build: tree showing dependents per hub with file paths - Interfaces: fixed false detection of enum values, grouped by source set - Flows: removed entirely (import-chain tracing produced identical diagrams) - Dashboard: slimmed to overview + tables + links only, removed class diagram - Fixed false positive cycles from KDoc @see references (strip comments) - Fixed self-cycles in cycle detection (skip self-edges)
Remove pinned 2.1.20 version — use Gradle's embedded Kotlin version instead. The pinned version conflicts with Gradle 9.4.1's embedded Kotlin 2.3.0, causing ClasspathEntrySnapshotTransform failures in composite builds. Also remove self-applied srcx plugin from settings (causes bootstrap conflict) and rename build-logic for composite build compatibility.
…ST/APP/MOCK split Test classes (ending in Test/Spec, in /test/ paths) were incorrectly listed as App Entry Points because buildEntryPoints() used findEntryPoints() which returns graph roots without filtering tests. Replaced with classifyEntryPoints() which already handles TEST/MOCK/APP classification. Simplified EntryPointsRenderer to accept pre-classified entries instead of doing its own broken source-set-based detection.
context.md lives inside .srcx/, so relative links to included build reports need an extra ../ to escape the .srcx directory first.
- Fix DI violation message: "Dependency on concrete" instead of misleading "Constructor takes concrete" (detection is import-based) - Add Windows path separator check for test file exclusion consistency - Include included-build warnings in dashboard totalWarnings count - Add empty-state message in CrossBuildRenderer for present but empty analysis - Exclude mock/fake/stub classes from interface implementation counts
Owner
Author
|
Owner
Author
|
@coderabbitai The missing |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
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
1 participant
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.
Summary
utils,helpers,managers,misc,baseTest plan
com.example.helpersSummary by CodeRabbit
New Features
Style
Tests