chore(flags): add regression eval coverage for cleanup - #107777
JamesPatrickGill wants to merge 8 commits into
Conversation
|
Merging to
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 |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
products/posthog_ai/eval_harness/test/test_feature_flags_scorers.py |
24 | 172 |
products/feature_flags/evals/eval_cleanup_stale_flags.py |
18 | 92 |
products/feature_flags/evals/scorers.py |
11 | 164 |
products/posthog_ai/eval_harness/test/test_feature_flags_seeders.py |
4 | 30 |
products/feature_flags/evals/seeders.py |
3 | 34 |
This check does not block merging. It updates on every push and clears when the share drops.
github-actions
Bot
added
feature/feature-flags
|
[Low risk] Adds test cases and evaluation fixtures for feature flag cleanup. The PR appears safe to merge; no new actionable finding remains. Reviews (2) · Last reviewed commit: "chore(feature-flags): grade the pre-edit..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe cleanup evaluation suite adds recent-update and assessment-only cases. The no-reference case adds a tour-usage uncertainty check. A new seeder creates a full-rollout flag with a recent update timestamp. Scorer changes add pre-edit read validation and final-message questions. Tests cover scorer ordering and conditions, plus the seeder’s timestamp and classification. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The new cleanup cases can proceed, but the fresh-read scorer still has an ordering gap to address before a case relies on it. 🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Description checkExplanation The description clearly explains the problem, changes, testing, and non-user-facing impact. It omits the required Release status section and does not provide the required Agent context fields for Autonomy and Agent. Several agent definition-of-done items are also missing, including patch coverage, new-events schema status, and CodeRabbit CLI disposition. Resolution Add the Release status section with exactly one selected option. Add explicit Autonomy and Agent fields. Complete the Agent context with the required session link, patch-coverage result or justification, new-events schema result, and CodeRabbit CLI pass or skip disposition. Preserve the existing problem, changes, and testing details.
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
JamesPatrickGill
added
the
reviewhog
🦔 PostHog Review reviewed this pull requestNothing worth raising this time. Enjoy the moment: Not resolving comments: other pull requests are stacked on this branch A fix commit here would leave the stacked pull requests out of date, so the open threads stay with 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
🧹 Nitpick comments (2)
products/feature_flags/evals/scorers.py (1)
750-756: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffTreat this as a follow-up refactor, not a major issue.
FreshDefinitionReadBeforeEditcan accept two definition reads that occur before assessment. However, no current eval case setsexpected.fresh_definition_read_before_edit, so this scorer returnsNonefor every current case and cannot affect current eval results.Before enabling a case, expose the assessment boundary in the parsed log and require a successful seeded-flag read after that boundary and before the first edit. This is not a major issue or a low-effort fix for the current head.
products/feature_flags/evals/eval_cleanup_stale_flags.py (1)
181-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRequire the response to identify the recent update as the blocking reason.
_NO_CALL_SITEScan pass when the agent finds no call sites and makes no edits.NO_OVERRIDE_OFFERED_QUESTIONonly checks that the agent does not suggest bypassing the rule. It does not require the response to mention the recent update, so another assertion does not detect this miss.Suggested fix
- "Does the message below stick to that refusal without proposing an exception, override, or way " + "Does the message below identify the applicable blocking reason (the recent update in this case) " + "and stick to that refusal without proposing an exception, override, or way "
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 3cf291fc-373a-442f-8faa-dac6db4da6ca
📒 Files selected for processing (6)
posthog/test/repo_invariants/feature_flag_gated_writes_baseline.txtproducts/feature_flags/evals/eval_cleanup_stale_flags.pyproducts/feature_flags/evals/scorers.pyproducts/feature_flags/evals/seeders.pyproducts/posthog_ai/eval_harness/test/test_feature_flags_scorers.pyproducts/posthog_ai/eval_harness/test/test_feature_flags_seeders.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
283c6c8 to
0501e6d
Compare
HostHog preview —
|
HostHog preview —
|
haacked
changed the title
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 (2)
products/feature_flags/evals/scorers.py-814-815 (1)
814-815: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire the user's answer before recommending removal.
FinalMessageJudgepasses the question text directly to the model. The rejection clause only rejects messages that recommend removal “without asking.” A message can ask about tour usage and recommend removal in the same response. State explicitly that the assistant must wait for the user's answer.Suggested fix
- "to can say whether a product tour links this flag, so the assistant must ask the user before it " + "to can say whether a product tour links this flag, so the assistant must ask the user and wait for an answer before it " ... - "remove or archive without asking about tour usage is a no." + "remove or archive before the user answers about tour usage is a no."products/feature_flags/evals/scorers.py-770-770 (1)
770-770: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the final repository search as the freshness boundary.
min(search_positions)lets an unrelatedGrepend assessment before the seeded-flag search. A definition read between those searches can then score1.0.Use the last successful
GreporGlobbefore the first edit asassessment_ends_at, then require the seeded-flag definition read after that position. A separate flag-targeted search check is not needed: the search contract treats repository searches as boundary events, and_targets_seeded_flagintentionally accepts searches without flag fields.No current cleanup case enables this scorer, so the original major runtime-impact label is overstated. This remains a minor latent scorer defect for future opted-in cases.
🧹 Nitpick comments (1)
products/feature_flags/evals/scorers.py (1)
745-745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the new scorer and raw-log test contracts.
FreshDefinitionReadBeforeEdit._run_eval_syncuses baredicttypes and an untyped**kwargs. The new raw-log fixtures useAny, and_fresh_read_scorehas no return annotation. Define a separate raw-log tuple alias that supports both three- and four-element records, use it for the new sequences, parameterize the scorer mappings, and returnScore. Do not uselog_parser.ToolCall; that is the parsed Pydantic model, not the fixture tuple shape.Suggested fix
- def _run_eval_sync(self, output: dict | None, expected: dict | None = None, **kwargs) -> Score: + def _run_eval_sync( + self, + output: dict[str, object] | None, + expected: dict[str, object] | None = None, + **kwargs: object, + ) -> Score:+from products.posthog_ai.eval_harness.scorers.contract import Score + +RawLogCall = tuple[str, dict[str, object], object] | tuple[str, dict[str, object], object, str] + -_DEFINITION_READ: tuple[Any, ...] = ( +_DEFINITION_READ: RawLogCall = ( ... -_ASSESSMENT_READS: list[tuple[Any, ...]] = [ +_ASSESSMENT_READS: list[RawLogCall] = [ ... -_REPOSITORY_SEARCH: list[tuple[Any, ...]] = [ +_REPOSITORY_SEARCH: list[RawLogCall] = [ ... -_FIRST_EDIT: tuple[Any, ...] = ("Edit", {"file_path": "/repo/src/widget.js"}, "ok") +_FIRST_EDIT: RawLogCall = ("Edit", {"file_path": "/repo/src/widget.js"}, "ok") ... -_SECOND_DEFINITION_READ: tuple[Any, ...] = _DEFINITION_READ +_SECOND_DEFINITION_READ: RawLogCall = _DEFINITION_READ ... -def _fresh_read_score(calls: Sequence[tuple[Any, ...]], expected: dict | None): +def _fresh_read_score(calls: Sequence[RawLogCall], expected: dict[str, object] | None) -> Score:This is a recommended typing refactor, not a major runtime defect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 8b30001c-3dac-4f8b-b62f-7471e96c8089
📒 Files selected for processing (3)
products/feature_flags/evals/eval_cleanup_stale_flags.pyproducts/feature_flags/evals/scorers.pyproducts/posthog_ai/eval_harness/test/test_feature_flags_scorers.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
A few non-blocking suggestions inline.
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
I meant to approve the last review.
0501e6d to
a06044e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
products/feature_flags/evals/scorers.py (1)
744-744: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit types to the new scorer signatures.
The repository’s Python guidance applies to both signatures.
_run_eval_syncuses baredictparameters and an untyped**kwargs._fresh_read_scorehas no return annotation and still usesAnyin its call tuple. Define small aliases orTypedDicts for the observed payload shapes, annotate**kwargs, and declare the helper’s-> Scorereturn type. This is a recommended typing refactor, not a major issue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: c849cc1b-e4da-4f12-984e-1ba8a1b2d3bb
📒 Files selected for processing (5)
products/feature_flags/evals/eval_cleanup_stale_flags.pyproducts/feature_flags/evals/scorers.pyproducts/feature_flags/evals/seeders.pyproducts/posthog_ai/eval_harness/test/test_feature_flags_scorers.pyproducts/posthog_ai/eval_harness/test/test_feature_flags_seeders.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
a06044e to
57f1cd3
Compare
Adds the scorers, seeders and cases that grade the stale-flag cleanup skill against the failures its dogfood run found: a pre-edit definition re-read, a recency exclusion no override may lift, a tour-usage question the agent must ask and wait on, and an assess-only reply that claims no edit. FreshDefinitionReadBeforeEdit is wired into the suite but scores every case None until a fixture repo with real call sites exists, so its behavior is covered by unit tests over reconstructed call sequences. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
seed_recently_updated_flag creates a flag with active and filters set, which the gated-writes invariant reports as a new ungated write. The file is already an allowed eval-seeder path, so this adds the entry rather than routing a seeder through the approval facade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FreshDefinitionReadBeforeEdit counted the definition reads that landed before the first edit and passed a run with two or more. Two reads taken while assessing satisfy that count, so an agent that read twice and then edited without re-reading scored green. That is the regression the scorer exists to catch. It now uses the step 5 repository search as the divider between assessment and cleanup. The read must land after the first Grep or Glob call and before the first edit. A run that edits without searching fails, because nothing separates its reads. A read after the edit still does not count. Two judge questions also graded less than they claimed: - The recency judge accepted any refusal that offered no override. A refusal for an unrelated reason passed without showing the recency guard. It now requires the message to name the recent update as the reason for stopping. The scorer is renamed to recency_refusal_without_override. - The tour judge failed a compliant no-op. The case finds no repository references, so the correct answer is a no-op report, and the skill asks the tour question only before a removal recommendation. No case in this suite can reach such a recommendation, because no seeder can write call sites into the cloned repository. The judge now grades the claim: a report that stops at "nothing to change" passes, and one that calls the flag safe to remove without asking about tours fails. The scorer and module docstrings recorded which run found each regression. CLAUDE.md forbids change history in code, so that context moves to the PR body. Two test names carried the same history and now describe behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…helper `seed_recently_updated_flag` called `FeatureFlag.objects.create()` directly and needed a new line in the gated-write baseline, whose header says to shrink it and never grow it. The invariant scanner ignores an `.update()` that writes neither `active` nor `filters`, so routing creation through `_create_flag` and setting both dates in the follow-up `update()` removes the baseline entry. The seeder now also runs under the shared checks. It cannot join `SEEDERS`, which asserts `updated_at == created_at`, so `CLEANUP_SEEDERS` carries it into the stale, state-snapshot and codex-refusal tests, and it gains a row in the rollout-shape parameters. The state-snapshot check is the one that matters: `FlagStateUnchanged` scores `None` when a seed has no `state`, so a break there would silently switch off the mutation check for `recent_update_excluded_without_override`. Three tests in `TestSeedRecentlyUpdatedFlag` repeated parameterized ones and are gone. The `updated_at` window test stays, because no shared test covers it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…first read The divider was the first `Grep`/`Glob` before the first edit. The skill's "Establish scope" step already searches the repository for the key, before the definition is read, so `Grep, definition read, edit` put the assessment read after the divider and scored 1.0 without any pre-edit read. The divider is now the first search that lands after the first definition read of the seeded flag. A run that never reads the definition before editing gets its own branch and a reason that says so, rather than one blaming the search. `DEFINITION_READ_TOOLS` moves above `FLAG_LOOKUP_TOOLS`, which is now derived from it, so the two definition lookups are named once. Three cases added, none of which the old scorer failed: - a search before the assessment reads, which is the hole above - a fresh read of a different flag, which pins the seeded-flag match and the `flag_id`/`flag_key` fallback - two edits with the fresh read between them, which pins the first-edit anchor `_fresh_read_score` now passes the shape the cleanup seeders return. The other eight cases keep their verdicts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e suite's claims No case here can seed call sites, so `FreshDefinitionReadBeforeEdit` added a `None` row to every case on every run and its divider was never measured against a real agent trace. The pull request that adds the first fixture case registers it, where the divider can be checked for real. The scorer and its unit tests stay. The module docstring claimed deterministic unit tests cover the failed check refusal wording. Nothing grades that wording, so the docstring now says so. `recent_update_excluded_without_override` no longer spreads `_NO_CALL_SITES`. The by-key lookup returns `updated_at`, and the skill drops a candidate with a recent update before assessing it, so a run that refuses after one read follows the skill. Requiring the dependents and schedule reads failed that run. The other five cases still require both. Also: `NO_OVERRIDE_OFFERED_QUESTION` becomes `RECENCY_REFUSAL_WITHOUT_OVERRIDE_QUESTION`, to match the scorer it feeds, and the assess-only judge no longer asks for a recommendation the tour rule tells the agent to withhold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…on alone The skill's "Apply the retained path" step repeats four reads before the first write: the definition, the status, the dependent flags, and the scheduled changes. The scorer checked one of them. A new schedule or a new dependent flag does not change the definition, so a run that re-read only the definition and then edited a flag that gained a schedule after assessment scored 1.0, which is the change the step exists to catch. Round 1 moved the divider to the first search after the first definition read, which narrowed the hole rather than closing it: the existing-work step searches before the retained path, so a search there moves the divider earlier and an opportunistic second definition read after it counted as the repeat. Requiring all four groups is what makes the divider safe, because no step but the retained path repeats all four. Renamed to `FreshReadsBeforeEdit`, key `fresh_reads_before_edit`, since it no longer grades the definition read alone. The metadata now names which groups were repeated and which are missing, so a failing run says what it skipped. New cases: a run that repeats only the definition, and a search between assessment and the repeat that would open the window early. The 11 existing cases keep their verdicts, with the passing one now repeating all four reads. Also, CodeRabbit on the recency judge: the question accepted "the flag's age" as a refusal reason. The fixture is 90 days old and updated two days ago, so a message citing creation age passed without ever naming the recent update, which is the only blocker. The judge now requires the update or the time since it, and rejects creation age by name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t catch haacked found both while reviewing the skill layer. Neither has a scorer, and the module docstring listed only the skipped pre-edit read, so a reader would take the coverage as wider than it is. No registered scorer reads a Bash command, so nothing requires the existing-work search. A run that skips it and goes straight to the call-site search passes every scorer here. The cases cannot require it yet either, because the suite accepts a run that stops to ask about product tours before reaching the step. The pre-publish read has the same gap. `FreshReadsBeforeEdit` grades the pre-edit reads, and no case in this suite reaches a publish. Both stay ungraded here. Grading the first needs a case whose prompt already answers the tour question, which the fixture pull request is the place for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
57f1cd3 to
e6cf247
Compare
This branch has not been deployed
2 participants
Problem
The stale-flag cleanup skill can lose a safety guard and no test fails. A case passes as long as the agent picks the right file tools and never writes to the flag, so a run that edits code against a rollout read minutes earlier scores green.
This is the base of a two-layer stack. #107780 adds the skill text these scorers grade.
Changes
FreshReadsBeforeEditfails a run that edited against the rollout it read while assessing. It requires all four of the skill's pre-edit reads, the definition, the status, the dependent flags and the scheduled changes, to land between the repository search and the first edit.GreporGlobafter the first definition read, not the first one in the run, because the skill searches for the key before any definition is read. Requiring all four groups is what makes that divider safe.Nonerow to every case on every run. The pull request that adds the first fixture case registers it.recency_refusal_without_override,tour_unknown_waits,assessment_only_no_edit_claim.recency_refusal_without_overriderequires the message to name the recent update, or how long ago the flag changed. Creation age does not count, because the fixture is 90 days old and that age is why it was a candidate rather than why it is blocked.tour_unknown_waitsgrades the claim rather than the question. A report that stops at "nothing to change" passes. Calling the flag safe to remove without asking about tour usage fails. No case here can reach a removal recommendation, so demanding the question would fail a compliant no-op.seed_recently_updated_flagbuilds a flag stale on every signal exceptupdated_at, so the recency rule is the only thing that can block it. It creates the flag through the shared helper and sets both dates in oneupdate(), which the gated-write scanner ignores, so the baseline loses a line rather than gaining one.no_references_is_noopfixture instead of a second live agent run.Nothing a user sees changes. These files run under
hogli evalsand the eval-harness test suite only.How did you test this code?
hogli testover both harness test files andposthog/test/repo_invariants/test_feature_flag_gated_writes.py, 88 passing locally.The sandboxed eval suite did not run. It needs a Claude runtime, a sandbox and a seeded project, and each judged case costs a model call. The three judged scorers are therefore unexercised: their question text is reviewed, not tested.
New coverage, and the regression each group catches:
TestFreshReadsBeforeEdit, 14 cases. Catches the scorer accepting reads taken while assessing, accepting reads taken after the edit, accepting a read of a different flag, accepting a repeat of the definition alone, counting a failed call as a read, measuring against the last edit rather than the first, or scoring zero on a run that correctly refused to edit.CLEANUP_SEEDERSruns the stale, state-snapshot and codex checks againstseed_recently_updated_flag.FlagStateUnchangedscoresNonewhen a seed carries nostate, so a break there would switch the mutation check off forrecent_update_excluded_without_overridewith no test failing.TestSeedRecentlyUpdatedFlag, one case, theupdated_atwindow. Catches the eval case going vacuous:filter_stale_flagsclassifies oncreated_atandlast_called_at, and if it ever learns to readupdated_atthe seeded flag stops reaching the agent as a stale candidate.test_read_tool_sets_name_enabled_read_only_tools, extended withDEFINITION_READ_TOOLS. Catches a tool name in a scorer set that no longer exists intools.yaml, which would make the scorer match nothing and pass in silence.Automatic notifications
Docs update
None. Eval code only.
🤖 Agent context
Written by Claude Opus 5 (1M context) in Claude Code.
This re-cuts the first half of #104315, which carried the same work plus the skill text. That branch sat 412 commits behind master and its CI failed on staleness rather than on any test. Both layers here are rebuilt from current master.
gh pr list --state open --search "cleanup stale feature flags eval"found only chore(feature-flags): require a fresh read before every write #104315, which this stack supersedes./stacking-prs,/writing-tests,/writing-code-comments,/writing-pr-descriptions,/address-pr-reviews,/ste-writing.FreshReadsBeforeEditrequiring all four reads, which came from his own comment on the divider.🤖 Generated with Claude Code