Skip to content

feat: syntax-check JavaScript code fields after find/replace patches (v2.72.0) - #1014

Merged
czlonkowski merged 8 commits into
mainfrom
feat/patch-code-syntax-guard
Aug 18, 2026
Merged

feat: syntax-check JavaScript code fields after find/replace patches (v2.72.0)#1014
czlonkowski merged 8 commits into
mainfrom
feat/patch-code-syntax-guard

Conversation

@czlonkowski

@czlonkowski czlonkowski commented Aug 18, 2026

Copy link
Copy Markdown
Owner

Summary

Implements the expansion proposed by @NextLevelManagementAdvisors in #1012: after a find/replace patch lands on a JavaScript code field, the engine now parses the result and fails the operation if the patch left the code syntactically broken — instead of saving corrupted code to the workflow (live, if the workflow is active).

How it works

  • After patchNodeField (and the legacy __patch_find_replace path in updateNode) applies its patches to a field whose last path segment is jsCode or functionCode, the final value is parsed via the AsyncFunction constructor — parse-only, the code is never executed.
  • The async-function wrapping matches how n8n itself wraps Code-node JavaScript, so top-level return and await parse fine.
  • On a SyntaxError, the operation throws with the parser's message before any mutation lands on the node (patchNodeField validates before setNestedProperty; __patch_find_replace validates before the draft swap), preserving atomicity.
  • Regression-only: the guard blocks only breakage the patch itself introduces. A field that was already unparseable before patching stays patchable, so pre-existing corruption (e.g. saved by the pre-2.71.1 $ bug) can be repaired incrementally instead of being blamed on the patch.
  • Bounded and fail-open: code over 1MB is not parsed (the synchronous parse would otherwise be an event-loop DoS lever), and any non-SyntaxError parse failure (CSP EvalError, RangeError from pathological nesting) lets the patch through — the guard is a best-effort seatbelt, and it steps aside whenever it cannot judge. If it ever rejects newer syntax that the n8n runtime's V8 accepts, the tool docs point to updateNode full-field set as the unguarded escape hatch.

Scope decisions

  • Per-operation check on the final value: intermediate invalid states within one operation's patches array are fine; the error message tells agents to combine dependent edits into a single operation.
  • Skipped: values starting with = (n8n expressions embed {{ }}, not plain JS), pythonCode, non-code fields, and non-SyntaxError constructor failures (e.g. environments where Function construction is blocked) — the guard never blocks what it cannot check.
  • Direct updateNode sets of jsCode are intentionally out of scope: there the agent supplies the whole value deliberately; the corruption risk this guards against is specific to find/replace splicing.

Testing

  • 17 new unit tests across the review rounds: corruption rejected with clear message and workflow unchanged (jsCode and parameters.functionCode), already-broken field still patchable on both paths, genuine broken-to-fixed repair, expression-prefix stripping still guarded, top-level return/await accepted, multi-patch operation with invalid intermediate state accepted, regex-mode results guarded, =-expressions/pythonCode/non-code fields skipped, checked code never executed (sentinel), 1MB ceiling and RangeError nesting fail open, and continueOnError lets later operations proceed after a guard rejection.
  • Full diff-engine suites pass (355 tests across the two files) and the full unit suite is green; typecheck and build clean. Live-verified by the MCP tester agent: 7 scenarios plus an expression-exemption check against a real n8n instance, all passing.

Version

  • 2.72.0 (minor — new validation behavior), changelog entry added, runtime version synced.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

🤖 Generated with Claude Code

…(v2.72.0)

After patchNodeField (and the legacy __patch_find_replace path in
updateNode) applies its patches to parameters.jsCode or functionCode, the
result is parsed as an async function body — matching n8n's own wrapping,
so top-level return/await are valid. A patch that leaves the field
unparseable fails the operation with the parser's message instead of
saving broken code. Expressions (leading =), pythonCode, and non-code
fields are skipped; the check parses only and never executes.

Proposed by @NextLevelManagementAdvisors in #1012.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 18, 2026 21:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a safety guard to the workflow diff engine so that find/replace-style patches against JavaScript code fields (e.g. Code node jsCode, legacy functionCode) are syntax-checked before being committed, preventing silent corruption of live workflows during partial updates.

Changes:

  • Parse patched jsCode / functionCode values via an async-function constructor and reject patches that introduce SyntaxError before mutating workflow state.
  • Add unit tests covering syntax-guard behavior for patchNodeField and the legacy __patch_find_replace update path.
  • Bump version to 2.72.0 and document the new behavior in tool docs and the changelog.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit/services/workflow-diff-engine.test.ts Adds unit coverage for rejecting syntactically broken patched JS code and ensuring atomicity (no mutation on failure).
