Skip to content

fix: Enhance EFV audit logs #7535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SahilJat wants to merge 3 commits into
base: main
Choose a base branch
from
+150 −3
Open

fix: Enhance EFV audit logs #7535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6a130d
Enhance EFV audit logs
SahilJat May 19, 2026
3e96552
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 19, 2026
3eb06b6
fix: Add missing Segment import in test
SahilJat May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions api/features/versioning/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,20 +261,51 @@
)


def _build_feature_state_change_summary(fs: FeatureState) -> str:
if fs.identity_id:
scope = f"Identity override ({fs.identity.identifier})" # type: ignore[union-attr]
elif fs.feature_segment_id:
scope = f"Segment override ({fs.feature_segment.segment.name})" # type: ignore[union-attr]
else:
scope = "Environment default"

state = "enabled" if fs.enabled else "disabled"

Check failure on line 272 in api/features/versioning/tasks.py

View check run for this annotation

Claude / Claude Code Review

Audit log summary omits value and multivariate changes 

The audit-log summary only reports each changed feature state's scope and current `enabled`/`disabled` flag, but `get_updated_feature_states_for_version` also flags states whose `feature_state_value` or multivariate values changed while `enabled` stayed the same. Such value-only edits produce a line indistinguishable from an enabled-toggle change (e.g. `- Environment default: enabled`), defeating this PR's stated goal of making EFV audit logs less opaque. Consider including the value/MV diff (or
Comment on lines +264 to +272
Copy link
Copy Markdown

@claude claude Bot May 22, 2026

Choose a reason for hiding this comment

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

🔴 The audit-log summary only reports each changed feature state's scope and current enabled/disabled flag, but get_updated_feature_states_for_version also flags states whose feature_state_value or multivariate values changed while enabled stayed the same. Such value-only edits produce a line indistinguishable from an enabled-toggle change (e.g. - Environment default: enabled), defeating this PR's stated goal of making EFV audit logs less opaque. Consider including the value/MV diff (or at least marking which attribute changed) in _build_feature_state_change_summary, and add a regression test for a value-only change.

Extended reasoning...

What the bug is

The new _build_feature_state_change_summary (api/features/versioning/tasks.py:264-272) only renders two pieces of information for each changed feature state: its scope (Environment default / Segment override (...) / Identity override (...)) and the current enabled/disabled flag. It never inspects (or surfaces) the feature-state value or the multivariate allocations.

However, get_updated_feature_states_for_version (api/features/versioning/versioning_service.py:510-522) classifies a feature state as "changed" if any of the following differs from the previous version:

if previous_fs is None or (
    feature_state.enabled != previous_fs.enabled
    or feature_state.get_feature_state_value()
       != previous_fs.get_feature_state_value()
    or multivariate_values_changed(feature_state, previous_fs)
):
    changed_feature_states.append(feature_state)

So a feature state whose value changed (e.g. string_value "v1" → "v2") or whose multivariate allocation changed, with enabled unchanged, will be in the changed list — but the audit log line will only say the current enabled state.

Step-by-step proof

  1. Start with an EFV where the env-default feature state has enabled=True, string_value="v1".
  2. Create a new version, copy the previous states, then edit only the env-default value to "v2" (still enabled=True).
  3. Publish. create_environment_feature_version_published_audit_log_task runs.
  4. get_updated_feature_states_for_version returns that env-default FS (value differs).
  5. _build_feature_state_change_summary builds: scope = "Environment default", state = "enabled" (because fs.enabled is True), output: "Environment default: enabled".
  6. Audit log: New version published for feature: foo\n- Environment default: enabled.
  7. Now perform a different change on a fresh version: leave value at "v1" and toggle enabled from False to True. The audit log is identical: - Environment default: enabled.

Two semantically distinct changes (value edit vs. enabled toggle) produce the same audit-log line, and a reader cannot tell what actually changed — exactly the problem this PR was meant to fix (linked issue #7526: "opaque audit logs … completely omitting which feature states were actually changed").

Why existing code does not prevent it

Nothing in the new code path reads feature_state_value or compares against the previous version. The line template is f"{scope}: {state}" with state derived purely from the current fs.enabled. The test suite added in this PR only exercises (a) no changes, (b) an env-default toggle, (c) a brand-new segment override, (d) a brand-new identity override — none cover a value-only or MV-only change against a previous version.

Impact

Quality-of-message regression vs. the PR's stated goal. No crash, audit logs are still created, but they remain misleading/opaque for the very common case of users editing a feature state's value (e.g. config string, JSON payload) without toggling enabled. For multivariate features the line is similarly uninformative.

How to fix

In _build_feature_state_change_summary, accept (or look up) the previous feature state and emit what actually changed — e.g.:

  • Environment default: value changed ("v1" → "v2")
  • Environment default: enabled (was disabled)
  • Environment default: multivariate allocation changed

Or, less ambitiously, append the current value alongside the enabled flag so at least the value is visible (still not great for MV). Either way, add a regression test that mutates feature_state_value only and asserts the audit log differs from the equivalent enabled-toggle case.

return f"{scope}: {state}"


@register_task_handler()
def create_environment_feature_version_published_audit_log_task(
environment_feature_version_uuid: str,
) -> None:
environment_feature_version = EnvironmentFeatureVersion.objects.select_related(
"environment", "feature"
).get(uuid=environment_feature_version_uuid)

header = (
ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE
% environment_feature_version.feature.name
)

changed_states = get_updated_feature_states_for_version(environment_feature_version)

if changed_states:
changed_states = list(
FeatureState.objects.filter(
id__in=[fs.id for fs in changed_states]
).select_related("feature_segment__segment", "identity")
)
change_lines = "\n".join(
f"- {_build_feature_state_change_summary(fs)}" for fs in changed_states
)
log = f"{header}\n{change_lines}"
else:
log = header

Check failure on line 302 in api/features/versioning/tasks.py

View check run for this annotation

Claude / Claude Code Review

Removed feature states omitted from audit log 

The new audit log silently omits removed feature states. `get_updated_feature_states_for_version()` only iterates the **new** version's feature states and compares them against the previous version — it never walks the previous version looking for keys that don't appear in the new one. So when a user publishes a version whose only change is dropping a segment override (e.g. via `segment_ids_to_delete_overrides`), the removed override is never visited and the audit log falls back to just the head
Comment on lines 281 to +302
Copy link
Copy Markdown

@claude claude Bot May 22, 2026

Choose a reason for hiding this comment

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

🔴 The new audit log silently omits removed feature states. get_updated_feature_states_for_version() only iterates the new version's feature states and compares them against the previous version — it never walks the previous version looking for keys that don't appear in the new one. So when a user publishes a version whose only change is dropping a segment override (e.g. via segment_ids_to_delete_overrides), the removed override is never visited and the audit log falls back to just the header ("New version published for feature: X"), contradicting the PR's stated goal of enumerating each changed Feature State's action and scope.

Extended reasoning...

What the bug is

create_environment_feature_version_published_audit_log_task (api/features/versioning/tasks.py:281-302) delegates change detection to get_updated_feature_states_for_version (api/features/versioning/versioning_service.py:479-522). That helper iterates only the new version's feature states:

previous_feature_states_map = (
    {get_match_key(fs): fs for fs in previous_version.feature_states.all()}
    if previous_version
    else {}
)

changed_feature_states = []
for feature_state in version.feature_states.all():   # NEW version only
    previous_fs = previous_feature_states_map.get(get_match_key(feature_state))
    ...

It never enumerates the previous map looking for keys absent in the new version, so feature states that existed in the previous version but were removed in the new one are invisible to the audit log.

Why this triggers for deletions

The versioning create serializer (api/features/versioning/serializers.py:312-315) handles segment_ids_to_delete_overrides by hard-deleting the matching feature_segments on the new version:

def _delete_feature_states(self, segment_ids, version):
    version.feature_segments.filter(segment_id__in=segment_ids).delete()

Because FeatureState.feature_segment cascades, the corresponding FeatureState rows are removed from the new version. So new_version.feature_states.all() simply doesn't contain them, and the loop never visits them. A similar single-override path exists in _delete_segment_override_v2 (versioning_service.py:447-474), which also deletes the FS from the new version after cloning.

Step-by-step proof

  1. Environment with v2 versioning enabled; feature F has an active segment override on segment S1 in version V1 (live).
  2. User creates a new version V2 whose only change is segment_ids_to_delete_overrides: [S1.id] and publishes it.
  3. During create, _delete_feature_states([S1.id], V2) is called → V2.feature_segments.filter(segment_id=S1.id).delete() cascades and the cloned segment-override FeatureState in V2 is removed.
  4. V2.feature_states.all() now contains only the environment-default FS, which is identical to V1's default → it matches by (identity_id=None, segment_id=None), all attributes equal, so changed_feature_states is empty.
  5. create_environment_feature_version_published_audit_log_task therefore takes the else: log = header branch.
  6. Resulting audit log is just "New version published for feature: F" — no mention of the removed S1 override.

Impact

The PR description explicitly promises that the audit log enumerates each changed Feature State's action and scope. A pure-deletion publish produces a log that's indistinguishable from a no-op publish, which is misleading for users auditing what changed. The same blind spot exists in the parallel webhook code (_trigger_feature_state_webhooks_for_version, tasks.py:154-240), so this is a pre-existing limitation of the helper — but this PR is the first to surface it in a user-facing audit message.

How to fix

Either extend get_updated_feature_states_for_version to also yield previous-version FS whose keys are absent in the new version (tagged as removals), or add a parallel removal-detection step inside create_environment_feature_version_published_audit_log_task that diffs previous_version.feature_states against the new version's keys and renders something like - Segment override (S1): removed. The summary builder in this PR (_build_feature_state_change_summary) would need a small extension to render a "removed" state, or a separate helper for removed states.


AuditLog.objects.create(
environment=environment_feature_version.environment,
related_object_type=RelatedObjectType.EF_VERSION.name,
related_object_uuid=environment_feature_version.uuid,
log=ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE
% environment_feature_version.feature.name,
log=log,
author_id=environment_feature_version.published_by_id,
master_api_key_id=environment_feature_version.published_by_api_key_id,
)
Expand Down
113 changes: 113 additions & 0 deletions api/tests/unit/audit/test_unit_audit_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
create_feature_state_updated_by_change_request_audit_log,
create_segment_priorities_changed_audit_log,
)
from environments.identities.models import Identity
from environments.models import Environment
from features.models import Feature, FeatureSegment, FeatureState
from features.versioning.models import EnvironmentFeatureVersion
from features.versioning.tasks import (
create_environment_feature_version_published_audit_log_task,
)
from features.workflows.core.models import ChangeRequest
from segments.models import Segment


def test_get_audited_instance_from_audit_log_record__change_request__return_expected(
Expand Down Expand Up @@ -98,6 +100,117 @@ def test_get_audited_instance_from_audit_log_record__historical_record__return_e
assert instance == change_request


def test_create_environment_feature_version_published_audit_log_task__no_changes__uses_header_only(
environment_v2_versioning: Environment,
feature: Feature,
) -> None:
# Given
version = EnvironmentFeatureVersion.objects.create(
feature=feature,
environment=environment_v2_versioning,
)

# When
create_environment_feature_version_published_audit_log_task(str(version.uuid))

# Then
audit_log = AuditLog.objects.get(
related_object_type=RelatedObjectType.EF_VERSION.name,
related_object_uuid=version.uuid,
)
assert audit_log.log == f"New version published for feature: {feature.name}"


def test_create_environment_feature_version_published_audit_log_task__environment_default_changed__includes_detail(
environment_v2_versioning: Environment,
feature: Feature,
) -> None:
# Given
version = EnvironmentFeatureVersion.objects.create(
feature=feature,
environment=environment_v2_versioning,
)
fs = version.feature_states.filter(feature=feature).first()
assert fs is not None
fs.enabled = not fs.enabled
fs.save()

# When
create_environment_feature_version_published_audit_log_task(str(version.uuid))

# Then
audit_log = AuditLog.objects.get(
related_object_type=RelatedObjectType.EF_VERSION.name,
related_object_uuid=version.uuid,
)
assert f"New version published for feature: {feature.name}" in audit_log.log
assert "Environment default:" in audit_log.log


def test_create_environment_feature_version_published_audit_log_task__segment_override_changed__includes_segment_name(
environment_v2_versioning: Environment,
feature: Feature,
segment: Segment,
) -> None:
# Given
version = EnvironmentFeatureVersion.objects.create(
feature=feature,
environment=environment_v2_versioning,
)
feature_segment = FeatureSegment.objects.create(
feature=feature,
segment=segment,
environment=environment_v2_versioning,
environment_feature_version=version,
)
FeatureState.objects.create(
feature=feature,
environment=environment_v2_versioning,
feature_segment=feature_segment,
environment_feature_version=version,
enabled=True,
)

# When
create_environment_feature_version_published_audit_log_task(str(version.uuid))

# Then
audit_log = AuditLog.objects.get(
related_object_type=RelatedObjectType.EF_VERSION.name,
related_object_uuid=version.uuid,
)
assert f"Segment override ({segment.name})" in audit_log.log


def test_create_environment_feature_version_published_audit_log_task__identity_override_changed__includes_identity(
environment_v2_versioning: Environment,
feature: Feature,
identity: Identity,
) -> None:
# Given
version = EnvironmentFeatureVersion.objects.create(
feature=feature,
environment=environment_v2_versioning,
)
FeatureState.objects.create(
feature=feature,
environment=environment_v2_versioning,
identity=identity,
environment_feature_version=version,
enabled=True,
)

# When
create_environment_feature_version_published_audit_log_task(str(version.uuid))

# Then
audit_log = AuditLog.objects.get(
related_object_type=RelatedObjectType.EF_VERSION.name,
related_object_uuid=version.uuid,
)
assert f"Identity override ({identity.identifier})" in audit_log.log


def test_get_audited_instance_from_audit_log_record__unexpected_audit_log__return_none(
change_request: ChangeRequest,
) -> None:
Expand Down
5 changes: 4 additions & 1 deletion api/tests/unit/features/versioning/test_unit_versioning_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,10 @@ def test_publish_feature_version__unpublished_version__publishes_and_creates_aud
related_object_uuid=environment_feature_version.uuid,
).first()
assert record
assert record.log == ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE % feature.name
assert record.log == (
f"{ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE % feature.name}\n"
f"- Environment default: disabled"
)


@pytest.mark.parametrize("live_from", (None, tomorrow))
Expand Down
Loading