fix(agiloft): return the create record ID and authenticate natural language search - #6650
fix(agiloft): return the create record ID and authenticate natural language search#6650mzxchandra wants to merge 15 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Create Record no longer posts JSON to undocumented Natural Language Search sends credentials as form request parameters (not JSON payload keys), adds Shared changes: Reviewed by Cursor Bugbot for commit 07970d0. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThe PR repairs Agiloft record creation and natural-language search while hardening credential handling and non-idempotent write failures.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported response and log credential disclosures are addressed on the current head.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/tools/agiloft/create_record/route.ts | Migrates creation to EWCreate, validates returned IDs, distinguishes definite refusals from uncertain writes, and redacts caller-facing and logged errors. |
| apps/sim/app/api/tools/agiloft/nlp_search/route.ts | Sends authenticated form parameters to EWNLPSearch, normalizes result shapes, and safely handles and redacts upstream refusals. |
| apps/sim/tools/agiloft/utils.ts | Adds form-body builders, reserved-field validation, and credential-redaction helpers used by the changed routes. |
| apps/sim/tools/agiloft/utils.server.ts | Redacts credential-bearing response text before truncation and prevents automatic redirects on credentialed Agiloft requests. |
| apps/sim/blocks/blocks/agiloft.ts | Makes page and limit controls available for natural-language search and preserves execution-time parameter mapping. |
Reviews (11): Last reviewed commit: "docs: describe credential shapes instead..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@cursor review |
|
Fixed in 952cdff. The counterexample was right, and my round-2 reply was too narrow: I fixed the ordering in the create route but not in
const text = credentials ? redactAgiloftSecrets(rawText, credentials) : rawTextIt takes credentials only from the operations that send them on the request itself. The other alrest callers authenticate with a bearer token and cannot echo a password back, so they pass nothing and are unchanged. Chasing this turned up a worse one next to it. The regression test asserts that no prefix of the password survives, not just the whole value, since a prefix is exactly what the boundary produces: for (let cut = 6; cut < PLACEHOLDER_PASSWORD.length; cut++) {
expect(data.error).not.toContain(PLACEHOLDER_PASSWORD.slice(0, cut))
}113 tests pass, type-check clean, API contract audit passes. |
|
@cursor review |
|
@cursor review |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b769a15. Configure here.
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 242ef7c. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5c0e49c. Configure here.
…surface Extract the form-encoding already used by EWUpsert into a shared encodeEwFormBody helper plus a pushRecordFields expander, so the documented encodings live in one place: multi-value fields as repeated key/value pairs, and a TypeError for object values that would otherwise serialize as "[object Object]". Adds builders for EWCreate and EWNLPSearch on top of it. Both operations document application/x-www-form-urlencoded as a supported Content-Type and accept their parameters in the request body, which keeps credentials out of URLs, access logs, and proxy traces. No behavior change: EWUpsert produces the same body it did before.
…create as failed Create posted JSON to the alrest collection URL and read the new record's ID from result.id. The write landed but the ID was not there, so every create reported failure with no ID. A caller retrying that failure wrote another record, making each attempt another orphan. Move create onto EWCreate, the documented create operation: form-encoded body, and the new record's ID published as an EWREST_id assignment, which the existing parseEwRest already handles. EWCreate authenticates from its own body, so the login/logout pair is gone and a create is now one request instead of three, dropping two of Agiloft's one-second WSDelay waits. Every failure path now answers 200 with success: false. A non-2xx makes the tool runner retry, and a retried create writes a second record rather than converging on the first. When the write is accepted but no ID comes back, the error says the record may exist and that retrying duplicates it.
…ameters EWNLPSearch takes $KB, $login, and $password as request parameters. The connector sent them as members of a JSON payload instead, so Agiloft refused every call with "One has to specify $login, $password parameters" and the operation never worked. Send the whole request form-encoded, which the endpoint documents as a supported Content-Type and which keeps the password out of the URL. The field list repeats once per requested field, matching Agiloft's multi-value encoding. Response handling is unchanged: the documented envelope already matches what the route reads. Also make Page and Limit reachable for this operation. Both were already in the contract and the tool params, but their condition was pinned to Search Records, leaving them with no UI field. This search ignores the table and runs across the whole knowledge base, so pagination is the only bound a caller has on the result size. Drop the Limit output from natural language search: the response schema does not return it and nothing populated it.
Agiloft addresses fields and tables by logical names that often differ from the labels in its UI, and the ones that most often send a workflow to the wrong place are not discoverable from the block: attachments live on their own table, contract status is status_1a rather than wfstate, the contract title is autopopulated on create so it cannot locate a record you just wrote, reads want an explicit field list because a contract record carries several hundred columns, and natural language search ignores the table. Also regenerates the create record output description.
The guard that rejects unencodable field values only ran on the top-level value, so an object inside an array fell through to String() and wrote "[object Object]" into the record while reporting success. Extracting the render step means array entries get the same refusal a bare value does. Pre-existing in the upsert body builder, but it now sits on the create path too, which is the operation this branch is making trustworthy. Also fills the coverage gaps a diff audit turned up: invalid and non-object JSON in the data param, an empty field list for natural language search, a search that matched nothing, separator characters in an encoded value, and the block wiring that makes page and limit reachable for natural language search.
…le 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.
…firmed writes Adversarial review findings, two of which two independent reviewers raised. secureFetchWithPinnedIP replays the whole options object to a redirect's Location - same method, same body - and its stripAuthOnRedirect only removes the Authorization header. Moving credentials into the request body therefore made a 3xx from the instance POST the Agiloft username and password to whatever public host it named, and on a create it would re-send the write. The redirect target is screened for private addresses but is not held to the original host. Every Agiloft call that carries a credential or a token now refuses redirects outright; none of these operations redirect in normal use. A create that failed after the request was on the wire - a timeout, a reset, a refused redirect, an oversized body - still escaped to the outer handler and returned 500. That is the retryable status this operation exists to avoid, on precisely the paths where the write may already have committed. Those failures are now settled with the same do-not-retry warning, and the instance URL is resolved up front so a rejected URL stays a 400. The warning itself was being applied too widely: a typed Agiloft exception means the create was declined and nothing was written, so a corrected retry is safe. Only an unexplained missing ID leaves the write in doubt. The two now read differently. Also: describes the error before truncating rather than after, which was cutting the exception text out of the message it was meant to explain; refuses a record ID that is not a number, since it is chained straight into reads and updates; redacts the instance credentials from any relayed transport error; normalises a non-array search result that would otherwise throw past the handler as a 500; and logs the field names and creator login so the "check the table" instruction has something to search on.
Review finding. The credentials for these operations travel in the submitted form body, so an Agiloft error page or an intermediary that echoes request parameters hands them back in the response. The non-OK create branch and the natural language search refusal both relayed that text to the workflow caller untouched; only the transport-error path was redacting. Redaction now happens where the description is built, so every branch that relays upstream text is covered rather than each one remembering, and the helper moved to the shared utils since a second route needs it. Natural language search grows the same nested try create has, so the refusal branch can see the parsed parameters it needs to redact against.
…nconfirmed writes Review round 2. Redaction only replaced the raw credential strings, but the values are sent form-encoded, so an error page quoting the submitted parameters quotes the encoded spelling. A password with a space leaves as a%20b or a+b and sailed past a replace that only knew a b. All three spellings are now replaced, longest first. Redaction also ran after truncation on the create path, so clipping the text could cut through a credential and leave a prefix that no longer matched anything being replaced. The order is reversed and the reason recorded, since the two read as interchangeable and are not. The non-OK create branch always warned that the record might exist, including on a 4xx validation decline where Agiloft had refused the request and written nothing. That is the opposite of the rule this branch introduces: it told the caller not to retry a create that never happened. A 4xx carrying a typed exception is now reported as a definite refusal; a 5xx keeps the warning, because a server fault may have committed first. Redirects were refused on the calls carrying credentials in a body but not on executeAgiloftRequest's operation fetch or on logout, both of which send a Bearer token that secureFetchWithPinnedIP would replay to a redirect host. Those are the calls behind list tables and saved search.
…he body Review round 3. The create path redacts before truncating, but natural language search reads its response through readAlrestJson, which embeds its own truncated slice of the body in the error it throws. The route redacted that message afterwards, by which point a credential clipped at the 300-character boundary had already been reduced to a prefix that a full-value replace can never match. readAlrestJson now redacts while the text is whole, for the callers that send credentials on the request itself. The rest authenticate with a bearer token and cannot echo one back, so they pass nothing and are unchanged. Login had the same shape and no redaction at all, which is worse than the reported case: EWLogin posts the credentials in its form body, and all three of its failure paths relayed the response text. It now redacts before any of them run. The regression test asserts no prefix of the password survives, not just the whole value, since a prefix is what the boundary produces.
…-flight Review finding. The create route resolves the instance URL itself to keep a rejected host on the pre-flight side of the line, but executeEwRequest then resolved it a second time before sending. A DNS failure on that second lookup never sent the create, yet every throw out of the executor was reported as an unconfirmed write that must not be retried - telling the caller a record might exist when nothing had been transmitted. The resolved IP is now handed down, so there is exactly one resolution and everything after it is genuinely post-transmit. It also drops the duplicate DNS lookup the two-resolve arrangement was paying for.
Review round 4. The previous round redacted the login response before parsing it. A token is opaque base64, so a short credential can appear inside one by coincidence - a three-character password is near certain to - and redacting first rewrote the token, breaking bearer auth for every alrest operation. Parsing now stays on the raw text and only the failure messages use the redacted copy, which is the split the shared alrest reader already had. The post-transmit create handler recorded the Agiloft username in structured logs. It was added so the "check the table" instruction had something to search on, but the login is half of a credential pair and whoever reads that log already knows which account the block is configured with. It is gone, and the logged error message now goes through the same redaction as the one returned to the caller, since it can carry echoed upstream text. The regression test builds a token with the password inside it and asserts the token comes back byte for byte. The fixture is deliberately not JWT-shaped: a realistic header segment reads as a live credential to secret scanning.
…tput This branch reworded the create record fields output, which feeds the generated tool metadata. The only entry that changes is agiloft_create_record.
…elay Review finding. Create redacted both its log line and its response; natural language search redacted only the refusal branch, leaving the non-refusal path logging the raw error object and returning an unredacted message. Both routes now relay the same redacted string to the log and to the caller on every branch. The outermost handler could not redact at all, because the parsed body it would need is scoped to the try it is catching. Both routes now capture the credentials as they are parsed, so that handler can redact too. It is not reachable with upstream text today - it catches auth and contract validation, neither of which talks to Agiloft - but "unreachable today" is the kind of reasoning that stops being true without anyone noticing.
Review finding. The create path ran describeAgiloftError on the raw body and redacted the result. That helper strips tags and collapses whitespace, so a credential containing bracket or repeated-space characters was reshaped before redaction looked for it, and a fragment could still reach the caller. That is the fifth time these three steps have been ordered wrongly in this branch, in both directions, each time in a different file. The steps are now one function: callers hand over the raw body and get back a string that is safe to relay, and there is no correct way to compose them by hand. Tests cover both reshaping transforms - a password carrying angle brackets and one carrying a double space - and assert the helper still reduces a body to its typed exception message.
5c0e49c to
07970d0
Compare
1 participant
Summary
Two operations on the
agiloftblock were broken in ways the connector made unrecoverable for the caller. Both root causes are settled against Agiloft's published REST documentation rather than inferred.Create Record returned the new record's ID
agiloft_create_recordposted JSON to the undocumentedalrestcollection URL and read the new record's ID fromresult.id. The write landed but the ID was not there, so every create reported failure with no ID — and a caller retrying that failure wrote another record, making each attempt another orphan.Create now uses
EWCreate, the documented create operation: a form-encoded body, with the new record's ID published as anEWREST_idassignment that the existingparseEwRestalready handles.EWCreateauthenticates from its own body, so the login/logout pair is gone and a create is one request instead of three, dropping two of Agiloft's one-secondWSDelaywaits.Natural Language Search authenticates
EWNLPSearchtakes$KB,$login, and$passwordas request parameters. The connector sent them as members of a JSON payload, so Agiloft refused every call withOne has to specify $login, $password parametersand the operation never worked. The request is now form-encoded, which the endpoint documents as a supported Content-Type and which keeps the password out of the URL. Response handling was already correct and is unchanged.PageandLimitare now reachable for this operation. Both were already in the contract and the tool params, but their condition was pinned to Search Records, leaving them with no UI field. This search ignores the table and runs across the whole knowledge base, so pagination is the caller's only bound on result size.Write-safety hardening
The rule this PR is built on is that a create must never return a status that makes the caller retry, because a retried create duplicates a record in a customer's contract database. Review found three paths that still broke it, all now returning a settled failure with an explicit do-not-retry warning:
The warning is also applied more precisely: a typed Agiloft exception means the create was declined and nothing was written, so a corrected retry is safe. Only an unexplained missing ID leaves the write in doubt.
Security
secureFetchWithPinnedIPreplays the whole options object to a redirect'sLocation— same method, same body — and itsstripAuthOnRedirectonly removes theAuthorizationheader. Moving credentials into the request body therefore made a 3xx from the instance POST the Agiloft username and password to whatever public host it named, and on a create it would re-send the write. The redirect target is screened for private addresses but is not held to the original host. Every Agiloft call carrying a credential or token now refuses redirects; none of these operations redirect in normal use.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, as are objects nested inside a multi-value field, which previously wrote[object Object]into the record while reporting success.Documentation
Adds the field and table conventions that are not discoverable from the block: attachments live on their own table, contract status is
status_1arather thanwfstate, the contract title is autopopulated on create so it cannot locate a record you just wrote, reads want an explicit field list because a contract record carries several hundred columns, and natural language search ignores the table.Verification
Grounded in Agiloft's published REST documentation for
EWCreate,EWNLPSearch,EWSearch, andEWDelete. Not exercised against a live Agiloft instance — no credentials were available. The behavior asserted here is what those endpoints document, not what was observed on a running system.bun run type-checkcleanbun run check:api-validationpassedTest Coverage
Tests: 86 → 105 (+19 new) across the Agiloft suite.
Covered: the form-encoded
EWCreatebody and its single round trip; ID extraction from the documentedEWREST_id; multi-value fields as repeated pairs; refusal of object values, objects nested in arrays, and reserved$names; the no-ID warning and the definite-refusal wording; a non-numeric ID; a post-transmit transport failure settling rather than 500-ing; credential redaction in a relayed error; natural language search credentials as request parameters with the body asserted to be non-JSON; envelope mapping; an empty result; the truncation cap; and the block wiring for page/limit.Known gaps: the auth-401 and contract-validation-400 branches are untested on both routes (shared repo-wide pattern, untouched here).
Pre-Landing Review
13 findings from 5 specialists (testing, maintainability, security, performance, api-contract). 3 critical, all fixed. 7 informational fixed or auto-fixed, 3 skipped as out of scope:
create_record/route.test.tshas become a catch-all for the whole Agiloft surface.details: error.issueson the 400 path is not declared in the response schema (pre-existing, repo-wide).Adversarial Review
Claude and Codex reviewed independently. Both ranked the same finding first: post-transmit failures returning a retryable 500 on a non-idempotent write. Both are fixed, along with the redirect credential exposure, an error message that was truncated before being parsed (destroying the exception text it was meant to surface), a non-array search result that would throw past the handler, and a non-numeric record ID being chained downstream.
Not changed, noted for the record:
nullfield values are dropped rather than refused (pre-existing upsert behavior), anddatahas no maximum length.Scope Drift
Scope Check: CLEAN. Every commit touches only Agiloft tools, routes, block, and docs.
Plan Completion
34 of 36 plan items complete. The 2 remaining are the live-instance end-to-end checks, which cannot be run without instance access.
Out of scope
The
alrestsurface is essentially undocumented — the only published reference is a single cURL example.read_record,update_record,delete_record, andsearch_recordsstill ride it and are untouched here, so whether it honorssearch,page, andlimitis unverified. Attachment persistence was excluded by decision; it is a platform-wide property of the file-output pipeline, not an Agiloft defect.Test plan
🤖 Generated with Claude Code