fix: Apply AI edits in the subgraph the entity lives in - #2683
Conversation
🎩 PreviewA preview build has been created at: |
camielvs
changed the title
2abbff4 to
d2433d2
Compare
camielvs
changed the title
0c1c357 to
f6c5953
Compare
d2433d2 to
b85983b
Compare
There was a problem hiding this comment.
Reviewed against base 08-31-fix_entity_chips_for_nested_entities. I read every hunk and verified behaviour with throwaway probe tests at the PR head — the resolver plumbing itself is right: locateEntity parent-context chaining, undo/autosave still anchored at the root via mobx-keystone, and port-rename propagation all check out. tsc --noEmit clean, 47 tests in toolBridge.test.ts pass.
Inline findings below. One extra with no line in this diff to attach to: src/routes/v2/shared/components/AiChat/serializeSpecForAi.ts:11 still says "Edits always target the root spec; the path is a hint, not a target" — both prompts and csomBridge's own header were updated in this PR, that one was missed.
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
b85983b to
45533d3
Compare
f6c5953 to
da70de1
Compare
|
On the finding with no line to attach: the stale |
45533d3 to
9b875f1
Compare
morgan-wowk
left a comment
morgan-wowk
left a comment
There was a problem hiding this comment.
🤖 Automated review
Approving. Verified fail-closed throughout: every handler (deleteTask/renameTask/delete|renameInput|Output/unpackSubgraph/deleteEdge/connectNodes/setTaskArgument/createSubgraph) resolves the owning spec and mutates location.spec, not root, and the pre-checks (name collision, not-a-subgraph, cross-boundary connect) short-circuit before the mutation. deleteEdge now returns success only after verifying the binding is actually gone. Port rename/delete propagates to the immediate parent correctly at depth ≥2. Nested edits go through undo.withGroup on the live nested spec, so undo is one step. The old top-graph-only path is fully removed with no straggler callers, and the BridgeResult widening is backward-compatible. Strong refusal-case tests.
| const spec = requireSpec(deps); | ||
| const root = requireSpec(deps); | ||
|
|
||
| const source = resolveConnectable(root, args.sourceEntityId); |
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
High: validate endpoint roles and ports before creating the binding.
resolveConnectable proves only that the id is a non-binding entity; the editor action below assumes React Flow already supplied valid handles. I reproduced this with a source task that exposes only real_output: passing made_up_output to a real required target input returns { success: true }, and validate_pipeline then returns { valid: true, issueCount: 0 }. The serialized task references an output that does not exist. Graph outputs are likewise accepted as sources, and graph inputs as targets.
Please reject invalid source/target kinds and verify task port names against resolvedComponentSpec before mutating (while preserving any supported synthetic ports), with refusal tests. Otherwise the new nested connection path can still create a pipeline that validation considers safe to submit.
There was a problem hiding this comment.
Confirmed, and deferring this one too — the gap is pre-existing and unchanged by this PR.
master's handler was byte-identical on this point: connectNodes only rejects input→output via getNodeTypeFromId, and spec.connectNodes constructs the Binding without consulting either endpoint's ports. This PR added resolveConnectable and the cross-boundary guard on top of that; it did not introduce the port hole. I probed the validation half as well — a binding from made_up_output produces zero issues from collectValidationIssues, because there is no issue code for a binding port that does not exist (ORPHANED_BINDING_SOURCE/TARGET only check that the entity exists).
That last part is why I would rather not do this as a bridge-only guard. The missing check belongs in validateSpec as a real issue code, which would catch the same corruption from the UI and from imported YAML, and would let the agent's own validate_pipeline report it instead of returning valid: true. Doing it only in connect_nodes leaves the pipeline just as submittable by every other route.
The synthetic-port caveat you flagged is the reason it wants its own change rather than a quick guard here: is_enabled shares the targetPortName namespace with real inputs (there is a note on this at validateSpec.ts:278) and aggregator inputs are created outside resolvedComponentSpec, so a naive name check breaks conditional execution and aggregators. Noted here as a follow-up on the validation layer.
| taskEntityId, | ||
| "task", | ||
| (location) => | ||
| unpackSubgraphTask(deps.undo, location.spec, taskEntityId), |
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
High: preserve child subgraphs when unpacking at a nested level.
I reproduced Root > Outer > Middle > Deep > Leaf and called this new nested path on Middle. It returns success and moves Deep into Outer, but Deep now has subgraphSpec === undefined and Leaf has disappeared. The existing addInnerTasks rebuilds children from innerTask.componentRef, while promoted subgraphs intentionally keep their graph only in innerTask.subgraphSpec.
Please rebuild through the promotion path using resolvedComponentRef, or explicitly clone the child subgraphSpec, and add a three-level regression test. Delegating directly here currently makes nested unpack destructive.
There was a problem hiding this comment.
Confirmed the repro, and it is worse than nested-only — I want to defer it rather than fix it here, because it is not this PR making it destructive.
addInnerTasks rebuilds each child as new Task({ componentRef }), and the Task constructor does not promote an inline graph — only setComponentRef does. Every path that produces a subgraph task also strips the graph from componentRef: promoteInlineSubgraph sets spec: undefined, and createSubgraph builds the replacement task with componentRef: { name } and no spec at all. So the graph only ever lives in subgraphSpec, which addInnerTasks drops. unpackSubgraph.ts is untouched by this stack (last changed in #2657).
Which means the loss does not need a nested unpack, or the agent: unpacking any subgraph that contains a child subgraph loses the grandchildren, and UnpackSubgraphButton plus the node menu reach the same function. I verified with a throwaway three-level test — Deep survives with subgraphSpec === undefined and isSubgraph: false, Leaf is gone.
Noting it here for a separate follow-up rather than taking it in this stack. The fix belongs in addInnerTasks — pass subgraphSpec through, or construct from innerTask.resolvedComponentRef — with a three-level regression test, and it needs to cover the UI paths too; a bridge-level guard here would leave the button broken. Treating it as silent data loss, so it goes near the front of the queue.
9b875f1 to
a57d93b
Compare
42cf28b to
44220cc
Compare
## Description Split out of the PR above it. That PR reroutes the AI assistant's edit tools so they act on the graph that actually owns the thing you named, instead of always the top level. Underneath that sits a layer answering a narrower question: given the pipeline and an entity's id, which graph owns it, is it the kind of thing the caller expected, and — when the answer is no — what do you tell the user? That layer is this PR. It's three things: - **Resolve.** Find the entity, confirm it's the kind asked for, hand back the graph that owns it. Connection endpoints resolve slightly differently, since they can be a step or a port but not an existing connection. - **Describe.** Turn a location into a phrase — "step `DropNulls` inside subgraph `Preprocess`" — so every failure can say where it was looking. - **Explain.** Three cases where the reason for a refusal is knowable up front: a rename onto a name already taken in that graph, unpacking something that isn't a subgraph, and a value referencing something that lives in a different graph (which can't be written down in the pipeline format at all). Nothing consumes it yet — the PR above swaps its handlers onto it. Same shape as the resolver PR further down the stack, and split for the same reason: it can be read on its own in one sitting. ## Related Issue and Pull requests Stacked on #2686 → #2685 → #2655. Carved out of #2683 after review, which is down from +999 to +744 as a result. ## Type of Change - [x] Improvement ## Checklist - [ ] I have tested this does not break current pipelines / runs functionality - [ ] I have tested the changes on staging ## Test Instructions Nothing to click through — no caller reaches this code until the PR above. 21 unit tests cover it: resolving at the top level and inside a nested subgraph, each failure message, and the three explained refusals. ## Additional Comments The two type helpers added to `locateEntity` are here rather than in #2685 because they exist to narrow a location to an expected kind, which is this layer's job and has no consumer in #2685. The ratio of message text to logic is high on purpose — explaining refusals in words the assistant can pass on is the point of the work, not a side effect of it.
Every editing tool took the `$id` the assistant gave it and applied the change
to the top-level pipeline, whatever depth the entity was actually at. That had
four visible symptoms, all from the same cause:
- Asking for a nested edit silently did nothing, or hit an unrelated top-level
entity that happened to match.
- `connect_nodes` across a subgraph boundary wrote a binding pointing at an
entity that isn't in that graph, corrupting the pipeline.
- `delete_edge` reported success while deleting nothing.
- Nine tools could only answer `{ success: false }`, so the assistant could not
tell "no such entity" from "that's a task, not an input" and made something up.
Each handler now resolves which graph owns the `$id` and edits that graph. The
editing actions already took the graph to act on as an argument — the editor has
always used that to edit inside a subgraph — so this is mainly passing the right
one. Renames and deletes of subgraph ports now also propagate to the parent, so
the wiring above survives.
Refusals that remain are structural, not depth-related, and now say what to do
instead: a connection can't cross a subgraph boundary, and `create_subgraph`
can't group tasks from different levels.
Validation issues now carry `entityName` and the `subgraphPath` locating them,
so the assistant can act on an issue found inside a subgraph.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four follow-ups to applying edits in the owning subgraph: - `set_task_argument` now checks that a graphInput / taskOutput reference resolves in the same graph as the task being written to. Writing a root input reference into a subgraph used to return success and produce YAML that fails to load. `taskOutput.taskId` accepts an $id as well as a name (the format stores the name, every prompt says to use $ids) and normalizes. - `create_subgraph` requires two distinct task ids. The guard only rejected an empty array, so one id — or the same id twice — wrapped a single task, contradicting both the tool description and the handler's own error copy. - Renames and unpacks explain themselves. A name collision and "that isn't a subgraph" are both knowable before the mutation runs; the generic "could not be applied" is now the branch we genuinely cannot explain. - `subgraphPath` reaches the model without its "root" prefix, matching `activeSubgraphPath` so the two can be correlated. Shared with RunView via `toValidationResult`, which also drops the duplicated mapping. Also corrects the architect / repair prompts, which claimed every edit tool resolves an $id — `add_task`, `add_input` and `add_output` take none — and a stale `serializeSpecForAi` header still saying edits always target the root. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a57d93b to
1348fb1
Compare
## Description The PR below this one made the assistant change existing things wherever they live. Creating something new still only worked at the top level, which left one job impossible: getting a value into or out of a subgraph. That needs a new port on the subgraph itself, and there was no way to ask for one. Adding a step, an input, or an output can now target a subgraph. Ask for something without saying where and nothing changes — it goes to the top level as before. Name a subgraph, or open one and say "add a filter step here", and it goes inside that one instead. An input added inside a subgraph shows up as a new input port on that subgraph step, which is how you feed a value into it. If you name something that isn't a subgraph, it says so and tells you how to add at the top level instead, rather than failing quietly. **Getting a value in or out now finishes the job.** A port on the boundary is only a third of the work — it also has to be wired to the step inside the subgraph that uses the value, and to whatever feeds it in the parent. Asked for this, the assistant used to create the port and stop, leaving a value that reaches nothing and two fresh validation errors where there had been none. It now does all three, so "feed this into \<subgraph>" is a single request that ends with a working pipeline. Two smaller corrections in the same area. Asked to fix a problem while you happen to be looking at a subgraph, it no longer drops the fix into that subgraph — repairs land on whatever they're repairing, wherever that lives. And it now gets the subgraph you're viewing handed to it directly rather than matching on its name, which was ambiguous whenever two subgraphs at different levels shared a name.  ## Related Issue and Pull requests Stacked on #2683 → #2687 → #2686 → #2685 → #2655. Re-cut alongside #2683 — see that PR for why. This one is now purely the new capability: it adds to the PR below rather than rewriting it. ## Type of Change - [x] New feature ## Checklist - [ ] I have tested this does not break current pipelines / runs functionality - [ ] I have tested the changes on staging ## Test Instructions 1. Open a pipeline with a subgraph. Ask the chat: "add a \<component> inside \<subgraph name>". Open the subgraph and confirm it's there, and that it did not also appear at the top level. 2. Open a subgraph, then say "add an input called threshold here" — it should go into the subgraph you're looking at, not the top level. 3. Go back up: that subgraph step should now have a `threshold` input port on it. 4. Ask for the whole route in one go — "feed \<some top-level value> into \<subgraph name> so \<inner step> can use it". It should end with the port wired on both sides: connected inside the subgraph to the step that uses it, and connected in the parent to the value you named. Ask it to validate afterwards — there should be no new errors, and in particular no unconnected port left behind. 5. Same again with an output — it should appear as an output port on the subgraph step, and the route out should get wired end to end the same way. 6. Ask for something with no location at all ("add a \<component>") while looking at the top level — it should go to the top level, as before. 7. Ask it to add something "inside" a step that isn't a subgraph — it should explain that step isn't a subgraph rather than silently doing something else. 8. Open a pipeline with a validation problem on a top-level step, then open a subgraph before asking the chat to fix it. The fix should land on the top-level step, not inside the subgraph you happen to be looking at. 9. Undo (Cmd/Ctrl+Z) after a nested add, and confirm the pipeline still saves. ## Additional Comments The earlier version of this PR also returned the subgraph trail on every single edit so the assistant could quote it back. That's been dropped: the assistant already knows which subgraph it targeted, because it passed the id, so the extra field was duplicating information it had. It's still told to name the subgraph it changed in its reply — the canvas doesn't move, so you need to be told where to look.
3 participants
Description
When you asked the AI assistant to change something inside a subgraph, it applied
the change to the top-level pipeline instead — whatever depth the thing you named
actually lived at. One cause, four ways of showing up:
quietly did nothing — or worse, hit an unrelated top-level step that happened to
match.
subgraph boundary, it wrote a connection pointing at a step that isn't in that
graph. The pipeline format has no such connection, so the result was broken.
work", with no way to say whether the thing didn't exist or was a different kind
of thing than expected — so the assistant guessed, and often told you something
untrue.
Each edit now works out which graph owns the thing you named and changes that
graph. This turned out to be mostly plumbing that was already in place: every
editing operation already took the graph to act on as an argument, and the editor
itself has always used that to let you edit inside a subgraph. The assistant was
the only caller that always handed over the top-level pipeline.
The layer that answers "which graph, and is this the kind of thing you asked
for" — along with the wording of every refusal — now lives in #2687, directly
below. This PR is the rewiring: swapping each handler onto it, and the knock-on
fixes below.
Also fixed here, because they're the same underlying thing:
The matching port on the subgraph step is renamed or removed with it, so the
connection feeding it in the parent survives.
the name and the trail to the subgraph they're in, so the assistant can act on
them instead of reporting a problem it can't locate.
corruption as the connection case above, through a different door: asked to set
a step's input to a value coming from another graph, it used to write it and
produce a pipeline that no longer loads. It's refused now, with the way round
it.
— or the same step named twice — it used to do it, despite saying elsewhere
that it wouldn't.
Two refusals remain, and both are about structure rather than depth, so they're
correct rather than missing. Both now say what to do instead:
Where a refusal has a knowable cause, it now names it — a rename that collides
with an existing name, or being asked to unpack something that isn't a subgraph.
The vague "couldn't do that" is what's left when there's genuinely nothing to
say, rather than the standard answer.
The canvas doesn't follow the assistant into a subgraph. A single request can
produce several edits across several subgraphs, and yanking the view around for
each one would lose wherever you were. The assistant is told to name the subgraph
it changed, so you're told where to look rather than being taken there.
Related Issue and Pull requests
Stacked on #2687 → #2686 → #2685 → #2655.
This PR and #2684 were re-cut. Previously the two split this fix down the middle:
the lower one described the failures and refused them, the upper one rewrote it to
actually fix them. Since all four symptoms above share one cause and one fix, that
seam meant ~635 lines were written twice and reviewed twice to net zero. The stack
is now split by concern instead: shared resolver (#2685), an unrelated chip bug
(#2686), this fix, then the new capability on top (#2684).
Type of Change
Checklist
Test Instructions
ask the AI chat for edits inside it: "rename the second step in <subgraph
name>", "delete the connection between X and Y in <subgraph name>", "set the
input file on <nested step>". Each should apply, and the reply should say which
subgraph it changed.
should still save normally.
the parent: the port should be renamed and still connected, not orphaned.
<a step inside a subgraph>" — and it should explain that a connection can't
cross a subgraph boundary and offer the way round it. Same for asking it to
group a top-level step together with a nested one.
("delete the connection <id of a step>"). It should say what's actually wrong
rather than inventing a reason. Same for renaming a step to a name already
taken in that graph, and for "unpack <a step that isn't a subgraph>".
top-level pipeline — it should refuse and explain, not write it. Then ask it
to group a single step into a subgraph, which it should also refuse.
report them and say where they are.
Additional Comments
Nothing new was needed for undo or saving: both already watch the whole pipeline
including everything nested, which the editor has always relied on.
One pre-existing quirk left alone: renaming a subgraph step doesn't update the
name stored on the subgraph's own contents. Nothing depends on it, and the same
thing happens when you rename a subgraph step by hand in the editor, so it's not
new here — but it's worth a separate look at some point.