feat(flags): add debugging-feature-flags support skill

[phillram]

Replaces #79994, which was opened from a fork. Fork PRs get a read-only GITHUB_TOKEN, so the sandbox image workflow — which triggers on products/*/skills/** because skills are baked into the Tasks sandbox image — failed with 403 Forbidden pushing to ghcr.io. Same commits, same diff, now on a branch in this repo so CI can run properly.

No merge-order dependency on #83465. An earlier revision of this description claimed one; that was stale. This skill references debugging-experiments by name only — in one routing row and the frontmatter DO NOT TRIGGER line. It imports no content and does not cite scripts/srm_check.py. Either PR can land first; until #83465 merges the routing row simply has nothing to point at. The two are otherwise independent: this one touches only products/feature_flags/, #83465 only products/experiments/.

What

Adds a new agent skill, debugging-feature-flags, under products/feature_flags/skills/. It's a customer-support front door for feature-flag tickets: resolve the flag → reproduce the evaluation read-only → match it to a known-cause catalog → produce a customer-facing reply. Same shape as the debugging-experiments skill (workflow → symptom catalogs → hand-off → access; references/pulling-the-data.md + references/customer-reply.md).

Why

The feature-flags product had lifecycle/hygiene skills (cleanup, copy, find-deleted, audit) but nothing that takes a "my flag isn't behaving" ticket end-to-end (diagnose from the flag's own evaluation → write the reply). This fills that gap.

Highlights

• Leads with server-side evaluation reproduction. feature-flags-evaluation-reasons-retrieve and feature-flags-test-evaluation-create return the value and the match reason for a specific user, so the agent rarely has to guess.
• Catalog backed by the real FeatureFlagMatchReason enum (condition_match, no_condition_match, out_of_rollout_bound, no_group_type, super_condition_value, holdout_condition_value, flag_disabled, missing_dependency) — each mapped to a customer-facing cause and fix.
• Client-side catalog for "server says it matches but the user doesn't get it" (flags-not-loaded, identify timing, stale local eval, bootstrap mismatch, wrong accessor, ad-blockers), plus no-usage and local-vs-prod sections.
• Cross-checked against PostHog's own feature-flag troubleshooting docs — the catalog matches the documented top causes (persons match-evaluation, identify() timing, ad-blocker/flags_api_host, dependencies, behavioral-cohort limitation, the false-vs-null-vs-doesn't-exist distinction).

Verification

• Core workflow exercised live against a real flag: evaluation-reasons and test-evaluation returned out_of_rollout_bound with per-condition detail, matching the catalog and a worked reply.
• All referenced MCP tool names resolve; the only cross-skill reference is the debugging-experiments routing row, which is a bare skill name rather than a file path, so nothing here breaks if that skill is absent; both diagnostic HogQL queries parse read-only; layout matches the skills build allowlist (SKILL.md + references/).

Files

• SKILL.md — front-door workflow + known-cause catalogs
• references/pulling-the-data.md — flag config pull, server-side evaluation reproduction, historical-usage HogQL, activity, offline-hash fallback
• references/customer-reply.md — reply voice, UI-term mapping, worked examples

Docs-only: +499/-0 across three markdown files, all under products/feature_flags/skills/debugging-feature-flags/. No application code touched.

---

Created with PostHog Code

[trunk-io]

Merging to master in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

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

[greptile-apps]

_{Reviews (1): Last reviewed commit: "chore(flags): fix markdown table alignme..." | Re-trigger Greptile}

[stamphog]

Purely additive documentation — a new agent skill (markdown only, no application code, no schema/API/CI/dependency changes) that guides read-only flag debugging and explicitly calls out injection and cross-tenant access risks defensively. Content matches its markdown extension throughout, and cross-team authorship is not a blocker outside risky territory.

• 👍 on the PR from greptile-apps[bot].

Gate mechanics and policy version

Gate		Result
prerequisites	✓	all clear
deny-list	✓	no deny categories matched
size	✓	0L, 0F substantive, 499L/3F incl. docs/generated/snapshots — within ceiling
tier	✓	T1-agent / T1d-complex (499L, 3F, single-area, feat)
stamphog 2.0.0b4		.stamphog/policy.yml @ 0d7cdfc · reviewed head d0f3f4d

