fix(oas:sync): recognize OAS 3.1 webhooks so sync stops deleting their pages - #43
Open
rossrdme wants to merge 5 commits into
Open
fix(oas:sync): recognize OAS 3.1 webhooks so sync stops deleting their pages#43rossrdme wants to merge 5 commits into
rossrdme wants to merge 5 commits into
Conversation
Contributor
Author
|
@erunion this fixes a webhooks bug |
Member
erunion
approved these changes
Aug 27, 2026
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
rossrdme
force-pushed
the
fix/oas-sync-webhooks
branch
from
August 28, 2026 04:20
83bb655 to
63af916
Compare
rossrdme
force-pushed
the
fix/oas-sync-webhooks
branch
from
August 28, 2026 05:02
63af916 to
109a29e
Compare
erunion
self-requested a review
August 28, 2026 16:16
…r pages extractOperations only read spec.paths, so an OAS 3.1 spec's top-level webhooks (calls the API itself makes to a client-registered URL — a separate, same-shaped sibling of paths, not a path the client calls) were invisible to it. Two symptoms, found while validating this against a repo synced by the real platform: - oas:sync's delete pass treats any existing page whose operationId isn't in its operation set as orphaned. A webhook-backed page's operationId is never in that set, so every sync run deleted it — reproduced against a real webhooks page and confirmed the deletion happens on unmodified main. - The oas-reference lint validator reported a false "Operation not found" for the same pages. extractOperations now also walks spec.webhooks, using the same synthetic `<method>_<name>` operationId scheme already used for paths (verified it reproduces the platform's own post_paymentcompleted / post_paymentfailed convention exactly), and marks generated pages with `api.webhook: true` to match what the platform stamps on them. Grouping (tag, or the webhook's own name when untagged) and page generation fall out of the existing operationGroup/buildPageContent machinery from #35 with no special-casing. Stacked on fix/oas-sync-upload-parity (#35): untagged webhook grouping reuses that branch's path-derived-group logic, so this targets that branch rather than main. Verified end-to-end against a repo synced by the real platform: wiping and regenerating all specs from scratch now reproduces the platform's webhook pages exactly (folder, filename, api.webhook, category title from the raw webhook name) with zero remaining diff beyond already-known, out-of-scope gaps (category ordering with no declared position, YAML quoting of brace-containing titles).
…lve $ref webhooks
Two issues from Greptile review:
- Operation IDs collide: paths and webhooks are separate namespaces, but a
synthetic <method>_<name> id can legitimately be identical across them
(e.g. POST /orders and webhook POST orders both omitting operationId both
synthesize to post_orders). extractOperations collected both into one Map
keyed only by operationId, so the second (webhook) pass silently
overwrote the path entry — sync then omitted the path's page entirely,
and both oas-sync's delete pass and oas-reference's lint checks lost
visibility of it. Added operationKey({operationId, isWebhook}) and use it
everywhere an operation or an existing page is looked up by id — in
oas-sync.js's pagesByOpId/specOps, and oas-reference.js's "Operation not
found"/"Missing page" checks, which had the identical vulnerability on
the read side (two on-disk pages sharing an operationId would collapse
to one coveredOps entry). The written operationId itself is untouched —
only the internal lookup key changed.
- Webhook references stay unresolved: an OAS 3.1 webhooks (or paths) entry
can be a Reference Object (`{ $ref: '#/components/pathItems/Name' }`)
rather than a literal Path Item. collect() iterated it directly, and
since "$ref" isn't an HTTP method the whole entry was silently skipped.
Added resolveLocalPathItemRef to resolve same-document
#/components/pathItems/<name> refs before iterating methods (external
refs and other pointer shapes are left unresolved, same graceful
degradation as before). Applies uniformly to paths and webhooks since
both go through the same collect() helper — this was a pre-existing gap
for paths too, not unique to webhooks.
Verified both new regression tests fail against the prior code and pass
against this fix.
Addresses review from Greptile. resolveLocalPathItemRef only resolved one level: a webhook/path entry whose pathItems target was itself a $ref (rather than a literal Path Item) returned that intermediate Reference Object unchanged. collect() then skipped it (still just a "$ref" key, no HTTP methods), so sync deleted the existing page as orphaned and reference validation reported it missing. Now follows the chain until a literal Path Item is reached, tracking every $ref string seen so a cycle returns the current (still-unresolved) node instead of looping forever — same graceful degradation as an unresolvable name. Added tests for a two-hop chain and a circular reference (confirms it returns instead of hanging).
rossrdme
force-pushed
the
fix/oas-sync-webhooks
branch
from
August 28, 2026 17:00
109a29e to
9cd1905
Compare
Addresses review from Greptile. decodeURIComponent throws a URIError on a malformed percent-escape (e.g. a $ref segment containing "%zz"). That propagated straight up through resolveLocalPathItemRef -> collect -> extractOperations, aborting the entire oas:sync or lint run over one bad $ref, instead of the graceful degradation used for every other unresolvable case here (unknown name, external ref, cycle). Wrapped in try/catch; a malformed escape is now just left unresolved. Verified the added regression test throws against the prior code and passes against this fix.
greptile-apps
Bot
dismissed
their stale review
August 29, 2026 00:20
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
…m \$ref Addresses review from Greptile. OAS 3.1 explicitly permits sibling fields (e.g. an inline operation) alongside \$ref in a Path Item Object. resolveLocalPathItemRef replaced the entire entry with the referenced pathItem, discarding any inline sibling operation declared next to the \$ref — sync then deleted its existing page as orphaned, and reference lint couldn't recognize it. Now accumulates sibling fields from every hop in the chain and merges them over the final resolved (or last-reached, if unresolvable) node. An outer hop's field wins over the same field found deeper in the chain, since OAS itself leaves that case "undefined." Verified the added regression test (inline operation alongside a \$ref to a different pathItem) fails against the prior code and passes now.
Contributor
Author
|
@erunion I think this is good now |
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
2 participants
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.
Context
Found while validating #35's grouping fix against a repo synced by the real ReadMe platform.
extractOperationsonly readsspec.paths, so an OAS 3.1 spec's top-levelwebhooks(calls the API itself makes to a client-registered URL — a same-shaped sibling ofpaths, not something a client calls into) are invisible to it.That has two real, confirmed symptoms:
oas:sync's delete pass treats any existing page whoseoperationIdisn't in its known-operations set as orphaned and deletes it. A webhook-backed page's operationId is never in that set, so every sync run deletes it. Reproduced this against a real webhook page and confirmed it happens today on unmodifiedmain.oas-referencevalidator reportsOperation not foundfor the same, entirely valid pages.Fix
extractOperationsnow also walksspec.webhooks, using the same synthetic<method>_<name>operationId scheme already used forpathsoperations without an explicitoperationId. Generated pages are markedapi.webhook: true, matching what the platform stamps on them. Grouping (by tag, or by the webhook's own name when untagged) and page generation fall out of the existingoperationGroup/buildPageContentmachinery from #35 — no special-casing needed.Why this targets
fix/oas-sync-upload-parity, notmainUntagged webhook grouping reuses #35's path-derived-group logic (a webhook's own name plays the same role a path does). Rebasing this onto
maindirectly would mean duplicating that logic, or regressing untagged webhooks into the old sharedOther/bucket that #35 just fixed for paths. This is a stacked PR — review/merge after #35.Verification
test/oas-sync.test.js(page generation,api.webhookmarker, no-deletion, tag/path collision) andtest/oas-reference.test.js(no false "Operation not found").api.webhookmarkers now match exactly. Remaining diff is limited to already-known, out-of-scope gaps unrelated to this fix (category order with no declared position to anchor to, YAML quoting of brace-containing titles).Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com