Skip to content

Commit 0aba13d

Browse files
waleedlatif1waleedlatif1
committed
fix(agiloft): stop retrying refusals, and expose the outputs the new operations return
Five findings from review that had gone unanswered. An Agiloft refusal was surfacing as HTTP 500. readAlrestJson throws when the envelope reports success:false, the route catch mapped that to 500, and the tool runner retries 500s — so a create the server had already rejected could be retried and duplicate the record. Refusals now return a settled failure with the message intact; genuine faults still 500. list_tables could not run in its primary mode. EWTable is knowledge-base scoped, but some instances reject EWLogin without a $table, so whole-knowledge-base discovery failed at login with nothing to fall back to. It now says what the caller can do about it rather than surfacing the raw login error. Upsert corrupted structured values. Every field went through String(), so a multi-value field collapsed into one joined string instead of the documented repeated key/value pairs, and an object silently wrote "[object Object]" into the record. Arrays now encode as repeated pairs and objects are refused, since Agiloft documents no encoding for them. Two outputs were invisible in the editor. `records` was conditioned on search_records alone, so natural language search results could not be chained, and `callbackId` on run_action_button alone, so a queued upsert's callback could not be wired into Async Status even though both values exist at runtime.
1 parent be5d283 commit 0aba13d

12 files changed

Lines changed: 273 additions & 14 deletions

File tree

