Skip to content

fix(eval): preserve sibling params when Filepicker Binary data is passed to action run - #42021

Merged
sebastianiv21 merged 4 commits into
releasefrom
claude/github-issue-8639-56147e
Aug 3, 2026
Merged

fix(eval): preserve sibling params when Filepicker Binary data is passed to action run#42021
sebastianiv21 merged 4 commits into
releasefrom
claude/github-issue-8639-56147e

Conversation

@sebastianiv21

@sebastianiv21 sebastianiv21 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Description

Filepicker Binary-format data passed alongside other params to Api.run() caused every sibling param to become null and {{this.params}} to become null.

Reproduce: Filepicker with Data Format = Binary, then

await Api1.run({ name: "Test", sources: Filepicker1.files })

name arrives as null (and this.params is null). Base64 format and the no-file case work fine.

Root cause: DataTreeEvaluator.evaluateActionBindings serialized the whole params object into a single {{ ${JSON.stringify(executionParams)} }} binding and re-parsed it with the brace-counting getDynamicStringSegments. Filepicker Binary data is a raw readAsBinaryString byte string that routinely contains unescaped {/} bytes; JSON.stringify does not escape braces, so the counter unbalances and the entire params object collapses to undefined. Base64 works because its alphabet has no braces.

Fix: JSON.stringify emits only literals, so that round-trip could only deep-clone the (already fully-evaluated) params — it never resolved nested bindings. Replace it with the already-imported JSON-safe deep clone klonaJSON(executionParams). This is behavior-preserving for valid cases, faithfully passes JS values through, avoids re-serializing multi-MB binary payloads, and removes the brace vulnerability entirely.

Reviewer notes:

  • Intentional (more-faithful) behavior change now covered by tests: undefined/NaN/Date param values pass through unnormalized instead of being JSON-coerced.
  • generateOverrideContext already receives the raw executionParams object, so its (EE) contract is unchanged.
  • Verified red→green: on the old code the regression test returns all-undefined params (the exact bug); on the fix all evaluateActionBindings tests pass.

TL;DR: Filepicker Binary data in Api.run() params no longer nulls out the other params — the params object is now deep-cloned instead of round-tripped through the {{ }} binding parser.

Fixes https://github.com/appsmithorg/appsmith-ee/issues/8639

Automation

/ok-to-test tags="@tag.All"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/30824007861
Commit: f7bae03
Cypress dashboard.
Tags: @tag.All
Spec:


Mon, 03 Aug 2026 15:45:44 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Bug Fixes
    • Fixed action parameter evaluation when values contain unbalanced braces, preserving sibling parameters and correct results.
    • Prevented already-evaluated parameters from being altered through template parsing or type conversion.
    • Improved handling of execution parameter references during dynamic value evaluation.

…sed to action run (#8639)

Passing Filepicker Binary-format data alongside other params to Api.run(),
e.g. `Api1.run({ name: "Test", sources: Filepicker1.files })`, caused every
sibling param to resolve to null and `this.params` to become null.

Root cause: `evaluateActionBindings` serialized the whole params object into a
single `{{ ${JSON.stringify(executionParams)} }}` binding and re-parsed it with
the brace-counting `getDynamicStringSegments`. Binary file data (a raw
readAsBinaryString byte string) routinely contains unescaped '{'/'}' bytes,
which `JSON.stringify` does not escape, unbalancing the brace counter and
collapsing the entire params object to undefined. Base64/no-file cases worked
because their content has no braces.

Since `JSON.stringify` emits only literals, that round-trip could only
deep-clone the (already fully-evaluated) params — it never resolved nested
bindings. Replace it with the already-imported JSON-safe deep clone
`klonaJSON`, which is behavior-preserving for valid cases, faithfully passes
JS values through, avoids re-serializing multi-MB binary payloads, and removes
the brace vulnerability entirely.

Tests: add a regression test (unbalanced braces in a param preserve siblings),
a behavior-lock test (binding-like string preserved verbatim, NaN survives),
and update the getDynamicValue-call assertion to the new single-call-per-binding
sequence.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

evaluateActionBindings now deep-clones object execution parameters instead of parsing them as templates. Tests cover binary-like strings, already-evaluated values, NaN, and updated getDynamicValue override-context calls.

Changes

Execution parameter evaluation

Layer / File(s) Summary
Direct cloning and regression coverage
app/client/src/workers/common/DataTreeEvaluator/index.ts, app/client/src/workers/common/DataTreeEvaluator/dataTreeEvaluator.test.ts
Object execution parameters are cloned with klonaJSON, while tests verify preservation of sibling parameters, binding-looking strings, NaN, and per-binding override contexts.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

Braces may wander, bytes may gleam,
Params now travel as values, not a dream.
Strings stay steady, NaN stays true,
Each binding gets its context too.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code changes and regression tests address the issue by preserving all Api.run() params when Filepicker Binary data is present.
Out of Scope Changes check ✅ Passed The PR stays focused on the evaluator fix and related regression tests, with no clear unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly identifies the fix for preserving sibling parameters when Filepicker Binary data is passed to an action.
Description check ✅ Passed The description explains the issue, reproduction, root cause, fix, tests, linked issue, automation, and communication decision.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/github-issue-8639-56147e

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sebastianiv21 sebastianiv21 added the ok-to-test Required label for CI label Jul 20, 2026
@sebastianiv21

Copy link
Copy Markdown
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions

Copy link
Copy Markdown

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29781092349.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 42021.
recreate: .
base-image-tag: .

@github-actions

Copy link
Copy Markdown

Deploy-Preview-URL: https://ce-42021.dp.appsmith.com

@sebastianiv21
sebastianiv21 marked this pull request as ready for review July 20, 2026 22:05
@sebastianiv21
sebastianiv21 requested a review from a team as a code owner July 20, 2026 22:05
@hacktron-app

hacktron-app Bot commented Jul 22, 2026

Copy link
Copy Markdown

Hacktron Security Check - Skipped

Reason: Billing required for Code Review seats

Add a payment method and start Code Review seat billing in organization billing settings

Go to: https://app.hacktron.ai/appsmith-inc/billing

@github-actions

Copy link
Copy Markdown

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

@github-actions github-actions Bot added the Stale label Jul 31, 2026
@sebastianiv21
sebastianiv21 merged commit 199c4ab into release Aug 3, 2026
84 checks passed
@sebastianiv21
sebastianiv21 deleted the claude/github-issue-8639-56147e branch August 3, 2026 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Required label for CI Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants