diff --git a/.github/workflows/bot-serving-check.yml b/.github/workflows/bot-serving-check.yml index 607023608a..af71f8ce8f 100644 --- a/.github/workflows/bot-serving-check.yml +++ b/.github/workflows/bot-serving-check.yml @@ -14,6 +14,16 @@ # verified on the first dispatched run. The origin is also exactly the layer # that broke in the incident above; Cloudflare-edge issues are out of this # monitor's reach by design. +# +# What the bot pages are is DERIVED from the repo, never written out here. The +# routes come from the `@router.get("/seo-proxy/…")` decorators in +# api/routers/seo.py and the spec title from plots//specification.yaml. +# Hand-written literals go stale: the home title changed to "anyplot.ai — +# AI-generated plot catalog for 15 libraries" and this check sat red for ten +# consecutive nights on a healthy site — an alarm nobody can trust, which is +# worse than no alarm. Deriving turns the check into what it is meant to assert +# ("what is served == what the repo says"), covers every bot page the moment it +# lands in seo.py, and cannot drift on a copy change again. name: Bot Serving Check @@ -22,21 +32,47 @@ on: - cron: "23 6 * * *" # daily 06:23 UTC workflow_dispatch: +# One run at a time, and the newest wins. Two overlapping runs — a manual +# dispatch during the nightly one, which can take up to the timeout below — +# would both mutate the same alarm issue: two failures could open it twice, and +# an older run finishing last could close a newer failure or reopen an alarm +# after a newer success. Cancelling the superseded run means only the latest +# result ever touches the issue. +concurrency: + group: bot-serving-check-${{ github.ref }} + cancel-in-progress: true + +# issues: write is the alarm path. Without it the job could only go red in a +# tab nobody subscribes to — which is how those ten red nights stayed unnoticed. +# The failure step opens (or comments on) one fixed-title issue and the success +# step closes it again. permissions: contents: read + issues: write jobs: bot-serving: runs-on: ubuntu-latest - # 27 check() calls x (--retry 2 -> up to 3 attempts x --max-time 30) can - # reach ~41 min worst-case, plus four non-retried probes (llms.txt charset, - # trailing slash, og-image, .well-known redirect — 30s each); 46 leaves + # 36 check() calls (10 derived bot routes + spec page + impl page + + # ClaudeBot + 15 crawler UAs + 404 + robots + sitemap + 3 llms + 2 human + # controls) x (--retry 2 -> up to 3 attempts x --max-time 30) can reach + # ~54 min worst-case, plus five non-retried probes (llms.txt charset, + # trailing slash, og-image, .well-known redirect, the /{spec}/{language} + # 301 — 30s each); 62 leaves # room to report a clean failure rather than dying to the job timeout, # which reports nothing useful. Recompute this when adding checks: the - # ceiling is check() calls x 90s, plus margin. - timeout-minutes: 46 + # ceiling is check() calls x 90s, plus margin. The route sweep grows with + # api/routers/seo.py, so a new bot page adds 90s to that ceiling. + timeout-minutes: 62 steps: - - name: Crawler UAs must get 200 + per-route titles + # The routes and the expected title are read out of the repo, so it has + # to be here before the first request goes out. + - name: Check out the repo + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 1 + + - name: Crawler UAs must get 200 + per-route pages run: | set -uo pipefail # Cloud Run origin of the anyplot-app service (see header comment @@ -66,21 +102,175 @@ jobs: fi } - # Bot path: prerendered per-route HTML from the seo-proxy - # Prefix, not the full title: the copy changed to "anyplot.ai — - # AI-generated plot catalog for 15 libraries" and this check went red - # for ten consecutive days without anyone noticing. The prefix still - # separates the prerendered page from the SPA shell, whose title is - # "any.plot() — any library.", which is the property under test. - check "$GOOGLEBOT" "$ORIGIN/" "anyplot.ai" - check "$GOOGLEBOT" "$ORIGIN/scatter-basic" "<title>Basic Scatter Plot | anyplot.ai" - check "$TWITTERBOT" "$ORIGIN/scatter-basic/python/matplotlib" "Basic Scatter Plot - Matplotlib | anyplot.ai" + # Bot path: prerendered per-route HTML from the seo-proxy. + # + # The routes are the API's own bot pages — every + # `@router.get("/seo-proxy/…")` in api/routers/seo.py is a page a + # crawler must be able to reach. Reading them from the source means a + # page added there is swept the moment it lands, and no list here can + # go stale. + # + # The extraction parses the AST rather than grepping for a literal + # spelling: a decorator written with single quotes, wrapped over two + # lines, or carrying kwargs would be silently skipped by a regex, and + # a silently short list is exactly the failure this file is trying to + # stop making. The parameterised routes (paths containing `{`) cannot + # be swept generically and are each probed by hand below, so their + # COUNT is returned too and checked against what this file probes — + # a fourth one added to seo.py fails the run instead of going + # unnoticed. + # + # The assertion is the canonical link, not a title: it is GENERATED + # from the route, so it can never drift on a copy change, and it + # proves both halves at once — the SPA shell carries no + # `` at all, so a match means the bot hop ran, + # and the href names the route, so it means the right page came back. + seo_decorators=$(python3 - <<'PY' + import ast + + SRC = "api/routers/seo.py" + static, templated, unresolved = set(), 0, 0 + for node in ast.walk(ast.parse(open(SRC, encoding="utf-8").read())): + for dec in getattr(node, "decorator_list", []): + if not isinstance(dec, ast.Call): + continue + fn = dec.func + if not (isinstance(fn, ast.Attribute) and fn.attr == "get"): + continue + if not (isinstance(fn.value, ast.Name) and fn.value.id == "router"): + continue + # FastAPI takes the path positionally OR as `path=`. Anything + # else — a variable, an f-string, a constant folded elsewhere — + # is a route this cannot reason about, and staying silent about + # it is how a bot page would drop out of the sweep unnoticed. + arg = dec.args[0] if dec.args else next( + (kw.value for kw in dec.keywords if kw.arg == "path"), None + ) + if not isinstance(arg, ast.Constant) or not isinstance(arg.value, str): + unresolved += 1 + continue + path = arg.value + if not path.startswith("/seo-proxy"): + continue + if "{" in path: + templated += 1 + else: + static.add(path.removeprefix("/seo-proxy") or "/") + for route in sorted(static): + print("route " + route) + print("templated %d" % templated) + print("unresolved %d" % unresolved) + PY + ) + routes=$(printf '%s\n' "$seo_decorators" | sed -n 's/^route //p') + templated=$(printf '%s\n' "$seo_decorators" | sed -n 's/^templated //p') + unresolved=$(printf '%s\n' "$seo_decorators" | sed -n 's/^unresolved //p') + + swept=0 + while IFS= read -r route; do + [ -n "$route" ] || continue + swept=$((swept + 1)) + check "$GOOGLEBOT" "$ORIGIN$route" "" + done <<< "$routes" + + # A checkout that silently produced nothing, a parse error, or a + # refactor that moves the decorators somewhere this cannot see would + # otherwise pass the whole sweep by checking zero routes. + if [ "$swept" -lt 1 ]; then + echo "::error::no static bot routes found in api/routers/seo.py — the sweep asserted nothing" + fail=1 + else + echo "swept $swept derived bot route(s)" + fi + + # The parameterised routes are probed one by one below: /{spec_id}, + # /{spec_id}/{language} and /{spec_id}/{language}/{library}. Bump this + # number only together with a new probe for the new route. + if [ "$templated" != "3" ]; then + echo "::error::api/routers/seo.py has $templated parameterised /seo-proxy route(s); this monitor probes 3 by hand — add a probe for the new one and update this guard" + fail=1 + fi + + # A router.get whose path is not a string literal cannot be classified + # at all, and a route this cannot see is a route it cannot promise to + # cover. Loud, not silent. + if [ "$unresolved" != "0" ]; then + echo "::error::$unresolved router.get decorator(s) in api/routers/seo.py carry a path this monitor cannot resolve — it may be skipping a bot page" + fail=1 + fi + + # The spec page's expected title comes from the spec file, not from a + # literal here. `$SPEC_TITLE` as a prefix: the suffix is site + # copy ("| anyplot.ai") and the impl page inserts the library name, + # neither of which this monitor is the right place to pin. + # Two decodings, both mirroring what the page actually does with the + # value. yaml.safe_load, not the source line: a quoted title is valid + # YAML and already occurs in this repo (plots/heatmap-chromagram). + # html.escape, because api/routers/seo.py escapes the title before it + # reaches `<title>` — `Mohr's Circle` is served as `Mohr's + # Circle`, and a raw needle would be a false alarm against a perfectly + # healthy page. Both are the same failure this rewrite exists to + # remove: an expectation that does not match what is served. + SPEC="scatter-basic" + python3 -c "import yaml" 2>/dev/null || pip install --quiet --disable-pip-version-check pyyaml + SPEC_TITLE=$(python3 - "$SPEC" <<'PY' + import html + import sys + + import yaml + + with open("plots/%s/specification.yaml" % sys.argv[1], encoding="utf-8") as fh: + doc = yaml.safe_load(fh) or {} + title = doc.get("title") + print(html.escape(title) if isinstance(title, str) else "") + PY + ) + if [ -z "$SPEC_TITLE" ]; then + echo "::error::plots/$SPEC/specification.yaml carries no title — nothing to assert against" + fail=1 + SPEC_TITLE="__no_title_in_the_spec_file__" + else + echo "expecting the $SPEC pages to be titled: $SPEC_TITLE" + fi + check "$GOOGLEBOT" "$ORIGIN/$SPEC" "<title>$SPEC_TITLE" + # The implementation page is asserted on its canonical instead: a + # three-segment route is the one that would still look right with the + # hub page served in its place, and the title prefix cannot tell them + # apart. + check "$TWITTERBOT" "$ORIGIN/$SPEC/python/matplotlib" \ + "<link rel=\"canonical\" href=\"https://anyplot.ai/$SPEC/python/matplotlib\" />" + + # The middle tier, /{spec}/{language}, was consolidated onto the hub + # and must answer 301 -> /{spec}. Its own docstring records why it is + # worth a probe: a Location of /seo-proxy/{spec} is re-prefixed by + # nginx, arrives back at this route and redirects forever — Googlebot + # logged 48 "Redirect error" URLs before the target was sanitised. The + # loop signature is a target that still carries /seo-proxy. + # The STATUS is part of the contract, not only the target: a 302/307/308 + # to the same hub would consolidate nothing, and the endpoint documents + # a permanent redirect. + read -r lang_code lang_target <<< "$(curl -sS --max-time 30 -o /dev/null \ + -A "$GOOGLEBOT" -w '%{http_code} %{redirect_url}' "$ORIGIN/$SPEC/python")" + if [ "$lang_code" != "301" ]; then + echo "::error::/$SPEC/python answered HTTP $lang_code (expected a permanent 301 onto the hub)" + fail=1 + else + case "$lang_target" in + *"/seo-proxy"*) + echo "::error::/$SPEC/python redirects into the internal proxy path: $lang_target" + fail=1 ;; + "$ORIGIN/$SPEC") echo "OK: /$SPEC/python -> 301 $lang_target" ;; + *) + echo "::error::/$SPEC/python should 301 to $ORIGIN/$SPEC, got '$lang_target'" + fail=1 ;; + esac + fi # AI assistants take the same prerendered path (nginx $is_bot). These # checks hit the ORIGIN, so they verify the nginx map independently of # whether Cloudflare's AI Crawl Control currently 403s these UAs at # the edge — an edge-level policy change needs no change here. - check "$CLAUDEBOT" "$ORIGIN/scatter-basic" "<title>Basic Scatter Plot | anyplot.ai" + check "$CLAUDEBOT" "$ORIGIN/$SPEC" "$SPEC_TITLE" # User-directed fetchers: a human asked their assistant to open the # page. All of these were verified receiving the empty SPA shell on @@ -104,7 +294,7 @@ jobs: "Grok/1.0" \ "Mozilla/5.0 (compatible; xAI-Bot/1.0)" do - check "$ua" "$ORIGIN/scatter-basic" "<title>Basic Scatter Plot | anyplot.ai" + check "$ua" "$ORIGIN/$SPEC" "$SPEC_TITLE" done # A crawler asking for a URL that is no page gets a real 404 from the @@ -162,7 +352,7 @@ jobs: # It used to 307 to http://api.anyplot.ai/seo-proxy/... — internal # path, wrong host, plain http, and that host disallows all crawling. slash_target=$(curl -sS --max-time 30 -o /dev/null -A "$GOOGLEBOT" \ - -w '%{redirect_url}' "$ORIGIN/scatter-basic/") + -w '%{redirect_url}' "$ORIGIN/$SPEC/") case "$slash_target" in "") # No redirect at all: %{redirect_url} is empty, which the previous @@ -178,7 +368,78 @@ jobs: # Control: humans must still get the SPA shell — on the home page # and on a deep route. - check "$HUMAN" "$ORIGIN/" '<div id="root">' - check "$HUMAN" "$ORIGIN/scatter-basic" '<div id="root">' + check "$HUMAN" "$ORIGIN/" '<div id="root">' + check "$HUMAN" "$ORIGIN/$SPEC" '<div id="root">' exit $fail + + # The alarm. One issue with a fixed title carries the whole history of + # this monitor: a first failure opens it, every further failure comments + # on it (so a long outage is one thread, not one issue per night), and + # the first green run closes it again. + # + # Only from the default branch. The schedule always runs there, but + # workflow_dispatch can pick any branch that carries this file — and a + # branch experimenting with the derived expectations must not be able to + # raise, or silently close, a repository-wide production incident. Off + # main the checks still run and still red the job; only the issue is left + # alone. + - name: Raise the alarm + if: failure() && github.ref == format('refs/heads/{0}', github.event.repository.default_branch) + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + ALARM_TITLE: Bot serving check is red + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + # env.ALARM_TITLE inside the jq filter, not string interpolation: + # the title travels as data and never as jq syntax. + num=$(gh issue list --state open --limit 100 --json number,title \ + --jq '[.[] | select(.title == env.ALARM_TITLE)] | .[0].number // empty') + if [ -n "$num" ]; then + echo "alarm issue #$num is already open — appending this run" + gh issue comment "$num" --body "Still red: $RUN_URL" + exit 0 + fi + # printf over one argument per line: a YAML block scalar cannot carry + # unindented lines, and a leading run of spaces inside the body would + # render as a markdown code block on the issue. + body=$(printf '%s\n' \ + "The daily bot-serving monitor failed: $RUN_URL" \ + "" \ + "Crawlers may be getting the SPA shell or an error page while the site" \ + "looks healthy to humans — the failure mode that went unnoticed for four" \ + "weeks in 2026 (see the header of the workflow file)." \ + "" \ + "Two causes are worth separating before anything else:" \ + "" \ + '- **The serving path broke** — `app/nginx.conf` (the `$is_bot` map, the' \ + ' `@seo_proxy` upstream) or the API `/seo-proxy` routes. A real incident.' \ + '- **The deployed pages are behind the repo** — the swept routes come from' \ + ' `api/routers/seo.py` and the expected title from the spec file, so a' \ + ' merged change that has not been deployed yet reads as a mismatch. Check' \ + ' for a pending deploy first.' \ + "" \ + "This issue closes itself on the next green run.") + echo "opening the alarm issue" + gh issue create --title "$ALARM_TITLE" --label bug --label infrastructure --body "$body" + + - name: Stand the alarm down + if: success() && github.ref == format('refs/heads/{0}', github.event.repository.default_branch) + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + ALARM_TITLE: Bot serving check is red + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + num=$(gh issue list --state open --limit 100 --json number,title \ + --jq '[.[] | select(.title == env.ALARM_TITLE)] | .[0].number // empty') + if [ -n "$num" ]; then + echo "closing alarm issue #$num" + gh issue comment "$num" --body "Green again: $RUN_URL" + gh issue close "$num" + else + echo "no open alarm issue — nothing to close" + fi diff --git a/CHANGELOG.md b/CHANGELOG.md index ec1b748100..3e10c6a984 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,33 @@ aggregate instead: an italic *Catalog* line at the end of the version section an ### Added +- **The bot-serving monitor now raises an alarm instead of only turning a tab red, and + derives what it asserts from the repo** — `bot-serving-check.yml` gained `issues: write`: + a failing run opens the fixed-title issue **Bot serving check is red** (or comments on it, + so a long outage is one thread rather than one issue per night) and the next green run + comments and closes it. Until now the check could only go red in the Actions tab, which is + exactly how it sat red for ten consecutive nights unnoticed — caused by the other half of + this change: hard-coded expectations. The swept routes now come from the + `@router.get("/seo-proxy/…")` decorators in `api/routers/seo.py` — read off the parsed + AST, so a decorator in single quotes, wrapped over two lines, carrying kwargs or passing + its path as `path=` is not silently skipped the way a regex skips it, and a path that is + no string literal at all fails the run rather than disappearing (10 routes today, and a + new bot page is covered the moment it lands) — and the expected spec title, YAML-decoded + and then HTML-escaped the way the renderer escapes it, from + `plots/<spec>/specification.yaml`, so a copy change can no longer make the alarm lie. The + per-route assertion is the generated `<link rel="canonical">` — the SPA shell carries none + at all and the href names the route, so one match proves both that the bot hop ran and + that the right page came back. Two watchdogs stop a silent no-op: the run fails if the + sweep found no routes or the spec file no title, and it fails if `seo.py` grows a fourth + parameterised route beyond the three this file probes by hand. The middle of those three, + the consolidated `/{spec}/{language}`, gained a probe of its own — it must answer 301, and + a target still carrying `/seo-proxy` is the redirect loop that once cost 48 Googlebot + "Redirect error" URLs. A `concurrency` group keeps a manual dispatch from racing the + nightly run over the same issue, and the two issue-mutating steps run only on the default + branch, so a dispatch from a feature branch can never raise or close a production + incident. Timeout recomputed to 62 min by the file's own formula (36 checks x 90 s + five + non-retried probes). (#11209) + - **The API image is built and its container smoke-tested before merge, not after** — the first build attempt of a changed Dockerfile used to happen in Cloud Build, once the PR was already on `main`; that is how the deploy-api trigger sat red from 2026-08-30 until #10821 diff --git a/agentic/docs/project-guide.md b/agentic/docs/project-guide.md index 7728070b79..a7c0254634 100644 --- a/agentic/docs/project-guide.md +++ b/agentic/docs/project-guide.md @@ -743,7 +743,7 @@ Issue ready for maintainer review | **daily-regen.yml** | Scheduled: picks the oldest specs, runs spec polish + cross-library similarity audit, then dispatches bulk-generate | | **auto-update-pr-branches.yml** | When `main` advances, updates open PRs that have auto-merge enabled | | **watchdog-stuck-jobs.yml** | Periodic safety net: detects and unsticks stalled impl-pipeline PRs | -| **bot-serving-check.yml** | Synthetic monitor for the nginx bot -> seo-proxy path | +| **bot-serving-check.yml** | Daily synthetic monitor for the nginx bot -> seo-proxy path; routes and titles derived from `api/routers/seo.py` and the spec files, and a failure opens/comments the fixed-title issue "Bot serving check is red" (closed again by the next green run) | ### Decoupled Architecture diff --git a/docs/workflows/overview.md b/docs/workflows/overview.md index a70c0c658a..98d347a7d1 100644 --- a/docs/workflows/overview.md +++ b/docs/workflows/overview.md @@ -178,7 +178,7 @@ Located in `.github/workflows/`: | `ci-tests.yml` | Unit + integration tests on PRs | | `ci-image.yml` | Builds `api/Dockerfile` and smoke-tests the container before merge — `/health`, the reported version against `pyproject.toml`, the runtime stage's COPY payload, non-root uid — plus hadolint on both Dockerfiles. Skips when only `plots/**` or frontend sources changed | | `notify-deployment.yml` | Records GitHub deployment events for `app` / `api` | -| `bot-serving-check.yml` | Daily synthetic monitor: curls production with crawler UAs (Googlebot/Twitterbot) and fails on non-200 or missing per-route titles — the bot→seo-proxy path is invisible to human traffic and needs its own alarm | +| `bot-serving-check.yml` | Daily synthetic monitor: curls the Cloud Run origin with crawler UAs and fails on non-200 or a page that is not the prerendered one. Routes are derived from the `@router.get("/seo-proxy/…")` decorators in `api/routers/seo.py` and the expected spec title from `plots/<spec>/specification.yaml`, so no literal here can go stale. A failure opens (or comments on) the fixed-title issue **Bot serving check is red**; the next green run closes it — the bot→seo-proxy path is invisible to human traffic and needs its own alarm | | `util-claude.yml` | On-demand `@claude` utility (issue/PR comments) | ---