pal-e-docs

Review: Trading fields migration + Strategy model validations

review-1764-2026-07-04 Review

review approved
Project

Verdict: APPROVED

Board item #1764 — Forgejo issue ldraney/prediction-assistant#88. Sub-ticket of #85 (Go-live activation flow), split 1 of 3. First review.

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Sub-ticket of #85
  • [x] Repo — ldraney/prediction-assistant
  • [x] User Story — present, well-formed
  • [x] Context — includes naming conflict rationale
  • [x] File Targets — 4 files (1 new, 3 existing) + explicit do-not-touch list
  • [x] Feature Flag — "none" with rationale (internal model change)
  • [x] Acceptance Criteria — 9 criteria
  • [x] Test Expectations — 7 unit tests + run command
  • [x] Constraints — 4 constraints including column naming rationale
  • [x] Checklist — standard 3-item
  • [x] Related — project, parent issue, user story doc

Traceability

  • [x] story:portfolio-builder label — AI Portfolio Builder
  • [x] story note verified — found in project-prediction-assistant user-stories section
  • [x] arch:rails label — present on board item
  • [ ] arch note MISSING — [SCOPE] Create architecture note arch-rails for the Rails application component. This is a project-wide gap affecting all arch:rails tickets, not specific to this issue.
  • [x] Forgejo issue — #88, open

File Targets

  • [x] db/migrate/YYYYMMDD_add_trading_fields_to_strategies.rb — new file; naming convention matches existing migrations (e.g. 20260704000001_create_price_patterns.rb)
  • [x] app/models/strategy.rb — verified: exists (82 lines), has base validations, config_value helper, and validation helpers for subclasses. Correct target for new trading validations.
  • [x] test/models/strategy_test.rb — verified: exists (335 lines), comprehensive test suite for STI, config validations. Correct target for new trading validation tests.
  • [x] db/schema.rb — verified: exists, strategies table at lines 76-85 currently has active, config, name, type columns. Auto-updated by migration.
  • [x] app/models/late_game_lock_config.rb — verified: max_concurrent_positions confirmed on line 10 as JSONB config key. Do-not-touch designation correct.

Repo Placement

OK — issue filed on ldraney/prediction-assistant, all file targets are in the same repo. Single-repo change.

Dependencies

  • Sub-ticket of #85 (board item #1761, backlog). Split 1 of 3 — this is the data layer (migration + model), independent of the other splits (OrderService guardrails, Activation UI).
  • No blocking dependencies in in_progress or todo columns.
  • Sibling sub-tickets: #89 (OrderService dry_run enforcement, item #1765) and #90 (Activation UI panel, item #1766) — both in backlog, both depend on this ticket's columns existing.
  • Dependencies correctly documented in Lineage section.

Acceptance Criteria

9 AC, all agent-verifiable. Test command provided: bin/rails test test/models/strategy_test.rb. Each criterion is specific and testable.
Minor observation: Only budget_cents has an explicit presence validation AC. The Constraints section says "Budget fields are nullable (only required when trading_enabled is true)" which could imply all three monetary/position fields need presence validations. However, the AC is intentionally asymmetric — max_bet_cents and trading_max_positions have numericality checks but no presence requirement, meaning they are optional even when trading is enabled. This is a reasonable design choice (budget is the hard constraint; bet size and position limits are optional tuning).

Blast Radius

  • max_concurrent_positions is referenced in 6+ files as a JSONB config key (late_game_lock_bot.rb, position_tracker.rb, bot_catalog.rb, and multiple test files). The ticket correctly avoids this name by using trading_max_positions.
  • No existing code references trading_enabled, budget_cents, max_bet_cents, or trading_max_positions — clean namespace.
  • New columns are nullable (except trading_enabled default false). No existing Strategy records, tests, or fixtures will break.
  • BotPresenter, BotsController, and job classes reference Strategy but only query active, config, type, and name. No downstream breakage risk.

Decomposition Assessment

4 file targets in 1 repo — under the 3-files/2-repos threshold. 9 AC exceeds the >5 threshold, but the work is atomically cohesive: one migration adding 4 columns, one model adding conditional validations, one test file. Splitting would create artificial dependencies (migration in one PR, validations in another). Estimated agent time: ~3 minutes. 1 story point. No decomposition needed.

Recommendation

  • [SCOPE] Create architecture note arch-rails in pal-e-docs for the Rails application component. This is a project-wide gap, not a blocker for this ticket.
Scope is solid. All file targets verified. Naming conflict analysis confirmed against live code. Blast radius minimal. Ticket is ready for implementation.