feat(settings): add new confirmation settings to control default/invalid input behavior#138
Conversation
WalkthroughAdds configurable confirmation behaviors and wiring: new ConfirmationSettings and ConfirmationPromptBehavior, updated Settings, completion helpers, and docs; ConfirmationInput now accepts ConfirmationPromptOptions and implements scanner-based re-prompting; command handlers and SSH host-key prompt use the new options; enables cobra traversal run hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cmd as Command Handler
participant Cfg as Config
participant Utils as ConfirmationInput
User->>Cmd: run command
Cmd->>Cfg: read cfg.Confirmation
alt opts.AlwaysConfirm OR Cfg.Confirmation.Always
Cmd->>Cmd: skip confirmation -> proceed
else
Cmd->>Utils: ConfirmationInput(msg, ConfirmationPromptOptions)
loop prompt loop
Utils->>User: display prompt
User->>Utils: input (y/n/empty/invalid)
alt empty
Utils->>Utils: apply EmptyBehavior (Retry / DefaultYes / DefaultNo)
else invalid
Utils->>Utils: apply InvalidBehavior (Retry / DefaultYes / DefaultNo)
else valid
Utils->>Cmd: return result
end
end
alt confirmed
Cmd->>Cmd: proceed action
else not confirmed
Cmd->>User: abort / log
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧬 Code graph analysis (5)cmd/generation/switch/switch.go (1)
internal/cmd/utils/confirmation.go (1)
internal/settings/completion.go (1)
internal/settings/settings.go (1)
cmd/generation/delete/delete.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (9)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/system/ssh.go (1)
213-219: Use the defined constant for consistency.The hard-coded
"retry"string literals work correctly sinceConfirmationPromptBehavioris a string type, but using thesettings.ConfirmationPromptRetryconstant would be more type-safe and maintainable. If the constant value ever changes, this code would silently diverge.confirm, err := cmdUtils.ConfirmationInput("Are you sure you want to continue connecting?", cmdUtils.ConfirmationPromptOptions{ // Copy the default SSH behavior of retrying for invalid input. // Disregard user configuration in this case, since this is mimicking // OpenSSH's behavior. - InvalidBehavior: "retry", - EmptyBehavior: "retry", + InvalidBehavior: settings.ConfirmationPromptRetry, + EmptyBehavior: settings.ConfirmationPromptRetry, })Note: This requires importing
settingsif not already imported via thecmdUtilspath.internal/settings/completion.go (1)
211-218: Consider extracting a shared completion function.The two anonymous functions for
"confirmation.empty"and"confirmation.invalid"are identical. While this is fine for just two entries, you could reduce duplication with a helper:+func confirmationPromptCompletionFunc(key, candidate string) ([]string, cobra.ShellCompDirective) { + return stringCompletionFunc(key, candidate, AvailableConfirmationPromptSettings) +} + var completionValueFuncs = map[string]CompletionValueFunc{ - "confirmation.empty": func(key, candidate string) ([]string, cobra.ShellCompDirective) { - return stringCompletionFunc(key, candidate, AvailableConfirmationPromptSettings) - }, - "confirmation.invalid": func(key, candidate string) ([]string, cobra.ShellCompDirective) { - return stringCompletionFunc(key, candidate, AvailableConfirmationPromptSettings) - }, + "confirmation.empty": confirmationPromptCompletionFunc, + "confirmation.invalid": confirmationPromptCompletionFunc, }internal/cmd/utils/confirmation.go (1)
43-43: Consider returning an error instead of panicking for unhandled enum values.While panic is acceptable for exhaustive enum handling, returning an error would be more robust if configuration values are corrupted or if there's a version mismatch between the settings and this code.
Example refactor for line 43:
default: - panic("unhandled case " + opts.EmptyBehavior) + return false, fmt.Errorf("unhandled EmptyBehavior case: %s", opts.EmptyBehavior)Apply a similar pattern for line 67.
Also applies to: 67-67
internal/settings/settings.go (1)
326-331: Defensive pointer handling added to reflection logic.This enhancement handles pointer fields in the settings traversal. While no pointer fields are currently present in the
Settingsstruct, this defensive programming improves robustness for future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/apply/apply.go(1 hunks)cmd/generation/delete/delete.go(1 hunks)cmd/generation/rollback/rollback.go(1 hunks)cmd/generation/switch/switch.go(1 hunks)cmd/root/root.go(1 hunks)internal/cmd/utils/confirmation.go(1 hunks)internal/settings/completion.go(2 hunks)internal/settings/settings.go(10 hunks)internal/system/ssh.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
internal/system/ssh.go (1)
internal/cmd/utils/confirmation.go (2)
ConfirmationInput(18-70)ConfirmationPromptOptions(13-16)
cmd/generation/delete/delete.go (1)
internal/cmd/utils/confirmation.go (2)
ConfirmationInput(18-70)ConfirmationPromptOptions(13-16)
internal/settings/completion.go (1)
internal/settings/settings.go (1)
AvailableConfirmationPromptSettings(70-74)
cmd/generation/rollback/rollback.go (1)
internal/cmd/utils/confirmation.go (2)
ConfirmationInput(18-70)ConfirmationPromptOptions(13-16)
cmd/generation/switch/switch.go (1)
internal/cmd/utils/confirmation.go (2)
ConfirmationInput(18-70)ConfirmationPromptOptions(13-16)
internal/cmd/utils/confirmation.go (1)
internal/settings/settings.go (4)
ConfirmationPromptBehavior(62-62)ConfirmationPromptRetry(65-65)ConfirmationPromptDefaultNo(67-67)ConfirmationPromptDefaultYes(66-66)
cmd/apply/apply.go (1)
internal/cmd/utils/confirmation.go (2)
ConfirmationInput(18-70)ConfirmationPromptOptions(13-16)
internal/settings/settings.go (1)
internal/settings/errors.go (1)
SettingsError(7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (13)
cmd/root/root.go (1)
167-168: LGTM!Enabling
cobra.EnableTraverseRunHooksensures thatPersistentPreRunEhooks execute for all subcommands, which is necessary for the confirmation settings to be properly loaded and validated before commands likeapply,delete,rollback, andswitchrun.cmd/generation/delete/delete.go (1)
168-181: LGTM!The confirmation logic correctly combines the
--yesflag (AlwaysConfirm) with the newcfg.Confirmation.Alwayssetting, and properly passes the configuredInvalidandEmptybehaviors to the prompt. Error handling is appropriately maintained.cmd/apply/apply.go (1)
504-519: LGTM!The confirmation logic correctly integrates the new configuration-driven prompt behavior. Note that on confirmation denial, this command returns an error (line 517), while
delete.goreturnsnil. This appears intentional sinceapplytreats denial as an error state (skipping activation is considered a failure), whereasdeletetreats it as a graceful exit. The pattern is consistent with the command semantics.cmd/generation/switch/switch.go (1)
146-161: LGTM!The confirmation logic is consistent with the pattern used in
apply.goandrollback.go. The prompt options are correctly sourced from configuration, and the error handling on confirmation denial matches other activation commands.cmd/generation/rollback/rollback.go (1)
94-109: LGTM!The confirmation flow correctly integrates with the new configuration-driven prompt system, maintaining consistency with other activation commands (
apply,switch).internal/settings/completion.go (1)
190-208: LGTM!Good refactoring to extract
stringCompletionFuncas a reusable helper. TheboolCompletionFuncdelegation keeps the code DRY.internal/cmd/utils/confirmation.go (1)
13-16: LGTM!The struct definition is clear and appropriately uses the
ConfirmationPromptBehaviortype from the settings package.internal/settings/settings.go (6)
39-74: LGTM!The new confirmation settings types and constants are well-designed and follow Go conventions. The string-based enum pattern with descriptive mappings is idiomatic and clear.
76-85: LGTM!The
UnmarshalTextimplementation correctly validates configuration values at parse time, preventing invalid enum values from reaching the runtime code.
99-100: LGTM!The documentation entries are comprehensive and clearly explain the new settings. The deprecation notice for
no_confirmappropriately guides users to the newconfirmation.alwayssetting.Also applies to: 144-158, 177-181
212-216: LGTM!The default values are sensible and align with the PR objectives. Defaulting
EmptytoDefaultNopreserves the previous behavior, and defaultingInvalidtoRetryensures users provide valid input.
292-298: LGTM!The deprecation handling for
NoConfirmis well-implemented. It warns users while automatically migrating to the new setting, ensuring backward compatibility.
379-384: LGTM!The
boldedhelper is simple and appropriate for its current use in static documentation strings. The warning about lack of escaping is appropriately documented.
9051058 to
34a1302
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cmd/utils/confirmation.go (1)
25-29: Consider checkingScan()return value for EOF detection.When stdin is closed or EOF is reached,
scanner.Scan()returnsfalsewithout setting an error. Currently, this results in an infinite loop with empty input being processed repeatedly. Consider checking the return value to handle EOF gracefully.- _ = scanner.Scan() - input = scanner.Text() - if err := scanner.Err(); err != nil { - return false, err - } + if !scanner.Scan() { + if err := scanner.Err(); err != nil { + return false, err + } + // EOF reached - treat as empty input + input = "" + } else { + input = scanner.Text() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/apply/apply.go(1 hunks)cmd/generation/delete/delete.go(1 hunks)cmd/generation/rollback/rollback.go(1 hunks)cmd/generation/switch/switch.go(1 hunks)cmd/root/root.go(1 hunks)internal/cmd/utils/confirmation.go(1 hunks)internal/settings/completion.go(1 hunks)internal/settings/settings.go(10 hunks)internal/system/ssh.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/root/root.go
- cmd/generation/delete/delete.go
- internal/system/ssh.go
- internal/settings/completion.go
- cmd/generation/rollback/rollback.go
- cmd/apply/apply.go
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/generation/switch/switch.go (1)
internal/cmd/utils/confirmation.go (2)
ConfirmationInput(18-70)ConfirmationPromptOptions(13-16)
internal/cmd/utils/confirmation.go (1)
internal/settings/settings.go (4)
ConfirmationPromptBehavior(62-62)ConfirmationPromptRetry(65-65)ConfirmationPromptDefaultNo(67-67)ConfirmationPromptDefaultYes(66-66)
internal/settings/settings.go (1)
internal/settings/errors.go (1)
SettingsError(7-10)
🔇 Additional comments (5)
internal/cmd/utils/confirmation.go (1)
31-47: Previous concern addressed: whitespace input no longer causes panic.The input is now trimmed before the empty check on line 33, which correctly handles whitespace-only input. This resolves the previously identified issue.
cmd/generation/switch/switch.go (1)
146-161: LGTM!The confirmation logic correctly respects both the CLI flag (
--yes) and the configuration setting (cfg.Confirmation.Always). TheConfirmationPromptOptionsare properly populated from configuration.internal/settings/settings.go (3)
292-298: Good deprecation handling with automatic migration.The validation correctly warns about the deprecated
no_confirmsetting and automatically migrates its effect tocfg.Confirmation.Always = true. This maintains backward compatibility while guiding users to the new setting.
212-216: Default values are sensible and backward-compatible.The defaults (
Always: false,Invalid: retry,Empty: default-no) preserve the previous behavior where empty input defaulted to "no" while introducing the more user-friendly retry behavior for invalid input.
62-68: Given that I cannot access the repository directly to verify the concern, I'll provide a rewritten review comment based on the technical analysis of what was provided:
SetValuemay not correctly validateConfirmationPromptBehaviorfields if used.The
ConfirmationPromptBehaviortype is based onstring, and whileSetValuecorrectly identifies it viareflect.Kind()returningreflect.String, callingcurrent.SetString(value)bypasses theUnmarshalTextvalidation. This would allow invalid values like"foo"to be set directly.If
SetValueis used forConfirmation.InvalidorConfirmation.Emptyfields, consider adding validation:case reflect.String: + // Validate ConfirmationPromptBehavior values + if current.Type().Name() == "ConfirmationPromptBehavior" { + if _, ok := AvailableConfirmationPromptSettings[value]; !ok { + return SettingsError{Field: field, Message: fmt.Sprintf("invalid value '%s' for confirmation behavior", value)} + } + } current.SetString(value)
…lid input behavior
34a1302 to
4c6acac
Compare
Description
The confirmation prompt was a bit flaky and didn't respect the
no_confirmsetting, and always defaulted to a response of "no" if no input/invalid input was given.Since this behavior may not be expected by all users, this PR makes it configurable through some new settings;
confirmation.always(skip entirely),confirmation.invalid(to control what to do when invalid input is given, defaults to retrying), andconfirmation.empty(to control what to do when no input is given, defaults to treating it as a "no" input, like before).This also deprecates the
no_confirmoption that was unused anyway, and tells users to useconfirmation.always. It will be removed in a future release.Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.