diff --git a/products/engineering_analytics/skills/rafapherding/SKILL.md b/products/engineering_analytics/skills/rafapherding/SKILL.md new file mode 100644 index 000000000000..563733152c77 --- /dev/null +++ b/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 --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. + +### 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 +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. 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..48f3d50ebf23 --- /dev/null +++ b/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 ` 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. + +### 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. + +```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.