Open
Conversation
…st with profiler)
Contributor
📝 WalkthroughWalkthroughLarge cross-repo changes: migrated Okio ByteString usage to kotlinx-io, replaced Ktor OkHttp engine with CIO across modules, added buildConfig support and propagated BuildConfig.VERSION, made ServiceConfiguration fields required, introduced startup time tracking and extended logging/healthcheck behavior. Changes
Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
waltid-services/waltid-verifier-api/build.gradle.kts (1)
38-61:⚠️ Potential issue | 🟡 MinorRemove duplicated CIO client dependency declaration.
Line 61 repeats the same dependency already declared at Line 38. Keep a single declaration.
Proposed fix
- implementation(identityLibs.ktor.client.cio)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@waltid-services/waltid-verifier-api/build.gradle.kts` around lines 38 - 61, The build script declares the same dependency twice — identityLibs.ktor.client.cio appears at the top and again at the bottom; remove the duplicate by deleting the second occurrence so identityLibs.ktor.client.cio is only declared once among the implementation entries (leave other dependencies like identityLibs.ktor.client.logging and kotlinx entries untouched).
🧹 Nitpick comments (2)
waltid-libraries/credentials/waltid-digital-credentials/src/commonMain/kotlin/id/walt/credentials/presentations/formats/JwtVcJsonPresentation.kt (1)
58-61: Document the null/blank audience allowance or add logging for this validation path.The audience check passes when both
audienceandexpectedAudienceare null or blank. While this pattern appears intentional (identical logic in bothJwtVcJsonPresentation.ktandAudienceCheckJwtVcJsonVPPolicy.kt), it differs from the stricter validation ofnonceon the next line and lacks documentation explaining when and why this fallback is needed. Consider adding a log statement when both values are null/blank to clarify this acceptance path during debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@waltid-libraries/credentials/waltid-digital-credentials/src/commonMain/kotlin/id/walt/credentials/presentations/formats/JwtVcJsonPresentation.kt` around lines 58 - 61, The audience check in JwtVcJsonPresentation.presentationRequire allows success when both audience and expectedAudience are null/blank but has no explanation or trace; either add a short comment explaining this permissive fallback (matching AudienceCheckJwtVcJsonVPPolicy) or emit a debug/log message just prior to the presentationRequire when both values are nullOrBlank to record that this acceptance path was taken (reference symbols: presentationRequire, audience, expectedAudience, JwtVcJsonPresentation.kt and AudienceCheckJwtVcJsonVPPolicy.kt). Ensure the log uses the project’s logger at debug/info level and includes the variable values for visibility during debugging.waltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/iso/IsoCertificateHelpers.jvm.kt (1)
21-21: Use direct byte manipulation to ensure stable 20-byte serial output instead ofBigIntegerround-trip.The current implementation occasionally produces 19-byte encodings (~0.5% of cases) due to Java's two's-complement handling in
BigInteger.toByteArray(). This occurs when the first byte is negative (≥0x80) or zero, causing truncation. For ISO certificate serials that should be exactly 20 octets and larger than 63 bits, this variability is problematic.The proposed fix clears the high bit (ensuring positive representation) and sets bit 6 (ensuring ≥64 bits of entropy across all cases), guaranteeing stable 20-byte output that satisfies the specification requirements.
Suggested refactor
internal actual fun generateIsoCompliantX509CertificateSerialNo(): ByteString { val random = SecureRandom() val randomBytes = ByteArray(ISO_CERT_SERIAL_NUMBER_REQUIRED_LENGTH) random.nextBytes(randomBytes) - return ByteString(BigInteger(randomBytes).abs().toByteArray()) + // Force positive (top bit = 0) and ensure high entropy bit is set. + randomBytes[0] = ((randomBytes[0].toInt() and 0x7F) or 0x40).toByte() + return ByteString(randomBytes) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@waltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/iso/IsoCertificateHelpers.jvm.kt` at line 21, Replace the BigInteger round-trip in the serial generation (currently returning ByteString(BigInteger(randomBytes).abs().toByteArray())) with direct 20-byte manipulation: ensure the randomBytes buffer is exactly 20 bytes, clear the high sign bit and set bit 6 on the first byte (e.g., firstByte = (firstByte.toInt() and 0x7F or 0x40).toByte()) so the value is positive and has ≥64 bits of entropy, then construct the ByteString from that fixed 20-byte array; update the code around ByteString(...) and remove the BigInteger(...) conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gradle/libs.versions.toml`:
- Line 100: The toml alias "ktor-client-apache" is misconfigured to point at the
CIO module; update the alias value so the module string is
"io.ktor:ktor-client-apache" (keep version.ref = "ktor" unchanged) so any
imports referencing the alias "ktor-client-apache" (e.g., in build files that
use that alias) will resolve to the Apache Ktor client instead of the CIO
client.
In
`@waltid-libraries/crypto/waltid-crypto/src/jvmMain/kotlin/id/walt/crypto/keys/jwk/JWKKey.jvm.kt`:
- Around line 523-531: The plain var hasSetupBouncyCastle and
ensureBouncyCastleSetup() are racy; replace the mutable flag with a thread-safe
lazy initializer that performs the provider registration exactly once (use
Kotlin's lazy which defaults to LazyThreadSafetyMode.SYNCHRONIZED). Create a val
e.g. bouncyCastleSetup = lazy { if (bouncyCastleProvider !in
Security.getProviders()) Security.addProvider(bouncyCastleProvider); true } and
change ensureBouncyCastleSetup() to trigger bouncyCastleSetup.value (and remove
or stop using hasSetupBouncyCastle). This keeps the bouncyCastleProvider /
Security.addProvider logic the same but ensures single-threaded, visible
initialization.
In
`@waltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/iaca/certificate/IACACertificateInfo.kt`:
- Line 4: The change replaces okio.ByteString with
kotlinx.io.bytestring.ByteString in IACACertificateInfo.kt which is a public API
breaking change affecting the data class properties certificate, serialNumber,
ski, issuer and subject; either declare it as a breaking change in the release
notes or restore compatibility by adding a migration layer (e.g., provide a
typealias or adapter functions to convert between okio.ByteString and
kotlinx.io.bytestring.ByteString and overloads/constructors for
IACACertificateInfo that accept okio.ByteString) so existing consumers compiled
against okio.ByteString continue to work; update documentation and changelog
accordingly to mention the breaking change or the provided compatibility
helpers.
In `@waltid-services/waltid-service-commons-test/build.gradle.kts`:
- Line 21: The global exclusion declared in configurations.all (the block
excluding group "io.ktor" artifact "ktor-client-cio") conflicts with the direct
dependency implementation(identityLibs.ktor.client.cio) so the CIO client never
gets included; either remove the exclusion from the configurations.all block or
delete the direct implementation(identityLibs.ktor.client.cio) line so they no
longer contradict each other—locate the configurations.all { exclude(group =
"io.ktor", module = "ktor-client-cio") } and the
implementation(identityLibs.ktor.client.cio) occurrences and keep only the
appropriate one per the module's intent.
---
Outside diff comments:
In `@waltid-services/waltid-verifier-api/build.gradle.kts`:
- Around line 38-61: The build script declares the same dependency twice —
identityLibs.ktor.client.cio appears at the top and again at the bottom; remove
the duplicate by deleting the second occurrence so identityLibs.ktor.client.cio
is only declared once among the implementation entries (leave other dependencies
like identityLibs.ktor.client.logging and kotlinx entries untouched).
---
Nitpick comments:
In
`@waltid-libraries/credentials/waltid-digital-credentials/src/commonMain/kotlin/id/walt/credentials/presentations/formats/JwtVcJsonPresentation.kt`:
- Around line 58-61: The audience check in
JwtVcJsonPresentation.presentationRequire allows success when both audience and
expectedAudience are null/blank but has no explanation or trace; either add a
short comment explaining this permissive fallback (matching
AudienceCheckJwtVcJsonVPPolicy) or emit a debug/log message just prior to the
presentationRequire when both values are nullOrBlank to record that this
acceptance path was taken (reference symbols: presentationRequire, audience,
expectedAudience, JwtVcJsonPresentation.kt and
AudienceCheckJwtVcJsonVPPolicy.kt). Ensure the log uses the project’s logger at
debug/info level and includes the variable values for visibility during
debugging.
In
`@waltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/iso/IsoCertificateHelpers.jvm.kt`:
- Line 21: Replace the BigInteger round-trip in the serial generation (currently
returning ByteString(BigInteger(randomBytes).abs().toByteArray())) with direct
20-byte manipulation: ensure the randomBytes buffer is exactly 20 bytes, clear
the high sign bit and set bit 6 on the first byte (e.g., firstByte =
(firstByte.toInt() and 0x7F or 0x40).toByte()) so the value is positive and has
≥64 bits of entropy, then construct the ByteString from that fixed 20-byte
array; update the code around ByteString(...) and remove the BigInteger(...)
conversion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9279f8b3-3ba4-4d04-8401-e25fb4ef0827
📒 Files selected for processing (76)
.run/Verifier.run.xmlbuild-logic/build.gradle.ktsbuild-logic/src/main/kotlin/waltid.ktorbackend.gradle.ktsgradle/libs.versions.tomlwaltid-applications/waltid-cli/build.gradle.ktswaltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/flows/AuthFlow.ktwaltid-libraries/credentials/waltid-digital-credentials/src/commonMain/kotlin/id/walt/credentials/presentations/formats/JwtVcJsonPresentation.ktwaltid-libraries/credentials/waltid-verification-policies2-vp/src/commonMain/kotlin/id/walt/policies2/vp/policies/jwt_vc_json/AudienceCheckJwtVcJsonVPPolicy.ktwaltid-libraries/credentials/waltid-verification-policies2/build.gradle.ktswaltid-libraries/credentials/waltid-verification-policies2/src/commonMain/kotlin/id/walt/policies2/vc/policies/VicalPolicy.ktwaltid-libraries/credentials/waltid-w3c-credentials/build.gradle.ktswaltid-libraries/crypto/waltid-crypto/build.gradle.ktswaltid-libraries/crypto/waltid-crypto/src/jvmMain/kotlin/id/walt/crypto/keys/jwk/JWKKey.jvm.ktwaltid-libraries/crypto/waltid-x509/build.gradle.ktswaltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/X509.ktwaltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/IsoCertificateHelpers.ktwaltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/ValidationCommons.ktwaltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/documentsigner/certificate/DocumentSignerDecodedCertificate.ktwaltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/iaca/certificate/IACACertificateInfo.ktwaltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/iaca/certificate/IACADecodedCertificate.ktwaltid-libraries/crypto/waltid-x509/src/commonTest/kotlin/id/walt/x509/iso/IsoTestHarnessAssertions.ktwaltid-libraries/crypto/waltid-x509/src/commonTest/kotlin/id/walt/x509/iso/IsoTestHarnessPlatformHelpers.ktwaltid-libraries/crypto/waltid-x509/src/commonTest/kotlin/id/walt/x509/iso/iaca/IACACertificateInfoVectorsMPTest.ktwaltid-libraries/crypto/waltid-x509/src/commonTest/kotlin/id/walt/x509/iso/vical/AAMVAVicalDecodeValidateIACAEntriesTest.ktwaltid-libraries/crypto/waltid-x509/src/commonTest/kotlin/id/walt/x509/iso/vical/AustroadsVicalDecodeValidateIACAEntriesTest.ktwaltid-libraries/crypto/waltid-x509/src/jsMain/kotlin/id/walt/x509/iso/IsoCertificateHelpers.js.ktwaltid-libraries/crypto/waltid-x509/src/jsTest/kotlin/id/walt/x509/iso/IsoTestHarnessPlatformHelpers.js.ktwaltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/X509Jvm.ktwaltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/JcaX509CertificateHandle.ktwaltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/X509V3Extensions.ktwaltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/iso/IsoCertificateHelpers.jvm.ktwaltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/iso/documentsigner/builder/DocumentSignerCertificateBuilder.jvm.ktwaltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/iso/documentsigner/parser/DocumentSignerCertificateParser.jvm.ktwaltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/iso/iaca/builder/IACACertificateBuilder.jvm.ktwaltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/iso/iaca/certificate/IACACertificateInfo.jvm.ktwaltid-libraries/crypto/waltid-x509/src/jvmMain/kotlin/id/walt/x509/iso/iaca/parser/IACACertificateParser.jvm.ktwaltid-libraries/crypto/waltid-x509/src/jvmTest/kotlin/id/walt/x509/X509Test.ktwaltid-libraries/crypto/waltid-x509/src/jvmTest/kotlin/id/walt/x509/X509V3ExtensionsTest.ktwaltid-libraries/crypto/waltid-x509/src/jvmTest/kotlin/id/walt/x509/iso/IsoTestHarnessPlatformHelpers.jvm.ktwaltid-libraries/crypto/waltid-x509/src/jvmTest/kotlin/id/walt/x509/iso/documentsigner/DocumentSignerCertificateBuilderTest.ktwaltid-libraries/crypto/waltid-x509/src/jvmTest/kotlin/id/walt/x509/iso/iaca/IACACertificateBuilderTest.ktwaltid-libraries/crypto/waltid-x509/src/jvmTest/kotlin/id/walt/x509/iso/iaca/IACACertificateInfoTest.ktwaltid-libraries/protocols/waltid-openid4vc/build.gradle.ktswaltid-libraries/protocols/waltid-openid4vci/build.gradle.ktswaltid-libraries/protocols/waltid-openid4vp-verifier/src/commonMain/kotlin/id/walt/verifier2/handlers/authrequest/Verifier2AuthorizationRequestHandler.ktwaltid-libraries/protocols/waltid-openid4vp-verifier/src/jvmMain/kotlin/id/walt/verifier2/handlers/sessioncreation/VerificationSessionCreator.ktwaltid-libraries/waltid-core-wallet/build.gradle.ktswaltid-libraries/waltid-did/build.gradle.ktswaltid-services/waltid-e2e-tests/src/test/kotlin/WaltidServicesE2ETests.ktwaltid-services/waltid-integration-tests/src/main/kotlin/id/walt/test/integration/environment/InMemoryCommunityStackEnvironment.ktwaltid-services/waltid-integration-tests/src/test/kotlin/WaltidServicesIntegrationTests.ktwaltid-services/waltid-issuer-api/build.gradle.ktswaltid-services/waltid-issuer-api/src/main/kotlin/id/walt/issuer/Main.ktwaltid-services/waltid-issuer-api/src/test/kotlin/id/walt/onboarding/service/IsoMdlOnboardingTests.ktwaltid-services/waltid-openid4vp-conformance-runners/build.gradle.ktswaltid-services/waltid-openid4vp-conformance-runners/src/main/kotlin/id/walt/openid4vp/conformance/testplans/http/ConformanceInterface.ktwaltid-services/waltid-openid4vp-conformance-runners/src/main/kotlin/id/walt/openid4vp/conformance/testplans/runner/TestPlanRunner.ktwaltid-services/waltid-service-commons-test/build.gradle.ktswaltid-services/waltid-service-commons-test/src/main/kotlin/id/walt/commons/testing/E2ETest.ktwaltid-services/waltid-service-commons/build.gradle.ktswaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/ServiceCommons.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/ServiceMain.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/config/statics/BuildConfig.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/config/statics/RunConfiguration.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/logging/LoggingManager.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/logging/setups/SuperTraceLoggingSetup.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/logging/setups/TraceLoggingSetup.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/WebService.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/OpenApiModule.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/ServiceHealthchecksDebugModule.ktwaltid-services/waltid-verifier-api/build.gradle.ktswaltid-services/waltid-verifier-api/src/main/kotlin/id/walt/verifier/Main.ktwaltid-services/waltid-verifier-api2/build.gradle.ktswaltid-services/waltid-verifier-api2/src/main/kotlin/id/walt/verifier2/Main.ktwaltid-services/waltid-wallet-api/build.gradle.ktswaltid-services/waltid-wallet-api/src/main/kotlin/id/walt/webwallet/Main.kt
💤 Files with no reviewable changes (1)
- waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/config/statics/BuildConfig.kt
| ktor-client-okhttp = { module = "io.ktor:ktor-client-okhttp", version.ref = "ktor" } | ||
| #ktor-client-okhttp = { module = "io.ktor:ktor-client-okhttp", version.ref = "ktor" } | ||
| ktor-client-cio = { module = "io.ktor:ktor-client-cio", version.ref = "ktor" } | ||
| ktor-client-apache = { module = "io.ktor:ktor-client-cio", version.ref = "ktor" } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ktor-client-apache is actually used anywhere in the codebase
rg -n 'ktor.client.apache|ktor-client-apache' --type=ktsRepository: walt-id/waltid-identity
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Search for ktor-client-apache usage in gradle files
rg -n 'ktor-client-apache' --type gradleRepository: walt-id/waltid-identity
Length of output: 215
🏁 Script executed:
#!/bin/bash
# Also search more broadly for apache client references
rg -n 'ktor-client-apache|ktor-client-cio' -A 2 -B 2Repository: walt-id/waltid-identity
Length of output: 18517
🏁 Script executed:
#!/bin/bash
# Check the gradle/libs.versions.toml file around line 100
sed -n '95,105p' gradle/libs.versions.tomlRepository: walt-id/waltid-identity
Length of output: 817
🏁 Script executed:
#!/bin/bash
# Search for usage of ktor.client.apache alias in build files
rg -n 'ktor\.client\.apache|ktor\.client\.cio' --glob='*.gradle.kts' -C 3Repository: walt-id/waltid-identity
Length of output: 15481
Critical: ktor-client-apache alias is misconfigured and actively used in production code.
Line 100 has a copy-paste error:
ktor-client-apache = { module = "io.ktor:ktor-client-cio", version.ref = "ktor" }Should be:
ktor-client-apache = { module = "io.ktor:ktor-client-apache", version.ref = "ktor" }This alias is referenced in waltid-libraries/auth/waltid-ktor-authnz/build.gradle.kts:64 and is currently resolving to the CIO client instead of the Apache client, causing the wrong HTTP client implementation to be used.
Proposed fix
-ktor-client-apache = { module = "io.ktor:ktor-client-cio", version.ref = "ktor" }
+ktor-client-apache = { module = "io.ktor:ktor-client-apache", version.ref = "ktor" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gradle/libs.versions.toml` at line 100, The toml alias "ktor-client-apache"
is misconfigured to point at the CIO module; update the alias value so the
module string is "io.ktor:ktor-client-apache" (keep version.ref = "ktor"
unchanged) so any imports referencing the alias "ktor-client-apache" (e.g., in
build files that use that alias) will resolve to the Apache Ktor client instead
of the CIO client.
Comment on lines
+523
to
+531
| var hasSetupBouncyCastle = false | ||
| fun ensureBouncyCastleSetup() { | ||
| if (!hasSetupBouncyCastle) { | ||
| if (bouncyCastleProvider !in Security.getProviders()) { | ||
| Security.addProvider(bouncyCastleProvider) | ||
| } | ||
| hasSetupBouncyCastle = true | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition in BouncyCastle provider setup.
The hasSetupBouncyCastle flag is a plain var without synchronization. Multiple threads creating JWKKey instances concurrently can experience:
- Memory visibility issues — one thread's write may not be seen by another
- TOCTOU — multiple threads pass the
if (!hasSetupBouncyCastle)check simultaneously
While Security.addProvider() likely handles duplicates gracefully, the flag itself needs proper thread-safety guarantees.
🔒 Proposed fix using lazy initialization
- var hasSetupBouncyCastle = false
- fun ensureBouncyCastleSetup() {
- if (!hasSetupBouncyCastle) {
- if (bouncyCastleProvider !in Security.getProviders()) {
- Security.addProvider(bouncyCastleProvider)
- }
- hasSetupBouncyCastle = true
- }
- }
+ private val bouncyCastleSetup: Unit by lazy {
+ if (bouncyCastleProvider !in Security.getProviders()) {
+ Security.addProvider(bouncyCastleProvider)
+ }
+ }
+
+ fun ensureBouncyCastleSetup() {
+ bouncyCastleSetup
+ }Kotlin's lazy is thread-safe by default (LazyThreadSafetyMode.SYNCHRONIZED), ensuring the provider registration runs exactly once.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@waltid-libraries/crypto/waltid-crypto/src/jvmMain/kotlin/id/walt/crypto/keys/jwk/JWKKey.jvm.kt`
around lines 523 - 531, The plain var hasSetupBouncyCastle and
ensureBouncyCastleSetup() are racy; replace the mutable flag with a thread-safe
lazy initializer that performs the provider registration exactly once (use
Kotlin's lazy which defaults to LazyThreadSafetyMode.SYNCHRONIZED). Create a val
e.g. bouncyCastleSetup = lazy { if (bouncyCastleProvider !in
Security.getProviders()) Security.addProvider(bouncyCastleProvider); true } and
change ensureBouncyCastleSetup() to trigger bouncyCastleSetup.value (and remove
or stop using hasSetupBouncyCastle). This keeps the bouncyCastleProvider /
Security.addProvider logic the same but ensures single-threaded, visible
initialization.
|
|
||
| import id.walt.x509.X509CertificateHandle | ||
| import okio.ByteString | ||
| import kotlinx.io.bytestring.ByteString |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify this file changed from Okio to Kotlinx IO on this branch
git diff --unified=0 origin/main...HEAD -- \
waltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/iaca/certificate/IACACertificateInfo.kt
# Show externally visible ByteString-typed properties in the public data class
rg -n -C2 'data class IACACertificateInfo|val\s+\w+:\s+ByteString' \
waltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/iaca/certificate/IACACertificateInfo.ktRepository: walt-id/waltid-identity
Length of output: 109
🏁 Script executed:
cd waltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/iaca/certificate && \
head -100 IACACertificateInfo.ktRepository: walt-id/waltid-identity
Length of output: 948
🏁 Script executed:
# Also search for ByteString imports and usage in this file
rg -n "ByteString|import" \
waltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/iaca/certificate/IACACertificateInfo.ktRepository: walt-id/waltid-identity
Length of output: 410
🏁 Script executed:
# Check if okio.ByteString is used anywhere in the codebase
rg "okio\.ByteString|from okio import.*ByteString" \
waltid-libraries/crypto/waltid-x509/Repository: walt-id/waltid-identity
Length of output: 495
🏁 Script executed:
# Search for any compatibility or migration patterns with ByteString
rg -i "migration|compat|deprecated|okio" \
waltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/iaca/certificate/ \
-A 3 -B 3Repository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
# Check for any compatibility conversions or adapters between the two ByteString types
rg "okio.*ByteString|kotlinx.*bytestring.*ByteString" \
waltid-libraries/crypto/waltid-x509/ \
--type kotlin -A 5 -B 5 | head -150Repository: walt-id/waltid-identity
Length of output: 20696
🏁 Script executed:
# Search for usages of IACACertificateInfo to see how it's instantiated
rg -n "IACACertificateInfo" \
waltid-libraries/crypto/waltid-x509/src/ \
--type kotlinRepository: walt-id/waltid-identity
Length of output: 3778
🏁 Script executed:
# Check if there are extension functions or type aliases converting between ByteString types
rg -n "typealias.*ByteString|fun.*ByteString.*okio|fun.*ByteString.*kotlinx" \
waltid-libraries/crypto/waltid-x509/src/Repository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
# Look at the README to understand what the documented API was
cat waltid-libraries/crypto/waltid-x509/README.md | head -80Repository: walt-id/waltid-identity
Length of output: 3413
🏁 Script executed:
# Check if there's any CHANGELOG, breaking changes documentation, or migration guide
find waltid-libraries/crypto/waltid-x509 -iname "*change*" -o -iname "*breaking*" -o -iname "*migration*" | head -20Repository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
# Check PR/commit context - look for any declarations about breaking changes in comments or docs
rg -i "breaking|BREAKING|deprecated|migration" \
waltid-libraries/crypto/waltid-x509/ \
--type markdown --type kotlin -B 2 -A 2 | head -100Repository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
# Check if there's a version bump or pre-release indicator
cat waltid-libraries/crypto/waltid-x509/build.gradle.kts 2>/dev/null | grep -i version || echo "build.gradle.kts not found"Repository: walt-id/waltid-identity
Length of output: 93
Public API type change is a breaking change and should be declared.
Line 4 changes the ByteString type from okio.ByteString to kotlinx.io.bytestring.ByteString. This breaks source/ABI compatibility for consumers compiled against the Okio variant. The data class exports multiple public properties using this type (certificate, serialNumber, ski, issuer, subject). Mark this explicitly as breaking in release notes or provide a migration/compatibility layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@waltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/iso/iaca/certificate/IACACertificateInfo.kt`
at line 4, The change replaces okio.ByteString with
kotlinx.io.bytestring.ByteString in IACACertificateInfo.kt which is a public API
breaking change affecting the data class properties certificate, serialNumber,
ski, issuer and subject; either declare it as a breaking change in the release
notes or restore compatibility by adding a migration layer (e.g., provide a
typealias or adapter functions to convert between okio.ByteString and
kotlinx.io.bytestring.ByteString and overloads/constructors for
IACACertificateInfo that accept okio.ByteString) so existing consumers compiled
against okio.ByteString continue to work; update documentation and changelog
accordingly to mention the breaking change or the provided compatibility
helpers.
| implementation(kotlin("test")) | ||
| implementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.10.2") | ||
| implementation(identityLibs.ktor.client.okhttp) | ||
| implementation(identityLibs.ktor.client.cio) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify contradiction: same module both excludes and declares ktor-client-cio
rg -n 'exclude\(group = "io\.ktor", module = "ktor-client-cio"\)|identityLibs\.ktor\.client\.cio' -C2Repository: walt-id/waltid-identity
Length of output: 11197
🏁 Script executed:
cat -n waltid-services/waltid-service-commons-test/build.gradle.ktsRepository: walt-id/waltid-identity
Length of output: 1792
Remove the global CIO engine exclusion or the direct dependency declaration—they contradict each other.
The configurations.all { exclude(...) } rule on lines 11–13 prevents the direct implementation of ktor-client-cio on line 21 from being included, rendering it ineffective. In Gradle, global exclusion rules take precedence and cannot be overridden by direct dependency declarations in the same module.
Suggested fix
dependencies {
-
- configurations.all {
- exclude(group = "io.ktor", module = "ktor-client-cio")
- }
-
api(project(":waltid-services:waltid-service-commons"))
api(identityLibs.ktor.server.test.host)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@waltid-services/waltid-service-commons-test/build.gradle.kts` at line 21, The
global exclusion declared in configurations.all (the block excluding group
"io.ktor" artifact "ktor-client-cio") conflicts with the direct dependency
implementation(identityLibs.ktor.client.cio) so the CIO client never gets
included; either remove the exclusion from the configurations.all block or
delete the direct implementation(identityLibs.ktor.client.cio) line so they no
longer contradict each other—locate the configurations.all { exclude(group =
"io.ktor", module = "ktor-client-cio") } and the
implementation(identityLibs.ktor.client.cio) occurrences and keep only the
appropriate one per the module's intent.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/ServiceHealthchecksDebugModule.kt`:
- Around line 61-64: The ServerReady callback in ServiceHealthchecksDebugModule
assumes RunConfiguration.serviceStartupTime is always set and can throw for
alternate entrypoints (e.g., tests); update the monitor.subscribe(ServerReady)
handler to guard access to RunConfiguration.serviceStartupTime (nullable) and
only compute totalStartupTime with Clock.System.now() -
RunConfiguration.serviceStartupTime when the timestamp is present, otherwise log
the same ready message without the startup time (or with a clear "startup time
unavailable" note); touch the subscribe block around KtorStatus.ServerReady so
KtorStatus, RunConfiguration.serviceStartupTime, and the log statement are
updated accordingly.
- Around line 38-42: The KtorStatusChecker.ktorStatus field is accessed from
different threads and lacks safe publication; annotate the var ktorStatus with
`@Volatile` (kotlin.concurrent.Volatile) so updates from Ktor lifecycle handlers
are immediately visible to request threads (ensure the annotation is imported or
fully qualified and placed directly above the var declaration in the
KtorStatusChecker object).
In
`@waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/WebService.kt`:
- Around line 20-25: The constructor's parameter type in WebService expects a
suspending function (module: suspend Application.() -> Unit) while callers
provide non-suspending Application.() -> Unit; add a secondary constructor to
WebService that accepts module: Application.() -> Unit and delegates to the
primary constructor by wrapping the non-suspending module into a suspending one
(e.g., suspend Application.() -> Unit { module() }) so existing callers like
Application.verifierModule, Application.issuerModule,
Application.webWalletModule and E2ETestWebService compile without changes; keep
the original webServiceModule property and logger intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7c79e9b-3418-42f4-85cf-05955c871f5a
📒 Files selected for processing (5)
waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/featureflag/CommonsFeatureCatalog.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/featureflag/FeatureManager.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/featureflag/OptionalFeature.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/WebService.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/ServiceHealthchecksDebugModule.kt
Comment on lines
38
to
42
| object KtorStatusChecker { | ||
|
|
||
| private val log = noCoLogger("Ktor status") | ||
| private val logger = noCoLogger("EnterpriseStatus") | ||
|
|
||
| var ktorStatus: KtorStatus = KtorStatus.Unknown |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd waltid-services/waltid-service-commons && find . -name "ServiceHealthchecksDebugModule.kt" -type fRepository: walt-id/waltid-identity
Length of output: 146
🏁 Script executed:
# Get the full file content to understand the context
cat -n "src/main/kotlin/id/walt/commons/web/modules/ServiceHealthchecksDebugModule.kt"Repository: walt-id/waltid-identity
Length of output: 176
🏁 Script executed:
# Check the Kotlin version in the project
fd "pom.xml|build.gradle|build.gradle.kts" | head -5 | xargs -I {} grep -l "kotlin" {} 2>/dev/null | head -1Repository: walt-id/waltid-identity
Length of output: 95
🏁 Script executed:
find . -name "ServiceHealthchecksDebugModule.kt" -type fRepository: walt-id/waltid-identity
Length of output: 185
🏁 Script executed:
# Also check Kotlin version in main build config
find . -name "build.gradle.kts" -type f | head -3Repository: walt-id/waltid-identity
Length of output: 170
🏁 Script executed:
cat -n "./waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/ServiceHealthchecksDebugModule.kt"Repository: walt-id/waltid-identity
Length of output: 9091
🏁 Script executed:
# Check Kotlin version in the root build.gradle.kts
grep -i "kotlin" ./build.gradle.kts | head -20Repository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
grep -i "kotlin.version\|kotlin =" ./build.gradle.kts | head -10Repository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
# Also check the main service commons build file
cat -n "./waltid-services/waltid-service-commons/build.gradle.kts" | head -50Repository: walt-id/waltid-identity
Length of output: 1895
🏁 Script executed:
# Check the gradle configuration for Kotlin version
cat -n "./build.gradle.kts" | grep -A 5 -B 5 -i "kotlin"Repository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
# Also check gradle/libs.versions.toml which is commonly used
find . -name "libs.versions.toml" -o -name "gradle.properties" | head -5Repository: walt-id/waltid-identity
Length of output: 169
🏁 Script executed:
cat -n "./gradle/libs.versions.toml"Repository: walt-id/waltid-identity
Length of output: 12868
🏁 Script executed:
# Check gradle.properties for Kotlin version
cat -n "./gradle.properties"Repository: walt-id/waltid-identity
Length of output: 1465
🏁 Script executed:
# Check if `@Volatile` annotations are used elsewhere in the codebase
rg "@kotlin.concurrent.Volatile|@kotlin.jvm.Volatile" --type kotlinRepository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
# Check if kotlin.concurrent is available in Kotlin stdlib by searching imports
rg "import.*kotlin.concurrent" --type kotlinRepository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
# Verify the lifecycle callbacks and health check are indeed on different threads
rg "monitor.subscribe|healthcheck|livez" --type kotlin -A 2 -B 2Repository: walt-id/waltid-identity
Length of output: 8206
🏁 Script executed:
web_search
Kotlin 2.3.10 kotlin.concurrent.Volatile annotation availabilityRepository: walt-id/waltid-identity
Length of output: 162
🏁 Script executed:
# Check if the handler/endpoint runs on a different thread than lifecycle callbacks
rg "Dispatcher|runBlocking|launch|async|Thread" waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/ServiceHealthchecksDebugModule.ktRepository: walt-id/waltid-identity
Length of output: 319
🌐 Web query:
Kotlin 2.3 kotlin.concurrent.Volatile annotation support
💡 Result:
Kotlin 2.3 fully supports the kotlin.concurrent.Volatile annotation. It marks the backing field of a var property as volatile, ensuring atomic reads/writes and visibility across threads on JVM and Native targets. This multiplatform annotation was introduced in Kotlin 1.9 as the recommended replacement for the deprecated kotlin.jvm.Volatile (deprecated with warning in 1.9, error since 2.1). No new changes or issues with Volatile appear in Kotlin 2.3 release notes or compatibility guides; it remains stable and available. Usage example: import kotlin.concurrent.Volatile @Volatile var sharedCounter: Int = 0 Note: Only backing field operations are atomic; custom getters/setters may require additional synchronization.
Citations:
- 1: https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.concurrent/-volatile/
- 2: https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.concurrent
- 3: https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.jvm/-volatile/
- 4: https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.concurrent/
🏁 Script executed:
# Check RunConfiguration.serviceStartupTime initialization
cat -n "./waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/config/statics/RunConfiguration.kt"Repository: walt-id/waltid-identity
Length of output: 374
🏁 Script executed:
# Check where serviceStartupTime is initialized
rg "serviceStartupTime" --type kotlin -B 2 -A 2Repository: walt-id/waltid-identity
Length of output: 3162
🏁 Script executed:
# Check the exact import statement needed
rg "kotlin.concurrent" --type kotlin | head -10Repository: walt-id/waltid-identity
Length of output: 49
🏁 Script executed:
# Check if there are any test cases that might reveal threading issues
find . -path "*/test*" -name "*ServiceHealthchecks*" -o -path "*/test*" -name "*Health*" | head -10Repository: walt-id/waltid-identity
Length of output: 49
Add @kotlin.concurrent.Volatile to ensure safe publication of ktorStatus.
This singleton field is written by lifecycle event handlers (running on the Ktor dispatcher) and read by the livez health check handler (running on request threads). Without volatile semantics, writes may not be visible to concurrent readers; the annotation ensures atomic reads/writes and proper memory visibility across threads.
Fix
object KtorStatusChecker {
private val logger = noCoLogger("EnterpriseStatus")
+ `@kotlin.concurrent.Volatile`
var ktorStatus: KtorStatus = KtorStatus.Unknown🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/ServiceHealthchecksDebugModule.kt`
around lines 38 - 42, The KtorStatusChecker.ktorStatus field is accessed from
different threads and lacks safe publication; annotate the var ktorStatus with
`@Volatile` (kotlin.concurrent.Volatile) so updates from Ktor lifecycle handlers
are immediately visible to request threads (ensure the annotation is imported or
fully qualified and placed directly above the var declaration in the
KtorStatusChecker object).
Comment on lines
+61
to
+64
| monitor.subscribe(ServerReady) { | ||
| ktorStatus = KtorStatus.ServerReady | ||
| val totalStartupTime = Clock.System.now() - RunConfiguration.serviceStartupTime | ||
| logger.info("${config.vendor} ${config.name} - Web server ready! [total startup time: ${totalStartupTime.inWholeMilliseconds}ms]") |
Contributor
There was a problem hiding this comment.
Don’t assume serviceStartupTime exists in every bootstrap path.
RunConfiguration.serviceStartupTime is only initialized in ServiceMain.main(), but waltid-services/waltid-service-commons-test/src/main/kotlin/id/walt/commons/testing/E2ETestWebService.kt:1-22 starts WebService.webServiceModule directly. After moving KtorStatusChecker.init() into WebService.kt, this ServerReady callback can now throw in tests or any alternate entrypoint that skips ServiceMain.
🛡️ Guard the optional startup timestamp
monitor.subscribe(ServerReady) {
ktorStatus = KtorStatus.ServerReady
- val totalStartupTime = Clock.System.now() - RunConfiguration.serviceStartupTime
- logger.info("${config.vendor} ${config.name} - Web server ready! [total startup time: ${totalStartupTime.inWholeMilliseconds}ms]")
+ val startupTime = runCatching { RunConfiguration.serviceStartupTime }.getOrNull()
+ if (startupTime != null) {
+ val totalStartupTime = Clock.System.now() - startupTime
+ logger.info("${config.vendor} ${config.name} - Web server ready! [total startup time: ${totalStartupTime.inWholeMilliseconds}ms]")
+ } else {
+ logger.info("${config.vendor} ${config.name} - Web server ready!")
+ }
}📝 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
| monitor.subscribe(ServerReady) { | |
| ktorStatus = KtorStatus.ServerReady | |
| val totalStartupTime = Clock.System.now() - RunConfiguration.serviceStartupTime | |
| logger.info("${config.vendor} ${config.name} - Web server ready! [total startup time: ${totalStartupTime.inWholeMilliseconds}ms]") | |
| monitor.subscribe(ServerReady) { | |
| ktorStatus = KtorStatus.ServerReady | |
| val startupTime = runCatching { RunConfiguration.serviceStartupTime }.getOrNull() | |
| if (startupTime != null) { | |
| val totalStartupTime = Clock.System.now() - startupTime | |
| logger.info("${config.vendor} ${config.name} - Web server ready! [total startup time: ${totalStartupTime.inWholeMilliseconds}ms]") | |
| } else { | |
| logger.info("${config.vendor} ${config.name} - Web server ready!") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/ServiceHealthchecksDebugModule.kt`
around lines 61 - 64, The ServerReady callback in ServiceHealthchecksDebugModule
assumes RunConfiguration.serviceStartupTime is always set and can throw for
alternate entrypoints (e.g., tests); update the monitor.subscribe(ServerReady)
handler to guard access to RunConfiguration.serviceStartupTime (nullable) and
only compute totalStartupTime with Clock.System.now() -
RunConfiguration.serviceStartupTime when the timestamp is present, otherwise log
the same ready message without the startup time (or with a clear "startup time
unavailable" note); touch the subscribe block around KtorStatus.ServerReady so
KtorStatus, RunConfiguration.serviceStartupTime, and the log statement are
updated accordingly.
Comment on lines
20
to
+25
| data class WebService( | ||
| val module: Application.() -> Unit, | ||
| val module: suspend Application.() -> Unit, | ||
| ) { | ||
| private val log = logger("WebService") | ||
|
|
||
| val webServiceModule: Application.() -> Unit = { | ||
| val webServiceModule: suspend Application.() -> Unit = { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the WebService.kt file
find . -type f -name "WebService.kt" | head -20Repository: walt-id/waltid-identity
Length of output: 157
🏁 Script executed:
# Examine WebService.kt around lines 20-25
fd "WebService.kt" -x cat -n {} | head -40Repository: walt-id/waltid-identity
Length of output: 1743
🏁 Script executed:
# Find and examine E2ETestWebService.kt
find . -type f -name "E2ETestWebService.kt" | head -10Repository: walt-id/waltid-identity
Length of output: 173
🏁 Script executed:
# Check for all instantiations of WebService to understand the scope
rg "WebService\(" --type kotlin -B 2 -A 2Repository: walt-id/waltid-identity
Length of output: 5103
🏁 Script executed:
# Get the full WebService.kt file
cat -n waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/WebService.ktRepository: walt-id/waltid-identity
Length of output: 3586
🏁 Script executed:
# Search for Application::verifierModule, issuerModule, webWalletModule definitions
rg "fun Application\.verifierModule|fun Application\.issuerModule|fun Application\.webWalletModule" --type kotlin -B 2 -A 5Repository: walt-id/waltid-identity
Length of output: 3363
🏁 Script executed:
# Also check for suspend Application.() functions
rg "suspend Application\(\)" --type kotlin -B 2 -A 2 | head -60Repository: walt-id/waltid-identity
Length of output: 49
Fix the constructor parameter type mismatch.
The module parameter expects suspend Application.() -> Unit, but all existing callers pass non-suspending function types. Application.verifierModule, Application.issuerModule, and Application.webWalletModule are all non-suspending functions, and E2ETestWebService passes Application.() -> Unit to the constructor. Kotlin treats suspending and non-suspending function types as incompatible, causing compilation failures across all service entrypoints.
Add a secondary constructor accepting the non-suspending variant:
Fix
data class WebService(
val module: suspend Application.() -> Unit,
) {
+ constructor(module: Application.() -> Unit) : this({ module.invoke(this) })
+
private val log = logger("WebService")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/WebService.kt`
around lines 20 - 25, The constructor's parameter type in WebService expects a
suspending function (module: suspend Application.() -> Unit) while callers
provide non-suspending Application.() -> Unit; add a secondary constructor to
WebService that accepts module: Application.() -> Unit and delegates to the
primary constructor by wrapping the non-suspending module into a suspending one
(e.g., suspend Application.() -> Unit { module() }) so existing callers like
Application.verifierModule, Application.issuerModule,
Application.webWalletModule and E2ETestWebService compile without changes; keep
the original webServiceModule property and logger intact.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/FeatureFlagInformationModule.kt (1)
18-19: Incomplete API response:defaultedEnabledis computed but not exposed.The partition at line 63 computes both
defaultedEnabledanddefaultedDisabled, but onlydefaultedDisabledis returned in the API response. This creates an information gap: consumers cannot determine which features are running by default (BaseFeature or OptionalFeature withdefault=true), since these won't appear inenabled(explicitly enabled only) nor indefaultedDisabled.Consider either:
- Including
defaultedEnabledin the response for full visibility- Removing the dead computation if
defaultedEnabledis intentionally omittedOption 1: Include defaultedEnabled in response
`@Serializable` data class FeatureFlagInformations( val enabled: FeatureFlagInformation, val disabled: FeatureFlagInformation, - //val defaultedEnabled: FeatureFlagInformation, + val defaultedEnabled: FeatureFlagInformation, val defaultedDisabled: FeatureFlagInformation, )call.respond( FeatureFlagInformations( enabled = FeatureFlagInformation(enabled), disabled = FeatureFlagInformation(disabled), - //defaultedEnabled = FeatureFlagInformation(defaultedEnabled), + defaultedEnabled = FeatureFlagInformation(defaultedEnabled), defaultedDisabled = FeatureFlagInformation(defaultedDisabled) ) )Option 2: Remove unused computation if intentionally omitted
-val (defaultedEnabled, defaultedDisabled) = defaulted.entries.partition { it.value.shouldDefaultEnable() } - .associateBoth { (k, v) -> k to v.description } +val defaultedDisabled = defaulted.entries + .filterNot { it.value.shouldDefaultEnable() } + .associate { (k, v) -> k to v.description }Also applies to: 63-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/FeatureFlagInformationModule.kt` around lines 18 - 19, The code in FeatureFlagInformationModule computes both defaultedEnabled and defaultedDisabled but only returns defaultedDisabled, leaving defaultedEnabled unused; update the API response to include defaultedEnabled (i.e., add the defaultedEnabled field to whatever response object/JSON is constructed in the module) so consumers can see features that are enabled by default, and ensure any response data class or serializer is updated accordingly (refer to defaultedEnabled, defaultedDisabled and the response construction in FeatureFlagInformationModule).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/FeatureFlagInformationModule.kt`:
- Around line 18-19: The code in FeatureFlagInformationModule computes both
defaultedEnabled and defaultedDisabled but only returns defaultedDisabled,
leaving defaultedEnabled unused; update the API response to include
defaultedEnabled (i.e., add the defaultedEnabled field to whatever response
object/JSON is constructed in the module) so consumers can see features that are
enabled by default, and ensure any response data class or serializer is updated
accordingly (refer to defaultedEnabled, defaultedDisabled and the response
construction in FeatureFlagInformationModule).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f564623-8396-40cd-a43f-d31473526c7f
📒 Files selected for processing (3)
waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/featureflag/AbstractFeature.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/featureflag/FeatureManager.ktwaltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/web/modules/FeatureFlagInformationModule.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- waltid-services/waltid-service-commons/src/main/kotlin/id/walt/commons/featureflag/FeatureManager.kt
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related PRs:
https://github.com/walt-id/waltid-identity-enterprise/pull/396
https://github.com/walt-id/waltid-unified-build/pull/38
Type of Change
Checklist
Breaking
Summary by CodeRabbit
New Features
Bug Fixes
Improvements