Repository navigation
fix(flows): translate n8n $json bindings to the real engine scope root - #5285
Conversation
📝 WalkthroughWalkthroughThe n8n importer now warns about duplicate node names while retaining last-name-wins wiring. Changesn8n import behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
ca8e457 to
1a1187c
Compare
|
Review follow-up: clean, no changes applied — the translation was independently verified correct for every shape (dotted paths, nested dot+bracket, quoted keys, bare One real gap found, recommended as a follow-up rather than a blocker.
Two things make it a follow-up rather than a fix here:
This PR is strictly better than before in every case — the pre-fix code was wrong here too, missing both |
…oot (R-C1)
The n8n importer translated `={{ $json.email }}` to `=.email`. The tinyflows
expression scope root is `{item, items, run, nodes}`, so `.email` dereferences a
key that does not exist and silently resolves to null at runtime. n8n's `$json`
is the current input item, which the engine binds as `item`, so the correct
translation is `=.item.email`; `={{ $json }}` likewise becomes `=.item` rather
than `=.` (which returned the whole scope object).
Every imported workflow using `$json` bindings therefore saved cleanly, reported
no warnings, and then ran hollow — producing empty tool args. This is the exact
silent-null-binding class the domain's propose/save gates were built to prevent,
and the importer bypassed all of them.
The existing test pinned the bug (it asserted `=.email` and an empty warning
list), so it is corrected here in the same commit, along with a second test that
was relying on the buggy output. Two new tests evaluate a translated expression
against a realistic `{item, items, run, nodes}` scope and assert it resolves the
real field, keeping the old form as a negative control that resolves null.
Also warns on duplicate n8n node names (R-m6), which previously mis-wired edges
silently via a last-wins name map — every other approximation in this importer
already warns.
The fix is forward-only by design: already-imported flows keep their broken
bindings, and rewriting stored graphs cannot distinguish an importer artifact
from a hand-authored root-level jq program.
1a1187c to
2fe5ca7
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/flows/n8n_import.rs (1)
990-992: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the duplicate-id assertions;
contains('a')/contains('b')are nearly tautological.
collision_warning.contains('a')and.contains('b')check for single letters, which appear in common words of the warning message (for example "named", "and", "by") regardless of whether the actual ids"a"and"b"are present. This test would still pass if the id-substitution logic silently broke. Assert the quoted id tokens instead.🔧 Proposed fix
- assert!(collision_warning.contains('a')); - assert!(collision_warning.contains('b')); + assert!(collision_warning.contains("'a'")); + assert!(collision_warning.contains("'b'"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/n8n_import.rs` around lines 990 - 992, Update the duplicate-ID assertions in the collision warning test to verify the quoted token values for IDs “a” and “b” rather than checking single-character containment, ensuring the assertions fail when ID substitution is broken.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/flows/n8n_import.rs`:
- Around line 111-129: Change the tracing call in the duplicate-name handling
around name_to_id.insert from warn to debug or trace, preserving its existing
fields and message; keep the warnings.push user-facing warning unchanged.
---
Nitpick comments:
In `@src/openhuman/flows/n8n_import.rs`:
- Around line 990-992: Update the duplicate-ID assertions in the collision
warning test to verify the quoted token values for IDs “a” and “b” rather than
checking single-character containment, ensuring the assertions fail when ID
substitution is broken.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6880534-2fb7-496b-bbdc-f29f9e5f1de8
📒 Files selected for processing (1)
src/openhuman/flows/n8n_import.rs
| if let Some(previous_id) = name_to_id | ||
| .insert(n8n_name.clone(), Value::String(id.clone())) | ||
| .and_then(|v| v.as_str().map(str::to_string)) | ||
| { | ||
| tracing::warn!( | ||
| target: "flows", | ||
| name = %n8n_name, | ||
| previous_id = %previous_id, | ||
| new_id = %id, | ||
| "[flows] n8n_import: duplicate node name collision — connections will mis-wire" | ||
| ); | ||
| warnings.push(format!( | ||
| "Multiple n8n nodes are named '{n8n_name}' (ids '{previous_id}' and '{id}') — \ | ||
| n8n connections are keyed by node name, so every connection naming \ | ||
| '{n8n_name}' was rewired onto node '{id}' and may now point at the wrong \ | ||
| node. Rename the duplicates in the source n8n workflow and re-import, or fix \ | ||
| the affected edges by hand." | ||
| )); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check existing tracing severity conventions in n8n_import.rs
rg -n 'tracing::(warn|error|info|debug|trace)!' src/openhuman/flows/n8n_import.rsRepository: tinyhumansai/openhuman
Length of output: 274
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- n8n_import.rs logging contexts ---'
sed -n '65,175p' src/openhuman/flows/n8n_import.rs
printf '%s\n' '--- warning-level logging in adjacent flow modules ---'
rg -n -C 3 'tracing::warn!|tracing::error!|tracing::info!' src/openhuman/flows --glob '*.rs'Repository: tinyhumansai/openhuman
Length of output: 50378
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- logging and PII guidance ---'
rg -n -i -C 2 'never log|full PII|PII|secrets|debug/trace|logging guideline|tracing::warn' \
--glob '*.md' --glob '*.rs' --glob '*.toml' . \
| head -n 240
printf '%s\n' '--- exact n8n import warning test/context ---'
sed -n '940,1020p' src/openhuman/flows/n8n_import.rsRepository: tinyhumansai/openhuman
Length of output: 20910
Lower this log to debug or trace. The new path logs user-controlled n8n_name and node IDs. The logging guideline requires debug/trace for new or changed flows. Keep the user-facing warning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/flows/n8n_import.rs` around lines 111 - 129, Change the tracing
call in the duplicate-name handling around name_to_id.insert from warn to debug
or trace, preserving its existing fields and message; keep the warnings.push
user-facing warning unchanged.
Source: Coding guidelines
1 participant
Summary
$jsonexpressions saved cleanly, reported no warnings, and then ran hollow.={{ $json.email }}now translates to=.item.email(was=.email, which resolves tonullat runtime).={{ $json }}becomes=.item(was=.).Problem
The tinyflows expression scope root is
{item, items, run, nodes}(seeexpr_scope_forinvendor/tinyflows/src/nodes/mod.rs). n8n's$jsonmeans the current input item, which the engine binds asitem.The importer translated the
$jsontail directly onto the scope root, so$json.emailbecame=.email— dereferencing a key that does not exist at the root. jq yieldsnullrather than erroring, so:nullat runtime, producing empty tool args and hollow runs.This is exactly the silent-null-binding class the domain's propose/save gates (
validate_binding_resolvability, thedry_run_workflownull-resolution gate) were built to prevent — and the importer bypassed all of them.Worse,
translates_trivial_json_expression_to_jqasserted=.emailandwarnings.is_empty(), so the bug was pinned as intended behaviour.Solution
json_path_to_jqreturns the tail path;translate_exprprepends.itemat the single call site, so the empty-tail case naturally becomes.itemwith no special-casing.={{ $json["first name"] }}→=.item."first name".name_to_idnow push a warning naming both ids, matching how every other approximation in this importer already behaves.Deliberately forward-only. Already-imported flows keep their broken bindings. Auto-rewriting stored graphs was rejected: it cannot distinguish an importer artifact from a hand-authored root-level jq program, so it risks corrupting valid graphs. Detecting affected saved graphs via the existing validation banner is the suggested follow-up.
Worth reviewers' attention:
validate_binding_resolvability's regex only matches=nodes.<ref>.item...cross-node references — it does not statically inspect plain.item.<field>bindings, so that gate would not have caught this on its own. The tests that actually prove the fix evaluate a translated expression against a realistic{item, items, run, nodes}scope, keeping the old form as a negative control that resolvesnull.Submission Checklist
cargo test --lib openhuman::flows::n8n_import= 18 passed, 0 failedN/A: behaviour-only bug fix, no feature rows added/removed/renamed## Related—N/A: no matrix feature rows affectedN/A: importer-internal expression translation, not a release-cut surfaceCloses #NNN—N/A: found by code review, no tracking issue filed yetImpact
openhuman::flows::n8n_import). No frontend, no CLI surface change.Cargo.toml/Cargo.lockchange.Related
N/An8n_import.rs, no file overlap.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/flows-n8n-json-item-bindingca8e4575fValidation Run
pnpm --filter openhuman-app format:check— N/A, no frontend files changedpnpm typecheck— N/A, no TypeScript changedGGML_NATIVE=OFF cargo test --lib openhuman::flows::n8n_import→ 18 passed, 0 failedGGML_NATIVE=OFF cargo check --manifest-path Cargo.tomlcleanapp/src-tauriuntouchedValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
$jsonexpressions translate to the real engine scope root, so imported bindings resolve real values instead ofnull.Parity Contract
$json, untranslatable=-expressions, unmapped node types, extra-trigger demotion) are unchanged and still warn exactly as before.$jsonfast path changed.Duplicate / Superseded PR Handling
Summary by CodeRabbit
$jsonexpression handling for more reliable data access in imported workflows.