diff --git a/.claude/guardrails.md b/.claude/guardrails.md index dd8c19e6b8a..dc49ef62166 100644 --- a/.claude/guardrails.md +++ b/.claude/guardrails.md @@ -164,6 +164,43 @@ instead of trusting a "done" in chat. A runbook that shipped this way belongs in the owning README, so the next round starts from a reviewed text rather than a fresh improvisation. +## After every merge, check that the site renders + +Owner rule, 2026-10-22, after an outage nothing caught. + +**What happened.** A Dependabot batch merged nine PRs in an afternoon, one +lock file at a time, each with green checks. One of them, vite 8.0.16 to +8.1.6 (#12225), deployed at 17:55 UTC. Rolldown 1.1.5 put its CJS-interop +runtime helper into the app's `theme` chunk, which imports `mui`, while +`mui` imports the helper back. The chunk cycle left the helper undefined, the +bundle threw `TypeError: t is not a function` at module load, and every page +on anyplot.ai rendered blank. Every PR check passed, including "Build app +image and smoke the origin gate", which proves nginx serves the shell but +never runs the JavaScript. The deploy was green too. The owner noticed about +45 minutes later; a cloud session found the cause and shipped vite 8.3.5 +(#12235), which emits the helper as its own `rolldown-runtime` chunk. + +**The check.** `scripts/site-smoke.mjs` loads `/`, `/plots` and +`/scatter-basic` in headless Chromium and fails on a status other than 200, +an uncaught page error, or a `#root` with next to no text; it also checks +`api.anyplot.ai/health`. It takes about two seconds. The recipe is in +`/open-pr` §4. Run against a build of the broken commit, it fails exactly as +the outage did; against a healthy one it passes, with the CORS error of a +foreign origin reported as a warning only. + +**Two layers.** The `app-image` job of `ci-image.yml` runs the same script +against the built container before the merge, so a bump that breaks the boot +fails its PR. It runs when the container's inputs change (`package.json`, +`yarn.lock`, `vite.config.ts` and the like), not for `app/src` edits, and it +cannot see what only production has: the CDN, the origin gate armed, the real +data. The post-merge run covers those, and every merge that never builds the +container. + +**Why after every merge.** A merge that touches no `app/` file can still +break the site through the API, a Cloudflare rule, or a data change, and the +run costs seconds. A `FAIL` stops further merges until the site renders, +because each extra merge widens the set of suspects and the revert. + ## Modify repo files only with the Edit/Write tools Appending with `>>` counts — appending at the end of a file is exactly the diff --git a/.claude/skills/open-pr/SKILL.md b/.claude/skills/open-pr/SKILL.md index 6bd88e3c944..faf33289648 100644 --- a/.claude/skills/open-pr/SKILL.md +++ b/.claude/skills/open-pr/SKILL.md @@ -1,6 +1,6 @@ --- name: open-pr -description: Take a finished change from diff to an open, green, review-clean PR — run the matching verify skills and local CI gates first, then commit, push and open the PR via /pull_request, watch the pipeline AND the Copilot review, fix sensible findings, resolve the review threads, and watch any Cloud Build deploy after merge. Detects local vs. cloud GitHub tooling (gh vs. GitHub MCP). Never merges unless the user explicitly authorizes it in this session. Use when asked to open or create a PR, ship a change, or finish up a change. +description: Take a finished change from diff to an open, green, review-clean PR — run the matching verify skills and local CI gates first, then commit, push and open the PR via /pull_request, watch the pipeline AND the Copilot review, fix sensible findings, resolve the review threads, and after a merge watch the Cloud Build deploys and check in a browser that the live site still renders (scripts/site-smoke.mjs). Detects local vs. cloud GitHub tooling (gh vs. GitHub MCP). Never merges unless the user explicitly authorizes it in this session. Use when asked to open or create a PR, ship a change, or finish up a change. --- # Open a PR (ship a change) @@ -316,6 +316,42 @@ while both deploys are already done (2026-08-28, #10808 follow-through: a 20-minute poll on the global list never saw the builds). Match the `SHORT_SHA` column against the merge commit before trusting a row. +**Then check that the site renders — after every merge, whatever it +touched.** A green PR and a green deploy do not prove the bundle boots: +on 2026-10-22 vite 8.1.6 (#12225) passed every check and both, and every +page on anyplot.ai stayed blank for about 45 minutes until #12235. Once +the merge's deploys have finished (the watcher above shows them +`SUCCESS`; right after the merge when it triggered none), run: + +```bash +SCRATCH= +[ -d "$SCRATCH/node_modules/playwright-core" ] || \ + (cd "$SCRATCH" && PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD=1 npm i --silent playwright-core@1.64.1) +NODE_PATH="$SCRATCH/node_modules" node scripts/site-smoke.mjs +``` + +It loads `/`, `/plots` and `/scatter-basic` in headless Chromium, +checks `api.anyplot.ai/health`, and prints one `OK` or `FAIL` line per +check, with the entry bundle (`bundle=index-*.js`) that answered; it +never downloads a browser. A failed route counts too: the app's error +boundary ("Something went wrong") keeps the masthead and footer, so +the script looks for its text, not only for an empty page. + +Without `gcloud` (a cloud session) the deploy cannot be watched, and +elapsed time proves nothing: Cloud Build may still be waiting for an +earlier deploy. For a merge that touches `app/`, run the script once +before the merge and wait until `bundle=` changes; until it does, or +when you cannot tell, report the post-merge check as unconfirmed +rather than passed. + +- **Exit 1 is an incident.** Tell the owner at once with the failing + lines and every merge since the last passing run, propose reverting + the suspect merge (a revert PR is the fastest rollback), and land no + further merge until the site renders again. +- **Exit 2** means no browser was found, so nothing was checked: say + so, then load `/` and a spec page with the Playwright MCP and read + its console errors instead. + ## Gotchas - **`isOutdated` ≠ `isResolved`.** A fix-push can outdate a Copilot diff --git a/.github/workflows/ci-image.yml b/.github/workflows/ci-image.yml index 19adcd4fcfd..9023a283c7b 100644 --- a/.github/workflows/ci-image.yml +++ b/.github/workflows/ci-image.yml @@ -454,6 +454,28 @@ jobs: want "armed, no secret: empty header too" "$(code -H 'X-Origin-Secret;' localhost:8083/)" 403 want "armed, no secret: /_health exempt" "$(code localhost:8083/_health)" 200 + # Served is not booted. On 2026-10-22 a Vite minor bump (#12225) passed + # every step above and blanked every page in production: Rolldown put its + # runtime helper into a chunk cycle, and the bundle threw `TypeError: t is + # not a function` at module load (fixed by #12235). The runner's Google + # Chrome loads the gate-off container's home page and fails on an + # uncaught page error or an empty #root; scripts/site-smoke.mjs is the + # same check the post-merge rule runs against production. No API check: + # the page's data calls go to production, whose CORS does not know this + # origin, so only the boot is asserted. + - name: Browser boot (the bundle renders) + if: steps.check.outputs.should_build == 'true' + env: + SITE: http://localhost:8081 + API: none + CHROME_PATH: /usr/bin/google-chrome + MIN_TEXT: "50" + PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD: "1" + run: | + set -euo pipefail + npm install --no-save --no-audit --no-fund --silent --prefix "$RUNNER_TEMP/smoke" playwright-core@1.64.1 + NODE_PATH="$RUNNER_TEMP/smoke/node_modules" node scripts/site-smoke.mjs / + - name: Container logs on failure if: failure() && steps.check.outputs.should_build == 'true' run: | diff --git a/CLAUDE.md b/CLAUDE.md index 767cbbe9b8b..d06a66fae8b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,6 +21,7 @@ A companion guide `.github/copilot-instructions.md` carries the shared rules for - **External-system writes need explicit, named authorization** - Merging or closing PRs/issues this session did not create, bulk merges, and label changes on others' PRs are blocked by the permission classifier unless the user named that action; a generic "ok, sounds good" authorizes nothing. The same discipline covers every prod-touching action in interactive sessions — Cloud SQL writes/DDL, GCS production-folder changes, Secret Manager access, Cloud Build config: name the exact action, resource, and id, and ask before acting (the automated `spec-*`/`impl-*` workflows write these by design and are exempt). For any change to `.claude/settings*.json`, use the built-in `/update-config` skill (a Claude Code harness skill, not a repo command) immediately — direct writes are blocked as self-modification and retrying variants just burns round trips. - **Never echo secret values into the transcript** - Verify secrets by exit code or metadata, never by printing them; never create a Secret Manager version via `echo` (the trailing newline corrupts the value). - **Snapshot before destructive prod operations** - Before anything that can overwrite or delete shared prod DB data or GCS production objects (bulk UPDATE/DELETE, a data-rewriting migration, bulk GCS overwrite/delete): take a timestamped backup first (`pg_dump` to a new directory outside the working tree, `gsutil cp` to a backup prefix), sanity-check it (row/object counts — a silent empty snapshot is worse than none because it looks like safety), and never write into, delete, or rename an existing snapshot. +- **After every merge, check that the site renders** - Yours, Dependabot's, or one the owner asked for: once the merge's deploys finish, run `scripts/site-smoke.mjs` (recipe in `/open-pr` §4); a `FAIL` is an incident, reported at once with the suspect merge and a revert proposal, and nothing else merges until the site renders. A green PR plus a green deploy blanked anyplot.ai for 45 minutes on 2026-10-22 (#12225). - **Long-running work reports progress proactively** - During pipeline babysitting, bulk operations, or extended planning, post a one-line status roughly every 10 minutes and flag a suspected stall immediately with evidence; never let the user be the one to ask "still running?". Before starting a multi-item queue, confirm its extent and stop-point with the user. - **Manual user tasks go to Todoist** - Whenever a session identifies a step only the user can or should do (adding `approved` labels, merge authorization, console/billing/DNS actions, secret rotations), create a task in the user's Todoist project **Anyplot** (via the Todoist MCP tools) naming the concrete action and a context link (PR/issue/run URL) — instead of leaving it buried in a chat reply. Interactive sessions only; GitHub Actions workflows have no MCP access. - **Fix small build/test blockers directly, even when out of scope** - If a typecheck error, failing test, lint failure, or other small pipeline blocker shows up while working on something unrelated (incl. things the current PR was not meant to touch), fix it in the same PR or a tiny follow-up — never leave it parked under "out of scope". A latent `tsc` error that doesn't surface locally will silently break the next Cloud Build, which deploys nothing new and leaves production stale even though every PR check looks green (this is exactly how PR #6961's frontend fixes never reached anyplot.ai — the unrelated `prism/r` TS7016 from #6944 was deferred, then blocked the next `yarn build`). The bar: if the fix is < ~20 lines and obviously correct, just do it; if it would expand scope meaningfully, ask first rather than deferring silently. diff --git a/agentic/commands/dependabot.md b/agentic/commands/dependabot.md index 41f3bf85f47..b377b8d7c07 100644 --- a/agentic/commands/dependabot.md +++ b/agentic/commands/dependabot.md @@ -11,6 +11,15 @@ known GitHub quirks. Every gotcha below cost real session round-trips. - Auto-merge is the mechanism of choice: `gh pr merge --auto --squash` — the PR merges itself when checks pass; no polling needed. - Grouped bumps (npm-minor, python-minor, actions) are one PR each — treat like any other. +- **One lock-file PR at a time, and every `app/` bump checked in the browser.** Of the PRs that + touch the same lock file (`uv.lock`, `app/yarn.lock`), only one has auto-merge at a time: several + squash-merged within a minute have duplicated package blocks in `uv.lock`. After an `app/` PR + merges, wait for its `deploy-app` build and run the site smoke (`/open-pr` skill §4, + `scripts/site-smoke.mjs`) before the next `app/` PR gets auto-merge. On 2026-10-22 vite 8.1.6 + (#12225) passed every check, blanked every page, and stayed unnoticed for about 45 minutes + inside a batch. Comment `@dependabot rebase` on the next PR of a chain before you enable its + auto-merge, and expect Dependabot to close and regroup a PR now and then ("updatable in another + way"); list the PRs again when it does. - Per the changelog contract, individual Dependabot bumps get NO `CHANGELOG.md` entries — they are aggregated into one **Dependencies:** bullet at release time (`/release` step 2). @@ -40,11 +49,13 @@ known GitHub quirks. Every gotcha below cost real session round-trips. 3. A dep bump breaking tests/config (e.g. a major with changed exports): consult **Context7** for the library's current API (CLAUDE.md MCP tools section) — do not grep `node_modules` `.d.ts` files. Fix on the Dependabot branch or note the PR for manual handling. -4. When all PRs have auto-merge set, **stop — do not leave a background monitor running.** - Auto-merge needs no supervision, and orphaned monitors fire notifications into later - sessions. Report the list with each PR's end state instead. +4. When the last PR has merged and its deploys finished, run the site smoke once more + (`/open-pr` skill §4). A failure is an incident: report it at once with the merges since the + last passing run. Then **stop — do not leave a background monitor running.** Orphaned + monitors fire notifications into later sessions. Report the list with each PR's end state + instead. ## Report Per PR: number, title, action taken (auto-merge set / updated+re-triggered / needs manual -attention + why). +attention + why), and the site smoke's result after the last merge. diff --git a/changelog.d/site-smoke-after-merge.md b/changelog.d/site-smoke-after-merge.md new file mode 100644 index 00000000000..bac50ccca0d --- /dev/null +++ b/changelog.d/site-smoke-after-merge.md @@ -0,0 +1,10 @@ +### Added + +- **A browser check that the site renders, before and after every merge.** + `scripts/site-smoke.mjs` loads the home page, `/plots` and a spec page in + headless Chromium and fails on a JavaScript error or an empty page. The + app-image CI job runs it against the built container, so a dependency bump + that breaks the boot fails its PR, and a new rule in `CLAUDE.md` (with the + recipe in the `open-pr` skill and the Dependabot batch command) runs it + against production after every merge. On 2026-10-22 a vite bump passed + every check and left every page blank for about 45 minutes. (#12236) diff --git a/docs/reference/repository.md b/docs/reference/repository.md index b7ba4427a2d..38d4af09c1d 100644 --- a/docs/reference/repository.md +++ b/docs/reference/repository.md @@ -194,6 +194,7 @@ anyplot/ │ └── cloudflare/ # Source for the apex Worker + its rollout notes │ ├── scripts/ # One-time and manual scripts +│ ├── site-smoke.mjs # Does the site render? After every merge, and in CI │ ├── evaluate-plot.py # Manual plot evaluation │ ├── regenerate-thumbnails.py # Image processing │ ├── backfill_review_metadata.py # One-time migration diff --git a/scripts/site-smoke.mjs b/scripts/site-smoke.mjs new file mode 100644 index 00000000000..d438fba3aa2 --- /dev/null +++ b/scripts/site-smoke.mjs @@ -0,0 +1,141 @@ +// Smoke test: does the site render? Run it after every merge to main, once the +// deploys the merge triggered have finished (CLAUDE.md, open-pr skill section +// 4); the app-image job of ci-image.yml runs it against the built container +// before the merge. A green PR did not prove the bundle boots: on 2026-10-22 a +// Vite minor bump (#12225) passed every check and blanked every page with +// `TypeError: t is not a function` until #12235. +// +// Per page it fails on a status other than 200, an uncaught page error, a +// page whose #root holds next to no text, or the app's error boundary ("Something +// went wrong", "A new version is available"), which keeps the masthead and +// footer around a failed route; the

