fix: apply pre-flatten passes to imported subgraphs - #2263
Open
noron12234 wants to merge 1 commit into
Open
Conversation
Contributor
Author
|
Closing for now — I want to re-check this myself before asking for review. Will reopen once I'm confident. Sorry for the noise. |
`load_graph_from_uri` only deserializes a subgraph file, and `process_subgraph_node` handed the result straight to the flattening logic. The three passes that `flatten_graph` runs before flattening - `names` expansion, selector resolution and reversed connection conversion - were therefore never applied to an imported subgraph, only to the top-level one. A subgraph using any of those three features was mis-compiled: - `source`: the reversed connection was left unconverted and its extension name kept its pre-prefix value, so it referenced an extension that no longer existed after flattening. No error was reported, the connection was silently dropped at runtime. - `names`: the array reached the flattening logic unexpanded, where `name` is expected to be `Some` and `names` to be `None`. - `selector`: the selector node reached the flattening logic and triggered the `Unexpected non-extension node in flattened subgraph` panic. Extract the three passes into `Graph::apply_pre_flatten_passes` and run them on a subgraph right after it is loaded. The other `load_graph_from_uri` caller already runs them via `flatten_graph`.
noron12234
force-pushed
the
fix/subgraph-pre-flatten-passes
branch
from
July 31, 2026 07:59
0eed2fb to
e0802d1
Compare
Contributor
Author
|
Reopened and rebased onto current Verified the bug is still present. Applying only the new tests to
With the fix, all 87 tests under On the earlier CI failure: it failed at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Graph::flatten_graphruns four passes over a graph:expand_names_to_individual_items—names: [...]into individualnameitemsflatten_selectors— selector references into the nodes they matchconvert_reversed_connections_to_forward_connections—sourceintodestflatten_subgraphs— inline imported subgraphs, prefixing names with the subgraph namePasses 1-3 were only ever applied to the top-level graph.
process_subgraph_nodeloads an imported subgraph withload_graph_from_uri, which only deserializes the file, and hands it straight to pass 4. A subgraph that usesnames, aselectornode, or a reversedsourceconnection therefore reaches the flattening logic unlowered, even though that logic assumes all three passes have already run.Reproduction
sub.json— inside the subgraph,ext_asendshellotoext_b, written in reverse form:{ "nodes": [ {"type": "extension", "name": "ext_a", "addon": "addon_a"}, {"type": "extension", "name": "ext_b", "addon": "addon_b"} ], "connections": [ {"extension": "ext_b", "cmd": [{"name": "hello", "source": [{"extension": "ext_a"}]}]} ] }Main graph:
{"nodes": [{"type": "subgraph", "name": "sg", "graph": {"import_uri": "file://.../sub.json"}}]}Expected after flattening:
sg_ext_a --hello--> sg_ext_b.Actual on
main:{ "nodes": [ {"type": "extension", "name": "sg_ext_a", "addon": "addon_a"}, {"type": "extension", "name": "sg_ext_b", "addon": "addon_b"} ], "connections": [ {"extension": "sg_ext_b", "cmd": [{"name": "hello", "source": [{"extension": "ext_a"}]}]} ] }The connection is still in reverse form, and its
sourcestill namesext_awhile the node has been renamed tosg_ext_a. It now points at an extension that does not exist, and nothing reports an error.Impact per feature
mainsource(reversed connection)names: [...]nameis expected to beSomeandnamesto beNone.selectornodeUnexpected non-extension node in flattened subgraph(subgraph/flatten.rs:493).The
selectorcase is the most visible one: this code path is reachable from the tman designer HTTP backend and from the C runtime throughten_rust_graph_validate_complete_flatten, so a malformed-but-plausible subgraph file takes down the process instead of returning an error.Fix
Extract passes 1-3 into
Graph::apply_pre_flatten_passesand run them on a subgraph immediately after it is loaded, so an imported subgraph goes through the same lowering as the top-level graph.flatten_graphnow calls the same helper, so there is a single definition of what has to happen before flattening.The other
load_graph_from_uricaller (GraphContent::validate_and_complete_and_flatten) already reaches these passes throughflatten_graphand needed no change.Tests
Three tests added to
tests/test_case/graph/subgraph.rs, one per feature:test_flatten_subgraph_with_reversed_connectiontest_flatten_subgraph_with_names_arraytest_flatten_subgraph_with_selectorVerified they fail on
mainand pass with this change:The rest of the graph suite is unaffected —
graph::reverse(14),graph::flatten_integration(4),graph::names_expansion(6),graph::selector(2),graph::exposed_message(10),graph::graph_info(5),graph::import_uri(6) all still pass.Possible follow-up
validate_and_completeis also never run on an imported subgraph, so the app-URI rules are not enforced there. I left that out of this PR because it changes which graphs are accepted rather than fixing a mis-compilation. Happy to send a separate PR if you would like that tightened too.