chore(engineering-analytics): add rafapherding skill for driving PRs to green

products/engineering_analytics/skills/rafapherding/SKILL.md

@@ -0,0 +1,214 @@
+---
+name: rafapherding
+description: >
+  Shepherds a list of open PRs to green, unattended: reads review-bot comments, judges each one
+  accurate or not, replies to every one, fixes what is real, and drives CI to passing. Use for
+  "get these PRs green", "babysit my PRs", "handle the review bot comments", "watch CI on these
+  PRs and fix what breaks", "shepherd this stack". Understands Graphite stacks and always works a
+  stack bottom-up (fix base, gt restack, gt up, repeat, then gt ss once). Covers separating real
+  failures from infra flakes, the migration-number conflict every restack hits, and how to reply
+  to a bot you disagree with. Not for authoring a single PR from scratch, and not for aggregate CI
+  health (use diagnosing-ci-and-merge-bottlenecks).
+---
+
+# Rafapherding: driving PRs to green
+
+The job is custody, not authorship. Someone hands you a list of open PRs and goes away. You bring
+each one to a state where CI is green, every review-bot comment has a reasoned reply, and every
+accurate finding is fixed with a test that would have caught it. You are the last reviewer awake.
+
+Two rules shape everything else:
+
+1. **A bot comment is a claim, not an instruction.** Verify it against the code before you touch
+   anything. Bots produce real findings, findings that are true but out of scope, and findings that
+   are simply wrong. All three deserve a reply; only the first deserves a commit.
+2. **Stacks are ordered.** Never fix a stacked PR in the middle. Go to the bottom, fix, restack,
+   move up. A fix landed out of order gets clobbered by the next restack.
+
+## Order of work
+
+Group the PR list into stacks first (`gt log short`, or `gh pr view <n> --json baseRefName` — a PR
+whose base is another PR's head is stacked on it). Then, per stack, bottom to top:
+
+```sh
+gt log short                    # see the whole forest and which branches need restacking
+git checkout <bottom-branch>
+# ... judge comments, fix, test, commit ...
+gt restack                      # rebases this branch and everything above it
+gt up                           # move to the next PR in the stack, repeat
+gt ss                           # submit the whole stack, once, at the end
+```
+
+Push once per stack with `gt ss`, not once per branch. Every push fans out a full CI run, and
+force-pushing a deep stack at once can exceed GitHub's dispatch cap and take unrelated runs down
+with it. Do the whole stack's work, then submit.
+
+Work stacks sequentially when you share one working tree. Two stacks in flight in one checkout is
+how you end up committing a half-finished fix to the wrong branch.
+
+## Triage CI before you read a single comment
+
+Most of what looks like a broken PR is not. Classify every red check before fixing anything:
+
+```sh
+gh pr checks <n> | grep -E "\bfail\b"
+```
+
+Then, for each failing job, ask whether it actually ran:
+
+```sh
+gh api repos/PostHog/posthog/actions/jobs/<job-id> --jq '{name, conclusion, steps: (.steps|length)}'
+```
+
+- **`steps: 0` and a duration of exactly the job timeout** (10m0s, 15m0s) means the job never
+  acquired a runner. That is infra, not you. Several jobs sharing the same zero-step timeout in one
+  run is the signature of a bad window. Do not chase it; it clears on the next push.
+- **`Something Tests Pass` / `Checks Pass` failing in seconds** is an aggregation gate. It is
+  reporting a dependency's failure, not its own. Find the real job.
+- **A job that ran and failed** is the only kind worth reading logs for:
+  `gh run view --job <id> --log-failed`.
+
+Migration conflicts and lint failures are always yours. Flaky Playwright specs called out in the CI
+report comment usually are not.
+
+See [references/reading-ci.md](references/reading-ci.md) for the failure taxonomy in detail.
+
+## Judging bot comments
+
+Fetch both kinds — inline review comments and top-level ones:
+
+```sh
+gh api repos/PostHog/posthog/pulls/<n>/comments  --jq '.[] | "\(.user.login) | \(.path):\(.line) | id=\(.id)\n\(.body)\n"'
+gh api repos/PostHog/posthog/issues/<n>/comments --jq '.[] | "\(.user.login)\n\(.body)\n"'
+```
+
+For each finding, answer three questions in order, and stop at the first "no":
+
+1. **Is the mechanism real?** Trace it in the code. Do not accept the bot's description of what a
+   function does — open the function. A high-confidence score is not evidence.
+2. **Is it reachable?** A real mechanism behind an unreachable path is a note, not a bug. Say so.
+3. **Is it this PR's?** Check `git show master:<path>` — if the code is new in this PR, it is in
+   scope even when the pattern predates it. If the PR merely moved existing code, the finding may
+   be true and still belong in a follow-up.
+
+Then act:
+
+- **Accurate and in scope** → fix it, add a regression test, reply naming the commit.
+- **Accurate but out of scope** → reply agreeing, say why it is not this PR, and be specific about
+  where it should go.
+- **Wrong** → reply with the evidence that makes it wrong, not just "false positive". If it is a
+  linter false positive, suppress it at the line with a justified reason rather than silencing the
+  rule.
+
+Reply to the finding, in its thread, so it resolves where the reviewer left it:
+
+```sh
+gh api repos/PostHog/posthog/pulls/<n>/comments/<comment-id>/replies -f body='...'
+```
+
+A reply is worth writing even when you agree completely. The next human to open the PR needs to
+know the finding was read and what happened to it. See
+[references/replying-to-bots.md](references/replying-to-bots.md) for what a good reply contains and
+worked examples of all three verdicts.
+
+## Prove the fix
+
+Every fix you land for a bot finding needs a test that fails without it. Write the test, then
+verify it actually catches the bug by reverting the fix and watching it fail:
+
+```sh
+cp <file> /tmp/fix.bak
+# revert the fix by hand
+hogli test <test-file>::<TestClass>::<test_name>   # must FAIL
+cp /tmp/fix.bak <file>
+hogli test <test-file>                             # must PASS
+```
+
+A test you never saw fail is a test you do not know works. This step catches the vacuous
+assertion — the one that passes because the request 401s before it reaches the code under test —
+which is the single most common way a security regression test turns out to be worthless.
+
+## Restacking
+
+`gt restack` walks the whole stack upward and will stop on the first conflict. The one you will hit
+constantly in this repo is `posthog/migrations/max_migration.txt`, because master keeps taking the
+migration number your branch claimed. Do not hand-edit it:
+
+```sh
+python manage.py rebase_migration posthog   # or `ee` — renumbers and repoints dependencies
+gt add posthog/migrations
+gt continue
+```
+
+Then confirm no state drift before moving on: `DEBUG=1 python manage.py makemigrations --dry-run
+--check` must say "No changes detected".
+
+Renumbering a migration invalidates your local test database, which will then fail with
+`DuplicateColumn: column ... already exists`. That is local-only; rebuild with
+`hogli test <path> -- --create-db`. It is not a CI failure and not something to "fix" in the code.
+
+### A blocked migration has to be split
+
+`Migration risk` failing with `❌ BLOCKED` is not acknowledgeable the way the hot-table and
+`atomic = False` policies are — there is no allowlist file for it. The two rules that catch people:
+a `RunPython` may not share a file with schema changes, and a `RunSQL` may not share a file with
+anything. Both hold their locks for the whole file, so the fix is one operation shape per migration:
+
+```text
+NNNN    AddField / AddField          schema only
+NNNN+1  RunPython                    data only
+NNNN+2  RunSQL                       alone
+NNNN+3  SeparateDatabaseAndState     state only
+```
+
+Splitting has to preserve whatever ordering the original relied on — a backfill reading a column
+must still run before the migration that drops it from the model state, and Postgres needs a default
+before the model stops listing a `NOT NULL` column. Read the original's docstring for the constraints
+before resequencing, then verify:
+
+```sh
+DEBUG=1 python manage.py analyze_migration_risk | grep -E "Summary:|BLOCKED"   # must be 0 Blocked
+DEBUG=1 python manage.py makemigrations --dry-run --check                     # "No changes detected"
+DEBUG=1 python manage.py sqlmigrate posthog <each_new_migration>              # SQL is what you meant
+```
+
+Expect this to surface late: the job that reports it is one of the ones most often lost to runner
+starvation, so a migration can sit blocked for several pushes without anyone seeing it.
+
+## Changing production code breaks test seams
+
+When you fix a finding by changing how code reaches the network — swapping `requests.get` for a
+pinned session, moving a validation call — every test that patched the old seam silently stops
+intercepting and starts making real calls. The symptom is a wall of failures with a message about
+DNS or connection refused for a hostname like `partner.example.com`.
+
+Retarget the patch to the new seam rather than reverting the fix. Prefer patching the narrowest
+thing that still covers the path; when you must patch something process-wide (`requests.Session.get`),
+expect unrelated traffic to land on the mock, and tighten raw `assert_called_once` /
+`assert_not_called` assertions to filter for the call you actually mean.
+
+**Grep the whole repo for the old seam, not just the files you already know about.** A stale patch
+does not always fail. If the URL under test resolves for real, the test quietly makes a live request
+and passes on whatever production returns — green locally, and failing in CI for some unrelated
+reason like an IPv4-only runner. That is worse than a red test, because it looks finished.
+
+```sh
+rg -n "module\.requests\.get|module\.old_helper_name" --type py
+```
+
+Every hit is either a seam to retarget or a deliberate exception you can name. Do this immediately
+after moving a seam, before running anything: the local suite going green is not evidence that you
+found them all.
+
+## Before you call a PR done
+
+- Every failing check is either green or classified as infra with the evidence for that.
+- Every bot finding has a reply.
+- Every fix has a test you watched fail.
+- `ruff check --fix` / `ruff format` clean, and the pre-push `hogli ci:preflight` passes without
+  `--no-verify`.
+- The stack is restacked and submitted once with `gt ss`.
+
+When something needs a decision only the author can make — whether a production partner actually
+holds a credential, whether a behavior change is acceptable — do not guess. Land the code-side
+safety net, say plainly in the PR reply what you could not verify, and leave it flagged.

products/engineering_analytics/skills/rafapherding/references/reading-ci.md

@@ -0,0 +1,117 @@
+# Reading a red PR
+
+The goal of this pass is a short list: which failures are yours. Everything else you note and move
+past. Getting this wrong in either direction is expensive — chasing an infra flake wastes a night,
+and dismissing a real failure as flaky ships a bug.
+
+## Get the failures
+
+`gh pr checks <n>` prints every check including hundreds of skipped ones. Filter:
+
+```sh
+gh pr checks <n> | grep -E "\bfail\b"
+gh pr checks <n> | grep -E "\bpending\b"    # still running, revisit later
+```
+
+A PR with no `fail` and no `pending` lines is green regardless of what the Graphite UI shows.
+
+## The taxonomy
+
+### Runner starvation (not yours)
+
+```sh
+gh api repos/PostHog/posthog/actions/jobs/<job-id> --jq '{name, conclusion, steps: (.steps|length), started_at, completed_at}'
+```
+
+`steps: 0`, and `completed_at - started_at` exactly equal to the job's `timeout-minutes`. The job
+was queued, never got a runner, and was killed by its own timeout. `gh run view --job <id>
+--log-failed` returns `log not found`, which is itself a tell — there are no logs because nothing
+ran.
+
+Several unrelated jobs in one workflow run sharing the same zero-step timeout means the whole run
+hit a bad capacity window. Push and they will schedule normally.
+
+### Aggregation gates (not the real failure)
+
+Jobs named `<Something> Tests Pass`, `<Something> Checks Pass`, `Check matrix outcome`, `Check
+dependency results`. They fail in seconds with a step that just reads its dependencies' results.
+They tell you a matrix leg failed; they never tell you which or why. Find the matrix leg.
+
+### Superseded runs (not yours either)
+
+A push cancels the run already in flight for that branch, and every aggregation gate then reports its
+CANCELLED dependencies as FAILURE. So a wall of `Tests Pass` failures appearing right after you push
+means _superseded_, not broken. Check the state breakdown before reading anything:
+
+```sh
+gh pr checks <n> --json name,state --jq 'group_by(.state)[] | "\(.[0].state): \(length)"'
+```
+
+A large `CANCELLED` count next to `IN_PROGRESS` is the signature. The gates will be replaced as the
+new run's legs finish. This is worth filtering out of any CI monitor you set up, or it will wake you
+for every push you make:
+
+```sh
+# Real failures only: drop the gates, which carry nothing the underlying job doesn't
+gh pr checks <n> --json name,state --jq '.[] | select(.state=="FAILURE")
+  | select(.name | test("Tests Pass|tests pass|CI Pass|Checks Pass|does not block merge") | not) | .name'
+```
+
+### Real failures
+
+The job has steps, ran for a plausible duration, and `--log-failed` returns content.
+
+```sh
+gh run view --job <id> --log-failed | grep -E "FAILED|^E |AssertionError|Error:" | head -30
+```
+
+The GitHub Actions log format prefixes every line with the job name and a timestamp, so grep for
+the assertion rather than reading linearly. For pytest, `FAILED <nodeid>` lines at the end are the
+summary; the `E` lines above them are the actual assertion.
+
+### Semgrep
+
+`semgrep-python` failing with `Ran N rules on M files: 1 finding` is a real finding. Extract it:
+
+```sh
+gh run view --job <id> --log | grep -A 4 "Code Finding"
+```
+
+Judge it like any other bot finding. A security rule firing on a **test** that deliberately
+constructs the unsafe thing in order to assert it is rejected is a false positive — suppress it at
+the line with the rule id and a reason, per the repo's nosemgrep convention:
+
+```python
+# nosemgrep: python.jwt.security.jwt-none-alg.jwt-python-none-alg (forging an alg=none token is the point: this asserts the verifier refuses it)
+```
+
+Never suppress by disabling the rule globally, and never suppress a finding in production code you
+have not traced.
+
+### Migration validation
+
+`Validate migrations` failing after your branch has sat for a day is almost always the number
+collision: master took the migration number you claimed. This surfaces as a `max_migration.txt`
+conflict during restack, not usually as a standalone CI failure — if CI reports it directly, restack
+first and it resolves.
+
+### The CI report comment
+
+The bot posts a `## 🤖 CI report` comment with sections for Playwright flakes and backend patch
+coverage. Flaky specs listed there are explicitly annotated "not necessarily caused by your
+changes" — treat them as informational unless your diff touches that area. Patch coverage is
+advisory: it names uncovered changed lines. Worth a look when you are already adding a test, not
+worth chasing to a number.
+
+### stamphog REFUSED
+
+Not a CI failure. It means the automated reviewer declined to review — usually because the diff is
+too large for its ceiling or matches a deny-list (auth-sensitive code). It also surfaces unresolved
+findings from other bots, which is useful as a checklist. A REFUSED verdict does not block merge and
+is not something to "fix"; resolve the findings it cites.
+
+## What to re-run vs. what to fix
+
+You do not need `gh run rerun`. Pushing the stack re-triggers everything, and you will be pushing
+anyway. Only re-run explicitly when a PR is otherwise finished and its sole red check is a confirmed
+infra timeout.