Skip to content

fix(agent-client): serialize fields[] projection as comma-separated string (PRD-437) #1622

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.

Sign up for GitHub

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
christophebrun-forest wants to merge 1 commit into
base: feat/prd-214-server-step-mapper
Choose a base branch
from
+50 −61
Open

fix(agent-client): serialize fields[] projection as comma-separated string (PRD-437) #1622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/agent-client/src/query-serializer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export default class QuerySerializer {
.toLowerCase();
}

private static formatFields(collectionName: string, fields: string[]): Record<string, string[]> {
private static formatFields(collectionName: string, fields: string[]): Record<string, string> {
if (!fields) return {};

const projectionName = `fields[${HttpRequester.escapeUrlSlug(collectionName)}]`;
Expand Down Expand Up @@ -138,6 +138,10 @@ export default class QuerySerializer {
}
});

return projection;
// Join per type (`fields[users]=id,name`): an array would serialize as repeated params,
// which Ruby (Rack) agents collapse to the last value only — dropping every field but one.
return Object.fromEntries(
Object.entries(projection).map(([key, values]) => [key, values.join(',')]),
);
}
}
88 changes: 38 additions & 50 deletions packages/agent-client/test/query-serializer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,23 @@ describe('QuerySerializer', () => {
expect(result.search).toBe('john');
expect(result['page[size]']).toBe(15);
expect(result['page[number]']).toBe(3);
expect(result['fields[users]']).toEqual(['id', 'name']);
expect(result['fields[users]']).toBe('id,name');
});
});

it('should serialize fields with escaped collection name', () => {
const result = QuerySerializer.serialize({ fields: ['id', 'name', 'email'] }, 'users');
expect(result['fields[users]']).toEqual(['id', 'name', 'email']);
expect(result['fields[users]']).toBe('id,name,email');
});

it('should serialize fields as a single comma-separated value, not an array', () => {
// Regression (PRD-437): an array serializes as repeated params that Ruby agents collapse to one.
Copy link
Copy Markdown

@hercemer42 hercemer42 Jun 4, 2026

Choose a reason for hiding this comment

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

Claude Opus 4.8 — [Violates conventions] Comments must not reference PRD/Linear ticket numbers, including in tests. Drop the (PRD-437) token, e.g.:

// Regression: an array serializes as repeated params that Ruby agents collapse to one.

The source comment in query-serializer.ts correctly avoids the reference; this is the only ticket reference in the changed files.

const result = QuerySerializer.serialize(
{ fields: ['first_name', 'last_name', 'email'] },
'users',
);
expect(result['fields[users]']).toBe('first_name,last_name,email');
expect(Array.isArray(result['fields[users]'])).toBe(false);
});

