diff --git a/.claude/skills/bug-fixing/SKILL.md b/.claude/skills/bug-fixing/SKILL.md new file mode 100644 index 00000000000..63a59d87573 --- /dev/null +++ b/.claude/skills/bug-fixing/SKILL.md @@ -0,0 +1,83 @@ +--- +name: bug-fixing +description: Fix a PHPStan bug the way it gets merged — reproduce, write the failing test first, fix the root cause, pass the full gate, and open the PR +argument-hint: "[issue-number or bug description]" +#allowed-tools: Read, Grep, Glob, Write, Edit, Bash(curl *), Bash(gh *), Bash(git *), Bash(make *), Bash(php *), Bash(vendor/bin/phpunit *), Agent +--- + +# Fixing a PHPStan bug + +You are given a bug (an issue number, or one you have already reproduced). Your goal is a change that a maintainer would merge as-is: the fix, a regression test that proves it, all gates green, and adjacent occurrences of the same bug covered. This skill assumes the principles in `CLAUDE.md` (never `instanceof *Type`, never sort/compare via `describe()`, prefer adding a `Type` method over scattered checks, fix the algorithm not the limit, named `_LIMIT` constants) and does not repeat them. + +The rule this skill exists to enforce: **"tests pass" is not the finish line.** That only proves you did not break the suite. The finish line is a fix proven to fail-before and pass-after, with every gate read, not just run. + +## Step 1 — Reproduce first + +You cannot fix what you cannot reproduce. If given an issue number, fetch it and its playground reproducer (`gh issue view --repo phpstan/phpstan --json title,body,comments`, then `curl -s 'https://api.phpstan.org/sample?id='`). Get to a state where you observe the bug — a failing `vendor/bin/phpunit` run or `php bin/phpstan analyse -l test.php --debug`. Match the playground's level and `config.*` flags rather than the repo's own `phpstan.neon.dist`, which forces strict-rules and bleedingEdge on and can add or mask errors (see `bug-reporting` Step 2 for how). + +## Step 2 — Write the failing test first + +Pick the test home by what the bug is about (see `CLAUDE.md` "Choosing the test home for a regression"): +- Wrong inferred type / missing narrowing → `tests/PHPStan/Analyser/nsrt/bug-.php` with `assertType()` (auto-discovered). +- Rule reports a wrong or missing error → `tests/PHPStan/Rules//data/bug-.php` plus a test method in the rule's `*Test.php`. +- Cache, config, DI, or parser behaviour → an `e2e/` fixture (see the existing `result-cache-*` projects for the shape). +- If the bug touches more than one, add a test for each. + +Copy the reproducer verbatim from the playground. Run the test and watch it **fail for the right reason** — the same symptom the issue describes, not an unrelated error. This failing test is the anchor for everything that follows. Once it fails correctly, do not rephrase the tested code. + +## Step 3 — Fix the root cause + +Fix the mechanism, not the symptom. Teach inference or the engine to handle the original code; do not rewrite the code under analysis, and do not lower or cap a limit to dodge slowness (find the algorithmic cause — a temporary raise to *expose* the slowness is fine, but restore it). When the check you need is "is this type a Foo?", look for an existing `Type` method first and add one if the check belongs on every type. + +## Step 4 — Scan adjacent code and cover it + +After the fix passes its test, look for the same bug in the sibling code paths (other accessory types, methods vs properties vs constants, other call sites of the changed API). For each occurrence you find, add its own failing test and fix it in the same change. If the siblings are genuinely unaffected, say why. Skipping this step is how the same bug gets reported again a month later. + +## Step 5 — Verify (the gate — nothing is done until all pass) + +Read the **output** of each command, not just its exit code. + +1. **Fails-before / passes-after.** This is the proof the test is real. Stash only the source fix by path so the test stays in place (plain `git stash` would also stash an uncommitted test unless it is a brand-new untracked file): + ```bash + git stash push -- src/ # stash ONLY the source fix; adjust to wherever it lives + vendor/bin/phpunit # MUST fail, for the right reason + git stash pop + vendor/bin/phpunit # MUST pass + ``` + Confirm the stash actually captured the fix (`git stash show` should list your source files): a fix can live under `stubs/`, `conf/`, or `resources/` rather than `src/`, and an empty stash silently leaves the fix applied, so the test passes and fails-before proves nothing. If the test still passes with the fix genuinely stashed, it does not cover the bug — fix the test before touching anything else. If it errors with "no tests found," you stashed the test too: narrow the stash path to the source files only. +2. **Full suite.** `make tests` (filter to your test during iteration, full suite before commit). +3. **Self-analysis.** `make phpstan` — clean. This catches things unit tests miss, notably constructor-signature changes that break instantiation sites in test base classes. +4. **Coding standard on the files you touched.** `php build-cs/vendor/bin/phpcs ` (run `make cs-install` once first if `build-cs/` is not yet present; `make cs` scans everything). Do this **before** you commit or push — a `ReferenceViaFallbackGlobalName` or missing `use function` will turn CI red after the fact. +5. **e2e and YAML.** If you touched config, DI, result-cache, or the parser, run the relevant `e2e/` fixture locally and validate any YAML you edited. There is no `make e2e` target: each fixture's exact steps live in `.github/workflows/e2e-tests.yml` (`cd e2e/ && …`), so replicate them by hand. Top-level `e2e/` is not phpcs- or self-analysis-scanned, so its correctness rides entirely on that run. +6. **Adjacent-bug scan complete.** You looked (Step 4) and either fixed the siblings or recorded why they are safe. +7. **Adversarial double-check (non-trivial change).** Spawn a fresh-context verifier subagent (Agent tool). Give it the diff and the claim: *"this fix is complete, all gates pass, there is no regression, and it does not over-reach."* Task it to refute, specifically: + - Does the test actually fail before the fix and pass after (re-derive, do not trust the claim)? + - Is every committed test fixture in its intended baseline state, not a mutated/patched state left over from a validation run? + - Are there sibling code paths with the same unfixed bug? + - Does the fix over-reach — break a fast path, over-invalidate a cache, widen a type that should stay narrow? + A fresh context catches what the context that wrote the code rationalises away. Treat its findings as blocking. + +## Step 6 — Commit + +One fix, one commit. The test, any bench script, and the source change go **together** in the same commit. The commit subject describes the **change**, not the bug it closes ("Do not subtract TemplateType from TemplateType", not "Fix #14459"). Issue closure belongs in the PR body (`Closes https://github.com/phpstan/phpstan/issues/`), not the subject. Follow the repo's commit-trailer conventions. + +```bash +git add tests/ src/ # stage specific paths, never `git add -A` +git commit +``` + +Stage specific paths. `git add -A` sweeps up stray tmp/cache artifacts and, via rename detection, can pair unrelated identical files. + +## Step 7 — Open the PR + +Branch and base matter. Do the work on a feature branch off the correct target: PHPStan fixes go on the active minor branch (`upstream/2.2.x` at the time of writing — confirm which minor is currently active before you branch), never straight onto a default branch. If you committed on the base branch by accident, branch first, then reset the base back. + +```bash +git checkout -b upstream/2.2.x # if not already on a feature branch +git push -u +gh pr create --repo phpstan/phpstan-src --base 2.2.x \ + --title "" \ + --body " Closes https://github.com/phpstan/phpstan/issues/" +``` + +Title and body describe the **change**, not the bug they close; the `Closes` link lives in the body. Keep the PR to **one concern** — split anything that mixes, say, a behavioural fix with an unrelated refactor. Before you propose, read the git history and referenced PRs of the files you touch, so the change does not fight a recent deliberate decision. Opening a PR is an outward action — confirm with the human first unless they have already said to. Then watch the first CI run settle green before calling it done: read the check rollup, tell your checks apart from repo-wide-flaky jobs, and never report success on red or pending. Handling review feedback afterwards is the `pr-review-feedback` skill. diff --git a/.claude/skills/bug-reporting/SKILL.md b/.claude/skills/bug-reporting/SKILL.md new file mode 100644 index 00000000000..ddb6d50a659 --- /dev/null +++ b/.claude/skills/bug-reporting/SKILL.md @@ -0,0 +1,87 @@ +--- +name: bug-reporting +description: Turn an observation that PHPStan misbehaves into a verified root cause and a GitHub issue that reproduces +argument-hint: "[claim, playground URL, or short description]" +#allowed-tools: Read, Grep, Glob, Write, Edit, Bash(curl *), Bash(gh *), Bash(git *), Bash(php *), Agent +--- + +# Reporting a PHPStan bug + +You are given an observation that PHPStan gets something wrong: a false positive, a false negative, a wrong inferred type, a crash, a performance regression, or a stale-cache result. Your goal is to reach a **verified root cause** and write an issue that anyone can reproduce. Do not write the issue until you have reproduced the bug yourself. + +A bug report is only as good as its reproduction. A plausible-sounding report that nobody can reproduce wastes everyone's time, including the AI that will be asked to fix it. + +## Step 1 — Pin down the claim + +State, in one line, what PHPStan does and what it should do instead. If you cannot state the expected behaviour precisely, you do not understand the bug yet. + +If you were given a PHPStan Playground link (`https://phpstan.org/r/`), fetch the sample so you have the exact code, level, and config: + +``` +curl -s 'https://api.phpstan.org/sample?id=' +``` + +The JSON gives you `code`, `level`, `config.*` flags, and `versionedErrors` (what PHPStan reported per PHP version). The reproducer for the issue comes from this link, not from prose paraphrased later in a thread. + +## Step 2 — Reproduce it verbatim + +Copy the reproducing snippet **as-is** into a `test.php` at the repo root. Do not tidy it, rename things, or trim it yet. + +```bash +php bin/phpstan analyse -l test.php --debug +# add -vvv for hangs, infinite loops, or memory blowups +``` + +Beware the repo's own config: at the repo root, `bin/phpstan` auto-loads `phpstan.neon.dist`, which forces strict-rules, bleedingEdge, and the deprecation and PHPUnit rules on. That can add errors that are not the bug, or hide one that only appears under the playground's settings. Reproduce with the playground's own `level` and `config.*` flags — point `-c` at a throwaway config that matches them, or analyse from a scratch directory outside the repo. + +Confirm you observe the reported misbehaviour with your own eyes. If you cannot reproduce it, stop: either the report is incomplete (note exactly what is missing) or there is no bug. Never write an issue for something you have not seen happen. + +Reach for the debug helpers before guessing: +- `\PHPStan\dumpType($expr)` in the analysed code prints the inferred type at that point. +- `\PHPStan\debugScope()` prints the current scope. +- Inside `NodeScopeResolver`, `var_dump($scope->debug())` is the canonical inspection point. + +## Step 3 — Root-cause to a mechanism + +An RCA is not "it seems related to X." It is: **this exact `file:line` does this, which produces the wrong result, because of this reason.** Trace it until you can point at the line and explain the mechanism. If you are reporting a performance or cache-soundness bug, back the mechanism with a counter or a before/after observation, not a hunch — profiles can be silently truncated and still look internally consistent. + +You do not have to fix the bug to report it, but you should understand it well enough that the fix location is not a mystery. + +## Step 4 — Verify any regression claim + +If the issue will say "regression from " or "worked before ", you must observe **both** sides. Check out the earlier version (or `git revert --no-commit `), reproduce, and confirm the old behaviour differs. Do not assert a regression you have only seen one side of. A wrong "regression from" claim sends the fix hunting in the wrong place. + +## Step 5 — Scan adjacent code + +Bugs in the type system rarely live alone. A fault in one accessory type usually has counterparts in the sibling accessory types; a fault in property handling often repeats for methods and constants; a fault at one call site of a `Type` API often repeats at the others. Note the parallel code paths so the issue (and later the fix) can name them. + +## Step 6 — Write the issue + +Structure it so a stranger can act on it: + +- **What's wrong** — one or two sentences. +- **Reproduction** — the minimal code (verbatim from the playground, trimmed only if you re-verified the trim still reproduces), the level, and the relevant config flags. Use only clearly fictional placeholders for names, IDs, and values. Never paste real customer data, production credentials, API keys, tokens, or connection strings into a public issue. +- **Root cause** — the `file:line` and mechanism from Step 3, if you have it. +- **Expected output** — what should happen instead. +- Whether you intend to open a PR. + +Write it plainly, in your own prose. Describe the behaviour, not who found it. + +## Step 7 — Verify (do not skip) + +Read the output of each check; an exit code alone is not evidence. + +1. **Re-reproduce from clean.** `git stash` any local edits (including your `test.php` scaffolding if it changed source), reproduce the bug once more from a clean tree, and confirm the symptom is exactly what the issue describes. +2. **Every claim is backed.** Walk the issue body sentence by sentence. Each factual claim must map to something you observed: the repro output, the per-version behaviour for a regression claim, the counter for a perf claim. Delete any claim you cannot back. +3. **Confidentiality gate.** Search the whole issue body for internal names, real identifiers, and secrets before it leaves your machine. Only fictional placeholders and public project names belong in a public issue. +4. **Adversarial double-check (non-trivial RCA).** Spawn a fresh-context verifier subagent (Agent tool) given only the drafted issue body and the reproducer. Task it to (a) reproduce the bug independently from the issue text alone, and (b) list any claim in the body not supported by evidence it can see. Treat its findings as blocking: fix the body or the repro before proceeding. + +## Step 8 — Post (outward action) + +Posting a public issue is an outward action. Confirm with the human before posting unless they have already told you to post it. Then: + +``` +gh issue create --repo phpstan/phpstan --title "" --body-file <path> +``` + +If you will follow up with a fix, keep the reproducer you built: the `bug-fixing` skill starts from exactly this failing state. diff --git a/.claude/skills/performance-work/SKILL.md b/.claude/skills/performance-work/SKILL.md new file mode 100644 index 00000000000..fd897bc075f --- /dev/null +++ b/.claude/skills/performance-work/SKILL.md @@ -0,0 +1,74 @@ +--- +name: performance-work +description: Investigate a PHPStan performance problem and land it as a measured fix or a well-framed issue, without dumbing down analysis +argument-hint: "[perf symptom, issue number, or profile]" +#allowed-tools: Read, Grep, Glob, Write, Edit, Bash(curl *), Bash(gh *), Bash(git *), Bash(make *), Bash(php *), Bash(hyperfine *), Bash(vendor/bin/phpunit *), Agent +--- + +# Landing a PHPStan performance change + +You are given a performance symptom: something is slow, memory-heavy, or a run regressed. Your goal is either a **measured** fix that a maintainer merges, or a **well-framed issue** that lets someone else fix it — and to know which one you are aiming for before you write code. + +Set expectations honestly up front. In our experience most per-op micro-optimizations come back **sub-1%**: PHPStan's hot paths (reflection, parsing, PHPDoc resolution) are already heavily cached, so the eliminable per-op residual is usually tiny. The perf changes that actually land are **algorithmic** (a genuine super-linear blowup) or about **what a long-lived process retains** (cache policy, eviction, reference sharing, retained graphs), not about shaving cheap calls. This skill assumes the principles in `CLAUDE.md`; one is load-bearing enough to restate: **solve the algorithm, not the limit** — never "fix" slowness by lowering or capping iterations, recursion depth, fan-out, or collection size (a temporary raise to *expose* the cost is fine; restore it once the real fix lands, and keep every threshold in a named `_LIMIT` constant). + +## Step 1 — Decide: fix (PR) or report (issue)? + +First separate two things people both call "performance": a **slow-but-correct** run (the subject of this skill) versus a **cache-soundness** bug where a fast path returns the *wrong* result (stale cache, under-invalidation). A soundness bug has wrong output and therefore *can* have a failing test — treat it as a correctness bug via `bug-reporting` / `bug-fixing`, not here. + +A genuinely slow-but-correct run produces **correct output**, so there is **no failing test**. Benches under `tests/bench/data/` guard a *landed* optimization or a *fixed* blowup; there is no standalone "this is still slow" bench, so submitting a bench for code you have not fixed does not fit. Route the work by where the cost lives: + +- **A localized fast-path, or a retention / cache-policy fix** (memoize a repeated computation, add an eviction bound, restore reference sharing) — usually a **PR**, and these are the changes that most reliably land. Continue here, then ship it through `bug-fixing`'s gate. +- **The cost is the caller generating O(N²⁺) cheap calls** deep in the engine (`MutatingScope`, `TypeSpecifier`, `NodeScopeResolver`) — maintainer-grade. Aim for an **issue**: a minimal synthetic reproducer, a generator one-liner, a scaling table, and a *narrowed* diagnosis (which call count explodes, plus hypotheses you already disproved to shrink the fix-space). Use the `bug-reporting` skill; file it on phpstan/phpstan as a **Feature request** so the issue-template bot does not demand a playground link (the Bug-report path requires one, and a perf blowup is too large to paste on the playground anyway). + +Do not start writing a fast-path before you know the cost is localized. Fixing the symptom in the wrong layer is the most common wasted-effort mode here. + +## Step 2 — Establish an honest baseline + +Reproduce on a **real-world codebase**, not a microbenchmark — maintainers reject microbenchmark-only wins ("did you run this on a real project?"). Measure two clean checkouts (before and after) with `hyperfine`, and clear PHPStan's result cache before every timed run so you measure analysis, not a warm-cache restore: + +```bash +hyperfine --warmup 1 \ + -L bin before/bin/phpstan,after/bin/phpstan \ + --prepare '{bin} clear-result-cache -c <config>' \ + '{bin} analyse -c <config> --no-progress' +``` + +The result cache lives in a config-keyed temp dir *outside* the checkouts, so without `--prepare` the warmup populates it and every timed run is incremental — dominated by startup plus cache restore, near-zero re-analysis, the opposite of what you meant to measure. State whether you are timing **cold** (cleared each run, as above) or **warm** (cache pre-populated). For a warm comparison, give each build its own `-c <config>` (and thus its own `tmpDir`) so the two cannot share a cache; the cold command above sidesteps this by clearing before every run. + +State the scope with every number: corpus and file count, PHP version, arch, and **wall time vs user-CPU separately**. Interleave the two builds (alternate A/B) rather than all-A-then-all-B, so machine drift does not masquerade as a delta. + +Validate on **more than one kind of project**. A lever can be a win on one and flat or negative on another: a Doctrine/Symfony codebase and a Laravel + larastan codebase stress different paths (type diversity vs extension and DI load), and a change that helps one can regress the other. A single-corpus "win" is not validated. + +## Step 3 — Find the algorithmic root cause + +Measure the **warm residual**, never `redundancy × whole-run-per-op-cost`. That product overcounts every time the expensive part is shared or cached: the first-hit reflection deserialize is paid once (the cache's job), not redundantly, so counting it as eliminable invents a lever that is not there. Warm the caches, then isolate the thin per-op cost that actually repeats. + +Convert every count to the real metric before believing it. A huge call count or object-dedup count implies neither CPU nor memory: leaf `Type` objects are on the order of tens of bytes, so a 22× dedup can be ~0 MB, and a 46M no-op dispatch count can be well under 1% CPU. **Count is not time and count is not MB** — gate on an interleaved CPU A/B or a peak-memory A/B, not on the count. + +## Step 4 — Validate profile numbers with a counter + +Profiles lie by omission. An xdebug/callgrind profile can be **silently truncated** and still parse cleanly and look internally consistent — consistency checks do not catch truncation (in one real case the headline count was undercounted by roughly 66×). Before you publish or act on any profile-derived call count, confirm the headline number with a **direct instrumented counter**: patch a `$GLOBALS` counter plus a shutdown dump into the run. It is cheap, exact, and immune to the truncation failure mode. Interleaved *timing* conclusions are independent of this and can be trusted. + +## Step 5 — Verify (the gate — nothing is done until all pass) + +Read the **output** of each check, not just its exit code. + +1. **Footprint + correctness, both halves.** A *no-win* bug — the optimization silently never fires — is output-correct by construction, so it passes byte-identical and unit tests trivially. Only a footprint assertion catches it: + - **Footprint:** trigger the opt narrowly and assert only the expected subset is recomputed; trigger an *irrelevant* change and assert it recomputes **zero** / is served entirely from the fast path. + - **Correctness:** the optimized result is **byte-identical** to the unoptimized (cold / opt-disabled) result, so nothing is under-invalidated or stale. +2. **Both axes, on real corpora.** Peak memory **and** CPU — a memory win can cost CPU and the reverse (a retention fix that cut memory cost a few percent CPU on one corpus). Gate both; a self-analysis-only number over-represents wins. +3. **Measure, do not infer.** If you believe an existing PR or a related change already fixes this, run its reproducer against that branch and confirm — a maintainer's "this will likely improve it" is a hypothesis, and two similarly-named code paths are often orthogonal axes. +4. **If code changed, run the full `bug-fixing` gate** (a perf fix still ships a test or a `tests/bench/data` guard): fails-before/passes-after, `make tests`, `make phpstan`, `phpcs` on touched files. +5. **Adversarial double-check.** Spawn a fresh-context verifier subagent (Agent tool) with the diff, the numbers, and the claim: *"this is a real win, it actually fires, it is not a no-op, the numbers are not a profile artifact, and it generalizes beyond one corpus."* Task it to refute each clause — re-derive the footprint, re-check the byte-identical result, question the measurement method, and test the second-corpus claim. Treat its findings as blocking. + +## Step 6 — Frame it for acceptance + +Perf review is blunt. What converts it to a merge: + +- **Honesty about scope.** If the win is niche or synthetic (fires only on a shape no real project hits), say so plainly — "anti-pattern cleanup plus a scaling demo," not a claimed real-world hotspot. Maintainers merge an honest niche cleanup and reject an overclaimed one. +- **Numbers with explicit scope.** hyperfine before/after, corpus and file count, PHP, arch, wall vs CPU. Overblown or unscoped numbers get the PR closed. +- **One concern per PR.** Split aggressively — a hash-algorithm change and a cache-policy change are two PRs, not one. +- **Read the prior art.** Read the git history and referenced PRs of every file you touch before proposing, so you do not fight a recent deliberate decision (much of what looks like an easy win was already tried and rejected for portability or correctness). +- **Try the maintainer's counter-idea.** If a reviewer proposes a different home for the change, actually build and A/B it, then report the trade-off honestly and offer to switch. + +If Step 1 sent you to an issue, this framing is the issue body. If it is a PR, open it through `bug-fixing` Step 7 and carry these points into the description. diff --git a/.claude/skills/pr-review-feedback/SKILL.md b/.claude/skills/pr-review-feedback/SKILL.md new file mode 100644 index 00000000000..36c6790b03b --- /dev/null +++ b/.claude/skills/pr-review-feedback/SKILL.md @@ -0,0 +1,63 @@ +--- +name: pr-review-feedback +description: Apply PHPStan PR review feedback with critical evaluation, re-run the gate, and confirm CI is green before replying +argument-hint: "[PR number and/or review/comment URL]" +#allowed-tools: Read, Grep, Glob, Write, Edit, Bash(gh *), Bash(git *), Bash(make *), Bash(php *), Bash(vendor/bin/phpunit *), Agent +--- + +# Applying PHPStan PR review feedback + +You have review feedback on an open PR. Your goal is to address every **valid** ask fully, re-prove the change with the gate, and confirm CI is green before you tell anyone it is done. This skill reuses the `bug-fixing` Verify gate. + +## Core principle — evaluate, do not apply blindly + +**"Address every ask fully" is not "apply every suggestion."** Any reviewer can be wrong, and a suggested fix can be worse than the code it replaces. Judge each piece of feedback on its merits before you touch anything, and weight it by who wrote it: + +- **A maintainer or another human** is usually right about *what* is wrong, but their proposed fix is a suggestion, not a spec. Implement the intent; if you disagree or see a better option, discuss rather than comply silently. +- **A bot or automated reviewer** (CI annotations, Copilot, and the like) is frequently wrong. Be skeptical of "dead code" (may be deliberately unused), generic security warnings (verify a real issue exists), and "missing type hint" nags (the repo's own rules may already cover it). +- **Your own adversarial-verifier subagent** is feedback too — and it can be mistaken or propose a suboptimal fix. Verify each finding before acting: a claim about git history, a fixture's state, or a command's behaviour is checkable, so check it. + +Before acting on any ask, run it through: does it improve correctness or clarity? does it match the repo's conventions and a recent deliberate decision? is it a subjective preference? is it simply wrong? Apply the ones that hold up; skip or push back on the rest with a reason. **Verify factual claims in the feedback before you rebuild around them** — that verification is exactly what catches a reviewer (human or bot) who is confidently wrong. + +## Step 1 — Read the feedback precisely + +Fetch the review and comments so you work from the exact words: + +``` +gh pr view <n> --repo phpstan/phpstan-src --json title,body,reviews,comments,statusCheckRollup,headRefOid +gh api repos/phpstan/phpstan-src/pulls/<n>/comments # inline review comments, if needed +``` + +Separate the distinct asks and restate each in one line, tagged with who raised it (maintainer / bot / self). If an ask is ambiguous, ask — do not guess what a maintainer meant and build the wrong thing. + +## Step 2 — Make the change (only for asks that hold up) + +For each ask you judged valid, make the smallest change that **fully** addresses it, and keep the PR to one concern. When a comment points at a specific example, decide whether it is really pointing at a general gap: if a reviewer says "this misses X-style packages," fix the class of the problem, not just that one example — papering over the named case leaves the reviewer to find the next one. For an ask you are skipping or disagree with, draft a short reason instead of a change. + +## Step 3 — Verify (before you push) + +Re-run the full `bug-fixing` Verify gate on the **exact files this response touched** — fails-before/passes-after for any new test, `make tests`, `make phpstan`, `php build-cs/vendor/bin/phpcs <touched files>`, and the relevant `e2e/` fixture plus YAML validation if you touched config, DI, cache, or the parser. In addition: + +1. **Check every committed fixture and baseline is in its intended pre-change state.** Validation runs mutate fixtures (a `patch` step, a `composer update`, a generated cache). It is easy to `git commit --amend` with a fixture left in its post-run state — for example a package fixture committed at its patched `v2` instead of the `v1` baseline the test expects. Confirm with `git show HEAD:<fixture-path>` that what is committed is the baseline, then reset your working tree so a stray validation artifact does not get re-staged. +2. **phpcs before the push, not after.** On an unreviewed-since branch, prefer `git commit --amend` and `git push --force-with-lease` to keep one clean commit — but run phpcs on the touched files *first*. Force-pushing and then discovering a style violation costs a second force-push and a red CI in between. +3. **Adversarial double-check (non-trivial response).** Spawn a fresh-context verifier subagent (Agent tool) with the diff and the claim "this response fully addresses the feedback, the gate passes, and every fixture is at its baseline." Task it to refute — especially the fixture-state check above, which a fresh reader catches and the author's own context rationalises away. Then run *its* findings through the core principle: confirm each before acting, since the verifier can be wrong too. + +## Step 4 — Push, then wait for CI to settle green + +Pushing is not "done." Read the actual check rollup and let it finish: + +``` +gh pr view <n> --repo phpstan/phpstan-src --json headRefOid,mergeable,statusCheckRollup \ + --jq '{head:.headRefOid, mergeable:.mergeable, failing:[.statusCheckRollup[]|select(.conclusion=="FAILURE")|.name], pending:[.statusCheckRollup[]|select(.status!="COMPLETED")|.name]}' +``` + +Confirm the reported head matches the commit you pushed. Distinguish **your** checks from repo-wide-flaky ones (some integration jobs fail on every recent PR regardless of the change — verify a failure reproduces on the target branch before blaming your change). Do not report success while any of your checks are red or still pending. + +## Step 5 — Reply and resolve (outward action) + +Reply only once CI is green. Posting to a public PR is an outward action, and replying to or resolving a **maintainer's** thread is theirs to expect, not yours to auto-do: + +- **Never auto-reply or auto-resolve a maintainer's review thread** without the human's go-ahead in the current conversation. Applying the code change they asked for is within intent; speaking on the PR on their behalf is not. Present your replies as drafts and let the human send them, unless they have said to post. +- Keep any reply short, in your own voice, and acknowledge what the review surfaced when it genuinely improved the change. Do not repeat the reviewer's comment back to them. + +If the Step 3 double-check found a real problem, that is a signal to *fix and re-verify*, not to post "done" and hope.