From 1ffb4be3bd2760e3c178a15fde44611e37546cbc Mon Sep 17 00:00:00 2001 From: Rafa Audibert Date: Tue, 28 Jul 2026 03:30:35 -0300 Subject: [PATCH 1/3] chore(engineering-analytics): add rafapherding skill for driving PRs to green Captures the unattended PR-custody workflow: read the review bots, judge each finding accurate or not, reply to every one, fix what is real with a test that fails without the fix, and drive CI to green. Graphite stacks are worked bottom-up throughout, with a single submit at the end. Lives in engineering_analytics alongside the other CI skills. Two references carry the detail that does not belong in the entry point: the failure taxonomy that separates a real red check from a runner-starvation timeout or an aggregation gate, and what a reply to a bot should contain, with worked examples for accurate, out-of-scope, and wrong. Co-Authored-By: Claude Opus 5 (1M context) --- .../skills/rafapherding/SKILL.md | 173 ++++++++++++++++++ .../rafapherding/references/reading-ci.md | 97 ++++++++++ .../references/replying-to-bots.md | 97 ++++++++++ 3 files changed, 367 insertions(+) create mode 100644 products/engineering_analytics/skills/rafapherding/SKILL.md create mode 100644 products/engineering_analytics/skills/rafapherding/references/reading-ci.md create mode 100644 products/engineering_analytics/skills/rafapherding/references/replying-to-bots.md diff --git a/products/engineering_analytics/skills/rafapherding/SKILL.md b/products/engineering_analytics/skills/rafapherding/SKILL.md new file mode 100644 index 000000000000..59871bc9e6d8 --- /dev/null +++ b/products/engineering_analytics/skills/rafapherding/SKILL.md @@ -0,0 +1,173 @@ +--- +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 --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 +# ... 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 | grep -E "\bfail\b" +``` + +Then, for each failing job, ask whether it actually ran: + +```sh +gh api repos/PostHog/posthog/actions/jobs/ --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 --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//comments --jq '.[] | "\(.user.login) | \(.path):\(.line) | id=\(.id)\n\(.body)\n"' +gh api repos/PostHog/posthog/issues//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:` — 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//comments//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 /tmp/fix.bak +# revert the fix by hand +hogli test :::: # must FAIL +cp /tmp/fix.bak +hogli test # 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 -- --create-db`. It is not a CI failure and not something to "fix" in the code. + +## 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. + +## 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. diff --git a/products/engineering_analytics/skills/rafapherding/references/reading-ci.md b/products/engineering_analytics/skills/rafapherding/references/reading-ci.md new file mode 100644 index 000000000000..fd650977b80b --- /dev/null +++ b/products/engineering_analytics/skills/rafapherding/references/reading-ci.md @@ -0,0 +1,97 @@ +# 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 ` prints every check including hundreds of skipped ones. Filter: + +```sh +gh pr checks | grep -E "\bfail\b" +gh pr checks | 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/ --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 +--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 ` Tests Pass`, ` 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. + +### Real failures + +The job has steps, ran for a plausible duration, and `--log-failed` returns content. + +```sh +gh run view --job --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 ` 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 --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. diff --git a/products/engineering_analytics/skills/rafapherding/references/replying-to-bots.md b/products/engineering_analytics/skills/rafapherding/references/replying-to-bots.md new file mode 100644 index 000000000000..ebdb4256fa56 --- /dev/null +++ b/products/engineering_analytics/skills/rafapherding/references/replying-to-bots.md @@ -0,0 +1,97 @@ +# Replying to review bots + +Every finding gets a reply. The audience is the human who opens the PR next and needs to know, per +thread, what was concluded and why — not whether a bot was satisfied. + +## What a reply contains + +1. **The verdict up front.** "Accurate, fixed in ``." / "Real, but out of scope here." + / "This does not hold, here is why." Do not bury it under analysis. +2. **The mechanism, in your own words.** Restating the mechanism proves you traced it rather than + pattern-matching the bot's summary. If the bot's description was subtly wrong about the + mechanism, this is where that surfaces. +3. **What you did, and why that approach.** When the bot offered options, say which you took and + what it bought. +4. **The test.** Name it, and say what it does on the unfixed code. +5. **Honest severity.** If the bot rated it high and you think it is low, say so with the reason. + If it rated it low and it is worse than that, say that too. + +Keep it plain. No preamble, no thanking the bot. + +## Posting + +Reply inside the thread so it resolves in place: + +```sh +gh api repos/PostHog/posthog/pulls//comments//replies -f body='...' +``` + +`` is the `id` of the original review comment. A new top-level comment is not a reply +and leaves the thread open. + +For bodies with backticks and quotes, prefer a heredoc into a file and `-F body=@file`, or use +single-quoted shell strings and escape internal single quotes. Getting shell-mangled markdown into +a public repo is worse than a terse reply. + +## Worked examples + +### Accurate and fixed + +> Accurate, and fixed in `d8fe3be`. +> +> You are right on the mechanics: DRF runs `check_throttles` in `initial()`, before the handler +> calls `load_grant()`, and the key was the grant id alone. So a partner that learned another +> partner's grant id could spend the owner's poll budget on it with requests that all end in 404. +> +> Went with your second suggestion (partner in the key) rather than moving the check after +> `load_grant()`, because keeping it a declarative throttle means it still gets the base view's +> `rate_limited` envelope and `Retry-After` for free. A grant belongs to exactly one partner, so +> scoping the key does not split any legitimate caller's quota. +> +> Added `test_poll_budget_is_not_shared_across_partners`, which fails with a 429 on the old key. +> +> Worth noting the practical severity matches your "low": `grant_id` is opaque and only ever +> returned to the partner that created it, so the attacker needs a secret they should not have. +> Fixed anyway since it was two lines. + +Note the last paragraph. Agreeing with a finding and disagreeing with its framing is normal. + +### Accurate but not this PR's to fix + +> Real, and it is not new here — `git show master:` has the same call. This PR moves the +> function into the DRF view without changing its behavior, so fixing it here would mix a security +> change into a refactor and make both harder to review or revert. +> +> Filed as a follow-up. Not blocking this one. + +Only say this when you have actually checked master. "Pre-existing" asserted without evidence is +how real bugs get waved through. + +### Wrong + +> This does not hold. The claim is that `` is user-controlled, but it comes from +> `.get_search_fields()`, which returns a fixed tuple defined on the viewset — there is no +> request data on that path. Traced from the caller at `:`. +> +> Suppressed at the line with the rule id and that reason rather than leaving it to fire on every +> future run. + +Evidence, not adjectives. "False positive" on its own tells the next reader nothing and they will +re-litigate it. + +## When you cannot decide + +Some findings turn on facts outside the repo: whether a production row has a usable credential, +whether a partner integration relies on the behavior you are about to tighten. Do not guess and do +not stall. + +Land the part you can — a log line that surfaces the affected rows at migration time, a check that +fails closed — then say exactly what you could not verify and who has to: + +> The mechanism is right and I have added a migration-time warning listing affected application +> ids, so this cannot fail silently on deploy. Whether any such row actually exists is an +> operational question I cannot answer from the repo — each one needs a secret set or a jwks_uri +> published before the old column is dropped. Flagging for you rather than guessing. + +That is a finished reply, not a punt. It moves the code to a safe state and puts a named decision +in front of the person who can make it. From 6b227e6ea726f6069e94e9c8fef4130ac8bc1e94 Mon Sep 17 00:00:00 2001 From: Rafa Audibert Date: Tue, 28 Jul 2026 04:46:08 -0300 Subject: [PATCH 2/3] chore(engineering-analytics): add two CI-reading traps to the rafapherding skill Both hit live while shepherding the stack the skill was written from, and neither was in it. A push cancels the run in flight for that branch, and the aggregation gates then report their CANCELLED dependencies as FAILURE. A wall of "Tests Pass" failures right after pushing means superseded, not broken, and a CI monitor that does not filter gates will wake you on every push. Adds the state-breakdown check that distinguishes them and the jq filter for real failures only. A stale test patch does not reliably fail. When the URL under test resolves for real, the test makes a live request and passes on whatever production returns, so the local suite going green proves nothing about whether every seam was found. Says to grep the repo for the old seam immediately after moving one, before running anything. Co-Authored-By: Claude Opus 5 (1M context) --- .../skills/rafapherding/SKILL.md | 13 ++++++++++++ .../rafapherding/references/reading-ci.md | 20 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/products/engineering_analytics/skills/rafapherding/SKILL.md b/products/engineering_analytics/skills/rafapherding/SKILL.md index 59871bc9e6d8..9e40c7e6b6f3 100644 --- a/products/engineering_analytics/skills/rafapherding/SKILL.md +++ b/products/engineering_analytics/skills/rafapherding/SKILL.md @@ -159,6 +159,19 @@ thing that still covers the path; when you must patch something process-wide (`r 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. diff --git a/products/engineering_analytics/skills/rafapherding/references/reading-ci.md b/products/engineering_analytics/skills/rafapherding/references/reading-ci.md index fd650977b80b..48f3d50ebf23 100644 --- a/products/engineering_analytics/skills/rafapherding/references/reading-ci.md +++ b/products/engineering_analytics/skills/rafapherding/references/reading-ci.md @@ -37,6 +37,26 @@ Jobs named ` Tests Pass`, ` Checks Pass`, `Check matrix ou 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 --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 --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. From 152b483d7a8da6a5c62219bfc53845e89d72f17f Mon Sep 17 00:00:00 2001 From: Rafa Audibert Date: Tue, 28 Jul 2026 05:01:47 -0300 Subject: [PATCH 3/3] chore(engineering-analytics): document splitting a blocked migration Unlike the hot-table and atomic=False policies, a BLOCKED verdict from the migration risk analyzer has no acknowledgement file: it has to be split. The two rules that catch people are that RunPython may not share a file with schema changes and RunSQL may not share a file with anything, since both hold their locks for the whole file. Records the one-shape-per-migration layout, the ordering constraints a split has to preserve, and the three commands that verify the result. Also notes that this surfaces late, because the job reporting it is one of the ones most often lost to runner starvation, so a migration can sit blocked across several pushes unseen. That is how it went here. Co-Authored-By: Claude Opus 5 (1M context) --- .../skills/rafapherding/SKILL.md | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/products/engineering_analytics/skills/rafapherding/SKILL.md b/products/engineering_analytics/skills/rafapherding/SKILL.md index 9e40c7e6b6f3..563733152c77 100644 --- a/products/engineering_analytics/skills/rafapherding/SKILL.md +++ b/products/engineering_analytics/skills/rafapherding/SKILL.md @@ -147,6 +147,34 @@ Renumbering a migration invalidates your local test database, which will then fa `DuplicateColumn: column ... already exists`. That is local-only; rebuild with `hogli test -- --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 # 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