feat(core): add StringUtils for random string generation with customizable constraints by Ziedelth · Pull Request #28 · Shikkanime/framework

Ziedelth · 2026-08-31T13:20:57Z

No description provided.

Ziedelth

Review Hermes

Verdict: 4 findings — no blockers, tests pass (8/8). Code is clean, well-tested (Given/When/Then, @nested, parameterized), and core stays zero-dependency. One MAJOR (inherited from the original RandomManager pattern) and two MINORs worth addressing before merge.

Warning: MAJOR

StringUtils.kt:17 - duplicate ' in ALPHABET_SPECIAL. The string contains the single quote twice (verified programmatically: 24 chars, 23 unique). ' therefore has double the selection probability versus every other special character, in both the required-special injection and the random fills. Note: the same duplicate exists in the legacy RandomManager.RANDOM_STRING_CHARACTERS (shikkanime/core) - worth fixing in both. Suggested: "_-.!~*'();:@&=+$,/?#[]%".

Suggestion: MINOR

StringUtils.kt:26 - randomness source undocumented. Char.random() uses kotlin.random.Random.Default (non-cryptographic). Fine for identifiers (the legacy call sites generate member identifiers), but the KDoc should state the non-cryptographic nature so nobody uses it for secrets; optionally accept a random: Random = Random.Default parameter for SecureRandom().asKotlinRandom() injection.
StringUtils.kt:37-48 - per-call allocations. listOf(...).count { it } boxes 4 booleans and allocates a list per call; the alphabet is re-concatenated on every invocation. Cheap to fix with two precomputed constants (ALPHABET_ALPHANUMERIC/ALPHABET_ALL) and integer counting. Cosmetic at this scale but the constants also read better.

NIT

StringUtils.kt:29-30 - blank line inside the parameter list between includeSpecial and shouldHaveAtLeastOneUppercase.

Looks good

Zero-dependency core respected (pure Kotlin stdlib).
Tests are thorough: alphabet membership, exclusion, all-required-types, minimum-length edge (4), silent-ignore case, negative/zero length (parameterized), and the length < required-types rejection with exact messages - all 8 pass.
Silent-ignore of shouldHaveAtLeastOneSpecial when includeSpecial=false is documented in KDoc and covered by a dedicated test - acceptable as designed.

Uncertain points / to clarify

