Skip to content

feat(incidents): Add SLO nag for stale P0/P1 action items#230

Open
taylor-osler-sentry wants to merge 7 commits into
mainfrom
feat/slo-nag
Open

feat(incidents): Add SLO nag for stale P0/P1 action items#230
taylor-osler-sentry wants to merge 7 commits into
mainfrom
feat/slo-nag

Conversation

@taylor-osler-sentry
Copy link
Copy Markdown
Contributor

@taylor-osler-sentry taylor-osler-sentry commented Jun 1, 2026

Summary

  • Adds last_nag DateTimeField to ActionItem for throttling reminder notifications
  • Adds a send_action_item_reminder django-q schedule (every 30 min) that syncs Linear action items for P0/P1 incidents aged 21-90 days and filters down to Urgent/High items eligible for a nag
  • Test coverage for the age window, force-refresh, priority filtering, and per-incident sync failure tolerance

Test plan

  • Run pytest src/firetower/incidents/tests/test_tasks.py::TestSendActionItemReminder
  • Apply migrations locally and confirm last_nag column is added
  • Verify the django-q schedule is created on migrate
  • Remember to update the secret values in prod

Test action item showing a comment: https://linear.app/getsentry/issue/RELENG-797/test-action-item

🤖 Generated with Claude Code

…ling

Adds `last_nag` DateTimeField to track when an action item was last
surfaced to its owner, so the upcoming SLO nag task can throttle
reminders to every `ACTION_ITEM_REMINDER_NAG_EVERY_DAYS` days.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Agent transcript: https://claudescope.sentry.dev/share/1r7-Ige7a9lDONGmhCRfAbT2RaFS5j2E-AOIGjxgG1A
Comment thread src/firetower/incidents/tests/test_tasks.py Outdated
Comment thread src/firetower/incidents/tests/test_tasks.py
Comment thread src/firetower/incidents/tasks.py
Copy link
Copy Markdown

@sentry-warden sentry-warden Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests for send_action_item_reminder are absent despite PR description claiming coverage

The PR description says there is test coverage for age window, force-refresh, priority filtering, and per-incident sync failure tolerance, but TestSendActionItemReminder and send_action_item_reminder do not appear anywhere in test_tasks.py.

Evidence
  • test_tasks.py imports only schedule_demo, send_statuspage_followup_reminder, send_statuspage_reminder, and datadog_logsend_action_item_reminder is not imported.
  • Reading the full test file reveals no TestSendActionItemReminder class.
  • The test plan in the PR body calls pytest test_tasks.py::TestSendActionItemReminder, which would fail with a collection error.
  • The last_nag-is-never-updated bug above would also not be caught without tests exercising the post-notification state.

Identified by Warden code-review

Comment thread src/firetower/incidents/tasks.py
Comment thread src/firetower/incidents/tasks.py
Wires `_nag` up to `LinearService.create_comment`, posting the new
`linear.action_item_nag_comment` config string as a comment on the
Linear issue and stamping `last_nag` only on success so failures
remain retriable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Agent transcript: https://claudescope.sentry.dev/share/X7voUvjVhAXYxD_25oenlNeLubFTdZh_3NGWU7n0fV8
Copy link
Copy Markdown

@sentry-warden sentry-warden Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestSendActionItemReminder tests referenced in PR but absent from test file

The PR description and test plan both reference pytest src/firetower/incidents/tests/test_tasks.py::TestSendActionItemReminder, but that class doesn't exist in the test file — the new send_action_item_reminder task has zero test coverage.

Evidence
  • test_tasks.py contains TestDatadogLogTaskName, TestDatadogLogStatsdIncrements, TestScheduleDemoPrivateIncident, TestSendStatuspageReminder, and TestSendStatuspageFollowupReminder, but no TestSendActionItemReminder.
  • A grep for TestSendActionItemReminder across the test directory returns no matches.
  • The task has several branching conditions (age window, priority filter, last_nag throttle, per-incident sync failure tolerance) that the PR description claims are covered.
  • test_tasks.py is not in the list of changed files, confirming no test additions were made.

Identified by Warden code-review

Comment thread src/firetower/incidents/tasks.py
Comment thread src/firetower/incidents/tasks.py
Comment thread src/firetower/incidents/tests/test_tasks.py
Parametrizes a test across Linear priorities 0/3/4 to lock in that
non-P0/P1 action items are filtered out before `_nag` is ever reached.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Agent transcript: https://claudescope.sentry.dev/share/LyuDB0moEsT4eGO1ZHMkQ0Jt0tgOFEEwB-HNw18VRtU
@taylor-osler-sentry taylor-osler-sentry marked this pull request as ready for review June 1, 2026 21:19
@taylor-osler-sentry taylor-osler-sentry requested a review from a team as a code owner June 1, 2026 21:19
Comment thread src/firetower/incidents/tasks.py
Comment thread src/firetower/incidents/tasks.py
Comment thread src/firetower/incidents/tasks.py
Moves the `ACTION_ITEM_NAG_COMMENT` check above the incident query so
the task exits immediately instead of fanning out Linear syncs only to
discard the result inside `_nag`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Agent transcript: https://claudescope.sentry.dev/share/O83EiO556xn-0ssKPHTg7TfGB_1Tce1jQmaHJ5IQL3s

for incident in incidents:
try:
sync_action_items_from_linear(incident)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The call to sync_action_items_from_linear is missing force=True, which can cause the sync to be skipped due to throttling, leading to reminders based on stale data.
Severity: MEDIUM

Suggested Fix

To ensure the task always operates on the freshest data, call the sync function with the force=True parameter: sync_action_items_from_linear(incident, force=True). This bypasses the throttle and guarantees data is synchronized before any reminders are evaluated, aligning with the behavior of other callers that require up-to-date information.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/firetower/incidents/tasks.py#L302

Potential issue: The `send_action_item_reminder` task calls
`sync_action_items_from_linear(incident)` without the `force=True` parameter. This
function includes a throttling mechanism that will silently skip the data
synchronization if it has been run recently. As a result, the task may operate on stale
action item data, potentially sending unnecessary reminder notifications for items that
have already been completed or updated in the source system. This behavior is missed by
tests because the `sync_action_items_from_linear` function is mocked, preventing the
throttling logic from being exercised.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1cfa517. Configure here.

Comment thread .gitignore
.env.production.local
.warden/logs/

./claude
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gitignore entry uses invalid syntax, won't match

Low Severity

The .gitignore entry ./claude uses a ./ prefix which is not valid .gitignore syntax. Git internally represents paths without ./, so this pattern will never match any file or directory. If the intent is to ignore the Claude Code configuration directory, the correct pattern is .claude. If it's a plain claude directory, the correct pattern is claude or /claude.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1cfa517. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant