feat(skills): add cull-feature-flags skill - #387
Conversation
skill half of `wizard cull-feature-flags`. the wizard does the deterministic work (scan, fetch, classify, seed the ledger) so this skill only verifies each proposed row at its call site, asks once, applies what was approved and writes the report. config + description only here, steps follow. one variant per framework the wizard scanner supports (nextjs for now), keyed by the wizard detection id the same way integration/config.yaml does it.
step 1 reads each pending ledger row at its file:line and either confirms the proposed action or downgrades it to keep (never the other way round). step 2 is one wizard_ask over the confirmed rows, decline first, then code edit before the posthog disable so a failed edit never leaves a disabled flag behind live code.
writes posthog-feature-flag-cull-report.md from the ledger: full findings table first (stale / warning / healthy per flag) so a report-only run still gives something to act on by hand, then applied, manual action, failed and kept. terminal step.
the consent question now says exactly what happens to a picked flag (disabled in posthog, check removed in code) and that both are reversible. the report gets an undo section: git checkout for the touched files (the wizard refuses to start on a dirty tree, so every change is its own) and a re-enable link per disabled flag.
the wizard now aborts before the run when it cannot read the project's flags, so the report never sees a half-seeded ledger.
ledger areas are now the display names the wizard seeds (Rolled out, Never enabled, Archived in PostHog, ...) instead of bucket ids, defined once in the description. step 1 emits a status line per flag and resolves each row as soon as it is decided so the run screen moves while the agent works.
…g flags already off first live run: "apply nothing" was one checkbox among the flags, easy to miss. consent is still one wizard_ask call, now two questions: a single choice (report only first, then cull) and the flag multi-select. report only wins whatever the list holds. also skips the posthog disable for archived and already-disabled rows; the slide already promised nothing changes in posthog for those.
"applied" read as process, not outcome. the report sections are now Culled / Left for you / Failed / Kept, the status lines say culling, and the ledger marker the wizard reads is "; culled".
gewenyu99
left a comment
There was a problem hiding this comment.
On vibes this looks solid
|
|
||
| ## Read the ledger | ||
|
|
||
| `Read` `.posthog-audit-checks.json` at the project root. |
There was a problem hiding this comment.
Mmmm creative use of this ledger
There was a problem hiding this comment.
On purpose, the ledger already had the tui and the resolve tool. Open to a rename.
second live run said "0 left for you" while the ledger and report had 1. the closing status now derives its three counts from the ledger rows.
| 2. `id: "flags"`, `kind: "multi"`, prompt verbatim: | ||
|
|
||
| > Pick the flags to cull. Each one gets disabled in PostHog (re-enable any time from the flag page) and its check removed from code (revert with git). | ||
|
|
||
| One option per row: label `[<area>] <key>: <proposed action from label>`, value `<key>`. | ||
|
|
||
| If the call errors (non-interactive host, cap reached), treat it as report-only. Do not retry more than once. | ||
|
|
||
| `mode` is `report-only`: nothing is approved, whatever `flags` holds. Otherwise the approved rows are exactly the `flags` answer. | ||
|
|
||
| Rows not approved: resolve to `status: "pass"` with `details` = seeded details plus `; declined by user`. |
There was a problem hiding this comment.
this is v cool and powerful but it's hard to navigate. we're lacking a lot of UI components to work with, so this isn't a huge priority, but how can we break up the culling selection?
more importantly, how can we use that moment to explain/teach the developer why this flag should be culled (stale, dead code, unreferenced, etc.)?
There was a problem hiding this comment.
Agreed. For now each flag carries a one-line why and effect and the list is seeded in bucket order. Testing a few UI options for the pick itself, per-bucket questions being one, before settling on a shape.
The wizard prompt shrinks to facts only, so the verify step now owns the truncation rule: Unreferenced and Comment only rows on a truncated scan are kept with a reason, never proposed and never grepped for.
sarahxsanders
left a comment
There was a problem hiding this comment.
this is great!! the guardrails in here are great, looks like you're keeping the model from calls it's genuinely bad at lolol
some tips (and these really are just nits):
- think about what might look like a disabled flag but really isn't, and build in some guardrails there too
- maybe some verification/typechecking/linting for things you want verified
overall, it's nice :) these are really just super fine nits
review asked for real verification instead of "parses by eye", and it was right: a bad edit would have gone straight to a posthog disable. the apply step now edits every approved row, runs the project's own lint and typecheck on just those files (same pattern as the migrate skill), and only then disables flags for rows whose files pass. a row that still fails resolves to error and never reaches posthog. that also broke the cull section into three passes with their own lists (edit, verify, disable and resolve), since one numbered list spanning per-row and once-per-run steps read as "lint after every flag". the 0% bucket is now "off for everyone" to match the wizard: the api has no history, so a rollback and a never-shipped flag look the same. the verify table treats anything that reads like a kill switch as keep. Confidence: high Scope-risk: narrow
the flag picker was a flat list of "[bucket] key: action" labels and a reviewer rightly said it teaches nothing. wizard_ask already renders a dimmed description under each multi-select option, so every flag now carries one sentence: the bucket and why it landed there, then what culling does to the code and the flag. label is the bare key. options stay in ledger order, which the wizard now seeds grouped by bucket. Confidence: high Scope-risk: narrow
two reviewers hit the same thing: the screen looked frozen before the pick and dead after it. the skill went silent between reads and edits. verify now emits a status per call-site file and "ready to ask" before the prompt; the cull pass emits one per edit, one for the typecheck, and one per posthog disable. cheap speed-ups while in there: archived, disabled and deleted rows read only their first call site (the table already calls them always safe), and the disable tool is discovered once before the loop instead of search plus info per flag. Confidence: high Scope-risk: narrow
"culled 6" next to "4 disabled" read like two flags fell through. they did not: archived and deleted flags have nothing to disable, the code check is all that goes. the culled table now has a code column and a posthog column, with the posthog cell fixed per bucket, and the summary counts how many culled flags had a live flag to switch off. Confidence: high Scope-risk: narrow
|
Thanks. The skill now runs lint and typecheck on the touched files before any disable, and failing rows never reach PostHog. The false-disabled guards are a good call, same answer as on the wizard PR: worth doing properly, not today. |
… prompt one flat multi-select of eight flags mixed 100% features with 0% flags that might be rollback levers, and its descriptions repeated the action per row. four role-played developers refused to run it that way. the one wizard_ask now asks one pick list per lane, only for lanes with proposals, rolled out and off for everyone as separate questions with the rollback caveat on the latter, then the report-only or cull choice last as the confirm step. every option's description is that flag's plan: reason, the branch kept in plain words, every call site, and whether posthog changes. nothing is preselected. report-only emits its own status line so the wizard can show the outcome. Confidence: high Scope-risk: narrow
gewenyu99
left a comment
There was a problem hiding this comment.
There's definitely a few issue here, especially with the order which we remove flags after disabling them in code. There's a whole deploy process 🤔
| For each approved row whose code edit and file verification passed, in ledger order: | ||
|
|
||
| 1. `Deleted in PostHog` has no flag, and `Archived in PostHog` and `Disabled in PostHog` are already off: these rows need no PostHog call, go straight to step 4. | ||
| 2. Emit `[STATUS] Disabling <key> in PostHog`, then run only `exec({ "command": "call <tool_name> <json> })` with the flag key or id from `details`. |
There was a problem hiding this comment.
wait, the local changes haven't been deployed yet? This would break production, no?
| |---|---|---| | ||
| | `Rolled out` | the call site is a boolean check whose true branch is the current behaviour | the flag gates something that must stay switchable (kill switch, ops toggle named as such) | | ||
| | `Off for everyone` | the call site is a boolean check whose false branch is the current behaviour | the feature is clearly mid-build (recent scaffolding, TODOs pointing at it), or the flag or its call site reads like a kill switch or rollback (name, comment, a recent change back to 0%) | | ||
| | `Archived in PostHog`, `Disabled in PostHog` | the call site is a boolean check; keep the false branch | never, these are always safe to propose | |
There was a problem hiding this comment.
never, these are always safe to propose
Not always true. Sometimes we temporarily disable a flag. I think premise is correct but wording can be tweaked
| Once per run, after every approved row has been edited: | ||
|
|
||
| 1. Emit `[STATUS] Type checking <n> files`. | ||
| 2. `Read` `package.json` for the project's lint and typecheck scripts. Run them only on the files edited in this session, never across the whole project. Capture stdout and stderr; truncate long output to the failure region. |
There was a problem hiding this comment.
In this case, this isn't quite true actually. Broken imports can happen cross package, for verification like type checks, definitely run the whole project
| @@ -0,0 +1,62 @@ | |||
| # Cull stale PostHog feature flags in a {display_name} project | |||
|
|
|||
| This skill removes feature flags that have outlived their purpose: rolled out to everyone, off for everyone, archived or deleted in PostHog but still checked in code, or defined in PostHog and never evaluated anywhere. The wizard already did the detection before you started: it scanned the source tree for flag calls, fetched the project's flags, classified every flag with plain rules, and wrote one row per flag into `.posthog-audit-checks.json` at the project root. That ledger is the ground truth. You never grep for flags, never re-classify a row, and never promote a healthy flag into a removal. | |||
There was a problem hiding this comment.
I understand the intent, but disabling greps outright probably:
- really messes with the ability for the agent to properly carry out the task to independently verify. In the wild, flags are weird and projects are weird
- The agent will just use another tool call xD
I think we need to restrict a slightly different way
Skill half of
wizard cull-feature-flags; the wizard PR (PostHog/wizard#1210) carries thecommand and the deterministic scan and classification. Opening as a draft.
The commits are chunked so the steps can be read in order.
Shape
A three-step docs-only chain with one Next.js variant, keyed by framework the
same way
integration/config.yamldoes it.file:line, confirm theproposed action or downgrade the row to keep (never the other way round),
and resolve rows one at a time so the run screen moves.
wizard_askcall with two questions: "Report only, changenothing" as its own single choice first, then the flag multi-select.
Approved rows get the code edit first, then
feature-flag-disablethroughexec(search, info, call). The skill never deletes or archives, and skipsthe disable for flags that are already archived or off.
report-only run still gives something to act on, then Culled, Left for you,
Failed and Kept, plus the undo block.
The skill never classifies anything itself. The ledger the wizard seeds is
ground truth. Areas are the human bucket names, defined once in the
description: Rolled out, Never enabled, Archived in PostHog, Disabled in
PostHog, Unreferenced, Comment only, Dead code, Deleted in PostHog, Many call
sites, Healthy.
Ships as
role: skillper the promotion criterion in CONTRIBUTING; the wizardregisters the flat command on its side.
Consent by lane (second round)
Reviewers would not run a flat multi-select of eight flags with pre-checked boxes and 100% kill-switch-looking flags next to 0% rollback levers. Step 2's single
wizard_asknow asks one pick list per lane, only for lanes with proposals: Rolled out; Off for everyone (with the rollback caveat, keeping costs nothing); Archived, Disabled or Deleted in PostHog; Unreferenced, Comment only or Dead code (says only this repository was scanned). Then the report-only or cull choice last, as the confirm step. Every option's description is that flag's plan: reason, the branch kept in plain words, every call site, and "disables the flag" or "no PostHog change". Nothing is pre-selected. Report only emits its own status line so the wizard shows the outcome. Zero proposals skip the ask entirely. The lane table in the description is shared word for word with the wizard's run screen (PostHog/wizard#1210).Testing
npm testandnpm run buildare green; the zip carriesSKILL.mdplus thethree references with the preamble and continuation links in place. One
interactive run against a planted Next.js repo
(https://github.com/johncwaters/nextjs-stale-flags, seed it into your own
project per its README): 9 rows verified, one consent
prompt, 8 culled, 0 failed, and the report's findings matched the fixture's
expected verdicts 19/19.
Running it locally