Gate changes with Alpaca and Tradier paper tests - #1127
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
📝 WalkthroughWalkthroughAdds a paper-broker integration gate for Alpaca and Tradier, wires it into pull-request CI, documents credentials and execution, updates timezone-aware market-open handling and lazy imports, and refreshes one backtest baseline. ChangesPaper broker CI gate
Runtime compatibility updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant LiveBrokerGate
participant AlpacaTradier
participant Trader
GitHubActions->>LiveBrokerGate: run broker gate
LiveBrokerGate->>AlpacaTradier: read accounts, positions, and orders
LiveBrokerGate->>AlpacaTradier: submit and cancel paper order
LiveBrokerGate->>Trader: run one strategy iteration
Trader->>AlpacaTradier: submit strategy order
AlpacaTradier-->>LiveBrokerGate: report cancelled status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.6)tests/test_live_broker_gate.py************* Module pylintrc ... [truncated 16144 characters] ... te", Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/cicd.yaml (1)
57-57: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon the remaining checkout steps. Thelive-broker-gatecheckout already opts out, but thelint,unit-tests, andbacktest-testscheckouts persist theGITHUB_TOKENinto the workspace git config unnecessarily (none of these jobs push). zizmor flags this asartipacked.
.github/workflows/cicd.yaml#L57-L57: addwith: { persist-credentials: false }to thelintjob checkout..github/workflows/cicd.yaml#L102-L102: addwith: { persist-credentials: false }to theunit-testsjob checkout..github/workflows/cicd.yaml#L192-L192: addwith: { persist-credentials: false }to thebacktest-testsjob checkout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cicd.yaml at line 57, Update the checkout steps in the lint, unit-tests, and backtest-tests jobs at .github/workflows/cicd.yaml lines 57-57, 102-102, and 192-192 to set persist-credentials: false under each action's with configuration; apply the same change at all three sites.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/cicd.yaml:
- Line 57: Update the checkout steps in the lint, unit-tests, and backtest-tests
jobs at .github/workflows/cicd.yaml lines 57-57, 102-102, and 192-192 to set
persist-credentials: false under each action's with configuration; apply the
same change at all three sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b6d6120-91ed-424b-9352-6b1b5c6840b6
📒 Files selected for processing (2)
.github/workflows/cicd.yamltests/test_live_broker_gate.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lumibot/credentials.py (1)
461-475: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSync docs for the new IBKR REST env vars.
RUNNING_ON_SERVER,IB_GATEWAY_PORT,IB_GATEWAY_INSTANCE_ID,IB_USE_PAPER_ACCOUNT,IBEAM_DOCKER_TAG,IB_AUTH_TIMEOUT,IB_AUTH_POLL_INTERVAL,IB_REQUEST_TIMEOUT, andIB_VERIFY_SSLare newly wired intoINTERACTIVE_BROKERS_REST_CONFIG. Please confirm public/engineering docs for Interactive Brokers REST setup mention these. Also noteRUNNING_ON_SERVERbreaks theIB_-prefix convention used by its siblings — worth double-checking that's intentional.As per coding guidelines, "If an environment variable is introduced or changed, make sure both engineering and public documentation stay synchronized with the new behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lumibot/credentials.py` around lines 461 - 475, Update the engineering and public Interactive Brokers REST setup documentation to describe all environment variables wired through INTERACTIVE_BROKERS_REST_CONFIG, including their purpose and defaults where applicable. Also verify whether RUNNING_ON_SERVER intentionally lacks the IB_ prefix; if not, align the configuration and documentation with the established naming convention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lumibot/credentials.py`:
- Around line 461-475: Update the engineering and public Interactive Brokers
REST setup documentation to describe all environment variables wired through
INTERACTIVE_BROKERS_REST_CONFIG, including their purpose and defaults where
applicable. Also verify whether RUNNING_ON_SERVER intentionally lacks the IB_
prefix; if not, align the configuration and documentation with the established
naming convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2a4c85d-0631-4319-801b-e372c66f642b
📒 Files selected for processing (6)
.github/workflows/cicd.yamllumibot/brokers/alpaca.pylumibot/credentials.pylumibot/data_sources/interactive_brokers_rest_data.pytests/backtest/acceptance_backtests_baselines.jsontests/test_alpaca.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/cicd.yaml
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Lint
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,md,rst,txt,yml,yaml,json,ini,env}
📄 CodeRabbit inference engine (CLAUDE.md)
Never fabricate, synthesize, forward-fill, interpolate, or default-fill missing market data in backtests; return empty / explicit absence instead, and remove any code that returns fake bars as real data.
Files:
lumibot/credentials.pylumibot/brokers/alpaca.pytests/test_alpaca.pytests/backtest/acceptance_backtests_baselines.jsonlumibot/data_sources/interactive_brokers_rest_data.py
**/*.{py,md,rst}
📄 CodeRabbit inference engine (CLAUDE.md)
If an environment variable is introduced or changed, make sure both engineering and public documentation stay synchronized with the new behavior.
Files:
lumibot/credentials.pylumibot/brokers/alpaca.pytests/test_alpaca.pylumibot/data_sources/interactive_brokers_rest_data.py
**
⚙️ CodeRabbit configuration file
**: Review every pull request as if LumiBot is a public open-source trading framework.
Prioritize real bugs, security/privacy issues, user-facing regressions, and release risk.Always check changed lines for:
- hardcoded credentials, API keys, tokens, account emails, private URLs, private hostnames, local credential paths, or personal filesystem paths;
- accidental leakage of BotSpot, Lumiwealth, customer, broker, paid-vendor, CI, or maintainer-only operational details into public code/docs/tests;
- code that logs, prints, persists, screenshots, or commits secrets or customer/broker data;
- test fixtures or docs that look fake but could be copied into real usage as credentials or private endpoints;
- changes that weaken authentication, authorization, data-source safety, broker/order safety, or CI/release gates.
If a finding depends on repository context, explain the specific source file and invariant instead of giving generic advice.
Files:
lumibot/credentials.pylumibot/brokers/alpaca.pytests/test_alpaca.pytests/backtest/acceptance_backtests_baselines.jsonlumibot/data_sources/interactive_brokers_rest_data.py
lumibot/**
⚙️ CodeRabbit configuration file
lumibot/**: Review runtime/library changes for trading safety, broker/account isolation, data
correctness, secret handling, and public/private boundary violations. Flag hardcoded
BotSpot-specific behavior unless it is clearly provider-generic and appropriate for
open-source LumiBot.
Files:
lumibot/credentials.pylumibot/brokers/alpaca.pylumibot/data_sources/interactive_brokers_rest_data.py
tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
tests/**/*.py: Add unit tests for any new functionality
Ensure high level of test coverage using pytest with coverage reporting
Ensure all tests are well-documented and follow best practices
tests/**/*.py: Treat any test whose earliest commit date is before 2025-06-01 as LEGACY. For LEGACY tests, fix the code, not the test. Only change a LEGACY test when you can clearly justify that the old expectation was incorrect or behavior was intentionally changed for correctness, and document it in the test file.
Treat any test whose earliest commit date is before 2025-01-01 as FROZEN LEGACY (effectively 'do not change'). Only change a FROZEN LEGACY test in exceptional cases and include a clear write-up in the test and PR.
If you change any expected values or assertions in a test, add a short note near the change explaining why (what changed, and why the new expectation is correct). Prefer making the test more robust (less brittle) over updating magic numbers.
Files:
tests/test_alpaca.py
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
For tests under
tests/, older tests have higher authority than newer ones; when they conflict, fix the code first unless the newer test clearly represents new functionality.
Files:
tests/test_alpaca.pytests/backtest/acceptance_backtests_baselines.json
⚙️ CodeRabbit configuration file
tests/**: Tests are public. Flag hardcoded real-looking credentials, account emails, personal
paths, private endpoints, and fixtures that could accidentally exercise production or
paid-vendor resources without explicit opt-in.
Files:
tests/test_alpaca.pytests/backtest/acceptance_backtests_baselines.json
🔇 Additional comments (9)
tests/backtest/acceptance_backtests_baselines.json (1)
127-141: LGTM!lumibot/brokers/alpaca.py (1)
580-582: LGTM!tests/test_alpaca.py (2)
12-12: LGTM!Also applies to: 105-105
117-140: 🩺 Stability & AvailabilityNo issue: the calendar fallback uses
lumibot.brokers.broker.datetime, not the patched Alpaca module.
_is_market_open_from_initialized_calendar()is defined inlumibot.brokers.brokerand callsdatetime.now(timezone.utc), so theSimpleNamespace(datetime=FixedDatetime)patch onlumibot.brokers.alpaca.datetimedoes not trigger thetz is not Noneassert.> Likely an incorrect or invalid review comment.lumibot/data_sources/interactive_brokers_rest_data.py (3)
13-20: LGTM!
58-91: LGTM!
115-131: 🩺 Stability & AvailabilityNo module-level dependency here The repo doesn’t import or monkeypatch
InteractiveBrokersRESTData.IBeamGateway,ExternalIbkrGateway,DEFAULT_IBEAM_HOST_PORT, orDEFAULT_IBEAM_TAG, so moving them into__init__doesn’t break current callers.> Likely an incorrect or invalid review comment.lumibot/credentials.py (2)
65-78: LGTM!
1191-1195: 🔒 Security & PrivacyDrop this comment. The cleanup also removes
BROKERandDATA_SOURCE, so there’s no stale cached export left behind.> Likely an incorrect or invalid review comment.
…ker-gate-20260715
1 participant
Summary
Safety
Consolidation
This supersedes #1104. Its useful Alpaca strategy data and option-chain checks are folded into this PR. The duplicate workflow, duplicate deterministic agent/MCP runs, alternate news-credential plumbing, unrelated IBKR formatting, and reverted performance churn are intentionally omitted. Deterministic agent, MCP, permission, provider-key, and built-in news tests remain in the normal unit shards.
Validation
Summary by CodeRabbit
mainanddev, with concurrency-controlled live broker validation and stricter workflow success gating.BackdoorButterfly0DTESmartLimitbacktest baseline results.