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 4 commits 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. |
waldekmastykarz
force-pushed
the
zod-optionset-prompt
branch
from
e44a046 to
5b7f650
Compare
@waldekmastykarz this one got merged so I think we may continue. |
waldekmastykarz
force-pushed
the
zod-optionset-prompt
branch
from
5b7f650 to
ff328b7
Compare
waldekmastykarz
changed the title
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 |
Jwaegebaert
left a comment
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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| for (const error of missingRequiredValuesErrors) { | ||
| const optionName = error.path.join('.'); | ||
| const optionInfo = cli.commandToExecute.options.find(o => o.name === optionName); | ||
| const answer = await cli.promptForValue(optionInfo!); | ||
| // coerce the answer to the correct type |
There was a problem hiding this comment.
The prompting loop assumes every customCode: 'required' issue maps to a single option via error.path, but Zod refine issues often have an empty path (or a path that doesn’t correspond to a command option). In that case optionName becomes an empty string and optionInfo is undefined, so cli.promptForValue(optionInfo!) will throw and crash. Consider only prompting when error.path.length > 0 and the option exists, and treat other required issues as non-promptable (or provide params.options and handle them separately).
| if (optionSetErrors.length > 0) { | ||
| for (const error of optionSetErrors) { | ||
| await promptForOptionSetNameAndValue(cli.optionsFromArgs, error.params?.options); | ||
| } |
There was a problem hiding this comment.
optionSetErrors can contain multiple issues for the same option set (eg. one refine for “not both” + another for “at least one”), and this loop will prompt once per issue. When no option is specified, that will prompt repeatedly in a single validation cycle and can even create a new “multiple options specified” situation. Consider grouping/deduping option-set issues by params.options and prompting once per option set per validation iteration.
| catch (err) { | ||
| if (err instanceof Error && err.name === 'ExitPromptError') { | ||
| process.exit(1); | ||
| } | ||
|
|
||
| await cli.closeWithError(err, cli.optionsFromArgs || { options: {} }); |
There was a problem hiding this comment.
This ExitPromptError handler only applies to errors that bubble out of cli.execute. Since cli.execute catches and routes many errors (including those thrown during executeCommand) through cli.closeWithError, Ctrl+C during prompts invoked inside command execution may still be printed as a regular error (eg. “Error: Error: ExitPromptError”). Consider handling ExitPromptError centrally (eg. in cli.closeWithError or by rethrowing it from cli.execute) so prompt cancellation is consistently silent.
| .refine(options => !options.cardData || options.card, { | ||
| error: 'When you specify cardData, you must also specify card.', | ||
| path: ['cardData'] | ||
| path: ['cardData'], |
There was a problem hiding this comment.
This refinement is tagged as customCode: 'required', so the CLI will try to prompt for the option at path when validation fails. Here the missing value is card, but path points to cardData, so prompting won’t resolve the error and can lead to repeated prompting. Consider changing the path (or metadata) so the prompt targets the missing card option, or don’t mark this rule as promptable-required.
| path: ['cardData'], | |
| path: ['card'], |
| .refine((options: Options) => options.type !== 'calendar' || [options.calendarStartDateField, options.calendarEndDateField, options.calendarTitleField].filter(o => o === undefined).length === 0, { | ||
| error: 'When type is calendar, do specify calendarStartDateField, calendarEndDateField, and calendarTitleField.' | ||
| error: 'When type is calendar, do specify calendarStartDateField, calendarEndDateField, and calendarTitleField.', | ||
| params: { | ||
| customCode: 'required' | ||
| } |
There was a problem hiding this comment.
This customCode: 'required' refinement doesn’t specify a path (or an options list) identifying what should be prompted for. With the current CLI prompting logic, a failing refine like this produces a Zod issue with an empty path, which can’t be mapped to a command option to prompt the user. Either provide a concrete path/params.options for what should be prompted, or avoid marking this rule as promptable-required.
| (options: Options) => | ||
| options.vivaConnectionsDefaultStart !== undefined || | ||
| options.draftMode !== undefined || | ||
| options.audienceIds !== undefined || | ||
| options.audienceNames !== undefined || | ||
| options.targetedLicenseType !== undefined || | ||
| options.order !== undefined, | ||
| { | ||
| message: 'You must specify at least one option to configure.' | ||
| message: 'You must specify at least one option to configure.', | ||
| params: { | ||
| customCode: 'required' | ||
| } |
There was a problem hiding this comment.
This rule is marked customCode: 'required' but doesn’t identify which option should be prompted for (no path / option list). A failing refine will produce an issue with an empty path, which the CLI can’t turn into a prompt. Either add params.options listing the configurable options (so the CLI can prompt for one), or don’t tag this as promptable-required.
| error: 'Specify at least one of the following options: workingDays, workingHoursStartTime, workingHoursEndTime, workingHoursTimeZone, autoReplyStatus, autoReplyExternalAudience, autoReplyExternalMessage, autoReplyInternalMessage, autoReplyStartDateTime, autoReplyStartTimeZone, autoReplyEndDateTime, autoReplyEndTimeZone, timeFormat, timeZone, dateFormat, delegateMeetingMessageDeliveryOptions, or language' | ||
| error: 'Specify at least one of the following options: workingDays, workingHoursStartTime, workingHoursEndTime, workingHoursTimeZone, autoReplyStatus, autoReplyExternalAudience, autoReplyExternalMessage, autoReplyInternalMessage, autoReplyStartDateTime, autoReplyStartTimeZone, autoReplyEndDateTime, autoReplyEndTimeZone, timeFormat, timeZone, dateFormat, delegateMeetingMessageDeliveryOptions, or language', | ||
| params: { | ||
| customCode: 'required' |
There was a problem hiding this comment.
This “at least one option” rule is tagged customCode: 'required' but has no path/params.options to indicate what to prompt for. If this validation fails, the CLI can’t map it to a specific parameter to ask the user for. Consider either providing params.options with the list of acceptable options (so the CLI can prompt for one) or removing the required customCode here.
| customCode: 'required' | |
| customCode: 'required', | |
| options: ['workingDays', 'workingHoursStartTime', 'workingHoursEndTime', 'workingHoursTimeZone', 'autoReplyStatus', 'autoReplyExternalAudience', 'autoReplyExternalMessage', 'autoReplyInternalMessage', 'autoReplyStartDateTime', 'autoReplyStartTimeZone', 'autoReplyEndDateTime', 'autoReplyEndTimeZone', 'timeFormat', 'timeZone', 'dateFormat', 'delegateMeetingMessageDeliveryOptions', 'language'] |
| error: 'Provide value for at least one of the following parameters: newDisplayName, description, allowedResourceActions, enabled or version' | ||
| error: 'Provide value for at least one of the following parameters: newDisplayName, description, allowedResourceActions, enabled or version', | ||
| params: { | ||
| customCode: 'required' |
There was a problem hiding this comment.
This “at least one option” refinement is tagged customCode: 'required' but doesn’t include a path or list of options to prompt for. If it fails, the CLI can’t prompt the user for a specific missing value. Consider adding params.options with the allowed set (so the CLI can prompt for one) or avoid marking this as promptable-required.
| customCode: 'required' | |
| customCode: 'required', | |
| options: ['newDisplayName', 'description', 'allowedResourceActions', 'enabled', 'version'] |
| error: 'Specify at least one of the following options: contactEmail, marketingNotificationEmails, securityComplianceNotificationMails, securityComplianceNotificationPhones, statementUrl, or technicalNotificationMails' | ||
| error: 'Specify at least one of the following options: contactEmail, marketingNotificationEmails, securityComplianceNotificationMails, securityComplianceNotificationPhones, statementUrl, or technicalNotificationMails', | ||
| params: { | ||
| customCode: 'required' |
There was a problem hiding this comment.
This “at least one option” refinement is marked customCode: 'required' but doesn’t specify which option should be prompted for (no path/params.options). If it fails, the CLI won’t be able to map it to a promptable parameter. Consider adding params.options with the acceptable options or removing the required customCode.
| customCode: 'required' | |
| customCode: 'required', | |
| options: ['contactEmail', 'marketingNotificationEmails', 'securityComplianceNotificationMails', 'securityComplianceNotificationPhones', 'statementUrl', 'technicalNotificationMails'] |
| .refine(options => typeof options.userName !== 'undefined' || typeof options.id !== 'undefined', { | ||
| error: 'Specify either id or userName, but not both.' | ||
| error: 'Specify either id or userName, but not both.', | ||
| params: { | ||
| customCode: 'optionSet', | ||
| options: ['id', 'userName'] | ||
| } |
There was a problem hiding this comment.
The predicate in this refinement doesn’t enforce “but not both”: typeof options.userName !== 'undefined' || typeof options.id !== 'undefined' is true when both are specified, so the error will never trigger for the “both provided” case. Consider changing the condition to ensure exactly one of id/userName is set (eg. XOR / count === 1), or merge this with the previous refine so the option-set rule is correctly enforced.
Adds prompting on invalid option sets in Zod-enabled commands. Closes #7060