Skip to content

refactor(cmd): use cobra MarkFlags for flag validation #726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
skevetter merged 1 commit into from
Jul 23, 2026
Merged

refactor(cmd): use cobra MarkFlags for flag validation #726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions cmd/config/read.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ func NewReadCmd(f *flags.GlobalFlags) *cobra.Command {
"Include the merged configuration in the output"),
)

readConfigCmd.MarkFlagsOneRequired(names.WorkspaceFolder, names.ContainerID, names.IDLabel)

Comment on lines +62 to +63

@coderabbitai coderabbitai Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files =="
git ls-files | rg '(^|/)read\.go$|(^|/)runusercommands\.go$|names\.go$' || true

echo "== read.go outline =="
ast-grep outline cmd/config/read.go --view compact || true
echo "== read.go relevant =="
sed -n '1,180p' cmd/config/read.go | cat -n

echo "== runusercommands.go relevant =="
sed -n '120,210p' cmd/internal/runusercommands.go | cat -n

echo "== search names definitions =="
rg -n "WorkspaceFolder|ContainerID|IDLabel|FlagsOneRequired|MarkFlags|validate" cmd -S

Repository: devsy-org/devsy

Length of output: 32251

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== go.mod cobra version =="
if [ -f go.mod ]; then
  rg '(^github.com/spf13/cobra|require \()' go.mod
fi

echo "== cobra flag groups source from module cache if present =="
MODCACHE=$(go env GOPATH 2>/dev/null || true)
if [ -n "${MODCACHE}" ]; then
  for f in "$MODCACHE/pkg/mod/github.com/spf13/cobra@*/flag_groups.go" "$MODCACHE/pkg/mod/github.com/spf13/cobra@*/flag_groups.go"; do
    [ -e "$f" ] && { echo "--- $f"; sed -n '1,180p' "$f"; break; }
  done
fi

echo "== read.go resolveConfigFromIDLabels =="
sed -n '180,245p' cmd/config/read.go | cat -n

echo "== runusercommands tests empty values if present =="
rg -n 'empty|""|WorkspaceFolder|ContainerID|MarkFlagsOneRequired|validate' cmd/internal/runusercommands_test.go -A 4 -B 4

echo "== config name definitions =="
sed -n '1,160p' pkg/flags/names/names.go | cat -n

Repository: devsy-org/devsy

Length of output: 13987

🌐 Web query:

spf13 cobra MarkFlagsOneRequired flag groups Changed value empty behavior

💡 Result:

In the spf13/cobra library, the MarkFlagsOneRequired function ensures that at least one flag from a specified group is provided when a command is executed [1][2]. Regarding the behavior of empty or default values: 1. Flag State Determination: Cobra determines if a flag is "set" based on whether it has been explicitly changed from its default value by the user, rather than whether the value itself is empty [3]. If a flag is defined with a default value (e.g., an empty string "" for a String flag), providing that same value on the command line typically does not register as the flag being "set" [3]. 2. Validation Behavior: When MarkFlagsOneRequired is applied, Cobra checks if at least one of the flags in the group has been changed from its default value [1][4]. If no flags in the group are set, Cobra returns an error [2][4]. 3. Shell Completion: As a helper mechanism, if none of the flags in a "one-required" group are present, Cobra marks all flags in that group as "required" for the purpose of shell completion, so that the shell suggests them to the user [1][2][5]. Once at least one flag from the group is provided, this requirement is removed [2]. If your application logic requires an empty value (like an empty string) to be treated as a valid, non-empty user input, you must handle that validation manually [3]. Cobra's flag group validation is primarily designed to enforce presence/usage rather than validating the semantic content of the flag's value [4].

Citations:

🌐 Web query:

spf13/pflag MarkOneRequired Changed empty string behavior

💡 Result:

There is no behavior change in spf13/pflag specifically regarding MarkFlagsOneRequired (or similar flag marking functions) that causes it to treat empty strings as invalid or valid in a new way. The confusion stems from a fundamental design aspect of the spf13/cobra and spf13/pflag libraries: "Required" status in these libraries guarantees that a flag has been set (i.e., it appears in the command line arguments), not that the flag's value is non-empty [1][2]. Key takeaways: 1. Flag presence vs. value: When you use MarkFlagRequired or MarkFlagsOneRequired (a method in spf13/cobra, not pflag), the library checks only if the flag was provided by the user [1][3][4]. If a user explicitly passes an empty string (e.g., --flag=""), the flag is considered "set," and the "required" validation passes, even if the value is empty [5][1]. 2. No behavior change: There is no documented or historical change in pflag that altered this behavior to treat empty strings as invalid. This has been the consistent behavior of the library [5][1][2]. 3. Recommended approach: If you require a flag to have a non-empty value, you must perform this validation manually within your command's execution logic (e.g., in a PreRun or Run function) [5][1][2]. The libraries currently do not provide a built-in mechanism to mark a flag as "required to have a non-empty value" [1][6][2]. If you are encountering unexpected behavior, it is likely that your application logic is relying on the flag being absent rather than checking if the provided value is valid. Adding custom validation code to check for empty strings after parsing remains the standard practice for these libraries [1][2].

