Repository navigation
fix(coverage-floor): pull the service images from mirror.gcr.io, not Docker Hub - #296
Conversation
…Docker Hub dotclaude#479 (run 37991904725) and #485 failed "coverage-floor / Measure coverage and enforce floor" on 2026-10-09 before their first step: `docker pull postgres:16` -> `toomanyrequests: You have reached your unauthenticated pull rate limit` on every retry. Docker Hub rate-limits anonymous pulls per IP, GitHub-hosted runners share egress IPs, and this job starts its postgres and redis services on every run of every caller (that run had services_postgres: false). A bare image name (no `/`, so a Docker Official Image), the reusable's own default or one a caller passes, now resolves to mirror.gcr.io/library/<name>, Google's Docker Hub mirror, which needs no credential. The 18 callers code search finds all call this reusable @main and 17 pass no image, so they get the fix with no edit. A reference with a `/` is pulled exactly as given: GitHub's expression language cannot split a string to tell a registry host from a Docker Hub namespace. inbox_superpilot's `pgvector/pgvector:pg16` therefore keeps pulling from Docker Hub until its caller passes `mirror.gcr.io/pgvector/pgvector:pg16`; the input description now says so. Why mirror.gcr.io and not public.ecr.aws/docker/library: ECR Public caps anonymous pulls at 1/s (not adjustable) and 500 GB/month, lacks some library tags (postgres:12.1-alpine is a 404 there), and does not carry pgvector. mirror.gcr.io served every real tag probed, and its digests for postgres:16, redis:7 and pgvector/pgvector:pg16 match Docker Hub's. Images stay tag-pinned, as everywhere else in this repo. selftest/test_job_images_off_docker_hub.py resolves every image a job in .github/workflows pulls (services, the job container, docker:// steps) by evaluating its expression with empty inputs, the declared defaults and caller-passed bare names, and fails on any Docker Hub result. Hardcoded expectations pin both coverage-floor services. Negative controls: the pre-fix default, a mirrored default that lets a caller's bare name through, a mangled registry ref, a typo in the mirror path, a service renamed out of the scan, and fixtures of every slot kind. No other workflow here has a services:, container: or docker:// pull. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and string services From the independent review of the previous commit (no defects; nits only): - The headlines no longer overclaim. A caller that passes a Docker Hub `<namespace>/<name>` reference (inbox_superpilot's pgvector/pgvector:pg16) is still pulled as given, so the guard is "no default or bare name goes to Docker Hub", and the workflow comment says how such a caller opts in. - Each negative control now asserts the failure it should cause, not just any AssertionError. The caller-name control mirrors the declared default too, so only a name a caller passes can catch it; before, the declared default caught it first. - The scan reads .yaml workflows as well as .yml, and a service given as a plain string. - Input names resolve case-insensitively, as GitHub resolves them; a re-cased fixture pins it (it fails against the old case-sensitive lookup). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Risk class: This PR touches one of the blocked path categories from Auto-merge is refused by (This is a policy notice, not a code-quality failure. The classify job itself does not fail — required CI checks remain authoritative for "is the code green.") |
|
The review is complete. Key findings: Workflow YAML ( Expression evaluator (
Negative controls: All five mutations ( No issues found. Diff is well-reasoned, well-tested, and correct. Claude Review · workflow run |
Both fixes target the same Docker Hub rate limit and compose: a service starts only when its services_* input asks (this branch), and once on, a bare image name is pulled from mirror.gcr.io/library (#296). Each image is #296's expression wrapped in the gate: inputs.services_x && (<#296's expression>) || '' test_job_images_off_docker_hub.py now switches a service on before asking where its image comes from (its scan and REQUIRED), since switched off the image is '' and nothing is pulled. test_coverage_floor_services.py uses that file's resolve() (the evaluator it used could not parse contains() or format()) and expects the mirror references once a service is on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#295) ## Summary `coverage-floor.yml`'s `measure` job started its postgres and redis services on **every** run of every caller. `services_postgres` and `services_redis` (both default `false`) gated only the `DATABASE_URL` / `TEST_DATABASE_URL` / `REDIS_URL` exports, never the containers. On 2026-10-09, Docker Hub's unauthenticated pull limit (`toomanyrequests`) failed: - every coverage-floor run of topcoder1/dotclaude#485, - a wxa_webcat PR run (37993037252). Each run died starting a container that caller never uses. The check is required directly in 8 repos and through a `coverage-floor-gate` job in 2 more. **#296 (merged first) fixed *where* the images come from:** a bare name is pulled from `mirror.gcr.io/library/`. **This PR fixes *whether* they are pulled at all:** a service starts only when its input asks. Both are kept, merged in `5378307`. ## Fix GitHub skips a service whose image is an empty string ([workflow syntax, `jobs.<job_id>.services.<service_id>.image`](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#jobsjob_idservicesservice_idimage)). Each image is #296's expression wrapped in a gate on its input: ```yaml image: ${{ inputs.services_postgres && (contains(inputs.postgres_image || 'postgres:16', '/') && inputs.postgres_image || format('mirror.gcr.io/library/{0}', inputs.postgres_image || 'postgres:16')) || '' }} image: ${{ inputs.services_redis && (contains(inputs.redis_image || 'redis:7', '/') && inputs.redis_image || format('mirror.gcr.io/library/{0}', inputs.redis_image || 'redis:7')) || '' }} ``` - **Off:** the image is `''`. No container starts, nothing is pulled, and the runner adds no "Initialize containers" step (actions/runner `JobExtension.cs`). - **On:** the image is exactly what #296 resolves. #296's inner expression is never empty, so the gate passes it through unchanged. - The URL exports were already gated on the same inputs, so a URL is exported exactly when its container runs. Each image input's description now also says it is pulled only when its `services_*` input is true. The job name and required-check context are unchanged. ## Who this changes There are 18 callers, all on `@main`. | Callers | On main (#296) | With this PR | |---|---|---| | `topcoder1/inbox_superpilot` (both `services_*: true`, `pgvector/pgvector:pg16`) | both services | both services, unchanged | | `wxa_vpn`, `webcrawl` (pre-measured coverage: no tests run in this job) | pull both images from the mirror | pull nothing | | The other 15 | pull both images from the mirror, unused | pull nothing | **A hidden dependency was checked before landing, twice and independently.** #296's "Not done" section names this exact change and the audit it needs. The risk was a non-opted-in caller whose tests reached the always-on containers without the env exports. Postgres would need hardcoded `postgres:postgres`; redis has no auth, so a client defaulting to `localhost:6379` would connect silently. Both audits grepped the callers' code, with `inbox_superpilot` as the positive control: mine on shallow clones of each caller's default branch, the reviewer's on its local clones. Findings for the repos where the check is required: - **wxa_sanctions:** identical pytest results with and without the containers on one commit (160 passed, 3 skipped). - **wxa_webcat:** an autouse conftest fixture swaps every default-`localhost:6379` `CacheLayer` for a private fakeredis, and no test opts out. - Known side effect: its app lifespan pings `localhost:6379`, so `TrafficTracker` used to switch on in two test files and now stays off. - Nothing asserts on it, and it costs about 5 statements against 91.2% measured vs an 83.0 floor. - **techrecon:** its PG tests skip without `TECHRECON_TEST_PG`, and its redis tests use their own listeners or dead addresses. - **wxa-graph:** uses `testing.Short()` skips and its own testcontainers postgres. - **wxa-mcp-server:** its integration tests skip unless `WHOISXMLAPI_*` is set. - **wxa-jake-ai:** its localhost DSNs are `test-user:test-pass` stubs the container rejects, and its DB tests run in a separate required job. - **domains_collector:** sqlite plus a config-override stub. The non-required callers either mock, or read `DATABASE_URL`/`REDIS_URL`, which only opted-in callers get. Their localhost DSNs use roles the container refuses. No caller injects those URLs via `extra_env`. If a caller turns out to need a container after all, it adds `services_postgres: true` / `services_redis: true`. ## Tests - **`selftest/test_coverage_floor_services.py` (new)** pins *whether* a service starts. It renders each image with #296's `resolve`, using the declared defaults with a caller's values on top. The cases: - a caller that sets nothing, or only an image, gets `''`; - each switch works independently; - inbox_superpilot's caller config resolves as expected; - an empty image input falls back to the default. Expected images are hardcoded. - **`selftest/test_job_images_off_docker_hub.py` (#296)** pins *where* an image comes from. It now switches a service on, both in its scan and in `REQUIRED`, before asking that, since a switched-off service pulls nothing. Its negative controls are unchanged and still fail for the reasons they name. - **TDD on the gating:** 4 failed / 2 passed on the old workflow, then green. - **Mutation check on the merged workflow** (unmutated control: 6 and 55 passed): | Mutant | test_coverage_floor_services | test_job_images_off_docker_hub | |---|---|---| | ungated (#296 alone) | **3 failed** | passes, by design | | gated on the wrong switch | **2 failed** | **2 failed** | | no `\|\| ''` (off renders `false`) | **3 failed** | **3 failed** | | `services_postgres` defaults to `true` | **3 failed** | passes, by design | | gated, mirror dropped (this PR alone) | **2 failed** | **3 failed** | - **Suite runs:** - `uv run pytest -q`: 711 passed, 1 skipped, 0 failed before the merge. - After the merge, the two image tests (61) plus every guard that reads `coverage-floor.yml` (47 with them) pass. CI runs the full suite on `5378307`. - **Lint:** `actionlint` as `lint.yml` runs it (no shellcheck) is clean. `ruff check`, `ruff format --check`, prettier and `git diff --check` are clean. ## Review **Independent code review** (code-reviewer agent, on `80cfbbe`): **APPROVE WITH NITS**, no production defects. - It checked the expressions against GitHub's runner source as well as the docs. - It also did its own caller audit, and caught the wxa_webcat lifespan effect above. - Fixed in `8d80995`: the fleet counts, and a reference that made dotclaude#485 read like an issue about this bug. - Declined, as a separate cleanup: consolidating the suite's expression evaluators into one helper. **Codex pre-review:** 2 rounds on `80cfbbe`, both with no findings. The merge resolution (`5378307`) is covered by the mutation table above. ## Not in this PR - **inbox_superpilot** still pulls `pgvector/pgvector:pg16` from Docker Hub. This is #296's documented follow-up: that caller should pass `mirror.gcr.io/pgvector/pgvector:pg16`. - **wxa_webcat and wxa-graph** start `postgres:16` through testcontainers, which still uses Docker Hub. Both fail loudly on a pull error. **After merge:** a coverage-floor run of any non-opted-in caller should show no "Initialize containers" step. Auto-merge rationale: manual click-merge. This changes `.github/workflows/**` in topcoder1/ci-workflows (always manual), in a reusable that 18 repos call and 10 require. Codex rounds: 2 (native `codex review`, gpt-5.6-sol, reasoning high, on `80cfbbe` before the #296 merge; both clean and read by hand) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
coverage-floor.yml'smeasurejob pulled both of its service images anonymously from Docker Hub. On 2026-10-09 dotclaude#479 (run 37991904725) and dotclaude#485 failed before their first step:Docker Hub rate-limits anonymous pulls per IP, and GitHub-hosted runners share egress IPs. The job starts both services on every run of every caller, whatever
services_postgres/services_redissay (that run hadservices_postgres: false), so every caller is exposed.Fix
A bare image name (no
/, so a Docker Official Image) now resolves to Google's Docker Hub mirror,mirror.gcr.io/library/<name>, which needs no credential:postgres_image/redis_image'', or this repo's own runsmirror.gcr.io/library/postgres:16/mirror.gcr.io/library/redis:7postgres:16,postgres:15-alpine,redis:7-alpinemirror.gcr.io/library/<same>pgvector/pgvector:pg16mirror.gcr.io/pgvector/pgvector:pg16,ghcr.io/...The 18 callers code search finds all call this reusable
@main, and 17 of them pass no image input, so they get the fix with no edit. A reference with a/is pulled exactly as given: GitHub's expression language has no split or substring, so it cannot tell a registry host from a Docker Hub namespace. The input descriptions now say so.Follow-up, not in this PR: topcoder1/inbox_superpilot passes
postgres_image: pgvector/pgvector:pg16, so its postgres pull stays on Docker Hub until that caller passesmirror.gcr.io/pgvector/pgvector:pg16(same digest as Docker Hub's). Its redis pull is fixed here.Why mirror.gcr.io and not public.ecr.aws/docker/library
postgres:12.1-alpineis a 404 there and a 200 on Docker Hub and mirror.gcr.io) and does not carry pgvector at all.crane digestmatches Docker Hub's forpostgres:16,redis:7andpgvector/pgvector:pg16, and a 29.8 MB layer downloads through it.postgres:16andredis:7are among Docker Hub's most-pulled images.No digest pin: nothing in this repo pins an image by digest, and the old
postgres:16/redis:7were floating tags. No Docker Hub credentials were needed, so there are no new secrets.Scope
No other workflow in this repo has a
services:,container:ordocker://pull, and the two local composite actions pull no images.Test
selftest/test_job_images_off_docker_hub.py(new;uv run pyteston the tests-runner self-test path collects it):.github/workflowspulls (each service's image, the job container, eachdocker://step) by evaluating its expression with emptyinputs, the declared defaults, and bare names a caller might pass. Any Docker Hub result fails. The evaluator models only the expression subset image fields use and raises on anything else, so it fails closed.inputs.postgres_image || 'mirror.gcr.io/library/postgres:16'); every reference prefixed (a pgvector ref mangled); a typo in the mirror path; the postgres service renamed out of the scan. Fixtures cover every slot kind (literal, string-form service, pass-through, container string and mapping,docker://step), a re-cased input name, and a clean set.caller passes nothing (declared defaults) -> postgres:16, dotclaude's exact case. GREEN after: 55 passed. Full suite: 760 passed, 1 pre-existing skip. actionlint 1.7.12, ruff 0.15.10 (check and format) and prettier 3: clean.Review
code-reviewerlane (fresh context, Opus): no defects (0 critical, 0 high). One risk, inbox_superpilot's namespaced image, is the follow-up above. Four nits were applied in the second commit: headlines no longer overclaim; each negative control pins its failure reason;.yamland string-form services are scanned; input lookup is case-insensitive like GitHub's. It suggested auto-mapping refs with a/but no.tomirror.gcr.io/as well. I left that out: mirror.gcr.io's coverage of less-pulled namespaced images is unverified, so it could turn an occasional rate limit into a hard 404 for a rare image, where an explicit caller opt-in cannot.gpt-5.6-sol, reasoning high,codex review --base origin/main): three rounds, zero findings. Rounds 1 and 2 ran on the first commit, round 3 on the final diff.The real-runner proof is this PR's own
Coverage Floorself-test run, which now pulls both images from mirror.gcr.io.Not done (possible follow-up)
The services start even when
services_postgres/services_redisare false, as they are for 17 of 18 callers. Gating each service on its flag would drop those pulls entirely, but a caller whose tests reachlocalhost:5432without setting the flag would break, so it needs a caller audit first.Auto-merge rationale: manual. PRs to topcoder1/ci-workflows are always click-merge, and this changes a reusable that every caller runs
@main.Codex rounds: 3
🤖 Generated with Claude Code