feat: Add incbot command handlers #134
2 issues
code-review: Found 2 issues (1 medium, 1 low)
Medium
New on_title_changed hook lacks unit tests - `src/firetower/incidents/hooks.py:126-138`
The new on_title_changed function follows the same pattern as on_status_changed and on_severity_changed, which both have dedicated test classes in test_hooks.py. However, on_title_changed is not imported in the test file and has no corresponding tests. While it's mocked in test_channel_commands.py, this only verifies callers don't crash—it doesn't test the hook's actual behavior (posting messages, updating topics, handling missing Slack links).
Low
Non-atomic dual serializer updates may leave incident in inconsistent state - `src/firetower/slack_app/handlers/mitigated.py:92-115`
The handle_mitigated_submission function performs two sequential serializer.save() calls (status at line 101 followed by description at line 115) outside of a database transaction. If the first succeeds but the second fails, the incident will be marked as 'Mitigated' but the mitigation notes won't be appended to the description, leaving the incident in a partially-updated state with no record of the impact/action items that were collected.
Duration: 224.6s · Tokens: 1.5M in / 16.5k out · Cost: $2.64 (+extraction: $0.00, +merge: $0.00)
Annotations
Check warning on line 138 in src/firetower/incidents/hooks.py
github-actions / warden: code-review
New on_title_changed hook lacks unit tests
The new `on_title_changed` function follows the same pattern as `on_status_changed` and `on_severity_changed`, which both have dedicated test classes in `test_hooks.py`. However, `on_title_changed` is not imported in the test file and has no corresponding tests. While it's mocked in `test_channel_commands.py`, this only verifies callers don't crash—it doesn't test the hook's actual behavior (posting messages, updating topics, handling missing Slack links).