Fix critical gameplay issues and add input logging

[MineForeman]

Summary

• Fixed game lockup issue that occurred when player movement was blocked
• Implemented comprehensive keystroke logging for better debugging
• Fixed multiple combat and inventory system bugs

Changes

Bug Fixes

• Movement lockup fix: Added immediate movement processing after queueing to prevent turn freezing
• Duplicate combat messages: Removed redundant logging in combat update loop
• Combat after death: Added dead entity removal after combat processing
• Dropped items invisible: Fixed event processing and component restoration for dropped items
• Invisible monsters in corridors: Fixed FOV propagation to render system

New Features

• Input logging system: Complete keystroke logging with action mapping to veyrm_input.log
• Inventory key bindings: Context-aware lowercase key support (d/D for drop, e/E/x/X for examine)

Test Plan

• Verified movement no longer locks up when blocked
• Confirmed combat messages appear only once
• Tested that dead entities cannot attack
• Verified dropped items appear on map
• Checked monsters are visible in corridors
• Tested inventory key bindings work with lowercase
• Verified input logging captures all keystrokes

🤖 Generated with Claude Code

[claude]

Claude finished @MineForeman's task —— View job

---

Code Review Complete

Summary

This is a substantial and well-executed pull request that addresses critical gameplay bugs while introducing comprehensive input logging. The architectural transition to ECS appears solid with good separation of concerns.

🔍 Code Quality & Best Practices

✅ Strengths

• Well-documented code: Comprehensive Doxygen comments throughout ECS system
• Clean architecture: Proper ECS separation with components, systems, and entities
• Consistent patterns: Good use of RAII, const correctness, and modern C++ features
• Defensive programming: Proper null checks and validation in critical paths

⚠️ Areas for Improvement

• Deep call stacks: Some functions like GameWorld::processQueue() could benefit from decomposition
• Include organization: Mixed global/namespace includes could be standardized
• Error handling: Some functions return bool for success/failure but don't provide error details

🐛 Bug Fixes Analysis

✅ Well-Addressed Issues

1. Movement lockup fix (src/ecs/movement_system.cpp:39): Immediate processing prevents turn freeze
2. Combat after death (src/ecs/combat_system.cpp:25): Proper dead entity handling
3. Invisible items fix (src/ecs/game_world.cpp:94): Event system restores position components correctly

🔍 Potential Issues

• Input command queuing (include/ecs/input_system.h:58): Queue could grow unbounded without rate limiting
• Entity lookup performance (src/ecs/movement_system.cpp:109): Linear search through entities for each lookup

🔒 Security Concerns

⚠️ Input Logging Security

• Keystroke logging (src/input_handler.cpp:61): All keystrokes logged to veyrm_input.log
  • Risk: Could capture sensitive data if passwords/secrets entered
  • Recommendation: Add filtering for sensitive contexts or implement log rotation
• Log file permissions: No evidence of restricted file permissions for log files
• Memory safety: ECS system uses smart pointers appropriately, reducing memory corruption risks

🔍 Injection Risks

• JSON parsing (data/monsters.json, data/items.json): Need validation to prevent malformed data attacks
• File path handling: Log file paths appear to be hardcoded, reducing path traversal risk

⚡ Performance Considerations

✅ Good Patterns

• System priorities (include/ecs/input_system.h:134): Proper system ordering for performance
• Component pools: ECS uses efficient component storage patterns
• Move semantics: Appropriate use of std::move in entity transfers

⚠️ Performance Concerns

• Entity iteration: Multiple linear searches through entity vectors per frame
• String operations: Heavy string construction in logging paths (src/input_handler.cpp:62)
• Memory allocations: Frequent queue operations in movement system

💡 Recommendations

1. Consider entity indexing/hashing for faster lookups
2. Implement log level filtering to reduce string operations
3. Profile frame time to identify bottlenecks

🧪 Test Coverage

✅ Well-Covered Areas

• Input system tests (tests/test_input_handler.cpp): Good coverage of key bindings
• ECS integration tests (tests/test_ecs_integration.cpp): Component interaction testing
• Map generation tests: Multiple test files for map validation

❌ Missing Coverage

• Input logging functionality: No tests for keystroke logging feature
• Event system: Drop events and other ECS events lack dedicated tests
• Error paths: Limited testing of failure scenarios in combat/movement

📝 Test Requirement Compliance

• CLAUDE.md requirement: "ALL tests must pass before marking any task complete"
• Status: Unable to verify test execution due to tool permissions
• Recommendation: Ensure ./build.sh test passes before merge

🔄 Architecture Assessment

✅ ECS Implementation

• Clean separation: Components are data-only, systems contain logic
• Proper dependencies: Systems properly inject dependencies via constructor
• Event system: Good decoupling through event-driven architecture

🏗️ Migration Quality

• Legacy removal: Old entity/monster/item classes properly removed
• Bridge elimination: Monster AI and spawn manager bridges cleaned up
• Consistent patterns: New ECS code follows established patterns

📊 Overall Assessment

🎯 Recommendation: APPROVE with Minor Changes

This PR successfully addresses the critical gameplay issues while introducing valuable debugging capabilities. The code quality is high and the architectural improvements are solid. The few concerns identified are minor and don't block the core functionality.

🔄 Suggested Pre-Merge Actions

