Skip to content

fix(maestro-flow/e2e): accept per-outcome exits in the devcon HITL completion gate - #3239

Closed
dmetzgar wants to merge 1 commit into
mainfrom
fix/devcon-hitl-accept-outcome-ports
Closed

dmetzgar wants to merge 1 commit into
mainfrom
fix/devcon-hitl-accept-outcome-ports

Conversation

@dmetzgar

Copy link
Copy Markdown
Collaborator

The DevCon HITL completion gate accepted only one of two correct authoring shapes, so the grade turned on which one the agent happened to pick.

The two shapes

  • The base node's 1.0 definition — and every variant (quick-form, action-app, document-validation) — declares one source handle: completed.
  • outcomePorts: true / exposeError: true select the base node's 1.1/1.2 definition, whose only source handle is outcome-{item.id}, repeated over the outcomes. There is no completed handle there, and an edge to one is refused by uip maestro flow validate as an undeclared source handle.

The gate required completed, or the outcome-completed special case #1477 added — which only helps when an outcome is literally named "Completed", so an Approve/Reject design can never satisfy it. The task's initial_prompt asks for neither shape; it describes approve/reject review plus a downstream log step.

What that cost

skill-flow-e2e-devcon-expense-approval scored 1.0 when the agent authored the first shape and 0.37 the next run when it authored the second — with nothing in the stack changed in between. outcomePorts shipped 2026-08-19, and the SDK's HITL docs are byte-identical across the two SDK versions involved (git diff v3.31.2 v5.0.9 -- references/hitl.md is empty). Details in flow-builder-sdk#718.

In the earlier passing comparison run, v1 emitted …quick-form@1.0 and v2 the base …human-in-the-loop@1.0 — both on completed. Neither agent entered the failing shape, so the gate had never been exercised against it.

The change

The gate now asserts what it means to assert — that the review continues somewhere: an edge on completed, or any outcome-<id> edge. Nothing wired at all is still a failure, and the message names what it found instead of naming one handle.

The success line no longer claims "uses v1.0 schema … wires completed"; it reports the exits it saw. Nothing in the repo asserted that string.

Verification

Same fixture, both checkers:

old: FAIL: HITL completed handle must be wired
new: OK: HITL node reviewExpense uses a v1.x schema, captures approval + reason,
     continues on ['outcome-approve', 'outcome-reject'], and uses .output paths

_flow_doc now takes node_type, type_version and exit_ports, defaulting to exactly the shape every existing test used, so the two new cases — the outcome-ports shape, and a no-continuation flow that must still fail — sit alongside unchanged coverage. 11 passed.

Upstream half

The doc gap that sent the agent down the unsupported path is fixed separately in flow-builder-sdk#724: hitl.md now states that outcomePorts replaces the completed exit and how to choose between the shapes, and flow check gained HITL_COMPLETED_PORT_GONE so the SDK stops emitting an edge the product rejects. The two changes are independent — neither waits on the other.

🤖 Generated with Claude Code

…mpletion gate

The gate required an edge on `completed` (or the `outcome-completed` special
case #1477 added, which only helps when an outcome is literally NAMED
"Completed"). The SDK has two correct shapes for a human task, and that gate
accepts only one of them:

  * the base node's 1.0 definition — and every variant: quick-form,
    action-app, document-validation — declares one source handle, `completed`;
  * `outcomePorts: true` / `exposeError: true` select the base node's 1.1/1.2
    definition, whose ONLY source handle is `outcome-{item.id}` repeated over
    the outcomes. There is no `completed` handle there, and an edge to one is
    refused by `uip maestro flow validate` as an undeclared source handle.

So the second shape could not pass, and the task's `initial_prompt` asks for
neither — it describes approve/reject review and a downstream log step. Which
shape the agent picks decided the grade: `skill-flow-e2e-devcon-expense-approval`
scored 1.0 when the agent wrote the first shape and 0.37 the next run when it
wrote the second, with nothing in the stack changed between them
(flow-builder-sdk#718; `outcomePorts` shipped 2026-08-19 and the SDK's HITL docs
are byte-identical across the two SDK versions involved).

What the gate means to assert is that the review CONTINUES somewhere, so it now
asserts that: `completed`, or any `outcome-<id>` edge. Nothing wired at all is
still a failure, and the message names what it found.

