fix(feature-flags): gate trash and restore on a standalone flag - #108143
yasen-posthog wants to merge 4 commits into
Conversation
Trash flips active on any flag in the file tree, so an editor could disable a flag an approval policy protects by trashing it, and enable it again with undo. The same flip through the flags API answers 409. Ownership picks the path, the way resource-scoped policies do everywhere else. A standalone flag is what feature_flag.* governs, so trash and restore now honour an enable or disable policy on it. A product-owned flag matches no feature_flag.* policy and keeps the raw write.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
github-actions
Bot
added
feature/feature-flags
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
products/feature_flags/backend/facade/api.py |
3 | 36 |
products/approvals/backend/tests/test_scheduled_change_gating.py |
1 | 3 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Bundle size — 🔺 +35.2 KiB (+0.0%)
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 68.92 MiB · 🔺 +35.2 KiB (+0.0%)
| File | Size | Δ vs base |
|---|---|---|
posthog-app/_parent/products/business_knowledge/frontend/scenes/BusinessKnowledgePlaygroundScene.js |
removed | 🟢 -26.2 KiB (-100.0%) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/playground/BusinessKnowledgePlaygroundScene.js |
26.1 KiB | 🔺 +26.1 KiB (new) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/BusinessKnowledgeScene.js |
removed | 🟢 -18.7 KiB (-100.0%) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/sources/BusinessKnowledgeScene.js |
18.6 KiB | 🔺 +18.6 KiB (new) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/KnowledgeSourceScene.js |
removed | 🟢 -18.0 KiB (-100.0%) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/source/KnowledgeSourceScene.js |
18.0 KiB | 🔺 +18.0 KiB (new) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/BusinessKnowledgeSettingsScene.js |
removed | 🟢 -16.8 KiB (-100.0%) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/settings/BusinessKnowledgeSettingsScene.js |
16.8 KiB | 🔺 +16.8 KiB (new) |
posthog-app/_parent/products/signals/frontend/inbox/InboxScene.js |
488.4 KiB | 🔺 +13.6 KiB (+2.9%) |
render-query/src/render-query/render-query.js |
20.16 MiB | 🔺 +7.8 KiB (+0.0%) |
posthog-app/_parent/products/ai_observability/frontend/evaluations/AIObservabilityEvaluation.js |
87.1 KiB | 🔺 +6.0 KiB (+7.4%) |
posthog-app/_parent/products/workflows/frontend/Broadcasts/BroadcastScene.js |
67.3 KiB | 🔺 +3.7 KiB (+5.9%) |
posthog-app/_parent/products/stamphog/frontend/scenes/StamphogDigestsScene/StamphogDigestsScene.js |
9.5 KiB | 🔺 +1.6 KiB (+20.7%) |
posthog-app/_parent/products/stamphog/frontend/scenes/StamphogScene/StamphogScene.js |
22.1 KiB | 🔺 +1.1 KiB (+5.4%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.58 MiB · 22 files | 🔺 +188 B (+0.0%) | █████████░ 85.8% of 1.84 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
3.52 MiB · 629 files | 🔺 +1.8 KiB (+0.0%) | █████████░ 87.3% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.35 MiB · 2,339 files | 🔺 +4.2 KiB (+0.1%) | █████████░ 88.1% of 8.34 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/index.tsx |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 854 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 216.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 100.5 KiB | src/lib/api.ts |
| 88.5 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.1 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.7 KiB | ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js |
| 33.9 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 28.4 KiB | src/scenes/scenes.ts |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 272.2 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 216.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 100.5 KiB | src/lib/api.ts |
| 98.8 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 88.5 KiB | src/products.tsx |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.16 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.16 MiB · 19 files | 🔺 +336 B (+0.0%) | ████░░░░░░ 37.8% of 5.72 MiB |
| Deferred (lazy) | 2.10 MiB · 44 files | 🔺 +528 B (+0.0%) | n/a — loads on demand |
Loader dist/toolbar.js |
1.2 KiB | no change | █░░░░░░░░░ 6.0% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 805.4 KiB | dist/toolbar/toolbar-app-5UT2PX3W.css |
| 651.7 KiB | dist/toolbar/chunk-chunk-SEZBMT4M.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-RK3L52NC.js |
| 138.3 KiB | dist/toolbar/chunk-chunk-YMECCIGG.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-SN7ERW5L.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-A6ER72L5.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-E6HGNIFN.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +804.6 KiB (+0.1%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 947.31 MiB · 🔺 +804.6 KiB (+0.1%)
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe approval gate is renamed to Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Trash and restore can misrepresent a system write’s actor, and an approval-required response omits approver details. These are bounded issues to address or explicitly accept before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Standalone flags gain an approval gate for trash and restore, but approving a request does not complete the corresponding file-tree operation. This can leave the approval result and the visible file-tree state out of sync. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
posthog/api/test/test_file_system.py (1)
726-726: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd standalone restore approval coverage.
Standalone restore now uses
feature_flag.enableapproval. The remaining restore cases do not install an enable policy, and the removed case did not assert the gated response. Add a test that expects HTTP 409, a pendingChangeRequest, and the flag to remain trashed.Suggested restore approval test
+ def _create_enable_policy(self) -> None: + ApprovalPolicy.objects.create( + organization=self.organization, + team=self.team, + action_key="feature_flag.enable", + conditions={}, + approver_config={"quorum": 1, "users": [self.user.id]}, + created_by=self.user, + ) + + @patch("products.approvals.backend.decorators._is_approvals_enabled", return_value=True) + def test_restoring_a_standalone_flag_under_an_enable_policy_needs_approval( + self, _mock_approvals_enabled + ) -> None: + flag = FeatureFlag.objects.create(team=self.team, key="gated-restore", created_by=self.user) + self._create_enable_policy() + file_entry = FileSystem.objects.get(team=self.team, type="feature_flag", ref=str(flag.id)) + + assert self.client.delete(f"/api/environments/{self.team.id}/file_system/{file_entry.id}/").status_code == 200 + + response = self.client.post( + f"/api/environments/{self.team.id}/file_system/undo_delete/", + {"items": [{"type": "feature_flag", "ref": str(flag.id)}]}, + ) + + assert response.status_code == status.HTTP_409_CONFLICT, response.json() + flag.refresh_from_db() + assert flag.deleted is True + assert flag.active is False + assert not FileSystem.objects.filter(team=self.team, type="feature_flag", ref=str(flag.id)).exists() + assert ChangeRequest.objects.filter(team=self.team, action_key="feature_flag.enable").exists()The dependency-case comment is a separate serializer-validation regression and should be updated with that fix, not combined with this approval-coverage test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 768e683a-632a-4973-b97b-0c76a89b97d5
📒 Files selected for processing (4)
posthog/api/file_system/file_system.pyposthog/api/file_system/registrations.pyposthog/api/test/test_file_system.pyproducts/feature_flags/backend/facade/api.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
|
[Critical risk] Changes how trash and restore interact with feature flag policies. The PR is not safe to merge until approval requests persist and approved actions complete the requested operation without breaking existing trash and restore behavior. Reviews (1) · Last reviewed commit: "fix(feature-flags): gate trash and resto..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The facade shape check rejects an Any-typed user parameter, because it can carry a Django object across the boundary. Take the id and read the row inside.
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
…write Routing trash through set_flag_active brought the serializer's other checks with it, so trashing a flag that other flags depend on started failing, and a second activity entry landed beside the file system's own. Use the approvals gate directly, which answers the approval question and creates the change request without a serializer write. Rename gate_scheduled_change to gate_flag_change: it already served the copy path, and now trash and restore. destroy and undo_delete take gated_atomic, so a rejected trash reports a change request that still exists.
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
products/feature_flags/backend/facade/api.py-220-222 (1)
220-222: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not evaluate system writes as the flag creator.
When
user_idisNone, the gate substitutesflag.created_byas the actor. A creator with a configured bypass role can then receive anALLOWdecision, so trash or restore proceeds without an approval request.The module docstring already defines
user=Noneas a system write. Skip the gate only whenuser_idisNone. Reject an invalid non-nulluser_idinstead of falling back to the creator.Proposed fix
- if flag_owner_kind(flag) is None: + acting_user = User.objects.filter(pk=user_id).first() if user_id is not None else None + if flag_owner_kind(flag) is None and user_id is not None: + if acting_user is None: + raise ValueError(f"Unknown acting user: {user_id}") # The gate alone, not a serializer write. Trash never ran the dependents check or filter # validation, and routing it through the serializer would start rejecting a flag other # flags depend on, and log a second activity entry beside the file system's own. change_request = gate_flag_change( - flag, {"operation": "update_status", "value": active}, User.objects.filter(pk=user_id).first() + flag, {"operation": "update_status", "value": active}, acting_user )products/feature_flags/backend/facade/api.py-223-228 (1)
223-228: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve approver details in the file-system 409 response.
When a standalone flag trash or restore matches an approval policy,
gate_flag_changereturns a change request and_flip_trashed_flagraisesApprovalRequired. The policy snapshot storesquorum,users, androlesat the top level, soapprover_configis absent. The 409 response therefore returns an emptyrequired_approversfield.Suggested fix
if change_request is not None: + policy_snapshot = change_request.policy_snapshot or {} raise ApprovalRequired( change_request=change_request, message="Approval required", - required_approvers=(change_request.policy_snapshot or {}).get("approver_config") or {}, + required_approvers={ + key: policy_snapshot[key] + for key in ("quorum", "users", "roles") + if key in policy_snapshot + }, )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 16596422-cfea-4e54-887a-9879aec75257
📒 Files selected for processing (7)
posthog/api/file_system/file_system.pyproducts/approvals/backend/scheduled_changes.pyproducts/approvals/backend/tests/test_feature_flag_bypass_matrix.pyproducts/approvals/backend/tests/test_scheduled_change_gating.pyproducts/feature_flags/backend/api/organization_feature_flag.pyproducts/feature_flags/backend/api/scheduled_change.pyproducts/feature_flags/backend/facade/api.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Approval-policy territory, but the author has STRONG familiarity with the code, there's a current-head reviewer comment with only minor nits, and the diff matches the description. It closes a real policy bypass with regression tests. The stated behavior change is disclosed: trashing a standalone flag under a policy now answers 409.
- Author wrote 93% of the modified lines and has 55 merged PRs in these paths (familiarity STRONG).
- coderabbitai[bot] reviewed the current head.
- Minor (CodeRabbit, non-blocking): the 409 for a gated trash or restore may report empty required_approvers, because the policy snapshot doesn't nest the details under approver_config.
- Minor (CodeRabbit, non-blocking): if the acting user is missing, the gate evaluates as the flag creator. The file-system caller always passes the request user, so this isn't reachable today.
- Restore has no test for the gated case where an enable policy makes it answer 409.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 94L, 6F substantive, 300L/11F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (300L, 11F, cross-cutting, fix) |
| stamphog 2.3.0 | .stamphog/policy.yml @ 103b7ee · reviewed head 103b7ee |
This branch was successfully deployed
1 participant
Problem
feature_flag.enableand 5 gatefeature_flag.disable, so the gap is reachable wherever those policies exist.TODOs, which this PR closes.Changes
Warning
Trashing or restoring a standalone flag can now answer 409
approval_requiredwhere it previously succeeded. A product-owned flag is unaffected.feature_flag.*governs standalone flags, and an experiment-owned or survey-owned flag matches that family under no endpoint, so gating it here would put an approval prompt in front of starting a survey or launching an experiment.gate_scheduled_changebecomesgate_flag_change. It already served the copy-between-projects path, where nothing is scheduled, and now serves trash and restore too. Its docstring states the payload shape, which stays scheduled-change shaped for every caller.FileSystemViewSettakesApprovalHandlingMixin, soApprovalRequiredrenders as 409 rather than escaping as a 500.destroyandundo_deletetakegated_atomic()instead oftransaction.atomic(). The gate writes its change request inside the caller's transaction, so a plain block rolled it back and the 409 carried an id with no row behind it.How did you test this code?
feature_flag.disablepolicy is refused with a 409, stays untrashed, and leaves a change request an approver can act on; a survey-owned flag under the same policy trashes with no change request.test_undo_delete_restores_feature_flagloses itsdisable_and_enable_policiescase, which asserted that a policy does not block trash. That assertion is what this PR reverses.test_undo_delete_restores_feature_flag_1_active_dependent_flagand_2_depends_on_inactive_flagfail. With plaintransaction.atomic(), the change-request assertion fails.posthog/api/test/test_file_system.py(47),products/approvals/backend/tests/(285),products/feature_flags/backend/api/test/test_scheduled_change.py(52), the gated-write invariant (32),hogli lint:tachandhogli product:lint --all.Release status
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Opus 5 (
claude-opus-5)/writing-pr-descriptions,/writing-tests.set_flag_active, which applies the whole serializer. CodeRabbit caught that it would reject a flag with dependents, and that the change request rolled back. Both held up, and the gate-only approach replaced it.TODOs rather than fixed. This PR removes both.