Skip to content

fix(mcp): classify paid call errors by structure and codes before text matching (#410) - #442

Open
sparkysolutions10-pixel wants to merge 1 commit into
Divinity-Labs:mainfrom
sparkysolutions10-pixel:fix/mcp-error-classification-structure-410
Open

sparkysolutions10-pixel wants to merge 1 commit into
Divinity-Labs:mainfrom
sparkysolutions10-pixel:fix/mcp-error-classification-structure-410

Conversation

@sparkysolutions10-pixel

@sparkysolutions10-pixel sparkysolutions10-pixel commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Resolves #410

Summary of Changes

This PR refactors handlePaidCallError in src/mcp/server.ts to classify failures by error structure, Horizon result codes, and HTTP statuses first, rather than relying on brittle message substring inspection:

  1. Structure-First Error Classification (Errors are classified by searching the message text #410):

    • Horizon Result Codes: Inspects e.response?.data?.extras?.result_codes and e.resultCodes:
      • op_no_trust maps directly to ERR_MISSING_TRUSTLINE
      • op_underfunded, op_low_reserve, tx_insufficient_balance, tx_insufficient_fee map directly to ERR_INSUFFICIENT_BALANCE
      • tx_bad_auth, tx_bad_seq, tx_too_late, op_bad_auth map to ERR_EXPIRED_AUTHORISATION
    • Explicit Ledger Expiration: Inspects signatureExpirationLedger and currentLedger properties or error code fields.
    • HTTP Status Codes: Maps x402 HTTP client status codes directly:
      • 402 -> ERR_PAYMENT_REJECTED
      • 401/403 -> ERR_EXPIRED_AUTHORISATION
      • 502/503/504 -> ERR_SERVICE_UNREACHABLE
      • 400-range -> ERR_BAD_REQUEST
    • Network & Socket Codes: Inspects e.code / e.cause.code (ENOTFOUND, ECONNREFUSED, ETIMEDOUT) and timeout names.
    • Anti-Fragile Fallback: Text matching (msg.includes(...)) is retained purely as a last-resort fallback for unstructured third-party errors. Messages containing the word "fetch" with underlying trustline errors are no longer misclassified as service outages.
  2. Vitest Unit Test Suite:

    • Added tests in tests/mcp.test.ts verifying structure-first discrimination across Horizon operations, transaction codes, HTTP statuses, and anti-fragility against reworded messages.
    • All 12 tests passing in 37ms.
    • Changeset added under .changeset/mcp-structured-error-classification.md.

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: one issue per PR, and only issues assigned to you

This is a process requirement, applied to all 23 of your open PRs in this repo. It is not a judgement about the code, which I have not been able to review — and that is the first half of the problem.

The bundling

Across your open PRs, 150 distinct issues are claimed. This one claims several. #483 claims 22, #481 claims 20, #477 claims 20, #474 claims 20.

A PR that closes twenty issues cannot be reviewed. Each of those issues has its own acceptance criteria — typically five — so twenty issues is a hundred separate things to verify in one diff, and if any one of them is wrong the whole PR is blocked while it is fixed. Every batch I published says the issues are deliberately independent so they can be worked in parallel, by different people, without colliding. Bundling them removes exactly that property.

The assignments

135 of the 150 issues claimed are assigned to other contributors — 67 different people.

This PR is an example: it names issues currently assigned to other contributors who are working on them now. Several have open PRs of their own. If this merged as it stands, those people would have done the work and received nothing for it, and I would have no way to tell the two efforts apart.

I am not assuming anything about why. Assignments change, Drips and GitHub can disagree, and a wide-ranging branch picks up issue numbers easily. But the effect on the people holding those assignments is the same regardless of intent, which is why I have to act on it.

What I need

  • One issue per PR. The issue number in the title, its acceptance criteria met, and nothing else in the diff.
  • Only issues assigned to you, or unassigned ones you have claimed through Drips. If an issue is assigned to someone else, it is theirs.
  • Re-open the work that meets both conditions as individual PRs. I will review them in the order they arrive and at the same pace as everyone else's.

Nothing is being closed. These stay open and are yours to re-cut, withdraw or split as you choose.

Two things worth knowing before you re-cut

Some of this has already landed. #488 (SSRF guard for bazaar_search) and #487 (the paid_call host allow-list) cover the same ground as your #449; #486 covers the MCP README. Check current main before rebuilding something that is already there.

main has moved a long way today — #306, #309, #252, #256, #355, #381, #390, #402, #476, #488 and more. Cut from current main, not from where these branches started.

If you believe an issue named here is genuinely yours, say which and I will check the assignment myself — I would rather be corrected than have someone lose work.

@sparkysolutions10-pixel
sparkysolutions10-pixel force-pushed the fix/mcp-error-classification-structure-410 branch from df6893c to 7f2e66b Compare October 19, 2026 20:39
@Miracle656

Copy link
Copy Markdown
Collaborator

This has been overtaken, but part of it is better than what landed and I want to say exactly which part.

#410 was assigned to @Izzyabbey2020, and their PR for it — #480 — merged this morning. handlePaidCallError now delegates to classifyPaidCallError in src/mcp/codes.ts, which reads structured signals before falling back to text and records which path answered. So the headline of this PR is done, and the issue is closed.

What yours covers that main still does not:

$ grep -cE "402|403|502|503|504|status" src/mcp/codes.ts
0

The merged classifier reads Horizon result_codes and error codes, but it does not look at HTTP status at all on a thrown error. Yours maps:

  • 402 → ERR_PAYMENT_REJECTED
  • 401 / 403 → ERR_EXPIRED_AUTHORISATION
  • 502 / 503 / 504 → ERR_SERVICE_UNREACHABLE

and it reads a wider set of result codes — op_low_reserve, tx_insufficient_balance, tx_insufficient_fee, tx_bad_auth, tx_bad_seq, tx_too_late — plus explicit signatureExpirationLedger / currentLedger properties.

That is real coverage. server.ts handles a 402 on a response, but an x402 client that throws with a status attached currently falls through to text matching, which is the exact failure mode #410 was about.

Being straight about what I can offer for it. The Lens Wave budget for this round is spent — 24,950 of 25,000 — so I cannot fund a follow-up issue here at the moment, and I would rather tell you that than invite work that cannot be paid. If you want to extend classifyPaidCallError with the status mapping and the extra result codes as a small PR against current main, I will review and merge it on its merits, and it would be first in line for points when the budget resets.

If you would rather not, that is entirely reasonable and nothing is lost — the analysis above is now on the record for whoever picks it up.

I am not closing this; that is not mine to do unilaterally. Close it whenever suits, or say the word and I will.

@Miracle656

Copy link
Copy Markdown
Collaborator

Re-reviewed after your push. The situation has changed underneath this PR, and not in your favour — but there is something here worth keeping, so please read to the end before closing it.

#410 is now closed. It was assigned to @Izzyabbey2020, and their implementation is merged. The structural classification this PR does is therefore already on main, which also means this branch now conflicts.

I want to be straight that this is partly our process failing you rather than you doing anything wrong: you did the work, it is competent, and it collided with an assignee. That has now happened across a number of your PRs, and the fix on our side is that an assigned issue is spoken for — if an issue has an assignee, the PR that lands is theirs, even if yours arrives first and is good.

What is still genuinely missing, and is yours if you want it. I checked the merged classifier rather than assuming it superseded you:

$ grep -cE "402|403|502|503|504" src/mcp/codes.ts
0

The merged version classifies by structure and by error codes, but it never maps transport status. A 502 from a Bazaar resource and a 403 are both just failures to it. Your PR covers exactly that gap.

So: close this one, and open a small PR that adds only the HTTP status mapping on top of the merged classifier. That is a much smaller diff than this, it does not duplicate anyone, and it is a real improvement to code that is already shipped. If you do that, link it here and I will review it quickly.

I am not closing this myself — it is yours to close.

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.

Errors are classified by searching the message text

2 participants