pal-e-docs

Review: SimulationResult model + P&L aggregation service

review-1758-2026-07-04 Review

review approved
Project

Verdict: APPROVED

Re-review of issue #82 after fix for dry_run status pattern. The issue body now correctly references status: "cancelled" with metadata: {"dry_run": true}, matching the actual OrderService#handle_dry_run implementation at app/services/order_service.rb:270-289.

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Parent and dependency chain documented
  • [x] Repo — ldraney/prediction-assistant
  • [x] User Story — AI Portfolio Builder reference
  • [x] Context — Dry_run mode background, corrected status pattern
  • [x] File Targets — 7 files (4 new, 3 existing)
  • [x] Feature Flag — "none" (internal data model, acceptable)
  • [x] Acceptance Criteria — 6 criteria
  • [x] Test Expectations — Model, job, idempotency, scopes
  • [x] Constraints — Listed
  • [x] Checklist — Present
  • [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
  • [x] arch:rails label — Rails architecture
  • [ ] arch note MISSING — [SCOPE] No arch-rails note exists in pal-e-docs. Low priority: arch:rails is used pervasively across all prediction-assistant tickets; creating a dedicated note is a project-wide task, not a blocker for this ticket.
  • [x] Forgejo issue — #82, open

File Targets

  • [x] app/models/simulation_result.rb — NEW file, does not exist yet (correct)
  • [x] db/migrate/XXXXXX_create_simulation_results.rb — NEW migration (correct)
  • [x] app/jobs/simulation_aggregator_job.rb — NEW job (correct, existing jobs directory at app/jobs/ confirmed)
  • [x] app/models/strategy.rb — EXISTS, verified. Has has_many :trades and config validation framework. Adding simulated_pnl(period:) method is clean.
  • [x] test/models/simulation_result_test.rb — NEW test (correct)
  • [x] test/models/strategy_test.rb — EXISTS, verified. 12k file with existing test coverage.
  • [x] test/jobs/simulation_aggregator_job_test.rb — NEW test (correct)

Critical Fix Verified

The previous review found the issue assumed status: "dry_run" but OrderService uses status: "cancelled" with metadata: {"dry_run": true}. Verified against actual code:
  • OrderService#handle_dry_run (order_service.rb:270-289): trade.update!(status: "cancelled", metadata: trade.metadata.merge("dry_run" => true))
  • Trade::STATUSES = %w[pending filled cancelled settled] — "dry_run" is NOT a valid trade status, confirming "cancelled" is correct
  • Issue body AC, Context, and Constraints sections all consistently reference status: "cancelled" + metadata['dry_run'] == true

Repo Placement

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

Dependencies

  • Depends on S6 bots (all 4 with dry_run support) — S6 is merged per project memory
  • Blocks #83 (Bot card performance indicators, board item #1759, in todo) and #84 (Bot detail P&L dashboard, board item #1760, in backlog)
  • Dependencies are documented in the Lineage section

Acceptance Criteria

6 ACs, all testable by an agent:
  • AC1: Model existence + schema — verifiable via migration and model file
  • AC2: Job aggregation from cancelled/dry_run trades — verifiable via test fixtures
  • AC3: Idempotency — verifiable by running job twice
  • AC4: Period scopes — verifiable via scope queries
  • AC5: Strategy convenience method — verifiable via unit test
  • AC6: Test coverage — meta-criterion, verified by presence of tests
Note: The trades table has a pnl column (decimal) that the existing bots_controller already sums. The aggregation job can rely on this column for P&L computation. For dry_run trades where pnl may be nil, the agent will need to determine the simulation P&L mechanism — this is a reasonable implementation decision within scope.

Blast Radius

Minimal. SimulationResult is a new, standalone table. No existing code references it. The existing bots_controller.rb:13 sums Trade.pnl for display, but SimulationResult is a separate aggregation layer that does not modify Trade records. The recurring.yml will need an entry for SimulationAggregatorJob, following the existing MarketScannerJob pattern.

Decomposition Assessment

7 file targets / 1 repo / 6 ACs / ~5 min estimated. The AC count (6) is marginally over the >5 threshold, but the work is highly cohesive: model + migration + job + convenience method + tests for a single concept (simulation P&L aggregation). Splitting would create artificial dependency chains between tightly coupled artifacts. No decomposition needed.

Recommendation

No action needed — issue body is correct and complete after the dry_run status fix.
[SCOPE] (non-blocking): Create architecture note arch-rails for the Rails architecture component. This applies to all prediction-assistant tickets with the arch:rails label, not just this one.