Skip to content

refactor: restructure mcp tools fetching with options object pattern #296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

xierenyuan
Copy link
Contributor

@xierenyuan xierenyuan commented Aug 5, 2025

close #295

Copy link

changeset-bot bot commented Aug 5, 2025

🦋 Changeset detected

Latest commit: c7f503a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@openai/agents-core Patch
@openai/agents-openai Patch
@openai/agents-realtime Patch
@openai/agents Patch
@openai/agents-extensions Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@xierenyuan
Copy link
Contributor Author

Test results
sse
image
tool-filter
image
image

@vrtnis
Copy link
Contributor

vrtnis commented Aug 5, 2025

thanks for the options‑object updates, it’s definitely a nicer api going forward! I had some input on #164 that is referenced here. The original array‑only call to getAllMcpTools was removed in #164 (when tool‑filtering landed) and this PR replaces it with the options object, so existing code like getAllMcpTools(Array.from(servers.values())); i.e., 1‑arg form may fail.

As such, I'm thinking something along the lines of :

// overloads
export function getAllMcpTools(mcpServers: MCPServer[]): Promise<Tool[]>;
export function getAllMcpTools(opts: GetAllMcpToolsOptions): Promise<Tool[]>;

// implementation accepts MCPServer[] | opts
export async function getAllMcpTools(
  mcpServersOrOpts: MCPServer[] | GetAllMcpToolsOptions,
  runContext?: RunContext,
  agent?: Agent,
  convertSchemasToStrict = false,
) {
  const opts = Array.isArray(mcpServersOrOpts)
    ? { mcpServers: mcpServersOrOpts, runContext, agent, convertSchemasToStrict }
    : mcpServersOrOpts;
  // …existing logic…
}

Would you be open to restoring that legacy overload while keeping the new one? Happy to send a quick patch for this part. My quick thought here is that it would restore back‑compat, lets new callers use the options object (potentially the array form can be marked deprecated in a future PR)

@xierenyuan
Copy link
Contributor Author

xierenyuan commented Aug 6, 2025

thanks for the options‑object updates, it’s definitely a nicer api going forward! I had some input on #164 that is referenced here. The original array‑only call to getAllMcpTools was removed in #164 (when tool‑filtering landed) and this PR replaces it with the options object, so existing code like getAllMcpTools(Array.from(servers.values())); i.e., 1‑arg form may fail.

As such, I'm thinking something along the lines of :

// overloads
export function getAllMcpTools(mcpServers: MCPServer[]): Promise<Tool[]>;
export function getAllMcpTools(opts: GetAllMcpToolsOptions): Promise<Tool[]>;

// implementation accepts MCPServer[] | opts
export async function getAllMcpTools(
  mcpServersOrOpts: MCPServer[] | GetAllMcpToolsOptions,
  runContext?: RunContext,
  agent?: Agent,
  convertSchemasToStrict = false,
) {
  const opts = Array.isArray(mcpServersOrOpts)
    ? { mcpServers: mcpServersOrOpts, runContext, agent, convertSchemasToStrict }
    : mcpServersOrOpts;
  // …existing logic…
}

Would you be open to restoring that legacy overload while keeping the new one? Happy to send a quick patch for this part. My quick thought here is that it would restore back‑compat, lets new callers use the options object (potentially the array form can be marked deprecated in a future PR)

Hi, I've adjusted it. You see if it meets expectations

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.

getAllMcpTools introduced breaking changes
2 participants