Add portfolio rebalancing logic and update Alpaca orders to use notional values#749
Merged
forstmeier merged 5 commits intomasterfrom Feb 7, 2026
Merged
Conversation
…nal values This commit implements automatic portfolio rebalancing when stop-loss triggers create position count imbalances, and updates the Alpaca client to use dollar amounts (notional) instead of share quantities for orders. ## Changes ### Portfolio Rebalancing (risk_management.py) - Added automatic rebalancing logic in add_portfolio_performance_columns() - When longs hit stop-loss, system closes equal number of best-performing shorts - When shorts hit stop-loss, system closes equal number of best-performing longs - Rebalancing respects PDT-locked positions and existing closure decisions - Best performers selected by cumulative return to lock in profits - Enables existing create_optimal_portfolio() to maintain 10/10 position balance ### Alpaca Client Updates (alpaca_client.py) - Changed open_position() to use 'notional' parameter instead of 'qty' - Removed _get_current_price() method and associated quote fetching - Removed unused StockLatestQuoteRequest import - Simplifies order submission and leverages Alpaca's fractional share handling - Reduces API calls (no longer needs price quotes before orders) ### Testing - Added 4 comprehensive rebalancing tests in test_risk_management.py - Test coverage: 96% for risk_management.py - All 83 tests passing ## Root Cause & Fix Previously, when stop-loss triggers caused asymmetric position closures (e.g., 3 longs closed but 0 shorts), the portfolio would fail schema validation (requires exactly 10 long + 10 short positions). The system would struggle to find enough quality predictions to fill all 10 positions on the depleted side. Solution: When position closures are imbalanced, automatically close an equal number of positions from the opposite side. This: 1. Locks in profits from best performers 2. Frees capital symmetrically 3. Enables create_optimal_portfolio() to open 3 new longs + 3 new shorts 4. Maintains 10/10 balance and ±5% dollar balance requirements The Alpaca notional change simplifies order execution and ensures exact dollar allocations without rounding errors from quantity calculations. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAlpaca trade execution now uses notional (dollar_amount) instead of computing share quantity; risk management replaced complex portfolio-performance logic with a fixed 20-position (10 long / 10 short) selection and uniform allocation, requiring 20 available predictions; server and schema updated to accept prior portfolio tickers only. Changes
Sequence Diagram(s)sequenceDiagram
participant Strategy as "Strategy"
participant AlpacaClient as "AlpacaClient"
participant AlpacaAPI as "Alpaca API"
Strategy->>AlpacaClient: open_position(ticker, side, dollar_amount)
AlpacaClient->>AlpacaClient: validate dollar_amount > 0
AlpacaClient->>AlpacaAPI: submit OrderRequest(notional=dollar_amount, side)
AlpacaAPI-->>AlpacaClient: success / APIError
alt success
AlpacaClient->>Strategy: return order confirmation
else APIError
AlpacaClient->>Strategy: raise/map APIError (insufficient buying power, not shortable, etc.)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds automatic portfolio rebalancing to maintain position balance when stop-losses trigger asymmetrically, and simplifies Alpaca order execution by using dollar amounts instead of share quantities.
Changes:
- Implemented rebalancing logic that closes best-performing positions from the opposite side when stop-losses create imbalances
- Updated Alpaca client to use notional values for orders, eliminating price fetching and quantity calculations
- Added comprehensive test coverage for rebalancing scenarios
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| applications/portfoliomanager/src/portfoliomanager/risk_management.py | Added rebalancing logic to close opposite-side positions when stop-loss closures are imbalanced |
| applications/portfoliomanager/src/portfoliomanager/alpaca_client.py | Replaced quantity-based orders with notional orders and removed price fetching logic |
| applications/portfoliomanager/tests/test_risk_management.py | Added 4 test cases covering various rebalancing scenarios with PDT-locked positions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Greptile OverviewGreptile SummaryThis PR implements automatic portfolio rebalancing when stop-loss triggers create position imbalances, and simplifies Alpaca order submission by switching from quantity-based to notional (dollar-based) orders. Key Changes:
Critical Issue: Missing Test Coverage: Alpaca Client Changes: Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| applications/portfoliomanager/src/portfoliomanager/alpaca_client.py | Removed price fetching logic and switched to notional (dollar-based) orders - simplifies order submission and leverages Alpaca's fractional share handling |
| applications/portfoliomanager/src/portfoliomanager/risk_management.py | Added automatic portfolio rebalancing logic when stop-loss creates imbalanced closures - CRITICAL ISSUE: rebalancing overrides MAINTAIN_POSITION (take-profit) decisions, contradicting PR description |
| applications/portfoliomanager/tests/test_risk_management.py | Added 4 rebalancing tests covering basic scenarios - missing test coverage for MAINTAIN_POSITION interaction with rebalancing logic |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/portfoliomanager/src/portfoliomanager/risk_management.py
Line: 140:142
Comment:
**Missing returns in join.**
`returns = pl.DataFrame(position_returns)` will be an empty DataFrame with **no schema** when `prior_portfolio_predictions` has 0 rows, so the subsequent join on `["ticker", "timestamp"]` fails because those columns don’t exist. This breaks `add_portfolio_performance_columns()` for an empty portfolio (which can happen on a cold start / after full liquidation). Define an explicit empty schema for `returns` (at least `ticker`, `timestamp`, `cumulative_simple_return`) when `position_returns` is empty so the join always has the expected keys.
How can I resolve this? If you propose a fix, please make it concise. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/portfoliomanager/src/portfoliomanager/alpaca_client.py (1)
46-49:⚠️ Potential issue | 🟡 MinorRemove unused
StockHistoricalDataClientimport anddata_clientassignment.With
_get_current_priceremoved,self.data_clientis never referenced and theStockHistoricalDataClientimport (line 6) is no longer needed. Remove both the import and the initialization at lines 46-49.
🤖 Fix all issues with AI agents
In `@applications/portfoliomanager/src/portfoliomanager/risk_management.py`:
- Around line 221-228: The log currently records the requested
shorts_to_rebalance rather than the actual number closed; change the logger.info
call around best_shorts to include the actual count (use best_shorts.height) as
a separate field (e.g., actual_shorts_closed or shorts_closed_actual) and
replace or add it alongside shorts_to_rebalance so the entry reflects the real
number closed; update the logger.info invocation that references best_shorts,
closed_short_count and shorts_to_rebalance to emit best_shorts.height for
operators to see partial rebalances.
- Around line 230-235: The current when() uses ticker-only matching and can flip
the wrong side; update the predicate in the portfolio_with_actions.with_columns
calls (the one using best_shorts and the one using best_longs) to require both
pl.col("ticker").is_in(best_shorts["ticker"]) (or best_longs["ticker"]) AND
pl.col("side") == <short/long side constant/string> so you only close positions
for the matching side (e.g., combine .is_in(...) .and(pl.col("side") ==
PositionSide.SHORT.value) for best_shorts and similarly use
PositionSide.LONG.value for best_longs while keeping the same
.then(PositionAction.CLOSE_POSITION.value).
In `@applications/portfoliomanager/tests/test_risk_management.py`:
- Around line 805-822: The test comments around the closed shorts are
misleading: update the explanatory comments near the closed_shorts and
closed_short_tickers checks to state that SHORT1 and SHORT2 were closed by
hitting the upper stop-loss threshold (cumulative +20% and +25%) while only
SHORT3 was closed by the rebalancing logic, and correct the sentence that
currently calls SHORT2 the "most negative" performer to reflect that SHORT2 has
the most positive cumulative return; leave the assertions (closed_shorts and
closed_short_tickers) unchanged.
- Around line 754-757: Remove the unused noqa directives in the new tests: open
the test functions such as
test_add_portfolio_performance_columns_rebalancing_closes_shorts_when_longs_stopped
and the other listed tests (lines referenced around the same functions) and
delete the redundant "# noqa: E501" and "# noqa: PLR2004" comments (and any
other unused noqa tokens) so Ruff RUF100 no longer flags them; ensure you only
remove noqa comments that Ruff reports as unused and keep any remaining valid
noqa directives if they are actually suppressing enabled rules.
- Around line 951-1020: The test
test_add_portfolio_performance_columns_rebalancing_respects_pdt_locked_positions
does not trigger the rebalancing branch because SHORT2 is being closed by
stop-loss instead of being chosen by rebalancing; update the test data so
closed_long_count > closed_short_count (e.g., create two LONG positions that
both hit stop-loss and zero SHORTs that hit stop-loss) while keeping SHORT1 as
PDT_LOCKED and the other SHORTs as UNSPECIFIED with worse performance than
SHORT1, then call add_equity_bars_returns_and_realized_volatility_columns and
add_portfolio_performance_columns as before and assert that rebalancing closes
the eligible UNSPECIFIED SHORTs (e.g., SHORT2 and SHORT3) while SHORT1 remains
"PDT_LOCKED".
Collaborator
Author
|
Fixed empty portfolio join bug in risk_management.py:140-149. Added explicit schema definition when position_returns is empty to ensure join columns (ticker, timestamp, cumulative_simple_return) exist for cold start scenarios. |
Contributor
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/portfoliomanager/src/portfoliomanager/alpaca_client.py
Line: 79:87
Comment:
**Notional may reject shorts**
`open_position()` now always submits `OrderRequest(..., notional=dollar_amount, ...)` (alpaca_client.py:79-86). Alpaca generally supports `notional` for buys, but some APIs/brokerages reject notional sizing for `sell` (including short sells) and require `qty` instead. If `TradeSide` can be `SELL` for opening shorts, this change can turn valid short opens into hard API errors. Consider branching: use `notional` for buys and `qty` (or validate Alpaca’s support for notional on sells/short sells) for shorts.
How can I resolve this? If you propose a fix, please make it concise. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@applications/portfoliomanager/tests/test_risk_management.py`:
- Around line 754-825: The test
test_add_portfolio_performance_columns_rebalancing_closes_shorts_when_longs_stopped
doesn't exercise the selection priority because number of shorts equals number
to close; update the test setup (positions, predictions, and raw_equity_bars) to
include more eligible shorts than needed (e.g., add a fourth SHORT ticker with
appropriate predictions and equity bars) and adjust longs so fewer longs hit
stop-loss (e.g., only 2 LONGs cross stop-loss) so
add_portfolio_performance_columns must pick the top-performing shorts; then
change assertions to expect only the required number of closed_shorts (e.g., 2)
and assert that the specific best-performing short tickers were chosen while at
least one short remains open.
Collaborator
Author
|
Empty portfolio join bug was already fixed in commit bef64ea. Lines 143-149 in risk_management.py now define explicit schema with ticker, timestamp, and cumulative_simple_return columns when position_returns is empty, ensuring the join always has the expected keys for cold start scenarios. |
Collaborator
Author
|
Deferring investigation on notional shorts API concern until production monitoring confirms behavior. The existing error handling at alpaca_client.py:88 will surface API rejections in Sentry if notional sizing is unsupported for short positions. We should validate through monitoring before adding complexity. |
…N and improve test coverage Fixed critical bug where rebalancing logic was overriding positions marked MAINTAIN_POSITION (take-profit targets). The rebalancing filters at risk_management.py:224 and :262 now exclude MAINTAIN_POSITION positions from selection, ensuring only UNSPECIFIED positions are eligible for rebalancing closures. Root cause: Rebalancing filtered only CLOSE_POSITION and PDT_LOCKED actions, allowing MAINTAIN_POSITION positions to be incorrectly flipped to CLOSE_POSITION when balancing long/short closure counts. This contradicted the PR description stating the logic "respects existing closure decisions." Fix: Added filter condition to exclude MAINTAIN_POSITION from both best_shorts and best_longs selection logic. Also improved test coverage by updating test_add_portfolio_performance_columns_rebalancing_closes_shorts_when_longs_stopped to add a 4th short ticker and adjust long closures to properly exercise sort-based selection priority. Previous test had exact 3:3 match between shorts needed and available, preventing validation of selection logic. Test data was adjusted across multiple rebalancing tests to ensure no positions hit take-profit thresholds, which would trigger MAINTAIN_POSITION and be correctly excluded by the fix. All Python checks pass with 94% test coverage maintained. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@applications/portfoliomanager/src/portfoliomanager/risk_management.py`:
- Around line 219-228: best_shorts and best_longs currently select only
"ticker", which makes the subsequent portfolio_with_actions.is_in(...) flip all
rows for that ticker; update the selection to include both "ticker" and the
unique position identifier (timestamp) — e.g., change the .select("ticker") in
the best_shorts / best_longs pipeline to .select("ticker", "timestamp") (or the
actual unique column used for positions), and then perform the membership test
using a two-column is_in/semi-join against that two-column DataFrame so only the
intended rows are marked with PositionAction.CLOSE_POSITION (ensure the same
change is applied in both short and long flows and where is_in is used).
In `@applications/portfoliomanager/tests/test_risk_management.py`:
- Around line 754-861: Update the misleading inline comment inside
test_add_portfolio_performance_columns_rebalancing_closes_shorts_when_longs_stopped:
change the line that reads "Rebalancing closes top 2 performing shorts: SHORT1
(-5%) and SHORT4 (-3%)" to accurately reflect the computed returns and selection
(e.g., "Rebalancing closes top 2 performing shorts: SHORT1 (~-4.5%) and SHORT2
(~-4.0%)"), so the comment matches the assertions that check for SHORT1 and
SHORT2 being closed.
Contributor
Greptile OverviewGreptile Summary
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| applications/portfoliomanager/src/portfoliomanager/alpaca_client.py | Switched order submission from share qty to notional dollar amount and removed quote-based sizing; added validation for non-positive dollar_amount. |
| applications/portfoliomanager/src/portfoliomanager/enums.py | Removed PositionAction values (PDT_LOCKED/MAINTAIN_POSITION); risks breaking persisted data and any remaining logic that expects those states. |
| applications/portfoliomanager/src/portfoliomanager/portfolio_schema.py | Updated schema allowed PositionAction values to match enums (removed PDT_LOCKED/MAINTAIN_POSITION), which may be incompatible with existing stored portfolios/consumers. |
| applications/portfoliomanager/src/portfoliomanager/risk_management.py | Removed portfolio performance/action and rebalancing logic; create_optimal_portfolio now always builds a fresh 10/10 portfolio from predictions and excludes only high-uncertainty + prior tickers. |
| applications/portfoliomanager/src/portfoliomanager/server.py | Changed workflow to fetch only prior tickers and then close all of them + open all new portfolio positions, which causes full churn each run and likely violates intended rebalance semantics. |
| applications/portfoliomanager/tests/test_risk_management.py | Replaced extensive action/performance tests with new tests for ticker exclusion and fixed 20-position construction; tests no longer cover stop-loss/take-profit/PDT logic described in PR. |
| uv.lock | Dependency lockfile updated (not reviewed in depth for logic issues). |
Contributor
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/portfoliomanager/src/portfoliomanager/server.py
Line: 152:160
Comment:
**Closes all prior positions**
`get_positions()` currently sets `close_positions = [{"ticker": ticker} for ticker in prior_portfolio_tickers]` and then opens *every* row in `optimal_portfolio` (server.py:442-462). That means on every rebalance you will attempt to close every previously-held ticker even if it should remain in the portfolio, causing unnecessary churn and potentially turning a “rebalance” into “liquidate and repurchase” every run. This also contradicts the earlier behavior where only tickers not present in the new portfolio were closed.
How can I resolve this? If you propose a fix, please make it concise. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
applications/portfoliomanager/src/portfoliomanager/risk_management.py:1
head(10)/tail(10)assumescurrent_predictionsis already sorted by desirability (composite score descending). That’s true when callers useadd_predictions_zscore_ranked_columns(), butcreate_optimal_portfolio()itself doesn’t enforce ordering, so unsorted inputs will silently pick the wrong longs/shorts. Fix by sortingavailable_predictionsinsidecreate_optimal_portfolio()(e.g., bycomposite_scoredesc andinter_quartile_rangeasc) before selecting head/tail.
import os
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/portfoliomanager/src/portfoliomanager/server.py (1)
442-460:⚠️ Potential issue | 🟡 MinorSwapped return order: function returns
(open, close)but callers expect(open, close)— verify intent.The function signature and docstring say "positions to close and open" (line 446), but it returns
open_positionsfirst andclose_positionssecond (line 462, implied by the order). The caller at line 152 destructures asopen_positions, close_positions = get_positions(...). The return order matches the caller, but contradicts the docstring ordering ("close and open").Consider aligning the docstring with the actual return order to avoid confusion.
🔧 Proposed fix
- """Get positions to close and open.""" + """Get positions to open and close."""
🤖 Fix all issues with AI agents
In `@applications/portfoliomanager/src/portfoliomanager/risk_management.py`:
- Around line 90-94: Replace the five hardcoded "10" usages with a derived
per-side constant so the code follows REQUIRED_PORTFOLIO_SIZE; add
POSITIONS_PER_SIDE = REQUIRED_PORTFOLIO_SIZE // 2 and use it in the selections
and sizing: use available_predictions.head(POSITIONS_PER_SIDE) and
.tail(POSITIONS_PER_SIDE) instead of 10, compute dollar_amount_per_position =
(maximum_capital / 2) / POSITIONS_PER_SIDE (and replace the other two
occurrences of 10 later in this method with POSITIONS_PER_SIDE) so portfolio
sizing always stays in sync with REQUIRED_PORTFOLIO_SIZE.
In `@applications/portfoliomanager/src/portfoliomanager/server.py`:
- Line 42: Remove the unused Ruff `# noqa` directives: delete `# noqa: E402` on
the import line that brings in PositionSide and TradeSide and also remove the `#
noqa: PLR0911` attached to the function/class that currently has that comment
(the long function flagged at the same location), leaving the code to rely on
actual linter configuration; ensure no other code changes are made beyond
removing those unused noqa comments.
- Around line 404-409: Replace the two logger.warning(...) calls in the
exception handlers that catch (ValueError, requests.exceptions.JSONDecodeError)
and the broad Exception with logger.exception(...) so the stack trace is
captured; keep the existing log messages ("Failed to parse prior portfolio JSON"
and "Unexpected error fetching prior portfolio") and the return [] behavior
unchanged, i.e., change logger.warning(..., error=str(e)) to
logger.exception(...) in those except blocks where logger is used.
- Around line 365-409: The current get_prior_portfolio_tickers function swallows
all exceptions which prevents create_portfolio from handling upstream failures;
modify get_prior_portfolio_tickers so it only handles known/expected cases
(e.g., HTTP_NOT_FOUND, JSON decode errors/ValueError) and either re-raise
unexpected exceptions or return a distinct error sentinel that create_portfolio
can detect; specifically remove or change the broad "except Exception as e"
block in get_prior_portfolio_tickers, ensure JSONDecodeError/ValueError still
return [] for "no prior portfolio", and for network/timeouts/other unexpected
errors let the exception propagate (or raise a custom exception) so
create_portfolio can surface a 500 instead of silently proceeding.
In `@applications/portfoliomanager/tests/test_risk_management.py`:
- Around line 168-205: The test
test_create_optimal_portfolio_high_uncertainty_exclusions currently passes an
unsorted predictions DataFrame into create_optimal_portfolio which in production
expects data pre-ranked (e.g. by add_predictions_zscore_ranked_columns) because
create_optimal_portfolio relies on head(10)/tail(10) for long/short selection;
update the test to include a short comment inside
test_create_optimal_portfolio_high_uncertainty_exclusions explaining that
ranking is intentionally omitted (i.e., predictions are unsorted) because the
test only verifies filtering of high-uncertainty tickers, or alternatively sort
the DataFrame by composite_score descending before calling
create_optimal_portfolio to mirror production behavior.
- Around line 243-276: Refactor the
test_create_optimal_portfolio_equal_capital_allocation to use pytest parametrize
instead of the for-capital loop: add `@pytest.mark.parametrize` decorator for the
capital values (10000.0, 25000.0, 50000.0), change the test signature to accept
a capital parameter, remove the internal for loop, and run the existing
assertions against that capital; ensure pytest is imported if not already and
keep references to create_optimal_portfolio and
add_predictions_zscore_ranked_columns unchanged.
…and tests Root cause: PR #749 had 14 unresolved review threads from copilot and coderabbitai covering: - Hard-coded magic numbers (10, 20) scattered throughout risk_management.py - Fragile sorting assumption in create_optimal_portfolio (relied on pre-sorted input) - Potential duplicate counting in excluded_tickers logging - Poor error handling (logger.warning instead of logger.exception, broad exception swallowing) - Missing test coverage for unsorted input - Non-parameterized capital loop test Fix: 1. Constants (Group 1): - Added SIDE_SIZE = REQUIRED_PORTFOLIO_SIZE // 2 constant - Replaced all 5 hard-coded "10" occurrences with SIDE_SIZE - Replaced hard-coded "20" in log message with REQUIRED_PORTFOLIO_SIZE 2. Sorting logic (Group 2): - Added internal sorting in create_optimal_portfolio by composite_score (desc) and inter_quartile_range (asc) before head/tail selection - Ensures correct long/short selection even if input is unsorted 3. Duplicate exclusion tracking (Group 6): - Convert excluded_tickers to set to remove duplicates before logging - Ensures total_excluded count accurately reflects unique tickers 4. Error handling (Group 4): - Changed logger.warning to logger.exception in JSON parse exception handler - Removed broad "except Exception" block that was swallowing unexpected errors - Kept E402 noqa comments (imports must follow structlog configuration) - Kept PLR0911 noqa (get_prior_portfolio_tickers has 7 returns by design) 5. Test improvements (Group 5): - Added test_create_optimal_portfolio_unsorted_input to verify sorting works - Parameterized test_create_optimal_portfolio_equal_capital_allocation with @pytest.mark.parametrize for cleaner testing across capital values - Added comment explaining intentional lack of ranking in high_uncertainty test Rejected feedback: - Thread 1 (Alpaca notional for shorts): Alpaca API docs show notional parameter usage with SELL side. Current implementation follows documented patterns. - Thread 8 (PDT lock logic): Delete-and-replace strategy is intentional per commit e119243. System now closes all positions and opens fresh portfolio. Verification: All Python checks pass (mask development python all): - Formatting: ✓ - Linting: ✓ - Type checking: ✓ - Dead code analysis: ✓ - Tests: 68 passed, 94% coverage ✓ Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Collaborator
Author
PR #749 Review ResponseAddressed all actionable feedback in commit 24576bb. Summary of changes: ✅ Addressed1. Hard-coded constants (Comments on lines 13, 78, 94, 103)
2. Sorting assumption (Comment on line 92)
3. Duplicate exclusion tracking (Comment on line 69)
4. Unsorted input test coverage (Comment on line 197)
5. Error handling improvements
6. Test improvements
❌ Rejected (with rationale)1. Alpaca notional for shorts (Comment on line 85 - alpaca_client.py)Rationale: Alpaca API documentation explicitly shows notional parameter usage with SELL orders: # From Alpaca docs
MarketOrderRequest(
symbol="SPY",
notional=4000,
side=OrderSide.SELL
)The current implementation follows Alpaca's documented patterns. If short selling had restrictions with notional, we would encounter API errors in production/paper trading. This concern is speculative without evidence of actual API failures. 2. Unconditional closing of prior positions (Comment on line 460 - server.py)Rationale: The "delete-and-replace" strategy is intentional per commit e119243. The PR simplified portfolio rebalancing by removing complex PDT_LOCKED/MAINTAIN_POSITION logic. The system now:
This eliminates complex position action decisioning and is the correct behavior for this PR. VerificationAll Python checks pass:
|
Collaborator
Author
Specific Thread ResolutionsResponding to individual review threads: Thread: Hard-coded constants (IDs 2776731213, 2776731223, 2776731229, 2776731238) Thread: Sorting assumption (ID 2776731201) Thread: Duplicate exclusions (ID 2776731240) Thread: Unsorted input test (ID 2776731269) Thread: Alpaca notional for shorts (ID 2776731194) Thread: Unconditional closing (ID 2776731254) Threads about removed features (IDs 2776731058, 2776731109, 2771967662, etc.) |
Contributor
Greptile OverviewGreptile SummarySimplified portfolio rebalancing to a "delete-and-replace" strategy that closes all prior positions and opens a fresh 10-long/10-short portfolio each cycle. Removed complex performance tracking (stop-loss, take-profit, PDT-lock) and switched Alpaca orders from quantity-based to notional (dollar-based) to let the broker handle fractional shares automatically. Major changes:
Test coverage:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| applications/portfoliomanager/src/portfoliomanager/risk_management.py | Simplified to "delete-and-replace" strategy: removed performance tracking, stop-loss, take-profit, and PDT-lock logic; now creates fresh 10-long/10-short portfolio each time |
| applications/portfoliomanager/src/portfoliomanager/server.py | Simplified to fetch only prior tickers (not full portfolio with performance), close all prior positions, and open fresh portfolio; removed equity bars and predictions fetching |
| applications/portfoliomanager/src/portfoliomanager/alpaca_client.py | Switched from quantity-based orders (with manual price fetching) to notional dollar-based orders, letting Alpaca handle fractional shares automatically |
| applications/portfoliomanager/tests/test_risk_management.py | Removed tests for performance tracking, PDT-lock, and stop-loss/take-profit logic; added tests for fresh portfolio creation, ticker exclusion, and capital allocation |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@applications/portfoliomanager/src/portfoliomanager/risk_management.py`:
- Around line 82-89: The error message in the block that raises
InsufficientPredictionsError uses a hardcoded "10 long + 10 short" text which
will be incorrect if SIDE_SIZE or REQUIRED_PORTFOLIO_SIZE change; update the
message in the condition that checks available_predictions.height <
REQUIRED_PORTFOLIO_SIZE to reference the SIDE_SIZE constant (e.g., "{SIDE_SIZE}
long + {SIDE_SIZE} short" or compute from REQUIRED_PORTFOLIO_SIZE) instead of
the literal "10 long + 10 short", while keeping the rest of the context
(available_predictions.height, REQUIRED_PORTFOLIO_SIZE,
len(high_uncertainty_tickers), len(prior_portfolio_tickers)) the same before
raising InsufficientPredictionsError.
In `@applications/portfoliomanager/src/portfoliomanager/server.py`:
- Around line 365-406: The function get_prior_portfolio_tickers treats all HTTP
errors >=400 as "no prior portfolio" which hides server-side (5xx) failures;
change the logic so that 404 returns [] (no prior portfolio) and other 4xx
client errors log a warning and return [], but any 5xx server error should log
an error with the response status/text and raise an exception (or call
response.raise_for_status()) so the failure propagates to the caller instead of
silently returning an empty exclusion list; update references to
prior_portfolio_response and the HTTP status handling accordingly and ensure
logs include the status_code and response_text for debugging.
chrisaddy
approved these changes
Feb 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Changes
Context
This is a gutting for simplicity. It will need to be updated in the future.
Summary by CodeRabbit
Release Notes
Refactor
Bug Fixes