feat: agent usability — bounded outputs, structured errors, complete tool schemas - #383
feat: agent usability — bounded outputs, structured errors, complete tool schemas#383chenxin-yan wants to merge 21 commits into
Conversation
…nuation Keep detailed as the compatibility default for small responses while applying a 64k-character hard ceiling. Concise responses use a 12k ceiling and remove screenshots, base64 fields, and duplicate raw HTML with explicit in-band omission guidance. Crawl collection stops at page boundaries after 25 documents or 48k bytes and exposes the upstream cursor through check_crawl_status.
…tion Pin the application schema runtime to Zod 4.1.13. Zod 4.1.11 schemas lose registry metadata when xsschema converts them through its newer Zod peer, while 4.1.13 preserves descriptions without changing the emitted schema shape.
Document every full-surface property and nested object field with concise semantics, defaults, constraints, and material cost implications. The tools/list integration test guards at least 95 percent coverage and checks opaque high-risk fields.
M1: enforce the total serialized response ceiling for wide payloads and add a failing-first regression test. m1: merge truncation metadata with existing _firecrawl result notices. m2: limit LIKELY_BLOCKED notices to empty or near-empty scrape content. m3: classify only the server's exact crawl-continuation validation message as INVALID_REQUEST. m4: validate path-prefixed crawl continuations and pass a base-relative path to the SDK. m5: cover cross-origin and wrong-job continuation rejection. m7: force one Zod 4.1.13 tree through the pnpm workspace override; pnpm 11 ignores package.json pnpm.overrides. n1: avoid splitting UTF-16 surrogate pairs when truncating strings. n2: reject empty crawl job IDs at schema validation. n3: correlate early validation action logs with the structured payload request_id. n4: document the deliberate pre-FastMCP validation tradeoff. n6: remove the fork-internal source-path reference. Known scope limits: monitor tools remain unbounded by design in this PR; firecrawl_monitor_check is the only potentially large monitor response surface.
Firecrawl's domain denylist rejection ('we do not support this site')
previously fell through to the generic retryable UPSTREAM_REQUEST_FAILED,
telling agents to retry a permanently blocked URL. Hosted eval EXP-048
(session ax-db-20260821-201031-c8200fb6) surfaced agents relaying that
misleading retry guidance verbatim. The rejection now maps to a
non-retryable UNSUPPORTED_SITE code that steers agents to a different
source instead of a retry.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…able Cursor-fetched crawl pages bypassed the document/byte caps entirely: the initial page assignment took pageDocuments.slice() unwindowed, so a continuation page larger than the caps was returned whole. Every page now goes through takeCrawlDocumentWindow. A partially shown page is resumable by combining next with offset (previously next zeroed offset), and the truncation notice carries both so agents can retrieve the remainder instead of skipping to the following cursor. Found by PR review on firecrawl#383.
Type options as Record<string, unknown> so removing parse-only keys with delete is legal on the union, and key the interact session guard on !scrapeId so control flow narrows it to string before client.interact.
Upstream per-operation 401s (seen chronically on firecrawl_interact in
hosted evals) previously got operator-only guidance ('configure an API
key'), which gpt-5.6-terra obeyed by abandoning the task even though
firecrawl_scrape worked in the same session (EXP-048 round 4: the only 2
paired losses in 90 pairs). The wrapped-error guidance now leads with a
same-session fallback: retry once or use another Firecrawl tool. The
pre-flight no-credential AUTH_REQUIRED sites keep setup-only guidance
since there the diagnosis is certain.
…file
Hosted A/B rounds (EXP-048) showed the concise/detailed knob is
second-order: the caps, compaction, truncation notices, and crawl
continuation carry the output bounding, while concise adoption is
model-dependent (heavy under gpt-5.6-sol, near-zero under gpt-5.6-terra)
and its savings overlap request narrowing agents already do. Dropping the
parameter removes seven schema fields and a second compaction profile.
All responses now use the former detailed behavior: 64k cap,
{arrayItems 50, objectFields 100, stringChars 12000} compaction with
iterative shrinking and _firecrawl truncation metadata.
There was a problem hiding this comment.
2 issues found across 10 files
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="tests/helpers.mjs">
<violation number="1" location="tests/helpers.mjs:18">
P3: getFreePort has a check-then-use race: the port is released before the spawned server binds it, so a concurrent process can claim it in between and cause a bind failure or a bad target, making tests flaky. Prefer binding port 0 directly in the child and reading the bound port back, or retry on EADDRINUSE.</violation>
</file>
<file name="src/index.ts">
<violation number="1" location="src/index.ts:1327">
P2: Local keyless sessions are filtered out of tools/list via isLocalKeylessSession in canList, but beforeValidate and execute never check that same condition, so calling a tool hidden from discovery (e.g. firecrawl_parse) is not rejected with the structured KEYLESS_TOOL_NOT_AVAILABLE payload like hosted keyless sessions. Add the isLocalKeylessSession && !localKeylessTool guard (with the same KEYLESS_TOOL_NOT_AVAILABLE recovery) to beforeValidate and execute so advertising and call-time behavior stay consistent.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| ? keylessTool | ||
| : true) && | ||
| : isLocalKeylessSession(session) | ||
| ? localKeylessTool |
There was a problem hiding this comment.
P2: Local keyless sessions are filtered out of tools/list via isLocalKeylessSession in canList, but beforeValidate and execute never check that same condition, so calling a tool hidden from discovery (e.g. firecrawl_parse) is not rejected with the structured KEYLESS_TOOL_NOT_AVAILABLE payload like hosted keyless sessions. Add the isLocalKeylessSession && !localKeylessTool guard (with the same KEYLESS_TOOL_NOT_AVAILABLE recovery) to beforeValidate and execute so advertising and call-time behavior stay consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/index.ts, line 1327:
<comment>Local keyless sessions are filtered out of tools/list via isLocalKeylessSession in canList, but beforeValidate and execute never check that same condition, so calling a tool hidden from discovery (e.g. firecrawl_parse) is not rejected with the structured KEYLESS_TOOL_NOT_AVAILABLE payload like hosted keyless sessions. Add the isLocalKeylessSession && !localKeylessTool guard (with the same KEYLESS_TOOL_NOT_AVAILABLE recovery) to beforeValidate and execute so advertising and call-time behavior stay consistent.</comment>
<file context>
@@ -1202,7 +1323,9 @@ function guardHostedTool(
? keylessTool
- : true) &&
+ : isLocalKeylessSession(session)
+ ? localKeylessTool
+ : true) &&
(canList?.(session) ?? true),
</file context>
| await new Promise((resolve, reject) => { | ||
| server.close((error) => (error ? reject(error) : resolve())); | ||
| }); | ||
| return port; |
There was a problem hiding this comment.
P3: getFreePort has a check-then-use race: the port is released before the spawned server binds it, so a concurrent process can claim it in between and cause a bind failure or a bad target, making tests flaky. Prefer binding port 0 directly in the child and reading the bound port back, or retry on EADDRINUSE.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/helpers.mjs, line 18:
<comment>getFreePort has a check-then-use race: the port is released before the spawned server binds it, so a concurrent process can claim it in between and cause a bind failure or a bad target, making tests flaky. Prefer binding port 0 directly in the child and reading the bound port back, or retry on EADDRINUSE.</comment>
<file context>
@@ -0,0 +1,72 @@
+ await new Promise((resolve, reject) => {
+ server.close((error) => (error ? reject(error) : resolve()));
+ });
+ return port;
+}
+
</file context>
…ssified failures firecrawl_interact and firecrawl_monitor_create can produce external side effects before an upstream error surfaces (a submitted form, a created monitor). Marking those failures retryable invites an obedient agent to repeat the side effect; guidance now says to verify current state first. Read-only tools keep retryable unclassified failures.
…teardown The monitor_create description lost the 'one or more non-empty values' qualifier in the dedup refactor while the code still filters empty queries before choosing search over page targets. stopChild now awaits child exit after the SIGKILL fallback so killed servers cannot outlive test cleanup.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
I ran a local 72-run A/B against the current version using natural prompts that did not mention Firecrawl. This PR did not improve tool selection: Firecrawl was picked 15/36 times versus 16/36 on current, and succeeded 11/36 versus 13/36. Output quality was flat. I would keep the inline argument descriptions and the keyless parse fix, but I would not position this as a discoverability improvement. |
|
I also ran a local 72-run A/B where agents were told to use Firecrawl. Both versions used it 36/36 times and succeeded 29/36, with similar output quality. The clearer blocked-site errors are useful and should stay. Before merging, I would classify errors from SDK status and code instead of message text. I would also fix crawl pagination: a partial page can return next: null and make an incomplete crawl look finished. The truncation approach is good, but it needs to preserve a usable cursor and have a regression test. |
apps/api has a second permanent domain-policy 403 besides the blocklist
message: org-level threat protection ('blocked by your organization's
threat protection policy', threat-protection/error.ts). The classifier
only matched the blocklist phrasing, so threat-protection blocks fell
through to UPSTREAM_REQUEST_FAILED with retryable=true - inviting the
futile retry loop UNSUPPORTED_SITE exists to prevent. Found by auditing
the MCP error taxonomy against the service source.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
yes I was running eval closely just making sure the behavior changes of the mcp tools would not lead to any regressions. I will make some changes and let you know when its ready |
… by SDK status/code Two pre-merge review requests: Crawl cursor: when the client-side document window truncated a page that had no upstream cursor, the response carried next:null while documents remained, making an incomplete crawl look finished to anything keying on the API-native field. A native skip cursor (/v2/crawl/:id?skip=N) is now synthesized in that case - the service supports skip pagination and the indices align with the shown-document count - so next is always usable when truncated. Regression test follows the synthesized cursor and asserts documents 26-50 arrive. Error classification: wrapToolError now reads duck-typed SDK error metadata (status, code, details) before falling back to message regexes: 401 -> AUTH_REQUIRED, 408/SCRAPE_TIMEOUT -> UPSTREAM_TIMEOUT (also catches opaque messages like heartbeat_failed), 429 -> new RATE_LIMITED (retryable - a 429 is rejected before execution - with concrete retry-after seconds parsed from the body since the service sends no Retry-After header). Plain-string errors still classify via regex.
…ble API code monitor_run queues a new run and feedback creates a new record on every POST, so their unclassified upstream failures must not advertise retryable; update/delete stay retryable because repeating them converges on the same target state. Threat-protection blocks now classify by the service's stable code (unsafe_domain_blocked - kept for API stability upstream) instead of relying on message wording; the regex remains as fallback for the codeless global-blocklist 403.
…y review Simplifications from an over-engineering review pass, no behavior change: - reuse ClientLike from research.ts instead of the identical local CrawlClientLike - drop the guidance.slice(0, 2000) cap (all guidance strings are short internal literals) and the unreachable never-typed default arm in withResultNotice - drop per-separator byte bookkeeping in takeCrawlDocumentWindow; the 48k cap does not need comma-level precision - dedup the new smoke tests through startHostedCloudServer and startStdioClient helpers (exact-match conversions only) - remove the brittle toolNames.length === 26 assertion; the name-based assertions already pin the tool contract Verified: tsc --noEmit clean, npm test 89/89 pass.
2 participants
Agents hit three failure modes with this server: unbounded outputs that flood the context window, opaque errors that trigger pointless retries, and parameter descriptions that are silently dropped from
tools/list. This PR fixes all three. Every change is validated by paired A/B evals against the base commit (results below).What changed
offsetcontinuation for capped crawls (continuation pages bounded too)1b0eb40928222a82234684bf7792366cb7bcodetaxonomy +retryableflag + recovery guidance;EMPTY_RESULT/LIKELY_BLOCKEDflags on silent-empty results; non-retryableUNSUPPORTED_SITEfor domain-policy rejections; agent-actionable fallback inAUTH_REQUIREDrecovery guidance; non-idempotent tools never advertiseretryable; threat-protection blocks classified permanenta6d966bdcf19a81f18740f98fe9cd77be3e.describe()in serialization — published npm packages ship zero param descriptions today); describe all 326 params on callable tools; cut description prose −48%b8c22e22ea6f6c11f8520tools/listadvertisedfirecrawl_parse, which is not callable without a key; the advertised set now matches the callable set (Search + Scrape)8c1bf20Eval results
Paired A/B, 5 independent hosted runs: 300 traces, 150 pairs (2 models × 5
tasks × 3 reps × 5 runs). Baseline = this repo @
678c92a, packed identicallyto the branch, so the diff isolates exactly these commits. The final run
covers the exact branch tip.
* Of the 8 non-ties, 5 traced to Firecrawl rate-limit/quota hits (3 favoring
this PR, 2 favoring baseline — environmental noise, roughly symmetric). The
other 3 are product-attributable and tell one story: an early revision's
AUTH_REQUIREDrecovery text gave only operator guidance ("configure a key"),so on a spurious per-operation 401 one model obediently gave up (2 losses in
run 4); after rewording the guidance to include a same-session fallback
(
1f18740), that model recovered 6/6 in runs 5–6 and beat baseline'simprovised recovery once (1 win). The eval loop caught and corrected its own
regression.
Behavior deltas (from transcripts):
UNSUPPORTED_SITETool 'firecrawl_scrape' execution failed: <raw string>Recovery:guidanceContext-window cost of the schema changes:
tools/listtotal payloadLocal adversarial suite: 360 runs; adversarial-recovery scenarios 0/30 → 24/30.
Known service-side limitation: crawl-status pagination orders documents by
finished_at, created_atwith no unique tie-break column, so skip-basedresume (which the
offsetcontinuation builds on) is not provably stablewhen those timestamps tie. The continuation inherits that property; it cannot
be fixed client-side.
Verification
85/85 tests,
tsc --noEmitclean, eslint clean. Install verified from git andtarball (builds via
prepare, zod resolves 4.1.13 flat, 326/326 params oncallable tools described over stdio
tools/list; only the deprecatedfirecrawl_extractstub, which rejects immediately with redirect guidance,leaves its unused params undescribed).