Skip to content

Fix visibility change early return and dualreview cleanup

9ef30aa
Select commit
Loading
Failed to load commit list.
Sign in for the full log view
Closed

feat: Add Linear action item syncing for incidents #139

Fix visibility change early return and dualreview cleanup
9ef30aa
Select commit
Loading
Failed to load commit list.
GitHub Actions / warden: code-review completed Apr 30, 2026 in 2m 48s

4 issues

code-review: Found 4 issues (3 medium, 1 low)

Medium

Module-level LinearService singleton retains stale workflow state cache across incidents - `src/firetower/incidents/services.py:232`

_get_linear_service() returns a process-wide singleton whose _workflow_states_cache is populated on first call and never invalidated. If a Linear admin renames or adds workflow states, every Firetower process will keep using stale state IDs (potentially writing to a nonexistent state) until the worker is restarted. This is the service used in sync_action_items_from_linear at line 232.

Also found at:

  • src/firetower/incidents/services.py:198-209
GET request triggers external Linear sync as a side effect - `src/firetower/incidents/views.py:343-351`

ActionItemListView.list() calls sync_action_items_from_linear(incident) on every GET request. Performing write/sync side effects on a GET violates HTTP semantics and can cause unintended behavioral changes (extra Linear API calls, DB writes) whenever clients, crawlers, or retries hit the endpoint. Although described as throttled, the sync is still invoked synchronously inside the request path, which can also slow down list responses and hide Linear outages from users when failures are only logged.

Also found at:

  • src/firetower/incidents/services.py:240-251
Potential N+1 on assignee userprofile in ActionItemSerializer - `src/firetower/incidents/views.py:332-333`

get_queryset only select_related's assignee__userprofile. If ActionItemSerializer references other related fields (e.g. created_by, linear metadata, or many-to-many relations), each serialized item will issue extra queries, producing an N+1 pattern on the list endpoint. Consider adding the necessary select_related/prefetch_related based on the serializer's fields.

Also found at:

  • src/firetower/incidents/services.py:174-179

Low

Broad except may mask programming errors and leak as 500 - `src/firetower/incidents/views.py:372-390`

SyncActionItemsView.post catches bare Exception around sync_action_items_from_linear and returns a generic 500. This will swallow non-Linear errors (e.g. TypeError, permission/programming bugs) and report them as Linear failures, making debugging harder. Consider catching the specific Linear/integration exceptions raised by the service and letting unexpected errors propagate to Django's error handling.


Duration: 162.0s · Tokens: 516.7k in / 3.4k out · Cost: $2.79 (+merge: $0.00)

Annotations

Check warning on line 232 in src/firetower/incidents/services.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

Module-level LinearService singleton retains stale workflow state cache across incidents

`_get_linear_service()` returns a process-wide singleton whose `_workflow_states_cache` is populated on first call and never invalidated. If a Linear admin renames or adds workflow states, every Firetower process will keep using stale state IDs (potentially writing to a nonexistent state) until the worker is restarted. This is the service used in `sync_action_items_from_linear` at line 232.

Check warning on line 209 in src/firetower/incidents/services.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[GKG-DR8] Module-level LinearService singleton retains stale workflow state cache across incidents (additional location)

`_get_linear_service()` returns a process-wide singleton whose `_workflow_states_cache` is populated on first call and never invalidated. If a Linear admin renames or adds workflow states, every Firetower process will keep using stale state IDs (potentially writing to a nonexistent state) until the worker is restarted. This is the service used in `sync_action_items_from_linear` at line 232.

Check warning on line 351 in src/firetower/incidents/views.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

GET request triggers external Linear sync as a side effect

ActionItemListView.list() calls sync_action_items_from_linear(incident) on every GET request. Performing write/sync side effects on a GET violates HTTP semantics and can cause unintended behavioral changes (extra Linear API calls, DB writes) whenever clients, crawlers, or retries hit the endpoint. Although described as throttled, the sync is still invoked synchronously inside the request path, which can also slow down list responses and hide Linear outages from users when failures are only logged.

Check warning on line 251 in src/firetower/incidents/services.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[4BF-NUS] GET request triggers external Linear sync as a side effect (additional location)

ActionItemListView.list() calls sync_action_items_from_linear(incident) on every GET request. Performing write/sync side effects on a GET violates HTTP semantics and can cause unintended behavioral changes (extra Linear API calls, DB writes) whenever clients, crawlers, or retries hit the endpoint. Although described as throttled, the sync is still invoked synchronously inside the request path, which can also slow down list responses and hide Linear outages from users when failures are only logged.

Check warning on line 333 in src/firetower/incidents/views.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

Potential N+1 on assignee userprofile in ActionItemSerializer

get_queryset only select_related's assignee__userprofile. If ActionItemSerializer references other related fields (e.g. created_by, linear metadata, or many-to-many relations), each serialized item will issue extra queries, producing an N+1 pattern on the list endpoint. Consider adding the necessary select_related/prefetch_related based on the serializer's fields.

Check warning on line 179 in src/firetower/incidents/services.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[SX5-52K] Potential N+1 on assignee userprofile in ActionItemSerializer (additional location)

get_queryset only select_related's assignee__userprofile. If ActionItemSerializer references other related fields (e.g. created_by, linear metadata, or many-to-many relations), each serialized item will issue extra queries, producing an N+1 pattern on the list endpoint. Consider adding the necessary select_related/prefetch_related based on the serializer's fields.