fix: prevent system segments from being overwritten via change request drafts by srijantrpth · Pull Request #8298 · Flagsmith/flagsmith

srijantrpth · 2026-08-14T19:29:06Z

Thanks for submitting a PR! Please check the boxes below:

I have read the Contributing Guide.
I have added information to docs/ if required so people know about the feature.
I have filled in the "Changes" section below.
I have filled in the "How did you test this code" section below.

Changes

Added a guard clause in ChangeRequestCommitService._publish_segments (api/core/workflows_services.py) to prevent change request drafts from overwriting system segments (is_system_segment=True).

How did you test this code?

Added a new unit test (test_change_request_commit__system_segment_draft__raises_value_error) in tests/unit/features/workflows/core/test_unit_workflows_models.py to verify that attempting to commit a change request targeting a system segment correctly raises a ValueError.

…t drafts and add unit test

vercel · 2026-08-14T19:29:11Z

@srijantrpth is attempting to deploy a commit to the Flagsmith Team on Vercel.

A member of the Team first needs to authorize it.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

for more information, see https://pre-commit.ci

coderabbitai · 2026-08-14T19:29:30Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 087822f0-a9da-45d7-b0f4-c799d9adcc46

📥 Commits

Reviewing files that changed from the base of the PR and between 14e860c and c3c40a9.

📒 Files selected for processing (2)

api/core/workflows_services.py
api/tests/unit/features/workflows/core/test_unit_workflows_models.py

📝 Walkthrough

Walkthrough

The change request commit now runs within an atomic transaction. The segment publishing workflow rejects drafts that target system segments before revision creation or live-segment updates. A unit test verifies the ValueError and confirms that feature-state and change-request updates roll back.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🟡 Moderate · up to c3c40

The change correctly blocks system-segment drafts, but rejection can still trigger external updates before the database transaction commits, potentially causing inconsistent downstream state. The PR should address this transaction-boundary issue and the remaining formatting check before merge.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Create stacked PR
Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7d4c283a-be0a-47ee-8f1c-4983a6315816

📥 Commits

Reviewing files that changed from the base of the PR and between 14e860c and 0b5ab68.

📒 Files selected for processing (2)

api/core/workflows_services.py
api/tests/unit/features/workflows/core/test_unit_workflows_models.py

codecov · 2026-08-14T19:38:03Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.62%. Comparing base (14e860c) to head (3d0b1ba).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8298      +/-   ##
==========================================
- Coverage   98.76%   98.62%   -0.15%     
==========================================
  Files        1594     1594              
  Lines       63657    63667      +10     
==========================================
- Hits        62873    62793      -80     
- Misses        784      874      +90

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…on to test

for more information, see https://pre-commit.ci

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3549c7b1-2004-485c-80d0-cf7dfa1ca444

📥 Commits

Reviewing files that changed from the base of the PR and between 0b5ab68 and 00269fc.

📒 Files selected for processing (2)

api/core/workflows_services.py
api/tests/unit/features/workflows/core/test_unit_workflows_models.py

coderabbitai · 2026-08-14T19:51:31Z

    def __init__(self, change_request: "ChangeRequest") -> None:
        self.change_request = change_request
-
+    @transaction.atomic


🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Defer external side effects until commit succeeds.

The decorator on Line 23 now wraps _publish_environment_feature_versions() and _publish_segments(). The Celery .delay(...) calls and environment_feature_version_published.send(...) still run before the transaction commits. If the system-segment guard raises ValueError, database changes roll back, but broker messages and signal-receiver side effects can remain. Register these calls with transaction.on_commit() and bind each loop value per callback.

Thanks for the catch! Since _publish_environment_feature_versions handles core versioning logic outside the scope of this specific system segment security patch, I'll leave the transaction hooks as-is for now to avoid altering existing versioning behavior, but I appreciate the architectural insight.

@srijantrpth, understood. The system-segment protection can remain scoped to this patch.

The side-effect ordering risk remains after the new outer transaction. Would you like me to open a follow-up issue for deferring the Celery dispatches and environment_feature_version_published.send() calls with transaction.on_commit()?

🐇

🧠 Learnings used

Learnt from: khvn26 Repo: Flagsmith/flagsmith PR: 8042 File: api/trust_relationships/services.py:30-53 Timestamp: 2026-07-20T09:52:12.198Z Learning: In the Flagsmith Django API, do not rely on `ATOMIC_REQUESTS` being enabled (it is not used and will not be adopted). When reviewing code in the service layer (e.g., `services.py`), avoid assumptions that view-layer operations run inside an enclosing request transaction. If you see `transaction.atomic()` in service code, treat it as defining the transaction boundary itself (or explicitly justify nesting/interaction), and do not flag issues based solely on the absence/presence of a request-level transaction.