Does this PR replace RandomManager.generateRandomString and StringUtils.generateRandomString from shikkanime/core (same alphabets, same ' duplicate)? If so, plan the migration and removal on the core side to avoid two diverging implementations.
The proposed random: Random parameter (SecureRandom injection) is not covered by any guideline - to define: useful now or YAGNI?

Reviewed by Hermes Agent (fan-out multi-models; 2 OpenRouter reviewers unavailable - credits exhausted - consolidated review from the primary reviewer + local verifications)

Ziedelth

Review Hermes

Verdict : ⚠️ COMMENT (1 point vérifié à corriger + points de design)

Le code est propre, bien structuré et suit scrupuleusement les conventions du framework (KDoc, typage explicite, immutabilité, tests JUnit 6 @Nested / @DisplayName / Given-When-Then, zéro dépendance externe pour le module core).

Récapitulatif des retours

core/src/main/kotlin/StringUtils.kt:17 : Le caractère quote simple (') est présent en double dans ALPHABET_SPECIAL (aux index 3 et 7), ce qui double sa probabilité de tirage lors de la génération aléatoire.

⚠️ Points incertains / pistes d'évolution (architecture & design)

Source de randomisation cryptographique vs PRNG par défaut :
- generateRandomString utilise kotlin.random.Random.Default via random() et shuffled().
- Pour des identifiants non sensibles ou du mocking de test, c'est suffisant. En revanche, si la fonction doit servir à générer des tokens d'authentification, secrets ou clés de session, permettre de passer une instance kotlin.random.Random personnalisée (ex. SecureRandom().asKotlinRandom()) ou documenter explicitement l'usage non cryptographique dans la KDoc serait bénéfique.
Allocations intermédiaires à chaque appel :
- listOf(...).count { it } alloue une liste de 4 booléens à chaque exécution, et val alphabet = ... concatène les chaînes à chaque appel.
- Les alphabets combinés (ALPHABET_ALPHANUMERIC et ALPHABET_ALL) pourraient être pré-calculés en constantes private const val, et le comptage calculé sans allocation.
Ligne vide dans la liste des paramètres (StringUtils.kt:30) :
- Une ligne vide sépare includeSpecial des paramètres de contraintes shouldHaveAtLeastOne*. C'est un choix cosmétique d'aération, mais peut être nettoyé si le style projet préfère des listes de paramètres compactes.

Ziedelth · 2026-08-31T16:34:00Z

Complément de review (findings additionnels du reviewer après analyse complète)

En plus des 4 points postés en review inline :

MINOR — constantes sans type explicite (StringUtils.kt:8,11,14,17). Les const val ALPHABET_* n'ont pas de type déclaré, alors que l'AGENTS.md exige des types explicites pour les APIs publiques du framework. Ajouter : String aux 4 constantes.
MINOR — alternative au silent-ignore (StringUtils.kt:41). includeSpecial=false + shouldHaveAtLeastOneSpecial=true est silencieusement ignoré (documenté + testé, donc délibéré). Alternative plus stricte si souhaité : require(!shouldHaveAtLeastOneSpecial || includeSpecial) avec un message clair.
NIT — KDoc sans @PARAM (StringUtils.kt:19-25). Ajouter les @param pour les 6 paramètres, dont un documentant le contrat de randomness (voir point MINOR de la review inline).
NIT — 2 tests d'edge cases manquants (StringUtilsTest.kt). length=1 avec un type requis (le cas valide le plus serré) et includeSpecial=false combiné aux 3 autres requirements. Peu coûteux et ils verrouillent le comportement d'angle.

Note de sévérité ajustée : la randomness non cryptographique (point 2 de la review inline) mérite d'être traitée en priorité — la forme de l'API invite à générer des secrets, et c'est le point que le reviewer a remonté en MAJOR.

Ziedelth · 2026-08-31T16:50:57Z

+    const val ALPHABET_NUMBERS = "0123456789"
+
+    /** Special characters available for random string generation. */
+    const val ALPHABET_SPECIAL = "_-.'!~*'();:@&=+$,/?#[]%"


WARNING MAJOR - duplicate ' in ALPHABET_SPECIAL. The single quote appears twice (24 chars, 23 unique - verified programmatically), so ' has double the selection probability versus other special characters. Same duplicate exists in the legacy RandomManager.RANDOM_STRING_CHARACTERS (shikkanime/core).

Suggested change

const val ALPHABET_SPECIAL = "_-.'!~*'();:@&=+$,/?#[]%"

const val ALPHABET_SPECIAL = "_-.!~*'();:@&=+$,/?#[]%"

Ziedelth · 2026-08-31T16:50:58Z

+        length: Int,
+        includeSpecial: Boolean = true,
+
+        shouldHaveAtLeastOneUppercase: Boolean = false,


NIT - blank line inside the parameter list breaks the visual grouping of the requirement flags.

Suggested change

shouldHaveAtLeastOneUppercase: Boolean = false,

includeSpecial: Boolean = true,

shouldHaveAtLeastOneUppercase: Boolean = false,

Ziedelth · 2026-08-31T16:51:00Z

+     *
+     * @throws IllegalArgumentException if [length] is not positive or cannot accommodate all required character types
+     */
+    fun generateRandomString(


SUGGESTION MINOR - randomness source undocumented. Char.random() uses kotlin.random.Random.Default (non-cryptographic). Fine for identifiers, but the KDoc should state it so nobody uses this for secrets; also accept a random parameter for SecureRandom().asKotlinRandom() injection.

Suggested change

fun generateRandomString(

fun generateRandomString(

length: Int,

includeSpecial: Boolean = true,

shouldHaveAtLeastOneUppercase: Boolean = false,

shouldHaveAtLeastOneLowercase: Boolean = false,

shouldHaveAtLeastOneNumber: Boolean = false,

shouldHaveAtLeastOneSpecial: Boolean = false,

random: Random = Random.Default

): String {

Ziedelth · 2026-08-31T16:51:01Z

+    ): String {
+        require(length > 0) { "Length must be greater than 0" }
+
+        val requiredCharacterTypeCount = listOf(


SUGGESTION MINOR - per-call allocations. listOf(...).count { it } boxes 4 booleans and allocates a list per call; the alphabet is re-concatenated on every invocation. Precomputed constants (ALPHABET_ALPHANUMERIC / ALPHABET_ALL) read better and cost nothing.

Suggested change

val requiredCharacterTypeCount = listOf(

var requiredCharacterTypeCount = 0

if (shouldHaveAtLeastOneUppercase) requiredCharacterTypeCount++

if (shouldHaveAtLeastOneLowercase) requiredCharacterTypeCount++

if (shouldHaveAtLeastOneNumber) requiredCharacterTypeCount++

if (includeSpecial && shouldHaveAtLeastOneSpecial) requiredCharacterTypeCount++

require(requiredCharacterTypeCount <= length) { "Length must be greater than or equal to the number of required character types" }

val alphabet = if (includeSpecial) ALPHABET_ALL else ALPHABET_ALPHANUMERIC

Ziedelth

Review Hermes

Verdict: 💡 COMMENT — no blocker, 3 verified inline points, open design questions in the details block.

Inline (each verified against the head SHA): duplicate apostrophe in ALPHABET_SPECIAL; contradictory includeSpecial/shouldHaveAtLeastOneSpecial silently ignored; Random.Default (ThreadLocalRandom on JVM) is not cryptographically secure — document the contract or make the randomness source injectable.

⚠️ Points incertains / à clarifier

object StringUtils vs the transverse "no singletons" convention: the framework already ships utility objects (Validator, ControllerBinder), so the codebase pattern contradicts a strict reading — decide whether stateless utility objects are the sanctioned exception and codify it.
Explicit types on the public const val ALPHABET_* (the root guideline asks for explicit types on public framework APIs, but existing consts in the codebase also omit them) — pick one convention.
includeSpecial = true as default is surprising for a generic generator (URL/shell-sensitive punctuation); no requirement fixes the intent — consider false or no default.
ALPHABET_NUMBERS/shouldHaveAtLeastOneNumber hold digits, not numbers — optional rename to *DIGITS.
Blank line separating includeSpecial from the shouldHaveAtLeastOne* group — cosmetic, keep or drop.
Per-call allocations (listOf(...).count, alphabet re-concatenation) — micro-optimization only; combined alphabet constants could be precomputed if this ever sits on a hot path.

Guideline follow-ups proposed for the feedback loop: (1) codify the randomness-source rule (Random.Default vs SecureRandom) for public framework APIs; (2) codify whether utility objects are allowed in the framework.

Ziedelth · 2026-08-31T20:41:20Z

+    const val ALPHABET_NUMBERS = "0123456789"
+
+    /** Special characters available for random string generation. */
+    const val ALPHABET_SPECIAL = "_-.'!~*'();:@&=+$,/?#[]%"


The apostrophe appears twice in ALPHABET_SPECIAL, so it is selected with twice the probability of any other special character (verified programmatically: 24 characters, 23 unique):

Suggested change

const val ALPHABET_SPECIAL = "_-.'!~*'();:@&=+$,/?#[]%"

const val ALPHABET_SPECIAL = "_-.!~*'();:@&=+$,/?#[]%"

Ziedelth · 2026-08-31T20:41:21Z

+        shouldHaveAtLeastOneNumber: Boolean = false,
+        shouldHaveAtLeastOneSpecial: Boolean = false
+    ): String {
+        require(length > 0) { "Length must be greater than 0" }


includeSpecial = false combined with shouldHaveAtLeastOneSpecial = true is silently ignored: the caller requests a postcondition that is never enforced, and the returned string can legitimately contain no special character. Failing fast on contradictory arguments prevents that class of caller mistakes (the current "should ignore" test would become a rejection test, and the KDoc @throws updated):

Suggested change

require(length > 0) { "Length must be greater than 0" }

require(length > 0) { "Length must be greater than 0" }

require(includeSpecial || !shouldHaveAtLeastOneSpecial) { "Cannot require special characters when includeSpecial is false" }

Ziedelth · 2026-08-31T20:41:21Z

+    /**
+     * Generates a random string with the requested length and character constraints.
+     *
+     * The special-character requirement is applied only when [includeSpecial] is `true`.


random() and shuffled() use kotlin.random.Random.Default, which on the JVM delegates to ThreadLocalRandom/java.util.Random — both documented as NOT cryptographically secure (verified on kotlin-stdlib 2.4.10: JDK8PlatformImplementations.defaultPlatformRandom() returns PlatformThreadLocalRandom). For a public framework API exposing password-style constraints, that is a predictable-output footgun if a consumer ever generates tokens or credentials with it. Either document the non-cryptographic contract (suggestion below), or add an injectable random: Random = Random.Default parameter so SecureRandom().asKotlinRandom() can be supplied:

Suggested change

* The special-character requirement is applied only when [includeSpecial] is `true`.

* The special-character requirement is applied only when [includeSpecial] is `true`.

*

* Not cryptographically secure: relies on [kotlin.random.Random.Default]. Do not use for

* security-sensitive values such as tokens, secrets, or passwords.

Ziedelth

Review Hermes

Verdict : 💡 COMMENT — aucun bug vérifié ; 1 point vérifié inline, quelques points de design à clarifier (collapsés).

✅ Points vérifiés conformes

core reste zéro-dépendance (aucun import de module) — ok.
KDoc sur l'API publique, anglais partout, types explicites, immutabilité — ok.
Tests : JUnit 6, @Nested + @DisplayName, noms en backticks, Given/When/Then, @ParameterizedTest — conformes à guidelines/TESTING.md.
CI Build & Test : PASS (confirmé en local : ./gradlew test green).

⚠️ Points incertains / à clarifier (design, non vérifiables comme violation de guideline)

Source d'aléatoire (Random.Default) — core/src/main/kotlin/StringUtils.kt:50-57 s'appuie sur String.random() / Iterable.random(), donc kotlin.random.Random.Default, non cryptographiquement sûr. Aucune guideline n'exige SecureRandom : si l'utilitaire peut servir à des tokens/mots de passe, prévoir soit un paramètre injectable (random: Random = Random.Default, accepte SecureRandom().asKotlinRandom()), soit une mention KDoc explicite « usage non cryptographique ».
Contradiction de paramètres silencieuse — includeSpecial = false + shouldHaveAtLeastOneSpecial = true ignore silencieusement la contrainte demandée (documenté en KDoc et testé, mais un require(includeSpecial || !shouldHaveAtLeastOneSpecial) éviterait de produire une valeur plus faible que demandée).
object StringUtils vs convention « pas de singletons » — conventions.md interdit les singletons (règle écrite pour l'injection de dépendances app) ; un utilitaire stateless en object + const est idiomatique Kotlin. À clarifier : top-level declarations / const vs object ?
Allocations par appel — listOf(...).count { } + concaténation d'alphabets à chaque invocation ; précalculer des constantes combinées (ex. ALPHABET_ALPHANUMERIC) si l'utilitaire est appelé en hot path.
NITs — ligne vide entre includeSpecial et les flags shouldHaveAtLeastOne* (ligne 29) ; les deux nouveaux fichiers n'ont pas de newline final.

Ziedelth · 2026-08-31T20:43:18Z

+    const val ALPHABET_NUMBERS = "0123456789"
+
+    /** Special characters available for random string generation. */
+    const val ALPHABET_SPECIAL = "_-.'!~*'();:@&=+$,/?#[]%"


🔴 Vérifié en local : l'apostrophe ' apparaît 2 fois dans ALPHABET_SPECIAL (24 caractères, 23 distincts) — double probabilité de sélection par rapport à chaque autre caractère spécial, y compris pour le caractère requis injecté.

Suggested change

const val ALPHABET_SPECIAL = "_-.'!~*'();:@&=+$,/?#[]%"

const val ALPHABET_SPECIAL = "_-.'!~*();:@&=+$,/?#[]%"

…zable constraints

…erator and improved validation

Ziedelth commented Aug 31, 2026

View reviewed changes

Ziedelth added 2 commits September 1, 2026 08:36

feat(core): add StringUtils for random string generation with customi…

547cdad

…zable constraints

feat(core): enhance generateRandomString with customizable random gen…

333140c

…erator and improved validation

Ziedelth force-pushed the feat/string-utils branch from db2ec37 to 333140c Compare September 1, 2026 07:26

Ziedelth merged commit a61f816 into master Sep 3, 2026
2 checks passed

Ziedelth deleted the feat/string-utils branch September 3, 2026 07:21

	const val ALPHABET_SPECIAL = "_-.'!~*'();:@&=+$,/?#[]%"
	const val ALPHABET_SPECIAL = "_-.!~*'();:@&=+$,/?#[]%"

	shouldHaveAtLeastOneUppercase: Boolean = false,
	includeSpecial: Boolean = true,
	shouldHaveAtLeastOneUppercase: Boolean = false,

-        val requiredCharacterTypeCount = listOf(
+        var requiredCharacterTypeCount = 0
+        if (shouldHaveAtLeastOneUppercase) requiredCharacterTypeCount++
+        if (shouldHaveAtLeastOneLowercase) requiredCharacterTypeCount++
+        if (shouldHaveAtLeastOneNumber) requiredCharacterTypeCount++
+        if (includeSpecial && shouldHaveAtLeastOneSpecial) requiredCharacterTypeCount++
+        require(requiredCharacterTypeCount <= length) { "Length must be greater than or equal to the number of required character types" }
+        val alphabet = if (includeSpecial) ALPHABET_ALL else ALPHABET_ALPHANUMERIC

	require(length > 0) { "Length must be greater than 0" }
	require(length > 0) { "Length must be greater than 0" }
	require(includeSpecial \|\| !shouldHaveAtLeastOneSpecial) { "Cannot require special characters when includeSpecial is false" }

	const val ALPHABET_SPECIAL = "_-.'!~*'();:@&=+$,/?#[]%"
	const val ALPHABET_SPECIAL = "_-.'!~*();:@&=+$,/?#[]%"

Uh oh!

Conversation

Ziedelth commented Aug 31, 2026

Uh oh!

Ziedelth left a comment

Choose a reason for hiding this comment

Review Hermes

Warning: MAJOR

Suggestion: MINOR

NIT

Looks good

Uh oh!

Ziedelth left a comment

Choose a reason for hiding this comment

Review Hermes

Récapitulatif des retours

Uh oh!

Ziedelth commented Aug 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ziedelth left a comment

Choose a reason for hiding this comment

Review Hermes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ziedelth left a comment

Choose a reason for hiding this comment

Review Hermes

✅ Points vérifiés conformes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant