lint: enable detekt rule: ConstructorParameterNaming#328
Open
AdrianLeeElder wants to merge 2 commits into
Open
lint: enable detekt rule: ConstructorParameterNaming#328AdrianLeeElder wants to merge 2 commits into
AdrianLeeElder wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe PR refactors internal Kotlin naming conventions from snake_case to camelCase across LLM provider DTOs while maintaining API compatibility via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/src/test/kotlin/com/moneat/services/EventServiceCoverageTest.kt (1)
147-158:⚠️ Potential issue | 🟠 MajorThese fixtures don't validate the external wire contract for future changes.
Using
Json.encodeToString(...)round-trips the same Kotlin DTO shape, so a missing@SerialNameannotation on a future camelCase field would pass the test. The round-trip succeeds because both encode and decode use the same field name when no annotation exists, even though the wire format would be incorrect. Keep at least one literal snake_case JSON fixture per DTO type to explicitly test the external Sentry payload schema (or assert the serialized keys match the expected wire format).The same vulnerability applies to all other
Json.encodeToString(Sentry...)fixtures in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/kotlin/com/moneat/services/EventServiceCoverageTest.kt` around lines 147 - 158, Replace the self-encoded fixtures with an explicit snake_case JSON fixture (or add an assertion) so the test validates the external wire contract: instead of using Json.encodeToString(...) for SentryEvent/ExceptionInfo/ExceptionValue (the eventJson variable), supply a hard-coded snake_case JSON string representing the Sentry payload (e.g. "event_id", "exception": {"values":[{"type","value"}]}, etc.) and use that fixture for decoding/round-trip checks, or after Json.encodeToString(SentryEvent(...)) assert the produced JSON object's keys match the expected snake_case names to ensure `@SerialName` mappings are exercised.ee/backend/src/main/kotlin/com/moneat/enterprise/mcp/services/SummaryService.kt (1)
261-269:⚠️ Potential issue | 🟠 MajorUse
suspendRunCatchingfor the suspend metric fetch.The
monitorService.getLatestMetrics(sys.id)call at line 262 is a suspend function inside a suspend context. UsingrunCatchingwill swallowCancellationException; usesuspendRunCatchingfromcom.moneat.utilsinstead to preserve proper cancellation semantics.+import com.moneat.utils.suspendRunCatching ... val hostMetrics = systems.map { sys -> - val metrics = runCatching { + val metrics = suspendRunCatching { monitorService.getLatestMetrics(sys.id) }.getOrNull()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/mcp/services/SummaryService.kt` around lines 261 - 269, The mapping that builds hostMetrics in SummaryService uses runCatching around the suspend call monitorService.getLatestMetrics(sys.id), which can swallow CancellationException; replace runCatching with the suspend-aware helper suspendRunCatching (imported from com.moneat.utils) so cancellation semantics are preserved when fetching metrics inside the coroutine. Locate the block that constructs HostMetricSnapshot (the systems.map { sys -> ... } lambda) and change the error wrapper to suspendRunCatching { monitorService.getLatestMetrics(sys.id) } and keep the subsequent .getOrNull() usage to preserve behavior.backend/src/main/kotlin/com/moneat/summary/services/SummaryService.kt (1)
302-315:⚠️ Potential issue | 🟠 MajorReplace
runCatchingwithsuspendRunCatchingin suspend contexts.
monitorService.getLatestMetrics(...)is a suspend function. UsingrunCatchingin suspend/coroutine functions catchesCancellationException, preventing proper cancellation propagation. UsesuspendRunCatchingfromcom.moneat.utilsinstead to preserveCancellationExceptionbehavior.Affects lines 303, 375, and 407 in
getWeeklyReport()andgetIncidentContext(). Add the import and replace the three occurrences as shown:Suggested change
import com.moneat.uptime.services.UptimeService import com.moneat.utils.recoverOnExpectedFailures +import com.moneat.utils.suspendRunCatching import io.ktor.client.statement.bodyAsText import kotlinx.coroutines.async import kotlinx.coroutines.coroutineScope @@ val hostMetrics = if (hosts.isEmpty()) { emptyList() } else { hosts.mapNotNull { - runCatching { + suspendRunCatching { monitorService.getLatestMetrics(it.id) }.getOrNull() } } @@ - val metrics = runCatching { monitorService.getLatestMetrics(host.id) }.getOrNull() + val metrics = suspendRunCatching { monitorService.getLatestMetrics(host.id) }.getOrNull() if (metrics != null) { hostMetrics = HostMetricsWindow( @@ - val metrics = runCatching { monitorService.getLatestMetrics(host.id) }.getOrNull() + val metrics = suspendRunCatching { monitorService.getLatestMetrics(host.id) }.getOrNull() if (metrics != null) { hostMetrics = HostMetricsWindow(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/summary/services/SummaryService.kt` around lines 302 - 315, In getWeeklyReport() and getIncidentContext(), replace usages of runCatching { monitorService.getLatestMetrics(it.id) } with suspendRunCatching { monitorService.getLatestMetrics(it.id) } so the suspend call in monitorService.getLatestMetrics(...) preserves coroutine cancellation semantics; add the import for com.moneat.utils.suspendRunCatching and update the three occurrences referenced in those methods to use suspendRunCatching instead of runCatching.
🧹 Nitpick comments (1)
backend/src/main/kotlin/com/moneat/monitor/services/MonitorService.kt (1)
339-355: Extract the default history interval constant.The same raw
3600fallback is now repeated in both historical response builders. Pulling it into one constant keeps the default synchronized and avoids a new magic-number footgun.♻️ Suggested refactor
companion object { const val ALERT_SCOPE_GLOBAL = "global" const val ALERT_SCOPE_SYSTEM = "system" const val ALERT_SCOPE_HOST = "host" const val INFRA_LOOKBACK_DAYS = 7 const val MONITOR_HISTORY_CACHE_TTL_SECONDS = 30L + private const val DEFAULT_HISTORY_INTERVAL_SECONDS = 3600 } @@ val host = getHostById(hostId) ?: return@cached HistoricalMetricsResponse( systemId = "", hostId = hostId, from = fromTimestamp, to = toTimestamp, - intervalSeconds = intervalSeconds ?: 3600, + intervalSeconds = intervalSeconds ?: DEFAULT_HISTORY_INTERVAL_SECONDS, dataPoints = emptyList() ) @@ return@cached HistoricalMetricsResponse( systemId = "", hostId = hostId, from = fromTimestamp, to = toTimestamp, - intervalSeconds = intervalSeconds ?: 3600, + intervalSeconds = intervalSeconds ?: DEFAULT_HISTORY_INTERVAL_SECONDS, dataPoints = emptyList() ) @@ val host = getHostById(hostId) ?: return ContainerMetricsResponse( containerName = containerName, from = fromTimestamp, to = toTimestamp, - intervalSeconds = intervalSeconds ?: 3600, + intervalSeconds = intervalSeconds ?: DEFAULT_HISTORY_INTERVAL_SECONDS, dataPoints = emptyList() ) @@ return ContainerMetricsResponse( containerName = containerName, from = fromTimestamp, to = toTimestamp, - intervalSeconds = intervalSeconds ?: 3600, + intervalSeconds = intervalSeconds ?: DEFAULT_HISTORY_INTERVAL_SECONDS, dataPoints = emptyList() )As per coding guidelines, "Use named constants instead of magic numbers (e.g.,
const val MAX_RETRIES = 5instead ofif (count > 5))".Also applies to: 637-651
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/monitor/services/MonitorService.kt` around lines 339 - 355, Replace the repeated magic number 3600 with a single named constant (e.g., DEFAULT_HISTORY_INTERVAL_SECONDS) and use it wherever the HistoricalMetricsResponse fallback interval is set; specifically, add a const val DEFAULT_HISTORY_INTERVAL_SECONDS = 3600 in the MonitorService companion object (or appropriate top-level/constants file) and change the intervalSeconds ?: 3600 occurrences in the HistoricalMetricsResponse constructions (the blocks around getHostById(...), clampRangeToRetention(...), and the other similar block referenced) to intervalSeconds ?: DEFAULT_HISTORY_INTERVAL_SECONDS so the default is centralized and 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 `@backend/src/main/kotlin/com/moneat/events/services/EventService.kt`:
- Around line 628-634: The current fallback SyntheticReplayMetadata
instantiation uses platform "android" and environment "e2e-testing", which will
misclassify real sessions; change the fallback (the SyntheticReplayMetadata
instance named fallback in EventService.kt) to neutral values (e.g. empty
strings or an explicit neutral token such as "unknown") for sdkName, sdkVersion,
platform, environment and release so synthetic replay rows are not attributed to
a specific platform or test environment. Ensure the single fallback variable
used for the standalone replay_video path no longer defaults to
"sentry.java.android" or "e2e-testing".
In `@backend/src/main/kotlin/com/moneat/monitor/services/MonitorService.kt`:
- Around line 339-355: Several return paths in the cached block populate
HistoricalMetricsResponse with systemId = "" causing the host identifier to be
dropped; update all such returns (the ones after the getHostById null check,
after clampRangeToRetention null, and the other mentioned return paths) to set
systemId to the host ID string (use the existing hostId variable or host.id if
the Host model exposes it) so HistoricalMetricsResponse.systemId carries the
host identifier; ensure you change every occurrence where systemId is hardcoded
to "" within the HistoricalMetricsResponse constructions in this method
(references: getHostById, clampRangeToRetention, HistoricalMetricsResponse).
---
Outside diff comments:
In `@backend/src/main/kotlin/com/moneat/summary/services/SummaryService.kt`:
- Around line 302-315: In getWeeklyReport() and getIncidentContext(), replace
usages of runCatching { monitorService.getLatestMetrics(it.id) } with
suspendRunCatching { monitorService.getLatestMetrics(it.id) } so the suspend
call in monitorService.getLatestMetrics(...) preserves coroutine cancellation
semantics; add the import for com.moneat.utils.suspendRunCatching and update the
three occurrences referenced in those methods to use suspendRunCatching instead
of runCatching.
In `@backend/src/test/kotlin/com/moneat/services/EventServiceCoverageTest.kt`:
- Around line 147-158: Replace the self-encoded fixtures with an explicit
snake_case JSON fixture (or add an assertion) so the test validates the external
wire contract: instead of using Json.encodeToString(...) for
SentryEvent/ExceptionInfo/ExceptionValue (the eventJson variable), supply a
hard-coded snake_case JSON string representing the Sentry payload (e.g.
"event_id", "exception": {"values":[{"type","value"}]}, etc.) and use that
fixture for decoding/round-trip checks, or after
Json.encodeToString(SentryEvent(...)) assert the produced JSON object's keys
match the expected snake_case names to ensure `@SerialName` mappings are
exercised.
In
`@ee/backend/src/main/kotlin/com/moneat/enterprise/mcp/services/SummaryService.kt`:
- Around line 261-269: The mapping that builds hostMetrics in SummaryService
uses runCatching around the suspend call
monitorService.getLatestMetrics(sys.id), which can swallow
CancellationException; replace runCatching with the suspend-aware helper
suspendRunCatching (imported from com.moneat.utils) so cancellation semantics
are preserved when fetching metrics inside the coroutine. Locate the block that
constructs HostMetricSnapshot (the systems.map { sys -> ... } lambda) and change
the error wrapper to suspendRunCatching {
monitorService.getLatestMetrics(sys.id) } and keep the subsequent .getOrNull()
usage to preserve behavior.
---
Nitpick comments:
In `@backend/src/main/kotlin/com/moneat/monitor/services/MonitorService.kt`:
- Around line 339-355: Replace the repeated magic number 3600 with a single
named constant (e.g., DEFAULT_HISTORY_INTERVAL_SECONDS) and use it wherever the
HistoricalMetricsResponse fallback interval is set; specifically, add a const
val DEFAULT_HISTORY_INTERVAL_SECONDS = 3600 in the MonitorService companion
object (or appropriate top-level/constants file) and change the intervalSeconds
?: 3600 occurrences in the HistoricalMetricsResponse constructions (the blocks
around getHostById(...), clampRangeToRetention(...), and the other similar block
referenced) to intervalSeconds ?: DEFAULT_HISTORY_INTERVAL_SECONDS so the
default is centralized and consistent.
🪄 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: c2ba56e6-8d64-4023-9fa0-41e85810bc30
📒 Files selected for processing (31)
backend/detekt.ymlbackend/src/main/kotlin/com/moneat/ai/AiModels.ktbackend/src/main/kotlin/com/moneat/ai/OpenAiClient.ktbackend/src/main/kotlin/com/moneat/auth/services/OAuthService.ktbackend/src/main/kotlin/com/moneat/events/models/SentryModels.ktbackend/src/main/kotlin/com/moneat/events/services/DashboardQueryHelper.ktbackend/src/main/kotlin/com/moneat/events/services/EventService.ktbackend/src/main/kotlin/com/moneat/incident/services/IncidentIoProvider.ktbackend/src/main/kotlin/com/moneat/monitor/models/MonitorModels.ktbackend/src/main/kotlin/com/moneat/monitor/routes/MonitorRoutes.ktbackend/src/main/kotlin/com/moneat/monitor/services/MonitorService.ktbackend/src/main/kotlin/com/moneat/notifications/services/DiscordService.ktbackend/src/main/kotlin/com/moneat/notifications/services/SlackService.ktbackend/src/main/kotlin/com/moneat/org/routes/IntegrationRoutes.ktbackend/src/main/kotlin/com/moneat/shared/services/SdkVersionService.ktbackend/src/main/kotlin/com/moneat/summary/services/SummaryService.ktbackend/src/test/kotlin/com/moneat/ai/AiChatServiceTest.ktbackend/src/test/kotlin/com/moneat/models/SentryTimestampParsingTest.ktbackend/src/test/kotlin/com/moneat/routes/MonitorRoutesMockTest.ktbackend/src/test/kotlin/com/moneat/services/DiscordServiceBuildersTest.ktbackend/src/test/kotlin/com/moneat/services/EventServiceCoverageTest.ktbackend/src/test/kotlin/com/moneat/services/EventServiceTest.ktbackend/src/test/kotlin/com/moneat/services/MonitorServiceExtendedTest.ktbackend/src/test/kotlin/com/moneat/services/NotificationFormattingTest.ktbackend/src/test/kotlin/com/moneat/services/NotificationServiceRoutingTest.ktbackend/src/test/kotlin/com/moneat/services/NotificationServiceTest.ktbackend/src/test/kotlin/com/moneat/services/SummaryServiceTest.ktee/backend/detekt.ymlee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/AnthropicProvider.ktee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/OpenAiProvider.ktee/backend/src/main/kotlin/com/moneat/enterprise/mcp/services/SummaryService.kt
AdrianLeeElder
changed the title
AdrianLeeElder
force-pushed
the
fix/issue-324
branch
from
e7fb872 to
f983f5a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/test/kotlin/com/moneat/services/EventServiceTest.kt (1)
43-43: 🛠️ Refactor suggestion | 🟠 MajorReplace wildcard import with explicit import.
The wildcard import
java.util.*violates the coding guideline requiring explicit imports. Based on usage in this file, onlyUUIDis used.Proposed fix
-import java.util.* +import java.util.UUIDAs per coding guidelines: "Never use wildcard imports; always use explicit imports (e.g.,
import com.moneat.models.Userinstead ofimport com.moneat.models.*)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/kotlin/com/moneat/services/EventServiceTest.kt` at line 43, Replace the wildcard import `java.util.*` in EventServiceTest with an explicit import for only the used symbol: `UUID`; update the import statement to `import java.util.UUID` (remove the wildcard), save and reformat to satisfy the coding guideline requiring explicit imports and to prevent accidental wildcard usage elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/src/test/kotlin/com/moneat/services/EventServiceTest.kt`:
- Line 43: Replace the wildcard import `java.util.*` in EventServiceTest with an
explicit import for only the used symbol: `UUID`; update the import statement to
`import java.util.UUID` (remove the wildcard), save and reformat to satisfy the
coding guideline requiring explicit imports and to prevent accidental wildcard
usage elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbfffec0-6903-475b-8535-815b64cd2b71
📒 Files selected for processing (31)
backend/detekt.ymlbackend/src/main/kotlin/com/moneat/ai/AiModels.ktbackend/src/main/kotlin/com/moneat/ai/OpenAiClient.ktbackend/src/main/kotlin/com/moneat/auth/services/OAuthService.ktbackend/src/main/kotlin/com/moneat/events/models/SentryModels.ktbackend/src/main/kotlin/com/moneat/events/services/DashboardQueryHelper.ktbackend/src/main/kotlin/com/moneat/events/services/EventService.ktbackend/src/main/kotlin/com/moneat/incident/services/IncidentIoProvider.ktbackend/src/main/kotlin/com/moneat/monitor/models/MonitorModels.ktbackend/src/main/kotlin/com/moneat/monitor/routes/MonitorRoutes.ktbackend/src/main/kotlin/com/moneat/monitor/services/MonitorService.ktbackend/src/main/kotlin/com/moneat/notifications/services/DiscordService.ktbackend/src/main/kotlin/com/moneat/notifications/services/SlackService.ktbackend/src/main/kotlin/com/moneat/org/routes/IntegrationRoutes.ktbackend/src/main/kotlin/com/moneat/shared/services/SdkVersionService.ktbackend/src/main/kotlin/com/moneat/summary/services/SummaryService.ktbackend/src/test/kotlin/com/moneat/ai/AiChatServiceTest.ktbackend/src/test/kotlin/com/moneat/models/SentryTimestampParsingTest.ktbackend/src/test/kotlin/com/moneat/routes/MonitorRoutesMockTest.ktbackend/src/test/kotlin/com/moneat/services/DiscordServiceBuildersTest.ktbackend/src/test/kotlin/com/moneat/services/EventServiceCoverageTest.ktbackend/src/test/kotlin/com/moneat/services/EventServiceTest.ktbackend/src/test/kotlin/com/moneat/services/MonitorServiceExtendedTest.ktbackend/src/test/kotlin/com/moneat/services/NotificationFormattingTest.ktbackend/src/test/kotlin/com/moneat/services/NotificationServiceRoutingTest.ktbackend/src/test/kotlin/com/moneat/services/NotificationServiceTest.ktbackend/src/test/kotlin/com/moneat/services/SummaryServiceTest.ktee/backend/detekt.ymlee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/AnthropicProvider.ktee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/OpenAiProvider.ktee/backend/src/main/kotlin/com/moneat/enterprise/mcp/services/SummaryService.kt
✅ Files skipped from review due to trivial changes (10)
- backend/detekt.yml
- ee/backend/detekt.yml
- backend/src/main/kotlin/com/moneat/ai/OpenAiClient.kt
- backend/src/test/kotlin/com/moneat/ai/AiChatServiceTest.kt
- backend/src/test/kotlin/com/moneat/models/SentryTimestampParsingTest.kt
- backend/src/test/kotlin/com/moneat/services/NotificationServiceTest.kt
- backend/src/main/kotlin/com/moneat/incident/services/IncidentIoProvider.kt
- backend/src/test/kotlin/com/moneat/services/DiscordServiceBuildersTest.kt
- backend/src/test/kotlin/com/moneat/routes/MonitorRoutesMockTest.kt
- backend/src/main/kotlin/com/moneat/shared/services/SdkVersionService.kt
🚧 Files skipped from review as they are similar to previous changes (13)
- backend/src/main/kotlin/com/moneat/monitor/routes/MonitorRoutes.kt
- backend/src/main/kotlin/com/moneat/org/routes/IntegrationRoutes.kt
- backend/src/test/kotlin/com/moneat/services/NotificationServiceRoutingTest.kt
- backend/src/main/kotlin/com/moneat/auth/services/OAuthService.kt
- ee/backend/src/main/kotlin/com/moneat/enterprise/mcp/services/SummaryService.kt
- backend/src/test/kotlin/com/moneat/services/EventServiceCoverageTest.kt
- backend/src/main/kotlin/com/moneat/notifications/services/DiscordService.kt
- ee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/AnthropicProvider.kt
- backend/src/main/kotlin/com/moneat/ai/AiModels.kt
- ee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/OpenAiProvider.kt
- backend/src/main/kotlin/com/moneat/monitor/models/MonitorModels.kt
- backend/src/main/kotlin/com/moneat/events/services/EventService.kt
- backend/src/main/kotlin/com/moneat/events/models/SentryModels.kt
AdrianLeeElder
force-pushed
the
fix/issue-324
branch
from
50a8e5e to
f40cd81
Compare
AdrianLeeElder
force-pushed
the
fix/issue-324
branch
from
f40cd81 to
39a8ba7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/kotlin/com/moneat/ai/AiModels.kt`:
- Line 170: Replace the magic literal 2048 used as the default for the maxTokens
property with a named constant: define a top-level or companion-object const val
(e.g., DEFAULT_MAX_TOKENS or MAX_TOKENS_DEFAULT = 2048) in the same AiModels.kt
file and then change the property declaration "@SerialName("max_tokens") val
maxTokens: Int = 2048" to use that constant (e.g., = DEFAULT_MAX_TOKENS); keep
the constant visible to the class and add a brief descriptive name per project
naming conventions.
🪄 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: 5e3127a1-c847-4f75-90d6-09c2020956e0
📒 Files selected for processing (31)
backend/detekt.ymlbackend/src/main/kotlin/com/moneat/ai/AiModels.ktbackend/src/main/kotlin/com/moneat/ai/OpenAiClient.ktbackend/src/main/kotlin/com/moneat/auth/services/OAuthService.ktbackend/src/main/kotlin/com/moneat/events/models/SentryModels.ktbackend/src/main/kotlin/com/moneat/events/services/DashboardQueryHelper.ktbackend/src/main/kotlin/com/moneat/events/services/EventService.ktbackend/src/main/kotlin/com/moneat/incident/services/IncidentIoProvider.ktbackend/src/main/kotlin/com/moneat/monitor/models/MonitorModels.ktbackend/src/main/kotlin/com/moneat/monitor/routes/MonitorRoutes.ktbackend/src/main/kotlin/com/moneat/monitor/services/MonitorService.ktbackend/src/main/kotlin/com/moneat/notifications/services/DiscordService.ktbackend/src/main/kotlin/com/moneat/notifications/services/SlackService.ktbackend/src/main/kotlin/com/moneat/org/routes/IntegrationRoutes.ktbackend/src/main/kotlin/com/moneat/shared/services/SdkVersionService.ktbackend/src/main/kotlin/com/moneat/summary/services/SummaryService.ktbackend/src/test/kotlin/com/moneat/ai/AiChatServiceTest.ktbackend/src/test/kotlin/com/moneat/models/SentryTimestampParsingTest.ktbackend/src/test/kotlin/com/moneat/routes/MonitorRoutesMockTest.ktbackend/src/test/kotlin/com/moneat/services/DiscordServiceBuildersTest.ktbackend/src/test/kotlin/com/moneat/services/EventServiceCoverageTest.ktbackend/src/test/kotlin/com/moneat/services/EventServiceTest.ktbackend/src/test/kotlin/com/moneat/services/MonitorServiceExtendedTest.ktbackend/src/test/kotlin/com/moneat/services/NotificationFormattingTest.ktbackend/src/test/kotlin/com/moneat/services/NotificationServiceRoutingTest.ktbackend/src/test/kotlin/com/moneat/services/NotificationServiceTest.ktbackend/src/test/kotlin/com/moneat/services/SummaryServiceTest.ktee/backend/detekt.ymlee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/AnthropicProvider.ktee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/OpenAiProvider.ktee/backend/src/main/kotlin/com/moneat/enterprise/mcp/services/SummaryService.kt
✅ Files skipped from review due to trivial changes (12)
- ee/backend/detekt.yml
- backend/src/main/kotlin/com/moneat/ai/OpenAiClient.kt
- ee/backend/src/main/kotlin/com/moneat/enterprise/mcp/services/SummaryService.kt
- backend/src/test/kotlin/com/moneat/services/DiscordServiceBuildersTest.kt
- backend/src/test/kotlin/com/moneat/models/SentryTimestampParsingTest.kt
- backend/src/test/kotlin/com/moneat/ai/AiChatServiceTest.kt
- backend/src/test/kotlin/com/moneat/services/NotificationFormattingTest.kt
- backend/src/main/kotlin/com/moneat/incident/services/IncidentIoProvider.kt
- backend/src/test/kotlin/com/moneat/services/NotificationServiceRoutingTest.kt
- backend/src/main/kotlin/com/moneat/notifications/services/DiscordService.kt
- backend/src/test/kotlin/com/moneat/routes/MonitorRoutesMockTest.kt
- backend/src/test/kotlin/com/moneat/services/EventServiceCoverageTest.kt
🚧 Files skipped from review as they are similar to previous changes (11)
- backend/detekt.yml
- backend/src/test/kotlin/com/moneat/services/NotificationServiceTest.kt
- backend/src/main/kotlin/com/moneat/monitor/routes/MonitorRoutes.kt
- backend/src/main/kotlin/com/moneat/org/routes/IntegrationRoutes.kt
- backend/src/test/kotlin/com/moneat/services/SummaryServiceTest.kt
- backend/src/test/kotlin/com/moneat/services/MonitorServiceExtendedTest.kt
- ee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/AnthropicProvider.kt
- ee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/OpenAiProvider.kt
- backend/src/main/kotlin/com/moneat/notifications/services/SlackService.kt
- backend/src/main/kotlin/com/moneat/events/services/EventService.kt
- backend/src/main/kotlin/com/moneat/events/models/SentryModels.kt
| val model: String, | ||
| val messages: List<OpenAiMessage>, | ||
| val max_tokens: Int = 2048, | ||
| @SerialName("max_tokens") val maxTokens: Int = 2048, |
There was a problem hiding this comment.
Replace the 2048 default with a named constant.
Line 170 introduces an inline magic number for maxTokens; please lift it to a constant.
♻️ Proposed fix
+private const val DEFAULT_MAX_TOKENS = 2048
+
`@Serializable`
data class OpenAiChatRequest(
val model: String,
val messages: List<OpenAiMessage>,
- `@SerialName`("max_tokens") val maxTokens: Int = 2048,
+ `@SerialName`("max_tokens") val maxTokens: Int = DEFAULT_MAX_TOKENS,
val temperature: Double = 0.3,
`@SerialName`("response_format") val responseFormat: OpenAiResponseFormat? = null
)As per coding guidelines: "Don't use magic numbers; use named constants instead (e.g., const val MAX_RETRIES = 5)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @SerialName("max_tokens") val maxTokens: Int = 2048, | |
| private const val DEFAULT_MAX_TOKENS = 2048 | |
| `@Serializable` | |
| data class OpenAiChatRequest( | |
| val model: String, | |
| val messages: List<OpenAiMessage>, | |
| `@SerialName`("max_tokens") val maxTokens: Int = DEFAULT_MAX_TOKENS, | |
| val temperature: Double = 0.3, | |
| `@SerialName`("response_format") val responseFormat: OpenAiResponseFormat? = null | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/kotlin/com/moneat/ai/AiModels.kt` at line 170, Replace the
magic literal 2048 used as the default for the maxTokens property with a named
constant: define a top-level or companion-object const val (e.g.,
DEFAULT_MAX_TOKENS or MAX_TOKENS_DEFAULT = 2048) in the same AiModels.kt file
and then change the property declaration "@SerialName("max_tokens") val
maxTokens: Int = 2048" to use that constant (e.g., = DEFAULT_MAX_TOKENS); keep
the constant visible to the class and add a brief descriptive name per project
naming conventions.
39a8ba7 to
a624387
Compare
AdrianLeeElder
force-pushed
the
fix/issue-324
branch
from
a624387 to
b73f886
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/OpenAiProvider.kt (1)
24-24: Extract the touched max-token default into a named constant.Line 81 still embeds
4096directly. Since this DTO default is being touched, naming it keeps the provider defaults self-documenting.♻️ Proposed refactor
private val logger = KotlinLogging.logger {} +private const val DEFAULT_OPENAI_MAX_TOKENS = 4096 class OpenAiProvider : LlmProvider { @@ `@Serializable` data class OpenAiRequest( val model: String, val messages: List<OpenAiMsg>, - `@SerialName`("max_tokens") val maxTokens: Int = 4096, + `@SerialName`("max_tokens") val maxTokens: Int = DEFAULT_OPENAI_MAX_TOKENS, val temperature: Double = 0.3, `@SerialName`("response_format") val responseFormat: OpenAiFormat? = null, )As per coding guidelines, "Don't use magic numbers; use named constants instead."
Also applies to: 78-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/OpenAiProvider.kt` at line 24, Extract the magic number 4096 into a named constant (e.g., private const val DEFAULT_MAX_TOKENS = 4096) in OpenAiProvider.kt (place it near the existing logger/private vals), then replace the hard-coded 4096 used as the DTO/default max-tokens value (the embedded literal around where the DTO or parameter maxTokens is defined and the code region touched in the OpenAiProvider class) with that constant so the provider default is self-documenting and consistent across the 78-84 / ~81 code region.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/OpenAiProvider.kt`:
- Line 24: Extract the magic number 4096 into a named constant (e.g., private
const val DEFAULT_MAX_TOKENS = 4096) in OpenAiProvider.kt (place it near the
existing logger/private vals), then replace the hard-coded 4096 used as the
DTO/default max-tokens value (the embedded literal around where the DTO or
parameter maxTokens is defined and the code region touched in the OpenAiProvider
class) with that constant so the provider default is self-documenting and
consistent across the 78-84 / ~81 code region.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 479dd6fb-dedc-4b24-ba38-c651e0afa30d
📒 Files selected for processing (6)
backend/src/main/kotlin/com/moneat/events/services/DashboardQueryHelper.ktbackend/src/main/kotlin/com/moneat/events/services/EventService.ktbackend/src/main/kotlin/com/moneat/monitor/services/MonitorService.ktee/backend/detekt.ymlee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/AnthropicProvider.ktee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/OpenAiProvider.kt
✅ Files skipped from review due to trivial changes (1)
- ee/backend/detekt.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main/kotlin/com/moneat/monitor/services/MonitorService.kt
- ee/backend/src/main/kotlin/com/moneat/enterprise/ai/llm/providers/AnthropicProvider.kt
Contributor
|
❌ Quality Gate failed Issues Measures |
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.
Closes #324
Automated by auto-agent.
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores