fix: correct every tool to Sherweb's published API contract - #67
Merged
Conversation
Eight of the nine API-backed tools called endpoints that do not exist.
The paths were REST-shaped guesses (/customers/{id}/subscriptions/{subId},
/payable-charges/{chargeId}, /catalog/products) against an API that is
operation-shaped: Sherweb keys subscriptions and receivable charges off
/billing/ with the customer passed as a customerId query parameter. Every
call 404'd while tools/list looked healthy, so the server advertised a full
toolset and then failed on use.
Paths are transcribed from the vendor's published definitions —
Sherweb.Apis.Distributor.OpenAPI.json and the Distributor Postman collection
in github.com/sherweb/Public-Apis, plus the OpenAPI definitions behind each
operation on developers.sherweb.com/reference — and pinned by URL-asserting
contract tests in src/domains/endpoints.test.ts.
- billing_payable_charges: /payable-charges -> /billing/payable-charges.
Dropped billingCycleType/periodFrom/periodTo (response fields on Charge,
not query inputs) and page/pageSize (no Sherweb endpoint paginates);
added the one documented parameter, date.
- billing_charge_details: no per-charge endpoint exists; selects from the
billing period instead.
- customers_list: search/page/pageSize were silently ignored (GetCustomers
documents no query parameters); search is now applied client-side.
- customers_get: no GET /customers/{id}; selects from the collection.
- customers_accounts_receivable: -> /billing/receivable-charges?customerId=
- subscriptions_list: -> /billing/subscriptions?customerId=
- subscriptions_get: no single-subscription endpoint; reads
/billing/subscriptions/details?customerId= and filters.
- subscriptions_change_quantity: -> POST /billing/subscriptions/amendments
?customerId= with the documented subscriptionAmendmentParameters batch
body. Amendments are asynchronous, so the tool now returns the amendment
and tracking IDs rather than presenting the change as applied.
- catalog_list_products: -> /customer-catalogs/{customerId}. Catalogs are
per-customer, so customerId is required.
- The MCP Apps subscription card's customer lookup used the same
nonexistent /customers/{id} and now resolves from the collection.
Adds sherweb_subscriptions_amendment_status, polling GET /tracking/{id}
for an amendment's outcome (Sherweb deprecates amendments/{id}/status in
favour of TrackRequest).
BREAKING CHANGE: tool input schemas changed. sherweb_catalog_list_products
now requires customerId; sherweb_billing_payable_charges takes date instead
of billingCycleType/periodFrom/periodTo; page/pageSize and the customers
search pagination parameters are removed; and
sherweb_subscriptions_change_quantity returns an async receipt rather than a
completed change. The previous parameters had no effect, since the endpoints
they were sent to did not exist.
Cleanup pass over the endpoint-contract fix. No change to which endpoint any
tool calls.
- Extract jsonResult/errorResult/findByKey/matches and the shared date-param
description into utils/types.ts. The four domain handlers were spelling out
{content:[{type:"text",text:JSON.stringify(x,null,2)}]} 11 times and the
isError shape 7 times, and each had hand-rolled its own fetch-collection /
find-by-id / not-found sequence.
- Give the "there is no GET /customers/{id}" workaround one owner: export
findCustomer from domains/customers.ts and call it from the subscription
card lookup, which had a verbatim second copy.
- Derive the lazy-loading category registry from handler.getTools() instead of
hand-listing tool names, and single-source the domain descriptions as
DOMAIN_DESCRIPTIONS. The hand-written copy had already drifted in the
previous commit: two of four descriptions still read the pre-fix wording.
A tool missing from that list fails asymmetrically -- advertised by
sherweb_list_category_tools, rejected by sherweb_execute_tool -- so
endpoints.test.ts now pins that every exposed tool resolves to a domain.
- Run the subscription-details fetch and the card's customer lookup
concurrently; they share only customerId, so serializing them was pure
latency. The details payload has no customer name, so the second request is
genuinely required.
- Unify the two copies of the "no search -> return everything" branch, which
had silently diverged: the customers path dropped sibling response fields
while the catalog path preserved them. Both preserve them now.
- Delete elicitSelection/elicitText/ElicitOption, dead since the parameters
they prompted for (billing cycle, product search) turned out not to exist in
the Sherweb API. Correct the card.builder docblock, which still cited the
removed /customers/{id} endpoint.
- Drop the hand-built params objects that re-did the undefined-skipping
utils/client.ts already does, and log at the call sites so charge_details and
customers_get stop emitting another tool's label.
Deliberately not done: no response caching or memoization was added. The
collection-per-lookup cost is real, but a shared cache here would have to be
tenant-keyed, and this codebase removed a cross-tenant token cache two commits
ago (3ee79d9) -- not worth reintroducing that shape for a latency win. List
results are also still unbounded; capping them is a behavior change worth
deciding separately.
release.yml already states that "PR-time lint/test gating is each repo's ci.yml concern" -- but no ci.yml existed, so that concern had no owner. The repo's only two workflows were mcp-assert.yml (boot + canary tool) and release.yml (release path, main-only), which means npm test, npm run lint and npm run typecheck have never run on a pull request here. That is the same shape as the bug this branch fixes: the endpoint-contract defect shipped past 33 green tests, none of which asserted an outbound URL -- and no automation was running them in either case. The new URL-asserting contract tests would have caught it, and without this file they would never have executed in CI either. Adopts wyre-technology/.github mcp-server-ci.yml, pinned to adad2aa6 (the current head of that file, 2026-08-07, which adds the process.env credential-mutation guard). Defaults cover the Node 22/24 matrix, lint, typecheck, build, unit tests, and that guard. Pre-flighted the two inputs that could fail on this repo before enabling them: - credential-mutation guard: grep for process.env assignment in src/ is clean (the only such assignment is in s2s-guard-ordering.test.ts, which the guard's own exclusion list skips). - lint-destructive-warnings: left off -- this repo has no scripts/lint-destructive-warnings.mjs. docker-smoke-test is on so Dockerfile breakage surfaces at PR time instead of at release time; release.yml builds the same image on main. Integration inputs are left empty because every test here is hermetic (fetch is stubbed), so there is no vendor secret to gate on. mcp-assert.yml stays: boot-and-list is a genuinely different signal from the test suite, and it caught nothing wrong here only because the failure was in the request URLs rather than the tool list.
Contributor
Author
|
Reviewed (forge). Diff read in full, not just the summary. Two independently significant fixes bundled here, both real:
New CI green (Test Node22/24, Docker smoke test, assert), mergeable, no conflicts. This is a live security fix sitting unreviewed 122h+ — recommend prioritizing over the routine mechanical fixes in this batch. Flagging to boss/Aaron for merge sign-off given the security content. |
Contributor
Author
|
Correction to my review above (boss caught this, verified independently): the security-escalation framing was wrong. I misread an unchanged CONTEXT line in the CHANGELOG.md diff hunk (no leading What #67 actually does, correctly characterized: fixes 8/9 tools calling nonexistent Sherweb endpoints (every real call 404ing for 2+ weeks per the PR body's own deploy-ordering note), transcribed against the vendor's published OpenAPI/Postman definitions and pinned by new contract tests. Still a real, valuable, well-tested fix — just a correctness fix, not a security one. CI green, mergeable, recommend merge in the normal queue alongside itglue-mcp#90. |
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's wrong
Eight of the nine API-backed tools called Sherweb endpoints that do not exist. Every call 404'd while
tools/listlooked perfectly healthy — the server advertised a full toolset and then failed on use.The paths were idiomatic-REST guesses (
/customers/{id}/subscriptions/{subId},/payable-charges/{chargeId},/catalog/products) against an API that is operation-shaped: Sherweb keys subscriptions and receivable charges off/billing/with the customer passed as a query parameter, and has no per-charge, per-customer or per-subscription endpoint at all.The tell:
card.builder.tsmapped Sherweb's response schemas exactly right while the routes were invented — someone read the response docs and guessed the URLs. All 33 existing tests passed because not one asserted an outbound URL.Ground truth
Every path is transcribed from the vendor's published definitions and pinned by URL-asserting contract tests in
src/domains/endpoints.test.ts:Sherweb.Apis.Distributor.OpenAPI.json+ the Distributor Postman collection in sherweb/Public-Apisbilling_payable_charges/payable-charges/billing/payable-chargesbilling_charge_details/payable-charges/{chargeId}customers_list/customers+ ignoredsearch/page/pageSize/customers, search applied client-sidecustomers_get/customers/{customerId}customers_accounts_receivable/customers/{id}/accounts-receivable/billing/receivable-charges?customerId=subscriptions_list/customers/{id}/subscriptions/billing/subscriptions?customerId=subscriptions_get/customers/{id}/subscriptions/{subId}/billing/subscriptions/details?customerId=, filteredsubscriptions_change_quantityPOST .../change-quantity{quantity}POST /billing/subscriptions/amendments?customerId=with the documented batch bodycatalog_list_products/catalog/products/customer-catalogs/{customerId}Beyond path swaps, two semantic corrections:
sherweb_subscriptions_amendment_statuspollsGET /tracking/{id}for the outcome. (Sherweb deprecatesamendments/{id}/statusin favour ofTrackRequest.)customerIdis now required.Also dropped:
page/pageSizeeverywhere (no Sherweb endpoint paginates) andbillingCycleType/periodFrom/periodToon payable charges (those are fields on the responseChargeschema, not query inputs). The one documented parameter,date, is now supported.The auth layer was already correct and is untouched. I verified the token endpoint,
client_credentials, theOcp-Apim-Subscription-Keyheader, and the space-separated"distributor service-provider"scope against the vendor docs rather than assuming — and conduit's own credential probe independently mirrors it.Second commit: cleanup
A review pass caught that the first commit introduced drift of its own — the lazy-loading category registry hand-listed tool names and duplicated the domain descriptions, and two of four descriptions were already stale. Tool membership and counts are now derived from
handler.getTools(), with a test pinning that every exposed tool resolves to a domain. That failure mode is asymmetric and nasty: a tool missing from the old hand-written list is advertised bysherweb_list_category_toolsbut rejected bysherweb_execute_tool.Also extracted the repeated result/lookup helpers, gave the "no
GET /customers/{id}" workaround a single owner, parallelized the two independent requests insubscriptions_get, and deleted the elicitation helpers left dead by the parameter removals.Verification
npm test52 passed ·npm run buildclean ·npm run lintclean ·npm run typecheckcleanRuntime-probed over stdio and HTTP: 12 tools in normal mode, 4 meta-tools in
LAZY_LOADINGmode, category counts and router suggestions correct.Not verified against the live API — there is no Sherweb credential in the secret store, so this is validated against published specs, not a real 200. Worth one smoke test before the image pin is bumped.
Deploy ordering
Blocked on / pairs with wyre-technology/conduit#1448 —
sherwebis absent fromVENDOR_TOOL_CONFIG, so all its tools are unclassified, fail closed to ADMIN, and are hidden from every non-admin caller. A live org has seen zero Sherweb tools for 2+ weeks.Landing either fix alone is insufficient:
Both, then bump the
sherwebpin invendor-fleet.conduit-prod.bicepparam.BREAKING CHANGE
Tool input schemas changed:
catalog_list_productsnow requirescustomerId;billing_payable_chargestakesdateinstead ofbillingCycleType/periodFrom/periodTo;page/pageSizeand the customers search-pagination params are gone;change_quantityreturns an async receipt rather than a completed change. The removed parameters had no effect, since the endpoints they were sent to did not exist.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.