Skip to content

Commit 0d4ae1a

Browse files
Antriksh JainCopilot
andcommitted
[azure.ai.agents] nextstep: surface every fix-up category after init/doctor, broaden placeholder regex
Two correctness fixes from the 3/3-consensus review of commit 4.6 (`2194327e8`). GPT-5.5 surfaced both initially; cross-pollination to Opus 4.7 (xhigh) and Sonnet 4.6 confirmed both findings 3/3 — Sonnet flipped its initial APPROVE after re-reading the renderer. G1 (renderer): when ResolveAfterInit emits multiple fix-up categories in a single state (e.g. the toolbox sample: 1 unresolved {{NAME}} placeholder + 1 missing manual env var + the trailing `azd deploy` reminder), PrintNext silently truncated the env-set line. PrintNext caps total rendered lines at maxRendered=2 with one slot reserved for the Trailing entry — so the budget for primaries is 1, and any secondary category gets dropped on the floor. Pre-4.6 the toolbox state was [env-set, deploy/trailing] → both rendered. Post-4.6 the state became [placeholder, env-set, deploy/trailing] → only [placeholder, deploy] rendered. Net effect: the user saw the placeholder hint and the deploy reminder, but had no idea they also needed to `azd env set TOOLBOX_WEB_SEARCH_TOOLS_MCP_ENDPOINT <value>`. The 2-line cap exists for mid-flow resolvers (Run/Invoke/Show, all of which produce ≤2 suggestions naturally — the cap prevents drowning out subsequent command output). It does NOT apply to ResolveAfterInit or doctor, both of which fire at the very END of their command with no further output to drown. Fix: parameterize renderBlock with a limit argument (0 = uncapped) and add a sibling `PrintAllNext` that calls renderBlock(suggestions, 0). Switch init.go and doctor_format.go to PrintAllNext. PrintNext semantics (and all its existing tests) preserved byte-for-byte for the mid-flow callers (invoke.go, run.go, show.go). Worst-case bound for the uncapped renderer is 7 lines (3 placeholders + 3 manual vars + 1 trailing), already capped at the resolver level by maxFixupLines=3 per category in resolver.go. G2 (regex): the placeholder-detection regex in state.go was `\{\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}\}` — accepted only Go-style identifier names. But parameters.go:237-244 substitutes the raw YAML parameter name into the template via `fmt.Sprintf("{{%s}}", paramName)` WITHOUT any shape validation, and yaml.go:425-431 assigns the raw YAML key to Property.Name with no validation either. So a manifest can legally use names like `toolbox-endpoint`, `my.component.id`, or even quoted YAML keys containing whitespace — and 4.6's placeholder-detection silently missed all of them. Broadened to `\{\{\s*([^\s{}][^{}]*?)\s*\}\}` (first char must be non-whitespace non-brace; remaining is lazy any-non-brace). Allows hyphens, dots, and internal whitespace in paramName. Empty braces `{{}}` and whitespace-only braces `{{ }}` correctly do NOT match (because the first capture-group char must be non-whitespace non-brace). Picked Sonnet's regex over Opus's `[^\s{}]+?` because Sonnet's is slightly more permissive — covers the rare-but-valid quoted-YAML-key-with- whitespace case. Trade-off: tiny false-positive risk (the surfaced suggestion is benign — just an "edit agent.yaml" line — and false negatives are the bug we're fixing, so we err toward detection). The toolbox-sample regression is locked at two layers: - format_test.go: a hand-crafted Suggestions slice matching the exact toolbox state ("G1 regression repro" sub-case). - resolver_test.go: a new end-to-end test that builds the State, runs it through ResolveAfterInit, renders with PrintAllNext, and asserts all three lines (placeholder, env-set, deploy) appear. - state_test.go: extended the placeholder-extraction table with hyphenated, dotted, empty-braces, and whitespace-only-braces cases. - format_test.go: new TestPrintAllNext / TestPrintAllNext_Propagates WriteError / TestPrintAllNext_EmptyInputSkipsWrite covering empty input, single-suggestion, the G1 toolbox shape, worst-case 7-line uncapped render, and the trailing-last invariant under uncapped mode. Files: 7 changed. - format.go: renderBlock(suggestions) → renderBlock(suggestions, limit). New PrintAllNext(w, suggestions). PrintNext doc-comment points at PrintAllNext for multi-category flows. - state.go: placeholderPattern broadened. Doc comment cites the YAML key examples (toolbox-endpoint, my.param, "my key") and links to parameters.go's substitution code. - init.go:1608, doctor_format.go:116: PrintNext → PrintAllNext. doctor_format.go's surrounding doc-comment updated to mention PrintAllNext rationale. - format_test.go, state_test.go, resolver_test.go: tests as above. Pre-flight: gofmt clean, vet clean, build clean, full extension test suite green (cmd 17.2s, nextstep 5.8s, doctor 5.8s, agent_api 9.1s, agent_yaml 2.1s, etc.), golangci-lint 0 issues on ./internal/cmd/nextstep/... + ./internal/cmd/..., cspell 0 issues on the 4 production files. The existing TestPrintNext suite (including "more than two suggestions are truncated by priority" and "trailing suggestion survives truncation when primaries fill the block") still passes — PrintNext is unchanged. Refs PR Azure#8057 (azd ai agent context-aware next-step guidance). Consensus tally: G1 = GPT-5.5 (MEDIUM) + Sonnet (High, flipped) + Opus (Medium, acknowledged miss) = 3/3. G2 = GPT-5.5 (Low) + Sonnet (Low) + Opus (Low) = 3/3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent da75a17 commit 0d4ae1a

7 files changed

Lines changed: 236 additions & 12 deletions

File tree

cli/azd/extensions/azure.ai.agents/internal/cmd/doctor_format.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,12 @@ func printDoctorReportJSON(w io.Writer, report doctor.Report) error {
8686
// Summary line is appended after the per-check block.
8787
//
8888
// The trailing Next: block is rendered only when showNext is true.
89-
// nextstep.PrintNext owns the leading blank-line separator (see
89+
// nextstep.PrintAllNext owns the leading blank-line separator (see
9090
// nextstep/format.go renderBlock), so this function does not pre-emit
91-
// one.
91+
// one. PrintAllNext (not PrintNext) is used because doctor surfaces
92+
// the same multi-category fix-up list as `azd ai agent init` — every
93+
// line is a required action, and silently dropping any of them would
94+
// hide work the user still has to do.
9295
func printDoctorReportText(
9396
w io.Writer,
9497
report doctor.Report,
@@ -113,7 +116,7 @@ func printDoctorReportText(
113116
}
114117

115118
if showNext {
116-
if err := nextstep.PrintNext(w, trailing); err != nil {
119+
if err := nextstep.PrintAllNext(w, trailing); err != nil {
117120
return err
118121
}
119122
}

cli/azd/extensions/azure.ai.agents/internal/cmd/init.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2098,7 +2098,7 @@ func (a *InitAction) addToProject(ctx context.Context, targetDir string, agentMa
20982098
// trailing line. State-assembly errors are intentionally ignored: the
20992099
// resolver degrades gracefully on partial state per the design spec.
21002100
state, _ := nextstep.AssembleState(ctx, a.azdClient)
2101-
_ = nextstep.PrintNext(os.Stdout, nextstep.ResolveAfterInit(state))
2101+
_ = nextstep.PrintAllNext(os.Stdout, nextstep.ResolveAfterInit(state))
21022102
return nil
21032103
}
21042104

cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/format.go

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,34 @@ const (
3131
// PrintNext does not inspect TTY state or output-format flags — those
3232
// decisions live at the call site so the same renderer can serve both
3333
// interactive stdout writes and string capture for tests / JSON envelopes.
34+
//
35+
// Use PrintAllNext when the resolver produces multiple REQUIRED follow-up
36+
// actions (init / doctor fix-ups) where silently dropping any of them
37+
// would mislead the user.
3438
func PrintNext(w io.Writer, suggestions []Suggestion) error {
35-
block := renderBlock(suggestions)
39+
block := renderBlock(suggestions, maxRendered)
40+
if block == "" {
41+
return nil
42+
}
43+
_, err := io.WriteString(w, block)
44+
return err
45+
}
46+
47+
// PrintAllNext writes a "Next:" guidance block to w like PrintNext but
48+
// renders every suggestion (no two-line cap). Use this for flows where
49+
// the suggestions are all REQUIRED follow-up actions rather than
50+
// alternatives — the post-init flow can surface unresolved manifest
51+
// placeholders, missing `azd env set` keys, AND the trailing
52+
// `azd deploy` reminder simultaneously, and the user has to act on each
53+
// one. Dropping any of them silently leaves the user thinking they are
54+
// ready to deploy when they are not.
55+
//
56+
// Suggestions are still stable-sorted by Priority (ties preserve input
57+
// order), the Trailing entry is still rendered last, and framing
58+
// (leading blank line + trailing newline) matches PrintNext. Empty
59+
// input is a no-op.
60+
func PrintAllNext(w io.Writer, suggestions []Suggestion) error {
61+
block := renderBlock(suggestions, 0)
3662
if block == "" {
3763
return nil
3864
}
@@ -65,9 +91,11 @@ func FormatNextForNote(suggestions []Suggestion) string {
6591

6692
// renderBlock returns the formatted "Next:" block (with a leading blank
6793
// line and trailing newline) or an empty string when there is nothing to
68-
// render. The block is capped at maxRendered visible lines.
69-
func renderBlock(suggestions []Suggestion) string {
70-
body := renderRows(suggestions, maxRendered)
94+
// render. limit is forwarded to renderRows: a positive value caps the
95+
// block at that many visible lines (PrintNext default), while limit <= 0
96+
// renders every suggestion (PrintAllNext).
97+
func renderBlock(suggestions []Suggestion, limit int) string {
98+
body := renderRows(suggestions, limit)
7199
if body == "" {
72100
return ""
73101
}

cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/format_test.go

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,116 @@ func TestPrintNext_EmptyInputSkipsWrite(t *testing.T) {
144144
require.NoError(t, PrintNext(failingWriter{}, nil))
145145
}
146146

147+
func TestPrintAllNext(t *testing.T) {
148+
t.Parallel()
149+
150+
tests := []struct {
151+
name string
152+
suggestions []Suggestion
153+
want string
154+
}{
155+
{
156+
name: "empty input produces no output",
157+
suggestions: nil,
158+
want: "",
159+
},
160+
{
161+
name: "single suggestion renders identically to PrintNext",
162+
suggestions: []Suggestion{
163+
{Command: "azd provision", Description: "set up Foundry"},
164+
},
165+
want: "\nNext: azd provision -- set up Foundry\n",
166+
},
167+
{
168+
name: "G1 regression repro: placeholder + manual var + trailing deploy all render (no cap)",
169+
// This is the toolbox-sample state that motivated commit 2194327e8.
170+
// PrintNext (capped at 2 with trailing reservation) would render
171+
// only [placeholder, deploy] and drop the env-set line, leaving
172+
// the user thinking they only need to fix the placeholder before
173+
// deploying. PrintAllNext must surface all three.
174+
suggestions: []Suggestion{
175+
{
176+
Command: "edit agent.yaml: replace {{TOOLBOX_ENDPOINT}} with the actual value",
177+
Description: "agent.yaml has unresolved manifest placeholders",
178+
Priority: 5,
179+
},
180+
{
181+
Command: "azd env set TOOLBOX_WEB_SEARCH_TOOLS_MCP_ENDPOINT <value>",
182+
Description: "supply the agent.yaml variable",
183+
Priority: 6,
184+
},
185+
{
186+
Command: "azd deploy",
187+
Description: "when ready to deploy to Azure",
188+
Priority: 90,
189+
Trailing: true,
190+
},
191+
},
192+
want: "\n" +
193+
"Next: edit agent.yaml: replace {{TOOLBOX_ENDPOINT}} with the actual value -- agent.yaml has unresolved manifest placeholders\n" +
194+
" azd env set TOOLBOX_WEB_SEARCH_TOOLS_MCP_ENDPOINT <value> -- supply the agent.yaml variable\n" +
195+
" azd deploy -- when ready to deploy to Azure\n",
196+
},
197+
{
198+
name: "renders well beyond maxRendered (3 placeholders + 3 manual vars + trailing = 7 lines)",
199+
// Worst-case shape from ResolveAfterInit when both
200+
// maxFixupLines caps are saturated.
201+
suggestions: []Suggestion{
202+
{Command: "edit agent.yaml: replace {{A}} with the actual value", Description: "p1", Priority: 5},
203+
{Command: "edit agent.yaml: replace {{B}} with the actual value", Description: "p1", Priority: 6},
204+
{Command: "edit agent.yaml: replace {{C}} with the actual value", Description: "p1", Priority: 7},
205+
{Command: "azd env set FOO <value>", Description: "p2", Priority: 8},
206+
{Command: "azd env set BAR <value>", Description: "p2", Priority: 9},
207+
{Command: "azd env set BAZ <value>", Description: "p2", Priority: 10},
208+
{Command: "azd deploy", Description: "p3", Priority: 90, Trailing: true},
209+
},
210+
want: "\n" +
211+
"Next: edit agent.yaml: replace {{A}} with the actual value -- p1\n" +
212+
" edit agent.yaml: replace {{B}} with the actual value -- p1\n" +
213+
" edit agent.yaml: replace {{C}} with the actual value -- p1\n" +
214+
" azd env set FOO <value> -- p2\n" +
215+
" azd env set BAR <value> -- p2\n" +
216+
" azd env set BAZ <value> -- p2\n" +
217+
" azd deploy -- p3\n",
218+
},
219+
{
220+
name: "trailing entry still rendered last regardless of input order",
221+
suggestions: []Suggestion{
222+
{Command: "azd deploy", Description: "when ready", Priority: 90, Trailing: true},
223+
{Command: "first", Description: "f", Priority: 5},
224+
{Command: "second", Description: "s", Priority: 6},
225+
},
226+
want: "\n" +
227+
"Next: first -- f\n" +
228+
" second -- s\n" +
229+
" azd deploy -- when ready\n",
230+
},
231+
}
232+
233+
for _, tt := range tests {
234+
t.Run(tt.name, func(t *testing.T) {
235+
t.Parallel()
236+
237+
var buf bytes.Buffer
238+
require.NoError(t, PrintAllNext(&buf, tt.suggestions))
239+
assert.Equal(t, tt.want, buf.String())
240+
})
241+
}
242+
}
243+
244+
func TestPrintAllNext_PropagatesWriteError(t *testing.T) {
245+
t.Parallel()
246+
247+
err := PrintAllNext(failingWriter{}, []Suggestion{{Command: "x", Description: "y"}})
248+
require.ErrorIs(t, err, io.ErrUnexpectedEOF)
249+
}
250+
251+
func TestPrintAllNext_EmptyInputSkipsWrite(t *testing.T) {
252+
t.Parallel()
253+
254+
require.NoError(t, PrintAllNext(failingWriter{}, nil))
255+
}
256+
147257
func TestFormatNextForNote(t *testing.T) {
148258
t.Parallel()
149259

cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,35 @@ func TestResolveAfterInit_NilState(t *testing.T) {
107107
assert.Nil(t, ResolveAfterInit(nil))
108108
}
109109

110+
// TestResolveAfterInit_ToolboxReproRendersAllCategories locks the full
111+
// regression for the toolbox-sample bug end-to-end: the state contains
112+
// BOTH an unresolved manifest placeholder AND a missing manual env var,
113+
// and the rendered "Next:" block must surface both fix-up categories
114+
// plus the trailing `azd deploy` reminder. PrintNext would silently
115+
// drop one category here because of its 2-line cap; PrintAllNext must
116+
// not.
117+
func TestResolveAfterInit_ToolboxReproRendersAllCategories(t *testing.T) {
118+
t.Parallel()
119+
120+
state := &State{
121+
HasProjectEndpoint: true,
122+
UnresolvedPlaceholders: []string{"TOOLBOX_ENDPOINT"},
123+
MissingManualVars: []string{"TOOLBOX_WEB_SEARCH_TOOLS_MCP_ENDPOINT"},
124+
}
125+
126+
var buf strings.Builder
127+
require.NoError(t, PrintAllNext(&buf, ResolveAfterInit(state)))
128+
rendered := buf.String()
129+
130+
assert.Contains(t, rendered,
131+
"edit agent.yaml: replace {{TOOLBOX_ENDPOINT}} with the actual value",
132+
"placeholder fix-up missing")
133+
assert.Contains(t, rendered,
134+
"azd env set TOOLBOX_WEB_SEARCH_TOOLS_MCP_ENDPOINT <value>",
135+
"manual-var fix-up missing — this is the original toolbox-sample regression")
136+
assert.Contains(t, rendered, "azd deploy", "trailing deploy reminder missing")
137+
}
138+
110139
func TestResolveAfterInit_UnresolvedPlaceholders(t *testing.T) {
111140
t.Parallel()
112141

cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,18 @@ var envVarRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)(:-[^}]*)
6262
// environment_variables values are deploy-time landmines: the value will
6363
// land in the container literally as `{{NAME}}`, breaking the agent.
6464
//
65-
// Allows optional internal whitespace (`{{ NAME }}`) because parameters.go
66-
// substitutes both forms. Names follow the same convention as env vars
67-
// (leading letter or underscore, then alphanumeric or underscore).
68-
var placeholderPattern = regexp.MustCompile(`\{\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}\}`)
65+
// The capture group accepts any run of non-brace characters (allowing
66+
// internal whitespace as long as the name starts with a non-whitespace,
67+
// non-brace char) because parameters.go substitutes the raw manifest
68+
// parameter key without validating its shape (`strings.ReplaceAll` of
69+
// `{{<paramName>}}` and `{{ <paramName> }}`). A legitimate manifest
70+
// parameter named `toolbox-endpoint` (hyphen), `my.param` (dot), or
71+
// even `"my key"` (quoted YAML key with whitespace) would otherwise
72+
// slip past detection. Allows optional surrounding whitespace inside
73+
// the braces — matches both `{{NAME}}` and `{{ NAME }}` (the two
74+
// forms parameters.go knows how to substitute) plus more liberal
75+
// spacing for forgiving detection.
76+
var placeholderPattern = regexp.MustCompile(`\{\{\s*([^\s{}][^{}]*?)\s*\}\}`)
6977

7078
// Source is the read-only view of azd that AssembleState needs.
7179
//

cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,52 @@ environment_variables:
571571
wantRefs: []string{"TOOLBOX_WEB_SEARCH_TOOLS_MCP_ENDPOINT"},
572572
wantPlaceholders: []string{"TOOLBOX_ENDPOINT"},
573573
},
574+
{
575+
// Manifest parameter names are not constrained to env-var
576+
// identifier shape — parameters.go:injectParameterValues
577+
// substitutes the raw YAML key without validating it.
578+
// A surviving `{{toolbox-endpoint}}` (hyphen) must therefore
579+
// still be flagged or the user gets no Next: hint.
580+
name: "mustache placeholder with hyphen in name",
581+
manifest: `kind: hostedAgent
582+
environment_variables:
583+
- name: TOOLBOX_ENDPOINT
584+
value: '{{toolbox-endpoint}}'
585+
`,
586+
wantPlaceholders: []string{"toolbox-endpoint"},
587+
},
588+
{
589+
name: "mustache placeholder with dot in name",
590+
manifest: `kind: hostedAgent
591+
environment_variables:
592+
- name: COMPONENT
593+
value: '{{my.component.id}}'
594+
`,
595+
wantPlaceholders: []string{"my.component.id"},
596+
},
597+
{
598+
// Empty placeholder body must not be flagged — it cannot
599+
// correspond to a manifest parameter and is more likely
600+
// stray literal text.
601+
name: "empty mustache braces are ignored",
602+
manifest: `kind: hostedAgent
603+
environment_variables:
604+
- name: NOISE
605+
value: 'preamble {{}} suffix'
606+
`,
607+
wantPlaceholders: nil,
608+
},
609+
{
610+
// Whitespace-only placeholder body is similarly garbage —
611+
// must not be flagged.
612+
name: "whitespace-only mustache braces are ignored",
613+
manifest: `kind: hostedAgent
614+
environment_variables:
615+
- name: NOISE
616+
value: 'preamble {{ }} suffix'
617+
`,
618+
wantPlaceholders: nil,
619+
},
574620
}
575621

576622
for _, tt := range tests {

0 commit comments

Comments
 (0)