Skip to content

fix(news): isolate the backtest signals projection#122

Merged
FlyM1ss merged 3 commits into
mainfrom
fix/backtest-signals-drift-isolation
Jul 15, 2026
Merged

fix(news): isolate the backtest signals projection#122
FlyM1ss merged 3 commits into
mainfrom
fix/backtest-signals-drift-isolation

Conversation

@FlyM1ss

@FlyM1ss FlyM1ss commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

get_news_sentiment projected via a dict comprehension, so one off-spec ticker raised out of the whole projection and load_news_sentiment's fail-closed except emptied the sentiment slot for every ticker that step — a producer typo on MSFT silently deleting NVDA's news. The panel has dropped-and-carried-on since _representative_feed; this is the projection that never learned it.

Per-entry drop + the existing _alarm_if_all_dropped. Three symptoms, one cause — the adapter left contract-break reporting to its caller:

  • Isolation. Blast radius drops from "every ticker" to "the broken one". Safe because the dict is already partial by nature (only tickers with signals appear), so a dropped ticker is the shape callers already handle.
  • Layering. backtest_backend's except no longer has to explain _project_entry's KeyError from another module. That hint is now removed — it would go stale the moment the adapter's internals move, which is what this PR does.
  • The second caller. get_news_sentiment has one production caller today. The fail-loud property fix(backtest): log news-sentiment failures #116 added lives in the caller's try/except, so caller Jifeng #2 (live paper-trading — anticipated in finsearch-news-signals-panel-design.md:115) silently regresses unless it remembers to re-implement it. Now there is no exception to forget to catch.

Isolation without the alarm would be a regression, not a fix. A rename makes every entry unreadable, so log-and-drop alone yields an empty slot plus a few warnings — indistinguishable from a quiet news day. That is how the 2026-07-14 outage ran for hours. Per _alarm_if_all_dropped's docstring, the alarm is what separates "one story is off-spec" from "the contract moved".

The alarm's raw arg is the post-filter set, never the wire's signals block. A signal for a ticker outside the universe is filtered by design, not dropped by drift; feeding the raw block would fire on every narrow-universe run and train the reader to ignore the one alarm that matters. test_universe_filter_alone_is_not_drift pins it.

_project_entry stays the single source of which fields are required — no duplicated key list, and KeyError's own text names the missing key, so the log reads missing 'sentiment_score' rather than a bare "malformed".

Escalation is log-only, deliberately. The panel escalates drift to a degraded badge; the backtest has no equivalent channel. RunManifest.news_sentiment_source is the candidate but is a provenance field that is unwired today (always None) — filing a follow-up rather than half-wiring it into a status flag here.

Mutation-tested (all three new guards fail when reverted; the alarm-deletion mutation was re-run after the first attempt produced a vacuous syntax error rather than a real failure). Suite green, delta is exactly +3 and nothing else moves (main 990 -> 993 passed, 2 skipped). Absolute counts vary with optional deps — discord installed or not swings the total, which is where an earlier 980/4 reading came from.

Review fix (ca36d12, docs only). _alarm_if_all_dropped's docstring claimed "the caller escalates a True to degraded" — true for the panel's call sites, false for the one this PR adds, which discards the return by design. Same hazard this PR removes from backtest_backend.py, one function over; #123 already quotes the line as if it were universal. Also NVDA-for-AAPL (AAPL is the one watchlist ticker the fixture gives no signals to) and the test docstring no longer re-narrates the history verbatim.

Re #120: independent (get_news_sentiment vs get_latest_panel_payload), and test-merged clean. Both widen _alarm_if_all_dropped to Union[list, dict]; this makes that change byte-identical to #120's so it merges either order. #120's "applied to ALL THREE projections" is left alone — it enumerates the panel's projections and stays accurate.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Pbs5wnQZhTi9CQ3pb14vDo

get_news_sentiment projected via a dict comprehension, so one off-spec
ticker raised out of the whole projection and load_news_sentiment's
fail-closed except emptied the sentiment slot for every ticker that step.
The panel has dropped-and-carried-on since _representative_feed; this is
the projection that never learned it.

Per-entry drop + the existing _alarm_if_all_dropped, so the adapter
reports its own contract breaks. That also removes the reason
backtest_backend's except had to explain _project_entry's KeyError from
another module, and means a second caller (live paper-trading, per the
panel design spec) inherits the reporting instead of re-implementing it.

The alarm's raw arg is the post-filter set, not the wire's signals block:
a signal outside the universe is filtered by design, not dropped by
drift. Mutation-tested: all three new guards fail when reverted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pbs5wnQZhTi9CQ3pb14vDo
@vercel

vercel Bot commented Jul 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agentic-trading-lab Ready Ready Preview, Comment Jul 15, 2026 3:52pm

