Skip to content

Commit b8a5031

Browse files
Copilothotlong
andauthored
fix: address code review — use toolCallId matching, fix leftover code, clean test descriptions
Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/05de3b78-8ac6-4821-a0c8-ae863084c76b Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
1 parent a2bcd19 commit b8a5031

2 files changed

Lines changed: 16 additions & 13 deletions

File tree

packages/services/service-ai/src/__tests__/auth-and-toolcalling.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ describe('Conversation Ownership Enforcement', () => {
149149
expect((responseB.body as any).conversations).toHaveLength(1);
150150
});
151151

152-
it('should reject adding a message to another user\'s conversation', async () => {
152+
it('should reject adding a message to another user conversation', async () => {
153153
const createRoute = getRoute('POST', '/api/v1/ai/conversations');
154154
const addMsgRoute = getRoute('POST', '/api/v1/ai/conversations/:id/messages');
155155

@@ -170,7 +170,7 @@ describe('Conversation Ownership Enforcement', () => {
170170
expect((response.body as any).error).toContain('do not have access');
171171
});
172172

173-
it('should reject deleting another user\'s conversation', async () => {
173+
it('should reject deleting another user conversation', async () => {
174174
const createRoute = getRoute('POST', '/api/v1/ai/conversations');
175175
const deleteRoute = getRoute('DELETE', '/api/v1/ai/conversations/:id');
176176

packages/services/service-ai/src/ai-service.ts

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -187,16 +187,17 @@ export class AIService implements IAIService {
187187

188188
// Process results: track errors and honour onToolError callback
189189
let aborted = false;
190-
for (let i = 0; i < toolResults.length; i++) {
191-
const tr = toolResults[i];
192-
190+
for (const tr of toolResults) {
193191
if (tr.isError) {
194-
const errorEntry = { iteration, toolName: result.toolCalls[i].name, error: tr.content };
192+
// Match tool call by toolCallId for robust attribution
193+
const matchedCall = result.toolCalls!.find(tc => tc.id === tr.toolCallId);
194+
const toolName = matchedCall?.name ?? 'unknown';
195+
const errorEntry = { iteration, toolName, error: tr.content };
195196
toolErrors.push(errorEntry);
196197
this.logger.warn('[AI] chatWithTools tool error', errorEntry);
197198

198-
if (onToolError) {
199-
const action = onToolError(result.toolCalls[i], tr.content);
199+
if (onToolError && matchedCall) {
200+
const action = onToolError(matchedCall, tr.content);
200201
if (action === 'abort') {
201202
aborted = true;
202203
}
@@ -288,12 +289,14 @@ export class AIService implements IAIService {
288289
const toolResults = await this.toolRegistry.executeAll(result.toolCalls);
289290

290291
let aborted = false;
291-
for (let i = 0; i < toolResults.length; i++) {
292-
const tr = toolResults[i];
292+
for (const tr of toolResults) {
293293
if (tr.isError && onToolError) {
294-
const action = onToolError(result.toolCalls[i], tr.content);
295-
if (action === 'abort') {
296-
aborted = true;
294+
const matchedCall = result.toolCalls!.find(tc => tc.id === tr.toolCallId);
295+
if (matchedCall) {
296+
const action = onToolError(matchedCall, tr.content);
297+
if (action === 'abort') {
298+
aborted = true;
299+
}
297300
}
298301
}
299302
conversation.push({

0 commit comments

Comments
 (0)