_{You are interacting with an AI system.}

Sure, opening a follow-up issue for that side-effect ordering risk would be a great proactive step!

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash set -euo pipefail repo="Flagsmith/flagsmith" body=$(cat <<'EOF' ## Summary Defer external side effects from change request publishing until the enclosing database transaction commits successfully. ## Rationale `ChangeRequestCommitService.commit()` now runs in `transaction.atomic`. `_publish_environment_feature_versions()` dispatches Celery tasks and sends `environment_feature_version_published` before this transaction commits. If a later operation fails, for example when `_publish_segments()` rejects a system segment, the database transaction rolls back but queued tasks and signal-receiver side effects can still run against data that was not committed. ## Affected area - `api/core/workflows_services.py` - `ChangeRequestCommitService._publish_environment_feature_versions` ## Required changes Register `trigger_update_version_webhooks.delay(...)`, `rebuild_environment_document.delay(...)`, and `environment_feature_version_published.send(...)` through `transaction.on_commit()`. Bind each loop value in its callback so callbacks use the correct environment feature version and scheduled timestamp. ## Acceptance criteria - No Celery task is dispatched when `ChangeRequestCommitService.commit()` rolls back. - No `environment_feature_version_published` receiver runs when `ChangeRequestCommitService.commit()` rolls back. - The existing side effects run after a successful transaction commit. - Tests cover both rollback and successful commit behaviour. ## Backlinks - Pull request: https://github.com/Flagsmith/flagsmith/pull/8298 - Review comment: https://github.com/Flagsmith/flagsmith/pull/8298#discussion_r3786731701 - Requested by: `@srijantrpth` EOF ) gh issue create \ --repo "$repo" \ --title "Defer change request publishing side effects until transaction commit" \ --body "$body" \ --assignee "srijantrpth"

Length of output: 2047

@srijantrpth, I created follow-up issue #8299: Defer change request publishing side effects until transaction commit.

🐇

🧠 Learnings used

Learnt from: khvn26 Repo: Flagsmith/flagsmith PR: 8042 File: api/trust_relationships/services.py:30-53 Timestamp: 2026-07-20T09:52:12.198Z Learning: In the Flagsmith Django API, do not rely on `ATOMIC_REQUESTS` being enabled (it is not used and will not be adopted). When reviewing code in the service layer (e.g., `services.py`), avoid assumptions that view-layer operations run inside an enclosing request transaction. If you see `transaction.atomic()` in service code, treat it as defining the transaction boundary itself (or explicitly justify nesting/interaction), and do not flag issues based solely on the absence/presence of a request-level transaction.

_{You are interacting with an AI system.}

coderabbitai · 2026-08-14T19:51:40Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

for more information, see https://pre-commit.ci

fix: prevent system segments from being overwritten via change reques…

f59da3a

…t drafts and add unit test

Copilot AI lite review requested due to automatic review settings August 14, 2026 19:29

srijantrpth requested a review from a team as a code owner August 14, 2026 19:29

srijantrpth requested review from khvn26 and removed request for a team August 14, 2026 19:29

Copilot AI reviewed Aug 14, 2026

[pre-commit.ci] auto fixes from pre-commit.com hooks

0b5ab68

for more information, see https://pre-commit.ci

github-actions Bot added the api Issue related to the REST API label Aug 14, 2026

coderabbitai Bot reviewed Aug 14, 2026

View reviewed changes

Comment thread api/core/workflows_services.py Outdated

Comment thread api/tests/unit/features/workflows/core/test_unit_workflows_models.py

srijantrpth and others added 3 commits August 14, 2026 19:46

fix: make change request commit fully atomic and add rollback asserti…

00269fc

…on to test

Merge branch 'main' into fix-8269-system-segments

14137a2

[pre-commit.ci] auto fixes from pre-commit.com hooks

c3c40a9

for more information, see https://pre-commit.ci

coderabbitai Bot reviewed Aug 14, 2026

View reviewed changes

srijantrpth and others added 2 commits August 14, 2026 19:58

style: add trailing newline to pass ruff linter

3e15279

[pre-commit.ci] auto fixes from pre-commit.com hooks

3d0b1ba

for more information, see https://pre-commit.ci

coderabbitai Bot mentioned this pull request Aug 14, 2026

Defer change request publishing side effects until transaction commit #8299

Open

Uh oh!

Conversation

srijantrpth commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

How did you test this code?

Uh oh!

vercel Bot commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srijantrpth Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

srijantrpth Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

srijantrpth commented Aug 14, 2026 •

edited

Loading

coderabbitai Bot commented Aug 14, 2026 •

edited

Loading

codecov Bot commented Aug 14, 2026 •

edited

Loading

coderabbitai Bot Aug 14, 2026 •

edited

Loading