Consolidate broker CI and audit fixes - #1122
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds opt-in Alpaca paper-account CI and live broker strategy tests, improves Alpaca multi-leg order payloads, defers Tradovate stream startup until authentication, validates native data requests earlier, updates dependencies, and refreshes related documentation. ChangesLive broker CI and test coverage
Broker and data runtime changes
Packaging and deployment configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant Pytest
participant Alpaca
CI->>Pytest: run live broker strategy tests
Pytest->>Alpaca: authenticate and submit paper order
Alpaca-->>Pytest: return order status
Pytest->>Alpaca: poll and cancel order
Alpaca-->>Pytest: return terminal cancellation status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/cicd.yaml (1)
285-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConcurrency group name has misleading
-unit-testssuffix.The
broker-live-strategy-testsjob's concurrency group isbroker-live-strategy-tests-${{ github.repository }}-unit-tests. The-unit-testssuffix is misleading since this is a live broker test job, not a unit-test job. This appears to be a copy-paste artifact. It's functionally harmless but could cause confusion when debugging concurrency issues.🤖 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 285, Rename the concurrency group for the broker-live-strategy-tests job to remove the misleading “-unit-tests” suffix, preserving the existing broker-live-strategy-tests and repository identifiers.tests/test_alpaca_live_broker_apitest.py (1)
15-35: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePass
start_orders_thread=Falsein_require_alpaca()
connect_stream=Falseis enough for this path, andBroker.cleanup_streams()already stops and joins the orders thread if it starts. Matchingtests/test_broker_live_strategy_run_apitest.pykeeps the fixture lighter and avoids an unused background thread.🤖 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 `@tests/test_alpaca_live_broker_apitest.py` around lines 15 - 35, Update the Alpaca constructor call in _require_alpaca() to pass start_orders_thread=False alongside connect_stream=False, matching the configuration used by tests/test_broker_live_strategy_run_apitest.py and avoiding an unnecessary background orders thread.
🤖 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.
Inline comments:
In @.github/workflows/alpaca-live-broker.yml:
- Around line 63-65: Use a shared concurrency group keyed only by
github.repository for alpaca-live-broker.yml and the broker-live-strategy-tests
job in cicd.yaml, replacing their independent group names so all paper-account
tests serialize across workflows while preserving cancel-in-progress: false.
In @.github/workflows/cicd.yaml:
- Line 58: Set persist-credentials: false on every actions/checkout@v4 step in
the workflow, covering the lint, unit-tests, backtest-tests, and
broker-live-strategy-tests jobs.
- Around line 352-359: Replace direct GitHub expression interpolation in the
bash conditions within the broker-live-strategy-tests result check with
environment variables. Define env vars for github.event_name,
github.event.pull_request.head.repo.full_name, and github.actor, then reference
those quoted variables in the conditional logic while preserving the existing
skip and failure behavior.
In `@tests/test_broker_live_strategy_run_apitest.py`:
- Around line 248-255: Update the finally-block safety-net around
strategy.submitted_order to invoke _cancel_order_until_terminal only when the
order is not already terminal, and preserve any existing strategy.cancel_error
from _cancel_submitted_order. Remove the unconditional assignment of
cancel_error = None, ensuring successful cleanup does not mask prior
cancellation failures; use the existing terminal-status representation and
strategy fields to perform the check.
---
Nitpick comments:
In @.github/workflows/cicd.yaml:
- Line 285: Rename the concurrency group for the broker-live-strategy-tests job
to remove the misleading “-unit-tests” suffix, preserving the existing
broker-live-strategy-tests and repository identifiers.
In `@tests/test_alpaca_live_broker_apitest.py`:
- Around line 15-35: Update the Alpaca constructor call in _require_alpaca() to
pass start_orders_thread=False alongside connect_stream=False, matching the
configuration used by tests/test_broker_live_strategy_run_apitest.py and
avoiding an unnecessary background orders thread.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81f35670-482e-468b-ad16-dc71aff01b3a
📒 Files selected for processing (21)
.github/workflows/alpaca-live-broker.yml.github/workflows/cicd.yamldocs/ALPACA_LIVE_BROKER_CI.mddocs/SMART_LIMIT_LIVE_TESTING.mddocsrc/deployment.rstdocsrc/environment_variables.rstlumibot/brokers/alpaca.pylumibot/brokers/tradovate.pylumibot/entities/data.pyrequirements.txtsetup.cfgsetup.pytests/conftest.pytests/test_agent_alpaca_news_live_apitest.pytests/test_alpaca_live_broker_apitest.pytests/test_alpaca_multileg_fix.pytests/test_broker_live_strategy_run_apitest.pytests/test_broker_live_strategy_run_safety.pytests/test_data_entity.pytests/test_ibkr_futures_daily_series.pytests/test_tradovate.py
💤 Files with no reviewable changes (2)
- requirements.txt
- setup.py
| concurrency: | ||
| group: alpaca-live-broker-${{ github.repository }} | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Cross-workflow paper-account concurrency risk.
Both alpaca-live-broker.yml (concurrency group alpaca-live-broker-*) and cicd.yaml's broker-live-strategy-tests job (concurrency group broker-live-strategy-tests-*-unit-tests) can trigger on PRs to dev/main. They use independent concurrency groups, so both can run simultaneously against the same Alpaca paper account, potentially interfering with each other's order lifecycle tests.
Consider a shared concurrency group (e.g., keyed on github.repository alone) across both workflows to serialize all paper-account access.
Also applies to: 284-286
🤖 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/alpaca-live-broker.yml around lines 63 - 65, Use a shared
concurrency group keyed only by github.repository for alpaca-live-broker.yml and
the broker-live-strategy-tests job in cicd.yaml, replacing their independent
group names so all paper-account tests serialize across workflows while
preserving cancel-in-progress: false.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lumibot/entities/data.py (1)
1259-1442: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftKeep the native request guards aligned with
check_data
_validate_native_bars_requeststill duplicates the end-boundary and insufficient-history checks fromcheck_data. The intraday stale-bar check is already shared, so the remaining risk is divergence in date-range handling between the two paths. Consider extracting the shared boundary logic or adding a parity test that exercises both entry points.🤖 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/entities/data.py` around lines 1259 - 1442, Keep _validate_native_bars_request aligned with check_data by extracting the shared end-boundary and insufficient-history validation into reusable helpers, then invoke those helpers from both entry points; alternatively, add parity tests covering before-start, after-end, and insufficient-history requests to ensure both paths produce identical behavior and diagnostics.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/entities/data.py`:
- Around line 1259-1442: Keep _validate_native_bars_request aligned with
check_data by extracting the shared end-boundary and insufficient-history
validation into reusable helpers, then invoke those helpers from both entry
points; alternatively, add parity tests covering before-start, after-end, and
insufficient-history requests to ensure both paths produce identical behavior
and diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3388f7c0-c07b-4c1c-8297-e251a445845d
📒 Files selected for processing (2)
lumibot/entities/data.pytests/test_data_entity.py
1 participant
Summary
Why
Three related broker changes were split across stale branches. Audit found missing fork and Dependabot secret guards, cancellation cleanup gaps, stale-data validation gaps, and unsupported acceptance relaxations.
Impact
Live broker jobs remain credential-gated, skip untrusted pull requests, serialize shared paper-account access, and retry cancellation to terminal state. Standard unit and backtest shards remain deterministic.
Excluded unsupported changes: backdoor_smartlimit tolerance 700, runtime gates 900 and 1020, and LEAPS queue allowance 5. #1107 and #1108 are not included.
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests / CI