fix(agiloft): settle encoding refusals instead of returning a retryable 500

Pre-landing review findings.

The create body was encoded inside the request builder, so a field value
Agiloft cannot encode threw out through the executor and became a 500. The
tool runner retries 500s, and an unencodable field is a permanent refusal,
not a transient fault. Encoding now happens before the request is issued and
answers 200 with success:false like every other create failure.

Record data reaches the body builders from workflow input, so a field named
after a reserved parameter - $table, $KB, $login, $password - appended a
second occurrence of it and let that data choose the table the record lands
in or the credentials the call runs under. Reserved names are now refused.

Natural language search returned an Agiloft refusal as a 500 while its six
sibling operations return 200 with success:false, so a refused search was
retried. It now follows the same convention, and its test pins the status
rather than only the body.

Also bounds both create error messages to 300 characters, matching the alrest
reader, so an unmatched HTML error page cannot be relayed whole into the tool
response and the workflow log; drops a pagination value that does not read as
a whole number rather than forwarding it for Agiloft to ignore; removes the
alrest collection URL builder left dead by the move to EWCreate; and corrects
three doc comments the move left describing the wrong function or surface.

Author: mzxchandra

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

@@ -261,16 +261,39 @@ describe('EWCreate', () => {
     expect(inputValidationMockFns.mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
   })
 
-  it('refuses an object field value rather than writing [object Object]', async () => {
+  /**
+   * An unencodable field is a permanent refusal, so it has to come back settled
+   * like every other create failure. Encoding it inside the request builder
+   * would surface the TypeError as a 500, which the tool runner then retries.
+   */
+  it('refuses an object field value as a settled failure, not a retryable 500', async () => {
     const response = await POST(
       createMockRequest('POST', { ...baseBody, data: '{"nested":{"a":1}}' })
     )
-    const data = (await response.json()) as { success: boolean; error?: string }
+    const data = (await response.json()) as {
+      success: boolean
+      output: unknown
+      error?: string
+    }
 
+    expect(response.status).toBe(200)
     expect(data.success).toBe(false)
+    expect(data.output).toEqual({ id: null, fields: {} })
     expect(data.error).toContain('has no encoding for')
     expect(inputValidationMockFns.mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
   })
+
+  it('refuses a field that reuses a reserved $ parameter name', async () => {
+    const response = await POST(
+      createMockRequest('POST', { ...baseBody, data: '{"$table":"other_table"}' })
+    )
+    const data = (await response.json()) as { success: boolean; error?: string }
+
+    expect(response.status).toBe(200)
+    expect(data.success).toBe(false)
+    expect(data.error).toContain('reserved')
+    expect(inputValidationMockFns.mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
+  })
 })
 
 describe('field projection', () => {

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

@@ -1,5 +1,6 @@
 import { createLogger } from '@sim/logger'
-import { toError } from '@sim/utils/errors'
+import { getErrorMessage, toError } from '@sim/utils/errors'
+import { truncate } from '@sim/utils/string'
 import { type NextRequest, NextResponse } from 'next/server'
 import { agiloftCreateRecordContract } from '@/lib/api/contracts/tools/agiloft'
 import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
@@ -69,13 +70,29 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
       })
     }
 
+    /**
+     * Encoded before the request is issued, not inside the request builder: an
+     * unencodable field is a permanent refusal, and a TypeError thrown from the
+     * builder would surface as a 500 the tool runner then retries.
+     */
+    let requestBody: string
+    try {
+      requestBody = buildCreateRecordBody(params, fieldValues)
+    } catch (error) {
+      return NextResponse.json({
+        success: false,
+        output: { id: null, fields: {} },
+        error: getErrorMessage(error, 'The data parameter contains a value Agiloft cannot encode'),
+      })
+    }
+
     const result = await executeEwRequest<AgiloftRecordResponse>(
       params,
       (base) => ({
         url: buildCreateRecordUrl(base),
         method: 'POST',
         headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
-        body: buildCreateRecordBody(params, fieldValues),
+        body: requestBody,
       }),
       async (response) => {
         const text = await response.text()
@@ -90,7 +107,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
           return {
             success: false,
             output: { id: null, fields: {} },
-            error: `Agiloft error ${response.status}: ${describeAgiloftError(text)}`,
+            error: `Agiloft error ${response.status}: ${describeAgiloftError(truncate(text, 300))}`,
           }
         }
 
@@ -110,7 +127,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
           return {
             success: false,
             output: { id: null, fields: {} },
-            error: `Agiloft accepted the create but returned no record ID, so the record may exist. Check the table before retrying - retrying creates a second record. Response: ${describeAgiloftError(text)}`,
+            error: `Agiloft accepted the create but returned no record ID, so the record may exist. Check the table before retrying - retrying creates a second record. Response: ${describeAgiloftError(truncate(text, 300))}`,
           }
         }
 

apps/sim/app/api/tools/agiloft/nlp_search/route.test.ts

@@ -155,9 +155,19 @@ describe('EWNLPSearch response', () => {
     )
 
     const response = await POST(createMockRequest('POST', baseBody))
-    const data = (await response.json()) as { success: boolean; error?: string }
+    const data = (await response.json()) as {
+      success: boolean
+      output: unknown
+      error?: string
+    }
 
+    /**
+     * A refusal Agiloft already settled must come back as a completed failure.
+     * A 500 would have the tool runner retry a search it has already declined.
+     */
+    expect(response.status).toBe(200)
     expect(data.success).toBe(false)
+    expect(data.output).toEqual({ records: [], totalCount: 0, truncated: false })
     expect(data.error).toContain('One has to specify $login, $password')
   })
 

apps/sim/app/api/tools/agiloft/nlp_search/route.ts

@@ -12,7 +12,7 @@ import {
   buildNlpSearchBody,
   buildNlpSearchUrl,
 } from '@/tools/agiloft/utils'
-import { executeEwRequest, readAlrestJson } from '@/tools/agiloft/utils.server'
+import { executeEwRequest, isAgiloftRefusal, readAlrestJson } from '@/tools/agiloft/utils.server'
 
 export const dynamic = 'force-dynamic'
 
@@ -62,6 +62,14 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
         body: buildNlpSearchBody(params),
       }),
       async (response) => {
+        /**
+         * EWNLPSearch is the one legacy `EW*` operation that answers in the JSON
+         * envelope the `alrest` surface uses — `{success, message, result}` with
+         * `result` as the record array — rather than `EWREST_` assignments. Its
+         * request half is form-encoded like the rest of the `EW*` surface, so
+         * the two halves deliberately use different conventions. Do not
+         * "correct" this to `parseEwRest` to match create.
+         */
         const returned = (await readAlrestJson<Record<string, unknown>[]>(response)) ?? []
         const records = returned.slice(0, AGILOFT_MAX_SEARCH_RECORDS)
 
@@ -84,6 +92,21 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
 
     return NextResponse.json(result)
   } catch (error) {
+    /**
+     * A refusal Agiloft already decided on is a final answer, not a transient
+     * fault, so it is reported in a 200 body like every sibling operation does.
+     * A 500 would have the tool runner retry a search Agiloft has already
+     * declined.
+     */
+    if (isAgiloftRefusal(error)) {
+      logger.warn(`[${requestId}] Agiloft refused the request`, { error: error.message })
+      return NextResponse.json({
+        success: false,
+        output: { records: [], totalCount: 0, truncated: false },
+        error: error.message,
+      })
+    }
+
     logger.error(`[${requestId}] Error running Agiloft NLP search:`, error)
 
     return NextResponse.json({ success: false, error: toError(error).message }, { status: 500 })

apps/sim/tools/agiloft/utils.server.ts

@@ -277,8 +277,11 @@ export async function executeAlrestRequest<R extends ToolResponse>(
 
 /**
  * Runs a single `/ewws/EW*` call. No login round-trip: that surface rejects the
- * bearer token and authenticates from the inline `$login`/`$password` already
- * present in the URL built by the caller.
+ * bearer token and authenticates from the inline `$login`/`$password` the
+ * caller puts on the request — in the query string for the operations that only
+ * accept parameters there, in the form-encoded body for the ones that take it
+ * (EWCreate, EWUpsert, EWNLPSearch), which keeps the password out of URLs and
+ * access logs.
  */
 export async function executeEwRequest<R extends ToolResponse>(
   params: AgiloftCredentials,

apps/sim/tools/agiloft/utils.test.ts

@@ -5,7 +5,6 @@ import { describe, expect, it } from 'vitest'
 import {
   agiloftAlrestBase,
   alrestDeleteRecordUrl,
-  alrestRecordCollectionUrl,
   alrestRecordUrl,
   alrestSearchUrl,
   buildAttachFileUrl,
@@ -52,7 +51,6 @@ describe('agiloftAlrestBase', () => {
 
 describe('alrest record routes', () => {
   it('builds collection, item, and search paths under the KB base', () => {
-    expect(alrestRecordCollectionUrl(BASE, 'contract')).toBe(`${BASE}/contract?lang=en`)
     expect(alrestRecordUrl(BASE, 'contract', ' 6342 ')).toBe(`${BASE}/contract/6342?lang=en`)
     expect(alrestSearchUrl(BASE, 'contract')).toBe(`${BASE}/contract/search?lang=en`)
   })
@@ -344,3 +342,47 @@ describe('multi-value field encoding', () => {
     expect(sent.getAll('contactMethod')).toEqual(['phone', '42', 'true'])
   })
 })
+
+describe('reserved parameter namespace', () => {
+  /**
+   * Record data reaches the body builders from workflow input, so a field named
+   * after a reserved parameter would append a second occurrence of it and let
+   * that data choose the table or the credentials.
+   */
+  it('refuses a field that reuses a reserved $ parameter name', () => {
+    expect(() => buildCreateRecordBody(baseParams, { $table: 'other_table' })).toThrow(TypeError)
+    expect(() => buildCreateRecordBody(baseParams, { $password: 'evil' })).toThrow(TypeError)
+    expect(() => buildUpsertRecordBody({ ...baseParams, match: 'id' }, { $KB: 'other' })).toThrow(
+      TypeError
+    )
+  })
+
+  it('leaves the reserved pairs single-valued for ordinary field data', () => {
+    const sent = new URLSearchParams(buildCreateRecordBody(baseParams, { contract_title1: 'X' }))
+    expect(sent.getAll('$table')).toEqual(['contract'])
+    expect(sent.getAll('$password')).toEqual([PLACEHOLDER_PASSWORD])
+  })
+})
+
+describe('pagination input', () => {
+  const nlpBase = {
+    instanceUrl: INSTANCE,
+    knowledgeBase: baseParams.knowledgeBase,
+    login: baseParams.login,
+    password: PLACEHOLDER_PASSWORD,
+    nlpQuery: 'Active NDAs',
+    fields: 'id',
+  }
+
+  it('drops a non-numeric or negative value rather than widening the search', () => {
+    const sent = new URLSearchParams(buildNlpSearchBody({ ...nlpBase, page: 'abc', limit: '-1' }))
+    expect(sent.has('page')).toBe(false)
+    expect(sent.has('limit')).toBe(false)
+  })
+
+  it('keeps a zero page, which is the documented first page', () => {
+    const sent = new URLSearchParams(buildNlpSearchBody({ ...nlpBase, page: '0', limit: '10' }))
+    expect(sent.get('page')).toBe('0')
+    expect(sent.get('limit')).toBe('10')
+  })
+})

apps/sim/tools/agiloft/utils.ts

@@ -60,10 +60,6 @@ function tableSegment(table: string): string {
   return encodeURIComponent(table.trim())
 }
 
-export function alrestRecordCollectionUrl(base: string, table: string): string {
-  return `${base}/${tableSegment(table)}?lang=${AGILOFT_LANG}`
-}
-
 export function alrestRecordUrl(base: string, table: string, recordId: string): string {
   return `${base}/${tableSegment(table)}/${encodeURIComponent(recordId.trim())}?lang=${AGILOFT_LANG}`
 }
@@ -232,13 +228,6 @@ export function encodeEwFormBody(fields: Array<[string, string]>): string {
     .join('&')
 }
 
-/**
- * Expands caller-supplied record data into form pairs, appending to `fields`.
- *
- * Multi-value fields are encoded as repeated key/value pairs, not as a joined
- * string. Objects have no documented encoding at all, and String()-ing one
- * silently writes "[object Object]" into the record.
- */
 /**
  * Renders one field value, refusing anything Agiloft has no encoding for.
  *
@@ -255,10 +244,30 @@ function encodeFieldValue(field: string, value: unknown): string {
   return String(value)
 }
 
+/**
+ * Expands caller-supplied record data into form pairs, appending to `fields`.
+ *
+ * Multi-value fields are encoded as repeated key/value pairs, not as a joined
+ * string.
+ *
+ * `$` opens Agiloft's reserved request-parameter namespace — `$table`, `$KB`,
+ * `$login`, `$password` — which this body has already set. Record data reaches
+ * here from workflow input, so a field carrying one of those names would append
+ * a second occurrence of a reserved parameter and let the caller's data decide
+ * which table the record lands in, or which credentials the call runs under.
+ * Whether the duplicate wins is Agiloft's parser's business, so the name is
+ * refused rather than sent.
+ */
 function pushRecordFields(fields: Array<[string, string]>, data: Record<string, unknown>): void {
   for (const [field, value] of Object.entries(data)) {
     if (value === undefined || value === null) continue
 
+    if (field.startsWith('$')) {
+      throw new TypeError(
+        `Field "${field}" uses Agiloft's reserved "$" parameter prefix, which record data cannot set. Rename the field.`
+      )
+    }
+
     if (Array.isArray(value)) {
       for (const entry of value) {
         if (entry === undefined || entry === null) continue
@@ -295,16 +304,17 @@ export function buildUpsertRecordBody(
  * EWCreate is the documented create operation. It answers with the ID of the
  * new record as an `EWREST_id` assignment, which is the only place that ID is
  * published. There is no documented JSON create that returns it.
- *
- * Every parameter travels in a form-encoded body: EWCreate lists
- * `application/x-www-form-urlencoded` as its supported Content-Type, it is one
- * of the operations that accept credentials in the body rather than the query
- * string, and a body has no request-line length ceiling on the record data.
  */
 export function buildCreateRecordUrl(base: string): string {
   return `${base}/ewws/EWCreate`
 }
 
+/**
+ * Every parameter travels in a form-encoded body: EWCreate lists
+ * `application/x-www-form-urlencoded` as its supported Content-Type, it is one
+ * of the operations that accept credentials in the body rather than the query
+ * string, and a body has no request-line length ceiling on the record data.
+ */
 export function buildCreateRecordBody(
   params: AgiloftBaseParams,
   data: Record<string, unknown>
@@ -442,6 +452,13 @@ export function buildNlpSearchUrl(base: string): string {
  * of the URL, access logs, and proxy traces. `field` repeats once per requested
  * field, matching Agiloft's multi-value encoding.
  */
+/** Keeps a pagination input only when it reads as a non-negative whole number. */
+function toPaginationValue(value?: string): string | undefined {
+  const trimmed = value?.trim()
+  if (!trimmed) return undefined
+  return /^\d+$/.test(trimmed) ? trimmed : undefined
+}
+
 export function buildNlpSearchBody(params: AgiloftNlpSearchParams): string {
   const fields: Array<[string, string]> = [
     ['$KB', params.knowledgeBase],
@@ -458,9 +475,16 @@ export function buildNlpSearchBody(params: AgiloftNlpSearchParams): string {
   /**
    * EWNLPSearch ignores the table and searches the whole knowledge base, so
    * pagination is the only bound a caller has on the result size.
+   *
+   * Both are documented as integers, and the block accepts them as free text,
+   * so a non-numeric value is dropped rather than forwarded — Agiloft ignores
+   * parameters it cannot read, which would silently widen the search instead of
+   * bounding it.
    */
-  if (params.page) fields.push(['page', params.page])
-  if (params.limit) fields.push(['limit', params.limit])
+  const page = toPaginationValue(params.page)
+  const limit = toPaginationValue(params.limit)
+  if (page !== undefined) fields.push(['page', page])
+  if (limit !== undefined) fields.push(['limit', limit])
 
   return encodeEwFormBody(fields)
 }