is printed for the reader only, since +// some pages (`/plots`) have none, and so is the entry bundle (`index-*.js`), +// which tells which deploy answered. It also checks the API's /health. Console +// errors are printed as warnings only, since third-party noise (an analytics +// blocker, a GitHub rate limit) is not an outage. Exit code 0 means every +// check passed, 1 means at least one failed, 2 means no browser could be found +// or started, so nothing was checked. +// +// Run (playwright-core lives in the scratchpad, not the repo): +// SCRATCH= +// cd "$SCRATCH" && PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD=1 npm i --silent playwright-core@1.64.1 +// NODE_PATH="$SCRATCH/node_modules" node scripts/site-smoke.mjs +// +// Optional arguments replace the default pages. SITE and API override the +// origins (SITE=http://localhost:3000 against a dev server, API=none to skip +// the health check), CHROME_PATH names a browser binary (CI's Google Chrome), +// and MIN_TEXT the characters a rendered #root must hold. NODE_PATH does not +// reach ESM bare imports, so playwright-core is loaded through createRequire, +// which honours it. + +import { createRequire } from 'node:module'; +import { existsSync, readdirSync } from 'node:fs'; +import { homedir } from 'node:os'; + +const require = createRequire(import.meta.url); +const { chromium } = require('playwright-core'); + +const SITE = (process.env.SITE || 'https://anyplot.ai').replace(/\/$/, ''); +const API = (process.env.API || 'https://api.anyplot.ai').replace(/\/$/, ''); +const pages = process.argv.slice(2); +if (pages.length === 0) pages.push('/', '/plots', '/scatter-basic'); +// A rendered page carries far more; the blank page of #12225 carried none. +const MIN_TEXT = Number(process.env.MIN_TEXT || 200); + +// A browser that is already on the machine: CHROME_PATH, the cloud container's +// Chromium, or the one the Playwright MCP plugin installed locally. Never +// download one. +function findChromium() { + if (process.env.CHROME_PATH) return existsSync(process.env.CHROME_PATH) ? process.env.CHROME_PATH : null; + const roots = [process.env.PLAYWRIGHT_BROWSERS_PATH, '/opt/pw-browsers', `${homedir()}/.cache/ms-playwright`]; + for (const root of roots.filter(Boolean)) { + if (!existsSync(root)) continue; + const revisions = readdirSync(root) + .filter((d) => /^chromium-\d+$/.test(d)) + .sort((a, b) => Number(b.slice(9)) - Number(a.slice(9))); + for (const rev of revisions) { + for (const sub of ['chrome-linux/chrome', 'chrome-linux64/chrome']) { + const path = `${root}/${rev}/${sub}`; + if (existsSync(path)) return path; + } + } + } + return null; +} + +let failed = 0; + +if (API !== 'none') { + const health = await fetch(`${API}/health`).then( + (r) => r.status, + (e) => `error ${e.message}`, + ); + console.log(`${health === 200 ? 'OK ' : 'FAIL'}\t${API}/health\tstatus=${health}`); + if (health !== 200) failed += 1; +} + +const executablePath = findChromium(); +if (!executablePath) { + console.log('FAIL\tno browser found (CHROME_PATH, PLAYWRIGHT_BROWSERS_PATH, /opt/pw-browsers, ~/.cache/ms-playwright)'); + process.exit(2); +} +let browser; +try { + browser = await chromium.launch({ executablePath }); +} catch (e) { + // A browser that will not start (missing libraries, a sandbox) checked nothing: not an incident. + console.log(`FAIL\tthe browser at ${executablePath} did not start: ${e.message.split('\n')[0]}`); + process.exit(2); +} +for (const path of pages) { + const url = `${SITE}${path.startsWith('/') ? path : `/${path}`}`; + const page = await browser.newPage({ viewport: { width: 1440, height: 900 } }); + const pageErrors = []; + const consoleErrors = []; + page.on('pageerror', (e) => pageErrors.push(e.message)); + page.on('console', (m) => { + if (m.type() === 'error' && !/Failed to load resource/.test(m.text())) consoleErrors.push(m.text()); + }); + let status = 'no response'; + try { + const response = await page.goto(url, { waitUntil: 'domcontentloaded', timeout: 30000 }); + status = response ? response.status() : status; + // The app renders client-side: wait for #root to fill, bounded, then read it. + await page + .waitForFunction((min) => (document.getElementById('root')?.innerText.length ?? 0) >= min, MIN_TEXT, { + timeout: 15000, + }) + .catch(() => {}); + // The layout chrome alone can pass the text threshold while the route's lazy chunk still + // loads, so let the network settle (bounded) before the route is judged. + await page.waitForLoadState('networkidle', { timeout: 8000 }).catch(() => {}); + } catch (e) { + pageErrors.push(`navigation: ${e.message.split('\n')[0]}`); + } + const seen = await page + .evaluate(() => { + const text = document.getElementById('root')?.innerText ?? ''; + const entry = document.querySelector('script[type="module"][src*="/assets/index-"]')?.getAttribute('src'); + return { + text: text.length, + boundary: text.match(/Something went wrong|A new version is available/)?.[0] ?? null, + h1: document.querySelector('h1')?.innerText.trim().slice(0, 60) ?? null, + bundle: entry ? entry.split('/').pop() : null, + }; + }) + .catch(() => ({ text: 0, boundary: null, h1: null, bundle: null })); + const ok = status === 200 && pageErrors.length === 0 && seen.text >= MIN_TEXT && !seen.boundary; + if (!ok) failed += 1; + const detail = + (pageErrors.length ? `\tpageerror: ${pageErrors.slice(0, 2).join(' | ')}` : '') + + (seen.boundary ? `\terror boundary: "${seen.boundary}"` : ''); + const warn = consoleErrors.length ? `\tconsole errors (warning): ${consoleErrors.length}` : ''; + console.log( + `${ok ? 'OK ' : 'FAIL'}\t${url}\tstatus=${status}\ttext=${seen.text}\th1=${JSON.stringify(seen.h1)}` + + `\tbundle=${seen.bundle}${detail}${warn}`, + ); + await page.close(); +} +await browser.close(); +console.log(failed ? `${failed} check(s) failed: treat it as an incident` : 'all checks passed'); +process.exit(failed ? 1 : 0); diff --git a/tests/unit/test_agent_instructions.py b/tests/unit/test_agent_instructions.py index 1562e8afb61..67a51403ca2 100644 --- a/tests/unit/test_agent_instructions.py +++ b/tests/unit/test_agent_instructions.py @@ -425,6 +425,11 @@ def test_guardrails_split_stays_subordinate() -> None: "never extract, copy, or proxy the agy login", ], "External-system writes need explicit, named authorization": ["explicit, named authorization"], + "After every merge, check that the site renders": [ + "after every merge, check that the site renders", + "`scripts/site-smoke.mjs`", + "nothing else merges until the site renders", + ], "Modify repo files only with the Edit/Write tools": ["heredocs/sed", "outranks any harness or agent-mode reminder"], }