Repository navigation
feat: persist permanent webhook delivery failures - #388
Merged
Merged
Conversation
|
@Herikcool Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Collaborator
Collaborator
|
Needs a rebase — Worth knowing before you rebase: #402 replaced every Push the rebase and I will pick it up; nothing is queued ahead of it. |
Herikcool
force-pushed
the
fix/issue-342
branch
from
October 20, 2026 10:34
8f238e1 to
2936372
Compare
Contributor
Author
|
@Miracle656 The branch has been rebased onto the latest main and the conflicts are resolved. Please review and merge when ready. |
Miracle656
added a commit
that referenced
this pull request
Oct 22, 2026
tsc --noEmit clean vitest 91 files, 1052 passing (up from 86 / 962 on main) #251 read and scale token decimals from the chain #307 standardise empty-result vs not-found status codes #318 validate the live service against the published OpenAPI #324 content-derived strong ETag and honour conditional requests #326 cover vwap and the aggregator #388 persist permanent webhook delivery failures BREAKING, and deliberate: #307 changes routes that returned 404 for "no data" to 200 with an explicit reason. The rule is now 404 = the resource does not exist (pair not watched, key not found), 200 + empty = the query matched nothing. Our dropped-test guard surfaced it as - src/__tests__/basket.test.ts :: returns 404 when an asset has no price data which turned out to be renamed rather than deleted, to "returns 200 with error and reason when an asset has no price data". That is the guard working as intended: it made a behaviour change a decision instead of an accident. Any client branching on 404 for empty results needs updating; #427 (API versioning) is open and is where this belongs long-term. Three fixes applied on merge: - #388 and #318 each added a console.* call to non-test source, which the repo's own logging-hygiene test forbids. Both now use createLogger, which is what delivery.ts was already doing two lines above the offending call. - #326's test asserted CONFIDENCE_HIGH/MEDIUM_THRESHOLD_SECONDS, which do not exist. The constants are real and the values match, but the shipped names are HIGH_/MEDIUM_CONFIDENCE_MAX_AGE_SECONDS, from #545. Updated the test to the shipped names rather than renaming source to fit a stale test. - npx prisma generate, for #388's WebhookDeliveryFailure model. NOT merged: #384 (periodic WebSocket ping) does not compile -- it reads config.ws and never adds it to src/config.ts, and it drops a fanout test. #254 conflicts. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Miracle656
added a commit
that referenced
this pull request
Oct 22, 2026
tsc --noEmit clean vitest 97 files, 1127 passing (up from 93 / 1068) dropped-tests none #325 GET /movers for market gainers and losers #327 GET /correlation for basket construction #485 net arbitrage spread against fees #494 declare the resources capability and expose pages over MCP Two fixes on merge. #325 shipped the route file and the OpenAPI entry but never registered it. The repo's own guard caught it: AssertionError: Documented but unregistered: ["GET /movers"] So the endpoint was documented, generated into clients, and did not exist. registerMoversRoutes already followed the house signature, so wiring it was an import and a call next to registerScreenerRoutes. That guard is worth its weight -- a documented route that 404s is worse than an undocumented one, because a client is generated against it. #327 added a console.error to src/routes/correlation.ts, which the logging hygiene test forbids. Now createLogger, the same change #388 and #318 needed in the previous batch. Three PRs in two batches have tripped that test, which suggests the rule is not discoverable enough from the outside -- worth a line in CONTRIBUTING rather than catching it per-PR. NOT merged, each failing on its own branch rather than from merge order: #460 Phoenix AMM adapter -- 5 failures alone, including its own networkVenueConfig and venues/registry tests #353 1 tsc error #354 1 tsc error #482 4 tsc errors #393 #431 #434 #472 conflict Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
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
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.
Summary
Closes #342