fix(uipath-maestro-flow): wire per-outcome handles, drop the forced Decision node after HITL - #3180
Conversation
|
Claude finished @dushyant-uipath's task in 2m 39s —— View job PR Review: fix(uipath-maestro-flow): wire per-outcome handles, drop the forced Decision node after HITL
SummaryCorrects two confirmed bugs in HITL authoring guidance: (1) docs only documented one output port ( Change-by-Change Review1.
|
| Area | Status | Notes |
|---|---|---|
| Frontmatter | OK | No frontmatter changes in this PR |
| E2E Tests | OK | Two new negative tests + updated existing checkers and smoke YAML |
| Skill Body | OK | N/A — no SKILL.md changes |
| References & Assets | OK | Both impl.md and planning.md updated consistently; all links verified |
| Repo Hygiene | OK | No secrets, no cross-skill structural deps, changes scoped to one skill |
Issues for Manual Review
- Domain accuracy: The claim that
outcome-completedis exclusively the zero-outcome placeholder (disappearing when any outcome is defined) comes from flow-workbench source verification per the PR description. Reviewers familiar with the flow runtime should confirm this matches current behavior, especially for the edge case where only the defaultSubmitoutcome (idsubmit) is present. - Coder-eval run: The PR checklist shows the full coder-eval run against affected HITL tasks is still pending. This should complete before merge to confirm agents follow the updated guidance correctly.
Conclusion
Clean, well-scoped fix. The doc changes are internally consistent and the test updates match the new behavior. One stale print message in check_devcon_expense_approval.py:149 is the only concrete issue — it's cosmetic (Low severity). Approve once the pending coder-eval run completes.
|
Claude finished @dushyant-uipath's task in 2m 48s —— View job Coder-eval task lint (advisory)1 task YAML changed; verdicts: 0 Critical, 0 High, 0 Medium, 0 Low, 1 OK. Rubric: Evidence of passing run❌ High — PR body has an explicit unchecked box: Per-task lint
|
… ordering
CI caught two real bugs in the outcome-<id> checker changes:
1. check_simulated_hitl.py and check_devcon_expense_approval.py required an
explicit outcome.id, but test_batch2_alignment.py's pre-existing fixture
uses outcomes with only a name ({"name": "Approve"}), which is a valid
shape per hitl-node-quickform.md's own convention of defaulting id to the
lowercased name. Both checkers now fall back to name when id is absent.
Also updated that fixture's edges from the legacy bare "completed" port to
outcome-approve/outcome-reject, since it wires a 2-outcome node and bare
"completed" is the same stale assumption this PR removes elsewhere.
2. The "wired outcome-completed alongside real outcomes" check ran after the
"missing ports" check, so it could never actually fire: if outcome-completed
is wired instead of the real ports, the real ports are by definition also
missing, and that check always won the race. Reordered so the more
specific, more useful message fires first. Updated
test_rejects_outcome_completed_with_real_outcomes's expected message to
match.
Confirmed separately that PR #3180's "Run skill smoke tests" failure is
unrelated: skill-flow-hitl-smoke-completed-port itself passed
(status=SUCCESS, score=1.00); the suite dropped below the 95% pass-rate gate
because of 3 failing skill-flow-datafabric-smoke-* tasks, pre-existing and
outside this change's scope.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ecision node after HITL
Two confirmed bugs in the HITL authoring guidance, verified against
flow-workbench source with the Flow skill team:
1. impl.md and planning.md only ever documented one output port,
outcome-completed, regardless of how many outcomes a QuickForm schema
declares. outcome-completed is actually the zero-outcome placeholder only
— it disappears the instant the schema has any real outcome (even the
default Submit gets outcome-submit). Agents following the old docs left
every real outcome's handle dangling: invisible in canvas, but the runtime
still executed the flow as if it were connected.
2. Both docs canonicalized inserting a Decision node after every HITL node to
re-branch on $vars.{nodeId}.status. That's redundant: each outcome's own
handle has showButton: true specifically so it can branch directly.
Rewrote the wiring rules, port tables, and example flows in both docs to use
outcome-<outcome.id> per outcome and branch directly off those handles.
Also fixed three uipath-maestro-flow test files that graded the old (wrong)
behavior as correct — they would have started failing agents that follow the
corrected skill, not because of a regression but because the grading itself
still expected the generic completed port:
- smoke_02_completed_port_wired.yaml asserted the bare substring "completed"
against a 2-outcome (Approve/Reject) schema, which the corrected skill
never emits. Now asserts outcome-approve and outcome-reject are both wired
and outcome-completed is absent; updated the prompt to ask for both outcome
branches explicitly.
- check_simulated_hitl.py and check_devcon_expense_approval.py both
hard-required a completed/outcome-completed edge. Now derive the required
outcome-<id> ports from the flow's own parsed schema.outcomes[] and fail if
any is unwired or if outcome-completed appears alongside real outcomes.
Added two negative tests to test_check_devcon_expense_approval.py covering
a dangling outcome port and a wired outcome-completed placeholder.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… ordering
CI caught two real bugs in the outcome-<id> checker changes:
1. check_simulated_hitl.py and check_devcon_expense_approval.py required an
explicit outcome.id, but test_batch2_alignment.py's pre-existing fixture
uses outcomes with only a name ({"name": "Approve"}), which is a valid
shape per hitl-node-quickform.md's own convention of defaulting id to the
lowercased name. Both checkers now fall back to name when id is absent.
Also updated that fixture's edges from the legacy bare "completed" port to
outcome-approve/outcome-reject, since it wires a 2-outcome node and bare
"completed" is the same stale assumption this PR removes elsewhere.
2. The "wired outcome-completed alongside real outcomes" check ran after the
"missing ports" check, so it could never actually fire: if outcome-completed
is wired instead of the real ports, the real ports are by definition also
missing, and that check always won the race. Reordered so the more
specific, more useful message fires first. Updated
test_rejects_outcome_completed_with_real_outcomes's expected message to
match.
Confirmed separately that PR #3180's "Run skill smoke tests" failure is
unrelated: skill-flow-hitl-smoke-completed-port itself passed
(status=SUCCESS, score=1.00); the suite dropped below the 95% pass-rate gate
because of 3 failing skill-flow-datafabric-smoke-* tasks, pre-existing and
outside this change's scope.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
a9e42cd to
68d4c0e
Compare
rockymadden
left a comment
There was a problem hiding this comment.
What
Rewrites the HITL wiring guidance in uipath-maestro-flow: one outcome-<outcome.id> port per outcome instead of a single outcome-completed, and drops the canonical "insert a Decision node after HITL" pattern. Realigns 3 checkers + 1 smoke task, adds 2 negative tests. Also flips uipath-human-in-the-loop to stable.
Both core claims hold. I verified them against flow-workbench@develop rather than taking the description at face value:
packages/canvas/src/components/properties-panel/hitl/build-handle-customization.tsbuilds one handle per outcome, idoutcome-<outcome.id>;outcome-completedonly whenoutcomesis empty.inputDefaultsships[{id:"submit"}], so even a fresh node isoutcome-submit. ✅packages/services/src/serialization/hitl-serialization.ts→collectOutcomeConditionsstamps=js:vars.<node>.output.Action === "<name>"onto eachoutcome-*edge. The handle is the conditional branch, so the Decision node is genuinely redundant. ✅
The diagnosis is right. The execution is half-done, and the checker rewrite encodes a rule the product does not follow.
Overall findings
🔴 Critical — the fix stops at 2 of 8 files in its own skill
uipath-maestro-flow still teaches completed in six places, all untouched:
| File:line | Text |
|---|---|
references/author/editing-operations.md:31 |
"Includes HITL QuickForm; wire completed after adding." |
references/author/planning-arch.md:227 |
| uipath.human-in-the-loop.quick-form | input | completed | |
references/author/brownfield.md:49 |
"wire its completed port" |
references/shared/cli-commands.md:216 |
"After adding, wire the completed port" |
references/diagnose/failure-modes.md:14,132,140,144,343 |
whole section "HITL completed port unwired" |
references/diagnose/CAPABILITY.md:49 |
links to that section |
impl.md:11 — the line this PR edits — points at editing-operations.md for wiring in the same sentence that now says outcome-<id>. An agent following the link gets the old rule back.
Fix: update all six in this PR, including the failure-modes.md anchor (#hitl-completed-port-unwired) and its two referrers.
🔴 Critical — the sibling skill this one delegates node writing to still teaches completed, and this PR marks it stable
impl.md:9 sends schema design, node writing and JSON examples to uipath-human-in-the-loop. That skill says:
SKILL.md:36, Critical Rule 2: "Always wire thecompletedhandle. … Onlycompletedis available as an output handle — notoutput,success, or any other name."references/hitl-node-quickform.md:246,249,251— the exact sectionimpl.md:9links to — wires"sourcePort": "completed"and repeats "Always wirecompleted."
This PR proves that Critical Rule is wrong for every quick-form node with outcomes — and in the same commit flips the skill to stable. Either fix the sibling skill here, or drop the status change.
🟠 Major — sibling tasks still instruct and grade the old shape
smoke_02 is not the only task pinned to completed. Untouched, in this skill's own suite:
hitl/quality_04_brownfield_insert.yaml:74—flow_contains.py --flow-name ContractReview 'completed'. The corrected skill can never emit that substring, so this criterion now scores 0. Integration tier, so the smoke gate will not catch it.hitl/quality_01_schema_design.yaml:29— "Wire completed → decision node." Both halves now wrong.hitl/quality_02_result_downstream.yaml:29,hitl/quality_03_boolean_decision.yaml:28,hitl/smoke_03_multi_outcome_routing.yaml:29— prompts still say→|completed|.tests/tasks/uipath-human-in-the-loop/— ~14 tasks plusTEST_PLAN.md:39("Available handles:completedonly").
🟠 Major — impl.md Option 2's example now produces the wrong port
Pre-existing, but this PR is what makes port naming schema-derived, so it lands here. impl.md:129-132 gives the coded-action-app example an inputs.schema.outcomes: [{id:"submit"}] block. The product's manifest ships no schema for that node (ootb-nodes/uipath-human-in-the-loop-coded-action-app/v1.0-v1.4.ts, inputDefaults is recipient only), and isHitlNodeType includes coded-action-app, so getEffectiveHitlHandleCustomization reads that stray inputs.schema and flips the handle from completed to outcome-submit.
Fix: drop inputs.schema from the Option 2 example, and state that Option 2 keeps the static completed handle because its outcomes come from inputs.app.
🟡 Minor — the description overstates the failure mode
"the runtime still executed the flow as if it were connected to the next node" is the wrong half. packages/canvas/src/utils/hitl-edge-reconciliation.ts remaps a lone placeholder edge onto the primary outcome on load — its own comment calls that "the purely-cosmetic case where the connector was invisible but the workflow still ran correctly." The genuinely broken shapes are (a) multi-outcome, where only the primary gets remapped and the rest dangle, and (b) a placeholder edge duplicating a correct outcome-<id> edge, which the BPMN engine takes unconditionally alongside the conditioned one and double-executes the branch. Worth stating accurately — it is the stronger argument.
🟡 Minor — test plan claim is short of the repo bar
.claude/rules/test-writing.md § Workflow step 5 wants a passing-run claim for affected tasks. The last box is unchecked and the affected integration/e2e tasks (quality_01-04, expense_approval_simulated, devcon_expense_approval) were not run. The failing smoke gate on this PR is 4 Data Fabric tasks (create-all-types, error, query, update, 84.6%) — unrelated, confirmed from run 34487479174.
🟡 Minor — one logical change per PR
The uipath-human-in-the-loop → stable flip is unrelated to the wiring fix and rests on nightly scores that are not in the diff. .claude/rules/pr-review.md § Commit and Branch Conventions. Split it.
What I verified vs. reasoned
- Verified from source: handle id format, the placeholder's zero-outcome condition, id used verbatim (no case folding), outcomes without an
idgetting no handle at all, per-outcome conditions in BPMN serialization, coded-action-appinputDefaults, legacy edge reconciliation. - Could not verify: the "96-100% across the last several claude-sonnet-5 runs" nightly claim behind the
stableflip. Not in the diff, no linked run. Reported, not confirmed. - Did not run: the checker unit tests (read only, per repo convention).
maestro-flow checker unit testspasses in CI, which is consistent with the findings below — the fixtures never exercise the broken paths.
Recommendation
Request changes. The diagnosis is correct and well-sourced, but the skill now contradicts itself in six files and contradicts the sibling skill it delegates to, so an agent lands back on the old rule either way. Separately, both checkers lower-case the outcome id and fall back to name, neither of which the product does — they will fail correct work and pass invalid work. See inline.
tl;dr — Right bug, right root cause, verified against flow-workbench. But the fix touches 2 of 8 files that teach completed, leaves the sibling HITL skill teaching the opposite as a Critical Rule while marking it stable, and the new checkers lower-case the outcome id and invent a name fallback the product does not have. Request changes.
…e-port fix Rocky's review found incomplete
Rocky Madden reviewed the original fix (request changes, verified against
flow-workbench@develop source directly) and found the fix touched 2 of 8
uipath-maestro-flow files that teach the completed port, left the sibling
uipath-human-in-the-loop skill teaching the opposite as a Critical Rule while
marking it stable in the same PR, missed ~14 affected task YAMLs in both
skills' own test suites, and got the outcome-id derivation wrong in the
checkers (falling back to a lowercased name, which the product does not do).
Verified the id-derivation question with the Flow session against source
(build-handle-customization.ts filters outcomes by explicit id, no name
fallback; the editor always assigns one at creation) before touching the
checkers again.
Docs, uipath-maestro-flow (6 more files):
- editing-operations.md, planning-arch.md, brownfield.md, cli-commands.md:
wiring instructions now say outcome-<outcome.id> per outcome.
- diagnose/failure-modes.md: renamed the "HITL completed port unwired"
section to "HITL outcome port unwired", rewrote symptom/cause/fix, updated
the anchor and its two referrers (the table row and CAPABILITY.md).
- impl.md Option 2 (coded-action-app): removed the inputs.schema.outcomes
block from the example — Rocky found it flips the node's real static
completed port to an outcome-derived one, which the product does not do
for this node type. Documented that Option 2 keeps a static completed
port regardless of the app's own outcomes.
Docs, uipath-human-in-the-loop (the sibling skill impl.md delegates node
writing to):
- SKILL.md Critical Rule 2 rewritten: QuickForm wires outcome-<outcome.id>
per outcome; App-based keeps a static completed port and must not carry an
inputs.schema block. Rule 3 and the CLI-add step no longer assert a
literal completed handle.
- hitl-node-quickform.md Edge Wiring section rewritten to the per-outcome
model; the static handleConfiguration JSON block is untouched per Rocky's
own line citations.
- hitl-node-apptask.md's two "identical to QuickForm" claims corrected —
App-based is the one that stays static now, not QuickForm.
Checkers (reverted the wrong id fallback):
- check_simulated_hitl.py, check_devcon_expense_approval.py: outcome ids
must be explicit (outcomes[].id) with no name-derived fallback, matching
the confirmed product behavior. Also confirmed check ordering already
flags a wired outcome-completed placeholder before the generic
missing-ports message.
- test_batch2_alignment.py: its fixture used name-only outcomes
({"name": "Approve"}), which the checker fallback used to paper over; gave
it explicit ids instead.
Tests (the ~14+ files Rocky flagged in both skills' suites):
- uipath-maestro-flow/hitl/quality_01..04, smoke_03: prompts and criteria
updated. quality_01 drops its Decision node (redundant once outcome ports
branch directly); quality_03, smoke_03, and human-in-the-loop's
quality_10 keep their Decision nodes since those branch on a boolean
field value, not on which outcome was clicked — a genuinely different
case the corrected planning.md calls out as still valid.
quality_04_brownfield_insert.yaml's criterion always scored 0 against the
corrected skill (it asserted the literal substring "completed" against a
2-outcome schema); replaced with a check for both real outcome ports.
- uipath-human-in-the-loop/e2e_01..06, quality_04/05/07/08/09/10/11:
prompts and criteria updated the same way. e2e_02 and e2e_04 used
file_contains (no regex support) and were converted to run_command +
flow_contains.py --regex to express "a real outcome port, not the
placeholder."
e2e_07 (App-based, skip: true) is unchanged — it already correctly wires
a static completed handle.
- TEST_PLAN.md: invariant table and per-task summaries updated to match.
- uipath-maestro-flow/interactive/expense_approval_simulated.yaml and
e2e/devcon_expense_approval.yaml: only their descriptive comments said
"completed" — the actual grading already runs through the corrected
checker scripts above, so behavior did not change, only the prose.
Not fixed here: the description of the failure mode in the original PR
overstated it ("ran the flow as if connected"). Rocky's source read
(hitl-edge-reconciliation.ts) says a lone placeholder edge remaps onto the
primary outcome and is cosmetic; the real breakage is (a) multi-outcome
schemas where only the primary gets remapped and the rest dangle, and (b) a
placeholder edge duplicating a real outcome-<id> edge, which the BPMN engine
takes unconditionally alongside the conditioned one and double-executes that
branch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
68d4c0e to
40fd0df
Compare
…2572) The criteria migration in #2534 replaced the file's final criterion block and accidentally swallowed the trailing run_limits block, so the task fell back to the experiment defaults (max_turns 200 / task_timeout 1200) instead of its 40-turn / 900s limits. Restore the exact prior block. Config-only revert to the pre-#2534 state; no criteria changed. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-09-11 run 2026-09-11_04-17-20: ERROR, "Agent turn timed out after 2400s" (iteration 1, 119 assistant turns, max_turns_exhausted False). The iteration log shows continuous real work for the full 2400s, not a loop: live SharePoint/SAP connection discovery, an IXP taxonomy lookup for invoice extraction, and registry lookups for every node type, each a real network round trip, with the agent still on setup work at the 25-minute mark. Raised both task_timeout and turn_timeout to 3600, matching the established heavy-single-turn budget already used elsewhere in the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iscovery skill-hitl-e2e-invoice-approval-greenfield kept timing out for a reason neither #3196 (1200s -> 2400s) nor the 2400s -> 3600s bump addressed at the root: the prompt required real SharePoint extraction and real SAP posting, so every run did live connector/registry/IXP-taxonomy discovery before ever touching the HITL node the test is meant to grade. 2026-09-11 run 2026-09-11_04-17-20 spent all 2400s and 119 assistant turns on exactly that discovery work, still mid-setup. Mocked both integration points as Script nodes instead of raising the timeout again. The test exists to exercise HITL schema design and outcome-port wiring, not connector discovery, and the prompt still requires the agent to recognize the write-back + approval gate signal on its own. Dropped task_timeout/turn_timeout back to 1200/1200, matching the sibling e2e brownfield tasks that already mock their upstream/downstream the same way, and confirmed locally: coder-eval 0.12.0, claude-sonnet-4-6, SUCCESS, score 1.000, 7/7 criteria, 482.7s, 34 assistant turns. Raised expected_turns (14 -> 34) and max_turns (40 -> 50) to match that real usage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… inline review comments Went back through every inline comment on the review (not just the summary) and found several the earlier round-2/round-3 fixes had not actually addressed: - impl.md:69/182 and planning.md:55 overclaimed "outcome-completed never reappears" / "is not a real outcome port". It is an ordinary string, not a reserved one: an outcome whose own id is literally "completed" produces the identical port. Reworded to the accurate claim (zero-outcome placeholder, or a real outcome id'd "completed") in both files, plus the impl.md debug-table row. - check_simulated_hitl.py's new outcome-id/wiring block made outcomes mandatory, contradicting the function's own earlier gate that explicitly accepts a boolean-decision field with no outcomes. Extracted the shared logic into assert_outcome_wiring() and gated it on outcomes being non-empty; the zero-outcome path now correctly falls back to requiring the completed placeholder, matching pre-existing behavior. - Same file (and check_devcon_expense_approval.py) had the outcome-completed check running after the missing-ports check, so it could never fire (a wired outcome-completed always looks like every real port is missing). Reordered in both. Caught by re-running the unit tests locally before pushing, not just by inspection. - Added check_outcome_wiring, a generic node-scoped check (handles 1+ HITL nodes) built on the same assert_outcome_wiring(), and pointed smoke_02 and 7 other task YAMLs at it instead of a flow_contains.py --absent-regex. That regex was file-scoped: it would false-fail the moment a flow gained an unrelated node with a legitimate completed port (an Option 2 HITL node), and it pinned outcome-approve/outcome-reject as guessed literals rather than deriving them from the schema. - Added the missing outcome-id conversion rule to hitl-node-quickform.md's table (same lowercase-name convention as field id), so those literals are now backed by a documented rule instead of one example. - Deduplicated the repeated 6-line comment block across check_simulated_hitl.py, check_devcon_expense_approval.py, and smoke_02 into one full statement in check_simulated_hitl.py plus short pointers, and dropped the "confirmed against ... 2026-09-10" narration in favor of a file a reader can open. - Added two tests to test_check_devcon_expense_approval.py: a mixed-case outcome id (the handle must use it verbatim, never lowercased) and an outcome with no id (must fail loudly, not get silently dropped). Ran the full local test suite before pushing: 19/19 in the two directly affected test files, no HITL/outcome-related failures anywhere else in uipath-maestro-flow (133 pre-existing failures elsewhere are unrelated — test_criterion_budgets.py and a Data Fabric connector suite, matching CI's already-known unrelated flakiness). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @dushyant-uipath's task in 6m 1s —— View job Coder-eval task lint (advisory)
Coder-eval task lint (advisory)22 task YAMLs changed; verdicts: 0 Critical, 0 High, 0 Medium, 20 Low, 2 OK. Rubric: Evidence of passing run
❌ High — The remaining 20 changed tasks do not have passing-run claims. The PR body explicitly acknowledges this: "Remaining ~13 affected integration/e2e tasks not run through a full coder-eval pass." Please consider editing the PR description to add passing-run claims for at least the substantively changed tasks (quality_01–04, quality_04_all_handles, smoke_03), or note that the changes are wording-only and the criteria behavior is verified through the checker unit tests. Per-task lint
|
rockymadden
left a comment
There was a problem hiding this comment.
What
Second pass. Last round I requested changes on 12 inline findings; all 12 are addressed. This is a re-review of the delta plus a re-verification of the core claim against flow-workbench@develop.
The model still holds, and I re-confirmed it from source rather than from the replies:
packages/canvas/src/hooks/useResolvedHandleNodes.ts—isHitlNodeType(nodeType) ? getEffectiveHitlHandleCustomization(node.data) : …. No version gate. Any HITL node type at anytypeVersiongets per-outcome handles onceinputs.schema.outcomeshas one valid-id entry. ✅packages/canvas/src/validation/hitl-outcome-handles-rule.ts— header: "Per-outcome handles are always enabled; this rule always fires for HITL nodes." EmitsHITL_DANGLING_EDGEfor acompleted/outcome-completededge once real outcomes exist. ✅packages/services/src/serialization/hitl-serialization.ts→collectOutcomeConditions— keys off theoutcome-prefix ando.id, no version check. ✅outcomePortsdoes not exist anywhere in flow-workbench@develop. ✅
Against the tickets: this matches UV-16499 (outcome port ids) and the HITL half of UV-16428 (skill-doc contradictions). Neither is linked from the PR body — worth adding.
Fix-by-fix verification of the 12:
| # | Claim | Verdict |
|---|---|---|
| 1 | 6 doc files | ✅ all 6 + anchor rename + both referrers |
| 2 | Sibling skill | ✅ SKILL.md:36, hitl-node-quickform.md:246-253, hitl-node-apptask.md:3,421 |
| 3,4 | outcome-completed over-claim |
impl.md:69 / planning.md:55, not in 3 other sites (inline) |
| 5,8 | Verbatim id, no name fallback |
✅ both checkers |
| 6 | has_decision-only path |
✅ assert_outcome_wiring elif keeps the old contract |
| 7 | Comment dedup | ✅ one full comment, pointers elsewhere, no dates |
| 9 | Fixture ids + negative case | ✅ + test_rejects_outcome_without_id |
| 10 | Mixed-case test | ✅ test_accepts_mixed_case_outcome_id |
| 11 | Node-scoped check | ✅ new outcome-wiring subcommand, replaces the file regex in 9 tasks |
| 12 | stable flip |
✅ reverted, split to #3224 |
Check ordering is fixed — the placeholder-misuse message now fires before the missing-ports one, and the "completed" not in outcome_ids escape correctly preserves the real-outcome-named-completed case.
Overall findings
🟠 Major — #3239 asserts the opposite and touches the same two files
The author flagged this. My read of the source, for the record:
- Every
uipath-human-in-the-loop-quick-formmanifest v1.0 → v1.6 declares one static source handle,completed. Baseuipath.human-in-the-loopv1.1/v1.2 declareoutcome-completed— a placeholder, per their own comment: "OncehandleCustomizationis populated (one handle per outcome), it overrides this group entirely." - Neither declares
outcome-{item.id}repeated over outcomes. That shape comes from the runtimehandleCustomizationoverride, not from a manifest version.
So #3239's premise — "outcomePorts: true selects the base node's 1.1/1.2 definition, whose only source handle is outcome-{item.id}" — is not what flow-workbench does. outcomePorts is a flow-builder-sdk concept (that PR cites flow-builder-sdk#718/#724), and the SDK's emit gate is a separate question from what the canvas renders and what the runtime routes on.
Could not verify from source: what uip maestro flow validate itself accepts — that validator lives in flow-builder-sdk, which I do not have. Two pieces of external evidence point the same way: smoke_02 requires outcome-approve/outcome-reject and exit 0 from validate, and reports SUCCESS 1.00; and UV-16499 states edges from completed/outcome-completed "validate and run as unconditional flows but never render" — i.e. validate accepts both shapes and rejects neither. That is consistent but not source-confirmed. Reporting it as unconfirmed.
Recommendation: merge this first, then rebase #3239's gate relaxation onto it. Accepting either completed or outcome-<id> would let a HITL_DANGLING_EDGE flow score 1.0 — the exact double-execution shape behind UV-16499.
🟡 Minor — hitl-node-coded-action-app.md:322 missed the inputs.schema warning
impl.md:155 and hitl-node-apptask.md:3 both gained "do not add an inputs.schema block to an app-based node." The third file describing the same node type did not. Line 322 reads "wire completed (only handle available in v1.0)" — correct, but it is the one place a reader lands without the trap warning. File is not in the diff, so no inline. One clause fixes it.
🟡 Minor — 20 of 22 changed tasks unrun
Per the advisory lint and the PR body. quality_01–quality_04 and smoke_03 got substantive prompt rewrites (Decision-node guidance inverted), not just wording. .claude/rules/test-writing.md § Workflow step 5 wants a passing-run claim. Not a blocker given the checker unit tests are green and smoke_02 + e2e_01 both passed, but the rewrites are more than cosmetic.
🔵 Informational — branch carries a duplicate of a commit already on main
a2f1e0f13 ("restore scaffold_multinode run_limits (#2572)") is byte-identical to 7f1773dbe on main. Merge is a no-op, but it puts an unrelated file in the PR's file list. Rebase clears it.
Recommendation
Approve. The diagnosis was right last round and the execution now matches it. All 12 findings are genuinely fixed, not papered over — the name fallback is gone, the invalid fixture has real ids, the mixed-case test fails without the fix, and the file-scoped regex is replaced by a node-scoped check across 9 tasks. The residue is doc consistency and one unrelated test rescope, none of which makes an agent do the wrong thing on the main path.
Six inline comments, all non-blocking.
tl;dr — All 12 prior findings fixed and verified, not just claimed. Re-confirmed from flow-workbench that per-outcome handles are ungated by version, which makes #3239's premise wrong; merge this first. Residue: the QuickForm definition block still declares only completed 30 lines above the rule saying never to wire it, 3 files kept the over-claim that impl.md dropped, and an unrelated e2e_01 rescope got bundled in. Approve.
|
Reconciling this against #3239, as your "needs reconciling before merge" note asks. Your direction is right and #3239 is the weaker fix — I'm closing it in favour of this. Three findings, all measured on the alpha tenant today, one of which corrects a line in your description. Your counter-measurement is correct, and here is the mechanismYou wrote that a
A literal handle is checked against the definition; anything shaped The designer source you cite backs the direction: One line in the description needs a correction
True of the canvas, not of the manifest. After Neither declares an outcome handle. The per-outcome handles come from the instance-level The consequence worth deciding before mergeYour gate, run against each authorable shape:
The first and last are improvements — the last is a real bug my version missed, since a partially-routed outcome set leaves an outcome dangling at runtime. Rows 2 and 3 are the thing to weigh. From the SDK arm, the only way to satisfy this gate is base +
So a quick-form HITL becomes ungradeable on this task. That is satisfiable overall — base + Also worth knowing for row 2's message: the product itself emits What I am doing with #3239Closing it. Nothing in it survives that this PR does not do better: the |
…e review comments on #3180 - hitl-node-quickform.md: the definition entry's static completed handle contradicted the Edge Wiring section 30 lines down with no reconciling note. Added one: the static handle mirrors the manifest, per-outcome handles come from schema at render time, never from handleConfiguration. - SKILL.md, hitl-node-quickform.md, failure-modes.md: all three stated outcome-completed can never exist once outcomes are present. Corrected to the exact phrasing already used in impl.md: the port for a zero-outcome node, or for a real outcome whose id is literally completed, never a shared exit for several outcomes. - e2e_01_invoice_approval_greenfield.yaml: mocking SharePoint/SAP to fix a timeout dropped this suite's only e2e coverage of live connector discovery. Added an explicit scope-change note to the task and a new row in TEST_PLAN.md's Coverage Gaps table tracking the gap instead of leaving it unstated. - check_devcon_expense_approval.py: assert_outcome_wiring was duplicated verbatim from check_simulated_hitl.py. Now imported, using the same dual-import fallback pattern already used for find_flow_file so the frozen/standalone checker copy still resolves it via PYTHONPATH. - check_simulated_hitl.py's outcome-completed escape branch (wiring a real outcome whose own id is "completed") had no test covering it. Added one. Local verification: python3 -m pytest on the two directly affected test files, 20/20 passing (was 19, +1 new). Full uipath-maestro-flow suite: same 133 pre-existing failures as before this change (test_criterion_budgets.py, Data Fabric connector suite), no new HITL/outcome-related failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ecision node after HITL (#3180) * fix(uipath-maestro-flow): wire per-outcome handles, drop the forced Decision node after HITL Two confirmed bugs in the HITL authoring guidance, verified against flow-workbench source with the Flow skill team: 1. impl.md and planning.md only ever documented one output port, outcome-completed, regardless of how many outcomes a QuickForm schema declares. outcome-completed is actually the zero-outcome placeholder only — it disappears the instant the schema has any real outcome (even the default Submit gets outcome-submit). Agents following the old docs left every real outcome's handle dangling: invisible in canvas, but the runtime still executed the flow as if it were connected. 2. Both docs canonicalized inserting a Decision node after every HITL node to re-branch on $vars.{nodeId}.status. That's redundant: each outcome's own handle has showButton: true specifically so it can branch directly. Rewrote the wiring rules, port tables, and example flows in both docs to use outcome-<outcome.id> per outcome and branch directly off those handles. Also fixed three uipath-maestro-flow test files that graded the old (wrong) behavior as correct — they would have started failing agents that follow the corrected skill, not because of a regression but because the grading itself still expected the generic completed port: - smoke_02_completed_port_wired.yaml asserted the bare substring "completed" against a 2-outcome (Approve/Reject) schema, which the corrected skill never emits. Now asserts outcome-approve and outcome-reject are both wired and outcome-completed is absent; updated the prompt to ask for both outcome branches explicitly. - check_simulated_hitl.py and check_devcon_expense_approval.py both hard-required a completed/outcome-completed edge. Now derive the required outcome-<id> ports from the flow's own parsed schema.outcomes[] and fail if any is unwired or if outcome-completed appears alongside real outcomes. Added two negative tests to test_check_devcon_expense_approval.py covering a dangling outcome port and a wired outcome-completed placeholder. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(uipath-maestro-flow): fall back to outcome.name for id, fix check ordering CI caught two real bugs in the outcome-<id> checker changes: 1. check_simulated_hitl.py and check_devcon_expense_approval.py required an explicit outcome.id, but test_batch2_alignment.py's pre-existing fixture uses outcomes with only a name ({"name": "Approve"}), which is a valid shape per hitl-node-quickform.md's own convention of defaulting id to the lowercased name. Both checkers now fall back to name when id is absent. Also updated that fixture's edges from the legacy bare "completed" port to outcome-approve/outcome-reject, since it wires a 2-outcome node and bare "completed" is the same stale assumption this PR removes elsewhere. 2. The "wired outcome-completed alongside real outcomes" check ran after the "missing ports" check, so it could never actually fire: if outcome-completed is wired instead of the real ports, the real ports are by definition also missing, and that check always won the race. Reordered so the more specific, more useful message fires first. Updated test_rejects_outcome_completed_with_real_outcomes's expected message to match. Confirmed separately that PR #3180's "Run skill smoke tests" failure is unrelated: skill-flow-hitl-smoke-completed-port itself passed (status=SUCCESS, score=1.00); the suite dropped below the 95% pass-rate gate because of 3 failing skill-flow-datafabric-smoke-* tasks, pre-existing and outside this change's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(uipath-maestro-flow, uipath-human-in-the-loop): finish the outcome-port fix Rocky's review found incomplete Rocky Madden reviewed the original fix (request changes, verified against flow-workbench@develop source directly) and found the fix touched 2 of 8 uipath-maestro-flow files that teach the completed port, left the sibling uipath-human-in-the-loop skill teaching the opposite as a Critical Rule while marking it stable in the same PR, missed ~14 affected task YAMLs in both skills' own test suites, and got the outcome-id derivation wrong in the checkers (falling back to a lowercased name, which the product does not do). Verified the id-derivation question with the Flow session against source (build-handle-customization.ts filters outcomes by explicit id, no name fallback; the editor always assigns one at creation) before touching the checkers again. Docs, uipath-maestro-flow (6 more files): - editing-operations.md, planning-arch.md, brownfield.md, cli-commands.md: wiring instructions now say outcome-<outcome.id> per outcome. - diagnose/failure-modes.md: renamed the "HITL completed port unwired" section to "HITL outcome port unwired", rewrote symptom/cause/fix, updated the anchor and its two referrers (the table row and CAPABILITY.md). - impl.md Option 2 (coded-action-app): removed the inputs.schema.outcomes block from the example — Rocky found it flips the node's real static completed port to an outcome-derived one, which the product does not do for this node type. Documented that Option 2 keeps a static completed port regardless of the app's own outcomes. Docs, uipath-human-in-the-loop (the sibling skill impl.md delegates node writing to): - SKILL.md Critical Rule 2 rewritten: QuickForm wires outcome-<outcome.id> per outcome; App-based keeps a static completed port and must not carry an inputs.schema block. Rule 3 and the CLI-add step no longer assert a literal completed handle. - hitl-node-quickform.md Edge Wiring section rewritten to the per-outcome model; the static handleConfiguration JSON block is untouched per Rocky's own line citations. - hitl-node-apptask.md's two "identical to QuickForm" claims corrected — App-based is the one that stays static now, not QuickForm. Checkers (reverted the wrong id fallback): - check_simulated_hitl.py, check_devcon_expense_approval.py: outcome ids must be explicit (outcomes[].id) with no name-derived fallback, matching the confirmed product behavior. Also confirmed check ordering already flags a wired outcome-completed placeholder before the generic missing-ports message. - test_batch2_alignment.py: its fixture used name-only outcomes ({"name": "Approve"}), which the checker fallback used to paper over; gave it explicit ids instead. Tests (the ~14+ files Rocky flagged in both skills' suites): - uipath-maestro-flow/hitl/quality_01..04, smoke_03: prompts and criteria updated. quality_01 drops its Decision node (redundant once outcome ports branch directly); quality_03, smoke_03, and human-in-the-loop's quality_10 keep their Decision nodes since those branch on a boolean field value, not on which outcome was clicked — a genuinely different case the corrected planning.md calls out as still valid. quality_04_brownfield_insert.yaml's criterion always scored 0 against the corrected skill (it asserted the literal substring "completed" against a 2-outcome schema); replaced with a check for both real outcome ports. - uipath-human-in-the-loop/e2e_01..06, quality_04/05/07/08/09/10/11: prompts and criteria updated the same way. e2e_02 and e2e_04 used file_contains (no regex support) and were converted to run_command + flow_contains.py --regex to express "a real outcome port, not the placeholder." e2e_07 (App-based, skip: true) is unchanged — it already correctly wires a static completed handle. - TEST_PLAN.md: invariant table and per-task summaries updated to match. - uipath-maestro-flow/interactive/expense_approval_simulated.yaml and e2e/devcon_expense_approval.yaml: only their descriptive comments said "completed" — the actual grading already runs through the corrected checker scripts above, so behavior did not change, only the prose. Not fixed here: the description of the failure mode in the original PR overstated it ("ran the flow as if connected"). Rocky's source read (hitl-edge-reconciliation.ts) says a lone placeholder edge remaps onto the primary outcome and is cosmetic; the real breakage is (a) multi-outcome schemas where only the primary gets remapped and the rest dangle, and (b) a placeholder edge duplicating a real outcome-<id> edge, which the BPMN engine takes unconditionally alongside the conditioned one and double-executes that branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(flow): restore scaffold_multinode run_limits dropped by #2534 (#2572) The criteria migration in #2534 replaced the file's final criterion block and accidentally swallowed the trailing run_limits block, so the task fell back to the experiment defaults (max_turns 200 / task_timeout 1200) instead of its 40-turn / 900s limits. Restore the exact prior block. Config-only revert to the pre-#2534 state; no criteria changed. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(test): raise e2e_01 timeout to 3600, 2400 confirmed insufficient 2026-09-11 run 2026-09-11_04-17-20: ERROR, "Agent turn timed out after 2400s" (iteration 1, 119 assistant turns, max_turns_exhausted False). The iteration log shows continuous real work for the full 2400s, not a loop: live SharePoint/SAP connection discovery, an IXP taxonomy lookup for invoice extraction, and registry lookups for every node type, each a real network round trip, with the agent still on setup work at the 25-minute mark. Raised both task_timeout and turn_timeout to 3600, matching the established heavy-single-turn budget already used elsewhere in the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): scale e2e_01 down to focus on HITL wiring, not connector discovery skill-hitl-e2e-invoice-approval-greenfield kept timing out for a reason neither #3196 (1200s -> 2400s) nor the 2400s -> 3600s bump addressed at the root: the prompt required real SharePoint extraction and real SAP posting, so every run did live connector/registry/IXP-taxonomy discovery before ever touching the HITL node the test is meant to grade. 2026-09-11 run 2026-09-11_04-17-20 spent all 2400s and 119 assistant turns on exactly that discovery work, still mid-setup. Mocked both integration points as Script nodes instead of raising the timeout again. The test exists to exercise HITL schema design and outcome-port wiring, not connector discovery, and the prompt still requires the agent to recognize the write-back + approval gate signal on its own. Dropped task_timeout/turn_timeout back to 1200/1200, matching the sibling e2e brownfield tasks that already mock their upstream/downstream the same way, and confirmed locally: coder-eval 0.12.0, claude-sonnet-4-6, SUCCESS, score 1.000, 7/7 criteria, 482.7s, 34 assistant turns. Raised expected_turns (14 -> 34) and max_turns (40 -> 50) to match that real usage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(uipath-maestro-flow, uipath-human-in-the-loop): address remaining inline review comments Went back through every inline comment on the review (not just the summary) and found several the earlier round-2/round-3 fixes had not actually addressed: - impl.md:69/182 and planning.md:55 overclaimed "outcome-completed never reappears" / "is not a real outcome port". It is an ordinary string, not a reserved one: an outcome whose own id is literally "completed" produces the identical port. Reworded to the accurate claim (zero-outcome placeholder, or a real outcome id'd "completed") in both files, plus the impl.md debug-table row. - check_simulated_hitl.py's new outcome-id/wiring block made outcomes mandatory, contradicting the function's own earlier gate that explicitly accepts a boolean-decision field with no outcomes. Extracted the shared logic into assert_outcome_wiring() and gated it on outcomes being non-empty; the zero-outcome path now correctly falls back to requiring the completed placeholder, matching pre-existing behavior. - Same file (and check_devcon_expense_approval.py) had the outcome-completed check running after the missing-ports check, so it could never fire (a wired outcome-completed always looks like every real port is missing). Reordered in both. Caught by re-running the unit tests locally before pushing, not just by inspection. - Added check_outcome_wiring, a generic node-scoped check (handles 1+ HITL nodes) built on the same assert_outcome_wiring(), and pointed smoke_02 and 7 other task YAMLs at it instead of a flow_contains.py --absent-regex. That regex was file-scoped: it would false-fail the moment a flow gained an unrelated node with a legitimate completed port (an Option 2 HITL node), and it pinned outcome-approve/outcome-reject as guessed literals rather than deriving them from the schema. - Added the missing outcome-id conversion rule to hitl-node-quickform.md's table (same lowercase-name convention as field id), so those literals are now backed by a documented rule instead of one example. - Deduplicated the repeated 6-line comment block across check_simulated_hitl.py, check_devcon_expense_approval.py, and smoke_02 into one full statement in check_simulated_hitl.py plus short pointers, and dropped the "confirmed against ... 2026-09-10" narration in favor of a file a reader can open. - Added two tests to test_check_devcon_expense_approval.py: a mixed-case outcome id (the handle must use it verbatim, never lowercased) and an outcome with no id (must fail loudly, not get silently dropped). Ran the full local test suite before pushing: 19/19 in the two directly affected test files, no HITL/outcome-related failures anywhere else in uipath-maestro-flow (133 pre-existing failures elsewhere are unrelated — test_criterion_budgets.py and a Data Fabric connector suite, matching CI's already-known unrelated flakiness). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Nikhil Maryala <87343340+nikhil-maryala@users.noreply.github.com> (cherry picked from commit 81bb535)
…ecision node after HITL (#3180) * fix(uipath-maestro-flow): wire per-outcome handles, drop the forced Decision node after HITL Two confirmed bugs in the HITL authoring guidance, verified against flow-workbench source with the Flow skill team: 1. impl.md and planning.md only ever documented one output port, outcome-completed, regardless of how many outcomes a QuickForm schema declares. outcome-completed is actually the zero-outcome placeholder only — it disappears the instant the schema has any real outcome (even the default Submit gets outcome-submit). Agents following the old docs left every real outcome's handle dangling: invisible in canvas, but the runtime still executed the flow as if it were connected. 2. Both docs canonicalized inserting a Decision node after every HITL node to re-branch on $vars.{nodeId}.status. That's redundant: each outcome's own handle has showButton: true specifically so it can branch directly. Rewrote the wiring rules, port tables, and example flows in both docs to use outcome-<outcome.id> per outcome and branch directly off those handles. Also fixed three uipath-maestro-flow test files that graded the old (wrong) behavior as correct — they would have started failing agents that follow the corrected skill, not because of a regression but because the grading itself still expected the generic completed port: - smoke_02_completed_port_wired.yaml asserted the bare substring "completed" against a 2-outcome (Approve/Reject) schema, which the corrected skill never emits. Now asserts outcome-approve and outcome-reject are both wired and outcome-completed is absent; updated the prompt to ask for both outcome branches explicitly. - check_simulated_hitl.py and check_devcon_expense_approval.py both hard-required a completed/outcome-completed edge. Now derive the required outcome-<id> ports from the flow's own parsed schema.outcomes[] and fail if any is unwired or if outcome-completed appears alongside real outcomes. Added two negative tests to test_check_devcon_expense_approval.py covering a dangling outcome port and a wired outcome-completed placeholder. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(uipath-maestro-flow): fall back to outcome.name for id, fix check ordering CI caught two real bugs in the outcome-<id> checker changes: 1. check_simulated_hitl.py and check_devcon_expense_approval.py required an explicit outcome.id, but test_batch2_alignment.py's pre-existing fixture uses outcomes with only a name ({"name": "Approve"}), which is a valid shape per hitl-node-quickform.md's own convention of defaulting id to the lowercased name. Both checkers now fall back to name when id is absent. Also updated that fixture's edges from the legacy bare "completed" port to outcome-approve/outcome-reject, since it wires a 2-outcome node and bare "completed" is the same stale assumption this PR removes elsewhere. 2. The "wired outcome-completed alongside real outcomes" check ran after the "missing ports" check, so it could never actually fire: if outcome-completed is wired instead of the real ports, the real ports are by definition also missing, and that check always won the race. Reordered so the more specific, more useful message fires first. Updated test_rejects_outcome_completed_with_real_outcomes's expected message to match. Confirmed separately that PR #3180's "Run skill smoke tests" failure is unrelated: skill-flow-hitl-smoke-completed-port itself passed (status=SUCCESS, score=1.00); the suite dropped below the 95% pass-rate gate because of 3 failing skill-flow-datafabric-smoke-* tasks, pre-existing and outside this change's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(uipath-maestro-flow, uipath-human-in-the-loop): finish the outcome-port fix Rocky's review found incomplete Rocky Madden reviewed the original fix (request changes, verified against flow-workbench@develop source directly) and found the fix touched 2 of 8 uipath-maestro-flow files that teach the completed port, left the sibling uipath-human-in-the-loop skill teaching the opposite as a Critical Rule while marking it stable in the same PR, missed ~14 affected task YAMLs in both skills' own test suites, and got the outcome-id derivation wrong in the checkers (falling back to a lowercased name, which the product does not do). Verified the id-derivation question with the Flow session against source (build-handle-customization.ts filters outcomes by explicit id, no name fallback; the editor always assigns one at creation) before touching the checkers again. Docs, uipath-maestro-flow (6 more files): - editing-operations.md, planning-arch.md, brownfield.md, cli-commands.md: wiring instructions now say outcome-<outcome.id> per outcome. - diagnose/failure-modes.md: renamed the "HITL completed port unwired" section to "HITL outcome port unwired", rewrote symptom/cause/fix, updated the anchor and its two referrers (the table row and CAPABILITY.md). - impl.md Option 2 (coded-action-app): removed the inputs.schema.outcomes block from the example — Rocky found it flips the node's real static completed port to an outcome-derived one, which the product does not do for this node type. Documented that Option 2 keeps a static completed port regardless of the app's own outcomes. Docs, uipath-human-in-the-loop (the sibling skill impl.md delegates node writing to): - SKILL.md Critical Rule 2 rewritten: QuickForm wires outcome-<outcome.id> per outcome; App-based keeps a static completed port and must not carry an inputs.schema block. Rule 3 and the CLI-add step no longer assert a literal completed handle. - hitl-node-quickform.md Edge Wiring section rewritten to the per-outcome model; the static handleConfiguration JSON block is untouched per Rocky's own line citations. - hitl-node-apptask.md's two "identical to QuickForm" claims corrected — App-based is the one that stays static now, not QuickForm. Checkers (reverted the wrong id fallback): - check_simulated_hitl.py, check_devcon_expense_approval.py: outcome ids must be explicit (outcomes[].id) with no name-derived fallback, matching the confirmed product behavior. Also confirmed check ordering already flags a wired outcome-completed placeholder before the generic missing-ports message. - test_batch2_alignment.py: its fixture used name-only outcomes ({"name": "Approve"}), which the checker fallback used to paper over; gave it explicit ids instead. Tests (the ~14+ files Rocky flagged in both skills' suites): - uipath-maestro-flow/hitl/quality_01..04, smoke_03: prompts and criteria updated. quality_01 drops its Decision node (redundant once outcome ports branch directly); quality_03, smoke_03, and human-in-the-loop's quality_10 keep their Decision nodes since those branch on a boolean field value, not on which outcome was clicked — a genuinely different case the corrected planning.md calls out as still valid. quality_04_brownfield_insert.yaml's criterion always scored 0 against the corrected skill (it asserted the literal substring "completed" against a 2-outcome schema); replaced with a check for both real outcome ports. - uipath-human-in-the-loop/e2e_01..06, quality_04/05/07/08/09/10/11: prompts and criteria updated the same way. e2e_02 and e2e_04 used file_contains (no regex support) and were converted to run_command + flow_contains.py --regex to express "a real outcome port, not the placeholder." e2e_07 (App-based, skip: true) is unchanged — it already correctly wires a static completed handle. - TEST_PLAN.md: invariant table and per-task summaries updated to match. - uipath-maestro-flow/interactive/expense_approval_simulated.yaml and e2e/devcon_expense_approval.yaml: only their descriptive comments said "completed" — the actual grading already runs through the corrected checker scripts above, so behavior did not change, only the prose. Not fixed here: the description of the failure mode in the original PR overstated it ("ran the flow as if connected"). Rocky's source read (hitl-edge-reconciliation.ts) says a lone placeholder edge remaps onto the primary outcome and is cosmetic; the real breakage is (a) multi-outcome schemas where only the primary gets remapped and the rest dangle, and (b) a placeholder edge duplicating a real outcome-<id> edge, which the BPMN engine takes unconditionally alongside the conditioned one and double-executes that branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(flow): restore scaffold_multinode run_limits dropped by #2534 (#2572) The criteria migration in #2534 replaced the file's final criterion block and accidentally swallowed the trailing run_limits block, so the task fell back to the experiment defaults (max_turns 200 / task_timeout 1200) instead of its 40-turn / 900s limits. Restore the exact prior block. Config-only revert to the pre-#2534 state; no criteria changed. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(test): raise e2e_01 timeout to 3600, 2400 confirmed insufficient 2026-09-11 run 2026-09-11_04-17-20: ERROR, "Agent turn timed out after 2400s" (iteration 1, 119 assistant turns, max_turns_exhausted False). The iteration log shows continuous real work for the full 2400s, not a loop: live SharePoint/SAP connection discovery, an IXP taxonomy lookup for invoice extraction, and registry lookups for every node type, each a real network round trip, with the agent still on setup work at the 25-minute mark. Raised both task_timeout and turn_timeout to 3600, matching the established heavy-single-turn budget already used elsewhere in the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): scale e2e_01 down to focus on HITL wiring, not connector discovery skill-hitl-e2e-invoice-approval-greenfield kept timing out for a reason neither #3196 (1200s -> 2400s) nor the 2400s -> 3600s bump addressed at the root: the prompt required real SharePoint extraction and real SAP posting, so every run did live connector/registry/IXP-taxonomy discovery before ever touching the HITL node the test is meant to grade. 2026-09-11 run 2026-09-11_04-17-20 spent all 2400s and 119 assistant turns on exactly that discovery work, still mid-setup. Mocked both integration points as Script nodes instead of raising the timeout again. The test exists to exercise HITL schema design and outcome-port wiring, not connector discovery, and the prompt still requires the agent to recognize the write-back + approval gate signal on its own. Dropped task_timeout/turn_timeout back to 1200/1200, matching the sibling e2e brownfield tasks that already mock their upstream/downstream the same way, and confirmed locally: coder-eval 0.12.0, claude-sonnet-4-6, SUCCESS, score 1.000, 7/7 criteria, 482.7s, 34 assistant turns. Raised expected_turns (14 -> 34) and max_turns (40 -> 50) to match that real usage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(uipath-maestro-flow, uipath-human-in-the-loop): address remaining inline review comments Went back through every inline comment on the review (not just the summary) and found several the earlier round-2/round-3 fixes had not actually addressed: - impl.md:69/182 and planning.md:55 overclaimed "outcome-completed never reappears" / "is not a real outcome port". It is an ordinary string, not a reserved one: an outcome whose own id is literally "completed" produces the identical port. Reworded to the accurate claim (zero-outcome placeholder, or a real outcome id'd "completed") in both files, plus the impl.md debug-table row. - check_simulated_hitl.py's new outcome-id/wiring block made outcomes mandatory, contradicting the function's own earlier gate that explicitly accepts a boolean-decision field with no outcomes. Extracted the shared logic into assert_outcome_wiring() and gated it on outcomes being non-empty; the zero-outcome path now correctly falls back to requiring the completed placeholder, matching pre-existing behavior. - Same file (and check_devcon_expense_approval.py) had the outcome-completed check running after the missing-ports check, so it could never fire (a wired outcome-completed always looks like every real port is missing). Reordered in both. Caught by re-running the unit tests locally before pushing, not just by inspection. - Added check_outcome_wiring, a generic node-scoped check (handles 1+ HITL nodes) built on the same assert_outcome_wiring(), and pointed smoke_02 and 7 other task YAMLs at it instead of a flow_contains.py --absent-regex. That regex was file-scoped: it would false-fail the moment a flow gained an unrelated node with a legitimate completed port (an Option 2 HITL node), and it pinned outcome-approve/outcome-reject as guessed literals rather than deriving them from the schema. - Added the missing outcome-id conversion rule to hitl-node-quickform.md's table (same lowercase-name convention as field id), so those literals are now backed by a documented rule instead of one example. - Deduplicated the repeated 6-line comment block across check_simulated_hitl.py, check_devcon_expense_approval.py, and smoke_02 into one full statement in check_simulated_hitl.py plus short pointers, and dropped the "confirmed against ... 2026-09-10" narration in favor of a file a reader can open. - Added two tests to test_check_devcon_expense_approval.py: a mixed-case outcome id (the handle must use it verbatim, never lowercased) and an outcome with no id (must fail loudly, not get silently dropped). Ran the full local test suite before pushing: 19/19 in the two directly affected test files, no HITL/outcome-related failures anywhere else in uipath-maestro-flow (133 pre-existing failures elsewhere are unrelated — test_criterion_budgets.py and a Data Fabric connector suite, matching CI's already-known unrelated flakiness). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Nikhil Maryala <87343340+nikhil-maryala@users.noreply.github.com> (cherry picked from commit 81bb535)
…ecision node after HITL (#3180) * fix(uipath-maestro-flow): wire per-outcome handles, drop the forced Decision node after HITL Two confirmed bugs in the HITL authoring guidance, verified against flow-workbench source with the Flow skill team: 1. impl.md and planning.md only ever documented one output port, outcome-completed, regardless of how many outcomes a QuickForm schema declares. outcome-completed is actually the zero-outcome placeholder only — it disappears the instant the schema has any real outcome (even the default Submit gets outcome-submit). Agents following the old docs left every real outcome's handle dangling: invisible in canvas, but the runtime still executed the flow as if it were connected. 2. Both docs canonicalized inserting a Decision node after every HITL node to re-branch on $vars.{nodeId}.status. That's redundant: each outcome's own handle has showButton: true specifically so it can branch directly. Rewrote the wiring rules, port tables, and example flows in both docs to use outcome-<outcome.id> per outcome and branch directly off those handles. Also fixed three uipath-maestro-flow test files that graded the old (wrong) behavior as correct — they would have started failing agents that follow the corrected skill, not because of a regression but because the grading itself still expected the generic completed port: - smoke_02_completed_port_wired.yaml asserted the bare substring "completed" against a 2-outcome (Approve/Reject) schema, which the corrected skill never emits. Now asserts outcome-approve and outcome-reject are both wired and outcome-completed is absent; updated the prompt to ask for both outcome branches explicitly. - check_simulated_hitl.py and check_devcon_expense_approval.py both hard-required a completed/outcome-completed edge. Now derive the required outcome-<id> ports from the flow's own parsed schema.outcomes[] and fail if any is unwired or if outcome-completed appears alongside real outcomes. Added two negative tests to test_check_devcon_expense_approval.py covering a dangling outcome port and a wired outcome-completed placeholder. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(uipath-maestro-flow): fall back to outcome.name for id, fix check ordering CI caught two real bugs in the outcome-<id> checker changes: 1. check_simulated_hitl.py and check_devcon_expense_approval.py required an explicit outcome.id, but test_batch2_alignment.py's pre-existing fixture uses outcomes with only a name ({"name": "Approve"}), which is a valid shape per hitl-node-quickform.md's own convention of defaulting id to the lowercased name. Both checkers now fall back to name when id is absent. Also updated that fixture's edges from the legacy bare "completed" port to outcome-approve/outcome-reject, since it wires a 2-outcome node and bare "completed" is the same stale assumption this PR removes elsewhere. 2. The "wired outcome-completed alongside real outcomes" check ran after the "missing ports" check, so it could never actually fire: if outcome-completed is wired instead of the real ports, the real ports are by definition also missing, and that check always won the race. Reordered so the more specific, more useful message fires first. Updated test_rejects_outcome_completed_with_real_outcomes's expected message to match. Confirmed separately that PR #3180's "Run skill smoke tests" failure is unrelated: skill-flow-hitl-smoke-completed-port itself passed (status=SUCCESS, score=1.00); the suite dropped below the 95% pass-rate gate because of 3 failing skill-flow-datafabric-smoke-* tasks, pre-existing and outside this change's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(uipath-maestro-flow, uipath-human-in-the-loop): finish the outcome-port fix Rocky's review found incomplete Rocky Madden reviewed the original fix (request changes, verified against flow-workbench@develop source directly) and found the fix touched 2 of 8 uipath-maestro-flow files that teach the completed port, left the sibling uipath-human-in-the-loop skill teaching the opposite as a Critical Rule while marking it stable in the same PR, missed ~14 affected task YAMLs in both skills' own test suites, and got the outcome-id derivation wrong in the checkers (falling back to a lowercased name, which the product does not do). Verified the id-derivation question with the Flow session against source (build-handle-customization.ts filters outcomes by explicit id, no name fallback; the editor always assigns one at creation) before touching the checkers again. Docs, uipath-maestro-flow (6 more files): - editing-operations.md, planning-arch.md, brownfield.md, cli-commands.md: wiring instructions now say outcome-<outcome.id> per outcome. - diagnose/failure-modes.md: renamed the "HITL completed port unwired" section to "HITL outcome port unwired", rewrote symptom/cause/fix, updated the anchor and its two referrers (the table row and CAPABILITY.md). - impl.md Option 2 (coded-action-app): removed the inputs.schema.outcomes block from the example — Rocky found it flips the node's real static completed port to an outcome-derived one, which the product does not do for this node type. Documented that Option 2 keeps a static completed port regardless of the app's own outcomes. Docs, uipath-human-in-the-loop (the sibling skill impl.md delegates node writing to): - SKILL.md Critical Rule 2 rewritten: QuickForm wires outcome-<outcome.id> per outcome; App-based keeps a static completed port and must not carry an inputs.schema block. Rule 3 and the CLI-add step no longer assert a literal completed handle. - hitl-node-quickform.md Edge Wiring section rewritten to the per-outcome model; the static handleConfiguration JSON block is untouched per Rocky's own line citations. - hitl-node-apptask.md's two "identical to QuickForm" claims corrected — App-based is the one that stays static now, not QuickForm. Checkers (reverted the wrong id fallback): - check_simulated_hitl.py, check_devcon_expense_approval.py: outcome ids must be explicit (outcomes[].id) with no name-derived fallback, matching the confirmed product behavior. Also confirmed check ordering already flags a wired outcome-completed placeholder before the generic missing-ports message. - test_batch2_alignment.py: its fixture used name-only outcomes ({"name": "Approve"}), which the checker fallback used to paper over; gave it explicit ids instead. Tests (the ~14+ files Rocky flagged in both skills' suites): - uipath-maestro-flow/hitl/quality_01..04, smoke_03: prompts and criteria updated. quality_01 drops its Decision node (redundant once outcome ports branch directly); quality_03, smoke_03, and human-in-the-loop's quality_10 keep their Decision nodes since those branch on a boolean field value, not on which outcome was clicked — a genuinely different case the corrected planning.md calls out as still valid. quality_04_brownfield_insert.yaml's criterion always scored 0 against the corrected skill (it asserted the literal substring "completed" against a 2-outcome schema); replaced with a check for both real outcome ports. - uipath-human-in-the-loop/e2e_01..06, quality_04/05/07/08/09/10/11: prompts and criteria updated the same way. e2e_02 and e2e_04 used file_contains (no regex support) and were converted to run_command + flow_contains.py --regex to express "a real outcome port, not the placeholder." e2e_07 (App-based, skip: true) is unchanged — it already correctly wires a static completed handle. - TEST_PLAN.md: invariant table and per-task summaries updated to match. - uipath-maestro-flow/interactive/expense_approval_simulated.yaml and e2e/devcon_expense_approval.yaml: only their descriptive comments said "completed" — the actual grading already runs through the corrected checker scripts above, so behavior did not change, only the prose. Not fixed here: the description of the failure mode in the original PR overstated it ("ran the flow as if connected"). Rocky's source read (hitl-edge-reconciliation.ts) says a lone placeholder edge remaps onto the primary outcome and is cosmetic; the real breakage is (a) multi-outcome schemas where only the primary gets remapped and the rest dangle, and (b) a placeholder edge duplicating a real outcome-<id> edge, which the BPMN engine takes unconditionally alongside the conditioned one and double-executes that branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(flow): restore scaffold_multinode run_limits dropped by #2534 (#2572) The criteria migration in #2534 replaced the file's final criterion block and accidentally swallowed the trailing run_limits block, so the task fell back to the experiment defaults (max_turns 200 / task_timeout 1200) instead of its 40-turn / 900s limits. Restore the exact prior block. Config-only revert to the pre-#2534 state; no criteria changed. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(test): raise e2e_01 timeout to 3600, 2400 confirmed insufficient 2026-09-11 run 2026-09-11_04-17-20: ERROR, "Agent turn timed out after 2400s" (iteration 1, 119 assistant turns, max_turns_exhausted False). The iteration log shows continuous real work for the full 2400s, not a loop: live SharePoint/SAP connection discovery, an IXP taxonomy lookup for invoice extraction, and registry lookups for every node type, each a real network round trip, with the agent still on setup work at the 25-minute mark. Raised both task_timeout and turn_timeout to 3600, matching the established heavy-single-turn budget already used elsewhere in the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): scale e2e_01 down to focus on HITL wiring, not connector discovery skill-hitl-e2e-invoice-approval-greenfield kept timing out for a reason neither #3196 (1200s -> 2400s) nor the 2400s -> 3600s bump addressed at the root: the prompt required real SharePoint extraction and real SAP posting, so every run did live connector/registry/IXP-taxonomy discovery before ever touching the HITL node the test is meant to grade. 2026-09-11 run 2026-09-11_04-17-20 spent all 2400s and 119 assistant turns on exactly that discovery work, still mid-setup. Mocked both integration points as Script nodes instead of raising the timeout again. The test exists to exercise HITL schema design and outcome-port wiring, not connector discovery, and the prompt still requires the agent to recognize the write-back + approval gate signal on its own. Dropped task_timeout/turn_timeout back to 1200/1200, matching the sibling e2e brownfield tasks that already mock their upstream/downstream the same way, and confirmed locally: coder-eval 0.12.0, claude-sonnet-4-6, SUCCESS, score 1.000, 7/7 criteria, 482.7s, 34 assistant turns. Raised expected_turns (14 -> 34) and max_turns (40 -> 50) to match that real usage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(uipath-maestro-flow, uipath-human-in-the-loop): address remaining inline review comments Went back through every inline comment on the review (not just the summary) and found several the earlier round-2/round-3 fixes had not actually addressed: - impl.md:69/182 and planning.md:55 overclaimed "outcome-completed never reappears" / "is not a real outcome port". It is an ordinary string, not a reserved one: an outcome whose own id is literally "completed" produces the identical port. Reworded to the accurate claim (zero-outcome placeholder, or a real outcome id'd "completed") in both files, plus the impl.md debug-table row. - check_simulated_hitl.py's new outcome-id/wiring block made outcomes mandatory, contradicting the function's own earlier gate that explicitly accepts a boolean-decision field with no outcomes. Extracted the shared logic into assert_outcome_wiring() and gated it on outcomes being non-empty; the zero-outcome path now correctly falls back to requiring the completed placeholder, matching pre-existing behavior. - Same file (and check_devcon_expense_approval.py) had the outcome-completed check running after the missing-ports check, so it could never fire (a wired outcome-completed always looks like every real port is missing). Reordered in both. Caught by re-running the unit tests locally before pushing, not just by inspection. - Added check_outcome_wiring, a generic node-scoped check (handles 1+ HITL nodes) built on the same assert_outcome_wiring(), and pointed smoke_02 and 7 other task YAMLs at it instead of a flow_contains.py --absent-regex. That regex was file-scoped: it would false-fail the moment a flow gained an unrelated node with a legitimate completed port (an Option 2 HITL node), and it pinned outcome-approve/outcome-reject as guessed literals rather than deriving them from the schema. - Added the missing outcome-id conversion rule to hitl-node-quickform.md's table (same lowercase-name convention as field id), so those literals are now backed by a documented rule instead of one example. - Deduplicated the repeated 6-line comment block across check_simulated_hitl.py, check_devcon_expense_approval.py, and smoke_02 into one full statement in check_simulated_hitl.py plus short pointers, and dropped the "confirmed against ... 2026-09-10" narration in favor of a file a reader can open. - Added two tests to test_check_devcon_expense_approval.py: a mixed-case outcome id (the handle must use it verbatim, never lowercased) and an outcome with no id (must fail loudly, not get silently dropped). Ran the full local test suite before pushing: 19/19 in the two directly affected test files, no HITL/outcome-related failures anywhere else in uipath-maestro-flow (133 pre-existing failures elsewhere are unrelated — test_criterion_budgets.py and a Data Fabric connector suite, matching CI's already-known unrelated flakiness). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Nikhil Maryala <87343340+nikhil-maryala@users.noreply.github.com> (cherry picked from commit 81bb535)
Problem
QuickForm HITL nodes wire one
outcome-<outcome.id>port per outcome. Docs, task fixtures, and one checker across bothuipath-maestro-flowanduipath-human-in-the-looptaught or graded a singlecompletedhandle instead, and a canonicalized Decision node was forced after every HITL node regardless of outcome count.Fixed
completedunconditionally: corrected to per-outcomeoutcome-<id>ports.inputs.schemablock it shouldn't have had: block removed, port stays the staticcompletedfor that node type.outcome-completeddocumented as never reappearing once outcomes exist (impl.md, planning.md): corrected. It is the zero-outcome placeholder, or the port for a real outcome whose ownidhappens to becompleted.completedhandle, or checked wiring with a file-scoped regex against a guessed literal port id: prompts and success criteria reworded to outcome-port language;smoke_02,quality_04in both skills, and the 6e2e_*invoice/escalation/gdpr/multi-hitl/expense tasks now call a new node-scoped, id-derivedcheck_simulated_hitl.py outcome-wiringcheck instead.TEST_PLAN.mdupdated to match.check_simulated_hitl.pyandcheck_devcon_expense_approval.pyderived the wiring port from a lowercasedoutcome.name, falling back to that whenidwas missing: both now readidverbatim (matching flow-workbench'sbuild-handle-customization.ts) and fail loudly when it is missing, instead of validating a flow that would leave an outcome dangling at runtime.uipath-human-in-the-loopwas bundled into the original fix: split out to chore(uipath-human-in-the-loop, uipath-tasks): mark both skills stable #3224, gated on this merging first.Review findings addressed
All 12 inline review comments from the last pass replied to individually and resolved: documentation cascade (6 files), sibling-skill contradiction, the two overclaim rewordings, the checker id-derivation bug, the check-ordering bug, both test fixture gaps (mixed-case id, missing id), and the file-scoped-regex replacement.
Test plan
skill-flow-hitl-smoke-completed-port(smoke_02): SUCCESS, score 1.00, in CIskill-hitl-e2e-invoice-approval-greenfield(e2e_01) run locally: SUCCESS, score 1.000, 7/7 criteria, 482.7stest_criterion_budgets.pyand a Data Fabric connector suite, matching CI's known flakiness there)Known overlap, needs reconciling before merge
#3239 (different author) touches the same two checker files here and claims the default v1.0 HITL node only ever accepts a
completededge, with outcome-id ports gated behind an opt-inoutcomePortsflag on v1.1+. A local coder-eval run authored atypeVersion: "1.0"node wired withoutcome-approve/outcome-rejectedges anduip maestro flow validatepassed clean, contradicting that. Needs reconciling before either PR merges.🤖 Generated with Claude Code