feat: Surface compact input schema on search-actors results - #737
Conversation
5f31e83 to
a873a4a
Compare
a873a4a to
9c1de65
Compare
|
I'm afraid this PR adds a lot of complexity. It was supposed to be a simple addition to the existing logic. The branch adds: a new cache, a generic semaphore utility, a parallel fetch pipeline that duplicates defaultBuild()+schema, and a third argument. I can see three approaches:
My order of preference would be 3 -> 2 -> 1 but let me pull @Jkuzz for his opinion here. |
jirispilka
left a comment
There was a problem hiding this comment.
See my comment here: #737
|
Makes sense, it is large and inefficient change as I did not want to touch the API or MongoDB - let's touch the API then. I would be for the option 2 so we do the input fields lookup from MongoDB for the default build - no need to handle the Algolia. Right? @Jkuzz |
6c6670c to
3d97156
Compare
There was a problem hiding this comment.
I like the direction!
Glad rental actors are gone and the code is simpler. But the tool description's "input schema" will confuse an LLM.
IMO the naming is off in a few places. I know you hate nitpick comments but I think we should approach a similar review bar as in apify-core :). We keep coming back to this code, and we've got the bandwidth now, so let's decrease the YOLO part a bit :)
`STORE_INPUT_SCHEMA_PAGE_LIMIT` -> `MAX_LIMIT_WITH_INPUT_SCHEMA` (mirrors apify-core's constant name). `STORE_INPUT_SCHEMA_TEXT_FIELD_LIMIT` -> `MAX_INPUT_SCHEMA_TEXT_FIELDS`. Per review on #737 — old names brought back the `STORE_*` prefix style the codebase has been moving away from.
The model expects "input schema" to mean a full JSON Schema (descriptions, defaults, ...). Our compact projection is just a typed argument list, so the old label was misleading. - Tool description: rephrased + added concrete example (\`url: string, maxResults?: number\`) - Card markdown line: \`- **Input schema:**\` -> \`- **Input fields:**\` - Helper: \`formatInputSchemaForText\` -> \`inputFieldsToString\` (matches \`pricingInfoToString\`) - Tests updated to assert the new label The structured output field stays \`inputSchema\` (it IS a partial JSON Schema returned by apify-core). Per review on #737.
… Partial overrides
Mirrors the `makeActorTool` pattern from `tools.skyfire.test.ts`:
fully-typed `baseStoreActor` const + `(overrides: Partial<X>) => ({ ...defaults, ...overrides })`.
No more per-call `as unknown as ActorStoreList` double cast.
Per review on #737.
The original "renders every property without truncation" test created 15 properties and sat inside the 20-field no-truncation window — the name implied truncation coverage that wasn't actually being exercised. - Renamed to "renders all input fields when count is within MAX_INPUT_SCHEMA_TEXT_FIELDS". - Added a separate test that puts MAX_INPUT_SCHEMA_TEXT_FIELDS + 5 properties through and asserts: * the last in-window field is rendered * the first overflow field is NOT rendered * the " ... (+5 more)" suffix appears - Dropped the negative regex assertion against `/Input schema \(\d+ of \d+\)/` — asserted a format the code never produces. Per review on #737.
Each search result now carries a compact `inputSchema` (types-only JSON
Schema projected by apify-core#27466). LLMs can construct Actor input
directly without a separate fetch-actor-details round-trip per hit.
- Pass-through `includeInputSchema=true` to `GET /v2/store`; drops the
per-Actor build fetch fan-out the previous revision used.
- Public `search-actors.limit` capped at STORE_INPUT_SCHEMA_PAGE_LIMIT
(10) to match the API cap. Bounded pagination preserves the existing
+50 rental-filter scan budget across pages.
- Structured field renamed: `inputFields[]` + `totalInputFields` →
pass-through `inputSchema?: { type:'object', properties, required? }`.
Text card renders the first 10 properties as `name?: type, ...` with
an `(N of M)` suffix when truncated.
- Public `search-actors.limit` returns to 1-100; tracked in #791 to cap at 10 once we are willing to drop schema enrichment for the >10 case. - Schema enrichment is requested only when the caller's `limit` fits the API cap; larger requests fall back to a plain search since the API rejects the flag above the cap. - Drop the legacy MCP-side rental over-fetch (`ACTOR_SEARCH_ABOVE_LIMIT`) and `userRentedActorIds` plumbing through `searchAndFilterActors`. The store API filters rentals (`AGENT_SAFE_PRICING_MODELS`) and applies safety filters server-side now. Internal-repo Mongo lookup for rented Actor IDs becomes dead code; tracked in apify-mcp-server-internal#500. - Log a `log.error` when the search response still contains a rental Actor — observability hook in case the upstream contract changes. - Rename description line "Input fields" → "Input schema" for consistency with the structured field.
… "Input schema" - Drop truncation: late-section properties may be required or otherwise important; the structured card already exposes the full schema, so keeping the text view in sync avoids hiding fields the LLM may need. - Remove `STORE_INPUT_SCHEMA_TEXT_FIELD_LIMIT` (no longer used). - Rename text label to match the structured field name.
Caps public `searchActorsBaseArgsSchema.limit` at apify-core's `MAX_LIMIT_WITH_INPUT_SCHEMA` (10) so every search is uniformly enriched with `inputSchema`. Drops the conditional `includeInputSchema` branch and the runtime Math.min clamp in `searchActorsByKeywords`. Also drops the rental-detection `log.error` block — the filter is upstream's job (`AGENT_SAFE_PRICING_MODELS` + `filterSafeActors`), and a log we don't control would page on upstream contract drift. Closes #791
`STORE_INPUT_SCHEMA_PAGE_LIMIT` -> `MAX_LIMIT_WITH_INPUT_SCHEMA` (mirrors apify-core's constant name). `STORE_INPUT_SCHEMA_TEXT_FIELD_LIMIT` -> `MAX_INPUT_SCHEMA_TEXT_FIELDS`. Per review on #737 — old names brought back the `STORE_*` prefix style the codebase has been moving away from.
…ombo `searchActorsByKeywords` now throws synchronously when called with `includeInputSchema=true` and `limit > MAX_LIMIT_WITH_INPUT_SCHEMA`, instead of relying on apify-core to return a 400. The public tool path can never trigger this (Zod caps `limit` at the same value), so this is a fail-fast guard for future internal callers. A clear error here beats a confusing 400 from apify-core.
The model expects "input schema" to mean a full JSON Schema (descriptions, defaults, ...). Our compact projection is just a typed argument list, so the old label was misleading. - Tool description: rephrased + added concrete example (\`url: string, maxResults?: number\`) - Card markdown line: \`- **Input schema:**\` -> \`- **Input fields:**\` - Helper: \`formatInputSchemaForText\` -> \`inputFieldsToString\` (matches \`pricingInfoToString\`) - Tests updated to assert the new label The structured output field stays \`inputSchema\` (it IS a partial JSON Schema returned by apify-core). Per review on #737.
… Partial overrides
Mirrors the `makeActorTool` pattern from `tools.skyfire.test.ts`:
fully-typed `baseStoreActor` const + `(overrides: Partial<X>) => ({ ...defaults, ...overrides })`.
No more per-call `as unknown as ActorStoreList` double cast.
Per review on #737.
The original "renders every property without truncation" test created 15 properties and sat inside the 20-field no-truncation window — the name implied truncation coverage that wasn't actually being exercised. - Renamed to "renders all input fields when count is within MAX_INPUT_SCHEMA_TEXT_FIELDS". - Added a separate test that puts MAX_INPUT_SCHEMA_TEXT_FIELDS + 5 properties through and asserts: * the last in-window field is rendered * the first overflow field is NOT rendered * the " ... (+5 more)" suffix appears - Dropped the negative regex assertion against `/Input schema \(\d+ of \d+\)/` — asserted a format the code never produces. Per review on #737.
…edundant tests
Renames the field on `StructuredActorCard` and the corresponding
`actorInfoSchema` property in the search-actors output schema so the
structured output matches the LLM-facing text label ("Input fields") and
no longer claims to be a full JSON Schema. The schema description is also
shortened to a single example line.
The apify-core API response field stays `inputSchema` (we do not own that
contract) and `fetch-actor-details` still emits a full `inputSchema` (it
is a real JSON Schema there).
Also trims tests added earlier in this PR:
- Drop "forwards `limit` verbatim when `includeInputSchema` is omitted"
in actor_search: pure tautology after the clamping logic was removed.
- Drop the structured-card `inputSchema` describe block: the discriminator
is already exercised by the 5 text-rendering tests, and field-name
regressions are caught by integration outputSchema validation.
Lowers the `limit` from 100 to MAX_LIMIT_WITH_INPUT_SCHEMA (10) so the test passes the new Zod cap, and rewrites the stale comment block that referred to the old MongoDB-vs-standby code path. Reframed as an upstream-contract canary: rental filtering is now apify-core's job (`AGENT_SAFE_PRICING_MODELS`), this test guards the MCP side against upstream contract drift.
…on into base test The "renders all input fields when count is within MAX_INPUT_SCHEMA_TEXT_FIELDS" test only uniquely caught the \`overflow >= 0\` mutant at exactly the cap; moving its \`not.toMatch(/\\(\\+\\d+ more\\)/)\` assertion into the base rendering test (which already exercises overflow=0 with 2 fields) covers the same mutant. Truncation behavior at >MAX is still covered separately.
The local rental filter is gone; the function is now a preset around searchActorsByKeywords that pins includeInputSchema=true and forwards allowsAgenticUsers. New name reflects that; doc updated accordingly.
…ption The local user-rentals lookup is gone, so rentals are never returned regardless of access level. Drop the 'depending on the user's access level' hedge and the redundant 'private' (covered by 'publicly available').
Was a bare { type: 'object' }, leaving strict consumers with no structural
info on the new field. Mirror ActorStoreInputSchema (type/properties/required)
so the structured contract matches the wire shape.
6239334 to
ea5182b
Compare
|
@jirispilka I agree, I should have checked the changes more - there was quite a lot of issues as I wanted to finally ship that and move fast. Let's slow down and do things properly. I did put more time and effort into this PR and it should now be finalized. |
jirispilka
left a comment
There was a problem hiding this comment.
I like it.
I'm requesting the changes as I believe the throw inside search should not be there at all. The both callers fill a limit. And if we start to call this function somewhere else we might get unexpected error.
Co-authored-by: Jiří Spilka <jiri.spilka@apify.com>
Co-authored-by: Jiří Spilka <jiri.spilka@apify.com>
- MAX_INPUT_SCHEMA_TEXT_FIELDS -> MAX_INPUT_FIELDS_IN_TEXT_CARD (constant name now contains "fields" and reflects that it only applies to the text card; structured output keeps the full schema) - searchActorsByKeywords: make limit required in SearchActorsByKeywordsOptions to match how both callers already use it (actor_details.ts passes ACTOR_DETAILS_PICTURE_SEARCH_LIMIT, searchAgentSafeActors passes the Zod-validated limit). Drops the redundant `limit !== undefined` guard.
…orce Mirroring apify-core's MAX_LIMIT_WITH_INPUT_SCHEMA in a local throw created two sources of truth that could drift, and surprised any future caller hitting it from outside searchAgentSafeActors. The public tool's Zod schema already caps limit at the same value, so the throw never fired in practice; apify-core 400s above the cap with an informative message when an internal caller misuses it.
|
@jirispilka makes sense, the throw was too defensive - let's make the API throw instead. |
## Context `userRentedActorIds` is dead code in this repo after #737. The metadata is parsed off `_meta`, threaded through `InternalToolArgs` / `executeToolAndUpdateTask`, and forwarded to three `tool.call(...)` sites — none of which consume it. Closes #805. ## Solution Drop the type fields (`InternalToolArgs.userRentedActorIds`, `ApifyRequestParams._meta.userRentedActorIds`), the `meta?.userRentedActorIds` extraction in `mcp/server.ts`, the `executeToolAndUpdateTask` parameter, and the three pass-through sites. Also strip the matching stale example from `call_actor_widget.ts`'s schema comment: `async` was replaced by `waitSecs` in #825 and `previewOutput` was deprecated even earlier, so the "stray keys such as `async` or `previewOutput`" example is doubly wrong. Comment now just says "stray keys" — the schema's contract holds regardless of which key. ## Worth your attention - **Receiver-side only** — internal repo's Mongo lookup that populated `_meta.userRentedActorIds` is being removed in parallel in apify-mcp-server-internal#568. Order doesn't matter: once neither side reads or writes the field, both sides can land independently. - **`InternalToolArgs` is a public interface** — removing an optional field is technically a wire-shape change for any external consumer that destructured it. Grep confirms no in-tree tool reads it; external consumers couldn't have been using it for anything useful since search filtering moved upstream. - **Test fixtures unchanged** — `tests/unit/tools.call_actor_widget.response.test.ts` and `tests/integration/suite.ts` still use `async` / `previewOutput` as arbitrary stray-key fixtures. Those tests assert "schema strips unknown keys", which is still what they exercise; the field names are just convenient unknowns. ## Follow-up - apify-mcp-server-internal#568 — drops the upstream Mongo lookup that fed `_meta.userRentedActorIds`
Context
LLMs hitting
search-actorspreviously had to make an N+1 round-trip (fetch-actor-detailsper hit) to learn each Actor's input shape. apify-core#27466 addedincludeInputSchema=trueonGET /v2/storeto project a compact schema server-side.Solution
Pass
includeInputSchema=trueand surface the returned shape asinputFieldson both the structured card and the text card.Worth your attention
limitcapped at 10 —searchActorsBaseArgsSchema.limit.max(MAX_LIMIT_WITH_INPUT_SCHEMA)so every search is uniformly enriched. Closes feat: Cap search-actors limit at 10 so includeInputSchema is always returned #791.searchActorsByKeywordsthrows fail-fast onincludeInputSchema=truewithlimit > MAX_LIMIT_WITH_INPUT_SCHEMArather than silently clamping or letting apify-core 400. Public path is Zod-capped, so this is a guard against future internal-caller misuse.AGENT_SAFE_PRICING_MODELSalready filters rentals. The legacy+50over-fetch,filterRentalActors, and thelog.errorobservability check are gone.inputFields), card label, helper (inputFieldsToString), and tool description all use "input fields" now;actorInfoSchema.inputFieldsencodes the wire shape so strict consumers can validate it.Follow-up
_meta.userRentedActorIdsuserRentedActorIdsplumbing #805 — drop the now-deaduserRentedActorIdsplumbing in this repo (InternalToolArgs,_meta,mcp/server.ts)