Fix DATA-RECOVERY-001 review findings: risk-gate test coverage + honest source labeling - #61
Conversation
…st source labeling Independent review of PR #59 (DATA-RECOVERY-001) verified the trustworthy- source risk gate works correctly by hand but flagged two Medium, non-blocking findings (LA-QG-001, LA-QG-002). Addressing both here. - paperEngine.test.ts: adds regression coverage for checkTrustworthySource at its actual call sites -- processNewOrder (via submitOrder) and sweepLimitOrders -- which previously had zero automated coverage; only the standalone risk-check function and evaluateTrade's dispatch of it were tested. Each new describe block includes a positive control (a genuinely Hyperliquid-sourced order still fills) alongside the negative case, matching this file's existing ownership-test convention. - server.ts: the 1s WS-driven marketUpdate broadcast loop always labeled its payload source: 'hyperliquid', even though change24h/volume in that same payload are only as fresh as the last REST ingestion cycle, which could itself have fallen back to CoinGecko while the WS stays healthy. Now reports meta.source (the REST cycle's actual source) instead of hardcoding a label the merged-in fields don't necessarily earn. - docs/architecture/market-data.md: documents this source-attribution nuance in the broadcast loop. Full server suite: 195/195 passing (191 baseline + 4 new). Build clean.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adcc116cca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // trustworthiness of the change24h/volume figures being merged in | ||
| // alongside it (flagged by independent review of PR #59, LA-QG-002) | ||
| // -- meta.source truthfully reflects what those fields actually are. | ||
| source: meta.source, |
There was a problem hiding this comment.
When REST has fallen back to CoinGecko while the Hyperliquid WebSocket remains healthy, this payload contains a Hyperliquid mid-price but labels the entire update coingecko. The client copies that field into its market snapshot and explicitly renders the current price as Source: CoinGecko (fallback) in chartAccessibility.ts and AssetCandlestickCard.tsx, so the attempted provenance fix now misattributes the user-visible live price. Carry separate price and metadata source fields (or otherwise update the client contract) rather than assigning the metadata source to this mixed-provider row.
Useful? React with 👍 / 👎.
Independent review: PASSFull server suite: 195/195 passing. Build clean. CI green (3/3 checks). Non-vacuity of the new tests was empirically verified, not just inspected: the reviewer temporarily stripped LA-QG-002 fix confirmed correct: No findings. Ready to merge at your convenience. |
There was a problem hiding this comment.
Pull request overview
Fast-follow hardening for DATA-RECOVERY-001 review findings from PR #59 by expanding regression coverage around the trustworthy-source gate at real paper-engine call sites, and by adjusting market-data WebSocket source labeling to better reflect provenance of merged REST metadata.
Changes:
- Update the WS
marketUpdatepayload to reportsource: meta.source(REST-derived provenance) instead of hardcoding'hyperliquid'. - Add 4 unit tests in
paperEngine.test.tsto exercisecheckTrustworthySourcebehavior throughsubmitOrderandsweepLimitOrdersintegration paths. - Document the WS publish-loop provenance nuance in
docs/architecture/market-data.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/src/server.ts | Changes WS payload source labeling to use REST meta provenance (meta.source). |
| server/src/execution/paperEngine.test.ts | Adds integration-level tests for trustworthy-source gating at submitOrder / sweepLimitOrders call sites. |
| docs/architecture/market-data.md | Documents the provenance semantics of fields merged into WS market updates. |
Suppressed comments (2)
server/src/execution/paperEngine.test.ts:172
- Same issue as the previous test:
isUserHaltedis mocked as if there is noriskLimitsrow, butgetOrCreateRiskLimitsthen returns a row without any intervening insert. Keeping the mocked DB state consistent will make this test less brittle and more representative.
selectMock
.mockReturnValueOnce(dbChain([])) // isUserHalted
.mockReturnValueOnce(dbChain([marketRow('hyperliquid')])) // getMarketSnapshot
.mockReturnValueOnce(dbChain([RISK_LIMITS_ROW])) // getOrCreateRiskLimits
.mockReturnValueOnce(dbChain([{ value: 0 }])) // countOpenPositions
server/src/execution/paperEngine.test.ts:216
- Same as the prior test: the positive-control
restingOrdershould include at leastquantitysofillOrderisn't operating onNaNin test. Otherwise this can pass while not actually exercising a realistic fill path.
it('positive control: a genuinely Hyperliquid-sourced marketable resting limit order fills', async () => {
const restingOrder = { id: 'order-2', userId: 'user-a', asset: 'BTC', side: 'LONG', orderType: 'LIMIT', limitPrice: '100', status: 'ACKNOWLEDGED' };
selectMock
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // `price` here is always genuinely Hyperliquid-sourced (a live WS | ||
| // mid), but change24h/volume are only as fresh as the *last REST | ||
| // ingestion cycle*, which could itself have fallen back to | ||
| // CoinGecko even while this WS connection is healthy. Labeling the | ||
| // whole row "hyperliquid" in that window would overstate the | ||
| // trustworthiness of the change24h/volume figures being merged in | ||
| // alongside it (flagged by independent review of PR #59, LA-QG-002) | ||
| // -- meta.source truthfully reflects what those fields actually are. | ||
| source: meta.source, |
| .mockReturnValueOnce(dbChain([])) // isUserHalted -- no risk_limits row, defaults to false | ||
| .mockReturnValueOnce(dbChain([marketRow('coingecko')])) // getMarketSnapshot | ||
| .mockReturnValueOnce(dbChain([RISK_LIMITS_ROW])) // getOrCreateRiskLimits |
| it('leaves a marketable resting limit order ACKNOWLEDGED (not filled) when the market is CoinGecko-fallback-sourced', async () => { | ||
| const restingOrder = { id: 'order-2', userId: 'user-a', asset: 'BTC', side: 'LONG', orderType: 'LIMIT', limitPrice: '100', status: 'ACKNOWLEDGED' }; | ||
| selectMock |
Summary
Fast-follow to PR #59 (DATA-RECOVERY-001, issue #35), addressing the two Medium (non-blocking) findings from its independent review:
checkTrustworthySource's actual call sites inpaperEngine.ts(processNewOrder/submitOrder,sweepLimitOrders) had zero automated regression coverage -- only the standalone risk-check function andevaluateTrade's dispatch of it were unit-tested. Added 4 tests topaperEngine.test.ts: a CoinGecko-fallback new order getsREJECTED, a Hyperliquid-sourced control stillFILLEDs, a CoinGecko-fallback marketable resting limit order staysACKNOWLEDGED, and a Hyperliquid-sourced control still fills viasweepLimitOrders.marketUpdatebroadcast loop inserver.tsalways hardcodedsource: 'hyperliquid', even though thechange24h/volumefields merged into that same payload come from the last REST ingestion cycle'sgetLastKnownMarketMeta, which could itself be CoinGecko-sourced in a narrow window (WS healthy, REST degraded). Now reportsmeta.source-- the real provenance of those fields -- instead. Documented indocs/architecture/market-data.md.Neither finding affected risk-gating correctness (the reviewer independently confirmed the gate itself works via direct DB manipulation) -- this is test-coverage and UI/labeling-accuracy hardening.
Verification
npm run buildclean.git diff --statvsgit diff -w --stat-- identical).Test plan
meta.sourceis the correct fix for LA-QG-002