Skip to content

fix(serializer): apply tools.config.params before validating required tool params#4391

Merged
icecrasher321 merged 7 commits intostagingfrom
waleedlatif1/serializer-canonical-validation
May 2, 2026
Merged

fix(serializer): apply tools.config.params before validating required tool params#4391
icecrasher321 merged 7 commits intostagingfrom
waleedlatif1/serializer-canonical-validation

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • pre-execution validator was looking up tool param names directly on extracted params, but blocks that rename inputs in tools.config.params (canonical document → tool param file, e.g. firecrawl parse, reducto v2) only have the tool name after that mapper runs at execution time
  • run the block's tools.config.params once on a shallow copy before validating, then check the mapped value with raw-params fallback — restores correct validation for canonical-rename blocks without changing behavior for any other block

Type of Change

  • Bug fix

Testing

Tested manually + 97 serializer tests pass

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented May 2, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 2, 2026 1:53am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 2, 2026

PR Summary

Medium Risk
Changes pre-execution required-field validation for tool-backed blocks by applying a params-mapping function first; incorrect mapping could cause workflows to fail validation or skip missing required inputs.

Overview
Pre-execution serializer validation for tool-backed blocks now runs the block’s tools.config.params mapper (with try/catch) and validates required user-only tool params against the mapped result instead of the raw extracted params.

This fixes false “missing required field” errors for blocks that rename canonical inputs to tool param names during execution, while falling back to raw params if the mapper throws.

Reviewed by Cursor Bugbot for commit afadcf5. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 2, 2026

Greptile Summary

Fixes a pre-execution validation false-negative: blocks that rename canonical inputs to tool param names via tools.config.params (e.g. Firecrawl Parse renaming documentfile) were always reported as missing a required field because the validator read from the pre-mapper params object. The fix runs the mapper on a shallow copy of params before the loop, spreads the result into mappedParams, and reads mappedParams[paramId] instead — with a silent catch fallback to raw params if the mapper throws on placeholder values.

Confidence Score: 5/5

Safe to merge — targeted, well-guarded fix with no new failure modes introduced.

The change is minimal and surgical: one new try/catch block that maps params before validation, falling back to raw params on any error. Previous review feedback (array guard, redundant fallback removal) has been addressed. No P1 or P0 issues found.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/serializer/index.ts Applies tools.config.params mapper before required-field validation so canonically-renamed params (e.g. document → file) are correctly resolved; mapper errors fall back to raw params safely.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Enter validateBlock] --> B{currentTool?}
    B -- No --> G[validate subBlocks only]
    B -- Yes --> C{tools.config.params is a function?}
    C -- No --> D[mappedParams = params]
    C -- Yes --> E[call mapper on shallow copy of params]
    E --> F{result is non-null non-array object?}
    F -- Yes --> H[mappedParams = spread params plus result]
    F -- No --> D
    E -- throws --> D
    D --> I[iterate tool.params entries]
    H --> I
    I --> J{param required and visibility=user-only?}
    J -- No --> I
    J -- Yes --> K{shouldValidateParam?}
    K -- No --> I
    K -- Yes --> L[check mappedParams at paramId]
    L --> M{undefined or null or empty?}
    M -- Yes --> N[push to missingFields]
    M -- No --> I
Loading

Reviews (2): Last reviewed commit: "fix(serializer): guard array results and..." | Re-trigger Greptile

Comment thread apps/sim/serializer/index.ts Outdated
Comment thread apps/sim/serializer/index.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit afadcf5. Configure here.

Renames the canonical id from `document` to `file` on firecrawl, reducto v2,
pulse v2, and extend v2 so pre-execution validation resolves the value
under the same key the tool expects, eliminating false "missing required
fields: file" errors at submit time.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ames

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
waleedlatif1 and others added 2 commits May 1, 2026 18:43
…nonical-id contract via audit

The serializer's pre-execution validator no longer runs the block's
`tools.config.params` mapper to discover renamed tool param ids. Instead
it relies on the contract that every required+user-only tool param is
backed by a subBlock whose `id` or `canonicalParamId` equals the tool
param id, and a new audit (`bun run check:block-canonical`) enforces
this. Migrates posthog (`personalApiKey` → canonical `apiKey`) so the
audit passes cleanly.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Folds the canonical-id contract audit into the existing subblock ID
stability script and renames it to `check-block-registry.ts`. Both
checks share the same `getAllBlocks()` import and registry-invariant
purpose, so a single CI gate now catches both regression classes.
The early-exit path on the stability check no longer short-circuits
the canonical-id check.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Each check returns a discriminated `CheckResult` (pass | skip | fail)
so the runner prints one definitive line per check instead of mixing a
"skipping" message with a redundant "passed" line. Failure messages
include a per-check header explaining the runtime impact.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@icecrasher321 icecrasher321 merged commit 50e118a into staging May 2, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants