Four small things the whole-branch review found, none of which changes what a
guard decides.
`_change_not_applied`'s docstring said the gate's other two exits "both clear
this store". Task 7's fix round moved the clear to `handle_impact_analysis` and
made it conditional on the rules actually changing, so neither
continue_adjusting nor revert clears it -- and two tests assert exactly that. A
maintainer trusting the comment would re-add the clear to the revert path,
which is the bug Task 7 removed. Reworded to name the sole, conditional clear
and the two tests that pin it.
`already_present_predicate` keyed a connect on `(from, to)` alone, so a repair
that re-routes an arm of a branch between the same pair of nodes matched the
edge already there and was dropped as a no-op -- in the dry run and at
`apply_repair`, which runs the same predicate. The handle is now part of the
key on both sides, defaulting to "source" the way graphon reads a missing one.
`_build_node`'s defaults merge was shallow, so `model: {provider, name}`
replaced the whole default block and the node was refused for `model.mode` --
spending the one corrective re-prompt on a field the default was carrying. It
now merges one level into a dict-valued default, with one exclusion that the
suite caught immediately: a block whose `type` the caller did not write is left
alone, because `type` reinterprets its siblings and completing a partial
`authorization` from the default produces a node that looks authorized, sends
no credential, and is written in silence.
`_withheld_args` redacted a `value` shaped like a whole node `data`, which is
not what `set_node_config` sends: `path="headers"` with a header line reached
the retry prompt unredacted. The value is now also redacted as what it is --
the value at that path -- by wrapping it back into its own nesting and running
the one redaction profile over it, so `credentials` stays the single definition
of what a credential is. Unreachable with a stored secret today.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Asked to ADD a case to an if-else, the live S6 edit re-sent the whole `cases`
array -- the only way to append one -- and regenerated the case it was not
asked about: the condition's `id` and `varType` were lost and `= 60` became
`≥ 60`. The engine accepts all of it (graphon's `Condition` has no `id` field
at all, and `≥ 60` is a perfectly valid condition), `diff_graphs` reports one
line, and the user's threshold has silently changed meaning.
`vet_intents` now refuses a `set_node_config` that replaces an existing array
with another array from which an element identity (`id`, `case_id`) present
before is absent after, naming the ids that went missing. The comparison walks
the whole subtree, because the live clobber was one level down: both `case_id`s
survived and the surviving case's CONDITIONS were regenerated.
The check is scoped by (node type, path) to the arrays where those keys MEAN
something, each verified against the engine: if-else `cases` and
`cases.*.conditions` (`Case.case_id` is required and is the edge sourceHandle),
question-classifier `classes` (`ClassConfig.id` is required and documented as
the routing handle), and knowledge-retrieval
`metadata_filtering_conditions.conditions` (the canvas mints an id per
condition and a regenerated filter silently changes what is retrieved).
Unscoped, the rule refused an ordinary edit: an llm `prompt_template` message's
`id` is a frontend uuid the canvas REGENERATES when absent and graphon's
`ChatModelMessage` has no field for, so rewriting a prompt -- the commonest
Builder request there is -- looked exactly like the S6 clobber and parked the
user's change. Inside a scoped array the walk stays recursive, which is what
catches S6's nested condition id.
The op schema already tells the model to re-send existing elements
byte-identical; this is the half that does not depend on it obeying. The
neighbouring shapes stay untouched: an append that repeats every existing
element, a change addressed by index (which writes a scalar, never an array),
and an array whose elements never carried an identity -- a variable-aggregator's
`variables`, which the rejoin guard requires be re-sent whole.
A DELETION is not a clobber either. Removing an if-else case is a legitimate
edit and a whole-array write is the only way to express one, so a guard that
refused it would make the feature unusable -- loudly, but unusably. The two
shapes are distinguishable without asking the model what it meant, which is the
only reason the exception exists: a deletion leaves every element it KEEPS
byte-identical to one of the originals, each claimed once, and keeps fewer than
it was given; a regeneration loses an identity in the same write that modified
or added something else. The live S6 batch is the second kind and is still
refused, as is the mixed write that deletes one element while editing another.
The comparison runs against a HEALED copy of `before`, because `after` is the
dry run's already-healed graph: a draft holding `">="` or a JSON `60` -- the
ESQ1-303 shape -- would otherwise make a survivor the model re-sent verbatim
look modified, and a genuine deletion would be refused for a difference the
heal set introduced. Healing can only make two elements more equal, so it can
only reduce false rejections.
`graph_ops.value_at_path` is the read half of `apply_set_node_config`, through
the same `_resolve_path` walk, so the before/after comparison cannot disagree
with the write about which slot a path names.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
At edit.plan_approval the gate re-reads an UNCHANGED draft, so edit_rules and
graph are byte-identical on every re-approval, and _change_not_applied stored
nothing about WHY the port refused the write. The live F4 user approved the
same plan three times and got a byte-identical card each time.
Add DifyBuilderContext.last_edit_rejection: the raw text of the
DraftWouldNotStartError/ValueError the port raised, capped at 2000 chars,
stored by _change_not_applied and passed into build_edit_intents, which quotes
it into the user prompt as a "PREVIOUS ATTEMPT REJECTED BY THE ENGINE" block.
It is engine-sourced by construction -- a PreflightError's every message is
built by re-validating a credentials.redact_node_config copy of the node, and
graph_ops' ValueErrors carry only paths, ids and type names -- so no model
prose enters. Nothing branches on it. Cleared on a successful write, on a new
edit goal, and by the recovery reset.
Close the silent false success that left, rather than only recording it.
new_preflight_problems now takes the ids whose NODE DATA the batch wrote and
gives them no exemption at all: a node this batch wrote must be startable, not
merely no worse than it was. Keyed on locations alone, a repair that swaps one
bad value for another at the same field path -- or fixes one of three missing
fields -- produced nothing new, was written, and the card said "Applied the
changes" over a draft that dies at Graph.init. Since a repair's culprit is by
definition already invalid, that was Fix's main line. Every OTHER node keeps
the location-keyed exemption, so a defect the user left on the canvas still
cannot veto an unrelated change.
That set is deliberately narrower than a change set, on both sides (graph_ops
.DryRun gains the field filter_applicable already computed and discarded;
apply_repair keeps a separate written_nodes beside changed_nodes). connect is
excluded, because apply_connect reports both endpoints and an edge cannot
change either one's validate_node_config verdict -- counting them would let
merely wiring an already-broken node veto the batch. The heal is excluded too,
because heal_nodes_for_preflight scans the whole graph and a node it merely
normalized is not one this batch is answerable for.
Also, from the reviews of preflight.py: route an intent's config/value through
redact_node_config before a structural rejection line inlines it; key a
non-ValidationError crash on its truncated message as well as its class, and
keep that message out of the text the model is shown; and correct the
_CRASH_LOCATION comment -- verified against graphon, the rule is a config key
present AND type absent-or-invalid.
Finally, stop the docstrings overclaiming: redaction covers authorization,
headers and params, and a secret typed into a request body value is NOT
covered and reaches the stored text. Widening the profile is the wrong fix --
it shares the sentinel guard that refuses any write carrying REDACTED, so it
would make http-request bodies uneditable. Recorded in credentials.py as a
follow-up needing a separate persistence-only profile.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`filter_applicable` dry-ran STRUCTURE only. In the live F4 Edit four intents
came back "applicable, 0 rejected", the batch then died at `apply_repair`'s
preflight, and the one corrective re-prompt could never fire -- it is keyed on
`rejected` being non-empty. Every re-approval re-sent a byte-identical prompt
and got a byte-identical batch back.
`filter_applicable` now returns its dry-run graph (`DryRun` NamedTuple) after
running it through the shared `heal_nodes_for_preflight` -- the same
normalizers, at the same point in the sequence, the write chokepoint runs. A
dry run that healed less would report a defect the write was going to fix; one
that healed more would pass a batch the write refuses. It also skips the
intents the port drops as already-present, via a predicate both now share, so
a re-sent duplicate `create_node` no longer burns the re-prompt.
`preflight.vet_intents` then puts that graph through `validate_node_config` and
feeds any NEW problem into the existing re-prompt, quoting the engine's own
message. `new_preflight_problems` is the one definition of that rule, shared
with `apply_repair` so the dry run and the write cannot answer differently.
It is keyed on the set of pydantic error LOCATIONS per node, which is the only
key that gets both halves right. Node identity exempts an already-refused node
from every later verdict -- and since a Fix repair targets a node that is by
definition already invalid, that would leave `propose_repair` with no node-data
coverage on its own culprit. Message text is too fine: pydantic quotes a
truncated repr of the input, so filling one of three missing fields rewrites
the other two messages and an exact-text diff refuses the repair that improved
the node.
The refusal text is rebuilt from a credential-redacted copy of the node. The
verdict still comes from the real one; only the words the model is shown are
withheld, because a model-level pydantic error echoes the node's whole `data`
and that is where a live bearer token sits.
The retry budget is unchanged: one corrective re-prompt, then Edit's existing
fallback to the first attempt's applicable intents and Fix's `_no_fix()`.
Also folds in the Task 1 review: the generator's postprocess calls the shared
heal set plus `derive_if_else_var_types` instead of re-spelling three
normalizers inline, and the valid-operator no-op sweep covers the
list-operator container so `FilterOperator` gets its own check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Edit prompt showed each node as title + type + handles and never its
`data`, so an LLM asked to add one if-else branch had nothing to copy from:
it regenerated the whole `cases` array, silently turned `= 60` into `>= 60`
and dropped `id` and `varType`. `diff_graphs` reported the lot as one line,
"cases updated". Triage edit-branch-failure-2026-09-22 cause (c).
`_graph_context` now inlines the JSON `data` of the nodes impact analysis
named, plus their direct neighbours -- and one hop further past a BRANCH
target's successors, because a branch's arms reconverge. In the live
failure the if-else fed two template-transforms that fed a
variable-aggregator, and that aggregator's `variables` is what says whether
the new arm reaches the End node at all; at one hop it rendered as a bare
one-liner the model could not extend, which is cause (d) and ends in "All
checks passed" on empty output. The extra hop is taken only past a branch,
so a long linear chain does not drag the whole graph into the prompt.
Every other node keeps today's one-line form.
Keys still sitting at their type's default are subtracted, with
`node_defaults` as the sole source of what "default" means: without that an
http-request node's two dozen method/auth/timeout/retry keys crowd the
field the edit is actually about out of the prompt. A config over the
1500-character budget sheds whole trailing keys and then names them, rather
than cutting the JSON mid-string where a purpose field that happened to
serialize last would vanish without a trace.
That subtraction is also what would have un-hidden every credential in the
node: a live bearer token never equals the `no-auth` / `""` default, and
Edit renders the target AND its neighbours, so the exposure would be wider
than Fix's one culprit node. `credentials.redact_node_config` therefore
withholds the `authorization` config and every non-empty `headers` /
`params` value behind an exact sentinel, keeping the KEYS visible so the
model can still reason about which header exists. Every non-empty value,
not the suspicious-looking ones: deciding by name means a denylist, and a
denylist misses `X-Company-Token`.
Fix had the same leak and it is closed here too, not scheduled: a failing
http-request node is one of the likeliest culprits there is, and
`fix._culprit_config` inlined its whole `data` raw.
Redaction alone would be worse than the leak -- the model reads the
placeholder, rewrites `headers` around it and destroys a working
credential -- so `validate_intent_args` refuses any `value` or `config`
carrying the sentinel. That is the one chokepoint both the
`filter_applicable` dry run and `apply_repair`'s write loop pass every
intent through, so the write is rejected with a reason the existing
corrective re-prompt can read and the stored secret stands. The refusal is
keyed on the exact generated string, never on model prose. It also covers
`create_node` / `insert_between`, which carry node data by the same route.
A rewrite that merely OMITS the credential line still passes, and the
comment beside the guard records why that is accepted: it fails loudly at
the next verify run, where the leak closed here was silent.
`build_edit_intents` grows two keyword-only, defaulted parameters:
`edit_target_node_ids`, threaded from `fc.edit_target_node_ids` by
`handle_plan_approval` and used here, and `last_edit_rejection`, declared
but not yet read so the task that feeds a refused write back into the
prompt does not have to change this method's arity a second time. Both
default to empty, so an agent that passes neither gets today's behaviour.
`edit_target_node_ids` is typed `Sequence[str]` rather than `list[str]`:
mypy rejects `list[str] = ()` as an incompatible default, and widening the
annotation keeps the `()` default and the keyword-only shape intact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An Edit LLM writes only the fields it was thinking about. graphon requires a
handful per node type with no default, so `create_node{template-transform,
{title, template}}` produced a node the draft preflight refused -- and the
refusal killed the WHOLE repair batch, parking the session at
`edit.plan_approval` with an identical card on every approval (triage
`edit-branch-failure-2026-09-22` cause (b)).
Extend the node-type default registry to every type the Build/Edit/Fix agents
may create -- template-transform, code, http-request, variable-aggregator,
answer, question-classifier, if-else, tool -- each taken from the matching
`web/app/components/workflow/nodes/<type>/default.ts` and each cited alongside
the graphon field it covers. Add `default_config_or_empty`, a non-raising
lookup: a type the registry does not cover contributes no defaults rather than
aborting the edit that is creating it.
Merge those defaults UNDER the caller's config at `graph_ops._build_node`, the
one construction point every created node passes through. A field the caller
supplied always wins; only a missing one is filled. That covers Build, Edit and
Fix, `create_node` and `insert_between`, and the `filter_applicable` dry run,
so the dry run sees exactly the node the port will write.
Defaults fill MISSING required fields; a field that is present but WRONG stays
`graph_normalizers.heal_nodes_for_preflight`'s job.
Two editor-only keys are deliberately not copied. `_targetBranches` (if-else,
question-classifier) is frontend state the canvas regenerates from
`cases`/`classes`, which is also where `declared_branch_handles` reads handles
from. A caller-supplied `cases`/`classes` wins the merge while a default
`_targetBranches` would not, so shipping it would attach a stale branch list to
every Builder-created branch node.
Move the registry from `services/dify_builder/` to `core/dify_builder/`: it was
imported by `core/dify_builder/placeholder_agent.py`, against the constraint
that `core/` must not import from `services/`. The dependency now points the
legal way, and the module stays pure -- copy, typing and graphon only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
apply_repair runs the same graph_normalizers the generator's postprocess
runs, for the intents the generator never saw: a Fix/Edit set_node_config
that writes a condition value as a JSON number (ESQ1-285's flip-flop) or
an http body item without type is healed before the preflight would
reject it.
apply_connect rejects an edge from a branch node on a handle the node
does not declare -- including the default "source" it used to fill in
-- with the same ValueError the handlers already surface as "Couldn't
apply the fix". Such an edge hangs off nothing and its arm never runs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous ESQ1-288 fix dropped the iteration/loop start marker, stripped
start_node_id from the container config and dropped the generator's container
entry edge. All three are required by the ENGINE, not just the canvas:
IterationNodeData.start_node_id / LoopNodeData.start_node_id have no default,
node data is validated eagerly at graph construction, and the body frame is
rooted at start_node_id. Every container workflow the Builder produced was
therefore non-runnable -- worse than the canvas crash it replaced.
The real root cause was never the marker node. The generator already emits it
with the correct node-level ReactFlow type ("custom-iteration-start" /
"custom-loop-start", runner.py:1350-1362); _build_node hardcoded "type":
"custom" and threw it away, leaving a node the frontend has no component for
(React error #130).
Carry it through instead, the same way the previous commit carried
parentId/position: a defaulted `flow_type` on _build_node / apply_create_node
(absent -> "custom", so every existing caller is byte-identical), set from the
source node's type in to_intents. The marker, start_node_id and the entry edge
are all persisted again.
Tests: the three tests that asserted the ABSENCE of the marker, start_node_id
and the entry edge are replaced by ones asserting their presence, plus the
round-trip test that would have caught this -- to_intents -> filter_applicable
-> APPLY_FNS -> IterationNodeData/LoopNodeData.model_validate on the persisted
container.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
to_intents read only id and data, so every node-level ReactFlow key --
parentId, extent, zIndex, position -- was dropped on each Builder build.
Iteration and loop children were written un-nested, not inside their
container at all, and the generator's computed layout was replaced by
default positions. The data-level markers (isInIteration, iteration_id)
survived via config, which is why the graphs looked almost right.
apply_create_node already took position; parent_id is new and implies
extent="parent" + zIndex=1002, matching what the generator computes.
Unfiled; found while tracing ESQ1-288. Skipping the synthetic start node
stops the canvas crash, but an iteration whose children are not inside it
still does not work.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An LLM-supplied int node_id reached diff_graphs, which drops falsy and
non-string ids from its node maps, so result.changes came back empty and
build_change_set's fallback joined a list containing an int. The
TypeError took the whole session to `failed` (ESQ1-271) after the
mutation had already been committed to the draft.
validate_intent_args now rejects a non-string or empty node id, so
filter_applicable turns it into a rejection reason the agent can act on.
build_change_set str()s defensively -- that join must never be what kills
a session.
Also extends the same guard to set_node_config's `path` arg: a non-string
path used to reach _resolve_path's str.split() and raise AttributeError,
surfacing "'int' object has no attribute 'split'" to the repair agent
instead of an actionable rejection reason. Same boundary, same defect
shape, deliberately included here rather than deferred.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
apply_set_node_config wrote node["data"][path] as a flat key, so every
dotted path the repair agent emits (cases.0.conditions.1.value) created a
junk key the workflow engine ignores. The dry run passed, the change set
printed a plausible line, and the graph never changed -- which is why
ESQ1-285 survived nine approvals and ESQ1-290 six.
Paths now resolve through lists and mappings, and an unwalkable path
raises. Raising is half the fix: filter_applicable dry-runs intents
through the real apply_* and feeds rejection reasons back to the repair
agent, so a bad path now produces corrective feedback instead of a
silent no-op.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
filter_applicable's per-intent try/except only caught (ValueError, TypeError),
so a hallucinated insert_between with a non-dict edge arg raised an uncaught
AttributeError and crashed the whole advance instead of being rejected by the
dry-run. Broaden to except Exception (ruff does not flag it, so no noqa is
added). Add a regression test for the string-edge case.
Also update llm_agent.py's stale module docstring (Fix cognition is now real,
not delegated; Protocol has 14 methods, not 13) and fix a stray closing paren
in a test_handlers_fix.py comment.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Behavior-preserving code-quality cleanup across the Dify Builder engine:
extract emit_canvas, build_change_set, mint_checkpoint, action_kind, and
merge_known_keys into handlers_fix.py (shared by handlers_build.py and
handlers_edit.py); extract _add in repository.py for the id-preservation
idiom; extract _make_edge (and unify the node/edge id-collision loops
into _next_unique_id) in graph_ops.py; extract _sync_graph_only in
dify_port.py. No behavior, generated ids, or wire output changed — same
387-test suite passes, ruff clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Charles Yao
twwu
