pal-e-docs

Review: Pre-settlement exit to avoid catastrophic losses

review-1847-2026-07-10 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Discovered during Spike #11
  • [x] Repo -- ldraney/botfire
  • [x] User Story -- As a bot operator / I want / So that
  • [x] Context -- Detailed with settlement loss data
  • [x] File Targets -- 2 modify + 1 do-not-touch
  • [x] Feature Flag -- none
  • [x] Acceptance Criteria -- 5 criteria
  • [x] Test Expectations -- Unit tests + run command
  • [x] Constraints -- 3 constraints listed
  • [x] Checklist -- present
  • [x] Related -- present

Traceability

  • [x] story:bot-strategy label -- present on board item
  • [ ] story note MISSING -- project-botfire has no user-stories section. [SCOPE] Create user-stories section on project-botfire with bot-strategy story entry.
  • [x] arch:bots label -- present on board item
  • [ ] arch note MISSING -- search for arch-bots returned no results. [SCOPE] Create architecture note arch-bots for the bots component.
  • [x] Forgejo issue -- ldraney/botfire#13, open

File Targets

  • [x] app/services/position_monitor.rb -- verified: exists, contains evaluate_exit and resolve_settlement logic (lines 48-94). Correct target for pre-settlement exit check.
  • [ ] app/concerns/game_phase.rb -- ISSUE: wrong path. File is at app/services/game_phase.rb. No app/concerns/ directory exists in this project. The module already has game_nearly_over? which is the exact method needed.
  • [x] app/services/bot_engine.rb -- verified: correctly listed as do-not-touch.
  • [ ] app/services/scoreboard.rb -- MISSING from targets. Scoreboard counts exits only as stop_loss and take_profit (lines 35-36). New pre_settlement_exit reason will be invisible in stats.
  • [ ] app/views/dashboard/index.html.erb -- MISSING from targets. Line 592: isWin = f.exit_reason?.includes('win') || f.exit_reason?.includes('take'). A profitable pre_settlement_exit would be styled as a loss pill because "pre_settlement_exit" contains neither "win" nor "take".

Repo Placement

OK -- all affected files are in ldraney/botfire. Issue correctly filed on that repo. Single-repo change.

Dependencies

  • Spawned from Spike #11 (board item #1845, ldraney/botfire#11) -- in QA column, upstream work complete.
  • Sibling items from same spike: #12 (botfire#12, backlog), #14 (botfire#14, backlog) -- no blocking dependency.
  • Item #1843 (botfire#8, "Add game phase tracking to BotFire records") -- could overlap on game_phase.rb but this ticket only consumes existing game_nearly_over?, no conflict.
  • PositionMonitor does NOT currently include GamePhase -- BotEngine and VolatilityBot do, but PositionMonitor does not. The ticket must add this include.

Acceptance Criteria

  • AC1-3, AC5 are directly verifiable via unit tests.
  • AC4 ("Settlement loss rate drops to near zero for new trades") is an outcome metric, not directly testable in a unit test. An agent could verify it by checking that settlement paths are unreachable when pre-settlement exit fires, but the criterion as written reads as a production metric. Consider rephrasing to: "Games where pre-settlement exit fires produce no settlement_loss exit reasons."
  • Test command bin/rails test is valid for Rails projects.

Blast Radius

  • scoreboard.rb -- Only counts stop_loss and take_profit. New exit reason will not appear in any scoreboard tallies. Must either add a pre_settlement_exit counter or generalize the counting logic.
  • dashboard/index.html.erb -- isWin classification at line 592 will misclassify profitable pre-settlement exits as losses (red pill). The pill text will render correctly ("pre settlement exit") but the color will always be loss-colored.
  • volatility_bot.rb -- Has its own settlement logic (lines 116-127) but is currently disabled/self-managing. Not directly affected but same pattern exists.
  • dashboard_controller.rb -- Computes avg_sl and avg_tp from exit reasons (lines 87-90). Pre-settlement exits won't be included in either bucket, which is correct but means the analytics won't reflect them. Consider whether a new stat is needed.

Decomposition Assessment

With blast-radius additions: 4 file targets across 1 repo, 5 AC. All changes are tightly coupled (add exit path + update consumers of exit_reason). Estimated agent work: under 5 minutes. No decomposition needed.

Recommendations

  • [BODY] Fix file path: app/concerns/game_phase.rb → app/services/game_phase.rb
  • [BODY] Add app/services/scoreboard.rb to file targets -- must handle pre_settlement_exit in exit reason stats
  • [BODY] Add app/views/dashboard/index.html.erb to file targets -- isWin logic at line 592 must classify pre_settlement_exit by pnl sign, not by string match
  • [BODY] Note in file targets that PositionMonitor must include GamePhase to access game_nearly_over?
  • [BODY] Rephrase AC4 to be agent-verifiable: "Games where pre-settlement exit fires produce no settlement_loss exit reasons" instead of outcome metric
  • [SCOPE] Create user-stories section on project-botfire with bot-strategy story entry
  • [SCOPE] Create architecture note arch-bots for the bots component