Skip downstream tasks on LLMBranchOperator reject - #71073
Conversation
3f6f5f9 to
5e7f377
Compare
5e7f377 to
a8db156
Compare
| if self.fail_on_reject: | ||
| raise | ||
| self.log.info("Rejected. Skipping all downstream tasks...") | ||
| return self.do_branch(context, None) |
There was a problem hiding this comment.
do_branch(context, None) skips every direct downstream task, teardowns included, but ApprovalOperator's reject path filters them out (yield from (t for t in tasks if not t.is_teardown)). I checked this on a real DAG with branch >> [task_a, task_b, cleanup.as_teardown()]: this path skips ['cleanup', 'task_a', 'task_b'] where ApprovalOperator skips ['task_a', 'task_b']. Before this change a reject failed the task and the teardown still ran on its all_done-style rule, so cleanup now silently stops running after a rejection.
Either filter teardowns here, or drop the "matching ApprovalOperator" wording from the docstring and the rst.
There was a problem hiding this comment.
Good catch. Now filters teardowns like ApprovalOperator and updated the docs wording accordingly.
| except HITLRejectException: | ||
| if self.fail_on_reject: | ||
| raise | ||
| self.log.info("Rejected. Skipping all downstream tasks...") |
There was a problem hiding this comment.
The exception being swallowed here carries the reviewer name (Output was rejected by the reviewer <user>.), and the task log was the only place that surfaced. event["responded_by_user"] is right here, so self.log.info("Rejected by %s. Skipping all downstream tasks.", event.get("responded_by_user")) would keep the attribution. Right now the log for a rejected gate no longer says who rejected it.
There was a problem hiding this comment.
Fixed. Reject log now includes responded_by_user, keeping reviewer attribution.
2 participants
Why
LLMBranchOperatorreview failed the task, leaving downstream tasksupstream_failed.ApprovalOperatortreats reject as a human decision and skips downstream instead; failing on reject is documented as generally discouraged.How
do_branch(context, None), skipping every downstream task.fail_on_rejectflag (defaultFalse) restores the old fail behavior, mirroringApprovalOperator.approval_timeoutexpiry still fails the task.E2E
Verified in breeze: trigger, review pauses the task, hit Reject,
route_ticketendssuccessand all downstream branches endskipped(previouslyupstream_failed).llm_branch_reject_skip_demo.webm
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Fable 5) following the guidelines