From ccf028b14c7826ec835fc124f6a66bdaf73b593c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 11:28:01 +0000 Subject: [PATCH 1/6] Add a skill for verifying Dependabot PRs before merging The skill finds where a bumped package touches our code, adds the smallest test of that integration, proves it red and green against both our code and the library, and pushes it to the Dependabot branch for a human to review. Once a package's tests are reviewed and recorded in .github/dependency-coverage.json, later minor and patch bumps can auto-merge on green CI. Major bumps always go to a human. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019QZFw9N8p8vhvtvA3LDZG1 --- .claude/skills/verify-dependabot/SKILL.md | 279 ++++++++++++++++++++++ .github/dependency-coverage.json | 3 + readme.md | 6 + 3 files changed, 288 insertions(+) create mode 100644 .claude/skills/verify-dependabot/SKILL.md create mode 100644 .github/dependency-coverage.json diff --git a/.claude/skills/verify-dependabot/SKILL.md b/.claude/skills/verify-dependabot/SKILL.md new file mode 100644 index 0000000..1f99bd5 --- /dev/null +++ b/.claude/skills/verify-dependabot/SKILL.md @@ -0,0 +1,279 @@ +--- +name: verify-dependabot +description: Decide whether a Dependabot pull request can be merged. Finds where the bumped package touches our code, adds the smallest test that would catch a change in that integration (proved with red/green), pushes it to the Dependabot branch, and either hands the PR to a human or enables auto-merge, depending on whether the package has been verified before and how big the version jump is. Use when asked to check, verify, triage or merge Dependabot PRs, or with a PR number of a Dependabot PR. +--- + +# Verify a Dependabot PR + +We don't have enough test coverage to trust a green CI run on a dependency +bump. This skill closes that gap one package at a time: the first time a +package is bumped, we write a test that pins down how *our code* uses it, and +a human reviews that test. From then on, minor and patch bumps of that package +can merge on their own as long as the test stays green. + +We are not testing the library. It has its own tests. We are testing the seam +between the library and our code: the inputs we pass it and the outputs we +rely on. + +Arguments: a PR number, or nothing to work through every open Dependabot PR +(oldest first). + +## The gears + +The ledger is `.github/dependency-coverage.json`. An entry for a package on +the default branch means a human has reviewed and merged the tests listed in +it. The ledger only reaches the default branch through a merged PR, so "the +entry exists on `master`" is the approval record. Read it from `origin/master`, +never from the PR branch. + +| Situation | Gear | What happens | +|---|---|---| +| No ledger entry on `master` | 1 | Write tests, push them, comment, **wait for a human**. | +| Ledger entry, minor or patch bump | 2 | Re-check usage, wait for CI, **enable auto-merge**. | +| Major bump (any ledger state) | 3 | Do the gear 1 or 2 checks, read the changelog, **wait for a human**. | + +Rules that decide the gear: + +- `0.x` versions: treat a minor bump (`0.10` to `0.11`) as major, since semver + allows breaking changes there. +- A PR that bumps several packages takes the highest gear of any of them. +- If anything in gear 2 needs a code change (a new test, a fix to our code), + the PR drops to gear 1. +- Anything you are unsure about goes to a human. Auto-merge is the reward for + certainty, not the default. + +## Step 1: Read the PR + +1. Get the PR (`mcp__github__pull_request_read`, `get` and `get_files`). Check + the author is `dependabot[bot]`. If not, stop: this skill is only for + Dependabot PRs. +2. From the title, body and lockfile diff, list every package that changes, + with old and new versions, and the directory (`packages/join-flow`, + `packages/join-e2e`, `packages/join-block`, or the root). +3. Work out the gear for each package from the table above. +4. Fetch the branch: `git fetch origin ` and check it out. + +## Step 2: Find where it touches our code + +Most Dependabot PRs here bump **transitive** dependencies (security updates +for things like `fast-uri` or `follow-redirects`) that our code never imports. +So first find out how the package gets in: + +- npm: `npm why ` in the package's directory. +- Composer: `composer why ` in `packages/join-block`. + +Then put it in one of these classes. The class decides what "the smallest +test" is. + +| Class | How to tell | What verifies it | +|---|---|---| +| `runtime-direct` | We import it in `src/` (JS) or `src/` (PHP) | A unit test of our code that calls it. | +| `runtime-transitive` | A direct dependency we import at runtime depends on it, and it ends up in the shipped bundle or plugin | A unit test of our code that calls the direct dependency, along a path that reaches this package. | +| `build` | Only webpack, Babel, loaders or Sass depend on it | `npm run build` succeeds, and the E2E suite (which runs on the built bundle) passes. | +| `dev-server` | Only `webpack-dev-server` depends on it | Start the dev server and fetch a page and a proxied path (see below). Never ships to users. | +| `test-tooling` | Only Jest, Testing Library, PHPUnit, PHPStan, phpcs or Playwright depend on it | The suite that uses it runs and passes. | + +For `runtime-direct` and `runtime-transitive`, list every call site: + +```bash +# JS: imports and requires of the package (and its subpaths) +grep -rnE "from ['\"](/[^'\"]*)?['\"]|require\(['\"]" packages/join-flow/src +# PHP: use statements and fully qualified names of its namespace +grep -rn "" packages/join-block/src packages/join-block/join.php +``` + +Group call sites by **usage pattern**: the functions we call and the shape of +what we pass. Five call sites that all call `format(date, "yyyy-MM-dd")` are one +pattern. A test per pattern is enough. A test per call site is too many. + +For `build`, `dev-server` and `test-tooling`, skip to step 4. There is no seam in +our code to test, and CI or the smoke check already exercises the tool. + +## Step 3: Write the smallest test (runtime classes only) + +First check whether an existing test already covers each pattern: grep the +test directories for the function in our code that wraps the call. If a test +covers the pattern *with the real library* (not mocked), you can use it. +Record it in the ledger and still do the red/green proof below. + +Otherwise write a new one. What makes a test small and useful here: + +- **Test our function, not the library's.** Call the function in our code that + uses the package, with the real package installed. Never mock the package + you are verifying. +- **Assert on what we depend on.** If we use `parsePhoneNumber(x).format(E164)` + to build a CRM payload, assert on the E.164 string we send, not on every + property of the parsed object. +- **One `it`/test method per usage pattern**, with inputs that hit the + behaviour we rely on, including the edge cases our code handles (empty + values, the formats our members actually enter). +- **No snapshots of whole components** and no network calls. +- **Put it where it belongs**: next to the code, following the existing + conventions. Jest tests are `packages/join-flow/src/**/*.test.ts(x)`. PHPUnit + tests are `packages/join-block/tests/*Test.php`. Add to an existing test file + for that module when there is one. +- **Make it findable**: name the `describe` block (Jest) or add a docblock line + (PHPUnit) with `dependency: `, so the next run can grep for it. +- Add a one-line comment saying which behaviour of the package the test + depends on and why it matters to members or organisers. + +### Red/green proof + +A test that cannot fail proves nothing. Prove each new or reused test in this +order and keep the output of each run for the PR comment. + +1. **Green on the old version.** Restore the base lockfile and install: + ```bash + git checkout origin/master -- /package.json /package-lock.json # or composer.json/composer.lock + npm --prefix ci # or composer install + ``` + Run only the new test (`npx jest -t ""`, or + `./vendor/bin/phpunit --filter `). It must pass. +2. **Red when our integration breaks.** Make a temporary change in *our* code + at the call site: pass a different argument, drop an option, or use the + result differently. Run the test. It must fail, and it must fail on the + assertion you wrote rather than on a crash in the test setup. Revert the change. +3. **Red when the library's behaviour changes.** Temporarily edit the + package's code in `node_modules/` or `vendor//` + so the function we rely on returns something slightly different (a changed + format, an off-by-one, a missing field). Run the test. It must fail. + Restore with `npm ci` or `composer install`. + This is the step that shows the test would catch a bad upgrade. If you + can't make the test fail this way, the test is checking our code only. + Rewrite it. +4. **Green on the new version.** Restore the PR's lockfile and install: + ```bash + git checkout HEAD -- /package.json /package-lock.json + npm --prefix ci + ``` + Run the test again. + - Passes: good. + - Fails: this is what the skill exists to catch. Read the package's + changelog for the change. If our code needs a small, clear fix, make + it and note it for the human reviewer. Otherwise leave the code alone and + report the failure on the PR. Either way the PR goes to a human. + +After the proof, `git status` must show only the new or changed test files, +the ledger, and any fix to our code. No leftover mutations, and no changes to +`package.json` or the lockfiles beyond what Dependabot made. + +Then run the full suite for the affected package on the new version (the +same commands CI runs: `npm test` and `npx prettier --check packages/join-flow` +for join-flow; `./vendor/bin/phpunit` and `composer lint` for join-block). + +## Step 4: Non-runtime classes + +- `build`: on the PR's lockfile, run `npm --prefix packages/join-flow run build` + and check it finishes without new errors or warnings compared with the base. + CI's E2E job runs the test build and the suite against it. +- `dev-server`: no CI job runs the dev server, so smoke-test it yourself on the + PR's lockfile: + ```bash + cd packages/join-flow && npx webpack serve --config ./webpack/dev.js & + # wait until it reports it is listening, then: + curl -fsS http://localhost:/ >/dev/null && echo "page ok" + kill %1 + ``` + If the PR touches the proxy chain (`http-proxy`, `http-proxy-middleware`, + `follow-redirects`), also fetch a path that goes through the proxy and check + you get the WordPress response, or a clean proxy error if WordPress isn't + running, rather than a crash. +- `test-tooling`: the suite that uses it is the test. Run it. + +These classes have nothing new to commit except the ledger entry. + +## Step 5: Update the ledger + +Add or update the package's entry in `.github/dependency-coverage.json`: + +```json +{ + "packages": { + "npm:packages/join-flow:google-libphonenumber": { + "class": "runtime-direct", + "via": [], + "usage": [ + "packages/join-flow/src/schema.ts" + ], + "tests": [ + "packages/join-flow/src/schema.test.ts › dependency: google-libphonenumber" + ], + "reviewedVersion": "3.2.40", + "reviewedIn": "#142" + } + } +} +``` + +- The key is `::`. The same package in + two directories gets two entries, because the usage is different. +- `via` is the dependency chain from `npm why`/`composer why`, top level first. + It is empty for direct dependencies. +- `usage` lists the files of ours that use it (from step 2). It is empty for + non-runtime classes. +- `tests` lists what verifies it. For non-runtime classes, name the CI + job or the smoke check, for example `"ci: E2E Tests › Build join-flow bundle"`. +- `reviewedVersion` and `reviewedIn` are the new version and this PR. Only + gear 1 and 3 PRs, which a human reviews, write them. + +Keep keys sorted so diffs stay readable. + +## Step 6: Push to the Dependabot branch + +Commit the tests, any fix and the ledger in one commit on the PR's head +branch, with a message saying what the tests pin down. Then +`git push origin HEAD:`. + +Things to know about pushing to a Dependabot branch: + +- After anyone else commits, Dependabot stops rebasing the PR. If it later + needs a rebase, rebase it yourself (`git rebase origin/master`, then + `git push --force-with-lease`). Do not comment `@dependabot recreate`, + because that throws away our commit. +- Don't touch `package.json` or the lockfiles. Dependabot owns those. + +## Step 7: Hand off or merge + +**Gear 1 and 3: hand off to a human.** Post one comment on the PR with: + +- The packages, versions, class and gear, and why it needs a human ("first + time this package has been verified", or "major version"). +- Where the package touches our code (file:line, grouped by pattern). +- The tests added or reused, and what each one pins down. +- The red/green evidence: for each test, the four runs from step 3 with the + mutation you made and a short excerpt of each failure. +- For major bumps: the breaking changes in the changelog and whether any of + them affect our usage. +- Any change to our code, and why it was needed. +- What the reviewer should check: that the tests assert on behaviour we + actually depend on, and that nothing we use was missed. + +Do not merge, approve, or enable auto-merge. + +**Gear 2: merge on green.** All of the following must hold. If any of them +fails, drop to gear 1. + +1. The ledger entry on `origin/master` matches this package and directory. +2. The usage grep from step 2 finds no files beyond the entry's `usage`. If + it finds new ones, the package is used in a new way that has no test yet: + cover it (step 3), which makes this PR gear 1. +3. `npm why`/`composer why` gives the same class as the entry. A dev-only + package that now ships to users needs a new review. +4. Every test listed in the entry still exists and passes on the new version. +5. Every CI check on the PR's head commit has finished and succeeded + (`get_check_runs`). Wait for checks that are still running rather than + enabling auto-merge while they are pending. + +Gear 2 makes no commits: the ledger records what a human reviewed, so it +keeps the reviewed version, and leaving the branch alone lets Dependabot keep +rebasing it. Enable auto-merge (`mcp__github__enable_pr_auto_merge`, merge +method `MERGE`) and post a short comment naming the ledger entry and the tests +that approved it. + +## When working through every open PR + +List open PRs by `dependabot[bot]` and handle them one at a time, oldest +first. Several PRs often bump packages in the same lockfile. After one +merges, the others may need a rebase (see step 6) before their CI means +anything. Finish with a table: PR, packages, class, gear, outcome (handed to +human / auto-merge enabled / failed, and why). diff --git a/.github/dependency-coverage.json b/.github/dependency-coverage.json new file mode 100644 index 0000000..5d5e1bf --- /dev/null +++ b/.github/dependency-coverage.json @@ -0,0 +1,3 @@ +{ + "packages": {} +} diff --git a/readme.md b/readme.md index 49768c3..f665d94 100644 --- a/readme.md +++ b/readme.md @@ -496,6 +496,12 @@ The suite tests against one pinned WordPress release: the `wordpress:-p The PHP version in the tags (8.2) matches the plugin's minimum and the PHP version used in CI and `composer.json`; change them together. +### Dependabot PRs + +The `/verify-dependabot` Claude Code skill (`.claude/skills/verify-dependabot/SKILL.md`) decides whether a Dependabot PR can merge. The first time a package is bumped, it adds the smallest test that pins down how our code uses the package, proves the test can fail, pushes it to the Dependabot branch and leaves the PR for a human to review. After that, minor and patch bumps of the same package auto-merge when CI is green. Major bumps always wait for a human. + +`.github/dependency-coverage.json` records which packages have reviewed tests. An entry only reaches `master` through a reviewed, merged PR, so editing it by hand bypasses that review. + ### Claude Code cloud sessions `.claude/hooks/session-start.sh` runs at the start of each Claude Code cloud session (and does nothing elsewhere). It installs the npm and Composer dependencies and starts Docker, so all three test suites can run. Two things are specific to that environment: From 2870999680dd121d771bd43817eece54a25d4b63 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 11:38:31 +0000 Subject: [PATCH 2/6] Check the dev server in CI and pick Dependabot reviewers by history Designers, engineers and headless Claude Code sessions all rely on the join-flow dev server, and no CI job ran it. scripts/dev-server-smoke.sh starts it and checks the page, the hot-reloading bundle and the stubbed REST endpoints; the new Dev Server workflow runs it on every PR. The verify-dependabot skill now requests review from whoever added the dependency (or the top-level dependency that pulls in a transitive one), falling back to whoever last changed it. scripts/bundled-packages.js tells it whether a package ships in the bundle or is build tooling only. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019QZFw9N8p8vhvtvA3LDZG1 --- .claude/skills/verify-dependabot/SKILL.md | 65 ++++++++++++++++------- .github/workflows/dev-server.yml | 27 ++++++++++ packages/join-flow/package.json | 3 +- readme.md | 4 ++ scripts/bundled-packages.js | 38 +++++++++++++ scripts/dev-server-smoke.sh | 63 ++++++++++++++++++++++ 6 files changed, 180 insertions(+), 20 deletions(-) create mode 100644 .github/workflows/dev-server.yml create mode 100755 scripts/bundled-packages.js create mode 100755 scripts/dev-server-smoke.sh diff --git a/.claude/skills/verify-dependabot/SKILL.md b/.claude/skills/verify-dependabot/SKILL.md index 1f99bd5..8fcc588 100644 --- a/.claude/skills/verify-dependabot/SKILL.md +++ b/.claude/skills/verify-dependabot/SKILL.md @@ -69,8 +69,8 @@ test" is. |---|---|---| | `runtime-direct` | We import it in `src/` (JS) or `src/` (PHP) | A unit test of our code that calls it. | | `runtime-transitive` | A direct dependency we import at runtime depends on it, and it ends up in the shipped bundle or plugin | A unit test of our code that calls the direct dependency, along a path that reaches this package. | -| `build` | Only webpack, Babel, loaders or Sass depend on it | `npm run build` succeeds, and the E2E suite (which runs on the built bundle) passes. | -| `dev-server` | Only `webpack-dev-server` depends on it | Start the dev server and fetch a page and a proxied path (see below). Never ships to users. | +| `build` | Only webpack, Babel, loaders or Sass depend on it, and none of its code ends up in the bundle | `npm run build` succeeds, and the E2E suite (which runs on the built bundle) passes. | +| `dev-server` | Only `webpack-dev-server` depends on it | `scripts/dev-server-smoke.sh`, which CI also runs as the Dev Server workflow. Never ships to users, but designers, engineers and headless Claude Code sessions all depend on the dev server, so it must keep working. | | `test-tooling` | Only Jest, Testing Library, PHPUnit, PHPStan, phpcs or Playwright depend on it | The suite that uses it runs and passes. | For `runtime-direct` and `runtime-transitive`, list every call site: @@ -158,26 +158,22 @@ the ledger, and any fix to our code. No leftover mutations, and no changes to `package.json` or the lockfiles beyond what Dependabot made. Then run the full suite for the affected package on the new version (the -same commands CI runs: `npm test` and `npx prettier --check packages/join-flow` -for join-flow; `./vendor/bin/phpunit` and `composer lint` for join-block). +same commands CI runs: `npm test`, `npx prettier --check packages/join-flow` +and `scripts/dev-server-smoke.sh` for join-flow; `./vendor/bin/phpunit` and +`composer lint` for join-block). ## Step 4: Non-runtime classes -- `build`: on the PR's lockfile, run `npm --prefix packages/join-flow run build` - and check it finishes without new errors or warnings compared with the base. - CI's E2E job runs the test build and the suite against it. -- `dev-server`: no CI job runs the dev server, so smoke-test it yourself on the - PR's lockfile: - ```bash - cd packages/join-flow && npx webpack serve --config ./webpack/dev.js & - # wait until it reports it is listening, then: - curl -fsS http://localhost:/ >/dev/null && echo "page ok" - kill %1 - ``` - If the PR touches the proxy chain (`http-proxy`, `http-proxy-middleware`, - `follow-redirects`), also fetch a path that goes through the proxy and check - you get the WordPress response, or a clean proxy error if WordPress isn't - running, rather than a crash. +- `build`: first confirm none of the package's code ends up in the bundle. + `scripts/bundled-packages.js ` builds the production bundle and + prints how many of the package's modules are in it. If the count isn't 0, + the package ships to users. It is `runtime-transitive`, + so go back to step 3. Otherwise compare `npm run build` on the base and PR + lockfiles and check for no new errors or warnings. CI's E2E job runs the + test build and the suite against it. +- `dev-server`: run `scripts/dev-server-smoke.sh` on the PR's lockfile. It + checks the page, the in-memory bundle (with hot reloading) and the stubbed + `/join/v1/*` endpoints. CI runs the same script in the Dev Server workflow. - `test-tooling`: the suite that uses it is the test. Run it. These classes have nothing new to commit except the ledger entry. @@ -248,8 +244,39 @@ Things to know about pushing to a Dependabot branch: - What the reviewer should check: that the tests assert on behaviour we actually depend on, and that nothing we use was missed. +Then request a review from the person who chose the dependency (see below) +and mention them in the comment, saying why they were picked. + Do not merge, approve, or enable auto-merge. +**Choosing the reviewer.** The reviewer is the person who added the +dependency to our `package.json` or `composer.json`: the package itself if it +is direct, or the top-level dependency that pulls it in if it is transitive +(the top of the `via` chain). They chose the library, so they know best what +we use it for. + +1. For each top-level dependency, find the commit that added it. History must + be complete first: `git fetch --unshallow origin` if + `git rev-parse --is-shallow-repository` says `true`. + ```bash + git log --reverse --format='%H %an <%ae>' -S '""' -- /package.json | head -1 + ``` + For Composer, search `packages/join-block/composer.json` instead. +2. Get their GitHub login from `mcp__github__get_commit` (`author.login`). +3. Use them if they have a login, aren't a bot, and are in + `mcp__github__list_repository_collaborators`. +4. Otherwise use the most recent person who changed that dependency's line, + skipping bots and anyone who fails step 3: + ```bash + git log --format='%H %an <%ae>' -G '^\s+"": ' -- /package.json + ``` +5. If nobody qualifies, request no reviewer and say so in the comment. + +A transitive package can have several top-level parents, for example +`fast-uri` comes in through both `babel-loader` and `webpack`. Request +everyone who qualifies, without duplicates. Request reviews with +`mcp__github__update_pull_request` (`reviewers`). + **Gear 2: merge on green.** All of the following must hold. If any of them fails, drop to gear 1. diff --git a/.github/workflows/dev-server.yml b/.github/workflows/dev-server.yml new file mode 100644 index 0000000..8066381 --- /dev/null +++ b/.github/workflows/dev-server.yml @@ -0,0 +1,27 @@ +name: Dev Server + +on: + push: + branches: + - master + pull_request: + workflow_call: + +jobs: + dev-server: + name: Dev server smoke test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: '20' + + - name: Install root deps + run: npm ci + + # Designers, engineers and Claude Code cloud sessions all use the dev + # server, so check it starts, compiles and answers the stubbed endpoints. + - name: Start dev server and check it + run: scripts/dev-server-smoke.sh diff --git a/packages/join-flow/package.json b/packages/join-flow/package.json index 5d2f2b3..65eef47 100644 --- a/packages/join-flow/package.json +++ b/packages/join-flow/package.json @@ -47,7 +47,8 @@ "start": "webpack serve --config './webpack/dev.js'", "build": "webpack --config './webpack/production.js'", "test": "jest", - "test:build": "REACT_APP_USE_TEST_DATA=true webpack --config './webpack/production.js'" + "test:build": "REACT_APP_USE_TEST_DATA=true webpack --config './webpack/production.js'", + "test:dev-server": "../../scripts/dev-server-smoke.sh" }, "proxy": "http://localhost:8080", "eslintConfig": { diff --git a/readme.md b/readme.md index f665d94..4435a26 100644 --- a/readme.md +++ b/readme.md @@ -496,6 +496,10 @@ The suite tests against one pinned WordPress release: the `wordpress:-p The PHP version in the tags (8.2) matches the plugin's minimum and the PHP version used in CI and `composer.json`; change them together. +### Dev server + +`.github/workflows/dev-server.yml` runs `scripts/dev-server-smoke.sh` on every PR, which starts the dev server and checks it serves the page, compiles the bundle and answers the stubbed REST endpoints. Run it locally with `npm --prefix packages/join-flow run test:dev-server` (port 3000 must be free). + ### Dependabot PRs The `/verify-dependabot` Claude Code skill (`.claude/skills/verify-dependabot/SKILL.md`) decides whether a Dependabot PR can merge. The first time a package is bumped, it adds the smallest test that pins down how our code uses the package, proves the test can fail, pushes it to the Dependabot branch and leaves the PR for a human to review. After that, minor and patch bumps of the same package auto-merge when CI is green. Major bumps always wait for a human. diff --git a/scripts/bundled-packages.js b/scripts/bundled-packages.js new file mode 100755 index 0000000..ec316bd --- /dev/null +++ b/scripts/bundled-packages.js @@ -0,0 +1,38 @@ +#!/usr/bin/env node +// Prints how many modules of each named package end up in the production +// join-flow bundle. 0 means the package is build tooling only and never ships +// to users. Usage: scripts/bundled-packages.js [...] +const { execFileSync } = require("child_process"); +const fs = require("fs"); +const os = require("os"); +const path = require("path"); + +const packages = process.argv.slice(2); +if (packages.length === 0) { + console.error("Usage: scripts/bundled-packages.js [...]"); + process.exit(2); +} + +const joinFlow = path.resolve(__dirname, "../packages/join-flow"); +const statsFile = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), "join-flow-stats-")), + "stats.json" +); +execFileSync( + path.join(joinFlow, "node_modules/.bin/webpack"), + ["--config", "./webpack/production.js", `--json=${statsFile}`], + { cwd: joinFlow, stdio: ["ignore", "ignore", "inherit"] } +); +const stats = JSON.parse(fs.readFileSync(statsFile, "utf8")); + +// Concatenated modules hold the modules they were built from in `modules`. +const flatten = (modules) => + modules.flatMap((m) => [m, ...flatten(m.modules || [])]); +const modules = flatten(stats.modules); + +for (const name of packages) { + const count = modules.filter((m) => + (m.name || "").includes(`node_modules/${name}/`) + ).length; + console.log(`${name}\t${count}`); +} diff --git a/scripts/dev-server-smoke.sh b/scripts/dev-server-smoke.sh new file mode 100755 index 0000000..eb09372 --- /dev/null +++ b/scripts/dev-server-smoke.sh @@ -0,0 +1,63 @@ +#!/bin/bash +# Starts the join-flow dev server and checks it serves the page, compiles the +# bundle and answers the stubbed REST endpoints. Designers, engineers and +# Claude Code cloud sessions all rely on the dev server, and nothing else in +# CI runs it. +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +PORT=3000 +BASE="http://localhost:$PORT" +LOG="$(mktemp)" + +if curl -fsS -o /dev/null "$BASE/" 2>/dev/null; then + echo "FAIL: something is already listening on port $PORT; stop it first" + exit 1 +fi + +cd "$ROOT/packages/join-flow" +# Run webpack's own binary rather than npx so $! is the server process. +node node_modules/.bin/webpack serve --config ./webpack/dev.js >"$LOG" 2>&1 & +SERVER_PID=$! +trap 'kill "$SERVER_PID" 2>/dev/null || true' EXIT + +fail() { + echo "FAIL: $1" + echo "--- dev server output ---" + cat "$LOG" + exit 1 +} + +# webpack-dev-server holds requests until the first compile finishes, so +# waiting for the page also waits for the bundle. +for _ in $(seq 1 120); do + if curl -fsS -o /dev/null "$BASE/" 2>/dev/null; then + break + fi + kill -0 "$SERVER_PID" 2>/dev/null || fail "dev server exited" + sleep 1 +done + +PAGE="$(curl -fsS "$BASE/")" || fail "page did not load" +grep -q 'src="bundle.js"' <<<"$PAGE" || + fail "page does not include bundle.js" +echo "ok: page" + +BUNDLE="$(curl -fsS "$BASE/bundle.js")" || fail "bundle.js did not load" +grep -q 'webpack/hot' <<<"$BUNDLE" || + fail "bundle.js did not compile with hot reloading" +echo "ok: bundle" + +# The server answers before webpack has written its summary, so wait for it. +for _ in $(seq 1 30); do + grep -qE "compiled (successfully|with)" "$LOG" && break + sleep 1 +done +grep -q "compiled successfully" "$LOG" || + fail "webpack reported errors or warnings" +echo "ok: compiled without errors" + +[ "$(curl -fsS -X POST -H 'Content-Type: application/json' -d '{"a":1}' \ + "$BASE/join/v1/join")" = "{}" ] || + fail "stubbed REST endpoint did not answer" +echo "ok: stubbed REST endpoint" From 2ef86f9ea037bbe160a8b6b0e7af8ef1f6ac7037 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 11:47:32 +0000 Subject: [PATCH 3/6] Fold lessons from the first verify-dependabot run into the skill Running it on #141 (fast-uri) showed: - One ledger file meant every first-time PR edited the same JSON, so parallel Dependabot PRs would conflict. The ledger is now one file per package under .github/dependency-coverage/. - Build-only packages still need the red/green proof, and the loader mutation has to hit a rule that runs (.scss; nothing imports .css). - Updating a Dependabot branch from master should merge, not rebase and force-push. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019QZFw9N8p8vhvtvA3LDZG1 --- .claude/skills/verify-dependabot/SKILL.md | 87 ++++++++++++++--------- .github/dependency-coverage.json | 3 - .github/dependency-coverage/README.md | 5 ++ readme.md | 2 +- 4 files changed, 59 insertions(+), 38 deletions(-) delete mode 100644 .github/dependency-coverage.json create mode 100644 .github/dependency-coverage/README.md diff --git a/.claude/skills/verify-dependabot/SKILL.md b/.claude/skills/verify-dependabot/SKILL.md index 8fcc588..030394b 100644 --- a/.claude/skills/verify-dependabot/SKILL.md +++ b/.claude/skills/verify-dependabot/SKILL.md @@ -20,11 +20,14 @@ Arguments: a PR number, or nothing to work through every open Dependabot PR ## The gears -The ledger is `.github/dependency-coverage.json`. An entry for a package on +The ledger is `.github/dependency-coverage/`, with one JSON file per package +at `//.json`, for example +`npm/packages/join-flow/fast-uri.json`. One file per package means Dependabot +PRs that are open at the same time never conflict over the ledger. An entry on the default branch means a human has reviewed and merged the tests listed in -it. The ledger only reaches the default branch through a merged PR, so "the -entry exists on `master`" is the approval record. Read it from `origin/master`, -never from the PR branch. +it. Entries only reach the default branch through a merged PR, so "the file +exists on `master`" is the approval record. Read it from `origin/master` +(`git show origin/master:`), never from the PR branch. | Situation | Gear | What happens | |---|---|---| @@ -51,7 +54,11 @@ Rules that decide the gear: with old and new versions, and the directory (`packages/join-flow`, `packages/join-e2e`, `packages/join-block`, or the root). 3. Work out the gear for each package from the table above. -4. Fetch the branch: `git fetch origin ` and check it out. +4. If the PR is behind `master`, bring it up to date first with + `mcp__github__update_pull_request_branch`. This merges `master` into the + branch, so CI runs the current checks (such as the Dev Server workflow) + and the helper scripts in `scripts/` are there to use. +5. Fetch the branch (`git fetch origin `) and check it out. ## Step 2: Find where it touches our code @@ -176,35 +183,48 @@ and `scripts/dev-server-smoke.sh` for join-flow; `./vendor/bin/phpunit` and `/join/v1/*` endpoints. CI runs the same script in the Dev Server workflow. - `test-tooling`: the suite that uses it is the test. Run it. -These classes have nothing new to commit except the ledger entry. +These classes have nothing new to commit except the ledger entry, but they +still need the red/green proof from step 3. It shows the build or the dev +server really does exercise the package: + +1. Green on the old version, green on the new version (as in step 3). +2. Red on our side: break something in our use of the tool. For a package + used for options validation (`schema-utils`, `ajv`), add an unknown option + to a loader in `webpack/common.js` that actually runs. Use the `.scss` + rule: nothing imports `.css`, so that rule never runs. +3. Red on the library side: edit the package in `node_modules` so a function + on the path returns something different, and check the build or dev server + fails. If it still passes, the tool never calls that code. Say so on the + PR, because then our checks can't catch a bad upgrade of this package. ## Step 5: Update the ledger -Add or update the package's entry in `.github/dependency-coverage.json`: +Create or update the package's file in `.github/dependency-coverage/`, for +example `npm/packages/join-flow/google-libphonenumber.json`: ```json { - "packages": { - "npm:packages/join-flow:google-libphonenumber": { - "class": "runtime-direct", - "via": [], - "usage": [ - "packages/join-flow/src/schema.ts" - ], - "tests": [ - "packages/join-flow/src/schema.test.ts › dependency: google-libphonenumber" - ], - "reviewedVersion": "3.2.40", - "reviewedIn": "#142" - } - } + "class": "runtime-direct", + "via": [], + "usage": [ + "packages/join-flow/src/schema.ts" + ], + "tests": [ + "packages/join-flow/src/schema.test.ts › dependency: google-libphonenumber" + ], + "reviewedVersion": "3.2.40", + "reviewedIn": "#142" } ``` -- The key is `::`. The same package in - two directories gets two entries, because the usage is different. -- `via` is the dependency chain from `npm why`/`composer why`, top level first. - It is empty for direct dependencies. +- The path is `//.json`, with the ecosystem + `npm` or `composer`. A scoped or vendored name becomes a subdirectory + (`npm/packages/join-flow/@sentry/react.json`, + `composer/packages/join-block/guzzlehttp/guzzle.json`). The same package in + two directories gets two files, because the usage is different. +- `via` is the dependency chain from `npm why`/`composer why`, top level + first. When several top-level dependencies pull it in, list them all, + comma-separated, as the first item. It is empty for direct dependencies. - `usage` lists the files of ours that use it (from step 2). It is empty for non-runtime classes. - `tests` lists what verifies it. For non-runtime classes, name the CI @@ -212,8 +232,6 @@ Add or update the package's entry in `.github/dependency-coverage.json`: - `reviewedVersion` and `reviewedIn` are the new version and this PR. Only gear 1 and 3 PRs, which a human reviews, write them. -Keep keys sorted so diffs stay readable. - ## Step 6: Push to the Dependabot branch Commit the tests, any fix and the ledger in one commit on the PR's head @@ -222,10 +240,11 @@ branch, with a message saying what the tests pin down. Then Things to know about pushing to a Dependabot branch: -- After anyone else commits, Dependabot stops rebasing the PR. If it later - needs a rebase, rebase it yourself (`git rebase origin/master`, then - `git push --force-with-lease`). Do not comment `@dependabot recreate`, - because that throws away our commit. +- After anyone else commits, Dependabot stops rebasing the PR. When it falls + behind `master` or conflicts, update it with + `mcp__github__update_pull_request_branch`, or merge `origin/master` into it + and push. Never rebase or force-push a Dependabot branch, and never comment + `@dependabot recreate`: both throw away commits. - Don't touch `package.json` or the lockfiles. Dependabot owns those. ## Step 7: Hand off or merge @@ -280,7 +299,7 @@ everyone who qualifies, without duplicates. Request reviews with **Gear 2: merge on green.** All of the following must hold. If any of them fails, drop to gear 1. -1. The ledger entry on `origin/master` matches this package and directory. +1. The package's ledger file exists on `origin/master`. 2. The usage grep from step 2 finds no files beyond the entry's `usage`. If it finds new ones, the package is used in a new way that has no test yet: cover it (step 3), which makes this PR gear 1. @@ -301,6 +320,6 @@ that approved it. List open PRs by `dependabot[bot]` and handle them one at a time, oldest first. Several PRs often bump packages in the same lockfile. After one -merges, the others may need a rebase (see step 6) before their CI means -anything. Finish with a table: PR, packages, class, gear, outcome (handed to +merges, the others may need updating from `master` (see step 6) before +their CI means anything. Finish with a table: PR, packages, class, gear, outcome (handed to human / auto-merge enabled / failed, and why). diff --git a/.github/dependency-coverage.json b/.github/dependency-coverage.json deleted file mode 100644 index 5d5e1bf..0000000 --- a/.github/dependency-coverage.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "packages": {} -} diff --git a/.github/dependency-coverage/README.md b/.github/dependency-coverage/README.md new file mode 100644 index 0000000..80d2c93 --- /dev/null +++ b/.github/dependency-coverage/README.md @@ -0,0 +1,5 @@ +# Dependency coverage ledger + +One JSON file per dependency, at `//.json`, records the tests that verify how our code uses it. The `/verify-dependabot` skill (`.claude/skills/verify-dependabot/SKILL.md`) writes these files when a package is first bumped, and a human reviews them in that Dependabot PR. + +A file on `master` means its tests have been reviewed, so later minor and patch bumps of that package can auto-merge when CI is green. Adding or editing a file outside a reviewed PR bypasses that review. diff --git a/readme.md b/readme.md index 4435a26..ac30d51 100644 --- a/readme.md +++ b/readme.md @@ -504,7 +504,7 @@ The PHP version in the tags (8.2) matches the plugin's minimum and the PHP versi The `/verify-dependabot` Claude Code skill (`.claude/skills/verify-dependabot/SKILL.md`) decides whether a Dependabot PR can merge. The first time a package is bumped, it adds the smallest test that pins down how our code uses the package, proves the test can fail, pushes it to the Dependabot branch and leaves the PR for a human to review. After that, minor and patch bumps of the same package auto-merge when CI is green. Major bumps always wait for a human. -`.github/dependency-coverage.json` records which packages have reviewed tests. An entry only reaches `master` through a reviewed, merged PR, so editing it by hand bypasses that review. +`.github/dependency-coverage/` records which packages have reviewed tests, one file per package. A file only reaches `master` through a reviewed, merged PR, so adding or editing one by hand bypasses that review. ### Claude Code cloud sessions From 058f952db0abc98b5b568e234e713110381bdcdb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 11:58:30 +0000 Subject: [PATCH 4/6] Turn off webpack's colours in the dev server smoke test On GitHub Actions webpack colours its output, putting escape codes between "compiled" and "successfully", so the check never matched and the Dev Server job failed after its 30 second wait. With --no-color the log is plain text everywhere. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019QZFw9N8p8vhvtvA3LDZG1 --- scripts/dev-server-smoke.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/dev-server-smoke.sh b/scripts/dev-server-smoke.sh index eb09372..2ddc7e3 100755 --- a/scripts/dev-server-smoke.sh +++ b/scripts/dev-server-smoke.sh @@ -17,7 +17,8 @@ fi cd "$ROOT/packages/join-flow" # Run webpack's own binary rather than npx so $! is the server process. -node node_modules/.bin/webpack serve --config ./webpack/dev.js >"$LOG" 2>&1 & +# --no-color keeps the log greppable: webpack colours its output on CI. +node node_modules/.bin/webpack serve --config ./webpack/dev.js --no-color >"$LOG" 2>&1 & SERVER_PID=$! trap 'kill "$SERVER_PID" 2>/dev/null || true' EXIT From 05370cf8fc48e64ee5596c51b1713662e2200881 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 13:54:38 +0000 Subject: [PATCH 5/6] Have verify-dependabot fix what an upgrade breaks and get it reviewed Previously a failing test was reported to the human reviewer as it was. Now the skill fixes our code until CI is green (new and existing tests, build, lint, E2E), then has a fresh review agent run the code-review skill on our commits, fixes what it finds and asks a new agent to review again. Only when CI is green and a review agent is satisfied does it ask a person to review. After three rounds it stops and asks the reviewer for help instead. A status comment on the PR records the round, so the daily run picks up where it stopped. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019QZFw9N8p8vhvtvA3LDZG1 --- .claude/skills/verify-dependabot/SKILL.md | 130 ++++++++++++++++++---- readme.md | 2 +- 2 files changed, 109 insertions(+), 23 deletions(-) diff --git a/.claude/skills/verify-dependabot/SKILL.md b/.claude/skills/verify-dependabot/SKILL.md index 030394b..b893670 100644 --- a/.claude/skills/verify-dependabot/SKILL.md +++ b/.claude/skills/verify-dependabot/SKILL.md @@ -1,6 +1,6 @@ --- name: verify-dependabot -description: Decide whether a Dependabot pull request can be merged. Finds where the bumped package touches our code, adds the smallest test that would catch a change in that integration (proved with red/green), pushes it to the Dependabot branch, and either hands the PR to a human or enables auto-merge, depending on whether the package has been verified before and how big the version jump is. Use when asked to check, verify, triage or merge Dependabot PRs, or with a PR number of a Dependabot PR. +description: Get a Dependabot pull request to merge-ready. Finds where the bumped package touches our code, adds the smallest test that would catch a change in that integration (proved with red/green), fixes whatever the upgrade breaks until CI is green, has fresh review agents review our commits until they are satisfied (at most three rounds), then either hands the PR to a human or enables auto-merge, depending on whether the package has been verified before and how big the version jump is. Use when asked to check, verify, triage, fix or merge Dependabot PRs, or with a PR number of a Dependabot PR. --- # Verify a Dependabot PR @@ -15,6 +15,12 @@ We are not testing the library. It has its own tests. We are testing the seam between the library and our code: the inputs we pass it and the outputs we rely on. +Broken things get fixed, not reported. If the upgrade breaks a new test, an +existing test, the build, lint or E2E, fix our code until CI is green. A +person should only ever be handed a PR that works and that review agents have +already been through (step 7), or one that three rounds of fixing and review +could not get there, with a clear ask for help. + Arguments: a PR number, or nothing to work through every open Dependabot PR (oldest first). @@ -31,17 +37,19 @@ exists on `master`" is the approval record. Read it from `origin/master` | Situation | Gear | What happens | |---|---|---| -| No ledger entry on `master` | 1 | Write tests, push them, comment, **wait for a human**. | +| No ledger entry on `master` | 1 | Write tests, fix what breaks, get CI green and review agents satisfied, **then hand to a human**. | | Ledger entry, minor or patch bump | 2 | Re-check usage, wait for CI, **enable auto-merge**. | -| Major bump (any ledger state) | 3 | Do the gear 1 or 2 checks, read the changelog, **wait for a human**. | +| Major bump (any ledger state) | 3 | As gear 1 or 2, plus read the changelog, **then hand to a human**. | Rules that decide the gear: - `0.x` versions: treat a minor bump (`0.10` to `0.11`) as major, since semver allows breaking changes there. - A PR that bumps several packages takes the highest gear of any of them. -- If anything in gear 2 needs a code change (a new test, a fix to our code), - the PR drops to gear 1. +- If anything in gear 2 needs a code change (a new test, a fix to our code, + because CI is red), the PR drops to gear 1. +- Red CI on a Dependabot PR is always work to do, whatever the gear and + whether or not we have pushed anything to it yet. Go to step 7. - Anything you are unsure about goes to a human. Auto-merge is the reward for certainty, not the default. @@ -156,13 +164,12 @@ order and keep the output of each run for the PR comment. Run the test again. - Passes: good. - Fails: this is what the skill exists to catch. Read the package's - changelog for the change. If our code needs a small, clear fix, make - it and note it for the human reviewer. Otherwise leave the code alone and - report the failure on the PR. Either way the PR goes to a human. + changelog and find out what changed, then fix our code so it works with + the new version (step 7 says what counts as a fix). The test stays as + it is unless our expectation, not our code, was what was out of date. After the proof, `git status` must show only the new or changed test files, -the ledger, and any fix to our code. No leftover mutations, and no changes to -`package.json` or the lockfiles beyond what Dependabot made. +the ledger, and any fix. No leftover mutations. Then run the full suite for the affected package on the new version (the same commands CI runs: `npm test`, `npx prettier --check packages/join-flow` @@ -245,11 +252,81 @@ Things to know about pushing to a Dependabot branch: `mcp__github__update_pull_request_branch`, or merge `origin/master` into it and push. Never rebase or force-push a Dependabot branch, and never comment `@dependabot recreate`: both throw away commits. -- Don't touch `package.json` or the lockfiles. Dependabot owns those. - -## Step 7: Hand off or merge - -**Gear 1 and 3: hand off to a human.** Post one comment on the PR with: +- Never change the version Dependabot bumped. If the fix needs another + dependency to move too (a companion or peer package), change it with + `npm install @` or `composer require`, so the lockfile is + regenerated properly, and explain why in the commit and the hand-off. + +## Step 7: Get it green and reviewed + +Every PR we have pushed commits to (tests, fixes, even a ledger entry alone) +goes through this before a person sees it. Tests are code, and code gets +reviewed. So does every Dependabot PR whose CI is red, even before we have +added anything: an existing test, lint, the build or E2E failing on the new +version is a break to fix. + +Keep one status comment on the PR, with `` as its +first line, and edit it (`mcp__github__update_issue_comment`) rather than +posting new ones. It holds the gear, the current round, and what is +outstanding, so a later run can pick up where this one stopped. + +**A round** starts when you push and ends when a review agent has reviewed +that push, or when CI on it comes back red. + +1. **Get CI green.** For each failing check, read its log + (`get_check_runs`, `get_job_logs`), reproduce the failure locally, find + the root cause and fix it, as you would for any broken build. Fixing + means making our code work with the new version: adapting a call to a + changed API, updating a type, handling a new return value, updating + config. Run the affected suites locally, then push. Then subscribe to the + PR (`mcp__claude-code-remote__subscribe_pr_activity`) and end your turn: + the CI result wakes you, so don't poll. Never: + - skip, disable, delete or weaken a test or assertion to get green; + - pin the package back to its old version, which undoes the PR; + - push an empty commit, or close and reopen the PR, to re-run CI. + + Changing a test's expectation is fine only when the library's new + behaviour is correct and our expectation was what was out of date. Say + so explicitly in the hand-off. If the fix would change something members + or organisers notice (the form, an email, a payment, CRM data), stop and + escalate (below), because that is a product decision. +2. **Review.** Once CI is green on the head commit, start a review agent with + fresh context. Use `Agent` with `subagent_type: "general-purpose"` and + `run_in_background: false`, never `fork`, which would inherit your + reasoning. Give it the PR, not your opinion of it: + + > Run the `code-review` skill (Skill tool, args `high `) on + > commonknowledge/join#. It is a Dependabot dependency bump. + > The commits after Dependabot's add tests, fixes and a ledger entry under + > `.github/dependency-coverage/` that verify how our code uses the bumped + > package. Review all of it. Do not post anything on GitHub. Reply with + > each finding (file:line, what is wrong, how serious) and end with exactly + > one line: `VERDICT: ready` if nothing blocking remains, or + > `VERDICT: changes needed`. + +3. **Act on the findings.** Fix every finding that is correct and in + proportion. For each one you decide not to fix, note a one-line reason for + the hand-off. If you pushed anything, that push starts the next round, + which gets a **new** review agent. Never resume an earlier one with + `SendMessage`: every review needs a fresh read. + +The PR is ready for a person when CI is green on the head and the latest +review agent said `VERDICT: ready`. Go to step 8. + +**At most three rounds.** If after the third round CI is still red or the +latest review agent still says `VERDICT: changes needed`, stop pushing and +escalate. Also escalate straight away, at any round, when the fix is a product +decision or the new version can't work with our code without a larger change +than a dependency bump should carry. To escalate, post a new comment (so the +mention notifies) that mentions the reviewer chosen as in step 8 and asks for +their help. Say what is still failing (check names and log excerpts), the +open review findings, what you tried and what you think the options are. +Request their review too. Update the status comment to say it was escalated. + +## Step 8: Hand off or merge + +**Gear 1 and 3: hand off to a human.** Post one new comment on the PR (the +status comment's edits don't notify anyone) with: - The packages, versions, class and gear, and why it needs a human ("first time this package has been verified", or "major version"). @@ -259,7 +336,10 @@ Things to know about pushing to a Dependabot branch: mutation you made and a short excerpt of each failure. - For major bumps: the breaking changes in the changelog and whether any of them affect our usage. -- Any change to our code, and why it was needed. +- Any change to our code, and why it was needed. Call out any test whose + expectation changed, and why the new behaviour is right. +- The review rounds from step 7: how many, what the review agents found, what + was fixed, and why anything they raised was left. - What the reviewer should check: that the tests assert on behaviour we actually depend on, and that nothing we use was missed. @@ -297,7 +377,8 @@ everyone who qualifies, without duplicates. Request reviews with `mcp__github__update_pull_request` (`reviewers`). **Gear 2: merge on green.** All of the following must hold. If any of them -fails, drop to gear 1. +fails, drop to gear 1. If a check is red or a ledgered test fails, fix it in +step 7 rather than handing it over as it is. 1. The package's ledger file exists on `origin/master`. 2. The usage grep from step 2 finds no files beyond the entry's `usage`. If @@ -318,8 +399,13 @@ that approved it. ## When working through every open PR -List open PRs by `dependabot[bot]` and handle them one at a time, oldest -first. Several PRs often bump packages in the same lockfile. After one -merges, the others may need updating from `master` (see step 6) before -their CI means anything. Finish with a table: PR, packages, class, gear, outcome (handed to -human / auto-merge enabled / failed, and why). +List open PRs by `dependabot[bot]` and take them oldest first. For each one, +read its status comment first and carry on from the round it records, +instead of starting again. While one PR waits for CI, move on to the next +and come back when the CI result arrives. Several PRs often bump packages in +the same lockfile. After one merges, the others may need updating from +`master` (see step 6) before their CI means anything. + +Finish with a table: PR, packages, class, gear, rounds used, and outcome +(handed to a person / auto-merge enabled / waiting on CI / escalated, and +why). diff --git a/readme.md b/readme.md index ac30d51..91097c4 100644 --- a/readme.md +++ b/readme.md @@ -502,7 +502,7 @@ The PHP version in the tags (8.2) matches the plugin's minimum and the PHP versi ### Dependabot PRs -The `/verify-dependabot` Claude Code skill (`.claude/skills/verify-dependabot/SKILL.md`) decides whether a Dependabot PR can merge. The first time a package is bumped, it adds the smallest test that pins down how our code uses the package, proves the test can fail, pushes it to the Dependabot branch and leaves the PR for a human to review. After that, minor and patch bumps of the same package auto-merge when CI is green. Major bumps always wait for a human. +The `/verify-dependabot` Claude Code skill (`.claude/skills/verify-dependabot/SKILL.md`) decides whether a Dependabot PR can merge. The first time a package is bumped, it adds the smallest test that pins down how our code uses the package, proves the test can fail, and pushes it to the Dependabot branch. If the upgrade breaks anything, new tests or existing ones, it fixes our code until CI is green. Fresh review agents then review its commits, and it fixes what they find, for up to three rounds. Only then does it ask the person who added the dependency to review. If three rounds aren't enough, it asks that person for help instead. After that first review, minor and patch bumps of the same package auto-merge when CI is green. Major bumps always wait for a human. `.github/dependency-coverage/` records which packages have reviewed tests, one file per package. A file only reaches `master` through a reviewed, merged PR, so adding or editing one by hand bypasses that review. From eee3d40312237057c1a2a32f6179992ead9cbb2c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 13:59:20 +0000 Subject: [PATCH 6/6] Post verify-dependabot's review findings on the PR The review agent now posts its findings as inline review comments, and the skill answers each thread: fixed ones get the commit and are resolved, declined ones get the reason and stay open for the person reviewing. That leaves a visible record of what was found and done, instead of a summary only. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019QZFw9N8p8vhvtvA3LDZG1 --- .claude/skills/verify-dependabot/SKILL.md | 33 ++++++++++++++--------- readme.md | 2 +- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/.claude/skills/verify-dependabot/SKILL.md b/.claude/skills/verify-dependabot/SKILL.md index b893670..3bbe96a 100644 --- a/.claude/skills/verify-dependabot/SKILL.md +++ b/.claude/skills/verify-dependabot/SKILL.md @@ -295,18 +295,26 @@ that push, or when CI on it comes back red. `run_in_background: false`, never `fork`, which would inherit your reasoning. Give it the PR, not your opinion of it: - > Run the `code-review` skill (Skill tool, args `high `) on - > commonknowledge/join#. It is a Dependabot dependency bump. - > The commits after Dependabot's add tests, fixes and a ledger entry under - > `.github/dependency-coverage/` that verify how our code uses the bumped - > package. Review all of it. Do not post anything on GitHub. Reply with - > each finding (file:line, what is wrong, how serious) and end with exactly - > one line: `VERDICT: ready` if nothing blocking remains, or - > `VERDICT: changes needed`. + > Run the `code-review` skill (Skill tool, args + > `high --comment `) on commonknowledge/join#, so + > your findings are posted as inline review comments on the PR. It is a + > Dependabot dependency bump. The commits after Dependabot's add tests, + > fixes and a ledger entry under `.github/dependency-coverage/` that + > verify how our code uses the bumped package. Review all of it. Then + > reply to me with each finding (file:line, what is wrong, how serious) + > and end with exactly one line: `VERDICT: ready` if nothing blocking + > remains, or `VERDICT: changes needed`. 3. **Act on the findings.** Fix every finding that is correct and in - proportion. For each one you decide not to fix, note a one-line reason for - the hand-off. If you pushed anything, that push starts the next round, + proportion, then answer every thread the review agent opened, so the + person reviewing later can follow what happened: + - Fixed: reply with the commit that fixes it, and resolve the thread + (`mcp__github__add_reply_to_pull_request_comment`, + `mcp__github__resolve_review_thread`). + - Not fixed: reply with the reason and leave the thread open, so the + person reviewing can see it and decide. + + If you pushed anything, that push starts the next round, which gets a **new** review agent. Never resume an earlier one with `SendMessage`: every review needs a fresh read. @@ -338,8 +346,9 @@ status comment's edits don't notify anyone) with: them affect our usage. - Any change to our code, and why it was needed. Call out any test whose expectation changed, and why the new behaviour is right. -- The review rounds from step 7: how many, what the review agents found, what - was fixed, and why anything they raised was left. +- The review rounds from step 7: how many, and a short summary of what was + fixed. The detail is in the review threads. Point the reviewer at any + thread left open, which is a finding we chose not to fix. - What the reviewer should check: that the tests assert on behaviour we actually depend on, and that nothing we use was missed. diff --git a/readme.md b/readme.md index 91097c4..cad66b8 100644 --- a/readme.md +++ b/readme.md @@ -502,7 +502,7 @@ The PHP version in the tags (8.2) matches the plugin's minimum and the PHP versi ### Dependabot PRs -The `/verify-dependabot` Claude Code skill (`.claude/skills/verify-dependabot/SKILL.md`) decides whether a Dependabot PR can merge. The first time a package is bumped, it adds the smallest test that pins down how our code uses the package, proves the test can fail, and pushes it to the Dependabot branch. If the upgrade breaks anything, new tests or existing ones, it fixes our code until CI is green. Fresh review agents then review its commits, and it fixes what they find, for up to three rounds. Only then does it ask the person who added the dependency to review. If three rounds aren't enough, it asks that person for help instead. After that first review, minor and patch bumps of the same package auto-merge when CI is green. Major bumps always wait for a human. +The `/verify-dependabot` Claude Code skill (`.claude/skills/verify-dependabot/SKILL.md`) decides whether a Dependabot PR can merge. The first time a package is bumped, it adds the smallest test that pins down how our code uses the package, proves the test can fail, and pushes it to the Dependabot branch. If the upgrade breaks anything, new tests or existing ones, it fixes our code until CI is green. Fresh review agents then review its commits as review comments on the PR, and it fixes what they find and answers each thread, for up to three rounds. Only then does it ask the person who added the dependency to review. If three rounds aren't enough, it asks that person for help instead. After that first review, minor and patch bumps of the same package auto-merge when CI is green. Major bumps always wait for a human. `.github/dependency-coverage/` records which packages have reviewed tests, one file per package. A file only reaches `master` through a reviewed, merged PR, so adding or editing one by hand bypasses that review.