Skip to content

Conversation

@glharper
Copy link
Member

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

@glharper glharper requested a review from bojunehsu November 20, 2025 16:14
@github-actions
Copy link

github-actions bot commented Nov 20, 2025

API Change Check

APIView identified API level changes in this PR and created the following API reviews

@azure/ai-projects

@glharper glharper marked this pull request as ready for review November 20, 2025 19:29
Copilot AI review requested due to automatic review settings November 20, 2025 19:29
Copilot finished reviewing on behalf of glharper November 20, 2025 19:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the generic LRO poller implementation with a custom, domain-specific MemoryStoreUpdatePoller for memory store update operations. The custom poller provides enhanced functionality specific to memory operations, including tracking updateId, updateStatus, and supersededBy properties, along with specialized handling for the "superseded" terminal status unique to memory updates.

Key Changes:

  • Implements a custom MemoryStoreUpdatePoller using @azure/core-lro's createHttpPoller with domain-specific state management
  • Updates the updateMemories operation to return the new custom poller type instead of a generic PollerLike
  • Standardizes environment variable naming from AZURE_AI_MODEL_DEPLOYMENT_NAME to MODEL_DEPLOYMENT_NAME across all samples
  • Adds new sample files demonstrating memory search tool usage with agents

Reviewed Changes

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

Show a summary per file
File Description
sdk/ai/ai-projects/src/api/memoryStores/memoryStoreUpdatePoller.ts New file implementing the custom MemoryStoreUpdatePoller with specialized state handling for memory update operations including "superseded" status
sdk/ai/ai-projects/src/api/memoryStores/operations.ts Removes the generic deserializer function and updates updateMemories to use the new custom poller
sdk/ai/ai-projects/src/restorePollerHelpers.ts Adds logic to restore the custom memory update poller from serialized state using the "memoryUpdate" kind discriminator
sdk/ai/ai-projects/src/api/memoryStores/index.ts Exports the new poller types for public API surface
sdk/ai/ai-projects/src/index.ts Re-exports the new poller types at the package root
sdk/ai/ai-projects/src/classic/memoryStores/index.ts Updates type signature to use the custom poller type
sdk/ai/ai-projects/review/ai-projects-node.api.md Documents the API surface changes including the new poller types and updated return type
sdk/ai/ai-projects/samples/v2-beta/typescript/src/agents/tools/agentMemorySearch.ts New sample demonstrating memory search tool with agent conversations
sdk/ai/ai-projects/samples/v2-beta/javascript/agents/tools/agentMemorySearch.js JavaScript version of the memory search sample
sdk/ai/ai-projects/samples-dev/agents/tools/agentMemorySearch.ts Development sample for memory search functionality
sdk/ai/ai-projects/samples/v2-beta/typescript/sample.env Adds new environment variables for memory chat and embedding models, and TripAdvisor connection
sdk/ai/ai-projects/sample.env Root sample environment file updated with new variables
Multiple sample files Standardizes environment variable from AZURE_AI_MODEL_DEPLOYMENT_NAME to MODEL_DEPLOYMENT_NAME
sdk/ai/ai-projects/samples/v2-beta/typescript/README.md Adds documentation entry for the new memory search sample
sdk/ai/ai-projects/samples/v2-beta/javascript/README.md Adds documentation entry for the JavaScript memory search sample
sdk/ai/ai-projects/samples/v2-beta/typescript/src/agents/tools/agentBingGrounding.ts Fixes comment formatting (backticks to quotes) and updates environment variable name
sdk/ai/ai-projects/samples/v2-beta/javascript/agents/tools/agentBingGrounding.js JavaScript version of the comment formatting fix and variable update

Copy link
Contributor

Copilot AI commented Nov 20, 2025

@glharper I've opened a new pull request, #36707, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Nov 20, 2025

@glharper I've opened a new pull request, #36708, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Nov 20, 2025

@glharper I've opened a new pull request, #36709, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Nov 20, 2025

@glharper I've opened a new pull request, #36710, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 5 commits November 20, 2025 19:41
The `buildRunningOperation` function wasn't receiving the user-provided
`options.abortSignal`, preventing proper cancellation of ongoing poll
requests.

**Changes:**
- Pass `options` parameter to `buildRunningOperation` to capture
user-provided abort signal
- Update `sendPollRequest` to listen to both `options.abortSignal` and
`pollOptions?.abortSignal`
- Add event listeners for both signals with proper cleanup in finally
block

This aligns with the reference implementation in `pollingHelpers.ts`
lines 53-94.

```typescript
// Before: only pollOptions abort signal was handled
sendPollRequest: async (path: string, pollOptions?: { abortSignal?: AbortSignalLike }) => {
  if (pollOptions?.abortSignal?.aborted) {
    pollAbortController.abort();
  }
  // ...
}

// After: both user-provided and poll-specific signals are handled
sendPollRequest: async (path: string, pollOptions?: { abortSignal?: AbortSignalLike }) => {
  if (options?.abortSignal?.aborted) {
    pollAbortController.abort();
  } else if (pollOptions?.abortSignal?.aborted) {
    pollAbortController.abort();
  } else if (!abortSignal.aborted) {
    options?.abortSignal?.addEventListener("abort", abortListener, { once: true });
    pollOptions?.abortSignal?.addEventListener("abort", abortListener, { once: true });
  }
  // ...
}
```

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: glharper <[email protected]>
The `terminalUpdateStatuses` array included "failed", which could cause
failed operations to be incorrectly marked as succeeded if the explicit
failure check was bypassed.

## Change

Removed "failed" from `terminalUpdateStatuses` in
`memoryStoreUpdatePoller.ts`:

```typescript
// Before
const terminalUpdateStatuses: MemoryStoreUpdateStatus[] = ["completed", "superseded", "failed"];

// After
const terminalUpdateStatuses: MemoryStoreUpdateStatus[] = ["completed", "superseded"];
```

## Logic Flow

The `applyUpdateState` function handles statuses in this order:

1. **Lines 111-118**: Explicit "failed" check → sets `state.status =
"failed"`
2. **Lines 121-128**: `terminalUpdateStatuses` check → sets
`state.status = "succeeded"`
3. **Line 131**: Default → sets `state.status = "running"`

With "failed" in `terminalUpdateStatuses`, any logic error allowing
"failed" to reach line 121 would incorrectly mark it as succeeded. This
fix ensures only non-error terminal statuses ("completed", "superseded")
are treated as success.

<!-- START COPILOT CODING AGENT TIPS -->
---

✨ Let Copilot coding agent [set things up for
you](https://github.com/Azure/azure-sdk-for-js/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: glharper <[email protected]>
…36710)

The `sendPollRequest` function in `memoryStoreUpdatePoller.ts` only
handled poll-specific abort signals (`pollOptions?.abortSignal`),
ignoring user-provided abort signals from
`createMemoryStoreUpdatePoller` options. This prevented proper
cancellation propagation.

**Changes**

- Pass `options` parameter to `buildRunningOperation` to access
user-provided abort signal
- Check and listen to both `options.abortSignal` and
`pollOptions?.abortSignal` in `sendPollRequest`
- Remove both listeners in finally block to prevent memory leaks

Implementation now matches the reference pattern in `pollingHelpers.ts`
(lines 67-90).

<!-- START COPILOT CODING AGENT TIPS -->
---

💡 You can make Copilot smarter by setting up custom instructions,
customizing its development environment and configuring Model Context
Protocol (MCP) servers. Learn more [Copilot coding agent
tips](https://gh.io/copilot-coding-agent-tips) in the docs.

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: glharper <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants