feat: Consolidate portfolio and position ops into portfolio block

[jfrench9]

Summary

Introduces the Portfolio Block abstraction, consolidating the previously separate portfolios and positions command/read modules into a unified portfolio_block module. This refactor simplifies the roboinvestor domain by treating portfolio and position operations as a single cohesive unit, alongside updates to the investor API model, GraphQL layer, and router operations.

Key Accomplishments

• New Portfolio Block module: Added portfolio_block.py for both commands and reads, replacing the separate portfolios.py and positions.py modules. The new command module (279 lines) and read module (135 lines) provide a unified interface for managing portfolios and their associated positions as a block.

• Investor model refactoring: Significantly reworked robosystems/models/api/extensions/investor.py (~256 lines changed) to align with the portfolio block concept, updating data structures and validation logic.

• GraphQL layer updates: Extended investor.py types with new GraphQL type definitions (37 new lines) and updated resolvers to support the portfolio block pattern.

• Router simplification: Refactored roboinvestor/operations.py router to use the new portfolio block abstraction, resulting in a cleaner and more maintainable API surface.

• Dependency update: Bumped robosystems-client to version 0.3.17 in optional dependencies.

Breaking Changes

• Removed modules: commands/portfolios.py and commands/positions.py have been deleted and replaced by commands/portfolio_block.py. Any code importing from these modules will break.
• API endpoint changes: The router operations have been restructured, which likely changes request/response shapes and/or endpoint paths for roboinvestor portfolio and position operations.
• Model changes: The investor extension model has been significantly modified, which may affect serialization, API contracts, and downstream consumers.

Testing

• New tests added: test_portfolio_block.py (242 lines) and test_portfolio_block_commands.py (438 lines) provide comprehensive coverage for the new portfolio block reads and commands.
• Removed tests: test_portfolios.py (215 lines) and test_positions.py (337 lines) were deleted along with their corresponding modules.
• Updated tests: Router operation tests were refactored (net reduction of ~200 lines), and GraphQL extension tests were updated to reflect the new types and resolvers.
• Net test balance: ~680 lines added vs. ~552 lines removed, indicating improved test coverage for the new abstraction.

Infrastructure Considerations

• The robosystems-client dependency version has been updated, requiring a lock file refresh. Environments using the optional dependency group should be rebuilt to pick up the new client version.
• No migration scripts are included — downstream services consuming the affected API endpoints should be coordinated for deployment.

---

🤖 Generated with Claude Code

Branch Info:

• Source: feature/portfolio-block
• Target: main
• Type: feature

Co-Authored-By: Claude noreply@anthropic.com

[jfrench9]

@claude please review this PR

[claude]

Claude finished @jfrench9's task in 3m 22s —— View job

---

PR Review: feat: Consolidate portfolio and position ops into portfolio block

Tasks:

• Gather context and understand PR scope
• Review core portfolio block implementation (commands/portfolio_block.py, reads/portfolio_block.py)
• Review investor API models (models/api/extensions/investor.py)
• Review GraphQL layer changes (types + resolvers)
• Review router changes (roboinvestor/operations.py)
• Review test coverage
• Post final review

---

Overall this is a clean, well-structured refactor. The "molecule envelope" abstraction is the right call — treating portfolio + positions as an atomic unit removes a whole class of partial-write bugs that atom-level CRUD invites. The command layer, model definitions, and router are all well organized. A few issues to address before merge:

---

Bugs

1. Cost accumulation before security guard in get_portfolio_block — reads/portfolio_block.py:85–95

for p in positions:
    sec = sec_map.get(p.security_id)
    ...
    total_cost += cost_dollars      # ← always accumulated
    if value_dollars is not None:
        total_value += value_dollars
        has_value = True

    if sec is None:                 # ← skip position_blocks.append
        continue

    position_blocks.append(...)

