[JSv4]

Summary

Closes #1505. Addresses all six review observations on PR #1500 (the slim-production-image change) without altering deployed runtime semantics.

#	Observation	Resolution
1	locale/ excluded from build context — does anything need compilemessages?	Verified safe. locale/ contains only README.rst (no .po/.mo), and grep -r compilemessages across *.py, *.sh, Dockerfile*, *.yml/yaml finds zero invocations. Documented why exclusion is safe in .dockerignore and clarified gettext's role as a build hook in the runtime apt comment.
2	load: true + push: true co-existence in release workflow	Restructured. Django path is now build+load → size-budget gate → docker push <tag> per tag. Non-Django images keep the original combined push: true, load: false path.
3	ffmpeg removal — verify no other consumers	Verified absent. grep -r "ffmpeg" across opencontractserver/, compose/, config/, frontend/, and the workflow tree returns nothing. No code change required; verification recorded in CHANGELOG.
4	Inconsistent spaCy download method between local and production	Mirrored to pinned URLs. Production now downloads en_core_web_sm-3.8.0 and en_core_web_lg-3.8.0 wheels in the build stage with the same wget retry semantics as compose/local/django/Dockerfile, into the existing /usr/src/app/wheels cache. The runtime stage installs them via the existing pip install --no-index --find-links=/wheels/ /wheels/* step (no per-package change) and runs a spacy.load(...) smoke test. wget is added to the build stage only — never reaches the runtime image.
5	Python version not pinned to a patch	Pinned to 3.11.15-slim-bookworm in both compose/production/django/Dockerfile and compose/local/django/Dockerfile (verified available on Docker Hub). Rebuilds on different dates can no longer silently pick up a new CPython point release.
6	brotli>=1.1.0 floor rationale	Documented in requirements/base.txt. 1.1.0 is the current major-version line under active upstream maintenance; the underlying _brotli C-extension API that httpx depends on has been stable since 1.0.x, so the floor can be lowered safely if a transitive constraint demands it.

Files changed

• .dockerignore — comment block above the locale/ line documenting why exclusion is safe.
• .github/workflows/docker-build-release.yml — split Django path into build+load / push steps; non-Django path unchanged.
• CHANGELOG.md — new Changed entry under [Unreleased] listing all six items.
• compose/production/django/Dockerfile — pin Python, add wget to build stage, fetch pinned spaCy wheels, replace runtime spacy download with a spacy.load(...) smoke test, clarify gettext runtime comment.
• compose/local/django/Dockerfile — pin Python (kept in sync with production).
• requirements/base.txt — extend the brotli>=1.1.0 comment with explicit rationale.

Test plan

• docker compose -f production.yml build django builds against python:3.11.15-slim-bookworm end-to-end and the spaCy spacy.load(...) smoke test passes.
• docker compose -f local.yml build django still builds (Python pin is the only change here).
• Release workflow on a release-tag (or workflow_dispatch dry-run) shows the Django image building once with load: true, the size-budget gate running against the loaded image, then docker push succeeding for each tag.
• Backend test suite still green (no Python/Django code paths changed).

---

Generated by Claude Code

[claude]

Code Review — PR #1524 (Docker image hardening follow-ups)

Overall this is a clean, well-documented set of housekeeping changes that address all six items from the prior review without touching runtime semantics. The comments throughout are unusually good — they explain why, not just what. A few issues worth addressing before merge:

---

Issues

1. id: build removal may silently break the size-budget gate

The original combined step carried id: build. That ID was removed when the step was split. If the existing size-gate step (unchanged in this diff) references ${{ steps.build.outputs.imageid }} to identify the image to inspect, it will silently evaluate to an empty string and the gate will either error or pass vacuously. Please verify the size-gate step uses ${{ steps.meta.outputs.tags }} (or a hard-coded tag) to identify the image rather than steps.build.outputs.imageid. If it does use the old ID, the Django build step needs either id: build restored or the gate updated to reference the new step name.

2. No checksum verification on fetched spaCy wheels (supply-chain risk)

RUN wget ... https://github.com/explosion/spacy-models/releases/download/en_core_web_sm-3.8.0/en_core_web_sm-3.8.0-py3-none-any.whl \
    && wget ... https://github.com/explosion/spacy-models/releases/download/en_core_web_lg-3.8.0/en_core_web_lg-3.8.0-py3-none-any.whl

Pinning to an explicit release URL is a good improvement over python -m spacy download, but without a SHA-256 check the build is still vulnerable to a GitHub release being retroactively modified (or a MITM on the CDN). The local Dockerfile has the same gap. Adding a verification step costs two lines:

echo "<sha256>  /usr/src/app/wheels/en_core_web_sm-3.8.0-py3-none-any.whl" | sha256sum -c - \
&& echo "<sha256>  /usr/src/app/wheels/en_core_web_lg-3.8.0-py3-none-any.whl" | sha256sum -c -

The hashes can be obtained once with sha256sum on the downloaded files. Strictly speaking this is a pre-existing gap in compose/local/django/Dockerfile, but since this PR explicitly frames the two paths as being in sync it is a good moment to close it in both.

---

Suggestions (non-blocking)

3. Python version sync relies on manual discipline

Both Dockerfiles now carry a "keep this in sync" comment pointing at each other. That comment is easy to miss on a future bump. A lightweight alternative: define the version once in a shared .env file (e.g. PYTHON_VERSION=3.11.15-slim-bookworm) and have both Dockerfiles source it via --build-arg. Not essential for this PR but worth considering before the next Python bump.

4. Long inline comment in requirements/base.txt

The brotli line is now ~200 characters. Most editors/linters will flag it and it is awkward to read in a diff. The rationale is worth keeping — consider moving it to a two-line comment block above the requirement instead:

# Floor 1.1.0: current major-version line under active maintenance (released 2023-09).
# The _brotli C-extension API httpx looks up is stable since 1.0.x; loosening is safe if a transitive constraint demands it.
brotli>=1.1.0  # decoder for httpx Content-Encoding: br responses

---

What's working well

• The set -euo pipefail + IFS= read -r pattern in the push loop is correct and handles edge cases (empty lines, tags with spaces) cleanly.
• Replacing python -m spacy download with a spacy.load(...) smoke test is strictly better — it catches missing-or-corrupt models at build time without adding download logic to the runtime stage.
• wget correctly scoped to the build stage only.
• CHANGELOG entry is detailed and includes file paths and issue references per project conventions.
• The load: true, push: false → push-per-tag split is the right architectural decision for Buildx compatibility.

The two issues above (ID removal risk, missing checksums) are worth resolving before merge; the suggestions are optional polish.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

Code Review: PR #1524 — Slim-image follow-ups

Overall this is a well-scoped, well-documented cleanup PR. Every change addresses a concrete observation from PR #1500 and the rationale is clearly recorded. A few items worth flagging:

---

Security: SHA-256 hash provenance

The new SHA-256 checks are the right call — pinning a URL alone is insufficient against a retroactive release modification. However, neither the PR description nor the Dockerfiles document where these hashes were sourced (e.g., the release page checksum file, a local sha256sum run after an initial download, etc.). For supply-chain auditability a single-line comment is enough:

# Hashes verified against the upstream release on 2026-05-05:
# https://github.com/explosion/spacy-models/releases/tag/en_core_web_sm-3.8.0

There's also a subtle discrepancy worth double-checking: the en_core_web_lg hash appears 63 hex characters in the diff as rendered:

293e9547a655b25499198ab15a525b05b9407a75f10255e405e8c3854329ab63

Please verify the raw file character count — SHA-256 must be exactly 64 hex chars. If it's truly 63, the sha256sum -c check would silently fail on some implementations rather than erroring loudly, defeating the purpose.

---

Workflow: size-budget gate reference after id: build removal

The step previously had id: build; this PR removes it. The CHANGELOG notes that the gate at line 93 uses ${{ steps.meta.outputs.tags }} rather than steps.build.outputs.imageid, so this is claimed safe. But the gate step's code is unchanged (not shown in the diff), and the assertion can't be verified from this PR alone.

Could you paste the relevant portion of the size-budget gate step in the PR description or as an inline comment so reviewers can confirm the image reference? If the gate did reference steps.build.outputs.imageid, removing id: build would cause the gate to silently pass (empty string resolves to no image), making the budget check a no-op.

---

Workflow: docker push multi-line tag loop

The push loop looks correct. One minor robustness note — ${{ steps.meta.outputs.tags }} is injected directly into a run: heredoc, which is fine for standard Docker tags (alphanumeric, :, /). No quoting concern there.

One thing to confirm: does docker/login-action run before this job's matrix starts (not just before the build-push-action step)? The docker push shell step relies on the same daemon credentials but outside the action — if the login is scoped to a prior step in the same job (which it almost certainly is), this is fine.

---

requirements/base.txt: comment line length

The new brotli comment is ~170 characters on a single line, which is harder to read in side-by-side diffs. Since requirements.txt isn't linted for line length this is purely cosmetic, but splitting into two comment lines would improve readability:

brotli>=1.1.0  # decoder for httpx Content-Encoding: br responses (e.g. OpenAI API, VCR cassettes).
               # Floor is 1.1.0 — active upstream major-version line; _brotli C-extension API stable since 1.0.x.

---

Minor positives worth calling out

• Python pinned to 3.11.15-slim-bookworm in both Dockerfiles is correct and the sync comment is helpful.
• The wget-retry semantics are now consistent between local and production, and wget is correctly kept out of the runtime stage.
• The cast(Optional[type[BaseEmbedder]], embedder_class) in embeddings.py is the right way to handle the typing boundary; no runtime change, clean.
• The workflow split (build+load → size gate → per-tag push) is the correct architecture for this constraint.
• ffmpeg verification-by-grep is an appropriate documentation-only response — no code change needed when the package was already removed.

---

Summary: Approve conditional on verifying the en_core_web_lg hash character count and confirming the size-budget gate step's image reference. Everything else is well-reasoned and well-documented.

[claude]

Code Review — PR #1524 (Docker image hardening follow-ups)

Note: this PR is already merged. The review is posted for the record and to surface any items worth tracking for future work.

Overview

This is a clean, focused follow-up to PR #1500. It addresses six review observations about the slim production image without changing any deployed runtime semantics. All changes are infrastructure/DevOps (Dockerfiles, CI workflow, comments, and changelog).

---

Positives

• Python patch pinning (3.11.15-slim-bookworm) is the right call. Floating minor-version aliases are a common source of silent regressions on CI machines that run on different days.
• spaCy model install unification (both local and production now use wget + pinned URL + smoke test) is good for parity. The existing approach in local was already solid; mirroring it to production closes a real reproducibility gap.
• SHA-256 wheel verification is a meaningful supply-chain improvement — it was not part of the original six observations but is a net win. Hardcoded hashes in source control create a verifiable audit trail.
• Workflow split (build+load → size gate → push per tag) is the correct fix for the load: true + push: true incompatibility on multi-platform builds with older Buildx versions. The inline comment explaining why this was split (rather than just what was changed) is especially helpful.
• Comments throughout (locale/, gettext, brotli rationale) are well-targeted: they explain why a decision was made, not just what it does.

---

Issues & Suggestions

1. SHA-256 hashes — no provenance trail in the PR (minor)

The checksums for en_core_web_sm-3.8.0 (1932429d…) and en_core_web_lg-3.8.0 (293e9547…) are 64-hex-character values (verified by character count) and are consistently applied across both Dockerfiles — that's good. However, the PR description and CHANGELOG don't record how the hashes were obtained (e.g., sha256sum of a locally-downloaded file, or from a PyPI/GitHub release manifest). A one-line "computed via sha256sum of the wheel from the GitHub release page" note in the CHANGELOG entry or a Dockerfile comment would let a future maintainer reproduce the verification without guessing. This is minor given that the hashes are committed and will be checked by CI, but it improves the audit trail.

2. wget -P vs -O asymmetry between local and production (informational)

• Production Dockerfile: wget -P /usr/src/app/wheels (saves directly to the wheel cache directory, filename derived from URL)
• Local Dockerfile: wget -O /tmp/<name>.whl (explicit output path, then pip install, then rm)

Both are correct for their contexts (production uses the --no-index cache; local installs directly), but the asymmetry is something to keep in mind if either Dockerfile is ever refactored independently — the sha256sum path in the echo line would need updating.

3. docker push loop — shell injection surface (low risk, informational)

done <<< "${{ steps.meta.outputs.tags }}"

The steps.meta.outputs.tags value is interpolated directly into the shell heredoc. In practice the tag strings are registry/image:semver format with no shell-special characters, and docker/metadata-action controls the content, so the actual risk is negligible. Still, for defense-in-depth on future workflow changes, a slightly safer pattern is to iterate using jq on the JSON output (steps.meta.outputs.json) or to pipe through xargs -I{} docker push {}. Not a blocker — just worth knowing for the next workflow touch.

4. Size-budget gate step ID reference (verified, noting for clarity)

The CHANGELOG confirms that the gate at line 93 uses steps.meta.outputs.tags (not the now-removed steps.build.outputs.imageid). It would be good to add an inline comment on that gate step explicitly noting this dependency so a future refactor of the meta step doesn't silently break it.

---

Test Plan Gaps

The PR test plan lists manual build verification steps (docker compose build) but doesn't include a step to confirm the SHA-256 gate rejects a corrupted wheel. A single negative test (e.g., printf 'deadbeef' >> wheel && sha256sum -c - fails non-zero) in a local build script or CI smoke-test would give confidence that the check is wired correctly and not just vacuously passing. Low priority given the sha256sum -c semantics are well-known, but worth adding as a footnote in the test plan for the next person who bumps a model version.

---

Summary

Area	Assessment
Correctness	✅ All changes are safe and correct
Security	✅ Supply-chain improvement (SHA-256); minor workflow injection surface (informational only)
Reproducibility	✅ Python pin + spaCy pin + hash check all improve build determinism
Documentation	✅ Comments explain the why; provenance of hashes could be noted
Test coverage	✅ Backend test suite unaffected; hash rejection path untested (low priority)

Overall this is a solid hardening PR. The issues noted above are minor and informational — nothing here warrants a re-review or follow-up PR unless the team wants to add the hash provenance comment.