azd x: non-fatal metadata warnings and azd x init wizard polish#8197
Merged
Conversation
Copilot
AI
changed the title
[WIP] Fix azd x init to handle extension metadata warnings
Treat extension metadata warnings as non-fatal
May 15, 2026
JeffreyCA
force-pushed
the
copilot/fix-azd-x-init-metadata-warnings
branch
from
7072a55 to
bac949d
Compare
JeffreyCA
changed the base branch from
main
to
copilot/migrate-to-new-extension-root-command
JeffreyCA
force-pushed
the
copilot/fix-azd-x-init-metadata-warnings
branch
from
bac949d to
47e5638
Compare
JeffreyCA
changed the title
azd x: non-fatal metadata warnings and azd x init wizard polish
Base automatically changed from
copilot/migrate-to-new-extension-root-command
to
main
May 18, 2026 18:15
`azd x build` previously treated validation warnings (e.g. missing `providers` when `service-target-provider` is declared) as failures. That aborted `azd x init`, and the build subprocess output was discarded so the user never saw why. Warning handling: - `ux.TaskList`: a task returning `(Warning, err)` no longer cancels subsequent tasks, and warning detail renders with warning styling instead of error styling. - Extract `validateExtensionMetadata` as a shared helper. `azd x init` validates the in-memory schema directly instead of shelling out to `azd x build` and parsing its rendered output. - Show a short summary in the task list and the full bulleted detail once below it. - Drop the redundant "Local extension source already exists." message. - Rephrase warnings to name the YAML field, the capability that needs it, and what to set. Wizard UX (resolves #8207): - Confirmation prompt defaults to "yes". - Tags prompt is optional; also fixes a pre-existing bug where parsed tags were discarded. - Translate dotted namespaces (`ai.project`) into the actual command path (`azd ai project <command>`) in the generated `usage` field and follow-up hints, matching how `bindExtension` registers them. - Generated `extension.yaml` includes a `yaml-language-server` header pointing at the published schema URL. Scaffolded extension root command: - The Go template set `Use` to `"azd <namespace> <command> [options]"`, so cobra parsed `"azd"` as the root command name and rendered `Usage: azd [command]`. Use the leaf segment of the namespace for both `Name` and `Use`, matching the convention in `azure.ai.agents`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JeffreyCA
force-pushed
the
copilot/fix-azd-x-init-metadata-warnings
branch
from
47e5638 to
08859f8
Compare
Contributor
|
/azp run azure-dev - cli |
|
Azure Pipelines successfully started running 1 pipeline(s). |
JeffreyCA
requested review from
hemarina,
jongio,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves azd x init and azd x build extension metadata validation behavior by making warnings non-fatal and polishing generated scaffold UX for extension authors.
Changes:
- Updates task list warning handling so warning-state errors render as warnings and do not stop subsequent tasks.
- Adds shared extension metadata validation with improved warning text and uses it from both build and init flows.
- Improves init wizard/scaffold output for tags, confirmation defaults, dotted namespaces, schema modelines, and generated Go root command usage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
cli/azd/pkg/ux/task_list.go |
Changes task warning errors to be non-fatal and renders warning details with warning styling. |
cli/azd/pkg/ux/render_test.go |
Adds coverage for warning tasks continuing to subsequent tasks. |
cli/azd/extensions/microsoft.azd.extensions/internal/resources/languages/go/internal/cmd/root.go.tmpl |
Uses the leaf namespace for generated Go cobra root command metadata. |
cli/azd/extensions/microsoft.azd.extensions/internal/cmd/init.go |
Adds metadata validation, warning output, namespace formatting, optional tags, schema header, and build failure output handling. |
cli/azd/extensions/microsoft.azd.extensions/internal/cmd/init_test.go |
Adds tests for namespace formatting and metadata validation behavior. |
cli/azd/extensions/microsoft.azd.extensions/internal/cmd/build.go |
Extracts shared metadata validation and makes warnings non-fatal during build. |
wbreza
reviewed
May 18, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Code Review — �zd x: non-fatal metadata warnings & init wizard polish
Automated review (Claude Opus 4.7) — 8 parallel focus-area passes. Multi-reviewer consensus findings called out. Recommendation: comment-only so this is discussion fuel, not a block — but the P1 items below are real and worth resolving before merge.
TL;DR
The core change — splitting Warning from Error in TaskList — is conceptually sound and the new �alidateExtensionMetadata extraction is clean. However the implementation has one real backward-compat regression in an unrelated consumer (cmd/tool.go), a rendering bug (multi-line warning string corrupts the live canvas), an ordering bug (filesystem mutation before validation), and several documentation/test gaps for behavior the PR is explicitly trying to change.
🔴 P1 — Should fix before merge
| # | File:line | Finding |
|---|---|---|
| 1 | pkg/ux/task_list.go:373–376 (shouldCollectTaskError) + cmd/tool.go:~1241,1296,1303–1310 | Silent backward-compat break in unrelated consumer. Pre-PR, any task returning (ux.Warning, err) had its error joined into Run()'s aggregated return. Existing �zd tool install/uninstall already returns (ux.Warning, fmt.Errorf("%s did not succeed", …)) when a tool fails. Post-PR that error is silently dropped → the "Some tools could not be installed…" notice never fires and �zd tool install/uninstall exits 0 even when tools fail. The Warning semantics flip wasn't audited against existing call sites. Fix: either (a) audit and migrate cmd/tool.go to return Error for real failures (preferred — its current "Warning + err" was always semantically muddled), or (b) keep collecting Warning errors but branch on Warning only at the "skip remaining" gate. |
| 2 | �xtensions/microsoft.azd.extensions/internal/cmd/build.go:135 (�alidationWarningsMessage) → rendered via pkg/ux/task_list.go:255–262 | Multi-line warning string corrupts the live canvas. �alidationWarningsMessage returns "validation warnings:\n - foo\n - bar" and is set as the task's error. The TaskList renders it inline as (%s) on a single status row; embedded newlines + bullets break ANSI cursor positioning during the per-second canvas re-render. Pre-PR this string never reached this code path because Warning was treated as Error. Fix: keep the inline summary single-line (e.g. "N validation warning(s)") and emit detailed bullets after askList.Run() returns — exactly the pattern init.go's new writeCollectedWarnings already uses. Have �uild.go reuse it. |
| 3 | �xtensions/microsoft.azd.extensions/internal/cmd/init.go (task order in | |
| unInitAction) | Validation runs after the filesystem mutation. �alidateExtensionAction currently runs after createExtensionDirectoryAction. Schema validation is pure (no I/O); if it returns Error, the partially-created directory is left on disk and confuses subsequent re-runs ("directory exists / continue to create/update files"). Move validation before directory creation. | |
| 4 | �xtensions/microsoft.azd.extensions/internal/cmd/init.go:~373–380 (early-return on askList.Run() != nil) | Collected warnings dropped exactly when users need them. If a later task (build/package/publish) fails, the function returns early and writeCollectedWarnings(buildWarnings) is never invoked — even though the inline status said "see details below". Fix: defer writeCollectedWarnings(buildWarnings) once warnings are populated, or call it unconditionally before the early return. |
| 5 | �xtensions/microsoft.azd.extensions/CHANGELOG.md (top) | No changelog entry for a user-visible extension behavior change. Latest release (0.10.0, 2026-03-04) doesn't include this and there's no Unreleased section. Extension authors need to know about it — �zd x build exit semantics change, and the generated |
| oot.go's Name: field changes from full namespace to leaf (see #9 below). | ||
| 6 | pkg/ux/render_test.go:561–589 (TestTaskList_Run_warningContinues) + :173–195 (TestTaskList_Render_warning) | The actual visual bug fix is untested. The literal change at ask_list.go:261 is WithErrorFormat → WithWarningFormat (red → yellow). Neither test asserts the ANSI format used — they only �ssert.Contains(output, "partial failure"), which would have passed pre-PR too. Add an assertion that the warning row uses output.WithWarningFormat(...) and explicitly does not contain WithErrorFormat(...). Also assert l.errors is empty after the warning-only run, to lock shouldCollectTaskError's contract. |
🟡 P2 — Should fix before merge, lower urgency
| # | File:line | Finding |
|---|---|---|
| 7 | pkg/ux/task_list.go:85–95 (TaskState constants) | Warning, Error, Success, etc. have no GoDoc. The Warning semantics just changed silently; document that (Warning, err) is display-only and execution continues. AGENTS.md requires GoDoc on exported types. |
| 8 | �xtensions/.../build.go (overall) | �zd x build with warnings now silently exits 0, undocumented, no --strict/--warnings-as-errors flag. CI scripts get no signal. Either document explicitly in the command's Long help, or add a flag. |
| 9 | �xtensions/.../resources/languages/go/internal/cmd/root.go.tmpl:13–14 | Template Use: and Name: changed from full {{.Metadata.Namespace}} to {{.LeafNamespace}}. Breaking change for any generated extension that referenced its own name (telemetry/log scopes). At minimum, expose both LeafNamespace and FullNamespace and call out the change in the changelog. |
| 10 | �xtensions/.../init.go collectExtensionMetadataFromFlags (--no-prompt path) | No validation of namespace shape. Values like "a..b" or empty produce |
| amespaceCommandPath == "a b" and LeafNamespace == "", generating Name: "" in | ||
| oot.go.tmpl which cobra rejects at runtime. Validate ^[a-z][a-z0-9](.[a-z][a-z0-9])*$ in both interactive and scripted paths. | ||
| 11 | �xtensions/.../init.go (deleted "Local extension source already exists" notice) | The post-run output.WithWarningFormat(...) notice that fired when localRegistryExists was removed. The Skipped row still appears but the explicit follow-up is gone — UX regression. Restore or route through writeCollectedWarnings. |
| 12 | init.go writeCollectedWarnings vs pkg/ux/task_list.go:43 vs �uild.go validationWarningsMessage | Inconsistent warning text formatting across three sites: "(!) Warning " (TaskList), "WARNING:" (writeCollectedWarnings, ALL CAPS), "validation warnings:" (build.go, lowercase + colon), "%d warning(s) — see details below" (init.go, em-dash). Pick one. |
| 13 | �uild.go:56–72 ↔ init.go:196–214 | Duplicated validation-result handling. Both call �alidateExtensionMetadata, format errors with slightly different prefixes, wrap with common.NewDetailedError("Validation failed", …), and format warnings differently. Extract a shared helper. |
| 14 | init.go buildExtensionAction (~line 226) | cmd.CombinedOutput() captured to �uildCommandOutput only when �rr != nil. If �zd x build succeeds but emits warnings to stdout/stderr (now possible because warnings are non-fatal), the user never sees them. Always retain subprocess output. |
| 15 | init.go buildExtensionAction (~lines 249–292) | PATH-dependent guarantee. Shells out to whichever �zd is on PATH. The "warnings are non-fatal" promise only holds when the spawned binary contains this PR. With an older �zd installed, the subprocess still aborts on warnings. Document the runtime dependency. |
| 16 | init.go (tags prompt) | Tags now optional but no length/control-char sanitization. Malformed tags break the YAML and the new schema-header line. Add a cap and reject control chars. |
| 17 | �uild.go:~73 �alidateExtensionMetadata GoDoc | Comment says warnings "flag recommended but optional fields", but missing |
| amespace (with custom-commands capability) is now reported as an error, not a warning. Update doc. | ||
| 18 | init_test.go:464–526 (TestValidateExtensionMetadata) | Pins exact long multi-sentence error/warning strings. These are tunable user-facing copy and will break on every wording tweak without catching real regressions. Use �ssert.Len + �ssert.Contains on stable key prefixes. |
| 19 | pkg/ux/task_list.go:373–376 (shouldCollectTaskError) | New helper has no direct unit test; only covered transitively. Add a table test covering each TaskState × |
| il/non-nil �rr. | ||
| 20 | �xtensions/.../init.go writeCollectedWarnings, writeFailedCommandOutput | Both write to mt.Println/ mt.Print directly, making them untestable. Refactor to accept an io.Writer. |
🟢 P3 — Nice to have
| # | File:line | Finding |
|---|---|---|
| 21 | pkg/ux/task_list.go:307, 166 | Pre-existing data race: |
| unSyncTasks reads len(t.errors) without �rrorMutex; Run() reads .errors after Wait() without the lock. Safe today via happens-before from waitGroup.Wait(), but the new indirection makes it easier to introduce a real race later. Tighten now or document the synchronization contract. | ||
| 22 | pkg/ux/task_list.go:230–244 (Render) | Render reads ask.State/ ask.Error/ ask.endTime from the render goroutine while runners mutate them unsynchronized. Warning rendering now exposes ask.Error via statusDescription, giving readers a new path to observe a torn interface value. Per-task mutex or snapshot under �rrorMutex. |
| 23 | ||
| oot.go.tmpl namespace template injection | LeafNamespace/Description are user-supplied values dropped into Go string literals via ext/template with no escaping. Low impact (self-scaffold), but a namespace containing ", , or newline produces broken/attacker-controlled Go. Validate input format. | |
| 24 | init.go writeFailedCommandOutput | Subprocess CombinedOutput() may contain ANSI sequences; printing verbatim ignores parent's NO_COLOR. Strip ANSI or set NO_COLOR=1 in the child env. |
| 25 | init.go (confirm prompt default flip from "no" → "yes") | Pressing Enter now confirms creation. Reasonable for happy path but combined with "directory exists → continue to create/update files" can silently overwrite user files. Consider warning if target dir is non-empty. |
| 26 | init.go:~209–213 | Manual pluralization + em-dash literal in code is unusual for this codebase (rest uses ASCII hyphens). Minor. |
| 27 | init_test.go imports | Single import group mixes external ( estify) and internal (github.com/azure/azure-dev/...). Split per AGENTS.md import order. |
| 28 | �uild.go:~58 | Error string starts with capital "Extension contains validation failures..."; parallel init.go:~201 uses lowercase. Pick one. |
| 29 | ExtensionTemplate struct (init.go) | Lacks GoDoc on the type itself (only the new LeafNamespace field has it). |
| 30 | New tests don't call .Parallel(); sub-test with empty namespace produces blank .Run name. |
Multi-reviewer consensus: Findings #1, #2, #3, #4, #8 were each flagged independently by 2+ focus-area reviewers, giving them high confidence.
Not flagged anywhere: No command-injection issues, no permissive file mode regressions, no performance concerns, copyright headers all intact, no interface{} regressions, no Go 1.26 modernization misses.
wbreza
reviewed
May 18, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-review @ bd94cbb
Thanks for the iteration. The "Address comments" commit is a well-reasoned, narrowly-scoped fix:
✅ What the delta does well
- Promotes
namespace-missing-with-custom-commandsfrom warning → fatal error- Inline comment clearly explains why it's fatal (
bindExtensionuses the last.-segment as the cobra command name → empty namespace silently installs an unreachable command)- Inline comment explains why
providers-missing stays a warning (init wizard doesn't yet prompt for providers — promoting would block every service-target-provider scaffold)- Test case renamed and expectation moved from
wantWarnings→wantErrorsconsistently
🆕 New findings introduced by this delta
| Priority | File:line | Finding |
|---|---|---|
| P2 | build.go validateExtensionMetadata GoDoc (function-level comment) |
The promotion of namespace-missing from warning → error makes the existing GoDoc more inaccurate. The function now has two reasons to return errors (structural Missing required field: X and capability-conditional fatal misuse), but the doc still frames errors as the structural-only category. The new inline comments compensate at the call site but the public-facing doc is now more misleading than before. |
| P2 | build.go validateExtensionMetadata (new error path itself) |
The warning→error promotion is a breaking validation change for hand-edited extension.yaml files that previously built successfully. The inline comment acknowledges this ("only triggers on hand-edited files") — a reasonable risk assessment, but exactly the kind of change that needs a CHANGELOG entry (see open #5 below). |
| P3 | init_test.go:102–124 |
Renamed test still pins the exact long error string verbatim — same brittleness flagged previously. Worth refactoring to assert.Contains on a stable key prefix alongside this rename, rather than as a separate cleanup. |
📋 Open from prior review (not addressed in this commit)
Still outstanding from the previous review — flagging here so they don't get lost:
P1 (worth fixing before merge):
- #1
cmd/tool.gobackward-compat regression —azd tool install/uninstallsilently exits 0 on failure because(ux.Warning, err)returns are now dropped byshouldCollectTaskError. (Highest-impact finding — affects an unrelated command.) - #2 Multi-line
validationWarningsMessagecorrupts the live TaskList canvas when rendered inline as(%s)with embedded\n -bullets. - #3
validateExtensionActionruns aftercreateExtensionDirectoryAction— schema validation should precede the filesystem mutation. - #4
writeCollectedWarningsis skipped on the early-return path when a later task (build/package/publish) fails — warnings dropped exactly when users need them.defer writeCollectedWarnings(buildWarnings)fixes it. - #5 No
CHANGELOG.mdentry incli/azd/extensions/microsoft.azd.extensions/for a user-visible behavior change (now compounded by the warning→error promotion in this commit). - #6 The literal visual fix (
WithErrorFormat→WithWarningFormat, red → yellow) is untested — neitherTestTaskList_Render_warningnorTestTaskList_Run_warningContinuesasserts the format used.
P2 (still relevant):
- #7
TaskState.Warningconstant lacks GoDoc explaining the new(Warning, err)display-only semantics. - #8
azd x buildexiting 0 on validation warnings is undocumented; consider--strict/--warnings-as-errorsfor CI. - #9
root.go.tmplName:field changed from full to leaf namespace — breaking for downstream extensions that referenced their own name. - #10
--no-promptpath still doesn't validate namespace shape (e.g."a..b", leading/trailing dot) — only thecustom-commands+ empty case is now caught. - #11 Deleted "Local extension source already exists" post-run notice is a UX regression.
- #17
validateExtensionMetadataGoDoc — see new P2 above; the prior staleness is now worse.
Full prior review with all P3 items: #8197 (review)
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
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address self-review nits on top of the prior commit: - Replace the duplicated success/failure flushes in runInitAction with a single named-return defer so warnings are always printed and subprocess output is surfaced only on failure (avoids dumping the full build transcript on success). - Replace the duplicated flush in runBuildAction with a defer for the same reason; restores the simple 'return taskList.Run()' shape. - Reorder runInitAction tasks so metadata validation runs before any filesystem side-effects, including the local extension source creation. - Drop the redundant '(!) Warning' prefix from the post-run warnings header; the TaskList row already shows that marker. - Add a boundary test asserting exactly maxExtensionTags parses successfully. - Wrap the long assert.True lines in TestValidateExtensionMetadata to satisfy the 125-char lll lint cap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…publish/install step fails
Contributor
Thanks for the thorough review - addressed the following: Review feedback
Other changes
|
hemarina
reviewed
May 19, 2026
Contributor
hemarina
left a comment
hemarina
left a comment
There was a problem hiding this comment.
Re-review @ 9cc6d11
Follow-up after the prior review on bd94cbb. The 5 new commits (25e770df → 9cc6d11) resolve every P1 item and almost all P2/P3 items from that review — net result is a substantially cleaner change. Calling out two small net-new findings I caught in the runSubprocess/subprocessErrorTail helpers and a couple of items that remain open from the prior review. Not posting blockers; the PR is in good shape.
✅ Resolved from prior review
- P1 #1
cmd/tool.go(Warning, err)regression — now returnsux.Error, covered byTestRunToolOperationUnsuccessfulResultReturnsError - P1 #2 Multi-line warning corrupting the canvas — inline message is now a single-line summary; details flushed after via
writeCollectedWarnings - P1 #3 Validation running after the filesystem mutation —
validateExtensionActionis now the first task - P1 #4 Collected warnings dropped on early-return —
defer writeCollectedWarnings(os.Stdout, buildWarnings)in bothbuild.goandinit.go - P1 #5 Missing CHANGELOG entry — new
0.11.0section - P1 #6 Color-format fix untested —
assert.Contains(output, outputpkg.WithWarningFormat("(partial failure)"))+NotContains(WithErrorFormat(...)) - P2 #7
TaskStateconstants — GoDoc added - P2 #8
azd x buildsilent exit 0 — documented in the newLong:help text (no--strictflag, but the behavior is now discoverable) - P2 #10
--no-promptnamespace shape —validateExtensionNamespaceenforces^[a-z][a-z0-9]*(\.[a-z][a-z0-9]*)*$in both interactive and scripted paths - P2 #16 Tag sanitization —
parseTagsenforcesmaxExtensionTags=10,maxExtensionTagLength=64, rejects control characters - P2 #17
validateExtensionMetadataGoDoc updated to reflect the warning→error promotion - P2 #18 Tests no longer pin exact long strings —
assert.Len+slicesContainSubstringon stable prefixes - P2 #19
shouldCollectTaskError— direct table test added - P2 #20
writeCollectedWarningsnow takesio.Writer;writeFailedCommandOutputreplaced bysubprocessErrorTail - P3 #23 Template injection —
goString = strconv.Quotetemplate helper +TestTemplateGoStringQuotesDescription - P3 #24 ANSI in subprocess output — stripped via
ansiEscapeRegex - P3 #25 Overwrite warning —
isDirNonEmpty+ amended confirm prompt withHelpMessage - P3 #28/29 Capitalization +
ExtensionTemplateGoDoc
🆕 New nits (P3)
| # | File:line | Finding |
|---|---|---|
| 1 | init.go:312-317 (runSubprocess) |
Title duplicated in the wrapped error. common.NewDetailedError(description, fmt.Errorf("%s: %w%s", description, err, ...)) puts description in both the title and the inner message prefix. The live TaskList row uses Description() only so the canvas is fine, but when the error propagates up through taskList.Run() and the CLI error middleware prints it, the final output renders as Build failed\n\nDetails:\nBuild failed: exit status 1: <tail> — "Build failed" appears twice. Drop description from the inner Errorf. |
| 2 | init.go:905-908 (subprocessErrorTail) |
Bare ERROR:/Error: line yields a trailing ": " artifact. Input "ERROR:\n" produces ": " because TrimSpace(TrimPrefix(...)) returns "" but the function still prepends ": ". Wrapped error becomes "Build failed: exit status 1: ". azd subcommands today always emit "ERROR: <message>" so this is defensive, but the fix is one line — if the post-trim string is empty, fall through to fallback instead of returning. Add a test row {name: "bare ERROR", in: "ERROR:\n", want: ""} to TestSubprocessErrorTail. |
📋 Still open from prior review
- P2 #11 Removed "Local extension source already exists" notice — users now see
(-) SkippedagainstCreate local azd extension sourcewith no follow-up explanation. The deferred warnings flush only handles validation warnings, not the registry-skipped case. Minor UX regression; either restore the post-run notice or move it into aHelpMessage-style hint on the task title. - P2 #14 When the spawned subprocess succeeds but emits warnings to stdout, those are still discarded — only the parent''s in-memory validation feeds
writeCollectedWarnings. Documented as PATH-dependent in the newLong:help, so this is acceptable, but worth a one-line note in the help text that nested-build warnings from a different azd version may not surface. - P3 #12 Warning wording still drifts slightly across sites:
(!) Warning(TaskList),Validation warnings:(deferred flush),N validation warning(s); see details below(inline summary). Lower priority than before but not fully harmonized. - P3 #22 Race on
Task.State/Task.Errorreads from the render goroutine — unaddressed (pre-existing).
Other observations (P3 / informational)
runSubprocess''s/* #nosec G204 */waiver covers all four call sites, but three of them pass only hardcoded literals ("x build --skip-install", etc.); only the install action passes user-controlledextensionMetadata.Id. Moving the comment to the install closure would be more precise.runInitActionis now(err error)named return, but inner scopes reuseerrvia:=(e.g. line 220nonEmpty, err := isDirNonEmpty(...)). Every early return is explicit so it works today, but named-return + heavy:=reuse is a known foot-gun. Either name it something distinctive (result error) or skip the named return and passbuildWarningsto a small helper that does the flush.promptExtensionNamespaceloops indefinitely untilvalidateExtensionNamespacepasses. Relies onPrompt()propagating Ctrl+C as an error, which it does today. Worth a one-line comment documenting that assumption.ansiEscapeRegexonly matches CSI sequences; OSC hyperlinks (\x1b]8;;…\x07) would leak through. Low impact —fatih/coloronly emits CSI today.CHANGELOG.mdis dated2026-05-19; verify against the actual release date.
Verdict
Looks good to me. The two runSubprocess nits are the only items I''d actually want to see fixed before merge, and even those are P3-level. Recommend comment-only; happy to re-review once the nits land if you want.
wbreza
reviewed
May 19, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-review @ 9cc6d11
Excellent iteration — 11 of 12 prior findings addressed across 4 well-scoped commits. The fixes are clean and well-tested. A couple of small things in the new code below.
✅ Prior findings — resolved
| Prior | Resolution |
|---|---|
| P1 #1 cmd/tool.go backward-compat (silent exit-0) | Warning → Error + new TestRunToolOperationUnsuccessfulResultReturnsError |
| P1 #2 multi-line warning corrupts TaskList canvas | Replaced �alidationWarningsMessage with single-line �alidationWarningSummary |
| P1 #3 validation runs after dir creation | alidateExtensionAction moved first in task list |
| P1 #4 dropped warnings on early-return | defer writeCollectedWarnings(...) in both init.go and �uild.go |
| P1 #5 missing CHANGELOG | 0.11.0 entry added with PR references |
| P1 #6 visual fix untested | New TestTaskList_Render_warning asserts WithWarningFormat (and not WithErrorFormat); also TestShouldCollectTaskError |
| P2 #7 TaskState no GoDoc | All 6 constants now documented |
| P2 #8 exit-0 on warnings undocumented | Added to Long help text |
| P2 #9 | |
| oot.go.tmpl template injection | {{goString .Metadata.Description}} via strconv.Quote ✨ — clean fix |
| P2 #10 no namespace shape validation in --no-prompt | Regex + interactive retry loop |
| P2 #17 stale �alidateExtensionMetadata GoDoc | Updated |
Only outstanding from prior review: P2 #11 (deleted "Local extension source already exists" notice).
🆕 New findings (in the delta)
🟡 P2
| # | File:line | Finding |
|---|---|---|
| 1 | init.go:57 namespace regex | The new regex ^[a-z][a-z0-9](.[a-z][a-z0-9])*$ rejects hyphens, but the existing first-party extension �zure.coding-agent ships |
| amespace: coding-agent (see cli/azd/extensions/azure.coding-agent/extension.yaml:8 and cli/azd/extensions/registry.json:1148). The new validator would refuse to scaffold an analog of it. cli/azd/schemas/v1.0/extension.schema.json has no | ||
| amespace pattern, so this is stricter than the canonical schema. Suggest: ^[a-z][a-z0-9-](.[a-z][a-z0-9-])*$ (and a test case for hyphenated segments). | ||
| 2 | init.go:885 �nsiEscapeRegex | \x1b[[0-9;]*[A-Za-z] only strips CSI sequences. The child �zd x build subprocess emits OSC 8 hyperlinks via output.WithHyperlink (�uild.go:115 "Output Path" line) using the form \x1b]8;;\x07\x1b]8;;\x07. These pass through and end up as garbled bytes inside the wrapped error returned by |
| unSubprocess. Either extend the regex (?:\x1b[[0-9;][A-Za-z])|(?:\x1b][^\x07\x1b](?:\x07|\x1b\)) or use a maintained stripper like github.com/acarl005/stripansi. Worth a TestSubprocessErrorTail case with an OSC hyperlink. |
🟢 P3
| # | File:line | Finding |
|---|---|---|
| 3 | init.go:451, 686 | Tag limits (10 / 64 chars) are interactive-only; collectExtensionMetadataFromFlags hardcodes ags := []string{} so --no-prompt has no way to provide tags. Add a --tags flag (with same validation) or document the gap. |
| 4 | init.go:898–916 subprocessErrorTail | If subprocess prints a Go compiler diagnostic without an ERROR: prefix, the last non-empty line wins — often "exit status 1" from the wrapper, which is useless. Consider preferring the first line matching ile:line:col: (or first non-empty) as a tie-breaker. |
| 5 | init.go:744 isDirNonEmpty | A freshly git init'd folder triggers the chatty "may overwrite existing files" prompt because .git/ counts as non-empty. Non-destructive thanks to the default- |
| ew(false) fix, just noisy. Consider filtering dotfiles. | ||
| 6 | cmd/tool_test.go | New TestRunToolOperationUnsuccessfulResultReturnsError only exercises the failure path. No happy-path companion for |
| unToolOperation. Low risk; integration paths cover it. |
Deliberately checked / clean
- Overwrite-prompt default is correctly
ew(false) ✓ - �uildWarnings defer closures capture by reference; only �ppend-mutated; no race; askList.Run() is synchronous ✓
- strconv.Quote handles multi-line/quotes/control chars in description; other template fields (Name, Use, LeafNamespace) are regex-constrained so don't need quoting ✓
- Warning → Error in ool.go is the correct semantic given the shouldCollectTaskError filter ✓
- 0.10.0 → 0.11.0 version bump is appropriate for tightened validation + UX changes ✓
Nice work tightening this up.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Thanks @hemarina, @wbreza - fixed the duplicated subprocess error title, added the empty I also added improved extension-pack handling: dependency-only packs now validate without
|
wbreza
reviewed
May 19, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-review @ 2587eaa
Nice — closed 3 outstanding items (P2-1 hyphens, P2-2 OSC stripping, and the old "Local extension source already exists" notice) and added the azd x extension pack mode. A couple of things in the new pack flow below.
✅ Closed by this commit
| Prior | Resolution |
|---|---|
| P2-1 namespace regex hyphens | Regex now ^[a-z][a-z0-9-]*(\.[a-z][a-z0-9-]*)*$; tests for coding-agent / azure.coding-agent added |
| P2-2 ANSI OSC 8 stripping | Extended regex handles CSI + OSC 8; new strips OSC hyperlinks test |
| Old P2 #11 deleted "Local extension source already exists" notice | Restored — tracked in task closure, printed after taskList.Run() |
⏭️ Still outstanding from prior reviews
- P3-3 Tag limits (10 / 64 chars) still enforced interactively only;
--no-prompthas no--tagsflag. - P3-4
subprocessErrorTail— partially improved (emptyERROR:lines skipped, duplicated description prefix removed) but the generic fallback can still surface bare"exit status 1". - P3-5 Chatty "may overwrite" prompt for
.git/.vscode-only directories. - P3-6
tool_test.gohappy-path companion forrunToolOperation.
🆕 New findings (introduced by this commit)
🟡 P2
| # | File:line | Finding |
|---|---|---|
| 1 | build.go:350–356 isExtensionPack |
Pack mode is detected by heuristic (len(Dependencies)>0 && Capabilities=="" && Namespace=="" && Language=="" && EntryPoint=="") with no explicit kind:/type: discriminator. An author who momentarily clears capabilities flips silently into pack mode; one who later fills language flips back and gets confusing "Missing required field: capabilities" instead of pack-mode validation. Real footgun for azd x init re-runs on partial state. Suggest: add an explicit kind: pack (or type: pack) field to extension.yaml, or at minimum log the detected mode under --debug and add a doc comment on isExtensionPack calling out the inference. |
| 2 | publish.go:~299–308 |
New hard-error path for non-pack publishes with len(assets)==0. Previously a publish with no matching artifacts proceeded and wrote a version with an empty artifact map; now it returns "Artifacts not found / no artifacts found for this extension version". Probably intended (failing loud is better) but is a real behavior change for executable extensions invoked before artifacts exist (e.g. CI ordering issues). Worth a unit test + CHANGELOG callout. |
🟢 P3
| # | File:line | Finding |
|---|---|---|
| 3 | pack.go:198–201 |
Pack-mode task message "Extension packs are published directly to the registry" is misleading inside azd x pack (pack doesn't publish). Reword: "Extension packs contain no artifacts; nothing to package". |
| 4 | pack.go:40 header subtitle |
"Packages the azd extension project into distributable artifacts" — pack mode has no artifacts. Either generalize or branch the subtitle for pack mode. |
| 5 | publish.go:~120 |
Error strings reference --repo and --artifacts. Confirm the cobra flag short form is actually --repo (vs. --repository); align if mismatched. |
| 6 | init.go:316 |
Removing the duplicated description prefix from the wrapped error (since NewDetailedError(description, ...) already shows it) is correct, but silent — worth a sanity check that no caller logs the bare wrapped error outside the DetailedError formatter. |
| 7 | init_test.go |
New TestValidatePublishOptionsExtensionPack exercises the validator, but isExtensionPack itself has no direct table test for the boundary cases (deps + namespace empty + language populated, deps + capabilities non-empty, etc.). Small table-driven test would lock the contract. |
Verified clean
- Existing executable-extension validation untouched (
namespacerequired → excluded from pack detection) TestAddOrUpdateExtensionExtensionPackasserts emptyArtifacts/Capabilities, preservedDependencies✓pack/build/publishall consistently skip artifact tasks viaux.Skippedwith progress message — uniform UX- No new top-level cobra subcommands; no help-text gaps in the new pack-mode paths
Looking good overall.
wbreza
reviewed
May 19, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-review @ 4f5e069
Excellent wrap-up commit — closes 8 of 10 outstanding findings cleanly.
✅ Closed
| Prior | Resolution |
|---|---|
| P2-2 publish hard-error needs test + CHANGELOG | New TestValidatePublishAssets (3 branches) + dedicated CHANGELOG bullet; logic factored into �alidatePublishAssets() helper |
| P3-3 tag limits in --no-prompt | New --tags StringSlice flag wired through parseTags; TestCollectExtensionMetadataFromFlagsTags + …InvalidTags confirm normalization and control-char rejection |
| P3-6 ool_test.go happy-path | New TestRunToolOperationSuccessfulResultReturnsNoError covers success + InstalledVersion propagation |
| P3 pack.go:198 misleading "published directly" message | Reworded to "Extension packs contain no artifacts; nothing to package" |
| P3 pack.go:40 header subtitle | Generalized to "Prepares the azd extension project for publishing" |
| P3 missing isExtensionPack table test | New TestIsExtensionPack with 6 boundary cases — locks the contract |
🟡 Partially addressed
P2-1 explicit pack-mode discriminator — Added GoDoc explaining the heuristic and a log.Printf("debug: …") when detected. This matches the "at minimum" half of the prior suggestion (no explicit kind:/ ype: field), but the log call needs fixing — see new P3-1 below.
⏭️ Still untouched (acceptable as follow-ups)
- P3-4 subprocessErrorTail "exit status 1" fallback
- P3-5 chatty prompt for .git-only dirs
- P3 --repo vs --repository — verified --repo is correct (matches �zd x release --repo {owner}/{repo}); non-issue
🆕 New findings
🟢 P3
| # | File:line | Finding |
|---|---|---|
| 1 | �uild.go:358–362 (isExtensionPack debug log) | Uses stdlib log.Printf from the global logger — emits unconditionally on every �zd x build against a pack manifest, polluting stderr regardless of --debug. The prior suggestion was "log the detected mode under --debug"; this needs the project's debug/log facility (or slog at Debug level), not the unconditional log package. The �zure.ai.agents extension and similar codepaths silence stdlib log unless debug mode is enabled (see setupDebugLogging) — microsoft.azd.extensions should follow the same pattern. |
| 2 | publish.go:299–305 | The new �lse if state == ux.Skipped branch reads slightly awkwardly (helper returns (Skipped, nil), then the printf adds a message). Cosmetic. |
| 3 | init_test.go TestIsExtensionPack | "Executable extension" cases rely on zero-value want: false rather than explicit — a future field reorder could mask a regression. Minor. |
Verdict
The one item worth tightening before merge is the log.Printf debug emission — it will spam stderr on every pack build because there's no --debug gate. Everything else is in good shape.
tg-msft
approved these changes
May 19, 2026
This was referenced May 19, 2026
Contributor
This isn't the case anymore after the |
hemarina
approved these changes
May 20, 2026
Contributor
hemarina
left a comment
hemarina
left a comment
There was a problem hiding this comment.
Nice polish on the azd x flows overall — the dependency-only pack handling and clearer subprocess error surfacing are great improvements! Just one small nit inline.
vhvb1989
approved these changes
May 20, 2026
Copilot AI
added a commit
that referenced
this pull request
May 21, 2026
…8197) * Treat extension metadata warnings as non-fatal and polish `azd x init` `azd x build` previously treated validation warnings (e.g. missing `providers` when `service-target-provider` is declared) as failures. That aborted `azd x init`, and the build subprocess output was discarded so the user never saw why. Warning handling: - `ux.TaskList`: a task returning `(Warning, err)` no longer cancels subsequent tasks, and warning detail renders with warning styling instead of error styling. - Extract `validateExtensionMetadata` as a shared helper. `azd x init` validates the in-memory schema directly instead of shelling out to `azd x build` and parsing its rendered output. - Show a short summary in the task list and the full bulleted detail once below it. - Drop the redundant "Local extension source already exists." message. - Rephrase warnings to name the YAML field, the capability that needs it, and what to set. Wizard UX (resolves #8207): - Confirmation prompt defaults to "yes". - Tags prompt is optional; also fixes a pre-existing bug where parsed tags were discarded. - Translate dotted namespaces (`ai.project`) into the actual command path (`azd ai project <command>`) in the generated `usage` field and follow-up hints, matching how `bindExtension` registers them. - Generated `extension.yaml` includes a `yaml-language-server` header pointing at the published schema URL. Scaffolded extension root command: - The Go template set `Use` to `"azd <namespace> <command> [options]"`, so cobra parsed `"azd"` as the root command name and rendered `Usage: azd [command]`. Use the leaf segment of the namespace for both `Name` and `Use`, matching the convention in `azure.ai.agents`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address comments * Address extension validation review feedback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Defer warning/output flushes and tighten init task ordering Address self-review nits on top of the prior commit: - Replace the duplicated success/failure flushes in runInitAction with a single named-return defer so warnings are always printed and subprocess output is surfaced only on failure (avoids dumping the full build transcript on success). - Replace the duplicated flush in runBuildAction with a defer for the same reason; restores the simple 'return taskList.Run()' shape. - Reorder runInitAction tasks so metadata validation runs before any filesystem side-effects, including the local extension source creation. - Drop the redundant '(!) Warning' prefix from the post-run warnings header; the TaskList row already shows that marker. - Add a boundary test asserting exactly maxExtensionTags parses successfully. - Wrap the long assert.True lines in TestValidateExtensionMetadata to satisfy the 125-char lll lint cap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Surface child subprocess failures when an `azd x init` build/package/publish/install step fails * Address additional feedback * Improve azd x extension pack and init flows Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address comments, update CHANGELOG * Address feedback and remove "Local extension source already exists." message --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: therealjohn <1501196+therealjohn@users.noreply.github.com>
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.
Fixes #8207
Fixes #8196
This PR improves
azd xextension development flows by making metadata warnings non-fatal, surfacing clearer init subprocess errors, supporting dependency-only extension packs, and polishing the init wizard.Warning and error handling
azd x buildpreviously treated validation warnings, such as missingprovidersforservice-target-provider, as command failures. This causedazd x initto abort even when the generated extension could still build. Build now reports recommended metadata as warnings while preserving errors for missing required fields or unusable extension metadata.azd x initnow validates the in-memory schema before filesystem side effects, shares the same validation wording as build, and prints detailed warning bullets after the live task list completes. Init child steps also capture subprocess output, strip ANSI/OSC noise, and surface the useful error line inDetails:instead of showing onlyexit status 1or duplicating task-list output.Improved extension pack support
Dependency-only extension packs like
azd.internal.packcan now validate, build, and publish without fake executable metadata.azd x buildandazd x packskip artifact work with clear messages, whileazd x publishwrites dependency-only metadata directly toregistry.jsonand rejects artifact-specific options such as--repoand--artifactsfor packs.Init wizard UX
The init wizard now validates namespaces consistently in interactive and
--no-promptmodes, including existing hyphenated namespace patterns, and translates dotted namespaces into the actual nested command path used in generated usage text and follow-up hints. It also preserves optional tags, uses safer overwrite confirmation for non-empty directories, adds anextension.yamlschema modeline for editor validation, and generates root-command scaffolding that matches nested namespace behavior.