Skip to content

Commit 41d2917

Browse files
committed
fix(pi): correct search normalization edge cases and the budget's stated scope
Follow-ups from review of the web-search work. Each fix lands in both the host adapter (`normalize.ts`) and the Create PR sandbox copy (`extension-source.ts`), with the extension test asserting the two produce byte-identical envelopes. - `usableUrl` was the one provider-controlled field not whitespace-bounded: title/snippet/date all go through `collapseWhitespace`, `url` only trimmed. Up to 2048 chars of newlines and control characters could ride into the envelope. Dropped rather than collapsed — `url` must stay byte-exact to stay resolvable, so collapsing would emit a different, still-dead link, and a URL carrying raw whitespace is already malformed under RFC 3986. - `numResults: null` (or `''`, or `[]`) returned 1 result, not the documented default of 5: `Number(null)` is a finite 0, so the clamp floor won rather than the default. Only a real number or a non-blank numeric string now counts as the model having asked for a count. - Envelope truncation was silent. When results were dropped to fit the 50 KB ceiling the model read the short list as the complete answer. It now carries a message saying so, and the message is inside what gets measured so the note cannot push a truncated envelope back over the ceiling. - The budget is per *block execution*, not per workflow run: the counter lives in the tool spec and both adapters build a fresh one per execution, so a Pi block inside a Loop gets the full allowance every iteration. The constant, the agent-facing message, and the docs all claimed "per run". Renamed to `PI_SEARCH_MAX_CALLS_PER_EXECUTION` and corrected the wording rather than tightening the cap, since a shared ceiling would fail late iterations of a legitimate fan-out. - The Search API Key tooltip promised "switching providers clears this field". That clear is driven through the collaborative editor setter, so a workflow imported, forked, or updated via the API keeps the previous provider's key — exactly the case where sending it to a new vendor matters. Docs also gain a warning that Create PR hands both the model key and the search key to the agent as environment variables, which Pi copies into every bash child. That matters most for Settings > BYOK keys: those are workspace-scoped, only admins can manage them, and the API only ever returns them masked — yet anyone who can run a Pi block in Create PR mode can read the raw value.
1 parent c6d0ad9 commit 41d2917

9 files changed

Lines changed: 207 additions & 35 deletions

File tree

apps/docs/content/docs/en/workflows/blocks/pi.mdx

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,15 @@ Your key for the chosen provider. On hosted Sim it is optional for Local Dev and
6565

6666
### Internet Search
6767

68-
Off by default. Pick a provider — **Exa**, **Serper**, **Parallel AI**, or **Firecrawl** — and the agent gains a single `web_search` tool that returns a handful of results, each with a title, URL, snippet, and (where the provider reports one) a publication date. It works the same way in all three modes, and it is the agent's only network access in Review Code. The tool accepts at most 20 calls per run, which bounds accidental tool loops and the quota one run can consume.
68+
Off by default. Pick a provider — **Exa**, **Serper**, **Parallel AI**, or **Firecrawl** — and the agent gains a single `web_search` tool that returns a handful of results, each with a title, URL, snippet, and (where the provider reports one) a publication date. It works the same way in all three modes, and it is the agent's only network access in Review Code. The tool accepts at most 20 calls **per block execution**, which bounds accidental tool loops. A Pi block inside a Loop or Parallel gets that allowance again on every iteration, so bound the iteration count too if you care about what a single workflow run can spend.
6969

70-
Search always uses **your own key** for the selected provider, never a Sim-hosted one, because Create PR places the key inside the coding sandbox. Enter it in **Search API Key** or store it in **Settings → BYOK**; the run fails with a setup error before any sandbox is created when neither is present. Switching providers clears the key field in the editor, so re-enter the key that belongs to the provider you picked.
70+
Search always uses **your own key** for the selected provider, never a Sim-hosted one, because Create PR places the key inside the coding sandbox. Enter it in **Search API Key** or store it in **Settings → BYOK**; the run fails with a setup error before any sandbox is created when neither is present. Changing the provider in the editor clears the key field, so re-enter the key that belongs to the provider you picked — a workflow you import, fork, or update through the API keeps whatever key was saved, so check it there.
71+
72+
<Callout type="warn">
73+
**Create PR exposes both keys to the agent.** Create PR runs the model client and the search client *inside* the sandbox, so the model key and the search key reach it as environment variables — and Pi copies its own environment into every shell command it runs. Your prompt, or instructions injected through the contents of the cloned repository, can therefore read either key and write it anywhere the agent can reach, including into the pull request itself. Sim strips verbatim key text out of run output, but that does not stop an agent that encodes the value first.
74+
75+
This matters most for keys stored in **Settings → BYOK**, which belong to the workspace rather than to you. Sim otherwise only ever displays those masked, and only workspace admins can add or remove them — but anyone who can run a Pi block in Create PR mode can read the raw value. When that is not acceptable for a given key, put a separate key on the block's own **Search API Key** field instead of relying on the BYOK fallback, and scope it to something you are willing to rotate.
76+
</Callout>
7177

