Conversation
CLI applications need a typed way to read environment fallbacks without hand-rolled parsing glue. This introduces @optique/env with createEnvContext(), bindEnv(), and bool(), and adds package-level tests that import @optique/env so the built package surface is exercised.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Optique framework by introducing a dedicated package for environment variable integration, allowing for robust and type-safe configuration management. It also addresses a critical typing bug in the core library that previously affected the correct propagation of required options in complex context scenarios, thereby improving overall type safety and developer experience. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Add docs navigation, README package metadata, and changelog entries so users can discover and adopt @optique/env consistently across the project.
Keep context option inference from collapsing when a context tuple mixes required and non-required sources. Map void-required contexts to never, then fall back to unknown only when no context contributes required options. Add a type-level regression test for mixed contexts and update the env integration example to show direct runAsync() usage without an extra runOptions variable.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the new @optique/env package for environment variable integration, a valuable addition that enhances the library's flexibility. The implementation is thorough, complete with comprehensive documentation and tests. Additionally, the pull request includes a crucial typing fix for ExtractRequiredOptions in @optique/core, which correctly preserves required options when dealing with mixed contexts. The changes are well-executed and align with the project's standards. I've provided a couple of suggestions to address a potential bug and improve maintainability in the new package.
![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: 1fa798f7c7
ℹ️ 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".
When the inner parser fails after consuming tokens (e.g., `--port` without a following value), propagate the failure instead of silently converting it to success. This preserves specific error messages like "requires a value" rather than degrading to a generic "Unexpected option or argument" message. #135 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
When the bound parser is async but the env/default/error path returns a plain object, the ModeValue<"async", T> contract (which expects Promise<T>) is violated. Pass the parser mode to getEnvOrDefault and wrap results in Promise.resolve() when mode is "async". #135 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Extract processResult helper to eliminate repeated logic for handling successful, consumed-failure, and absent-CLI cases in both sync and async code paths. Addresses: #135 (comment)
Add comprehensive documentation sections to match the quality level of the config integration docs: - Prefix and key resolution rules with examples - Using other value parsers (url, port, zod) - Error handling behavior and diagnostics - Full API reference for all exports - Known limitations and design constraints Co-Authored-By: Claude <noreply@anthropic.com>
When the inner parser fails after consuming tokens (e.g., `--port` without a following value), propagate the failure instead of silently converting it to success. This preserves specific error messages like "requires a value" rather than degrading to a generic "Unexpected option or argument" message. #135 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
When the bound parser is async but the env/default/error path returns a plain object, the ModeValue<"async", T> contract (which expects Promise<T>) is violated. Pass the parser mode to getEnvOrDefault and wrap results in Promise.resolve() when mode is "async". #135 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Extract processResult helper to eliminate repeated logic for handling successful, consumed-failure, and absent-CLI cases in both sync and async code paths. Addresses: #135 (comment)
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new @optique/env package for environment variable integration and fixes a typing issue in @optique/core. The implementation of the new package is comprehensive, including tests and documentation. The typing fix for ExtractRequiredOptions is also a valuable improvement, complete with a regression test. My review includes a couple of suggestions: one to clarify a documentation example for better user understanding, and another to align an implementation detail with the project's established coding patterns for handling synchronous and asynchronous logic, as outlined in your style guide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67abb07b66
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR introduces a new @optique/env integration package to provide environment-variable fallbacks for Optique parsers, updates documentation/metadata to expose the integration, and fixes a type-level edge case in @optique/core where required runner options could be lost when mixing contexts.
Changes:
- Added
@optique/envwithcreateEnvContext(),bindEnv(), andbool()plus package scaffolding and tests. - Updated
@optique/coretyping (ExtractRequiredOptions) and added a type regression test. - Wired the new package into workspace/tooling and added docs/navigation/changelog entries.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds workspace link for the new @optique/env package and lock updates. |
| packages/env/tsdown.config.ts | Build configuration for emitting ESM/CJS + d.ts. |
| packages/env/src/index.ts | Implements env context, env binding wrapper, and boolean value parser. |
| packages/env/src/index.test.ts | Adds runtime tests for bool() and bindEnv() behavior. |
| packages/env/package.json | New package metadata, exports, and scripts. |
| packages/env/deno.json | Deno publishing/tasks configuration for the new package. |
| packages/env/README.md | Package-level documentation and quick start. |
| packages/core/src/facade.ts | Fixes ExtractRequiredOptions to handle mixed void/non-void contexts. |
| packages/core/src/facade-types.test.ts | Adds a type-level regression test for mixed-context required options. |
| docs/package.json | Adds @optique/env as a docs workspace dependency. |
| docs/integrations/env.md | New docs page describing env context/binding/boolean parsing. |
| docs/.vitepress/config.mts | Adds env integration to nav + JSR reference links. |
| README.md | Adds @optique/env to feature list and package table. |
| CHANGES.md | Changelog entry for the new integration package. |
| AGENTS.md | Updates architecture overview to include @optique/env. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
packages/env/src/index.ts:301
getEnvOrDefault()only reads fromsourceData?.source(...). When annotations/active-registry are unavailable (e.g., usingbindEnv()without running throughrunWith()/without callingcontext.getAnnotations()), env lookup will never happen even thoughoptions.context.sourceis available. Consider falling back tooptions.context.sourcewhensourceDatais undefined (similar to how prefix falls back tooptions.context.prefix).
const fullKey = `${
sourceData?.prefix ?? options.context.prefix
}${options.key}`;
const rawValue = sourceData?.source(fullKey);
if (rawValue !== undefined) {
const parsed = options.parser.parse(rawValue);
packages/env/src/index.ts:132
createEnvContext()usesSymbol.for()with a random string forcontextId. BecauseSymbol.for()entries are kept in the global symbol registry, repeated context creation can permanently grow the registry in long-running processes. Consider usingSymbol()(orSymbol("@optique/env")) for per-instance IDs and reservingSymbol.for()for stable, shared annotation keys likeenvKey.
export function createEnvContext(options: EnvContextOptions = {}): EnvContext {
const contextId = Symbol.for(`@optique/env:${Math.random()}`);
const source = options.source ?? defaultEnvSource;
const prefix = options.prefix ?? "";
When object() calls bindEnv.parse() on subsequent
iterations, context.state contains the wrapped
{ hasCliValue, cliState } object from a previous call.
The inner parser (e.g., option()) expects its own native
state for features like duplicate detection. Without
unwrapping, duplicate options silently succeed.
Addresses:
#135 (comment)
Extract wrapForMode() to centralize the async/sync dispatch pattern in getEnvOrDefault, replacing three repeated if (mode !== "async") blocks. Addresses: #135 (comment)
Use option("--verbose") instead of
option("--verbose", bool()) since --verbose is a
boolean flag, not an option that expects a value.
Consistent with the object() example below.
Addresses:
#135 (comment)
When object() calls bindEnv.parse() on subsequent
iterations, context.state contains the wrapped
{ hasCliValue, cliState } object from a previous call.
The inner parser (e.g., option()) expects its own native
state for features like duplicate detection. Without
unwrapping, duplicate options silently succeed.
#135 (comment)
Extract wrapForMode() to centralize the async/sync dispatch pattern in getEnvOrDefault, replacing three repeated if (mode !== "async") blocks. #135 (comment)
Use option("--verbose") instead of
option("--verbose", bool()) since --verbose is a
boolean flag, not an option that expects a value.
Consistent with the object() example below.
#135 (comment)
There was a problem hiding this comment.
Code Review
This pull request introduces the new @optique/env package for environment variable integration, which is a great addition. The implementation is thorough, with comprehensive tests and documentation. The PR also includes a necessary type fix for ExtractRequiredOptions in @optique/core, improving type safety for mixed contexts. My review focuses on a few areas for improvement regarding best practices and documentation consistency.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40826efc3a
ℹ️ 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".
Only mark hasCliValue when the inner parser actually consumed input tokens. Wrappers like bindConfig return success with consumed: [] when the CLI option is absent; treating those as "CLI provided" would skip the env fallback entirely. When env is also absent, fall back to parser.complete() so downstream wrappers (e.g., bindConfig) can still supply values. This enables correct composition like bindConfig(bindEnv(...)). #135 (comment)
The outermost wrapper is checked first during completion, so bindEnv(bindConfig(option(...))) gives priority: CLI > Env > Config > Default. #135 (comment)
Symbol.for() pollutes the global symbol registry with non-reusable random keys. Symbol() creates a truly unique, non-shared symbol, which is the correct choice for context instance identity. #135 (comment)
Add yarn command, remove misleading [Deno] label, and fix alignment of package manager commands. #135 (comment)
Align continuation lines with item content in definition list parameters for createEnvContext, bindEnv, and bool. Remove extra blank lines between bindEnv parameter items. #135 (comment) #135 (comment) #135 (comment)
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new @optique/env package for environment variable integration, which is a great addition. The implementation is well-structured, with comprehensive tests and documentation that align with the project's standards. The fix for ExtractRequiredOptions is also a valuable improvement to the core typing. I have one suggestion to improve type safety in the async handling within the new @optique/env package.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/env/src/index.test.ts:394
- There’s no regression test covering the
--option terminator interaction: whenbindEnvwrapsoption()(especially booleanoption("--verbose")) and the args contain--to end options,bindEnvshould not treat that consumption as a CLI-provided value and should still fall back to env/default. Adding a test for this case would prevent thehasCliValuelogic from breaking common--usage.
it("only marks hasCliValue when inner parser consumed tokens", () => {
// A mock inner parser that always succeeds with consumed: [] and
// provides a value through complete(). This simulates wrappers
// like bindConfig or withDefault.
const mockParser = {
$mode: "sync" as const,
$valueType: undefined,
$stateType: undefined,
priority: 0,
usage: [],
initialState: undefined,
parse: (context: { readonly buffer: readonly string[] }) => ({
success: true as const,
next: context,
consumed: [] as string[],
}),
complete: () => ({
success: true as const,
value: "from-inner-complete",
}),
suggest: () => [],
getDocFragments: () => ({ fragments: [] }),
} as unknown as Parser<"sync", string, undefined>;
const context = createEnvContext({
source: () => undefined, // no env value
prefix: "APP_",
});
const parser = bindEnv(mockParser, {
context,
key: "HOST",
parser: string(),
// No default — if hasCliValue is wrongly true, it would call
// mockParser.complete() which returns "from-inner-complete".
// If hasCliValue is correctly false and env is absent, we want
// to fall through to the inner parser's complete() so downstream
// wrappers can provide their value.
});
const result = parse(parser, []);
assert.ok(result.success);
assert.equal(result.value, "from-inner-complete");
});
When the value parser is async, options.parser.parse() returns a Promise. Use instanceof Promise to detect this and return it directly instead of wrapping it with wrapForMode(), which would pass a Promise where a sync Result was expected. #135 (comment)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new @optique/env package for environment variable integration, which is a great addition. The implementation is thorough, with comprehensive tests and documentation. The fix for ExtractRequiredOptions also correctly addresses the typing issue with mixed contexts. I've found one potential issue in the state handling logic within the new bindEnv implementation that could affect composition with certain parsers, which is detailed in the specific comment.
When hasCliValue is true, cliState is always set by processResult(). The extra undefined check could mask issues with parsers that produce undefined as valid state. #135 (comment)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed @optique/env package for environment variable integration, complete with thorough documentation and tests. The included type-level fix for ExtractRequiredOptions is also a valuable improvement. My review focuses on a couple of architectural and type-safety points within the new package to ensure it aligns with the project's established patterns for maintainability and robustness.
Use a per-closure unique symbol to identify EnvBindState objects instead of checking for the hasCliValue property, which could collide with inner parser state shapes. #135 (comment)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new @optique/env package, providing well-designed integration for environment variables. The implementation is thorough, featuring comprehensive documentation and an extensive test suite that covers numerous edge cases. Additionally, it includes a valuable fix for a complex typing issue in @optique/core. The overall quality is high and aligns well with the project's existing architecture. I have one suggestion to improve type safety by replacing a type assertion with a type guard, following the repository's style guide and aiding TypeScript's type narrowing.
Replace `as unknown as EnvBindState` casts in parse() and complete() with an isEnvBindState() type guard function that uses TypeScript's type narrowing instead of unsafe assertions. #135 (comment)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new @optique/env package for environment variable integration, which is a great addition. The implementation is well-documented and comes with a comprehensive test suite. The PR also includes a valuable type-level fix for ExtractRequiredOptions in @optique/core, complete with a regression test. My main feedback is a medium-severity issue in the new @optique/env package, where the implementation for handling synchronous and asynchronous operations deviates from the project's established architectural pattern for mode dispatching. A refactor is recommended to align with the style guide and improve maintainability.
Add two pattern examples demonstrating @optique/env usage: - env-variables.ts: standalone bindEnv with prefix, bool(), and the CLI > env > default fallback chain - env-config-composition.ts: composing bindEnv with bindConfig for the four-level CLI > env > config > default chain Update docs/cookbook.md: - Add "Environment variable fallbacks" section with a standalone @optique/env code sample - Replace the "Combining with environment variables" subsection that used process.env directly with proper bindEnv(bindConfig(...)) composition #135 Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Add a new
@optique/envpackage for environment-variable integration withcreateEnvContext(),bindEnv(), andbool().Implement env fallback behavior with clear precedence (CLI > environment > default) and support env-only values with
fail<T>().Add package-level tests and metadata for dual publishing/build integration (
deno.json,package.json,tsdown.config.ts, lockfile/workspace updates).Document the new integration across docs and project metadata (docs page, docs navigation, README package table, and changelog entries).
Fix
ExtractRequiredOptionsin@optique/coreso mixed context tuples (required + non-required contexts) keep required runner options.Add a type-level regression test for the mixed-context required-options case (
packages/core/src/facade-types.test.ts).Why
@optique/envadds first-class environment support without introducing a separate runner wrapper, keeping the context-based execution model consistent with@optique/config.The
ExtractRequiredOptionsfix addresses a typing edge case where required options likegetConfigPathcould be lost when contexts were mixed, causing unnecessary friction in docs/examples and real-world usage.Closes #86