Skip to content

fix(coverage-floor): pull a service image only for a caller that asks - #295

Merged
topcoder1 merged 3 commits into
mainfrom
claude/coverage-floor-conditional-services
Oct 11, 2026
Merged

topcoder1 merged 3 commits into
mainfrom
claude/coverage-floor-conditional-services

Conversation

@topcoder1

@topcoder1 topcoder1 commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

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). Each image is #296's expression wrapped in a gate on its input:

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')) || '' }}

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 fix(coverage-floor): pull the service images from mirror.gcr.io, not Docker Hub #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 (fix(coverage-floor): pull the service images from mirror.gcr.io, not Docker Hub #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

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

topcoder1 and others added 2 commits October 9, 2026 14:34
The measure job's postgres and redis services were unconditional, so every
caller pulled postgres:16 and redis:7 from Docker Hub on every run;
services_postgres and services_redis only gated the DATABASE_URL and
REDIS_URL exports. Docker Hub's unauthenticated pull limit then failed the
job before it measured anything (dotclaude#485, twice), and coverage-floor
is a required check in nine repos.

GitHub skips a service whose image is an empty string, so each image is now
gated on its input. inbox_superpilot, the one caller that opts in, keeps
both services and its pgvector image; the other sixteen stop pulling two
images they never used. selftest/test_coverage_floor_services.py renders
the image expressions the way Actions does for each input combination.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From review: #485 is a PR whose coverage-floor runs hit the pull limit,
not an issue about this bug, and the test's caller counts were off (there
are 18 callers, not 17) and would go stale anyway.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@topcoder1 topcoder1 added the manual-merge Durable hold: never auto-merge; a human must click. Outranks all automerge bypasses. label Oct 9, 2026
@github-actions github-actions Bot added the risk:blocked Risk class: blocked label Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

Risk class: blocked — manual merge required.

This PR touches one of the blocked path categories from .github/risk-paths.yml (Dockerfiles, docker-compose, .github/workflows/**, **/.env*, **/secrets*, infra/, terraform/, k8s/, or the classifier config itself).

Auto-merge is refused by claude-author-automerge.yml. A maintainer should review the diff and click "Squash and merge" themselves.

(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.")

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

The logic is sound. Let me verify the expression evaluation trace for the key cases:

Expression: inputs.services_postgres && (inputs.postgres_image || 'postgres:16') || ''

Using _and: returns right if left is truthy, else left. Using _or: returns left if truthy, else right.

  • services_postgres=false: false && X → false; then false || '' → '' ✓
  • services_postgres=true, postgres_image="": true && ('' || 'postgres:16') → 'postgres:16'; then 'postgres:16' || '' → 'postgres:16' ✓
  • services_postgres=true, postgres_image="pgvector/pgvector:pg16": correctly returns the custom image ✓

The YAML on: key gotcha (workflow.get("on", workflow.get(True))) is handled correctly. The substitute function receives Python booleans from YAML defaults and caller inputs, which the evaluator handles consistently with GitHub Actions semantics. All test parametrize cases correctly cover the intended behavior.

No issues found. The fix is targeted, the expression logic is correct, and the new test pins the behavior accurately.

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>
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

I've reviewed the full diff — the workflow YAML expression changes, the new test_coverage_floor_services.py, and the updates to test_job_images_off_docker_hub.py. Here's what I checked:

Image gating expressions (coverage-floor.yml)
Both new expressions follow the pattern inputs.services_* && (...inner...) || ''. GitHub's expression language has && binding tighter than ||, so when the switch is false the whole && arm collapses to False, then False || '' yields ''. GitHub skips a service whose image is an empty string, so no pull happens. When the switch is true the inner expression (mirror logic) is returned. Operator precedence is correct.

_scenarios update (test_job_images_off_docker_hub.py)
The new on dict collects all boolean inputs referenced in the image template and sets them to True before generating string-input scenarios. For the postgres template that's {"services_postgres": True}, for redis {"services_redis": True}. String inputs like postgres_image are still iterated with samples — but now with the service switched on, so the sample actually feeds through to the image. The "inputs empty" and "declared defaults" scenarios still fire (service off → image '' → not a Docker Hub pull → no false positives). The added "caller switches on …" scenario covers the mirror-default case that the defaults scenario lost.

REQUIRED / _check_required update
Tuple shape changed from (input_name, cases) to (switch, input_name, cases). Destructuring in _check_required matches. The None-value case now supplies {switch: True} so the default-image scenario still resolves through the gated expression correctly.

Negative control mutations
All five mutations still trigger the expected assertion. _back_to_docker_hub drops the services_postgres gate, so _scenarios finds no boolean input and fires the "inputs empty" scenario with a bare Docker Hub ref. _prefix_every_reference is caught by _check_required (mirror-prefixed namespace image ≠ expected bare form). The others are unaffected by the new gating logic.

New test file
_images uses the established workflow.get("on", workflow.get(True)) pattern (PyYAML 1.1 parses on: as True:). The parametrized cases cover the off, image-only, each-service-on, and combined scenarios cleanly.

No issues found. The diff is well-scoped and correctly tested.

Claude Review · workflow run

@topcoder1
topcoder1 merged commit bd86424 into main Oct 11, 2026
17 checks passed
@topcoder1
topcoder1 deleted the claude/coverage-floor-conditional-services branch October 11, 2026 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual-merge Durable hold: never auto-merge; a human must click. Outranks all automerge bypasses. risk:blocked Risk class: blocked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant