Pull request overview

This PR adds a dedicated Azure DevOps pipeline and supporting Python drivers/docs to run the Tier 2 live golden-path E2E for the azure.ai.agents azd extension (init → provision → deploy → invoke → down) against real Azure resources, outside of the GitHub PR gate.

Changes:

• Added an on-demand + weekly ADO pipeline (ext-azure-ai-agents-live) to run Tier 2 live E2E for both code and container deploy modes.
• Added a tmux-driven Python Tier 2 runner (test_tier2.py) and a full golden-path driver (test_full_e2e.py) adapted for ADO CI auth detection.
• Added documentation for running the live Tier 2 tests locally and in CI.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

One minor nit on the location dispatch case for consistency with the other tightened prompt matches.

Previous feedback (errors.AsType, t.Context(), yaml.Unmarshal for service name parsing, tightening prompt dispatch matches, logging default-case fall-throughs) all addressed in recent commits. Code quality is solid.

One observation: the PR description mentions a condition: always() AzureCLI@2 step as a "second safety net" for crash-path cleanup, but the pipeline YAML only has PublishPipelineArtifact@1 with condition: always(). The Go .Cleanup(r.teardown) approach is sufficient given the timeout hierarchy (job 150m > step 130m > go test 125m > per-mode 60m), so this isn't a gap in practice, but the description is slightly overstated. Consider updating the PR body to reflect the actual cleanup mechanism (t.Cleanup + LIFO ordering) or adding the explicit cleanup step if you want the belt-and-suspenders guarantee for cases where the Go process is killed before t.Cleanup runs.

No blocking issues. The event-driven prompt loop design, the timeout hierarchy, and the �ssertSafeTestDir guard are all well thought out.

Incremental commit fixes the subscription dispatch to match azd-core's actual picker message (Select subscription from prompt_service.go:507) rather than the extension's preamble text. Verified: activePrompt lowercases before dispatch, so the case-sensitive has() correctly matches.

Minor note for future awareness: select subscription (singular) is also a prefix/substring of select subscriptions to include in the filter (config_sub_filter.go:188). This won't cause false matches since that multi-subscription filter prompt doesn't appear in the agent init flow, but worth keeping in mind if future init steps add subscription filtering.

This adds a Tier 2 live golden-path test (init/provision/deploy/invoke/down) for the azure.ai.agents extension, driven through a real PTY with go-expect + vt10x, plus an on-demand/weekly Azure DevOps pipeline that runs it against the TME subscription. The shape is right: it is gated behind AZURE_AI_AGENTS_E2E_LIVE, kept out of the PR pipeline, runs the two deploy modes sequentially, and registers an azd down teardown with a condition: always() safety net so a crash does not leak billable resources. go.sum already carries the three new direct dependencies, so the extension module still builds, and the answer matcher is well-factored and covered on its own by assert_test.go.

One thing is worth tightening before a green run is trusted as signal: the invoke assertion matches a standalone "4" against the entire combined CLI output rather than the agent's actual response, so incidental azd chatter (an elapsed-seconds token, a count, a step number) could turn the check green while the agent is broken. That is the difference between a live test that verifies the agent and one that mostly verifies the pipeline ran, so I would scope it to the response payload.

The rest are non-blocking. The numerous file:line source citations will rot silently the first time those files are edited, and the pipeline hand-writes the extension manifest into config.json where it can drift from the real one. Nothing here runs in PR CI, so the blast radius is bounded to the manual or scheduled live run. None of it blocks merge; I would fix the invoke matcher scope before relying on a green run and treat the other two as follow-ups.

Confirmed RickWinter's invoke assertion concern (replied on his thread with details). responseHasExpectedAnswer runs on the full combined stdout+stderr stream, and patterns like "4 seconds" or "step 4:" pass the standalone-four check since space/colon aren't word runes. Consider adding -o raw to the invoke args so the assertion is scoped to the server response payload only.

No other new findings. The event-driven dispatch loop, layered timeout hierarchy (per-mode 60m < go test 125m < ADO step 130m < job 150m), and LIFO cleanup ordering are correctly structured. Prior review feedback has been addressed in the current HEAD.

Incremental review of commit 3c0f717 (scopes the 2+2 invoke assertion to the agent response region).

The agentResponseRegion approach is correct: slicing between [<agent>] and "Server responded in" cleanly isolates the model's answer from surrounding CLI noise (model names, status codes, timing info). Fallback to full text when markers are absent is a reasonable defensive choice since hasStandaloneFour already rejects most false-positive patterns.

The yq-from-extension.yaml change in the pipeline eliminates the manifest drift problem. Comment references now use function names instead of fragile line numbers. Both solid improvements.

One suggestion on test coverage below.

