Repository navigation
feat(flags): expose the flag lifecycle endpoints as MCP tools - #92134
Conversation
🤖 CI report
|
| App | JS | CSS |
|---|---|---|
| debug | 600.0 KB | 195.2 KB |
| action | 458.2 KB | 195.2 KB |
| action-list | 564.9 KB | 195.2 KB |
| cohort | 457.2 KB | 195.2 KB |
| cohort-list | 563.8 KB | 195.2 KB |
| email-template | 457.0 KB | 195.2 KB |
| error-details | 472.9 KB | 195.2 KB |
| error-issue | 457.9 KB | 195.2 KB |
| error-issue-list | 564.7 KB | 195.2 KB |
| experiment | 562.0 KB | 195.2 KB |
| experiment-list | 565.6 KB | 195.2 KB |
| experiment-results | 567.1 KB | 195.2 KB |
| feature-flag | 567.6 KB | 195.2 KB |
| feature-flag-list | 571.4 KB | 195.2 KB |
| feature-flag-testing | 461.4 KB | 195.2 KB |
| inline-scan | 457.7 KB | 195.2 KB |
| insight-actors | 563.0 KB | 195.2 KB |
| invite-email-preview | 456.4 KB | 195.2 KB |
| llm-costs | 560.0 KB | 195.2 KB |
| session-recording | 459.0 KB | 195.2 KB |
| survey | 458.8 KB | 195.2 KB |
| survey-global-stats | 562.7 KB | 195.2 KB |
| survey-list | 565.5 KB | 195.2 KB |
| survey-stats | 562.7 KB | 195.2 KB |
| trace-span | 457.6 KB | 195.2 KB |
| trace-span-list | 564.7 KB | 195.2 KB |
| vision-observation-list | 563.9 KB | 195.2 KB |
| workflow | 457.5 KB | 195.2 KB |
| workflow-list | 564.2 KB | 195.2 KB |
| loops-review | 461.9 KB | 195.2 KB |
| query-results | 755.4 KB | 195.2 KB |
| render-ui | 838.5 KB | 195.2 KB |
| visual-review-snapshots | 462.0 KB | 195.2 KB |
684ee3b to
bde993f
Compare
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 2 consider. Published 3 findings (view the review). Resolved comments: 2 fixed, 1 declined, 1 left for you |
Prompt To Fix All With AI### Issue 1
services/mcp/src/tools/generated/feature_flags.ts:128-134
**Lifecycle handlers lack error handling**
The new archive, disable, enable, and unarchive handlers await project lookup and API requests without local error handling. Rejected operations therefore propagate without lifecycle-specific diagnostics, making failures less actionable.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(flags): correct the lifecycle tool d..." | Re-trigger Greptile |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
| title: Enable feature flag | ||
| description: > | ||
| Turn a feature flag on. Requires the flag's numeric ID; use `feature-flag-get-definition-by-key` to get it | ||
| from a key, or `feature-flag-get-all` to search by key or name. | ||
|
|
||
|
|
||
| Sets `active` to true and changes nothing else. This tool never accepts or replaces the `filters` object, so | ||
| targeting, rollout percentages, variants and payloads cannot change. There is no request body. To change who | ||
| the flag is served to, use `update-feature-flag`. | ||
|
|
||
|
|
||
| Returns 400 when the flag is archived (call `feature-flag-unarchive` first) or when a flag it depends on is | ||
| disabled. Returns 409 when an approval policy gates enabling. Enabling an already-enabled flag succeeds and | ||
| changes nothing, so retrying is safe. |
There was a problem hiding this comment.
Lifecycle tools accept soft-deleted flags
Why we think it's a valid issue
- Checked: the viewset's base queryset and where
deletedis filtered, each lifecycle action's guard clauses, whether an equivalent guard already exists in this viewset, what evaluation does with a deleted flag, how an agent could obtain a deleted flag's ID, and the layer's test coverage. - Found: detail routes really do resolve soft-deleted flags.
queryset = FeatureFlag.objects_including_soft_deleted.all()atproducts/feature_flags/backend/api/feature_flag.py:3129, with a comment at:3126-3128stating that non-list actions can access soft-deleted flags, and only thelistbranch appliesqueryset.filter(deleted=False)at:3189-3191. - Found: none of the four actions check
deleted.archive(:3591-3604),unarchive(:3620-3627) and_set_active(:3563-3575) go fromself.get_object()straight to the facade. The viewset already owns a guard for exactly this —_deleted_flag_rejectionat:3367-3379, applied at:3389and:3425— so the omission is inconsistent with the local pattern and the fix is two lines per action. - Found: the misleading-success consequence holds.
FeatureFlagManager.get_querysetexcludesdeleted=True(products/feature_flags/backend/models/feature_flag.py:134-136), and the evaluation/cache paths read throughFeatureFlag.objects(products/feature_flags/backend/flags_cache.py:916,products/feature_flags/backend/tasks.py:389), so a deleted flag never evaluates howeveractivereads back.feature-flag-unarchive's promise to restore a flag "to the default flag list" (products/feature_flags/mcp/tools.yaml:266) is also false for a deleted flag, since the list filters it out. - Found: no coverage.
products/feature_flags/backend/api/test/test_feature_flag_lifecycle_actions.pycontains no deleted-flag case. - Impact: a caller holding a deleted flag's numeric ID gets 200 and
active: truefor a flag that serves nobody, or a reported restore that leaves the flag invisible. Nothing in the response or the description corrects that, which is why this stays valid rather than being dismissed as already-handled. - Priority: lowered to
consider. The same behavior already exists on the tool these replace —partial_update(:3136-3138) adds no guard either, soupdate-feature-flagcan enable a deleted flag on master today. Every lookup the new descriptions recommend is list-backed (feature-flag-get-all, andfeature-flag-get-definition-by-keyvia the list endpoint), so both hide deleted flags and an agent has to be carrying a stale ID from an earlier turn or one the user supplied. Nothing is corrupted and the write is reversible, so the ceiling is a wrong report on a rare input rather than theshould_fiximpact claimed.
Issue description
The lifecycle endpoints can resolve soft-deleted flags and do not reject deleted: true. Enable can return active: true, but evaluation still excludes the flag. Unarchive can return success, but the default list still excludes it. A caller with an old numeric ID receives a successful result for an unusable flag.
Suggested fix
Reject deleted flags in every lifecycle endpoint. If restoration is supported, add a dedicated restore action and require it before enable or unarchive. Add tests for all four actions with a deleted flag.
Prompt to fix with AI (copy-paste)
## Context
@products/feature_flags/mcp/tools.yaml#L178-191
@products/feature_flags/mcp/tools.yaml#L264-275
<issue_description>
The lifecycle endpoints can resolve soft-deleted flags and do not reject `deleted: true`. Enable can return `active: true`, but evaluation still excludes the flag. Unarchive can return success, but the default list still excludes it. A caller with an old numeric ID receives a successful result for an unusable flag.
</issue_description>
<issue_validation>
- **Checked:** the viewset's base queryset and where `deleted` is filtered, each lifecycle action's guard clauses, whether an equivalent guard already exists in this viewset, what evaluation does with a deleted flag, how an agent could obtain a deleted flag's ID, and the layer's test coverage.
- **Found:** detail routes really do resolve soft-deleted flags. `queryset = FeatureFlag.objects_including_soft_deleted.all()` at `products/feature_flags/backend/api/feature_flag.py:3129`, with a comment at `:3126-3128` stating that non-list actions can access soft-deleted flags, and only the `list` branch applies `queryset.filter(deleted=False)` at `:3189-3191`.
- **Found:** none of the four actions check `deleted`. `archive` (`:3591-3604`), `unarchive` (`:3620-3627`) and `_set_active` (`:3563-3575`) go from `self.get_object()` straight to the facade. The viewset already owns a guard for exactly this — `_deleted_flag_rejection` at `:3367-3379`, applied at `:3389` and `:3425` — so the omission is inconsistent with the local pattern and the fix is two lines per action.
- **Found:** the misleading-success consequence holds. `FeatureFlagManager.get_queryset` excludes `deleted=True` (`products/feature_flags/backend/models/feature_flag.py:134-136`), and the evaluation/cache paths read through `FeatureFlag.objects` (`products/feature_flags/backend/flags_cache.py:916`, `products/feature_flags/backend/tasks.py:389`), so a deleted flag never evaluates however `active` reads back. `feature-flag-unarchive`'s promise to restore a flag "to the default flag list" (`products/feature_flags/mcp/tools.yaml:266`) is also false for a deleted flag, since the list filters it out.
- **Found:** no coverage. `products/feature_flags/backend/api/test/test_feature_flag_lifecycle_actions.py` contains no deleted-flag case.
- **Impact:** a caller holding a deleted flag's numeric ID gets 200 and `active: true` for a flag that serves nobody, or a reported restore that leaves the flag invisible. Nothing in the response or the description corrects that, which is why this stays valid rather than being dismissed as already-handled.
- **Priority:** lowered to `consider`. The same behavior already exists on the tool these replace — `partial_update` (`:3136-3138`) adds no guard either, so `update-feature-flag` can enable a deleted flag on master today. Every lookup the new descriptions recommend is list-backed (`feature-flag-get-all`, and `feature-flag-get-definition-by-key` via the list endpoint), so both hide deleted flags and an agent has to be carrying a stale ID from an earlier turn or one the user supplied. Nothing is corrupted and the write is reversible, so the ceiling is a wrong report on a rare input rather than the `should_fix` impact claimed.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Reject deleted flags in every lifecycle endpoint. If restoration is supported, add a dedicated restore action and require it before enable or unarchive. Add tests for all four actions with a deleted flag.
</potential_solution>
There was a problem hiding this comment.
This is a real gap, but it belongs to the layer below, not this PR. The behavior the finding describes lives entirely in the lifecycle viewset actions (_set_active, archive, unarchive in products/feature_flags/backend/api/feature_flag.py): they resolve soft-deleted flags and don't reject deleted, unlike the sibling _deleted_flag_rejection guard already used by the usage-dashboard actions. That file ships in the base branch feat/flags-lifecycle-endpoints; this PR only wires the existing endpoints up as MCP tools and changes no backend code, so I can't fix it here without pulling a change into the wrong layer of the stack (the split was deliberate). It also needs a decision I shouldn't make unattended: reject deleted flags in all four actions, or add a dedicated restore action and require it before enable/unarchive — plus the backend tests for a deleted flag that are currently missing. Handing this to a human to land in the endpoints' own PR (or a follow-up) with the design choice made. I did not add a caveat to the tool descriptions instead: it wouldn't correct the misleading success response, and the recommended lookups (feature-flag-get-all, feature-flag-get-definition-by-key) are all list-backed and already hide deleted flags, so an agent can only reach this with a stale id carried from an earlier turn.
haacked
left a comment
There was a problem hiding this comment.
Looks good, one non-blocking suggestion inline.
400738e to
f6cd4d7
Compare
f6cd4d7 to
e414c8b
Compare
|
/trunk merge |
|
😎 Stack merged successfully - details. |
The layer below added four endpoints that change one flag's state and take no request body. This turns them on for agents as `feature-flag-enable`, `feature-flag-disable`, `feature-flag-archive` and `feature-flag-unarchive`. Each tool's generated schema is the flag id and nothing else, so an agent cannot send `filters` through one. The descriptions say which field the action changes, state that the tool never accepts or replaces `filters`, and point at `scheduled-changes-create` for a future-dated change where that endpoint supports the operation. The disable description asks the agent to read the full definition and report linked-consumer impact when the user did not ask for the flag to be turned off. `update-feature-flag` keeps `active` and `archived` for combined edits. Its description now names the dedicated tool when one matches the request. `disable` joins the trailing-action list so the compact domain index renders `feature-flag` rather than `feature-flag-disable`. Eval tasks cover tool selection for each action, and that a targeting edit still picks `update-feature-flag`. Their success criteria judge the tool chosen and what the agent reports, so they score without a seeded fixture flag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An agent asked to turn an archived flag back on reported that the flag did not exist. The enable description sent it to `feature-flag-get-definition-by-key` or `feature-flag-get-all` to resolve the key. Both hide archived flags, so the 400 that names unarchive as the fix was unreachable. Enable now says a key miss can mean an archived flag, and names the `archived=true` lookup. Archiving an enabled flag turns it off, which stops it evaluating everywhere. Only the disable description asked the agent to report that impact. Archive now asks for it too. Disable asks for it unconditionally, because the old condition (the user did not ask for the flag to be turned off) is almost never true for a tool named disable. `update-feature-flag` pointed at `feature-flag-get-all` for key resolution while the four new tools pointed at the by-key tool. It also claimed the lifecycle tools "cannot lose a concurrent edit". In this repo that phrase means a version precondition and a 409, and these endpoints have neither. What they do have is narrower and now stated: they send no `filters`, so a targeting change made between the read and the write survives. The one legitimate use of `active` and `archived` on the generic tool is now named too. The stale-flags skill told agents to disable through `update-feature-flag` while the rewritten description told them to prefer the dedicated tool. An agent loading both got two answers. `flag-unarchive-stays-off` could pass without unarchiving anything. Its escape clause accepted "the flag does not exist", which is what a plain key lookup returns for an archived flag. The benchmark comment also claimed a flag the project owns. Nothing seeds it, and one shared key makes the run order-dependent. Regenerated the tool-definition JSON from the YAML. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The archive and disable tool descriptions promise a safe no-op when the flag is already in the target state, but route key resolution through `feature-flag-get-definition-by-key`, which hides archived flags. A retry by key after a flag is archived (or a disable of an already-archived flag) missed and reported the flag as absent. Add the same archived-lookup fallback note the sibling `feature-flag-enable` and `feature-flag-unarchive` tools already carry: pass `archived=true` to `feature-flag-get-all` before reporting the flag missing. Regenerate the two derived tool-definition JSON files to match. Generated-By: PostHog Desktop Task-Id: 78f746f0-750e-41db-86f0-a9750a91318f
The lifecycle tool descriptions told agents to `Pass archived=true to
feature-flag-get-all`, but the list endpoint's `archived` param is a string
enum (`"false"` / `"true"`), so a JSON boolean fails schema validation and
costs the agent a turn before it can find the archived flag.
Show the exact call argument instead: `{"archived":"true"}`. Applies to the
enable, disable, archive and unarchive descriptions, and regenerates the two
derived tool-definition JSON files. Only the feature-flags copy changes; the
survey and AI observability tools use real booleans and are untouched.
Generated-By: PostHog Desktop
Task-Id: 78f746f0-750e-41db-86f0-a9750a91318f
A lifecycle tool result carried the whole flag: 1,717 bytes over 32 fields for a flag with two conditions, two variants and one payload, and `filters` alone can reach the 512 KiB cap. All of it lands in the agent's context to confirm a state change, and `filters` in the result reintroduces the targeting object these tools exist to keep out of an agent's hands. `response.include` narrows the four results to id, key, active, archived, status and version. `id` stays because the handler builds the enrich URL from it. The endpoint keeps returning the full representation. Widening a response later needs every client to agree, and only the MCP caller exists so far. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`FeatureFlagsListQueryParams.archived` is a Zod enum of the strings "false" and "true", so a JSON boolean fails schema validation. The criterion said `archived=true`, which an agent can satisfy with a boolean and have the call rejected, while the tool descriptions already show the valid form. The judge could not require a working archived lookup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e414c8b to
a857b77
Compare
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: tests-posthog[bot] <250237707+tests-posthog[bot]@users.noreply.github.com> Co-authored-by: posthog[bot] <206114724+posthog[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: tests-posthog[bot] <250237707+tests-posthog[bot]@users.noreply.github.com> Co-authored-by: posthog[bot] <206114724+posthog[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: tests-posthog[bot] <250237707+tests-posthog[bot]@users.noreply.github.com> Co-authored-by: posthog[bot] <206114724+posthog[bot]@users.noreply.github.com>
Problem
An agent asked to turn one feature flag off can silently wipe that flag's targeting, and nothing stops it.
The only tool for the job is
update-feature-flag. Its own description tells the agent to read the whole flag, changeactive, then send everything back.filtersis not partial: sending it replaces every release condition, variant, payload and holdout.Two things turn that from verbose into unsafe.
FeatureFlagSerializer.updaterejects a stale write when the caller sendsversionandoriginal_flag, and the flag UI sends both on every PATCH.versionsits on the excluded list infeature_flag_openapi_test_helpers.py, so it never reaches the tool schema. Any targeting edit made between the agent's read and its write is lost with no error.The layer below added endpoints that fix this. Until they are turned on, no agent can reach them.
Before, four steps and a targeting payload:
flowchart LR Agent{{Agent}} --> Read[feature-flag-get-definition] Read --> Snapshot[filters snapshot] Snapshot --> Tool[update-feature-flag] Tool --> Write[(PATCH active and filters)] classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class Agent phYellow; class Read,Tool phBlue; class Snapshot phGray; class Write phRed;After, one step and no targeting payload:
flowchart LR Agent{{Agent}} --> Tool[feature-flag-disable] Tool --> Write[(POST, flag id only)] classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; class Agent phYellow; class Tool phBlue; class Write phRed;Changes
feature-flag-enable,feature-flag-disable,feature-flag-archiveandfeature-flag-unarchive.{ id }, because the endpoint declares no request body.filters.update-feature-flagkeepsactiveandarchivedfor combined edits. Its description now names the dedicated tool when one matches, so an agent has a reason to prefer the narrow call.feature-flag-get-definition-by-key, which asks for at most five flags, rather thanfeature-flag-get-all, which returns a hundred per page.enablewarns that a key lookup misses an archived flag. Both resolvers go through the list endpoint, which hides archived rows unlessarchivedis in the query, so an agent would otherwise report the flag missing instead of unarchiving it.Mechanical: the regenerated Orval schemas, tool handlers, tool-definition JSON and four schema snapshots.
disablealso joins the trailing-action list, so the compact domain index rendersfeature-flagrather thanfeature-flag-disable.How did you test this code?
Automated only. The eval tasks were not executed.
feature-flag-lifecycle-tools.test.tsasserts each tool's schema exposesidand nothing else. The schema snapshots would also catch afiltersparam appearing, butvitest -usilences them and these assertions it cannot.feature-flag-enablewired to/disable/. An agent asked to turn a flag on would turn it off.update-feature-flag. Their criteria judge the tool chosen and what the agent reports, so they score without a seeded fixture flag. Seedingmcp-eval-lifecyclein the benchmark project would also exercise the state change.Not checked: no run against a deployed MCP client in either mode.
Automatic notifications
Docs update
None. The endpoint reference ships with the layer below.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Opus 5) wrote this. It is the top layer of a two-PR stack and depends on the endpoints in the layer below.
Skills invoked:
/implementing-mcp-tools,/writing-tests,/writing-user-facing-copy,/review-code --fix,/stacking-prs,/writing-pr-descriptions.The work started as one PR covering both layers, and was split on review.
ci-things-already-tried.mdrecords a rejected stack whose layers corrected each other's lines, so the split was checked against it. Layer 1'stools.yamlstubs carry the final tool names, andscaffold-yaml --sync-allmatches onoperationrather than the key. This layer only fills each block in. Its tentools.yamldeletions are the fourenabled: falselines and the one description rewritten on purpose.products/feature_flags/skills/cleaning-up-stale-feature-flags/SKILL.mdstill points atupdate-feature-flagfor disabling. That was left alone deliberately: the brief puts cleanup-task callers out of scope for this work.Public artifact: nothing here carries material from the agent session that is not already public. The eval task intents are invented rather than derived, and the planning notes stay outside the repository.