If a position's security is absent from sec_map (e.g. a dangling FK after manual data surgery), its cost is folded into total_cost_basis_dollars but the position doesn't appear in positions or count toward active_position_count. The envelope will report costs that don't reconcile with the individual line items. Move the cost accumulation inside the sec is not None branch, or raise before the loop so callers see a clear invariant violation.

Fix this →

2. Timezone-naive date.today() in dispose path — commands/portfolio_block.py:200

row.disposition_date = date.today()   # ← local system time

The line just below uses datetime.now(UTC), and the rest of the codebase uses UTC-aware timestamps. This should be:

row.disposition_date = datetime.now(UTC).date()

Fix this →

---

Design observations

3. Three separate queries in delete_portfolio_block — commands/portfolio_block.py:230–255

The function makes three queries: active_count, total_positions count, then loads all positions for deletion. The total count is redundant — len(positions_to_delete) gives the same number without an extra round-trip:

positions_to_delete = (
    session.execute(select(Position).where(...)).scalars().all()
)
for row in positions_to_delete:
    session.delete(row)
session.delete(portfolio)
session.flush()

return DeletePortfolioBlockResponse(
    deleted=True,
    portfolio_id=body.portfolio_id,
    positions_deleted=len(positions_to_delete),   # no separate count query needed
)

4. SecurityNotFoundError reports only the first missing ID — commands/portfolio_block.py:76–77

raise SecurityNotFoundError(sorted(missing)[0])

When a caller provides add deltas with several unknown security_ids, only the lexicographically first one surfaces. If all IDs are bad (a typo in a bulk import), the caller has to fix one error at a time. Raising with sorted(missing) directly is an easy improvement.

5. type: ignore on ActivePositionsRequireConfirmationError error map — router/operations.py:195

lambda e: (  # type: ignore[attr-defined]
    f"Portfolio has {e.active_count} active position(s); ..."
),

The active_count attribute is typed on the exception class. The ignore is likely here because the error_map lambda parameter is typed as Exception. Consider casting: lambda e: f"... {cast(ActivePositionsRequireConfirmationError, e).active_count} ..." — eliminates the ignore and makes the intent explicit.

---

Minor

6. Bare dict annotations in get_portfolio_block — reads/portfolio_block.py:58–59

sec_map: dict = {}
entity_map: dict = {}

These should carry specific type arguments consistent with the rest of the codebase: dict[str, Security] and dict[str, Entity].

7. New portfolio block types missing from __init__.py public surface — models/api/extensions/__init__.py

The __init__.py now exports CreatePortfolioBlockRequest, DeletePortfolioBlockOperation, DeletePortfolioBlockResponse, and UpdatePortfolioBlockOperation but not the read-side shapes: PortfolioBlockEnvelope, PortfolioBlockPositions, EntityLite, SecurityLite, PositionBlock, PortfolioBlockPortfolioFields, PortfolioBlockPortfolioPatch, and the position delta models. The router test imports them directly from investor.py, which works, but the __init__.py re-export surface is now asymmetric. Either export all or none of the portfolio block types through __init__.py.

8. id scalar inconsistency in new GraphQL types — graphql/types/investor.py:172–192

The pydantic-derived EntityLite, SecurityLite, and PositionBlock types will expose id as a GraphQL String scalar (the pydantic decorator maps str to String). The hand-written Security type explicitly uses id: strawberry.ID. For GraphQL clients and tooling, ID and String are semantically different — id fields should use the ID scalar.

---

What's well done

• The validate-before-write pattern in create_portfolio_block (securities validated before any session.add) correctly ensures no partial writes on bad input.
• The atomic update_portfolio_block — loading both update_ids and dispose_ids in one query, then validating portfolio membership before any mutation — is clean and correct.
• ActivePositionsRequireConfirmationError carrying active_count as a typed attribute (not just in the message) is a nice touch for the router.
• The OperationSpec/OperationRegistrar pattern in the router is used consistently and keeps the route surface thin.
• Test coverage is thorough: the _build_session helper in test_portfolio_block.py elegantly sequences mock execute() returns without brittle position-index tricks.