Show real app version in container and add cloud favicon #58
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Reference
marvin8/yunjin!58
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/container-version-and-favicon"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Two user-visible fixes, both verified.
Real version in the container (Closes #56)
The image footer showed the placeholder
Yunjin v0.0.0+unknown: the builder stage ranuv sync --no-install-project, so noyunjinpackage metadata existed anywhere in the image andimportlib.metadatafell back. The builder stage now installs the project, so the version frompyproject.tomllands in the venv and the footer matches a source-checkout run. The code still runs from/app/srcviaPYTHONPATH(which takes precedence over site-packages), so the alembic.ini-relativeinit_dbbehaviour is unchanged. Verified with a scratch venv built both ways:0.0.0+unknownbefore,2026.9.26.1after, module still resolving fromsrc/.Cloud favicon (Closes #57)
Adds an SVG favicon rendering the same cloud emoji as the page header, linked from
base.html, plus a/favicon.icoredirect so browsers no longer log a 404 on every page. Covered by three new tests intests/test_web.py(written failing-first).CI smoke test
The
containerjob now boots the built image and asserts the footer shows thepyproject.tomlversion, the page head links the favicon, and/favicon.icoresolves — the only automated check at the layer where the version regressed.Closes #56
Closes #57
WuMing
Found 10 issue(s). See inline comments below.
@ -86,1 +86,4 @@.- name: Smoke-test built imagerun: |EXPECTED_VERSION=$(grep -oP '^version = "\K[^"]+' pyproject.toml)config [MEDIUM]
The expected version is extracted with GNU-only
grep -P/\Kand is not validated. If the runner's grep lacks PCRE support or the TOML formatting changes, this can fail for the wrong reason or (if not caught byset -e) produce an empty value that makesgrep "Yunjin v${EXPECTED_VERSION}"match any placeholder. Use a portable extractor such assed -n 's/^version = "\([^"]*\)"/\1/p' pyproject.tomland asserttest -n "$EXPECTED_VERSION"before using it.@ -87,0 +87,4 @@- name: Smoke-test built imagerun: |EXPECTED_VERSION=$(grep -oP '^version = "\K[^"]+' pyproject.toml)podman run -d --name yunjin-smoke -p 18080:8080 \config [MEDIUM]
Publishing with
-p 18080:8080binds the smoke-test app to all host interfaces (0.0.0.0) rather than localhost, so other machines on the runner network can reach it. Since the test only callshttp://localhost:18080/, bind to-p 127.0.0.1:18080:8080instead.security [LOW]
The smoke-test container publishes port 18080 on all interfaces (0.0.0.0) rather than loopback only. On a shared CI runner this can expose the test application beyond the build host. Additionally, several failure paths exit before the
podman rmcleanup, leaving the container running and exposed. Bind to loopback (-p 127.0.0.1:18080:8080) and use--rmor a trap to remove the container on exit. (A05)@ -87,0 +102,4 @@podman logs yunjin-smoke || trueexit 1figrep -q "Yunjin v${EXPECTED_VERSION}" /tmp/smoke-page.html || {config [LOW]
The footer assertion uses
grep -qwithout-F, so the dots in the version are treated as regex wildcards and the match is only a substring. A footer such asYunjin v2026.9.26.10would pass for expected version2026.9.26.1. Usegrep -Fqand add a trailing boundary/known suffix to assert the exact version string.@ -87,0 +114,4 @@echo "/favicon.ico did not resolve to the SVG icon" >&2exit 1}podman rm -f yunjin-smokeconfig [HIGH]
podman rm -f yunjin-smokeruns only after every assertion succeeds. Any failinggrep/curlpath callsexit 1and skips cleanup, leaving the container running and port 18080 bound; the next CI run on the same runner will then fail atpodman run --name yunjin-smokewith a name/port conflict. Add atrap 'podman rm -f yunjin-smoke' EXITafter starting the container, and remove any stale container beforepodman run.@ -14,3 +16,2 @@WORKDIR /buildCOPY pyproject.toml uv.lock ./RUN uv sync --frozen --no-dev --no-install-projectCOPY pyproject.toml uv.lock README.md ./shell [LOW]
Installing the project means the build backend now reads pyproject.toml's file references (readme, license-files, version files, etc.). Only README.md is copied, so the build succeeds only if pyproject.toml references README.md and nothing else; a
license = {file = "LICENSE"}or a dynamically read version file will makeuv syncfail with a missing-file error at build time. Copy the complete set of metadata files the project declares, or build/install a wheel prepared outside the container.@ -15,2 +17,2 @@COPY pyproject.toml uv.lock ./RUN uv sync --frozen --no-dev --no-install-projectCOPY pyproject.toml uv.lock README.md ./COPY src/ ./src/shell [MEDIUM]
The source tree is copied before
uv sync, so any change tosrc/invalidates the dependency-install layer and forces a full resolve/install of every pinned dependency on each rebuild. Restore layer caching by installing dependencies first, then the project, e.g.:COPY pyproject.toml uv.lock README.md ./+RUN uv sync --frozen --no-dev --no-install-project, thenCOPY src/ ./src/+RUN uv sync --frozen --no-dev --no-editable(or build a wheel anduv pip install --no-depsit). That keeps the version metadata while making source-only changes cheap to rebuild.@ -16,1 +17,3 @@RUN uv sync --frozen --no-dev --no-install-projectCOPY pyproject.toml uv.lock README.md ./COPY src/ ./src/RUN uv sync --frozen --no-devshell [MEDIUM]
uv syncinstalls the root project in editable mode by default, which bakes the build-time path (/build/src) into the venv via a .pth/meta-path finder. Because the venv is copied to /app/.venv in the runtime stage while the sources live at /app/src, that install is left dangling and only appears to work because PYTHONPATH is evaluated before the editable shim. Add--no-editableso a real copy of the package (and its dist-info metadata) is written into the venv, making imports and console-script entry points self-contained regardless of PYTHONPATH.@ -138,0 +143,4 @@@app.route("/favicon.ico")def favicon() -> ResponseReturnValue:"""Redirect legacy favicon requests to the SVG icon."""return redirect(url_for("static", filename="favicon.svg"))code [HIGH]
The new route redirects to
/static/favicon.svg, but the diff does not addsrc/yunjin/static/favicon.svg. As shown,/favicon.icowill redirect to a 404 and the new tests/CI smoke test will fail. Add the SVG asset to the PR.@ -1784,0 +1793,4 @@assert response.status_code == 200html = response.data.decode()assert 'rel="icon"' in htmlassert "/static/favicon.svg" in htmlcode [HIGH]
This test asserts the rendered page contains
/static/favicon.svg, but the diff does not updatebase.htmlto include the<link rel="icon">tag. Unlessbase.htmlis changed elsewhere, the test will fail.WuMing wave 1 — responses
Context: the first
containerrun failed becausepodman runcannot work on zhuiri — rootless podman there has no subuid/subgid range, so container storage creation fails (lchown ... invalid argument) for any image with non-root UIDs. The smoke test now verifies the image through buildah storage ops (the same machinery the build step already used) instead of booting it: mount the image, assert the venvdist-infoversion equals thepyproject.tomlversion, and assert the favicon is shipped. That rework plus the Containerfile fixes below address most of this wave.✅ Fixed — smoke test robustness and hygiene
#3932 · #3933 · #3934 · #3935 · #3939
Fixed/superseded in commit
5b0a621: the version extractor issedwith an explicit non-empty check (portable, nogrep -P);podman run, the published port and the footer grep are gone entirely — the version assertion is now exact string equality between the image'sdist-infoMETADATA andpyproject.toml, and the working container is removed via anEXITtrap plus explicitumount/rm. (#3939 carries the samewuming:sha256as #3933, so it is the same finding; answered once here.)✅ Fixed — Containerfile build
#3936 · #3937
Fixed in commit
6cd1ec1: two-phaseuv sync— dependencies install beforesrc/is copied (layer caching restored), then the project installs with--no-editable, writing a real copy plus itsdist-infointo the venv instead of a dangling/buildeditable shim.🔴 Not actioned — favicon.svg missing from the diff
#3930
False positive:
src/yunjin/web/static/favicon.svgis added in commit5461c0f(note theweb/path segment). Thechecksjob on this PR runs the three new favicon tests on Python 3.12–3.14 — all green.🔴 Not actioned — base.html missing the icon link
#3931
False positive: the
<link rel="icon" ...>tag was added tobase.htmlin the same commit5461c0f; the rendered-page test asserting it passes in the greenchecksjob.🔴 Not actioned — copy the complete set of metadata files
#3938
pyproject.tomlreferences exactly one file:readme = "README.md". The licence is inline (license = {text = ...}), the version is static — there are no other file references to copy. If that ever changes,uv syncfails loudly at build time (as the comment predicts), which the container job would catch immediately; speculative pre-copying would just rot.WuMing
Found 5 issue(s). See inline comments below.
@ -13,3 +11,3 @@WORKDIR /buildCOPY pyproject.toml uv.lock ./COPY pyproject.toml uv.lock README.md ./shell [LOW]
README.mdis copied into the dependency-install layer (line 22), but it is only needed by the project build at line 24 (becausepyproject.tomldeclares it as the readme). As a result, any edit to the README invalidates the expensive dependency install, which contradicts the stated goal on lines 14–15 of keeping documentation/source changes out of that layer. Move the readme into the later stage, e.g.COPY src/ README.md ./right beforeRUN uv sync --frozen --no-dev --no-editable, so only the project-install layer is invalidated.@ -16,1 +21,4 @@# relative to the source tree root.RUN uv sync --frozen --no-dev --no-install-projectCOPY src/ ./src/RUN uv sync --frozen --no-dev --no-editableshell [MEDIUM]
Installing the project non-editable copies the whole
yunjinpackage into/app/.venv/lib/python3.12/site-packages, so the runtime image now contains two divergent copies of the application code (site-packages and/app/src). Correct behaviour depends entirely onPYTHONPATHwinning over site-packages for every process that touches the code. Any process that does not inherit that variable (an exec'd child, a subprocess launched with a scrubbed env, a console-script invocation in a different context) will silently import the stale site-packages copy and reproduce exactly the class of divergence this change is meant to remove. SetPYTHONPATH=/app/srcviaENVin the runtime stage rather than only in the entrypoint/CMD so it is inherited unconditionally, and consider adding a startup assertion (or test) thatyunjin.__file__resolves under/app/src.@ -14,1 +14,4 @@The build installs the project's package metadata (version from`pyproject.toml`) alongside the locked dependencies, so the web footerreports the real application version — e.g. `Yunjin v2026.9.26.1` — thedocs [LOW]
The example version
Yunjin v2026.9.26.1is a concrete, date-based version pinned into prose. It will become stale (and potentially misleading) as soon as the next release bumpspyproject.toml, and it also reads as a future-dated version for the current release schedule. Prefer a neutral placeholder such asYunjin v<version>or a note that the value trackspyproject.toml, so the doc doesn't require editing on every version bump.@ -1784,0 +1788,4 @@def test_base_template_links_svg_favicon(self, client: Any) -> None:"""Test every page advertises the SVG favicon in the head."""response = client.get("/")code [LOW]
The docstring says 'every page advertises the SVG favicon', but only '/' is checked. If another template omits the link, this test will not catch it. Check additional pages or narrow the docstring.
@ -1784,0 +1797,4 @@def test_svg_favicon_is_served(self, client: Any) -> None:"""Test the SVG icon file is served from static."""response = client.get("/static/favicon.svg")code [HIGH]
The test requests /static/favicon.svg, but the SVG asset is not added anywhere in the diff. The new /favicon.ico redirect will also point at a 404 until this file exists and is packaged with the app.
WuMing wave 2 — responses
🔴 Duplicate of wave-1 finding — favicon.svg "not in the diff"
#3943
Identical to #3930, answered above:
src/yunjin/web/static/favicon.svgis added in commit5461c0f, and the favicon tests pass in the greenchecksjob on this PR. The comment reappeared only because the diff shifted; nothing to do.✅ Fixed — docstring overclaims page coverage
#3944
Fixed in commit
43e8697: the docstring now says what the test exercises — the reader index rendered through the shared base template — instead of "every page".✅ Fixed — concrete version pinned in docs prose
#3945
Fixed in commit
43e8697:docs/container.mdnow uses a neutralYunjin v<version>placeholder.✅ Actioned — import resolution must come from /app/src
#3946
One premise is already true:
PYTHONPATH=/app/srcis set viaENVin the runtime stage (it has been since the image existed), so every process in the container inherits it unconditionally — nothing is set in the entrypoint. What this wave added is your suggested detection: the smoke test now runspython -c "import yunjin; assert yunjin.__file__ == '/app/src/yunjin/__init__.py'"inside the image (buildah, chroot isolation) so a divergence would fail CI loudly. Commit43e8697.✅ Fixed — README.md invalidating the dependency layer
#3947
Fixed in commit
43e8697:README.mdmoved out of the dependency-installCOPY; it now lands withsrc/in the project-install layer, so README edits only invalidate the cheap layer.WuMing
Found 3 issue(s). See inline comments below.
@ -86,1 +86,4 @@.- name: Smoke-test built imagerun: |EXPECTED_VERSION=$(sed -n 's/^version = "\([^"]*\)"/\1/p' pyproject.toml | head -1)config [MEDIUM]
The expected version is scraped with
sed -n 's/^version = "..."/.../p' pyproject.toml | head -1, which is not scoped to the[project]table — it returns the first line in the whole file that literally starts withversion = ". Any other table that carries aversionkey (e.g.[tool.commitizen], VCS/release tooling) or a future reordering of the file would silently produce a differentEXPECTED_VERSION, so the smoke test would either fail spuriously or, worse, compare two wrong values and pass. Parse the TOML instead of pattern-matching, e.g.EXPECTED_VERSION=$(python -c "import tomllib;print(tomllib.load(open('pyproject.toml','rb'))['project']['version'])")(or restrict the sed to the[project]section).@ -87,0 +109,4 @@# Cloud favicon shipped with the app.favicon="$rootfs/app/src/yunjin/web/static/favicon.svg"test -f "$favicon" || { echo "favicon.svg missing from image" >&2; exit 1; }grep -q '<svg' "$favicon" || { echo "favicon.svg is not SVG" >&2; exit 1; }config [LOW]
The favicon assertions only prove that a file exists at that path in the image and that it contains the substring
<svg(which any XML/HTML fragment would satisfy). They do not exercise the two things the PR claims the job verifies — thatbase.htmlactually links/favicon.svgand that/favicon.icoresolves instead of 404ing — so a broken href or a missing/misconfigured redirect would still ship green. Add a check for the link tag in the template (grep the href in the baked/app/src/yunjin/web/templates/base.html) and assert the redirect route exists (e.g. aTestClientcall in the smoke test, or a grep of the router registration).@ -87,0 +112,4 @@grep -q '<svg' "$favicon" || { echo "favicon.svg is not SVG" >&2; exit 1; }# The app must resolve from /app/src (PYTHONPATH); the venv copy of# the package exists to carry version metadata, never to be imported.buildah run --isolation chroot "$ctr" \config [LOW]
Lines 94–98 state that
podman runcannot work on this runner because rootless storage/user-namespace mapping for the image's non-root UIDs is unavailable, and conclude that the container must not be booted — yet this line boots it anyway viabuildah run, which performs the same container process setup and UID mapping that was reported to fail. Either the rationale is wrong or this command will fail in CI. Confirmbuildah run --isolation chrootactually executes on zhuiri (e.g. wrap it inbuildah unshare) and update the comment, otherwise the smoke step will break the pipeline.WuMing wave 3 — responses
Context: run 121 failed before
buildah runever executed — my previous commit broke the build itself:COPY src/ README.md ./copies the contents ofsrc/, flatteningsrc/yunjinto/build/yunjin, souv_buildfound no module atsrc/yunjin/__init__.py. Fixed in commit95135b2by copying the tree and the readme as separateCOPYinstructions. All three findings below are also actioned in the same commit.✅ Fixed — buildah run rationale / will it execute?
#3954
The comment now states precisely why chroot isolation works where
podman rundoes not:podman runneeds a user namespace to map the image's non-root UIDs (none available rootless on zhuiri), whilebuildahwith--isolation chrootnever creates a user namespace. The next CI run is the first actual execution ofbuildah runon zhuiri — if it disproves the rationale, the pipeline says so loudly and the assertion falls back to filesystem checks.✅ Fixed — version extraction not scoped to [project]
#3952
The extraction is now an
awksnippet that only reads aversion =key inside the[project]table (verified againstpyproject.toml→2026.9.26.1), with the non-empty check retained. No Python dependency added on the runner.✅ Fixed — favicon assertions don't cover the wiring
#3953
The smoke step now also greps the baked
base.htmlfor thefavicon.svglink and the bakedweb/__init__.pyfor thefavicon.icoredirect route. End-to-end request behaviour (template render, 302 chain) stays covered by the pytest suite in thechecksjob; the container job now verifies all of it shipped inside the image.WuMing
Found 7 issue(s). See inline comments below.
@ -86,1 +86,4 @@.- name: Smoke-test built imagerun: |EXPECTED_VERSION=$(awk '/^\[project\]/{inproj=1;next} /^\[/{inproj=0} inproj && /^version = /{gsub(/"/,"",$3); print $3; exit}' pyproject.toml)config [LOW]
This hand-rolled awk parser duplicates TOML parsing in CI and depends on
$3being the version token. It silently yields an empty string for valid TOML written without spaces around=(version="2026.9.26.1") and returns the quotes for single-quoted values (version = '2026.9.26.1'), in both cases failing the later comparison for a reason unrelated to the image. Prefer a real parser, e.g.python -c "import tomllib,sys;print(tomllib.load(open('pyproject.toml','rb'))['project']['version'])"(the runner already builds Python images), or reuse the project's own version source.@ -87,0 +97,4 @@# with chroot isolation never creates a user namespace, so the# storage ops and in-chroot exec below work where podman fails.ctr=$(buildah from --isolation chroot registry.marvin8.zone/marvin8/yunjin:ci)trap 'buildah rm -f "$ctr" >/dev/null 2>&1 || :' EXITconfig [LOW]
The EXIT trap only removes the container; it does not unmount the rootfs created by
buildah mounton line 101. If any command between the mount and line 125 fails, the trap fires while the container is still mounted, leaving a stale mount (andbuildah rm -fon a mounted container can itself fail, so the|| :hides cleanup failure). Make the trap alsobuildah umountbeforebuildah rm, e.g.trap 'buildah umount "$ctr" >/dev/null 2>&1 || :; buildah rm -f "$ctr" >/dev/null 2>&1 || :' EXIT.@ -87,0 +100,4 @@trap 'buildah rm -f "$ctr" >/dev/null 2>&1 || :' EXITrootfs=$(buildah mount "$ctr")# Version metadata baked into the venv — the footer's source.installed=$(sed -n 's/^Version: \(.*\)$/\1/p' \config [LOW]
The version is read via a glob that can match more than one distribution (e.g. a stale
yunjin-0.0.0+unknown.dist-infoleft next to the real one) andhead -1then silently trusts whicheversedemits first; conversely, if the glob matches nothing,sedfails on a literal path and the only diagnosis is a confusingimage metadata version '' != ...message. Pick the newest/matching metadata explicitly (or query importlib.metadata inside the container) and emit a clear error when the dist-info path is not found.@ -87,0 +102,4 @@# Version metadata baked into the venv — the footer's source.installed=$(sed -n 's/^Version: \(.*\)$/\1/p' \"$rootfs"/app/.venv/lib/python3.*/site-packages/yunjin-*.dist-info/METADATA | head -1)if [ "$installed" != "$EXPECTED_VERSION" ]; thenconfig [MEDIUM]
This compares the raw string from pyproject.toml against the Version field of the installed dist-info METADATA. dist-info versions are PEP 440-normalized (e.g. a pyproject version of
1.0.0-rc1or1.0.0+localis stored/rendered as1.0.0rc1), so this equality check will start producing false failures as soon as the project version is not already normalized. Compare derived values instead, e.g. read the version inside the container withbuildah run --isolation chroot "$ctr" python -c "import importlib.metadata as m; print(m.version('yunjin'))"and compare that to a value normalized the same way (or use packaging.version.parse on both sides).@ -87,0 +124,4 @@env PYTHONPATH=/app/src python -c "import yunjin; assert yunjin.__file__ == '/app/src/yunjin/__init__.py', yunjin.__file__"buildah umount "$ctr"buildah rm "$ctr"echo "image smoke test OK: yunjin ${EXPECTED_VERSION} with favicon"config [MEDIUM]
The step never starts the application, so it does not verify the two user-visible behaviours described in the PR: the rendered footer text and that
/favicon.icoactually resolves. The checks are static file/metadata greps (grep 'favicon.ico'in init.py only proves the string exists somewhere in the module). Add a runtime assertion — boot the image (orbuildah runthe app with the test DB) andcurl -fsS localhost:PORT/expecting the version string in the body, pluscurl -sI localhost:PORT/favicon.icoexpecting a 3xx to favicon.svg.@ -87,0 +121,4 @@# The app must resolve from /app/src (PYTHONPATH); the venv copy of# the package exists to carry version metadata, never to be imported.buildah run --isolation chroot "$ctr" \env PYTHONPATH=/app/src python -c "import yunjin; assert yunjin.__file__ == '/app/src/yunjin/__init__.py', yunjin.__file__"config [MEDIUM]
The check explicitly sets PYTHONPATH=/app/src, so it can never detect the regression it is meant to guard: if the Containerfile forgets
ENV PYTHONPATH=/app/src(or sets a different value), the app at runtime would import the freshly-installed copy from site-packages and the footer could drift from /app/src. Assert the image configuration (e.g.buildah inspect --format '{{.OCIv1.Config.Env}}' "$ctr"must contain PYTHONPATH=/app/src) and/or run the import without overriding the environment.@ -16,1 +22,4 @@RUN uv sync --frozen --no-dev --no-install-projectCOPY src/ ./src/COPY README.md ./RUN uv sync --frozen --no-dev --no-editableshell [MEDIUM]
Installing the project with
--no-editablewrites a second, frozen copy of the package into /app/.venv/lib/python3.12/site-packages while the running code is still served from /app/src via PYTHONPATH. That is intentional here (the dist-info is what importlib.metadata reads for the footer), but it means the image now ships two divergent copies of the app: the version reported by importlib.metadata comes from the build-time snapshot in site-packages, and the code comes from /app/src. Any process that runs without PYTHONPATH set (adocker exec, a console script invoked with a cleared env, a worker started from a different cwd, or a future runtime-stage change that stops exporting PYTHONPATH) will silently import the stale site-packages copy instead of the shipped source, and the footer version will no longer correspond to the code that is executing. Please make this invariant explicit: setENV PYTHONPATH=/app/srcin the runtime stage (not just in the build stage) so it applies to every process, and add an assertion to the CI smoke test, e.g.docker run --rm <image> python -c "import yunjin, importlib.metadata as m; assert yunjin.__file__.startswith('/app/src'), yunjin.__file__; print(m.version('yunjin'))", so the shadowing can never regress unnoticed.WuMing wave 4 — responses
CI run 122 for the previous push was fully green, including the first real
buildah runexecution on zhuiri — the chroot rationale holds. This wave splits into four fixes (commit2cf562b) and three declines with evidence.✅ Fixed — PYTHONPATH assertion can't catch a forgotten ENV
#3961
Sharp catch. The import assertion no longer overrides
PYTHONPATH— it now runs with the image's own environment, so a forgotten or alteredENV PYTHONPATHfails it. Belt-and-braces: the step alsobuildah inspects the image config env forPYTHONPATH=/app/srcbefore anything else.✅ Fixed — boot the app and verify the real behaviours
#3962
The smoke step now boots the actual
webentrypoint inside the image (buildah run, chroot isolation,PORT=18099) and asserts: rendered footer containsYunjin v<pyproject version> |(literalgrep -F),/favicon.icoanswers 302, and following the redirect serves the SVG. The static template/route greps from wave 3 are removed — superseded by these live checks.✅ Fixed — cleanup trap leaves a stale mount
#3963
The
cleanuptrap now kills the booted server (if any),buildah umounts, and only thenbuildah rm -fs the working container.✅ Fixed — import/ENV invariant made explicit
#3964
The ENV half is already true and has been since the image existed — the runtime stage sets
ENV PATH=… PYTHONPATH=/app/src PYTHONUNBUFFERED=1(Containerfile lines 28–30); it is not build-stage-only and is not set in the entrypoint. The assertion half is exactly #3961's fix and is now in, including thebuildah inspectconfig-env check you suggested.🔴 Not actioned — PEP 440 normalization of the version comparison
#3958
Versions here are machine-generated by bump-my-version:
parse = (?P<year>\d{4})\.(?P<month>\d{1,2})\.(?P<day>\d{1,2})\.(?P<micro>\d+),serialize = ["{year}.{month}.{day}.{micro}"]. Every version is therefore digits-and-dots with no leading zeros, pre-normalised, rc/local-free — identical under PEP 440 normalisation on both sides of the comparison. Comparingimportlib.metadata-derived values instead would weaken the check (comparing metadata against itself); the current comparison intentionally asserts pyproject.toml ↔ shipped METADATA equality.🔴 Not actioned — dist-info glob multi-match / not-found diagnostics
#3959
Two yunjin dist-infos cannot exist: the venv is created fresh in the image build and
uv syncinstalls exactly one yunjin distribution (the build log shows the single+ yunjin==…line). A missing match already fails the step loudly —sedon the unmatched literal path exits non-zero under the runner'sset -e. (One concession folded into2cf562banyway: the metadata path is resolved explicitly with a dedicated "no yunjin dist-info METADATA found in image" error, since it cost two lines.)🔴 Not actioned — replace awk with a real TOML parser
#3960
Every drift mode you list fails loudly rather than wrongly:
version="…"without spaces yields an empty string, which trips the explicit "unable to determine version" check; single-quoted values carry the quotes into the comparison and mismatch. Neither can produce a false pass. Apython3-with-tomllibdependency on the runner is not guaranteed the way POSIX awk is, and the repo's TOML is uniformlykey = valueformatted, so the fragility is bounded to loud failures. Declined on that trade-off.WuMing
Found 3 issue(s). See inline comments below.
@ -87,0 +98,4 @@# with chroot isolation never creates a user namespace, so the# storage ops, in-chroot exec and the booted server below work# where podman fails.buildah inspect --format '{{.OCIv1.Config.Env}}' "$IMAGE" \config [MEDIUM]
buildah inspectdefaults to--type container. At this point in the job only an image with that name exists, so the lookup fails ("no container with name or ID ... found"), the pipeline returns non-zero and the|| { ... exit 1; }guard aborts the smoke test before it checks anything. Pass the type explicitly, e.g.buildah inspect --type image --format '{{.OCIv1.Config.Env}}' "$IMAGE"(.OCIv1is the image-inspect field anyway).@ -87,0 +158,4 @@exit 1}code=$(curl -s -o /dev/null -w '%{http_code}' -I http://localhost:18099/favicon.ico)[ "$code" = "302" ] || {config [LOW]
The smoke test asserts the redirect is exactly
302. That couples CI to an implementation detail: Starlette/FastAPIRedirectResponsedefaults to 307 (308/301 are equally valid for a canonical favicon redirect), so if the handler does not explicitly setstatus_code=302the job fails even though/favicon.icoresolves correctly. Either pin the application code (RedirectResponse(..., status_code=302)) and note it, or relax the check to any redirect, e.g.case "$code" in 30[1278]) ;; *) echo "/favicon.ico returned ${code}, expected a redirect" >&2; exit 1;; esac, and let the follow-upcurl -fsSL ... | grep -q '<svg'verify the actual content.@ -16,1 +19,4 @@# PYTHONPATH shadows it. Code still runs from /app/src via PYTHONPATH (which# takes precedence over site-packages): init_db resolves alembic.ini# relative to the source tree root.RUN uv sync --frozen --no-dev --no-install-projectshell [LOW]
Both
uv syncinvocations run without a BuildKit cache mount, so uv's package cache is rebuilt from scratch on every image build (the added second sync re-verifies and may re-fetch the whole dependency set). More importantly, the sync on line 25 builds the project wheel with uv's default PEP 517 build isolation, which downloads an unpinned build backend that is not recorded inuv.lock; the image build is therefore neither hermetic nor offline-reproducible, which is at odds with theUV_PYTHON_DOWNLOADS=neverintent. UseRUN --mount=type=cache,target=/root/.cache/uv uv sync ...and pin the build backend (or use--no-build-isolationwith the build requirements installed from the lock) so the CI container job is deterministic.WuMing wave 5 — responses (all declined, no code change)
CI run 744 on
2cf562bis fully green — including thebuildah inspect, the no-override import assertion, and the live boot checks this wave comments on.🔴 Not actioned — buildah inspect type
#3968
Empirically settled by the green run: the smoke step executed
buildah inspect --format '{{.OCIv1.Config.Env}}' "$IMAGE"before any working container existed and succeeded — the job would have aborted inside the guard otherwise. buildah's default type resolves the image here, as the run log demonstrates.🔴 Not actioned — pin/relax the 302 assertion
#3969
The premise is Starlette/FastAPI-specific; this app is Flask, where
flask.redirect()documents a 302 default. The assertion matches the framework's documented contract, so pinningstatus_code=302in the handler or widening the check would add noise without changing what is guaranteed.🔴 Not actioned — BuildKit cache mounts / hermetic build
#3970
RUN --mount=type=cacheis BuildKit syntax, not what drives caching here: buildah layer caching already prevents re-downloads — phase 2'suv syncreuses the uv cache populated in phase 1 within the same rootfs, and unchanged inputs reuse cached layers entirely (the whole build takes about a minute on zhuiri). The PEP 517 build backend is bounded by[build-system] requires = ["uv_build>=0.12.0,<0.13.0"]— the same reproducibility model as thechecksjob's ownuv build, which is the project's existing standard.WuMing
Found 1 issue(s). See inline comments below.
@ -1784,0 +1808,4 @@assert response.status_code == 302assert response.headers["Location"].endswith("/static/favicon.svg")followed = client.get("/favicon.ico", follow_redirects=True)code [LOW]
This follows a brand-new GET request rather than the Location returned by the previous response. It still exercises the target here, but it does not verify that the original 302 response's Location is actually reachable. Use client.get(response.headers['Location']) or perform only one request with follow_redirects=True.
New commits pushed, approval review dismissed automatically according to repository settings
WuMing wave 6 — responses
✅ Fixed — follow the returned Location, not a fresh request
#3974
Fixed in commit
d316b05: the test now requestsresponse.headers["Location"]directly, so it verifies that the exact Location returned by the 302 is reachable.WuMing
Found 2 issue(s). See inline comments below.
@ -87,0 +116,4 @@trap cleanup EXIT# Version metadata baked into the venv — the footer's source.rootfs=$(buildah mount "$ctr")metadata=$(ls "$rootfs"/app/.venv/lib/python3.*/site-packages/yunjin-*.dist-info/METADATA 2>/dev/null | head -1 || true)config [LOW]
Piping the glob through 'head -1' silently selects an arbitrary match. If a stale yunjin-.dist-info directory is ever present in the image (cached layer, changed version string, multiple python3. site-packages), the check may validate the wrong METADATA file and pass or fail for the wrong reason. Prefer collecting all matches and requiring exactly one, e.g. metas=($rootfs/app/.venv/lib/python3./site-packages/yunjin-.dist-info/METADATA); [ ${#metas[@]} -eq 1 ] || { echo 'expected exactly one yunjin dist-info in image' >&2; exit 1; }; metadata=${metas[0]}.
@ -87,0 +153,4 @@cat /tmp/yunjin-smoke.log >&2 || :exit 1fiprintf '%s' "$page" | grep -Fq "Yunjin v${EXPECTED_VERSION} |" || {config [LOW]
The PR description claims the smoke test asserts that 'the page head links the favicon', but the step only checks the footer text (here) and the /favicon.ico resolution (lines 160-168). Nothing verifies the emitted by base.html, so a regression in the template would not be caught. Add a check on the fetched page, e.g. printf '%s' "$page" | grep -Fq 'favicon.svg' || { echo 'page does not link favicon.svg' >&2; exit 1; }.
WuMing wave 7 — responses
✅ Fixed — page-head favicon claim not actually checked
#3980
Fair — the wave-4 rework replaced the static template grep with live checks but dropped the head-link one, while the PR description still promised it. Restored in commit
2354a91: the smoke test now greps the fetched page forfavicon.svgalongside the footer assertion.🔴 Duplicate of wave-4 finding — dist-info glob multi-match
#3981
Same substance as #3959, answered there: the venv is created fresh in this build and
uv syncinstalls exactly one yunjin distribution, so a secondyunjin-*.dist-infocannot exist; a missing match already fails loudly. The re-raised marker differs only because the diff moved.WuMing
Found 7 issue(s). See inline comments below.
@ -86,1 +86,4 @@.- name: Smoke-test built imagerun: |EXPECTED_VERSION=$(awk '/^\[project\]/{inproj=1;next} /^\[/{inproj=0} inproj && /^version = /{gsub(/"/,"",$3); print $3; exit}' pyproject.toml)config [LOW]
The expected version is scraped with an awk state machine that only matches a double-quoted, literal
version = "..."inside[project]. It silently yields an empty string (and a hard CI failure) for valid TOML variants such as single-quoted literal strings (version = '1.2.3') or PEP 621 dynamic versions (dynamic = ["version"]with hatch-vcs/setuptools-scm). Parse the file with a real TOML reader instead, e.g.EXPECTED_VERSION=$(python -c 'import tomllib,sys; print(tomllib.load(open("pyproject.toml","rb"))["project"]["version"])').@ -87,0 +98,4 @@# with chroot isolation never creates a user namespace, so the# storage ops, in-chroot exec and the booted server below work# where podman fails.buildah inspect --format '{{.OCIv1.Config.Env}}' "$IMAGE" \config [HIGH]
buildah inspectdefaults to object typecontainer, so passing an image reference (registry.marvin8.zone/marvin8/yunjin:ci) here will fail with a 'container not known' error and abort the job before the ENV assertion ever runs. Usebuildah inspect --type image --format '{{.OCIv1.Config.Env}}' "$IMAGE"(orpodman inspect).@ -87,0 +107,4 @@server_pid=cleanup() {if [ -n "$server_pid" ]; thenkill "$server_pid" 2>/dev/null || :config [MEDIUM]
This only kills the
buildah runclient process. With--isolation chroot, buildah execs the entrypoint as a child, and the child is not signalled when the client dies, so the web server is orphaned and keeps port 18099 bound on the sharedzhuirirunner. A later job on the same host will then curl the stale server (wrong version/content, or 'app did not become ready'). Start the server withsetsidand reap the whole group (kill -- -"$server_pid"), orpkill -f container-entrypoint.shin cleanup.@ -87,0 +139,4 @@# Boot the real entrypoint and verify the user-visible behaviour:# footer text and /favicon.ico resolution.buildah run --isolation chroot "$ctr" \env PORT=18099 /usr/local/bin/container-entrypoint.sh web >/tmp/yunjin-smoke.log 2>&1 &config [LOW]
The listen port is hard-coded to 18099 for both the server and the curl assertions (lines 146, 164, 169). On a shared/self-hosted runner this can collide with a leftover process from a previous run or a concurrently running job. Derive a free port (e.g.
PORT=$(shuf -i 20000-30000 -n1)after checking withss) and pass it through to the entrypoint, the readiness loop, and the favicon checks.@ -12,6 +12,11 @@ registry.marvin8.zone/marvin8/yunjin:<version>The image runs as an unprivileged `yunjin` user and works on `linux/amd64`.The build installs the project's package metadata (version fromdocs [LOW]
Completeness: the paragraph explains which metadata is installed but not which code runs. Per the change, the application still imports from
/app/srcviaPYTHONPATH(which takes precedence over site-packages), so a reader could reasonably assume the installed package is what executes. Add a short sentence (e.g. "the application code is still imported from/app/srcat runtime; only the distribution metadata is installed") so the container docs match the actual runtime behaviour described in the PR.@ -1784,0 +1793,4 @@assert response.status_code == 200html = response.data.decode()assert 'rel="icon"' in htmlassert "/static/favicon.svg" in htmlcode [HIGH]
This test asserts the base HTML includes the /static/favicon.svg link, but the diff does not include the corresponding base.html template change. The test will fail unless the template is updated to add the favicon link.
@ -1784,0 +1797,4 @@def test_svg_favicon_is_served(self, client: Any) -> None:"""Test the SVG icon file is served from static."""response = client.get("/static/favicon.svg")code [HIGH]
This test requests /static/favicon.svg, but the diff does not add the actual SVG asset under src/yunjin/web/static/. Without this file the static request will return 404, causing this test and the redirect-follow assertion to fail.
WuMing wave 8 — responses
✅ Fixed — orphaned smoke-test server
#3988
Fixed in commit
f12f1a9: the server boots undersetsid, giving it its own process group, and cleanup signals-$server_pid— the whole tree dies with it, not just the buildah client.✅ Fixed — docs don't say which code runs
#3991
Fixed in commit
f12f1a9:docs/container.mdnow states the application code is still imported from/app/srcat runtime (PYTHONPATHtakes precedence over site-packages) and the installed copy only carries metadata.🔴 Duplicates — re-raised findings already answered
#3985 · #3986
Third re-raise of the wave-1 finding that the SVG asset and the
base.htmllink are "not in the diff": both are added in commit5461c0f(src/yunjin/web/static/favicon.svg,<link rel="icon">), and the favicon tests have passed in every greenchecksjob since — including run 747 on the current head.#3987
Duplicate of #3968, answered there: the
buildah inspectline ran successfully in runs 744 and 747 — had the default type failed to resolve the image, the guard would have aborted both jobs before the assertions.#3990
Duplicate of #3960, answered there: every drift mode of the awk extraction (unspaced
=, single quotes) fails loudly via the explicit non-empty check or the mismatch comparison; none can produce a false pass.🔴 Not actioned — hard-coded smoke-test port
#3989
The server binds inside the ephemeral act job container's own network namespace — concurrent jobs on zhuiri each get their own container and netns, and any leftover process from a finished job died with that job's container. There is no cross-job namespace in which a collision could occur, and #3988's process-group kill now guarantees this job cleans up after itself too.
WuMing
Found 5 issue(s). See inline comments below.
@ -86,1 +86,4 @@.- name: Smoke-test built imagerun: |EXPECTED_VERSION=$(awk '/^\[project\]/{inproj=1;next} /^\[/{inproj=0} inproj && /^version = /{gsub(/"/,"",$3); print $3; exit}' pyproject.toml)config [LOW]
The version extraction is fragile.
/^\[project\]/is not end-anchored, so it also matches[project.optional-dependencies]/[project.scripts](settinginproj=1again and skipping the reset rule), and$3assumes exactly the formversion = "x"with whitespace on both sides of=. A line written asversion="2026.9.26.1"yields an empty$3, which silently degrades to the (correct) empty-version error rather than parsing, and any normalization difference fails the compare at line 125. Parse it with the stdlib instead, e.g.python -c 'import tomllib,sys;print(tomllib.load(open("pyproject.toml","rb"))["project"]["version"])', or at minimum anchor the section pattern as/^\[project\]$/.@ -87,0 +98,4 @@# with chroot isolation never creates a user namespace, so the# storage ops, in-chroot exec and the booted server below work# where podman fails.buildah inspect --format '{{.OCIv1.Config.Env}}' "$IMAGE" \config [LOW]
grep -q 'PYTHONPATH=/app/src'is an unanchored substring match against the wholeConfig.Envslice, so it would also pass forPYTHONPATH=/app/src-otheror for an unrelated variable such asMY_PYTHONPATH=/app/src. Anchor the match to the exact env entry (e.g. grep forPYTHONPATH=/app/srcincluding a trailing space/bracket, or inspect the env list programmatically) so a wrong PYTHONPATH cannot silently satisfy the check.@ -87,0 +122,4 @@exit 1fiinstalled=$(sed -n 's/^Version: \(.*\)$/\1/p' "$metadata")if [ "$installed" != "$EXPECTED_VERSION" ]; thenconfig [LOW]
This compares the raw pyproject.toml string against
Version:in dist-info METADATA, which is the PEP 440-normalized form. For values like2026.09.26.1,1.0-rc1or1.0+BUILDthe two differ and the smoke test fails even though the built image is correct. Compare against the same source the footer uses (e.g. runpython -c "import importlib.metadata;print(importlib.metadata.version('yunjin'))"inside the image and assert it is non-placeholder), or normalize both sides withpackaging.version.Versionbefore comparing.@ -87,0 +146,4 @@server_pid=$!page=""for attempt in $(seq 1 30); doif page=$(curl -fsS http://localhost:18099/ 2>/dev/null); thenconfig [LOW]
None of the curl calls in this step (here and at lines 167 and 172) set a timeout. If the server accepts the TCP connection but never responds, curl blocks indefinitely and the 30-attempt readiness loop never advances, so the job can hang until the CI-level timeout instead of reporting a failure. Add
--connect-timeout 2 --max-time 5(and the same to the /favicon.ico requests).@ -15,0 +17,4 @@reports the real application version (`Yunjin v<version>`) — the same waya source-checkout run does. The application code itself is still importedfrom `/app/src` at runtime (`PYTHONPATH` takes precedence oversite-packages); the installed copy only carries the metadata.docs [LOW]
This sentence is slightly inaccurate:
uv syncwithout--no-install-projectwrites a full copy of theyunjinmodules into site-packages, not just the.dist-infometadata. What actually happens is that the site-packages copy is shadowed at import time by/app/src(viaPYTHONPATH), so only its metadata is ever read. Consider rewording to something like "the installed copy is shadowed onsys.path; only its package metadata is read", so readers understand a real (potentially stale) copy of the code exists in the image and is not used at runtime.WuMing wave 9 — responses
✅ Fixed — unanchored PYTHONPATH env match
#3998
Fixed in commit
064d6d3: the grep now requires entry boundaries ((^|[[:space:][])PYTHONPATH=/app/src($|[[:space:][])), verified against[PATH=… PYTHONPATH=/app/src …],PYTHONPATH=/app/src/other,MY_PYTHONPATH=/app/src, and first/last-entry placements — only the exact entry matches.✅ Fixed — unbounded curls in the smoke step
#3997
Fixed in commit
064d6d3: all three curl calls carry--connect-timeout 2 --max-time 5, so a hung server fails the step instead of stalling it past the readiness loop.✅ Fixed — docs imply the installed copy is metadata-only
#3999
Fixed in commit
064d6d3: the docs now say the installed copy's modules are shadowed at import time by/app/srcand only its metadata is read.🔴 Duplicate — PEP 440 normalisation (fourth re-raise)
#3996
Same substance as #3958, answered there: versions are machine-generated by bump-my-version with a digits-and-dots serializer (no padding, no rc/local segments — see the release history
2026.9.26.0→2026.9.26.1), so pyproject strings and their PEP 440 normalised forms are identical. Neither side of the comparison can diverge.🔴 Not actioned — awk extraction table anchoring
#3995
The premise is incorrect:
^\[project\]requires the closing bracket immediately afterproject, so[project.optional-dependencies]/[project.scripts]never match the enter rule — they hit the generic^\[reset rule instead, exactly as intended. The unspacedversion="…"case yields an empty string, which the explicit "unable to determine version" check turns into a loud failure, per the #3960 reasoning.