Feat/ops foundation#1107
Conversation
Captures the agreed design for the first slice of the live-trading buildout: broker abstraction, guardrail engine, position guardian, scheduler, post-earnings momentum strategy, journal, and notifications. Paper-only in v1; live Robinhood execution wired but gated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implementation plan for the first of three phased plans under the live-v1 spec. Covers Broker ABC, PaperBroker, GuardedBroker, all 13 guardrail rules, RuleEngine, and SQLite Journal with full TDD task breakdown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Create empty ops/ and tests/ops/ package skeletons (broker + guardrails sub-packages) as the foundation for the live trading operations layer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t comment - load_config() now builds a kwargs-only-if-set dict so OpsConfig field defaults are the single source of truth; duplicate inline defaults removed. - _env_decimal/_env_int no longer accept a default arg; they return None when the var is absent and raise ValueError naming the env var on bad input. - deny_list field annotated with "Not env-overridable; extend via code". - Three new tests: defaults parity, bad-decimal attribution, bad-int attribution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add __enter__/__exit__ for context-manager support (closes connection on exit) - Add check_same_thread=False so orchestrator and position-guardian threads can share one Journal instance without ProgrammingError - Enable WAL mode (concurrent reads during writes) - Validate tzinfo in _from_iso to symmetrically reject naive datetimes on read - Add 3 tests: naive-datetime rejection for fills and snapshots, and context-manager connection lifecycle Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…long-only, stop, fractional)
…itions, cash reserve)
Wraps any Broker with a RuleEngine; every order placement runs the chain first. Rejections raise OrderRejected and are appended to the journal as "order_rejected" events. Inner broker is private — the only Broker callers see outside the broker package is GuardedBroker. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dation
Address the Critical and Important findings from the final code review of
the Foundation branch:
* Add `build_guarded_paper_broker` factory in `ops/__init__.py` as the
canonical assembly path. The factory constructs the inner PaperBroker
and the 13-rule chain internally and never returns the unwrapped
reference, so callers cannot bypass guardrails by holding it.
* Name-mangle the inner broker in GuardedBroker (`__inner`) so the
conventional `_inner` bypass the reviewer demonstrated is mechanically
blocked. Tested: `g._inner` now raises AttributeError.
* Wrap `place_order` in a `threading.Lock` so guardrail evaluation +
inner fill are atomic. Without this, two concurrent BUYs can both
read pre-trade state, both pass sizing/cash rules, and both fill.
* Journal broker-layer rejections (InsufficientFunds, NoSuchPosition)
from the inner broker as `order_rejected` events with rule="broker".
Closes a gap the journal docstring's "every state change MUST land
here" promise was leaving open.
* Add `OpsConfig.__post_init__` validation: drawdown/stop pcts must be
negative, cap/reserve pcts in [0,1], max_open_positions > 0,
per_trade_dollar_floor >= 0, broker_mode in {paper,robinhood}.
Catches the "OPS_DAILY_DRAWDOWN_PCT=0.07 (missing minus)" failure
mode that would silently disable the kill switch.
Tests: 14 new in tests/ops/test_factory.py, including a concurrent-
buy race verifying the lock holds. Full ops suite: 85 passed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements the foundation of the live-trading layer, introducing a guarded paper broker (GuardedBroker wrapping PaperBroker), a SQLite-based append-only event journal, and a suite of thirteen safety guardrail rules evaluated by a RuleEngine. Feedback on the implementation highlights several critical improvements: in PaperBroker, qty_to_sell should be capped to existing.quantity when within the epsilon threshold to prevent negative remaining quantities, and quote prices should be validated to be strictly positive to avoid division-by-zero errors. Additionally, CashReserveRule should explicitly reject trades resulting in negative cash to preserve the cash-only constraint under negative equity or zero-reserve configurations, and the Journal class should synchronize SQLite writes with a lock to ensure thread safety when accessed concurrently.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if qty_to_sell > existing.quantity + _EPSILON: | ||
| raise NoSuchPosition( | ||
| f"sell qty {qty_to_sell} exceeds position {existing.quantity}" | ||
| ) |
There was a problem hiding this comment.
If qty_to_sell is slightly larger than existing.quantity but within the _EPSILON threshold, the NoSuchPosition exception is not raised. However, the code proceeds to sell qty_to_sell which exceeds the actual position quantity. This results in a negative remaining quantity and credits the account with extra proceeds (effectively creating shares/cash out of thin air). To prevent this, qty_to_sell should be capped to existing.quantity when it is within the _EPSILON boundary.
| if qty_to_sell > existing.quantity + _EPSILON: | |
| raise NoSuchPosition( | |
| f"sell qty {qty_to_sell} exceeds position {existing.quantity}" | |
| ) | |
| if qty_to_sell > existing.quantity: | |
| if qty_to_sell > existing.quantity + _EPSILON: | |
| raise NoSuchPosition( | |
| f"sell qty {qty_to_sell} exceeds position {existing.quantity}" | |
| ) | |
| qty_to_sell = existing.quantity |
| floor = equity * ctx.config.cash_reserve_pct | ||
| post_cash = cash - ctx.order.notional_dollars | ||
| if post_cash < floor: | ||
| return RuleResult.reject( | ||
| f"post-trade cash ${post_cash} below reserve floor ${floor}" | ||
| ) |
There was a problem hiding this comment.
In CashReserveRule, if equity is negative, the calculated floor will also be negative. Similarly, if cash_reserve_pct is configured to 0, the floor is 0. In these scenarios, checking only post_cash < floor can allow post_cash to become negative (e.g., if floor is -20, a post_cash of -15 is allowed). This violates the core cash-only / no-margin constraint. We should explicitly reject any trade that would result in negative cash.
| floor = equity * ctx.config.cash_reserve_pct | |
| post_cash = cash - ctx.order.notional_dollars | |
| if post_cash < floor: | |
| return RuleResult.reject( | |
| f"post-trade cash ${post_cash} below reserve floor ${floor}" | |
| ) | |
| floor = equity * ctx.config.cash_reserve_pct | |
| post_cash = cash - ctx.order.notional_dollars | |
| if post_cash < 0: | |
| return RuleResult.reject(f"post-trade cash ${post_cash} would be negative") | |
| if post_cash < floor: | |
| return RuleResult.reject( | |
| f"post-trade cash ${post_cash} below reserve floor ${floor}" | |
| ) |
| notional_dollars=order.notional_dollars, | ||
| stop_loss_price=order.stop_loss_price, | ||
| ) | ||
| price = self._quote(order.symbol) |
There was a problem hiding this comment.
The quote price returned by self._quote(order.symbol) is not validated. If the quote source returns a zero or negative price (e.g., due to an API error or a missing quote), it will cause a ZeroDivisionError or result in an invalid negative position quantity during the trade execution. Consider validating that price > 0 before proceeding with the order fill.
| class Journal: | ||
| def __init__(self, path: str): | ||
| self._path = path | ||
| self._conn = sqlite3.connect(path, isolation_level=None, check_same_thread=False) |
There was a problem hiding this comment.
Passing check_same_thread=False allows the SQLite connection to be accessed across multiple threads (such as the background PositionGuardian thread and the main Orchestrator thread). However, Python's sqlite3 connection is not thread-safe for concurrent write operations without external synchronization. To prevent database corruption or operational errors, consider adding a threading.Lock to serialize all database executions within the Journal class.
No description provided.