feat(gunshi): add onResolveValue lifecycle hook to CliOptions#503
feat(gunshi): add onResolveValue lifecycle hook to CliOptions#503imjuni wants to merge 6 commits into
Conversation
Add `onResolveValue` hook that runs once after argument parsing and before command execution. The hook receives parsed argv values (with schema defaults filled in) and the explicit CLI keys map, allowing developers to merge values from external sources such as config files or environment variables. - Add `ValueResolutionSources<A>` interface to types.ts - Add `onResolveValue` option to `CliOptions` - Extract `resolveValue` helper into resolver.ts with 100% test coverage - Call hook directly in cli/core.ts consistent with other lifecycle hooks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds a post-parse value-resolution hook and supporting utilities: new types, resolver functions (resolveValue, revalidateError), tests, and integration that replaces parsed values/validationError with resolvedValues/resolvedError in the CLI command context. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (parser)
participant Resolver as resolver.resolveValue / revalidateError
participant Hook as onResolveValue (user)
participant Runner as Command Runner
CLI->>Resolver: parsed `values` + `explicit`
Resolver->>Hook: frozen snapshot of `values` + `explicit`
alt Hook returns resolved values
Hook-->>Resolver: `resolvedValues`
Resolver->>Resolver: revalidate errors against `resolvedValues`
Resolver-->>CLI: `resolvedValues` + `resolvedError`
CLI->>Runner: invoke with `resolvedValues`
else Hook returns undefined
Hook-->>Resolver: undefined
Resolver-->>CLI: original `values` + original `validationError`
CLI->>Runner: skip (validation error present)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/gunshi/src/cli/core.ts (1)
88-102:⚠️ Potential issue | 🟠 MajorRecompute validation after applying
onResolveValue.Line 88 swaps in
resolvedValues, but the validation result still comes from the pre-hookresolveArgscall. If the hook fills a required option from config/env,ctx.valuesbecomes usable whilectx.validationErrorstill reports the old missing-arg failure.This needs validation state recomputed from the final resolved values, or the hook contract needs to explicitly say it cannot satisfy validation. An end-to-end test with a required arg provided only by
onResolveValuewould catch this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gunshi/src/cli/core.ts` around lines 88 - 102, The validation result must be recomputed after applying onResolveValue so ctx.validationError reflects the final values: after calling resolveValue (symbol resolveValue) and before using createCommandContext, re-run the argument validation (the same logic used in resolveArgs/validateArgs) against resolvedValues and update the validation state that createCommandContext or commandContext will use (or pass the freshly computed validation result into createCommandContext) so ctx.validationError no longer reflects the pre-hook missing-arg failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gunshi/src/resolver.ts`:
- Around line 19-27: The hook currently receives the live values/explicit
objects so if the hook mutates sources.values and returns undefined those
mutations leak into ctx; change resolveValue to pass an immutable snapshot
instead (e.g., create deep-cloned copies of values and explicit or use
structuredClone and Object.freeze on the snapshot object) and call hook({
values: snapshotValues, explicit: snapshotExplicit }) so mutated hook-local
objects don't affect the originals; add a regression test that sets up a hook
which mutates the received sources.values and returns undefined and then assert
that the original ArgValues (passed into resolveValue) were not modified and
that resolveValue returns the original values.
---
Outside diff comments:
In `@packages/gunshi/src/cli/core.ts`:
- Around line 88-102: The validation result must be recomputed after applying
onResolveValue so ctx.validationError reflects the final values: after calling
resolveValue (symbol resolveValue) and before using createCommandContext, re-run
the argument validation (the same logic used in resolveArgs/validateArgs)
against resolvedValues and update the validation state that createCommandContext
or commandContext will use (or pass the freshly computed validation result into
createCommandContext) so ctx.validationError no longer reflects the pre-hook
missing-arg failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 836cd656-f7fd-471e-8fab-ddadabf71d5f
📒 Files selected for processing (4)
packages/gunshi/src/cli/core.tspackages/gunshi/src/resolver.test.tspackages/gunshi/src/resolver.tspackages/gunshi/src/types.ts
…utation Pass an immutable frozen copy of values to the hook instead of the live object, so that a hook that mutates sources.values and returns undefined no longer corrupts the original fallback values. Add a regression test covering the "mutate then return undefined" case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After the hook runs, re-filter the pre-hook AggregateError using revalidateError(): required-argument errors are dropped for any key whose value is now non-nullish in the resolved values. Other error types (type, conflict) are kept unchanged. Add unit tests for revalidateError and an integration test that verifies a required arg supplied only by onResolveValue correctly clears ctx.validationError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 `@packages/gunshi/src/cli.test.ts`:
- Around line 2032-2051: The test "validationError remains when onResolveValue
does not satisfy required arg" currently only asserts the error output; capture
the runner spy by creating a runSpy = vi.fn() and pass it as the run property to
cli, then after awaiting cli assert expect(runSpy).not.toHaveBeenCalled() so the
command is not executed when resolution fails (referencing cli, onResolveValue
and the run spy).
In `@packages/gunshi/src/resolver.test.ts`:
- Around line 75-94: Add a nested-mutation test: create inputValues where one
field is a nested object/array (e.g., settings: { nested: 'orig' } or items:
[{x:1}]) and pass it to resolveValue; in the hook (vi.fn mock) attempt to mutate
a deep property (e.g., sources.values.settings.nested = 'mutated' or
sources.values.items[0].x = 2) and return undefined, then assert the returned
result still equals the original inputValues and that the nested property
remains the original value. Reference resolveValue, ArgValues<TestArgs>, the
hook mock, and explicit so the new test mirrors the existing top-level mutation
test but targets nested data. Ensure the test name reflects nested mutation
(e.g., "should return original values when hook mutates nested snapshot and
returns undefined").
- Around line 29-104: Add a sibling type test file
(packages/gunshi/src/resolver.test-d.ts) that asserts compile-time inference for
the public generics onResolveValue and ValueResolutionSources<A>: import
onResolveValue and ValueResolutionSources (and ArgValues/A sample TestArgs) and
use expectTypeOf to assert that sources.values has type ArgValues<TestArgs>,
sources.explicit has type ArgExplicitlyProvided<TestArgs>, and the hook return
type is assignable to ArgValues<TestArgs> | undefined; place the file next to
resolver.test.ts, import the same test helper TestArgs used in runtime tests (or
declare a minimal TestArgs locally) and write expectTypeOf checks so future
changes to CliOptions/ArgValues<A> will fail at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c7a97d1-f4d3-4226-99b1-75fae96c0a07
📒 Files selected for processing (4)
packages/gunshi/src/cli.test.tspackages/gunshi/src/cli/core.tspackages/gunshi/src/resolver.test.tspackages/gunshi/src/resolver.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/gunshi/src/cli/core.ts
- packages/gunshi/src/resolver.ts
…required arg unresolved Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/gunshi/src/cli.test.ts (1)
2005-2029: Lock down theonResolveValuecontract, not just the final context.This proves the resolved
ctx.values, but it still won’t catch a regression inpackages/gunshi/src/cli/core.tsthat calls the hook more than once or wires an incompletesourcespayload into it. Please wraponResolveValuein a spy and assert the once-only call plus the incomingvalues/explicitshape for this integration path. As per coding guidelines, "Add tests for all new features with test coverage tracked invitest.config.ts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gunshi/src/cli.test.ts` around lines 2005 - 2029, The test currently verifies only the final ctx.values but not the onResolveValue contract; update the test for the cli helper to wrap the provided onResolveValue hook in a spy (use vi.fn or similar) and assert it was invoked exactly once and that its single incoming argument is an object containing both a values property and an explicit property with expected shapes; target the test invoking cli(...) and the onResolveValue hook so you catch regressions in packages/gunshi/src/cli/core.ts where the hook might be called multiple times or given an incomplete sources payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/gunshi/src/cli.test.ts`:
- Around line 2005-2029: The test currently verifies only the final ctx.values
but not the onResolveValue contract; update the test for the cli helper to wrap
the provided onResolveValue hook in a spy (use vi.fn or similar) and assert it
was invoked exactly once and that its single incoming argument is an object
containing both a values property and an explicit property with expected shapes;
target the test invoking cli(...) and the onResolveValue hook so you catch
regressions in packages/gunshi/src/cli/core.ts where the hook might be called
multiple times or given an incomplete sources payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55031445-cc15-4237-9121-2f5f7eb31184
📒 Files selected for processing (1)
packages/gunshi/src/cli.test.ts
…rces shape Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lue hook - Add resolver.test-d.ts to lock down ValueResolutionSources<A> inference: sources.values, sources.explicit field types, per-field resolution (string|undefined, number|undefined, boolean|undefined), explicit flags as boolean, hook parameter/return types, and default type parameter - Add nested-mutation test to resolver.test.ts documenting that Object.freeze is shallow; top-level properties are protected but nested objects can still be mutated through the snapshot Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gunshi/src/resolver.test-d.ts`:
- Around line 52-60: The tests currently define a local Hook alias which repeats
the expected signature and can miss drift in the exported API; change the Hook
type to derive from the real exported option by using
NonNullable<CliOptions<MyArgs>['onResolveValue']> (or the appropriate generic
for CliOptions) so both assertions reference the actual implementation, e.g.
replace "type Hook = (sources: ValueResolutionSources<MyArgs>) =>
Awaitable<ArgValues<MyArgs> | undefined>" with "type Hook =
NonNullable<CliOptions<MyArgs>['onResolveValue']>" and keep the rest of the
assertions (ensuring CliOptions is imported if not already).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ed0e8cb-be6d-4119-a45c-8c3e4a46dde7
📒 Files selected for processing (3)
packages/gunshi/src/cli.test.tspackages/gunshi/src/resolver.test-d.tspackages/gunshi/src/resolver.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/gunshi/src/resolver.test.ts
- packages/gunshi/src/cli.test.ts
|
Sorry, late my reply. Thanks for working on this.
For example, if an arg is defined as Since config/env values are external runtime data, I think hook-resolved values should be validated against the same arg schema, or required-error clearing should at least verify that the resolved value matches the expected type. Minor follow-ups:
I would consider the validation gap the main issue here. |
2 participants
Summary
Implements the
onResolveValuelifecycle hook as discussed in the related issue.ValueResolutionSources<A>interface — containsvalues(parsed argv with schema defaults) andexplicit(map of keys explicitly provided via CLI)onResolveValueoption toCliOptions— called once after argument parsing, before command execution, consistent withonBeforeCommand/onAfterCommandpatternresolveValuehelper intoresolver.tswith 100% test coverage (statements, branches, functions, lines)Design decisions
valuesfromresolveArgsalready has schema defaults filled in by args-tokens, so no separatedefaultsfield is needed —explicitis sufficient to distinguish CLI-provided values from defaultsvaluesif the hook returnsundefinedCliOptionsonly (notPluginContext), consistent with other lifecycle hooks — plugins that need this behavior can useextensionordecorateCommandTest plan
pnpm test:core— 207 tests pass, no type errorspnpm lint— all checks passresolver.tscoverage — 100% statements / branches / functions / lines🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests