Fix SYSTEM_REVIEW.md - correct count of fixed issues (4/8, not 5/8)

Issues #2, #3, #4, #6 are still open:
- #2: CardDefinition field validation not added
- #3: end_turn() knockout processing not fixed
- #4: Win condition timing not fixed
- #6: Engine doesn't call process_knockout for status KOs

The refactor improved process_knockout() internals but didn't fix the
callers that should invoke it.
This commit is contained in:
Cal Corum 2026-01-26 10:08:58 -06:00
parent dd2cadf82d
commit 8af326ecee

View File

@ -9,7 +9,10 @@
## Executive Summary
The core engine has a solid foundation with good separation of concerns, comprehensive documentation, and thorough test coverage. The review identified **8 critical issues** - **5 have been fixed** as part of the Energy/Evolution Stack refactor. The remaining 3 critical issues and several medium-priority gaps still need attention.
The core engine has a solid foundation with good separation of concerns, comprehensive documentation, and thorough test coverage. The review identified **8 critical issues** - **4 have been fixed** as part of the Energy/Evolution Stack refactor. The remaining 4 critical issues and several medium-priority gaps still need attention.
**Fixed:** #1, #5, #7, #8
**Still Open:** #2 (model validation), #3 (end_turn knockouts), #4 (win condition timing), #6 (engine knockout processing)
---
@ -258,12 +261,15 @@ This catches all exceptions including programming errors, making debugging diffi
## Recommended Fix Priority
### Phase 1: Critical Fixes (Before Any Testing) - MOSTLY COMPLETE
### Phase 1: Critical Fixes (Before Any Testing) - 4/8 COMPLETE
1. ~~Fix `find_card_instance` to include `energy_zone`~~ DONE
2. ~~Fix energy attachment to store cards properly~~ DONE (full refactor to CardInstance)
2. Add CardDefinition field validation - STILL NEEDED
3. Fix knockout processing in `end_turn` - STILL NEEDED
4. Fix win condition check timing - STILL NEEDED
5. ~~Add confusion handling in attack execution~~ DONE
5. ~~Fix energy attachment to store cards properly~~ DONE (full refactor to CardInstance)
6. Fix engine to call `process_knockout()` for status KOs - STILL NEEDED
7. ~~Add confusion handling in attack execution~~ DONE
8. ~~Fix energy discard handler to move cards~~ DONE
### Phase 2: Functionality Gaps (Before Feature Complete)
6. Add `SelectPrizeAction` executor
@ -307,17 +313,21 @@ This catches all exceptions including programming errors, making debugging diffi
## Next Steps
1. ~~Review this document and prioritize fixes~~ DONE
2. Create GitHub issues for remaining critical items (issues #3, #4)
3. ~~Address Phase 1 fixes before continuing development~~ 5/5 DONE
2. Create GitHub issues for remaining critical items (#2, #3, #4, #6)
3. ~~Address Phase 1 fixes before continuing development~~ 4/8 DONE
4. ~~Update tests as fixes are implemented~~ DONE (766 tests)
5. Re-run system review after remaining fixes
## Change Log
### 2026-01-26 - Energy/Evolution Stack Refactor
**Commit:** 2b8fac4
**Commits:** 2b8fac4, dd2cadf
Fixed issues #1, #5, #7, #8 plus major enhancements:
Note: Issues #3 and #6 (knockout processing) were NOT fixed - we improved
`process_knockout()` to handle attached cards correctly, but the engine still
doesn't call it for status damage knockouts. That's a separate fix needed.
- `attached_energy` and `attached_tools` changed from `list[str]` to `list[CardInstance]`
- Added `cards_underneath` for evolution stack tracking
- Added `devolve` effect handler