✨ feat(environment): add type-safe environment variable API - #400
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f0a9d38d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (exception: Exception) { | ||
| throw failure( | ||
| exception.message ?: "the validation rule threw ${exception::class.simpleName}", | ||
| exception |
There was a problem hiding this comment.
When a sensitive validator throws an exception whose message includes its input—for example, a NumberFormatException from parsing the secret inside require—the message is copied into reason and then exposed through EnvironmentVariableValidationException.message. Although the raw-value and cause fields are redacted, this path can still leak credentials into logs; sensitive validation failures should use a generic reason rather than propagating the validator's message.
Useful? React with 👍 / 👎.
| val resolved = lazy(LazyThreadSafetyMode.SYNCHRONIZED) { | ||
| resolver(variableName) | ||
| } |
There was a problem hiding this comment.
When the resolver throws because a variable is missing, malformed, or fails validation, Kotlin's lazy remains uninitialized and retries the resolver on every later property access. With a mutable custom EnvironmentSource, the same delegate can therefore fail once and later return a value, while converters with side effects run repeatedly, contrary to the advertised once-only, non-refreshing delegate behavior; cache a success-or-failure result explicitly.
Useful? React with 👍 / 👎.
| inline fun <reified T> required( | ||
| name: String, | ||
| noinline converter: (String) -> T, | ||
| sensitive: Boolean = false, | ||
| noinline validation: EnvironmentVariableValidation<T>.() -> Unit = {} | ||
| ): T = requiredConverted(name, targetTypeName<T>(), sensitive, converter, validation) |
There was a problem hiding this comment.
The unconstrained type parameter permits calls such as required<String?>("TOKEN") { null }, so a present required variable can resolve to null even though the API documents required values as non-null. Constrain T to Any (and apply the same constraint to the delegated converter overload) or reject null converter results so required declarations retain their stated type guarantee.
Useful? React with 👍 / 👎.
…e.kts - delete testImplementation and test task configuration from build script
There was a problem hiding this comment.
💡 Codex Review
Redact domain exceptions from sensitive converters
When a custom converter for a sensitive variable throws an EnvironmentVariableException, this branch rethrows it without applying the sensitive-value redaction used for other converter failures. For example, required("SECRET", { throw MissingEnvironmentVariableException(it) }, sensitive = true) exposes the raw secret in the resulting exception message, contradicting the API's guarantee that sensitive values stay out of error details; wrap or sanitize these exceptions when sensitive is true.
Escape remaining control characters in raw values
When an invalid non-sensitive value contains a control character such as ESC (\u001b) or backspace, this fallback copies it directly into the exception message. If that exception is written to a terminal or text log, the value can alter displayed output or erase preceding characters even though newline, carriage return, and tab are already escaped; encode all control characters rather than appending the remainder verbatim.
Forward sensitivity through typed delegates
When a defaulted UUID delegate represents a sensitive value, this overload provides no sensitive parameter and invokes the direct overload with its default of false. Consequently, a malformed present credential is copied into InvalidEnvironmentVariableException even though the equivalent direct call supports sensitive = true; the same omission affects the other typed delegate overloads, so add and forward the sensitivity flag.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
Adds a small Kotlin-first environment-variable API to
surf-api-corewith direct typed reads and lazy property delegates.What changed
env.required("ID", UUID::fromString)require { ... }andrequireIn(...)requiredNonBlankEnvironmentSourceplus map-backed environments for deterministic testsenv.required().named("DATABASE_HOST")Design decisions
Direct calls resolve immediately. Delegates resolve on first access and cache their value using synchronized Kotlin
lazy, including cachednullresults. Enum names are case-sensitive; booleans accept onlytrueorfalse, case-insensitively. Required strings preserve blank values unlessrequiredNonBlankis used.The
.named(...)modifier avoids an ambiguous Kotlin overload between directrequired("NAME")reads and delegates while keeping the implementation smaller than a duplicated named-delegate facade.Deliberate omissions
Validation
./gradlew :surf-api-core:surf-api-core:compileKotlin./gradlew :surf-api-core:surf-api-core:test— 17 tests passed./gradlew :surf-api-core:surf-api-core:updateKotlinAbi./gradlew :surf-api-core:surf-api-core:check— compilation, tests, and ABI check passed