it('should serialize ascending sort', () => {
Expand Down Expand Up @@ -188,7 +198,7 @@ describe('QuerySerializer', () => {
expect(result.search).toBe('john');
expect(result['page[size]']).toBe(20);
expect(result['page[number]']).toBe(1);
expect(result['fields[users]']).toEqual(['id', 'name']);
expect(result['fields[users]']).toBe('id,name');
expect(result.sort).toBe('-createdAt');
const parsed = JSON.parse(result.filters as string);
expect(parsed.conditions[0].operator).toBe('equal');
Expand All @@ -197,7 +207,7 @@ describe('QuerySerializer', () => {

it('should handle collection names with special characters', () => {
const result = QuerySerializer.serialize({ fields: ['id'] }, 'user+data');
expect(result['fields[user\\+data]']).toEqual(['id']);
expect(result['fields[user\\+data]']).toBe('id');
});

it('should handle undefined sort', () => {
Expand All @@ -214,11 +224,8 @@ describe('QuerySerializer', () => {
it('should handle single relation field with @@@ separator', () => {
const result = QuerySerializer.serialize({ fields: ['id', 'customer@@@name'] }, 'orders');

// Main collection should have id and the relation name (customer)
expect(result['fields[orders]']).toContain('id');
expect(result['fields[orders]']).toContain('customer');
// Related collection should have the field name
expect(result['fields[customer]']).toContain('name');
expect(result['fields[orders]']).toBe('id,customer');
expect(result['fields[customer]']).toBe('name');
});

it('should handle multiple relation fields from the same relation', () => {
Expand All @@ -227,12 +234,8 @@ describe('QuerySerializer', () => {
'orders',
);

// Main collection should have id and the relation name (customer) - may appear twice
expect(result['fields[orders]']).toContain('id');
expect(result['fields[orders]']).toContain('customer');
// Related collection should have both field names
expect(result['fields[customer]']).toContain('name');
expect(result['fields[customer]']).toContain('email');
expect(result['fields[orders]']).toBe('id,customer');
expect(result['fields[customer]']).toBe('name,email');
});

it('should handle multiple different relations', () => {
Expand All @@ -241,13 +244,9 @@ describe('QuerySerializer', () => {
'orders',
);

// Main collection should have id and both relation names
expect(result['fields[orders]']).toContain('id');
expect(result['fields[orders]']).toContain('customer');
expect(result['fields[orders]']).toContain('product');
// Each related collection should have its field
expect(result['fields[customer]']).toContain('name');
expect(result['fields[product]']).toContain('title');
expect(result['fields[orders]']).toBe('id,customer,product');
expect(result['fields[customer]']).toBe('name');
expect(result['fields[product]']).toBe('title');
});

it('should only process first level of relation with multiple @@@ separators', () => {
Expand All @@ -256,12 +255,9 @@ describe('QuerySerializer', () => {
'orders',
);

// Main collection should have id and customer relation
expect(result['fields[orders]']).toContain('id');
expect(result['fields[orders]']).toContain('customer');
// Only first level is processed - "address@@@city" becomes the field name
expect(result['fields[customer]']).toContain('address@@@city');
// No further nesting - address collection should not exist
// "address@@@city" stays the field name — no nesting beyond the first level
expect(result['fields[orders]']).toBe('id,customer');
expect(result['fields[customer]']).toBe('address@@@city');
expect(result['fields[address]']).toBeUndefined();
});

Expand All @@ -271,21 +267,15 @@ describe('QuerySerializer', () => {
'orders',
);

// Main collection should only have the relation name
expect(result['fields[orders]']).toContain('customer');
expect(result['fields[orders]']).not.toContain('name');
expect(result['fields[orders]']).not.toContain('email');
// Related collection should have both fields
expect(result['fields[customer]']).toContain('name');
expect(result['fields[customer]']).toContain('email');
expect(result['fields[orders]']).toBe('customer');
expect(result['fields[customer]']).toBe('name,email');
});

it('should escape special characters in relation names', () => {
const result = QuerySerializer.serialize({ fields: ['id', 'user+data@@@name'] }, 'orders');

expect(result['fields[orders]']).toContain('id');
expect(result['fields[orders]']).toContain('user+data');
expect(result['fields[user\\+data]']).toContain('name');
expect(result['fields[orders]']).toBe('id,user+data');
expect(result['fields[user\\+data]']).toBe('name');
});

it('should handle empty fields array', () => {
Expand All @@ -298,28 +288,28 @@ describe('QuerySerializer', () => {
it('should skip empty strings in fields', () => {
const result = QuerySerializer.serialize({ fields: ['', 'name', ''] }, 'users');

expect(result['fields[users]']).toEqual(['name']);
expect(result['fields[users]']).toBe('name');
});

it('should skip malformed @@@ separator with missing relation name', () => {
const result = QuerySerializer.serialize({ fields: ['id', '@@@fieldName'] }, 'orders');

// Should only include 'id', skip the malformed '@@@fieldName'
expect(result['fields[orders]']).toEqual(['id']);
expect(result['fields[orders]']).toBe('id');
});

it('should skip malformed @@@ separator with missing field name', () => {
const result = QuerySerializer.serialize({ fields: ['id', 'customer@@@'] }, 'orders');

// Should only include 'id', skip the malformed 'customer@@@'
expect(result['fields[orders]']).toEqual(['id']);
expect(result['fields[orders]']).toBe('id');
});

it('should skip standalone @@@ separator', () => {
const result = QuerySerializer.serialize({ fields: ['id', '@@@'] }, 'orders');

// Should only include 'id', skip the malformed '@@@'
expect(result['fields[orders]']).toEqual(['id']);
expect(result['fields[orders]']).toBe('id');
});

it('should not include duplicate relation names', () => {
Expand All @@ -328,29 +318,27 @@ describe('QuerySerializer', () => {
'orders',
);

// customer should only appear once in the orders fields
const ordersFields = result['fields[orders]'] as string[];
const customerCount = ordersFields.filter(f => f === 'customer').length;
expect(customerCount).toBe(1);
// customer appears once even though two of its fields were requested
expect(result['fields[orders]']).toBe('id,customer');
});

it('should not include duplicate field names', () => {
const result = QuerySerializer.serialize({ fields: ['id', 'id', 'name', 'name'] }, 'users');

expect(result['fields[users]']).toEqual(['id', 'name']);
expect(result['fields[users]']).toBe('id,name');
});

it('should handle whitespace in field names', () => {
const result = QuerySerializer.serialize({ fields: [' id ', ' name '] }, 'users');

expect(result['fields[users]']).toEqual(['id', 'name']);
expect(result['fields[users]']).toBe('id,name');
});

it('should handle whitespace around @@@ separator', () => {
const result = QuerySerializer.serialize({ fields: [' customer @@@ name '] }, 'orders');

expect(result['fields[orders]']).toContain('customer');
expect(result['fields[customer]']).toContain('name');
expect(result['fields[orders]']).toBe('customer');
expect(result['fields[customer]']).toBe('name');
});
});
});
Expand Down
15 changes: 6 additions & 9 deletions packages/mcp-server/test/server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1700,7 +1700,7 @@
});

expect(response.status).toBe(200);
expect(capturedQueryParams['fields[users]']).toEqual(['id', 'name', 'email']);
expect(capturedQueryParams['fields[users]']).toBe('id,name,email');
});

it('should pass relation fields with @@@ separator to the agent', async () => {
Expand All @@ -1724,13 +1724,10 @@
});

expect(response.status).toBe(200);
// Main collection should have the relation name
expect(capturedQueryParams['fields[users]']).toContain('id');
expect(capturedQueryParams['fields[users]']).toContain('name');
expect(capturedQueryParams['fields[users]']).toContain('company');
// Related collection should have the field names
expect(capturedQueryParams['fields[company]']).toContain('name');
expect(capturedQueryParams['fields[company]']).toContain('address');
const usersFields = (capturedQueryParams['fields[users]'] as string).split(',');
const companyFields = (capturedQueryParams['fields[company]'] as string).split(',');
expect(usersFields).toEqual(expect.arrayContaining(['id', 'name', 'company']));
expect(companyFields).toEqual(expect.arrayContaining(['name', 'address']));
});
});

Expand Down Expand Up @@ -1970,7 +1967,7 @@
expect(capturedQueryParams.search).toBe('john');
expect(capturedQueryParams.searchExtended).toBe(true);
expect(capturedQueryParams.sort).toBe('name');
expect(capturedQueryParams['fields[users]']).toEqual(['id', 'name', 'email']);
expect(capturedQueryParams['fields[users]']).toBe('id,name,email');
expect(capturedQueryParams['page[size]']).toBe(20);
expect(capturedQueryParams['page[number]']).toBe(2);

Expand Down Expand Up @@ -2915,7 +2912,7 @@
envSecret: 'ENV_SECRET',
authSecret: 'AUTH_SECRET',
logger,
enabledTools: ['list', 'lst', 'creat' as any],

Check warning on line 2915 in packages/mcp-server/test/server.test.ts

View workflow job for this annotation

GitHub Actions / Linting & Testing (mcp-server) 

Unexpected any. Specify a different type
});

expect(server).toBeDefined();
Expand Down
Loading