Conversation
Add transform() for value parsers that can map parsed values to a public type while using the inverse mapping for formatting and fallback validation. The wrapper preserves parser mode, suggestions, dependency metadata, and derived default snapshots. Document the combinator in the value parser guide, catalog, agent skill, and changelog for the PR #866 branch. #866 Assisted-by: Codex:gpt-5.5
Value parsers need a first-class way to expose domain values while still preserving CLI-facing spelling, formatting, fallback validation, and completion metadata. Add transform() for reversible value-parser mapping and biject() for one-to-one string-to-value dictionaries. The biject() parser snapshots mappings at construction time, rejects arrays, empty mappings, empty keys, and duplicate mapped values, and keeps fallback validation tied to the mapped value set. #866 Assisted-by: Codex:gpt-5.5
|
@codex review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
==========================================
- Coverage 93.45% 93.41% -0.05%
==========================================
Files 75 75
Lines 38181 38503 +322
Branches 9519 9577 +58
==========================================
+ Hits 35683 35967 +284
- Misses 1699 1733 +34
- Partials 799 803 +4 ☔ View full report in Codecov by Harness. 🚀 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 biject() parser and the transform() combinator to @optique/core/valueparser, allowing for reversible value parser transformations and one-to-one string-to-value dictionaries. Feedback focuses on improving runtime safety and robustness, including using wrapForMode in transform(), refining object/array validation in biject(), handling undefined placeholders properly, and guarding against non-object values in wrapIterableForMode.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
💡 Codex Reviewoptique/packages/core/src/valueparser.ts Line 1010 in 77665cb  Keep transformed placeholder enumerable
When a ℹ️ 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". |
|
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:
WalkthroughThis PR adds Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/valueparser.test.ts`:
- Around line 3133-3149: The constructor-time duplicate-value tests in biject
are only checking RangeError and should also pin the exact diagnostic message
for each case. Update the assertions in valueparser.test.ts around biject so the
duplicate Map-equality cases for positiveZero/negativeZero and first/second
verify both the thrown error type and the specific duplicate-value message text,
using the same constructor-time validation pattern as the other
option-validation tests.
In `@packages/core/src/valueparser.ts`:
- Around line 1027-1030: The fallback validation in ValueParser.value should
preserve the original transformed value when the round-trip fails, instead of
letting mapping.unmap()/format() exceptions escape. Update the value() path to
catch errors around the syncParser.format(mapping.unmap(value)) and
syncParser.parse(...) round-trip, and treat a thrown formatting/parsing step as
successful preservation of the original value. Keep the behavior aligned with
the existing validate() fallback flow in ValueParser and preserve the original
value rather than crashing default/fallback validation.
🪄 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: 7eb97bba-c484-4535-8e2b-6700c5da2be7
📒 Files selected for processing (8)
CHANGES.mddocs/.vitepress/theme/components/ParserCatalog.vuedocs/concepts/valueparsers.mddocs/index.mdpackages/core/skills/optique/SKILL.mdpackages/core/src/internal/mode-dispatch.tspackages/core/src/valueparser.test.tspackages/core/src/valueparser.ts
Transformed value parsers sit on top of existing parser contracts, so they need to keep the same mode and metadata guardrails as other wrappers. Route wrapped parse results through the mode boundary, keep missing placeholders missing, and leave transformed placeholders enumerable for dependency source cloning. Also guard async-iterator detection before using the in operator and keep the array mapping error short and consistent with the surrounding TypeErrors. #866 (comment) #866 (comment) #866 (comment) #866 (comment) #866 (comment) #866 (comment) Assisted-by: Codex:gpt-5.5
|
Addressed in 43dec86: transformed placeholders remain enumerable, so dependency(transform(...)) and dependency(biject(...)) survive the dependency source object spread. |
|
/gemini review |
|
@codex review |
Fallback validation should not crash when the validation round trip cannot serialize a transformed value. Preserve the original transformed value when unmapping, formatting, or parsing throws, matching the existing fallback validation behavior elsewhere in the parser stack. Also pin the duplicate-value diagnostics for the Map-equality biject() cases so the constructor-time error messages stay covered. #866 (comment) #866 (comment) Assisted-by: Codex:gpt-5.5
There was a problem hiding this comment.
Code Review
This pull request introduces the biject() parser and the transform() combinator to @optique/core/valueparser, along with updated documentation, skills guides, and comprehensive unit tests. Feedback focuses on strengthening the input validation in biject() to explicitly reject null and non-object primitives with a TypeError rather than letting them bypass checks or throw misleading errors, as well as adding corresponding unit tests for these edge cases.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@codex review |
|
/gemini review |
The TypeScript overloads describe object mappings, but JavaScript callers can still pass null or primitive values at runtime. Reject those values with a TypeError before enumerating keys, so strings cannot become accidental indexed mappings and null does not fall through to the empty-mapping RangeError. #866 (comment) #866 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request introduces the biject() parser and the transform() combinator to @optique/core/valueparser, allowing for one-to-one string-to-value mappings and reversible value parser transformations. It also includes comprehensive unit tests, updated documentation, and internal mode-dispatch helpers. The review feedback suggests enhancing the robustness of biject() by explicitly validating that the input mapping is a non-null object before checking if it is an array, and using Object.keys() for a more idiomatic key extraction. Additionally, it recommends wrapping the inner parser's suggest result in transform() to guarantee consistent runtime safety and mode-dispatch behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/src/valueparser.ts (2)
1012-1023: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame fallback-crash risk in the
validate()delegate branch.
mapping.unmap(value)andmapping.map(result.value)here are unguarded, so a throwingunmap/mapimplementation (e.g. for out-of-domain fallback/default values) propagates out ofvalidate()instead of degrading gracefully. This mirrors the already-flagged issue in the adjacent sync-fallback branch below (lines 1024-1038), where the established convention in this codebase is to preserve/tolerate transformation failures during fallback validation rather than let them escape.🐛 Proposed fix
value(value: U): ValueParserResult<U> { - const result = validate(mapping.unmap(value)); - return result.success - ? { success: true as const, value: mapping.map(result.value) } - : result; + let unmapped: T; + try { + unmapped = mapping.unmap(value); + } catch { + return { success: true as const, value }; + } + const result = validate(unmapped); + if (!result.success) return result; + try { + return { success: true as const, value: mapping.map(result.value) }; + } catch { + return { success: true as const, value }; + } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/valueparser.ts` around lines 1012 - 1023, The `validate()` delegate in `valueparser.ts` still calls `mapping.unmap(value)` and `mapping.map(result.value)` without protection, so a throwing mapping can escape instead of being tolerated like the adjacent fallback path. Update the `typeof parser.validate === "function"` branch in the transformed parser setup to catch transformation failures around the `validate` wrapper, and make it return a safe validation result rather than propagating exceptions; keep the behavior aligned with the nearby sync-fallback handling and the `transformed.validate` override.
439-466: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnguarded
mapping.map()in the non-deferred success path can crashparse().The deferred branch (450-464) wraps
mapping.map(result.value)in try/catch and falls back toundefinedon failure, but the primary non-deferred branch (465) — which handles real, successfully-parsed user input — callsmapping.map(result.value)with no error handling at all. If a user-suppliedmap()throws for any valid parsed value, this propagates as an uncaught exception throughtransform()'sparse()(line 979-985): synchronously for sync-mode parsers, or as an unhandled promise rejection for async-mode parsers (since it runs insidemapModeValue's.then()). This breaks theValueParserResultcontract, where callers expect{success: false, error}rather than a thrown/rejected value.This asymmetry (deferred tolerates failures, non-deferred does not) is most concerning precisely because non-deferred is the case triggered by actual CLI input.
🐛 Proposed fix
- return preserveSnapshot({ success: true, value: mapping.map(result.value) }); + try { + return preserveSnapshot({ + success: true, + value: mapping.map(result.value), + }); + } catch (error) { + return { + success: false, + error: error instanceof Error ? error.message : String(error), + }; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/valueparser.ts` around lines 439 - 466, The non-deferred success path in transformValueParserResult still calls mapping.map(result.value) without protection, unlike the deferred branch, so a thrown mapping error can escape parse(). Update transformValueParserResult to handle mapping.map failures in the non-deferred branch the same way as the deferred branch, preserving the snapshot via preserveSnapshot and returning a failed ValueParserResult instead of throwing. Keep the fix localized to transformValueParserResult so callers of transform() and parse() continue to receive the expected result shape.packages/core/src/internal/mode-dispatch.ts (1)
125-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard fix looks correct.
canCheckAsyncIteratorcorrectly prevents theinoperator from throwing onnull/primitive inputs before checkingSymbol.asyncIterator, matching the accompanying test.Minor: the
canCheckAsyncIterator && Symbol.asyncIterator in valuecheck is duplicated in both the sync and async closures. Hoisting it into a singleconst isAsyncIterable = ...abovedispatchIterableByMode(...)would remove the duplication without changing behavior (only one closure executes per call, so this is a readability nit, not a bug).♻️ Optional dedup
export function wrapIterableForMode<M extends Mode, T>( mode: M, value: Iterable<T> | AsyncIterable<T>, ): ModeIterable<M, T> { const canCheckAsyncIterator = value != null && (typeof value === "object" || typeof value === "function"); + const isAsyncIterable = canCheckAsyncIterator && + Symbol.asyncIterator in value; return dispatchIterableByMode( mode, () => { - if (canCheckAsyncIterator && Symbol.asyncIterator in value) { + if (isAsyncIterable) { throw new TypeError( "Synchronous mode cannot wrap AsyncIterable value.", ); } return value; }, () => { - if (canCheckAsyncIterator && Symbol.asyncIterator in value) return value; + if (isAsyncIterable) return value; return (async function* () { yield* value; })(); }, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/internal/mode-dispatch.ts` around lines 125 - 148, The guard in wrapIterableForMode is correct, but the async-iterable check is duplicated in both closures passed to dispatchIterableByMode. Hoist the Symbol.asyncIterator presence check into a single isAsyncIterable const in wrapIterableForMode, then reuse that value in the sync and async branches so the logic is centralized and easier to read without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/internal/mode-dispatch.test.ts`:
- Around line 1-18: Add a regression test for the async branch of
wrapIterableForMode to cover the same canCheckAsyncIterator guard used by
dispatchIterableByMode’s async path. Mirror the existing sync/null assertion in
mode-dispatch.test.ts by invoking wrapIterableForMode("async", null as never),
asserting it throws TypeError, and verifying the message does not contain the
native "in' operator" text so both branches are protected.
In `@packages/core/src/valueparser.test.ts`:
- Around line 2820-2831: The current test only indirectly proves the
enumerability guarantee for transformed placeholders. Strengthen the assertion
in the dependency/transform/biject test by explicitly checking that the
placeholder survives the `dependency(...)` object spread and remains enumerable
on the resulting parser object, rather than relying only on
`isValueParser(...)`. Use the existing `dependency`, `transform`, `biject`, and
`isValueParser` helpers to locate the test and add a direct assertion on the
placeholder’s presence/enumerability.
---
Outside diff comments:
In `@packages/core/src/internal/mode-dispatch.ts`:
- Around line 125-148: The guard in wrapIterableForMode is correct, but the
async-iterable check is duplicated in both closures passed to
dispatchIterableByMode. Hoist the Symbol.asyncIterator presence check into a
single isAsyncIterable const in wrapIterableForMode, then reuse that value in
the sync and async branches so the logic is centralized and easier to read
without changing behavior.
In `@packages/core/src/valueparser.ts`:
- Around line 1012-1023: The `validate()` delegate in `valueparser.ts` still
calls `mapping.unmap(value)` and `mapping.map(result.value)` without protection,
so a throwing mapping can escape instead of being tolerated like the adjacent
fallback path. Update the `typeof parser.validate === "function"` branch in the
transformed parser setup to catch transformation failures around the `validate`
wrapper, and make it return a safe validation result rather than propagating
exceptions; keep the behavior aligned with the nearby sync-fallback handling and
the `transformed.validate` override.
- Around line 439-466: The non-deferred success path in
transformValueParserResult still calls mapping.map(result.value) without
protection, unlike the deferred branch, so a thrown mapping error can escape
parse(). Update transformValueParserResult to handle mapping.map failures in the
non-deferred branch the same way as the deferred branch, preserving the snapshot
via preserveSnapshot and returning a failed ValueParserResult instead of
throwing. Keep the fix localized to transformValueParserResult so callers of
transform() and parse() continue to receive the expected result shape.
🪄 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: 8168b56a-faad-4561-8c2a-eff49792fcbf
📒 Files selected for processing (4)
packages/core/src/internal/mode-dispatch.test.tspackages/core/src/internal/mode-dispatch.tspackages/core/src/valueparser.test.tspackages/core/src/valueparser.ts
There was a problem hiding this comment.
Code Review
This pull request introduces two new value parsers to @optique/core/valueparser: biject(), which provides a one-to-one mapping between CLI string keys and domain values, and transform(), a combinator that maps the output of an existing parser to a different type using a reversible mapping. The changes also include corresponding documentation updates, comprehensive unit tests, and helper utilities in mode-dispatch.ts. Feedback on the implementation highlights a potential validation bypass in the fallback validate method for synchronous parsers, where exceptions thrown during unmap are caught and silently treated as successful.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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: a04840e1d6
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/valueparser.test.ts (1)
2662-2866: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBasic
transform()suite covers the intended round-trip, failure, and throw scenarios.This is a good baseline suite. Given the fallback
validate()concern raised invalueparser.ts(sync fallback treatingformat()/unmap()throws the same asparse()failures), it would help to add/confirm an explicit assertion in the "Cannot format sentinel" case (around lines 2781-2798) that fallback validation preserves the original value rather than rejecting it, once/if that implementation is corrected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/valueparser.test.ts` around lines 2662 - 2866, Add an explicit test in the transform() suite around the fallback validation path using the existing sentinel/“Cannot format sentinel” setup to assert the intended behavior once valueparser.ts is fixed. In the parser.validate?.(sentinel) case, verify that a format() throw during round-trip validation does not reject the original fallback value and that the result stays successful/preserves the sentinel. Use the existing transform(), validate(), and formatter objects in the “should reject fallback values when round-trip validation throws” test to locate the change.
♻️ Duplicate comments (1)
packages/core/src/valueparser.ts (1)
1040-1060: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSync fallback
validate()rejects instead of preserving the original value whenformat()/unmap()throws.This single try/catch treats a thrown
mapping.unmap(value)/syncParser.format(...)exactly the same as a thrown/failedsyncParser.parse(...), returningtransformMappingFailure()in both cases. Per the retrieved learning on this exact fallback pattern in this file, a thrownformat()step should be treated as successful preservation of the original value, while onlyparse()failing/throwing on the formatted output should cause rejection. A prior review thread on this exact code path (samesyncParser.parse(syncParser.format(mapping.unmap(value)))shape) was reportedly addressed by preserving the original value on throw, but the current code does not reflect that — it now stops the crash but still rejects instead of preserving.This means fallback/default validation (e.g.
bindEnv()/bindConfig()) for atransform()-wrapped parser will incorrectly reject values whoseformat()step throws, even though the value itself is perfectly valid.🐛 Proposed fix to separate format()/unmap() throws from parse() failures
} else if (parser.mode === "sync") { const syncParser = parser as ValueParser<"sync", T>; Object.defineProperty(transformed, "validate", { value(value: U): ValueParserResult<U> { - let result: ValueParserResult<T>; - try { - result = syncParser.parse(syncParser.format(mapping.unmap(value))); - } catch { - return transformMappingFailure(); - } + let formatted: string; + try { + formatted = syncParser.format(mapping.unmap(value)); + } catch { + return { success: true as const, value }; + } + let result: ValueParserResult<T>; + try { + result = syncParser.parse(formatted); + } catch { + return transformMappingFailure(); + } if (!result.success) return result; try { return { success: true as const, value: mapping.map(result.value) }; } catch { return transformMappingFailure(); } },Based on learnings: "do not flag the 'fallback' validation pattern that round-trips via
ValueParser.format()+ValueParser.parse()as invalid when it intentionally treats a thrownValueParser.format()duringformat()→parse()round-trips as successful preservation of the original value... Review should still flag... cases whereparse()fails or throws while processing the formatted output."Please verify against the actual test suite (e.g. the
format()-throwing "sentinel" fixture aroundpackages/core/src/valueparser.test.ts:2781-2798) whether this fallback currently preserves or rejects such values.#!/bin/bash # Inspect the sync fallback validate() implementation and its test coverage sed -n '1030,1065p' packages/core/src/valueparser.ts rg -n -B5 -A25 'Cannot format sentinel' packages/core/src/valueparser.test.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/valueparser.ts` around lines 1040 - 1060, The sync fallback validate path in ValueParser’s transformed `validate()` currently collapses `mapping.unmap(value)` / `syncParser.format(...)` throws into the same rejection as `syncParser.parse(...)` failures. Update the `parser.mode === "sync"` branch so `transformMappingFailure()` is only returned when `syncParser.parse(syncParser.format(...))` fails or throws on the formatted output, while a thrown `format()`/`unmap()` step preserves the original value instead of rejecting. Keep the fix localized to the `validate` implementation around `syncParser`, `mapping.unmap`, and `transformMappingFailure`, and align it with the existing sentinel test coverage in `valueparser.test.ts`.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/valueparser.test.ts`:
- Around line 2662-2866: Add an explicit test in the transform() suite around
the fallback validation path using the existing sentinel/“Cannot format
sentinel” setup to assert the intended behavior once valueparser.ts is fixed. In
the parser.validate?.(sentinel) case, verify that a format() throw during
round-trip validation does not reject the original fallback value and that the
result stays successful/preserves the sentinel. Use the existing transform(),
validate(), and formatter objects in the “should reject fallback values when
round-trip validation throws” test to locate the change.
---
Duplicate comments:
In `@packages/core/src/valueparser.ts`:
- Around line 1040-1060: The sync fallback validate path in ValueParser’s
transformed `validate()` currently collapses `mapping.unmap(value)` /
`syncParser.format(...)` throws into the same rejection as
`syncParser.parse(...)` failures. Update the `parser.mode === "sync"` branch so
`transformMappingFailure()` is only returned when
`syncParser.parse(syncParser.format(...))` fails or throws on the formatted
output, while a thrown `format()`/`unmap()` step preserves the original value
instead of rejecting. Keep the fix localized to the `validate` implementation
around `syncParser`, `mapping.unmap`, and `transformMappingFailure`, and align
it with the existing sentinel test coverage in `valueparser.test.ts`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2eebf891-45df-4333-a053-19ce37292397
📒 Files selected for processing (2)
packages/core/src/valueparser.test.tspackages/core/src/valueparser.ts
Sync fallback validation should distinguish failures while preparing the round-trip from failures while parsing the formatted value. Preserve the original transformed fallback when unmap() or format() cannot produce a CLI spelling, while keeping malformed fallbacks that collapse to undefined rejected. #866 (review) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the transform() and biject() value parsers to @optique/core/valueparser, along with comprehensive tests and updated documentation. transform() allows wrapping an existing parser to map its results to another type using reversible mapping functions, while biject() provides a convenient way to map CLI string keys to unique values. Feedback on the changes highlights an inconsistency in packages/core/src/valueparser.ts regarding how unmap failures are handled during fallback validation between the explicit and synthesized validate branches, suggesting a consistent approach to preserve fallback values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ 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". |
The transformed parser should treat unmap() failures consistently whether the wrapped parser provides validate() directly or falls back to a round-trip validator. Preserve sentinel fallback values when the inverse mapping cannot serialize them, while still letting inner validate() failures report normally. #866 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two new value parsers to @optique/core/valueparser: biject(), which provides a one-to-one mapping between CLI string keys and domain values, and transform(), which allows wrapping an existing parser to map its output to a different type. The changes also include documentation updates, comprehensive unit tests, and internal helpers to manage execution modes. Feedback on the implementation suggests simplifying the validate method in the biject parser by removing a redundant parsing step when a value is not found in the reverse mapping.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be3862ceba
ℹ️ 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".
The transformed value parser should keep deferred placeholder results alive when the placeholder cannot be mapped, since phase-two parsing still needs to replay the real value later. Direct dependency sources are now rejected instead of silently registering transformed dependency values against an untransformed source ID. Invalid biject fallbacks also return the choice-shaped error directly without an extra parse pass. #866 (comment) #866 (comment) #866 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces two new value parsers to @optique/core/valueparser: transform(), which wraps an existing parser to map its results to another type, and biject(), which creates a parser from a one-to-one mapping of CLI keys to values. The changes also include comprehensive documentation updates, new tests, and internal helpers to manage execution modes. The review feedback suggests a critical improvement in transform()'s synchronous validation to wrap the inner parser's parse call with wrapForMode, preventing potential asynchronous leakage in synchronous contexts, along with importing the helper.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a72f08318
ℹ️ 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".
The transformed sync fallback validator now checks the formatted value before replaying it through the inner parser, so malformed fallback shapes cannot slip through as non-string input. It also routes the replay through wrapForMode so sync validation reports an accidental Promise return at the same runtime boundary as parse(). #866 (comment) #866 (comment) #866 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the biject() parser and transform() combinator to @optique/core/valueparser, enabling reversible value transformations and one-to-one string-to-value mappings. The changes include implementation details, extensive unit tests, documentation updates, and internal mode-dispatch helpers to preserve synchronous or asynchronous execution modes. There are no review comments on this pull request, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ 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". |
Value parsers already know how to parse, validate fallbacks, format values, and expose completion metadata, but parser-level
map()cannot carry those contracts through a value-only conversion. This PR adds two helpers that make that conversion explicit instead of asking callers to rebuild the same parser behavior by hand.The lower-level helper is
transform(). It wraps an existing value parser with a reversible mapping, so the forward direction is used after parsing and the reverse direction is used for formatting, fallback validation, placeholders, choices, suggestions, and dependency replay. Keeping both directions in the API is the important part: without the reverse mapping, Optique would have to guess how a domain value should be serialized back into the original CLI-facing value.The second helper is
biject(), which covers the common one-to-one dictionary case. It builds onchoice()plustransform(), but snapshots the mapping at construction time and validates the dictionary before any parsing happens. Empty dictionaries, empty input keys, array mappings, and duplicate mapped values fail early because they would make the public parser contract ambiguous. Mapped values are compared with the same equality rules asMap/Set, which keeps the runtime behavior familiar for primitives,NaN, and object identity.The implementation also preserves the metadata that dependency-aware parsers need. A transformed derived parser keeps its dependency markers and default snapshots, and placeholder mapping stays lazy so a missing derived placeholder does not throw during parser construction. That lets
transform()behave like a value-parser counterpart to parser-levelmap()without weakening the two-pass parse path.Tests cover the straightforward parse and format cases, but also the edges that define the API shape: dynamic dependency defaults, lazy placeholders, fallback validation, mutated input mappings, numeric object keys, typed interfaces without index signatures, duplicate values,
NaN, object identity, and invalid array input. The docs in docs/concepts/valueparsers.md describe both helpers, and the value parser catalog, landing page count, changelog, and Optique agent skill were updated so the new surface is discoverable.