Incremental review of commit 3c0f717 (scopes the 2+2 invoke assertion to the agent response region).

The agentResponseRegion approach is correct: slicing between [<agent>] and "Server responded in" cleanly isolates the model's answer from surrounding CLI noise (model names, status codes, timing info). Fallback to full text when markers are absent is a reasonable defensive choice since hasStandaloneFour already rejects most false-positive patterns.

The yq-from-extension.yaml change in the pipeline eliminates the manifest drift problem. Comment references now use function names instead of fragile line numbers. Both solid improvements.

One suggestion on test coverage below.

I've been reviewing this PR across multiple rounds. Here's my final assessment of the current state:

The Go PTY-based test driver is well-structured. The event-driven prompt dispatch (waitForQuiet + screen-read + dispatch) is a significant improvement over fixed-interval polling, and the loop detection (promptKey + repeat counter) prevents silent hangs when prompts change upstream.

The answer assertion logic (assert.go) correctly handles the tricky problem of detecting a standalone "4" without false-positiving on model names, versions, or status codes. The agentResponseRegion scoping adds a solid second layer of defense.

Timeout hierarchy is correctly ordered (per-mode runTimeout < go test -timeout < ADO step < ADO job) so the in-test teardown always gets a chance to run before anything force-kills the process.

My prior comments were all non-blocking suggestions (broader keyword-match tightening in dispatchPrompt, an additional test case for region-scoping in assert_test.go). None are blockers.

One note for the broader reviewer group: the go.mod adds go-expect, creack/pty, and vt10x at 2022-era commits. These are stable, widely-used packages for PTY testing in Go (used by AlecAivazis/survey itself). They don't have newer tagged releases that would be materially better.

One gap to close before merge: the EngSys DeleteAfter tag on resource groups (as @danieljurek noted). The in-test azd down and the condition: always() safety-net cover the happy and crash paths, but if both fail (VM hard-kill, network timeout during teardown), resources leak with no EngSys visibility.

Suggested fix: after phaseProvision returns, shell out to tag the resource group:

// inside runner, after phaseProvision succeeds:
rgName := r.resolveResourceGroup() // azd env get-values | grep AZURE_RESOURCE_GROUP
if rgName != "" {
    deleteAfter := time.Now().Add(48 * time.Hour).UTC().Format(time.RFC3339)
    r.runCmd(ctx, "az", "tag", "update",
        "--resource-id", "/subscriptions/"+sub+"/resourceGroups/"+rgName,
        "--operation", "merge", "--tags", "DeleteAfter="+deleteAfter)
}

This way the EngSys cleanup pipeline can find and purge any survivors. The 48h window gives plenty of room for the normal teardown to succeed first.

The rest of the implementation is solid: event-driven prompt loop, proper timeout hierarchy, LIFO cleanup ordering, and the agentResponseRegion scoping all look correct.

Incremental review against 6c66c25 (new commits since my last review at 2d9dfb7).

The DeleteAfter tagging implementation addresses my prior concern and @danieljurek's feedback. Both the in-test tag (after successful provision) and the pipeline cleanup step correctly use RFC 3339 UTC format per the EngSys spec. One non-blocking edge case noted inline.

Incremental review of 2 new commits since my last pass (6c66c25).

30dfb29 (SplitSeq): Correct go fix modernization. strings.SplitSeq returns a lazy iterator, semantically identical to the previous strings.Split usage.

e1c7ab8 (cleanup tagging): The pipeline safety-net step now enumerates .azure/*/ directories to construct
g- directly, instead of reading AZURE_RESOURCE_GROUP from �zd env get-values. This closes the edge case I flagged in my last review: if provision times out before persisting env values, the old approach would skip tagging entirely. The hard-coded
g- convention is safe here since the test always uses default azd naming and the || true guard makes it best-effort.

All prior findings (errors.AsType, t.Context(), yaml.Unmarshal for service parsing, stderr visibility, prompt dispatch tightening, DeleteAfter tagging) are addressed.

No new issues.

All prior review concerns (response region scoping, function-name anchors instead of file:line, yq-generated config.json, DeleteAfter tagging including the provision-timeout case) are addressed in the latest commits.

One low-severity note on the test dependencies (non-blocking).

Incremental review of 990ffa5 (bumps creack/pty from v1.1.17 to v1.1.24).

This addresses my prior comment about starting on the latest stable version for new test infrastructure. v1.1.24 is a legitimate release (Oct 2024), the pty.Open/Setsize API is unchanged, and the author confirmed clean Linux build/vet/lint.

No new issues found across the full changeset. Prior concerns (response region scoping, function-name anchors instead of file:line, yq-generated config.json, DeleteAfter tagging for the provision-timeout case) are all resolved in earlier commits.