azure.ai.agents - adopt SDK root command and remove reserved flag conflicts#7947
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the azure.ai.agents extension command tree to rely on the azd extension SDK’s root command and inherited reserved global flags, eliminating collisions with azd’s reserved flag registry and aligning output/environment/no-prompt handling with the SDK.
Changes:
- Switch root command creation to
azdext.NewExtensionRootCommandand thread*azdext.ExtensionContextthrough subcommand constructors. - Remove subcommand redeclarations of reserved flags (
--output/-o,--environment) and useazdext.RegisterFlagOptions+extCtx.OutputFormatinstead. - Remove manual
PersistentPreRunEchaining on intermediate commands (files,sessions) now that traverse hooks are enabled by the SDK.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/extensions/azure.ai.agents/internal/cmd/root.go | Adopt SDK root command and pass ExtensionContext to subcommands. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/extension_context.go | Add ensureExtensionContext helper to allow nil context in tests. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/show.go | Use RegisterFlagOptions for --output and read extCtx.NoPrompt/OutputFormat. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/show_test.go | Update constructor signature + validate output flag options via annotations. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/files.go | Remove pre-run chaining; use extCtx.NoPrompt + RegisterFlagOptions for output. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/files_test.go | Remove recursion regression test; update constructors and output assertions. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/session.go | Remove pre-run chaining; use extCtx.NoPrompt/OutputFormat and RegisterFlagOptions. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/session_test.go | Update constructors and output assertions. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/run.go | Thread NoPrompt via ExtensionContext instead of global flags. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/monitor.go | Use extCtx.NoPrompt instead of global flags. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/monitor_test.go | Update constructors to accept ExtensionContext. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/invoke.go | Thread NoPrompt via action field sourced from ExtensionContext. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/invoke_test.go | Update constructor signature to accept ExtensionContext. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/init.go | Remove local --environment; source environment + no-prompt from ExtensionContext. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/init_*.go | Replace flags.NoPrompt usage with flags.noPrompt. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/init_test.go | Update tests for new initFlags shape (noPrompt field). |
| cli/azd/extensions/azure.ai.agents/internal/cmd/flag_options_test.go | Add helper asserting RegisterFlagOptions annotations for output. |
| cli/azd/extensions/azure.ai.agents/go.mod | Bump azd SDK dependency version. |
| cli/azd/extensions/azure.ai.agents/go.sum | Update sums for the azd SDK bump. |
| cli/azd/extensions/azure.ai.agents/extension.yaml | Bump requiredAzdVersion to a version that contains reserved-flag + flag-options support. |
jongio
left a comment
There was a problem hiding this comment.
Clean migration to azdext.NewExtensionRootCommand. Traced the full flag-threading path from SDK root through every subcommand - no stale rootFlags references survive and all noPrompt/OutputFormat reads happen after PersistentPreRunE populates ExtensionContext.
A few things I verified:
EnableTraverseRunHooks = true(set by the SDK) means the manualPersistentPreRunEchains infiles.goandsession.gowould double-fire if kept. Removing them is correct.RegisterFlagOptionsstores config as cobra annotations, so the oldcmd.Flags().GetString("output")assertions in tests correctly migrated toassertOutputFlagOptionscheckingcmd.Annotations.ensureExtensionContextreturning a zero-value context for nil input keeps test constructors simple without risk - the SDK populates the real context before anyRunEexecutes.- The
--environmentremoval frominitis safe since the SDK's root persistent flag covers it, andinit'sRunEreadsextCtx.Environmentbefore proceeding.
Pseudo-version in go.mod is expected per the existing conversation - just needs the semver tag bump before merge.
No blocking issues found.
…flicts Switch the azure.ai.agents extension to azdext.NewExtensionRootCommand so it stops registering flags that conflict with azd's reserved global flag set (--debug, --no-prompt, --environment, --output/-o).
a9ee765 to
18109be
Compare
2e14fbb to
a34ac8b
Compare
wbreza
left a comment
There was a problem hiding this comment.
✅ Approved — Clean SDK Migration
The migration from manual flag registration to azdext.NewExtensionRootCommand is correct, complete, and maintains full backward compatibility. No merge-blocking issues found. Confirms jongio's prior review.
Advisory Findings (P2-Medium — non-blocking)
-
RegisterFlagOptionsduplication — The same 5-line output flag registration block is repeated 6+ times acrossfiles.go,session.go, andshow.go. Consider extracting a shared helper likeregisterOutputFlagOptions(cmd). -
Missing
ensureExtensionContext(nil)unit test — The nil → zero-value contract inextension_context.gois intentional but undocumented by tests. A single test pinning this behavior would prevent accidental regressions. -
No integration test for
extCtxthreading — Tests verify command structure but none exercise the PersistentPreRunE → RunE pipeline to confirmextCtxis populated at execution time. -
PersistentPreRunErecursion test removed without replacement — Justified since the SDK now owns this viaEnableTraverseRunHooks, but a brief comment or replacement test would document that assumption.
Verified Clean
| Dimension | Result |
|---|---|
| Correctness | Migration complete — no stale rootFlags references |
| Backward Compat | All CLI flags still work identically for users |
| Security | Posture improved — input validation strengthened via AllowedValues |
| Description Alignment | All 7 PR claims verified against actual diff |
| Concurrency | Improved — global mutable state eliminated |
|
/check-enforcer evaluate |
|
/check-enforcer override |
Fixes #7796
This PR migrates the
azure.ai.agentsextension to the new azd extension SDK command helpers —azdext.NewExtensionRootCommand,azdext.NewListenCommand, andazdext.NewMetadataCommand— and consolidates per-command boilerplate that was duplicated across the cobra command tree.Why
Each azd extension previously hand-rolled its own root command, which meant manually registering the standard global flags (
--debug,--no-prompt,--cwd,-e/--environment,-o/--output), wiring the env-var fallback for those flags (AZD_*), threading the access token onto the command context, and constructing theExtensionHost/AzdClientlifecycle inlisten. This duplication caused several real problems in the existing code:show,files list,files stat) and theaddSessionFlagshelper redeclared--output/-o, conflicting with the SDK's reserved global flag and producingflag redefined: "o"panics when the SDK began enforcing its global registration.sessionscommand group manually chainedPersistentPreRunEto its parent and carried a long warning comment about an infinite-recursion footgun if the wrongParent()reference was used.setupDebugLoggingwas called explicitly from every command'sRunE(≈14 sites), and it was easy for a new command to forget to call it.listencommand's setup were ~50 lines of boilerplate that drifted from the equivalent code in other extensions.Changes
The root command is now built via
azdext.NewExtensionRootCommand, which provides the standard global flags, env-var fallback, trace context, and access token wiring. Per-subcommand customization of--output(default value, allowed values, completion) flows throughazdext.RegisterFlagOptionsinstead of redeclaring the inherited flag. Commands that need flag values now read them from the typed*azdext.ExtensionContextpassed into their constructors, replacing the previousrootFlagsglobal; a smallensureExtensionContexthelper makes it safe to construct commands withnilcontext in tests that don't exerciseRunE.The
listencommand is now created byazdext.NewListenCommand, with extension-specific service target and event handler registration moved into aconfigureExtensionHostcallback. Themetadatacommand is created inline viaazdext.NewMetadataCommand, deleting the equivalent local implementation. Debug logging setup (setupDebugLogging) is hoisted to the root command'sPersistentPreRunEso every subcommand inherits it via cobra'sEnableTraverseRunHooks, removing the per-command setup/cleanup pairs.The previous fragile
PersistentPreRunEchaining on thesessionsgroup is no longer needed and has been deleted along with its warning comment.Scope notes
NewVersionCommandwas evaluated but not adopted in this PR: the SDK helper exposes onlynameandversion, while the current command also surfacesCommitandBuildDate(populated at build time via-ldflags).Testing
go build,go vet,go test ./..., andgolangci-lint run ./...all pass cleanly. Existing tests were updated to read flag values fromextCtx.OutputFormatinstead of poking at cobra flag internals, and a newflag_options_test.goasserts that per-command output overrides are registered via the SDK's annotation contract.