[Agents Extension] Fix for session persistence - #8096
Conversation
Signed-off-by: trangevi <trangevi@microsoft.com>
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
There was a problem hiding this comment.
Pull request overview
This PR addresses #8094 by restoring a stable agent base endpoint in the azd environment so session-related commands (notably azd ai agent files ...) can correctly resolve persisted session IDs rather than issuing requests with an empty session path segment.
Changes:
- Set
AGENT_{SERVICE_KEY}_ENDPOINTto<AZURE_AI_PROJECT_ENDPOINT>/agents/<agentName>for session persistence/session management use. - Update unit tests to assert the new base endpoint value.
- Clarify naming/comments in
showendpoint resolution fallback logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent.go |
Sets the base AGENT_{KEY}_ENDPOINT env var to support session persistence. |
cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent_test.go |
Updates the test to validate the new base endpoint env var value. |
cli/azd/extensions/azure.ai.agents/internal/cmd/show.go |
Renames local variable/comment for the single-endpoint fallback lookup. |
wbreza
left a comment
There was a problem hiding this comment.
✅ Approve — Functionally Correct
The core change is sound: populating AGENT_{KEY}_ENDPOINT with the base agent URL enables session persistence and cleanup paths that were previously dead code (gated by endpoint != ""). Storage and cleanup both use buildRemoteAgentKeyFromEndpoint() with the same input, so keys match correctly.
Findings
P1 — Misleading comment in config_store.go:241
The comment says AGENT_{SVC}_ENDPOINT should be <projectEndpoint>/agents/<name>/versions/<ver>, but this PR correctly sets it to <projectEndpoint>/agents/<name> (without version). The code works fine — the comment is stale and could mislead future developers.
P1 — PR description could use more context
"Fixes #8094" doesn't convey the behavioral impact. This change activates session persistence paths across 5+ files (helpers.go, files.go, invoke.go, session.go, monitor.go) that were previously dead code. A few sentences describing the before/after would help future on-call engineers.
P2 — Consider defensive guard for empty agent name
agentVersionResponse.Name is validated upstream by the API, but a one-line guard in registerAgentEnvironmentVariables would be cheap insurance.
Note — Minor naming inconsistency
singleEndpointKey in show.go vs baseEndpointKey in service_target_agent.go — same concept, different names. Not blocking; worth aligning in a future pass.
Signed-off-by: trangevi <trangevi@microsoft.com>
jongio
left a comment
There was a problem hiding this comment.
Fixes #8094 - restoring the base agent endpoint instead of clearing it is the right call. The previous approach of setting AGENT_{KEY}_ENDPOINT to empty broke session URL construction for iles and likely other session-dependent commands.
The URL format (/agents/) makes sense for session management since it's agent-scoped, not version-scoped.
Test is properly updated. show.go rename from "legacy" to "singleEndpoint" accurately reflects the new semantics.
Signed-off-by: trangevi <trangevi@microsoft.com>
wbreza
left a comment
There was a problem hiding this comment.
Re-review after new commit (3d38b44)
The new commit "Add version to agent endpoint" addresses the P1 finding from my previous review. Session storage and cleanup keys now match:
- Storage (buildAgentKey):
normalizedEndpoint/agents/name/versions/ver/remote - Cleanup (buildRemoteAgentKeyFromEndpoint):
normalizedEndpoint/agents/name/versions/ver/remote
The comment in config_store.go:241 is now accurate — it documents the /versions/<ver> format that the code produces.
Remaining advisory items (non-blocking)
- PR description is still minimal ("Fixes #8094") — a few sentences about the behavioral change would help future on-call
- Consider a defensive guard for empty
agentVersionResponse.NameoragentVersionResponse.Version - Minor:
singleEndpointKey(show.go) vsbaseEndpointKey(service_target_agent.go) naming inconsistency
LGTM — approving.
|
/check-enforcer override |
* Fix for session persistence Signed-off-by: trangevi <trangevi@microsoft.com> * Add version to agent endpoint Signed-off-by: trangevi <trangevi@microsoft.com> * PR comments Signed-off-by: trangevi <trangevi@microsoft.com> --------- Signed-off-by: trangevi <trangevi@microsoft.com> Co-authored-by: therealjohn <1501196+therealjohn@users.noreply.github.com>
Fixes #8094
Reverts change which was declaring the base agent endpoint as legacy, and uses it for session state. This needs to be used since sessions are independent of /invocations or /responses routes