src/services/workflow-diff-engine.ts Introduces getJsSyntaxError() and integrates it into patchNodeField and __patch_find_replace application flow.
src/mcp/tool-docs/workflow_management/n8n-update-partial-workflow.ts Documents the new syntax-checking behavior for patch operations on JS code fields.
package.runtime.json Bumps runtime package version to 2.72.0.
package.json Bumps main package version to 2.72.0.
package-lock.json Syncs lockfile version fields to 2.72.0.
CHANGELOG.md Adds a 2.72.0 entry describing the new syntax-check guard.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +1407 to +1419
describe('JavaScript syntax guard on code fields', () => {
const codeWorkflow = (jsCode: string) => {
const workflow = JSON.parse(JSON.stringify(baseWorkflow));
workflow.nodes.push({
id: 'code-1',
name: 'Code',
type: 'n8n-nodes-base.code',
typeVersion: 1,
position: [900, 300],
parameters: { jsCode }
});
return workflow;
};
Comment on lines +465 to +466
'patchNodeField literal mode (regex not set) inserts replace verbatim, so $ needs no escaping. With regex: true, replace supports JS replacement patterns: $1 for a capture group, $$ for a literal $',
'Patches to parameters.jsCode or functionCode are parsed as JavaScript after applying: a patch that leaves invalid syntax fails the operation. Only the final result of one operation\'s patches array is checked, so apply dependent edits in a single operation. Values starting with = (expressions) and pythonCode are not checked',
Comment thread CHANGELOG.md Outdated

### Added

- **Patches to JavaScript code fields are now syntax-checked before saving.** After `patchNodeField` (and the older `__patch_find_replace` path in `updateNode`) applies its find/replace patches to `parameters.jsCode` or `functionCode`, the engine parses the result as the body of an async function — matching how n8n wraps Code-node JavaScript, so top-level `return` and `await` are valid. A patch that leaves the field unparseable now fails the operation with the parser's message instead of saving broken code to the workflow — live, if the workflow is active. Only the final result of one operation's patches array is checked, so several dependent edits that pass through an invalid intermediate state can ride a single operation. Values starting with `=` (n8n expressions), `pythonCode`, and non-code fields are not checked; the check parses only and never executes the code. Proposed by @NextLevelManagementAdvisors in the #1012 report. (#1012)
… pre-patch validity

Simplifier pass: single assertPatchedJsSyntax helper shared by both patch
paths, one parameterized error message (both now carry the combine-edits
hint), shorter AsyncFunction derivation, merged duplicate tests.

Behavior refinement from the same review: the guard now blocks only
regressions the patch introduces — a code field that was already
unparseable before patching stays patchable, so pre-existing corruption
can be repaired incrementally instead of being blamed on the patch.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 18, 2026 21:36
…ejection

Both suggested by the code review pass.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Test Results Summary

📊 Artifacts


Generated at Tue, 18 Aug 2026 22:04:25 GMT
Commit: 18201d0
Run: #1498

…roken JS

An "=" original is an unchecked expression, not broken JavaScript — it must
not open the incremental-repair gate. Also covers the repair allowance on
the __patch_find_replace path with a test.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

tests/unit/services/workflow-diff-engine.test.ts:1441

  • This test claims the workflow is unchanged on failure, but applyDiff() always clones the input workflow, so checking the original workflow object doesn't validate atomicity of the failed patch against the returned workflow state. Using continueOnError lets you assert result.workflow remained unchanged while still expecting success=false.
      expect(codeNode?.parameters.jsCode).toBe('const x = 2;');
    });

    describe('JavaScript syntax guard on code fields', () => {
      const codeWorkflow = (jsCode: string) => {
        const workflow = JSON.parse(JSON.stringify(baseWorkflow));

tests/unit/services/workflow-diff-engine.test.ts:662

  • These assertions check that the input workflow object wasn't mutated, but applyDiff() always deep-clones the workflow at the start. That makes this test redundant/misleading for verifying that a failed __patch_find_replace doesn't partially update the returned workflow. Consider running this in continueOnError mode so you can assert against result.workflow while still expecting success=false.

This issue also appears on line 1436 of the same file.

      const codeNode = result.workflow!.nodes.find((n: any) => n.name === 'Code');
      expect(codeNode?.parameters.jsCode).toBe('const x = (;\nreturn x + 1;');
    });

    it('should reject __patch_find_replace patches that leave jsCode with a syntax error', async () => {

Copilot AI review requested due to automatic review settings August 18, 2026 21:40
… tests

Codex review findings on #1014: the synchronous AsyncFunction parse had no
size ceiling (a 60MB body costs ~1s on the event loop), so the guard now
steps aside above 1MB. New tests pin the ceiling, the RangeError fail-open
on pathological nesting, that the checked code is never executed, that
regex-mode patch results are guarded too, and a genuine broken-to-fixed
repair. Tool docs note the updateNode escape hatch for syntax newer than
the MCP host's Node.js.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/services/workflow-diff-engine.ts:230

  • The error message says "The workflow was not modified.", but in continueOnError mode later operations can still modify the workflow, making this statement misleading. Consider wording it as "No changes were applied for this operation" (or similar) so it's always accurate.
  if (!syntaxError) return;
  // An "=" original is an expression — unchecked but not broken JS. Only a
  // plain-JS original that itself fails to parse opens the repair gate, so
  // patching an expression into broken plain JS is still caught.
  if (!original.startsWith('=') && jsSyntaxErrorOf(original) !== undefined) return;

Copilot AI review requested due to automatic review settings August 18, 2026 21:43
…l docs

Copilot review comments on #1014.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

jsCode and functionCode are noDataExpression fields: n8n strips one
leading "=" and executes the rest as code (node-helpers), so "=" values
are not expressions there. The previous exemption let a patch smuggle
broken code past the guard as "=return (". The guard now parses exactly
what n8n will run, for the patched value and the repair-gate baseline
alike.

Found by the Codex review on #1014.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 18, 2026 21:48
Codex finding on #1014: the guard returned "no error" both when code
parsed clean and when it could not be checked (over 1MB, or a RangeError
from pathological nesting), so a verifiably-valid field could be patched
into an unverifiable blob unnoticed. checkJsSyntax now reports
valid/invalid/uncheckable; an uncheckable result from a valid baseline is
rejected with a pointer to updateNode, while broken or unverifiable
baselines still open the repair gate. CSP environments where the Function
constructor is blocked stay neutral (both sides uncheckable).

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/services/workflow-diff-engine.ts:209

  • jsSyntaxErrorOf() currently returns undefined both when code is syntactically valid and when the guard could not check (size > MAX_SYNTAX_CHECKED_LENGTH or a non-SyntaxError like EvalError/RangeError). In assertPatchedJsSyntax(), that makes an uncheckable original look “valid”, so a patch that leaves a SyntaxError can be rejected even though the guard explicitly intends to fail-open whenever it cannot judge (and should not treat unknown-original as a confirmed regression).
type JsSyntaxCheck =
  | { status: 'valid' }
  | { status: 'invalid'; message: string }
  // Could not judge the code either way: oversized, or the parser gave up
  // with a non-SyntaxError (a CSP EvalError, a RangeError from pathological

Copilot AI review requested due to automatic review settings August 18, 2026 21:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/services/workflow-diff-engine.ts:1169

  • The error message for invalid JS syntax uses the hard-coded operation label passed here. Using "__patch_find_replace" can be confusing for callers who invoked an updateNode operation (the outer error wrapper will become Failed to apply operation: __patch_find_replace: ...). Consider labeling it as updateNode + the patch mode so the message matches the user-facing operation.
        assertPatchedJsSyntax('__patch_find_replace', path, current, original);

src/mcp/tool-docs/workflow_management/n8n-update-partial-workflow.ts:466

  • This doc string says the syntax guard applies only to parameters.jsCode / parameters.functionCode, but the implementation checks the last path segment (jsCode or functionCode) regardless of prefix. To avoid surprising users (e.g. nested paths ending in .jsCode), update the wording to match the actual behavior.
      'Patches to parameters.jsCode or parameters.functionCode are parsed as JavaScript after applying: a patch that breaks previously-valid code fails the operation. Only the final result of one operation\'s patches array is checked, so apply dependent edits in a single operation. A leading = is stripped before parsing, matching how n8n runs these noDataExpression fields. Fields that were already invalid before patching and pythonCode are not checked; code over 1MB is never parsed, and patching valid code into something unverifiable is rejected — set the full value via updateNode (unchecked) if that is intended. The parse runs on the MCP server\'s Node.js: in the rare case it rejects newer syntax your n8n runtime accepts, set the full field value via updateNode instead (not guarded)',

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/services/workflow-diff-engine.ts 94.87% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@czlonkowski
czlonkowski merged commit 41ac6d6 into main Aug 18, 2026
16 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