Fix suggest() ignoring withDefault() on dependency sources#522
Conversation
When a dependency source was wrapped with withDefault() and no explicit CLI value was provided, suggest() returned an empty array because the default value was never registered in the dependency registry. Added completeDependencySourceDefaults() to pre-complete dependency source parsers during suggestion, mirroring Phase 1 of parse-time dependency resolution. #186 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split completeDependencySourceDefaults into sync and async versions so that suggestObjectAsync properly awaits complete() results. Async parsers with transformsDependencyValue (e.g., withDefault wrapping map) return a Promise from complete(), which the sync version would silently ignore. Also added async test cases for dependency-aware completion. #186 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@codex review |
![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 effectively fixes a bug where suggest() was ignoring withDefault() on dependency sources by adding logic to pre-complete these sources, mirroring the behavior of parse(). The changes are well-reasoned and accompanied by a comprehensive set of regression tests for both synchronous and asynchronous scenarios. My review includes a few suggestions to improve maintainability by reducing code duplication in the implementation, as well as a minor clarification for a JSDoc comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #522 +/- ##
=======================================
Coverage 96.18% 96.18%
=======================================
Files 38 38
Lines 15679 15738 +59
Branches 4026 4038 +12
=======================================
+ Hits 15081 15138 +57
- Misses 591 593 +2
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where suggest() returned an empty array when a dependency source was wrapped with withDefault() and no explicit CLI value was provided, despite parse() handling this correctly. The fix adds pre-completion of dependency source defaults during the suggest path, mirroring Phase 1 of parse-time dependency resolution.
Changes:
- Added
completeDependencySourceDefaults()(sync) andcompleteDependencySourceDefaultsAsync()(async) helper functions that pre-complete unpopulated dependency sources wrapped inwithDefault(), registering their default values in the dependency registry before child parsers are queried for suggestions. - Added
registerCompletedDependency()helper to factor out the common dependency registration logic. - Added four regression tests covering sync/async suggest with
withDefault()dependency sources.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/src/constructs.ts | Added completeDependencySourceDefaults, completeDependencySourceDefaultsAsync, and registerCompletedDependency helpers; integrated them into suggestObjectSync and suggestObjectAsync. |
| packages/core/src/suggest.test.ts | Added four tests: sync default, sync explicit, async with map(), and async without map() — covering the bug fix. |
| CHANGES.md | Documented the fix with issue/PR references. |
![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: 7be673b7c5
ℹ️ 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 comment claimed the function handles Promise-wrapped results, but it only receives already-resolved values; the async caller awaits before passing the result. #522 (comment) #522 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deduplicate the asyncChoice helper that was defined identically in two test cases. #522 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
registerCompletedDependency() now checks registry.has() before calling registry.set(), so parsed dependency values from collectDependencies() are never overwritten by withDefault() fallbacks. #522 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deduplicate the shared iteration logic between completeDependencySourceDefaults() and its async variant into a single generator function. #522 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a bug where suggest() failed to provide correct completions for dependency-aware parsers wrapped with withDefault(). The fix correctly mirrors the dependency resolution logic from the parse() function by introducing new helper functions to pre-complete dependency sources and populate their default values into the dependency registry before generating suggestions. The implementation is well-structured, with clear separation for synchronous and asynchronous paths, and is supported by a comprehensive set of regression tests that cover all relevant cases. The changes are clean, well-documented, and align with the existing architecture of the library.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where suggest() returned empty results when a dependency source was wrapped with withDefault() and no explicit CLI value was provided, despite parse() handling this correctly. The fix adds pre-completion of dependency source defaults during the suggestion phase, mirroring the Phase 1 logic already present in the parse-time path.
Changes:
- Added
completeDependencySourceDefaults()(sync) andcompleteDependencySourceDefaultsAsync()(async) helper functions, along with sharedpendingDependencyDefaults()generator andregisterCompletedDependency()utility, to pre-complete defaulted dependency sources during suggestion. - Integrated these helpers into
suggestObjectSyncandsuggestObjectAsyncto populate the dependency registry with default values before child parsers generate suggestions. - Added four regression tests covering sync/async scenarios with
withDefault()dependency sources, including themap()+withDefault()path that exercisestransformsDependencyValue.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/core/src/constructs.ts | Added dependency source default pre-completion logic for suggest paths and integrated into object suggest functions |
| packages/core/src/suggest.test.ts | Added regression tests for dependency-aware completion with withDefault() |
| CHANGES.md | Documented the bug fix with issue/PR references |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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". |
Summary
withDefault()and no explicit CLI value was provided,suggest()returned an empty array instead of suggesting values based on the default.parse()handled this correctly because its Phase 1 pre-completes dependency sources, but the suggestion path lacked equivalent logic.completeDependencySourceDefaults()(sync) andcompleteDependencySourceDefaultsAsync()(async) to pre-complete dependency source parsers during suggestion, mirroring Phase 1 of parse-time dependency resolution.awaitscomplete()results, which return aPromisefor async parsers withtransformsDependencyValue(e.g.,withDefault(map(...), default)).Test plan
suggest()withwithDefault()dependency source returns correct suggestions based on default valuewithDefault()dependency sourcemap()+withDefault()dependency source (exercises thetransformsDependencyValuepath)mise check,mise test:deno)Closes #186