7278
Results are third-party data. The agent is instructed to treat them as quoted evidence and never to follow instructions found inside them — the same posture Pi takes toward repository contents.
7379

apps/sim/blocks/blocks/pi.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,8 +166,12 @@ export const PiBlock: BlockConfig<PiResponse> = {
166166
paramVisibility: 'user-only',
167167
connectionDroppable: false,
168168
placeholder: 'Falls back to the key stored in Settings > BYOK',
169+
// Scoped to the editor on purpose: the clear-on-switch is driven by `dependsOn` through the
170+
// collaborative setter, so a workflow imported, forked, or updated through the API keeps
171+
// whatever key was stored. Promising an unconditional clear would be wrong in exactly the
172+
// case where sending the previous provider's key to a new vendor actually matters.
169173
tooltip:
170-
'Key for the selected search provider. Switching providers clears this field, so re-enter the key for the provider you picked.',
174+
'Key for the selected search provider. Changing the provider in the editor clears this field, so re-enter the key for the one you picked. Imported or API-updated workflows keep the saved key — check it belongs to the selected provider.',
171175
condition: getSearchApiKeyCondition(),
172176
dependsOn: ['searchProvider'],
173177
},

apps/sim/executor/handlers/pi/cloud-review-tools.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ const REVIEW_TOOLS_COMMAND = `python3 ${REVIEW_TOOLS_SCRIPT_PATH}`
1616
const REVIEW_TOOL_TIMEOUT_MS = 30_000
1717
/**
1818
* Both ceilings bound `runOperation`, i.e. sandbox traffic only. Optional web search is registered
19-
* separately by the review backend and counts against its own `PI_SEARCH_MAX_CALLS_PER_RUN` instead.
19+
* separately by the review backend and counts against its own
20+
* `PI_SEARCH_MAX_CALLS_PER_EXECUTION` instead.
2021
*/
2122
const MAX_TOOL_CALLS = 200
2223
const MAX_TOOL_OUTPUT_BYTES = 5_000_000

apps/sim/executor/handlers/pi/search/extension-source.test.ts

Lines changed: 52 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,13 @@ import {
2222
extractPiSearchRecords,
2323
normalizePiSearchRecords,
2424
PI_SEARCH_BUDGET_MESSAGE,
25-
PI_SEARCH_MAX_CALLS_PER_RUN,
25+
PI_SEARCH_DEFAULT_RESULTS,
26+
PI_SEARCH_MAX_CALLS_PER_EXECUTION,
27+
PI_SEARCH_MAX_SNIPPET_LENGTH,
28+
PI_SEARCH_MAX_TITLE_LENGTH,
2629
PI_SEARCH_TIMEOUT_MS,
2730
PI_SEARCH_TOOL_NAME,
31+
PI_SEARCH_TRUNCATED_MESSAGE,
2832
serializePiSearchEnvelope,
2933
} from '@/executor/handlers/pi/search/normalize'
3034

@@ -182,19 +186,19 @@ describe('provider requests', () => {
182186
expect(fetchMock).not.toHaveBeenCalled()
183187
})
184188

