cherry-pick(release/v1.202): wire per-outcome handles, drop the forced Decision node after HITL (#3180) - #3268
dushyant-uipath wants to merge 1 commit into
Conversation
|
Claude finished @dushyant-uipath's task in 3m 38s —— View job PR Review: Cherry-pick HITL per-outcome handles onto
|
| Area | Status | Notes |
|---|---|---|
| Frontmatter | OK | No frontmatter changes in this PR |
| E2E Tests | OK | New negative tests for outcome-wiring edge cases; all tasks updated |
| Skill Body | OK | Critical Rules, wiring patterns, and anti-patterns all updated consistently |
| References & Assets | OK | All anchor links verified; no broken cross-references |
| Repo Hygiene | OK | No secrets, no cross-skill structural dependencies, no unrelated changes |
Issues for Manual Review
- Domain accuracy: The claim that
outcome-completedis a zero-outcome placeholder that disappears once real outcomes exist is a UiPath platform behavior — cannot be verified from the docs alone. The PR author's commit message and test evidence suggest this is correct. - Conflict resolution: The PR states one conflict in
impl.mdwas resolved by keeping incoming rules verbatim. The resulting file reads cleanly with no merge artifacts.
Conclusion
Clean cherry-pick. The conceptual shift from single completed port to per-outcome outcome-<id> ports is applied consistently across all 37 changed files — skill docs, test tasks, and test checkers are all aligned. The new assert_outcome_wiring checker is well-designed with good negative test coverage. Two low-severity items noted (stale success message, duplicated logic) — neither blocks merge.
|
Claude finished @dushyant-uipath's task in 4m 47s —— View job Coder-eval task lint (advisory)21 task YAMLs changed (all modifications, no new tasks); verdicts: 0 Critical, 0 High, 0 Medium, 4 Low, 17 OK. Rubric: .claude/commands/lint-task.md. This check is advisory and never blocks merge. Evidence of passing run❌ High — PR body claims Per-task lint
|
9e1205c to
e665e91
Compare
…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)
e665e91 to
2d98151
Compare
|
Closing. The fix (#3180) is already present in release/v1.203, which branched off main after #3180 merged, so no cherry-pick was needed there. Skipping this 1.202 backport rather than fighting the release-branch CI infra issue (org secrets not reaching the smoke-test job's auth step, needs org-admin access to diagnose, tracked separately). 1.203.0 is still in preview on npm (highest published: 1.203.0-preview.889; latest stable is 1.202.1) as of 2026-09-21. |
1 participant
Cherry-pick of #3180 (main, merge commit 81bb535) onto
release/v1.202. Rebased 2026-09-17 onto currentrelease/v1.202tip (0c48173..31d5b7c), which already carries the separately-merged #3277 and #3280 cherry-picks. No conflicts on this rebase.One conflict on the original cherry-pick (
skills/uipath-maestro-flow/references/author/plugins/hitl/impl.md, context drift around the AppTask rules bullet list), resolved by keeping the incoming rules verbatim.Real run proof the fix works
uip maestro flow validatereturns{"Result": "Success", "Data": {"Status": "Valid"}}with the outcomes wired asoutcome-approve/outcome-reject(notcompleted), and the App-based node's staticcompletedport left untouched.skill-hitl-e2e-invoice-approval-greenfield(e2e_01) run locally through coder-eval: SUCCESS, score 1.000, 7/7 criteria, 482.7s.skill-hitl-e2e-expense-approval-brownfield(e2e_05) run locally through coder-eval: SUCCESS, score 1.000, 4/4 criteria, 332.3s.Test plan
python3 -m pyteston the two directly affected test files: 19/19 passing on this branch.uip maestro flow validaterun against a hand-built flow using this branch's documented wiring: passes.🤖 Generated with Claude Code