Skip to content

ref(flags): Move 2 should-be-permanent flags to permanent.py #114934

Open
wedamija wants to merge 2 commits intomasterfrom
flag-cleanup/should-be-permanent-batch
Open

ref(flags): Move 2 should-be-permanent flags to permanent.py #114934
wedamija wants to merge 2 commits intomasterfrom
flag-cleanup/should-be-permanent-batch

Conversation

@wedamija
Copy link
Copy Markdown
Member

@wedamija wedamija commented May 5, 2026

Per the flag scanner, these flags use the INTERNAL strategy with no
flagpole config and are subscription-managed (granted via plan
handlers / feature lists), so they belong in permanent.py rather than
temporary.py. Pure registration move — no behavior change.

  • auth:register
  • organizations:continuous-profiling-stats

wedamija and others added 2 commits May 5, 2026 16:26
Per the flag scanner, these flags use the INTERNAL strategy with no
flagpole config and are subscription-managed (granted via plan
handlers / feature lists), so they belong in permanent.py rather than
temporary.py. Pure registration move — no behavior change.

- auth:register
- organizations:continuous-profiling-stats

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror the org/project pattern for system-scoped permanent flags so
adding new ones is a one-line dict edit instead of an ad-hoc
manager.add() block. Consolidates the existing auth:register and
system:multi-region registrations into the new dict.

Note: SystemFeatures cannot be flagpole-controlled (flagpole only
handles org/project scope), so the loop hardcodes
FeatureHandlerStrategy.INTERNAL.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@wedamija wedamija requested a review from a team May 5, 2026 23:28
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 5, 2026
Comment on lines +179 to +185
for system_feature, default in permanent_system_features.items():
manager.add(
system_feature,
SystemFeature,
FeatureHandlerStrategy.INTERNAL,
default=default,
)
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.

auth:register system feature loses explicit api_expose=False

The previous registration of system:multi-region passed api_expose=False explicitly. The new loop registers both auth:register and system:multi-region without passing api_expose, so both will use the manager.add default. If the default for api_expose is True (or differs from False), this silently exposes auth:register and system:multi-region via the API, which is a behavior change despite the PR claiming none. Verify FeatureManager.add's api_expose default before merging.

Verification

Read the diff: original system:multi-region registration explicitly set api_expose=False, while the new loop omits api_expose. Did not read FeatureManager.add to confirm its default value, hence medium confidence. This falls under Check 11 (logic correctness) — a behavior-changing omission in a 'pure registration move' PR.

Identified by Warden sentry-backend-bugs · WXT-XCJ

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

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant