|
34 | 34 | * normalization `fetch` performs — instead of string-matching the template |
35 | 35 | * output. String matching is exactly what let dot-segment traversal through: |
36 | 36 | * the template looks correct and the parser rewrites it afterwards. |
| 37 | + * |
| 38 | + * **Do not loosen these assertions into `toThrow()` or `toContain()`.** They |
| 39 | + * deliberately pin the *exact* encoded output and the *exact* error text, and |
| 40 | + * that precision is load-bearing rather than fussy: these guards live in |
| 41 | + * `tools/url-path.ts`, which belongs to a different PR that this branch is |
| 42 | + * rebased onto, so changes to them land *underneath* this suite without |
| 43 | + * touching a line of it. |
| 44 | + * |
| 45 | + * That has happened twice, and both times only the exact assertions noticed: |
| 46 | + * |
| 47 | + * - `safeUrlPath` stopped trimming each segment, so a storage key's interior |
| 48 | + * whitespace began surviving to the wire. Caught by an equality assertion on |
| 49 | + * the encoded output. |
| 50 | + * - Its empty-segment check narrowed from `!segment.trim()` to `!segment`, so |
| 51 | + * `a/ /b` became legal while `a//b` stayed rejected. Caught by an assertion |
| 52 | + * on the exact error text. |
| 53 | + * |
| 54 | + * A suite asserting only "it throws" would have gone green through both, and |
| 55 | + * the second one is a silent correctness change in either direction — permitting |
| 56 | + * `a//b` retargets the request at a different object, while rejecting `a/ /b` |
| 57 | + * makes a real object key permanently unreachable. The precision is what turns |
| 58 | + * an upstream edit into a failing test instead of a behaviour change nobody |
| 59 | + * sees. |
37 | 60 | */ |
38 | 61 | import { getErrorMessage } from '@sim/utils/errors' |
39 | 62 | import { expect, it } from 'vitest' |
@@ -300,6 +323,32 @@ export function discoverPathParams( |
300 | 323 | covered: PathParam[] |
301 | 324 | unbuildable: UnbuildableTool[] |
302 | 325 | undiscoverable: UndiscoverableParam[] |
| 326 | + /** |
| 327 | + * Every tool of the service that contributes no (tool, parameter) pair. |
| 328 | + * |
| 329 | + * Returned from the same sweep rather than recomputed, because discovery |
| 330 | + * builds a URL for every tool, every branch assignment and every declared |
| 331 | + * parameter — running it twice per suite doubled that for a list already in |
| 332 | + * hand. |
| 333 | + * |
| 334 | + * The enumeration is deliberately **looser** than `covered`. That list only |
| 335 | + * holds tools whose `request.url` is a function, since discovery has to call |
| 336 | + * it. Filtering the inventory the same way made three categories invisible to |
| 337 | + * *both* sides and let the pin pass vacuously — `box_create_folder` declares |
| 338 | + * `url` as a plain string and `box_upload_file` is an `InternalToolConfig` |
| 339 | + * with no `request`, so neither appeared in the covered pairs *or* in the |
| 340 | + * pinned set. Eleven tools across four services were invisible that way. So |
| 341 | + * this walks every export carrying the service id prefix, whatever shape its |
| 342 | + * request takes, and each suite pins the result exactly: a tool that gains a |
| 343 | + * guarded path parameter leaves the list, the assertion fails, and someone |
| 344 | + * looks. |
| 345 | + * |
| 346 | + * An `InternalToolConfig` stays here permanently — its URL is built in |
| 347 | + * `lib/internal/**`, which this suite cannot drive. Pinning it proves it is |
| 348 | + * accounted for, not that it is traversal-safe; that coverage comes from |
| 349 | + * direct unit tests on the helper it shares. |
| 350 | + */ |
| 351 | + withoutPathParams: string[] |
303 | 352 | } { |
304 | 353 | const covered: PathParam[] = [] |
305 | 354 | const unbuildable: UnbuildableTool[] = [] |
@@ -376,51 +425,14 @@ export function discoverPathParams( |
376 | 425 | } |
377 | 426 | } |
378 | 427 |
|
379 | | - return { covered, unbuildable, undiscoverable } |
380 | | -} |
381 | | - |
382 | | -/** |
383 | | - * Lists every tool of a service that contributes **no** (tool, parameter) pair. |
384 | | - * |
385 | | - * Each suite pins this set exactly, so a tool cannot leave path coverage |
386 | | - * unnoticed: if one ever gains a guarded path parameter it becomes a covered |
387 | | - * pair, the set shrinks, and the assertion fails until someone looks. |
388 | | - * |
389 | | - * The enumeration is deliberately **looser** than {@link discoverPathParams}. |
390 | | - * That function can only drive a tool whose `request.url` is a function, since |
391 | | - * it has to call it. Filtering the inventory the same way would make three |
392 | | - * whole categories invisible to *both* sides and let the pin pass vacuously — |
393 | | - * which is exactly what happened before: `box_create_folder` declares |
394 | | - * `url` as a plain **string**, and `box_upload_file` is an `InternalToolConfig` |
395 | | - * with no `request` at all, so neither appeared in the covered pairs *or* in |
396 | | - * the pinned set, and `toEqual(['box_search'])` passed precisely because they |
397 | | - * could not be seen. Eleven tools across four services were invisible that way. |
398 | | - * |
399 | | - * So this walks every export whose `id` carries the service prefix, whatever |
400 | | - * shape its request takes, and reports the ones no pair covers. The pinned list |
401 | | - * then states the real inventory, and each entry has to be justified as one of: |
402 | | - * |
403 | | - * - a genuinely static or query-string-only URL (`box_search`); |
404 | | - * - a `url` declared as a constant string (`box_create_folder`); |
405 | | - * - an `InternalToolConfig` whose URL is built in `lib/internal/**` |
406 | | - * (`supabase_storage_upload`). **These are outside what this suite can |
407 | | - * reach**, and are covered instead by direct unit tests on the helper they |
408 | | - * use — see the `encodeStoragePath` / `encodeStorageSegment` describes in |
409 | | - * `supabase/path_safety.test.ts`. |
410 | | - */ |
411 | | -export function toolsWithoutPathParams( |
412 | | - barrel: Record<string, unknown>, |
413 | | - idPrefix: string, |
414 | | - fixed: Record<string, unknown> = {} |
415 | | -): string[] { |
416 | | - const { covered } = discoverPathParams(barrel, idPrefix, fixed) |
417 | 428 | const withParams = new Set(covered.map(({ tool }) => tool.id)) |
418 | | - |
419 | | - return Object.values(barrel) |
| 429 | + const withoutPathParams = Object.values(barrel) |
420 | 430 | .map((value) => (value as { id?: unknown } | null)?.id) |
421 | 431 | .filter((id): id is string => typeof id === 'string' && id.startsWith(idPrefix)) |
422 | 432 | .filter((id) => !withParams.has(id)) |
423 | 433 | .sort() |
| 434 | + |
| 435 | + return { covered, unbuildable, undiscoverable, withoutPathParams } |
424 | 436 | } |
425 | 437 |
|
426 | 438 | /** |
|
0 commit comments