Skip to content

ci: build the API image and smoke the container before merge - #11205

Merged
MarkusNeusinger merged 3 commits into
mainfrom
infra/ci-image-smoke
Sep 2, 2026
Merged

ci: build the API image and smoke the container before merge#11205
MarkusNeusinger merged 3 commits into
mainfrom
infra/ci-image-smoke

Conversation

@MarkusNeusinger

@MarkusNeusinger MarkusNeusinger commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Why

The first build attempt of a changed api/Dockerfile happened in Cloud Build — after the merge. That is not a hypothetical: the deploy-api trigger sat red from 2026-08-30 until #10821, and every PR check was green the whole time, because nothing in CI ever built or ran the image. The sibling repo kurrentschrift added the same job for the same class of miss (its pyproject.toml fell out of the runtime stage, so the API would have reported version 0.0.0 in production and no check could have seen it). Transferred here per the sibling rule; keep the two jobs in the same shape.

What the new ci-image.yml does

A single image job, in the shape ci-lint.yml / ci-tests.yml already use:

  1. Change detection. Builds only when api/**, core/**, pyproject.toml, uv.lock, README.md, a root .dockerignore, app/Dockerfile or the workflow itself changed. plots/** is deliberately excluded — the automated plot pipeline opens hundreds of PRs that touch nothing the image serves, and each would otherwise pay for a container build. Verified against a sample file list: plots/…/plot.py, app/src/App.tsx, app/README.md and docs/*.md do not match; api/main.py, core/images.py, pyproject.toml, uv.lock, README.md, .dockerignore, app/Dockerfile and .github/workflows/ci-image.yml do. One case branch per event, and each fails closed: a PR or merge-group diff that cannot be computed stops the job rather than reading as "nothing changed", and a push diffs its whole range (github.event.before..after, so an earlier commit touching api/ under an unrelated tip commit still counts) with a new branch or an unreachable force-pushed tip building rather than skipping. All four branches simulated against a throwaway repo — see Evidence.
  2. Build with docker/build-push-action, push: false / load: true (nothing reaches a registry — Cloud Build still owns the published image; load puts the result in the local daemon so it can actually be run) and cache-from/cache-to: type=gha.
  3. DB-free container smoke. docker run with no database and no secrets — api/main.py guards its DB init with is_db_configured(), so a bare container boots. Then:
    • /health answers "healthy" within a 90 s readiness window (this image imports matplotlib, scikit-learn, statsmodels and the MCP server first).
    • The version assert. /health's version must equal pyproject.toml's. api/version.py reads the installed distribution's metadata and falls back to 0.0.0+unknown when it is absent — silently, in a field /health, /openapi.json and the MCP server all report. The builder stage installs the project from a context that has pyproject.toml and uv.lock but no source yet, so that dist-info is a genuinely fragile artefact of the stage ordering and nothing else in CI looks at it.
    • The COPY list. test -f /app/api/static/og-image.png — the one payload the runtime stage must ship that no import would catch. og_images.py reads it off disk as the last resort when dynamic OG rendering fails, i.e. exactly in a fresh container that cannot reach the font bucket.
    • Non-root. id -u must be 1000. USER appuser is one line above the CMD; nothing else notices if a rebuild drops it, and Cloud Run runs whatever the image says.
    • Container logs are dumped on failure.
  4. Two hadolint steps, threshold warning rather than a non-blocking run, so a warning of any other code blocks — which is the point of having the linter. The three exceptions are named at their line. Note honestly what ignore is: hadolint applies it file-wide, so a second DL3013/DL3008/DL3025 elsewhere in api/Dockerfile is suppressed too. Line-scoping needs # hadolint ignore=<code> comments in the Dockerfile itself — the better home, and the named follow-up below, but a Dockerfile edit is not this change's to make.

Evidence

Both runs on this PR, Build API image and smoke the container:

build step whole job
cold (no GHA layer cache) — run 33683067699 1 m 59 s 2 m 25 s
warm (cache from the run above) — run 33683725363 32 s 1 m 01 s

The job runs in parallel with the other CI workflows, so the pipeline's wall clock is unchanged; it only adds runner minutes, and only on PRs that touch the image.

Smoke output, verbatim:

health OK
version OK: 3.2.0
COPY list OK
non-root OK: uid 1000

(Container answered /health about 6 s after docker run; 3.2.0 is pyproject.toml's project.version.)

Hadolint was reproduced locally against 2.15.1, the version hadolint-action@v3.5.0 pins:

Dockerfile threshold warning, no ignores with the ignores in this PR
api/Dockerfile DL3013 (line 25), DL3008 (47), DL3025 (90) → exit 1; DL3066 (80) at info exit 0
app/Dockerfile exit 0 — (no ignores needed; clean even at --failure-threshold info)

The three ignores and why each is a deliberate choice, not a suppression:

  • DL3013pip install uv unpinned. uv is the installer; the versions that matter are pinned in uv.lock, which the very next line honours with uv sync --frozen.
  • DL3008 — unpinned apt curl / libraqm0. Pinning a Debian point release breaks the build on every security update of the base image.
  • DL3025 — shell-form HEALTHCHECK CMD … || exit 1. The fallback needs a shell; JSON form cannot express it.

DL3066 (non-numeric USER) also fires but only at info level, so it stays visible in the log without blocking — useradd -u 1000 already gives the user a fixed uid.

Change detection simulated against a throwaway repo, all four branches:

case result
push, 2 commits, only the first touches api/, tip is unrelated should_build=true (the old HEAD~1..HEAD form saw only unrelated.txt)
push, new branch (before = zeroes) build, with a warning
push, force-pushed / unreachable before build, with a warning
pull_request with an unreachable base sha exit 1, ::error::…refusing to decide…
push, nothing image-relevant anywhere in the range should_build=false

actionlint 1.7.7 on the new file: clean (exit 0). bash -n on the extracted change-detection script: clean.

Decisions taken here (routine, flagged for override)

  • A third ci-*.yml rather than a job inside ci-tests.yml. The repo already separates CI concerns file by file (ci-lint, ci-tests, bot-serving-check, notify-deployment, sync-postgres), and a docker build has a different runtime profile and a different skip condition than the Python test job. This extends the existing layout rather than changing it.
  • Event data reaches the change-detection shell through env:, not ${{ }} inside the script. Same logic as the sibling files, one notch safer; a shell that never sees interpolated event data cannot be made to execute it.
  • The non-root assert is not in kurrentschrift's version. It is three lines, guards a property nothing else covers, and belongs to the same question the COPY assert asks ("is the image still what the Dockerfile says").
  • New uses: are pinned by commit SHA of the current major tag, as everything else in .github/workflows/ is; all three SHAs were verified against the upstream tag lists (docker/setup-buildx-action v4.3.0 = v4, docker/build-push-action v7.3.0 = v7, hadolint/hadolint-action v3.5.0). Dependabot's github-actions ecosystem keeps them current.

Findings for the author — not changed here, api/ is out of this PR's scope

  1. api/.dockerignore is dead. The build context is the repo root (-f api/Dockerfile .), so Docker looks for /.dockerignore, which does not exist. Proof: api/.dockerignore excludes *.md, yet the builder's COPY pyproject.toml uv.lock README.md ./ succeeds — it could not if that file were in effect. Consequence: every build ships the full repo (~210 MB incl. .git and plots/) as context.
  2. The three hadolint exceptions belong in api/Dockerfile as # hadolint ignore=<code> comments, one line above RUN pip install uv, the apt-get install, and the HEALTHCHECK. That makes them line-scoped, so a new occurrence of the same code elsewhere blocks — which the workflow-level ignore cannot do. Three comment lines; left out only because api/ is not this PR's to touch.
  3. COPY plots/ ./plots/ in the runtime stage looks like dead weight (~98 MB). Nothing under api/ or core/ reads the directory at runtime — implementations are served from Postgres (sync-postgres.yml). If that holds, dropping it would take about a fifth off the image. I did not touch it: it is a behaviour change in a file this PR only lints.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke

The first build attempt of a changed Dockerfile happened in Cloud Build,
after the merge — which is how the deploy-api trigger sat red from
2026-08-30 until #10821 while every PR check stayed green.

ci-image.yml builds api/Dockerfile with Buildx (push: false, load: true,
GHA layer cache), runs the result with no database and no secrets, and
asserts what only a running container can show: /health answers, its
version equals pyproject.toml's, the OG disk fallback survived the
runtime stage's COPY, and the process runs as uid 1000. Two hadolint
steps gate both Dockerfiles at threshold warning, so a NEW warning
blocks; the three exceptions in api/Dockerfile are named at their line.

Change detection follows the ci-lint/ci-tests shape and deliberately
excludes plots/**: the plot pipeline's PRs touch nothing the image
serves.

Adopted from the sibling repo kurrentschrift, which added the same job
after its pyproject.toml fell out of the runtime stage.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI balanced review requested due to automatic review settings September 2, 2026 21:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Change detection can fail open and omits build-context inputs, allowing relevant changes to bypass the image gate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a pre-merge CI gate for building, linting, and smoke-testing the API container.

Changes:

  • Adds image build, runtime assertions, and Hadolint checks.
  • Documents the workflow and adds a changelog entry.
File summaries
File Description
.github/workflows/ci-image.yml Adds the container CI job.
docs/workflows/overview.md Documents the workflow.
CHANGELOG.md Records the CI improvement.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/ci-image.yml Outdated
Comment thread .github/workflows/ci-image.yml Outdated
Comment thread docs/workflows/overview.md
Review findings on #11205:

- The change detector failed OPEN: a git diff that could not be computed
  produced empty output, which read as "nothing changed" and skipped the
  gate — a check that passes by failing. A PR or merge-group diff that
  fails now stops the job (fetch-depth 0 guarantees those refs), and a
  push with no reachable parent builds rather than skips.
- README.md and a root .dockerignore are build-context inputs too: the
  builder copies README.md next to pyproject.toml before `uv sync`, and
  a root .dockerignore decides what a `context: .` build can see at all.
  Both now trigger the job.
- The workflow inventory in agentic/docs/project-guide.md carries the
  same list as docs/workflows/overview.md; ci-image.yml was missing
  there.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 21:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Push detection can miss earlier commits, and global Hadolint ignores allow future violations of the supposedly gated rules.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread .github/workflows/ci-image.yml Outdated
Comment thread .github/workflows/ci-image.yml
Comment thread CHANGELOG.md Outdated
… cover

Second review round on #11205:

- A push can carry several commits; HEAD~1..HEAD missed an earlier one
  touching api/ under an unrelated tip commit. The push branch now diffs
  github.event.before..after, with a new branch (before = zeroes) and an
  unreachable old tip after a force-push both building rather than
  skipping. The four branches are a case statement now, one per event.
- hadolint's `ignore` is file-wide, so the comment claiming "a NEW
  warning blocks" was too strong: a second DL3013/DL3008/DL3025 anywhere
  in api/Dockerfile is suppressed too. The comment now says exactly what
  is covered and names the line-scoped follow-up (`# hadolint
  ignore=<code>` next to each instruction), which is a Dockerfile edit
  this change does not make.
- Changelog: all-caps emphasis replaced with sentence case, and the entry
  carries its PR reference like every other bullet.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Copilot AI review requested due to automatic review settings September 2, 2026 21:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The change introduces branch-gating CI infrastructure and therefore warrants final human review despite successful workflow runs.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@MarkusNeusinger
MarkusNeusinger merged commit e842261 into main Sep 2, 2026
9 checks passed
@MarkusNeusinger
MarkusNeusinger deleted the infra/ci-image-smoke branch September 2, 2026 21:50
MarkusNeusinger added a commit that referenced this pull request Sep 2, 2026
Merge resolves the CHANGELOG conflict by keeping both Added entries (#11205
landed on main while this branch was open).

Copilot review, two findings.

1. /debug/cache/invalidate is exempt from the gate on the grounds that it has
   its own lock - so that lock has to be as cheap to fail as the gate is, and
   it was not: it compared the token as str, and it is the one endpoint
   reachable on the direct run.app URL, so a non-ASCII X-Cache-Token turned a
   401 into an unhandled, logged 500. X-Admin-Token had the same comparison.
   Rather than fixing three call sites separately, the byte-wise compare moves
   into api/secret_compare.py and all three use it; a comparator that is
   correct in two places out of three is exactly what nobody notices.

2. --set-env-vars is destructive over the revision environment, where a
   secret-backed variable lives alongside a literal one - so it was a second
   way for a pipeline deploy to drop the hand-attached ORIGIN_SECRET and
   disarm the gate, undoing what --update-secrets had just bought. Both flags
   are additive now.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
MarkusNeusinger added a commit that referenced this pull request Sep 2, 2026
Merge resolves the CHANGELOG conflict by keeping both Added entries (#11205
and #11206 landed on main while this branch was open).

Copilot review: _candidate_paths treated a whole code span as one token, so
'.claude/commands/ -> ../agentic/commands/' - a sentence about two paths -
was not path-shaped and went unchecked as a whole. Spans are now split on
whitespace before the shape test; fragments that are not path-shaped (the
arrow, a command word, a flag) drop out, which is what makes the split safe.
A '../'-relative fragment is skipped rather than resolved against the
repository root, where it would mean something else entirely.

That leaves the arrow's meaning unpinned, and the sharper failure is the one
where somebody replaces the symlink with a real directory: both ends still
exist, the guide still reads true, and commands written on either side
quietly stop matching the other. So the symlink gets its own test, which
follows the link instead of matching its text.

(The specific example was in fact already covered, because
'.claude/commands/' appears standalone later in the same sentence - but the
hole in the heuristic was real.)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PBQdMbboxo59sSThGSbfke
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants