Repository navigation
chore(signals): sharpen feature flag scout handoff - #96068
Merged
Merged
Conversation
The `stale_feature_flags` health check classifies staleness deterministically and emits no generic Signals report. The feature-flags scout must therefore be the only author of stale reports. Teach it that handoff. The scout no longer runs its own stale scan. It reads active, undismissed `stale_feature_flags` issues, re-verifies each one against the live flag definition, ranks the survivors, and authors one cleanup report per flag. It gates immediate actionability to a deterministic `fully_rolled_out` or `not_rolled_out` direction with no blockers. Everything else asks for human input or stays in memory. A guarded fallback scan stays for projects the check does not reach yet. A fallback finding can never be immediately actionable. One flag gets one report, which reverses the bundling doctrine the rest of the fleet follows. Each stale flag is a separate code removal with its own owner, diff, and PR, so a debt count gives nobody a decision to act on. Per-flag reports also meet the team's daily report cap. The scout therefore ranks candidates by evidence strength and queues the rest in memory. `signals-scout-health-checks` cedes the kind. Without that, both scouts see the same issues once they persist, and every flag gets two reports. Memory identity: stale pointers use `report:feature-flags:stale:<key>`, not the plan's `report:feature-flags:<flag-id>:cleanup`. The file already keys pointers by flag key, and the key is what the inbox and scratchpad searches match. The `stale:` segment separates the cleanup lane from the anomaly lane, because one flag can carry a live report in both. The trusted `flag_id` goes in the pointer value. The payload contract comes from PR #95189, branch `feat/flags-stale-health-check` at `6842c7e`. Nobody merged that branch yet, so re-verify the field names at merge. The harness stamps `scout-tags: [feature-flags]` only when it creates a config, so existing configs stay untagged. This change includes no backfill. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the previous commit found two blocking defects in the instruction sequence and several claims that do not match the code. Rank before you verify. The scout re-verified every candidate before ranking any of them, at several tool calls each. A roster of forty stale flags spends the whole run on flags it then discards. A scout run has a hard 15-minute wall (`scout_harness/limits.py:10`), and overrunning it kills the activity, marks the run failed, and loses the anomaly findings from the same pass. Every ranking input arrives in the list response, so ranking first costs nothing. The shortlist is now capped at three, and the close-out reports how many were deferred. Read the payload from the list. `HealthIssueSerializer` already carries `payload` and `snoozed_until`, and `remediation` is static per kind. The per-candidate `health-issues-get` fetched data the scout already held. It now runs only on the shortlist, for the `link`. The fallback gate let a dismissal come back. A project where a human dismissed every stale issue returns the same empty filtered list as a project the check never reached. The scout read that as "check absent", ran the `STALE` scan, and recovered the flags the human waved off. An unfiltered probe now separates the two cases. The fallback also states that it bundles, because the one-flag-one-report rule is earned by a re-verified health issue. The zero-call branch reached the health issues but not the fallback, so projects that evaluate flags server-side got nothing from a run. It now points at the whole section. A completed-migration cliff routed to the dead-check bundle, which excludes it: the bundle needs sustained call volume and a cliffed flag has none. It routes to the stale lane now. Renaming `addressed:feature-flags:debt-bundle` stranded a durable row. Scratchpad entries with no `expires_at` are never swept (`models.py:2098-2107`), the orientation search matches content, and the old text reads as "already filed". It would have suppressed the reports this change exists to produce. The skill now retires the legacy key on sight. The trust grant said "the MCP tool descriptions", which covers every connected external server, not the health-issues tools it meant. Scoped it. `signals-scout-health-checks` cedes the kind but still counts it in the `health-issues-summary` total behind its zero-issue exit, so a project whose only issues are stale flags lost its cheap empty run. It subtracts the kind now. Corrected two claims: scratchpad entries do not expire by default, and `health-issues-summary` cannot settle whether the check runs for a project. `AGENTS.md` records that the payload table is coupled to `StaleFeatureFlagsCheck`. No test reads a scout's prose, so a rename there drifts silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
😎 Merged successfully - details. |
Contributor
🤖 CI report
|
JamesPatrickGill
marked this pull request as ready for review
September 8, 2026 07:09
Contributor
🦔 PostHog Review reviewed this pull requestFound 2 must fix, 4 should fix, 2 consider. Published 8 findings (view the review). Resolved comments: 9 fixed, 1 left for you |
Contributor
Prompt To Fix All With AI### Issue 1
products/signals/skills/signals-scout-feature-flags/SKILL.md:194
**Staleness can become outdated**
If a flag is called again after detection, its `last_called_at` changes without changing the definition version, while the health issue remains active until the weekly check runs again. This re-verification checks only the definition, filters, and version, so the scout can turn outdated `not_called_recently` evidence into a cleanup report or immediately actionable task for a flag that is no longer stale. Re-check the live usage-based status before authoring the report.
### Issue 2
products/signals/skills/signals-scout-health-checks/SKILL.md:145
**Summary fields are misread**
`health-issues-summary` has no top-level `total` or `by_kind`; those fields exist separately under `unsnoozed` and `snoozed`. The subtraction therefore cannot be performed as written and may cause the scout to apply the wrong close-out condition and store a misleading clean-team marker. Specify the nested fields to use, such as `unsnoozed.total - unsnoozed.by_kind.stale_feature_flags`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore(signals): fix stale-flag handoff o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
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}
The re-verification step showed `feature-flag-get-definition {flag_id}`, but
the tool's only required input is `id` (`products/feature_flags/mcp/tools.yaml`
declares `param_overrides: id`, and the generated schema extends
`FeatureFlagsRetrieveParams` with `id`). A literal call keyed `flag_id` fails
validation before the candidate is re-verified, burning run budget against the
15-minute wall the same section caps re-verification for.
Show the call as `{"id": <flag_id>}`, which also matches the three sibling
shorthands in this body that already name the request field `id`.
Generated-By: PostHog Desktop
Task-Id: 111bf653-e77f-4376-b3ae-134d410dc5ba
…t path A `fully_rolled_out` verdict on a multivariate flag does not mean one variant survives. `is_multivariate_flag_fully_rolled_out` counts a release condition only when it has no property filters (`is_group_fully_rolled_out` in `products/feature_flags/backend/flag_status.py`), and it reads variant overrides from those untargeted groups alone. A targeted group is never inspected. The runtime disagrees: the matcher walks conditions in declaration order and returns the first match with that condition's variant override (`rust/feature-flags/src/flags/flag_matching.rs`), so a targeted group listed before the blanket group keeps serving its own variant to its segment. Pinning one segment to control while everyone else gets the winner is an ordinary pattern, and such a flag can go 30 days uncalled and enter the stale set. The gate then passed it as immediately actionable, which routes to a draft PR that deletes a variant path still in use. The gate now requires an untargeted multivariate definition, the requires-human-input list names the targeted case, and the retained-behavior paragraph says to hand over the segment and its variant instead of naming one path to remove. Boolean flags keep the fast path: a blanket 100% condition matches everyone whatever earlier targeted conditions exist, so the direction stays deterministic there. Generated-By: PostHog Desktop Task-Id: 111bf653-e77f-4376-b3ae-134d410dc5ba
The shortlist was ranked off the whole active issue set, and the dedupe check sat at step 3 of re-verification, inside the three-flag cap. Nothing clears a `stale_feature_flags` issue when the scout reports it: the only resolution path is the detector batch (`resolve_stale_issues_with_deltas` -> `HealthIssue.bulk_resolve`), which resolves an issue when the check stops emitting it, and the Signals backend touches `HealthIssue` nowhere. Ranking leans on `days_since_evidence`, which only grows for an uncalled flag, and the cleanup the scout recommends removes the code checks, so the flag goes colder. The same strongest flags therefore reclaimed every shortlist, and the deferred queue grew without ever producing a report. Rank now excludes the flags a live cleanup report covers before the cap applies, using the `report:feature-flags:stale:<key>` pointers the orientation scratchpad search already returns, and the queue is rewritten without what the run reported or dropped. Each run then shortlists the strongest uncovered candidates, so the backlog drains one batch at a time. Generated-By: PostHog Desktop Task-Id: 111bf653-e77f-4376-b3ae-134d410dc5ba
The migration retired the `addressed:feature-flags:debt-bundle` scratchpad key because a durable row reading "already filed" would suppress the per-flag stale reports. The report that key pointed at was left unaddressed. Master's scout already authors reports and bundles stale flags into one P3 finding that names its candidates, and `inbox-reports-list` searches titles and summaries, so a flag named in a live bundle satisfies the search that the dedupe rails turn into a skip. The strongest candidates are the ones a bundle is most likely to name, and they are the ones this lane shortlists first. The inbox-search rail now says a bundle is not coverage, and directs the scout to read a match in full, author the single-flag report anyway, and note on the bundle which flag moved to its own report. Generated-By: PostHog Desktop Task-Id: 111bf653-e77f-4376-b3ae-134d410dc5ba
The new disqualifier routes a project whose only active issues are stale flags
to the `pattern:health:clean-team{team_id}` marker, whose content template
reads "0 active health issues at {timestamp}". That claim is false there. The
branch is deterministic, and re-running rewrites the entry in place, so the row
never ages out.
The decision the marker gates is right — a stale-only project really is an
empty run for this scout, and the scout re-reads `health-issues-summary` every
run rather than trusting the marker — so keep the key and correct the wording.
The content now says the count is in-scope only and names the excluded kind, so
`scout-scratchpad-search` no longer hands a person an all-clear on a project
that has issues.
Generated-By: PostHog Desktop
Task-Id: 111bf653-e77f-4376-b3ae-134d410dc5ba
The stale read passed no `limit` or `offset`, so it saw one page of an arbitrarily large set. `HealthIssuePagination` serves 50 rows by default and 250 at most, and `safely_get_queryset` orders by severity then `-created_at`. The detector files every stale issue at `INFO`, so the severity term is constant and the effective order is newest row first: a flag that just crossed the 30-day line ranks above one cold for 210 days, which inverts the evidence strength the scout ranks on. The coldest candidates sank to later pages and were never reported, and the deferred number in the close-out counted one page instead of the set. The read now pages with an explicit `limit`, follows `offset` until `count` is covered while carrying a running top ten, and takes the close-out number from `count`. The ordering claim in Rank is corrected to say the paged set, and the tool bullet documents the limits the way the flag roster bullet already does. Generated-By: PostHog Desktop Task-Id: 111bf653-e77f-4376-b3ae-134d410dc5ba
`rollout_state` describes the value a flag returns, not the payload attached to
that value. `filters.payloads` is validated with no flag-type condition
(`products/feature_flags/backend/api/feature_flag.py`), and only *encrypted*
payloads require a remote-config flag, so a plain boolean or multivariate flag
can serve one. The matcher resolves it separately from the match:
`get_matching_payload` reads `match_variant.unwrap_or("true")` and returns
`get_payload(variant)` (`rust/feature-flags/src/flags/flag_matching.rs`),
mirrored by the Django model.
The check excludes the remote-config type and nothing else about payloads, and
a locally-evaluated flag serving a live payload still reaches 30 days without a
call event. Every gate bullet passed on such a flag, so the report earned
immediate actionability and a PR agent removing "the flag checks" would drop
the payload retrieval with them.
The gate now requires an empty `filters.payloads` in the definition read it
already performs, the requires-human-input list names the payload case, and the
retained-behavior paragraph says to hand over the payload and its readers.
Generated-By: PostHog Desktop
Task-Id: 111bf653-e77f-4376-b3ae-134d410dc5ba
Re-verification read the definition, the version, the blockers, and work in flight, but never the call stream. The check runs weekly (`schedule = "0 6 * * 1"`), and a flag that starts being called again moves `last_called_at` without touching the definition, so the issue stays active and the version check passes while the evidence is up to a week out of date. A resumed flag could therefore earn a cleanup report, or a PR that removes code now back in use. Re-verification now confirms the flag is still cold before it earns a report, reading `calls_14d` from the orientation query the run already makes and falling back to one scoped count for a key sitting in that query's tail. Calls since `evidence_date` drop the candidate and leave the issue to the next weekly pass. The step also states the asymmetry: a zero count disqualifies nothing, because a locally evaluated flag sends no call events. Generated-By: PostHog Desktop Task-Id: 111bf653-e77f-4376-b3ae-134d410dc5ba
The disqualifier asked for `by_kind` to be subtracted from `total`, but the
summary endpoint returns neither at the top level: it responds with
`{"unsnoozed": ..., "snoozed": ...}`, and each group carries its own `total`,
`by_severity`, and `by_kind` (`posthog/api/health_issue.py`, and
`HealthIssueSummary` / `HealthIssueCounts` in the generated schema). The
arithmetic is now named on the unsnoozed group, and the line records where the
counts live so the same misread does not reappear.
Generated-By: PostHog Desktop
Task-Id: 111bf653-e77f-4376-b3ae-134d410dc5ba
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
Contributor
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 |
…task Addresses two review findings on #96068. ReviewHog: the actionability gate never asked whether the flag key appears in the chosen repository. Two shapes reach it with nothing to remove. `fully_rolled_out_without_usage_data` means no call was ever recorded, which is also a flag someone created in the UI and never wired. A completed migration arrives the same way, because its checks were removed when the migration finished. Autostart then opens a draft PR the implementation agent cannot fill. The gate now requires one confirmed call site. `gh` already carries a read-only token in the sandbox for the search. When the search finds nothing, the remaining work is archival, so the report asks a human to confirm and archive. veria-ai: the autonomous implementation task takes an immediately-actionable report's summary verbatim as its opening text, and that task holds repository write access (`products/signals/backend/auto_start.py:249`). The retained-behavior rule told the scout to name the winning variant, and a variant key is validated only as a string, so a project member chooses that text. The summary now carries structured identity alone. The implementation agent reads the variant from the flag definition. Declined the reviewer's own remedy of making every stale report `requires_human_input`. That removes the deterministic per-flag cleanup this work exists to produce, and the injection route closes without it. The untrusted-data section said the roster holds "your trusted identifiers". Trusted for identity, not as prose: a key, name, description, and variant key are strings a project member chose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
This is a documentation/prompt update to internal agent "skill" files, not application code — no schema, billing, auth, dependency, or CI changes; the underlying health check stays in dry-run and autostart remains disabled, so nothing changes in production behavior yet. The two substantive Greptile concerns (stale-evidence re-verification, health-issues-summary field nesting) are both visibly addressed in the current diff text, and a separate bot security review found no open concerns.
- 👍 on the PR from greptile-apps[bot].
- PostHog bot review notes '1 left for you' from an older reviewed commit with no visibility into what remains — likely minor given 9 of 10 findings were resolved, but worth a final glance.
- Author is not on the owning team (@PostHog/team-self-driving) and no familiarity band was reported, though this is a routing signal only given the low-risk nature of the change.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 0L, 0F substantive, 112L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T0 auto-approve: T0-deterministic (112L, 3F, single-area, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 33142c6 · reviewed head 33142c6 |
andrewm4894
approved these changes
Sep 10, 2026
Member
Author
|
/trunk merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Nobody gets a stale-flag cleanup decision in their Signals inbox today, and the detector that would produce one cannot leave dry-run until this lands.
stale_feature_flagshealth check (feat(flags): detect stale flag health issues #95189) persists one issue per stale flag. It deliberately emits no generic Signals report, so nothing turns those issues into inbox decisions.signals-scout-health-checksalso sees them. That scout lists issues without a kind filter, so every stale flag would get two reports.Note
Merge order. Nothing blocks this PR, and it can land before or after #95189. Both are safe in either order, because #95189 deploys with
dry_run = Trueand writes no issues.The ordering that does matter is the
dry_runflip, which is an operational step after both merge. Flip it while this PR is unmerged andsignals-scout-health-checkspicks the issues up first, bundling them into the debt-count report this PR exists to replace. Land this one first and that window never opens.Changes
signals-scout-health-checksgets one disqualifier ceding thestale_feature_flagskind. That is a deliberate crossing into a sibling scout's skill file.products/signals/skills/AGENTS.md, and the frontmatter tagscout-tags: [feature-flags].The autostart gate
Four conditions block a flag from an automatic draft PR, each one a case where the cleanup would be wrong rather than merely unhelpful.
filters.payloadsThe report summary is a prompt
_build_autostart_task_descriptionplaces the report summary verbatim at the top of an autonomous implementation task with repository write access. A variant key is validated only as a string, so a project member chooses that text.The summary therefore carries structured identity only: flag id, flag key, rollout direction as its enum value, health issue id. Where the retained behavior depends on the winning variant, the implementation agent reads it from the flag definition rather than from the report.
Reviewers proposed making every stale report
requires_human_inputinstead. This PR declines that. It removes the deterministic per-flag cleanup the change exists to produce, and hardening the summary closes the same route.Why one report per flag
This reverses the doctrine the rest of the fleet follows.
signals-scout-health-checksstates it twice: filing one report per issue is the noise that scout exists to avoid.Stale flags are the exception. Each one is a separate code removal with its own owner, diff, and PR. A debt count of nine flags gives nobody a decision to act on.
Billing changes hands
Reports from
signals-scout-health-checksare billing-exempt (products/signals/backend/billing.py:110); reports from the feature-flags scout are not. Moving authorship therefore bills the customer. The exemption covers PRs that fix PostHog's own systems, and a stale flag in the customer's own repo is not one.Tag behavior is forward-only
register_missing_configsstampsscout-tagsonly when it creates aSignalScoutConfig. Existing configs stay untagged. This change includes no backfill, and no surface consumes afeature-flagstag yet.How did you test this code?
hogli lint:skillsandhogli build:skillspass. The five advisory stale-reference warnings are pre-existing and unrelated.products/signals/backend/test/test_scout_harness_lazy_seed.pyalready covers the frontmatter tag syntax and hash stability. No test reads a scout's prose, so review is the only gate on this diff.Warning
The payload contract is written against #95189, which nobody merged yet. That branch moves: the field names were last checked against
3c37246, and its head has advanced since. Re-verify the kind string, payload field names, severity, and hash keys against the merge commit before flippingdry_run. No test reads this skill's prose, so a rename there drifts it silently on every agent-enabled team.Automatic notifications
Docs update
None. No user-facing surface or API changes.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code wrote this from a scoped brief, reading the unmerged detector branch through git only. Skills invoked:
/writing-skills,/ste-writing,/writing-pr-descriptions,/review-code --fix,/address-pr-reviews.Three rounds shaped the result. A local
/review-codepass before opening caught the ranking order and the dismissal loop. ReviewHog then published 8 findings and auto-fixed most of them as commits on this branch. The last round added the call-site gate and the summary hardening.Two decisions worth a reviewer's attention:
report:feature-flags:<flag-id>:cleanup. The shipped convention isreport:feature-flags:stale:<key>. The file already keys pointers by flag key, and the key is what the inbox and scratchpad searches match. Thestale:segment separates the cleanup lane from the anomaly lane, since one flag can carry a live report in both.Public artifact: nothing here draws on customer material. The scratchpad examples in the skill use invented flag keys and ids.