Skip to content

feat(strategy): add MACD alignment requirement for buy signals#334

Merged
TheodorStorm merged 1 commit intodevelopfrom
fix/issue-331
Jan 22, 2026
Merged

feat(strategy): add MACD alignment requirement for buy signals#334
TheodorStorm merged 1 commit intodevelopfrom
fix/issue-331

Conversation

@claude
Copy link
Contributor

@claude claude bot commented Jan 10, 2026

Summary

  • Add configurable option to require MACD histogram to be above a threshold before allowing buy trades
  • Prevents buying during declining momentum conditions (falling knife scenarios)
  • Feature disabled by default (opt-in) with threshold of -0.5

Changes

  • config/settings.py: Add require_macd_alignment and macd_alignment_threshold config parameters
  • src/strategy/signal_scorer.py: Implement MACD alignment check in calculate_score() - when enabled and histogram < threshold, buy signals downgrade to hold
  • src/daemon/runner.py: Pass new settings to SignalScorer initialization
  • .env.example: Add documentation for new config parameters
  • tests/test_signal_scorer.py: Add comprehensive unit tests for MACD alignment logic
  • tests/test_trading_daemon.py: Add mock settings for new parameters

Context

From post-mortem analysis (2026-01-09):

  • All 4 losing buy trades had MACD histogram showing clear negative momentum (-37 to -85)
  • RSI and Bollinger indicated oversold/buy opportunity, but MACD said momentum still declining
  • Requiring MACD alignment would have blocked every losing trade in that session

Test plan

  • Unit tests for MACD alignment disabled by default (backward compatibility)
  • Unit tests for buy blocked when MACD histogram below threshold
  • Unit tests for buy allowed when MACD histogram above threshold
  • Unit tests for sell/hold signals not affected
  • Unit tests for configurable threshold
  • Unit tests for combined with other filters (HTF, trend)
  • Run full test suite: python3 -m pytest tests/ --ignore=tests/test_coinbase_integration_live.py -v

Closes #331

🤖 Generated with Claude Code

Add configurable option to require MACD histogram to be above a threshold
before allowing buy trades. This prevents buying during declining momentum
conditions (falling knife scenarios).

Changes:
- Add `require_macd_alignment` and `macd_alignment_threshold` config params
- Implement MACD alignment check in SignalScorer.calculate_score()
- When enabled and histogram < threshold, buy signals downgrade to hold
- Add comprehensive unit tests for MACD alignment logic
- Update .env.example with documentation

Post-mortem analysis showed all 4 losing trades had negative MACD histogram
(-37 to -85) while RSI/Bollinger indicated oversold. Requiring MACD alignment
would have blocked every losing trade in that session.

Feature is disabled by default (opt-in) with threshold of -0.5.

Closes #331

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link
Contributor Author

claude bot commented Jan 10, 2026

PR Review: feat(strategy): add MACD alignment requirement for buy signals

Overall Assessment: ✅ APPROVE

This is a well-implemented, targeted improvement to the trading strategy that addresses a documented post-mortem finding. The code is clean, well-tested, and follows the existing patterns in the codebase.


🟢 Strengths

1. Post-Mortem Driven Development

  • Feature directly addresses real losses from 2026-01-09 post-mortem analysis
  • All 4 losing trades had negative MACD histogram (-37 to -85)
  • This would have prevented every losing trade in that session

2. Conservative Default Configuration

  • Disabled by default (require_macd_alignment=false) - opt-in behavior
  • Default threshold of -0.5 allows slight momentum decline
  • Backward compatible - existing users unaffected

3. Well-Structured Implementation

  • Follows existing filter patterns (HTF, trend filter)
  • Clean placement after action determination, before confidence calculation
  • Proper metadata tracking (_macd_alignment_blocked)
  • Good structured logging for observability

4. Comprehensive Test Coverage

  • 12 test cases covering all edge cases:
    • Default disabled (backward compat)
    • Buy blocked with negative histogram
    • Buy allowed with positive histogram
    • Sell/hold signals unaffected
    • Configurable threshold
    • Missing MACD data handled
    • Combined with other filters

🟡 Minor Observations (Low Priority)

1. Metadata key naming convention

metadata["_macd_alignment_blocked"] = 1 if macd_alignment_blocked else 0

Other metadata uses underscored prefix consistently. This matches the pattern - looks good.

2. Conservative behavior on missing MACD data (Line 1237-1243)
The code blocks buys when MACD data is unavailable. This is the right choice for a financial system - fail-safe rather than fail-open. However, this could mask data issues if the MACD calculation is broken.

Suggestion (optional): Consider incrementing a counter/metric when MACD data is missing to catch any data pipeline issues early. Not blocking merge.

3. Test robustness
Some tests use conditional assertions like:

if result.score >= macd_alignment_scorer.threshold:
    assert result.action == "buy"

This is acceptable given the stochastic nature of the test data generation, but doesn't guarantee the condition is actually exercised. The seed-based approach (np.random.seed()) helps with reproducibility.


🔒 Security Considerations

No security concerns. The changes:

  • Only affect internal trading logic
  • Don't introduce new external inputs
  • Don't modify protected paths (safety systems, exchange clients)
  • Are fully configurable via existing config system

Performance Considerations

Minimal performance impact:

  • Single comparison operation (macd_histogram < threshold)
  • Only executed when feature is enabled AND action is "buy"
  • No additional API calls or database queries
  • MACD histogram is already calculated as part of indicator computation

Checklist

  • Code follows existing patterns in signal_scorer.py
  • Configuration added to settings.py with proper Field validation
  • .env.example updated with documentation
  • Test coverage is comprehensive
  • Feature is disabled by default (backward compatible)
  • No security vulnerabilities introduced
  • Conventional commit format used correctly
  • PR properly references issue (Add MACD alignment requirement for buy signals #331)

📝 Summary

This PR adds a valuable safety feature to prevent buying during declining momentum conditions. The implementation is clean, well-tested, and follows all codebase conventions. The opt-in design ensures no disruption to existing deployments.

Recommendation: Merge when CI passes.


Review by Claude Opus 4.5

@TheodorStorm TheodorStorm merged commit 189a130 into develop Jan 22, 2026
1 of 2 checks passed
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