Citations:

🌐 Web query:

github.com/Azure/dev-spaces flag_groups.go pflag.MarkOneRequired Changed default value

💡 Result:

I couldn’t find any evidence connecting Azure/dev-spaces’ flag_groups.go (or a pflag.MarkOneRequired call) to a “Changed default value”. What I did find is that in Cobra’s flag group implementation, MarkFlagsOneRequired only annotates flags and does not define/change any default value behavior itself. In Cobra’s flag_groups.go, MarkFlagsOneRequired “marks the given flags with annotations” for validation/completion; it does not touch pflag.Flag.DefValue/Changed/NoOptDefVal or set a default value [1]. Separately, in pflag, Flag.changed semantics relate to whether the user set the flag versus it being left at default; and pflag’s default value is stored as DefValue. But I did not find any Azure/dev-spaces code or PR/commit where that default value for a specific flag group changed due to pflag.MarkOneRequired [2][3]. If you can share either: 1) the exact repository commit/branch you mean for Azure/dev-spaces, or 2) the exact flag name(s) involved, or 3) the exact diff/PR text that claims “Changed default value”, I can pinpoint the before/after and cite the specific line(s) in Azure/dev-spaces’ flag_groups.go/pflag usage.

Citations:

🌐 Web query:

github.com/Azure/dev-spaces flag_groups.go pflag.MarkOneRequired Changed default value

💡 Result:

The file flag_groups.go is a component of the spf13/cobra library, not the Azure/dev-spaces repository [1][2]. Cobra uses this file to manage logical flag relationships, such as mutually exclusive flags or groups where at least one flag is required [3][4][2]. The function MarkFlagsOneRequired (often referenced in the context of flag groups) marks a set of flags such that Cobra enforces that at least one of them must be provided [3][4]. If none of the flags in such a group are provided, Cobra triggers a validation error [3][4]. Regarding your query about a changed default value, there is no evidence of a direct modification to flag_groups.go within the Azure/dev-spaces repository, primarily because Azure Dev Spaces has been retired since October 31, 2023 [5]. It is possible you may be observing a change in a different project that vendors the cobra library (such as the Docker CLI or other tools that use pflag/cobra), or you might be encountering behavior related to how flag defaults are handled in modern CLI frameworks via annotations [6][2]. If you are looking to manage flag defaults or configurations in a more modern Azure-related CLI tool, techniques such as RegisterFlagOptions are now used to handle per-subcommand flag configuration and defaults declaratively [6].

Citations:

🌐 Web query:

spf13/cobra v1.10.2 flag_groups.go MarkFlagsOneRequired Changed

💡 Result:

The function MarkFlagsOneRequired, located in the flag_groups.go file of the spf13/cobra library, was introduced in version 1.8.0 [1]. This function allows developers to specify a group of flags where at least one flag from the set must be provided by the user [2][3]. If none of the specified flags are present when the command is executed, Cobra will generate an error [2]. In version 1.10.2, MarkFlagsOneRequired functions by performing the following steps: 1. It merges persistent flags to ensure the command has access to the full flag set [2]. 2. It iterates through the provided flag names, ensuring each exists; if a flag is not found, the function triggers a panic [2]. 3. It sets a oneRequiredAnnotation on each specified flag in the FlagSet [2]. This annotation stores the group of flag names, which Cobra later uses to validate that at least one flag from the set is present during execution [2][3]. Additionally, the implementation supports shell completion integration: if no flags from a one-required group are present, Cobra marks all flags in that group as required, which prompts the shell completion system to suggest them automatically [2][4]. MarkFlagsOneRequired is often combined with MarkFlagsMutuallyExclusive to enforce that exactly one flag from a specific group must be used [5][3].

Citations:

🌐 Web query:

github.com/spf13/cobra flag_groups.go validateRequiredFlags Changed annotation oneRequired

💡 Result:

In the spf13/cobra library, the reference to changing the annotation "oneRequired" refers to a cleanup and standardization effort introduced in pull request #2140 [1][2]. Previously, the library used internal constants like oneRequired as shorthand for flag group annotations [3][4]. In pull request #2140, these were updated to include an "Annotation" suffix to improve code clarity and distinguish them as constant identifiers [1][2][4]. Specifically: - The constant previously named oneRequired (with the value "cobra_annotation_one_required") was complemented or replaced in internal usage by oneRequiredAnnotation (which shares the same value) [1][4]. - This change was applied across the codebase, including in flag_groups.go, to allow developers to use more descriptive variable names when working with these annotations [1][2]. This update does not change the functionality of the "one required" flag group logic, which is used by the ValidateFlagGroups method to ensure that at least one flag from a specified set is provided by the user [5][6][3]. The underlying string value ("cobra_annotation_one_required") remains the same [4].