‎apps/sim/app/api/tools/agiloft/create_record/route.test.ts‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -551,3 +551,81 @@ describe('review round 2 fixes', () => {
551551
expect(data.output.id).toBe('123')
552552
})
553553
})
554+
555+
describe('review round 3 fixes', () => {
556+
it('reports an Agiloft refusal as a non-retryable failure, not a 500', async () => {
557+
arrange(
558+
res({ json: { success: false, errors: [{ message: 'Field contract_title1 is required' }] } })
559+
)
560+
561+
const response = await POST(createMockRequest('POST', { ...baseBody, data: '{"a":"b"}' }))
562+
563+
/**
564+
* The tool runner retries 500s, and retrying a refused create can duplicate
565+
* a record — a refusal must come back as a settled failure.
566+
*/
567+
expect(response.status).toBe(200)
568+
const data = (await response.json()) as { success: boolean; error?: string }
569+
expect(data.success).toBe(false)
570+
expect(data.error).toContain('Field contract_title1 is required')
571+
})
572+
573+
it('tells the user what to do when EWTable login needs a table', async () => {
574+
const { POST: LIST } = await import('@/app/api/tools/agiloft/list_tables/route')
575+
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockResolvedValueOnce(
576+
res({
577+
ok: false,
578+
status: 400,
579+
text: 'EWWrongDataException has occurred: One has to specify $table, $KB, $lang parameters',
580+
})
581+
)
582+
583+
const response = await LIST(
584+
createMockRequest('POST', {
585+
instanceUrl: baseBody.instanceUrl,
586+
knowledgeBase: baseBody.knowledgeBase,
587+
login: baseBody.login,
588+
password: baseBody.password,
589+
})
590+
)
591+
const data = (await response.json()) as { success: boolean; error?: string }
592+
593+
expect(data.success).toBe(false)
594+
expect(data.error).toContain('requires a table name to authenticate')
595+
})
596+
597+
it('encodes a multi-value upsert field as repeated pairs, not a joined string', async () => {
598+
const { POST: UPSERT } = await import('@/app/api/tools/agiloft/upsert_record/route')
599+
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockResolvedValueOnce(
600+
res({ status: 200, text: "EWREST_id='353';" })
601+
)
602+
603+
await UPSERT(
604+
createMockRequest('POST', {
605+
...baseBody,
606+
match: 'ext_id',
607+
data: '{"contactMethod":["phone","email"]}',
608+
})
609+
)
610+
611+
const body = inputValidationMockFns.mockSecureFetchWithPinnedIP.mock.calls[0][2].body as string
612+
expect(new URLSearchParams(body).getAll('contactMethod')).toEqual(['phone', 'email'])
613+
})
614+
615+
it('refuses an object field value rather than writing [object Object]', async () => {
616+
const { POST: UPSERT } = await import('@/app/api/tools/agiloft/upsert_record/route')
617+
618+
const response = await UPSERT(
619+
createMockRequest('POST', {
620+
...baseBody,
621+
match: 'ext_id',
622+
data: '{"nested":{"a":1}}',
623+
})
624+
)
625+
const data = (await response.json()) as { success: boolean; error?: string }
626+
627+
expect(data.success).toBe(false)
628+
expect(data.error).toContain('has no encoding for')
629+
expect(inputValidationMockFns.mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
630+
})
631+
})

‎apps/sim/app/api/tools/agiloft/create_record/route.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@ import { generateRequestId } from '@/lib/core/utils/request'
88
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
99
import type { AgiloftRecordResponse } from '@/tools/agiloft/types'
1010
import { alrestRecordCollectionUrl } from '@/tools/agiloft/utils'
11-
import { executeAlrestRequest, readAlrestJson } from '@/tools/agiloft/utils.server'
11+
import {
12+
executeAlrestRequest,
13+
isAgiloftRefusal,
14+
readAlrestJson,
15+
} from '@/tools/agiloft/utils.server'
1216

1317
export const dynamic = 'force-dynamic'
1418

@@ -98,6 +102,19 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
98102

99103
return NextResponse.json(result)
100104
} catch (error) {
105+
/**
106+
* A refusal Agiloft already decided on is a final answer, not a transient
107+
* fault — returning 500 would make the tool runner retry it.
108+
*/
109+
if (isAgiloftRefusal(error)) {
110+
logger.warn(`[${requestId}] Agiloft refused the request`, { error: error.message })
111+
return NextResponse.json({
112+
success: false,
113+
output: { id: null, fields: {} },
114+
error: error.message,
115+
})
116+
}
117+
101118
logger.error(`[${requestId}] Error creating Agiloft record:`, error)
102119

103120
return NextResponse.json({ success: false, error: toError(error).message }, { status: 500 })

‎apps/sim/app/api/tools/agiloft/delete_record/route.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@ import { generateRequestId } from '@/lib/core/utils/request'
88
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
99
import type { AgiloftDeleteResponse } from '@/tools/agiloft/types'
1010
import { alrestDeleteRecordUrl } from '@/tools/agiloft/utils'
11-
import { executeAlrestRequest, readAlrestJson } from '@/tools/agiloft/utils.server'
11+
import {
12+
executeAlrestRequest,
13+
isAgiloftRefusal,
14+
readAlrestJson,
15+
} from '@/tools/agiloft/utils.server'
1216

1317
export const dynamic = 'force-dynamic'
1418

@@ -73,6 +77,19 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
7377

7478
return NextResponse.json(result)
7579
} catch (error) {
80+
/**
81+
* A refusal Agiloft already decided on is a final answer, not a transient
82+
* fault — returning 500 would make the tool runner retry it.
83+
*/
84+
if (isAgiloftRefusal(error)) {
85+
logger.warn(`[${requestId}] Agiloft refused the request`, { error: error.message })
86+
return NextResponse.json({
87+
success: false,
88+
output: { id: '', deleted: false },
89+
error: error.message,
90+
})
91+
}
92+
7693
logger.error(`[${requestId}] Error deleting Agiloft record:`, error)
7794

7895
return NextResponse.json({ success: false, error: toError(error).message }, { status: 500 })

‎apps/sim/app/api/tools/agiloft/list_tables/route.ts‎

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,17 @@ import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
66
import { checkInternalAuth } from '@/lib/auth/hybrid'
77
import { generateRequestId } from '@/lib/core/utils/request'
88
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
9-
import type { AgiloftListTablesResponse, AgiloftTableField } from '@/tools/agiloft/types'
9+
import type {
10+
AgiloftListTablesParams,
11+
AgiloftListTablesResponse,
12+
AgiloftTableField,
13+
} from '@/tools/agiloft/types'
1014
import { buildListTablesUrl } from '@/tools/agiloft/utils'
11-
import { executeAgiloftRequest, readAlrestJson } from '@/tools/agiloft/utils.server'
15+
import {
16+
executeAgiloftRequest,
17+
isAgiloftRefusal,
18+
readAlrestJson,
19+
} from '@/tools/agiloft/utils.server'
1220

1321
export const dynamic = 'force-dynamic'
1422

@@ -34,6 +42,7 @@ interface EwTableResult {
3442

3543
export const POST = withRouteHandler(async (request: NextRequest) => {
3644
const requestId = generateRequestId()
45+
let params: AgiloftListTablesParams | undefined
3746

3847
try {
3948
const authResult = await checkInternalAuth(request, { requireWorkflowId: false })
@@ -65,13 +74,14 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
6574
}
6675
)
6776
if (!parsed.success) return parsed.response
68-
const params = parsed.data.body
77+
const listParams = parsed.data.body
78+
params = listParams
6979

7080
/** EWTable must run under EWLogin or OAuth authorization. */
7181
const result = await executeAgiloftRequest<AgiloftListTablesResponse>(
72-
params,
82+
listParams,
7383
(base) => ({
74-
url: buildListTablesUrl(base, params),
84+
url: buildListTablesUrl(base, listParams),
7585
method: 'GET',
7686
headers: { Accept: 'application/json' },
7787
}),
@@ -108,6 +118,33 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
108118

109119
return NextResponse.json(result)
110120
} catch (error) {
121+
/**
122+
* A refusal Agiloft already decided on is a final answer, not a transient
123+
* fault — returning 500 would make the tool runner retry it.
124+
*/
125+
/**
126+
* EWTable is knowledge-base scoped, but some instances reject EWLogin
127+
* without a $table. When that happens there is nothing to fall back to, so
128+
* say what the caller can actually do about it.
129+
*/
130+
if (!params?.table && /\$table/.test(toError(error).message)) {
131+
return NextResponse.json({
132+
success: false,
133+
output: { tables: [], totalCount: 0 },
134+
error:
135+
'This Agiloft instance requires a table name to authenticate. Put any known table in the Table field — it also narrows the result to that table.',
136+
})
137+
}
138+
139+
if (isAgiloftRefusal(error)) {
140+
logger.warn(`[${requestId}] Agiloft refused the request`, { error: error.message })
141+
return NextResponse.json({
142+
success: false,
143+
output: { tables: [], totalCount: 0 },
144+
error: error.message,
145+
})
146+
}
147+
111148
logger.error(`[${requestId}] Error listing Agiloft tables:`, error)
112149

113150
return NextResponse.json({ success: false, error: toError(error).message }, { status: 500 })

‎apps/sim/app/api/tools/agiloft/read_record/route.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { alrestRecordUrl, alrestSearchUrl, parseFieldList } from '@/tools/agilof
1111
import {
1212
type AgiloftRequestConfig,
1313
executeAlrestRequest,
14+
isAgiloftRefusal,
1415
readAlrestJson,
1516
} from '@/tools/agiloft/utils.server'
1617

@@ -138,6 +139,19 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
138139

139140
return NextResponse.json(result)
140141
} catch (error) {
142+
/**
143+
* A refusal Agiloft already decided on is a final answer, not a transient
144+
* fault — returning 500 would make the tool runner retry it.
145+
*/
146+
if (isAgiloftRefusal(error)) {
147+
logger.warn(`[${requestId}] Agiloft refused the request`, { error: error.message })
148+
return NextResponse.json({
149+
success: false,
150+
output: { id: null, fields: {} },
151+
error: error.message,
152+
})
153+
}
154+
141155
logger.error(`[${requestId}] Error reading Agiloft record:`, error)
142156

143157
return NextResponse.json({ success: false, error: toError(error).message }, { status: 500 })

‎apps/sim/app/api/tools/agiloft/saved_search/route.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@ import { generateRequestId } from '@/lib/core/utils/request'
88
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
99
import type { AgiloftSavedSearchResponse } from '@/tools/agiloft/types'
1010
import { buildSavedSearchUrl } from '@/tools/agiloft/utils'
11-
import { executeAgiloftRequest, readAlrestJson } from '@/tools/agiloft/utils.server'
11+
import {
12+
executeAgiloftRequest,
13+
isAgiloftRefusal,
14+
readAlrestJson,
15+
} from '@/tools/agiloft/utils.server'
1216

1317
export const dynamic = 'force-dynamic'
1418

@@ -84,6 +88,19 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
8488

8589
return NextResponse.json(result)
8690
} catch (error) {
91+
/**
92+
* A refusal Agiloft already decided on is a final answer, not a transient
93+
* fault — returning 500 would make the tool runner retry it.
94+
*/
95+
if (isAgiloftRefusal(error)) {
96+
logger.warn(`[${requestId}] Agiloft refused the request`, { error: error.message })
97+
return NextResponse.json({
98+
success: false,
99+
output: { searches: [], totalCount: 0 },
100+
error: error.message,
101+
})
102+
}
103+
87104
logger.error(`[${requestId}] Error listing Agiloft saved searches:`, error)
88105

89106
return NextResponse.json({ success: false, error: toError(error).message }, { status: 500 })

‎apps/sim/app/api/tools/agiloft/search_records/route.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,11 @@ import { generateRequestId } from '@/lib/core/utils/request'
99
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1010
import type { AgiloftSearchResponse } from '@/tools/agiloft/types'
1111
import { AGILOFT_MAX_SEARCH_RECORDS, alrestSearchUrl, parseFieldList } from '@/tools/agiloft/utils'
12-
import { executeAlrestRequest, readAlrestJson } from '@/tools/agiloft/utils.server'
12+
import {
13+
executeAlrestRequest,
14+
isAgiloftRefusal,
15+
readAlrestJson,
16+
} from '@/tools/agiloft/utils.server'
1317

1418
export const dynamic = 'force-dynamic'
1519

@@ -95,6 +99,19 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
9599

96100
return NextResponse.json(result)
97101
} catch (error) {
102+
/**
103+
* A refusal Agiloft already decided on is a final answer, not a transient
104+
* fault — returning 500 would make the tool runner retry it.
105+
*/
106+
if (isAgiloftRefusal(error)) {
107+
logger.warn(`[${requestId}] Agiloft refused the request`, { error: error.message })
108+
return NextResponse.json({
109+
success: false,
110+
output: { records: [], totalCount: 0, page: 0, limit: 0, truncated: false },
111+
error: error.message,
112+
})
113+
}
114+
98115
logger.error(`[${requestId}] Error searching Agiloft records:`, error)
99116

100117
return NextResponse.json({ success: false, error: toError(error).message }, { status: 500 })

‎apps/sim/app/api/tools/agiloft/update_record/route.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@ import { generateRequestId } from '@/lib/core/utils/request'
88
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
99
import type { AgiloftRecordResponse } from '@/tools/agiloft/types'
1010
import { alrestRecordUrl } from '@/tools/agiloft/utils'
11-
import { executeAlrestRequest, readAlrestJson } from '@/tools/agiloft/utils.server'
11+
import {
12+
executeAlrestRequest,
13+
isAgiloftRefusal,
14+
readAlrestJson,
15+
} from '@/tools/agiloft/utils.server'
1216

1317
export const dynamic = 'force-dynamic'
1418

@@ -85,6 +89,19 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
8589

8690
return NextResponse.json(result)
8791
} catch (error) {
92+
/**
93+
* A refusal Agiloft already decided on is a final answer, not a transient
94+
* fault — returning 500 would make the tool runner retry it.
95+
*/
96+
if (isAgiloftRefusal(error)) {
97+
logger.warn(`[${requestId}] Agiloft refused the request`, { error: error.message })
98+
return NextResponse.json({
99+
success: false,
100+
output: { id: null, fields: {} },
101+
error: error.message,
102+
})
103+
}
104+
88105
logger.error(`[${requestId}] Error updating Agiloft record:`, error)
89106

90107
return NextResponse.json({ success: false, error: toError(error).message }, { status: 500 })

‎apps/sim/app/api/tools/agiloft/upsert_record/route.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,13 +69,24 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
6969
})
7070
}
7171

72+
let body: string
73+
try {
74+
body = buildUpsertRecordBody(params, fieldValues)
75+
} catch (error) {
76+
return NextResponse.json({
77+
success: false,
78+
output: { id: null, created: false, callbackId: null },
79+
error: toError(error).message,
80+
})
81+
}
82+
7283
const result = await executeEwRequest<AgiloftUpsertRecordResponse>(
7384
params,
7485
(base) => ({
7586
url: buildUpsertRecordUrl(base),
7687
method: 'POST',
7788
headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
78-
body: buildUpsertRecordBody(params, fieldValues),
89+
body,
7990
}),
8091
async (response) => {
8192
const body = await response.text()

0 commit comments

Comments
 (0)