Review: OrderService dry_run enforcement + budget guardrails

review-1765-2026-07-04 Review

review approved

Project

Verdict: APPROVED

Ticket is well-scoped, file targets verified, traceability solid. One minor arch note gap flagged as systemic (not blocking).

[x] Type — Feature
[x] Lineage — Sub-ticket of #85, split 2 of 3
[x] Repo — ldraney/prediction-assistant
[x] User Story — present, well-formed
[x] Context — explains relationship to #85, dependency on sub-ticket 1
[x] File Targets — 2 files to modify, 2 files explicitly excluded
[x] Feature Flag — "none" with justification (internal service change, no FF infra)
[x] Acceptance Criteria — 7 items
[x] Test Expectations — 5 unit tests + run command
[x] Constraints — backward compat, per-strategy budget, integer cents, log-not-raise
[x] Checklist — present
[x] Related — parent issue, dependency, user story doc

[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)
[x] arch:rails label — present on board item
[ ] arch note MISSING — [SCOPE] No arch-rails note found in pal-e-docs. This is a project-wide gap: many prediction-assistant board items carry arch:rails but no backing note exists. Recommend creating arch-rails as a project-level architecture note covering the Rails 8.1 + Hotwire + Solid Queue stack.
[x] Forgejo issue — #89, open

[x] app/services/order_service.rb — verified: exists (14k), contains OrderService class with place_order, cancel_order, check_fill_status methods. Already has @dry_run pattern with handle_dry_run method. The bot: param is typed as Strategy, so bot.trading_enabled? and bot.budget_cents access path is valid once sub-ticket 1 adds those fields.
[x] test/services/order_service_test.rb — verified: exists (18k), established test file for OrderService
[x] NOT-TOUCH: app/models/strategy.rb — verified: currently has no trading_enabled or budget_cents fields (correct, those come from sub-ticket 1)
[x] NOT-TOUCH: Bot job files — verified: jobs (sweep_eligible_job.rb, pregame_threshold_job.rb) do not call OrderService directly; they delegate through bot service classes

OK. Issue #89 filed on ldraney/prediction-assistant, all file targets are in that repo. Single-repo change.

Depends on sub-ticket 1: issue #88 "Trading fields migration + Strategy model validations" (board item #1764, currently in backlog). This dependency is documented in the Lineage and Context sections. trading_enabled? and budget_cents must exist on Strategy before this ticket can be implemented.
No downstream blockers identified — this ticket is a prerequisite for sub-ticket 3 (Activation UI panel, #90).

7 ACs are well-defined and testable by an agent:

AC 1-2: trading_enabled? check + dry_run forcing — verifiable via unit test assertions on return value
AC 3-5: Budget enforcement — verifiable via unit tests with known trade amounts
AC 6: ActivityLog recording — verifiable by asserting ActivityLog.count change
AC 7: Backward compatibility — verifiable by running existing test suite unchanged

Run command is valid: bin/rails test test/services/order_service_test.rb

Callers safe: PregameStackerBot and LateGameLockBot both create OrderService.new(client:, dry_run:) and pass bot: strategy to place_order. The new enforcement is internal to OrderService — callers need no changes.
cancel_order not affected: ACs scope enforcement to "placing any order" — cancellations are correctly excluded since the concern is preventing real money outflow.
No sibling service concern: OrderService is the single shared trade execution service; no similar pattern exists elsewhere that would need the same fix.

File targets: 2 files in 1 repo — under the 3-file threshold
Acceptance criteria: 7 — over the 5-AC threshold on paper, but all are tightly coupled variations of one behavioral change (guard clause + budget check in a single method)
Estimated agent work: well under 5 minutes — add guard clause to place_order, add budget helper method, add ~5 test cases to existing test file
No decomposition needed

[SCOPE] Create architecture note arch-rails covering the Rails 8.1 + Hotwire + Solid Queue stack. This is a project-wide gap affecting many board items, not specific to this ticket. Non-blocking for this review.