Citations:

🌐 Web query:

github.com/spf13/cobra v1.10.2 ValidateFlagGroups Changed annotations cobra_annotation_one_required Changed

💡 Result:

In Cobra version 1.10.2, the ValidateFlagGroups method and the cobra_annotation_one_required annotation continue to function as part of the flag validation system established in previous versions [1][2]. There were no breaking changes or modifications to the core logic of ValidateFlagGroups or this specific annotation in the v1.10.2 release [2][3]. ValidateFlagGroups is responsible for validating flag relationships, specifically: - Mutually exclusive flags (cobra_annotation_mutually_exclusive) [1][4] - Required flags within a group (cobra_annotation_required_if_others_set) [1][4] - At least one required flag in a group (cobra_annotation_one_required) [1][4][5] The cobra_annotation_one_required annotation is used to ensure that at least one flag from a defined set is provided by the user [6][5]. When ValidateFlagGroups is called (typically during command execution), it checks the status of these flags; if none of the flags in a group marked with cobra_annotation_one_required are set, it returns an error [1][5]. The release of v1.10.2 focused primarily on dependency updates (such as migrating YAML packages), linter fixes, and documentation improvements, rather than changes to the command or flag validation logic [2][3].

Citations:

🌐 Web query:

github.com/spf13/pflag Changed empty string value behavior

💡 Result:

The handling of empty string values in the pflag package has been the subject of ongoing discussion and targeted improvements, particularly regarding how slice-based flags (like StringArray and other typed slices) interpret an empty input [1][2][3]. Key behavior and changes include: 1. StringArray and Typed Slices: Historically, passing an empty string to certain slice flags resulted in an array containing a single empty string element (e.g., --flag="" resulting in [""], rather than an empty slice []) [1][2][3]. Recent updates, such as those found in PR #481 and PR #485 (implemented mid-2026), have been introduced to treat an explicit empty value (e.g., --flag=) as a non-nil empty slice [1][2]. This change aims to provide consistency across flag types and allow users to clear or reset slice flags to empty states [1][2]. 2. StringToString Flags: There have been long-standing requests to allow an empty value for StringToString flags to enable overriding default, non-empty maps [4]. However, maintainers have noted that changing this behavior is complex and carries the risk of breaking existing implementations, leading to it being deferred, often cited as out of scope for the 1.x release series [4]. 3. Usage/Help Text: Changes have been made to allow the suppression of default values in help/usage text [5]. By setting a flag's DefValue to an empty string, developers can hide the "(default %v)" suffix in the automatically generated usage information, which is useful for custom types where the zero value should not be explicitly shown [5][6]. Because these changes involve modifying how flags are parsed, maintainers have been cautious to ensure that these updates do not unintentionally break existing applications [4][3]. If you are working with these flags, it is recommended to review the specific pull requests and associated discussions (such as #415, #481, and #485) to understand how these behaviors may impact your specific implementation [1][2][3].

Citations:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== all occurrences of MarkFlagsOneRequired =="
rg -n 'MarkFlagsOneRequired' cmd pkg -S

echo "== all occurrences of ValidateFlagGroups ValidateRequiredFlags =="
rg -n 'ValidateFlagGroups|ValidateRequiredFlags|ValidateFlags|PreRunE' cmd -S

echo "== ReadCmd struct and Resolve-related methods =="
rg -n 'func \(cmd \*ReadCmd\)|resolveConfigFromContainer|resolveConfigFromIDLabels|FindRunning' cmd/config/read.go -A 40 -B 3

Repository: devsy-org/devsy

Length of output: 7215

Reject empty values from the Cobra flag groups.

MarkFlagsOneRequired only requires one flag in the group to be present; Cobra still accepts explicit empty values such as --workspace-folder="", which makes cmd/config/read.go#127 fall through to reading the current directory and cmd/internal/runusercommands.go#140-184 proceed with non-empty IdLabel values instead of the workspace/container sources. Add a post-parse guard in RunCmd.validate()/ReadCmd.Resolve that rejects these empty explicit values and test the "" case.

