aws-samples / aws-samples/sample-multi-agent-procure-to-pay
create_payment swallows a failed submit and returns a Payment anyway, so an unposted or unknown payment is indistinguishable from a completed one
- Dominant language
- Python
- Stars
- 3
- Forks
- 2
- Avg merge
- 18h 43m
- Merged PRs (30d)
- 9
Description
The README's claim is the reason I read the payment path rather than anything else:
> Every decision is reasoned, audited, and reversible.
Three of the four hold up well — Cedar at the gateway, the reasoning guardrail, and the decision log are more machinery than most reference implementations carry. This is about the fourth link, where the money actually posts.
## The path
`backend/adapters/erpnext/adapter.py`, `create_payment` (`:565`):
```python
doc = self.client.insert("Payment Entry", doc_data)
if doc and doc.get("name"):
try:
self.client.submit("Payment Entry", doc["name"])
except Exception as e:
logger.warning(f"Payment created but submit failed: {e}") # ← swallowed
record = self.client.get("Payment Entry", doc["name"])
mapped = map_record(record, PAYMENT_TO_CANONICAL)
mapped["status"] = map_status_to_canonical(record.get("status", ""), "Payment Entry")
```
In ERPNext the `insert` creates a **draft**; the `submit` is the posting. So the two steps mean different things, and only the second one moves money.
**1. A failed submit is a warning, and the function returns normally.** The caller receives a `Payment` object and no exception. The status field is honest — `"Draft" → "draft"` per `field_maps.py:232` — but nothing *raises*, so a caller that does not inspect the status has been handed a payment that did not post. The agent's own instructions (`agents/payment_agent.py:92`) describe amounts and deductions in detail and say nothing about checking the returned status, which is a reasonable thing for a prompt to omit and exactly why it should not be the only guard.
**2. The submit that matters most is the one this cannot classify.** `submit` throwing on a timeout, a dropped connection or a 5xx does not mean the posting did not happen — it means nobody knows. The `except Exception` treats that identically to a validation rejection. The `client.get` immediately afterwards may resolve it, or may read a stale replica and return `draft` for an entry that in fact posted.
**3. There is no dedup, so the natural remedy creates a second payment.** `create_payment` has no idempotency key and no check for an existing Payment Entry against `data.invoice_id`. (The one `Deduplicate` comment in the file, `:391`, is about child-table rows in a list query and unrelated.) An agent that sees `draft` — or an operator who sees a payment that "did not go through" — retries, and ERPNext accepts a **second** Payment Entry against the same invoice. The supplier is paid twice, both entries are individually valid, and nothing in the system ever saw a duplicate.
That is the failure the README's "reversible" is doing work against: reversible assumes you know what happened.
## A smaller one in the same function
`:581`:
```python
try:
inv = self.client.get("Purchase Invoice", data.invoice_id)
invoice_outstanding = float(inv.get("outstanding_amount", 0) or ...)
except Exception:
pass # nosec B110 -- fall back to caller-supplied amount if fetch fails
```
If that fetch fails, the allocation silently becomes the caller-supplied amount instead of the true outstanding. With the discount logic below it, a failed read can produce a different write-off than intended, and the only trace is that no trace exists.
## What would close it
1. **Do not return normally when `submit` fails.** Raise, or return a result whose type says `unposted`. A draft that is indistinguishable from a payment is the whole problem.
2. **Separate "rejected" from "unknown."** A validation error means it did not post. A timeout means nobody knows, and that state should be terminal and visible until reconciled — never silently retried.
3. **Give `create_payment` an idempotency key** derived from the invoice and the intent, and check for an existing Payment Entry against `invoice_id` before inserting. This is the single change that makes a retry safe regardless of everything above.
## Why I am filing on a sample
Because it is a reference implementation for procure-to-pay, the shape gets copied into places where the supplier is real, and the guarantee in the README is the one people will rely on. Unlike some other samples in this collection there is no disclaimer in the README limiting it to demonstration, so a reader is entitled to read "audited and reversible" as a property of the design.
This is one instance of a class I read money paths for: an outcome that could not be determined, recorded as one that did not happen. Related work in the open x402 specification, if useful: [x402-foundation/x402#3437](https://github.com/x402-foundation/x402/pull/3437) and [#3438](https://github.com/x402-foundation/x402/issues/3438).
Nothing to buy and nothing needed from me. If the position is that a sample should stay simple here, that is legitimate — I would only suggest the README say so, since the current sentence reads as a design guarantee.
Contributor guide
Research direction
Start in backend/adapters/erpnext/adapter.py at create_payment (:565), then read field_maps.py:232 and agents/payment_agent.py:92 to understand the current status and caller expectations. Review the README guarantee and the invoice lookup around :581. Done means rejected and unknown submissions are distinguishable, retries cannot create duplicate Payment Entries, and invoice-fetch failures are visible rather than silently changing the allocation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, payments
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 38/100