fix(sixtyfour,enrow): stop nulling model-supplied struct, retry transient poll failures - #7261
fix(sixtyfour,enrow): stop nulling model-supplied struct, retry transient poll failures#7261waleedlatif1 wants to merge 8 commits into
Conversation
…ient poll failures Sixtyfour's block param mapper wrote `result.struct = params.leadStruct` unconditionally. On the agent path the model supplies the *tool* param name (`struct`), not the subBlock name (`leadStruct`), so the mapper's return — which is overlaid on the model's arguments — set the required `struct` to `undefined` and the call went out without it. Same for `enrich_company`. Every rename is now guarded on its source being present; a configured block value still wins because it is present. Enrow's find/verify polls threw on any non-2xx, so a single transient 5xx killed a job that was still running. Both polls now share `poll.ts`, which retries 429 and 5xx up to three times for the whole poll using `backoffWithJitter`/`parseRetryAfter`, charging the backoff against the existing 120s budget rather than extending it. Non-transient statuses still fail immediately. Also corrects the `ENROW_CREDIT_USD` docblock, which cited a $24/2,000-credit Starter plan that does not exist on Enrow's price list. The rate itself is left alone — see the PR body.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThe PR preserves model-supplied SixtyFour struct parameters and consolidates Enrow polling with bounded transient retries and deadline-aware response handling.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/blocks/blocks/sixtyfour.ts | Guards operation-specific remaps so absent canvas values cannot overwrite model-supplied tool parameters. |
| apps/sim/blocks/blocks/sixtyfour.test.ts | Exercises agent and canvas parameter transformation paths, including null, empty, false, and configured values. |
| apps/sim/tools/enrow/poll.ts | Implements shared deadline-bounded polling, transient retries, body cleanup, and timeout-aware decoding. |
| apps/sim/tools/enrow/find_email.ts | Delegates polling to the shared helper and returns an explicit failed ToolResponse when polling fails. |
| apps/sim/tools/enrow/verify_email.ts | Applies the same shared polling and explicit failure-response behavior to verification jobs. |
| apps/sim/tools/enrow/find_email.test.ts | Covers retry limits, Retry-After, wall-clock deadlines, body aborts, resource cleanup, and failed response propagation. |
| apps/sim/tools/enrow/hosting.ts | Corrects explanatory pricing and rate-limit documentation without changing runtime configuration. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Tool as Enrow Tool
participant Poll as pollEnrowJob
participant API as Enrow API
Caller->>Tool: Execute find/verify
Tool->>Poll: Poll submitted job id
loop Within 120-second budget
Poll->>API: GET result with deadline signal
alt 202 in progress
API-->>Poll: 202
else 429 or 5xx within retry cap
API-->>Poll: transient error
Poll->>Poll: bounded backoff
else 200 complete
API-->>Poll: result body
Poll-->>Tool: decoded result
else terminal error or deadline
Poll-->>Tool: throw poll failure
Tool-->>Caller: success: false with job id
end
end
Reviews (8): Last reviewed commit: "fix(enrow): return a failed tool respons..." | Re-trigger Greptile
There was a problem hiding this comment.
5 issues found across 7 files
Confidence score: 2/5
apps/sim/tools/enrow/poll.tsdoes not enforceMAX_POLL_TIME_MSwhile awaiting a hangingfetch, so an Enrow request can block beyond the polling budget — add a real deadline and abort each request when time expires.apps/sim/tools/enrow/poll.tsabandons response bodies on 202/retryable responses, which can retain pooled HTTP connections across polling jobs — cancel or drain the body before retrying.apps/sim/tools/enrow/poll.tscan stop up to 15 seconds early when a transient response arrives near the deadline, reducing the intended polling window — wait for the remaining budget before terminating.apps/sim/tools/enrow/find_email.test.tsdoes not verify Retry-After timing, andapps/sim/tools/enrow/hosting.tscites the wrong documentation endpoint; assert the delay and link the verifier documentation separately.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/tools/enrow/hosting.ts">
<violation number="1" location="apps/sim/tools/enrow/hosting.ts:13">
P3: The citation points to the email-finder endpoint, not the verifier documentation, so it does not substantiate the adjacent 0.25-credit claim. Link the verifier endpoint separately.</violation>
</file>
<file name="apps/sim/tools/enrow/poll.ts">
<violation number="1" location="apps/sim/tools/enrow/poll.ts:63">
P1: If an Enrow poll request hangs or exceeds the remaining budget, `await fetch` never rechecks `elapsed`, so `MAX_POLL_TIME_MS` does not bound this operation. Track a real deadline and abort each request when the remaining budget expires.</violation>
<violation number="2" location="apps/sim/tools/enrow/poll.ts:67">
P2: When a 202 or retryable response has a body, this loop abandons it without draining or cancelling the stream, which can retain pooled HTTP connections across polling jobs. Cancel the response body before each retry or polling `continue`.</violation>
<violation number="3" location="apps/sim/tools/enrow/poll.ts:77">
P2: When a transient response arrives late in the polling window, this check breaks before `delayMs` is slept, so polling can end up to 15 seconds before the 120-second deadline. Wait out the remaining budget before terminating instead of breaking on the projected elapsed value.</violation>
</file>
<file name="apps/sim/tools/enrow/find_email.test.ts">
<violation number="1" location="apps/sim/tools/enrow/find_email.test.ts:164">
P2: This test's name claims Retry-After is honored, but it never asserts the backoff delay. `sleep` is mocked to resolve immediately, so the test only proves a 429 is retried (already covered by the 5xx test) — a regression that ignores the Retry-After header and falls back to exponential backoff would still pass. Assert that the mocked `sleep` was called with the Retry-After-derived delay (parseRetryAfter('2') clamps to 2000ms), e.g. expect(sleep).toHaveBeenCalledWith(expect.any(Function)) won't work; instead capture sleep calls via vi.mocked and check the delay argument, or export/verify the computed delay.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
…e abandoned bodies Review follow-ups on the polling helper. The budget was accounted purely in intended sleep time, which `await fetch` never advances — a hung socket outlived the 120s window entirely. The loop now also carries a wall-clock deadline and caps each request with `AbortSignal.timeout(remaining)`, so a stalled connection is aborted at the window rather than after it. An abort surfaces as the window message; any other transport failure is rethrown unchanged. A transient response late in the window added the prospective backoff to `elapsed` and broke before sleeping it, ending polling up to 15s early and abandoning a job that was still running. The backoff is now clamped to the remaining budget and always slept, so the loop waits out the full window. 202 and to-be-retried responses had their bodies abandoned mid-stream, holding the socket out of the pool for the rest of the poll. Both paths now cancel the body before continuing. Tests: the Retry-After case now asserts the delay it claims to prove rather than only that a 429 is retried — dropping `readRetryAfterMs` from the backoff call turns it red. Added coverage for body release, the per-request abort signal, abort-to-window-message, transport-error passthrough, and the full-window wait. Also splits the finder and verifier doc citations on ENROW_CREDIT_USD; the 0.25-credit claim was hanging off the finder endpoint. The rate itself is unchanged.
|
Pushed 7a201a8 addressing all six threads; each has an individual reply and is resolved.
|
The backoff was clamped against `MAX_POLL_TIME_MS - elapsed`, but `elapsed` charges nothing for time spent inside `await fetch`. With slow polls the real clock runs ahead of it, so that term overstates the remainder and a wait sized against it lands past the deadline — up to 15s past, on a capped Retry-After. The poll interval had the same defect at a smaller scale, worth up to 3s. Both waits now go through one `remainingBudgetMs()` helper returning whichever of the two budgets has less left, so there is a single clamp rule rather than two that disagree. The request timeout keeps using the wall clock directly, which is the only bound that means anything to an in-flight socket. The new test drives a stubbed clock that the mocked `sleep` and the `fetch` stub both advance, so the second poll completes past the deadline while `elapsed` still believes 99s remain. It asserts the backoff is dropped rather than clamped to that phantom budget, and that no sleep begins after the deadline. Reverting the clamp to `MAX_POLL_TIME_MS - elapsed` turns it red.
|
Pushed 37b8101. The P1 on the backoff clamp was correct and is fixed — Went slightly wider than the suggested three-term All 7 threads across both rounds are replied to and resolved. Gates green: |
|
Thanks for the follow-up. The P1 backoff-budget issue is addressed, and factoring both the retry backoff and poll interval through |
|
@greptile review Requesting a full re-review so the summary reflects Both waits now route through a single All 7 threads across both rounds are replied to and resolved. |
The comment justifying `requestsPerMinute: 60` claimed "Enrow rate limit is ~50 req/s". Enrow documents 10 req/s per API key on every POST endpoint (https://docs.enrow.io/rate-limits) — 600/min, not 3,000. The cap itself stays correct: 60/min is conservative against either figure, and the reason for it (not bursting into the limit while a job polls) is unchanged. Only the number a future reader would size the cap against was wrong — the same false-citation class this branch already fixes on ENROW_CREDIT_USD.
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 4/5
- In
apps/sim/tools/enrow/poll.ts,isTransientStatustreats statuses such as 600 as transient and retries them, which can delay failure handling or cause unnecessary polling; restrict the check to the 500–599 range.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/tools/enrow/poll.ts">
<violation number="1" location="apps/sim/tools/enrow/poll.ts:25">
P2: When the poll endpoint or an intermediary returns a status outside the 5xx range, such as 600, `isTransientStatus` retries it because it checks only `>= 500`. Restrict the range to 500–599 so only 5xx responses and 429 are retried.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
`isTransientStatus` checked `status >= 500`, so a 6xx would have been retried as if it were a server error. A status is a three-digit field: the `Response` constructor refuses anything outside 200-599, but a status parsed off the wire is not built that way, so a misbehaving intermediary can surface one. A 6xx is not something a later poll recovers from, so it now fails fast with the status attached rather than burning the poll's retry budget on it. Test asserts a single call for a 600; verified red against the old predicate.
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
apps/sim/tools/enrow/hosting.tsdoes not rate-limit repeated polling requests, so concurrent Enrow calls can exceed the provider’s 60/minute limit and create request bursts — apply the limiter to each poll request.apps/sim/blocks/blocks/sixtyfour.tscan emitstruct: nullfor an untouched required subBlock, causing the mapper to send an invalid required parameter; treatnulland cleared empty strings as absent in both struct paths.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/tools/enrow/hosting.ts">
<violation number="1" location="apps/sim/tools/enrow/hosting.ts:48">
P2: During concurrent Enrow calls, this setting does not cap provider requests at 60/min or prevent polling bursts: the limiter counts only initial tool executions, not the repeated poll requests. Apply the limiter to each provider request or document this as a per-workspace execution cap rather than a provider-request safeguard.</violation>
</file>
<file name="apps/sim/blocks/blocks/sixtyfour.ts">
<violation number="1" location="apps/sim/blocks/blocks/sixtyfour.ts:242">
P2: When an untouched required struct subBlock resolves to `null`, this guard still writes `struct: null`, so the mapper sends an invalid required parameter. Treat `null` (and cleared empty strings) as absent in both struct mappings.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
The guard added earlier tested `!== undefined`, which is not enough. The serializer stores every untouched subBlock as `params[id] ?? null`, so an unfilled field arrives as `null` and a cleared one as `''` — both pass `!== undefined` and get written straight over the model's tool-call argument. `struct: null` reaches the tool as an invalid required param; `struct: ''` fails its `JSON.parse` with "struct must be valid JSON". Replaced with a `present()` helper testing all three, which is the same guard `trigger_dev`'s `scoped()` and the `enrow` mapper already use. Applied to the switches too, so `Boolean(null)` can no longer force `false` over a `true` the model sent, while a switch the user actually turned off still forwards as `false`. Also corrects what the hosted-key rate limit claims to do. The limiter is consulted once per tool execution, not per outbound request, so it never throttled the poll loop's GETs and the old comment's "avoid bursting into the limit during polling" was wrong. It is accurate as a per-workspace execution cap: each execution issues exactly one POST, and Enrow's documented 10 req/s applies to POST endpoints only, so 60/min sits an order of magnitude under the ceiling. Four tests, each verified red against the weaker guard.
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 3/5
apps/sim/tools/enrow/poll.ts: If headers arrive before the timeout but the response body stalls,response.json()can reject outside the current abort handling, allowing the post-processor to return an unintended result; extend timeout/abort handling through body parsing and cover the stalled-body case.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/tools/enrow/poll.ts">
<violation number="1" location="apps/sim/tools/enrow/poll.ts:149">
P1: Handle timeout aborts while reading the response body. When headers arrive before the deadline but the body does not, `response.json()` rejects outside the current abort handler, allowing the post-processor to return the original result with null fields instead of reporting a polling timeout.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
`AbortSignal.timeout` fires against the whole exchange, so a 200 whose headers arrive just inside the window can still have its body aborted mid-read. That rejection surfaces at `response.json()`, outside the try/catch guarding the `fetch`, so the raw `TimeoutError` escaped — naming neither Enrow nor the window it exhausted. It now takes the same abort path and ends the poll with this module's own window error. The error-body read gets the same treatment from the other direction: a body that cannot be read no longer replaces a terminal failure with an unrelated transport error. The status is the diagnostic and it now survives, with `<unreadable body>` standing in for the text. Both tests verified red against the unguarded reads.
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
- In
apps/sim/tools/enrow/poll.ts, a timeout while reading a 200 response can restore the original successful submission result, returningsuccess: truewith incomplete null fields; handle the polling timeout so incomplete results are not reported as successful. - In
apps/sim/tools/enrow/hosting.ts, 429/503 retries can re-acquire a key and issue up to three hosted POSTs for one execution, so the comment should distinguish the 60-executions/min admission limit from retry behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/tools/enrow/hosting.ts">
<violation number="1" location="apps/sim/tools/enrow/hosting.ts:50">
P3: When Enrow returns 429/503, `executeTool` retries the hosted POST up to three times and may re-acquire a key, so one execution can issue multiple POSTs. Update this comment to distinguish the 60 executions/min admission cap from actual provider request volume.</violation>
</file>
<file name="apps/sim/tools/enrow/poll.ts">
<violation number="1" location="apps/sim/tools/enrow/poll.ts:125">
P1: When the 200 response body times out, `pollEnrowJob` throws and the executor restores the original successful submission result, so the tool returns `success: true` with incomplete null fields. Catch the polling-window error in each Enrow `postProcess` and return a failed tool response instead of allowing the exception to escape.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
A throw out of `postProcess` never reaches the user. `executeTool` wraps every `postProcess` call in a catch that logs and then restores the pre-`postProcess` result — which for these tools is the *submit* response: `success: true` with every result field null. So a poll that timed out, exhausted its retries, or hit a terminal status was reported as a successful lookup that simply found nothing. Both Enrow tools now catch the poll failure and return it as `success: false` with the message and the job id preserved. Returning rather than throwing also stops the hosted-key cost hook, which is gated on `finalResult.success`, from billing an execution that produced no result. The eleven existing failure-path tests asserted the throwing contract and were bypassing the executor wrapper, which is exactly why this went unnoticed; they now assert the returned failure. One test pins the whole shape — `success: false`, the error, and the null-field output — so the silent-success regression cannot come back. Also corrects the rate-limit comment again: `executeTool` retries an upstream 429/503 and can re-acquire a key, so an admitted execution can issue more than one POST. The 60/min figure is an admission cap, and even at that retry ceiling it stays an order of magnitude under Enrow's documented 600/min.
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
|
Closing for now — not because of a defect. This batch grew to 17 PRs across ~700 changed call sites, and we would rather revisit it as smaller, independently testable pieces than merge this much at once. Nothing here is lost: the branch |
Three reported defects across the SixtyFour and Enrow integrations. Two were real and are fixed here; the third is real but the corrected value is not derivable from public sources, so the number is left alone and only its (false) citation is corrected.
1. SixtyFour nulled a model-supplied required field — VERIFIED, FIXED
SixtyfourBlock.tools.config.paramswrote two renames unconditionally:On the canvas path that is fine —
leadStructis a real subBlock. On the agent path it is not.transformBlockTool(apps/sim/providers/utils.ts) builds aparamsTransformthat runsresult = { ...result, ...blockParamsFn(result) }over the model's tool-call arguments, and the LLM schema is derived fromtoolConfig.params, which names the fieldstruct, notleadStruct. So the model sendsstruct, the mapper reads aleadStructthat was never there, and writesstruct: undefinedstraight over the model's value.structisrequired: trueon both tools, so the request went out toapi.sixtyfour.aiwith the required field missing.Fix: guard every rename on its source being present. A configured block value still wins, because it is present.
Proof is
apps/sim/blocks/blocks/sixtyfour.test.ts, which drives the realtransformBlockToolwith the real block and tool configs — not a hand-rolled imitation of the agent path.2. Enrow aborted a polling job on any transient 5xx — VERIFIED, FIXED
Both
enrow_find_emailandenrow_verify_emailpolled withif (!pollResponse.ok) throw. A single 500 ended a job that was still running server-side — and Enrow documents 500 as a retrieval failure on the result endpoint ("could not retrieve single search results"), and documents that retrieving a result consumes no credits, so retrying is both free and correct.Both polls now share
apps/sim/tools/enrow/poll.ts, which:backoffWithJitter(attempt, parseRetryAfter(header))from@sim/utils/retry, honouringRetry-AfterMAX_POLL_TIME_MSbudget rather than extending it, so worst-case wall clock is unchanged3. Enrow hosted-credit rate cites a plan that does not exist — VERIFIED, NUMBER NOT CHANGED
ENROW_CREDIT_USD = 0.012was documented as "Enrow's Starter plan is $24/month for 2,000 finder credits/month". No such plan exists. Enrow's published monthly tiers (https://enrow.io/pricing) are:plus a 40% annual discount.
0.012matches none of them, monthly or annual.The number is unchanged and this PR does not alter anyone's bill. Which rate is correct depends entirely on the plan the hosted
ENROW_API_KEY_*keys are actually enrolled on, which is not a public fact. Only the docblock is corrected, to state the real price list and to say plainly that the rate corresponds to no published tier.Billing exposure if someone later "corrects" it from the price list alone:
To pin this correctly I need one of: the Enrow plan on the hosted account (billing portal or invoice), or the contracted per-credit rate if it is a Custom tier. Until then, changing the number is a coin flip in both directions and is deliberately not done here.
Testing
bun run lint,bun run check:audits(39/39),check-block-registry, andtype-checkare all clean.tool-metadata:checkpasses — no params/outputs changed, so no artifact regeneration was needed.Every fix has a test that was watched fail against the pre-fix code and pass after:
sixtyfour.tsputs both agent-path assertions red (expected undefined to be '{"website":"Company website URL"}') while the four canvas-path tests stay green — i.e. the tests pin the agent path specifically, not the mapper's shapeEnrow find-email poll error: 503,expected 4 calls, got 1, and the same on the verify poll), while the existing 202/401/window tests stay green — the 4xx no-retry test passes both before and after, which is the pointFollow-up: second false citation in the same file (de50aca)
A validation audit against Enrow's published API docs found the rate-limit comment in the same
hosting.tswas wrong in the same wayENROW_CREDIT_USD's was — it claimed "Enrow rate limit is ~50 req/s". Enrow documents 10 req/s per API key on every POST endpoint (rate-limits) — 600/min, not 3,000.requestsPerMinute: 60is unchanged and stays correct: it is conservative against either figure, and the reason for it (not bursting into the limit while a job polls) is untouched. Only the number a future reader would size the cap against was wrong.Two audit findings worth recording
The retry design matches Enrow's own guidance exactly. The rate-limit page recommends "exponential backoff… default maximum of 3 retries" and shows 1s / 2s / 4s. This PR implements precisely that —
MAX_TRANSIENT_RETRIES = 3withbackoffWithJitteratbaseMs: 1000. The retryable set is confirmed too: 429 is documented there, 500 "Could not retrieve single search results" is documented on the result endpoints, and 400 ("id missing in the URL query string") is correctly not retried. Critically, both result endpoints state verbatim that "Retrieving a result does not consume credits — only the verification itself does", so a retry is provably free.SixtyFour's
struct: the prose docs and the OpenAPI schema disagree. The company-intelligence prose listsstructas optional, but the OpenAPI schema lists it underrequired. The tool'srequired: trueis correct and the prose is wrong — which matters, becausestructbeing required is exactly what made the unguardedresult.struct = undefinedoverwrite a data-loss bug rather than a cosmetic one.Audit also confirmed no other subBlock-id-vs-tool-param mismatch in that mapper: all 20 block inputs map 1:1 to tool param names except six deliberate remaps (
emailInput→email,phoneInput→phone,leadStruct/companyStruct→struct,companyLeadStruct→leadStruct,*ResearchPlan→researchPlan), and every one is now guarded.