1. Add input filtering to prevent sensitive data logging
2. Verify all tests pass with ./build.sh test
3. Consider adding tests for the new input logging feature
4. Review log file permissions and rotation policies

📈 Future Considerations

• Performance profiling of entity lookup patterns
• Comprehensive error handling strategy
• Security audit of logging subsystem

Great work on this substantial refactoring while maintaining stability! 🚀

---

[claude]

Claude finished @MineForeman's task —— View job

---

Comprehensive Pull Request Review

I've conducted a thorough analysis of this substantial PR covering 100 files with 16,593 additions and 7,736 deletions. Here's my comprehensive feedback on the requested areas:

Review Checklist:

• Analyze code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide constructive feedback and recommendations

---

🔍 Code Quality & Best Practices

✅ Strengths

• Excellent ECS Architecture: Clean separation between components, systems, and entities with proper dependency injection
• Comprehensive Doxygen Documentation: Well-documented classes and methods throughout the ECS system
• Modern C++ Patterns: Good use of smart pointers, RAII, const correctness, and [[maybe_unused]] attributes
• Consistent Naming: Clear naming conventions across components and systems
• Proper Component Design: Components are data-only with systems containing logic

⚠️ Areas for Improvement

• Include Organization: Mix of global and namespace includes could be standardized (src/ecs/game_world.cpp:6-33)
• Deep Call Stacks: Some functions like GameWorld::processQueue() could benefit from decomposition
• Inconsistent Error Handling: Some functions return bool for success/failure without error details

🐛 Potential Bugs & Issues

✅ Critical Fixes Implemented

1. Movement lockup fix (src/ecs/movement_system.cpp:39): Immediate processing prevents turn freeze ✅
2. Combat after death (src/ecs/combat_system.cpp:25): Proper dead entity removal ✅
3. Invisible items fix (src/ecs/game_world.cpp:94): Event system restores position components ✅

⚠️ Potential Issues Identified

• Input Queue Growth (include/ecs/input_system.h:58): Command queue could grow unbounded without rate limiting
• Entity Lookup Performance (src/ecs/movement_system.cpp:109): Linear search through entities for each lookup
• Shared Pointer Safety (src/ecs/combat_system.cpp:220): Non-owning shared_ptr creation with custom deleter could be risky

⚡ Performance Considerations

✅ Good Patterns

• System Priorities (include/ecs/input_system.h:134): Proper system ordering for optimal performance
• Component Pools: ECS uses efficient component storage patterns
• Move Semantics: Appropriate use of std::move in entity transfers

⚠️ Performance Concerns

• Multiple Entity Iterations: Systems perform linear searches through entity vectors each frame
• String Operations: Heavy string construction in logging paths (src/input_handler.cpp:62)
• Memory Allocations: Frequent queue operations in movement system could cause allocations

💡 Recommendations

1. Consider entity indexing/spatial hashing for faster position-based lookups
2. Implement log level filtering to reduce string operations
3. Profile frame time to identify actual bottlenecks

🔒 Security Concerns

⚠️ Input Logging Security Risk

• Comprehensive Keystroke Logging (src/input_handler.cpp:61): ALL keystrokes logged to veyrm_input.log
  • Risk: Could inadvertently capture sensitive data if passwords/secrets entered
  • Recommendation: Add filtering for sensitive contexts or implement log rotation
• Log File Permissions: No evidence of restricted file permissions for sensitive log files

✅ Good Security Practices

• Memory Safety: ECS system uses smart pointers appropriately, reducing memory corruption risks
• Input Validation: JSON parsing appears to have proper structure validation
• No Injection Risks: File paths are hardcoded, reducing path traversal vulnerabilities

🔧 Security Recommendations

1. Add log file rotation and permission restrictions
2. Implement input filtering to prevent logging sensitive data
3. Consider adding data validation for JSON configuration files

🧪 Test Coverage Assessment

✅ Well-Covered Areas

• Input System: tests/test_input_handler.cpp covers key bindings and mappings
• ECS Integration: tests/test_ecs_integration.cpp tests component interactions
• Core Systems: Multiple test files for map generation, FOV, and data loading

❌ Missing Coverage

• Input Logging Feature: No tests for the new keystroke logging functionality
• Event System: Drop events and ECS event handling lack dedicated tests
• Error Paths: Limited testing of failure scenarios in combat/movement systems
• Security: No tests for log file permissions or input filtering

📝 Test Compliance

• CLAUDE.md Requirement: "ALL tests must pass before marking any task complete"
• Status: Cannot verify test execution due to review-only context
• Recommendation: Ensure ./build.sh test passes before merge

📊 Overall Assessment

🎯 Recommendation: APPROVE with Minor Security Enhancements

This is an excellent PR that successfully addresses critical gameplay issues while introducing valuable debugging capabilities. The architectural migration to ECS is well-executed with high code quality.

🔄 Pre-Merge Recommendations

1. Security: Add input filtering to prevent sensitive data logging
2. Testing: Verify all tests pass with ./build.sh test
3. Documentation: Consider adding tests for the new input logging feature
4. Performance: Review entity lookup patterns if performance issues arise

📈 Future Considerations

• Performance profiling of entity search patterns
• Comprehensive security audit of logging subsystem
• Enhanced error handling strategy across systems

Excellent work on this substantial refactoring while maintaining system stability! 🚀

---