Add reserved global flags registry for extensions#7312
Open
Conversation
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. |
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
}
jongio
force-pushed
the
fix/reserved-flags-registry
branch
2 times, most recently
from
517a3cc to
191e9bb
Compare
Member
Author
|
Responding to @spboyer's review: 1. Universal reserved-flag check will break existing extensions All 9 flags in the reserved list are legitimate azd globals: The extensions that currently collide ( 2. collectConflicts skip logic masks subcommand flag conflicts Good catch. Implemented pointer equality in d14bf4b — now only skips when the flag is literally the same |
jongio
force-pushed
the
fix/reserved-flags-registry
branch
from
d14bf4b to
5288d7b
Compare
jongio
requested review from
achauhan-scc,
bwateratmsft,
danieljurek,
karolz-ms,
kingernupur,
rabollin,
saanikaguptamicrosoft,
therealjohn,
trangevi and
trrwilson
as code owners
Collaborator
VSCode Extension Installation Instructions
|
Member
Observation: Two extension authoring models and the enforcement gapAfter PR #6856 introduced
The reserved flag enforcement in this PR only applies to extensions that opt into This creates a gap that extension authors need to understand when choosing how to build their extension. I've created:
Worth considering whether the design doc in this PR ( |
Member
Author
|
Great analysis. You're right — enforcement only covers SDK-managed extensions using Since all in-repo extensions are Go, the simplest path is to have each self-managed extension add a Added a 'Scope & Limitations' section to the design doc in this PR referencing #7405 and your authoring models doc in #7406. The migration can happen incrementally as a follow-up. |
Member
Author
|
/azp run azure-dev - cli |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jongio
force-pushed
the
fix/reserved-flags-registry
branch
from
8750eb3 to
9f96cf5
Compare
wbreza
approved these changes
Mar 31, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-Review — PR #7312 (3 new commits since Mar 31 review)
Prior Findings: All 4 ✅ Resolved
| # | Priority | Finding | Status |
|---|---|---|---|
| 1 | High | Root persistent flag exemption allows reserved flag collisions | ✅ Fixed — azd-sdk-root annotation check gates the exemption |
| 2 | Medium | checkFlag short-circuits on first collision |
✅ Fixed — returns []FlagConflict for compound conflicts |
| 3 | Low | init() vs func-literal inconsistency |
✅ Fixed — func-literal var initializers |
| 4 | Low | commandPath O(n²) prepend-based slice building |
✅ Fixed — append + slices.Reverse |
New Commits Reviewed
| Commit | Assessment |
|---|---|
6011f2b fix: tighten root persistent flag exemption to SDK-created roots only |
✅ Correct — adds annotation guard, new test validates |
1dbc24f docs: add scope & limitations section for enforcement gap |
✅ Good — documents SDK-only enforcement boundary |
9f96cf5 fix: revert unrelated apphost changes |
✅ Clean revert |
New Findings: None
All 11 test cases pass. Build succeeds. Backwards compatibility preserved.
Overall: ✅ APPROVE — all findings resolved, SDK annotation approach is clean and correct.
Review performed with GitHub Copilot CLI
JeffreyCA
reviewed
Mar 31, 2026
weikanglim
reviewed
Apr 1, 2026
weikanglim
reviewed
Apr 1, 2026
Comment on lines
+32
to
+33
| {Long: "trace-log-file", Short: "", Description: "Write a diagnostics trace to a file."}, | ||
| {Long: "trace-log-url", Short: "", Description: "Send traces to an OpenTelemetry-compatible endpoint."}, |
Contributor
There was a problem hiding this comment.
These feel like internals that azd should handle through the SDK rather than via the CLI interface.
Member
Author
There was a problem hiding this comment.
These are already registered as CLI flags - +""+--docs+""+ is public (visible in +""+--help+""+ on every command via +""+cobra_builder.go+""+), and the trace flags are hidden but pre-parsed by +""+CreateGlobalFlagSet+""+ in +""+�uto_install.go+""+. The SDK also mirrors them in +""+�xtension_command.go+""+ for host-to-extension propagation.
They need to be reserved because azd pre-parses them before dispatching to the extension. If an extension defines +""+--trace-log-file+""+ for a different purpose, azd's pre-parsing would consume the value, causing silent breakage. The reservation prevents that shadowing.
weikanglim
reviewed
Apr 1, 2026
| {Long: "cwd", Short: "C", Description: "Sets the current working directory."}, | ||
| {Long: "debug", Short: "", Description: "Enables debugging and diagnostics logging."}, | ||
| {Long: "no-prompt", Short: "", Description: "Accepts the default value instead of prompting."}, | ||
| {Long: "output", Short: "o", Description: "The output format (json, table, none)."}, |
Contributor
There was a problem hiding this comment.
The output and environment flag are NOT global in azd. They are lit up per-command based on the intended usage. How should one reconcile the differences?
Member
Author
There was a problem hiding this comment.
You're right that --environment/-e and --output/-o aren't registered globally via CreateGlobalFlagSet - they're added per-command. But they still need to be reserved for extensions because:
- Short flags are the scarce resource. If an extension uses
-efor--endpoint, any azd command that adds--environmenton the same command path creates a shorthand collision. Cobra panics on duplicate shorthands, so the extension breaks when azd evolves. - User expectations. Users expect
-eto mean environment across azd. An extension that uses-efor something else creates confusion even if there's no technical collision today.
The reserved list is better understood as "namespace reservations" rather than "currently global flags." The per-command vs. global distinction matters for how azd registers them internally but not for whether extensions should avoid them.
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>
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>
…tentFlags check - Fix spec test name reference (TestReservedFlagsInSyncWithInternal) - Categorize host-side flags by registration mechanism (CreateGlobalFlagSet vs root persistent vs cobra built-in) - Add short-name parity assertion to sync test - Check cmd.PersistentFlags() in collectConflicts (not just cmd.Flags()) - Add PersistentFlagCollision test case Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ag shadows The previous name-based lookup would skip validation when a subcommand defined its own flag with the same name as a root persistent flag. Using pointer equality ensures only the actual inherited root flag objects are skipped, while same-named flags on subcommands are still validated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ct compound conflicts, consistency fixes
The root persistent flag skip logic previously exempted ALL flags on root.PersistentFlags() via pointer equality. This meant an extension using a plain root (not NewExtensionRootCommand) could add a root persistent flag colliding with reserved globals without detection. Now only roots created by NewExtensionRootCommand (marked with an azd-sdk-root annotation) get the exemption. Extensions using plain roots have all their root persistent flags validated against the reserved list. Adds test proving plain-root collisions are caught. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document that reserved flag enforcement only applies to SDK-managed extensions using azdext.Run(). Self-managed extensions (6 of 9 in-repo) bypass enforcement. Reference #7405 and #7406 for tracking and the authoring models documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Removes accidental changes to detect_confirm_apphost.go that don't belong in this PR. Restores warning message display and removes magenta coloring that was part of a different feature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
force-pushed
the
fix/reserved-flags-registry
branch
from
9f96cf5 to
1897488
Compare
📋 Milestone: April 2026This work is tracked for April 2026. The team will review it soon! |
- internal/reserved_flags.go is now the single source of truth - pkg/azdext/reserved_flags.go imports from internal instead of maintaining its own copy - Removed the sync test (TestReservedFlagsInSyncWithInternal) since there's only one list - Added cross-reference comment in CreateGlobalFlagSet (auto_install.go) 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
|
wbreza
approved these changes
Apr 7, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Code Re-Review — PR #7312
Add reserved global flags registry for extensions by @jongio
Re-review after new commit
Latest commit (f701b5a, Apr 7) centralizes reserved flag definitions — internal/reserved_flags.go is now the single source of truth, eliminating the duplicate list in pkg/azdext. Clean DRY improvement: -50 net lines, sync test removed since there's only one list.
Prior Review: All ✅ Resolved
All 5 findings from my two review rounds remain addressed:
| Round | Findings | Status |
|---|---|---|
| Round 1 (4 findings) | Validation errors lost, short-circuits, init inconsistency, O(n²) | All ✅ |
| Round 2 (1 finding) | Root persistent flag exemption | ✅ Fixed (SDK annotation) |
✅ What Looks Good
- Single source of truth for reserved flags eliminates drift risk
- Pointer-equality skip logic in
collectConflictsis correct - SDK annotation (
azd-sdk-root) gates the exemption properly - Comprehensive 11-test suite covers all validation paths
Overall Assessment: Re-approve — all prior findings resolved, new commit is a clean improvement.
Review performed with GitHub Copilot CLI
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