chore: add missing files from kmp-project-template #1951
Conversation
📝 WalkthroughWalkthroughAdds numerous Gradle convention plugins and build tooling, a comprehensive keystore/secrets management subsystem (tasks, parsers, validators, environment overrides), multiplatform network primitives and Ktor client implementations, platform entry points/resources, CI workflow for cache cleanup, and many tooling/helpers and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / CI
participant Gradle as Gradle
participant KeystoreTask as KeystoreGenerationTask
participant Keytool as keytool (system)
participant FS as FileSystem
participant SecretsTask as SecretsEnvUpdateTask
participant ConfigTask as ConfigurationFileUpdatesTask
Note over Dev,Gradle: Keystore generation + secrets update + config update flow
Dev->>Gradle: trigger keystore workflow (task invocation)
Gradle->>KeystoreTask: execute generateKeystores()
KeystoreTask->>Keytool: run keytool commands
Keytool-->>KeystoreTask: return status/results
KeystoreTask->>FS: write keystore files (original/upload)
KeystoreTask-->>Gradle: report generation summary
Gradle->>SecretsTask: execute updateSecretsEnv() with keystore outputs
SecretsTask->>FS: read existing secrets.env (parse heredoc)
SecretsTask->>FS: base64-encode keystores and merge, write secrets.env (backup if enabled)
SecretsTask-->>Gradle: return update summary
Gradle->>ConfigTask: execute configuration updates (fastlane/gradle)
ConfigTask->>FS: read/backup target files, apply replacements or create files
ConfigTask-->>Gradle: final status
Note over KeystoreTask,SecretsTask: EnvironmentOverrideHandler may inject CI env overrides during config resolution
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
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 |
biplab1
changed the title
biplab1
changed the title
biplab1
force-pushed
the
MW-328-add-missing-template-modules-files
branch
from
010c032 to
4bc40f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI Agents
In @build-logic/convention/src/main/kotlin/GitHooksConventionPlugin.kt:
- Around line 36-40: The Windows branch is using an incorrect attrib command to
set executable bits; instead, make the permission-setting step only run on Unix
by removing or replacing the else branch that calls commandLine("cmd", "/c",
"attrib", ...). Update the isLinuxOrMacOs() conditional so that the Exec task
invokes commandLine("chmod", "-R", "+x", ".git/hooks/") only when true and
otherwise either do nothing or perform a harmless verification (e.g., check/copy
files exist), removing the incorrect attrib call; adjust the Exec task
configuration where commandLine(...) is used to reflect this change.
- Around line 49-55: The current project.afterEvaluate block makes many build
tasks (including clean) always depend on installGitHooks; change this to only
wire installGitHooks to preBuild (remove "clean" and the broad list) and/or make
the installGitHooks task itself incremental by adding an up-to-date check
(compare existing hooks vs desired content or a marker file) so it no-ops when
nothing changed; update the matching/configureEach invocation to reference only
"preBuild" (or make the dependency opt-in via a project property) and implement
the up-to-date logic inside the installGitHooks task implementation to avoid
re-installing hooks on every build or in CI.
- Line 38: In GitHooksConventionPlugin (locate the occurrence of the stray
"}else {" token), fix the spacing around the else keyword by replacing "}else {"
with "} else {" so the else has a leading space after the closing brace and
proper spacing before the opening brace.
In
@build-logic/convention/src/main/kotlin/KeystoreManagementConventionPlugin.kt:
- Around line 40-43: The code eagerly realizes the generation task by calling
generateKeystoresTask.get() inside the registration of updateSecretsEnv; change
this to wire the dependency lazily by passing the TaskProvider (or a mapped
Provider) instead of calling .get() — e.g., remove .get() and use
generateKeystoresTask or generateKeystoresTask.flatMap { ... } (and likewise
avoid forcing keystoreExtension.secretsConfig.get() if present) when calling
SecretsEnvUpdateTask.createFromKeystoreGeneration so the updateSecretsEnv task
remains registered without realizing the generateKeystoresTask during
configuration.
- Around line 24-31: The task registration eagerly calls
keystoreExtension.secretsConfig.get() via
KeystoreGenerationTask.createWithSecretsConfig(this,
keystoreExtension.secretsConfig.get()), which forces provider evaluation during
configuration; instead remove that .get() call and either (a) wire the task
properties lazily by passing the Provider into the task (use
keystoreConfig.set(keystoreExtension.keystoreConfig) and
secretsConfig.set(keystoreExtension.secretsConfig) as you already do) and move
the createWithSecretsConfig logic into KeystoreGenerationTask's @TaskAction so
it reads secretsConfig.get() at execution time, or (b) defer execution with
generateKeystoresTask.configure { it.doFirst {
KeystoreGenerationTask.createWithSecretsConfig(it,
keystoreExtension.secretsConfig.get()) } } so the provider is resolved at
runtime; ensure all references use the task property names (keystoreConfig,
secretsConfig) and remove any direct .get() in the configuration block.
In @build-logic/convention/src/main/kotlin/org/convention/AndroidCompose.kt:
- Around line 46-61: The helper relativeToRootProject is using flatMap and
captures the provider value `it` but never uses it, so replace flatMap with map
(or otherwise stop capturing `it`) and compute the directory directly: change
Provider<*>.relativeToRootProject(dir: String) to use map {
rootProject.layout.buildDirectory.dir(projectDir.toRelativeString(rootDir)).dir(dir)
} (or document that the input provider is intentionally discarded), and remove
the unused `it` parameter to avoid the misleading capture.
In @build-logic/convention/src/main/kotlin/org/convention/Badging.kt:
- Around line 116-118: Replace the artifact reference used when wiring APK
outputs: change the call that sets apk via
variant.artifacts.get(SingleArtifact.APK_FROM_BUNDLE) to use SingleArtifact.APK
instead so apk.set(...) points to regular APK outputs; update the reference in
the apk.set(...) line where variant.artifacts.get(...) is used to use
SingleArtifact.APK in place of SingleArtifact.APK_FROM_BUNDLE.
- Around line 30-36: Finish the incomplete KDoc sentence in Badging.kt to
explain the incremental behavior of the task: update the trailing sentence so it
reads something like "This task is also incremental, meaning that if only some
inputs have changed, the task will run only for those changes rather than
reprocessing everything," ensuring the documentation for the Badging task fully
describes incremental execution semantics.
In @build-logic/convention/src/main/kotlin/org/convention/Detekt.kt:
- Around line 17-19: The hard-coded project path in
tasks.named<Detekt>("detekt") calling
mustRunAfter(":cmp-android:dependencyGuard") makes the convention plugin
non-reusable; replace the direct path with a safe conditional lookup (e.g., use
tasks.findByPath for ":cmp-android:dependencyGuard" and only call mustRunAfter
when that task exists) or make the dependency task name configurable via a
plugin extension and use that value instead of the literal; update the block
around tasks.named<Detekt>("detekt") to perform the conditional check (or read
the extension property) before invoking mustRunAfter so the plugin no longer
assumes a specific project structure.
In @build-logic/convention/src/main/kotlin/org/convention/Jacoco.kt:
- Around line 61-66: The Project.configureJacoco code calls
libs.findVersion("jacoco").get() which will throw if the version key is missing;
add a "jacoco" entry to the version catalog (gradle/libs.versions.toml) under
the [versions] section (e.g. jacoco = "0.8.8" or your desired version) so
libs.findVersion("jacoco") returns a value for configure<JacocoPluginExtension>
in configureJacoco.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/BaseKeystoreTask.kt:
- Around line 81-96: checkKeytoolAvailable currently starts a
ProcessBuilder("keytool", "-help") then calls waitFor() without consuming
stdout/stderr or applying a timeout, which can hang; fix by consuming or
redirecting the process streams and using a timed wait: in
checkKeytoolAvailable, after starting the process (ProcessBuilder.start()),
either call process.inputStream and process.errorStream readers on background
threads or use processBuilder.redirectOutput(ProcessBuilder.Redirect.DISCARD)
and redirectError(ProcessBuilder.Redirect.DISCARD) before start(), then call
process.waitFor(timeoutMillis, TimeUnit.MILLISECONDS) (or waitFor with a
reasonable timeout) and if it returns false destroy the process and log an
error; also catch InterruptedException and ensure streams are closed and
process.destroyForcibly() is called when needed, and include the exit code in
the log when available.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationFileUpdatesTask.kt:
- Around line 156-184: Add the fastlane-config directory to .gitignore to
prevent committing plaintext credentials: update the repo .gitignore to include
"fastlane-config/" (or "fastlane-config/**") so files written by
createNewFastlaneConfig (which writes the Android Fastlane config via
configFile.writeText) are ignored; commit the .gitignore change and verify that
fastlane-config/android_config.rb is no longer tracked (remove from git index if
already committed using git rm --cached) before merging.
- Around line 167-172: The FIREBASE_CONFIG map in ConfigurationFileUpdatesTask
(symbol: FIREBASE_CONFIG) contains hardcoded Firebase app IDs that look real;
change those values to clearly fake placeholders (e.g.,
"YOUR_FIREBASE_APP_ID_HERE" and "YOUR_FIREBASE_DEMO_APP_ID_HERE") or wire them
to configurable sources (project properties / environment variables) so the task
reads keys instead of embedding real IDs; update the map entries
firebase_prod_app_id and firebase_demo_app_id accordingly and add a brief
comment explaining they must be provided via configuration when running the
task.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationValidator.kt:
- Around line 214-218: validateFilePaths currently has a side effect: it calls
config.keystoreDir.mkdirs(), which creates directories; change it to a pure
read-only validation by removing the mkdirs() call and instead check existence
and writability only (e.g., use config.keystoreDir.exists() and
config.keystoreDir.isDirectory(), or
java.nio.file.Files.exists/Files.isDirectory and Files.isWritable on the path or
its nearest existing parent) and add an error if the directory doesn't exist or
cannot be written to, keeping the checks confined to the validateFilePaths
function and referencing config.keystoreDir and validateFilePaths when updating
the logic.
- Around line 42-48: The guard on config.validity contains an unreachable
branch: after checking config.validity <= 0 the subsequent else if
(config.validity < 1) can never be true for integers; update
ConfigurationValidator to replace that unreachable condition with a meaningful
short-validity threshold (for example use config.validity <= 2 to warn about 1–2
year validity), keeping the existing errors.add and warnings.add flows
(references: config.validity, errors, warnings in ConfigurationValidator).
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/EnvironmentOverrideHandler.kt:
- Around line 296-303: The constants in EnvironmentOverrideHandler
(ORIGINAL_KEYSTORE_PASSWORD, ORIGINAL_KEYSTORE_ALIAS,
ORIGINAL_KEYSTORE_ALIAS_PASSWORD, UPLOAD_KEYSTORE_PASSWORD,
UPLOAD_KEYSTORE_ALIAS, UPLOAD_KEYSTORE_ALIAS_PASSWORD) have mismatched
identifier names vs. string values (some values include "_FILE_"). Fix by making
the identifiers and their string values consistent: either rename the constants
to include "FILE" (e.g., ORIGINAL_KEYSTORE_FILE_PASSWORD,
UPLOAD_KEYSTORE_FILE_PASSWORD) or change the string values to remove "_FILE_" so
they match the current constant names; update all referenced usages to the new
names to keep callers consistent.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreConfig.kt:
- Around line 10-12: KeystoreConfig currently uses weak hardcoded defaults for
keystorePassword and keyPassword ("android"); remove those defaults so callers
must supply secure values (i.e., change the parameters keystorePassword and
keyPassword in KeystoreConfig to have no default), and optionally keyAlias if
you want to avoid any implicit value; additionally add a runtime check in
KeystoreConfig initialization that detects if either password equals the legacy
debug value ("android") and either logs a prominent warning or throws an error
to prevent accidental use in production (use the class/constructor
KeystoreConfig and the properties keystorePassword, keyPassword, keyAlias to
locate where to change and add the check).
- Line 51: KeystoreConfig currently flags keySize < 1024 as an error but
ConfigurationValidator.validateKeyAlgorithm() requires RSA keys to be >= 2048;
update KeystoreConfig's validation (the keySize check in KeystoreConfig) to
enforce the same minimum (use 2048) or make the check algorithm-aware (e.g., if
algorithm is "RSA" require keySize >= 2048) so both validations are consistent
with ConfigurationValidator.validateKeyAlgorithm().
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreGenerationTask.kt:
- Around line 196-199: The log currently prints the full command (including
sensitive `-storepass`/`-keypass` values) in executeKeytoolCommand; change the
logging to mask sensitive arguments by constructing a maskedCommand from the
command List<String> before logInfo: treat both "--flag=value" and separate
flag/value forms, replace the password value with a fixed mask (e.g. "****")
whenever an element startsWith("-storepass") or startsWith("-keypass") or
immediately follows those flags, and then call logInfo with the maskedCommand
instead of the raw command to avoid exposing keystore passwords.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvParser.kt:
- Around line 96-98: The else branch that adds "Invalid multiline syntax" is
unreachable because when line.contains("<<") is true, split("<<", limit = 2)
always produces parts.size == 2; remove the dead else branch inside the
multiline handling in SecretsEnvParser (the block that checks
line.contains("<<") and assigns parts) and instead validate the second part
(parts[1]) for being empty or only whitespace — if parts[1].isBlank() then call
errors.add with a clear message; update any related tests to expect this
validation instead of relying on the unreachable branch.
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvUpdateTask.kt:
- Around line 396-406: The header-write logic currently checks
secretsFile.exists() after calling secretsFile.printWriter() which
creates/truncates the file, so the header condition never triggers; move the
existence check before opening the writer (i.e., evaluate !secretsFile.exists()
|| secrets.comments.isEmpty() using secretsFile.exists() prior to calling
secretsFile.printWriter().use { writer -> ... }) and only then open the writer
to write the header and body; keep the rest of the write logic (the
printWriter().use block and the header lines) unchanged.
In @build-logic/convention/src/main/kotlin/org/convention/KotlinAndroid.kt:
- Around line 28-34: Update the misleading comment above the compileOptions
block: it currently references "Up to Java 11 APIs" but the code sets
sourceCompatibility and targetCompatibility to JavaVersion.VERSION_17 and
enables isCoreLibraryDesugaringEnabled; change the comment to state that Java 17
language level and desugaring are configured (and clarify what desugaring covers
for Java 17, if needed) so it accurately reflects the settings in
compileOptions.
- Around line 48-53: Update the misleading comment above the
extensions.configure<JavaPluginExtension> block: remove the Android/desugaring
reference and state that sourceCompatibility and targetCompatibility are set to
Java 17 (JavaVersion.VERSION_17) for non-Android JVM projects; ensure the
comment succinctly reflects Java 17 configuration and its applicability (e.g.,
"Configure Java 17 (sourceCompatibility and targetCompatibility) for JVM
projects — not related to Android desugaring").
In @build-logic/convention/src/main/kotlin/org/convention/PrintTestApks.kt:
- Around line 88-91: Update the RuntimeException messages thrown after loading
APKs to include contextual details: when
builtArtifactsLoader.get().load(apkFolder.get()) returns null, throw an
exception that includes the apkFolder path (e.g.,
apkFolder.get().asFile.toString()) and the variant name if available (e.g.,
variantName or variant.toString()); similarly, when checking
builtArtifacts.elements.size != 1, include the actual size, the apkFolder path
and the variant identifier in the error message so the exceptions provide
actionable debugging info; modify the two throw RuntimeException(...) calls
accordingly near builtArtifactsLoader.get().load(...) and the size check.
In @build-logic/convention/src/main/kotlin/org/convention/Spotless.kt:
- Line 6: Update the ktlint version constant to remove the vulnerable old
release: change the const val ktlintVersion (in Spotless.kt) from "1.0.1" to
"1.8.0", then refresh dependencies/lockfiles and run the build/spotless checks
to ensure compatibility and resolve any formatter rule changes from the upgrade.
In @cmp-android/prodRelease-badging.txt:
- Line 1: The manifest badging shows versionCode='1' while
versionName='2026.1.1-beta.0.6'; update the integer versionCode to an
incremented value that reflects this new release (ensure versionCode is a
monotonically increasing integer used by the Play Store), e.g. bump versionCode
from 1 to an appropriate higher integer that corresponds to this release, and
make the same change in the canonical source of truth for app versions (e.g.,
the module's build.gradle / android.defaultConfig versionCode or wherever
versionCode is injected) so versionCode and versionName remain consistent across
builds.
In @cmp-desktop/src/jvmMain/kotlin/main.kt:
- Around line 31-34: Move the initKoin() call out of the application composable
scope so Koin is started exactly once at app startup: call initKoin() at the
start of main() before entering the application { ... } block (not inside any
composable or UI lambda), keeping the application { ... } block purely for UI;
update main() to invoke initKoin() first and then call application { /* UI */ }
to avoid reinitialization.
- Line 43: The window title is a placeholder ("DesktopApp") in main.kt; update
the title assignment (title = "DesktopApp") to a descriptive app name (e.g.,
"MifosPay" or "Mifos Mobile Wallet") so the UI shows a meaningful name; locate
the title property in the main window creation code (the title = ... line) and
replace the string accordingly.
In
@cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/iosApp.xcscheme:
- Around line 1-32: The scheme file for the iosApp (LiteDo.app, BlueprintName
"iosApp") is located under xcuserdata which is user-specific; either make the
scheme shared so it moves to xcshareddata by opening Xcode, selecting the iosApp
scheme, choosing "Manage Schemes..." and checking "Shared" (this will relocate
the scheme out of xcuserdata), or remove this xcuserdata/iosApp.xcscheme from
version control and add the appropriate xcuserdata/ path to .gitignore if the
scheme is truly user-specific.
- Line 11: Update the Xcode scheme's BuildableName entries from "LiteDo.app" to
"MifosPay.app": find the two occurrences of BuildableName = "LiteDo.app" in the
iosApp.xcscheme and replace each with BuildableName = "MifosPay.app" so the
scheme matches the product name configured in Config.xcconfig and the
org.mifospay bundle identifier.
In
@cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/xcschememanagement.plist:
- Around line 1-14: Update .gitignore to explicitly ignore developer xcuserdata
by adding patterns for cmp-ios/iosApp.xcodeproj/xcuserdata/ and
cmp-ios/iosApp.xcworkspace/xcuserdata/ so these per-developer Xcode files aren’t
tracked; then remove the currently tracked xcuserdata files (those under
cmp-ios/.../xcuserdata, e.g., entries for skniyajali, kapilmaurya,
hekmatullahamin) from git’s index and commit that change to stop them from
appearing in future commits; optionally, if historical removal is required,
purge those paths from the repository history using your repo’s history-rewrite
tool.
In
@cmp-ios/iosApp.xcworkspace/xcuserdata/skniyajali.xcuserdatad/xcschemes/xcschememanagement.plist:
- Around line 1-5: The committed user-specific Xcode workspace file
xcschememanagement.plist located under xcuserdata/skniyajali.xcuserdatad should
be removed and excluded: remove the file from the repo index (e.g., git rm
--cached) and commit the deletion, then update .gitignore to ignore xcuserdata
directories for both .xcodeproj and .xcworkspace (add patterns like
*.xcodeproj/xcuserdata/ and *.xcworkspace/xcuserdata/ or equivalent), verify no
other xcuserdata files remain tracked, and commit the .gitignore change.
In
@core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt:
- Line 80: In ResultSuspendConverterFactory (locate the println calls in the
suspend conversion logic inside the class/method handling Result wrapping),
replace all println("Failure: " + result.throwable.message) uses with a proper
logger call from your chosen logging abstraction (e.g., kermit or napier);
obtain or inject a logger instance (e.g., a private val logger or constructor
parameter) and call logger.error/diagnostic with a descriptive message and
include the throwable so the stacktrace is captured (for example:
logger.error("Failure converting Result", throwable) or equivalent in your
logging API), and apply the same replacement for the other occurrences
referenced in the file.
- Line 88: In ResultSuspendConverterFactory's status handling (the when on
status), the HTTP success range currently uses 200..209 which incorrectly treats
2xx codes >=210 as errors; change the branch to use 200..299 so all RFC 7231
success codes (e.g., 201, 204, 206) are handled by the success path (i.e.,
update the condition from in 200..209 to in 200..299 and keep the existing body
that reads result.response.body(successType) and returns NetworkResult.Success
or maps serialization exceptions to NetworkError.SERIALIZATION).
- Around line 57-58: Update the KDoc on ResultSuspendConverterFactory so it
references NetworkResult (the actual type checked by the implementation) instead
of Result; change the sentence that currently says "`Result`" to
"`NetworkResult`" (and adjust surrounding wording/backticks as needed) so the
doc matches the converter's behavior and types used by suspendResponseConverter.
- Around line 40-46: The example in the KDoc uses the wrong generic types;
update the sample ApiService declaration so the suspend function getUsers()
returns NetworkResult<List<User>, NetworkError> instead of Result<List<User>,
RemoteError>, i.e., replace the type names in the documentation example to match
the codebase's NetworkResult and NetworkError types.
In
@core-base/network/src/commonMain/kotlin/template/core/base/network/KtorHttpClient.kt:
- Around line 118-126: Digest auth configuration is missing proactive credential
sending; update the digest auth block (where install(Auth) { digest {
credentials { digestCredentialsProvider() } } } ) to include a
sendWithoutRequest { request -> request.url.host in authRequiredUrl } clause
similar to the Bearer/Basic providers so digest credentials are sent proactively
to hosts in authRequiredUrl.
In
@core-base/network/src/jsMain/kotlin/template/core/base/network/KtorHttpClient.js.kt:
- Line 10: The package declaration in KtorHttpClient.js.kt uses the placeholder
package template.core.base.network; rename it to org.mifospay.core.base.network
to match the project's package structure, and apply the same change across all
files in the core-base module (ui, platform, network) so package declarations
consistently use org.mifospay.core.base.*; ensure any imports, references, and
module metadata that rely on the old package are updated accordingly (e.g.,
KtorHttpClient and other classes in core-base).
🟠 Major comments (21)
cmp-android/prodRelease-badging.txt-1-1 (1)
1-1: Address the versionCode/versionName mismatch.The versionCode is set to '1' while versionName is '2026.1.1-beta.0.6'. This inconsistency suggests version tracking may not be properly maintained. Typically, versionCode increments with each release. If this is a production build, versionCode should be incremented accordingly (e.g., to match release numbers).
core-base/analytics/src/commonMain/kotlin/template/core/base/analytics/AnalyticsEvent.kt-258-279 (1)
258-279: Documentation contradicts implementation: validation vs. sanitization.The implementation silently sanitizes invalid inputs instead of throwing exceptions as documented:
- Blank keys → replaced with
"unknown_param"(line 272)- Keys > 40 chars → truncated (line 271)
- Values > 100 chars → truncated (line 274)
However, the KDoc at lines 238, 249-253 explicitly states:
"@throws IllegalArgumentException if validation constraints are violated"
Impact:
- Developers expecting exceptions won't receive them, potentially masking bugs
- Blank keys silently becoming
"unknown_param"obscures data quality issues- Silent truncation may cause unnoticed data loss in analytics
Recommendation: Choose one approach consistently:
- Match documentation (throw exceptions for invalid input) — better for catching bugs early, OR
- Match implementation (update docs to reflect sanitization) — better for resilience but risks hiding issues
Option 1: Throw exceptions to match documentation
operator fun invoke(key: String, value: String): Param { - val safeKey = key - .takeIf { it.isNotBlank() } - ?.take(MAX_KEY_LENGTH) - ?: FALLBACK_KEY - - val safeValue = value.take(MAX_VALUE_LENGTH) - - return Param(safeKey, safeValue) + require(key.isNotBlank()) { "Parameter key must not be blank" } + require(key.length <= MAX_KEY_LENGTH) { + "Parameter key exceeds maximum length of $MAX_KEY_LENGTH characters" + } + require(value.length <= MAX_VALUE_LENGTH) { + "Parameter value exceeds maximum length of $MAX_VALUE_LENGTH characters" + } + + return Param(key, value) }Option 2: Update documentation to reflect sanitization behavior
Update the KDoc to remove
@throws IllegalArgumentExceptionreferences (lines 238, 249-253) and add:/** * Represents a validated analytics parameter with automatic constraint sanitization. * * This data class encapsulates a key-value pair for analytics events with built-in * sanitization that enforces analytics platform constraints. The sanitization occurs * during object construction to ensure data integrity. * * **Sanitization Rules:** * - Blank keys are replaced with "unknown_param" * - Keys are truncated to ≤ 40 characters (Firebase Analytics constraint) * - Values are truncated to ≤ 100 characters (Firebase Analytics constraint) * ... */Also update withParam/withParams documentation at lines 70, 97, 128 to remove exception throwing claims.
cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/iosApp.xcscheme-1-32 (1)
1-32: Move scheme to shared location or remove from version control.This scheme file is in the
xcuserdata/directory, making it user-specific. If this scheme needs to be shared across the team:
- In Xcode, select the scheme from the scheme dropdown
- Choose "Manage Schemes..."
- Check the "Shared" checkbox for the iosApp scheme
This will move the scheme file to
xcshareddata/instead ofxcuserdata/, making it appropriate for version control. If the scheme is truly user-specific and doesn't need to be shared, it should be excluded from the repository via.gitignore.cmp-ios/iosApp.xcworkspace/xcuserdata/skniyajali.xcuserdatad/xcschemes/xcschememanagement.plist-1-5 (1)
1-5: Remove user-specific Xcode workspace files from version control.This file is also located in a
xcuserdata/skniyajali.xcuserdatad/directory, making it user-specific configuration that should not be committed. Even though it contains only an empty dictionary, it will still cause unnecessary noise and potential conflicts in the repository.Ensure that
xcuserdata/patterns are properly excluded in.gitignorefor both.xcodeprojand.xcworkspacedirectories.cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/iosApp.xcscheme-11-11 (1)
11-11: Update BuildableName from "LiteDo.app" to "MifosPay.app" in the Xcode scheme.The
BuildableNameis set to "LiteDo.app" (lines 11 and 26), but the project's actual app name is configured as "MifosPay" incmp-ios/Configuration/Config.xcconfigand the bundle identifier isorg.mifospay. The scheme file was copied from the kmp-project-template without proper customization. Update both occurrences to"MifosPay.app"to match the configured product name.cmp-ios/iosApp.xcodeproj/xcuserdata/skniyajali.xcuserdatad/xcschemes/xcschememanagement.plist-1-14 (1)
1-14: Remove user-specific Xcode files from version control.Multiple xcuserdata files are currently tracked in git (from developers skniyajali, kapilmaurya, and hekmatullahamin), which should be excluded. These files:
- Are specific to each developer's machine
- Cause merge conflicts when multiple developers commit their preferences
- Clutter the repository with personal Xcode configuration
The root cause is that
.gitignorecontains patterns foriosApp/(e.g.,iosApp/iosApp.xcodeproj/*), but the actual iOS project is located incmp-ios/. Update.gitignoreto excludecmp-ios/iosApp.xcodeproj/xcuserdata/andcmp-ios/iosApp.xcworkspace/xcuserdata/, then remove the tracked xcuserdata files from git history.core-base/network/src/jsMain/kotlin/template/core/base/network/KtorHttpClient.js.kt-10-10 (1)
10-10: Rename package to match project structure across all core-base modules.The package name
template.core.base.networkis a placeholder that doesn't align with the project's actual package structure. The codebase consistently usesorg.mifospay.*for packages (see feature modules), but the entirecore-basemodule (ui, platform, network) uses thetemplate.*prefix. Rename this toorg.mifospay.core.base.networkand apply the same fix to all other files in thecore-basemodule.core-base/network/src/commonMain/kotlin/template/core/base/network/KtorHttpClient.kt-141-157 (1)
141-157: Logging configuration has two issues.
httpLoggerparameter is ignored: Line 142 assignslogger = httpLogger, but lines 152-156 immediately overwrite it with a Kermit-based logger. ThehttpLoggerparameter becomes dead code.Empty
loggableHostsdisables all logging: The filter on lines 144-148 usesloggableHosts.any { ... }. With the default empty list, this always returnsfalse, meaning no requests are logged by default.🔎 Proposed fix
Choose either the custom Kermit logger or the configurable
httpLogger, and fix the filter logic:install(Logging) { - logger = httpLogger level = httpLogLevel - filter { request -> - loggableHosts.any { host -> - request.url.host.contains(host) + if (loggableHosts.isNotEmpty()) { + filter { request -> + loggableHosts.any { host -> + request.url.host.contains(host) + } } } sanitizeHeader { header -> header in sensitiveHeaders } logger = object : Logger { override fun log(message: String) { KermitLogger.d(tag = "KtorClient", messageString = message) } } }Alternatively, if
httpLoggershould be used instead of the hardcoded Kermit logger, remove lines 152-156.core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-80-80 (1)
80-80: Replace println with proper logging framework.Using
printlnfor logging is not production-ready. It lacks log levels, structured output, and configurability. Consider using a proper logging framework (e.g.,kermit,napier, or platform-specific loggers).🔎 Example refactor using a logging abstraction
Assuming you have a logger instance available:
is KtorfitResult.Failure -> { - println("Failure: " + result.throwable.message) + logger.error("Network request failed: ${result.throwable.message}", result.throwable) NetworkResult.Error(NetworkError.UNKNOWN) }} catch (e: SerializationException) { - println("Serialization error: ${e.message}") + logger.error("Failed to deserialize response: ${e.message}", e) NetworkResult.Error(NetworkError.SERIALIZATION) }else -> { - println("Status code $status") + logger.warn("Unhandled HTTP status code: $status") NetworkResult.Error(NetworkError.UNKNOWN) }Also applies to: 95-95, 107-107
core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-88-88 (1)
88-88: Change the HTTP success range from200..209to200..299.The current range only covers status codes 200-209, leaving codes 210-299 (201 Created, 204 No Content, 206 Partial Content, etc.) to fall through to the
elsebranch where they are incorrectly treated as UNKNOWN errors. HTTP success codes span the full 2xx range (200-299) per RFC 7231.Code context
when (status) { in 200..209 -> { try { val data = result.response.body(successType) as Any NetworkResult.Success(data) } catch (e: NoTransformationFoundException) { NetworkResult.Error(NetworkError.SERIALIZATION) } catch (e: SerializationException) { println("Serialization error: ${e.message}") NetworkResult.Error(NetworkError.SERIALIZATION) } } // ... other branches ... else -> { println("Status code $status") NetworkResult.Error(NetworkError.UNKNOWN) } }build-logic/convention/src/main/kotlin/org/convention/PrintTestApks.kt-88-91 (1)
88-91: Enhance error messages for better debugging.The error messages are generic and don't provide helpful context when things go wrong. Including the variant name and potentially the APK folder path would significantly improve debuggability.
🔎 Proposed improvement with descriptive error messages
- val builtArtifacts = builtArtifactsLoader.get().load(apkFolder.get()) - ?: throw RuntimeException("Cannot load APKs") - if (builtArtifacts.elements.size != 1) - throw RuntimeException("Expected one APK !") + val builtArtifacts = builtArtifactsLoader.get().load(apkFolder.get()) + ?: throw RuntimeException("Cannot load APKs for variant '${variantName.get()}' from ${apkFolder.get()}") + if (builtArtifacts.elements.size != 1) { + throw RuntimeException( + "Expected exactly one APK for variant '${variantName.get()}', but found ${builtArtifacts.elements.size}" + ) + } val apk = File(builtArtifacts.elements.single().outputFile).toPath()cmp-desktop/src/jvmMain/kotlin/main.kt-31-34 (1)
31-34: Move Koin initialization outside the composable scope.Calling
initKoin()inside theapplicationblock is problematic. Whileapplicationdoesn't recompose frequently, Koin'sstartKoinshould be called exactly once at application startup, outside any composable scope. This prevents potential reinitialization issues and follows DI best practices.🔎 Proposed fix
fun main() { + // Initialize Koin before entering composable scope + initKoin() + application { - // Initializes the Koin dependency injection framework. - initKoin() - // Creates a window state to manage the window's state. val windowState = rememberWindowState()build-logic/convention/src/main/kotlin/GitHooksConventionPlugin.kt-49-55 (1)
49-55: Reconsider aggressive task dependency on installGitHooks.Making all build-related tasks depend on
installGitHookswill force hook installation on every build operation, includingclean. This could:
- Slow down builds unnecessarily
- Cause issues in CI environments where
.gitmight not be available or writable- Re-install hooks even when they haven't changed
Consider:
- Running hook installation only once (check if hooks already exist and are up-to-date)
- Removing
cleanfrom the dependency list- Making this opt-in rather than automatic, or only depending on
preBuild🔎 Suggested improvements
Option 1: Only depend on preBuild:
project.afterEvaluate { project.tasks.matching { - it.name in listOf("preBuild", "build", "assembleDebug", "assembleRelease", "installDebug", "installRelease", "clean") + it.name == "preBuild" }.configureEach { dependsOn(project.tasks.named("installGitHooks")) } }Option 2: Add up-to-date checks to installGitHooks to skip when hooks are already installed.
build-logic/convention/src/main/kotlin/org/convention/Detekt.kt-17-19 (1)
17-19: Hard-coded project path breaks reusability.Line 18 hard-codes the project path
:cmp-android:dependencyGuard, which assumes a specific project structure. If this convention plugin is intended to be reusable across different projects or if the project structure changes, this will break.Consider one of these approaches:
- Make the dependency task name configurable via the plugin extension
- Use a conditional check:
tasks.findByPath(":cmp-android:dependencyGuard")?.let { mustRunAfter(it) }- Remove this constraint if the ordering isn't critical
🔎 Proposed fix using conditional
tasks.named<Detekt>("detekt") { - mustRunAfter(":cmp-android:dependencyGuard") + tasks.findByPath(":cmp-android:dependencyGuard")?.let { mustRunAfter(it) } jvmTarget = "17"build-logic/convention/src/main/kotlin/KtlintConventionPlugin.kt-7-18 (1)
7-18: Plugin ID is correct, but configuration implementation is missing compared to similar convention plugins.The plugin ID
"org.jlleitschuh.gradle.ktlint"correctly matches the version catalog declaration (version 12.1.1). However, unlikeMifosSpotlessConventionPluginandMifosDetektConventionPluginwhich both call configuration functions after applying their plugins,KtlintConventionPluginapplies the plugin without any additional configuration. NoconfigureKtlintfunction orKtlint.ktconfiguration file exists in the codebase, breaking the established pattern. Either add configuration support consistent with the other convention plugins, or document why ktlint intentionally uses defaults.build-logic/convention/src/main/kotlin/org/convention/GradleManagedDevices.kt-27-57 (1)
27-57: Add API levels 35 and 36 to device configurations.The current API levels 30–31 are still valid (API 30 is required for ATDs, and API 31 is supported). However, as of January 2026, the Google Play target API requirement is now 35+, and API 36 (Android 16) is the latest platform. Add test configurations for these levels to ensure compatibility with Play requirements and catch forward-compatibility issues.
Optionally, document why Pixel 6 (API 31) is excluded from CI if this is intentional for performance reasons.
build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreConfig.kt-10-12 (1)
10-12: Default passwords are weak and may pose a security risk if used inadvertently in production.The default values
"android"forkeystorePasswordandkeyPasswordare common debug defaults. Consider either:
- Requiring explicit password configuration (no defaults)
- Adding prominent documentation that these are debug-only defaults
- Logging a warning when defaults are used
build-logic/convention/src/main/kotlin/KeystoreManagementConventionPlugin.kt-40-43 (1)
40-43: Calling.get()on a task provider during configuration realizes the task eagerly.
generateKeystoresTask.get()on line 42 forces the task to be created immediately, defeating the purpose oftasks.register(). UseflatMapor wire dependencies lazily.🔎 Suggested approach
val updateSecretsEnvTask = tasks.register("updateSecretsEnv", SecretsEnvUpdateTask::class.java) { - SecretsEnvUpdateTask.createFromKeystoreGeneration(this, generateKeystoresTask.get(), keystoreExtension.secretsConfig.get()) + dependsOn(generateKeystoresTask) + // Wire outputs lazily or defer configuration to doFirst/TaskAction }Committable suggestion skipped: line range outside the PR's diff.
build-logic/convention/src/main/kotlin/KeystoreManagementConventionPlugin.kt-24-31 (1)
24-31: Avoid calling.get()on providers during task configuration phase.Calling
keystoreExtension.secretsConfig.get()at configuration time (line 30) forces eager evaluation and breaks Gradle's lazy configuration model. This can cause ordering issues if the extension hasn't been configured yet by the build script.🔎 Suggested approach
Wire task properties lazily using provider APIs:
val generateKeystoresTask = tasks.register("generateKeystores", KeystoreGenerationTask::class.java) { keystoreConfig.set(keystoreExtension.keystoreConfig) secretsConfig.set(keystoreExtension.secretsConfig) - - // Load configuration from secrets.env if it exists - KeystoreGenerationTask.createWithSecretsConfig(this, keystoreExtension.secretsConfig.get()) }Move the
createWithSecretsConfiglogic into the task's@TaskActionmethod or usedoFirstto defer execution to runtime.Committable suggestion skipped: line range outside the PR's diff.
build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationValidator.kt-214-218 (1)
214-218: Validation method has side effects: it creates directories.
validateFilePathscallsconfig.keystoreDir.mkdirs()on line 216, which modifies the filesystem. Validation should be a pure, read-only operation to avoid unexpected behavior when called multiple times or in dry-run scenarios.🔎 Suggested fix
Check only if the directory exists or is creatable without actually creating it:
private fun validateFilePaths(config: KeystoreConfig, errors: MutableList<String>, warnings: MutableList<String>) { // Check keystore directory - if (!config.keystoreDir.exists() && !config.keystoreDir.mkdirs()) { - errors.add("Cannot create keystore directory: ${config.keystoreDir.absolutePath}") + if (!config.keystoreDir.exists()) { + val parent = config.keystoreDir.parentFile + if (parent != null && !parent.canWrite()) { + errors.add("Cannot create keystore directory (no write permission): ${config.keystoreDir.absolutePath}") + } else { + // Directory will be created during task execution + } }build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvUpdateTask.kt-396-406 (1)
396-406: Logic issue: File existence check occurs after file creation.
printWriter().use { writer -> ... }creates/truncates the file before the existence check on line 399. The condition!secretsFile.exists()will always befalseat this point, so the header is never written for new files.🔎 Proposed fix - check before opening writer
private fun writeSecretsFile( secretsFile: File, secrets: ParsedSecretsData, ) { try { + val isNewFile = !secretsFile.exists() || secrets.comments.isEmpty() secretsFile.printWriter().use { writer -> // Write header comment if it's a new file or no comments exist - if (!secretsFile.exists() || secrets.comments.isEmpty()) { + if (isNewFile) { writer.println("# GitHub Secrets Environment File")
🟡 Minor comments (17)
core-base/network/src/commonMain/kotlin/template/core/base/network/KtorHttpClient.kt-118-126 (1)
118-126: Digest auth missingsendWithoutRequestconfiguration.Bearer and Basic auth providers use
sendWithoutRequest { request -> request.url.host in authRequiredUrl }to proactively send credentials to specified hosts. Digest auth lacks this, causing inconsistent behavior across auth types.🔎 Proposed fix
digestCredentialsProvider != null -> { install(Auth) { digest { credentials { digestCredentialsProvider() } + sendWithoutRequest { request -> + request.url.host in authRequiredUrl + } } } }build-logic/convention/src/main/kotlin/org/convention/KotlinAndroid.kt-28-34 (1)
28-34: Update comment to reflect Java 17 configuration.The comment mentions "Up to Java 11 APIs" but the code configures Java 17. Update the comment to accurately reflect the Java version and desugaring capabilities.
🔎 Suggested comment update
compileOptions { - // Up to Java 11 APIs are available through desugaring - // https://developer.android.com/studio/write/java11-minimal-support-table + // Java 17 target with core library desugaring for backward compatibility + // https://developer.android.com/studio/write/java11-minimal-support-table sourceCompatibility = JavaVersion.VERSION_17 targetCompatibility = JavaVersion.VERSION_17 isCoreLibraryDesugaringEnabled = truebuild-logic/convention/src/main/kotlin/org/convention/KotlinAndroid.kt-48-53 (1)
48-53: Update comment to reflect Java 17 configuration.The comment references Java 11 and Android desugaring, but this configures Java 17 for non-Android JVM projects where desugaring isn't applicable. Update the comment for clarity.
🔎 Suggested comment update
extensions.configure<JavaPluginExtension> { - // Up to Java 11 APIs are available through desugaring - // https://developer.android.com/studio/write/java11-minimal-support-table + // Configure Java 17 for JVM projects sourceCompatibility = JavaVersion.VERSION_17 targetCompatibility = JavaVersion.VERSION_17core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-57-58 (1)
57-58: Fix the documentation to reference the correct type.The documentation mentions "Result" but should reference "NetworkResult" for consistency with the actual implementation.
🔎 Proposed fix
* @return A [Converter.SuspendResponseConverter] if the return type is - * `Result`, or `null` otherwise. + * `NetworkResult`, or `null` otherwise.core-base/network/src/commonMain/kotlin/template/core/base/network/factory/ResultSuspendConverterFactory.kt-40-46 (1)
40-46: Fix the example code to use correct type names.The example shows
Result<List<User>, RemoteError>but should useNetworkResult<List<User>, NetworkError>to match the actual types defined in this codebase.🔎 Proposed fix
* Example usage: * ```kotlin * interface ApiService { * @GET("users") - * suspend fun getUsers(): Result<List<User>, RemoteError> + * suspend fun getUsers(): NetworkResult<List<User>, NetworkError> * } * ```cmp-desktop/src/jvmMain/kotlin/main.kt-43-43 (1)
43-43: Use a more descriptive window title.The hardcoded title "DesktopApp" appears to be a placeholder from the template. Consider using a more descriptive name that reflects the actual application, such as "MifosPay" or "Mifos Mobile Wallet".
🔎 Proposed fix
- title = "DesktopApp", + title = "MifosPay",build-logic/convention/src/main/kotlin/GitHooksConventionPlugin.kt-38-38 (1)
38-38: Fix spacing afterelsekeyword.Missing space after the closing brace.
🔎 Proposed fix
- }else { + } else {build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreConfig.kt-51-51 (1)
51-51: Key size validation is inconsistent withConfigurationValidator.Here,
keySize < 1024is an error, butConfigurationValidator.validateKeyAlgorithm()enforceskeySize < 2048for RSA. This inconsistency could allow 1024-bit RSA keys, which are considered weak.🔎 Suggested fix
-if (keySize < 1024) errors.add("Key size must be at least 1024 bits") +if (keySize < 2048) errors.add("Key size must be at least 2048 bits for secure key generation")build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationValidator.kt-42-48 (1)
42-48: Unreachable code:validity < 1condition can never be true.After checking
validity <= 0on line 42, any value that reaches line 44 must be>= 1. The conditionvalidity < 1is therefore unreachable for integer types.🔎 Suggested fix
// Validate validity period if (config.validity <= 0) { errors.add("Validity period must be positive") -} else if (config.validity < 1) { - warnings.add("Validity period is very short (${config.validity} years)") } else if (config.validity > 50) { warnings.add("Validity period is very long (${config.validity} years)") }If you want to warn about short validity (e.g., 1-2 years), change the condition:
} else if (config.validity <= 2) { warnings.add("Validity period is very short (${config.validity} years)") }build-logic/convention/src/main/kotlin/org/convention/keystore/BaseKeystoreTask.kt-81-96 (1)
81-96: Process output streams are not consumed, which may cause hangs on some systems.When a process writes to stdout/stderr and the buffers fill up,
waitFor()can block indefinitely. Additionally, there's no timeout, so a stuckkeytoolprocess would hang the build.🔎 Suggested fix
protected fun checkKeytoolAvailable(): Boolean { return try { - val process = ProcessBuilder("keytool", "-help").start() - val exitCode = process.waitFor() + val process = ProcessBuilder("keytool", "-help") + .redirectErrorStream(true) + .start() + // Consume output to prevent buffer blocking + process.inputStream.bufferedReader().readText() + val exitCode = process.waitFor(30, java.util.concurrent.TimeUnit.SECONDS) + .let { completed -> if (completed) process.exitValue() else -1 } if (exitCode == 0) { logInfo("keytool is available") true } else { logError("keytool command failed") false } } catch (e: Exception) { logError("keytool not found. Please ensure JDK is installed and keytool is in PATH") false } }build-logic/convention/src/main/kotlin/org/convention/AndroidCompose.kt-46-61 (1)
46-61: TherelativeToRootProjecthelper ignores its input provider's value.The
itparameter fromflatMapon line 48 is captured but never used. The function computes the directory path based onprojectDirregardless of what the provider contains. If the intent is simply to transform the provider to a directory, consider documenting that the input provider's value is intentionally discarded or refactor to usemapdirectly instead offlatMap.build-logic/convention/src/main/kotlin/org/convention/keystore/EnvironmentOverrideHandler.kt-218-229 (1)
218-229: Overly broad regex patterns will match unrelated environment variables.Patterns like
.*UPLOAD.*,.*ORIGINAL.*,.*RELEASE.*,.*DEBUG.*will match many unrelated variables (e.g.,FILE_UPLOAD_LIMIT,ORIGINAL_IMAGE_PATH,RELEASE_DATE,DEBUG_MODE). Consider more specific patterns:🔎 Suggested more specific patterns
val keystorePatterns = listOf( ".*KEYSTORE.*", ".*SIGNING.*", "STORE_PASSWORD", "STORE_FILE", "KEY_ALIAS", "KEY_PASSWORD", "UPLOAD_KEYSTORE.*", "ORIGINAL_KEYSTORE.*", "RELEASE_KEYSTORE.*", "DEBUG_KEYSTORE.*" )build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationFileUpdatesTask.kt-167-172 (1)
167-172: Hardcoded Firebase app IDs look like real values.The Firebase app IDs on lines 168-169 appear to be real-looking identifiers rather than obvious placeholders. Consider using clearly fake placeholder values (e.g.,
"YOUR_FIREBASE_APP_ID_HERE") or making these configurable to avoid confusion.build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvParser.kt-114-116 (1)
114-116: Unreachable code: same issue as multiline block handling.When
split("=", limit = 2)is called on a string thatcontains("="), it will always return exactly 2 parts. Thiselsebranch is dead code.build-logic/convention/src/main/kotlin/org/convention/keystore/EnvironmentOverrideHandler.kt-296-303 (1)
296-303: Naming inconsistency: Constant names don't match their values.
ORIGINAL_KEYSTORE_PASSWORDcontains"ORIGINAL_KEYSTORE_FILE_PASSWORD"andUPLOAD_KEYSTORE_PASSWORDcontains"UPLOAD_KEYSTORE_FILE_PASSWORD". This is confusing and error-prone. Either align the constant names with the values or add clarifying comments.🔎 Proposed fix - align names
// Original keystore specific - const val ORIGINAL_KEYSTORE_PASSWORD = "ORIGINAL_KEYSTORE_FILE_PASSWORD" + const val ORIGINAL_KEYSTORE_FILE_PASSWORD = "ORIGINAL_KEYSTORE_FILE_PASSWORD" const val ORIGINAL_KEYSTORE_ALIAS = "ORIGINAL_KEYSTORE_ALIAS" const val ORIGINAL_KEYSTORE_ALIAS_PASSWORD = "ORIGINAL_KEYSTORE_ALIAS_PASSWORD" // Upload keystore specific - const val UPLOAD_KEYSTORE_PASSWORD = "UPLOAD_KEYSTORE_FILE_PASSWORD" + const val UPLOAD_KEYSTORE_FILE_PASSWORD = "UPLOAD_KEYSTORE_FILE_PASSWORD" const val UPLOAD_KEYSTORE_ALIAS = "UPLOAD_KEYSTORE_ALIAS" const val UPLOAD_KEYSTORE_ALIAS_PASSWORD = "UPLOAD_KEYSTORE_ALIAS_PASSWORD"Committable suggestion skipped: line range outside the PR's diff.
build-logic/convention/src/main/kotlin/org/convention/keystore/SecretsEnvParser.kt-96-98 (1)
96-98: Unreachable code:parts.sizewill always be 2 whenline.contains("<<")is true.When
split("<<", limit = 2)is called on a string thatcontains("<<"), it will always return exactly 2 parts. Theelsebranch on line 96-98 is dead code.🔎 Proposed fix
- if (parts.size == 2) { - multilineKey = parts[0].trim() - multilineEnd = parts[1].trim() - - if (multilineKey.isBlank()) { - errors.add("Line $lineNumber: Empty key in multiline declaration") - } else if (multilineEnd.isBlank()) { - errors.add("Line $lineNumber: Empty delimiter in multiline declaration") - } else { - inMultilineBlock = true - multilineValue.clear() - } - } else { - errors.add("Line $lineNumber: Invalid multiline syntax") - } + multilineKey = parts[0].trim() + multilineEnd = parts[1].trim() + + if (multilineKey.isBlank()) { + errors.add("Line $lineNumber: Empty key in multiline declaration") + } else if (multilineEnd.isBlank()) { + errors.add("Line $lineNumber: Empty delimiter in multiline declaration") + } else { + inMultilineBlock = true + multilineValue.clear() + }Committable suggestion skipped: line range outside the PR's diff.
build-logic/convention/src/main/kotlin/org/convention/Badging.kt-30-36 (1)
30-36: Complete the incomplete documentation sentence.The KDoc comment has an incomplete sentence on lines 34-35 that trails off without completing the explanation of incremental behavior.
🔎 Proposed fix
/** * Generates the badging information of the APK. * This task is cacheable, meaning that if the inputs and outputs have not changed, * the task will be considered up-to-date and will not run. - * This task is also incremental, meaning that if the inputs have not changed, - * + * This task is also incremental, meaning that if the inputs have not changed, + * the task will skip execution and reuse the previous output. */
| if (isLinuxOrMacOs()) { | ||
| commandLine("chmod", "-R", "+x", ".git/hooks/") | ||
| }else { | ||
| commandLine("cmd", "/c", "attrib", "-R", "+X", ".git/hooks/*.*") | ||
| } |
There was a problem hiding this comment.
Windows command incorrect: attrib doesn't set executable permissions.
The Windows command uses attrib -R +X, but attrib is for file attributes (read-only, hidden, system, archive) and doesn't control executable permissions. Windows doesn't use Unix-style executable bits. Git for Windows handles hook executability automatically when hooks are placed in .git/hooks/ without a file extension.
🔎 Recommended fix
For Windows, either skip the permission-setting step or simply verify hooks were copied:
if (isLinuxOrMacOs()) {
commandLine("chmod", "-R", "+x", ".git/hooks/")
- } else {
- commandLine("cmd", "/c", "attrib", "-R", "+X", ".git/hooks/*.*")
}Alternatively, you can make the Exec task conditional:
if (isLinuxOrMacOs()) {
commandLine("chmod", "-R", "+x", ".git/hooks/")
+ } else {
+ // Git for Windows handles hook executability automatically
+ commandLine("cmd", "/c", "echo", "Git hooks copied (Windows)")
}📝 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.
| if (isLinuxOrMacOs()) { | |
| commandLine("chmod", "-R", "+x", ".git/hooks/") | |
| }else { | |
| commandLine("cmd", "/c", "attrib", "-R", "+X", ".git/hooks/*.*") | |
| } | |
| if (isLinuxOrMacOs()) { | |
| commandLine("chmod", "-R", "+x", ".git/hooks/") | |
| } |
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/GitHooksConventionPlugin.kt around
lines 36 - 40, The Windows branch is using an incorrect attrib command to set
executable bits; instead, make the permission-setting step only run on Unix by
removing or replacing the else branch that calls commandLine("cmd", "/c",
"attrib", ...). Update the isLinuxOrMacOs() conditional so that the Exec task
invokes commandLine("chmod", "-R", "+x", ".git/hooks/") only when true and
otherwise either do nothing or perform a harmless verification (e.g., check/copy
files exist), removing the incorrect attrib call; adjust the Exec task
configuration where commandLine(...) is used to reflect this change.
| apk.set( | ||
| variant.artifacts.get(SingleArtifact.APK_FROM_BUNDLE), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for bundle-related build configurations
echo "=== Searching for bundle configurations ==="
rg -i "bundle" --type gradle -C 2
echo ""
echo "=== Searching for regular APK build configurations ==="
rg -i "assemble|apk" --type gradle -C 2 | head -100
echo ""
echo "=== Checking build.gradle files in repository ==="
find . -name "build.gradle*" -type f | head -20Repository: openMF/mobile-wallet
Length of output: 1206
🏁 Script executed:
#!/bin/bash
# Search gradle files directly without type restriction
echo "=== Searching for bundle configurations in gradle files ==="
rg -i "bundle" ./build.gradle.kts ./cmp-android/build.gradle.kts ./build-logic/convention/build.gradle.kts 2>/dev/null | head -50
echo ""
echo "=== Searching for APK/assemble configurations ==="
rg -i "apk|assemble" ./build.gradle.kts ./cmp-android/build.gradle.kts 2>/dev/null | head -50
echo ""
echo "=== Examining the Badging.kt file for context ==="
cat -n ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt | head -130
echo ""
echo "=== Check if there are other artifact types referenced in the codebase ==="
rg "SingleArtifact\." ./build-logic/convention/src/main/kotlin/org/convention/ 2>/dev/nullRepository: openMF/mobile-wallet
Length of output: 6133
🏁 Script executed:
#!/bin/bash
# Check the main build.gradle.kts to understand the project structure
echo "=== Examining main build.gradle.kts ==="
cat ./build.gradle.kts | head -100
echo ""
echo "=== Checking cmp-android build configuration ==="
cat ./cmp-android/build.gradle.kts | head -150
echo ""
echo "=== Check for bundle release configuration in cmp-android ==="
rg -i "release|bundle" ./cmp-android/build.gradle.kts | head -20Repository: openMF/mobile-wallet
Length of output: 7944
🏁 Script executed:
#!/bin/bash
# Check if there's any bundle configuration elsewhere
echo "=== Search for 'bundle' keyword across entire codebase ==="
rg "bundle" --type kotlin -C 2 | head -50
echo ""
echo "=== Check the entire Badging.kt file to understand context ==="
wc -l ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt
echo ""
echo "=== View the rest of Badging.kt (lines 130-200) ==="
sed -n '130,200p' ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt
echo ""
echo "=== Check if variant.artifacts supports APK_FROM_BUNDLE or if APK should be used ==="
rg "APK_FROM_BUNDLE|SingleArtifact\.APK" --type kotlin -B 3 -A 3Repository: openMF/mobile-wallet
Length of output: 8813
🏁 Script executed:
#!/bin/bash
# Check how the Badging tasks are integrated into the build
echo "=== Search for where configureBadgingTasks is called ==="
rg "configureBadgingTasks" --type kotlin -B 5 -A 5
echo ""
echo "=== Check if badging tasks are in the default build flow ==="
rg "configureBadgingTasks|GenerateBadgingTask|CheckBadgingTask" ./build-logic/convention/build.gradle.kts -C 3
echo ""
echo "=== Check the convention plugins to see if badging is always applied ==="
fd ".*\.kt$" ./build-logic/convention/src/main/kotlin/ | xargs rg "configureBadgingTasks" -lRepository: openMF/mobile-wallet
Length of output: 5060
🏁 Script executed:
#!/bin/bash
# Check if there are any CI/CD workflows or gradle tasks that actually build bundles
echo "=== Search for bundleRelease or bundle tasks in gradle ==="
rg "bundleRelease|bundle" ./cmp-android/build.gradle.kts -i
echo ""
echo "=== Check if there's a separate module or flavor for bundle builds ==="
rg "bundle" ./cmp-android/build.gradle.kts -C 5
echo ""
echo "=== Check the AndroidApplicationConventionPlugin more carefully ==="
cat ./build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt | head -80
echo ""
echo "=== Check if both Badging.kt files are identical (convention vs mifospay) ==="
diff ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt ./build-logic/convention/src/main/kotlin/org/mifospay/Badging.kt | head -50Repository: openMF/mobile-wallet
Length of output: 2718
🏁 Script executed:
#!/bin/bash
# Let me check if the project actually produces APK_FROM_BUNDLE artifacts
echo "=== Check if the app's build actually creates APK_FROM_BUNDLE artifacts ==="
rg "APK_FROM_BUNDLE" --type kotlin -B 3 -A 3
echo ""
echo "=== Verify what artifact types are available for main variants (not test) ==="
# Search for Android Gradle Plugin documentation usage
rg "SingleArtifact\." --type kotlin | grep -v androidTest | head -20
echo ""
echo "=== Check if there are any conditional checks or try-catch around APK_FROM_BUNDLE ==="
sed -n '100,156p' ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt
echo ""
echo "=== Check git history to see if this is newly added or changed ==="
git log --oneline -n 20 -- ./build-logic/convention/src/main/kotlin/org/convention/Badging.kt 2>/dev/null | head -10Repository: openMF/mobile-wallet
Length of output: 4393
Use SingleArtifact.APK instead of SingleArtifact.APK_FROM_BUNDLE for regular APK builds.
The project builds regular APKs (not Android App Bundles), as evidenced by the absence of any bundleRelease or bundle-related configuration. The current code uses SingleArtifact.APK_FROM_BUNDLE, which only provides APKs extracted from bundles and will not wire correctly for standard APK builds. Change to SingleArtifact.APK at line 117.
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/org/convention/Badging.kt around
lines 116 - 118, Replace the artifact reference used when wiring APK outputs:
change the call that sets apk via
variant.artifacts.get(SingleArtifact.APK_FROM_BUNDLE) to use SingleArtifact.APK
instead so apk.set(...) points to regular APK outputs; update the reference in
the apk.set(...) line where variant.artifacts.get(...) is used to use
SingleArtifact.APK in place of SingleArtifact.APK_FROM_BUNDLE.
| internal fun Project.configureJacoco( | ||
| androidComponentsExtension: AndroidComponentsExtension<*, *, *>, | ||
| ) { | ||
| configure<JacocoPluginExtension> { | ||
| toolVersion = libs.findVersion("jacoco").get().toString() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that the jacoco version is defined in the version catalog
# Expected: A 'jacoco' version entry should exist
fd -t f 'libs.versions.toml' --exec cat {} | grep -i 'jacoco'Repository: openMF/mobile-wallet
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if libs.versions.toml exists
fd -t f 'libs.versions.toml'
# List gradle/build configuration files
echo "=== Gradle files ==="
fd -t f '\.(gradle|toml)$' | head -20
# Search for any jacoco references in the codebase
echo "=== JaCoCo references ==="
rg -i 'jacoco' --type gradle --type toml --type kotlin | head -30Repository: openMF/mobile-wallet
Length of output: 2188
🏁 Script executed:
cat gradle/libs.versions.tomlRepository: openMF/mobile-wallet
Length of output: 28053
Add JaCoCo version to the version catalog.
The code references libs.findVersion("jacoco") but no jacoco version entry exists in gradle/libs.versions.toml. This will cause a NoSuchElementException at runtime when .get() is called on an empty Optional. Define the jacoco version in the version catalog under the [versions] section.
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/org/convention/Jacoco.kt around lines
61 - 66, The Project.configureJacoco code calls libs.findVersion("jacoco").get()
which will throw if the version key is missing; add a "jacoco" entry to the
version catalog (gradle/libs.versions.toml) under the [versions] section (e.g.
jacoco = "0.8.8" or your desired version) so libs.findVersion("jacoco") returns
a value for configure<JacocoPluginExtension> in configureJacoco.
| private fun createNewFastlaneConfig(configFile: File, config: KeystoreConfig, keystoreName: String) { | ||
| val content = """ | ||
| module FastlaneConfig | ||
| module AndroidConfig | ||
| STORE_CONFIG = { | ||
| default_store_file: "$keystoreName", | ||
| default_store_password: "${config.keystorePassword}", | ||
| default_key_alias: "${config.keyAlias}", | ||
| default_key_password: "${config.keyPassword}" | ||
| } | ||
| FIREBASE_CONFIG = { | ||
| firebase_prod_app_id: "1:728433984912738:android:3902eb32kjaska3363b0938f1a1dbb", | ||
| firebase_demo_app_id: "1:72843493212738:android:8392hjks3298ak9032skja", | ||
| firebase_service_creds_file: "secrets/firebaseAppDistributionServiceCredentialsFile.json", | ||
| firebase_groups: "mifos-mobile-apps" | ||
| } | ||
| BUILD_PATHS = { | ||
| prod_apk_path: "cmp-android/build/outputs/apk/prod/release/cmp-android-prod-release.apk", | ||
| demo_apk_path: "cmp-android/build/outputs/apk/demo/release/cmp-android-demo-release.apk", | ||
| prod_aab_path: "cmp-android/build/outputs/bundle/prodRelease/cmp-android-prod-release.aab" | ||
| } | ||
| end | ||
| end | ||
| """.trimIndent() | ||
|
|
||
| configFile.writeText(content) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if .gitignore exists and what it contains regarding fastlane
cat .gitignore | grep -i fastlane || echo "No fastlane entries found"Repository: openMF/mobile-wallet
Length of output: 83
🏁 Script executed:
# Check if there are any references to the actual fastlane config directory
rg -i "fastlane" --type kotlin -A 2 -B 2Repository: openMF/mobile-wallet
Length of output: 15074
🏁 Script executed:
# Also check what the actual output path is in the code
rg "configFile" build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationFileUpdatesTask.kt -B 5 -A 5Repository: openMF/mobile-wallet
Length of output: 2708
🏁 Script executed:
# Check entire .gitignore for any patterns that might match fastlane-config
cat .gitignoreRepository: openMF/mobile-wallet
Length of output: 1302
Add fastlane-config/ directory to .gitignore to prevent credential exposure.
The fastlane config file writes plaintext keystore passwords to fastlane-config/android_config.rb, which is not currently gitignored and risks accidental version control commits. Add fastlane-config/ to .gitignore immediately.
🤖 Prompt for AI Agents
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/ConfigurationFileUpdatesTask.kt
around lines 156 - 184, Add the fastlane-config directory to .gitignore to
prevent committing plaintext credentials: update the repo .gitignore to include
"fastlane-config/" (or "fastlane-config/**") so files written by
createNewFastlaneConfig (which writes the Android Fastlane config via
configFile.writeText) are ignored; commit the .gitignore change and verify that
fastlane-config/android_config.rb is no longer tracked (remove from git index if
already committed using git rm --cached) before merging.
| private fun buildKeytoolCommand(config: KeystoreConfig, keystorePath: File): List<String> { | ||
| return listOf( | ||
| "keytool", | ||
| "-genkey", | ||
| "-v", | ||
| "-keystore", keystorePath.absolutePath, | ||
| "-alias", config.keyAlias, | ||
| "-keyalg", config.keyAlgorithm, | ||
| "-keysize", config.keySize.toString(), | ||
| "-validity", (config.validity * 365).toString(), // Convert years to days | ||
| "-storepass", config.keystorePassword, | ||
| "-keypass", config.keyPassword, | ||
| "-dname", config.distinguishedName | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
keytool -storepass env file password input methods
💡 Result:
Options for supplying a keystore password to keytool
-
Direct on command line (not recommended — visible in process list/shell history):
keytool ... -storepass MyPassword -
From an environment variable (recommended over plain CLI):
keytool ... -storepass:env MY_VAR
(keytool reads the env var named MY_VAR). [1] -
From a file:
keytool ... -storepass:file /path/to/passfile
(keytool reads the file contents as the password). [1] -
Interactive prompt:
Omit -storepass and keytool will prompt you to type the password (no echo). [1]
Notes / best practices
- Avoid plain CLI passwords (exposed in history / ps). Use -storepass:env or -storepass:file, or the interactive prompt. [1][2]
- Ensure file permissions or environment handling keep secrets protected (e.g., restrict file mode, avoid exporting vars in shared shells). [2]
Sources: Oracle keytool docs and community examples. [1][2]
🏁 Script executed:
# First, let's examine the file context to understand how passwords are handled
cd build-logic/convention/src/main/kotlin/org/convention/keystore && wc -l KeystoreGenerationTask.ktRepository: openMF/mobile-wallet
Length of output: 93
🏁 Script executed:
# Examine the file to understand the password handling context
cat -n build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreGenerationTask.kt | head -200 | tail -100Repository: openMF/mobile-wallet
Length of output: 4706
🏁 Script executed:
# Also check how this function is called and where passwords come from
rg -B5 -A5 "buildKeytoolCommand" build-logic/convention/src/main/kotlin/org/convention/keystore/Repository: openMF/mobile-wallet
Length of output: 2696
🏁 Script executed:
# Check the executeKeytoolCommand function to see how the command is executed
cat -n build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreGenerationTask.kt | sed -n '196,230p'Repository: openMF/mobile-wallet
Length of output: 1659
🏁 Script executed:
# Check KeystoreConfig to understand where passwords come from
rg -B3 -A10 "class KeystoreConfig" build-logic/convention/src/main/kotlin/org/convention/keystore/Repository: openMF/mobile-wallet
Length of output: 1718
Security: Passwords exposed in command-line arguments and build logs.
Passwords passed directly as command-line arguments (-storepass, -keypass) are exposed in process listings (ps aux) and build logs. Line 198 logs the complete command including passwords via logInfo(). Use keytool's -storepass:env or -storepass:file options instead, and mask sensitive values when logging commands.
| private fun executeKeytoolCommand(command: List<String>): Boolean { | ||
| return try { | ||
| logInfo("Command: ${command.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}") | ||
|
|
There was a problem hiding this comment.
Critical: Logging passwords in plain text.
Line 198 logs the entire command including -storepass and -keypass values. This will expose keystore passwords in build logs.
🔎 Proposed fix - mask sensitive arguments
private fun executeKeytoolCommand(command: List<String>): Boolean {
return try {
- logInfo("Command: ${command.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}")
+ // Mask password arguments in logs
+ val maskedCommand = command.mapIndexed { index, arg ->
+ val prevArg = command.getOrNull(index - 1)
+ if (prevArg in listOf("-storepass", "-keypass")) "****" else arg
+ }
+ logInfo("Command: ${maskedCommand.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}")📝 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.
| private fun executeKeytoolCommand(command: List<String>): Boolean { | |
| return try { | |
| logInfo("Command: ${command.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}") | |
| private fun executeKeytoolCommand(command: List<String>): Boolean { | |
| return try { | |
| // Mask password arguments in logs | |
| val maskedCommand = command.mapIndexed { index, arg -> | |
| val prevArg = command.getOrNull(index - 1) | |
| if (prevArg in listOf("-storepass", "-keypass")) "****" else arg | |
| } | |
| logInfo("Command: ${maskedCommand.joinToString(" ") { if (it.contains(" ")) "\"$it\"" else it }}") |
🤖 Prompt for AI Agents
In
@build-logic/convention/src/main/kotlin/org/convention/keystore/KeystoreGenerationTask.kt
around lines 196 - 199, The log currently prints the full command (including
sensitive `-storepass`/`-keypass` values) in executeKeytoolCommand; change the
logging to mask sensitive arguments by constructing a maskedCommand from the
command List<String> before logInfo: treat both "--flag=value" and separate
flag/value forms, replace the password value with a fixed mask (e.g. "****")
whenever an element startsWith("-storepass") or startsWith("-keypass") or
immediately follows those flags, and then call logInfo with the maskedCommand
instead of the raw command to avoid exposing keystore passwords.
| import com.diffplug.gradle.spotless.SpotlessExtension | ||
| import org.gradle.api.Project | ||
|
|
||
| const val ktlintVersion = "1.0.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ktlint latest version 2025 stable release
💡 Result:
The latest stable ktlint release is 1.8.0 (released Nov 14, 2025). [1][2]
Sources:
[1] GitHub Releases — pinterest/ktlint (1.8.0).
[2] Maven / mvnrepository listing for ktlint artifacts (shows 1.8.0 as the newest version).
🌐 Web query:
ktlint 1.0.1 security vulnerabilities CVE
💡 Result:
Short answer: ktlint 1.0.1 has no CVE assigned to the ktlint project itself, but scanners/metadata flag a dependency vulnerability (CVE-2023-6378 — a Logback deserialization/DOS issue). The older ktlint CVE (CVE-2019-1010260) affected ktlint versions before 0.30.0 and therefore does NOT apply to 1.0.1. [1][2][3]
Sources:
- mvnrepository entry for ktlint-cli 1.0.1 (lists CVE-2023-6378 from dependencies). [1]
- NVD entry for CVE-2019-1010260 (ktlint custom-ruleset MITM → RCE; fixed in 0.30.0+). [2]
- NVD / vendor pages for CVE-2023-6378 (Logback receiver deserialization DoS). [3]
Upgrade ktlint from 1.0.1 to 1.8.0.
ktlint 1.0.1 is significantly outdated and has a transitive dependency vulnerability (CVE-2023-6378 in Logback, a deserialization/DoS issue). The latest stable release is 1.8.0.
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/org/convention/Spotless.kt at line 6,
Update the ktlint version constant to remove the vulnerable old release: change
the const val ktlintVersion (in Spotless.kt) from "1.0.1" to "1.8.0", then
refresh dependencies/lockfiles and run the build/spotless checks to ensure
compatibility and resolve any formatter rule changes from the upgrade.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@build-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvUpdateTaskTest.kt:
- Around line 3-4: Replace the internal TestNG imports with JUnit 5 assertions
by removing imports of org.gradle.internal.impldep.org.testng.Assert and
importing org.junit.jupiter.api.Assertions; update all assertion calls in
SecretsEnvUpdateTaskTest (e.g., usages of assertEquals and assertTrue) to use
Assertions.assertEquals/assertTrue and, for assertEquals, swap the argument
order from (actual, expected, message) to (expected, actual, message) to match
JUnit 5's signature; ensure static import or fully-qualified References to
Assertions are consistent across the test file.
🧹 Nitpick comments (1)
build-logic/convention/src/test/kotlin/org/convention/keystore/ConfigurationFileUpdatesTaskTest.kt (1)
269-363: Test double doesn't test the actual task and misses critical error-recovery logic.The
TestConfigurationFileUpdatesTaskreimplements helper methods instead of testing the realConfigurationFileUpdatesTask. More critically, the test'stestUpdateGradleConfig()is missing the backup-and-restore mechanism present in the actual implementation (lines 214–250 of the real task), meaning backup file failures go undetected.Recommended approaches:
- Test the actual task via Gradle's
ProjectBuilderinstead of a duplicate helper class- Extract the configuration update logic into a utility class that both the real task and tests can use
- Ensure test coverage includes the backup/recovery error path
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build-logic/convention/build.gradle.ktsbuild-logic/convention/src/main/kotlin/KMPRoomConventionPlugin.ktbuild-logic/convention/src/test/kotlin/org/convention/keystore/ConfigurationFileUpdatesTaskTest.ktbuild-logic/convention/src/test/kotlin/org/convention/keystore/KeystoreGenerationTaskTest.ktbuild-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvParserTest.ktbuild-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvUpdateTaskTest.ktgradle/libs.versions.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (17)
build-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvUpdateTaskTest.kt (2)
34-51: LGTM!The test setup is well-structured, using
ProjectBuilderfor Gradle integration testing and@TempDirfor isolated file system operations. The configuration of task properties is clear and follows good testing practices.
376-437: LGTM!The helper methods are well-designed for test isolation. The base64 validation helpers (
isValidBase64,isValidBase64Line,extractBase64Content) provide good utilities for verifying the encoding output, and the file creation helpers set up realistic test scenarios.build-logic/convention/src/test/kotlin/org/convention/keystore/ConfigurationFileUpdatesTaskTest.kt (2)
25-44: LGTM!The test setup creates a comprehensive
KeystoreConfigwith all relevant fields populated. The use of@TempDirfor file isolation and proper cleanup via the annotation is appropriate.
184-202: LGTM!Good validation coverage testing multiple error conditions in a single configuration. The test verifies that all expected validation messages are present for invalid inputs (empty password/alias, small key size, negative validity, invalid country code).
build-logic/convention/src/test/kotlin/org/convention/keystore/KeystoreGenerationTaskTest.kt (3)
22-29: LGTM!The setup correctly uses
ProjectBuilderto create a Gradle project context and registers the actualKeystoreGenerationTaskfor testing. This approach ensures tests exercise the real implementation rather than a test double.
91-157: LGTM!Excellent test coverage for loading configuration from secrets. The test validates all expected fields for both ORIGINAL and UPLOAD keystore configurations, including passwords, aliases, DN components, cryptographic parameters, and the overwrite flag.
243-275: LGTM!Good validation testing that covers both valid and invalid configurations. The test properly checks for specific error messages using
any { it.contains(...) }which is flexible enough to handle message variations while still verifying the error types.build-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvParserTest.kt (4)
24-28: LGTM!Simple and clean test setup. Using a default
SecretsConfig()is appropriate since individual tests can override configuration as needed.
151-170: LGTM!Good error handling coverage testing both empty delimiter (
BAD_KEY<<) and unterminated block scenarios. The assertions properly verify that specific error messages are generated for each failure mode.
360-382: LGTM!This is an important edge case test. The parser correctly treats everything between the outer
OUTERdelimiters as literal content, including what looks like nested heredoc syntax. This matches expected shell heredoc behavior where the content is opaque until the terminating delimiter.
339-358: LGTM!Comprehensive coverage of malformed input scenarios. The test verifies that the parser:
- Successfully parses valid lines
- Handles empty values (
key_without_value=)- Reports errors for truly malformed lines (missing key, no equals sign)
- Continues parsing after encountering errors (partial parse with error reporting)
build-logic/convention/src/main/kotlin/KMPRoomConventionPlugin.kt (1)
1-17: LGTM! Clean plugin setup.The plugin structure and KSP configuration for Room are correct.
gradle/libs.versions.toml (2)
206-209: LGTM! Library declarations follow standard conventions.The library declarations are properly structured and correctly reference the version catalog entries.
Also applies to: 242-250
42-46: Consider upgrading Jackson and OkHttp to current releases; JUnit version is safe but behind latest.These versions exist and are valid. However:
- JUnit 5.13.4 is behind the latest (6.0.2, released Jan 6, 2026), but is safe: it includes the fix for CVE-2025-53103 that affected 5.12.0–5.13.1.
- Jackson 2.19.2 (latest 2.x: 2.20.1) addressed CVEs in the 2.19 line; consider upgrading if compatible.
- OkHttp 4.12.0 (latest: 5.3.0) lacks fixes for upstream vulnerabilities (okio CVE-2023-3635, brotli decompression-bomb DoS); upgrade recommended if compatible.
Likely an incorrect or invalid review comment.
build-logic/convention/build.gradle.kts (3)
32-49: LGTM! Dependencies are correctly configured.
- Room gradle plugin correctly added as
compileOnly- Keystore management dependencies properly declared as
implementation- JUnit 5 test dependencies appropriately configured
57-63: LGTM! Test configuration is correct.JUnit Platform is properly configured with useful test logging.
131-143: Plugin implementation classes are properly defined.Both plugin registrations reference existing implementation classes in
build-logic/convention/src/main/kotlin/:
KMPRoomConventionPlugin.kt✓KeystoreManagementConventionPlugin.kt✓The registrations are correctly structured.
| dependencies { | ||
| // Adding ksp dependencies for multiple platforms | ||
| "implementation"(libs.findLibrary("androidx.room.ktx").get()) | ||
| listOf( | ||
| "kspDesktop", | ||
| "kspAndroid", | ||
| "kspIosArm64", | ||
| "kspIosX64", | ||
| "kspIosSimulatorArm64", | ||
| // Add any other platform you may support | ||
| ).forEach { platform -> | ||
| add(platform, libs.findLibrary("androidx.room.compiler").get()) | ||
| // Kotlin Extensions and Coroutines support for Room | ||
| // add(platform, libs.findLibrary("androidx.room.ktx").get()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Android-specific dependency in KMP context will break non-Android builds.
Line 28 adds androidx.room.ktx as a general "implementation" dependency, but this is Android-specific and will cause failures on other KMP targets (Desktop, iOS).
In KMP projects, platform-specific dependencies must be added conditionally. The Room KTX extension should only be added for Android.
🔧 Proposed fix for platform-specific dependencies
dependencies {
// Adding ksp dependencies for multiple platforms
- "implementation"(libs.findLibrary("androidx.room.ktx").get())
listOf(
"kspDesktop",
"kspAndroid",
"kspIosArm64",
"kspIosX64",
"kspIosSimulatorArm64",
// Add any other platform you may support
).forEach { platform ->
add(platform, libs.findLibrary("androidx.room.compiler").get())
-// Kotlin Extensions and Coroutines support for Room
-// add(platform, libs.findLibrary("androidx.room.ktx").get())
+ // Add Room KTX only for Android (it's Android-specific)
+ if (platform == "kspAndroid") {
+ add("androidMainImplementation", libs.findLibrary("androidx.room.ktx").get())
+ }
}
}Alternatively, configure it using Kotlin Multiplatform source sets:
// After the dependencies block, configure source sets
val commonMain = extensions.getByType<KotlinMultiplatformExtension>()
.sourceSets.getByName("commonMain")
val androidMain = extensions.getByType<KotlinMultiplatformExtension>()
.sourceSets.getByName("androidMain")
androidMain.dependencies {
implementation(libs.findLibrary("androidx.room.ktx").get())
}📝 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.
| dependencies { | |
| // Adding ksp dependencies for multiple platforms | |
| "implementation"(libs.findLibrary("androidx.room.ktx").get()) | |
| listOf( | |
| "kspDesktop", | |
| "kspAndroid", | |
| "kspIosArm64", | |
| "kspIosX64", | |
| "kspIosSimulatorArm64", | |
| // Add any other platform you may support | |
| ).forEach { platform -> | |
| add(platform, libs.findLibrary("androidx.room.compiler").get()) | |
| // Kotlin Extensions and Coroutines support for Room | |
| // add(platform, libs.findLibrary("androidx.room.ktx").get()) | |
| } | |
| } | |
| dependencies { | |
| // Adding ksp dependencies for multiple platforms | |
| listOf( | |
| "kspDesktop", | |
| "kspAndroid", | |
| "kspIosArm64", | |
| "kspIosX64", | |
| "kspIosSimulatorArm64", | |
| // Add any other platform you may support | |
| ).forEach { platform -> | |
| add(platform, libs.findLibrary("androidx.room.compiler").get()) | |
| } | |
| } | |
| // Add Room KTX only for Android using proper KMP sourceSet configuration | |
| extensions.getByType<KotlinMultiplatformExtension>().apply { | |
| sourceSets.getByName("androidMain").dependencies { | |
| implementation(libs.findLibrary("androidx.room.ktx").get()) | |
| } | |
| } |
| import org.gradle.internal.impldep.org.testng.Assert.assertEquals | ||
| import org.gradle.internal.impldep.org.testng.Assert.assertTrue |
There was a problem hiding this comment.
Avoid using internal Gradle APIs for assertions.
The imports org.gradle.internal.impldep.org.testng.Assert reference internal Gradle implementation classes that are not part of the public API and may change or be removed without notice. Use org.junit.jupiter.api.Assertions instead, which is the standard for JUnit 5 and is already used in other test files in this PR.
Proposed fix
-import org.gradle.internal.impldep.org.testng.Assert.assertEquals
-import org.gradle.internal.impldep.org.testng.Assert.assertTrue
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Assertions.assertTrueNote: After switching to JUnit 5 assertions, the argument order for assertEquals changes from (actual, expected, message) to (expected, actual, message). You'll need to swap the first two arguments in calls like:
-assertEquals(originalKeystoreFile.readBytes().toList(), originalDecoded.toList(),
- "Decoded original keystore should match original file")
+assertEquals(originalDecoded.toList(), originalKeystoreFile.readBytes().toList(),
+ "Decoded original keystore should match original file")📝 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.
| import org.gradle.internal.impldep.org.testng.Assert.assertEquals | |
| import org.gradle.internal.impldep.org.testng.Assert.assertTrue | |
| import org.junit.jupiter.api.Assertions.assertEquals | |
| import org.junit.jupiter.api.Assertions.assertTrue |
🤖 Prompt for AI Agents
In
@build-logic/convention/src/test/kotlin/org/convention/keystore/SecretsEnvUpdateTaskTest.kt
around lines 3 - 4, Replace the internal TestNG imports with JUnit 5 assertions
by removing imports of org.gradle.internal.impldep.org.testng.Assert and
importing org.junit.jupiter.api.Assertions; update all assertion calls in
SecretsEnvUpdateTaskTest (e.g., usages of assertEquals and assertTrue) to use
Assertions.assertEquals/assertTrue and, for assertEquals, swap the argument
order from (actual, expected, message) to (expected, actual, message) to match
JUnit 5's signature; ensure static import or fully-qualified References to
Assertions are consistent across the test file.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core-base/ui/README.md (2)
388-405: Update code examples to provide requiredcontextparameter torememberImageLoader()andrememberImageRequest().Both the
CircularProfileImageandBackgroundImageexamples are broken. The functionsrememberImageLoader(context: PlatformContext)andrememberImageRequest(context: PlatformContext, url: String)require a context parameter, but the examples either don't pass it or reference an undefinedcontextvariable.🐛 Fixes required
For
CircularProfileImage(lines 388-405):@Composable fun CircularProfileImage(url: String, size: Dp = 48.dp) { + val context = LocalPlatformContext.current - val imageLoader = rememberImageLoader() + val imageLoader = rememberImageLoader(context) AsyncImage( model = rememberImageRequest(context, url),For
BackgroundImage(lines 411-432):@Composable fun BackgroundImage(url: String, overlay: Color = Color.Black.copy(alpha = 0.3f)) { + val context = LocalPlatformContext.current - val imageLoader = rememberImageLoader() + val imageLoader = rememberImageLoader(context) Box { AsyncImage( model = rememberImageRequest(context, url),
411-431: BothrememberImageLoader()andrememberImageRequest()require context parameter, but none is provided.Line 414 calls
rememberImageLoader()without the requiredPlatformContextparameter, and line 417 references an undefinedcontextvariable inrememberImageRequest(). Both function signatures require a context parameter that must be retrieved fromLocalPlatformContext.current:🐛 Proposed fix
@Composable fun BackgroundImage(url: String, overlay: Color = Color.Black.copy(alpha = 0.3f)) { + val context = LocalPlatformContext.current - val imageLoader = rememberImageLoader() + val imageLoader = rememberImageLoader(context) Box { AsyncImage( model = rememberImageRequest(context, url),cmp-android/src/main/AndroidManifest.xml (1)
14-16: Re-add the android.permission.CAMERA permission to the manifest.The codebase uses CameraX (androidx.camera library) extensively for QR code scanning in
feature/qr, not intent-based camera access as assumed. CameraX requires theandroid.permission.CAMERApermission to function. Without it, the camera will fail to initialize and the QR scanning feature will not work. The optional hardware feature declaration does not eliminate the need for the runtime permission.
🤖 Fix all issues with AI agents
In @build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.kt:
- Line 42: KMPLibraryConventionPlugin currently adds the
kotlinx.serialization.json dependency but does not apply the Kotlin
serialization compiler plugin, causing @Serializable uses to fail; update
KMPLibraryConventionPlugin to also apply the
"org.jetbrains.kotlin.plugin.serialization" plugin when configuring the KMP
convention (or alternatively remove the add("commonMainImplementation",
libs.findLibrary("kotlinx.serialization.json").get()) line and require modules
to opt into both the dependency and plugin themselves); locate
KMPLibraryConventionPlugin and either call the Kotlin serialization plugin
application in its setup or remove the serialization dependency entry to keep
plugin/dependency inclusion consistent.
In @cmp-android/src/prod/AndroidManifest.xml:
- Around line 16-19: The prod manifest enables
google_analytics_adid_collection_enabled via the meta-data tag
(android:name="google_analytics_adid_collection_enabled") but the main manifest
intentionally strips the AD_ID permission; to resolve, change the prod manifest
meta-data android:value to "false" to match the main manifest intent (or
alternatively remove the AD_ID removal in the main manifest and add proper
disclosure if you genuinely need ad ID collection); update the meta-data entry
in the prod manifest (google_analytics_adid_collection_enabled) or adjust the
AD_ID permission removal to keep manifest behavior consistent and avoid runtime
failures.
In
@core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt:
- Around line 84-96: The SMS URL in sendViaSMS is using "&" between the
recipient and query string; change it to use "?" so the body query parameter is
valid: update the smsUrl construction in sendViaSMS (where smsUrl is built) to
use "sms:$number?body=$encodedMessage" when number is present and
"sms:?body=$encodedMessage" when empty, keeping the existing encode function and
the subsequent
NSURL.URLWithString/UIApplication.sharedApplication.canOpenURL/openURL logic
intact.
🧹 Nitpick comments (6)
core-base/ui/src/jsCommonMain/kotlin/template/core/base/ui/ShareUtils.kt (1)
46-62: Document or implement the empty JS platform functions.All six utility functions are empty stubs with no implementation. If these operations are not supported on the JS/web platform, consider:
- Adding KDoc comments explaining why they're no-ops
- Throwing
NotImplementedErrorto make the limitation explicit to callers- Implementing web-based alternatives where possible (e.g.,
openUrlcould usewindow.open())Silent no-ops may confuse developers who expect these functions to work.
core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt (1)
69-82: Extract the duplicateencodefunction to reduce code duplication.The local
encodefunction is defined identically in bothsendEmail(line 70) andsendViaSMS(line 85). Extract it as a private top-level function or extension to avoid duplication.♻️ Proposed refactor
Add this private function before the
ShareUtilsobject:private fun String.encodeForUrl(): String = replace(" ", "%20").replace("\n", "%0A")Then use it in both functions:
actual fun sendEmail(to: String, subject: String?, body: String?) { - fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A") - val encodedSubject = subject?.let { encode(it) } ?: "" - val encodedBody = body?.let { encode(it) } ?: "" + val encodedSubject = subject?.encodeForUrl() ?: "" + val encodedBody = body?.encodeForUrl() ?: "" // ... rest of implementation }core-base/ui/src/desktopMain/java/template/core/base/ui/ShareUtils.desktop.kt (1)
47-110: Consider using structured logging instead of println.Error messages are printed to console using
println(lines 53, 67, 82, 98, 108). For production code, consider using a proper logging framework (e.g., SLF4J, kotlin-logging) to:
- Enable log level control
- Support structured logging and filtering
- Facilitate debugging in production environments
core-base/ui/src/androidMain/kotlin/template/core/base/ui/ShareUtils.android.kt (2)
108-116: Remove redundantletblock.Line 110 uses
url.let { url.toUri() }, but the lambda doesn't use theitparameter and directly referencesurl. This can be simplified to justurl.toUri().♻️ Proposed refactor
actual fun openUrl(url: String) { val context = ShareUtils.activityProvider.invoke().application.baseContext - val uri = url.let { url.toUri() } + val uri = url.toUri() val intent = Intent(Intent.ACTION_VIEW).apply { data = uri addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) } context.startActivity(intent) }
108-173: Consider adding error handling for Intent operations.None of the new functions handle
ActivityNotFoundExceptionthat could be thrown if no app is available to handle the intent. While this might be acceptable for the current use case, consider wrappingstartActivitycalls in try-catch blocks to prevent crashes.Example:
try { context.startActivity(intent) } catch (e: ActivityNotFoundException) { Log.w("ShareUtils", "No activity found to handle intent: ${e.message}") }cmp-android/src/prod/AndroidManifest.xml (1)
21-25: Clarify the double-negative configuration naming.The comment says "Enable Firebase analytics for
prodbuilds" but the metadata name isfirebase_analytics_collection_deactivatedset tofalse. While the logic is correct (false = not deactivated = enabled), the double-negative is confusing.Consider adding a clarifying comment such as:
<!-- Enable Firebase analytics for `prod` builds (deactivated=false means enabled) -->
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build-logic/convention/src/main/kotlin/AndroidApplicationComposeConventionPlugin.ktbuild-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.ktbuild-logic/convention/src/main/kotlin/KMPKoinConventionPlugin.ktbuild-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.ktcmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txtcmp-android/src/main/AndroidManifest.xmlcmp-android/src/prod/AndroidManifest.xmlcmp-ios/iosApp.xcodeproj/project.xcworkspace/xcuserdata/hekmatullahamin.xcuserdatad/UserInterfaceState.xcuserstatecmp-web/build.gradle.ktscore-base/ui/README.mdcore-base/ui/src/androidMain/kotlin/template/core/base/ui/ShareUtils.android.ktcore-base/ui/src/commonMain/kotlin/template/core/base/ui/ShareUtils.ktcore-base/ui/src/desktopMain/java/template/core/base/ui/ShareUtils.desktop.ktcore-base/ui/src/jsCommonMain/kotlin/template/core/base/ui/ShareUtils.ktcore-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt
✅ Files skipped from review due to trivial changes (3)
- build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt
- build-logic/convention/src/main/kotlin/AndroidApplicationComposeConventionPlugin.kt
- build-logic/convention/src/main/kotlin/KMPKoinConventionPlugin.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (5)
cmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txt (1)
1-3082: This is a generated dependency tree artifact—verify actual dependency declarations in build.gradle files.This file documents the resolved Gradle dependency graph for the
prodReleaseRuntimeClasspathconfiguration. The marked changes showkotlinx-serialization-json:1.8.1being consistently added across numerous modules (core:common, core:datastore, core:model, core-base:network, and all feature modules).Key observations:
- No dependency conflicts or resolution issues are evident; version convergence is clean.
- The serialization library addition aligns with the PR objective to include template modules.
Recommendations:
- Verify that the actual dependency declarations (e.g.,
build.gradle.ktsorgradle.toml) in each module have been updated to explicitly declarekotlinx-serialization-json:1.8.1. This generated tree is a consequence, not the source.- Check whether this file should be committed to version control. Dependency trees can often be regenerated on-demand via
./gradlew dependencies, so verify whether keeping this in the repo adds value or introduces merge-conflict noise.- Run
./gradlew check(as mentioned in the PR checklist) to ensure the new dependencies don't introduce compilation or test failures.cmp-web/build.gradle.kts (1)
53-54: No issues with the Koin and Ktor client dependencies added at lines 53–54.
libs.koin.coreis actively used viainitKoin()in bothjsMainandwasmJsMainentry points.libs.ktor.client.jscorrectly provides the JavaScript engine;ktor-client-coreis already available through the transitive dependency chain viacore-base/network(which exposes it as anapidependency), and the sameJsengine is properly configured for both JS and WASM targets incore-base/network/src/jsMain/andcore-base/network/src/wasmJsMain/.core-base/ui/src/commonMain/kotlin/template/core/base/ui/ShareUtils.kt (1)
43-87: LGTM! Well-documented API surface.The expect declarations are clear, well-documented, and provide a consistent cross-platform API for utility operations. The KDoc comments effectively explain the purpose and parameters of each function.
cmp-android/src/main/AndroidManifest.xml (1)
69-76: Firebase configuration pattern looks good.The default-disabled configuration for Firebase analytics and Crashlytics in the main manifest, with the prod variant overriding to enable them, follows a good privacy-conscious pattern. This ensures analytics/crash reporting is only active in production builds.
build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.kt (1)
12-14: LGTM!Clear and concise documentation that accurately describes the plugin's purpose.
| } | ||
|
|
||
| dependencies { | ||
| add("commonMainImplementation", libs.findLibrary("kotlinx.serialization.json").get()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for serialization plugin application in convention plugins
rg -n "plugin.serialization|kotlin.*serialization" build-logic/convention/src/ -A 2 -B 2Repository: openMF/mobile-wallet
Length of output: 2069
🏁 Script executed:
#!/bin/bash
# Read the full KMPLibraryConventionPlugin.kt file to see context around line 42
wc -l build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.ktRepository: openMF/mobile-wallet
Length of output: 135
🏁 Script executed:
#!/bin/bash
# List all convention plugin files to check for serialization plugin elsewhere
fd -e ".kt" build-logic/convention/src/main/kotlin/ | head -20Repository: openMF/mobile-wallet
Length of output: 525
🏁 Script executed:
#!/bin/bash
cat -n build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.ktRepository: openMF/mobile-wallet
Length of output: 2229
🏁 Script executed:
#!/bin/bash
# Also read the CMPFeatureConventionPlugin.kt to see the context
cat -n build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.ktRepository: openMF/mobile-wallet
Length of output: 3905
🏁 Script executed:
#!/bin/bash
# Search for modules/build files that apply org.convention.kmp.library
rg -n "org.convention.kmp.library" --type kotlin --type gradle-kotlinRepository: openMF/mobile-wallet
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Also search for plugins.id pattern in build files
fd -e ".gradle.kts" -e ".gradle" | head -30Repository: openMF/mobile-wallet
Length of output: 1012
🏁 Script executed:
#!/bin/bash
# Search for org.convention.kmp.library in build files
rg -n "org.convention.kmp.library" .Repository: openMF/mobile-wallet
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Also search for which plugins are commonly applied
rg -n "id\(\"org.convention" . -A 1 -B 1 | head -50Repository: openMF/mobile-wallet
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for modules that use org.convention.kmp.library directly
rg -n "kmp-library-convention|org\.convention\.kmp\.library" --type gradle-kts . 2>/dev/null || rg -n "kmp-library-convention|org\.convention\.kmp\.library" .Repository: openMF/mobile-wallet
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Check a few module build files to see what convention plugins they apply
head -20 core/common/build.gradle.ktsRepository: openMF/mobile-wallet
Length of output: 662
🏁 Script executed:
#!/bin/bash
# Check more module build files
head -20 core/data/build.gradle.ktsRepository: openMF/mobile-wallet
Length of output: 681
🏁 Script executed:
#!/bin/bash
# Check more module build files to see the pattern of plugin usage
for file in core/designsystem/build.gradle.kts core/domain/build.gradle.kts core/model/build.gradle.kts core/network/build.gradle.kts; do
echo "=== $file ===" && head -20 "$file" && echo ""
doneRepository: openMF/mobile-wallet
Length of output: 2506
🏁 Script executed:
#!/bin/bash
# Count how many modules apply kotlin.serialization
rg -l "kotlin.serialization|kotlin\.serialization" . --glob "*/build.gradle.kts" | wc -lRepository: openMF/mobile-wallet
Length of output: 65
🏁 Script executed:
#!/bin/bash
# List all modules that apply kmp-library-convention
rg -l "kmp.library.convention|kmp-library-convention" . --glob "*/build.gradle.kts"Repository: openMF/mobile-wallet
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Get a complete list of modules by checking all build.gradle.kts files
find . -name "build.gradle.kts" -not -path "*/build-logic/*" -not -path "*/build/*" -not -path "*/.gradle/*" | sortRepository: openMF/mobile-wallet
Length of output: 1668
🏁 Script executed:
#!/bin/bash
# Check which modules apply serialization plugin
for file in $(find . -name "build.gradle.kts" -not -path "*/build-logic/*" -not -path "*/build/*" -not -path "*/.gradle/*" | sort); do
if grep -q "kotlin.serialization\|kotlinx-serialization" "$file" 2>/dev/null; then
echo "$file: YES"
else
echo "$file: NO"
fi
doneRepository: openMF/mobile-wallet
Length of output: 1866
🏁 Script executed:
#!/bin/bash
# Check the full plugin section of core/designsystem/build.gradle.kts since it was truncated
head -40 core/designsystem/build.gradle.ktsRepository: openMF/mobile-wallet
Length of output: 1294
🏁 Script executed:
#!/bin/bash
# Check core/domain which applies kmp-library-convention without serialization
cat -n core/domain/build.gradle.kts | head -30Repository: openMF/mobile-wallet
Length of output: 1117
Add the Kotlin serialization plugin to KMPLibraryConventionPlugin.
The kotlinx-serialization-json dependency is added at line 42, but the org.jetbrains.kotlin.plugin.serialization plugin is not applied in this convention plugin. This creates a mismatch: modules that use KMPLibraryConventionPlugin (like core/domain, core/designsystem, core/ui) receive the dependency but lack the required compiler plugin. If any of these modules attempt to use @Serializable annotations, compilation will fail.
Either apply the serialization plugin here alongside the dependency, or remove the dependency and require it to be added explicitly in modules that need it (along with the plugin).
🤖 Prompt for AI Agents
In @build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.kt at line
42, KMPLibraryConventionPlugin currently adds the kotlinx.serialization.json
dependency but does not apply the Kotlin serialization compiler plugin, causing
@Serializable uses to fail; update KMPLibraryConventionPlugin to also apply the
"org.jetbrains.kotlin.plugin.serialization" plugin when configuring the KMP
convention (or alternatively remove the add("commonMainImplementation",
libs.findLibrary("kotlinx.serialization.json").get()) line and require modules
to opt into both the dependency and plugin themselves); locate
KMPLibraryConventionPlugin and either call the Kotlin serialization plugin
application in its setup or remove the serialization dependency entry to keep
plugin/dependency inclusion consistent.
| <meta-data | ||
| android:name="google_analytics_adid_collection_enabled" | ||
| android:value="true" | ||
| tools:replace="android:value" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd AndroidManifest.xml | head -20Repository: openMF/mobile-wallet
Length of output: 1081
🏁 Script executed:
cat -n cmp-android/src/main/AndroidManifest.xmlRepository: openMF/mobile-wallet
Length of output: 3474
🏁 Script executed:
cat -n cmp-android/src/prod/AndroidManifest.xmlRepository: openMF/mobile-wallet
Length of output: 1389
Critical conflict: Prod manifest enables AD_ID collection but permission is removed.
The main manifest explicitly removes the AD_ID permission (lines 25-27) with a clear comment stating that Firebase adds it automatically but the app doesn't use it to avoid Google Play rejection. It also disables google_analytics_adid_collection_enabled (line 66).
However, the prod variant overrides this and enables google_analytics_adid_collection_enabled="true" (lines 16-19), which requires the AD_ID permission that was removed. This will cause runtime failures when the analytics library attempts to collect ad IDs in production builds.
Recommended action: Either set android:value="false" in the prod manifest to align with the main manifest and its documented intent, or remove the AD_ID permission removal from the main manifest and add proper disclosure in the privacy policy if ad ID collection is actually needed.
🤖 Prompt for AI Agents
In @cmp-android/src/prod/AndroidManifest.xml around lines 16 - 19, The prod
manifest enables google_analytics_adid_collection_enabled via the meta-data tag
(android:name="google_analytics_adid_collection_enabled") but the main manifest
intentionally strips the AD_ID permission; to resolve, change the prod manifest
meta-data android:value to "false" to match the main manifest intent (or
alternatively remove the AD_ID removal in the main manifest and add proper
disclosure if you genuinely need ad ID collection); update the meta-data entry
in the prod manifest (google_analytics_adid_collection_enabled) or adjust the
AD_ID permission removal to keep manifest behavior consistent and avoid runtime
failures.
| actual fun sendViaSMS(number: String, message: String) { | ||
| fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A") | ||
| val encodedMessage = encode(message) | ||
| val smsUrl = if (number.isNotEmpty()) { | ||
| "sms:$number&body=$encodedMessage" | ||
| } else { | ||
| "sms:&body=$encodedMessage" | ||
| } | ||
| val url = NSURL.URLWithString(smsUrl) | ||
| if (url != null && UIApplication.sharedApplication.canOpenURL(url)) { | ||
| UIApplication.sharedApplication.openURL(url) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect SMS URL format.
Line 88 uses & as the separator between the phone number and the body parameter, but SMS URLs require ? for the query string separator (standard URL format).
🐛 Proposed fix
actual fun sendViaSMS(number: String, message: String) {
fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A")
val encodedMessage = encode(message)
val smsUrl = if (number.isNotEmpty()) {
- "sms:$number&body=$encodedMessage"
+ "sms:$number?body=$encodedMessage"
} else {
- "sms:&body=$encodedMessage"
+ "sms:?body=$encodedMessage"
}
val url = NSURL.URLWithString(smsUrl)
if (url != null && UIApplication.sharedApplication.canOpenURL(url)) {
UIApplication.sharedApplication.openURL(url)
}
}📝 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.
| actual fun sendViaSMS(number: String, message: String) { | |
| fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A") | |
| val encodedMessage = encode(message) | |
| val smsUrl = if (number.isNotEmpty()) { | |
| "sms:$number&body=$encodedMessage" | |
| } else { | |
| "sms:&body=$encodedMessage" | |
| } | |
| val url = NSURL.URLWithString(smsUrl) | |
| if (url != null && UIApplication.sharedApplication.canOpenURL(url)) { | |
| UIApplication.sharedApplication.openURL(url) | |
| } | |
| } | |
| actual fun sendViaSMS(number: String, message: String) { | |
| fun encode(s: String): String = s.replace(" ", "%20").replace("\n", "%0A") | |
| val encodedMessage = encode(message) | |
| val smsUrl = if (number.isNotEmpty()) { | |
| "sms:$number?body=$encodedMessage" | |
| } else { | |
| "sms:?body=$encodedMessage" | |
| } | |
| val url = NSURL.URLWithString(smsUrl) | |
| if (url != null && UIApplication.sharedApplication.canOpenURL(url)) { | |
| UIApplication.sharedApplication.openURL(url) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@core-base/ui/src/nativeMain/kotlin/template/core/base/ui/ShareUtils.native.kt
around lines 84 - 96, The SMS URL in sendViaSMS is using "&" between the
recipient and query string; change it to use "?" so the body query parameter is
valid: update the smsUrl construction in sendViaSMS (where smsUrl is built) to
use "sms:$number?body=$encodedMessage" when number is present and
"sms:?body=$encodedMessage" when empty, keeping the existing encode function and
the subsequent
NSURL.URLWithString/UIApplication.sharedApplication.canOpenURL/openURL logic
intact.
1 participant
Issue Fix
Jira Task: MW-328
Screenshots
Description
Summary of changes:
Apply the
AndroidStyle.xmlstyle template to your code in Android Studio.Run the unit tests with
./gradlew checkto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
New Features
Build & Tooling
Platform Support
Config/Privacy
✏️ Tip: You can customize this high-level summary in your review settings.