Review of #122. _alarm_if_all_dropped's docstring claimed "the caller
escalates a True to `degraded`" — true for the panel's call sites, false
for the backtest one this PR adds, which discards the return by design
(log-only pending #123). It is the same hazard the PR removes from
backtest_backend.py, one function over: a comment that narrates another
layer's behavior goes stale the moment that layer moves. #123 already
quotes the line as if it were universal, so the drift had started.

Also: the isolation docstring cited AAPL, the one watchlist ticker the
fixture gives no signals to, where the test and the incident both mean
NVDA; and the test docstring re-narrated the history verbatim from
_project_step_signals, so it now pins what the test buys and points at
the single home for the why.

Docs only — no behavior change. Suite 993 passed / 2 skipped, and all
three guards still fail under their mutations.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@FlyM1ss

FlyM1ss commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Reviewed. One real finding, docs-only, fixed in ca36d12.

_alarm_if_all_dropped's docstring promised an escalation its new caller doesn't make — "the caller escalates a True to degraded" holds for the panel's call sites, not the backtest one added here, which discards the return by design (log-only pending #123). It's the hazard this PR removes from backtest_backend.py, one function over: a comment narrating another layer's behavior goes stale the moment that layer moves. #123 quoting the line as if it were universal is that drift already starting. The docstring now says what a True is worth is the caller's to decide, and names the backtest gap.

Plus two nits: NVDA-for-AAPL in the isolation docstring (AAPL is the one watchlist ticker the fixture gives no signals to — the one that couldn't be deleted this way), and the test docstring no longer re-narrates the history verbatim from _project_step_signals.

Verified rather than assumed, on head c95b7ae:

  • Mutations — reverting isolation → comprehension, deleting the alarm call, and swapping considered → the raw signals block each turn exactly the intended test red, and only that one. Re-run after the docstring edits.
  • fix(news): guard the panel signals projection #120git merge-tree clean both before and after ca36d12; the identical signature widening collapses.
  • Suite — 993 passed / 2 skipped; delta vs main is exactly +3.
  • The alarm actually lands. These loggers inherit root's WARNING (uvicorn's LOGGING_CONFIG has no root key), so error() escapes to stderr via lastResort. Worth stating because it's one level of margin — the same line as logger.info() would have been silently dead in prod.

Design call looks right: isolation without the alarm would have converted a loud failure into a quiet one, which is the 2026-07-14 shape. No new security surface — entries are dropped, never coerced, so the LLM context can only get smaller, never malformed.

load_news_sentiment's docstring spelled out the adapter's drop-and-alarm
mechanism in the sentence before warning that a comment here "that explains
the adapter's internals goes stale the moment they move" — the rule and its
violation two lines apart.

Swap the mechanism detail for a pointer, the idiom the file already uses
elsewhere (_project_step_signals: "see the caller's _alarm_if_all_dropped").
The rationale now lives once, in get_news_sentiment, where it can't drift out
of sync with the code it describes.

Docstring-only; no behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@FlyM1ss

FlyM1ss commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

/simplify pass — 4 review angles (reuse, simplification, efficiency, altitude). One fix applied, rest skipped as scoped-out or rejected on merits.

Fixed (516f05f, docstring-only). load_news_sentiment's docstring spelled out the adapter's drop-and-alarm mechanism in the sentence before warning that a comment here "goes stale the moment they move" — the rule and its violation two lines apart, and the same species of claim ca36d12 removed one layer down. Now points (see get_news_sentiment) per the file's existing idiom, so the rationale lives once.

Skipped — scope. _project_step_signals is the 4th hand-written loop+isolate+warn projector; reuse and altitude flagged it independently and both said don't fix here. The sharp version: the _alarm_if_all_dropped call is a separately-rememberable step, not forced by the primitive — which is how #119 happened. Fixing it rewrites three functions this PR never touches. Worth a pass if a 5th caller lands.

Rejected on merits. Unifying _project_step_signals with #120's _project_panel_signals: one is a pass-through filter, the other a transform. The panel can pre-check keys cheaply because it converts nothing; the backtest can't, since float(sig["published"]) only fails during the transform. Forced abstraction.

Confirmed correct, not changed. The escalation asymmetry (3 callers OR the bool, this one discards it) — #123 is real and states why deferring is right. Altitude added the clincher: api/v2/runs.py:367 writes the manifest once at run creation, before the field could be mutated per-step, so wiring it is genuinely #123-sized. Efficiency: no findings — the extra dict pass is ~microseconds/step against a cached fetch, ~0.1s across a multi-year backtest.

Suite 993 passed / 2 skipped, unchanged from this PR's own baseline.

Follow-up (docs, not touched here). This PR makes stale: finsearch-news-items.md:96 ("Drift escalates status to degraded" stated universally — now false for the backtest call site), :52 ("a rename of a field ATL doesn't read is invisible here" — false after #120 and this PR), and the Traceability table (103-114, no row for the new guard). Also finsearch-news-sentiment.md:141-153's failure-mode table has no row for the drop/ERROR behavior this PR completes.

@FlyM1ss FlyM1ss merged commit f8fba0a into main Jul 15, 2026
5 checks passed
@FlyM1ss FlyM1ss deleted the fix/backtest-signals-drift-isolation branch July 16, 2026 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant