Revalidate fallback values in bindEnv() and bindConfig()#777
Merged
Conversation
bindEnv() and bindConfig() previously returned fallback values
(environment variables parsed by a looser env-level parser, values
loaded from config files, and configured defaults) directly to the
caller without running them through the bound CLI parser's
validation. Parser constraints like integer({ min }) and
string({ pattern }) could be silently bypassed through any of these
fallback paths.
This commit adds an optional Parser.validateValue() hook that
re-validates a value as if it had been parsed from CLI input. The
primitive parsers option() and argument() implement it by
round-tripping the value through their stored ValueParser's format()
and parse() calls, which re-runs every constraint check. optional()
and withDefault() forward the hook from their inner parser; map()
strips it because the mapping function is one-way; bindEnv() and
bindConfig() forward it from their wrapped parser so nested
compositions like bindEnv(bindConfig(option(...))) propagate
validation down to the primitive.
With the hook in place, bindEnv's getEnvOrDefault() and bindConfig's
getConfigOrDefault() now route every fallback value through
innerParser.validateValue() when available. When the inner parser
does not implement the hook (for example, it sits behind map()), the
wrappers fall through to the current behavior so existing use cases
that pre-validate their defaults are preserved.
Fixes #414.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Close the gaps in the initial bindEnv()/bindConfig() validation fix:
- Skip validateValue on dependency-derived value parsers. Their
format() rebuilds the parser from default dependency values,
so a format+parse round-trip would validate against the wrong
branch.
- Forward validateValue through group() and command() so the
inner primitive's validator survives wrapping.
- Validate env-sourced values and defaults in the dependency-source
extraction path (getEnvSourceValue), closing a gap where a
constraint-violating env value could still reach a derived parser.
- Validate config-sourced values and defaults in the dependency-source
extraction path (getConfigSourceValue) and in the getMissingSourceValue
hook used for source-default fill.
Also adds regression tests for each of the above in the fallback
validation blocks of the env/config test suites, and replaces the
flag-form boolean shortcut in option().validateValue with an explicit
trivial validator so value-parser options always go through the full
format+parse round-trip.
See: #414
Update the env and config integration guides to match the actual behavior now that validateValue also forwards through group() and command(), and is exempt for dependency-derived value parsers: - Add group() and command() to the list of combinators that forward fallback validation. - Note that derived value parsers (derive / deriveFrom) are also exempt alongside map(), and explain why (format() rebuilds from default dependency values rather than live-resolved ones). See: #414 Codex: /home/dahlia/.codex/sessions/019d6a08-7539-7812-aa17-7f721a1c124f
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #777 +/- ##
==========================================
- Coverage 89.53% 89.39% -0.15%
==========================================
Files 43 43
Lines 26564 26904 +340
Branches 6711 6784 +73
==========================================
+ Hits 23785 24051 +266
- Misses 2694 2769 +75
+ Partials 85 84 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 Parser.validateValue() method to @optique/core, enabling re-validation of fallback values from environment variables, configuration files, and defaults against inner CLI parser constraints. The implementation updates bindEnv() and bindConfig() to enforce these constraints and ensures that combinators like optional() and withDefault() forward validation correctly. Feedback identifies a ReferenceError in packages/core/src/primitives.ts involving an undefined variable, a redundant try/catch block in packages/core/src/modifiers.ts that may miss async rejections, and a style guide violation in packages/env/src/index.ts where manual promise checks are used instead of the required mode dispatch patterns.
There was a problem hiding this comment.
Pull request overview
This PR closes a validation gap in Optique’s env/config integrations by ensuring that fallback values (env vars, config values, and configured defaults) are revalidated against the inner CLI parser’s constraints, preventing constraint bypasses described in issue #414.
Changes:
- Added optional
Parser.validateValue()and implemented/forwarded it through relevant primitives and wrappers. - Updated
bindEnv()/bindConfig()to route all fallback values (including dependency-source paths) throughvalidateValue()when available. - Added regression/unit tests and updated integration docs + changelog to describe the new behavior and exemptions.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/env/src/index.ts | Revalidates env/default fallback values via inner parser validateValue() and forwards the hook through bind wrappers. |
| packages/env/src/index.test.ts | Adds regression tests covering fallback revalidation behavior (sync/async, wrappers, dependency-source paths, exemptions). |
| packages/core/src/primitives.ts | Introduces validateValue() for option()/argument() and forwards through command(). |
| packages/core/src/primitives.test.ts | Adds unit tests for validateValue() on primitives. |
| packages/core/src/parser.ts | Extends the Parser interface with optional validateValue() and documents intended forwarding/exemptions. |
| packages/core/src/modifiers.ts | Forwards/strips validateValue() through optional(), withDefault(), and map(). |
| packages/core/src/modifiers.test.ts | Adds tests verifying validateValue() forwarding/stripping behavior in modifiers. |
| packages/core/src/constructs.ts | Documents non-forwarding in branchy constructs and forwards validateValue() through group(). |
| packages/core/src/constructs.test.ts | Updates async parser helper to avoid invalid validateValue spreading across mode boundaries. |
| packages/config/src/index.ts | Revalidates config/default fallback values via inner parser validateValue() (including dependency-source default path). |
| packages/config/src/index.test.ts | Adds regression tests for bindConfig fallback validation behavior and forwarding/exemptions. |
| docs/integrations/env.md | Documents “Fallback validation” behavior and the forwarding/exemption rules. |
| docs/integrations/config.md | Documents “Fallback validation” behavior and the forwarding/exemption rules. |
| CHANGES.md | Changelog entries describing the new validateValue() hook and the bindEnv/bindConfig behavior change. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional Parser.validateValue(value) hook and propagates it through primitives, modifiers, and integration bindings so fallback values are re-validated as if provided on the CLI. Primitive parsers implement validateValue via format()→parse() round-trip; wrappers selectively forward or omit the hook (optional, withDefault, multiple, nonEmpty, group, command forward; map, or, longestMatch, merge, concat do not). bindEnv() and bindConfig() now thread the inner parser into fallback resolution and invoke validateValue when present. Tests and documentation updated to cover forwarding, arity rules, and exempted combinators. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/integrations/config.md`:
- Around line 801-807: Change the fenced block language to "typescript twoslash"
and add a tiny fixture prelude that declares the example variable used by the
snippet (declare the configContext symbol before the snippet body, e.g. a
`declare const configContext: any`/appropriate type placed in the prelude before
the // ---cut-before--- marker) so the VitePress twoslash type-checker sees
configContext and the example remains validated; keep the code inside the block
using the existing bindConfig, option, integer and key "port" identifiers.
In `@docs/integrations/env.md`:
- Around line 370-377: The TypeScript example using bindEnv(...) inside the docs
lacks the twoslash modifier so it isn't type-checked by the VitePress pipeline;
update the fenced code block that contains the bindEnv(...) example (the block
showing option("--name", string({ pattern: /^[A-Z]+$/ })) and parser: string())
to use the twoslash modifier (i.e., change the fence to include "twoslash") so
the snippet is validated and shows TypeScript hover/info in VitePress.
- Around line 388-389: The final warning sentence currently mentions only
derive(), so update it to include deriveFrom() as well (e.g., "derive() and
deriveFrom() will therefore silently bypass fallback validation") to keep the
documentation consistent; locate the sentence referring to derive() in the docs
text (mentions derive and deriveFrom earlier) and amend the wording to reference
both derive() and deriveFrom() so both bypass cases are documented.
In `@packages/core/src/modifiers.ts`:
- Around line 646-668: The multiple() wrapper must implement and forward a
validateValue hook similar to optionalParser: add a validateValue method on the
parser returned by multiple() that, when v is undefined/empty delegates
appropriately or else runs the inner parser.validateValue element-wise and
enforces multiple()'s own min/max rules, returning the wrapped ModeValue;
nonEmpty() should not override or replace validateValue but simply pass through
the inner parser's validateValue (i.e., preserve/forward it unchanged) so
bindEnv()/bindConfig() re-runs the inner validations; apply the same changes in
the corresponding duplicate block around the other occurrence mentioned (the
block at the 1145-1171 region). Ensure you reference and use
parser.validateValue, innerValidate bound to the inner parser, and return
ModeValue<M, ValueParserResult<...>> just like the optionalParser
implementation.
In `@packages/core/src/primitives.test.ts`:
- Line 7175: Replace the type-obscuring cast on the test call to
parser.validateValue! by using an explicit TypeScript negative-test directive:
remove `"purple" as "red"` and instead put `// `@ts-expect-error`` immediately
before the direct call `parser.validateValue!("purple")`, preserving the
non-null assertion and making the intentional type violation explicit to match
the file's existing pattern.
In `@packages/core/src/primitives.ts`:
- Around line 1410-1478: The validateValue implementation currently returns raw
ValueParser.parse failures which bypass option()/argument() error formatting;
update validateValue (the non-derived branch that defines result.validateValue)
to catch parse errors and forward them through the same error formatter/handler
used by option().complete()/argument().complete() instead of returning raw
failures — i.e., wrap vp.parse(...) and the async branch inside try/catch and on
parse failure convert the thrown/returned error into the existing
errors.invalidValue format (preserving the CLI prefixes/metavar) before
returning via wrapForMode/dispatchByMode; ensure both sync and async parse paths
and sentinel/default early-return branches use the shared formatter so
bindEnv()/bindConfig() revalidation yields identical formatted errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11dfb2ce-9b95-42fa-a478-a1df5581fc1f
📒 Files selected for processing (14)
CHANGES.mddocs/integrations/config.mddocs/integrations/env.mdpackages/config/src/index.test.tspackages/config/src/index.tspackages/core/src/constructs.test.tspackages/core/src/constructs.tspackages/core/src/modifiers.test.tspackages/core/src/modifiers.tspackages/core/src/parser.tspackages/core/src/primitives.test.tspackages/core/src/primitives.tspackages/env/src/index.test.tspackages/env/src/index.ts
CLI-sourced parser failures are prefixed with the option name or the argument metavar (and respect options.errors.invalidValue when set) via a formatInvalidValueError helper defined inside option().complete() / argument().complete(). Until now, validateValue() did not share that formatter, so fallback revalidation from bindEnv() / bindConfig() would surface the raw ValueParser.parse() error without the option/metavar prefix or the errors.invalidValue customization. Hoist formatInvalidValueError out of complete() into the enclosing option() / argument() scope so validateValue() can apply the same wrapping to round-trip parse failures. CLI-sourced and fallback-sourced errors are now identical, which also makes the CHANGES.md claim that failures are rejected "with the same error that a CLI-sourced value would produce" accurate. Also clarify the flag-form option() comment which previously claimed "the only runtime value is `true`" even though complete() returns `false` for the missing case. Adds tests asserting option()/argument() prefixes, the errors.invalidValue override, and end-to-end bindEnv/bindConfig fallback errors including the --port prefix; the choice() unit test switches from the `"purple" as "red"` cast to an explicit @ts-expect-error directive, matching the repository's negative-type convention. Review comments addressed: - #777 (comment) - #777 (comment) - #777 (comment) - #777 (comment) - #777 (comment) - #777 (comment)
…Value
The try/catch wrapper around withDefault() validateValue forwarding is
redundant: option() / argument() validateValue already swallow format()
exceptions internally for sentinel defaults, so the only thing the
outer try/catch would catch is a hypothetical programming error inside
the inner validator. It also fails to catch async rejections from the
returned Promise, so it never fully delivered on the safety it was
trying to provide. Delegate directly instead.
getEnvSourceValue() previously checked `parsed instanceof Promise` to
route sync vs. async validation through the inner parser. This is
against the repository's mode-dispatch rule ("All mode-based type
assertions are isolated in mode-dispatch.ts" in CLAUDE.md), so switch
to mapModeValue() keyed on the env value parser's own \$mode instead.
Review comments addressed:
- #777 (comment)
- #777 (comment)
Until now, multiple() and nonEmpty() did not expose a validateValue
hook, which meant bindConfig(multiple(option("--role",
choice(["admin","user"])))) would silently accept ["root"] from a
config file because the inner choice() constraint never ran on
fallback values. The same hole applied to multiple()'s own min/max
arity rules: a default shorter than min or longer than max slipped
through bindEnv() / bindConfig() fallback paths.
multiple().validateValue() now:
- runs the inner parser's validateValue() on each array element when
available (returning the first element failure), and
- re-checks the array length against the configured min / max bounds
regardless of whether the inner parser has a validator attached.
nonEmpty().validateValue() passes through unchanged from the inner
parser — nonEmpty() is state-shape preserving and only constrains
parse-time token consumption, so there is nothing of its own to
enforce against a fallback value. Users who need an arity-1 check on
fallback values should use `multiple(..., { min: 1 })` instead.
Adds forwarding tests in modifiers.test.ts and an end-to-end bindConfig
integration test that loads an array containing an out-of-set element
and another asserting the multiple() min rule on defaults. CHANGES.md
and the env/config integration guides now mention multiple() /
nonEmpty() alongside the other forwarding combinators.
Review comment addressed:
- #777 (comment)
The new bindEnv and bindConfig fallback-validation examples were plain \`typescript\` fences, so the VitePress build no longer type-checked them against live Optique types. The config example also relied on an undeclared \`configContext\` symbol. Promote both examples to \`typescript twoslash\` and add a short fixture prelude (imports plus the context construction) before the \`// ---cut-before---\` marker so the docs pipeline validates them while the reader still sees only the bind call. Review comments addressed: - #777 (comment) - #777 (comment)
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/integrations/env.md`:
- Around line 387-391: Replace the source-specific phrase "config-loaded array"
with a source-neutral term (e.g., "fallback array" or "resolved array") in the
docs text that discusses multiple() validation; update the sentence that
currently reads "a config-loaded array that contains..." so it instead reads
something like "a fallback array that contains an invalid element..." to make
the description apply to both bindEnv() and bindConfig() cases while keeping
references to multiple(), bindEnv(), and bindConfig().
In `@packages/config/src/index.test.ts`:
- Around line 357-371: The test currently only asserts !result.success after
calling parse(parser, []); instead, assert the actual diagnostic from the failed
parse to ensure the failure is due to the inner string pattern (e.g., check
result.success === false and that result.error/diagnostic contains the pattern
validation message or the formatted diagnostic from the inner parser). Update
the test that constructs parser via bindConfig(option("--name", string({
pattern: /^[A-Z]+$/ })), { context, key: "name", default: "abc" as never }) to
assert the specific error fragment (from result.error or the parser's diagnostic
formatter) that indicates the pattern constraint failed rather than relying
solely on success boolean; apply the same change to the other listed tests that
use parse(parser, []) so they assert the stable diagnostic text from the inner
parser.
In `@packages/core/src/modifiers.test.ts`:
- Around line 8022-8041: Add a regression test that verifies
nonEmpty(multiple(...)).validateValue is a direct pass-through to the inner
multiple's validation for empty arrays: construct two inner parsers with
multiple(option("-x", integer(...))) where one has min:0 and the other has
min:1, wrap each with nonEmpty(...), call validateValue!([]) on both wrapped
parsers, and assert that the results match the inner multiple's
validateValue([]) behavior (first succeeds, second fails), referencing the
nonEmpty, multiple and validateValue symbols to locate the code to update.
- Around line 7922-8042: Add tests that exercise async-mode validateValue
forwarding: create async parsers using option("--format", asyncChoice(...)) (or
similar asyncChoice) and wrap them with optional, withDefault, multiple and
nonEmpty, then call parser.validateValue! with Promise-resolving inputs and
assert the returned result shape and success/failure mirrors the inner async
parser (e.g., invalid value yields success: false, undefined passes for
optional, default is accepted for withDefault, arrays validated element-wise for
multiple, empty rejected for nonEmpty). Ensure you include a map case to confirm
async mapping strips validateValue and verify validateValue is undefined for
map-wrapped async parsers.
In `@packages/core/src/modifiers.ts`:
- Around line 2329-2350: The validateArity error messages are inconsistent with
validateMultipleResult; update validateArity (the validateArity function) to use
the same number formatting and wording as validateMultipleResult: replace
String(min), String(max) and String(values.length) usage with
number.toLocaleString("en") (e.g., min.toLocaleString("en"),
max.toLocaleString("en"), values.length.toLocaleString("en")) and adjust
phrasing to match ("but got only ..." instead of "but got ...") so both fallback
validation and CLI parsing produce identical formatted messages.
In `@packages/env/src/index.ts`:
- Around line 543-574: Update the JSDoc for the exported bindEnv() function(s)
to document that the inner parser's validateValue() hook may be invoked during
fallback resolution and therefore can throw from within bindEnv(); add an
`@throws` tag that references innerParser.validateValue / validateSync /
validateAsync as the source of the exception and note that thrown errors can
occur even when no CLI tokens are parsed (fallback/default path). Ensure both
exported overloads/variants of bindEnv (the docblocks around the bindEnv
declaration and the second docblock at the later overload, lines referenced in
review) are updated so all public bindEnv JSDoc blocks include this `@throws`
entry describing the exception path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37b05e86-7279-4f41-8a5c-4f90cac3f6a2
📒 Files selected for processing (10)
CHANGES.mddocs/integrations/config.mddocs/integrations/env.mdpackages/config/src/index.test.tspackages/core/src/modifiers.test.tspackages/core/src/modifiers.tspackages/core/src/primitives.test.tspackages/core/src/primitives.tspackages/env/src/index.test.tspackages/env/src/index.ts
Both integration guides described a \`multiple()\` fallback array as "config-loaded", but the sentence lives in a section that covers \`bindEnv()\` and \`bindConfig()\` equally. The env guide in particular made the behavior sound specific to \`bindConfig()\`. Rename to "fallback array" so the description applies to both env and config fallback paths. Review comment addressed: - #777 (comment)
multiple().validateValue() was constructing arity failures with its
own inline messages: \`String(min)\` formatting, a "but got ..."
phrasing, and no honoring of \`options.errors.tooFew\` / \`tooMany\`.
The CLI path (\`validateMultipleResult\` inside the result-building
branch) already uses \`.toLocaleString("en")\`, a "but got only"
phrasing for the too-few branch, and routes through the custom
error messages. This left fallback validation errors visibly
different from CLI errors for the same violation.
Rewrite \`validateArity\` to mirror that branch exactly, so the same
\`multiple({ min, errors })\` configuration produces identical error
text regardless of whether the violation was caught during CLI
parsing or during \`bindEnv()\` / \`bindConfig()\` fallback
revalidation.
Adds tests that lock in the formatting (min violation matches
\`"Expected at least 3 values, but got only 1."\`) and that the
\`errors.tooFew\` and \`errors.tooMany\` customizations flow through
\`validateValue\` the same way they flow through \`complete()\`.
Review comment addressed:
- #777 (comment)
Address three CodeRabbit review comments:
- Tests that assert parse failure without inspecting the error could
pass even if the rejection came from an unrelated code path. Lock
in the actual pattern-violation / bounds-violation errors by
formatting the message and checking it mentions the offending
option name and the relevant constraint.
- Add a regression test for the `nonEmpty(multiple(..., { min: 0 }))`
empty-array case, documenting that `nonEmpty()` delegates fallback
arity entirely to the inner parser instead of layering an extra
non-empty check on top.
- Cover async mode explicitly for `optional()`, `withDefault()`,
`multiple()`, `nonEmpty()`, and `map()` to prove `validateValue`
forwarding (and the `map()` strip) behave the same way when the
inner value parser is async.
See:
- #777 (comment)
- #777 (comment)
- #777 (comment)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Since the fallback-validation fix for #414 landed, both bindEnv() and bindConfig() invoke the inner parser's validateValue() hook while resolving environment/config/default values. A throwing inner validator now surfaces out of bindEnv()/bindConfig() even when no CLI tokens are parsed, so the public JSDoc must list that exception path. Per the project guideline that every function/method which can throw must document the throw via @throws, extend the existing @throws entries for both exported wrappers to name validateValue() explicitly and cross-reference issue #414. See: #777 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@codex review |
Owner
Author
|
/gemini review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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.
Code Review
This pull request introduces the Parser.validateValue() method to the core library, enabling bindEnv() and bindConfig() to re-validate fallback values—such as environment variables, configuration files, and defaults—against the constraints of the inner CLI parser. The implementation includes logic to forward this validation through common combinators like optional(), withDefault(), and multiple(), while intentionally excluding it from one-way transformations like map(). Feedback was provided regarding the multiple() modifier's validation logic, specifically to ensure that non-array inputs are rejected with descriptive error messages that include the received type.
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 (2)
packages/env/src/index.ts (1)
643-666:⚠️ Potential issue | 🟡 MinorAdd the
validateValue()exception path to this helper’s JSDoc.
getEnvSourceValue()now revalidates successful fallbacks viainnerParser.validateValue(), so it can throw from that path in addition to the env-source/value-parser calls already documented here.As per coding guidelines, "Functions or methods that throw exceptions must include the
@throwstag in their JSDoc comments."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/env/src/index.ts` around lines 643 - 666, The JSDoc for getEnvSourceValue is missing the new exception path: update its `@throws` block to also document that innerParser.validateValue() can throw when revalidating fallback values (in addition to errors from sourceData.source(fullKey) and options.parser.parse(rawValue)); reference getEnvSourceValue and innerParser.validateValue in the description so callers know validation errors propagate from that hook.packages/config/src/index.ts (1)
971-986:⚠️ Potential issue | 🟡 MinorDocument the new
validateValue()throw path on these fallback helpers.
getConfigOrDefault(),validateFallbackValue(), andgetConfigSourceValue()now propagate exceptions frominnerParser.validateValue(), but the updated docblocks still only document the key-callbackTypeError(or nothing for the new helper). Add explicit@throwsentries so the helper contract matches the implementation.As per coding guidelines, "Functions or methods that throw exceptions must include the
@throwstag in their JSDoc comments."Also applies to: 1057-1075, 1078-1101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/index.ts` around lines 971 - 986, Update the JSDoc for the fallback helper functions to document that exceptions thrown by innerParser.validateValue() are propagated: add an explicit "@throws {Error}" (or more specific error type if known) to the docblocks for getConfigOrDefault, validateFallbackValue, and getConfigSourceValue indicating that validation errors from innerParser.validateValue() will be thrown; do the same for the related docblocks in the other ranges noted (lines ~1057-1075 and ~1078-1101) so each helper's contract matches the implementation and mentions both the key-callback TypeError and the validateValue propagation.
♻️ Duplicate comments (1)
packages/config/src/index.test.ts (1)
461-475:⚠️ Potential issue | 🟡 MinorPin the failure cause in the remaining fallback-validation tests.
These cases still pass on any failure, including an unrelated missing-value/default path. Assert a stable diagnostic fragment—or compare against the equivalent CLI failure result—so the tests prove the new
validateValue()forwarding is what failed.Based on learnings, "when verifying behavioral equivalence between two parser configurations (e.g., uuid() default vs uuid({ strict: true })), prefer asserting deep equality of the entire parse results ... rather than only comparing .success, so differences in returned values or diagnostics are detected."
Also applies to: 477-494, 514-538, 540-563, 565-589, 591-625, 681-698
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/index.test.ts` around lines 461 - 475, The test currently only checks result.success and can pass for unrelated failures; update the failing tests (e.g., the case using createConfigContext + bindConfig + optional(option("--name", string(...))) and other similar blocks) to assert a stable diagnostic fragment or perform deep equality against the equivalent CLI parse result instead of only checking .success. Locate the parser creation (bindConfig/optional/option), call parse(parser, []) as before, then compare result.diagnostics (or the full result object) to the expected diagnostics/parse result from the CLI-equivalent run so the assertion pins the validateValue() forwarding failure rather than any missing-default path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/integrations/config.md`:
- Around line 818-832: The paragraph incorrectly implies wrapping a parser in
map()/derive()/deriveFrom() disables all fallback checks and that nonEmpty()
enforces array non-emptiness itself; update the text to state precisely that
map()/derive()/deriveFrom() suppress revalidation of the inner parser's
validateValue only (so inner value-level validation is skipped), while outer
combinators like multiple() always perform their own checks
(multiple().validateValue enforces min/max arity unconditionally), and
nonEmpty() merely forwards the inner validateValue hook unchanged rather than
adding its own arity enforcement. Mention the specific symbols map, derive,
deriveFrom, multiple, nonEmpty, and validateValue so readers can locate the
behavior.
In `@packages/core/src/modifiers.test.ts`:
- Line 8113: Replace the typed casts that mask intentionally invalid inputs by
changing the test calls like parser.validateValue!("xml" as "json") to use an
explicit as never cast (e.g., "xml" as never) so the runtime validation tests
clearly signal invalid-type inputs; update all occurrences where
parser.validateValue! (and similar test invocations at the noted spots) use
"xml" as "json" to use as never instead.
In `@packages/core/src/modifiers.ts`:
- Around line 2371-2376: The current branch in modifiers.ts returns success when
a fallback for a multiple(...) parser is not an array (the if
(!Array.isArray(values)) block), which lets mis-typed defaults slip through
bindEnv()/bindConfig(); instead, change that branch to reject non-array
fallbacks by returning a failure ModeValue for parser.$mode (i.e., a
ModeValue<M, ValueParserResult<readonly TValue[]>>) with success: false and a
clear error describing "expected array for multiple(...) but got <type/value>",
so validateValue() remains the guardrail and multiple(...) only accepts arrays
from defaults/fallbacks.
---
Outside diff comments:
In `@packages/config/src/index.ts`:
- Around line 971-986: Update the JSDoc for the fallback helper functions to
document that exceptions thrown by innerParser.validateValue() are propagated:
add an explicit "@throws {Error}" (or more specific error type if known) to the
docblocks for getConfigOrDefault, validateFallbackValue, and
getConfigSourceValue indicating that validation errors from
innerParser.validateValue() will be thrown; do the same for the related
docblocks in the other ranges noted (lines ~1057-1075 and ~1078-1101) so each
helper's contract matches the implementation and mentions both the key-callback
TypeError and the validateValue propagation.
In `@packages/env/src/index.ts`:
- Around line 643-666: The JSDoc for getEnvSourceValue is missing the new
exception path: update its `@throws` block to also document that
innerParser.validateValue() can throw when revalidating fallback values (in
addition to errors from sourceData.source(fullKey) and
options.parser.parse(rawValue)); reference getEnvSourceValue and
innerParser.validateValue in the description so callers know validation errors
propagate from that hook.
---
Duplicate comments:
In `@packages/config/src/index.test.ts`:
- Around line 461-475: The test currently only checks result.success and can
pass for unrelated failures; update the failing tests (e.g., the case using
createConfigContext + bindConfig + optional(option("--name", string(...))) and
other similar blocks) to assert a stable diagnostic fragment or perform deep
equality against the equivalent CLI parse result instead of only checking
.success. Locate the parser creation (bindConfig/optional/option), call
parse(parser, []) as before, then compare result.diagnostics (or the full result
object) to the expected diagnostics/parse result from the CLI-equivalent run so
the assertion pins the validateValue() forwarding failure rather than any
missing-default path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e8ed2fc-b45e-4f58-9096-f95fa5952673
📒 Files selected for processing (7)
docs/integrations/config.mddocs/integrations/env.mdpackages/config/src/index.test.tspackages/config/src/index.tspackages/core/src/modifiers.test.tspackages/core/src/modifiers.tspackages/env/src/index.ts
Fallback validation is the only barrier between a mis-typed default (e.g., one escaped through \`as never\`) and the parsed result now that bindEnv() / bindConfig() re-run inner validators on fallback paths. multiple() can never produce a non-array shape from CLI input, so a non-array fallback is always a type error and must be surfaced instead of silently passed through. Replace the non-array escape hatch in multiple().validateValue with an explicit failure that names the received type so users can locate the offending source. Cover both "string" and "null" shapes in a new regression test. See: - #777 (comment) - #777 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous paragraph in both env and config docs said validation
was forwarded through \`nonEmpty()\` and claimed that wrapping an
inner parser in \`map()\` / \`derive()\` / \`deriveFrom()\` would
\"silently bypass fallback validation\". Both read the shipped
behavior imprecisely:
- \`nonEmpty()\` does not add its own arity check on the fallback
path; it merely forwards the inner parser's \`validateValue\`
hook unchanged. Listing it alongside forwarding combinators
implied users get an empty-array rejection for free from
\`nonEmpty(multiple(...))\`, which is not the case.
- \`map()\` / \`derive()\` / \`deriveFrom()\` only strip the inner
parser's \`validateValue\`. Outer combinators layered above them
(notably \`multiple()\`'s own unconditional arity enforcement)
still apply to fallback values.
Rewrite the paragraph to split the three behaviors — plain
forwarding, \`multiple()\`'s unconditional arity + non-array
rejection, \`nonEmpty()\`'s pass-through, and the \`map()\` / derive
strip — so readers can tell exactly which constraints survive each
wrapper.
See: #777 (comment)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The \`toAsyncParser()\` test helper destructures \`validateValue\` off the sync parser and re-attaches it on the async wrapper. Before this change it called the destructured function directly, dropping whatever \`this\` binding the original implementation might have relied on. Other forwarding sites in the codebase (e.g., \`bindEnv()\`, \`bindConfig()\`, \`optional()\`) explicitly bind \`parser\` when forwarding \`validateValue\`. Optique's own primitives don't use \`this\` today, but keeping the helper consistent with the rest of the codebase prevents third-party custom parsers from subtly breaking when their sync variant is tested through the async wrapper. Use \`validateValue.call(parser, v)\` since we already hold the destructured reference. See: #777 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The flag-form \`option()\` test for \`validateValue\` previously stated \"the only valid value is \`true\`\" and only asserted the \`true\` branch. \`option().complete()\` actually yields \`false\` when the flag is missing, and the attached trivial validator accepts both booleans, so the comment was misleading and the coverage one-sided. Rewrite the comment to reflect the shipped semantics and add an assertion that \`validateValue(false)\` also returns success. See: #777 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The async-mode validateValue tests used \`\"xml\" as \"json\"\` to force a type-level invalid value through the \`choice(['json', 'yaml'])\` validator. That cast obscures the intent — the literal is being coerced into a type it cannot structurally satisfy — while \`as never\` is the project's established signal for \"this is deliberately an invalid-type test input\" (already used in the bindEnv/bindConfig fallback validation tests added earlier in this PR). Replace all four occurrences in the async-mode block so the invalid-input tests are easier to grep for and match the repo-wide convention. See: #777 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@codex review |
Owner
Author
|
/gemini review |
Per the project guideline that every function which can throw must document the throw via @throws, extend the docblocks of the internal fallback helpers so their contracts match the implementation: - \`getEnvSourceValue()\` in @optique/env — already mentioned throws from the env source callback and \`options.parser.parse()\`, but not the \`innerParser.validateValue()\` call added in #414. - \`getConfigOrDefault()\` in @optique/config — only mentioned the \`TypeError\` thrown when a key callback returns a thenable. - \`validateFallbackValue()\` in @optique/config — had no @throws entry at all even though it is the actual call site for \`innerParser.validateValue()\`. - \`getConfigSourceValue()\` in @optique/config — only mentioned the key-callback \`TypeError\`. Each docblock now explicitly names \`innerParser.validateValue()\` as a possible throw source and cross-references issue #414 so callers know validation errors can escape from these helpers. See: #777 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the Parser.validateValue() method to enforce CLI-level constraints (such as regex patterns and numeric bounds) on fallback values sourced from environment variables, configuration files, and defaults. The implementation includes updates to bindEnv() and bindConfig() to trigger re-validation, along with forwarding logic for standard combinators and modifiers. Review feedback identifies a type mismatch in the validator for flag-form options when operating in asynchronous mode and highlights several instances where manual mode-based type assertions should be replaced by existing helpers to comply with the repository's style guide.
![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: e24e3b8a71
ℹ️ 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.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/integrations/env.md`:
- Around line 361-362: The doc line claiming "Since Optique 1.0.0" is incorrect
for the fallback revalidation change; update the sentence in
docs/integrations/env.md that begins with "Since Optique 1.0.0" to either the
correct release that includes PR `#777` (replace "Optique 1.0.0" with the actual
release version) or remove the "Since ..." clause entirely if the release is not
finalized, and optionally mention "PR `#777`" for traceability; locate the
sentence around the phrase "fallback values produced by `bindEnv()` are
re-validated" and edit it accordingly.
In `@packages/core/src/modifiers.ts`:
- Around line 2397-2413: multiple().validateValue() is returning the original
values array instead of the inner parser's normalized results; update both
branches inside dispatchByMode (the sync branch where innerValidate is called
and the async branch where await innerValidate is used) to collect each
successful r.value into a new array (e.g., normalizedValues) and return that
normalized array as the value in the success result instead of returning the
original values variable; ensure types remain ModeValue<M,
ValueParserResult<readonly TValue[]>> and keep the early-return behavior on
r.success === false.
In `@packages/core/src/primitives.test.ts`:
- Around line 7148-7162: The test currently skips assertions if
parser.validateValue is missing, letting regressions pass; update the boolean
option test to assert that parser.validateValue exists before using it by
explicitly failing when typeof parser.validateValue !== "function" (e.g.,
assert.ok(typeof parser.validateValue === "function", "...")), then continue to
call parser.validateValue(true) and parser.validateValue(false) and assert their
success results; reference the parser.validateValue checks in the boolean option
test so the test fails hard if the hook is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47908938-36aa-4d09-91cc-7fd84fb8e634
📒 Files selected for processing (6)
docs/integrations/config.mddocs/integrations/env.mdpackages/core/src/constructs.test.tspackages/core/src/modifiers.test.tspackages/core/src/modifiers.tspackages/core/src/primitives.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/env/src/index.ts (1)
547-618:⚠️ Potential issue | 🟡 MinorAdd a
@throwsdocblock forgetEnvOrDefault().This helper now calls
sourceData.source(),options.parser.parse(), andinnerParser.validateValue(), but it still has no JSDoc@throws. Please document that internal throw contract here as well.Suggested doc fix
+/** + * Resolves an env-backed fallback value from the environment or the + * configured default. + * + * `@throws` {Error} Propagates errors thrown by the env source callback + * (`sourceData.source(fullKey)`), `options.parser.parse()`, + * or `innerParser.validateValue()` while re-validating + * fallback values (see issue `#414`). + */ function getEnvOrDefault<M extends Mode, TValue>( state: unknown, options: BindEnvOptions<M, TValue>, mode: M,As per coding guidelines, "Functions or methods that throw exceptions must include the
@throwstag in their JSDoc comments."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/env/src/index.ts` around lines 547 - 618, Add a JSDoc `@throws` entry to the getEnvOrDefault() comment that documents the internal throw contracts: note it can throw errors from sourceData.source() (when reading the env source), from options.parser.parse() (sync or async parsing), and from innerParser.validateValue() (validation failures), and include brief descriptions for each to aid callers; update the existing JSDoc directly above getEnvOrDefault() to list these three possible throw sources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/env/src/index.ts`:
- Around line 547-618: Add a JSDoc `@throws` entry to the getEnvOrDefault()
comment that documents the internal throw contracts: note it can throw errors
from sourceData.source() (when reading the env source), from
options.parser.parse() (sync or async parsing), and from
innerParser.validateValue() (validation failures), and include brief
descriptions for each to aid callers; update the existing JSDoc directly above
getEnvOrDefault() to list these three possible throw sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a54fda0-0371-4625-94b7-c73df25b447b
📒 Files selected for processing (2)
packages/config/src/index.tspackages/env/src/index.ts
The flag-form \`option()\` \`validateValue\` implementation had three related issues called out in the latest review round: - The return type was hardcoded to \`ModeValue<\"sync\", ...>\` with a non-\`wrapForMode\` sentinel cast, which violates the declared \`ModeValue<M, ...>\` contract even though at runtime M is always \"sync\" for a valueless option. - The validator accepted any value unconditionally, so a \`bindEnv()\` / \`bindConfig()\` fallback coming from an \`unknown\`-typed config / env source (e.g., a string \"yes\") would leak through as the parsed result even though the CLI parser can only ever produce a boolean. - The existing test gated assertions behind \`if (typeof parser.validateValue === \"function\")\`, so the #414 regression contract would silently disappear if the hook were ever dropped. Rework the validator to: - Use \`wrapForMode(mode, ...)\` so the returned shape matches \`ModeValue<M, ValueParserResult<boolean>>\`, matching the pattern used for the non-flag branch just below. - Reject non-boolean inputs with an option-scoped error produced via the shared \`formatInvalidValueError\` helper (so it picks up the \`options.errors.invalidValue\` customization uniformly). Tighten the existing test to hard-fail on a missing hook and add a new regression test that covers both string and number fallback rejection with an assertion on the option-name prefix. See: - #777 (comment) - #777 (comment) - #777 (comment) - #777 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The per-element loop in \`multiple().validateValue\` walked each element through the inner parser's \`validateValue\` but then threw away the returned \`r.value\` and returned the original \`values\` array. If the inner parser canonicalizes on a successful round-trip — for example a URL parser that strips a trailing slash, or a string parser that folds casing — the CLI parse path uses the canonical value but the fallback path would silently leak the pre-canonical one, breaking the \"same semantics as CLI\" guarantee that issue #414's fix is supposed to provide. Collect each successful \`r.value\` into a new array, track whether any element actually changed, and return the new array only when a change occurred (so the common \"already canonical\" path does not pay for a pointless copy). Mirror the fix across both the sync and async dispatch branches. Regression tested with an inline uppercasing \`ValueParser\` (one sync, one async) that feeds \`[\"hello\", \"world\"]\` into the fallback path and asserts the returned array is \`[\"HELLO\", \"WORLD\"]\`. See: - #777 (comment) - #777 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The project style guide asks that any \`as ModeValue<M, T>\` assertions live inside \`mode-dispatch.ts\`, because the helpers exported from there (\`wrapForMode\`, \`dispatchByMode\`, \`mapModeValue\`) already encode the unsafe narrowing once and return a properly typed \`ModeValue<M, T>\`. The fallback-validation work added in #414 left three fresh hand-written casts behind that the helpers can absorb instead: - \`option()\` -> \`argument()\` \`validateValue\` in \`primitives.ts\`: the three \`wrapForMode(vpMode, ...) as ModeValue<M, ValueParserResult<T>>\` and \`dispatchByMode(...) as ModeValue<M, ...>\` sites can be replaced with explicit generic parameters on the helper calls. - \`optional()\` \`validateValue\` in \`modifiers.ts\`: the \`wrapForMode(..., { success: true, value: v })\` cast for the \`undefined\` branch can be passed as an explicit generic, and the \`innerValidate(v) as ModeValue<...>\` cast is redundant since \`ValueParserResult<TValue>\` is structurally a subtype of \`ValueParserResult<TValue | undefined>\` under \`ModeValue<M, …>\`. - \`multiple()\` \`validateValue\` in \`modifiers.ts\`: four casts on \`wrapForMode\` / \`dispatchByMode\` calls collapse into explicit generic parameters. - \`withDefault()\` dependency-source branch in \`modifiers.ts\`: the \`mapModeValue(...) as ModeValue<...>\` cast collapses into an explicit generic parameter. No behavior change; the full test suite still passes. Other \`as ModeValue<...>\` sites in the codebase (e.g., the facade / \`parse()\` API surface and the internal helpers inside \`mode-dispatch.ts\`) are outside the review scope and left alone. See: - #777 (comment) - #777 (comment) - #777 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@codex review |
Owner
Author
|
/gemini review |
\`getEnvOrDefault()\` had no JSDoc at all — just a lowercase comment inside the function body describing the inline \`validateSync\` / \`validateAsync\` helpers. Per the project guideline that every function which can throw must document the throw via \`@throws\`, attach a proper JSDoc block that lists the four sources of thrown errors the helper propagates: - \`sourceData.source(fullKey)\` (env source callback, sync/async). - \`options.parser.parse(rawValue)\` (env value parser, sync/async). - \`innerParser.validateValue()\` when re-validating a successful env-sourced value or the configured \`default\`. - \`innerParser.complete()\` when falling through to the inner parser because neither env nor default is set (important for nested \`bindEnv(bindConfig(...))\` composition). Also document the parameter list so future readers don't have to guess why \`innerParser\`, \`innerState\`, and \`exec\` are optional. See: #777 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the Parser.validateValue() method to the @optique/core package, enabling the re-validation of fallback values—such as environment variables, configuration file entries, and hardcoded defaults—against a parser's underlying constraints. The implementation updates bindEnv() and bindConfig() to enforce these constraints and includes logic in combinators like optional(), withDefault(), and multiple() to forward or implement validation. Documentation and extensive regression tests for issue #414 have been added across the affected packages. I have no feedback to provide.
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #414.
Until this change,
bindEnv()andbindConfig()handed fallback values (environment variables parsed through an env-levelparser, config values extracted from a loaded file, and the configureddefault) back to the caller without running them through the inner CLI parser's validation. As a result, constraints attached to the inner parser (regex patterns, numeric bounds,choice()values, and so on) could be silently bypassed whenever a value came from a source other than the CLI. The issue reproduction showed thatbindEnv(option("--name", string({ pattern: /^[A-Z]+$/ })), { parser: string(), default: "abc" })would happily accept"abc"even though that same value would be rejected on the command line. This was both a correctness hole and a validation gap that could quietly accept invalid data in production.The fix introduces a new optional method on the
Parserinterface,validateValue(), which re-checks whether an arbitrary value satisfies the parser's underlyingValueParserconstraints. Primitive parsers (option()andargument()) implement it as aformat()+parse()round-trip: the value is stringified through the sameValueParser.format()that would be used to display it, and the resulting string is fed back throughValueParser.parse()so that every constraint the user attached to the parser is applied. Combinators that can meaningfully forward validation (optional(),withDefault(),group(),command()) now passvalidateValue()through from their inner parser, and outer wrappers like nestedbindEnv(bindConfig(...))compositions forward it as well.map()explicitly strips it, because the mapping function is one-way and the mapped output type no longer corresponds to the inner parser's constraints.or(),longestMatch(),merge(), andconcat()deliberately do not forward because the active branch is not known at validation time.bindEnv()andbindConfig()then use this new hook to revalidate every fallback value they produce. The env-levelparser(which is often intentionally looser than the inner CLI parser) no longer has the final word: once it successfully parses an env var, the result is piped through the inner parser'svalidateValue()so the stricter constraints are applied. Configureddefaults receive the same treatment. The same is true for the dependency-source extraction path, so a constraint-violating env or config value cannot leak to a derived parser either. InbindConfig(), thegetMissingSourceValuehook that feeds defaults into the dependency runtime is also routed through the validator.Dependency-derived value parsers (
derive/deriveFrom) are explicitly exempt from validation attachment. Theirformat()has to rebuild the parser from default dependency values rather than the live-resolved ones, so a round-trip would end up validating against the wrong branch. The implementation guards against this withisDerivedValueParser()checks in bothoption()andargument(), and the docs and changelog now spell that out alongside the existingmap()exemption.Here is a concrete example of what flips with this change:
Before this change the
"abc"default was silently accepted. After this change it is rejected with the same error that a CLI-sourced value would produce. The same holds for integer bounds, choices, and any other constraint expressible through a value parser.This is a behavior change for any code that previously relied on constraint-violating env/config values or defaults being accepted. The changelog calls this out under
@optique/core,@optique/env, and@optique/config. The documentation under docs/integrations/env.md and docs/integrations/config.md gained a new "Fallback validation" subsection, and thebindEnv()/bindConfig()API reference entries now mention that fallback values are revalidated.The work followed the TDD workflow: regression tests reproducing the issue were written first against
main, confirmed to fail, then the implementation was added, and finally the full test matrix was run. The diff adds regression tests in packages/env/src/index.test.ts and packages/config/src/index.test.ts (pattern rejection, integer bound rejection, env/config value revalidation,optional()andwithDefault()forwarding,map()fall-through, async mode,group()forwarding, dependency-source env and config validation, and derived value parser exemption), plus unit tests forvalidateValueitself in packages/core/src/primitives.test.ts and packages/core/src/modifiers.test.ts.The fix was reviewed by Codex across two rounds. The first round flagged four issues: derived value parsers being incorrectly validated, the dependency-source paths not being routed through the validator, a boolean short-circuit in
option().validateValuethat bypassed validation for custom boolean parsers, and missing forwarding throughgroup()andcommand(). All four were addressed in the second commit. The second round flagged a documentation mismatch where the integration guides still described the old forwarding set, which was fixed in the third commit. The final Codex review reports no findings.Test plan
mise test:denomise test:nodemise test:bunmise checkcd docs && pnpm build