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>
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>
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>
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>
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 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 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>
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>
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>
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>
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>
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>
discover_resources initialised its selection to the entire tenant inventory and
only replaced it when the model returned a non-empty pick, so three paths -- no
model, a model that picked nothing, a failed call -- recommended everything. On
a workspace with 57 resources that is not merely noisy: the client pre-checks
every recommendation and submits them all by default, and bind_resources then
appends all 57 labels to the last plan item, which is the text fed to the graph
generator.
Recommending nothing is the honest answer when there is no signal. The card is
still emitted so the confirm gate keeps its active interaction and a later pass
cannot inherit a stale one, and a notice says why rather than leaving a blank
card. The conflict-policy control no longer renders over an empty list.
The test that should have caught this used a one-resource catalog, where "all"
and "none" are indistinguishable; it now uses 57.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Spec N4 has the build-complete card headline read "<App name> is ready", with
the annotation 名字只在回复句里念一次；记录卡上不出现 -- said once, in that
sentence, and never repeated on the completion receipt. We said it nowhere: the
card was titled "Build plan" and the engine had no app name to say.
DifyBuilderContext now carries app_name, seeded at session creation through a
get_app_name_fn seam alongside the existing revision/authorize ones. Env.rename_app
returns the name the database actually took rather than None, so the refine
records what landed instead of what it proposed -- the service normalizes after
the engine hands off, and the two can differ.
The headline is interpolated, so it gets a strings.py template or it would
survive localization untranslated. That pattern matches almost anything ending
in "is ready", so it sits last and every more specific template gets first
refusal.
The receipt already omitted the name; there is now a test pinning that.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A verify/affected-path run that fails because the workflow references a model
that isn't configured/available (ModelNotExistError, "Model X does not exist",
provider/credentials errors) was routed into the config-repair path -> diagnose
+ propose_repair kept proposing graph mutations (if-else / check nodes) that
can't fix a missing model, thrashing across rounds.
Add model_config_error_text() (classifier parallel to is_input_failure; pairs an
existence/credential signal with a model/provider/credential token so unrelated
"<node> does not exist" faults aren't swallowed). In both build and edit failure
branches, when it matches, surface a clear "model not configured" diagnosis with
NO staged repair and route to the await-repair gate: the user configures the
model (global update_model) or changes it, then re-runs. Catalog the two new
user-facing strings so the reply-language layer localizes them.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to the reply-language feature. The update_model action commits its
"Model changed to <name>" notice directly via compare_and_advance, bypassing
Runner._commit and thus the Env.localize_items hook, so that one notice stayed
English. Localize it in the service (when the session has a detected
reply_language) with a "Model changed to {name}" catalog template that
translates the frame and re-inserts the model name verbatim.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Final-review fix wave for the "reply in the user's input language" feature:
- serde.py: context_from_dict omitted reply_language from the explicit field
allowlist, so it round-tripped to "" on every read despite context_to_dict
(asdict) serializing it -- the localization feature silently no-op'd after
the first persisted write. Added it to the constructor call; TDD'd with a
failing-then-passing round-trip test in test_serde.py.
- localize.py: Localizer._cache was per-instance, but a fresh Localizer is
built per advance-task invocation, so the static string catalog was
re-translated every turn instead of once per language. Moved the cache to a
module-level _TRANSLATION_CACHE dict (safe to share: keys are
tenant/session-independent catalog-string+language pairs); added an autouse
fixture in test_localize.py to clear it between tests for isolation.
- localize.py: detect_language and _fill_cache called self._model_provider()
outside their try/except, violating the Env callback contract that these
must never raise. Moved both calls inside their try/except so a raising
provider degrades to the existing safe fallback instead of propagating into
advance/_commit.
- strings.py: documented that Template.translate_fields is advisory only --
the Localizer re-inserts all captured groups verbatim and does not consult
it. No behavior change.
uv run --project api pytest tests/unit_tests/core/dify_builder tests/unit_tests/services/dify_builder -q
-> 722 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
