Re-Review: Pre-settlement exit to avoid catastrophic losses

Verdict: APPROVED

Previous Issue Resolution

• [x] Wrong file path -- Fixed: issue now says app/services/game_phase.rb with explicit note "(note: this is in app/services/, NOT app/concerns/)"
• [x] Missing blast-radius files -- Fixed: scoreboard.rb and dashboard/index.html.erb now in File Targets with clear change descriptions
• [x] Missing include -- Fixed: first file target notes "must also include GamePhase (currently not included, but game_nearly_over? is needed from that module)"
• [x] Non-testable AC -- Fixed: old AC4 (outcome metric "settlement loss rate drops to near zero") replaced with two verifiable ACs: scoreboard counting and dashboard styling
• [x] Traceability notes -- Fixed: project-botfire user-stories section exists with story:bot-strategy entry; arch-bots note exists (Architecture: BotFire Bot Engine, status=active)

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 settlement loss data ($2.97 catastrophic loss, TP $0.59 vs settlement win $0.33)
• [x] File Targets -- 4 modify + 1 do-not-touch, all verified against codebase
• [x] Feature Flag -- none
• [x] Acceptance Criteria -- 5 verifiable criteria
• [x] Test Expectations -- 3 unit tests + run command
• [x] Constraints -- 5 constraints including include requirement and downstream consumer note
• [x] Checklist -- present
• [x] Related -- present

Traceability

• [x] story:bot-strategy label -- present on board item
• [x] story note verified -- project-botfire user-stories section contains bot-strategy entry
• [x] arch:bots label -- present on board item
• [x] arch note verified -- arch-bots note exists in pal-e-docs (Architecture: BotFire Bot Engine, covers 7 services, key files, order pattern, data model, infrastructure)
• [x] Forgejo issue -- ldraney/botfire#13, open

File Targets

• [x] app/services/position_monitor.rb -- verified: exists, has evaluate_exit (line 48) and resolve_settlement (line 69). Does NOT currently include GamePhase. Correct target for pre-settlement exit check.
• [x] app/services/game_phase.rb -- verified: exists at app/services/ (no app/concerns/ directory in project). Has game_nearly_over? method. Correct path.
• [x] app/services/scoreboard.rb -- verified: lines 35-36 only count stop_loss and take_profit. New exit reason would be invisible in stats. Correct target.
• [x] app/views/dashboard/index.html.erb -- verified: line 592 has isWin = f.exit_reason?.includes('win') || f.exit_reason?.includes('take'). A profitable pre_settlement_exit would show as loss pill. Correct target.
• [x] app/services/bot_engine.rb -- verified: exists. Correctly listed as do-not-touch (entry logic separate from exit logic).

Repo Placement

Dependencies

• Spawned from Spike #11 (board item #1845, ldraney/botfire#11) -- in QA column, upstream work complete.
• Sibling items from same spike: #1846 (botfire#12, backlog), #1848 (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 must include GamePhase -- documented in issue File Targets.

Acceptance Criteria

• AC1: "Position monitor detects games approaching final settlement" -- testable via unit test with game_nearly_over? returning true
• AC2: "Open positions in near-settlement games are exited at market price" -- testable via unit test
• AC3: "Exit logged with exit_reason: 'pre_settlement_exit'" -- testable via assertion on BotFire record
• AC4: "Scoreboard correctly counts pre_settlement_exit in win/loss stats based on P&L sign" -- testable
• AC5: "Dashboard renders pre_settlement_exit with correct win/loss styling based on P&L" -- testable via DOM assertion

Blast Radius

• dashboard/index.html.erb line 326 -- Exit breakdown table has same isWin = reason.includes('win') || reason.includes('take') pattern. Not explicitly called out in issue, but the constraint "All downstream consumers of exit_reason must handle the new value" covers it, and an agent grepping for exit_reason will find both locations.
• dashboard_controller.rb lines 87-90 -- Computes avg_sl/avg_tp only for stop_loss/take_profit. Pre-settlement exits are a distinct category and correctly excluded from SL/TP averages. No change needed.
• volatility_bot.rb -- Has own exit logic but currently disabled. Not affected.
• volatility_bot_test.rb -- Tests check take_profit, stop_loss, time_stop exit reasons. New exit reason doesn't affect existing tests.

Decomposition Assessment

Recommendation