[haacked]

Really useful skill, and the parts I spot-checked against the matcher mostly hold up. Four factual claims don't though, and they're inline: the flag_not_found one can end up telling a customer their working flag is off.

[haacked]

blocking: This tells the agent that flag_not_found means the flag is inactive, and an agent that believes it will tell a customer their working flag is switched off. test-evaluation doesn't pass evaluation_runtime when it calls the Rust service (feature_flag.py:4248-4258), so the service detects it from the internal request instead. That request goes through a stock requests.Session, so it reads as server-side, and any flag scoped to evaluation_runtime: "client" is dropped from the evaluated set. A flag that's absent from the response hits the flag_result is None branch that sets flag_not_found. evaluation-reasons isn't affected, because it pins evaluation_runtime to "all".

So a customer has a working client-side flag, test-evaluation says flag_not_found, and the agent follows line 119, checks active, finds it already true, and is holding a contradiction this bullet says can't happen. The reason table at line 80 and customer-reply.md:27 then say "the flag is turned off in your project" about a flag that's on and serving traffic.

Scope the claim to what actually produces it. evaluation_tags doesn't, since the tag filter is empty when the caller sends no tags.

• disabled / flag_not_found — inactive, or missing from the evaluated set. evaluation-reasons
  names an inactive flag disabled. test-evaluation names it flag_not_found, but that covers every
  way a flag can be absent from the set the Rust service evaluated, not only active: false.
  evaluation-reasons reproduces with evaluation_runtime: "all", while test-evaluation lets the
  service detect the runtime from the internal (server-side) request, so an active flag scoped to
  evaluation_runtime: "client" is filtered out and returns flag_not_found. On flag_not_found, read
  active and evaluation_runtime on the config, and cross-check evaluation-reasons, which still
  returns the real reason. Neither tool ever returns flag_disabled: that enum value exists in the
  matcher, but disabled flags are filtered out before matching, so nothing reaches the code that would
  emit it — don't wait for it.

The same equivalence needs correcting at SKILL.md:80, references/pulling-the-data.md:10-12, and references/customer-reply.md:27. Worth adding evaluation_runtime to the config field list in pulling-the-data.md §1 too, since the runbook never tells the agent to read the field that explains the discrepancy.

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. Every correction you listed is in the current head:

• The catalog bullet now scopes the claim exactly as you proposed: flag_not_found covers any way a flag is absent from the evaluated set, not just active: false, and it explains that test-evaluation lets the Rust service detect the runtime from the internal (server-side) request, so an active evaluation_runtime: "client" flag is filtered out and returns flag_not_found. It also notes neither tool ever emits flag_disabled.
• The reason table no longer equates flag_not_found with inactive — it reads "inactive, or scoped to the other runtime" and tells the agent to read active and evaluation_runtime and cross-check evaluation-reasons.
• pulling-the-data.md §1 corrects the active bullet and adds a dedicated evaluation_runtime field bullet, which is the field the runbook previously never told the agent to read.
• customer-reply.md changes the flag_not_found customer-facing mapping to "resolve the cause first: either turned off, or not available to that kind of SDK" instead of a flat "turned off".

[haacked]

blocking: The activity-log row does carry the flag key, so this rules out the one query that answers "what changed in this project last Tuesday". Every feature-flag activity write sets Detail(name=<the flag key>) — the model-signal path, evaluation-context changes, version bumps, and bulk delete. The list endpoint serializes fields = "__all__", and the MCP tool's response allowlist names both detail.name and item_id. It's the key field, not the display name.

An agent reading this believes a project-wide sweep returns rows it can't attribute to a flag, so it won't run the sweep for the ticket shape where the sweep is the right first move: the customer knows something changed but can't name the flag. The investigation isn't blocked, it just takes the slow route of enumerating flags and calling the per-flag endpoint N times.

The endpoint also filters on that field, so the sentence works with the premise replaced:

