Executor pattern, RobinhoodClient, deferred task scheduler

[IamJasonBian]

Summary

• Split SafeCashBot into RobinhoodClient (thin broker wrapper) and Executor (execution quality layer)
• Add Brokerage ABC (utils/brokerage.py) for dependency inversion — Executor depends on the interface, not the concrete client
• Add TradeTask / ScheduledTask / DeferredTask (trade_task.py) using Command + Strategy patterns
• PDT-blocked orders are deferred to next trading day via in-memory _deferred_queue, drained at top of each engine cycle
• Fix cancel_order_by_id → cancel_order across main.py and all tests
• Align lot_size and strategy pricing defaults so tests match live config

Test plan

• pytest tests/ — all 166 tests pass
• Engine dry-run completes without attribute errors

[IamJasonBian]

Code Review — PR #53

Overall: Well-structured refactor splitting SafeCashBot into RobinhoodClient (thin broker wrapper) and Executor (execution quality layer). The Brokerage ABC for dependency inversion is a clean architectural improvement that will make testing and future broker integrations much easier.

Issues

1. Hardcoded account number — RobinhoodClient.EXPECTED_ACCOUNT = "490706777" is a class constant. This should be an env var or config value, not committed to source. Even as a safety check, hardcoding an account number in a public repo is a security concern.

2. Singleton RobinhoodClient complicates testing — create() returns a cached singleton and calls sys.exit(1) on missing env vars. Tests will need to call reset() between runs and mock env vars carefully. Consider making create() accept optional overrides or having a create_for_test() factory.

3. PDT retry in place_limit_buy bypasses the Executor — The client has its own PDT retry logic (cancel existing buy, resubmit) baked into place_limit_buy. But the Executor also owns PDT gating. This creates two competing PDT handling paths. The client should be a thin passthrough — move retry logic to the Executor.

4. _next_trading_day doesn't handle market holidays — Only skips weekends. An order deferred on Friday before a Monday holiday would attempt execution on that holiday. Consider using a holiday calendar or at minimum documenting the limitation.

5. 774-line robinhood_client.py — This is a large file for a "thin wrapper." Methods like get_portfolio_history, get_recent_orders, get_recent_option_orders, and order validation logic may belong in separate modules or mixins to keep the client focused on order placement and account state.

6. cancel_order_by_id → cancel_order rename — Good cleanup, but verify no other callers exist outside this PR (scripts, notebooks, etc.) that still reference the old name.

Strengths

• Brokerage ABC is minimal and well-defined (8 abstract methods)
• TradeTask hierarchy (ScheduledTask, DeferredTask) with Strategy pattern for should_execute() is clean
• Deferred queue draining at cycle start is the right approach
• Test updates are thorough — renaming load_broker_sell_orders → load_broker_orders and fixing created_at type assertions

---

Generated by Claude Code

[IamJasonBian]

Follow-up Review — PR #53

A few additional items not covered in the initial review:

1. drain_deferred() is never called — Executor.drain_deferred() is documented to run "at the top of each engine cycle," but there is no call site in main.py. Deferred orders will accumulate and never be retried.

2. self.executor can remain None — In main.py, self.executor = None is set initially, then only assigned inside a try block. If initialization fails, any subsequent self.executor.submit(...) call raises AttributeError. Add a None guard or fail fast.

3. validate_buy_order bypasses the Brokerage ABC — Called directly on self.trading_bot (the concrete RobinhoodClient), but validate_buy_order is not declared as an abstract method on Brokerage. This partially defeats the dependency inversion the PR introduces.

4. PDT check duplicated in _handle_order_replacement — main.py has an inline PDT status check via self.trading_bot.get_pdt_status(), separate from the PDTGate inside the Executor. Combined with the retry logic in place_limit_buy, this creates three competing PDT handling paths.

5. ScheduledTask is unused — Defined and imported but never instantiated. If this is forward scaffolding, consider deferring it to a later PR to keep the diff focused.

---

Generated by Claude Code

[IamJasonBian]

Stale PR notice — This PR has been open for 15 days with no new commits since Mar 27. Two code reviews have been posted with actionable items (e.g., drain_deferred() never called, hardcoded account number, competing PDT paths). Please address the review feedback or close if superseded.

---

Generated by Claude Code

[IamJasonBian]

Stale — 4 weeks, no activity since Mar 27. Two prior reviews identified actionable bugs: drain_deferred() is never called (deferred orders silently accumulate), hardcoded account number 490706777 in a public repo, and three competing PDT handling paths. With PR #55 now adding IBKR broker support separately, the broker abstraction story is diverging. Recommend addressing the review feedback or closing if the Executor design has evolved.

---

Generated by Claude Code