The upstream doc gap is fixed separately (flow-builder-sdk#724): hitl.md now
says outcomePorts REPLACES the completed exit, and `flow check` refuses an edge
to a handle the selected definition does not declare.

Verified: the old checker answers `FAIL: HITL completed handle must be wired` on
an outcome-port flow, the new one accepts it and reports the exits it found. The
fixture helper takes the node type, version and exit ports so both shapes are
covered; a no-continuation case pins the refusal. 11 passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dmetzgar
dmetzgar requested a review from a team as a code owner September 11, 2026 05:39
@github-actions

github-actions Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @dmetzgar's task in 3m 13s —— View job


PR Review: fix(maestro-flow/e2e): accept per-outcome exits in the devcon HITL completion gate

Summary

Fixes a flaky e2e grade caused by the DevCon expense-approval checker only accepting the completed exit handle, rejecting the equally valid outcome-<id> shape produced when outcomePorts: true is used. The gate now asserts "the HITL continues somewhere" instead of "the HITL uses completed." Two new tests lock in both the positive (outcome-ports accepted) and negative (no continuation rejected) paths.

Change-by-Change Review

1. tests/tasks/uipath-maestro-flow/e2e/check_devcon_expense_approval.py

Severity: OK

The core logic change (lines 140-153) is correct and well-reasoned:

  • Builds a set of all exit ports from edges leaving the HITL node.
  • Passes if "completed" is present or any port starts with "outcome-".
  • Fails with a descriptive message naming the expected shapes and what was actually found.

The edge-case handling is sound:

  • str(e.get("sourcePort") or "") produces "" for missing/None ports, which won't false-positive on either condition.
  • sorted(exit_ports) or 'no outgoing edges' — when the set is empty, sorted() returns [] (falsy), so the fallback string is used. Good.
  • The success message (lines 164-167) now reports the actual exits instead of claiming "wires completed", which was inaccurate for the outcome-ports shape.

One minor observation: the comment block (lines 120-139) is 20 lines long. It's explaining genuinely non-obvious domain behavior that would prevent a future developer from "fixing" the gate back to completed-only, so the length is defensible — but it could be trimmed to ~8 lines (the two bullet shapes + the one-sentence failure history) without losing the protective value.

2. tests/tasks/uipath-maestro-flow/e2e/test_check_devcon_expense_approval.py

Severity: OK

Clean, backward-compatible extension of the _flow_doc helper:

  • Three new keyword args (node_type, type_version, exit_ports) all default to exactly what existing callers produced, so no existing test behavior changes.
  • The edges list comprehension for port in exit_ports (line 67) neatly generates 0, 1, or N edges — replacing the former single hardcoded edge. When exit_ports=(), it produces an empty list, which is exactly what test_rejects_hitl_with_no_continuation needs.
  • test_accepts_outcome_ports_instead_of_completed (line 175): exercises the base uipath.human-in-the-loop node at typeVersion: "1.1" with outcome-approve / outcome-reject exits. This is the exact shape that caused the flaky grade in flow-builder-sdk#718. Correct.
  • test_rejects_hitl_with_no_continuation (line 198): verifies exit_ports=() fails with the expected message. Asserts against result.stdout + result.stderr (line 210), which is appropriate since sys.exit(string) writes to stderr. Correct.

What's Missing

Nothing identified. The change is tightly scoped to the checker and its tests. No skill docs, frontmatter, CODEOWNERS, or registry files are affected.

Area Ratings

Area Status Notes
Frontmatter N/A No skill doc changes
E2E Tests OK Two new tests cover both the accept and reject paths for the new shape
Skill Body N/A No SKILL.md changes
References & Assets N/A No reference changes
Repo Hygiene OK Changes scoped to two test files, no secrets, no cross-skill deps

Issues for Manual Review

  • Comment verbosity (check_devcon_expense_approval.py:120-139): The 20-line rationale comment is protective but long. Whether to trim is an editorial call — it doesn't affect correctness. Could compress to ~8 lines (the two shapes + the one-sentence failure history) without losing the "don't revert this" signal.

Conclusion

Clean fix. The logic is correct, the edge cases are handled, backward compatibility is preserved via defaults, and both directions (accept + reject) are tested. No issues to block on.


@dushyant-uipath

Copy link
Copy Markdown
Collaborator

#3180
@dmetzgar I had an open PR under review for the same here - Rocky was reviewing it.

We can reverify if this one's needed once mine merges?

@dmetzgar

Copy link
Copy Markdown
Collaborator Author

Closing in favour of #3180, which fixes the same gate more thoroughly and from the right premise.

Reconciliation measured and written up in #3180 (comment). The short version:

  • fix(uipath-maestro-flow): wire per-outcome handles, drop the forced Decision node after HITL #3180's author was right that a typeVersion: "1.0" node wired with outcome-* edges validates clean. The mechanism is that flow validate special-cases the outcome- prefix — a literal handle like bogus-port is refused against the embedded definition, anything shaped outcome-<id> is waved through. So per-outcome wiring is legitimate on 1.0 as well, and this PR's framing described the SDK's authoring surface rather than the platform's contract.
  • The canvas derives one outcome-<id> handle per schema outcome at render time (getEffectiveHitlHandleCustomization), with completed as the zero-outcome placeholder. So the leniency this PR added — accept completed OR any outcome-* — would keep blessing a shape the designer does not render.
  • fix(uipath-maestro-flow): wire per-outcome handles, drop the forced Decision node after HITL #3180 also catches a partially-routed outcome set (missing: outcome-reject), which this PR did not.

The one piece worth lifting from here is the fixture parameterisation (_flow_doc taking node_type / type_version / exit_ports), if #3180's tests don't already reach those shapes another way.

The SDK half of flow-builder-sdk#718 is merged as flow-builder-sdk#724; the remaining SDK-side gap — the quick-form and action-app variants cannot express per-outcome handles at all — is being filed separately.

@dmetzgar dmetzgar closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants