Add explainable Signal strength scoring model (SIGNAL-SCORE-001) - #60
Conversation
Issue #37: technical-analysis.ts computes real indicators (EMA/MACD/RSI/ ADX/Fisher/Keltner/ATR) but had no unified, explainable 0-100 score composing them -- ruleAlignmentScore is a raw confirmation count, not a documented, versioned model. - signals/signalScore.ts: pure, DB-free computeSignalScore(), mirroring analytics/metrics.ts's testability discipline. Six weighted components (trend agreement 25, momentum agreement 20, trend-strength confirmation 15, volatility suitability 15, data freshness 15, indicator availability 10 -- documented rationale in docs/product/signal-strength.md), reusing technical-analysis.ts's own EMA-separation and ADX thresholds as normalization references rather than inventing new arbitrary constants. - Conflict handling: trend/momentum disagreement (or an undeterminable trend) forces direction to NEUTRAL and the score to exactly 0, mirroring evaluateSignal()'s own choice to fire no signal at all in that case. - Invalidation handling: RSI/Fisher pointing opposite the thesis, or price already breaking the Keltner band against it, caps the score at 15 without flipping direction -- the call hasn't reversed, only weakened. - schemas/signalScore.ts: versioned Zod contract (SCORE_MODEL_VERSION), mirrored on the client (no shared package exists between them, matching this repo's existing convention). - Wired into technical-analysis.ts's generateSignals(); persisted in a new nullable signals.signal_score jsonb column -- nullable because signals generated before this shipped never had a score computed, and backfilling a synthetic value would fabricate evidence that doesn't exist. - client: SignalStrength.tsx replaces the old raw "Rule Alignment" bar with the new Signal strength score, a Strong/Moderate/Weak/None label (never color alone, per the mission's accessibility requirement), and a native <details> disclosure for the component breakdown -- no new UI dependency. - Never labeled "confidence"/"probability"/"expected return" anywhere, matching the ruleAlignmentScore precedent (GH F-5). Verified against real market data end-to-end, not just unit tests: ran the real server against live Hyperliquid data, confirmed generated signals persist a correctly-computed score (including a real invalidation-condition case -- RSI oversold capping a SHORT signal at 15/100), and confirmed the API round-trips the jsonb value intact. 8 new unit tests cover every fixture category from the test plan: full agreement (100), full disagreement (exactly 0), undeterminable trend (exactly 0), missing-indicator handling, stale-data handling, invalidation-condition capping, version stability, and the always-null candleInterval. Full suite: server 199/199, client 33/33. Both builds clean. Closes #37.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8a7a809d2
ℹ️ 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".
…rework) Independent review of PR #60 found a confirmed High-severity regression: signals.signal_score is a nullable column by design (any signal generated before this feature shipped never had a score computed, and the shared dev DB alone had 6,728 of 6,731 rows genuinely null), but the client's Signal.signalScore type declared it non-nullable and SignalStrength dereferenced it directly -- rendering any un-scored signal threw "Cannot read properties of null (reading 'totalScore')", which the app's top-level ErrorBoundary caught by blanking the *entire* Signals page, not degrading a single card. Reviewer reproduced this with a throwaway test rendering <SignalStrength score={null} />. - types.ts: Signal.signalScore is now SignalScore | null, matching the real DB column and API response shape. - SignalStrength.tsx: renders an explicit "Not available" state for a null score (signal generated before this feature existed) instead of crashing -- consistent with this PR's own "don't fabricate evidence that doesn't exist" principle already applied to candleInterval and the nullable column itself. - SignalStrength.test.tsx (new): regression test for the null case, plus a real-score render test. Also addresses the review's two non-blocking Low findings: - signalScore.ts: volatilitySuitability now guards keltnerUpper/ keltnerLower against zero, matching the existing ema200 !== 0 guard (the outer clamp already prevented a NaN/Infinity leak, but the reviewer correctly noted the inconsistency). - signalScore.test.ts: two more fixtures -- momentum missing while trend is still determinable (direction preserved, score reduced), and every indicator null simultaneously (NEUTRAL, exactly 0, no crash). Full suite: server 201/201, client 35/35. Both builds clean, client lint/format clean.
Rework applied (independent review: FAIL -> addressed)Independent review found a confirmed High regression: `signals.signal_score` is a nullable column by design (this DB alone had 6,728 of 6,731 rows genuinely null), but the client's `Signal.signalScore` type was non-nullable and `SignalStrength` dereferenced it directly -- any un-scored signal crashed with `Cannot read properties of null (reading 'totalScore')`, taking down the entire Signals page via the app's ErrorBoundary. Reviewer reproduced this directly with a throwaway test. Fixed:
Also addressed the two non-blocking Low findings:
Full suite: server 201/201, client 35/35. Both builds clean, client lint/format clean. |
Independent re-review: PASSThe confirmed High-severity crash (un-scored signal -> Both Low findings also confirmed genuinely addressed: the Full suite: server 201/201, client 35/35. Both builds clean. Ready for merge at your convenience. |
Summary
Closes #37 (SIGNAL-SCORE-001).
technical-analysis.tsalready computes real indicators (EMA/MACD/RSI/ADX/Fisher/Keltner/ATR), but there was no unified, explainable 0-100 "Signal strength" score composing them into a documented, versioned model --ruleAlignmentScoreis a raw confirmation count, not that.signals/signalScore.ts-- pure, DB-freecomputeSignalScore(), same testability discipline asanalytics/metrics.ts. Six weighted components (trend agreement 25, momentum agreement 20, trend-strength confirmation 15, volatility suitability 15, data freshness 15, indicator availability 10). Full weighting rationale indocs/product/signal-strength.md. Reusestechnical-analysis.ts's own EMA-separation and ADX thresholds as normalization references rather than inventing new arbitrary constants.NEUTRALand the score to exactly 0 -- mirrorsevaluateSignal()'s own choice to fire no signal at all in that case.schemas/signalScore.ts-- versioned Zod contract (SCORE_MODEL_VERSION), mirrored on the client (no shared package exists between client/server in this repo, matching its existing convention).technical-analysis.ts'sgenerateSignals(); persisted in a new nullablesignals.signal_scorejsonb column -- nullable because signals generated before this shipped never had a score computed, and backfilling a synthetic value for them would fabricate evidence that doesn't exist. Verified this decision was necessary: the shared dev DB had 2,431 pre-existing signal rows, and aNOT NULLmigration would have been a real (blocked) data-loss operation.SignalStrength.tsxreplaces the old raw "Rule Alignment" bar with the new Signal strength score, a Strong/Moderate/Weak/None label (never color alone, per the mission's accessibility requirement), and a native<details>disclosure for the component breakdown -- no new UI dependency added.ruleAlignmentScoreprecedent (GH audit finding F-5).Verification
candleInterval.npm run buildclean. Client lint/format clean.(2*25+0*20+100*15+50*15+100*15+100*10)/100 = 48, matching exactly) -- and confirmed the API round-trips the jsonb value intact to the client-facing shape.git diff --statvsgit diff -w --stat-- identical).Test plan