feat: plumb tool-reported researchability through to the strategy layer - #1035
feat: plumb tool-reported researchability through to the strategy layer#1035jmoreira-valory wants to merge 4 commits into
Conversation
|
Review verdict: DO NOT MERGE as a gate — this implements the exact design the market-aware study falsified, and the gate's exit path has a policy side effect. (Posted as a comment because GitHub blocks request-changes on one's own PR.) 1. The hard researchability gate is empirically contraindicated (blocking)The A/B study that shipped
The validated forward candidate is continuous stake shrinkage inside the strategy (kelly_shrink: 2. Gate-fire aliases with tool failure in the selection policy (bug)The gate exits via 3. Nits
Suggested resolutionRepurpose the PR to what the study actually calls for: pass the raw |
Reworked from the original gate design per the market-aware study evidence (mech-predict #450): hard researchability gates were pre-registered, tested, and failed out-of-sample (the discarded bets were still profitable, and the tool over-reports the field), so the trader keeps its decision path byte-identical and the signal becomes ADVISORY plumbing instead: - PredictionResponse gains an optional `researchability` field: absent, boolean, non-numeric, or out-of-range values all degrade to None, so responses from every existing tool parse exactly as before. - decision_receive logs the signal when present and forwards it to get_bet_amount, which passes it into the strategy kwargs next to `confidence` -- shipped strategies ignore it; a future sizing strategy (kelly_shrink: continuous stake shrinkage toward the price, the study's forward-test candidate) can read it with no skill change. - No new config, no gate, no new decision path; the tool-selection policy update is untouched. Tests: parsing matrix (float kept / absent / bool / out-of-range -> None) and a parametrized forwarding test (0.35 and None) verified by mutation (dropping the kwarg fails it). 254 tests green; lint sextet green; packages lock clean with third-party untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7590583 to
36330fc
Compare
|
Reworked per the review above: the branch is reset onto main and force-pushed as |
…ixture The optional field now round-trips through the stored bets (deliberate: the signal is recorded), so the serialize_bets golden string gains "researchability": null. Full market_manager+decision_maker suites: 2083 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
OjusWiZard
left a comment
There was a problem hiding this comment.
Multi-perspective review (correctness, tests, silent failures, type design, comments, simplification).
One blocking issue, inline on bets.py:116: adding a field to PredictionResponse breaks BetsDecoder.hook's exact-key-set type discrimination, so every bets store written before this deploy decodes prediction_response as a plain dict. It fails silently at read time and crashes later in rebet_allowed for any market with an existing position. Reproduced, and a fix is prototyped and verified in that comment.
The rest are non-blocking: a latent TypeError in the reworked validator, an unobservable tool-misbehaviour path, and four comments/docstrings that restate what the PR description and history already record.
Verified alongside this: 254 tests pass, check-hash clean, and both shipped strategies take **kwargs with .get() so the extra kwarg is harmless. The parsing matrix itself is correct — bool, NaN, out-of-range, non-numeric and absent all degrade to None as designed. The decision path really is unchanged; the problem is purely the decoder that was only ever safe for Bet.
Per review discussion: the mech delivery already records the signal permanently (IPFS + subgraph), so the trader stores nothing. The value now travels _get_decision -> behaviour attribute -> strategy kwargs: PredictionResponse and the stored-bets serialization are untouched (bets.py and its golden fixtures revert to main), the log line is gone, and the only remaining purpose is the one that cannot be recovered after the fact: a sizing strategy (kelly_shrink) reading the signal at bet time. Coercion (bool / out-of-range / non-numeric -> None) is a module helper with parametrized tests; stale values reset per response. Both new tests mutation-verified. 2080 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Slimmed further per discussion: local recording removed ( |
- Present-but-rejected researchability values now log a warning (distinguishing 'tool never implemented the field' from 'implemented and broken'); absent stays quiet. Asserted in the capture matrix and mutation-verified (removing the warning fails 6 cases). - bool-subclasses-int note added to the coercion guard; design-rationale comments in base.py and the forwarding test docstring trimmed per the comment bar. - The two blocking BetsDecoder findings and the validator-loop trap were resolved by the earlier scope reduction (629a5a0): bets.py and its serialization are byte-identical to main again. 2080 tests green; lint sextet green; lock clean, third-party untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All 8 review threads addressed and resolved (details in-thread). Five targeted the pre-slim-down scope and were resolved by the |
feat: plumb tool-reported researchability through to the strategy layer
Summary
The mech's optional
researchabilitysignal (0-1, "can research even answerthis question?") travels to the betting strategy and nowhere else:
_get_decisioncoerces the raw response's optionalresearchability(absent / boolean / non-numeric / out-of-range all degrade to
None, and astale value can never leak across markets) onto a behaviour attribute. A
value that is present but rejected logs a warning, so "tool never
implemented the field" and "implemented and broken" are distinguishable;
an absent key stays quiet.
get_bet_amountforwards it into the strategyrun(**kwargs)next toconfidence(already plumbed, unread). Shipped strategies ignore it; afuture sizing strategy (kelly_shrink -- continuous stake shrinkage toward
the price, the market-aware study's forward-test candidate) can read it
with zero further trader changes.
Deliberately absent, per the study evidence (mech-predict #450) and review
discussion below:
failed out-of-sample (the discarded bets were still profitable; the tool
over-reports the field). The "skip hopeless markets" effect already emerges
from the tool's price-anchored probability meeting the existing min-edge
rule.
PredictionResponseand the stored-betsserialization are untouched -- every delivery already records the signal
permanently on IPFS/subgraph, so offline analysis needs nothing from the
trader. The only thing that cannot be recovered after the fact is the
signal at bet-sizing time -- which is exactly and only what this PR plumbs.
untouched.
Request side
Unchanged: the trader already attaches
request_context(market price, closetime, resolution rules, ...) to every mech request via
sampled_bet.to_request_context().Tests
_get_decisioncapture matrix (0.35 kept; absent / bool /out-of-range / non-numeric -> None; stale-value reset pinned; rejected
values warn, absent stays quiet).
fails a test. 2080 tests green; isort/black/pylint/mypy/flake8/darglint
green;
autonomy packages lockclean, third-party CIDs untouched.🤖 Generated with Claude Code