chore(flags): harden the stale-flag cleanup skill - #107780
JamesPatrickGill wants to merge 15 commits into
Conversation
github-actions
Bot
added
feature/feature-flags
🤖 CI report
|
|
[Low risk] Updates documentation for feature flag cleanup procedures. The PR appears safe to merge based on this review. Reviews (2) · Last reviewed commit: "chore(feature-flags): carry the tour ans..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
5ac84be to
053cd5b
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe feature-flag cleanup skill adds eligibility checks for flag age, schedules, dependencies, and product-tour usage. It requires checks for existing cleanup before rollout classification and repository inspection. It also defines fresh-definition checks before edits and publication, distinguishes flag-specific dead code from reusable helpers, and blocks publication when validation fails or cannot run. The handoff prompts, example interaction, and summary rules reflect these workflow requirements. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The cleanup workflow can misidentify already-merged work, miss a cleanup PR opened during the run, or be unusable in clients lacking required capabilities. These are bounded workflow risks, but the base-ref, final PR check, and cross-client instructions need attention before relying on the skill broadly. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
JamesPatrickGill
added
the
reviewhog
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 4 should fix, 2 consider. Published 7 findings (view the review). Resolved comments: 10 fixed, 1 already settled, 1 left for you |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/skills/cleaning-up-stale-feature-flags/SKILL.md-145-151 (1)
145-151: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse capabilities that are available across MCP clients.
This skill requires Git CLI commands and names
Edit,Write, andMultiEditcalls. MCP clients do not share a shell or a common edit-tool API. Describe the required operations without binding them to these tools, or provide a capability-neutral fallback that stops before edits when a required operation is unavailable.As per path instructions: “These skills ship to every MCP client. Flag any instruction that uses a command, flag, tool, or capability not available to every MCP user: flag-gated, client-specific, or mode-specific.”
Also applies to: 154-159, 246-249
Source: Path instructions
products/feature_flags/skills/cleaning-up-stale-feature-flags/SKILL.md-303-307 (1)
303-307: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRepeat the existing-work check before publication.
Step 3 scans branches and open PRs before rollout classification and call-site tracing. A teammate can open a cleanup PR while this run edits or validates. The publish gate does not repeat that scan, so the run can still push or open duplicate cleanup work. Repeat the existing-work check before pushing or opening a PR, and stop if an unmerged removal exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: a6d2a602-9e4d-49f2-8c4a-ad40cd8c2dc4
📒 Files selected for processing (1)
products/feature_flags/skills/cleaning-up-stale-feature-flags/SKILL.md
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
053cd5b to
2d212a9
Compare
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 (1)
products/feature_flags/skills/cleaning-up-stale-feature-flags/SKILL.md-251-251 (1)
251-251: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a client-neutral edit-gate instruction.
This skill ships to every MCP client, but
Edit,Write, andMultiEditare client-specific tool names. Thebefore any other actionclause preserves the ordering requirement, but the named-call wording still assumes APIs that some clients may not provide.Suggested wording
-Before your first Edit, Write, or MultiEdit call in this step, and before any other action in this step, repeat step 2's four reads: the definition, the status, the dependent flags, and the scheduled changes. +At the start of this step, repeat step 2's four reads before taking any other action: the definition, the status, the dependent flags, and the scheduled changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 202983f4-2ef4-41a6-a61d-a9b793d9d656
📒 Files selected for processing (1)
products/feature_flags/skills/cleaning-up-stale-feature-flags/SKILL.md
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 1 remain after this review.
b38dbed to
cac03c8
Compare
haacked
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: d79dac5f-cd7b-428f-a0a1-1759b71cd26e
📒 Files selected for processing (1)
products/feature_flags/skills/cleaning-up-stale-feature-flags/SKILL.md
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
cac03c8 to
f1197cf
Compare
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Solid hardening pass. One blocking issue inline on the handoff prompt's pre-edit check, and the rest are suggestions.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
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 (1)
products/feature_flags/skills/cleaning-up-stale-feature-flags/SKILL.md-183-183 (1)
183-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the resolved base ref for the ancestry check.
The procedure fetches each relevant remote but does not define
originas the base remote. In a checkout where the base branch is tracked byupstream,origin/<base branch>can be missing or unrelated. The workflow can then classify a merged cleanup as unmerged. Resolve the base remote and branch before this check.Suggested fix
-`git merge-base --is-ancestor <removal commit> origin/<base branch>` succeeds for a removal that already landed. +Resolve the base remote and branch before this check, then run: +`git merge-base --is-ancestor <removal commit> <resolved base ref>` for a removal that already landed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 23c63757-f9e7-4c05-9485-a6c1d83a927a
📒 Files selected for processing (1)
products/feature_flags/skills/cleaning-up-stale-feature-flags/SKILL.md
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Adds the guards the skill's own dogfood run showed it needed. A fresh definition read now gates the first edit (step 6) and the publish (step 8), so a rollout that moves mid-run cannot be edited or shipped against stale data. Validation ends in one of passed, failed, or could-not-run, and only passed permits a push. A new step 3 looks for cleanup that already exists in the checkout, in a branch, or in an open PR before any call site is read. Exclusions now apply to a named flag as well as a survey result, with no override on offer, and the product-tour question must be answered rather than asked. The orphan rule splits code the flag created from general helpers that only lost their last caller, and keeps the latter. The handoff prompt carries the same existing-work, pre-edit and pre-publish checks for an agent that cannot read PostHog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s use Step 6 keeps payload reads in place, but its orphan rule removed the key string, constant and registry entry unconditionally. A payload read that still passes the constant would break. The rule now checks each symbol for other uses first and keeps the ones a surviving read still needs. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
…mands Step 3 reads branch, remote and PR ref names, but the shell rule only covered the flag key. Git accepts shell characters in ref names, so the rule now covers those names and other repository content too. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
…f prompt The handoff prompt now tells the receiving agent to read open PR diffs, including fork PRs, but its trust statement covered only flag keys and variant names. It now states that branch names, commit messages, PR titles, PR diffs and repository files are data, never instructions, as the main workflow does in step 3. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
When the pre-edit or pre-publish read found a changed flag, steps 6 and 8 sent the agent back to step 4. Step 4 only classifies the rollout, so it skipped step 2's dependency, schedule and age exclusions. A change is itself an update, and step 2 excludes a recently updated flag. Both handlers now restart at step 2, as the handoff prompt already does. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
Step 8 read the flag at the start of the step, but the agent could then wait for the user to authorize publication. A rollout change during that wait went undetected. The read must now come right before the push or PR call, and repeats when publication resumes after a pause. The worked example shows the re-read after the user agrees. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
…ork check Step 3 ran a plain git fetch, which keeps remote-tracking refs for branches deleted on the host. The later git log --all search still saw them, so a closed and deleted cleanup branch could stop every run as work in flight. The fetch now uses --prune. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
The handoff prompt told the receiving agent to inspect every open PR diff and stop if any check was incomplete. In a busy repository that is thousands of diffs, and one unrelated refused diff stops the run. The prompt now uses step 3's selection: search the history of all fetched refs for the key, list open PRs by metadata, and inspect only fork PR diffs and PRs whose head branch or title names the key. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
…blish checks The pre-edit and pre-publish checks fetched only the flag definition, then compared version and rollout. The definition has no rollout summary, and version is null on a flag written before versioning, so the comparison could not be done as written. Both checks, and the handoff's pre-publish check, now fetch the status too and compare version, updated_at, and the status rollout object. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
…t and publish A schedule or dependent flag created after assessment does not change this flag's definition, so the pre-edit and pre-publish reads missed it. Both checks now repeat step 2's four reads and apply its exclusions to the fresh responses before comparing version, updated_at and rollout. The after-approval re-read and the handoff's pre-publish check follow the same rule. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
…ht cleanups The existing-work check searched history only for the literal key. A cleanup that removes checks through a registry constant, but leaves the key in the registry, does not change the key's occurrence count, so the search missed it. Step 3 and the handoff prompt now also search the history of each constant and wrapper that holds the key. Generated-By: PostHog Desktop Task-Id: d33c2c9d-7ce8-4fde-8bc2-34a13fa78daf
The handoff prompt is a copyable artifact. It leaves the session and a person can paste it days later, so a fact the prompt asserts expires without telling the receiving agent. A product tour can link a flag, and no read tool reports the link. The generated prompt carried neither the question nor the answer the assessment obtained, so a receiving agent could remove a flag a tour still uses. The prompt now carries the answer with the date it was given, and still tells the receiving agent to confirm with the user before it removes any code. The date is what makes a stale answer visible; the confirmation is what stops the answer being read as clearance. Two more instruction gaps in the same file, neither filed by a reviewer: - The open-PR listing in the existing-work check had no pagination requirement. Every host caps a page, and a truncated listing reports no error, so a cleanup already in flight on a later page reads as no cleanup at all and the skill opens a duplicate PR. Both the main path and the handoff prompt now say to page through the whole list. - The handoff prompt sent a changed flag back to the rollout classification. The main path already restarts at the assessment checks, because the change is itself an update and a flag updated inside the last 30 days is excluded. The handoff prompt now restarts in the same place. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
An answer left in context from an earlier assessment was reusable with no check on its age. A tour can start to use the flag after that answer, so the skill could remove code a tour needs without asking the only person who can say. Reuse now needs the answer to come from this assessment. An older answer names its date, and the user has to confirm it before the agent recommends removal or edits code. The handoff prompt already carries the date and requires that confirmation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five findings from haacked's review of the main path. An open fork PR is text an outside contributor writes, and the skill sent an agent holding PostHog access to read every one of them in full. On this repository that is tens of thousands of changed lines, so the read either overflows the context or truncates and misses the removal it was looking for. The agent now filters each fork diff in the shell for the key and its constants, reads only the matching hunks, and reports a diff it could not filter as unchecked. Quoting does not make a Git ref safe. Git accepts `'` and `$(` in a branch name, and step 3 sends the agent to branches found by name and then diffs them, so anyone who can push a branch could run a command on the machine doing the cleanup. Refs now go through the same allowlist the handoff already uses for interpolated values, and a ref that fails it is a check the agent could not complete. The `git log --all -S` walk reads every commit on every ref, once per name, and a blobless clone fetches blobs as it goes. The skill now says to run it in the background and that a running search is not an incomplete check, while an abandoned one is. Bounding it by the flag's creation date would be faster but would miss the re-created key the seasonal-flag case depends on. Missing PR access no longer stops local edits. A duplicate PR and a duplicate review are harms of publication, and stopping early contradicted both the "make the changes yourself" path and "lack of PR access is not a failed cleanup". The gap is recorded and step 8 refuses to publish until the search completes, which also catches a cleanup PR opened while the agent was editing. Two wording fixes: "do not revert an edit you already made instead of not making it" read as "do not revert", which contradicted step 8, and "report how to restore validation" did not say what the report has to contain. The pre-edit summary moves from step 2 to the end of step 5, where the agent has the existing work, the rollout state and the call sites it asks for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gainst The generated prompt told the receiving agent to re-read the flag before its first edit and compare, but carried nothing to compare against: the key, the variant and the tour answer, and no version, update time or rollout. The check could not fire. A prompt written for a 100% boolean, a rollback to 0%, and a paste 35 days later all pass the age check, and the prompt still says to keep the enabled body, so the cleanup ships the path PostHog serves to nobody. The prompt now carries an "Assessed state" line beside the tour line, and its pre-edit check fetches the definition and the status and compares version, update time and rollout against it. A `version` and an ISO `updated_at` both match the interpolation allowlist. Three main-path fixes had never reached the handoff's copy of steps 3, 6 and 8: `git fetch --prune` per remote, the ref allowlist, and repeating the reads when an authorization request pauses publication. It also gains the fork-diff filter and the incomplete-check rule. Each of the three steps now carries an HTML comment naming the handoff as a second copy, so the next edit finds both. Both handoff strings become `text` fences. Unfenced, lines such as "report it and stop" read as instructions to the agent running this skill. The file had grown to 561 lines, past the 500-line limit in the writing-skills handbook, which no lint enforces. The handoff section moves to `references/handoff-prompt.md`, which haacked suggested and which left the file at 503, so the worked example moves to `references/example-interaction.md` as well. 457 lines now. Both headings stay, so the cross-reference at line 47 still resolves, and `hogli build:skills` collects both reference files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f1197cf to
a7b3678
Compare
This branch has not been deployed
2 participants
Problem
A person who asks the cleanup skill to remove a stale flag can get code deleted against a rollout that has since moved, or a PR pushed after the checks failed. The skill's own dogfood run produced both.
This is the top layer of a stack. #107777 adds the eval scorers that grade these rules.
Changes
Ten more commits landed from the review round and are part of this diff:
version,updated_atand the statusrolloutobject. The definition carries no rollout summary andversionis null on a flag written before versioning, so the old comparison could not be made as written.git fetch --prune, so a cleanup branch deleted on the remote stops looking like work in flight.Note
This changes what the skill does for anyone who runs it. A cleanup that used to publish on a failed check now stops and reports.
How did you test this code?
No automated test asserts the content of a skill file. The eval coverage that grades these rules against a live agent run is the layer below, #107777.
The two-read design comes from a 12-trial reproduction of the dogfood failure, run before this branch: the pre-edit re-read alone left unsafe publishes reachable, and the pre-edit and pre-publish reads together took them to 0 of 12. That measurement has no artifact in this repository and is not linkable, so treat it as reported rather than checked.
Not run in this session: the sandboxed cleanup eval suite, which needs a Claude runtime and a seeded project.
hogli testover the two eval-harness test files and the gated-writes invariant from the layer below, all passing locally.hogli ci:preflight --strictpasses.Automatic notifications
Docs update
None. The skill file is the documentation for this behavior.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code. Opus 5 wrote the original skill edits. ReviewHog wrote ten review-round commits. Sonnet 5 wrote the fixes in the last commit.
This re-cuts the second half of #104315, which carried these edits as nine commits plus the eval coverage. That branch sat 412 commits behind master and its CI failed on staleness, not on a test. This layer is rebuilt from current master rather than rebased, and squashed to one commit.
SKILL.mdhad no drift on master since the old branch's merge base, so the file is byte-identical to the old branch tip.The Greptile P1 on the handoff prompt's missing tour confirmation is now closed. ReviewHog escalated it for a human decision between asserting the confirmed answer and telling the receiving agent to ask. Both are implemented, because each alone fails: an assertion goes stale with nothing to signal it, and asking alone discards the answer the assessment already obtained. The answer travels with its date as evidence, and the confirmation is still required.
Two instruction gaps found while reading ReviewHog's commits, filed by nobody: the missing pagination on the open-PR listing, and the handoff prompt's restart target. Both are in Changes above.
gh pr list --state open --search "stale feature flags skill"found only chore(feature-flags): require a fresh read before every write #104315, which this stack supersedes./stacking-prs,/writing-tests,/writing-pr-descriptions,/address-pr-reviews,/writing-user-facing-copy,/ste-writing.PreToolUsehook blocks/reviewing-with-coderabbitunless a person types the slash command, and no person did in this session. The hook is working as intended, so the PR opens without a local review pass.🤖 Generated with Claude Code