Adds prompting on invalid option sets in Zod-enabled commands#7061
Adds prompting on invalid option sets in Zod-enabled commands#7061waldekmastykarz wants to merge 1 commit intopnp:mainfrom
Conversation
|
To test, run:
Notice the additional information that we'll need in refined schema definition to feed prompts. Let's verify that this approach works and meets all our needs. I'll then add the necessary tests and fix existing Zod-enabled commands to support this properly. |
|
Let's continue with this after we merge #7005 so that we do the migration/alignment work once. |
e44a046 to
5b7f650
Compare
@waldekmastykarz this one got merged so I think we may continue. |
5b7f650 to
ff328b7
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements interactive prompting for invalid option sets in Zod-enabled commands to maintain UX consistency with non-Zod commands. When users provide invalid option combinations (e.g., specifying both mutually exclusive options or none of the required options), the CLI now prompts them to select and provide values for the correct options instead of immediately exiting with an error.
Changes:
- Added prompting logic in
src/cli/cli.tsto detect and handle option set validation errors from Zod schemas by checking for custom error params withcustomCode: 'optionSet' - Modified error handling in
src/index.tsto catchExitPromptErrorat the top level (moved fromsrc/utils/prompt.ts) - Updated 50+ command files to add
paramsmetadata to their Zod.refine()calls, marking them as eitheroptionSetorrequiredvalidation errors for proper prompting behavior
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Added top-level ExitPromptError handling to exit gracefully when user cancels prompts |
| src/utils/prompt.ts | Removed ExitPromptError catch block from prompt.forInput (now handled at top level) |
| src/cli/cli.ts | Implemented option set prompting logic that classifies validation errors and prompts users to select and provide values for option sets |
| src/cli/cli.spec.ts | Added comprehensive tests for option set prompting, required field prompting, and error handling with refined schemas |
| src/m365/viva/commands/engage/*.ts | Added optionSet params to refine() calls for community and role option validations |
| src/m365/teams/commands/callrecord/callrecord-list.ts | Added optionSet params for userId/userName validation |
| src/m365/spp/commands/**/*.ts | Added optionSet params for model and autofill column option validations |
| src/m365/spo/commands/**/*.ts | Added optionSet/required params for list, page, homesite, file, and web alert option validations |
| src/m365/spe/commands/**/*.ts | Added optionSet params for container and containertype option validations |
| src/m365/pp/commands/**/*.ts | Added optionSet params for website and environment option validations |
| src/m365/pa/commands/environment/environment-get.ts | Added optionSet params for name/default validation |
| src/m365/outlook/commands/**/*.ts | Added optionSet/required params for mailbox and mail searchfolder option validations |
| src/m365/graph/commands/directoryextension/*.ts | Added optionSet params for directory extension option validations |
| src/m365/flow/commands/environment/environment-get.ts | Added optionSet params for name/default validation |
| src/m365/exo/commands/approleassignment/approleassignment-add.ts | Added optionSet params for extensive role and scope option validations |
| src/m365/entra/commands/**/*.ts | Added optionSet/required params for user, role, admin unit, and organization option validations |
| src/m365/commands/login.ts | Added optionSet/required params for authentication option validations |
| src/m365/booking/commands/business/business-get.ts | Added optionSet params for id/name validation |
| src/m365/app/commands/permission/permission-add.ts | Added required params for permission option validation |
| src/m365/adaptivecard/commands/adaptivecard-send.ts | Added required params for card option validation |
| const selectedOptionName = await prompt.forSelection<string>({ message: `Option to use:`, choices: options.map((choice: any) => { return { name: choice, value: choice }; }) }); | ||
| const optionValue = await prompt.forInput({ message: `${selectedOptionName}:` }); | ||
|
|
||
| args.options[selectedOptionName] = optionValue; |
There was a problem hiding this comment.
The prompted option value is directly assigned as a string without type coercion. Unlike the handling of missing required values (lines 222-223), which uses getCommandOptionsFromArgs to coerce the value to the correct type, this code doesn't perform any type coercion. If an option in the option set expects a non-string type (e.g., number, boolean, UUID), validation may fail on the next iteration of the while loop. Consider using the same type coercion approach as the missing required values handling.
| args.options[selectedOptionName] = optionValue; | |
| let coercedValue: unknown = optionValue; | |
| // Basic type coercion to align with how CLI arguments are typically parsed | |
| if (typeof optionValue === 'string') { | |
| const trimmed = optionValue.trim(); | |
| if (trimmed === 'true') { | |
| coercedValue = true; | |
| } | |
| else if (trimmed === 'false') { | |
| coercedValue = false; | |
| } | |
| else if (trimmed !== '' && !isNaN(Number(trimmed))) { | |
| coercedValue = Number(trimmed); | |
| } | |
| } | |
| args.options[selectedOptionName] = coercedValue as any; |
There was a problem hiding this comment.
So I actually managed to reproduce the type coercion bug. Here's what happens with m365 pp environment get:
- You get prompted to pick an option > select default
- Type something as the value > CLI crashes with Error: The value 'test' for option '--default' is not a valid boolean
- Even if you type true, it loops back and prompts again for the default as if it's still missing
The value gets stored as a raw string ("true") in args.options, but the Zod schema expects a proper boolean true. So validation fails again, this time as a missingRequiredValuesErrors error, which re-prompts you.
Jwaegebaert
left a comment
There was a problem hiding this comment.
Great work @waldekmastykarz! Left a few minor suggestion and the topic Copilot suggested seems something interesting to look into. That function works fine for strings but as soon as we work with other types the behavior seems to change a bit.
| const otherErrors: z.core.$ZodIssue[] = result.error.issues | ||
| .filter(e => !missingRequiredValuesErrors.includes(e) && !optionSetErrors.includes(e as z.core.$ZodIssueCustom)); | ||
|
|
||
| if (otherErrors.some(e => e)) { |
There was a problem hiding this comment.
Maybe a bit nitpicking but I think .length > 0 would be a bit cleaner in this scenario
There was a problem hiding this comment.
I wonder if there's a performance penalty for using length over some. Typically, when you use length, the runtime needs to calculate the full length of a collection. In comparison, when you use some, the runtime uses the iterator and stops as soon as the condition is met, which would be more efficient.
There was a problem hiding this comment.
If I'm not mistaken, it’s a field the engine keeps up to date when the array changes, so checking otherErrors.length > 0 is basically just a quick property read.
Adds prompting on invalid option sets in Zod-enabled commands. Closes #7060