📍 Affects 2 files
  • cmd/config/read.go#L62-L63 (this comment)
  • cmd/config/read.go#L127-L127
  • cmd/internal/runusercommands.go#L140-L141
  • cmd/internal/runusercommands.go#L184-L184
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/config/read.go` around lines 62 - 63, Reject explicitly provided empty
flag values after parsing in RunCmd.validate() or ReadCmd.Resolve, rather than
relying only on MarkFlagsOneRequired. Ensure empty workspace-folder,
container-id, or id-label values cannot fall through to current-directory
handling or override workspace/container sources. Update cmd/config/read.go
lines 62-63 and 127, and cmd/internal/runusercommands.go lines 140-141 and 184
as applicable; add coverage for the "" input case.

Source: MCP tools

return readConfigCmd
}

Expand Down Expand Up @@ -122,14 +124,6 @@ func (cmd *ReadCmd) resolve(ctx context.Context) (
string,
error,
) {
if cmd.ContainerID == "" && cmd.WorkspaceFolder == "" && len(cmd.IDLabels) == 0 {
return nil, "", fmt.Errorf(
"either %s, %s, or %s must be provided",
names.Flag(names.WorkspaceFolder),
names.Flag(names.ContainerID),
names.Flag(names.IDLabel),
)
}
if cmd.ContainerID != "" {
return cmd.resolveConfigFromContainer(ctx)
}
Expand Down
9 changes: 2 additions & 7 deletions cmd/internal/runusercommands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ func NewRunUserCommandsCmd(f *flags.GlobalFlags) *cobra.Command {
),
)

runCmd.MarkFlagsOneRequired(names.WorkspaceFolder, names.ContainerID)

return runCmd
}

Expand Down Expand Up @@ -180,13 +182,6 @@ func (cmd *RunUserCommandsCmd) Run(ctx context.Context) error {
}

func (cmd *RunUserCommandsCmd) validate() error {
if cmd.WorkspaceFolder == "" && cmd.ContainerID == "" {
return fmt.Errorf(
"either %s or %s must be provided",
names.Flag(names.WorkspaceFolder),
names.Flag(names.ContainerID),
)
}
if cmd.ContainerID != "" && cmd.WorkspaceFolder == "" && cmd.Config == "" {
return fmt.Errorf(
"--config is required when --container-id is used without --workspace-folder",
Expand Down
8 changes: 2 additions & 6 deletions cmd/internal/runusercommands_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ func TestNewRunUserCommandsCmd_RequiresWorkspaceFolderOrContainerID(t *testing.T
cmd.SetArgs([]string{})
err := cmd.Execute()
assert.Error(t, err)
assert.Contains(t, err.Error(), "either --workspace-folder or --container-id must be provided")
assert.Contains(t, err.Error(),
"at least one of the flags in the group [workspace-folder container-id] is required")
}

func TestNewRunUserCommandsCmd_ContainerIDWithoutConfigFails(t *testing.T) {
Expand Down Expand Up @@ -297,11 +298,6 @@ func TestRunUserCommandsCmd_Validate(t *testing.T) {
false,
"",
},
{
"neither provided",
&RunUserCommandsCmd{GlobalFlags: &flags.GlobalFlags{}},
true, "either --workspace-folder or --container-id",
},
{
"container-id without config or workspace",
&RunUserCommandsCmd{GlobalFlags: &flags.GlobalFlags{}, ContainerID: testContainerID},
Expand Down
5 changes: 1 addition & 4 deletions cmd/workspace/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ func (cmd *BuildCmd) registerImageFlags(buildCmd *cobra.Command) {
cliflags.Bool(&cmd.Pull, names.Pull, false,
"Always attempt to pull a newer version of the base image when building"),
)
buildCmd.MarkFlagsMutuallyExclusive(names.Push, names.SkipPush)
}

func (cmd *BuildCmd) registerTestingFlags(buildCmd *cobra.Command) {
Expand Down Expand Up @@ -194,10 +195,6 @@ func (cmd *BuildCmd) prepareBuild(ctx context.Context) (*config.Config, error) {
}

func (cmd *BuildCmd) validateBuildFlags() error {
if cmd.PushDuringBuild && cmd.SkipPush {
return fmt.Errorf("cannot use %s and %s together",
names.Flag(names.Push), names.Flag(names.SkipPush))
}
if cmd.PushDuringBuild && cmd.Repository == "" {
return fmt.Errorf("%s requires %s to be specified",
names.Flag(names.Push), names.Flag(names.Repository))
Expand Down
8 changes: 8 additions & 0 deletions cmd/workspace/build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ func TestBuildCmd_NoBuildFlagParsesValue(t *testing.T) {
assert.True(t, val)
}

func TestBuildCmd_PushAndSkipPushMutuallyExclusive(t *testing.T) {
buildCmd := NewBuildCmd(&flags.GlobalFlags{})
buildCmd.SetArgs([]string{"--" + names.Push, "--" + names.SkipPush})
err := buildCmd.Execute()
require.Error(t, err)
assert.Contains(t, err.Error(), "none of the others can be")
}

func TestBuildCmd_NoCacheDefaultFalse(t *testing.T) {
buildCmd := NewBuildCmd(&flags.GlobalFlags{})
err := buildCmd.ParseFlags([]string{})
Expand Down
Loading