jihoson/The-Ouroboros
1
0
Fork 0
Code Issues 14 Pull Requests 1 Actions 47 Packages Projects Releases Wiki Activity

fix: SELL outcome PnL uses sell quantity (#322) #337

Merged
jihoson merged 2 commits from feature/issue-322-sell-pnl-sell-qty into feature/v3-session-policy-stream 2026-02-28 18:21:34 +09:00
Conversation 3 Commits 2 Files Changed 2 +33 -4
agentson commented 2026-02-28 14:38:17 +09:00
Collaborator
Copy Link

Summary

  • Change SELL outcome PnL calculation to use actual sell quantity, not latest BUY quantity
  • Cover partial-fill scenario in main loop test

Validation

  • pytest -q tests/test_main.py -k "sell_order_uses_broker_balance_qty_not_db or sell_updates_original_buy_decision_outcome"
## Summary - Change SELL outcome PnL calculation to use actual sell quantity, not latest BUY quantity - Cover partial-fill scenario in main loop test ## Validation - pytest -q tests/test_main.py -k "sell_order_uses_broker_balance_qty_not_db or sell_updates_original_buy_decision_outcome"
agentson added 1 commit 2026-02-28 14:38:17 +09:00
fix: compute SELL decision outcome using sell quantity (#322)
Some checks failed
Gitea CI / test (pull_request) Waiting to run
Details
Gitea CI / test (push) Has been cancelled
Details
6d7e6557d2
agentson commented 2026-02-28 16:12:00 +09:00
Author
Collaborator
Copy Link

Ticket Maturity Update (2026-02-28)

  • Implemented: done
  • Integrated: done
  • Observed: done (targeted test commands executed and passing)
  • Accepted: pending (reviewer + runtime verifier approval required)

Coverage Matrix:

Stage Status
Implemented PASS
Integrated PASS
Observed PASS
Accepted NOT_OBSERVED
## Ticket Maturity Update (2026-02-28) - Implemented: done - Integrated: done - Observed: done (targeted test commands executed and passing) - Accepted: pending (reviewer + runtime verifier approval required) Coverage Matrix: | Stage | Status | | --- | --- | | Implemented | PASS | | Integrated | PASS | | Observed | PASS | | Accepted | NOT_OBSERVED |
agentson reviewed 2026-02-28 16:15:35 +09:00
agentson left a comment
Author
Collaborator
Copy Link

Review: PR #337 — fix: SELL outcome PnL uses sell quantity (#322)

플랜 일치: ✅ ACT-05 정확히 일치

  • trade_pnl = (trade_price - buy_price) * sell_qty 로 변경

코드 품질: ✅ 정확

  • 실시간(trading_cycle) + 일간(run_daily_session) 2곳 모두 수정
  • sell_qty = int(quantity or 0) — quantity가 None인 경우 0으로 폴백

테스트: ⚠️ 부족

  • 기존 테스트에 assertion 3줄 추가만 됨 (기존 test_sell_order_uses_broker_balance_qty_not_db에 PnL 검증 추가)
  • ACT-05 테스트 계획에 명시된 케이스 중 누락:
    • ❌ 부분 매도 전용 테스트 (buy_qty=100, sell_qty=30 시나리오)
    • ❌ 전량 매도 동일 결과 보장 테스트
    • ❌ quantity=None 엣지 케이스 (sell_qty=0이 되어 PnL=0, 의도된 동작인지 불명확)

우려사항

  • int(quantity or 0) — quantity가 None일 때 sell_qty=0이 되어 PnL이 항상 0이 됨. 이 경우 buy_qty 폴백이 더 안전할 수 있음. 명시적 방어 로직 또는 경고 로그 권장.

결론: Changes Requested — 부분 매도 테스트 추가 + quantity=None 케이스 방어 로직 필요

## Review: PR #337 — fix: SELL outcome PnL uses sell quantity (#322) ### 플랜 일치: ✅ ACT-05 정확히 일치 - `trade_pnl = (trade_price - buy_price) * sell_qty` 로 변경 ### 코드 품질: ✅ 정확 - 실시간(trading_cycle) + 일간(run_daily_session) 2곳 모두 수정 - `sell_qty = int(quantity or 0)` — quantity가 None인 경우 0으로 폴백 ### 테스트: ⚠️ 부족 - 기존 테스트에 assertion 3줄 추가만 됨 (기존 test_sell_order_uses_broker_balance_qty_not_db에 PnL 검증 추가) - ACT-05 테스트 계획에 명시된 케이스 중 누락: - ❌ 부분 매도 전용 테스트 (buy_qty=100, sell_qty=30 시나리오) - ❌ 전량 매도 동일 결과 보장 테스트 - ❌ quantity=None 엣지 케이스 (sell_qty=0이 되어 PnL=0, 의도된 동작인지 불명확) ### 우려사항 - `int(quantity or 0)` — quantity가 None일 때 `sell_qty=0`이 되어 PnL이 항상 0이 됨. 이 경우 buy_qty 폴백이 더 안전할 수 있음. 명시적 방어 로직 또는 경고 로그 권장. ### 결론: **Changes Requested** — 부분 매도 테스트 추가 + quantity=None 케이스 방어 로직 필요
agentson added 1 commit 2026-02-28 17:13:57 +09:00
test: add sell qty fallback guard and quantity-basis coverage (#322)
Some checks are pending
Gitea CI / test (push) Waiting to run
Details
Gitea CI / test (pull_request) Waiting to run
Details
fd0246769a
agentson commented 2026-02-28 17:14:02 +09:00
Author
Collaborator
Copy Link

리뷰 반영 완료했습니다.

변경 사항:

  • SELL PnL 수량 계산에 안전 폴백 추가: sell_qty가 비정상(None/0)일 때 buy_qty 폴백
  • _resolve_sell_qty_for_pnl() 헬퍼 추가로 수량 기준 로직을 명시화
  • 테스트 보강:
    • 부분 매도 우선 수량 테스트
    • 전량/기본 폴백 수량 테스트
    • 둘 다 미존재 시 0 처리 테스트
    • 기존 test_sell_order_uses_broker_balance_qty_not_db/test_sell_updates_original_buy_decision_outcome 유지

검증:

  • pytest -q tests/test_main.py -k "resolve_sell_qty_for_pnl or sell_order_uses_broker_balance_qty_not_db or sell_updates_original_buy_decision_outcome"
  • 결과: 5 passed
리뷰 반영 완료했습니다. 변경 사항: - SELL PnL 수량 계산에 안전 폴백 추가: `sell_qty`가 비정상(None/0)일 때 `buy_qty` 폴백 - `_resolve_sell_qty_for_pnl()` 헬퍼 추가로 수량 기준 로직을 명시화 - 테스트 보강: - 부분 매도 우선 수량 테스트 - 전량/기본 폴백 수량 테스트 - 둘 다 미존재 시 0 처리 테스트 - 기존 `test_sell_order_uses_broker_balance_qty_not_db`/`test_sell_updates_original_buy_decision_outcome` 유지 검증: - `pytest -q tests/test_main.py -k "resolve_sell_qty_for_pnl or sell_order_uses_broker_balance_qty_not_db or sell_updates_original_buy_decision_outcome"` - 결과: 5 passed
agentson reviewed 2026-02-28 17:42:05 +09:00
agentson left a comment
Author
Collaborator
Copy Link

Re-review: PR #337 — fix: SELL outcome PnL uses sell quantity (#322)

이전 리뷰 지적사항 반영 확인

  1. quantity=None 방어 로직: ✅ 해결

    • _resolve_sell_qty_for_pnl() 헬퍼 함수 추가
    • sell_qty > 0이면 사용, 아니면 buy_qty 폴백, 둘 다 없으면 0 반환
    • 이전 int(quantity or 0)의 무조건 0 문제 해결됨

  2. 부분 매도 전용 테스트: ✅ 추가됨

    • test_resolve_sell_qty_for_pnl_prefers_sell_qty (sell_qty=30, buy_qty=100 → 30)
    • test_resolve_sell_qty_for_pnl_uses_buy_qty_fallback_when_sell_qty_missing (None → buy_qty 폴백)
    • test_resolve_sell_qty_for_pnl_returns_zero_when_both_missing (둘 다 None → 0)

  3. 기존 테스트 PnL 검증: ✅ 유지

    • test_sell_order_uses_broker_balance_qty_not_db에 outcome_pnl 검증 추가

코드 품질

  • 헬퍼 함수 분리로 테스트 가능성 향상, 실시간/일간 2곳 모두 동일 함수 사용

결론: LGTM ✅

## Re-review: PR #337 — fix: SELL outcome PnL uses sell quantity (#322) ### 이전 리뷰 지적사항 반영 확인 1. **quantity=None 방어 로직**: ✅ 해결 - `_resolve_sell_qty_for_pnl()` 헬퍼 함수 추가 - sell_qty > 0이면 사용, 아니면 buy_qty 폴백, 둘 다 없으면 0 반환 - 이전 `int(quantity or 0)`의 무조건 0 문제 해결됨 2. **부분 매도 전용 테스트**: ✅ 추가됨 - `test_resolve_sell_qty_for_pnl_prefers_sell_qty` (sell_qty=30, buy_qty=100 → 30) - `test_resolve_sell_qty_for_pnl_uses_buy_qty_fallback_when_sell_qty_missing` (None → buy_qty 폴백) - `test_resolve_sell_qty_for_pnl_returns_zero_when_both_missing` (둘 다 None → 0) 3. **기존 테스트 PnL 검증**: ✅ 유지 - `test_sell_order_uses_broker_balance_qty_not_db`에 outcome_pnl 검증 추가 ### 코드 품질 - 헬퍼 함수 분리로 테스트 가능성 향상, 실시간/일간 2곳 모두 동일 함수 사용 ### 결론: **LGTM** ✅
jihoson merged commit 3b135c3080 into feature/v3-session-policy-stream 2026-02-28 18:21:34 +09:00
jihoson deleted branch feature/issue-322-sell-pnl-sell-qty 2026-02-28 18:21:34 +09:00
Sign in to join this conversation.
Reviewers
No Reviewers
agentson
Labels
Clear labels
bug
Something is not working
 duplicate
This issue or pull request already exists
 enhancement
New feature
 help wanted
Need some help
 invalid
Something is wrong
 question
More information is needed
 wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agentson jihoson
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jihoson/The-Ouroboros#337
Delete Branch "feature/issue-322-sell-pnl-sell-qty"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?