-
Notifications
You must be signed in to change notification settings - Fork 3
fix: Fix slow list endpoints for object oriented sdk #767
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -116,10 +116,10 @@ export class Agent { | |||||||||||||||||||||||||||||||||
| params?: AgentListParams, | ||||||||||||||||||||||||||||||||||
| options?: Core.RequestOptions, | ||||||||||||||||||||||||||||||||||
| ): Promise<Agent[]> { | ||||||||||||||||||||||||||||||||||
| const agents = await client.agents.list(params, options); | ||||||||||||||||||||||||||||||||||
| const page = await client.agents.list(params, options); | ||||||||||||||||||||||||||||||||||
| const result: Agent[] = []; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| for await (const agent of agents) { | ||||||||||||||||||||||||||||||||||
| for (const agent of page.getPaginatedItems()) { | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+119
to
+122
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: This change breaks the previous Severity Level: Major
|
||||||||||||||||||||||||||||||||||
| const page = await client.agents.list(params, options); | |
| const result: Agent[] = []; | |
| for await (const agent of agents) { | |
| for (const agent of page.getPaginatedItems()) { | |
| const result: Agent[] = []; | |
| if (params?.limit !== undefined) { | |
| const page = await client.agents.list(params, options); | |
| for (const agent of page.getPaginatedItems()) { | |
| result.push(new Agent(client, agent.id)); | |
| } | |
| } else { | |
| for await (const agent of client.agents.list(params, options)) { | |
| result.push(new Agent(client, agent.id)); | |
| } |
Steps of Reproduction ✅
1. In a consumer project using this SDK, call `runloop.agent.list()` **without** passing a
`limit` parameter, which routes to `Agent.list(client, params?, options?)` in
`src/sdk/agent.ts:114-127` (final file state).
2. Inside `Agent.list`, the code at `src/sdk/agent.ts:119` executes `const page = await
client.agents.list(params, options);`, obtaining only the first page of results from the
underlying `client.agents.list` paginator.
3. The subsequent loop at `src/sdk/agent.ts:122-123` iterates `for (const agent of
page.getPaginatedItems())`, which, per the PR description, returns only items from that
single page rather than auto-paginating through all pages.
4. The function returns `result` at `src/sdk/agent.ts:126`, containing only the first page
of agents; any existing caller that previously relied on `runloop.agent.list()` to
auto-paginate and return all agents now silently receives a truncated list instead of the
complete set.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/sdk/agent.ts
**Line:** 119:122
**Comment:**
*Logic Error: This change breaks the previous `list()` contract by always returning only the first page, even when no `limit` is provided. Callers using `runloop.agent.list()` to fetch all agents will now silently get partial results. Keep first-page behavior only when `limit` is explicitly set, and preserve auto-pagination otherwise.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: This change breaks the documented behavior of listing all items when no pagination limit is provided: it now always returns only the first page. Preserve full auto-pagination when
limitis not explicitly set, and only usegetPaginatedItems()for the single-page path when alimitis provided. [logic error]Severity Level: Major⚠️
Steps of Reproduction ✅
Prompt for AI Agent 🤖