feat(cull-feature-flags): cull stale feature flags from a repo, on consent - #1210
feat(cull-feature-flags): cull stale feature flags from a repo, on consent#1210johncwaters wants to merge 29 commits into
Conversation
new flat command that runs the cull-feature-flags skill. nothing clever yet, just the wiring so `wizard cull-feature-flags` resolves and the program shows up in the registry (scan + classify land in the next commits). reuses the audit-run screen so the ledger rows show up live in the tui instead of a blank spinner. scopes borrow AGENT_SKILL_SCOPE_ADDITIONS since the skill needs feature_flag:write to disable flags. skill id is the nextjs variant for now, framework resolution comes with the deferred run.
deterministic scan of the target repo for posthog flag api calls. regex over a bounded glob of js/ts files, comments stripped, literal first arg captures the key and anything else gets recorded as a dynamic site (so we know when a missing reference doesn't prove anything). also records getAllFlags usage (and treats keys read out of its result as real call sites), known keys that only show up quoted in a comment, and whether each call-site file is actually reachable (next.js convention entry or imported somewhere). the llm never greps for flags in this program, this is the ground truth it gets.
pulls the project's flags after auth (default list plus archived=true since the api hides archived flags by default, deleted ones never come back) and buckets each one against the scan. every bucket is a plain rule on rollout / active / archived / reachability / call-site count, no age, no llm. buckets follow the fixture manifest: fully-rolled-out, never-enabled, archived-still-referenced, disabled-but-referenced, unreferenced (plus a comment-only flavour), dead-code-reference, deleted-still-referenced, and a multi-callsite-no-wrapper warning. experiment, remote config and encrypted payload flags are guarded to healthy so we never propose removing something that is doing more than a bool check.
run is now deferred: dirty-tree check, framework detect (nextjs only for now, resolved to the matching context-mill variant), fetch, scan, classify, then seed .posthog-audit-checks.json with one row per flag before the agent starts. the prompt tells the skill the ledger is ground truth, one batched consent before any mutation, disable only (never delete or archive). two things that keep the undo promise honest: the run aborts on uncommitted changes (new PHW_DETECT_DIRTY_WORKING_TREE code) so "revert with git" never mixes with the user's own work, and the outro lists the touched files plus a re-enable link per disabled flag instead of dashboard links. fetch failure seeds zero rows and the skill goes report only instead of crashing the run.
the generic agent-skill intro says nothing about edits and the audit intro promises none happen, neither is true here. cull-intro says what will happen up front: nothing changes until you pick, flags are disabled never deleted, code edits are a git diff away from undo. one area slide per bucket on the run screen (the ledger seeds area = bucket) so the side pane explains what culling that flag does and how to undo it while the agent works through the rows.
…registry two exhaustiveness tests caught what the scaffold commit missed: every registered program needs a switchboard binding (default linear / anthropic here) and every screen id needs an e2e action (cull-intro confirms setup like the other intros).
the host copied POSTHOG_WIZARD_LOCAL_CONTEXT_MILL onto the session but never initialised the process-wide local-dev targets that getSkillsBaseUrl reads, so a headless run always fetched the published skill menu. now it calls initLocalDev from the same env vars before starting the tui.
the fixed e2e profile only knew the existing intro screens, so a headless cull run sat on cull-intro forever. it now confirms setup like the others. the host also mirrors POSTHOG_WIZARD_CAPTURE_AIO onto the session so a headless test run's llm and tool calls land in ai observability.
first headless run showed the deferred run resolves before the runner's own auth step on the ci / headless path, so credentials were still null, the fetch was skipped and every key in code got seeded as deleted-still-referenced. run now calls the runner's authenticate first (no-op once credentials exist) and aborts with a clear scope hint when the fetch fails instead of seeding a ledger built from half the data. pins the ordering with a run test: authenticate before fetch, dirty tree and unsupported framework abort before posthog is touched, fetch failure aborts before the ledger exists.
first interactive run showed the audit's "we've wrapped up the review" copy the moment verification finished, while consent and apply were still ahead, and long row labels wrapping into each other on the right. - left pane now carries cull stage copy: waiting for your pick, culling n more, writing the cull report (AuditAreaPane takes an optional wrapUp) - checks rows truncate instead of wrapping - ledger areas are display names (Rolled out, Never enabled, Dead code...) so the audit plan tab and the slides read as english, and the action phrases are short enough to fit a row and the consent overlay - details drop the first call site (the row's file already has it) and the "posthog status" prefix
…sthog line the first live outro listed a truncated flag url per disabled flag and put untracked files (.claude/, the ledger, the report itself) in the git checkout line. the revert line now covers tracked changes only and the posthog side is one sentence; the report already carries every flag link.
./posthog-feature-flag-cull-report.md is relative to --install-dir, which reads wrong when the wizard runs from another directory. the outro now prints the full path.
matches the skill's new ledger marker ("; culled") and outcome wording in
the outro and the run screen's stage copy.
it rode along with the cull-intro e2e fix but has nothing to do with the program. observability for headless runs can come in on its own.
every wording tweak on the prompt and the outro had to edit this file (five of the last six commits). the test now checks what has to hold: the ledger path and each area show up in the prompt, the caveats only appear when the scan hit them, and a cull produces one undo step for code (naming the touched files) and one for posthog.
gewenyu99
left a comment
gewenyu99
left a comment
There was a problem hiding this comment.
I like where your head's at on vibes. Taking a deeper look and running it before bringing more feedback in
| ); | ||
| } | ||
|
|
||
| export function buildCullPrompt(input: CullPromptInput): string { |
There was a problem hiding this comment.
I would think about if the context here should really be in this repo or context mill 🤔
There was a problem hiding this comment.
Agreed, moved. Prompt carries facts, skill carries rules.
| projectId: number, | ||
| query: string, | ||
| ): Promise<FeatureFlag[]> { | ||
| const endpoint = `/api/projects/${projectId}/feature_flags/`; |
There was a problem hiding this comment.
This is a very interesting design choice 🤔
I like where you're going with this, but I'd think about how this would scale (not smthing to address now)
Hundreds of flags in the ledger could get quite messy and quite challenging to display. I also wonder how well the agent would handle a large list.
There was a problem hiding this comment.
yea this is cool. we normally rely on the MCP and prompting to agent to grab data at its discretion, but this makes way more sense to keep the wizard program on rails
There was a problem hiding this comment.
Fair, follow-up: seed only actionable rows, page the fetch, cap proposals.
| function isNeverEnabled(flag: FeatureFlag): boolean { | ||
| const groups = flag.filters?.groups ?? []; | ||
| if (groups.length === 0) return false; | ||
| return groups.every((group) => group.rollout_percentage === 0); |
There was a problem hiding this comment.
Hmm some gut checks here.
Currently at 0% vs it's never been turned on have different implications. Just as a callout
There was a problem hiding this comment.
Right, the list api has no history. Renamed it "Off for everyone" and the skill keeps anything that reads like a kill switch. A real split needs the activity endpoint.
| return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| } | ||
|
|
||
| function importedModuleNames(source: string): string[] { |
There was a problem hiding this comment.
This feels really fragile to me and really really really tough to later scale to other frameworks. This will also miss a lot of side effect imports and special files like app/sitemap.ts
There was a problem hiding this comment.
Agreed. Every next.js file convention is a root now and bare side-effect imports are matched. Basename matching stays until a resolver lands; other frameworks would add a root list, not a heuristic.
| projectId: number, | ||
| query: string, | ||
| ): Promise<FeatureFlag[]> { | ||
| const endpoint = `/api/projects/${projectId}/feature_flags/`; |
There was a problem hiding this comment.
yea this is cool. we normally rely on the MCP and prompting to agent to grab data at its discretion, but this makes way more sense to keep the wizard program on rails
| 'events-audit': DEFAULT_BINDING, | ||
| 'posthog-doctor': DEFAULT_BINDING, | ||
| 'web-analytics-doctor': DEFAULT_BINDING, | ||
| 'cull-feature-flags': DEFAULT_BINDING, |
There was a problem hiding this comment.
the program is a bit slow, understandable since it scans and analyzes all feature flags. any thoughts on what model or harness you might use instead of the defaults?
There was a problem hiding this comment.
Default binding this pass. Cut the reads and the per-flag tool discovery instead, and added a learn deck to cover the wait. Harness or model swap is worth a measured run later.
… rules The seed prompt repeated five rules the skill already states, so the two copies would drift on their own release cycles. The prompt now sends only what the host knows: the ledger, per-area counts, and a Scan facts block that always prints bulk evaluation, dynamic key sites, and truncation as yes/no. The skill decides what those mean.
sarahxsanders
left a comment
sarahxsanders
left a comment
There was a problem hiding this comment.
nice job!! I like the deterministic work on the host with the agent verifying and applying. the dirty-tree gate + disable, never delete rules are good calls too, I am a biiiig fan of safety in our beloved wizard
got the program to run which is great :))
a few things:
it looks like there's a few paths that can disable a live flag, some guardrails here would be nice
for the wizard TUI itself (this is gonna span context mill and this code):
- when I got to the confirmation step I was a little confused about what I was supposed to do/overwhelmed with the context on that Learn tab - the screen like froze for me before giving me the option to choose
- lack of context in the Learn tab to help me orient and understand what I was looking at on the screen. we have some time with attention captured here, so I'd love to see what kind of information you can seed the user to help them learn here
- after confirmation, the wizard looks like it lagged for a bit, there was no visual progress on the screen besides the status tab. and the status tab wasn't moving. I thought it was broken
- runs a little slow for me, so if you have time I'd love to see what performance gains you can add (I also confirmed every single one lolol so maybe this was my bad)
…s a root review caught two things. "never enabled" claimed history the api does not give us: a flag rolled back to 0% after an incident looks identical to one that never shipped. the area is now "off for everyone" and the row's details say it may be a rollback, so the skill verifies before it proposes. splitting the two for real needs the activity endpoint, later. reachability missed the metadata conventions (sitemap, robots, manifest, opengraph-image, twitter-image, icon, apple-icon) and the bare side effect import form (`import './register'`), so a flag checked only from those files landed in dead code. both are one regex each. require() and dynamic import() were already matched. the ledger is now seeded in bucket order so the grouped run list reads top to bottom as the agent moves through it. Confidence: high Scope-risk: narrow
reviewers said the run screen gives you nothing to read while the agent works and the area slides alone do not explain what is happening. the default program plays a content deck through LearnCard, but the audit run screen never read getContentBlocks, so skill programs on that screen had no deck at all. cull now ships its own deck (what a stale flag is, the four ways one goes stale, why the wizard scanned instead of asking the agent, what consent covers, how undo works) and the audit run screen plays it in the left pane for this program until it completes, then hands over to the area slides as before. other programs on the screen are untouched. Confidence: high Scope-risk: narrow
the outro counted every culled row with a flag id as "disabled in posthog", but the skill makes no posthog call for archived, disabled or deleted rows, so the undo line told people to re-enable flags nothing had touched. disabled now means culled in an area the skill disables. "left for you" counted every row that was not culled or failed, which swept the healthy flags in. it now counts declined rows only, and the zero counts stay out of the message. the undo copy says the three things reviewers asked for: each flag was its own unit, the tree was clean so git diff is all ours, and disabling kept the flag's rollout conditions. Confidence: high Scope-risk: narrow
ten buckets is the right granularity for the classifier and the report, and the wrong one for a run screen. every bucket falls into one of four lanes by what drifted: posthog decided for everyone, posthog is off but the code still asks, posthog has it but nothing asks, nothing to cull. the lane map is keyed by the ledger row's area because that is all the ui ever holds. the seed order now follows the lanes so the list reads top to bottom as the agent moves. Confidence: high Scope-risk: narrow
the left pane guessed the run's stage from ledger statuses alone. since the skill edits every approved row, type checks once and only then disables and resolves, rows stay proposed through the whole edit pass and the screen said "waiting for your pick" while files were changing. the store now reduces the skill's mandated status lines (culling, editing, type checking, disabling) into a small progress record as they arrive, because the status list itself keeps ten lines. cullPhase reads the ledger and that record and returns one of verify, pick, cull, report plus the copy for the pane. the cull copy is a pass card: which pass, which flag and file, and why the order is code, check, posthog. it only names a flag that exists in the ledger. esc gets its own outcome copy. Confidence: high Scope-risk: narrow
the right pane listed every ledger row under ten headings: forty lines on the fixture, never fit, and ten of the rows were healthy flags that never change. reviewers called the layout busy and they were right. cull now gets its own right pane grouped by lane, with healthy and suggestion rows folded into one footer line, five rows per lane before "+n more", and the rows carrying their reason (kept why, failed why). during the cull it shows which flag is being edited or disabled. a one-line stepper above the panes names the phase. the deck only plays while verifying, so a phase change preempts it. other programs keep the shared list untouched. a playground demo cycles the phases against a 19-row and a 400-row ledger. Confidence: medium Scope-risk: narrow
the first deck listed four ways a flag goes stale and used "winning branch" like the skill does. a junior reviewer did not know what a call site or a winning branch was, and nothing on screen told them. the deck now teaches one model in the same words the list uses: a flag lives in two places, posthog decides, the code asks, and the three lanes are the three ways that drifts. it defines call site, borrows the status peek and "press s" from the integration deck so people find the live narration, and says what culling and undo do. about 97 seconds. slides drop the consent and undo lines the deck now owns and say that only this repository was scanned. the deck test loops every program deck so both get the width guards. Confidence: high Scope-risk: narrow
|
Thanks for running it. Frozen before the pick and dead after were the same bug, the skill went quiet between reads; it now emits a status per file, typecheck, and disable. Added a learn deck. Guardrails on the live-flag paths are a great idea and would take more time to do properly than I have today; the gate for now is the approved list plus typechecked code before PostHog. |
the pass card named the flag and the file but not the reason, so during the cull you saw "culling server-rate-limit" with no idea what put it on the list. the card now adds one paragraph from the ledger row: the bucket and its rollout summary, which branch stays, and whether posthog is touched. the set of areas that disable a flag moves to classify so the outro and the card cannot disagree. Confidence: high Scope-risk: narrow
…to do" the folded footer said "10 healthy, 2 kept, 1 suggestion, nothing to do". the kept flags were kept for a reason and the suggestion asks for a wrapper, both of which are in the report. the footer now ends with "details in the report" whenever either count is nonzero, and keeps "nothing to do" for a footer that is healthy flags only. Confidence: high Scope-risk: narrow
the wizard's own "Culling stale feature flags..." spinner message matched the reducer's `^Culling (.+)$`, so the run screen sat in the edit pass from the first second and jumped straight from Verify to Cull once every row was verified, skipping the Pick screen while the ask overlay was still coming. flag keys never contain whitespace, so the reducer now only accepts a single token as the culled key.
…iting "Waiting for your pick" showed while the agent was still composing the wizard_ask call, so the screen read as stuck when nothing had opened yet. the pick card now says the list is being written and will open here, then repeats the nothing-changes-until-you-confirm promise.
…screen the pick and cull cards sat still for a minute or more while the agent composed the ask or edited files, and a still card reads as a hang. the stage copy now carries an isWorking flag and the pane draws a spinner before the title while it is set; the pick card also says the wait is usually a minute or two.
619784b to
36746f6
Compare
sarahxsanders
left a comment
sarahxsanders
left a comment
There was a problem hiding this comment.
this is just a high level question, but how would you have split the code to make it easier to review if say, this way the day to day on the team? really great calling it out, just curious how you'd approach it!
| healthy: 'healthy', | ||
| }; | ||
|
|
||
| function guardReason(flag: FeatureFlag): string | undefined { |
There was a problem hiding this comment.
niceeeee this is a nice safety consideration
good question, and honestly this PR is bigger than I'd open day to day. I'd split it into four PR's that each land on their own:
1 and 3 are independent so they'd go up in parallel. 2 depends on 1. I'd be open to better ways though, but I find smaller/focused pr's are the way to go. |
| @@ -188,6 +193,7 @@ export class WizardStore { | |||
| private $statusMessages = atom<string[]>([]); | |||
| private $statusExpanded = atom(false); | |||
| private $tasks = atom<TaskItem[]>([]); | |||
| private $cullProgress = atom<CullProgress>(INITIAL_CULL_PROGRESS); | |||
| private $eventPlan = atom<PlannedEvent[]>([]); | |||
| private $handoffText = atom<string | null>(null); | |||
| private $learnCardBlockIdx = atom(0); | |||
| { | ||
| id: 'cull-run', | ||
| label: 'Cull run', | ||
| component: <CullRunDemo />, | ||
| }, |
There was a problem hiding this comment.
yay love when we can test in the playground
gewenyu99
left a comment
gewenyu99
left a comment
There was a problem hiding this comment.
Some more, mainly around scanner.
I really think for us to do static code analysis and not use the LLM's brain, we need to AST tree parse it.
🤔 think about the implications
| continue; | ||
| } | ||
| const blockStart = line.indexOf('/*', index); | ||
| const lineStart = line.indexOf('//', index); |
There was a problem hiding this comment.
I think this will tread // inside strings as comments, there might be better regex you can use.
This will match const url = 'https://example.com'; const enabled = posthog.isFeatureEnabled('url-flag');
| ): void { | ||
| codeLines.forEach((code, lineIndex) => { | ||
| const line = lineIndex + 1; | ||
| for (const match of code.matchAll(LITERAL_CALL_RE)) { |
There was a problem hiding this comment.
There's a real issue with going with regex instead of AST tree sitting the scanner.
I think this will miss this for example if we're just scanning line by line. I think we'd definitely need to tree sit this
const enabled = posthog.isFeatureEnabled( // Scanner sees the function…
'checkout' // …but the key is on another line.
); // It never matches them together.
| cwd: installDir, | ||
| stdio: ['ignore', 'pipe', 'ignore'], | ||
| }).toString(); | ||
| } catch { |
There was a problem hiding this comment.
This will catch any failure and return []. For example, an invalid git directory will return as clean, but git crashing will do the same. Is there a better signal?
4 participants
This is the wizard half of a new
wizard cull-feature-flagscommand; thecontext-mill PR (PostHog/context-mill#387) carries the skill. Opening as a draft. The commits are
chunked on purpose so the process can be walked in order.
What it does
Finds feature flags that look done and removes them, but only the ones you
pick. The deterministic part runs on the host before the agent starts:
files, comments stripped, dynamic keys and
getAllFlagsrecorded, and areachability check per call-site file (Next.js convention entry or imported
somewhere).
archived=true, since the APIhides archived flags by default).
Buckets: Rolled out (100%, no conditions), Never enabled (0%), Archived in
PostHog, Disabled in PostHog, Unreferenced, Comment only, Dead code (the only
caller is a module nothing imports), Deleted in PostHog (code checks a key that
is gone), Many call sites (warning only), Healthy. Experiment, remote config
and encrypted-payload flags are guarded to Healthy.
The result is seeded into the existing audit ledger (
.posthog-audit-checks.json),one row per flag, so the audit run screen renders it live with a slide per
bucket. The skill then verifies each row at its call site, asks once (report
only is its own choice, followed by a multi-select), culls the approved rows
(code edit first, PostHog disable second) and writes the report.
Assurances that are code, not copy
PHW_DETECT_DIRTY_WORKING_TREE). This isstricter than every other program on purpose: a clean starting tree is what
makes
git checkout -- <files>a true one-step undo. No existing programgates on git state today.
report carry the undo recipe: one git revert line over the touched files and
a re-enable link per disabled flag.
reclassifies a row.
Run screen v2
Second round, after the first feedback (the layout was busy and the cull step was hard to follow):
Playground:
pnpm try --playground, tab "Cull run" (n/pphases,sstatus script,l400-row ledger) and tab "Learn deck". Live run against the fixture after these changes: 9 verified, 7 proposed, 2 kept, 5 consent questions, culls applied in three passes.Testing
Tested against a planted Next.js repo with 19 flags (8 stale, 1 warning,
10 healthy): https://github.com/johncwaters/nextjs-stale-flags. Its README
covers seeding the flags into your own project;
flags.manifest.jsonis theexpected verdict per flag and
scripts/verify-report.mjschecks a runagainst it. Host-side classification
matches the manifest 19/19. One full interactive run: 9 rows verified, one
consent prompt, 8 culled, 0 failed. The diff was 3 clean edits plus 1 deleted
dead module, 6 flags confirmed
active=falseover the API, healthy flagsuntouched.
pnpm testandpnpm buildare green.Running it locally
Needs the context-mill dev server on
:8765(pnpm devin that repo) and anOAuth login.
--capture-aiolands the run's LLM and tool calls in theproject's AI observability tab.
Example outro from the fixture run:
Notes for reviewers
--ciruns cannot drive the agent half:/api/wizard/gateway_token/accepts OAuth access tokens only, so a personal API key gets
"Invalid access token". The tui-host changes in this PR (local context-mill
env flag,
cull-introaction,capture-aiomirror) still make thedeterministic half snapshot-testable.
context-mill variant plus scanner patterns, no new command.
and this is not merging yet. README line ~252 ("auto-continues on git
warnings") describes a flow that no longer exists in
src/.--modeland parses the wizard log into one JSONL row per run are covered in the write-up instead.STALEstatus per flag (age based).That could feed in as an additional signal later.