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

Open
#91 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.