pal-e-docs

Review: Go-live activation flow -- simulation to real trading transition

review-1761-2026-07-04 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Parent: Paper trading dashboard (S7). Depends on: Bot detail P&L dashboard, Registration form (#58)
  • [x] Repo -- ldraney/prediction-assistant
  • [x] User Story -- Links to docs/user-stories/ai-portfolio-builder.md (uses link style, not As/I want/So that format -- acceptable)
  • [x] Context -- Clear explanation of simulation vs real trading separation
  • [x] File Targets -- 6 files listed (4 existing, 2 new)
  • [x] Feature Flag -- none (core safety mechanism)
  • [x] Acceptance Criteria -- 8 items
  • [x] Test Expectations -- 5 expectations listed
  • [x] Constraints -- 3 constraints listed
  • [x] Checklist -- Standard 3-item checklist
  • [x] Related -- project-prediction-assistant

Traceability

  • [x] story:portfolio-builder label -- AI Portfolio Builder
  • [x] story note verified -- found in project-prediction-assistant user-stories section (Key: portfolio-builder, Role: Trader (Lucas))
  • [x] arch:rails label -- Rails Architecture
  • [x] arch note verified -- arch-rails note exists in pal-e-docs (title: "Rails Architecture: Prediction Assistant", status: active)
  • [x] Forgejo issue -- ldraney/prediction-assistant#85, open

File Targets

  • [x] app/models/strategy.rb -- verified: EXISTS. Currently has active boolean, no trading_enabled or budget fields. Correct modification target.
  • [x] db/migrate/XXXXXX_add_trading_fields_to_strategies.rb -- new migration, expected to not exist yet
  • [x] app/views/bots/_activation_panel.html.erb -- new partial, expected to not exist yet
  • [x] app/services/order_service.rb -- verified: EXISTS. Currently uses @dry_run constructor parameter. Ticket wants it to check strategy.trading_enabled? -- this is achievable since place_order already receives bot: (the strategy).
  • [x] test/models/strategy_test.rb -- verified: EXISTS, 320 lines of existing tests
  • [x] test/controllers/bots_controller_test.rb -- verified: EXISTS, 457 lines of existing tests

Repo Placement

OK -- issue filed on ldraney/prediction-assistant, all file targets are in the same repo. No cross-repo concerns.

Dependencies

  • Registration form (#58, S6) -- board item 1729 is currently in todo column, not yet complete. Documented in Lineage.
  • Bot detail P&L dashboard -- dependency is documented but no specific issue number referenced. This should be clarified.
  • No in_progress items block this ticket directly.

Acceptance Criteria

8 ACs total. All are testable by an agent. However:
  • AC #4 (OrderService checks trading_enabled) is achievable -- place_order(bot: strategy, ...) already receives the strategy record.
  • AC #7 (Budget guardrail) requires summing existing trades against budget_cents -- straightforward but adds scope.
  • Naming conflict: AC #1 lists max_concurrent_positions as a new integer column on the strategies table. However, max_concurrent_positions already exists as a JSONB config key in LateGameLockConfig (app/models/late_game_lock_config.rb:10) and BotCatalog (app/models/bot_catalog.rb:214). Adding a same-named column creates ambiguity -- ActiveRecord will shadow the JSONB key with the column accessor.

Blast Radius

  • OrderService consumers: PregameStackerBot (app/services/pregame_stacker_bot.rb) and BulkSweepJob (app/jobs/bulk_sweep_job.rb) both construct OrderService with dry_run: parameter. The ticket's change to check strategy.trading_enabled? inside OrderService would affect all callers -- they would get forced dry_run regardless of their constructor param. This is the intended behavior but must be tested across all consumers.
  • Toggle flow: The existing active toggle in BotsController (app/controllers/bots_controller.rb:74-85) and BotPresenter (app/presenters/bot_presenter.rb:19) use strategy.active?. The ticket introduces trading_enabled as a separate concept -- existing active remains for monitoring. No unintended collision.

Decomposition Assessment

  • File targets: 6 files, 1 repo -- does NOT trigger the >3 files across >2 repos rule
  • Acceptance criteria: 8 items -- TRIGGERS the >5 AC rule
  • Estimated agent work: Model + migration + view partial + OrderService change + tests across 2 test files -- estimated 10-15 minutes, TRIGGERS the >5 minutes rule
NEEDS DECOMPOSITION -- suggested split:
  • Model + migration (3 AC): Add trading_enabled, budget_cents, max_bet_cents fields; migration; model validations and tests
  • OrderService guard + budget enforcement (2 AC): trading_enabled? check forces dry_run; budget guardrail rejects over-budget orders
  • Activation UI + controller flow (3 AC): Activation panel partial, LIVE badge, pause button, controller integration tests

Recommendation

  • [BODY] Rename max_concurrent_positions column to avoid collision with existing JSONB config key in LateGameLockConfig. Suggested: trading_max_positions or move to JSONB config instead of a column.
  • [DECOMPOSE] 8 AC across 6 files, estimated >5 minutes. Route to skill-decompose-ticket with the three-way split above.