Add reserved global flags registry for extensions#7312
Open
Conversation
Introduce a formal registry of azd's global flags that extensions must not reuse. This prevents flag collisions like the -e conflict that caused #7271 where an extension's -e (--project-endpoint) shadowed azd's -e (--environment). The registry lists all global flags (-e, -C, -o, -h, --debug, --no-prompt, --docs, --trace-log-file, --trace-log-url) with lookup helpers that can be used for build-time validation and runtime conflict detection. Closes #7307 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a centralized registry of global “reserved” flags so extensions can be prevented from reusing azd-owned flags (avoiding collisions like -e).
Changes:
- Introduces
ReservedFlag+ canonicalReservedFlagslist and lookup helpers for short/long flags. - Builds short/long lookup indexes and exposes helper functions (
IsReserved*,GetReserved*). - Adds a focused test suite covering lookup behavior, duplicates, and registry consistency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cli/azd/internal/reserved_flags.go | Adds the reserved flags registry, lookup maps, and helper APIs. |
| cli/azd/internal/reserved_flags_test.go | Adds unit tests validating registry population, lookups, and invariants. |
ValidateNoReservedFlagConflicts() walks the extension command tree before execution and errors if any extension-defined flag collides with a reserved azd global flag (e.g. -e, -C, -o). This catches conflicts at extension development/test time, regardless of which repo the extension lives in. The SDK's own root persistent flags (--environment, --debug, etc.) are excluded from the check since they intentionally mirror azd globals. A sync test ensures the SDK and internal reserved flag lists stay aligned. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lags section - Add docs/specs/extension-flags/spec.md: core architecture spec documenting how azd pre-parses global flags, propagates via env vars, and dispatches to extensions. Includes reserved flag registry and Adding a New Global Flag checklist. - Update cli/azd/docs/extensions/extensions-style-guide.md: add Reserved Global Flags section under Parameter and Flag Consistency with flag table, DO/DON'T guidance, collision error example, and link to core spec. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move spec from docs/specs/extension-flags/spec.md to cli/azd/docs/design/extension-flag-architecture.md to follow the existing convention for azd design docs. Update cross-reference in extensions style guide. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Change ReservedFlags (exported slice) to reservedFlags (unexported) - Add ReservedFlags() function returning a defensive copy - Replace init() with func-literal var initializers to avoid ordering hazards - Fix 'Open Telemetry' → 'OpenTelemetry-compatible' spelling - Update test files and docs to use ReservedFlags() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
|
|
||
| ### Sync Test | ||
|
|
||
| A test (`TestReservedFlagNames_SyncWithInternal`) ensures the SDK-side and host-side reserved flag lists stay in sync. If a developer adds a new global flag to one list but not the other, the test fails. |
There was a problem hiding this comment.
The sync-test name referenced here (TestReservedFlagNames_SyncWithInternal) doesn’t match the actual test in this PR (TestReservedFlagsInSyncWithInternal in pkg/azdext/reserved_flags_test.go). Please update the spec so readers can find the correct test.
Suggested change
| A test (`TestReservedFlagNames_SyncWithInternal`) ensures the SDK-side and host-side reserved flag lists stay in sync. If a developer adds a new global flag to one list but not the other, the test fails. | |
| A test (`TestReservedFlagsInSyncWithInternal`) ensures the SDK-side and host-side reserved flag lists stay in sync. If a developer adds a new global flag to one list but not the other, the test fails. |
Comment on lines
+120
to
+145
| func TestReservedFlagsInSyncWithInternal(t *testing.T) { | ||
| // Verify the SDK reserved flag list stays in sync with the internal registry. | ||
| // If this test fails, you added a flag to one list but not the other. | ||
| sdkNames := ReservedFlagNames() | ||
| internalFlags := internal.ReservedFlags() | ||
|
|
||
| sdkSet := make(map[string]bool, len(sdkNames)) | ||
| for _, name := range sdkNames { | ||
| sdkSet[name] = true | ||
| } | ||
|
|
||
| internalSet := make(map[string]bool, len(internalFlags)) | ||
| for _, f := range internalFlags { | ||
| internalSet[f.Long] = true | ||
| } | ||
|
|
||
| for name := range sdkSet { | ||
| require.True(t, internalSet[name], | ||
| "azdext reserved_flags.go has %q but internal.ReservedFlags() does not — add it to internal/reserved_flags.go", | ||
| name) | ||
| } | ||
|
|
||
| for name := range internalSet { | ||
| require.True(t, sdkSet[name], | ||
| "internal.ReservedFlags() has %q but azdext reserved_flags.go does not — add it to pkg/azdext/reserved_flags.go", | ||
| name) |
There was a problem hiding this comment.
TestReservedFlagsInSyncWithInternal only compares the reserved flag long names across the SDK and internal registries. A mismatch in short-hands (e.g., if one side drops/changes C or h) would not be caught by this sync test. Consider asserting short-name parity too (or adding targeted tests for each reserved shorthand).
Comment on lines
+115
to
+133
| // collectConflicts recursively collects flag conflicts from the command tree. | ||
| func collectConflicts(root, cmd *cobra.Command) []FlagConflict { | ||
| var conflicts []FlagConflict | ||
|
|
||
| // Check flags defined directly on this command (not inherited from parents). | ||
| // We use cmd.Flags() instead of cmd.LocalFlags() because LocalFlags() | ||
| // triggers cobra's mergePersistentFlags, which panics on shorthand conflicts. | ||
| cmd.Flags().VisitAll(func(f *pflag.Flag) { | ||
| // Skip flags that are defined on the root's persistent flag set. | ||
| // Those are the SDK-provided azd-compatible flags and are intentional. | ||
| if rootFlag := root.PersistentFlags().Lookup(f.Name); rootFlag != nil { | ||
| return | ||
| } | ||
|
|
||
| if c, ok := checkFlag(cmd, f); ok { | ||
| conflicts = append(conflicts, c) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
collectConflicts only visits cmd.Flags(), so it never validates flags defined via cmd.PersistentFlags() (on root or subcommands). That allows reserved short/long collisions to slip through (e.g., an extension can define a persistent -e flag on the root and pass this validation). Consider checking both cmd.Flags() and cmd.PersistentFlags() for each command, and ensure the “allow SDK root persistent flags” exemption only skips the known SDK global flags rather than skipping every root persistent flag unconditionally.
Suggested change
| // collectConflicts recursively collects flag conflicts from the command tree. | |
| func collectConflicts(root, cmd *cobra.Command) []FlagConflict { | |
| var conflicts []FlagConflict | |
| // Check flags defined directly on this command (not inherited from parents). | |
| // We use cmd.Flags() instead of cmd.LocalFlags() because LocalFlags() | |
| // triggers cobra's mergePersistentFlags, which panics on shorthand conflicts. | |
| cmd.Flags().VisitAll(func(f *pflag.Flag) { | |
| // Skip flags that are defined on the root's persistent flag set. | |
| // Those are the SDK-provided azd-compatible flags and are intentional. | |
| if rootFlag := root.PersistentFlags().Lookup(f.Name); rootFlag != nil { | |
| return | |
| } | |
| if c, ok := checkFlag(cmd, f); ok { | |
| conflicts = append(conflicts, c) | |
| } | |
| }) | |
| // sdkRootPersistentFlagNames lists the known SDK-provided global flags that are | |
| // intentionally allowed on the root command and should be exempt from | |
| // reserved-flag collision checks. Only these specific root persistent flags | |
| // are skipped; any other root persistent flag (e.g., defined by an extension) | |
| // is still validated. | |
| var sdkRootPersistentFlagNames = map[string]struct{}{ | |
| // NOTE: Keep this list in sync with the SDK's global/root persistent flags. | |
| // Examples (adjust as appropriate for the actual SDK): | |
| // "environment": {}, | |
| // "output": {}, | |
| // "debug": {}, | |
| } | |
| func isSDKRootPersistentFlag(name string) bool { | |
| _, ok := sdkRootPersistentFlagNames[name] | |
| return ok | |
| } | |
| // collectConflicts recursively collects flag conflicts from the command tree. | |
| func collectConflicts(root, cmd *cobra.Command) []FlagConflict { | |
| var conflicts []FlagConflict | |
| // Track which (name, shorthand) combinations we've already checked for this | |
| // command to avoid duplicate conflict reports when flags appear in multiple | |
| // flag sets (e.g., due to cobra's flag merging behavior). | |
| checked := make(map[string]struct{}) | |
| checkFlagFromSet := func(f *pflag.Flag) { | |
| key := f.Name + "|" + f.Shorthand | |
| if _, seen := checked[key]; seen { | |
| return | |
| } | |
| checked[key] = struct{}{} | |
| // Skip specific SDK-provided root persistent flags. We only skip when the | |
| // flag is actually defined on the root's persistent flag set and its name | |
| // is in the known SDK list. | |
| if rootFlag := root.PersistentFlags().Lookup(f.Name); rootFlag != nil && isSDKRootPersistentFlag(f.Name) && rootFlag == f { | |
| return | |
| } | |
| if c, ok := checkFlag(cmd, f); ok { | |
| conflicts = append(conflicts, c) | |
| } | |
| } | |
| // Check flags visible on this command (may include merged persistent flags). | |
| // We use cmd.Flags() instead of cmd.LocalFlags() because LocalFlags() | |
| // triggers cobra's mergePersistentFlags, which panics on shorthand conflicts. | |
| cmd.Flags().VisitAll(checkFlagFromSet) | |
| // Also explicitly check flags defined as persistent on this command. This | |
| // ensures that persistent flags (on root or subcommands) are validated even | |
| // if they are not uniquely represented in cmd.Flags(). | |
| cmd.PersistentFlags().VisitAll(checkFlagFromSet) |
Comment on lines
+73
to
+87
| ## Global Flags (Host Side) | ||
|
|
||
| These flags are registered in `CreateGlobalFlagSet()` (`cmd/auto_install.go`) and pre-parsed by `ParseGlobalFlags()` before command dispatch: | ||
|
|
||
| | Long Name | Short | Type | Default | Hidden | Description | | ||
| |-----------|-------|------|---------|--------|-------------| | ||
| | `environment` | `e` | string | `$AZURE_ENV_NAME` | No | The name of the environment to use | | ||
| | `cwd` | `C` | string | `""` | No | Sets the current working directory | | ||
| | `debug` | — | bool | `false` | No | Enables debugging and diagnostics logging | | ||
| | `no-prompt` | — | bool | `false` | No | Accepts default value instead of prompting | | ||
| | `output` | `o` | string | `"default"` | No | The output format (json, table, none) | | ||
| | `help` | `h` | — | — | No | Help for the current command (cobra built-in) | | ||
| | `docs` | — | — | — | No | Opens documentation for the current command | | ||
| | `trace-log-file` | — | string | `""` | Yes | Write a diagnostics trace to a file | | ||
| | `trace-log-url` | — | string | `""` | Yes | Send traces to an OpenTelemetry endpoint | |
There was a problem hiding this comment.
This section says the listed flags are "registered in CreateGlobalFlagSet()" and "pre-parsed by ParseGlobalFlags()", but in code cmd/auto_install.go's CreateGlobalFlagSet only defines cwd, debug, no-prompt, trace-log-file, and trace-log-url. Flags like environment, output, and docs appear to be registered elsewhere, so the spec should be updated to reflect the actual registration/parsing pipeline (or the code should be aligned with the spec).
spboyer
reviewed
Mar 26, 2026
Member
spboyer
left a comment
spboyer
left a comment
There was a problem hiding this comment.
Code Review
1. Universal reserved-flag check will break existing extensions
cli/azd/pkg/azdext/reserved_flags.go:22 — High
The registry treats environment, output, docs, etc. as universally reserved for every extension. azdext.Run() exits on any match before command execution. This is broader than what azd actually pre-parses today (CreateGlobalFlagSet only pre-parses cwd, debug, no-prompt, trace-log-file, trace-log-url). In-tree extensions like azure.ai.agents (uses -e for --environment, -o for --output) and azure.ai.finetune (uses -e for --project-endpoint) will fail at startup.
Fix: Only reserve flags that are truly universal, or scope the extra reservations to roots created with NewExtensionRootCommand() that actually register those globals.
2. collectConflicts skip logic masks subcommand flag conflicts
cli/azd/pkg/azdext/reserved_flags.go (collectConflicts function) — Medium
The skip logic uses name-based lookup (root.PersistentFlags().Lookup(f.Name)) to avoid flagging the SDK's own persistent flags. But before Cobra's mergePersistentFlags (which happens during Execute(), not validation), a subcommand's cmd.Flags().VisitAll() only yields locally-defined flags. If a subcommand locally defines a flag with the same long name as a root persistent flag (e.g., --output meaning "output file" instead of "output format"), the name lookup finds root's flag and skips validation entirely. The conflict goes unreported.
Fix: Use pointer equality instead of name equality:
if rootFlag := root.PersistentFlags().Lookup(f.Name); rootFlag != nil && rootFlag == f {
return
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a formal registry of reserved global short flags that extensions must not reuse. This prevents future conflicts like the
-ecollision that caused #7271.What's included
cli/azd/internal/reserved_flags.go-ReservedFlagstruct with 9 entries (-e,-C,-o,--debug,--no-prompt, etc.), lookup maps, and helper functions (IsReserved,GetReserved)cli/azd/internal/reserved_flags_test.go- 7 test functions with 29 subtests covering all lookup paths and edge casesWhy
@vhvb1989 raised this concern on #7035 - there should be a way to block extensions from using known global flags. This is the foundation for that enforcement.
Closes #7307
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com