185-
it('enforces the same per-run budget as the host adapter, per extension load', async () => {
189+
it('enforces the same per-execution budget as the host adapter, per extension load', async () => {
186190
// A fresh Response per call: a body stream can only be read once.
187191
fetchMock.mockImplementation(() => jsonResponse({ results: [] }))
188192
const tool = register('exa')
189193

190-
for (let call = 0; call < PI_SEARCH_MAX_CALLS_PER_RUN; call++) {
194+
for (let call = 0; call < PI_SEARCH_MAX_CALLS_PER_EXECUTION; call++) {
191195
await tool.execute('call-1', { query: `pi ${call}` })
192196
}
193197

194198
await expect(tool.execute('call-1', { query: 'one too many' })).rejects.toThrow(
195199
PI_SEARCH_BUDGET_MESSAGE
196200
)
197-
expect(fetchMock).toHaveBeenCalledTimes(PI_SEARCH_MAX_CALLS_PER_RUN)
201+
expect(fetchMock).toHaveBeenCalledTimes(PI_SEARCH_MAX_CALLS_PER_EXECUTION)
198202
// A fresh load is a fresh sandbox run, so the count starts over.
199203
await expect(register('exa').execute('call-1', { query: 'fresh run' })).resolves.toBeDefined()
200204
})
@@ -286,6 +290,50 @@ describe('normalization parity with the host adapter', () => {
286290
const result = await register('firecrawl').execute('call-1', { query: 'pi' })
287291
expect(JSON.parse(result.content[0].text).results).toHaveLength(1)
288292
})
293+
294+
// `url` is dropped rather than whitespace-collapsed in both copies; a divergence here would let
295+
// the sandbox hand the agent a link the host modes would have refused.
296+
it('drops links carrying whitespace or control characters, as the host adapter does', async () => {
297+
const records = [
298+
{ title: 'Space', url: 'https://example.com/a b' },
299+
{ title: 'Newline', url: 'https://example.com/a\nb' },
300+
{ title: 'Nul', url: 'https://example.com/a\u0000b' },
301+
{ title: 'Del', url: 'https://example.com/a\u007fb' },
302+
{ title: 'Kept', url: 'https://example.com/a-b_c%20d' },
303+
]
304+
fetchMock.mockResolvedValue(jsonResponse({ results: records }))
305+
306+
const result = await register('exa').execute('call-1', { query: 'pi', numResults: 10 })
307+
expect(result.content[0].text).toBe(
308+
serializePiSearchEnvelope(normalizePiSearchRecords('exa', records, 10))
309+
)
310+
expect(JSON.parse(result.content[0].text).results).toHaveLength(1)
311+
})
312+
313+
it('treats a blank result count as absent, as the host adapter does', async () => {
314+
fetchMock.mockResolvedValue(jsonResponse({ results: [] }))
315+
316+
await register('exa').execute('call-1', { query: 'pi', numResults: null })
317+
expect(JSON.parse(fetchMock.mock.calls[0][1].body).numResults).toBe(PI_SEARCH_DEFAULT_RESULTS)
318+
})
319+
320+
it('says so when results were dropped to fit the envelope, as the host adapter does', async () => {
321+
const records = Array.from({ length: 10 }, (_, i) => ({
322+
title: '\u898b'.repeat(PI_SEARCH_MAX_TITLE_LENGTH),
323+
url: `https://example.com/${i}?${'q'.repeat(1500)}`,
324+
text: '\u6f22'.repeat(PI_SEARCH_MAX_SNIPPET_LENGTH),
325+
}))
326+
fetchMock.mockResolvedValue(jsonResponse({ results: records }))
327+
328+
const result = await register('exa').execute('call-1', { query: 'pi', numResults: 10 })
329+
const parsed = JSON.parse(result.content[0].text)
330+
331+
expect(parsed.results.length).toBeLessThan(records.length)
332+
expect(parsed.message).toBe(PI_SEARCH_TRUNCATED_MESSAGE)
333+
expect(result.content[0].text).toBe(
334+
serializePiSearchEnvelope(normalizePiSearchRecords('exa', records, 10))
335+
)
336+
})
289337
})
290338

291339
describe('failure handling', () => {

apps/sim/executor/handlers/pi/search/extension-source.ts

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {
1616
EXA_MAX_TEXT_CHARACTERS,
1717
PI_SEARCH_BUDGET_MESSAGE,
1818
PI_SEARCH_DEFAULT_RESULTS,
19-
PI_SEARCH_MAX_CALLS_PER_RUN,
19+
PI_SEARCH_MAX_CALLS_PER_EXECUTION,
2020
PI_SEARCH_MAX_DATE_LENGTH,
2121
PI_SEARCH_MAX_ENVELOPE_BYTES,
2222
PI_SEARCH_MAX_QUERY_LENGTH,
@@ -31,6 +31,7 @@ import {
3131
PI_SEARCH_TOOL_DESCRIPTION,
3232
PI_SEARCH_TOOL_NAME,
3333
PI_SEARCH_TOOL_PARAMETERS,
34+
PI_SEARCH_TRUNCATED_MESSAGE,
3435
} from '@/executor/handlers/pi/search/normalize'
3536

3637
/**
@@ -70,8 +71,9 @@ const MAX_DATE_LENGTH = ${PI_SEARCH_MAX_DATE_LENGTH}
7071
const MAX_URL_LENGTH = ${PI_SEARCH_MAX_URL_LENGTH}
7172
const MAX_ENVELOPE_BYTES = ${PI_SEARCH_MAX_ENVELOPE_BYTES}
7273
const NO_RESULTS_MESSAGE = ${JSON.stringify(PI_SEARCH_NO_RESULTS_MESSAGE)}
74+
const TRUNCATED_MESSAGE = ${JSON.stringify(PI_SEARCH_TRUNCATED_MESSAGE)}
7375
const EXA_MAX_TEXT_CHARACTERS = ${EXA_MAX_TEXT_CHARACTERS}
74-
const MAX_CALLS_PER_RUN = ${PI_SEARCH_MAX_CALLS_PER_RUN}
76+
const MAX_CALLS_PER_RUN = ${PI_SEARCH_MAX_CALLS_PER_EXECUTION}
7577
const BUDGET_MESSAGE = ${JSON.stringify(PI_SEARCH_BUDGET_MESSAGE)}
7678
7779
const PROVIDER_LABELS = {
@@ -146,10 +148,13 @@ function collapseWhitespace(value) {
146148
return value.replace(/\\s+/g, " ").trim()
147149
}
148150
151+
const URL_FORBIDDEN_CHARACTERS = /[\\s\\u0000-\\u001f\\u007f]/
152+
149153
function usableUrl(value) {
150154
const raw = asText(value).trim()
151155
if (!raw || raw.length > MAX_URL_LENGTH) return undefined
152156
if (!/^https?:\\/\\//i.test(raw)) return undefined
157+
if (URL_FORBIDDEN_CHARACTERS.test(raw)) return undefined
153158
return raw
154159
}
155160
@@ -221,7 +226,11 @@ function serializeEnvelope(results) {
221226
const kept = results.slice()
222227
for (;;) {
223228
const envelope =
224-
kept.length === 0 ? { results: [], message: NO_RESULTS_MESSAGE } : { results: kept }
229+
kept.length === 0
230+
? { results: [], message: NO_RESULTS_MESSAGE }
231+
: kept.length < results.length
232+
? { results: kept, message: TRUNCATED_MESSAGE }
233+
: { results: kept }
225234
const serialized = JSON.stringify(envelope)
226235
if (kept.length === 0 || new TextEncoder().encode(serialized).length <= MAX_ENVELOPE_BYTES) {
227236
return serialized
@@ -259,7 +268,7 @@ export default function (pi) {
259268
throw new Error("Unsupported search provider: " + provider)
260269
}
261270
262-
// Per extension load, which the CLI does once per run — same ceiling as the host adapter.
271+
// Per extension load, which the CLI does once per invocation — same ceiling as the host adapter.
263272
let calls = 0
264273
265274
pi.registerTool({
@@ -272,9 +281,15 @@ export default function (pi) {
272281
const rawQuery = params && typeof params.query === "string" ? params.query.trim() : ""
273282
if (!rawQuery) throw new Error("query is required")
274283
const query = rawQuery.slice(0, MAX_QUERY_LENGTH)
275-
const rawCount = Number(params ? params.numResults : undefined)
276-
const numResults = Number.isFinite(rawCount)
277-
? Math.min(MAX_RESULTS, Math.max(MIN_RESULTS, Math.floor(rawCount)))
284+
const rawCount = params ? params.numResults : undefined
285+
const parsedCount =
286+
typeof rawCount === "number"
287+
? rawCount
288+
: typeof rawCount === "string" && rawCount.trim()
289+
? Number(rawCount)
290+
: Number.NaN
291+
const numResults = Number.isFinite(parsedCount)
292+
? Math.min(MAX_RESULTS, Math.max(MIN_RESULTS, Math.floor(parsedCount)))
278293
: DEFAULT_RESULTS
279294
280295
calls += 1

apps/sim/executor/handlers/pi/search/normalize.test.ts

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
PI_SEARCH_MAX_SNIPPET_LENGTH,
1515
PI_SEARCH_MAX_TITLE_LENGTH,
1616
PI_SEARCH_TOOL_PARAMETERS,
17+
PI_SEARCH_TRUNCATED_MESSAGE,
1718
parsePiSearchArgs,
1819
serializePiSearchEnvelope,
1920
} from '@/executor/handlers/pi/search/normalize'
@@ -31,6 +32,20 @@ describe('parsePiSearchArgs', () => {
3132
)
3233
})
3334

35+
// `Number(null)` / `Number('')` / `Number([])` are a finite 0, so a naive clamp turns every one of
36+
// these into a single result — the opposite of the documented default.
37+
it('treats a blank or null result count as absent, not as zero', () => {
38+
for (const numResults of [null, undefined, '', ' ', []]) {
39+
expect(parsePiSearchArgs({ query: 'pi', numResults }).numResults).toBe(
40+
PI_SEARCH_DEFAULT_RESULTS
41+
)
42+
}
43+
})
44+
45+
it('accepts a numeric string the way it accepts a number', () => {
46+
expect(parsePiSearchArgs({ query: 'pi', numResults: '3' }).numResults).toBe(3)
47+
})
48+
3449
it('clamps the result count instead of failing the call', () => {
3550
expect(parsePiSearchArgs({ query: 'pi', numResults: 0 }).numResults).toBe(1)
3651
expect(parsePiSearchArgs({ query: 'pi', numResults: 500 }).numResults).toBe(
@@ -201,6 +216,25 @@ describe('normalizePiSearchRecords', () => {
201216
).toEqual([{ title: 'Kept', url: 'https://example.com/ok', snippet: '(no snippet)' }])
202217
})
203218

219+
// `url` is the one field that must stay byte-exact to stay resolvable, so unlike title/snippet it
220+
// is dropped rather than whitespace-collapsed — collapsing would emit a different, still-dead link.
221+
it('drops links carrying whitespace or control characters', () => {
222+
expect(
223+
normalizePiSearchRecords(
224+
'exa',
225+
[
226+
{ title: 'Space', url: 'https://example.com/a b' },
227+
{ title: 'Newline', url: 'https://example.com/a\nb' },
228+
{ title: 'Tab', url: 'https://example.com/a\tb' },
229+
{ title: 'Nul', url: 'https://example.com/a\u0000b' },
230+
{ title: 'Del', url: 'https://example.com/a\u007fb' },
231+
{ title: 'Kept', url: 'https://example.com/a-b_c%20d' },
232+
],
233+
10
234+
)
235+
).toEqual([{ title: 'Kept', url: 'https://example.com/a-b_c%20d', snippet: '(no snippet)' }])
236+
})
237+
204238
it('honors the requested limit', () => {
205239
const records = Array.from({ length: 9 }, (_, i) => ({
206240
title: `T${i}`,
@@ -244,6 +278,25 @@ describe('serializePiSearchEnvelope', () => {
244278
expect('publishedDate' in parsed.results[0]).toBe(false)
245279
})
246280

281+
it('says so when results were dropped, instead of reading as a complete answer', () => {
282+
const results = Array.from({ length: 10 }, (_, i) => ({
283+
title: '見'.repeat(PI_SEARCH_MAX_TITLE_LENGTH),
284+
url: `https://example.com/${i}?${'q'.repeat(1500)}`,
285+
snippet: '漢'.repeat(PI_SEARCH_MAX_SNIPPET_LENGTH),
286+
}))
287+
const parsed = JSON.parse(serializePiSearchEnvelope(results))
288+
289+
expect(parsed.results.length).toBeLessThan(results.length)
290+
expect(parsed.message).toBe(PI_SEARCH_TRUNCATED_MESSAGE)
291+
})
292+
293+
it('stays silent when everything fit', () => {
294+
const parsed = JSON.parse(
295+
serializePiSearchEnvelope([{ title: 'A', url: 'https://example.com/a', snippet: 'S' }])
296+
)
297+
expect('message' in parsed).toBe(false)
298+
})
299+
247300
// The per-field caps count UTF-16 units, so ten maximal results only exceed the byte ceiling once
248301
// the text is multi-byte — which is exactly the case a character-counted cap alone would miss.
249302
it('drops whole results to fit the byte ceiling, never truncating mid-string', () => {

0 commit comments

Comments
 (0)