Parity for the two semantic guards, which Task 8 committed to for exactly this
reason: a guard Edit honours and Fix merely implies is worse than no guard,
because the inconsistency is invisible to whoever reads only one of them.
The hole was not the one I reported at the end of round 2. Probed against the
literal S6 batch, `fix.propose_repair` already refuses to hand on a batch
carrying any rejection -- unlike Edit it never had a fallback to the first
attempt, so the condemned batch was never written. What it did have was a BLIND
surface: the engine-grounded reason was discarded and `_no_fix()` returned the
generic "No safe automatic fix found", which the handler then stored on
`fc.risk` and showed to nobody. The human at the gate was told a fix could not
be found and given nothing to act on -- while the guard had just described the
aggregator, the node and the selector that would have made a manual fix obvious.
So: `_no_fix()` takes an optional reason (one dead-end path, not a second), a
`would_run_wrong` verdict fills it in, and `handle_propose` renders it on an
ErrorCard beside the reply line -- the same division of labour Edit's refusal
uses. The verdict is read off the structured `would_run_wrong` list, never off
the rejection prose, and deliberately NOT off `rejections` at large: those
inline whole `intent.args` and belong in a re-prompt, not on a card, and a
structural refusal says nothing about the batch that remains. A purely
structural refusal therefore still ends at the generic sentence, as before.
The `would_run_wrong` check is written as its own line rather than left to the
broader rejection check it currently duplicates. That redundancy is a property
of the line below it, not of the guard: if Fix is ever loosened the way Edit
was, this is what still holds -- and it is what carries the reason either way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two semantic guards describe defects the ENGINE ACCEPTS: a new branch wired
into a variable-aggregator that was never told about it, an array re-sent with
an element quietly dropped. Both drafts start, both run green, and neither
produces a `DraftWouldNotStartError` for `apply_repair` to veto. So the guards
had one hole left, and it was in the agent's own fallback:
`build_edit_intents` spent its corrective re-prompt on the guard's reason and
then, if the retry yielded nothing usable, returned the FIRST attempt's
applicable intents -- the very batch the guard had condemned. The guard fired
and the silently-wrong draft was written anyway.
`vet_intents` now carries `would_run_wrong` alongside `rejections`: the subset
of refusals the engine does not make. The two kinds fail differently and a
caller has to tell them apart without reading the text -- a structural refusal
DROPPED its intent, so what survives is a batch nobody judged wrong and keeping
it is a deliberate choice; a semantic verdict drops nothing and condemns what
remains. Reading that off the prose would be the same mistake as keying control
flow on the model's prose.
`build_edit_intents` therefore raises `ProposalWouldRunWrongError` instead of
returning a batch still carrying one, whichever attempt it came from. Nothing is
written and nothing is attempted. `handle_plan_approval` catches it into the
same `_change_not_applied` recovery a refused write gets: the plan stays at its
gate, the reason goes on an error card and into `last_edit_rejection` for the
next attempt.
Surfacing is a real exit now and not the dead end it would have been before this
plan: Task 7 gave the gate a revert and a keep-adjusting action, and Task 6
carries the refusal into the next proposal, so the user can change the rules and
the retry is not blind.
`ProposalWouldRunWrongError` is deliberately neither a `DraftWouldNotStartError`
nor a `ValueError`: the draft WOULD start, and a broad `except ValueError`
around a port write must not swallow it as if the write had been attempted.
`_change_not_applied` takes the activity to mark failed, defaulting to the write
where the usual refusal happens. This exit gives up during "prepare", so failing
the write would have shown a LATER step failed while an earlier one was still
running, with the step between never resolving.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
edit.plan_approval offered exactly one action, approve_plan, while the card
its own refusal emitted said "Adjust it and approve again" -- an affordance
that did not exist. A chat message kept the PC at the gate and never touched
edit_rules, so an approval the engine refused had no exit at all.
The gate now also offers Continue adjusting and Discard this change plan,
admitted end to end: _ACTIONS_FOR renders them, _BACKEND_ACTIONS_FOR
[EDIT_PLAN_APPROVAL] admits re_fix and undo, and handle_plan_approval routes
both past the early return that used to swallow every non-approve kind. The
discard reuses perform_revert against the checkpoint handle_impact_analysis
already mints; Continue adjusting reuses handle_review's re_fix body, now
shared as _resume_adjusting, which returns to edit.impact_analysis with the
edit-rules form repopulated and unfrozen.
last_edit_rejection is cleared where its cause is: in handle_impact_analysis,
and only when the submitted payload actually changes fc.edit_rules. A stored
refusal goes stale precisely when the rules it refused stop being the rules,
and nowhere else -- so the actions that merely route the user to the form do
not clear it, and neither does edit.reverted's Retry. A revert taken at the
gate writes nothing, which makes it the one path reaching edit.reverted with
a refusal still true of the restored draft; clearing there would have made
Retry re-propose the identical plan blind.
Copy corrected in strings.py and handlers_edit.py to name the three buttons
that now exist.
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>
The question-classifier default shipped two `classes` with blank names. That
turns a loud preflight refusal into a node that is written, reported as a
success, and silently classifies into two meaningless categories -- and the
fabricated ids `["1", "2"]` pin `declared_branch_handles`, so a semantic
`connect` is dropped while the create survives. That is the failure shape the
registry exists to eliminate.
Draw the line explicitly and apply it to every entry: a default may supply a
field whose empty value is semantically NEUTRAL -- a container, a mode, a config
or auth block -- but never one that carries the node's PURPOSE, where an empty
value produces a node that RUNS and yields nothing. Dropped, with the engine
behaviour each empty value would have produced:
end.outputs End runs OK, so run_finished_without_output is
False -> "All checks passed" on an empty result
answer.answer streams nothing
template-transform.template renders ""
variable-aggregator.variables SUCCEEDED, outputs={} -- triage cause (d)
question-classifier.classes two blank categories + fabricated handle ids
question-classifier.query_variable_selector classifies `query or ""`
knowledge-retrieval.dataset_ids retrieves from no dataset, succeeds empty
The whole `if-else` entry goes too, though nothing is gained or lost by it: its
one case has an empty `conditions` list that graphon combines with `all([])`, so
it always matches -- but `iter_cases()` synthesizes that identical case when
`cases` is absent, so dropping the entry does not cure it. It only stops us
writing a fabricated always-true condition that makes the node look configured.
An empty value that fails LOUDLY is not in that category: there is no silence to
protect against, and dropping it would pay the full batch-refusal cost for
nothing. Verified loud, so these stay -- `llm.model` (never launches, "model not
configured"), `llm.prompt_template` (empty messages are filtered out, leaving
zero, so graphon raises `NoPromptFoundError`), `code.code` (the sandbox runner
calls `main(**inputs_obj)` with no `main` defined), `code.outputs` (graphon
raises `CodeNodeError("Not all output parameters are validated.")` for any key
the code returns), and `http-request.url` (`Executor._init_url` raises
`InvalidURLError("url is required")` before any request goes out).
`template-transform.variables` stays too -- it is structural, and it is the
exact field the live F4 failure was missing.
The placeholder agent genuinely means "no dataset, no output", so it now says
so itself instead of borrowing a default that would say it for every caller.
The engine sweep is re-pinned in both directions: the fields the engine reports
missing from a node built from a default ALONE must be exactly the purpose
fields the registry refuses to fabricate -- no more (a structural regression)
and no fewer (a silent-success regression).
Also: name the `filter_applicable` test after what it actually asserts, since
that function does not return its working graph; and reword the comment in
`graph_normalizers` that quoted a bare `type` marker, one reflow away from
being read as a type comment and silently killing the type check again.
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>
Final-review minors:
- is_user_supplied_secret: a verbatim match needs a remainder of at least
8 characters, so "Bearer key" no longer counts as supplied because the goal
says "key".
- is_credential_key: Ocp-Apim-Subscription-Key ("subscription" key prefix),
and cookie / credential / credentials / bearer as last segments. No
"header" segment -- accept_header stays non-credential.
- A bare Bearer/Basic/Token word is kept as an auth scheme only on an
Authorization / Proxy-Authorization line; under any other credential key
(X-Auth-Token: token, api_key: token) it is replaced whole.
- endpoint_variable_names also collects a start template inside the parsed
host (https://{{#s.host#}}/v1), via the shared core/dify_builder/urls.py;
path/query templates stay mockable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
is_user_supplied_url exempted ANY url containing a Dify template, so
https://api.pptrender.io/v1/render?topic={{#node1.topic#}} (a host the user
never typed) was left in place and every test run hit a host that does not
exist. Only a template in the HOST position (the url starts with one, or its
host contains one) exempts a url now; a literal host is checked as usual.
Grounding such a url replaces only scheme://host[:port] with the start
variable (labelled "<title> base URL") and keeps the path, query and the data
templates in them verbatim; a url whose host is already a template is never
re-grounded, so a second pass is a no-op. The host-position rule lives in the
new stdlib-only core/dify_builder/urls.py (urlsplit reads a template's '#' as
a fragment), shared with endpoint_variable_names' side.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test form and mock data skipped every start variable an http node
read, so a search query or a language header was never mocked. Only a
host-position URL template, credential-keyed header/param lines and an
api-key auth now count, with one shared is_credential_key in core. A
bare "key" param, an empty credential and a bare "Bearer" are grounded
too, and an existing Dify template is never re-grounded.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- bind_resources fixtures use the production model label, the full
provider/model id (langgenius/tokener/tokener/deepseek-v4-flash), and the
real Code Interpreter id (code/simple_code); the docstring's ESQ1-302 quote
now shows that shape too.
- The run_test prefill uses without_endpoint_values instead of re-evaluating
endpoint_variable_names(graph) for every key.
- _ground_placeholder_endpoints: two placeholder http-request nodes get two
start variables, and a start variable that already exists is not duplicated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Build, Edit and Fix mock call sites passed generate_mock_inputs through
unfiltered, so an endpoint variable got a mocked URL. The engine accepts any
URL at launch; the http-request node then fails on connection, which
is_input_failure reads as a config failure, so the flow fed the config-repair
loop (the ESQ1-302 thrash). without_endpoint_values drops the endpoint keys at
every mock call site: the missing required key now fails the launch as
"<var> is required in input form", which routes back to the test-data gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A placeholder endpoint is re-pointed to a required start variable, but
the test-data form pre-filled every text field with a mocked value, so
one click ran against another invented URL. Start variables an
http-request node reads its URL from now stay empty, like upload fields.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Build's and Edit's re_fix branches (handle_review's continue_adjusting,
handle_reverted's retry_after_revert) reset repair_attempts and
last_repair_error but not unknown_outcome_count. After the cap (count=2)
-> keep_draft -> review -> re_fix, the new cycle's FIRST unknown outcome
re-capped with "couldn't be determined twice in a row". Reset it at all
four sites, as Fix's re_fix already does.
Also pins that Edit's unknown-outcome cap drops a stale diagnosis and
staged repair (seeded, so the assertion is not vacuous).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
apply_repair's preflight raises PreflightError when the graph it is about
to write would fail at Graph.init. Every handler caught it as a plain
ValueError, so the five apply sites could not tell it apart from a
graph_ops rejection:
- plan approval (Build, Edit) called ANY ValueError "The workflow can't
start", including "node not found: x", sending the user hunting for a
broken node that does not exist;
- the repair-apply gates (Build/Edit await_repair, Fix apply) called a
preflight rejection "The proposed fix no longer applies to the current
draft" -- the fix applied fine; it would leave an unstartable draft.
Add core DraftWouldNotStartError(ValueError) and make PreflightError
subclass it (services imports core; core still imports no services).
Each site now catches it before the ValueError branch, with the same
control flow and the true reason: plan approval keeps "The workflow
can't start" for it and says "Couldn't apply the workflow" otherwise;
the repair gates say "The workflow can't start" for it and keep
"Couldn't apply the fix" otherwise. The shared card/turn construction is
factored (_graph_not_applied, _change_not_applied, and
drop_unapplied_repair in handlers_fix) and the new literals are
catalogued in strings.py.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Build resets the breaker counter when the user re-enters the repair
loop from review or after a revert; Edit's equivalent branches did not,
so a count left over from an earlier cycle could trip the breaker one
round early.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Edit's affected-path test diagnosed and staged a repair for the same
engine failure with no counter at all, so the loop 6877532f8e closed for
Build stayed open for Edit. The breaker (engine-keyed signature,
consecutive-repeat counter, MAX_REPEATED_REPAIRS) moves to handlers_fix
under public names, Build keeps its underscore names as aliases, and
Edit stops at its gate on the third identical failure. A new edit goal
resets the counter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fix's re_fix action resets the rest of the repair loop's state but kept
unknown_outcome_count, so after the cap tripped the first unknown
outcome of the new repair cycle re-capped immediately instead of being
re-runnable once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The unknown-outcome branch (a stream that ended with neither a terminal
nor an error frame) returned the user to the re-run state with no
limit in all three flows. With error frames now reported as failures
that branch is rare, but a stream that keeps ending early could still be
re-run forever. DifyBuilderContext.unknown_outcome_count counts
consecutive unknown outcomes (reset by any determinate one); the second
in a row stops at the gate with a card telling the user to look at the
run itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first cut flagged any succeeded run where a branch node ran and no
End did, which also caught a legitimate side-effect-only arm that ran
to completion, and treated any fail-branch node as a router. The rule
is now the ESQ1-303 mechanism itself: a branch node ran and none of its
edge targets ran (the engine skipped every arm), in a graph that has an
End that was never reached. The card points at that branch node.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ESQ1-303: both edges of a Builder-made if-else used handles the node
does not declare, so the engine skipped both arms, reached no End node,
and reported "succeeded" with outputs={}. Every verify handler took any
"succeeded" as "All checks passed".
A succeeded run that executed a branch node (if-else, question-
classifier, human-input, or a fail-branch node) and no End node is now
"Finished without output": an error-toned test card plus a card pointing
at the branch node, waiting at the gate with no staged repair -- the
engine reported no error, so there is nothing to diagnose. The rule is
deliberately that narrow: requiring an End node for every succeeded run
would false-fail side-effect-only branches, which the structural
validator does not forbid.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
apply_repair streams a canvas marker per applied intent before its
preflight runs, so a rejected build left the client with markers for
mutations that were never written. The plan-approval catch now emits
revert_checkpoint: nothing was written and the draft is still the
checkpoint taken at approval.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
apply_repair now rejects a graph Graph.init would refuse (PreflightError).
Build's and Edit's plan approval called apply_repair with no guard, so
the rejection would have escaped the handler and left the operation
interrupted mid-step. Both now catch it: the step is marked failed, an
error card names the node and the reason, nothing has been written, and
the plan stays at its gate -- re-approving regenerates the graph.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
run_draft populates Run.error, and all three consumers threw it away:
Build and Edit assigned run_error = "" and Fix's Run(...) literal had no
error= at all. A launch failure therefore persisted with an empty error
(the long-open "TRUNCATED_STREAM_ERROR not persisted, err_len=0" item),
diagnose saw nothing, and the repair breaker -- which keys a launch
failure on run.error -- could never fire on one.
Fix's handle_verify also gains the try/except Build and Edit already
have, so a run_draft that raises degrades to a failed Run carrying the
launch error instead of killing the advance.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The streaming pipeline reports a run that threw as one ``event: error``
frame with top-level message/code and no data (ESQ1-302: Graph.init
rejected a node before workflow_started, so there is no run id either).
run_mapping had no representation for it: _TERMINAL_STREAM_EVENTS lists
only workflow_finished/paused, so the frame was neither terminal nor a
node event and the run came back "running" with the truncated-stream
notice.
error_from_stream_chunk reads such a frame; map_error_frame_run turns it
into a FAILED Run carrying the message (whitespace collapsed, so it is a
stable repair-breaker signature) and the code. map_run_result now keeps
the engine's run-level error text for a failed run instead of dropping
it -- a launch failure has no per-node rows, so that text is the only
evidence diagnose and the breaker have.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Run workflow streaming inside the Builder worker to avoid queue deadlocks. Forward native events to shared editor callbacks and wait for graph refresh before retesting repairs.
Keep command cleanup stable, preserve batched canvas events, and allow failed or unfinished historical runs to refresh again.
Starting a NEW Build session on an app whose draft already holds a previous
build's graph silently changed nothing and still reported "graph built".
_already_present matches a create_node by node id ALONE, and the generator
always emits node1, node2, ... So a new plan whose node2 is a
parameter-extractor was filtered out against an existing node2 that is an
llm: to_apply became empty, apply_repair([]) no-oped, change_set fell back
to "graph built", and the session advanced to build.execution over an
untouched canvas. Observed live -- the snapshot taken before apply was
byte-identical to the draft after it, and the subsequent test run "passed"
because it ran the OLD graph.
The id filter itself is deliberate and stays: it is what makes a loop-back
re-approve (continue_adjusting / revert) idempotent instead of raising on a
colliding id. The guard therefore keys on built_node_ids, which names what
THIS session built -- empty only on a session's first build, which is
exactly the broken case. A loop-back still no-ops silently and advances.
Does NOT clear the canvas on the user's behalf: that would destroy an
existing workflow. It reports honestly and stays in plan_approval.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_repair_is_repeating could never fire in production. _note_repair_error
counted repeats by exact string equality on diagnosis.root_cause, which is
LLM prose regenerated on every diagnosis call. Observed live across four
turns of one session: same failure, reworded every time, one turn switching
to Chinese. Equality therefore never held, repair_attempts reset forever,
and the guard was unreachable -- that session burned 9 failed verify runs
and 8 repair approvals with the guard in place.
The call site's own comment reads "ESQ1-285 burned nine approvals this way,
ESQ1-290 six". The reproduction burned nine again.
Key on the ENGINE's output instead: _failure_signature(run) returns the
first failed node's id plus its error text (whitespace-collapsed), falling
back to the launch error, and "" when the run identifies nothing -- so two
unknown failures never compare equal.
Why the existing tests passed while production was broken: every one of them
fed _note_repair_error the SAME literal string twice, and StubAgent.diagnose
always returns one fixed root_cause. They encoded the premise that is false
in production. The new handler-level test drives three turns with an agent
that rewords its root_cause each call while the engine fails identically,
and fails against the old implementation with repair_attempts == 0.
last_repair_error now holds that signature rather than prose. It was never
displayed -- its only use was this comparison.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four findings from the whole-branch review, all in the repair loop:
I1 -- repair_attempts/last_repair_error were added to the context and to
serde but not to recovery._reset_working_fields (which clears eleven sibling
fields) nor to the re_fix/restart re-plan branches, so a fresh build could
inherit repair_attempts >= the ceiling and decline a repair on its FIRST
failure. They are cleared with their siblings now. The guard also ANDed a
monotonic counter with a consecutive-error test, so A -> B -> B tripped it
although B had survived one repair. _note_repair_error now makes the counter
mean what the docstring says: consecutive repeats of ONE error, reset when
the error changes.
I2 -- the repeated-failure return dropped run/run_id_sink, so repo.save_run
never ran while fc.verify_run_id already pointed at that run: a dangling
pointer and the third failing run lost from the debug export.
I3 -- ESQ1-291's empty-staged-repair guard existed in Build only. Edit's
model-config branch GUARANTEES an empty staged repair and routes to a state
whose approve applied it blindly; handlers_fix.handle_apply had the same
shape. Both are guarded now, reusing Build's notice.
I5 -- apply_repair re-validates intents against the draft as it is NOW, so a
dotted path that was applicable at propose time can raise ValueError at
approve time. Uncaught at all three approve sites, it reached the runner and
drove the session to FAILED -- the exact ESQ1-271 outcome this branch set out
to remove. Each site now degrades to a "Couldn't apply the fix" error card
naming the reason and clears the staged repair (engaging the I3 guard).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
handle_await_repair offered "Apply the fix" unconditionally, so an empty
staged_repair produced approve -> apply nothing -> retest -> same failure
-> offer again, with no LLM in the loop. ESQ1-291's trace shows five such
approvals, every change set {count: 0, changes: []}.
Adds repair_attempts / last_repair_error to the context (both defaulted,
so persisted contexts deserialize unchanged) and _repair_is_repeating for
the follow-on gate. An empty staged repair is now declined with a decision
the user can act on instead of a run that cannot help.
_repair_is_repeating is also wired in at the point a repair is OFFERED
(the config-failure path in handle_test_and_repair, right after
diagnose()): a third identical failure clears staged_repair and surfaces
a "Repeated failure" card instead of proposing a fourth repair
(ESQ1-285/290 burned nine and six approvals this way).
Also catalogs the two new user-facing literals in core/dify_builder/
strings.py so test_catalog_covers_handler_literals keeps passing.
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>
Mocking the inputs removed the work of inventing test data, but it also
removed the user's sight of what was actually tested. A green check
produced by {"topic": "test"} is weak evidence about a workflow, and an
invisible mock is a quieter version of the same false confidence a
passing-but-unverified test gives.
So keep the gate and pre-fill it: the form arrives with the mocked values
already in it, unfrozen, so the common case is one click rather than N
fields to type, and anyone who cares can edit before running. The cost
item 5 removed was PRODUCING the data, not confirming it.
Upload fields stay empty and still ask, because nothing can mock a file --
and the mock generator falls back to a plain string for types it does not
know, so it would otherwise pre-fill a file field with "test". New
``upload_variable_names`` and ``without_upload_values`` strip those values;
``needs_upload_inputs`` is re-expressed on top of the first rather than
duplicating the type check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The run id already reaches a client over SSE, which is enough to open the
run on the canvas while the page is up. It is not enough afterwards: SSE
frames do not survive a reload, and the persisted card's ``run_ids`` holds
BUILDER run ids -- uuid4s the engine mints, which resolve to nothing
outside it. So on reopening a session the card was the only record of the
result, and the canvas view was unreachable.
Put ``dify_run_id`` on TestResultCard, beside ``run_ids``, wired at all
nine construction sites across the Build, Edit and Fix flows. Now the
canvas is reachable from the durable conversation too, which is what makes
the card's ``output`` genuinely the client's choice to render or skip
rather than the only surviving copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Streaming the test run gives the user a live progress bar but no results:
NodeEvent carries node_id/title/status/error and nothing else, so a client
watching the stream learns which node is running, never what it produced.
The results already exist -- the run is a normal InvokeFrom.DEBUGGER draft
run, its node executions are in the database, and the editor's canvas
already knows how to render exactly that view. It was simply never told
which run to open.
So put the Dify run id where a client can reach it:
- NodeEvent gains run_id, filled from the streaming frame. Every non-ping
frame is stamped with workflow_run_id, so this arrives with the FIRST
node event -- while the run is still going, which is the point.
- The node SSE frame publishes it alongside the existing fields.
- mark_test_success / mark_test_error carry dify_run_id in both the Build
and Edit flows, so the terminal state names its run too.
This is what makes per-node results land on the graph, where a node's
inputs and outputs already have a home, instead of only as a JSON blob on
a conversation card.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three minor whole-branch-review fixups:
- ResourceOption.readiness ("ready"/"missing_config") was computed but never
shown to the model: discover_resources's recommender prompt listed ids and
labels only, so it could recommend an unauthorized tool; _GAP_SYSTEM showed
readiness in the listing (assess_capability_gap) but never defined what
missing_config means. Both prompts now show readiness and state that
missing_config is installed-but-not-authorized, i.e. not usable -- pinned
with tests asserting the literal "missing_config" in the constructed
prompts.
- strings.py: dropped three orphaned catalogue entries ("Here's the initial
plan.", "Ask each time", "Prefer audited") -- the conflict-policy concept
they backed was removed end to end; nothing in the handlers emits them.
- core/workflow/generator/tool_catalogue.py: provider.need_credentials was
read OUTSIDE the per-provider try/except that already guards
get_tools()/tool parsing. This module is shared with the cmd+K /create and
/refine flows, so a provider whose property raised would break those too,
not just the Builder. Moved the read inside the try/except so a bad
provider is skipped like any other.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_terminal_output json.dumps'd a node's outputs with no length limit. Node
outputs are only truncated upstream at ~100,000 chars per string, so a
report-generating workflow -- the Builder's showcase case -- could push
hundreds of KB of JSON into a dify_builder_conversation_items row, the SSE
frame, and the localizer walk on every successful build.
Cap the rendered output at 2000 chars with a visible truncation marker
before it reaches TestResultCard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
run_draft's streaming path introduced Run.status="running" (outcome
genuinely unknown -- the progress stream ended before the run did) as a
third status alongside "succeeded"/"failed", but neither
handle_test_and_repair (Build) nor handle_verify (Fix) knew about it: both
treated anything short of "succeeded" as a failure, so a truncated run got
labeled "Test run -- Failed" and, in Build, had a graph repair diagnosed
and staged against it while the run might still be executing.
Both handlers now branch on status=="running" before the failure path: a
neutral notice explains the outcome couldn't be determined, and the flow
returns to a re-runnable waiting state (build.execution / fix.await_testdata)
without ever calling diagnose or propose_repair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add assess_capability_gap: one plain sentence naming a plan step nothing
installed can perform, or "" when the plan is covered. Grounded only in
installed resources -- there is no marketplace lookup, so it names a missing
CAPABILITY, never a product to install. A missing_config tool (installed but
unauthorized, from the prior task's real readiness) doesn't count as
covering a step either, since the model is shown each option's readiness
alongside its label and kind.
Wired through the full agent seam: the DifyBuilderAgent protocol, the real
LlmBuilderAgent (delegates to build.assess_capability_gap), and the
PlaceholderAgent test double (returns ""). handlers_build.py calls it
after discover_resources and, when non-empty, appends a NoticeItem beside
the existing empty-resources notice -- the two are independent and can
both fire.
Like its sibling cognition functions, it degrades to "" (never raises) on
no model, an LLM error, or an unparseable reply: a gap that can't be
assessed isn't a gap worth inventing. The sentence is LLM-generated prose
already in the user's language (json_language_instruction), not a
catalogue entry, so no static string was added.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A passing build test reported "All checks passed" and threw away the
run's actual node outputs. TestResultCard gains an `output: str` field
(pretty-printed JSON of the last node that produced anything, "" when
none); handle_test_and_repair's success card now populates it via a new
_terminal_output(per_node) helper.
_terminal_output is deliberately status-agnostic (no node.status filter):
per-node status spellings differ by source ("success" vs "succeeded"), and
a status filter that silently misses would defeat the point of the card.
FakeDifyPort (tests/unit_tests/core/dify_builder/fakes.py) gains a
settable run_outputs dict, seeded onto the success-path NodeOutput's
outputs, so tests can assert on what the card shows without hand-rolling
a Run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
handle_execution's run_test branch used to stop and ask a human for test
data any time none was prepared yet, even when every declared start
variable was something the agent could invent. Add
needs_upload_inputs(schema) -> bool (true only for a declared file /
file-list variable) and gate BUILD_AWAIT_TESTDATA on it: everything else
is now mocked inline via env.agent.generate_mock_inputs and the flow goes
straight to build.test_and_repair. A schema mixing text and file
variables still stops at the gate, since the file can't be invented.
Updates three full-build-flow tests whose generated graph has no file
start variable, so run_test now lands directly at build.review instead of
pausing at the testdata gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code review finding: handle_review's re_fix branch and handle_reverted both
emitted PlanCard("Build plan", "v1") before falling through to
build.initial_plan's unconditional resource discovery, which falls through
again to confirm_resources -- which emits PlanCard("Build plan", "v1") a
SECOND time, now with resources bound. Before this task those two cards were
tagged v1 then v2, so they were visibly different plans; now both say v1,
so the loop-back shows the same plan card twice in one pass. Task 2 already
removed this exact duplicate from the straight-through path
(handle_goal_analysis); it should not survive on the loop-back and not on
the straight-through path.
Fix: handle_review's re_fix branch and handle_reverted still draft
fc.plan_items/fc.plan_version_tag (needed by discovery and the eventual
confirm_resources card) but no longer emit a PlanCard or claim cards=["plan"]
on their AssistantTurnItem (now cards=[], matching the existing
handlers_fix.py precedent for a turn with no card). There is now exactly ONE
plan card per pass, tagged v1, on every path: straight-through,
continue_adjusting, and retry_after_revert.
Updated the two direct-handler unit tests that asserted the now-removed
card, and extended test_review_continue_adjusting_then_reapprove_is_
idempotent (an end-to-end Runner-driven test) to assert re_fix adds zero new
plan cards and the subsequent confirm_resources adds exactly one, tagged v1.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code review finding: handle_goal_analysis built its own ProgressReporter
(build-review-requirements/build-draft-plan) and finish()ed it, then
tail-called _discover_and_offer_resources, which built a SECOND reporter
under the SAME env.operation_id. ProgressReporter's revision counter starts
at 0 per instance, so the second reporter's activate/finish calls restarted
revision at 1 -- two progress events with revision 1 (and 2, and 3) under
one operation_id. contract.py's ProgressEventData docstring states revision
is monotonic within an operation_id; every other handler across the three
handler files uses exactly one reporter per step -- this was the sole
exception, on the most-travelled build path.
Fix: _discover_and_offer_resources now takes an optional `progress`
parameter. handle_goal_analysis passes its live (still-open) reporter
through instead of finishing it early; the helper adds the discovery steps
onto that SAME reporter via add_steps() and is the one that finish()es it.
handle_initial_plan (the continue_adjusting/retry_after_revert loop-back,
which has no reporter of its own -- a separate CAS commit/operation) still
gets progress=None and the helper builds a fresh one, unchanged.
Added test_submitting_requirements_progress_is_one_operation_with_monotonic_
revisions: drives the real Runner with submit_requirements, captures every
emitted ProgressEventData, and asserts a single operation_id with strictly
increasing, non-repeating revisions across all four progress steps.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code review finding: the plan card's version_tag was renumbered v2 -> v1
in an earlier commit, but handle_resource_recommendation's own reply_text
still read "Plan v2 ready for approval." -- a card that says v1 next to
prose that says v2 is worse than leaving both at v2, since it's exactly
the user-visible inconsistency the renumber exists to eliminate.
Updated the reply_text to "Plan v1 ready for approval." and its matching
entry in the strings.py literal catalog, so test_catalog_covers_handler_
literals keeps passing for the right reason (a registered v1 string, not
a stale v2 one). Grepped the whole of core/dify_builder/ for any other
quoted v2/v2.1 string this renumber might have missed -- this was the
only one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
