Fix: orchestrator confirms payment to vendor even when it fails - #574
Conversation
OrchestratorAgent.delegate_to_payments set result["next_step"] to a "you MUST notify the vendor... use notification_type payment_confirmation" instruction unconditionally, regardless of whether the Payments Agent actually succeeded. A failed payment (task_status="failed") still carried this instruction into the orchestrator's next LLM turn, risking the vendor being told a payment succeeded when it never did. next_step is now only set when task_status == "success". Note: the value "completed" does not exist anywhere in this codebase -- the complete_task tool schema declares enum ["success", "failed"] (finbot/agents/base.py:373-376) -- so the guard checks "success". Fixes GenAI-Security-Project#460
There was a problem hiding this comment.
Pull request overview
This PR prevents the orchestrator from instructing itself to send a vendor payment confirmation when the Payments Agent reports a failure, eliminating a data-integrity risk in the invoice payment workflow.
Changes:
- Gate
OrchestratorAgent.delegate_to_paymentssoresult["next_step"]is only added whentask_status == "success". - Add a focused unit test suite covering success, failure, missing/unknown
task_status, and delegation-cap early return behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
finbot/agents/orchestrator.py |
Only emits the “delegate_to_communication with payment_confirmation” instruction when the payment succeeded. |
tests/unit/agents/test_orchestrator.py |
Adds regression tests ensuring next_step is present only for successful payment outcomes and absent for all other cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if result.get("task_status") == "success": | ||
| result["next_step"] = ( | ||
| "IMPORTANT: You MUST now delegate_to_communication to notify the vendor " | ||
| "about this payment outcome. Use notification_type 'payment_confirmation'. " |
There was a problem hiding this comment.
Fair point, reworded to say the payment succeeded explicitly instead of the vaguer 'outcome' wording.
There was a problem hiding this comment.
Fair point, reworded to say the payment succeeded explicitly instead of the vaguer 'outcome' wording.
| # | ||
| # Note on the fix: the issue's own suggested patch checked | ||
| # `task_status == "completed"`. That value does not exist in this codebase -- | ||
| # verified against finbot/agents/base.py:373-376, where the complete_task |
There was a problem hiding this comment.
Swapped it for a reference to the schema itself instead of line numbers, you're right those drift.
There was a problem hiding this comment.
Swapped it for a reference to the schema itself instead of line numbers, you're right those drift.
- Reworded the next_step instruction to say "this payment succeeded" instead of the more generic "this payment outcome", since the guard already limits it to task_status == "success" -- removes any ambiguity for the orchestrator LLM reading the instruction. - Replaced a hardcoded cross-file line-number reference (finbot/agents/base.py:373-376) in the test module header with a reference to the symbol itself (BaseAgent's complete_task tool schema), since line numbers drift and the exact lines cited were already off by the time this review comment was posted.
Summary
OrchestratorAgent.delegate_to_paymentsunconditionally setresult["next_step"]to an instruction telling the orchestrator's own LLM to notify the vendor withnotification_type: payment_confirmation— regardless of whether the Payments Agent actually succeeded.When the Payments Agent returns
task_status: "failed"(e.g. declined payment, insufficient funds), the orchestrator still received anext_stepsaying "IMPORTANT: You MUST now delegate_to_communication... payment_confirmation." Nothing in the flow gated this on the actual outcome, so a failed payment could result in the vendor being told it succeeded — a data integrity issue in a financial workflow.Fix
next_stepis now only set whenresult.get("task_status") == "success".Note: an earlier version of this fix (matching a suggestion in the linked issue) checked
task_status == "completed". That value doesn't exist anywhere in this codebase —complete_task's tool schema declares"enum": ["success", "failed"](finbot/agents/base.py:373-376), and a repo-wide search confirms no agent ever returns"completed". Checking for it would have silently broken the legitimate case too, sincenext_stepwould never fire even on genuine success. Verified against the actual schema before writing the guard.Test plan
tests/unit/agents/test_orchestrator.pywith 5 cases:next_stepset on success, omitted on failure, omitted whentask_statusis missing entirely, omitted on an unrecognized status value, and omitted when the delegation cap is reached (early-return path, confirmsrun_payments_agentis never even called)pytest tests/unit/agents/test_orchestrator.py -v— 5/5 passingpytest tests/unit/agents/ -q— no regressions against the rest of the agent suite (3 pre-existing, unrelated failures intest_specialized_agents.pypresent before this change)