posthog:advanced-activity-logs-list covers the same rows across the project, carrying item_id (the
flag's numeric ID) and detail.name (its key) — sweep with scopes: ["FeatureFlag"] plus search_text
or detail_filters when the customer can't name the flag, then come back here for the field-level diff.

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. §4 now uses the replacement you proposed almost verbatim: it describes posthog:advanced-activity-logs-list as carrying both item_id (the flag's numeric ID) and detail.name (its key), and tells the agent to sweep project-wide with scopes: ["FeatureFlag"] plus search_text or detail_filters when the customer can't name the flag, then come back to the per-flag endpoint for the field-level diff. The earlier premise that the sweep returns unattributable rows is gone, so the project-wide sweep is now the recommended first move for the ticket shape you described.

[haacked]

blocking: The holdout formula reads as one thing and means another, and this section only runs when you can't check it against the instance. The two bullets above both spell the key out as sha1(f"{flag_key}.{identifier}"), so "prefix holdout-" naturally reads as prepending to that. The code doesn't do that: get_holdout_hash calls calculate_hash("holdout-", &hashed_identifier, ""), and calculate_hash builds its key as prefix + identifier + salt. So there's no flag key and no dot.

That omission is the whole point of a holdout: a user has to bucket identically across every flag in it, which can't happen if the flag key is in the hash. Reach this section because the instance is unreachable, recompute with the key in, and you get a plausible number and tell the customer the wrong thing with confidence. Worth fixing precisely because the bullet two lines up already warns that mixing up the salt is "the classic reimplementation bug".

The comparison direction is unstated too. A user is in the holdout when the hash is at or below the threshold.

• Holdout: h = sha1(f"holdout-{identifier}"), same 15-hex-digit conversion. The flag key is
  not in this one — that's what makes a holdout consistent across every flag in it. Empty salt.
  The user is in the holdout if h <= filters.holdout.exclusion_percentage / 100.

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. The holdout bullet in §5 now uses the correction you proposed verbatim: the hash is sha1(f"holdout-{identifier}") with the flag key dropped and no dot, it calls out explicitly that the flag key is not in this hash (which is what keeps a user bucketed identically across every flag in the holdout), and it states the comparison direction — the user is in the holdout when h <= filters.holdout.exclusion_percentage / 100. So an agent recomputing offline from an unreachable instance now gets the same answer the service would.

[haacked]

blocking: This states the entitlement rule but gives the agent no way to run it, and the workflow never stops to check. Steps 1 through 3 go from "extract project ID from the ticket" (line 42) straight to feature-flag-get-definition-by-key (line 46) and feature-flags-evaluation-reasons-retrieve (line 48). Nothing between them references this section, and neither reference file carries a requester or authorization step either. With staff-broad access every read succeeds, so the cross-tenant case this paragraph names — a customer pasting another tenant's project ID — plays out exactly as described, and the reply hands back that tenant's flag config and person properties.

This repo already solved it for the same class of skill. triaging-warehouse-sync-tickets has a "Verify the requester is actually authorized for that team" section with four numbered steps, a named query, explicit escalation on mismatch, and the instruction to run it once per ticket before Step 1. It also answers the "a human operator is in the loop" objection in its own words: the triage flow has no other gate in front of fleet-wide production access.

Mirror that as a numbered step before the first read, rather than leaving the rule 160 lines below the workflow. Two constraints if you reach for posthog:org-members-list: it resolves the target organization only after switch-project to the ticket's project, which this skill's tool list doesn't mention, and it's gated behind the organization_member:read scope. A ticket email matched against a member list is corroboration rather than proof, so the step needs the same escalate-when-anything-looks-off posture the warehouse skill uses.

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. The entitlement rule is now a numbered workflow step, not a paragraph 160 lines down. Step 2, "Check the requester belongs to the project, before the first read," sits between parsing the ticket (step 1) and the first read (step 3), and mirrors the triaging-warehouse-sync-tickets pattern you pointed at:

• It uses posthog:switch-project { projectId } to scope to the ticket's project first — which is what resolves the target organization — then posthog:org-members-list (noted as requiring the organization_member:read scope) to look for the requester's email among the members.
• It states plainly that a pasted project ID is a way to find the project, never authorization to read it, and that a cross-tenant paste must not get flag config, person properties, or evaluation results back.
• It treats even a clean email match as corroboration rather than proof, and enumerates the escalate-instead-of-read cases (email absent, empty or self-only member list, missing scope, or a fuzzy search_match_type: similar hit), with a lower bar for a flag mutation.
• It ends with "Once per ticket, before every read below," and step 1 now pulls the requester's email via conversations-tickets-retrieve before the switch. The "Access for debugging" section and pulling-the-data.md both now point back to this step.

[haacked]

suggestion: The identifier rule misses device-ID bucketing, so the offline hash silently returns the wrong answer for any flag using it. hashed_identifier returns the device ID rather than the distinct_id when the flag sets bucketing_identifier: "device_id", and it's a customer-settable choice — the serializer offers distinct_id or device_id.

Line 140 already warns that ensure_experience_continuity makes this section unreliable, so this is the same class of caveat and just needs saying:

identifier is the distinct_id, the group key for a group-aggregated flag, or the device ID when
the flag sets bucketing_identifier: "device_id" — read that field with the rest of the config in §1.
SHA1 isn't in HogQL's whitelist, so this runs outside the database.

Worth adding bucketing_identifier to the §1 field list next to ensure_experience_continuity, since §5 now depends on it.

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. Both of your suggestions are in the current version:

• The §5 identifier rule now spells out the device-ID case: identifier is the distinct_id, the group key for a group-aggregated flag, or the device ID when the flag sets bucketing_identifier: "device_id", with a pointer to read that field in §1. So the offline hash no longer silently uses the wrong identifier for device-ID-bucketed flags.
• §1 now lists bucketing_identifier (distinct_id default or device_id) right next to ensure_experience_continuity, noting that §5 depends on it and that device_id is incompatible with experience continuity.

[haacked]

suggestion: This preamble reads as "run all five sections on arrival", which fires the 7-day ClickHouse aggregation in §3 on every flag ticket, including the majority that §2 already answered. SKILL.md disagrees with it: step 3 calls the server-side reproduction "the step that usually answers it", and step 4 routes on what the server said.

The file already knows how to gate a section. §5 is titled "(fallback only)" and opens "You rarely need this." §3 has no equivalent, so it reads as mandatory. Replacing both lines of the preamble:

Run this read-only before diagnosing or asking the customer anything. §1 and §2 produce the config and
the reproduced evaluation, and you need both on every ticket. §3 costs a scan of the project's events,
so reach for it only when §2 didn't settle the question, or when the ticket is about usage itself — "I
see no `$feature_flag_called`", "works locally but not in production", or "the value flipped".

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. The preamble now reads exactly as you proposed: it keeps §1 and §2 as the every-ticket steps and gates §3 explicitly — "§3 costs a scan of the project's events, so reach for it only when §2 didn't settle the question, or when the ticket is about usage itself." So the 7-day ClickHouse aggregation no longer reads as mandatory on arrival, matching the SKILL's stance that the server-side reproduction usually answers the ticket.

[haacked]

suggestion: count(DISTINCT person_id) is the one line here that can fail outright on a busy flag, and uniq gives you the same answer for this purpose.

Two things stack up. person_id isn't a physical column on events — under persons-on-events it resolves through the person-overrides join, adding a LEFT JOIN over the team's whole overrides set. And count(DISTINCT …) compiles to uniqExact, which holds every distinct person UUID in a hash table for the query's duration, so a flag doing millions of evaluations a week lands in the hundreds of megabytes across the value/reason/lib groups.

uniq is an approximate counter with roughly 0.5% error and a few kilobytes of state, and it's already whitelisted in HogQL. For a diagnostic that exists to say "about how many users got each value", exactness buys nothing.

Suggested change

	count(DISTINCT person_id) AS persons
	uniq(person_id) AS persons

If you want the join gone too, uniq(distinct_id) reads a column physically on events — worth a sentence in the prose so an agent knows the cheaper option exists when a query times out.

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. Both of your points are in the current version:

• The runnable query now uses uniq(person_id) AS persons instead of count(DISTINCT person_id), so it no longer compiles to uniqExact on a busy flag.
• The prose above it explains why (uniq is approximate, ~0.5% error, which is all a "roughly how many users got each value" diagnostic needs) and adds the cheaper fallback you suggested: if it still times out, uniq(distinct_id) reads a column physically on events and skips the person-overrides join that person_id resolves through.

The only remaining mention of count(DISTINCT person_id) is in that prose, where it's named as the thing to avoid.

[haacked]

suggestion: The escaping rule is correct but compressed into a form that can be read as two ordered passes, and applied that way it reintroduces the injection this paragraph is warning about. Run the quote rule then the backslash rule over the payload named two lines up and x' OR 1=1 -- becomes x\\' OR 1=1 --, which lexes as the literal x\ followed by executable SQL. Applied as a single pass over the input, the same value gives the safe x\' OR 1=1 --.

escape_param_clickhouse maps characters in one pass, so saying so costs one clause and closes the reading:

Suggested change

	`\'` and a backslash as `\\` inside a literal; apply that before substituting.
	`\'` and a backslash as `\\` inside a literal; apply both in a single pass over the value (never the
	quote rule and then the backslash rule over your own output) before substituting.

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. The escaping rule now states the single-pass requirement you proposed and adds the failure it prevents inline: "apply both in a single pass over the value (never the quote rule and then the backslash rule over your own output, which turns x' OR 1=1 -- into the literal x\ followed by live SQL) before substituting." So the two-ordered-passes reading that would reintroduce the injection is ruled out explicitly, with the concrete example showing why.

[haacked]

nit: This says to scope telemetry queries "to the requester's organization/team group" without naming the group key, while the sentence before it hands over a ready-made flag-key filter that the same line says matches other tenants' rows. Naming the concrete group property, or showing the WHERE clause with both filters, would make the guardrail as runnable as the thing it guards against.

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. The "Mind the instance" guidance now names the concrete group properties and shows the full filter, so the guardrail is as runnable as the flag-key filter beside it. It states that PostHog tags its internal events with an organization group (the organization's ID) and a project group (the team's UUID, not its numeric ID), tells you to resolve the group-type index first, and gives the WHERE clause with both filters:

WHERE properties.$group_<organization_index> = '<organization_id>'
  AND properties.$feature_flag = '<flag-key>'

[haacked]

nit: A flag created before the holdout format change still carries a holdout_groups array alongside filters.holdout, since the backfill in posthog/migrations/1051_backfill_holdout_format.py adds the new key without removing the old one. Worth a parenthetical like the one super_groups gets two bullets up, so an agent looking at both keys doesn't quote the stale legacy percentage back to a customer.

[posthog]

Already addressed in commit cffc6c5 ("correct four claims and gate the project read"), which landed after this thread. The filters.holdout bullet now has the parenthetical you suggested, in the same style as the super_groups one above it: it notes a flag created before the holdout format change still carries a legacy holdout_groups array alongside filters.holdout because the backfill added the new key without removing the old one, and tells the agent to read filters.holdout and not quote the stale percentage from holdout_groups.

A new stamphog review started for this PR — the fresh verdict replaces this approval.

[veria-ai]

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 2 · PR risk: 0/10

[stamphog]

Gates already denied this PR (changes requested by haacked), and the diff still has multiple unresolved blocking review comments identifying concrete factual errors in the skill that would cause an agent to give customers wrong/harmful guidance — these have not been addressed on the current head.

• 👍 on the PR from greptile-apps[bot].
• Reviewer haacked requested changes; prerequisite gate failed
• Multiple unresolved blocking inline comments describing factual errors (flag_not_found misattribution, activity log key field, holdout hash formula, missing authorization/consent gating) that are not addressed in the current diff
• No independent assurance overriding these unresolved concerns

Gate mechanics and policy version

Gate		Result
prerequisites	✗	changes requested by: haacked
deny-list	✓	no deny categories matched
size	✓	0L, 0F substantive, 584L/3F incl. docs/generated/snapshots — within ceiling
tier	✓	T1-agent / T1d-complex (584L, 3F, single-area, feat)
stamphog 2.0.0b4		.stamphog/policy.yml @ cffc6c5 · reviewed head cffc6c5

[posthog]

🦔 ReviewHog reviewed this pull request

Found 0 must fix, 2 should fix, 1 consider.

Published 3 findings (view the review).

Resolved comments: 3 fixed, 13 already settled, 2 left for you · couldn't handle 2

[posthog]

ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

[posthog]

ReviewHog Report

Found 2 should fix, 1 consider.